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

(no body)

On Tue, Aug 17, 2010 at 03:47:59PM +0800, Haojian Zhuang wrote:

This looks to be unused.

Can you explain what's going on with this mute handling please?

I still don't follow why you need a custom event for this.

This mute handling looks very suspicious.

As I said last time you should handle this via DAPM.

This should be associated with enabling microphone detection.

You're setting the same register configuration for two different DAI
clock master configurations here.  Presumably one of the settings is
inaccurate?

What is this actually setting - which clock is being configured here?

It would seem better to just remove the interrupt handling support
entirely if you're not going to implement jack detection.  Right now all
the curernt code will do is waste power by enabling the feature but
ignoring the result.

On Tue, Aug 17, 2010 at 5:58 PM, Mark Brown
[off-list ref] wrote:

On Tue, Aug 17, 2010 at 03:47:59PM +0800, Haojian Zhuang wrote:

quoted

+/* DAPM Widget Events */
+static int pm860x_rsync_event(struct snd_soc_dapm_widget *w,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? struct snd_kcontrol *kcontrol, int event)
+{
+ ? ? struct snd_soc_codec *codec = w->codec;
+
+ ? ? /* unmute DAC */
+ ? ? snd_soc_update_bits(codec, PM860X_DAC_OFFSET, DAC_MUTE, 0);

Can you explain what's going on with this mute handling please?

Em. Actually there should be automute variable. I shouldn't delete
that variable.  In order to anti-pop, mute DAC before enabling DAC.
Unmute it after enabling DAC. It's required by silicon.

quoted

+ ? ? switch (event) {
+ ? ? case SND_SOC_DAPM_PRE_PMU:
+ ? ? ? ? ? ? snd_soc_update_bits(codec, PM860X_ADC_EN_1, en1, en1);
+ ? ? ? ? ? ? snd_soc_update_bits(codec, PM860X_ADC_EN_2, en2, en2);

I still don't follow why you need a custom event for this.

Enabling both bit 0 of ADC_EN_1 and bit 5 of ADC_EN_2 can enable left
ADC. Enabling both bit 1 of ADC_EN_1 and bit 4 of ADC_EN_2 can enable
right ADC. I can't find any DAPM API can handle this. So I implement
the custom event.

quoted

+static int pm860x_mic1_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_POST_PMU:
+ ? ? ? ? ? ? /* Enable Mic1 Bias & MICDET, HSDET */
+ ? ? ? ? ? ? snd_soc_update_bits(codec, PM860X_ADC_ANA_1, MIC1BIAS_MASK,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? MIC1BIAS_MASK);

As I said last time you should handle this via DAPM.

I registered it as DAPM widget. Why do you say it's not handled via DAPM?

quoted

+ ? ? ? ? ? ? pm860x_set_bits(codec->control_data, REG_MIC_DET,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? MICDET_MASK, MICDET_MASK);
+ ? ? ? ? ? ? pm860x_set_bits(codec->control_data, REG_HS_DET,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? EN_HS_DET, EN_HS_DET);

This should be associated with enabling microphone detection.

Yes, but you said that it could be controlled by enable_pin(). I
forced to enable microphone pins in machine driver.

quoted

+ ? ? /* 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;
+ ? ? ? ? ? ? else
+ ? ? ? ? ? ? ? ? ? ? return -EINVAL;
+ ? ? ? ? ? ? break;

You're setting the same register configuration for two different DAI
clock master configurations here. ?Presumably one of the settings is
inaccurate?

No, they're different registers. But offsets are same. So I just return pointer.

quoted

+static int pm860x_set_dai_sysclk(struct snd_soc_dai *codec_dai,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?int clk_id, unsigned int freq, int dir)
+{
+ ? ? struct snd_soc_codec *codec = codec_dai->codec;
+ ? ? struct pm860x_priv *pm860x = snd_soc_codec_get_drvdata(codec);
+
+ ? ? if (dir == PM860X_CLK_DIR_OUT)
+ ? ? ? ? ? ? pm860x->dir = PM860X_CLK_DIR_OUT;
+ ? ? else
+ ? ? ? ? ? ? pm860x->dir = PM860X_CLK_DIR_IN;
+
+ ? ? return 0;
+}

What is this actually setting - which clock is being configured here?

While codec is master, the clock is fixed. I needn't set detail clock.
While codec is slave, it's not supported in this patch yet.

quoted

+static irqreturn_t pm860x_codec_handler(int irq, void *data)
+{
+ ? ? struct pm860x_priv *pm860x = data;
+ ? ? int status, shrt, report = 0;
+
+ ? ? status = pm860x_reg_read(pm860x->i2c, REG_STATUS_1);
+ ? ? shrt = pm860x_reg_read(pm860x->i2c, REG_SHORTS);
+
+ ? ? if (status & HEADSET_STATUS)
+ ? ? ? ? ? ? report |= PM860X_DET_HEADSET;
+ ? ? if (status & MIC_STATUS)
+ ? ? ? ? ? ? report |= PM860X_DET_MIC;
+ ? ? if (status & HOOK_STATUS)
+ ? ? ? ? ? ? report |= PM860X_DET_HOOK;
+ ? ? if (shrt & (SHORT_LO1 | SHORT_LO2))
+ ? ? ? ? ? ? report |= PM860X_SHORT_LINEOUT;
+ ? ? if (shrt & (SHORT_HS1 | SHORT_HS2))
+ ? ? ? ? ? ? report |= PM860X_SHORT_HEADSET;
+ ? ? dev_dbg(pm860x->codec->dev, "report:0x%x\n", report);
+ ? ? return IRQ_HANDLED;

It would seem better to just remove the interrupt handling support
entirely if you're not going to implement jack detection. ?Right now all
the curernt code will do is waste power by enabling the feature but
ignoring the result.

I need a document on illustrating jack on alsa. Could you share one?

On Tue, Aug 17, 2010 at 06:44:44PM +0800, Haojian Zhuang wrote:

On Tue, Aug 17, 2010 at 5:58 PM, Mark Brown

quoted

quoted

+ ? ? /* unmute DAC */
+ ? ? snd_soc_update_bits(codec, PM860X_DAC_OFFSET, DAC_MUTE, 0);

quoted

Can you explain what's going on with this mute handling please?

Em. Actually there should be automute variable. I shouldn't delete
that variable.  In order to anti-pop, mute DAC before enabling DAC.
Unmute it after enabling DAC. It's required by silicon.

As requested in the previous review comments you need to provide better
documentation of this in the driver.  I'd also expect to see a custom
mute control somewhere which implements this by keeping the DAC muted
unless it has been enabled rather than some sort of automute handling.

quoted

quoted

+ ? ? switch (event) {
+ ? ? case SND_SOC_DAPM_PRE_PMU:
+ ? ? ? ? ? ? snd_soc_update_bits(codec, PM860X_ADC_EN_1, en1, en1);
+ ? ? ? ? ? ? snd_soc_update_bits(codec, PM860X_ADC_EN_2, en2, en2);

quoted

I still don't follow why you need a custom event for this.

Enabling both bit 0 of ADC_EN_1 and bit 5 of ADC_EN_2 can enable left
ADC. Enabling both bit 1 of ADC_EN_1 and bit 4 of ADC_EN_2 can enable
right ADC. I can't find any DAPM API can handle this. So I implement
the custom event.

As I said previously I would expect you to be using a DAPM supply widget
for this.

quoted

quoted

+static int pm860x_mic1_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_POST_PMU:
+ ? ? ? ? ? ? /* Enable Mic1 Bias & MICDET, HSDET */
+ ? ? ? ? ? ? snd_soc_update_bits(codec, PM860X_ADC_ANA_1, MIC1BIAS_MASK,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? MIC1BIAS_MASK);

quoted

As I said last time you should handle this via DAPM.

I registered it as DAPM widget. Why do you say it's not handled via DAPM?

I say this because this code is manually going in and enabling the
microphone bias here.  If the microphone bias is being handled using
DAPM you shouldn't be going around explicitly writing to the power
control bit like this.

quoted

quoted

+ ? ? ? ? ? ? pm860x_set_bits(codec->control_data, REG_MIC_DET,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? MICDET_MASK, MICDET_MASK);
+ ? ? ? ? ? ? pm860x_set_bits(codec->control_data, REG_HS_DET,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? EN_HS_DET, EN_HS_DET);

quoted

This should be associated with enabling microphone detection.

Yes, but you said that it could be controlled by enable_pin(). I
forced to enable microphone pins in machine driver.

I don't follow what you're saying at all.  This code will unconditonally
enable headset and microphone detection while the microphone is enabled,
regardless of the user deciding if they want to use that feature or not.
The use of enable_pin() was for the microphone bias which is a separate
thing.

quoted

quoted

+ ? ? /* 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;
+ ? ? ? ? ? ? else
+ ? ? ? ? ? ? ? ? ? ? return -EINVAL;
+ ? ? ? ? ? ? break;

quoted

You're setting the same register configuration for two different DAI
clock master configurations here. ?Presumably one of the settings is
inaccurate?

No, they're different registers. But offsets are same. So I just return pointer.

I can't associate your comment there with the code at all.  The code
does nothing different for the two case statements and there's no other
code I can see.

quoted

quoted

+static int pm860x_set_dai_sysclk(struct snd_soc_dai *codec_dai,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?int clk_id, unsigned int freq, int dir)
+{
+ ? ? struct snd_soc_codec *codec = codec_dai->codec;
+ ? ? struct pm860x_priv *pm860x = snd_soc_codec_get_drvdata(codec);
+
+ ? ? if (dir == PM860X_CLK_DIR_OUT)
+ ? ? ? ? ? ? pm860x->dir = PM860X_CLK_DIR_OUT;
+ ? ? else
+ ? ? ? ? ? ? pm860x->dir = PM860X_CLK_DIR_IN;
+
+ ? ? return 0;
+}

quoted

What is this actually setting - which clock is being configured here?

While codec is master, the clock is fixed. I needn't set detail clock.
While codec is slave, it's not supported in this patch yet.

What is "the clock", and if slave mode is not supported surely an error
should be returned?

quoted

quoted

+ ? ? if (shrt & (SHORT_LO1 | SHORT_LO2))
+ ? ? ? ? ? ? report |= PM860X_SHORT_LINEOUT;
+ ? ? if (shrt & (SHORT_HS1 | SHORT_HS2))
+ ? ? ? ? ? ? report |= PM860X_SHORT_HEADSET;
+ ? ? dev_dbg(pm860x->codec->dev, "report:0x%x\n", report);
+ ? ? return IRQ_HANDLED;

quoted

It would seem better to just remove the interrupt handling support
entirely if you're not going to implement jack detection. ?Right now all
the curernt code will do is waste power by enabling the feature but
ignoring the result.

I need a document on illustrating jack on alsa. Could you share one?

There's a number of in tree examples - seach for snd_soc_jack.

On Tue, Aug 17, 2010 at 7:02 PM, Mark Brown
[off-list ref] wrote:

On Tue, Aug 17, 2010 at 06:44:44PM +0800, Haojian Zhuang wrote:

quoted

On Tue, Aug 17, 2010 at 5:58 PM, Mark Brown

quoted

quoted

quoted

+ ? ? switch (event) {
+ ? ? case SND_SOC_DAPM_PRE_PMU:
+ ? ? ? ? ? ? snd_soc_update_bits(codec, PM860X_ADC_EN_1, en1, en1);
+ ? ? ? ? ? ? snd_soc_update_bits(codec, PM860X_ADC_EN_2, en2, en2);

quoted

quoted

I still don't follow why you need a custom event for this.

quoted

Enabling both bit 0 of ADC_EN_1 and bit 5 of ADC_EN_2 can enable left
ADC. Enabling both bit 1 of ADC_EN_1 and bit 4 of ADC_EN_2 can enable
right ADC. I can't find any DAPM API can handle this. So I implement
the custom event.

As I said previously I would expect you to be using a DAPM supply widget
for this.

Supply widget should be always on while DAPM is working. How about
only enable left ADC?

quoted

quoted

quoted

+ ? ? /* 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;
+ ? ? ? ? ? ? else
+ ? ? ? ? ? ? ? ? ? ? return -EINVAL;
+ ? ? ? ? ? ? break;

quoted

quoted

You're setting the same register configuration for two different DAI
clock master configurations here. ?Presumably one of the settings is
inaccurate?

quoted

No, they're different registers. But offsets are same. So I just return pointer.

I can't associate your comment there with the code at all. ?The code
does nothing different for the two case statements and there's no other
code I can see.

You can find pm860x_pcm_set_dai_fmt() and pm860x_i2s_set_dai_fmt().
Both functions called set_dai_fmt(). In set_dai_fmt(), all bit
operations are based on variable, not registers. Register operation is
implemented in pcm_set_dai_fmt() and i2s_set_dai_fmt().

quoted

quoted

quoted

+ ? ? if (shrt & (SHORT_LO1 | SHORT_LO2))
+ ? ? ? ? ? ? report |= PM860X_SHORT_LINEOUT;
+ ? ? if (shrt & (SHORT_HS1 | SHORT_HS2))
+ ? ? ? ? ? ? report |= PM860X_SHORT_HEADSET;
+ ? ? dev_dbg(pm860x->codec->dev, "report:0x%x\n", report);
+ ? ? return IRQ_HANDLED;

quoted

quoted

It would seem better to just remove the interrupt handling support
entirely if you're not going to implement jack detection. ?Right now all
the curernt code will do is waste power by enabling the feature but
ignoring the result.

quoted

I need a document on illustrating jack on alsa. Could you share one?

There's a number of in tree examples - seach for snd_soc_jack.

OK. How to test it? Still no document?

On Tue, Aug 17, 2010 at 07:37:21PM +0800, Haojian Zhuang wrote:

On Tue, Aug 17, 2010 at 7:02 PM, Mark Brown

quoted

As I said previously I would expect you to be using a DAPM supply widget
for this.

Supply widget should be always on while DAPM is working. How about
only enable left ADC?

What makes you say this?  A supply widget is only enabled when the
things it supplies are enabled.

quoted

I can't associate your comment there with the code at all. ?The code
does nothing different for the two case statements and there's no other
code I can see.

You can find pm860x_pcm_set_dai_fmt() and pm860x_i2s_set_dai_fmt().
Both functions called set_dai_fmt(). In set_dai_fmt(), all bit
operations are based on variable, not registers. Register operation is
implemented in pcm_set_dai_fmt() and i2s_set_dai_fmt().

You need to move this code into the individual functions.  What you've
got now is clearly buggy - it will accept either configuration when it
sets the variable.

quoted

quoted

quoted

entirely if you're not going to implement jack detection. ?Right now all
the curernt code will do is waste power by enabling the feature but
ignoring the result.

quoted

quoted

I need a document on illustrating jack on alsa. Could you share one?

quoted

There's a number of in tree examples - seach for snd_soc_jack.

OK. How to test it? Still no document?

A standard API input test application such as evtest can be used.