[PATCH 1/3] ASoC: add 88pm860x codec driver

[PATCH 1/3] ASoC: add 88pm860x codec driver

[Haojian Zhuang]

[alsa-devel] [PATCH 1/3] ASoC: add 88pm860x codec driver

[Mark Brown]

On Wed, Aug 18, 2010 at 09:49:26PM +0800, Haojian Zhuang wrote:

> +		if (dac) {
> +			/* automute is set before operating DAC. Anti-pop */
> +			mutex_lock(&pm860x->mutex);
> +			pm860x->automute = 1;
> +			dac |= MODULATOR;
> +			/* mute */
> +			snd_soc_update_bits(codec, PM860X_DAC_OFFSET,
> +					    DAC_MUTE, DAC_MUTE);
> +			snd_soc_update_bits(codec, PM860X_EAR_CTRL_2,
> +					    RSYNC_CHANGE, RSYNC_CHANGE);
> +			mutex_unlock(&pm860x->mutex);
> +			/* update dac */
> +			snd_soc_update_bits(codec, PM860X_DAC_EN_2,
> +					    dac, dac);
> +		}


I still can't follow what this automute variable is supposed to be
doing.  In both the PMU and the PMD cases you set automute, and the fact
that you forcibly clear it when you apply the change means that it
doesn't look like it's doing anything I'd recognise as automute,
suggesting that the variable is misnamed.  It's very difficult to follow
what all this is supposed to be doing.

As I said last time what I'd expect to see is a user visible mute
control which sets a variable in the driver which is then updated in the
chip only while the DAC is powered.  The other option is to tie the
control to the DAC power, in which case things should be handleable in
the widget event without the variable.

> +static int pm860x_digital_mute(struct snd_soc_dai *codec_dai, int mute)
> +{
> +	struct snd_soc_codec *codec = codec_dai->codec;
> +	int data = 0;
> +
> +	if (mute)
> +		data = DAC_MUTE;
> +	snd_soc_update_bits(codec, PM860X_DAC_OFFSET, DAC_MUTE, data);

This isn't joined up at all with the automute code above...

> +	/* set master/slave audio interface */
> +	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> +	case SND_SOC_DAIFMT_CBM_CFM:
> +	case SND_SOC_DAIFMT_CBM_CFS:
> +		if (pm860x->dir == PM860X_CLK_DIR_OUT) {
> +			inf |= PCM_INF2_MASTER;
> +			ret = 0;

This is still setting the same register value for two different DAI
formats.  You need to fix this, and in the other set_fmt() function.

You're also missing a default: case.

> +	if ((pm860x->det.hs_shrt & SND_JACK_BTN_0)
> +		&& (shrt & (SHORT_HS1 | SHORT_HS2)))
> +		report |= SND_JACK_BTN_0;
> +
> +	if ((pm860x->det.hook_det & SND_JACK_BTN_1)
> +		&& (status & HOOK_STATUS))
> +		report |= SND_JACK_BTN_1;
> +
> +	if ((pm860x->det.lo_shrt & SND_JACK_BTN_2)
> +		&& (shrt & (SHORT_LO1 | SHORT_LO2)))
> +		report |= SND_JACK_BTN_2;

You should ideally allow the user to override these via platform data if
the buttons have specific functions.

One other thing you should do is split the jack configuration for
headphone and microphone - a system may have two physical jacks, one for
each, so you should allow the user to configure the detection onto
separate jacks if they want to.

[alsa-devel] [PATCH 1/3] ASoC: add 88pm860x codec driver

[Haojian Zhuang]

On Wed, Aug 18, 2010 at 10:19 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Aug 18, 2010 at 09:49:26PM +0800, Haojian Zhuang wrote:
>
>> + ? ? ? ? ? ? if (dac) {
>> + ? ? ? ? ? ? ? ? ? ? /* automute is set before operating DAC. Anti-pop */
>> + ? ? ? ? ? ? ? ? ? ? mutex_lock(&pm860x->mutex);
>> + ? ? ? ? ? ? ? ? ? ? pm860x->automute = 1;
>> + ? ? ? ? ? ? ? ? ? ? dac |= MODULATOR;
>> + ? ? ? ? ? ? ? ? ? ? /* mute */
>> + ? ? ? ? ? ? ? ? ? ? snd_soc_update_bits(codec, PM860X_DAC_OFFSET,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? DAC_MUTE, DAC_MUTE);
>> + ? ? ? ? ? ? ? ? ? ? snd_soc_update_bits(codec, PM860X_EAR_CTRL_2,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? RSYNC_CHANGE, RSYNC_CHANGE);
>> + ? ? ? ? ? ? ? ? ? ? mutex_unlock(&pm860x->mutex);
>> + ? ? ? ? ? ? ? ? ? ? /* update dac */
>> + ? ? ? ? ? ? ? ? ? ? snd_soc_update_bits(codec, PM860X_DAC_EN_2,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? dac, dac);
>> + ? ? ? ? ? ? }
>
>
> I still can't follow what this automute variable is supposed to be
> doing. ?In both the PMU and the PMD cases you set automute, and the fact
> that you forcibly clear it when you apply the change means that it
> doesn't look like it's doing anything I'd recognise as automute,
> suggesting that the variable is misnamed. ?It's very difficult to follow
> what all this is supposed to be doing.
>
Mute while DAC is enabled and PGA is updated. It's the purpose of
anti-pop by silicon.

> As I said last time what I'd expect to see is a user visible mute
> control which sets a variable in the driver which is then updated in the
> chip only while the DAC is powered. ?The other option is to tie the
> control to the DAC power, in which case things should be handleable in
> the widget event without the variable.
>
Why should I move the work to invoking amixer by user? Why shouldn't
it be taken by driver automatically?

>> +static int pm860x_digital_mute(struct snd_soc_dai *codec_dai, int mute)
>> +{
>> + ? ? struct snd_soc_codec *codec = codec_dai->codec;
>> + ? ? int data = 0;
>> +
>> + ? ? if (mute)
>> + ? ? ? ? ? ? data = DAC_MUTE;
>> + ? ? snd_soc_update_bits(codec, PM860X_DAC_OFFSET, DAC_MUTE, data);
>
> This isn't joined up at all with the automute code above...
>

>> + ? ? /* set master/slave audio interface */
>> + ? ? switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
>> + ? ? case SND_SOC_DAIFMT_CBM_CFM:
>> + ? ? case SND_SOC_DAIFMT_CBM_CFS:
>> + ? ? ? ? ? ? if (pm860x->dir == PM860X_CLK_DIR_OUT) {
>> + ? ? ? ? ? ? ? ? ? ? inf |= PCM_INF2_MASTER;
>> + ? ? ? ? ? ? ? ? ? ? ret = 0;
>
> This is still setting the same register value for two different DAI
> formats. ?You need to fix this, and in the other set_fmt() function.
>
These two registers are EXACTLY same. Why do I need to define two same
bit macro?

> You're also missing a default: case.
>
Please check the code. If it's default case, it'll return -EINVAL. I
didn't miss it.

>> + ? ? if ((pm860x->det.hs_shrt & SND_JACK_BTN_0)
>> + ? ? ? ? ? ? && (shrt & (SHORT_HS1 | SHORT_HS2)))
>> + ? ? ? ? ? ? report |= SND_JACK_BTN_0;
>> +
>> + ? ? if ((pm860x->det.hook_det & SND_JACK_BTN_1)
>> + ? ? ? ? ? ? && (status & HOOK_STATUS))
>> + ? ? ? ? ? ? report |= SND_JACK_BTN_1;
>> +
>> + ? ? if ((pm860x->det.lo_shrt & SND_JACK_BTN_2)
>> + ? ? ? ? ? ? && (shrt & (SHORT_LO1 | SHORT_LO2)))
>> + ? ? ? ? ? ? report |= SND_JACK_BTN_2;
>
> You should ideally allow the user to override these via platform data if
> the buttons have specific functions.
>
OK.

> One other thing you should do is split the jack configuration for
> headphone and microphone - a system may have two physical jacks, one for
> each, so you should allow the user to configure the detection onto
> separate jacks if they want to.
>
Up to now, I can't split it to two different jacks. Since there's some
issue on mic detection.

[alsa-devel] [PATCH 1/3] ASoC: add 88pm860x codec driver

[Mark Brown]

On Wed, Aug 18, 2010 at 10:33:19PM +0800, Haojian Zhuang wrote:
> On Wed, Aug 18, 2010 at 10:19 PM, Mark Brown

> >
> > I still can't follow what this automute variable is supposed to be
> > doing. ?In both the PMU and the PMD cases you set automute, and the fact
> > that you forcibly clear it when you apply the change means that it
> > doesn't look like it's doing anything I'd recognise as automute,
> > suggesting that the variable is misnamed. ?It's very difficult to follow
> > what all this is supposed to be doing.

> Mute while DAC is enabled and PGA is updated. It's the purpose of
> anti-pop by silicon.

This behaviour is entirely non-obvious from the code.  Please improve
the documentation within the code so that someone reading the code can
tell what it's supposed to do.  This will allow the code to be reviewed,
it certainly seems buggy in relation to the DAI mute operation at the
minute.

> > As I said last time what I'd expect to see is a user visible mute
> > control which sets a variable in the driver which is then updated in the
> > chip only while the DAC is powered. ?The other option is to tie the
> > control to the DAC power, in which case things should be handleable in
> > the widget event without the variable.

> Why should I move the work to invoking amixer by user? Why shouldn't
> it be taken by driver automatically?

Either of my suggestions would leave the workaround transparent to the
user.  The first suggestion will provide additional control to the user
but will still hide the workaround from them.

> >> + ? ? switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> >> + ? ? case SND_SOC_DAIFMT_CBM_CFM:
> >> + ? ? case SND_SOC_DAIFMT_CBM_CFS:
> >> + ? ? ? ? ? ? if (pm860x->dir == PM860X_CLK_DIR_OUT) {
> >> + ? ? ? ? ? ? ? ? ? ? inf |= PCM_INF2_MASTER;
> >> + ? ? ? ? ? ? ? ? ? ? ret = 0;

> > This is still setting the same register value for two different DAI
> > formats. ?You need to fix this, and in the other set_fmt() function.

> These two registers are EXACTLY same. Why do I need to define two same
> bit macro?

You need to validate the arguments you're being passed.  If your driver
reports that it successfully set _CFS and really it set the hardware
into _CFM then the system isn't going to work as expected.  

> > You're also missing a default: case.

> Please check the code. If it's default case, it'll return -EINVAL. I
> didn't miss it.

No, that's not the case.  Your error handling strategy here is
problematic - if any one of the parameters set is correct (eg, the
format mask sets I2S) then ret will be reset to zero and you'll return
success.

> > One other thing you should do is split the jack configuration for
> > headphone and microphone - a system may have two physical jacks, one for
> > each, so you should allow the user to configure the detection onto
> > separate jacks if they want to.

> Up to now, I can't split it to two different jacks. Since there's some
> issue on mic detection.

You can do this in software even if the individual board designs you're
using don't have any separation.

[alsa-devel] [PATCH 1/3] ASoC: add 88pm860x codec driver

[Haojian Zhuang]

On Wed, Aug 18, 2010 at 10:40 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Aug 18, 2010 at 10:33:19PM +0800, Haojian Zhuang wrote:
>> On Wed, Aug 18, 2010 at 10:19 PM, Mark Brown
>
>> >
>> > I still can't follow what this automute variable is supposed to be
>> > doing. ?In both the PMU and the PMD cases you set automute, and the fact
>> > that you forcibly clear it when you apply the change means that it
>> > doesn't look like it's doing anything I'd recognise as automute,
>> > suggesting that the variable is misnamed. ?It's very difficult to follow
>> > what all this is supposed to be doing.
>
>> Mute while DAC is enabled and PGA is updated. It's the purpose of
>> anti-pop by silicon.
>
> This behaviour is entirely non-obvious from the code. ?Please improve
> the documentation within the code so that someone reading the code can
> tell what it's supposed to do. ?This will allow the code to be reviewed,
> it certainly seems buggy in relation to the DAI mute operation at the
> minute.
>
I'll add comments.

>> > As I said last time what I'd expect to see is a user visible mute
>> > control which sets a variable in the driver which is then updated in the
>> > chip only while the DAC is powered. ?The other option is to tie the
>> > control to the DAC power, in which case things should be handleable in
>> > the widget event without the variable.
>
>> Why should I move the work to invoking amixer by user? Why shouldn't
>> it be taken by driver automatically?
>
> Either of my suggestions would leave the workaround transparent to the
> user. ?The first suggestion will provide additional control to the user
> but will still hide the workaround from them.
>
I'll use another way to implement digital mute. So there won't be any
confliction.

>> >> + ? ? switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
>> >> + ? ? case SND_SOC_DAIFMT_CBM_CFM:
>> >> + ? ? case SND_SOC_DAIFMT_CBM_CFS:
>> >> + ? ? ? ? ? ? if (pm860x->dir == PM860X_CLK_DIR_OUT) {
>> >> + ? ? ? ? ? ? ? ? ? ? inf |= PCM_INF2_MASTER;
>> >> + ? ? ? ? ? ? ? ? ? ? ret = 0;
>
>> > This is still setting the same register value for two different DAI
>> > formats. ?You need to fix this, and in the other set_fmt() function.
>
>> These two registers are EXACTLY same. Why do I need to define two same
>> bit macro?
>
> You need to validate the arguments you're being passed. ?If your driver
> reports that it successfully set _CFS and really it set the hardware
> into _CFM then the system isn't going to work as expected.
>
>> > You're also missing a default: case.
>
>> Please check the code. If it's default case, it'll return -EINVAL. I
>> didn't miss it.
>
> No, that's not the case. ?Your error handling strategy here is
> problematic - if any one of the parameters set is correct (eg, the
> format mask sets I2S) then ret will be reset to zero and you'll return
> success.
>
OK. I can change.

>> > One other thing you should do is split the jack configuration for
>> > headphone and microphone - a system may have two physical jacks, one for
>> > each, so you should allow the user to configure the detection onto
>> > separate jacks if they want to.
>
>> Up to now, I can't split it to two different jacks. Since there's some
>> issue on mic detection.
>
> You can do this in software even if the individual board designs you're
> using don't have any separation.
>
In my system, there's only one physical jacks. It's four phased plug
(mic,left,right,gnd). So I needn't split them into two different
jacks.

[alsa-devel] [PATCH 1/3] ASoC: add 88pm860x codec driver

[Mark Brown]

On Wed, Aug 18, 2010 at 11:23:10PM +0800, Haojian Zhuang wrote:

> In my system, there's only one physical jacks. It's four phased plug
> (mic,left,right,gnd). So I needn't split them into two different
> jacks.

Right, but the CODEC driver (which will be used by all systems with this
CODEC) should allow the user that option.