Re: [PATCH 7/8] ASoC: codecs: Add AB8500 codec-driver

Re: [PATCH 7/8] ASoC: codecs: Add AB8500 codec-driver

[Mark Brown]

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

On Fri, Apr 20, 2012 at 11:33:22AM +0200, Ola Lilja wrote:

This is massively better than previous versions!  There's still some
issues but hopefully not hard to correct.

> @@ -12,6 +12,7 @@ config SND_SOC_ALL_CODECS
>  	tristate "Build all ASoC CODEC drivers"
>  	select SND_SOC_88PM860X if MFD_88PM860X
>  	select SND_SOC_L3
> +	select SND_SOC_AB8500_CODEC if SND_SOC_UX500

Shouldn't this depend on the MFD core for the device instead?

> +static int regulators_init(struct device *dev)
> +{
> +	struct ab8500_codec_drvdata *drvdata = dev_get_drvdata(dev);
> +
> +	dev_dbg(dev, "%s: Enter\n", __func__);
> +
> +	regulators_get_regulator(dev, &drvdata->reg_vaud, "V-AUD");
> +	regulators_get_regulator(dev, &drvdata->reg_vamic1, "V-AMIC1");
> +	regulators_get_regulator(dev, &drvdata->reg_vamic2, "V-AMIC2");
> +	regulators_get_regulator(dev, &drvdata->reg_vdmic, "V-DMIC");
> +
> +	if (IS_ERR(drvdata->reg_vaud.consumer) ||
> +		IS_ERR(drvdata->reg_vamic1.consumer) ||
> +		IS_ERR(drvdata->reg_vamic2.consumer) ||
> +		IS_ERR(drvdata->reg_vdmic.consumer)) {
> +		regulators_cleanup(drvdata);
> +		return -ENXIO;
> +	}

This won't work with probe deferral - if we need to defer then the
driver will squash down the -EPROBE_DEFER and fail totally.  Not a big
deal, though.

> +static int dapm_audioclk_event(struct snd_soc_dapm_widget *w,
> +			struct snd_kcontrol *k, int event)
> +{

We should add a variant of REGULATOR_SUPPLY for clocks too, have a
framework thing that will own the clock - this is all generic code which
gets repeated for each clock, we could easily factor it out into the
core.

> +static int dapm_audioreg_event(struct snd_soc_dapm_widget *w,
> +			struct snd_kcontrol *k, int event)
> +{
> +	struct device *dev = w->codec->dev;
> +	struct ab8500_codec_drvdata *drvdata = dev_get_drvdata(dev);
> +	int status = 0;
> +
> +	if (SND_SOC_DAPM_EVENT_ON(event))
> +		status = regulator_enable(drvdata->reg_vaud.consumer);
> +	else
> +		regulator_disable(drvdata->reg_vaud.consumer);

Similarly this is just a REGULATOR_SUPPLY.

> +/* Mic 1b - Regulator select */
> +static const struct soc_enum enum_mic1breg_sel = SOC_ENUM_SINGLE(0, 0, 2,
> +								enum_micreg);
> +static const struct snd_kcontrol_new dapm_mic1breg_mux[] = {
> +				SOC_DAPM_ENUM_VIRT("Mic 1b Regulator Select",
> +						enum_mic1breg_sel),
> +};

Can you explain how this hardware works in more detail?  It seems very
odd to be changing the regulator used to supply something at runtime.

> +static int mclk_input_control_put(struct snd_kcontrol *kcontrol,
> +				struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
> +	struct ab8500_codec_drvdata *drvdata = dev_get_drvdata(codec->dev);
> +	unsigned int val;
> +
> +	val = (ucontrol->value.enumerated.item[0] != 0);
> +	if (drvdata->mclk_sel == val)
> +		return 0;
> +
> +	drvdata->mclk_sel = val;
> +
> +	return 1;
> +}

This is really weird

> +static const char * const enum_earselcm[] = {"0.95V", "1.10V", "1.27V",
> +					"1.58V"};
> +static SOC_ENUM_SINGLE_DECL(soc_enum_earselcm,
> +	AB8500_ANACONF1, AB8500_ANACONF1_EARSELCM, enum_earselcm);

Some of this stuff looks awfully like it ought to be platform data...

> +static const char * const enum_ensemicx[] = {"Differential", "Single Ended"};
> +static SOC_ENUM_SINGLE_DECL(soc_enum_ensemic1,
> +	AB8500_ANAGAIN1, AB8500_ANAGAINX_ENSEMICX, enum_ensemicx);
> +static SOC_ENUM_SINGLE_DECL(soc_enum_lowpowmic1,
> +	AB8500_ANAGAIN1, AB8500_ANAGAINX_LOWPOWMICX, enum_dis_ena);

This for example is normally fixed by the physical design and can't
sensibly be varied at runtime.

> +	/* Digital interface - AD to slot mapping */
> +	SOC_ENUM("Digital Interface AD To Slot 0 Map", soc_enum_adslot0map),
> +	SOC_ENUM("Digital Interface AD To Slot 1 Map", soc_enum_adslot1map),

Can you usefully leave these fixed with a default configuration?
There's been some chat about adding a framework feature for this but
it's not there yet - lots of devices have similar features.  If not then
this code is fine.

> +static int ab8500_codec_configure_audio_macrocell(struct snd_soc_codec *codec)
> +{
> +	u8 value8;
> +	unsigned int value;
> +	int status;
> +
> +	status = ab8500_sysctrl_write(AB8500_STW4500CTRL3,
> +				AB8500_STW4500CTRL3_CLK32KOUT2DIS |
> +					AB8500_STW4500CTRL3_RESETAUDN,
> +				AB8500_STW4500CTRL3_RESETAUDN);
> +	if (status < 0)
> +		return status;
> +
> +	status = abx500_get_register_interruptible(codec->dev, (u8)AB8500_MISC,
> +						(u8)AB8500_GPIO_DIR4_REG,
> +						&value8);
> +	if (status < 0)
> +		return status;
> +	value = value8 | GPIO27_DIR_OUTPUT | GPIO29_DIR_OUTPUT |
> +		GPIO31_DIR_OUTPUT;
> +	status |= abx500_set_register_interruptible(codec->dev,
> +						(u8)AB8500_MISC,
> +						(u8)AB8500_GPIO_DIR4_REG,
> +						value);

Still not sure why this isn't platform data.

> +	/* Add controls */
> +	status = snd_soc_add_codec_controls(codec, ab8500_ctrls,
> +				ARRAY_SIZE(ab8500_ctrls));
> +	if (status < 0) {
> +		dev_err(dev, "%s: failed to add codec-controls (%d).\n",
> +			__func__, status);
> +		return status;
> +	}

At least this one could be done from the driver struct.

> +	/* Add DAPM-widgets */
> +	status = snd_soc_dapm_new_controls(&codec->dapm, ab8500_dapm_widgets,
> +			ARRAY_SIZE(ab8500_dapm_widgets));
> +	if (status < 0) {
> +		dev_err(codec->dev,
> +			"%s: Failed to create DAPM controls (%d).\n",
> +			__func__, status);
> +		return status;
> +	}

This could also be done from the driver struct.

> +	status = snd_soc_dapm_add_routes(&codec->dapm, ab8500_dapm_routes,
> +					ARRAY_SIZE(ab8500_dapm_routes));
> +	if (status < 0) {
> +		dev_err(codec->dev, "%s: Failed to add DAPM routes (%d).\n",
> +			__func__, status);
> +		return status;
> +	}

This too.

> +	if (IS_ERR(drvdata->clk_ptr_sysclk)) {
> +		dev_err(dev,
> +			"%s: ERROR: Clocks needed for streaming not available!",
> +			__func__);
> +		return -ENXIO;

return PTR_ERR().

> +	dev_info(&pdev->dev, "%s: Register codec.\n", __func__);

Remove this or downgrade to debug, it's noisy and not adding much
information.

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

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

Re: [PATCH 7/8] ASoC: codecs: Add AB8500 codec-driver

[Ola Lilja]

On 04/23/2012 08:59 PM, Mark Brown wrote:

> On Fri, Apr 20, 2012 at 11:33:22AM +0200, Ola Lilja wrote:
> 
> This is massively better than previous versions!  There's still some
> issues but hopefully not hard to correct.


Thanks :)

> 
>> @@ -12,6 +12,7 @@ config SND_SOC_ALL_CODECS
>>  	tristate "Build all ASoC CODEC drivers"
>>  	select SND_SOC_88PM860X if MFD_88PM860X
>>  	select SND_SOC_L3
>> +	select SND_SOC_AB8500_CODEC if SND_SOC_UX500
> 
> Shouldn't this depend on the MFD core for the device instead?


Hmm, that might be better, yes.

> 
>> +static int regulators_init(struct device *dev)
>> +{
>> +	struct ab8500_codec_drvdata *drvdata = dev_get_drvdata(dev);
>> +
>> +	dev_dbg(dev, "%s: Enter\n", __func__);
>> +
>> +	regulators_get_regulator(dev, &drvdata->reg_vaud, "V-AUD");
>> +	regulators_get_regulator(dev, &drvdata->reg_vamic1, "V-AMIC1");
>> +	regulators_get_regulator(dev, &drvdata->reg_vamic2, "V-AMIC2");
>> +	regulators_get_regulator(dev, &drvdata->reg_vdmic, "V-DMIC");
>> +
>> +	if (IS_ERR(drvdata->reg_vaud.consumer) ||
>> +		IS_ERR(drvdata->reg_vamic1.consumer) ||
>> +		IS_ERR(drvdata->reg_vamic2.consumer) ||
>> +		IS_ERR(drvdata->reg_vdmic.consumer)) {
>> +		regulators_cleanup(drvdata);
>> +		return -ENXIO;
>> +	}
> 
> This won't work with probe deferral - if we need to defer then the
> driver will squash down the -EPROBE_DEFER and fail totally.  Not a big
> deal, though.


Hmm, OK, how should I solve this?

> 
>> +static int dapm_audioclk_event(struct snd_soc_dapm_widget *w,
>> +			struct snd_kcontrol *k, int event)
>> +{
> 
> We should add a variant of REGULATOR_SUPPLY for clocks too, have a
> framework thing that will own the clock - this is all generic code which
> gets repeated for each clock, we could easily factor it out into the
> core.


OK, I'll look into this.

> 
>> +static int dapm_audioreg_event(struct snd_soc_dapm_widget *w,
>> +			struct snd_kcontrol *k, int event)
>> +{
>> +	struct device *dev = w->codec->dev;
>> +	struct ab8500_codec_drvdata *drvdata = dev_get_drvdata(dev);
>> +	int status = 0;
>> +
>> +	if (SND_SOC_DAPM_EVENT_ON(event))
>> +		status = regulator_enable(drvdata->reg_vaud.consumer);
>> +	else
>> +		regulator_disable(drvdata->reg_vaud.consumer);
> 
> Similarly this is just a REGULATOR_SUPPLY.


Is there already support for this in the core?

> 
>> +/* Mic 1b - Regulator select */
>> +static const struct soc_enum enum_mic1breg_sel = SOC_ENUM_SINGLE(0, 0, 2,
>> +								enum_micreg);
>> +static const struct snd_kcontrol_new dapm_mic1breg_mux[] = {
>> +				SOC_DAPM_ENUM_VIRT("Mic 1b Regulator Select",
>> +						enum_mic1breg_sel),
>> +};
> 
> Can you explain how this hardware works in more detail?  It seems very
> odd to be changing the regulator used to supply something at runtime.


Yes, different customers have different regulators tied to the different mics.
E.g. VMIC1 can be used with MIC2 etc. These settings are stored in a DB in
userspace and therefor we have it as a control reachable from userspace, but I
see the point that this could also be put in the mach-folder and thus fixed
during build-time.

> 
>> +static int mclk_input_control_put(struct snd_kcontrol *kcontrol,
>> +				struct snd_ctl_elem_value *ucontrol)
>> +{
>> +	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
>> +	struct ab8500_codec_drvdata *drvdata = dev_get_drvdata(codec->dev);
>> +	unsigned int val;
>> +
>> +	val = (ucontrol->value.enumerated.item[0] != 0);
>> +	if (drvdata->mclk_sel == val)
>> +		return 0;
>> +
>> +	drvdata->mclk_sel = val;
>> +
>> +	return 1;
>> +}
> 
> This is really weird


This is a way of selecting what clock is to be used during a plaback/capture.
It cannot direclty turn on/off the clock because this is not allowed for
power-saving reasons. Also, it needs to be accessible from user-space as we have
code in user-space which makes this decision during runtime.

> 
>> +static const char * const enum_earselcm[] = {"0.95V", "1.10V", "1.27V",
>> +					"1.58V"};
>> +static SOC_ENUM_SINGLE_DECL(soc_enum_earselcm,
>> +	AB8500_ANACONF1, AB8500_ANACONF1_EARSELCM, enum_earselcm);
> 
> Some of this stuff looks awfully like it ought to be platform data...


This information is also stored as parameters in the database, which customers
can control. I could hard-code the value in the machine-driver...

> 
>> +static const char * const enum_ensemicx[] = {"Differential", "Single Ended"};
>> +static SOC_ENUM_SINGLE_DECL(soc_enum_ensemic1,
>> +	AB8500_ANAGAIN1, AB8500_ANAGAINX_ENSEMICX, enum_ensemicx);
>> +static SOC_ENUM_SINGLE_DECL(soc_enum_lowpowmic1,
>> +	AB8500_ANAGAIN1, AB8500_ANAGAINX_LOWPOWMICX, enum_dis_ena);
> 
> This for example is normally fixed by the physical design and can't
> sensibly be varied at runtime.


The same goes here... customers set this value in the database to match their
board-design. It is not a matter of being able to reconfigure this during
runtime but it is stored in the database, being one common place for most
settings; runtime or one-time-settings.
Also this could be put as a hard-coded parameter in the machine-driver.

> 
>> +	/* Digital interface - AD to slot mapping */
>> +	SOC_ENUM("Digital Interface AD To Slot 0 Map", soc_enum_adslot0map),
>> +	SOC_ENUM("Digital Interface AD To Slot 1 Map", soc_enum_adslot1map),
> 
> Can you usefully leave these fixed with a default configuration?
> There's been some chat about adding a framework feature for this but
> it's not there yet - lots of devices have similar features.  If not then
> this code is fine.


For the moment we need to be able to change what slots in the I2S-IF is mapped
to what AD/DA. In the future we might change it so that it can be fixed in the
kernel-space world, though.

> 
>> +static int ab8500_codec_configure_audio_macrocell(struct snd_soc_codec *codec)
>> +{
>> +	u8 value8;
>> +	unsigned int value;
>> +	int status;
>> +
>> +	status = ab8500_sysctrl_write(AB8500_STW4500CTRL3,
>> +				AB8500_STW4500CTRL3_CLK32KOUT2DIS |
>> +					AB8500_STW4500CTRL3_RESETAUDN,
>> +				AB8500_STW4500CTRL3_RESETAUDN);
>> +	if (status < 0)
>> +		return status;
>> +
>> +	status = abx500_get_register_interruptible(codec->dev, (u8)AB8500_MISC,
>> +						(u8)AB8500_GPIO_DIR4_REG,
>> +						&value8);
>> +	if (status < 0)
>> +		return status;
>> +	value = value8 | GPIO27_DIR_OUTPUT | GPIO29_DIR_OUTPUT |
>> +		GPIO31_DIR_OUTPUT;
>> +	status |= abx500_set_register_interruptible(codec->dev,
>> +						(u8)AB8500_MISC,
>> +						(u8)AB8500_GPIO_DIR4_REG,
>> +						value);
> 
> Still not sure why this isn't platform data.


We never change these values, but I could put them as platform-data.

> 
>> +	/* Add controls */
>> +	status = snd_soc_add_codec_controls(codec, ab8500_ctrls,
>> +				ARRAY_SIZE(ab8500_ctrls));
>> +	if (status < 0) {
>> +		dev_err(dev, "%s: failed to add codec-controls (%d).\n",
>> +			__func__, status);
>> +		return status;
>> +	}
> 
> At least this one could be done from the driver struct.


OK!

> 
>> +	/* Add DAPM-widgets */
>> +	status = snd_soc_dapm_new_controls(&codec->dapm, ab8500_dapm_widgets,
>> +			ARRAY_SIZE(ab8500_dapm_widgets));
>> +	if (status < 0) {
>> +		dev_err(codec->dev,
>> +			"%s: Failed to create DAPM controls (%d).\n",
>> +			__func__, status);
>> +		return status;
>> +	}
> 
> This could also be done from the driver struct.


OK!

> 
>> +	status = snd_soc_dapm_add_routes(&codec->dapm, ab8500_dapm_routes,
>> +					ARRAY_SIZE(ab8500_dapm_routes));
>> +	if (status < 0) {
>> +		dev_err(codec->dev, "%s: Failed to add DAPM routes (%d).\n",
>> +			__func__, status);
>> +		return status;
>> +	}
> 
> This too.


OK!

> 
>> +	if (IS_ERR(drvdata->clk_ptr_sysclk)) {
>> +		dev_err(dev,
>> +			"%s: ERROR: Clocks needed for streaming not available!",
>> +			__func__);
>> +		return -ENXIO;
> 
> return PTR_ERR().


OK.

> 
>> +	dev_info(&pdev->dev, "%s: Register codec.\n", __func__);
> 
> Remove this or downgrade to debug, it's noisy and not adding much
> information.


OK.

Re: [PATCH 7/8] ASoC: codecs: Add AB8500 codec-driver

[Mark Brown]

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

On Fri, Apr 27, 2012 at 11:19:26AM +0200, Ola Lilja wrote:
> On 04/23/2012 08:59 PM, Mark Brown wrote:

> > This won't work with probe deferral - if we need to defer then the
> > driver will squash down the -EPROBE_DEFER and fail totally.  Not a big
> > deal, though.

> Hmm, OK, how should I solve this?

Just passing back the first error you get should do the trick.

> > Similarly this is just a REGULATOR_SUPPLY.

> Is there already support for this in the core?

If you search the code for REGULATOR_SUPPLY... :)

> >> +static const struct soc_enum enum_mic1breg_sel = SOC_ENUM_SINGLE(0, 0, 2,
> >> +								enum_micreg);
> >> +static const struct snd_kcontrol_new dapm_mic1breg_mux[] = {
> >> +				SOC_DAPM_ENUM_VIRT("Mic 1b Regulator Select",
> >> +						enum_mic1breg_sel),
> >> +};

> > Can you explain how this hardware works in more detail?  It seems very
> > odd to be changing the regulator used to supply something at runtime.

> Yes, different customers have different regulators tied to the different mics.
> E.g. VMIC1 can be used with MIC2 etc. These settings are stored in a DB in
> userspace and therefor we have it as a control reachable from userspace, but I
> see the point that this could also be put in the mach-folder and thus fixed
> during build-time.

What are these regulators?  Are they internal PGA supplies or are they
microphone biases?

> >> +static int mclk_input_control_put(struct snd_kcontrol *kcontrol,
> >> +				struct snd_ctl_elem_value *ucontrol)
> >> +{
> >> +	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
> >> +	struct ab8500_codec_drvdata *drvdata = dev_get_drvdata(codec->dev);
> >> +	unsigned int val;
> >> +
> >> +	val = (ucontrol->value.enumerated.item[0] != 0);
> >> +	if (drvdata->mclk_sel == val)
> >> +		return 0;
> >> +
> >> +	drvdata->mclk_sel = val;
> >> +
> >> +	return 1;
> >> +}

> > This is really weird

> This is a way of selecting what clock is to be used during a plaback/capture.
> It cannot direclty turn on/off the clock because this is not allowed for
> power-saving reasons. Also, it needs to be accessible from user-space as we have
> code in user-space which makes this decision during runtime.

Normally the clocking control is under the control of the machine driver
and if the machine driver wants to offer any options to userspace it'd
provide its own control - usually there's way more stuff going on here
than just selecting a source and much more coordination needed with the
drivers involved.

> The same goes here... customers set this value in the database to match their
> board-design. It is not a matter of being able to reconfigure this during
> runtime but it is stored in the database, being one common place for most
> settings; runtime or one-time-settings.
> Also this could be put as a hard-coded parameter in the machine-driver.

Or put in platform data as I've repeatedly suggested.  This is the
standard way of providing per-board fixed configuration to devices in
Linux, the fact that you have a proprietary mechanism for doing this
shouldn't really influence upstream too much.

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

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

Re: [PATCH 7/8] ASoC: codecs: Add AB8500 codec-driver

[Ola Lilja]

On 04/27/2012 11:35 AM, Mark Brown wrote:

> On Fri, Apr 27, 2012 at 11:19:26AM +0200, Ola Lilja wrote:
>> On 04/23/2012 08:59 PM, Mark Brown wrote:
> 
>> > This won't work with probe deferral - if we need to defer then the
>> > driver will squash down the -EPROBE_DEFER and fail totally.  Not a big
>> > deal, though.
> 
>> Hmm, OK, how should I solve this?
> 
> Just passing back the first error you get should do the trick.
> 
>> > Similarly this is just a REGULATOR_SUPPLY.
> 
>> Is there already support for this in the core?
> 
> If you search the code for REGULATOR_SUPPLY... :)
> 
>> >> +static const struct soc_enum enum_mic1breg_sel = SOC_ENUM_SINGLE(0, 0, 2,
>> >> +								enum_micreg);
>> >> +static const struct snd_kcontrol_new dapm_mic1breg_mux[] = {
>> >> +				SOC_DAPM_ENUM_VIRT("Mic 1b Regulator Select",
>> >> +						enum_mic1breg_sel),
>> >> +};
> 
>> > Can you explain how this hardware works in more detail?  It seems very
>> > odd to be changing the regulator used to supply something at runtime.
> 
>> Yes, different customers have different regulators tied to the different mics.
>> E.g. VMIC1 can be used with MIC2 etc. These settings are stored in a DB in
>> userspace and therefor we have it as a control reachable from userspace, but I
>> see the point that this could also be put in the mach-folder and thus fixed
>> during build-time.
> 
> What are these regulators?  Are they internal PGA supplies or are they
> microphone biases?


These are mic-biases.

> 
>> >> +static int mclk_input_control_put(struct snd_kcontrol *kcontrol,
>> >> +				struct snd_ctl_elem_value *ucontrol)
>> >> +{
>> >> +	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
>> >> +	struct ab8500_codec_drvdata *drvdata = dev_get_drvdata(codec->dev);
>> >> +	unsigned int val;
>> >> +
>> >> +	val = (ucontrol->value.enumerated.item[0] != 0);
>> >> +	if (drvdata->mclk_sel == val)
>> >> +		return 0;
>> >> +
>> >> +	drvdata->mclk_sel = val;
>> >> +
>> >> +	return 1;
>> >> +}
> 
>> > This is really weird
> 
>> This is a way of selecting what clock is to be used during a plaback/capture.
>> It cannot direclty turn on/off the clock because this is not allowed for
>> power-saving reasons. Also, it needs to be accessible from user-space as we have
>> code in user-space which makes this decision during runtime.
> 
> Normally the clocking control is under the control of the machine driver
> and if the machine driver wants to offer any options to userspace it'd
> provide its own control - usually there's way more stuff going on here
> than just selecting a source and much more coordination needed with the
> drivers involved.


Yes, we are only selecting what clock to request to the clock-driver-code, which
then does its magic and taking decisions related to the whole platform.
More specifically, in my context (that is audio) we will act upon being in
voicecall or not, and in these cases requesting these two different clocks. By
design this request comes from audio-code in userspace down to the audio-driver
in the kernel. The clock-code will then turn of the clock not needed by us
(audio) or any other party.

> 
>> The same goes here... customers set this value in the database to match their
>> board-design. It is not a matter of being able to reconfigure this during
>> runtime but it is stored in the database, being one common place for most
>> settings; runtime or one-time-settings.
>> Also this could be put as a hard-coded parameter in the machine-driver.
> 
> Or put in platform data as I've repeatedly suggested.  This is the
> standard way of providing per-board fixed configuration to devices in
> Linux, the fact that you have a proprietary mechanism for doing this
> shouldn't really influence upstream too much.


OK, will put it in platform-data.

Re: [PATCH 7/8] ASoC: codecs: Add AB8500 codec-driver

[Mark Brown]

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

On Fri, Apr 27, 2012 at 12:54:59PM +0200, Ola Lilja wrote:
> On 04/27/2012 11:35 AM, Mark Brown wrote:

> > What are these regulators?  Are they internal PGA supplies or are they
> > microphone biases?

> These are mic-biases.

OK, then why aren't they hooked up in the board anyway (probably fixed to
a specific configuration there, though there are some use cases for
changing)?  This is what other CODEC drivers do with their MICBIASes...

> > Normally the clocking control is under the control of the machine driver
> > and if the machine driver wants to offer any options to userspace it'd
> > provide its own control - usually there's way more stuff going on here
> > than just selecting a source and much more coordination needed with the
> > drivers involved.

> Yes, we are only selecting what clock to request to the clock-driver-code, which
> then does its magic and taking decisions related to the whole platform.
> More specifically, in my context (that is audio) we will act upon being in
> voicecall or not, and in these cases requesting these two different clocks. By
> design this request comes from audio-code in userspace down to the audio-driver
> in the kernel. The clock-code will then turn of the clock not needed by us
> (audio) or any other party.

So what one would expect there would be that the machine driver would
figure things out - usually for something like the voice call case the
machine driver would be doing something like switching to the voice call
clock when the baseband DAI comes up.

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

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

Re: [PATCH 7/8] ASoC: codecs: Add AB8500 codec-driver

[Ola Lilja]

On 04/27/2012 01:11 PM, Mark Brown wrote:

> On Fri, Apr 27, 2012 at 12:54:59PM +0200, Ola Lilja wrote:
>> On 04/27/2012 11:35 AM, Mark Brown wrote:
> 
>> > What are these regulators?  Are they internal PGA supplies or are they
>> > microphone biases?
> 
>> These are mic-biases.
> 
> OK, then why aren't they hooked up in the board anyway (probably fixed to
> a specific configuration there, though there are some use cases for
> changing)?  This is what other CODEC drivers do with their MICBIASes...


OK, I will have to investigate this possibility then... I'm not certain that we
have this information available from the board in any easy way.

> 
>> > Normally the clocking control is under the control of the machine driver
>> > and if the machine driver wants to offer any options to userspace it'd
>> > provide its own control - usually there's way more stuff going on here
>> > than just selecting a source and much more coordination needed with the
>> > drivers involved.
> 
>> Yes, we are only selecting what clock to request to the clock-driver-code, which
>> then does its magic and taking decisions related to the whole platform.
>> More specifically, in my context (that is audio) we will act upon being in
>> voicecall or not, and in these cases requesting these two different clocks. By
>> design this request comes from audio-code in userspace down to the audio-driver
>> in the kernel. The clock-code will then turn of the clock not needed by us
>> (audio) or any other party.
> 
> So what one would expect there would be that the machine driver would
> figure things out - usually for something like the voice call case the
> machine driver would be doing something like switching to the voice call
> clock when the baseband DAI comes up.

Re: [PATCH 7/8] ASoC: codecs: Add AB8500 codec-driver

[Mark Brown]

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

On Mon, Apr 30, 2012 at 10:50:55AM +0200, Ola Lilja wrote:
> On 04/27/2012 01:11 PM, Mark Brown wrote:
> > On Fri, Apr 27, 2012 at 12:54:59PM +0200, Ola Lilja wrote:

> > OK, then why aren't they hooked up in the board anyway (probably fixed to
> > a specific configuration there, though there are some use cases for
> > changing)?  This is what other CODEC drivers do with their MICBIASes...

> OK, I will have to investigate this possibility then... I'm not certain that we
> have this information available from the board in any easy way.

Usually it's very clear in the schematic?

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

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