[alsa-devel] [PATCH 1/2] ASoC: Add support for CS43130 codec

[Mark Brown]

On Wed, Dec 07, 2016 at 02:17:27PM -0600, Li Xu wrote:

Overall this looks pretty good - there's a fair number of issues below
but they're all fairly simple stylistic things rather than anything
majorly wrong so hopefully should be easy to correct.

>  	select SND_SOC_CS53L30 if I2C
> +	select SND_SOC_CS43130 if I2C
>  	select SND_SOC_CX20442 if TTY

Please keep Kconfig and Makefile sorted.

> +static bool cs43130_volatile_register(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case CS43130_DEVID_AB ... CS43130_SUBREV_ID:
> +	case CS43130_INT_STATUS_1 ... CS43130_INT_STATUS_5:
> +		return true;

You don't need to mark the device ID volatile, just don't provide
defaults and regmap will cache it the first time it reads it.  If the
device ID is volatile you've got bigger problems.

> +			/*PDN_PLL= 0,enable*/

Please use the standard kernel coding style, need spaces here.

> +			regmap_update_bits(cs43130->regmap, CS43130_PLL_SET_9,
> +					    CS43130_PLL_REF_PREDIV_MASK,
> +						pll_ratio_table[i].sclk_prediv
> +						);

There's no need for the ); to be on a new line here, nor for the extra
indentation on the line before.  There are lots more coding style
issues, checkpatch will probably pick up many of them.

> +	case SND_SOC_DAPM_PRE_PMU:
> +		if (cs43130->pll_bypass)
> +			cs43130_change_clksrc(codec, CS43130_MCLK_SRC_XTAL);
> +		else
> +			cs43130_change_clksrc(codec, CS43130_MCLK_SRC_PLL);
> +
> +		usleep_range(10000, 10050);
> +		/*ASP_3ST = 0 in master mode*/
> +		if (cs43130->dai_mode)
> +			regmap_update_bits(cs43130->regmap, CS43130_PAD_INT_CFG,
> +						    0x01, 0x00);

No need to undo this in slave mode?

> +	if (stickies[0] & CS43130_XTAL_ERR_INT)
> +		pr_debug("%s: Crystal err\n", __func__);

dev_ prints and shouldn't this one be an error?

> +	/* Enable HP detect */
> +	regmap_update_bits(cs43130->regmap, CS43130_HP_DETECT,
> +		CS43130_HP_DETECT_CTRL_MASK, CS43130_HP_DETECT_CTRL_MASK);

Why enable this when the only handling is a couple of log messages?

> +#ifdef CONFIG_PM
> +static int cs43130_runtime_suspend(struct device *dev)
> +{
> +	return 0;
> +}

Remove empty functions, they don't serve any purpose.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20161208/940738f9/attachment.sig>