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);elsecs43130_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);elsecs43130_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.