[PATCH 1/2] drm/panel: Add DT bindings for Sony ACX424AKP

[PATCH 1/2] drm/panel: Add DT bindings for Sony ACX424AKP

[Linus Walleij]

This adds device tree bindings for the Sony ACX424AKP panel.
Let's use YAML.

Cc: devicetree@vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 .../display/panel/sony,acx424akp.yaml         | 53 +++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/sony,acx424akp.yaml

diff --git a/Documentation/devicetree/bindings/display/panel/sony,acx424akp.yaml b/Documentation/devicetree/bindings/display/panel/sony,acx424akp.yaml
new file mode 100644
index 000000000000..29f41fee1b69
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/sony,acx424akp.yaml
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/sony,acx424akp.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sony ACX424AKP 4" 480x864 AMOLED panel
+
+maintainers:
+  - Linus Walleij <linus.walleij@linaro.org>
+
+allOf:
+  - $ref: panel-common.yaml#
+
+properties:
+  compatible:
+    const: sony,acx424akp
+  reg: true
+  port: true
+  reset-gpios: true
+  vddi-supply:
+     description: regulator that supplies the vddi voltage
+  dsi-command-mode:
+     type: boolean
+     description:
+       If this is specified, the panel will be used in command
+       mode instead of video mode.
+
+required:
+  - compatible
+  - reg
+  - port
+  - reset-gpios
+  - power-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    panel@0 {
+        compatible = "sony,acx424akp";
+        reg = <0>;
+        vddi-supply = <&foo>;
+        reset-gpios = <&foo_gpio 0 GPIO_ACTIVE_LOW>;
+
+        port {
+            panel_in: endpoint {
+                remote-endpoint = <&dsi_out>;
+            };
+        };
+    };
+
+...
\ No newline at end of file
-- 
2.21.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/panel: Add driver for Sony ACX424AKP panel

[Linus Walleij]

The Sony ACX424AKP is a command/videomode DSI panel for
mobile devices. It is used on the ST-Ericsson HREF520
reference design. We support video mode by default, but
it is possible to switch the panel into command mode
by using the bool property "dsi-command-mode".

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpu/drm/panel/Kconfig                |   7 +
 drivers/gpu/drm/panel/Makefile               |   1 +
 drivers/gpu/drm/panel/panel-sony-acx424akp.c | 530 +++++++++++++++++++
 3 files changed, 538 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-sony-acx424akp.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index d9d931aa6e26..435388a874e2 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -275,6 +275,13 @@ config DRM_PANEL_SITRONIX_ST7789V
 	  Say Y here if you want to enable support for the Sitronix
 	  ST7789V controller for 240x320 LCD panels
 
+config DRM_PANEL_SONY_ACX424AKP
+	tristate "Sony ACX424AKP DSI command mode panel"
+	depends on OF
+	depends on DRM_MIPI_DSI
+	depends on BACKLIGHT_CLASS_DEVICE
+	select VIDEOMODE_HELPERS
+
 config DRM_PANEL_TPO_TPG110
 	tristate "TPO TPG 800x400 panel"
 	depends on OF && SPI && GPIOLIB
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index fb0cb3aaa9e6..9ed4d853267e 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -28,5 +28,6 @@ obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
 obj-$(CONFIG_DRM_PANEL_SHARP_LS043T1LE01) += panel-sharp-ls043t1le01.o
 obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7701) += panel-sitronix-st7701.o
 obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += panel-sitronix-st7789v.o
+obj-$(CONFIG_DRM_PANEL_SONY_ACX424AKP) += panel-sony-acx424akp.o
 obj-$(CONFIG_DRM_PANEL_TPO_TPG110) += panel-tpo-tpg110.o
 obj-$(CONFIG_DRM_PANEL_TRULY_NT35597_WQXGA) += panel-truly-nt35597.o
diff --git a/drivers/gpu/drm/panel/panel-sony-acx424akp.c b/drivers/gpu/drm/panel/panel-sony-acx424akp.c
new file mode 100644
index 000000000000..807560d2a8ec
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-sony-acx424akp.c
@@ -0,0 +1,530 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * MIPI-DSI Sony ACX424AKP panel driver. This is a 480x864
+ * AMOLED panel with a command-only DSI interface.
+ *
+ * Copyright (C) Linaro Ltd. 2019
+ * Author: Linus Walleij
+ * Based on code and know-how from Marcus Lorentzon
+ * Copyright (C) ST-Ericsson SA 2010
+ */
+
+#include <drm/drm_modes.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_panel.h>
+#include <drm/drm_print.h>
+#include <video/mipi_display.h>
+
+#include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>
+#include <linux/delay.h>
+#include <linux/of.h>
+#include <linux/module.h>
+#include <linux/backlight.h>
+
+/* FIXME: move to <video/mipi_display.h> ? */
+#define MIPI_DSI_DCS_SET_MDDI 0xAE
+
+#define DISPLAY_SONY_ACX424AKP_ID1	0x1b81
+#define DISPLAY_SONY_ACX424AKP_ID2	0x1a81
+#define DISPLAY_SONY_ACX424AKP_ID3	0x0080
+
+struct acx424akp {
+	struct device *dev;
+	struct drm_panel panel;
+	struct backlight_device *bl;
+	struct regulator *supply;
+	struct gpio_desc *reset_gpio;
+	u16 id;
+	bool video_mode;
+};
+
+static const struct drm_display_mode sony_acx424akp_vid_mode = {
+	.clock = 330000,
+	.hdisplay = 480,
+	.hsync_start = 480 + 15,
+	.hsync_end = 480 + 15 + 0,
+	.htotal = 480 + 15 + 0 + 15,
+	.vdisplay = 864,
+	.vsync_start = 864 + 14,
+	.vsync_end = 864 + 14 + 1,
+	.vtotal = 864 + 14 + 1 + 11,
+	.vrefresh = 60,
+	.width_mm = 48,
+	.height_mm = 84,
+	.flags = DRM_MODE_FLAG_PVSYNC,
+};
+
+/*
+ * The timings are not very helpful as the display is used in
+ * command mode.
+ */
+static const struct drm_display_mode sony_acx424akp_cmd_mode = {
+	/* HS clock, (htotal*vtotal*vrefresh)/1000 */
+	.clock = 420160,
+	.hdisplay = 480,
+	.hsync_start = 480 + 154,
+	.hsync_end = 480 + 154 + 16,
+	.htotal = 480 + 154 + 16 + 32,
+	.vdisplay = 864,
+	.vsync_start = 864 + 1,
+	.vsync_end = 864 + 1 + 1,
+	.vtotal = 864 + 1 + 1 + 1,
+	/*
+	 * This depends on the clocking HS vs LP rate, this value
+	 * is calculated as:
+	 * vrefresh = (clock * 1000) / (htotal*vtotal)
+	 */
+	.vrefresh = 816,
+	.width_mm = 48,
+	.height_mm = 84,
+};
+
+static inline struct acx424akp *panel_to_acx424akp(struct drm_panel *panel)
+{
+	return container_of(panel, struct acx424akp, panel);
+}
+
+#define FOSC			20 /* 20Mhz */
+#define SCALE_FACTOR_NS_DIV_MHZ	1000
+
+static int acx424akp_set_brightness(struct backlight_device *bl)
+{
+	struct acx424akp *acx = bl_get_data(bl);
+	struct mipi_dsi_device *dsi = to_mipi_dsi_device(acx->dev);
+	int period_ns = 1023;
+	int duty_ns = bl->props.brightness;
+	u8 pwm_ratio;
+	u8 pwm_div;
+	u8 par;
+	int ret;
+
+	/* Calculate the PWM duty cycle in n/256's */
+	pwm_ratio = max(((duty_ns * 256) / period_ns) - 1, 1);
+	pwm_div = max(1,
+		      ((FOSC * period_ns) / 256) /
+		      SCALE_FACTOR_NS_DIV_MHZ);
+
+	/* Set up PWM dutycycle ONE byte (differs from the standard) */
+	DRM_DEV_DEBUG(acx->dev, "calculated duty cycle %02x\n", pwm_ratio);
+	ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS,
+				 &pwm_ratio, 1);
+	if (ret < 0) {
+		DRM_DEV_ERROR(acx->dev,
+			      "failed to set display PWM ratio (%d)\n",
+			      ret);
+		return ret;
+	}
+
+	/*
+	 * Sequence to write PWMDIV:
+	 *	address		data
+	 *	0xF3		0xAA   CMD2 Unlock
+	 *	0x00		0x01   Enter CMD2 page 0
+	 *	0X7D		0x01   No reload MTP of CMD2 P1
+	 *	0x22		PWMDIV
+	 *	0x7F		0xAA   CMD2 page 1 lock
+	 */
+	par = 0xaa;
+	ret = mipi_dsi_dcs_write(dsi, 0xf3, &par, 1);
+	if (ret < 0) {
+		DRM_DEV_ERROR(acx->dev,
+			      "failed to unlock CMD 2 (%d)\n",
+			      ret);
+		return ret;
+	}
+	par = 0x01;
+	ret = mipi_dsi_dcs_write(dsi, 0x00, &par, 1);
+	if (ret < 0) {
+		DRM_DEV_ERROR(acx->dev,
+			      "failed to enter page 1 (%d)\n",
+			      ret);
+		return ret;
+	}
+	par = 0x01;
+	ret = mipi_dsi_dcs_write(dsi, 0x7d, &par, 1);
+	if (ret < 0) {
+		DRM_DEV_ERROR(acx->dev,
+			      "failed to disable MTP reload (%d)\n",
+			      ret);
+		return ret;
+	}
+	ret = mipi_dsi_dcs_write(dsi, 0x22, &pwm_div, 1);
+	if (ret < 0) {
+		DRM_DEV_ERROR(acx->dev,
+			      "failed to set PWM divisor (%d)\n",
+			      ret);
+		return ret;
+	}
+	par = 0xaa;
+	ret = mipi_dsi_dcs_write(dsi, 0x7f, &par, 1);
+	if (ret < 0) {
+		DRM_DEV_ERROR(acx->dev,
+			      "failed to lock CMD 2 (%d)\n",
+			      ret);
+		return ret;
+	}
+
+	/* Enable backlight */
+	par = 0x24;
+	ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY,
+				 &par, 1);
+	if (ret < 0) {
+		DRM_DEV_ERROR(acx->dev,
+			      "failed to enable display backlight (%d)\n",
+			      ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct backlight_ops acx424akp_bl_ops = {
+	.update_status = acx424akp_set_brightness,
+};
+
+static int acx424akp_read_id(struct acx424akp *acx)
+{
+	struct mipi_dsi_device *dsi = to_mipi_dsi_device(acx->dev);
+	u8 id1, id2, id3;
+	u16 val;
+	size_t len;
+	int ret;
+
+	len = 1;
+
+	ret = mipi_dsi_dcs_read(dsi, 0xda, &id1, len);
+	if (ret < 0) {
+		DRM_DEV_ERROR(acx->dev, "could not read device ID byte 1\n");
+		return ret;
+	}
+	ret = mipi_dsi_dcs_read(dsi, 0xdb, &id2, len);
+	if (ret < 0) {
+		DRM_DEV_ERROR(acx->dev, "could not read device ID byte 2\n");
+		return ret;
+	}
+	ret = mipi_dsi_dcs_read(dsi, 0xdc, &id3, len);
+	if (ret < 0) {
+		DRM_DEV_ERROR(acx->dev, "could not read device ID byte 3\n");
+		return ret;
+	}
+
+	val = (id3 << 8) | id2;
+
+	switch (val) {
+	case DISPLAY_SONY_ACX424AKP_ID1:
+	case DISPLAY_SONY_ACX424AKP_ID2:
+	case DISPLAY_SONY_ACX424AKP_ID3:
+		DRM_DEV_INFO(acx->dev, "panel ID: %04x\n", val);
+		break;
+	default:
+		DRM_DEV_ERROR(acx->dev, "unknown panel ID: %04x\n", val);
+		break;
+	};
+	acx->id = val;
+
+	return 0;
+}
+
+static int acx424akp_power_on(struct acx424akp *acx)
+{
+	int ret;
+
+	ret = regulator_enable(acx->supply);
+	if (ret) {
+		DRM_DEV_ERROR(acx->dev, "failed to enable supply (%d)\n", ret);
+		return ret;
+	}
+
+	/* Assert RESET */
+	gpiod_set_value_cansleep(acx->reset_gpio, 1);
+	udelay(20);
+	/* De-assert RESET */
+	gpiod_set_value_cansleep(acx->reset_gpio, 0);
+	msleep(11);
+
+	return 0;
+}
+
+static int acx424akp_prepare(struct drm_panel *panel)
+{
+	struct acx424akp *acx = panel_to_acx424akp(panel);
+	struct mipi_dsi_device *dsi = to_mipi_dsi_device(acx->dev);
+	const u8 mddi = 3;
+	int ret;
+
+	ret = acx424akp_power_on(acx);
+	if (ret)
+		return ret;
+
+	/* Enabe tearing mode: send TE (tearing effect) at VBLANK */
+	ret = mipi_dsi_dcs_set_tear_on(dsi,
+				       MIPI_DSI_DCS_TEAR_MODE_VBLANK);
+	if (ret) {
+		DRM_DEV_ERROR(acx->dev, "failed to enable vblank TE (%d)\n",
+			      ret);
+		return ret;
+	}
+
+	/* Set MDDI - doesn't seem to work? */
+	ret = mipi_dsi_dcs_write(dsi, MIPI_DSI_DCS_SET_MDDI,
+				 &mddi, sizeof(mddi));
+	if (ret < 0) {
+		DRM_DEV_ERROR(acx->dev, "failed to set MDDI (%d)\n", ret);
+		return ret;
+	}
+
+	acx->bl->props.power = FB_BLANK_NORMAL;
+
+	return 0;
+}
+
+static void acx424akp_power_off(struct acx424akp *acx)
+{
+	/* Assert RESET */
+	gpiod_set_value_cansleep(acx->reset_gpio, 1);
+	msleep(11);
+
+	regulator_disable(acx->supply);
+}
+
+static int acx424akp_unprepare(struct drm_panel *panel)
+{
+	struct acx424akp *acx = panel_to_acx424akp(panel);
+
+	acx424akp_power_off(acx);
+	acx->bl->props.power = FB_BLANK_POWERDOWN;
+
+	return 0;
+}
+
+static int acx424akp_enable(struct drm_panel *panel)
+{
+	struct acx424akp *acx = panel_to_acx424akp(panel);
+	struct mipi_dsi_device *dsi = to_mipi_dsi_device(acx->dev);
+	int ret;
+
+	/* Exit sleep mode */
+	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
+	if (ret) {
+		DRM_DEV_ERROR(acx->dev, "failed to exit sleep mode (%d)\n",
+			      ret);
+		return ret;
+	}
+	msleep(140);
+
+	ret = mipi_dsi_dcs_set_display_on(dsi);
+	if (ret) {
+		DRM_DEV_ERROR(acx->dev, "failed to turn display on (%d)\n",
+			      ret);
+		return ret;
+	}
+	if (acx->video_mode) {
+		/* In video mode turn peripheral on */
+		ret = mipi_dsi_turn_on_peripheral(dsi);
+		if (ret) {
+			dev_err(acx->dev, "failed to turn on peripheral\n");
+			return ret;
+		}
+	}
+
+	acx->bl->props.power = FB_BLANK_UNBLANK;
+
+	return 0;
+}
+
+static int acx424akp_disable(struct drm_panel *panel)
+{
+	struct acx424akp *acx = panel_to_acx424akp(panel);
+	struct mipi_dsi_device *dsi = to_mipi_dsi_device(acx->dev);
+	u8 par;
+	int ret;
+
+	/* Disable backlight */
+	par = 0x00;
+	ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY,
+				 &par, 1);
+	if (ret) {
+		DRM_DEV_ERROR(acx->dev,
+			      "failed to disable display backlight (%d)\n",
+			      ret);
+		return ret;
+	}
+
+	ret = mipi_dsi_dcs_set_display_off(dsi);
+	if (ret) {
+		DRM_DEV_ERROR(acx->dev, "failed to turn display off (%d)\n",
+			      ret);
+		return ret;
+	}
+
+	/* Enter sleep mode */
+	ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
+	if (ret) {
+		DRM_DEV_ERROR(acx->dev, "failed to enter sleep mode (%d)\n",
+			      ret);
+		return ret;
+	}
+	msleep(85);
+
+	acx->bl->props.power = FB_BLANK_NORMAL;
+
+	return 0;
+}
+
+static int acx424akp_get_modes(struct drm_panel *panel)
+{
+	struct acx424akp *acx = panel_to_acx424akp(panel);
+	struct drm_connector *connector = panel->connector;
+	struct drm_display_mode *mode;
+
+	if (acx->video_mode)
+		mode = drm_mode_duplicate(panel->drm,
+					  &sony_acx424akp_vid_mode);
+	else
+		mode = drm_mode_duplicate(panel->drm,
+					  &sony_acx424akp_cmd_mode);
+	if (!mode) {
+		DRM_ERROR("bad mode or failed to add mode\n");
+		return -EINVAL;
+	}
+	drm_mode_set_name(mode);
+	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
+
+	connector->display_info.width_mm = mode->width_mm;
+	connector->display_info.height_mm = mode->height_mm;
+
+	drm_mode_probed_add(connector, mode);
+
+	return 1; /* Number of modes */
+}
+
+static const struct drm_panel_funcs acx424akp_drm_funcs = {
+	.disable = acx424akp_disable,
+	.unprepare = acx424akp_unprepare,
+	.prepare = acx424akp_prepare,
+	.enable = acx424akp_enable,
+	.get_modes = acx424akp_get_modes,
+};
+
+static int acx424akp_probe(struct mipi_dsi_device *dsi)
+{
+	struct device *dev = &dsi->dev;
+	struct acx424akp *acx;
+	int ret;
+	int i;
+
+	acx = devm_kzalloc(dev, sizeof(struct acx424akp), GFP_KERNEL);
+	if (!acx)
+		return -ENOMEM;
+	acx->video_mode = !of_property_read_bool(dev->of_node,
+						 "dsi-command-mode");
+
+	mipi_dsi_set_drvdata(dsi, acx);
+	acx->dev = dev;
+
+	dsi->lanes = 2;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->hs_rate = 420160000;
+	dsi->lp_rate = 19200000;
+
+	if (acx->video_mode)
+		dsi->mode_flags =
+			MIPI_DSI_MODE_VIDEO |
+			MIPI_DSI_MODE_VIDEO_BURST |
+			MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
+	else
+		dsi->mode_flags =
+			MIPI_DSI_CLOCK_NON_CONTINUOUS |
+			MIPI_DSI_MODE_EOT_PACKET;
+
+
+	acx->supply = devm_regulator_get(dev, "vddi");
+	if (IS_ERR(acx->supply))
+		return PTR_ERR(acx->supply);
+
+	/* This asserts RESET by default */
+	acx->reset_gpio = devm_gpiod_get_optional(dev, "reset",
+						 GPIOD_OUT_HIGH);
+	if (IS_ERR(acx->reset_gpio)) {
+		ret = PTR_ERR(acx->reset_gpio);
+		if (ret != -EPROBE_DEFER)
+			DRM_DEV_ERROR(dev, "failed to request GPIO (%d)\n",
+				      ret);
+		return ret;
+	}
+
+	drm_panel_init(&acx->panel);
+	acx->panel.dev = dev;
+	acx->panel.funcs = &acx424akp_drm_funcs;
+
+	acx->bl = devm_backlight_device_register(dev, "acx424akp", dev, acx,
+						 &acx424akp_bl_ops, NULL);
+	if (IS_ERR(acx->bl)) {
+		DRM_DEV_ERROR(dev, "failed to register backlight device\n");
+		return PTR_ERR(acx->bl);
+	}
+	acx->bl->props.max_brightness = 1023;
+	acx->bl->props.brightness = 512;
+	acx->bl->props.power = FB_BLANK_POWERDOWN;
+
+	ret = drm_panel_add(&acx->panel);
+	if (ret < 0)
+		return ret;
+
+	ret = acx424akp_power_on(acx);
+	if (ret) {
+		DRM_DEV_ERROR(dev, "failed to power up display\n");
+		return ret;
+	}
+
+	/* Read device ID */
+	i = 0;
+	do {
+		ret = acx424akp_read_id(acx);
+		if (ret)
+			continue;
+	} while (ret && i++ < 5);
+
+	if (ret) {
+		DRM_DEV_ERROR(acx->dev, "failed to read panel ID (%d)\n", ret);
+		return ret;
+	}
+	acx424akp_power_off(acx);
+
+	ret = mipi_dsi_attach(dsi);
+	if (ret < 0)
+		drm_panel_remove(&acx->panel);
+
+	return ret;
+}
+
+static int acx424akp_remove(struct mipi_dsi_device *dsi)
+{
+	struct acx424akp *acx = mipi_dsi_get_drvdata(dsi);
+
+	mipi_dsi_detach(dsi);
+	drm_panel_remove(&acx->panel);
+
+	return 0;
+}
+
+static const struct of_device_id acx424akp_of_match[] = {
+	{ .compatible = "sony,acx424akp" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, acx424akp_of_match);
+
+static struct mipi_dsi_driver acx424akp_driver = {
+	.probe = acx424akp_probe,
+	.remove = acx424akp_remove,
+	.driver = {
+		.name = "panel-sony-acx424akp",
+		.of_match_table = acx424akp_of_match,
+	},
+};
+module_mipi_dsi_driver(acx424akp_driver);
+
+MODULE_AUTHOR("Linus Wallei <linus.walleij@linaro.org>");
+MODULE_DESCRIPTION("MIPI-DSI Sony acx424akp Panel Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.21.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/panel: Add DT bindings for Sony ACX424AKP

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2646 bytes --]

On Mon, Sep 02, 2019 at 11:06:32AM +0200, Linus Walleij wrote:
> This adds device tree bindings for the Sony ACX424AKP panel.
> Let's use YAML.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  .../display/panel/sony,acx424akp.yaml         | 53 +++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/sony,acx424akp.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/sony,acx424akp.yaml b/Documentation/devicetree/bindings/display/panel/sony,acx424akp.yaml
> new file mode 100644
> index 000000000000..29f41fee1b69
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/sony,acx424akp.yaml
> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/panel/sony,acx424akp.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sony ACX424AKP 4" 480x864 AMOLED panel
> +
> +maintainers:
> +  - Linus Walleij <linus.walleij@linaro.org>
> +
> +allOf:
> +  - $ref: panel-common.yaml#
> +
> +properties:
> +  compatible:
> +    const: sony,acx424akp
> +  reg: true
> +  port: true
> +  reset-gpios: true
> +  vddi-supply:
> +     description: regulator that supplies the vddi voltage
> +  dsi-command-mode:
> +     type: boolean
> +     description:
> +       If this is specified, the panel will be used in command
> +       mode instead of video mode.

I'm not sure there's concensus on this one yet. I think so far the
driver decides which mode to use the panel in. Technically this falls
into the category of configuration, so it doesn't really belong in the
DT.

I vaguely recall from discussions I've had on this subject that there's
usually no reason to do video mode if you can do command mode because
command mode is more power efficient. This was a long time ago, so I may
be misremembering. Perhaps you have different information on this?

Thierry

> +
> +required:
> +  - compatible
> +  - reg
> +  - port
> +  - reset-gpios
> +  - power-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    panel@0 {
> +        compatible = "sony,acx424akp";
> +        reg = <0>;
> +        vddi-supply = <&foo>;
> +        reset-gpios = <&foo_gpio 0 GPIO_ACTIVE_LOW>;
> +
> +        port {
> +            panel_in: endpoint {
> +                remote-endpoint = <&dsi_out>;
> +            };
> +        };
> +    };
> +
> +...
> \ No newline at end of file
> -- 
> 2.21.0
> 

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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/panel: Add driver for Sony ACX424AKP panel

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 16840 bytes --]

On Mon, Sep 02, 2019 at 11:06:33AM +0200, Linus Walleij wrote:
> The Sony ACX424AKP is a command/videomode DSI panel for
> mobile devices. It is used on the ST-Ericsson HREF520
> reference design. We support video mode by default, but
> it is possible to switch the panel into command mode
> by using the bool property "dsi-command-mode".
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpu/drm/panel/Kconfig                |   7 +
>  drivers/gpu/drm/panel/Makefile               |   1 +
>  drivers/gpu/drm/panel/panel-sony-acx424akp.c | 530 +++++++++++++++++++
>  3 files changed, 538 insertions(+)
>  create mode 100644 drivers/gpu/drm/panel/panel-sony-acx424akp.c
> 
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index d9d931aa6e26..435388a874e2 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -275,6 +275,13 @@ config DRM_PANEL_SITRONIX_ST7789V
>  	  Say Y here if you want to enable support for the Sitronix
>  	  ST7789V controller for 240x320 LCD panels
>  
> +config DRM_PANEL_SONY_ACX424AKP
> +	tristate "Sony ACX424AKP DSI command mode panel"
> +	depends on OF
> +	depends on DRM_MIPI_DSI
> +	depends on BACKLIGHT_CLASS_DEVICE
> +	select VIDEOMODE_HELPERS
> +
>  config DRM_PANEL_TPO_TPG110
>  	tristate "TPO TPG 800x400 panel"
>  	depends on OF && SPI && GPIOLIB
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index fb0cb3aaa9e6..9ed4d853267e 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -28,5 +28,6 @@ obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
>  obj-$(CONFIG_DRM_PANEL_SHARP_LS043T1LE01) += panel-sharp-ls043t1le01.o
>  obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7701) += panel-sitronix-st7701.o
>  obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += panel-sitronix-st7789v.o
> +obj-$(CONFIG_DRM_PANEL_SONY_ACX424AKP) += panel-sony-acx424akp.o
>  obj-$(CONFIG_DRM_PANEL_TPO_TPG110) += panel-tpo-tpg110.o
>  obj-$(CONFIG_DRM_PANEL_TRULY_NT35597_WQXGA) += panel-truly-nt35597.o
> diff --git a/drivers/gpu/drm/panel/panel-sony-acx424akp.c b/drivers/gpu/drm/panel/panel-sony-acx424akp.c
> new file mode 100644
> index 000000000000..807560d2a8ec
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-sony-acx424akp.c
> @@ -0,0 +1,530 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * MIPI-DSI Sony ACX424AKP panel driver. This is a 480x864
> + * AMOLED panel with a command-only DSI interface.
> + *
> + * Copyright (C) Linaro Ltd. 2019
> + * Author: Linus Walleij
> + * Based on code and know-how from Marcus Lorentzon
> + * Copyright (C) ST-Ericsson SA 2010
> + */
> +
> +#include <drm/drm_modes.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_print.h>
> +#include <video/mipi_display.h>
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/delay.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +#include <linux/backlight.h>
> +
> +/* FIXME: move to <video/mipi_display.h> ? */
> +#define MIPI_DSI_DCS_SET_MDDI 0xAE
> +
> +#define DISPLAY_SONY_ACX424AKP_ID1	0x1b81
> +#define DISPLAY_SONY_ACX424AKP_ID2	0x1a81
> +#define DISPLAY_SONY_ACX424AKP_ID3	0x0080
> +
> +struct acx424akp {
> +	struct device *dev;
> +	struct drm_panel panel;
> +	struct backlight_device *bl;
> +	struct regulator *supply;
> +	struct gpio_desc *reset_gpio;
> +	u16 id;
> +	bool video_mode;
> +};
> +
> +static const struct drm_display_mode sony_acx424akp_vid_mode = {
> +	.clock = 330000,
> +	.hdisplay = 480,
> +	.hsync_start = 480 + 15,
> +	.hsync_end = 480 + 15 + 0,
> +	.htotal = 480 + 15 + 0 + 15,
> +	.vdisplay = 864,
> +	.vsync_start = 864 + 14,
> +	.vsync_end = 864 + 14 + 1,
> +	.vtotal = 864 + 14 + 1 + 11,
> +	.vrefresh = 60,
> +	.width_mm = 48,
> +	.height_mm = 84,
> +	.flags = DRM_MODE_FLAG_PVSYNC,
> +};
> +
> +/*
> + * The timings are not very helpful as the display is used in
> + * command mode.
> + */
> +static const struct drm_display_mode sony_acx424akp_cmd_mode = {
> +	/* HS clock, (htotal*vtotal*vrefresh)/1000 */
> +	.clock = 420160,
> +	.hdisplay = 480,
> +	.hsync_start = 480 + 154,
> +	.hsync_end = 480 + 154 + 16,
> +	.htotal = 480 + 154 + 16 + 32,
> +	.vdisplay = 864,
> +	.vsync_start = 864 + 1,
> +	.vsync_end = 864 + 1 + 1,
> +	.vtotal = 864 + 1 + 1 + 1,
> +	/*
> +	 * This depends on the clocking HS vs LP rate, this value
> +	 * is calculated as:
> +	 * vrefresh = (clock * 1000) / (htotal*vtotal)
> +	 */
> +	.vrefresh = 816,

That's a bit odd. My understanding is that command mode can be done in
continuous mode or in non-continuous mode. In continuous mode you would
typically achieve a similar refresh rate as in video mode. For non-
continuous mode you basically have a variable refresh rate.

For continuous mode you probably want 60 Hz here and for VRR there's
probably no sensible value to set this to. In the latter case, I don't
think it makes sense to set anything arbitrary like you have above.
Perhaps better to just set it to 0 as a way of signalling that this is
actually dependent on when the display hardware sends a new frame?

Have you actually run this is command mode? If so, what's the actual
refresh rate? Do you do on-demand updates or do you run in continuous
mode?

> +	.width_mm = 48,
> +	.height_mm = 84,
> +};
> +
> +static inline struct acx424akp *panel_to_acx424akp(struct drm_panel *panel)
> +{
> +	return container_of(panel, struct acx424akp, panel);
> +}
> +
> +#define FOSC			20 /* 20Mhz */
> +#define SCALE_FACTOR_NS_DIV_MHZ	1000
> +
> +static int acx424akp_set_brightness(struct backlight_device *bl)
> +{
> +	struct acx424akp *acx = bl_get_data(bl);
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(acx->dev);
> +	int period_ns = 1023;
> +	int duty_ns = bl->props.brightness;
> +	u8 pwm_ratio;
> +	u8 pwm_div;
> +	u8 par;
> +	int ret;
> +
> +	/* Calculate the PWM duty cycle in n/256's */
> +	pwm_ratio = max(((duty_ns * 256) / period_ns) - 1, 1);
> +	pwm_div = max(1,
> +		      ((FOSC * period_ns) / 256) /
> +		      SCALE_FACTOR_NS_DIV_MHZ);
> +
> +	/* Set up PWM dutycycle ONE byte (differs from the standard) */
> +	DRM_DEV_DEBUG(acx->dev, "calculated duty cycle %02x\n", pwm_ratio);
> +	ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS,
> +				 &pwm_ratio, 1);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(acx->dev,
> +			      "failed to set display PWM ratio (%d)\n",
> +			      ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Sequence to write PWMDIV:
> +	 *	address		data
> +	 *	0xF3		0xAA   CMD2 Unlock
> +	 *	0x00		0x01   Enter CMD2 page 0
> +	 *	0X7D		0x01   No reload MTP of CMD2 P1
> +	 *	0x22		PWMDIV
> +	 *	0x7F		0xAA   CMD2 page 1 lock
> +	 */
> +	par = 0xaa;
> +	ret = mipi_dsi_dcs_write(dsi, 0xf3, &par, 1);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(acx->dev,
> +			      "failed to unlock CMD 2 (%d)\n",
> +			      ret);
> +		return ret;
> +	}
> +	par = 0x01;
> +	ret = mipi_dsi_dcs_write(dsi, 0x00, &par, 1);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(acx->dev,
> +			      "failed to enter page 1 (%d)\n",
> +			      ret);
> +		return ret;
> +	}
> +	par = 0x01;
> +	ret = mipi_dsi_dcs_write(dsi, 0x7d, &par, 1);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(acx->dev,
> +			      "failed to disable MTP reload (%d)\n",
> +			      ret);
> +		return ret;
> +	}
> +	ret = mipi_dsi_dcs_write(dsi, 0x22, &pwm_div, 1);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(acx->dev,
> +			      "failed to set PWM divisor (%d)\n",
> +			      ret);
> +		return ret;
> +	}
> +	par = 0xaa;
> +	ret = mipi_dsi_dcs_write(dsi, 0x7f, &par, 1);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(acx->dev,
> +			      "failed to lock CMD 2 (%d)\n",
> +			      ret);
> +		return ret;
> +	}
> +
> +	/* Enable backlight */
> +	par = 0x24;
> +	ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY,
> +				 &par, 1);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(acx->dev,
> +			      "failed to enable display backlight (%d)\n",
> +			      ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct backlight_ops acx424akp_bl_ops = {
> +	.update_status = acx424akp_set_brightness,
> +};
> +
> +static int acx424akp_read_id(struct acx424akp *acx)
> +{
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(acx->dev);
> +	u8 id1, id2, id3;
> +	u16 val;
> +	size_t len;
> +	int ret;
> +
> +	len = 1;
> +
> +	ret = mipi_dsi_dcs_read(dsi, 0xda, &id1, len);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(acx->dev, "could not read device ID byte 1\n");
> +		return ret;
> +	}
> +	ret = mipi_dsi_dcs_read(dsi, 0xdb, &id2, len);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(acx->dev, "could not read device ID byte 2\n");
> +		return ret;
> +	}
> +	ret = mipi_dsi_dcs_read(dsi, 0xdc, &id3, len);
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(acx->dev, "could not read device ID byte 3\n");
> +		return ret;
> +	}
> +
> +	val = (id3 << 8) | id2;

Don't you want to OR in id1 here as well? Seems a bit odd to read it but
then not use it.

> +
> +	switch (val) {
> +	case DISPLAY_SONY_ACX424AKP_ID1:
> +	case DISPLAY_SONY_ACX424AKP_ID2:
> +	case DISPLAY_SONY_ACX424AKP_ID3:
> +		DRM_DEV_INFO(acx->dev, "panel ID: %04x\n", val);
> +		break;
> +	default:
> +		DRM_DEV_ERROR(acx->dev, "unknown panel ID: %04x\n", val);
> +		break;
> +	};
> +	acx->id = val;
> +
> +	return 0;
> +}
> +
> +static int acx424akp_power_on(struct acx424akp *acx)
> +{
> +	int ret;
> +
> +	ret = regulator_enable(acx->supply);
> +	if (ret) {
> +		DRM_DEV_ERROR(acx->dev, "failed to enable supply (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	/* Assert RESET */
> +	gpiod_set_value_cansleep(acx->reset_gpio, 1);
> +	udelay(20);
> +	/* De-assert RESET */
> +	gpiod_set_value_cansleep(acx->reset_gpio, 0);
> +	msleep(11);
> +
> +	return 0;
> +}
> +
> +static int acx424akp_prepare(struct drm_panel *panel)
> +{
> +	struct acx424akp *acx = panel_to_acx424akp(panel);
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(acx->dev);
> +	const u8 mddi = 3;
> +	int ret;
> +
> +	ret = acx424akp_power_on(acx);
> +	if (ret)
> +		return ret;
> +
> +	/* Enabe tearing mode: send TE (tearing effect) at VBLANK */
> +	ret = mipi_dsi_dcs_set_tear_on(dsi,
> +				       MIPI_DSI_DCS_TEAR_MODE_VBLANK);
> +	if (ret) {
> +		DRM_DEV_ERROR(acx->dev, "failed to enable vblank TE (%d)\n",
> +			      ret);
> +		return ret;
> +	}
> +
> +	/* Set MDDI - doesn't seem to work? */
> +	ret = mipi_dsi_dcs_write(dsi, MIPI_DSI_DCS_SET_MDDI,
> +				 &mddi, sizeof(mddi));
> +	if (ret < 0) {
> +		DRM_DEV_ERROR(acx->dev, "failed to set MDDI (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	acx->bl->props.power = FB_BLANK_NORMAL;
> +
> +	return 0;
> +}
> +
> +static void acx424akp_power_off(struct acx424akp *acx)
> +{
> +	/* Assert RESET */
> +	gpiod_set_value_cansleep(acx->reset_gpio, 1);
> +	msleep(11);
> +
> +	regulator_disable(acx->supply);
> +}
> +
> +static int acx424akp_unprepare(struct drm_panel *panel)
> +{
> +	struct acx424akp *acx = panel_to_acx424akp(panel);
> +
> +	acx424akp_power_off(acx);
> +	acx->bl->props.power = FB_BLANK_POWERDOWN;
> +
> +	return 0;
> +}
> +
> +static int acx424akp_enable(struct drm_panel *panel)
> +{
> +	struct acx424akp *acx = panel_to_acx424akp(panel);
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(acx->dev);
> +	int ret;
> +
> +	/* Exit sleep mode */
> +	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> +	if (ret) {
> +		DRM_DEV_ERROR(acx->dev, "failed to exit sleep mode (%d)\n",
> +			      ret);
> +		return ret;
> +	}
> +	msleep(140);
> +
> +	ret = mipi_dsi_dcs_set_display_on(dsi);
> +	if (ret) {
> +		DRM_DEV_ERROR(acx->dev, "failed to turn display on (%d)\n",
> +			      ret);
> +		return ret;
> +	}
> +	if (acx->video_mode) {
> +		/* In video mode turn peripheral on */
> +		ret = mipi_dsi_turn_on_peripheral(dsi);
> +		if (ret) {
> +			dev_err(acx->dev, "failed to turn on peripheral\n");
> +			return ret;
> +		}
> +	}
> +
> +	acx->bl->props.power = FB_BLANK_UNBLANK;
> +
> +	return 0;
> +}
> +
> +static int acx424akp_disable(struct drm_panel *panel)
> +{
> +	struct acx424akp *acx = panel_to_acx424akp(panel);
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(acx->dev);
> +	u8 par;
> +	int ret;
> +
> +	/* Disable backlight */
> +	par = 0x00;
> +	ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY,
> +				 &par, 1);
> +	if (ret) {
> +		DRM_DEV_ERROR(acx->dev,
> +			      "failed to disable display backlight (%d)\n",
> +			      ret);
> +		return ret;
> +	}
> +
> +	ret = mipi_dsi_dcs_set_display_off(dsi);
> +	if (ret) {
> +		DRM_DEV_ERROR(acx->dev, "failed to turn display off (%d)\n",
> +			      ret);
> +		return ret;
> +	}
> +
> +	/* Enter sleep mode */
> +	ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
> +	if (ret) {
> +		DRM_DEV_ERROR(acx->dev, "failed to enter sleep mode (%d)\n",
> +			      ret);
> +		return ret;
> +	}
> +	msleep(85);
> +
> +	acx->bl->props.power = FB_BLANK_NORMAL;
> +
> +	return 0;
> +}
> +
> +static int acx424akp_get_modes(struct drm_panel *panel)
> +{
> +	struct acx424akp *acx = panel_to_acx424akp(panel);
> +	struct drm_connector *connector = panel->connector;
> +	struct drm_display_mode *mode;
> +
> +	if (acx->video_mode)
> +		mode = drm_mode_duplicate(panel->drm,
> +					  &sony_acx424akp_vid_mode);
> +	else
> +		mode = drm_mode_duplicate(panel->drm,
> +					  &sony_acx424akp_cmd_mode);
> +	if (!mode) {
> +		DRM_ERROR("bad mode or failed to add mode\n");
> +		return -EINVAL;
> +	}
> +	drm_mode_set_name(mode);
> +	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> +
> +	connector->display_info.width_mm = mode->width_mm;
> +	connector->display_info.height_mm = mode->height_mm;
> +
> +	drm_mode_probed_add(connector, mode);
> +
> +	return 1; /* Number of modes */
> +}
> +
> +static const struct drm_panel_funcs acx424akp_drm_funcs = {
> +	.disable = acx424akp_disable,
> +	.unprepare = acx424akp_unprepare,
> +	.prepare = acx424akp_prepare,
> +	.enable = acx424akp_enable,
> +	.get_modes = acx424akp_get_modes,
> +};
> +
> +static int acx424akp_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct device *dev = &dsi->dev;
> +	struct acx424akp *acx;
> +	int ret;
> +	int i;

unsigned int?

> +
> +	acx = devm_kzalloc(dev, sizeof(struct acx424akp), GFP_KERNEL);
> +	if (!acx)
> +		return -ENOMEM;
> +	acx->video_mode = !of_property_read_bool(dev->of_node,
> +						 "dsi-command-mode");
> +
> +	mipi_dsi_set_drvdata(dsi, acx);
> +	acx->dev = dev;
> +
> +	dsi->lanes = 2;
> +	dsi->format = MIPI_DSI_FMT_RGB888;
> +	dsi->hs_rate = 420160000;
> +	dsi->lp_rate = 19200000;
> +
> +	if (acx->video_mode)
> +		dsi->mode_flags =
> +			MIPI_DSI_MODE_VIDEO |
> +			MIPI_DSI_MODE_VIDEO_BURST |
> +			MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
> +	else
> +		dsi->mode_flags =
> +			MIPI_DSI_CLOCK_NON_CONTINUOUS |
> +			MIPI_DSI_MODE_EOT_PACKET;
> +
> +

Gratuituous blank line.

> +	acx->supply = devm_regulator_get(dev, "vddi");
> +	if (IS_ERR(acx->supply))
> +		return PTR_ERR(acx->supply);
> +
> +	/* This asserts RESET by default */
> +	acx->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> +						 GPIOD_OUT_HIGH);
> +	if (IS_ERR(acx->reset_gpio)) {
> +		ret = PTR_ERR(acx->reset_gpio);
> +		if (ret != -EPROBE_DEFER)
> +			DRM_DEV_ERROR(dev, "failed to request GPIO (%d)\n",
> +				      ret);
> +		return ret;
> +	}
> +
> +	drm_panel_init(&acx->panel);
> +	acx->panel.dev = dev;
> +	acx->panel.funcs = &acx424akp_drm_funcs;
> +
> +	acx->bl = devm_backlight_device_register(dev, "acx424akp", dev, acx,
> +						 &acx424akp_bl_ops, NULL);
> +	if (IS_ERR(acx->bl)) {
> +		DRM_DEV_ERROR(dev, "failed to register backlight device\n");
> +		return PTR_ERR(acx->bl);
> +	}
> +	acx->bl->props.max_brightness = 1023;
> +	acx->bl->props.brightness = 512;
> +	acx->bl->props.power = FB_BLANK_POWERDOWN;
> +
> +	ret = drm_panel_add(&acx->panel);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = acx424akp_power_on(acx);
> +	if (ret) {
> +		DRM_DEV_ERROR(dev, "failed to power up display\n");
> +		return ret;
> +	}
> +
> +	/* Read device ID */
> +	i = 0;
> +	do {
> +		ret = acx424akp_read_id(acx);
> +		if (ret)
> +			continue;
> +	} while (ret && i++ < 5);

Seems rather redundant to have both an "if (ret) continue;" and the ret
check in the while's condition. A more idiomatic way to write this would
be:

	for (i = 0; i < 5; i++) {
		ret = acx424akp_read_id(acx);
		if (!ret)
			break;
	}

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/panel: Add DT bindings for Sony ACX424AKP

[Linus Walleij]

On Mon, Sep 2, 2019 at 11:35 AM Thierry Reding <thierry.reding@gmail.com> wrote:

> > +  dsi-command-mode:
> > +     type: boolean
> > +     description:
> > +       If this is specified, the panel will be used in command
> > +       mode instead of video mode.
>
> I'm not sure there's concensus on this one yet. I think so far the
> driver decides which mode to use the panel in. Technically this falls
> into the category of configuration, so it doesn't really belong in the
> DT.

The way we've used DT is for a bit of both hardware description
and configuration I'd say, but I'm no authority on the subject.

> I vaguely recall from discussions I've had on this subject that there's
> usually no reason to do video mode if you can do command mode because
> command mode is more power efficient. This was a long time ago, so I may
> be misremembering. Perhaps you have different information on this?

No idea. I was under the impression that video mode was preferred
but I have no idea why.

Yours,
Linus Walleij
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/panel: Add driver for Sony ACX424AKP panel

[Linus Walleij]

On Mon, Sep 2, 2019 at 12:32 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> On Mon, Sep 02, 2019 at 11:06:33AM +0200, Linus Walleij wrote:

> > +     /*
> > +      * This depends on the clocking HS vs LP rate, this value
> > +      * is calculated as:
> > +      * vrefresh = (clock * 1000) / (htotal*vtotal)
> > +      */
> > +     .vrefresh = 816,
>
> That's a bit odd. My understanding is that command mode can be done in
> continuous mode or in non-continuous mode. In continuous mode you would
> typically achieve a similar refresh rate as in video mode. For non-
> continuous mode you basically have a variable refresh rate.
>
> For continuous mode you probably want 60 Hz here and for VRR there's
> probably no sensible value to set this to. In the latter case, I don't
> think it makes sense to set anything arbitrary like you have above.
> Perhaps better to just set it to 0 as a way of signalling that this is
> actually dependent on when the display hardware sends a new frame?

I guess I should set it to 60.

> Have you actually run this is command mode?

Yes that is what I am primarily using.

> If so, what's the actual
> refresh rate? Do you do on-demand updates or do you run in continuous
> mode?

I run continuous mode, so the refresh rate is essentially dependent on
the HS frequency that the host uses. For command mode we use as
fast as it can go which is 420MHz, backwards calculated to ~816Hz
updates.

I took some data from the system when running:
175608 "vblank" interrupts in 25 minutes, yielding
175608/(25*60) ~= 117 Hz so I guess that is the pace the
hardware actually recieves it and triggers new updates.

> > +     ret = mipi_dsi_dcs_read(dsi, 0xda, &id1, len);
> > +     if (ret < 0) {
> > +             DRM_DEV_ERROR(acx->dev, "could not read device ID byte 1\n");
> > +             return ret;
> > +     }
> > +     ret = mipi_dsi_dcs_read(dsi, 0xdb, &id2, len);
> > +     if (ret < 0) {
> > +             DRM_DEV_ERROR(acx->dev, "could not read device ID byte 2\n");
> > +             return ret;
> > +     }
> > +     ret = mipi_dsi_dcs_read(dsi, 0xdc, &id3, len);
> > +     if (ret < 0) {
> > +             DRM_DEV_ERROR(acx->dev, "could not read device ID byte 3\n");
> > +             return ret;
> > +     }
> > +
> > +     val = (id3 << 8) | id2;
>
> Don't you want to OR in id1 here as well? Seems a bit odd to read it but
> then not use it.

The way I have understood these "MTP IDs" is that the first byte
should just be checked for not being 0 so I will add a check like that.

The only other DSI panel doing this is panel-samsung-s6e8aa0.c function
s6e8aa0_read_mtp_id() which also reads three bytes and ignores the
first byte, also the second byte being version and the third ID matches
the data this display returns.

I'll try to make it a bit clearer and inspect the individual bytes since they
seem to have individual meanings.

> > +     struct device *dev = &dsi->dev;
> > +     struct acx424akp *acx;
> > +     int ret;
> > +     int i;
>
> unsigned int?

Just following the common idiom for integer enumerator i to be a
plain int but sure, if you prefer :)

> > +     /* Read device ID */
> > +     i = 0;
> > +     do {
> > +             ret = acx424akp_read_id(acx);
> > +             if (ret)
> > +                     continue;
> > +     } while (ret && i++ < 5);
>
> Seems rather redundant to have both an "if (ret) continue;" and the ret
> check in the while's condition. A more idiomatic way to write this would
> be:
>
>         for (i = 0; i < 5; i++) {
>                 ret = acx424akp_read_id(acx);
>                 if (!ret)
>                         break;
>         }

OK! I fix.

Yours,
Linus Walleij
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/panel: Add driver for Sony ACX424AKP panel

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 5397 bytes --]

On Mon, Sep 02, 2019 at 02:57:25PM +0200, Linus Walleij wrote:
> On Mon, Sep 2, 2019 at 12:32 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Mon, Sep 02, 2019 at 11:06:33AM +0200, Linus Walleij wrote:
> 
> > > +     /*
> > > +      * This depends on the clocking HS vs LP rate, this value
> > > +      * is calculated as:
> > > +      * vrefresh = (clock * 1000) / (htotal*vtotal)
> > > +      */
> > > +     .vrefresh = 816,
> >
> > That's a bit odd. My understanding is that command mode can be done in
> > continuous mode or in non-continuous mode. In continuous mode you would
> > typically achieve a similar refresh rate as in video mode. For non-
> > continuous mode you basically have a variable refresh rate.
> >
> > For continuous mode you probably want 60 Hz here and for VRR there's
> > probably no sensible value to set this to. In the latter case, I don't
> > think it makes sense to set anything arbitrary like you have above.
> > Perhaps better to just set it to 0 as a way of signalling that this is
> > actually dependent on when the display hardware sends a new frame?
> 
> I guess I should set it to 60.

Not sure, perhaps someone on the list has a good idea of what vrefresh
is set for VRR monitors. I think that'd be a good example to follow.

I'm also not sure your formula to compute the refresh rate is good for
command mode. You're assuming one pixel per clock cycle. I don't think
that's accurate. Shouldn't that at least depend also on the number of
lanes and the pixel format? 816 frames per second also seems a bit much
for any type of panel.

> > Have you actually run this is command mode?
> 
> Yes that is what I am primarily using.
> 
> > If so, what's the actual
> > refresh rate? Do you do on-demand updates or do you run in continuous
> > mode?
> 
> I run continuous mode, so the refresh rate is essentially dependent on
> the HS frequency that the host uses. For command mode we use as
> fast as it can go which is 420MHz, backwards calculated to ~816Hz
> updates.
> 
> I took some data from the system when running:
> 175608 "vblank" interrupts in 25 minutes, yielding
> 175608/(25*60) ~= 117 Hz so I guess that is the pace the
> hardware actually recieves it and triggers new updates.

That's a factor of almost 8, so I think there's something really wrong
in your refresh rate calculation, or you have an issue somewhere in the
code that computes the pixel clock and byte clock from the mode's clock
rate.

> 
> > > +     ret = mipi_dsi_dcs_read(dsi, 0xda, &id1, len);
> > > +     if (ret < 0) {
> > > +             DRM_DEV_ERROR(acx->dev, "could not read device ID byte 1\n");
> > > +             return ret;
> > > +     }
> > > +     ret = mipi_dsi_dcs_read(dsi, 0xdb, &id2, len);
> > > +     if (ret < 0) {
> > > +             DRM_DEV_ERROR(acx->dev, "could not read device ID byte 2\n");
> > > +             return ret;
> > > +     }
> > > +     ret = mipi_dsi_dcs_read(dsi, 0xdc, &id3, len);
> > > +     if (ret < 0) {
> > > +             DRM_DEV_ERROR(acx->dev, "could not read device ID byte 3\n");
> > > +             return ret;
> > > +     }
> > > +
> > > +     val = (id3 << 8) | id2;
> >
> > Don't you want to OR in id1 here as well? Seems a bit odd to read it but
> > then not use it.
> 
> The way I have understood these "MTP IDs" is that the first byte
> should just be checked for not being 0 so I will add a check like that.
> 
> The only other DSI panel doing this is panel-samsung-s6e8aa0.c function
> s6e8aa0_read_mtp_id() which also reads three bytes and ignores the
> first byte, also the second byte being version and the third ID matches
> the data this display returns.
> 
> I'll try to make it a bit clearer and inspect the individual bytes since they
> seem to have individual meanings.

I vaguely recall MTP IDs. It's a little funny that both S6E8AA0 and this
new driver seem to be reading the same type of ID, but using different
DCS register offsets. I had hoped that these would be somewhat
standardized. Or perhaps these are standardized as part of a larger type
of descriptor to which an offset is read from somewhere else?

> > > +     struct device *dev = &dsi->dev;
> > > +     struct acx424akp *acx;
> > > +     int ret;
> > > +     int i;
> >
> > unsigned int?
> 
> Just following the common idiom for integer enumerator i to be a
> plain int but sure, if you prefer :)

The common idiom is wrong. =)

Jokes aside, why worry about the sign if you know up front that your
values are never going to be negative? If you consistently use unsigned
types for values that can never be negative, the compiler will even be
able to point out inconsistent usage, etc.

Thierry

> > > +     /* Read device ID */
> > > +     i = 0;
> > > +     do {
> > > +             ret = acx424akp_read_id(acx);
> > > +             if (ret)
> > > +                     continue;
> > > +     } while (ret && i++ < 5);
> >
> > Seems rather redundant to have both an "if (ret) continue;" and the ret
> > check in the while's condition. A more idiomatic way to write this would
> > be:
> >
> >         for (i = 0; i < 5; i++) {
> >                 ret = acx424akp_read_id(acx);
> >                 if (!ret)
> >                         break;
> >         }
> 
> OK! I fix.
> 
> Yours,
> Linus Walleij

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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/panel: Add DT bindings for Sony ACX424AKP

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2149 bytes --]

On Mon, Sep 02, 2019 at 01:44:38PM +0200, Linus Walleij wrote:
> On Mon, Sep 2, 2019 at 11:35 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > > +  dsi-command-mode:
> > > +     type: boolean
> > > +     description:
> > > +       If this is specified, the panel will be used in command
> > > +       mode instead of video mode.
> >
> > I'm not sure there's concensus on this one yet. I think so far the
> > driver decides which mode to use the panel in. Technically this falls
> > into the category of configuration, so it doesn't really belong in the
> > DT.
> 
> The way we've used DT is for a bit of both hardware description
> and configuration I'd say, but I'm no authority on the subject.
> 
> > I vaguely recall from discussions I've had on this subject that there's
> > usually no reason to do video mode if you can do command mode because
> > command mode is more power efficient. This was a long time ago, so I may
> > be misremembering. Perhaps you have different information on this?
> 
> No idea. I was under the impression that video mode was preferred
> but I have no idea why.

Hm... my recollection is that command mode is only supported on "smart"
panels that have an internal framebuffer. So the commands actually
instruct the panel to update their internal framebuffer, which means you
can technically switch off the display engine when there are no updates.

Under those circumstances I think it'd make sense to default to command
mode if both the panel and the host support it and stick with video mode
if for example the host can't do command mode.

Or perhaps this is something that could be set from some userspace
policy maker via a connector property? A compositor for instance would
have a pretty good idea of what kind of activity is going on, so it
could at some point decide to switch between video mode and command mode
if one of them is more appropriate for the given workload.

Command mode can also be used to do partial updates, if I remember
correctly, which again would make it possible for a compositor to send
only a subset of a screen update.

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/panel: Add DT bindings for Sony ACX424AKP

[Rob Herring]

On Mon, Sep 02, 2019 at 04:40:06PM +0200, Thierry Reding wrote:
> On Mon, Sep 02, 2019 at 01:44:38PM +0200, Linus Walleij wrote:
> > On Mon, Sep 2, 2019 at 11:35 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> > 
> > > > +  dsi-command-mode:
> > > > +     type: boolean
> > > > +     description:
> > > > +       If this is specified, the panel will be used in command
> > > > +       mode instead of video mode.
> > >
> > > I'm not sure there's concensus on this one yet. I think so far the
> > > driver decides which mode to use the panel in. Technically this falls
> > > into the category of configuration, so it doesn't really belong in the
> > > DT.
> > 
> > The way we've used DT is for a bit of both hardware description
> > and configuration I'd say, but I'm no authority on the subject.

I'm okay with this if there's consensus, but it should be in a common 
doc. We probably need a dsi-commom schema with this, reg, ??.

> > 
> > > I vaguely recall from discussions I've had on this subject that there's
> > > usually no reason to do video mode if you can do command mode because
> > > command mode is more power efficient. This was a long time ago, so I may
> > > be misremembering. Perhaps you have different information on this?

30 or 60fps updates tend to be impossible because you have less b/w and 
it's async to the refresh.

I think most panels that can do both, always need command mode too for 
initialization.

> > No idea. I was under the impression that video mode was preferred
> > but I have no idea why.
> 
> Hm... my recollection is that command mode is only supported on "smart"
> panels that have an internal framebuffer. So the commands actually
> instruct the panel to update their internal framebuffer, which means you
> can technically switch off the display engine when there are no updates.
>
> Under those circumstances I think it'd make sense to default to command
> mode if both the panel and the host support it and stick with video mode
> if for example the host can't do command mode.
> 
> Or perhaps this is something that could be set from some userspace
> policy maker via a connector property? A compositor for instance would
> have a pretty good idea of what kind of activity is going on, so it
> could at some point decide to switch between video mode and command mode
> if one of them is more appropriate for the given workload.
> 
> Command mode can also be used to do partial updates, if I remember
> correctly, which again would make it possible for a compositor to send
> only a subset of a screen update.

All makes sense to me.

Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/panel: Add DT bindings for Sony ACX424AKP

[Linus Walleij]

On Mon, Sep 2, 2019 at 4:40 PM Thierry Reding <thierry.reding@gmail.com> wrote:

> Hm... my recollection is that command mode is only supported on "smart"
> panels that have an internal framebuffer. So the commands actually
> instruct the panel to update their internal framebuffer, which means you
> can technically switch off the display engine when there are no updates.

That is my understanding as well.

> Under those circumstances I think it'd make sense to default to command
> mode if both the panel and the host support it and stick with video mode
> if for example the host can't do command mode.

Makes sense to me.

Maybe we should rather have a generic setting like:
dsi-enforce-video-mode;
and the default always being command mode, if
we have consensus that command mode should always
be preferred.

> Or perhaps this is something that could be set from some userspace
> policy maker via a connector property? A compositor for instance would
> have a pretty good idea of what kind of activity is going on, so it
> could at some point decide to switch between video mode and command mode
> if one of them is more appropriate for the given workload.

It's probably more that userspace should be made aware of the
fact that we have partial updates, and if I understand correctly that
is done by dirtyrect/damage API in
drm_damage_helper.c

(Very nice overview doc at the top of the file!)

The driver enables damage by calling
drm_plane_enable_fb_damage_clips()
then in the .update() callback uses drm_atomic_helper_damage_merged()
to figure out damaged rectangles and update those with special
commands.

tinydrm/ili9225.c is a pretty clean example of a driver
actually doing this. There are not many of them.

> Command mode can also be used to do partial updates, if I remember
> correctly, which again would make it possible for a compositor to send
> only a subset of a screen update.

Indeed, who needs a blitter when you can just update the
dirtyrects.

It is a bit terse but intuitively I feel that the damage interface is what
userspace should use, then DRM should be able to infer that a
damaged rectangle can be updated on the display, and the
display controller need to announce that it can handle these
partial updates.

This requires that the display controller can generate such
complex command streams in response to
drm_atomic_helper_damage_merged()
of course, from a DSI protocol
level it is supported, but we are not writing these command
sequences with software, they are generated by hardware.

So the display controller DSI portions must be able to send
individual rectangles as well.

But this is all science fiction. What all DSI display controllers
in mailine do today (I think) is to simply scan out the whole
framebuffer continously with a vblank TE IRQ to avoid tearing.

Andrzej knows for sure what is out there I think.

Yours,
Linus Walleij
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel