[alsa-devel] [PATCH v2] ASoC: Add FreeScale SGTL5000 codec suppor - Alsa-devel - mailman.alsa-project.org

newer
Re: [alsa-devel] [PATCH 0/5]...

[alsa-devel] [PATCH v2] ASoC: Add FreeScale SGTL5000 codec suppor

older
Re: [alsa-devel] [PATCH] ASoC:...

Zeng Zhaoming

17 Jan 2011 17 Jan '11

7:48 a.m.

v1 url: http://mailman.alsa-project.org/pipermail/alsa-devel/2010-December/034615.ht...

Changes since v1: - Add DAPM widges as Mark and Arnaud point out. - cleanup regulator code - new implement dai_ops functions - sort configs in Kconfig and Makefile

Reply

Show replies by date

Mark Brown

17 Jan 17 Jan

4:32 p.m.

New subject: [alsa-devel] [PATCH v2] ASoC: Add FreeScale SGTL5000 codec suppor

On Mon, Jan 17, 2011 at 02:48:54PM +0800, Zeng Zhaoming wrote:

v1 url: http://mailman.alsa-project.org/pipermail/alsa-devel/2010-December/034615.ht...

Changes since v1:

Add DAPM widges as Mark and Arnaud point out.

cleanup regulator code

new implement dai_ops functions

sort configs in Kconfig and Makefile

From 1704fd5e7cf2540e247aa8a4f69d81ae5e6cbd4e Mon Sep 17 00:00:00 2001 From: Zeng Zhaoming zengzm.kernel@gmail.com Date: Mon, 17 Jan 2011 10:34:42 +0800

Please submit patches in the form documented in SubmittingPatches. In particular please don't send as an attachement, particularly not with extraneous stuff before the changelog.

+struct sgtl5000_priv {

int sysclk; /* sysclk rate */

int master; /* i2s master or not? */

int fmt; /* i2s data format */

int lrclk; /* frame clock rate */

int capture_channels; /* the num of channels for capture. */

int small_pop; /* hw assistant to eliminate pop */

It's not clear why you're storing some of this stuff in the private data - for example, capture_channels is only referred to in the place where it is set (so you may as well use the original value) and small_pop is set unconditionally (it looks like the intention was to have platform data for it?).

+static int mic_bias_event(struct snd_soc_dapm_widget *w,

struct snd_kcontrol *kcontrol, int event)

+{

/* change mic bias resistor to 4Kohm */

snd_soc_write(w->codec, SGTL5000_CHIP_MIC_CTRL, SGTL5000_BIAS_R_4k);

return 0;

+}

This seems really odd - this does the write unconditionally regardless of event and never turns anything off. If this can't just be set once at startup then a comment explaining why an event is needed would be good.

+static int dac_event(struct snd_soc_dapm_widget *w,

struct snd_kcontrol *kcontrol, int event)

+{
switch (event) {

case SND_SOC_DAPM_PRE_PMU:
snd_soc_update_bits(w->codec, SGTL5000_CHIP_ANA_POWER,
	SGTL5000_VAG_POWERUP|SGTL5000_DAC_POWERUP,
	SGTL5000_VAG_POWERUP|SGTL5000_DAC_POWERUP);
break;
case SND_SOC_DAPM_POST_PMD:
snd_soc_update_bits(w->codec, SGTL5000_CHIP_ANA_POWER,
	SGTL5000_DAC_POWERUP, 0);
break;

Why does the powerdown not disable VAG_POWERUP? My first thought was that VAG_POWERUP ought to be a supply widget...

+static int adc_event(struct snd_soc_dapm_widget *w,

struct snd_kcontrol *kcontrol, int event)

+{
snd_soc_update_bits(w->codec, SGTL5000_CHIP_ANA_POWER,
SGTL5000_DAC_POWERUP,
SGTL5000_DAC_POWERUP);

Again, this is really unclear.

+/* custom function to get of PCM playback volume */ +static int dac_get_volsw(struct snd_kcontrol *kcontrol,
	 struct snd_ctl_elem_value *ucontrol)
+{

struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);

int reg, l, r;

reg = snd_soc_read(codec, SGTL5000_CHIP_DAC_VOL);

l = (reg & SGTL5000_DAC_VOL_LEFT_MASK) >> SGTL5000_DAC_VOL_LEFT_SHIFT;

r = (reg & SGTL5000_DAC_VOL_RIGHT_MASK) >> SGTL5000_DAC_VOL_RIGHT_SHIFT;

l = l < 0x3c ? 0x3c : l;

l = l > 0xfc ? 0xfc : l;

r = r < 0x3c ? 0x3c : r;

r = r > 0xfc ? 0xfc : r;

/* revert it */

l = 0xfc - l;

r = 0xfc - r;

This is really unclear. The lack of any comments and use of the ternery operator isn't helping... I'd suggest describing the register semantics in a comment.

It also looks like you're missing some locking.

l = ucontrol->value.integer.value[0];

r = ucontrol->value.integer.value[1];

l = l < 0 ? 0 : l;

l = l > 0xfc - 0x3c ? 0xfc - 0x3c : l;

r = r < 0 ? 0 : r;

r = r > 0xfc - 0x3c ? 0xfc - 0x3c : r;

l = 0xfc - l;

r = 0xfc - r;

Likewise.

/* we need SOC_DOUBLE_S8_TLV with invert */
{
```
.iface = SNDRV_CTL_ELEM_IFACE_MIXER,
```
```
.name = "PCM Playback Volume",
```

.access = SNDRV_CTL_ELEM_ACCESS_TLV_READ |

```
	SNDRV_CTL_ELEM_ACCESS_READWRITE,
```
```
.info = dac_info_volsw,
```
```
.get = dac_get_volsw,
```
```
.put = dac_put_volsw,
```
},

Perhaps implementing the abstract type would help with the clarity issues?

case SND_SOC_DAIFMT_CBM_CFS:

case SND_SOC_DAIFMT_CBS_CFM:
return -EINVAL;
break;
default:
return -EINVAL;
}

Could just have the default: case.

if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
```
sgtl5000->capture_channels = channels;
```

reg = snd_soc_read(codec, SGTL5000_CHIP_ANA_POWER);

```
reg |= SGTL5000_ADC_POWERUP;
```
```
if (sgtl5000->capture_channels == 1)
```
```
	reg &= ~SGTL5000_ADC_STEREO;
```
```
else
```
```
	reg |= SGTL5000_ADC_STEREO;
```

snd_soc_write(codec, SGTL5000_CHIP_ANA_POWER, reg);

}

snd_soc_update_bits().

/* get devided factor of frame clock */

switch (sys_fs / sgtl5000->lrclk) {

case 4:
clk_ctl |= SGTL5000_RATE_MODE_DIV_4 << SGTL5000_RATE_MODE_SHIFT;
break;
case 2:
clk_ctl |= SGTL5000_RATE_MODE_DIV_2 << SGTL5000_RATE_MODE_SHIFT;
break;
default:
break;
}

I'd expect to see this error out if it can't find an appropriate divider?

if ((clk_ctl & SGTL5000_MCLK_FREQ_MASK) == SGTL5000_MCLK_FREQ_PLL) {
snd_soc_write(codec, SGTL5000_CHIP_PLL_CTRL, pll_ctl);

This looks very odd - why has pll_ctrl been precomputed if it's only going to be used here?

+/*

set dac bias

common state changes:

startup:

off --> standby --> prepare --> on

standby --> prepare --> on

stop:

on --> prepare --> standby

*/

+static int sgtl5000_set_bias_level(struct snd_soc_codec *codec,
		   enum snd_soc_bias_level level)
+{

codec->bias_level = level;

return 0;

+}

May as well remove this, it does nothing.

+#define SGTL5000_FORMATS (SNDRV_PCM_FMTBIT_S16_LE |\
	SNDRV_PCM_FMTBIT_S20_3LE |\
	SNDRV_PCM_FMTBIT_S24_LE)

Your hw_params() claimed 32 bit support too.

/* setup i2c data ops */

ret = snd_soc_codec_set_cache_io(codec, 16, 16, SND_SOC_I2C);

if (ret < 0) {
dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret);
return ret;
}

Your resume code might get a bit clearer if you were able to use the standard cache sync functionality that the generic register cache code has.

/* get and enable all regulators */

for (i = 0; i < SGTL5000_SUPPLY_NUM; i++) {
struct regulator *reg;
reg = regulator_get(codec->dev, supply_names[i]);
if (IS_ERR(reg))
	continue;
regulator_enable(reg);
sgtl5000->supplies[i] = reg;
}

/* vdda and vddio regulator must configed */

if (!sgtl5000->supplies[VDDA] || !sgtl5000->supplies[VDDIO]) {
dev_err(codec->dev,
	"Not set vdda or vddio regulator correctly\n");
/* FIXME, no such platform regulator configed */
return -ENODEV;
}

This looks rather wrong; you're not clearing up any regulators you did get, you're not logging any errors if you fail to get regulators and it's *very* unusual to see any supplies being optional. You certainly need comments here.

vdda = regulator_get_voltage(sgtl5000->supplies[VDDA]) / 1000;

vddio = regulator_get_voltage(sgtl5000->supplies[VDDIO]) / 1000;

if (regulator_get_voltage(sgtl5000->supplies[VDDD]))
vddd = regulator_get_voltage(sgtl5000->supplies[VDDD]) / 1000;
else
vddd = 0;
/* workaround for rev 0x11: use vddd linear regulator */

if (!vddd || (rev >= 0x11)) {
/* set VDDD to 1.2v */
lreg_ctrl |= 0x8 << SGTL5000_LINREG_VDDD_SHIFT;
/* power internal linear regulator */
ana_pwr |= SGTL5000_LINEREG_D_POWERUP;
} else {
/* turn of startup power */
ana_pwr &= ~SGTL5000_STARTUP_POWERUP;
ana_pwr &= ~SGTL5000_LINREG_SIMPLE_POWERUP;
}

If you have an internal LDO that can optinally be used it'd probably be better to have it visible via the regulator API. Also, if the internal regulator is in use surely an external supply should be left off?

/* set ADC/DAC ref voltage to vdda / 2 */

vag = vdda / 2;

if (vag <= SGTL5000_ANA_GND_BASE)
vag = 0;
else if (vag >= SGTL5000_ANA_GND_BASE + SGTL5000_ANA_GND_STP *
 (SGTL5000_ANA_GND_MASK >> SGTL5000_ANA_GND_SHIFT))
vag = SGTL5000_ANA_GND_MASK >> SGTL5000_ANA_GND_SHIFT;
else
vag = (vag - SGTL5000_ANA_GND_BASE) / SGTL5000_ANA_GND_STP;
/* set line out ref voltage to vddio / 2 */

vag = vddio / 2;

The above block of code calculates a value for vag but does nothing with it before it's overwritten here...

if (vag <= SGTL5000_LINE_OUT_GND_BASE)
vag = 0;
else if (vag >= SGTL5000_LINE_OUT_GND_BASE + SGTL5000_LINE_OUT_GND_STP *
 SGTL5000_LINE_OUT_GND_MAX)
vag = SGTL5000_LINE_OUT_GND_MAX;
else
vag = (vag - SGTL5000_LINE_OUT_GND_BASE) /
    SGTL5000_LINE_OUT_GND_STP;

...then there's no immediate use of this calculation either, instead we start doing...

snd_soc_write(codec, SGTL5000_CHIP_LINREG_CTRL, lreg_ctrl);

snd_soc_write(codec, SGTL5000_CHIP_ANA_POWER, ana_pwr);

msleep(10);

...this. It'd most likely be *much* clearer if the calculations and register writes were kept together.

+static const struct i2c_device_id sgtl5000_id[] = {

{"sgtl5000-codec", 0},

{},

+};

Remove the -codec, it's not at all idiomatic.

.driver = {
```
   .name = "sgtl5000-codec",
```
```
   .owner = THIS_MODULE,
```

Please remove it here too.

Reply

Zeng Zhaoming

18 Jan 18 Jan

3:32 a.m.

New subject: [alsa-devel] [PATCH v2] ASoC: Add FreeScale SGTL5000 codec suppor

On Mon 2011-01-17 15:32:11, Mark Brown wrote:

On Mon, Jan 17, 2011 at 02:48:54PM +0800, Zeng Zhaoming wrote:

...
v1 url: http://mailman.alsa-project.org/pipermail/alsa-devel/2010-December/034615.ht...

Changes since v1:

Add DAPM widges as Mark and Arnaud point out.

cleanup regulator code

new implement dai_ops functions

sort configs in Kconfig and Makefile

...
From 1704fd5e7cf2540e247aa8a4f69d81ae5e6cbd4e Mon Sep 17 00:00:00 2001 From: Zeng Zhaoming zengzm.kernel@gmail.com Date: Mon, 17 Jan 2011 10:34:42 +0800

Please submit patches in the form documented in SubmittingPatches. In particular please don't send as an attachement, particularly not with extraneous stuff before the changelog.

...
+struct sgtl5000_priv {
int sysclk;     /* sysclk rate */
int master;     /* i2s master or not? */
int fmt;        /* i2s data format */
int lrclk;      /* frame clock rate */
int capture_channels;   /* the num of channels for capture. */
int small_pop;  /* hw assistant to eliminate pop */
It's not clear why you're storing some of this stuff in the private data

for example, capture_channels is only referred to in the place where

it is set (so you may as well use the original value) and small_pop is set unconditionally (it looks like the intention was to have platform data for it?).

yes, you are right. lrclk and capture_channels fields can be remove. small_pop is a configurable option of platform data.

...
+static int mic_bias_event(struct snd_soc_dapm_widget *w,
struct snd_kcontrol *kcontrol, int event)
+{
/* change mic bias resistor to 4Kohm */
snd_soc_write(w->codec, SGTL5000_CHIP_MIC_CTRL, SGTL5000_BIAS_R_4k);
return 0;
+}
This seems really odd - this does the write unconditionally regardless of event and never turns anything off. If this can't just be set once at startup then a comment explaining why an event is needed would be good.

the odd code is cuased by: If this is set to zero the micbias block is powered off, and 4Kohm is common case for enabled micbias. 0x0 = Powered off 0x1 = 2Kohm 0x2 = 4Kohm 0x3 = 8Kohm when turn on micbias, it will set to 2Kohm, so we need to change it to 4Kohm after widget powerup.

+static int dac_event(struct snd_soc_dapm_widget *w,

struct snd_kcontrol *kcontrol, int event)

+{

```
switch (event) {
```
```
case SND_SOC_DAPM_PRE_PMU:
```

        snd_soc_update_bits(w->codec, SGTL5000_CHIP_ANA_POWER,

                SGTL5000_VAG_POWERUP|SGTL5000_DAC_POWERUP,

                SGTL5000_VAG_POWERUP|SGTL5000_DAC_POWERUP);

```
        break;
```
```
case SND_SOC_DAPM_POST_PMD:
```

        snd_soc_update_bits(w->codec, SGTL5000_CHIP_ANA_POWER,

                SGTL5000_DAC_POWERUP, 0);

```
        break;
```

Why does the powerdown not disable VAG_POWERUP? My first thought was that VAG_POWERUP ought to be a supply widget...

My first version, VAG_POWERUP is a supply widget, but spec says: The headphone (and/or lineout) powerup should be set BEFORE clearing this bit.

But the powerdown sequence go against it. So I turn to powerup VAG with DAC, and powerdown it before HP and Line_out.

+static int adc_event(struct snd_soc_dapm_widget *w,

struct snd_kcontrol *kcontrol, int event)

+{

snd_soc_update_bits(w->codec, SGTL5000_CHIP_ANA_POWER,

```
        SGTL5000_ADC_POWERUP,
```
```
        SGTL5000_ADC_POWERUP);
```

Again, this is really unclear.

ADC powerup be controlled by both CHIP_ANA_POWER and CHIP_DIG_POWER. It seems we need to set both of them.

...
+/* custom function to get of PCM playback volume */ +static int dac_get_volsw(struct snd_kcontrol *kcontrol,
                 struct snd_ctl_elem_value *ucontrol)
+{
struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
int reg, l, r;
reg = snd_soc_read(codec, SGTL5000_CHIP_DAC_VOL);
l = (reg & SGTL5000_DAC_VOL_LEFT_MASK) >> SGTL5000_DAC_VOL_LEFT_SHIFT;
r = (reg & SGTL5000_DAC_VOL_RIGHT_MASK) >> SGTL5000_DAC_VOL_RIGHT_SHIFT;
l = l < 0x3c ? 0x3c : l;
l = l > 0xfc ? 0xfc : l;
r = r < 0x3c ? 0x3c : r;
r = r > 0xfc ? 0xfc : r;
/* revert it */
l = 0xfc - l;
r = 0xfc - r;
This is really unclear. The lack of any comments and use of the ternery operator isn't helping... I'd suggest describing the register semantics in a comment.

Ok, I will add more useful comment for this. spec says: Set the Right channel DAC volume with 0.5017 dB steps from 0 to -90 dB 0x3B and less = Reserved 0x3C = 0 dB 0x3D = -0.5 dB 0xF0 = -90 dB 0xFC and greater = Muted

It also looks like you're missing some locking.

```
l = ucontrol->value.integer.value[0];
```
```
r = ucontrol->value.integer.value[1];
```
```
l = l < 0 ? 0 : l;
```
```
l = l > 0xfc - 0x3c ? 0xfc - 0x3c : l;
```
```
r = r < 0 ? 0 : r;
```
```
r = r > 0xfc - 0x3c ? 0xfc - 0x3c : r;
```
```
l = 0xfc - l;
```
```
r = 0xfc - r;
```

Likewise.

/* we need SOC_DOUBLE_S8_TLV with invert */

```
{
```

        .iface = SNDRV_CTL_ELEM_IFACE_MIXER,

```
        .name = "PCM Playback Volume",
```

        .access = SNDRV_CTL_ELEM_ACCESS_TLV_READ |

                SNDRV_CTL_ELEM_ACCESS_READWRITE,

```
        .info = dac_info_volsw,
```
```
        .get = dac_get_volsw,
```
```
        .put = dac_put_volsw,
```
```
},
```

Perhaps implementing the abstract type would help with the clarity issues?

```
case SND_SOC_DAIFMT_CBM_CFS:
```
```
case SND_SOC_DAIFMT_CBS_CFM:
```
```
        return -EINVAL;
```
```
        break;
```
```
default:
```
```
        return -EINVAL;
```
```
}
```

Could just have the default: case.

if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {

        sgtl5000->capture_channels = channels;

        reg = snd_soc_read(codec, SGTL5000_CHIP_ANA_POWER);

```
        reg |= SGTL5000_ADC_POWERUP;
```

        if (sgtl5000->capture_channels == 1)

                reg &= ~SGTL5000_ADC_STEREO;

```
        else
```

                reg |= SGTL5000_ADC_STEREO;

        snd_soc_write(codec, SGTL5000_CHIP_ANA_POWER, reg);

```
}
```

snd_soc_update_bits().

/* get devided factor of frame clock */

```
switch (sys_fs / sgtl5000->lrclk) {
```
```
case 4:
```

        clk_ctl |= SGTL5000_RATE_MODE_DIV_4 << SGTL5000_RATE_MODE_SHIFT;

```
        break;
```
```
case 2:
```

        clk_ctl |= SGTL5000_RATE_MODE_DIV_2 << SGTL5000_RATE_MODE_SHIFT;

```
        break;
```
```
default:
```
```
        break;
```
```
}
```

I'd expect to see this error out if it can't find an appropriate divider?

the default means sys_fs / lrclk = 1, not a error.

Reply

Mark Brown

12:02 p.m.

New subject: [alsa-devel] [PATCH v2] ASoC: Add FreeScale SGTL5000 codec suppor

On Tue, Jan 18, 2011 at 10:32:55AM +0800, Zeng Zhaoming wrote:

On Mon 2011-01-17 15:32:11, Mark Brown wrote:

...
It's not clear why you're storing some of this stuff in the private data

for example, capture_channels is only referred to in the place where

it is set (so you may as well use the original value) and small_pop is set unconditionally (it looks like the intention was to have platform data for it?).

yes, you are right. lrclk and capture_channels fields can be remove. small_pop is a configurable option of platform data.

Your patch unconditionally sets small_pop, there's no configuration via platform data or otherwise.

...
of event and never turns anything off. If this can't just be set once at startup then a comment explaining why an event is needed would be good.

the odd code is cuased by:

Please note what I'm saying about making the code clear.

```
case SND_SOC_DAPM_PRE_PMU:
```

        snd_soc_update_bits(w->codec, SGTL5000_CHIP_ANA_POWER,

                SGTL5000_VAG_POWERUP|SGTL5000_DAC_POWERUP,

                SGTL5000_VAG_POWERUP|SGTL5000_DAC_POWERUP);

```
        break;
```
```
case SND_SOC_DAPM_POST_PMD:
```

        snd_soc_update_bits(w->codec, SGTL5000_CHIP_ANA_POWER,

                SGTL5000_DAC_POWERUP, 0);

```
        break;
```

...
Why does the powerdown not disable VAG_POWERUP? My first thought was that VAG_POWERUP ought to be a supply widget...

My first version, VAG_POWERUP is a supply widget, but spec says: The headphone (and/or lineout) powerup should be set BEFORE clearing this bit.

But the powerdown sequence go against it. So I turn to powerup VAG with DAC, and powerdown it before HP and Line_out.

The above doesn't seem to tie in with your code terribly well. A frequent issue throughout this code is that you're doing a bunch of unusual stuff and it's really not clear what the code is supposed to do, please work to make the code more comprehensible. This is a problem both from a legibility point of view and from the point of view of being likely to break if the core changes.

The same issue applies to a lot of the other concerns I raised, for brevity I've cut most of the discussion - in general the major issue with the code is that it's really hard to read.

```
switch (sys_fs / sgtl5000->lrclk) {
```
```
case 4:
```

        clk_ctl |= SGTL5000_RATE_MODE_DIV_4 << SGTL5000_RATE_MODE_SHIFT;

```
        break;
```
```
case 2:
```

        clk_ctl |= SGTL5000_RATE_MODE_DIV_2 << SGTL5000_RATE_MODE_SHIFT;

```
        break;
```
```
default:
```
```
        break;
```
```
}
```

...
I'd expect to see this error out if it can't find an appropriate divider?

the default means sys_fs / lrclk = 1, not a error.

So what happens when sys_fs / lrclk is not 1? If nothing else the above code needs to be *much* clearer.

Reply

5088

Age (days ago)

5089

Last active (days ago)

Download

3 comments

3 participants

tags (0)

participants (3)

Mark Brown
Zeng Zhaoming
Zeng Zhaoming