| Message ID | 20170515090312.32051-4-peter.ujfalusi@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Peter, Thank you for the patch. On Monday 15 May 2017 12:03:12 Peter Ujfalusi wrote: > Use interrupt handler for hpd GPIO to react to HPD changes. > > Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> > --- > .../gpu/drm/omapdrm/displays/encoder-tpd12s015.c | 81 +++++++++++++++++++ > 1 file changed, 81 insertions(+) > > diff --git a/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c > b/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c index > 58276a48112e..529b8509dd99 100644 > --- a/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c > +++ b/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c > @@ -15,12 +15,17 @@ > #include <linux/slab.h> > #include <linux/platform_device.h> > #include <linux/gpio/consumer.h> > +#include <linux/spinlock.h> Did you mean linux/mutex.h ? > > #include "../dss/omapdss.h" > > struct panel_drv_data { > struct omap_dss_device dssdev; > struct omap_dss_device *in; > + void (*hpd_cb)(void *cb_data, enum drm_connector_status status); > + void *hpd_cb_data; > + bool hpd_enabled; > + struct mutex hpd_lock; > > struct gpio_desc *ct_cp_hpd_gpio; > struct gpio_desc *ls_oe_gpio; > @@ -162,6 +167,49 @@ static bool tpd_detect(struct omap_dss_device *dssdev) > return gpiod_get_value_cansleep(ddata->hpd_gpio); > } > > +static int tpd_register_hpd_cb(struct omap_dss_device *dssdev, > + void (*cb)(void *cb_data, > + enum drm_connector_status status), > + void *cb_data) > +{ > + struct panel_drv_data *ddata = to_panel_data(dssdev); > + > + mutex_lock(&ddata->hpd_lock); > + ddata->hpd_cb = cb; > + ddata->hpd_cb_data = cb_data; > + mutex_unlock(&ddata->hpd_lock); > + > + return 0; > +} > + > +static void tpd_unregister_hpd_cb(struct omap_dss_device *dssdev) > +{ > + struct panel_drv_data *ddata = to_panel_data(dssdev); > + > + mutex_lock(&ddata->hpd_lock); > + ddata->hpd_cb = NULL; > + ddata->hpd_cb_data = NULL; > + mutex_unlock(&ddata->hpd_lock); > +} > + > +static void tpd_enable_hpd(struct omap_dss_device *dssdev) > +{ > + struct panel_drv_data *ddata = to_panel_data(dssdev); > + > + mutex_lock(&ddata->hpd_lock); > + ddata->hpd_enabled = true; > + mutex_unlock(&ddata->hpd_lock); > +} > + > +static void tpd_disable_hpd(struct omap_dss_device *dssdev) > +{ > + struct panel_drv_data *ddata = to_panel_data(dssdev); > + > + mutex_lock(&ddata->hpd_lock); > + ddata->hpd_enabled = false; > + mutex_unlock(&ddata->hpd_lock); > +} > + > static int tpd_set_infoframe(struct omap_dss_device *dssdev, > const struct hdmi_avi_infoframe *avi) > { > @@ -193,10 +241,34 @@ static const struct omapdss_hdmi_ops tpd_hdmi_ops = { > > .read_edid = tpd_read_edid, > .detect = tpd_detect, > + .register_hpd_cb = tpd_register_hpd_cb, > + .unregister_hpd_cb = tpd_unregister_hpd_cb, > + .enable_hpd = tpd_enable_hpd, > + .disable_hpd = tpd_disable_hpd, > .set_infoframe = tpd_set_infoframe, > .set_hdmi_mode = tpd_set_hdmi_mode, > }; > > +static irqreturn_t tpd_hpd_isr(int irq, void *data) > +{ > + struct panel_drv_data *ddata = data; > + > + mutex_lock(&ddata->hpd_lock); > + if (ddata->hpd_enabled && ddata->hpd_cb) { > + enum drm_connector_status status; > + > + if (tpd_detect(&ddata->dssdev)) > + status = connector_status_connected; > + else > + status = connector_status_disconnected; > + > + ddata->hpd_cb(ddata->hpd_cb_data, status); > + } > + mutex_unlock(&ddata->hpd_lock); > + > + return IRQ_HANDLED; > +} > + > static int tpd_probe_of(struct platform_device *pdev) > { > struct panel_drv_data *ddata = platform_get_drvdata(pdev); > @@ -261,6 +333,15 @@ static int tpd_probe(struct platform_device *pdev) > > ddata->hpd_gpio = gpio; > > + mutex_init(&ddata->hpd_lock); > + > + r = devm_request_threaded_irq(&pdev->dev, gpiod_to_irq(ddata- >hpd_gpio), As the connector code can handle GPIO HPD thanks to patch 2/3, why does it have to be duplicated here ? I agree that encoders should support reporting of hotplug events when the HPD signal is connected to an encoder, but if it's connected to a GPIO, it seems to me that it should be the sole responsibility of the connector code to handle it. > + NULL, tpd_hpd_isr, > + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > + "tpd12s015 hpd", ddata); > + if (r) > + goto err_gpio; > + > dssdev = &ddata->dssdev; > dssdev->ops.hdmi = &tpd_hdmi_ops; > dssdev->dev = &pdev->dev;
On 23/05/17 12:48, Laurent Pinchart wrote: > As the connector code can handle GPIO HPD thanks to patch 2/3, why does it > have to be duplicated here ? I agree that encoders should support reporting of > hotplug events when the HPD signal is connected to an encoder, but if it's > connected to a GPIO, it seems to me that it should be the sole responsibility > of the connector code to handle it. The HPD line goes from the connector to TPD12S015. From TPD12S015 another line goes to the SoCs GPIO. Tomi
Hi Tomi, On Tuesday 23 May 2017 13:25:34 Tomi Valkeinen wrote: > On 23/05/17 12:48, Laurent Pinchart wrote: > > As the connector code can handle GPIO HPD thanks to patch 2/3, why does it > > have to be duplicated here ? I agree that encoders should support > > reporting of hotplug events when the HPD signal is connected to an > > encoder, but if it's connected to a GPIO, it seems to me that it should > > be the sole responsibility of the connector code to handle it. > > The HPD line goes from the connector to TPD12S015. From TPD12S015 > another line goes to the SoCs GPIO. Isn't it the same signal, just with glitches filtered ? The TPD12S015 is an ESD clamp, level shifter and DC-DC converter, if it wasn't for the two control inputs CT_CP_HPD and LS_OE, we could probably do without a driver. I wouldn't add HPD support to this driver, as the chip really can't detect HPD.
On 23/05/17 13:42, Laurent Pinchart wrote: > Hi Tomi, > > On Tuesday 23 May 2017 13:25:34 Tomi Valkeinen wrote: >> On 23/05/17 12:48, Laurent Pinchart wrote: >>> As the connector code can handle GPIO HPD thanks to patch 2/3, why does it >>> have to be duplicated here ? I agree that encoders should support >>> reporting of hotplug events when the HPD signal is connected to an >>> encoder, but if it's connected to a GPIO, it seems to me that it should >>> be the sole responsibility of the connector code to handle it. >> >> The HPD line goes from the connector to TPD12S015. From TPD12S015 >> another line goes to the SoCs GPIO. > > Isn't it the same signal, just with glitches filtered ? The TPD12S015 is an > ESD clamp, level shifter and DC-DC converter, if it wasn't for the two control > inputs CT_CP_HPD and LS_OE, we could probably do without a driver. I wouldn't > add HPD support to this driver, as the chip really can't detect HPD. Yes, it is the same signal, level shifted and filtered, if I'm not mistaken. The HPD gpio is defined in TPD's DT data. And can we know what is the state of the HPD signal when TPD hasn't configured CT_CP_HPD and LS_OE. Will there be a glitch when CT_CP_HPD is enabled? What if connector-hdmi is already listening to HPD signal at that point? My guess is that having gpio handling just in the connector would work, especially as we set the CT_CP_HPD very early when TPD is being set up. With this series we could probably do a bit more optimized management for CT_CP_HPD, but probably we could still make sure the sequence is such that the CT_CP_HPD is enabled before connector-hdmi registers the irq. But that doesn't sound quite correct to me... I do think it's TPD driver's responsibility, and it removes all the "make sure that"'s. Tomi
diff --git a/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c b/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c index 58276a48112e..529b8509dd99 100644 --- a/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c +++ b/drivers/gpu/drm/omapdrm/displays/encoder-tpd12s015.c @@ -15,12 +15,17 @@ #include <linux/slab.h> #include <linux/platform_device.h> #include <linux/gpio/consumer.h> +#include <linux/spinlock.h> #include "../dss/omapdss.h" struct panel_drv_data { struct omap_dss_device dssdev; struct omap_dss_device *in; + void (*hpd_cb)(void *cb_data, enum drm_connector_status status); + void *hpd_cb_data; + bool hpd_enabled; + struct mutex hpd_lock; struct gpio_desc *ct_cp_hpd_gpio; struct gpio_desc *ls_oe_gpio; @@ -162,6 +167,49 @@ static bool tpd_detect(struct omap_dss_device *dssdev) return gpiod_get_value_cansleep(ddata->hpd_gpio); } +static int tpd_register_hpd_cb(struct omap_dss_device *dssdev, + void (*cb)(void *cb_data, + enum drm_connector_status status), + void *cb_data) +{ + struct panel_drv_data *ddata = to_panel_data(dssdev); + + mutex_lock(&ddata->hpd_lock); + ddata->hpd_cb = cb; + ddata->hpd_cb_data = cb_data; + mutex_unlock(&ddata->hpd_lock); + + return 0; +} + +static void tpd_unregister_hpd_cb(struct omap_dss_device *dssdev) +{ + struct panel_drv_data *ddata = to_panel_data(dssdev); + + mutex_lock(&ddata->hpd_lock); + ddata->hpd_cb = NULL; + ddata->hpd_cb_data = NULL; + mutex_unlock(&ddata->hpd_lock); +} + +static void tpd_enable_hpd(struct omap_dss_device *dssdev) +{ + struct panel_drv_data *ddata = to_panel_data(dssdev); + + mutex_lock(&ddata->hpd_lock); + ddata->hpd_enabled = true; + mutex_unlock(&ddata->hpd_lock); +} + +static void tpd_disable_hpd(struct omap_dss_device *dssdev) +{ + struct panel_drv_data *ddata = to_panel_data(dssdev); + + mutex_lock(&ddata->hpd_lock); + ddata->hpd_enabled = false; + mutex_unlock(&ddata->hpd_lock); +} + static int tpd_set_infoframe(struct omap_dss_device *dssdev, const struct hdmi_avi_infoframe *avi) { @@ -193,10 +241,34 @@ static const struct omapdss_hdmi_ops tpd_hdmi_ops = { .read_edid = tpd_read_edid, .detect = tpd_detect, + .register_hpd_cb = tpd_register_hpd_cb, + .unregister_hpd_cb = tpd_unregister_hpd_cb, + .enable_hpd = tpd_enable_hpd, + .disable_hpd = tpd_disable_hpd, .set_infoframe = tpd_set_infoframe, .set_hdmi_mode = tpd_set_hdmi_mode, }; +static irqreturn_t tpd_hpd_isr(int irq, void *data) +{ + struct panel_drv_data *ddata = data; + + mutex_lock(&ddata->hpd_lock); + if (ddata->hpd_enabled && ddata->hpd_cb) { + enum drm_connector_status status; + + if (tpd_detect(&ddata->dssdev)) + status = connector_status_connected; + else + status = connector_status_disconnected; + + ddata->hpd_cb(ddata->hpd_cb_data, status); + } + mutex_unlock(&ddata->hpd_lock); + + return IRQ_HANDLED; +} + static int tpd_probe_of(struct platform_device *pdev) { struct panel_drv_data *ddata = platform_get_drvdata(pdev); @@ -261,6 +333,15 @@ static int tpd_probe(struct platform_device *pdev) ddata->hpd_gpio = gpio; + mutex_init(&ddata->hpd_lock); + + r = devm_request_threaded_irq(&pdev->dev, gpiod_to_irq(ddata->hpd_gpio), + NULL, tpd_hpd_isr, + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT, + "tpd12s015 hpd", ddata); + if (r) + goto err_gpio; + dssdev = &ddata->dssdev; dssdev->ops.hdmi = &tpd_hdmi_ops; dssdev->dev = &pdev->dev;
Use interrupt handler for hpd GPIO to react to HPD changes. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com> --- .../gpu/drm/omapdrm/displays/encoder-tpd12s015.c | 81 ++++++++++++++++++++++ 1 file changed, 81 insertions(+)