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 at 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 at ravnborg.org>
> > > Cc: Joonas Kylmälä <joonas.kylmala at iki.fi>
> > > Cc: Andrzej Hajda <a.hajda at samsung.com>
> > > Cc: Thierry Reding <thierry.reding at gmail.com>
> > > Cc: Inki Dae <inki.dae at samsung.com>
> > > Cc: Hyungwon Hwang <human.hwang at samsung.com>
> > > Cc: Hoegeun Kwon <hoegeun.kwon at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch