[PATCH] drm/bridge: ti-sn65dsi83: use dev_err_probe when failing to get panel bridge

[PATCH] drm/bridge: ti-sn65dsi83: use dev_err_probe when failing to get panel bridge

[Luca Ceresoli]

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");
 
 	ctx->panel_bridge = panel_bridge;
 

---
base-commit: 8400291e289ee6b2bf9779ff1c83a291501f017b
change-id: 20240808-ti-sn65dsi83-dev_err_probe-98daf0ddafb1

Best regards,
-- 
Luca Ceresoli <luca.ceresoli@bootlin.com>

Re: [PATCH] drm/bridge: ti-sn65dsi83: use dev_err_probe when failing to get panel bridge

[Andi Shyti]

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

Re: [PATCH] drm/bridge: ti-sn65dsi83: use dev_err_probe when failing to get panel bridge

[Luca Ceresoli]

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

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] drm/bridge: ti-sn65dsi83: use dev_err_probe when failing to get panel bridge

[Andi Shyti]

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

Re: [PATCH] drm/bridge: ti-sn65dsi83: use dev_err_probe when failing to get panel bridge

[Luca Ceresoli]

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

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] drm/bridge: ti-sn65dsi83: use dev_err_probe when failing to get panel bridge

[Andi Shyti]

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