Those 2 patches are adding the USB high speed
support to the at91sam9g45 SOC family.
They are relying on the integration of at91sam9g45
already posted. Here is the patch series that you
will have to take into account before applying
those patches.
clock.c changes:
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=5438/1
9g45 introduction:
http://lkml.org/lkml/2009/6/4/77
http://lkml.org/lkml/2009/6/4/76
http://lkml.org/lkml/2009/6/4/78
atmel_usba tiny modification (bias function only for 9rl):
http://lkml.org/lkml/2009/6/5/355
arch/arm/mach-at91/at91sam9g45_devices.c | 56 +++++
arch/arm/mach-at91/board-sam9m10g45ek.c | 1 +
arch/arm/mach-at91/include/mach/board.h | 1 +
arch/arm/tools/mach-types | 1 +
drivers/usb/Kconfig | 1 +
drivers/usb/gadget/Kconfig | 4 +-
drivers/usb/host/ehci-atmel.c | 251 ++++++++++++++++++++
drivers/usb/host/ehci-hcd.c | 5 +
drivers/usb/host/ohci-at91.c | 3 +-
9 files changed, 320 insertions(+), 3 deletions(-)
create mode 100644 drivers/usb/host/ehci-atmel.c
Add both host and gadget USB drivers for at91sam9g45 series. Those SOC embed
high speed USB interfaces.
The gadget driver is the already available atmel_usba_udc.
The host driver is an EHCI with its companion OHCI. EHCI is handled by the new
ehci-atmel.c whereas the OHCI is always handled by ohci-at91.c. This last
wrapper is modified to allow IRQ sharing between two controllers.
Signed-off-by: Nicolas Ferre <[email protected]>
---
drivers/usb/Kconfig | 1 +
drivers/usb/gadget/Kconfig | 4 +-
drivers/usb/host/ehci-atmel.c | 251 +++++++++++++++++++++++++++++++++++++++++
drivers/usb/host/ehci-hcd.c | 5 +
drivers/usb/host/ohci-at91.c | 3 +-
5 files changed, 261 insertions(+), 3 deletions(-)
create mode 100644 drivers/usb/host/ehci-atmel.c
diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
index c6c816b..3974c9c 100644
--- a/drivers/usb/Kconfig
+++ b/drivers/usb/Kconfig
@@ -57,6 +57,7 @@ config USB_ARCH_HAS_EHCI
default y if PPC_83xx
default y if SOC_AU1200
default y if ARCH_IXP4XX
+ default y if ARCH_AT91SAM9G45
default PCI
# ARM SA1111 chips have a non-PCI based "OHCI-compatible" USB host interface.
diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 080bb1e..9beea52 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -124,7 +124,7 @@ choice
config USB_GADGET_AT91
boolean "Atmel AT91 USB Device Port"
- depends on ARCH_AT91 && !ARCH_AT91SAM9RL && !ARCH_AT91CAP9
+ depends on ARCH_AT91 && !ARCH_AT91SAM9RL && !ARCH_AT91CAP9 && !ARCH_AT91SAM9G45
select USB_GADGET_SELECTED
help
Many Atmel AT91 processors (such as the AT91RM2000) have a
@@ -143,7 +143,7 @@ config USB_AT91
config USB_GADGET_ATMEL_USBA
boolean "Atmel USBA"
select USB_GADGET_DUALSPEED
- depends on AVR32 || ARCH_AT91CAP9 || ARCH_AT91SAM9RL
+ depends on AVR32 || ARCH_AT91CAP9 || ARCH_AT91SAM9RL || ARCH_AT91SAM9G45
help
USBA is the integrated high-speed USB Device controller on
the AT32AP700x, some AT91SAM9 and AT91CAP9 processors from Atmel.
diff --git a/drivers/usb/host/ehci-atmel.c b/drivers/usb/host/ehci-atmel.c
new file mode 100644
index 0000000..c5f2303
--- /dev/null
+++ b/drivers/usb/host/ehci-atmel.c
@@ -0,0 +1,251 @@
+/*
+ * Driver for EHCI UHP on Atmel chips
+ *
+ * Copyright (C) 2009 Atmel Corporation,
+ * Nicolas Ferre <[email protected]>
+ *
+ * Based on various ehci-*.c drivers
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License. See the file COPYING in the main directory of this archive for
+ * more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/platform_device.h>
+
+/* interface and function clocks */
+static struct clk *iclk, *fclk;
+static int clocked;
+
+/*-------------------------------------------------------------------------*/
+
+static void atmel_start_clock(void)
+{
+ clk_enable(iclk);
+ clk_enable(fclk);
+ clocked = 1;
+}
+
+static void atmel_stop_clock(void)
+{
+ clk_disable(fclk);
+ clk_disable(iclk);
+ clocked = 0;
+}
+
+static void atmel_start_ehci(struct platform_device *pdev)
+{
+
+ dev_dbg(&pdev->dev, "start\n");
+
+ /*
+ * Start the USB clocks.
+ */
+ atmel_start_clock();
+}
+
+static void atmel_stop_ehci(struct platform_device *pdev)
+{
+ dev_dbg(&pdev->dev, "stop\n");
+
+ /*
+ * Stop the USB clocks.
+ */
+ atmel_stop_clock();
+}
+
+/*-------------------------------------------------------------------------*/
+
+static int ehci_atmel_setup(struct usb_hcd *hcd)
+{
+ struct ehci_hcd *ehci = hcd_to_ehci(hcd);
+ int retval = 0;
+
+ /*
+ * registers start at offset 0x0
+ */
+ ehci->caps = hcd->regs;
+ ehci->regs = hcd->regs +
+ HC_LENGTH(ehci_readl(ehci, &ehci->caps->hc_capbase));
+ dbg_hcs_params(ehci, "reset");
+ dbg_hcc_params(ehci, "reset");
+
+ /*
+ * cache this readonly data; minimize chip reads
+ */
+ ehci->hcs_params = ehci_readl(ehci, &ehci->caps->hcs_params);
+
+ retval = ehci_halt(ehci);
+ if (retval)
+ return retval;
+
+ /*
+ * data structure init
+ */
+ retval = ehci_init(hcd);
+ if (retval)
+ return retval;
+
+ ehci->sbrn = 0x20;
+
+ ehci_reset(ehci);
+ ehci_port_power(ehci, 0);
+
+ return retval;
+}
+
+static const struct hc_driver ehci_atmel_hc_driver = {
+ .description = hcd_name,
+ .product_desc = "Atmel EHCI UHP HS",
+ .hcd_priv_size = sizeof(struct ehci_hcd),
+
+ /*
+ * generic hardware linkage
+ */
+ .irq = ehci_irq,
+ .flags = HCD_MEMORY | HCD_USB2,
+
+ /*
+ * basic lifecycle operations
+ */
+ .reset = ehci_atmel_setup,
+ .start = ehci_run,
+ .stop = ehci_stop,
+ .shutdown = ehci_shutdown,
+
+ /*
+ * managing i/o requests and associated device resources
+ */
+ .urb_enqueue = ehci_urb_enqueue,
+ .urb_dequeue = ehci_urb_dequeue,
+ .endpoint_disable = ehci_endpoint_disable,
+
+ /*
+ * scheduling support
+ */
+ .get_frame_number = ehci_get_frame,
+
+ /*
+ * root hub support
+ */
+ .hub_status_data = ehci_hub_status_data,
+ .hub_control = ehci_hub_control,
+ .bus_suspend = ehci_bus_suspend,
+ .bus_resume = ehci_bus_resume,
+ .relinquish_port = ehci_relinquish_port,
+ .port_handed_over = ehci_port_handed_over,
+};
+
+static int __init ehci_atmel_drv_probe(struct platform_device *pdev)
+{
+ struct usb_hcd *hcd;
+ const struct hc_driver *driver = &ehci_atmel_hc_driver;
+ struct resource *res;
+ int irq;
+ int retval;
+
+ if (usb_disabled())
+ return -ENODEV;
+
+ pr_debug("Initializing Atmel-SoC USB Host Controller\n");
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq <= 0) {
+ dev_err(&pdev->dev,
+ "Found HC with no IRQ. Check %s setup!\n",
+ dev_name(&pdev->dev));
+ retval = -ENODEV;
+ goto fail_create_hcd;
+ }
+
+ hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
+ if (!hcd) {
+ retval = -ENOMEM;
+ goto fail_create_hcd;
+ }
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
+ dev_err(&pdev->dev,
+ "Found HC with no register addr. Check %s setup!\n",
+ dev_name(&pdev->dev));
+ retval = -ENODEV;
+ goto fail_request_resource;
+ }
+ hcd->rsrc_start = res->start;
+ hcd->rsrc_len = res->end - res->start + 1;
+
+ if (!request_mem_region(hcd->rsrc_start, hcd->rsrc_len,
+ driver->description)) {
+ dev_dbg(&pdev->dev, "controller already in use\n");
+ retval = -EBUSY;
+ goto fail_request_resource;
+ }
+
+ hcd->regs = ioremap_nocache(hcd->rsrc_start, hcd->rsrc_len);
+ if (hcd->regs == NULL) {
+ dev_dbg(&pdev->dev, "error mapping memory\n");
+ retval = -EFAULT;
+ goto fail_ioremap;
+ }
+
+ iclk = clk_get(&pdev->dev, "ehci_clk");
+ fclk = clk_get(&pdev->dev, "uhpck");
+ if (IS_ERR(iclk) || IS_ERR(fclk)) {
+ dev_err(&pdev->dev, "Error getting clocks\n");
+ retval = -ENOENT;
+ goto fail_get_clk;
+ }
+
+ atmel_start_ehci(pdev);
+
+ retval = usb_add_hcd(hcd, irq, IRQF_SHARED);
+ if (retval)
+ goto fail_add_hcd;
+
+ return retval;
+
+fail_add_hcd:
+ atmel_stop_ehci(pdev);
+fail_get_clk:
+ clk_put(fclk);
+ clk_put(iclk);
+ iounmap(hcd->regs);
+fail_ioremap:
+ release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
+fail_request_resource:
+ usb_put_hcd(hcd);
+fail_create_hcd:
+ dev_err(&pdev->dev, "init %s fail, %d\n",
+ dev_name(&pdev->dev), retval);
+
+ return retval;
+}
+
+static int __exit ehci_atmel_drv_remove(struct platform_device *pdev)
+{
+ struct usb_hcd *hcd = platform_get_drvdata(pdev);
+
+ ehci_shutdown(hcd);
+ usb_remove_hcd(hcd);
+ iounmap(hcd->regs);
+ release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
+ usb_put_hcd(hcd);
+
+ atmel_stop_ehci(pdev);
+ clk_put(fclk);
+ clk_put(iclk);
+ fclk = iclk = NULL;
+
+ return 0;
+}
+
+MODULE_ALIAS("platform:atmel-ehci");
+
+static struct platform_driver ehci_atmel_driver = {
+ .probe = ehci_atmel_drv_probe,
+ .remove = __exit_p(ehci_atmel_drv_remove),
+ .shutdown = usb_hcd_platform_shutdown,
+ .driver.name = "atmel-ehci",
+};
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index c637207..df2ddee 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1072,6 +1072,11 @@ MODULE_LICENSE ("GPL");
#define PLATFORM_DRIVER ixp4xx_ehci_driver
#endif
+#ifdef CONFIG_ARCH_AT91
+#include "ehci-atmel.c"
+#define PLATFORM_DRIVER ehci_atmel_driver
+#endif
+
#if !defined(PCI_DRIVER) && !defined(PLATFORM_DRIVER) && \
!defined(PS3_SYSTEM_BUS_DRIVER) && !defined(OF_PLATFORM_DRIVER)
#error "missing bus glue for ehci-hcd"
diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c
index bb5e6f6..b29b0fe 100644
--- a/drivers/usb/host/ohci-at91.c
+++ b/drivers/usb/host/ohci-at91.c
@@ -148,7 +148,8 @@ static int usb_hcd_at91_probe(const struct hc_driver *driver,
at91_start_hc(pdev);
ohci_hcd_init(hcd_to_ohci(hcd));
- retval = usb_add_hcd(hcd, pdev->resource[1].start, IRQF_DISABLED);
+ retval = usb_add_hcd(hcd, pdev->resource[1].start,
+ IRQF_DISABLED | IRQF_SHARED);
if (retval == 0)
return retval;
--
1.5.3.7
This is the at91 specific part of USB host integration. The EHCI high speed
controller has a companion OHCI controller to manage USB full and low speed.
They are sharing the same IRQ line and vbus pin.
Signed-off-by: Nicolas Ferre <[email protected]>
---
arch/arm/mach-at91/at91sam9g45_devices.c | 56 ++++++++++++++++++++++++++++++
arch/arm/mach-at91/board-sam9m10g45ek.c | 1 +
arch/arm/mach-at91/include/mach/board.h | 1 +
3 files changed, 58 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
index d746e86..c2ecbb6 100644
--- a/arch/arm/mach-at91/at91sam9g45_devices.c
+++ b/arch/arm/mach-at91/at91sam9g45_devices.c
@@ -83,6 +83,62 @@ void __init at91_add_device_usbh_ohci(struct at91_usbh_data *data) {}
/* --------------------------------------------------------------------
+ * USB Host HS (EHCI)
+ * Needs an OHCI host for low and full speed management
+ * -------------------------------------------------------------------- */
+
+#if defined(CONFIG_USB_EHCI_HCD) || defined(CONFIG_USB_EHCI_HCD_MODULE)
+static u64 ehci_dmamask = DMA_BIT_MASK(32);
+static struct at91_usbh_data usbh_ehci_data;
+
+static struct resource usbh_ehci_resources[] = {
+ [0] = {
+ .start = AT91SAM9G45_EHCI_BASE,
+ .end = AT91SAM9G45_EHCI_BASE + SZ_1M - 1,
+ .flags = IORESOURCE_MEM,
+ },
+ [1] = {
+ .start = AT91SAM9G45_ID_UHPHS,
+ .end = AT91SAM9G45_ID_UHPHS,
+ .flags = IORESOURCE_IRQ,
+ },
+};
+
+static struct platform_device at91_usbh_ehci_device = {
+ .name = "atmel-ehci",
+ .id = -1,
+ .dev = {
+ .dma_mask = &ehci_dmamask,
+ .coherent_dma_mask = DMA_BIT_MASK(32),
+ .platform_data = &usbh_ehci_data,
+ },
+ .resource = usbh_ehci_resources,
+ .num_resources = ARRAY_SIZE(usbh_ehci_resources),
+};
+
+void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data)
+{
+ int i;
+
+ if (!data)
+ return;
+
+ /* Enable VBus control for UHP ports */
+ for (i = 0; i < data->ports; i++) {
+ if (data->vbus_pin[i])
+ at91_set_gpio_output(data->vbus_pin[i], 0);
+ }
+
+ usbh_ehci_data = *data;
+ at91_clock_associate("uhphs_clk", &at91_usbh_ehci_device.dev, "ehci_clk");
+ platform_device_register(&at91_usbh_ehci_device);
+}
+#else
+void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data) {}
+#endif
+
+
+/* --------------------------------------------------------------------
* USB HS Device (Gadget)
* -------------------------------------------------------------------- */
diff --git a/arch/arm/mach-at91/board-sam9m10g45ek.c b/arch/arm/mach-at91/board-sam9m10g45ek.c
index b8558ea..d7251b7 100644
--- a/arch/arm/mach-at91/board-sam9m10g45ek.c
+++ b/arch/arm/mach-at91/board-sam9m10g45ek.c
@@ -358,6 +358,7 @@ static void __init ek_board_init(void)
at91_add_device_serial();
/* USB HS Host */
at91_add_device_usbh_ohci(&ek_usbh_hs_data);
+ at91_add_device_usbh_ehci(&ek_usbh_hs_data);
/* USB HS Device */
at91_add_device_usba(&ek_usba_udc_data);
/* SPI */
diff --git a/arch/arm/mach-at91/include/mach/board.h b/arch/arm/mach-at91/include/mach/board.h
index 74801d2..13640b0 100644
--- a/arch/arm/mach-at91/include/mach/board.h
+++ b/arch/arm/mach-at91/include/mach/board.h
@@ -92,6 +92,7 @@ struct at91_usbh_data {
};
extern void __init at91_add_device_usbh(struct at91_usbh_data *data);
extern void __init at91_add_device_usbh_ohci(struct at91_usbh_data *data);
+extern void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data);
/* NAND / SmartMedia */
struct atmel_nand_data {
--
1.5.3.7
Hi,
Ryan Mallon :
> Nicolas Ferre wrote:
>> Add both host and gadget USB drivers for at91sam9g45 series. Those SOC embed
>> high speed USB interfaces.
>> The gadget driver is the already available atmel_usba_udc.
>> The host driver is an EHCI with its companion OHCI. EHCI is handled by the new
>> ehci-atmel.c whereas the OHCI is always handled by ohci-at91.c. This last
>> wrapper is modified to allow IRQ sharing between two controllers.
>>
>> Signed-off-by: Nicolas Ferre <[email protected]>
>
> Some comments below.
Thanks a lot for them.
>> ---
>> diff --git a/drivers/usb/host/ehci-atmel.c b/drivers/usb/host/ehci-atmel.c
>> new file mode 100644
>> index 0000000..c5f2303
>> --- /dev/null
>> +++ b/drivers/usb/host/ehci-atmel.c
>> @@ -0,0 +1,251 @@
>> +/*
>> + * Driver for EHCI UHP on Atmel chips
>> + *
>> + * Copyright (C) 2009 Atmel Corporation,
>> + * Nicolas Ferre <[email protected]>
>> + *
>> + * Based on various ehci-*.c drivers
>> + *
>> + * This file is subject to the terms and conditions of the GNU General Public
>> + * License. See the file COPYING in the main directory of this archive for
>> + * more details.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/platform_device.h>
>> +
>> +/* interface and function clocks */
>> +static struct clk *iclk, *fclk;
>> +static int clocked;
>> +
>> +/*-------------------------------------------------------------------------*/
>> +
>> +static void atmel_start_clock(void)
>> +{
>> + clk_enable(iclk);
>> + clk_enable(fclk);
>> + clocked = 1;
>> +}
>> +
>> +static void atmel_stop_clock(void)
>> +{
>> + clk_disable(fclk);
>> + clk_disable(iclk);
>> + clocked = 0;
>> +}
>> +
>> +static void atmel_start_ehci(struct platform_device *pdev)
>> +{
>> +
>> + dev_dbg(&pdev->dev, "start\n");
>> +
>> + /*
>> + * Start the USB clocks.
>> + */
>> + atmel_start_clock();
>> +}
>
> The clocked variable is never used outside of the start/stop functions.
> How come you have separate functions, why not just:
>
> static void atmel_start_ehci(struct platform_device *pdev)
> {
> dev_dbg(&pdev->dev, "start\n");
> clk_enable(iclk);
> clk_enable(fclk);
> }
>
> and similarly for the atmel_stop_ehci function. Also, currently these
> are only called from the probe/remove functions, so you could just move
> the clock enable/disable into those, or have you separated these out so
> that can eventually be called from pm_suspend/resume functions?
I must say that this creation of clock management function comes from an
habit of sharing drivers between AVR32 and AT91; moreover between
different kind of AT91 devices that share different clock management
schemes (Cf. at91sam9261 vs other sam9 chips).
Experience has taught me that separating clock management in a single
function set ease things.
You are right also that this may be useful for power management.
For all this I prefer to keep them.
>> +
>> +static void atmel_stop_ehci(struct platform_device *pdev)
>> +{
>> + dev_dbg(&pdev->dev, "stop\n");
>> +
>> + /*
>> + * Stop the USB clocks.
>> + */
>> + atmel_stop_clock();
>> +}
>> +
>> +/*-------------------------------------------------------------------------*/
>
> Probably don't need this comment.
Totally agree. Those start/stop comments are useless.
>> +
>> +static int ehci_atmel_setup(struct usb_hcd *hcd)
>> +{
>> + struct ehci_hcd *ehci = hcd_to_ehci(hcd);
>> + int retval = 0;
>> +
>> + /*
>> + * registers start at offset 0x0
>> + */
>> + ehci->caps = hcd->regs;
>> + ehci->regs = hcd->regs +
>> + HC_LENGTH(ehci_readl(ehci, &ehci->caps->hc_capbase));
>> + dbg_hcs_params(ehci, "reset");
>> + dbg_hcc_params(ehci, "reset");
>> +
>> + /*
>> + * cache this readonly data; minimize chip reads
>> + */
>> + ehci->hcs_params = ehci_readl(ehci, &ehci->caps->hcs_params);
>> +
>> + retval = ehci_halt(ehci);
>> + if (retval)
>> + return retval;
>> +
>> + /*
>> + * data structure init
>> + */
>> + retval = ehci_init(hcd);
>> + if (retval)
>> + return retval;
>> +
>> + ehci->sbrn = 0x20;
>> +
>> + ehci_reset(ehci);
>> + ehci_port_power(ehci, 0);
>> +
>> + return retval;
>> +}
>> +
>> +static const struct hc_driver ehci_atmel_hc_driver = {
>> + .description = hcd_name,
>> + .product_desc = "Atmel EHCI UHP HS",
>> + .hcd_priv_size = sizeof(struct ehci_hcd),
>> +
>> + /*
>> + * generic hardware linkage
>> + */
>
> Nitpick: put single line comments on single lines.
Ok.
>> + .irq = ehci_irq,
>> + .flags = HCD_MEMORY | HCD_USB2,
>> +
>> + /*
>> + * basic lifecycle operations
>> + */
>> + .reset = ehci_atmel_setup,
>> + .start = ehci_run,
>> + .stop = ehci_stop,
>> + .shutdown = ehci_shutdown,
>> +
>> + /*
>> + * managing i/o requests and associated device resources
>> + */
>> + .urb_enqueue = ehci_urb_enqueue,
>> + .urb_dequeue = ehci_urb_dequeue,
>> + .endpoint_disable = ehci_endpoint_disable,
>> +
>> + /*
>> + * scheduling support
>> + */
>> + .get_frame_number = ehci_get_frame,
>> +
>> + /*
>> + * root hub support
>> + */
>> + .hub_status_data = ehci_hub_status_data,
>> + .hub_control = ehci_hub_control,
>> + .bus_suspend = ehci_bus_suspend,
>> + .bus_resume = ehci_bus_resume,
>> + .relinquish_port = ehci_relinquish_port,
>> + .port_handed_over = ehci_port_handed_over,
>> +};
>> +
>> +static int __init ehci_atmel_drv_probe(struct platform_device *pdev)
>> +{
>> + struct usb_hcd *hcd;
>> + const struct hc_driver *driver = &ehci_atmel_hc_driver;
>> + struct resource *res;
>> + int irq;
>> + int retval;
>> +
>> + if (usb_disabled())
>> + return -ENODEV;
>> +
>> + pr_debug("Initializing Atmel-SoC USB Host Controller\n");
>> +
>> + irq = platform_get_irq(pdev, 0);
>> + if (irq <= 0) {
>> + dev_err(&pdev->dev,
>> + "Found HC with no IRQ. Check %s setup!\n",
>> + dev_name(&pdev->dev));
>> + retval = -ENODEV;
>> + goto fail_create_hcd;
>> + }
>> +
>> + hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
>> + if (!hcd) {
>> + retval = -ENOMEM;
>> + goto fail_create_hcd;
>> + }
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res) {
>> + dev_err(&pdev->dev,
>> + "Found HC with no register addr. Check %s setup!\n",
>> + dev_name(&pdev->dev));
>> + retval = -ENODEV;
>> + goto fail_request_resource;
>> + }
>> + hcd->rsrc_start = res->start;
>> + hcd->rsrc_len = res->end - res->start + 1;
>> +
>> + if (!request_mem_region(hcd->rsrc_start, hcd->rsrc_len,
>> + driver->description)) {
>> + dev_dbg(&pdev->dev, "controller already in use\n");
>> + retval = -EBUSY;
>> + goto fail_request_resource;
>> + }
>> +
>> + hcd->regs = ioremap_nocache(hcd->rsrc_start, hcd->rsrc_len);
>> + if (hcd->regs == NULL) {
>> + dev_dbg(&pdev->dev, "error mapping memory\n");
>> + retval = -EFAULT;
>> + goto fail_ioremap;
>> + }
>> +
>> + iclk = clk_get(&pdev->dev, "ehci_clk");
>> + fclk = clk_get(&pdev->dev, "uhpck");
>> + if (IS_ERR(iclk) || IS_ERR(fclk)) {
>> + dev_err(&pdev->dev, "Error getting clocks\n");
>> + retval = -ENOENT;
>> + goto fail_get_clk;
>> + }
>
> This doesn't seem right. If one or neither of the clk_gets succeed here
> then you will clk_put a clock which you never got in fail_get_clk. You
> should probably get the two clocks individually an have separate error
> paths for both.
Ok.
Thanks, I will post a modified patch soon.
Best regards,
--
Nicolas Ferre
On Tuesday 09 June 2009, Nicolas Ferre wrote:
> This is the at91 specific part of USB host integration. The EHCI high speed
> controller has a companion OHCI controller to manage USB full and low speed.
> They are sharing the same IRQ line and vbus pin.
>
> Signed-off-by: Nicolas Ferre <[email protected]>
My only issue here is with the GPIO stuff; see below.
> ---
> arch/arm/mach-at91/at91sam9g45_devices.c | 56 ++++++++++++++++++++++++++++++
> arch/arm/mach-at91/board-sam9m10g45ek.c | 1 +
> arch/arm/mach-at91/include/mach/board.h | 1 +
> 3 files changed, 58 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
> index d746e86..c2ecbb6 100644
> --- a/arch/arm/mach-at91/at91sam9g45_devices.c
> +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
> @@ -83,6 +83,62 @@ void __init at91_add_device_usbh_ohci(struct at91_usbh_data *data) {}
>
>
> /* --------------------------------------------------------------------
> + * USB Host HS (EHCI)
> + * Needs an OHCI host for low and full speed management
> + * -------------------------------------------------------------------- */
> +
> +#if defined(CONFIG_USB_EHCI_HCD) || defined(CONFIG_USB_EHCI_HCD_MODULE)
> +static u64 ehci_dmamask = DMA_BIT_MASK(32);
> +static struct at91_usbh_data usbh_ehci_data;
> +
> +static struct resource usbh_ehci_resources[] = {
> + [0] = {
> + .start = AT91SAM9G45_EHCI_BASE,
> + .end = AT91SAM9G45_EHCI_BASE + SZ_1M - 1,
> + .flags = IORESOURCE_MEM,
> + },
> + [1] = {
> + .start = AT91SAM9G45_ID_UHPHS,
> + .end = AT91SAM9G45_ID_UHPHS,
> + .flags = IORESOURCE_IRQ,
> + },
> +};
> +
> +static struct platform_device at91_usbh_ehci_device = {
> + .name = "atmel-ehci",
> + .id = -1,
> + .dev = {
> + .dma_mask = &ehci_dmamask,
> + .coherent_dma_mask = DMA_BIT_MASK(32),
> + .platform_data = &usbh_ehci_data,
> + },
> + .resource = usbh_ehci_resources,
> + .num_resources = ARRAY_SIZE(usbh_ehci_resources),
> +};
> +
> +void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data)
> +{
> + int i;
> +
> + if (!data)
> + return;
> +
> + /* Enable VBus control for UHP ports */
> + for (i = 0; i < data->ports; i++) {
> + if (data->vbus_pin[i])
> + at91_set_gpio_output(data->vbus_pin[i], 0);
This should gpio_request() and gpio_direction_output().
Don't use AT91-specific GPIO calls except for things that
the generic calls don't support ... like enabling open-drain
outputs, the deglitching support, or input pullups.
> + }
> +
> + usbh_ehci_data = *data;
> + at91_clock_associate("uhphs_clk", &at91_usbh_ehci_device.dev, "ehci_clk");
> + platform_device_register(&at91_usbh_ehci_device);
> +}
> +#else
> +void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data) {}
> +#endif
> +
> +
> +/* --------------------------------------------------------------------
> * USB HS Device (Gadget)
> * -------------------------------------------------------------------- */
>
> diff --git a/arch/arm/mach-at91/board-sam9m10g45ek.c b/arch/arm/mach-at91/board-sam9m10g45ek.c
> index b8558ea..d7251b7 100644
> --- a/arch/arm/mach-at91/board-sam9m10g45ek.c
> +++ b/arch/arm/mach-at91/board-sam9m10g45ek.c
> @@ -358,6 +358,7 @@ static void __init ek_board_init(void)
> at91_add_device_serial();
> /* USB HS Host */
> at91_add_device_usbh_ohci(&ek_usbh_hs_data);
> + at91_add_device_usbh_ehci(&ek_usbh_hs_data);
> /* USB HS Device */
> at91_add_device_usba(&ek_usba_udc_data);
> /* SPI */
> diff --git a/arch/arm/mach-at91/include/mach/board.h b/arch/arm/mach-at91/include/mach/board.h
> index 74801d2..13640b0 100644
> --- a/arch/arm/mach-at91/include/mach/board.h
> +++ b/arch/arm/mach-at91/include/mach/board.h
> @@ -92,6 +92,7 @@ struct at91_usbh_data {
> };
> extern void __init at91_add_device_usbh(struct at91_usbh_data *data);
> extern void __init at91_add_device_usbh_ohci(struct at91_usbh_data *data);
> +extern void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data);
>
> /* NAND / SmartMedia */
> struct atmel_nand_data {
> --
> 1.5.3.7
>
>
David Brownell wrote:
> > --- a/arch/arm/mach-at91/at91sam9g45_devices.c
> > +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
> > + /* Enable VBus control for UHP ports */
> > + for (i = 0; i < data->ports; i++) {
> > + if (data->vbus_pin[i])
> > + at91_set_gpio_output(data->vbus_pin[i], 0);
>
> This should gpio_request() and gpio_direction_output().
Hmm...I thought the driver was supposed to call gpio_request(), not the
platform code?
> Don't use AT91-specific GPIO calls except for things that
> the generic calls don't support ... like enabling open-drain
> outputs, the deglitching support, or input pullups.
This call does port configuration, which you convinced me a long time
ago was a fundamentally different thing from GPIO. If the pin really
requires one of those features, this would definitely be the place to
set it up.
Haavard
On Friday 19 June 2009, Haavard Skinnemoen wrote:
> David Brownell wrote:
> > > --- a/arch/arm/mach-at91/at91sam9g45_devices.c
> > > +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
>
> > > + /* Enable VBus control for UHP ports */
> > > + for (i = 0; i < data->ports; i++) {
> > > + if (data->vbus_pin[i])
> > > + at91_set_gpio_output(data->vbus_pin[i], 0);
> >
> > This should gpio_request() and gpio_direction_output().
>
> Hmm...I thought the driver was supposed to call gpio_request(), not the
> platform code?
In some cases. This isn't a good case for that. Especially
if it's going to call gpio_direction_output() ... which needs
gpio_request() to have been done first.
> > Don't use AT91-specific GPIO calls except for things that
> > the generic calls don't support ... like enabling open-drain
> > outputs, the deglitching support, or input pullups.
>
> This call does port configuration, which you convinced me a long time
> ago was a fundamentally different thing from GPIO.
Yes, pin/port config is certainly part of what the platform's
code to set up devices should handle. That can include making
sure a given pin is configured as a GPIO ... and in the normal
case where it's dedicated to that task, it simplifies the driver
to have it pre-allocated and configured for I/O/both.
I'm pulling in some discussion from a different email thread
earlier, which proposed doing the right thing and finally
getting rid of the at91-specific GPIO calls except in the few
cases they could not be avoided.
It might be that AT91 needs to add some pin config calls which
resemble what you did for AT32AP7 chips.
- Dave
> If the pin really
> requires one of those features, this would definitely be the place to
> set it up.
>
> Haavard
>
>
Hi,
I come back on this topic as at91sam9g45 is now in mainline. I need this
integration code to have the USB EHCI work on the evaluation kit.
David Brownell :
> On Friday 19 June 2009, Haavard Skinnemoen wrote:
>> David Brownell wrote:
>>>> --- a/arch/arm/mach-at91/at91sam9g45_devices.c
>>>> +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
>>>> + /* Enable VBus control for UHP ports */
>>>> + for (i = 0; i < data->ports; i++) {
>>>> + if (data->vbus_pin[i])
>>>> + at91_set_gpio_output(data->vbus_pin[i], 0);
>>> This should gpio_request() and gpio_direction_output().
>> Hmm...I thought the driver was supposed to call gpio_request(), not the
>> platform code?
>
> In some cases. This isn't a good case for that. Especially
> if it's going to call gpio_direction_output() ... which needs
> gpio_request() to have been done first.
Ok, I am building a patch on top of this one that uses these calls. This
way I can change both vbus pin configuration: OCHI and EHCI.
Best regards,
--
Nicolas Ferre
Change pin configuration for USB vbus on at91sam9g45: use the generic gpiolib
call instead of the at91 specific one.
Use gpio_request() function with same identifier for OHCI and EHCI hosts as
they are sharing the same pin.
Signed-off-by: Nicolas Ferre <[email protected]>
---
This patch is on top of at91sam9g45 USB integration one:
"[PATCH 2/2] at91/USB: at91sam9g45 series USB host integration"
http://lkml.org/lkml/2009/6/9/221
arch/arm/mach-at91/at91sam9g45_devices.c | 12 ++++++++----
1 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
index 5be8cf2..7d939c0 100644
--- a/arch/arm/mach-at91/at91sam9g45_devices.c
+++ b/arch/arm/mach-at91/at91sam9g45_devices.c
@@ -118,8 +118,10 @@ void __init at91_add_device_usbh_ohci(struct at91_usbh_data *data)
/* Enable VBus control for UHP ports */
for (i = 0; i < data->ports; i++) {
- if (data->vbus_pin[i])
- at91_set_gpio_output(data->vbus_pin[i], 0);
+ if (data->vbus_pin[i]) {
+ gpio_request(data->vbus_pin[i], "usb host vbus");
+ gpio_direction_output(data->vbus_pin[i], 0);
+ }
}
usbh_ohci_data = *data;
@@ -173,8 +175,10 @@ void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data)
/* Enable VBus control for UHP ports */
for (i = 0; i < data->ports; i++) {
- if (data->vbus_pin[i])
- at91_set_gpio_output(data->vbus_pin[i], 0);
+ if (data->vbus_pin[i]) {
+ gpio_request(data->vbus_pin[i], "usb host vbus");
+ gpio_direction_output(data->vbus_pin[i], 0);
+ }
}
usbh_ehci_data = *data;
--
1.5.6.5
On 19:29 Wed 16 Sep , Nicolas Ferre wrote:
> Change pin configuration for USB vbus on at91sam9g45: use the generic gpiolib
> call instead of the at91 specific one.
> Use gpio_request() function with same identifier for OHCI and EHCI hosts as
> they are sharing the same pin.
>
> Signed-off-by: Nicolas Ferre <[email protected]>
> ---
> This patch is on top of at91sam9g45 USB integration one:
> "[PATCH 2/2] at91/USB: at91sam9g45 series USB host integration"
> http://lkml.org/lkml/2009/6/9/221
>
> arch/arm/mach-at91/at91sam9g45_devices.c | 12 ++++++++----
> 1 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
> index 5be8cf2..7d939c0 100644
> --- a/arch/arm/mach-at91/at91sam9g45_devices.c
> +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
> @@ -118,8 +118,10 @@ void __init at91_add_device_usbh_ohci(struct at91_usbh_data *data)
>
> /* Enable VBus control for UHP ports */
> for (i = 0; i < data->ports; i++) {
> - if (data->vbus_pin[i])
> - at91_set_gpio_output(data->vbus_pin[i], 0);
> + if (data->vbus_pin[i]) {
> + gpio_request(data->vbus_pin[i], "usb host vbus");
> + gpio_direction_output(data->vbus_pin[i], 0);
> + }
> }
>
> usbh_ohci_data = *data;
> @@ -173,8 +175,10 @@ void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data)
>
> /* Enable VBus control for UHP ports */
> for (i = 0; i < data->ports; i++) {
> - if (data->vbus_pin[i])
> - at91_set_gpio_output(data->vbus_pin[i], 0);
> + if (data->vbus_pin[i]) {
> + gpio_request(data->vbus_pin[i], "usb host vbus");
> + gpio_direction_output(data->vbus_pin[i], 0);
> + }
> }
as you do the same think for ehci & ohci why not factorize it?
Best Regards,
J.
Jean-Christophe PLAGNIOL-VILLARD :
> On 19:29 Wed 16 Sep , Nicolas Ferre wrote:
>> Change pin configuration for USB vbus on at91sam9g45: use the generic gpiolib
>> call instead of the at91 specific one.
>> Use gpio_request() function with same identifier for OHCI and EHCI hosts as
>> they are sharing the same pin.
>>
>> Signed-off-by: Nicolas Ferre <[email protected]>
>> ---
>> This patch is on top of at91sam9g45 USB integration one:
>> "[PATCH 2/2] at91/USB: at91sam9g45 series USB host integration"
>> http://lkml.org/lkml/2009/6/9/221
>>
>> arch/arm/mach-at91/at91sam9g45_devices.c | 12 ++++++++----
>> 1 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
>> index 5be8cf2..7d939c0 100644
>> --- a/arch/arm/mach-at91/at91sam9g45_devices.c
>> +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
>> @@ -118,8 +118,10 @@ void __init at91_add_device_usbh_ohci(struct at91_usbh_data *data)
>>
>> /* Enable VBus control for UHP ports */
>> for (i = 0; i < data->ports; i++) {
>> - if (data->vbus_pin[i])
>> - at91_set_gpio_output(data->vbus_pin[i], 0);
>> + if (data->vbus_pin[i]) {
>> + gpio_request(data->vbus_pin[i], "usb host vbus");
>> + gpio_direction_output(data->vbus_pin[i], 0);
>> + }
>> }
>>
>> usbh_ohci_data = *data;
>> @@ -173,8 +175,10 @@ void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data)
>>
>> /* Enable VBus control for UHP ports */
>> for (i = 0; i < data->ports; i++) {
>> - if (data->vbus_pin[i])
>> - at91_set_gpio_output(data->vbus_pin[i], 0);
>> + if (data->vbus_pin[i]) {
>> + gpio_request(data->vbus_pin[i], "usb host vbus");
>> + gpio_direction_output(data->vbus_pin[i], 0);
>> + }
>> }
> as you do the same think for ehci & ohci why not factorize it?
Just because you may choose only one or the other controller.
For instance, if you only want ohci, you can disable ehci selection and
always have its configuration done.
Best regards,
--
Nicolas Ferre
On 10:13 Thu 17 Sep , Nicolas Ferre wrote:
> Jean-Christophe PLAGNIOL-VILLARD :
> > On 19:29 Wed 16 Sep , Nicolas Ferre wrote:
> >> Change pin configuration for USB vbus on at91sam9g45: use the generic gpiolib
> >> call instead of the at91 specific one.
> >> Use gpio_request() function with same identifier for OHCI and EHCI hosts as
> >> they are sharing the same pin.
> >>
> >> Signed-off-by: Nicolas Ferre <[email protected]>
> >> ---
> >> This patch is on top of at91sam9g45 USB integration one:
> >> "[PATCH 2/2] at91/USB: at91sam9g45 series USB host integration"
> >> http://lkml.org/lkml/2009/6/9/221
> >>
> >> arch/arm/mach-at91/at91sam9g45_devices.c | 12 ++++++++----
> >> 1 files changed, 8 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
> >> index 5be8cf2..7d939c0 100644
> >> --- a/arch/arm/mach-at91/at91sam9g45_devices.c
> >> +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
> >> @@ -118,8 +118,10 @@ void __init at91_add_device_usbh_ohci(struct at91_usbh_data *data)
> >>
> >> /* Enable VBus control for UHP ports */
> >> for (i = 0; i < data->ports; i++) {
> >> - if (data->vbus_pin[i])
> >> - at91_set_gpio_output(data->vbus_pin[i], 0);
> >> + if (data->vbus_pin[i]) {
> >> + gpio_request(data->vbus_pin[i], "usb host vbus");
> >> + gpio_direction_output(data->vbus_pin[i], 0);
> >> + }
> >> }
> >>
> >> usbh_ohci_data = *data;
> >> @@ -173,8 +175,10 @@ void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data)
> >>
> >> /* Enable VBus control for UHP ports */
> >> for (i = 0; i < data->ports; i++) {
> >> - if (data->vbus_pin[i])
> >> - at91_set_gpio_output(data->vbus_pin[i], 0);
> >> + if (data->vbus_pin[i]) {
> >> + gpio_request(data->vbus_pin[i], "usb host vbus");
> >> + gpio_direction_output(data->vbus_pin[i], 0);
> >> + }
> >> }
> > as you do the same think for ehci & ohci why not factorize it?
>
> Just because you may choose only one or the other controller.
> For instance, if you only want ohci, you can disable ehci selection and
> always have its configuration done.
I agree but both configuration function share some code which could be
factorize, is it not?
Best Regards,
J.
Jean-Christophe PLAGNIOL-VILLARD :
> On 10:13 Thu 17 Sep , Nicolas Ferre wrote:
>> Jean-Christophe PLAGNIOL-VILLARD :
>>> On 19:29 Wed 16 Sep , Nicolas Ferre wrote:
>>>> Change pin configuration for USB vbus on at91sam9g45: use the generic gpiolib
>>>> call instead of the at91 specific one.
>>>> Use gpio_request() function with same identifier for OHCI and EHCI hosts as
>>>> they are sharing the same pin.
>>>>
>>>> Signed-off-by: Nicolas Ferre <[email protected]>
>>>> ---
>>>> This patch is on top of at91sam9g45 USB integration one:
>>>> "[PATCH 2/2] at91/USB: at91sam9g45 series USB host integration"
>>>> http://lkml.org/lkml/2009/6/9/221
>>>>
>>>> arch/arm/mach-at91/at91sam9g45_devices.c | 12 ++++++++----
>>>> 1 files changed, 8 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
>>>> index 5be8cf2..7d939c0 100644
>>>> --- a/arch/arm/mach-at91/at91sam9g45_devices.c
>>>> +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
>>>> @@ -118,8 +118,10 @@ void __init at91_add_device_usbh_ohci(struct at91_usbh_data *data)
>>>>
>>>> /* Enable VBus control for UHP ports */
>>>> for (i = 0; i < data->ports; i++) {
>>>> - if (data->vbus_pin[i])
>>>> - at91_set_gpio_output(data->vbus_pin[i], 0);
>>>> + if (data->vbus_pin[i]) {
>>>> + gpio_request(data->vbus_pin[i], "usb host vbus");
>>>> + gpio_direction_output(data->vbus_pin[i], 0);
>>>> + }
>>>> }
>>>>
>>>> usbh_ohci_data = *data;
>>>> @@ -173,8 +175,10 @@ void __init at91_add_device_usbh_ehci(struct at91_usbh_data *data)
>>>>
>>>> /* Enable VBus control for UHP ports */
>>>> for (i = 0; i < data->ports; i++) {
>>>> - if (data->vbus_pin[i])
>>>> - at91_set_gpio_output(data->vbus_pin[i], 0);
>>>> + if (data->vbus_pin[i]) {
>>>> + gpio_request(data->vbus_pin[i], "usb host vbus");
>>>> + gpio_direction_output(data->vbus_pin[i], 0);
>>>> + }
>>>> }
>>> as you do the same think for ehci & ohci why not factorize it?
>> Just because you may choose only one or the other controller.
>> For instance, if you only want ohci, you can disable ehci selection and
>> always have its configuration done.
> I agree but both configuration function share some code which could be
> factorize, is it not?
Well it is true that it is duplication *but* we are talking about only a
very few lines with very linear configuration instructions.
I do not think it is even worth doing.
Bye,
--
Nicolas Ferre
hi David,
> On Friday 19 June 2009, Haavard Skinnemoen wrote:
>> David Brownell wrote:
>> > > --- a/arch/arm/mach-at91/at91sam9g45_devices.c
>> > > +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
>>
>> > > + /* Enable VBus control for UHP ports */
>> > > + for (i = 0; i < data->ports; i++) {
>> > > + ? ? ? ? if (data->vbus_pin[i])
>> > > + ? ? ? ? ? ? ? ? at91_set_gpio_output(data->vbus_pin[i], 0);
>> >
>> > This should gpio_request() and gpio_direction_output().
>>
>> Hmm...I thought the driver was supposed to call gpio_request(), not the
>> platform code?
>
> In some cases. ?This isn't a good case for that. ?Especially
> if it's going to call gpio_direction_output() ... which needs
> gpio_request() to have been done first.
I have to agree with Haavard on this - it's really not clear if
gpio_request() should be called in the platform-code or in the driver.
If the platform code performs a gpio_request() then surely it needs to
call a gpio_free() after configuring the pin.
Otherwise the driver's initialization code performs another
gpio_request() for that pin, but it is still "in-use" by the platform
code.
Also Documentation/gpio.txt doesn't say if a GPIO pin should even
retain it's configured state across after a gpio_free().
So for the core AT91 platform code, I'd continue to use the
AT91-specific GPIO configuration functions.
The drivers should perform the gpio_request() / gpio_free(), and it
can still call gpio_direction_output() if necessary.
Regards,
Andrew Victor
Andrew Victor :
> hi David,
>
>> On Friday 19 June 2009, Haavard Skinnemoen wrote:
>>> David Brownell wrote:
>>>>> --- a/arch/arm/mach-at91/at91sam9g45_devices.c
>>>>> +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
>>>>> + /* Enable VBus control for UHP ports */
>>>>> + for (i = 0; i < data->ports; i++) {
>>>>> + if (data->vbus_pin[i])
>>>>> + at91_set_gpio_output(data->vbus_pin[i], 0);
>>>> This should gpio_request() and gpio_direction_output().
>>> Hmm...I thought the driver was supposed to call gpio_request(), not the
>>> platform code?
>> In some cases. This isn't a good case for that. Especially
>> if it's going to call gpio_direction_output() ... which needs
>> gpio_request() to have been done first.
>
> I have to agree with Haavard on this - it's really not clear if
> gpio_request() should be called in the platform-code or in the driver.
>
> If the platform code performs a gpio_request() then surely it needs to
> call a gpio_free() after configuring the pin.
> Otherwise the driver's initialization code performs another
> gpio_request() for that pin, but it is still "in-use" by the platform
> code.
>
> Also Documentation/gpio.txt doesn't say if a GPIO pin should even
> retain it's configured state across after a gpio_free().
>
>
> So for the core AT91 platform code, I'd continue to use the
> AT91-specific GPIO configuration functions.
> The drivers should perform the gpio_request() / gpio_free(), and it
> can still call gpio_direction_output() if necessary.
Fair enough. So I forget my gpiolib patch on top of the former USB
integration one.
With your Ack, I will publish it to Russell's patch tracking system...
Bye,
--
Nicolas Ferre