Re: [PATCH v7 3/5] ASoC: meson: g12a-toacodec: Add S4 tocodec driver - sashiko-bot

From: sashiko-bot@kernel.org
To: "Jiebing Chen" <jiebing.chen@amlogic.com>
Cc: devicetree@vger.kernel.org, linux-amlogic@lists.infradead.org,
	linux-clk@vger.kernel.org, robh@kernel.org, conor+dt@kernel.org,
	neil.armstrong@linaro.org
Subject: Re: [PATCH v7 3/5] ASoC: meson: g12a-toacodec: Add S4 tocodec driver
Date: Wed, 21 Oct 2026 07:28:39 +0000	[thread overview]
Message-ID: <sashiko-outbox-179905@kernel.org> (raw)
In-Reply-To: <20261021-audio_drvier-v7-3-2b9aeae9cb8a@amlogic.com>

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ASoC: meson: g12a-toacodec: wrong mux shift in s4_toacodec_widgets
--

commit 3fd35e52fdcaaa2efffaba05b21f58404303e9de
Author: Jiebing Chen <jiebing.chen@amlogic.com>

ASoC: meson: g12a-toacodec: Add S4 tocodec driver

This patch adds Amlogic S4 SoC support to the g12a-toacodec driver,
including DAPM supply widgets for the BCLK and MCLK gates, an 8-lane
Lane Select control, and the S4 register field layout.

> diff --git a/sound/soc/meson/g12a-toacodec.c b/sound/soc/meson/g12a-toacodec.c
> index a95375b53f0a3..10f26b4bc8b0d 100644
> --- a/sound/soc/meson/g12a-toacodec.c
> +++ b/sound/soc/meson/g12a-toacodec.c

[ ... ]

> @@ -141,6 +144,16 @@ static const struct snd_soc_dapm_widget sm1_toacodec_widgets[] = {
>  			    &g12a_toacodec_out_enable),
>  };
>  
> +static const struct snd_soc_dapm_widget s4_toacodec_widgets[] = {
> +	SND_SOC_DAPM_MUX("SRC", SND_SOC_NOPM, 0, 0,
> +			 &sm1_toacodec_mux),

[Severity: Medium]
Should s4_toacodec_widgets use a dedicated mux control with shift 19 instead
of sm1_toacodec_mux?

On S4, s4_toacodec_controls defines "Lane Select" at bit 16
(CTRL0_LANE_SEL_SM1) with max 7, occupying bits 18:16 of TOACODEC_CTRL0, and
s4_toacodec_match_data moves field_dat_sel to bits 20:19:

	.field_dat_sel	= REG_FIELD(TOACODEC_CTRL0, 19, 20),

While g12a_toacodec_mux_put_enum() writes to priv->field_dat_sel,
sm1_toacodec_mux uses sm1_toacodec_mux_enum, which hardcodes the shift to
CTRL0_DAT_SEL_SM1_LSB (bit 18):

static SOC_ENUM_SINGLE_DECL(sm1_toacodec_mux_enum, TOACODEC_CTRL0,
			    CTRL0_DAT_SEL_SM1_LSB,
			    g12a_toacodec_mux_texts);

When snd_soc_dapm_get_enum_double() or dapm_connect_mux() reads the "Source"
mux state on S4, won't it extract bits 19:18 (overlapping the MSB of "Lane
Select") instead of bits 20:19, returning the wrong enum selection or an
unmapped value of 3 when "I2S B" and lanes 4..7 are active?

> +	SND_SOC_DAPM_SWITCH("OUT EN", SND_SOC_NOPM, 0, 0,
> +			    &g12a_toacodec_out_enable),
> +	SND_SOC_DAPM_SUPPLY("I2S BCLK Gate", TOACODEC_CTRL0, CTRL0_BCLK_ENABLE_SHIFT, 0, NULL, 0),
> +	SND_SOC_DAPM_SUPPLY("I2S MCLK Gate", TOACODEC_CTRL0, CTRL0_MCLK_ENABLE_SHIFT, 0, NULL, 0),
> +
> +};

[ ... ]

> @@ -234,6 +257,10 @@ static const struct snd_kcontrol_new sm1_toacodec_controls[] = {
>  	SOC_SINGLE("Lane Select", TOACODEC_CTRL0, CTRL0_LANE_SEL_SM1, 3, 0),
>  };
>  
> +static const struct snd_kcontrol_new s4_toacodec_controls[] = {
> +	SOC_SINGLE("Lane Select", TOACODEC_CTRL0, CTRL0_LANE_SEL_SM1, 7, 0),
> +};

[ ... ]

> @@ -276,6 +314,13 @@ static const struct g12a_toacodec_match_data sm1_toacodec_match_data = {
>  	.field_bclk_sel	= REG_FIELD(TOACODEC_CTRL0, 4, 6),
>  };
>  
> +static const struct g12a_toacodec_match_data s4_toacodec_match_data = {
> +	.component_drv	= &s4_toacodec_component_drv,
> +	.field_dat_sel	= REG_FIELD(TOACODEC_CTRL0, 19, 20),
> +	.field_lrclk_sel = REG_FIELD(TOACODEC_CTRL0, 12, 14),
> +	.field_bclk_sel	= REG_FIELD(TOACODEC_CTRL0, 4, 6),
> +};

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-audio_drvier-v7-0-2b9aeae9cb8a@amlogic.com?part=3