| Message ID | 20240619102219.138927-3-jacopo.mondi@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | drm: rcar-du: Add support for R8A779H0 | expand |
Hi Jacopo, Thank you for the patch. On Wed, Jun 19, 2024 at 12:22:16PM +0200, Jacopo Mondi wrote: > From: Phong Hoang <phong.hoang.wz@renesas.com> > > Add a check to the register access function when attaching a bridge > device. > > 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> > --- > 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 84698a0b27a8..b7df53577987 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; >
On 19/06/2024 22:32, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Wed, Jun 19, 2024 at 12:22:16PM +0200, Jacopo Mondi wrote: >> From: Phong Hoang <phong.hoang.wz@renesas.com> >> >> Add a check to the register access function when attaching a bridge >> device. I think the desc is missing the "why". I'm guessing it's the first register access to the IC, and thus verifies that it is accessible. Tomi >> >> 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> > >> --- >> 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 84698a0b27a8..b7df53577987 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; >> >
On Thu, Jun 20, 2024 at 09:43:05AM +0300, Tomi Valkeinen wrote: > On 19/06/2024 22:32, Laurent Pinchart wrote: > > Hi Jacopo, > > > > Thank you for the patch. > > > > On Wed, Jun 19, 2024 at 12:22:16PM +0200, Jacopo Mondi wrote: > >> From: Phong Hoang <phong.hoang.wz@renesas.com> > >> > >> Add a check to the register access function when attaching a bridge > >> device. > > I think the desc is missing the "why". I'm guessing it's the first > register access to the IC, and thus verifies that it is accessible. Isn't it a good idea in general to always check if I2C reads succeeded ? > >> > >> 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> > > > >> --- > >> 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 84698a0b27a8..b7df53577987 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; > >>
On 20/06/2024 13:42, Laurent Pinchart wrote: > On Thu, Jun 20, 2024 at 09:43:05AM +0300, Tomi Valkeinen wrote: >> On 19/06/2024 22:32, Laurent Pinchart wrote: >>> Hi Jacopo, >>> >>> Thank you for the patch. >>> >>> On Wed, Jun 19, 2024 at 12:22:16PM +0200, Jacopo Mondi wrote: >>>> From: Phong Hoang <phong.hoang.wz@renesas.com> >>>> >>>> Add a check to the register access function when attaching a bridge >>>> device. >> >> I think the desc is missing the "why". I'm guessing it's the first >> register access to the IC, and thus verifies that it is accessible. > > Isn't it a good idea in general to always check if I2C reads succeeded ? It is. But if there are tens of other i2c accesses for which the return value is ignored, the question remains: why this single one was specifically fixed? Tomi > >>>> >>>> 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> >>> >>>> --- >>>> 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 84698a0b27a8..b7df53577987 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; >>>> >
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 84698a0b27a8..b7df53577987 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;