RE: [PATCH v8 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: Tuesday, December 9, 2025 4:06 AM
> 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 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 2025-12-01 11:59 AM, Oder Chiou wrote:
> > The ALC5575 integrates an audio DSP that typically loads its firmware
> > from an external flash via its own SPI host interface. In certain
> > hardware configurations, the firmware can alternatively be loaded
> > through the SPI client interface. The driver provides basic mute and
> > volume control functions. When the SPI client interface is enabled,
> > firmware loading is handled by the SPI driver.
> >
> > Signed-off-by: Oder Chiou <oder_chiou@realtek.com>
> 
> ...
> 
> > diff --git a/sound/soc/codecs/rt5575-spi.c b/sound/soc/codecs/rt5575-spi.c
> > new file mode 100644
> > index 000000000000..12c2379a061e
> > --- /dev/null
> > +++ b/sound/soc/codecs/rt5575-spi.c
> > @@ -0,0 +1,84 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * rt5575-spi.c  --  ALC5575 SPI driver
> > + *
> > + * Copyright(c) 2025 Realtek Semiconductor Corp.
> > + *
> > + */
> > +
> > +#include <linux/of.h>
> > +#include <linux/spi/spi.h>
> > +
> > +#include "rt5575-spi.h"
> > +
> > +#define RT5575_SPI_CMD_BURST_WRITE   5
> > +#define RT5575_SPI_BUF_LEN           240
> > +
> > +struct rt5575_spi_burst_write {
> > +     u8 cmd;
> > +     u32 addr;
> > +     u8 data[RT5575_SPI_BUF_LEN];
> > +     u8 dummy;
> > +} __packed;
> > +
> > +struct spi_device *rt5575_spi;
> > +EXPORT_SYMBOL_GPL(rt5575_spi);
> > +
> > +/**
> > + * rt5575_spi_burst_write - Write data to SPI by rt5575 address.
> > + * @addr: Start address.
> > + * @txbuf: Data buffer for writing.
> > + * @len: Data length.
> > + *
> > + */
> > +int rt5575_spi_burst_write(u32 addr, const u8 *txbuf, size_t len)
> > +{
> > +     struct rt5575_spi_burst_write buf = {
> > +             .cmd = RT5575_SPI_CMD_BURST_WRITE
> > +     };
> > +     unsigned int end, offset = 0;
> > +
> > +     while (offset < len) {
> > +             if (offset + RT5575_SPI_BUF_LEN <= len)
> > +                     end = RT5575_SPI_BUF_LEN;
> > +             else
> > +                     end = len % RT5575_SPI_BUF_LEN;
> > +
> > +             buf.addr = cpu_to_le32(addr + offset);
> > +
> > +             memcpy(&buf.data, &txbuf[offset], end);
> > +
> > +             spi_write(rt5575_spi, &buf, sizeof(buf));
> > +
> > +             offset += RT5575_SPI_BUF_LEN;
> 
> Make it cohesive by proper spacing - it's a logical block, and logical
> blocks are easier to read if grouped together e.g.:
> 
> <assuming newline after the if-else-statement>
> 
>                 buf.addr = cpu_to_le32(addr + offset);
>                 memcpy(&buf.data, &txbuf[offset], end);
>                 spi_write(rt5575_spi, &buf, sizeof(buf));
> 
>                 offset += RT5575_SPI_BUF_LEN;
> 
> See?  Clear separation of operations leading to spi_write() and the
> offset increment.
> 
> > +     }
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(rt5575_spi_burst_write);
> > +
> > +static int rt5575_spi_probe(struct spi_device *spi)
> > +{
> > +     rt5575_spi = spi;
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct of_device_id rt5575_of_match[] = {
> > +     { .compatible = "realtek,rt5575" },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(of, rt5575_of_match);
> > +
> > +static struct spi_driver rt5575_spi_driver = {
> > +     .driver = {
> > +             .name = "rt5575",
> > +             .of_match_table = of_match_ptr(rt5575_of_match),
> > +     },
> > +     .probe = rt5575_spi_probe,
> > +};
> > +module_spi_driver(rt5575_spi_driver);
> > +
> > +MODULE_DESCRIPTION("ALC5575 SPI driver");
> > +MODULE_AUTHOR("Oder Chiou <oder_chiou@realtek.com>");
> > +MODULE_LICENSE("GPL");
> > diff --git a/sound/soc/codecs/rt5575-spi.h b/sound/soc/codecs/rt5575-spi.h
> > new file mode 100644
> > index 000000000000..b364b49bb43e
> > --- /dev/null
> > +++ b/sound/soc/codecs/rt5575-spi.h
> > @@ -0,0 +1,16 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * rt5575-spi.h  --  ALC5575 SPI driver
> > + *
> > + * Copyright(c) 2025 Realtek Semiconductor Corp.
> > + *
> > + */
> > +
> > +#ifndef __RT5575_SPI_H__
> > +#define __RT5575_SPI_H__
> > +
> > +extern struct spi_device *rt5575_spi;
> > +
> > +int rt5575_spi_burst_write(u32 addr, const u8 *txbuf, size_t len);
> > +
> > +#endif /* __RT5575_SPI_H__ */
> 
> The entire header can be dropped and its content moved to rt5575.h.
> It's included in both C-files plus we want to get rid of the global
> rt5575_spi anyway.  I do not think rt5575_spi_burst_write() belongs here
> either, see my comments regarding rt5575_fw_load_by_spi().
> 
> > diff --git a/sound/soc/codecs/rt5575.c b/sound/soc/codecs/rt5575.c
> > new file mode 100644
> > index 000000000000..c7e8f5a606bc
> > --- /dev/null
> > +++ b/sound/soc/codecs/rt5575.c
> > @@ -0,0 +1,375 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * rt5575.c  --  ALC5575 ALSA SoC audio component driver
> > + *
> > + * Copyright(c) 2025 Realtek Semiconductor Corp.
> > + *
> > + */
> > +
> > +#include <linux/firmware.h>
> > +#include <linux/i2c.h>
> > +#include <sound/soc.h>
> > +#include <sound/tlv.h>
> > +
> > +#include "rt5575.h"
> > +#if IS_ENABLED(CONFIG_SND_SOC_RT5575_SPI)
> > +#include "rt5575-spi.h"
> > +#endif
> > +
> > +static bool rt5575_readable_register(struct device *dev, unsigned int reg)
> > +{
> > +     switch (reg) {
> > +     case RT5575_ID:
> > +     case RT5575_ID_1:
> > +     case RT5575_MIXL_VOL:
> > +     case RT5575_MIXR_VOL:
> > +     case RT5575_PROMPT_VOL:
> > +     case RT5575_SPK01_VOL:
> > +     case RT5575_SPK23_VOL:
> > +     case RT5575_MIC1_VOL:
> > +     case RT5575_MIC2_VOL:
> > +     case RT5575_WNC_CTRL:
> > +     case RT5575_MODE_CTRL:
> > +     case RT5575_I2S_RATE_CTRL:
> > +     case RT5575_SLEEP_CTRL:
> > +     case RT5575_ALG_BYPASS_CTRL:
> > +     case RT5575_PINMUX_CTRL_2:
> > +     case RT5575_GPIO_CTRL_1:
> > +     case RT5575_DSP_BUS_CTRL:
> > +     case RT5575_SW_INT:
> > +     case RT5575_DSP_BOOT_ERR:
> > +     case RT5575_DSP_READY:
> > +     case RT5575_DSP_CMD_ADDR:
> > +     case RT5575_EFUSE_DATA_2:
> > +     case RT5575_EFUSE_DATA_3:
> > +             return true;
> > +     default:
> > +             return false;
> > +     }
> > +}
> > +
> > +static const DECLARE_TLV_DB_SCALE(ob_tlv, -9525, 75, 0);
> > +
> > +#if IS_ENABLED(CONFIG_SND_SOC_RT5575_SPI)
> > +static void rt5575_fw_load_by_spi(const struct firmware *fw, void *context)
> 
> So, rt5575_spi_burst_write(), a wrapper for spi_write(), is exported to
> be available outside of the rt5575-spi module while its only user,
> rt5575_fw_load_by_spi(), which performs no I2C-specific tasks, is found
> with the common, rt5575.c file?

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;
	}

	...
}

> We can do better.  There are couple options here, one of them consists of:
> 
> 1) privatize rt5575_spi_burst_write()
> 2) make rt5575_fw_load_by_spi() public
> 3) change rt5575_fw_load_by_spi() from void to int and return
> request_firmware_xxx() result
> 4) reword to rt5575_spi_fw_load() to match its friends
> 
> In regard to 1), have a #if-else preproc added to rt5575.h that governs
> the implementation of said function.  If xxx_RT5575_SPI is disabled, let
> is be a stub that returns 0.
> 
> In regard to 2), please do not ignore failures from firmware loading,
> that's a recurring point in this review and keeps being ignored.  No,
> async-firmware loading in not the answer why potential errors are left
> unhandled.
> 
> Another option, perhaps a better one is to have both
> rt5575_spi_burst_write() and rt5575_spi_fw_load() privatized and move
> the firmware-loading to the SPI-device probe.  See my comments targeting
> rt5575_probe().
> 
> > +{
> > +     struct rt5575_priv *rt5575 = context;
> > +     struct i2c_client *i2c = rt5575->i2c;
> 
> The whole reason this function needs i2c_client is ->dev retrieval.
> Let's simplify by listing 'struct device *dev' as an argument instead.
> With that, your function's argument list is also more explicit.
> 
> > +     const struct firmware *firmware;
> > +     static const char * const fw_path[] = {
> > +             "realtek/rt5575/rt5575_fw2.bin",
> > +             "realtek/rt5575/rt5575_fw3.bin",
> > +             "realtek/rt5575/rt5575_fw4.bin"
> > +     };
> > +     static const u32 fw_addr[] = { 0x5f600000, 0x5f7fe000, 0x5f7ff000 };
> > +     int i, ret;
> > +
> > +     regmap_write(rt5575->dsp_regmap, 0xfafafafa, 0x00000004);
> > +     regmap_write(rt5575->dsp_regmap, 0x18008064, 0x00000000);
> > +     regmap_write(rt5575->dsp_regmap, 0x18008068, 0x0002ffff);
> > +
> > +     rt5575_spi_burst_write(0x5f400000, fw->data, fw->size);
> > +     release_firmware(fw);
> > +
> > +     for (i = 0; i < ARRAY_SIZE(fw_addr); i++) {
> > +             ret = request_firmware(&firmware, fw_path[i], &i2c->dev);
> > +             if (!ret) {
> > +                     rt5575_spi_burst_write(fw_addr[i],
> firmware->data, firmware->size);
> > +                     release_firmware(firmware);
> > +             } else {
> > +                     dev_err(&i2c->dev, "Request firmware failure:
> %d\n", ret);
> > +                     return;
> > +             }
> > +     }
> > +
> > +     regmap_write(rt5575->dsp_regmap, 0x18000000, 0x00000000);
> > +
> > +     regmap_update_bits(rt5575->regmap, RT5575_SW_INT, 1, 1);
> > +
> > +     regmap_read_poll_timeout(rt5575->regmap, RT5575_SW_INT,
> ret, !ret, 100000, 10000000);
> > +
> > +     if (ret)
> > +             dev_err(&i2c->dev, "Run firmware failure: %d\n", ret);
> > +}
> > +#endif
> > +
> > +static const struct snd_kcontrol_new rt5575_snd_controls[] = {
> > +     SOC_DOUBLE("Speaker CH-01 Playback Switch", RT5575_SPK01_VOL,
> 31, 15, 1, 1),
> > +     SOC_DOUBLE_TLV("Speaker CH-01 Playback Volume",
> RT5575_SPK01_VOL, 17, 1, 167, 0, ob_tlv),
> > +     SOC_DOUBLE("Speaker CH-23 Playback Switch", RT5575_SPK23_VOL,
> 31, 15, 1, 1),
> > +     SOC_DOUBLE_TLV("Speaker CH-23 Playback Volume",
> RT5575_SPK23_VOL, 17, 1, 167, 0, ob_tlv),
> > +     SOC_DOUBLE("Mic1 Capture Switch", RT5575_MIC1_VOL, 31, 15, 1,
> 1),
> > +     SOC_DOUBLE_TLV("Mic1 Capture Volume", RT5575_MIC1_VOL, 17,
> 1, 167, 0, ob_tlv),
> > +     SOC_DOUBLE("Mic2 Capture Switch", RT5575_MIC2_VOL, 31, 15, 1,
> 1),
> > +     SOC_DOUBLE_TLV("Mic2 Capture Volume", RT5575_MIC2_VOL, 17,
> 1, 167, 0, ob_tlv),
> > +     SOC_DOUBLE_R("Mix Playback Switch", RT5575_MIXL_VOL,
> RT5575_MIXR_VOL, 31, 1, 1),
> > +     SOC_DOUBLE_R_TLV("Mix Playback Volume", RT5575_MIXL_VOL,
> RT5575_MIXR_VOL, 1, 127, 0,
> > +             ob_tlv),
> > +     SOC_DOUBLE("Prompt Playback Switch", RT5575_PROMPT_VOL, 31,
> 15, 1, 1),
> > +     SOC_DOUBLE_TLV("Prompt Playback Volume",
> RT5575_PROMPT_VOL, 17, 1, 167, 0, ob_tlv),
> > +};
> > +
> > +static const struct snd_soc_dapm_widget rt5575_dapm_widgets[] = {
> > +     SND_SOC_DAPM_AIF_IN("AIF1RX", "AIF1 Playback", 0,
> SND_SOC_NOPM, 0, 0),
> > +     SND_SOC_DAPM_AIF_OUT("AIF1TX", "AIF1 Capture", 0,
> SND_SOC_NOPM, 0, 0),
> > +     SND_SOC_DAPM_AIF_IN("AIF2RX", "AIF2 Playback", 0,
> SND_SOC_NOPM, 0, 0),
> > +     SND_SOC_DAPM_AIF_OUT("AIF2TX", "AIF2 Capture", 0,
> SND_SOC_NOPM, 0, 0),
> > +     SND_SOC_DAPM_AIF_IN("AIF3RX", "AIF3 Playback", 0,
> SND_SOC_NOPM, 0, 0),
> > +     SND_SOC_DAPM_AIF_OUT("AIF3TX", "AIF3 Capture", 0,
> SND_SOC_NOPM, 0, 0),
> > +     SND_SOC_DAPM_AIF_IN("AIF4RX", "AIF4 Playback", 0,
> SND_SOC_NOPM, 0, 0),
> > +     SND_SOC_DAPM_AIF_OUT("AIF4TX", "AIF4 Capture", 0,
> SND_SOC_NOPM, 0, 0),
> > +
> > +     SND_SOC_DAPM_INPUT("INPUT"),
> > +     SND_SOC_DAPM_OUTPUT("OUTPUT"),
> > +};
> > +
> > +static const struct snd_soc_dapm_route rt5575_dapm_routes[] = {
> > +     { "AIF1TX", NULL, "INPUT" },
> > +     { "AIF2TX", NULL, "INPUT" },
> > +     { "AIF3TX", NULL, "INPUT" },
> > +     { "AIF4TX", NULL, "INPUT" },
> > +     { "OUTPUT", NULL, "AIF1RX" },
> > +     { "OUTPUT", NULL, "AIF2RX" },
> > +     { "OUTPUT", NULL, "AIF3RX" },
> > +     { "OUTPUT", NULL, "AIF4RX" },
> > +};
> > +
> > +static long long rt5575_get_priv_id(struct rt5575_priv *rt5575)
> > +{
> > +     int priv_id_low, priv_id_high;
> > +
> > +     regmap_write(rt5575->regmap, RT5575_EFUSE_PID, 0xa0000000);
> > +     regmap_read(rt5575->regmap, RT5575_EFUSE_DATA_2,
> &priv_id_low);
> > +     regmap_read(rt5575->regmap, RT5575_EFUSE_DATA_3,
> &priv_id_high);
> > +     regmap_write(rt5575->regmap, RT5575_EFUSE_PID, 0);
> > +
> > +     return ((long long)priv_id_high << 32) | (long long)priv_id_low;
> > +}
> > +
> > +static const struct of_device_id rt5575_of_match[] = {
> > +     { .compatible = "realtek,rt5575" },
> > +     { .compatible = "realtek,rt5575-with-spi" },
> > +     { }
> > +};
> > +MODULE_DEVICE_TABLE(of, rt5575_of_match);
> > +
> > +static int rt5575_probe(struct snd_soc_component *component)
> > +{
> > +     struct rt5575_priv *rt5575 =
> snd_soc_component_get_drvdata(component);
> > +     struct device *dev = component->dev;
> > +
> > +     rt5575->component = component;
> > +
> > +     dev_info(dev, "Private ID: %llx\n", rt5575_get_priv_id(rt5575));
> > +
> > +#if IS_ENABLED(CONFIG_SND_SOC_RT5575_SPI)
> > +     if (of_device_is_compatible(dev->of_node,
> rt5575_of_match[1].compatible))
> > +             request_firmware_nowait(THIS_MODULE,
> FW_ACTION_UEVENT,
> 
> 
> An ASoC card is typically a mariage of platform-component (e.g.: SoC
> level DSP) and a codec-component (e.g.: rt5575).  If for any reason the
> platform-component becomes unloaded, codec-component will also be.
> When
> the platform-component becomes available again, the
> codec-component->probe() would be invoked again - firmware would be
> loaded again.  Is this the desired behaviour?
> 
> To answer that, the follow up question is:  Is the firmware-loading for
> this particular solution, a chip -level operation or, a sound-card
> -level operation?  Typically firmware-loading is the former.  Once
> succeeeds, an ASoC component can be registered.  There is no reason to
> register ASoC component, which is mainly used for PCM/streaming reasons,
> if the preliminary setup - firmware-loading - is unsuccefull.
> 
> Perhaps a good approach is to move the firmware loading to the
> SPI-device's probe()!

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

Thanks,
Oder