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

On Tue, May 22, 2012 at 06:30:56AM +0200, Marek Vasut wrote:

Dear Richard Zhao,

quoted

Just add host support.

Maybe rephrase it to something like "This patch supports only the host-mode 
functionality so far."

Thanks.

quoted

Signed-off-by: Richard Zhao <redacted>
Signed-off-by: Marek Vasut <redacted>
Cc: Peter Chen <redacted>
Cc: Alexander Shishkin <alexander.shishkin-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
Cc: Greg Kroah-Hartman <redacted>
---
 .../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?

quoted

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.

quoted

+	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.

quoted

+	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.

quoted

+		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.

quoted

+			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.

quoted

+	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.

quoted

+	{ /* 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