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

Groups > linux.kernel > #1641420 > unrolled thread

[PATCH 2/3] drm/omap: displays: connector-hdmi: Support for hot plug detection

Started byPeter Ujfalusi <peter.ujfalusi@ti.com>
First post2017-05-15 11:10 +0200
Last post2017-05-24 11:20 +0200
Articles 3 — 2 participants

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

  [PATCH 2/3] drm/omap: displays: connector-hdmi: Support for hot plug detection Peter Ujfalusi <peter.ujfalusi@ti.com> - 2017-05-15 11:10 +0200
    Re: [PATCH 2/3] drm/omap: displays: connector-hdmi: Support for hot plug detection Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-05-23 11:50 +0200
      Re: [PATCH 2/3] drm/omap: displays: connector-hdmi: Support for hot  plug detection Peter Ujfalusi <peter.ujfalusi@ti.com> - 2017-05-24 11:20 +0200

#1641420 — [PATCH 2/3] drm/omap: displays: connector-hdmi: Support for hot plug detection

FromPeter Ujfalusi <peter.ujfalusi@ti.com>
Date2017-05-15 11:10 +0200
Subject[PATCH 2/3] drm/omap: displays: connector-hdmi: Support for hot plug detection
Message-ID<tHkaT-67d-25@gated-at.bofh.it>
If the hpd_gpio is valid, use interrupt handler to react to HPD changes.
In case the hpd_gpio is not valid, try to enable hpd detection on the
encoder if it supports it.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/gpu/drm/omapdrm/displays/connector-hdmi.c | 104 ++++++++++++++++++++++
 1 file changed, 104 insertions(+)

diff --git a/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c b/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c
index 1ef130641bae..3a90f89ada77 100644
--- a/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c
+++ b/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c
@@ -15,6 +15,7 @@
 #include <linux/platform_device.h>
 #include <linux/of.h>
 #include <linux/of_gpio.h>
+#include <linux/spinlock.h>
 
 #include <drm/drm_edid.h>
 #include <video/omap-panel-data.h>
@@ -38,6 +39,10 @@ static const struct videomode hdmic_default_vm = {
 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 device *dev;
 
@@ -168,6 +173,70 @@ static bool hdmic_detect(struct omap_dss_device *dssdev)
 		return in->ops.hdmi->detect(in);
 }
 
+static int hdmic_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);
+	struct omap_dss_device *in = ddata->in;
+
+	if (gpio_is_valid(ddata->hpd_gpio)) {
+		mutex_lock(&ddata->hpd_lock);
+		ddata->hpd_cb = cb;
+		ddata->hpd_cb_data = cb_data;
+		mutex_unlock(&ddata->hpd_lock);
+		return 0;
+	} else if (in->ops.hdmi->register_hpd_cb) {
+		return in->ops.hdmi->register_hpd_cb(in, cb, cb_data);
+	}
+
+	return -ENOTSUPP;
+}
+
+static void hdmic_unregister_hpd_cb(struct omap_dss_device *dssdev)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+	struct omap_dss_device *in = ddata->in;
+
+	if (gpio_is_valid(ddata->hpd_gpio)) {
+		mutex_lock(&ddata->hpd_lock);
+		ddata->hpd_cb = NULL;
+		ddata->hpd_cb_data = NULL;
+		mutex_unlock(&ddata->hpd_lock);
+	} else if (in->ops.hdmi->unregister_hpd_cb) {
+		in->ops.hdmi->unregister_hpd_cb(in);
+	}
+}
+
+static void hdmic_enable_hpd(struct omap_dss_device *dssdev)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+	struct omap_dss_device *in = ddata->in;
+
+	if (gpio_is_valid(ddata->hpd_gpio)) {
+		mutex_lock(&ddata->hpd_lock);
+		ddata->hpd_enabled = true;
+		mutex_unlock(&ddata->hpd_lock);
+	} else if (in->ops.hdmi->enable_hpd) {
+		in->ops.hdmi->enable_hpd(in);
+	}
+}
+
+static void hdmic_disable_hpd(struct omap_dss_device *dssdev)
+{
+	struct panel_drv_data *ddata = to_panel_data(dssdev);
+	struct omap_dss_device *in = ddata->in;
+
+	if (gpio_is_valid(ddata->hpd_gpio)) {
+		mutex_lock(&ddata->hpd_lock);
+		ddata->hpd_enabled = false;
+		mutex_unlock(&ddata->hpd_lock);
+	} else if (in->ops.hdmi->disable_hpd) {
+		in->ops.hdmi->disable_hpd(in);
+	}
+}
+
 static int hdmic_set_hdmi_mode(struct omap_dss_device *dssdev, bool hdmi_mode)
 {
 	struct panel_drv_data *ddata = to_panel_data(dssdev);
@@ -200,10 +269,34 @@ static struct omap_dss_driver hdmic_driver = {
 
 	.read_edid		= hdmic_read_edid,
 	.detect			= hdmic_detect,
+	.register_hpd_cb	= hdmic_register_hpd_cb,
+	.unregister_hpd_cb	= hdmic_unregister_hpd_cb,
+	.enable_hpd		= hdmic_enable_hpd,
+	.disable_hpd		= hdmic_disable_hpd,
 	.set_hdmi_mode		= hdmic_set_hdmi_mode,
 	.set_hdmi_infoframe	= hdmic_set_infoframe,
 };
 
+static irqreturn_t hdmic_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 (hdmic_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 hdmic_probe_of(struct platform_device *pdev)
 {
 	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
@@ -249,11 +342,22 @@ static int hdmic_probe(struct platform_device *pdev)
 	if (r)
 		return r;
 
+	mutex_init(&ddata->hpd_lock);
+
 	if (gpio_is_valid(ddata->hpd_gpio)) {
 		r = devm_gpio_request_one(&pdev->dev, ddata->hpd_gpio,
 				GPIOF_DIR_IN, "hdmi_hpd");
 		if (r)
 			goto err_reg;
+
+		r = devm_request_threaded_irq(&pdev->dev,
+				gpio_to_irq(ddata->hpd_gpio),
+				NULL, hdmic_hpd_isr,
+				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |
+				IRQF_ONESHOT,
+				"hdmic hpd", ddata);
+		if (r)
+			goto err_reg;
 	}
 
 	ddata->vm = hdmic_default_vm;
-- 
2.12.2

[toc] | [next] | [standalone]

#1647902

FromLaurent Pinchart <laurent.pinchart@ideasonboard.com>
Date2017-05-23 11:50 +0200
Message-ID<tKeBY-1li-23@gated-at.bofh.it>
In reply to#1641420
Hi Peter,

Thank you for the patch.

On Monday 15 May 2017 12:03:11 Peter Ujfalusi wrote:
> If the hpd_gpio is valid, use interrupt handler to react to HPD changes.
> In case the hpd_gpio is not valid, try to enable hpd detection on the
> encoder if it supports it.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/displays/connector-hdmi.c | 104 +++++++++++++++++++
>  1 file changed, 104 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c
> b/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c index
> 1ef130641bae..3a90f89ada77 100644
> --- a/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c
> +++ b/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c
> @@ -15,6 +15,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/of.h>
>  #include <linux/of_gpio.h>
> +#include <linux/spinlock.h>

Did you mean linux/mutex.h ?

> 
>  #include <drm/drm_edid.h>
>  #include <video/omap-panel-data.h>
> @@ -38,6 +39,10 @@ static const struct videomode hdmic_default_vm = {
>  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 device *dev;
> 
> @@ -168,6 +173,70 @@ static bool hdmic_detect(struct omap_dss_device
> *dssdev) return in->ops.hdmi->detect(in);
>  }
> 
> +static int hdmic_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);
> +	struct omap_dss_device *in = ddata->in;
> +
> +	if (gpio_is_valid(ddata->hpd_gpio)) {
> +		mutex_lock(&ddata->hpd_lock);
> +		ddata->hpd_cb = cb;
> +		ddata->hpd_cb_data = cb_data;
> +		mutex_unlock(&ddata->hpd_lock);
> +		return 0;
> +	} else if (in->ops.hdmi->register_hpd_cb) {
> +		return in->ops.hdmi->register_hpd_cb(in, cb, cb_data);
> +	}
> +
> +	return -ENOTSUPP;
> +}
> +
> +static void hdmic_unregister_hpd_cb(struct omap_dss_device *dssdev)
> +{
> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
> +	struct omap_dss_device *in = ddata->in;
> +
> +	if (gpio_is_valid(ddata->hpd_gpio)) {
> +		mutex_lock(&ddata->hpd_lock);
> +		ddata->hpd_cb = NULL;
> +		ddata->hpd_cb_data = NULL;
> +		mutex_unlock(&ddata->hpd_lock);
> +	} else if (in->ops.hdmi->unregister_hpd_cb) {
> +		in->ops.hdmi->unregister_hpd_cb(in);
> +	}
> +}
> +
> +static void hdmic_enable_hpd(struct omap_dss_device *dssdev)
> +{
> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
> +	struct omap_dss_device *in = ddata->in;
> +
> +	if (gpio_is_valid(ddata->hpd_gpio)) {
> +		mutex_lock(&ddata->hpd_lock);
> +		ddata->hpd_enabled = true;
> +		mutex_unlock(&ddata->hpd_lock);
> +	} else if (in->ops.hdmi->enable_hpd) {
> +		in->ops.hdmi->enable_hpd(in);
> +	}
> +}
> +
> +static void hdmic_disable_hpd(struct omap_dss_device *dssdev)
> +{
> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
> +	struct omap_dss_device *in = ddata->in;
> +
> +	if (gpio_is_valid(ddata->hpd_gpio)) {
> +		mutex_lock(&ddata->hpd_lock);
> +		ddata->hpd_enabled = false;
> +		mutex_unlock(&ddata->hpd_lock);
> +	} else if (in->ops.hdmi->disable_hpd) {
> +		in->ops.hdmi->disable_hpd(in);
> +	}
> +}
> +
>  static int hdmic_set_hdmi_mode(struct omap_dss_device *dssdev, bool
> hdmi_mode) {
>  	struct panel_drv_data *ddata = to_panel_data(dssdev);
> @@ -200,10 +269,34 @@ static struct omap_dss_driver hdmic_driver = {
> 
>  	.read_edid		= hdmic_read_edid,
>  	.detect			= hdmic_detect,
> +	.register_hpd_cb	= hdmic_register_hpd_cb,
> +	.unregister_hpd_cb	= hdmic_unregister_hpd_cb,
> +	.enable_hpd		= hdmic_enable_hpd,
> +	.disable_hpd		= hdmic_disable_hpd,
>  	.set_hdmi_mode		= hdmic_set_hdmi_mode,
>  	.set_hdmi_infoframe	= hdmic_set_infoframe,
>  };
> 
> +static irqreturn_t hdmic_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 (hdmic_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);

Shouldn't ddata->hpd_cb() be called without the mutex held ? I don't think 
core code should rely on callers to handle locking in this case.


> +	return IRQ_HANDLED;
> +}
> +
>  static int hdmic_probe_of(struct platform_device *pdev)
>  {
>  	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
> @@ -249,11 +342,22 @@ static int hdmic_probe(struct platform_device *pdev)
>  	if (r)
>  		return r;
> 
> +	mutex_init(&ddata->hpd_lock);
> +
>  	if (gpio_is_valid(ddata->hpd_gpio)) {
>  		r = devm_gpio_request_one(&pdev->dev, ddata->hpd_gpio,
>  				GPIOF_DIR_IN, "hdmi_hpd");
>  		if (r)
>  			goto err_reg;
> +
> +		r = devm_request_threaded_irq(&pdev->dev,
> +				gpio_to_irq(ddata->hpd_gpio),

What is hpd_gpio is valid but has no IRQ support ?

> +				NULL, hdmic_hpd_isr,
> +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |
> +				IRQF_ONESHOT,
> +				"hdmic hpd", ddata);
> +		if (r)
> +			goto err_reg;
>  	}
> 
>  	ddata->vm = hdmic_default_vm;

-- 
Regards,

Laurent Pinchart

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

#1649365 — Re: [PATCH 2/3] drm/omap: displays: connector-hdmi: Support for hot plug detection

FromPeter Ujfalusi <peter.ujfalusi@ti.com>
Date2017-05-24 11:20 +0200
SubjectRe: [PATCH 2/3] drm/omap: displays: connector-hdmi: Support for hot plug detection
Message-ID<tKACt-pV-17@gated-at.bofh.it>
In reply to#1647902
On 2017-05-23 12:45, Laurent Pinchart wrote:
> Hi Peter,
> 
> Thank you for the patch.
> 
> On Monday 15 May 2017 12:03:11 Peter Ujfalusi wrote:
>> If the hpd_gpio is valid, use interrupt handler to react to HPD changes.
>> In case the hpd_gpio is not valid, try to enable hpd detection on the
>> encoder if it supports it.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
>>  drivers/gpu/drm/omapdrm/displays/connector-hdmi.c | 104 +++++++++++++++++++
>>  1 file changed, 104 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c
>> b/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c index
>> 1ef130641bae..3a90f89ada77 100644
>> --- a/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c
>> +++ b/drivers/gpu/drm/omapdrm/displays/connector-hdmi.c
>> @@ -15,6 +15,7 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/of.h>
>>  #include <linux/of_gpio.h>
>> +#include <linux/spinlock.h>
> 
> Did you mean linux/mutex.h ?

Oops, yes I mean linux/mutex.h.

>> +static irqreturn_t hdmic_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 (hdmic_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);
> 
> Shouldn't ddata->hpd_cb() be called without the mutex held ? I don't think 
> core code should rely on callers to handle locking in this case.

the mutex is for protecting the internal data of the driver (hpd_cb
mainly). W/o keeping the mutex there might be a chance that we are going
to get an unregister call (unlikely, but in theory it can happen) which
would NULL out the hpd_cb, if we save the hpd_cb and data while holding
the mux, it is possible that the  omap DRM driver was removed already
causing other type of crash.

> 
> 
>> +	return IRQ_HANDLED;
>> +}
>> +
>>  static int hdmic_probe_of(struct platform_device *pdev)
>>  {
>>  	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
>> @@ -249,11 +342,22 @@ static int hdmic_probe(struct platform_device *pdev)
>>  	if (r)
>>  		return r;
>>
>> +	mutex_init(&ddata->hpd_lock);
>> +
>>  	if (gpio_is_valid(ddata->hpd_gpio)) {
>>  		r = devm_gpio_request_one(&pdev->dev, ddata->hpd_gpio,
>>  				GPIOF_DIR_IN, "hdmi_hpd");
>>  		if (r)
>>  			goto err_reg;
>> +
>> +		r = devm_request_threaded_irq(&pdev->dev,
>> +				gpio_to_irq(ddata->hpd_gpio),
> 
> What is hpd_gpio is valid but has no IRQ support ?
> 
>> +				NULL, hdmic_hpd_isr,
>> +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING |
>> +				IRQF_ONESHOT,
>> +				"hdmic hpd", ddata);
>> +		if (r)
>> +			goto err_reg;
>>  	}
>>
>>  	ddata->vm = hdmic_default_vm;
> 

- Péter

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web