| Message ID | 1518428694-18018-4-git-send-email-tomi.valkeinen@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/omap: misc patches | expand |
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) {
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
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> #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; }; #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); + + 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) {
Add HPD support to the DVI connector driver. The code is almost identical to the HPD code in the HDMI connector driver. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/omapdrm/displays/connector-dvi.c | 114 +++++++++++++++++++++++ 1 file changed, 114 insertions(+)