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.kylm...@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....@samsung.com>
> * Tomasz Figa <t.f...@samsung.com>
> * Andrzej Hajda <a.ha...@samsung.com>
> + *
> + * Derived from panel-samsung-s6e63m0.c:
> + * Copyright (C) 2019 Paweł Chmiel <pawel.mikolaj.chm...@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);
