Re: [PATCH RFC 1/5] ASoC: tlv320aic31xx: Add basic codec driver implementation

[Jyri Sarha <jsarha@ti.com>]

On 03/03/2014 08:34 AM, Mark Brown wrote:
> On Wed, Feb 26, 2014 at 11:14:25AM +0200, Jyri Sarha wrote:
>> This commit adds a bare bones driver support for TLV320AIC31XX family
>> audio codecs. The driver adds basic stereo playback trough headphone
>> and speaker outputs and mono capture trough microphone inputs.
>
> Always CC maintainers on patches please.
>

Will do.

>> +static bool aic31xx_volatile(struct device *dev, unsigned int reg)
>> +{
>> +	if (aic31xx_precious(dev, reg) || aic31xx_real_volatile(dev, reg))
>> +		return true;
>> +
>> +	switch (reg) {
>> +	case AIC31XX_PAGECTL: /* regmap implementation requires this */
>> +	case AIC31XX_RESET: /* always clears after write */
>> +		return true;
>> +	}
>> +	return false;
>> +}
>
> This is more complex than you need, just write a standalone volatile
> function with some comments in it.
>

Replaced with straight forward aic31xx_volatile() and aic31xx_writable() 
functions.

>> +static const struct regmap_range_cfg aic31xx_ranges[] = {
>> +	{
>> +		.name = "codec-regmap",
>
> Make this meaningful or omit it.
>

Removed.

>> +#define AIC31XX_NUM_SUPPLIES	6
>> +static const char * const aic31xx_supply_names[] = {
>
> Use the define in the array size to help check everything lines up.
>

Done.

>> +static const char * const ldac_in_text[] = {
>> +	"off", "Left Data", "Right Data", "Mono"
>> +};
>
> Off not off - be consistent both internally and with the general style.
>

Done.

>> +static const struct snd_kcontrol_new aic311x_snd_controls[] = {
>> +	SOC_DOUBLE_R("SP Driver Playback Switch", AIC31XX_SPLGAIN,
>> +		     AIC31XX_SPRGAIN, 2, 1, 0),
>
> SP?
>

"SP" replaced with "Speaker", also in DAPM switches.

>> +	if (!strcmp(w->name, "DAC Left")) {
>> +		mask = AIC31XX_LDACPWRSTATUS_MASK;
>
> There's no overlap with the enable bits?  In general it would seem nicer
> to have a switch based on the register bits used for the enable rather
> than the tree of string comparisions but it's probably not that big a
> deal overall.
>

Not with bits, but with regs. I decided this structure would be easier 
to implement and understand than having a switch(w->reg) with nested 
switch(w->shift) cases. I replaced the string comparisons with a single 
switch case using a binary combination of w->reg and w->shift.

>> +		dev_err(w->codec->dev, "Unknown widget '%s' calling %s/n",
>> +			w->name, __func__);
>> +		return -1;		switch (w->shift) {
		case 7:
			mask = AIC31XX_LDACPWRSTATUS_MASK;

>
> Return real error codes.
>

Changed to -EINVAL.

>> +	if (event == SND_SOC_DAPM_POST_PMU)
>> +		return aic31xx_wait_bits(aic31xx, reg, mask, mask, 5000, 100);
>> +	else if (event == SND_SOC_DAPM_POST_PMD)
>> +		return aic31xx_wait_bits(aic31xx, reg, mask, 0, 5000, 100);
>
> switch.
>

Done.

>> +	SND_SOC_DAPM_DAC_E("DAC Right", "DAC Right Input",
>> +			   AIC31XX_DACSETUP, 6, 0, aic31xx_dapm_power_event,
>> +			   SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD),
>
> Don't use stream names, use DAPM to route from the AIF to the widgets.
>

Renamed DAC stnames to "Left Playback" and "Right Playback".

>> +	switch (params_format(params)) {
>> +	case SNDRV_PCM_FORMAT_S16_LE:
>> +		break;
>
> params_width().
>

Done.

>> +			    AIC31XX_IFACE1_DATALEN_MASK,
>> +			    data);
>> +
>> +	return aic31xx_setup_pll(codec, params);
>
> This is going to mean that you have a symmetry constraint I think, if
> you try to set up different rates for playback and capture presumably
> things will break.  Setting symmetric_rates will tell applications about
> that.
>

Done.

>> +	case SND_SOC_DAIFMT_CBS_CFM:
>> +	case SND_SOC_DAIFMT_CBM_CFS:
>> +	case SND_SOC_DAIFMT_CBS_CFS:
>> +		dev_err(codec->dev, "Unsupported DAI master/slave interface\n");
>> +		return -EINVAL;
>> +	default:
>> +		dev_alert(codec->dev, "Invalid DAI master/slave interface\n");
>> +		return -EINVAL;
>
> Just have a default.
>

Done.

>> +	for (i = 0; aic31xx_divs[i].mclk != freq; i++)
>> +		if (i == ARRAY_SIZE(aic31xx_divs)) {
>> +			dev_err(aic31xx->dev, "%s: Unsupported frequency %d\n",
>> +				__func__, freq);
>> +			return -EINVAL;
>> +		}
>
> Braces around the for () too please.
>

Done.

>> +static int aic31xx_set_power(struct snd_soc_codec *codec, int power)
>> +{
>> +	struct aic31xx_priv *aic31xx = snd_soc_codec_get_drvdata(codec);
>> +	int ret;
>> +
>> +	dev_dbg(codec->dev, "## %s: %d -> %d\n", __func__,
>> +		aic31xx->power, power);
>> +	if (aic31xx->power == power)
>> +		return 0;
>
> This looks like you need sensible refcounting somewhere?  Implementing
> this as the standard PM operations may be sensible.
>

Not really. This was just avoid power on sequence when bias level goes 
from SND_SOC_BIAS_PREPARE back to SND_SOC_BIAS_STANDBY. I'll change this 
to a "if (codec->dapm.bias_level == SND_SOC_BIAS_OFF)" in set_bias_level 
like other codecs do.

>> +		if (gpio_is_valid(aic31xx->pdata.gpio_reset)) {
>> +			gpio_set_value(aic31xx->pdata.gpio_reset, 1);
>> +			udelay(100);
>> +		}
>> +		snd_soc_write(codec, AIC31XX_RESET, 0x01);
>> +		udelay(100);
>> +		regcache_cache_only(aic31xx->regmap, false);
>
> You should be coming out of cache only mode before you issue the reset
> write.  Is it not sensible to skip that if you have a physical reset
> line?
>
>> +		snd_soc_write(codec, AIC31XX_RESET, 0x01);
>> +		regcache_cache_only(aic31xx->regmap, true);
>> +
>> +		if (gpio_is_valid(aic31xx->pdata.gpio_reset))
>> +			gpio_set_value(aic31xx->pdata.gpio_reset, 0);
>
> Likewise here.  Also if you do reset the CODEC then the dance you did
> with the regulator notifiers becomes pointless - the goal with that is
> to avoid the need to resync the cache if the CODEC wasn't actually reset
> by a power cycle but since the CODEC is going to be explicitly reset
> before it's powered off there's no benefit.
>

I rewrote the bias-level/power logic a bit. It should now look more like 
the other codecs. I hope you like this version better.

>> +	switch (level) {
>> +	/* full On */
>> +	case SND_SOC_BIAS_ON:
>> +		/* All power is driven by DAPM system*/
>> +		break;
>> +	/* partial On */
>
> Coding style, please.
>

Removed the redundant badly indented comments.

Fixed also the Kconfig and Makefile ordering and added dt-include for 
micbias and changed the default to 2.0V. All are now squashed to a 
single patch.

In addition to addressing your comments I also added an internal ADC 
input and DAC output for codec to turn on at playback/capture start even 
if the alsamixers are not set correctly.

I'll mail the patches shortly.

Best regards,
Jyri