[PATCH 2/2] drm/panel: Add driver for Novatek NT35510-based panels
Sam Ravnborg
sam at ravnborg.org
Sat Jan 4 18:25:21 UTC 2020
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 at 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 at redhat.com>
> L: dri-devel at 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 at 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 */ },
More information about the dri-devel
mailing list