-----Original Message----- From: Cezary Rojewski cezary.rojewski@intel.com Sent: Wednesday, December 17, 2025 6:46 PM To: Oder Chiou oder_chiou@realtek.com Cc: linux-spi@vger.kernel.org; perex@perex.cz; linux-sound@vger.kernel.org; devicetree@vger.kernel.org; alsa-devel@alsa-project.org; Flove(HsinFu) flove@realtek.com; Shuming [范書銘] shumingf@realtek.com; Jack Yu jack.yu@realtek.com; Derek [方德義] derek.fang@realtek.com; broonie@kernel.org; lgirdwood@gmail.com; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org Subject: Re: [PATCH v10 3/3] 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 2025-12-16 8:18 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
sound/soc/codecs/Kconfig | 10 + sound/soc/codecs/Makefile | 3 + sound/soc/codecs/rt5575-spi.c | 119 +++++++++++ sound/soc/codecs/rt5575-spi.h | 26 +++ sound/soc/codecs/rt5575.c | 359
++++++++++++++++++++++++++++++++++
sound/soc/codecs/rt5575.h | 54 +++++ 6 files changed, 571 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
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index 061791e61907..14f3d09e7117 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -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@@ -1783,6 +1784,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 && I2Cdepends on SND_SOC_RT5575- config SND_SOC_RT5616 tristate "Realtek RT5616 CODEC" depends on I2C
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile index d687d4f74363..a6406bc907a9 100644 --- a/sound/soc/codecs/Makefile +++ b/sound/soc/codecs/Makefile @@ -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-$(CONFIG_SND_SOC_RT5575_SPI) += rt5575-spi.o snd-soc-rt5616-y := rt5616.o snd-soc-rt5631-y := rt5631.o snd-soc-rt5640-y := rt5640.o @@ -686,6 +688,7 @@ 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_RT5616) += snd-soc-rt5616.o obj-$(CONFIG_SND_SOC_RT5631) += snd-soc-rt5631.o obj-$(CONFIG_SND_SOC_RT5640) += snd-soc-rt5640.o diff --git a/sound/soc/codecs/rt5575-spi.c b/sound/soc/codecs/rt5575-spi.c new file mode 100644 index 000000000000..c8eadb2f59a6 --- /dev/null +++ b/sound/soc/codecs/rt5575-spi.c @@ -0,0 +1,119 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*
- rt5575-spi.c -- ALC5575 SPI driver
- Copyright(c) 2025 Realtek Semiconductor Corp.
- */
+#include <linux/firmware.h> +#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_get_device(struct device *dev) +{
struct device_node *spi_np;struct spi_controller *ctlr;Reorder to achieve reverse-christmas-tree notation. Waterfall to a number of functions found in this patch.
struct spi_device *spi;u32 cs;spi_np = of_parse_phandle(dev->of_node, "spi-parent", 0);if (!spi_np) {dev_err(dev, "Failed to get spi-parent phandle\n");return NULL;}if (of_property_read_u32_index(dev->of_node, "spi-parent", 1, &cs))cs = 0;if (cs >= ctlr->num_chipselect) {dev_err(dev, "Chip select has wrong number %d\n", cs);of_node_put(spi_np);return NULL;}ctlr = of_find_spi_controller_by_node(spi_np);of_node_put(spi_np);if (!ctlr)return NULL;spi = spi_new_device(ctlr, &(struct spi_board_info){.modalias = "rt5575",.chip_select = cs,.max_speed_hz = 10000000,});spi_controller_put(ctlr);return spi;+}
+/**
- rt5575_spi_burst_write - Write data to SPI by rt5575 address.
- @spi: SPI device.
- @addr: Start address.
- @txbuf: Data buffer for writing.
- @len: Data length.
- */
+static int rt5575_spi_burst_write(struct spi_device *spi, u32 addr, const u8
*txbuf,
size_t len)Alignment or, just make it one-liner. It fits the limit.
+{
struct rt5575_spi_burst_write buf = {.cmd = RT5575_SPI_CMD_BURST_WRITEMissing comma at the end.
};unsigned int end, offset = 0;while (offset < len) {if (offset + RT5575_SPI_BUF_LEN <= len)end = RT5575_SPI_BUF_LEN;elseend = len % RT5575_SPI_BUF_LEN;buf.addr = cpu_to_le32(addr + offset);memcpy(&buf.data, &txbuf[offset], end);spi_write(spi, &buf, sizeof(buf));offset += RT5575_SPI_BUF_LEN;}return 0;If you intend on ignoring result of spi_write()s, then I see no reason why rt5575_spi_burst_write() shouldn't be a void function.
+}
+int rt5575_spi_fw_load(struct spi_device *spi) +{
const struct firmware *firmware;struct device *dev = &spi->dev;static const char * const fw_path[] = {"realtek/rt5575/rt5575_fw1.bin","realtek/rt5575/rt5575_fw2.bin","realtek/rt5575/rt5575_fw3.bin","realtek/rt5575/rt5575_fw4.bin"};static const u32 fw_addr[] = { 0x5f400000, 0x5f600000, 0x5f7fe000,0x5f7ff000 };
int i, ret;for (i = 0; i < ARRAY_SIZE(fw_addr); i++) {ret = request_firmware(&firmware, fw_path[i], dev);So, in v8 or earlier the ordering of operations was different:
request_firmware(fw1); regmap_write(); regmap_write(); regmap_write(); rt5575_spi_burst_write(); release_firmware(fw1); /* Proceed with loop for the remaining fw2, fw3, fw4. */What changed that suddenly the ordering could be simplified?
Originally, request_firmware_nowait was used when the driver built into the kernel image and the firmwares was in filesystem. In order to match the following flow, if the driver is built into the kernel image, the firmwares should also be built into the kernel image.
1) boot I2C, init regmaps 2) boot SPI, load firmware 3) register ASoC component if 1) and 2) succeed
if (!ret) {rt5575_spi_burst_write(spi, fw_addr[i],firmware->data, firmware->size);
release_firmware(firmware);} else {dev_err(dev, "Request firmware failure: %d\n",ret);
return ret;}Please refactor this construct. When facing else-return, favour returning early to reduce indentation and make it easier to read the code.
if (ret) { dev_err(); return ret; } rt5575_spi_burst_write(); release_firmware();Much better, isn't it?
}return 0;+} diff --git a/sound/soc/codecs/rt5575-spi.h b/sound/soc/codecs/rt5575-spi.h new file mode 100644 index 000000000000..3b296bcac9a6 --- /dev/null +++ b/sound/soc/codecs/rt5575-spi.h @@ -0,0 +1,26 @@ +/* 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__
+#if IS_ENABLED(CONFIG_SND_SOC_RT5575_SPI) +struct spi_device *rt5575_spi_get_device(struct device *dev); +int rt5575_spi_fw_load(struct spi_device *spi); +#else +static inline struct spi_device *rt5575_spi_get_device(struct device *dev) +{
return NULL;+}
Missing newline.
+static inline int rt5575_spi_fw_load(struct spi_device *spi) +{
return -EINVAL;+} +#endif
+#endif /* __RT5575_SPI_H__ */ diff --git a/sound/soc/codecs/rt5575.c b/sound/soc/codecs/rt5575.c new file mode 100644 index 000000000000..c3a9ba22a90d --- /dev/null +++ b/sound/soc/codecs/rt5575.c @@ -0,0 +1,359 @@ +// SPDX-License-Identifier: GPL-2.0-only +/*
- rt5575.c -- ALC5575 ALSA SoC audio component driver
- Copyright(c) 2025 Realtek Semiconductor Corp.
- */
+#include <linux/i2c.h> +#include <sound/soc.h> +#include <sound/tlv.h>
+#include "rt5575.h" +#include "rt5575-spi.h"
(...)
+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;Not an expect in regmap, but I'd leave decision to the caller whether to squelch an error or not.
For both, rt5575_i2c_read()/write() I'd just return the corresponding regmap_xxx() operation rather then '0'.
+}
+static const struct regmap_config rt5575_regmap = {
.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,+};
+static int rt5575_fw_load_by_spi(struct rt5575_priv *rt5575) +{
struct i2c_client *i2c = rt5575->i2c;struct spi_device *spi;struct device *dev = &i2c->dev;int ret;spi = rt5575_spi_get_device(dev);if (!spi) {dev_warn(dev, "The spi-parent not described in the DTS\n""The hardware configuration should bewith built-in flash\n");
if (!IS_ENABLED(CONFIG_SND_SOC_RT5575_SPI))dev_warn(dev, "If the spi-parent is set in the DTSand it is without built-in flash\n"
"Please enableCONFIG_SND_SOC_RT5575_SPI\n");
Does this mean we always get a warning, even in case when we're dealing with non-SPI variant?
return -ENXIO;}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(spi);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(rt5575->regmap, RT5575_SW_INT,ret, !ret, 100000, 10000000);
if (ret) {dev_err(dev, "Run firmware failure: %d\n", ret);return -ENODEV;}return 0;+}
+static int rt5575_i2c_probe(struct i2c_client *i2c) +{
struct rt5575_priv *rt5575;struct device *dev = &i2c->dev;int ret, val;rt5575 = devm_kzalloc(dev, sizeof(struct rt5575_priv), GFP_KERNEL);if (!rt5575)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 DSP 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) {
- The 'RT5575_DEVICE_ID' check above fails, 'val' equals
RT5575_DEVICE_ID, non-zero. 2. regmap_read(RT5575_ID_1) fails, let's skip the reasoning for now. 'val' remains unchanged. 3. The '!val' check fails though nothing has been actually read, 'val' could be simply invalid.
What's the goal of this check? Shouldn't we be more precise when checking the 'formal version'?
Since there is no in-formal version available, the formal version check will be removed.