Re: [PATCH] ASoC: Add support for TI LM49453 Audio codec

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Wed, Jan 04, 2012 at 03:11:03AM -0800, Reddy, MR Swami wrote:
> On Wednesday, January 04, 2012 2:48 AM, Mark Brown wrote:

Your mail client appears to be very broken, it's reflowed all my text to
remove line breaks which does nothing for legibility.

> >> +       SND_SOC_DAPM_OUT_DRV("Sidetone DAC HPR Playback",
> >> +                             LM49453_P0_STN_SEL_REG, 1, 0, NULL, 0),

> >What exactly do these register bits do?  The use of OUT_DRV widgets looks very suspicious for a sidetone, an output driver would usually be a power
> >amplifier or transducer driver so I suspect the power sequencing will be all wrong.  It looks like these are mixer input controls which wouldn't
> >normally be widgets at all.

> The above selects the Sidetone onto left and right HP DAC output. Sidetone is not a mixer control.

What makes you say that the sidetone is not a mixer?  That's generally
exactly what a sidetone is - mixing an input into an output, usually
with a lot of attenuation.  What you're describing above just sounds
like a normal sidetone, why have you done these as output driver widgets?

> >> +       { "AMIC1Bias", NULL, "AMIC1"},
> >> +       { "Mic1_Input Capture Route", "MIC1", "AMIC1Bias"},

> >Your CODEC driver should not be connecting the microphone biases, the connections are board specific.

> The above Micbias is internal to codec. Please advice.

You're saying that the CODEC has a MEMS microphone built into it?

> >That memset is just going to mask warnings, it shouldn't be needed.

> OK.

Generally it's OK to just make updates in new versions without
explicitly commenting on each one, this makes it much easier to see
areas of your reply that are signifigant.

> >> +       snd_soc_write(codec, LM49453_P0_ADC_CLK_DIV_REG, val);
> >> +       snd_soc_write(codec, LM49453_P0_DAC_OT_CLK_DIV_REG, val);
> >> +       snd_soc_write(codec, LM49453_P0_DAC_HP_CLK_DIV_REG, val);

> >...and frankly I've no idea what this code is doing, I'd expect a clock divider configuration to vary depending on the >input clock as well as the sample rate.

> The 'val' is the clock divider value based on the sampling rate.

If these are clock dividers shouldn't they also depend on the input
clock?

> >> +       case 19000000:
> >> +               val = 0;
> >> +               break;
> >> +       case 48000:
> >> +       case 32576:
> >> +               val = 1;
> >> +               break;
> >> +       default:
> >> +               return -EINVAL;
> >> +       }

> >> +       snd_soc_update_bits(codec, LM49453_P0_PMC_SETUP_REG, BIT(4),
> >> + val);

> >I don't think this does what you think it does, your mask and value don't overlap at all.

> In the above _update_bits(), the LM49453_P0_PMC_SETUP_REG register's  4th bit is set, based on the 'val'. Please let me know, if something wrong there.

Please read what I wrote above.

> >> +       snd_soc_update_bits(dai->codec, LM49453_P0_DAC_DSP_REG, BIT(0),
> >> +                           (!!mute << 0));
> >> +       snd_soc_update_bits(dai->codec, LM49453_P0_DAC_DSP_REG, BIT(1),
> >> +                           (!!mute << 1));

> >Why are there two separate update bits operations here?

> For muting the HP left and right channels using BIT(0) and BIT(1).

That doesn't answer my question, my question was why there are two
*separate* update_bits() operations.