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

[Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 4830 bytes --]

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.

> +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.

> +static const struct regmap_range_cfg aic31xx_ranges[] = {
> +	{
> +		.name = "codec-regmap",

Make this meaningful or omit it.

> +#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.

> +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.

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

SP?

> +	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.

> +		dev_err(w->codec->dev, "Unknown widget '%s' calling %s/n",
> +			w->name, __func__);
> +		return -1;

Return real error codes.

> +	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.

> +	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.

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

params_width().

> +			    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.

> +	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.

> +	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.

> +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.

> +		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.

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

Coding style, please.

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