Re: [PATCH v2 1/3] usb: gadget: pxa27x_udc: add devicetree support
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.jarz...@free.fr
Cc: devicet...@vger.kernel.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 majord...@vger.kernel.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 mark.rutl...@arm.com 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/3] usb: gadget: pxa27x_udc: add devicetree support
Add support for device-tree device discovery. If devicetree is not
provided, fallback to legacy platform data discovery.
Signed-off-by: Robert Jarzmik robert.jarz...@free.fr
Cc: devicet...@vger.kernel.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
