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

[Ola Lilja <ola.o.lilja@stericsson.com>]

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.