Re: [PATCH] ASoC: add RT5640 CODEC driver

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2822 bytes --]

On Fri, May 31, 2013 at 03:04:59PM +0800, bardliao@realtek.com wrote:

> From: Bard Liao <bardliao@realtek.com>

This looks pretty good, a few small things below and probably one actual
bug.

> 
> * Use regmap_range_cfg to replace index read/write function.
> * Remove I2S3 related code since there is no I2S3 in ALC5640.
> * Remove Voice DSP related code since there is no Voice DSP in ALC5640.
> * Remove MICBIAS2 since there is no MICBIAS2 in ALC5640.
> * Change DMIC1/2 CLK to DMIC1/2 Power since it is for enable/disable DMIC1/2
> * Modify some texts for consistent coding style.
> * Merge sto adc l/r mux since it shares the same control bits.
> * Other minor changes.

Put stuff like this after the --- so it doesn't end up in the commit
log.

> +	switch (event) {
> +	case SND_SOC_DAPM_POST_PMU:
> +		snd_soc_update_bits(codec, RT5640_PWR_DIG1, 0x0001, 0x0001);
> +		regmap_update_bits(rt5640->regmap, 0x1c, 0xf000, 0xf000);
> +		break;

It's a bit weird to be using snd_soc_update_bits() and
regmap_update_bits()...

> +	SND_SOC_DAPM_SUPPLY("DMIC1 Power", SND_SOC_NOPM, 0, 0,
> +		rt5640_set_dmic1_event,
> +		SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
> +	SND_SOC_DAPM_SUPPLY("DMIC2 Power", SND_SOC_NOPM, 0, 0,
> +		rt5640_set_dmic2_event,
> +		SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),

The enable bits could be listed here - this would be a bit more
idiomatic, would save the PMD event and would enable the writes to be
combined.

> +	case SND_SOC_BIAS_STANDBY:
> +		if (SND_SOC_BIAS_OFF == codec->dapm.bias_level) {
> +			snd_soc_update_bits(codec, RT5640_PWR_ANLG1,
> +				RT5640_PWR_VREF1 | RT5640_PWR_MB |
> +				RT5640_PWR_BG | RT5640_PWR_VREF2,
> +				RT5640_PWR_VREF1 | RT5640_PWR_MB |
> +				RT5640_PWR_BG | RT5640_PWR_VREF2);
> +			mdelay(10);
> +			snd_soc_update_bits(codec, RT5640_PWR_ANLG1,
> +				RT5640_PWR_FV1 | RT5640_PWR_FV2,
> +				RT5640_PWR_FV1 | RT5640_PWR_FV2);
> +			regcache_cache_only(rt5640->regmap, false);
> +			regcache_sync(rt5640->regmap);

This looks wrong - you're writing to the device before you take it out
of cache only mode so your delay in the above sequence will have no
effect.

> +	val = snd_soc_read(codec, RT5640_VENDOR_ID2);
> +	if ((val != RT5640_DEVICE_ID)) {
> +		dev_err(codec->dev,
> +			"Device with ID register %x is not rt5640/39\n", val);
> +		return -ENODEV;
> +	}
> +
> +	rt5640_reset(codec);

Do these in the I2C probe function before you register the CODEC, that
way we don't try to start the sound card if we can't talk to the device.

> +	ret = regmap_register_patch(rt5640->regmap, init_list,
> +				    ARRAY_SIZE(init_list));
> +	if (ret != 0)
> +		dev_warn(codec->dev, "Failed to apply regmap patch: %d\n",
> +			 ret);

This should also be in the I2C probe.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Bard Liao]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: Monday, June 03, 2013 11:36 PM
> To: Bard Liao
> Cc: lgirdwood@gmail.com; alsa-devel@alsa-project.org; Flove; Oder Chiou;
> swarren@nvidia.com; swarren@wwwdotorg.org
> Subject: Re: [PATCH] ASoC: add RT5640 CODEC driver
> > +	case SND_SOC_BIAS_STANDBY:
> > +		if (SND_SOC_BIAS_OFF == codec->dapm.bias_level) {
> > +			snd_soc_update_bits(codec, RT5640_PWR_ANLG1,
> > +				RT5640_PWR_VREF1 | RT5640_PWR_MB |
> > +				RT5640_PWR_BG | RT5640_PWR_VREF2,
> > +				RT5640_PWR_VREF1 | RT5640_PWR_MB |
> > +				RT5640_PWR_BG | RT5640_PWR_VREF2);
> > +			mdelay(10);
> > +			snd_soc_update_bits(codec, RT5640_PWR_ANLG1,
> > +				RT5640_PWR_FV1 | RT5640_PWR_FV2,
> > +				RT5640_PWR_FV1 | RT5640_PWR_FV2);
> > +			regcache_cache_only(rt5640->regmap, false);
> > +			regcache_sync(rt5640->regmap);
> 
> This looks wrong - you're writing to the device before you take it out of cache
> only mode so your delay in the above sequence will have no effect.

Actually, I don't know if I need to call "regcache_cache_only(rt5640->regmap, false);" here.
We never call "regcache_cache_only(rt5640->regmap, true);" in the codec driver.
I assume the cache_only flag is default false.
If yes, I think I can remove "regcache_cache_only(rt5640->regmap, false);" from the code driver.
If not, I will move it to the beginning of if (SND_SOC_BIAS_OFF == codec->dapm.bias_level) condition.


> ------Please consider the environment before printing this e-mail.

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 653 bytes --]

On Tue, Jun 04, 2013 at 02:39:36PM +0800, Bard Liao wrote:

> Actually, I don't know if I need to call "regcache_cache_only(rt5640->regmap, false);" here.
> We never call "regcache_cache_only(rt5640->regmap, true);" in the codec driver.
> I assume the cache_only flag is default false.
> If yes, I think I can remove "regcache_cache_only(rt5640->regmap, false);" from the code driver.
> If not, I will move it to the beginning of if (SND_SOC_BIAS_OFF == codec->dapm.bias_level) condition.

I'd actually put in code to enable cache only mode when you power off,
that way when the support for the LDO enable GPIO is added it will not
need to be re-added.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Stephen Warren]

On 05/31/2013 01:15 AM, Bard Liao wrote:
> Stephen,
> 
> I remove the DT support from this patch.
> Can you add the DT support as a separate patch?

Yes, I'll do that.

I assume you also want me to add the code to control the LDO1_EN GPIO,
since that seems missing from this patch. I thought you said you were
going to add it. I assume you tested this on Dalmore by manually setting
the GPIO from U-Boot before booting the kernel?

I'll try to get to this ASAP; perhaps not today though - we'll see how
long the other things I have stacked up take.

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Bard Liao]

> -----Original Message-----
> From: Stephen Warren [mailto:swarren@wwwdotorg.org]
> Sent: Monday, June 03, 2013 11:49 PM
> To: Bard Liao
> Cc: broonie@kernel.org; lgirdwood@gmail.com; alsa-devel@alsa-project.org;
> Flove; Oder Chiou; swarren@nvidia.com
> Subject: Re: [PATCH] ASoC: add RT5640 CODEC driver
> 
> On 05/31/2013 01:15 AM, Bard Liao wrote:
> > Stephen,
> >
> > I remove the DT support from this patch.
> > Can you add the DT support as a separate patch?
> 
> Yes, I'll do that.
> 
> I assume you also want me to add the code to control the LDO1_EN GPIO,
> since that seems missing from this patch. I thought you said you were going to
> add it. I assume you tested this on Dalmore by manually setting the GPIO from
> U-Boot before booting the kernel?

Yes, please add the code to control the LDO1_EN_GPIO.
Thanks a lot.
I execute "gpio set 171" command in U-Boot every time before booting the kernel.

> 
> I'll try to get to this ASAP; perhaps not today though - we'll see how long the
> other things I have stacked up take.
> 
> ------Please consider the environment before printing this e-mail.

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Stephen Warren]

On 05/31/2013 01:04 AM, bardliao@realtek.com wrote:
> From: Bard Liao <bardliao@realtek.com>
> 
> * Use regmap_range_cfg to replace index read/write function.
> * Remove I2S3 related code since there is no I2S3 in ALC5640.
> * Remove Voice DSP related code since there is no Voice DSP in ALC5640.
> * Remove MICBIAS2 since there is no MICBIAS2 in ALC5640.
> * Change DMIC1/2 CLK to DMIC1/2 Power since it is for enable/disable DMIC1/2
> * Modify some texts for consistent coding style.
> * Merge sto adc l/r mux since it shares the same control bits.
> * Other minor changes.

> +static const struct snd_soc_dapm_widget rt5640_dapm_widgets[] = {

> +	/* Input Lines */
> +	SND_SOC_DAPM_INPUT("DMIC1"),
> +	SND_SOC_DAPM_INPUT("DMIC2"),

Can you explain that more? In the spec for this part, I don't see any
DMIC1/2 input pins. Instead, I think they're alternative uses for the
IN1P/N pins, right?

> +	SND_SOC_DAPM_MIXER("HPO MIX L", SND_SOC_NOPM, 0, 0,
> +		rt5640_hpo_mix, ARRAY_SIZE(rt5640_hpo_mix)),
> +	SND_SOC_DAPM_MIXER("HPO MIX R", SND_SOC_NOPM, 0, 0,
> +		rt5640_hpo_mix, ARRAY_SIZE(rt5640_hpo_mix)),

In the version of the driver I posted, I had replaced this with a single
"HPO MIX" DAPM_MIXER, because there is a single bit in HW that controls
both the L and R channels. I think you should incorporate that same change.

That relies on commit 85762e7 "ASoC: dapm: Implement mixer control sharing".

In an earlier review, IIRC, Mark had asked you to implement platform
data to configure whether the microphones were single-ended or not. I
don't see that in this patch.

Finally, I really would prefer if you could implement the support for
the LDO1_EN GPIO via platform data. Getting that right requires a bit of
knowledge of how set_bias_level() is supposed to work on this CODEC, and
I think Realtek have the best information to get that right. I'll
certainly send a patch to implement this all by device tree after that.

Anyway, I'll go test this patch and see if it works for me on my HW.

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Stephen Warren]

On 06/04/2013 03:51 PM, Stephen Warren wrote:
> On 05/31/2013 01:04 AM, bardliao@realtek.com wrote:
>> From: Bard Liao <bardliao@realtek.com>
>>
>> * Use regmap_range_cfg to replace index read/write function.
>> * Remove I2S3 related code since there is no I2S3 in ALC5640.
>> * Remove Voice DSP related code since there is no Voice DSP in ALC5640.
>> * Remove MICBIAS2 since there is no MICBIAS2 in ALC5640.
>> * Change DMIC1/2 CLK to DMIC1/2 Power since it is for enable/disable DMIC1/2
>> * Modify some texts for consistent coding style.
>> * Merge sto adc l/r mux since it shares the same control bits.
>> * Other minor changes.
> 
>> +static const struct snd_soc_dapm_widget rt5640_dapm_widgets[] = {

>> +	SND_SOC_DAPM_MIXER("HPO MIX L", SND_SOC_NOPM, 0, 0,
>> +		rt5640_hpo_mix, ARRAY_SIZE(rt5640_hpo_mix)),
>> +	SND_SOC_DAPM_MIXER("HPO MIX R", SND_SOC_NOPM, 0, 0,
>> +		rt5640_hpo_mix, ARRAY_SIZE(rt5640_hpo_mix)),
> 
> In the version of the driver I posted, I had replaced this with a single
> "HPO MIX" DAPM_MIXER, because there is a single bit in HW that controls
> both the L and R channels. I think you should incorporate that same change.
> 
> That relies on commit 85762e7 "ASoC: dapm: Implement mixer control sharing".

Sorry, ignore that comment; what I had done earlier was to make both the
SND_SOC_DAPM_MIXER() invocations above use the same rt5640_hpo_mix
kcontrol, which is exactly what you have in your patch.

So, this part is fine. Sorry for the noise.

BTW, I tested on my HW, and I see that headphone output still works fine
with this version of the driver.

I didn't test speaker output though, but that didn't work with the
previous driver I sent either. Are you able to get that working? I also
didn't test any capture path.

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Bard Liao]

> On 05/31/2013 01:04 AM, bardliao@realtek.com wrote:
> > From: Bard Liao <bardliao@realtek.com>
> >
> > * Use regmap_range_cfg to replace index read/write function.
> > * Remove I2S3 related code since there is no I2S3 in ALC5640.
> > * Remove Voice DSP related code since there is no Voice DSP in ALC5640.
> > * Remove MICBIAS2 since there is no MICBIAS2 in ALC5640.
> > * Change DMIC1/2 CLK to DMIC1/2 Power since it is for enable/disable DMIC1/2
> > * Modify some texts for consistent coding style.
> > * Merge sto adc l/r mux since it shares the same control bits.
> > * Other minor changes.
>
> > +static const struct snd_soc_dapm_widget rt5640_dapm_widgets[] = {
>
> > +     /* Input Lines */
> > +     SND_SOC_DAPM_INPUT("DMIC1"),
> > +     SND_SOC_DAPM_INPUT("DMIC2"),
>
> Can you explain that more? In the spec for this part, I don't see any
> DMIC1/2 input pins. Instead, I think they're alternative uses for the
> IN1P/N pins, right?

Yes, DMIC1/2 are alternative uses for the IN1P/N pins.
If you connect it with a DMIC, it will work as a DMIC input.
There are DMIC1/2 component in ALC5640. 
You can see chapter 4.3 Digital Mixer Path in ALC5640 datasheet for reference.
If IN1P/N use as DMIC1/2, the audio path should go through DMIC1/DMIC2.

> In an earlier review, IIRC, Mark had asked you to implement platform
> data to configure whether the microphones were single-ended or not. I
> don't see that in this patch.

Ok, I will implement the microphones type configuration in my patch.
I thought it will be add in the DT patch.

>
> Finally, I really would prefer if you could implement the support for
> the LDO1_EN GPIO via platform data. Getting that right requires a bit of
> knowledge of how set_bias_level() is supposed to work on this CODEC, and
> I think Realtek have the best information to get that right. I'll
> certainly send a patch to implement this all by device tree after that.

I will do that also, if I have question I will ask you.
Thanks.

>
> Anyway, I'll go test this patch and see if it works for me on my HW.
> 
> ------Please consider the environment before printing this e-mail.

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1360 bytes --]

On Tue, Jun 11, 2013 at 01:10:16PM +0800, bardliao@realtek.com wrote:

> Signed-off-by: Bard Liao <bardliao@realtek.com>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

I don't think that signoff for Stephen is right...

> +static int rt5640_set_bias_level(struct snd_soc_codec *codec,
> +			enum snd_soc_bias_level level)
> +{
> +	struct rt5640_priv *rt5640 = snd_soc_codec_get_drvdata(codec);
> +	switch (level) {
> +	case SND_SOC_BIAS_STANDBY:
> +		if (SND_SOC_BIAS_OFF == codec->dapm.bias_level) {
> +			regcache_cache_only(rt5640->regmap, false);

You mark the device cache only in suspend so for symmetry this should
really be in _BIAS_OFF, especially since the device is idle_bias_off so
it'll be able to power off without a full suspend.

> +	if (rt5640->pdata.ldo1_en) {
> +		ret = devm_gpio_request_one(&i2c->dev, rt5640->pdata.ldo1_en,
> +					    GPIOF_OUT_INIT_HIGH,
> +					    "RT5640 LDO1_EN");
> +		if (ret < 0) {
> +			dev_err(&i2c->dev, "Failed to request LDO1_EN %d: %d\n",
> +				rt5640->pdata.ldo1_en, ret);
> +			return ret;
> +		}
> +		msleep(400);
> +	}

I'd expect to see some handling fo the LDO over suspend but that can be
dealt with in a followup as previously discussed.  Ideally we'd be able
to power off when in _BIAS_OFF too but if it's going to take 400ms to
ramp the LDO (which does seem high) that might be an issue.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Stephen Warren]

On 06/11/2013 03:12 AM, Mark Brown wrote:
> On Tue, Jun 11, 2013 at 01:10:16PM +0800, bardliao@realtek.com
> wrote:
> 
>> Signed-off-by: Bard Liao <bardliao@realtek.com> Signed-off-by:
>> Stephen Warren <swarren@nvidia.com>
> 
> I don't think that signoff for Stephen is right...

I believe this is based on the patch I sent out a little while back,
which obviously included my s-o-b, so this probably makes sense.

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Bard Liao]

> > +static int rt5640_set_bias_level(struct snd_soc_codec *codec,
> > +                     enum snd_soc_bias_level level)
> > +{
> > +     struct rt5640_priv *rt5640 = snd_soc_codec_get_drvdata(codec);
> > +     switch (level) {
> > +     case SND_SOC_BIAS_STANDBY:
> > +             if (SND_SOC_BIAS_OFF == codec->dapm.bias_level) {
> > +                     regcache_cache_only(rt5640->regmap, false);
> 
> You mark the device cache only in suspend so for symmetry this should
> really be in _BIAS_OFF, especially since the device is idle_bias_off so
> it'll be able to power off without a full suspend.

Actually, we prefer turn off the codec fully ,ie. set the LDO pin to low, only in suspend.
Because it need 400 ms when we set the LDO pin to high and reset the codec.
So, may I mark the device cache only in suspend and resume? 

>
> > +     if (rt5640->pdata.ldo1_en) {
> > +             ret = devm_gpio_request_one(&i2c->dev, rt5640->pdata.ldo1_en,
> > +                                         GPIOF_OUT_INIT_HIGH,
> > +                                         "RT5640 LDO1_EN");
> > +             if (ret < 0) {
> > +                     dev_err(&i2c->dev, "Failed to request LDO1_EN %d: %d\n",
> > +                             rt5640->pdata.ldo1_en, ret);
> > +                     return ret;
> > +             }
> > +             msleep(400);
> > +     }
> 
> I'd expect to see some handling fo the LDO over suspend but that can be
> dealt with in a followup as previously discussed.  Ideally we'd be able
> to power off when in _BIAS_OFF too but if it's going to take 400ms to
> ramp the LDO (which does seem high) that might be an issue.

I think I can't set LDO low in _BIAS_OFF bue to the timing issue.
I will handle in suspend.

> 
> ------Please consider the environment before printing this e-mail.

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Stephen Warren]

On 06/10/2013 11:10 PM, bardliao@realtek.com wrote:
> From: Bard Liao <bardliao@realtek.com>
> 
> This patch adds the ALC5640 codec driver.

> diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c

> +static int rt5640_i2c_probe(struct i2c_client *i2c,

> +	rt5640->regmap = devm_regmap_init_i2c(i2c, &rt5640_regmap);

> +	if (rt5640->pdata.ldo1_en) {
> +		ret = devm_gpio_request_one(&i2c->dev, rt5640->pdata.ldo1_en,
> +					    GPIOF_OUT_INIT_HIGH,
> +					    "RT5640 LDO1_EN");
> +		if (ret < 0) {
> +			dev_err(&i2c->dev, "Failed to request LDO1_EN %d: %d\n",
> +				rt5640->pdata.ldo1_en, ret);
> +			return ret;
> +		}
> +		msleep(400);
> +	}

Oh I see this is the only place ldo1_en is touched. I had assumed you
were going to add code to turn it off/on based on bias level. That's why
I had asked you to add that feature, since you'd know any HW
requirements for doing that. Still, as Mark mentioned, that can
certainly be added later.

One question though: Don't you want to initially enable ldo1_en before
you create the regmap? At least some regmap_init() calls end up trying
to read from the device to populate the register cache, and that won't
work until ldo1_en is active.

Anyway, I'll go test this again, and work on the patch to pull the
platform data from DT.

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Bard Liao]

> On 06/10/2013 11:10 PM, bardliao@realtek.com wrote:
> > From: Bard Liao <bardliao@realtek.com>
> >
> > This patch adds the ALC5640 codec driver.
> 
> > diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c
> 
> > +static int rt5640_i2c_probe(struct i2c_client *i2c,
> 
> > +     rt5640->regmap = devm_regmap_init_i2c(i2c, &rt5640_regmap);
> 
> > +     if (rt5640->pdata.ldo1_en) {
> > +             ret = devm_gpio_request_one(&i2c->dev, rt5640->pdata.ldo1_en,
> > +                                         GPIOF_OUT_INIT_HIGH,
> > +                                         "RT5640 LDO1_EN");
> > +             if (ret < 0) {
> > +                     dev_err(&i2c->dev, "Failed to request LDO1_EN %d: %d\n",
> > +                             rt5640->pdata.ldo1_en, ret);
> > +                     return ret;
> > +             }
> > +             msleep(400);
> > +     }
> 
> Oh I see this is the only place ldo1_en is touched. I had assumed you
> were going to add code to turn it off/on based on bias level. That's why
> I had asked you to add that feature, since you'd know any HW
> requirements for doing that. Still, as Mark mentioned, that can
> certainly be added later.

I think we can't turn on/off ldo1_en based on bias level.
Because it will take too long from _BIAS_OFF to _BIAS_ON.
I think we can do it in suspend/resume function.

> 
> One question though: Don't you want to initially enable ldo1_en before
> you create the regmap? At least some regmap_init() calls end up trying
> to read from the device to populate the register cache, and that won't
> work until ldo1_en is active.
> 

Sure, it is better. Thanks for reminding me.

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 441 bytes --]

On Tue, Jun 11, 2013 at 11:41:49AM -0600, Stephen Warren wrote:

> One question though: Don't you want to initially enable ldo1_en before
> you create the regmap? At least some regmap_init() calls end up trying
> to read from the device to populate the register cache, and that won't
> work until ldo1_en is active.

They shouldn't if a set of defaults is provided, though if a patch is
going to be applied then that'll write to the device.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Stephen Warren]

On 06/10/2013 11:10 PM, bardliao@realtek.com wrote:
> From: Bard Liao <bardliao@realtek.com>
> 
> This patch adds the ALC5640 codec driver.
> 
> Signed-off-by: Bard Liao <bardliao@realtek.com>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

(Oh, I suppose those two lines above should really be swapped)

Tested-by: Stephen Warren <swarren@nvidia.com>

This time, both headphones /and/ speakers worked for me:-) I haven't
tested capture yet though.

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 273 bytes --]

On Tue, Jun 11, 2013 at 01:10:16PM +0800, bardliao@realtek.com wrote:
> From: Bard Liao <bardliao@realtek.com>
> 
> This patch adds the ALC5640 codec driver.

I'll apply this on the basis that there's going to be some followup
patches fixing the outstanding issues.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Bard]

Dear Mark,

Please see my reply below.

Thanks.

-----Original Message-----
From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] 
Sent: Tuesday, April 16, 2013 10:38 PM
To: Bard
Cc: Flove; Oder Chiou; swarren@nvidia.com; bard
Subject: Re: [PATCH] ASoC: add RT5640 CODEC driver

On Tue, Apr 16, 2013 at 10:10:37PM +0800, bardliao@realtek.com wrote:

Please remember to post things to the list...  just a quick scan through
here:

> +static int rt5640_reg_init(struct snd_soc_codec *codec) {
> +	int i;
> +	for (i = 0; i < RT5640_INIT_REG_LEN; i++)
> +		snd_soc_write(codec, init_list[i].reg, init_list[i].val);
> +	return 0;
> +}

This should be converted over to regmap.  There's support for paging in the core, look at regmap_range.

Bard: I will use regmap_register_patch to do that.

> +/* IN1/IN2 Input Type */
> +static const char * const rt5640_input_mode[] = {
> +	"Single ended", "Differential"};
> +
> +static const SOC_ENUM_SINGLE_DECL(
> +	rt5640_in1_mode_enum, RT5640_IN1_IN2,
> +	RT5640_IN_SFT1, rt5640_input_mode);
> +
> +static const SOC_ENUM_SINGLE_DECL(
> +	rt5640_in2_mode_enum, RT5640_IN3_IN4,
> +	RT5640_IN_SFT2, rt5640_input_mode);

Platform data...

Bard: Can I export a function that machine can configure it by this function?

> +static int rt5640_vol_rescale_put(struct snd_kcontrol *kcontrol,
> +		struct snd_ctl_elem_value *ucontrol) {
> +	struct soc_mixer_control *mc =
> +		(struct soc_mixer_control *)kcontrol->private_value;
> +	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
> +	unsigned int val, val2;
> +
> +	val = VOL_RESCALE_MAX_VOL - ucontrol->value.integer.value[0];
> +	val2 = VOL_RESCALE_MAX_VOL - ucontrol->value.integer.value[1];
> +	return snd_soc_update_bits_locked(codec, mc->reg, RT5640_L_VOL_MASK |
> +			RT5640_R_VOL_MASK, val << mc->shift | val2); }

This looks like a variant on the _RANGE controls?

Bard: Yes, we want to limit the max value of volume.
     If it is not good, I will change it.

> +static int hp_event(struct snd_soc_dapm_widget *w,
> +	struct snd_kcontrol *kcontrol, int event) {
> +	switch (event) {
> +	case SND_SOC_DAPM_POST_PMU:
> +		break;
> +
> +	case SND_SOC_DAPM_PRE_PMD:
> +		break;
> +
> +	default:
> +		return 0;
> +	}
> +	return 0;
> +}

Remove this, it doesn't actually do anything any more.

Bard: I will remove it.

------Please consider the environment before printing this e-mail.

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1374 bytes --]

On Mon, Apr 22, 2013 at 03:03:05PM +0800, Bard wrote:

> Please see my reply below.

As you've been told several times now please fix your mailer to quote
the text you're replying to.  You shouldn't need to point out that
you're replying inline, this is how we do things...

> > +static const SOC_ENUM_SINGLE_DECL(
> > +	rt5640_in2_mode_enum, RT5640_IN3_IN4,
> > +	RT5640_IN_SFT2, rt5640_input_mode);

> Platform data...

> Bard: Can I export a function that machine can configure it by this function?

Why on earth would you want to do that?

> > +static int rt5640_vol_rescale_put(struct snd_kcontrol *kcontrol,
> > +		struct snd_ctl_elem_value *ucontrol) {
> > +	struct soc_mixer_control *mc =
> > +		(struct soc_mixer_control *)kcontrol->private_value;
> > +	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
> > +	unsigned int val, val2;

> > +	val = VOL_RESCALE_MAX_VOL - ucontrol->value.integer.value[0];
> > +	val2 = VOL_RESCALE_MAX_VOL - ucontrol->value.integer.value[1];
> > +	return snd_soc_update_bits_locked(codec, mc->reg, RT5640_L_VOL_MASK |
> > +			RT5640_R_VOL_MASK, val << mc->shift | val2); }

> This looks like a variant on the _RANGE controls?

> Bard: Yes, we want to limit the max value of volume.
>      If it is not good, I will change it.

This shouldn't be open coded in the driver, the functions should be
generic.  Code wise it's fine.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 7120 bytes --]

On Tue, Mar 26, 2013 at 05:35:38PM -0600, Stephen Warren wrote:

> Mark, a couple questions:
> * Is the custom index_reg device attr file OK, or should I remove that?
> * Do new CODEC drivers have to use regmap directly, or is using the ASoC
>   IO system still OK?

Convert to regmap please - it looks like this ought to be using the
paging support from a quick glance at what the register I/O stuff is
doing.  The device file appears to duplicate the register dump stuff and
isn't suitable for sysfs anyway as it's not one value per file, it ought
to be in debugfs.

> +/* IN1/IN2 Input Type */
> +static const char * const rt5640_input_mode[] = {
> +	"Single ended", "Differential"};

This looks like platform data.

> +static const char * const rt5640_data_select[] = {
> +	"Normal", "left copy to right", "right copy to left", "Swap"};

DAPM?

> +/* DMIC */
> +static const char * const rt5640_dmic_mode[] = {"Disable", "DMIC1", "DMIC2"};
> +
> +static const SOC_ENUM_SINGLE_DECL(rt5640_dmic_enum, 0, 0, rt5640_dmic_mode);

DAPM?

> +static int rt5640_vol_rescale_get(struct snd_kcontrol *kcontrol,
> +		struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct soc_mixer_control *mc =
> +		(struct soc_mixer_control *)kcontrol->private_value;
> +	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
> +	unsigned int val = snd_soc_read(codec, mc->reg);
> +
> +	ucontrol->value.integer.value[0] = VOL_RESCALE_MAX_VOL -
> +		((val & RT5640_L_VOL_MASK) >> mc->shift);
> +	ucontrol->value.integer.value[1] = VOL_RESCALE_MAX_VOL -
> +		(val & RT5640_R_VOL_MASK);

This looks like a range control.

> +static int hp_event(struct snd_soc_dapm_widget *w,
> +	struct snd_kcontrol *kcontrol, int event)
> +{
> +	switch (event) {
> +	case SND_SOC_DAPM_POST_PMU:
> +		break;
> +
> +	case SND_SOC_DAPM_PRE_PMD:
> +		break;
> +
> +	default:
> +		return 0;
> +	}
> +	return 0;
> +}

This does nothing, it should be removed.

> +static int rt5640_set_dmic1_event(struct snd_soc_dapm_widget *w,
> +	struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_codec *codec = w->codec;
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_PRE_PMU:
> +		snd_soc_update_bits(codec, RT5640_GPIO_CTRL1,
> +			RT5640_GP2_PIN_MASK | RT5640_GP3_PIN_MASK,
> +			RT5640_GP2_PIN_DMIC1_SCL | RT5640_GP3_PIN_DMIC1_SDA);
> +		snd_soc_update_bits(codec, RT5640_DMIC,
> +			RT5640_DMIC_1L_LH_MASK | RT5640_DMIC_1R_LH_MASK |
> +			RT5640_DMIC_1_DP_MASK,
> +			RT5640_DMIC_1L_LH_FALLING | RT5640_DMIC_1R_LH_RISING |
> +			RT5640_DMIC_1_DP_IN1P);
> +		snd_soc_update_bits(codec, RT5640_DMIC,
> +			RT5640_DMIC_1_EN_MASK, RT5640_DMIC_1_EN);
> +	default:
> +		return 0;

The last write there looks awfully like an enable but there's not any
corresponding disable...

> +static int rt5640_set_dmic2_event(struct snd_soc_dapm_widget *w,
> +	struct snd_kcontrol *kcontrol, int event)
> +{
> +	struct snd_soc_codec *codec = w->codec;
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_PRE_PMU:
> +		snd_soc_update_bits(codec, RT5640_GPIO_CTRL1,
> +			RT5640_GP2_PIN_MASK | RT5640_GP4_PIN_MASK,
> +			RT5640_GP2_PIN_DMIC1_SCL | RT5640_GP4_PIN_DMIC2_SDA);

There's a degree of overlap between these two functions...

> +static const struct snd_soc_dapm_route rt5640_dapm_routes[] = {
> +	{"IN1P", NULL, "LDO2"},
> +	{"IN2P", NULL, "LDO2"},
> +
> +	{"IN1P", NULL, "MIC1"},
> +	{"IN1N", NULL, "MIC1"},
> +	{"IN2P", NULL, "MIC2"},
> +	{"IN2N", NULL, "MIC2"},

Are MIC1 and MIC2 pins on the device?  It's a bit odd to have a single
physical input feeding both sides of a differential path internally.

> +	{"DMIC L1", NULL, "DMIC CLK"},
> +	{"DMIC L2", NULL, "DMIC CLK"},

Shouldn't the DMIC CLK widget be handling the clock enables/muxes from
the DMIC event above?

> +static int get_sdp_info(struct snd_soc_codec *codec, int dai_id)
> +{
> +	int ret = 0, val = snd_soc_read(codec, RT5640_I2S1_SDP);
> +
> +	if (codec == NULL)
> +		return -EINVAL;

No point in the NULL check immediately after a dereference.

> +	val = (val & RT5640_I2S_IF_MASK) >> RT5640_I2S_IF_SFT;
> +	switch (dai_id) {
> +	case RT5640_AIF1:
> +		if (val == RT5640_IF_123 || val == RT5640_IF_132 ||
> +			val == RT5640_IF_113)
> +			ret |= RT5640_U_IF1;
> +		if (val == RT5640_IF_312 || val == RT5640_IF_213 ||
> +			val == RT5640_IF_113)
> +			ret |= RT5640_U_IF2;
> +		if (val == RT5640_IF_321 || val == RT5640_IF_231)
> +			ret |= RT5640_U_IF3;

The tree of if ( || || ) looks like a switch statement and should
probably be written as such.

> +	frame_size = snd_soc_params_to_frame_size(params);
> +	if (frame_size < 0) {
> +		dev_err(codec->dev, "Unsupported frame size: %d\n", frame_size);
> +		return -EINVAL;
> +	}

The error message here is confusing as a negative code is an errno not a
frame size and it probably ought to be passed to the caller.

> +	bclk_ms = frame_size > 32 ? 1 : 0;

Grumble.

> +static int rt5640_prepare(struct snd_pcm_substream *substream,
> +				struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_soc_codec *codec = rtd->codec;
> +	struct rt5640_priv *rt5640 = snd_soc_codec_get_drvdata(codec);
> +
> +	rt5640->aif_pu = dai->id;
> +	return 0;
> +}

This looks like only one DAI can be active at once, shouldn't there be
some sort of checking for busy here?

> +	dai_sel = get_sdp_info(codec, dai->id);
> +	if (dai_sel < 0) {
> +		dev_err(codec->dev, "Failed to get sdp info: %d\n", dai_sel);
> +		return -EINVAL;
> +	}

I suspect using dai->base (or just picking a suitable value for dai->id
even) would be simpler.

> +	if (dai_sel & RT5640_U_IF1) {
> +		snd_soc_update_bits(codec, RT5640_I2S1_SDP,
> +			RT5640_I2S_MS_MASK | RT5640_I2S_BP_MASK |
> +			RT5640_I2S_DF_MASK, reg_val);
> +	}
> +	if (dai_sel & RT5640_U_IF2) {
> +		snd_soc_update_bits(codec, RT5640_I2S2_SDP,
> +			RT5640_I2S_MS_MASK | RT5640_I2S_BP_MASK |
> +			RT5640_I2S_DF_MASK, reg_val);
> +	}

This looks like switch statements again, though (not having checked) I'd
expect that there's just blocks of registers for each DAI and we could
just work in terms of a base?

> +/**
> + * rt5640_pll_calc - Calcualte PLL M/N/K code.

Typo.

> +	snd_soc_add_codec_controls(codec, rt5640_snd_controls,
> +		ARRAY_SIZE(rt5640_snd_controls));

Set in the driver structure.

> +static int rt5640_resume(struct snd_soc_codec *codec)
> +{
> +	int ret = 0 ;
> +
> +	codec->cache_sync = 1;
> +	ret = snd_soc_cache_sync(codec);
> +	if (ret) {
> +		dev_err(codec->dev, "Failed to sync cache: %d\n", ret);
> +		return ret;
> +	}
> +	rt5640_set_bias_level(codec, SND_SOC_BIAS_STANDBY);

We also sync the cache in the bias management code.

> +static int rt5640_i2c_probe(struct i2c_client *i2c,
> +		    const struct i2c_device_id *id)
> +{
> +	struct rt5640_priv *rt5640;
> +	int ret;
> +
> +	rt5640 = kzalloc(sizeof(struct rt5640_priv), GFP_KERNEL);
> +	if (NULL == rt5640)
> +		return -ENOMEM;

devm_kzalloc()

> +struct rt5640_pll_code {
> +	bool m_bp; /* Indicates bypass m code or not. */
> +	int m_code;
> +	int n_code;
> +	int k_code;
> +};
> +
> +struct rt5640_priv {
> +	struct snd_soc_codec *codec;

Move these into the body of the driver.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Stephen Warren]

On 03/26/2013 07:15 PM, Mark Brown wrote:
> On Tue, Mar 26, 2013 at 05:35:38PM -0600, Stephen Warren wrote:
> 
>> Mark, a couple questions: * Is the custom index_reg device attr
>> file OK, or should I remove that? * Do new CODEC drivers have to
>> use regmap directly, or is using the ASoC IO system still OK?
> 
> Convert to regmap please - it looks like this ought to be using
> the paging support from a quick glance at what the register I/O
> stuff is doing.  The device file appears to duplicate the register
> dump stuff and isn't suitable for sysfs anyway as it's not one
> value per file, it ought to be in debugfs.

Ah, I hadn't seen the ranges stuff before. I think that should work.

>> +/* IN1/IN2 Input Type */ +static const char * const
>> rt5640_input_mode[] = { +	"Single ended", "Differential"};
> 
> This looks like platform data.

I thought the idea was to expose all the CODEC's configuration/routing
through DAPM, and then let everything get set up through controls at
run-time? Looking at the WM8903 driver, it seems like it has the exact
same kind of option.

>> +static const char * const rt5640_data_select[] = { +	"Normal",
>> "left copy to right", "right copy to left", "Swap"};
> 
> DAPM?
> 
>> +/* DMIC */ +static const char * const rt5640_dmic_mode[] =
>> {"Disable", "DMIC1", "DMIC2"}; + +static const
>> SOC_ENUM_SINGLE_DECL(rt5640_dmic_enum, 0, 0, rt5640_dmic_mode);
> 
> DAPM?

I'm not sure what change you're asking for here; isn't this defining a
control that influences the DAPM routing?

Perhaps the issue is that the enum above feeds into a snd_kcontrol,
rather than being an snd_soc_dapm_route that's conditional upon one of
the values in that list?

If so, that's going to be a lot of stuff to change in the driver
considering that I can't actually test any of the DMIC (or even analog
mic) support yet since I can't get it working. Perhaps I should just
rip out all the widgets/controls I can't test?

>> +static const struct snd_soc_dapm_route rt5640_dapm_routes[] = { 
>> +	{"IN1P", NULL, "LDO2"}, +	{"IN2P", NULL, "LDO2"}, + +	{"IN1P",
>> NULL, "MIC1"}, +	{"IN1N", NULL, "MIC1"}, +	{"IN2P", NULL,
>> "MIC2"}, +	{"IN2N", NULL, "MIC2"},
> 
> Are MIC1 and MIC2 pins on the device?

Hmmm. I see SND_SOC_DAPM_INPUT()s for MIC1 and MIC2, but they don't
appear to be pins on the device. Rather, IN1P/N and IN2P/N are the
pins, and also are declared as SND_SOC_DAPM_INPUT().

I'm not really sure what MIC1/MIC2 are meant to represent...

>> +	{"DMIC L1", NULL, "DMIC CLK"}, +	{"DMIC L2", NULL, "DMIC
>> CLK"},
> 
> Shouldn't the DMIC CLK widget be handling the clock enables/muxes
> from the DMIC event above?

Oh right, so you mean:

* Add a SND_SOC_DAPM_OUTPUT() for the "DMIC CLK".

* Change the "DMIC L*" from SND_SOC_DAPM_PGA_E() to
SND_SOC_DAPM_PGA(), and move the event to the "DMIC CLK" widget.

* Add routing table entries where "DMIC1 *" are each fed from "DMIC
CLK", with "conditions" based on which DMIC is "on":

{"DMIC L1" "DMIC1" "DMIC CLK"},
{"DMIC R1" "DMIC1" "DMIC CLK"},
{"DMIC L2" "DMIC2" "DMIC CLK"},
{"DMIC R2" "DMIC2" "DMIC CLK"},

or something like that?

>> +static int get_sdp_info(struct snd_soc_codec *codec, int
>> dai_id)
...
>> +	bclk_ms = frame_size > 32 ? 1 : 0;
> 
> Grumble.

Sorry, I don't understand the issue here. (through lack of familiarity
with the issue; I'm not saying there isn't one).

>> +static int rt5640_prepare(struct snd_pcm_substream *substream, +
>> struct snd_soc_dai *dai) +{ +	struct snd_soc_pcm_runtime *rtd =
>> substream->private_data; +	struct snd_soc_codec *codec =
>> rtd->codec; +	struct rt5640_priv *rt5640 =
>> snd_soc_codec_get_drvdata(codec); + +	rt5640->aif_pu = dai->id; +
>> return 0; +}
> 
> This looks like only one DAI can be active at once, shouldn't there
> be some sort of checking for busy here?

I haven't read the spec through in explicit detail, but I didn't see
anything to indicate that only one DAI could be active at once. In
fact the diagrams in the documentation make it look at least somewhat
cross-bar-like.

It looks like rt5640->aif_pu is used solely by set_dmic_clk() to
somehow derive the DMIC CLK from the DAI LRCLK based on which DAI is
actually active. I assume "which DAI is actually active" should be
implemented as "which DAI is routed from the DMICs". I guess this
should be implemented in a very different way then?

> 
>> +	dai_sel = get_sdp_info(codec, dai->id); +	if (dai_sel < 0) { +
>> dev_err(codec->dev, "Failed to get sdp info: %d\n", dai_sel); +
>> return -EINVAL; +	}
> 
> I suspect using dai->base (or just picking a suitable value for
> dai->id even) would be simpler.

So there is something dynamic here; there are 2 physical DAIs on the
chip package, but somehow I think there are 3 digital channels within
the device that can be routed to/from the physical DAIs in various
(sometimes broadcast?) configurations based on the rt5640_dai_iis_map.
Hence, the implementation of get_sdp_info() looks up the value of the
register that configures that routing, and returns data that isn't
static...

I wonder if this all shouldn't be explicitly represented by DAPM
widgets/routes, but the diagrams and text in the documentation aren't
particularly clear on what this third thing is that the code seems to
support:-(

>> +static int rt5640_resume(struct snd_soc_codec *codec) +{ +	int
>> ret = 0 ; + +	codec->cache_sync = 1; +	ret =
>> snd_soc_cache_sync(codec); +	if (ret) { +		dev_err(codec->dev,
>> "Failed to sync cache: %d\n", ret); +		return ret; +	} +
>> rt5640_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
> 
> We also sync the cache in the bias management code.

In other words, I should just remove the call to
snd_soc_cache_sync(codec) here, since it's redundant?

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 4949 bytes --]

On Wed, Mar 27, 2013 at 04:50:21PM -0600, Stephen Warren wrote:
> On 03/26/2013 07:15 PM, Mark Brown wrote:
> > On Tue, Mar 26, 2013 at 05:35:38PM -0600, Stephen Warren wrote:

> >> +/* IN1/IN2 Input Type */ +static const char * const
> >> rt5640_input_mode[] = { +	"Single ended", "Differential"};

> > This looks like platform data.

> I thought the idea was to expose all the CODEC's configuration/routing
> through DAPM, and then let everything get set up through controls at

Given that changing between single ended and differential generally
involves a modification of the circuit this is something we know at
build time in the same way we know how the microphone biases are wired
up.  Changing it would generally need to be coordinated with some
external circuit (eg, flipping an analouge switch to reroute the
signals) and hence done from the machine driver.

> run-time? Looking at the WM8903 driver, it seems like it has the exact
> same kind of option.

Older drivers aren't always good examples of this stuff.

> >> +/* DMIC */ +static const char * const rt5640_dmic_mode[] =
> >> {"Disable", "DMIC1", "DMIC2"}; + +static const
> >> SOC_ENUM_SINGLE_DECL(rt5640_dmic_enum, 0, 0, rt5640_dmic_mode);

> > DAPM?

> I'm not sure what change you're asking for here; isn't this defining a
> control that influences the DAPM routing?

It looks like just a plain old enum to me.

> Perhaps the issue is that the enum above feeds into a snd_kcontrol,
> rather than being an snd_soc_dapm_route that's conditional upon one of
> the values in that list?

Probably, or a mux or something.

> I'm not really sure what MIC1/MIC2 are meant to represent...

I think they're supposed to represent the microphones someone might
connect to the inputs but that should be done in the machine driver.

> > Shouldn't the DMIC CLK widget be handling the clock enables/muxes
> > from the DMIC event above?

> Oh right, so you mean:

> * Add a SND_SOC_DAPM_OUTPUT() for the "DMIC CLK".

No need for that, it can just be handled as a supply within the device
since it's an integral part of the DMIC interface.

> * Add routing table entries where "DMIC1 *" are each fed from "DMIC
> CLK", with "conditions" based on which DMIC is "on":

> {"DMIC L1" "DMIC1" "DMIC CLK"},
> {"DMIC R1" "DMIC1" "DMIC CLK"},
> {"DMIC L2" "DMIC2" "DMIC CLK"},
> {"DMIC R2" "DMIC2" "DMIC CLK"},

> or something like that?

Pretty much.

> >> +static int get_sdp_info(struct snd_soc_codec *codec, int
> >> dai_id)

> >> +	bclk_ms = frame_size > 32 ? 1 : 0;

> > Grumble.

> Sorry, I don't understand the issue here. (through lack of familiarity
> with the issue; I'm not saying there isn't one).

I don't like the ternery operator where it's not especially idiomatic,
if I wanted to read line noise I'd be working with perl.

> >> +static int rt5640_prepare(struct snd_pcm_substream *substream, +
> >> struct snd_soc_dai *dai) +{ +	struct snd_soc_pcm_runtime *rtd =
> >> substream->private_data; +	struct snd_soc_codec *codec =
> >> rtd->codec; +	struct rt5640_priv *rt5640 =
> >> snd_soc_codec_get_drvdata(codec); + +	rt5640->aif_pu = dai->id; +
> >> return 0; +}

> > This looks like only one DAI can be active at once, shouldn't there
> > be some sort of checking for busy here?

> I haven't read the spec through in explicit detail, but I didn't see
> anything to indicate that only one DAI could be active at once. In
> fact the diagrams in the documentation make it look at least somewhat
> cross-bar-like.

> It looks like rt5640->aif_pu is used solely by set_dmic_clk() to
> somehow derive the DMIC CLK from the DAI LRCLK based on which DAI is
> actually active. I assume "which DAI is actually active" should be
> implemented as "which DAI is routed from the DMICs". I guess this
> should be implemented in a very different way then?

Probably, yes.  This code really doesn't look like it'll work properly
if the two interfaces are running at once, especially if they are
started simultaneously.

> I wonder if this all shouldn't be explicitly represented by DAPM
> widgets/routes, but the diagrams and text in the documentation aren't
> particularly clear on what this third thing is that the code seems to
> support:-(

Yes, putting it all into DAPM seems like the way forward.

> >> +static int rt5640_resume(struct snd_soc_codec *codec) +{ +	int
> >> ret = 0 ; + +	codec->cache_sync = 1; +	ret =
> >> snd_soc_cache_sync(codec); +	if (ret) { +		dev_err(codec->dev,
> >> "Failed to sync cache: %d\n", ret); +		return ret; +	} +
> >> rt5640_set_bias_level(codec, SND_SOC_BIAS_STANDBY);

> > We also sync the cache in the bias management code.

> In other words, I should just remove the call to
> snd_soc_cache_sync(codec) here, since it's redundant?

Or remove it from the bias management since there's no regulator or
other code I can see which cuts the power or otherwise resets the device
so restoring the cache outside of suspend is probably a waste of time at
the minute.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2150 bytes --]

On Tue, Apr 16, 2013 at 04:29:40PM -0600, Stephen Warren wrote:

> a) There are many different VDD inputs to the chip. I imagine that some
> of these only need to be enabled/active under certain conditions. For
> example, perhaps the SPKVDD or MICVDD inputs are only required if
> actively using the speakers or microphones, and could be disabled at
> other times? If so, all of these "optional" or "part-time" VDD inputs
> should be represented as regulators, and retrieved/manipulated using
> Linux APIs such as regulator_get(), regulator_enable(), etc.

There's framework support for regulator supplies in ASoC so you just
need to specify the name and the core will do the rest.

> b) Some VDD inputs are optional; for example, either I believe that a
> board should either provide DCVDD, or provide LDO1_IN and assert the
> LDO1_EN input signal. Similarly, I think that either MICVDD should be
> provided, or SPKVDDL be provided, coupled with LDO2_EN register bit
> being set. This may require the RT5640 driver to provide two regulator
> objects (LDO1, LDO2), which the board file or device tree can connect
> back to the DCVDD and MICVDD inputs if appropriate for the board's HW
> configuration. In the case where LDO1 is used, a separate fixed
> regulator with GPIO should be used to control the LDO1_EN GPIO input.

That said if boards generally don't use external supplies and use the
built in regulators then it's probably best to at least have the driver
assume that by default.

> > Bard: You are right. The mic type should be defined according to circuit.
> >           And it should be change in run time.
> >          So, should we just remove this control in codec driver?

> Mark's point is that the driver should either/both accept some platform
> data structure, or parse device tree, to configure the input pins. I
> don't think completely removing all mechanisms to configure this would
> be a good idea, since then how would the board/device-tree author
> configure the CODEC?

Indeed, there definitely should be configuration - however what I was
commenting on there was a user-visible ALSA control which should
definitely go.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Stephen Warren]

On 04/17/2013 08:01 AM, Mark Brown wrote:
> On Tue, Apr 16, 2013 at 04:29:40PM -0600, Stephen Warren wrote:
> 
>> a) There are many different VDD inputs to the chip. I imagine
>> that some of these only need to be enabled/active under certain
>> conditions. For example, perhaps the SPKVDD or MICVDD inputs are
>> only required if actively using the speakers or microphones, and
>> could be disabled at other times? If so, all of these "optional"
>> or "part-time" VDD inputs should be represented as regulators,
>> and retrieved/manipulated using Linux APIs such as
>> regulator_get(), regulator_enable(), etc.
> 
> There's framework support for regulator supplies in ASoC so you
> just need to specify the name and the core will do the rest.
> 
>> b) Some VDD inputs are optional; for example, either I believe
>> that a board should either provide DCVDD, or provide LDO1_IN and
>> assert the LDO1_EN input signal. Similarly, I think that either
>> MICVDD should be provided, or SPKVDDL be provided, coupled with
>> LDO2_EN register bit being set. This may require the RT5640
>> driver to provide two regulator objects (LDO1, LDO2), which the
>> board file or device tree can connect back to the DCVDD and
>> MICVDD inputs if appropriate for the board's HW configuration. In
>> the case where LDO1 is used, a separate fixed regulator with GPIO
>> should be used to control the LDO1_EN GPIO input.
> 
> That said if boards generally don't use external supplies and use
> the built in regulators then it's probably best to at least have
> the driver assume that by default.

So the issue here is that regulators aren't supposed to be optional,
right? So if there's a reasonable chance that regulators would ever be
needed, we should add them now.

With board files, we probably could have just added them later, but
with device tree (which is my use-case for this CODEC at least), the
DT binding needs to specify which regulator(s) the device requires (if
any) right from the start, so that all DTs will include the regulator
definitions.

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 931 bytes --]

On Wed, Apr 17, 2013 at 09:18:30AM -0600, Stephen Warren wrote:
> On 04/17/2013 08:01 AM, Mark Brown wrote:

> > That said if boards generally don't use external supplies and use
> > the built in regulators then it's probably best to at least have
> > the driver assume that by default.

> So the issue here is that regulators aren't supposed to be optional,
> right? So if there's a reasonable chance that regulators would ever be
> needed, we should add them now.

> With board files, we probably could have just added them later, but
> with device tree (which is my use-case for this CODEC at least), the
> DT binding needs to specify which regulator(s) the device requires (if
> any) right from the start, so that all DTs will include the regulator
> definitions.

You can do the same thing with DT as you do with board files - make
those supplies an optional property and then if the property is missing
do the default thing.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Stephen Warren]

On 04/17/2013 09:28 AM, Mark Brown wrote:
> On Wed, Apr 17, 2013 at 09:18:30AM -0600, Stephen Warren wrote:
>> On 04/17/2013 08:01 AM, Mark Brown wrote:
> 
>>> That said if boards generally don't use external supplies and
>>> use the built in regulators then it's probably best to at least
>>> have the driver assume that by default.
> 
>> So the issue here is that regulators aren't supposed to be
>> optional, right? So if there's a reasonable chance that
>> regulators would ever be needed, we should add them now.
> 
>> With board files, we probably could have just added them later,
>> but with device tree (which is my use-case for this CODEC at
>> least), the DT binding needs to specify which regulator(s) the
>> device requires (if any) right from the start, so that all DTs
>> will include the regulator definitions.
> 
> You can do the same thing with DT as you do with board files -
> make those supplies an optional property and then if the property
> is missing do the default thing.

But then, you end up with an optional regulator, and the driver has to
do things like:

if (!IS_ERR(x->reg_foo))
	regulator_enable(x->reg_foo);

I thought the whole point of the rule that "if a regulator is ever
needed, it must always be provided, and if there isn't one on the
board, use a 'dummy' fixed-regulator" was to avoid exactly that?

But if that rule is relaxed, and the code above is fine, then indeed
one can do as you say.

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Mark Brown]

On Wed, Apr 17, 2013 at 10:25:30AM -0600, Stephen Warren wrote:
> On 04/17/2013 09:28 AM, Mark Brown wrote:

> > You can do the same thing with DT as you do with board files -
> > make those supplies an optional property and then if the property
> > is missing do the default thing.

> But then, you end up with an optional regulator, and the driver has to
> do things like:

> if (!IS_ERR(x->reg_foo))
> 	regulator_enable(x->reg_foo);

Not if you do it at the other end - do it during device registration.
If nothing is set up then feed the regulator API whatever the default
configuration is for the device, otherwise use what you were given.
The consumer side can't tell where the configuration came from and will
always have one.

This does mean you need to do the regulator driver if you support
non-default configurations but that's no bad thing.

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Stephen Warren]

On 04/17/2013 12:52 PM, Mark Brown wrote:
> On Wed, Apr 17, 2013 at 10:25:30AM -0600, Stephen Warren wrote:
>> On 04/17/2013 09:28 AM, Mark Brown wrote:
> 
>>> You can do the same thing with DT as you do with board files -
>>> make those supplies an optional property and then if the property
>>> is missing do the default thing.
> 
>> But then, you end up with an optional regulator, and the driver has to
>> do things like:
> 
>> if (!IS_ERR(x->reg_foo))
>> 	regulator_enable(x->reg_foo);
> 
> Not if you do it at the other end - do it during device registration.
> If nothing is set up then feed the regulator API whatever the default
> configuration is for the device, otherwise use what you were given.
> The consumer side can't tell where the configuration came from and will
> always have one.
> 
> This does mean you need to do the regulator driver if you support
> non-default configurations but that's no bad thing.

OK, so something like:

During DT parsing:

if (!dt_property_exits())
    // regulator API call to set up a dummy regulator for this device

Somewhere later in the probe() path:

x->reg = regulator_get();

The one thing I may have forgotten to mention here for the LDO1 case is
that if we don't explicitly support regulators right away, then instead
the driver needs to explicitly request a GPIO to control the LDO1_EN
input on the chip; without that, the device won't even respond to I2C
accesses. And hence, that GPIO also needs to be described in device
tree. The other regulators could certainly all work as you're pointing out.

Re: [PATCH] ASoC: add RT5640 CODEC driver

[Mark Brown]

On Wed, Apr 17, 2013 at 12:56:30PM -0600, Stephen Warren wrote:
> On 04/17/2013 12:52 PM, Mark Brown wrote:

> > Not if you do it at the other end - do it during device registration.
> > If nothing is set up then feed the regulator API whatever the default
> > configuration is for the device, otherwise use what you were given.
> > The consumer side can't tell where the configuration came from and will
> > always have one.

> > This does mean you need to do the regulator driver if you support
> > non-default configurations but that's no bad thing.

> OK, so something like:

> During DT parsing:

> if (!dt_property_exits())
>     // regulator API call to set up a dummy regulator for this device

You should probably have an actual regulator for the regulator that's
physically there and doing things but yes.  If (as one should) you're
supporting platform data as well then the DT code should just work with
no extra effort.

The WM8994 code I posted recently does all this, the regulator side is
in -next though Samuel didn't pick up the DT stuff yet and non-default
configurations just aren't supported except with platform data since I
am not aware of any such designs.

> The one thing I may have forgotten to mention here for the LDO1 case is
> that if we don't explicitly support regulators right away, then instead
> the driver needs to explicitly request a GPIO to control the LDO1_EN
> input on the chip; without that, the device won't even respond to I2C
> accesses. And hence, that GPIO also needs to be described in device
> tree. The other regulators could certainly all work as you're pointing out.

That's expected, yes - you can just define a property for the GPIO and
move it over to being handled by a proper regulator driver when you
write one.