Skip to contentSkip to conversation

ASoC: rt5575: Add the codec driver for the ALC5575

PATCH Superseded Oder Chiou 3 participants 19 messages Dec 1, 2025 – Dec 10, 2025

Subsystems (4)

v8 of 11 Superseded Latest is v11 →

lore
Share & tools
lore

Oder Chiou

author maintainer
#0 lore raw

v8 cover ASoC: rt5575: Add the codec driver for the ALC5575

Hi all,

This patch series adds support for the Realtek ALC5575 audio codec.

Changes in v8:
- Patch 1/2:
- remove the variable rt5575_spi_ready
- use the multiple compatible names to distinguish between w/wo flash
- Patch 2/2
- add compatible enum "realtek,rt5575-with-spi"

Changes in v7:
- Patch 1/2:
- add a caption for the tristates
- remove the redundant enum of the SPI command
- add the error log in the request firmware failure
- change the function name rt5575_spi_fw_loaded to rt5575_fw_load_by_spi
- minor fixes
- Patch 2/2
- modify commit message
- Link to v7: https://lore.kernel.org/all/[email protected]/

Changes in v6:
- Patch 1/2:
- modify commit message
- add select SND_SOC_RT5575 to config SND_SOC_RT5575_SPI in the Kconfig
- revise the boiler plate in the head of the file
- sort the include files
- use a structure to transfer the spi data
- use the poll() related function instead the for-loop
- revise the UUID to the private ID
- minor fixes
- Patch 2/2
- modify description
- Link to v6: https://lore.kernel.org/all/[email protected]/

Changes in v2 to v5:
- Patch 1/2:
- move the firmware to the subdirectory
- remove the empty functions
- remove the cache_type in the regmap_config
- add the error log in the run firmware failure
- Patch 2/2:
- nothing
- Link to v5: https://lore.kernel.org/all/[email protected]/

Oder Chiou (2):
ASoC: rt5575: Add the codec driver for the ALC5575
ASoC: dt-bindings: realtek,rt5575: add bindings for ALC5575

 .../bindings/sound/realtek,rt5575.yaml        |  44 ++
 sound/soc/codecs/Kconfig                      |  10 +
 sound/soc/codecs/Makefile                     |   4 +
 sound/soc/codecs/rt5575-spi.c                 |  84 ++++
 sound/soc/codecs/rt5575-spi.h                 |  16 +
 sound/soc/codecs/rt5575.c                     | 375 ++++++++++++++++++
 sound/soc/codecs/rt5575.h                     |  54 +++
 7 files changed, 587 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/realtek,rt5575.yaml
 create mode 100644 sound/soc/codecs/rt5575-spi.c
 create mode 100644 sound/soc/codecs/rt5575-spi.h
 create mode 100644 sound/soc/codecs/rt5575.c
 create mode 100644 sound/soc/codecs/rt5575.h
Signature · 1 line
2.52.0
author maintainer
#1 lore raw

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

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.

---
 sound/soc/codecs/Kconfig      |  10 +
 sound/soc/codecs/Makefile     |   4 +
 sound/soc/codecs/rt5575-spi.c |  84 ++++++++
 sound/soc/codecs/rt5575-spi.h |  16 ++
 sound/soc/codecs/rt5575.c     | 375 ++++++++++++++++++++++++++++++++++
 sound/soc/codecs/rt5575.h     |  54 +++++
 6 files changed, 543 insertions(+)
 create mode 100644 sound/soc/codecs/rt5575-spi.c
 create mode 100644 sound/soc/codecs/rt5575-spi.h
 create mode 100644 sound/soc/codecs/rt5575.c
 create mode 100644 sound/soc/codecs/rt5575.h
sound/soc/codecs/Kconfig +10 -0
index 6087ebde9523..484618cf3cdd 100644
@@ -212,6 +212,7 @@ config SND_SOC_ALL_CODECS
 	imply SND_SOC_RT1305
 	imply SND_SOC_RT1308
 	imply SND_SOC_RT5514
+	imply SND_SOC_RT5575
 	imply SND_SOC_RT5616
 	imply SND_SOC_RT5631
 	imply SND_SOC_RT5640
@@ -1784,6 +1785,15 @@ config SND_SOC_RT5514_SPI_BUILTIN
 	bool # force RT5514_SPI to be built-in to avoid link errors
 	default SND_SOC_RT5514=y && SND_SOC_RT5514_SPI=m
 
+config SND_SOC_RT5575
+	tristate "Realtek ALC5575 Codec - I2C"
+	depends on I2C
+
+config SND_SOC_RT5575_SPI
+	tristate "Realtek ALC5575 Codec - SPI"
+	depends on SPI_MASTER
+	select SND_SOC_RT5575
+
 config SND_SOC_RT5616
 	tristate "Realtek RT5616 CODEC"
 	depends on I2C
sound/soc/codecs/Makefile +4 -0
index d687d4f74363..1b9642318f77 100644
@@ -253,6 +253,8 @@ snd-soc-rt286-y := rt286.o
 snd-soc-rt298-y := rt298.o
 snd-soc-rt5514-y := rt5514.o
 snd-soc-rt5514-spi-y := rt5514-spi.o
+snd-soc-rt5575-y := rt5575.o
+snd-soc-rt5575-spi-y := rt5575-spi.o
 snd-soc-rt5616-y := rt5616.o
 snd-soc-rt5631-y := rt5631.o
 snd-soc-rt5640-y := rt5640.o
@@ -686,6 +688,8 @@ obj-$(CONFIG_SND_SOC_RT298)	+= snd-soc-rt298.o
 obj-$(CONFIG_SND_SOC_RT5514)	+= snd-soc-rt5514.o
 obj-$(CONFIG_SND_SOC_RT5514_SPI)	+= snd-soc-rt5514-spi.o
 obj-$(CONFIG_SND_SOC_RT5514_SPI_BUILTIN)	+= snd-soc-rt5514-spi.o
+obj-$(CONFIG_SND_SOC_RT5575)	+= snd-soc-rt5575.o
+obj-$(CONFIG_SND_SOC_RT5575_SPI)	+= snd-soc-rt5575-spi.o
 obj-$(CONFIG_SND_SOC_RT5616)	+= snd-soc-rt5616.o
 obj-$(CONFIG_SND_SOC_RT5631)	+= snd-soc-rt5631.o
 obj-$(CONFIG_SND_SOC_RT5640)	+= snd-soc-rt5640.o
sound/soc/codecs/rt5575-spi.c new file +84 -0

84 lines not on this page · raw message

sound/soc/codecs/rt5575-spi.h new file +16 -0
new file mode 100644
index 000000000000..b364b49bb43e
@@ -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__ */
sound/soc/codecs/rt5575.c new file +375 -0

375 lines not on this page · raw message

sound/soc/codecs/rt5575.h new file +54 -0

54 lines not on this page · raw message

Signature · 1 line
2.52.0
#2 lore raw
Quoted 22 lines · Oder Chiou
sound/soc/codecs/rt5575.c new file +22 -0
+
+static int rt5575_i2c_read(void *context, unsigned int reg, unsigned int *val)
+{
+	struct i2c_client *client = context;
+	struct rt5575_priv *rt5575 = i2c_get_clientdata(client);
+
+	regmap_read(rt5575->dsp_regmap, reg | RT5575_DSP_MAPPING, val);
+
+	return 0;
+}
+
+static int rt5575_i2c_write(void *context, unsigned int reg, unsigned int val)
+{
+	struct i2c_client *client = context;
+	struct rt5575_priv *rt5575 = i2c_get_clientdata(client);
+
+	regmap_write(rt5575->dsp_regmap, reg | RT5575_DSP_MAPPING, val);
+
+	return 0;
+}
+
+static const struct regmap_config rt5575_regmap = {

sound/soc/codecs/rt5575.c line 304

Quoted 10 lines
sound/soc/codecs/rt5575.c new file +10 -0
+	.reg_bits = 16,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.max_register = 0xfffc,
+	.readable_reg = rt5575_readable_register,
+	.reg_read = rt5575_i2c_read,
+	.reg_write = rt5575_i2c_write,
+	.use_single_read = true,
+	.use_single_write = true,
+};

OF device ID table goes around here - together with I2C.

Quoted 4 lines
sound/soc/codecs/rt5575.c new file +4 -0
+
+static const struct i2c_device_id rt5575_i2c_id[] = {
+	{ "rt5575" },
+	{ }

sound/soc/codecs/rt5575.c line 318

Quoted 10 lines
sound/soc/codecs/rt5575.c new file +10 -0
+};
+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)

No ifdefs in driver code.

sound/soc/codecs/rt5575.c line 319

Quoted 1 line
sound/soc/codecs/rt5575.c new file +1 -0
+	if (!rt5575_spi && of_device_is_compatible(dev->of_node, rt5575_of_match[1].compatible))

No, use driver match data if ever, but this is just wrong. You said it depends on SPI flash, not SPI interface.

sound/soc/codecs/rt5575.c line 325

Quoted 6 lines
sound/soc/codecs/rt5575.c new file +6 -0
+		return -EPROBE_DEFER;
+#endif
+
+	rt5575 = devm_kzalloc(dev, sizeof(struct rt5575_priv),
+				GFP_KERNEL);
+	if (rt5575 == NULL)

This is not Linux coding style. Open existing drivers. It's everywhere written (!foo)

Quoted 30 lines
sound/soc/codecs/rt5575.c new file +30 -0
+		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);
+		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;

sound/soc/codecs/rt5575.c line 365

Quoted 10 lines
sound/soc/codecs/rt5575.c new file +10 -0
+	}
+
+	return devm_snd_soc_register_component(dev, &rt5575_soc_component_dev, rt5575_dai,
+		ARRAY_SIZE(rt5575_dai));
+}
+
+static struct i2c_driver rt5575_i2c_driver = {
+	.driver = {
+		.name = "rt5575",
+		.owner = THIS_MODULE,

Please drop it.

Please run standard kernel tools for static analysis, like coccinelle, smatch and sparse, and fix reported warnings. Also please check for warnings when building with W=1 for gcc and clang. Most of these commands (checks or W=1 build) can build specific targets, like some directory, to narrow the scope to only your code. The code here looks like it needs a fix. Feel free to get in touch if the warning is not clear.

Review

Quoted 1 line
(quoted diff) +1 -0
+		.of_match_table = of_match_ptr(rt5575_of_match),

You have warning here. Drop of_match_ptr.

sound/soc/codecs/rt5575.c

Quoted 5 lines
sound/soc/codecs/rt5575.c new file +5 -0
+	},
+	.probe = rt5575_i2c_probe,
+	.id_table = rt5575_i2c_id,
+};
+module_i2c_driver(rt5575_i2c_driver)

Best regards,
Krzysztof

author maintainer
#3 lore raw
Quoted 50 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Monday, December 8, 2025 2:05 PM
To: Oder Chiou <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Flove(HsinFu) <[email protected]>; Shuming [范 書銘] <[email protected]>; Jack Yu <[email protected]>; Derek [方德 義] <[email protected]>
Subject: Re: [PATCH v8 1/2] ASoC: rt5575: Add the codec driver for the ALC5575

External mail : This email originated from outside the organization. Do not reply, click links, or open attachments unless you recognize the sender and know the content is safe.

Review

Quoted 2 lines · Oder Chiou
(quoted diff) +2 -0
+
+static int rt5575_i2c_read(void *context, unsigned int reg, unsigned int

*val)

sound/soc/codecs/rt5575.c

Quoted 5 lines
sound/soc/codecs/rt5575.c new file +5 -0
+{
+     struct i2c_client *client = context;
+     struct rt5575_priv *rt5575 = i2c_get_clientdata(client);
+
+     regmap_read(rt5575->dsp_regmap, reg | RT5575_DSP_MAPPING,

val);

sound/soc/codecs/rt5575.c

Quoted 10 lines
sound/soc/codecs/rt5575.c new file +10 -0
+
+     return 0;
+}
+
+static int rt5575_i2c_write(void *context, unsigned int reg, unsigned int val)
+{
+     struct i2c_client *client = context;
+     struct rt5575_priv *rt5575 = i2c_get_clientdata(client);
+
+     regmap_write(rt5575->dsp_regmap, reg | RT5575_DSP_MAPPING,

val);

Quoted 9 lines
sound/soc/codecs/rt5575.c new file +9 -0
+
+     return 0;
+}
+
+static const struct regmap_config rt5575_regmap = {
+     .reg_bits = 16,
+     .val_bits = 32,
+     .reg_stride = 4,
+     .max_register = 0xfffc,
Quoted 8 lines

sound/soc/codecs/rt5575.c line 304

Quoted 6 lines
sound/soc/codecs/rt5575.c new file +6 -0
+     .readable_reg = rt5575_readable_register,
+     .reg_read = rt5575_i2c_read,
+     .reg_write = rt5575_i2c_write,
+     .use_single_read = true,
+     .use_single_write = true,
+};

OF device ID table goes around here - together with I2C.

I will correct it.

Quoted 8 lines
Quoted 8 lines
sound/soc/codecs/rt5575.c new file +8 -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)
Quoted 8 lines

sound/soc/codecs/rt5575.c line 318

Quoted 6 lines
sound/soc/codecs/rt5575.c new file +6 -0
+{
+     struct rt5575_priv *rt5575;
+     struct device *dev = &i2c->dev;
+     int ret, val;
+
+#if IS_ENABLED(CONFIG_SND_SOC_RT5575_SPI)

No ifdefs in driver code.

I am not understanding exactly.
If the machine is without SPI interface and the codec with flash, the CONFIG_SND_SOC_RT5575_SPI can be disabled.

Quoted 5 lines
Quoted 1 line

+ if (!rt5575_spi && of_device_is_compatible(dev->of_node,

rt5575_of_match[1].compatible))

No, use driver match data if ever, but this is just wrong. You said it depends on SPI flash, not SPI interface.

I will modify it to use the match data as following.

static const struct of_device_id rt5575_of_match[] = {
	{ .compatible = "realtek,rt5575", .data = (void *)RT5575_WITH_FLASH },
	{ .compatible = "realtek,rt5575-use-spi", .data = (void *)RT5575_WITHOUT_FLASH },
	{ }
};
Quoted 9 lines

sound/soc/codecs/rt5575.c line 325

Quoted 6 lines
sound/soc/codecs/rt5575.c new file +6 -0
+             return -EPROBE_DEFER;
+#endif
+
+     rt5575 = devm_kzalloc(dev, sizeof(struct rt5575_priv),
+                             GFP_KERNEL);
+     if (rt5575 == NULL)

This is not Linux coding style. Open existing drivers. It's everywhere written (!foo)

I will correct it.

Quoted 54 lines

sound/soc/codecs/rt5575.c

Quoted 7 lines
sound/soc/codecs/rt5575.c new file +7 -0
+             return -ENOMEM;
+
+     i2c_set_clientdata(i2c, rt5575);
+
+     rt5575->i2c = i2c;
+
+     rt5575->dsp_regmap = devm_regmap_init_i2c(i2c,

&rt5575_dsp_regmap);

sound/soc/codecs/rt5575.c

Quoted 7 lines
sound/soc/codecs/rt5575.c new file +7 -0
+     if (IS_ERR(rt5575->dsp_regmap)) {
+             ret = PTR_ERR(rt5575->dsp_regmap);
+             dev_err(dev, "Failed to allocate register map: %d\n", ret);
+             return ret;
+     }
+
+     rt5575->regmap = devm_regmap_init(dev, NULL, i2c,

&rt5575_regmap);

sound/soc/codecs/rt5575.c

Quoted 9 lines
sound/soc/codecs/rt5575.c new file +9 -0
+     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);

sound/soc/codecs/rt5575.c

Quoted 10 lines
sound/soc/codecs/rt5575.c new file +10 -0
+             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,

sound/soc/codecs/rt5575.c line 365

Quoted 7 lines
sound/soc/codecs/rt5575.c new file +7 -0
+             ARRAY_SIZE(rt5575_dai));
+}
+
+static struct i2c_driver rt5575_i2c_driver = {
+     .driver = {
+             .name = "rt5575",
+             .owner = THIS_MODULE,

Please drop it.

Please run standard kernel tools for static analysis, like coccinelle, smatch and sparse, and fix reported warnings. Also please check for warnings when building with W=1 for gcc and clang. Most of these commands (checks or W=1 build) can build specific targets, like some directory, to narrow the scope to only your code. The code here looks like it needs a fix. Feel free to get in touch if the warning is not clear.

Quoted 3 lines
Quoted 1 line

+ .of_match_table = of_match_ptr(rt5575_of_match),

You have warning here. Drop of_match_ptr.

I will remove it.

Thanks,
Oder

#4 lore raw
Quoted 77 lines · Oder Chiou
Quoted 50 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Monday, December 8, 2025 2:05 PM
To: Oder Chiou <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Flove(HsinFu) <[email protected]>; Shuming [范 書銘] <[email protected]>; Jack Yu <[email protected]>; Derek [方德 義] <[email protected]>
Subject: Re: [PATCH v8 1/2] ASoC: rt5575: Add the codec driver for the ALC5575

External mail : This email originated from outside the organization. Do not reply, click links, or open attachments unless you recognize the sender and know the content is safe.

Review

Quoted 2 lines · Oder Chiou
(quoted diff) +2 -0
+
+static int rt5575_i2c_read(void *context, unsigned int reg, unsigned int

*val)

sound/soc/codecs/rt5575.c

Quoted 5 lines
sound/soc/codecs/rt5575.c new file +5 -0
+{
+     struct i2c_client *client = context;
+     struct rt5575_priv *rt5575 = i2c_get_clientdata(client);
+
+     regmap_read(rt5575->dsp_regmap, reg | RT5575_DSP_MAPPING,

val);

sound/soc/codecs/rt5575.c

Quoted 10 lines
sound/soc/codecs/rt5575.c new file +10 -0
+
+     return 0;
+}
+
+static int rt5575_i2c_write(void *context, unsigned int reg, unsigned int val)
+{
+     struct i2c_client *client = context;
+     struct rt5575_priv *rt5575 = i2c_get_clientdata(client);
+
+     regmap_write(rt5575->dsp_regmap, reg | RT5575_DSP_MAPPING,

val);

Quoted 9 lines
sound/soc/codecs/rt5575.c new file +9 -0
+
+     return 0;
+}
+
+static const struct regmap_config rt5575_regmap = {
+     .reg_bits = 16,
+     .val_bits = 32,
+     .reg_stride = 4,
+     .max_register = 0xfffc,
Quoted 8 lines

sound/soc/codecs/rt5575.c line 304

Quoted 6 lines
sound/soc/codecs/rt5575.c new file +6 -0
+     .readable_reg = rt5575_readable_register,
+     .reg_read = rt5575_i2c_read,
+     .reg_write = rt5575_i2c_write,
+     .use_single_read = true,
+     .use_single_write = true,
+};

OF device ID table goes around here - together with I2C.

I will correct it.

Quoted 8 lines
Quoted 8 lines
sound/soc/codecs/rt5575.c new file +8 -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)
Quoted 8 lines

sound/soc/codecs/rt5575.c line 318

Quoted 6 lines
sound/soc/codecs/rt5575.c new file +6 -0
+{
+     struct rt5575_priv *rt5575;
+     struct device *dev = &i2c->dev;
+     int ret, val;
+
+#if IS_ENABLED(CONFIG_SND_SOC_RT5575_SPI)

No ifdefs in driver code.

Quoted 3 lines

I am not understanding exactly.
If the machine is without SPI interface and the codec with flash, the CONFIG_SND_SOC_RT5575_SPI can be disabled.

But you still should not use #ifdef. Coding style gives you alternative, please look at the doc.

Quoted 11 lines
Quoted 6 lines
Quoted 2 lines

+ if (!rt5575_spi && of_device_is_compatible(dev->of_node,

rt5575_of_match[1].compatible))

No, use driver match data if ever, but this is just wrong. You said it depends on SPI flash, not SPI interface.

I will modify it to use the match data as following.

static const struct of_device_id rt5575_of_match[] = {
	{ .compatible = "realtek,rt5575", .data = (void *)RT5575_WITH_FLASH },
	{ .compatible = "realtek,rt5575-use-spi", .data = (void *)RT5575_WITHOUT_FLASH },

What is still wrong is that why you defer probe if there is no flash. I really do not get it...

Best regards,
Krzysztof

author maintainer
#5 lore raw
Quoted 118 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Monday, December 8, 2025 4:02 PM
To: Oder Chiou <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Flove(HsinFu) <[email protected]>; Shuming [范 書銘] <[email protected]>; Jack Yu <[email protected]>; Derek [方德 義] <[email protected]>
Subject: Re: [PATCH v8 1/2] ASoC: rt5575: Add the codec driver for the ALC5575

External mail : This email originated from outside the organization. Do not reply, click links, or open attachments unless you recognize the sender and know the content is safe.

Quoted 10 lines · Oder Chiou
Quoted 10 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Monday, December 8, 2025 2:05 PM
To: Oder Chiou <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Flove(HsinFu) <[email protected]>; Shuming [范
書銘] <[email protected]>; Jack Yu <[email protected]>; Derek [方

德

Quoted 69 lines
Quoted 43 lines

義] <[email protected]>
Subject: Re: [PATCH v8 1/2] ASoC: rt5575: Add the codec driver for the
ALC5575

External mail : This email originated from outside the organization. Do not reply, click links, or open attachments unless you recognize the sender and know the content is safe.

Review

Quoted 3 lines · Oder Chiou
(quoted diff) +3 -0
+
+static int rt5575_i2c_read(void *context, unsigned int reg,
+unsigned int

*val)

sound/soc/codecs/rt5575.c

Quoted 5 lines
sound/soc/codecs/rt5575.c new file +5 -0
+{
+     struct i2c_client *client = context;
+     struct rt5575_priv *rt5575 = i2c_get_clientdata(client);
+
+     regmap_read(rt5575->dsp_regmap, reg | RT5575_DSP_MAPPING,

val);

sound/soc/codecs/rt5575.c

Quoted 10 lines
sound/soc/codecs/rt5575.c new file +10 -0
+
+     return 0;
+}
+
+static int rt5575_i2c_write(void *context, unsigned int reg,
+unsigned int val) {
+     struct i2c_client *client = context;
+     struct rt5575_priv *rt5575 = i2c_get_clientdata(client);
+
+     regmap_write(rt5575->dsp_regmap, reg | RT5575_DSP_MAPPING,

val);

Quoted 9 lines
sound/soc/codecs/rt5575.c new file +9 -0
+
+     return 0;
+}
+
+static const struct regmap_config rt5575_regmap = {
+     .reg_bits = 16,
+     .val_bits = 32,
+     .reg_stride = 4,
+     .max_register = 0xfffc,
Quoted 8 lines

sound/soc/codecs/rt5575.c line 304

Quoted 6 lines
sound/soc/codecs/rt5575.c new file +6 -0
+     .readable_reg = rt5575_readable_register,
+     .reg_read = rt5575_i2c_read,
+     .reg_write = rt5575_i2c_write,
+     .use_single_read = true,
+     .use_single_write = true,
+};

OF device ID table goes around here - together with I2C.

I will correct it.

Quoted 7 lines
Quoted 7 lines
sound/soc/codecs/rt5575.c new file +7 -0
+
+static const struct i2c_device_id rt5575_i2c_id[] = {
+     { "rt5575" },
+     { }
+};
+MODULE_DEVICE_TABLE(i2c, rt5575_i2c_id);
+
Quoted 8 lines

sound/soc/codecs/rt5575.c line 318

Quoted 6 lines
sound/soc/codecs/rt5575.c new file +6 -0
+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)

No ifdefs in driver code.

Quoted 3 lines

I am not understanding exactly.
If the machine is without SPI interface and the codec with flash, the CONFIG_SND_SOC_RT5575_SPI can be disabled.

But you still should not use #ifdef. Coding style gives you alternative, please look at the doc.

Quoted 10 lines
Quoted 6 lines
Quoted 2 lines

+ if (!rt5575_spi && of_device_is_compatible(dev->of_node,

rt5575_of_match[1].compatible))

No, use driver match data if ever, but this is just wrong. You said it depends on SPI flash, not SPI interface.

I will modify it to use the match data as following.

static const struct of_device_id rt5575_of_match[] = {
      { .compatible = "realtek,rt5575", .data = (void
Quoted 6 lines

*)RT5575_WITH_FLASH },

Quoted 2 lines
      { .compatible = "realtek,rt5575-use-spi", .data = (void

*)RT5575_WITHOUT_FLASH },

What is still wrong is that why you defer probe if there is no flash. I really do not get it...

If the codec has flash, the flash is connected to the codec's SPI host interface.

If the codec has no flash, the codec SPI driver should load the firmware from the codec's SPI slave interface. The I2C driver must wait until the SPI driver is ready to ensure the firmware is loaded correctly.

#6 lore raw
Quoted 125 lines · Oder Chiou
Quoted 118 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Monday, December 8, 2025 4:02 PM
To: Oder Chiou <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Flove(HsinFu) <[email protected]>; Shuming [范 書銘] <[email protected]>; Jack Yu <[email protected]>; Derek [方德 義] <[email protected]>
Subject: Re: [PATCH v8 1/2] ASoC: rt5575: Add the codec driver for the ALC5575

External mail : This email originated from outside the organization. Do not reply, click links, or open attachments unless you recognize the sender and know the content is safe.

Quoted 10 lines · Oder Chiou
Quoted 10 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Monday, December 8, 2025 2:05 PM
To: Oder Chiou <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Flove(HsinFu) <[email protected]>; Shuming [范
書銘] <[email protected]>; Jack Yu <[email protected]>; Derek [方

德

Quoted 69 lines
Quoted 43 lines

義] <[email protected]>
Subject: Re: [PATCH v8 1/2] ASoC: rt5575: Add the codec driver for the
ALC5575

External mail : This email originated from outside the organization. Do not reply, click links, or open attachments unless you recognize the sender and know the content is safe.

Review

Quoted 3 lines · Oder Chiou
(quoted diff) +3 -0
+
+static int rt5575_i2c_read(void *context, unsigned int reg,
+unsigned int

*val)

sound/soc/codecs/rt5575.c

Quoted 5 lines
sound/soc/codecs/rt5575.c new file +5 -0
+{
+     struct i2c_client *client = context;
+     struct rt5575_priv *rt5575 = i2c_get_clientdata(client);
+
+     regmap_read(rt5575->dsp_regmap, reg | RT5575_DSP_MAPPING,

val);

sound/soc/codecs/rt5575.c

Quoted 10 lines
sound/soc/codecs/rt5575.c new file +10 -0
+
+     return 0;
+}
+
+static int rt5575_i2c_write(void *context, unsigned int reg,
+unsigned int val) {
+     struct i2c_client *client = context;
+     struct rt5575_priv *rt5575 = i2c_get_clientdata(client);
+
+     regmap_write(rt5575->dsp_regmap, reg | RT5575_DSP_MAPPING,

val);

Quoted 9 lines
sound/soc/codecs/rt5575.c new file +9 -0
+
+     return 0;
+}
+
+static const struct regmap_config rt5575_regmap = {
+     .reg_bits = 16,
+     .val_bits = 32,
+     .reg_stride = 4,
+     .max_register = 0xfffc,
Quoted 8 lines

sound/soc/codecs/rt5575.c line 304

Quoted 6 lines
sound/soc/codecs/rt5575.c new file +6 -0
+     .readable_reg = rt5575_readable_register,
+     .reg_read = rt5575_i2c_read,
+     .reg_write = rt5575_i2c_write,
+     .use_single_read = true,
+     .use_single_write = true,
+};

OF device ID table goes around here - together with I2C.

I will correct it.

Quoted 7 lines
Quoted 7 lines
sound/soc/codecs/rt5575.c new file +7 -0
+
+static const struct i2c_device_id rt5575_i2c_id[] = {
+     { "rt5575" },
+     { }
+};
+MODULE_DEVICE_TABLE(i2c, rt5575_i2c_id);
+
Quoted 8 lines

sound/soc/codecs/rt5575.c line 318

Quoted 6 lines
sound/soc/codecs/rt5575.c new file +6 -0
+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)

No ifdefs in driver code.

Quoted 3 lines

I am not understanding exactly.
If the machine is without SPI interface and the codec with flash, the CONFIG_SND_SOC_RT5575_SPI can be disabled.

But you still should not use #ifdef. Coding style gives you alternative, please look at the doc.

Quoted 10 lines
Quoted 6 lines
Quoted 2 lines

+ if (!rt5575_spi && of_device_is_compatible(dev->of_node,

rt5575_of_match[1].compatible))

No, use driver match data if ever, but this is just wrong. You said it depends on SPI flash, not SPI interface.

I will modify it to use the match data as following.

static const struct of_device_id rt5575_of_match[] = {
      { .compatible = "realtek,rt5575", .data = (void
Quoted 6 lines

*)RT5575_WITH_FLASH },

Quoted 2 lines
      { .compatible = "realtek,rt5575-use-spi", .data = (void

*)RT5575_WITHOUT_FLASH },

What is still wrong is that why you defer probe if there is no flash. I really do not get it...

Quoted 7 lines

If the codec has flash, the flash is connected to the codec's SPI host interface.

If the codec has no flash, the codec SPI driver should load the firmware from the codec's SPI slave interface. The I2C driver must wait until the SPI driver is ready to ensure the firmware is loaded correctly.

Ah, so for that reason you created that singleton, exported it and you wait for it? Singletons are pretty no-go anyway, how do you handle to codecs in the system? Fragile design.

Best regards,
Krzysztof

author maintainer
#7 lore raw
Quoted 155 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Tuesday, December 9, 2025 2:08 PM
To: Oder Chiou <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Flove(HsinFu) <[email protected]>; Shuming [范 書銘] <[email protected]>; Jack Yu <[email protected]>; Derek [方德 義] <[email protected]>
Subject: Re: [PATCH v8 1/2] ASoC: rt5575: Add the codec driver for the ALC5575

External mail : This email originated from outside the organization. Do not reply, click links, or open attachments unless you recognize the sender and know the content is safe.

Quoted 7 lines · Oder Chiou
Quoted 7 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Monday, December 8, 2025 4:02 PM
To: Oder Chiou <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]; [email protected];

[email protected];

Quoted 1 line
Quoted 1 line

[email protected]; Flove(HsinFu) <[email protected]>; Shuming

[范

Quoted 1 line
Quoted 1 line

書銘] <[email protected]>; Jack Yu <[email protected]>; Derek [方

德

Quoted 12 lines
Quoted 12 lines

義] <[email protected]>
Subject: Re: [PATCH v8 1/2] ASoC: rt5575: Add the codec driver for the ALC5575

External mail : This email originated from outside the organization. Do not reply, click links, or open attachments unless you recognize the sender and know the content is safe.

On 08/12/2025 08:29, Oder Chiou wrote:

Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Monday, December 8, 2025 2:05 PM
To: Oder Chiou <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Flove(HsinFu) <[email protected]>; Shuming [范
書銘] <[email protected]>; Jack Yu <[email protected]>; Derek

[方

Quoted 18 lines
Quoted 18 lines

德

Quoted 17 lines
Quoted 17 lines

義] <[email protected]>
Subject: Re: [PATCH v8 1/2] ASoC: rt5575: Add the codec driver for the
ALC5575

External mail : This email originated from outside the organization. Do not reply, click links, or open attachments unless you recognize the sender and know the content is safe.

Review

Quoted 3 lines · Oder Chiou
(quoted diff) +3 -0
+
+static int rt5575_i2c_read(void *context, unsigned int reg,
+unsigned int

*val)

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines

sound/soc/codecs/rt5575.c

Quoted 5 lines
sound/soc/codecs/rt5575.c new file +5 -0
+{
+     struct i2c_client *client = context;
+     struct rt5575_priv *rt5575 = i2c_get_clientdata(client);
+
+     regmap_read(rt5575->dsp_regmap, reg |

RT5575_DSP_MAPPING,

Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines

val);

sound/soc/codecs/rt5575.c

Quoted 10 lines
sound/soc/codecs/rt5575.c new file +10 -0
+
+     return 0;
+}
+
+static int rt5575_i2c_write(void *context, unsigned int reg,
+unsigned int val) {
+     struct i2c_client *client = context;
+     struct rt5575_priv *rt5575 = i2c_get_clientdata(client);
+
+     regmap_write(rt5575->dsp_regmap, reg |

RT5575_DSP_MAPPING,

Quoted 63 lines
Quoted 53 lines
Quoted 36 lines
Quoted 10 lines

val);

Quoted 9 lines
sound/soc/codecs/rt5575.c new file +9 -0
+
+     return 0;
+}
+
+static const struct regmap_config rt5575_regmap = {
+     .reg_bits = 16,
+     .val_bits = 32,
+     .reg_stride = 4,
+     .max_register = 0xfffc,
Quoted 8 lines

sound/soc/codecs/rt5575.c line 304

Quoted 6 lines
sound/soc/codecs/rt5575.c new file +6 -0
+     .readable_reg = rt5575_readable_register,
+     .reg_read = rt5575_i2c_read,
+     .reg_write = rt5575_i2c_write,
+     .use_single_read = true,
+     .use_single_write = true,
+};

OF device ID table goes around here - together with I2C.

I will correct it.

Quoted 7 lines
Quoted 7 lines
sound/soc/codecs/rt5575.c new file +7 -0
+
+static const struct i2c_device_id rt5575_i2c_id[] = {
+     { "rt5575" },
+     { }
+};
+MODULE_DEVICE_TABLE(i2c, rt5575_i2c_id);
+
Quoted 8 lines

sound/soc/codecs/rt5575.c line 318

Quoted 6 lines
sound/soc/codecs/rt5575.c new file +6 -0
+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)

No ifdefs in driver code.

Quoted 3 lines

I am not understanding exactly.
If the machine is without SPI interface and the codec with flash, the CONFIG_SND_SOC_RT5575_SPI can be disabled.

But you still should not use #ifdef. Coding style gives you alternative, please look at the doc.

Quoted 10 lines
Quoted 6 lines
Quoted 2 lines

+ if (!rt5575_spi && of_device_is_compatible(dev->of_node,

rt5575_of_match[1].compatible))

No, use driver match data if ever, but this is just wrong. You said it depends on SPI flash, not SPI interface.

I will modify it to use the match data as following.

static const struct of_device_id rt5575_of_match[] = {
      { .compatible = "realtek,rt5575", .data = (void
Quoted 6 lines

*)RT5575_WITH_FLASH },

Quoted 2 lines
      { .compatible = "realtek,rt5575-use-spi", .data = (void

*)RT5575_WITHOUT_FLASH },

What is still wrong is that why you defer probe if there is no flash. I really do not get it...

If the codec has flash, the flash is connected to the codec's SPI host interface.

Quoted 8 lines
Quoted 4 lines

If the codec has no flash, the codec SPI driver should load the firmware from the codec's SPI slave interface. The I2C driver must wait until the SPI driver is ready to ensure the firmware is loaded correctly.

Ah, so for that reason you created that singleton, exported it and you wait for it? Singletons are pretty no-go anyway, how do you handle to codecs in the system? Fragile design.

The code will be implemented in the top of the i2c_probe() as following.

if (dev_type == RT5575_WITHOUT_FLASH) {
	if (IS_ENABLED(CONFIG_SND_SOC_RT5575_SPI)) {
		if (!rt5575_spi) {
			dev_err(dev, "Wait SPI driver ready\n");
			return -EPROBE_DEFER;
		}
	} else {
		dev_err(dev, "This dev type should enable CONFIG_SND_SOC_RT5575_SPI\n");
		return -ENODEV;
	}
}

Thanks,
Oder

#8 lore raw
Quoted 12 lines · Oder Chiou
Quoted 12 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Tuesday, December 9, 2025 2:08 PM
To: Oder Chiou <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Flove(HsinFu) <[email protected]>; Shuming [范 書銘] <[email protected]>; Jack Yu <[email protected]>; Derek [方德 義] <[email protected]>
Subject: Re: [PATCH v8 1/2] ASoC: rt5575: Add the codec driver for the ALC5575

...

Quoted 6 lines
Quoted 3 lines

Ah, so for that reason you created that singleton, exported it and you wait for it? Singletons are pretty no-go anyway, how do you handle to codecs in the system? Fragile design.

The code will be implemented in the top of the i2c_probe() as following.

Quoted 11 lines
if (dev_type == RT5575_WITHOUT_FLASH) {
	if (IS_ENABLED(CONFIG_SND_SOC_RT5575_SPI)) {
		if (!rt5575_spi) {
			dev_err(dev, "Wait SPI driver ready\n");
			return -EPROBE_DEFER;
		}
	} else {
		dev_err(dev, "This dev type should enable CONFIG_SND_SOC_RT5575_SPI\n");
		return -ENODEV;
	}
}

The idea is to get rid of 'rt5575_spi' global. Removing 'rt5575_spi_ready' was just one of the steps to improve the flow. Removal of 'rt5575_spi' is the next one.

#9 lore raw
In reply to Oder Chiou
Quoted 8 lines · Oder Chiou

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.

...

Quoted 49 lines
sound/soc/codecs/rt5575-spi.c new file +43 -0
new file mode 100644
index 000000000000..12c2379a061e
@@ -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;

sound/soc/codecs/rt5575-spi.c line 53

Quoted 10 lines
sound/soc/codecs/rt5575-spi.c new file +10 -0
+		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.

Quoted 43 lines
sound/soc/codecs/rt5575-spi.c new file +31 -0
+	}
+
+	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 <[email protected]>");
+MODULE_LICENSE("GPL");
sound/soc/codecs/rt5575-spi.h new file +6 -0
new file mode 100644
index 000000000000..b364b49bb43e
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * rt5575-spi.h  --  ALC5575 SPI driver
+ *
+ * Copyright(c) 2025 Realtek Semiconductor Corp.
+ *

sound/soc/codecs/rt5575-spi.h line 16

Quoted 10 lines
sound/soc/codecs/rt5575-spi.h new file +10 -0
+ */
+
+#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().

Quoted 50 lines
sound/soc/codecs/rt5575.c new file +44 -0
new file mode 100644
index 000000000000..c7e8f5a606bc
@@ -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:

sound/soc/codecs/rt5575.c line 54

Quoted 10 lines
sound/soc/codecs/rt5575.c new file +10 -0
+		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().

sound/soc/codecs/rt5575.c line 57

Quoted 3 lines
sound/soc/codecs/rt5575.c new file +3 -0
+{
+	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.

Quoted 100 lines
sound/soc/codecs/rt5575.c new file +100 -0
+	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)
+{

sound/soc/codecs/rt5575.c line 167

Quoted 10 lines
sound/soc/codecs/rt5575.c new file +10 -0
+	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()!

Quoted 6 lines
sound/soc/codecs/rt5575.c new file +6 -0
+		"realtek/rt5575/rt5575_fw1.bin", component->dev, GFP_KERNEL, rt5575,
+		rt5575_fw_load_by_spi);
+#endif
+
+	return 0;
+}

...

Quoted 21 lines
sound/soc/codecs/rt5575.c new file +21 -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)

sound/soc/codecs/rt5575.c line 335

Quoted 10 lines
sound/soc/codecs/rt5575.c new file +10 -0
+		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?

Quoted 14 lines
sound/soc/codecs/rt5575.c new file +14 -0
+		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;

sound/soc/codecs/rt5575.c line 359

Quoted 10 lines
sound/soc/codecs/rt5575.c new file +10 -0
+	}
+
+	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.

Quoted 16 lines
sound/soc/codecs/rt5575.c new file +16 -0
+}
+
+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 <[email protected]>");
+MODULE_LICENSE("GPL");
author maintainer
#10 lore raw
Quoted 221 lines

-----Original Message-----
From: Cezary Rojewski <[email protected]>
Sent: Tuesday, December 9, 2025 4:06 AM
To: Oder Chiou <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Flove(HsinFu) <[email protected]>; Shuming [范 書銘] <[email protected]>; Jack Yu <[email protected]>; Derek [方德 義] <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v8 1/2] ASoC: rt5575: Add the codec driver for the ALC5575

External mail : This email originated from outside the organization. Do not reply, click links, or open attachments unless you recognize the sender and know the content is safe.

Quoted 8 lines · Oder Chiou

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.

...

Quoted 49 lines
sound/soc/codecs/rt5575-spi.c new file +43 -0
new file mode 100644
index 000000000000..12c2379a061e
@@ -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;

sound/soc/codecs/rt5575-spi.c line 53

Quoted 10 lines
sound/soc/codecs/rt5575-spi.c new file +10 -0
+             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.

Quoted 43 lines
sound/soc/codecs/rt5575-spi.c new file +31 -0
+     }
+
+     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 <[email protected]>");
+MODULE_LICENSE("GPL");
sound/soc/codecs/rt5575-spi.h new file +6 -0
new file mode 100644
index 000000000000..b364b49bb43e
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * rt5575-spi.h  --  ALC5575 SPI driver
+ *
+ * Copyright(c) 2025 Realtek Semiconductor Corp.
+ *

sound/soc/codecs/rt5575-spi.h line 16

Quoted 10 lines
sound/soc/codecs/rt5575-spi.h new file +10 -0
+ */
+
+#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().

Quoted 57 lines
sound/soc/codecs/rt5575.c new file +51 -0
new file mode 100644
index 000000000000..c7e8f5a606bc
@@ -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);
Quoted 8 lines

sound/soc/codecs/rt5575.c line 54

Quoted 3 lines
sound/soc/codecs/rt5575.c new file +3 -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?

There are few I2C r/w in rt5575_fw_load_by_spi().

But the SPI load firmware function can move to rt5575-spi.c. The rt5575_spi_fw_load() will do all firmware load related works, and it will return request_firmware_xxx() result.
The rt5575_fw_load_by_spi() will be implement as following, and it will put into the bottom of rt5575_i2c_probe().

rt5575_fw_load_by_spi()

{
	...

	regmap_write(rt5575->dsp_regmap, 0xfafafafa, 0x00000004);
	regmap_write(rt5575->dsp_regmap, 0x18008064, 0x00000000);
	regmap_write(rt5575->dsp_regmap, 0x18008068, 0x0002ffff);

	ret = rt5575_spi_fw_load();
	if (ret) {
		dev_err(dev, "Load firmware failure: %d\n", ret);
		return ENODEV;
	}

	regmap_write(rt5575->dsp_regmap, 0x18000000, 0x00000000);
	regmap_update_bits(rt5575->regmap, RT5575_SW_INT, 1, 1);

	regmap_read_poll_timeout();
	if (ret) {
		dev_err(dev, "Run firmware failure: %d\n", ret);
		return ENODEV;
	}

	...
}
Quoted 185 lines

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().

sound/soc/codecs/rt5575.c line 57

Quoted 3 lines
sound/soc/codecs/rt5575.c new file +3 -0
+{
+     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.

Quoted 10 lines
sound/soc/codecs/rt5575.c new file +10 -0
+     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);

sound/soc/codecs/rt5575.c

Quoted 10 lines
sound/soc/codecs/rt5575.c new file +10 -0
+     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);

sound/soc/codecs/rt5575.c

Quoted 3 lines
sound/soc/codecs/rt5575.c new file +3 -0
+                     release_firmware(firmware);
+             } else {
+                     dev_err(&i2c->dev, "Request firmware failure:

%d\n", ret);

sound/soc/codecs/rt5575.c

Quoted 9 lines
sound/soc/codecs/rt5575.c new file +9 -0
+                     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);

sound/soc/codecs/rt5575.c

Quoted 8 lines
sound/soc/codecs/rt5575.c new file +8 -0
+
+     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),

sound/soc/codecs/rt5575.c line 98

Quoted 1 line
sound/soc/codecs/rt5575.c new file +1 -0
+     SOC_DOUBLE_TLV("Speaker CH-01 Playback Volume",

RT5575_SPK01_VOL, 17, 1, 167, 0, ob_tlv),

sound/soc/codecs/rt5575.c line 99

Quoted 1 line
sound/soc/codecs/rt5575.c new file +1 -0
+     SOC_DOUBLE("Speaker CH-23 Playback Switch", RT5575_SPK23_VOL,

31, 15, 1, 1),

sound/soc/codecs/rt5575.c line 100

Quoted 1 line
sound/soc/codecs/rt5575.c new file +1 -0
+     SOC_DOUBLE_TLV("Speaker CH-23 Playback Volume",

RT5575_SPK23_VOL, 17, 1, 167, 0, ob_tlv),

sound/soc/codecs/rt5575.c line 101

Quoted 1 line
sound/soc/codecs/rt5575.c new file +1 -0
+     SOC_DOUBLE("Mic1 Capture Switch", RT5575_MIC1_VOL, 31, 15, 1,

1),

sound/soc/codecs/rt5575.c line 102

Quoted 1 line
sound/soc/codecs/rt5575.c new file +1 -0
+     SOC_DOUBLE_TLV("Mic1 Capture Volume", RT5575_MIC1_VOL, 17,

1, 167, 0, ob_tlv),

sound/soc/codecs/rt5575.c line 103

Quoted 1 line
sound/soc/codecs/rt5575.c new file +1 -0
+     SOC_DOUBLE("Mic2 Capture Switch", RT5575_MIC2_VOL, 31, 15, 1,

1),

sound/soc/codecs/rt5575.c line 104

Quoted 1 line
sound/soc/codecs/rt5575.c new file +1 -0
+     SOC_DOUBLE_TLV("Mic2 Capture Volume", RT5575_MIC2_VOL, 17,

1, 167, 0, ob_tlv),

sound/soc/codecs/rt5575.c line 105

Quoted 1 line
sound/soc/codecs/rt5575.c new file +1 -0
+     SOC_DOUBLE_R("Mix Playback Switch", RT5575_MIXL_VOL,

RT5575_MIXR_VOL, 31, 1, 1),

sound/soc/codecs/rt5575.c line 106

Quoted 1 line
sound/soc/codecs/rt5575.c new file +1 -0
+     SOC_DOUBLE_R_TLV("Mix Playback Volume", RT5575_MIXL_VOL,

RT5575_MIXR_VOL, 1, 127, 0,

sound/soc/codecs/rt5575.c line 108

Quoted 2 lines
sound/soc/codecs/rt5575.c new file +2 -0
+             ob_tlv),
+     SOC_DOUBLE("Prompt Playback Switch", RT5575_PROMPT_VOL, 31,

15, 1, 1),

sound/soc/codecs/rt5575.c line 109

Quoted 1 line
sound/soc/codecs/rt5575.c new file +1 -0
+     SOC_DOUBLE_TLV("Prompt Playback Volume",

RT5575_PROMPT_VOL, 17, 1, 167, 0, ob_tlv),

sound/soc/codecs/rt5575.c

Quoted 4 lines
sound/soc/codecs/rt5575.c new file +4 -0
+};
+
+static const struct snd_soc_dapm_widget rt5575_dapm_widgets[] = {
+     SND_SOC_DAPM_AIF_IN("AIF1RX", "AIF1 Playback", 0,

SND_SOC_NOPM, 0, 0),

sound/soc/codecs/rt5575.c line 114

Quoted 1 line
sound/soc/codecs/rt5575.c new file +1 -0
+     SND_SOC_DAPM_AIF_OUT("AIF1TX", "AIF1 Capture", 0,

SND_SOC_NOPM, 0, 0),

sound/soc/codecs/rt5575.c line 115

Quoted 1 line
sound/soc/codecs/rt5575.c new file +1 -0
+     SND_SOC_DAPM_AIF_IN("AIF2RX", "AIF2 Playback", 0,

SND_SOC_NOPM, 0, 0),

sound/soc/codecs/rt5575.c line 116

Quoted 1 line
sound/soc/codecs/rt5575.c new file +1 -0
+     SND_SOC_DAPM_AIF_OUT("AIF2TX", "AIF2 Capture", 0,

SND_SOC_NOPM, 0, 0),

sound/soc/codecs/rt5575.c line 117

Quoted 1 line
sound/soc/codecs/rt5575.c new file +1 -0
+     SND_SOC_DAPM_AIF_IN("AIF3RX", "AIF3 Playback", 0,

SND_SOC_NOPM, 0, 0),

sound/soc/codecs/rt5575.c line 118

Quoted 1 line
sound/soc/codecs/rt5575.c new file +1 -0
+     SND_SOC_DAPM_AIF_OUT("AIF3TX", "AIF3 Capture", 0,

SND_SOC_NOPM, 0, 0),

sound/soc/codecs/rt5575.c line 119

Quoted 1 line
sound/soc/codecs/rt5575.c new file +1 -0
+     SND_SOC_DAPM_AIF_IN("AIF4RX", "AIF4 Playback", 0,

SND_SOC_NOPM, 0, 0),

sound/soc/codecs/rt5575.c line 120

Quoted 1 line
sound/soc/codecs/rt5575.c new file +1 -0
+     SND_SOC_DAPM_AIF_OUT("AIF4TX", "AIF4 Capture", 0,

SND_SOC_NOPM, 0, 0),

Quoted 12 lines
sound/soc/codecs/rt5575.c new file +12 -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" },

sound/soc/codecs/rt5575.c

Quoted 10 lines
sound/soc/codecs/rt5575.c new file +10 -0
+     { "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);

sound/soc/codecs/rt5575.c line 143

Quoted 1 line
sound/soc/codecs/rt5575.c new file +1 -0
+     regmap_read(rt5575->regmap, RT5575_EFUSE_DATA_3,

&priv_id_high);

Quoted 5 lines
sound/soc/codecs/rt5575.c new file +5 -0
+     regmap_write(rt5575->regmap, RT5575_EFUSE_PID, 0);
+
+     return ((long long)priv_id_high << 32) | (long long)priv_id_low;
+}
+

sound/soc/codecs/rt5575.c

Quoted 10 lines
sound/soc/codecs/rt5575.c new file +10 -0
+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);

sound/soc/codecs/rt5575.c

Quoted 8 lines
sound/soc/codecs/rt5575.c new file +8 -0
+     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))

sound/soc/codecs/rt5575.c line 167

Quoted 1 line
sound/soc/codecs/rt5575.c new file +1 -0
+             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.

Quoted 2 lines

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

But before the firmware-loading, I2C commands are needed. The SPI firmware-loading cannot be independent by I2C.

Thanks,
Oder

#11 lore raw
Quoted 11 lines · Oder Chiou
Quoted 11 lines

-----Original Message-----
From: Cezary Rojewski <[email protected]>
Sent: Tuesday, December 9, 2025 4:06 AM
To: Oder Chiou <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; Flove(HsinFu) <[email protected]>; Shuming [范 書銘] <[email protected]>; Jack Yu <[email protected]>; Derek [方德 義] <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v8 1/2] ASoC: rt5575: Add the codec driver for the ALC5575

...

Quoted 9 lines
Quoted 7 lines

Review

Quoted 2 lines
(quoted diff) +2 -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?

There are few I2C r/w in rt5575_fw_load_by_spi().

It's all abstracted away by regmap.

Quoted 6 lines

But the SPI load firmware function can move to rt5575-spi.c. The rt5575_spi_fw_load() will do all firmware load related works, and it will return request_firmware_xxx() result.
The rt5575_fw_load_by_spi() will be implement as following, and it will put into the bottom of rt5575_i2c_probe().

Did you try my suggestions, compiled them and tested before following up? The above looks like an evasion-tactic.

Quoted 97 lines

rt5575_fw_load_by_spi()

{
	...

	regmap_write(rt5575->dsp_regmap, 0xfafafafa, 0x00000004);
	regmap_write(rt5575->dsp_regmap, 0x18008064, 0x00000000);
	regmap_write(rt5575->dsp_regmap, 0x18008068, 0x0002ffff);

	ret = rt5575_spi_fw_load();
	if (ret) {
		dev_err(dev, "Load firmware failure: %d\n", ret);
		return ENODEV;
	}

	regmap_write(rt5575->dsp_regmap, 0x18000000, 0x00000000);
	regmap_update_bits(rt5575->regmap, RT5575_SW_INT, 1, 1);

	regmap_read_poll_timeout();
	if (ret) {
		dev_err(dev, "Run firmware failure: %d\n", ret);
		return ENODEV;
	}

	...
}
Quoted 70 lines

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().

sound/soc/codecs/rt5575.c line 57

Quoted 3 lines
sound/soc/codecs/rt5575.c new file +3 -0
+{
+     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.

Quoted 10 lines
sound/soc/codecs/rt5575.c new file +10 -0
+     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);

sound/soc/codecs/rt5575.c

Quoted 10 lines
sound/soc/codecs/rt5575.c new file +10 -0
+     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);

sound/soc/codecs/rt5575.c

Quoted 3 lines
sound/soc/codecs/rt5575.c new file +3 -0
+                     release_firmware(firmware);
+             } else {
+                     dev_err(&i2c->dev, "Request firmware failure:

%d\n", ret);

sound/soc/codecs/rt5575.c

Quoted 9 lines
sound/soc/codecs/rt5575.c new file +9 -0
+                     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);

Quoted 5 lines
sound/soc/codecs/rt5575.c new file +5 -0
+
+     if (ret)
+             dev_err(&i2c->dev, "Run firmware failure: %d\n", ret);
+}
+#endif

...

Quoted 32 lines
Quoted 32 lines

sound/soc/codecs/rt5575.c

Quoted 3 lines
sound/soc/codecs/rt5575.c new file +3 -0
+static int rt5575_probe(struct snd_soc_component *component)
+{
+     struct rt5575_priv *rt5575 =

snd_soc_component_get_drvdata(component);

sound/soc/codecs/rt5575.c

Quoted 8 lines
sound/soc/codecs/rt5575.c new file +8 -0
+     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))

Quoted 1 line

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

Quoted 5 lines
Quoted 2 lines

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

But before the firmware-loading, I2C commands are needed. The SPI firmware-loading cannot be independent by I2C.

From the design perspective, we're dealing with up to components and somewhat of parent (I2C) <- child (SPI) relation.

Code should make sure a child is never probed before the parent is operational.

1) boot I2C, init regmaps
2) boot SPI, load firmware
3) register ASoC component if 1) and 2) succeed

author maintainer
#12 lore raw

PATCH v8 2/2 ASoC: dt-bindings: realtek,rt5575: add bindings for ALC5575

In reply to Oder Chiou

Realtek ALC5575 is a highly advanced DSP and microphone CODEC that has been designed for AI audio technology. Its impressive features include an advanced HiFi-5 DSP core, a Neural Network Processing Unit (NPU) owned by Realtek, and embedded 4MB memory, which enables it to operate highly advanced AI audio algorithms. The ALC5575 supports 4xA-mic input and 8xD-mic input, as well as a rich set of interfaces such as I2S, I2C, SPI, and UART.

---
 .../bindings/sound/realtek,rt5575.yaml        | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/realtek,rt5575.yaml
Documentation/devicetree/bindings/sound/realtek,rt5575.yaml new file +44 -0
new file mode 100644
index 000000000000..83ccc79e6769
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/realtek,rt5575.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ALC5575 audio CODEC
+
+maintainers:
+  - Oder Chiou <[email protected]>
+
+description:
+  The device supports both I2C and SPI. I2C is mandatory, while SPI is
+  optional depending on the hardware configuration.
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+  - $ref: dai-common.yaml#
+
+properties:
+  compatible:
+    enum:
+      - realtek,rt5575
+      - realtek,rt5575-with-spi
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        codec@57 {
+            compatible = "realtek,rt5575";
+            reg = <0x57>;
+        };
+    };
Signature · 1 line
2.52.0
#13 lore raw

Re: [PATCH v8 2/2] ASoC: dt-bindings: realtek,rt5575: add bindings for ALC5575

Quoted 7 lines · Oder Chiou

Realtek ALC5575 is a highly advanced DSP and microphone CODEC that has been designed for AI audio technology. Its impressive features include an advanced HiFi-5 DSP core, a Neural Network Processing Unit (NPU) owned by Realtek, and embedded 4MB memory, which enables it to operate highly advanced AI audio algorithms. The ALC5575 supports 4xA-mic input and 8xD-mic input, as well as a rich set of interfaces such as I2S, I2C, SPI, and UART.

When I asked to describe hardware, I did not meant marketing junk! Drop all impressive features and simply describe hardware in basic terms. This is not advanced DSP, not designed for AI audio (AI is the easiest way to get a grumpy review), not "highly advanced AI audio algorithms" and does not have "as a rich set of interfaces".

Use simple terms what is this. Audio codec with I2S.... interfaces.

Quoted 27 lines
---
 .../bindings/sound/realtek,rt5575.yaml        | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/realtek,rt5575.yaml
Documentation/devicetree/bindings/sound/realtek,rt5575.yaml new file +14 -0
new file mode 100644
index 000000000000..83ccc79e6769
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/realtek,rt5575.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ALC5575 audio CODEC
+
+maintainers:
+  - Oder Chiou <[email protected]>
+
+description:
+  The device supports both I2C and SPI. I2C is mandatory, while SPI is
+  optional depending on the hardware configuration.

Documentation/devicetree/bindings/sound/realtek,rt5575.yaml line 24

Quoted 10 lines
Documentation/devicetree/bindings/sound/realtek,rt5575.yaml new file +10 -0
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+  - $ref: dai-common.yaml#
+
+properties:
+  compatible:
+    enum:
+      - realtek,rt5575
+      - realtek,rt5575-with-spi

Drop the second compatible. It's the same device. Whether it supports SPI it is already known and obvious - you cannot place non-SPI chip on SPI bus and expect it to work.

Documentation/devicetree/bindings/sound/realtek,rt5575.yaml line 28

Quoted 4 lines
Documentation/devicetree/bindings/sound/realtek,rt5575.yaml new file +4 -0
+
+  reg:
+    maxItems: 1
+

You listed so many "impressive" and "rich" features that for sure this is incomplete.

Please post complete bindings for "impressive" device.

You miss supplies, you miss all the AI related hype (no clue what that would be, but for sure NPU feels like needing remoteproc or at least some other way to communicate). We all know that AI is power hungry, so it is impossible to run it without electricity (thus supplies).

Best regards,
Krzysztof

#14 lore raw

Re: [PATCH v8 2/2] ASoC: dt-bindings: realtek,rt5575: add bindings for ALC5575

Quoted 9 lines · Krzysztof Kozlowski

Documentation/devicetree/bindings/sound/realtek,rt5575.yaml line 28

Quoted 5 lines
Documentation/devicetree/bindings/sound/realtek,rt5575.yaml new file +4 -0
+
+  reg:
+    maxItems: 1
+

You listed so many "impressive" and "rich" features that for sure this is incomplete.

Quoted 7 lines

Please post complete bindings for "impressive" device.

You miss supplies, you miss all the AI related hype (no clue what that would be, but for sure NPU feels like needing remoteproc or at least some other way to communicate). We all know that AI is power hungry, so it is impossible to run it without electricity (thus supplies).

Also two standard nits, in case I did not ask about them before:

A nit, subject: drop second/last, redundant "bindings". The "dt-bindings" prefix is already stating that these are bindings. See also:
https://elixir.bootlin.com/linux/v6.17-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

Please organize the patch documenting the compatible (DT bindings) before the patch using that compatible.
See also:
https://elixir.bootlin.com/linux/v6.14-rc6/source/Documentation/devicetree/bindings/submitting-patches.rst#L46

Best regards,
Krzysztof

author maintainer
#15 lore raw

RE: [PATCH v8 2/2] ASoC: dt-bindings: realtek,rt5575: add bindings for ALC5575

Quoted 38 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Friday, December 5, 2025 4:32 PM
To: Oder Chiou <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Flove(HsinFu) <[email protected]>; Shuming [范 書銘] <[email protected]>; Jack Yu <[email protected]>; Derek [方德 義] <[email protected]>
Subject: Re: [PATCH v8 2/2] ASoC: dt-bindings: realtek,rt5575: add bindings for ALC5575

External mail : This email originated from outside the organization. Do not reply, click links, or open attachments unless you recognize the sender and know the content is safe.

Quoted 9 lines · Krzysztof Kozlowski

Documentation/devicetree/bindings/sound/realtek,rt5575.yaml line 28

Quoted 5 lines
Documentation/devicetree/bindings/sound/realtek,rt5575.yaml new file +4 -0
+
+  reg:
+    maxItems: 1
+

You listed so many "impressive" and "rich" features that for sure this is incomplete.

Quoted 7 lines

Please post complete bindings for "impressive" device.

You miss supplies, you miss all the AI related hype (no clue what that would be, but for sure NPU feels like needing remoteproc or at least some other way to communicate). We all know that AI is power hungry, so it is impossible to run it without electricity (thus supplies).

Quoted 7 lines

Also two standard nits, in case I did not ask about them before:

A nit, subject: drop second/last, redundant "bindings". The "dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.17-rc3/source/Documentation/devicetree/bi
ndings/submitting-patches.rst#L18

The subject will be change to
ASoC: dt-bindings: realtek,rt5575: add support for ALC5575

Quoted 5 lines

Please organize the patch documenting the compatible (DT bindings) before the patch using that compatible.
See also:
https://elixir.bootlin.com/linux/v6.14-rc6/source/Documentation/devicetree/bi
ndings/submitting-patches.rst#L46

I will reorder the patches so that the DT bindings come first.

Thanks.

author maintainer
#16 lore raw

RE: [PATCH v8 2/2] ASoC: dt-bindings: realtek,rt5575: add bindings for ALC5575

In reply to Krzysztof Kozlowski
Quoted 29 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Friday, December 5, 2025 4:31 PM
To: Oder Chiou <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Flove(HsinFu) <[email protected]>; Shuming [范 書銘] <[email protected]>; Jack Yu <[email protected]>; Derek [方德 義] <[email protected]>
Subject: Re: [PATCH v8 2/2] ASoC: dt-bindings: realtek,rt5575: add bindings for ALC5575

External mail : This email originated from outside the organization. Do not reply, click links, or open attachments unless you recognize the sender and know the content is safe.

Quoted 7 lines · Oder Chiou

Realtek ALC5575 is a highly advanced DSP and microphone CODEC that has been designed for AI audio technology. Its impressive features include an advanced HiFi-5 DSP core, a Neural Network Processing Unit (NPU) owned by Realtek, and embedded 4MB memory, which enables it to operate highly advanced AI audio algorithms. The ALC5575 supports 4xA-mic input and 8xD-mic input, as well as a rich set of interfaces such as I2S, I2C, SPI, and UART.

Quoted 7 lines

When I asked to describe hardware, I did not meant marketing junk! Drop all impressive features and simply describe hardware in basic terms. This is not advanced DSP, not designed for AI audio (AI is the easiest way to get a grumpy review), not "highly advanced AI audio algorithms" and does not have "as a rich set of interfaces".

Use simple terms what is this. Audio codec with I2S.... interfaces.

The description will be change to
Audio codec with I2S, I2C and SPI.

Quoted 36 lines
Quoted 4 lines
---
 .../bindings/sound/realtek,rt5575.yaml        | 44

+++++++++++++++++++

Quoted 2 lines
 1 file changed, 44 insertions(+)
 create mode 100644

Documentation/devicetree/bindings/sound/realtek,rt5575.yaml

Review

Quoted 2 lines
(quoted diff) +0 -0

b/Documentation/devicetree/bindings/sound/realtek,rt5575.yaml

Quoted 25 lines

new file mode 100644
index 000000000000..83ccc79e6769

Documentation/devicetree/bindings/sound/realtek,rt5575.yaml new file +20 -0
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/realtek,rt5575.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ALC5575 audio CODEC
+
+maintainers:
+  - Oder Chiou <[email protected]>
+
+description:
+  The device supports both I2C and SPI. I2C is mandatory, while SPI is
+  optional depending on the hardware configuration.
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+  - $ref: dai-common.yaml#
+
+properties:
Quoted 8 lines

Documentation/devicetree/bindings/sound/realtek,rt5575.yaml line 24

Quoted 4 lines
Documentation/devicetree/bindings/sound/realtek,rt5575.yaml new file +4 -0
+  compatible:
+    enum:
+      - realtek,rt5575
+      - realtek,rt5575-with-spi

Drop the second compatible. It's the same device. Whether it supports SPI it is already known and obvious - you cannot place non-SPI chip on SPI bus and expect it to work.

There are two hardware configurations: with SPI flash and without SPI flash. If the codec is shipped without an SPI flash, the SPI driver can still load the firmware through the SPI interface. The second compatible is intended to distinguish between the versions with and without the SPI flash.

#17 lore raw

Re: [PATCH v8 2/2] ASoC: dt-bindings: realtek,rt5575: add bindings for ALC5575

Quoted 6 lines · Oder Chiou
Quoted 6 lines
Quoted 5 lines
Documentation/devicetree/bindings/sound/realtek,rt5575.yaml new file +5 -0
+properties:
+  compatible:
+    enum:
+      - realtek,rt5575
+      - realtek,rt5575-with-spi
Quoted 9 lines
Quoted 3 lines

Drop the second compatible. It's the same device. Whether it supports SPI it is already known and obvious - you cannot place non-SPI chip on SPI bus and expect it to work.

There are two hardware configurations: with SPI flash and without SPI flash. If the codec is shipped without an SPI flash, the SPI driver can still load the firmware through the SPI interface. The second compatible is intended to distinguish between the versions with and without the SPI flash.

Ah, so this is not about the bus. I don't see that compatible referenced in your device ID table, so this is dead code. Drop or justify - by writing driver code using it....

Best regards,
Krzysztof

#18 lore raw

Re: [PATCH v8 2/2] ASoC: dt-bindings: realtek,rt5575: add bindings for ALC5575

Quoted 17 lines · Krzysztof Kozlowski
Quoted 6 lines · Oder Chiou
Quoted 6 lines
Quoted 5 lines
Documentation/devicetree/bindings/sound/realtek,rt5575.yaml new file +5 -0
+properties:
+  compatible:
+    enum:
+      - realtek,rt5575
+      - realtek,rt5575-with-spi
Quoted 9 lines
Quoted 3 lines

Drop the second compatible. It's the same device. Whether it supports SPI it is already known and obvious - you cannot place non-SPI chip on SPI bus and expect it to work.

There are two hardware configurations: with SPI flash and without SPI flash. If the codec is shipped without an SPI flash, the SPI driver can still load the firmware through the SPI interface. The second compatible is intended to distinguish between the versions with and without the SPI flash.

Quoted 4 lines

Ah, so this is not about the bus. I don't see that compatible referenced in your device ID table, so this is dead code. Drop or justify - by writing driver code using it....

Wait, I saw it in deice ID table, but there is no driver data so still pointless.

Best regards,
Krzysztof