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@xxxxxxxxxxxxxxxxxxxx>
> ---
>
> 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
