Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1571092 > unrolled thread

Re: [PATCH 09/11] drm/panel: Add support for the Raspberry Pi 7" Touchscreen.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 09/11] drm/panel: Add support for the Raspberry Pi 7"  Touchscreen. Thierry Reding <thierry.reding@gmail.com> - 2017-01-31 22:10 +0100
    Re: [PATCH 09/11] drm/panel: Add support for the Raspberry Pi 7"  Touchscreen. Daniel Vetter <daniel@ffwll.ch> - 2017-01-31 22:30 +0100
      Re: [PATCH 09/11] drm/panel: Add support for the Raspberry Pi 7"  Touchscreen. Thierry Reding <thierry.reding@gmail.com> - 2017-01-31 22:40 +0100
    Re: [PATCH 09/11] drm/panel: Add support for the Raspberry Pi 7"  Touchscreen. Daniel Vetter <daniel@ffwll.ch> - 2017-01-31 22:30 +0100
      Re: [PATCH 09/11] drm/panel: Add support for the Raspberry Pi 7"  Touchscreen. Thierry Reding <thierry.reding@gmail.com> - 2017-01-31 23:20 +0100

#1571092 — Re: [PATCH 09/11] drm/panel: Add support for the Raspberry Pi 7" Touchscreen.

[Multipart message — attachments visible in raw view] — view raw

On Wed, Dec 14, 2016 at 11:46:19AM -0800, Eric Anholt wrote:
[...]
> diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
[...]
> +/**
> + * DOC: Raspberry Pi 7" touchscreen panel driver.
> + *
> + * The 7" touchscreen consists of a DPI LCD panel, a Toshiba
> + * TC358762XBG DSI-DPI bridge, and an I2C-connected Atmel ATTINY88-MUR
> + * controlling power management, the LCD PWM, and the touchscreen.
> + *
> + * This driver presents this device as a MIPI DSI panel to the DRM
> + * driver, and should expose the touchscreen as a HID device.
> + */

This sounds like these should be multiple drivers rather than wrapping
it all in a single one.

It might not be worth enforcing this now, provided that at least the
device tree is done properly, which would allow the driver structure
to change later on if, for example, a different panel needs to be
supported.

> +struct rpi_touchscreen {
> +	struct drm_panel base;
> +	struct mipi_dsi_device *dsi;
> +	struct i2c_client *bridge_i2c;
> +
> +	/* Version of the firmware on the bridge chip */
> +	int atmel_ver;

I don't see this used other than to store a version number. There's no
code in the driver that's conditional on this version.

> +static int rpi_touchscreen_enable(struct drm_panel *panel)
> +{
> +	struct rpi_touchscreen *ts = panel_to_ts(panel);
> +	int i;
> +
> +	rpi_touchscreen_i2c_write(ts, REG_POWERON, 1);
> +	/* Wait for nPWRDWN to go low to indicate poweron is done. */
> +	for (i = 0; i < 100; i++) {
> +		if (rpi_touchscreen_i2c_read(ts, REG_PORTB) & 1)
> +			break;
> +	}

Don't you want to fail when power on doesn't succeed? Seems kind of
pointless to continue if the panel doesn't power on.

> +
> +	rpi_touchscreen_write(ts, DSI_LANEENABLE,
> +			      DSI_LANEENABLE_CLOCK |
> +			      DSI_LANEENABLE_D0 |
> +			      (ts->dsi->lanes > 1 ? DSI_LANEENABLE_D1 : 0));

ts->dsi->lanes is set to 1 in ->probe(), so effectively this is dead
code, but I guess it can't hurt to leave it in in case you ever want to
extend this to support other display panels.

Which makes me think even more that this should really be at least two
drivers: a bridge driver and a panel driver, with the bridge getting
parameters from the panel to program registers accordingly.

> +	rpi_touchscreen_write(ts, PPI_D0S_CLRSIPOCOUNT, 0x05);
> +	rpi_touchscreen_write(ts, PPI_D1S_CLRSIPOCOUNT, 0x05);
> +	rpi_touchscreen_write(ts, PPI_D0S_ATMR, 0x00);
> +	rpi_touchscreen_write(ts, PPI_D1S_ATMR, 0x00);
> +	rpi_touchscreen_write(ts, PPI_LPTXTIMECNT, 0x03);
> +
> +	rpi_touchscreen_write(ts, SPICMR, 0x00);
> +	rpi_touchscreen_write(ts, LCDCTRL, 0x00100150);
> +	rpi_touchscreen_write(ts, SYSCTRL, 0x040f);
> +	msleep(100);
> +
> +	rpi_touchscreen_write(ts, PPI_STARTPPI, 0x01);
> +	rpi_touchscreen_write(ts, DSI_STARTDSI, 0x01);
> +	msleep(100);
> +
> +	/* Turn on the backklight. */
> +	rpi_touchscreen_i2c_write(ts, REG_PWM, 255);

It might be worth implementing a backlight here so that you can control
it from userspace like you would any other backlight.

> +static int rpi_touchscreen_dsi_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct device *dev = &dsi->dev;
> +	struct rpi_touchscreen *ts;
> +	int ret, ver;
> +
> +	ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
> +	if (!ts)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(dev, ts);
> +
> +	ts->dsi = dsi;
> +	dsi->mode_flags = (MIPI_DSI_MODE_VIDEO |
> +			   MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> +			   MIPI_DSI_MODE_LPM);
> +	dsi->format = MIPI_DSI_FMT_RGB888;
> +	dsi->lanes = 1;
> +
> +	ts->bridge_i2c =
> +		rpi_touchscreen_get_i2c(dev, "raspberrypi,touchscreen-bridge");
> +	if (!ts->bridge_i2c) {
> +		ret = -EPROBE_DEFER;
> +		return ret;
> +	}
> +
> +	ver = rpi_touchscreen_i2c_read(ts, REG_ID);
> +	if (ver < 0) {
> +		dev_err(dev, "Atmel I2C read failed: %d\n", ver);
> +		return -ENODEV;
> +	}

Should this not goto err_release_bridge?

> +
> +	switch (ver) {
> +	case 0xde:
> +		ts->atmel_ver = 1;
> +		break;
> +	case 0xc3:
> +		ts->atmel_ver = 2;
> +		break;
> +	default:
> +		dev_err(dev, "Unknown Atmel firmware revision: 0x%02x\n", ver);
> +		return -ENODEV;
> +	}

Same here.

> +
> +	/* Turn off at boot, so we can cleanly sequence powering on. */
> +	rpi_touchscreen_i2c_write(ts, REG_POWERON, 0);
> +
> +	drm_panel_init(&ts->base);
> +	ts->base.dev = dev;
> +	ts->base.funcs = &rpi_touchscreen_funcs;
> +
> +	ret = drm_panel_add(&ts->base);
> +	if (ret < 0)
> +		goto err_release_bridge;
> +
> +	return mipi_dsi_attach(dsi);
> +
> +err_release_bridge:
> +	put_device(&ts->bridge_i2c->dev);
> +	return ret;
> +}
> +
> +static int rpi_touchscreen_dsi_remove(struct mipi_dsi_device *dsi)
> +{
> +	struct device *dev = &dsi->dev;
> +	struct rpi_touchscreen *ts = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = mipi_dsi_detach(dsi);
> +	if (ret < 0) {
> +		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret);
> +		return ret;
> +	}

You might want to continue after this anyway, because the driver will be
unloaded regardless of your error code and you'll leave behind a
dangling panel and leak a reference to the I2C bridge.

Thierry

[toc] | [next] | [standalone]

#1571109

On Tue, Jan 31, 2017 at 10:07:19PM +0100, Thierry Reding wrote:
> On Wed, Dec 14, 2016 at 11:46:19AM -0800, Eric Anholt wrote:
> > +static int rpi_touchscreen_dsi_remove(struct mipi_dsi_device *dsi)
> > +{
> > +	struct device *dev = &dsi->dev;
> > +	struct rpi_touchscreen *ts = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = mipi_dsi_detach(dsi);
> > +	if (ret < 0) {
> > +		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret);
> > +		return ret;
> > +	}
> 
> You might want to continue after this anyway, because the driver will be
> unloaded regardless of your error code and you'll leave behind a
> dangling panel and leak a reference to the I2C bridge.

Sounds like we should switch the mipi_dsi_driver->remove callback to
return void then? But separate cleanup series if someone bothers with it.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1571150

[Multipart message — attachments visible in raw view] — view raw

On Tue, Jan 31, 2017 at 10:19:52PM +0100, Daniel Vetter wrote:
> On Tue, Jan 31, 2017 at 10:07:19PM +0100, Thierry Reding wrote:
> > On Wed, Dec 14, 2016 at 11:46:19AM -0800, Eric Anholt wrote:
> > > +static int rpi_touchscreen_dsi_remove(struct mipi_dsi_device *dsi)
> > > +{
> > > +	struct device *dev = &dsi->dev;
> > > +	struct rpi_touchscreen *ts = dev_get_drvdata(dev);
> > > +	int ret;
> > > +
> > > +	ret = mipi_dsi_detach(dsi);
> > > +	if (ret < 0) {
> > > +		dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret);
> > > +		return ret;
> > > +	}
> > 
> > You might want to continue after this anyway, because the driver will be
> > unloaded regardless of your error code and you'll leave behind a
> > dangling panel and leak a reference to the I2C bridge.
> 
> Sounds like we should switch the mipi_dsi_driver->remove callback to
> return void then? But separate cleanup series if someone bothers with it.

I think there are advantages to keeping this consistent with the driver
core's definition of ->remove(). There have been efforts lately to deny
unloading drivers if they are a dependency for other drivers, so we may
yet see the day where the driver core actually does something with this
return value.

Thierry

[toc] | [prev] | [next] | [standalone]

#1571120

On Tue, Jan 31, 2017 at 10:07:19PM +0100, Thierry Reding wrote:
> On Wed, Dec 14, 2016 at 11:46:19AM -0800, Eric Anholt wrote:
> > +static int rpi_touchscreen_enable(struct drm_panel *panel)
> > +{
> > +	struct rpi_touchscreen *ts = panel_to_ts(panel);
> > +	int i;
> > +
> > +	rpi_touchscreen_i2c_write(ts, REG_POWERON, 1);
> > +	/* Wait for nPWRDWN to go low to indicate poweron is done. */
> > +	for (i = 0; i < 100; i++) {
> > +		if (rpi_touchscreen_i2c_read(ts, REG_PORTB) & 1)
> > +			break;
> > +	}
> 
> Don't you want to fail when power on doesn't succeed? Seems kind of
> pointless to continue if the panel doesn't power on.

kms works under the assumption that even when the sink is dead, the
display pipe (well, vblanks and pageflips) keep working. There's a patch
floating around to give userspace more information about what's going
wrong through an async uevent+read-only property for cases where an
unresponsive sink is normal, i.e. link training for dp.

But either way, continuing is generally the right thing to do, there's no
way to report -EIO from here (because no reasons than that's where
accidentally ended up with our evolved design ...).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1571169

[Multipart message — attachments visible in raw view] — view raw

On Tue, Jan 31, 2017 at 10:17:02PM +0100, Daniel Vetter wrote:
> On Tue, Jan 31, 2017 at 10:07:19PM +0100, Thierry Reding wrote:
> > On Wed, Dec 14, 2016 at 11:46:19AM -0800, Eric Anholt wrote:
> > > +static int rpi_touchscreen_enable(struct drm_panel *panel)
> > > +{
> > > +	struct rpi_touchscreen *ts = panel_to_ts(panel);
> > > +	int i;
> > > +
> > > +	rpi_touchscreen_i2c_write(ts, REG_POWERON, 1);
> > > +	/* Wait for nPWRDWN to go low to indicate poweron is done. */
> > > +	for (i = 0; i < 100; i++) {
> > > +		if (rpi_touchscreen_i2c_read(ts, REG_PORTB) & 1)
> > > +			break;
> > > +	}
> > 
> > Don't you want to fail when power on doesn't succeed? Seems kind of
> > pointless to continue if the panel doesn't power on.
> 
> kms works under the assumption that even when the sink is dead, the
> display pipe (well, vblanks and pageflips) keep working. There's a patch
> floating around to give userspace more information about what's going
> wrong through an async uevent+read-only property for cases where an
> unresponsive sink is normal, i.e. link training for dp.
> 
> But either way, continuing is generally the right thing to do, there's no
> way to report -EIO from here (because no reasons than that's where
> accidentally ended up with our evolved design ...).

I think this depends on the specific case. I was assuming that if the
panel fails to power up, then any subsequent operations like register
reads or writes would also fail, potentially causing a lot of confusing
error messages that could easily be avoided.

Also, the panel API is usually called from encoder or connector drivers
and propagating error codes might give them a chance of reacting.

Thierry

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web