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

[Mark Brown]

On Thu, Oct 15, 2026 at 03:04:07PM +0900, Hyunuk Son wrote:
> 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.

There's some issues below, but nothing too major I think.

> +static bool ma5311_adc_volatile_register(struct device *dev, unsigned int reg)
> +{
> +	return false;
> +}

You can just not provide a volatile function, the default is nothing
volatile.  Since the driver isn't enabling a register cache volatile
makes no difference anyway.

> +static int ma5311_reset(struct snd_soc_component *component)
> +{

> +	ma5311->adc1_vol_left = 0xc0;
> +	ma5311->adc1_vol_right = 0xc0;
> +	ma5311->adc2_vol_left = 0xc0;
> +	ma5311->adc2_vol_right = 0xc0;
> +	ma5311->adc3_vol_left = 0xc0;
> +	ma5311->adc3_vol_right = 0xc0;
> +	ma5311->adc4_vol_left = 0xc0;
> +	ma5311->adc4_vol_right = 0xc0;
> +	ma5311->adc1_pga_left = 0x10;
> +	ma5311->adc1_pga_right = 0x10;
> +	ma5311->adc2_pga_left = 0x10;
> +	ma5311->adc2_pga_right = 0x10;
> +	ma5311->adc3_pga_left = 0x10;
> +	ma5311->adc3_pga_right = 0x10;
> +	ma5311->adc4_pga_left = 0x10;
> +	ma5311->adc4_pga_right = 0x10;
> +	ma5311->capture_mode = I2S_ADC1;

Normally we just go with the hardware defaults for everything and let
userspace set what it needs, this is mainly because different use cases
often need different defaults.  Why are we setting all these volumes
here?

> +static int ma5311_adc_pga_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 *cache;
> +	unsigned int val = ucontrol->value.integer.value[0];
> +
> +	if (val < 0 || val > MA5311_ADC_PGA_CTRL_MASK)
> +		return -EINVAL;
> +
> +	cache = ma5311_adc_pga_control_cache(ma5311, mc->reg);

It's not clear to me why we have this cache code - I didn't spot where
do anything with it outside of these get/put operations, it looks like
exactly the sort of cache that you'd get from the regmap cache code.
AFAICT could get it for free and just use the standard control
implementations by enabling a register cache in regmap, if you don't
want a cache for other registers for some reason you can mark them as
volatile.

> +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) {
> +		dev_err(ma5311->dev, "Try after capture is complete.\n");
> +		return -EBUSY;
> +	}

There's no locking between this and DAPM so we could race with power up
here.  Taking the DAPM mutex ought to do enough.

> +static int ma5311_reset_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);
> +
> +	if (!ma5311->reset_gpio)
> +		return -ENODEV;

It'd be better to only register this control if the GPIO is present,
that way we don't have a broken control that reports errors to userspace
if the system can't support it.  It's a bit of a dodgy control to have
in the first place, it's rather disruptive if used at the wrong time,
but I'm not totally against it for devices that are likely to only be
used with highly controlled userspaces.

> +	switch (params_width(params)) {
> +	case 16:
> +	case 24:
> +	case 32:
> +		break;
> +	default:
> +		dev_err(component->dev, "%s: unsupported PCM format: %d\n",
> +			__func__, params_format(params));
> +		return -EINVAL;
> +	}

Don't we need to configure anything in the hardware for clock provider
mode here?

> +#define MA5311_RATES_PLAYBACK SNDRV_PCM_RATE_8000_96000
> +#define MA5311_RATES_CAPTURE SNDRV_PCM_RATE_8000_96000
> +#define MA5311_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S32_LE)

These don't seem to match what hw_params() accepts.

> +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,
> +	  },
> +};

I think the device needs symmetric rate and widths too?

> +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;

This should be IRQ_NONE, we're returning irqreturn_t not an error code.

Attachment: signature.asc
Description: PGP signature