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