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

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

[Wolfram Sang]

From: Phong Hoang <phong.hoang.wz@renesas.com>

Add a check to the very first register access function when attaching a
bridge device, so we can bail out if I2C communication does not work.

Signed-off-by: Phong Hoang <phong.hoang.wz@renesas.com>
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Build tested only!

Changes since v1:

* rebased to v6.14-rc6
* add Laurent's tag (Thanks!)
* update patch description according to Tomi's suggestions (Thanks!)

 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index e4d9006b59f1..59508e82a991 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -696,6 +696,7 @@ static struct ti_sn65dsi86 *bridge_to_ti_sn65dsi86(struct drm_bridge *bridge)
 
 static int ti_sn_attach_host(struct auxiliary_device *adev, struct ti_sn65dsi86 *pdata)
 {
+	int ret;
 	int val;
 	struct mipi_dsi_host *host;
 	struct mipi_dsi_device *dsi;
@@ -720,8 +721,11 @@ static int ti_sn_attach_host(struct auxiliary_device *adev, struct ti_sn65dsi86
 
 	/* check if continuous dsi clock is required or not */
 	pm_runtime_get_sync(dev);
-	regmap_read(pdata->regmap, SN_DPPLL_SRC_REG, &val);
+	ret = regmap_read(pdata->regmap, SN_DPPLL_SRC_REG, &val);
 	pm_runtime_put_autosuspend(dev);
+	if (ret)
+		return ret;
+
 	if (!(val & DPPLL_CLK_SRC_DSICLK))
 		dsi->mode_flags |= MIPI_DSI_CLOCK_NON_CONTINUOUS;
 
-- 
2.47.2

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

[Doug Anderson]

Hi,

On Fri, Mar 14, 2025 at 3:42 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> From: Phong Hoang <phong.hoang.wz@renesas.com>
>
> Add a check to the very first register access function when attaching a
> bridge device, so we can bail out if I2C communication does not work.
>
> Signed-off-by: Phong Hoang <phong.hoang.wz@renesas.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>
> Build tested only!
>
> Changes since v1:
>
> * rebased to v6.14-rc6
> * add Laurent's tag (Thanks!)
> * update patch description according to Tomi's suggestions (Thanks!)
>
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index e4d9006b59f1..59508e82a991 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -696,6 +696,7 @@ static struct ti_sn65dsi86 *bridge_to_ti_sn65dsi86(struct drm_bridge *bridge)
>
>  static int ti_sn_attach_host(struct auxiliary_device *adev, struct ti_sn65dsi86 *pdata)
>  {
> +       int ret;
>         int val;
>         struct mipi_dsi_host *host;
>         struct mipi_dsi_device *dsi;
> @@ -720,8 +721,11 @@ static int ti_sn_attach_host(struct auxiliary_device *adev, struct ti_sn65dsi86
>
>         /* check if continuous dsi clock is required or not */
>         pm_runtime_get_sync(dev);
> -       regmap_read(pdata->regmap, SN_DPPLL_SRC_REG, &val);
> +       ret = regmap_read(pdata->regmap, SN_DPPLL_SRC_REG, &val);
>         pm_runtime_put_autosuspend(dev);
> +       if (ret)
> +               return ret;

Seems reasonable to me. I would probably put an error message in this
case, though? I don't think regmap_read() necessarily prints an error
so it would just be a mysterious failure for why things didn't probe,
right?

This also only solves the problems for one of the 4 devices in this
file. I think the GPIO device, PWM device, and DP-AUX device will
still confusingly stick around. It's probably better to add a bogus
regmap read in ti_sn65dsi86_probe().

-Doug

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

[Wolfram Sang]

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

Hi Doug,

> Seems reasonable to me. I would probably put an error message in this
> case, though? I don't think regmap_read() necessarily prints an error
> so it would just be a mysterious failure for why things didn't probe,
> right?

OK, can do that.

> This also only solves the problems for one of the 4 devices in this
> file. I think the GPIO device, PWM device, and DP-AUX device will
> still confusingly stick around. It's probably better to add a bogus
> regmap read in ti_sn65dsi86_probe().

Could be argued. I just wonder about all the other error cases in
ti_sn_attach_host() plus ti_sn_bridge_probe(). They have the same
problem with the other devices dangling?

Happy hacking,

   Wolfram


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

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

[Doug Anderson]

Hi,

On Sat, Mar 15, 2025 at 2:00 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> Hi Doug,
>
> > Seems reasonable to me. I would probably put an error message in this
> > case, though? I don't think regmap_read() necessarily prints an error
> > so it would just be a mysterious failure for why things didn't probe,
> > right?
>
> OK, can do that.
>
> > This also only solves the problems for one of the 4 devices in this
> > file. I think the GPIO device, PWM device, and DP-AUX device will
> > still confusingly stick around. It's probably better to add a bogus
> > regmap read in ti_sn65dsi86_probe().
>
> Could be argued. I just wonder about all the other error cases in
> ti_sn_attach_host() plus ti_sn_bridge_probe(). They have the same
> problem with the other devices dangling?

They do, though the other ones don't seem quite the same to me. It
feels like an i2c device not actually showing up on the bus is a
_somewhat_ normal/expected error. It also is an error that would
affect all of the other sub-devices, so making sure they're not
dangling is something that is worth doing. It feels like other errors
that ti_sn_attach_host() plus ti_sn_bridge_probe() are likely to face
are either unexpected (like a small memory allocation failing or an
i2c transfer failing after a previous one succeeded) or are errors
where leaving the dangling devices isn't really a problem. For
instance: if somehow you fail to link up to a MIPI source then it
feels OK that the GPIO / PWM and DP-AUX bus parts of the bridge
continue to dangle. You can still use them, right?

-Doug

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

[Wolfram Sang]

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


> where leaving the dangling devices isn't really a problem. For
> instance: if somehow you fail to link up to a MIPI source then it
> feels OK that the GPIO / PWM and DP-AUX bus parts of the bridge
> continue to dangle. You can still use them, right?

Ok, convinced. Thanks for the input. Will send another version.


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