[PATCH 0/3] drm: panel: add support lincoln LCD197 panel

[PATCH 0/3] drm: panel: add support lincoln LCD197 panel

[Jerome Brunet]

This patchset adds support for the Lincoln LCD197 1080x1920 DSI panel.

Jerome Brunet (3):
  dt-bindings: vendor-prefixes: add prefix for lincoln
  dt-bindings: panel-simple-dsi: add lincoln LCD197 panel bindings
  drm/panel: add lincoln lcd197 support

 .../display/panel/panel-simple-dsi.yaml       |   2 +
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 drivers/gpu/drm/panel/Kconfig                 |  11 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 drivers/gpu/drm/panel/panel-lincoln-lcd197.c  | 333 ++++++++++++++++++
 5 files changed, 349 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-lincoln-lcd197.c

-- 
2.43.0

[PATCH 1/3] dt-bindings: vendor-prefixes: add prefix for lincoln

[Jerome Brunet]

Lincoln Technology Solutions is a design services and LCD integration
company

Link: https://lincolntechsolutions.com/
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index fbf47f0bacf1..dc7edf8c33c1 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -820,6 +820,8 @@ patternProperties:
     description: Lichee Pi
   "^linaro,.*":
     description: Linaro Limited
+  "^lincoln,.*":
+    description: Lincoln Technology Solutions
   "^lineartechnology,.*":
     description: Linear Technology
   "^linksprite,.*":
-- 
2.43.0

[PATCH 2/3] dt-bindings: panel-simple-dsi: add lincoln LCD197 panel bindings

[Jerome Brunet]

This adds the bindings for the 1080x1920 Licoln LCD197 DSI panel to
panel-simple-dsi.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 .../devicetree/bindings/display/panel/panel-simple-dsi.yaml     | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/panel/panel-simple-dsi.yaml b/Documentation/devicetree/bindings/display/panel/panel-simple-dsi.yaml
index db5acd2807ed..49e663dd267d 100644
--- a/Documentation/devicetree/bindings/display/panel/panel-simple-dsi.yaml
+++ b/Documentation/devicetree/bindings/display/panel/panel-simple-dsi.yaml
@@ -46,6 +46,8 @@ properties:
       - lg,ld070wx3-sl01
         # LG Corporation 5" HD TFT LCD panel
       - lg,lh500wx1-sd03
+        # Lincoln LCD197 5" 1080x1920 LCD panel
+      - lincoln,lcd197
         # One Stop Displays OSD101T2587-53TS 10.1" 1920x1200 panel
       - osddisplays,osd101t2587-53ts
         # Panasonic 10" WUXGA TFT LCD panel
-- 
2.43.0

[PATCH 3/3] drm/panel: add lincoln lcd197 support

[Jerome Brunet]

Add support for the Lincoln LCD197 1080x1920 DSI panel.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/gpu/drm/panel/Kconfig                |  11 +
 drivers/gpu/drm/panel/Makefile               |   1 +
 drivers/gpu/drm/panel/panel-lincoln-lcd197.c | 333 +++++++++++++++++++
 3 files changed, 345 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-lincoln-lcd197.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 2ae0eb0638f3..a4e68981e740 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -319,6 +319,17 @@ config DRM_PANEL_LEADTEK_LTK500HD1829
 	  24 bit RGB per pixel. It provides a MIPI DSI interface to
 	  the host and has a built-in LED backlight.
 
+config DRM_PANEL_LINCOLN_LCD197
+	tristate "Lincoln lcd197 panel"
+	depends on OF
+	depends on DRM_MIPI_DSI
+	depends on BACKLIGHT_CLASS_DEVICE
+	help
+	  Say Y here if you want to enable support for lincoln lcd197
+	  TFT-LCD modules. The panel has a 1080x1920 resolution and uses
+	  24 bit RGB per pixel. It provides a MIPI DSI interface to
+	  the host.
+
 config DRM_PANEL_LG_LB035Q02
 	tristate "LG LB035Q024573 RGB panel"
 	depends on GPIOLIB && OF && SPI
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index f0203f6e02f4..06141ec2e065 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_DRM_PANEL_KHADAS_TS050) += panel-khadas-ts050.o
 obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD097D04) += panel-kingdisplay-kd097d04.o
 obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK050H3146W) += panel-leadtek-ltk050h3146w.o
 obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK500HD1829) += panel-leadtek-ltk500hd1829.o
+obj-$(CONFIG_DRM_PANEL_LINCOLN_LCD197) += panel-lincoln-lcd197.o
 obj-$(CONFIG_DRM_PANEL_LG_LB035Q02) += panel-lg-lb035q02.o
 obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
 obj-$(CONFIG_DRM_PANEL_LG_SW43408) += panel-lg-sw43408.o
diff --git a/drivers/gpu/drm/panel/panel-lincoln-lcd197.c b/drivers/gpu/drm/panel/panel-lincoln-lcd197.c
new file mode 100644
index 000000000000..977790797d70
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-lincoln-lcd197.c
@@ -0,0 +1,333 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2023 BayLibre, SAS
+ * Author: Jerome Brunet <jbrunet@baylibre.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_crtc.h>
+#include <drm/drm_device.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_panel.h>
+
+struct lincoln_lcd197_panel {
+	struct drm_panel panel;
+	struct mipi_dsi_device *dsi;
+	struct regulator *supply;
+	struct gpio_desc *enable_gpio;
+	struct gpio_desc *reset_gpio;
+
+	bool prepared;
+	bool enabled;
+};
+
+static inline
+struct lincoln_lcd197_panel *to_lincoln_lcd197_panel(struct drm_panel *panel)
+{
+	return container_of(panel, struct lincoln_lcd197_panel, panel);
+}
+
+static int lincoln_lcd197_panel_prepare(struct drm_panel *panel)
+{
+	struct lincoln_lcd197_panel *lcd = to_lincoln_lcd197_panel(panel);
+	int err;
+	u8 display_id[3];
+
+	if (lcd->prepared)
+		return 0;
+
+	gpiod_set_value_cansleep(lcd->enable_gpio, 0);
+	err = regulator_enable(lcd->supply);
+	if (err < 0)
+		return err;
+
+	gpiod_set_value_cansleep(lcd->enable_gpio, 1);
+	usleep_range(1000, 2000);
+	gpiod_set_value_cansleep(lcd->reset_gpio, 1);
+	usleep_range(5000, 6000);
+	gpiod_set_value_cansleep(lcd->reset_gpio, 0);
+	msleep(50);
+
+	mipi_dsi_dcs_write_seq(lcd->dsi, 0xB9, 0xFF, 0x83, 0x99);
+	mipi_dsi_dcs_write_seq(lcd->dsi, 0xD2, 0x55);
+	mipi_dsi_dcs_write_seq(lcd->dsi, 0xB1, 0x02, 0x04, 0x70, 0x90, 0x01,
+			       0x32, 0x33, 0x11, 0x11, 0x4D, 0x57, 0x56, 0x73,
+			       0x02, 0x02);
+	mipi_dsi_dcs_write_seq(lcd->dsi, 0xB2, 0x00, 0x80, 0x80, 0xAE, 0x0A,
+			       0x0E, 0x75, 0x11, 0x00, 0x00, 0x00);
+	mipi_dsi_dcs_write_seq(lcd->dsi, 0xB4, 0x00, 0xFF, 0x04, 0xA4, 0x02,
+			       0xA0, 0x00, 0x00, 0x10, 0x00, 0x00, 0x02, 0x00,
+			       0x24, 0x02, 0x04, 0x0A, 0x21, 0x03, 0x00, 0x00,
+			       0x08, 0xA6, 0x88, 0x04, 0xA4, 0x02, 0xA0, 0x00,
+			       0x00, 0x10, 0x00, 0x00, 0x02, 0x00, 0x24, 0x02,
+			       0x04, 0x0A, 0x00, 0x00, 0x08, 0xA6, 0x00, 0x08,
+			       0x11);
+	mipi_dsi_dcs_write_seq(lcd->dsi, 0xD3, 0x00, 0x00, 0x00, 0X00, 0x00,
+			       0x00, 0x18, 0x18, 0x32, 0x10, 0x09, 0x00, 0x09,
+			       0x32, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+			       0x00, 0x00, 0x11, 0x00, 0x02, 0x02, 0x03, 0x00,
+			       0x00, 0x00, 0x0A, 0x40);
+	mipi_dsi_dcs_write_seq(lcd->dsi, 0xD5, 0x18, 0x18, 0x18, 0x18, 0x21,
+			       0x20, 0x18, 0x18, 0x19, 0x19, 0x19, 0x19, 0x18,
+			       0x18, 0x18, 0x18, 0x03, 0x02, 0x01, 0x00, 0x2F,
+			       0x2F, 0x30, 0x30, 0x31, 0x31, 0x18, 0x18, 0x18,
+			       0x18, 0x18, 0x18);
+	mipi_dsi_dcs_write_seq(lcd->dsi, 0xD6, 0x18, 0x18, 0x18, 0x18, 0x20,
+			       0x21, 0x19, 0x19, 0x18, 0x18, 0x19, 0x19, 0x18,
+			       0x18, 0x18, 0x18, 0x00, 0x01, 0x02, 0x03, 0x2F,
+			       0x2F, 0x30, 0x30, 0x31, 0x31, 0x18, 0x18, 0x18,
+			       0x18, 0x18, 0x18);
+	mipi_dsi_dcs_write_seq(lcd->dsi, 0xBD, 0x01);
+	mipi_dsi_dcs_write_seq(lcd->dsi, 0xD8, 0x0A, 0xBE, 0xFA, 0xA0, 0x0A,
+			       0xBE, 0xFA, 0xA0);
+	mipi_dsi_dcs_write_seq(lcd->dsi, 0xD8, 0x0F, 0xFF, 0xFF, 0xE0, 0x0F,
+			       0xFF, 0xFF, 0xE0);
+	mipi_dsi_dcs_write_seq(lcd->dsi, 0xBD, 0x02);
+	mipi_dsi_dcs_write_seq(lcd->dsi, 0xD8, 0x0F, 0xFF, 0xFF, 0xE0, 0x0F,
+			       0xFF, 0xFF, 0xE0);
+	mipi_dsi_dcs_write_seq(lcd->dsi, 0xE0, 0x01, 0x11, 0x1C, 0x17, 0x39,
+			       0x43, 0x54, 0x51, 0x5A, 0x64, 0x6C, 0x74, 0x7A,
+			       0x83, 0x8D, 0x92, 0x99, 0xA4, 0xA9, 0xB4, 0xAA,
+			       0xBA, 0xBE, 0x63, 0x5E, 0x69, 0x73, 0x01, 0x11,
+			       0x1C, 0x17, 0x39, 0x43, 0x54, 0x51, 0x5A, 0x64,
+			       0x6C, 0x74, 0x7A, 0x83, 0x8D, 0x92, 0x99, 0xA4,
+			       0xA7, 0xB2, 0xA9, 0xBA, 0xBE, 0x63, 0x5E, 0x69,
+			       0x73);
+	usleep_range(200, 300);
+	mipi_dsi_dcs_write_seq(lcd->dsi, 0xB6, 0x92, 0x92);
+	mipi_dsi_dcs_write_seq(lcd->dsi, 0xCC, 0x00);
+	mipi_dsi_dcs_write_seq(lcd->dsi, 0xBF, 0x40, 0x41, 0x50, 0x49);
+	mipi_dsi_dcs_write_seq(lcd->dsi, 0xC6, 0xFF, 0xF9);
+	mipi_dsi_dcs_write_seq(lcd->dsi, 0xC0, 0x25, 0x5A);
+	mipi_dsi_dcs_write_seq(lcd->dsi, MIPI_DCS_SET_ADDRESS_MODE, 0x02);
+
+	err = mipi_dsi_dcs_exit_sleep_mode(lcd->dsi);
+	if (err < 0) {
+		dev_err(panel->dev, "failed to exit sleep mode: %d\n", err);
+		goto poweroff;
+	}
+	msleep(120);
+
+	err = mipi_dsi_dcs_read(lcd->dsi, MIPI_DCS_GET_DISPLAY_ID, display_id, 3);
+	if (err < 0) {
+		dev_err(panel->dev, "Failed to read display id: %d\n", err);
+	} else {
+		dev_dbg(panel->dev, "Display id: 0x%02x-0x%02x-0x%02x\n",
+			display_id[0], display_id[1], display_id[2]);
+	}
+
+	lcd->prepared = true;
+
+	return 0;
+
+poweroff:
+	gpiod_set_value_cansleep(lcd->enable_gpio, 0);
+	gpiod_set_value_cansleep(lcd->reset_gpio, 1);
+	regulator_disable(lcd->supply);
+
+	return err;
+}
+
+static int lincoln_lcd197_panel_unprepare(struct drm_panel *panel)
+{
+	struct lincoln_lcd197_panel *lcd = to_lincoln_lcd197_panel(panel);
+	int err;
+
+	if (!lcd->prepared)
+		return 0;
+
+	lcd->prepared = false;
+
+	err = mipi_dsi_dcs_enter_sleep_mode(lcd->dsi);
+	if (err < 0)
+		dev_err(panel->dev, "failed to enter sleep mode: %d\n", err);
+
+	usleep_range(5000, 6000);
+	gpiod_set_value_cansleep(lcd->enable_gpio, 0);
+	gpiod_set_value_cansleep(lcd->reset_gpio, 1);
+	regulator_disable(lcd->supply);
+
+	return err;
+}
+
+static int lincoln_lcd197_panel_enable(struct drm_panel *panel)
+{
+	struct lincoln_lcd197_panel *lcd = to_lincoln_lcd197_panel(panel);
+	int err;
+
+	if (lcd->enabled)
+		return 0;
+
+	err = mipi_dsi_dcs_set_display_on(lcd->dsi);
+	if (err < 0) {
+		dev_err(panel->dev, "failed to set display on: %d\n", err);
+		return err;
+	}
+
+	msleep(20);
+	lcd->enabled = true;
+
+	return 0;
+}
+
+static int lincoln_lcd197_panel_disable(struct drm_panel *panel)
+{
+	struct lincoln_lcd197_panel *lcd = to_lincoln_lcd197_panel(panel);
+	int err;
+
+	if (!lcd->enabled)
+		return 0;
+
+	err = mipi_dsi_dcs_set_display_off(lcd->dsi);
+	if (err < 0)
+		dev_err(panel->dev, "failed to set display off: %d\n", err);
+
+
+	msleep(50);
+	lcd->enabled = false;
+
+	return 0;
+}
+
+static const struct drm_display_mode default_mode = {
+	.clock = 154002,
+	.hdisplay = 1080,
+	.hsync_start = 1080 + 20,
+	.hsync_end = 1080 + 20 + 6,
+	.htotal = 1080 + 204,
+	.vdisplay = 1920,
+	.vsync_start = 1920 + 4,
+	.vsync_end = 1920 + 4 + 4,
+	.vtotal = 1920 + 79,
+	.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
+};
+
+static int lincoln_lcd197_panel_get_modes(struct drm_panel *panel,
+					  struct drm_connector *connector)
+{
+	struct drm_display_mode *mode;
+
+	mode = drm_mode_duplicate(connector->dev, &default_mode);
+	if (!mode) {
+		dev_err(panel->dev, "failed to add mode %ux%u@%u\n",
+			default_mode.hdisplay, default_mode.vdisplay,
+			drm_mode_vrefresh(&default_mode));
+		return -ENOMEM;
+	}
+
+	drm_mode_set_name(mode);
+	drm_mode_probed_add(connector, mode);
+	connector->display_info.width_mm = 79;
+	connector->display_info.height_mm = 125;
+
+	return 1;
+}
+
+static const struct drm_panel_funcs lincoln_lcd197_panel_funcs = {
+	.prepare = lincoln_lcd197_panel_prepare,
+	.unprepare = lincoln_lcd197_panel_unprepare,
+	.enable = lincoln_lcd197_panel_enable,
+	.disable = lincoln_lcd197_panel_disable,
+	.get_modes = lincoln_lcd197_panel_get_modes,
+};
+
+static int lincoln_lcd197_panel_probe(struct mipi_dsi_device *dsi)
+{
+	struct lincoln_lcd197_panel *lcd;
+	struct device *dev = &dsi->dev;
+	int err;
+
+	dsi->lanes = 4;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->mode_flags = (MIPI_DSI_MODE_VIDEO |
+			   MIPI_DSI_MODE_VIDEO_BURST |
+			   MIPI_DSI_MODE_LPM);
+
+	lcd = devm_kzalloc(&dsi->dev, sizeof(*lcd), GFP_KERNEL);
+	if (!lcd)
+		return -ENOMEM;
+
+	mipi_dsi_set_drvdata(dsi, lcd);
+	lcd->dsi = dsi;
+
+	lcd->supply = devm_regulator_get(dev, "power");
+	if (IS_ERR(lcd->supply))
+		return dev_err_probe(dev, PTR_ERR(lcd->supply),
+				     "failed to get power supply");
+
+	lcd->enable_gpio = devm_gpiod_get(dev, "enable",
+					  GPIOD_OUT_HIGH);
+	if (IS_ERR(lcd->enable_gpio))
+		return dev_err_probe(dev, PTR_ERR(lcd->enable_gpio),
+				     "failed to get enable gpio");
+
+	lcd->reset_gpio = devm_gpiod_get(dev, "reset",
+					  GPIOD_OUT_HIGH);
+	if (IS_ERR(lcd->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(lcd->reset_gpio),
+				     "failed to get reset gpio");
+
+	drm_panel_init(&lcd->panel, dev,
+		       &lincoln_lcd197_panel_funcs, DRM_MODE_CONNECTOR_DSI);
+
+	err = drm_panel_of_backlight(&lcd->panel);
+	if (err)
+		return err;
+
+	drm_panel_add(&lcd->panel);
+	err = mipi_dsi_attach(dsi);
+	if (err)
+		drm_panel_remove(&lcd->panel);
+
+	return err;
+}
+
+static void lincoln_lcd197_panel_shutdown(struct mipi_dsi_device *dsi)
+{
+	struct lincoln_lcd197_panel *lcd = mipi_dsi_get_drvdata(dsi);
+
+	drm_panel_disable(&lcd->panel);
+	drm_panel_unprepare(&lcd->panel);
+}
+
+static void lincoln_lcd197_panel_remove(struct mipi_dsi_device *dsi)
+{
+	struct lincoln_lcd197_panel *lcd = mipi_dsi_get_drvdata(dsi);
+	int err;
+
+	err = mipi_dsi_detach(dsi);
+	if (err < 0)
+		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", err);
+
+	lincoln_lcd197_panel_shutdown(dsi);
+	drm_panel_remove(&lcd->panel);
+}
+
+static const struct of_device_id lincoln_lcd197_of_match[] = {
+	{ .compatible = "lincoln,lcd197", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, lincoln_lcd197_of_match);
+
+static struct mipi_dsi_driver lincoln_lcd197_panel_driver = {
+	.driver = {
+		.name = "panel-licoln-lcd197",
+		.of_match_table = lincoln_lcd197_of_match,
+	},
+	.probe = lincoln_lcd197_panel_probe,
+	.remove = lincoln_lcd197_panel_remove,
+	.shutdown = lincoln_lcd197_panel_shutdown,
+};
+module_mipi_dsi_driver(lincoln_lcd197_panel_driver);
+
+MODULE_AUTHOR("Jerome Brunet <jbrunet@baylibre.com>");
+MODULE_DESCRIPTION("Lincoln LCD197 panel driver");
+MODULE_LICENSE("GPL");
-- 
2.43.0

Re: [PATCH 1/3] dt-bindings: vendor-prefixes: add prefix for lincoln

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 305 bytes --]

On Tue, Jun 25, 2024 at 04:25:48PM +0200, Jerome Brunet wrote:
> Lincoln Technology Solutions is a design services and LCD integration
> company
> 
> Link: https://lincolntechsolutions.com/
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/3] dt-bindings: panel-simple-dsi: add lincoln LCD197 panel bindings

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 270 bytes --]

On Tue, Jun 25, 2024 at 04:25:49PM +0200, Jerome Brunet wrote:
> This adds the bindings for the 1080x1920 Licoln LCD197 DSI panel to
> panel-simple-dsi.
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 3/3] drm/panel: add lincoln lcd197 support

[Dmitry Baryshkov]

On Tue, Jun 25, 2024 at 04:25:50PM GMT, Jerome Brunet wrote:
> Add support for the Lincoln LCD197 1080x1920 DSI panel.
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  drivers/gpu/drm/panel/Kconfig                |  11 +
>  drivers/gpu/drm/panel/Makefile               |   1 +
>  drivers/gpu/drm/panel/panel-lincoln-lcd197.c | 333 +++++++++++++++++++
>  3 files changed, 345 insertions(+)
>  create mode 100644 drivers/gpu/drm/panel/panel-lincoln-lcd197.c
> 

[...]

> +
> +	mipi_dsi_dcs_write_seq(lcd->dsi, 0xB9, 0xFF, 0x83, 0x99);

- Please use lowercase hex instead
- Please consider switching to _multi() functions.


> +	usleep_range(200, 300);

This will require new helper msm_dsi_usleep_range(ctx, 200, 300);

> +	mipi_dsi_dcs_write_seq(lcd->dsi, 0xB6, 0x92, 0x92);
> +	mipi_dsi_dcs_write_seq(lcd->dsi, 0xCC, 0x00);
> +	mipi_dsi_dcs_write_seq(lcd->dsi, 0xBF, 0x40, 0x41, 0x50, 0x49);
> +	mipi_dsi_dcs_write_seq(lcd->dsi, 0xC6, 0xFF, 0xF9);
> +	mipi_dsi_dcs_write_seq(lcd->dsi, 0xC0, 0x25, 0x5A);
> +	mipi_dsi_dcs_write_seq(lcd->dsi, MIPI_DCS_SET_ADDRESS_MODE, 0x02);
> +
> +	err = mipi_dsi_dcs_exit_sleep_mode(lcd->dsi);
> +	if (err < 0) {
> +		dev_err(panel->dev, "failed to exit sleep mode: %d\n", err);
> +		goto poweroff;
> +	}
> +	msleep(120);
> +
> +	err = mipi_dsi_dcs_read(lcd->dsi, MIPI_DCS_GET_DISPLAY_ID, display_id, 3);

This probably needs new _multi helper too.

> +	if (err < 0) {
> +		dev_err(panel->dev, "Failed to read display id: %d\n", err);
> +	} else {
> +		dev_dbg(panel->dev, "Display id: 0x%02x-0x%02x-0x%02x\n",
> +			display_id[0], display_id[1], display_id[2]);
> +	}
> +
> +	lcd->prepared = true;

Should not be required anymore.

> +
> +	return 0;
> +
> +poweroff:
> +	gpiod_set_value_cansleep(lcd->enable_gpio, 0);
> +	gpiod_set_value_cansleep(lcd->reset_gpio, 1);
> +	regulator_disable(lcd->supply);
> +
> +	return err;
> +}
> +

> +
> +static const struct drm_display_mode default_mode = {
> +	.clock = 154002,
> +	.hdisplay = 1080,
> +	.hsync_start = 1080 + 20,
> +	.hsync_end = 1080 + 20 + 6,
> +	.htotal = 1080 + 204,
> +	.vdisplay = 1920,
> +	.vsync_start = 1920 + 4,
> +	.vsync_end = 1920 + 4 + 4,
> +	.vtotal = 1920 + 79,
> +	.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
> +};
> +
> +static int lincoln_lcd197_panel_get_modes(struct drm_panel *panel,
> +					  struct drm_connector *connector)
> +{
> +	struct drm_display_mode *mode;
> +
> +	mode = drm_mode_duplicate(connector->dev, &default_mode);
> +	if (!mode) {
> +		dev_err(panel->dev, "failed to add mode %ux%u@%u\n",
> +			default_mode.hdisplay, default_mode.vdisplay,
> +			drm_mode_vrefresh(&default_mode));
> +		return -ENOMEM;
> +	}
> +
> +	drm_mode_set_name(mode);
> +	drm_mode_probed_add(connector, mode);
> +	connector->display_info.width_mm = 79;
> +	connector->display_info.height_mm = 125;

drm_connector_helper_get_modes_fixed()

> +
> +	return 1;
> +}
> +


> +
> +static void lincoln_lcd197_panel_shutdown(struct mipi_dsi_device *dsi)
> +{
> +	struct lincoln_lcd197_panel *lcd = mipi_dsi_get_drvdata(dsi);
> +
> +	drm_panel_disable(&lcd->panel);
> +	drm_panel_unprepare(&lcd->panel);
> +}

I think the agreement was that there should be no need for the panel's
shutdown, the DRM driver should shutdown the panel.


-- 
With best wishes
Dmitry

Re: [PATCH 3/3] drm/panel: add lincoln lcd197 support

[Jerome Brunet]

On Wed 26 Jun 2024 at 07:41, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> On Tue, Jun 25, 2024 at 04:25:50PM GMT, Jerome Brunet wrote:
>> Add support for the Lincoln LCD197 1080x1920 DSI panel.
>> 
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>>  drivers/gpu/drm/panel/Kconfig                |  11 +
>>  drivers/gpu/drm/panel/Makefile               |   1 +
>>  drivers/gpu/drm/panel/panel-lincoln-lcd197.c | 333 +++++++++++++++++++
>>  3 files changed, 345 insertions(+)
>>  create mode 100644 drivers/gpu/drm/panel/panel-lincoln-lcd197.c
>> 
>
> [...]
>
>> +
>> +	mipi_dsi_dcs_write_seq(lcd->dsi, 0xB9, 0xFF, 0x83, 0x99);
>
> - Please use lowercase hex instead
> - Please consider switching to _multi() functions.

Could you be a bit more specific about these '_multi' function ?
I've looked at 'drm_mipi_dsi.h' and can't really make what you mean.

Maybe I'm not looking in the right place.

>
>
>> +	usleep_range(200, 300);
>
> This will require new helper msm_dsi_usleep_range(ctx, 200, 300);

I don't really understand why I would need something else to just sleep
? Could you add some context please ?

Isn't 'msm_' usually something Qcom specific ?

>
>> +	mipi_dsi_dcs_write_seq(lcd->dsi, 0xB6, 0x92, 0x92);
>> +	mipi_dsi_dcs_write_seq(lcd->dsi, 0xCC, 0x00);
>> +	mipi_dsi_dcs_write_seq(lcd->dsi, 0xBF, 0x40, 0x41, 0x50, 0x49);
>> +	mipi_dsi_dcs_write_seq(lcd->dsi, 0xC6, 0xFF, 0xF9);
>> +	mipi_dsi_dcs_write_seq(lcd->dsi, 0xC0, 0x25, 0x5A);
>> +	mipi_dsi_dcs_write_seq(lcd->dsi, MIPI_DCS_SET_ADDRESS_MODE, 0x02);
>> +
>> +	err = mipi_dsi_dcs_exit_sleep_mode(lcd->dsi);
>> +	if (err < 0) {
>> +		dev_err(panel->dev, "failed to exit sleep mode: %d\n", err);
>> +		goto poweroff;
>> +	}
>> +	msleep(120);
>> +
>> +	err = mipi_dsi_dcs_read(lcd->dsi, MIPI_DCS_GET_DISPLAY_ID, display_id, 3);
>
> This probably needs new _multi helper too.
>
>> +	if (err < 0) {
>> +		dev_err(panel->dev, "Failed to read display id: %d\n", err);
>> +	} else {
>> +		dev_dbg(panel->dev, "Display id: 0x%02x-0x%02x-0x%02x\n",
>> +			display_id[0], display_id[1], display_id[2]);
>> +	}
>> +
>> +	lcd->prepared = true;
>
> Should not be required anymore.

The whole driver is heavily inspired by what is already in
drivers/gpu/drm/panel/ and a lot are doing something similar.

Maybe there has been a change since then and the existing have been
reworked yet. Would you mind pointing me that change if that is
the case ?

>
>> +
>> +	return 0;
>> +
>> +poweroff:
>> +	gpiod_set_value_cansleep(lcd->enable_gpio, 0);
>> +	gpiod_set_value_cansleep(lcd->reset_gpio, 1);
>> +	regulator_disable(lcd->supply);
>> +
>> +	return err;
>> +}
>> +
>
>> +
>> +static const struct drm_display_mode default_mode = {
>> +	.clock = 154002,
>> +	.hdisplay = 1080,
>> +	.hsync_start = 1080 + 20,
>> +	.hsync_end = 1080 + 20 + 6,
>> +	.htotal = 1080 + 204,
>> +	.vdisplay = 1920,
>> +	.vsync_start = 1920 + 4,
>> +	.vsync_end = 1920 + 4 + 4,
>> +	.vtotal = 1920 + 79,
>> +	.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
>> +};
>> +
>> +static int lincoln_lcd197_panel_get_modes(struct drm_panel *panel,
>> +					  struct drm_connector *connector)
>> +{
>> +	struct drm_display_mode *mode;
>> +
>> +	mode = drm_mode_duplicate(connector->dev, &default_mode);
>> +	if (!mode) {
>> +		dev_err(panel->dev, "failed to add mode %ux%u@%u\n",
>> +			default_mode.hdisplay, default_mode.vdisplay,
>> +			drm_mode_vrefresh(&default_mode));
>> +		return -ENOMEM;
>> +	}
>> +
>> +	drm_mode_set_name(mode);
>> +	drm_mode_probed_add(connector, mode);
>> +	connector->display_info.width_mm = 79;
>> +	connector->display_info.height_mm = 125;
>
> drm_connector_helper_get_modes_fixed()

Thanks for the hint

>
>> +
>> +	return 1;
>> +}
>> +
>
>
>> +
>> +static void lincoln_lcd197_panel_shutdown(struct mipi_dsi_device *dsi)
>> +{
>> +	struct lincoln_lcd197_panel *lcd = mipi_dsi_get_drvdata(dsi);
>> +
>> +	drm_panel_disable(&lcd->panel);
>> +	drm_panel_unprepare(&lcd->panel);
>> +}
>
> I think the agreement was that there should be no need for the panel's
> shutdown, the DRM driver should shutdown the panel.

I'm happy to drop that if there is such agreement. Again, most panel
drivers do implement that callback so I just did the same.

Could you point me to this 'agreement' please, so I can get a better
understanding of it ? 

-- 
Jerome

Re: [PATCH 3/3] drm/panel: add lincoln lcd197 support

[Neil Armstrong]

On 26/06/2024 11:02, Jerome Brunet wrote:
> On Wed 26 Jun 2024 at 07:41, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> 
>> On Tue, Jun 25, 2024 at 04:25:50PM GMT, Jerome Brunet wrote:
>>> Add support for the Lincoln LCD197 1080x1920 DSI panel.
>>>
>>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>>> ---
>>>   drivers/gpu/drm/panel/Kconfig                |  11 +
>>>   drivers/gpu/drm/panel/Makefile               |   1 +
>>>   drivers/gpu/drm/panel/panel-lincoln-lcd197.c | 333 +++++++++++++++++++
>>>   3 files changed, 345 insertions(+)
>>>   create mode 100644 drivers/gpu/drm/panel/panel-lincoln-lcd197.c
>>>
>>
>> [...]
>>
>>> +
>>> +	mipi_dsi_dcs_write_seq(lcd->dsi, 0xB9, 0xFF, 0x83, 0x99);
>>
>> - Please use lowercase hex instead
>> - Please consider switching to _multi() functions.
> 
> Could you be a bit more specific about these '_multi' function ?
> I've looked at 'drm_mipi_dsi.h' and can't really make what you mean.
> 
> Maybe I'm not looking in the right place.
> 
>>
>>
>>> +	usleep_range(200, 300);
>>
>> This will require new helper msm_dsi_usleep_range(ctx, 200, 300);
> 
> I don't really understand why I would need something else to just sleep
> ? Could you add some context please ?
> 
> Isn't 'msm_' usually something Qcom specific ?
> 
>>
>>> +	mipi_dsi_dcs_write_seq(lcd->dsi, 0xB6, 0x92, 0x92);
>>> +	mipi_dsi_dcs_write_seq(lcd->dsi, 0xCC, 0x00);
>>> +	mipi_dsi_dcs_write_seq(lcd->dsi, 0xBF, 0x40, 0x41, 0x50, 0x49);
>>> +	mipi_dsi_dcs_write_seq(lcd->dsi, 0xC6, 0xFF, 0xF9);
>>> +	mipi_dsi_dcs_write_seq(lcd->dsi, 0xC0, 0x25, 0x5A);
>>> +	mipi_dsi_dcs_write_seq(lcd->dsi, MIPI_DCS_SET_ADDRESS_MODE, 0x02);
>>> +
>>> +	err = mipi_dsi_dcs_exit_sleep_mode(lcd->dsi);
>>> +	if (err < 0) {
>>> +		dev_err(panel->dev, "failed to exit sleep mode: %d\n", err);
>>> +		goto poweroff;
>>> +	}
>>> +	msleep(120);
>>> +
>>> +	err = mipi_dsi_dcs_read(lcd->dsi, MIPI_DCS_GET_DISPLAY_ID, display_id, 3);
>>
>> This probably needs new _multi helper too.
>>
>>> +	if (err < 0) {
>>> +		dev_err(panel->dev, "Failed to read display id: %d\n", err);
>>> +	} else {
>>> +		dev_dbg(panel->dev, "Display id: 0x%02x-0x%02x-0x%02x\n",
>>> +			display_id[0], display_id[1], display_id[2]);
>>> +	}
>>> +
>>> +	lcd->prepared = true;
>>
>> Should not be required anymore.
> 
> The whole driver is heavily inspired by what is already in
> drivers/gpu/drm/panel/ and a lot are doing something similar.
> 
> Maybe there has been a change since then and the existing have been
> reworked yet. Would you mind pointing me that change if that is
> the case ?
> 
>>
>>> +
>>> +	return 0;
>>> +
>>> +poweroff:
>>> +	gpiod_set_value_cansleep(lcd->enable_gpio, 0);
>>> +	gpiod_set_value_cansleep(lcd->reset_gpio, 1);
>>> +	regulator_disable(lcd->supply);
>>> +
>>> +	return err;
>>> +}
>>> +
>>
>>> +
>>> +static const struct drm_display_mode default_mode = {
>>> +	.clock = 154002,
>>> +	.hdisplay = 1080,
>>> +	.hsync_start = 1080 + 20,
>>> +	.hsync_end = 1080 + 20 + 6,
>>> +	.htotal = 1080 + 204,
>>> +	.vdisplay = 1920,
>>> +	.vsync_start = 1920 + 4,
>>> +	.vsync_end = 1920 + 4 + 4,
>>> +	.vtotal = 1920 + 79,
>>> +	.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
>>> +};
>>> +
>>> +static int lincoln_lcd197_panel_get_modes(struct drm_panel *panel,
>>> +					  struct drm_connector *connector)
>>> +{
>>> +	struct drm_display_mode *mode;
>>> +
>>> +	mode = drm_mode_duplicate(connector->dev, &default_mode);
>>> +	if (!mode) {
>>> +		dev_err(panel->dev, "failed to add mode %ux%u@%u\n",
>>> +			default_mode.hdisplay, default_mode.vdisplay,
>>> +			drm_mode_vrefresh(&default_mode));
>>> +		return -ENOMEM;
>>> +	}
>>> +
>>> +	drm_mode_set_name(mode);
>>> +	drm_mode_probed_add(connector, mode);
>>> +	connector->display_info.width_mm = 79;
>>> +	connector->display_info.height_mm = 125;
>>
>> drm_connector_helper_get_modes_fixed()
> 
> Thanks for the hint
> 
>>
>>> +
>>> +	return 1;
>>> +}
>>> +
>>
>>
>>> +
>>> +static void lincoln_lcd197_panel_shutdown(struct mipi_dsi_device *dsi)
>>> +{
>>> +	struct lincoln_lcd197_panel *lcd = mipi_dsi_get_drvdata(dsi);
>>> +
>>> +	drm_panel_disable(&lcd->panel);
>>> +	drm_panel_unprepare(&lcd->panel);
>>> +}
>>
>> I think the agreement was that there should be no need for the panel's
>> shutdown, the DRM driver should shutdown the panel.
> 
> I'm happy to drop that if there is such agreement. Again, most panel
> drivers do implement that callback so I just did the same.
> 
> Could you point me to this 'agreement' please, so I can get a better
> understanding of it ?
> 

please rebase on linux-next or drm-misc-next and use the new introduced macros
dmitry pointed out.

Neil

Re: [PATCH 3/3] drm/panel: add lincoln lcd197 support

[Dmitry Baryshkov]

On Wed, Jun 26, 2024 at 11:02:25AM GMT, Jerome Brunet wrote:
> On Wed 26 Jun 2024 at 07:41, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> 
> > On Tue, Jun 25, 2024 at 04:25:50PM GMT, Jerome Brunet wrote:
> >> Add support for the Lincoln LCD197 1080x1920 DSI panel.
> >> 
> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> >> ---
> >>  drivers/gpu/drm/panel/Kconfig                |  11 +
> >>  drivers/gpu/drm/panel/Makefile               |   1 +
> >>  drivers/gpu/drm/panel/panel-lincoln-lcd197.c | 333 +++++++++++++++++++
> >>  3 files changed, 345 insertions(+)
> >>  create mode 100644 drivers/gpu/drm/panel/panel-lincoln-lcd197.c
> >> 
> >
> > [...]
> >
> >> +
> >> +	mipi_dsi_dcs_write_seq(lcd->dsi, 0xB9, 0xFF, 0x83, 0x99);
> >
> > - Please use lowercase hex instead
> > - Please consider switching to _multi() functions.
> 
> Could you be a bit more specific about these '_multi' function ?
> I've looked at 'drm_mipi_dsi.h' and can't really make what you mean.
> 
> Maybe I'm not looking in the right place.

What is your baseline? Please see commits 966e397e4f60 ("drm/mipi-dsi:
Introduce mipi_dsi_*_write_seq_multi()") and f79d6d28d8fe
("drm/mipi-dsi: wrap more functions for streamline handling") (and
66055636a146 ("drm/mipi-dsi: fix handling of ctx in mipi_dsi_msleep") as
it fixes a mistake in those two).

> 
> >
> >
> >> +	usleep_range(200, 300);
> >
> > This will require new helper msm_dsi_usleep_range(ctx, 200, 300);
> 
> I don't really understand why I would need something else to just sleep
> ? Could you add some context please ?
> 
> Isn't 'msm_' usually something Qcom specific ?

Yes, mipi_dsi_usleep_range(). Mea culpa.

> >> +	mipi_dsi_dcs_write_seq(lcd->dsi, 0xB6, 0x92, 0x92);
> >> +	mipi_dsi_dcs_write_seq(lcd->dsi, 0xCC, 0x00);
> >> +	mipi_dsi_dcs_write_seq(lcd->dsi, 0xBF, 0x40, 0x41, 0x50, 0x49);
> >> +	mipi_dsi_dcs_write_seq(lcd->dsi, 0xC6, 0xFF, 0xF9);
> >> +	mipi_dsi_dcs_write_seq(lcd->dsi, 0xC0, 0x25, 0x5A);
> >> +	mipi_dsi_dcs_write_seq(lcd->dsi, MIPI_DCS_SET_ADDRESS_MODE, 0x02);
> >> +
> >> +	err = mipi_dsi_dcs_exit_sleep_mode(lcd->dsi);
> >> +	if (err < 0) {
> >> +		dev_err(panel->dev, "failed to exit sleep mode: %d\n", err);
> >> +		goto poweroff;
> >> +	}
> >> +	msleep(120);
> >> +
> >> +	err = mipi_dsi_dcs_read(lcd->dsi, MIPI_DCS_GET_DISPLAY_ID, display_id, 3);
> >
> > This probably needs new _multi helper too.
> >
> >> +	if (err < 0) {
> >> +		dev_err(panel->dev, "Failed to read display id: %d\n", err);
> >> +	} else {
> >> +		dev_dbg(panel->dev, "Display id: 0x%02x-0x%02x-0x%02x\n",
> >> +			display_id[0], display_id[1], display_id[2]);
> >> +	}
> >> +
> >> +	lcd->prepared = true;
> >
> > Should not be required anymore.
> 
> The whole driver is heavily inspired by what is already in
> drivers/gpu/drm/panel/ and a lot are doing something similar.
> 
> Maybe there has been a change since then and the existing have been
> reworked yet. Would you mind pointing me that change if that is
> the case ?

See d2aacaf07395 ("drm/panel: Check for already prepared/enabled in
drm_panel")

> 
> >
> >> +
> >> +	return 0;
> >> +
> >> +poweroff:
> >> +	gpiod_set_value_cansleep(lcd->enable_gpio, 0);
> >> +	gpiod_set_value_cansleep(lcd->reset_gpio, 1);
> >> +	regulator_disable(lcd->supply);
> >> +
> >> +	return err;
> >> +}
> >> +
> >
> >> +
> >> +static const struct drm_display_mode default_mode = {
> >> +	.clock = 154002,
> >> +	.hdisplay = 1080,
> >> +	.hsync_start = 1080 + 20,
> >> +	.hsync_end = 1080 + 20 + 6,
> >> +	.htotal = 1080 + 204,
> >> +	.vdisplay = 1920,
> >> +	.vsync_start = 1920 + 4,
> >> +	.vsync_end = 1920 + 4 + 4,
> >> +	.vtotal = 1920 + 79,
> >> +	.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
> >> +};
> >> +
> >> +static int lincoln_lcd197_panel_get_modes(struct drm_panel *panel,
> >> +					  struct drm_connector *connector)
> >> +{
> >> +	struct drm_display_mode *mode;
> >> +
> >> +	mode = drm_mode_duplicate(connector->dev, &default_mode);
> >> +	if (!mode) {
> >> +		dev_err(panel->dev, "failed to add mode %ux%u@%u\n",
> >> +			default_mode.hdisplay, default_mode.vdisplay,
> >> +			drm_mode_vrefresh(&default_mode));
> >> +		return -ENOMEM;
> >> +	}
> >> +
> >> +	drm_mode_set_name(mode);
> >> +	drm_mode_probed_add(connector, mode);
> >> +	connector->display_info.width_mm = 79;
> >> +	connector->display_info.height_mm = 125;
> >
> > drm_connector_helper_get_modes_fixed()
> 
> Thanks for the hint
> 
> >
> >> +
> >> +	return 1;
> >> +}
> >> +
> >
> >
> >> +
> >> +static void lincoln_lcd197_panel_shutdown(struct mipi_dsi_device *dsi)
> >> +{
> >> +	struct lincoln_lcd197_panel *lcd = mipi_dsi_get_drvdata(dsi);
> >> +
> >> +	drm_panel_disable(&lcd->panel);
> >> +	drm_panel_unprepare(&lcd->panel);
> >> +}
> >
> > I think the agreement was that there should be no need for the panel's
> > shutdown, the DRM driver should shutdown the panel.
> 
> I'm happy to drop that if there is such agreement. Again, most panel
> drivers do implement that callback so I just did the same.
> 
> Could you point me to this 'agreement' please, so I can get a better
> understanding of it ? 

Quoting one of commit messages:

    It's the responsibility of a correctly written DRM modeset driver to
    call drm_atomic_helper_shutdown() at shutdown time and that should be
    disabling / unpreparing the panel if needed. Panel drivers shouldn't
    be calling these functions themselves.

I could not describe it better.

-- 
With best wishes
Dmitry