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

[PATCH v2] 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.  The
instance ID is derived from the I2C adapter number and the bridge's I2C
address, to support multiple instances on the same bus.

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>
---
On the White Hawk development board:

    /sys/bus/auxiliary/devices/
    |-- ti_sn65dsi86.aux.1068
    |-- ti_sn65dsi86.aux.4140
    |-- ti_sn65dsi86.bridge.1068
    |-- ti_sn65dsi86.bridge.4140
    |-- ti_sn65dsi86.gpio.1068
    |-- ti_sn65dsi86.gpio.4140
    |-- ti_sn65dsi86.pwm.1068
    `-- ti_sn65dsi86.pwm.4140

Discussion after v1:
  - https://lore.kernel.org/8c2df6a903f87d4932586b25f1d3bd548fe8e6d1.1729180470.git.geert+renesas@glider.be

Notes:
  - While the bridge supports only two possible I2C addresses, I2C
    translators may be present, increasing the address space.  Hence the
    instance ID calculation assumes 10-bit addressing.  Perhaps it makes
    sense to introduce a global I2C helper function for this?

  - I think this is the simplest solution.  If/when the auxiliary bus
    receives support à la PLATFORM_DEVID_AUTO, the driver can be
    updated.

v2:
  - Use I2C adapter/address instead of ida_alloc().
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 9e31f750fd889745..fb452d1b46995673 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -480,6 +480,7 @@ static int ti_sn65dsi86_add_aux_device(struct ti_sn65dsi86 *pdata,
 				       const char *name)
 {
 	struct device *dev = pdata->dev;
+	const struct i2c_client *client = to_i2c_client(dev);
 	struct auxiliary_device *aux;
 	int ret;
 
@@ -488,6 +489,7 @@ static int ti_sn65dsi86_add_aux_device(struct ti_sn65dsi86 *pdata,
 		return -ENOMEM;
 
 	aux->name = name;
+	aux->id = (client->adapter->nr << 10) | client->addr;
 	aux->dev.parent = dev;
 	aux->dev.release = ti_sn65dsi86_aux_device_release;
 	device_set_of_node_from_dev(&aux->dev, dev);
-- 
2.34.1

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

[Laurent Pinchart]

On Tue, Dec 10, 2024 at 03:18:46PM +0100, 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.  The
> instance ID is derived from the I2C adapter number and the bridge's I2C
> address, to support multiple instances on the same bus.
> 
> 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>
> ---
> On the White Hawk development board:
> 
>     /sys/bus/auxiliary/devices/
>     |-- ti_sn65dsi86.aux.1068
>     |-- ti_sn65dsi86.aux.4140
>     |-- ti_sn65dsi86.bridge.1068
>     |-- ti_sn65dsi86.bridge.4140
>     |-- ti_sn65dsi86.gpio.1068
>     |-- ti_sn65dsi86.gpio.4140
>     |-- ti_sn65dsi86.pwm.1068
>     `-- ti_sn65dsi86.pwm.4140
> 
> Discussion after v1:
>   - https://lore.kernel.org/8c2df6a903f87d4932586b25f1d3bd548fe8e6d1.1729180470.git.geert+renesas@glider.be
> 
> Notes:
>   - While the bridge supports only two possible I2C addresses, I2C
>     translators may be present, increasing the address space.  Hence the
>     instance ID calculation assumes 10-bit addressing.  Perhaps it makes
>     sense to introduce a global I2C helper function for this?
> 
>   - I think this is the simplest solution.  If/when the auxiliary bus
>     receives support à la PLATFORM_DEVID_AUTO, the driver can be
>     updated.

As I'd like to get a proper solution for this I'm not going to ack this
patch, but I won't nack it either.

> v2:
>   - Use I2C adapter/address instead of ida_alloc().
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 9e31f750fd889745..fb452d1b46995673 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -480,6 +480,7 @@ static int ti_sn65dsi86_add_aux_device(struct ti_sn65dsi86 *pdata,
>  				       const char *name)
>  {
>  	struct device *dev = pdata->dev;
> +	const struct i2c_client *client = to_i2c_client(dev);
>  	struct auxiliary_device *aux;
>  	int ret;
>  
> @@ -488,6 +489,7 @@ static int ti_sn65dsi86_add_aux_device(struct ti_sn65dsi86 *pdata,
>  		return -ENOMEM;
>  
>  	aux->name = name;
> +	aux->id = (client->adapter->nr << 10) | client->addr;
>  	aux->dev.parent = dev;
>  	aux->dev.release = ti_sn65dsi86_aux_device_release;
>  	device_set_of_node_from_dev(&aux->dev, dev);

-- 
Regards,

Laurent Pinchart

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

[Doug Anderson]

Hi,

On Tue, Dec 10, 2024 at 6:19 AM Geert Uytterhoeven
<geert+renesas@glider.be> 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.  The
> instance ID is derived from the I2C adapter number and the bridge's I2C
> address, to support multiple instances on the same bus.
>
> 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>
> ---
> On the White Hawk development board:
>
>     /sys/bus/auxiliary/devices/
>     |-- ti_sn65dsi86.aux.1068
>     |-- ti_sn65dsi86.aux.4140
>     |-- ti_sn65dsi86.bridge.1068
>     |-- ti_sn65dsi86.bridge.4140
>     |-- ti_sn65dsi86.gpio.1068
>     |-- ti_sn65dsi86.gpio.4140
>     |-- ti_sn65dsi86.pwm.1068
>     `-- ti_sn65dsi86.pwm.4140
>
> Discussion after v1:
>   - https://lore.kernel.org/8c2df6a903f87d4932586b25f1d3bd548fe8e6d1.1729180470.git.geert+renesas@glider.be
>
> Notes:
>   - While the bridge supports only two possible I2C addresses, I2C
>     translators may be present, increasing the address space.  Hence the
>     instance ID calculation assumes 10-bit addressing.  Perhaps it makes
>     sense to introduce a global I2C helper function for this?
>
>   - I think this is the simplest solution.  If/when the auxiliary bus
>     receives support à la PLATFORM_DEVID_AUTO, the driver can be
>     updated.
>
> v2:
>   - Use I2C adapter/address instead of ida_alloc().
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 2 ++
>  1 file changed, 2 insertions(+)

While I agree with Laurent that having a more automatic solution would
be nice, this is small and fixes a real problem. I'd be of the opinion
that we should land it.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

If I personally end up being the person to land it, I'll likely wait
until January since I'll be on vacation soon for the holidays and I
don't want to check something that's slightly controversial in and
then disappear. If someone else feels it's ready to land before then I
have no objections.

-Doug

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

[Geert Uytterhoeven]

Hi Doug,

On Tue, Dec 10, 2024 at 6:09 PM Doug Anderson <dianders@chromium.org> wrote:
> On Tue, Dec 10, 2024 at 6:19 AM Geert Uytterhoeven
> <geert+renesas@glider.be> 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.  The
> > instance ID is derived from the I2C adapter number and the bridge's I2C
> > address, to support multiple instances on the same bus.
> >
> > 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>
> > ---
> > On the White Hawk development board:
> >
> >     /sys/bus/auxiliary/devices/
> >     |-- ti_sn65dsi86.aux.1068
> >     |-- ti_sn65dsi86.aux.4140
> >     |-- ti_sn65dsi86.bridge.1068
> >     |-- ti_sn65dsi86.bridge.4140
> >     |-- ti_sn65dsi86.gpio.1068
> >     |-- ti_sn65dsi86.gpio.4140
> >     |-- ti_sn65dsi86.pwm.1068
> >     `-- ti_sn65dsi86.pwm.4140
> >
> > Discussion after v1:
> >   - https://lore.kernel.org/8c2df6a903f87d4932586b25f1d3bd548fe8e6d1.1729180470.git.geert+renesas@glider.be
> >
> > Notes:
> >   - While the bridge supports only two possible I2C addresses, I2C
> >     translators may be present, increasing the address space.  Hence the
> >     instance ID calculation assumes 10-bit addressing.  Perhaps it makes
> >     sense to introduce a global I2C helper function for this?
> >
> >   - I think this is the simplest solution.  If/when the auxiliary bus
> >     receives support à la PLATFORM_DEVID_AUTO, the driver can be
> >     updated.
> >
> > v2:
> >   - Use I2C adapter/address instead of ida_alloc().
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 2 ++
> >  1 file changed, 2 insertions(+)
>
> While I agree with Laurent that having a more automatic solution would
> be nice, this is small and fixes a real problem. I'd be of the opinion
> that we should land it.
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Thanks!

> If I personally end up being the person to land it, I'll likely wait
> until January since I'll be on vacation soon for the holidays and I
> don't want to check something that's slightly controversial in and
> then disappear. If someone else feels it's ready to land before then I
> have no objections.

There is no need to hurry. The only board I have that needs this has
another issue in its second display pipeline, which will require a
new driver no one is working on yet.

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 v2] drm/bridge: ti-sn65dsi86: Fix multiple instances

[Doug Anderson]

Hi

On Wed, Dec 11, 2024 at 12:27 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Doug,
>
> On Tue, Dec 10, 2024 at 6:09 PM Doug Anderson <dianders@chromium.org> wrote:
> > On Tue, Dec 10, 2024 at 6:19 AM Geert Uytterhoeven
> > <geert+renesas@glider.be> 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.  The
> > > instance ID is derived from the I2C adapter number and the bridge's I2C
> > > address, to support multiple instances on the same bus.
> > >
> > > Fixes: bf73537f411b0d4f ("drm/bridge: ti-sn65dsi86: Break GPIO and MIPI-to-eDP bridge into sub-drivers")

When I applied the patch, the DRM tools ran checkpatch in strict mode
which pointed out that you have too many digits in your "Fixes" hash.
I've adjusted them to make checkpatch happy.


> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > ---
> > > On the White Hawk development board:
> > >
> > >     /sys/bus/auxiliary/devices/
> > >     |-- ti_sn65dsi86.aux.1068
> > >     |-- ti_sn65dsi86.aux.4140
> > >     |-- ti_sn65dsi86.bridge.1068
> > >     |-- ti_sn65dsi86.bridge.4140
> > >     |-- ti_sn65dsi86.gpio.1068
> > >     |-- ti_sn65dsi86.gpio.4140
> > >     |-- ti_sn65dsi86.pwm.1068
> > >     `-- ti_sn65dsi86.pwm.4140
> > >
> > > Discussion after v1:
> > >   - https://lore.kernel.org/8c2df6a903f87d4932586b25f1d3bd548fe8e6d1.1729180470.git.geert+renesas@glider.be
> > >
> > > Notes:
> > >   - While the bridge supports only two possible I2C addresses, I2C
> > >     translators may be present, increasing the address space.  Hence the
> > >     instance ID calculation assumes 10-bit addressing.  Perhaps it makes
> > >     sense to introduce a global I2C helper function for this?
> > >
> > >   - I think this is the simplest solution.  If/when the auxiliary bus
> > >     receives support à la PLATFORM_DEVID_AUTO, the driver can be
> > >     updated.
> > >
> > > v2:
> > >   - Use I2C adapter/address instead of ida_alloc().
> > > ---
> > >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> >
> > While I agree with Laurent that having a more automatic solution would
> > be nice, this is small and fixes a real problem. I'd be of the opinion
> > that we should land it.
> >
> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
>
> Thanks!
>
> > If I personally end up being the person to land it, I'll likely wait
> > until January since I'll be on vacation soon for the holidays and I
> > don't want to check something that's slightly controversial in and
> > then disappear. If someone else feels it's ready to land before then I
> > have no objections.
>
> There is no need to hurry. The only board I have that needs this has
> another issue in its second display pipeline, which will require a
> new driver no one is working on yet.

As promised, I've landed this. In this case I've landed in
drm-misc-next. Even though it's a fix since it didn't sound urgent
enough to land in drm-misc-fixes. Since it changes sysfs paths
slightly, it feels like it would be good to give it extra bake time
and not rush it as a fix.

[1/1] drm/bridge: ti-sn65dsi86: Fix multiple instances
      commit: 574f5ee2c85a00a579549d50e9fc9c6c072ee4c4

-Doug

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

[Geert Uytterhoeven]

Hi Doug,

On Tue, Jan 7, 2025 at 6:27 PM Doug Anderson <dianders@chromium.org> wrote:
> On Wed, Dec 11, 2024 at 12:27 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
> > On Tue, Dec 10, 2024 at 6:09 PM Doug Anderson <dianders@chromium.org> wrote:
> > > On Tue, Dec 10, 2024 at 6:19 AM Geert Uytterhoeven
> > > <geert+renesas@glider.be> 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.  The
> > > > instance ID is derived from the I2C adapter number and the bridge's I2C
> > > > address, to support multiple instances on the same bus.
> > > >
> > > > Fixes: bf73537f411b0d4f ("drm/bridge: ti-sn65dsi86: Break GPIO and MIPI-to-eDP bridge into sub-drivers")
>
> When I applied the patch, the DRM tools ran checkpatch in strict mode
> which pointed out that you have too many digits in your "Fixes" hash.
> I've adjusted them to make checkpatch happy.

So the DRM tools don't use the latest version from linux-next yet...
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/scripts/checkpatch.pl?id=6356f18f09dc0781650c4f128ea48745fa48c415

> > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > ---
> > > > On the White Hawk development board:
> > > >
> > > >     /sys/bus/auxiliary/devices/
> > > >     |-- ti_sn65dsi86.aux.1068
> > > >     |-- ti_sn65dsi86.aux.4140
> > > >     |-- ti_sn65dsi86.bridge.1068
> > > >     |-- ti_sn65dsi86.bridge.4140
> > > >     |-- ti_sn65dsi86.gpio.1068
> > > >     |-- ti_sn65dsi86.gpio.4140
> > > >     |-- ti_sn65dsi86.pwm.1068
> > > >     `-- ti_sn65dsi86.pwm.4140
> > > >
> > > > Discussion after v1:
> > > >   - https://lore.kernel.org/8c2df6a903f87d4932586b25f1d3bd548fe8e6d1.1729180470.git.geert+renesas@glider.be
> > > >
> > > > Notes:
> > > >   - While the bridge supports only two possible I2C addresses, I2C
> > > >     translators may be present, increasing the address space.  Hence the
> > > >     instance ID calculation assumes 10-bit addressing.  Perhaps it makes
> > > >     sense to introduce a global I2C helper function for this?
> > > >
> > > >   - I think this is the simplest solution.  If/when the auxiliary bus
> > > >     receives support à la PLATFORM_DEVID_AUTO, the driver can be
> > > >     updated.
> > > >
> > > > v2:
> > > >   - Use I2C adapter/address instead of ida_alloc().
> > > > ---
> > > >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > >
> > > While I agree with Laurent that having a more automatic solution would
> > > be nice, this is small and fixes a real problem. I'd be of the opinion
> > > that we should land it.
> > >
> > > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> >
> > Thanks!
> >
> > > If I personally end up being the person to land it, I'll likely wait
> > > until January since I'll be on vacation soon for the holidays and I
> > > don't want to check something that's slightly controversial in and
> > > then disappear. If someone else feels it's ready to land before then I
> > > have no objections.
> >
> > There is no need to hurry. The only board I have that needs this has
> > another issue in its second display pipeline, which will require a
> > new driver no one is working on yet.
>
> As promised, I've landed this. In this case I've landed in
> drm-misc-next. Even though it's a fix since it didn't sound urgent
> enough to land in drm-misc-fixes. Since it changes sysfs paths
> slightly, it feels like it would be good to give it extra bake time
> and not rush it as a fix.
>
> [1/1] drm/bridge: ti-sn65dsi86: Fix multiple instances
>       commit: 574f5ee2c85a00a579549d50e9fc9c6c072ee4c4

Thanks!

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