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(-)
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; }
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; }
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
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
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
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
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; }
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Emil Velikov -
Sam Ravnborg