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

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

On 2025-11-21 9:41 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>

Please CC the reviewers who actively review your patches, makes it 
easier not miss the follow ups.

Not talking about the tag-area, just the email Cc :)

> ---
>   sound/soc/codecs/Kconfig      |  10 +
>   sound/soc/codecs/Makefile     |   4 +
>   sound/soc/codecs/rt5575-spi.c |  86 ++++++++
>   sound/soc/codecs/rt5575-spi.h |  16 ++
>   sound/soc/codecs/rt5575.c     | 370 ++++++++++++++++++++++++++++++++++
>   sound/soc/codecs/rt5575.h     |  54 +++++
>   6 files changed, 540 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 a0dfef57200c..a3ea5febd1e0 100644
> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig
> @@ -211,6 +211,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
> @@ -1767,6 +1768,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
> diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
> index 39138d96a720..82f660cbe8ec 100644
> --- a/sound/soc/codecs/Makefile
> +++ b/sound/soc/codecs/Makefile
> @@ -252,6 +252,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
> @@ -684,6 +686,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
> diff --git a/sound/soc/codecs/rt5575-spi.c b/sound/soc/codecs/rt5575-spi.c
> new file mode 100644
> index 000000000000..9dbc8170fb76
> --- /dev/null
> +++ b/sound/soc/codecs/rt5575-spi.c
> @@ -0,0 +1,86 @@
> +// 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;
> +
> +bool rt5575_spi_ready;

I believe the patch is dead on arrival, unfortunately as it won't 
compile as =m (module) when CONFIG_SND_SOC_RT5575_SPI is enabled as =m 
simultaneously. EXPORT_SYMBOL_GPL() or a friend of his is missing.

> +static struct spi_device *rt5575_spi;
> +
> +/**
> + * rt5575_spi_burst_write - Write data to SPI by rt5575 address.
> + * @addr: Start address.
> + * @txbuf: Data buffer for writing.
> + * @len: Data length.
> + *
> + */
> +int rt5575_spi_burst_write(u32 addr, const u8 *txbuf, size_t len)
> +{
> +	struct rt5575_spi_burst_write buf = {
> +		.cmd = RT5575_SPI_CMD_BURST_WRITE
> +	};
> +	unsigned int end, offset = 0;
> +
> +	while (offset < len) {
> +		if (offset + RT5575_SPI_BUF_LEN <= len)
> +			end = RT5575_SPI_BUF_LEN;
> +		else
> +			end = len % RT5575_SPI_BUF_LEN;
> +
> +		buf.addr = cpu_to_le32(addr + offset);
> +
> +		memcpy(&buf.data, &txbuf[offset], end);
> +
> +		spi_write(rt5575_spi, &buf, sizeof(buf));
> +
> +		offset += RT5575_SPI_BUF_LEN;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(rt5575_spi_burst_write);
> +
> +static int rt5575_spi_probe(struct spi_device *spi)
> +{
> +	rt5575_spi = spi;
> +
> +	rt5575_spi_ready = true;


I still do not understand the logic behind having rt5575_spi _and_ 
rt5575_spi_ready. The latter is used just one, in the I2C-device probe. 
rt5575_spi is static and will be by default initialized to NULL. 
Statement 'if (!rt5575_spi)' is enough to cover that single occurrence 
in your code.

> +
> +	return 0;
> +}

...

> diff --git a/sound/soc/codecs/rt5575.c b/sound/soc/codecs/rt5575.c
> new file mode 100644
> index 000000000000..58508d643273
> --- /dev/null
> +++ b/sound/soc/codecs/rt5575.c

...

> +static int rt5575_probe(struct snd_soc_component *component)
> +{
> +	struct rt5575_priv *rt5575 = snd_soc_component_get_drvdata(component);
> +
> +	rt5575->component = component;
> +
> +	dev_info(component->dev, "Private ID: %llx\n", rt5575_get_priv_id(rt5575));
> +
> +#if IS_ENABLED(CONFIG_SND_SOC_RT5575_SPI)
> +	request_firmware_nowait(THIS_MODULE, FW_ACTION_UEVENT, "realtek/rt5575/rt5575_fw1.bin",
> +		component->dev, GFP_KERNEL, rt5575, rt5575_fw_load_by_spi);

I do not believe this is OK. If the SPI component exists, from the 
code-perspective it looks like the firmware-loading procedure is 
essential and failing to load the firmware should halt driver's operations.

See below as this touches on the driver design which looks off and I 
haven't found any valid argumentation in your previous message that 
would clarify the situation.

> +#endif
> +
> +	return 0;
> +}

...

> +static const struct i2c_device_id rt5575_i2c_id[] = {
> +	{ "rt5575" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, rt5575_i2c_id);
> +
> +static int rt5575_i2c_probe(struct i2c_client *i2c)
> +{
> +	struct rt5575_priv *rt5575;
> +	int ret, val;
> +
> +#if IS_ENABLED(CONFIG_SND_SOC_RT5575_SPI)
> +	if (!rt5575_spi_ready)
> +		return -EPROBE_DEFER;
> +#endif

I finally understood what's off in the design, and this part is crucial 
to the subject.

If CONFIG_SND_SOC_RT5575_SPI is enabled, and the SPI kept failing for 
whatever reason, the I2C device probe would get deferred continuously. 
As the kconfig looks optional, the question is: Does rt5575 codec exist 
in a "without SPI flash" version or, is it always shipped with SPI flash?

If it does exist as a standalone component - without SPI - then enabling 
CONFIG_SND_SOC_RT5575_SPI effectively renders the driver dysfunctional.

> +
> +	rt5575 = devm_kzalloc(&i2c->dev, sizeof(struct rt5575_priv),
> +				GFP_KERNEL);
> +	if (rt5575 == NULL)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(i2c, rt5575);
> +
> +	rt5575->i2c = i2c;
> +
> +	rt5575->dsp_regmap = devm_regmap_init_i2c(i2c, &rt5575_dsp_regmap);
> +	if (IS_ERR(rt5575->dsp_regmap)) {
> +		ret = PTR_ERR(rt5575->dsp_regmap);
> +		dev_err(&i2c->dev, "Failed to allocate register map: %d\n", ret);
> +		return ret;
> +	}
> +
> +	rt5575->regmap = devm_regmap_init(&i2c->dev, NULL, i2c, &rt5575_regmap);
> +	if (IS_ERR(rt5575->regmap)) {
> +		ret = PTR_ERR(rt5575->regmap);
> +		dev_err(&i2c->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(&i2c->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(&i2c->dev, "This is not formal version\n");
> +		return -ENODEV;
> +	}
> +
> +	return devm_snd_soc_register_component(&i2c->dev, &rt5575_soc_component_dev, rt5575_dai,
> +		ARRAY_SIZE(rt5575_dai));
> +}
> +
> +static const struct of_device_id rt5575_of_match[] = {
> +	{ .compatible = "realtek,rt5575" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, rt5575_of_match);
> +
> +static struct i2c_driver rt5575_i2c_driver = {
> +	.driver = {
> +		.name = "rt5575",
> +		.owner = THIS_MODULE,
> +		.of_match_table = of_match_ptr(rt5575_of_match),
> +	},
> +	.probe = rt5575_i2c_probe,
> +	.id_table = rt5575_i2c_id,
> +};
> +module_i2c_driver(rt5575_i2c_driver);
> +
> +MODULE_DESCRIPTION("ASoC ALC5575 driver");
> +MODULE_AUTHOR("Oder Chiou <oder_chiou@realtek.com>");
> +MODULE_LICENSE("GPL");