From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S937325AbdEXJOD (ORCPT ); Wed, 24 May 2017 05:14:03 -0400 Received: from fllnx210.ext.ti.com ([198.47.19.17]:55365 "EHLO fllnx210.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759130AbdEXJN5 (ORCPT ); Wed, 24 May 2017 05:13:57 -0400 Subject: Re: [PATCH 2/3] drm/omap: displays: connector-hdmi: Support for hot plug detection To: Laurent Pinchart References: <20170515090312.32051-1-peter.ujfalusi@ti.com> <20170515090312.32051-3-peter.ujfalusi@ti.com> <3265236.TtRcDxutng@avalon> CC: , , , , From: Peter Ujfalusi Message-ID: <8a447d5c-b7ed-9d1a-9865-27d0acebdb2c@ti.com> Date: Wed, 24 May 2017 12:14:07 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <3265236.TtRcDxutng@avalon> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> --- >> 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 >> #include >> #include >> +#include > > 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