Message-ID: <55754EA1.40009@denx.de>
Date:	Mon, 08 Jun 2015 10:13:21 +0200
From:	Heiko Schocher <hs@...x.de>
To:	Thierry Reding <thierry.reding@...il.com>
CC:	linux-kernel@...r.kernel.org, David Airlie <airlied@...ux.ie>,
	dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH] drm/panel: add lg4573 driver

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@...x.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@...sung.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists