| Message ID | 20250318155549.19625-2-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | New |
| 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
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(+)