From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thierry Reding <thierry.reding@gmail.com>
Subject: Re: [PATCH v4 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel
Date: Mon, 13 Jun 2016 14:35:14 +0200
Message-ID: <20160613123514.GF27930@ulmo.ba.sec>
References: <1465813529-30621-1-git-send-email-simhavcs@gmail.com>
 <1465813529-30621-2-git-send-email-simhavcs@gmail.com>
Mime-Version: 1.0
Content-Type: multipart/mixed; boundary="===============1557078208=="
Return-path: <dri-devel-bounces@lists.freedesktop.org>
Received: from mail-wm0-x242.google.com (mail-wm0-x242.google.com
 [IPv6:2a00:1450:400c:c09::242])
 by gabe.freedesktop.org (Postfix) with ESMTPS id C0E966E2E1
 for <dri-devel@lists.freedesktop.org>; Mon, 13 Jun 2016 12:35:18 +0000 (UTC)
Received: by mail-wm0-x242.google.com with SMTP id r5so14555476wmr.0
 for <dri-devel@lists.freedesktop.org>; Mon, 13 Jun 2016 05:35:18 -0700 (PDT)
In-Reply-To: <1465813529-30621-2-git-send-email-simhavcs@gmail.com>
List-Unsubscribe: <https://lists.freedesktop.org/mailman/options/dri-devel>,
 <mailto:dri-devel-request@lists.freedesktop.org?subject=unsubscribe>
List-Archive: <https://lists.freedesktop.org/archives/dri-devel>
List-Post: <mailto:dri-devel@lists.freedesktop.org>
List-Help: <mailto:dri-devel-request@lists.freedesktop.org?subject=help>
List-Subscribe: <https://lists.freedesktop.org/mailman/listinfo/dri-devel>,
 <mailto:dri-devel-request@lists.freedesktop.org?subject=subscribe>
Errors-To: dri-devel-bounces@lists.freedesktop.org
Sender: "dri-devel" <dri-devel-bounces@lists.freedesktop.org>
To: Vinay Simha BN <simhavcs@gmail.com>
Cc: open list <linux-kernel@vger.kernel.org>, "open list:DRM PANEL DRIVERS" <dri-devel@lists.freedesktop.org>, Archit Taneja <archit.taneja@gmail.com>
List-Id: dri-devel@lists.freedesktop.org


--===============1557078208==
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="ni93GHxFvA+th69W"
Content-Disposition: inline


--ni93GHxFvA+th69W
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Jun 13, 2016 at 03:55:28PM +0530, Vinay Simha BN wrote:
> Add support for the JDI lt070me05000 WUXGA DSI panel used in

Can you please make the names consistent? Use the all-uppercase spelling
for the panel model.

> Nexus 7 2013 devices.
>=20
> Programming sequence for the panel is was originally found in the
> android-msm-flo-3.4-lollipop-release branch from:
>     https://android.googlesource.com/kernel/msm.git
>=20
> And video mode setting is from dsi-panel-jdi-dualmipi1-video.dtsi
> file in:
>     git://codeaurora.org/kernel/msm-3.10.git  LNX.LA.3.6_rb1.27
>=20
> Cc: Archit Taneja <archit.taneja@gmail.com>
> [sumit.semwal: Ported to the drm/panel framework]
> Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
> [jstultz: Cherry-picked to mainline, folded down other fixes
>  from Vinay and Archit]
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> [vinay simha bn: removed interface setting cmd mode, video
> mode panel setting selection]
> Cc: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Vinay Simha BN <simhavcs@gmail.com>
> --
> v2:
>  * incorporated code reviews from theiry, archit
>    code style, alphabetical soring in Makefile, Kconfig, regulator_bulk,
>    arrays of u8, generic helper function, documentation bindings,
>=20
> v3:
>  * dcs backlight support added
>  * tested this panel driver in nexus7 2013 device
>=20
> v4:
>  * backlight interface added in the panel driver
>  * incorporated width_mm and height_mm suggested by rob herring
> ---
>  drivers/gpu/drm/panel/Kconfig                  |  11 +
>  drivers/gpu/drm/panel/Makefile                 |   1 +
>  drivers/gpu/drm/panel/panel-jdi-lt070me05000.c | 539 +++++++++++++++++++=
++++++
>  3 files changed, 551 insertions(+)
>  create mode 100644 drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
>=20
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 1500ab9..83e89e7 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -7,6 +7,17 @@ config DRM_PANEL
>  menu "Display Panels"
>  	depends on DRM && DRM_PANEL
> =20
> +config DRM_PANEL_JDI_LT070ME05000
> +        tristate "JDI LT070ME05000 WUXGA DSI panel"
> +        depends on OF
> +        depends on DRM_MIPI_DSI
> +        depends on BACKLIGHT_CLASS_DEVICE
> +        help

Please use consistent indentation here. Also, this is badly sorted. The
DRM_PANEL_SIMPLE is special in that it doesn't have a vendor prefix, all
others should be sorted after panel-simple and in alphabetical order by
vendor, then model.

> +	   Say Y here if you want to enable support for JDI WUXGA DSI video
> +	   mode panel as found in Google Nexus 7 (2013) devices.
> +	   The panel has a 1200(RGB)=C3=971920 (WUXGA) resolution and uses
> +	   24 bit RGB per pixel.

It's kind of redundant to say 1200(RGB) when you say that it's 24 bit
RGB afterwards. You also repeat WUXGA twice, so you can drop either of
the occurrences as well.

> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makef=
ile
> index f277eed..5d74ac2 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -1,3 +1,4 @@
> +obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) +=3D panel-jdi-lt070me05000.o

Please sort this the same way as the Kconfig entries.

>  obj-$(CONFIG_DRM_PANEL_SIMPLE) +=3D panel-simple.o
>  obj-$(CONFIG_DRM_PANEL_LG_LG4573) +=3D panel-lg-lg4573.o
>  obj-$(CONFIG_DRM_PANEL_PANASONIC_VVX10F034N00) +=3D panel-panasonic-vvx1=
0f034n00.o
> diff --git a/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c b/drivers/gpu=
/drm/panel/panel-jdi-lt070me05000.c
> new file mode 100644
> index 0000000..8657e5f
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
> @@ -0,0 +1,539 @@
> +/*
> + * Copyright (C) 2016 InforceComputing
> + * Author: Vinay Simha BN <simhavcs@gmail.com>
> + *
> + * Copyright (C) 2016 Linaro Ltd
> + * Author: Sumit Semwal <sumit.semwal@linaro.org>
> + *
> + * From internet archives, the panel for Nexus 7 2nd Gen, 2013 model is a
> + * JDI model LT070ME05000, and its data sheet is at:
> + * http://panelone.net/en/7-0-inch/JDI_LT070ME05000_7.0_inch-datasheet
> + *
> + * 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 publis=
hed by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but W=
ITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License=
 for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License alo=
ng with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#include <linux/backlight.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_panel.h>
> +
> +#include <video/mipi_display.h>
> +
> +#define PANEL_DEV_REGULATOR_MAX   3

_MAX suggests that the number of regulators is actually variable, but
it's not. PANEL_NUM_REGULATORS, perhaps?

> +#define PANEL_MAX_BRIGHTNESS	0xFF

There isn't really a need for this macro, you only use the constant in
one location. Also, a brightness is usually represented in decimal.

> +const char *regs[] =3D {
> +	"vddp",
> +	"dcdc_en",
> +	"vcc"
> +};

This should be static. Also use a more sensible name, such as
regulator_names, please.

> +static int jdi_panel_init(struct jdi_panel *jdi)
> +{
[...]
> +	struct mipi_dsi_device *dsi =3D jdi->dsi;
> +	int ret;
> +
> +	dsi->mode_flags |=3D MIPI_DSI_MODE_LPM;
> +
> +	ret =3D mipi_dsi_dcs_soft_reset(dsi);
> +	if (ret < 0)
> +		return ret;
> +
> +	usleep_range(10000, 20000);
> +
> +	ret =3D mipi_dsi_dcs_set_pixel_format(dsi, 0x70);
> +	if (ret < 0)
> +		return ret;

Please use the existing symbolic constants for this.

> +
> +	ret =3D mipi_dsi_dcs_set_column_address(dsi, 0x0000, 0x04AF);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret =3D mipi_dsi_dcs_set_page_address(dsi, 0x0000, 0x077F);
> +	if (ret < 0)
> +		return ret;

Please parameterize these using the panel width and height.

> +
> +	ret =3D mipi_dsi_dcs_set_tear_on(dsi,
> +				       MIPI_DSI_DCS_TEAR_MODE_VBLANK);
> +	if (ret < 0)
> +		return ret;
> +	usleep_range(5000, 10000);

Use a blank line after conditional blocks, please.

> +
> +	ret =3D mipi_dsi_set_tear_scanline(dsi, 0x0300);
> +	if (ret < 0)
> +		return ret;

Might be worth making this a decimal. But then the question begs: why is
768 a good value for the tear scanline? Also, should you not set the
tear scanline before enabling tear mode?

> +
> +	ret =3D mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS,
> +				 (u8[]){ 0xFF }, 1);
> +	if (ret < 0)
> +		return ret;

Is this necessary? Shouldn't the backlight implementation set this upon
registration already?

> +
> +	ret =3D mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY,
> +				 (u8[]){ 0x24 }, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret =3D mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_POWER_SAVE,
> +				 (u8[]){ 0x00 }, 1);
> +	if (ret < 0)
> +		return ret;

What do these mean?

> +
> +	ret =3D mipi_dsi_dcs_exit_sleep_mode(dsi);
> +	if (ret < 0)
> +		return ret;
> +	mdelay(120);

Again, could use an extra blank line for readability. Also, don't ever
use mdelay() for such large delays! Use msleep() instead.

> +	ret =3D mipi_dsi_generic_write(dsi, (u8[]){0xB0, 0x00}, 2);
> +	if (ret < 0)
> +		return ret;
> +	mdelay(10);

Same here. This also needs at least a comment, though ideally you'd use
symbolic names for those magic numbers.

> +	ret =3D mipi_dsi_generic_write(dsi, (u8[])
> +				     {0xB3, 0x26, 0x08, 0x00, 0x20, 0x00}, 6);
> +	if (ret < 0)
> +		return ret;
> +	mdelay(20);

And here.

> +static int jdi_panel_unprepare(struct drm_panel *panel)
> +{
> +	struct jdi_panel *jdi =3D to_jdi_panel(panel);
> +	struct regulator_bulk_data *s =3D jdi->supplies;
> +	int num =3D PANEL_DEV_REGULATOR_MAX;
> +	struct device *dev =3D &jdi->dsi->dev;
> +	int ret;
> +
> +	if (!jdi->prepared)
> +		return 0;
> +
> +	DRM_DEBUG("unprepare\n");
> +
> +	ret =3D jdi_panel_off(jdi);
> +	if (ret) {
> +		dev_err(panel->dev, "failed to set panel off: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret =3D regulator_bulk_disable(num, s);
> +	if (ret < 0) {
> +		dev_err(dev, "regulator disable failed, %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (jdi->reset_gpio)
> +		gpiod_set_value(jdi->reset_gpio, 0);

That's odd. Wouldn't you typically assert the reset in ->unprepare()?
Perhaps you need to mark the GPIO as GPIO_ACTIVE_LOW?

> +	if (jdi->enable_gpio)
> +		gpiod_set_value(jdi->enable_gpio, 0);
> +
> +	jdi->prepared =3D false;
> +
> +	return 0;
> +}
> +
> +static int jdi_panel_prepare(struct drm_panel *panel)
> +{
> +	struct jdi_panel *jdi =3D to_jdi_panel(panel);
> +	struct regulator_bulk_data *s =3D jdi->supplies;

There's no need for this temporary variable, jdi->supplies isn't
terrible long.

> +	int num =3D PANEL_DEV_REGULATOR_MAX;

This should be unsigned int. Also, might be worth to simply drop this
usage and do...

> +	struct device *dev =3D &jdi->dsi->dev;
> +	int ret;
> +
> +	if (jdi->prepared)
> +		return 0;
> +
> +	DRM_DEBUG("prepare\n");
> +
> +	ret =3D regulator_bulk_enable(num, s);

	ret =3D regulator_bulk_enable(ARRAY_SIZE(jdi->supplies), jdi->supplies);

here.

> +	if (ret < 0) {
> +		dev_err(dev, "regulator enable failed, %d\n", ret);

Can you please be more consistent with how you report errors? Other
drivers use "...: %d\n", ret and you do so yourself in other parts of
this same driver, so make them all consistent, please.

> +static int jdi_panel_enable(struct drm_panel *panel)
> +{
> +	struct jdi_panel *jdi =3D to_jdi_panel(panel);
> +
> +	if (jdi->enabled)
> +		return 0;
> +
> +	DRM_DEBUG("enable\n");

Do you really need these? You can use the kernel's built-in function
tracing for the same purpose.

> +static const struct drm_display_mode default_mode =3D {
> +		.clock =3D 155493,
> +		.hdisplay =3D 1200,
> +		.hsync_start =3D 1200 + 48,
> +		.hsync_end =3D 1200 + 48 + 32,
> +		.htotal =3D 1200 + 48 + 32 + 60,
> +		.vdisplay =3D 1920,
> +		.vsync_start =3D 1920 + 3,
> +		.vsync_end =3D 1920 + 3 + 5,
> +		.vtotal =3D 1920 + 3 + 5 + 6,
> +		.vrefresh =3D 60,
> +		.flags =3D 0,
> +		.width_mm =3D 95,
> +		.height_mm =3D 151,

It's slightly unusual to set this in the mode. Typically you'd set the
connector->display_info.{width,height}_mm directly.

> +static int dsi_dcs_bl_get_brightness(struct backlight_device *bl)
> +{
> +	struct mipi_dsi_device *dsi =3D bl_get_data(bl);
> +	int ret;
> +	u16 brightness;
> +	u8 bl_value[2];
> +
> +	dsi->mode_flags &=3D ~MIPI_DSI_MODE_LPM;
> +
> +	ret =3D mipi_dsi_dcs_get_display_brightness(dsi, &brightness);
> +	if (ret < 0)
> +		return ret;
> +
> +	dsi->mode_flags |=3D MIPI_DSI_MODE_LPM;
> +
> +	bl_value[0] =3D brightness & 0xff;
> +	bl_value[1] =3D brightness >> 8;
> +
> +	return bl_value[0];
> +}

Why not simply return "brightness & 0xff" here?

> +static int dsi_dcs_bl_update_status(struct backlight_device *bl)
> +{
> +	struct mipi_dsi_device *dsi =3D bl_get_data(bl);
> +	int ret;
> +	u16 brightness =3D bl->props.brightness;

There's no need for this temporary variable.

> +
> +	dsi->mode_flags &=3D ~MIPI_DSI_MODE_LPM;
> +
> +	ret =3D mipi_dsi_dcs_set_display_brightness(dsi, brightness);
> +	if (ret < 0)
> +		return ret;
> +
> +	dsi->mode_flags |=3D MIPI_DSI_MODE_LPM;
> +
> +	return 0;
> +}
[...]
> +struct backlight_device
> +		*drm_panel_create_dsi_backlight(struct mipi_dsi_device *dsi)

Your indentation is wrong here. The * should go on the same line as the
type, and the function name should start in column 1.

> +{
> +	struct device *dev =3D &dsi->dev;
> +	struct backlight_properties props;
> +
> +	memset(&props, 0, sizeof(props));
> +	props.type =3D BACKLIGHT_RAW;
> +	props.brightness =3D PANEL_MAX_BRIGHTNESS;
> +	props.max_brightness =3D PANEL_MAX_BRIGHTNESS;

Just use 255 for the maximum and default brightness here.

> +static int jdi_panel_add(struct jdi_panel *jdi)
> +{
> +	struct device *dev =3D &jdi->dsi->dev;
> +	struct regulator_bulk_data *s =3D jdi->supplies;
> +	int num =3D PANEL_DEV_REGULATOR_MAX;

Again, no need for these two temporary variables.

> +	int i, ret;

i should be unsigned int.

> +
> +	jdi->mode =3D &default_mode;
> +
> +	for (i =3D 0; i < num; i++)
> +		s[i].supply =3D regs[i];
> +
> +	ret =3D devm_regulator_bulk_get(dev, num, s);
> +	if (ret < 0) {
> +		dev_err(dev, "%s: failed to init regulator, ret=3D%d\n",
> +			__func__, ret);
> +		return ret;
> +	}
> +
> +	jdi->reset_gpio =3D devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(jdi->reset_gpio)) {
> +		dev_err(dev, "cannot get reset-gpios %ld\n",
> +			PTR_ERR(jdi->reset_gpio));

This is a third variant of error reporting. Please stick to one.

> +		jdi->reset_gpio =3D NULL;
> +	} else {
> +		gpiod_direction_output(jdi->reset_gpio, 0);
> +	}
> +
> +	jdi->enable_gpio =3D devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
> +	if (IS_ERR(jdi->enable_gpio)) {
> +		dev_err(dev, "cannot get enable-gpio %ld\n",
> +			PTR_ERR(jdi->enable_gpio));
> +		jdi->enable_gpio =3D NULL;
> +	} else {
> +		gpiod_direction_output(jdi->enable_gpio, 0);
> +	}
> +
> +	jdi->backlight =3D drm_panel_create_dsi_backlight(jdi->dsi);

You should check for errors here.

> +MODULE_AUTHOR("Vinay Simha BN <simhavcs@gmail.com>");
> +MODULE_DESCRIPTION("JDI WUXGA LT070ME05000 DSI video mode panel driver");

The commit message names this "JDI LT070ME05000 WUXGA", please use the
same here.

Thierry

--ni93GHxFvA+th69W
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIcBAABCAAGBQJXXqh/AAoJEN0jrNd/PrOh80MP/2iuhWFCFcIB7pmZ07iUFSrt
3VrG2/ITBIAAK86T4Kpgt+WLX8PW8WhOf1g/jp3qhF4wswD1byI+KsQxmeP922nG
k7xu6cDOWiI8rvpNLuMKogSCDz277Z7GZLby3fkSBoYv36/Q3iUQiZMkBb5ZCYlW
qfYIXBM+TB5XjvjfegLN6+aBKBYWLWkCAUtpB0YVJmI8J3lw/BV2Az0aqVDHeEMw
VkiAirKknP3A0T8qJKB1JbRYMZfDeTGK3EJICKS/nI+TGRAT3HB6u4USfla9L7z0
OxZgO/MTsTstqKWdXxZD1i/LZOc8ozWq3gGj45UHbJd8s0gyhhEy92x+qR2T+E7D
D8oAuljCGe/jxk+vZnsbjxsfzXlOLUB0ELG6MaaS9kDWJeARH8gjU7E0cET8WipS
eEaG53FJwQpX6ErwpeU0fK2Yu36wsQ7d2jDQBrLu3vv6OXU5wT5klWQ8DR+heNG8
J5mLJEzIOGxVILjzZOaV1tRf4SYRrcXLNlaOIkzmklcKigLdycVAFFl7QCNKbL1p
nB8YxRovEm+o3n/BF6BjNc06DONI1Caw32yyXusxq2VT7tbVouajwxZFUGoJqwnU
fpA4dpA34f+zUFqUFIjPfHMgjjx335vQhK1JaeDXCpNUmhnHNnhNZuPylQy8NiTk
0emn4FbsPG9p9RX3+6WL
=1bc+
-----END PGP SIGNATURE-----

--ni93GHxFvA+th69W--

--===============1557078208==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: base64
Content-Disposition: inline

X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs
IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz
dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg==

--===============1557078208==--

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 S1422754AbcFMMfY (ORCPT <rfc822;w@1wt.eu>);
	Mon, 13 Jun 2016 08:35:24 -0400
Received: from mail-wm0-f68.google.com ([74.125.82.68]:36557 "EHLO
	mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1422788AbcFMMfS (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Mon, 13 Jun 2016 08:35:18 -0400
Date: Mon, 13 Jun 2016 14:35:14 +0200
From: Thierry Reding <thierry.reding@gmail.com>
To: Vinay Simha BN <simhavcs@gmail.com>
Cc: Archit Taneja <archit.taneja@gmail.com>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        John Stultz <john.stultz@linaro.org>, Rob Clark <robdclark@gmail.com>,
        David Airlie <airlied@linux.ie>,
        open list <linux-kernel@vger.kernel.org>,
        "open list:DRM PANEL DRIVERS" <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH v4 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel
Message-ID: <20160613123514.GF27930@ulmo.ba.sec>
References: <1465813529-30621-1-git-send-email-simhavcs@gmail.com>
 <1465813529-30621-2-git-send-email-simhavcs@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="ni93GHxFvA+th69W"
Content-Disposition: inline
In-Reply-To: <1465813529-30621-2-git-send-email-simhavcs@gmail.com>
User-Agent: Mutt/1.6.1 (2016-04-27)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


--ni93GHxFvA+th69W
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Jun 13, 2016 at 03:55:28PM +0530, Vinay Simha BN wrote:
> Add support for the JDI lt070me05000 WUXGA DSI panel used in

Can you please make the names consistent? Use the all-uppercase spelling
for the panel model.

> Nexus 7 2013 devices.
>=20
> Programming sequence for the panel is was originally found in the
> android-msm-flo-3.4-lollipop-release branch from:
>     https://android.googlesource.com/kernel/msm.git
>=20
> And video mode setting is from dsi-panel-jdi-dualmipi1-video.dtsi
> file in:
>     git://codeaurora.org/kernel/msm-3.10.git  LNX.LA.3.6_rb1.27
>=20
> Cc: Archit Taneja <archit.taneja@gmail.com>
> [sumit.semwal: Ported to the drm/panel framework]
> Signed-off-by: Sumit Semwal <sumit.semwal@linaro.org>
> [jstultz: Cherry-picked to mainline, folded down other fixes
>  from Vinay and Archit]
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> [vinay simha bn: removed interface setting cmd mode, video
> mode panel setting selection]
> Cc: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Vinay Simha BN <simhavcs@gmail.com>
> --
> v2:
>  * incorporated code reviews from theiry, archit
>    code style, alphabetical soring in Makefile, Kconfig, regulator_bulk,
>    arrays of u8, generic helper function, documentation bindings,
>=20
> v3:
>  * dcs backlight support added
>  * tested this panel driver in nexus7 2013 device
>=20
> v4:
>  * backlight interface added in the panel driver
>  * incorporated width_mm and height_mm suggested by rob herring
> ---
>  drivers/gpu/drm/panel/Kconfig                  |  11 +
>  drivers/gpu/drm/panel/Makefile                 |   1 +
>  drivers/gpu/drm/panel/panel-jdi-lt070me05000.c | 539 +++++++++++++++++++=
++++++
>  3 files changed, 551 insertions(+)
>  create mode 100644 drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
>=20
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 1500ab9..83e89e7 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -7,6 +7,17 @@ config DRM_PANEL
>  menu "Display Panels"
>  	depends on DRM && DRM_PANEL
> =20
> +config DRM_PANEL_JDI_LT070ME05000
> +        tristate "JDI LT070ME05000 WUXGA DSI panel"
> +        depends on OF
> +        depends on DRM_MIPI_DSI
> +        depends on BACKLIGHT_CLASS_DEVICE
> +        help

Please use consistent indentation here. Also, this is badly sorted. The
DRM_PANEL_SIMPLE is special in that it doesn't have a vendor prefix, all
others should be sorted after panel-simple and in alphabetical order by
vendor, then model.

> +	   Say Y here if you want to enable support for JDI WUXGA DSI video
> +	   mode panel as found in Google Nexus 7 (2013) devices.
> +	   The panel has a 1200(RGB)=C3=971920 (WUXGA) resolution and uses
> +	   24 bit RGB per pixel.

It's kind of redundant to say 1200(RGB) when you say that it's 24 bit
RGB afterwards. You also repeat WUXGA twice, so you can drop either of
the occurrences as well.

> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makef=
ile
> index f277eed..5d74ac2 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -1,3 +1,4 @@
> +obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) +=3D panel-jdi-lt070me05000.o

Please sort this the same way as the Kconfig entries.

>  obj-$(CONFIG_DRM_PANEL_SIMPLE) +=3D panel-simple.o
>  obj-$(CONFIG_DRM_PANEL_LG_LG4573) +=3D panel-lg-lg4573.o
>  obj-$(CONFIG_DRM_PANEL_PANASONIC_VVX10F034N00) +=3D panel-panasonic-vvx1=
0f034n00.o
> diff --git a/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c b/drivers/gpu=
/drm/panel/panel-jdi-lt070me05000.c
> new file mode 100644
> index 0000000..8657e5f
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
> @@ -0,0 +1,539 @@
> +/*
> + * Copyright (C) 2016 InforceComputing
> + * Author: Vinay Simha BN <simhavcs@gmail.com>
> + *
> + * Copyright (C) 2016 Linaro Ltd
> + * Author: Sumit Semwal <sumit.semwal@linaro.org>
> + *
> + * From internet archives, the panel for Nexus 7 2nd Gen, 2013 model is a
> + * JDI model LT070ME05000, and its data sheet is at:
> + * http://panelone.net/en/7-0-inch/JDI_LT070ME05000_7.0_inch-datasheet
> + *
> + * 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 publis=
hed by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but W=
ITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License=
 for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License alo=
ng with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#include <linux/backlight.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_panel.h>
> +
> +#include <video/mipi_display.h>
> +
> +#define PANEL_DEV_REGULATOR_MAX   3

_MAX suggests that the number of regulators is actually variable, but
it's not. PANEL_NUM_REGULATORS, perhaps?

> +#define PANEL_MAX_BRIGHTNESS	0xFF

There isn't really a need for this macro, you only use the constant in
one location. Also, a brightness is usually represented in decimal.

> +const char *regs[] =3D {
> +	"vddp",
> +	"dcdc_en",
> +	"vcc"
> +};

This should be static. Also use a more sensible name, such as
regulator_names, please.

> +static int jdi_panel_init(struct jdi_panel *jdi)
> +{
[...]
> +	struct mipi_dsi_device *dsi =3D jdi->dsi;
> +	int ret;
> +
> +	dsi->mode_flags |=3D MIPI_DSI_MODE_LPM;
> +
> +	ret =3D mipi_dsi_dcs_soft_reset(dsi);
> +	if (ret < 0)
> +		return ret;
> +
> +	usleep_range(10000, 20000);
> +
> +	ret =3D mipi_dsi_dcs_set_pixel_format(dsi, 0x70);
> +	if (ret < 0)
> +		return ret;

Please use the existing symbolic constants for this.

> +
> +	ret =3D mipi_dsi_dcs_set_column_address(dsi, 0x0000, 0x04AF);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret =3D mipi_dsi_dcs_set_page_address(dsi, 0x0000, 0x077F);
> +	if (ret < 0)
> +		return ret;

Please parameterize these using the panel width and height.

> +
> +	ret =3D mipi_dsi_dcs_set_tear_on(dsi,
> +				       MIPI_DSI_DCS_TEAR_MODE_VBLANK);
> +	if (ret < 0)
> +		return ret;
> +	usleep_range(5000, 10000);

Use a blank line after conditional blocks, please.

> +
> +	ret =3D mipi_dsi_set_tear_scanline(dsi, 0x0300);
> +	if (ret < 0)
> +		return ret;

Might be worth making this a decimal. But then the question begs: why is
768 a good value for the tear scanline? Also, should you not set the
tear scanline before enabling tear mode?

> +
> +	ret =3D mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS,
> +				 (u8[]){ 0xFF }, 1);
> +	if (ret < 0)
> +		return ret;

Is this necessary? Shouldn't the backlight implementation set this upon
registration already?

> +
> +	ret =3D mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY,
> +				 (u8[]){ 0x24 }, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret =3D mipi_dsi_dcs_write(dsi, MIPI_DCS_WRITE_POWER_SAVE,
> +				 (u8[]){ 0x00 }, 1);
> +	if (ret < 0)
> +		return ret;

What do these mean?

> +
> +	ret =3D mipi_dsi_dcs_exit_sleep_mode(dsi);
> +	if (ret < 0)
> +		return ret;
> +	mdelay(120);

Again, could use an extra blank line for readability. Also, don't ever
use mdelay() for such large delays! Use msleep() instead.

> +	ret =3D mipi_dsi_generic_write(dsi, (u8[]){0xB0, 0x00}, 2);
> +	if (ret < 0)
> +		return ret;
> +	mdelay(10);

Same here. This also needs at least a comment, though ideally you'd use
symbolic names for those magic numbers.

> +	ret =3D mipi_dsi_generic_write(dsi, (u8[])
> +				     {0xB3, 0x26, 0x08, 0x00, 0x20, 0x00}, 6);
> +	if (ret < 0)
> +		return ret;
> +	mdelay(20);

And here.

> +static int jdi_panel_unprepare(struct drm_panel *panel)
> +{
> +	struct jdi_panel *jdi =3D to_jdi_panel(panel);
> +	struct regulator_bulk_data *s =3D jdi->supplies;
> +	int num =3D PANEL_DEV_REGULATOR_MAX;
> +	struct device *dev =3D &jdi->dsi->dev;
> +	int ret;
> +
> +	if (!jdi->prepared)
> +		return 0;
> +
> +	DRM_DEBUG("unprepare\n");
> +
> +	ret =3D jdi_panel_off(jdi);
> +	if (ret) {
> +		dev_err(panel->dev, "failed to set panel off: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret =3D regulator_bulk_disable(num, s);
> +	if (ret < 0) {
> +		dev_err(dev, "regulator disable failed, %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (jdi->reset_gpio)
> +		gpiod_set_value(jdi->reset_gpio, 0);

That's odd. Wouldn't you typically assert the reset in ->unprepare()?
Perhaps you need to mark the GPIO as GPIO_ACTIVE_LOW?

> +	if (jdi->enable_gpio)
> +		gpiod_set_value(jdi->enable_gpio, 0);
> +
> +	jdi->prepared =3D false;
> +
> +	return 0;
> +}
> +
> +static int jdi_panel_prepare(struct drm_panel *panel)
> +{
> +	struct jdi_panel *jdi =3D to_jdi_panel(panel);
> +	struct regulator_bulk_data *s =3D jdi->supplies;

There's no need for this temporary variable, jdi->supplies isn't
terrible long.

> +	int num =3D PANEL_DEV_REGULATOR_MAX;

This should be unsigned int. Also, might be worth to simply drop this
usage and do...

> +	struct device *dev =3D &jdi->dsi->dev;
> +	int ret;
> +
> +	if (jdi->prepared)
> +		return 0;
> +
> +	DRM_DEBUG("prepare\n");
> +
> +	ret =3D regulator_bulk_enable(num, s);

	ret =3D regulator_bulk_enable(ARRAY_SIZE(jdi->supplies), jdi->supplies);

here.

> +	if (ret < 0) {
> +		dev_err(dev, "regulator enable failed, %d\n", ret);

Can you please be more consistent with how you report errors? Other
drivers use "...: %d\n", ret and you do so yourself in other parts of
this same driver, so make them all consistent, please.

> +static int jdi_panel_enable(struct drm_panel *panel)
> +{
> +	struct jdi_panel *jdi =3D to_jdi_panel(panel);
> +
> +	if (jdi->enabled)
> +		return 0;
> +
> +	DRM_DEBUG("enable\n");

Do you really need these? You can use the kernel's built-in function
tracing for the same purpose.

> +static const struct drm_display_mode default_mode =3D {
> +		.clock =3D 155493,
> +		.hdisplay =3D 1200,
> +		.hsync_start =3D 1200 + 48,
> +		.hsync_end =3D 1200 + 48 + 32,
> +		.htotal =3D 1200 + 48 + 32 + 60,
> +		.vdisplay =3D 1920,
> +		.vsync_start =3D 1920 + 3,
> +		.vsync_end =3D 1920 + 3 + 5,
> +		.vtotal =3D 1920 + 3 + 5 + 6,
> +		.vrefresh =3D 60,
> +		.flags =3D 0,
> +		.width_mm =3D 95,
> +		.height_mm =3D 151,

It's slightly unusual to set this in the mode. Typically you'd set the
connector->display_info.{width,height}_mm directly.

> +static int dsi_dcs_bl_get_brightness(struct backlight_device *bl)
> +{
> +	struct mipi_dsi_device *dsi =3D bl_get_data(bl);
> +	int ret;
> +	u16 brightness;
> +	u8 bl_value[2];
> +
> +	dsi->mode_flags &=3D ~MIPI_DSI_MODE_LPM;
> +
> +	ret =3D mipi_dsi_dcs_get_display_brightness(dsi, &brightness);
> +	if (ret < 0)
> +		return ret;
> +
> +	dsi->mode_flags |=3D MIPI_DSI_MODE_LPM;
> +
> +	bl_value[0] =3D brightness & 0xff;
> +	bl_value[1] =3D brightness >> 8;
> +
> +	return bl_value[0];
> +}

Why not simply return "brightness & 0xff" here?

> +static int dsi_dcs_bl_update_status(struct backlight_device *bl)
> +{
> +	struct mipi_dsi_device *dsi =3D bl_get_data(bl);
> +	int ret;
> +	u16 brightness =3D bl->props.brightness;

There's no need for this temporary variable.

> +
> +	dsi->mode_flags &=3D ~MIPI_DSI_MODE_LPM;
> +
> +	ret =3D mipi_dsi_dcs_set_display_brightness(dsi, brightness);
> +	if (ret < 0)
> +		return ret;
> +
> +	dsi->mode_flags |=3D MIPI_DSI_MODE_LPM;
> +
> +	return 0;
> +}
[...]
> +struct backlight_device
> +		*drm_panel_create_dsi_backlight(struct mipi_dsi_device *dsi)

Your indentation is wrong here. The * should go on the same line as the
type, and the function name should start in column 1.

> +{
> +	struct device *dev =3D &dsi->dev;
> +	struct backlight_properties props;
> +
> +	memset(&props, 0, sizeof(props));
> +	props.type =3D BACKLIGHT_RAW;
> +	props.brightness =3D PANEL_MAX_BRIGHTNESS;
> +	props.max_brightness =3D PANEL_MAX_BRIGHTNESS;

Just use 255 for the maximum and default brightness here.

> +static int jdi_panel_add(struct jdi_panel *jdi)
> +{
> +	struct device *dev =3D &jdi->dsi->dev;
> +	struct regulator_bulk_data *s =3D jdi->supplies;
> +	int num =3D PANEL_DEV_REGULATOR_MAX;

Again, no need for these two temporary variables.

> +	int i, ret;

i should be unsigned int.

> +
> +	jdi->mode =3D &default_mode;
> +
> +	for (i =3D 0; i < num; i++)
> +		s[i].supply =3D regs[i];
> +
> +	ret =3D devm_regulator_bulk_get(dev, num, s);
> +	if (ret < 0) {
> +		dev_err(dev, "%s: failed to init regulator, ret=3D%d\n",
> +			__func__, ret);
> +		return ret;
> +	}
> +
> +	jdi->reset_gpio =3D devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(jdi->reset_gpio)) {
> +		dev_err(dev, "cannot get reset-gpios %ld\n",
> +			PTR_ERR(jdi->reset_gpio));

This is a third variant of error reporting. Please stick to one.

> +		jdi->reset_gpio =3D NULL;
> +	} else {
> +		gpiod_direction_output(jdi->reset_gpio, 0);
> +	}
> +
> +	jdi->enable_gpio =3D devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
> +	if (IS_ERR(jdi->enable_gpio)) {
> +		dev_err(dev, "cannot get enable-gpio %ld\n",
> +			PTR_ERR(jdi->enable_gpio));
> +		jdi->enable_gpio =3D NULL;
> +	} else {
> +		gpiod_direction_output(jdi->enable_gpio, 0);
> +	}
> +
> +	jdi->backlight =3D drm_panel_create_dsi_backlight(jdi->dsi);

You should check for errors here.

> +MODULE_AUTHOR("Vinay Simha BN <simhavcs@gmail.com>");
> +MODULE_DESCRIPTION("JDI WUXGA LT070ME05000 DSI video mode panel driver");

The commit message names this "JDI LT070ME05000 WUXGA", please use the
same here.

Thierry

--ni93GHxFvA+th69W
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIcBAABCAAGBQJXXqh/AAoJEN0jrNd/PrOh80MP/2iuhWFCFcIB7pmZ07iUFSrt
3VrG2/ITBIAAK86T4Kpgt+WLX8PW8WhOf1g/jp3qhF4wswD1byI+KsQxmeP922nG
k7xu6cDOWiI8rvpNLuMKogSCDz277Z7GZLby3fkSBoYv36/Q3iUQiZMkBb5ZCYlW
qfYIXBM+TB5XjvjfegLN6+aBKBYWLWkCAUtpB0YVJmI8J3lw/BV2Az0aqVDHeEMw
VkiAirKknP3A0T8qJKB1JbRYMZfDeTGK3EJICKS/nI+TGRAT3HB6u4USfla9L7z0
OxZgO/MTsTstqKWdXxZD1i/LZOc8ozWq3gGj45UHbJd8s0gyhhEy92x+qR2T+E7D
D8oAuljCGe/jxk+vZnsbjxsfzXlOLUB0ELG6MaaS9kDWJeARH8gjU7E0cET8WipS
eEaG53FJwQpX6ErwpeU0fK2Yu36wsQ7d2jDQBrLu3vv6OXU5wT5klWQ8DR+heNG8
J5mLJEzIOGxVILjzZOaV1tRf4SYRrcXLNlaOIkzmklcKigLdycVAFFl7QCNKbL1p
nB8YxRovEm+o3n/BF6BjNc06DONI1Caw32yyXusxq2VT7tbVouajwxZFUGoJqwnU
fpA4dpA34f+zUFqUFIjPfHMgjjx335vQhK1JaeDXCpNUmhnHNnhNZuPylQy8NiTk
0emn4FbsPG9p9RX3+6WL
=1bc+
-----END PGP SIGNATURE-----

--ni93GHxFvA+th69W--