Re: [PATCH 24/60] drm/panel: Add driver for the Toppology TD043MTEA1 panel

From: Sam Ravnborg <sam@ravnborg.org>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>,
	Sebastian Reichel <sebastian.reichel@collabora.com>,
	dri-devel@lists.freedesktop.org,
	Tomi Valkeinen <tomi.valkeinen@ti.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	Sean Paul <sean@poorly.run>
Subject: Re: [PATCH 24/60] drm/panel: Add driver for the Toppology TD043MTEA1 panel
Date: Wed, 10 Jul 2019 15:09:17 +0200	[thread overview]
Message-ID: <20190710130917.GA11170@ravnborg.org> (raw)
In-Reply-To: <20190707181937.6250-21-laurent.pinchart@ideasonboard.com>

Hi Laurent.

I had assumed this driver would look like the other Topology driver, but
they differ a lot. So it makes sense to have different drivers.

This driver implements suspend/resume.
But the correct way would be to implment prepare/unprepare.

The power_on(), power_off() functions would then be embedded in
the prepare(), unprepare() functions as there would be only one user.

See additional comments in the following.

	Sam

On Sun, Jul 07, 2019 at 09:19:01PM +0300, Laurent Pinchart wrote:
> This panel is used on the OMAP3 Pandora.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/panel/Kconfig                |   7 +
>  drivers/gpu/drm/panel/Makefile               |   1 +
>  drivers/gpu/drm/panel/panel-tpo-td043mtea1.c | 510 +++++++++++++++++++
>  3 files changed, 518 insertions(+)
>  create mode 100644 drivers/gpu/drm/panel/panel-tpo-td043mtea1.c
> 
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index b7099d211061..8f3660c73044 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -312,6 +312,13 @@ config DRM_PANEL_TPO_TD028TTEC1
>  	  Say Y here if you want to enable support for TPO TD028TTEC1 480x640
>  	  2.8" panel.
>  
> +config DRM_PANEL_TPO_TD043MTEA1
> +	tristate "TPO TD043MTEA1 panel driver"
Spell out TPO?
> +	depends on GPIOLIB && OF && REGULATOR && SPI
> +	help
> +	  Say Y here if you want to enable support for TPO TD043MTEA1 800x480
> +	  4.3" panel.
Maybe tell it is used on OMAP3 Pandora

> +
>  config DRM_PANEL_TPO_TPG110
>  	tristate "TPO TPG 800x400 panel"
>  	depends on OF && SPI && GPIOLIB
> diff --git a/drivers/gpu/drm/panel/panel-tpo-td043mtea1.c b/drivers/gpu/drm/panel/panel-tpo-td043mtea1.c
> new file mode 100644
> index 000000000000..6b17e47582b8
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-tpo-td043mtea1.c
> @@ -0,0 +1,510 @@
> +// SPDX-License-Identifier: GPL-2.0+
Just noticed, this is a different license than the others.
But I guess this comes from the original file.

> +/*
> + * Toppology TD043MTEA1 Panel Driver
So I actually asked Google this time - the correct spelling is
"Toppoly".
See: http://www.innolux.com/Pages/EN/AboutUs/Company_Overview_EN.html

> +struct td043mtea1_device {
> +	struct drm_panel panel;
> +
> +	struct spi_device *spi;
> +	struct regulator *vcc_reg;
> +	struct gpio_desc *reset_gpio;
> +
> +	unsigned int mode;
> +	u16 gamma[12];
> +	bool vmirror;
> +	bool powered_on;
This flag will not be needed when prepare(), unprepare() are used.

> +	bool spi_suspended;
> +	bool power_on_resume;
> +};
> +
> +

> +static void td043mtea1_write_gamma(struct td043mtea1_device *lcd)
> +{
> +	const u16 *gamma = lcd->gamma;
> +	unsigned int i;
> +	u8 val;
> +
> +	/* gamma bits [9:8] */
> +	for (val = i = 0; i < 4; i++)
> +		val |= (gamma[i] & 0x300) >> ((i + 1) * 2);
> +	td043mtea1_write(lcd, 0x11, val);
> +
> +	for (val = i = 0; i < 4; i++)
> +		val |= (gamma[i+4] & 0x300) >> ((i + 1) * 2);
Spaces around operators.

> +	td043mtea1_write(lcd, 0x12, val);
> +
> +	for (val = i = 0; i < 4; i++)
> +		val |= (gamma[i+8] & 0x300) >> ((i + 1) * 2);
Same here.

> +	td043mtea1_write(lcd, 0x13, val);
> +
> +	/* gamma bits [7:0] */
> +	for (val = i = 0; i < 12; i++)
> +		td043mtea1_write(lcd, 0x14 + i, gamma[i] & 0xff);
> +}
This function (td043mtea1_write_gamma()) fails to check the result of
the write operations. Maybe on purpose. But looks strange we do it in
some places but not all.

> +
> +static int td043mtea1_write_mirror(struct td043mtea1_device *lcd)
> +{
> +	u8 reg4 = TPO_R04_NFLIP_H | TPO_R04_NFLIP_V |
> +		TPO_R04_CP_CLK_FREQ_1H | TPO_R04_VGL_FREQ_1H;
> +	if (lcd->vmirror)
> +		reg4 &= ~TPO_R04_NFLIP_V;
> +
> +	return td043mtea1_write(lcd, 4, reg4);
> +}
Add a:
#define TPO_R4	4
And then use it here?
Looks bad that the register number is hardcoded.

Same goes for several other calls to the write() function.

> +
> +static int td043mtea1_power_on(struct td043mtea1_device *lcd)
> +{
> +	int ret;
> +
> +	if (lcd->powered_on)
> +		return 0;
> +
> +	ret = regulator_enable(lcd->vcc_reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Wait for the panel to stabilize. */
> +	msleep(160);
> +
> +	gpiod_set_value(lcd->reset_gpio, 0);
> +
> +	td043mtea1_write(lcd, 2, TPO_R02_MODE(lcd->mode) | TPO_R02_NCLK_RISING);
> +	td043mtea1_write(lcd, 3, TPO_R03_VAL_NORMAL);
> +	td043mtea1_write(lcd, 0x20, 0xf0);
> +	td043mtea1_write(lcd, 0x21, 0xf0);
> +	td043mtea1_write_mirror(lcd);
> +	td043mtea1_write_gamma(lcd);
> +
> +	lcd->powered_on = true;
> +
> +	return 0;
> +}
The above should be part of prepare()

> +
> +static void td043mtea1_power_off(struct td043mtea1_device *lcd)
> +{
> +	if (!lcd->powered_on)
> +		return;
> +
> +	td043mtea1_write(lcd, 3, TPO_R03_VAL_STANDBY | TPO_R03_EN_PWM);
> +
> +	gpiod_set_value(lcd->reset_gpio, 1);
> +
> +	/* wait for at least 2 vsyncs before cutting off power */
> +	msleep(50);
> +
> +	td043mtea1_write(lcd, 3, TPO_R03_VAL_STANDBY);
> +
> +	regulator_disable(lcd->vcc_reg);
> +
> +	lcd->powered_on = false;
> +}
The above should be part of unprepare()

> +
> +/* -----------------------------------------------------------------------------
> + * sysfs
> + */
> +
> +static ssize_t vmirror_show(struct device *dev, struct device_attribute *attr,
> +			    char *buf)
> +{
> +	struct td043mtea1_device *lcd = dev_get_drvdata(dev);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", lcd->vmirror);
> +}
> +
> +static ssize_t vmirror_store(struct device *dev, struct device_attribute *attr,
> +			     const char *buf, size_t count)
> +{
> +	struct td043mtea1_device *lcd = dev_get_drvdata(dev);
> +	int val;
> +	int ret;
> +
> +	ret = kstrtoint(buf, 0, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	lcd->vmirror = !!val;
> +
> +	ret = td043mtea1_write_mirror(lcd);
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +
> +static ssize_t mode_show(struct device *dev, struct device_attribute *attr,
> +			 char *buf)
> +{
> +	struct td043mtea1_device *lcd = dev_get_drvdata(dev);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", lcd->mode);
> +}
> +
> +static ssize_t mode_store(struct device *dev, struct device_attribute *attr,
> +			  const char *buf, size_t count)
> +{
> +	struct td043mtea1_device *lcd = dev_get_drvdata(dev);
> +	long val;
> +	int ret;
> +
> +	ret = kstrtol(buf, 0, &val);
> +	if (ret != 0 || val & ~7)
> +		return -EINVAL;
> +
> +	lcd->mode = val;
> +
> +	val |= TPO_R02_NCLK_RISING;
> +	td043mtea1_write(lcd, 2, val);
> +
> +	return count;
> +}
> +
> +static ssize_t gamma_show(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	struct td043mtea1_device *lcd = dev_get_drvdata(dev);
> +	ssize_t len = 0;
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(lcd->gamma); i++) {
> +		ret = snprintf(buf + len, PAGE_SIZE - len, "%u ",
> +				lcd->gamma[i]);
> +		if (ret < 0)
> +			return ret;
> +		len += ret;
> +	}
> +	buf[len - 1] = '\n';
> +
> +	return len;
> +}
> +
> +static ssize_t gamma_store(struct device *dev, struct device_attribute *attr,
> +			   const char *buf, size_t count)
> +{
> +	struct td043mtea1_device *lcd = dev_get_drvdata(dev);
> +	unsigned int g[12];
> +	unsigned int i;
> +	int ret;
> +
> +	ret = sscanf(buf, "%u %u %u %u %u %u %u %u %u %u %u %u",
> +			&g[0], &g[1], &g[2], &g[3], &g[4], &g[5],
> +			&g[6], &g[7], &g[8], &g[9], &g[10], &g[11]);
> +	if (ret != 12)
> +		return -EINVAL;
> +
> +	for (i = 0; i < 12; i++)
> +		lcd->gamma[i] = g[i];
> +
> +	td043mtea1_write_gamma(lcd);
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(vmirror);
> +static DEVICE_ATTR_RW(mode);
> +static DEVICE_ATTR_RW(gamma);
> +
> +static struct attribute *td043mtea1_attrs[] = {
> +	&dev_attr_vmirror.attr,
> +	&dev_attr_mode.attr,
> +	&dev_attr_gamma.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group td043mtea1_attr_group = {
> +	.attrs = td043mtea1_attrs,
> +};
I see what is done with mirror, mode and gamma - but the question is if
they are really needed?
And if needed, is it the right way to configure the panel?
This is likely questiosn that are not easy to answer definitive, so best
to keep this as it was before.

> +
> +/* -----------------------------------------------------------------------------
> + * Bridge Operations
> + */

Panel operations, not bridge operations?

> +
> +static int td043mtea1_disable(struct drm_panel *panel)
> +{
> +	struct td043mtea1_device *lcd = to_td043mtea1_device(panel);
> +
> +	if (!lcd->spi_suspended)
> +		td043mtea1_power_off(lcd);
> +
> +	return 0;
> +}
> +
> +static int td043mtea1_enable(struct drm_panel *panel)
> +{
> +	struct td043mtea1_device *lcd = to_td043mtea1_device(panel);
> +	int ret;
> +
> +	/*
> +	 * If we are resuming from system suspend, SPI might not be enabled
> +	 * yet, so we'll program the LCD from SPI PM resume callback.
> +	 */
> +	if (lcd->spi_suspended)
> +		return 0;
I do not recall this is needed in other panel drivers, so look at what
other spi based panels do here.
I think this is something that today is not required.

> +
> +	ret = td043mtea1_power_on(lcd);
> +	if (ret) {
> +		dev_err(&lcd->spi->dev, "%s: power on failed (%d)\n",
> +			__func__, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct drm_display_mode td043mtea1_mode = {
> +	.clock = 36000,
> +	.hdisplay = 800,
> +	.hsync_start = 800 + 68,
> +	.hsync_end = 800 + 68 + 1,
> +	.htotal = 800 + 68 + 1 + 214,
> +	.vdisplay = 480,
> +	.vsync_start = 480 + 39,
> +	.vsync_end = 480 + 39 + 1,
> +	.vtotal = 480 + 39 + 1 + 34,
> +	.vrefresh = 60,
> +	.type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
> +	.flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC,
> +};
height_mm, width_mm

> +
> +static int td043mtea1_get_modes(struct drm_panel *panel)
> +{
> +	struct drm_connector *connector = panel->connector;
> +	struct drm_display_mode *mode;
> +
> +	mode = drm_mode_duplicate(panel->drm, &td043mtea1_mode);
> +	if (!mode)
> +		return -ENOMEM;
> +
> +	drm_mode_set_name(mode);
> +	drm_mode_probed_add(connector, mode);
> +
> +	connector->display_info.width_mm = 94;
> +	connector->display_info.height_mm = 56;
> +	/*
> +	 * FIXME: According to the datasheet sync signals are sampled on the
> +	 * rising edge of the clock, but the code running on the OMAP3 Pandora
> +	 * indicates sampling on the falling edge. This should be tested on a
> +	 * real device.
> +	 */
> +	connector->display_info.bus_flags = DRM_BUS_FLAG_DE_HIGH
> +					  | DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE
> +					  | DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE;
> +
> +	return 1;
> +}
> +
> +static const struct drm_panel_funcs td043mtea1_funcs = {
> +	.disable = td043mtea1_disable,
> +	.enable = td043mtea1_enable,
> +	.get_modes = td043mtea1_get_modes,
> +};
> +
> +/* -----------------------------------------------------------------------------
> + * Power Management, Probe and Remove
> + */
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int td043mtea1_suspend(struct device *dev)
> +{
> +	struct td043mtea1_device *lcd = dev_get_drvdata(dev);
> +
> +	if (lcd->powered_on) {
> +		td043mtea1_power_off(lcd);
> +		lcd->powered_on = true;
> +	}
> +
> +	lcd->spi_suspended = true;
> +
> +	return 0;
> +}
> +
> +static int td043mtea1_resume(struct device *dev)
> +{
> +	struct td043mtea1_device *lcd = dev_get_drvdata(dev);
> +	int ret;
> +
> +	lcd->spi_suspended = false;
> +
> +	if (lcd->powered_on) {
> +		lcd->powered_on = false;
> +		ret = td043mtea1_power_on(lcd);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(td043mtea1_pm_ops, td043mtea1_suspend,
> +			 td043mtea1_resume);
> +#endif
> +
> +static int td043mtea1_probe(struct spi_device *spi)
> +{
> +	struct td043mtea1_device *lcd;
> +	int ret;
> +
> +	lcd = devm_kzalloc(&spi->dev, sizeof(*lcd), GFP_KERNEL);
> +	if (lcd == NULL)
> +		return -ENOMEM;
> +
> +	spi_set_drvdata(spi, lcd);
> +	lcd->spi = spi;
> +	lcd->mode = TPO_R02_MODE_800x480;
> +	memcpy(lcd->gamma, td043mtea1_def_gamma, sizeof(lcd->gamma));
> +
> +	lcd->vcc_reg = devm_regulator_get(&spi->dev, "vcc");
> +	if (IS_ERR(lcd->vcc_reg)) {
> +		dev_err(&spi->dev, "failed to get VCC regulator\n");
> +		return PTR_ERR(lcd->vcc_reg);
> +	}
> +
> +	lcd->reset_gpio = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(lcd->reset_gpio)) {
> +		dev_err(&spi->dev, "failed to get reset GPIO\n");
> +		return PTR_ERR(lcd->reset_gpio);
> +	}
> +
> +	spi->bits_per_word = 16;
> +	spi->mode = SPI_MODE_0;
> +
> +	ret = spi_setup(spi);
> +	if (ret < 0) {
> +		dev_err(&spi->dev, "failed to setup SPI: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = sysfs_create_group(&spi->dev.kobj, &td043mtea1_attr_group);
> +	if (ret < 0) {
> +		dev_err(&spi->dev, "failed to create sysfs files\n");
> +		return ret;
> +	}
> +
> +	drm_panel_init(&lcd->panel);
> +	lcd->panel.dev = &lcd->spi->dev;
> +	lcd->panel.funcs = &td043mtea1_funcs;
> +
> +	ret = drm_panel_add(&lcd->panel);
> +	if (ret < 0) {
> +		sysfs_remove_group(&spi->dev.kobj, &td043mtea1_attr_group);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int td043mtea1_remove(struct spi_device *spi)
> +{
> +	struct td043mtea1_device *lcd = spi_get_drvdata(spi);
> +
> +	drm_panel_remove(&lcd->panel);
> +	td043mtea1_disable(&lcd->panel);
> +
> +	sysfs_remove_group(&spi->dev.kobj, &td043mtea1_attr_group);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id td043mtea1_of_match[] = {
> +	{ .compatible = "tpo,td043mtea1", },
> +	{},
{ /* sentinel */ },

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