[PATCH v2 1/3] usb: gadget: pxa27x_udc: add devicetree support

[PATCH v2 1/3] usb: gadget: pxa27x_udc: add devicetree support

[Robert Jarzmik]

Add support for device-tree device discovery. If devicetree is not
provided, fallback to legacy platform data "discovery".

Signed-off-by: Robert Jarzmik <robert.jarzmik-GANU6spQydw@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

---
Since V1: change OF id mrvl,pxa27x_udc -> marvell,pxa27x-udc
          This is a consequence of other DT reviews on the marvell
          namings.
---
 drivers/usb/gadget/pxa27x_udc.c | 105 ++++++++++++++++++++++++++++++++--------
 drivers/usb/gadget/pxa27x_udc.h |   4 ++
 2 files changed, 90 insertions(+), 19 deletions(-)

diff --git a/drivers/usb/gadget/pxa27x_udc.c b/drivers/usb/gadget/pxa27x_udc.c
index cdf4d67..c7d1976 100644
--- a/drivers/usb/gadget/pxa27x_udc.c
+++ b/drivers/usb/gadget/pxa27x_udc.c
@@ -24,12 +24,15 @@
 #include <linux/gpio.h>
 #include <linux/slab.h>
 #include <linux/prefetch.h>
-#include <linux/byteorder/generic.h>
-#include <linux/platform_data/pxa2xx_udc.h>
+#include <linux/of_device.h>
+
+#include <asm/byteorder.h>
+#include <mach/hardware.h>
 
 #include <linux/usb.h>
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
+#include <mach/udc.h>
 
 #include "pxa27x_udc.h"
 
@@ -1508,16 +1511,16 @@ static struct usb_ep_ops pxa_ep_ops = {
 static void dplus_pullup(struct pxa_udc *udc, int on)
 {
 	if (on) {
-		if (gpio_is_valid(udc->mach->gpio_pullup))
-			gpio_set_value(udc->mach->gpio_pullup,
-				       !udc->mach->gpio_pullup_inverted);
-		if (udc->mach->udc_command)
+		if (gpio_is_valid(udc->gpio_pullup))
+			gpio_set_value(udc->gpio_pullup,
+				       !udc->gpio_pullup_inverted);
+		if (udc->mach && udc->mach->udc_command)
 			udc->mach->udc_command(PXA2XX_UDC_CMD_CONNECT);
 	} else {
-		if (gpio_is_valid(udc->mach->gpio_pullup))
-			gpio_set_value(udc->mach->gpio_pullup,
-				       udc->mach->gpio_pullup_inverted);
-		if (udc->mach->udc_command)
+		if (gpio_is_valid(udc->gpio_pullup))
+			gpio_set_value(udc->gpio_pullup,
+				       udc->gpio_pullup_inverted);
+		if (udc->mach && udc->mach->udc_command)
 			udc->mach->udc_command(PXA2XX_UDC_CMD_DISCONNECT);
 	}
 	udc->pullup_on = on;
@@ -1609,7 +1612,8 @@ static int pxa_udc_pullup(struct usb_gadget *_gadget, int is_active)
 {
 	struct pxa_udc *udc = to_gadget_udc(_gadget);
 
-	if (!gpio_is_valid(udc->mach->gpio_pullup) && !udc->mach->udc_command)
+	if (!gpio_is_valid(udc->gpio_pullup)
+	    && !(udc->mach && udc->mach->udc_command))
 		return -EOPNOTSUPP;
 
 	dplus_pullup(udc, is_active);
@@ -2404,6 +2408,56 @@ static struct pxa_udc memory = {
 	}
 };
 
+static struct of_device_id udc_pxa_dt_ids[] = {
+	{ .compatible = "marvell,pxa27x-udc" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, udc_pxa_dt_ids);
+
+/**
+ * pxa_udc_probe_dt - device tree specific probe
+ * @pdev: platform data
+ * @udc: pxa_udc structure to fill
+ *
+ * Fills udc from platform data out of device tree.
+ *
+ * Returns 0 if DT found, 1 if DT not found, and <0 on error
+ */
+bool pxa_udc_probe_dt(struct platform_device *pdev, struct pxa_udc *udc)
+{
+	struct device_node *np = pdev->dev.of_node;
+	const struct of_device_id *of_id =
+			of_match_device(udc_pxa_dt_ids, &pdev->dev);
+	int ret;
+	u32 gpio_pullup;
+
+	if (!np || !of_id)
+		return 1;
+	ret = of_alias_get_id(np, "udc");
+	pdev->id = (ret >= 0) ? ret : -1;
+
+	if (!of_property_read_u32(np, "udc-pullup-gpio", &gpio_pullup))
+		udc->gpio_pullup = gpio_pullup;
+	udc->gpio_pullup_inverted =
+		of_property_read_bool(np, "udc-pullup-gpio-inverted");
+	return 0;
+}
+
+/**
+ * pxa_udc_probe_pdata - legacy platform data probe
+ * @pdev: platform device
+ * @udc: pxa_udc structure to fill
+ *
+ * Simple copy of data from platform_data to udc control structure
+ */
+static void pxa_udc_probe_pdata(struct platform_device *pdev,
+			       struct pxa_udc *udc)
+{
+	udc->mach = dev_get_platdata(&pdev->dev);
+	udc->gpio_pullup = udc->mach->gpio_pullup;
+	udc->gpio_pullup_inverted = udc->mach->gpio_pullup_inverted;
+}
+
 /**
  * pxa_udc_probe - probes the udc device
  * @_dev: platform device
@@ -2415,7 +2469,13 @@ static int pxa_udc_probe(struct platform_device *pdev)
 {
 	struct resource *regs;
 	struct pxa_udc *udc = &memory;
-	int retval = 0, gpio;
+	int retval = 0;
+
+	retval = pxa_udc_probe_dt(pdev, udc);
+	if (retval < 0)
+		return retval;
+	if (retval > 0)
+		pxa_udc_probe_pdata(pdev, udc);
 
 	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!regs)
@@ -2425,19 +2485,17 @@ static int pxa_udc_probe(struct platform_device *pdev)
 		return udc->irq;
 
 	udc->dev = &pdev->dev;
-	udc->mach = dev_get_platdata(&pdev->dev);
 	udc->transceiver = usb_get_phy(USB_PHY_TYPE_USB2);
 
-	gpio = udc->mach->gpio_pullup;
-	if (gpio_is_valid(gpio)) {
-		retval = gpio_request(gpio, "USB D+ pullup");
+	if (gpio_is_valid(udc->gpio_pullup)) {
+		retval = gpio_request(udc->gpio_pullup, "USB D+ pullup");
 		if (retval == 0)
-			gpio_direction_output(gpio,
-				       udc->mach->gpio_pullup_inverted);
+			gpio_direction_output(udc->gpio_pullup,
+				       udc->gpio_pullup_inverted);
 	}
 	if (retval) {
 		dev_err(&pdev->dev, "Couldn't request gpio %d : %d\n",
-			gpio, retval);
+			udc->gpio_pullup, retval);
 		return retval;
 	}
 
@@ -2446,6 +2504,9 @@ static int pxa_udc_probe(struct platform_device *pdev)
 		retval = PTR_ERR(udc->clk);
 		goto err_clk;
 	}
+	retval = clk_prepare(udc->clk);
+	if (retval)
+		goto err_clk_prepare;
 
 	retval = -ENOMEM;
 	udc->regs = ioremap(regs->start, resource_size(regs));
@@ -2483,9 +2544,13 @@ err_add_udc:
 err_irq:
 	iounmap(udc->regs);
 err_map:
+	clk_unprepare(udc->clk);
+err_clk_prepare:
 	clk_put(udc->clk);
 	udc->clk = NULL;
 err_clk:
+	if (gpio_is_valid(udc->gpio_pullup))
+		gpio_free(udc->gpio_pullup);
 	return retval;
 }
 
@@ -2509,6 +2574,7 @@ static int pxa_udc_remove(struct platform_device *_dev)
 
 	udc->transceiver = NULL;
 	the_controller = NULL;
+	clk_unprepare(udc->clk);
 	clk_put(udc->clk);
 	iounmap(udc->regs);
 
@@ -2609,6 +2675,7 @@ static struct platform_driver udc_driver = {
 	.driver		= {
 		.name	= "pxa27x-udc",
 		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(udc_pxa_dt_ids),
 	},
 	.probe		= pxa_udc_probe,
 	.remove		= pxa_udc_remove,
diff --git a/drivers/usb/gadget/pxa27x_udc.h b/drivers/usb/gadget/pxa27x_udc.h
index 28f2b53..8995b34 100644
--- a/drivers/usb/gadget/pxa27x_udc.h
+++ b/drivers/usb/gadget/pxa27x_udc.h
@@ -421,6 +421,8 @@ struct udc_stats {
  * @driver: bound gadget (zero, g_ether, g_mass_storage, ...)
  * @dev: device
  * @mach: machine info, used to activate specific GPIO
+ * @gpio_pullup: if valid, D+ pullup GPIO
+ * @gpio_pullup_inverted: 1 if pullup has inverted logic
  * @transceiver: external transceiver to handle vbus sense and D+ pullup
  * @ep0state: control endpoint state machine state
  * @stats: statistics on udc usage
@@ -448,6 +450,8 @@ struct pxa_udc {
 	struct device				*dev;
 	struct pxa2xx_udc_mach_info		*mach;
 	struct usb_phy				*transceiver;
+	int					gpio_pullup;
+	int					gpio_pullup_inverted;
 
 	enum ep0_state				ep0state;
 	struct udc_stats			stats;
-- 
2.0.0.rc2

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/3] usb: gadget: pxa27x_udc device-tree documentation

[Robert Jarzmik]

Add documentation for device-tree binding of arm PXA 27x udc (usb
device) driver.

Signed-off-by: Robert Jarzmik <robert.jarzmik-GANU6spQydw@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

---
Since V1: change OF id mrvl,pxa27x_udc -> marvell,pxa27x-udc
          This is a consequence of other DT reviews on the marvell
          namings.
---
 Documentation/devicetree/bindings/usb/pxa-usb.txt | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/pxa-usb.txt b/Documentation/devicetree/bindings/usb/pxa-usb.txt
index 79729a9..0e4863f 100644
--- a/Documentation/devicetree/bindings/usb/pxa-usb.txt
+++ b/Documentation/devicetree/bindings/usb/pxa-usb.txt
@@ -29,3 +29,23 @@ Example:
 		marvell,port-mode = <2>; /* PMM_GLOBAL_MODE */
 	};
 
+UDC
+
+Required properties:
+ - compatible: Should be "marvell,pxa27x-udc" for USB controllers
+   used in device mode.
+
+Optional properties:
+ - "udc-pullup-gpio" - gpio activated to control the USB D+ pullup.
+ - "udc-pullup-gpio-inverted" - boolean inverting gpio pullup logic.
+
+Example:
+
+		pxa27x_udc: udc@40600000 {
+			compatible = "marvell,pxa27x-udc";
+			reg = <0x40600000 0x10000>;
+			interrupts = <11>;
+			clocks = <&pxa2xx_clks 11>;
+			clock-names = "pxa27x-udc";
+			udc-pullup-gpio = <22>;
+		};
-- 
2.0.0.rc2

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] usb: gadget: pxa27x_udc device-tree documentation

[Mark Rutland]

On Sun, Jun 22, 2014 at 10:04:58AM +0100, Robert Jarzmik wrote:
> Add documentation for device-tree binding of arm PXA 27x udc (usb
> device) driver.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik-GANU6spQydw@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> 
> ---
> Since V1: change OF id mrvl,pxa27x_udc -> marvell,pxa27x-udc
>           This is a consequence of other DT reviews on the marvell
>           namings.
> ---
>  Documentation/devicetree/bindings/usb/pxa-usb.txt | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/pxa-usb.txt b/Documentation/devicetree/bindings/usb/pxa-usb.txt
> index 79729a9..0e4863f 100644
> --- a/Documentation/devicetree/bindings/usb/pxa-usb.txt
> +++ b/Documentation/devicetree/bindings/usb/pxa-usb.txt
> @@ -29,3 +29,23 @@ Example:
>  		marvell,port-mode = <2>; /* PMM_GLOBAL_MODE */
>  	};
>  
> +UDC
> +
> +Required properties:
> + - compatible: Should be "marvell,pxa27x-udc" for USB controllers
> +   used in device mode.

We typically don't like wildcard compatible strings in DT.

> +
> +Optional properties:
> + - "udc-pullup-gpio" - gpio activated to control the USB D+ pullup.
> + - "udc-pullup-gpio-inverted" - boolean inverting gpio pullup logic.

Use the GPIO bindings.

> +
> +Example:
> +
> +		pxa27x_udc: udc@40600000 {
> +			compatible = "marvell,pxa27x-udc";
> +			reg = <0x40600000 0x10000>;
> +			interrupts = <11>;
> +			clocks = <&pxa2xx_clks 11>;
> +			clock-names = "pxa27x-udc";

Neither of these were mentioned above.

The name of the clock input doesn't make sense.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] usb: gadget: pxa27x_udc device-tree documentation

[Robert Jarzmik]

Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> writes:

> On Sun, Jun 22, 2014 at 10:04:58AM +0100, Robert Jarzmik wrote:
>> index 79729a9..0e4863f 100644
>> --- a/Documentation/devicetree/bindings/usb/pxa-usb.txt
>> +++ b/Documentation/devicetree/bindings/usb/pxa-usb.txt
>> @@ -29,3 +29,23 @@ Example:
>>  		marvell,port-mode = <2>; /* PMM_GLOBAL_MODE */
>>  	};
>>  
>> +UDC
>> +
>> +Required properties:
>> + - compatible: Should be "marvell,pxa27x-udc" for USB controllers
>> +   used in device mode.
>
> We typically don't like wildcard compatible strings in DT.
Same as in the other patch, "marvell,pxa270-udc".
>
>> +
>> +Optional properties:
>> + - "udc-pullup-gpio" - gpio activated to control the USB D+ pullup.
>> + - "udc-pullup-gpio-inverted" - boolean inverting gpio pullup logic.
>
> Use the GPIO bindings.
OK. I'm presuming in this case you think of something like :
	udc-pullup-gpio = <&gpio 22 GPIO_ACTIVE_LOW>
Which translates into:
	"udc-pullup-gpio" - gpio activated to control the USB D+ pullup (see
        gpio.txt)

>
>> +
>> +Example:
>> +
>> +		pxa27x_udc: udc@40600000 {
>> +			compatible = "marvell,pxa27x-udc";
>> +			reg = <0x40600000 0x10000>;
>> +			interrupts = <11>;
>> +			clocks = <&pxa2xx_clks 11>;
>> +			clock-names = "pxa27x-udc";
>
> Neither of these were mentioned above.
Ah you mean I shall describe the standard reg, interrupts as mandatory standard
options I take it. OK.
>
> The name of the clock input doesn't make sense.
I don't understand. With [1] does it make any more sense ? If not you'll have to
expand a bit more the "doesn't make sense".

Cheers.

--
Robert

[1] http://www.spinics.net/lists/arm-kernel/msg337522.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] usb: gadget: pxa27x_udc device-tree documentation

[Mark Rutland]

On Wed, Jun 25, 2014 at 08:54:01PM +0100, Robert Jarzmik wrote:
> Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> writes:
> 
> > On Sun, Jun 22, 2014 at 10:04:58AM +0100, Robert Jarzmik wrote:
> >> index 79729a9..0e4863f 100644
> >> --- a/Documentation/devicetree/bindings/usb/pxa-usb.txt
> >> +++ b/Documentation/devicetree/bindings/usb/pxa-usb.txt
> >> @@ -29,3 +29,23 @@ Example:
> >>  		marvell,port-mode = <2>; /* PMM_GLOBAL_MODE */
> >>  	};
> >>  
> >> +UDC
> >> +
> >> +Required properties:
> >> + - compatible: Should be "marvell,pxa27x-udc" for USB controllers
> >> +   used in device mode.
> >
> > We typically don't like wildcard compatible strings in DT.
> Same as in the other patch, "marvell,pxa270-udc".
> >
> >> +
> >> +Optional properties:
> >> + - "udc-pullup-gpio" - gpio activated to control the USB D+ pullup.
> >> + - "udc-pullup-gpio-inverted" - boolean inverting gpio pullup logic.
> >
> > Use the GPIO bindings.
> OK. I'm presuming in this case you think of something like :
> 	udc-pullup-gpio = <&gpio 22 GPIO_ACTIVE_LOW>
> Which translates into:
> 	"udc-pullup-gpio" - gpio activated to control the USB D+ pullup (see
>         gpio.txt)

Something like that, yes.

> >
> >> +
> >> +Example:
> >> +
> >> +		pxa27x_udc: udc@40600000 {
> >> +			compatible = "marvell,pxa27x-udc";
> >> +			reg = <0x40600000 0x10000>;
> >> +			interrupts = <11>;
> >> +			clocks = <&pxa2xx_clks 11>;
> >> +			clock-names = "pxa27x-udc";
> >
> > Neither of these were mentioned above.
> Ah you mean I shall describe the standard reg, interrupts as mandatory standard
> options I take it. OK.

Yes. While the properties are standard, their precise meanings (e.g.
which interrupt or address space region), and whether a particular
binding uses them is not.

> > The name of the clock input doesn't make sense.
> I don't understand. With [1] does it make any more sense ? If not you'll have to
> expand a bit more the "doesn't make sense".

My concern is that clock-names is supposed to describe the name of the
input clock line from the view of the IP block. "pxa27x-udc" doesn't
sound like the name of a clock input line from the view of the UDC
block.

I assume the clock input line you care about has a more specific name
than "pxa27x-udc"?

Cheers,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] usb: gadget: pxa27x_udc device-tree documentation

[Robert Jarzmik]

Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> writes:

> On Wed, Jun 25, 2014 at 08:54:01PM +0100, Robert Jarzmik wrote:
>> > The name of the clock input doesn't make sense.
>> I don't understand. With [1] does it make any more sense ? If not you'll have to
>> expand a bit more the "doesn't make sense".
>
> My concern is that clock-names is supposed to describe the name of the
> input clock line from the view of the IP block. "pxa27x-udc" doesn't
> sound like the name of a clock input line from the view of the UDC
> block.
>
> I assume the clock input line you care about has a more specific name
> than "pxa27x-udc"?
Not as far as I know. The technical reference manual call it "udc clock", so
it's even "less" specific ...

Marvell engineers have probably the internal schematics and the name of the
clock, but outsiders like me only have "udc" ...

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] usb: gadget: pxa27x_udc device-tree documentation

[Mark Rutland]

On Sun, Jun 29, 2014 at 10:29:49AM +0100, Robert Jarzmik wrote:
> Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> writes:
> 
> > On Wed, Jun 25, 2014 at 08:54:01PM +0100, Robert Jarzmik wrote:
> >> > The name of the clock input doesn't make sense.
> >> I don't understand. With [1] does it make any more sense ? If not you'll have to
> >> expand a bit more the "doesn't make sense".
> >
> > My concern is that clock-names is supposed to describe the name of the
> > input clock line from the view of the IP block. "pxa27x-udc" doesn't
> > sound like the name of a clock input line from the view of the UDC
> > block.
> >
> > I assume the clock input line you care about has a more specific name
> > than "pxa27x-udc"?
> Not as far as I know. The technical reference manual call it "udc clock", so
> it's even "less" specific ...

Not from the point of view of the device. The clock-names are namespaced
to the particular binding, so they're equally specific.

Given the above I'd recommend naming the clock "udc" or "udc_clk". That
doesn't pretend to be overly specific, and matches the TRM.

> Marvell engineers have probably the internal schematics and the name of the
> clock, but outsiders like me only have "udc" ...

Sure, not having the full specs is always a pain.

Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] usb: gadget: pxa27x_udc: add devicetree support

[Mark Rutland]

On Sun, Jun 22, 2014 at 10:04:57AM +0100, Robert Jarzmik wrote:
> Add support for device-tree device discovery. If devicetree is not
> provided, fallback to legacy platform data "discovery".
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik-GANU6spQydw@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> 
> ---
> Since V1: change OF id mrvl,pxa27x_udc -> marvell,pxa27x-udc
>           This is a consequence of other DT reviews on the marvell
>           namings.
> ---
>  drivers/usb/gadget/pxa27x_udc.c | 105 ++++++++++++++++++++++++++++++++--------
>  drivers/usb/gadget/pxa27x_udc.h |   4 ++
>  2 files changed, 90 insertions(+), 19 deletions(-)

[...]

> +/**
> + * pxa_udc_probe_dt - device tree specific probe
> + * @pdev: platform data
> + * @udc: pxa_udc structure to fill
> + *
> + * Fills udc from platform data out of device tree.
> + *
> + * Returns 0 if DT found, 1 if DT not found, and <0 on error

That's impossible as this function is marked as returning bool.

Make this an int and return negative error codes.

> + */
> +bool pxa_udc_probe_dt(struct platform_device *pdev, struct pxa_udc *udc)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	const struct of_device_id *of_id =
> +			of_match_device(udc_pxa_dt_ids, &pdev->dev);
> +	int ret;
> +	u32 gpio_pullup;
> +
> +	if (!np || !of_id)
> +		return 1;
> +	ret = of_alias_get_id(np, "udc");
> +	pdev->id = (ret >= 0) ? ret : -1;

The alias name wasn't mentioned in the binding...

> +
> +	if (!of_property_read_u32(np, "udc-pullup-gpio", &gpio_pullup))
> +		udc->gpio_pullup = gpio_pullup;

This number is a Linux internal detail. Use the GPIO bindings.

> +	udc->gpio_pullup_inverted =
> +		of_property_read_bool(np, "udc-pullup-gpio-inverted");

The GPIO bindings can describe this.

> @@ -2415,7 +2469,13 @@ static int pxa_udc_probe(struct platform_device *pdev)
>  {
>  	struct resource *regs;
>  	struct pxa_udc *udc = &memory;
> -	int retval = 0, gpio;
> +	int retval = 0;
> +
> +	retval = pxa_udc_probe_dt(pdev, udc);
> +	if (retval < 0)
> +		return retval;

This case can never happen given the prototype of pxa_udc_probe_dt.

> @@ -2446,6 +2504,9 @@ static int pxa_udc_probe(struct platform_device *pdev)
>  		retval = PTR_ERR(udc->clk);
>  		goto err_clk;
>  	}
> +	retval = clk_prepare(udc->clk);
> +	if (retval)
> +		goto err_clk_prepare;
>  
>  	retval = -ENOMEM;
>  	udc->regs = ioremap(regs->start, resource_size(regs));
> @@ -2483,9 +2544,13 @@ err_add_udc:
>  err_irq:
>  	iounmap(udc->regs);
>  err_map:
> +	clk_unprepare(udc->clk);
> +err_clk_prepare:
>  	clk_put(udc->clk);
>  	udc->clk = NULL;
>  err_clk:
> +	if (gpio_is_valid(udc->gpio_pullup))
> +		gpio_free(udc->gpio_pullup);
>  	return retval;
>  }
>  
> @@ -2509,6 +2574,7 @@ static int pxa_udc_remove(struct platform_device *_dev)
>  
>  	udc->transceiver = NULL;
>  	the_controller = NULL;
> +	clk_unprepare(udc->clk);
>  	clk_put(udc->clk);

I don't see why these clock changes should be in the same patch as the
DT support. They might be a prerequisite, but as far as I can tell they
are required even without DT probing.

Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] usb: gadget: pxa27x_udc: add devicetree support

[Robert Jarzmik]

Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> writes:

> On Sun, Jun 22, 2014 at 10:04:57AM +0100, Robert Jarzmik wrote:
>> +/**
>> + * pxa_udc_probe_dt - device tree specific probe
>> + * @pdev: platform data
>> + * @udc: pxa_udc structure to fill
>> + *
>> + * Fills udc from platform data out of device tree.
>> + *
>> + * Returns 0 if DT found, 1 if DT not found, and <0 on error
>
> That's impossible as this function is marked as returning bool.
> Make this an int and return negative error codes.
Yes, it was designed to be an int. I don't know why this "bool" appeared
... lack of coffee probably.
>
>> + */
>> +bool pxa_udc_probe_dt(struct platform_device *pdev, struct pxa_udc *udc)
>> +{
>> +	struct device_node *np = pdev->dev.of_node;
>> +	const struct of_device_id *of_id =
>> +			of_match_device(udc_pxa_dt_ids, &pdev->dev);
>> +	int ret;
>> +	u32 gpio_pullup;
>> +
>> +	if (!np || !of_id)
>> +		return 1;
>> +	ret = of_alias_get_id(np, "udc");
>> +	pdev->id = (ret >= 0) ? ret : -1;
>
> The alias name wasn't mentioned in the binding...
Ah, now I'm thinking of it, it doesn't serve any purpose ... I will remove this
piece of code and replace by "pdev->id = -1".

>
>> +
>> +	if (!of_property_read_u32(np, "udc-pullup-gpio", &gpio_pullup))
>> +		udc->gpio_pullup = gpio_pullup;
>
> This number is a Linux internal detail. Use the GPIO bindings.
Yes, as in documentation. Agreed.
>
>> +	udc->gpio_pullup_inverted =
>> +		of_property_read_bool(np, "udc-pullup-gpio-inverted");
>
> The GPIO bindings can describe this.
Yes.

>
>> @@ -2415,7 +2469,13 @@ static int pxa_udc_probe(struct platform_device *pdev)
>>  {
>>  	struct resource *regs;
>>  	struct pxa_udc *udc = &memory;
>> -	int retval = 0, gpio;
>> +	int retval = 0;
>> +
>> +	retval = pxa_udc_probe_dt(pdev, udc);
>> +	if (retval < 0)
>> +		return retval;
>
> This case can never happen given the prototype of pxa_udc_probe_dt.
>
>> @@ -2509,6 +2574,7 @@ static int pxa_udc_remove(struct platform_device *_dev)
>>  
>>  	udc->transceiver = NULL;
>>  	the_controller = NULL;
>> +	clk_unprepare(udc->clk);
>>  	clk_put(udc->clk);
>
> I don't see why these clock changes should be in the same patch as the
> DT support. They might be a prerequisite, but as far as I can tell they
> are required even without DT probing.
Ah they are another posted patch. I think I missed a rebase somewhere, and it
slipped in. It is as you say not this patch purpose, and I thought I had posted
a previous patch with this ... I will split it again.

Thanks for the review.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html