Re: [PATCH 2/2] drm/panel: Add driver for Novatek NT35510-based panels

[Sam Ravnborg <sam@ravnborg.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