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