Re: [PATCH 1/5] ASoC: Add ADAU1X61 and ADAU1X81 CODECs common code

[Lars-Peter Clausen <lars@metafoo.de>]

On 08/28/2013 06:59 PM, Mark Brown wrote:
> On Wed, Aug 28, 2013 at 05:20:09PM +0200, Lars-Peter Clausen wrote:
> 
>> +static void adau17x1_check_aifclk(struct snd_soc_codec *codec)
>> +{
>> +	struct adau *adau = snd_soc_codec_get_drvdata(codec);
>> +
>> +	/* If we are in master mode we need to generate bit- and frameclock,
>> +	 * regardless of whether there is an active path or not */
>> +	if (codec->active && adau->master)
>> +		snd_soc_dapm_force_enable_pin(&codec->dapm, "AIFCLK");
>> +	else
>> +		snd_soc_dapm_disable_pin(&codec->dapm, "AIFCLK");
>> +	snd_soc_dapm_sync(&codec->dapm);
>> +}
> 
> I think this is eminently sensible but it seems like it should be a
> framework feature.  That'd have to enable an actual AIF slot though
> which is a bit fun, which slot do we enable for example?

It would be nice if this could be handled by the core. But how? One option
might be tracking the the DAI format in the core and power up the DAI widget
in DAPM if the stream is active regardless of if there is an active path if
the DAI is in master mode. That may power up more than what is needed
though. E.g. for this CODEC the clock generator and the rest of the DAI can
be powered up independently and we only need to power up the clock generator.

> 
>> +static int adau17x1_set_dai_clkdiv(struct snd_soc_dai *dai, int div_id, int div)
>> +{
>> +	struct adau *adau = snd_soc_codec_get_drvdata(dai->codec);
>> +
>> +	if (div < 0 || div > 4)
>> +		return -EINVAL;
>> +
>> +	adau->sysclk_div = div;
>> +
>> +	return regmap_update_bits(adau->regmap, ADAU17X1_CLOCK_CONTROL,
>> +		ADAU17X1_CLOCK_CONTROL_INFREQ_MASK, (div - 1) << 1);
>> +}
> 
> What's this doing?  It'd be better to have a specific ID and check that
> too.

Setting the relationship between the external clock rate and the internal
base sample rate. There might be a better way to do this though.

> 
>> +int adau17x1_set_micbias_voltage(struct snd_soc_codec *codec,
>> +	enum adau17x1_micbias_voltage micbias)
>> +{
>> +	struct adau *adau = snd_soc_codec_get_drvdata(codec);
>> +
>> +	switch (micbias) {
>> +	case ADAU17X1_MICBIAS_0_90_AVDD:
>> +	case ADAU17X1_MICBIAS_0_65_AVDD:
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	regmap_write(adau->regmap, ADAU17X1_MICBIAS, micbias << 2);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(adau17x1_set_micbias_voltage);
> 
> When would a machine driver use this (as opposed to just letting it be
> set by platform data)?

It is not meant to be used by machine drivers, but by the adau1761 and
adau1781 CODEC drivers, which set the micbias based on platform data.

Thanks for the fast review,
- Lars