[PATCH V4 0/3] drm/panel: Add Samsung AMS495QA01 Panel

[PATCH V4 0/3] drm/panel: Add Samsung AMS495QA01 Panel

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

Add the Samsung AMS495QA01 panel as found on the Anbernic RG503. This
panel uses DSI to receive video signals, but 3-wire SPI to receive
command signals.

Changes since V3:
 - Updated documentation to add spi-peripheral-props.yaml per updates
   made for similar devices. Note that I removed a "Reviewed-by" tag
   from Rob Herring since this change probably needs to be confirmed.
 - Added binding for RG503, since this device is now accepted with this
   request: https://lore.kernel.org/linux-rockchip/166274831283.21181.6861718157177507544.b4-ty@sntech.de/

Changes since V2:
 - Added 50hz mode at request of userspace devs.
 - Renamed "dupa" to panel name. Good catch Maya.
 - Added Maya's Signed-off-by.
 - Removed check for max backlight, since it is already done by
   backlight_device_set_brightness.
 - Fixed minor formatting issues on devicetree binding documentation
   and added port to provided example.

Changes since V1:
 - Removed errant reference to backlight in documentation. This is an
   OLED panel.
 - Made elvss regulator optional. In my case its hard wired and not
   controllable.
 - Added "prepared" enum to track panel status to prevent unbalanced
   regulator enable/disable.

Chris Morgan (3):
  dt-bindings: display: panel: Add Samsung AMS495QA01
  drm/panel: Add Samsung AMS495QA01 MIPI-DSI LCD panel
  arm64: dts: rockchip: add display to RG503

 .../display/panel/samsung,ams495qa01.yaml     |  57 ++
 .../dts/rockchip/rk3566-anbernic-rg503.dts    |  61 ++
 drivers/gpu/drm/panel/Kconfig                 |  10 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 .../gpu/drm/panel/panel-samsung-ams495qa01.c  | 547 ++++++++++++++++++
 5 files changed, 676 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/samsung,ams495qa01.yaml
 create mode 100644 drivers/gpu/drm/panel/panel-samsung-ams495qa01.c

-- 
2.25.1

[PATCH V4 1/3] dt-bindings: display: panel: Add Samsung AMS495QA01

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

Add documentation for the Samsung AMS495QA01 panel.

Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
---
 .../display/panel/samsung,ams495qa01.yaml     | 57 +++++++++++++++++++
 1 file changed, 57 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/samsung,ams495qa01.yaml

diff --git a/Documentation/devicetree/bindings/display/panel/samsung,ams495qa01.yaml b/Documentation/devicetree/bindings/display/panel/samsung,ams495qa01.yaml
new file mode 100644
index 000000000000..dd4b9b626fb2
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/samsung,ams495qa01.yaml
@@ -0,0 +1,57 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/samsung,ams495qa01.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Samsung AMS495QA01 4.95in 960x544 DSI/SPI panel
+
+maintainers:
+  - Chris Morgan <macromorgan@hotmail.com>
+
+allOf:
+  - $ref: panel-common.yaml#
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    const: samsung,ams495qa01
+
+  reg: true
+  reset-gpios: true
+  elvdd-supply:
+    description: regulator that supplies voltage to the panel display
+  enable-gpios: true
+  port: true
+  vdd-supply:
+    description: regulator that supplies voltage to panel logic
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        panel@0 {
+            compatible = "samsung,ams495qa01";
+            reg = <0>;
+            enable-gpios = <&gpio4 15 GPIO_ACTIVE_HIGH>;
+            reset-gpios = <&gpio4 0 GPIO_ACTIVE_LOW>;
+            vdd-supply = <&vcc3v3_lcd0_n>;
+
+            port {
+                mipi_in_panel: endpoint {
+                  remote-endpoint = <&mipi_out_panel>;
+                };
+            };
+        };
+    };
+
+...
-- 
2.25.1

[PATCH V4 2/3] drm/panel: Add Samsung AMS495QA01 MIPI-DSI LCD panel

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

Support Samsung AMS495QA01 panel as found on the Anbernic RG503. Note
This panel receives video signals via DSI, however it receives
commands via 3-wire SPI.

Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
---
 drivers/gpu/drm/panel/Kconfig                 |  10 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 .../gpu/drm/panel/panel-samsung-ams495qa01.c  | 547 ++++++++++++++++++
 3 files changed, 558 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-samsung-ams495qa01.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index a582ddd583c2..0d9a9e9fd866 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -444,6 +444,16 @@ config DRM_PANEL_RONBO_RB070D30
 	  Say Y here if you want to enable support for Ronbo Electronics
 	  RB070D30 1024x600 DSI panel.
 
+config DRM_PANEL_SAMSUNG_AMS495QA01
+	tristate "Samsung AMS495QA01 DSI panel"
+	depends on OF && SPI
+	depends on DRM_MIPI_DSI
+	select DRM_MIPI_DBI
+	help
+	  DRM panel driver for the Samsung AMS495QA01 panel. This panel
+	  receives video data via DSI but commands via 3-Wire 9-bit
+	  SPI.
+
 config DRM_PANEL_SAMSUNG_ATNA33XC20
 	tristate "Samsung ATNA33XC20 eDP panel"
 	depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 34e717382dbb..de0b57baf851 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -42,6 +42,7 @@ obj-$(CONFIG_DRM_PANEL_RASPBERRYPI_TOUCHSCREEN) += panel-raspberrypi-touchscreen
 obj-$(CONFIG_DRM_PANEL_RAYDIUM_RM67191) += panel-raydium-rm67191.o
 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_AMS495QA01) += panel-samsung-ams495qa01.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_ATNA33XC20) += panel-samsung-atna33xc20.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_DB7430) += panel-samsung-db7430.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) += panel-samsung-ld9040.o
diff --git a/drivers/gpu/drm/panel/panel-samsung-ams495qa01.c b/drivers/gpu/drm/panel/panel-samsung-ams495qa01.c
new file mode 100644
index 000000000000..b59839d026da
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-samsung-ams495qa01.c
@@ -0,0 +1,547 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Samsung ams495qa01 MIPI-DSI panel driver
+ * Copyright (C) 2022 Chris Morgan
+ */
+
+#include <drm/drm_mipi_dbi.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_panel.h>
+
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/media-bus-format.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_graph.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#include <video/mipi_display.h>
+
+struct ams495qa01_panel_info {
+	/** @display_modes: the supported display modes */
+	const struct drm_display_mode *display_modes;
+	/** @num_modes: the number of supported display modes */
+	unsigned int num_modes;
+	/** @width_mm: panel width in mm */
+	u16 width_mm;
+	/** @height_mm: panel height in mm */
+	u16 height_mm;
+	/** @bus_flags: drm bus flags for panel */
+	u32 bus_flags;
+};
+
+struct ams495qa01 {
+	/** @dev: the container device */
+	struct device *dev;
+	/** @dbi: the DBI bus abstraction handle */
+	struct mipi_dbi dbi;
+	/** @panel: the DRM panel instance for this device */
+	struct drm_panel panel;
+	/** @reset: reset GPIO line */
+	struct gpio_desc *reset;
+	/** @enable: enable GPIO line */
+	struct gpio_desc *enable;
+	/** @reg_vdd: VDD supply regulator for panel logic */
+	struct regulator *reg_vdd;
+	/** @reg_elvdd: ELVDD supply regulator for panel display */
+	struct regulator *reg_elvdd;
+	/** @dsi_dev: DSI child device (panel) */
+	struct mipi_dsi_device *dsi_dev;
+	/** @bl_dev: pseudo-backlight device for oled panel */
+	struct backlight_device *bl_dev;
+	/** @panel_info: struct containing panel timing and info */
+	const struct ams495qa01_panel_info *panel_info;
+	/** @prepared: value tracking panel prepare status */
+	bool prepared;
+};
+
+#define NUM_GAMMA_LEVELS	16
+#define GAMMA_TABLE_COUNT	23
+#define MAX_BRIGHTNESS		(NUM_GAMMA_LEVELS - 1)
+
+/* Table of gamma values provided in datasheet */
+static u8 ams495qa01_gamma[NUM_GAMMA_LEVELS][GAMMA_TABLE_COUNT] = {
+	{0x01, 0x79, 0x78, 0x8d, 0xd9, 0xdf, 0xd5, 0xcb, 0xcf, 0xc5,
+	 0xe5, 0xe0, 0xe4, 0xdc, 0xb8, 0xd4, 0xfa, 0xed, 0xe6, 0x2f,
+	 0x00, 0x2f},
+	{0x01, 0x7d, 0x7c, 0x92, 0xd7, 0xdd, 0xd2, 0xcb, 0xd0, 0xc6,
+	 0xe5, 0xe1, 0xe3, 0xda, 0xbd, 0xd3, 0xfa, 0xed, 0xe6, 0x2f,
+	 0x00, 0x2f},
+	{0x01, 0x7f, 0x7e, 0x95, 0xd7, 0xde, 0xd2, 0xcb, 0xcf, 0xc5,
+	 0xe5, 0xe3, 0xe3, 0xda, 0xbf, 0xd3, 0xfa, 0xed, 0xe6, 0x2f,
+	 0x00, 0x2f},
+	{0x01, 0x82, 0x81, 0x99, 0xd6, 0xdd, 0xd1, 0xca, 0xcf, 0xc3,
+	 0xe4, 0xe3, 0xe3, 0xda, 0xc2, 0xd3, 0xfa, 0xed, 0xe6, 0x2f,
+	 0x00, 0x2f},
+	{0x01, 0x84, 0x83, 0x9b, 0xd7, 0xde, 0xd2, 0xc8, 0xce, 0xc2,
+	 0xe4, 0xe3, 0xe2, 0xd9, 0xc3, 0xd3, 0xfa, 0xed, 0xe6, 0x2f,
+	 0x00, 0x2f},
+	{0x01, 0x87, 0x86, 0x9f, 0xd6, 0xdd, 0xd1, 0xc7, 0xce, 0xc1,
+	 0xe4, 0xe3, 0xe2, 0xd9, 0xc6, 0xd3, 0xfa, 0xed, 0xe6, 0x2f,
+	 0x00, 0x2f},
+	{0x01, 0x89, 0x89, 0xa2, 0xd5, 0xdb, 0xcf, 0xc8, 0xcf, 0xc2,
+	 0xe3, 0xe3, 0xe1, 0xd9, 0xc7, 0xd3, 0xfa, 0xed, 0xe6, 0x2f,
+	 0x00, 0x2f},
+	{0x01, 0x8b, 0x8b, 0xa5, 0xd5, 0xdb, 0xcf, 0xc7, 0xce, 0xc0,
+	 0xe3, 0xe3, 0xe1, 0xd8, 0xc7, 0xd3, 0xfa, 0xed, 0xe6, 0x2f,
+	 0x00, 0x2f},
+	{0x01, 0x8d, 0x8d, 0xa7, 0xd5, 0xdb, 0xcf, 0xc6, 0xce, 0xc0,
+	 0xe4, 0xe4, 0xe1, 0xd7, 0xc8, 0xd3, 0xfa, 0xed, 0xe6, 0x2f,
+	 0x00, 0x2f},
+	{0x01, 0x8f, 0x8f, 0xaa, 0xd4, 0xdb, 0xce, 0xc6, 0xcd, 0xbf,
+	 0xe3, 0xe3, 0xe1, 0xd7, 0xca, 0xd3, 0xfa, 0xed, 0xe6, 0x2f,
+	 0x00, 0x2f},
+	{0x01, 0x91, 0x91, 0xac, 0xd3, 0xda, 0xce, 0xc5, 0xcd, 0xbe,
+	 0xe3, 0xe3, 0xe0, 0xd7, 0xca, 0xd3, 0xfa, 0xed, 0xe6, 0x2f,
+	 0x00, 0x2f},
+	{0x01, 0x93, 0x93, 0xaf, 0xd3, 0xda, 0xcd, 0xc5, 0xcd, 0xbe,
+	 0xe2, 0xe3, 0xdf, 0xd6, 0xca, 0xd3, 0xfa, 0xed, 0xe6, 0x2f,
+	 0x00, 0x2f},
+	{0x01, 0x95, 0x95, 0xb1, 0xd2, 0xd9, 0xcc, 0xc4, 0xcd, 0xbe,
+	 0xe2, 0xe3, 0xdf, 0xd7, 0xcc, 0xd3, 0xfa, 0xed, 0xe6, 0x2f,
+	 0x00, 0x2f},
+	{0x01, 0x99, 0x99, 0xb6, 0xd1, 0xd9, 0xcc, 0xc3, 0xcb, 0xbc,
+	 0xe2, 0xe4, 0xdf, 0xd6, 0xcc, 0xd3, 0xfa, 0xed, 0xe6, 0x2f,
+	 0x00, 0x2f},
+	{0x01, 0x9c, 0x9c, 0xba, 0xd0, 0xd8, 0xcb, 0xc3, 0xcb, 0xbb,
+	 0xe2, 0xe4, 0xdf, 0xd6, 0xce, 0xd3, 0xfa, 0xed, 0xe6, 0x2f,
+	 0x00, 0x2f},
+	{0x01, 0x9f, 0x9f, 0xbe, 0xcf, 0xd7, 0xc9, 0xc2, 0xcb, 0xbb,
+	 0xe1, 0xe3, 0xde, 0xd6, 0xd0, 0xd3, 0xfa, 0xed, 0xe6, 0x2f,
+	 0x00, 0x2f},
+};
+
+/*
+ * Table of elvss values provided in datasheet and corresponds to
+ * gamma values.
+ */
+static u8 ams495qa01_elvss[NUM_GAMMA_LEVELS] = {
+	0x15, 0x15, 0x15, 0x15, 0x15, 0x15, 0x15, 0x15, 0x15, 0x15,
+	0x15, 0x15, 0x14, 0x14, 0x13, 0x12,
+};
+
+static inline struct ams495qa01 *to_ams495qa01(struct drm_panel *panel)
+{
+	return container_of(panel, struct ams495qa01, panel);
+}
+
+static int ams495qa01_update_gamma(struct mipi_dbi *dbi, u32 brightness)
+{
+	u32 tmp = brightness;
+
+	/* Set gamma values */
+	mipi_dbi_command_buf(dbi, 0xf9, ams495qa01_gamma[tmp],
+			     ARRAY_SIZE(ams495qa01_gamma[tmp]));
+	/* Set gamma change */
+	mipi_dbi_command(dbi, 0xf9, 0x00);
+	/* Undocumented command */
+	mipi_dbi_command(dbi, 0x26, 0x00);
+	/* Set ELVSS value */
+	mipi_dbi_command(dbi, 0xb2, ams495qa01_elvss[tmp]);
+
+	return 0;
+}
+
+static int ams495qa01_prepare(struct drm_panel *panel)
+{
+	struct ams495qa01 *db = to_ams495qa01(panel);
+	struct mipi_dbi *dbi = &db->dbi;
+	int ret;
+
+	if (db->prepared)
+		return 0;
+
+	/* Power up */
+	ret = regulator_enable(db->reg_vdd);
+	if (ret) {
+		dev_err(db->dev, "failed to enable vdd regulator: %d\n", ret);
+		return ret;
+	}
+	if (db->reg_elvdd) {
+		ret = regulator_enable(db->reg_elvdd);
+		if (ret) {
+			dev_err(db->dev,
+				"failed to enable elvdd regulator: %d\n", ret);
+			regulator_disable(db->reg_vdd);
+			return ret;
+		}
+	}
+
+	/* Enable */
+	if (db->enable)
+		gpiod_set_value_cansleep(db->enable, 1);
+
+	msleep(50);
+
+	/* Reset */
+	gpiod_set_value_cansleep(db->reset, 1);
+	usleep_range(1000, 5000);
+	gpiod_set_value_cansleep(db->reset, 0);
+	msleep(20);
+
+	/* Panel Init Sequence */
+
+	/* Password to start command sequence */
+	mipi_dbi_command(dbi, 0xf0, 0x5a, 0x5a);
+	mipi_dbi_command(dbi, 0xf1, 0x5a, 0x5a);
+
+	/* Undocumented commands */
+	mipi_dbi_command(dbi, 0xb0, 0x02);
+	mipi_dbi_command(dbi, 0xf3, 0x3b);
+
+	/* Analog Power condition set */
+	mipi_dbi_command(dbi, 0xf4, 0x33, 0x42, 0x00, 0x08);
+	mipi_dbi_command(dbi, 0xf5, 0x00, 0x06, 0x26, 0x35, 0x03);
+
+	/* Undocumented commands */
+	mipi_dbi_command(dbi, 0xf6, 0x02);
+	mipi_dbi_command(dbi, 0xc6, 0x0b, 0x00, 0x00, 0x3c, 0x00, 0x22,
+			 0x00, 0x00, 0x00, 0x00);
+
+	/* GTCON set */
+	mipi_dbi_command(dbi, 0xf7, 0x20);
+
+	/* TEMP_SWIRE set */
+	mipi_dbi_command(dbi, 0xb2, 0x06, 0x06, 0x06, 0x06);
+
+	/* ELVSS_CON set */
+	mipi_dbi_command(dbi, 0xb1, 0x07, 0x00, 0x10);
+
+	/* Gateless signal set */
+	mipi_dbi_command(dbi, 0xf8, 0x7f, 0x7a, 0x89, 0x67, 0x26, 0x38,
+			 0x00, 0x00, 0x09, 0x67, 0x70, 0x88, 0x7a,
+			 0x76, 0x05, 0x09, 0x23, 0x23, 0x23);
+
+	/* Undocumented commands */
+	mipi_dbi_command(dbi, 0xb5, 0xff, 0xef, 0x35, 0x42, 0x0d, 0xd7,
+			 0xff, 0x07, 0xff, 0xff, 0xfd, 0x00, 0x01,
+			 0xff, 0x05, 0x12, 0x0f, 0xff, 0xff, 0xff,
+			 0xff);
+	mipi_dbi_command(dbi, 0xb4, 0x15);
+	mipi_dbi_command(dbi, 0xb3, 0x00);
+
+	ams495qa01_update_gamma(dbi, MAX_BRIGHTNESS);
+
+	mipi_dbi_command(dbi, MIPI_DCS_EXIT_SLEEP_MODE);
+	msleep(200);
+	mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_ON);
+	usleep_range(10000, 15000);
+
+	db->prepared = true;
+
+	return 0;
+}
+
+static int ams495qa01_unprepare(struct drm_panel *panel)
+{
+	struct ams495qa01 *db = to_ams495qa01(panel);
+	struct mipi_dbi *dbi = &db->dbi;
+
+	if (!db->prepared)
+		return 0;
+
+	/* Panel Exit Sequence */
+	mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_OFF);
+	msleep(20);
+	mipi_dbi_command(dbi, MIPI_DCS_ENTER_SLEEP_MODE);
+	usleep_range(10000, 15000);
+
+	if (db->enable)
+		gpiod_set_value_cansleep(db->enable, 0);
+	if (db->reg_elvdd)
+		regulator_disable(db->reg_elvdd);
+	regulator_disable(db->reg_vdd);
+	msleep(20);
+
+	db->prepared = false;
+
+	return 0;
+}
+
+static int ams495qa01_get_modes(struct drm_panel *panel,
+				struct drm_connector *connector)
+{
+	struct ams495qa01 *db = to_ams495qa01(panel);
+	const struct ams495qa01_panel_info *panel_info = db->panel_info;
+	struct drm_display_mode *mode;
+	static const u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
+	unsigned int i;
+
+	for (i = 0; i < panel_info->num_modes; i++) {
+		mode = drm_mode_duplicate(connector->dev,
+					  &panel_info->display_modes[i]);
+		if (!mode)
+			return -ENOMEM;
+
+		drm_mode_set_name(mode);
+
+		mode->type = DRM_MODE_TYPE_DRIVER;
+		if (panel_info->num_modes == 1)
+			mode->type |= DRM_MODE_TYPE_PREFERRED;
+
+		drm_mode_probed_add(connector, mode);
+	}
+
+	connector->display_info.bpc = 8;
+	connector->display_info.width_mm = panel_info->width_mm;
+	connector->display_info.height_mm = panel_info->height_mm;
+	connector->display_info.bus_flags = panel_info->bus_flags;
+
+	drm_display_info_set_bus_formats(&connector->display_info,
+					 &bus_format, 1);
+
+	return 1;
+}
+
+static const struct drm_panel_funcs ams495qa01_drm_funcs = {
+	.unprepare = ams495qa01_unprepare,
+	.prepare = ams495qa01_prepare,
+	.get_modes = ams495qa01_get_modes,
+};
+
+static int ams495qa01_set_brightness(struct backlight_device *bd)
+{
+	struct ams495qa01 *db = bl_get_data(bd);
+	struct mipi_dbi *dbi = &db->dbi;
+	int brightness = bd->props.brightness;
+
+	ams495qa01_update_gamma(dbi, brightness);
+
+	return 0;
+}
+
+static const struct backlight_ops ams495qa01_backlight_ops = {
+	.update_status	= ams495qa01_set_brightness,
+};
+
+static int ams495qa01_backlight_register(struct ams495qa01 *db)
+{
+	struct backlight_properties props = {
+		.type		= BACKLIGHT_RAW,
+		.brightness	= MAX_BRIGHTNESS,
+		.max_brightness = MAX_BRIGHTNESS,
+	};
+	struct device *dev = db->dev;
+	int ret = 0;
+
+	db->bl_dev = devm_backlight_device_register(dev, "panel", dev, db,
+						     &ams495qa01_backlight_ops,
+						     &props);
+	if (IS_ERR(db->bl_dev)) {
+		ret = PTR_ERR(db->bl_dev);
+		dev_err(dev, "error registering backlight device (%d)\n", ret);
+	}
+
+	return ret;
+}
+
+static int ams495qa01_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct device_node *endpoint, *dsi_host_node;
+	struct mipi_dsi_host *dsi_host;
+	struct ams495qa01 *db;
+	int ret;
+	struct mipi_dsi_device_info info = {
+		.type = "ams495qa01",
+		.channel = 0,
+		.node = NULL,
+	};
+
+	db = devm_kzalloc(dev, sizeof(*db), GFP_KERNEL);
+	if (!db)
+		return -ENOMEM;
+
+	spi_set_drvdata(spi, db);
+
+	db->dev = dev;
+
+	db->panel_info = of_device_get_match_data(dev);
+	if (!db->panel_info)
+		return -EINVAL;
+
+	db->reg_vdd = devm_regulator_get(dev, "vdd");
+	if (IS_ERR(db->reg_vdd))
+		return dev_err_probe(dev, PTR_ERR(db->reg_vdd),
+				     "Failed to get vdd supply\n");
+
+	db->reg_elvdd = devm_regulator_get_optional(dev, "elvdd");
+	if (IS_ERR(db->reg_elvdd))
+		db->reg_elvdd = NULL;
+
+	db->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(db->reset)) {
+		ret = PTR_ERR(db->reset);
+		return dev_err_probe(dev, ret, "no RESET GPIO\n");
+	}
+
+	db->enable = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);
+	if (IS_ERR(db->enable)) {
+		ret = PTR_ERR(db->enable);
+		return dev_err_probe(dev, ret, "cannot get ENABLE GPIO\n");
+	}
+
+	ret = mipi_dbi_spi_init(spi, &db->dbi, NULL);
+	if (ret)
+		return dev_err_probe(dev, ret, "MIPI DBI init failed\n");
+
+	/*
+	 * Get the DSI controller that is supplying data for this display
+	 * which is controlled via SPI 3-wire.
+	 */
+	endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
+	if (!endpoint) {
+		dev_err(dev, "failed to get endpoint\n");
+		return -ENODEV;
+	}
+	dsi_host_node = of_graph_get_remote_port_parent(endpoint);
+	if (!dsi_host_node) {
+		dev_err(dev, "failed to get remote port parent\n");
+		goto put_endpoint;
+	}
+	dsi_host = of_find_mipi_dsi_host_by_node(dsi_host_node);
+	if (!dsi_host) {
+		dev_err(dev, "failed to find dsi host\n");
+		goto put_host;
+	}
+	info.node = of_graph_get_remote_port(endpoint);
+	if (!info.node) {
+		dev_err(dev, "failed to get remote port node\n");
+		ret = -ENODEV;
+		goto put_host;
+	}
+
+	db->dsi_dev = devm_mipi_dsi_device_register_full(dev, dsi_host, &info);
+	if (IS_ERR(db->dsi_dev)) {
+		dev_err(dev, "failed to register dsi device: %ld\n",
+			PTR_ERR(db->dsi_dev));
+		ret = PTR_ERR(db->dsi_dev);
+		goto put_host;
+	}
+
+	db->dsi_dev->lanes = 2;
+	db->dsi_dev->format = MIPI_DSI_FMT_RGB888;
+	db->dsi_dev->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
+			  MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_NO_EOT_PACKET;
+
+	drm_panel_init(&db->panel, dev, &ams495qa01_drm_funcs,
+		       DRM_MODE_CONNECTOR_DSI);
+
+	ret = ams495qa01_backlight_register(db);
+	if (ret < 0)
+		return ret;
+
+	drm_panel_add(&db->panel);
+
+	ret = devm_mipi_dsi_attach(dev, db->dsi_dev);
+	if (ret < 0) {
+		dev_err(dev, "mipi_dsi_attach failed: %d\n", ret);
+		drm_panel_remove(&db->panel);
+		return ret;
+	}
+
+	of_node_put(dsi_host_node);
+	of_node_put(endpoint);
+
+	return 0;
+
+put_host:
+	of_node_put(dsi_host_node);
+
+put_endpoint:
+	of_node_put(endpoint);
+	return -ENODEV;
+}
+
+static void ams495qa01_shutdown(struct spi_device *spi)
+{
+	struct ams495qa01 *db = spi_get_drvdata(spi);
+	int ret;
+
+	ret = drm_panel_unprepare(&db->panel);
+	if (ret < 0)
+		dev_err(&spi->dev, "Failed to unprepare panel: %d\n", ret);
+
+	ret = drm_panel_disable(&db->panel);
+	if (ret < 0)
+		dev_err(&spi->dev, "Failed to disable panel: %d\n", ret);
+}
+
+static void ams495qa01_remove(struct spi_device *spi)
+{
+	struct ams495qa01 *db = spi_get_drvdata(spi);
+
+	ams495qa01_shutdown(spi);
+
+	drm_panel_remove(&db->panel);
+}
+
+static const struct drm_display_mode ams495qa01_modes[] = {
+	{ /* 60hz */
+		.clock = 33500,
+		.hdisplay = 960,
+		.hsync_start = 960 + 10,
+		.hsync_end = 960 + 10 + 2,
+		.htotal = 960 + 10 + 2 + 10,
+		.vdisplay = 544,
+		.vsync_start = 544 + 10,
+		.vsync_end = 544 + 10 + 2,
+		.vtotal = 544 + 10 + 2 + 10,
+		.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
+		},
+	{ /* 50hz */
+		.clock = 27800,
+		.hdisplay = 960,
+		.hsync_start = 960 + 10,
+		.hsync_end = 960 + 10 + 2,
+		.htotal = 960 + 10 + 2 + 10,
+		.vdisplay = 544,
+		.vsync_start = 544 + 10,
+		.vsync_end = 544 + 10 + 2,
+		.vtotal = 544 + 10 + 2 + 10,
+		.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
+	},
+};
+
+static const struct ams495qa01_panel_info ams495qa01_info = {
+	.display_modes = ams495qa01_modes,
+	.num_modes = ARRAY_SIZE(ams495qa01_modes),
+	.width_mm = 117,
+	.height_mm = 74,
+	.bus_flags = DRM_BUS_FLAG_DE_LOW | DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE,
+};
+
+static const struct of_device_id ams495qa01_match[] = {
+	{ .compatible = "samsung,ams495qa01", .data = &ams495qa01_info },
+	{},
+};
+MODULE_DEVICE_TABLE(of, ams495qa01_match);
+
+static const struct spi_device_id ams495qa01_ids[] = {
+	{ "ams495qa01", 0 },
+	{},
+};
+MODULE_DEVICE_TABLE(spi, ams495qa01_ids);
+
+static struct spi_driver ams495qa01_driver = {
+	.driver		= {
+		.name	= "ams495qa01-panel",
+		.of_match_table = ams495qa01_match,
+	},
+	.id_table	= ams495qa01_ids,
+	.probe		= ams495qa01_probe,
+	.remove		= ams495qa01_remove,
+	.shutdown	= ams495qa01_shutdown,
+};
+module_spi_driver(ams495qa01_driver);
+
+MODULE_AUTHOR("Chris Morgan <macromorgan@hotmail.com>");
+MODULE_DESCRIPTION("Samsung ams495qa01 panel driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1

[PATCH V4 3/3] arm64: dts: rockchip: add display to RG503

[Chris Morgan]

From: Chris Morgan <macromorgan@hotmail.com>

Add Samsung AMS495QA01 panel to RG503.

Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
---
 .../dts/rockchip/rk3566-anbernic-rg503.dts    | 61 +++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3566-anbernic-rg503.dts b/arch/arm64/boot/dts/rockchip/rk3566-anbernic-rg503.dts
index 5dafcc86296b..23442e38a751 100644
--- a/arch/arm64/boot/dts/rockchip/rk3566-anbernic-rg503.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3566-anbernic-rg503.dts
@@ -47,6 +47,22 @@ gpio_spi: spi {
 		mosi-gpios = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>;
 		cs-gpios = <&gpio4 RK_PA7 GPIO_ACTIVE_HIGH>;
 		num-chipselects = <0>;
+
+		panel@0 {
+			compatible = "samsung,ams495qa01";
+			reg = <0>;
+			enable-gpios = <&gpio4 RK_PB7 GPIO_ACTIVE_HIGH>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&lcd_enable>, <&lcd_reset>;
+			reset-gpios = <&gpio4 RK_PA0 GPIO_ACTIVE_LOW>;
+			vdd-supply = <&vcc3v3_lcd0_n>;
+
+			port {
+				mipi_in_panel: endpoint {
+					remote-endpoint = <&mipi_out_panel>;
+				};
+			};
+		};
 	};
 
 	/* Channels reversed for both headphones and speakers. */
@@ -94,6 +110,32 @@ &cru {
 	assigned-clock-rates = <1200000000>, <200000000>, <500000000>;
 };
 
+&dsi_dphy0 {
+	status = "okay";
+};
+
+&dsi0 {
+	status = "okay";
+
+	ports {
+		dsi0_in: port@0 {
+			reg = <0>;
+
+			dsi0_in_vp1: endpoint {
+				remote-endpoint = <&vp1_out_dsi0>;
+			};
+		};
+
+		dsi0_out: port@1 {
+			reg = <1>;
+
+			mipi_out_panel: endpoint {
+				remote-endpoint = <&mipi_in_panel>;
+			};
+		};
+	};
+};
+
 &gpio_keys_control {
 	button-a {
 		gpios = <&gpio3 RK_PC1 GPIO_ACTIVE_LOW>;
@@ -146,6 +188,18 @@ spk_amp_enable_h: spk-amp-enable-h {
 		};
 	};
 
+	gpio-lcd {
+		lcd_enable: lcd-enable {
+			rockchip,pins =
+				<4 RK_PB7 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+
+		lcd_reset: lcd-reset {
+			rockchip,pins =
+				<4 RK_PA0 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+	};
+
 	gpio-spi {
 		spi_pins: spi-pins {
 			rockchip,pins =
@@ -164,3 +218,10 @@ rk817_charger: charger {
 		rockchip,sleep-filter-current-microamp = <100000>;
 	};
 };
+
+&vp1 {
+	vp1_out_dsi0: endpoint@ROCKCHIP_VOP2_EP_MIPI0 {
+		reg = <ROCKCHIP_VOP2_EP_MIPI0>;
+		remote-endpoint = <&dsi0_in_vp1>;
+	};
+};
-- 
2.25.1

Re: [PATCH V4 1/3] dt-bindings: display: panel: Add Samsung AMS495QA01

[Rob Herring]

On Tue, 29 Nov 2022 11:29:10 -0600, Chris Morgan wrote:
> From: Chris Morgan <macromorgan@hotmail.com>
> 
> Add documentation for the Samsung AMS495QA01 panel.
> 
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
> ---
>  .../display/panel/samsung,ams495qa01.yaml     | 57 +++++++++++++++++++
>  1 file changed, 57 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/samsung,ams495qa01.yaml
> 

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

Re: [PATCH V4 1/3] dt-bindings: display: panel: Add Samsung AMS495QA01

[Linus Walleij]

Hi Chris,

thanks for your patch!

On Tue, Nov 29, 2022 at 6:29 PM Chris Morgan <macroalpha82@gmail.com> wrote:

> From: Chris Morgan <macromorgan@hotmail.com>
>
> Add documentation for the Samsung AMS495QA01 panel.
>
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>

> +  reset-gpios: true

Can you add a description saying that this must always be specified
GPIO_ACTIVE_LOW.

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

Yours,
Linus Walleij

Re: [PATCH V4 2/3] drm/panel: Add Samsung AMS495QA01 MIPI-DSI LCD panel

[Linus Walleij]

Hi Chris,

thanks for your patch!

overall this looks very good. Just some nitpicks.

On Tue, Nov 29, 2022 at 6:29 PM Chris Morgan <macroalpha82@gmail.com> wrote:

> From: Chris Morgan <macromorgan@hotmail.com>
>
> Support Samsung AMS495QA01 panel as found on the Anbernic RG503. Note
> This panel receives video signals via DSI, however it receives
> commands via 3-wire SPI.
>
> Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>

> +config DRM_PANEL_SAMSUNG_AMS495QA01
> +       tristate "Samsung AMS495QA01 DSI panel"

I am always a bit careful with these "Samsung" panels. Well the
panel is surely Samsungs and usually it has that name, but very often
it is mainly a display controller, made by someone else, then a physical
panel slapped on and then the actual display controller is obscured.

Typical example:
panel-novatek-nt35560.c handing Samsung ACX424AKP.

If you know the actual display controller, then the driver should be
renamed after that, but keeping the compatible and DT bindings
as is. The reason being that tomorrow there is an LG panel
using the same display controller and then we don't get duplicate
code.

It feels like this is a Novatek controller. Just a feeling I have.
Mostly from the way it does brightness using ELVSS and
gamma. But who knows.

> +       depends on OF && SPI
> +       depends on DRM_MIPI_DSI
> +       select DRM_MIPI_DBI

Nice that you use these helpers!

> +#include <linux/media-bus-format.h>
(...)
> +#include <linux/of_graph.h>

Hm. We get to this.

> +       /** @prepared: value tracking panel prepare status */
> +       bool prepared;

Drop this and associated code. The framework keeps track of this.

> +       /* Set gamma change */
> +       mipi_dbi_command(dbi, 0xf9, 0x00);

For cases like this where you know what the command does,
please add:

#define MCS_SET_GAMMA 0xf9

(MCS = Manufacturer Command Set, cf
drivers/gpu/drm/panel/panel-samsung-s6e63m0.h)

and collect them somewhere, then you can drop the comment
because it is self-evident what is going on.

> +       /* Undocumented command */
> +       mipi_dbi_command(dbi, 0x26, 0x00);
> +       /* Set ELVSS value */
> +       mipi_dbi_command(dbi, 0xb2, ams495qa01_elvss[tmp]);

Same.

> +static int ams495qa01_prepare(struct drm_panel *panel)
> +{
> +       struct ams495qa01 *db = to_ams495qa01(panel);
> +       struct mipi_dbi *dbi = &db->dbi;
> +       int ret;
> +
> +       if (db->prepared)
> +               return 0;

As mentioned skip this.

> +       /* Power up */
> +       ret = regulator_enable(db->reg_vdd);
> +       if (ret) {
> +               dev_err(db->dev, "failed to enable vdd regulator: %d\n", ret);
> +               return ret;
> +       }
> +       if (db->reg_elvdd) {

Do you really need to if() this? I thought the regulator
framework would just ignore the calls for an optional
regulator.

> +       /* Password to start command sequence */
> +       mipi_dbi_command(dbi, 0xf0, 0x5a, 0x5a);
> +       mipi_dbi_command(dbi, 0xf1, 0x5a, 0x5a);

Use the #defines from:
drivers/gpu/drm/panel/panel-samsung-s6e63m0.h
I'm pretty sure they mean the same thing:

#define MCS_LEVEL_2_KEY         0xf0
#define MCS_MTP_KEY             0xf1

> +       /* Analog Power condition set */
> +       mipi_dbi_command(dbi, 0xf4, 0x33, 0x42, 0x00, 0x08);
> +       mipi_dbi_command(dbi, 0xf5, 0x00, 0x06, 0x26, 0x35, 0x03);

Define MCS_ commands.

> +       /* GTCON set */
> +       mipi_dbi_command(dbi, 0xf7, 0x20);
> +
> +       /* TEMP_SWIRE set */
> +       mipi_dbi_command(dbi, 0xb2, 0x06, 0x06, 0x06, 0x06);
> +
> +       /* ELVSS_CON set */
> +       mipi_dbi_command(dbi, 0xb1, 0x07, 0x00, 0x10);
> +
> +       /* Gateless signal set */
> +       mipi_dbi_command(dbi, 0xf8, 0x7f, 0x7a, 0x89, 0x67, 0x26, 0x38,
> +                        0x00, 0x00, 0x09, 0x67, 0x70, 0x88, 0x7a,
> +                        0x76, 0x05, 0x09, 0x23, 0x23, 0x23);

Dito

> +       db->prepared = true;

Drop.

> +static int ams495qa01_unprepare(struct drm_panel *panel)
> +{
> +       struct ams495qa01 *db = to_ams495qa01(panel);
> +       struct mipi_dbi *dbi = &db->dbi;
> +
> +       if (!db->prepared)
> +               return 0;

Drop.

> +static int ams495qa01_get_modes(struct drm_panel *panel,
> +                               struct drm_connector *connector)
> +{
> +       struct ams495qa01 *db = to_ams495qa01(panel);
> +       const struct ams495qa01_panel_info *panel_info = db->panel_info;
> +       struct drm_display_mode *mode;
> +       static const u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> +       unsigned int i;
> +
> +       for (i = 0; i < panel_info->num_modes; i++) {
> +               mode = drm_mode_duplicate(connector->dev,
> +                                         &panel_info->display_modes[i]);
> +               if (!mode)
> +                       return -ENOMEM;
> +
> +               drm_mode_set_name(mode);
> +
> +               mode->type = DRM_MODE_TYPE_DRIVER;
> +               if (panel_info->num_modes == 1)
> +                       mode->type |= DRM_MODE_TYPE_PREFERRED;

I think you should probably set the preferred mode even
if there are several of them? But maybe just on the first
or something. (A bit unsure here, Sam?)

> +static int ams495qa01_probe(struct spi_device *spi)
> +{
> +       struct device *dev = &spi->dev;
> +       struct device_node *endpoint, *dsi_host_node;
(...)
> +       db->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);

I would request this GPIOD_OUT_HIGH so reset is asserted on probe.

> +       db->enable = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);

And this GPIOD_OUT_LOW so panel is disabled after probe.

> +       /*
> +        * Get the DSI controller that is supplying data for this display
> +        * which is controlled via SPI 3-wire.
> +        */
> +       endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
> +       if (!endpoint) {
> +               dev_err(dev, "failed to get endpoint\n");
> +               return -ENODEV;
> +       }
> +       dsi_host_node = of_graph_get_remote_port_parent(endpoint);
> +       if (!dsi_host_node) {
> +               dev_err(dev, "failed to get remote port parent\n");
> +               goto put_endpoint;
> +       }
> +       dsi_host = of_find_mipi_dsi_host_by_node(dsi_host_node);
> +       if (!dsi_host) {
> +               dev_err(dev, "failed to find dsi host\n");
> +               goto put_host;
> +       }
> +       info.node = of_graph_get_remote_port(endpoint);
> +       if (!info.node) {
> +               dev_err(dev, "failed to get remote port node\n");
> +               ret = -ENODEV;
> +               goto put_host;
> +       }
> +
> +       db->dsi_dev = devm_mipi_dsi_device_register_full(dev, dsi_host, &info);

I don't get this part.

Why do you have to go through all this trouble when no other panel
drivers, not even the other SPI DBI panel drivers, does this?

drm_of_find_panel_or_bridge() should typically result in the core
doing all the lookup and connecting for you from the host side
should it not?

At the very least this requires a big comment about what is
going on and why and what makes this necessary.

I can only guess the following:

> +       db->dsi_dev->lanes = 2;
> +       db->dsi_dev->format = MIPI_DSI_FMT_RGB888;
> +       db->dsi_dev->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
> +                         MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_NO_EOT_PACKET;
> +
> +       drm_panel_init(&db->panel, dev, &ams495qa01_drm_funcs,
> +                      DRM_MODE_CONNECTOR_DSI);

Pixel data passes to the display using DSI but all display control
is done over SPI, and the core will not help with this.

So from the display controller POV this is a DSI display
and from the display POV this is an SPI-controlled display.
So it sits on two buses. Data path is on DSI, control path
is on SPI.

This would be kind of odd actually, normally DSI displays
do the display control over DSI as well. SPI control is usually
used on DPI displays. But I'm not surprised.

If this is necessary, isn't this something we need to teach the
core to handle instead of adding quirks like this to all drivers that
have this characteristic?

Yours,
Linus Walleij

Re: [PATCH V4 2/3] drm/panel: Add Samsung AMS495QA01 MIPI-DSI LCD panel

[Heiko Stübner]

Hi Linus,

Am Samstag, 3. Dezember 2022, 10:03:42 CET schrieb Linus Walleij:
> On Tue, Nov 29, 2022 at 6:29 PM Chris Morgan <macroalpha82@gmail.com> wrote:
> 
> > From: Chris Morgan <macromorgan@hotmail.com>
> >
> > Support Samsung AMS495QA01 panel as found on the Anbernic RG503. Note
> > This panel receives video signals via DSI, however it receives
> > commands via 3-wire SPI.
> >
> > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> > Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
> 
> > +config DRM_PANEL_SAMSUNG_AMS495QA01
> > +       tristate "Samsung AMS495QA01 DSI panel"
> 
> I am always a bit careful with these "Samsung" panels. Well the
> panel is surely Samsungs and usually it has that name, but very often
> it is mainly a display controller, made by someone else, then a physical
> panel slapped on and then the actual display controller is obscured.
>
> Typical example:
> panel-novatek-nt35560.c handing Samsung ACX424AKP.
> 
> If you know the actual display controller, then the driver should be
> renamed after that, but keeping the compatible and DT bindings
> as is. The reason being that tomorrow there is an LG panel
> using the same display controller and then we don't get duplicate
> code.
> 
> It feels like this is a Novatek controller. Just a feeling I have.
> Mostly from the way it does brightness using ELVSS and
> gamma. But who knows.

Though in past projects I've seen the same display-controller used with
different panels (and different dsi-init-sequences). In one project the
display manufacturer even EOL'ed the first panel and provided a replacement
with said same display controller (and a different init) - but the
datasheets for the display-controller were for the same chip still.

So while in my experience the actual display name from the manufacturer
identifies the display + controller combo, I don't really think you can
go the other way with the controller name identifying the display+controller
combination.

But that whole display business is arcane anyway, so this also only
stems from past projects and not factual knowledge ;-) .


Heiko

Re: [PATCH V4 2/3] drm/panel: Add Samsung AMS495QA01 MIPI-DSI LCD panel

[Chris Morgan]

On Sat, Dec 03, 2022 at 10:03:42AM +0100, Linus Walleij wrote:
> Hi Chris,
> 
> thanks for your patch!
> 
> overall this looks very good. Just some nitpicks.
> 
> On Tue, Nov 29, 2022 at 6:29 PM Chris Morgan <macroalpha82@gmail.com> wrote:
> 
> > From: Chris Morgan <macromorgan@hotmail.com>
> >
> > Support Samsung AMS495QA01 panel as found on the Anbernic RG503. Note
> > This panel receives video signals via DSI, however it receives
> > commands via 3-wire SPI.
> >
> > Signed-off-by: Chris Morgan <macromorgan@hotmail.com>
> > Signed-off-by: Maya Matuszczyk <maccraft123mc@gmail.com>
> 
> > +config DRM_PANEL_SAMSUNG_AMS495QA01
> > +       tristate "Samsung AMS495QA01 DSI panel"
> 
> I am always a bit careful with these "Samsung" panels. Well the
> panel is surely Samsungs and usually it has that name, but very often
> it is mainly a display controller, made by someone else, then a physical
> panel slapped on and then the actual display controller is obscured.
> 
> Typical example:
> panel-novatek-nt35560.c handing Samsung ACX424AKP.
> 
> If you know the actual display controller, then the driver should be
> renamed after that, but keeping the compatible and DT bindings
> as is. The reason being that tomorrow there is an LG panel
> using the same display controller and then we don't get duplicate
> code.
> 
> It feels like this is a Novatek controller. Just a feeling I have.
> Mostly from the way it does brightness using ELVSS and
> gamma. But who knows.
> 

Will do. I'll make the changes and resubmit. For what it's worth the
documentation says this one is a Samsung AMS495QA01 panel on a
Magnachip D53E6EA8966 controller IC.

> > +       depends on OF && SPI
> > +       depends on DRM_MIPI_DSI
> > +       select DRM_MIPI_DBI
> 
> Nice that you use these helpers!
> 
> > +#include <linux/media-bus-format.h>
> (...)
> > +#include <linux/of_graph.h>
> 
> Hm. We get to this.

*gulp*

> 
> > +       /** @prepared: value tracking panel prepare status */
> > +       bool prepared;
> 
> Drop this and associated code. The framework keeps track of this.
> 

Will do.

> > +       /* Set gamma change */
> > +       mipi_dbi_command(dbi, 0xf9, 0x00);
> 
> For cases like this where you know what the command does,
> please add:
> 
> #define MCS_SET_GAMMA 0xf9
> 
> (MCS = Manufacturer Command Set, cf
> drivers/gpu/drm/panel/panel-samsung-s6e63m0.h)
> 
> and collect them somewhere, then you can drop the comment
> because it is self-evident what is going on.
> 

Gotcha, will do.

> > +       /* Undocumented command */
> > +       mipi_dbi_command(dbi, 0x26, 0x00);
> > +       /* Set ELVSS value */
> > +       mipi_dbi_command(dbi, 0xb2, ams495qa01_elvss[tmp]);
> 
> Same.
> 
> > +static int ams495qa01_prepare(struct drm_panel *panel)
> > +{
> > +       struct ams495qa01 *db = to_ams495qa01(panel);
> > +       struct mipi_dbi *dbi = &db->dbi;
> > +       int ret;
> > +
> > +       if (db->prepared)
> > +               return 0;
> 
> As mentioned skip this.
> 
> > +       /* Power up */
> > +       ret = regulator_enable(db->reg_vdd);
> > +       if (ret) {
> > +               dev_err(db->dev, "failed to enable vdd regulator: %d\n", ret);
> > +               return ret;
> > +       }
> > +       if (db->reg_elvdd) {
> 
> Do you really need to if() this? I thought the regulator
> framework would just ignore the calls for an optional
> regulator.

I don't know for sure, but I'll make the change if you request it. I
think other drivers had an if in this scenario which is why I did it.

> 
> > +       /* Password to start command sequence */
> > +       mipi_dbi_command(dbi, 0xf0, 0x5a, 0x5a);
> > +       mipi_dbi_command(dbi, 0xf1, 0x5a, 0x5a);
> 
> Use the #defines from:
> drivers/gpu/drm/panel/panel-samsung-s6e63m0.h
> I'm pretty sure they mean the same thing:
> 
> #define MCS_LEVEL_2_KEY         0xf0
> #define MCS_MTP_KEY             0xf1
> 
> > +       /* Analog Power condition set */
> > +       mipi_dbi_command(dbi, 0xf4, 0x33, 0x42, 0x00, 0x08);
> > +       mipi_dbi_command(dbi, 0xf5, 0x00, 0x06, 0x26, 0x35, 0x03);
> 
> Define MCS_ commands.
> 
> > +       /* GTCON set */
> > +       mipi_dbi_command(dbi, 0xf7, 0x20);
> > +
> > +       /* TEMP_SWIRE set */
> > +       mipi_dbi_command(dbi, 0xb2, 0x06, 0x06, 0x06, 0x06);
> > +
> > +       /* ELVSS_CON set */
> > +       mipi_dbi_command(dbi, 0xb1, 0x07, 0x00, 0x10);
> > +
> > +       /* Gateless signal set */
> > +       mipi_dbi_command(dbi, 0xf8, 0x7f, 0x7a, 0x89, 0x67, 0x26, 0x38,
> > +                        0x00, 0x00, 0x09, 0x67, 0x70, 0x88, 0x7a,
> > +                        0x76, 0x05, 0x09, 0x23, 0x23, 0x23);
> 
> Dito
> 
> > +       db->prepared = true;
> 
> Drop.
> 
> > +static int ams495qa01_unprepare(struct drm_panel *panel)
> > +{
> > +       struct ams495qa01 *db = to_ams495qa01(panel);
> > +       struct mipi_dbi *dbi = &db->dbi;
> > +
> > +       if (!db->prepared)
> > +               return 0;
> 
> Drop.
> 
> > +static int ams495qa01_get_modes(struct drm_panel *panel,
> > +                               struct drm_connector *connector)
> > +{
> > +       struct ams495qa01 *db = to_ams495qa01(panel);
> > +       const struct ams495qa01_panel_info *panel_info = db->panel_info;
> > +       struct drm_display_mode *mode;
> > +       static const u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> > +       unsigned int i;
> > +
> > +       for (i = 0; i < panel_info->num_modes; i++) {
> > +               mode = drm_mode_duplicate(connector->dev,
> > +                                         &panel_info->display_modes[i]);
> > +               if (!mode)
> > +                       return -ENOMEM;
> > +
> > +               drm_mode_set_name(mode);
> > +
> > +               mode->type = DRM_MODE_TYPE_DRIVER;
> > +               if (panel_info->num_modes == 1)
> > +                       mode->type |= DRM_MODE_TYPE_PREFERRED;
> 
> I think you should probably set the preferred mode even
> if there are several of them? But maybe just on the first
> or something. (A bit unsure here, Sam?)
> 

I'll keep 60hz as the preferred. 50hz was added at the request of some
userspace folks for running PAL based emulators and stuff.

> > +static int ams495qa01_probe(struct spi_device *spi)
> > +{
> > +       struct device *dev = &spi->dev;
> > +       struct device_node *endpoint, *dsi_host_node;
> (...)
> > +       db->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> 
> I would request this GPIOD_OUT_HIGH so reset is asserted on probe.
> 

Will do.

> > +       db->enable = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);
> 
> And this GPIOD_OUT_LOW so panel is disabled after probe.
> 

Will do.

> > +       /*
> > +        * Get the DSI controller that is supplying data for this display
> > +        * which is controlled via SPI 3-wire.
> > +        */
> > +       endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
> > +       if (!endpoint) {
> > +               dev_err(dev, "failed to get endpoint\n");
> > +               return -ENODEV;
> > +       }
> > +       dsi_host_node = of_graph_get_remote_port_parent(endpoint);
> > +       if (!dsi_host_node) {
> > +               dev_err(dev, "failed to get remote port parent\n");
> > +               goto put_endpoint;
> > +       }
> > +       dsi_host = of_find_mipi_dsi_host_by_node(dsi_host_node);
> > +       if (!dsi_host) {
> > +               dev_err(dev, "failed to find dsi host\n");
> > +               goto put_host;
> > +       }
> > +       info.node = of_graph_get_remote_port(endpoint);
> > +       if (!info.node) {
> > +               dev_err(dev, "failed to get remote port node\n");
> > +               ret = -ENODEV;
> > +               goto put_host;
> > +       }
> > +
> > +       db->dsi_dev = devm_mipi_dsi_device_register_full(dev, dsi_host, &info);
> 
> I don't get this part.
> 
> Why do you have to go through all this trouble when no other panel
> drivers, not even the other SPI DBI panel drivers, does this?
> 
> drm_of_find_panel_or_bridge() should typically result in the core
> doing all the lookup and connecting for you from the host side
> should it not?
> 
> At the very least this requires a big comment about what is
> going on and why and what makes this necessary.
> 
> I can only guess the following:
> 

This is the path that "works", but I'll happily change to something
else.

> > +       db->dsi_dev->lanes = 2;
> > +       db->dsi_dev->format = MIPI_DSI_FMT_RGB888;
> > +       db->dsi_dev->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
> > +                         MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_NO_EOT_PACKET;
> > +
> > +       drm_panel_init(&db->panel, dev, &ams495qa01_drm_funcs,
> > +                      DRM_MODE_CONNECTOR_DSI);
> 
> Pixel data passes to the display using DSI but all display control
> is done over SPI, and the core will not help with this.
> 
> So from the display controller POV this is a DSI display
> and from the display POV this is an SPI-controlled display.
> So it sits on two buses. Data path is on DSI, control path
> is on SPI.
> 
> This would be kind of odd actually, normally DSI displays
> do the display control over DSI as well. SPI control is usually
> used on DPI displays. But I'm not surprised.
> 
> If this is necessary, isn't this something we need to teach the
> core to handle instead of adding quirks like this to all drivers that
> have this characteristic?
> 

You are correct, this panel is controlled via 3-wire SPI in my example.
The panel can be controlled either by 3-wire SPI or DSI commands
depending on whether or not pin 15 is driven high or low. Unfortunately
in my case it's hardwired high, so I am forced to do it via 3-wire SPI.
I have no way of testing it with pure DSI but that would simplify
things quite a bit. Pixel data is transmitted soley through DSI.

The way I have it implemented currently is to put the panel on the SPI
bus as a DBI panel; traverse through the DT bindings to find the
associated DSI controller, then attach it as a DSI device so the DSI
bus can transmit the pixel data.

I'm absolutely cool with making those functions part of the core and
not just specific to this panel, only I might need a bit of help on
that part to make sure I do it the right way. I just wasn't sure how
often that would be needed since this is the only panel I've ever seen
driven this way, especially since it seems like any sane person would
just want to do the whole control data/pixel data over DSI to keep
things simple.

> Yours,
> Linus Walleij

Thank you for your input.

Re: [PATCH V4 2/3] drm/panel: Add Samsung AMS495QA01 MIPI-DSI LCD panel

[Linus Walleij]

On Sat, Dec 3, 2022 at 11:18 AM Heiko Stübner <heiko@sntech.de> wrote:

> Though in past projects I've seen the same display-controller used with
> different panels (and different dsi-init-sequences). In one project the
> display manufacturer even EOL'ed the first panel and provided a replacement
> with said same display controller (and a different init) - but the
> datasheets for the display-controller were for the same chip still.
>
> So while in my experience the actual display name from the manufacturer
> identifies the display + controller combo, I don't really think you can
> go the other way with the controller name identifying the display+controller
> combination.

I don't mean we should do that.

What I mean is:

- Name the driver after the controller, if we know which one it is,
  such as panel-novatek-nt35510.c

- Provide identifications of the controller+panel combo in e.g.
  DT compatible strings, provide init data as arrays in the per-variant
  match data (or similar for ACPI).

Yours,
Linus Walleij

Re: [PATCH V4 2/3] drm/panel: Add Samsung AMS495QA01 MIPI-DSI LCD panel

[Linus Walleij]

On Sat, Dec 3, 2022 at 8:01 PM Chris Morgan <macroalpha82@gmail.com> wrote:

> Will do. I'll make the changes and resubmit. For what it's worth the
> documentation says this one is a Samsung AMS495QA01 panel on a
> Magnachip D53E6EA8966 controller IC.

I would name the driver panel-magnachip-d53e6ea8966.c and
KConfig PANEL_MAGNACHIP_D53E6EA8966 for now
but keep the Samsung compatible string & match.

Maybe this driver will match more magnachips in the future,
maybe not. Sometimes people get hold of datasheets and submit
proper code and #defines etc.

> > > +       if (db->reg_elvdd) {
> >
> > Do you really need to if() this? I thought the regulator
> > framework would just ignore the calls for an optional
> > regulator.
>
> I don't know for sure, but I'll make the change if you request it. I
> think other drivers had an if in this scenario which is why I did it.

Okay but I don't think that is necessary.

> > > +               mode->type = DRM_MODE_TYPE_DRIVER;
> > > +               if (panel_info->num_modes == 1)
> > > +                       mode->type |= DRM_MODE_TYPE_PREFERRED;
> >
> > I think you should probably set the preferred mode even
> > if there are several of them? But maybe just on the first
> > or something. (A bit unsure here, Sam?)
>
> I'll keep 60hz as the preferred. 50hz was added at the request of some
> userspace folks for running PAL based emulators and stuff.

OK

> This is the path that "works", but I'll happily change to something
> else.
>
> > > +       db->dsi_dev->lanes = 2;
> > > +       db->dsi_dev->format = MIPI_DSI_FMT_RGB888;
> > > +       db->dsi_dev->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
> > > +                         MIPI_DSI_MODE_LPM | MIPI_DSI_MODE_NO_EOT_PACKET;
> > > +
> > > +       drm_panel_init(&db->panel, dev, &ams495qa01_drm_funcs,
> > > +                      DRM_MODE_CONNECTOR_DSI);
> >
> > Pixel data passes to the display using DSI but all display control
> > is done over SPI, and the core will not help with this.
> >
> > So from the display controller POV this is a DSI display
> > and from the display POV this is an SPI-controlled display.
> > So it sits on two buses. Data path is on DSI, control path
> > is on SPI.
> >
> > This would be kind of odd actually, normally DSI displays
> > do the display control over DSI as well. SPI control is usually
> > used on DPI displays. But I'm not surprised.
> >
> > If this is necessary, isn't this something we need to teach the
> > core to handle instead of adding quirks like this to all drivers that
> > have this characteristic?
> >
>
> You are correct, this panel is controlled via 3-wire SPI in my example.
> The panel can be controlled either by 3-wire SPI or DSI commands
> depending on whether or not pin 15 is driven high or low. Unfortunately
> in my case it's hardwired high, so I am forced to do it via 3-wire SPI.
> I have no way of testing it with pure DSI but that would simplify
> things quite a bit. Pixel data is transmitted soley through DSI.

OK

> The way I have it implemented currently is to put the panel on the SPI
> bus as a DBI panel; traverse through the DT bindings to find the
> associated DSI controller, then attach it as a DSI device so the DSI
> bus can transmit the pixel data.
>
> I'm absolutely cool with making those functions part of the core and
> not just specific to this panel, only I might need a bit of help on
> that part to make sure I do it the right way. I just wasn't sure how
> often that would be needed since this is the only panel I've ever seen
> driven this way, especially since it seems like any sane person would
> just want to do the whole control data/pixel data over DSI to keep
> things simple.

It doesn't seem all that unique.

Can you put some helper in drivers/gpu/drm/drm_of.c with the rest
and it will (hopefully) not be linked in unless used anyway.

Yours,
Linus Walleij