Re: [PATCH v1 2/3] drm/panel: update backlight handling for samsung-s6e63j0x03

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Apr 09, 2020 at 04:46:13PM +0200, Sam Ravnborg wrote:
> Hi Emil.
> 
> Thanks for your feedback!
> 
> On Thu, Apr 09, 2020 at 03:13:28PM +0100, Emil Velikov wrote:
> > On Thu, 9 Apr 2020 at 12:53, Sam Ravnborg <sam@ravnborg.org> wrote:
> > >
> > > The samsung-s6e63j0x03 had a local way to handle backlight.
> > >
> > > Update the driver to use a devm_ based register function
> > > and utilize drm_panel backlight support. The changes results
> > > in a simpler driver with the same functionality.
> > >
> > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > > Cc: Joonas Kylmälä <joonas.kylmala@iki.fi>
> > > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > > Cc: Thierry Reding <thierry.reding@gmail.com>
> > > Cc: Inki Dae <inki.dae@samsung.com>
> > > Cc: Hyungwon Hwang <human.hwang@samsung.com>
> > > Cc: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> > > ---
> > >  .../gpu/drm/panel/panel-samsung-s6e63j0x03.c  | 55 ++++++++++---------
> > >  1 file changed, 29 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c b/drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c
> > > index a3570e0a90a8..2c035f87e3f0 100644
> > > --- a/drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c
> > > +++ b/drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c
> > > @@ -36,7 +36,6 @@
> > >  struct s6e63j0x03 {
> > >         struct device *dev;
> > >         struct drm_panel panel;
> > > -       struct backlight_device *bl_dev;
> > >
> > >         struct regulator_bulk_data supplies[2];
> > >         struct gpio_desc *reset_gpio;
> > > @@ -184,7 +183,7 @@ static unsigned int s6e63j0x03_get_brightness_index(unsigned int brightness)
> > >  static int s6e63j0x03_update_gamma(struct s6e63j0x03 *ctx,
> > >                                         unsigned int brightness)
> > >  {
> > > -       struct backlight_device *bl_dev = ctx->bl_dev;
> > > +       struct backlight_device *bl_dev = ctx->panel.backlight;
> > >         unsigned int index = s6e63j0x03_get_brightness_index(brightness);
> > >         int ret;
> > >
> > > @@ -217,6 +216,30 @@ static const struct backlight_ops s6e63j0x03_bl_ops = {
> > >         .update_status = s6e63j0x03_set_brightness,
> > >  };
> > >
> > > +static int s6e63j0x03_backlight_register(struct s6e63j0x03 *ctx)
> > > +{
> > > +       struct backlight_properties props = {
> > Pretty sure we can (should really) make the props const.
> Thanks, will fix either in v2 or when I apply.
> 
> > 
> > Quick grep through drm, shows that there're other offenders, so might
> > as well do that in separate series.
> > Seems like other panels could follow suite, with later series of course.
> > 
> > Back on topic, it's not immediately obvious why the FB_BLANK_*
> > handling is safe to remove. Please add small mention in the commit log
> > mentioning why.
> 
> Maybe because it is not so?
> Lets take a closer look.
> backlight_enable() and backlight_disable() are called from
> drm_panel - because drm_panel->backlight is assigned.
> 
> 
> drm_panel_prepare:
> OLD:	ctx->bl_dev->props.power = FB_BLANK_NORMAL;
> NEW:
> 
> drm_panel_enable:
> OLD:	ctx->bl_dev->props.power = FB_BLANK_UNBLANK;
> NEW:	backlight_enable() => 
> 		bd->props.power = FB_BLANK_UNBLANK;
> 		bd->props.fb_blank = FB_BLANK_UNBLANK;
> 	        bd->props.state &= ~BL_CORE_FBBLANK;
> 
> drm_panel_disable:
> OLD:	ctx->bl_dev->props.power = FB_BLANK_NORMAL;
> NEW:	backlight_disable() =>
> 		bd->props.power = FB_BLANK_POWERDOWN;
> 	        bd->props.fb_blank = FB_BLANK_POWERDOWN;
>         	bd->props.state |= BL_CORE_FBBLANK;
> 
> 
> drm_panel_unprepare:
> OLD:	ctx->bl_dev->props.power = FB_BLANK_POWERDOWN;
> NEW:
> 
> So old and new code are not exactly the same.
> 
> But with my (limited) backlight understanding this should
> work as expected - and it works for many other drivers.
> So if this does not work, then we should look at the backlight
> handling and not do workarounds in the driver.
> 
> I will summarize the above in the individual changelogs.

This is a long-term conversion even listed in the todo.rst. Backlight has
3 different flags that control on/off, and it's a complete mess. There's
uber-arcane rules as to in which order backlight drivers should be
following them (I think it's a logical and, but not sure), the goal is to
condense it all down to 1 on/off switch. todo.rst has the full plan.

So moving stuff over to backlight_enable/disable() functions is Very Good.
-Daniel

> 
> 	Sam
> 
> 
> > 
> > -Emil
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel