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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
