On Thu, Jul 22, 2010 at 10:25 AM, Anatolij Gustschin <agust@xxxxxxx> wrote:
> The driver creates platform devices based on the information
> from USB nodes in the flat device tree. This is the replacement
> for old arch fsl_soc usb code removed by the previous patch.
> It uses usual of-style binding, available EHCI-HCD and UDC
> drivers can be bound to the created devices. The new of-style
> driver additionaly instantiates USB OTG platform device, as the
> appropriate USB OTG driver will be added soon.
>
> Signed-off-by: Anatolij Gustschin <agust@xxxxxxx>
> Cc: Kumar Gala <galak@xxxxxxxxxxxxxxxxxxx>
> Cc: Grant Likely <grant.likely@xxxxxxxxxxxx>
Hi Anatolij,
Looks pretty good, but some comments below.
g.
> ---
> drivers/usb/gadget/Kconfig | 1 +
> drivers/usb/host/Kconfig | 5 +
> drivers/usb/host/Makefile | 1 +
> drivers/usb/host/fsl-mph-dr-of.c | 189 ++++++++++++++++++++++++++++++++++++++
> 4 files changed, 196 insertions(+), 0 deletions(-)
> create mode 100644 drivers/usb/host/fsl-mph-dr-of.c
>
> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index cd27f9b..e15e314 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -158,6 +158,7 @@ config USB_GADGET_FSL_USB2
> boolean "Freescale Highspeed USB DR Peripheral Controller"
> depends on FSL_SOC || ARCH_MXC
> select USB_GADGET_DUALSPEED
> + select USB_FSL_MPH_DR_OF
> help
> Some of Freescale PowerPC processors have a High Speed
> Dual-Role(DR) USB controller, which supports device mode.
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 2d926ce..6687523 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -112,10 +112,15 @@ config XPS_USB_HCD_XILINX
> support both high speed and full speed devices, or high speed
> devices only.
>
> +config USB_FSL_MPH_DR_OF
> + bool
> + depends on PPC_OF
Drop the depends. This is selected by USB_EHCI_FSL and
USB_GADGET_FSL_USB2, which are already PPC only.
> +
> config USB_EHCI_FSL
> bool "Support for Freescale on-chip EHCI USB controller"
> depends on USB_EHCI_HCD && FSL_SOC
> select USB_EHCI_ROOT_HUB_TT
> + select USB_FSL_MPH_DR_OF
> ---help---
> Variation of ARC USB block used in some Freescale chips.
>
> diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
> index b6315aa..aacbe82 100644
> --- a/drivers/usb/host/Makefile
> +++ b/drivers/usb/host/Makefile
> @@ -33,4 +33,5 @@ obj-$(CONFIG_USB_R8A66597_HCD) += r8a66597-hcd.o
> obj-$(CONFIG_USB_ISP1760_HCD) += isp1760.o
> obj-$(CONFIG_USB_HWA_HCD) += hwa-hc.o
> obj-$(CONFIG_USB_IMX21_HCD) += imx21-hcd.o
> +obj-$(CONFIG_USB_FSL_MPH_DR_OF) += fsl-mph-dr-of.o
>
> diff --git a/drivers/usb/host/fsl-mph-dr-of.c b/drivers/usb/host/fsl-mph-dr-of.c
> new file mode 100644
> index 0000000..020a939
> --- /dev/null
> +++ b/drivers/usb/host/fsl-mph-dr-of.c
> @@ -0,0 +1,189 @@
> +/*
> + * Setup platform devices needed by the Freescale multi-port host
> + * and/or dual-role USB controller modules based on the description
> + * in flat device tree.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/fsl_devices.h>
> +#include <linux/io.h>
> +#include <linux/of_platform.h>
> +
> +struct fsl_usb2_dev_data {
> + char *dr_mode; /* controller mode */
> + char *drivers[3]; /* drivers to instantiate for this mode */
> + enum fsl_usb2_operating_modes op_mode; /* operating mode */
> +};
> +
> +struct fsl_usb2_dev_data dr_mode_data[] __devinitdata = {
> + {
> + "host",
> + { "fsl-ehci", NULL, NULL, },
> + FSL_USB2_DR_HOST,
> + },
> + {
> + "otg",
> + { "fsl-ehci", "fsl-usb2-udc", "fsl-usb2-otg", },
> + FSL_USB2_DR_OTG,
> + },
> + {
> + "periferal",
spelling. "peripheral"
> + { "fsl-usb2-udc", NULL, NULL, },
> + FSL_USB2_DR_DEVICE,
> + },
> +};
Program defensively. Use the following form:
+ {
+ .dr_mode = "host",
+ .drivers = { "fsl-ehci", NULL, NULL, },
+ .op_mode = FSL_USB2_DR_HOST,
+ },
> +
> +struct fsl_usb2_dev_data * __devinit get_dr_mode_data(struct device_node *np)
> +{
> + const unsigned char *prop;
> + int i;
> +
> + prop = of_get_property(np, "dr_mode", NULL);
> + if (prop) {
> + for (i = 0; i < ARRAY_SIZE(dr_mode_data); i++) {
> + if (!strcmp(prop, dr_mode_data[i].dr_mode))
> + return &dr_mode_data[i];
> + }
Print a warning if you get through the loop, but don't match anything.
That means that dr_mode has a bad value.
> + }
> + return &dr_mode_data[0]; /* mode not specified, use host */
> +}
> +
> +static enum fsl_usb2_phy_modes __devinit determine_usb_phy(const char *phy_type)
> +{
> + if (!phy_type)
> + return FSL_USB2_PHY_NONE;
> + if (!strcasecmp(phy_type, "ulpi"))
> + return FSL_USB2_PHY_ULPI;
> + if (!strcasecmp(phy_type, "utmi"))
> + return FSL_USB2_PHY_UTMI;
> + if (!strcasecmp(phy_type, "utmi_wide"))
> + return FSL_USB2_PHY_UTMI_WIDE;
> + if (!strcasecmp(phy_type, "serial"))
> + return FSL_USB2_PHY_SERIAL;
> +
> + return FSL_USB2_PHY_NONE;
> +}
> +
> +struct platform_device * __devinit
> +fsl_usb2_device_register(struct of_device *ofdev,
> + struct fsl_usb2_platform_data *pdata,
> + const char *name, int id)
In general, it is better to have the function name on the same line as
the return arguements so that grep shows the relevant info. Move the
arguments to subsequent lines.
> +{
> + struct platform_device *pdev;
> + const struct resource *res = ofdev->resource;
> + unsigned int num = ofdev->num_resources;
> + int retval;
> +
> + pdev = platform_device_alloc(name, id);
> + if (!pdev) {
> + retval = -ENOMEM;
> + goto error;
> + }
> +
> + pdev->dev.parent = &ofdev->dev;
Good! The sub-devices show up sanely in the device hierarchy by
setting the parent pointer.
> +
> + pdev->dev.coherent_dma_mask = ofdev->dev.coherent_dma_mask;
> + pdev->dev.dma_mask = &pdev->archdata.dma_mask;
> + *pdev->dev.dma_mask = *ofdev->dev.dma_mask;
> +
> + retval = platform_device_add_data(pdev, pdata, sizeof(*pdata));
> + if (retval)
> + goto error;
> +
> + if (num) {
> + retval = platform_device_add_resources(pdev, res, num);
> + if (retval)
> + goto error;
> + }
> +
> + retval = platform_device_add(pdev);
> + if (retval)
> + goto error;
> +
> + return pdev;
> +
> +error:
> + platform_device_put(pdev);
> + return ERR_PTR(retval);
> +}
> +
> +static int __devinit fsl_usb2_mph_dr_of_probe(struct of_device *ofdev,
> + const struct of_device_id *match)
> +{
> + struct device_node *np = ofdev->dev.of_node;
> + struct platform_device *usb_dev;
> + struct fsl_usb2_platform_data data, *pdata;
> + struct fsl_usb2_dev_data *dev_data;
> + const unsigned char *prop;
> + static unsigned int idx;
> + int i;
> +
> + if (!of_device_is_available(np))
> + return -ENODEV;
What is this for?
> +
> + pdata = match->data;
> + if (!pdata) {
The match table doesn't have any data, so this is a no-op.
> + memset(&data, 0, sizeof(data));
> + pdata = &data;
> + }
> +
> + dev_data = get_dr_mode_data(np);
> +
> + if (of_device_is_compatible(np, "fsl-usb2-mph")) {
> + prop = of_get_property(np, "port0", NULL);
> + if (prop)
if (of_get_property())
> + pdata->port_enables |= FSL_USB2_PORT0_ENABLED;
> +
> + prop = of_get_property(np, "port1", NULL);
> + if (prop)
Ditto
> + pdata->port_enables |= FSL_USB2_PORT1_ENABLED;
> +
> + pdata->operating_mode = FSL_USB2_MPH_HOST;
> + } else {
> + /* setup mode selected in the device tree */
> + pdata->operating_mode = dev_data->op_mode;
> + }
> +
> + prop = of_get_property(np, "phy_type", NULL);
> + pdata->phy_mode = determine_usb_phy(prop);
> +
> + for (i = 0; i < ARRAY_SIZE(dev_data->drivers); i++) {
> + if (!dev_data->drivers[i])
> + continue;
> + usb_dev = fsl_usb2_device_register(ofdev, pdata,
> + dev_data->drivers[i], idx);
> + if (IS_ERR(usb_dev)) {
> + dev_err(&ofdev->dev, "Can't register usb device\n");
> + return PTR_ERR(usb_dev);
> + }
> + }
> + idx++;
> + return 0;
> +}
> +
> +static const struct of_device_id fsl_usb2_mph_dr_of_match[] = {
> + { .compatible = "fsl-usb2-mph", },
> + { .compatible = "fsl-usb2-dr", },
> + {},
> +};
> +
> +static struct of_platform_driver fsl_usb2_mph_dr_driver = {
> + .driver = {
> + .name = "fsl-usb2-mph-dr",
> + .owner = THIS_MODULE,
> + .of_match_table = fsl_usb2_mph_dr_of_match,
> + },
> + .probe = fsl_usb2_mph_dr_of_probe,
> +};
No remove hook?
> +
> +static int __init fsl_usb2_mph_dr_init(void)
> +{
> + return of_register_platform_driver(&fsl_usb2_mph_dr_driver);
> +}
> +fs_initcall(fsl_usb2_mph_dr_init);
Why fs_initcall? Is module_init() not sufficient?
> --
> 1.7.0.4
>
>
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
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
