Re: [PATCH 1/3] ASoC: cs4271: Fix cs4271 I2C and SPI drivers automatic module loading

[Alexander Sverdlin <alexander.sverdlin@gmail.com>]

Hello Herve,

On Thu, 2025-10-16 at 15:03 +0200, Herve Codina wrote:
> In commit c973b8a7dc50 ("ASoC: cs4271: Split SPI and I2C code into
> different modules") the driver was slit into a core, an SPI and an I2C
> part.
> 
> However, the MODULE_DEVICE_TABLE(of, cs4271_dt_ids) was in the core part
> and so, module loading based on module.alias (based on DT compatible
> string matching) loads the core part but not the SPI or I2C parts.
> 
> In order to have the I2C or the SPI module loaded automatically, move
> the MODULE_DEVICE_TABLE(of, ...) the core to I2C and SPI parts.
> Also move cs4271_dt_ids itself from the core part to I2C and SPI parts
> as both the call to MODULE_DEVICE_TABLE(of, ...) and the cs4271_dt_ids
> table itself need to be in the same file.

I'm a bit confused by this change.
What do you have in SYSFS "uevent" entry for the real device?

If you consider spi_uevent() and i2c_device_uevent(), "MODALIAS=" in the
"uevent" should be prefixed with either "spi:" or "i2c:".
And this isn't what you adress in your patch.

You provide [identical] "of:" prefixed modalias to two different modules
(not sure, how this should work), but cs4271 is not an MMIO device,
so it should not generate an "of:" prefixed uevent.

Could you please show the relevant DT snippet for the affected HW?

> Fixes: c973b8a7dc50 ("ASoC: cs4271: Split SPI and I2C code into different modules")
> Cc: stable@vger.kernel.org
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  sound/soc/codecs/cs4271-i2c.c | 6 ++++++
>  sound/soc/codecs/cs4271-spi.c | 6 ++++++
>  sound/soc/codecs/cs4271.c     | 9 ---------
>  sound/soc/codecs/cs4271.h     | 1 -
>  4 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/sound/soc/codecs/cs4271-i2c.c b/sound/soc/codecs/cs4271-i2c.c
> index 1d210b969173..cefb8733fc61 100644
> --- a/sound/soc/codecs/cs4271-i2c.c
> +++ b/sound/soc/codecs/cs4271-i2c.c
> @@ -28,6 +28,12 @@ static const struct i2c_device_id cs4271_i2c_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, cs4271_i2c_id);
>  
> +static const struct of_device_id cs4271_dt_ids[] = {
> +	{ .compatible = "cirrus,cs4271", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, cs4271_dt_ids);
> +
>  static struct i2c_driver cs4271_i2c_driver = {
>  	.driver = {
>  		.name = "cs4271",
> diff --git a/sound/soc/codecs/cs4271-spi.c b/sound/soc/codecs/cs4271-spi.c
> index 4feb80436bd9..82abc654293c 100644
> --- a/sound/soc/codecs/cs4271-spi.c
> +++ b/sound/soc/codecs/cs4271-spi.c
> @@ -23,6 +23,12 @@ static int cs4271_spi_probe(struct spi_device *spi)
>  	return cs4271_probe(&spi->dev, devm_regmap_init_spi(spi, &config));
>  }
>  
> +static const struct of_device_id cs4271_dt_ids[] = {
> +	{ .compatible = "cirrus,cs4271", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, cs4271_dt_ids);
> +
>  static struct spi_driver cs4271_spi_driver = {
>  	.driver = {
>  		.name	= "cs4271",
> diff --git a/sound/soc/codecs/cs4271.c b/sound/soc/codecs/cs4271.c
> index 6a3cca3d26c7..ff9c6628224c 100644
> --- a/sound/soc/codecs/cs4271.c
> +++ b/sound/soc/codecs/cs4271.c
> @@ -543,15 +543,6 @@ static int cs4271_soc_resume(struct snd_soc_component *component)
>  #define cs4271_soc_resume	NULL
>  #endif /* CONFIG_PM */
>  
> -#ifdef CONFIG_OF
> -const struct of_device_id cs4271_dt_ids[] = {
> -	{ .compatible = "cirrus,cs4271", },
> -	{ }
> -};
> -MODULE_DEVICE_TABLE(of, cs4271_dt_ids);
> -EXPORT_SYMBOL_GPL(cs4271_dt_ids);
> -#endif
> -
>  static int cs4271_component_probe(struct snd_soc_component *component)
>  {
>  	struct cs4271_private *cs4271 = snd_soc_component_get_drvdata(component);
> diff --git a/sound/soc/codecs/cs4271.h b/sound/soc/codecs/cs4271.h
> index 290283a9149e..4965ce085875 100644
> --- a/sound/soc/codecs/cs4271.h
> +++ b/sound/soc/codecs/cs4271.h
> @@ -4,7 +4,6 @@
>  
>  #include <linux/regmap.h>
>  
> -extern const struct of_device_id cs4271_dt_ids[];
>  extern const struct regmap_config cs4271_regmap_config;
>  
>  int cs4271_probe(struct device *dev, struct regmap *regmap);

-- 
Alexander Sverdlin.