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