[PATCH 1/2 v2] usb: ohci-omap: Create private state container

[PATCH 1/2 v2] usb: ohci-omap: Create private state container

[Linus Walleij]

The OMAP1 was using static locals to hold the clock handles
which is uncommon and does not scale. Create a private data
struct and use that to hold the clocks.

Cc: Janusz Krzysztofik <jmkrzyszt@gmail.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Fix up the error path to free the HCD *after* putting the
  stuff inside the state container.
- Also fix up the remove() path similarly.
- Use some reasonable names on errorpath labels we are touching or
  adding.
- Fix a tab alignment.
---
 drivers/usb/host/ohci-omap.c | 85 +++++++++++++++++++++---------------
 1 file changed, 49 insertions(+), 36 deletions(-)

diff --git a/drivers/usb/host/ohci-omap.c b/drivers/usb/host/ohci-omap.c
index d8d35d456456..a4bdd2b7af83 100644
--- a/drivers/usb/host/ohci-omap.c
+++ b/drivers/usb/host/ohci-omap.c
@@ -69,22 +69,27 @@ static inline int tps65010_set_gpio_out_value(unsigned gpio, unsigned value)
 
 #endif
 
-static struct clk *usb_host_ck;
-static struct clk *usb_dc_ck;
+struct ohci_omap_priv {
+	struct clk *usb_host_ck;
+	struct clk *usb_dc_ck;
+};
 
 static const char hcd_name[] = "ohci-omap";
 static struct hc_driver __read_mostly ohci_omap_hc_driver;
 
-static void omap_ohci_clock_power(int on)
+#define hcd_to_ohci_omap_priv(h) \
+	((struct ohci_omap_priv *)hcd_to_ohci(h)->priv)
+
+static void omap_ohci_clock_power(struct ohci_omap_priv *priv, int on)
 {
 	if (on) {
-		clk_enable(usb_dc_ck);
-		clk_enable(usb_host_ck);
+		clk_enable(priv->usb_dc_ck);
+		clk_enable(priv->usb_host_ck);
 		/* guesstimate for T5 == 1x 32K clock + APLL lock time */
 		udelay(100);
 	} else {
-		clk_disable(usb_host_ck);
-		clk_disable(usb_dc_ck);
+		clk_disable(priv->usb_host_ck);
+		clk_disable(priv->usb_dc_ck);
 	}
 }
 
@@ -196,6 +201,7 @@ static int ohci_omap_reset(struct usb_hcd *hcd)
 {
 	struct ohci_hcd		*ohci = hcd_to_ohci(hcd);
 	struct omap_usb_config	*config = dev_get_platdata(hcd->self.controller);
+	struct ohci_omap_priv	*priv = hcd_to_ohci_omap_priv(hcd);
 	int			need_transceiver = (config->otg != 0);
 	int			ret;
 
@@ -235,7 +241,7 @@ static int ohci_omap_reset(struct usb_hcd *hcd)
 	}
 #endif
 
-	omap_ohci_clock_power(1);
+	omap_ohci_clock_power(priv, 1);
 
 	if (cpu_is_omap15xx()) {
 		omap_1510_local_bus_power(1);
@@ -305,6 +311,7 @@ static int ohci_hcd_omap_probe(struct platform_device *pdev)
 {
 	int retval, irq;
 	struct usb_hcd *hcd = 0;
+	struct ohci_omap_priv *priv;
 
 	if (pdev->num_resources != 2) {
 		dev_err(&pdev->dev, "invalid num_resources: %i\n",
@@ -318,34 +325,35 @@ static int ohci_hcd_omap_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	usb_host_ck = clk_get(&pdev->dev, "usb_hhc_ck");
-	if (IS_ERR(usb_host_ck))
-		return PTR_ERR(usb_host_ck);
+	hcd = usb_create_hcd(&ohci_omap_hc_driver, &pdev->dev,
+			dev_name(&pdev->dev));
+	if (!hcd)
+		return -ENOMEM;
 
-	if (!cpu_is_omap15xx())
-		usb_dc_ck = clk_get(&pdev->dev, "usb_dc_ck");
-	else
-		usb_dc_ck = clk_get(&pdev->dev, "lb_ck");
+	hcd->rsrc_start = pdev->resource[0].start;
+	hcd->rsrc_len = pdev->resource[0].end - pdev->resource[0].start + 1;
+	priv = hcd_to_ohci_omap_priv(hcd);
 
-	if (IS_ERR(usb_dc_ck)) {
-		clk_put(usb_host_ck);
-		return PTR_ERR(usb_dc_ck);
+	priv->usb_host_ck = clk_get(&pdev->dev, "usb_hhc_ck");
+	if (IS_ERR(priv->usb_host_ck)) {
+		retval = PTR_ERR(priv->usb_host_ck);
+		goto err_put_hcd;
 	}
 
+	if (!cpu_is_omap15xx())
+		priv->usb_dc_ck = clk_get(&pdev->dev, "usb_dc_ck");
+	else
+		priv->usb_dc_ck = clk_get(&pdev->dev, "lb_ck");
 
-	hcd = usb_create_hcd(&ohci_omap_hc_driver, &pdev->dev,
-			dev_name(&pdev->dev));
-	if (!hcd) {
-		retval = -ENOMEM;
-		goto err0;
+	if (IS_ERR(priv->usb_dc_ck)) {
+		retval = PTR_ERR(priv->usb_dc_ck);
+		goto err_put_host_ck;
 	}
-	hcd->rsrc_start = pdev->resource[0].start;
-	hcd->rsrc_len = pdev->resource[0].end - pdev->resource[0].start + 1;
 
 	if (!request_mem_region(hcd->rsrc_start, hcd->rsrc_len, hcd_name)) {
 		dev_dbg(&pdev->dev, "request_mem_region failed\n");
 		retval = -EBUSY;
-		goto err1;
+		goto err_put_dc_ck;
 	}
 
 	hcd->regs = ioremap(hcd->rsrc_start, hcd->rsrc_len);
@@ -370,11 +378,12 @@ static int ohci_hcd_omap_probe(struct platform_device *pdev)
 	iounmap(hcd->regs);
 err2:
 	release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
-err1:
+err_put_dc_ck:
+	clk_put(priv->usb_dc_ck);
+err_put_host_ck:
+	clk_put(priv->usb_host_ck);
+err_put_hcd:
 	usb_put_hcd(hcd);
-err0:
-	clk_put(usb_dc_ck);
-	clk_put(usb_host_ck);
 	return retval;
 }
 
@@ -393,10 +402,11 @@ static int ohci_hcd_omap_probe(struct platform_device *pdev)
 static int ohci_hcd_omap_remove(struct platform_device *pdev)
 {
 	struct usb_hcd	*hcd = platform_get_drvdata(pdev);
+	struct ohci_omap_priv *priv = hcd_to_ohci_omap_priv(hcd);
 
 	dev_dbg(hcd->self.controller, "stopping USB Controller\n");
 	usb_remove_hcd(hcd);
-	omap_ohci_clock_power(0);
+	omap_ohci_clock_power(priv, 0);
 	if (!IS_ERR_OR_NULL(hcd->usb_phy)) {
 		(void) otg_set_host(hcd->usb_phy->otg, 0);
 		usb_put_phy(hcd->usb_phy);
@@ -405,9 +415,9 @@ static int ohci_hcd_omap_remove(struct platform_device *pdev)
 		gpio_free(9);
 	iounmap(hcd->regs);
 	release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
+	clk_put(priv->usb_dc_ck);
+	clk_put(priv->usb_host_ck);
 	usb_put_hcd(hcd);
-	clk_put(usb_dc_ck);
-	clk_put(usb_host_ck);
 	return 0;
 }
 
@@ -419,6 +429,7 @@ static int ohci_omap_suspend(struct platform_device *pdev, pm_message_t message)
 {
 	struct usb_hcd *hcd = platform_get_drvdata(pdev);
 	struct ohci_hcd *ohci = hcd_to_ohci(hcd);
+	struct ohci_omap_priv *priv = hcd_to_ohci_omap_priv(hcd);
 	bool do_wakeup = device_may_wakeup(&pdev->dev);
 	int ret;
 
@@ -430,7 +441,7 @@ static int ohci_omap_suspend(struct platform_device *pdev, pm_message_t message)
 	if (ret)
 		return ret;
 
-	omap_ohci_clock_power(0);
+	omap_ohci_clock_power(priv, 0);
 	return ret;
 }
 
@@ -438,12 +449,13 @@ static int ohci_omap_resume(struct platform_device *dev)
 {
 	struct usb_hcd	*hcd = platform_get_drvdata(dev);
 	struct ohci_hcd	*ohci = hcd_to_ohci(hcd);
+	struct ohci_omap_priv *priv = hcd_to_ohci_omap_priv(hcd);
 
 	if (time_before(jiffies, ohci->next_statechange))
 		msleep(5);
 	ohci->next_statechange = jiffies;
 
-	omap_ohci_clock_power(1);
+	omap_ohci_clock_power(priv, 1);
 	ohci_resume(hcd, false);
 	return 0;
 }
@@ -470,7 +482,8 @@ static struct platform_driver ohci_hcd_omap_driver = {
 
 static const struct ohci_driver_overrides omap_overrides __initconst = {
 	.product_desc	= "OMAP OHCI",
-	.reset		= ohci_omap_reset
+	.reset		= ohci_omap_reset,
+	.extra_priv_size = sizeof(struct ohci_omap_priv),
 };
 
 static int __init ohci_omap_init(void)
-- 
2.26.2

[PATCH 2/2 v2] usb: ohci-omap: Convert to use GPIO descriptors

[Linus Walleij]

The OMAP1 OHCI driver is using the legacy GPIO API to grab some
random GPIO lines. One is from the TPS65010 chip and used for
power, another one is for overcurrent and while the driver picks
this line it doesn't watch it at all.

Convert the driver and the OMAP1 OSK board file to pass these
two GPIOs as machine descrived GPIO descriptors.

Cc: Janusz Krzysztofik <jmkrzyszt@gmail.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Rebase on the changes to patch 1
- Fix up the errorpath in the same vein as in patch 1
---
 arch/arm/mach-omap1/board-osk.c | 17 +++++++++++
 drivers/usb/host/ohci-omap.c    | 53 ++++++++++++++++-----------------
 2 files changed, 43 insertions(+), 27 deletions(-)

diff --git a/arch/arm/mach-omap1/board-osk.c b/arch/arm/mach-omap1/board-osk.c
index 4df15e693b6e..d5d139bbac34 100644
--- a/arch/arm/mach-omap1/board-osk.c
+++ b/arch/arm/mach-omap1/board-osk.c
@@ -26,6 +26,7 @@
  * 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 #include <linux/gpio.h>
+#include <linux/gpio/machine.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/platform_device.h>
@@ -55,6 +56,9 @@
 
 #include "common.h"
 
+/* Name of the GPIO chip used by the OMAP for GPIOs 0..15 */
+#define OMAP_GPIO_LABEL		"gpio-0-15"
+
 /* At OMAP5912 OSK the Ethernet is directly connected to CS1 */
 #define OMAP_OSK_ETHR_START		0x04800300
 
@@ -240,7 +244,9 @@ static struct tps65010_board tps_board = {
 
 static struct i2c_board_info __initdata osk_i2c_board_info[] = {
 	{
+		/* This device will get the name "i2c-tps65010" */
 		I2C_BOARD_INFO("tps65010", 0x48),
+		.dev_name = "tps65010",
 		.platform_data	= &tps_board,
 
 	},
@@ -278,6 +284,16 @@ static void __init osk_init_cf(void)
 	irq_set_irq_type(gpio_to_irq(62), IRQ_TYPE_EDGE_FALLING);
 }
 
+static struct gpiod_lookup_table osk_usb_gpio_table = {
+	.dev_id = "ohci",
+	.table = {
+		/* Power GPIO on the I2C-attached TPS65010 */
+                GPIO_LOOKUP("i2c-tps65010", 1, "power", GPIO_ACTIVE_HIGH),
+		GPIO_LOOKUP(OMAP_GPIO_LABEL, 9, "overcurrent",
+			    GPIO_ACTIVE_HIGH),
+	},
+};
+
 static struct omap_usb_config osk_usb_config __initdata = {
 	/* has usb host connector (A) ... for development it can also
 	 * be used, with a NONSTANDARD gender-bending cable/dongle, as
@@ -581,6 +597,7 @@ static void __init osk_init(void)
 	l |= (3 << 1);
 	omap_writel(l, USB_TRANSCEIVER_CTRL);
 
+	gpiod_add_lookup_table(&osk_usb_gpio_table);
 	omap1_usb_init(&osk_usb_config);
 
 	/* irq for tps65010 chip */
diff --git a/drivers/usb/host/ohci-omap.c b/drivers/usb/host/ohci-omap.c
index a4bdd2b7af83..5d5ecca3739e 100644
--- a/drivers/usb/host/ohci-omap.c
+++ b/drivers/usb/host/ohci-omap.c
@@ -18,7 +18,7 @@
 #include <linux/clk.h>
 #include <linux/dma-mapping.h>
 #include <linux/err.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/io.h>
 #include <linux/jiffies.h>
 #include <linux/kernel.h>
@@ -53,25 +53,11 @@
 
 #define DRIVER_DESC "OHCI OMAP driver"
 
-#ifdef CONFIG_TPS65010
-#include <linux/mfd/tps65010.h>
-#else
-
-#define LOW	0
-#define HIGH	1
-
-#define GPIO1	1
-
-static inline int tps65010_set_gpio_out_value(unsigned gpio, unsigned value)
-{
-	return 0;
-}
-
-#endif
-
 struct ohci_omap_priv {
 	struct clk *usb_host_ck;
 	struct clk *usb_dc_ck;
+	struct gpio_desc *power;
+	struct gpio_desc *overcurrent;
 };
 
 static const char hcd_name[] = "ohci-omap";
@@ -97,22 +83,22 @@ static void omap_ohci_clock_power(struct ohci_omap_priv *priv, int on)
  * Board specific gang-switched transceiver power on/off.
  * NOTE:  OSK supplies power from DC, not battery.
  */
-static int omap_ohci_transceiver_power(int on)
+static int omap_ohci_transceiver_power(struct ohci_omap_priv *priv, int on)
 {
 	if (on) {
 		if (machine_is_omap_innovator() && cpu_is_omap1510())
 			__raw_writeb(__raw_readb(INNOVATOR_FPGA_CAM_USB_CONTROL)
 				| ((1 << 5/*usb1*/) | (1 << 3/*usb2*/)),
 			       INNOVATOR_FPGA_CAM_USB_CONTROL);
-		else if (machine_is_omap_osk())
-			tps65010_set_gpio_out_value(GPIO1, LOW);
+		else if (priv->power)
+			gpiod_set_value(priv->power, 0);
 	} else {
 		if (machine_is_omap_innovator() && cpu_is_omap1510())
 			__raw_writeb(__raw_readb(INNOVATOR_FPGA_CAM_USB_CONTROL)
 				& ~((1 << 5/*usb1*/) | (1 << 3/*usb2*/)),
 			       INNOVATOR_FPGA_CAM_USB_CONTROL);
-		else if (machine_is_omap_osk())
-			tps65010_set_gpio_out_value(GPIO1, HIGH);
+		else if (priv->power)
+			gpiod_set_value(priv->power, 1);
 	}
 
 	return 0;
@@ -272,8 +258,6 @@ static int ohci_omap_reset(struct usb_hcd *hcd)
 
 			/* gpio9 for overcurrent detction */
 			omap_cfg_reg(W8_1610_GPIO9);
-			gpio_request(9, "OHCI overcurrent");
-			gpio_direction_input(9);
 
 			/* for paranoia's sake:  disable USB.PUEN */
 			omap_cfg_reg(W4_USB_HIGHZ);
@@ -287,7 +271,7 @@ static int ohci_omap_reset(struct usb_hcd *hcd)
 	}
 
 	/* FIXME hub_wq hub requests should manage power switching */
-	omap_ohci_transceiver_power(1);
+	omap_ohci_transceiver_power(priv, 1);
 
 	/* board init will have already handled HMC and mux setup.
 	 * any external transceiver should already be initialized
@@ -334,6 +318,23 @@ static int ohci_hcd_omap_probe(struct platform_device *pdev)
 	hcd->rsrc_len = pdev->resource[0].end - pdev->resource[0].start + 1;
 	priv = hcd_to_ohci_omap_priv(hcd);
 
+	/* Obtain two optional GPIO lines */
+	priv->power = devm_gpiod_get_optional(&pdev->dev, "power", GPIOD_ASIS);
+	if (IS_ERR(priv->power)) {
+		retval = PTR_ERR(priv->power);
+		goto err_put_hcd;
+	}
+	if (priv->power)
+		gpiod_set_consumer_name(priv->power, "OHCI power");
+	priv->overcurrent = devm_gpiod_get_optional(&pdev->dev, "overcurrent",
+						    GPIOD_IN);
+	if (IS_ERR(priv->overcurrent)) {
+		retval = PTR_ERR(priv->overcurrent);
+		goto err_put_hcd;
+	}
+	if (priv->overcurrent)
+		gpiod_set_consumer_name(priv->overcurrent, "OHCI overcurrent");
+
 	priv->usb_host_ck = clk_get(&pdev->dev, "usb_hhc_ck");
 	if (IS_ERR(priv->usb_host_ck)) {
 		retval = PTR_ERR(priv->usb_host_ck);
@@ -411,8 +412,6 @@ static int ohci_hcd_omap_remove(struct platform_device *pdev)
 		(void) otg_set_host(hcd->usb_phy->otg, 0);
 		usb_put_phy(hcd->usb_phy);
 	}
-	if (machine_is_omap_osk())
-		gpio_free(9);
 	iounmap(hcd->regs);
 	release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
 	clk_put(priv->usb_dc_ck);
-- 
2.26.2

Re: [PATCH 1/2 v2] usb: ohci-omap: Create private state container

[Alan Stern]

On Thu, Jul 16, 2020 at 02:34:32PM +0200, Linus Walleij wrote:
> The OMAP1 was using static locals to hold the clock handles
> which is uncommon and does not scale. Create a private data
> struct and use that to hold the clocks.
> 
> Cc: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Fix up the error path to free the HCD *after* putting the
>   stuff inside the state container.
> - Also fix up the remove() path similarly.
> - Use some reasonable names on errorpath labels we are touching or
>   adding.
> - Fix a tab alignment.
> ---

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Re: [PATCH 2/2 v2] usb: ohci-omap: Convert to use GPIO descriptors

[Alan Stern]

On Thu, Jul 16, 2020 at 02:34:33PM +0200, Linus Walleij wrote:
> The OMAP1 OHCI driver is using the legacy GPIO API to grab some
> random GPIO lines. One is from the TPS65010 chip and used for
> power, another one is for overcurrent and while the driver picks
> this line it doesn't watch it at all.

If the driver doesn't use this GPIO, why keep the code to grab it?

> Convert the driver and the OMAP1 OSK board file to pass these
> two GPIOs as machine descrived GPIO descriptors.
-----------------------------^

> 
> Cc: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---

Aside from this I don't have any objections.

Alan Stern

Re: [PATCH 2/2 v2] usb: ohci-omap: Convert to use GPIO descriptors

[Linus Walleij]

On Thu, Jul 16, 2020 at 5:41 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> On Thu, Jul 16, 2020 at 02:34:33PM +0200, Linus Walleij wrote:
> > The OMAP1 OHCI driver is using the legacy GPIO API to grab some
> > random GPIO lines. One is from the TPS65010 chip and used for
> > power, another one is for overcurrent and while the driver picks
> > this line it doesn't watch it at all.
>
> If the driver doesn't use this GPIO, why keep the code to grab it?

I think it gives a hint to someone who later tries to debug
problems with the hardware that they should maybe go and
complete this overcurrent feature.

Keeping with the paradigm that a patch should be one technological
step (converting to GPIO descriptors) that should be a
third patch (retireing dead code). But I can certainly add a
patch like that if required.

> > Convert the driver and the OMAP1 OSK board file to pass these
> > two GPIOs as machine descrived GPIO descriptors.
> -----------------------------^

Oooups.

> > Cc: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> > Cc: Tony Lindgren <tony@atomide.com>
> > Cc: Alan Stern <stern@rowland.harvard.edu>
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
>
> Aside from this I don't have any objections.

Does that qualify as an ACK or do you still want me to remove
the overcurrent template code?

Yours,
Linus Walleij

Re: [PATCH 2/2 v2] usb: ohci-omap: Convert to use GPIO descriptors

[Alan Stern]

On Fri, Jul 17, 2020 at 03:49:19PM +0200, Linus Walleij wrote:
> On Thu, Jul 16, 2020 at 5:41 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Thu, Jul 16, 2020 at 02:34:33PM +0200, Linus Walleij wrote:
> > > The OMAP1 OHCI driver is using the legacy GPIO API to grab some
> > > random GPIO lines. One is from the TPS65010 chip and used for
> > > power, another one is for overcurrent and while the driver picks
> > > this line it doesn't watch it at all.
> >
> > If the driver doesn't use this GPIO, why keep the code to grab it?
> 
> I think it gives a hint to someone who later tries to debug
> problems with the hardware that they should maybe go and
> complete this overcurrent feature.

Okay, but in that case a nice comment would better serve the purpose.

> Keeping with the paradigm that a patch should be one technological
> step (converting to GPIO descriptors) that should be a
> third patch (retireing dead code). But I can certainly add a
> patch like that if required.

For example, you could insert a patch before this one to remove the 
unused references and replace them with a comment.

> > > Convert the driver and the OMAP1 OSK board file to pass these
> > > two GPIOs as machine descrived GPIO descriptors.
> > -----------------------------^
> 
> Oooups.
> 
> > > Cc: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> > > Cc: Tony Lindgren <tony@atomide.com>
> > > Cc: Alan Stern <stern@rowland.harvard.edu>
> > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> > > ---
> >
> > Aside from this I don't have any objections.
> 
> Does that qualify as an ACK or do you still want me to remove
> the overcurrent template code?

A little dead code with no explanation doesn't bother me very much, 
although it will seem rather odd to anyone who reads the driver 
carefully.  Mainly I wanted to bring this matter to your attention and 
leave it up to your own taste as to how you should proceed.

If you want to resubmit this patch as is (with just the typo fixed), you 
can add my Acked-by.

Alan Stern