Re: [PATCH 03/24] drm/omap: add HPD support to connector-dvi

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Tomi,

Thank you for the patch.

On Monday, 12 February 2018 11:44:33 EET Tomi Valkeinen wrote:
> Add HPD support to the DVI connector driver. The code is almost
> identical to the HPD code in the HDMI connector driver.

Would it make sense to share a single implementation then ? Or is that an 
exercise left for the reader (that is, me) ? :-)

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/displays/connector-dvi.c | 114 ++++++++++++++++++++
>  1 file changed, 114 insertions(+)
> 
> diff --git a/drivers/gpu/drm/omapdrm/displays/connector-dvi.c
> b/drivers/gpu/drm/omapdrm/displays/connector-dvi.c index
> 391d80364346..f9f8700223c3 100644
> --- a/drivers/gpu/drm/omapdrm/displays/connector-dvi.c
> +++ b/drivers/gpu/drm/omapdrm/displays/connector-dvi.c
> @@ -13,6 +13,7 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
> +#include <linux/gpio/consumer.h>

Could you please keep headers alphabetically sorted ?

>  #include <drm/drm_edid.h>
> 
> @@ -44,6 +45,13 @@ struct panel_drv_data {
>  	struct videomode vm;
> 
>  	struct i2c_adapter *i2c_adapter;
> +
> +	struct gpio_desc *hpd_gpio;
> +
> +	void (*hpd_cb)(void *cb_data, enum drm_connector_status status);
> +	void *hpd_cb_data;
> +	bool hpd_enabled;
> +	struct mutex hpd_lock;

Locks should have a comment that describes which fields they protect.

>  };
> 
>  #define to_panel_data(x) container_of(x, struct panel_drv_data, dssdev)
> @@ -189,6 +197,9 @@ static int dvic_read_edid(struct omap_dss_device
> *dssdev, struct panel_drv_data *ddata = to_panel_data(dssdev);
>  	int r, l, bytes_read;
> 
> +	if (ddata->hpd_gpio && !gpiod_get_value_cansleep(ddata->hpd_gpio))
> +		return -ENODEV;
> +
>  	if (!ddata->i2c_adapter)
>  		return -ENODEV;
> 
> @@ -220,6 +231,9 @@ static bool dvic_detect(struct omap_dss_device *dssdev)
>  	unsigned char out;
>  	int r;
> 
> +	if (ddata->hpd_gpio)
> +		return gpiod_get_value_cansleep(ddata->hpd_gpio);
> +
>  	if (!ddata->i2c_adapter)
>  		return true;
> 
> @@ -228,6 +242,60 @@ static bool dvic_detect(struct omap_dss_device *dssdev)
> return r == 0;
>  }
> 
> +static int dvic_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);
> +
> +	if (!ddata->hpd_gpio)
> +		return -ENOTSUPP;
> +
> +	mutex_lock(&ddata->hpd_lock);
> +	ddata->hpd_cb = cb;
> +	ddata->hpd_cb_data = cb_data;
> +	mutex_unlock(&ddata->hpd_lock);
> +	return 0;
> +}
> +
> +static void dvic_unregister_hpd_cb(struct omap_dss_device *dssdev)
> +{
> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
> +
> +	if (!ddata->hpd_gpio)
> +		return;
> +
> +	mutex_lock(&ddata->hpd_lock);
> +	ddata->hpd_cb = NULL;
> +	ddata->hpd_cb_data = NULL;
> +	mutex_unlock(&ddata->hpd_lock);
> +}
> +
> +static void dvic_enable_hpd(struct omap_dss_device *dssdev)
> +{
> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
> +
> +	if (!ddata->hpd_gpio)
> +		return;
> +
> +	mutex_lock(&ddata->hpd_lock);
> +	ddata->hpd_enabled = true;
> +	mutex_unlock(&ddata->hpd_lock);
> +}
> +
> +static void dvic_disable_hpd(struct omap_dss_device *dssdev)
> +{
> +	struct panel_drv_data *ddata = to_panel_data(dssdev);
> +
> +	if (!ddata->hpd_gpio)
> +		return;
> +
> +	mutex_lock(&ddata->hpd_lock);
> +	ddata->hpd_enabled = false;
> +	mutex_unlock(&ddata->hpd_lock);
> +}
> +
>  static struct omap_dss_driver dvic_driver = {
>  	.connect	= dvic_connect,
>  	.disconnect	= dvic_disconnect,
> @@ -241,14 +309,60 @@ static struct omap_dss_driver dvic_driver = {
> 
>  	.read_edid	= dvic_read_edid,
>  	.detect		= dvic_detect,
> +
> +	.register_hpd_cb	= dvic_register_hpd_cb,
> +	.unregister_hpd_cb	= dvic_unregister_hpd_cb,
> +	.enable_hpd		= dvic_enable_hpd,
> +	.disable_hpd		= dvic_disable_hpd,
>  };
> 
> +static irqreturn_t dvic_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 (dvic_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 dvic_probe_of(struct platform_device *pdev)
>  {
>  	struct panel_drv_data *ddata = platform_get_drvdata(pdev);
>  	struct device_node *node = pdev->dev.of_node;
>  	struct device_node *adapter_node;
>  	struct i2c_adapter *adapter;
> +	struct gpio_desc *gpio;
> +	int r;
> +
> +	gpio = devm_gpiod_get_optional(&pdev->dev, "hpd", GPIOD_IN);
> +	if (IS_ERR(gpio)) {
> +		dev_err(&pdev->dev, "failed to parse HPD gpio\n");
> +		return PTR_ERR(gpio);
> +	}
> +
> +	ddata->hpd_gpio = gpio;
> +
> +	mutex_init(&ddata->hpd_lock);

Shouldn't you also have a mutex_destroy ?

> +	if (ddata->hpd_gpio) {
> +		r = devm_request_threaded_irq(&pdev->dev,
> +			gpiod_to_irq(ddata->hpd_gpio), NULL, dvic_hpd_isr,
> +			IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +			"DVI HPD", ddata);
> +		if (r)
> +			return r;
> +	}
> 
>  	adapter_node = of_parse_phandle(node, "ddc-i2c-bus", 0);
>  	if (adapter_node) {

-- 
Regards,

Laurent Pinchart

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