[PATCH v3] drm/bridge: ti-sn65dsi86: Check bridge connection failure

[PATCH v3] drm/bridge: ti-sn65dsi86: Check bridge connection failure

[Wolfram Sang]

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(+)

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
-- 
2.47.2

Re: [PATCH v3] drm/bridge: ti-sn65dsi86: Check bridge connection failure

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 820 bytes --]

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"


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3] drm/bridge: ti-sn65dsi86: Check bridge connection failure

[Doug Anderson]

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>

Re: [PATCH v3] drm/bridge: ti-sn65dsi86: Check bridge connection failure

[Doug Anderson]

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

Re: [PATCH v3] drm/bridge: ti-sn65dsi86: Check bridge connection failure

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3] drm/bridge: ti-sn65dsi86: Check bridge connection failure

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 570 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3] drm/bridge: ti-sn65dsi86: Check bridge connection failure

[Laurent Pinchart]

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().

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3] drm/bridge: ti-sn65dsi86: Check bridge connection failure

[Doug Anderson]

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

Re: [PATCH v3] drm/bridge: ti-sn65dsi86: Check bridge connection failure

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3] drm/bridge: ti-sn65dsi86: Check bridge connection failure

[Doug Anderson]

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