Re: [PATCH] drm/panel: samsung: s6e8aa0: Add backlight control support

Hi Joonas,

On 21.09.2019 14:48, Joonas Kylmälä wrote:

This makes the backlight brightness controllable from the userspace.

Signed-off-by: Joonas Kylmälä joonas.kylmala@iki.fi

drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c | 82 ++++++++++++++++++++------- 1 file changed, 60 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c b/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c index dbced6501204..aa75934f5bed 100644 --- a/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c +++ b/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c @@ -10,8 +10,12 @@

• Eunchul Kim chulspro.kim@samsung.com
• Tomasz Figa t.figa@samsung.com
• Andrzej Hajda a.hajda@samsung.com

• • Derived from panel-samsung-s6e63m0.c:
• • Copyright (C) 2019 Paweł Chmiel pawel.mikolaj.chmiel@gmail.com

*/

+#include <linux/backlight.h> #include <linux/delay.h> #include <linux/gpio/consumer.h> #include <linux/module.h> @@ -85,6 +89,8 @@ #define AID_2 (0x6) #define AID_3 (0x7)

+#define MAX_BRIGHTNESS (GAMMA_LEVEL_NUM - 1)

typedef u8 s6e8aa0_gamma_table[GAMMA_TABLE_LEN];

struct s6e8aa0_variant { @@ -95,6 +101,7 @@ struct s6e8aa0_variant { struct s6e8aa0 { struct device *dev; struct drm_panel panel;

• struct backlight_device *bl_dev;

  struct regulator_bulk_data supplies[2]; struct gpio_desc *reset_gpio;

@@ -110,7 +117,6 @@ struct s6e8aa0 { u8 version; u8 id; const struct s6e8aa0_variant *variant;

• int brightness;

  /* This field is tested by functions directly accessing DSI bus before

  • transfer, transfer is skipped if it is set. In case of transfer

@@ -321,9 +327,10 @@ static void s6e8aa0_etc_elvss_control(struct s6e8aa0 *ctx)

static void s6e8aa0_elvss_nvm_set_v142(struct s6e8aa0 *ctx) {

• struct backlight_device *bd = ctx->bl_dev; u8 br;

• switch (ctx->brightness) {

• switch (bd->props.brightness) { case 0 ... 6: /* 30cd ~ 100cd */ br = 0xdf; break;

@@ -762,24 +769,6 @@ static const struct s6e8aa0_variant s6e8aa0_variants[] = { } };

-static void s6e8aa0_brightness_set(struct s6e8aa0 *ctx) -{

• const u8 *gamma;
• if (ctx->error)

• return;
  

• gamma = ctx->variant->gamma_tables[ctx->brightness];
• if (ctx->version >= 142)

• s6e8aa0_elvss_nvm_set(ctx);
  

• s6e8aa0_dcs_write(ctx, gamma, GAMMA_TABLE_LEN);
• /* update gamma table. */
• s6e8aa0_dcs_write_seq_static(ctx, 0xf7, 0x03);

-}

static void s6e8aa0_panel_init(struct s6e8aa0 *ctx) { s6e8aa0_apply_level_1_key(ctx); @@ -791,7 +780,7 @@ static void s6e8aa0_panel_init(struct s6e8aa0 *ctx)

s6e8aa0_panel_cond_set(ctx); s6e8aa0_display_condition_set(ctx);

• s6e8aa0_brightness_set(ctx);

• backlight_enable(ctx->bl_dev); s6e8aa0_etc_source_control(ctx); s6e8aa0_etc_pentile_control(ctx); s6e8aa0_elvss_nvm_set(ctx);

@@ -974,6 +963,53 @@ static int s6e8aa0_parse_dt(struct s6e8aa0 *ctx) return 0; }

+static int s6e8aa0_set_brightness(struct backlight_device *bd) +{

• struct s6e8aa0 *ctx = bl_get_data(bd);
• const u8 *gamma;
• if (ctx->error)

• return;
  

• gamma = ctx->variant->gamma_tables[bd->props.brightness];
• if (ctx->version >= 142)

• s6e8aa0_elvss_nvm_set(ctx);
  

• s6e8aa0_dcs_write(ctx, gamma, GAMMA_TABLE_LEN);
• /* update gamma table. */
• s6e8aa0_dcs_write_seq_static(ctx, 0xf7, 0x03);
• return s6e8aa0_clear_error(ctx);

+}

+static const struct backlight_ops s6e8aa0_backlight_ops = {

• .update_status = s6e8aa0_set_brightness,

This is racy, update_status can be called in any time between probe and remove, particularly:

a) before panel enable,

b) during panel enable,

c) when panel is enabled,

d) during panel disable,

e) after panel disable,

b and d are racy for sure - backlight and drm callbacks are async.

IMO the best solution would be to register backlight after attaching panel to drm, but for this drm_panel_funcs should have attach/detach callbacks (like drm_bridge_funcs),

then update_status callback should take some drm_connector lock to synchronize with drm, and write to hw only when pipe is enabled.

Regards

Andrzej

+};

+static int s6e8aa0_backlight_register(struct s6e8aa0 *ctx) +{

• struct backlight_properties props = {

• .type		= BACKLIGHT_RAW,
  

• .brightness	= MAX_BRIGHTNESS,
  

• .max_brightness = MAX_BRIGHTNESS
  

• };
• struct device *dev = ctx->dev;
• int ret = 0;
• ctx->bl_dev = devm_backlight_device_register(dev, "panel", dev, ctx,

• 				     &s6e8aa0_backlight_ops,
  

• 				     &props);
  

• if (IS_ERR(ctx->bl_dev)) {

• ret = PTR_ERR(ctx->bl_dev);
  

• DRM_DEV_ERROR(dev, "error registering backlight device (%d)\n",
  

• 	      ret);
  

• }
• return ret;

+}

static int s6e8aa0_probe(struct mipi_dsi_device *dsi) { struct device *dev = &dsi->dev; @@ -1015,7 +1051,9 @@ static int s6e8aa0_probe(struct mipi_dsi_device *dsi) return PTR_ERR(ctx->reset_gpio); }

• ctx->brightness = GAMMA_LEVEL_NUM - 1;

• ret = s6e8aa0_backlight_register(ctx);

• if (ret < 0)

• return ret;
  
  

  drm_panel_init(&ctx->panel, dev, &s6e8aa0_drm_funcs, DRM_MODE_CONNECTOR_DSI);