[PATCH 0/3] Add support for an external Master Clock in the Cirrus CS4271 codec

[PATCH 0/3] Add support for an external Master Clock in the Cirrus CS4271 codec

[Herve Codina]

The Cirrus CS4271 codec can have its Master Clock provided by an
external clock when no crystal is used.

This series adds support for this external Master clock.

The first patch in the series is not related to the clock but fixes an
issue related to module loading and MODULE_DEVICE_TABLE() due to a
driver split between i2c part and spi part.

The next patch document the Master clock in the binding and the last
patch implement this clock handling in the existing driver.

Best regards,
Hervé

Herve Codina (3):
  ASoC: cs4271: Fix cs4271 I2C and SPI drivers automatic module loading
  ASoC: dt-bindings: cirrus,cs4271: Document mclk clock
  ASoC: cs4271: Add support for the external mclk

 .../bindings/sound/cirrus,cs4271.yaml         | 10 +++++
 sound/soc/codecs/cs4271-i2c.c                 |  6 +++
 sound/soc/codecs/cs4271-spi.c                 |  6 +++
 sound/soc/codecs/cs4271.c                     | 43 +++++++++++++------
 sound/soc/codecs/cs4271.h                     |  1 -
 5 files changed, 53 insertions(+), 13 deletions(-)

-- 
2.51.0

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

[Herve Codina]

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.

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);
-- 
2.51.0

[PATCH 2/3] ASoC: dt-bindings: cirrus,cs4271: Document mclk clock

[Herve Codina]

The Cirrus CS4271 codec can use an external clock as an input Master
Clock. When no crystal is used, the CS4271 component considers its MCLK
pin as an input pin and expects the external clock connected to provide
the Master Clock.

This clock is not documented in the binding.

Add the missing clock.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 .../devicetree/bindings/sound/cirrus,cs4271.yaml       | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/cirrus,cs4271.yaml b/Documentation/devicetree/bindings/sound/cirrus,cs4271.yaml
index 68fbf5cc208f..d286eb169915 100644
--- a/Documentation/devicetree/bindings/sound/cirrus,cs4271.yaml
+++ b/Documentation/devicetree/bindings/sound/cirrus,cs4271.yaml
@@ -25,6 +25,16 @@ properties:
   reg:
     maxItems: 1
 
+  clocks:
+    items:
+      - description:
+          Master clock connected to the MCLK pin if MCLK is an input (i.e. no
+          crystal used).
+
+  clock-names:
+    items:
+      - const: mclk
+
   spi-cpha: true
 
   spi-cpol: true
-- 
2.51.0

[PATCH 3/3] ASoC: cs4271: Add support for the external mclk

[Herve Codina]

The mclk (master clock) of the cs4271 codec can be an input clock.

In this case the connected clock needs to be enabled outside of any
audio stream. Indeed, this clock is needed for i2c communication.

Add support of this clock and enable it before the first i2c transfer.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 sound/soc/codecs/cs4271.c | 34 +++++++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/cs4271.c b/sound/soc/codecs/cs4271.c
index ff9c6628224c..481a2c20b7cf 100644
--- a/sound/soc/codecs/cs4271.c
+++ b/sound/soc/codecs/cs4271.c
@@ -10,6 +10,7 @@
  * DAPM support not implemented.
  */
 
+#include <linux/clk.h>
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/delay.h>
@@ -163,6 +164,7 @@ struct cs4271_private {
 	/* enable soft reset workaround */
 	bool				enable_soft_reset;
 	struct regulator_bulk_data      supplies[ARRAY_SIZE(supply_names)];
+	struct clk *clk;
 };
 
 static const struct snd_soc_dapm_widget cs4271_dapm_widgets[] = {
@@ -567,22 +569,36 @@ static int cs4271_component_probe(struct snd_soc_component *component)
 		cs4271->enable_soft_reset = cs4271plat->enable_soft_reset;
 	}
 
+	ret = clk_prepare_enable(cs4271->clk);
+	if (ret) {
+		dev_err(component->dev, "Failed to enable clk: %d\n", ret);
+		goto err_disable_regulators;
+	}
+
+	/*
+	 * Be sure to have the clock and power-supplies stable before releasing
+	 * the reset.
+	 */
+	fsleep(1000);
+
 	/* Reset codec */
 	cs4271_reset(component);
 
 	ret = regcache_sync(cs4271->regmap);
 	if (ret < 0)
-		return ret;
+		goto err_force_reset;
 
 	ret = regmap_update_bits(cs4271->regmap, CS4271_MODE2,
 				 CS4271_MODE2_PDN | CS4271_MODE2_CPEN,
 				 CS4271_MODE2_PDN | CS4271_MODE2_CPEN);
 	if (ret < 0)
-		return ret;
+		goto err_force_reset;
+
 	ret = regmap_update_bits(cs4271->regmap, CS4271_MODE2,
 				 CS4271_MODE2_PDN, 0);
 	if (ret < 0)
-		return ret;
+		goto err_force_reset;
+
 	/* Power-up sequence requires 85 uS */
 	udelay(85);
 
@@ -592,6 +608,13 @@ static int cs4271_component_probe(struct snd_soc_component *component)
 				   CS4271_MODE2_MUTECAEQUB);
 
 	return 0;
+
+err_force_reset:
+	gpiod_set_value(cs4271->reset, 1);
+	clk_disable_unprepare(cs4271->clk);
+err_disable_regulators:
+	regulator_bulk_disable(ARRAY_SIZE(cs4271->supplies), cs4271->supplies);
+	return ret;
 }
 
 static void cs4271_component_remove(struct snd_soc_component *component)
@@ -603,6 +626,7 @@ static void cs4271_component_remove(struct snd_soc_component *component)
 
 	regcache_mark_dirty(cs4271->regmap);
 	regulator_bulk_disable(ARRAY_SIZE(cs4271->supplies), cs4271->supplies);
+	clk_disable_unprepare(cs4271->clk);
 };
 
 static const struct snd_soc_component_driver soc_component_dev_cs4271 = {
@@ -637,6 +661,10 @@ static int cs4271_common_probe(struct device *dev,
 				     "error retrieving RESET GPIO\n");
 	gpiod_set_consumer_name(cs4271->reset, "CS4271 Reset");
 
+	cs4271->clk = devm_clk_get_optional(dev, "mclk");
+	if (IS_ERR(cs4271->clk))
+		return dev_err_probe(dev, PTR_ERR(cs4271->clk), "Failed to get mclk\n");
+
 	for (i = 0; i < ARRAY_SIZE(supply_names); i++)
 		cs4271->supplies[i].supply = supply_names[i];
 
-- 
2.51.0

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

[Alexander Sverdlin]

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.

Re: [PATCH 3/3] ASoC: cs4271: Add support for the external mclk

[Alexander Sverdlin]

Hello Herve,

On Thu, 2025-10-16 at 15:03 +0200, Herve Codina wrote:
> The mclk (master clock) of the cs4271 codec can be an input clock.
> 
> In this case the connected clock needs to be enabled outside of any
> audio stream. Indeed, this clock is needed for i2c communication.
> 
> Add support of this clock and enable it before the first i2c transfer.

what about suspend/resume, would it make sense to disable/enable the clock
there?

> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  sound/soc/codecs/cs4271.c | 34 +++++++++++++++++++++++++++++++---
>  1 file changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/soc/codecs/cs4271.c b/sound/soc/codecs/cs4271.c
> index ff9c6628224c..481a2c20b7cf 100644
> --- a/sound/soc/codecs/cs4271.c
> +++ b/sound/soc/codecs/cs4271.c
> @@ -10,6 +10,7 @@
>   * DAPM support not implemented.
>   */
>  
> +#include <linux/clk.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/delay.h>
> @@ -163,6 +164,7 @@ struct cs4271_private {
>  	/* enable soft reset workaround */
>  	bool				enable_soft_reset;
>  	struct regulator_bulk_data      supplies[ARRAY_SIZE(supply_names)];
> +	struct clk *clk;
>  };
>  
>  static const struct snd_soc_dapm_widget cs4271_dapm_widgets[] = {
> @@ -567,22 +569,36 @@ static int cs4271_component_probe(struct snd_soc_component *component)
>  		cs4271->enable_soft_reset = cs4271plat->enable_soft_reset;
>  	}
>  
> +	ret = clk_prepare_enable(cs4271->clk);
> +	if (ret) {
> +		dev_err(component->dev, "Failed to enable clk: %d\n", ret);
> +		goto err_disable_regulators;
> +	}
> +
> +	/*
> +	 * Be sure to have the clock and power-supplies stable before releasing
> +	 * the reset.
> +	 */

Would "if (cs4271->clk)" make sense for the fsleep() call?

> +	fsleep(1000);
> +
>  	/* Reset codec */
>  	cs4271_reset(component);
>  
>  	ret = regcache_sync(cs4271->regmap);
>  	if (ret < 0)
> -		return ret;
> +		goto err_force_reset;
>  
>  	ret = regmap_update_bits(cs4271->regmap, CS4271_MODE2,
>  				 CS4271_MODE2_PDN | CS4271_MODE2_CPEN,
>  				 CS4271_MODE2_PDN | CS4271_MODE2_CPEN);
>  	if (ret < 0)
> -		return ret;
> +		goto err_force_reset;
> +
>  	ret = regmap_update_bits(cs4271->regmap, CS4271_MODE2,
>  				 CS4271_MODE2_PDN, 0);
>  	if (ret < 0)
> -		return ret;
> +		goto err_force_reset;
> +
>  	/* Power-up sequence requires 85 uS */
>  	udelay(85);
>  
> @@ -592,6 +608,13 @@ static int cs4271_component_probe(struct snd_soc_component *component)
>  				   CS4271_MODE2_MUTECAEQUB);
>  
>  	return 0;
> +
> +err_force_reset:
> +	gpiod_set_value(cs4271->reset, 1);

Is the reset line assertion really required here?

> +	clk_disable_unprepare(cs4271->clk);
> +err_disable_regulators:
> +	regulator_bulk_disable(ARRAY_SIZE(cs4271->supplies), cs4271->supplies);

Would it make sense to fix the error path regarding regulators handling
in a separate patch (especially if you decide to extend suspend/resume)?

> +	return ret;
>  }
>  
>  static void cs4271_component_remove(struct snd_soc_component *component)
> @@ -603,6 +626,7 @@ static void cs4271_component_remove(struct snd_soc_component *component)
>  
>  	regcache_mark_dirty(cs4271->regmap);
>  	regulator_bulk_disable(ARRAY_SIZE(cs4271->supplies), cs4271->supplies);
> +	clk_disable_unprepare(cs4271->clk);
>  };
>  
>  static const struct snd_soc_component_driver soc_component_dev_cs4271 = {
> @@ -637,6 +661,10 @@ static int cs4271_common_probe(struct device *dev,
>  				     "error retrieving RESET GPIO\n");
>  	gpiod_set_consumer_name(cs4271->reset, "CS4271 Reset");
>  
> +	cs4271->clk = devm_clk_get_optional(dev, "mclk");
> +	if (IS_ERR(cs4271->clk))
> +		return dev_err_probe(dev, PTR_ERR(cs4271->clk), "Failed to get mclk\n");
> +
>  	for (i = 0; i < ARRAY_SIZE(supply_names); i++)
>  		cs4271->supplies[i].supply = supply_names[i];
>  

-- 
Alexander Sverdlin.

Re: [PATCH 3/3] ASoC: cs4271: Add support for the external mclk

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]

On Thu, Oct 16, 2025 at 03:03:39PM +0200, Herve Codina wrote:

> +	ret = clk_prepare_enable(cs4271->clk);
> +	if (ret) {
> +		dev_err(component->dev, "Failed to enable clk: %d\n", ret);
> +		goto err_disable_regulators;
> +	}
> +
> +	/*
> +	 * Be sure to have the clock and power-supplies stable before releasing
> +	 * the reset.
> +	 */
> +	fsleep(1000);

The regulator and clock drivers should be doing that.  Any sleep here
should come from the device's own requirements for waiting after they
become available.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Herve Codina]

Hi Alexander,

On Thu, 16 Oct 2025 20:40:34 +0200
Alexander Sverdlin <alexander.sverdlin@gmail.com> wrote:

...

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

Here is my uevent content:
--- 8<---
# cat /sys/bus/i2c/devices/3-0010/uevent 
DRIVER=cs4271
OF_NAME=cs4271
OF_FULLNAME=/i2c@ff130000/cs4271@10
OF_COMPATIBLE_0=cirrus,cs4271
OF_COMPATIBLE_N=1
MODALIAS=of:Ncs4271T(null)Ccirrus,cs4271
# 
--- 8< ---

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

And this is the related DT part:
--- 8< ---
&i2c3 {
	status = "okay";

	cs4271@10 {
		compatible = "cirrus,cs4271";
		reg = <0x10>;
		clocks = <&cru SCLK_I2S_8CH_OUT>;
		clock-names = "mclk";

		...
	};
};
--- 8< ---

i2c3 is the following node:
https://elixir.bootlin.com/linux/v6.17.1/source/arch/arm64/boot/dts/rockchip/rk3399-base.dtsi#L732

About the related module, I have the following:
--- 8< ---
# modinfo snd_soc_cs4271_i2c
filename:       /lib/modules/6.18.0-rc1xxxx-00050-g4fa36970abe5-dirty/kernel/sound/soc/codecs/snd-soc-cs4271-i2c.ko
license:        GPL
author:         Alexander Sverdlin <subaparts@yandex.ru>
description:    ASoC CS4271 I2C Driver
alias:          i2c:cs4271
alias:          of:N*T*Ccirrus,cs4271C*
alias:          of:N*T*Ccirrus,cs4271
depends:        snd-soc-cs4271
intree:         Y
name:           snd_soc_cs4271_i2c
vermagic:       6.18.0-rc1xxxx-00050-g4fa36970abe5-dirty SMP preempt mod_unload aarch64
# 
--- 8< ---

Best regards,
Hervé

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

[Alexander Sverdlin]

Hi Herve,

On Fri, 2025-10-17 at 08:32 +0200, Herve Codina wrote:
> ...
> 
> > > 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?
> 
> Here is my uevent content:
> --- 8<---
> # cat /sys/bus/i2c/devices/3-0010/uevent 
> DRIVER=cs4271
> OF_NAME=cs4271
> OF_FULLNAME=/i2c@ff130000/cs4271@10
> OF_COMPATIBLE_0=cirrus,cs4271
> OF_COMPATIBLE_N=1
> MODALIAS=of:Ncs4271T(null)Ccirrus,cs4271
> # 
> --- 8< ---

that's what I get with SPI-connected CS4271, and this is actually what I'd
expect (linux-next as of 2433b8476165):

# cat /sys/bus/spi/devices/spi0.0/uevent
DRIVER=cs4271
OF_NAME=codec
OF_FULLNAME=/soc/spi@808a0000/codec@0
OF_COMPATIBLE_0=cirrus,cs4271
OF_COMPATIBLE_N=1
MODALIAS=spi:cs4271

> > 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?
> 
> And this is the related DT part:
> --- 8< ---
> &i2c3 {
>  status = "okay";
> 
>  cs4271@10 {
>  compatible = "cirrus,cs4271";
>  reg = <0x10>;
>  clocks = <&cru SCLK_I2S_8CH_OUT>;
>  clock-names = "mclk";
> 
>  ...
>  };
> };
> --- 8< ---
> 
> i2c3 is the following node:
> https://elixir.bootlin.com/linux/v6.17.1/source/arch/arm64/boot/dts/rockchip/rk3399-base.dtsi#L732

the above looks OK to me on the first glance, I'm really puzzled what
is the reason for "of:" prefixed MODALIAS in the uevent for an i2c device.

I still believe, that the culprit is the creation of a platform device
from the DT for an i2c device.

I don't have any real I2C-connected CS4271, but I think I could fake one
in any DT just to verify how uevents would look like on my side.

> About the related module, I have the following:

I assume, this is with your patch applied though.

> --- 8< ---
> # modinfo snd_soc_cs4271_i2c
> filename:       /lib/modules/6.18.0-rc1xxxx-00050-g4fa36970abe5-dirty/kernel/sound/soc/codecs/snd-soc-cs4271-i2c.ko
> license:        GPL
> author:         Alexander Sverdlin <subaparts@yandex.ru>
> description:    ASoC CS4271 I2C Driver
> alias:          i2c:cs4271
> alias:          of:N*T*Ccirrus,cs4271C*
> alias:          of:N*T*Ccirrus,cs4271
> depends:        snd-soc-cs4271
> intree:         Y
> name:           snd_soc_cs4271_i2c
> vermagic:       6.18.0-rc1xxxx-00050-g4fa36970abe5-dirty SMP preempt mod_unload aarch64
> # 
> --- 8< ---

-- 
Alexander Sverdlin.

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

[Alexander Sverdlin]

Hi Herve,

On Fri, 2025-10-17 at 15:25 +0200, Alexander Sverdlin wrote:
> > > > 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?
> > 
> > Here is my uevent content:
> > --- 8<---
> > # cat /sys/bus/i2c/devices/3-0010/uevent 
> > DRIVER=cs4271
> > OF_NAME=cs4271
> > OF_FULLNAME=/i2c@ff130000/cs4271@10
> > OF_COMPATIBLE_0=cirrus,cs4271
> > OF_COMPATIBLE_N=1
> > MODALIAS=of:Ncs4271T(null)Ccirrus,cs4271
> > # 
> > --- 8< ---
> 
> that's what I get with SPI-connected CS4271, and this is actually what I'd
> expect (linux-next as of 2433b8476165):
> 
> # cat /sys/bus/spi/devices/spi0.0/uevent
> DRIVER=cs4271
> OF_NAME=codec
> OF_FULLNAME=/soc/spi@808a0000/codec@0
> OF_COMPATIBLE_0=cirrus,cs4271
> OF_COMPATIBLE_N=1
> MODALIAS=spi:cs4271
> 
> > > 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?
> > 
> > And this is the related DT part:
> > --- 8< ---
> > &i2c3 {
> >   status = "okay";
> > 
> >   cs4271@10 {
> >   compatible = "cirrus,cs4271";
> >   reg = <0x10>;
> >   clocks = <&cru SCLK_I2S_8CH_OUT>;
> >   clock-names = "mclk";
> > 
> >   ...
> >   };
> > };
> > --- 8< ---
> > 
> > i2c3 is the following node:
> > https://elixir.bootlin.com/linux/v6.17.1/source/arch/arm64/boot/dts/rockchip/rk3399-base.dtsi#L732
> 
> the above looks OK to me on the first glance, I'm really puzzled what
> is the reason for "of:" prefixed MODALIAS in the uevent for an i2c device.
> 
> I still believe, that the culprit is the creation of a platform device
> from the DT for an i2c device.
> 
> I don't have any real I2C-connected CS4271, but I think I could fake one
> in any DT just to verify how uevents would look like on my side.

indeed, that's what I've got for a fake I2C device:

# cat /sys/bus/i2c/devices/0-0010/uevent 
OF_NAME=cs4271
OF_FULLNAME=/soc/i2c@4000000/cs4271@10
OF_COMPATIBLE_0=cirrus,cs4271
OF_COMPATIBLE_N=1
MODALIAS=of:Ncs4271T(null)Ccirrus,cs4271

to me it looks like a bug somewhere in I2C core...

-- 
Alexander Sverdlin.

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 475 bytes --]

On Fri, Oct 17, 2025 at 04:41:53PM +0200, Alexander Sverdlin wrote:

> indeed, that's what I've got for a fake I2C device:

> # cat /sys/bus/i2c/devices/0-0010/uevent 
> OF_NAME=cs4271
> OF_FULLNAME=/soc/i2c@4000000/cs4271@10
> OF_COMPATIBLE_0=cirrus,cs4271
> OF_COMPATIBLE_N=1
> MODALIAS=of:Ncs4271T(null)Ccirrus,cs4271

> to me it looks like a bug somewhere in I2C core...

IIRC this has been round the loop a few times and whatever you do
something breaks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Herve Codina]

Hi Alexander,

On Fri, 17 Oct 2025 15:25:42 +0200
Alexander Sverdlin <alexander.sverdlin@gmail.com> wrote:

> Hi Herve,
> 
> On Fri, 2025-10-17 at 08:32 +0200, Herve Codina wrote:
> > ...
> >   
> > > > 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?  
> > 
> > Here is my uevent content:
> > --- 8<---
> > # cat /sys/bus/i2c/devices/3-0010/uevent 
> > DRIVER=cs4271
> > OF_NAME=cs4271
> > OF_FULLNAME=/i2c@ff130000/cs4271@10
> > OF_COMPATIBLE_0=cirrus,cs4271
> > OF_COMPATIBLE_N=1
> > MODALIAS=of:Ncs4271T(null)Ccirrus,cs4271
> > # 
> > --- 8< ---  
> 
> that's what I get with SPI-connected CS4271, and this is actually what I'd
> expect (linux-next as of 2433b8476165):
> 
> # cat /sys/bus/spi/devices/spi0.0/uevent
> DRIVER=cs4271
> OF_NAME=codec
> OF_FULLNAME=/soc/spi@808a0000/codec@0
> OF_COMPATIBLE_0=cirrus,cs4271
> OF_COMPATIBLE_N=1
> MODALIAS=spi:cs4271

So, this is without my patch applied.
I don't have any CS4271 connected on SPI bus to perform the same test
with my patch applied.

> 
> > > 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?  
> > 
> > And this is the related DT part:
> > --- 8< ---
> > &i2c3 {
> >  status = "okay";
> > 
> >  cs4271@10 {
> >  compatible = "cirrus,cs4271";
> >  reg = <0x10>;
> >  clocks = <&cru SCLK_I2S_8CH_OUT>;
> >  clock-names = "mclk";
> > 
> >  ...
> >  };
> > };
> > --- 8< ---
> > 
> > i2c3 is the following node:
> > https://elixir.bootlin.com/linux/v6.17.1/source/arch/arm64/boot/dts/rockchip/rk3399-base.dtsi#L732  
> 
> the above looks OK to me on the first glance, I'm really puzzled what
> is the reason for "of:" prefixed MODALIAS in the uevent for an i2c device.
> 
> I still believe, that the culprit is the creation of a platform device
> from the DT for an i2c device.
> 
> I don't have any real I2C-connected CS4271, but I think I could fake one
> in any DT just to verify how uevents would look like on my side.
> 
> > About the related module, I have the following:  
> 
> I assume, this is with your patch applied though.

Yes, exactly.

> 
> > --- 8< ---
> > # modinfo snd_soc_cs4271_i2c
> > filename:       /lib/modules/6.18.0-rc1xxxx-00050-g4fa36970abe5-dirty/kernel/sound/soc/codecs/snd-soc-cs4271-i2c.ko
> > license:        GPL
> > author:         Alexander Sverdlin <subaparts@yandex.ru>
> > description:    ASoC CS4271 I2C Driver
> > alias:          i2c:cs4271
> > alias:          of:N*T*Ccirrus,cs4271C*
> > alias:          of:N*T*Ccirrus,cs4271
> > depends:        snd-soc-cs4271
> > intree:         Y
> > name:           snd_soc_cs4271_i2c
> > vermagic:       6.18.0-rc1xxxx-00050-g4fa36970abe5-dirty SMP preempt mod_unload aarch64
> > # 
> > --- 8< ---  
> 

Best regards,
Hervé

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

[Alexander Sverdlin]

Hi Herve,

On Fri, 2025-10-17 at 17:10 +0200, Herve Codina wrote:
> > > > > 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?  
> > > 
> > > Here is my uevent content:
> > > --- 8<---
> > > # cat /sys/bus/i2c/devices/3-0010/uevent 
> > > DRIVER=cs4271
> > > OF_NAME=cs4271
> > > OF_FULLNAME=/i2c@ff130000/cs4271@10
> > > OF_COMPATIBLE_0=cirrus,cs4271
> > > OF_COMPATIBLE_N=1
> > > MODALIAS=of:Ncs4271T(null)Ccirrus,cs4271
> > > # 
> > > --- 8< ---  
> > 
> > that's what I get with SPI-connected CS4271, and this is actually what I'd
> > expect (linux-next as of 2433b8476165):
> > 
> > # cat /sys/bus/spi/devices/spi0.0/uevent
> > DRIVER=cs4271
> > OF_NAME=codec
> > OF_FULLNAME=/soc/spi@808a0000/codec@0
> > OF_COMPATIBLE_0=cirrus,cs4271
> > OF_COMPATIBLE_N=1
> > MODALIAS=spi:cs4271
> 
> So, this is without my patch applied.

this is the modalias of the device, it doesn't depend on your patch series.

I'd say that modalias for SPI device is correct but commit c973b8a7dc50
lacks MODULE_DEVICE_TABLE(spi, ...) in the driver.

I'd argue that I2C modalias is correct in the driver:

# modinfo snd-soc-cs4271-i2c
...
alias:          i2c:cs4271

But I still have to understand what happened to I2C core.

> I don't have any CS4271 connected on SPI bus to perform the same test
> with my patch applied.

-- 
Alexander Sverdlin.

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

[Herve Codina]

Hi Alexander,

On Fri, 17 Oct 2025 17:35:56 +0200
Alexander Sverdlin <alexander.sverdlin@gmail.com> wrote:

> Hi Herve,
> 
> On Fri, 2025-10-17 at 17:10 +0200, Herve Codina wrote:
> > > > > > 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?    
> > > > 
> > > > Here is my uevent content:
> > > > --- 8<---
> > > > # cat /sys/bus/i2c/devices/3-0010/uevent 
> > > > DRIVER=cs4271
> > > > OF_NAME=cs4271
> > > > OF_FULLNAME=/i2c@ff130000/cs4271@10
> > > > OF_COMPATIBLE_0=cirrus,cs4271
> > > > OF_COMPATIBLE_N=1
> > > > MODALIAS=of:Ncs4271T(null)Ccirrus,cs4271
> > > > # 
> > > > --- 8< ---    
> > > 
> > > that's what I get with SPI-connected CS4271, and this is actually what I'd
> > > expect (linux-next as of 2433b8476165):
> > > 
> > > # cat /sys/bus/spi/devices/spi0.0/uevent
> > > DRIVER=cs4271
> > > OF_NAME=codec
> > > OF_FULLNAME=/soc/spi@808a0000/codec@0
> > > OF_COMPATIBLE_0=cirrus,cs4271
> > > OF_COMPATIBLE_N=1
> > > MODALIAS=spi:cs4271  
> > 
> > So, this is without my patch applied.  
> 
> this is the modalias of the device, it doesn't depend on your patch series.
> 
> I'd say that modalias for SPI device is correct but commit c973b8a7dc50
> lacks MODULE_DEVICE_TABLE(spi, ...) in the driver.
> 
> I'd argue that I2C modalias is correct in the driver:
> 
> # modinfo snd-soc-cs4271-i2c
> ...
> alias:          i2c:cs4271
> 
> But I still have to understand what happened to I2C core.
> 
> > I don't have any CS4271 connected on SPI bus to perform the same test
> > with my patch applied.  
> 

In my next iteration, I will fix the MODULE_DEVICE_TABLE() in cs4271-spi.c
replacing
  MODULE_DEVICE_TABLE(of, cs4271_dt_ids);
by
  MODULE_DEVICE_TABLE(spi, cs4271_dt_ids);

For the moment, related to I2C, I keep MODULE_DEVICE_TABLE(of, cs4271_dt_ids)
in cs4271-i2c.c

Best regards,
Hervé

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

[Alexander Sverdlin]

Hi Mark, Wolfram and all,

On Fri, 2025-10-17 at 16:01 +0100, Mark Brown wrote:
> On Fri, Oct 17, 2025 at 04:41:53PM +0200, Alexander Sverdlin wrote:
> 
> > indeed, that's what I've got for a fake I2C device:
> 
> > # cat /sys/bus/i2c/devices/0-0010/uevent 
> > OF_NAME=cs4271
> > OF_FULLNAME=/soc/i2c@4000000/cs4271@10
> > OF_COMPATIBLE_0=cirrus,cs4271
> > OF_COMPATIBLE_N=1
> > MODALIAS=of:Ncs4271T(null)Ccirrus,cs4271
> 
> > to me it looks like a bug somewhere in I2C core...
> 
> IIRC this has been round the loop a few times and whatever you do
> something breaks.

it turns out it has been changed back in v4.18, with commit af503716ac14
("i2c: core: report OF style module alias for devices registered via OF"), 

but the change is not symmetric with SPI, and SPI core doesn't call 
of_device_uevent_modalias().

Not only is this inconsistent with SPI, but I anticipate yet another problem
if SPI would also adopt the same mechanism:

for instance there are many CODEC drivers in sound/soc, which have
SPI and I2C backends modelled as distinct modules.

In the current situation most of them have MODULE_DEVICE_TABLE(i2c,
and are therefore broken regarding module autoloading.

"Reparing" them as Herve proposed would result in I2C modules being
loaded only via "of:" style modalias and SPI still via "spi:". Which
sounds all but consistent.

If SPI ever adopts the same of_device_uevent_modalias(), both backends
would require "of:" prefixed modalias, and it will not be possible to
load the proper one for the corresponding bus type.

What are your thoughs on this?

-- 
Alexander Sverdlin.

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

[Herve Codina]

On Fri, 17 Oct 2025 18:03:13 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> Hi Alexander,
> 
> On Fri, 17 Oct 2025 17:35:56 +0200
> Alexander Sverdlin <alexander.sverdlin@gmail.com> wrote:
> 
> > Hi Herve,
> > 
> > On Fri, 2025-10-17 at 17:10 +0200, Herve Codina wrote:  
> > > > > > > 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?      
> > > > > 
> > > > > Here is my uevent content:
> > > > > --- 8<---
> > > > > # cat /sys/bus/i2c/devices/3-0010/uevent 
> > > > > DRIVER=cs4271
> > > > > OF_NAME=cs4271
> > > > > OF_FULLNAME=/i2c@ff130000/cs4271@10
> > > > > OF_COMPATIBLE_0=cirrus,cs4271
> > > > > OF_COMPATIBLE_N=1
> > > > > MODALIAS=of:Ncs4271T(null)Ccirrus,cs4271
> > > > > # 
> > > > > --- 8< ---      
> > > > 
> > > > that's what I get with SPI-connected CS4271, and this is actually what I'd
> > > > expect (linux-next as of 2433b8476165):
> > > > 
> > > > # cat /sys/bus/spi/devices/spi0.0/uevent
> > > > DRIVER=cs4271
> > > > OF_NAME=codec
> > > > OF_FULLNAME=/soc/spi@808a0000/codec@0
> > > > OF_COMPATIBLE_0=cirrus,cs4271
> > > > OF_COMPATIBLE_N=1
> > > > MODALIAS=spi:cs4271    
> > > 
> > > So, this is without my patch applied.    
> > 
> > this is the modalias of the device, it doesn't depend on your patch series.
> > 
> > I'd say that modalias for SPI device is correct but commit c973b8a7dc50
> > lacks MODULE_DEVICE_TABLE(spi, ...) in the driver.
> > 
> > I'd argue that I2C modalias is correct in the driver:
> > 
> > # modinfo snd-soc-cs4271-i2c
> > ...
> > alias:          i2c:cs4271
> > 
> > But I still have to understand what happened to I2C core.
> >   
> > > I don't have any CS4271 connected on SPI bus to perform the same test
> > > with my patch applied.    
> >   
> 
> In my next iteration, I will fix the MODULE_DEVICE_TABLE() in cs4271-spi.c
> replacing
>   MODULE_DEVICE_TABLE(of, cs4271_dt_ids);
> by
>   MODULE_DEVICE_TABLE(spi, cs4271_dt_ids);

I have the feeling that this was not correct.

Looking at several SPI driver, both
MODULE_DEVICE_TABLE(of, ...) and MODULE_DEVICE_TABLE(spi, ...) are present.

I should keep the MODULE_DEVICE_TABLE(of, cs4271_dt_ids) in the cs4271-spi.c
but a MODULE_DEVICE_TABLE(spi, ...) is missing.

I plan to add the following for the cs4271-spi.c file:
--- 8< ---
static const struct spi_device_id cs4271_spi_id[] = {
	{ "cs4271" },
	{ }
};
MODULE_DEVICE_TABLE(spi, cs4271_spi_id);

static struct spi_driver cs4271_spi_driver = {
	...
	.id_table = cs4271_spi_id,     /* <---- This was also missing */
};

--- 8< ---

Also no change on cs4271-i2c.c file as both MODULE_DEVICE_TABLE(i2c, ...)
and MODULE_DEVICE_TABLE(of, ...) are already present.

Looking at other drivers in the kernel for devices that can be available on
SPI or I2C busses, a lot of them have the same MODULE_DEVICE_TABLE(of, ...)
for the SPI and the I2C parts and a specific MODULE_DEVICE_TABLE(spi, ...)
for the SPI part or MODULE_DEVICE_TABLE(i2c, ...) for the I2C part.

My next iteration will be consistent with this pattern.
- cs4271-spi.c:
    The cs4271_spi_id table contains { "cs4271" }
    The cs4271_dt_ids table contains { .compatible = "cirrus,cs4271" }
    
    MODULE_DEVICE_TABLE(spi, cs4271_spi_id);
    MODULE_DEVICE_TABLE(of, cs4271_dt_ids);

- cs4271-i2c.c
    The cs4271_i2c_id table contains { "cs4271" }
    The cs4271_dt_ids table contains { .compatible = "cirrus,cs4271" }

    MODULE_DEVICE_TABLE(i2c, cs4271_i2c_id);
    MODULE_DEVICE_TABLE(of, cs4271_dt_ids);

Best regards,
Hervé

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 787 bytes --]

On Fri, Oct 17, 2025 at 08:14:43PM +0200, Alexander Sverdlin wrote:

> "Reparing" them as Herve proposed would result in I2C modules being
> loaded only via "of:" style modalias and SPI still via "spi:". Which
> sounds all but consistent.

> If SPI ever adopts the same of_device_uevent_modalias(), both backends
> would require "of:" prefixed modalias, and it will not be possible to
> load the proper one for the corresponding bus type.

> What are your thoughs on this?

Or at least you'd get both modules loaded with one being redundant.  TBH
I'm very reluctant to touch this stuff for SPI without some very careful
analysis that it's not going to cause things to explode on people, right
now things seem to be working well enough so I'm not clear we'd be
solving an actual problem.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Alexander Sverdlin]

Hi Mark!

On Tue, 2025-10-21 at 20:00 +0100, Mark Brown wrote:
> On Fri, Oct 17, 2025 at 08:14:43PM +0200, Alexander Sverdlin wrote:
> 
> > "Reparing" them as Herve proposed would result in I2C modules being
> > loaded only via "of:" style modalias and SPI still via "spi:". Which
> > sounds all but consistent.
> 
> > If SPI ever adopts the same of_device_uevent_modalias(), both backends
> > would require "of:" prefixed modalias, and it will not be possible to
> > load the proper one for the corresponding bus type.
> 
> > What are your thoughs on this?
> 
> Or at least you'd get both modules loaded with one being redundant.  TBH

I'm quite confident that udev/modprobe will load only the first module
from modules.alias file.

> I'm very reluctant to touch this stuff for SPI without some very careful
> analysis that it's not going to cause things to explode on people, right
> now things seem to be working well enough so I'm not clear we'd be
> solving an actual problem.

The actual problem is that i2c-core is producing "of:" prefixed uevents
instead of "i2c:" prefixed uevents starting from v4.18.

Most of the dual-bus ASoC CODECs are affected.

Now declaring "of:" to be the new I2C bus prefix for uevents starting from
Linux v4.18 sounds strange.

-- 
Alexander Sverdlin.

Re: [PATCH 2/3] ASoC: dt-bindings: cirrus,cs4271: Document mclk clock

[Krzysztof Kozlowski]

On Thu, Oct 16, 2025 at 03:03:38PM +0200, Herve Codina wrote:
> The Cirrus CS4271 codec can use an external clock as an input Master
> Clock. When no crystal is used, the CS4271 component considers its MCLK
> pin as an input pin and expects the external clock connected to provide
> the Master Clock.
> 
> This clock is not documented in the binding.
> 
> Add the missing clock.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  .../devicetree/bindings/sound/cirrus,cs4271.yaml       | 10 ++++++++++
>  1 file changed, 10 insertions(+)

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1237 bytes --]

On Tue, Oct 21, 2025 at 09:12:13PM +0200, Alexander Sverdlin wrote:
> On Tue, 2025-10-21 at 20:00 +0100, Mark Brown wrote:

> > Or at least you'd get both modules loaded with one being redundant.  TBH

> I'm quite confident that udev/modprobe will load only the first module
> from modules.alias file.

Oh, I'm sure that's the case now but I can see someone changing that.

> > I'm very reluctant to touch this stuff for SPI without some very careful
> > analysis that it's not going to cause things to explode on people, right
> > now things seem to be working well enough so I'm not clear we'd be
> > solving an actual problem.

> The actual problem is that i2c-core is producing "of:" prefixed uevents
> instead of "i2c:" prefixed uevents starting from v4.18.

> Most of the dual-bus ASoC CODECs are affected.

That's a description of what change but not of a concrete problem that
users are experiencing.

> Now declaring "of:" to be the new I2C bus prefix for uevents starting from
> Linux v4.18 sounds strange.

I think a robust solution would involve having the OF aliases namespaced
by bus, or just not using the OF aliases but potentially having
collisions if two vendors pick the same device name.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Alexander Sverdlin]

Hi Mark,

On Wed, 2025-10-22 at 15:56 +0100, Mark Brown wrote:
> > > I'm very reluctant to touch this stuff for SPI without some very careful
> > > analysis that it's not going to cause things to explode on people, right
> > > now things seem to be working well enough so I'm not clear we'd be
> > > solving an actual problem.
> 
> > The actual problem is that i2c-core is producing "of:" prefixed uevents
> > instead of "i2c:" prefixed uevents starting from v4.18.
> 
> > Most of the dual-bus ASoC CODECs are affected.
> 
> That's a description of what change but not of a concrete problem that
> users are experiencing.

the concrete problem Herve has experienced is that cs4271-i2c will not be
loaded automatically starting with Linux v4.18 (commit af503716ac14
"i2c: core: report OF style module alias for devices registered via OF").

> > Now declaring "of:" to be the new I2C bus prefix for uevents starting from
> > Linux v4.18 sounds strange.
> 
> I think a robust solution would involve having the OF aliases namespaced
> by bus, or just not using the OF aliases but potentially having
> collisions if two vendors pick the same device name.

But this sounds like the situation before the above mentioned commit
af503716ac14, when both i2c and spi were symmetrically namespaced with
i2c: and spi: respectively and contained the "compatible" stripped of the
vendor prefix.

And I must admit that I had more understanding for the prior state of things.

-- 
Alexander Sverdlin.

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1233 bytes --]

On Wed, Oct 22, 2025 at 07:46:26PM +0200, Alexander Sverdlin wrote:
> On Wed, 2025-10-22 at 15:56 +0100, Mark Brown wrote:

> > > Now declaring "of:" to be the new I2C bus prefix for uevents starting from
> > > Linux v4.18 sounds strange.

> > I think a robust solution would involve having the OF aliases namespaced
> > by bus, or just not using the OF aliases but potentially having
> > collisions if two vendors pick the same device name.

> But this sounds like the situation before the above mentioned commit
> af503716ac14, when both i2c and spi were symmetrically namespaced with
> i2c: and spi: respectively and contained the "compatible" stripped of the
> vendor prefix.

> And I must admit that I had more understanding for the prior state of things.

Right, it is.  The issue with this situation is that if the compatible
strings for a device are not simply vendor,part or if two vendors happen
to end up with the same part name (Wondermedia and Wolfson used to both
use wmNNNN for example, there were actual collisions though not on the
same bus type) we will have trouble figuring things out.  We don't have
a robust mechanism for translating from a compatible string to a
platform registration style name for a device.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Javier Martinez Canillas]

Alexander Sverdlin <alexander.sverdlin@gmail.com> writes:

Hello Alexander,

> Hi Mark,
>
> On Wed, 2025-10-22 at 15:56 +0100, Mark Brown wrote:
>> > > I'm very reluctant to touch this stuff for SPI without some very careful
>> > > analysis that it's not going to cause things to explode on people, right
>> > > now things seem to be working well enough so I'm not clear we'd be
>> > > solving an actual problem.
>> 
>> > The actual problem is that i2c-core is producing "of:" prefixed uevents
>> > instead of "i2c:" prefixed uevents starting from v4.18.
>> 
>> > Most of the dual-bus ASoC CODECs are affected.
>> 
>> That's a description of what change but not of a concrete problem that
>> users are experiencing.
>
> the concrete problem Herve has experienced is that cs4271-i2c will not be
> loaded automatically starting with Linux v4.18 (commit af503716ac14
> "i2c: core: report OF style module alias for devices registered via OF").
>
>> > Now declaring "of:" to be the new I2C bus prefix for uevents starting from
>> > Linux v4.18 sounds strange.
>>

I don't find that strange at all. My opinion is that is the correct
thing to do for the following reasons:

* The struct of_device_id table (and not the struct i2c_device_id table)
  is used to match registered devices through DT / OF with I2C drivers.

* All other bus types but SPI report an MODALIAS=of: for devices that
  are registered through OF.

* I2C (and even SPI) devices registered by ACPI report a MODALIAS=acpi:
  and not a MODALIAS=i2c: or MODALIAS=spi:.

So I would claim that I2C reporting MODALIAS=of: when devices are 
registered through OF are consistent with other buses, using the same
data to both load modules and match drivers and also more consistent
on how the I2C subsystem handles registration through ACPI, OF and pdata.

Unfortunately the DT support in SPI was not complete at the time, and I
don't think it can't be changed at this time without breaking something
as Mark correctly said.

I fixed a lot of I2C drivers and DTS when doing the I2C converstion and
even with that some regressions were introduced like the one you report.

>> I think a robust solution would involve having the OF aliases namespaced
>> by bus, or just not using the OF aliases but potentially having
>> collisions if two vendors pick the same device name.
>
> But this sounds like the situation before the above mentioned commit
> af503716ac14, when both i2c and spi were symmetrically namespaced with
> i2c: and spi: respectively and contained the "compatible" stripped of the
> vendor prefix.
>

Is not the same for the reasons I mentioned above. What Mark suggests is
to encode the bus type information in the OF compatible string, while still
being consistent about the table used to report modaliases and match devices.

Maybe we could have something like the following (not much tested) patch ?

From b00f5914606fb72a5f7bdb38e63d109264261dee Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <javierm@redhat.com>
Date: Thu, 23 Oct 2025 13:32:04 +0200
Subject: [PATCH RFC] of: Report the bus type in module alias type sub-field

The modaliases for devices registered through Device Trees don't have any
information about the bus of the device. For example, an I2C device has:

$ cat /sys/devices/platform/soc/fe804000.i2c/i2c-1/1-003c/uevent
DRIVER=ssd130x-i2c
OF_NAME=oled
OF_FULLNAME=/soc/i2c at 7e804000/oled at 3c
OF_COMPATIBLE_0=solomon,ssd1306fb-i2c
OF_COMPATIBLE_N=1
MODALIAS=of:NoledT(null)Csolomon,ssd1306fb-i2c

$ modinfo ssd130x-i2c | grep alias
alias:          of:N*T*Csolomon,ssd1309fb-i2cC*
alias:          of:N*T*Csolomon,ssd1309fb-i2c
alias:          of:N*T*Csolomon,ssd1307fb-i2cC*
alias:          of:N*T*Csolomon,ssd1307fb-i2c
alias:          of:N*T*Csolomon,ssd1306fb-i2cC*
alias:          of:N*T*Csolomon,ssd1306fb-i2c
alias:          of:N*T*Csolomon,ssd1305fb-i2cC*
alias:          of:N*T*Csolomon,ssd1305fb-i2c
alias:          of:N*T*Csinowealth,sh1106-i2cC*
alias:          of:N*T*Csinowealth,sh1106-i2c

The module aliases and compatible string have the bus (-i2c) as suffix to
denote that is a driver for a device that can be accessed through I2C.

This is done to prevent disambiguate in the case that the same device can
be accessed through another bus (i.e: SPI) and have a different driver.

To prevent this and allow to use the same compatible string for the same
device regardless of the bus type used, let's add information about the
bus type in the devide type module aliases sub-field that are reported to
user-space. The same device then will report something like following:

$ cat /sys/devices/platform/soc/fe804000.i2c/i2c-1/1-003c/uevent
DRIVER=ssd130x-i2c
OF_NAME=oled
OF_FULLNAME=/soc/i2c at 7e804000/oled at 3c
OF_COMPATIBLE_0=solomon,ssd1306fb-i2c
OF_COMPATIBLE_N=1
OF_TYPE=i2c
MODALIAS=of:NoledTi2cCsolomon,ssd1306fb-i2c

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/of/device.c | 6 ++++--
 drivers/of/module.c | 8 ++++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index f7e75e527667..4187decc2873 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -225,8 +225,10 @@ void of_device_uevent(const struct device *dev, struct kobj_uevent_env *env)
 	add_uevent_var(env, "OF_NAME=%pOFn", dev->of_node);
 	add_uevent_var(env, "OF_FULLNAME=%pOF", dev->of_node);
 	type = of_node_get_device_type(dev->of_node);
-	if (type)
-		add_uevent_var(env, "OF_TYPE=%s", type);
+	if (!type)
+		type = dev_bus_name(dev);
+
+	add_uevent_var(env, "OF_TYPE=%s", type);
 
 	/* Since the compatible field can contain pretty much anything
 	 * it's not really legal to split it out with commas. We split it
diff --git a/drivers/of/module.c b/drivers/of/module.c
index 1e735fc130ad..f22ddc83ef40 100644
--- a/drivers/of/module.c
+++ b/drivers/of/module.c
@@ -11,6 +11,7 @@
 ssize_t of_modalias(const struct device_node *np, char *str, ssize_t len)
 {
 	const char *compat;
+	const char *type;
 	char *c;
 	struct property *p;
 	ssize_t csize;
@@ -24,10 +25,13 @@ ssize_t of_modalias(const struct device_node *np, char *str, ssize_t len)
 	if ((len > 0 && !str) || len < 0)
 		return -EINVAL;
 
+	type = of_node_get_device_type(dev->of_node);
+	if (!type)
+		type = dev_bus_name(dev);
+
 	/* Name & Type */
 	/* %p eats all alphanum characters, so %c must be used here */
-	csize = snprintf(str, len, "of:N%pOFn%c%s", np, 'T',
-			 of_node_get_device_type(np));
+	csize = snprintf(str, len, "of:N%pOFn%c%s", np, 'T', type);
 	tsize = csize;
 	if (csize >= len)
 		csize = len > 0 ? len - 1 : 0;

base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787
-- 
2.51.0

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

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

[Alexander Sverdlin]

Hi Javier, DT guys,

On Thu, 2025-10-23 at 13:40 +0200, Javier Martinez Canillas wrote:
> > On Wed, 2025-10-22 at 15:56 +0100, Mark Brown wrote:
> > > > > I'm very reluctant to touch this stuff for SPI without some very careful
> > > > > analysis that it's not going to cause things to explode on people, right
> > > > > now things seem to be working well enough so I'm not clear we'd be
> > > > > solving an actual problem.
> > > 
> > > > The actual problem is that i2c-core is producing "of:" prefixed uevents
> > > > instead of "i2c:" prefixed uevents starting from v4.18.
> > > 
> > > > Most of the dual-bus ASoC CODECs are affected.
> > > 
> > > That's a description of what change but not of a concrete problem that
> > > users are experiencing.
> > 
> > the concrete problem Herve has experienced is that cs4271-i2c will not be
> > loaded automatically starting with Linux v4.18 (commit af503716ac14
> > "i2c: core: report OF style module alias for devices registered via OF").
> > 
> > > > Now declaring "of:" to be the new I2C bus prefix for uevents starting from
> > > > Linux v4.18 sounds strange.
> > > 
> 
> I don't find that strange at all. My opinion is that is the correct
> thing to do for the following reasons:
> 
> * The struct of_device_id table (and not the struct i2c_device_id table)
>   is used to match registered devices through DT / OF with I2C drivers.
> 
> * All other bus types but SPI report an MODALIAS=of: for devices that
>   are registered through OF.
> 
> * I2C (and even SPI) devices registered by ACPI report a MODALIAS=acpi:
>   and not a MODALIAS=i2c: or MODALIAS=spi:.
> 
> So I would claim that I2C reporting MODALIAS=of: when devices are 
> registered through OF are consistent with other buses, using the same
> data to both load modules and match drivers and also more consistent
> on how the I2C subsystem handles registration through ACPI, OF and pdata.
> 
> Unfortunately the DT support in SPI was not complete at the time, and I
> don't think it can't be changed at this time without breaking something
> as Mark correctly said.
> 
> I fixed a lot of I2C drivers and DTS when doing the I2C converstion and
> even with that some regressions were introduced like the one you report.
> 
> > > I think a robust solution would involve having the OF aliases namespaced
> > > by bus, or just not using the OF aliases but potentially having
> > > collisions if two vendors pick the same device name.
> > 
> > But this sounds like the situation before the above mentioned commit
> > af503716ac14, when both i2c and spi were symmetrically namespaced with
> > i2c: and spi: respectively and contained the "compatible" stripped of the
> > vendor prefix.
> > 
> 
> Is not the same for the reasons I mentioned above. What Mark suggests is
> to encode the bus type information in the OF compatible string, while still
> being consistent about the table used to report modaliases and match devices.
> 
> Maybe we could have something like the following (not much tested) patch ?
> 
> From b00f5914606fb72a5f7bdb38e63d109264261dee Mon Sep 17 00:00:00 2001
> From: Javier Martinez Canillas <javierm@redhat.com>
> Date: Thu, 23 Oct 2025 13:32:04 +0200
> Subject: [PATCH RFC] of: Report the bus type in module alias type sub-field
> 
> The modaliases for devices registered through Device Trees don't have any
> information about the bus of the device. For example, an I2C device has:
> 
> $ cat /sys/devices/platform/soc/fe804000.i2c/i2c-1/1-003c/uevent
> DRIVER=ssd130x-i2c
> OF_NAME=oled
> OF_FULLNAME=/soc/i2c at 7e804000/oled at 3c
> OF_COMPATIBLE_0=solomon,ssd1306fb-i2c
> OF_COMPATIBLE_N=1
> MODALIAS=of:NoledT(null)Csolomon,ssd1306fb-i2c
> 
> $ modinfo ssd130x-i2c | grep alias
> alias:          of:N*T*Csolomon,ssd1309fb-i2cC*
> alias:          of:N*T*Csolomon,ssd1309fb-i2c
> alias:          of:N*T*Csolomon,ssd1307fb-i2cC*
> alias:          of:N*T*Csolomon,ssd1307fb-i2c
> alias:          of:N*T*Csolomon,ssd1306fb-i2cC*
> alias:          of:N*T*Csolomon,ssd1306fb-i2c
> alias:          of:N*T*Csolomon,ssd1305fb-i2cC*
> alias:          of:N*T*Csolomon,ssd1305fb-i2c
> alias:          of:N*T*Csinowealth,sh1106-i2cC*
> alias:          of:N*T*Csinowealth,sh1106-i2c
> 
> The module aliases and compatible string have the bus (-i2c) as suffix to
> denote that is a driver for a device that can be accessed through I2C.
> 
> This is done to prevent disambiguate in the case that the same device can
> be accessed through another bus (i.e: SPI) and have a different driver.
> 
> To prevent this and allow to use the same compatible string for the same
> device regardless of the bus type used, let's add information about the
> bus type in the devide type module aliases sub-field that are reported to
> user-space. The same device then will report something like following:
> 
> $ cat /sys/devices/platform/soc/fe804000.i2c/i2c-1/1-003c/uevent
> DRIVER=ssd130x-i2c
> OF_NAME=oled
> OF_FULLNAME=/soc/i2c at 7e804000/oled at 3c
> OF_COMPATIBLE_0=solomon,ssd1306fb-i2c
> OF_COMPATIBLE_N=1
> OF_TYPE=i2c
> MODALIAS=of:NoledTi2cCsolomon,ssd1306fb-i2c
> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>  drivers/of/device.c | 6 ++++--
>  drivers/of/module.c | 8 ++++++--
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index f7e75e527667..4187decc2873 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -225,8 +225,10 @@ void of_device_uevent(const struct device *dev, struct kobj_uevent_env *env)
>  	add_uevent_var(env, "OF_NAME=%pOFn", dev->of_node);
>  	add_uevent_var(env, "OF_FULLNAME=%pOF", dev->of_node);
>  	type = of_node_get_device_type(dev->of_node);
> -	if (type)
> -		add_uevent_var(env, "OF_TYPE=%s", type);
> +	if (!type)
> +		type = dev_bus_name(dev);
> +
> +	add_uevent_var(env, "OF_TYPE=%s", type);
>  
>  	/* Since the compatible field can contain pretty much anything
>  	 * it's not really legal to split it out with commas. We split it
> diff --git a/drivers/of/module.c b/drivers/of/module.c
> index 1e735fc130ad..f22ddc83ef40 100644
> --- a/drivers/of/module.c
> +++ b/drivers/of/module.c
> @@ -11,6 +11,7 @@
>  ssize_t of_modalias(const struct device_node *np, char *str, ssize_t len)
>  {
>  	const char *compat;
> +	const char *type;
>  	char *c;
>  	struct property *p;
>  	ssize_t csize;
> @@ -24,10 +25,13 @@ ssize_t of_modalias(const struct device_node *np, char *str, ssize_t len)
>  	if ((len > 0 && !str) || len < 0)
>  		return -EINVAL;
>  
> +	type = of_node_get_device_type(dev->of_node);
> +	if (!type)
> +		type = dev_bus_name(dev);
> +
>  	/* Name & Type */
>  	/* %p eats all alphanum characters, so %c must be used here */
> -	csize = snprintf(str, len, "of:N%pOFn%c%s", np, 'T',
> -			 of_node_get_device_type(np));
> +	csize = snprintf(str, len, "of:N%pOFn%c%s", np, 'T', type);
>  	tsize = csize;
>  	if (csize >= len)
>  		csize = len > 0 ? len - 1 : 0;
> 
> base-commit: 3a8660878839faadb4f1a6dd72c3179c1df56787

to me the patch looks promising, it would both solve the ambiguity with
modules and avoid having several compatible strings per device, with individual suffixes
per interface (bus), similar to the above solomon,ssd1306fb-i2c example.

Let's see how DT maintainers react on this, because I have an impression that
everything except "cpu" and "memory" is discouraged in device-type (even though
these shall never appear in live device trees, but people would probably try
to copy paste the values from modalias back into dts ;-)

There are 134 counterexamples of device-type = "pci" under Documentation/devicetree/bindings
in the current kernel though. Which is just another bus, like i2c and spi.

-- 
Alexander Sverdlin.