| Message ID | 20250318155549.19625-2-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | [v3] drm/bridge: ti-sn65dsi86: Check bridge connection failure | expand |
On Tue, Mar 18, 2025 at 04:52:56PM +0100, Wolfram Sang wrote: > Read out and check the ID registers, so we can bail out if I2C > communication does not work or if the device is unknown. Tested on a > Renesas GrayHawk board (R-Car V4M) by using a wrong I2C address and by > not enabling RuntimePM for the device. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > Changes since v2: > * switched to a new approach suggested by Doug (Thanks!). We add a > dedicated read instead of using the first read. This prevents creating > the aux devices. As a side-gain, we check now if the chip at the address > is really the one we want to support. Forgot to mention that it depends on my previous patch "[PATCH v2] drm/bridge: ti-sn65dsi86: make use of debugfs_init callback"
Hi, On Tue, Mar 18, 2025 at 8:56 AM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > Read out and check the ID registers, so we can bail out if I2C > communication does not work or if the device is unknown. Tested on a > Renesas GrayHawk board (R-Car V4M) by using a wrong I2C address and by > not enabling RuntimePM for the device. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > Changes since v2: > * switched to a new approach suggested by Doug (Thanks!). We add a > dedicated read instead of using the first read. This prevents creating > the aux devices. As a side-gain, we check now if the chip at the address > is really the one we want to support. > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) Looks good to me. Reviewed-by: Douglas Anderson <dianders@chromium.org>
Hi Wolfram, Thank you for the patch. On Tue, Mar 18, 2025 at 04:52:56PM +0100, Wolfram Sang wrote: > Read out and check the ID registers, so we can bail out if I2C > communication does not work or if the device is unknown. What's the advantage of that, what are you trying to guard against ? > Tested on a > Renesas GrayHawk board (R-Car V4M) by using a wrong I2C address and by > not enabling RuntimePM for the device. What do you mean by not enabling runtime PM for the device ? > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > Changes since v2: > * switched to a new approach suggested by Doug (Thanks!). We add a > dedicated read instead of using the first read. This prevents creating > the aux devices. As a side-gain, we check now if the chip at the address > is really the one we want to support. > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index 87fffaa52bb0..8caa7918933d 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -36,6 +36,7 @@ > #include <drm/drm_print.h> > #include <drm/drm_probe_helper.h> > > +#define SN_DEVICE_ID_REGS 0x00 /* up to 0x07 */ > #define SN_DEVICE_REV_REG 0x08 > #define SN_DPPLL_SRC_REG 0x0A > #define DPPLL_CLK_SRC_DSICLK BIT(0) > @@ -1875,6 +1876,7 @@ static int ti_sn65dsi86_probe(struct i2c_client *client) > { > struct device *dev = &client->dev; > struct ti_sn65dsi86 *pdata; > + u8 id_buf[8]; > int ret; > > if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { > @@ -1918,6 +1920,16 @@ static int ti_sn65dsi86_probe(struct i2c_client *client) > if (ret) > return ret; > > + pm_runtime_get_sync(dev); Missing error checking. You should probably use pm_runtime_resume_and_get(). > + ret = regmap_bulk_read(pdata->regmap, SN_DEVICE_ID_REGS, id_buf, ARRAY_SIZE(id_buf)); > + pm_runtime_put_autosuspend(dev); > + if (ret) > + return dev_err_probe(dev, ret, "failed to read device id\n"); > + > + /* The ID string is stored backwards */ > + if (strncmp(id_buf, "68ISD ", ARRAY_SIZE(id_buf))) > + return dev_err_probe(dev, -EOPNOTSUPP, "unsupported device id\n"); > + > /* > * Break ourselves up into a collection of aux devices. The only real > * motiviation here is to solve the chicken-and-egg problem of probe
Hi Laurent, > > Read out and check the ID registers, so we can bail out if I2C > > communication does not work or if the device is unknown. > > What's the advantage of that, what are you trying to guard against ? That a random chip at address 0x2c will be used. > > Tested on a > > Renesas GrayHawk board (R-Car V4M) by using a wrong I2C address and by > > not enabling RuntimePM for the device. > > What do you mean by not enabling runtime PM for the device ? I left out pm_runtime_get() before regmap_read_bulk(). Happy hacking, Wolfram
Hi, On Tue, Mar 18, 2025 at 10:56 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Tue, Mar 18, 2025 at 8:56 AM Wolfram Sang > <wsa+renesas@sang-engineering.com> wrote: > > > > Read out and check the ID registers, so we can bail out if I2C > > communication does not work or if the device is unknown. Tested on a > > Renesas GrayHawk board (R-Car V4M) by using a wrong I2C address and by > > not enabling RuntimePM for the device. > > > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > > --- > > > > Changes since v2: > > * switched to a new approach suggested by Doug (Thanks!). We add a > > dedicated read instead of using the first read. This prevents creating > > the aux devices. As a side-gain, we check now if the chip at the address > > is really the one we want to support. > > > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > Looks good to me. > > Reviewed-by: Douglas Anderson <dianders@chromium.org> Pushed to drm-misc-next: [1/1] drm/bridge: ti-sn65dsi86: Check bridge connection failure commit: d69362f55fba92eb4cac10fe8da618de52b49bfc
On Tue, Mar 18, 2025 at 10:00:31PM +0100, Wolfram Sang wrote: > Hi Laurent, > > > > Read out and check the ID registers, so we can bail out if I2C > > > communication does not work or if the device is unknown. > > > > What's the advantage of that, what are you trying to guard against ? > > That a random chip at address 0x2c will be used. Is that really a problem ? That would only occur with a broken DT, is it worth guarding against a development-time issue with a runtime check that will increase boot time for every user ? > > > Tested on a > > > Renesas GrayHawk board (R-Car V4M) by using a wrong I2C address and by > > > not enabling RuntimePM for the device. > > > > What do you mean by not enabling runtime PM for the device ? > > I left out pm_runtime_get() before regmap_read_bulk().
Hi, On Mon, Mar 24, 2025 at 9:40 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Tue, Mar 18, 2025 at 10:00:31PM +0100, Wolfram Sang wrote: > > Hi Laurent, > > > > > > Read out and check the ID registers, so we can bail out if I2C > > > > communication does not work or if the device is unknown. > > > > > > What's the advantage of that, what are you trying to guard against ? > > > > That a random chip at address 0x2c will be used. > > Is that really a problem ? That would only occur with a broken DT, is it > worth guarding against a development-time issue with a runtime check > that will increase boot time for every user ? FWIW, this can also happen simply due to broken / damaged hardware. If a board gets stressed and causes a pin to become disconnected or if a regulator ages and stops providing power then we can also end up in this state. Getting a nice obvious error at probe when the device isn't responding at all can make problems like this much easier to debug. It's not uncommon for i2c devices to probe to make sure the device is really there at bootup. Checking for the full 8-byte ID is probably a bit overkill, but at the same time if we're going to probe something the ID is not a terrible thing to probe and reading 8 bytes won't really take much longer than reading 1. -Doug
On Mon, Mar 24, 2025 at 10:17:05AM -0700, Doug Anderson wrote: > On Mon, Mar 24, 2025 at 9:40 AM Laurent Pinchart wrote: > > On Tue, Mar 18, 2025 at 10:00:31PM +0100, Wolfram Sang wrote: > > > Hi Laurent, > > > > > > > > Read out and check the ID registers, so we can bail out if I2C > > > > > communication does not work or if the device is unknown. > > > > > > > > What's the advantage of that, what are you trying to guard against ? > > > > > > That a random chip at address 0x2c will be used. > > > > Is that really a problem ? That would only occur with a broken DT, is it > > worth guarding against a development-time issue with a runtime check > > that will increase boot time for every user ? > > FWIW, this can also happen simply due to broken / damaged hardware. If > a board gets stressed and causes a pin to become disconnected or if a > regulator ages and stops providing power then we can also end up in > this state. Getting a nice obvious error at probe when the device > isn't responding at all can make problems like this much easier to > debug. I'm not fully convinced it's worth it, compared to an error at runtime, and especially given that there are way more pins than supplies or SCL/SDA that could suffer from a similar issue. I won't block the patch, but I think it's overkill. > It's not uncommon for i2c devices to probe to make sure the > device is really there at bootup. Checking for the full 8-byte ID is > probably a bit overkill, but at the same time if we're going to probe > something the ID is not a terrible thing to probe and reading 8 bytes > won't really take much longer than reading 1.
Hi, On Mon, Mar 24, 2025 at 10:31 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Mon, Mar 24, 2025 at 10:17:05AM -0700, Doug Anderson wrote: > > On Mon, Mar 24, 2025 at 9:40 AM Laurent Pinchart wrote: > > > On Tue, Mar 18, 2025 at 10:00:31PM +0100, Wolfram Sang wrote: > > > > Hi Laurent, > > > > > > > > > > Read out and check the ID registers, so we can bail out if I2C > > > > > > communication does not work or if the device is unknown. > > > > > > > > > > What's the advantage of that, what are you trying to guard against ? > > > > > > > > That a random chip at address 0x2c will be used. > > > > > > Is that really a problem ? That would only occur with a broken DT, is it > > > worth guarding against a development-time issue with a runtime check > > > that will increase boot time for every user ? > > > > FWIW, this can also happen simply due to broken / damaged hardware. If > > a board gets stressed and causes a pin to become disconnected or if a > > regulator ages and stops providing power then we can also end up in > > this state. Getting a nice obvious error at probe when the device > > isn't responding at all can make problems like this much easier to > > debug. > > I'm not fully convinced it's worth it, compared to an error at runtime, > and especially given that there are way more pins than supplies or > SCL/SDA that could suffer from a similar issue. I won't block the patch, > but I think it's overkill. FWIW, Wolfram's previous patch [1] did check it at runtime without a dedicated i2c transfer. The problem was that it only handled one of the sub-AUX-devices and left the other sub-AUX-devices dangling. Many of the sub-AUX-devices didn't need to talk to the chip at probe time so there wasn't a good way to make the "probe" fail for them. ...so in this situation you'd end up with every GPIO operation or PWM operation failing and the device would still exist. It didn't seem ideal to me... [1] http://lore.kernel.org/r/20250314224202.13128-2-wsa+renesas@sang-engineering.com
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 87fffaa52bb0..8caa7918933d 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -36,6 +36,7 @@ #include <drm/drm_print.h> #include <drm/drm_probe_helper.h> +#define SN_DEVICE_ID_REGS 0x00 /* up to 0x07 */ #define SN_DEVICE_REV_REG 0x08 #define SN_DPPLL_SRC_REG 0x0A #define DPPLL_CLK_SRC_DSICLK BIT(0) @@ -1875,6 +1876,7 @@ static int ti_sn65dsi86_probe(struct i2c_client *client) { struct device *dev = &client->dev; struct ti_sn65dsi86 *pdata; + u8 id_buf[8]; int ret; if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { @@ -1918,6 +1920,16 @@ static int ti_sn65dsi86_probe(struct i2c_client *client) if (ret) return ret; + pm_runtime_get_sync(dev); + ret = regmap_bulk_read(pdata->regmap, SN_DEVICE_ID_REGS, id_buf, ARRAY_SIZE(id_buf)); + pm_runtime_put_autosuspend(dev); + if (ret) + return dev_err_probe(dev, ret, "failed to read device id\n"); + + /* The ID string is stored backwards */ + if (strncmp(id_buf, "68ISD ", ARRAY_SIZE(id_buf))) + return dev_err_probe(dev, -EOPNOTSUPP, "unsupported device id\n"); + /* * Break ourselves up into a collection of aux devices. The only real * motiviation here is to solve the chicken-and-egg problem of probe
Read out and check the ID registers, so we can bail out if I2C communication does not work or if the device is unknown. Tested on a Renesas GrayHawk board (R-Car V4M) by using a wrong I2C address and by not enabling RuntimePM for the device. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- Changes since v2: * switched to a new approach suggested by Doug (Thanks!). We add a dedicated read instead of using the first read. This prevents creating the aux devices. As a side-gain, we check now if the chip at the address is really the one we want to support. drivers/gpu/drm/bridge/ti-sn65dsi86.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)