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

[Oder Chiou <oder_chiou@realtek.com>]

> -----Original Message-----
> From: Cezary Rojewski <cezary.rojewski@intel.com>
> Sent: Monday, November 24, 2025 6:45 PM
> To: Oder Chiou <oder_chiou@realtek.com>
> Cc: 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 v7 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 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.

The variable rt5575_spi_ready will be removed.
Declare the rt5575_spi as extern variable, adds it in EXPORT_SYMBOL_GPL(), and
uses the statement 'if (!rt5575_spi)' in the i2c_probe.

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

Yes, the customers can choose 2 different hardware configurations in
w/wo external SPI flash.

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

The multiple compatible names can distinguish between w/wo flash to avoid it.

static const struct of_device_id rt5575_of_match[] = {
	{ .compatible = "realtek,rt5575" },
	{ .compatible = "realtek,rt5575-with-spi" },
	{ }
};

#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(&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");