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

[Mark Brown]

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?
-------------- 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/20210727/01e14294/attachment-0001.sig>