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

[Krzysztof Kozlowski <krzk@kernel.org>]

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.
> 
> Signed-off-by: Hyunuk Son <hyunuk.son@irondevice.com>

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

...

> +static void ma5311_unregister_ancillary_client(void *data)
> +{
> +	struct i2c_client *client = data;
> +
> +	i2c_unregister_device(client);
> +}
> +
> +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;
> +	}
> +	if (dac_status & MA5311_DAC_EA_FAULT_MASK)
> +		dev_crit(ma5311->dev, "FAULT asserted\n");
> +
> +	if (!(dac_ocp_status & MA5311_DAC_EB_OCP_1A_MASK))
> +		dev_crit(ma5311->dev, "CH1A OCP\n");
> +
> +	if (~dac_status & MA5311_DAC_EA_UVLON_CH1_MASK)
> +		dev_crit(ma5311->dev, "%s: CH1 UVLO\n", __func__);
> +
> +	if (~dac_status & MA5311_DAC_EA_UVLON_CH2_MASK)
> +		dev_crit(ma5311->dev, "%s: CH2 UVLO\n", __func__);
> +
> +	if (dac_status & MA5311_DAC_EA_TSDW_MASK)
> +		dev_crit(ma5311->dev, "%s: TSD Warning\n", __func__);
> +
> +	if (dac_status & MA5311_DAC_EA_TSD_MASK)
> +		dev_crit(ma5311->dev, "%s: TSD Fault\n", __func__);
> +
> +	if (~dac_ocp_status & MA5311_DAC_EB_IPSC_2U_MASK)
> +		dev_crit(ma5311->dev,
> +			 "%s: IPSC CH2 Highside Fault\n", __func__);

How many errors are you going to print during some failure?

> +
> +	if (~dac_ocp_status & MA5311_DAC_EB_IPSC_2L_MASK)
> +		dev_crit(ma5311->dev, "%s: IPSC CH2 Lowside Fault\n", __func__);
> +
> +	if (~dac_ocp_status & MA5311_DAC_EB_IPSC_1U_MASK)
> +		dev_crit(ma5311->dev,
> +			 "%s: IPSC CH1 Highside Fault\n", __func__);
> +
> +	if (~dac_ocp_status & MA5311_DAC_EB_IPSC_1L_MASK)
> +		dev_crit(ma5311->dev, "%s: IPSC CH1 Lowside Fault\n", __func__);
> +
> +	if (~dac_scp_status & MA5311_DAC_EC_SCP_2B_MASK)
> +		dev_crit(ma5311->dev, "%s: CH2B SCP\n", __func__);
> +
> +	if (~dac_scp_status & MA5311_DAC_EC_SCP_2A_MASK)
> +		dev_crit(ma5311->dev, "%s: CH2A SCP\n", __func__);
> +
> +	if (~dac_scp_status & MA5311_DAC_EC_SCP_1B_MASK)
> +		dev_crit(ma5311->dev, "%s: CH1B SCP\n", __func__);
> +
> +	if (~dac_scp_status & MA5311_DAC_EC_SCP_1A_MASK)
> +		dev_crit(ma5311->dev, "%s: CH1A SCP\n", __func__);
> +
> +	dev_crit(ma5311->dev, "MA5311 INTN fault detected: irq=%d\n", irq);

Not really critical. That's just a codec.

Also, this should be ratelimited.

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ma5311_i2c_probe(struct i2c_client *client)
> +{
> +	struct ma5311_priv *ma5311;
> +	int ret;
> +
> +	ma5311 = devm_kzalloc(&client->dev, sizeof(*ma5311), GFP_KERNEL);
> +
> +	if (!ma5311)
> +		return -ENOMEM;
> +
> +	ma5311->ctrl_regmap =
> +	    devm_regmap_init_i2c(client, &ma5311_ctrl_regmap_config);

You use odd formatting. Was this clang? It's known for making it
non-kernel style (see Linux kernel coding style)

> +
> +	if (IS_ERR(ma5311->ctrl_regmap))
> +		return dev_err_probe(&client->dev, PTR_ERR(ma5311->ctrl_regmap),
> +				     "%s: Failed to allocate control regmap\n",
> +				     __func__);
> +
> +	ma5311->adc_client = i2c_new_ancillary_device(client, "adc", 0x3f);
> +

Drop such blank lines after assignment and before check.

> +	if (IS_ERR(ma5311->adc_client))
> +		return dev_err_probe(&client->dev, PTR_ERR(ma5311->adc_client),
> +				     "Failed to create ADC ancillary client\n");
> +
> +	ret = devm_add_action_or_reset(&client->dev,
> +				       ma5311_unregister_ancillary_client,
> +				       ma5311->adc_client);
> +	if (ret)
> +		return ret;
> +
> +	ma5311->adc_regmap = devm_regmap_init_i2c(ma5311->adc_client,
> +						  &ma5311_adc_regmap_config);
> +
> +	if (IS_ERR(ma5311->adc_regmap))
> +		return dev_err_probe(&client->dev, PTR_ERR(ma5311->adc_regmap),
> +				     "Failed to allocate ADC regmap\n");
> +
> +	/* Logical 1 asserts reset, using the polarity from reset-gpios. */

No need to explain basics of GPIO handling in Linux kernel.

> +	ma5311->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> +						     GPIOD_OUT_LOW);
> +	if (IS_ERR(ma5311->reset_gpio))
> +		return dev_err_probe(&client->dev, PTR_ERR(ma5311->reset_gpio),
> +				     "Failed to acquire reset GPIO\n");
> +
> +	if (ma5311->reset_gpio)
> +		msleep(MA5311_RESET_RECOVERY_MS);
> +
> +	ma5311->amp_mode = MA5311_MONO_BOTH_MODE;
> +	ma5311->num_of_pll_matches = ARRAY_SIZE(ma5311_pll_matches);
> +	ma5311->pll_matches = ma5311_pll_matches;
> +	ma5311->format = SND_SOC_DAIFMT_I2S;
> +	ma5311->sys_clk_id = MA5311_PLL_CLKIN_BCLK;
> +	ma5311->dev = &client->dev;
> +
> +	i2c_set_clientdata(client, ma5311);
> +
> +	ret = devm_snd_soc_register_component(&client->dev, &ma5311_component,
> +					      ma5311_dai, 1);
> +
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to register component: %d\n",
> +			ret);
> +
> +		return ret;
> +	}
> +
> +	dev_dbg(&client->dev, "MA5311 client->irq = %d\n", client->irq);
> +
> +	if (client->irq > 0) {
> +		ret = devm_request_threaded_irq(&client->dev,
> +						client->irq,
> +						NULL,
> +						ma5311_irq_thread,
> +						IRQF_ONESHOT, "ma5311", ma5311);
> +		if (ret)
> +			return dev_err_probe(&client->dev, ret,
> +					     "Failed to request IRQ %d\n",
> +					     client->irq);
> +
> +		dev_dbg(&client->dev, "MA5311 IRQ %d registered\n",
> +			client->irq);

How many debug messages for simple IRQ?

> +	} else {
> +		dev_warn(&client->dev, "No IRQ configured\n");
> +	}
> +
> +	dev_dbg(&client->dev, "Codec component registered\n");

Drop. debug for standard probe success is completely redundant.

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

Odd formattting

> +
> +MODULE_DEVICE_TABLE(i2c, ma5311_i2c_id);
> +
> +static const struct of_device_id ma5311_of_match[] = {
> +	{
> +	 .compatible = "irondevice,ma5311",
> +	  },

Even odder... Please look at recent code how this is written.


Best regards,
Krzysztof