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