Re: [PATCH v4 1/2] ASoC: cs35l41: CS35L41 Boosted Smart Amplifier

On Mon, Jul 26, 2021 at 05:34:37PM -0500, David Rhodes wrote:

This looks mostly good, a few pretty small things below:

+++ b/sound/soc/codecs/cs35l41-i2c.c @@ -0,0 +1,116 @@ +/* SPDX-License-Identifier: GPL-2.0 */

C files need to have a C++ style SPDX header, please make the entire comment a C++ one so things look more consistent.

+static const struct snd_kcontrol_new cs35l41_aud_controls[] = {

•   SOC_SINGLE_SX_TLV("Digital PCM Volume", CS35L41_AMP_DIG_VOL_CTRL,
  

•                 3, 0x4CF, 0x391, dig_vol_tlv),
  

•   SOC_SINGLE_TLV("AMP PCM Gain", CS35L41_AMP_GAIN_CTRL, 5, 0x14, 0,
  

•                   amp_gain_tlv),
  

Volume controls need to end in Volume like the one immediately above, see control-names.rst.

• /* returning NULL can be an option if in stereo mode */
• cs35l41->reset_gpio = devm_gpiod_get_optional(cs35l41->dev, "reset",

• 					GPIOD_OUT_LOW);
  

Was this included in the DT binding?

• ret = devm_snd_soc_register_component(cs35l41->dev,

• 			&soc_component_dev_cs35l41,
  

• 			cs35l41_dai, ARRAY_SIZE(cs35l41_dai));
  

• if (ret < 0) {

• dev_err(cs35l41->dev, "%s: Register codec failed\n", __func__);
  

• goto err;
  

• }
• ret = cs35l41_set_pdata(cs35l41);
• if (ret < 0) {

• dev_err(cs35l41->dev, "%s: Set pdata failed\n", __func__);
  

• goto err;
  

• }

Are you sure it's safe to register the device before applying the platform data configuration - as soon as the device is registered the sound card might become visible?