Re: [PATCH v8 1/2] ASoC: rt5575: Add the codec driver for the ALC5575

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2025-12-01 11:59 AM, Oder Chiou wrote:
> The ALC5575 integrates an audio DSP that typically loads its firmware
> from an external flash via its own SPI host interface. In certain
> hardware configurations, the firmware can alternatively be loaded
> through the SPI client interface. The driver provides basic mute and
> volume control functions. When the SPI client interface is enabled,
> firmware loading is handled by the SPI driver.
> 
> Signed-off-by: Oder Chiou <oder_chiou@realtek.com>

...

> diff --git a/sound/soc/codecs/rt5575-spi.c b/sound/soc/codecs/rt5575-spi.c
> new file mode 100644
> index 000000000000..12c2379a061e
> --- /dev/null
> +++ b/sound/soc/codecs/rt5575-spi.c
> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * rt5575-spi.c  --  ALC5575 SPI driver
> + *
> + * Copyright(c) 2025 Realtek Semiconductor Corp.
> + *
> + */
> +
> +#include <linux/of.h>
> +#include <linux/spi/spi.h>
> +
> +#include "rt5575-spi.h"
> +
> +#define RT5575_SPI_CMD_BURST_WRITE	5
> +#define RT5575_SPI_BUF_LEN		240
> +
> +struct rt5575_spi_burst_write {
> +	u8 cmd;
> +	u32 addr;
> +	u8 data[RT5575_SPI_BUF_LEN];
> +	u8 dummy;
> +} __packed;
> +
> +struct spi_device *rt5575_spi;
> +EXPORT_SYMBOL_GPL(rt5575_spi);
> +
> +/**
> + * rt5575_spi_burst_write - Write data to SPI by rt5575 address.
> + * @addr: Start address.
> + * @txbuf: Data buffer for writing.
> + * @len: Data length.
> + *
> + */
> +int rt5575_spi_burst_write(u32 addr, const u8 *txbuf, size_t len)
> +{
> +	struct rt5575_spi_burst_write buf = {
> +		.cmd = RT5575_SPI_CMD_BURST_WRITE
> +	};
> +	unsigned int end, offset = 0;
> +
> +	while (offset < len) {
> +		if (offset + RT5575_SPI_BUF_LEN <= len)
> +			end = RT5575_SPI_BUF_LEN;
> +		else
> +			end = len % RT5575_SPI_BUF_LEN;
> +
> +		buf.addr = cpu_to_le32(addr + offset);
> +
> +		memcpy(&buf.data, &txbuf[offset], end);
> +
> +		spi_write(rt5575_spi, &buf, sizeof(buf));
> +
> +		offset += RT5575_SPI_BUF_LEN;

Make it cohesive by proper spacing - it's a logical block, and logical 
blocks are easier to read if grouped together e.g.:

<assuming newline after the if-else-statement>

		buf.addr = cpu_to_le32(addr + offset);
		memcpy(&buf.data, &txbuf[offset], end);
		spi_write(rt5575_spi, &buf, sizeof(buf));

		offset += RT5575_SPI_BUF_LEN;

See?  Clear separation of operations leading to spi_write() and the 
offset increment.

> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(rt5575_spi_burst_write);
> +
> +static int rt5575_spi_probe(struct spi_device *spi)
> +{
> +	rt5575_spi = spi;
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id rt5575_of_match[] = {
> +	{ .compatible = "realtek,rt5575" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, rt5575_of_match);
> +
> +static struct spi_driver rt5575_spi_driver = {
> +	.driver = {
> +		.name = "rt5575",
> +		.of_match_table = of_match_ptr(rt5575_of_match),
> +	},
> +	.probe = rt5575_spi_probe,
> +};
> +module_spi_driver(rt5575_spi_driver);
> +
> +MODULE_DESCRIPTION("ALC5575 SPI driver");
> +MODULE_AUTHOR("Oder Chiou <oder_chiou@realtek.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/sound/soc/codecs/rt5575-spi.h b/sound/soc/codecs/rt5575-spi.h
> new file mode 100644
> index 000000000000..b364b49bb43e
> --- /dev/null
> +++ b/sound/soc/codecs/rt5575-spi.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * rt5575-spi.h  --  ALC5575 SPI driver
> + *
> + * Copyright(c) 2025 Realtek Semiconductor Corp.
> + *
> + */
> +
> +#ifndef __RT5575_SPI_H__
> +#define __RT5575_SPI_H__
> +
> +extern struct spi_device *rt5575_spi;
> +
> +int rt5575_spi_burst_write(u32 addr, const u8 *txbuf, size_t len);
> +
> +#endif /* __RT5575_SPI_H__ */

The entire header can be dropped and its content moved to rt5575.h. 
It's included in both C-files plus we want to get rid of the global 
rt5575_spi anyway.  I do not think rt5575_spi_burst_write() belongs here 
either, see my comments regarding rt5575_fw_load_by_spi().

> diff --git a/sound/soc/codecs/rt5575.c b/sound/soc/codecs/rt5575.c
> new file mode 100644
> index 000000000000..c7e8f5a606bc
> --- /dev/null
> +++ b/sound/soc/codecs/rt5575.c
> @@ -0,0 +1,375 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * rt5575.c  --  ALC5575 ALSA SoC audio component driver
> + *
> + * Copyright(c) 2025 Realtek Semiconductor Corp.
> + *
> + */
> +
> +#include <linux/firmware.h>
> +#include <linux/i2c.h>
> +#include <sound/soc.h>
> +#include <sound/tlv.h>
> +
> +#include "rt5575.h"
> +#if IS_ENABLED(CONFIG_SND_SOC_RT5575_SPI)
> +#include "rt5575-spi.h"
> +#endif
> +
> +static bool rt5575_readable_register(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case RT5575_ID:
> +	case RT5575_ID_1:
> +	case RT5575_MIXL_VOL:
> +	case RT5575_MIXR_VOL:
> +	case RT5575_PROMPT_VOL:
> +	case RT5575_SPK01_VOL:
> +	case RT5575_SPK23_VOL:
> +	case RT5575_MIC1_VOL:
> +	case RT5575_MIC2_VOL:
> +	case RT5575_WNC_CTRL:
> +	case RT5575_MODE_CTRL:
> +	case RT5575_I2S_RATE_CTRL:
> +	case RT5575_SLEEP_CTRL:
> +	case RT5575_ALG_BYPASS_CTRL:
> +	case RT5575_PINMUX_CTRL_2:
> +	case RT5575_GPIO_CTRL_1:
> +	case RT5575_DSP_BUS_CTRL:
> +	case RT5575_SW_INT:
> +	case RT5575_DSP_BOOT_ERR:
> +	case RT5575_DSP_READY:
> +	case RT5575_DSP_CMD_ADDR:
> +	case RT5575_EFUSE_DATA_2:
> +	case RT5575_EFUSE_DATA_3:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static const DECLARE_TLV_DB_SCALE(ob_tlv, -9525, 75, 0);
> +
> +#if IS_ENABLED(CONFIG_SND_SOC_RT5575_SPI)
> +static void rt5575_fw_load_by_spi(const struct firmware *fw, void *context)

So, rt5575_spi_burst_write(), a wrapper for spi_write(), is exported to 
be available outside of the rt5575-spi module while its only user, 
rt5575_fw_load_by_spi(), which performs no I2C-specific tasks, is found 
with the common, rt5575.c file?

We can do better.  There are couple options here, one of them consists of:

1) privatize rt5575_spi_burst_write()
2) make rt5575_fw_load_by_spi() public
3) change rt5575_fw_load_by_spi() from void to int and return 
request_firmware_xxx() result
4) reword to rt5575_spi_fw_load() to match its friends

In regard to 1), have a #if-else preproc added to rt5575.h that governs 
the implementation of said function.  If xxx_RT5575_SPI is disabled, let 
is be a stub that returns 0.

In regard to 2), please do not ignore failures from firmware loading, 
that's a recurring point in this review and keeps being ignored.  No, 
async-firmware loading in not the answer why potential errors are left 
unhandled.

Another option, perhaps a better one is to have both 
rt5575_spi_burst_write() and rt5575_spi_fw_load() privatized and move 
the firmware-loading to the SPI-device probe.  See my comments targeting 
rt5575_probe().

> +{
> +	struct rt5575_priv *rt5575 = context;
> +	struct i2c_client *i2c = rt5575->i2c;

The whole reason this function needs i2c_client is ->dev retrieval. 
Let's simplify by listing 'struct device *dev' as an argument instead. 
With that, your function's argument list is also more explicit.

> +	const struct firmware *firmware;
> +	static const char * const fw_path[] = {
> +		"realtek/rt5575/rt5575_fw2.bin",
> +		"realtek/rt5575/rt5575_fw3.bin",
> +		"realtek/rt5575/rt5575_fw4.bin"
> +	};
> +	static const u32 fw_addr[] = { 0x5f600000, 0x5f7fe000, 0x5f7ff000 };
> +	int i, ret;
> +
> +	regmap_write(rt5575->dsp_regmap, 0xfafafafa, 0x00000004);
> +	regmap_write(rt5575->dsp_regmap, 0x18008064, 0x00000000);
> +	regmap_write(rt5575->dsp_regmap, 0x18008068, 0x0002ffff);
> +
> +	rt5575_spi_burst_write(0x5f400000, fw->data, fw->size);
> +	release_firmware(fw);
> +
> +	for (i = 0; i < ARRAY_SIZE(fw_addr); i++) {
> +		ret = request_firmware(&firmware, fw_path[i], &i2c->dev);
> +		if (!ret) {
> +			rt5575_spi_burst_write(fw_addr[i], firmware->data, firmware->size);
> +			release_firmware(firmware);
> +		} else {
> +			dev_err(&i2c->dev, "Request firmware failure: %d\n", ret);
> +			return;
> +		}
> +	}
> +
> +	regmap_write(rt5575->dsp_regmap, 0x18000000, 0x00000000);
> +
> +	regmap_update_bits(rt5575->regmap, RT5575_SW_INT, 1, 1);
> +
> +	regmap_read_poll_timeout(rt5575->regmap, RT5575_SW_INT, ret, !ret, 100000, 10000000);
> +
> +	if (ret)
> +		dev_err(&i2c->dev, "Run firmware failure: %d\n", ret);
> +}
> +#endif
> +
> +static const struct snd_kcontrol_new rt5575_snd_controls[] = {
> +	SOC_DOUBLE("Speaker CH-01 Playback Switch", RT5575_SPK01_VOL, 31, 15, 1, 1),
> +	SOC_DOUBLE_TLV("Speaker CH-01 Playback Volume", RT5575_SPK01_VOL, 17, 1, 167, 0, ob_tlv),
> +	SOC_DOUBLE("Speaker CH-23 Playback Switch", RT5575_SPK23_VOL, 31, 15, 1, 1),
> +	SOC_DOUBLE_TLV("Speaker CH-23 Playback Volume", RT5575_SPK23_VOL, 17, 1, 167, 0, ob_tlv),
> +	SOC_DOUBLE("Mic1 Capture Switch", RT5575_MIC1_VOL, 31, 15, 1, 1),
> +	SOC_DOUBLE_TLV("Mic1 Capture Volume", RT5575_MIC1_VOL, 17, 1, 167, 0, ob_tlv),
> +	SOC_DOUBLE("Mic2 Capture Switch", RT5575_MIC2_VOL, 31, 15, 1, 1),
> +	SOC_DOUBLE_TLV("Mic2 Capture Volume", RT5575_MIC2_VOL, 17, 1, 167, 0, ob_tlv),
> +	SOC_DOUBLE_R("Mix Playback Switch", RT5575_MIXL_VOL, RT5575_MIXR_VOL, 31, 1, 1),
> +	SOC_DOUBLE_R_TLV("Mix Playback Volume", RT5575_MIXL_VOL, RT5575_MIXR_VOL, 1, 127, 0,
> +		ob_tlv),
> +	SOC_DOUBLE("Prompt Playback Switch", RT5575_PROMPT_VOL, 31, 15, 1, 1),
> +	SOC_DOUBLE_TLV("Prompt Playback Volume", RT5575_PROMPT_VOL, 17, 1, 167, 0, ob_tlv),
> +};
> +
> +static const struct snd_soc_dapm_widget rt5575_dapm_widgets[] = {
> +	SND_SOC_DAPM_AIF_IN("AIF1RX", "AIF1 Playback", 0, SND_SOC_NOPM, 0, 0),
> +	SND_SOC_DAPM_AIF_OUT("AIF1TX", "AIF1 Capture", 0, SND_SOC_NOPM, 0, 0),
> +	SND_SOC_DAPM_AIF_IN("AIF2RX", "AIF2 Playback", 0, SND_SOC_NOPM, 0, 0),
> +	SND_SOC_DAPM_AIF_OUT("AIF2TX", "AIF2 Capture", 0, SND_SOC_NOPM, 0, 0),
> +	SND_SOC_DAPM_AIF_IN("AIF3RX", "AIF3 Playback", 0, SND_SOC_NOPM, 0, 0),
> +	SND_SOC_DAPM_AIF_OUT("AIF3TX", "AIF3 Capture", 0, SND_SOC_NOPM, 0, 0),
> +	SND_SOC_DAPM_AIF_IN("AIF4RX", "AIF4 Playback", 0, SND_SOC_NOPM, 0, 0),
> +	SND_SOC_DAPM_AIF_OUT("AIF4TX", "AIF4 Capture", 0, SND_SOC_NOPM, 0, 0),
> +
> +	SND_SOC_DAPM_INPUT("INPUT"),
> +	SND_SOC_DAPM_OUTPUT("OUTPUT"),
> +};
> +
> +static const struct snd_soc_dapm_route rt5575_dapm_routes[] = {
> +	{ "AIF1TX", NULL, "INPUT" },
> +	{ "AIF2TX", NULL, "INPUT" },
> +	{ "AIF3TX", NULL, "INPUT" },
> +	{ "AIF4TX", NULL, "INPUT" },
> +	{ "OUTPUT", NULL, "AIF1RX" },
> +	{ "OUTPUT", NULL, "AIF2RX" },
> +	{ "OUTPUT", NULL, "AIF3RX" },
> +	{ "OUTPUT", NULL, "AIF4RX" },
> +};
> +
> +static long long rt5575_get_priv_id(struct rt5575_priv *rt5575)
> +{
> +	int priv_id_low, priv_id_high;
> +
> +	regmap_write(rt5575->regmap, RT5575_EFUSE_PID, 0xa0000000);
> +	regmap_read(rt5575->regmap, RT5575_EFUSE_DATA_2, &priv_id_low);
> +	regmap_read(rt5575->regmap, RT5575_EFUSE_DATA_3, &priv_id_high);
> +	regmap_write(rt5575->regmap, RT5575_EFUSE_PID, 0);
> +
> +	return ((long long)priv_id_high << 32) | (long long)priv_id_low;
> +}
> +
> +static const struct of_device_id rt5575_of_match[] = {
> +	{ .compatible = "realtek,rt5575" },
> +	{ .compatible = "realtek,rt5575-with-spi" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, rt5575_of_match);
> +
> +static int rt5575_probe(struct snd_soc_component *component)
> +{
> +	struct rt5575_priv *rt5575 = snd_soc_component_get_drvdata(component);
> +	struct device *dev = component->dev;
> +
> +	rt5575->component = component;
> +
> +	dev_info(dev, "Private ID: %llx\n", rt5575_get_priv_id(rt5575));
> +
> +#if IS_ENABLED(CONFIG_SND_SOC_RT5575_SPI)
> +	if (of_device_is_compatible(dev->of_node, rt5575_of_match[1].compatible))
> +		request_firmware_nowait(THIS_MODULE, FW_ACTION_UEVENT,


An ASoC card is typically a mariage of platform-component (e.g.: SoC 
level DSP) and a codec-component (e.g.: rt5575).  If for any reason the 
platform-component becomes unloaded, codec-component will also be.  When 
the platform-component becomes available again, the 
codec-component->probe() would be invoked again - firmware would be 
loaded again.  Is this the desired behaviour?

To answer that, the follow up question is:  Is the firmware-loading for 
this particular solution, a chip -level operation or, a sound-card 
-level operation?  Typically firmware-loading is the former.  Once 
succeeeds, an ASoC component can be registered.  There is no reason to 
register ASoC component, which is mainly used for PCM/streaming reasons, 
if the preliminary setup - firmware-loading - is unsuccefull.

Perhaps a good approach is to move the firmware loading to the 
SPI-device's probe()!

> +		"realtek/rt5575/rt5575_fw1.bin", component->dev, GFP_KERNEL, rt5575,
> +		rt5575_fw_load_by_spi);
> +#endif
> +
> +	return 0;
> +}

...

> +
> +static const struct i2c_device_id rt5575_i2c_id[] = {
> +	{ "rt5575" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, rt5575_i2c_id);
> +
> +static int rt5575_i2c_probe(struct i2c_client *i2c)
> +{
> +	struct rt5575_priv *rt5575;
> +	struct device *dev = &i2c->dev;
> +	int ret, val;
> +
> +#if IS_ENABLED(CONFIG_SND_SOC_RT5575_SPI)
> +	if (!rt5575_spi && of_device_is_compatible(dev->of_node, rt5575_of_match[1].compatible))
> +		return -EPROBE_DEFER;
> +#endif
> +
> +	rt5575 = devm_kzalloc(dev, sizeof(struct rt5575_priv),
> +				GFP_KERNEL);
> +	if (rt5575 == NULL)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(i2c, rt5575);
> +
> +	rt5575->i2c = i2c;
> +
> +	rt5575->dsp_regmap = devm_regmap_init_i2c(i2c, &rt5575_dsp_regmap);
> +	if (IS_ERR(rt5575->dsp_regmap)) {
> +		ret = PTR_ERR(rt5575->dsp_regmap);
> +		dev_err(dev, "Failed to allocate register map: %d\n", ret);

"Failed to allocate DSP register map:" so it differs from the one below?

> +		return ret;
> +	}
> +
> +	rt5575->regmap = devm_regmap_init(dev, NULL, i2c, &rt5575_regmap);
> +	if (IS_ERR(rt5575->regmap)) {
> +		ret = PTR_ERR(rt5575->regmap);
> +		dev_err(dev, "Failed to allocate register map: %d\n", ret);
> +		return ret;
> +	}
> +
> +	regmap_read(rt5575->regmap, RT5575_ID, &val);
> +	if (val != RT5575_DEVICE_ID) {
> +		dev_err(dev, "Device with ID register %08x is not rt5575\n", val);
> +		return -ENODEV;
> +	}
> +
> +	regmap_read(rt5575->regmap, RT5575_ID_1, &val);
> +	if (!val) {
> +		dev_err(dev, "This is not formal version\n");
> +		return -ENODEV;
> +	}
> +
> +	return devm_snd_soc_register_component(dev, &rt5575_soc_component_dev, rt5575_dai,
> +		ARRAY_SIZE(rt5575_dai));

Alignment.

> +}
> +
> +static struct i2c_driver rt5575_i2c_driver = {
> +	.driver = {
> +		.name = "rt5575",
> +		.owner = THIS_MODULE,
> +		.of_match_table = of_match_ptr(rt5575_of_match),
> +	},
> +	.probe = rt5575_i2c_probe,
> +	.id_table = rt5575_i2c_id,
> +};
> +module_i2c_driver(rt5575_i2c_driver);
> +
> +MODULE_DESCRIPTION("ASoC ALC5575 driver");
> +MODULE_AUTHOR("Oder Chiou <oder_chiou@realtek.com>");
> +MODULE_LICENSE("GPL");