Re: [PATCH 3/3] drm/omap: displays: encoder-tpd12s015: Support for hot plug detection

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;