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

From: Emil Velikov <emil.l.velikov@gmail.com>
To: Sam Ravnborg <sam@ravnborg.org>
Cc: "Joonas Kylmälä" <joonas.kylmala@iki.fi>,
	"ML dri-devel" <dri-devel@lists.freedesktop.org>,
	"Andrzej Hajda" <a.hajda@samsung.com>,
	"Thierry Reding" <thierry.reding@gmail.com>,
	"Hyungwon Hwang" <human.hwang@samsung.com>,
	"Hoegeun Kwon" <hoegeun.kwon@samsung.com>,
	"Paweł Chmiel" <pawel.mikolaj.chmiel@gmail.com>
Subject: Re: [PATCH v1 2/3] drm/panel: update backlight handling for samsung-s6e63j0x03
Date: Tue, 5 May 2020 15:30:24 +0100	[thread overview]
Message-ID: <CACvgo51YSx5SsnLONAbSdixqQnWCDASAqJKRde2ckNZE4YuCZg@mail.gmail.com> (raw)
In-Reply-To: <20200409144613.GA5396@ravnborg.org>

On Thu, 9 Apr 2020 at 15:46, Sam Ravnborg <sam@ravnborg.org> 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.
>
AFAICT the new states are actually more consistent.

> I will summarize the above in the individual changelogs.
>
That'll be perfect thanks.

Meanwhile I'll try to finish my `cleanup bd->props states` series \o/

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