[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 at 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 at 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 at linux.intel.com>
> 
> Otherwise, I'm sending v2 with your review tag by the end of the week.

Thanks,
Andi