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

[Mark Brown <broonie@kernel.org>]

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

On Tue, Mar 15, 2016 at 05:47:46PM -0500, tim.howe@cirrus.com wrote:

> +/* ADC Preamp gain select */
> +static const char * const cs53l30_preamp_gain_sel_text[] = {
> +	"0dB", "10dB", "20dB"};
> +
> +static const struct soc_enum adc1a_preamp_gain_enum =
> +	SOC_ENUM_SINGLE(CS53L30_ADC1A_AFE_CTL, 6,
> +		ARRAY_SIZE(cs53l30_preamp_gain_sel_text),
> +		cs53l30_preamp_gain_sel_text);

Why are you writing volume controls as enums?  Please use normal volume
controls with TLV information.

> +/* Set MIC Bias Voltage Control */
> +static const char * const cs53l30_micbias_text[] = {
> +	"HiZ", "1.8V", "2.75V"};

Why is this exposed to userspace and not platform data like other bias
controls?  The voltage to use will normally be determined by the
hardware design and the enable/disable should be managed via DAPM.

> +	case SND_SOC_DAPM_PRE_PMU:
> +		regmap_update_bits(priv->regmap, CS53L30_ASP1_CTL,
> +				   CS53L30_ASP1_3ST, 0);
> +		break;
> +	case SND_SOC_DAPM_POST_PMD:
> +		regmap_update_bits(priv->regmap, CS53L30_ASP1_CTL,
> +				   CS53L30_ASP1_3ST, 1);
> +		break;

Could this be a supply widget?

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

You're using regmap_update_bits() but still implementing a manual
read/modify write cycle.  Just use regmap_update_bits.

> +	regmap_read(priv->regmap, CS53L30_ASPCFG_CTL, &asp_config_ctl);
> +
> +	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> +	case SND_SOC_DAIFMT_CBM_CFM:
> +		asp_config_ctl |= CS53L30_ASP_MS;

> +	priv->asp_config_ctl = asp_config_ctl;

The driver appears to be mixing use of priv->asp_config_ctl and the
physical value, that seems likely to go wrong somewhere along the line.
Either the variable is authorative and we should always work from that
then write it to the device when needed or the device is authorative but
currently we're mixing both.

> +	regmap_update_bits(priv->regmap, CS53L30_ASPCFG_CTL,
> +			   CS53L30_ASP_RATE_MASK, priv->asp_config_ctl);

We also write out via update_bits but don't specify all the bits
sometimes...

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

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