[PATCH 0/3] ARM: davinci: ohci-da8xx: model the vbus GPIO as a fixed regulator

[PATCH 0/3] ARM: davinci: ohci-da8xx: model the vbus GPIO as a fixed regulator

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Adding the vbus GPIO support to the ohci-da8xx driver isn't really the
optimal solution. Rather: it should be modeled as a fixed regulator
in which case the driver already has support.

This series adds necessary fixups to the board files and removes the
vbus GPIO from the ohci driver.

Bartosz Golaszewski (3):
  ARM: davinci: omapl138-hawk: add a fixed regulator for ohci-da8xx
  ARM: davinci: da830-evm: add a fixed regulator for ohci-da8xx
  usb: ohci-da8xx: drop the vbus GPIO

 arch/arm/mach-davinci/board-da830-evm.c     | 48 +++++++++++++++++++--
 arch/arm/mach-davinci/board-omapl138-hawk.c | 48 +++++++++++++++++++--
 drivers/usb/host/ohci-da8xx.c               | 39 ++++++++---------
 3 files changed, 108 insertions(+), 27 deletions(-)

-- 
2.20.1

[PATCH 1/3] ARM: davinci: omapl138-hawk: add a fixed regulator for ohci-da8xx

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Instead of directly using the vbus GPIO we should model it as a fixed
regulator. Add all necessary fix-ups for the regulator to be registered
and configure the vbus GPIO as its enable pin.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/mach-davinci/board-omapl138-hawk.c | 48 +++++++++++++++++++--
 1 file changed, 45 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-davinci/board-omapl138-hawk.c b/arch/arm/mach-davinci/board-omapl138-hawk.c
index 0896af2bed24..ea7110e47e8a 100644
--- a/arch/arm/mach-davinci/board-omapl138-hawk.c
+++ b/arch/arm/mach-davinci/board-omapl138-hawk.c
@@ -21,6 +21,7 @@
 #include <linux/platform_data/mtd-davinci.h>
 #include <linux/platform_data/mtd-davinci-aemif.h>
 #include <linux/platform_data/ti-aemif.h>
+#include <linux/regulator/fixed.h>
 #include <linux/regulator/machine.h>
 
 #include <asm/mach-types.h>
@@ -298,14 +299,48 @@ static const short da850_hawk_usb11_pins[] = {
 	-1
 };
 
-static struct gpiod_lookup_table hawk_usb_gpio_lookup = {
+static struct regulator_consumer_supply hawk_usb_supplies[] = {
+	REGULATOR_SUPPLY("vbus", NULL),
+};
+
+static struct regulator_init_data hawk_usb_vbus_data = {
+	.consumer_supplies	= hawk_usb_supplies,
+	.num_consumer_supplies	= ARRAY_SIZE(hawk_usb_supplies),
+};
+
+static struct fixed_voltage_config hawk_usb_vbus = {
+	.supply_name		= "vbus",
+	.microvolts		= 3300000,
+	.init_data		= &hawk_usb_vbus_data,
+};
+
+static struct platform_device hawk_usb_vbus_device = {
+	.name		= "reg-fixed-voltage",
+	.id		= 1,
+	.dev		= {
+		.platform_data = &hawk_usb_vbus,
+	},
+};
+
+static struct gpiod_lookup_table hawk_usb_oc_gpio_lookup = {
 	.dev_id		= "ohci-da8xx",
 	.table = {
-		GPIO_LOOKUP("davinci_gpio", DA850_USB1_VBUS_PIN, "vbus", 0),
 		GPIO_LOOKUP("davinci_gpio", DA850_USB1_OC_PIN, "oc", 0),
 	},
 };
 
+static struct gpiod_lookup_table hawk_usb_vbus_gpio_lookup = {
+	.dev_id		= "reg-fixed-voltage.1",
+	.table = {
+		GPIO_LOOKUP("davinci_gpio", DA850_USB1_VBUS_PIN, NULL, 0),
+	},
+};
+
+static struct gpiod_lookup_table *hawk_usb_gpio_lookups[] = {
+	&hawk_usb_oc_gpio_lookup,
+	&hawk_usb_vbus_gpio_lookup,
+};
+
 static struct da8xx_ohci_root_hub omapl138_hawk_usb11_pdata = {
 	/* TPS2087 switch @ 5V */
 	.potpgt         = (3 + 1) / 2,  /* 3 ms max */
@@ -326,12 +361,19 @@ static __init void omapl138_hawk_usb_init(void)
 		pr_warn("%s: USB PHY CLK registration failed: %d\n",
 			__func__, ret);
 
+	gpiod_add_lookup_tables(hawk_usb_gpio_lookups,
+				ARRAY_SIZE(hawk_usb_gpio_lookups));
+
 	ret = da8xx_register_usb_phy();
 	if (ret)
 		pr_warn("%s: USB PHY registration failed: %d\n",
 			__func__, ret);
 
-	gpiod_add_lookup_table(&hawk_usb_gpio_lookup);
+	ret = platform_device_register(&hawk_usb_vbus_device);
+	if (ret) {
+		pr_warn("%s: Unable to register the vbus supply\n", __func__);
+		return;
+	}
 
 	ret = da8xx_register_usb11(&omapl138_hawk_usb11_pdata);
 	if (ret)
-- 
2.20.1

[PATCH 2/3] ARM: davinci: da830-evm: add a fixed regulator for ohci-da8xx

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Instead of directly using the vbus GPIO we should model it as a fixed
regulator. Add all necessary fix-ups for the regulator to be registered
and configure the vbus GPIO as its enable pin.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm/mach-davinci/board-da830-evm.c | 48 +++++++++++++++++++++++--
 1 file changed, 45 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-davinci/board-da830-evm.c b/arch/arm/mach-davinci/board-da830-evm.c
index ff097ecfa451..2ae1eff54345 100644
--- a/arch/arm/mach-davinci/board-da830-evm.c
+++ b/arch/arm/mach-davinci/board-da830-evm.c
@@ -29,6 +29,7 @@
 #include <linux/platform_data/spi-davinci.h>
 #include <linux/platform_data/usb-davinci.h>
 #include <linux/platform_data/ti-aemif.h>
+#include <linux/regulator/fixed.h>
 #include <linux/regulator/machine.h>
 #include <linux/nvmem-provider.h>
 
@@ -53,14 +54,48 @@ static const short da830_evm_usb11_pins[] = {
 	-1
 };
 
-static struct gpiod_lookup_table da830_evm_usb_gpio_lookup = {
+static struct regulator_consumer_supply da830_evm_usb_supplies[] = {
+	REGULATOR_SUPPLY("vbus", NULL),
+};
+
+static struct regulator_init_data da830_evm_usb_vbus_data = {
+	.consumer_supplies	= da830_evm_usb_supplies,
+	.num_consumer_supplies	= ARRAY_SIZE(da830_evm_usb_supplies),
+};
+
+static struct fixed_voltage_config da830_evm_usb_vbus = {
+	.supply_name		= "vbus",
+	.microvolts		= 33000000,
+	.init_data		= &da830_evm_usb_vbus_data,
+};
+
+static struct platform_device da830_evm_usb_vbus_device = {
+	.name		= "reg-fixed-voltage",
+	.id		= 1,
+	.dev		= {
+		.platform_data = &da830_evm_usb_vbus,
+	},
+};
+
+static struct gpiod_lookup_table da830_evm_usb_oc_gpio_lookup = {
 	.dev_id		= "ohci-da8xx",
 	.table = {
-		GPIO_LOOKUP("davinci_gpio", ON_BD_USB_DRV, "vbus", 0),
 		GPIO_LOOKUP("davinci_gpio", ON_BD_USB_OVC, "oc", 0),
 	},
 };
 
+static struct gpiod_lookup_table da830_evm_usb_vbus_gpio_lookup = {
+	.dev_id		= "reg-fixed-voltage.1",
+	.table = {
+		GPIO_LOOKUP("davinci_gpio", ON_BD_USB_DRV, "vbus", 0),
+	},
+};
+
+static struct gpiod_lookup_table *da830_evm_usb_gpio_lookups[] = {
+	&da830_evm_usb_oc_gpio_lookup,
+	&da830_evm_usb_vbus_gpio_lookup,
+};
+
 static struct da8xx_ohci_root_hub da830_evm_usb11_pdata = {
 	/* TPS2065 switch @ 5V */
 	.potpgt		= (3 + 1) / 2,	/* 3 ms max */
@@ -75,6 +110,9 @@ static __init void da830_evm_usb_init(void)
 		pr_warn("%s: USB PHY CLK registration failed: %d\n",
 			__func__, ret);
 
+	gpiod_add_lookup_tables(da830_evm_usb_gpio_lookups,
+				ARRAY_SIZE(da830_evm_usb_gpio_lookups));
+
 	ret = da8xx_register_usb_phy();
 	if (ret)
 		pr_warn("%s: USB PHY registration failed: %d\n",
@@ -100,7 +138,11 @@ static __init void da830_evm_usb_init(void)
 		return;
 	}
 
-	gpiod_add_lookup_table(&da830_evm_usb_gpio_lookup);
+	ret = platform_device_register(&da830_evm_usb_vbus_device);
+	if (ret) {
+		pr_warn("%s: Unable to register the vbus supply\n", __func__);
+		return;
+	}
 
 	ret = da8xx_register_usb11(&da830_evm_usb11_pdata);
 	if (ret)
-- 
2.20.1

[PATCH 3/3] usb: ohci-da8xx: drop the vbus GPIO

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

All users now setup a fixed regulator for the vbus supply. We can drop
the vbus GPIO code. We need to modify the over-current interrupt
handler to use a threaded irq as we cannot disable a regulator from
interrupt context.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/usb/host/ohci-da8xx.c | 39 ++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
index ca8a94f15ac0..f3892034de58 100644
--- a/drivers/usb/host/ohci-da8xx.c
+++ b/drivers/usb/host/ohci-da8xx.c
@@ -41,7 +41,6 @@ struct da8xx_ohci_hcd {
 	struct regulator *vbus_reg;
 	struct notifier_block nb;
 	unsigned int reg_enabled;
-	struct gpio_desc *vbus_gpio;
 	struct gpio_desc *oc_gpio;
 };
 
@@ -92,11 +91,6 @@ static int ohci_da8xx_set_power(struct usb_hcd *hcd, int on)
 	struct device *dev = hcd->self.controller;
 	int ret;
 
-	if (da8xx_ohci->vbus_gpio) {
-		gpiod_set_value_cansleep(da8xx_ohci->vbus_gpio, on);
-		return 0;
-	}
-
 	if (!da8xx_ohci->vbus_reg)
 		return 0;
 
@@ -124,9 +118,6 @@ static int ohci_da8xx_get_power(struct usb_hcd *hcd)
 {
 	struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
 
-	if (da8xx_ohci->vbus_gpio)
-		return gpiod_get_value_cansleep(da8xx_ohci->vbus_gpio);
-
 	if (da8xx_ohci->vbus_reg)
 		return regulator_is_enabled(da8xx_ohci->vbus_reg);
 
@@ -159,9 +150,6 @@ static int ohci_da8xx_has_set_power(struct usb_hcd *hcd)
 {
 	struct da8xx_ohci_hcd *da8xx_ohci = to_da8xx_ohci(hcd);
 
-	if (da8xx_ohci->vbus_gpio)
-		return 1;
-
 	if (da8xx_ohci->vbus_reg)
 		return 1;
 
@@ -211,7 +199,16 @@ static irqreturn_t ohci_da8xx_oc_handler(int irq, void *data)
 	struct da8xx_ohci_hcd *da8xx_ohci = data;
 
 	if (gpiod_get_value(da8xx_ohci->oc_gpio))
-		gpiod_set_value(da8xx_ohci->vbus_gpio, 0);
+		return IRQ_WAKE_THREAD;
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t ohci_da8xx_oc_thread(int irq, void *data)
+{
+	struct da8xx_ohci_hcd *da8xx_ohci = data;
+
+	regulator_disable(da8xx_ohci->vbus_reg);
 
 	return IRQ_HANDLED;
 }
@@ -424,11 +421,6 @@ static int ohci_da8xx_probe(struct platform_device *pdev)
 		}
 	}
 
-	da8xx_ohci->vbus_gpio = devm_gpiod_get_optional(dev, "vbus",
-							GPIOD_OUT_HIGH);
-	if (IS_ERR(da8xx_ohci->vbus_gpio))
-		goto err;
-
 	da8xx_ohci->oc_gpio = devm_gpiod_get_optional(dev, "oc", GPIOD_IN);
 	if (IS_ERR(da8xx_ohci->oc_gpio))
 		goto err;
@@ -438,9 +430,14 @@ 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,
-				"OHCI over-current indicator", da8xx_ohci);
+		error = devm_request_threaded_irq(dev, oc_irq,
+						  ohci_da8xx_oc_handler,
+						  ohci_da8xx_oc_thread,
+						  IRQF_TRIGGER_RISING |
+						  IRQF_TRIGGER_FALLING |
+						  IRQF_ONESHOT,
+						  "OHCI over-current indicator",
+						  da8xx_ohci);
 		if (error)
 			goto err;
 	}
-- 
2.20.1

Re: [PATCH 0/3] ARM: davinci: ohci-da8xx: model the vbus GPIO as a fixed regulator

[Sekhar Nori]

Hi Bart,

On 26/03/19 9:27 PM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Adding the vbus GPIO support to the ohci-da8xx driver isn't really the
> optimal solution. Rather: it should be modeled as a fixed regulator
> in which case the driver already has support.

Can you clarify "driver already has support"? You are introducing
support to use the VBUS gpio as regulator as part of 3/3.

I do see other instances of VBUS regulator being used in USB tree. But
we just converted the driver to use VBUS and over-current GPIOs in v5.1.
So this is a bit of "churn".

Can you document why the current solution is not optimal? Is it to make
future device-tree conversion for these boards easier? Or?

> 
> This series adds necessary fixups to the board files and removes the
> vbus GPIO from the ohci driver.

Thanks,
Sekhar

Re: [PATCH 0/3] ARM: davinci: ohci-da8xx: model the vbus GPIO as a fixed regulator

[Bartosz Golaszewski]

śr., 27 mar 2019 o 12:37 Sekhar Nori <nsekhar@ti.com> napisał(a):
>
> Hi Bart,
>
> On 26/03/19 9:27 PM, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > Adding the vbus GPIO support to the ohci-da8xx driver isn't really the
> > optimal solution. Rather: it should be modeled as a fixed regulator
> > in which case the driver already has support.
>
> Can you clarify "driver already has support"? You are introducing
> support to use the VBUS gpio as regulator as part of 3/3.
>

The support is there as in: if the driver can obtain the regulator, it
will enable it. The overcurrent protection does not work however and
this is what patch 3 adds. Maybe I should rework the ordering in that
I'd first add the irq thread disabling the regulator if it exists,
then the regulator fixups to board files and then remove the vbus
GPIO.

> I do see other instances of VBUS regulator being used in USB tree. But
> we just converted the driver to use VBUS and over-current GPIOs in v5.1.
> So this is a bit of "churn".
>

Yes and it's my fault - I simply converted the legacy code without
giving it enough consideration. I should have used a fixed regulator
right away, but now it's upstream and we need a follow-up series.

> Can you document why the current solution is not optimal? Is it to make
> future device-tree conversion for these boards easier? Or?
>

It's sub-optimal from the HW modeling in SW PoV - it is in fact a
regulator enabled/disabled by a GPIO. Also: it's code duplication as
currently we check if the vbus GPIO exists and then use it or check if
the regulator exists and use this as the second choice. The third
patch actually shrinks the driver.

Bart

> >
> > This series adds necessary fixups to the board files and removes the
> > vbus GPIO from the ohci driver.
>
> Thanks,
> Sekhar

Re: [PATCH 0/3] ARM: davinci: ohci-da8xx: model the vbus GPIO as a fixed regulator

[Sekhar Nori]

On 27/03/19 6:46 PM, Bartosz Golaszewski wrote:
> śr., 27 mar 2019 o 12:37 Sekhar Nori <nsekhar@ti.com> napisał(a):
>>
>> Hi Bart,
>>
>> On 26/03/19 9:27 PM, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>>>
>>> Adding the vbus GPIO support to the ohci-da8xx driver isn't really the
>>> optimal solution. Rather: it should be modeled as a fixed regulator
>>> in which case the driver already has support.
>>
>> Can you clarify "driver already has support"? You are introducing
>> support to use the VBUS gpio as regulator as part of 3/3.
>>
> 
> The support is there as in: if the driver can obtain the regulator, it
> will enable it. The overcurrent protection does not work however and
> this is what patch 3 adds. Maybe I should rework the ordering in that
> I'd first add the irq thread disabling the regulator if it exists,
> then the regulator fixups to board files and then remove the vbus
> GPIO.
> 
>> I do see other instances of VBUS regulator being used in USB tree. But
>> we just converted the driver to use VBUS and over-current GPIOs in v5.1.
>> So this is a bit of "churn".
>>
> 
> Yes and it's my fault - I simply converted the legacy code without
> giving it enough consideration. I should have used a fixed regulator
> right away, but now it's upstream and we need a follow-up series.
> 
>> Can you document why the current solution is not optimal? Is it to make
>> future device-tree conversion for these boards easier? Or?
>>
> 
> It's sub-optimal from the HW modeling in SW PoV - it is in fact a
> regulator enabled/disabled by a GPIO. Also: it's code duplication as
> currently we check if the vbus GPIO exists and then use it or check if
> the regulator exists and use this as the second choice. The third
> patch actually shrinks the driver.

I see now that the driver supports controlling the VBUS gpio as
regulator already. Something I should have caught in review last time
around.

I agree this patch is an improvement. Lets see what Alan feels.

Also, reg_enabled member of da8xx_ohci_hcd structure seems to be pretty
useless considering regulator API already has use counting. Can you take
a look and remove that too as an added bonus.

Thanks,
Sekhar

Re: [PATCH 0/3] ARM: davinci: ohci-da8xx: model the vbus GPIO as a fixed regulator

[Alan Stern]

On Thu, 28 Mar 2019, Sekhar Nori wrote:

> >> Can you document why the current solution is not optimal? Is it to make
> >> future device-tree conversion for these boards easier? Or?
> >>
> > 
> > It's sub-optimal from the HW modeling in SW PoV - it is in fact a
> > regulator enabled/disabled by a GPIO. Also: it's code duplication as
> > currently we check if the vbus GPIO exists and then use it or check if
> > the regulator exists and use this as the second choice. The third
> > patch actually shrinks the driver.
> 
> I see now that the driver supports controlling the VBUS gpio as
> regulator already. Something I should have caught in review last time
> around.
> 
> I agree this patch is an improvement. Lets see what Alan feels.

I'm not an expert on this stuff, but the patch looks reasonable.  
However, I do wish that in the devm_request_threaded_irq() call, the 
indentation of the continuation lines was left unchanged.

Alan Stern

> Also, reg_enabled member of da8xx_ohci_hcd structure seems to be pretty
> useless considering regulator API already has use counting. Can you take
> a look and remove that too as an added bonus.
> 
> Thanks,
> Sekhar

Re: [PATCH 0/3] ARM: davinci: ohci-da8xx: model the vbus GPIO as a fixed regulator

[Bartosz Golaszewski]

czw., 28 mar 2019 o 15:11 Alan Stern <stern@rowland.harvard.edu> napisał(a):
>
> On Thu, 28 Mar 2019, Sekhar Nori wrote:
>
> > >> Can you document why the current solution is not optimal? Is it to make
> > >> future device-tree conversion for these boards easier? Or?
> > >>
> > >
> > > It's sub-optimal from the HW modeling in SW PoV - it is in fact a
> > > regulator enabled/disabled by a GPIO. Also: it's code duplication as
> > > currently we check if the vbus GPIO exists and then use it or check if
> > > the regulator exists and use this as the second choice. The third
> > > patch actually shrinks the driver.
> >
> > I see now that the driver supports controlling the VBUS gpio as
> > regulator already. Something I should have caught in review last time
> > around.
> >
> > I agree this patch is an improvement. Lets see what Alan feels.
>
> I'm not an expert on this stuff, but the patch looks reasonable.
> However, I do wish that in the devm_request_threaded_irq() call, the
> indentation of the continuation lines was left unchanged.
>

I don't think it's possible - the function name is longer and the
first line exceeds the 80 characters limit. I can put all the
parameters below the function name if you prefer that?

Bart

> Alan Stern
>
> > Also, reg_enabled member of da8xx_ohci_hcd structure seems to be pretty
> > useless considering regulator API already has use counting. Can you take
> > a look and remove that too as an added bonus.
> >
> > Thanks,
> > Sekhar
>
>

Re: [PATCH 0/3] ARM: davinci: ohci-da8xx: model the vbus GPIO as a fixed regulator

[Alan Stern]

On Thu, 28 Mar 2019, Bartosz Golaszewski wrote:

> czw., 28 mar 2019 o 15:11 Alan Stern <stern@rowland.harvard.edu> napisał(a):
> >
> > On Thu, 28 Mar 2019, Sekhar Nori wrote:
> >
> > > >> Can you document why the current solution is not optimal? Is it to make
> > > >> future device-tree conversion for these boards easier? Or?
> > > >>
> > > >
> > > > It's sub-optimal from the HW modeling in SW PoV - it is in fact a
> > > > regulator enabled/disabled by a GPIO. Also: it's code duplication as
> > > > currently we check if the vbus GPIO exists and then use it or check if
> > > > the regulator exists and use this as the second choice. The third
> > > > patch actually shrinks the driver.
> > >
> > > I see now that the driver supports controlling the VBUS gpio as
> > > regulator already. Something I should have caught in review last time
> > > around.
> > >
> > > I agree this patch is an improvement. Lets see what Alan feels.
> >
> > I'm not an expert on this stuff, but the patch looks reasonable.
> > However, I do wish that in the devm_request_threaded_irq() call, the
> > indentation of the continuation lines was left unchanged.
> >
> 
> I don't think it's possible - the function name is longer and the
> first line exceeds the 80 characters limit. I can put all the
> parameters below the function name if you prefer that?

Which line the arguments go on doesn't matter.  But increasing the 
amount of indentation, like the patch did, makes the whole thing less 
readable, IMO.  It makes everything end up crammed against the right 
margin.

Alan Stern