linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 1/2] dt-bindings: panel: Add Samsung S6E3FA2 panel
@ 2021-02-01 16:53 Iskren Chernev
  2021-02-01 16:53 ` [PATCH v3 2/2] drm/panel: Add panel for Samsung Galaxy S5 Iskren Chernev
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Iskren Chernev @ 2021-02-01 16:53 UTC (permalink / raw)
  To: Rob Herring, Sam Ravnborg, Thierry Reding
  Cc: Daniel Vetter, David Airlie, devicetree, dri-devel, linux-kernel,
	phone-devel, ~postmarketos/upstreaming, Iskren Chernev

The Samsung S6E3FA2 AMOLED cmd LCD panel is used on the Samsung Galaxy
S5 (klte).

Signed-off-by: Iskren Chernev <iskren.chernev@gmail.com>
---
Add a simple generated panel driver that supports on/off and the corresponding
binding documentation.

Changes in v3:
- fix dt_binding_check issue with missing include
- fix panel type (cmd) in kconfig description

Changes in v2:
- move bindings to separate file, add 2 regulators
- add standalone panel driver

v1: https://lkml.org/lkml/2020/12/30/293
v2: https://lkml.org/lkml/2021/2/1/313

 .../display/panel/samsung,s6e3fa2.yaml        | 64 +++++++++++++++++++
 1 file changed, 64 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/samsung,s6e3fa2.yaml

diff --git a/Documentation/devicetree/bindings/display/panel/samsung,s6e3fa2.yaml b/Documentation/devicetree/bindings/display/panel/samsung,s6e3fa2.yaml
new file mode 100644
index 000000000000..c751ad589480
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/samsung,s6e3fa2.yaml
@@ -0,0 +1,64 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/samsung,s6e3fa2.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Samsung s6e3fa2 AMOLED CMD LCD panel
+
+maintainers:
+  - Iskren Chernev <iskren.chernev@gmail.com>
+
+allOf:
+  - $ref: panel-common.yaml#
+
+properties:
+  compatible:
+    const: samsung,s6e3fa2
+
+  reg: true
+  reset-gpios: true
+  port: true
+
+  iovdd-supply:
+    description: IOVDD regulator
+
+  vddr-supply:
+    description: VDDR regulator
+
+required:
+  - compatible
+  - reset-gpios
+  - iovdd-supply
+  - vddr-supply
+  - port
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    /* from Samsung Galaxy S5 klte */
+    #include <dt-bindings/gpio/gpio.h>
+
+    dsi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        panel@0 {
+            compatible = "samsung,s6e3fa2";
+            reg = <0>;
+
+            reset-gpios = <&pma8084_gpios 17 GPIO_ACTIVE_LOW>;
+
+            iovdd-supply = <&pma8084_lvs4>;
+            vddr-supply = <&vreg_panel>;
+
+            port {
+                panel_in: endpoint {
+                    remote-endpoint = <&dsi0_out>;
+                };
+            };
+        };
+    };
+
+...

base-commit: fd821bf0ed9a7db09d2e007df697f4d9ecfda99a
--
2.30.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v3 2/2] drm/panel: Add panel for Samsung Galaxy S5
  2021-02-01 16:53 [PATCH v3 1/2] dt-bindings: panel: Add Samsung S6E3FA2 panel Iskren Chernev
@ 2021-02-01 16:53 ` Iskren Chernev
  2021-07-26  7:34   ` Linus Walleij
  2021-02-09 20:55 ` [PATCH v3 1/2] dt-bindings: panel: Add Samsung S6E3FA2 panel Rob Herring
  2021-07-26  7:14 ` Linus Walleij
  2 siblings, 1 reply; 6+ messages in thread
From: Iskren Chernev @ 2021-02-01 16:53 UTC (permalink / raw)
  To: Rob Herring, Sam Ravnborg, Thierry Reding
  Cc: Daniel Vetter, David Airlie, devicetree, dri-devel, linux-kernel,
	phone-devel, ~postmarketos/upstreaming, Iskren Chernev

The Samsung Galaxy S5 uses the samsung s6e3fa2 AMOLED cmd LCD panel.

This driver was generated with [1], with the addition of
mipi_dsi_dcs_set_display_on at the end of the on method.

[1] https://github.com/msm8916-mainline/linux-mdss-dsi-panel-driver-generator

Signed-off-by: Iskren Chernev <iskren.chernev@gmail.com>
---
 drivers/gpu/drm/panel/Kconfig                 |   6 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 drivers/gpu/drm/panel/panel-samsung-s6e3fa2.c | 299 ++++++++++++++++++
 3 files changed, 306 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-samsung-s6e3fa2.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 4894913936e9..82dff2afd5f1 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -348,6 +348,12 @@ config DRM_PANEL_SAMSUNG_S6D16D0
 	depends on DRM_MIPI_DSI
 	select VIDEOMODE_HELPERS
 
+config DRM_PANEL_SAMSUNG_S6E3FA2
+	tristate "Samsung S6E3FA2 DSI cmd mode panel"
+	depends on OF
+	depends on 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 cae4d976c069..87d3f76f050e 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -35,6 +35,7 @@ obj-$(CONFIG_DRM_PANEL_RAYDIUM_RM68200) += panel-raydium-rm68200.o
 obj-$(CONFIG_DRM_PANEL_RONBO_RB070D30) += panel-ronbo-rb070d30.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_S6E3FA2) += panel-samsung-s6e3fa2.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-s6e3fa2.c b/drivers/gpu/drm/panel/panel-samsung-s6e3fa2.c
new file mode 100644
index 000000000000..8985fccf9792
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-samsung-s6e3fa2.c
@@ -0,0 +1,299 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// Copyright (c) 2021 Iskren Chernev <iskren.chernev@gmail.com>
+// Generated with linux-mdss-dsi-panel-driver-generator from vendor device tree:
+// Copyright (c) 2021, The Linux Foundation. All rights reserved.
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regulator/consumer.h>
+
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_panel.h>
+
+struct samsung_s6e3fa2 {
+	struct drm_panel panel;
+	struct mipi_dsi_device *dsi;
+	struct regulator_bulk_data supplies[2];
+	struct gpio_desc *reset_gpio;
+	bool prepared;
+};
+
+static inline
+struct samsung_s6e3fa2 *to_samsung_s6e3fa2(struct drm_panel *panel)
+{
+	return container_of(panel, struct samsung_s6e3fa2, panel);
+}
+
+#define dsi_generic_write_seq(dsi, seq...) do {				\
+		static const u8 d[] = { seq };				\
+		int ret;						\
+		ret = mipi_dsi_generic_write(dsi, d, ARRAY_SIZE(d));	\
+		if (ret < 0)						\
+			return ret;					\
+	} while (0)
+
+#define dsi_dcs_write_seq(dsi, seq...) do {				\
+		static const u8 d[] = { seq };				\
+		int ret;						\
+		ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d));	\
+		if (ret < 0)						\
+			return ret;					\
+	} while (0)
+
+static void samsung_s6e3fa2_reset(struct samsung_s6e3fa2 *ctx)
+{
+	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
+	usleep_range(5000, 6000);
+	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
+	usleep_range(5000, 6000);
+	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
+	usleep_range(7000, 8000);
+}
+
+static int samsung_s6e3fa2_on(struct samsung_s6e3fa2 *ctx)
+{
+	struct mipi_dsi_device *dsi = ctx->dsi;
+	struct device *dev = &dsi->dev;
+	int ret;
+
+	dsi->mode_flags |= MIPI_DSI_MODE_LPM;
+
+	dsi_generic_write_seq(dsi, 0xf0, 0x5a, 0x5a);
+	dsi_generic_write_seq(dsi, 0xfc, 0x5a, 0x5a);
+	dsi_dcs_write_seq(dsi, 0xf2);
+	dsi_dcs_write_seq(dsi, 0xf9);
+	usleep_range(5000, 6000);
+
+	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(20);
+
+	dsi_generic_write_seq(dsi, 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);
+	dsi_generic_write_seq(dsi, 0xb2, 0x00, 0x0e, 0x00, 0x0e);
+	dsi_generic_write_seq(dsi, 0xb6,
+			      0x98, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00, 0x03,
+			      0x55, 0x54, 0x20, 0x00, 0x0a, 0xaa, 0xaf, 0x0f,
+			      0x02, 0x22, 0x22, 0x10);
+	dsi_generic_write_seq(dsi, 0xb5, 0x41);
+	dsi_generic_write_seq(dsi, 0xf7, 0x03);
+	dsi_generic_write_seq(dsi, 0xf7, 0x00);
+	dsi_generic_write_seq(dsi, 0xb0, 0x02);
+	dsi_generic_write_seq(dsi, 0xfd, 0x0a);
+	dsi_generic_write_seq(dsi, 0xfe, 0x80);
+	dsi_generic_write_seq(dsi, 0xfe, 0x00);
+	dsi_generic_write_seq(dsi, 0x35, 0x00);
+	dsi_generic_write_seq(dsi, 0xbd, 0x05, 0x02, 0x02);
+	dsi_generic_write_seq(dsi, 0xf0, 0xa5, 0xa5);
+	dsi_generic_write_seq(dsi, 0xfc, 0xa5, 0xa5);
+
+	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 samsung_s6e3fa2_off(struct samsung_s6e3fa2 *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;
+	}
+	usleep_range(10000, 11000);
+
+	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 samsung_s6e3fa2_prepare(struct drm_panel *panel)
+{
+	struct samsung_s6e3fa2 *ctx = to_samsung_s6e3fa2(panel);
+	struct device *dev = &ctx->dsi->dev;
+	int ret;
+
+	if (ctx->prepared)
+		return 0;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
+	if (ret < 0) {
+		dev_err(dev, "Failed to enable regulators: %d\n", ret);
+		return ret;
+	}
+
+	samsung_s6e3fa2_reset(ctx);
+
+	ret = samsung_s6e3fa2_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(ctx->supplies), ctx->supplies);
+		return ret;
+	}
+
+	ctx->prepared = true;
+	return 0;
+}
+
+static int samsung_s6e3fa2_unprepare(struct drm_panel *panel)
+{
+	struct samsung_s6e3fa2 *ctx = to_samsung_s6e3fa2(panel);
+	struct device *dev = &ctx->dsi->dev;
+	int ret;
+
+	if (!ctx->prepared)
+		return 0;
+
+	ret = samsung_s6e3fa2_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(ctx->supplies), ctx->supplies);
+
+	ctx->prepared = false;
+	return 0;
+}
+
+static const struct drm_display_mode samsung_s6e3fa2_mode = {
+	.clock = (1080 + 162 + 10 + 36) * (1920 + 13 + 2 + 3) * 60 / 1000,
+	.hdisplay = 1080,
+	.hsync_start = 1080 + 162,
+	.hsync_end = 1080 + 162 + 10,
+	.htotal = 1080 + 162 + 10 + 36,
+	.vdisplay = 1920,
+	.vsync_start = 1920 + 13,
+	.vsync_end = 1920 + 13 + 2,
+	.vtotal = 1920 + 13 + 2 + 3,
+	.width_mm = 65,
+	.height_mm = 115,
+};
+
+static int samsung_s6e3fa2_get_modes(struct drm_panel *panel,
+				     struct drm_connector *connector)
+{
+	struct drm_display_mode *mode;
+
+	mode = drm_mode_duplicate(connector->dev, &samsung_s6e3fa2_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;
+	drm_mode_probed_add(connector, mode);
+
+	return 1;
+}
+
+static const struct drm_panel_funcs samsung_s6e3fa2_panel_funcs = {
+	.prepare = samsung_s6e3fa2_prepare,
+	.unprepare = samsung_s6e3fa2_unprepare,
+	.get_modes = samsung_s6e3fa2_get_modes,
+};
+
+static int samsung_s6e3fa2_probe(struct mipi_dsi_device *dsi)
+{
+	struct device *dev = &dsi->dev;
+	struct samsung_s6e3fa2 *ctx;
+	int ret;
+
+	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->supplies[0].supply = "iovdd";
+	ctx->supplies[1].supply = "vddr";
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ctx->supplies),
+				      ctx->supplies);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to get regulators\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_BURST |
+			  MIPI_DSI_CLOCK_NON_CONTINUOUS;
+
+	drm_panel_init(&ctx->panel, dev, &samsung_s6e3fa2_panel_funcs,
+		       DRM_MODE_CONNECTOR_DSI);
+
+	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 int samsung_s6e3fa2_remove(struct mipi_dsi_device *dsi)
+{
+	struct samsung_s6e3fa2 *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);
+
+	return 0;
+}
+
+static const struct of_device_id samsung_s6e3fa2_of_match[] = {
+	{ .compatible = "samsung,s6e3fa2" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, samsung_s6e3fa2_of_match);
+
+static struct mipi_dsi_driver samsung_s6e3fa2_driver = {
+	.probe = samsung_s6e3fa2_probe,
+	.remove = samsung_s6e3fa2_remove,
+	.driver = {
+		.name = "panel-samsung-s6e3fa2",
+		.of_match_table = samsung_s6e3fa2_of_match,
+	},
+};
+module_mipi_dsi_driver(samsung_s6e3fa2_driver);
+
+MODULE_AUTHOR("Iskren Chernev <iskren.chernev@gmail.com>");
+MODULE_DESCRIPTION("DRM driver for samsung,s6e3fa2 panel");
+MODULE_LICENSE("GPL v2");
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: panel: Add Samsung S6E3FA2 panel
  2021-02-01 16:53 [PATCH v3 1/2] dt-bindings: panel: Add Samsung S6E3FA2 panel Iskren Chernev
  2021-02-01 16:53 ` [PATCH v3 2/2] drm/panel: Add panel for Samsung Galaxy S5 Iskren Chernev
@ 2021-02-09 20:55 ` Rob Herring
  2021-07-26  7:14 ` Linus Walleij
  2 siblings, 0 replies; 6+ messages in thread
From: Rob Herring @ 2021-02-09 20:55 UTC (permalink / raw)
  To: Iskren Chernev
  Cc: David Airlie, Sam Ravnborg, Thierry Reding, dri-devel,
	phone-devel, ~postmarketos/upstreaming, linux-kernel,
	Rob Herring, devicetree

On Mon, 01 Feb 2021 18:53:05 +0200, Iskren Chernev wrote:
> The Samsung S6E3FA2 AMOLED cmd LCD panel is used on the Samsung Galaxy
> S5 (klte).
> 
> Signed-off-by: Iskren Chernev <iskren.chernev@gmail.com>
> ---
> Add a simple generated panel driver that supports on/off and the corresponding
> binding documentation.
> 
> Changes in v3:
> - fix dt_binding_check issue with missing include
> - fix panel type (cmd) in kconfig description
> 
> Changes in v2:
> - move bindings to separate file, add 2 regulators
> - add standalone panel driver
> 
> v1: https://lkml.org/lkml/2020/12/30/293
> v2: https://lkml.org/lkml/2021/2/1/313
> 
>  .../display/panel/samsung,s6e3fa2.yaml        | 64 +++++++++++++++++++
>  1 file changed, 64 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/samsung,s6e3fa2.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: panel: Add Samsung S6E3FA2 panel
  2021-02-01 16:53 [PATCH v3 1/2] dt-bindings: panel: Add Samsung S6E3FA2 panel Iskren Chernev
  2021-02-01 16:53 ` [PATCH v3 2/2] drm/panel: Add panel for Samsung Galaxy S5 Iskren Chernev
  2021-02-09 20:55 ` [PATCH v3 1/2] dt-bindings: panel: Add Samsung S6E3FA2 panel Rob Herring
@ 2021-07-26  7:14 ` Linus Walleij
  2 siblings, 0 replies; 6+ messages in thread
From: Linus Walleij @ 2021-07-26  7:14 UTC (permalink / raw)
  To: Iskren Chernev
  Cc: Rob Herring, Sam Ravnborg, Thierry Reding, Daniel Vetter,
	David Airlie,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:DRM PANEL DRIVERS, linux-kernel, phone-devel,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
	<devicetree@vger.kernel.org>,
	Hans de Goede <hdegoede@redhat.com>,
	Andy Shevchenko <andy.shevchenko@gmail.com>,

Hi Iskren,

thanks for your patch!

On Mon, Feb 1, 2021 at 5:56 PM Iskren Chernev <iskren.chernev@gmail.com> wrote:

> +required:
> +  - compatible
> +  - reset-gpios
> +  - iovdd-supply
> +  - vddr-supply
> +  - port

I do not think port should be required because the DSI
framework allows panels to be put as children right inside
the DSI controller node without any port linking the two.

Other than that it looks good!

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 2/2] drm/panel: Add panel for Samsung Galaxy S5
  2021-02-01 16:53 ` [PATCH v3 2/2] drm/panel: Add panel for Samsung Galaxy S5 Iskren Chernev
@ 2021-07-26  7:34   ` Linus Walleij
  2021-07-26  7:52     ` Iskren Chernev
  0 siblings, 1 reply; 6+ messages in thread
From: Linus Walleij @ 2021-07-26  7:34 UTC (permalink / raw)
  To: Iskren Chernev
  Cc: Rob Herring, Sam Ravnborg, Thierry Reding, Daniel Vetter,
	David Airlie,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:DRM PANEL DRIVERS, linux-kernel, phone-devel,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
	<devicetree@vger.kernel.org>,
	Hans de Goede <hdegoede@redhat.com>,
	Andy Shevchenko <andy.shevchenko@gmail.com>,

Hi Iskren,

thanks for your patch!

On Mon, Feb 1, 2021 at 5:56 PM Iskren Chernev <iskren.chernev@gmail.com> wrote:

> The Samsung Galaxy S5 uses the samsung s6e3fa2 AMOLED cmd LCD panel.
>
> This driver was generated with [1], with the addition of
> mipi_dsi_dcs_set_display_on at the end of the on method.
>
> [1] https://github.com/msm8916-mainline/linux-mdss-dsi-panel-driver-generator
>
> Signed-off-by: Iskren Chernev <iskren.chernev@gmail.com>

(...)

> +config DRM_PANEL_SAMSUNG_S6E3FA2
> +       tristate "Samsung S6E3FA2 DSI cmd mode panel"

Spell it out "command mode" no need to abbreviate.

> +struct samsung_s6e3fa2 {
> +       struct drm_panel panel;
> +       struct mipi_dsi_device *dsi;
> +       struct regulator_bulk_data supplies[2];
> +       struct gpio_desc *reset_gpio;
> +       bool prepared;

Why do you need to keep track of prepared?
The framework already does that and that is why it keeps
different callbacks for prepare and enable.

> +#define dsi_generic_write_seq(dsi, seq...) do {                                \
> +               static const u8 d[] = { seq };                          \
> +               int ret;                                                \
> +               ret = mipi_dsi_generic_write(dsi, d, ARRAY_SIZE(d));    \
> +               if (ret < 0)                                            \
> +                       return ret;                                     \
> +       } while (0)
> +
> +#define dsi_dcs_write_seq(dsi, seq...) do {                            \
> +               static const u8 d[] = { seq };                          \
> +               int ret;                                                \
> +               ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \
> +               if (ret < 0)                                            \
> +                       return ret;                                     \
> +       } while (0)

Maybe this is something we should add to
include/drm/drm_mipi_dsi.h?
It looks very generic and helpful.

> +static void samsung_s6e3fa2_reset(struct samsung_s6e3fa2 *ctx)
> +{
> +       gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> +       usleep_range(5000, 6000);

There is really no need to do this, the next statement asserts
reset. It looks like a copy/paste thing from a less informed
framework.

> +       gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> +       usleep_range(5000, 6000);
> +       gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> +       usleep_range(7000, 8000);

These two look good.

> +static int samsung_s6e3fa2_on(struct samsung_s6e3fa2 *ctx)
> +{
> +       struct mipi_dsi_device *dsi = ctx->dsi;
> +       struct device *dev = &dsi->dev;
> +       int ret;
> +
> +       dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> +
> +       dsi_generic_write_seq(dsi, 0xf0, 0x5a, 0x5a);
> +       dsi_generic_write_seq(dsi, 0xfc, 0x5a, 0x5a);

These are very similar to other Samsung panels so you can definately
add this to your driver:

#define S6E3FA2_LEVEL_2_KEY         0xf0

> +       dsi_generic_write_seq(dsi, 0xf0, 0xa5, 0xa5);
> +       dsi_generic_write_seq(dsi, 0xfc, 0xa5, 0xa5);

Add a comment that this locks level 2 control again.

> +static int samsung_s6e3fa2_prepare(struct drm_panel *panel)
> +{
> +       struct samsung_s6e3fa2 *ctx = to_samsung_s6e3fa2(panel);
> +       struct device *dev = &ctx->dsi->dev;
> +       int ret;
> +
> +       if (ctx->prepared)
> +               return 0;

Looks unnecessary. The framework should keep track of this.

> +       dsi->lanes = 4;
> +       dsi->format = MIPI_DSI_FMT_RGB888;
> +       dsi->mode_flags = MIPI_DSI_MODE_VIDEO_BURST |
> +                         MIPI_DSI_CLOCK_NON_CONTINUOUS;

You are not setting dsi->hs_rate and dsi->lp_rate.
The panel definitely has these max timings so when you
don't set them the DSI host will just go with some defaults
(some of them don't even allow you to configure this).

For reusability it would be great if you know roughly what these
clocks are, sometimes board files in vendor trees give a hint.

Worst case drop a comment that HS and LP rates are unknown.

I also wonder if the panel can report an MTP ID?

If you add some code like this and call from prepare(), what
happens?

#define READ_ID1                 0xda /* Read panel ID 1 */
#define READ_ID2                 0xdb /* Read panel ID 2 */
#define READ_ID3                 0xdc /* Read panel ID 3 */

static int s6e3fa2_read_id(struct samsung_s6e3fa2 *ctx)
{
        struct mipi_dsi_device *dsi = ctx->dsi;
        struct device *dev = &dsi->dev;
        u8 id1, id2, id3;
        int ret;

        ret = mipi_dsi_dcs_read(dsi, READ_ID1, &id1, 1);
        if (ret < 0) {
                dev_err(dev, "could not read MTP ID1\n");
                return ret;
        }
        ret = mipi_dsi_dcs_read(dsi, READ_ID2, &id2, 1);
        if (ret < 0) {
                dev_err(dev, "could not read MTP ID2\n");
                return ret;
        }
        ret = mipi_dsi_dcs_read(dsi, READ_ID3, &id3, 1);
        if (ret < 0) {
                dev_err(dev, "could not read MTP ID3\n");
                return ret;
        }

        dev_info(dev, "MTP ID manufacturer: %02x version: %02x driver:
%02x\n", id1, id2, id3);

        return 0;
}

(Dry coded, but you get the idea.)

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 2/2] drm/panel: Add panel for Samsung Galaxy S5
  2021-07-26  7:34   ` Linus Walleij
@ 2021-07-26  7:52     ` Iskren Chernev
  0 siblings, 0 replies; 6+ messages in thread
From: Iskren Chernev @ 2021-07-26  7:52 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Rob Herring, Sam Ravnborg, Thierry Reding, Daniel Vetter,
	David Airlie,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:DRM PANEL DRIVERS, linux-kernel, phone-devel,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Hans de Goede, Andy Shevchenko, ~postmarketos/upstreaming



On 7/26/21 10:34 AM, Linus Walleij wrote:
> Hi Iskren,
> 
> thanks for your patch!

Thanks a lot for reviewing this. Alexey wrote a new version of the patch [1]
that was recently reviewed by Sam, and it also includes support for brightness
and another panel found on the S5. My patch is pretty much untouched out of the
panel driver generator, so we can definitely improve the generated code based
on your comments.

[1] https://lists.freedesktop.org/archives/dri-devel/2021-July/316804.html

I guess Alexey will submit a new version with the two panels split, and he
might incorporate your suggestions (as his code comes from the generator as
well).

Regards,
Iskren

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-07-26  7:52 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-01 16:53 [PATCH v3 1/2] dt-bindings: panel: Add Samsung S6E3FA2 panel Iskren Chernev
2021-02-01 16:53 ` [PATCH v3 2/2] drm/panel: Add panel for Samsung Galaxy S5 Iskren Chernev
2021-07-26  7:34   ` Linus Walleij
2021-07-26  7:52     ` Iskren Chernev
2021-02-09 20:55 ` [PATCH v3 1/2] dt-bindings: panel: Add Samsung S6E3FA2 panel Rob Herring
2021-07-26  7:14 ` Linus Walleij

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).