[PATCH v3 0/5] Add new panel driver Samsung S6E88A0-AMS427AP24

[PATCH v3 0/5] Add new panel driver Samsung S6E88A0-AMS427AP24

[Jakob Hauser]

The patchset adds a new driver for Samsung AMS427AP24 panel with S6E88A0
controller. Patches are based on current branch drm-misc-next.

Changes in v3:
 - Patch 2: Dropped the second "bindings" in the commit subject.
 - Patch 2: Applied 4 spaces indentation in the example.
 - Patch 3: Made struct s6e88a0_ams427ap24_supplies[] "static".
 - Patch 3: Removed the "panel->prepared" parts from functions
   s6e88a0_ams427ap24_prepare() and s6e88a0_ams427ap24_unprepare().
 - Patch 5: Dissolved function s6e88a0_ams427ap24_parse_dt() and placed
   the parsing in the probe function. Changed the parsing from
   of_property_read_bool() to device_property_read_bool().

v1: https://lore.kernel.org/dri-devel/cover.1728582727.git.jahau@rocketmail.com/T/#t
v2: https://lore.kernel.org/dri-devel/cover.1729630039.git.jahau@rocketmail.com/T/#t

Jakob Hauser (5):
  dt-bindings: display: panel: Move flip properties to panel-common
  dt-bindings: display: panel: Add Samsung S6E88A0-AMS427AP24
  drm/panel: samsung-s6e88a0-ams427ap24: Add initial driver
  drm/panel: samsung-s6e88a0-ams427ap24: Add brightness control
  drm/panel: samsung-s6e88a0-ams427ap24: Add flip option

 .../bindings/display/panel/panel-common.yaml  |   8 +
 .../panel/samsung,s6e88a0-ams427ap24.yaml     |  65 ++
 .../display/panel/samsung,s6e8aa0.yaml        |  10 +-
 drivers/gpu/drm/panel/Kconfig                 |  10 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 .../panel/panel-samsung-s6e88a0-ams427ap24.c  | 754 ++++++++++++++++++
 6 files changed, 840 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/panel/samsung,s6e88a0-ams427ap24.yaml
 create mode 100644 drivers/gpu/drm/panel/panel-samsung-s6e88a0-ams427ap24.c

-- 
2.39.5

[PATCH v3 1/5] dt-bindings: display: panel: Move flip properties to panel-common

[Jakob Hauser]

The flip properties were used by "samsung,s6e8aa0.yaml" only so far. By
introducing "samsung,s6e88a0-ams427ap24.yaml" they become more common.

Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
Patch is based on current branch drm-misc-next.
---
 .../bindings/display/panel/panel-common.yaml           |  8 ++++++++
 .../bindings/display/panel/samsung,s6e8aa0.yaml        | 10 ++--------
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.yaml b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
index 0a57a31f4f3d..087415753d60 100644
--- a/Documentation/devicetree/bindings/display/panel/panel-common.yaml
+++ b/Documentation/devicetree/bindings/display/panel/panel-common.yaml
@@ -51,6 +51,14 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32
     enum: [0, 90, 180, 270]
 
+  flip-horizontal:
+    description: boolean to flip image horizontally
+    type: boolean
+
+  flip-vertical:
+    description: boolean to flip image vertically
+    type: boolean
+
   # Display Timings
   panel-timing:
     description:
diff --git a/Documentation/devicetree/bindings/display/panel/samsung,s6e8aa0.yaml b/Documentation/devicetree/bindings/display/panel/samsung,s6e8aa0.yaml
index 4601fa460680..19c8cc83db97 100644
--- a/Documentation/devicetree/bindings/display/panel/samsung,s6e8aa0.yaml
+++ b/Documentation/devicetree/bindings/display/panel/samsung,s6e8aa0.yaml
@@ -21,6 +21,8 @@ properties:
 
   reset-gpios: true
   display-timings: true
+  flip-horizontal: true
+  flip-vertical: true
 
   vdd3-supply:
     description: core voltage supply
@@ -46,14 +48,6 @@ properties:
   panel-height-mm:
     description: physical panel height [mm]
 
-  flip-horizontal:
-    description: boolean to flip image horizontally
-    type: boolean
-
-  flip-vertical:
-    description: boolean to flip image vertically
-    type: boolean
-
 required:
   - compatible
   - reg
-- 
2.39.5

[PATCH v3 2/5] dt-bindings: display: panel: Add Samsung S6E88A0-AMS427AP24

[Jakob Hauser]

Add bindings for Samsung AMS427AP24 panel with S6E88A0 controller.

Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
Patch is based on current branch drm-misc-next.

Changes in v3:
 - Dropped the second "bindings" in the commit subject.
 - Applied 4 spaces indentation in the example.
---
 .../panel/samsung,s6e88a0-ams427ap24.yaml     | 65 +++++++++++++++++++
 1 file changed, 65 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/samsung,s6e88a0-ams427ap24.yaml

diff --git a/Documentation/devicetree/bindings/display/panel/samsung,s6e88a0-ams427ap24.yaml b/Documentation/devicetree/bindings/display/panel/samsung,s6e88a0-ams427ap24.yaml
new file mode 100644
index 000000000000..db284ba5be20
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/samsung,s6e88a0-ams427ap24.yaml
@@ -0,0 +1,65 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/samsung,s6e88a0-ams427ap24.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Samsung AMS427AP24 panel with S6E88A0 controller
+
+maintainers:
+  - Jakob Hauser <jahau@rocketmail.com>
+
+allOf:
+  - $ref: panel-common.yaml#
+
+properties:
+  compatible:
+    const: samsung,s6e88a0-ams427ap24
+
+  reg:
+    maxItems: 1
+
+  port: true
+  reset-gpios: true
+  flip-horizontal: true
+
+  vdd3-supply:
+    description: core voltage supply
+
+  vci-supply:
+    description: voltage supply for analog circuits
+
+required:
+  - compatible
+  - reg
+  - port
+  - reset-gpios
+  - vdd3-supply
+  - vci-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    dsi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        panel@0 {
+            compatible = "samsung,s6e88a0-ams427ap24";
+            reg = <0>;
+
+            vdd3-supply = <&pm8916_l17>;
+            vci-supply = <&pm8916_l6>;
+            reset-gpios = <&tlmm 25 GPIO_ACTIVE_LOW>;
+            flip-horizontal;
+
+            port {
+                panel_in: endpoint {
+                    remote-endpoint = <&mdss_dsi0_out>;
+                };
+            };
+        };
+    };
-- 
2.39.5

[PATCH v3 3/5] drm/panel: samsung-s6e88a0-ams427ap24: Add initial driver

[Jakob Hauser]

This initial part of the panel driver was mostly generated by the
"linux-mdss-dsi-panel-driver-generator" tool [1], reading downstream
Android kernel file "dsi_panel_S6E88A0_AMS427AP24_qhd_octa_video.dtsi" [2].

On top of the generic output of the tool, there were a couple of changes
applied:
- Added mipi_dsi_dcs_set_display_on() to function s6e88a0_ams427ap24_on(),
  otherwise the display does not show up.
- In functions s6e88a0_ams427ap24_on() and s6e88a0_ams427ap24_off()
  changed DSI commands to multi context and used "accum_err" returns.
- In functions s6e88a0_ams427ap24_on() and s6e88a0_ams427ap24_off() replaced
  msleep() by mipi_dsi_msleep().
- The function s6e88a0_ams427ap24_get_modes() was changed to make use of
  drm_connector_helper_get_modes_fixed(). This also required to include
  drm/drm_probe_helper.h.
- In function s6e88a0_ams427ap24_probe() registring the regulators was changed
  to devm_regulator_bulk_get_const(). This required to change supplies in struct
  s6e88a0_ams427ap24 to a pointer.
- Removed bool "prepared" from struct s6e88a0_ams427ap24 and according parts in
  functions s6e88a0_ams427ap24_prepare() and s6e88a0_ams427ap24_unprepare().

[1] https://github.com/msm8916-mainline/linux-mdss-dsi-panel-driver-generator
[2] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/video/msm/mdss/samsung/S6E88A0_AMS427AP24/dsi_panel_S6E88A0_AMS427AP24_qhd_octa_video.dtsi

Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
---
Changes in v3:
 - Made struct s6e88a0_ams427ap24_supplies[] "static".
 - Removed the "panel->prepared" parts from functions
   s6e88a0_ams427ap24_prepare() and s6e88a0_ams427ap24_unprepare().
---
 drivers/gpu/drm/panel/Kconfig                 |   9 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 .../panel/panel-samsung-s6e88a0-ams427ap24.c  | 252 ++++++++++++++++++
 3 files changed, 262 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-samsung-s6e88a0-ams427ap24.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index bce4cb64184a..f8adc38447fb 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -632,6 +632,15 @@ config DRM_PANEL_SAMSUNG_AMS639RQ08
 	  Say Y or M here if you want to enable support for the
 	  Samsung AMS639RQ08 FHD Plus (2340x1080@60Hz) CMD mode panel.
 
+config DRM_PANEL_SAMSUNG_S6E88A0_AMS427AP24
+	tristate "Samsung AMS427AP24 panel with S6E88A0 controller"
+	depends on GPIOLIB && OF && REGULATOR
+	depends on DRM_MIPI_DSI
+	help
+	  Say Y here if you want to enable support for Samsung AMS427AP24 panel
+	  with S6E88A0 controller (found in Samsung Galaxy S4 Mini Value Edition
+	  GT-I9195I). To compile this driver as a module, choose M here.
+
 config DRM_PANEL_SAMSUNG_S6E88A0_AMS452EF01
 	tristate "Samsung AMS452EF01 panel with S6E88A0 DSI video mode controller"
 	depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 4db02c2411ae..7dcf72646cac 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -77,6 +77,7 @@ obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E63J0X03) += panel-samsung-s6e63j0x03.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E63M0) += panel-samsung-s6e63m0.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E63M0_SPI) += panel-samsung-s6e63m0-spi.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E63M0_DSI) += panel-samsung-s6e63m0-dsi.o
+obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E88A0_AMS427AP24) += panel-samsung-s6e88a0-ams427ap24.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E88A0_AMS452EF01) += panel-samsung-s6e88a0-ams452ef01.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0) += panel-samsung-s6e8aa0.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_SOFEF00) += panel-samsung-sofef00.o
diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e88a0-ams427ap24.c b/drivers/gpu/drm/panel/panel-samsung-s6e88a0-ams427ap24.c
new file mode 100644
index 000000000000..7435e0fcb4f5
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-samsung-s6e88a0-ams427ap24.c
@@ -0,0 +1,252 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Samsung AMS427AP24 panel with S6E88A0 controller
+ * Copyright (c) 2024 Jakob Hauser <jahau@rocketmail.com>
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regulator/consumer.h>
+
+#include <video/mipi_display.h>
+
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_panel.h>
+#include <drm/drm_probe_helper.h>
+
+struct s6e88a0_ams427ap24 {
+	struct drm_panel panel;
+	struct mipi_dsi_device *dsi;
+	struct regulator_bulk_data *supplies;
+	struct gpio_desc *reset_gpio;
+};
+
+static const struct regulator_bulk_data s6e88a0_ams427ap24_supplies[] = {
+	{ .supply = "vdd3" },
+	{ .supply = "vci" },
+};
+
+static inline
+struct s6e88a0_ams427ap24 *to_s6e88a0_ams427ap24(struct drm_panel *panel)
+{
+	return container_of(panel, struct s6e88a0_ams427ap24, panel);
+}
+
+static void s6e88a0_ams427ap24_reset(struct s6e88a0_ams427ap24 *ctx)
+{
+	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
+	usleep_range(5000, 6000);
+	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
+	usleep_range(1000, 2000);
+	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
+	usleep_range(18000, 19000);
+}
+
+static int s6e88a0_ams427ap24_on(struct s6e88a0_ams427ap24 *ctx)
+{
+	struct mipi_dsi_device *dsi = ctx->dsi;
+	struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
+
+	dsi->mode_flags |= MIPI_DSI_MODE_LPM;
+
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf0, 0x5a, 0x5a);
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfc, 0x5a, 0x5a);
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb0, 0x11);
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfd, 0x11);
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb0, 0x13);
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfd, 0x18);
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb0, 0x02);
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb8, 0x30);
+
+	mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
+	mipi_dsi_msleep(&dsi_ctx, 20);
+
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf1, 0x5a, 0x5a);
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xcc, 0x4c);
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf2, 0x03, 0x0d);
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf1, 0xa5, 0xa5);
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xca,
+				     0x01, 0x00, 0x01, 0x00, 0x01, 0x00, 0x80,
+				     0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
+				     0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
+				     0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
+				     0x80, 0x80, 0x00, 0x00, 0x00);
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb2,
+				     0x40, 0x08, 0x20, 0x00, 0x08);
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb6, 0x28, 0x0b);
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf7, 0x03);
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x55, 0x00);
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf0, 0xa5, 0xa5);
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfc, 0xa5, 0xa5);
+
+	mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
+
+	return dsi_ctx.accum_err;
+}
+
+static int s6e88a0_ams427ap24_off(struct s6e88a0_ams427ap24 *ctx)
+{
+	struct mipi_dsi_device *dsi = ctx->dsi;
+	struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
+
+	dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
+
+	mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
+	mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
+	mipi_dsi_msleep(&dsi_ctx, 120);
+
+	return dsi_ctx.accum_err;
+}
+
+static int s6e88a0_ams427ap24_prepare(struct drm_panel *panel)
+{
+	struct s6e88a0_ams427ap24 *ctx = to_s6e88a0_ams427ap24(panel);
+	struct device *dev = &ctx->dsi->dev;
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(s6e88a0_ams427ap24_supplies),
+				    ctx->supplies);
+	if (ret < 0) {
+		dev_err(dev, "Failed to enable regulators: %d\n", ret);
+		return ret;
+	}
+
+	s6e88a0_ams427ap24_reset(ctx);
+
+	ret = s6e88a0_ams427ap24_on(ctx);
+	if (ret < 0) {
+		dev_err(dev, "Failed to initialize panel: %d\n", ret);
+		gpiod_set_value_cansleep(ctx->reset_gpio, 1);
+		regulator_bulk_disable(ARRAY_SIZE(s6e88a0_ams427ap24_supplies),
+				       ctx->supplies);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int s6e88a0_ams427ap24_unprepare(struct drm_panel *panel)
+{
+	struct s6e88a0_ams427ap24 *ctx = to_s6e88a0_ams427ap24(panel);
+	struct device *dev = &ctx->dsi->dev;
+	int ret;
+
+	ret = s6e88a0_ams427ap24_off(ctx);
+	if (ret < 0)
+		dev_err(dev, "Failed to un-initialize panel: %d\n", ret);
+
+	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
+	regulator_bulk_disable(ARRAY_SIZE(s6e88a0_ams427ap24_supplies),
+			       ctx->supplies);
+
+	return 0;
+}
+
+static const struct drm_display_mode s6e88a0_ams427ap24_mode = {
+	.clock = (540 + 94 + 4 + 18) * (960 + 12 + 1 + 3) * 60 / 1000,
+	.hdisplay = 540,
+	.hsync_start = 540 + 94,
+	.hsync_end = 540 + 94 + 4,
+	.htotal = 540 + 94 + 4 + 18,
+	.vdisplay = 960,
+	.vsync_start = 960 + 12,
+	.vsync_end = 960 + 12 + 1,
+	.vtotal = 960 + 12 + 1 + 3,
+	.width_mm = 55,
+	.height_mm = 95,
+	.type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
+};
+
+static int s6e88a0_ams427ap24_get_modes(struct drm_panel *panel,
+					struct drm_connector *connector)
+{
+	return drm_connector_helper_get_modes_fixed(connector,
+						    &s6e88a0_ams427ap24_mode);
+}
+
+static const struct drm_panel_funcs s6e88a0_ams427ap24_panel_funcs = {
+	.prepare = s6e88a0_ams427ap24_prepare,
+	.unprepare = s6e88a0_ams427ap24_unprepare,
+	.get_modes = s6e88a0_ams427ap24_get_modes,
+};
+
+static int s6e88a0_ams427ap24_probe(struct mipi_dsi_device *dsi)
+{
+	struct device *dev = &dsi->dev;
+	struct s6e88a0_ams427ap24 *ctx;
+	int ret;
+
+	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ret = devm_regulator_bulk_get_const(dev,
+				      ARRAY_SIZE(s6e88a0_ams427ap24_supplies),
+				      s6e88a0_ams427ap24_supplies,
+				      &ctx->supplies);
+	if (ret < 0)
+		return ret;
+
+	ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(ctx->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(ctx->reset_gpio),
+				     "Failed to get reset-gpios\n");
+
+	ctx->dsi = dsi;
+	mipi_dsi_set_drvdata(dsi, ctx);
+
+	dsi->lanes = 2;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
+			  MIPI_DSI_MODE_NO_EOT_PACKET;
+
+	drm_panel_init(&ctx->panel, dev, &s6e88a0_ams427ap24_panel_funcs,
+		       DRM_MODE_CONNECTOR_DSI);
+	ctx->panel.prepare_prev_first = true;
+
+	drm_panel_add(&ctx->panel);
+
+	ret = mipi_dsi_attach(dsi);
+	if (ret < 0) {
+		dev_err(dev, "Failed to attach to DSI host: %d\n", ret);
+		drm_panel_remove(&ctx->panel);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void s6e88a0_ams427ap24_remove(struct mipi_dsi_device *dsi)
+{
+	struct s6e88a0_ams427ap24 *ctx = mipi_dsi_get_drvdata(dsi);
+	int ret;
+
+	ret = mipi_dsi_detach(dsi);
+	if (ret < 0)
+		dev_err(&dsi->dev, "Failed to detach from DSI host: %d\n", ret);
+
+	drm_panel_remove(&ctx->panel);
+}
+
+static const struct of_device_id s6e88a0_ams427ap24_of_match[] = {
+	{ .compatible = "samsung,s6e88a0-ams427ap24" },
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, s6e88a0_ams427ap24_of_match);
+
+static struct mipi_dsi_driver s6e88a0_ams427ap24_driver = {
+	.probe = s6e88a0_ams427ap24_probe,
+	.remove = s6e88a0_ams427ap24_remove,
+	.driver = {
+		.name = "panel-s6e88a0-ams427ap24",
+		.of_match_table = s6e88a0_ams427ap24_of_match,
+	},
+};
+module_mipi_dsi_driver(s6e88a0_ams427ap24_driver);
+
+MODULE_AUTHOR("Jakob Hauser <jahau@rocketmail.com>");
+MODULE_DESCRIPTION("Samsung AMS427AP24 panel with S6E88A0 controller");
+MODULE_LICENSE("GPL v2");
-- 
2.39.5

[PATCH v3 4/5] drm/panel: samsung-s6e88a0-ams427ap24: Add brightness control

[Jakob Hauser]

The tables for brightness to candela, aid and elvss were taken from downstream
kernel file "dsi_panel_S6E88A0_AMS427AP24_qhd_octa_video.dtsi" [1][2][3].

The gamma table gets generated in "ss_dsi_smart_dimming_S6E88A0_AMS427AP24.c" [4]
with hard-coded starting values. The function smart_dimming_init() [5] goes
through the v{*}_adjustments, generate_gray_scale and gamma_init procedure.
Instead of calculating it manually, it's easier to compile a custom downstream
kernel with SMART_DIMMING_DEBUG enabled and read out dmesg early at boot.

Selection of the values for aid and elvss are again according to downstream
file "dsi_panel_S6E88A0_AMS427AP24_qhd_octa_video.dtsi" [6][7].

The set of write commands is guided by downstream file "ss_dsi_panel_common.c" [8]
followed by "ss_dsi_panel_S6E88A0_AMS427AP24.c" [9].

The dsi mode flag MIPI_DSI_MODE_VIDEO_NO_HFP prevents screen flickering while
changing the brightness.

[1] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/video/msm/mdss/samsung/S6E88A0_AMS427AP24/dsi_panel_S6E88A0_AMS427AP24_qhd_octa_video.dtsi#L341-L397
[2] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/video/msm/mdss/samsung/S6E88A0_AMS427AP24/dsi_panel_S6E88A0_AMS427AP24_qhd_octa_video.dtsi#L214-L254
[3] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/video/msm/mdss/samsung/S6E88A0_AMS427AP24/dsi_panel_S6E88A0_AMS427AP24_qhd_octa_video.dtsi#L301-L320
[4] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/video/msm/mdss/samsung/S6E88A0_AMS427AP24/ss_dsi_smart_dimming_S6E88A0_AMS427AP24.c
[5] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/video/msm/mdss/samsung/S6E88A0_AMS427AP24/ss_dsi_smart_dimming_S6E88A0_AMS427AP24.c#L1816-L1900
[6] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/video/msm/mdss/samsung/S6E88A0_AMS427AP24/dsi_panel_S6E88A0_AMS427AP24_qhd_octa_video.dtsi#L256-L268
[7] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/video/msm/mdss/samsung/S6E88A0_AMS427AP24/dsi_panel_S6E88A0_AMS427AP24_qhd_octa_video.dtsi#L322-L334
[8] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/video/msm/mdss/samsung/ss_dsi_panel_common.c#L1389-L1517
[9] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/video/msm/mdss/samsung/S6E88A0_AMS427AP24/ss_dsi_panel_S6E88A0_AMS427AP24.c#L666-L678

Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/gpu/drm/panel/Kconfig                 |   1 +
 .../panel/panel-samsung-s6e88a0-ams427ap24.c  | 519 +++++++++++++++++-
 2 files changed, 508 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index f8adc38447fb..d7469c565d1d 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -636,6 +636,7 @@ config DRM_PANEL_SAMSUNG_S6E88A0_AMS427AP24
 	tristate "Samsung AMS427AP24 panel with S6E88A0 controller"
 	depends on GPIOLIB && OF && REGULATOR
 	depends on DRM_MIPI_DSI
+	depends on BACKLIGHT_CLASS_DEVICE
 	help
 	  Say Y here if you want to enable support for Samsung AMS427AP24 panel
 	  with S6E88A0 controller (found in Samsung Galaxy S4 Mini Value Edition
diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e88a0-ams427ap24.c b/drivers/gpu/drm/panel/panel-samsung-s6e88a0-ams427ap24.c
index 7435e0fcb4f5..9e211be432d7 100644
--- a/drivers/gpu/drm/panel/panel-samsung-s6e88a0-ams427ap24.c
+++ b/drivers/gpu/drm/panel/panel-samsung-s6e88a0-ams427ap24.c
@@ -4,6 +4,7 @@
  * Copyright (c) 2024 Jakob Hauser <jahau@rocketmail.com>
  */
 
+#include <linux/backlight.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
@@ -17,8 +18,17 @@
 #include <drm/drm_panel.h>
 #include <drm/drm_probe_helper.h>
 
+#define NUM_STEPS_CANDELA	54
+#define NUM_STEPS_AID		39
+#define NUM_STEPS_ELVSS		17
+
+#define SEQ_LENGTH_AID		5
+#define SEQ_LENGTH_ELVSS	2
+#define SEQ_LENGTH_GAMMA	33
+
 struct s6e88a0_ams427ap24 {
 	struct drm_panel panel;
+	struct backlight_device *bl_dev;
 	struct mipi_dsi_device *dsi;
 	struct regulator_bulk_data *supplies;
 	struct gpio_desc *reset_gpio;
@@ -35,6 +45,464 @@ struct s6e88a0_ams427ap24 *to_s6e88a0_ams427ap24(struct drm_panel *panel)
 	return container_of(panel, struct s6e88a0_ams427ap24, panel);
 }
 
+enum candela {
+	CANDELA_10CD, /* 0 */
+	CANDELA_11CD,
+	CANDELA_12CD,
+	CANDELA_13CD,
+	CANDELA_14CD,
+	CANDELA_15CD,
+	CANDELA_16CD,
+	CANDELA_17CD,
+	CANDELA_19CD,
+	CANDELA_20CD,
+	CANDELA_21CD,
+	CANDELA_22CD,
+	CANDELA_24CD,
+	CANDELA_25CD,
+	CANDELA_27CD,
+	CANDELA_29CD,
+	CANDELA_30CD,
+	CANDELA_32CD,
+	CANDELA_34CD,
+	CANDELA_37CD,
+	CANDELA_39CD,
+	CANDELA_41CD,
+	CANDELA_44CD,
+	CANDELA_47CD,
+	CANDELA_50CD,
+	CANDELA_53CD,
+	CANDELA_56CD,
+	CANDELA_60CD,
+	CANDELA_64CD,
+	CANDELA_68CD,
+	CANDELA_72CD,
+	CANDELA_77CD,
+	CANDELA_82CD,
+	CANDELA_87CD,
+	CANDELA_93CD,
+	CANDELA_98CD,
+	CANDELA_105CD,
+	CANDELA_111CD,
+	CANDELA_119CD,
+	CANDELA_126CD,
+	CANDELA_134CD,
+	CANDELA_143CD,
+	CANDELA_152CD,
+	CANDELA_162CD,
+	CANDELA_172CD,
+	CANDELA_183CD,
+	CANDELA_195CD,
+	CANDELA_207CD,
+	CANDELA_220CD,
+	CANDELA_234CD,
+	CANDELA_249CD,
+	CANDELA_265CD,
+	CANDELA_282CD,
+	CANDELA_300CD, /* 53 */
+};
+
+static const int s6e88a0_ams427ap24_br_to_cd[NUM_STEPS_CANDELA] = {
+	/* brightness till, candela */
+	10, /* 10CD */
+	11, /* 11CD */
+	12, /* 12CD */
+	13, /* 13CD */
+	14, /* 14CD */
+	15, /* 15CD */
+	16, /* 16CD */
+	17, /* 17CD */
+	18, /* 19CD */
+	19, /* 20CD */
+	20, /* 21CD */
+	21, /* 22CD */
+	22, /* 24CD */
+	23, /* 25CD */
+	24, /* 27CD */
+	25, /* 29CD */
+	26, /* 30CD */
+	27, /* 32CD */
+	28, /* 34CD */
+	29, /* 37CD */
+	30, /* 39CD */
+	32, /* 41CD */
+	34, /* 44CD */
+	36, /* 47CD */
+	38, /* 50CD */
+	40, /* 53CD */
+	43, /* 56CD */
+	46, /* 60CD */
+	49, /* 64CD */
+	52, /* 68CD */
+	56, /* 72CD */
+	59, /* 77CD */
+	63, /* 82CD */
+	67, /* 87CD */
+	71, /* 93CD */
+	76, /* 98CD */
+	80, /* 105CD */
+	86, /* 111CD */
+	91, /* 119CD */
+	97, /* 126CD */
+	104, /* 134CD */
+	110, /* 143CD */
+	118, /* 152CD */
+	125, /* 162CD */
+	133, /* 172CD */
+	142, /* 183CD */
+	150, /* 195CD */
+	160, /* 207CD */
+	170, /* 220CD */
+	181, /* 234CD */
+	205, /* 249CD */
+	234, /* 265CD */
+	254, /* 282CD */
+	255, /* 300CD */
+};
+
+static const u8 s6e88a0_ams427ap24_aid[NUM_STEPS_AID][SEQ_LENGTH_AID] = {
+	{ 0x40, 0x08, 0x20, 0x03, 0x77 }, /* AOR 90.9%, 10CD */
+	{ 0x40, 0x08, 0x20, 0x03, 0x73 }, /* AOR 90.5%, 11CD */
+	{ 0x40, 0x08, 0x20, 0x03, 0x69 }, /* AOR 89.4%, 12CD */
+	{ 0x40, 0x08, 0x20, 0x03, 0x65 }, /* AOR 89.0%, 13CD */
+	{ 0x40, 0x08, 0x20, 0x03, 0x61 }, /* AOR 88.6%, 14CD */
+	{ 0x40, 0x08, 0x20, 0x03, 0x55 }, /* AOR 87.4%, 15CD */
+	{ 0x40, 0x08, 0x20, 0x03, 0x50 }, /* AOR 86.9%, 16CD */
+	{ 0x40, 0x08, 0x20, 0x03, 0x45 }, /* AOR 85.8%, 17CD */
+	{ 0x40, 0x08, 0x20, 0x03, 0x35 }, /* AOR 84.1%, 19CD */
+	{ 0x40, 0x08, 0x20, 0x03, 0x27 }, /* AOR 82.7%, 20CD */
+	{ 0x40, 0x08, 0x20, 0x03, 0x23 }, /* AOR 82.3%, 21CD */
+	{ 0x40, 0x08, 0x20, 0x03, 0x17 }, /* AOR 81.0%, 22CD */
+	{ 0x40, 0x08, 0x20, 0x03, 0x11 }, /* AOR 80.4%, 24CD */
+	{ 0x40, 0x08, 0x20, 0x03, 0x04 }, /* AOR 79.1%, 25CD */
+	{ 0x40, 0x08, 0x20, 0x02, 0xf4 }, /* AOR 77.5%, 27CD */
+	{ 0x40, 0x08, 0x20, 0x02, 0xe3 }, /* AOR 75.7%, 29CD */
+	{ 0x40, 0x08, 0x20, 0x02, 0xd7 }, /* AOR 74.5%, 30CD */
+	{ 0x40, 0x08, 0x20, 0x02, 0xc6 }, /* AOR 72.7%, 32CD */
+	{ 0x40, 0x08, 0x20, 0x02, 0xb7 }, /* AOR 71.2%, 34CD */
+	{ 0x40, 0x08, 0x20, 0x02, 0xa1 }, /* AOR 69.0%, 37CD */
+	{ 0x40, 0x08, 0x20, 0x02, 0x91 }, /* AOR 67.3%, 39CD */
+	{ 0x40, 0x08, 0x20, 0x02, 0x78 }, /* AOR 64.8%, 41CD */
+	{ 0x40, 0x08, 0x20, 0x02, 0x62 }, /* AOR 62.5%, 44CD */
+	{ 0x40, 0x08, 0x20, 0x02, 0x45 }, /* AOR 59.5%, 47CD */
+	{ 0x40, 0x08, 0x20, 0x02, 0x30 }, /* AOR 57.4%, 50CD */
+	{ 0x40, 0x08, 0x20, 0x02, 0x13 }, /* AOR 54.4%, 53CD */
+	{ 0x40, 0x08, 0x20, 0x01, 0xf5 }, /* AOR 51.3%, 56CD */
+	{ 0x40, 0x08, 0x20, 0x01, 0xd3 }, /* AOR 47.8%, 60CD */
+	{ 0x40, 0x08, 0x20, 0x01, 0xb1 }, /* AOR 44.4%, 64CD */
+	{ 0x40, 0x08, 0x20, 0x01, 0x87 }, /* AOR 40.1%, 68CD */
+	{ 0x40, 0x08, 0x20, 0x01, 0x63 }, /* AOR 36.6%, 72CD */
+	{ 0x40, 0x08, 0x20, 0x01, 0x35 }, /* AOR 31.7%, 77CD */
+	{ 0x40, 0x08, 0x20, 0x01, 0x05 }, /* AOR 26.9%, 82CD */
+	{ 0x40, 0x08, 0x20, 0x00, 0xd5 }, /* AOR 21.8%, 87CD */
+	{ 0x40, 0x08, 0x20, 0x00, 0xa1 }, /* AOR 16.5%, 93CD */
+	{ 0x40, 0x08, 0x20, 0x00, 0x6f }, /* AOR 11.4%, 98CD */
+	{ 0x40, 0x08, 0x20, 0x00, 0x31 }, /* AOR 5.0%, 105CD */
+	{ 0x40, 0x08, 0x20, 0x01, 0x86 }, /* AOR 40.0%, 111CD ~ 172CD */
+	{ 0x40, 0x08, 0x20, 0x00, 0x08 }, /* AOR 0.6%, 183CD ~ 300CD */
+};
+
+static const u8 s6e88a0_ams427ap24_elvss[NUM_STEPS_ELVSS][SEQ_LENGTH_ELVSS] = {
+	{ 0x28, 0x14 }, /* 10CD ~ 111CD */
+	{ 0x28, 0x13 }, /* 119CD */
+	{ 0x28, 0x12 }, /* 126CD */
+	{ 0x28, 0x12 }, /* 134CD */
+	{ 0x28, 0x11 }, /* 143CD */
+	{ 0x28, 0x10 }, /* 152CD */
+	{ 0x28, 0x0f }, /* 162CD */
+	{ 0x28, 0x0e }, /* 172CD */
+	{ 0x28, 0x11 }, /* 183CD */
+	{ 0x28, 0x11 }, /* 195CD */
+	{ 0x28, 0x10 }, /* 207CD */
+	{ 0x28, 0x0f }, /* 220CD */
+	{ 0x28, 0x0f }, /* 234CD */
+	{ 0x28, 0x0e }, /* 249CD */
+	{ 0x28, 0x0d }, /* 265CD */
+	{ 0x28, 0x0c }, /* 282CD */
+	{ 0x28, 0x0b }, /* 300CD */
+};
+
+static const u8 s6e88a0_ams427ap24_gamma[NUM_STEPS_CANDELA][SEQ_LENGTH_GAMMA] = {
+	/* 10CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8c, 0x8a, 0x8a, 0x8c, 0x8b,
+	  0x8c, 0x87, 0x89, 0x89, 0x88, 0x87, 0x8c, 0x80, 0x82, 0x88, 0x7b,
+	  0x72, 0x8c, 0x60, 0x68, 0x8c, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 },
+	/* 11CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8c, 0x8a, 0x8a, 0x8c, 0x8b,
+	  0x8c, 0x87, 0x89, 0x89, 0x88, 0x87, 0x8c, 0x80, 0x82, 0x88, 0x7b,
+	  0x72, 0x8c, 0x60, 0x68, 0x8c, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 },
+	/* 12CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8c, 0x8a, 0x8a, 0x8b, 0x8b,
+	  0x8c, 0x88, 0x89, 0x8a, 0x88, 0x87, 0x8c, 0x81, 0x82, 0x87, 0x7a,
+	  0x72, 0x8b, 0x60, 0x68, 0x8c, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 },
+	/* 13CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8c, 0x8a, 0x8a, 0x8b, 0x8b,
+	  0x8c, 0x88, 0x89, 0x8a, 0x88, 0x87, 0x8c, 0x81, 0x82, 0x87, 0x7a,
+	  0x72, 0x8b, 0x61, 0x69, 0x8c, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 },
+	/* 14CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8c, 0x8a, 0x89, 0x8c, 0x8b,
+	  0x8c, 0x88, 0x89, 0x8a, 0x87, 0x86, 0x8a, 0x82, 0x82, 0x87, 0x79,
+	  0x71, 0x89, 0x63, 0x6c, 0x8e, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 },
+	/* 15CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8c, 0x8a, 0x8a, 0x8c, 0x8c,
+	  0x8c, 0x86, 0x87, 0x88, 0x85, 0x85, 0x8a, 0x83, 0x83, 0x88, 0x78,
+	  0x72, 0x89, 0x64, 0x6c, 0x8e, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 },
+	/* 16CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8c, 0x8a, 0x89, 0x8c, 0x8b,
+	  0x8c, 0x86, 0x88, 0x88, 0x86, 0x86, 0x8a, 0x84, 0x84, 0x88, 0x78,
+	  0x72, 0x89, 0x5d, 0x67, 0x8b, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 },
+	/* 17CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8c, 0x8a, 0x89, 0x8b, 0x8b,
+	  0x8b, 0x87, 0x89, 0x89, 0x86, 0x86, 0x8a, 0x84, 0x83, 0x87, 0x78,
+	  0x73, 0x89, 0x64, 0x6e, 0x8e, 0x38, 0x32, 0x24, 0x00, 0x00, 0x00 },
+	/* 19CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8c, 0x8a, 0x89, 0x8b, 0x8b,
+	  0x8b, 0x87, 0x89, 0x89, 0x86, 0x86, 0x89, 0x84, 0x84, 0x87, 0x77,
+	  0x72, 0x88, 0x65, 0x6f, 0x8e, 0x38, 0x32, 0x24, 0x00, 0x00, 0x00 },
+	/* 20CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8c, 0x8a, 0x89, 0x8b, 0x8b,
+	  0x8b, 0x88, 0x89, 0x89, 0x85, 0x85, 0x88, 0x82, 0x83, 0x85, 0x79,
+	  0x73, 0x88, 0x65, 0x6f, 0x8e, 0x38, 0x32, 0x24, 0x00, 0x00, 0x00 },
+	/* 21CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8c, 0x8a, 0x89, 0x8b, 0x8b,
+	  0x8b, 0x88, 0x89, 0x89, 0x85, 0x85, 0x88, 0x82, 0x83, 0x85, 0x79,
+	  0x74, 0x88, 0x65, 0x6f, 0x8e, 0x38, 0x32, 0x24, 0x00, 0x00, 0x00 },
+	/* 22CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8c, 0x8a, 0x89, 0x8c, 0x8b,
+	  0x8c, 0x86, 0x88, 0x87, 0x86, 0x86, 0x89, 0x82, 0x83, 0x85, 0x7c,
+	  0x75, 0x87, 0x65, 0x6f, 0x8e, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 },
+	/* 24CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8c, 0x8a, 0x89, 0x8c, 0x8b,
+	  0x8c, 0x86, 0x88, 0x87, 0x86, 0x86, 0x89, 0x82, 0x83, 0x85, 0x7c,
+	  0x76, 0x86, 0x66, 0x6f, 0x8e, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 },
+	/* 25CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8c, 0x8a, 0x89, 0x8b, 0x8b,
+	  0x8b, 0x86, 0x89, 0x88, 0x87, 0x87, 0x89, 0x82, 0x82, 0x84, 0x7f,
+	  0x7a, 0x89, 0x6b, 0x73, 0x8f, 0x33, 0x2f, 0x22, 0x00, 0x00, 0x00 },
+	/* 27CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8c, 0x8a, 0x89, 0x8b, 0x8b,
+	  0x8b, 0x86, 0x89, 0x88, 0x87, 0x87, 0x89, 0x82, 0x82, 0x84, 0x7f,
+	  0x7a, 0x89, 0x6b, 0x73, 0x8f, 0x33, 0x2f, 0x22, 0x00, 0x00, 0x00 },
+	/* 29CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8c, 0x8a, 0x89, 0x8b, 0x8b,
+	  0x8b, 0x86, 0x89, 0x88, 0x85, 0x84, 0x87, 0x84, 0x85, 0x86, 0x80,
+	  0x7b, 0x88, 0x6a, 0x73, 0x8f, 0x33, 0x2f, 0x22, 0x00, 0x00, 0x00 },
+	/* 30CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8c, 0x8a, 0x89, 0x8b, 0x8b,
+	  0x8b, 0x86, 0x89, 0x88, 0x85, 0x84, 0x87, 0x84, 0x85, 0x86, 0x80,
+	  0x7b, 0x88, 0x6a, 0x73, 0x8f, 0x33, 0x2f, 0x22, 0x00, 0x00, 0x00 },
+	/* 32CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8c, 0x8a, 0x89, 0x8b, 0x8b,
+	  0x8b, 0x86, 0x89, 0x88, 0x85, 0x84, 0x87, 0x84, 0x85, 0x86, 0x80,
+	  0x7b, 0x88, 0x6a, 0x73, 0x8f, 0x33, 0x2f, 0x22, 0x00, 0x00, 0x00 },
+	/* 34CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8c, 0x8a, 0x89, 0x8b, 0x8b,
+	  0x8b, 0x86, 0x89, 0x88, 0x85, 0x84, 0x87, 0x83, 0x84, 0x84, 0x7f,
+	  0x79, 0x86, 0x6c, 0x76, 0x91, 0x33, 0x2f, 0x22, 0x00, 0x00, 0x00 },
+	/* 37CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8b, 0x89, 0x89, 0x8b, 0x8b,
+	  0x8b, 0x86, 0x88, 0x88, 0x87, 0x86, 0x87, 0x83, 0x84, 0x84, 0x7f,
+	  0x79, 0x86, 0x6c, 0x76, 0x90, 0x33, 0x2f, 0x22, 0x00, 0x00, 0x00 },
+	/* 39CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8b, 0x89, 0x89, 0x8b, 0x8b,
+	  0x8b, 0x86, 0x88, 0x87, 0x84, 0x84, 0x86, 0x83, 0x85, 0x85, 0x80,
+	  0x79, 0x85, 0x6c, 0x76, 0x90, 0x33, 0x2f, 0x22, 0x00, 0x00, 0x00 },
+	/* 41CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8b, 0x89, 0x89, 0x8b, 0x8b,
+	  0x8b, 0x86, 0x88, 0x87, 0x84, 0x84, 0x86, 0x81, 0x84, 0x83, 0x7f,
+	  0x79, 0x84, 0x6e, 0x79, 0x93, 0x33, 0x2f, 0x22, 0x00, 0x00, 0x00 },
+	/* 44CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8b, 0x89, 0x89, 0x8b, 0x8b,
+	  0x8b, 0x86, 0x88, 0x87, 0x84, 0x84, 0x86, 0x81, 0x84, 0x83, 0x7f,
+	  0x79, 0x84, 0x6e, 0x79, 0x92, 0x33, 0x2f, 0x22, 0x00, 0x00, 0x00 },
+	/* 47CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8b, 0x89, 0x89, 0x8b, 0x8b,
+	  0x8b, 0x86, 0x88, 0x87, 0x84, 0x85, 0x86, 0x81, 0x84, 0x83, 0x7f,
+	  0x79, 0x83, 0x6f, 0x79, 0x91, 0x33, 0x2f, 0x22, 0x00, 0x00, 0x00 },
+	/* 50CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8b, 0x89, 0x89, 0x8b, 0x8b,
+	  0x8b, 0x86, 0x88, 0x87, 0x84, 0x85, 0x86, 0x82, 0x84, 0x83, 0x7f,
+	  0x79, 0x83, 0x6f, 0x79, 0x90, 0x33, 0x2f, 0x22, 0x00, 0x00, 0x00 },
+	/* 53CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8b, 0x89, 0x89, 0x8b, 0x8b,
+	  0x8b, 0x86, 0x88, 0x87, 0x83, 0x83, 0x85, 0x84, 0x85, 0x85, 0x7f,
+	  0x79, 0x83, 0x70, 0x79, 0x8f, 0x33, 0x2f, 0x22, 0x00, 0x00, 0x00 },
+	/* 56CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8b, 0x89, 0x89, 0x8b, 0x8a,
+	  0x8a, 0x87, 0x89, 0x87, 0x83, 0x83, 0x85, 0x84, 0x85, 0x84, 0x7f,
+	  0x79, 0x82, 0x70, 0x7a, 0x8e, 0x33, 0x2f, 0x22, 0x00, 0x00, 0x00 },
+	/* 60CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8b, 0x89, 0x89, 0x8b, 0x8a,
+	  0x8a, 0x87, 0x89, 0x87, 0x83, 0x83, 0x85, 0x84, 0x85, 0x84, 0x7e,
+	  0x79, 0x82, 0x71, 0x7a, 0x8d, 0x33, 0x2f, 0x22, 0x00, 0x00, 0x00 },
+	/* 64CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8b, 0x89, 0x89, 0x8b, 0x8a,
+	  0x8a, 0x86, 0x88, 0x86, 0x84, 0x84, 0x86, 0x82, 0x83, 0x82, 0x80,
+	  0x7a, 0x84, 0x71, 0x7a, 0x8c, 0x33, 0x2f, 0x22, 0x00, 0x00, 0x00 },
+	/* 68CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8a, 0x89, 0x89, 0x8c, 0x8a,
+	  0x8a, 0x86, 0x88, 0x86, 0x84, 0x84, 0x86, 0x82, 0x84, 0x82, 0x81,
+	  0x7b, 0x83, 0x72, 0x7b, 0x8b, 0x33, 0x2f, 0x22, 0x00, 0x00, 0x00 },
+	/* 72CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8a, 0x89, 0x89, 0x8c, 0x8a,
+	  0x8a, 0x86, 0x88, 0x86, 0x85, 0x85, 0x86, 0x82, 0x84, 0x82, 0x81,
+	  0x7b, 0x83, 0x72, 0x7c, 0x8a, 0x33, 0x2f, 0x22, 0x00, 0x00, 0x00 },
+	/* 77CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8a, 0x89, 0x89, 0x8c, 0x8a,
+	  0x8a, 0x85, 0x87, 0x85, 0x85, 0x87, 0x87, 0x82, 0x84, 0x82, 0x81,
+	  0x7c, 0x82, 0x72, 0x7c, 0x89, 0x33, 0x2f, 0x22, 0x00, 0x00, 0x00 },
+	/* 82CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8a, 0x89, 0x89, 0x8c, 0x8a,
+	  0x8a, 0x85, 0x87, 0x85, 0x85, 0x87, 0x87, 0x82, 0x84, 0x82, 0x81,
+	  0x7c, 0x82, 0x73, 0x7c, 0x88, 0x33, 0x2f, 0x22, 0x00, 0x00, 0x00 },
+	/* 87CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8a, 0x89, 0x89, 0x8c, 0x8a,
+	  0x8a, 0x85, 0x87, 0x85, 0x84, 0x84, 0x86, 0x80, 0x84, 0x81, 0x80,
+	  0x7a, 0x82, 0x76, 0x7f, 0x89, 0x33, 0x2f, 0x22, 0x00, 0x00, 0x00 },
+	/* 93CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8a, 0x89, 0x89, 0x8b, 0x8a,
+	  0x8a, 0x86, 0x87, 0x85, 0x84, 0x85, 0x86, 0x80, 0x84, 0x80, 0x80,
+	  0x7a, 0x82, 0x76, 0x80, 0x88, 0x33, 0x2f, 0x22, 0x00, 0x00, 0x00 },
+	/* 98CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8a, 0x89, 0x89, 0x8b, 0x8a,
+	  0x8a, 0x86, 0x87, 0x85, 0x85, 0x85, 0x86, 0x80, 0x84, 0x80, 0x80,
+	  0x7a, 0x82, 0x76, 0x80, 0x88, 0x33, 0x2f, 0x22, 0x00, 0x00, 0x00 },
+	/* 105CD */
+	{ 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x89, 0x88, 0x88, 0x8b, 0x8a,
+	  0x8a, 0x84, 0x87, 0x85, 0x85, 0x85, 0x85, 0x80, 0x84, 0x80, 0x7f,
+	  0x79, 0x81, 0x71, 0x7d, 0x87, 0x38, 0x32, 0x24, 0x00, 0x00, 0x00 },
+	/* 111CD */
+	{ 0x00, 0xdf, 0x00, 0xde, 0x00, 0xde, 0x85, 0x85, 0x84, 0x87, 0x86,
+	  0x87, 0x85, 0x86, 0x85, 0x83, 0x83, 0x83, 0x81, 0x82, 0x82, 0x80,
+	  0x7d, 0x82, 0x75, 0x7f, 0x86, 0x85, 0x85, 0x82, 0x00, 0x00, 0x00 },
+	/* 119CD */
+	{ 0x00, 0xe3, 0x00, 0xe1, 0x00, 0xe2, 0x85, 0x85, 0x84, 0x86, 0x85,
+	  0x85, 0x84, 0x85, 0x84, 0x83, 0x83, 0x83, 0x82, 0x82, 0x82, 0x7e,
+	  0x7b, 0x81, 0x75, 0x7f, 0x86, 0x85, 0x85, 0x82, 0x00, 0x00, 0x00 },
+	/* 126CD */
+	{ 0x00, 0xe6, 0x00, 0xe5, 0x00, 0xe5, 0x85, 0x84, 0x84, 0x85, 0x85,
+	  0x85, 0x84, 0x84, 0x84, 0x82, 0x83, 0x83, 0x80, 0x81, 0x81, 0x80,
+	  0x7f, 0x83, 0x73, 0x7c, 0x84, 0x85, 0x85, 0x82, 0x00, 0x00, 0x00 },
+	/* 134CD */
+	{ 0x00, 0xe9, 0x00, 0xe8, 0x00, 0xe8, 0x84, 0x84, 0x83, 0x85, 0x85,
+	  0x85, 0x84, 0x84, 0x83, 0x81, 0x82, 0x82, 0x81, 0x81, 0x81, 0x7f,
+	  0x7d, 0x81, 0x73, 0x7c, 0x83, 0x85, 0x85, 0x82, 0x00, 0x00, 0x00 },
+	/* 143CD */
+	{ 0x00, 0xed, 0x00, 0xec, 0x00, 0xec, 0x84, 0x83, 0x83, 0x84, 0x84,
+	  0x84, 0x84, 0x84, 0x83, 0x82, 0x83, 0x83, 0x81, 0x80, 0x81, 0x7f,
+	  0x7e, 0x81, 0x70, 0x79, 0x81, 0x85, 0x85, 0x82, 0x00, 0x00, 0x00 },
+	/* 152CD */
+	{ 0x00, 0xf0, 0x00, 0xf0, 0x00, 0xf0, 0x83, 0x83, 0x83, 0x83, 0x83,
+	  0x83, 0x84, 0x84, 0x83, 0x81, 0x81, 0x81, 0x80, 0x80, 0x81, 0x80,
+	  0x80, 0x82, 0x6f, 0x78, 0x7f, 0x85, 0x85, 0x82, 0x00, 0x00, 0x00 },
+	/* 162CD */
+	{ 0x00, 0xf4, 0x00, 0xf3, 0x00, 0xf4, 0x83, 0x83, 0x83, 0x83, 0x83,
+	  0x83, 0x82, 0x81, 0x81, 0x81, 0x81, 0x81, 0x80, 0x80, 0x81, 0x80,
+	  0x7f, 0x82, 0x6f, 0x78, 0x7f, 0x85, 0x85, 0x82, 0x00, 0x00, 0x00 },
+	/* 172CD */
+	{ 0x00, 0xf8, 0x00, 0xf8, 0x00, 0xf8, 0x82, 0x82, 0x82, 0x82, 0x82,
+	  0x82, 0x82, 0x81, 0x81, 0x80, 0x81, 0x80, 0x80, 0x80, 0x81, 0x81,
+	  0x80, 0x83, 0x6d, 0x76, 0x7d, 0x85, 0x85, 0x82, 0x00, 0x00, 0x00 },
+	/* 183CD */
+	{ 0x00, 0xe0, 0x00, 0xdf, 0x00, 0xdf, 0x84, 0x84, 0x83, 0x86, 0x86,
+	  0x86, 0x83, 0x84, 0x83, 0x82, 0x82, 0x82, 0x81, 0x83, 0x81, 0x81,
+	  0x7e, 0x81, 0x80, 0x82, 0x84, 0x85, 0x85, 0x82, 0x00, 0x00, 0x00 },
+	/* 195CD */
+	{ 0x00, 0xe4, 0x00, 0xe3, 0x00, 0xe3, 0x84, 0x83, 0x83, 0x85, 0x85,
+	  0x85, 0x83, 0x84, 0x83, 0x81, 0x82, 0x82, 0x82, 0x83, 0x81, 0x81,
+	  0x80, 0x82, 0x7d, 0x7f, 0x81, 0x85, 0x85, 0x82, 0x00, 0x00, 0x00 },
+	/* 207CD */
+	{ 0x00, 0xe7, 0x00, 0xe6, 0x00, 0xe6, 0x83, 0x82, 0x82, 0x85, 0x85,
+	  0x85, 0x82, 0x83, 0x83, 0x82, 0x82, 0x82, 0x80, 0x81, 0x80, 0x81,
+	  0x80, 0x82, 0x7d, 0x7f, 0x81, 0x85, 0x85, 0x82, 0x00, 0x00, 0x00 },
+	/* 220CD */
+	{ 0x00, 0xeb, 0x00, 0xea, 0x00, 0xea, 0x83, 0x83, 0x82, 0x84, 0x84,
+	  0x84, 0x82, 0x83, 0x82, 0x81, 0x81, 0x82, 0x81, 0x82, 0x81, 0x80,
+	  0x7e, 0x80, 0x7d, 0x7f, 0x81, 0x85, 0x85, 0x82, 0x00, 0x00, 0x00 },
+	/* 234CD */
+	{ 0x00, 0xef, 0x00, 0xee, 0x00, 0xee, 0x83, 0x82, 0x82, 0x83, 0x83,
+	  0x83, 0x82, 0x82, 0x82, 0x81, 0x81, 0x81, 0x80, 0x80, 0x80, 0x80,
+	  0x80, 0x81, 0x7b, 0x7c, 0x7f, 0x85, 0x85, 0x82, 0x00, 0x00, 0x00 },
+	/* 249CD */
+	{ 0x00, 0xf3, 0x00, 0xf2, 0x00, 0xf2, 0x82, 0x81, 0x81, 0x83, 0x83,
+	  0x83, 0x82, 0x82, 0x82, 0x81, 0x81, 0x81, 0x80, 0x81, 0x80, 0x7f,
+	  0x7e, 0x7f, 0x7b, 0x7c, 0x7f, 0x85, 0x85, 0x82, 0x00, 0x00, 0x00 },
+	/* 265CD */
+	{ 0x00, 0xf7, 0x00, 0xf7, 0x00, 0xf7, 0x81, 0x81, 0x80, 0x82, 0x82,
+	  0x82, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x81, 0x80, 0x7f,
+	  0x7e, 0x7f, 0x7b, 0x7c, 0x7f, 0x85, 0x85, 0x82, 0x00, 0x00, 0x00 },
+	/* 282CD */
+	{ 0x00, 0xfb, 0x00, 0xfb, 0x00, 0xfb, 0x80, 0x80, 0x80, 0x81, 0x81,
+	  0x81, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x7f, 0x7f, 0x7f, 0x7f,
+	  0x7f, 0x7f, 0x78, 0x79, 0x7d, 0x85, 0x85, 0x82, 0x00, 0x00, 0x00 },
+	/* 300CD */
+	{ 0x01, 0x00, 0x01, 0x00, 0x01, 0x00, 0x80, 0x80, 0x80, 0x80, 0x80,
+	  0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
+	  0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x00, 0x00, 0x00 },
+};
+
+static int s6e88a0_ams427ap24_set_brightness(struct backlight_device *bd)
+{
+	struct s6e88a0_ams427ap24 *ctx = bl_get_data(bd);
+	struct mipi_dsi_device *dsi = ctx->dsi;
+	struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
+	struct device *dev = &dsi->dev;
+	int brightness = bd->props.brightness;
+	int candela_enum;
+	u8 b2[SEQ_LENGTH_AID + 1];
+	u8 b6[SEQ_LENGTH_ELVSS + 1];
+	u8 ca[SEQ_LENGTH_GAMMA + 1];
+
+	/* get candela enum from brightness */
+	for (candela_enum = 0; candela_enum < NUM_STEPS_CANDELA; candela_enum++)
+		if (brightness <= s6e88a0_ams427ap24_br_to_cd[candela_enum])
+			break;
+
+	/* get aid */
+	b2[0] = 0xb2;
+	switch (candela_enum) {
+	case CANDELA_10CD ... CANDELA_105CD:
+		memcpy(&b2[1], s6e88a0_ams427ap24_aid[candela_enum],
+		       SEQ_LENGTH_AID);
+		break;
+	case CANDELA_111CD ... CANDELA_172CD:
+		memcpy(&b2[1], s6e88a0_ams427ap24_aid[CANDELA_111CD],
+		       SEQ_LENGTH_AID);
+		break;
+	case CANDELA_183CD ... CANDELA_300CD:
+		memcpy(&b2[1], s6e88a0_ams427ap24_aid[CANDELA_111CD + 1],
+		       SEQ_LENGTH_AID);
+		break;
+	default:
+		dev_err(dev, "Failed to get aid data\n");
+		return -EINVAL;
+	}
+
+	/* get elvss */
+	b6[0] = 0xb6;
+	if (candela_enum <= CANDELA_111CD) {
+		memcpy(&b6[1], s6e88a0_ams427ap24_elvss[0], SEQ_LENGTH_ELVSS);
+	} else {
+		memcpy(&b6[1], s6e88a0_ams427ap24_elvss[candela_enum - CANDELA_111CD],
+		       SEQ_LENGTH_ELVSS);
+	}
+
+	/* get gamma */
+	ca[0] = 0xca;
+	memcpy(&ca[1], s6e88a0_ams427ap24_gamma[candela_enum], SEQ_LENGTH_GAMMA);
+
+	/* write: key on, aid, acl off, elvss, gamma, gamma update, key off */
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf0, 0x5a, 0x5a);
+	mipi_dsi_dcs_write_buffer_multi(&dsi_ctx, b2, ARRAY_SIZE(b2));
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x55, 0x00);
+	mipi_dsi_dcs_write_buffer_multi(&dsi_ctx, b6, ARRAY_SIZE(b6));
+	mipi_dsi_dcs_write_buffer_multi(&dsi_ctx, ca, ARRAY_SIZE(ca));
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf7, 0x03);
+	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf0, 0xa5, 0xa5);
+
+	return dsi_ctx.accum_err;
+}
+
 static void s6e88a0_ams427ap24_reset(struct s6e88a0_ams427ap24 *ctx)
 {
 	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
@@ -49,6 +517,8 @@ static int s6e88a0_ams427ap24_on(struct s6e88a0_ams427ap24 *ctx)
 {
 	struct mipi_dsi_device *dsi = ctx->dsi;
 	struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
+	struct device *dev = &dsi->dev;
+	int ret;
 
 	dsi->mode_flags |= MIPI_DSI_MODE_LPM;
 
@@ -68,20 +538,15 @@ static int s6e88a0_ams427ap24_on(struct s6e88a0_ams427ap24 *ctx)
 	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xcc, 0x4c);
 	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf2, 0x03, 0x0d);
 	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf1, 0xa5, 0xa5);
-	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xca,
-				     0x01, 0x00, 0x01, 0x00, 0x01, 0x00, 0x80,
-				     0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
-				     0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
-				     0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
-				     0x80, 0x80, 0x00, 0x00, 0x00);
-	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb2,
-				     0x40, 0x08, 0x20, 0x00, 0x08);
-	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb6, 0x28, 0x0b);
-	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf7, 0x03);
-	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x55, 0x00);
 	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf0, 0xa5, 0xa5);
 	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfc, 0xa5, 0xa5);
 
+	ret = s6e88a0_ams427ap24_set_brightness(ctx->bl_dev);
+	if (ret < 0) {
+		dev_err(dev, "Failed to set brightness: %d\n", ret);
+		return ret;
+	}
+
 	mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
 
 	return dsi_ctx.accum_err;
@@ -173,6 +638,32 @@ static const struct drm_panel_funcs s6e88a0_ams427ap24_panel_funcs = {
 	.get_modes = s6e88a0_ams427ap24_get_modes,
 };
 
+static const struct backlight_ops s6e88a0_ams427ap24_bl_ops = {
+	.update_status	= s6e88a0_ams427ap24_set_brightness,
+};
+
+static int s6e88a0_ams427ap24_register_backlight(struct s6e88a0_ams427ap24 *ctx)
+{
+	struct backlight_properties props = {
+		.type		= BACKLIGHT_RAW,
+		.brightness	= 180,
+		.max_brightness = 255,
+	};
+	struct mipi_dsi_device *dsi = ctx->dsi;
+	struct device *dev = &dsi->dev;
+	int ret = 0;
+
+	ctx->bl_dev = devm_backlight_device_register(dev, dev_name(dev), dev, ctx,
+						     &s6e88a0_ams427ap24_bl_ops,
+						     &props);
+	if (IS_ERR(ctx->bl_dev)) {
+		ret = PTR_ERR(ctx->bl_dev);
+		dev_err(dev, "error registering backlight device (%d)\n", ret);
+	}
+
+	return ret;
+}
+
 static int s6e88a0_ams427ap24_probe(struct mipi_dsi_device *dsi)
 {
 	struct device *dev = &dsi->dev;
@@ -201,12 +692,16 @@ static int s6e88a0_ams427ap24_probe(struct mipi_dsi_device *dsi)
 	dsi->lanes = 2;
 	dsi->format = MIPI_DSI_FMT_RGB888;
 	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
-			  MIPI_DSI_MODE_NO_EOT_PACKET;
+			  MIPI_DSI_MODE_NO_EOT_PACKET | MIPI_DSI_MODE_VIDEO_NO_HFP;
 
 	drm_panel_init(&ctx->panel, dev, &s6e88a0_ams427ap24_panel_funcs,
 		       DRM_MODE_CONNECTOR_DSI);
 	ctx->panel.prepare_prev_first = true;
 
+	ret = s6e88a0_ams427ap24_register_backlight(ctx);
+	if (ret < 0)
+		return ret;
+
 	drm_panel_add(&ctx->panel);
 
 	ret = mipi_dsi_attach(dsi);
-- 
2.39.5

[PATCH v3 5/5] drm/panel: samsung-s6e88a0-ams427ap24: Add flip option

[Jakob Hauser]

The way of implementing a flip option follows the existing
panel-samsung-s6e8aa0.c [1][2][3].

The value to flip the screen is taken from a downstream kernel file of
a similar but older panel [4]. The mipi clock [5] for the new panel
samsung-s6e88a0-ams427ap24 matches 461 MHz and a hardware read-out of the
0xcb values corresponds to revision R01 of that older panel [6]. Although
for samsung-s6e88a0-ams427ap24 that's in non-flipped state while in this
older driver it seems to be the other way around. Further up there is a
hint [7] basically saying for revision R01 to change the first word of the
0xcb command from 0x06 to 0x0e, which is actually setting BIT(3) of that
word. This causes a horizontal flip.

[1] https://github.com/torvalds/linux/blob/v6.11/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c#L103
[2] https://github.com/torvalds/linux/blob/v6.11/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c#L207-L211
[3] https://github.com/torvalds/linux/blob/v6.11/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c#L954-L974
[4] https://github.com/LineageOS/android_kernel_samsung_msm8930-common/blob/lineage-15.1/drivers/video/msm/mipi_samsung_oled_video_qhd_pt-8930.c
[5] https://github.com/LineageOS/android_kernel_samsung_msm8930-common/blob/lineage-15.1/drivers/video/msm/mipi_samsung_oled_video_qhd_pt-8930.c#L2027-L2028
[6] https://github.com/LineageOS/android_kernel_samsung_msm8930-common/blob/lineage-15.1/drivers/video/msm/mipi_samsung_oled_video_qhd_pt-8930.c#L137-L151
[7] https://github.com/LineageOS/android_kernel_samsung_msm8930-common/blob/lineage-15.1/drivers/video/msm/mipi_samsung_oled_video_qhd_pt-8930.c#L66-L74

Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
---
Changes in v3:
 - Dissolved function s6e88a0_ams427ap24_parse_dt() and placed
   the parsing in the probe function. Changed the parsing from
   of_property_read_bool() to device_property_read_bool().
---
 drivers/gpu/drm/panel/panel-samsung-s6e88a0-ams427ap24.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e88a0-ams427ap24.c b/drivers/gpu/drm/panel/panel-samsung-s6e88a0-ams427ap24.c
index 9e211be432d7..cbd51351a325 100644
--- a/drivers/gpu/drm/panel/panel-samsung-s6e88a0-ams427ap24.c
+++ b/drivers/gpu/drm/panel/panel-samsung-s6e88a0-ams427ap24.c
@@ -32,6 +32,7 @@ struct s6e88a0_ams427ap24 {
 	struct mipi_dsi_device *dsi;
 	struct regulator_bulk_data *supplies;
 	struct gpio_desc *reset_gpio;
+	bool flip_horizontal;
 };
 
 static const struct regulator_bulk_data s6e88a0_ams427ap24_supplies[] = {
@@ -538,6 +539,10 @@ static int s6e88a0_ams427ap24_on(struct s6e88a0_ams427ap24 *ctx)
 	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xcc, 0x4c);
 	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf2, 0x03, 0x0d);
 	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf1, 0xa5, 0xa5);
+
+	if (ctx->flip_horizontal)
+		mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xcb, 0x0e);
+
 	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf0, 0xa5, 0xa5);
 	mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfc, 0xa5, 0xa5);
 
@@ -698,6 +703,8 @@ static int s6e88a0_ams427ap24_probe(struct mipi_dsi_device *dsi)
 		       DRM_MODE_CONNECTOR_DSI);
 	ctx->panel.prepare_prev_first = true;
 
+	ctx->flip_horizontal = device_property_read_bool(dev, "flip-horizontal");
+
 	ret = s6e88a0_ams427ap24_register_backlight(ctx);
 	if (ret < 0)
 		return ret;
-- 
2.39.5

Re: [PATCH v3 2/5] dt-bindings: display: panel: Add Samsung S6E88A0-AMS427AP24

[Linus Walleij]

On Thu, Oct 24, 2024 at 5:18 AM Jakob Hauser <jahau@rocketmail.com> wrote:

> Add bindings for Samsung AMS427AP24 panel with S6E88A0 controller.
>
> Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v3 1/5] dt-bindings: display: panel: Move flip properties to panel-common

[Linus Walleij]

On Thu, Oct 24, 2024 at 5:18 AM Jakob Hauser <jahau@rocketmail.com> wrote:

> The flip properties were used by "samsung,s6e8aa0.yaml" only so far. By
> introducing "samsung,s6e88a0-ams427ap24.yaml" they become more common.
>
> Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

I think I even have a legacy panel that needs this, good!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v3 3/5] drm/panel: samsung-s6e88a0-ams427ap24: Add initial driver

[Linus Walleij]

Hi Jakob,

thanks for your patch!

On Thu, Oct 24, 2024 at 5:18 AM Jakob Hauser <jahau@rocketmail.com> wrote:

> This initial part of the panel driver was mostly generated by the
> "linux-mdss-dsi-panel-driver-generator" tool [1], reading downstream
> Android kernel file "dsi_panel_S6E88A0_AMS427AP24_qhd_octa_video.dtsi" [2].
>
> On top of the generic output of the tool, there were a couple of changes
> applied:
> - Added mipi_dsi_dcs_set_display_on() to function s6e88a0_ams427ap24_on(),
>   otherwise the display does not show up.
> - In functions s6e88a0_ams427ap24_on() and s6e88a0_ams427ap24_off()
>   changed DSI commands to multi context and used "accum_err" returns.
> - In functions s6e88a0_ams427ap24_on() and s6e88a0_ams427ap24_off() replaced
>   msleep() by mipi_dsi_msleep().
> - The function s6e88a0_ams427ap24_get_modes() was changed to make use of
>   drm_connector_helper_get_modes_fixed(). This also required to include
>   drm/drm_probe_helper.h.
> - In function s6e88a0_ams427ap24_probe() registring the regulators was changed
>   to devm_regulator_bulk_get_const(). This required to change supplies in struct
>   s6e88a0_ams427ap24 to a pointer.
> - Removed bool "prepared" from struct s6e88a0_ams427ap24 and according parts in
>   functions s6e88a0_ams427ap24_prepare() and s6e88a0_ams427ap24_unprepare().
>
> [1] https://github.com/msm8916-mainline/linux-mdss-dsi-panel-driver-generator
> [2] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/video/msm/mdss/samsung/S6E88A0_AMS427AP24/dsi_panel_S6E88A0_AMS427AP24_qhd_octa_video.dtsi
>
> Signed-off-by: Jakob Hauser <jahau@rocketmail.com>

Nice job on this driver so far!

> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/of.h>

Why do you need this include? .of_match_table is part of
<linux/driver.h>

> +static int s6e88a0_ams427ap24_on(struct s6e88a0_ams427ap24 *ctx)
> +{
> +       struct mipi_dsi_device *dsi = ctx->dsi;
> +       struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
> +
> +       dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> +
> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf0, 0x5a, 0x5a);

Can we provide #defines for at least some of this magic?
See other drivers for a very good idea of what some of them mean.
panel-samsung-s6d27a1.c:#define S6D27A1_PASSWD_L2       0xF0    /*
Password Command for Level 2 Control */
panel-samsung-s6d7aa0.c:#define MCS_PASSWD1             0xf0

> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfc, 0x5a, 0x5a);

panel-samsung-s6d7aa0.c:#define MCS_PASSWD3             0xfc

> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb0, 0x11);
> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfd, 0x11);
> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb0, 0x13);
> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfd, 0x18);
> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb0, 0x02);
> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb8, 0x30);
(...)
> +       mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
> +       mipi_dsi_msleep(&dsi_ctx, 20);
> +
> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf1, 0x5a, 0x5a);

panel-samsung-s6d7aa0.c:#define MCS_PASSWD2             0xf1

> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xcc, 0x4c);
> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf2, 0x03, 0x0d);
> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf1, 0xa5, 0xa5);

panel-samsung-s6d7aa0.c:#define MCS_PASSWD2             0xf1
Send in the reverse password: disable access.

> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xca,
> +                                    0x01, 0x00, 0x01, 0x00, 0x01, 0x00, 0x80,
> +                                    0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
> +                                    0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
> +                                    0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
> +                                    0x80, 0x80, 0x00, 0x00, 0x00);
> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb2,
> +                                    0x40, 0x08, 0x20, 0x00, 0x08);
> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb6, 0x28, 0x0b);
> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf7, 0x03);
> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x55, 0x00);
> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf0, 0xa5, 0xa5);
> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfc, 0xa5, 0xa5);

Send in the reverse password: disable access.

A bit of #defines and comments would make it much more clear what
is going on.

Yours,
Linus Walleij

Re: [PATCH v3 4/5] drm/panel: samsung-s6e88a0-ams427ap24: Add brightness control

[Linus Walleij]

Hi Jakob,

thanks for your patch!

On Thu, Oct 24, 2024 at 5:18 AM Jakob Hauser <jahau@rocketmail.com> wrote:
> +static const int s6e88a0_ams427ap24_br_to_cd[NUM_STEPS_CANDELA] = {

(...)
> +       /* brightness till, candela */

Brightness to candela conversion table? Edit comment?

> +static const u8 s6e88a0_ams427ap24_aid[NUM_STEPS_AID][SEQ_LENGTH_AID] = {

If you know that the sequence 0xb2, 0x40, 0x08, 0x20 means "set AID"
(or is it AOR??) you can #define

#define S6E88A0_SET_AID 0xb2

Then make a small buffer:

u8 set_aid[5] = { S6E88A0_SET_AID, 0x40, 0x08, 0x20, 0x00, 0x00 };

then you can strip the first three bytes from the entire table,
just copy in the two relevant bytes into set_aor[]
and send that.

> +static const u8 s6e88a0_ams427ap24_elvss[NUM_STEPS_ELVSS][SEQ_LENGTH_ELVSS] = {
> +       { 0x28, 0x14 }, /* 10CD ~ 111CD */
> +       { 0x28, 0x13 }, /* 119CD */

Command 0xb6 is
#define S6E88A0_SET_LVSS 0xb6

Same comment: just define
u8 set_lvss[2] = {S6E88A0_SET_LVSS, 0x28, 0x00};
copy the second byte into the command, this becomes
an array of single bytes.

> +static const u8 s6e88a0_ams427ap24_gamma[NUM_STEPS_CANDELA][SEQ_LENGTH_GAMMA] = {
> +       /* 10CD */
> +       { 0x00, 0xc8, 0x00, 0xc4, 0x00, 0xc5, 0x8c, 0x8a, 0x8a, 0x8c, 0x8b,
> +         0x8c, 0x87, 0x89, 0x89, 0x88, 0x87, 0x8c, 0x80, 0x82, 0x88, 0x7b,
> +         0x72, 0x8c, 0x60, 0x68, 0x8c, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 },

This array looks fine though, it seems to be all unique gamma calibration.

> +static int s6e88a0_ams427ap24_set_brightness(struct backlight_device *bd)
> +{
> +       struct s6e88a0_ams427ap24 *ctx = bl_get_data(bd);
> +       struct mipi_dsi_device *dsi = ctx->dsi;
> +       struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
> +       struct device *dev = &dsi->dev;
> +       int brightness = bd->props.brightness;
> +       int candela_enum;
> +       u8 b2[SEQ_LENGTH_AID + 1];
> +       u8 b6[SEQ_LENGTH_ELVSS + 1];
> +       u8 ca[SEQ_LENGTH_GAMMA + 1];

Rename them to something like my suggestions so we understand what it is
all about. It seems the infrastructure for what I suggested is mostly already
there.

See comment above how to modify arrays to contain stuff that is always
the same.

> +       /* get aid */
> +       b2[0] = 0xb2;

Use a define per above.

> +       switch (candela_enum) {
> +       case CANDELA_10CD ... CANDELA_105CD:
> +               memcpy(&b2[1], s6e88a0_ams427ap24_aid[candela_enum],
> +                      SEQ_LENGTH_AID);
> +               break;
> +       case CANDELA_111CD ... CANDELA_172CD:
> +               memcpy(&b2[1], s6e88a0_ams427ap24_aid[CANDELA_111CD],
> +                      SEQ_LENGTH_AID);
> +               break;
> +       case CANDELA_183CD ... CANDELA_300CD:
> +               memcpy(&b2[1], s6e88a0_ams427ap24_aid[CANDELA_111CD + 1],
> +                      SEQ_LENGTH_AID);
> +               break;
> +       default:
> +               dev_err(dev, "Failed to get aid data\n");
> +               return -EINVAL;
> +       }
> +
> +       /* get elvss */
> +       b6[0] = 0xb6;

Use a define per above.

> +       if (candela_enum <= CANDELA_111CD) {
> +               memcpy(&b6[1], s6e88a0_ams427ap24_elvss[0], SEQ_LENGTH_ELVSS);
> +       } else {
> +               memcpy(&b6[1], s6e88a0_ams427ap24_elvss[candela_enum - CANDELA_111CD],
> +                      SEQ_LENGTH_ELVSS);
> +       }
> +
> +       /* get gamma */
> +       ca[0] = 0xca;

#define S6E88A0_SET_GAMMA 0xca

> +       memcpy(&ca[1], s6e88a0_ams427ap24_gamma[candela_enum], SEQ_LENGTH_GAMMA);
> +
> +       /* write: key on, aid, acl off, elvss, gamma, gamma update, key off */
> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf0, 0x5a, 0x5a);

0xf0 is clearly an unlocking key as per comment in the previous patch.

> +       mipi_dsi_dcs_write_buffer_multi(&dsi_ctx, b2, ARRAY_SIZE(b2));
> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x55, 0x00);

0x55 is MIPI_DCS_WRITE_POWER_SAVE in <video/mipi_display.h>

> +       mipi_dsi_dcs_write_buffer_multi(&dsi_ctx, b6, ARRAY_SIZE(b6));
> +       mipi_dsi_dcs_write_buffer_multi(&dsi_ctx, ca, ARRAY_SIZE(ca));

> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf7, 0x03);
> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf0, 0xa5, 0xa5);

Clearly this locks the L2 access again.

Yours,
Linus Walleij

Re: [PATCH v3 5/5] drm/panel: samsung-s6e88a0-ams427ap24: Add flip option

[Linus Walleij]

Hi Jakob,

thanks for your patch!

On Thu, Oct 24, 2024 at 5:18 AM Jakob Hauser <jahau@rocketmail.com> wrote:

> The way of implementing a flip option follows the existing
> panel-samsung-s6e8aa0.c [1][2][3].

That driver is notoriously hard to read because it uses so much
magic numbers so please don't copy that aspect of the driver.

(...)

> +       if (ctx->flip_horizontal)
> +               mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xcb, 0x0e);

#define S6E88A0_SET_FLIP 0xcb
or something like this.

Other than that it looks good!

Yours,
Linus Walleij

Re: [PATCH v3 3/5] drm/panel: samsung-s6e88a0-ams427ap24: Add initial driver

[Jakob Hauser]

Hi Linus,

On 25.10.24 18:36, Linus Walleij wrote:
...
> On Thu, Oct 24, 2024 at 5:18 AM Jakob Hauser <jahau@rocketmail.com> wrote:
...
> 
>> +#include <linux/delay.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
> 
> Why do you need this include? .of_match_table is part of
> <linux/driver.h>

You're right, I'll remove it.

>> +static int s6e88a0_ams427ap24_on(struct s6e88a0_ams427ap24 *ctx)
>> +{
>> +       struct mipi_dsi_device *dsi = ctx->dsi;
>> +       struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
>> +
>> +       dsi->mode_flags |= MIPI_DSI_MODE_LPM;
>> +
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf0, 0x5a, 0x5a);
> 
> Can we provide #defines for at least some of this magic?
> See other drivers for a very good idea of what some of them mean.
> panel-samsung-s6d27a1.c:#define S6D27A1_PASSWD_L2       0xF0    /*
> Password Command for Level 2 Control */
> panel-samsung-s6d7aa0.c:#define MCS_PASSWD1             0xf0
> 
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfc, 0x5a, 0x5a);
> 
> panel-samsung-s6d7aa0.c:#define MCS_PASSWD3             0xfc
> 
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb0, 0x11);
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfd, 0x11);
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb0, 0x13);
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfd, 0x18);
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb0, 0x02);
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb8, 0x30);
> (...)
>> +       mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
>> +       mipi_dsi_msleep(&dsi_ctx, 20);
>> +
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf1, 0x5a, 0x5a);
> 
> panel-samsung-s6d7aa0.c:#define MCS_PASSWD2             0xf1
> 
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xcc, 0x4c);
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf2, 0x03, 0x0d);
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf1, 0xa5, 0xa5);
> 
> panel-samsung-s6d7aa0.c:#define MCS_PASSWD2             0xf1
> Send in the reverse password: disable access.
> 
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xca,
>> +                                    0x01, 0x00, 0x01, 0x00, 0x01, 0x00, 0x80,
>> +                                    0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
>> +                                    0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
>> +                                    0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
>> +                                    0x80, 0x80, 0x00, 0x00, 0x00);
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb2,
>> +                                    0x40, 0x08, 0x20, 0x00, 0x08);
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb6, 0x28, 0x0b);
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf7, 0x03);
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x55, 0x00);
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf0, 0xa5, 0xa5);
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfc, 0xa5, 0xa5);
> 
> Send in the reverse password: disable access.
> 
> A bit of #defines and comments would make it much more clear what
> is going on.

The difficulty I'm in is that I don't have a datasheet.

 From the Android kernel downstream driver data ams427ap24 [1], from the similar 
but older panel ams427ap01 downstream driver [2] and the similar panel 
ams452ef01 upstream driver [3] I can guess what the commands do... except I 
couldn't find out what "0xf2, 0x03, 0x0d" does and for "0xf1, 0x5a, 0x5a" I was 
guessing level 3 key on because level 1 and 2 was already there.

[1] 
https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/video/msm/mdss/samsung/S6E88A0_AMS427AP24/dsi_panel_S6E88A0_AMS427AP24_qhd_octa_video.dtsi
[2] 
https://github.com/LineageOS/android_kernel_samsung_msm8930-common/blob/lineage-15.1/drivers/video/msm/mipi_samsung_oled_video_qhd_pt-8930.c
[3] 
https://github.com/torvalds/linux/blob/v6.11/drivers/gpu/drm/panel/panel-samsung-s6e88a0-ams452ef01.c

That said, I can offer to comment the single command lines like this:

mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf0, 0x5a, 0x5a); // level 1 key on
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfc, 0x5a, 0x5a); // level 2 key on
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb0, 0x11); // src latch set global 1
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfd, 0x11); // src latch set 1
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb0, 0x13); // src latch set global 2
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfd, 0x18); // src latch set 2
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb0, 0x02); // avdd set 1
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb8, 0x30); // avdd set 2

mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
mipi_dsi_msleep(&dsi_ctx, 20);

mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf1, 0x5a, 0x5a); // level 3 key on
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xcc, 0x4c); // pixel clock divider pol.
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf2, 0x03, 0x0d); // unknown
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf1, 0xa5, 0xa5); // level 3 key off
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xca,
                              0x01, 0x00, 0x01, 0x00, 0x01, 0x00, 0x80,
                              0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
                              0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
                              0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
                              0x80, 0x80, 0x00, 0x00, 0x00); // set gamma
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb2,
                              0x40, 0x08, 0x20, 0x00, 0x08); // set aid
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb6, 0x28, 0x0b); // set elvss
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf7, 0x03); // gamma update
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x55, 0x00); // acl off
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf0, 0xa5, 0xa5); // level 1 key off
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfc, 0xa5, 0xa5); // level 2 key off

Some of those lines exceed the 80 characters limit. I would still prefer not to 
break those lines to keep readability.

Now commenting the single command lines is one thing. But giving names to the 
command registers is something different. E.g. in 0xb0 there is "src latch set 
global" but also one of the "avdd". I wouldn't know what name that command 
register should have. For 0xcc and 0xf2 in the downstream ams427ap24 they're 
both in a block labelled "PCD" and in upstream ams452ef01 the 0xcc line is 
commented as "set Pixel Clock Divider polarity" but the 0xf2 register seems to 
be more general because in downstream ams427ap24 it also shows up in a block 
called "AVC" and in downstream ams427ap01 in a command labelled "vporch". 
Guessing names for the command registers will become too arbitrary.

Additionally, I actually would like to avoid the #defines for the command 
registers because this will definitively break the 80 character lines limits 
strong enough, splintering the above command blocks into quite some mess.

Kind regards,
Jakob

Re: [PATCH v3 4/5] drm/panel: samsung-s6e88a0-ams427ap24: Add brightness control

[Jakob Hauser]

Hi Linus,

On 25.10.24 21:27, Linus Walleij wrote:
...
> On Thu, Oct 24, 2024 at 5:18 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>> +static const int s6e88a0_ams427ap24_br_to_cd[NUM_STEPS_CANDELA] = {
> 
> (...)
>> +       /* brightness till, candela */
> 
> Brightness to candela conversion table? Edit comment?

In the downstream driver there is a table with four columns:
<idx> <from> <till> <candella>.

https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/video/msm/mdss/samsung/S6E88A0_AMS427AP24/dsi_panel_S6E88A0_AMS427AP24_qhd_octa_video.dtsi#L341-L397

The first column is a counter, the second and third is the 
from-till-range of brightness steps that correspond to the forth column 
of candela identifier.

In the patch here I only adopted the third and forth column, because the 
others were not necessary. The comment "brightness till, candela" was 
intended to label those two columns.

To make it more clear, I could add the keyword "columns" and the column 
"brightness from".

         /* columns: brightness from, brightness till, candela */
         /* 0 */    10,  /* 10CD */
         /* 11 */   11,  /* 11CD */
         /* 12 */   12,  /* 12CD */
         /* 13 */   13,  /* 13CD */
         /* 14 */   14,  /* 14CD */

         ...

         /* 30 */   30,  /* 39CD */
         /* 31 */   32,  /* 41CD */
         /* 33 */   34,  /* 44CD */
         /* 35 */   36,  /* 47CD */

         ...

         /* 92 */   97,  /* 126CD */
         /* 98 */   104, /* 134CD */
         /* 105 */  110, /* 143CD */
         /* 111 */  118, /* 152CD */

         ...

         /* 182 */  205, /* 249CD */
         /* 206 */  234, /* 265CD */
         /* 235 */  254, /* 282CD */
         /* 255 */  255, /* 300CD */

>> +static const u8 s6e88a0_ams427ap24_aid[NUM_STEPS_AID][SEQ_LENGTH_AID] = {
> 
> If you know that the sequence 0xb2, 0x40, 0x08, 0x20 means "set AID"
> (or is it AOR??) you can #define
> 
> #define S6E88A0_SET_AID 0xb2

Thanks to Alexey Min, who looked this up, I can say:

"The PWM mechanism used on Samsung displays is called AOR (AMOLED off 
ratio), the related function on the kernel driver is called AID (AMOLED 
impulsive driving)."

source: xdaforums

The downstream driver of ams427ap24 uses the "aid" labeling, therefore I 
stick to that. (Interestingly the older downstream driver ams427ap01 
uses the "aor" labeling.)

> Then make a small buffer:
> 
> u8 set_aid[5] = { S6E88A0_SET_AID, 0x40, 0x08, 0x20, 0x00, 0x00 };
> 
> then you can strip the first three bytes from the entire table,
> just copy in the two relevant bytes into set_aor[]
> and send that.

Ok, I'll try to implement that. The size of the second array dimension 
of that table will then become [SEQ_LENGTH_AID - 3].

...

>> +static int s6e88a0_ams427ap24_set_brightness(struct backlight_device *bd)
>> +{
>> +       struct s6e88a0_ams427ap24 *ctx = bl_get_data(bd);
>> +       struct mipi_dsi_device *dsi = ctx->dsi;
>> +       struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
>> +       struct device *dev = &dsi->dev;
>> +       int brightness = bd->props.brightness;
>> +       int candela_enum;
>> +       u8 b2[SEQ_LENGTH_AID + 1];
>> +       u8 b6[SEQ_LENGTH_ELVSS + 1];
>> +       u8 ca[SEQ_LENGTH_GAMMA + 1];
> 
> Rename them to something like my suggestions so we understand what it is
> all about. It seems the infrastructure for what I suggested is mostly already
> there.

These defines are intended to be the sequence length of the payload for 
commands aid, elvss and gamma. The naming makes sense to me.

The "+ 1" became necessary because when changing the DCS commands to 
multi type I ran into the issue that there is one for 
"mipi_dsi_dcs_write_seq" and one for "mipi_dsi_dcs_write_buffer"... but 
none for "mipi_dsi_dcs_write" :( So I had to convert those into 
"mipi_dsi_dcs_write_buffer"+multi, thus including the command register 
value into the payload string.

...

>> +       if (candela_enum <= CANDELA_111CD) {
>> +               memcpy(&b6[1], s6e88a0_ams427ap24_elvss[0], SEQ_LENGTH_ELVSS);
>> +       } else {
>> +               memcpy(&b6[1], s6e88a0_ams427ap24_elvss[candela_enum - CANDELA_111CD],
>> +                      SEQ_LENGTH_ELVSS);
>> +       }
>> +
>> +       /* get gamma */
>> +       ca[0] = 0xca;
> 
> #define S6E88A0_SET_GAMMA 0xca

As stated in my reply on patch 3, I would like to avoid those defines 
because firstly the naming becomes arbitrary and secondly it spoils the 
readability of the larger DCS command blocks due to necessary line breaks.

In this specific case here a define would make sense. But I can hardly 
implement it here without doing it elsewhere. Therefore I would like to 
keep that as it is.

...

>> +       mipi_dsi_dcs_write_buffer_multi(&dsi_ctx, b2, ARRAY_SIZE(b2));
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x55, 0x00);
> 
> 0x55 is MIPI_DCS_WRITE_POWER_SAVE in <video/mipi_display.h>

It's the only one that could be used from <video/mipi_display.h>.

Though "MIPI_DCS_WRITE_POWER_SAVE, 0x00" doesn't say much. In the 
downstream driver there are four levels of ACL:
         0x55, 0x00 -> ACL off
         0x55, 0x01 -> default ACL 15 %
         0x55, 0x02 -> ACL 30 %, also corresponds to the "ACL on" command
         0x55, 0x03 -> doesn't seem to be used

https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/video/msm/mdss/samsung/S6E88A0_AMS427AP24/dsi_panel_S6E88A0_AMS427AP24_qhd_octa_video.dtsi#L275-L281

I would prefer to stay at 0x55 and add comment "acl off". Embedded in a 
block of other DCS commands with plain command register values and 
single line comments appended, as proposed in my reply on patch 3, it 
looks more readable and descriptive in the context of the other commands.

Kind regards,
Jakob

Re: [PATCH v3 5/5] drm/panel: samsung-s6e88a0-ams427ap24: Add flip option

[Jakob Hauser]

Hi Linus,

On 25.10.24 21:32, Linus Walleij wrote:
...
> On Thu, Oct 24, 2024 at 5:18 AM Jakob Hauser <jahau@rocketmail.com> wrote:
> 
>> The way of implementing a flip option follows the existing
>> panel-samsung-s6e8aa0.c [1][2][3].
> 
> That driver is notoriously hard to read because it uses so much
> magic numbers so please don't copy that aspect of the driver.

Actually I used that same approach for the flip option. The 
implementation of flip looked very well to me. I wanted to state it as 
source in the commit message. I don't feel comfortable without declaring it.

> (...)
> 
>> +       if (ctx->flip_horizontal)
>> +               mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xcb, 0x0e);
> 
> #define S6E88A0_SET_FLIP 0xcb
> or something like this.

The 0xcb command register might be more a general panel control, not 
only serving panel flip. But I can't say for sure. Translating single 
commands to command register names becomes too arbitrary.

...

Kind regards,
Jakob