[PATCH] drm/bridge: ti-sn65dsi86: Fix multiple instances

[PATCH] drm/bridge: ti-sn65dsi86: Fix multiple instances

[Geert Uytterhoeven]

Each bridge instance creates up to four auxiliary devices with different
names.  However, their IDs are always zero, causing duplicate filename
errors when a system has multiple bridges:

    sysfs: cannot create duplicate filename '/bus/auxiliary/devices/ti_sn65dsi86.gpio.0'

Fix this by using a unique instance ID per bridge instance.

Fixes: bf73537f411b0d4f ("drm/bridge: ti-sn65dsi86: Break GPIO and MIPI-to-eDP bridge into sub-drivers")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
/sys/bus/auxiliary/devices
├── ti_sn65dsi86.gpio.0
├── ti_sn65dsi86.pwm.0
├── ti_sn65dsi86.aux.0
├── ti_sn65dsi86.bridge.0
├── ti_sn65dsi86.gpio.1
├── ti_sn65dsi86.pwm.1
├── ti_sn65dsi86.aux.1
└── ti_sn65dsi86.bridge.1
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 9e31f750fd889745..8f6ac48aefdb70b3 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -13,6 +13,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
 #include <linux/i2c.h>
+#include <linux/idr.h>
 #include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/of_graph.h>
@@ -168,6 +169,7 @@
  * @pwm_enabled:  Used to track if the PWM signal is currently enabled.
  * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM.
  * @pwm_refclk_freq: Cache for the reference clock input to the PWM.
+ * @id:           Unique instance ID
  */
 struct ti_sn65dsi86 {
 	struct auxiliary_device		*bridge_aux;
@@ -202,8 +204,11 @@ struct ti_sn65dsi86 {
 	atomic_t			pwm_pin_busy;
 #endif
 	unsigned int			pwm_refclk_freq;
+	int				id;
 };
 
+static DEFINE_IDA(ti_sn65dsi86_ida);
+
 static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = {
 	{ .range_min = 0, .range_max = 0xFF },
 };
@@ -488,6 +493,7 @@ static int ti_sn65dsi86_add_aux_device(struct ti_sn65dsi86 *pdata,
 		return -ENOMEM;
 
 	aux->name = name;
+	aux->id = pdata->id;
 	aux->dev.parent = dev;
 	aux->dev.release = ti_sn65dsi86_aux_device_release;
 	device_set_of_node_from_dev(&aux->dev, dev);
@@ -1889,6 +1895,13 @@ static int ti_sn65dsi86_parse_regulators(struct ti_sn65dsi86 *pdata)
 				       pdata->supplies);
 }
 
+static void ti_sn65dsi86_devm_ida_free(void *data)
+{
+	struct ti_sn65dsi86 *pdata = data;
+
+	ida_free(&ti_sn65dsi86_ida, pdata->id);
+}
+
 static int ti_sn65dsi86_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
@@ -1903,6 +1916,17 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 	pdata = devm_kzalloc(dev, sizeof(struct ti_sn65dsi86), GFP_KERNEL);
 	if (!pdata)
 		return -ENOMEM;
+
+	ret = ida_alloc(&ti_sn65dsi86_ida, GFP_KERNEL);
+	if (ret < 0)
+		return ret;
+
+	pdata->id = ret;
+
+	ret = devm_add_action_or_reset(dev, ti_sn65dsi86_devm_ida_free, pdata);
+	if (ret)
+		return ret;
+
 	dev_set_drvdata(dev, pdata);
 	pdata->dev = dev;
 
-- 
2.34.1

Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix multiple instances

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Fri, Oct 18, 2024 at 09:45:52AM +0200, Geert Uytterhoeven wrote:
> Each bridge instance creates up to four auxiliary devices with different
> names.  However, their IDs are always zero, causing duplicate filename
> errors when a system has multiple bridges:
> 
>     sysfs: cannot create duplicate filename '/bus/auxiliary/devices/ti_sn65dsi86.gpio.0'
> 
> Fix this by using a unique instance ID per bridge instance.

Isn't this something that should be handled by the AUX core ? The code
below would otherwise need to be duplicated by all drivers, which seems
a burden we should avoid.

> Fixes: bf73537f411b0d4f ("drm/bridge: ti-sn65dsi86: Break GPIO and MIPI-to-eDP bridge into sub-drivers")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> /sys/bus/auxiliary/devices
> ├── ti_sn65dsi86.gpio.0
> ├── ti_sn65dsi86.pwm.0
> ├── ti_sn65dsi86.aux.0
> ├── ti_sn65dsi86.bridge.0
> ├── ti_sn65dsi86.gpio.1
> ├── ti_sn65dsi86.pwm.1
> ├── ti_sn65dsi86.aux.1
> └── ti_sn65dsi86.bridge.1
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 9e31f750fd889745..8f6ac48aefdb70b3 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -13,6 +13,7 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/gpio/driver.h>
>  #include <linux/i2c.h>
> +#include <linux/idr.h>
>  #include <linux/iopoll.h>
>  #include <linux/module.h>
>  #include <linux/of_graph.h>
> @@ -168,6 +169,7 @@
>   * @pwm_enabled:  Used to track if the PWM signal is currently enabled.
>   * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM.
>   * @pwm_refclk_freq: Cache for the reference clock input to the PWM.
> + * @id:           Unique instance ID
>   */
>  struct ti_sn65dsi86 {
>  	struct auxiliary_device		*bridge_aux;
> @@ -202,8 +204,11 @@ struct ti_sn65dsi86 {
>  	atomic_t			pwm_pin_busy;
>  #endif
>  	unsigned int			pwm_refclk_freq;
> +	int				id;
>  };
>  
> +static DEFINE_IDA(ti_sn65dsi86_ida);
> +
>  static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = {
>  	{ .range_min = 0, .range_max = 0xFF },
>  };
> @@ -488,6 +493,7 @@ static int ti_sn65dsi86_add_aux_device(struct ti_sn65dsi86 *pdata,
>  		return -ENOMEM;
>  
>  	aux->name = name;
> +	aux->id = pdata->id;
>  	aux->dev.parent = dev;
>  	aux->dev.release = ti_sn65dsi86_aux_device_release;
>  	device_set_of_node_from_dev(&aux->dev, dev);
> @@ -1889,6 +1895,13 @@ static int ti_sn65dsi86_parse_regulators(struct ti_sn65dsi86 *pdata)
>  				       pdata->supplies);
>  }
>  
> +static void ti_sn65dsi86_devm_ida_free(void *data)
> +{
> +	struct ti_sn65dsi86 *pdata = data;
> +
> +	ida_free(&ti_sn65dsi86_ida, pdata->id);
> +}
> +
>  static int ti_sn65dsi86_probe(struct i2c_client *client)
>  {
>  	struct device *dev = &client->dev;
> @@ -1903,6 +1916,17 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
>  	pdata = devm_kzalloc(dev, sizeof(struct ti_sn65dsi86), GFP_KERNEL);
>  	if (!pdata)
>  		return -ENOMEM;
> +
> +	ret = ida_alloc(&ti_sn65dsi86_ida, GFP_KERNEL);
> +	if (ret < 0)
> +		return ret;
> +
> +	pdata->id = ret;
> +
> +	ret = devm_add_action_or_reset(dev, ti_sn65dsi86_devm_ida_free, pdata);
> +	if (ret)
> +		return ret;
> +
>  	dev_set_drvdata(dev, pdata);
>  	pdata->dev = dev;
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix multiple instances

[Geert Uytterhoeven]

Hi Laurent,

CC GregKH

On Fri, Oct 18, 2024 at 3:10 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Fri, Oct 18, 2024 at 09:45:52AM +0200, Geert Uytterhoeven wrote:
> > Each bridge instance creates up to four auxiliary devices with different
> > names.  However, their IDs are always zero, causing duplicate filename
> > errors when a system has multiple bridges:
> >
> >     sysfs: cannot create duplicate filename '/bus/auxiliary/devices/ti_sn65dsi86.gpio.0'
> >
> > Fix this by using a unique instance ID per bridge instance.
>
> Isn't this something that should be handled by the AUX core ? The code
> below would otherwise need to be duplicated by all drivers, which seems
> a burden we should avoid.

According to the documentation, this is the responsibility of the caller
https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L81
I believe this is the same for platform devices.
See also the example at
https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L116

Note: the platform bus supports PLATFORM_DEVID_AUTO, but the auxiliary
bus does not.

> > Fixes: bf73537f411b0d4f ("drm/bridge: ti-sn65dsi86: Break GPIO and MIPI-to-eDP bridge into sub-drivers")
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > /sys/bus/auxiliary/devices
> > ├── ti_sn65dsi86.gpio.0
> > ├── ti_sn65dsi86.pwm.0
> > ├── ti_sn65dsi86.aux.0
> > ├── ti_sn65dsi86.bridge.0
> > ├── ti_sn65dsi86.gpio.1
> > ├── ti_sn65dsi86.pwm.1
> > ├── ti_sn65dsi86.aux.1
> > └── ti_sn65dsi86.bridge.1
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 9e31f750fd889745..8f6ac48aefdb70b3 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/gpio/driver.h>
> >  #include <linux/i2c.h>
> > +#include <linux/idr.h>
> >  #include <linux/iopoll.h>
> >  #include <linux/module.h>
> >  #include <linux/of_graph.h>
> > @@ -168,6 +169,7 @@
> >   * @pwm_enabled:  Used to track if the PWM signal is currently enabled.
> >   * @pwm_pin_busy: Track if GPIO4 is currently requested for GPIO or PWM.
> >   * @pwm_refclk_freq: Cache for the reference clock input to the PWM.
> > + * @id:           Unique instance ID
> >   */
> >  struct ti_sn65dsi86 {
> >       struct auxiliary_device         *bridge_aux;
> > @@ -202,8 +204,11 @@ struct ti_sn65dsi86 {
> >       atomic_t                        pwm_pin_busy;
> >  #endif
> >       unsigned int                    pwm_refclk_freq;
> > +     int                             id;
> >  };
> >
> > +static DEFINE_IDA(ti_sn65dsi86_ida);
> > +
> >  static const struct regmap_range ti_sn65dsi86_volatile_ranges[] = {
> >       { .range_min = 0, .range_max = 0xFF },
> >  };
> > @@ -488,6 +493,7 @@ static int ti_sn65dsi86_add_aux_device(struct ti_sn65dsi86 *pdata,
> >               return -ENOMEM;
> >
> >       aux->name = name;
> > +     aux->id = pdata->id;
> >       aux->dev.parent = dev;
> >       aux->dev.release = ti_sn65dsi86_aux_device_release;
> >       device_set_of_node_from_dev(&aux->dev, dev);
> > @@ -1889,6 +1895,13 @@ static int ti_sn65dsi86_parse_regulators(struct ti_sn65dsi86 *pdata)
> >                                      pdata->supplies);
> >  }
> >
> > +static void ti_sn65dsi86_devm_ida_free(void *data)
> > +{
> > +     struct ti_sn65dsi86 *pdata = data;
> > +
> > +     ida_free(&ti_sn65dsi86_ida, pdata->id);
> > +}
> > +
> >  static int ti_sn65dsi86_probe(struct i2c_client *client)
> >  {
> >       struct device *dev = &client->dev;
> > @@ -1903,6 +1916,17 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
> >       pdata = devm_kzalloc(dev, sizeof(struct ti_sn65dsi86), GFP_KERNEL);
> >       if (!pdata)
> >               return -ENOMEM;
> > +
> > +     ret = ida_alloc(&ti_sn65dsi86_ida, GFP_KERNEL);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     pdata->id = ret;
> > +
> > +     ret = devm_add_action_or_reset(dev, ti_sn65dsi86_devm_ida_free, pdata);
> > +     if (ret)
> > +             return ret;
> > +
> >       dev_set_drvdata(dev, pdata);
> >       pdata->dev = dev;
> >

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix multiple instances

[Greg KH]

On Fri, Oct 18, 2024 at 03:36:48PM +0200, Geert Uytterhoeven wrote:
> Hi Laurent,
> 
> CC GregKH
> 
> On Fri, Oct 18, 2024 at 3:10 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > On Fri, Oct 18, 2024 at 09:45:52AM +0200, Geert Uytterhoeven wrote:
> > > Each bridge instance creates up to four auxiliary devices with different
> > > names.  However, their IDs are always zero, causing duplicate filename
> > > errors when a system has multiple bridges:
> > >
> > >     sysfs: cannot create duplicate filename '/bus/auxiliary/devices/ti_sn65dsi86.gpio.0'
> > >
> > > Fix this by using a unique instance ID per bridge instance.
> >
> > Isn't this something that should be handled by the AUX core ? The code
> > below would otherwise need to be duplicated by all drivers, which seems
> > a burden we should avoid.
> 
> According to the documentation, this is the responsibility of the caller
> https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L81
> I believe this is the same for platform devices.
> See also the example at
> https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L116
> 
> Note: the platform bus supports PLATFORM_DEVID_AUTO, but the auxiliary
> bus does not.

Yes, it does not as it's up to the caller to create a unique name, like
your patch here does.  I'd argue that platform should also not do
automatic device ids, but that's a different argument :)

This change looks good to me!

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix multiple instances

[Laurent Pinchart]

Hi Greg,

On Fri, Oct 18, 2024 at 04:09:26PM +0200, Greg KH wrote:
> On Fri, Oct 18, 2024 at 03:36:48PM +0200, Geert Uytterhoeven wrote:
> > On Fri, Oct 18, 2024 at 3:10 PM Laurent Pinchart wrote:
> > > On Fri, Oct 18, 2024 at 09:45:52AM +0200, Geert Uytterhoeven wrote:
> > > > Each bridge instance creates up to four auxiliary devices with different
> > > > names.  However, their IDs are always zero, causing duplicate filename
> > > > errors when a system has multiple bridges:
> > > >
> > > >     sysfs: cannot create duplicate filename '/bus/auxiliary/devices/ti_sn65dsi86.gpio.0'
> > > >
> > > > Fix this by using a unique instance ID per bridge instance.
> > >
> > > Isn't this something that should be handled by the AUX core ? The code
> > > below would otherwise need to be duplicated by all drivers, which seems
> > > a burden we should avoid.
> > 
> > According to the documentation, this is the responsibility of the caller
> > https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L81
> > I believe this is the same for platform devices.
> > See also the example at
> > https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L116
> > 
> > Note: the platform bus supports PLATFORM_DEVID_AUTO, but the auxiliary
> > bus does not.
> 
> Yes, it does not as it's up to the caller to create a unique name, like
> your patch here does.  I'd argue that platform should also not do
> automatic device ids, but that's a different argument :)

__auxiliary_device_add() creates the device name with

	dev_set_name(dev, "%s.%s.%d", modname, auxdev->name, auxdev->id);

I'm not calling for a PLATFORM_DEVID_AUTO-like feature here, but
shouldn't the first component of the device name use the parent's name
instead of the module name ?

> This change looks good to me!
> 
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix multiple instances

[Greg KH]

On Fri, Oct 18, 2024 at 05:25:22PM +0300, Laurent Pinchart wrote:
> Hi Greg,
> 
> On Fri, Oct 18, 2024 at 04:09:26PM +0200, Greg KH wrote:
> > On Fri, Oct 18, 2024 at 03:36:48PM +0200, Geert Uytterhoeven wrote:
> > > On Fri, Oct 18, 2024 at 3:10 PM Laurent Pinchart wrote:
> > > > On Fri, Oct 18, 2024 at 09:45:52AM +0200, Geert Uytterhoeven wrote:
> > > > > Each bridge instance creates up to four auxiliary devices with different
> > > > > names.  However, their IDs are always zero, causing duplicate filename
> > > > > errors when a system has multiple bridges:
> > > > >
> > > > >     sysfs: cannot create duplicate filename '/bus/auxiliary/devices/ti_sn65dsi86.gpio.0'
> > > > >
> > > > > Fix this by using a unique instance ID per bridge instance.
> > > >
> > > > Isn't this something that should be handled by the AUX core ? The code
> > > > below would otherwise need to be duplicated by all drivers, which seems
> > > > a burden we should avoid.
> > > 
> > > According to the documentation, this is the responsibility of the caller
> > > https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L81
> > > I believe this is the same for platform devices.
> > > See also the example at
> > > https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L116
> > > 
> > > Note: the platform bus supports PLATFORM_DEVID_AUTO, but the auxiliary
> > > bus does not.
> > 
> > Yes, it does not as it's up to the caller to create a unique name, like
> > your patch here does.  I'd argue that platform should also not do
> > automatic device ids, but that's a different argument :)
> 
> __auxiliary_device_add() creates the device name with
> 
> 	dev_set_name(dev, "%s.%s.%d", modname, auxdev->name, auxdev->id);
> 
> I'm not calling for a PLATFORM_DEVID_AUTO-like feature here, but
> shouldn't the first component of the device name use the parent's name
> instead of the module name ?

Why would the parent's name not be the module name?  That name is
guaranteed unique in the system.  If you want "uniqueness" within the
driver/module, use the name and id field please.

That's worked well so far, but to be fair, aux devices are pretty new.
What problem is this naming scheme causing?

thanks,

greg k-h

Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix multiple instances

[Laurent Pinchart]

On Fri, Oct 18, 2024 at 04:31:21PM +0200, Greg KH wrote:
> On Fri, Oct 18, 2024 at 05:25:22PM +0300, Laurent Pinchart wrote:
> > On Fri, Oct 18, 2024 at 04:09:26PM +0200, Greg KH wrote:
> > > On Fri, Oct 18, 2024 at 03:36:48PM +0200, Geert Uytterhoeven wrote:
> > > > On Fri, Oct 18, 2024 at 3:10 PM Laurent Pinchart wrote:
> > > > > On Fri, Oct 18, 2024 at 09:45:52AM +0200, Geert Uytterhoeven wrote:
> > > > > > Each bridge instance creates up to four auxiliary devices with different
> > > > > > names.  However, their IDs are always zero, causing duplicate filename
> > > > > > errors when a system has multiple bridges:
> > > > > >
> > > > > >     sysfs: cannot create duplicate filename '/bus/auxiliary/devices/ti_sn65dsi86.gpio.0'
> > > > > >
> > > > > > Fix this by using a unique instance ID per bridge instance.
> > > > >
> > > > > Isn't this something that should be handled by the AUX core ? The code
> > > > > below would otherwise need to be duplicated by all drivers, which seems
> > > > > a burden we should avoid.
> > > > 
> > > > According to the documentation, this is the responsibility of the caller
> > > > https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L81
> > > > I believe this is the same for platform devices.
> > > > See also the example at
> > > > https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L116
> > > > 
> > > > Note: the platform bus supports PLATFORM_DEVID_AUTO, but the auxiliary
> > > > bus does not.
> > > 
> > > Yes, it does not as it's up to the caller to create a unique name, like
> > > your patch here does.  I'd argue that platform should also not do
> > > automatic device ids, but that's a different argument :)
> > 
> > __auxiliary_device_add() creates the device name with
> > 
> > 	dev_set_name(dev, "%s.%s.%d", modname, auxdev->name, auxdev->id);
> > 
> > I'm not calling for a PLATFORM_DEVID_AUTO-like feature here, but
> > shouldn't the first component of the device name use the parent's name
> > instead of the module name ?
> 
> Why would the parent's name not be the module name?  That name is
> guaranteed unique in the system.  If you want "uniqueness" within the
> driver/module, use the name and id field please.
> 
> That's worked well so far, but to be fair, aux devices are pretty new.
> What problem is this naming scheme causing?

Auxiliary devices are created as children of a parent device. When
multiple instances of the same parent type exist, this will be reflected
in the /sys/devices/ devices tree hierarchy without any issue. The
problem comes from the fact the the auxiliary devices need a unique name
for /sys/bus/auxialiary/devices/, where we somehow have to differenciate
devices of identical types.

Essentially, we're trying to summarize a whole hierarchy (path in
/sys/devices/) into a single string. There are different ways to solve
this. For platform devices, we use a device ID. For I2C devices, we use
the parent's bus number. Other buses use different schemes.

Geert's patch implements a mechanism in the ti-sn65dsi86 driver to
handle this, and assign an id managed by the parent. In a sense we could
consider this to be similar to what is done for I2C, where the bus
number is also a property of the parent. However, the big difference is
that the I2C bus number is managed by the I2C subsystem, while here the
id is managed by the ti-sn65dsi86 driver, not by the auxiliary device
core. This would require duplicating the same mechanism in every single
driver creating auxiliary devices. This strikes me as a fairly bad idea.
The problem should be solved by the core, not by individual drivers.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix multiple instances

[Greg KH]

On Sun, Oct 20, 2024 at 05:36:29PM +0300, Laurent Pinchart wrote:
> On Fri, Oct 18, 2024 at 04:31:21PM +0200, Greg KH wrote:
> > On Fri, Oct 18, 2024 at 05:25:22PM +0300, Laurent Pinchart wrote:
> > > On Fri, Oct 18, 2024 at 04:09:26PM +0200, Greg KH wrote:
> > > > On Fri, Oct 18, 2024 at 03:36:48PM +0200, Geert Uytterhoeven wrote:
> > > > > On Fri, Oct 18, 2024 at 3:10 PM Laurent Pinchart wrote:
> > > > > > On Fri, Oct 18, 2024 at 09:45:52AM +0200, Geert Uytterhoeven wrote:
> > > > > > > Each bridge instance creates up to four auxiliary devices with different
> > > > > > > names.  However, their IDs are always zero, causing duplicate filename
> > > > > > > errors when a system has multiple bridges:
> > > > > > >
> > > > > > >     sysfs: cannot create duplicate filename '/bus/auxiliary/devices/ti_sn65dsi86.gpio.0'
> > > > > > >
> > > > > > > Fix this by using a unique instance ID per bridge instance.
> > > > > >
> > > > > > Isn't this something that should be handled by the AUX core ? The code
> > > > > > below would otherwise need to be duplicated by all drivers, which seems
> > > > > > a burden we should avoid.
> > > > > 
> > > > > According to the documentation, this is the responsibility of the caller
> > > > > https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L81
> > > > > I believe this is the same for platform devices.
> > > > > See also the example at
> > > > > https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L116
> > > > > 
> > > > > Note: the platform bus supports PLATFORM_DEVID_AUTO, but the auxiliary
> > > > > bus does not.
> > > > 
> > > > Yes, it does not as it's up to the caller to create a unique name, like
> > > > your patch here does.  I'd argue that platform should also not do
> > > > automatic device ids, but that's a different argument :)
> > > 
> > > __auxiliary_device_add() creates the device name with
> > > 
> > > 	dev_set_name(dev, "%s.%s.%d", modname, auxdev->name, auxdev->id);
> > > 
> > > I'm not calling for a PLATFORM_DEVID_AUTO-like feature here, but
> > > shouldn't the first component of the device name use the parent's name
> > > instead of the module name ?
> > 
> > Why would the parent's name not be the module name?  That name is
> > guaranteed unique in the system.  If you want "uniqueness" within the
> > driver/module, use the name and id field please.
> > 
> > That's worked well so far, but to be fair, aux devices are pretty new.
> > What problem is this naming scheme causing?
> 
> Auxiliary devices are created as children of a parent device. When
> multiple instances of the same parent type exist, this will be reflected
> in the /sys/devices/ devices tree hierarchy without any issue. The
> problem comes from the fact the the auxiliary devices need a unique name
> for /sys/bus/auxialiary/devices/, where we somehow have to differenciate
> devices of identical types.
> 
> Essentially, we're trying to summarize a whole hierarchy (path in
> /sys/devices/) into a single string. There are different ways to solve
> this. For platform devices, we use a device ID. For I2C devices, we use
> the parent's bus number. Other buses use different schemes.
> 
> Geert's patch implements a mechanism in the ti-sn65dsi86 driver to
> handle this, and assign an id managed by the parent. In a sense we could
> consider this to be similar to what is done for I2C, where the bus
> number is also a property of the parent. However, the big difference is
> that the I2C bus number is managed by the I2C subsystem, while here the
> id is managed by the ti-sn65dsi86 driver, not by the auxiliary device
> core. This would require duplicating the same mechanism in every single
> driver creating auxiliary devices. This strikes me as a fairly bad idea.
> The problem should be solved by the core, not by individual drivers.

The "id" is just a unique number, it is "managed" by the thing that is
creating the devices themselves, not the aux core code.  I don't see why
the i2c bus number has to match the same number that the ti driver
creates, it could be anything, as long as it doesn't match anything else
currently created by that driver.

If we had the aux core code create the id, it would just use a unique
counter, and that would not reflect any mapping to anything, so I don't
see how that is any different here.

confused,

greg k-h

Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix multiple instances

[Geert Uytterhoeven]

Hi Greg,

On Mon, Oct 21, 2024 at 8:39 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> On Sun, Oct 20, 2024 at 05:36:29PM +0300, Laurent Pinchart wrote:
> > On Fri, Oct 18, 2024 at 04:31:21PM +0200, Greg KH wrote:
> > > On Fri, Oct 18, 2024 at 05:25:22PM +0300, Laurent Pinchart wrote:
> > > > On Fri, Oct 18, 2024 at 04:09:26PM +0200, Greg KH wrote:
> > > > > On Fri, Oct 18, 2024 at 03:36:48PM +0200, Geert Uytterhoeven wrote:
> > > > > > On Fri, Oct 18, 2024 at 3:10 PM Laurent Pinchart wrote:
> > > > > > > On Fri, Oct 18, 2024 at 09:45:52AM +0200, Geert Uytterhoeven wrote:
> > > > > > > > Each bridge instance creates up to four auxiliary devices with different
> > > > > > > > names.  However, their IDs are always zero, causing duplicate filename
> > > > > > > > errors when a system has multiple bridges:
> > > > > > > >
> > > > > > > >     sysfs: cannot create duplicate filename '/bus/auxiliary/devices/ti_sn65dsi86.gpio.0'
> > > > > > > >
> > > > > > > > Fix this by using a unique instance ID per bridge instance.
> > > > > > >
> > > > > > > Isn't this something that should be handled by the AUX core ? The code
> > > > > > > below would otherwise need to be duplicated by all drivers, which seems
> > > > > > > a burden we should avoid.
> > > > > >
> > > > > > According to the documentation, this is the responsibility of the caller
> > > > > > https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L81
> > > > > > I believe this is the same for platform devices.
> > > > > > See also the example at
> > > > > > https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L116
> > > > > >
> > > > > > Note: the platform bus supports PLATFORM_DEVID_AUTO, but the auxiliary
> > > > > > bus does not.
> > > > >
> > > > > Yes, it does not as it's up to the caller to create a unique name, like
> > > > > your patch here does.  I'd argue that platform should also not do
> > > > > automatic device ids, but that's a different argument :)
> > > >
> > > > __auxiliary_device_add() creates the device name with
> > > >
> > > >   dev_set_name(dev, "%s.%s.%d", modname, auxdev->name, auxdev->id);
> > > >
> > > > I'm not calling for a PLATFORM_DEVID_AUTO-like feature here, but
> > > > shouldn't the first component of the device name use the parent's name
> > > > instead of the module name ?
> > >
> > > Why would the parent's name not be the module name?  That name is
> > > guaranteed unique in the system.  If you want "uniqueness" within the
> > > driver/module, use the name and id field please.
> > >
> > > That's worked well so far, but to be fair, aux devices are pretty new.
> > > What problem is this naming scheme causing?
> >
> > Auxiliary devices are created as children of a parent device. When
> > multiple instances of the same parent type exist, this will be reflected
> > in the /sys/devices/ devices tree hierarchy without any issue. The
> > problem comes from the fact the the auxiliary devices need a unique name
> > for /sys/bus/auxialiary/devices/, where we somehow have to differenciate
> > devices of identical types.
> >
> > Essentially, we're trying to summarize a whole hierarchy (path in
> > /sys/devices/) into a single string. There are different ways to solve
> > this. For platform devices, we use a device ID. For I2C devices, we use
> > the parent's bus number. Other buses use different schemes.
> >
> > Geert's patch implements a mechanism in the ti-sn65dsi86 driver to
> > handle this, and assign an id managed by the parent. In a sense we could
> > consider this to be similar to what is done for I2C, where the bus
> > number is also a property of the parent. However, the big difference is
> > that the I2C bus number is managed by the I2C subsystem, while here the
> > id is managed by the ti-sn65dsi86 driver, not by the auxiliary device
> > core. This would require duplicating the same mechanism in every single
> > driver creating auxiliary devices. This strikes me as a fairly bad idea.
> > The problem should be solved by the core, not by individual drivers.
>
> The "id" is just a unique number, it is "managed" by the thing that is
> creating the devices themselves, not the aux core code.  I don't see why
> the i2c bus number has to match the same number that the ti driver
> creates, it could be anything, as long as it doesn't match anything else
> currently created by that driver.

Laurent does not say it has to match the i2c bus number.
He does think the auxilliary bus should provide a mechanism to
allocate these IDs (e.g. usin g AUX_DEVID_AUTO?).

However, using i2c_client->adapter->nr instead of ida_alloc()
in the TI driver does sound like a good idea to me...

> If we had the aux core code create the id, it would just use a unique
> counter, and that would not reflect any mapping to anything, so I don't
> see how that is any different here.

And then we would get something like:

/sys/bus/auxiliary/devices
├── ti_sn65dsi86.gpio.0
├── ti_sn65dsi86.pwm.1
├── ti_sn65dsi86.aux.2
├── ti_sn65dsi86.bridge.3
├── ti_sn65dsi86.gpio.4
├── ti_sn65dsi86.pwm.5
├── ti_sn65dsi86.aux.6
└── ti_sn65dsi86.bridge.7

Which is similar to the first approach I tried (calling ida_alloc() in
ti_sn65dsi86_add_aux_device() instead of ti_sn65dsi86_probe()).

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix multiple instances

[Greg KH]

On Mon, Oct 21, 2024 at 08:58:30AM +0200, Geert Uytterhoeven wrote:
> Hi Greg,
> 
> On Mon, Oct 21, 2024 at 8:39 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Sun, Oct 20, 2024 at 05:36:29PM +0300, Laurent Pinchart wrote:
> > > On Fri, Oct 18, 2024 at 04:31:21PM +0200, Greg KH wrote:
> > > > On Fri, Oct 18, 2024 at 05:25:22PM +0300, Laurent Pinchart wrote:
> > > > > On Fri, Oct 18, 2024 at 04:09:26PM +0200, Greg KH wrote:
> > > > > > On Fri, Oct 18, 2024 at 03:36:48PM +0200, Geert Uytterhoeven wrote:
> > > > > > > On Fri, Oct 18, 2024 at 3:10 PM Laurent Pinchart wrote:
> > > > > > > > On Fri, Oct 18, 2024 at 09:45:52AM +0200, Geert Uytterhoeven wrote:
> > > > > > > > > Each bridge instance creates up to four auxiliary devices with different
> > > > > > > > > names.  However, their IDs are always zero, causing duplicate filename
> > > > > > > > > errors when a system has multiple bridges:
> > > > > > > > >
> > > > > > > > >     sysfs: cannot create duplicate filename '/bus/auxiliary/devices/ti_sn65dsi86.gpio.0'
> > > > > > > > >
> > > > > > > > > Fix this by using a unique instance ID per bridge instance.
> > > > > > > >
> > > > > > > > Isn't this something that should be handled by the AUX core ? The code
> > > > > > > > below would otherwise need to be duplicated by all drivers, which seems
> > > > > > > > a burden we should avoid.
> > > > > > >
> > > > > > > According to the documentation, this is the responsibility of the caller
> > > > > > > https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L81
> > > > > > > I believe this is the same for platform devices.
> > > > > > > See also the example at
> > > > > > > https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L116
> > > > > > >
> > > > > > > Note: the platform bus supports PLATFORM_DEVID_AUTO, but the auxiliary
> > > > > > > bus does not.
> > > > > >
> > > > > > Yes, it does not as it's up to the caller to create a unique name, like
> > > > > > your patch here does.  I'd argue that platform should also not do
> > > > > > automatic device ids, but that's a different argument :)
> > > > >
> > > > > __auxiliary_device_add() creates the device name with
> > > > >
> > > > >   dev_set_name(dev, "%s.%s.%d", modname, auxdev->name, auxdev->id);
> > > > >
> > > > > I'm not calling for a PLATFORM_DEVID_AUTO-like feature here, but
> > > > > shouldn't the first component of the device name use the parent's name
> > > > > instead of the module name ?
> > > >
> > > > Why would the parent's name not be the module name?  That name is
> > > > guaranteed unique in the system.  If you want "uniqueness" within the
> > > > driver/module, use the name and id field please.
> > > >
> > > > That's worked well so far, but to be fair, aux devices are pretty new.
> > > > What problem is this naming scheme causing?
> > >
> > > Auxiliary devices are created as children of a parent device. When
> > > multiple instances of the same parent type exist, this will be reflected
> > > in the /sys/devices/ devices tree hierarchy without any issue. The
> > > problem comes from the fact the the auxiliary devices need a unique name
> > > for /sys/bus/auxialiary/devices/, where we somehow have to differenciate
> > > devices of identical types.
> > >
> > > Essentially, we're trying to summarize a whole hierarchy (path in
> > > /sys/devices/) into a single string. There are different ways to solve
> > > this. For platform devices, we use a device ID. For I2C devices, we use
> > > the parent's bus number. Other buses use different schemes.
> > >
> > > Geert's patch implements a mechanism in the ti-sn65dsi86 driver to
> > > handle this, and assign an id managed by the parent. In a sense we could
> > > consider this to be similar to what is done for I2C, where the bus
> > > number is also a property of the parent. However, the big difference is
> > > that the I2C bus number is managed by the I2C subsystem, while here the
> > > id is managed by the ti-sn65dsi86 driver, not by the auxiliary device
> > > core. This would require duplicating the same mechanism in every single
> > > driver creating auxiliary devices. This strikes me as a fairly bad idea.
> > > The problem should be solved by the core, not by individual drivers.
> >
> > The "id" is just a unique number, it is "managed" by the thing that is
> > creating the devices themselves, not the aux core code.  I don't see why
> > the i2c bus number has to match the same number that the ti driver
> > creates, it could be anything, as long as it doesn't match anything else
> > currently created by that driver.
> 
> Laurent does not say it has to match the i2c bus number.
> He does think the auxilliary bus should provide a mechanism to
> allocate these IDs (e.g. usin g AUX_DEVID_AUTO?).

As this is the first subsystem to ask for such a thing, I didn't think
it was needed, but the aux subsystem is new :)

> However, using i2c_client->adapter->nr instead of ida_alloc()
> in the TI driver does sound like a good idea to me...

Great!

> > If we had the aux core code create the id, it would just use a unique
> > counter, and that would not reflect any mapping to anything, so I don't
> > see how that is any different here.
> 
> And then we would get something like:
> 
> /sys/bus/auxiliary/devices
> ├── ti_sn65dsi86.gpio.0
> ├── ti_sn65dsi86.pwm.1
> ├── ti_sn65dsi86.aux.2
> ├── ti_sn65dsi86.bridge.3
> ├── ti_sn65dsi86.gpio.4
> ├── ti_sn65dsi86.pwm.5
> ├── ti_sn65dsi86.aux.6
> └── ti_sn65dsi86.bridge.7
> 
> Which is similar to the first approach I tried (calling ida_alloc() in
> ti_sn65dsi86_add_aux_device() instead of ti_sn65dsi86_probe()).

That id scheme looks really odd, don't you think?  Try using the
adapter->nr instead like other aux subsystems already do.

thanks,

greg k-h

Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix multiple instances

[Geert Uytterhoeven]

Hi Greg,

On Mon, Oct 21, 2024 at 9:27 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> On Mon, Oct 21, 2024 at 08:58:30AM +0200, Geert Uytterhoeven wrote:
> > On Mon, Oct 21, 2024 at 8:39 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > On Sun, Oct 20, 2024 at 05:36:29PM +0300, Laurent Pinchart wrote:
> > > > On Fri, Oct 18, 2024 at 04:31:21PM +0200, Greg KH wrote:
> > > > > On Fri, Oct 18, 2024 at 05:25:22PM +0300, Laurent Pinchart wrote:
> > > > > > On Fri, Oct 18, 2024 at 04:09:26PM +0200, Greg KH wrote:
> > > > > > > On Fri, Oct 18, 2024 at 03:36:48PM +0200, Geert Uytterhoeven wrote:
> > > > > > > > On Fri, Oct 18, 2024 at 3:10 PM Laurent Pinchart wrote:
> > > > > > > > > On Fri, Oct 18, 2024 at 09:45:52AM +0200, Geert Uytterhoeven wrote:
> > > > > > > > > > Each bridge instance creates up to four auxiliary devices with different
> > > > > > > > > > names.  However, their IDs are always zero, causing duplicate filename
> > > > > > > > > > errors when a system has multiple bridges:
> > > > > > > > > >
> > > > > > > > > >     sysfs: cannot create duplicate filename '/bus/auxiliary/devices/ti_sn65dsi86.gpio.0'
> > > > > > > > > >
> > > > > > > > > > Fix this by using a unique instance ID per bridge instance.
> > > > > > > > >
> > > > > > > > > Isn't this something that should be handled by the AUX core ? The code
> > > > > > > > > below would otherwise need to be duplicated by all drivers, which seems
> > > > > > > > > a burden we should avoid.
> > > > > > > >
> > > > > > > > According to the documentation, this is the responsibility of the caller
> > > > > > > > https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L81
> > > > > > > > I believe this is the same for platform devices.
> > > > > > > > See also the example at
> > > > > > > > https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L116
> > > > > > > >
> > > > > > > > Note: the platform bus supports PLATFORM_DEVID_AUTO, but the auxiliary
> > > > > > > > bus does not.
> > > > > > >
> > > > > > > Yes, it does not as it's up to the caller to create a unique name, like
> > > > > > > your patch here does.  I'd argue that platform should also not do
> > > > > > > automatic device ids, but that's a different argument :)
> > > > > >
> > > > > > __auxiliary_device_add() creates the device name with
> > > > > >
> > > > > >   dev_set_name(dev, "%s.%s.%d", modname, auxdev->name, auxdev->id);
> > > > > >
> > > > > > I'm not calling for a PLATFORM_DEVID_AUTO-like feature here, but
> > > > > > shouldn't the first component of the device name use the parent's name
> > > > > > instead of the module name ?
> > > > >
> > > > > Why would the parent's name not be the module name?  That name is
> > > > > guaranteed unique in the system.  If you want "uniqueness" within the
> > > > > driver/module, use the name and id field please.
> > > > >
> > > > > That's worked well so far, but to be fair, aux devices are pretty new.
> > > > > What problem is this naming scheme causing?
> > > >
> > > > Auxiliary devices are created as children of a parent device. When
> > > > multiple instances of the same parent type exist, this will be reflected
> > > > in the /sys/devices/ devices tree hierarchy without any issue. The
> > > > problem comes from the fact the the auxiliary devices need a unique name
> > > > for /sys/bus/auxialiary/devices/, where we somehow have to differenciate
> > > > devices of identical types.
> > > >
> > > > Essentially, we're trying to summarize a whole hierarchy (path in
> > > > /sys/devices/) into a single string. There are different ways to solve
> > > > this. For platform devices, we use a device ID. For I2C devices, we use
> > > > the parent's bus number. Other buses use different schemes.
> > > >
> > > > Geert's patch implements a mechanism in the ti-sn65dsi86 driver to
> > > > handle this, and assign an id managed by the parent. In a sense we could
> > > > consider this to be similar to what is done for I2C, where the bus
> > > > number is also a property of the parent. However, the big difference is
> > > > that the I2C bus number is managed by the I2C subsystem, while here the
> > > > id is managed by the ti-sn65dsi86 driver, not by the auxiliary device
> > > > core. This would require duplicating the same mechanism in every single
> > > > driver creating auxiliary devices. This strikes me as a fairly bad idea.
> > > > The problem should be solved by the core, not by individual drivers.
> > >
> > > The "id" is just a unique number, it is "managed" by the thing that is
> > > creating the devices themselves, not the aux core code.  I don't see why
> > > the i2c bus number has to match the same number that the ti driver
> > > creates, it could be anything, as long as it doesn't match anything else
> > > currently created by that driver.
> >
> > Laurent does not say it has to match the i2c bus number.
> > He does think the auxilliary bus should provide a mechanism to
> > allocate these IDs (e.g. usin g AUX_DEVID_AUTO?).
>
> As this is the first subsystem to ask for such a thing, I didn't think
> it was needed, but the aux subsystem is new :)
>
> > However, using i2c_client->adapter->nr instead of ida_alloc()
> > in the TI driver does sound like a good idea to me...
>
> Great!
>
> > > If we had the aux core code create the id, it would just use a unique
> > > counter, and that would not reflect any mapping to anything, so I don't
> > > see how that is any different here.
> >
> > And then we would get something like:
> >
> > /sys/bus/auxiliary/devices
> > ├── ti_sn65dsi86.gpio.0
> > ├── ti_sn65dsi86.pwm.1
> > ├── ti_sn65dsi86.aux.2
> > ├── ti_sn65dsi86.bridge.3
> > ├── ti_sn65dsi86.gpio.4
> > ├── ti_sn65dsi86.pwm.5
> > ├── ti_sn65dsi86.aux.6
> > └── ti_sn65dsi86.bridge.7
> >
> > Which is similar to the first approach I tried (calling ida_alloc() in
> > ti_sn65dsi86_add_aux_device() instead of ti_sn65dsi86_probe()).
>
> That id scheme looks really odd, don't you think?  Try using the

Yes, that's what happens if you let an external entity come up
with the unique IDs.

With the I2C adapter numbers, that becomes:

    /sys/bus/auxiliary/devices
    ├── ti_sn65dsi86.gpio.1
    ├── ti_sn65dsi86.pwm.1
    ├── ti_sn65dsi86.aux.1
    ├── ti_sn65dsi86.bridge.1
    ├── ti_sn65dsi86.gpio.4
    ├── ti_sn65dsi86.pwm.4
    ├── ti_sn65dsi86.aux.4
    └── ti_sn65dsi86.bridge.4

> adapter->nr instead like other aux subsystems already do.

FTR, according to "git grep "adapter->nr" -- $(git grep -l
auxiliary_device_init)",
no existing driver uses this mechanism yet.

"git grep ida_alloc -- $(git grep -l auxiliary_device_init)" does show
several hits.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix multiple instances

[Laurent Pinchart]

On Mon, Oct 21, 2024 at 10:23:26AM +0200, Geert Uytterhoeven wrote:
> On Mon, Oct 21, 2024 at 9:27 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Mon, Oct 21, 2024 at 08:58:30AM +0200, Geert Uytterhoeven wrote:
> > > On Mon, Oct 21, 2024 at 8:39 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > On Sun, Oct 20, 2024 at 05:36:29PM +0300, Laurent Pinchart wrote:
> > > > > On Fri, Oct 18, 2024 at 04:31:21PM +0200, Greg KH wrote:
> > > > > > On Fri, Oct 18, 2024 at 05:25:22PM +0300, Laurent Pinchart wrote:
> > > > > > > On Fri, Oct 18, 2024 at 04:09:26PM +0200, Greg KH wrote:
> > > > > > > > On Fri, Oct 18, 2024 at 03:36:48PM +0200, Geert Uytterhoeven wrote:
> > > > > > > > > On Fri, Oct 18, 2024 at 3:10 PM Laurent Pinchart wrote:
> > > > > > > > > > On Fri, Oct 18, 2024 at 09:45:52AM +0200, Geert Uytterhoeven wrote:
> > > > > > > > > > > Each bridge instance creates up to four auxiliary devices with different
> > > > > > > > > > > names.  However, their IDs are always zero, causing duplicate filename
> > > > > > > > > > > errors when a system has multiple bridges:
> > > > > > > > > > >
> > > > > > > > > > >     sysfs: cannot create duplicate filename '/bus/auxiliary/devices/ti_sn65dsi86.gpio.0'
> > > > > > > > > > >
> > > > > > > > > > > Fix this by using a unique instance ID per bridge instance.
> > > > > > > > > >
> > > > > > > > > > Isn't this something that should be handled by the AUX core ? The code
> > > > > > > > > > below would otherwise need to be duplicated by all drivers, which seems
> > > > > > > > > > a burden we should avoid.
> > > > > > > > >
> > > > > > > > > According to the documentation, this is the responsibility of the caller
> > > > > > > > > https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L81
> > > > > > > > > I believe this is the same for platform devices.
> > > > > > > > > See also the example at
> > > > > > > > > https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L116
> > > > > > > > >
> > > > > > > > > Note: the platform bus supports PLATFORM_DEVID_AUTO, but the auxiliary
> > > > > > > > > bus does not.
> > > > > > > >
> > > > > > > > Yes, it does not as it's up to the caller to create a unique name, like
> > > > > > > > your patch here does.  I'd argue that platform should also not do
> > > > > > > > automatic device ids, but that's a different argument :)
> > > > > > >
> > > > > > > __auxiliary_device_add() creates the device name with
> > > > > > >
> > > > > > >   dev_set_name(dev, "%s.%s.%d", modname, auxdev->name, auxdev->id);
> > > > > > >
> > > > > > > I'm not calling for a PLATFORM_DEVID_AUTO-like feature here, but
> > > > > > > shouldn't the first component of the device name use the parent's name
> > > > > > > instead of the module name ?
> > > > > >
> > > > > > Why would the parent's name not be the module name?  That name is
> > > > > > guaranteed unique in the system.  If you want "uniqueness" within the
> > > > > > driver/module, use the name and id field please.
> > > > > >
> > > > > > That's worked well so far, but to be fair, aux devices are pretty new.
> > > > > > What problem is this naming scheme causing?
> > > > >
> > > > > Auxiliary devices are created as children of a parent device. When
> > > > > multiple instances of the same parent type exist, this will be reflected
> > > > > in the /sys/devices/ devices tree hierarchy without any issue. The
> > > > > problem comes from the fact the the auxiliary devices need a unique name
> > > > > for /sys/bus/auxialiary/devices/, where we somehow have to differenciate
> > > > > devices of identical types.
> > > > >
> > > > > Essentially, we're trying to summarize a whole hierarchy (path in
> > > > > /sys/devices/) into a single string. There are different ways to solve
> > > > > this. For platform devices, we use a device ID. For I2C devices, we use
> > > > > the parent's bus number. Other buses use different schemes.
> > > > >
> > > > > Geert's patch implements a mechanism in the ti-sn65dsi86 driver to
> > > > > handle this, and assign an id managed by the parent. In a sense we could
> > > > > consider this to be similar to what is done for I2C, where the bus
> > > > > number is also a property of the parent. However, the big difference is
> > > > > that the I2C bus number is managed by the I2C subsystem, while here the
> > > > > id is managed by the ti-sn65dsi86 driver, not by the auxiliary device
> > > > > core. This would require duplicating the same mechanism in every single
> > > > > driver creating auxiliary devices. This strikes me as a fairly bad idea.
> > > > > The problem should be solved by the core, not by individual drivers.
> > > >
> > > > The "id" is just a unique number, it is "managed" by the thing that is
> > > > creating the devices themselves, not the aux core code.  I don't see why
> > > > the i2c bus number has to match the same number that the ti driver
> > > > creates, it could be anything, as long as it doesn't match anything else
> > > > currently created by that driver.
> > >
> > > Laurent does not say it has to match the i2c bus number.
> > > He does think the auxilliary bus should provide a mechanism to
> > > allocate these IDs (e.g. usin g AUX_DEVID_AUTO?).
> >
> > As this is the first subsystem to ask for such a thing, I didn't think
> > it was needed, but the aux subsystem is new :)
> >
> > > However, using i2c_client->adapter->nr instead of ida_alloc()
> > > in the TI driver does sound like a good idea to me...

It still means the driver needs to manage the ID space, but it's way
less intrusive so I'm OK with that, even if I still think a mechanism in
the auxiliary bus code would be best. After all, the id is needed to
construct a unique name, based on a naming scheme that is internal to
the auxiliary bus core (combining the module name with the device name
and the id). Requiring users to manage the id space to fulfil
constraints internal to the core seems a bit ill-designed to me.

> > Great!
> >
> > > > If we had the aux core code create the id, it would just use a unique
> > > > counter, and that would not reflect any mapping to anything, so I don't
> > > > see how that is any different here.
> > >
> > > And then we would get something like:
> > >
> > > /sys/bus/auxiliary/devices
> > > ├── ti_sn65dsi86.gpio.0
> > > ├── ti_sn65dsi86.pwm.1
> > > ├── ti_sn65dsi86.aux.2
> > > ├── ti_sn65dsi86.bridge.3
> > > ├── ti_sn65dsi86.gpio.4
> > > ├── ti_sn65dsi86.pwm.5
> > > ├── ti_sn65dsi86.aux.6
> > > └── ti_sn65dsi86.bridge.7
> > >
> > > Which is similar to the first approach I tried (calling ida_alloc() in
> > > ti_sn65dsi86_add_aux_device() instead of ti_sn65dsi86_probe()).
> >
> > That id scheme looks really odd, don't you think?  Try using the
> 
> Yes, that's what happens if you let an external entity come up
> with the unique IDs.
> 
> With the I2C adapter numbers, that becomes:
> 
>     /sys/bus/auxiliary/devices
>     ├── ti_sn65dsi86.gpio.1
>     ├── ti_sn65dsi86.pwm.1
>     ├── ti_sn65dsi86.aux.1
>     ├── ti_sn65dsi86.bridge.1
>     ├── ti_sn65dsi86.gpio.4
>     ├── ti_sn65dsi86.pwm.4
>     ├── ti_sn65dsi86.aux.4
>     └── ti_sn65dsi86.bridge.4
> 
> > adapter->nr instead like other aux subsystems already do.
> 
> FTR, according to "git grep "adapter->nr" -- $(git grep -l
> auxiliary_device_init)",
> no existing driver uses this mechanism yet.
> 
> "git grep ida_alloc -- $(git grep -l auxiliary_device_init)" does show
> several hits.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix multiple instances

[Geert Uytterhoeven]

Hi Greg,

On Mon, Oct 21, 2024 at 10:23 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Mon, Oct 21, 2024 at 9:27 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Mon, Oct 21, 2024 at 08:58:30AM +0200, Geert Uytterhoeven wrote:
> > > On Mon, Oct 21, 2024 at 8:39 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > On Sun, Oct 20, 2024 at 05:36:29PM +0300, Laurent Pinchart wrote:
> > > > > On Fri, Oct 18, 2024 at 04:31:21PM +0200, Greg KH wrote:
> > > > > > On Fri, Oct 18, 2024 at 05:25:22PM +0300, Laurent Pinchart wrote:
> > > > > > > On Fri, Oct 18, 2024 at 04:09:26PM +0200, Greg KH wrote:
> > > > > > > > On Fri, Oct 18, 2024 at 03:36:48PM +0200, Geert Uytterhoeven wrote:
> > > > > > > > > On Fri, Oct 18, 2024 at 3:10 PM Laurent Pinchart wrote:
> > > > > > > > > > On Fri, Oct 18, 2024 at 09:45:52AM +0200, Geert Uytterhoeven wrote:
> > > > > > > > > > > Each bridge instance creates up to four auxiliary devices with different
> > > > > > > > > > > names.  However, their IDs are always zero, causing duplicate filename
> > > > > > > > > > > errors when a system has multiple bridges:
> > > > > > > > > > >
> > > > > > > > > > >     sysfs: cannot create duplicate filename '/bus/auxiliary/devices/ti_sn65dsi86.gpio.0'
> > > > > > > > > > >
> > > > > > > > > > > Fix this by using a unique instance ID per bridge instance.
> > > > > > > > > >
> > > > > > > > > > Isn't this something that should be handled by the AUX core ? The code
> > > > > > > > > > below would otherwise need to be duplicated by all drivers, which seems
> > > > > > > > > > a burden we should avoid.
> > > > > > > > >
> > > > > > > > > According to the documentation, this is the responsibility of the caller
> > > > > > > > > https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L81
> > > > > > > > > I believe this is the same for platform devices.
> > > > > > > > > See also the example at
> > > > > > > > > https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L116
> > > > > > > > >
> > > > > > > > > Note: the platform bus supports PLATFORM_DEVID_AUTO, but the auxiliary
> > > > > > > > > bus does not.
> > > > > > > >
> > > > > > > > Yes, it does not as it's up to the caller to create a unique name, like
> > > > > > > > your patch here does.  I'd argue that platform should also not do
> > > > > > > > automatic device ids, but that's a different argument :)
> > > > > > >
> > > > > > > __auxiliary_device_add() creates the device name with
> > > > > > >
> > > > > > >   dev_set_name(dev, "%s.%s.%d", modname, auxdev->name, auxdev->id);
> > > > > > >
> > > > > > > I'm not calling for a PLATFORM_DEVID_AUTO-like feature here, but
> > > > > > > shouldn't the first component of the device name use the parent's name
> > > > > > > instead of the module name ?
> > > > > >
> > > > > > Why would the parent's name not be the module name?  That name is
> > > > > > guaranteed unique in the system.  If you want "uniqueness" within the
> > > > > > driver/module, use the name and id field please.
> > > > > >
> > > > > > That's worked well so far, but to be fair, aux devices are pretty new.
> > > > > > What problem is this naming scheme causing?
> > > > >
> > > > > Auxiliary devices are created as children of a parent device. When
> > > > > multiple instances of the same parent type exist, this will be reflected
> > > > > in the /sys/devices/ devices tree hierarchy without any issue. The
> > > > > problem comes from the fact the the auxiliary devices need a unique name
> > > > > for /sys/bus/auxialiary/devices/, where we somehow have to differenciate
> > > > > devices of identical types.
> > > > >
> > > > > Essentially, we're trying to summarize a whole hierarchy (path in
> > > > > /sys/devices/) into a single string. There are different ways to solve
> > > > > this. For platform devices, we use a device ID. For I2C devices, we use
> > > > > the parent's bus number. Other buses use different schemes.
> > > > >
> > > > > Geert's patch implements a mechanism in the ti-sn65dsi86 driver to
> > > > > handle this, and assign an id managed by the parent. In a sense we could
> > > > > consider this to be similar to what is done for I2C, where the bus
> > > > > number is also a property of the parent. However, the big difference is
> > > > > that the I2C bus number is managed by the I2C subsystem, while here the
> > > > > id is managed by the ti-sn65dsi86 driver, not by the auxiliary device
> > > > > core. This would require duplicating the same mechanism in every single
> > > > > driver creating auxiliary devices. This strikes me as a fairly bad idea.
> > > > > The problem should be solved by the core, not by individual drivers.
> > > >
> > > > The "id" is just a unique number, it is "managed" by the thing that is
> > > > creating the devices themselves, not the aux core code.  I don't see why
> > > > the i2c bus number has to match the same number that the ti driver
> > > > creates, it could be anything, as long as it doesn't match anything else
> > > > currently created by that driver.
> > >
> > > Laurent does not say it has to match the i2c bus number.
> > > He does think the auxilliary bus should provide a mechanism to
> > > allocate these IDs (e.g. usin g AUX_DEVID_AUTO?).
> >
> > As this is the first subsystem to ask for such a thing, I didn't think
> > it was needed, but the aux subsystem is new :)
> >
> > > However, using i2c_client->adapter->nr instead of ida_alloc()
> > > in the TI driver does sound like a good idea to me...
> >
> > Great!

> With the I2C adapter numbers, that becomes:
>
>     /sys/bus/auxiliary/devices
>     ├── ti_sn65dsi86.gpio.1
>     ├── ti_sn65dsi86.pwm.1
>     ├── ti_sn65dsi86.aux.1
>     ├── ti_sn65dsi86.bridge.1
>     ├── ti_sn65dsi86.gpio.4
>     ├── ti_sn65dsi86.pwm.4
>     ├── ti_sn65dsi86.aux.4
>     └── ti_sn65dsi86.bridge.4
>
> > adapter->nr instead like other aux subsystems already do.

Unfortunately the devil is in the details, as usual: there can be
multiple instances of the sn65dsi86 bridge on a single I2C bus,
so adapter->nr is not guaranteed to generate a unique name.

Changing the auxiliary bus to use the parent's name instead of the
module name, as suggested by Laurent, would fix that.

modname

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix multiple instances

[Doug Anderson]

Hi,

On Mon, Oct 21, 2024 at 1:48 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Greg,
>
> On Mon, Oct 21, 2024 at 10:23 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > On Mon, Oct 21, 2024 at 9:27 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > On Mon, Oct 21, 2024 at 08:58:30AM +0200, Geert Uytterhoeven wrote:
> > > > On Mon, Oct 21, 2024 at 8:39 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > On Sun, Oct 20, 2024 at 05:36:29PM +0300, Laurent Pinchart wrote:
> > > > > > On Fri, Oct 18, 2024 at 04:31:21PM +0200, Greg KH wrote:
> > > > > > > On Fri, Oct 18, 2024 at 05:25:22PM +0300, Laurent Pinchart wrote:
> > > > > > > > On Fri, Oct 18, 2024 at 04:09:26PM +0200, Greg KH wrote:
> > > > > > > > > On Fri, Oct 18, 2024 at 03:36:48PM +0200, Geert Uytterhoeven wrote:
> > > > > > > > > > On Fri, Oct 18, 2024 at 3:10 PM Laurent Pinchart wrote:
> > > > > > > > > > > On Fri, Oct 18, 2024 at 09:45:52AM +0200, Geert Uytterhoeven wrote:
> > > > > > > > > > > > Each bridge instance creates up to four auxiliary devices with different
> > > > > > > > > > > > names.  However, their IDs are always zero, causing duplicate filename
> > > > > > > > > > > > errors when a system has multiple bridges:
> > > > > > > > > > > >
> > > > > > > > > > > >     sysfs: cannot create duplicate filename '/bus/auxiliary/devices/ti_sn65dsi86.gpio.0'
> > > > > > > > > > > >
> > > > > > > > > > > > Fix this by using a unique instance ID per bridge instance.
> > > > > > > > > > >
> > > > > > > > > > > Isn't this something that should be handled by the AUX core ? The code
> > > > > > > > > > > below would otherwise need to be duplicated by all drivers, which seems
> > > > > > > > > > > a burden we should avoid.
> > > > > > > > > >
> > > > > > > > > > According to the documentation, this is the responsibility of the caller
> > > > > > > > > > https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L81
> > > > > > > > > > I believe this is the same for platform devices.
> > > > > > > > > > See also the example at
> > > > > > > > > > https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L116
> > > > > > > > > >
> > > > > > > > > > Note: the platform bus supports PLATFORM_DEVID_AUTO, but the auxiliary
> > > > > > > > > > bus does not.
> > > > > > > > >
> > > > > > > > > Yes, it does not as it's up to the caller to create a unique name, like
> > > > > > > > > your patch here does.  I'd argue that platform should also not do
> > > > > > > > > automatic device ids, but that's a different argument :)
> > > > > > > >
> > > > > > > > __auxiliary_device_add() creates the device name with
> > > > > > > >
> > > > > > > >   dev_set_name(dev, "%s.%s.%d", modname, auxdev->name, auxdev->id);
> > > > > > > >
> > > > > > > > I'm not calling for a PLATFORM_DEVID_AUTO-like feature here, but
> > > > > > > > shouldn't the first component of the device name use the parent's name
> > > > > > > > instead of the module name ?
> > > > > > >
> > > > > > > Why would the parent's name not be the module name?  That name is
> > > > > > > guaranteed unique in the system.  If you want "uniqueness" within the
> > > > > > > driver/module, use the name and id field please.
> > > > > > >
> > > > > > > That's worked well so far, but to be fair, aux devices are pretty new.
> > > > > > > What problem is this naming scheme causing?
> > > > > >
> > > > > > Auxiliary devices are created as children of a parent device. When
> > > > > > multiple instances of the same parent type exist, this will be reflected
> > > > > > in the /sys/devices/ devices tree hierarchy without any issue. The
> > > > > > problem comes from the fact the the auxiliary devices need a unique name
> > > > > > for /sys/bus/auxialiary/devices/, where we somehow have to differenciate
> > > > > > devices of identical types.
> > > > > >
> > > > > > Essentially, we're trying to summarize a whole hierarchy (path in
> > > > > > /sys/devices/) into a single string. There are different ways to solve
> > > > > > this. For platform devices, we use a device ID. For I2C devices, we use
> > > > > > the parent's bus number. Other buses use different schemes.
> > > > > >
> > > > > > Geert's patch implements a mechanism in the ti-sn65dsi86 driver to
> > > > > > handle this, and assign an id managed by the parent. In a sense we could
> > > > > > consider this to be similar to what is done for I2C, where the bus
> > > > > > number is also a property of the parent. However, the big difference is
> > > > > > that the I2C bus number is managed by the I2C subsystem, while here the
> > > > > > id is managed by the ti-sn65dsi86 driver, not by the auxiliary device
> > > > > > core. This would require duplicating the same mechanism in every single
> > > > > > driver creating auxiliary devices. This strikes me as a fairly bad idea.
> > > > > > The problem should be solved by the core, not by individual drivers.
> > > > >
> > > > > The "id" is just a unique number, it is "managed" by the thing that is
> > > > > creating the devices themselves, not the aux core code.  I don't see why
> > > > > the i2c bus number has to match the same number that the ti driver
> > > > > creates, it could be anything, as long as it doesn't match anything else
> > > > > currently created by that driver.
> > > >
> > > > Laurent does not say it has to match the i2c bus number.
> > > > He does think the auxilliary bus should provide a mechanism to
> > > > allocate these IDs (e.g. usin g AUX_DEVID_AUTO?).
> > >
> > > As this is the first subsystem to ask for such a thing, I didn't think
> > > it was needed, but the aux subsystem is new :)
> > >
> > > > However, using i2c_client->adapter->nr instead of ida_alloc()
> > > > in the TI driver does sound like a good idea to me...
> > >
> > > Great!
>
> > With the I2C adapter numbers, that becomes:
> >
> >     /sys/bus/auxiliary/devices
> >     ├── ti_sn65dsi86.gpio.1
> >     ├── ti_sn65dsi86.pwm.1
> >     ├── ti_sn65dsi86.aux.1
> >     ├── ti_sn65dsi86.bridge.1
> >     ├── ti_sn65dsi86.gpio.4
> >     ├── ti_sn65dsi86.pwm.4
> >     ├── ti_sn65dsi86.aux.4
> >     └── ti_sn65dsi86.bridge.4
> >
> > > adapter->nr instead like other aux subsystems already do.
>
> Unfortunately the devil is in the details, as usual: there can be
> multiple instances of the sn65dsi86 bridge on a single I2C bus,
> so adapter->nr is not guaranteed to generate a unique name.

In the case of sn65dsi86 I think we'd actually be OK. The TI bridge
chip is always at bus address 0x2d so you can't have more than one on
the same bus. Unless you added something funky atop it (like a mux of
some sort) you might be OK.


> Changing the auxiliary bus to use the parent's name instead of the
> module name, as suggested by Laurent, would fix that.

Right. On my system dev_name() of the sn65dsi86 device is "2-002d". If
we had a second on i2c bus 4, we'd have:

    /sys/bus/auxiliary/devices
    ├── 2-002d.gpio.0
    ├── 2-002d.pwm.0
    ├── 2-002d.aux.0
    ├── 2-002d.bridge.0
    ├── 4-002d.gpio.0
    ├── 4-002d.pwm.0
    ├── 4-002d.aux.0
    └── 4-002d.bridge.0

...and I think that's guaranteed to be unique because all the i2c
devices are flat in "/sys/bus/i2c/devices".

In any case, I've been standing on the sidelines because really any of
these ideas are fine to me.

Using the parent name is nice because it's completely automatic and
that's nice. It's a bigger change to paths / device names, though. I
could see it breaking someone somewhere.

The patch that Geert originally posted has the advantage of not
breaking things in case there's someone out there that wrote bad
userspace code and somehow hardcoded some old sysfs path. Others could
disagree, but IMO if it's easy it's nice to keep the sysfs paths
consistent even though it's not really an ABI promise. :-P I'm always
surprised how often people seem to hardcode paths like this. Sometimes
there's not a great alternative, but sometimes people just do it
because they don't know better...

The adapter->nr also seems OK to me in that it's a simple/small change.

Anyway, I'm happy to let others fight out for their favorite, but I
don't have a strong enough opinion to really argue one way or the
other...

-Doug

Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix multiple instances

[Geert Uytterhoeven]

Hi Doug,

On Tue, Oct 22, 2024 at 2:28 AM Doug Anderson <dianders@chromium.org> wrote:
> On Mon, Oct 21, 2024 at 1:48 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Mon, Oct 21, 2024 at 10:23 AM Geert Uytterhoeven
> > <geert@linux-m68k.org> wrote:
> > > On Mon, Oct 21, 2024 at 9:27 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > On Mon, Oct 21, 2024 at 08:58:30AM +0200, Geert Uytterhoeven wrote:
> > > > > On Mon, Oct 21, 2024 at 8:39 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > > > On Sun, Oct 20, 2024 at 05:36:29PM +0300, Laurent Pinchart wrote:
> > > > > > > On Fri, Oct 18, 2024 at 04:31:21PM +0200, Greg KH wrote:
> > > > > > > > On Fri, Oct 18, 2024 at 05:25:22PM +0300, Laurent Pinchart wrote:
> > > > > > > > > On Fri, Oct 18, 2024 at 04:09:26PM +0200, Greg KH wrote:
> > > > > > > > > > On Fri, Oct 18, 2024 at 03:36:48PM +0200, Geert Uytterhoeven wrote:
> > > > > > > > > > > On Fri, Oct 18, 2024 at 3:10 PM Laurent Pinchart wrote:
> > > > > > > > > > > > On Fri, Oct 18, 2024 at 09:45:52AM +0200, Geert Uytterhoeven wrote:
> > > > > > > > > > > > > Each bridge instance creates up to four auxiliary devices with different
> > > > > > > > > > > > > names.  However, their IDs are always zero, causing duplicate filename
> > > > > > > > > > > > > errors when a system has multiple bridges:
> > > > > > > > > > > > >
> > > > > > > > > > > > >     sysfs: cannot create duplicate filename '/bus/auxiliary/devices/ti_sn65dsi86.gpio.0'
> > > > > > > > > > > > >
> > > > > > > > > > > > > Fix this by using a unique instance ID per bridge instance.
> > > > > > > > > > > >
> > > > > > > > > > > > Isn't this something that should be handled by the AUX core ? The code
> > > > > > > > > > > > below would otherwise need to be duplicated by all drivers, which seems
> > > > > > > > > > > > a burden we should avoid.
> > > > > > > > > > >
> > > > > > > > > > > According to the documentation, this is the responsibility of the caller
> > > > > > > > > > > https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L81
> > > > > > > > > > > I believe this is the same for platform devices.
> > > > > > > > > > > See also the example at
> > > > > > > > > > > https://elixir.bootlin.com/linux/v6.11.4/source/include/linux/auxiliary_bus.h#L116
> > > > > > > > > > >
> > > > > > > > > > > Note: the platform bus supports PLATFORM_DEVID_AUTO, but the auxiliary
> > > > > > > > > > > bus does not.
> > > > > > > > > >
> > > > > > > > > > Yes, it does not as it's up to the caller to create a unique name, like
> > > > > > > > > > your patch here does.  I'd argue that platform should also not do
> > > > > > > > > > automatic device ids, but that's a different argument :)
> > > > > > > > >
> > > > > > > > > __auxiliary_device_add() creates the device name with
> > > > > > > > >
> > > > > > > > >   dev_set_name(dev, "%s.%s.%d", modname, auxdev->name, auxdev->id);
> > > > > > > > >
> > > > > > > > > I'm not calling for a PLATFORM_DEVID_AUTO-like feature here, but
> > > > > > > > > shouldn't the first component of the device name use the parent's name
> > > > > > > > > instead of the module name ?
> > > > > > > >
> > > > > > > > Why would the parent's name not be the module name?  That name is
> > > > > > > > guaranteed unique in the system.  If you want "uniqueness" within the
> > > > > > > > driver/module, use the name and id field please.
> > > > > > > >
> > > > > > > > That's worked well so far, but to be fair, aux devices are pretty new.
> > > > > > > > What problem is this naming scheme causing?
> > > > > > >
> > > > > > > Auxiliary devices are created as children of a parent device. When
> > > > > > > multiple instances of the same parent type exist, this will be reflected
> > > > > > > in the /sys/devices/ devices tree hierarchy without any issue. The
> > > > > > > problem comes from the fact the the auxiliary devices need a unique name
> > > > > > > for /sys/bus/auxialiary/devices/, where we somehow have to differenciate
> > > > > > > devices of identical types.
> > > > > > >
> > > > > > > Essentially, we're trying to summarize a whole hierarchy (path in
> > > > > > > /sys/devices/) into a single string. There are different ways to solve
> > > > > > > this. For platform devices, we use a device ID. For I2C devices, we use
> > > > > > > the parent's bus number. Other buses use different schemes.
> > > > > > >
> > > > > > > Geert's patch implements a mechanism in the ti-sn65dsi86 driver to
> > > > > > > handle this, and assign an id managed by the parent. In a sense we could
> > > > > > > consider this to be similar to what is done for I2C, where the bus
> > > > > > > number is also a property of the parent. However, the big difference is
> > > > > > > that the I2C bus number is managed by the I2C subsystem, while here the
> > > > > > > id is managed by the ti-sn65dsi86 driver, not by the auxiliary device
> > > > > > > core. This would require duplicating the same mechanism in every single
> > > > > > > driver creating auxiliary devices. This strikes me as a fairly bad idea.
> > > > > > > The problem should be solved by the core, not by individual drivers.
> > > > > >
> > > > > > The "id" is just a unique number, it is "managed" by the thing that is
> > > > > > creating the devices themselves, not the aux core code.  I don't see why
> > > > > > the i2c bus number has to match the same number that the ti driver
> > > > > > creates, it could be anything, as long as it doesn't match anything else
> > > > > > currently created by that driver.
> > > > >
> > > > > Laurent does not say it has to match the i2c bus number.
> > > > > He does think the auxilliary bus should provide a mechanism to
> > > > > allocate these IDs (e.g. usin g AUX_DEVID_AUTO?).
> > > >
> > > > As this is the first subsystem to ask for such a thing, I didn't think
> > > > it was needed, but the aux subsystem is new :)
> > > >
> > > > > However, using i2c_client->adapter->nr instead of ida_alloc()
> > > > > in the TI driver does sound like a good idea to me...
> > > >
> > > > Great!
> >
> > > With the I2C adapter numbers, that becomes:
> > >
> > >     /sys/bus/auxiliary/devices
> > >     ├── ti_sn65dsi86.gpio.1
> > >     ├── ti_sn65dsi86.pwm.1
> > >     ├── ti_sn65dsi86.aux.1
> > >     ├── ti_sn65dsi86.bridge.1
> > >     ├── ti_sn65dsi86.gpio.4
> > >     ├── ti_sn65dsi86.pwm.4
> > >     ├── ti_sn65dsi86.aux.4
> > >     └── ti_sn65dsi86.bridge.4
> > >
> > > > adapter->nr instead like other aux subsystems already do.
> >
> > Unfortunately the devil is in the details, as usual: there can be
> > multiple instances of the sn65dsi86 bridge on a single I2C bus,
> > so adapter->nr is not guaranteed to generate a unique name.
>
> In the case of sn65dsi86 I think we'd actually be OK. The TI bridge
> chip is always at bus address 0x2d so you can't have more than one on
> the same bus. Unless you added something funky atop it (like a mux of
> some sort) you might be OK.

It's 0x2c on mine ;-)

    8.5.1 Local I2C Interface Overview
    The 7-bit device address for SN65DSI86 is factory preset to 010110X
    with the least significant bit being determined by the ADDR control
    input.

> > Changing the auxiliary bus to use the parent's name instead of the
> > module name, as suggested by Laurent, would fix that.
>
> Right. On my system dev_name() of the sn65dsi86 device is "2-002d". If
> we had a second on i2c bus 4, we'd have:
>
>     /sys/bus/auxiliary/devices
>     ├── 2-002d.gpio.0
>     ├── 2-002d.pwm.0
>     ├── 2-002d.aux.0
>     ├── 2-002d.bridge.0
>     ├── 4-002d.gpio.0
>     ├── 4-002d.pwm.0
>     ├── 4-002d.aux.0
>     └── 4-002d.bridge.0
>
> ...and I think that's guaranteed to be unique because all the i2c
> devices are flat in "/sys/bus/i2c/devices".

Correct.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix multiple instances

[Doug Anderson]

Hi,

On Tue, Oct 22, 2024 at 12:12 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> > > > > > However, using i2c_client->adapter->nr instead of ida_alloc()
> > > > > > in the TI driver does sound like a good idea to me...
> > > > >
> > > > > Great!
> > >
> > > > With the I2C adapter numbers, that becomes:
> > > >
> > > >     /sys/bus/auxiliary/devices
> > > >     ├── ti_sn65dsi86.gpio.1
> > > >     ├── ti_sn65dsi86.pwm.1
> > > >     ├── ti_sn65dsi86.aux.1
> > > >     ├── ti_sn65dsi86.bridge.1
> > > >     ├── ti_sn65dsi86.gpio.4
> > > >     ├── ti_sn65dsi86.pwm.4
> > > >     ├── ti_sn65dsi86.aux.4
> > > >     └── ti_sn65dsi86.bridge.4
> > > >
> > > > > adapter->nr instead like other aux subsystems already do.
> > >
> > > Unfortunately the devil is in the details, as usual: there can be
> > > multiple instances of the sn65dsi86 bridge on a single I2C bus,
> > > so adapter->nr is not guaranteed to generate a unique name.
> >
> > In the case of sn65dsi86 I think we'd actually be OK. The TI bridge
> > chip is always at bus address 0x2d so you can't have more than one on
> > the same bus. Unless you added something funky atop it (like a mux of
> > some sort) you might be OK.
>
> It's 0x2c on mine ;-)
>
>     8.5.1 Local I2C Interface Overview
>     The 7-bit device address for SN65DSI86 is factory preset to 010110X
>     with the least significant bit being determined by the ADDR control
>     input.

Doh! I missed that in my search of the doc. I guess because they
decided to specify the address in binary in that part so my searching
for both the 7-bit and 8-bit I2C address didn't trigger. Oh well.


> > > Changing the auxiliary bus to use the parent's name instead of the
> > > module name, as suggested by Laurent, would fix that.
> >
> > Right. On my system dev_name() of the sn65dsi86 device is "2-002d". If
> > we had a second on i2c bus 4, we'd have:
> >
> >     /sys/bus/auxiliary/devices
> >     ├── 2-002d.gpio.0
> >     ├── 2-002d.pwm.0
> >     ├── 2-002d.aux.0
> >     ├── 2-002d.bridge.0
> >     ├── 4-002d.gpio.0
> >     ├── 4-002d.pwm.0
> >     ├── 4-002d.aux.0
> >     └── 4-002d.bridge.0
> >
> > ...and I think that's guaranteed to be unique because all the i2c
> > devices are flat in "/sys/bus/i2c/devices".
>
> Correct.

So given everything, using the dev_name() of the "parent" sounds
pretty good and seems like it addresses everyone's concerns. Was there
a part of the conversation where someone pointed out problems with it
that I missed? Is the next step to post a patch implementing that?
It'll change sysfs paths and dev names for everyone using AUX bus, but
presumably that's OK?

-Doug

Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix multiple instances

[Dmitry Baryshkov]

On Tue, Oct 22, 2024 at 07:37:01AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Tue, Oct 22, 2024 at 12:12 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> >
> > > > > > > However, using i2c_client->adapter->nr instead of ida_alloc()
> > > > > > > in the TI driver does sound like a good idea to me...
> > > > > >
> > > > > > Great!
> > > >
> > > > > With the I2C adapter numbers, that becomes:
> > > > >
> > > > >     /sys/bus/auxiliary/devices
> > > > >     ├── ti_sn65dsi86.gpio.1
> > > > >     ├── ti_sn65dsi86.pwm.1
> > > > >     ├── ti_sn65dsi86.aux.1
> > > > >     ├── ti_sn65dsi86.bridge.1
> > > > >     ├── ti_sn65dsi86.gpio.4
> > > > >     ├── ti_sn65dsi86.pwm.4
> > > > >     ├── ti_sn65dsi86.aux.4
> > > > >     └── ti_sn65dsi86.bridge.4
> > > > >
> > > > > > adapter->nr instead like other aux subsystems already do.
> > > >
> > > > Unfortunately the devil is in the details, as usual: there can be
> > > > multiple instances of the sn65dsi86 bridge on a single I2C bus,
> > > > so adapter->nr is not guaranteed to generate a unique name.
> > >
> > > In the case of sn65dsi86 I think we'd actually be OK. The TI bridge
> > > chip is always at bus address 0x2d so you can't have more than one on
> > > the same bus. Unless you added something funky atop it (like a mux of
> > > some sort) you might be OK.
> >
> > It's 0x2c on mine ;-)
> >
> >     8.5.1 Local I2C Interface Overview
> >     The 7-bit device address for SN65DSI86 is factory preset to 010110X
> >     with the least significant bit being determined by the ADDR control
> >     input.
> 
> Doh! I missed that in my search of the doc. I guess because they
> decided to specify the address in binary in that part so my searching
> for both the 7-bit and 8-bit I2C address didn't trigger. Oh well.
> 
> 
> > > > Changing the auxiliary bus to use the parent's name instead of the
> > > > module name, as suggested by Laurent, would fix that.
> > >
> > > Right. On my system dev_name() of the sn65dsi86 device is "2-002d". If
> > > we had a second on i2c bus 4, we'd have:
> > >
> > >     /sys/bus/auxiliary/devices
> > >     ├── 2-002d.gpio.0
> > >     ├── 2-002d.pwm.0
> > >     ├── 2-002d.aux.0
> > >     ├── 2-002d.bridge.0
> > >     ├── 4-002d.gpio.0
> > >     ├── 4-002d.pwm.0
> > >     ├── 4-002d.aux.0
> > >     └── 4-002d.bridge.0
> > >
> > > ...and I think that's guaranteed to be unique because all the i2c
> > > devices are flat in "/sys/bus/i2c/devices".
> >
> > Correct.
> 
> So given everything, using the dev_name() of the "parent" sounds
> pretty good and seems like it addresses everyone's concerns. Was there
> a part of the conversation where someone pointed out problems with it
> that I missed? Is the next step to post a patch implementing that?
> It'll change sysfs paths and dev names for everyone using AUX bus, but
> presumably that's OK?

It also requires changing in the way the auxiliary_match_id() works.
Currently matching is done using modname + ID.

So, maybe using MODNAME.NAME.parent-name.ID is better (e.g.
ti_sn65dsi86.gpio.2-002d.1). It will still require changes to the
match_id function, but they won't be that intrusive (one just has to
skip two parts of the name instead of skipping just one).

-- 
With best wishes
Dmitry

Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix multiple instances

[Geert Uytterhoeven]

Hi Dmitry,

On Mon, Oct 28, 2024 at 2:34 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
> On Tue, Oct 22, 2024 at 07:37:01AM -0700, Doug Anderson wrote:
> > On Tue, Oct 22, 2024 at 12:12 AM Geert Uytterhoeven
> > <geert@linux-m68k.org> wrote:
> > > > > > > > However, using i2c_client->adapter->nr instead of ida_alloc()
> > > > > > > > in the TI driver does sound like a good idea to me...
> > > > > > >
> > > > > > > Great!
> > > > >
> > > > > > With the I2C adapter numbers, that becomes:
> > > > > >
> > > > > >     /sys/bus/auxiliary/devices
> > > > > >     ├── ti_sn65dsi86.gpio.1
> > > > > >     ├── ti_sn65dsi86.pwm.1
> > > > > >     ├── ti_sn65dsi86.aux.1
> > > > > >     ├── ti_sn65dsi86.bridge.1
> > > > > >     ├── ti_sn65dsi86.gpio.4
> > > > > >     ├── ti_sn65dsi86.pwm.4
> > > > > >     ├── ti_sn65dsi86.aux.4
> > > > > >     └── ti_sn65dsi86.bridge.4
> > > > > >
> > > > > > > adapter->nr instead like other aux subsystems already do.
> > > > >
> > > > > Unfortunately the devil is in the details, as usual: there can be
> > > > > multiple instances of the sn65dsi86 bridge on a single I2C bus,
> > > > > so adapter->nr is not guaranteed to generate a unique name.
> > > >
> > > > In the case of sn65dsi86 I think we'd actually be OK. The TI bridge
> > > > chip is always at bus address 0x2d so you can't have more than one on
> > > > the same bus. Unless you added something funky atop it (like a mux of
> > > > some sort) you might be OK.
> > >
> > > It's 0x2c on mine ;-)
> > >
> > >     8.5.1 Local I2C Interface Overview
> > >     The 7-bit device address for SN65DSI86 is factory preset to 010110X
> > >     with the least significant bit being determined by the ADDR control
> > >     input.
> >
> > Doh! I missed that in my search of the doc. I guess because they
> > decided to specify the address in binary in that part so my searching
> > for both the 7-bit and 8-bit I2C address didn't trigger. Oh well.
> >
> > > > > Changing the auxiliary bus to use the parent's name instead of the
> > > > > module name, as suggested by Laurent, would fix that.
> > > >
> > > > Right. On my system dev_name() of the sn65dsi86 device is "2-002d". If
> > > > we had a second on i2c bus 4, we'd have:
> > > >
> > > >     /sys/bus/auxiliary/devices
> > > >     ├── 2-002d.gpio.0
> > > >     ├── 2-002d.pwm.0
> > > >     ├── 2-002d.aux.0
> > > >     ├── 2-002d.bridge.0
> > > >     ├── 4-002d.gpio.0
> > > >     ├── 4-002d.pwm.0
> > > >     ├── 4-002d.aux.0
> > > >     └── 4-002d.bridge.0
> > > >
> > > > ...and I think that's guaranteed to be unique because all the i2c
> > > > devices are flat in "/sys/bus/i2c/devices".
> > >
> > > Correct.
> >
> > So given everything, using the dev_name() of the "parent" sounds
> > pretty good and seems like it addresses everyone's concerns. Was there
> > a part of the conversation where someone pointed out problems with it
> > that I missed? Is the next step to post a patch implementing that?
> > It'll change sysfs paths and dev names for everyone using AUX bus, but
> > presumably that's OK?
>
> It also requires changing in the way the auxiliary_match_id() works.
> Currently matching is done using modname + ID.

Right, so just using the parent's name instead of modname won't work,
as the former is not a fixed string.

> So, maybe using MODNAME.NAME.parent-name.ID is better (e.g.
> ti_sn65dsi86.gpio.2-002d.1). It will still require changes to the
> match_id function, but they won't be that intrusive (one just has to
> skip two parts of the name instead of skipping just one).

IMHO this is becoming too complex. What if the parent's name contains
a period?

So just using ida_alloc() in the caller seems like the most
straight-forward solution.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix multiple instances

[Laurent Pinchart]

On Wed, Oct 30, 2024 at 11:25:40AM +0100, Geert Uytterhoeven wrote:
> On Mon, Oct 28, 2024 at 2:34 PM Dmitry Baryshkov wrote:
> > On Tue, Oct 22, 2024 at 07:37:01AM -0700, Doug Anderson wrote:
> > > On Tue, Oct 22, 2024 at 12:12 AM Geert Uytterhoeven wrote:
> > > > > > > > > However, using i2c_client->adapter->nr instead of ida_alloc()
> > > > > > > > > in the TI driver does sound like a good idea to me...
> > > > > > > >
> > > > > > > > Great!
> > > > > >
> > > > > > > With the I2C adapter numbers, that becomes:
> > > > > > >
> > > > > > >     /sys/bus/auxiliary/devices
> > > > > > >     ├── ti_sn65dsi86.gpio.1
> > > > > > >     ├── ti_sn65dsi86.pwm.1
> > > > > > >     ├── ti_sn65dsi86.aux.1
> > > > > > >     ├── ti_sn65dsi86.bridge.1
> > > > > > >     ├── ti_sn65dsi86.gpio.4
> > > > > > >     ├── ti_sn65dsi86.pwm.4
> > > > > > >     ├── ti_sn65dsi86.aux.4
> > > > > > >     └── ti_sn65dsi86.bridge.4
> > > > > > >
> > > > > > > > adapter->nr instead like other aux subsystems already do.
> > > > > >
> > > > > > Unfortunately the devil is in the details, as usual: there can be
> > > > > > multiple instances of the sn65dsi86 bridge on a single I2C bus,
> > > > > > so adapter->nr is not guaranteed to generate a unique name.
> > > > >
> > > > > In the case of sn65dsi86 I think we'd actually be OK. The TI bridge
> > > > > chip is always at bus address 0x2d so you can't have more than one on
> > > > > the same bus. Unless you added something funky atop it (like a mux of
> > > > > some sort) you might be OK.
> > > >
> > > > It's 0x2c on mine ;-)
> > > >
> > > >     8.5.1 Local I2C Interface Overview
> > > >     The 7-bit device address for SN65DSI86 is factory preset to 010110X
> > > >     with the least significant bit being determined by the ADDR control
> > > >     input.
> > >
> > > Doh! I missed that in my search of the doc. I guess because they
> > > decided to specify the address in binary in that part so my searching
> > > for both the 7-bit and 8-bit I2C address didn't trigger. Oh well.
> > >
> > > > > > Changing the auxiliary bus to use the parent's name instead of the
> > > > > > module name, as suggested by Laurent, would fix that.
> > > > >
> > > > > Right. On my system dev_name() of the sn65dsi86 device is "2-002d". If
> > > > > we had a second on i2c bus 4, we'd have:
> > > > >
> > > > >     /sys/bus/auxiliary/devices
> > > > >     ├── 2-002d.gpio.0
> > > > >     ├── 2-002d.pwm.0
> > > > >     ├── 2-002d.aux.0
> > > > >     ├── 2-002d.bridge.0
> > > > >     ├── 4-002d.gpio.0
> > > > >     ├── 4-002d.pwm.0
> > > > >     ├── 4-002d.aux.0
> > > > >     └── 4-002d.bridge.0
> > > > >
> > > > > ...and I think that's guaranteed to be unique because all the i2c
> > > > > devices are flat in "/sys/bus/i2c/devices".
> > > >
> > > > Correct.
> > >
> > > So given everything, using the dev_name() of the "parent" sounds
> > > pretty good and seems like it addresses everyone's concerns. Was there
> > > a part of the conversation where someone pointed out problems with it
> > > that I missed? Is the next step to post a patch implementing that?
> > > It'll change sysfs paths and dev names for everyone using AUX bus, but
> > > presumably that's OK?
> >
> > It also requires changing in the way the auxiliary_match_id() works.
> > Currently matching is done using modname + ID.
> 
> Right, so just using the parent's name instead of modname won't work,
> as the former is not a fixed string.
> 
> > So, maybe using MODNAME.NAME.parent-name.ID is better (e.g.
> > ti_sn65dsi86.gpio.2-002d.1). It will still require changes to the
> > match_id function, but they won't be that intrusive (one just has to
> > skip two parts of the name instead of skipping just one).
> 
> IMHO this is becoming too complex. What if the parent's name contains
> a period?
> 
> So just using ida_alloc() in the caller seems like the most
> straight-forward solution.

Why would we duplicate that in every user, when it should really be the
responsibility of the bus ? We need a better solution.

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm/bridge: ti-sn65dsi86: Fix multiple instances

[Dmitry Baryshkov]

On Wed, Oct 30, 2024 at 12:28:46PM +0200, Laurent Pinchart wrote:
> On Wed, Oct 30, 2024 at 11:25:40AM +0100, Geert Uytterhoeven wrote:
> > On Mon, Oct 28, 2024 at 2:34 PM Dmitry Baryshkov wrote:
> > > On Tue, Oct 22, 2024 at 07:37:01AM -0700, Doug Anderson wrote:
> > > > On Tue, Oct 22, 2024 at 12:12 AM Geert Uytterhoeven wrote:
> > > > > > > > > > However, using i2c_client->adapter->nr instead of ida_alloc()
> > > > > > > > > > in the TI driver does sound like a good idea to me...
> > > > > > > > >
> > > > > > > > > Great!
> > > > > > >
> > > > > > > > With the I2C adapter numbers, that becomes:
> > > > > > > >
> > > > > > > >     /sys/bus/auxiliary/devices
> > > > > > > >     ├── ti_sn65dsi86.gpio.1
> > > > > > > >     ├── ti_sn65dsi86.pwm.1
> > > > > > > >     ├── ti_sn65dsi86.aux.1
> > > > > > > >     ├── ti_sn65dsi86.bridge.1
> > > > > > > >     ├── ti_sn65dsi86.gpio.4
> > > > > > > >     ├── ti_sn65dsi86.pwm.4
> > > > > > > >     ├── ti_sn65dsi86.aux.4
> > > > > > > >     └── ti_sn65dsi86.bridge.4
> > > > > > > >
> > > > > > > > > adapter->nr instead like other aux subsystems already do.
> > > > > > >
> > > > > > > Unfortunately the devil is in the details, as usual: there can be
> > > > > > > multiple instances of the sn65dsi86 bridge on a single I2C bus,
> > > > > > > so adapter->nr is not guaranteed to generate a unique name.
> > > > > >
> > > > > > In the case of sn65dsi86 I think we'd actually be OK. The TI bridge
> > > > > > chip is always at bus address 0x2d so you can't have more than one on
> > > > > > the same bus. Unless you added something funky atop it (like a mux of
> > > > > > some sort) you might be OK.
> > > > >
> > > > > It's 0x2c on mine ;-)
> > > > >
> > > > >     8.5.1 Local I2C Interface Overview
> > > > >     The 7-bit device address for SN65DSI86 is factory preset to 010110X
> > > > >     with the least significant bit being determined by the ADDR control
> > > > >     input.
> > > >
> > > > Doh! I missed that in my search of the doc. I guess because they
> > > > decided to specify the address in binary in that part so my searching
> > > > for both the 7-bit and 8-bit I2C address didn't trigger. Oh well.
> > > >
> > > > > > > Changing the auxiliary bus to use the parent's name instead of the
> > > > > > > module name, as suggested by Laurent, would fix that.
> > > > > >
> > > > > > Right. On my system dev_name() of the sn65dsi86 device is "2-002d". If
> > > > > > we had a second on i2c bus 4, we'd have:
> > > > > >
> > > > > >     /sys/bus/auxiliary/devices
> > > > > >     ├── 2-002d.gpio.0
> > > > > >     ├── 2-002d.pwm.0
> > > > > >     ├── 2-002d.aux.0
> > > > > >     ├── 2-002d.bridge.0
> > > > > >     ├── 4-002d.gpio.0
> > > > > >     ├── 4-002d.pwm.0
> > > > > >     ├── 4-002d.aux.0
> > > > > >     └── 4-002d.bridge.0
> > > > > >
> > > > > > ...and I think that's guaranteed to be unique because all the i2c
> > > > > > devices are flat in "/sys/bus/i2c/devices".
> > > > >
> > > > > Correct.
> > > >
> > > > So given everything, using the dev_name() of the "parent" sounds
> > > > pretty good and seems like it addresses everyone's concerns. Was there
> > > > a part of the conversation where someone pointed out problems with it
> > > > that I missed? Is the next step to post a patch implementing that?
> > > > It'll change sysfs paths and dev names for everyone using AUX bus, but
> > > > presumably that's OK?
> > >
> > > It also requires changing in the way the auxiliary_match_id() works.
> > > Currently matching is done using modname + ID.
> > 
> > Right, so just using the parent's name instead of modname won't work,
> > as the former is not a fixed string.
> > 
> > > So, maybe using MODNAME.NAME.parent-name.ID is better (e.g.
> > > ti_sn65dsi86.gpio.2-002d.1). It will still require changes to the
> > > match_id function, but they won't be that intrusive (one just has to
> > > skip two parts of the name instead of skipping just one).
> > 
> > IMHO this is becoming too complex. What if the parent's name contains
> > a period?
> > 
> > So just using ida_alloc() in the caller seems like the most
> > straight-forward solution.
> 
> Why would we duplicate that in every user, when it should really be the
> responsibility of the bus ? We need a better solution.

Make AUX bus keep a hashtable of parent -> ID relationship and
get from the table / allocate via IDA the ID each time somebody adds new
AUX device?

-- 
With best wishes
Dmitry