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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 08/12/2025 10:16, Oder Chiou wrote:
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzk@kernel.org>
>> Sent: Monday, December 8, 2025 4:02 PM
>> To: Oder Chiou <oder_chiou@realtek.com>; cezary.rojewski@intel.com;
>> broonie@kernel.org; lgirdwood@gmail.com; robh@kernel.org;
>> krzk+dt@kernel.org; conor+dt@kernel.org
>> 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>
>> 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 08/12/2025 08:29, Oder Chiou wrote:
>>>> -----Original Message-----
>>>> From: Krzysztof Kozlowski <krzk@kernel.org>
>>>> Sent: Monday, December 8, 2025 2:05 PM
>>>> To: Oder Chiou <oder_chiou@realtek.com>; cezary.rojewski@intel.com;
>>>> broonie@kernel.org; lgirdwood@gmail.com; robh@kernel.org;
>>>> krzk+dt@kernel.org; conor+dt@kernel.org
>>>> 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>
>>>> 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 01/12/2025 11:59, Oder Chiou wrote:
>>>>> +
>>>>> +static int rt5575_i2c_read(void *context, unsigned int reg,
>>>>> +unsigned int
>>>> *val)
>>>>> +{
>>>>> +     struct i2c_client *client = context;
>>>>> +     struct rt5575_priv *rt5575 = i2c_get_clientdata(client);
>>>>> +
>>>>> +     regmap_read(rt5575->dsp_regmap, reg | RT5575_DSP_MAPPING,
>>>> val);
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static int rt5575_i2c_write(void *context, unsigned int reg,
>>>>> +unsigned int val) {
>>>>> +     struct i2c_client *client = context;
>>>>> +     struct rt5575_priv *rt5575 = i2c_get_clientdata(client);
>>>>> +
>>>>> +     regmap_write(rt5575->dsp_regmap, reg | RT5575_DSP_MAPPING,
>>>> val);
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static const struct regmap_config rt5575_regmap = {
>>>>> +     .reg_bits = 16,
>>>>> +     .val_bits = 32,
>>>>> +     .reg_stride = 4,
>>>>> +     .max_register = 0xfffc,
>>>>> +     .readable_reg = rt5575_readable_register,
>>>>> +     .reg_read = rt5575_i2c_read,
>>>>> +     .reg_write = rt5575_i2c_write,
>>>>> +     .use_single_read = true,
>>>>> +     .use_single_write = true,
>>>>> +};
>>>>
>>>> OF device ID table goes around here - together with I2C.
>>> I will correct it.
>>>
>>>>> +
>>>>> +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;
>>>>> +     struct device *dev = &i2c->dev;
>>>>> +     int ret, val;
>>>>> +
>>>>> +#if IS_ENABLED(CONFIG_SND_SOC_RT5575_SPI)
>>>>
>>>> No ifdefs in driver code.
>>>
>>> I am not understanding exactly.
>>> If the machine is without SPI interface and the codec with flash, the
>>> CONFIG_SND_SOC_RT5575_SPI can be disabled.
>>
>> But you still should not use #ifdef. Coding style gives you alternative, please
>> look at the doc.
>>
>>>
>>>>> +     if (!rt5575_spi && of_device_is_compatible(dev->of_node,
>>>> rt5575_of_match[1].compatible))
>>>>
>>>> No, use driver match data if ever, but this is just wrong. You said
>>>> it depends on SPI flash, not SPI interface.
>>>
>>> I will modify it to use the match data as following.
>>> static const struct of_device_id rt5575_of_match[] = {
>>>       { .compatible = "realtek,rt5575", .data = (void
>> *)RT5575_WITH_FLASH },
>>>       { .compatible = "realtek,rt5575-use-spi", .data = (void
>>> *)RT5575_WITHOUT_FLASH },
>>
>> What is still wrong is that why you defer probe if there is no flash. I really do
>> not get it...
> 
> If the codec has flash, the flash is connected to the codec's SPI host
> interface.
> 
> If the codec has no flash, the codec SPI driver should load the firmware
> from the codec's SPI slave interface. The I2C driver must wait until the
> SPI driver is ready to ensure the firmware is loaded correctly.
> 

Ah, so for that reason you created that singleton, exported it and you
wait for it? Singletons are pretty no-go anyway, how do you handle to
codecs in the system? Fragile design.

Best regards,
Krzysztof