| Message ID | 20240808-ti-sn65dsi83-dev_err_probe-v1-1-72417aa275ab@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: ti-sn65dsi83: use dev_err_probe when failing to get panel bridge | expand |
Hi Luca, On Thu, Aug 08, 2024 at 12:26:14PM +0200, Luca Ceresoli wrote: > When devm_drm_of_get_bridge() fails, the probe fails silently. Use > dev_err_probe() instead to log an error or report the deferral reason, > whichever is applicable. > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > --- > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > index 57a7ed13f996..60b9f14d769a 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > @@ -606,7 +606,7 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model) > > panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 2, 0); > if (IS_ERR(panel_bridge)) > - return PTR_ERR(panel_bridge); > + return dev_err_probe(dev, PTR_ERR(panel_bridge), "Failed to get panel bridge\n"); patch looks good, but the message is a bit misleading. You are not failing to get the panel bridge, but you are failing to find a panel bridge in a DT node. Right? Andi
Hello Andi, On Thu, 8 Aug 2024 11:35:23 +0100 Andi Shyti <andi.shyti@linux.intel.com> wrote: > Hi Luca, > > On Thu, Aug 08, 2024 at 12:26:14PM +0200, Luca Ceresoli wrote: > > When devm_drm_of_get_bridge() fails, the probe fails silently. Use > > dev_err_probe() instead to log an error or report the deferral reason, > > whichever is applicable. > > > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > > --- > > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > index 57a7ed13f996..60b9f14d769a 100644 > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > @@ -606,7 +606,7 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model) > > > > panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 2, 0); > > if (IS_ERR(panel_bridge)) > > - return PTR_ERR(panel_bridge); > > + return dev_err_probe(dev, PTR_ERR(panel_bridge), "Failed to get panel bridge\n"); > > patch looks good, but the message is a bit misleading. You are > not failing to get the panel bridge, but you are failing to find > a panel bridge in a DT node. Right? As I can see from both the documentation and the code, devm_drm_of_get_bridge() is really returning a pointer to a panel bridge, potentially allocating and adding it in case it was not present before. Navigating the device tree is only a part of what it does. Do you think I am missing something? Luca
Hi Luca, On Tue, Aug 13, 2024 at 10:16:43AM +0200, Luca Ceresoli wrote: > On Thu, 8 Aug 2024 11:35:23 +0100 > Andi Shyti <andi.shyti@linux.intel.com> wrote: > > On Thu, Aug 08, 2024 at 12:26:14PM +0200, Luca Ceresoli wrote: > > > When devm_drm_of_get_bridge() fails, the probe fails silently. Use > > > dev_err_probe() instead to log an error or report the deferral reason, > > > whichever is applicable. > > > > > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > > > --- > > > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > index 57a7ed13f996..60b9f14d769a 100644 > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > @@ -606,7 +606,7 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model) > > > > > > panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 2, 0); > > > if (IS_ERR(panel_bridge)) > > > - return PTR_ERR(panel_bridge); > > > + return dev_err_probe(dev, PTR_ERR(panel_bridge), "Failed to get panel bridge\n"); > > > > patch looks good, but the message is a bit misleading. You are > > not failing to get the panel bridge, but you are failing to find > > a panel bridge in a DT node. Right? > > As I can see from both the documentation and the code, > devm_drm_of_get_bridge() is really returning a pointer to a panel > bridge, potentially allocating and adding it in case it was not present > before. Navigating the device tree is only a part of what it does. > > Do you think I am missing something? No, maybe it's me being a bit pedantic. In the sense that we are not really failing to get the panel, but most probably the panel is not installed. I'm not strong on this comment, though, so that feel free to add: Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Thanks, Andi
Hello Andi, thanks for your additional comments. On Tue, 13 Aug 2024 16:32:50 +0200 Andi Shyti <andi.shyti@linux.intel.com> wrote: > Hi Luca, > > On Tue, Aug 13, 2024 at 10:16:43AM +0200, Luca Ceresoli wrote: > > On Thu, 8 Aug 2024 11:35:23 +0100 > > Andi Shyti <andi.shyti@linux.intel.com> wrote: > > > On Thu, Aug 08, 2024 at 12:26:14PM +0200, Luca Ceresoli wrote: > > > > When devm_drm_of_get_bridge() fails, the probe fails silently. Use > > > > dev_err_probe() instead to log an error or report the deferral reason, > > > > whichever is applicable. > > > > > > > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > > > > --- > > > > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > > index 57a7ed13f996..60b9f14d769a 100644 > > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > > @@ -606,7 +606,7 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model) > > > > > > > > panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 2, 0); > > > > if (IS_ERR(panel_bridge)) > > > > - return PTR_ERR(panel_bridge); > > > > + return dev_err_probe(dev, PTR_ERR(panel_bridge), "Failed to get panel bridge\n"); > > > > > > patch looks good, but the message is a bit misleading. You are > > > not failing to get the panel bridge, but you are failing to find > > > a panel bridge in a DT node. Right? > > > > As I can see from both the documentation and the code, > > devm_drm_of_get_bridge() is really returning a pointer to a panel > > bridge, potentially allocating and adding it in case it was not present > > before. Navigating the device tree is only a part of what it does. > > > > Do you think I am missing something? > > No, maybe it's me being a bit pedantic. In the sense that we are > not really failing to get the panel, but most probably the panel > is not installed. The panels I'm used to, which I believe to be the most common in embedded systems just have no way of being detected, so the operating system cannot detect a "panel not installed" condition. However I went back to the code and realized your initial remark ("you are failing to find a panel bridge in a DT node") is more correct than I initially thought. Indeed there are two failure reasons for devm_drm_of_get_bridge() to fail: DT lookup and panel bridge creation failures. The latter however can be due to -ENOMEM (unlikely) or (panel->connector_type == DRM_MODE_CONNECTOR_Unknown), which in turn can be due to either a panel driver error or again a DT error in case the driver gets the panel type from DT, as panel-simple.c does. That said, the role of devm_drm_of_get_bridge() is to provide a panel bridge object. If it fails, that means it is unable to provide such an object for whatever reason. Reasons currently include DT issues (the most likely), driver bug and -ENOMEM. There could be more reasons in future versions of the implementation. I'm afraid I'm unable to express all the above logic in a single commit title line. However, should you have a better commit title or message to suggest, I'm still open to improvements. I value good commit messages. > I'm not strong on this comment, though, so that > feel free to add: > > Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Otherwise, I'm sending v2 with your review tag by the end of the week. Luca
Hi Luca, On Mon, Aug 19, 2024 at 01:38:40PM +0200, Luca Ceresoli wrote: > On Tue, 13 Aug 2024 16:32:50 +0200 > > On Tue, Aug 13, 2024 at 10:16:43AM +0200, Luca Ceresoli wrote: > > > On Thu, 8 Aug 2024 11:35:23 +0100 > > > Andi Shyti <andi.shyti@linux.intel.com> wrote: > > > > On Thu, Aug 08, 2024 at 12:26:14PM +0200, Luca Ceresoli wrote: > > > > > When devm_drm_of_get_bridge() fails, the probe fails silently. Use > > > > > dev_err_probe() instead to log an error or report the deferral reason, > > > > > whichever is applicable. > > > > > > > > > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> > > > > > --- > > > > > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > > > index 57a7ed13f996..60b9f14d769a 100644 > > > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > > > @@ -606,7 +606,7 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model) > > > > > > > > > > panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 2, 0); > > > > > if (IS_ERR(panel_bridge)) > > > > > - return PTR_ERR(panel_bridge); > > > > > + return dev_err_probe(dev, PTR_ERR(panel_bridge), "Failed to get panel bridge\n"); > > > > > > > > patch looks good, but the message is a bit misleading. You are > > > > not failing to get the panel bridge, but you are failing to find > > > > a panel bridge in a DT node. Right? > > > > > > As I can see from both the documentation and the code, > > > devm_drm_of_get_bridge() is really returning a pointer to a panel > > > bridge, potentially allocating and adding it in case it was not present > > > before. Navigating the device tree is only a part of what it does. > > > > > > Do you think I am missing something? > > > > No, maybe it's me being a bit pedantic. In the sense that we are > > not really failing to get the panel, but most probably the panel > > is not installed. > > The panels I'm used to, which I believe to be the most common in > embedded systems just have no way of being detected, so the operating > system cannot detect a "panel not installed" condition. > > However I went back to the code and realized your initial remark ("you > are failing to find a panel bridge in a DT node") is more correct than > I initially thought. Indeed there are two failure reasons for > devm_drm_of_get_bridge() to fail: DT lookup and panel bridge creation > failures. The latter however can be due to -ENOMEM (unlikely) or > (panel->connector_type == DRM_MODE_CONNECTOR_Unknown), which in turn > can be due to either a panel driver error or again a DT error in case > the driver gets the panel type from DT, as panel-simple.c does. > > That said, the role of devm_drm_of_get_bridge() is to provide a panel > bridge object. If it fails, that means it is unable to provide such an > object for whatever reason. Reasons currently include DT issues (the > most likely), driver bug and -ENOMEM. There could be more reasons in > future versions of the implementation. > > I'm afraid I'm unable to express all the above logic in a single commit > title line. However, should you have a better commit title or message > to suggest, I'm still open to improvements. I value good commit > messages. yes, that's all correct... I'm just assuming that we don't fail for enomem's or similar. But if you want to include them, then a generic "get" might work. To be honest, I wouldn't know how to write it better :-D Writing error messages is skill per se. Maybe something like ... "Failed to get panel bridge from DT (%pe)", panel_bridge); Fact is that an error message should immidiately tell you what is failing and you understand without browsing the code. A generic "Failed to get..." says very little. A use case can be if you receive a bug report. If someone tells you "Failed to get..." you will need to start diggin on the report. While if someone tells you "Failed to get panel bridge from DT (-ENODEV)" you would immediately tell him to add the panel in the configuration. But... as I said... > > I'm not strong on this comment, though, so that > > feel free to add: ... this is a nitpick, feel free to ignore it. > > Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> > > Otherwise, I'm sending v2 with your review tag by the end of the week. Thanks, Andi
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index 57a7ed13f996..60b9f14d769a 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c @@ -606,7 +606,7 @@ static int sn65dsi83_parse_dt(struct sn65dsi83 *ctx, enum sn65dsi83_model model) panel_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 2, 0); if (IS_ERR(panel_bridge)) - return PTR_ERR(panel_bridge); + return dev_err_probe(dev, PTR_ERR(panel_bridge), "Failed to get panel bridge\n"); ctx->panel_bridge = panel_bridge;
When devm_drm_of_get_bridge() fails, the probe fails silently. Use dev_err_probe() instead to log an error or report the deferral reason, whichever is applicable. Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com> --- drivers/gpu/drm/bridge/ti-sn65dsi83.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- base-commit: 8400291e289ee6b2bf9779ff1c83a291501f017b change-id: 20240808-ti-sn65dsi83-dev_err_probe-98daf0ddafb1 Best regards,