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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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