2019-02-02 14:28:54

by Paweł Chmiel

[permalink] [raw]
Subject: [PATCH v3 1/2] dt-bindings: drm: panel: Add Samsung s6e63m0 panel documentation

From: Jonathan Bakker <[email protected]>

This commit adds documentation for Samsung s6e63m0 AMOLED LCD panel
driver.

Signed-off-by: Jonathan Bakker <[email protected]>
Signed-off-by: Paweł Chmiel <[email protected]>
---
Changes from v1:
- Add missing subject prefix
- Rename reset-gpio to reset-gpios
- Add link to spi properites documentation. They're required
for driver to work
- Removed delay properties, which are now hardcoded in driver
- Removed display timings, which are now hardcoded in driver
---
.../display/panel/samsung,s6e63m0.txt | 33 +++++++++++++++++++
1 file changed, 33 insertions(+)
create mode 100644 Documentation/devicetree/bindings/display/panel/samsung,s6e63m0.txt

diff --git a/Documentation/devicetree/bindings/display/panel/samsung,s6e63m0.txt b/Documentation/devicetree/bindings/display/panel/samsung,s6e63m0.txt
new file mode 100644
index 000000000000..9fb9ebeef8e4
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/samsung,s6e63m0.txt
@@ -0,0 +1,33 @@
+Samsung s6e63m0 AMOLED LCD panel
+
+Required properties:
+ - compatible: "samsung,s6e63m0"
+ - reset-gpios: GPIO spec for reset pin
+ - vdd3-supply: VDD regulator
+ - vci-supply: VCI regulator
+
+The panel must obey rules for SPI slave device specified in document [1].
+
+The device node can contain one 'port' child node with one child
+'endpoint' node, according to the bindings defined in [2]. This
+node should describe panel's video bus.
+
+[1]: Documentation/devicetree/bindings/spi/spi-bus.txt
+[2]: Documentation/devicetree/bindings/media/video-interfaces.txt
+
+Example:
+
+ s6e63m0: display@0 {
+ compatible = "samsung,s6e63m0";
+ reg = <0>;
+ reset-gpio = <&mp05 5 1>;
+ vdd3-supply = <&ldo12_reg>;
+ vci-supply = <&ldo11_reg>;
+ spi-max-frequency = <1200000>;
+
+ port {
+ lcd_ep: endpoint {
+ remote-endpoint = <&fimd_ep>;
+ };
+ };
+ };
--
2.17.1



2019-02-02 14:30:44

by Paweł Chmiel

[permalink] [raw]
Subject: [PATCH v3 2/2] drm/panel: Add driver for Samsung S6E63M0 panel

This patch adds Samsung S6E63M0 AMOLED LCD panel driver, connected over
spi. It's based on already removed, non dt s6e63m0 driver and
panel-samsung-ld9040. It can be found for example in some of Samsung
Aries based phones.

Signed-off-by: Paweł Chmiel <[email protected]>
Reviewed-by: Sam Ravnborg <[email protected]>
---
Changes from v2:
- VIDEOMODE_HELPERS is not needed in Kconfig
- Added help text to Kconfig
- Remove unneeded videomode includes/fields
- Add sentinel comment in s6e63m0_of_match struct
- Handle errors during registration of backlight device. We shouldn't
register panel if we fail to register backlight device
- Added Reviewed-by

Changes from v1:
- Correct order of Kconfig/Makefile entry
- Fix SPDX tag, so it matches value of MODULE_LICENSE
- Remove inclusion of drmP.h
- Fix code formatting
- Use DRM_DEV_ERROR/DEBUG
- Extract hardcoded values
- Remove possibility to change gamma through sysfs, leaving only one
gamma table values
- Fix reset_gpio handling, so it'll be asserted in power_on and
deasserted in power_off. Also do it before turning voltage on.
- Disable backlight and enter sleep mode in disable callback.
Previously it was done in unprepare
- Enable display and backlight in enable callback. Previously it was
done in prepare
- Hardcode display timings and delays. Previously they were readed
from device tree
- We're using SPDX, so we don't need to have license body
- Use MIPI_DCS_EXIT_SLEEP_MODE and MIPI_DCS_SET_DISPLAY_ON
- Rename MAX_GAMMA_LEVEL to NUM_GAMMA_LEVELS
- Ommit get_brightness callback
- Use backlight_enable/disable API, like it's done in other panel
drivers (for example panel-simple)
- Make set_brightness called only from backlight api, like it's done
in other panel drivers (for example panel-simple).
- Reset gpio should be set to GPIOD_OUT_HIGH. It's declared as active
low in device tree
- Don't call power_off in remove callback
---
drivers/gpu/drm/panel/Kconfig | 9 +
drivers/gpu/drm/panel/Makefile | 1 +
drivers/gpu/drm/panel/panel-samsung-s6e63m0.c | 517 ++++++++++++++++++
3 files changed, 527 insertions(+)
create mode 100644 drivers/gpu/drm/panel/panel-samsung-s6e63m0.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 3f3537719beb..45e9ab4b7857 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -158,6 +158,15 @@ config DRM_PANEL_SAMSUNG_S6E63J0X03
depends on BACKLIGHT_CLASS_DEVICE
select VIDEOMODE_HELPERS

+config DRM_PANEL_SAMSUNG_S6E63M0
+ tristate "Samsung S6E63M0 RGB/SPI panel"
+ depends on OF
+ depends on SPI
+ depends on BACKLIGHT_CLASS_DEVICE
+ help
+ Say Y here if you want to enable support for Samsung s6e63m0
+ AMOLED LCD panel.
+
config DRM_PANEL_SAMSUNG_S6E8AA0
tristate "Samsung S6E8AA0 DSI video mode panel"
depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 4396658a7996..4507a2d253ac 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) += panel-samsung-ld9040.o
obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6D16D0) += panel-samsung-s6d16d0.o
obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E3HA2) += panel-samsung-s6e3ha2.o
obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E63J0X03) += panel-samsung-s6e63j0x03.o
+obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E63M0) += panel-samsung-s6e63m0.o
obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0) += panel-samsung-s6e8aa0.o
obj-$(CONFIG_DRM_PANEL_SEIKO_43WVF1G) += panel-seiko-43wvf1g.o
obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c b/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
new file mode 100644
index 000000000000..4312aa3e4386
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
@@ -0,0 +1,517 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * S6E63M0 AMOLED LCD drm_panel driver.
+ *
+ * Copyright (C) 2019 Paweł Chmiel <[email protected]>
+ * Derived from drivers/gpu/drm/panel-samsung-ld9040.c
+ *
+ * Andrzej Hajda <[email protected]>
+ */
+
+#include <drm/drm_modes.h>
+#include <drm/drm_panel.h>
+#include <drm/drm_print.h>
+
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#include <video/mipi_display.h>
+
+/* Manufacturer Command Set */
+#define MCS_ELVSS_ON 0xb1
+#define MCS_ACL_CTRL 0xc0
+#define MCS_DISPLAY_CONDITION 0xf2
+#define MCS_ETC_CONDITION 0xf6
+#define MCS_PANEL_CONDITION 0xF8
+#define MCS_GAMMA_CTRL 0xfa
+
+#define NUM_GAMMA_LEVELS 11
+#define GAMMA_TABLE_COUNT 23
+
+#define DATA_MASK 0x100
+
+#define MAX_BRIGHTNESS (NUM_GAMMA_LEVELS - 1)
+
+/* array of gamma tables for gamma value 2.2 */
+static u8 const s6e63m0_gamma_22[NUM_GAMMA_LEVELS][GAMMA_TABLE_COUNT] = {
+ { MCS_GAMMA_CTRL, 0x00,
+ 0x18, 0x08, 0x24, 0x78, 0xEC, 0x3D, 0xC8,
+ 0xC2, 0xB6, 0xC4, 0xC7, 0xB6, 0xD5, 0xD7,
+ 0xCC, 0x00, 0x39, 0x00, 0x36, 0x00, 0x51 },
+ { MCS_GAMMA_CTRL, 0x00,
+ 0x18, 0x08, 0x24, 0x73, 0x4A, 0x3D, 0xC0,
+ 0xC2, 0xB1, 0xBB, 0xBE, 0xAC, 0xCE, 0xCF,
+ 0xC5, 0x00, 0x5D, 0x00, 0x5E, 0x00, 0x82 },
+ { MCS_GAMMA_CTRL, 0x00,
+ 0x18, 0x08, 0x24, 0x70, 0x51, 0x3E, 0xBF,
+ 0xC1, 0xAF, 0xB9, 0xBC, 0xAB, 0xCC, 0xCC,
+ 0xC2, 0x00, 0x65, 0x00, 0x67, 0x00, 0x8D },
+ { MCS_GAMMA_CTRL, 0x00,
+ 0x18, 0x08, 0x24, 0x6C, 0x54, 0x3A, 0xBC,
+ 0xBF, 0xAC, 0xB7, 0xBB, 0xA9, 0xC9, 0xC9,
+ 0xBE, 0x00, 0x71, 0x00, 0x73, 0x00, 0x9E },
+ { MCS_GAMMA_CTRL, 0x00,
+ 0x18, 0x08, 0x24, 0x69, 0x54, 0x37, 0xBB,
+ 0xBE, 0xAC, 0xB4, 0xB7, 0xA6, 0xC7, 0xC8,
+ 0xBC, 0x00, 0x7B, 0x00, 0x7E, 0x00, 0xAB },
+ { MCS_GAMMA_CTRL, 0x00,
+ 0x18, 0x08, 0x24, 0x66, 0x55, 0x34, 0xBA,
+ 0xBD, 0xAB, 0xB1, 0xB5, 0xA3, 0xC5, 0xC6,
+ 0xB9, 0x00, 0x85, 0x00, 0x88, 0x00, 0xBA },
+ { MCS_GAMMA_CTRL, 0x00,
+ 0x18, 0x08, 0x24, 0x63, 0x53, 0x31, 0xB8,
+ 0xBC, 0xA9, 0xB0, 0xB5, 0xA2, 0xC4, 0xC4,
+ 0xB8, 0x00, 0x8B, 0x00, 0x8E, 0x00, 0xC2 },
+ { MCS_GAMMA_CTRL, 0x00,
+ 0x18, 0x08, 0x24, 0x62, 0x54, 0x30, 0xB9,
+ 0xBB, 0xA9, 0xB0, 0xB3, 0xA1, 0xC1, 0xC3,
+ 0xB7, 0x00, 0x91, 0x00, 0x95, 0x00, 0xDA },
+ { MCS_GAMMA_CTRL, 0x00,
+ 0x18, 0x08, 0x24, 0x66, 0x58, 0x34, 0xB6,
+ 0xBA, 0xA7, 0xAF, 0xB3, 0xA0, 0xC1, 0xC2,
+ 0xB7, 0x00, 0x97, 0x00, 0x9A, 0x00, 0xD1 },
+ { MCS_GAMMA_CTRL, 0x00,
+ 0x18, 0x08, 0x24, 0x64, 0x56, 0x33, 0xB6,
+ 0xBA, 0xA8, 0xAC, 0xB1, 0x9D, 0xC1, 0xC1,
+ 0xB7, 0x00, 0x9C, 0x00, 0x9F, 0x00, 0xD6 },
+ { MCS_GAMMA_CTRL, 0x00,
+ 0x18, 0x08, 0x24, 0x5f, 0x50, 0x2d, 0xB6,
+ 0xB9, 0xA7, 0xAd, 0xB1, 0x9f, 0xbe, 0xC0,
+ 0xB5, 0x00, 0xa0, 0x00, 0xa4, 0x00, 0xdb },
+};
+
+struct s6e63m0 {
+ struct device *dev;
+ struct drm_panel panel;
+ struct backlight_device *bl_dev;
+
+ struct regulator_bulk_data supplies[2];
+ struct gpio_desc *reset_gpio;
+
+ bool prepared;
+ bool enabled;
+
+ /*
+ * This field is tested by functions directly accessing bus before
+ * transfer, transfer is skipped if it is set. In case of transfer
+ * failure or unexpected response the field is set to error value.
+ * Such construct allows to eliminate many checks in higher level
+ * functions.
+ */
+ int error;
+};
+
+static const struct drm_display_mode default_mode = {
+ .clock = 25628,
+ .hdisplay = 480,
+ .hsync_start = 480 + 16,
+ .hsync_end = 480 + 16 + 2,
+ .htotal = 480 + 16 + 2 + 16,
+ .vdisplay = 800,
+ .vsync_start = 800 + 28,
+ .vsync_end = 800 + 28 + 2,
+ .vtotal = 800 + 28 + 2 + 1,
+ .vrefresh = 60,
+ .width_mm = 53,
+ .height_mm = 89,
+ .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
+};
+
+static inline struct s6e63m0 *panel_to_s6e63m0(struct drm_panel *panel)
+{
+ return container_of(panel, struct s6e63m0, panel);
+}
+
+static int s6e63m0_clear_error(struct s6e63m0 *ctx)
+{
+ int ret = ctx->error;
+
+ ctx->error = 0;
+ return ret;
+}
+
+static int s6e63m0_spi_write_word(struct s6e63m0 *ctx, u16 data)
+{
+ struct spi_device *spi = to_spi_device(ctx->dev);
+ struct spi_transfer xfer = {
+ .len = 2,
+ .tx_buf = &data,
+ };
+ struct spi_message msg;
+
+ spi_message_init(&msg);
+ spi_message_add_tail(&xfer, &msg);
+
+ return spi_sync(spi, &msg);
+}
+
+static void s6e63m0_dcs_write(struct s6e63m0 *ctx, const u8 *data, size_t len)
+{
+ int ret = 0;
+
+ if (ctx->error < 0 || len == 0)
+ return;
+
+ DRM_DEV_DEBUG(ctx->dev, "writing dcs seq: %*ph\n", (int)len, data);
+ ret = s6e63m0_spi_write_word(ctx, *data);
+
+ while (!ret && --len) {
+ ++data;
+ ret = s6e63m0_spi_write_word(ctx, *data | DATA_MASK);
+ }
+
+ if (ret) {
+ DRM_DEV_ERROR(ctx->dev, "error %d writing dcs seq: %*ph\n", ret,
+ (int)len, data);
+ ctx->error = ret;
+ }
+
+ usleep_range(300, 310);
+}
+
+#define s6e63m0_dcs_write_seq_static(ctx, seq ...) \
+ ({ \
+ static const u8 d[] = { seq }; \
+ s6e63m0_dcs_write(ctx, d, ARRAY_SIZE(d)); \
+ })
+
+static void s6e63m0_brightness_set(struct s6e63m0 *ctx)
+{
+ int brightness = ctx->bl_dev->props.brightness;
+
+ /* disable and set new gamma */
+ s6e63m0_dcs_write(ctx, s6e63m0_gamma_22[brightness],
+ ARRAY_SIZE(s6e63m0_gamma_22[brightness]));
+
+ /* update gamma table. */
+ s6e63m0_dcs_write_seq_static(ctx, MCS_GAMMA_CTRL, 0x01);
+}
+
+static void s6e63m0_init(struct s6e63m0 *ctx)
+{
+ s6e63m0_dcs_write_seq_static(ctx, MCS_PANEL_CONDITION,
+ 0x01, 0x27, 0x27, 0x07, 0x07, 0x54, 0x9f,
+ 0x63, 0x86, 0x1a, 0x33, 0x0d, 0x00, 0x00);
+
+ s6e63m0_dcs_write_seq_static(ctx, MCS_DISPLAY_CONDITION,
+ 0x02, 0x03, 0x1c, 0x10, 0x10);
+ s6e63m0_dcs_write_seq_static(ctx, 0xf7,
+ 0x03, 0x00, 0x00);
+
+ s6e63m0_dcs_write_seq_static(ctx, MCS_GAMMA_CTRL,
+ 0x00, 0x18, 0x08, 0x24, 0x64, 0x56, 0x33,
+ 0xb6, 0xba, 0xa8, 0xac, 0xb1, 0x9d, 0xc1,
+ 0xc1, 0xb7, 0x00, 0x9c, 0x00, 0x9f, 0x00,
+ 0xd6);
+ s6e63m0_dcs_write_seq_static(ctx, MCS_GAMMA_CTRL,
+ 0x01);
+
+ s6e63m0_dcs_write_seq_static(ctx, MCS_ETC_CONDITION,
+ 0x00, 0x8c, 0x07);
+ s6e63m0_dcs_write_seq_static(ctx, 0xb3,
+ 0xc);
+
+ s6e63m0_dcs_write_seq_static(ctx, 0xb5,
+ 0x2c, 0x12, 0x0c, 0x0a, 0x10, 0x0e, 0x17,
+ 0x13, 0x1f, 0x1a, 0x2a, 0x24, 0x1f, 0x1b,
+ 0x1a, 0x17, 0x2b, 0x26, 0x22, 0x20, 0x3a,
+ 0x34, 0x30, 0x2c, 0x29, 0x26, 0x25, 0x23,
+ 0x21, 0x20, 0x1e, 0x1e);
+
+ s6e63m0_dcs_write_seq_static(ctx, 0xb6,
+ 0x00, 0x00, 0x11, 0x22, 0x33, 0x44, 0x44,
+ 0x44, 0x55, 0x55, 0x66, 0x66, 0x66, 0x66,
+ 0x66, 0x66);
+
+ s6e63m0_dcs_write_seq_static(ctx, 0xb7,
+ 0x2c, 0x12, 0x0c, 0x0a, 0x10, 0x0e, 0x17,
+ 0x13, 0x1f, 0x1a, 0x2a, 0x24, 0x1f, 0x1b,
+ 0x1a, 0x17, 0x2b, 0x26, 0x22, 0x20, 0x3a,
+ 0x34, 0x30, 0x2c, 0x29, 0x26, 0x25, 0x23,
+ 0x21, 0x20, 0x1e, 0x1e, 0x00, 0x00, 0x11,
+ 0x22, 0x33, 0x44, 0x44, 0x44, 0x55, 0x55,
+ 0x66, 0x66, 0x66, 0x66, 0x66, 0x66);
+
+ s6e63m0_dcs_write_seq_static(ctx, 0xb9,
+ 0x2c, 0x12, 0x0c, 0x0a, 0x10, 0x0e, 0x17,
+ 0x13, 0x1f, 0x1a, 0x2a, 0x24, 0x1f, 0x1b,
+ 0x1a, 0x17, 0x2b, 0x26, 0x22, 0x20, 0x3a,
+ 0x34, 0x30, 0x2c, 0x29, 0x26, 0x25, 0x23,
+ 0x21, 0x20, 0x1e, 0x1e);
+
+ s6e63m0_dcs_write_seq_static(ctx, 0xba,
+ 0x00, 0x00, 0x11, 0x22, 0x33, 0x44, 0x44,
+ 0x44, 0x55, 0x55, 0x66, 0x66, 0x66, 0x66,
+ 0x66, 0x66);
+
+ s6e63m0_dcs_write_seq_static(ctx, 0xc1,
+ 0x4d, 0x96, 0x1d, 0x00, 0x00, 0x01, 0xdf,
+ 0x00, 0x00, 0x03, 0x1f, 0x00, 0x00, 0x00,
+ 0x00, 0x00, 0x00, 0x00, 0x00, 0x03, 0x06,
+ 0x09, 0x0d, 0x0f, 0x12, 0x15, 0x18);
+
+ s6e63m0_dcs_write_seq_static(ctx, 0xb2,
+ 0x10, 0x10, 0x0b, 0x05);
+
+ s6e63m0_dcs_write_seq_static(ctx, MCS_ACL_CTRL,
+ 0x01);
+
+ s6e63m0_dcs_write_seq_static(ctx, MCS_ELVSS_ON,
+ 0x0b);
+
+ s6e63m0_dcs_write_seq_static(ctx, MIPI_DCS_EXIT_SLEEP_MODE);
+}
+
+static int s6e63m0_power_on(struct s6e63m0 *ctx)
+{
+ int ret;
+
+ gpiod_set_value(ctx->reset_gpio, 0);
+ msleep(120);
+
+ ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
+ if (ret < 0)
+ return ret;
+
+ msleep(25);
+
+ return 0;
+}
+
+static int s6e63m0_power_off(struct s6e63m0 *ctx)
+{
+ int ret;
+
+ ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
+ if (ret < 0)
+ return ret;
+
+ gpiod_set_value(ctx->reset_gpio, 1);
+ msleep(120);
+
+ return 0;
+}
+
+static int s6e63m0_disable(struct drm_panel *panel)
+{
+ struct s6e63m0 *ctx = panel_to_s6e63m0(panel);
+
+ if (!ctx->enabled)
+ return 0;
+
+ backlight_disable(ctx->bl_dev);
+
+ s6e63m0_dcs_write_seq_static(ctx, MIPI_DCS_ENTER_SLEEP_MODE);
+ msleep(200);
+
+ ctx->enabled = false;
+
+ return 0;
+}
+
+static int s6e63m0_unprepare(struct drm_panel *panel)
+{
+ struct s6e63m0 *ctx = panel_to_s6e63m0(panel);
+ int ret;
+
+ if (!ctx->prepared)
+ return 0;
+
+ s6e63m0_clear_error(ctx);
+
+ ret = s6e63m0_power_off(ctx);
+ if (ret < 0)
+ return ret;
+
+ ctx->prepared = false;
+
+ return 0;
+}
+
+static int s6e63m0_prepare(struct drm_panel *panel)
+{
+ struct s6e63m0 *ctx = panel_to_s6e63m0(panel);
+ int ret;
+
+ if (ctx->prepared)
+ return 0;
+
+ ret = s6e63m0_power_on(ctx);
+ if (ret < 0)
+ return ret;
+
+ s6e63m0_init(ctx);
+
+ ret = s6e63m0_clear_error(ctx);
+
+ if (ret < 0)
+ s6e63m0_unprepare(panel);
+
+ ctx->prepared = true;
+
+ return ret;
+}
+
+static int s6e63m0_enable(struct drm_panel *panel)
+{
+ struct s6e63m0 *ctx = panel_to_s6e63m0(panel);
+
+ if (ctx->enabled)
+ return 0;
+
+ s6e63m0_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_ON);
+
+ backlight_enable(ctx->bl_dev);
+
+ ctx->enabled = true;
+
+ return 0;
+}
+
+static int s6e63m0_get_modes(struct drm_panel *panel)
+{
+ struct drm_connector *connector = panel->connector;
+ struct drm_display_mode *mode;
+
+ mode = drm_mode_duplicate(panel->drm, &default_mode);
+ if (!mode) {
+ DRM_ERROR("failed to add mode %ux%ux@%u\n",
+ default_mode.hdisplay, default_mode.vdisplay,
+ default_mode.vrefresh);
+ return -ENOMEM;
+ }
+
+ drm_mode_set_name(mode);
+
+ mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
+ drm_mode_probed_add(connector, mode);
+
+ return 1;
+}
+
+static const struct drm_panel_funcs s6e63m0_drm_funcs = {
+ .disable = s6e63m0_disable,
+ .unprepare = s6e63m0_unprepare,
+ .prepare = s6e63m0_prepare,
+ .enable = s6e63m0_enable,
+ .get_modes = s6e63m0_get_modes,
+};
+
+static int s6e63m0_set_brightness(struct backlight_device *bd)
+{
+ struct s6e63m0 *ctx = bl_get_data(bd);
+
+ s6e63m0_brightness_set(ctx);
+
+ return s6e63m0_clear_error(ctx);
+}
+
+static const struct backlight_ops s6e63m0_backlight_ops = {
+ .update_status = s6e63m0_set_brightness,
+};
+
+static int s6e63m0_backlight_register(struct s6e63m0 *ctx)
+{
+ struct backlight_properties props = {
+ .type = BACKLIGHT_RAW,
+ .brightness = MAX_BRIGHTNESS,
+ .max_brightness = MAX_BRIGHTNESS
+ };
+ struct device *dev = ctx->dev;
+ int ret = 0;
+
+ ctx->bl_dev = devm_backlight_device_register(dev, "panel", dev, ctx,
+ &s6e63m0_backlight_ops,
+ &props);
+ if (IS_ERR(ctx->bl_dev)) {
+ ret = PTR_ERR(ctx->bl_dev);
+ DRM_DEV_ERROR(dev, "error registering backlight device (%d)\n",
+ ret);
+ }
+
+ return ret;
+}
+
+static int s6e63m0_probe(struct spi_device *spi)
+{
+ struct device *dev = &spi->dev;
+ struct s6e63m0 *ctx;
+ int ret;
+
+ ctx = devm_kzalloc(dev, sizeof(struct s6e63m0), GFP_KERNEL);
+ if (!ctx)
+ return -ENOMEM;
+
+ spi_set_drvdata(spi, ctx);
+
+ ctx->dev = dev;
+ ctx->enabled = false;
+ ctx->prepared = false;
+
+ ctx->supplies[0].supply = "vdd3";
+ ctx->supplies[1].supply = "vci";
+ ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ctx->supplies),
+ ctx->supplies);
+ if (ret < 0) {
+ DRM_DEV_ERROR(dev, "failed to get regulators: %d\n", ret);
+ return ret;
+ }
+
+ ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+ if (IS_ERR(ctx->reset_gpio)) {
+ DRM_DEV_ERROR(dev, "cannot get reset-gpios %ld\n",
+ PTR_ERR(ctx->reset_gpio));
+ return PTR_ERR(ctx->reset_gpio);
+ }
+
+ spi->bits_per_word = 9;
+ spi->mode = SPI_MODE_3;
+ ret = spi_setup(spi);
+ if (ret < 0) {
+ DRM_DEV_ERROR(dev, "spi setup failed.\n");
+ return ret;
+ }
+
+ drm_panel_init(&ctx->panel);
+ ctx->panel.dev = dev;
+ ctx->panel.funcs = &s6e63m0_drm_funcs;
+
+ ret = s6e63m0_backlight_register(ctx);
+ if (ret < 0)
+ return ret;
+
+ return drm_panel_add(&ctx->panel);
+}
+
+static int s6e63m0_remove(struct spi_device *spi)
+{
+ struct s6e63m0 *ctx = spi_get_drvdata(spi);
+
+ drm_panel_remove(&ctx->panel);
+
+ return 0;
+}
+
+static const struct of_device_id s6e63m0_of_match[] = {
+ { .compatible = "samsung,s6e63m0" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, s6e63m0_of_match);
+
+static struct spi_driver s6e63m0_driver = {
+ .probe = s6e63m0_probe,
+ .remove = s6e63m0_remove,
+ .driver = {
+ .name = "panel-samsung-s6e63m0",
+ .of_match_table = s6e63m0_of_match,
+ },
+};
+module_spi_driver(s6e63m0_driver);
+
+MODULE_AUTHOR("Paweł Chmiel <[email protected]>");
+MODULE_DESCRIPTION("s6e63m0 LCD Driver");
+MODULE_LICENSE("GPL v2");
--
2.17.1


2019-02-11 15:37:52

by Andrzej Hajda

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] drm/panel: Add driver for Samsung S6E63M0 panel

On 02.02.2019 15:27, Paweł Chmiel wrote:
> This patch adds Samsung S6E63M0 AMOLED LCD panel driver, connected over
> spi. It's based on already removed, non dt s6e63m0 driver and
> panel-samsung-ld9040. It can be found for example in some of Samsung
> Aries based phones.
>
> Signed-off-by: Paweł Chmiel <[email protected]>
> Reviewed-by: Sam Ravnborg <[email protected]>
> ---
> Changes from v2:
> - VIDEOMODE_HELPERS is not needed in Kconfig
> - Added help text to Kconfig
> - Remove unneeded videomode includes/fields
> - Add sentinel comment in s6e63m0_of_match struct
> - Handle errors during registration of backlight device. We shouldn't
> register panel if we fail to register backlight device
> - Added Reviewed-by
>
> Changes from v1:
> - Correct order of Kconfig/Makefile entry
> - Fix SPDX tag, so it matches value of MODULE_LICENSE
> - Remove inclusion of drmP.h
> - Fix code formatting
> - Use DRM_DEV_ERROR/DEBUG
> - Extract hardcoded values
> - Remove possibility to change gamma through sysfs, leaving only one
> gamma table values
> - Fix reset_gpio handling, so it'll be asserted in power_on and
> deasserted in power_off. Also do it before turning voltage on.
> - Disable backlight and enter sleep mode in disable callback.
> Previously it was done in unprepare
> - Enable display and backlight in enable callback. Previously it was
> done in prepare
> - Hardcode display timings and delays. Previously they were readed
> from device tree
> - We're using SPDX, so we don't need to have license body
> - Use MIPI_DCS_EXIT_SLEEP_MODE and MIPI_DCS_SET_DISPLAY_ON
> - Rename MAX_GAMMA_LEVEL to NUM_GAMMA_LEVELS
> - Ommit get_brightness callback
> - Use backlight_enable/disable API, like it's done in other panel
> drivers (for example panel-simple)
> - Make set_brightness called only from backlight api, like it's done
> in other panel drivers (for example panel-simple).
> - Reset gpio should be set to GPIOD_OUT_HIGH. It's declared as active
> low in device tree
> - Don't call power_off in remove callback
> ---
> drivers/gpu/drm/panel/Kconfig | 9 +
> drivers/gpu/drm/panel/Makefile | 1 +
> drivers/gpu/drm/panel/panel-samsung-s6e63m0.c | 517 ++++++++++++++++++
> 3 files changed, 527 insertions(+)
> create mode 100644 drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
>
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 3f3537719beb..45e9ab4b7857 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -158,6 +158,15 @@ config DRM_PANEL_SAMSUNG_S6E63J0X03
> depends on BACKLIGHT_CLASS_DEVICE
> select VIDEOMODE_HELPERS
>
> +config DRM_PANEL_SAMSUNG_S6E63M0
> + tristate "Samsung S6E63M0 RGB/SPI panel"
> + depends on OF
> + depends on SPI
> + depends on BACKLIGHT_CLASS_DEVICE
> + help
> + Say Y here if you want to enable support for Samsung s6e63m0
> + AMOLED LCD panel.
> +
> config DRM_PANEL_SAMSUNG_S6E8AA0
> tristate "Samsung S6E8AA0 DSI video mode panel"
> depends on OF
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index 4396658a7996..4507a2d253ac 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) += panel-samsung-ld9040.o
> obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6D16D0) += panel-samsung-s6d16d0.o
> obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E3HA2) += panel-samsung-s6e3ha2.o
> obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E63J0X03) += panel-samsung-s6e63j0x03.o
> +obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E63M0) += panel-samsung-s6e63m0.o
> obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0) += panel-samsung-s6e8aa0.o
> obj-$(CONFIG_DRM_PANEL_SEIKO_43WVF1G) += panel-seiko-43wvf1g.o
> obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
> diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c b/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
> new file mode 100644
> index 000000000000..4312aa3e4386
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
> @@ -0,0 +1,517 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * S6E63M0 AMOLED LCD drm_panel driver.
> + *
> + * Copyright (C) 2019 Paweł Chmiel <[email protected]>
> + * Derived from drivers/gpu/drm/panel-samsung-ld9040.c
> + *
> + * Andrzej Hajda <[email protected]>
> + */
> +
> +#include <drm/drm_modes.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_print.h>
> +
> +#include <linux/backlight.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#include <video/mipi_display.h>
> +
> +/* Manufacturer Command Set */
> +#define MCS_ELVSS_ON 0xb1
> +#define MCS_ACL_CTRL 0xc0
> +#define MCS_DISPLAY_CONDITION 0xf2
> +#define MCS_ETC_CONDITION 0xf6
> +#define MCS_PANEL_CONDITION 0xF8
> +#define MCS_GAMMA_CTRL 0xfa
> +
> +#define NUM_GAMMA_LEVELS 11
> +#define GAMMA_TABLE_COUNT 23
> +
> +#define DATA_MASK 0x100
> +
> +#define MAX_BRIGHTNESS (NUM_GAMMA_LEVELS - 1)
> +
> +/* array of gamma tables for gamma value 2.2 */
> +static u8 const s6e63m0_gamma_22[NUM_GAMMA_LEVELS][GAMMA_TABLE_COUNT] = {
> + { MCS_GAMMA_CTRL, 0x00,
> + 0x18, 0x08, 0x24, 0x78, 0xEC, 0x3D, 0xC8,
> + 0xC2, 0xB6, 0xC4, 0xC7, 0xB6, 0xD5, 0xD7,
> + 0xCC, 0x00, 0x39, 0x00, 0x36, 0x00, 0x51 },
> + { MCS_GAMMA_CTRL, 0x00,
> + 0x18, 0x08, 0x24, 0x73, 0x4A, 0x3D, 0xC0,
> + 0xC2, 0xB1, 0xBB, 0xBE, 0xAC, 0xCE, 0xCF,
> + 0xC5, 0x00, 0x5D, 0x00, 0x5E, 0x00, 0x82 },
> + { MCS_GAMMA_CTRL, 0x00,
> + 0x18, 0x08, 0x24, 0x70, 0x51, 0x3E, 0xBF,
> + 0xC1, 0xAF, 0xB9, 0xBC, 0xAB, 0xCC, 0xCC,
> + 0xC2, 0x00, 0x65, 0x00, 0x67, 0x00, 0x8D },
> + { MCS_GAMMA_CTRL, 0x00,
> + 0x18, 0x08, 0x24, 0x6C, 0x54, 0x3A, 0xBC,
> + 0xBF, 0xAC, 0xB7, 0xBB, 0xA9, 0xC9, 0xC9,
> + 0xBE, 0x00, 0x71, 0x00, 0x73, 0x00, 0x9E },
> + { MCS_GAMMA_CTRL, 0x00,
> + 0x18, 0x08, 0x24, 0x69, 0x54, 0x37, 0xBB,
> + 0xBE, 0xAC, 0xB4, 0xB7, 0xA6, 0xC7, 0xC8,
> + 0xBC, 0x00, 0x7B, 0x00, 0x7E, 0x00, 0xAB },
> + { MCS_GAMMA_CTRL, 0x00,
> + 0x18, 0x08, 0x24, 0x66, 0x55, 0x34, 0xBA,
> + 0xBD, 0xAB, 0xB1, 0xB5, 0xA3, 0xC5, 0xC6,
> + 0xB9, 0x00, 0x85, 0x00, 0x88, 0x00, 0xBA },
> + { MCS_GAMMA_CTRL, 0x00,
> + 0x18, 0x08, 0x24, 0x63, 0x53, 0x31, 0xB8,
> + 0xBC, 0xA9, 0xB0, 0xB5, 0xA2, 0xC4, 0xC4,
> + 0xB8, 0x00, 0x8B, 0x00, 0x8E, 0x00, 0xC2 },
> + { MCS_GAMMA_CTRL, 0x00,
> + 0x18, 0x08, 0x24, 0x62, 0x54, 0x30, 0xB9,
> + 0xBB, 0xA9, 0xB0, 0xB3, 0xA1, 0xC1, 0xC3,
> + 0xB7, 0x00, 0x91, 0x00, 0x95, 0x00, 0xDA },
> + { MCS_GAMMA_CTRL, 0x00,
> + 0x18, 0x08, 0x24, 0x66, 0x58, 0x34, 0xB6,
> + 0xBA, 0xA7, 0xAF, 0xB3, 0xA0, 0xC1, 0xC2,
> + 0xB7, 0x00, 0x97, 0x00, 0x9A, 0x00, 0xD1 },
> + { MCS_GAMMA_CTRL, 0x00,
> + 0x18, 0x08, 0x24, 0x64, 0x56, 0x33, 0xB6,
> + 0xBA, 0xA8, 0xAC, 0xB1, 0x9D, 0xC1, 0xC1,
> + 0xB7, 0x00, 0x9C, 0x00, 0x9F, 0x00, 0xD6 },
> + { MCS_GAMMA_CTRL, 0x00,
> + 0x18, 0x08, 0x24, 0x5f, 0x50, 0x2d, 0xB6,
> + 0xB9, 0xA7, 0xAd, 0xB1, 0x9f, 0xbe, 0xC0,
> + 0xB5, 0x00, 0xa0, 0x00, 0xa4, 0x00, 0xdb },
> +};
> +
> +struct s6e63m0 {
> + struct device *dev;
> + struct drm_panel panel;
> + struct backlight_device *bl_dev;
> +
> + struct regulator_bulk_data supplies[2];
> + struct gpio_desc *reset_gpio;
> +
> + bool prepared;
> + bool enabled;
> +
> + /*
> + * This field is tested by functions directly accessing bus before
> + * transfer, transfer is skipped if it is set. In case of transfer
> + * failure or unexpected response the field is set to error value.
> + * Such construct allows to eliminate many checks in higher level
> + * functions.
> + */
> + int error;
> +};
> +
> +static const struct drm_display_mode default_mode = {
> + .clock = 25628,
> + .hdisplay = 480,
> + .hsync_start = 480 + 16,
> + .hsync_end = 480 + 16 + 2,
> + .htotal = 480 + 16 + 2 + 16,
> + .vdisplay = 800,
> + .vsync_start = 800 + 28,
> + .vsync_end = 800 + 28 + 2,
> + .vtotal = 800 + 28 + 2 + 1,
> + .vrefresh = 60,
> + .width_mm = 53,
> + .height_mm = 89,
> + .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
> +};
> +
> +static inline struct s6e63m0 *panel_to_s6e63m0(struct drm_panel *panel)
> +{
> + return container_of(panel, struct s6e63m0, panel);
> +}
> +
> +static int s6e63m0_clear_error(struct s6e63m0 *ctx)
> +{
> + int ret = ctx->error;
> +
> + ctx->error = 0;
> + return ret;
> +}
> +
> +static int s6e63m0_spi_write_word(struct s6e63m0 *ctx, u16 data)
> +{
> + struct spi_device *spi = to_spi_device(ctx->dev);
> + struct spi_transfer xfer = {
> + .len = 2,
> + .tx_buf = &data,
> + };
> + struct spi_message msg;
> +
> + spi_message_init(&msg);
> + spi_message_add_tail(&xfer, &msg);
> +
> + return spi_sync(spi, &msg);
> +}
> +
> +static void s6e63m0_dcs_write(struct s6e63m0 *ctx, const u8 *data, size_t len)
> +{
> + int ret = 0;
> +
> + if (ctx->error < 0 || len == 0)
> + return;
> +
> + DRM_DEV_DEBUG(ctx->dev, "writing dcs seq: %*ph\n", (int)len, data);
> + ret = s6e63m0_spi_write_word(ctx, *data);
> +
> + while (!ret && --len) {
> + ++data;
> + ret = s6e63m0_spi_write_word(ctx, *data | DATA_MASK);
> + }
> +
> + if (ret) {
> + DRM_DEV_ERROR(ctx->dev, "error %d writing dcs seq: %*ph\n", ret,
> + (int)len, data);
> + ctx->error = ret;
> + }
> +
> + usleep_range(300, 310);
> +}
> +
> +#define s6e63m0_dcs_write_seq_static(ctx, seq ...) \
> + ({ \
> + static const u8 d[] = { seq }; \
> + s6e63m0_dcs_write(ctx, d, ARRAY_SIZE(d)); \
> + })
> +
> +static void s6e63m0_brightness_set(struct s6e63m0 *ctx)
> +{
> + int brightness = ctx->bl_dev->props.brightness;
> +
> + /* disable and set new gamma */
> + s6e63m0_dcs_write(ctx, s6e63m0_gamma_22[brightness],
> + ARRAY_SIZE(s6e63m0_gamma_22[brightness]));
> +
> + /* update gamma table. */
> + s6e63m0_dcs_write_seq_static(ctx, MCS_GAMMA_CTRL, 0x01);
> +}


I guess you can squash this callback into s6e63m0_set_brightness.
Moreover you performs transfer here unconditionally, regardless of panel
power state, it will fail when drm device is disabled (using echo 255
>/sys/class/backlight/...) - panel is also disabled - making transfer to
disabled panel by panel driver looks odd, moreover it could be little
bit racy. But in practice it could work.

Moreover I do not see usage of backlight_properties.power,
backlight_properties.state, but I do not understand how exactly
backlight interface should work, if someone can point out to proper
docs/usage of backlight API I will be glad.


> +
> +static void s6e63m0_init(struct s6e63m0 *ctx)
> +{
> + s6e63m0_dcs_write_seq_static(ctx, MCS_PANEL_CONDITION,
> + 0x01, 0x27, 0x27, 0x07, 0x07, 0x54, 0x9f,
> + 0x63, 0x86, 0x1a, 0x33, 0x0d, 0x00, 0x00);
> +
> + s6e63m0_dcs_write_seq_static(ctx, MCS_DISPLAY_CONDITION,
> + 0x02, 0x03, 0x1c, 0x10, 0x10);
> + s6e63m0_dcs_write_seq_static(ctx, 0xf7,
> + 0x03, 0x00, 0x00);
> +
> + s6e63m0_dcs_write_seq_static(ctx, MCS_GAMMA_CTRL,
> + 0x00, 0x18, 0x08, 0x24, 0x64, 0x56, 0x33,
> + 0xb6, 0xba, 0xa8, 0xac, 0xb1, 0x9d, 0xc1,
> + 0xc1, 0xb7, 0x00, 0x9c, 0x00, 0x9f, 0x00,
> + 0xd6);
> + s6e63m0_dcs_write_seq_static(ctx, MCS_GAMMA_CTRL,
> + 0x01);
> +
> + s6e63m0_dcs_write_seq_static(ctx, MCS_ETC_CONDITION,
> + 0x00, 0x8c, 0x07);
> + s6e63m0_dcs_write_seq_static(ctx, 0xb3,
> + 0xc);
> +
> + s6e63m0_dcs_write_seq_static(ctx, 0xb5,
> + 0x2c, 0x12, 0x0c, 0x0a, 0x10, 0x0e, 0x17,
> + 0x13, 0x1f, 0x1a, 0x2a, 0x24, 0x1f, 0x1b,
> + 0x1a, 0x17, 0x2b, 0x26, 0x22, 0x20, 0x3a,
> + 0x34, 0x30, 0x2c, 0x29, 0x26, 0x25, 0x23,
> + 0x21, 0x20, 0x1e, 0x1e);
> +
> + s6e63m0_dcs_write_seq_static(ctx, 0xb6,
> + 0x00, 0x00, 0x11, 0x22, 0x33, 0x44, 0x44,
> + 0x44, 0x55, 0x55, 0x66, 0x66, 0x66, 0x66,
> + 0x66, 0x66);
> +
> + s6e63m0_dcs_write_seq_static(ctx, 0xb7,
> + 0x2c, 0x12, 0x0c, 0x0a, 0x10, 0x0e, 0x17,
> + 0x13, 0x1f, 0x1a, 0x2a, 0x24, 0x1f, 0x1b,
> + 0x1a, 0x17, 0x2b, 0x26, 0x22, 0x20, 0x3a,
> + 0x34, 0x30, 0x2c, 0x29, 0x26, 0x25, 0x23,
> + 0x21, 0x20, 0x1e, 0x1e, 0x00, 0x00, 0x11,
> + 0x22, 0x33, 0x44, 0x44, 0x44, 0x55, 0x55,
> + 0x66, 0x66, 0x66, 0x66, 0x66, 0x66);
> +
> + s6e63m0_dcs_write_seq_static(ctx, 0xb9,
> + 0x2c, 0x12, 0x0c, 0x0a, 0x10, 0x0e, 0x17,
> + 0x13, 0x1f, 0x1a, 0x2a, 0x24, 0x1f, 0x1b,
> + 0x1a, 0x17, 0x2b, 0x26, 0x22, 0x20, 0x3a,
> + 0x34, 0x30, 0x2c, 0x29, 0x26, 0x25, 0x23,
> + 0x21, 0x20, 0x1e, 0x1e);
> +
> + s6e63m0_dcs_write_seq_static(ctx, 0xba,
> + 0x00, 0x00, 0x11, 0x22, 0x33, 0x44, 0x44,
> + 0x44, 0x55, 0x55, 0x66, 0x66, 0x66, 0x66,
> + 0x66, 0x66);
> +
> + s6e63m0_dcs_write_seq_static(ctx, 0xc1,
> + 0x4d, 0x96, 0x1d, 0x00, 0x00, 0x01, 0xdf,
> + 0x00, 0x00, 0x03, 0x1f, 0x00, 0x00, 0x00,
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x03, 0x06,
> + 0x09, 0x0d, 0x0f, 0x12, 0x15, 0x18);
> +
> + s6e63m0_dcs_write_seq_static(ctx, 0xb2,
> + 0x10, 0x10, 0x0b, 0x05);
> +
> + s6e63m0_dcs_write_seq_static(ctx, MCS_ACL_CTRL,
> + 0x01);
> +
> + s6e63m0_dcs_write_seq_static(ctx, MCS_ELVSS_ON,
> + 0x0b);
> +
> + s6e63m0_dcs_write_seq_static(ctx, MIPI_DCS_EXIT_SLEEP_MODE);
> +}
> +
> +static int s6e63m0_power_on(struct s6e63m0 *ctx)
> +{
> + int ret;
> +
> + gpiod_set_value(ctx->reset_gpio, 0);
> + msleep(120);
> +
> + ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> + if (ret < 0)
> + return ret;


Are you sure gpios before regulators, from my experience usually it is
reverse.


> +
> + msleep(25);
> +
> + return 0;
> +}
> +
> +static int s6e63m0_power_off(struct s6e63m0 *ctx)
> +{
> + int ret;
> +
> + ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> + if (ret < 0)
> + return ret;
> +
> + gpiod_set_value(ctx->reset_gpio, 1);


Ditto.


> + msleep(120);


What is the point of this msleep?


I guess, the answer for above comments can be difficult, since datasheet
is not available, so if you just followed vendor code please feel free
to add:

Reviewed-by: Andrzej Hajda <[email protected]>

--

Regards
Andrzej


> +
> + return 0;
> +}
> +
> +static int s6e63m0_disable(struct drm_panel *panel)
> +{
> + struct s6e63m0 *ctx = panel_to_s6e63m0(panel);
> +
> + if (!ctx->enabled)
> + return 0;
> +
> + backlight_disable(ctx->bl_dev);
> +
> + s6e63m0_dcs_write_seq_static(ctx, MIPI_DCS_ENTER_SLEEP_MODE);
> + msleep(200);
> +
> + ctx->enabled = false;
> +
> + return 0;
> +}
> +
> +static int s6e63m0_unprepare(struct drm_panel *panel)
> +{
> + struct s6e63m0 *ctx = panel_to_s6e63m0(panel);
> + int ret;
> +
> + if (!ctx->prepared)
> + return 0;
> +
> + s6e63m0_clear_error(ctx);
> +
> + ret = s6e63m0_power_off(ctx);
> + if (ret < 0)
> + return ret;
> +
> + ctx->prepared = false;
> +
> + return 0;
> +}
> +
> +static int s6e63m0_prepare(struct drm_panel *panel)
> +{
> + struct s6e63m0 *ctx = panel_to_s6e63m0(panel);
> + int ret;
> +
> + if (ctx->prepared)
> + return 0;
> +
> + ret = s6e63m0_power_on(ctx);
> + if (ret < 0)
> + return ret;
> +
> + s6e63m0_init(ctx);
> +
> + ret = s6e63m0_clear_error(ctx);
> +
> + if (ret < 0)
> + s6e63m0_unprepare(panel);
> +
> + ctx->prepared = true;
> +
> + return ret;
> +}
> +
> +static int s6e63m0_enable(struct drm_panel *panel)
> +{
> + struct s6e63m0 *ctx = panel_to_s6e63m0(panel);
> +
> + if (ctx->enabled)
> + return 0;
> +
> + s6e63m0_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_ON);
> +
> + backlight_enable(ctx->bl_dev);
> +
> + ctx->enabled = true;
> +
> + return 0;
> +}
> +
> +static int s6e63m0_get_modes(struct drm_panel *panel)
> +{
> + struct drm_connector *connector = panel->connector;
> + struct drm_display_mode *mode;
> +
> + mode = drm_mode_duplicate(panel->drm, &default_mode);
> + if (!mode) {
> + DRM_ERROR("failed to add mode %ux%ux@%u\n",
> + default_mode.hdisplay, default_mode.vdisplay,
> + default_mode.vrefresh);
> + return -ENOMEM;
> + }
> +
> + drm_mode_set_name(mode);
> +
> + mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> + drm_mode_probed_add(connector, mode);
> +
> + return 1;
> +}
> +
> +static const struct drm_panel_funcs s6e63m0_drm_funcs = {
> + .disable = s6e63m0_disable,
> + .unprepare = s6e63m0_unprepare,
> + .prepare = s6e63m0_prepare,
> + .enable = s6e63m0_enable,
> + .get_modes = s6e63m0_get_modes,
> +};
> +
> +static int s6e63m0_set_brightness(struct backlight_device *bd)
> +{
> + struct s6e63m0 *ctx = bl_get_data(bd);
> +
> + s6e63m0_brightness_set(ctx);
> +
> + return s6e63m0_clear_error(ctx);
> +}
> +
> +static const struct backlight_ops s6e63m0_backlight_ops = {
> + .update_status = s6e63m0_set_brightness,
> +};
> +
> +static int s6e63m0_backlight_register(struct s6e63m0 *ctx)
> +{
> + struct backlight_properties props = {
> + .type = BACKLIGHT_RAW,
> + .brightness = MAX_BRIGHTNESS,
> + .max_brightness = MAX_BRIGHTNESS
> + };
> + struct device *dev = ctx->dev;
> + int ret = 0;
> +
> + ctx->bl_dev = devm_backlight_device_register(dev, "panel", dev, ctx,
> + &s6e63m0_backlight_ops,
> + &props);
> + if (IS_ERR(ctx->bl_dev)) {
> + ret = PTR_ERR(ctx->bl_dev);
> + DRM_DEV_ERROR(dev, "error registering backlight device (%d)\n",
> + ret);
> + }
> +
> + return ret;
> +}
> +
> +static int s6e63m0_probe(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> + struct s6e63m0 *ctx;
> + int ret;
> +
> + ctx = devm_kzalloc(dev, sizeof(struct s6e63m0), GFP_KERNEL);
> + if (!ctx)
> + return -ENOMEM;
> +
> + spi_set_drvdata(spi, ctx);
> +
> + ctx->dev = dev;
> + ctx->enabled = false;
> + ctx->prepared = false;
> +
> + ctx->supplies[0].supply = "vdd3";
> + ctx->supplies[1].supply = "vci";
> + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ctx->supplies),
> + ctx->supplies);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev, "failed to get regulators: %d\n", ret);
> + return ret;
> + }
> +
> + ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> + if (IS_ERR(ctx->reset_gpio)) {
> + DRM_DEV_ERROR(dev, "cannot get reset-gpios %ld\n",
> + PTR_ERR(ctx->reset_gpio));
> + return PTR_ERR(ctx->reset_gpio);
> + }
> +
> + spi->bits_per_word = 9;
> + spi->mode = SPI_MODE_3;
> + ret = spi_setup(spi);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev, "spi setup failed.\n");
> + return ret;
> + }
> +
> + drm_panel_init(&ctx->panel);
> + ctx->panel.dev = dev;
> + ctx->panel.funcs = &s6e63m0_drm_funcs;
> +
> + ret = s6e63m0_backlight_register(ctx);
> + if (ret < 0)
> + return ret;
> +
> + return drm_panel_add(&ctx->panel);
> +}
> +
> +static int s6e63m0_remove(struct spi_device *spi)
> +{
> + struct s6e63m0 *ctx = spi_get_drvdata(spi);
> +
> + drm_panel_remove(&ctx->panel);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id s6e63m0_of_match[] = {
> + { .compatible = "samsung,s6e63m0" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, s6e63m0_of_match);
> +
> +static struct spi_driver s6e63m0_driver = {
> + .probe = s6e63m0_probe,
> + .remove = s6e63m0_remove,
> + .driver = {
> + .name = "panel-samsung-s6e63m0",
> + .of_match_table = s6e63m0_of_match,
> + },
> +};
> +module_spi_driver(s6e63m0_driver);
> +
> +MODULE_AUTHOR("Paweł Chmiel <[email protected]>");
> +MODULE_DESCRIPTION("s6e63m0 LCD Driver");
> +MODULE_LICENSE("GPL v2");



2019-02-11 16:09:21

by Paweł Chmiel

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] drm/panel: Add driver for Samsung S6E63M0 panel

On poniedziałek, 11 lutego 2019 16:35:19 CET Andrzej Hajda wrote:
> On 02.02.2019 15:27, Paweł Chmiel wrote:
> > This patch adds Samsung S6E63M0 AMOLED LCD panel driver, connected over
> > spi. It's based on already removed, non dt s6e63m0 driver and
> > panel-samsung-ld9040. It can be found for example in some of Samsung
> > Aries based phones.
> >
> > Signed-off-by: Paweł Chmiel <[email protected]>
> > Reviewed-by: Sam Ravnborg <[email protected]>
> > ---
> > Changes from v2:
> > - VIDEOMODE_HELPERS is not needed in Kconfig
> > - Added help text to Kconfig
> > - Remove unneeded videomode includes/fields
> > - Add sentinel comment in s6e63m0_of_match struct
> > - Handle errors during registration of backlight device. We shouldn't
> > register panel if we fail to register backlight device
> > - Added Reviewed-by
> >
> > Changes from v1:
> > - Correct order of Kconfig/Makefile entry
> > - Fix SPDX tag, so it matches value of MODULE_LICENSE
> > - Remove inclusion of drmP.h
> > - Fix code formatting
> > - Use DRM_DEV_ERROR/DEBUG
> > - Extract hardcoded values
> > - Remove possibility to change gamma through sysfs, leaving only one
> > gamma table values
> > - Fix reset_gpio handling, so it'll be asserted in power_on and
> > deasserted in power_off. Also do it before turning voltage on.
> > - Disable backlight and enter sleep mode in disable callback.
> > Previously it was done in unprepare
> > - Enable display and backlight in enable callback. Previously it was
> > done in prepare
> > - Hardcode display timings and delays. Previously they were readed
> > from device tree
> > - We're using SPDX, so we don't need to have license body
> > - Use MIPI_DCS_EXIT_SLEEP_MODE and MIPI_DCS_SET_DISPLAY_ON
> > - Rename MAX_GAMMA_LEVEL to NUM_GAMMA_LEVELS
> > - Ommit get_brightness callback
> > - Use backlight_enable/disable API, like it's done in other panel
> > drivers (for example panel-simple)
> > - Make set_brightness called only from backlight api, like it's done
> > in other panel drivers (for example panel-simple).
> > - Reset gpio should be set to GPIOD_OUT_HIGH. It's declared as active
> > low in device tree
> > - Don't call power_off in remove callback
> > ---
> > drivers/gpu/drm/panel/Kconfig | 9 +
> > drivers/gpu/drm/panel/Makefile | 1 +
> > drivers/gpu/drm/panel/panel-samsung-s6e63m0.c | 517 ++++++++++++++++++
> > 3 files changed, 527 insertions(+)
> > create mode 100644 drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
> >
> > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> > index 3f3537719beb..45e9ab4b7857 100644
> > --- a/drivers/gpu/drm/panel/Kconfig
> > +++ b/drivers/gpu/drm/panel/Kconfig
> > @@ -158,6 +158,15 @@ config DRM_PANEL_SAMSUNG_S6E63J0X03
> > depends on BACKLIGHT_CLASS_DEVICE
> > select VIDEOMODE_HELPERS
> >
> > +config DRM_PANEL_SAMSUNG_S6E63M0
> > + tristate "Samsung S6E63M0 RGB/SPI panel"
> > + depends on OF
> > + depends on SPI
> > + depends on BACKLIGHT_CLASS_DEVICE
> > + help
> > + Say Y here if you want to enable support for Samsung s6e63m0
> > + AMOLED LCD panel.
> > +
> > config DRM_PANEL_SAMSUNG_S6E8AA0
> > tristate "Samsung S6E8AA0 DSI video mode panel"
> > depends on OF
> > diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> > index 4396658a7996..4507a2d253ac 100644
> > --- a/drivers/gpu/drm/panel/Makefile
> > +++ b/drivers/gpu/drm/panel/Makefile
> > @@ -16,6 +16,7 @@ obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) += panel-samsung-ld9040.o
> > obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6D16D0) += panel-samsung-s6d16d0.o
> > obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E3HA2) += panel-samsung-s6e3ha2.o
> > obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E63J0X03) += panel-samsung-s6e63j0x03.o
> > +obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E63M0) += panel-samsung-s6e63m0.o
> > obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0) += panel-samsung-s6e8aa0.o
> > obj-$(CONFIG_DRM_PANEL_SEIKO_43WVF1G) += panel-seiko-43wvf1g.o
> > obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
> > diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c b/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
> > new file mode 100644
> > index 000000000000..4312aa3e4386
> > --- /dev/null
> > +++ b/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
> > @@ -0,0 +1,517 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * S6E63M0 AMOLED LCD drm_panel driver.
> > + *
> > + * Copyright (C) 2019 Paweł Chmiel <[email protected]>
> > + * Derived from drivers/gpu/drm/panel-samsung-ld9040.c
> > + *
> > + * Andrzej Hajda <[email protected]>
> > + */
> > +
> > +#include <drm/drm_modes.h>
> > +#include <drm/drm_panel.h>
> > +#include <drm/drm_print.h>
> > +
> > +#include <linux/backlight.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/module.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/spi/spi.h>
> > +
> > +#include <video/mipi_display.h>
> > +
> > +/* Manufacturer Command Set */
> > +#define MCS_ELVSS_ON 0xb1
> > +#define MCS_ACL_CTRL 0xc0
> > +#define MCS_DISPLAY_CONDITION 0xf2
> > +#define MCS_ETC_CONDITION 0xf6
> > +#define MCS_PANEL_CONDITION 0xF8
> > +#define MCS_GAMMA_CTRL 0xfa
> > +
> > +#define NUM_GAMMA_LEVELS 11
> > +#define GAMMA_TABLE_COUNT 23
> > +
> > +#define DATA_MASK 0x100
> > +
> > +#define MAX_BRIGHTNESS (NUM_GAMMA_LEVELS - 1)
> > +
> > +/* array of gamma tables for gamma value 2.2 */
> > +static u8 const s6e63m0_gamma_22[NUM_GAMMA_LEVELS][GAMMA_TABLE_COUNT] = {
> > + { MCS_GAMMA_CTRL, 0x00,
> > + 0x18, 0x08, 0x24, 0x78, 0xEC, 0x3D, 0xC8,
> > + 0xC2, 0xB6, 0xC4, 0xC7, 0xB6, 0xD5, 0xD7,
> > + 0xCC, 0x00, 0x39, 0x00, 0x36, 0x00, 0x51 },
> > + { MCS_GAMMA_CTRL, 0x00,
> > + 0x18, 0x08, 0x24, 0x73, 0x4A, 0x3D, 0xC0,
> > + 0xC2, 0xB1, 0xBB, 0xBE, 0xAC, 0xCE, 0xCF,
> > + 0xC5, 0x00, 0x5D, 0x00, 0x5E, 0x00, 0x82 },
> > + { MCS_GAMMA_CTRL, 0x00,
> > + 0x18, 0x08, 0x24, 0x70, 0x51, 0x3E, 0xBF,
> > + 0xC1, 0xAF, 0xB9, 0xBC, 0xAB, 0xCC, 0xCC,
> > + 0xC2, 0x00, 0x65, 0x00, 0x67, 0x00, 0x8D },
> > + { MCS_GAMMA_CTRL, 0x00,
> > + 0x18, 0x08, 0x24, 0x6C, 0x54, 0x3A, 0xBC,
> > + 0xBF, 0xAC, 0xB7, 0xBB, 0xA9, 0xC9, 0xC9,
> > + 0xBE, 0x00, 0x71, 0x00, 0x73, 0x00, 0x9E },
> > + { MCS_GAMMA_CTRL, 0x00,
> > + 0x18, 0x08, 0x24, 0x69, 0x54, 0x37, 0xBB,
> > + 0xBE, 0xAC, 0xB4, 0xB7, 0xA6, 0xC7, 0xC8,
> > + 0xBC, 0x00, 0x7B, 0x00, 0x7E, 0x00, 0xAB },
> > + { MCS_GAMMA_CTRL, 0x00,
> > + 0x18, 0x08, 0x24, 0x66, 0x55, 0x34, 0xBA,
> > + 0xBD, 0xAB, 0xB1, 0xB5, 0xA3, 0xC5, 0xC6,
> > + 0xB9, 0x00, 0x85, 0x00, 0x88, 0x00, 0xBA },
> > + { MCS_GAMMA_CTRL, 0x00,
> > + 0x18, 0x08, 0x24, 0x63, 0x53, 0x31, 0xB8,
> > + 0xBC, 0xA9, 0xB0, 0xB5, 0xA2, 0xC4, 0xC4,
> > + 0xB8, 0x00, 0x8B, 0x00, 0x8E, 0x00, 0xC2 },
> > + { MCS_GAMMA_CTRL, 0x00,
> > + 0x18, 0x08, 0x24, 0x62, 0x54, 0x30, 0xB9,
> > + 0xBB, 0xA9, 0xB0, 0xB3, 0xA1, 0xC1, 0xC3,
> > + 0xB7, 0x00, 0x91, 0x00, 0x95, 0x00, 0xDA },
> > + { MCS_GAMMA_CTRL, 0x00,
> > + 0x18, 0x08, 0x24, 0x66, 0x58, 0x34, 0xB6,
> > + 0xBA, 0xA7, 0xAF, 0xB3, 0xA0, 0xC1, 0xC2,
> > + 0xB7, 0x00, 0x97, 0x00, 0x9A, 0x00, 0xD1 },
> > + { MCS_GAMMA_CTRL, 0x00,
> > + 0x18, 0x08, 0x24, 0x64, 0x56, 0x33, 0xB6,
> > + 0xBA, 0xA8, 0xAC, 0xB1, 0x9D, 0xC1, 0xC1,
> > + 0xB7, 0x00, 0x9C, 0x00, 0x9F, 0x00, 0xD6 },
> > + { MCS_GAMMA_CTRL, 0x00,
> > + 0x18, 0x08, 0x24, 0x5f, 0x50, 0x2d, 0xB6,
> > + 0xB9, 0xA7, 0xAd, 0xB1, 0x9f, 0xbe, 0xC0,
> > + 0xB5, 0x00, 0xa0, 0x00, 0xa4, 0x00, 0xdb },
> > +};
> > +
> > +struct s6e63m0 {
> > + struct device *dev;
> > + struct drm_panel panel;
> > + struct backlight_device *bl_dev;
> > +
> > + struct regulator_bulk_data supplies[2];
> > + struct gpio_desc *reset_gpio;
> > +
> > + bool prepared;
> > + bool enabled;
> > +
> > + /*
> > + * This field is tested by functions directly accessing bus before
> > + * transfer, transfer is skipped if it is set. In case of transfer
> > + * failure or unexpected response the field is set to error value.
> > + * Such construct allows to eliminate many checks in higher level
> > + * functions.
> > + */
> > + int error;
> > +};
> > +
> > +static const struct drm_display_mode default_mode = {
> > + .clock = 25628,
> > + .hdisplay = 480,
> > + .hsync_start = 480 + 16,
> > + .hsync_end = 480 + 16 + 2,
> > + .htotal = 480 + 16 + 2 + 16,
> > + .vdisplay = 800,
> > + .vsync_start = 800 + 28,
> > + .vsync_end = 800 + 28 + 2,
> > + .vtotal = 800 + 28 + 2 + 1,
> > + .vrefresh = 60,
> > + .width_mm = 53,
> > + .height_mm = 89,
> > + .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
> > +};
> > +
> > +static inline struct s6e63m0 *panel_to_s6e63m0(struct drm_panel *panel)
> > +{
> > + return container_of(panel, struct s6e63m0, panel);
> > +}
> > +
> > +static int s6e63m0_clear_error(struct s6e63m0 *ctx)
> > +{
> > + int ret = ctx->error;
> > +
> > + ctx->error = 0;
> > + return ret;
> > +}
> > +
> > +static int s6e63m0_spi_write_word(struct s6e63m0 *ctx, u16 data)
> > +{
> > + struct spi_device *spi = to_spi_device(ctx->dev);
> > + struct spi_transfer xfer = {
> > + .len = 2,
> > + .tx_buf = &data,
> > + };
> > + struct spi_message msg;
> > +
> > + spi_message_init(&msg);
> > + spi_message_add_tail(&xfer, &msg);
> > +
> > + return spi_sync(spi, &msg);
> > +}
> > +
> > +static void s6e63m0_dcs_write(struct s6e63m0 *ctx, const u8 *data, size_t len)
> > +{
> > + int ret = 0;
> > +
> > + if (ctx->error < 0 || len == 0)
> > + return;
> > +
> > + DRM_DEV_DEBUG(ctx->dev, "writing dcs seq: %*ph\n", (int)len, data);
> > + ret = s6e63m0_spi_write_word(ctx, *data);
> > +
> > + while (!ret && --len) {
> > + ++data;
> > + ret = s6e63m0_spi_write_word(ctx, *data | DATA_MASK);
> > + }
> > +
> > + if (ret) {
> > + DRM_DEV_ERROR(ctx->dev, "error %d writing dcs seq: %*ph\n", ret,
> > + (int)len, data);
> > + ctx->error = ret;
> > + }
> > +
> > + usleep_range(300, 310);
> > +}
> > +
> > +#define s6e63m0_dcs_write_seq_static(ctx, seq ...) \
> > + ({ \
> > + static const u8 d[] = { seq }; \
> > + s6e63m0_dcs_write(ctx, d, ARRAY_SIZE(d)); \
> > + })
> > +
> > +static void s6e63m0_brightness_set(struct s6e63m0 *ctx)
> > +{
> > + int brightness = ctx->bl_dev->props.brightness;
> > +
> > + /* disable and set new gamma */
> > + s6e63m0_dcs_write(ctx, s6e63m0_gamma_22[brightness],
> > + ARRAY_SIZE(s6e63m0_gamma_22[brightness]));
> > +
> > + /* update gamma table. */
> > + s6e63m0_dcs_write_seq_static(ctx, MCS_GAMMA_CTRL, 0x01);
> > +}
>
>
> I guess you can squash this callback into s6e63m0_set_brightness.
> Moreover you performs transfer here unconditionally, regardless of panel
> power state, it will fail when drm device is disabled (using echo 255
> >/sys/class/backlight/...) - panel is also disabled - making transfer to
> disabled panel by panel driver looks odd, moreover it could be little
> bit racy. But in practice it could work.
>
> Moreover I do not see usage of backlight_properties.power,
> backlight_properties.state, but I do not understand how exactly
> backlight interface should work, if someone can point out to proper
> docs/usage of backlight API I will be glad.
I also had similar problems (and it was mentioned in previous versions review).
Now i've tried to replicate behaviour used by some other panel drivers.
>
>
> > +
> > +static void s6e63m0_init(struct s6e63m0 *ctx)
> > +{
> > + s6e63m0_dcs_write_seq_static(ctx, MCS_PANEL_CONDITION,
> > + 0x01, 0x27, 0x27, 0x07, 0x07, 0x54, 0x9f,
> > + 0x63, 0x86, 0x1a, 0x33, 0x0d, 0x00, 0x00);
> > +
> > + s6e63m0_dcs_write_seq_static(ctx, MCS_DISPLAY_CONDITION,
> > + 0x02, 0x03, 0x1c, 0x10, 0x10);
> > + s6e63m0_dcs_write_seq_static(ctx, 0xf7,
> > + 0x03, 0x00, 0x00);
> > +
> > + s6e63m0_dcs_write_seq_static(ctx, MCS_GAMMA_CTRL,
> > + 0x00, 0x18, 0x08, 0x24, 0x64, 0x56, 0x33,
> > + 0xb6, 0xba, 0xa8, 0xac, 0xb1, 0x9d, 0xc1,
> > + 0xc1, 0xb7, 0x00, 0x9c, 0x00, 0x9f, 0x00,
> > + 0xd6);
> > + s6e63m0_dcs_write_seq_static(ctx, MCS_GAMMA_CTRL,
> > + 0x01);
> > +
> > + s6e63m0_dcs_write_seq_static(ctx, MCS_ETC_CONDITION,
> > + 0x00, 0x8c, 0x07);
> > + s6e63m0_dcs_write_seq_static(ctx, 0xb3,
> > + 0xc);
> > +
> > + s6e63m0_dcs_write_seq_static(ctx, 0xb5,
> > + 0x2c, 0x12, 0x0c, 0x0a, 0x10, 0x0e, 0x17,
> > + 0x13, 0x1f, 0x1a, 0x2a, 0x24, 0x1f, 0x1b,
> > + 0x1a, 0x17, 0x2b, 0x26, 0x22, 0x20, 0x3a,
> > + 0x34, 0x30, 0x2c, 0x29, 0x26, 0x25, 0x23,
> > + 0x21, 0x20, 0x1e, 0x1e);
> > +
> > + s6e63m0_dcs_write_seq_static(ctx, 0xb6,
> > + 0x00, 0x00, 0x11, 0x22, 0x33, 0x44, 0x44,
> > + 0x44, 0x55, 0x55, 0x66, 0x66, 0x66, 0x66,
> > + 0x66, 0x66);
> > +
> > + s6e63m0_dcs_write_seq_static(ctx, 0xb7,
> > + 0x2c, 0x12, 0x0c, 0x0a, 0x10, 0x0e, 0x17,
> > + 0x13, 0x1f, 0x1a, 0x2a, 0x24, 0x1f, 0x1b,
> > + 0x1a, 0x17, 0x2b, 0x26, 0x22, 0x20, 0x3a,
> > + 0x34, 0x30, 0x2c, 0x29, 0x26, 0x25, 0x23,
> > + 0x21, 0x20, 0x1e, 0x1e, 0x00, 0x00, 0x11,
> > + 0x22, 0x33, 0x44, 0x44, 0x44, 0x55, 0x55,
> > + 0x66, 0x66, 0x66, 0x66, 0x66, 0x66);
> > +
> > + s6e63m0_dcs_write_seq_static(ctx, 0xb9,
> > + 0x2c, 0x12, 0x0c, 0x0a, 0x10, 0x0e, 0x17,
> > + 0x13, 0x1f, 0x1a, 0x2a, 0x24, 0x1f, 0x1b,
> > + 0x1a, 0x17, 0x2b, 0x26, 0x22, 0x20, 0x3a,
> > + 0x34, 0x30, 0x2c, 0x29, 0x26, 0x25, 0x23,
> > + 0x21, 0x20, 0x1e, 0x1e);
> > +
> > + s6e63m0_dcs_write_seq_static(ctx, 0xba,
> > + 0x00, 0x00, 0x11, 0x22, 0x33, 0x44, 0x44,
> > + 0x44, 0x55, 0x55, 0x66, 0x66, 0x66, 0x66,
> > + 0x66, 0x66);
> > +
> > + s6e63m0_dcs_write_seq_static(ctx, 0xc1,
> > + 0x4d, 0x96, 0x1d, 0x00, 0x00, 0x01, 0xdf,
> > + 0x00, 0x00, 0x03, 0x1f, 0x00, 0x00, 0x00,
> > + 0x00, 0x00, 0x00, 0x00, 0x00, 0x03, 0x06,
> > + 0x09, 0x0d, 0x0f, 0x12, 0x15, 0x18);
> > +
> > + s6e63m0_dcs_write_seq_static(ctx, 0xb2,
> > + 0x10, 0x10, 0x0b, 0x05);
> > +
> > + s6e63m0_dcs_write_seq_static(ctx, MCS_ACL_CTRL,
> > + 0x01);
> > +
> > + s6e63m0_dcs_write_seq_static(ctx, MCS_ELVSS_ON,
> > + 0x0b);
> > +
> > + s6e63m0_dcs_write_seq_static(ctx, MIPI_DCS_EXIT_SLEEP_MODE);
> > +}
> > +
> > +static int s6e63m0_power_on(struct s6e63m0 *ctx)
> > +{
> > + int ret;
> > +
> > + gpiod_set_value(ctx->reset_gpio, 0);
> > + msleep(120);
> > +
> > + ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> > + if (ret < 0)
> > + return ret;
>
>
> Are you sure gpios before regulators, from my experience usually it is
> reverse.
In v1 i had also regulators -> gpios order (like in vendor sources), but i got suggestion that changing it this way
we avoid situation where the panel is starting up, reset and then starting up again.
>
>
> > +
> > + msleep(25);
> > +
> > + return 0;
> > +}
> > +
> > +static int s6e63m0_power_off(struct s6e63m0 *ctx)
> > +{
> > + int ret;
> > +
> > + ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> > + if (ret < 0)
> > + return ret;
> > +
> > + gpiod_set_value(ctx->reset_gpio, 1);
>
>
> Ditto.
>
>
> > + msleep(120);
>
>
> What is the point of this msleep?
Yes, i missed it when fixing reset_gpio handling (previously it was done only on power_on, like in vendor driver).
After refactor and spliting code i leaved msleep in both places, where in poweroff it don't have any sense.
>
>
> I guess, the answer for above comments can be difficult, since datasheet
> is not available, so if you just followed vendor code please feel free
> to add:
>
> Reviewed-by: Andrzej Hajda <[email protected]>
I got information that this driver was tested on some tegra 2 based samsung device and it was working.
Samsung Captivate Glide (SGH-i927).

The only one change was that in original driver for that device, there was rotation set.
Currently i'm sending
0xf7, 0x03, 0x00, 0x00
where on that device driver was sending
0xf7, 0x00, 0x00, 0x00
Difference is that when running driver in current shape (sending 0x03), screen is rotated.

So it looks like that panel supports rotation. Should this be added to current driver (for example next patch after gettting initial driver fixed/working/merged),
or it should be handled somewhere in drm/userspace/some other place?

>
> --
>
> Regards
> Andrzej
>
>
> > +
> > + return 0;
> > +}
> > +
> > +static int s6e63m0_disable(struct drm_panel *panel)
> > +{
> > + struct s6e63m0 *ctx = panel_to_s6e63m0(panel);
> > +
> > + if (!ctx->enabled)
> > + return 0;
> > +
> > + backlight_disable(ctx->bl_dev);
> > +
> > + s6e63m0_dcs_write_seq_static(ctx, MIPI_DCS_ENTER_SLEEP_MODE);
> > + msleep(200);
> > +
> > + ctx->enabled = false;
> > +
> > + return 0;
> > +}
> > +
> > +static int s6e63m0_unprepare(struct drm_panel *panel)
> > +{
> > + struct s6e63m0 *ctx = panel_to_s6e63m0(panel);
> > + int ret;
> > +
> > + if (!ctx->prepared)
> > + return 0;
> > +
> > + s6e63m0_clear_error(ctx);
> > +
> > + ret = s6e63m0_power_off(ctx);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ctx->prepared = false;
> > +
> > + return 0;
> > +}
> > +
> > +static int s6e63m0_prepare(struct drm_panel *panel)
> > +{
> > + struct s6e63m0 *ctx = panel_to_s6e63m0(panel);
> > + int ret;
> > +
> > + if (ctx->prepared)
> > + return 0;
> > +
> > + ret = s6e63m0_power_on(ctx);
> > + if (ret < 0)
> > + return ret;
> > +
> > + s6e63m0_init(ctx);
> > +
> > + ret = s6e63m0_clear_error(ctx);
> > +
> > + if (ret < 0)
> > + s6e63m0_unprepare(panel);
> > +
> > + ctx->prepared = true;
> > +
> > + return ret;
> > +}
> > +
> > +static int s6e63m0_enable(struct drm_panel *panel)
> > +{
> > + struct s6e63m0 *ctx = panel_to_s6e63m0(panel);
> > +
> > + if (ctx->enabled)
> > + return 0;
> > +
> > + s6e63m0_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_ON);
> > +
> > + backlight_enable(ctx->bl_dev);
> > +
> > + ctx->enabled = true;
> > +
> > + return 0;
> > +}
> > +
> > +static int s6e63m0_get_modes(struct drm_panel *panel)
> > +{
> > + struct drm_connector *connector = panel->connector;
> > + struct drm_display_mode *mode;
> > +
> > + mode = drm_mode_duplicate(panel->drm, &default_mode);
> > + if (!mode) {
> > + DRM_ERROR("failed to add mode %ux%ux@%u\n",
> > + default_mode.hdisplay, default_mode.vdisplay,
> > + default_mode.vrefresh);
> > + return -ENOMEM;
> > + }
> > +
> > + drm_mode_set_name(mode);
> > +
> > + mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> > + drm_mode_probed_add(connector, mode);
> > +
> > + return 1;
> > +}
> > +
> > +static const struct drm_panel_funcs s6e63m0_drm_funcs = {
> > + .disable = s6e63m0_disable,
> > + .unprepare = s6e63m0_unprepare,
> > + .prepare = s6e63m0_prepare,
> > + .enable = s6e63m0_enable,
> > + .get_modes = s6e63m0_get_modes,
> > +};
> > +
> > +static int s6e63m0_set_brightness(struct backlight_device *bd)
> > +{
> > + struct s6e63m0 *ctx = bl_get_data(bd);
> > +
> > + s6e63m0_brightness_set(ctx);
> > +
> > + return s6e63m0_clear_error(ctx);
> > +}
> > +
> > +static const struct backlight_ops s6e63m0_backlight_ops = {
> > + .update_status = s6e63m0_set_brightness,
> > +};
> > +
> > +static int s6e63m0_backlight_register(struct s6e63m0 *ctx)
> > +{
> > + struct backlight_properties props = {
> > + .type = BACKLIGHT_RAW,
> > + .brightness = MAX_BRIGHTNESS,
> > + .max_brightness = MAX_BRIGHTNESS
> > + };
> > + struct device *dev = ctx->dev;
> > + int ret = 0;
> > +
> > + ctx->bl_dev = devm_backlight_device_register(dev, "panel", dev, ctx,
> > + &s6e63m0_backlight_ops,
> > + &props);
> > + if (IS_ERR(ctx->bl_dev)) {
> > + ret = PTR_ERR(ctx->bl_dev);
> > + DRM_DEV_ERROR(dev, "error registering backlight device (%d)\n",
> > + ret);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int s6e63m0_probe(struct spi_device *spi)
> > +{
> > + struct device *dev = &spi->dev;
> > + struct s6e63m0 *ctx;
> > + int ret;
> > +
> > + ctx = devm_kzalloc(dev, sizeof(struct s6e63m0), GFP_KERNEL);
> > + if (!ctx)
> > + return -ENOMEM;
> > +
> > + spi_set_drvdata(spi, ctx);
> > +
> > + ctx->dev = dev;
> > + ctx->enabled = false;
> > + ctx->prepared = false;
> > +
> > + ctx->supplies[0].supply = "vdd3";
> > + ctx->supplies[1].supply = "vci";
> > + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ctx->supplies),
> > + ctx->supplies);
> > + if (ret < 0) {
> > + DRM_DEV_ERROR(dev, "failed to get regulators: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> > + if (IS_ERR(ctx->reset_gpio)) {
> > + DRM_DEV_ERROR(dev, "cannot get reset-gpios %ld\n",
> > + PTR_ERR(ctx->reset_gpio));
> > + return PTR_ERR(ctx->reset_gpio);
> > + }
> > +
> > + spi->bits_per_word = 9;
> > + spi->mode = SPI_MODE_3;
> > + ret = spi_setup(spi);
> > + if (ret < 0) {
> > + DRM_DEV_ERROR(dev, "spi setup failed.\n");
> > + return ret;
> > + }
> > +
> > + drm_panel_init(&ctx->panel);
> > + ctx->panel.dev = dev;
> > + ctx->panel.funcs = &s6e63m0_drm_funcs;
> > +
> > + ret = s6e63m0_backlight_register(ctx);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return drm_panel_add(&ctx->panel);
> > +}
> > +
> > +static int s6e63m0_remove(struct spi_device *spi)
> > +{
> > + struct s6e63m0 *ctx = spi_get_drvdata(spi);
> > +
> > + drm_panel_remove(&ctx->panel);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id s6e63m0_of_match[] = {
> > + { .compatible = "samsung,s6e63m0" },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, s6e63m0_of_match);
> > +
> > +static struct spi_driver s6e63m0_driver = {
> > + .probe = s6e63m0_probe,
> > + .remove = s6e63m0_remove,
> > + .driver = {
> > + .name = "panel-samsung-s6e63m0",
> > + .of_match_table = s6e63m0_of_match,
> > + },
> > +};
> > +module_spi_driver(s6e63m0_driver);
> > +
> > +MODULE_AUTHOR("Paweł Chmiel <[email protected]>");
> > +MODULE_DESCRIPTION("s6e63m0 LCD Driver");
> > +MODULE_LICENSE("GPL v2");
>
>
>





2019-02-12 09:12:17

by Andrzej Hajda

[permalink] [raw]
Subject: Re: [PATCH v3 2/2] drm/panel: Add driver for Samsung S6E63M0 panel

On 11.02.2019 17:07, Paweł Chmiel wrote:
> On poniedziałek, 11 lutego 2019 16:35:19 CET Andrzej Hajda wrote:
>> On 02.02.2019 15:27, Paweł Chmiel wrote:
>>> This patch adds Samsung S6E63M0 AMOLED LCD panel driver, connected over
>>> spi. It's based on already removed, non dt s6e63m0 driver and
>>> panel-samsung-ld9040. It can be found for example in some of Samsung
>>> Aries based phones.
>>>
>>> Signed-off-by: Paweł Chmiel <[email protected]>
>>> Reviewed-by: Sam Ravnborg <[email protected]>
>>> ---
>>> Changes from v2:
>>> - VIDEOMODE_HELPERS is not needed in Kconfig
>>> - Added help text to Kconfig
>>> - Remove unneeded videomode includes/fields
>>> - Add sentinel comment in s6e63m0_of_match struct
>>> - Handle errors during registration of backlight device. We shouldn't
>>> register panel if we fail to register backlight device
>>> - Added Reviewed-by
>>>
>>> Changes from v1:
>>> - Correct order of Kconfig/Makefile entry
>>> - Fix SPDX tag, so it matches value of MODULE_LICENSE
>>> - Remove inclusion of drmP.h
>>> - Fix code formatting
>>> - Use DRM_DEV_ERROR/DEBUG
>>> - Extract hardcoded values
>>> - Remove possibility to change gamma through sysfs, leaving only one
>>> gamma table values
>>> - Fix reset_gpio handling, so it'll be asserted in power_on and
>>> deasserted in power_off. Also do it before turning voltage on.
>>> - Disable backlight and enter sleep mode in disable callback.
>>> Previously it was done in unprepare
>>> - Enable display and backlight in enable callback. Previously it was
>>> done in prepare
>>> - Hardcode display timings and delays. Previously they were readed
>>> from device tree
>>> - We're using SPDX, so we don't need to have license body
>>> - Use MIPI_DCS_EXIT_SLEEP_MODE and MIPI_DCS_SET_DISPLAY_ON
>>> - Rename MAX_GAMMA_LEVEL to NUM_GAMMA_LEVELS
>>> - Ommit get_brightness callback
>>> - Use backlight_enable/disable API, like it's done in other panel
>>> drivers (for example panel-simple)
>>> - Make set_brightness called only from backlight api, like it's done
>>> in other panel drivers (for example panel-simple).
>>> - Reset gpio should be set to GPIOD_OUT_HIGH. It's declared as active
>>> low in device tree
>>> - Don't call power_off in remove callback
>>> ---
>>> drivers/gpu/drm/panel/Kconfig | 9 +
>>> drivers/gpu/drm/panel/Makefile | 1 +
>>> drivers/gpu/drm/panel/panel-samsung-s6e63m0.c | 517 ++++++++++++++++++
>>> 3 files changed, 527 insertions(+)
>>> create mode 100644 drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
>>>
>>> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
>>> index 3f3537719beb..45e9ab4b7857 100644
>>> --- a/drivers/gpu/drm/panel/Kconfig
>>> +++ b/drivers/gpu/drm/panel/Kconfig
>>> @@ -158,6 +158,15 @@ config DRM_PANEL_SAMSUNG_S6E63J0X03
>>> depends on BACKLIGHT_CLASS_DEVICE
>>> select VIDEOMODE_HELPERS
>>>
>>> +config DRM_PANEL_SAMSUNG_S6E63M0
>>> + tristate "Samsung S6E63M0 RGB/SPI panel"
>>> + depends on OF
>>> + depends on SPI
>>> + depends on BACKLIGHT_CLASS_DEVICE
>>> + help
>>> + Say Y here if you want to enable support for Samsung s6e63m0
>>> + AMOLED LCD panel.
>>> +
>>> config DRM_PANEL_SAMSUNG_S6E8AA0
>>> tristate "Samsung S6E8AA0 DSI video mode panel"
>>> depends on OF
>>> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
>>> index 4396658a7996..4507a2d253ac 100644
>>> --- a/drivers/gpu/drm/panel/Makefile
>>> +++ b/drivers/gpu/drm/panel/Makefile
>>> @@ -16,6 +16,7 @@ obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) += panel-samsung-ld9040.o
>>> obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6D16D0) += panel-samsung-s6d16d0.o
>>> obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E3HA2) += panel-samsung-s6e3ha2.o
>>> obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E63J0X03) += panel-samsung-s6e63j0x03.o
>>> +obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E63M0) += panel-samsung-s6e63m0.o
>>> obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0) += panel-samsung-s6e8aa0.o
>>> obj-$(CONFIG_DRM_PANEL_SEIKO_43WVF1G) += panel-seiko-43wvf1g.o
>>> obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
>>> diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c b/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
>>> new file mode 100644
>>> index 000000000000..4312aa3e4386
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
>>> @@ -0,0 +1,517 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * S6E63M0 AMOLED LCD drm_panel driver.
>>> + *
>>> + * Copyright (C) 2019 Paweł Chmiel <[email protected]>
>>> + * Derived from drivers/gpu/drm/panel-samsung-ld9040.c
>>> + *
>>> + * Andrzej Hajda <[email protected]>
>>> + */
>>> +
>>> +#include <drm/drm_modes.h>
>>> +#include <drm/drm_panel.h>
>>> +#include <drm/drm_print.h>
>>> +
>>> +#include <linux/backlight.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/gpio/consumer.h>
>>> +#include <linux/module.h>
>>> +#include <linux/regulator/consumer.h>
>>> +#include <linux/spi/spi.h>
>>> +
>>> +#include <video/mipi_display.h>
>>> +
>>> +/* Manufacturer Command Set */
>>> +#define MCS_ELVSS_ON 0xb1
>>> +#define MCS_ACL_CTRL 0xc0
>>> +#define MCS_DISPLAY_CONDITION 0xf2
>>> +#define MCS_ETC_CONDITION 0xf6
>>> +#define MCS_PANEL_CONDITION 0xF8
>>> +#define MCS_GAMMA_CTRL 0xfa
>>> +
>>> +#define NUM_GAMMA_LEVELS 11
>>> +#define GAMMA_TABLE_COUNT 23
>>> +
>>> +#define DATA_MASK 0x100
>>> +
>>> +#define MAX_BRIGHTNESS (NUM_GAMMA_LEVELS - 1)
>>> +
>>> +/* array of gamma tables for gamma value 2.2 */
>>> +static u8 const s6e63m0_gamma_22[NUM_GAMMA_LEVELS][GAMMA_TABLE_COUNT] = {
>>> + { MCS_GAMMA_CTRL, 0x00,
>>> + 0x18, 0x08, 0x24, 0x78, 0xEC, 0x3D, 0xC8,
>>> + 0xC2, 0xB6, 0xC4, 0xC7, 0xB6, 0xD5, 0xD7,
>>> + 0xCC, 0x00, 0x39, 0x00, 0x36, 0x00, 0x51 },
>>> + { MCS_GAMMA_CTRL, 0x00,
>>> + 0x18, 0x08, 0x24, 0x73, 0x4A, 0x3D, 0xC0,
>>> + 0xC2, 0xB1, 0xBB, 0xBE, 0xAC, 0xCE, 0xCF,
>>> + 0xC5, 0x00, 0x5D, 0x00, 0x5E, 0x00, 0x82 },
>>> + { MCS_GAMMA_CTRL, 0x00,
>>> + 0x18, 0x08, 0x24, 0x70, 0x51, 0x3E, 0xBF,
>>> + 0xC1, 0xAF, 0xB9, 0xBC, 0xAB, 0xCC, 0xCC,
>>> + 0xC2, 0x00, 0x65, 0x00, 0x67, 0x00, 0x8D },
>>> + { MCS_GAMMA_CTRL, 0x00,
>>> + 0x18, 0x08, 0x24, 0x6C, 0x54, 0x3A, 0xBC,
>>> + 0xBF, 0xAC, 0xB7, 0xBB, 0xA9, 0xC9, 0xC9,
>>> + 0xBE, 0x00, 0x71, 0x00, 0x73, 0x00, 0x9E },
>>> + { MCS_GAMMA_CTRL, 0x00,
>>> + 0x18, 0x08, 0x24, 0x69, 0x54, 0x37, 0xBB,
>>> + 0xBE, 0xAC, 0xB4, 0xB7, 0xA6, 0xC7, 0xC8,
>>> + 0xBC, 0x00, 0x7B, 0x00, 0x7E, 0x00, 0xAB },
>>> + { MCS_GAMMA_CTRL, 0x00,
>>> + 0x18, 0x08, 0x24, 0x66, 0x55, 0x34, 0xBA,
>>> + 0xBD, 0xAB, 0xB1, 0xB5, 0xA3, 0xC5, 0xC6,
>>> + 0xB9, 0x00, 0x85, 0x00, 0x88, 0x00, 0xBA },
>>> + { MCS_GAMMA_CTRL, 0x00,
>>> + 0x18, 0x08, 0x24, 0x63, 0x53, 0x31, 0xB8,
>>> + 0xBC, 0xA9, 0xB0, 0xB5, 0xA2, 0xC4, 0xC4,
>>> + 0xB8, 0x00, 0x8B, 0x00, 0x8E, 0x00, 0xC2 },
>>> + { MCS_GAMMA_CTRL, 0x00,
>>> + 0x18, 0x08, 0x24, 0x62, 0x54, 0x30, 0xB9,
>>> + 0xBB, 0xA9, 0xB0, 0xB3, 0xA1, 0xC1, 0xC3,
>>> + 0xB7, 0x00, 0x91, 0x00, 0x95, 0x00, 0xDA },
>>> + { MCS_GAMMA_CTRL, 0x00,
>>> + 0x18, 0x08, 0x24, 0x66, 0x58, 0x34, 0xB6,
>>> + 0xBA, 0xA7, 0xAF, 0xB3, 0xA0, 0xC1, 0xC2,
>>> + 0xB7, 0x00, 0x97, 0x00, 0x9A, 0x00, 0xD1 },
>>> + { MCS_GAMMA_CTRL, 0x00,
>>> + 0x18, 0x08, 0x24, 0x64, 0x56, 0x33, 0xB6,
>>> + 0xBA, 0xA8, 0xAC, 0xB1, 0x9D, 0xC1, 0xC1,
>>> + 0xB7, 0x00, 0x9C, 0x00, 0x9F, 0x00, 0xD6 },
>>> + { MCS_GAMMA_CTRL, 0x00,
>>> + 0x18, 0x08, 0x24, 0x5f, 0x50, 0x2d, 0xB6,
>>> + 0xB9, 0xA7, 0xAd, 0xB1, 0x9f, 0xbe, 0xC0,
>>> + 0xB5, 0x00, 0xa0, 0x00, 0xa4, 0x00, 0xdb },
>>> +};
>>> +
>>> +struct s6e63m0 {
>>> + struct device *dev;
>>> + struct drm_panel panel;
>>> + struct backlight_device *bl_dev;
>>> +
>>> + struct regulator_bulk_data supplies[2];
>>> + struct gpio_desc *reset_gpio;
>>> +
>>> + bool prepared;
>>> + bool enabled;
>>> +
>>> + /*
>>> + * This field is tested by functions directly accessing bus before
>>> + * transfer, transfer is skipped if it is set. In case of transfer
>>> + * failure or unexpected response the field is set to error value.
>>> + * Such construct allows to eliminate many checks in higher level
>>> + * functions.
>>> + */
>>> + int error;
>>> +};
>>> +
>>> +static const struct drm_display_mode default_mode = {
>>> + .clock = 25628,
>>> + .hdisplay = 480,
>>> + .hsync_start = 480 + 16,
>>> + .hsync_end = 480 + 16 + 2,
>>> + .htotal = 480 + 16 + 2 + 16,
>>> + .vdisplay = 800,
>>> + .vsync_start = 800 + 28,
>>> + .vsync_end = 800 + 28 + 2,
>>> + .vtotal = 800 + 28 + 2 + 1,
>>> + .vrefresh = 60,
>>> + .width_mm = 53,
>>> + .height_mm = 89,
>>> + .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
>>> +};
>>> +
>>> +static inline struct s6e63m0 *panel_to_s6e63m0(struct drm_panel *panel)
>>> +{
>>> + return container_of(panel, struct s6e63m0, panel);
>>> +}
>>> +
>>> +static int s6e63m0_clear_error(struct s6e63m0 *ctx)
>>> +{
>>> + int ret = ctx->error;
>>> +
>>> + ctx->error = 0;
>>> + return ret;
>>> +}
>>> +
>>> +static int s6e63m0_spi_write_word(struct s6e63m0 *ctx, u16 data)
>>> +{
>>> + struct spi_device *spi = to_spi_device(ctx->dev);
>>> + struct spi_transfer xfer = {
>>> + .len = 2,
>>> + .tx_buf = &data,
>>> + };
>>> + struct spi_message msg;
>>> +
>>> + spi_message_init(&msg);
>>> + spi_message_add_tail(&xfer, &msg);
>>> +
>>> + return spi_sync(spi, &msg);
>>> +}
>>> +
>>> +static void s6e63m0_dcs_write(struct s6e63m0 *ctx, const u8 *data, size_t len)
>>> +{
>>> + int ret = 0;
>>> +
>>> + if (ctx->error < 0 || len == 0)
>>> + return;
>>> +
>>> + DRM_DEV_DEBUG(ctx->dev, "writing dcs seq: %*ph\n", (int)len, data);
>>> + ret = s6e63m0_spi_write_word(ctx, *data);
>>> +
>>> + while (!ret && --len) {
>>> + ++data;
>>> + ret = s6e63m0_spi_write_word(ctx, *data | DATA_MASK);
>>> + }
>>> +
>>> + if (ret) {
>>> + DRM_DEV_ERROR(ctx->dev, "error %d writing dcs seq: %*ph\n", ret,
>>> + (int)len, data);
>>> + ctx->error = ret;
>>> + }
>>> +
>>> + usleep_range(300, 310);
>>> +}
>>> +
>>> +#define s6e63m0_dcs_write_seq_static(ctx, seq ...) \
>>> + ({ \
>>> + static const u8 d[] = { seq }; \
>>> + s6e63m0_dcs_write(ctx, d, ARRAY_SIZE(d)); \
>>> + })
>>> +
>>> +static void s6e63m0_brightness_set(struct s6e63m0 *ctx)
>>> +{
>>> + int brightness = ctx->bl_dev->props.brightness;
>>> +
>>> + /* disable and set new gamma */
>>> + s6e63m0_dcs_write(ctx, s6e63m0_gamma_22[brightness],
>>> + ARRAY_SIZE(s6e63m0_gamma_22[brightness]));
>>> +
>>> + /* update gamma table. */
>>> + s6e63m0_dcs_write_seq_static(ctx, MCS_GAMMA_CTRL, 0x01);
>>> +}
>>
>> I guess you can squash this callback into s6e63m0_set_brightness.
>> Moreover you performs transfer here unconditionally, regardless of panel
>> power state, it will fail when drm device is disabled (using echo 255
>>> /sys/class/backlight/...) - panel is also disabled - making transfer to
>> disabled panel by panel driver looks odd, moreover it could be little
>> bit racy. But in practice it could work.
>>
>> Moreover I do not see usage of backlight_properties.power,
>> backlight_properties.state, but I do not understand how exactly
>> backlight interface should work, if someone can point out to proper
>> docs/usage of backlight API I will be glad.
> I also had similar problems (and it was mentioned in previous versions review).
> Now i've tried to replicate behaviour used by some other panel drivers.
>>
>>> +
>>> +static void s6e63m0_init(struct s6e63m0 *ctx)
>>> +{
>>> + s6e63m0_dcs_write_seq_static(ctx, MCS_PANEL_CONDITION,
>>> + 0x01, 0x27, 0x27, 0x07, 0x07, 0x54, 0x9f,
>>> + 0x63, 0x86, 0x1a, 0x33, 0x0d, 0x00, 0x00);
>>> +
>>> + s6e63m0_dcs_write_seq_static(ctx, MCS_DISPLAY_CONDITION,
>>> + 0x02, 0x03, 0x1c, 0x10, 0x10);
>>> + s6e63m0_dcs_write_seq_static(ctx, 0xf7,
>>> + 0x03, 0x00, 0x00);
>>> +
>>> + s6e63m0_dcs_write_seq_static(ctx, MCS_GAMMA_CTRL,
>>> + 0x00, 0x18, 0x08, 0x24, 0x64, 0x56, 0x33,
>>> + 0xb6, 0xba, 0xa8, 0xac, 0xb1, 0x9d, 0xc1,
>>> + 0xc1, 0xb7, 0x00, 0x9c, 0x00, 0x9f, 0x00,
>>> + 0xd6);
>>> + s6e63m0_dcs_write_seq_static(ctx, MCS_GAMMA_CTRL,
>>> + 0x01);
>>> +
>>> + s6e63m0_dcs_write_seq_static(ctx, MCS_ETC_CONDITION,
>>> + 0x00, 0x8c, 0x07);
>>> + s6e63m0_dcs_write_seq_static(ctx, 0xb3,
>>> + 0xc);
>>> +
>>> + s6e63m0_dcs_write_seq_static(ctx, 0xb5,
>>> + 0x2c, 0x12, 0x0c, 0x0a, 0x10, 0x0e, 0x17,
>>> + 0x13, 0x1f, 0x1a, 0x2a, 0x24, 0x1f, 0x1b,
>>> + 0x1a, 0x17, 0x2b, 0x26, 0x22, 0x20, 0x3a,
>>> + 0x34, 0x30, 0x2c, 0x29, 0x26, 0x25, 0x23,
>>> + 0x21, 0x20, 0x1e, 0x1e);
>>> +
>>> + s6e63m0_dcs_write_seq_static(ctx, 0xb6,
>>> + 0x00, 0x00, 0x11, 0x22, 0x33, 0x44, 0x44,
>>> + 0x44, 0x55, 0x55, 0x66, 0x66, 0x66, 0x66,
>>> + 0x66, 0x66);
>>> +
>>> + s6e63m0_dcs_write_seq_static(ctx, 0xb7,
>>> + 0x2c, 0x12, 0x0c, 0x0a, 0x10, 0x0e, 0x17,
>>> + 0x13, 0x1f, 0x1a, 0x2a, 0x24, 0x1f, 0x1b,
>>> + 0x1a, 0x17, 0x2b, 0x26, 0x22, 0x20, 0x3a,
>>> + 0x34, 0x30, 0x2c, 0x29, 0x26, 0x25, 0x23,
>>> + 0x21, 0x20, 0x1e, 0x1e, 0x00, 0x00, 0x11,
>>> + 0x22, 0x33, 0x44, 0x44, 0x44, 0x55, 0x55,
>>> + 0x66, 0x66, 0x66, 0x66, 0x66, 0x66);
>>> +
>>> + s6e63m0_dcs_write_seq_static(ctx, 0xb9,
>>> + 0x2c, 0x12, 0x0c, 0x0a, 0x10, 0x0e, 0x17,
>>> + 0x13, 0x1f, 0x1a, 0x2a, 0x24, 0x1f, 0x1b,
>>> + 0x1a, 0x17, 0x2b, 0x26, 0x22, 0x20, 0x3a,
>>> + 0x34, 0x30, 0x2c, 0x29, 0x26, 0x25, 0x23,
>>> + 0x21, 0x20, 0x1e, 0x1e);
>>> +
>>> + s6e63m0_dcs_write_seq_static(ctx, 0xba,
>>> + 0x00, 0x00, 0x11, 0x22, 0x33, 0x44, 0x44,
>>> + 0x44, 0x55, 0x55, 0x66, 0x66, 0x66, 0x66,
>>> + 0x66, 0x66);
>>> +
>>> + s6e63m0_dcs_write_seq_static(ctx, 0xc1,
>>> + 0x4d, 0x96, 0x1d, 0x00, 0x00, 0x01, 0xdf,
>>> + 0x00, 0x00, 0x03, 0x1f, 0x00, 0x00, 0x00,
>>> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x03, 0x06,
>>> + 0x09, 0x0d, 0x0f, 0x12, 0x15, 0x18);
>>> +
>>> + s6e63m0_dcs_write_seq_static(ctx, 0xb2,
>>> + 0x10, 0x10, 0x0b, 0x05);
>>> +
>>> + s6e63m0_dcs_write_seq_static(ctx, MCS_ACL_CTRL,
>>> + 0x01);
>>> +
>>> + s6e63m0_dcs_write_seq_static(ctx, MCS_ELVSS_ON,
>>> + 0x0b);
>>> +
>>> + s6e63m0_dcs_write_seq_static(ctx, MIPI_DCS_EXIT_SLEEP_MODE);
>>> +}
>>> +
>>> +static int s6e63m0_power_on(struct s6e63m0 *ctx)
>>> +{
>>> + int ret;
>>> +
>>> + gpiod_set_value(ctx->reset_gpio, 0);
>>> + msleep(120);
>>> +
>>> + ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
>>> + if (ret < 0)
>>> + return ret;
>>
>> Are you sure gpios before regulators, from my experience usually it is
>> reverse.
> In v1 i had also regulators -> gpios order (like in vendor sources), but i got suggestion that changing it this way
> we avoid situation where the panel is starting up, reset and then starting up again.


In Samsung panels RESET line usually behaves more like power line, not
as a reset impulse.

Power-on sequence is as follows:

Initial state: RESET line is physically low, supplies off.

Power on:

1. Enable power supplies (sometimes order and timings matters, sometimes
not, depends on panel).

2. After specific time RESET line should be physically raised.

3. After specific time panel is working.


Power off is reverse, with different timings.

See for example datasheets for some panels:

https://www.beyondinfinite.com/lcd/Library/Samsung/AMS369FG06-0.pdf

https://www.displayfuture.com/Display/datasheet/controller/S6D04H0X.pdf


BTW you can try to match your MCS command names with ones specified in
these pdfs.


There are some chances that in case of your panel the sequence is
different, but I do not think so.

I guess vendor code has also this sequence.


>>
>>> +
>>> + msleep(25);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int s6e63m0_power_off(struct s6e63m0 *ctx)
>>> +{
>>> + int ret;
>>> +
>>> + ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + gpiod_set_value(ctx->reset_gpio, 1);
>>
>> Ditto.
>>
>>
>>> + msleep(120);
>>
>> What is the point of this msleep?
> Yes, i missed it when fixing reset_gpio handling (previously it was done only on power_on, like in vendor driver).
> After refactor and spliting code i leaved msleep in both places, where in poweroff it don't have any sense.
>>
>> I guess, the answer for above comments can be difficult, since datasheet
>> is not available, so if you just followed vendor code please feel free
>> to add:
>>
>> Reviewed-by: Andrzej Hajda <[email protected]>
> I got information that this driver was tested on some tegra 2 based samsung device and it was working.
> Samsung Captivate Glide (SGH-i927).
>
> The only one change was that in original driver for that device, there was rotation set.
> Currently i'm sending
> 0xf7, 0x03, 0x00, 0x00
> where on that device driver was sending
> 0xf7, 0x00, 0x00, 0x00
> Difference is that when running driver in current shape (sending 0x03), screen is rotated.
>
> So it looks like that panel supports rotation. Should this be added to current driver (for example next patch after gettting initial driver fixed/working/merged),
> or it should be handled somewhere in drm/userspace/some other place?


IIRC rotation can be specified in devicetree property [1].

[1]: Documentation/devicetree/bindings/display/panel/panel.txt


Regards

Andrzej


>
>> --
>>
>> Regards
>> Andrzej
>>
>>
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int s6e63m0_disable(struct drm_panel *panel)
>>> +{
>>> + struct s6e63m0 *ctx = panel_to_s6e63m0(panel);
>>> +
>>> + if (!ctx->enabled)
>>> + return 0;
>>> +
>>> + backlight_disable(ctx->bl_dev);
>>> +
>>> + s6e63m0_dcs_write_seq_static(ctx, MIPI_DCS_ENTER_SLEEP_MODE);
>>> + msleep(200);
>>> +
>>> + ctx->enabled = false;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int s6e63m0_unprepare(struct drm_panel *panel)
>>> +{
>>> + struct s6e63m0 *ctx = panel_to_s6e63m0(panel);
>>> + int ret;
>>> +
>>> + if (!ctx->prepared)
>>> + return 0;
>>> +
>>> + s6e63m0_clear_error(ctx);
>>> +
>>> + ret = s6e63m0_power_off(ctx);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + ctx->prepared = false;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int s6e63m0_prepare(struct drm_panel *panel)
>>> +{
>>> + struct s6e63m0 *ctx = panel_to_s6e63m0(panel);
>>> + int ret;
>>> +
>>> + if (ctx->prepared)
>>> + return 0;
>>> +
>>> + ret = s6e63m0_power_on(ctx);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + s6e63m0_init(ctx);
>>> +
>>> + ret = s6e63m0_clear_error(ctx);
>>> +
>>> + if (ret < 0)
>>> + s6e63m0_unprepare(panel);
>>> +
>>> + ctx->prepared = true;
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int s6e63m0_enable(struct drm_panel *panel)
>>> +{
>>> + struct s6e63m0 *ctx = panel_to_s6e63m0(panel);
>>> +
>>> + if (ctx->enabled)
>>> + return 0;
>>> +
>>> + s6e63m0_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_ON);
>>> +
>>> + backlight_enable(ctx->bl_dev);
>>> +
>>> + ctx->enabled = true;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int s6e63m0_get_modes(struct drm_panel *panel)
>>> +{
>>> + struct drm_connector *connector = panel->connector;
>>> + struct drm_display_mode *mode;
>>> +
>>> + mode = drm_mode_duplicate(panel->drm, &default_mode);
>>> + if (!mode) {
>>> + DRM_ERROR("failed to add mode %ux%ux@%u\n",
>>> + default_mode.hdisplay, default_mode.vdisplay,
>>> + default_mode.vrefresh);
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + drm_mode_set_name(mode);
>>> +
>>> + mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
>>> + drm_mode_probed_add(connector, mode);
>>> +
>>> + return 1;
>>> +}
>>> +
>>> +static const struct drm_panel_funcs s6e63m0_drm_funcs = {
>>> + .disable = s6e63m0_disable,
>>> + .unprepare = s6e63m0_unprepare,
>>> + .prepare = s6e63m0_prepare,
>>> + .enable = s6e63m0_enable,
>>> + .get_modes = s6e63m0_get_modes,
>>> +};
>>> +
>>> +static int s6e63m0_set_brightness(struct backlight_device *bd)
>>> +{
>>> + struct s6e63m0 *ctx = bl_get_data(bd);
>>> +
>>> + s6e63m0_brightness_set(ctx);
>>> +
>>> + return s6e63m0_clear_error(ctx);
>>> +}
>>> +
>>> +static const struct backlight_ops s6e63m0_backlight_ops = {
>>> + .update_status = s6e63m0_set_brightness,
>>> +};
>>> +
>>> +static int s6e63m0_backlight_register(struct s6e63m0 *ctx)
>>> +{
>>> + struct backlight_properties props = {
>>> + .type = BACKLIGHT_RAW,
>>> + .brightness = MAX_BRIGHTNESS,
>>> + .max_brightness = MAX_BRIGHTNESS
>>> + };
>>> + struct device *dev = ctx->dev;
>>> + int ret = 0;
>>> +
>>> + ctx->bl_dev = devm_backlight_device_register(dev, "panel", dev, ctx,
>>> + &s6e63m0_backlight_ops,
>>> + &props);
>>> + if (IS_ERR(ctx->bl_dev)) {
>>> + ret = PTR_ERR(ctx->bl_dev);
>>> + DRM_DEV_ERROR(dev, "error registering backlight device (%d)\n",
>>> + ret);
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int s6e63m0_probe(struct spi_device *spi)
>>> +{
>>> + struct device *dev = &spi->dev;
>>> + struct s6e63m0 *ctx;
>>> + int ret;
>>> +
>>> + ctx = devm_kzalloc(dev, sizeof(struct s6e63m0), GFP_KERNEL);
>>> + if (!ctx)
>>> + return -ENOMEM;
>>> +
>>> + spi_set_drvdata(spi, ctx);
>>> +
>>> + ctx->dev = dev;
>>> + ctx->enabled = false;
>>> + ctx->prepared = false;
>>> +
>>> + ctx->supplies[0].supply = "vdd3";
>>> + ctx->supplies[1].supply = "vci";
>>> + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ctx->supplies),
>>> + ctx->supplies);
>>> + if (ret < 0) {
>>> + DRM_DEV_ERROR(dev, "failed to get regulators: %d\n", ret);
>>> + return ret;
>>> + }
>>> +
>>> + ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
>>> + if (IS_ERR(ctx->reset_gpio)) {
>>> + DRM_DEV_ERROR(dev, "cannot get reset-gpios %ld\n",
>>> + PTR_ERR(ctx->reset_gpio));
>>> + return PTR_ERR(ctx->reset_gpio);
>>> + }
>>> +
>>> + spi->bits_per_word = 9;
>>> + spi->mode = SPI_MODE_3;
>>> + ret = spi_setup(spi);
>>> + if (ret < 0) {
>>> + DRM_DEV_ERROR(dev, "spi setup failed.\n");
>>> + return ret;
>>> + }
>>> +
>>> + drm_panel_init(&ctx->panel);
>>> + ctx->panel.dev = dev;
>>> + ctx->panel.funcs = &s6e63m0_drm_funcs;
>>> +
>>> + ret = s6e63m0_backlight_register(ctx);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + return drm_panel_add(&ctx->panel);
>>> +}
>>> +
>>> +static int s6e63m0_remove(struct spi_device *spi)
>>> +{
>>> + struct s6e63m0 *ctx = spi_get_drvdata(spi);
>>> +
>>> + drm_panel_remove(&ctx->panel);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct of_device_id s6e63m0_of_match[] = {
>>> + { .compatible = "samsung,s6e63m0" },
>>> + { /* sentinel */ }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, s6e63m0_of_match);
>>> +
>>> +static struct spi_driver s6e63m0_driver = {
>>> + .probe = s6e63m0_probe,
>>> + .remove = s6e63m0_remove,
>>> + .driver = {
>>> + .name = "panel-samsung-s6e63m0",
>>> + .of_match_table = s6e63m0_of_match,
>>> + },
>>> +};
>>> +module_spi_driver(s6e63m0_driver);
>>> +
>>> +MODULE_AUTHOR("Paweł Chmiel <[email protected]>");
>>> +MODULE_DESCRIPTION("s6e63m0 LCD Driver");
>>> +MODULE_LICENSE("GPL v2");
>>
>>
>
>
>
>
>


2019-02-15 00:48:18

by Rob Herring (Arm)

[permalink] [raw]
Subject: Re: [PATCH v3 1/2] dt-bindings: drm: panel: Add Samsung s6e63m0 panel documentation

On Sat, 2 Feb 2019 15:27:47 +0100, =?UTF-8?q?Pawe=C5=82=20Chmiel?= wrote:
> From: Jonathan Bakker <[email protected]>
>
> This commit adds documentation for Samsung s6e63m0 AMOLED LCD panel
> driver.
>
> Signed-off-by: Jonathan Bakker <[email protected]>
> Signed-off-by: Paweł Chmiel <[email protected]>
> ---
> Changes from v1:
> - Add missing subject prefix
> - Rename reset-gpio to reset-gpios
> - Add link to spi properites documentation. They're required
> for driver to work
> - Removed delay properties, which are now hardcoded in driver
> - Removed display timings, which are now hardcoded in driver
> ---
> .../display/panel/samsung,s6e63m0.txt | 33 +++++++++++++++++++
> 1 file changed, 33 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/display/panel/samsung,s6e63m0.txt
>

Reviewed-by: Rob Herring <[email protected]>