Message-ID: <b1fbce9d07df3daa8657f56d254d279aa784cffa.camel@gmail.com>
Date: Thu, 23 Oct 2025 14:32:34 +0200
From: Alexander Sverdlin <alexander.sverdlin@...il.com>
To: Javier Martinez Canillas <javierm@...hat.com>, Mark Brown
	 <broonie@...nel.org>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
	 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>
Cc: Wolfram Sang <wsa@...-dreams.de>, Herve Codina
 <herve.codina@...tlin.com>,  David Rhodes <david.rhodes@...rus.com>,
 Richard Fitzgerald <rf@...nsource.cirrus.com>, Liam Girdwood	
 <lgirdwood@...il.com>, Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai	
 <tiwai@...e.com>, Nikita Shubin <nikita.shubin@...uefel.me>, Axel Lin	
 <axel.lin@...ics.com>, Brian Austin <brian.austin@...rus.com>, 
	linux-sound@...r.kernel.org, patches@...nsource.cirrus.com, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, Thomas Petazzoni	
 <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH 1/3] ASoC: cs4271: Fix cs4271 I2C and SPI drivers
 automatic module loading

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

Powered by blists - more mailing lists