From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <SRS0=SKwJ=2Z=lists.infradead.org=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@kernel.org>
X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on
	aws-us-west-2-korg-lkml-1.web.codeaurora.org
X-Spam-Level: 
X-Spam-Status: No, score=-10.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,
	MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1
	autolearn=unavailable autolearn_force=no version=3.4.0
Received: from mail.kernel.org (mail.kernel.org [198.145.29.99])
	by smtp.lore.kernel.org (Postfix) with ESMTP id EDFF1C2D0C2
	for <infradead-linux-arm-kernel@archiver.kernel.org>; Sat,  4 Jan 2020 18:25:32 +0000 (UTC)
Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(No client certificate requested)
	by mail.kernel.org (Postfix) with ESMTPS id C7458222C4
	for <infradead-linux-arm-kernel@archiver.kernel.org>; Sat,  4 Jan 2020 18:25:32 +0000 (UTC)
Authentication-Results: mail.kernel.org;
	dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="u3tmCPCk"
DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C7458222C4
Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ravnborg.org
Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org
DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed;
	d=lists.infradead.org; s=bombadil.20170209; h=Sender:
	Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post:
	List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:
	Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description:
	Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:
	List-Owner; bh=WG20UsgDQfe2tzQ7JGjlmB5isthFhYbRrTCdZS+y+lc=; b=u3tmCPCkO5jC0n
	Ocby5e4XurWz0V98WmxOTMGo0JDz7Pl5k017rKMIpA9VYobBmKL5+gWtyM6xnXtBbFC71FZ3MB+6i
	1iozq9BhMNZUoE/OT5qkK/x+lBozNQuPH3sXWoP5kvWcR7K1caD2PNk8+C8XQpzZSEuQLPFiWeCLn
	q1Rudq0UrY7MqZ/3UAmLxKnVVQHxwZeecbf/UCShucQ6LvXymd/cTz5l3+IoGW8PqfmGiSKP9rsqZ
	yQ3Z7UNcIdJHv+5bHXesGNr7XroqhkhS/K7ZborUJNVJ5nVBwW+CFD4NcSeIBCHarW572di0s3BO4
	DdovTov+HcxdjiOSD3LQ==;
Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org)
	by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux))
	id 1ino7A-0007qx-2B; Sat, 04 Jan 2020 18:25:32 +0000
Received: from asavdk3.altibox.net ([109.247.116.14])
 by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux))
 id 1ino75-0007ot-PL
 for linux-arm-kernel@lists.infradead.org; Sat, 04 Jan 2020 18:25:30 +0000
Received: from ravnborg.org (unknown [158.248.194.18])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
 (No client certificate requested)
 by asavdk3.altibox.net (Postfix) with ESMTPS id 006C220032;
 Sat,  4 Jan 2020 19:25:22 +0100 (CET)
Date: Sat, 4 Jan 2020 19:25:21 +0100
From: Sam Ravnborg <sam@ravnborg.org>
To: Linus Walleij <linus.walleij@linaro.org>
Subject: Re: [PATCH 2/2] drm/panel: Add driver for Novatek NT35510-based panels
Message-ID: <20200104182521.GC8724@ravnborg.org>
References: <20191225115610.14518-1-linus.walleij@linaro.org>
 <20191225115610.14518-2-linus.walleij@linaro.org>
MIME-Version: 1.0
Content-Disposition: inline
In-Reply-To: <20191225115610.14518-2-linus.walleij@linaro.org>
User-Agent: Mutt/1.10.1 (2018-07-13)
X-CMAE-Score: 0
X-CMAE-Analysis: v=2.3 cv=eMA9ckh1 c=1 sm=1 tr=0
 a=UWs3HLbX/2nnQ3s7vZ42gw==:117 a=UWs3HLbX/2nnQ3s7vZ42gw==:17
 a=jpOVt7BSZ2e4Z31A5e1TngXxSK0=:19 a=kj9zAlcOel0A:10 a=KKAkSRfTAAAA:8
 a=e5mUnYsNAAAA:8 a=20KFwNOVAAAA:8 a=J2hWI5m9r4qbfL5FBt0A:9
 a=CjuIK1q_8ugA:10 a=cvBusfyB2V15izCimMoJ:22 a=Vxmtnl_E_bksehYqCbjh:22
X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 
X-CRM114-CacheID: sfid-20200104_102528_380384_947D27DE 
X-CRM114-Status: GOOD (  30.46  )
X-BeenThere: linux-arm-kernel@lists.infradead.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: <linux-arm-kernel.lists.infradead.org>
List-Unsubscribe: <http://lists.infradead.org/mailman/options/linux-arm-kernel>, 
 <mailto:linux-arm-kernel-request@lists.infradead.org?subject=unsubscribe>
List-Archive: <http://lists.infradead.org/pipermail/linux-arm-kernel/>
List-Post: <mailto:linux-arm-kernel@lists.infradead.org>
List-Help: <mailto:linux-arm-kernel-request@lists.infradead.org?subject=help>
List-Subscribe: <http://lists.infradead.org/mailman/listinfo/linux-arm-kernel>, 
 <mailto:linux-arm-kernel-request@lists.infradead.org?subject=subscribe>
Cc: Stephan Gerhold <stephan@gerhold.net>,
 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
 Maxime Ripard <mripard@kernel.org>, dri-devel@lists.freedesktop.org,
 Sean Paul <sean@poorly.run>, linux-arm-kernel@lists.infradead.org
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Sender: "linux-arm-kernel" <linux-arm-kernel-bounces@lists.infradead.org>
Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org

Hi Linus.

Driver looks good.
Rahter complicated - but that what the controller/panel requires.
Lot's of good code comments - very nice.

A few comments in the following.

	Sam

> diff --git a/MAINTAINERS b/MAINTAINERS
> index e6db3889cb19..1372b4139ebd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5244,6 +5244,13 @@ F:	drivers/gpu/drm/msm/
>  F:	include/uapi/drm/msm_drm.h
>  F:	Documentation/devicetree/bindings/display/msm/
>  
> +DRM DRIVER FOR NOVATEK NT35510 PANELS
> +M:	Linus Walleij <linus.walleij@linaro.org>
> +T:	git git://anongit.freedesktop.org/drm/drm-misc
> +S:	Maintained
> +F:	drivers/gpu/drm/panel/panel-novatek-nt35510*
Unless you expect more files named panel-novatek-nt35510*  then use as
specific filename (no wildcard).

> +F:	Documentation/devicetree/bindings/display/panel/novatek-nt35510.yaml
> +
>  DRM DRIVER FOR NVIDIA GEFORCE/QUADRO GPUS
>  M:	Ben Skeggs <bskeggs@redhat.com>
>  L:	dri-devel@lists.freedesktop.org
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 46e3c931e5d9..620a0fd1e816 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -127,6 +127,17 @@ config DRM_PANEL_NEC_NL8048HL11
>  	  panel (found on the Zoom2/3/3630 SDP boards). To compile this driver
>  	  as a module, choose M here.
>  
> +config DRM_PANEL_NOVATEK_NT35510
> +	tristate "Novatek NT35510 RGB panel driver"
> +	depends on OF
> +	depends on DRM_MIPI_DSI
> +	depends on BACKLIGHT_CLASS_DEVICE

> +	select VIDEOMODE_HELPERS
Is this really needed? From a quick look you can drop it.

> +	help
> +	  Say Y here if you want to enable support for the panels built
> +	  around the Novatek NT35510 display controller, such as some
> +	  Hydis panels.
> +
>  config DRM_PANEL_NOVATEK_NT39016
>  	tristate "Novatek NT39016 RGB/SPI panel"
>  	depends on OF && SPI
> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35510.c b/drivers/gpu/drm/panel/panel-novatek-nt35510.c
> new file mode 100644
> index 000000000000..b312a8848c25
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-novatek-nt35510.c
> @@ -0,0 +1,1126 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Novatek NT35510 panel driver
> + * Copyright (C) 2019 Linus Walleij <linus.walleij@linaro.org>
> + * Based on code by Robert Teather (C) 2012 Samsung
> + *
> + * This display driver (and I refer to the physical component NT35510,
> + * not this Linux kernel software driver) can handle:
> + * 480x864, 480x854, 480x800, 480x720 and 480x640 pixel displays.
> + * It has 480x840x24bit SRAM embedded for storing a frame.
> + * When powered on the display is by default in 480x800 mode.
> + *
> + * The actual panels using this component have different names, but
> + * the code needed to set up and configure the panel will be similar,
> + * so they should all use the NT35510 driver with appropriate configuration
> + * per-panel, e.g. for physical size.
> + *
> + * This driver is for the DSI interface to panels using the NT35510.
> + *
> + * The NT35510 can also use an RGB (DPI) interface combined with an

> + * I2C or SPI interface for setting up the NT35510. If this is needed I
> + * this panel driver should be refactored to also support that use
An extra "I" sneaked in here.

> + * case.
> + */
You are using nice kernel-doc style comments.
Consider to wire this into Documentation/gpu/ somewhere.

> +#include <drm/drm_modes.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_print.h>
> +
> +#include <linux/bitops.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/backlight.h>
> +
> +#include <video/mipi_display.h>
> +#include <video/of_videomode.h>
> +#include <video/videomode.h>
> +

Please structure includes like this:

#include <linux/*>

#include <video/*>

#include <drm/*>

#include ""

Within each block sort the include fiels alphabetically.

I think you can drop of_videomode.h and videomode.h.


> +#define MCS_CMD_MAUCCTR		0xF0 /* Manufacturer command enable */
> +#define MCS_CMD_READ_ID1	0xDA
> +#define MCS_CMD_READ_ID2	0xDB
> +#define MCS_CMD_READ_ID3	0xDC
> +#define MCS_CMD_MTP_READ_SETTING 0xF8 /* Uncertain about name */
> +#define MCS_CMD_MTP_READ_PARAM 0xFF /* Uncertain about name */

> + *
> + * Gamma correction arrays are 10bit numbers, two consecutive bytes
> + * makes out one point on the gamma correction curve. The points are
> + * not linearly placed along the X axis, we get points 0, 1, 3, 5
> + * 7, 11, 15, 23, 31, 47, 63, 95, 127, 128, 160, 192, 208, 224, 232,
> + * 240, 244, 248, 250, 252, 254, 255. The voltages tuples form
> + * V0, V1, V3 ... V255, with 0x0000 being the lowest voltage and
> + * 0x03FF being the highest voltage.
> + *
> + * Each value must be strictly lower than the next value forming a
                                  ^ higher?
> + * rising curve like this:
> + *
> + * ^
> + * |                                        V255
> + * |                                 V254
> + * |                         ....
> + * |                    V5
> + * |           V3
> + * |     V1
> + * | V0
> + * +------------------------------------------->
> + *
> + * The details about all settings can be found in the NT35510 Application
> + * Note.
> + */
> +struct nt35510_config {
> +	/**
> +	 * @width_mm: physical panel width [mm]
> +	 */
> +	u32 width_mm;
> +	/**
> +	 * @height_mm: physical panel height [mm]
> +	 */
> +	u32 height_mm;
> +	/**
> +	 * @mode: the display mode. This is only relevant outside the panel
> +	 * in video mode: in command mode this is configuring the internal
> +	 * timing in the display controller.
> +	 */
> +	const struct drm_display_mode mode;
> +	/**
> +	 * @avdd: setting for AVDD ranging from 0x00 = 6.5V to 0x14 = 4.5V
> +	 * in 0.1V steps the default is 0x05 which means 6.0V
> +	 */
> +	u8 avdd[NT35510_P1_AVDD_LEN];
> +	/**
> +	 * @bt1ctr: setting for boost power control for the AVDD step-up
> +	 * circuit (1)
> +	 * bits 0..2 in the lower nybble controls PCK, the booster clock
s/nybble/nibble/ ?
Both spellings works so this is bike-shedding.

> +	 * frequency for the step-up circuit:
> +	 * 0 = Hsync/32
> +	 * 1 = Hsync/16
> +	 * 2 = Hsync/8
> +	 * 3 = Hsync/4
> +	 * 4 = Hsync/2
> +	 * 5 = Hsync
> +	 * 6 = Hsync x 2
> +	 * 7 = Hsync x 4
> +	 * bits 4..6 in the upper nybble controls BTP, the boosting
> +	 * amplification for the the step-up circuit:
> +	 * 0 = Disable
> +	 * 1 = 1.5 x VDDB
> +	 * 2 = 1.66 x VDDB
> +	 * 3 = 2 x VDDB
> +	 * 4 = 2.5 x VDDB
> +	 * 5 = 3 x VDDB
> +	 * The defaults are 4 and 4 yielding 0x44
> +	 */

> +
> +/**
> + * struct nt35510 - state container for the NT35510 panel
> + */
> +struct nt35510 {
> +	/**
> +	 * @dev: the container device
> +	 */
> +	struct device *dev;
> +	/**
> +	 * @conf: the specific panel configuration, as the NT35510
> +	 * can be combined with many physical panels, they can have
> +	 * different physical dimensions and gamma correction etc,
> +	 * so this is stored in the config.
> +	 */
> +	const struct nt35510_config *conf;
> +	/**
> +	 * @panel: the DRM panel object for the instance
> +	 */
> +	struct drm_panel panel;
> +	/**
> +	 * @bl: backlight device
> +	 */
> +	struct backlight_device *bl;
We have a backlight device as part of drm_panel now.
It is documented that drivers should not assign it.

We should consider to allow this - then this driver could
just assign it and then the enable() and disable() functions
would not be required.


> +	/**
> +	 * @supplies: regulators supplying the panel
> +	 */
> +	struct regulator_bulk_data supplies[2];
> +	/**
> +	 * @reset_gpio: the reset line
> +	 */
> +	struct gpio_desc *reset_gpio;
> +};
> +

> +
> +	/* Toggle RESET in accordance with datasheet page 370 */
> +	if (nt->reset_gpio) {
> +		gpiod_set_value(nt->reset_gpio, 1);
> +		/* Active min 10 us according to datasheet, let's say 20 */
> +		usleep_range(20, 1000);
> +		gpiod_set_value(nt->reset_gpio, 0);
> +		/*
> +		 * 5 ms during sleep mode, 120 ms during sleep out mode
> +		 * according to datasheet, let's use 120-140 ms.
> +		 */
> +		usleep_range(120000, 140000);
> +	}
Add an URL to the data sheet maybe?


> +
> +	ret = nt35510_read_id(nt);
> +	if (ret)
> +		return ret;
> +
> +	/* Set up stuff in  manufacturer control, page 1 */
> +	ret = nt35510_send_long(nt, dsi, MCS_CMD_MAUCCTR,
> +				ARRAY_SIZE(nt35510_mauc_select_page_1),
> +				nt35510_mauc_select_page_1);
> +	if (ret)
> +		return ret;
> +
> +	ret = nt35510_setup_power(nt);
> +	if (ret)
> +		return ret;
> +
> +	ret = nt35510_send_long(nt, dsi, NT35510_P1_SET_GAMMA_RED_POS,
> +				NT35510_P1_GAMMA_LEN,
> +				nt->conf->gamma_corr_pos_r);
> +	if (ret)
> +		return ret;
> +	ret = nt35510_send_long(nt, dsi, NT35510_P1_SET_GAMMA_GREEN_POS,
> +				NT35510_P1_GAMMA_LEN,
> +				nt->conf->gamma_corr_pos_g);
> +	if (ret)
> +		return ret;
> +	ret = nt35510_send_long(nt, dsi, NT35510_P1_SET_GAMMA_BLUE_POS,
> +				NT35510_P1_GAMMA_LEN,
> +				nt->conf->gamma_corr_pos_b);
> +	if (ret)
> +		return ret;
> +	ret = nt35510_send_long(nt, dsi, NT35510_P1_SET_GAMMA_RED_NEG,
> +				NT35510_P1_GAMMA_LEN,
> +				nt->conf->gamma_corr_neg_r);
> +	if (ret)
> +		return ret;
> +	ret = nt35510_send_long(nt, dsi, NT35510_P1_SET_GAMMA_GREEN_NEG,
> +				NT35510_P1_GAMMA_LEN,
> +				nt->conf->gamma_corr_neg_g);
> +	if (ret)
> +		return ret;
> +	ret = nt35510_send_long(nt, dsi, NT35510_P1_SET_GAMMA_BLUE_NEG,
> +				NT35510_P1_GAMMA_LEN,
> +				nt->conf->gamma_corr_neg_b);
> +	if (ret)
> +		return ret;
> +
> +	/* Set up stuff in  manufacturer control, page 0 */
> +	ret = nt35510_send_long(nt, dsi, MCS_CMD_MAUCCTR,
> +				ARRAY_SIZE(nt35510_mauc_select_page_0),
> +				nt35510_mauc_select_page_0);
> +	if (ret)
> +		return ret;
> +
> +	ret = nt35510_setup_display(nt);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}


> +
> +static int nt35510_get_modes(struct drm_panel *panel)
Add connector as argument to match drm-misc-next.

> +{
> +	struct drm_connector *connector = panel->connector;
> +	struct nt35510 *nt = panel_to_nt35510(panel);
> +	struct drm_display_mode *mode;
> +	struct drm_display_info *info;
> +
> +	info = &connector->display_info;
> +	info->width_mm = nt->conf->width_mm;
> +	info->height_mm = nt->conf->height_mm;
> +	mode = drm_mode_duplicate(panel->drm, &nt->conf->mode);
Use connector->dev - as panel no logner has a drm_device pointer.

> +	if (!mode) {
> +		DRM_ERROR("bad mode or failed to add mode\n");
> +		return -EINVAL;
> +	}
> +	drm_mode_set_name(mode);
> +	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> +
> +	mode->width_mm = nt->conf->width_mm;
> +	mode->height_mm = nt->conf->height_mm;
> +	drm_mode_probed_add(connector, mode);
> +
> +	return 1; /* Number of modes */
> +}

> +
> +static const struct of_device_id nt35510_of_match[] = {
> +	{
> +		.compatible = "hydis,hva40wv1",
> +		.data = &nt35510_hydis_hva40wv1,
> +	},
> +	{ }
Use { /* sentinel */ },


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel