[PATCH v2 0/2] Add support for the AUO A030JTN01 TFT LCD

[PATCH v2 0/2] Add support for the AUO A030JTN01 TFT LCD

[Christophe Branchereau]

Changes since v1:
 fixed the dt-bindings maintainer email adress

 dropped backlight, port, power-supply and reset-gpios as they're 
 provided by panel-common.yaml as pointed by Krzysztof Kozlowski

 changed reg: true to reg : maxItems: 1

Christophe Branchereau (2):
  drm/panel: add the orisetech ota5601a
  dt-bindings: display/panel: Add the Focaltech gpt3

 .../display/panel/focaltech,gpt3.yaml         |  56 +++
 drivers/gpu/drm/panel/Kconfig                 |   9 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 .../gpu/drm/panel/panel-orisetech-ota5601a.c  | 351 ++++++++++++++++++
 4 files changed, 417 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/focaltech,gpt3.yaml
 create mode 100644 drivers/gpu/drm/panel/panel-orisetech-ota5601a.c

-- 
2.35.1

[PATCH v2 1/2] drm/panel: add the orisetech ota5601a

[Christophe Branchereau]

Add the orisetech ota5601a ic driver

For now it only supports the focaltech gpt3 3" 640x480 ips panel
found in the ylm rg300x handheld.

Signed-off-by: Christophe Branchereau <cbranchereau@gmail.com>
---
 drivers/gpu/drm/panel/Kconfig                 |   9 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 .../gpu/drm/panel/panel-orisetech-ota5601a.c  | 351 ++++++++++++++++++
 3 files changed, 361 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-orisetech-ota5601a.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index a582ddd583c2..2f492e402cd1 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -381,6 +381,15 @@ config DRM_PANEL_OLIMEX_LCD_OLINUXINO
 	  Say Y here if you want to enable support for Olimex Ltd.
 	  LCD-OLinuXino panel.
 
+config DRM_PANEL_ORISETECH_OTA5601A
+	tristate "Orise Technology ota5601a RGB/SPI panel"
+	depends on OF && SPI
+	depends on BACKLIGHT_CLASS_DEVICE
+	select REGMAP_SPI
+	help
+	  Say Y here if you want to enable support for the panels built
+	  around the Orise Technology OTA9601A display controller.
+
 config DRM_PANEL_ORISETECH_OTM8009A
 	tristate "Orise Technology otm8009a 480x800 dsi 2dl panel"
 	depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 34e717382dbb..91d1870312af 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -35,6 +35,7 @@ obj-$(CONFIG_DRM_PANEL_NOVATEK_NT36672A) += panel-novatek-nt36672a.o
 obj-$(CONFIG_DRM_PANEL_NOVATEK_NT39016) += panel-novatek-nt39016.o
 obj-$(CONFIG_DRM_PANEL_MANTIX_MLAF057WE51) += panel-mantix-mlaf057we51.o
 obj-$(CONFIG_DRM_PANEL_OLIMEX_LCD_OLINUXINO) += panel-olimex-lcd-olinuxino.o
+obj-$(CONFIG_DRM_PANEL_ORISETECH_OTA5601A) += panel-orisetech-ota5601a.o
 obj-$(CONFIG_DRM_PANEL_ORISETECH_OTM8009A) += panel-orisetech-otm8009a.o
 obj-$(CONFIG_DRM_PANEL_OSD_OSD101T2587_53TS) += panel-osd-osd101t2587-53ts.o
 obj-$(CONFIG_DRM_PANEL_PANASONIC_VVX10F034N00) += panel-panasonic-vvx10f034n00.o
diff --git a/drivers/gpu/drm/panel/panel-orisetech-ota5601a.c b/drivers/gpu/drm/panel/panel-orisetech-ota5601a.c
new file mode 100644
index 000000000000..018fea7c6354
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-orisetech-ota5601a.c
@@ -0,0 +1,351 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Orisetech OTA5601A TFT LCD panel driver
+ *
+ * Copyright (C) 2021, Christophe Branchereau <cbranchereau@gmail.com>
+ */
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/media-bus-format.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#include <drm/drm_modes.h>
+#include <drm/drm_panel.h>
+
+struct ota5601a_panel_info {
+	const struct drm_display_mode *display_modes;
+	unsigned int num_modes;
+	u16 width_mm, height_mm;
+	u32 bus_format, bus_flags;
+};
+
+struct ota5601a {
+	struct drm_panel drm_panel;
+	struct regmap *map;
+	struct regulator *supply;
+	const struct ota5601a_panel_info *panel_info;
+
+	struct gpio_desc *reset_gpio;
+};
+
+static inline struct ota5601a *to_ota5601a(struct drm_panel *panel)
+{
+	return container_of(panel, struct ota5601a, drm_panel);
+}
+
+static const struct reg_sequence ota5601a_panel_regs[] = {
+	{ 0xfd, 0x00 },
+	{ 0x02, 0x00 },
+
+	{ 0x18, 0x00 },
+	{ 0x34, 0x20 },
+
+	{ 0x0c, 0x01 },
+	{ 0x0d, 0x48 },
+	{ 0x0e, 0x48 },
+	{ 0x0f, 0x48 },
+	{ 0x07, 0x40 },
+	{ 0x08, 0x33 },
+	{ 0x09, 0x3a },
+
+	{ 0x16, 0x01 },
+	{ 0x19, 0x8d },
+	{ 0x1a, 0x28 },
+	{ 0x1c, 0x00 },
+
+	{ 0xfd, 0xc5 },
+	{ 0x82, 0x0c },
+	{ 0xa2, 0xb4 },
+
+	{ 0xfd, 0xc4 },
+	{ 0x82, 0x45 },
+
+	{ 0xfd, 0xc1 },
+	{ 0x91, 0x02 },
+
+	{ 0xfd, 0xc0 },
+	{ 0xa1, 0x01 },
+	{ 0xa2, 0x1f },
+	{ 0xa3, 0x0b },
+	{ 0xa4, 0x38 },
+	{ 0xa5, 0x00 },
+	{ 0xa6, 0x0a },
+	{ 0xa7, 0x38 },
+	{ 0xa8, 0x00 },
+	{ 0xa9, 0x0a },
+	{ 0xaa, 0x37 },
+
+	{ 0xfd, 0xce },
+	{ 0x81, 0x18 },
+	{ 0x82, 0x43 },
+	{ 0x83, 0x43 },
+	{ 0x91, 0x06 },
+	{ 0x93, 0x38 },
+	{ 0x94, 0x02 },
+	{ 0x95, 0x06 },
+	{ 0x97, 0x38 },
+	{ 0x98, 0x02 },
+	{ 0x99, 0x06 },
+	{ 0x9b, 0x38 },
+	{ 0x9c, 0x02 },
+
+	{ 0xfd, 0x00 },
+};
+
+static const struct regmap_config ota5601a_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static int ota5601a_prepare(struct drm_panel *drm_panel)
+{
+	struct ota5601a *panel = to_ota5601a(drm_panel);
+	int err;
+
+	err = regulator_enable(panel->supply);
+	if (err) {
+		dev_err(drm_panel->dev, "Failed to enable power supply: %d\n", err);
+		return err;
+	}
+
+	/* Reset to be held low for 10us min according to the doc, 10ms before sending commands */
+	gpiod_set_value_cansleep(panel->reset_gpio, 1);
+	usleep_range(10, 30);
+	gpiod_set_value_cansleep(panel->reset_gpio, 0);
+	usleep_range(10000, 20000);
+
+	/* Init all registers. */
+	err = regmap_multi_reg_write(panel->map, ota5601a_panel_regs,
+				     ARRAY_SIZE(ota5601a_panel_regs));
+	if (err) {
+		dev_err(drm_panel->dev, "Failed to init registers: %d\n", err);
+		goto err_disable_regulator;
+	}
+
+	msleep(120);
+
+	return 0;
+
+err_disable_regulator:
+	regulator_disable(panel->supply);
+	return err;
+}
+
+static int ota5601a_unprepare(struct drm_panel *drm_panel)
+{
+	struct ota5601a *panel = to_ota5601a(drm_panel);
+
+	gpiod_set_value_cansleep(panel->reset_gpio, 1);
+
+	regulator_disable(panel->supply);
+
+	return 0;
+}
+
+static int ota5601a_enable(struct drm_panel *drm_panel)
+{
+	struct ota5601a *panel = to_ota5601a(drm_panel);
+	int err;
+
+	err = regmap_write(panel->map, 0x01, 0x01);
+
+	if (err) {
+		dev_err(drm_panel->dev, "Unable to enable panel: %d\n", err);
+		return err;
+	}
+
+	if (drm_panel->backlight) {
+		/* Wait for the picture to be ready before enabling backlight */
+		msleep(120);
+	}
+
+	return 0;
+}
+
+static int ota5601a_disable(struct drm_panel *drm_panel)
+{
+	struct ota5601a *panel = to_ota5601a(drm_panel);
+	int err;
+
+	err = regmap_write(panel->map, 0x01, 0x00);
+
+	if (err) {
+		dev_err(drm_panel->dev, "Unable to disable panel: %d\n", err);
+		return err;
+	}
+
+	return 0;
+}
+
+static int ota5601a_get_modes(struct drm_panel *drm_panel,
+			     struct drm_connector *connector)
+{
+	struct ota5601a *panel = to_ota5601a(drm_panel);
+	const struct ota5601a_panel_info *panel_info = panel->panel_info;
+	struct drm_display_mode *mode;
+	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;
+
+	drm_display_info_set_bus_formats(&connector->display_info,
+					 &panel_info->bus_format, 1);
+	connector->display_info.bus_flags = panel_info->bus_flags;
+
+	return panel_info->num_modes;
+}
+
+static const struct drm_panel_funcs ota5601a_funcs = {
+	.prepare	= ota5601a_prepare,
+	.unprepare	= ota5601a_unprepare,
+	.enable		= ota5601a_enable,
+	.disable	= ota5601a_disable,
+	.get_modes	= ota5601a_get_modes,
+};
+
+static int ota5601a_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct ota5601a *panel;
+	int err;
+
+	panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL);
+	if (!panel)
+		return -ENOMEM;
+
+	spi_set_drvdata(spi, panel);
+
+	panel->panel_info = of_device_get_match_data(dev);
+	if (!panel->panel_info)
+		return -EINVAL;
+
+	panel->supply = devm_regulator_get(dev, "power");
+	if (IS_ERR(panel->supply)) {
+		dev_err(dev, "Failed to get power supply\n");
+		return PTR_ERR(panel->supply);
+	}
+
+	panel->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(panel->reset_gpio)) {
+		dev_err(dev, "Failed to get reset GPIO\n");
+		return PTR_ERR(panel->reset_gpio);
+	}
+
+	spi->bits_per_word = 8;
+	spi->mode = SPI_MODE_3 | SPI_3WIRE;
+	err = spi_setup(spi);
+	if (err) {
+		dev_err(dev, "Failed to setup SPI\n");
+		return err;
+	}
+
+	panel->map = devm_regmap_init_spi(spi, &ota5601a_regmap_config);
+	if (IS_ERR(panel->map)) {
+		dev_err(dev, "Failed to init regmap\n");
+		return PTR_ERR(panel->map);
+	}
+
+	drm_panel_init(&panel->drm_panel, dev, &ota5601a_funcs,
+		       DRM_MODE_CONNECTOR_DPI);
+
+	err = drm_panel_of_backlight(&panel->drm_panel);
+	if (err) {
+		if (err != -EPROBE_DEFER)
+			dev_err(dev, "Failed to get backlight handle\n");
+		return err;
+	}
+
+	drm_panel_add(&panel->drm_panel);
+
+	return 0;
+}
+
+static void ota5601a_remove(struct spi_device *spi)
+{
+	struct ota5601a *panel = spi_get_drvdata(spi);
+
+	drm_panel_remove(&panel->drm_panel);
+
+	ota5601a_disable(&panel->drm_panel);
+	ota5601a_unprepare(&panel->drm_panel);
+}
+
+static const struct drm_display_mode gpt3_display_modes[] = {
+	{ /* 60 Hz */
+		.clock = 27000,
+		.hdisplay = 640,
+		.hsync_start = 640 + 220,
+		.hsync_end = 640 + 220 + 20,
+		.htotal = 640 + 220 + 20 + 20,
+		.vdisplay = 480,
+		.vsync_start = 480 + 7,
+		.vsync_end = 480 + 7 + 6,
+		.vtotal = 480 + 7 + 6 + 7,
+		.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
+	},
+
+	{ /* 50 Hz */
+		.clock = 24000,
+		.hdisplay = 640,
+		.hsync_start = 640 + 280,
+		.hsync_end = 640 + 280 + 20,
+		.htotal = 640 + 280 + 20 + 20,
+		.vdisplay = 480,
+		.vsync_start = 480 + 7,
+		.vsync_end = 480 + 7 + 6,
+		.vtotal = 480 + 7 + 6 + 7,
+		.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
+	},
+};
+
+static const struct ota5601a_panel_info gpt3_info = {
+	.display_modes = gpt3_display_modes,
+	.num_modes = ARRAY_SIZE(gpt3_display_modes),
+	.width_mm = 71,
+	.height_mm = 51,
+	.bus_format = MEDIA_BUS_FMT_RGB888_1X24,
+	.bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
+};
+
+static const struct of_device_id ota5601a_of_match[] = {
+	{ .compatible = "focaltech,gpt3", .data = &gpt3_info },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ota5601a_of_match);
+
+static struct spi_driver ota5601a_driver = {
+	.driver = {
+		.name = "ota5601a",
+		.of_match_table = ota5601a_of_match,
+	},
+	.probe = ota5601a_probe,
+	.remove = ota5601a_remove,
+};
+
+module_spi_driver(ota5601a_driver);
+
+MODULE_AUTHOR("Christophe Branchereau <cbranchereau@gmail.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.35.1

Re: [PATCH v2 1/2] drm/panel: add the orisetech ota5601a

[Linus Walleij]

Hi Christophe,

thanks for your patch!

On Wed, Dec 14, 2022 at 12:01 PM Christophe Branchereau
<cbranchereau@gmail.com> wrote:

> Add the orisetech ota5601a ic driver
>
> For now it only supports the focaltech gpt3 3" 640x480 ips panel
> found in the ylm rg300x handheld.
>
> Signed-off-by: Christophe Branchereau <cbranchereau@gmail.com>
(...)
> +config DRM_PANEL_ORISETECH_OTA5601A
> +       tristate "Orise Technology ota5601a RGB/SPI panel"
> +       depends on OF && SPI
> +       depends on BACKLIGHT_CLASS_DEVICE
> +       select REGMAP_SPI

Nice use of regmap in this driver.

> +static const struct reg_sequence ota5601a_panel_regs[] = {
> +       { 0xfd, 0x00 },
> +       { 0x02, 0x00 },
> +
> +       { 0x18, 0x00 },
> +       { 0x34, 0x20 },
> +
> +       { 0x0c, 0x01 },
> +       { 0x0d, 0x48 },
> +       { 0x0e, 0x48 },
> +       { 0x0f, 0x48 },
> +       { 0x07, 0x40 },
> +       { 0x08, 0x33 },
> +       { 0x09, 0x3a },
> +
> +       { 0x16, 0x01 },
> +       { 0x19, 0x8d },
> +       { 0x1a, 0x28 },
> +       { 0x1c, 0x00 },
> +
> +       { 0xfd, 0xc5 },
> +       { 0x82, 0x0c },
> +       { 0xa2, 0xb4 },
> +
> +       { 0xfd, 0xc4 },
> +       { 0x82, 0x45 },
> +
> +       { 0xfd, 0xc1 },
> +       { 0x91, 0x02 },
> +
> +       { 0xfd, 0xc0 },
> +       { 0xa1, 0x01 },
> +       { 0xa2, 0x1f },
> +       { 0xa3, 0x0b },
> +       { 0xa4, 0x38 },
> +       { 0xa5, 0x00 },
> +       { 0xa6, 0x0a },
> +       { 0xa7, 0x38 },
> +       { 0xa8, 0x00 },
> +       { 0xa9, 0x0a },
> +       { 0xaa, 0x37 },
> +
> +       { 0xfd, 0xce },
> +       { 0x81, 0x18 },
> +       { 0x82, 0x43 },
> +       { 0x83, 0x43 },
> +       { 0x91, 0x06 },
> +       { 0x93, 0x38 },
> +       { 0x94, 0x02 },
> +       { 0x95, 0x06 },
> +       { 0x97, 0x38 },
> +       { 0x98, 0x02 },
> +       { 0x99, 0x06 },
> +       { 0x9b, 0x38 },
> +       { 0x9c, 0x02 },
> +
> +       { 0xfd, 0x00 },
> +};

If these are registers, why is register 0xfd written 7 times with different
values?

This is rather a jam table or intializations sequence, so it should be
names something like that and a comment about undocumented
registers added probably as well.

> +static int ota5601a_enable(struct drm_panel *drm_panel)
> +{
> +       struct ota5601a *panel = to_ota5601a(drm_panel);
> +       int err;
> +
> +       err = regmap_write(panel->map, 0x01, 0x01);

Since you know what this register does, what about:

#include <linux/bits.h>

#define OTA5601A_CTL 0x01
#define OTA5601A_CTL_OFF 0x00
#define OTA5601A_CTL_ON BIT(0)

> +static int ota5601a_get_modes(struct drm_panel *drm_panel,
> +                            struct drm_connector *connector)
> +{
> +       struct ota5601a *panel = to_ota5601a(drm_panel);
> +       const struct ota5601a_panel_info *panel_info = panel->panel_info;
> +       struct drm_display_mode *mode;
> +       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)

But ... you have just added an array that contains 2 elements, you
KNOW it will not be 1.

> +                       mode->type |= DRM_MODE_TYPE_PREFERRED;

I think you should probably set this on the preferred resolution
anyways, take the one you actually use.

> +static const struct of_device_id ota5601a_of_match[] = {
> +       { .compatible = "focaltech,gpt3", .data = &gpt3_info },
> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ota5601a_of_match);

Does this mean the display controller is ota5601a and the display
manufacturer and product is focaltech gpt3? Then it's fine.

Overall the driver looks very good, just the above little details.

Yours,
Linus Walleij

Re: [PATCH v2 1/2] drm/panel: add the orisetech ota5601a

[Linus Walleij]

Hi Christophe,

thanks for your patch!

On Wed, Dec 14, 2022 at 12:01 PM Christophe Branchereau
<cbranchereau@gmail.com> wrote:

> Add the orisetech ota5601a ic driver
>
> For now it only supports the focaltech gpt3 3" 640x480 ips panel
> found in the ylm rg300x handheld.
>
> Signed-off-by: Christophe Branchereau <cbranchereau@gmail.com>
(...)
> +config DRM_PANEL_ORISETECH_OTA5601A
> +       tristate "Orise Technology ota5601a RGB/SPI panel"
> +       depends on OF && SPI
> +       depends on BACKLIGHT_CLASS_DEVICE
> +       select REGMAP_SPI

Nice use of regmap in this driver.

> +static const struct reg_sequence ota5601a_panel_regs[] = {
> +       { 0xfd, 0x00 },
> +       { 0x02, 0x00 },
> +
> +       { 0x18, 0x00 },
> +       { 0x34, 0x20 },
> +
> +       { 0x0c, 0x01 },
> +       { 0x0d, 0x48 },
> +       { 0x0e, 0x48 },
> +       { 0x0f, 0x48 },
> +       { 0x07, 0x40 },
> +       { 0x08, 0x33 },
> +       { 0x09, 0x3a },
> +
> +       { 0x16, 0x01 },
> +       { 0x19, 0x8d },
> +       { 0x1a, 0x28 },
> +       { 0x1c, 0x00 },
> +
> +       { 0xfd, 0xc5 },
> +       { 0x82, 0x0c },
> +       { 0xa2, 0xb4 },
> +
> +       { 0xfd, 0xc4 },
> +       { 0x82, 0x45 },
> +
> +       { 0xfd, 0xc1 },
> +       { 0x91, 0x02 },
> +
> +       { 0xfd, 0xc0 },
> +       { 0xa1, 0x01 },
> +       { 0xa2, 0x1f },
> +       { 0xa3, 0x0b },
> +       { 0xa4, 0x38 },
> +       { 0xa5, 0x00 },
> +       { 0xa6, 0x0a },
> +       { 0xa7, 0x38 },
> +       { 0xa8, 0x00 },
> +       { 0xa9, 0x0a },
> +       { 0xaa, 0x37 },
> +
> +       { 0xfd, 0xce },
> +       { 0x81, 0x18 },
> +       { 0x82, 0x43 },
> +       { 0x83, 0x43 },
> +       { 0x91, 0x06 },
> +       { 0x93, 0x38 },
> +       { 0x94, 0x02 },
> +       { 0x95, 0x06 },
> +       { 0x97, 0x38 },
> +       { 0x98, 0x02 },
> +       { 0x99, 0x06 },
> +       { 0x9b, 0x38 },
> +       { 0x9c, 0x02 },
> +
> +       { 0xfd, 0x00 },
> +};

If these are registers, why is register 0xfd written 7 times with different
values?

This is rather a jam table or intializations sequence, so it should be
names something like that and a comment about undocumented
registers added probably as well.

> +static int ota5601a_enable(struct drm_panel *drm_panel)
> +{
> +       struct ota5601a *panel = to_ota5601a(drm_panel);
> +       int err;
> +
> +       err = regmap_write(panel->map, 0x01, 0x01);

Since you know what this register does, what about:

#include <linux/bits.h>

#define OTA5601A_CTL 0x01
#define OTA5601A_CTL_OFF 0x00
#define OTA5601A_CTL_ON BIT(0)

> +static int ota5601a_get_modes(struct drm_panel *drm_panel,
> +                            struct drm_connector *connector)
> +{
> +       struct ota5601a *panel = to_ota5601a(drm_panel);
> +       const struct ota5601a_panel_info *panel_info = panel->panel_info;
> +       struct drm_display_mode *mode;
> +       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)

But ... you have just added an array that contains 2 elements, you
KNOW it will not be 1.

> +                       mode->type |= DRM_MODE_TYPE_PREFERRED;

I think you should probably set this on the preferred resolution
anyways, take the one you actually use.

> +static const struct of_device_id ota5601a_of_match[] = {
> +       { .compatible = "focaltech,gpt3", .data = &gpt3_info },
> +       { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ota5601a_of_match);

Does this mean the display controller is ota5601a and the display
manufacturer and product is focaltech gpt3? Then it's fine.

Overall the driver looks very good, just the above little details.

Yours,
Linus Walleij

Re: [PATCH v2 1/2] drm/panel: add the orisetech ota5601a

[Christophe Branchereau]

Hello Linus

On Thu, Dec 15, 2022 at 9:42 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Hi Christophe,
>
> thanks for your patch!
>
> On Wed, Dec 14, 2022 at 12:01 PM Christophe Branchereau
> <cbranchereau@gmail.com> wrote:
>
> > Add the orisetech ota5601a ic driver
> >
> > For now it only supports the focaltech gpt3 3" 640x480 ips panel
> > found in the ylm rg300x handheld.
> >
> > Signed-off-by: Christophe Branchereau <cbranchereau@gmail.com>
> (...)
> > +config DRM_PANEL_ORISETECH_OTA5601A
> > +       tristate "Orise Technology ota5601a RGB/SPI panel"
> > +       depends on OF && SPI
> > +       depends on BACKLIGHT_CLASS_DEVICE
> > +       select REGMAP_SPI
>
> Nice use of regmap in this driver.
>
> > +static const struct reg_sequence ota5601a_panel_regs[] = {
> > +       { 0xfd, 0x00 },
> > +       { 0x02, 0x00 },
> > +
> > +       { 0x18, 0x00 },
> > +       { 0x34, 0x20 },
> > +
> > +       { 0x0c, 0x01 },
> > +       { 0x0d, 0x48 },
> > +       { 0x0e, 0x48 },
> > +       { 0x0f, 0x48 },
> > +       { 0x07, 0x40 },
> > +       { 0x08, 0x33 },
> > +       { 0x09, 0x3a },
> > +
> > +       { 0x16, 0x01 },
> > +       { 0x19, 0x8d },
> > +       { 0x1a, 0x28 },
> > +       { 0x1c, 0x00 },
> > +
> > +       { 0xfd, 0xc5 },
> > +       { 0x82, 0x0c },
> > +       { 0xa2, 0xb4 },
> > +
> > +       { 0xfd, 0xc4 },
> > +       { 0x82, 0x45 },
> > +
> > +       { 0xfd, 0xc1 },
> > +       { 0x91, 0x02 },
> > +
> > +       { 0xfd, 0xc0 },
> > +       { 0xa1, 0x01 },
> > +       { 0xa2, 0x1f },
> > +       { 0xa3, 0x0b },
> > +       { 0xa4, 0x38 },
> > +       { 0xa5, 0x00 },
> > +       { 0xa6, 0x0a },
> > +       { 0xa7, 0x38 },
> > +       { 0xa8, 0x00 },
> > +       { 0xa9, 0x0a },
> > +       { 0xaa, 0x37 },
> > +
> > +       { 0xfd, 0xce },
> > +       { 0x81, 0x18 },
> > +       { 0x82, 0x43 },
> > +       { 0x83, 0x43 },
> > +       { 0x91, 0x06 },
> > +       { 0x93, 0x38 },
> > +       { 0x94, 0x02 },
> > +       { 0x95, 0x06 },
> > +       { 0x97, 0x38 },
> > +       { 0x98, 0x02 },
> > +       { 0x99, 0x06 },
> > +       { 0x9b, 0x38 },
> > +       { 0x9c, 0x02 },
> > +
> > +       { 0xfd, 0x00 },
> > +};
>
> If these are registers, why is register 0xfd written 7 times with different
> values?

It initiates a page shift

> This is rather a jam table or intializations sequence, so it should be
> names something like that and a comment about undocumented
> registers added probably as well.

Honestly I don't remember how I got the panel specsheet, thought it
was widely available but I can't find it anymore online,
so yes I guess at least documenting what each page does could be good
for future reference

> > +static int ota5601a_enable(struct drm_panel *drm_panel)
> > +{
> > +       struct ota5601a *panel = to_ota5601a(drm_panel);
> > +       int err;
> > +
> > +       err = regmap_write(panel->map, 0x01, 0x01);
>
> Since you know what this register does, what about:
>
> #include <linux/bits.h>
>
> #define OTA5601A_CTL 0x01
> #define OTA5601A_CTL_OFF 0x00
> #define OTA5601A_CTL_ON BIT(0)

I can definitely do that if it improves clarity

> > +static int ota5601a_get_modes(struct drm_panel *drm_panel,
> > +                            struct drm_connector *connector)
> > +{
> > +       struct ota5601a *panel = to_ota5601a(drm_panel);
> > +       const struct ota5601a_panel_info *panel_info = panel->panel_info;
> > +       struct drm_display_mode *mode;
> > +       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)
>
> But ... you have just added an array that contains 2 elements, you
> KNOW it will not be 1.

I know that for the specific panel supported by this driver there are 2 modes,
however in the future someone could want to add a panel using the same IC,
which could have any number of modes

>
> > +                       mode->type |= DRM_MODE_TYPE_PREFERRED;
>
> I think you should probably set this on the preferred resolution
> anyways, take the one you actually use.
>
> > +static const struct of_device_id ota5601a_of_match[] = {
> > +       { .compatible = "focaltech,gpt3", .data = &gpt3_info },
> > +       { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ota5601a_of_match);
>
> Does this mean the display controller is ota5601a and the display
> manufacturer and product is focaltech gpt3? Then it's fine.

Yes

> Overall the driver looks very good, just the above little details.

Ok, will do the v3 next week, holidays etc :)

> Yours,
> Linus Walleij

Re: [PATCH v2 1/2] drm/panel: add the orisetech ota5601a

[Christophe Branchereau]

Hello Linus

On Thu, Dec 15, 2022 at 9:42 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Hi Christophe,
>
> thanks for your patch!
>
> On Wed, Dec 14, 2022 at 12:01 PM Christophe Branchereau
> <cbranchereau@gmail.com> wrote:
>
> > Add the orisetech ota5601a ic driver
> >
> > For now it only supports the focaltech gpt3 3" 640x480 ips panel
> > found in the ylm rg300x handheld.
> >
> > Signed-off-by: Christophe Branchereau <cbranchereau@gmail.com>
> (...)
> > +config DRM_PANEL_ORISETECH_OTA5601A
> > +       tristate "Orise Technology ota5601a RGB/SPI panel"
> > +       depends on OF && SPI
> > +       depends on BACKLIGHT_CLASS_DEVICE
> > +       select REGMAP_SPI
>
> Nice use of regmap in this driver.
>
> > +static const struct reg_sequence ota5601a_panel_regs[] = {
> > +       { 0xfd, 0x00 },
> > +       { 0x02, 0x00 },
> > +
> > +       { 0x18, 0x00 },
> > +       { 0x34, 0x20 },
> > +
> > +       { 0x0c, 0x01 },
> > +       { 0x0d, 0x48 },
> > +       { 0x0e, 0x48 },
> > +       { 0x0f, 0x48 },
> > +       { 0x07, 0x40 },
> > +       { 0x08, 0x33 },
> > +       { 0x09, 0x3a },
> > +
> > +       { 0x16, 0x01 },
> > +       { 0x19, 0x8d },
> > +       { 0x1a, 0x28 },
> > +       { 0x1c, 0x00 },
> > +
> > +       { 0xfd, 0xc5 },
> > +       { 0x82, 0x0c },
> > +       { 0xa2, 0xb4 },
> > +
> > +       { 0xfd, 0xc4 },
> > +       { 0x82, 0x45 },
> > +
> > +       { 0xfd, 0xc1 },
> > +       { 0x91, 0x02 },
> > +
> > +       { 0xfd, 0xc0 },
> > +       { 0xa1, 0x01 },
> > +       { 0xa2, 0x1f },
> > +       { 0xa3, 0x0b },
> > +       { 0xa4, 0x38 },
> > +       { 0xa5, 0x00 },
> > +       { 0xa6, 0x0a },
> > +       { 0xa7, 0x38 },
> > +       { 0xa8, 0x00 },
> > +       { 0xa9, 0x0a },
> > +       { 0xaa, 0x37 },
> > +
> > +       { 0xfd, 0xce },
> > +       { 0x81, 0x18 },
> > +       { 0x82, 0x43 },
> > +       { 0x83, 0x43 },
> > +       { 0x91, 0x06 },
> > +       { 0x93, 0x38 },
> > +       { 0x94, 0x02 },
> > +       { 0x95, 0x06 },
> > +       { 0x97, 0x38 },
> > +       { 0x98, 0x02 },
> > +       { 0x99, 0x06 },
> > +       { 0x9b, 0x38 },
> > +       { 0x9c, 0x02 },
> > +
> > +       { 0xfd, 0x00 },
> > +};
>
> If these are registers, why is register 0xfd written 7 times with different
> values?

It initiates a page shift

> This is rather a jam table or intializations sequence, so it should be
> names something like that and a comment about undocumented
> registers added probably as well.

Honestly I don't remember how I got the panel specsheet, thought it
was widely available but I can't find it anymore online,
so yes I guess at least documenting what each page does could be good
for future reference

> > +static int ota5601a_enable(struct drm_panel *drm_panel)
> > +{
> > +       struct ota5601a *panel = to_ota5601a(drm_panel);
> > +       int err;
> > +
> > +       err = regmap_write(panel->map, 0x01, 0x01);
>
> Since you know what this register does, what about:
>
> #include <linux/bits.h>
>
> #define OTA5601A_CTL 0x01
> #define OTA5601A_CTL_OFF 0x00
> #define OTA5601A_CTL_ON BIT(0)

I can definitely do that if it improves clarity

> > +static int ota5601a_get_modes(struct drm_panel *drm_panel,
> > +                            struct drm_connector *connector)
> > +{
> > +       struct ota5601a *panel = to_ota5601a(drm_panel);
> > +       const struct ota5601a_panel_info *panel_info = panel->panel_info;
> > +       struct drm_display_mode *mode;
> > +       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)
>
> But ... you have just added an array that contains 2 elements, you
> KNOW it will not be 1.

I know that for the specific panel supported by this driver there are 2 modes,
however in the future someone could want to add a panel using the same IC,
which could have any number of modes

>
> > +                       mode->type |= DRM_MODE_TYPE_PREFERRED;
>
> I think you should probably set this on the preferred resolution
> anyways, take the one you actually use.
>
> > +static const struct of_device_id ota5601a_of_match[] = {
> > +       { .compatible = "focaltech,gpt3", .data = &gpt3_info },
> > +       { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, ota5601a_of_match);
>
> Does this mean the display controller is ota5601a and the display
> manufacturer and product is focaltech gpt3? Then it's fine.

Yes

> Overall the driver looks very good, just the above little details.

Ok, will do the v3 next week, holidays etc :)

> Yours,
> Linus Walleij

[PATCH v2 2/2] dt-bindings: display/panel: Add the Focaltech gpt3

[Christophe Branchereau]

Add bindings for the Forcaltech gpt3, which is a 640x480 3.0" 4:3
IPS LCD Panel found in the YLM/Anbernic RG300X handheld.

Signed-off-by: Christophe Branchereau <cbranchereau@gmail.com>
---
 .../display/panel/focaltech,gpt3.yaml         | 56 +++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/focaltech,gpt3.yaml

diff --git a/Documentation/devicetree/bindings/display/panel/focaltech,gpt3.yaml b/Documentation/devicetree/bindings/display/panel/focaltech,gpt3.yaml
new file mode 100644
index 000000000000..d54e96b2a9e1
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/focaltech,gpt3.yaml
@@ -0,0 +1,56 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/focaltech,gpt3.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Focaltech GPT3 3.0" (640x480 pixels) IPS LCD panel
+
+maintainers:
+  - Christophe Branchereau <cbranchereau@gmail.com>
+
+allOf:
+  - $ref: panel-common.yaml#
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    const: focaltech,gpt3
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - power-supply
+  - reset-gpios
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        panel@0 {
+            compatible = "focaltech,gpt3";
+            reg = <0>;
+
+            spi-max-frequency = <3125000>;
+
+            reset-gpios = <&gpe 2 GPIO_ACTIVE_LOW>;
+
+            backlight = <&backlight>;
+            power-supply = <&vcc>;
+
+            port {
+                panel_input: endpoint {
+                    remote-endpoint = <&panel_output>;
+                };
+            };
+        };
+    };
-- 
2.35.1

Re: [PATCH v2 2/2] dt-bindings: display/panel: Add the Focaltech gpt3

[Krzysztof Kozlowski]

On 14/12/2022 12:00, Christophe Branchereau wrote:
> Add bindings for the Forcaltech gpt3, which is a 640x480 3.0" 4:3
> IPS LCD Panel found in the YLM/Anbernic RG300X handheld.
> 
> Signed-off-by: Christophe Branchereau <cbranchereau@gmail.com>


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v2 0/2] Add support for the AUO A030JTN01 TFT LCD

[Christophe Branchereau]

Wrong cover title, should be "Add support for the orisetech ota5601" sorry...

On Wed, Dec 14, 2022 at 12:00 PM Christophe Branchereau
<cbranchereau@gmail.com> wrote:
>
> Changes since v1:
>  fixed the dt-bindings maintainer email adress
>
>  dropped backlight, port, power-supply and reset-gpios as they're
>  provided by panel-common.yaml as pointed by Krzysztof Kozlowski
>
>  changed reg: true to reg : maxItems: 1
>
> Christophe Branchereau (2):
>   drm/panel: add the orisetech ota5601a
>   dt-bindings: display/panel: Add the Focaltech gpt3
>
>  .../display/panel/focaltech,gpt3.yaml         |  56 +++
>  drivers/gpu/drm/panel/Kconfig                 |   9 +
>  drivers/gpu/drm/panel/Makefile                |   1 +
>  .../gpu/drm/panel/panel-orisetech-ota5601a.c  | 351 ++++++++++++++++++
>  4 files changed, 417 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/focaltech,gpt3.yaml
>  create mode 100644 drivers/gpu/drm/panel/panel-orisetech-ota5601a.c
>
> --
> 2.35.1
>