On 2025-12-09 6:35 AM, Oder Chiou wrote:
-----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
...
+#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.
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.
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:
- privatize rt5575_spi_burst_write()
- make rt5575_fw_load_by_spi() public
- 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 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.
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