Re: [PATCH v1 2/6] ASoC: codecs: rk3308: expose per-channel microphone boost

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ASoC: codecs: rk3308: non-contiguous mic boost range on rev A
--

commit eb32aeb2bd3d3cce70cc5e7d254a08bf85314017
Author: Xing Zheng <zhengxing@rock-chips.com>

ASoC: codecs: rk3308: expose per-channel microphone boost

This commit adds per-channel microphone boost controls for all eight ADC
inputs on the RK3308 codec, using revision-specific TLV dB scales and
rejecting intermediate gain settings on revision A.

> diff --git a/sound/soc/codecs/rk3308_codec.c b/sound/soc/codecs/rk3308_codec.c
> index 7a18ff45e391b..fc7e20e82700b 100644
> --- a/sound/soc/codecs/rk3308_codec.c
> +++ b/sound/soc/codecs/rk3308_codec.c
> @@ -66,6 +66,84 @@ static const DECLARE_TLV_DB_RANGE(rk3308_codec_dac_lineout_gain_tlv,
>  	3, 3, TLV_DB_SCALE_ITEM(0,    0, 0),
>  );
>  
> +static const DECLARE_TLV_DB_RANGE(rk3308_codec_adc_mic_gain_tlv_a,
> +	0, 0, TLV_DB_SCALE_ITEM(0, 0, 0),
> +	3, 3, TLV_DB_SCALE_ITEM(2000, 0, 0),
> +);
> +
> +static const DECLARE_TLV_DB_RANGE(rk3308_codec_adc_mic_gain_tlv_b,
> +	0, 0, TLV_DB_SCALE_ITEM(0, 0, 0),
> +	1, 1, TLV_DB_SCALE_ITEM(660, 0, 0),
> +	2, 2, TLV_DB_SCALE_ITEM(1300, 0, 0),
> +	3, 3, TLV_DB_SCALE_ITEM(2000, 0, 0),
> +);
> +
> +static int rk3308_codec_mic_boost_put(struct snd_kcontrol *kcontrol,
> +				      struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> +	struct rk3308_codec_priv *rk3308 = snd_soc_component_get_drvdata(component);
> +	long gain = ucontrol->value.integer.value[0];
> +
> +	if (rk3308->codec_ver == ACODEC_VERSION_A && gain != 0 && gain != 3)
> +		return -EINVAL;
> +
> +	return snd_soc_put_volsw(kcontrol, ucontrol);
> +}
> +
> +static const struct snd_kcontrol_new rk3308_codec_mic_boost_controls[] = {
> +	SOC_SINGLE_EXT_TLV("MIC1 Boost Volume", RK3308_ADC_ANA_CON01(0),
> +			   RK3308_ADC_CH1_MIC_GAIN_SFT, RK3308_ADC_CH1_MIC_GAIN_MAX, 0,
> +			   snd_soc_get_volsw, rk3308_codec_mic_boost_put,
> +			   rk3308_codec_adc_mic_gain_tlv_b),

[Severity: Medium]
On codec revision A (ACODEC_VERSION_A), rk3308_codec_probe() ->
rk3308_codec_add_mic_boost_controls() in sound/soc/codecs/rk3308_codec.c
registers the controls in rk3308_codec_mic_boost_controls[] using
SOC_SINGLE_EXT_TLV() with RK3308_ADC_CH1_MIC_GAIN_MAX (3) and
snd_soc_info_volsw().

When userspace queries SNDRV_CTL_IOCTL_ELEM_INFO on ACODEC_VERSION_A,
snd_soc_info_volsw() advertises an integer control with min = 0 and max = 3,
indicating that integer steps 0, 1, 2, and 3 are all valid. However, writing
intermediate steps 1 or 2 via SNDRV_CTL_IOCTL_ELEM_WRITE ->
rk3308_codec_mic_boost_put() returns -EINVAL, and
rk3308_codec_adc_mic_gain_tlv_a leaves control values 1 and 2 unmapped in the
TLV dB range.

In ALSA, integer controls are expected to expose a contiguous range of valid
steps from min to max without holes, as verified by
test_ctl_write_valid_integer() in tools/testing/selftests/alsa/mixer-test.c:

	for (i = 0; i < snd_ctl_elem_info_get_count(ctl->info); i++) {
		for (j = snd_ctl_elem_info_get_min(ctl->info);
		     j <= snd_ctl_elem_info_get_max(ctl->info); j += step) {

			snd_ctl_elem_value_set_integer(val, i, j);
			err = write_and_verify(ctl, val, NULL);
			if (err != 0)
				fail = true;
		}
	}

Wouldn't advertising 0..3 on ACODEC_VERSION_A while rejecting values 1 and 2
cause single-step mixer adjustments (such as incrementing from 0 to 1), TLV
dB lookups on intermediate values, and the ALSA mixer kselftest to fail? Could
revision A expose a contiguous 0..1 control range (mapping 0 -> 0 and 1 -> 3
in the register) instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101040125.1864141-1-zhengxing@rock-chips.com?part=2