Re: [PATCH v2 05/12] usb: chipidea: add imx driver binding

[Richard Zhao <richard.zhao-KZfg59tc24xl57MIdRCFDg@public.gmane.org>]

On Tue, May 22, 2012 at 06:30:56AM +0200, Marek Vasut wrote:
> Dear Richard Zhao,
> 
> > Just add host support.
> 
> Maybe rephrase it to something like "This patch supports only the host-mode 
> functionality so far."
Thanks.
> 
> > Signed-off-by: Richard Zhao <richard.zhao-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > Signed-off-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> > Cc: Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > Cc: Alexander Shishkin <alexander.shishkin-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > Cc: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
> > ---
> >  .../devicetree/bindings/usb/ci13xxx-imx.txt        |   20 ++
> >  drivers/usb/chipidea/Makefile                      |    8 +
> >  drivers/usb/chipidea/ci13xxx_imx.c                 |  191
> > ++++++++++++++++++++ 3 files changed, 219 insertions(+), 0 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> >  create mode 100644 drivers/usb/chipidea/ci13xxx_imx.c
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt new file mode
> > 100644
> > index 0000000..beb75d6
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > @@ -0,0 +1,20 @@
> > +* Freescale i.MX ci13xxx usb controllers
> > +
> > +Required properties:
> > +- compatible: Should be "fsl,imx31-usb"
> > +- reg: Should contain registers location and length
> > +- interrupts: Should contain controller interrupt
> > +
> > +Optional properties:
> > +- fsl,usbphy: phandler of usb phy that connects to the only one port
> > +- fsl,hub-reset-gpios: gpio used to reset on-board usb hub
> > +- fsl,vbus-power-gpios: gpio used to set vbus power of the only one port
> > +
> > +Examples:
> > +usb@02184000 { /* USB OTG */
> > +	compatible = "fsl,imx6q-usb", "fsl,imx31-usb";
> > +	reg = <0x02184000 0x200>;
> > +	interrupts = <0 43 0x04>;
> > +	fsl,usbphy = <&usbphy1>;
> > +	fsl,vbus-power-gpios = <&gpio3 22 0>;
> > +};
> > diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
> > index cc34937..ffa2f63 100644
> > --- a/drivers/usb/chipidea/Makefile
> > +++ b/drivers/usb/chipidea/Makefile
> > @@ -12,3 +12,11 @@ endif
> >  ifneq ($(CONFIG_ARCH_MSM),)
> >  	obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_msm.o
> >  endif
> > +
> > +ifneq ($(CONFIG_ARCH_MXC),)
> > +	obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_imx.o
> > +else
> > +  ifneq ($(CONFIG_ARCH_MXS),)
> > +	  obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_imx.o
> > +  endif
> > +endif
> 
> I think someone commented on these patches (my previous set of patches 
> actually), that this should be fixed in a way that you can actually select which 
> bindings you want to compile in (and not limit it via Makefile like it's now).
Yes. I think you're right.

Alexander,

Do you think it's a good idea to let user select binding driver directly
and the binding driver config depends on chipidea config?
> 
> > diff --git a/drivers/usb/chipidea/ci13xxx_imx.c
> > b/drivers/usb/chipidea/ci13xxx_imx.c new file mode 100644
> > index 0000000..9f64b3c
> > --- /dev/null
> > +++ b/drivers/usb/chipidea/ci13xxx_imx.c
> > @@ -0,0 +1,191 @@
> > +/*
> > + * Copyright 2012 Freescale Semiconductor, Inc.
> > + * Copyright (C) 2012 Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
> > + * on behalf of DENX Software Engineering GmbH
> > + *
> > + * The code contained herein is licensed under the GNU General Public
> > + * License. You may obtain a copy of the GNU General Public License
> > + * Version 2 or later at the following locations:
> > + *
> > + * http://www.opensource.org/licenses/gpl-license.html
> > + * http://www.gnu.org/copyleft/gpl.html
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_gpio.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/usb/chipidea.h>
> > +#include <linux/usb/ehci_def.h>
> > +#include <linux/usb/gadget.h>
> > +#include <linux/usb/chipidea.h>
> > +#include <linux/usb/otg.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/clk.h>
> > +#include <linux/string.h>
> > +
> > +#include "ci.h"
> > +
> > +#define PORT0_STATUS	0x184
> > +
> > +#define pdev_to_phy(pdev) \
> > +	((struct usb_phy *)platform_get_drvdata(pdev))
> > +
> > +struct ci13xxx_imx_data {
> > +	struct device_node *phy_np;
> > +	struct usb_phy *phy;
> > +	struct platform_device *ci_pdev;
> > +	struct clk *clk;
> > +	int gpio_hub_rst, gpio_vbus_pwr;
> > +};
> > +
> > +static struct ci13xxx_udc_driver ci13xxx_imx_udc_driver __devinitdata  = {
> > +	.name			= "ci13xxx_imx",
> > +	.flags			= CI13XXX_REQUIRE_TRANSCEIVER |
> > +				  CI13XXX_PULLUP_ON_VBUS |
> > +				  CI13XXX_DISABLE_STREAMING,
> > +	.capoffset		= DEF_CAPOFFSET,
> > +};
> > +
> > +static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
> > +{
> > +	struct ci13xxx_imx_data *data;
> > +	struct platform_device *plat_ci, *phy_pdev;
> > +	struct platform_device_info ci_pdevinfo;
> > +	struct device_node *phy_np;
> > +	struct resource *res;
> > +	int vbus_pwr;
> > +	int ret;
> > +
> > +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> > +	if (!data) {
> > +		dev_err(&pdev->dev, "Failed to allocate CI13xxx-IMX data!\n");
> > +		return -ENOMEM;
> > +	}
> > +	data->gpio_hub_rst = -1;
ah, I should remove hub reset here.
> > +	data->gpio_vbus_pwr = -1;
> 
> This really freaks me out. Maybe we should have some kind of a regulator that 
> manages the VBUS and let this driver toggle the regulator when fitting? That'd 
> solve the problem to some extent, as other people would be able to supply NOP 
> regulator.
Good point. It looks like we still lack gpio controlled fixed regulator
driver.
> 
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res) {
> > +		dev_err(&pdev->dev, "can't get device resources!\n");
> 
> Can't ... capital "C" ;-)
thanks.
> 
> > +		return -ENOENT;
> > +	}
> > +
> > +	data->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(data->clk)) {
> > +		dev_err(&pdev->dev, "Failed to get clock!\n");
> > +		return PTR_ERR(data->clk);
> > +	}
> > +
> > +	ret = clk_prepare_enable(data->clk);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to prepare or enable clock!\n");
> > +		return ret;
> > +	}
> > +
> > +	phy_np = of_parse_phandle(pdev->dev.of_node, "fsl,usbphy", 0);
> > +	if (phy_np) {
> > +		data->phy_np = phy_np;
> > +		phy_pdev = of_find_device_by_node(phy_np);
> > +		if (phy_pdev) {
> > +			struct usb_phy *phy;
> 
> Do we really want to define new variables in the middle of a code?
It's not a problem in kernel using gcc. You can find the style though
the kernel.
> 
> > +			phy = pdev_to_phy(phy_pdev);
> > +			if (phy &&
> > +			    try_module_get(phy_pdev->dev.driver->owner)) {
> > +				usb_phy_init(phy);
> > +				data->phy = phy;
> > +			}
> > +		}
> > +	}
> > +
> > +	/* we only support host now, so enable vbus here */
> > +	vbus_pwr = of_get_named_gpio(pdev->dev.of_node,
> > +					"fsl,vbus-power-gpios", 0);
> 
> (This one is for my personal enlightment) Why is the property called "gpios" 
> (plural) if it's only one GPIO?
It's just DT naming method. It can follow a gpio array.
> 
> > +	if (gpio_is_valid(vbus_pwr) &&
> > +	    !devm_gpio_request(&pdev->dev, vbus_pwr, "vbus-pwr")) {
> > +		gpio_direction_output(vbus_pwr, 1);
> > +		data->gpio_hub_rst = vbus_pwr;
> > +	}
> > +
> > +	ci13xxx_imx_udc_driver.phy = data->phy;
> > +	memset(&ci_pdevinfo, 0, sizeof(ci_pdevinfo));
> > +	ci_pdevinfo.parent = &pdev->dev;
> > +	ci_pdevinfo.name = "ci_hdrc";
> > +	ci_pdevinfo.id = (int)res->start;
> > +	ci_pdevinfo.res = pdev->resource;
> > +	ci_pdevinfo.num_res = pdev->num_resources;
> > +	ci_pdevinfo.data = &ci13xxx_imx_udc_driver;
> > +	ci_pdevinfo.size_data = sizeof(ci13xxx_imx_udc_driver);
> > +	ci_pdevinfo.dma_mask = DMA_BIT_MASK(32);
> > +
> > +	plat_ci = platform_device_register_full(&ci_pdevinfo);
> > +	if (IS_ERR(plat_ci)) {
> > +		dev_err(&pdev->dev, "can't register ci_hdrc platform device\n");
> > +		ret = PTR_ERR(plat_ci);
> > +		goto put_np;
> > +	}
> > +
> > +	data->ci_pdev = plat_ci;
> > +	platform_set_drvdata(pdev, data);
> > +
> > +	pm_runtime_no_callbacks(&pdev->dev);
> > +	pm_runtime_enable(&pdev->dev);
> > +
> > +	return 0;
> > +
> > +put_np:
> > +	if (phy_np)
> > +		of_node_put(phy_np);
> > +	return ret;
> > +}
> > +
> > +static int __devexit ci13xxx_imx_remove(struct platform_device *pdev)
> > +{
> > +	struct ci13xxx_imx_data *data = platform_get_drvdata(pdev);
> > +
> > +	kfree(data->ci_pdev->dev.dma_mask);
> > +	data->ci_pdev->dev.dma_mask = NULL;
> > +	platform_device_unregister(data->ci_pdev);
> > +
> > +	if (data->gpio_vbus_pwr != -1)
> > +		gpio_direction_output(data->gpio_vbus_pwr, 0);
> > +	if (data->gpio_hub_rst != -1)
> > +		gpio_direction_output(data->gpio_hub_rst, 0);
> > +
> > +	if (data->phy) {
> > +		usb_phy_shutdown(data->phy);
> > +		module_put(data->phy->dev->driver->owner);
> > +	}
> > +
> > +	of_node_put(data->phy_np);
> > +
> > +	clk_disable_unprepare(data->clk);
> > +
> > +	platform_set_drvdata(pdev, NULL);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id ci13xxx_imx_dt_ids[] = {
> > +	{ .compatible = "fsl,imx31-usb", },
> 
> Wow, now it's imx31-usb ?
Yes. we discussed that imx31 is the first SoC that use this IP.
> 
> > +	{ /* sentinel */ }
> > +};
> > +
> > +static struct platform_driver ci13xxx_imx_driver = {
> > +	.probe = ci13xxx_imx_probe,
> > +	.remove = __devexit_p(ci13xxx_imx_remove),
> > +	.driver = {
> > +		.name = "imx_usb",
> > +		.owner = THIS_MODULE,
> > +		.of_match_table = ci13xxx_imx_dt_ids,
> > +	 },
> > +};
> > +
> > +module_platform_driver(ci13xxx_imx_driver);
> > +
> 
> Richard, thanks for your work on these patches so far. Sorry for taking so long 
> with reviewing these.
My pleasure! Thanks for your review too!

Richard
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html