Patchwork [v4,3/6] usb: ohci-da8xx: disable the regulator if the overcurrent irq fired

login
register
mail settings
Submitter Bartosz Golaszewski
Date April 11, 2019, 9:30 a.m.
Message ID <20190411093024.23555-4-brgl@bgdev.pl>
Download mbox | patch
Permalink /patch/770369/
State New
Headers show

Comments

Bartosz Golaszewski - April 11, 2019, 9:30 a.m.
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Historically the power supply management in this driver has been handled
in two separate places in parallel. Device-tree users simply defined an
appropriate regulator, while two boards with no DT support (da830-evm and
omapl138-hawk) passed functions defined in their respective board files
over platform data. These functions simply used legacy GPIO calls to
watch the oc GPIO for interrupts and disable the vbus GPIO when the irq
fires.

Commit d193abf1c913 ("usb: ohci-da8xx: add vbus and overcurrent gpios")
updated these GPIO calls to the modern API and moved them inside the
driver.

This however is not the optimal solution for the vbus GPIO which should
be modeled as a fixed regulator that can be controlled with a GPIO.

In order to keep the overcurrent protection available once we move the
board files to using fixed regulators we need to disable the enable_reg
regulator when the overcurrent indicator interrupt fires. Since we
cannot call regulator_disable() from interrupt context, we need to
switch to using a oneshot threaded interrupt.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/usb/host/ohci-da8xx.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)
Sekhar Nori - April 12, 2019, 10:39 a.m.
Hi Bartosz,

On 11/04/19 3:00 PM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Historically the power supply management in this driver has been handled
> in two separate places in parallel. Device-tree users simply defined an
> appropriate regulator, while two boards with no DT support (da830-evm and
> omapl138-hawk) passed functions defined in their respective board files
> over platform data. These functions simply used legacy GPIO calls to
> watch the oc GPIO for interrupts and disable the vbus GPIO when the irq
> fires.
> 
> Commit d193abf1c913 ("usb: ohci-da8xx: add vbus and overcurrent gpios")
> updated these GPIO calls to the modern API and moved them inside the
> driver.
> 
> This however is not the optimal solution for the vbus GPIO which should
> be modeled as a fixed regulator that can be controlled with a GPIO.
> 
> In order to keep the overcurrent protection available once we move the
> board files to using fixed regulators we need to disable the enable_reg
> regulator when the overcurrent indicator interrupt fires. Since we
> cannot call regulator_disable() from interrupt context, we need to
> switch to using a oneshot threaded interrupt.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

I thought a bit on whether it makes sense to merge this patch into 6/6
since that is modifying some code you introduce here. But I think its
probably better to keep it separate like you have it now.

6/6 is about dropping support for vbus gpio, where as here you are
letting regulator be used for handing overcurrent detected using oci gpio.

Having it separate makes it possible to revert dropping support for vbus
gpio while keeping this functionality ;)

> -static irqreturn_t ohci_da8xx_oc_handler(int irq, void *data)
> +static irqreturn_t ohci_da8xx_oc_thread(int irq, void *data)
>  {
>  	struct da8xx_ohci_hcd *da8xx_ohci = data;
> +	struct device *dev = da8xx_ohci->hcd->self.controller;
> +	int ret;
>  
> -	if (gpiod_get_value(da8xx_ohci->oc_gpio))
> -		gpiod_set_value(da8xx_ohci->vbus_gpio, 0);
> +	if (gpiod_get_value_cansleep(da8xx_ohci->oc_gpio)) {
> +		if (da8xx_ohci->vbus_gpio) {
> +			gpiod_set_value_cansleep(da8xx_ohci->vbus_gpio, 0);
> +		} else if (da8xx_ohci->vbus_reg) {
> +			ret = regulator_disable(da8xx_ohci->vbus_reg);
> +			if (ret)
> +				dev_err(dev,
> +					"Failed to disable regulator: %d\n",
> +					ret);
> +		}
> +			

Unnecessary empty line here, that too one that introduces white space
error. Please drop.

Thanks,
Sekhar
Bartosz Golaszewski - April 12, 2019, 12:12 p.m.
pt., 12 kwi 2019 o 12:39 Sekhar Nori <nsekhar@ti.com> napisaƂ(a):
>
> Hi Bartosz,
>
> On 11/04/19 3:00 PM, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > Historically the power supply management in this driver has been handled
> > in two separate places in parallel. Device-tree users simply defined an
> > appropriate regulator, while two boards with no DT support (da830-evm and
> > omapl138-hawk) passed functions defined in their respective board files
> > over platform data. These functions simply used legacy GPIO calls to
> > watch the oc GPIO for interrupts and disable the vbus GPIO when the irq
> > fires.
> >
> > Commit d193abf1c913 ("usb: ohci-da8xx: add vbus and overcurrent gpios")
> > updated these GPIO calls to the modern API and moved them inside the
> > driver.
> >
> > This however is not the optimal solution for the vbus GPIO which should
> > be modeled as a fixed regulator that can be controlled with a GPIO.
> >
> > In order to keep the overcurrent protection available once we move the
> > board files to using fixed regulators we need to disable the enable_reg
> > regulator when the overcurrent indicator interrupt fires. Since we
> > cannot call regulator_disable() from interrupt context, we need to
> > switch to using a oneshot threaded interrupt.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> I thought a bit on whether it makes sense to merge this patch into 6/6
> since that is modifying some code you introduce here. But I think its
> probably better to keep it separate like you have it now.
>
> 6/6 is about dropping support for vbus gpio, where as here you are
> letting regulator be used for handing overcurrent detected using oci gpio.
>
> Having it separate makes it possible to revert dropping support for vbus
> gpio while keeping this functionality ;)
>

These two changes were together in v1, but I too think that having it
separate keeps the series bisectable.

Bart

> > -static irqreturn_t ohci_da8xx_oc_handler(int irq, void *data)
> > +static irqreturn_t ohci_da8xx_oc_thread(int irq, void *data)
> >  {
> >       struct da8xx_ohci_hcd *da8xx_ohci = data;
> > +     struct device *dev = da8xx_ohci->hcd->self.controller;
> > +     int ret;
> >
> > -     if (gpiod_get_value(da8xx_ohci->oc_gpio))
> > -             gpiod_set_value(da8xx_ohci->vbus_gpio, 0);
> > +     if (gpiod_get_value_cansleep(da8xx_ohci->oc_gpio)) {
> > +             if (da8xx_ohci->vbus_gpio) {
> > +                     gpiod_set_value_cansleep(da8xx_ohci->vbus_gpio, 0);
> > +             } else if (da8xx_ohci->vbus_reg) {
> > +                     ret = regulator_disable(da8xx_ohci->vbus_reg);
> > +                     if (ret)
> > +                             dev_err(dev,
> > +                                     "Failed to disable regulator: %d\n",
> > +                                     ret);
> > +             }
> > +
>
> Unnecessary empty line here, that too one that introduces white space
> error. Please drop.
>
> Thanks,
> Sekhar

Patch

diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
index 209a262b5565..6158408b1ea0 100644
--- a/drivers/usb/host/ohci-da8xx.c
+++ b/drivers/usb/host/ohci-da8xx.c
@@ -202,12 +202,24 @@  static int ohci_da8xx_regulator_event(struct notifier_block *nb,
 	return 0;
 }
 
-static irqreturn_t ohci_da8xx_oc_handler(int irq, void *data)
+static irqreturn_t ohci_da8xx_oc_thread(int irq, void *data)
 {
 	struct da8xx_ohci_hcd *da8xx_ohci = data;
+	struct device *dev = da8xx_ohci->hcd->self.controller;
+	int ret;
 
-	if (gpiod_get_value(da8xx_ohci->oc_gpio))
-		gpiod_set_value(da8xx_ohci->vbus_gpio, 0);
+	if (gpiod_get_value_cansleep(da8xx_ohci->oc_gpio)) {
+		if (da8xx_ohci->vbus_gpio) {
+			gpiod_set_value_cansleep(da8xx_ohci->vbus_gpio, 0);
+		} else if (da8xx_ohci->vbus_reg) {
+			ret = regulator_disable(da8xx_ohci->vbus_reg);
+			if (ret)
+				dev_err(dev,
+					"Failed to disable regulator: %d\n",
+					ret);
+		}
+			
+	}
 
 	return IRQ_HANDLED;
 }
@@ -434,8 +446,9 @@  static int ohci_da8xx_probe(struct platform_device *pdev)
 		if (oc_irq < 0)
 			goto err;
 
-		error = devm_request_irq(dev, oc_irq, ohci_da8xx_oc_handler,
-				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+		error = devm_request_threaded_irq(dev, oc_irq, NULL,
+				ohci_da8xx_oc_thread, IRQF_TRIGGER_RISING |
+				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 				"OHCI over-current indicator", da8xx_ohci);
 		if (error)
 			goto err;