| Message ID | 7a68a0e3f927e26edca6040067fb653eb06efb79.1733840089.git.geert+renesas@glider.be |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] drm/bridge: ti-sn65dsi86: Fix multiple instances | expand |
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
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
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);
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(+)