Hello.
Swaminathan S wrote:
This patch implements the generic DaVinci platform USB configuration
infrastructure. Re-structure's usb.c to subscribe for plaform resources for
the configured instances of controller. Adds phy_config data member in musb.h
to enable generic conrol of platform specific USB PHY functionality.
Signed-off-by: Swaminathan S <swami.iyer@xxxxxx>
I have to NAK this patchset, unfortunately...
@@ -77,5 +71,6 @@ extern void davinci_common_init(struct davinci_soc_info *soc_info);
/* standard place to map on-chip SRAMs; they *may* support DMA */
#define SRAM_VIRT 0xfffe0000
#define SRAM_SIZE SZ_128K
+#define DAVINCI_USB_OTG_BASE 0x01C64000
This file is a worst place for such macro. It should be in usb.h (or
hardware.h) instead.
diff --git a/arch/arm/mach-davinci/include/mach/usb_musb.h b/arch/arm/mach-davinci/include/mach/usb_musb.h
new file mode 100644
index 0000000..e9b35c9
--- /dev/null
+++ b/arch/arm/mach-davinci/include/mach/usb_musb.h
Oh, yet another USB header... Why in the world we need another one?
+struct plat_res_data {
+ struct musb_hdrc_platform_data *plat_data;
+ struct resource *res_data;
+ u8 num_res;
+};
+
+struct usb_plat_data {
+ struct plat_res_data *prdata;
+ u8 num_inst;
+};
Why not pass these fields as function parameters instead of creating 2
more structures?
+/* VBUS control fuction */
+extern void setup_usb(struct usb_plat_data *pdata);
+#endif
This will break the existing callers of setup_usb() -- that won't do.
Callers must be updated in the same patch.
diff --git a/arch/arm/mach-davinci/usb.c b/arch/arm/mach-davinci/usb.c
index 2fff9a6..2c25c55 100644
--- a/arch/arm/mach-davinci/usb.c
+++ b/arch/arm/mach-davinci/usb.c
[...]
-#ifdef CONFIG_ARCH_DAVINCI_DA8XX
-static struct resource da8xx_usb11_resources[] = {
- [0] = {
- .start = DA8XX_USB1_BASE,
- .end = DA8XX_USB1_BASE + SZ_4K - 1,
- .flags = IORESOURCE_MEM,
- },
- [1] = {
- .start = IRQ_DA8XX_IRQN,
- .end = IRQ_DA8XX_IRQN,
- .flags = IORESOURCE_IRQ,
- },
-};
-
-static u64 da8xx_usb11_dma_mask = DMA_BIT_MASK(32);
-
-static struct platform_device da8xx_usb11_device = {
- .name = "ohci",
- .id = 0,
- .dev = {
- .dma_mask = &da8xx_usb11_dma_mask,
- .coherent_dma_mask = DMA_BIT_MASK(32),
- },
- .num_resources = ARRAY_SIZE(da8xx_usb11_resources),
- .resource = da8xx_usb11_resources,
-};
-
-int __init da8xx_register_usb11(struct da8xx_ohci_root_hub *pdata)
Wait, why in the world you're removing my *OHCI* platform device?! :-/
+void __init setup_usb(struct usb_plat_data *pdata)
{
- da8xx_usb11_device.dev.platform_data = pdata;
- return platform_device_register(&da8xx_usb11_device);
+ u8 ninst = pdata->num_inst;
+ u8 inst, i = 0;
+ char name[20] = "musb_hdrc";
+ struct platform_device *pdev;
+ struct plat_res_data *plat_res_data;
+
+ do {
+ plat_res_data = &pdata->prdata[i++];
Ah, so this is an array... Why not just pass prdata to this function and
eliminate that *do {} while* loop? Why there's need (purely theoretic at
this point anyway) to register several platfrom devices at once instead of
doing it via several calls to setup_usb()? Why so complex?
+ inst = plat_res_data->plat_data->inst;
+ if (pdata->num_inst > 1)
+ sprintf(name, "musb_hdrc%d", inst);
Will the driver be able to match on such device name? Traditionally, the
names for the numbered platform devices look like "musb_hdrc.0", i.e. there
is period after the name and before the number.
+
+ pdev = platform_device_alloc(name, -1);
+
+ if (!pdev) {
+ pr_warning("WARNING: USB platform Alloc Failed\n");
+ break;
+ }
+
+ /* Verify whether the clock information has already been
+ * specified if so do not override it with generic definition.
+ */
+ if (!plat_res_data->plat_data->clock)
+ plat_res_data->plat_data->clock = "usb";
Such default won't do for DA8xx where we have "usb20" clock...
+ pdev->dev.platform_data = plat_res_data->plat_data;
+ pdev->dev.dma_mask = &usb_dmamask,
+ pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+ pdev->resource = plat_res_data->res_data;
+ pdev->num_resources = plat_res_data->num_res;
+
+ /* Associate the default configuration if not specified */
+ if (!plat_res_data->plat_data->config)
+ plat_res_data->plat_data->config = &musb_config;
+ platform_device_add(pdev);
+ } while (--ninst);
}
-#endif /* CONFIG_DAVINCI_DA8XX */
diff --git a/include/linux/usb/musb.h b/include/linux/usb/musb.h
index d437556..8fa115c 100644
--- a/include/linux/usb/musb.h
+++ b/include/linux/usb/musb.h
@@ -58,6 +58,10 @@ struct musb_hdrc_config {
};
struct musb_hdrc_platform_data {
+
+ /* MUSB instance */
+ u8 inst;
Is it assigned anywhere? Why the driver needs to know this anyway?
WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
