Re: [PATCH 2/2] ASoC: codecs: Add driver for Iron Device MA5311

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] asoc: missing symmetric constraints allows concurrent PLL reconfiguration
- [Medium] irq: thread handler incorrectly returns negative error codes
- [Medium] asoc: uninitialized kernel stack memory leak in ALSA control getters
- [Medium] asoc: hardware reset ALSA control irrevocably desynchronizes DAPM state
- [Medium] asoc: falsely advertised 44.1kHz capability fails unconditionally
- [Medium] asoc: ma5311: TOCTOU race between capture_mode_put and DAPM events
- [Low] i2c: missing named initializers for i2c_device_id array
--

commit cea868f0ad866edece054bdcc99ee119173a63e4
Author: Hyunuk Son <hyunuk.son@irondevice.com>
Date:   Thu Oct 15 06:04:07 2026 +0000

    ASoC: codecs: Add driver for Iron Device MA5311
    
    Add an ASoC codec driver for the Iron Device MA5311.
    
    The driver supports audio playback and capture through I2S
    and TDM interfaces. It provides ALSA mixer controls for audio
    configuration and uses regmap for register access.

> diff --git a/sound/soc/codecs/ma5311.c b/sound/soc/codecs/ma5311.c
> new file mode 100644
> index 0000000000000..ea6d6793bfb21
> --- /dev/null
> +++ b/sound/soc/codecs/ma5311.c

[ ... ]

> +static int ma5311_adc_volume_control_get(struct snd_kcontrol *kcontrol,
> +					 struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> +	struct ma5311_priv *ma5311 = snd_soc_component_get_drvdata(component);
> +	const struct soc_mixer_control *mc =
> +	    (const struct soc_mixer_control *)kcontrol->private_value;
> +	unsigned int *cache = ma5311_adc_volume_control_cache(ma5311, mc->reg);
> +	unsigned int val;
> +
> +	if (!cache)
> +		return -EINVAL;
> +
> +	regmap_read(ma5311->adc_regmap, mc->reg, &val);
> +
> +	ucontrol->value.integer.value[0] = val & MA5311_ADC_VOL_CTRL_MASK;

[Severity: Medium]
Does this code leak uninitialized kernel stack memory to userspace if the I2C
transaction fails? The driver uses REGCACHE_NONE, so every read triggers a
physical I2C transaction. If regmap_read() fails and returns a negative error
code, val remains uninitialized, but it is masked and copied to the ucontrol
value anyway.

> +	*cache = ucontrol->value.integer.value[0];
> +
> +	return 0;
> +}
> +
> +static int ma5311_adc_volume_control_put(struct snd_kcontrol *kcontrol,
> +					 struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> +	struct ma5311_priv *ma5311 = snd_soc_component_get_drvdata(component);
> +	const struct soc_mixer_control *mc =
> +	    (const struct soc_mixer_control *)kcontrol->private_value;
> +	unsigned int val = ucontrol->value.integer.value[0];
> +	unsigned int *cache;
> +
> +	if (val < 0 || val > MA5311_ADC_VOL_CTRL_MASK)
> +		return -EINVAL;
> +
> +	cache = ma5311_adc_volume_control_cache(ma5311, mc->reg);
> +
> +	if (!cache)
> +		return -EINVAL;
> +
> +	if (*cache == val)
> +		return 0;
> +
> +	regmap_update_bits(ma5311->adc_regmap, mc->reg,
> +			   MA5311_ADC_VOL_CTRL_MASK, val);

[Severity: Medium]
Should the software cache update be skipped if the regmap update fails? The
return value of regmap_update_bits() is ignored here, which silently updates
the software cache even when the hardware I2C write fails.

> +
> +	*cache = val;
> +
> +	return 1;
> +}

[ ... ]

> +static int ma5311_capture_mode_put(struct snd_kcontrol *kcontrol,
> +				   struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> +	struct ma5311_priv *ma5311 = snd_soc_component_get_drvdata(component);
> +	unsigned int val = ucontrol->value.enumerated.item[0];
> +
> +	if (val >= ARRAY_SIZE(ma5311_capture_mode_text)) {
> +		dev_err(ma5311->dev, "%s: Out of range\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	if (ma5311->capture_mode == val)
> +		return 0;
> +
> +	if (ma5311->adc_power_status) {

[Severity: Medium]
Could this lockless check of adc_power_status lead to a TOCTOU race window?
If a user-space application writes to this mixer control concurrently with
the start or stop of a stream, ma5311_adc_event() (which holds the
dapm_mutex) might update the hardware state right after this check passes,
leading to corrupted audio capture and an inconsistent hardware state.

> +		dev_err(ma5311->dev, "Try after capture is complete.\n");
> +		return -EBUSY;
> +	}

[ ... ]

> +static int ma5311_reset_control_put(struct snd_kcontrol *kcontrol,
> +				    struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> +	struct ma5311_priv *ma5311 = snd_soc_component_get_drvdata(component);
> +	unsigned int val = ucontrol->value.enumerated.item[0];
> +	int ret;
> +
> +	if (!val)
> +		return 0;
> +
> +	if (!ma5311->reset_gpio)
> +		return -ENODEV;
> +
> +	ma5311->last_bclk = 0;
> +
> +	gpiod_set_value_cansleep(ma5311->reset_gpio, 1);
> +	msleep(MA5311_RESET_ASSERT_MS);
> +	gpiod_set_value_cansleep(ma5311->reset_gpio, 0);
> +	msleep(MA5311_RESET_RECOVERY_MS);
> +
> +	ret = ma5311_reset(component);

[Severity: Medium]
Will this hardware reset irrevocably desynchronize the DAPM state if
triggered during an active stream? Because the regmaps use REGCACHE_NONE and
this operation holds no stream locks, the ALSA framework cannot automatically
restore the previously configured dynamic state (PLL settings, DAPM routing,
etc) after the reset.

> +	dev_dbg(component->dev, "Hardware Reset Done\n");
> +	return ret;
> +}

[ ... ]

> +static int ma5311_dai_hw_params(struct snd_pcm_substream *substream,
> +				struct snd_pcm_hw_params *params,
> +				struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct ma5311_priv *ma5311 = snd_soc_component_get_drvdata(component);
> +	unsigned int bclk = 0;
> +	int ret;
> +
> +	switch (params_rate(params)) {
> +	case 8000:
> +	case 12000:
> +	case 16000:
> +	case 24000:
> +	case 32000:
> +	case 44100:

[Severity: Medium]
Does the advertised support for 44.1kHz streams unconditionally fail in
ma5311_setup_pll()? The hardware parameters explicitly accept 44100 here,
but the ma5311_pll_matches table only contains exact multiples of 48kHz,
which will result in no matching BCLK entry and a failure to configure
the PLL.

> +	case 48000:
> +	case 96000:
> +		break;

[ ... ]

> +static struct snd_soc_dai_driver ma5311_dai[] = {
> +	{
> +	 .name = "ma5311-amplifier",
> +	 .id = 0,
> +	 .playback = {
> +		      .stream_name = "Playback",
> +		      .channels_min = 1,
> +		      .channels_max = 2,
> +		      .rates = MA5311_RATES_PLAYBACK,
> +		      .formats = MA5311_FORMATS,
> +		      },
> +	 .capture = {
> +		     .stream_name = "Capture",
> +		     .channels_min = 1,
> +		     .channels_max = 8,
> +		     .rates = MA5311_RATES_CAPTURE,
> +		     .formats = MA5311_FORMATS,
> +		     },
> +	 .ops = &ma5311_dai_ops_amp,

[Severity: High]
Is it possible for concurrent playback and capture streams with different
rates to silently overwrite the shared PLL settings? Without the
.symmetric_rates = 1, .symmetric_channels = 1, and .symmetric_samplebits = 1
flags, ALSA won't enforce symmetry. This means a second stream could
reconfigure the PLL via ma5311_setup_pll() and instantly break the clock and
format for the first stream.

> +	  },
> +};

[ ... ]

> +static irqreturn_t ma5311_irq_thread(int irq, void *data)
> +{
> +	struct ma5311_priv *ma5311 = data;
> +	unsigned int dac_status;
> +	unsigned int dac_ocp_status;
> +	unsigned int dac_scp_status;
> +	int ret = 0;
> +
> +	ret |= regmap_read(ma5311->ctrl_regmap,
> +			   MA5311_DAC_EA_DAC_STATUS, &dac_status);
> +
> +	ret |= regmap_read(ma5311->ctrl_regmap,
> +			   MA5311_DAC_EB_DAC_OCP_STATUS, &dac_ocp_status);
> +
> +	ret |= regmap_read(ma5311->ctrl_regmap,
> +			   MA5311_DAC_EC_DAC_SCP_STATUS, &dac_scp_status);
> +
> +	if (ret) {
> +		dev_crit(ma5311->dev,
> +			 "Error Occurred During Interrupt Handling\n");
> +		return ret;

[Severity: Medium]
Should this return IRQ_NONE instead of the negative error code from
regmap_read()? Returning a negative errno value from an IRQ thread handler
causes the core IRQ subsystem to misinterpret the state, which can trigger
spurious interrupt warnings and permanently disable the interrupt line.

> +	}

[ ... ]

> +static const struct i2c_device_id ma5311_i2c_id[] = { { "ma5311", 0 }, { } };

[Severity: Low]
Could you use named initializers for this array element? The global I2C
subsystem guidelines require arrays of type struct i2c_device_id to use named
initializers (e.g., { .name = "ma5311", .driver_data = 0 }).

> +
> +MODULE_DEVICE_TABLE(i2c, ma5311_i2c_id);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015060408.128711-1-hyunuk.son@irondevice.com?part=2