[PATCH 23/60] drm/panel: Add driver for the Toppology TD028TTEC1 panel

[Sam Ravnborg]

Hi Laurent.

This driver looks very good.

On Sun, Jul 07, 2019 at 09:19:00PM +0300, Laurent Pinchart wrote:
> This panel is used on the OpenMoko Neo FreeRunner and Neo 1973.
Add info in Kconfig help entry?

> 
> +config DRM_PANEL_TPO_TD028TTEC1
> +	tristate "TPO TD028TTEC1 panel driver"
Maybe spell out TPO like "TPO (Topology) TD028..."

> +	depends on OF && SPI
> +	depends on BACKLIGHT_CLASS_DEVICE
> +	help
> +	  Say Y here if you want to enable support for TPO TD028TTEC1 480x640
> +	  2.8" panel.
> +
>  config DRM_PANEL_TPO_TPG110
>  	tristate "TPO TPG 800x400 panel"
>  	depends on OF && SPI && GPIOLIB
>  obj-$(CONFIG_DRM_PANEL_TRULY_NT35597_WQXGA) += panel-truly-nt35597.o
> diff --git a/drivers/gpu/drm/panel/panel-tpo-td028ttec1.c b/drivers/gpu/drm/panel/panel-tpo-td028ttec1.c
> new file mode 100644
> index 000000000000..05af9ea6339c
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-tpo-td028ttec1.c
> +
> +static int jbt_ret_write_0(struct td028ttec1_device *lcd, u8 reg, int *err)
> +{
> +	struct spi_device *spi = lcd->spi;
> +	u16 tx_buf = JBT_COMMAND | reg;
> +	int ret;
> +
> +	if (err && *err)
> +		return *err;
> +
> +	ret = spi_write(spi, (u8 *)&tx_buf, sizeof(tx_buf));
> +	if (ret < 0) {
> +		dev_err(&spi->dev, "%s: SPI write failed: %d\n", __func__, ret);
> +		if (err)
> +			*err = ret;
> +	}
> +
> +	return ret;
> +}
I like the way *err is used here.
So if one call fails, the remaining calls are ignored.

The way the code is written above it will only work on a little endian
box, as the values are stored in an u16 that is later seen as an array of
bytes.
This is also true for the remaing similar functions and may be OK.
We do not see any real demands for big endian anyway.

> +static int td028ttec1_enable(struct drm_panel *panel)
> +{
> +	struct td028ttec1_device *lcd = to_td028ttec1_device(panel);
> +	unsigned int i;
> +	int ret = 0;
> +
> +	/* Three times command zero */
> +	for (i = 0; i < 3; ++i) {
> +		jbt_ret_write_0(lcd, 0x00, &ret);
> +		usleep_range(1000, 2000);
> +	}
> +
> +	if (ret)
> +		return ret;
This if (ret) is really not needed.
It somehow short-circuit the principle used in the rest of the function
here. All jbt_reg_write() will be nop if ret != 0.

> +
> +	/* deep standby out */
> +	jbt_reg_write_1(lcd, JBT_REG_POWER_ON_OFF, 0x17, &ret);
> +
> +	/* RGB I/F on, RAM write off, QVGA through, SIGCON enable */
> +	jbt_reg_write_1(lcd, JBT_REG_DISPLAY_MODE, 0x80, &ret);
> +
> +	/* Quad mode off */
> +	jbt_reg_write_1(lcd, JBT_REG_QUAD_RATE, 0x00, &ret);
> +
> +	/* AVDD on, XVDD on */
> +	jbt_reg_write_1(lcd, JBT_REG_POWER_ON_OFF, 0x16, &ret);
> +
> +	/* Output control */
> +	jbt_reg_write_2(lcd, JBT_REG_OUTPUT_CONTROL, 0xfff9, &ret);
> +
> +	/* Sleep mode off */
> +	jbt_ret_write_0(lcd, JBT_REG_SLEEP_OUT, &ret);
> +
> +	/* at this point we have like 50% grey */
> +
> +	/* initialize register set */
> +	jbt_reg_write_1(lcd, JBT_REG_DISPLAY_MODE1, 0x01, &ret);
> +	jbt_reg_write_1(lcd, JBT_REG_DISPLAY_MODE2, 0x00, &ret);
> +	jbt_reg_write_1(lcd, JBT_REG_RGB_FORMAT, 0x60, &ret);
> +	jbt_reg_write_1(lcd, JBT_REG_DRIVE_SYSTEM, 0x10, &ret);
> +	jbt_reg_write_1(lcd, JBT_REG_BOOSTER_OP, 0x56, &ret);
> +	jbt_reg_write_1(lcd, JBT_REG_BOOSTER_MODE, 0x33, &ret);
> +	jbt_reg_write_1(lcd, JBT_REG_BOOSTER_FREQ, 0x11, &ret);
> +	jbt_reg_write_1(lcd, JBT_REG_BOOSTER_FREQ, 0x11, &ret);
> +	jbt_reg_write_1(lcd, JBT_REG_OPAMP_SYSCLK, 0x02, &ret);
> +	jbt_reg_write_1(lcd, JBT_REG_VSC_VOLTAGE, 0x2b, &ret);
> +	jbt_reg_write_1(lcd, JBT_REG_VCOM_VOLTAGE, 0x40, &ret);
> +	jbt_reg_write_1(lcd, JBT_REG_EXT_DISPL, 0x03, &ret);
> +	jbt_reg_write_1(lcd, JBT_REG_DCCLK_DCEV, 0x04, &ret);
> +	/*
> +	 * default of 0x02 in JBT_REG_ASW_SLEW responsible for 72Hz requirement
> +	 * to avoid red / blue flicker
> +	 */
> +	jbt_reg_write_1(lcd, JBT_REG_ASW_SLEW, 0x04, &ret);
> +	jbt_reg_write_1(lcd, JBT_REG_DUMMY_DISPLAY, 0x00, &ret);
> +
> +	jbt_reg_write_1(lcd, JBT_REG_SLEEP_OUT_FR_A, 0x11, &ret);
> +	jbt_reg_write_1(lcd, JBT_REG_SLEEP_OUT_FR_B, 0x11, &ret);
> +	jbt_reg_write_1(lcd, JBT_REG_SLEEP_OUT_FR_C, 0x11, &ret);
> +	jbt_reg_write_2(lcd, JBT_REG_SLEEP_IN_LCCNT_D, 0x2040, &ret);
> +	jbt_reg_write_2(lcd, JBT_REG_SLEEP_IN_LCCNT_E, 0x60c0, &ret);
> +	jbt_reg_write_2(lcd, JBT_REG_SLEEP_IN_LCCNT_F, 0x1020, &ret);
> +	jbt_reg_write_2(lcd, JBT_REG_SLEEP_IN_LCCNT_G, 0x60c0, &ret);
> +
> +	jbt_reg_write_2(lcd, JBT_REG_GAMMA1_FINE_1, 0x5533, &ret);
> +	jbt_reg_write_1(lcd, JBT_REG_GAMMA1_FINE_2, 0x00, &ret);
> +	jbt_reg_write_1(lcd, JBT_REG_GAMMA1_INCLINATION, 0x00, &ret);
> +	jbt_reg_write_1(lcd, JBT_REG_GAMMA1_BLUE_OFFSET, 0x00, &ret);
> +
> +	jbt_reg_write_2(lcd, JBT_REG_HCLOCK_VGA, 0x1f0, &ret);
> +	jbt_reg_write_1(lcd, JBT_REG_BLANK_CONTROL, 0x02, &ret);
> +	jbt_reg_write_2(lcd, JBT_REG_BLANK_TH_TV, 0x0804, &ret);
> +
> +	jbt_reg_write_1(lcd, JBT_REG_CKV_ON_OFF, 0x01, &ret);
> +	jbt_reg_write_2(lcd, JBT_REG_CKV_1_2, 0x0000, &ret);
> +
> +	jbt_reg_write_2(lcd, JBT_REG_OEV_TIMING, 0x0d0e, &ret);
> +	jbt_reg_write_2(lcd, JBT_REG_ASW_TIMING_1, 0x11a4, &ret);
> +	jbt_reg_write_1(lcd, JBT_REG_ASW_TIMING_2, 0x0e, &ret);
> +
> +	jbt_ret_write_0(lcd, JBT_REG_DISPLAY_ON, &ret);
> +
> +	if (ret)
> +		return ret;
> +
> +	backlight_enable(lcd->backlight);
> +
> +	return 0;
> +}
> +
> +static const struct drm_display_mode td028ttec1_mode = {
> +	.clock = 22153,
> +	.hdisplay = 480,
> +	.hsync_start = 480 + 24,
> +	.hsync_end = 480 + 24 + 8,
> +	.htotal = 480 + 24 + 8 + 8,
> +	.vdisplay = 640,
> +	.vsync_start = 640 + 4,
> +	.vsync_end = 640 + 4 + 2,
> +	.vtotal = 640 + 4 + 2 + 2,
> +	.vrefresh = 66,
> +	.type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
> +	.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
> +};
Add width_mm + height_mm.

> +static int td028ttec1_remove(struct spi_device *spi)
> +{
> +	struct td028ttec1_device *lcd = spi_get_drvdata(spi);
> +
> +	drm_panel_remove(&lcd->panel);
> +	td028ttec1_disable(&lcd->panel);
Use drm_panel_disable();

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id td028ttec1_of_match[] = {
> +	{ .compatible = "tpo,td028ttec1", },
> +	/* DT backward compatibility. */
> +	{ .compatible = "toppoly,td028ttec1", },
> +	{},
{ /* sentinel */ },

With the above nits fixed/considered:
Reviewed-by: Sam Ravnborg <sam at ravnborg.org>

	Sam