[PATCH] drm/panel: add lg4573 driver

[PATCH] drm/panel: add lg4573 driver

[Heiko Schocher]

The patch adds LG4573 parallel RGB panel driver with SPI control interface.
The driver uses drm_panel framework.

Signed-off-by: Heiko Schocher <hs@denx.de>

---

 .../devicetree/bindings/panel/lg,lg4573.txt        |  42 +++
 drivers/gpu/drm/panel/Kconfig                      |   8 +
 drivers/gpu/drm/panel/Makefile                     |   1 +
 drivers/gpu/drm/panel/panel-lg4573.c               | 367 +++++++++++++++++++++
 4 files changed, 418 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/panel/lg,lg4573.txt
 create mode 100644 drivers/gpu/drm/panel/panel-lg4573.c

diff --git a/Documentation/devicetree/bindings/panel/lg,lg4573.txt b/Documentation/devicetree/bindings/panel/lg,lg4573.txt
new file mode 100644
index 0000000..55f495d
--- /dev/null
+++ b/Documentation/devicetree/bindings/panel/lg,lg4573.txt
@@ -0,0 +1,42 @@
+LG LG4573 TFT liquid crystal display with SPI control bus
+
+Required properties:
+  - compatible: "lg4573"
+  - reg: address of the panel on SPI bus
+  - display-timings: timings for the connected panel according to [1]
+
+The panel must obey rules for SPI slave device specified in document [2].
+
+Optional properties:
+  - power-on-delay: delay after turning regulators on [ms]
+
+[1]: Documentation/devicetree/bindings/video/display-timing.txt
+[2]: Documentation/devicetree/bindings/spi/spi-bus.txt
+
+Example:
+
+	lcd_panel: display@0 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "lg,lg4573";
+		spi-max-frequency = <10000000>;
+		reset-gpios = <&gpio2 11 0>;
+		reg = <0>;
+		power-on-delay = <10>;
+		display-timings {
+			480x800p57 {
+				native-mode;
+				clock-frequency = <27000027>;
+				hactive = <480>;
+				vactive = <800>;
+				hfront-porch = <10>;
+				hback-porch = <59>;
+				hsync-len = <10>;
+				vback-porch = <15>;
+				vfront-porch = <15>;
+				vsync-len = <15>;
+				hsync-active = <1>;
+				vsync-active = <1>;
+			};
+		};
+	};
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 6d64c7b..29c3407 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -23,6 +23,14 @@ config DRM_PANEL_LD9040
 	depends on OF && SPI
 	select VIDEOMODE_HELPERS
 
+config DRM_PANEL_LG4573
+	tristate "LG4573 RGB/SPI panel"
+	depends on OF && SPI
+	select VIDEOMODE_HELPERS
+	help
+	  Say Y here if you want to enable support for LG4573 RGB panel.
+	  To compile this driver as a module, choose M here.
+
 config DRM_PANEL_S6E8AA0
 	tristate "S6E8AA0 DSI video mode panel"
 	depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 4b2a043..715b95d 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -1,4 +1,5 @@
 obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
 obj-$(CONFIG_DRM_PANEL_LD9040) += panel-ld9040.o
+obj-$(CONFIG_DRM_PANEL_LG4573) += panel-lg4573.o
 obj-$(CONFIG_DRM_PANEL_S6E8AA0) += panel-s6e8aa0.o
 obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
diff --git a/drivers/gpu/drm/panel/panel-lg4573.c b/drivers/gpu/drm/panel/panel-lg4573.c
new file mode 100644
index 0000000..9d5e5a5
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-lg4573.c
@@ -0,0 +1,367 @@
+/*
+ *
+ * Copyright (C) 2015 Heiko Schocher <hs@denx.de>
+ *
+ * from:
+ * drivers/gpu/drm/panel/panel-ld9040.c
+ * ld9040 AMOLED LCD drm_panel driver.
+ *
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd
+ * Derived from drivers/video/backlight/ld9040.c
+ *
+ * Andrzej Hajda <a.hajda@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+*/
+
+#include <drm/drmP.h>
+#include <drm/drm_panel.h>
+
+#include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#include <video/mipi_display.h>
+#include <video/of_videomode.h>
+#include <video/videomode.h>
+
+struct lg4573 {
+	struct device *dev;
+	struct drm_panel panel;
+	u32 power_on_delay;
+	struct videomode vm;
+};
+
+static inline struct lg4573 *panel_to_lg4573(struct drm_panel *panel)
+{
+	return container_of(panel, struct lg4573, panel);
+}
+
+static int lg4573_spi_write_u16(struct lg4573 *ctx, u16 data)
+{
+	struct spi_device *spi = to_spi_device(ctx->dev);
+	struct spi_transfer xfer = {
+		.len		= 2,
+	};
+	struct spi_message msg;
+	u16 temp = htons(data);
+
+	dev_dbg(ctx->dev, "writing data: %x\n", data);
+	xfer.tx_buf = &temp;
+	spi_message_init(&msg);
+	spi_message_add_tail(&xfer, &msg);
+
+	return spi_sync(spi, &msg);
+}
+
+static void lg4573_spi_write_u16_array(struct lg4573 *ctx, u16 *buff, int size)
+{
+	int idx;
+
+	for (idx = 0; idx < size; idx++)
+		lg4573_spi_write_u16(ctx, buff[idx]);
+}
+
+static void lg4573_display_on(struct lg4573 *ctx)
+{
+	static u16 sleep_out = 0x7011;
+	static u16 display_on = 0x7029;
+
+	lg4573_spi_write_u16(ctx, sleep_out);
+	msleep(ctx->power_on_delay);
+	lg4573_spi_write_u16(ctx, display_on);
+}
+
+static int lg4573_display_off(struct lg4573 *ctx)
+{
+	static u16 display_off = 0x7028;
+	static u16 sleep_in	= 0x7010;
+
+	lg4573_spi_write_u16(ctx, display_off);
+	msleep(ctx->power_on_delay);
+	lg4573_spi_write_u16(ctx, sleep_in);
+	return 0;
+}
+
+static void lg4573_display_mode_settings(struct lg4573 *ctx)
+{
+	static u16 display_mode_settings[] = {
+	  0x703A,
+	  0x7270,
+	  0x70B1,
+	  0x7208,
+	  0x723B,
+	  0x720F,
+	  0x70B2,
+	  0x7200,
+	  0x72C8,
+	  0x70B3,
+	  0x7200,
+	  0x70B4,
+	  0x7200,
+	  0x70B5,
+	  0x7242,
+	  0x7210,
+	  0x7210,
+	  0x7200,
+	  0x7220,
+	  0x70B6,
+	  0x720B,
+	  0x720F,
+	  0x723C,
+	  0x7213,
+	  0x7213,
+	  0x72E8,
+	  0x70B7,
+	  0x7246,
+	  0x7206,
+	  0x720C,
+	  0x7200,
+	  0x7200,
+	};
+
+	dev_dbg(ctx->dev, "transfer display mode settings\n");
+	lg4573_spi_write_u16_array(ctx, display_mode_settings,
+			ARRAY_SIZE(display_mode_settings));
+}
+
+static void lg4573_power_settings(struct lg4573 *ctx)
+{
+	static u16 power_settings[] = {
+	  0x70C0,
+	  0x7201,
+	  0x7211,
+	  0x70C3,
+	  0x7207,
+	  0x7203,
+	  0x7204,
+	  0x7204,
+	  0x7204,
+	  0x70C4,
+	  0x7212,
+	  0x7224,
+	  0x7218,
+	  0x7218,
+	  0x7202,
+	  0x7249,
+	  0x70C5,
+	  0x726F,
+	  0x70C6,
+	  0x7241,
+	  0x7263,
+	};
+
+	dev_dbg(ctx->dev, "transfer power settings\n");
+	lg4573_spi_write_u16_array(ctx, power_settings,
+			ARRAY_SIZE(power_settings));
+}
+
+static void lg4573_gamma_settings(struct lg4573 *ctx)
+{
+	static u16 gamma_settings[] = {
+	  0x70D0,
+	  0x7203,
+	  0x7207,
+	  0x7273,
+	  0x7235,
+	  0x7200,
+	  0x7201,
+	  0x7220,
+	  0x7200,
+	  0x7203,
+	  0x70D1,
+	  0x7203,
+	  0x7207,
+	  0x7273,
+	  0x7235,
+	  0x7200,
+	  0x7201,
+	  0x7220,
+	  0x7200,
+	  0x7203,
+	  0x70D2,
+	  0x7203,
+	  0x7207,
+	  0x7273,
+	  0x7235,
+	  0x7200,
+	  0x7201,
+	  0x7220,
+	  0x7200,
+	  0x7203,
+	  0x70D3,
+	  0x7203,
+	  0x7207,
+	  0x7273,
+	  0x7235,
+	  0x7200,
+	  0x7201,
+	  0x7220,
+	  0x7200,
+	  0x7203,
+	  0x70D4,
+	  0x7203,
+	  0x7207,
+	  0x7273,
+	  0x7235,
+	  0x7200,
+	  0x7201,
+	  0x7220,
+	  0x7200,
+	  0x7203,
+	  0x70D5,
+	  0x7203,
+	  0x7207,
+	  0x7273,
+	  0x7235,
+	  0x7200,
+	  0x7201,
+	  0x7220,
+	  0x7200,
+	  0x7203,
+	};
+
+	dev_dbg(ctx->dev, "transfer gamma settings\n");
+	lg4573_spi_write_u16_array(ctx, gamma_settings,
+			ARRAY_SIZE(gamma_settings));
+}
+
+static void lg4573_init(struct lg4573 *ctx)
+{
+	dev_dbg(ctx->dev, "initializing LCD\n");
+
+	lg4573_display_mode_settings(ctx);
+	lg4573_power_settings(ctx);
+	lg4573_gamma_settings(ctx);
+}
+
+static int lg4573_power_on(struct lg4573 *ctx)
+{
+	msleep(ctx->power_on_delay);
+	lg4573_display_on(ctx);
+	return 0;
+}
+
+static int lg4573_disable(struct drm_panel *panel)
+{
+	struct lg4573 *ctx = panel_to_lg4573(panel);
+
+	return lg4573_display_off(ctx);
+}
+
+static int lg4573_enable(struct drm_panel *panel)
+{
+	struct lg4573 *ctx = panel_to_lg4573(panel);
+	int ret;
+
+	lg4573_init(ctx);
+
+	ret = lg4573_power_on(ctx);
+
+	return ret;
+}
+
+static int lg4573_get_modes(struct drm_panel *panel)
+{
+	struct drm_connector *connector = panel->connector;
+	struct lg4573 *ctx = panel_to_lg4573(panel);
+	struct drm_display_mode *mode;
+
+	mode = drm_mode_create(connector->dev);
+	if (!mode) {
+		DRM_ERROR("failed to create a new display mode\n");
+		return 0;
+	}
+
+	drm_display_mode_from_videomode(&ctx->vm, 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 lg4573_drm_funcs = {
+	.disable = lg4573_disable,
+	.enable = lg4573_enable,
+	.get_modes = lg4573_get_modes,
+};
+
+static int lg4573_parse_dt(struct lg4573 *ctx)
+{
+	struct device *dev = ctx->dev;
+	struct device_node *np = dev->of_node;
+	int ret;
+
+	ret = of_get_videomode(np, &ctx->vm, 0);
+	if (ret < 0)
+		return ret;
+
+	of_property_read_u32(np, "power-on-delay", &ctx->power_on_delay);
+
+	return 0;
+}
+
+static int lg4573_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct lg4573 *ctx;
+	int ret;
+
+	ctx = devm_kzalloc(dev, sizeof(struct lg4573), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	spi_set_drvdata(spi, ctx);
+	ctx->dev = dev;
+
+	ret = lg4573_parse_dt(ctx);
+	if (ret < 0)
+		return ret;
+
+	spi->bits_per_word = 8;
+	ret = spi_setup(spi);
+	if (ret < 0) {
+		dev_err(dev, "spi setup failed.\n");
+		return ret;
+	}
+
+	drm_panel_init(&ctx->panel);
+	ctx->panel.dev = dev;
+	ctx->panel.funcs = &lg4573_drm_funcs;
+
+	return drm_panel_add(&ctx->panel);
+}
+
+static int lg4573_remove(struct spi_device *spi)
+{
+	struct lg4573 *ctx = spi_get_drvdata(spi);
+
+	lg4573_display_off(ctx);
+	drm_panel_remove(&ctx->panel);
+
+	return 0;
+}
+
+static const struct of_device_id lg4573_of_match[] = {
+	{ .compatible = "lg,lg4573" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, lg4573_of_match);
+
+static struct spi_driver lg4573_driver = {
+	.probe		= lg4573_probe,
+	.remove		= lg4573_remove,
+	.driver = {
+		.name	= "lg4573",
+		.owner	= THIS_MODULE,
+		.of_match_table = lg4573_of_match,
+	},
+};
+module_spi_driver(lg4573_driver);
+
+MODULE_AUTHOR("Heiko Schocher <hs@denx.de>");
+MODULE_DESCRIPTION("lg4573 LCD Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.1.0

Re: [PATCH] drm/panel: add lg4573 driver

[Heiko Schocher]

Hello,

Am 06.05.2015 09:49, schrieb Heiko Schocher:
> The patch adds LG4573 parallel RGB panel driver with SPI control interface.
> The driver uses drm_panel framework.
>
> Signed-off-by: Heiko Schocher <hs@denx.de>
>
> ---
>
>   .../devicetree/bindings/panel/lg,lg4573.txt        |  42 +++
>   drivers/gpu/drm/panel/Kconfig                      |   8 +
>   drivers/gpu/drm/panel/Makefile                     |   1 +
>   drivers/gpu/drm/panel/panel-lg4573.c               | 367 +++++++++++++++++++++
>   4 files changed, 418 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/panel/lg,lg4573.txt
>   create mode 100644 drivers/gpu/drm/panel/panel-lg4573.c

ping...

bye,
Heiko
>
> diff --git a/Documentation/devicetree/bindings/panel/lg,lg4573.txt b/Documentation/devicetree/bindings/panel/lg,lg4573.txt
> new file mode 100644
> index 0000000..55f495d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/panel/lg,lg4573.txt
> @@ -0,0 +1,42 @@
> +LG LG4573 TFT liquid crystal display with SPI control bus
> +
> +Required properties:
> +  - compatible: "lg4573"
> +  - reg: address of the panel on SPI bus
> +  - display-timings: timings for the connected panel according to [1]
> +
> +The panel must obey rules for SPI slave device specified in document [2].
> +
> +Optional properties:
> +  - power-on-delay: delay after turning regulators on [ms]
> +
> +[1]: Documentation/devicetree/bindings/video/display-timing.txt
> +[2]: Documentation/devicetree/bindings/spi/spi-bus.txt
> +
> +Example:
> +
> +	lcd_panel: display@0 {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		compatible = "lg,lg4573";
> +		spi-max-frequency = <10000000>;
> +		reset-gpios = <&gpio2 11 0>;
> +		reg = <0>;
> +		power-on-delay = <10>;
> +		display-timings {
> +			480x800p57 {
> +				native-mode;
> +				clock-frequency = <27000027>;
> +				hactive = <480>;
> +				vactive = <800>;
> +				hfront-porch = <10>;
> +				hback-porch = <59>;
> +				hsync-len = <10>;
> +				vback-porch = <15>;
> +				vfront-porch = <15>;
> +				vsync-len = <15>;
> +				hsync-active = <1>;
> +				vsync-active = <1>;
> +			};
> +		};
> +	};
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 6d64c7b..29c3407 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -23,6 +23,14 @@ config DRM_PANEL_LD9040
>   	depends on OF && SPI
>   	select VIDEOMODE_HELPERS
>
> +config DRM_PANEL_LG4573
> +	tristate "LG4573 RGB/SPI panel"
> +	depends on OF && SPI
> +	select VIDEOMODE_HELPERS
> +	help
> +	  Say Y here if you want to enable support for LG4573 RGB panel.
> +	  To compile this driver as a module, choose M here.
> +
>   config DRM_PANEL_S6E8AA0
>   	tristate "S6E8AA0 DSI video mode panel"
>   	depends on OF
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index 4b2a043..715b95d 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -1,4 +1,5 @@
>   obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
>   obj-$(CONFIG_DRM_PANEL_LD9040) += panel-ld9040.o
> +obj-$(CONFIG_DRM_PANEL_LG4573) += panel-lg4573.o
>   obj-$(CONFIG_DRM_PANEL_S6E8AA0) += panel-s6e8aa0.o
>   obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
> diff --git a/drivers/gpu/drm/panel/panel-lg4573.c b/drivers/gpu/drm/panel/panel-lg4573.c
> new file mode 100644
> index 0000000..9d5e5a5
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-lg4573.c
> @@ -0,0 +1,367 @@
> +/*
> + *
> + * Copyright (C) 2015 Heiko Schocher <hs@denx.de>
> + *
> + * from:
> + * drivers/gpu/drm/panel/panel-ld9040.c
> + * ld9040 AMOLED LCD drm_panel driver.
> + *
> + * Copyright (c) 2014 Samsung Electronics Co., Ltd
> + * Derived from drivers/video/backlight/ld9040.c
> + *
> + * Andrzej Hajda <a.hajda@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> +*/
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_panel.h>
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#include <video/mipi_display.h>
> +#include <video/of_videomode.h>
> +#include <video/videomode.h>
> +
> +struct lg4573 {
> +	struct device *dev;
> +	struct drm_panel panel;
> +	u32 power_on_delay;
> +	struct videomode vm;
> +};
> +
> +static inline struct lg4573 *panel_to_lg4573(struct drm_panel *panel)
> +{
> +	return container_of(panel, struct lg4573, panel);
> +}
> +
> +static int lg4573_spi_write_u16(struct lg4573 *ctx, u16 data)
> +{
> +	struct spi_device *spi = to_spi_device(ctx->dev);
> +	struct spi_transfer xfer = {
> +		.len		= 2,
> +	};
> +	struct spi_message msg;
> +	u16 temp = htons(data);
> +
> +	dev_dbg(ctx->dev, "writing data: %x\n", data);
> +	xfer.tx_buf = &temp;
> +	spi_message_init(&msg);
> +	spi_message_add_tail(&xfer, &msg);
> +
> +	return spi_sync(spi, &msg);
> +}
> +
> +static void lg4573_spi_write_u16_array(struct lg4573 *ctx, u16 *buff, int size)
> +{
> +	int idx;
> +
> +	for (idx = 0; idx < size; idx++)
> +		lg4573_spi_write_u16(ctx, buff[idx]);
> +}
> +
> +static void lg4573_display_on(struct lg4573 *ctx)
> +{
> +	static u16 sleep_out = 0x7011;
> +	static u16 display_on = 0x7029;
> +
> +	lg4573_spi_write_u16(ctx, sleep_out);
> +	msleep(ctx->power_on_delay);
> +	lg4573_spi_write_u16(ctx, display_on);
> +}
> +
> +static int lg4573_display_off(struct lg4573 *ctx)
> +{
> +	static u16 display_off = 0x7028;
> +	static u16 sleep_in	= 0x7010;
> +
> +	lg4573_spi_write_u16(ctx, display_off);
> +	msleep(ctx->power_on_delay);
> +	lg4573_spi_write_u16(ctx, sleep_in);
> +	return 0;
> +}
> +
> +static void lg4573_display_mode_settings(struct lg4573 *ctx)
> +{
> +	static u16 display_mode_settings[] = {
> +	  0x703A,
> +	  0x7270,
> +	  0x70B1,
> +	  0x7208,
> +	  0x723B,
> +	  0x720F,
> +	  0x70B2,
> +	  0x7200,
> +	  0x72C8,
> +	  0x70B3,
> +	  0x7200,
> +	  0x70B4,
> +	  0x7200,
> +	  0x70B5,
> +	  0x7242,
> +	  0x7210,
> +	  0x7210,
> +	  0x7200,
> +	  0x7220,
> +	  0x70B6,
> +	  0x720B,
> +	  0x720F,
> +	  0x723C,
> +	  0x7213,
> +	  0x7213,
> +	  0x72E8,
> +	  0x70B7,
> +	  0x7246,
> +	  0x7206,
> +	  0x720C,
> +	  0x7200,
> +	  0x7200,
> +	};
> +
> +	dev_dbg(ctx->dev, "transfer display mode settings\n");
> +	lg4573_spi_write_u16_array(ctx, display_mode_settings,
> +			ARRAY_SIZE(display_mode_settings));
> +}
> +
> +static void lg4573_power_settings(struct lg4573 *ctx)
> +{
> +	static u16 power_settings[] = {
> +	  0x70C0,
> +	  0x7201,
> +	  0x7211,
> +	  0x70C3,
> +	  0x7207,
> +	  0x7203,
> +	  0x7204,
> +	  0x7204,
> +	  0x7204,
> +	  0x70C4,
> +	  0x7212,
> +	  0x7224,
> +	  0x7218,
> +	  0x7218,
> +	  0x7202,
> +	  0x7249,
> +	  0x70C5,
> +	  0x726F,
> +	  0x70C6,
> +	  0x7241,
> +	  0x7263,
> +	};
> +
> +	dev_dbg(ctx->dev, "transfer power settings\n");
> +	lg4573_spi_write_u16_array(ctx, power_settings,
> +			ARRAY_SIZE(power_settings));
> +}
> +
> +static void lg4573_gamma_settings(struct lg4573 *ctx)
> +{
> +	static u16 gamma_settings[] = {
> +	  0x70D0,
> +	  0x7203,
> +	  0x7207,
> +	  0x7273,
> +	  0x7235,
> +	  0x7200,
> +	  0x7201,
> +	  0x7220,
> +	  0x7200,
> +	  0x7203,
> +	  0x70D1,
> +	  0x7203,
> +	  0x7207,
> +	  0x7273,
> +	  0x7235,
> +	  0x7200,
> +	  0x7201,
> +	  0x7220,
> +	  0x7200,
> +	  0x7203,
> +	  0x70D2,
> +	  0x7203,
> +	  0x7207,
> +	  0x7273,
> +	  0x7235,
> +	  0x7200,
> +	  0x7201,
> +	  0x7220,
> +	  0x7200,
> +	  0x7203,
> +	  0x70D3,
> +	  0x7203,
> +	  0x7207,
> +	  0x7273,
> +	  0x7235,
> +	  0x7200,
> +	  0x7201,
> +	  0x7220,
> +	  0x7200,
> +	  0x7203,
> +	  0x70D4,
> +	  0x7203,
> +	  0x7207,
> +	  0x7273,
> +	  0x7235,
> +	  0x7200,
> +	  0x7201,
> +	  0x7220,
> +	  0x7200,
> +	  0x7203,
> +	  0x70D5,
> +	  0x7203,
> +	  0x7207,
> +	  0x7273,
> +	  0x7235,
> +	  0x7200,
> +	  0x7201,
> +	  0x7220,
> +	  0x7200,
> +	  0x7203,
> +	};
> +
> +	dev_dbg(ctx->dev, "transfer gamma settings\n");
> +	lg4573_spi_write_u16_array(ctx, gamma_settings,
> +			ARRAY_SIZE(gamma_settings));
> +}
> +
> +static void lg4573_init(struct lg4573 *ctx)
> +{
> +	dev_dbg(ctx->dev, "initializing LCD\n");
> +
> +	lg4573_display_mode_settings(ctx);
> +	lg4573_power_settings(ctx);
> +	lg4573_gamma_settings(ctx);
> +}
> +
> +static int lg4573_power_on(struct lg4573 *ctx)
> +{
> +	msleep(ctx->power_on_delay);
> +	lg4573_display_on(ctx);
> +	return 0;
> +}
> +
> +static int lg4573_disable(struct drm_panel *panel)
> +{
> +	struct lg4573 *ctx = panel_to_lg4573(panel);
> +
> +	return lg4573_display_off(ctx);
> +}
> +
> +static int lg4573_enable(struct drm_panel *panel)
> +{
> +	struct lg4573 *ctx = panel_to_lg4573(panel);
> +	int ret;
> +
> +	lg4573_init(ctx);
> +
> +	ret = lg4573_power_on(ctx);
> +
> +	return ret;
> +}
> +
> +static int lg4573_get_modes(struct drm_panel *panel)
> +{
> +	struct drm_connector *connector = panel->connector;
> +	struct lg4573 *ctx = panel_to_lg4573(panel);
> +	struct drm_display_mode *mode;
> +
> +	mode = drm_mode_create(connector->dev);
> +	if (!mode) {
> +		DRM_ERROR("failed to create a new display mode\n");
> +		return 0;
> +	}
> +
> +	drm_display_mode_from_videomode(&ctx->vm, 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 lg4573_drm_funcs = {
> +	.disable = lg4573_disable,
> +	.enable = lg4573_enable,
> +	.get_modes = lg4573_get_modes,
> +};
> +
> +static int lg4573_parse_dt(struct lg4573 *ctx)
> +{
> +	struct device *dev = ctx->dev;
> +	struct device_node *np = dev->of_node;
> +	int ret;
> +
> +	ret = of_get_videomode(np, &ctx->vm, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	of_property_read_u32(np, "power-on-delay", &ctx->power_on_delay);
> +
> +	return 0;
> +}
> +
> +static int lg4573_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct lg4573 *ctx;
> +	int ret;
> +
> +	ctx = devm_kzalloc(dev, sizeof(struct lg4573), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	spi_set_drvdata(spi, ctx);
> +	ctx->dev = dev;
> +
> +	ret = lg4573_parse_dt(ctx);
> +	if (ret < 0)
> +		return ret;
> +
> +	spi->bits_per_word = 8;
> +	ret = spi_setup(spi);
> +	if (ret < 0) {
> +		dev_err(dev, "spi setup failed.\n");
> +		return ret;
> +	}
> +
> +	drm_panel_init(&ctx->panel);
> +	ctx->panel.dev = dev;
> +	ctx->panel.funcs = &lg4573_drm_funcs;
> +
> +	return drm_panel_add(&ctx->panel);
> +}
> +
> +static int lg4573_remove(struct spi_device *spi)
> +{
> +	struct lg4573 *ctx = spi_get_drvdata(spi);
> +
> +	lg4573_display_off(ctx);
> +	drm_panel_remove(&ctx->panel);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id lg4573_of_match[] = {
> +	{ .compatible = "lg,lg4573" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, lg4573_of_match);
> +
> +static struct spi_driver lg4573_driver = {
> +	.probe		= lg4573_probe,
> +	.remove		= lg4573_remove,
> +	.driver = {
> +		.name	= "lg4573",
> +		.owner	= THIS_MODULE,
> +		.of_match_table = lg4573_of_match,
> +	},
> +};
> +module_spi_driver(lg4573_driver);
> +
> +MODULE_AUTHOR("Heiko Schocher <hs@denx.de>");
> +MODULE_DESCRIPTION("lg4573 LCD Driver");
> +MODULE_LICENSE("GPL v2");
>

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Re: [PATCH] drm/panel: add lg4573 driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 11460 bytes --]

On Wed, May 06, 2015 at 09:49:33AM +0200, Heiko Schocher wrote:
> The patch adds LG4573 parallel RGB panel driver with SPI control interface.
> The driver uses drm_panel framework.

This should be obvious by the location of the driver. Can you instead
provide information about the type or resolution of the panel?

>  .../devicetree/bindings/panel/lg,lg4573.txt        |  42 +++
>  drivers/gpu/drm/panel/Kconfig                      |   8 +
>  drivers/gpu/drm/panel/Makefile                     |   1 +
>  drivers/gpu/drm/panel/panel-lg4573.c               | 367 +++++++++++++++++++++
>  4 files changed, 418 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/panel/lg,lg4573.txt
>  create mode 100644 drivers/gpu/drm/panel/panel-lg4573.c
> 
> diff --git a/Documentation/devicetree/bindings/panel/lg,lg4573.txt b/Documentation/devicetree/bindings/panel/lg,lg4573.txt
> new file mode 100644
> index 0000000..55f495d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/panel/lg,lg4573.txt
> @@ -0,0 +1,42 @@
> +LG LG4573 TFT liquid crystal display with SPI control bus

Please capitalize "Liquid Crystal Display".

> +
> +Required properties:
> +  - compatible: "lg4573"

This is missing the vendor prefix.

> +  - reg: address of the panel on SPI bus

"on the"

> +  - display-timings: timings for the connected panel according to [1]

The timings are already implied by the compatible value, so there's no
need to list them in DT.

> +The panel must obey rules for SPI slave device specified in document [2].
> +
> +Optional properties:
> +  - power-on-delay: delay after turning regulators on [ms]

How is this a board-specific property? I'd assume that the same panel
always requires the same delay. Perhaps if this is board-specific it
really should be in the corresponding regulator's
regulator-enable-ramp-delay? Then again the binding doesn't describe any
regulators, so what exactly is this delay used for?

> +
> +[1]: Documentation/devicetree/bindings/video/display-timing.txt
> +[2]: Documentation/devicetree/bindings/spi/spi-bus.txt
> +
> +Example:
> +
> +	lcd_panel: display@0 {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		compatible = "lg,lg4573";
> +		spi-max-frequency = <10000000>;
> +		reset-gpios = <&gpio2 11 0>;

This isn't documented above.

> +		reg = <0>;
> +		power-on-delay = <10>;
> +		display-timings {
> +			480x800p57 {
> +				native-mode;
> +				clock-frequency = <27000027>;
> +				hactive = <480>;
> +				vactive = <800>;
> +				hfront-porch = <10>;
> +				hback-porch = <59>;
> +				hsync-len = <10>;
> +				vback-porch = <15>;
> +				vfront-porch = <15>;
> +				vsync-len = <15>;
> +				hsync-active = <1>;
> +				vsync-active = <1>;
> +			};
> +		};
> +	};
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 6d64c7b..29c3407 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -23,6 +23,14 @@ config DRM_PANEL_LD9040
>  	depends on OF && SPI
>  	select VIDEOMODE_HELPERS
>  
> +config DRM_PANEL_LG4573
> +	tristate "LG4573 RGB/SPI panel"

I'd like to get into the habit of prefixing the panels with a vendor
prefix, so this should become something like:

	config DRM_PANEL_LG_LG4573
		tristate "LG LG4573 RGB/SPI panel"

I have a local patch that converts existing boards, so with this
conversion it'd fit right it.

> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index 4b2a043..715b95d 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -1,4 +1,5 @@
>  obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
>  obj-$(CONFIG_DRM_PANEL_LD9040) += panel-ld9040.o
> +obj-$(CONFIG_DRM_PANEL_LG4573) += panel-lg4573.o

Similarly for filenames, this would become: panel-lg-lg4573.c

>  obj-$(CONFIG_DRM_PANEL_S6E8AA0) += panel-s6e8aa0.o
>  obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
> diff --git a/drivers/gpu/drm/panel/panel-lg4573.c b/drivers/gpu/drm/panel/panel-lg4573.c
> new file mode 100644
> index 0000000..9d5e5a5
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-lg4573.c
> @@ -0,0 +1,367 @@
> +/*
> + *
> + * Copyright (C) 2015 Heiko Schocher <hs@denx.de>
> + *
> + * from:
> + * drivers/gpu/drm/panel/panel-ld9040.c
> + * ld9040 AMOLED LCD drm_panel driver.
> + *
> + * Copyright (c) 2014 Samsung Electronics Co., Ltd
> + * Derived from drivers/video/backlight/ld9040.c
> + *
> + * Andrzej Hajda <a.hajda@samsung.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> +*/
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_panel.h>
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#include <video/mipi_display.h>
> +#include <video/of_videomode.h>
> +#include <video/videomode.h>
> +
> +struct lg4573 {
> +	struct device *dev;
> +	struct drm_panel panel;

Might be a slight optimization to put panel first in the structure.

> +	u32 power_on_delay;
> +	struct videomode vm;
> +};
> +
> +static inline struct lg4573 *panel_to_lg4573(struct drm_panel *panel)
> +{
> +	return container_of(panel, struct lg4573, panel);
> +}
> +
> +static int lg4573_spi_write_u16(struct lg4573 *ctx, u16 data)
> +{
> +	struct spi_device *spi = to_spi_device(ctx->dev);
> +	struct spi_transfer xfer = {
> +		.len		= 2,

No need for this padding. A single space will do.

> +	};
> +	struct spi_message msg;
> +	u16 temp = htons(data);

htons() always has this association to network programming. Perhaps it'd
be better to use cpu_to_be16() here?

> +
> +	dev_dbg(ctx->dev, "writing data: %x\n", data);
> +	xfer.tx_buf = &temp;
> +	spi_message_init(&msg);
> +	spi_message_add_tail(&xfer, &msg);
> +
> +	return spi_sync(spi, &msg);
> +}
> +
> +static void lg4573_spi_write_u16_array(struct lg4573 *ctx, u16 *buff, int size)

size should be of type size_t. Or in this case it's really a count, so
perhaps rename to count and make it unsigned int.

> +{
> +	int idx;

Then this would become unsigned int as well. And a more idiomatic name
would be i.

> +
> +	for (idx = 0; idx < size; idx++)
> +		lg4573_spi_write_u16(ctx, buff[idx]);
> +}

You completely ignore errors.

> +static void lg4573_display_on(struct lg4573 *ctx)
> +{
> +	static u16 sleep_out = 0x7011;
> +	static u16 display_on = 0x7029;

These seem to be (0x70 << 8 | MIPI_DCS_EXIT_SLEEP) and (0x70 << 8 |
MIPI_DCS_SET_DISPLAY_ON). Perhaps that 0x70 just means it's followed by
a MIPI DCS command, so I'm thinking perhaps you should add a function
such as lg4573_spi_write_dcs() which only takes the DCS command to avoid
all the repetition.

> +
> +	lg4573_spi_write_u16(ctx, sleep_out);
> +	msleep(ctx->power_on_delay);
> +	lg4573_spi_write_u16(ctx, display_on);
> +}

This also ignores errors. And the post-regulator delay is used here but
I don't see any regulators being powered up. According to the MIPI DCS
specification there needs to be a delay of 5 ms after the EXIT_SLEEP
command and any other command (120 ms if the command is ENTER_SLEEP).
Perhaps that's what you're after here? It would mean that the delay is
not board-specific and hence doesn't belong in DT.

> +static int lg4573_display_off(struct lg4573 *ctx)
> +{
> +	static u16 display_off = 0x7028;
> +	static u16 sleep_in	= 0x7010;

More standard DCS commands. Also unnecessary tab, it should be a single
space instead.

> +
> +	lg4573_spi_write_u16(ctx, display_off);
> +	msleep(ctx->power_on_delay);
> +	lg4573_spi_write_u16(ctx, sleep_in);
> +	return 0;
> +}

Also it's weird that this function returns a value. It always returns 0
so there's no point, really. You should perhaps think about propagating
any errors from the SPI write.

> +
> +static void lg4573_display_mode_settings(struct lg4573 *ctx)
> +{
> +	static u16 display_mode_settings[] = {
> +	  0x703A,
[...]
> +	  0x7200,
> +	};

Please make use of the 78/80 columns. Also, I don't suppose it'd be
possible to obtain symbolic names for these magic numbers? More of the
same below.

> +static void lg4573_init(struct lg4573 *ctx)
> +{
> +	dev_dbg(ctx->dev, "initializing LCD\n");
> +
> +	lg4573_display_mode_settings(ctx);
> +	lg4573_power_settings(ctx);
> +	lg4573_gamma_settings(ctx);
> +}
> +
> +static int lg4573_power_on(struct lg4573 *ctx)
> +{
> +	msleep(ctx->power_on_delay);
> +	lg4573_display_on(ctx);
> +	return 0;
> +}
> +
> +static int lg4573_disable(struct drm_panel *panel)
> +{
> +	struct lg4573 *ctx = panel_to_lg4573(panel);
> +
> +	return lg4573_display_off(ctx);
> +}
> +
> +static int lg4573_enable(struct drm_panel *panel)
> +{
> +	struct lg4573 *ctx = panel_to_lg4573(panel);
> +	int ret;
> +
> +	lg4573_init(ctx);
> +
> +	ret = lg4573_power_on(ctx);
> +
> +	return ret;
> +}

I think these should all propagate errors.

> +static int lg4573_get_modes(struct drm_panel *panel)
> +{
> +	struct drm_connector *connector = panel->connector;
> +	struct lg4573 *ctx = panel_to_lg4573(panel);
> +	struct drm_display_mode *mode;
> +
> +	mode = drm_mode_create(connector->dev);
> +	if (!mode) {
> +		DRM_ERROR("failed to create a new display mode\n");
> +		return 0;
> +	}
> +
> +	drm_display_mode_from_videomode(&ctx->vm, mode);
> +
> +	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> +	drm_mode_probed_add(connector, mode);
> +
> +	return 1;
> +}

You can either use a hard-coded mode or use display timings along with
the helpers to convert the timings to a default mode. No need to parse
the information from DT.

> +static const struct drm_panel_funcs lg4573_drm_funcs = {
> +	.disable = lg4573_disable,
> +	.enable = lg4573_enable,
> +	.get_modes = lg4573_get_modes,
> +};
> +
> +static int lg4573_parse_dt(struct lg4573 *ctx)
> +{
> +	struct device *dev = ctx->dev;
> +	struct device_node *np = dev->of_node;
> +	int ret;
> +
> +	ret = of_get_videomode(np, &ctx->vm, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	of_property_read_u32(np, "power-on-delay", &ctx->power_on_delay);
> +
> +	return 0;
> +}
> +
> +static int lg4573_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct lg4573 *ctx;
> +	int ret;
> +
> +	ctx = devm_kzalloc(dev, sizeof(struct lg4573), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	spi_set_drvdata(spi, ctx);
> +	ctx->dev = dev;
> +
> +	ret = lg4573_parse_dt(ctx);
> +	if (ret < 0)
> +		return ret;
> +
> +	spi->bits_per_word = 8;
> +	ret = spi_setup(spi);
> +	if (ret < 0) {
> +		dev_err(dev, "spi setup failed.\n");

You might want to include the error code in the message. Also "SPI".

> +static const struct of_device_id lg4573_of_match[] = {
> +	{ .compatible = "lg,lg4573" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, lg4573_of_match);
> +
> +static struct spi_driver lg4573_driver = {
> +	.probe		= lg4573_probe,
> +	.remove		= lg4573_remove,
> +	.driver = {
> +		.name	= "lg4573",
> +		.owner	= THIS_MODULE,
> +		.of_match_table = lg4573_of_match,
> +	},

No need for the padding. It's not consistent anyway.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] drm/panel: add lg4573 driver

[Heiko Schocher]

Hello Thierry,

Am 05.06.2015 14:19, schrieb Thierry Reding:
> On Wed, May 06, 2015 at 09:49:33AM +0200, Heiko Schocher wrote:
>> The patch adds LG4573 parallel RGB panel driver with SPI control interface.
>> The driver uses drm_panel framework.
>
> This should be obvious by the location of the driver. Can you instead
> provide information about the type or resolution of the panel?
>
>>   .../devicetree/bindings/panel/lg,lg4573.txt        |  42 +++
>>   drivers/gpu/drm/panel/Kconfig                      |   8 +
>>   drivers/gpu/drm/panel/Makefile                     |   1 +
>>   drivers/gpu/drm/panel/panel-lg4573.c               | 367 +++++++++++++++++++++
>>   4 files changed, 418 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/panel/lg,lg4573.txt
>>   create mode 100644 drivers/gpu/drm/panel/panel-lg4573.c
>>
>> diff --git a/Documentation/devicetree/bindings/panel/lg,lg4573.txt b/Documentation/devicetree/bindings/panel/lg,lg4573.txt
>> new file mode 100644
>> index 0000000..55f495d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/panel/lg,lg4573.txt
>> @@ -0,0 +1,42 @@
>> +LG LG4573 TFT liquid crystal display with SPI control bus
>
> Please capitalize "Liquid Crystal Display".

done.

>> +
>> +Required properties:
>> +  - compatible: "lg4573"
>
> This is missing the vendor prefix.

fixed.

>> +  - reg: address of the panel on SPI bus
>
> "on the"

fixed.

>> +  - display-timings: timings for the connected panel according to [1]
>
> The timings are already implied by the compatible value, so there's no
> need to list them in DT.

I look into it ... is there an example for a panel driver with fixed
timings? Should I do it like it is done in the
drivers/gpu/drm/panel/panel-simple.c driver?

>> +The panel must obey rules for SPI slave device specified in document [2].
>> +
>> +Optional properties:
>> +  - power-on-delay: delay after turning regulators on [ms]
>
> How is this a board-specific property? I'd assume that the same panel
> always requires the same delay. Perhaps if this is board-specific it
> really should be in the corresponding regulator's
> regulator-enable-ramp-delay? Then again the binding doesn't describe any
> regulators, so what exactly is this delay used for?

I rework this, and drop it.

>> +
>> +[1]: Documentation/devicetree/bindings/video/display-timing.txt
>> +[2]: Documentation/devicetree/bindings/spi/spi-bus.txt
>> +
>> +Example:
>> +
>> +	lcd_panel: display@0 {
>> +		#address-cells = <1>;
>> +		#size-cells = <1>;
>> +		compatible = "lg,lg4573";
>> +		spi-max-frequency = <10000000>;
>> +		reset-gpios = <&gpio2 11 0>;
>
> This isn't documented above.

removed.

>> +		reg = <0>;
>> +		power-on-delay = <10>;
>> +		display-timings {
>> +			480x800p57 {
>> +				native-mode;
>> +				clock-frequency = <27000027>;
>> +				hactive = <480>;
>> +				vactive = <800>;
>> +				hfront-porch = <10>;
>> +				hback-porch = <59>;
>> +				hsync-len = <10>;
>> +				vback-porch = <15>;
>> +				vfront-porch = <15>;
>> +				vsync-len = <15>;
>> +				hsync-active = <1>;
>> +				vsync-active = <1>;
>> +			};
>> +		};
>> +	};
>> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
>> index 6d64c7b..29c3407 100644
>> --- a/drivers/gpu/drm/panel/Kconfig
>> +++ b/drivers/gpu/drm/panel/Kconfig
>> @@ -23,6 +23,14 @@ config DRM_PANEL_LD9040
>>   	depends on OF && SPI
>>   	select VIDEOMODE_HELPERS
>>
>> +config DRM_PANEL_LG4573
>> +	tristate "LG4573 RGB/SPI panel"
>
> I'd like to get into the habit of prefixing the panels with a vendor
> prefix, so this should become something like:
>
> 	config DRM_PANEL_LG_LG4573
> 		tristate "LG LG4573 RGB/SPI panel"
>
> I have a local patch that converts existing boards, so with this
> conversion it'd fit right it.

done.

>> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
>> index 4b2a043..715b95d 100644
>> --- a/drivers/gpu/drm/panel/Makefile
>> +++ b/drivers/gpu/drm/panel/Makefile
>> @@ -1,4 +1,5 @@
>>   obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
>>   obj-$(CONFIG_DRM_PANEL_LD9040) += panel-ld9040.o
>> +obj-$(CONFIG_DRM_PANEL_LG4573) += panel-lg4573.o
>
> Similarly for filenames, this would become: panel-lg-lg4573.c

done.

>>   obj-$(CONFIG_DRM_PANEL_S6E8AA0) += panel-s6e8aa0.o
>>   obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
>> diff --git a/drivers/gpu/drm/panel/panel-lg4573.c b/drivers/gpu/drm/panel/panel-lg4573.c
>> new file mode 100644
>> index 0000000..9d5e5a5
>> --- /dev/null
>> +++ b/drivers/gpu/drm/panel/panel-lg4573.c
>> @@ -0,0 +1,367 @@
>> +/*
>> + *
>> + * Copyright (C) 2015 Heiko Schocher <hs@denx.de>
>> + *
>> + * from:
>> + * drivers/gpu/drm/panel/panel-ld9040.c
>> + * ld9040 AMOLED LCD drm_panel driver.
>> + *
>> + * Copyright (c) 2014 Samsung Electronics Co., Ltd
>> + * Derived from drivers/video/backlight/ld9040.c
>> + *
>> + * Andrzej Hajda <a.hajda@samsung.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> +*/
>> +
>> +#include <drm/drmP.h>
>> +#include <drm/drm_panel.h>
>> +
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/spi/spi.h>
>> +
>> +#include <video/mipi_display.h>
>> +#include <video/of_videomode.h>
>> +#include <video/videomode.h>
>> +
>> +struct lg4573 {
>> +	struct device *dev;
>> +	struct drm_panel panel;
>
> Might be a slight optimization to put panel first in the structure.

fixed.

>> +	u32 power_on_delay;
>> +	struct videomode vm;
>> +};
>> +
>> +static inline struct lg4573 *panel_to_lg4573(struct drm_panel *panel)
>> +{
>> +	return container_of(panel, struct lg4573, panel);
>> +}
>> +
>> +static int lg4573_spi_write_u16(struct lg4573 *ctx, u16 data)
>> +{
>> +	struct spi_device *spi = to_spi_device(ctx->dev);
>> +	struct spi_transfer xfer = {
>> +		.len		= 2,
>
> No need for this padding. A single space will do.

fixed.

>> +	};
>> +	struct spi_message msg;
>> +	u16 temp = htons(data);
>
> htons() always has this association to network programming. Perhaps it'd
> be better to use cpu_to_be16() here?

yep, fixed.

>> +
>> +	dev_dbg(ctx->dev, "writing data: %x\n", data);
>> +	xfer.tx_buf = &temp;
>> +	spi_message_init(&msg);
>> +	spi_message_add_tail(&xfer, &msg);
>> +
>> +	return spi_sync(spi, &msg);
>> +}
>> +
>> +static void lg4573_spi_write_u16_array(struct lg4573 *ctx, u16 *buff, int size)
>
> size should be of type size_t. Or in this case it's really a count, so
> perhaps rename to count and make it unsigned int.
>
>> +{
>> +	int idx;
>
> Then this would become unsigned int as well. And a more idiomatic name
> would be i.
>
>> +
>> +	for (idx = 0; idx < size; idx++)
>> +		lg4573_spi_write_u16(ctx, buff[idx]);
>> +}
>
> You completely ignore errors.

reworked this function complete and added error checking everywhere.

>> +static void lg4573_display_on(struct lg4573 *ctx)
>> +{
>> +	static u16 sleep_out = 0x7011;
>> +	static u16 display_on = 0x7029;
>
> These seem to be (0x70 << 8 | MIPI_DCS_EXIT_SLEEP) and (0x70 << 8 |
> MIPI_DCS_SET_DISPLAY_ON). Perhaps that 0x70 just means it's followed by
> a MIPI DCS command, so I'm thinking perhaps you should add a function
> such as lg4573_spi_write_dcs() which only takes the DCS command to avoid
> all the repetition.

done.

>> +
>> +	lg4573_spi_write_u16(ctx, sleep_out);
>> +	msleep(ctx->power_on_delay);
>> +	lg4573_spi_write_u16(ctx, display_on);
>> +}
>
> This also ignores errors. And the post-regulator delay is used here but
> I don't see any regulators being powered up. According to the MIPI DCS
> specification there needs to be a delay of 5 ms after the EXIT_SLEEP
> command and any other command (120 ms if the command is ENTER_SLEEP).
> Perhaps that's what you're after here? It would mean that the delay is
> not board-specific and hence doesn't belong in DT.

removed.

>> +static int lg4573_display_off(struct lg4573 *ctx)
>> +{
>> +	static u16 display_off = 0x7028;
>> +	static u16 sleep_in	= 0x7010;
>
> More standard DCS commands. Also unnecessary tab, it should be a single
> space instead.

reworked.

>> +
>> +	lg4573_spi_write_u16(ctx, display_off);
>> +	msleep(ctx->power_on_delay);
>> +	lg4573_spi_write_u16(ctx, sleep_in);
>> +	return 0;
>> +}
>
> Also it's weird that this function returns a value. It always returns 0
> so there's no point, really. You should perhaps think about propagating
> any errors from the SPI write.

Yes, reworked.

>> +
>> +static void lg4573_display_mode_settings(struct lg4573 *ctx)
>> +{
>> +	static u16 display_mode_settings[] = {
>> +	  0x703A,
> [...]
>> +	  0x7200,
>> +	};
>
> Please make use of the 78/80 columns. Also, I don't suppose it'd be
> possible to obtain symbolic names for these magic numbers? More of the
> same below.

Fixed ... I try to find out more about this magic numbers, but I
can;t promise it ...

>> +static void lg4573_init(struct lg4573 *ctx)
>> +{
>> +	dev_dbg(ctx->dev, "initializing LCD\n");
>> +
>> +	lg4573_display_mode_settings(ctx);
>> +	lg4573_power_settings(ctx);
>> +	lg4573_gamma_settings(ctx);
>> +}
>> +
>> +static int lg4573_power_on(struct lg4573 *ctx)
>> +{
>> +	msleep(ctx->power_on_delay);
>> +	lg4573_display_on(ctx);
>> +	return 0;
>> +}
>> +
>> +static int lg4573_disable(struct drm_panel *panel)
>> +{
>> +	struct lg4573 *ctx = panel_to_lg4573(panel);
>> +
>> +	return lg4573_display_off(ctx);
>> +}
>> +
>> +static int lg4573_enable(struct drm_panel *panel)
>> +{
>> +	struct lg4573 *ctx = panel_to_lg4573(panel);
>> +	int ret;
>> +
>> +	lg4573_init(ctx);
>> +
>> +	ret = lg4573_power_on(ctx);
>> +
>> +	return ret;
>> +}
>
> I think these should all propagate errors.

fixed.

>> +static int lg4573_get_modes(struct drm_panel *panel)
>> +{
>> +	struct drm_connector *connector = panel->connector;
>> +	struct lg4573 *ctx = panel_to_lg4573(panel);
>> +	struct drm_display_mode *mode;
>> +
>> +	mode = drm_mode_create(connector->dev);
>> +	if (!mode) {
>> +		DRM_ERROR("failed to create a new display mode\n");
>> +		return 0;
>> +	}
>> +
>> +	drm_display_mode_from_videomode(&ctx->vm, mode);
>> +
>> +	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
>> +	drm_mode_probed_add(connector, mode);
>> +
>> +	return 1;
>> +}
>
> You can either use a hard-coded mode or use display timings along with
> the helpers to convert the timings to a default mode. No need to parse
> the information from DT.

Ok... see question above, could I do it like it is done in the
panel-simple driver? Or is there another way?

>> +static const struct drm_panel_funcs lg4573_drm_funcs = {
>> +	.disable = lg4573_disable,
>> +	.enable = lg4573_enable,
>> +	.get_modes = lg4573_get_modes,
>> +};
>> +
>> +static int lg4573_parse_dt(struct lg4573 *ctx)
>> +{
>> +	struct device *dev = ctx->dev;
>> +	struct device_node *np = dev->of_node;
>> +	int ret;
>> +
>> +	ret = of_get_videomode(np, &ctx->vm, 0);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	of_property_read_u32(np, "power-on-delay", &ctx->power_on_delay);
>> +
>> +	return 0;
>> +}
>> +
>> +static int lg4573_probe(struct spi_device *spi)
>> +{
>> +	struct device *dev = &spi->dev;
>> +	struct lg4573 *ctx;
>> +	int ret;
>> +
>> +	ctx = devm_kzalloc(dev, sizeof(struct lg4573), GFP_KERNEL);
>> +	if (!ctx)
>> +		return -ENOMEM;
>> +
>> +	spi_set_drvdata(spi, ctx);
>> +	ctx->dev = dev;
>> +
>> +	ret = lg4573_parse_dt(ctx);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	spi->bits_per_word = 8;
>> +	ret = spi_setup(spi);
>> +	if (ret < 0) {
>> +		dev_err(dev, "spi setup failed.\n");
>
> You might want to include the error code in the message. Also "SPI".

done.

>> +static const struct of_device_id lg4573_of_match[] = {
>> +	{ .compatible = "lg,lg4573" },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, lg4573_of_match);
>> +
>> +static struct spi_driver lg4573_driver = {
>> +	.probe		= lg4573_probe,
>> +	.remove		= lg4573_remove,
>> +	.driver = {
>> +		.name	= "lg4573",
>> +		.owner	= THIS_MODULE,
>> +		.of_match_table = lg4573_of_match,
>> +	},
>
> No need for the padding. It's not consistent anyway.

fixed.

Thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Re: [PATCH] drm/panel: add lg4573 driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3154 bytes --]

On Mon, Jun 08, 2015 at 10:13:21AM +0200, Heiko Schocher wrote:
> Hello Thierry,
> 
> Am 05.06.2015 14:19, schrieb Thierry Reding:
> >On Wed, May 06, 2015 at 09:49:33AM +0200, Heiko Schocher wrote:
[...]
> >>+  - display-timings: timings for the connected panel according to [1]
> >
> >The timings are already implied by the compatible value, so there's no
> >need to list them in DT.
> 
> I look into it ... is there an example for a panel driver with fixed
> timings? Should I do it like it is done in the
> drivers/gpu/drm/panel/panel-simple.c driver?

The simple-panel driver actually implements two methods. The old method
is to provide a fixed mode. That works fairly well, but primarily
because we don't support very many boards in the upstream kernel. For a
couple of panels it was shown that the fixed mode doesn't work very
well. One of the reasons was that the mode used values that conflicted
with the restrictions of the display controller on another SoC. A more
future-proof way would be for the driver to expose the display timings.
That allows the driver to compute a "default" mode, but also provide the
display driver with a full range of valid timings so that an appropriate
mode can be computed, taking into account the restrictions of the
display hardware.

The two Hannstar panels supported by the driver use this mechanism.
Perhaps you can use those for reference.

> >>+static void lg4573_display_mode_settings(struct lg4573 *ctx)
> >>+{
> >>+	static u16 display_mode_settings[] = {
> >>+	  0x703A,
> >[...]
> >>+	  0x7200,
> >>+	};
> >
> >Please make use of the 78/80 columns. Also, I don't suppose it'd be
> >possible to obtain symbolic names for these magic numbers? More of the
> >same below.
> 
> Fixed ... I try to find out more about this magic numbers, but I
> can;t promise it ...

It's not usual to get full documentation on these numbers, but we should
at least try.

> >>+static int lg4573_get_modes(struct drm_panel *panel)
> >>+{
> >>+	struct drm_connector *connector = panel->connector;
> >>+	struct lg4573 *ctx = panel_to_lg4573(panel);
> >>+	struct drm_display_mode *mode;
> >>+
> >>+	mode = drm_mode_create(connector->dev);
> >>+	if (!mode) {
> >>+		DRM_ERROR("failed to create a new display mode\n");
> >>+		return 0;
> >>+	}
> >>+
> >>+	drm_display_mode_from_videomode(&ctx->vm, mode);
> >>+
> >>+	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> >>+	drm_mode_probed_add(connector, mode);
> >>+
> >>+	return 1;
> >>+}
> >
> >You can either use a hard-coded mode or use display timings along with
> >the helpers to convert the timings to a default mode. No need to parse
> >the information from DT.
> 
> Ok... see question above, could I do it like it is done in the
> panel-simple driver? Or is there another way?

Look at the panel_simple_get_fixed_modes() implementation. That computes
a default mode from the typical values of the display timings.
Alternatively you can also implement display timings support in your
display driver and directly use the ->get_timings() callback for the
panel you have.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]