From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1752181AbbFEMUH (ORCPT <rfc822;w@1wt.eu>);
	Fri, 5 Jun 2015 08:20:07 -0400
Received: from mail-qc0-f182.google.com ([209.85.216.182]:34218 "EHLO
	mail-qc0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751703AbbFEMUD (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Fri, 5 Jun 2015 08:20:03 -0400
Date: Fri, 5 Jun 2015 14:19:39 +0200
From: Thierry Reding <thierry.reding@gmail.com>
To: Heiko Schocher <hs@denx.de>
Cc: linux-kernel@vger.kernel.org, David Airlie <airlied@linux.ie>,
        dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/panel: add lg4573 driver
Message-ID: <20150605121934.GA26656@ulmo.nvidia.com>
References: <1430898573-14783-1-git-send-email-hs@denx.de>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="pf9I7BMVVzbSWLtt"
Content-Disposition: inline
In-Reply-To: <1430898573-14783-1-git-send-email-hs@denx.de>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


--pf9I7BMVVzbSWLtt
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, May 06, 2015 at 09:49:33AM +0200, Heiko Schocher wrote:
> The patch adds LG4573 parallel RGB panel driver with SPI control interfac=
e.
> 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
>=20
> diff --git a/Documentation/devicetree/bindings/panel/lg,lg4573.txt b/Docu=
mentation/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 =3D <1>;
> +		#size-cells =3D <1>;
> +		compatible =3D "lg,lg4573";
> +		spi-max-frequency =3D <10000000>;
> +		reset-gpios =3D <&gpio2 11 0>;

This isn't documented above.

> +		reg =3D <0>;
> +		power-on-delay =3D <10>;
> +		display-timings {
> +			480x800p57 {
> +				native-mode;
> +				clock-frequency =3D <27000027>;
> +				hactive =3D <480>;
> +				vactive =3D <800>;
> +				hfront-porch =3D <10>;
> +				hback-porch =3D <59>;
> +				hsync-len =3D <10>;
> +				vback-porch =3D <15>;
> +				vfront-porch =3D <15>;
> +				vsync-len =3D <15>;
> +				hsync-active =3D <1>;
> +				vsync-active =3D <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
> =20
> +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/Makef=
ile
> 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) +=3D panel-simple.o
>  obj-$(CONFIG_DRM_PANEL_LD9040) +=3D panel-ld9040.o
> +obj-$(CONFIG_DRM_PANEL_LG4573) +=3D panel-lg4573.o

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

>  obj-$(CONFIG_DRM_PANEL_S6E8AA0) +=3D panel-s6e8aa0.o
>  obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) +=3D 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 =3D to_spi_device(ctx->dev);
> +	struct spi_transfer xfer =3D {
> +		.len		=3D 2,

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

> +	};
> +	struct spi_message msg;
> +	u16 temp =3D 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 =3D &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, in=
t 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 =3D 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 =3D 0x7011;
> +	static u16 display_on =3D 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 =3D 0x7028;
> +	static u16 sleep_in	=3D 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[] =3D {
> +	  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 =3D panel_to_lg4573(panel);
> +
> +	return lg4573_display_off(ctx);
> +}
> +
> +static int lg4573_enable(struct drm_panel *panel)
> +{
> +	struct lg4573 *ctx =3D panel_to_lg4573(panel);
> +	int ret;
> +
> +	lg4573_init(ctx);
> +
> +	ret =3D 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 =3D panel->connector;
> +	struct lg4573 *ctx =3D panel_to_lg4573(panel);
> +	struct drm_display_mode *mode;
> +
> +	mode =3D 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 =3D 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 =3D {
> +	.disable =3D lg4573_disable,
> +	.enable =3D lg4573_enable,
> +	.get_modes =3D lg4573_get_modes,
> +};
> +
> +static int lg4573_parse_dt(struct lg4573 *ctx)
> +{
> +	struct device *dev =3D ctx->dev;
> +	struct device_node *np =3D dev->of_node;
> +	int ret;
> +
> +	ret =3D 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 =3D &spi->dev;
> +	struct lg4573 *ctx;
> +	int ret;
> +
> +	ctx =3D devm_kzalloc(dev, sizeof(struct lg4573), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	spi_set_drvdata(spi, ctx);
> +	ctx->dev =3D dev;
> +
> +	ret =3D lg4573_parse_dt(ctx);
> +	if (ret < 0)
> +		return ret;
> +
> +	spi->bits_per_word =3D 8;
> +	ret =3D 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[] =3D {
> +	{ .compatible =3D "lg,lg4573" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, lg4573_of_match);
> +
> +static struct spi_driver lg4573_driver =3D {
> +	.probe		=3D lg4573_probe,
> +	.remove		=3D lg4573_remove,
> +	.driver =3D {
> +		.name	=3D "lg4573",
> +		.owner	=3D THIS_MODULE,
> +		.of_match_table =3D lg4573_of_match,
> +	},

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

Thierry

--pf9I7BMVVzbSWLtt
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAABCAAGBQJVcZPQAAoJEN0jrNd/PrOhgS8QAIrcMsVXtifPq1oJypITkkNi
E0Sw7dD6uzaFCfJDF4oAeaCq/7v+LLh5SWBepywWa4HkYqwiLYOWHTpRji7W3HwM
1Z9Jmiw02GKbAsBvJKO+qOxewuEZK76wF9tYoZLgYvkXK+xK+Npq+amrETFTO6CW
fQK/EpwKLM5Q9hexEeLE6jXcL1NNW2MgD1B46xhzSPEyre+AjFtAR1Kxv4hApa9M
sSDkgdA8rN6xWt4vlGkk9apThdZJ65TiLLfFp7EiX60m4uKmOpjK26kYQjfekuAv
9DKO3z/DujZ2OgcbqkEgVoA2qB9nqi/AXEy/kpBcQj5HGAXO8CFERAE8oT2p+Ru/
enZ5Y24LifTe/rcwyqHzboBHekiZKf0jyjCxRsXTZZ6Fbg17DMS0ho53mtoy8LHn
BpQkvvVCIIB4db8FRaPDGy3TIkat94w2MLgJnKnGhONwqr8kE1nsc9Zme/36XWzC
wb3yH6cWSDXVlZEsz/NXSlgP0EjYgneYTGP31Dmwh+dyXmDPXWPErlDohgymLVQ0
AkHJ6kK4vRlhf6XtulqWpGDIvB8Crvmr29thhVLgfc0ikN/godjW4mLE7/4PuxfX
XGWPAjX3rihiolLFQP2qqZh8+lDJvd0YCQfWzLRHsyX7jMW1Q4yw1rEKvBa8Dct6
AjTr+E6cetYBGyKHqIVJ
=0K3D
-----END PGP SIGNATURE-----

--pf9I7BMVVzbSWLtt--