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

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

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:
>>
>> 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 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