[alsa-devel] [PATCH v3] ASoC: cs53l30: Add support for Cirrus Logic CS53L30

[Mark Brown]

On Mon, Jan 25, 2016 at 05:14:43PM -0600, tim.howe at cirrus.com wrote:
> From: Tim Howe <tim.howe at cirrus.com>

I'd have got to this sooner but there was an unreplied mail from the
0day bot...  anyway, a few fairly minor issues here:

> +	default:
> +		pr_err("Invalid event = 0x%x\n", event);
> +		return -EINVAL;
> +	}

dev_err().

> +	regmap_read(priv->regmap, CS53L30_MCLKCTL, &mclk_ctl);
> +	mclk_ctl &= ~MCLK_DIV;
> +	mclk_ctl |= cs53l30_mclkx_coeffs[mclkx_coeff].mclkdiv;
> +
> +	regmap_write(priv->regmap, CS53L30_MCLKCTL, mclk_ctl);

This is open coding regmap_update_bits(), several other bits of the
driver seem to be doing something similar.

> +		regmap_read(priv->regmap, CS53L30_IS, &reg); /* Clr status */
> +		for (i = 0; i < inter_max_check; i++) {
> +			usleep_range(1000, 1000);
> +			msleep(1);

So we do a usleep_range() for 1ms then a msleep() for 1ms...  why not
just do one or the other for 2ms?  It at least looks weird and needs a
comment.

> +/* CS53L30_ADCDMIC1_CTL1  */
> +#define ADC1B_PDN		(1 << 7)
> +#define ADC1A_PDN		(1 << 6)

These constants really need namespacing - a lot of them have pretty
generic names so look like they might end up colliding with a kernel
header.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20160205/9535af73/attachment-0001.sig>