[PATCH v1 0/3] drm/panel: update backlight support for samsung panels

[PATCH v1 0/3] drm/panel: update backlight support for samsung panels

[Sam Ravnborg]

Update the backlight handling in three samsung panels.
  - Use devm_ functions to automate cleanup
  - Utilize backlight support in drm_panel
  - Introduce a register backlight helper in all panels
    Using the same pattern in all panels makes it simpler
    to read the code.

With this change the backlight handling in these panels
is now much more like what we see in other panels.

Testing or any other feedback appreciated!

	Sam


Sam Ravnborg (3):
      drm/panel: update backlight handling for samsung-s6e3ha2
      drm/panel: update backlight handling for samsung-s6e63j0x03
      drm/panel: update backlight handling for samsung-s6e63m0

 drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c    | 55 +++++++++++++-----------
 drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c | 55 +++++++++++++-----------
 drivers/gpu/drm/panel/panel-samsung-s6e63m0.c    | 20 ++++-----
 3 files changed, 68 insertions(+), 62 deletions(-)


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

[PATCH v1 1/3] drm/panel: update backlight handling for samsung-s6e3ha2

[Sam Ravnborg]

The samsung-s6e3ha2 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>
---
 drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c | 55 +++++++++++--------
 1 file changed, 31 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c b/drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c
index 36ebd5a4ac7b..08862146aec3 100644
--- a/drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c
+++ b/drivers/gpu/drm/panel/panel-samsung-s6e3ha2.c
@@ -233,7 +233,6 @@ struct s6e3ha2_panel_desc {
 struct s6e3ha2 {
 	struct device *dev;
 	struct drm_panel panel;
-	struct backlight_device *bl_dev;
 
 	struct regulator_bulk_data supplies[2];
 	struct gpio_desc *reset_gpio;
@@ -415,7 +414,7 @@ static int s6e3ha2_get_brightness(struct backlight_device *bl_dev)
 
 static int s6e3ha2_set_vint(struct s6e3ha2 *ctx)
 {
-	struct backlight_device *bl_dev = ctx->bl_dev;
+	struct backlight_device *bl_dev = ctx->panel.backlight;
 	unsigned int brightness = bl_dev->props.brightness;
 	unsigned char data[] = { 0xf4, 0x8b,
 			vint_table[brightness * (S6E3HA2_VINT_STATUS_MAX - 1) /
@@ -432,7 +431,7 @@ static unsigned int s6e3ha2_get_brightness_index(unsigned int brightness)
 
 static int s6e3ha2_update_gamma(struct s6e3ha2 *ctx, unsigned int brightness)
 {
-	struct backlight_device *bl_dev = ctx->bl_dev;
+	struct backlight_device *bl_dev = ctx->panel.backlight;
 	unsigned int index = s6e3ha2_get_brightness_index(brightness);
 	u8 data[S6E3HA2_GAMMA_CMD_CNT + 1] = { 0xca, };
 	int ret;
@@ -476,6 +475,30 @@ static const struct backlight_ops s6e3ha2_bl_ops = {
 	.update_status = s6e3ha2_set_brightness,
 };
 
+static int s6e3ha2_backlight_register(struct s6e3ha2 *ctx)
+{
+	struct backlight_properties props = {
+		.type		= BACKLIGHT_RAW,
+		.brightness	= S6E3HA2_DEFAULT_BRIGHTNESS,
+		.max_brightness	= S6E3HA2_MAX_BRIGHTNESS,
+	};
+	struct backlight_device *backlight;
+	struct device *dev = ctx->dev;
+	int ret = 0;
+
+	backlight = devm_backlight_device_register(dev, "panel", dev, ctx,
+						   &s6e3ha2_bl_ops,
+						   &props);
+	if (IS_ERR(backlight)) {
+		ret = PTR_ERR(backlight);
+		DRM_DEV_ERROR(dev, "error registering backlight device (%d)\n",
+			      ret);
+	}
+	ctx->panel.backlight = backlight;
+
+	return ret;
+}
+
 static int s6e3ha2_panel_init(struct s6e3ha2 *ctx)
 {
 	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
@@ -509,7 +532,6 @@ static int s6e3ha2_disable(struct drm_panel *panel)
 	s6e3ha2_call_write_func(ret, mipi_dsi_dcs_set_display_off(dsi));
 
 	msleep(40);
-	ctx->bl_dev->props.power = FB_BLANK_NORMAL;
 
 	return 0;
 }
@@ -555,8 +577,6 @@ static int s6e3ha2_prepare(struct drm_panel *panel)
 	if (ret < 0)
 		goto err;
 
-	ctx->bl_dev->props.power = FB_BLANK_NORMAL;
-
 	return 0;
 
 err:
@@ -588,7 +608,6 @@ static int s6e3ha2_enable(struct drm_panel *panel)
 	s6e3ha2_call_write_func(ret, s6e3ha2_te_start_setting(ctx));
 
 	/* brightness setting */
-	s6e3ha2_call_write_func(ret, s6e3ha2_set_brightness(ctx->bl_dev));
 	s6e3ha2_call_write_func(ret, s6e3ha2_aor_control(ctx));
 	s6e3ha2_call_write_func(ret, s6e3ha2_caps_elvss_set(ctx));
 	s6e3ha2_call_write_func(ret, s6e3ha2_gamma_update(ctx));
@@ -602,7 +621,6 @@ static int s6e3ha2_enable(struct drm_panel *panel)
 	s6e3ha2_call_write_func(ret, s6e3ha2_test_key_off_f0(ctx));
 
 	s6e3ha2_call_write_func(ret, mipi_dsi_dcs_set_display_on(dsi));
-	ctx->bl_dev->props.power = FB_BLANK_UNBLANK;
 
 	return 0;
 }
@@ -721,23 +739,16 @@ static int s6e3ha2_probe(struct mipi_dsi_device *dsi)
 		return PTR_ERR(ctx->enable_gpio);
 	}
 
-	ctx->bl_dev = backlight_device_register("s6e3ha2", dev, ctx,
-						&s6e3ha2_bl_ops, NULL);
-	if (IS_ERR(ctx->bl_dev)) {
-		dev_err(dev, "failed to register backlight device\n");
-		return PTR_ERR(ctx->bl_dev);
-	}
-
-	ctx->bl_dev->props.max_brightness = S6E3HA2_MAX_BRIGHTNESS;
-	ctx->bl_dev->props.brightness = S6E3HA2_DEFAULT_BRIGHTNESS;
-	ctx->bl_dev->props.power = FB_BLANK_POWERDOWN;
-
 	drm_panel_init(&ctx->panel, dev, &s6e3ha2_drm_funcs,
 		       DRM_MODE_CONNECTOR_DSI);
 
+	ret = s6e3ha2_backlight_register(ctx);
+	if (ret)
+		return ret;
+
 	ret = drm_panel_add(&ctx->panel);
 	if (ret < 0)
-		goto unregister_backlight;
+		return ret;
 
 	ret = mipi_dsi_attach(dsi);
 	if (ret < 0)
@@ -748,9 +759,6 @@ static int s6e3ha2_probe(struct mipi_dsi_device *dsi)
 remove_panel:
 	drm_panel_remove(&ctx->panel);
 
-unregister_backlight:
-	backlight_device_unregister(ctx->bl_dev);
-
 	return ret;
 }
 
@@ -760,7 +768,6 @@ static int s6e3ha2_remove(struct mipi_dsi_device *dsi)
 
 	mipi_dsi_detach(dsi);
 	drm_panel_remove(&ctx->panel);
-	backlight_device_unregister(ctx->bl_dev);
 
 	return 0;
 }
-- 
2.20.1

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

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

[Sam Ravnborg]

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 = {
+		.type		= BACKLIGHT_RAW,
+		.brightness	= DEFAULT_BRIGHTNESS,
+		.max_brightness	= MAX_BRIGHTNESS,
+	};
+	struct backlight_device *backlight;
+	struct device *dev = ctx->dev;
+	int ret = 0;
+
+	backlight = devm_backlight_device_register(dev, "panel", dev, ctx,
+						   &s6e63j0x03_bl_ops,
+						   &props);
+	if (IS_ERR(backlight)) {
+		ret = PTR_ERR(backlight);
+		DRM_DEV_ERROR(dev, "error registering backlight device (%d)\n",
+			      ret);
+	}
+	ctx->panel.backlight = backlight;
+
+	return ret;
+}
+
 static int s6e63j0x03_disable(struct drm_panel *panel)
 {
 	struct s6e63j0x03 *ctx = panel_to_s6e63j0x03(panel);
@@ -227,8 +250,6 @@ static int s6e63j0x03_disable(struct drm_panel *panel)
 	if (ret < 0)
 		return ret;
 
-	ctx->bl_dev->props.power = FB_BLANK_NORMAL;
-
 	ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
 	if (ret < 0)
 		return ret;
@@ -247,8 +268,6 @@ static int s6e63j0x03_unprepare(struct drm_panel *panel)
 	if (ret < 0)
 		return ret;
 
-	ctx->bl_dev->props.power = FB_BLANK_POWERDOWN;
-
 	return 0;
 }
 
@@ -334,8 +353,6 @@ static int s6e63j0x03_prepare(struct drm_panel *panel)
 	if (ret < 0)
 		goto err;
 
-	ctx->bl_dev->props.power = FB_BLANK_NORMAL;
-
 	return 0;
 
 err:
@@ -395,8 +412,6 @@ static int s6e63j0x03_enable(struct drm_panel *panel)
 	if (ret < 0)
 		return ret;
 
-	ctx->bl_dev->props.power = FB_BLANK_UNBLANK;
-
 	return 0;
 }
 
@@ -469,20 +484,13 @@ static int s6e63j0x03_probe(struct mipi_dsi_device *dsi)
 	drm_panel_init(&ctx->panel, dev, &s6e63j0x03_funcs,
 		       DRM_MODE_CONNECTOR_DSI);
 
-	ctx->bl_dev = backlight_device_register("s6e63j0x03", dev, ctx,
-						&s6e63j0x03_bl_ops, NULL);
-	if (IS_ERR(ctx->bl_dev)) {
-		dev_err(dev, "failed to register backlight device\n");
-		return PTR_ERR(ctx->bl_dev);
-	}
-
-	ctx->bl_dev->props.max_brightness = MAX_BRIGHTNESS;
-	ctx->bl_dev->props.brightness = DEFAULT_BRIGHTNESS;
-	ctx->bl_dev->props.power = FB_BLANK_POWERDOWN;
+	ret = s6e63j0x03_backlight_register(ctx);
+	if (ret)
+		return ret;
 
 	ret = drm_panel_add(&ctx->panel);
 	if (ret < 0)
-		goto unregister_backlight;
+		return ret;
 
 	ret = mipi_dsi_attach(dsi);
 	if (ret < 0)
@@ -493,9 +501,6 @@ static int s6e63j0x03_probe(struct mipi_dsi_device *dsi)
 remove_panel:
 	drm_panel_remove(&ctx->panel);
 
-unregister_backlight:
-	backlight_device_unregister(ctx->bl_dev);
-
 	return ret;
 }
 
@@ -506,8 +511,6 @@ static int s6e63j0x03_remove(struct mipi_dsi_device *dsi)
 	mipi_dsi_detach(dsi);
 	drm_panel_remove(&ctx->panel);
 
-	backlight_device_unregister(ctx->bl_dev);
-
 	return 0;
 }
 
-- 
2.20.1

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

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

[Emil Velikov]

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.

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.

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

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

[Sam Ravnborg]

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.

	Sam


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

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

[Emil Velikov]

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

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

[Daniel Vetter]

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

[PATCH v1 3/3] drm/panel: update backlight handling for samsung-s6e63m0

[Sam Ravnborg]

The samsung-s6e63m0 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: Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
Cc: Joonas Kylmälä <joonas.kylmala@iki.fi>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
 drivers/gpu/drm/panel/panel-samsung-s6e63m0.c | 20 ++++++++-----------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c b/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
index a5f76eb4fa25..089bf4cff840 100644
--- a/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
+++ b/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
@@ -89,7 +89,6 @@ static u8 const s6e63m0_gamma_22[NUM_GAMMA_LEVELS][GAMMA_TABLE_COUNT] = {
 struct s6e63m0 {
 	struct device *dev;
 	struct drm_panel panel;
-	struct backlight_device *bl_dev;
 
 	struct regulator_bulk_data supplies[2];
 	struct gpio_desc *reset_gpio;
@@ -293,8 +292,6 @@ static int s6e63m0_disable(struct drm_panel *panel)
 	if (!ctx->enabled)
 		return 0;
 
-	backlight_disable(ctx->bl_dev);
-
 	s6e63m0_dcs_write_seq_static(ctx, MIPI_DCS_ENTER_SLEEP_MODE);
 	msleep(200);
 
@@ -355,8 +352,6 @@ static int s6e63m0_enable(struct drm_panel *panel)
 
 	s6e63m0_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_ON);
 
-	backlight_enable(ctx->bl_dev);
-
 	ctx->enabled = true;
 
 	return 0;
@@ -394,7 +389,6 @@ static const struct drm_panel_funcs s6e63m0_drm_funcs = {
 static int s6e63m0_set_brightness(struct backlight_device *bd)
 {
 	struct s6e63m0 *ctx = bl_get_data(bd);
-
 	int brightness = bd->props.brightness;
 
 	/* disable and set new gamma */
@@ -416,19 +410,21 @@ static int s6e63m0_backlight_register(struct s6e63m0 *ctx)
 	struct backlight_properties props = {
 		.type		= BACKLIGHT_RAW,
 		.brightness	= MAX_BRIGHTNESS,
-		.max_brightness = MAX_BRIGHTNESS
+		.max_brightness	= MAX_BRIGHTNESS,
 	};
+	struct backlight_device *backlight;
 	struct device *dev = ctx->dev;
 	int ret = 0;
 
-	ctx->bl_dev = devm_backlight_device_register(dev, "panel", dev, ctx,
-						     &s6e63m0_backlight_ops,
-						     &props);
-	if (IS_ERR(ctx->bl_dev)) {
-		ret = PTR_ERR(ctx->bl_dev);
+	backlight = devm_backlight_device_register(dev, "panel", dev, ctx,
+						   &s6e63m0_backlight_ops,
+						   &props);
+	if (IS_ERR(backlight)) {
+		ret = PTR_ERR(backlight);
 		DRM_DEV_ERROR(dev, "error registering backlight device (%d)\n",
 			      ret);
 	}
+	ctx->panel.backlight = backlight;
 
 	return ret;
 }
-- 
2.20.1

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