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

> > +     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 },
	{ }
};

> > +             return -EPROBE_DEFER;
> > +#endif
> > +
> > +     rt5575 = devm_kzalloc(dev, sizeof(struct rt5575_priv),
> > +                             GFP_KERNEL);
> > +     if (rt5575 == NULL)
> 
> This is not Linux coding style. Open existing drivers. It's everywhere
> written (!foo)
I will correct it.

> > +             return -ENOMEM;
> > +
> > +     i2c_set_clientdata(i2c, rt5575);
> > +
> > +     rt5575->i2c = i2c;
> > +
> > +     rt5575->dsp_regmap = devm_regmap_init_i2c(i2c,
> &rt5575_dsp_regmap);
> > +     if (IS_ERR(rt5575->dsp_regmap)) {
> > +             ret = PTR_ERR(rt5575->dsp_regmap);
> > +             dev_err(dev, "Failed to allocate register map: %d\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     rt5575->regmap = devm_regmap_init(dev, NULL, i2c,
> &rt5575_regmap);
> > +     if (IS_ERR(rt5575->regmap)) {
> > +             ret = PTR_ERR(rt5575->regmap);
> > +             dev_err(dev, "Failed to allocate register map: %d\n", ret);
> > +             return ret;
> > +     }
> > +
> > +     regmap_read(rt5575->regmap, RT5575_ID, &val);
> > +     if (val != RT5575_DEVICE_ID) {
> > +             dev_err(dev, "Device with ID register %08x is not rt5575\n",
> val);
> > +             return -ENODEV;
> > +     }
> > +
> > +     regmap_read(rt5575->regmap, RT5575_ID_1, &val);
> > +     if (!val) {
> > +             dev_err(dev, "This is not formal version\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     return devm_snd_soc_register_component(dev,
> &rt5575_soc_component_dev, rt5575_dai,
> > +             ARRAY_SIZE(rt5575_dai));
> > +}
> > +
> > +static struct i2c_driver rt5575_i2c_driver = {
> > +     .driver = {
> > +             .name = "rt5575",
> > +             .owner = THIS_MODULE,
> 
> Please drop it.
> 
> Please run standard kernel tools for static analysis, like coccinelle,
> smatch and sparse, and fix reported warnings. Also please check for
> warnings when building with W=1 for gcc and clang. Most of these
> commands (checks or W=1 build) can build specific targets, like some
> directory, to narrow the scope to only your code. The code here looks
> like it needs a fix. Feel free to get in touch if the warning is not clear.
> 
> > +             .of_match_table = of_match_ptr(rt5575_of_match),
> 
> You have warning here. Drop of_match_ptr.
I will remove it.

Thanks,
Oder