[PATCH v3 0/3] Add Samsung S6D7AA0 panel controller driver

[PATCH v3 0/3] Add Samsung S6D7AA0 panel controller driver

[Artur Weber]

This patchset adds initial support for Samsung S6D7AA0-based panels.
Currently, only the S6D7AA0-LSL080AL02 panel used in the Samsung
Galaxy Tab 3 8.0 family of tablets is supported; other panels can be
added in the future.

Changed in v2:
 - Added commit messages for dt-bindings and MAINTAINERS entry commits
 - dt-bindings: Applied suggestions from Krzysztof Kozlowski
 - driver: Removed unused panel_name property from desc struct

Changed in v3:
 - Correctly applied patch for dt-bindings

Artur Weber (3):
  dt-bindings: display: panel: Add Samsung S6D7AA0 LCD panel controller
  drm/panel: Add Samsung S6D7AA0 panel controller driver
  MAINTAINERS: Add entry for Samsung S6D7AA0 LCD panel controller driver

 .../display/panel/samsung,s6d7aa0.yaml        |  52 +++
 MAINTAINERS                                   |   6 +
 drivers/gpu/drm/panel/Kconfig                 |   7 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c | 395 ++++++++++++++++++
 5 files changed, 461 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/samsung,s6d7aa0.yaml
 create mode 100644 drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c

-- 
2.40.0

[PATCH v3 1/3] dt-bindings: display: panel: Add Samsung S6D7AA0 LCD panel controller

[Artur Weber]

Add bindings for the S6D7AA0 LCD panel controller, including the
S6D7AA0-LSL080AL02 panel used in the Samsung Galaxy Tab 3 8.0 family
of tablets.

Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
---
Changed in v2:
 - Updated commit message
 - Applied suggestions from Krzysztof Kozlowski
Changed in v3:
 - Correctly applied suggestions
---
 .../display/panel/samsung,s6d7aa0.yaml        | 52 +++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/samsung,s6d7aa0.yaml

diff --git a/Documentation/devicetree/bindings/display/panel/samsung,s6d7aa0.yaml b/Documentation/devicetree/bindings/display/panel/samsung,s6d7aa0.yaml
new file mode 100644
index 000000000000..079e2b3a9eaf
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/samsung,s6d7aa0.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/samsung,s6d7aa0.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Samsung S6D7AA0 MIPI-DSI LCD panel controller
+
+maintainers:
+  - Artur Weber <aweber.kernel@gmail.com>
+
+allOf:
+  - $ref: panel-common.yaml#
+
+properties:
+  compatible:
+    const: samsung,s6d7aa0-lsl080al02
+
+  reg: true
+  reset-gpios: true
+  backlight: true
+
+  enable-supply:
+    description: Enable supply
+
+required:
+  - compatible
+  - reg
+  - reset-gpios
+  - backlight
+  - enable-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    dsi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        panel@0 {
+            compatible = "samsung,s6d7aa0-lsl080al02";
+            reg = <0>;
+            reset-gpios = <&gpf0 4 GPIO_ACTIVE_LOW>;
+            backlight = <&backlight>;
+            enable-supply = <&lcd_enable_supply>;
+        };
+    };
+
+...
-- 
2.40.0

Re: [PATCH v3 1/3] dt-bindings: display: panel: Add Samsung S6D7AA0 LCD panel controller

[Rob Herring]

On Sun, Apr 16, 2023 at 03:16:30PM +0200, Artur Weber wrote:
> Add bindings for the S6D7AA0 LCD panel controller, including the
> S6D7AA0-LSL080AL02 panel used in the Samsung Galaxy Tab 3 8.0 family
> of tablets.
> 
> Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
> ---
> Changed in v2:
>  - Updated commit message
>  - Applied suggestions from Krzysztof Kozlowski
> Changed in v3:
>  - Correctly applied suggestions
> ---
>  .../display/panel/samsung,s6d7aa0.yaml        | 52 +++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/samsung,s6d7aa0.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/samsung,s6d7aa0.yaml b/Documentation/devicetree/bindings/display/panel/samsung,s6d7aa0.yaml
> new file mode 100644
> index 000000000000..079e2b3a9eaf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/samsung,s6d7aa0.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/panel/samsung,s6d7aa0.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Samsung S6D7AA0 MIPI-DSI LCD panel controller
> +
> +maintainers:
> +  - Artur Weber <aweber.kernel@gmail.com>
> +
> +allOf:
> +  - $ref: panel-common.yaml#
> +
> +properties:
> +  compatible:
> +    const: samsung,s6d7aa0-lsl080al02
> +
> +  reg: true
> +  reset-gpios: true
> +  backlight: true
> +
> +  enable-supply:
> +    description: Enable supply

Comments on v1 apply here.

Rob

Re: [PATCH v3 1/3] dt-bindings: display: panel: Add Samsung S6D7AA0 LCD panel controller

[Linus Walleij]

Hi Artur,

thanks for your patch!

On Sun, Apr 16, 2023 at 3:16 PM Artur Weber <aweber.kernel@gmail.com> wrote:

> Add bindings for the S6D7AA0 LCD panel controller, including the
> S6D7AA0-LSL080AL02 panel used in the Samsung Galaxy Tab 3 8.0 family
> of tablets.
>
> Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
(...)

> +  reset-gpios: true
(...)
> +            reset-gpios = <&gpf0 4 GPIO_ACTIVE_LOW>;

Since reset-gpios is always active low, add a description: to reset-gpios
saying that it should nominally be tagged GPIO_ACTIVE_LOW.

Yours.
Linus Walleij

[PATCH v3 2/3] drm/panel: Add Samsung S6D7AA0 panel controller driver

[Artur Weber]

Initial driver for S6D7AA0-controlled panels, currently only for the
LSL080AL02 panel used in the Samsung Galaxy Tab 3 8.0 family of tablets.

It should be possible to extend this driver to work with other panels
using this IC.

Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
---
Changed in v2:
 - Removed unused panel_name property from desc struct
---
 drivers/gpu/drm/panel/Kconfig                 |   7 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c | 395 ++++++++++++++++++
 3 files changed, 403 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 8eeee71c0000..2ce9d1a45625 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -532,6 +532,13 @@ config DRM_PANEL_SAMSUNG_S6D27A1
 	  This panel can be found in Samsung Galaxy Ace 2
 	  GT-I8160 mobile phone.
 
+config DRM_PANEL_SAMSUNG_S6D7AA0
+	tristate "Samsung S6D7AA0 MIPI-DSI video mode panel controller"
+	depends on OF
+	depends on BACKLIGHT_CLASS_DEVICE
+	select DRM_MIPI_DSI
+	select VIDEOMODE_HELPERS
+
 config DRM_PANEL_SAMSUNG_S6E3HA2
 	tristate "Samsung S6E3HA2 DSI video mode panel"
 	depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index c05aa9e23907..193f3067865d 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -52,6 +52,7 @@ obj-$(CONFIG_DRM_PANEL_SAMSUNG_DB7430) += panel-samsung-db7430.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) += panel-samsung-ld9040.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6D16D0) += panel-samsung-s6d16d0.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6D27A1) += panel-samsung-s6d27a1.o
+obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6D7AA0) += panel-samsung-s6d7aa0.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E3HA2) += panel-samsung-s6e3ha2.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E63J0X03) += panel-samsung-s6e63j0x03.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E63M0) += panel-samsung-s6e63m0.o
diff --git a/drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c b/drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c
new file mode 100644
index 000000000000..408e4082de74
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c
@@ -0,0 +1,395 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Samsung S6D7AA0 MIPI-DSI TFT LCD controller drm_panel driver.
+ *
+ * Copyright (C) 2022 Artur Weber <aweber.kernel@gmail.com>
+ */
+
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/regulator/consumer.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+
+#include <video/mipi_display.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_panel.h>
+
+/* Manufacturer command set */
+#define CMD_BL_CTL		0xc3
+#define CMD_OTP_RELOAD		0xd0
+#define CMD_PASSWD1		0xf0
+#define CMD_PASSWD2		0xf1
+#define CMD_PASSWD3		0xfc
+
+struct s6d7aa0 {
+	struct drm_panel panel;
+	struct mipi_dsi_device *dsi;
+	struct gpio_desc *reset_gpio;
+	struct regulator *enable_supply;
+	const struct s6d7aa0_panel_desc *desc;
+	bool prepared;
+};
+
+struct s6d7aa0_panel_desc {
+	int (*init_func)(struct s6d7aa0 *ctx);
+	const struct drm_display_mode drm_mode;
+	unsigned long mode_flags;
+	u32 bus_flags;
+	bool use_passwd3;
+};
+
+static inline struct s6d7aa0 *panel_to_s6d7aa0(struct drm_panel *panel)
+{
+	return container_of(panel, struct s6d7aa0, panel);
+}
+
+static void s6d7aa0_reset(struct s6d7aa0 *ctx)
+{
+	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
+	msleep(50);
+	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
+	msleep(50);
+	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
+	msleep(50);
+}
+
+static int s6d7aa0_lock(struct s6d7aa0 *ctx, bool lock)
+{
+	struct mipi_dsi_device *dsi = ctx->dsi;
+
+	if (lock) {
+		mipi_dsi_dcs_write_seq(dsi, CMD_PASSWD1, 0xa5, 0xa5);
+		mipi_dsi_dcs_write_seq(dsi, CMD_PASSWD2, 0xa5, 0xa5);
+		if (ctx->desc->use_passwd3)
+			mipi_dsi_dcs_write_seq(dsi, CMD_PASSWD3, 0x5a, 0x5a);
+	} else {
+		mipi_dsi_dcs_write_seq(dsi, CMD_PASSWD1, 0x5a, 0x5a);
+		mipi_dsi_dcs_write_seq(dsi, CMD_PASSWD2, 0x5a, 0x5a);
+		if (ctx->desc->use_passwd3)
+			mipi_dsi_dcs_write_seq(dsi, CMD_PASSWD3, 0xa5, 0xa5);
+	}
+
+	return 0;
+}
+
+static int s6d7aa0_bl_ctl_on(struct s6d7aa0 *ctx, bool enable)
+{
+	struct mipi_dsi_device *dsi = ctx->dsi;
+
+	if (enable)
+		mipi_dsi_dcs_write_seq(dsi, CMD_BL_CTL, 0x40, 0x00, 0x28);
+	else
+		mipi_dsi_dcs_write_seq(dsi, CMD_BL_CTL, 0x40, 0x00, 0x20);
+
+	return 0;
+}
+
+static int s6d7aa0_on(struct s6d7aa0 *ctx)
+{
+	struct mipi_dsi_device *dsi = ctx->dsi;
+	struct device *dev = &dsi->dev;
+	int ret;
+
+	dsi->mode_flags |= MIPI_DSI_MODE_LPM;
+
+	s6d7aa0_reset(ctx);
+
+	ret = ctx->desc->init_func(ctx);
+	if (ret < 0) {
+		dev_err(dev, "Failed to initialize panel: %d\n", ret);
+		gpiod_set_value_cansleep(ctx->reset_gpio, 1);
+		return ret;
+	}
+
+	ret = mipi_dsi_dcs_set_display_on(dsi);
+	if (ret < 0) {
+		dev_err(dev, "Failed to set display on: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int s6d7aa0_off(struct s6d7aa0 *ctx)
+{
+	struct mipi_dsi_device *dsi = ctx->dsi;
+	struct device *dev = &dsi->dev;
+	int ret;
+
+	dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
+
+	ret = mipi_dsi_dcs_set_display_off(dsi);
+	if (ret < 0) {
+		dev_err(dev, "Failed to set display off: %d\n", ret);
+		return ret;
+	}
+	msleep(64);
+
+	ret = s6d7aa0_bl_ctl_on(ctx, false);
+	if (ret < 0) {
+		dev_err(dev, "Failed to disable backlight control: %d\n", ret);
+		return ret;
+	}
+	usleep_range(1000, 1500);
+
+	ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
+	if (ret < 0) {
+		dev_err(dev, "Failed to enter sleep mode: %d\n", ret);
+		return ret;
+	}
+	msleep(120);
+
+	return 0;
+}
+
+static int s6d7aa0_prepare(struct drm_panel *panel)
+{
+	struct s6d7aa0 *ctx = panel_to_s6d7aa0(panel);
+	struct device *dev = &ctx->dsi->dev;
+	int ret;
+
+	if (ctx->prepared)
+		return 0;
+
+	ret = regulator_enable(ctx->enable_supply);
+	if (ret) {
+		dev_err(dev, "Failed to enable regulator: %d\n", ret);
+		return ret;
+	}
+
+	ret = s6d7aa0_on(ctx);
+	if (ret < 0) {
+		dev_err(dev, "Failed to initialize panel: %d\n", ret);
+		gpiod_set_value_cansleep(ctx->reset_gpio, 1);
+		return ret;
+	}
+
+	ctx->prepared = true;
+	return 0;
+}
+
+static int s6d7aa0_disable(struct drm_panel *panel)
+{
+	struct s6d7aa0 *ctx = panel_to_s6d7aa0(panel);
+	struct device *dev = &ctx->dsi->dev;
+	int ret;
+
+	ret = s6d7aa0_off(ctx);
+	if (ret < 0)
+		dev_err(dev, "Failed to un-initialize panel: %d\n", ret);
+
+	return 0;
+}
+
+static int s6d7aa0_unprepare(struct drm_panel *panel)
+{
+	struct s6d7aa0 *ctx = panel_to_s6d7aa0(panel);
+
+	if (!ctx->prepared)
+		return 0;
+
+	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
+	regulator_disable(ctx->enable_supply);
+
+	ctx->prepared = false;
+	return 0;
+}
+
+/* Initialization code and structures for LSL080AL02 panel */
+
+static int s6d7aa0_lsl080al02_init(struct s6d7aa0 *ctx)
+{
+	struct mipi_dsi_device *dsi = ctx->dsi;
+	struct device *dev = &dsi->dev;
+	int ret;
+
+	usleep_range(20000, 25000);
+
+	ret = s6d7aa0_lock(ctx, false);
+	if (ret < 0) {
+		dev_err(dev, "Failed to unlock registers: %d\n", ret);
+		return ret;
+	}
+
+	mipi_dsi_dcs_write_seq(dsi, CMD_OTP_RELOAD, 0x00, 0x10);
+	usleep_range(1000, 1500);
+
+	mipi_dsi_dcs_write_seq(dsi, 0xb6, 0x10); /* SEQ_B6_PARAM_8_R01 */
+
+	ret = s6d7aa0_bl_ctl_on(ctx, true);
+	if (ret < 0) {
+		dev_err(dev, "Failed to enable backlight control: %d\n", ret);
+		return ret;
+	}
+	usleep_range(5000, 6000);
+	mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_SET_ADDRESS_MODE, 0x04);
+
+	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
+	if (ret < 0) {
+		dev_err(dev, "Failed to exit sleep mode: %d\n", ret);
+		return ret;
+	}
+
+	msleep(120);
+	mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_SET_ADDRESS_MODE, 0x00);
+
+	ret = s6d7aa0_lock(ctx, true);
+	if (ret < 0) {
+		dev_err(dev, "Failed to lock registers: %d\n", ret);
+		return ret;
+	}
+
+	ret = mipi_dsi_dcs_set_display_on(dsi);
+	if (ret < 0) {
+		dev_err(dev, "Failed to set display on: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+};
+
+static const struct drm_display_mode s6d7aa0_lsl080al02_mode = {
+	.clock = (800 + 16 + 4 + 140) * (1280 + 8 + 4 + 4) * 60 / 1000,
+	.hdisplay = 800,
+	.hsync_start = 800 + 16,
+	.hsync_end = 800 + 16 + 4,
+	.htotal = 800 + 16 + 4 + 140,
+	.vdisplay = 1280,
+	.vsync_start = 1280 + 8,
+	.vsync_end = 1280 + 8 + 4,
+	.vtotal = 1280 + 8 + 4 + 4,
+	.width_mm = 108,
+	.height_mm = 173,
+};
+
+static const struct s6d7aa0_panel_desc s6d7aa0_lsl080al02_desc = {
+	.init_func = s6d7aa0_lsl080al02_init,
+	.drm_mode = s6d7aa0_lsl080al02_mode,
+	.mode_flags = MIPI_DSI_MODE_VSYNC_FLUSH,
+	.bus_flags = DRM_BUS_FLAG_DE_HIGH,
+	.use_passwd3 = false,
+};
+
+static int s6d7aa0_get_modes(struct drm_panel *panel,
+					struct drm_connector *connector)
+{
+	struct drm_display_mode *mode;
+	struct s6d7aa0 *ctx;
+
+	ctx = container_of(panel, struct s6d7aa0, panel);
+	if (!ctx)
+		return -EINVAL;
+
+	mode = drm_mode_duplicate(connector->dev, &ctx->desc->drm_mode);
+	if (!mode)
+		return -ENOMEM;
+
+	drm_mode_set_name(mode);
+
+	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
+	connector->display_info.width_mm = mode->width_mm;
+	connector->display_info.height_mm = mode->height_mm;
+	connector->display_info.bus_flags = ctx->desc->bus_flags;
+	drm_mode_probed_add(connector, mode);
+
+	return 1;
+}
+
+static const struct drm_panel_funcs s6d7aa0_panel_funcs = {
+	.disable = s6d7aa0_disable,
+	.prepare = s6d7aa0_prepare,
+	.unprepare = s6d7aa0_unprepare,
+	.get_modes = s6d7aa0_get_modes,
+};
+
+static int s6d7aa0_probe(struct mipi_dsi_device *dsi)
+{
+	struct device *dev = &dsi->dev;
+	struct s6d7aa0 *ctx;
+	int ret;
+
+	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->desc = of_device_get_match_data(dev);
+	if (!ctx->desc)
+		return -ENODEV;
+
+	ctx->enable_supply = devm_regulator_get(dev, "enable");
+	if (IS_ERR(ctx->enable_supply))
+		return dev_err_probe(dev, PTR_ERR(ctx->enable_supply),
+				     "Failed to get enable supply\n");
+
+	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 = 4;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST
+		| ctx->desc->mode_flags;
+
+	drm_panel_init(&ctx->panel, dev, &s6d7aa0_panel_funcs,
+		       DRM_MODE_CONNECTOR_DSI);
+	ctx->panel.prepare_prev_first = true;
+
+	ret = drm_panel_of_backlight(&ctx->panel);
+	if (ret) {
+		dev_err_probe(dev, ret, "Could not find backlight\n");
+		return ret;
+	}
+
+	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 s6d7aa0_remove(struct mipi_dsi_device *dsi)
+{
+	struct s6d7aa0 *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 s6d7aa0_of_match[] = {
+	{
+		.compatible = "samsung,s6d7aa0-lsl080al02",
+		.data = &s6d7aa0_lsl080al02_desc
+	},
+	{ /* sentinel */ }
+};
+
+static struct mipi_dsi_driver s6d7aa0_driver = {
+	.probe = s6d7aa0_probe,
+	.remove = s6d7aa0_remove,
+	.driver = {
+		.name = "panel-samsung-s6d7aa0",
+		.of_match_table = s6d7aa0_of_match,
+	},
+};
+module_mipi_dsi_driver(s6d7aa0_driver);
+
+MODULE_AUTHOR("Artur Weber <aweber.kernel@gmail.com>");
+MODULE_DESCRIPTION("Samsung S6D7AA0 MIPI-DSI LCD controller driver");
+MODULE_LICENSE("GPL");
-- 
2.40.0

Re: [PATCH v3 2/3] drm/panel: Add Samsung S6D7AA0 panel controller driver

[Linus Walleij]

Hi Artur,

thanks for your patch!

On Sun, Apr 16, 2023 at 3:16 PM Artur Weber <aweber.kernel@gmail.com> wrote:

> Initial driver for S6D7AA0-controlled panels, currently only for the
> LSL080AL02 panel used in the Samsung Galaxy Tab 3 8.0 family of tablets.
>
> It should be possible to extend this driver to work with other panels
> using this IC.
>
> Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
> ---
> Changed in v2:
>  - Removed unused panel_name property from desc struct

Overall this driver looks very good. I could merge it once the DT bindings
are ACKed by the DT maintainers and some minor stuff fixed.

Some comments below:

> +/* Manufacturer command set */
> +#define CMD_BL_CTL             0xc3
> +#define CMD_OTP_RELOAD         0xd0
> +#define CMD_PASSWD1            0xf0
> +#define CMD_PASSWD2            0xf1
> +#define CMD_PASSWD3            0xfc

Some drivers prefix these commands with "MCS" such as
MCS_BL_CTL.

MCS = Manufacturer Command Set (I think)

Some just name the identifers after the panel such as
s6d27a1 which has S6D27A1_RESCTL etc.

CMD seems a bit general to me and may be mistaken for
the actual DCS commands.

> +struct s6d7aa0 {
> +       struct drm_panel panel;
> +       struct mipi_dsi_device *dsi;
> +       struct gpio_desc *reset_gpio;
> +       struct regulator *enable_supply;
> +       const struct s6d7aa0_panel_desc *desc;
> +       bool prepared;

Skip this state variable, the core keeps track of whether the
panel is enabled or not.

> +static void s6d7aa0_reset(struct s6d7aa0 *ctx)
> +{
> +       gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> +       msleep(50);

This first de-assertion is unnecessary isn't it?

The reset line will just be asserted longer if it is already asserted.

> +       gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> +       msleep(50);
> +       gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> +       msleep(50);
> +}

(...)

> +static int s6d7aa0_on(struct s6d7aa0 *ctx)
> +{
> +       struct mipi_dsi_device *dsi = ctx->dsi;
> +       struct device *dev = &dsi->dev;
> +       int ret;
> +
> +       dsi->mode_flags |= MIPI_DSI_MODE_LPM;

(...)

> +static int s6d7aa0_off(struct s6d7aa0 *ctx)
> +{
> +       struct mipi_dsi_device *dsi = ctx->dsi;
> +       struct device *dev = &dsi->dev;
> +       int ret;
> +
> +       dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;

I haven't seen this mode flag MIPI_DSI_MODE_LPM set and
masked in other DSI panel drivers! Is this something we should
fix everywhere then? Or even something the core should be
doing?

Yours,
Linus Walleij

Re: [PATCH v3 2/3] drm/panel: Add Samsung S6D7AA0 panel controller driver

[Artur Weber]

Hi,

thank you for the review.

On 20/04/2023 09:35, Linus Walleij wrote:
>> +static int s6d7aa0_on(struct s6d7aa0 *ctx)
>> +{
>> +       struct mipi_dsi_device *dsi = ctx->dsi;
>> +       struct device *dev = &dsi->dev;
>> +       int ret;
>> +
>> +       dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> 
> (...)
> 
>> +static int s6d7aa0_off(struct s6d7aa0 *ctx)
>> +{
>> +       struct mipi_dsi_device *dsi = ctx->dsi;
>> +       struct device *dev = &dsi->dev;
>> +       int ret;
>> +
>> +       dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> 
> I haven't seen this mode flag MIPI_DSI_MODE_LPM set and
> masked in other DSI panel drivers! Is this something we should
> fix everywhere then? Or even something the core should be
> doing?

These bits were included in a driver for a similar panel with the same
controller in an MSM8916 close-to-mainline kernel fork[1]; that driver
was generated with lmdpdg[2], which adds the LPM mode flag automatically
based on some downstream DTS property. In this case, I left it in, since
it didn't seem to break anything... but I just re-tested without it and
it seems that it might've fixed some odd issues I'd get sometimes when
going out of sleep mode. I'll get rid of it in the next version.

(I based my panel driver off that driver; now that I think about it, it
might be worth mentioning somewhere in the copyright notice...?)

Best regards
Artur Weber

[1]
https://github.com/msm8916-mainline/linux/blob/msm8916/6.3-rc7/drivers/gpu/drm/panel/msm8916-generated/panel-samsung-s6d7aa0-lsl080al03.c
[2]
https://github.com/msm8916-mainline/linux-mdss-dsi-panel-driver-generator

[PATCH v3 3/3] MAINTAINERS: Add entry for Samsung S6D7AA0 LCD panel controller driver

[Artur Weber]

Add myself as maintainer of the Samsung S6D7AA0 panel driver.

Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 90abe83c02f3..6a55e6e3ac59 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6623,6 +6623,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/display/panel/samsung,s6d27a1.yaml
 F:	drivers/gpu/drm/panel/panel-samsung-s6d27a1.c
 
+DRM DRIVER FOR SAMSUNG S6D7AA0 PANELS
+M:	Artur Weber <aweber.kernel@gmail.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/display/panel/samsung,s6d7aa0.yaml
+F:	drivers/gpu/drm/panel/panel-samsung-s6d7aa0.c
+
 DRM DRIVER FOR SITRONIX ST7703 PANELS
 M:	Guido Günther <agx@sigxcpu.org>
 R:	Purism Kernel Team <kernel@puri.sm>
-- 
2.40.0