Re: [PATCH v3 02/21] drm/panel: panel-simple: add default connector_type

2020-07-11 Thread Sam Ravnborg
Hi Laurent.
On Sat, Jul 11, 2020 at 01:11:24AM +0300, Laurent Pinchart wrote:
> Hi Sam,
> 
> Thank you for the patch.
> 
> On Fri, Jul 03, 2020 at 09:23:58PM +0200, Sam Ravnborg wrote:
> > All panels shall report a connector type.
> > panel-simple has a lot of panels with no connector_type,
> > and for these fall back to DPI as the default.
> > 
> > Signed-off-by: Sam Ravnborg 
> > Cc: Thierry Reding 
> > Cc: Sam Ravnborg 
> > ---
> >  drivers/gpu/drm/panel/panel-simple.c | 10 --
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panel/panel-simple.c 
> > b/drivers/gpu/drm/panel/panel-simple.c
> > index 7b5d212215e0..b3ec965721b0 100644
> > --- a/drivers/gpu/drm/panel/panel-simple.c
> > +++ b/drivers/gpu/drm/panel/panel-simple.c
> > @@ -500,6 +500,7 @@ static int panel_simple_probe(struct device *dev, const 
> > struct panel_desc *desc)
> > struct panel_simple *panel;
> > struct display_timing dt;
> > struct device_node *ddc;
> > +   int connector_type;
> > int err;
> >  
> > panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL);
> > @@ -566,8 +567,13 @@ static int panel_simple_probe(struct device *dev, 
> > const struct panel_desc *desc)
> > desc->bpc != 8);
> > }
> >  
> > -   drm_panel_init(>base, dev, _simple_funcs,
> > -  desc->connector_type);
> > +   /* Default DRM_MODE_CONNECTOR_DPI if no connector_type is set */
> > +   if (desc->connector_type != 0)
> > +   connector_type = desc->connector_type;
> > +   else
> > +   connector_type = DRM_MODE_CONNECTOR_DPI;
> 
> This avoids a WARN_ON(), which is good, but should we add a dev_warn()
> to get this fixed ?

We need a proper check for all relevant properties for DPI
and the other connectors. Like you already did for LVDS.

The idea is we shall introduce the checks in one series,
so users when they fix the warnings they are all good.

And then we will have to catch less during review which is good.

It is on my TODO list, but wanted to have some other stuff done
first.

So in other words, good to go with this patch or do we need the proper
checks in place first?

Sam

> 
> > +
> > +   drm_panel_init(>base, dev, _simple_funcs, connector_type);
> >  
> > err = drm_panel_of_backlight(>base);
> > if (err)
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH v3 02/21] drm/panel: panel-simple: add default connector_type

2020-07-10 Thread Laurent Pinchart
Hi Sam,

Thank you for the patch.

On Fri, Jul 03, 2020 at 09:23:58PM +0200, Sam Ravnborg wrote:
> All panels shall report a connector type.
> panel-simple has a lot of panels with no connector_type,
> and for these fall back to DPI as the default.
> 
> Signed-off-by: Sam Ravnborg 
> Cc: Thierry Reding 
> Cc: Sam Ravnborg 
> ---
>  drivers/gpu/drm/panel/panel-simple.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c 
> b/drivers/gpu/drm/panel/panel-simple.c
> index 7b5d212215e0..b3ec965721b0 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -500,6 +500,7 @@ static int panel_simple_probe(struct device *dev, const 
> struct panel_desc *desc)
>   struct panel_simple *panel;
>   struct display_timing dt;
>   struct device_node *ddc;
> + int connector_type;
>   int err;
>  
>   panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL);
> @@ -566,8 +567,13 @@ static int panel_simple_probe(struct device *dev, const 
> struct panel_desc *desc)
>   desc->bpc != 8);
>   }
>  
> - drm_panel_init(>base, dev, _simple_funcs,
> -desc->connector_type);
> + /* Default DRM_MODE_CONNECTOR_DPI if no connector_type is set */
> + if (desc->connector_type != 0)
> + connector_type = desc->connector_type;
> + else
> + connector_type = DRM_MODE_CONNECTOR_DPI;

This avoids a WARN_ON(), which is good, but should we add a dev_warn()
to get this fixed ?

> +
> + drm_panel_init(>base, dev, _simple_funcs, connector_type);
>  
>   err = drm_panel_of_backlight(>base);
>   if (err)

-- 
Regards,

Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel