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

[Oder Chiou <oder_chiou@realtek.com>]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Tuesday, December 9, 2025 2:08 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 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.

The code will be implemented in the top of the i2c_probe() as following.

if (dev_type == RT5575_WITHOUT_FLASH) {
	if (IS_ENABLED(CONFIG_SND_SOC_RT5575_SPI)) {
		if (!rt5575_spi) {
			dev_err(dev, "Wait SPI driver ready\n");
			return -EPROBE_DEFER;
		}
	} else {
		dev_err(dev, "This dev type should enable CONFIG_SND_SOC_RT5575_SPI\n");
		return -ENODEV;
	}
}

Thanks,
Oder