Re: [PATCH 11/60] drm/bridge: Add driver for the TI TPD12S015 HDMI level shifter

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Maxime Ripard <maxime.ripard@bootlin.com>,
	Sebastian Reichel <sebastian.reichel@collabora.com>,
	dri-devel@lists.freedesktop.org, Sean Paul <sean@poorly.run>
Subject: Re: [PATCH 11/60] drm/bridge: Add driver for the TI TPD12S015 HDMI level shifter
Date: Tue, 27 Aug 2019 10:30:16 +0300	[thread overview]
Message-ID: <20190827073016.GB5054@pendragon.ideasonboard.com> (raw)
In-Reply-To: <3347b6c8-6f2d-17d6-3dc8-e62a3bac634b@ti.com>

Hi Tomi,

On Tue, Aug 27, 2019 at 09:36:00AM +0300, Tomi Valkeinen wrote:
> On 07/07/2019 21:18, Laurent Pinchart wrote:
> > The TI TPD12S015 is an HDMI level shifter and ESD protector controlled
> > through GPIOs. Add a DRM bridge driver for the device.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >   drivers/gpu/drm/bridge/Kconfig        |   8 +
> >   drivers/gpu/drm/bridge/Makefile       |   1 +
> >   drivers/gpu/drm/bridge/ti-tpd12s015.c | 204 ++++++++++++++++++++++++++
> >   3 files changed, 213 insertions(+)
> >   create mode 100644 drivers/gpu/drm/bridge/ti-tpd12s015.c
> > 
> > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > index 295a62f65ef9..3928651a0819 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -159,6 +159,14 @@ config DRM_TI_SN65DSI86
> >   	help
> >   	  Texas Instruments SN65DSI86 DSI to eDP Bridge driver
> >   
> > +config DRM_TI_TPD12S015
> > +	tristate "TI TPD12S015 HDMI level shifter and ESD protection"
> > +	depends on OF
> > +	select DRM_KMS_HELPER
> > +	help
> > +	  Texas Instruments TPD12S015 HDMI level shifter and ESD protection
> > +	  driver.
> > +
> >   source "drivers/gpu/drm/bridge/analogix/Kconfig"
> >   
> >   source "drivers/gpu/drm/bridge/adv7511/Kconfig"
> > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> > index e5987b3aaf62..ce635651e31b 100644
> > --- a/drivers/gpu/drm/bridge/Makefile
> > +++ b/drivers/gpu/drm/bridge/Makefile
> > @@ -17,4 +17,5 @@ obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
> >   obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
> >   obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o
> >   obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
> > +obj-$(CONFIG_DRM_TI_TPD12S015) += ti-tpd12s015.o
> >   obj-y += synopsys/
> > diff --git a/drivers/gpu/drm/bridge/ti-tpd12s015.c b/drivers/gpu/drm/bridge/ti-tpd12s015.c
> > new file mode 100644
> > index 000000000000..d01f0c4133a2
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/ti-tpd12s015.c
> > @@ -0,0 +1,204 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * TPD12S015 HDMI ESD protection & level shifter chip driver
> > + *
> > + * Copyright (C) 2013 Texas Instruments Incorporated
> > + * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > + */
> 
> You may want to mention that the driver is based on an existing omapdrm 
> driver. Otherwise the above lines don't quite make sense when adding a 
> new driver =).

Indeed. I'll fix that.

> > +
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/of.h>
> > +#include <linux/of_graph.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <drm/drm_bridge.h>
> > +
> > +struct tpd12s015_device {
> > +	struct drm_bridge bridge;
> > +
> > +	struct gpio_desc *ct_cp_hpd_gpio;
> > +	struct gpio_desc *ls_oe_gpio;
> > +	struct gpio_desc *hpd_gpio;
> > +	int hpd_irq;
> > +
> > +	struct drm_bridge *next_bridge;
> > +};
> > +
> > +static inline struct tpd12s015_device *to_tpd12s015(struct drm_bridge *bridge)
> > +{
> > +	return container_of(bridge, struct tpd12s015_device, bridge);
> > +}
> > +
> > +static int tpd12s015_attach(struct drm_bridge *bridge, bool create_connector)
> > +{
> > +	struct tpd12s015_device *tpd = to_tpd12s015(bridge);
> > +	int ret;
> > +
> > +	if (create_connector)
> > +		return -EINVAL;
> > +
> > +	ret = drm_bridge_attach(bridge->encoder, tpd->next_bridge,
> > +				bridge, false);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	gpiod_set_value_cansleep(tpd->ct_cp_hpd_gpio, 1);
> > +	gpiod_set_value_cansleep(tpd->ls_oe_gpio, 1);
> 
> These are fine (and as they were in the old driver), but just talking 
> out loud: LS_OE is needed when talking over i2c,

I2C or CEC, right ? For I2C this would require modelling the chip as an
I2C gate, which will have an impact on DT bindings. CEC would likely
need a similar treatment. If someone really wants to save power I think
that would be possible, but I won't do so as part of this series.

> and CT_CP_HPD is needed when interested in HPD.
> 
> Not sure what kind of power-savings are possible, but at least in theory 
> we could turn those off at times. At least when going to system suspend.
> 
> > +	/* DC-DC converter needs at max 300us to get to 90% of 5V. */
> > +	usleep_range(300, 1000);
> > +
> > +	return 0;
> > +}
> > +
> > +static void tpd12s015_detach(struct drm_bridge *bridge)
> > +{
> > +	struct tpd12s015_device *tpd = to_tpd12s015(bridge);
> > +
> > +	gpiod_set_value_cansleep(tpd->ct_cp_hpd_gpio, 0);
> > +	gpiod_set_value_cansleep(tpd->ls_oe_gpio, 0);
> > +}
> > +
> > +static enum drm_connector_status tpd12s015_detect(struct drm_bridge *bridge)
> > +{
> > +	struct tpd12s015_device *tpd = to_tpd12s015(bridge);
> > +
> > +	if (gpiod_get_value_cansleep(tpd->hpd_gpio))
> > +		return connector_status_connected;
> > +	else
> > +		return connector_status_disconnected;
> > +}
> > +
> > +static void tpd12s015_hpd_enable(struct drm_bridge *bridge)
> > +{
> > +}
> > +
> > +static void tpd12s015_hpd_disable(struct drm_bridge *bridge)
> > +{
> > +}
> 
> Shouldn't we manage the CT_HPD_GPIO here, and request the irq here? I'm 
> fine with leaving that for later, though, as it may be better to keep 
> this new driver as similar to the old one as possible to prevent 
> regressions during this big patch series.

That's a good point. It's fairly easy to do, so I'll give it a try
already.

> I guess it's fine for a bridge to do hpd_notify even if hpd_enable has 
> not been called?

Yes, the HPD events are blocked by the core in that case.

> > +static const struct drm_bridge_funcs tpd12s015_bridge_funcs = {
> > +	.attach			= tpd12s015_attach,
> > +	.detach			= tpd12s015_detach,
> > +	.detect			= tpd12s015_detect,
> > +	.hpd_enable		= tpd12s015_hpd_enable,
> > +	.hpd_disable		= tpd12s015_hpd_disable,
> > +};
> > +
> > +static irqreturn_t tpd12s015_hpd_isr(int irq, void *data)
> > +{
> > +	struct tpd12s015_device *tpd = data;
> > +	struct drm_bridge *bridge = &tpd->bridge;
> > +
> > +	drm_bridge_hpd_notify(bridge, tpd12s015_detect(bridge));
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int tpd12s015_probe(struct platform_device *pdev)
> > +{
> > +	struct tpd12s015_device *tpd;
> > +	struct device_node *node;
> > +	struct gpio_desc *gpio;
> > +	int ret;
> > +
> > +	tpd = devm_kzalloc(&pdev->dev, sizeof(*tpd), GFP_KERNEL);
> > +	if (!tpd)
> > +		return -ENOMEM;
> > +
> > +	platform_set_drvdata(pdev, tpd);
> > +
> > +	tpd->bridge.funcs = &tpd12s015_bridge_funcs;
> > +	tpd->bridge.of_node = pdev->dev.of_node;
> > +	tpd->bridge.type = DRM_MODE_CONNECTOR_HDMIA;
> > +	tpd->bridge.ops = DRM_BRIDGE_OP_DETECT;
> > +
> > +	/* Get the next bridge, connected to port@1. */
> > +	node = of_graph_get_remote_node(pdev->dev.of_node, 1, -1);
> > +	if (!node)
> > +		return -ENODEV;
> > +
> > +	tpd->next_bridge = of_drm_find_bridge(node);
> > +	of_node_put(node);
> > +
> > +	if (!tpd->next_bridge)
> > +		return -EPROBE_DEFER;
> > +
> > +	/* Get the control and HPD GPIOs. */
> > +	gpio = devm_gpiod_get_index_optional(&pdev->dev, NULL, 0,
> > +					     GPIOD_OUT_LOW);
> > +	if (IS_ERR(gpio))
> > +		return PTR_ERR(gpio);
> > +
> > +	tpd->ct_cp_hpd_gpio = gpio;
> > +
> > +	gpio = devm_gpiod_get_index_optional(&pdev->dev, NULL, 1,
> > +					     GPIOD_OUT_LOW);
> > +	if (IS_ERR(gpio))
> > +		return PTR_ERR(gpio);
> > +
> > +	tpd->ls_oe_gpio = gpio;
> > +
> > +	gpio = devm_gpiod_get_index(&pdev->dev, NULL, 2, GPIOD_IN);
> > +	if (IS_ERR(gpio))
> > +		return PTR_ERR(gpio);
> > +
> > +	tpd->hpd_gpio = gpio;
> > +
> > +	/* Register the IRQ if the HPD GPIO is IRQ-capable. */
> > +	if (tpd->hpd_gpio)
> > +		tpd->hpd_irq = gpiod_to_irq(tpd->hpd_gpio);
> 
> We always have tpd->hpd_gpio here, don't we?

Good point, I'll fix that.

-- 
Regards,

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