All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sam Ravnborg <sam@ravnborg.org>
To: dillon.minfei@gmail.com
Cc: thierry.reding@gmail.com, airlied@linux.ie, daniel@ffwll.ch,
	robh+dt@kernel.org, linus.walleij@linaro.org,
	alexandre.torgue@foss.st.com, mcoquelin.stm32@gmail.com,
	noralf@tronnes.org, dianders@chromium.org,
	linux-stm32@st-md-mailman.stormreply.com,
	linux-arm-kernel@lists.infradead.org,
	dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 3/3] drm/panel: Add ilitek ili9341 panel driver
Date: Wed, 21 Jul 2021 18:56:53 +0200	[thread overview]
Message-ID: <YPhR1citK6Nodep/@ravnborg.org> (raw)
In-Reply-To: <1626853288-31223-4-git-send-email-dillon.minfei@gmail.com>

Hi Dillon,

On Wed, Jul 21, 2021 at 03:41:28PM +0800, dillon.minfei@gmail.com wrote:
> From: Dillon Min <dillon.minfei@gmail.com>
> 
> This driver combine tiny/ili9341.c mipi_dbi_interface driver
> with mipi_dpi_interface driver, can support ili9341 with serial
> mode or parallel rgb interface mode by register configuration.
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Dillon Min <dillon.minfei@gmail.com>

I have not looked at the driver before, sorry for being late.
A few nits in the following.

	Sam

> +#include <linux/bitops.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/delay.h>
> +#include <video/mipi_display.h>
> +#include <drm/drm_mipi_dbi.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_fb_helper.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_gem_atomic_helper.h>
> +
> +#include <drm/drm_drv.h>
> +#include <drm/drm_modes.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_print.h>

Spaces between blocks of includes.
So linux/* goes in one block, video/* in next block, drm/* in the last
block.
Sort alphabetically in each block.

> +
> +/**
> + * struct ili9341_config - the system specific ILI9341 configuration
> + * @max_spi_speed: 10000000
> + */
Unless you plan to pull this into our kernel-doc there is really no need
for a kernel-doc marker. On the other hand W=1 builds will tell if you
missed something. So there are arguments both ways.

> +struct ili9341_config {
> +	u32 max_spi_speed;
> +	/** @mode: the drm display mode */
> +	const struct drm_display_mode mode;
> +	/** @ca: TODO: need comments for this register */
> +	u8 ca[ILI9341_CA_LEN];
> +	/** @power_b: TODO: need comments for this register */
> +	u8 power_b[ILI9341_POWER_B_LEN];
> +	/** @power_seq: TODO: need comments for this register */
> +	u8 power_seq[ILI9341_POWER_SEQ_LEN];
> +	/** @dtca: TODO: need comments for this register */
> +	u8 dtca[ILI9341_DTCA_LEN];
> +	/** @dtcb: TODO: need comments for this register */
> +	u8 dtcb[ILI9341_DTCB_LEN];
> +	/** @power_a: TODO: need comments for this register */
> +	u8 power_a[ILI9341_POWER_A_LEN];
> +	/** @frc: Frame Rate Control (In Normal Mode/Full Colors) (B1h) */
> +	u8 frc[ILI9341_FRC_LEN];
> +	/** @prc: TODO: need comments for this register */
> +	u8 prc;
> +	/** @dfc_1: B6h DISCTRL (Display Function Control) */
> +	u8 dfc_1[ILI9341_DFC_1_LEN];
> +	/** @power_1: Power Control 1 (C0h) */
> +	u8 power_1;
> +	/** @power_2: Power Control 2 (C1h) */
> +	u8 power_2;
> +	/** @vcom_1: VCOM Control 1(C5h) */
> +	u8 vcom_1[ILI9341_VCOM_1_LEN];
> +	/** @vcom_2: VCOM Control 2(C7h) */
> +	u8 vcom_2;
> +	/** @address_mode: Memory Access Control (36h) */
> +	u8 address_mode;
> +	/** @g3amma_en: TODO: need comments for this register */
> +	u8 g3amma_en;
> +	/** @rgb_interface: RGB Interface Signal Control (B0h) */
> +	u8 rgb_interface;
> +	/** @dfc_2: refer to dfc_1 */
> +	u8 dfc_2[ILI9341_DFC_2_LEN];
> +	/** @column_addr: Column Address Set (2Ah) */
> +	u8 column_addr[ILI9341_COLUMN_ADDR_LEN];
> +	/** @page_addr: Page Address Set (2Bh) */
> +	u8 page_addr[ILI9341_PAGE_ADDR_LEN];
> +	/** @interface: Interface Control (F6h) */
> +	u8 interface[ILI9341_INTERFACE_LEN];
> +	/** @pixel_format: This command sets the pixel format for the RGB */
> +	/* image data used by
> +	 */
> +	u8 pixel_format;
> +	/** @gamma_curve: This command is used to select the desired Gamma */
> +	/* curve for the
> +	 */
Make this a single comment block.

> +	u8 gamma_curve;
> +	/** @pgamma: Positive Gamma Correction (E0h) */
> +	u8 pgamma[ILI9341_PGAMMA_LEN];
> +	/** @ngamma: Negative Gamma Correction (E1h) */
> +	u8 ngamma[ILI9341_NGAMMA_LEN];
> +};
> +
> +struct ili9341 {
> +	struct device *dev;
We will have struct device * both in drm_panel and in ili9341.
You could brop the one in ili9341.

> +	const struct ili9341_config *conf;
> +	struct drm_panel panel;
> +	struct gpio_desc *reset_gpio;
> +	struct gpio_desc *dc_gpio;
> +	struct mipi_dbi *dbi;
> +	u32 max_spi_speed;
> +	struct regulator_bulk_data supplies[3];
> +};
> +
> +/*
> + * The Stm32f429-disco board has a panel ili9341 connected to ltdc controller
> + */
> +static const struct ili9341_config ili9341_stm32f429_disco_data = {
> +	.max_spi_speed = 10000000,
> +	.mode = {
> +		.clock = 6100,
> +		.hdisplay = 240,
> +		.hsync_start = 240 + 10,/* hfp 10 */
> +		.hsync_end = 240 + 10 + 10,/* hsync 10 */
> +		.htotal = 240 + 10 + 10 + 20,/* hbp 20 */
> +		.vdisplay = 320,
> +		.vsync_start = 320 + 4,/* vfp 4 */
> +		.vsync_end = 320 + 4 + 2,/* vsync 2 */
> +		.vtotal = 320 + 4 + 2 + 2,/* vbp 2 */
> +		.flags = 0,
> +		.width_mm = 65,
> +		.height_mm = 50,
> +		.type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
> +	},
> +	.ca = {0xc3, 0x08, 0x50},
> +	.power_b = {0x00, 0xc1, 0x30},
> +	.power_seq = {0x64, 0x03, 0x12, 0x81},
> +	.dtca = {0x85, 0x00, 0x78},
> +	.power_a = {0x39, 0x2c, 0x00, 0x34, 0x02},
> +	.prc = 0x20,
> +	.dtcb = {0x00, 0x00},
> +	/* 0x00 fosc, 0x1b 70hz */
> +	.frc = {0x00, 0x1b},
> +	/* 0x0a Interval scan, AGND AGND AGND AGND
> +	 * 0xa2 Normally white, G1 -> G320, S720 -> S1,
> +	 *	Scan Cycle 5 frames,85ms
> +	 */
> +	.dfc_1 = {0x0a, 0xa2},
> +	/* 0x10 3.65v */
> +	.power_1 = 0x10,
> +	/* 0x10 AVDD=vci*2, VGH=vci*7, VGL=-vci*4 */
> +	.power_2 = 0x10,
> +	/* 0x45 VCOMH 4.425v, 0x15 VCOML -1.975*/
> +	.vcom_1 = {0x45, 0x15},
> +	/* 0x90 offset voltage, VMH-48, VML-48 */
> +	.vcom_2 = 0x90,
> +	/* 0xc8 Row Address Order, Column Address Order
> +	 * BGR 1
> +	 */
> +	.address_mode = 0xc8,
> +	.g3amma_en = 0x00,
> +	/* 0xc2
> +	 * Display Data Path: Memory
> +	 * RGB: DE mode
> +	 * DOTCLK polarity set (data fetched at the falling time)
> +	 */
> +	.rgb_interface = ILI9341_RGB_DISP_PATH_MEM |
> +			ILI9341_RGB_DE_MODE |
> +			ILI9341_RGB_DPL,
> +	/*
> +	 * 0x0a
> +	 * Gate outputs in non-display area: Interval scan
> +	 * Determine source/VCOM output in a non-display area in the partial
> +	 * display mode: AGND AGND AGND AGND
> +	 *
> +	 * 0xa7
> +	 * Scan Cycle: 15 frames
> +	 * fFLM = 60Hz: 255ms
> +	 * Liquid crystal type: Normally white
> +	 * Gate Output Scan Direction: G1 -> G320
> +	 * Source Output Scan Direction: S720 -> S1
> +	 *
> +	 * 0x27
> +	 * LCD Driver Line: 320 lines
> +	 *
> +	 * 0x04
> +	 * PCDIV: 4
> +	 */
> +	.dfc_2 = {0x0a, 0xa7, 0x27, 0x04},
> +	/* column address: 240 */
> +	.column_addr = {0x00, 0x00, (ILI9341_COLUMN_ADDR >> 4) & 0xff,
> +				ILI9341_COLUMN_ADDR & 0xff},
> +	/* page address: 320 */
> +	.page_addr = {0x00, 0x00, (ILI9341_PAGE_ADDR >> 4) & 0xff,
> +				ILI9341_PAGE_ADDR & 0xff},
> +	/* Memory write control: When the transfer number of data exceeds
> +	 * (EC-SC+1)*(EP-SP+1), the column and page number will be
> +	 * reset, and the exceeding data will be written into the following
> +	 * column and page.
> +	 * Display Operation Mode: RGB Interface Mode
> +	 * Interface for RAM Access: RGB interface
> +	 * 16- bit RGB interface (1 transfer/pixel)
> +	 */
> +	.interface = {ILI9341_IF_WE_MODE, 0x00,
> +			ILI9341_IF_DM_RGB | ILI9341_IF_RM_RGB},
> +	/* DPI: 16 bits / pixel */
> +	.pixel_format = ILI9341_PIXEL_DPI_16_BITS,
> +	/* Curve Selected: Gamma curve 1 (G2.2) */
> +	.gamma_curve = ILI9341_GAMMA_CURVE_1,
> +	.pgamma = {0x0f, 0x29, 0x24, 0x0c, 0x0e,
> +			0x09, 0x4e, 0x78, 0x3c, 0x09,
> +			0x13, 0x05, 0x17, 0x11, 0x00},
> +	.ngamma = {0x00, 0x16, 0x1b, 0x04, 0x11,
> +			0x07, 0x31, 0x33, 0x42, 0x05,
> +			0x0c, 0x0a, 0x28, 0x2f, 0x0f},
> +};
> +
> +static inline struct ili9341 *panel_to_ili9341(struct drm_panel *panel)
> +{
> +	return container_of(panel, struct ili9341, panel);
> +}
> +
> +static void ili9341_dpi_init(struct ili9341 *ili)
> +{
> +	struct mipi_dbi *dbi = ili->dbi;
> +	struct ili9341_config *cfg = (struct ili9341_config *)ili->conf;
> +
> +	/* Power Control */
> +	mipi_dbi_command_stackbuf(dbi, 0xca, cfg->ca, ILI9341_CA_LEN);
> +	mipi_dbi_command_stackbuf(dbi, ILI9341_POWERB, cfg->power_b,
> +				  ILI9341_POWER_B_LEN);
> +	mipi_dbi_command_stackbuf(dbi, ILI9341_POWER_SEQ, cfg->power_seq,
> +				  ILI9341_POWER_SEQ_LEN);
> +	mipi_dbi_command_stackbuf(dbi, ILI9341_DTCA, cfg->dtca,
> +				  ILI9341_DTCA_LEN);
> +	mipi_dbi_command_stackbuf(dbi, ILI9341_POWERA, cfg->power_a,
> +				  ILI9341_POWER_A_LEN);
> +	mipi_dbi_command(ili->dbi, ILI9341_PRC, cfg->prc);
> +	mipi_dbi_command_stackbuf(dbi, ILI9341_DTCB, cfg->dtcb,
> +				  ILI9341_DTCB_LEN);
> +	mipi_dbi_command_stackbuf(dbi, ILI9341_FRC, cfg->frc, ILI9341_FRC_LEN);
> +	mipi_dbi_command_stackbuf(dbi, ILI9341_DFC, cfg->dfc_1,
> +				  ILI9341_DFC_1_LEN);
> +	mipi_dbi_command(dbi, ILI9341_POWER1, cfg->power_1);
> +	mipi_dbi_command(dbi, ILI9341_POWER2, cfg->power_2);
> +
> +	/* VCOM */
> +	mipi_dbi_command_stackbuf(dbi, ILI9341_VCOM1, cfg->vcom_1,
> +				  ILI9341_VCOM_1_LEN);
> +	mipi_dbi_command(dbi, ILI9341_VCOM2, cfg->vcom_2);
> +	mipi_dbi_command(dbi, MIPI_DCS_SET_ADDRESS_MODE, cfg->address_mode);
> +
> +	/* Gamma */
> +	mipi_dbi_command(dbi, ILI9341_3GAMMA_EN, cfg->g3amma_en);
> +	mipi_dbi_command(dbi, ILI9341_RGB_INTERFACE, cfg->rgb_interface);
> +	mipi_dbi_command_stackbuf(dbi, ILI9341_DFC, cfg->dfc_2,
> +				  ILI9341_DFC_2_LEN);
> +
> +	/* Colomn address set */
> +	mipi_dbi_command_stackbuf(dbi, MIPI_DCS_SET_COLUMN_ADDRESS,
> +				  cfg->column_addr, ILI9341_COLUMN_ADDR_LEN);
> +
> +	/* Page address set */
> +	mipi_dbi_command_stackbuf(dbi, MIPI_DCS_SET_PAGE_ADDRESS,
> +				  cfg->page_addr, ILI9341_PAGE_ADDR_LEN);
> +	mipi_dbi_command_stackbuf(dbi, ILI9341_INTERFACE, cfg->interface,
> +				  ILI9341_INTERFACE_LEN);
> +
> +	/* Format */
> +	mipi_dbi_command(dbi, MIPI_DCS_SET_PIXEL_FORMAT, cfg->pixel_format);
> +	mipi_dbi_command(dbi, MIPI_DCS_WRITE_MEMORY_START);
> +	msleep(200);
> +	mipi_dbi_command(dbi, MIPI_DCS_SET_GAMMA_CURVE, cfg->gamma_curve);
> +	mipi_dbi_command_stackbuf(dbi, ILI9341_PGAMMA, cfg->pgamma,
> +				  ILI9341_PGAMMA_LEN);
> +	mipi_dbi_command_stackbuf(dbi, ILI9341_NGAMMA, cfg->ngamma,
> +				  ILI9341_NGAMMA_LEN);
> +	mipi_dbi_command(dbi, MIPI_DCS_EXIT_SLEEP_MODE);
> +	msleep(200);
> +	mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_ON);
> +	mipi_dbi_command(dbi, MIPI_DCS_WRITE_MEMORY_START);
> +
> +	dev_info(ili->dev, "initialized display rgb interface\n");
> +}
> +
> +static int ili9341_dpi_power_on(struct ili9341 *ili)
> +{
> +	int ret = 0;
> +
> +	/* Assert RESET */
> +	gpiod_set_value(ili->reset_gpio, 1);
> +
> +	/* Enable power */
> +	ret = regulator_bulk_enable(ARRAY_SIZE(ili->supplies),
> +				    ili->supplies);
> +	if (ret < 0) {
> +		dev_err(ili->dev, "unable to enable vcc\n");
> +		return ret;
> +	}
> +	msleep(20);
> +
> +	/* De-assert RESET */
> +	gpiod_set_value(ili->reset_gpio, 0);
> +	msleep(20);
> +
> +	return 0;
> +}
> +
> +static int ili9341_dpi_power_off(struct ili9341 *ili)
> +{
> +	/* Assert RESET */
> +	gpiod_set_value(ili->reset_gpio, 1);
> +
> +	/* Disable power */
> +	return regulator_bulk_disable(ARRAY_SIZE(ili->supplies),
> +				      ili->supplies);
> +}
> +
> +static int ili9341_dpi_disable(struct drm_panel *panel)
> +{
> +	struct ili9341 *ili = panel_to_ili9341(panel);
> +
> +	mipi_dbi_command(ili->dbi, MIPI_DCS_SET_DISPLAY_OFF);
> +	return 0;
> +}
> +
> +static int ili9341_dpi_unprepare(struct drm_panel *panel)
> +{
> +	struct ili9341 *ili = panel_to_ili9341(panel);
> +
> +	return ili9341_dpi_power_off(ili);
> +}
> +
> +static int ili9341_dpi_prepare(struct drm_panel *panel)
> +{
> +	struct ili9341 *ili = panel_to_ili9341(panel);
> +	int ret;
> +
> +	ret = ili9341_dpi_power_on(ili);
> +	if (ret < 0)
> +		return ret;
> +
> +	ili9341_dpi_init(ili);
> +
> +	return ret;
> +}
> +
> +static int ili9341_dpi_enable(struct drm_panel *panel)
> +{
> +	struct ili9341 *ili = panel_to_ili9341(panel);
> +
> +	mipi_dbi_command(ili->dbi, MIPI_DCS_SET_DISPLAY_ON);
> +	return 0;
> +}
> +
> +static int ili9341_dpi_get_modes(struct drm_panel *panel,
> +				 struct drm_connector *connector)
> +{
> +	struct ili9341 *ili = panel_to_ili9341(panel);
> +	struct drm_device *drm = connector->dev;
> +	struct drm_display_mode *mode;
> +	struct drm_display_info *info;
> +
> +	info = &connector->display_info;
> +	info->width_mm = ili->conf->mode.width_mm;
> +	info->height_mm = ili->conf->mode.height_mm;
> +
> +	if (ili->conf->rgb_interface & ILI9341_RGB_DPL)
> +		info->bus_flags |= DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE;
> +	else
> +		info->bus_flags |= DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE;
> +
> +	if (ili->conf->rgb_interface & ILI9341_RGB_EPL)
> +		info->bus_flags |= DRM_BUS_FLAG_DE_LOW;
> +	else
> +		info->bus_flags |= DRM_BUS_FLAG_DE_HIGH;
> +
> +	mode = drm_mode_duplicate(drm, &ili->conf->mode);
> +	if (!mode) {
> +		DRM_ERROR("bad mode or failed to add mode\n");

DRM_ERROR and friends are not used in panel/* - the new variants
drm_err() shall be used.
This goes for the other uses of DRM_* as well.

> +		return -EINVAL;
> +	}
> +	drm_mode_set_name(mode);
> +
> +	/* Set up the polarity */
> +	if (ili->conf->rgb_interface & ILI9341_RGB_HSPL)
> +		mode->flags |= DRM_MODE_FLAG_PHSYNC;
> +	else
> +		mode->flags |= DRM_MODE_FLAG_NHSYNC;
> +
> +	if (ili->conf->rgb_interface & ILI9341_RGB_VSPL)
> +		mode->flags |= DRM_MODE_FLAG_PVSYNC;
> +	else
> +		mode->flags |= DRM_MODE_FLAG_NVSYNC;
> +
> +	drm_mode_probed_add(connector, mode);
> +
> +	return 1; /* Number of modes */
> +}
> +
> +static const struct drm_panel_funcs ili9341_dpi_funcs = {
> +	.disable = ili9341_dpi_disable,
> +	.unprepare = ili9341_dpi_unprepare,
> +	.prepare = ili9341_dpi_prepare,
> +	.enable = ili9341_dpi_enable,
> +	.get_modes = ili9341_dpi_get_modes,
> +};
> +
> +static void ili9341_dbi_enable(struct drm_simple_display_pipe *pipe,
> +			       struct drm_crtc_state *crtc_state,
> +			       struct drm_plane_state *plane_state)
> +{
> +	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
> +	struct mipi_dbi *dbi = &dbidev->dbi;
> +	u8 addr_mode;
> +	int ret, idx;
> +
> +	if (!drm_dev_enter(pipe->crtc.dev, &idx))
> +		return;
> +
> +	DRM_DEBUG_KMS("\n");
> +
> +	ret = mipi_dbi_poweron_conditional_reset(dbidev);
> +	if (ret < 0)
> +		goto out_exit;
> +	if (ret == 1)
> +		goto out_enable;
> +
> +	mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_OFF);
> +
> +	mipi_dbi_command(dbi, ILI9341_POWERB, 0x00, 0xc1, 0x30);
> +	mipi_dbi_command(dbi, ILI9341_POWER_SEQ, 0x64, 0x03, 0x12, 0x81);
> +	mipi_dbi_command(dbi, ILI9341_DTCA, 0x85, 0x00, 0x78);
> +	mipi_dbi_command(dbi, ILI9341_POWERA, 0x39, 0x2c, 0x00, 0x34, 0x02);
> +	mipi_dbi_command(dbi, ILI9341_PRC, ILI9341_DBI_PRC_NORMAL);
> +	mipi_dbi_command(dbi, ILI9341_DTCB, 0x00, 0x00);
> +
> +	/* Power Control */
> +	mipi_dbi_command(dbi, ILI9341_POWER1, ILI9341_DBI_VCOMH_4P6V);
> +	mipi_dbi_command(dbi, ILI9341_POWER2, ILI9341_DBI_PWR_2_DEFAULT);
> +	/* VCOM */
> +	mipi_dbi_command(dbi, ILI9341_VCOM1, ILI9341_DBI_VCOM_1_VMH_4P25V,
> +			 ILI9341_DBI_VCOM_1_VML_1P5V);
> +	mipi_dbi_command(dbi, ILI9341_VCOM2, ILI9341_DBI_VCOM_2_DEC_58);
> +
> +	/* Memory Access Control */
> +	mipi_dbi_command(dbi, MIPI_DCS_SET_PIXEL_FORMAT,
> +			 MIPI_DCS_PIXEL_FMT_16BIT);
> +
> +	/* Frame Rate */
> +	mipi_dbi_command(dbi, ILI9341_FRC, ILI9341_DBI_FRC_DIVA & 0x03,
> +			 ILI9341_DBI_FRC_RTNA & 0x1f);
> +
> +	/* Gamma */
> +	mipi_dbi_command(dbi, ILI9341_3GAMMA_EN, 0x00);
> +	mipi_dbi_command(dbi, MIPI_DCS_SET_GAMMA_CURVE, ILI9341_GAMMA_CURVE_1);
> +	mipi_dbi_command(dbi, ILI9341_PGAMMA,
> +			 0x0f, 0x31, 0x2b, 0x0c, 0x0e, 0x08, 0x4e, 0xf1,
> +			 0x37, 0x07, 0x10, 0x03, 0x0e, 0x09, 0x00);
> +	mipi_dbi_command(dbi, ILI9341_NGAMMA,
> +			 0x00, 0x0e, 0x14, 0x03, 0x11, 0x07, 0x31, 0xc1,
> +			 0x48, 0x08, 0x0f, 0x0c, 0x31, 0x36, 0x0f);
> +
> +	/* DDRAM */
> +	mipi_dbi_command(dbi, ILI9341_ETMOD, ILI9341_DBI_EMS_GAS |
> +			 ILI9341_DBI_EMS_DTS |
> +			 ILI9341_DBI_EMS_GON);
> +
> +	/* Display */
> +	mipi_dbi_command(dbi, ILI9341_DFC, 0x08, 0x82, 0x27, 0x00);
> +	mipi_dbi_command(dbi, MIPI_DCS_EXIT_SLEEP_MODE);
> +	msleep(100);
> +
> +	mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_ON);
> +	msleep(100);
> +
> +out_enable:
> +	switch (dbidev->rotation) {
> +	default:
> +		addr_mode = ILI9341_MADCTL_MX;
> +		break;
> +	case 90:
> +		addr_mode = ILI9341_MADCTL_MV;
> +		break;
> +	case 180:
> +		addr_mode = ILI9341_MADCTL_MY;
> +		break;
> +	case 270:
> +		addr_mode = ILI9341_MADCTL_MV | ILI9341_MADCTL_MY |
> +			    ILI9341_MADCTL_MX;
> +		break;
> +	}
> +	addr_mode |= ILI9341_MADCTL_BGR;
> +	mipi_dbi_command(dbi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
> +	mipi_dbi_enable_flush(dbidev, crtc_state, plane_state);
> +	DRM_DEBUG_KMS("initialized display serial interface\n");
> +out_exit:
> +	drm_dev_exit(idx);
> +}
> +
> +static const struct drm_simple_display_pipe_funcs ili9341_dbi_funcs = {
> +	.enable = ili9341_dbi_enable,
> +	.disable = mipi_dbi_pipe_disable,
> +	.update = mipi_dbi_pipe_update,
> +	.prepare_fb = drm_gem_simple_display_pipe_prepare_fb,
> +};

So this is a full display driver. The more relevant home for this driver
is inside drm/tiny/.

> +
> +static const struct drm_display_mode ili9341_dbi_mode = {
> +	DRM_SIMPLE_MODE(240, 320, 37, 49),
> +};
> +
> +DEFINE_DRM_GEM_CMA_FOPS(ili9341_dbi_fops);
> +
> +static struct drm_driver ili9341_dbi_driver = {
> +	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
> +	.fops			= &ili9341_dbi_fops,
> +	DRM_GEM_CMA_DRIVER_OPS_VMAP,
> +	.debugfs_init		= mipi_dbi_debugfs_init,
> +	.name			= "ili9341",
> +	.desc			= "Ilitek ILI9341",
> +	.date			= "20210716",
> +	.major			= 1,
> +	.minor			= 0,
> +};
> +
> +static int _ili9341_probe(struct spi_device *spi, bool dpi)
> +{
> +	struct gpio_desc *dc;
> +	struct gpio_desc *reset;
> +	struct device *dev = &spi->dev;
> +	int ret;
> +
> +	reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset))
> +		DRM_DEV_ERROR(dev, "Failed to get gpio 'reset'\n");
> +
> +	dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
> +	if (IS_ERR(dc))
> +		DRM_DEV_ERROR(dev, "Failed to get gpio 'dc'\n");
> +
> +	if (!dpi) {
> +		struct mipi_dbi_dev *dbidev;
> +		struct mipi_dbi *dbi;
> +		struct drm_device *drm;
> +		struct regulator *vcc;
> +		u32 rotation = 0;
> +
> +		vcc = devm_regulator_get_optional(dev, "vcc");
> +		if (IS_ERR(vcc))
> +			DRM_DEV_ERROR(dev, "get optional vcc failed\n");
> +
> +		dbidev = devm_drm_dev_alloc(dev, &ili9341_dbi_driver,
> +					    struct mipi_dbi_dev, drm);
> +		if (IS_ERR(dbidev))
> +			return PTR_ERR(dbidev);
> +
> +		dbi = &dbidev->dbi;
> +		drm = &dbidev->drm;
> +		dbi->reset = reset;
> +		dbidev->regulator = vcc;
> +
> +		drm_mode_config_init(drm);
> +		dbidev->backlight = devm_of_find_backlight(dev);
> +		if (IS_ERR(dbidev->backlight))
> +			return PTR_ERR(dbidev->backlight);
> +
> +		device_property_read_u32(dev, "rotation", &rotation);
> +
> +		ret = mipi_dbi_spi_init(spi, dbi, dc);
> +		if (ret)
> +			return ret;
> +
> +		ret = mipi_dbi_dev_init(dbidev, &ili9341_dbi_funcs,
> +					&ili9341_dbi_mode, rotation);
> +		if (ret)
> +			return ret;
> +
> +		drm_mode_config_reset(drm);
> +
> +		ret = drm_dev_register(drm, 0);
> +		if (ret)
> +			return ret;
> +
> +		spi_set_drvdata(spi, drm);
> +
> +		drm_fbdev_generic_setup(drm, 0);
> +	} else {
> +		struct ili9341 *ili;
> +
> +		ili = devm_kzalloc(dev, sizeof(struct ili9341), GFP_KERNEL);
> +		if (!ili)
> +			return -ENOMEM;
> +
> +		ili->dbi = devm_kzalloc(dev, sizeof(struct mipi_dbi),
> +					GFP_KERNEL);
> +		if (!ili->dbi)
> +			return -ENOMEM;
> +
> +		ili->supplies[0].supply = "vci";
> +		ili->supplies[1].supply = "vddi";
> +		ili->supplies[2].supply = "vddi-led";
> +
> +		ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ili->supplies),
> +					      ili->supplies);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to get regulators: %d\n", ret);
> +			return ret;
> +		}
> +
> +		ret = mipi_dbi_spi_init(spi, ili->dbi, dc);
> +		if (ret)
> +			return ret;
> +
> +		spi_set_drvdata(spi, ili);
> +
> +		ili->dev = dev;
> +		ili->reset_gpio = reset;
> +		/*
> +		 * Every new incarnation of this display must have a unique
> +		 * data entry for the system in this driver.
> +		 */
> +		ili->conf = of_device_get_match_data(dev);
> +		if (!ili->conf) {
> +			dev_err(dev, "missing device configuration\n");
> +			return -ENODEV;
> +		}
> +
> +		ili->max_spi_speed = ili->conf->max_spi_speed;
> +
> +		drm_panel_init(&ili->panel, dev, &ili9341_dpi_funcs,
> +			       DRM_MODE_CONNECTOR_DPI);
> +
> +		drm_panel_add(&ili->panel);
> +	}
> +
> +	return 0;
> +}
> +
> +static int ili9341_probe(struct spi_device *spi)
> +{
> +	const struct spi_device_id *id = spi_get_device_id(spi);
> +
> +	if (!strcmp(id->name, "sf-tc240t-9370-t"))
> +		return _ili9341_probe(spi, true);
> +	else if (!strcmp(id->name, "yx240qv29"))
> +		return _ili9341_probe(spi, false);
> +
> +	return -1;
> +}
> +
> +static int ili9341_remove(struct spi_device *spi)
> +{
> +	const struct spi_device_id *id = spi_get_device_id(spi);
> +	struct ili9341 *ili = spi_get_drvdata(spi);
> +	struct drm_device *drm = spi_get_drvdata(spi);
> +
> +	if (!strcmp(id->name, "sf-tc240t-9370-t")) {
> +		ili9341_dpi_power_off(ili);
> +		drm_panel_remove(&ili->panel);
> +	} else if (!strcmp(id->name, "yx240qv29")) {
> +		drm_dev_unplug(drm);
> +		drm_atomic_helper_shutdown(drm);
> +	}
> +	return 0;
> +}
> +
> +static void ili9341_shutdown(struct spi_device *spi)
> +{
> +	const struct spi_device_id *id = spi_get_device_id(spi);
> +
> +	if (!strcmp(id->name, "yx240qv29"))
> +		drm_atomic_helper_shutdown(spi_get_drvdata(spi));
> +}
> +
> +static const struct of_device_id ili9341_of_match[] = {
> +	{
> +		.compatible = "st,sf-tc240t-9370-t",
> +		.data = &ili9341_stm32f429_disco_data,
> +	},
> +	{
> +		/* porting from tiny/ili9341.c
> +		 * for original mipi dbi compitable
> +		 */
> +		.compatible = "adafruit,yx240qv29",
> +		.data = NULL,
> +	},
> +};
> +MODULE_DEVICE_TABLE(of, ili9341_of_match);
> +
> +static const struct spi_device_id ili9341_id[] = {
> +	{ "yx240qv29", 0 },
> +	{ "sf-tc240t-9370-t", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, ili9341_id);
> +
> +static struct spi_driver ili9341_driver = {
> +	.probe = ili9341_probe,
> +	.remove = ili9341_remove,
> +	.shutdown = ili9341_shutdown,
> +	.id_table = ili9341_id,
> +	.driver = {
> +		.name = "panel-ilitek-ili9341",
> +		.of_match_table = ili9341_of_match,
> +	},
> +};
> +module_spi_driver(ili9341_driver);
> +
> +MODULE_AUTHOR("Dillon Min <dillon.minfei@gmail.com>");
> +MODULE_DESCRIPTION("ILI9341 LCD panel driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.7.4

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

WARNING: multiple messages have this Message-ID (diff)
From: Sam Ravnborg <sam@ravnborg.org>
To: dillon.minfei@gmail.com
Cc: devicetree@vger.kernel.org, noralf@tronnes.org,
	mcoquelin.stm32@gmail.com, airlied@linux.ie,
	linux-kernel@vger.kernel.org, alexandre.torgue@foss.st.com,
	dri-devel@lists.freedesktop.org, dianders@chromium.org,
	robh+dt@kernel.org, thierry.reding@gmail.com,
	linux-stm32@st-md-mailman.stormreply.com,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 3/3] drm/panel: Add ilitek ili9341 panel driver
Date: Wed, 21 Jul 2021 18:56:53 +0200	[thread overview]
Message-ID: <YPhR1citK6Nodep/@ravnborg.org> (raw)
In-Reply-To: <1626853288-31223-4-git-send-email-dillon.minfei@gmail.com>

Hi Dillon,

On Wed, Jul 21, 2021 at 03:41:28PM +0800, dillon.minfei@gmail.com wrote:
> From: Dillon Min <dillon.minfei@gmail.com>
> 
> This driver combine tiny/ili9341.c mipi_dbi_interface driver
> with mipi_dpi_interface driver, can support ili9341 with serial
> mode or parallel rgb interface mode by register configuration.
> 
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Dillon Min <dillon.minfei@gmail.com>

I have not looked at the driver before, sorry for being late.
A few nits in the following.

	Sam

> +#include <linux/bitops.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/delay.h>
> +#include <video/mipi_display.h>
> +#include <drm/drm_mipi_dbi.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_fb_helper.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_gem_atomic_helper.h>
> +
> +#include <drm/drm_drv.h>
> +#include <drm/drm_modes.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_print.h>

Spaces between blocks of includes.
So linux/* goes in one block, video/* in next block, drm/* in the last
block.
Sort alphabetically in each block.

> +
> +/**
> + * struct ili9341_config - the system specific ILI9341 configuration
> + * @max_spi_speed: 10000000
> + */
Unless you plan to pull this into our kernel-doc there is really no need
for a kernel-doc marker. On the other hand W=1 builds will tell if you
missed something. So there are arguments both ways.

> +struct ili9341_config {
> +	u32 max_spi_speed;
> +	/** @mode: the drm display mode */
> +	const struct drm_display_mode mode;
> +	/** @ca: TODO: need comments for this register */
> +	u8 ca[ILI9341_CA_LEN];
> +	/** @power_b: TODO: need comments for this register */
> +	u8 power_b[ILI9341_POWER_B_LEN];
> +	/** @power_seq: TODO: need comments for this register */
> +	u8 power_seq[ILI9341_POWER_SEQ_LEN];
> +	/** @dtca: TODO: need comments for this register */
> +	u8 dtca[ILI9341_DTCA_LEN];
> +	/** @dtcb: TODO: need comments for this register */
> +	u8 dtcb[ILI9341_DTCB_LEN];
> +	/** @power_a: TODO: need comments for this register */
> +	u8 power_a[ILI9341_POWER_A_LEN];
> +	/** @frc: Frame Rate Control (In Normal Mode/Full Colors) (B1h) */
> +	u8 frc[ILI9341_FRC_LEN];
> +	/** @prc: TODO: need comments for this register */
> +	u8 prc;
> +	/** @dfc_1: B6h DISCTRL (Display Function Control) */
> +	u8 dfc_1[ILI9341_DFC_1_LEN];
> +	/** @power_1: Power Control 1 (C0h) */
> +	u8 power_1;
> +	/** @power_2: Power Control 2 (C1h) */
> +	u8 power_2;
> +	/** @vcom_1: VCOM Control 1(C5h) */
> +	u8 vcom_1[ILI9341_VCOM_1_LEN];
> +	/** @vcom_2: VCOM Control 2(C7h) */
> +	u8 vcom_2;
> +	/** @address_mode: Memory Access Control (36h) */
> +	u8 address_mode;
> +	/** @g3amma_en: TODO: need comments for this register */
> +	u8 g3amma_en;
> +	/** @rgb_interface: RGB Interface Signal Control (B0h) */
> +	u8 rgb_interface;
> +	/** @dfc_2: refer to dfc_1 */
> +	u8 dfc_2[ILI9341_DFC_2_LEN];
> +	/** @column_addr: Column Address Set (2Ah) */
> +	u8 column_addr[ILI9341_COLUMN_ADDR_LEN];
> +	/** @page_addr: Page Address Set (2Bh) */
> +	u8 page_addr[ILI9341_PAGE_ADDR_LEN];
> +	/** @interface: Interface Control (F6h) */
> +	u8 interface[ILI9341_INTERFACE_LEN];
> +	/** @pixel_format: This command sets the pixel format for the RGB */
> +	/* image data used by
> +	 */
> +	u8 pixel_format;
> +	/** @gamma_curve: This command is used to select the desired Gamma */
> +	/* curve for the
> +	 */
Make this a single comment block.

> +	u8 gamma_curve;
> +	/** @pgamma: Positive Gamma Correction (E0h) */
> +	u8 pgamma[ILI9341_PGAMMA_LEN];
> +	/** @ngamma: Negative Gamma Correction (E1h) */
> +	u8 ngamma[ILI9341_NGAMMA_LEN];
> +};
> +
> +struct ili9341 {
> +	struct device *dev;
We will have struct device * both in drm_panel and in ili9341.
You could brop the one in ili9341.

> +	const struct ili9341_config *conf;
> +	struct drm_panel panel;
> +	struct gpio_desc *reset_gpio;
> +	struct gpio_desc *dc_gpio;
> +	struct mipi_dbi *dbi;
> +	u32 max_spi_speed;
> +	struct regulator_bulk_data supplies[3];
> +};
> +
> +/*
> + * The Stm32f429-disco board has a panel ili9341 connected to ltdc controller
> + */
> +static const struct ili9341_config ili9341_stm32f429_disco_data = {
> +	.max_spi_speed = 10000000,
> +	.mode = {
> +		.clock = 6100,
> +		.hdisplay = 240,
> +		.hsync_start = 240 + 10,/* hfp 10 */
> +		.hsync_end = 240 + 10 + 10,/* hsync 10 */
> +		.htotal = 240 + 10 + 10 + 20,/* hbp 20 */
> +		.vdisplay = 320,
> +		.vsync_start = 320 + 4,/* vfp 4 */
> +		.vsync_end = 320 + 4 + 2,/* vsync 2 */
> +		.vtotal = 320 + 4 + 2 + 2,/* vbp 2 */
> +		.flags = 0,
> +		.width_mm = 65,
> +		.height_mm = 50,
> +		.type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
> +	},
> +	.ca = {0xc3, 0x08, 0x50},
> +	.power_b = {0x00, 0xc1, 0x30},
> +	.power_seq = {0x64, 0x03, 0x12, 0x81},
> +	.dtca = {0x85, 0x00, 0x78},
> +	.power_a = {0x39, 0x2c, 0x00, 0x34, 0x02},
> +	.prc = 0x20,
> +	.dtcb = {0x00, 0x00},
> +	/* 0x00 fosc, 0x1b 70hz */
> +	.frc = {0x00, 0x1b},
> +	/* 0x0a Interval scan, AGND AGND AGND AGND
> +	 * 0xa2 Normally white, G1 -> G320, S720 -> S1,
> +	 *	Scan Cycle 5 frames,85ms
> +	 */
> +	.dfc_1 = {0x0a, 0xa2},
> +	/* 0x10 3.65v */
> +	.power_1 = 0x10,
> +	/* 0x10 AVDD=vci*2, VGH=vci*7, VGL=-vci*4 */
> +	.power_2 = 0x10,
> +	/* 0x45 VCOMH 4.425v, 0x15 VCOML -1.975*/
> +	.vcom_1 = {0x45, 0x15},
> +	/* 0x90 offset voltage, VMH-48, VML-48 */
> +	.vcom_2 = 0x90,
> +	/* 0xc8 Row Address Order, Column Address Order
> +	 * BGR 1
> +	 */
> +	.address_mode = 0xc8,
> +	.g3amma_en = 0x00,
> +	/* 0xc2
> +	 * Display Data Path: Memory
> +	 * RGB: DE mode
> +	 * DOTCLK polarity set (data fetched at the falling time)
> +	 */
> +	.rgb_interface = ILI9341_RGB_DISP_PATH_MEM |
> +			ILI9341_RGB_DE_MODE |
> +			ILI9341_RGB_DPL,
> +	/*
> +	 * 0x0a
> +	 * Gate outputs in non-display area: Interval scan
> +	 * Determine source/VCOM output in a non-display area in the partial
> +	 * display mode: AGND AGND AGND AGND
> +	 *
> +	 * 0xa7
> +	 * Scan Cycle: 15 frames
> +	 * fFLM = 60Hz: 255ms
> +	 * Liquid crystal type: Normally white
> +	 * Gate Output Scan Direction: G1 -> G320
> +	 * Source Output Scan Direction: S720 -> S1
> +	 *
> +	 * 0x27
> +	 * LCD Driver Line: 320 lines
> +	 *
> +	 * 0x04
> +	 * PCDIV: 4
> +	 */
> +	.dfc_2 = {0x0a, 0xa7, 0x27, 0x04},
> +	/* column address: 240 */
> +	.column_addr = {0x00, 0x00, (ILI9341_COLUMN_ADDR >> 4) & 0xff,
> +				ILI9341_COLUMN_ADDR & 0xff},
> +	/* page address: 320 */
> +	.page_addr = {0x00, 0x00, (ILI9341_PAGE_ADDR >> 4) & 0xff,
> +				ILI9341_PAGE_ADDR & 0xff},
> +	/* Memory write control: When the transfer number of data exceeds
> +	 * (EC-SC+1)*(EP-SP+1), the column and page number will be
> +	 * reset, and the exceeding data will be written into the following
> +	 * column and page.
> +	 * Display Operation Mode: RGB Interface Mode
> +	 * Interface for RAM Access: RGB interface
> +	 * 16- bit RGB interface (1 transfer/pixel)
> +	 */
> +	.interface = {ILI9341_IF_WE_MODE, 0x00,
> +			ILI9341_IF_DM_RGB | ILI9341_IF_RM_RGB},
> +	/* DPI: 16 bits / pixel */
> +	.pixel_format = ILI9341_PIXEL_DPI_16_BITS,
> +	/* Curve Selected: Gamma curve 1 (G2.2) */
> +	.gamma_curve = ILI9341_GAMMA_CURVE_1,
> +	.pgamma = {0x0f, 0x29, 0x24, 0x0c, 0x0e,
> +			0x09, 0x4e, 0x78, 0x3c, 0x09,
> +			0x13, 0x05, 0x17, 0x11, 0x00},
> +	.ngamma = {0x00, 0x16, 0x1b, 0x04, 0x11,
> +			0x07, 0x31, 0x33, 0x42, 0x05,
> +			0x0c, 0x0a, 0x28, 0x2f, 0x0f},
> +};
> +
> +static inline struct ili9341 *panel_to_ili9341(struct drm_panel *panel)
> +{
> +	return container_of(panel, struct ili9341, panel);
> +}
> +
> +static void ili9341_dpi_init(struct ili9341 *ili)
> +{
> +	struct mipi_dbi *dbi = ili->dbi;
> +	struct ili9341_config *cfg = (struct ili9341_config *)ili->conf;
> +
> +	/* Power Control */
> +	mipi_dbi_command_stackbuf(dbi, 0xca, cfg->ca, ILI9341_CA_LEN);
> +	mipi_dbi_command_stackbuf(dbi, ILI9341_POWERB, cfg->power_b,
> +				  ILI9341_POWER_B_LEN);
> +	mipi_dbi_command_stackbuf(dbi, ILI9341_POWER_SEQ, cfg->power_seq,
> +				  ILI9341_POWER_SEQ_LEN);
> +	mipi_dbi_command_stackbuf(dbi, ILI9341_DTCA, cfg->dtca,
> +				  ILI9341_DTCA_LEN);
> +	mipi_dbi_command_stackbuf(dbi, ILI9341_POWERA, cfg->power_a,
> +				  ILI9341_POWER_A_LEN);
> +	mipi_dbi_command(ili->dbi, ILI9341_PRC, cfg->prc);
> +	mipi_dbi_command_stackbuf(dbi, ILI9341_DTCB, cfg->dtcb,
> +				  ILI9341_DTCB_LEN);
> +	mipi_dbi_command_stackbuf(dbi, ILI9341_FRC, cfg->frc, ILI9341_FRC_LEN);
> +	mipi_dbi_command_stackbuf(dbi, ILI9341_DFC, cfg->dfc_1,
> +				  ILI9341_DFC_1_LEN);
> +	mipi_dbi_command(dbi, ILI9341_POWER1, cfg->power_1);
> +	mipi_dbi_command(dbi, ILI9341_POWER2, cfg->power_2);
> +
> +	/* VCOM */
> +	mipi_dbi_command_stackbuf(dbi, ILI9341_VCOM1, cfg->vcom_1,
> +				  ILI9341_VCOM_1_LEN);
> +	mipi_dbi_command(dbi, ILI9341_VCOM2, cfg->vcom_2);
> +	mipi_dbi_command(dbi, MIPI_DCS_SET_ADDRESS_MODE, cfg->address_mode);
> +
> +	/* Gamma */
> +	mipi_dbi_command(dbi, ILI9341_3GAMMA_EN, cfg->g3amma_en);
> +	mipi_dbi_command(dbi, ILI9341_RGB_INTERFACE, cfg->rgb_interface);
> +	mipi_dbi_command_stackbuf(dbi, ILI9341_DFC, cfg->dfc_2,
> +				  ILI9341_DFC_2_LEN);
> +
> +	/* Colomn address set */
> +	mipi_dbi_command_stackbuf(dbi, MIPI_DCS_SET_COLUMN_ADDRESS,
> +				  cfg->column_addr, ILI9341_COLUMN_ADDR_LEN);
> +
> +	/* Page address set */
> +	mipi_dbi_command_stackbuf(dbi, MIPI_DCS_SET_PAGE_ADDRESS,
> +				  cfg->page_addr, ILI9341_PAGE_ADDR_LEN);
> +	mipi_dbi_command_stackbuf(dbi, ILI9341_INTERFACE, cfg->interface,
> +				  ILI9341_INTERFACE_LEN);
> +
> +	/* Format */
> +	mipi_dbi_command(dbi, MIPI_DCS_SET_PIXEL_FORMAT, cfg->pixel_format);
> +	mipi_dbi_command(dbi, MIPI_DCS_WRITE_MEMORY_START);
> +	msleep(200);
> +	mipi_dbi_command(dbi, MIPI_DCS_SET_GAMMA_CURVE, cfg->gamma_curve);
> +	mipi_dbi_command_stackbuf(dbi, ILI9341_PGAMMA, cfg->pgamma,
> +				  ILI9341_PGAMMA_LEN);
> +	mipi_dbi_command_stackbuf(dbi, ILI9341_NGAMMA, cfg->ngamma,
> +				  ILI9341_NGAMMA_LEN);
> +	mipi_dbi_command(dbi, MIPI_DCS_EXIT_SLEEP_MODE);
> +	msleep(200);
> +	mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_ON);
> +	mipi_dbi_command(dbi, MIPI_DCS_WRITE_MEMORY_START);
> +
> +	dev_info(ili->dev, "initialized display rgb interface\n");
> +}
> +
> +static int ili9341_dpi_power_on(struct ili9341 *ili)
> +{
> +	int ret = 0;
> +
> +	/* Assert RESET */
> +	gpiod_set_value(ili->reset_gpio, 1);
> +
> +	/* Enable power */
> +	ret = regulator_bulk_enable(ARRAY_SIZE(ili->supplies),
> +				    ili->supplies);
> +	if (ret < 0) {
> +		dev_err(ili->dev, "unable to enable vcc\n");
> +		return ret;
> +	}
> +	msleep(20);
> +
> +	/* De-assert RESET */
> +	gpiod_set_value(ili->reset_gpio, 0);
> +	msleep(20);
> +
> +	return 0;
> +}
> +
> +static int ili9341_dpi_power_off(struct ili9341 *ili)
> +{
> +	/* Assert RESET */
> +	gpiod_set_value(ili->reset_gpio, 1);
> +
> +	/* Disable power */
> +	return regulator_bulk_disable(ARRAY_SIZE(ili->supplies),
> +				      ili->supplies);
> +}
> +
> +static int ili9341_dpi_disable(struct drm_panel *panel)
> +{
> +	struct ili9341 *ili = panel_to_ili9341(panel);
> +
> +	mipi_dbi_command(ili->dbi, MIPI_DCS_SET_DISPLAY_OFF);
> +	return 0;
> +}
> +
> +static int ili9341_dpi_unprepare(struct drm_panel *panel)
> +{
> +	struct ili9341 *ili = panel_to_ili9341(panel);
> +
> +	return ili9341_dpi_power_off(ili);
> +}
> +
> +static int ili9341_dpi_prepare(struct drm_panel *panel)
> +{
> +	struct ili9341 *ili = panel_to_ili9341(panel);
> +	int ret;
> +
> +	ret = ili9341_dpi_power_on(ili);
> +	if (ret < 0)
> +		return ret;
> +
> +	ili9341_dpi_init(ili);
> +
> +	return ret;
> +}
> +
> +static int ili9341_dpi_enable(struct drm_panel *panel)
> +{
> +	struct ili9341 *ili = panel_to_ili9341(panel);
> +
> +	mipi_dbi_command(ili->dbi, MIPI_DCS_SET_DISPLAY_ON);
> +	return 0;
> +}
> +
> +static int ili9341_dpi_get_modes(struct drm_panel *panel,
> +				 struct drm_connector *connector)
> +{
> +	struct ili9341 *ili = panel_to_ili9341(panel);
> +	struct drm_device *drm = connector->dev;
> +	struct drm_display_mode *mode;
> +	struct drm_display_info *info;
> +
> +	info = &connector->display_info;
> +	info->width_mm = ili->conf->mode.width_mm;
> +	info->height_mm = ili->conf->mode.height_mm;
> +
> +	if (ili->conf->rgb_interface & ILI9341_RGB_DPL)
> +		info->bus_flags |= DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE;
> +	else
> +		info->bus_flags |= DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE;
> +
> +	if (ili->conf->rgb_interface & ILI9341_RGB_EPL)
> +		info->bus_flags |= DRM_BUS_FLAG_DE_LOW;
> +	else
> +		info->bus_flags |= DRM_BUS_FLAG_DE_HIGH;
> +
> +	mode = drm_mode_duplicate(drm, &ili->conf->mode);
> +	if (!mode) {
> +		DRM_ERROR("bad mode or failed to add mode\n");

DRM_ERROR and friends are not used in panel/* - the new variants
drm_err() shall be used.
This goes for the other uses of DRM_* as well.

> +		return -EINVAL;
> +	}
> +	drm_mode_set_name(mode);
> +
> +	/* Set up the polarity */
> +	if (ili->conf->rgb_interface & ILI9341_RGB_HSPL)
> +		mode->flags |= DRM_MODE_FLAG_PHSYNC;
> +	else
> +		mode->flags |= DRM_MODE_FLAG_NHSYNC;
> +
> +	if (ili->conf->rgb_interface & ILI9341_RGB_VSPL)
> +		mode->flags |= DRM_MODE_FLAG_PVSYNC;
> +	else
> +		mode->flags |= DRM_MODE_FLAG_NVSYNC;
> +
> +	drm_mode_probed_add(connector, mode);
> +
> +	return 1; /* Number of modes */
> +}
> +
> +static const struct drm_panel_funcs ili9341_dpi_funcs = {
> +	.disable = ili9341_dpi_disable,
> +	.unprepare = ili9341_dpi_unprepare,
> +	.prepare = ili9341_dpi_prepare,
> +	.enable = ili9341_dpi_enable,
> +	.get_modes = ili9341_dpi_get_modes,
> +};
> +
> +static void ili9341_dbi_enable(struct drm_simple_display_pipe *pipe,
> +			       struct drm_crtc_state *crtc_state,
> +			       struct drm_plane_state *plane_state)
> +{
> +	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
> +	struct mipi_dbi *dbi = &dbidev->dbi;
> +	u8 addr_mode;
> +	int ret, idx;
> +
> +	if (!drm_dev_enter(pipe->crtc.dev, &idx))
> +		return;
> +
> +	DRM_DEBUG_KMS("\n");
> +
> +	ret = mipi_dbi_poweron_conditional_reset(dbidev);
> +	if (ret < 0)
> +		goto out_exit;
> +	if (ret == 1)
> +		goto out_enable;
> +
> +	mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_OFF);
> +
> +	mipi_dbi_command(dbi, ILI9341_POWERB, 0x00, 0xc1, 0x30);
> +	mipi_dbi_command(dbi, ILI9341_POWER_SEQ, 0x64, 0x03, 0x12, 0x81);
> +	mipi_dbi_command(dbi, ILI9341_DTCA, 0x85, 0x00, 0x78);
> +	mipi_dbi_command(dbi, ILI9341_POWERA, 0x39, 0x2c, 0x00, 0x34, 0x02);
> +	mipi_dbi_command(dbi, ILI9341_PRC, ILI9341_DBI_PRC_NORMAL);
> +	mipi_dbi_command(dbi, ILI9341_DTCB, 0x00, 0x00);
> +
> +	/* Power Control */
> +	mipi_dbi_command(dbi, ILI9341_POWER1, ILI9341_DBI_VCOMH_4P6V);
> +	mipi_dbi_command(dbi, ILI9341_POWER2, ILI9341_DBI_PWR_2_DEFAULT);
> +	/* VCOM */
> +	mipi_dbi_command(dbi, ILI9341_VCOM1, ILI9341_DBI_VCOM_1_VMH_4P25V,
> +			 ILI9341_DBI_VCOM_1_VML_1P5V);
> +	mipi_dbi_command(dbi, ILI9341_VCOM2, ILI9341_DBI_VCOM_2_DEC_58);
> +
> +	/* Memory Access Control */
> +	mipi_dbi_command(dbi, MIPI_DCS_SET_PIXEL_FORMAT,
> +			 MIPI_DCS_PIXEL_FMT_16BIT);
> +
> +	/* Frame Rate */
> +	mipi_dbi_command(dbi, ILI9341_FRC, ILI9341_DBI_FRC_DIVA & 0x03,
> +			 ILI9341_DBI_FRC_RTNA & 0x1f);
> +
> +	/* Gamma */
> +	mipi_dbi_command(dbi, ILI9341_3GAMMA_EN, 0x00);
> +	mipi_dbi_command(dbi, MIPI_DCS_SET_GAMMA_CURVE, ILI9341_GAMMA_CURVE_1);
> +	mipi_dbi_command(dbi, ILI9341_PGAMMA,
> +			 0x0f, 0x31, 0x2b, 0x0c, 0x0e, 0x08, 0x4e, 0xf1,
> +			 0x37, 0x07, 0x10, 0x03, 0x0e, 0x09, 0x00);
> +	mipi_dbi_command(dbi, ILI9341_NGAMMA,
> +			 0x00, 0x0e, 0x14, 0x03, 0x11, 0x07, 0x31, 0xc1,
> +			 0x48, 0x08, 0x0f, 0x0c, 0x31, 0x36, 0x0f);
> +
> +	/* DDRAM */
> +	mipi_dbi_command(dbi, ILI9341_ETMOD, ILI9341_DBI_EMS_GAS |
> +			 ILI9341_DBI_EMS_DTS |
> +			 ILI9341_DBI_EMS_GON);
> +
> +	/* Display */
> +	mipi_dbi_command(dbi, ILI9341_DFC, 0x08, 0x82, 0x27, 0x00);
> +	mipi_dbi_command(dbi, MIPI_DCS_EXIT_SLEEP_MODE);
> +	msleep(100);
> +
> +	mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_ON);
> +	msleep(100);
> +
> +out_enable:
> +	switch (dbidev->rotation) {
> +	default:
> +		addr_mode = ILI9341_MADCTL_MX;
> +		break;
> +	case 90:
> +		addr_mode = ILI9341_MADCTL_MV;
> +		break;
> +	case 180:
> +		addr_mode = ILI9341_MADCTL_MY;
> +		break;
> +	case 270:
> +		addr_mode = ILI9341_MADCTL_MV | ILI9341_MADCTL_MY |
> +			    ILI9341_MADCTL_MX;
> +		break;
> +	}
> +	addr_mode |= ILI9341_MADCTL_BGR;
> +	mipi_dbi_command(dbi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
> +	mipi_dbi_enable_flush(dbidev, crtc_state, plane_state);
> +	DRM_DEBUG_KMS("initialized display serial interface\n");
> +out_exit:
> +	drm_dev_exit(idx);
> +}
> +
> +static const struct drm_simple_display_pipe_funcs ili9341_dbi_funcs = {
> +	.enable = ili9341_dbi_enable,
> +	.disable = mipi_dbi_pipe_disable,
> +	.update = mipi_dbi_pipe_update,
> +	.prepare_fb = drm_gem_simple_display_pipe_prepare_fb,
> +};

So this is a full display driver. The more relevant home for this driver
is inside drm/tiny/.

> +
> +static const struct drm_display_mode ili9341_dbi_mode = {
> +	DRM_SIMPLE_MODE(240, 320, 37, 49),
> +};
> +
> +DEFINE_DRM_GEM_CMA_FOPS(ili9341_dbi_fops);
> +
> +static struct drm_driver ili9341_dbi_driver = {
> +	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
> +	.fops			= &ili9341_dbi_fops,
> +	DRM_GEM_CMA_DRIVER_OPS_VMAP,
> +	.debugfs_init		= mipi_dbi_debugfs_init,
> +	.name			= "ili9341",
> +	.desc			= "Ilitek ILI9341",
> +	.date			= "20210716",
> +	.major			= 1,
> +	.minor			= 0,
> +};
> +
> +static int _ili9341_probe(struct spi_device *spi, bool dpi)
> +{
> +	struct gpio_desc *dc;
> +	struct gpio_desc *reset;
> +	struct device *dev = &spi->dev;
> +	int ret;
> +
> +	reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset))
> +		DRM_DEV_ERROR(dev, "Failed to get gpio 'reset'\n");
> +
> +	dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
> +	if (IS_ERR(dc))
> +		DRM_DEV_ERROR(dev, "Failed to get gpio 'dc'\n");
> +
> +	if (!dpi) {
> +		struct mipi_dbi_dev *dbidev;
> +		struct mipi_dbi *dbi;
> +		struct drm_device *drm;
> +		struct regulator *vcc;
> +		u32 rotation = 0;
> +
> +		vcc = devm_regulator_get_optional(dev, "vcc");
> +		if (IS_ERR(vcc))
> +			DRM_DEV_ERROR(dev, "get optional vcc failed\n");
> +
> +		dbidev = devm_drm_dev_alloc(dev, &ili9341_dbi_driver,
> +					    struct mipi_dbi_dev, drm);
> +		if (IS_ERR(dbidev))
> +			return PTR_ERR(dbidev);
> +
> +		dbi = &dbidev->dbi;
> +		drm = &dbidev->drm;
> +		dbi->reset = reset;
> +		dbidev->regulator = vcc;
> +
> +		drm_mode_config_init(drm);
> +		dbidev->backlight = devm_of_find_backlight(dev);
> +		if (IS_ERR(dbidev->backlight))
> +			return PTR_ERR(dbidev->backlight);
> +
> +		device_property_read_u32(dev, "rotation", &rotation);
> +
> +		ret = mipi_dbi_spi_init(spi, dbi, dc);
> +		if (ret)
> +			return ret;
> +
> +		ret = mipi_dbi_dev_init(dbidev, &ili9341_dbi_funcs,
> +					&ili9341_dbi_mode, rotation);
> +		if (ret)
> +			return ret;
> +
> +		drm_mode_config_reset(drm);
> +
> +		ret = drm_dev_register(drm, 0);
> +		if (ret)
> +			return ret;
> +
> +		spi_set_drvdata(spi, drm);
> +
> +		drm_fbdev_generic_setup(drm, 0);
> +	} else {
> +		struct ili9341 *ili;
> +
> +		ili = devm_kzalloc(dev, sizeof(struct ili9341), GFP_KERNEL);
> +		if (!ili)
> +			return -ENOMEM;
> +
> +		ili->dbi = devm_kzalloc(dev, sizeof(struct mipi_dbi),
> +					GFP_KERNEL);
> +		if (!ili->dbi)
> +			return -ENOMEM;
> +
> +		ili->supplies[0].supply = "vci";
> +		ili->supplies[1].supply = "vddi";
> +		ili->supplies[2].supply = "vddi-led";
> +
> +		ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ili->supplies),
> +					      ili->supplies);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to get regulators: %d\n", ret);
> +			return ret;
> +		}
> +
> +		ret = mipi_dbi_spi_init(spi, ili->dbi, dc);
> +		if (ret)
> +			return ret;
> +
> +		spi_set_drvdata(spi, ili);
> +
> +		ili->dev = dev;
> +		ili->reset_gpio = reset;
> +		/*
> +		 * Every new incarnation of this display must have a unique
> +		 * data entry for the system in this driver.
> +		 */
> +		ili->conf = of_device_get_match_data(dev);
> +		if (!ili->conf) {
> +			dev_err(dev, "missing device configuration\n");
> +			return -ENODEV;
> +		}
> +
> +		ili->max_spi_speed = ili->conf->max_spi_speed;
> +
> +		drm_panel_init(&ili->panel, dev, &ili9341_dpi_funcs,
> +			       DRM_MODE_CONNECTOR_DPI);
> +
> +		drm_panel_add(&ili->panel);
> +	}
> +
> +	return 0;
> +}
> +
> +static int ili9341_probe(struct spi_device *spi)
> +{
> +	const struct spi_device_id *id = spi_get_device_id(spi);
> +
> +	if (!strcmp(id->name, "sf-tc240t-9370-t"))
> +		return _ili9341_probe(spi, true);
> +	else if (!strcmp(id->name, "yx240qv29"))
> +		return _ili9341_probe(spi, false);
> +
> +	return -1;
> +}
> +
> +static int ili9341_remove(struct spi_device *spi)
> +{
> +	const struct spi_device_id *id = spi_get_device_id(spi);
> +	struct ili9341 *ili = spi_get_drvdata(spi);
> +	struct drm_device *drm = spi_get_drvdata(spi);
> +
> +	if (!strcmp(id->name, "sf-tc240t-9370-t")) {
> +		ili9341_dpi_power_off(ili);
> +		drm_panel_remove(&ili->panel);
> +	} else if (!strcmp(id->name, "yx240qv29")) {
> +		drm_dev_unplug(drm);
> +		drm_atomic_helper_shutdown(drm);
> +	}
> +	return 0;
> +}
> +
> +static void ili9341_shutdown(struct spi_device *spi)
> +{
> +	const struct spi_device_id *id = spi_get_device_id(spi);
> +
> +	if (!strcmp(id->name, "yx240qv29"))
> +		drm_atomic_helper_shutdown(spi_get_drvdata(spi));
> +}
> +
> +static const struct of_device_id ili9341_of_match[] = {
> +	{
> +		.compatible = "st,sf-tc240t-9370-t",
> +		.data = &ili9341_stm32f429_disco_data,
> +	},
> +	{
> +		/* porting from tiny/ili9341.c
> +		 * for original mipi dbi compitable
> +		 */
> +		.compatible = "adafruit,yx240qv29",
> +		.data = NULL,
> +	},
> +};
> +MODULE_DEVICE_TABLE(of, ili9341_of_match);
> +
> +static const struct spi_device_id ili9341_id[] = {
> +	{ "yx240qv29", 0 },
> +	{ "sf-tc240t-9370-t", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, ili9341_id);
> +
> +static struct spi_driver ili9341_driver = {
> +	.probe = ili9341_probe,
> +	.remove = ili9341_remove,
> +	.shutdown = ili9341_shutdown,
> +	.id_table = ili9341_id,
> +	.driver = {
> +		.name = "panel-ilitek-ili9341",
> +		.of_match_table = ili9341_of_match,
> +	},
> +};
> +module_spi_driver(ili9341_driver);
> +
> +MODULE_AUTHOR("Dillon Min <dillon.minfei@gmail.com>");
> +MODULE_DESCRIPTION("ILI9341 LCD panel driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.7.4

  parent reply	other threads:[~2021-07-21 16:59 UTC|newest]

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-21  7:41 [PATCH v2 0/3] Add ilitek ili9341 panel driver dillon.minfei
2021-07-21  7:41 ` dillon.minfei
2021-07-21  7:41 ` dillon.minfei
2021-07-21  7:41 ` [PATCH v2 1/3] dt-bindings: display: panel: Add ilitek ili9341 panel bindings dillon.minfei
2021-07-21  7:41   ` dillon.minfei
2021-07-21  7:41   ` dillon.minfei
2021-07-21 14:00   ` Linus Walleij
2021-07-21 14:00     ` Linus Walleij
2021-07-21 14:00     ` Linus Walleij
2021-07-21 14:17     ` Sam Ravnborg
2021-07-21 14:17       ` Sam Ravnborg
2021-07-21 14:22       ` Linus Walleij
2021-07-21 14:22         ` Linus Walleij
2021-07-21 14:22         ` Linus Walleij
2021-07-21  7:41 ` [PATCH v2 2/3] ARM: dts: stm32: fix dtbs_check warning on ili9341 dts binding dillon.minfei
2021-07-21  7:41   ` dillon.minfei
2021-07-21  7:41   ` dillon.minfei
2021-07-21 14:53   ` Dillon Min
2021-07-21 14:53     ` Dillon Min
2021-07-21 14:53     ` Dillon Min
2021-07-21  7:41 ` [PATCH v2 3/3] drm/panel: Add ilitek ili9341 panel driver dillon.minfei
2021-07-21  7:41   ` dillon.minfei
2021-07-21  7:41   ` dillon.minfei
2021-07-21 14:02   ` Linus Walleij
2021-07-21 14:02     ` Linus Walleij
2021-07-21 14:02     ` Linus Walleij
2021-07-21 15:00     ` Dillon Min
2021-07-21 15:00       ` Dillon Min
2021-07-21 15:00       ` Dillon Min
2021-07-21 15:47   ` Jagan Teki
2021-07-21 15:47     ` Jagan Teki
2021-07-21 15:47     ` Jagan Teki
2021-07-21 23:08     ` Dillon Min
2021-07-21 23:08       ` Dillon Min
2021-07-21 23:08       ` Dillon Min
2021-07-22  7:58       ` Jagan Teki
2021-07-22  7:58         ` Jagan Teki
2021-07-22  7:58         ` Jagan Teki
2021-07-21 16:56   ` Sam Ravnborg [this message]
2021-07-21 16:56     ` Sam Ravnborg
2021-07-21 23:28     ` Dillon Min
2021-07-21 23:28       ` Dillon Min
2021-07-21 23:28       ` Dillon Min
2021-07-21 17:42   ` Noralf Trønnes
2021-07-21 17:42     ` Noralf Trønnes
2021-07-21 17:42     ` Noralf Trønnes
2021-07-22  2:07     ` Dillon Min
2021-07-22  2:07       ` Dillon Min
2021-07-22  2:07       ` Dillon Min
2021-07-22  7:03       ` Noralf Trønnes
2021-07-22  7:03         ` Noralf Trønnes
2021-07-22  7:03         ` Noralf Trønnes
2021-07-22  7:10         ` Dillon Min
2021-07-22  7:10           ` Dillon Min
2021-07-22  7:10           ` Dillon Min

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YPhR1citK6Nodep/@ravnborg.org \
    --to=sam@ravnborg.org \
    --cc=airlied@linux.ie \
    --cc=alexandre.torgue@foss.st.com \
    --cc=daniel@ffwll.ch \
    --cc=devicetree@vger.kernel.org \
    --cc=dianders@chromium.org \
    --cc=dillon.minfei@gmail.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-stm32@st-md-mailman.stormreply.com \
    --cc=mcoquelin.stm32@gmail.com \
    --cc=noralf@tronnes.org \
    --cc=robh+dt@kernel.org \
    --cc=thierry.reding@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.