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

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