[PATCH v2 2/3] USB: add of_platform glue driver for FSL USB DR controller

Grant Likely grant.likely at secretlab.ca
Tue Sep 28 20:01:28 EST 2010


Hi Anatolij

Looks pretty good.  Comments below.  Main comment is that with the
recent changes in mainline, this no longer needs to be an
of_platform_driver.  It can be a plain old platform_driver instead.
It gets registered as a platform_driver anyway, but of_platform_driver
is a shim that adds a bit of overhead, so it is best to avoid it.
I'll detail the changes you need to make in my comments below.

On Tue, Sep 28, 2010 at 6:36 PM, Anatolij Gustschin <agust at denx.de> 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 at denx.de>
> Cc: Kumar Gala <galak at kernel.crashing.org>
> Cc: Grant Likely <grant.likely at secretlab.ca>
> ---
> v2:
>  - drop unneeded PPC_OF dependency
>  - fix spelling bug in mode table and fix coding style
>  - print a warning if no valid dr_mode found
>  - add remove hook to unregister platform devices
>   created by probe. So the driver can be unbound
>  - fix OTG platform device creation order
>  - drop fs_initcall, replaced by module_init() now
>  - add module_exit(), MODULE_DESCRIPTION, MODULE_LICENSE
>
>  drivers/usb/gadget/Kconfig       |    1 +
>  drivers/usb/host/Kconfig         |    4 +
>  drivers/usb/host/Makefile        |    1 +
>  drivers/usb/host/fsl-mph-dr-of.c |  213 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 219 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 fab765d..0fe5bc8 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..f3a90b0 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -112,10 +112,14 @@ config XPS_USB_HCD_XILINX
>                support both high speed and full speed devices, or high speed
>                devices only.
>
> +config USB_FSL_MPH_DR_OF
> +       tristate
> +
>  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..ee8cb94
> --- /dev/null
> +++ b/drivers/usb/host/fsl-mph-dr-of.c
> @@ -0,0 +1,213 @@
> +/*
> + * 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/err.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 = {
> +       {
> +               .dr_mode = "host",
> +               .drivers = { "fsl-ehci", NULL, NULL, },
> +               .op_mode = FSL_USB2_DR_HOST,
> +       },
> +       {
> +               .dr_mode = "otg",
> +               .drivers = { "fsl-usb2-otg", "fsl-ehci", "fsl-usb2-udc", },
> +               .op_mode = FSL_USB2_DR_OTG,
> +       },
> +       {
> +               .dr_mode = "peripheral",
> +               .drivers = { "fsl-usb2-udc", NULL, NULL, },
> +               .op_mode = FSL_USB2_DR_DEVICE,
> +       },
> +};
> +
> +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];
> +               }
> +       }
> +       pr_warn("%s: Invalid 'dr_mode' property, fallback to host mode\n",
> +               np->full_name);
> +       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 platform_device *ofdev,
> +                                       struct fsl_usb2_platform_data *pdata,
> +                                       const char *name, int id)
> +{
> +       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;
> +
> +       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 platform_device *ofdev,
> +                                          const struct of_device_id *match)

Change to:
static int __devinit fsl_usb2_mph_dr_of_probe(struct platform_device *ofdev)

You can get the 'match' pointer by calling of_match_device().

> +{
> +       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;
> +
> +       pdata = match->data;
> +       if (!pdata) {
> +               memset(&data, 0, sizeof(data));
> +               pdata = &data;
> +       }

This is bad behaviour.  *match->data must not be modified in probe
because multiple instances of the device can exist.  The target of
pdata is modified later in the probe routine.

However, the above 4 lines can be removed entirely since none of the
fsl_usb2_mph_dr_of_match entries actually set the data pointer.  The
local 'data' structure can be used directly.

> +
> +       dev_data = get_dr_mode_data(np);
> +
> +       if (of_device_is_compatible(np, "fsl-usb2-mph")) {
> +               if (of_get_property(np, "port0", NULL))
> +                       pdata->port_enables |= FSL_USB2_PORT0_ENABLED;
> +
> +               if (of_get_property(np, "port1", NULL))
> +                       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 int __devexit __unregister_subdev(struct device *dev, void *d)
> +{
> +       platform_device_unregister(to_platform_device(dev));
> +       return 0;
> +}
> +
> +static int __devexit fsl_usb2_mph_dr_of_remove(struct platform_device *ofdev)
> +{
> +       device_for_each_child(&ofdev->dev, NULL, __unregister_subdev);
> +       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,
> +       .remove = __devexit_p(fsl_usb2_mph_dr_of_remove),
> +};

Change of_platform_driver to platform_driver

> +
> +static int __init fsl_usb2_mph_dr_init(void)
> +{
> +       return of_register_platform_driver(&fsl_usb2_mph_dr_driver);

change to platform_driver_register()

> +}
> +module_init(fsl_usb2_mph_dr_init);
> +
> +static void __exit fsl_usb2_mph_dr_exit(void)
> +{
> +       of_unregister_platform_driver(&fsl_usb2_mph_dr_driver);

change to platform_driver_unregister()

> +}
> +module_exit(fsl_usb2_mph_dr_exit);
> +
> +MODULE_DESCRIPTION("FSL MPH DR OF devices driver");
> +MODULE_AUTHOR("Anatolij Gustschin <agust at denx.de>");
> +MODULE_LICENSE("GPL");
> --
> 1.7.0.4
>
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.


More information about the Linuxppc-dev mailing list