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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sam,

On Wed, Jul 10, 2019 at 09:48:55AM +0200, Sam Ravnborg wrote:
> 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.

I think it's a construct that could be used in more drivers, when
dealing with large sequences of writes.

> 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.

This could be fixed if desired, but I would do so on top of this patch
to minimise the changes compared to the omapdrm-specific panel driver.

> > +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.

I'll drop it.

> > +
> > +	/* 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@ravnborg.org>
> 
> 	Sam

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel