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@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>
> ---
> 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@xxxxxxx>");
> +MODULE_LICENSE("GPL");
> --
> 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
