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

On 27/02/18 14:58, Laurent Pinchart wrote:

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) ? :-)

It would, but I suspect all these will go away soon with the drm_bridge/panel work, so I didn't want to start doing a bigger refactoring.

...

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 ?

Sure.

...

#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.

Added.

...

};

#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 ?

Yep. Interestingly, we don't destroy any of the mutexes in omapdss/drm. Luckily mutex_destroy doesn't seem to do much (just for debug purposes), but still, we need to add those at some point.

Tomi