Re: [patch 1/3] ASoC: add support for alc562[123] codecs

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

On Tue, Sep 07, 2010 at 03:23:20PM +0200, Arnaud Patard wrote:
> Mark Brown <broonie@opensource.wolfsonmicro.com> writes:

> > It looks like it'd be simpler to do something like:

> > 	{ "HPL", NULL, "HP" },
> > 	{ "HPR", NULL, "HP" },

> > then have the two ADC channels feed into the left and right channels of
> > the headphone while having the single switches feed into the HP widget
> > (which would be a NOPM dummy widget for routing purposes).

> hmm... I'm not sure to understand correctly. The two bits
> ALC5623_PWR_ADD2_?_HP_MIXER are for the HP mixers. There are 2 mixers
> but I'm doing like there's only 1 mixer due to the single bit inputs.
> Can you please give more details about your suggestion ?

Add on to the above things like this:

	{ "HPL", "ADC Switch", "ADCL" },
	{ "HPR", "ADC Switch", "ADCR" },

	{ "HP", "AUX Switch", "Aux" },

so you get separate left/right control where you have stereo control
while still getting the mono control on the virtual mono HP widget.

> >> +#define ALC5623_ADD1_POWER_EN_5622 \
> >> +	(ALC5623_PWR_ADD1_MAIN_I2S_EN | ALC5623_PWR_ADD1_MIC1_BIAS_EN \
> >> +	| ALC5623_PWR_ADD1_SHORT_CURR_DET_EN \
> >> +	| ALC5623_PWR_ADD1_HP_OUT_AMP)

> > Most of the stuff in here looks like it ought to be managed by DAPM?

> hmm... I thought it was easier/better done like this. Is it a hard
> requirement ?

Given that you're already defining widgets for things like the I2S (the
AIF widgets) it seems silly not to.  For the micbias I'd say it's a hard
requirement since driving the micbias when not needed is potenially
harmful.

> > Since we now support out of line resume for ASoC devices (so we don't
> > hold up the rest of the system) it should be possible to just do this
> > stuff in the set_bias_level() function rather than faffing around with
> > delayed work like this.  This is less racy.

> I was even wondering about removing that stuff but as it was there in
> original driver, I choose to keep it. WDYT ?

If you look at the current WM8753 driver you'll see that it's been
removed from there.

> >> +	if (alc5623->add_ctrl) {
> >> +		snd_soc_write(codec, ALC5623_ADD_CTRL_REG,
> >> +				alc5623->add_ctrl);
> >> +	}

> > Hrm?

> this register contains some board specific values and I didn't see how
> it could be handled except by using platform_data.

What are they?

> >> +#ifndef _INCLUDE_LINUX_ALC5623_H
> >> +#define _INCLUDE_LINUX_ALC5623_H
> >> +struct alc5623_platform_data {
> >> +	unsigned int add_ctrl;
> >> +	unsigned int jack_det_ctrl;
> >> +};

> > Some documentation explaining what these are would probably be useful.

> The names are really near to the names in the specs. As you need to look
> at the specs to have their values, I thought it would be enough.

So saying something like "Values to be written to the add_ctrl and
jack_det_ctrl registers" would cover it.  Remember, if a software
engineer is picking up a preexisting driver they may never even look at
the datasheet.