| Message ID | 20250314224202.13128-2-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/bridge: ti-sn65dsi86: Check bridge connection failure | expand |
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
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
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
> 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.
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;