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

[Li Xu]

Thank you for your prompt reply.

I have updated v2 patch to include your recommmendations except
following:

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

Fixed.

In v2 patch, checkpatch do not show addional formatting issues.

I have cleaned 'regmap_update_bits' API call with 80-char line
constraint.
----------------------------------------------------------------------------
> > +	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?

Only needed for master mode, so no need for slave mode
----------------------------------------------------------------------------
> > +	/* 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?

I suppose I could remove it, but it may be helpful for
system integrators to know where HP detect hooks are.

But if you insist, I could remove it.
----------------------------------------------------------------------------


On Thu, Dec 08, 2016 at 04:23:36PM +0000, Mark Brown wrote:
> 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.