Re: [PATCH v3] ASoC: cs53l30: Add support for Cirrus Logic CS53L30

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 1205 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]