Re: [PATCH 4/5] ASoC: codecs: Add AB8500 codec-driver

Re: [PATCH 4/5] ASoC: codecs: Add AB8500 codec-driver

[Mark Brown]

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

On Thu, May 24, 2012 at 03:26:38PM +0200, Ola Lilja wrote:

> +static void show_regulator_status(struct device *dev)
> +{
> +	struct ab8500_codec_drvdata *drvdata = dev_get_drvdata(dev);
> +	struct ab8500_codec_drvdata_dbg *dbg = &drvdata->dbg;
> +
> +	dev_dbg(dev, "%s: Regulator-status:\n", __func__);
> +	dev_dbg(dev, "%s:     V-AUD: %s\n", __func__,
> +		(regulator_is_enabled(dbg->vaud) > 0) ?
> +			"On" : "Off");
> +	dev_dbg(dev, "%s:     V-AMIC1: %s\n", __func__,
> +		(regulator_is_enabled(dbg->vamic1) > 0) ?
> +			"On" : "Off");
> +	dev_dbg(dev, "%s:     V-AMIC2: %s\n", __func__,
> +		(regulator_is_enabled(dbg->vamic2) > 0) ?
> +			"On" : "Off");
> +	dev_dbg(dev, "%s:     V-DMIC: %s\n", __func__,
> +		(regulator_is_enabled(dbg->vdmic) > 0) ?
> +			"On" : "Off");

What problems are you finding when you try to use the debug
infrastructure in both the regulator API and DAPM to discover the state
of the regulators?

> +	/* Clocks */
> +	SND_SOC_DAPM_CLOCK_SUPPLY("audioclk"),
> +	SND_SOC_DAPM_CLOCK_SUPPLY("gpio.1"),

This looks wrong - audioclk looks reasonable but gpio.1 looks like a
board-specific name which shouldn't be encoded into the driver.

> +	if (ucontrol->value.integer.value[0] != SID_APPLY_FIR) {
> +		dev_err(codec->dev,
> +			"%s: ERROR: This control supports '%s' only!\n",
> +			__func__, enum_sid_state[SID_APPLY_FIR]);
> +		return 0;
> +	}

I'd expect this to return an error...

> +	status = snd_soc_dapm_force_enable_pin(&codec->dapm,
> +					"ANC Configure Input");
> +	if (status < 0) {
> +		dev_err(dev,
> +			"%s: ERROR: Failed to enable power (status = %d)!\n",
> +			__func__, status);
> +		goto cleanup;
> +	}
> +	snd_soc_dapm_sync(&codec->dapm);
> +
> +	mutex_lock(&codec->mutex);

Your locking looks bad here.  Nothing ensures that something doesn't
come along and undo the force enable.  Looking at the code this is the
only function that fiddles with the input but there's still a race where
one writer might exit the mutex section and disable the pin while a
second enters the mutex section.

> +static int filter_control_get(struct snd_kcontrol *kcontrol,
> +			struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct filter_control *fc =
> +			(struct filter_control *)kcontrol->private_value;
> +	unsigned int i;
> +
> +	for (i = 0; i < fc->count; i++)
> +		ucontrol->value.integer.value[i] = fc->value[i];
> +
> +	return 0;
> +}
> +
> +static int filter_control_put(struct snd_kcontrol *kcontrol,
> +		struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct filter_control *fc =
> +			(struct filter_control *)kcontrol->private_value;
> +	unsigned int i;
> +
> +	for (i = 0; i < fc->count; i++)
> +		fc->value[i] = ucontrol->value.integer.value[i];

These don't seem to be locked?

> +int ab8500_audio_init_audioblock(struct snd_soc_codec *codec)

static.  Lots of other functions in the rest of the driver have the same
issue.

> +static int ab8500_codec_pcm_hw_params(struct snd_pcm_substream *substream,
> +		struct snd_pcm_hw_params *hw_params, struct snd_soc_dai *dai)
> +{
> +	dev_dbg(dai->codec->dev, "%s Enter.\n", __func__);
> +
> +	return 0;
> +}

Remove empty functions.

> +	default:
> +		dev_err(dai->codec->dev,
> +			"%s: ERROR: Unsupported INV mask 0x%x\n",
> +			__func__, fmt & SND_SOC_DAIFMT_INV_MASK);
> +		return -EINVAL;
> +		break;

The break is redundant.

> +	/* Only 16 bit slot width is supported at the moment in TDM mode */
> +	if (slot_width != 16) {
> +		dev_err(dai->codec->dev,
> +			"%s: ERROR: Unsupported slot_width %d.\n",
> +			__func__, slot_width);
> +		return -EINVAL;
> +	}

You've got code which supports other widths...

> +static struct snd_soc_codec_driver ab8500_codec_driver = {
> +	.probe =		ab8500_codec_probe,
> +	.read =			ab8500_codec_read_reg,
> +	.write =		ab8500_codec_write_reg,
> +	.reg_cache_size =	0,

no need to init things to zero or NULL in static structs.

> +#define PRE_PMU_POST_PMD			(SND_SOC_DAPM_PRE_PMU | \
> +						SND_SOC_DAPM_POST_PMD)

You shouldn't define stuff like this in your driver!

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

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

Re: [PATCH 4/5] ASoC: codecs: Add AB8500 codec-driver

[Ola Lilja]

On 05/30/2012 03:14 PM, Mark Brown wrote:

> On Thu, May 24, 2012 at 03:26:38PM +0200, Ola Lilja wrote:
> 
>> +static void show_regulator_status(struct device *dev)
>> +{
>> +	struct ab8500_codec_drvdata *drvdata = dev_get_drvdata(dev);
>> +	struct ab8500_codec_drvdata_dbg *dbg = &drvdata->dbg;
>> +
>> +	dev_dbg(dev, "%s: Regulator-status:\n", __func__);
>> +	dev_dbg(dev, "%s:     V-AUD: %s\n", __func__,
>> +		(regulator_is_enabled(dbg->vaud) > 0) ?
>> +			"On" : "Off");
>> +	dev_dbg(dev, "%s:     V-AMIC1: %s\n", __func__,
>> +		(regulator_is_enabled(dbg->vamic1) > 0) ?
>> +			"On" : "Off");
>> +	dev_dbg(dev, "%s:     V-AMIC2: %s\n", __func__,
>> +		(regulator_is_enabled(dbg->vamic2) > 0) ?
>> +			"On" : "Off");
>> +	dev_dbg(dev, "%s:     V-DMIC: %s\n", __func__,
>> +		(regulator_is_enabled(dbg->vdmic) > 0) ?
>> +			"On" : "Off");
> 
> What problems are you finding when you try to use the debug
> infrastructure in both the regulator API and DAPM to discover the state
> of the regulators?


My vision here is that in a simple way, in one place, activate all
debug-information we need in our driver, prefixed with our dev_xxx. This is very
valuable for us when debugging, especially when a customer is told to activate
debug-information that we can use to debug.
I removed the menuconfig flag on your request, and then we lost the information
for regulators and clocks when I implemented the clock/regulator-widgets. I'm
just trying to keep some aspects of what we want to have but still conforming
what you want to see on mainline.

> 
>> +	/* Clocks */
>> +	SND_SOC_DAPM_CLOCK_SUPPLY("audioclk"),
>> +	SND_SOC_DAPM_CLOCK_SUPPLY("gpio.1"),
> 
> This looks wrong - audioclk looks reasonable but gpio.1 looks like a
> board-specific name which shouldn't be encoded into the driver.
> 
>> +	if (ucontrol->value.integer.value[0] != SID_APPLY_FIR) {
>> +		dev_err(codec->dev,
>> +			"%s: ERROR: This control supports '%s' only!\n",
>> +			__func__, enum_sid_state[SID_APPLY_FIR]);
>> +		return 0;
>> +	}
> 
> I'd expect this to return an error...


Yes.

> 
>> +	status = snd_soc_dapm_force_enable_pin(&codec->dapm,
>> +					"ANC Configure Input");
>> +	if (status < 0) {
>> +		dev_err(dev,
>> +			"%s: ERROR: Failed to enable power (status = %d)!\n",
>> +			__func__, status);
>> +		goto cleanup;
>> +	}
>> +	snd_soc_dapm_sync(&codec->dapm);
>> +
>> +	mutex_lock(&codec->mutex);
> 
> Your locking looks bad here.  Nothing ensures that something doesn't
> come along and undo the force enable.  Looking at the code this is the
> only function that fiddles with the input but there's still a race where
> one writer might exit the mutex section and disable the pin while a
> second enters the mutex section.


Will fix.

> 
>> +static int filter_control_get(struct snd_kcontrol *kcontrol,
>> +			struct snd_ctl_elem_value *ucontrol)
>> +{
>> +	struct filter_control *fc =
>> +			(struct filter_control *)kcontrol->private_value;
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < fc->count; i++)
>> +		ucontrol->value.integer.value[i] = fc->value[i];
>> +
>> +	return 0;
>> +}
>> +
>> +static int filter_control_put(struct snd_kcontrol *kcontrol,
>> +		struct snd_ctl_elem_value *ucontrol)
>> +{
>> +	struct filter_control *fc =
>> +			(struct filter_control *)kcontrol->private_value;
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < fc->count; i++)
>> +		fc->value[i] = ucontrol->value.integer.value[i];
> 
> These don't seem to be locked?


Will look into it.

> 
>> +int ab8500_audio_init_audioblock(struct snd_soc_codec *codec)
> 
> static.  Lots of other functions in the rest of the driver have the same
> issue.


This one should be static, yes. Cannot find any other non-static functions in
the codec-driver that is missing static.

> 
>> +static int ab8500_codec_pcm_hw_params(struct snd_pcm_substream *substream,
>> +		struct snd_pcm_hw_params *hw_params, struct snd_soc_dai *dai)
>> +{
>> +	dev_dbg(dai->codec->dev, "%s Enter.\n", __func__);
>> +
>> +	return 0;
>> +}
> 
> Remove empty functions.


OK.

> 
>> +	default:
>> +		dev_err(dai->codec->dev,
>> +			"%s: ERROR: Unsupported INV mask 0x%x\n",
>> +			__func__, fmt & SND_SOC_DAIFMT_INV_MASK);
>> +		return -EINVAL;
>> +		break;
> 
> The break is redundant.


Yes.

> 
>> +	/* Only 16 bit slot width is supported at the moment in TDM mode */
>> +	if (slot_width != 16) {
>> +		dev_err(dai->codec->dev,
>> +			"%s: ERROR: Unsupported slot_width %d.\n",
>> +			__func__, slot_width);
>> +		return -EINVAL;
>> +	}
> 
> You've got code which supports other widths...


Will look into it.

> 
>> +static struct snd_soc_codec_driver ab8500_codec_driver = {
>> +	.probe =		ab8500_codec_probe,
>> +	.read =			ab8500_codec_read_reg,
>> +	.write =		ab8500_codec_write_reg,
>> +	.reg_cache_size =	0,
> 
> no need to init things to zero or NULL in static structs.


Yes, was just for clarity. Will remove it.

> 
>> +#define PRE_PMU_POST_PMD			(SND_SOC_DAPM_PRE_PMU | \
>> +						SND_SOC_DAPM_POST_PMD)
> 
> You shouldn't define stuff like this in your driver!


This was mainly to avoid impossible situations trying to comply with the
80-char-width.

Re: [PATCH 4/5] ASoC: codecs: Add AB8500 codec-driver

[Mark Brown]

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

On Wed, May 30, 2012 at 03:49:51PM +0200, Ola Lilja wrote:
> On 05/30/2012 03:14 PM, Mark Brown wrote:

> > What problems are you finding when you try to use the debug
> > infrastructure in both the regulator API and DAPM to discover the state
> > of the regulators?

> My vision here is that in a simple way, in one place, activate all
> debug-information we need in our driver, prefixed with our dev_xxx. This is very
> valuable for us when debugging, especially when a customer is told to activate
> debug-information that we can use to debug.
> I removed the menuconfig flag on your request, and then we lost the information
> for regulators and clocks when I implemented the clock/regulator-widgets. I'm
> just trying to keep some aspects of what we want to have but still conforming
> what you want to see on mainline.

All the existing diagnostics use dev_name() too...  just turning on the
standard logging ought to give you exactly the same stuff, indeed if
debugfs is on there should be no need to even manually enable equivalent
debugging.

> >> +int ab8500_audio_init_audioblock(struct snd_soc_codec *codec)

> > static.  Lots of other functions in the rest of the driver have the same
> > issue.

> This one should be static, yes. Cannot find any other non-static functions in
> the codec-driver that is missing static.

IIRC the function immediately below was one.

> >> +#define PRE_PMU_POST_PMD			(SND_SOC_DAPM_PRE_PMU | \
> >> +						SND_SOC_DAPM_POST_PMD)

> > You shouldn't define stuff like this in your driver!

> This was mainly to avoid impossible situations trying to comply with the
> 80-char-width.

Adding things like this if they're useful are fine but they shouldn't be
driver specific things.  There is a SND_SOC_DAPM_PRE_POST_PMD already
though.

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

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

Re: [PATCH 4/5] ASoC: codecs: Add AB8500 codec-driver

[Ola Lilja]

On 05/30/2012 05:09 PM, Mark Brown wrote:

> On Wed, May 30, 2012 at 03:49:51PM +0200, Ola Lilja wrote:
>> On 05/30/2012 03:14 PM, Mark Brown wrote:
> 
>> > What problems are you finding when you try to use the debug
>> > infrastructure in both the regulator API and DAPM to discover the state
>> > of the regulators?
> 
>> My vision here is that in a simple way, in one place, activate all
>> debug-information we need in our driver, prefixed with our dev_xxx. This is very
>> valuable for us when debugging, especially when a customer is told to activate
>> debug-information that we can use to debug.
>> I removed the menuconfig flag on your request, and then we lost the information
>> for regulators and clocks when I implemented the clock/regulator-widgets. I'm
>> just trying to keep some aspects of what we want to have but still conforming
>> what you want to see on mainline.
> 
> All the existing diagnostics use dev_name() too...  just turning on the
> standard logging ought to give you exactly the same stuff, indeed if
> debugfs is on there should be no need to even manually enable equivalent
> debugging.
> 
>> >> +int ab8500_audio_init_audioblock(struct snd_soc_codec *codec)
> 
>> > static.  Lots of other functions in the rest of the driver have the same
>> > issue.
> 
>> This one should be static, yes. Cannot find any other non-static functions in
>> the codec-driver that is missing static.
> 
> IIRC the function immediately below was one.


Note that I wrote "...that is missing static". The one you previously mentioned
is indeed a function that we have moved from being called from the machine, but
this one (and one other) is still functions that we need to call from the
machine (or if you have a suggestion to how we could get rid of this connection).
Note that we have pretty much eliminated most of the extra functions we had
between machine and codec.

> 
>> >> +#define PRE_PMU_POST_PMD			(SND_SOC_DAPM_PRE_PMU | \
>> >> +						SND_SOC_DAPM_POST_PMD)
> 
>> > You shouldn't define stuff like this in your driver!
> 
>> This was mainly to avoid impossible situations trying to comply with the
>> 80-char-width.
> 
> Adding things like this if they're useful are fine but they shouldn't be
> driver specific things.  There is a SND_SOC_DAPM_PRE_POST_PMD already
> though.