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

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");