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@xxxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxx>
> > + */
>
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
