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.valkei...@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
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
