[PATCH 0/3] USB: add generic onboard USB HUB driver

[PATCH 0/3] USB: add generic onboard USB HUB driver

[Peter Chen]

Hi all,

There is a known issue that the USB code can't handle USB HUB's
external pins well, in that case, it may cause some onboard
USB HUBs can't work since their PHY's clock or reset pin needs to
operate.

The user reported this issue at below:
http://www.spinics.net/lists/linux-usb/msg131502.html

In this patch set, I add a generic onboard USB HUB driver to
handle this problem, the external signals will be configured
before usb controller's initialization, it much likes we did
it at board code before.

The user needs to add this generic hub node at dts or add releted
platform data to support it.

The udoo users, help to test please.

Peter Chen (3):
  usb: misc: generic_onboard_hub: add generic onboard USB HUB driver
  doc: dt-binding: generic onboard USB HUB
  ARM: dts: imx6qdl-udoo.dtsi: fix onboard USB HUB property

 .../bindings/usb/generic-onboard-hub.txt           |  28 ++++
 MAINTAINERS                                        |   7 +
 arch/arm/boot/dts/imx6qdl-udoo.dtsi                |  25 +---
 drivers/usb/misc/Kconfig                           |   9 ++
 drivers/usb/misc/Makefile                          |   1 +
 drivers/usb/misc/generic_onboard_hub.c             | 162 +++++++++++++++++++++
 include/linux/usb/generic_onboard_hub.h            |  11 ++
 7 files changed, 224 insertions(+), 19 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
 create mode 100644 drivers/usb/misc/generic_onboard_hub.c
 create mode 100644 include/linux/usb/generic_onboard_hub.h

-- 
1.9.1

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

[PATCH 0/3] USB: add generic onboard USB HUB driver

[Peter Chen]

Hi all,

There is a known issue that the USB code can't handle USB HUB's
external pins well, in that case, it may cause some onboard
USB HUBs can't work since their PHY's clock or reset pin needs to
operate.

The user reported this issue at below:
http://www.spinics.net/lists/linux-usb/msg131502.html

In this patch set, I add a generic onboard USB HUB driver to
handle this problem, the external signals will be configured
before usb controller's initialization, it much likes we did
it at board code before.

The user needs to add this generic hub node at dts or add releted
platform data to support it.

The udoo users, help to test please.

Peter Chen (3):
  usb: misc: generic_onboard_hub: add generic onboard USB HUB driver
  doc: dt-binding: generic onboard USB HUB
  ARM: dts: imx6qdl-udoo.dtsi: fix onboard USB HUB property

 .../bindings/usb/generic-onboard-hub.txt           |  28 ++++
 MAINTAINERS                                        |   7 +
 arch/arm/boot/dts/imx6qdl-udoo.dtsi                |  25 +---
 drivers/usb/misc/Kconfig                           |   9 ++
 drivers/usb/misc/Makefile                          |   1 +
 drivers/usb/misc/generic_onboard_hub.c             | 162 +++++++++++++++++++++
 include/linux/usb/generic_onboard_hub.h            |  11 ++
 7 files changed, 224 insertions(+), 19 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
 create mode 100644 drivers/usb/misc/generic_onboard_hub.c
 create mode 100644 include/linux/usb/generic_onboard_hub.h

-- 
1.9.1

[PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Peter Chen]

Current USB HUB driver lacks of platform interfaces to configure
external signal on HUB chip, eg, the PHY input clock and gpio reset
pin for HUB, these kinds of HUBs are usually soldered at the board,
and they are not hot-plug USB devices.

With this patch, the user can configure the HUB's pins at platform
part (through dts or platform), and these configuration will be
effective before the USB bus can be used, we use subsys_initcall
for this driver.

Signed-off-by: Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 MAINTAINERS                             |   7 ++
 drivers/usb/misc/Kconfig                |   9 ++
 drivers/usb/misc/Makefile               |   1 +
 drivers/usb/misc/generic_onboard_hub.c  | 162 ++++++++++++++++++++++++++++++++
 include/linux/usb/generic_onboard_hub.h |  11 +++
 5 files changed, 190 insertions(+)
 create mode 100644 drivers/usb/misc/generic_onboard_hub.c
 create mode 100644 include/linux/usb/generic_onboard_hub.h

diff --git a/MAINTAINERS b/MAINTAINERS
index e9caa4b..cc1981e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11121,6 +11121,13 @@ S:	Maintained
 F:	Documentation/usb/ohci.txt
 F:	drivers/usb/host/ohci*
 
+USB Generic Onboard HUB Driver
+M:	Peter Chen <Peter.Chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
+T:	git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
+L:	linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+S:	Maintained
+F:	drivers/usb/misc/generic_onboard_hub.c
+
 USB OTG FSM (Finite State Machine)
 M:	Peter Chen <Peter.Chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
index f7a7fc2..8921cae 100644
--- a/drivers/usb/misc/Kconfig
+++ b/drivers/usb/misc/Kconfig
@@ -268,3 +268,12 @@ config USB_CHAOSKEY
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called chaoskey.
+
+config USB_ONBOARD_HUB
+	tristate "Generic USB Onboard HUB"
+	help
+	  Say Y here if your board has an onboard HUB, and this hub needs
+	  to control its PHY clock and reset pin through external signals.
+	  If you are not sure, say N.
+
+	  To compile this driver as a module, choose M here.
diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
index 45fd4ac..da52e9a 100644
--- a/drivers/usb/misc/Makefile
+++ b/drivers/usb/misc/Makefile
@@ -29,3 +29,4 @@ obj-$(CONFIG_USB_CHAOSKEY)		+= chaoskey.o
 
 obj-$(CONFIG_USB_SISUSBVGA)		+= sisusbvga/
 obj-$(CONFIG_USB_LINK_LAYER_TEST)	+= lvstest.o
+obj-$(CONFIG_USB_ONBOARD_HUB)		+= generic_onboard_hub.o
diff --git a/drivers/usb/misc/generic_onboard_hub.c b/drivers/usb/misc/generic_onboard_hub.c
new file mode 100644
index 0000000..df722a0
--- /dev/null
+++ b/drivers/usb/misc/generic_onboard_hub.c
@@ -0,0 +1,162 @@
+/*
+ * usb_hub_generic.c	The generic onboard USB HUB driver
+ *
+ * Copyright (C) 2015 Freescale Semiconductor, Inc.
+ * Author: Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2  of
+ * the License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+/*
+ * This driver is only for the USB HUB devices which need to control
+ * their external pins(clock, reset, etc), and these USB HUB devices
+ * are soldered at the board.
+ */
+
+#define DEBUG
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/usb/generic_onboard_hub.h>
+
+struct usb_hub_generic_data {
+	struct clk *clk;
+};
+
+static int usb_hub_generic_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct usb_hub_generic_platform_data *pdata = dev->platform_data;
+	struct usb_hub_generic_data *hub_data;
+	int reset_pol = 0, duration_us = 50, ret = 0;
+	struct gpio_desc *gpiod_reset = NULL;
+
+	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
+	if (!hub_data)
+		return -ENOMEM;
+
+	if (dev->of_node) {
+		struct device_node *node = dev->of_node;
+
+		hub_data->clk = devm_clk_get(dev, "external_clk");
+		if (IS_ERR(hub_data->clk)) {
+			dev_dbg(dev, "Can't get external clock: %ld\n",
+					PTR_ERR(hub_data->clk));
+		}
+
+		/*
+		 * Try to get the information for HUB reset, the
+		 * default setting like below:
+		 *
+		 * - Reset state is low
+		 * - The duration is 50us
+		 */
+		if (of_find_property(node, "hub-reset-active-high", NULL))
+			reset_pol = 1;
+
+		of_property_read_u32(node, "hub-reset-duration-us",
+			&duration_us);
+
+		gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
+			reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
+		ret = PTR_ERR_OR_ZERO(gpiod_reset);
+		if (ret) {
+			dev_err(dev, "Failed to get reset gpio, err = %d\n",
+				ret);
+			return ret;
+		}
+	} else if (pdata) {
+		hub_data->clk = pdata->ext_clk;
+		duration_us = pdata->gpio_reset_duration_us;
+		reset_pol = pdata->gpio_reset_polarity;
+
+		if (gpio_is_valid(pdata->gpio_reset)) {
+			ret = devm_gpio_request_one(
+				dev, pdata->gpio_reset, reset_pol
+				? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
+				dev_name(dev));
+			if (!ret)
+				gpiod_reset = gpio_to_desc(pdata->gpio_reset);
+		}
+	}
+
+	if (!IS_ERR(hub_data->clk)) {
+		ret = clk_prepare_enable(hub_data->clk);
+		if (ret) {
+			dev_err(dev,
+				"Can't enable external clock: %d\n",
+				ret);
+			return ret;
+		}
+	}
+
+	if (gpiod_reset) {
+		/* Sanity check */
+		if (duration_us > 1000000)
+			duration_us = 50;
+		usleep_range(duration_us, duration_us + 100);
+		gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);
+	}
+
+	dev_set_drvdata(dev, hub_data);
+	return ret;
+}
+
+static int usb_hub_generic_remove(struct platform_device *pdev)
+{
+	struct usb_hub_generic_data *hub_data = platform_get_drvdata(pdev);
+
+	if (!IS_ERR(hub_data->clk))
+		clk_disable_unprepare(hub_data->clk);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id usb_hub_generic_dt_ids[] = {
+	{.compatible = "generic-onboard-hub"},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, usb_hub_generic_dt_ids);
+#endif
+
+static struct platform_driver usb_hub_generic_driver = {
+	.probe = usb_hub_generic_probe,
+	.remove = usb_hub_generic_remove,
+	.driver = {
+		.name = "usb_hub_generic_onboard",
+		.of_match_table = usb_hub_generic_dt_ids,
+	 },
+};
+
+static int __init usb_hub_generic_init(void)
+{
+	return platform_driver_register(&usb_hub_generic_driver);
+}
+subsys_initcall(usb_hub_generic_init);
+
+static void __exit usb_hub_generic_exit(void)
+{
+	platform_driver_unregister(&usb_hub_generic_driver);
+}
+module_exit(usb_hub_generic_exit);
+
+MODULE_AUTHOR("Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>");
+MODULE_DESCRIPTION("Generic Onboard USB HUB driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/usb/generic_onboard_hub.h b/include/linux/usb/generic_onboard_hub.h
new file mode 100644
index 0000000..1b70ccc
--- /dev/null
+++ b/include/linux/usb/generic_onboard_hub.h
@@ -0,0 +1,11 @@
+#ifndef __LINUX_USB_GENERIC_HUB_H
+#define __LINUX_USB_GENERIC_HUB_H
+
+struct usb_hub_generic_platform_data {
+	int gpio_reset;
+	int gpio_reset_polarity;
+	int gpio_reset_duration_us;
+	struct clk *ext_clk;
+};
+
+#endif /* __LINUX_USB_GENERIC_HUB_H */
-- 
1.9.1

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

[PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Peter Chen]

Current USB HUB driver lacks of platform interfaces to configure
external signal on HUB chip, eg, the PHY input clock and gpio reset
pin for HUB, these kinds of HUBs are usually soldered at the board,
and they are not hot-plug USB devices.

With this patch, the user can configure the HUB's pins at platform
part (through dts or platform), and these configuration will be
effective before the USB bus can be used, we use subsys_initcall
for this driver.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 MAINTAINERS                             |   7 ++
 drivers/usb/misc/Kconfig                |   9 ++
 drivers/usb/misc/Makefile               |   1 +
 drivers/usb/misc/generic_onboard_hub.c  | 162 ++++++++++++++++++++++++++++++++
 include/linux/usb/generic_onboard_hub.h |  11 +++
 5 files changed, 190 insertions(+)
 create mode 100644 drivers/usb/misc/generic_onboard_hub.c
 create mode 100644 include/linux/usb/generic_onboard_hub.h

diff --git a/MAINTAINERS b/MAINTAINERS
index e9caa4b..cc1981e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11121,6 +11121,13 @@ S:	Maintained
 F:	Documentation/usb/ohci.txt
 F:	drivers/usb/host/ohci*
 
+USB Generic Onboard HUB Driver
+M:	Peter Chen <Peter.Chen@freescale.com>
+T:	git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
+L:	linux-usb at vger.kernel.org
+S:	Maintained
+F:	drivers/usb/misc/generic_onboard_hub.c
+
 USB OTG FSM (Finite State Machine)
 M:	Peter Chen <Peter.Chen@freescale.com>
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
index f7a7fc2..8921cae 100644
--- a/drivers/usb/misc/Kconfig
+++ b/drivers/usb/misc/Kconfig
@@ -268,3 +268,12 @@ config USB_CHAOSKEY
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called chaoskey.
+
+config USB_ONBOARD_HUB
+	tristate "Generic USB Onboard HUB"
+	help
+	  Say Y here if your board has an onboard HUB, and this hub needs
+	  to control its PHY clock and reset pin through external signals.
+	  If you are not sure, say N.
+
+	  To compile this driver as a module, choose M here.
diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
index 45fd4ac..da52e9a 100644
--- a/drivers/usb/misc/Makefile
+++ b/drivers/usb/misc/Makefile
@@ -29,3 +29,4 @@ obj-$(CONFIG_USB_CHAOSKEY)		+= chaoskey.o
 
 obj-$(CONFIG_USB_SISUSBVGA)		+= sisusbvga/
 obj-$(CONFIG_USB_LINK_LAYER_TEST)	+= lvstest.o
+obj-$(CONFIG_USB_ONBOARD_HUB)		+= generic_onboard_hub.o
diff --git a/drivers/usb/misc/generic_onboard_hub.c b/drivers/usb/misc/generic_onboard_hub.c
new file mode 100644
index 0000000..df722a0
--- /dev/null
+++ b/drivers/usb/misc/generic_onboard_hub.c
@@ -0,0 +1,162 @@
+/*
+ * usb_hub_generic.c	The generic onboard USB HUB driver
+ *
+ * Copyright (C) 2015 Freescale Semiconductor, Inc.
+ * Author: Peter Chen <peter.chen@freescale.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2  of
+ * the License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+/*
+ * This driver is only for the USB HUB devices which need to control
+ * their external pins(clock, reset, etc), and these USB HUB devices
+ * are soldered at the board.
+ */
+
+#define DEBUG
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/gpio.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/usb/generic_onboard_hub.h>
+
+struct usb_hub_generic_data {
+	struct clk *clk;
+};
+
+static int usb_hub_generic_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct usb_hub_generic_platform_data *pdata = dev->platform_data;
+	struct usb_hub_generic_data *hub_data;
+	int reset_pol = 0, duration_us = 50, ret = 0;
+	struct gpio_desc *gpiod_reset = NULL;
+
+	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
+	if (!hub_data)
+		return -ENOMEM;
+
+	if (dev->of_node) {
+		struct device_node *node = dev->of_node;
+
+		hub_data->clk = devm_clk_get(dev, "external_clk");
+		if (IS_ERR(hub_data->clk)) {
+			dev_dbg(dev, "Can't get external clock: %ld\n",
+					PTR_ERR(hub_data->clk));
+		}
+
+		/*
+		 * Try to get the information for HUB reset, the
+		 * default setting like below:
+		 *
+		 * - Reset state is low
+		 * - The duration is 50us
+		 */
+		if (of_find_property(node, "hub-reset-active-high", NULL))
+			reset_pol = 1;
+
+		of_property_read_u32(node, "hub-reset-duration-us",
+			&duration_us);
+
+		gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
+			reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
+		ret = PTR_ERR_OR_ZERO(gpiod_reset);
+		if (ret) {
+			dev_err(dev, "Failed to get reset gpio, err = %d\n",
+				ret);
+			return ret;
+		}
+	} else if (pdata) {
+		hub_data->clk = pdata->ext_clk;
+		duration_us = pdata->gpio_reset_duration_us;
+		reset_pol = pdata->gpio_reset_polarity;
+
+		if (gpio_is_valid(pdata->gpio_reset)) {
+			ret = devm_gpio_request_one(
+				dev, pdata->gpio_reset, reset_pol
+				? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
+				dev_name(dev));
+			if (!ret)
+				gpiod_reset = gpio_to_desc(pdata->gpio_reset);
+		}
+	}
+
+	if (!IS_ERR(hub_data->clk)) {
+		ret = clk_prepare_enable(hub_data->clk);
+		if (ret) {
+			dev_err(dev,
+				"Can't enable external clock: %d\n",
+				ret);
+			return ret;
+		}
+	}
+
+	if (gpiod_reset) {
+		/* Sanity check */
+		if (duration_us > 1000000)
+			duration_us = 50;
+		usleep_range(duration_us, duration_us + 100);
+		gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);
+	}
+
+	dev_set_drvdata(dev, hub_data);
+	return ret;
+}
+
+static int usb_hub_generic_remove(struct platform_device *pdev)
+{
+	struct usb_hub_generic_data *hub_data = platform_get_drvdata(pdev);
+
+	if (!IS_ERR(hub_data->clk))
+		clk_disable_unprepare(hub_data->clk);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id usb_hub_generic_dt_ids[] = {
+	{.compatible = "generic-onboard-hub"},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, usb_hub_generic_dt_ids);
+#endif
+
+static struct platform_driver usb_hub_generic_driver = {
+	.probe = usb_hub_generic_probe,
+	.remove = usb_hub_generic_remove,
+	.driver = {
+		.name = "usb_hub_generic_onboard",
+		.of_match_table = usb_hub_generic_dt_ids,
+	 },
+};
+
+static int __init usb_hub_generic_init(void)
+{
+	return platform_driver_register(&usb_hub_generic_driver);
+}
+subsys_initcall(usb_hub_generic_init);
+
+static void __exit usb_hub_generic_exit(void)
+{
+	platform_driver_unregister(&usb_hub_generic_driver);
+}
+module_exit(usb_hub_generic_exit);
+
+MODULE_AUTHOR("Peter Chen <peter.chen@freescale.com>");
+MODULE_DESCRIPTION("Generic Onboard USB HUB driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/usb/generic_onboard_hub.h b/include/linux/usb/generic_onboard_hub.h
new file mode 100644
index 0000000..1b70ccc
--- /dev/null
+++ b/include/linux/usb/generic_onboard_hub.h
@@ -0,0 +1,11 @@
+#ifndef __LINUX_USB_GENERIC_HUB_H
+#define __LINUX_USB_GENERIC_HUB_H
+
+struct usb_hub_generic_platform_data {
+	int gpio_reset;
+	int gpio_reset_polarity;
+	int gpio_reset_duration_us;
+	struct clk *ext_clk;
+};
+
+#endif /* __LINUX_USB_GENERIC_HUB_H */
-- 
1.9.1

[PATCH 2/3] doc: dt-binding: generic onboard USB HUB

[Peter Chen]

Add dt-binding documentation for generic onboard USB HUB.

Signed-off-by: Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 .../bindings/usb/generic-onboard-hub.txt           | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt

diff --git a/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
new file mode 100644
index 0000000..ea92205
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
@@ -0,0 +1,28 @@
+Generic Onboard USB HUB
+
+Required properties:
+- compatible: should be "generic-onboard-hub"
+
+Optional properties:
+- clocks: the input clock for HUB.
+
+- clock-names: Should be "external_clk"
+
+- hub-reset-gpios: Should specify the GPIO for reset.
+
+- hub-reset-active-high: the active reset signal is high, if this property is
+  not set, the active reset signal is low.
+
+- hub-reset-duration-us: the duration for assert reset signal, the time unit
+  is microsecond.
+
+Example:
+
+	usb_hub1 {
+		compatible = "generic-onboard-hub";
+		clocks = <&clks IMX6QDL_CLK_CKO>;
+		clock-names = "external_clk";
+		hub-reset-gpios = <&gpio7 12 0>;
+		hub-reset-active-high;
+		hub-reset-duration-us = <2>;
+	};
-- 
1.9.1

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

[PATCH 2/3] doc: dt-binding: generic onboard USB HUB

[Peter Chen]

Add dt-binding documentation for generic onboard USB HUB.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 .../bindings/usb/generic-onboard-hub.txt           | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt

diff --git a/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
new file mode 100644
index 0000000..ea92205
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
@@ -0,0 +1,28 @@
+Generic Onboard USB HUB
+
+Required properties:
+- compatible: should be "generic-onboard-hub"
+
+Optional properties:
+- clocks: the input clock for HUB.
+
+- clock-names: Should be "external_clk"
+
+- hub-reset-gpios: Should specify the GPIO for reset.
+
+- hub-reset-active-high: the active reset signal is high, if this property is
+  not set, the active reset signal is low.
+
+- hub-reset-duration-us: the duration for assert reset signal, the time unit
+  is microsecond.
+
+Example:
+
+	usb_hub1 {
+		compatible = "generic-onboard-hub";
+		clocks = <&clks IMX6QDL_CLK_CKO>;
+		clock-names = "external_clk";
+		hub-reset-gpios = <&gpio7 12 0>;
+		hub-reset-active-high;
+		hub-reset-duration-us = <2>;
+	};
-- 
1.9.1

[PATCH 3/3] ARM: dts: imx6qdl-udoo.dtsi: fix onboard USB HUB property

[Peter Chen]

The current dts describes USB HUB's property at USB controller's
entry, it is improper. Fix it by using a generic USB HUB entry.

Signed-off-by: Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 arch/arm/boot/dts/imx6qdl-udoo.dtsi | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/arch/arm/boot/dts/imx6qdl-udoo.dtsi b/arch/arm/boot/dts/imx6qdl-udoo.dtsi
index 1211da8..9416d75 100644
--- a/arch/arm/boot/dts/imx6qdl-udoo.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-udoo.dtsi
@@ -18,21 +18,12 @@
 		reg = <0x10000000 0x40000000>;
 	};
 
-	regulators {
-		compatible = "simple-bus";
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		reg_usb_h1_vbus: regulator@0 {
-			compatible = "regulator-fixed";
-			reg = <0>;
-			regulator-name = "usb_h1_vbus";
-			regulator-min-microvolt = <5000000>;
-			regulator-max-microvolt = <5000000>;
-			enable-active-high;
-			startup-delay-us = <2>; /* USB2415 requires a POR of 1 us minimum */
-			gpio = <&gpio7 12 0>;
-		};
+	usb_hub1 {
+		compatible = "generic-onboard-hub";
+		clocks = <&clks IMX6QDL_CLK_CKO>;
+		clock-names = "external_clk";
+		hub-reset-gpios = <&gpio7 12 0>;
+		hub-reset-duration-us = <2>;
 	};
 };
 
@@ -119,10 +110,6 @@
 };
 
 &usbh1 {
-	pinctrl-names = "default";
-	pinctrl-0 = <&pinctrl_usbh>;
-	vbus-supply = <&reg_usb_h1_vbus>;
-	clocks = <&clks 201>;
 	status = "okay";
 };
 
-- 
1.9.1

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

[PATCH 3/3] ARM: dts: imx6qdl-udoo.dtsi: fix onboard USB HUB property

[Peter Chen]

The current dts describes USB HUB's property at USB controller's
entry, it is improper. Fix it by using a generic USB HUB entry.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 arch/arm/boot/dts/imx6qdl-udoo.dtsi | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/arch/arm/boot/dts/imx6qdl-udoo.dtsi b/arch/arm/boot/dts/imx6qdl-udoo.dtsi
index 1211da8..9416d75 100644
--- a/arch/arm/boot/dts/imx6qdl-udoo.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-udoo.dtsi
@@ -18,21 +18,12 @@
 		reg = <0x10000000 0x40000000>;
 	};
 
-	regulators {
-		compatible = "simple-bus";
-		#address-cells = <1>;
-		#size-cells = <0>;
-
-		reg_usb_h1_vbus: regulator at 0 {
-			compatible = "regulator-fixed";
-			reg = <0>;
-			regulator-name = "usb_h1_vbus";
-			regulator-min-microvolt = <5000000>;
-			regulator-max-microvolt = <5000000>;
-			enable-active-high;
-			startup-delay-us = <2>; /* USB2415 requires a POR of 1 us minimum */
-			gpio = <&gpio7 12 0>;
-		};
+	usb_hub1 {
+		compatible = "generic-onboard-hub";
+		clocks = <&clks IMX6QDL_CLK_CKO>;
+		clock-names = "external_clk";
+		hub-reset-gpios = <&gpio7 12 0>;
+		hub-reset-duration-us = <2>;
 	};
 };
 
@@ -119,10 +110,6 @@
 };
 
 &usbh1 {
-	pinctrl-names = "default";
-	pinctrl-0 = <&pinctrl_usbh>;
-	vbus-supply = <&reg_usb_h1_vbus>;
-	clocks = <&clks 201>;
 	status = "okay";
 };
 
-- 
1.9.1

Re: [PATCH 2/3] doc: dt-binding: generic onboard USB HUB

[Fabio Estevam]

On Mon, Dec 7, 2015 at 11:37 PM, Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> Add dt-binding documentation for generic onboard USB HUB.
>
> Signed-off-by: Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  .../bindings/usb/generic-onboard-hub.txt           | 28 ++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
>
> diff --git a/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> new file mode 100644
> index 0000000..ea92205
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> @@ -0,0 +1,28 @@
> +Generic Onboard USB HUB
> +
> +Required properties:
> +- compatible: should be "generic-onboard-hub"
> +
> +Optional properties:
> +- clocks: the input clock for HUB.
> +
> +- clock-names: Should be "external_clk"
> +
> +- hub-reset-gpios: Should specify the GPIO for reset.
> +
> +- hub-reset-active-high: the active reset signal is high, if this property is
> +  not set, the active reset signal is low.
> +
> +- hub-reset-duration-us: the duration for assert reset signal, the time unit
> +  is microsecond.
> +
> +Example:
> +
> +       usb_hub1 {
> +               compatible = "generic-onboard-hub";
> +               clocks = <&clks IMX6QDL_CLK_CKO>;
> +               clock-names = "external_clk";
> +               hub-reset-gpios = <&gpio7 12 0>;
> +               hub-reset-active-high;

I think you could drop the 'hub-reset-active-high' property and do
like this instead:

hub-reset-gpios = <&gpio7 12 GPIO_ACTIVE_HIGH>;

or

hub-reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;
--
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

[PATCH 2/3] doc: dt-binding: generic onboard USB HUB

[Fabio Estevam]

On Mon, Dec 7, 2015 at 11:37 PM, Peter Chen <peter.chen@freescale.com> wrote:
> Add dt-binding documentation for generic onboard USB HUB.
>
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
>  .../bindings/usb/generic-onboard-hub.txt           | 28 ++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
>
> diff --git a/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> new file mode 100644
> index 0000000..ea92205
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> @@ -0,0 +1,28 @@
> +Generic Onboard USB HUB
> +
> +Required properties:
> +- compatible: should be "generic-onboard-hub"
> +
> +Optional properties:
> +- clocks: the input clock for HUB.
> +
> +- clock-names: Should be "external_clk"
> +
> +- hub-reset-gpios: Should specify the GPIO for reset.
> +
> +- hub-reset-active-high: the active reset signal is high, if this property is
> +  not set, the active reset signal is low.
> +
> +- hub-reset-duration-us: the duration for assert reset signal, the time unit
> +  is microsecond.
> +
> +Example:
> +
> +       usb_hub1 {
> +               compatible = "generic-onboard-hub";
> +               clocks = <&clks IMX6QDL_CLK_CKO>;
> +               clock-names = "external_clk";
> +               hub-reset-gpios = <&gpio7 12 0>;
> +               hub-reset-active-high;

I think you could drop the 'hub-reset-active-high' property and do
like this instead:

hub-reset-gpios = <&gpio7 12 GPIO_ACTIVE_HIGH>;

or

hub-reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;

Re: [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 6928 bytes --]


Hi,

Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> writes:
> diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
> index 45fd4ac..da52e9a 100644
> --- a/drivers/usb/misc/Makefile
> +++ b/drivers/usb/misc/Makefile
> @@ -29,3 +29,4 @@ obj-$(CONFIG_USB_CHAOSKEY)		+= chaoskey.o
>  
>  obj-$(CONFIG_USB_SISUSBVGA)		+= sisusbvga/
>  obj-$(CONFIG_USB_LINK_LAYER_TEST)	+= lvstest.o
> +obj-$(CONFIG_USB_ONBOARD_HUB)		+= generic_onboard_hub.o
> diff --git a/drivers/usb/misc/generic_onboard_hub.c b/drivers/usb/misc/generic_onboard_hub.c
> new file mode 100644
> index 0000000..df722a0
> --- /dev/null
> +++ b/drivers/usb/misc/generic_onboard_hub.c
> @@ -0,0 +1,162 @@
> +/*
> + * usb_hub_generic.c	The generic onboard USB HUB driver
> + *
> + * Copyright (C) 2015 Freescale Semiconductor, Inc.
> + * Author: Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2  of
> + * the License as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +/*
> + * This driver is only for the USB HUB devices which need to control
> + * their external pins(clock, reset, etc), and these USB HUB devices
> + * are soldered at the board.
> + */
> +
> +#define DEBUG

nope

> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/usb/generic_onboard_hub.h>

<linux/platform_data/generic_onboard_hub.h> ?

> +struct usb_hub_generic_data {
> +	struct clk *clk;

seriously ? Is this really all ? What about that reset line below ?

In fact, that reset line should be using a generic reset-gpio.c instead
of a raw gpio.

> +static int usb_hub_generic_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct usb_hub_generic_platform_data *pdata = dev->platform_data;
> +	struct usb_hub_generic_data *hub_data;
> +	int reset_pol = 0, duration_us = 50, ret = 0;
> +	struct gpio_desc *gpiod_reset = NULL;
> +
> +	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> +	if (!hub_data)
> +		return -ENOMEM;
> +
> +	if (dev->of_node) {
> +		struct device_node *node = dev->of_node;
> +
> +		hub_data->clk = devm_clk_get(dev, "external_clk");
> +		if (IS_ERR(hub_data->clk)) {
> +			dev_dbg(dev, "Can't get external clock: %ld\n",
> +					PTR_ERR(hub_data->clk));

how about setting clock to NULL to here ? then you don't need IS_ERR()
checks anywhere else.

> +		}

braces shouldn't be used here, if you add NULL trick above,
however... then you can keep them.

> +		/*
> +		 * Try to get the information for HUB reset, the
> +		 * default setting like below:
> +		 *
> +		 * - Reset state is low
> +		 * - The duration is 50us
> +		 */
> +		if (of_find_property(node, "hub-reset-active-high", NULL))
> +			reset_pol = 1;

you see, this is definitely *not* generic. You should write a generic
reset-gpio.c reset controller and describe the polarity on the gpio
binding. This driver *always* uses reset_assert(); reset_deassert(); and
reset-gpio implements though by gpiod_set_value() correctly.

Polarity _must_ be described elsewhere in DT.

> +		of_property_read_u32(node, "hub-reset-duration-us",
> +			&duration_us);

likewise, this should be described as a debounce time for the GPIO.

> +		gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
> +			reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
> +		ret = PTR_ERR_OR_ZERO(gpiod_reset);
> +		if (ret) {
> +			dev_err(dev, "Failed to get reset gpio, err = %d\n",
> +				ret);
> +			return ret;
> +		}
> +	} else if (pdata) {
> +		hub_data->clk = pdata->ext_clk;
> +		duration_us = pdata->gpio_reset_duration_us;
> +		reset_pol = pdata->gpio_reset_polarity;
> +
> +		if (gpio_is_valid(pdata->gpio_reset)) {
> +			ret = devm_gpio_request_one(
> +				dev, pdata->gpio_reset, reset_pol
> +				? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> +				dev_name(dev));
> +			if (!ret)
> +				gpiod_reset = gpio_to_desc(pdata->gpio_reset);
> +		}
> +	}
> +
> +	if (!IS_ERR(hub_data->clk)) {
> +		ret = clk_prepare_enable(hub_data->clk);
> +		if (ret) {
> +			dev_err(dev,
> +				"Can't enable external clock: %d\n",
> +				ret);
> +			return ret;
> +		}
> +	}
> +
> +	if (gpiod_reset) {
> +		/* Sanity check */
> +		if (duration_us > 1000000)
> +			duration_us = 50;

this check makes no sense whatsoever. What I *really* need over 1s of
debounce ?

> +		usleep_range(duration_us, duration_us + 100);
> +		gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);

wrong. You should _not_ have polarity checks here. You should have
already initialized the gpio as ACTIVE_HIGH or ACTIVE_LOW and gpiolib
will handle the polarity for you.

> +	}
> +
> +	dev_set_drvdata(dev, hub_data);
> +	return ret;
> +}
> +
> +static int usb_hub_generic_remove(struct platform_device *pdev)
> +{
> +	struct usb_hub_generic_data *hub_data = platform_get_drvdata(pdev);
> +
> +	if (!IS_ERR(hub_data->clk))
> +		clk_disable_unprepare(hub_data->clk);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id usb_hub_generic_dt_ids[] = {
> +	{.compatible = "generic-onboard-hub"},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, usb_hub_generic_dt_ids);
> +#endif
> +
> +static struct platform_driver usb_hub_generic_driver = {
> +	.probe = usb_hub_generic_probe,
> +	.remove = usb_hub_generic_remove,
> +	.driver = {
> +		.name = "usb_hub_generic_onboard",
> +		.of_match_table = usb_hub_generic_dt_ids,
> +	 },
> +};
> +
> +static int __init usb_hub_generic_init(void)
> +{
> +	return platform_driver_register(&usb_hub_generic_driver);
> +}
> +subsys_initcall(usb_hub_generic_init);
> +
> +static void __exit usb_hub_generic_exit(void)
> +{
> +	platform_driver_unregister(&usb_hub_generic_driver);
> +}
> +module_exit(usb_hub_generic_exit);

module_platform_driver();

> +MODULE_AUTHOR("Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>");
> +MODULE_DESCRIPTION("Generic Onboard USB HUB driver");
> +MODULE_LICENSE("GPL");

comment header says GPL v2 only, this is GPL v2+. Which is correct ?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Felipe Balbi]

Hi,

Peter Chen <peter.chen@freescale.com> writes:
> diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
> index 45fd4ac..da52e9a 100644
> --- a/drivers/usb/misc/Makefile
> +++ b/drivers/usb/misc/Makefile
> @@ -29,3 +29,4 @@ obj-$(CONFIG_USB_CHAOSKEY)		+= chaoskey.o
>  
>  obj-$(CONFIG_USB_SISUSBVGA)		+= sisusbvga/
>  obj-$(CONFIG_USB_LINK_LAYER_TEST)	+= lvstest.o
> +obj-$(CONFIG_USB_ONBOARD_HUB)		+= generic_onboard_hub.o
> diff --git a/drivers/usb/misc/generic_onboard_hub.c b/drivers/usb/misc/generic_onboard_hub.c
> new file mode 100644
> index 0000000..df722a0
> --- /dev/null
> +++ b/drivers/usb/misc/generic_onboard_hub.c
> @@ -0,0 +1,162 @@
> +/*
> + * usb_hub_generic.c	The generic onboard USB HUB driver
> + *
> + * Copyright (C) 2015 Freescale Semiconductor, Inc.
> + * Author: Peter Chen <peter.chen@freescale.com>
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2  of
> + * the License as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +/*
> + * This driver is only for the USB HUB devices which need to control
> + * their external pins(clock, reset, etc), and these USB HUB devices
> + * are soldered at the board.
> + */
> +
> +#define DEBUG

nope

> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/usb/generic_onboard_hub.h>

<linux/platform_data/generic_onboard_hub.h> ?

> +struct usb_hub_generic_data {
> +	struct clk *clk;

seriously ? Is this really all ? What about that reset line below ?

In fact, that reset line should be using a generic reset-gpio.c instead
of a raw gpio.

> +static int usb_hub_generic_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct usb_hub_generic_platform_data *pdata = dev->platform_data;
> +	struct usb_hub_generic_data *hub_data;
> +	int reset_pol = 0, duration_us = 50, ret = 0;
> +	struct gpio_desc *gpiod_reset = NULL;
> +
> +	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> +	if (!hub_data)
> +		return -ENOMEM;
> +
> +	if (dev->of_node) {
> +		struct device_node *node = dev->of_node;
> +
> +		hub_data->clk = devm_clk_get(dev, "external_clk");
> +		if (IS_ERR(hub_data->clk)) {
> +			dev_dbg(dev, "Can't get external clock: %ld\n",
> +					PTR_ERR(hub_data->clk));

how about setting clock to NULL to here ? then you don't need IS_ERR()
checks anywhere else.

> +		}

braces shouldn't be used here, if you add NULL trick above,
however... then you can keep them.

> +		/*
> +		 * Try to get the information for HUB reset, the
> +		 * default setting like below:
> +		 *
> +		 * - Reset state is low
> +		 * - The duration is 50us
> +		 */
> +		if (of_find_property(node, "hub-reset-active-high", NULL))
> +			reset_pol = 1;

you see, this is definitely *not* generic. You should write a generic
reset-gpio.c reset controller and describe the polarity on the gpio
binding. This driver *always* uses reset_assert(); reset_deassert(); and
reset-gpio implements though by gpiod_set_value() correctly.

Polarity _must_ be described elsewhere in DT.

> +		of_property_read_u32(node, "hub-reset-duration-us",
> +			&duration_us);

likewise, this should be described as a debounce time for the GPIO.

> +		gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
> +			reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
> +		ret = PTR_ERR_OR_ZERO(gpiod_reset);
> +		if (ret) {
> +			dev_err(dev, "Failed to get reset gpio, err = %d\n",
> +				ret);
> +			return ret;
> +		}
> +	} else if (pdata) {
> +		hub_data->clk = pdata->ext_clk;
> +		duration_us = pdata->gpio_reset_duration_us;
> +		reset_pol = pdata->gpio_reset_polarity;
> +
> +		if (gpio_is_valid(pdata->gpio_reset)) {
> +			ret = devm_gpio_request_one(
> +				dev, pdata->gpio_reset, reset_pol
> +				? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> +				dev_name(dev));
> +			if (!ret)
> +				gpiod_reset = gpio_to_desc(pdata->gpio_reset);
> +		}
> +	}
> +
> +	if (!IS_ERR(hub_data->clk)) {
> +		ret = clk_prepare_enable(hub_data->clk);
> +		if (ret) {
> +			dev_err(dev,
> +				"Can't enable external clock: %d\n",
> +				ret);
> +			return ret;
> +		}
> +	}
> +
> +	if (gpiod_reset) {
> +		/* Sanity check */
> +		if (duration_us > 1000000)
> +			duration_us = 50;

this check makes no sense whatsoever. What I *really* need over 1s of
debounce ?

> +		usleep_range(duration_us, duration_us + 100);
> +		gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);

wrong. You should _not_ have polarity checks here. You should have
already initialized the gpio as ACTIVE_HIGH or ACTIVE_LOW and gpiolib
will handle the polarity for you.

> +	}
> +
> +	dev_set_drvdata(dev, hub_data);
> +	return ret;
> +}
> +
> +static int usb_hub_generic_remove(struct platform_device *pdev)
> +{
> +	struct usb_hub_generic_data *hub_data = platform_get_drvdata(pdev);
> +
> +	if (!IS_ERR(hub_data->clk))
> +		clk_disable_unprepare(hub_data->clk);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id usb_hub_generic_dt_ids[] = {
> +	{.compatible = "generic-onboard-hub"},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, usb_hub_generic_dt_ids);
> +#endif
> +
> +static struct platform_driver usb_hub_generic_driver = {
> +	.probe = usb_hub_generic_probe,
> +	.remove = usb_hub_generic_remove,
> +	.driver = {
> +		.name = "usb_hub_generic_onboard",
> +		.of_match_table = usb_hub_generic_dt_ids,
> +	 },
> +};
> +
> +static int __init usb_hub_generic_init(void)
> +{
> +	return platform_driver_register(&usb_hub_generic_driver);
> +}
> +subsys_initcall(usb_hub_generic_init);
> +
> +static void __exit usb_hub_generic_exit(void)
> +{
> +	platform_driver_unregister(&usb_hub_generic_driver);
> +}
> +module_exit(usb_hub_generic_exit);

module_platform_driver();

> +MODULE_AUTHOR("Peter Chen <peter.chen@freescale.com>");
> +MODULE_DESCRIPTION("Generic Onboard USB HUB driver");
> +MODULE_LICENSE("GPL");

comment header says GPL v2 only, this is GPL v2+. Which is correct ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151207/f2e29aad/attachment.sig>

Re: [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3359 bytes --]

Hi Peter,

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on v4.4-rc4 next-20151207]

url:    https://github.com/0day-ci/linux/commits/Peter-Chen/USB-add-generic-onboard-USB-HUB-driver/20151208-094428
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: tile-allmodconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=tile 

All error/warnings (new ones prefixed by >>):

   drivers/usb/misc/generic_onboard_hub.c: In function 'usb_hub_generic_probe':
>> drivers/usb/misc/generic_onboard_hub.c:76:3: error: implicit declaration of function 'devm_gpiod_get_optional'
>> drivers/usb/misc/generic_onboard_hub.c:77:16: error: 'GPIOD_OUT_HIGH' undeclared (first use in this function)
   drivers/usb/misc/generic_onboard_hub.c:77:16: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/usb/misc/generic_onboard_hub.c:77:33: error: 'GPIOD_OUT_LOW' undeclared (first use in this function)
>> drivers/usb/misc/generic_onboard_hub.c:95:5: error: implicit declaration of function 'gpio_to_desc'
>> drivers/usb/misc/generic_onboard_hub.c:95:17: warning: assignment makes pointer from integer without a cast [enabled by default]
>> drivers/usb/misc/generic_onboard_hub.c:114:3: error: implicit declaration of function 'gpiod_set_value'
   cc1: some warnings being treated as errors

vim +/devm_gpiod_get_optional +76 drivers/usb/misc/generic_onboard_hub.c

    70			if (of_find_property(node, "hub-reset-active-high", NULL))
    71				reset_pol = 1;
    72	
    73			of_property_read_u32(node, "hub-reset-duration-us",
    74				&duration_us);
    75	
  > 76			gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
  > 77				reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
    78			ret = PTR_ERR_OR_ZERO(gpiod_reset);
    79			if (ret) {
    80				dev_err(dev, "Failed to get reset gpio, err = %d\n",
    81					ret);
    82				return ret;
    83			}
    84		} else if (pdata) {
    85			hub_data->clk = pdata->ext_clk;
    86			duration_us = pdata->gpio_reset_duration_us;
    87			reset_pol = pdata->gpio_reset_polarity;
    88	
    89			if (gpio_is_valid(pdata->gpio_reset)) {
    90				ret = devm_gpio_request_one(
    91					dev, pdata->gpio_reset, reset_pol
    92					? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
    93					dev_name(dev));
    94				if (!ret)
  > 95					gpiod_reset = gpio_to_desc(pdata->gpio_reset);
    96			}
    97		}
    98	
    99		if (!IS_ERR(hub_data->clk)) {
   100			ret = clk_prepare_enable(hub_data->clk);
   101			if (ret) {
   102				dev_err(dev,
   103					"Can't enable external clock: %d\n",
   104					ret);
   105				return ret;
   106			}
   107		}
   108	
   109		if (gpiod_reset) {
   110			/* Sanity check */
   111			if (duration_us > 1000000)
   112				duration_us = 50;
   113			usleep_range(duration_us, duration_us + 100);
 > 114			gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);
   115		}
   116	
   117		dev_set_drvdata(dev, hub_data);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 42151 bytes --]

[PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[kbuild test robot]

Hi Peter,

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on v4.4-rc4 next-20151207]

url:    https://github.com/0day-ci/linux/commits/Peter-Chen/USB-add-generic-onboard-USB-HUB-driver/20151208-094428
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: tile-allmodconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=tile 

All error/warnings (new ones prefixed by >>):

   drivers/usb/misc/generic_onboard_hub.c: In function 'usb_hub_generic_probe':
>> drivers/usb/misc/generic_onboard_hub.c:76:3: error: implicit declaration of function 'devm_gpiod_get_optional'
>> drivers/usb/misc/generic_onboard_hub.c:77:16: error: 'GPIOD_OUT_HIGH' undeclared (first use in this function)
   drivers/usb/misc/generic_onboard_hub.c:77:16: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/usb/misc/generic_onboard_hub.c:77:33: error: 'GPIOD_OUT_LOW' undeclared (first use in this function)
>> drivers/usb/misc/generic_onboard_hub.c:95:5: error: implicit declaration of function 'gpio_to_desc'
>> drivers/usb/misc/generic_onboard_hub.c:95:17: warning: assignment makes pointer from integer without a cast [enabled by default]
>> drivers/usb/misc/generic_onboard_hub.c:114:3: error: implicit declaration of function 'gpiod_set_value'
   cc1: some warnings being treated as errors

vim +/devm_gpiod_get_optional +76 drivers/usb/misc/generic_onboard_hub.c

    70			if (of_find_property(node, "hub-reset-active-high", NULL))
    71				reset_pol = 1;
    72	
    73			of_property_read_u32(node, "hub-reset-duration-us",
    74				&duration_us);
    75	
  > 76			gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
  > 77				reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
    78			ret = PTR_ERR_OR_ZERO(gpiod_reset);
    79			if (ret) {
    80				dev_err(dev, "Failed to get reset gpio, err = %d\n",
    81					ret);
    82				return ret;
    83			}
    84		} else if (pdata) {
    85			hub_data->clk = pdata->ext_clk;
    86			duration_us = pdata->gpio_reset_duration_us;
    87			reset_pol = pdata->gpio_reset_polarity;
    88	
    89			if (gpio_is_valid(pdata->gpio_reset)) {
    90				ret = devm_gpio_request_one(
    91					dev, pdata->gpio_reset, reset_pol
    92					? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
    93					dev_name(dev));
    94				if (!ret)
  > 95					gpiod_reset = gpio_to_desc(pdata->gpio_reset);
    96			}
    97		}
    98	
    99		if (!IS_ERR(hub_data->clk)) {
   100			ret = clk_prepare_enable(hub_data->clk);
   101			if (ret) {
   102				dev_err(dev,
   103					"Can't enable external clock: %d\n",
   104					ret);
   105				return ret;
   106			}
   107		}
   108	
   109		if (gpiod_reset) {
   110			/* Sanity check */
   111			if (duration_us > 1000000)
   112				duration_us = 50;
   113			usleep_range(duration_us, duration_us + 100);
 > 114			gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);
   115		}
   116	
   117		dev_set_drvdata(dev, hub_data);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/octet-stream
Size: 42151 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151208/1811f234/attachment-0001.obj>

Re: [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Sascha Hauer]

On Tue, Dec 08, 2015 at 09:37:48AM +0800, Peter Chen wrote:
> Current USB HUB driver lacks of platform interfaces to configure
> external signal on HUB chip, eg, the PHY input clock and gpio reset
> pin for HUB, these kinds of HUBs are usually soldered at the board,
> and they are not hot-plug USB devices.
> 
> With this patch, the user can configure the HUB's pins at platform
> part (through dts or platform), and these configuration will be
> effective before the USB bus can be used, we use subsys_initcall
> for this driver.
> 
> Signed-off-by: Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  MAINTAINERS                             |   7 ++
>  drivers/usb/misc/Kconfig                |   9 ++
>  drivers/usb/misc/Makefile               |   1 +
>  drivers/usb/misc/generic_onboard_hub.c  | 162 ++++++++++++++++++++++++++++++++
>  include/linux/usb/generic_onboard_hub.h |  11 +++
>  5 files changed, 190 insertions(+)
>  create mode 100644 drivers/usb/misc/generic_onboard_hub.c
>  create mode 100644 include/linux/usb/generic_onboard_hub.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e9caa4b..cc1981e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11121,6 +11121,13 @@ S:	Maintained
>  F:	Documentation/usb/ohci.txt
>  F:	drivers/usb/host/ohci*
>  
> +USB Generic Onboard HUB Driver
> +M:	Peter Chen <Peter.Chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> +T:	git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
> +L:	linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> +S:	Maintained
> +F:	drivers/usb/misc/generic_onboard_hub.c
> +
>  USB OTG FSM (Finite State Machine)
>  M:	Peter Chen <Peter.Chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
> diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> index f7a7fc2..8921cae 100644
> --- a/drivers/usb/misc/Kconfig
> +++ b/drivers/usb/misc/Kconfig
> @@ -268,3 +268,12 @@ config USB_CHAOSKEY
>  
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called chaoskey.
> +
> +config USB_ONBOARD_HUB
> +	tristate "Generic USB Onboard HUB"
> +	help
> +	  Say Y here if your board has an onboard HUB, and this hub needs
> +	  to control its PHY clock and reset pin through external signals.
> +	  If you are not sure, say N.
> +
> +	  To compile this driver as a module, choose M here.
> diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
> index 45fd4ac..da52e9a 100644
> --- a/drivers/usb/misc/Makefile
> +++ b/drivers/usb/misc/Makefile
> @@ -29,3 +29,4 @@ obj-$(CONFIG_USB_CHAOSKEY)		+= chaoskey.o
>  
>  obj-$(CONFIG_USB_SISUSBVGA)		+= sisusbvga/
>  obj-$(CONFIG_USB_LINK_LAYER_TEST)	+= lvstest.o
> +obj-$(CONFIG_USB_ONBOARD_HUB)		+= generic_onboard_hub.o
> diff --git a/drivers/usb/misc/generic_onboard_hub.c b/drivers/usb/misc/generic_onboard_hub.c
> new file mode 100644
> index 0000000..df722a0
> --- /dev/null
> +++ b/drivers/usb/misc/generic_onboard_hub.c
> @@ -0,0 +1,162 @@
> +/*
> + * usb_hub_generic.c	The generic onboard USB HUB driver
> + *
> + * Copyright (C) 2015 Freescale Semiconductor, Inc.
> + * Author: Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2  of
> + * the License as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +/*
> + * This driver is only for the USB HUB devices which need to control
> + * their external pins(clock, reset, etc), and these USB HUB devices
> + * are soldered at the board.
> + */
> +
> +#define DEBUG
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/usb/generic_onboard_hub.h>
> +
> +struct usb_hub_generic_data {
> +	struct clk *clk;
> +};
> +
> +static int usb_hub_generic_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct usb_hub_generic_platform_data *pdata = dev->platform_data;
> +	struct usb_hub_generic_data *hub_data;
> +	int reset_pol = 0, duration_us = 50, ret = 0;
> +	struct gpio_desc *gpiod_reset = NULL;
> +
> +	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> +	if (!hub_data)
> +		return -ENOMEM;
> +
> +	if (dev->of_node) {
> +		struct device_node *node = dev->of_node;
> +
> +		hub_data->clk = devm_clk_get(dev, "external_clk");

Please drop such _clk suffixes. The context already makes it clear that
it's a clock.

> +		if (IS_ERR(hub_data->clk)) {
> +			dev_dbg(dev, "Can't get external clock: %ld\n",
> +					PTR_ERR(hub_data->clk));
> +		}
> +
> +		/*
> +		 * Try to get the information for HUB reset, the
> +		 * default setting like below:
> +		 *
> +		 * - Reset state is low
> +		 * - The duration is 50us
> +		 */
> +		if (of_find_property(node, "hub-reset-active-high", NULL))
> +			reset_pol = 1;
> +
> +		of_property_read_u32(node, "hub-reset-duration-us",
> +			&duration_us);
> +
> +		gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
> +			reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
> +		ret = PTR_ERR_OR_ZERO(gpiod_reset);
> +		if (ret) {
> +			dev_err(dev, "Failed to get reset gpio, err = %d\n",
> +				ret);
> +			return ret;
> +		}
> +	} else if (pdata) {
> +		hub_data->clk = pdata->ext_clk;

Passing clocks in platform_data is a no go. clocks must always be
retrieved with clk_get.

> +		duration_us = pdata->gpio_reset_duration_us;
> +		reset_pol = pdata->gpio_reset_polarity;
> +
> +		if (gpio_is_valid(pdata->gpio_reset)) {
> +			ret = devm_gpio_request_one(
> +				dev, pdata->gpio_reset, reset_pol
> +				? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> +				dev_name(dev));
> +			if (!ret)
> +				gpiod_reset = gpio_to_desc(pdata->gpio_reset);

If the gpio is valid then being unable to get it is an error.

Do you need this platform_data stuff at all?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Sascha Hauer]

On Tue, Dec 08, 2015 at 09:37:48AM +0800, Peter Chen wrote:
> Current USB HUB driver lacks of platform interfaces to configure
> external signal on HUB chip, eg, the PHY input clock and gpio reset
> pin for HUB, these kinds of HUBs are usually soldered at the board,
> and they are not hot-plug USB devices.
> 
> With this patch, the user can configure the HUB's pins at platform
> part (through dts or platform), and these configuration will be
> effective before the USB bus can be used, we use subsys_initcall
> for this driver.
> 
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
>  MAINTAINERS                             |   7 ++
>  drivers/usb/misc/Kconfig                |   9 ++
>  drivers/usb/misc/Makefile               |   1 +
>  drivers/usb/misc/generic_onboard_hub.c  | 162 ++++++++++++++++++++++++++++++++
>  include/linux/usb/generic_onboard_hub.h |  11 +++
>  5 files changed, 190 insertions(+)
>  create mode 100644 drivers/usb/misc/generic_onboard_hub.c
>  create mode 100644 include/linux/usb/generic_onboard_hub.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e9caa4b..cc1981e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11121,6 +11121,13 @@ S:	Maintained
>  F:	Documentation/usb/ohci.txt
>  F:	drivers/usb/host/ohci*
>  
> +USB Generic Onboard HUB Driver
> +M:	Peter Chen <Peter.Chen@freescale.com>
> +T:	git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
> +L:	linux-usb at vger.kernel.org
> +S:	Maintained
> +F:	drivers/usb/misc/generic_onboard_hub.c
> +
>  USB OTG FSM (Finite State Machine)
>  M:	Peter Chen <Peter.Chen@freescale.com>
>  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
> diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> index f7a7fc2..8921cae 100644
> --- a/drivers/usb/misc/Kconfig
> +++ b/drivers/usb/misc/Kconfig
> @@ -268,3 +268,12 @@ config USB_CHAOSKEY
>  
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called chaoskey.
> +
> +config USB_ONBOARD_HUB
> +	tristate "Generic USB Onboard HUB"
> +	help
> +	  Say Y here if your board has an onboard HUB, and this hub needs
> +	  to control its PHY clock and reset pin through external signals.
> +	  If you are not sure, say N.
> +
> +	  To compile this driver as a module, choose M here.
> diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
> index 45fd4ac..da52e9a 100644
> --- a/drivers/usb/misc/Makefile
> +++ b/drivers/usb/misc/Makefile
> @@ -29,3 +29,4 @@ obj-$(CONFIG_USB_CHAOSKEY)		+= chaoskey.o
>  
>  obj-$(CONFIG_USB_SISUSBVGA)		+= sisusbvga/
>  obj-$(CONFIG_USB_LINK_LAYER_TEST)	+= lvstest.o
> +obj-$(CONFIG_USB_ONBOARD_HUB)		+= generic_onboard_hub.o
> diff --git a/drivers/usb/misc/generic_onboard_hub.c b/drivers/usb/misc/generic_onboard_hub.c
> new file mode 100644
> index 0000000..df722a0
> --- /dev/null
> +++ b/drivers/usb/misc/generic_onboard_hub.c
> @@ -0,0 +1,162 @@
> +/*
> + * usb_hub_generic.c	The generic onboard USB HUB driver
> + *
> + * Copyright (C) 2015 Freescale Semiconductor, Inc.
> + * Author: Peter Chen <peter.chen@freescale.com>
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2  of
> + * the License as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +/*
> + * This driver is only for the USB HUB devices which need to control
> + * their external pins(clock, reset, etc), and these USB HUB devices
> + * are soldered at the board.
> + */
> +
> +#define DEBUG
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/usb/generic_onboard_hub.h>
> +
> +struct usb_hub_generic_data {
> +	struct clk *clk;
> +};
> +
> +static int usb_hub_generic_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct usb_hub_generic_platform_data *pdata = dev->platform_data;
> +	struct usb_hub_generic_data *hub_data;
> +	int reset_pol = 0, duration_us = 50, ret = 0;
> +	struct gpio_desc *gpiod_reset = NULL;
> +
> +	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> +	if (!hub_data)
> +		return -ENOMEM;
> +
> +	if (dev->of_node) {
> +		struct device_node *node = dev->of_node;
> +
> +		hub_data->clk = devm_clk_get(dev, "external_clk");

Please drop such _clk suffixes. The context already makes it clear that
it's a clock.

> +		if (IS_ERR(hub_data->clk)) {
> +			dev_dbg(dev, "Can't get external clock: %ld\n",
> +					PTR_ERR(hub_data->clk));
> +		}
> +
> +		/*
> +		 * Try to get the information for HUB reset, the
> +		 * default setting like below:
> +		 *
> +		 * - Reset state is low
> +		 * - The duration is 50us
> +		 */
> +		if (of_find_property(node, "hub-reset-active-high", NULL))
> +			reset_pol = 1;
> +
> +		of_property_read_u32(node, "hub-reset-duration-us",
> +			&duration_us);
> +
> +		gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
> +			reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
> +		ret = PTR_ERR_OR_ZERO(gpiod_reset);
> +		if (ret) {
> +			dev_err(dev, "Failed to get reset gpio, err = %d\n",
> +				ret);
> +			return ret;
> +		}
> +	} else if (pdata) {
> +		hub_data->clk = pdata->ext_clk;

Passing clocks in platform_data is a no go. clocks must always be
retrieved with clk_get.

> +		duration_us = pdata->gpio_reset_duration_us;
> +		reset_pol = pdata->gpio_reset_polarity;
> +
> +		if (gpio_is_valid(pdata->gpio_reset)) {
> +			ret = devm_gpio_request_one(
> +				dev, pdata->gpio_reset, reset_pol
> +				? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> +				dev_name(dev));
> +			if (!ret)
> +				gpiod_reset = gpio_to_desc(pdata->gpio_reset);

If the gpio is valid then being unable to get it is an error.

Do you need this platform_data stuff at all?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Peter Chen]

On Mon, Dec 07, 2015 at 08:59:19PM -0600, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> writes:
> > diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
> > index 45fd4ac..da52e9a 100644
> > --- a/drivers/usb/misc/Makefile
> > +++ b/drivers/usb/misc/Makefile
> > @@ -29,3 +29,4 @@ obj-$(CONFIG_USB_CHAOSKEY)		+= chaoskey.o
> >  
> >  obj-$(CONFIG_USB_SISUSBVGA)		+= sisusbvga/
> >  obj-$(CONFIG_USB_LINK_LAYER_TEST)	+= lvstest.o
> > +obj-$(CONFIG_USB_ONBOARD_HUB)		+= generic_onboard_hub.o
> > diff --git a/drivers/usb/misc/generic_onboard_hub.c b/drivers/usb/misc/generic_onboard_hub.c
> > new file mode 100644
> > index 0000000..df722a0
> > --- /dev/null
> > +++ b/drivers/usb/misc/generic_onboard_hub.c
> > @@ -0,0 +1,162 @@
> > +/*
> > + * usb_hub_generic.c	The generic onboard USB HUB driver
> > + *
> > + * Copyright (C) 2015 Freescale Semiconductor, Inc.
> > + * Author: Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > + *
> > + * This program is free software: you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2  of
> > + * the License as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +/*
> > + * This driver is only for the USB HUB devices which need to control
> > + * their external pins(clock, reset, etc), and these USB HUB devices
> > + * are soldered at the board.
> > + */
> > +
> > +#define DEBUG
> 
> nope
> 

Will change.

> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_gpio.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/slab.h>
> > +#include <linux/usb/generic_onboard_hub.h>
> 
> <linux/platform_data/generic_onboard_hub.h> ?
> 

Yes, you are right, I did not know it

> > +struct usb_hub_generic_data {
> > +	struct clk *clk;
> 
> seriously ? Is this really all ? What about that reset line below ?

The clock is PHY input clock on the HUB, this clock may from SoC's PLL.
> 
> In fact, that reset line should be using a generic reset-gpio.c instead
> of a raw gpio.
> 

This reset gpio is not at mainline, I don't know what's reason

http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/186693.html

Philipp, do you know the reason?

> > +static int usb_hub_generic_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct usb_hub_generic_platform_data *pdata = dev->platform_data;
> > +	struct usb_hub_generic_data *hub_data;
> > +	int reset_pol = 0, duration_us = 50, ret = 0;
> > +	struct gpio_desc *gpiod_reset = NULL;
> > +
> > +	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> > +	if (!hub_data)
> > +		return -ENOMEM;
> > +
> > +	if (dev->of_node) {
> > +		struct device_node *node = dev->of_node;
> > +
> > +		hub_data->clk = devm_clk_get(dev, "external_clk");
> > +		if (IS_ERR(hub_data->clk)) {
> > +			dev_dbg(dev, "Can't get external clock: %ld\n",
> > +					PTR_ERR(hub_data->clk));
> 
> how about setting clock to NULL to here ? then you don't need IS_ERR()
> checks anywhere else.
> 
> > +		}
> 
> braces shouldn't be used here, if you add NULL trick above,
> however... then you can keep them.
> 

Braces aren't needed, it may not too much useful to using NULL
as a indicator for error pointer.

> > +		/*
> > +		 * Try to get the information for HUB reset, the
> > +		 * default setting like below:
> > +		 *
> > +		 * - Reset state is low
> > +		 * - The duration is 50us
> > +		 */
> > +		if (of_find_property(node, "hub-reset-active-high", NULL))
> > +			reset_pol = 1;
> 
> you see, this is definitely *not* generic. You should write a generic
> reset-gpio.c reset controller and describe the polarity on the gpio
> binding. This driver *always* uses reset_assert(); reset_deassert(); and
> reset-gpio implements though by gpiod_set_value() correctly.
> 
> Polarity _must_ be described elsewhere in DT.
> 
> > +		of_property_read_u32(node, "hub-reset-duration-us",
> > +			&duration_us);
> 
> likewise, this should be described as a debounce time for the GPIO.
> 

Yes, if we are a reset gpio driver.

> > +		gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
> > +			reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
> > +		ret = PTR_ERR_OR_ZERO(gpiod_reset);
> > +		if (ret) {
> > +			dev_err(dev, "Failed to get reset gpio, err = %d\n",
> > +				ret);
> > +			return ret;
> > +		}
> > +	} else if (pdata) {
> > +		hub_data->clk = pdata->ext_clk;
> > +		duration_us = pdata->gpio_reset_duration_us;
> > +		reset_pol = pdata->gpio_reset_polarity;
> > +
> > +		if (gpio_is_valid(pdata->gpio_reset)) {
> > +			ret = devm_gpio_request_one(
> > +				dev, pdata->gpio_reset, reset_pol
> > +				? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> > +				dev_name(dev));
> > +			if (!ret)
> > +				gpiod_reset = gpio_to_desc(pdata->gpio_reset);
> > +		}
> > +	}
> > +
> > +	if (!IS_ERR(hub_data->clk)) {
> > +		ret = clk_prepare_enable(hub_data->clk);
> > +		if (ret) {
> > +			dev_err(dev,
> > +				"Can't enable external clock: %d\n",
> > +				ret);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	if (gpiod_reset) {
> > +		/* Sanity check */
> > +		if (duration_us > 1000000)
> > +			duration_us = 50;
> 
> this check makes no sense whatsoever. What I *really* need over 1s of
> debounce ?

Ok, I will delete this check.

> 
> > +		usleep_range(duration_us, duration_us + 100);
> > +		gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);
> 
> wrong. You should _not_ have polarity checks here. You should have
> already initialized the gpio as ACTIVE_HIGH or ACTIVE_LOW and gpiolib
> will handle the polarity for you.

Yes, you are right. I did not understand ACTIVE_LOW for gpio flag
before.

> 
> > +	}
> > +
> > +	dev_set_drvdata(dev, hub_data);
> > +	return ret;
> > +}
> > +
> > +static int usb_hub_generic_remove(struct platform_device *pdev)
> > +{
> > +	struct usb_hub_generic_data *hub_data = platform_get_drvdata(pdev);
> > +
> > +	if (!IS_ERR(hub_data->clk))
> > +		clk_disable_unprepare(hub_data->clk);
> > +
> > +	return 0;
> > +}
> > +
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id usb_hub_generic_dt_ids[] = {
> > +	{.compatible = "generic-onboard-hub"},
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, usb_hub_generic_dt_ids);
> > +#endif
> > +
> > +static struct platform_driver usb_hub_generic_driver = {
> > +	.probe = usb_hub_generic_probe,
> > +	.remove = usb_hub_generic_remove,
> > +	.driver = {
> > +		.name = "usb_hub_generic_onboard",
> > +		.of_match_table = usb_hub_generic_dt_ids,
> > +	 },
> > +};
> > +
> > +static int __init usb_hub_generic_init(void)
> > +{
> > +	return platform_driver_register(&usb_hub_generic_driver);
> > +}
> > +subsys_initcall(usb_hub_generic_init);
> > +
> > +static void __exit usb_hub_generic_exit(void)
> > +{
> > +	platform_driver_unregister(&usb_hub_generic_driver);
> > +}
> > +module_exit(usb_hub_generic_exit);
> 
> module_platform_driver();

I want this driver to be called before module init's.

> 
> > +MODULE_AUTHOR("Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>");
> > +MODULE_DESCRIPTION("Generic Onboard USB HUB driver");
> > +MODULE_LICENSE("GPL");
> 
> comment header says GPL v2 only, this is GPL v2+. Which is correct ?
> 

I will change it to GPL v2, thanks.


-- 

Best Regards,
Peter Chen
--
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

[PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Peter Chen]

On Mon, Dec 07, 2015 at 08:59:19PM -0600, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen <peter.chen@freescale.com> writes:
> > diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
> > index 45fd4ac..da52e9a 100644
> > --- a/drivers/usb/misc/Makefile
> > +++ b/drivers/usb/misc/Makefile
> > @@ -29,3 +29,4 @@ obj-$(CONFIG_USB_CHAOSKEY)		+= chaoskey.o
> >  
> >  obj-$(CONFIG_USB_SISUSBVGA)		+= sisusbvga/
> >  obj-$(CONFIG_USB_LINK_LAYER_TEST)	+= lvstest.o
> > +obj-$(CONFIG_USB_ONBOARD_HUB)		+= generic_onboard_hub.o
> > diff --git a/drivers/usb/misc/generic_onboard_hub.c b/drivers/usb/misc/generic_onboard_hub.c
> > new file mode 100644
> > index 0000000..df722a0
> > --- /dev/null
> > +++ b/drivers/usb/misc/generic_onboard_hub.c
> > @@ -0,0 +1,162 @@
> > +/*
> > + * usb_hub_generic.c	The generic onboard USB HUB driver
> > + *
> > + * Copyright (C) 2015 Freescale Semiconductor, Inc.
> > + * Author: Peter Chen <peter.chen@freescale.com>
> > + *
> > + * This program is free software: you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2  of
> > + * the License as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +/*
> > + * This driver is only for the USB HUB devices which need to control
> > + * their external pins(clock, reset, etc), and these USB HUB devices
> > + * are soldered at the board.
> > + */
> > +
> > +#define DEBUG
> 
> nope
> 

Will change.

> > +#include <linux/clk.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_gpio.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/slab.h>
> > +#include <linux/usb/generic_onboard_hub.h>
> 
> <linux/platform_data/generic_onboard_hub.h> ?
> 

Yes, you are right, I did not know it

> > +struct usb_hub_generic_data {
> > +	struct clk *clk;
> 
> seriously ? Is this really all ? What about that reset line below ?

The clock is PHY input clock on the HUB, this clock may from SoC's PLL.
> 
> In fact, that reset line should be using a generic reset-gpio.c instead
> of a raw gpio.
> 

This reset gpio is not at mainline, I don't know what's reason

http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/186693.html

Philipp, do you know the reason?

> > +static int usb_hub_generic_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct usb_hub_generic_platform_data *pdata = dev->platform_data;
> > +	struct usb_hub_generic_data *hub_data;
> > +	int reset_pol = 0, duration_us = 50, ret = 0;
> > +	struct gpio_desc *gpiod_reset = NULL;
> > +
> > +	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> > +	if (!hub_data)
> > +		return -ENOMEM;
> > +
> > +	if (dev->of_node) {
> > +		struct device_node *node = dev->of_node;
> > +
> > +		hub_data->clk = devm_clk_get(dev, "external_clk");
> > +		if (IS_ERR(hub_data->clk)) {
> > +			dev_dbg(dev, "Can't get external clock: %ld\n",
> > +					PTR_ERR(hub_data->clk));
> 
> how about setting clock to NULL to here ? then you don't need IS_ERR()
> checks anywhere else.
> 
> > +		}
> 
> braces shouldn't be used here, if you add NULL trick above,
> however... then you can keep them.
> 

Braces aren't needed, it may not too much useful to using NULL
as a indicator for error pointer.

> > +		/*
> > +		 * Try to get the information for HUB reset, the
> > +		 * default setting like below:
> > +		 *
> > +		 * - Reset state is low
> > +		 * - The duration is 50us
> > +		 */
> > +		if (of_find_property(node, "hub-reset-active-high", NULL))
> > +			reset_pol = 1;
> 
> you see, this is definitely *not* generic. You should write a generic
> reset-gpio.c reset controller and describe the polarity on the gpio
> binding. This driver *always* uses reset_assert(); reset_deassert(); and
> reset-gpio implements though by gpiod_set_value() correctly.
> 
> Polarity _must_ be described elsewhere in DT.
> 
> > +		of_property_read_u32(node, "hub-reset-duration-us",
> > +			&duration_us);
> 
> likewise, this should be described as a debounce time for the GPIO.
> 

Yes, if we are a reset gpio driver.

> > +		gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
> > +			reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
> > +		ret = PTR_ERR_OR_ZERO(gpiod_reset);
> > +		if (ret) {
> > +			dev_err(dev, "Failed to get reset gpio, err = %d\n",
> > +				ret);
> > +			return ret;
> > +		}
> > +	} else if (pdata) {
> > +		hub_data->clk = pdata->ext_clk;
> > +		duration_us = pdata->gpio_reset_duration_us;
> > +		reset_pol = pdata->gpio_reset_polarity;
> > +
> > +		if (gpio_is_valid(pdata->gpio_reset)) {
> > +			ret = devm_gpio_request_one(
> > +				dev, pdata->gpio_reset, reset_pol
> > +				? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> > +				dev_name(dev));
> > +			if (!ret)
> > +				gpiod_reset = gpio_to_desc(pdata->gpio_reset);
> > +		}
> > +	}
> > +
> > +	if (!IS_ERR(hub_data->clk)) {
> > +		ret = clk_prepare_enable(hub_data->clk);
> > +		if (ret) {
> > +			dev_err(dev,
> > +				"Can't enable external clock: %d\n",
> > +				ret);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	if (gpiod_reset) {
> > +		/* Sanity check */
> > +		if (duration_us > 1000000)
> > +			duration_us = 50;
> 
> this check makes no sense whatsoever. What I *really* need over 1s of
> debounce ?

Ok, I will delete this check.

> 
> > +		usleep_range(duration_us, duration_us + 100);
> > +		gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);
> 
> wrong. You should _not_ have polarity checks here. You should have
> already initialized the gpio as ACTIVE_HIGH or ACTIVE_LOW and gpiolib
> will handle the polarity for you.

Yes, you are right. I did not understand ACTIVE_LOW for gpio flag
before.

> 
> > +	}
> > +
> > +	dev_set_drvdata(dev, hub_data);
> > +	return ret;
> > +}
> > +
> > +static int usb_hub_generic_remove(struct platform_device *pdev)
> > +{
> > +	struct usb_hub_generic_data *hub_data = platform_get_drvdata(pdev);
> > +
> > +	if (!IS_ERR(hub_data->clk))
> > +		clk_disable_unprepare(hub_data->clk);
> > +
> > +	return 0;
> > +}
> > +
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id usb_hub_generic_dt_ids[] = {
> > +	{.compatible = "generic-onboard-hub"},
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, usb_hub_generic_dt_ids);
> > +#endif
> > +
> > +static struct platform_driver usb_hub_generic_driver = {
> > +	.probe = usb_hub_generic_probe,
> > +	.remove = usb_hub_generic_remove,
> > +	.driver = {
> > +		.name = "usb_hub_generic_onboard",
> > +		.of_match_table = usb_hub_generic_dt_ids,
> > +	 },
> > +};
> > +
> > +static int __init usb_hub_generic_init(void)
> > +{
> > +	return platform_driver_register(&usb_hub_generic_driver);
> > +}
> > +subsys_initcall(usb_hub_generic_init);
> > +
> > +static void __exit usb_hub_generic_exit(void)
> > +{
> > +	platform_driver_unregister(&usb_hub_generic_driver);
> > +}
> > +module_exit(usb_hub_generic_exit);
> 
> module_platform_driver();

I want this driver to be called before module init's.

> 
> > +MODULE_AUTHOR("Peter Chen <peter.chen@freescale.com>");
> > +MODULE_DESCRIPTION("Generic Onboard USB HUB driver");
> > +MODULE_LICENSE("GPL");
> 
> comment header says GPL v2 only, this is GPL v2+. Which is correct ?
> 

I will change it to GPL v2, thanks.


-- 

Best Regards,
Peter Chen

Re: [PATCH 2/3] doc: dt-binding: generic onboard USB HUB

[Peter Chen]

On Tue, Dec 08, 2015 at 12:30:59AM -0200, Fabio Estevam wrote:
> On Mon, Dec 7, 2015 at 11:37 PM, Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > Add dt-binding documentation for generic onboard USB HUB.
> >
> > Signed-off-by: Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > ---
> >  .../bindings/usb/generic-onboard-hub.txt           | 28 ++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> >
> > diff --git a/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > new file mode 100644
> > index 0000000..ea92205
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > @@ -0,0 +1,28 @@
> > +Generic Onboard USB HUB
> > +
> > +Required properties:
> > +- compatible: should be "generic-onboard-hub"
> > +
> > +Optional properties:
> > +- clocks: the input clock for HUB.
> > +
> > +- clock-names: Should be "external_clk"
> > +
> > +- hub-reset-gpios: Should specify the GPIO for reset.
> > +
> > +- hub-reset-active-high: the active reset signal is high, if this property is
> > +  not set, the active reset signal is low.
> > +
> > +- hub-reset-duration-us: the duration for assert reset signal, the time unit
> > +  is microsecond.
> > +
> > +Example:
> > +
> > +       usb_hub1 {
> > +               compatible = "generic-onboard-hub";
> > +               clocks = <&clks IMX6QDL_CLK_CKO>;
> > +               clock-names = "external_clk";
> > +               hub-reset-gpios = <&gpio7 12 0>;
> > +               hub-reset-active-high;
> 
> I think you could drop the 'hub-reset-active-high' property and do
> like this instead:
> 
> hub-reset-gpios = <&gpio7 12 GPIO_ACTIVE_HIGH>;
> 
> or
> 
> hub-reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;

Yes, you are right, would you have a test to see if can work for
udoo board?

-- 

Best Regards,
Peter Chen
--
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

[PATCH 2/3] doc: dt-binding: generic onboard USB HUB

[Peter Chen]

On Tue, Dec 08, 2015 at 12:30:59AM -0200, Fabio Estevam wrote:
> On Mon, Dec 7, 2015 at 11:37 PM, Peter Chen <peter.chen@freescale.com> wrote:
> > Add dt-binding documentation for generic onboard USB HUB.
> >
> > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > ---
> >  .../bindings/usb/generic-onboard-hub.txt           | 28 ++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> >
> > diff --git a/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > new file mode 100644
> > index 0000000..ea92205
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > @@ -0,0 +1,28 @@
> > +Generic Onboard USB HUB
> > +
> > +Required properties:
> > +- compatible: should be "generic-onboard-hub"
> > +
> > +Optional properties:
> > +- clocks: the input clock for HUB.
> > +
> > +- clock-names: Should be "external_clk"
> > +
> > +- hub-reset-gpios: Should specify the GPIO for reset.
> > +
> > +- hub-reset-active-high: the active reset signal is high, if this property is
> > +  not set, the active reset signal is low.
> > +
> > +- hub-reset-duration-us: the duration for assert reset signal, the time unit
> > +  is microsecond.
> > +
> > +Example:
> > +
> > +       usb_hub1 {
> > +               compatible = "generic-onboard-hub";
> > +               clocks = <&clks IMX6QDL_CLK_CKO>;
> > +               clock-names = "external_clk";
> > +               hub-reset-gpios = <&gpio7 12 0>;
> > +               hub-reset-active-high;
> 
> I think you could drop the 'hub-reset-active-high' property and do
> like this instead:
> 
> hub-reset-gpios = <&gpio7 12 GPIO_ACTIVE_HIGH>;
> 
> or
> 
> hub-reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;

Yes, you are right, would you have a test to see if can work for
udoo board?

-- 

Best Regards,
Peter Chen

Re: [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Peter Chen]

On Tue, Dec 08, 2015 at 07:19:05AM +0100, Sascha Hauer wrote:
> > +	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> > +	if (!hub_data)
> > +		return -ENOMEM;
> > +
> > +	if (dev->of_node) {
> > +		struct device_node *node = dev->of_node;
> > +
> > +		hub_data->clk = devm_clk_get(dev, "external_clk");
> 
> Please drop such _clk suffixes. The context already makes it clear that
> it's a clock.
> 

Will change.

> > +		if (IS_ERR(hub_data->clk)) {
> > +			dev_dbg(dev, "Can't get external clock: %ld\n",
> > +					PTR_ERR(hub_data->clk));
> > +		}
> > +
> > +		/*
> > +		 * Try to get the information for HUB reset, the
> > +		 * default setting like below:
> > +		 *
> > +		 * - Reset state is low
> > +		 * - The duration is 50us
> > +		 */
> > +		if (of_find_property(node, "hub-reset-active-high", NULL))
> > +			reset_pol = 1;
> > +
> > +		of_property_read_u32(node, "hub-reset-duration-us",
> > +			&duration_us);
> > +
> > +		gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
> > +			reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
> > +		ret = PTR_ERR_OR_ZERO(gpiod_reset);
> > +		if (ret) {
> > +			dev_err(dev, "Failed to get reset gpio, err = %d\n",
> > +				ret);
> > +			return ret;
> > +		}
> > +	} else if (pdata) {
> > +		hub_data->clk = pdata->ext_clk;
> 
> Passing clocks in platform_data is a no go. clocks must always be
> retrieved with clk_get.

Yes, you are right.

> 
> > +		duration_us = pdata->gpio_reset_duration_us;
> > +		reset_pol = pdata->gpio_reset_polarity;
> > +
> > +		if (gpio_is_valid(pdata->gpio_reset)) {
> > +			ret = devm_gpio_request_one(
> > +				dev, pdata->gpio_reset, reset_pol
> > +				? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> > +				dev_name(dev));
> > +			if (!ret)
> > +				gpiod_reset = gpio_to_desc(pdata->gpio_reset);
> 
> If the gpio is valid then being unable to get it is an error.

I can't catch you, if the gpio is invalid, its descriptor will be NULL,
then, the code will not do any gpio operation.

> 
> Do you need this platform_data stuff at all?
> 

If not, how can I cover the platform which does not use dts, but still
uses these kinds of HUBs?

-- 

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

[PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Peter Chen]

On Tue, Dec 08, 2015 at 07:19:05AM +0100, Sascha Hauer wrote:
> > +	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> > +	if (!hub_data)
> > +		return -ENOMEM;
> > +
> > +	if (dev->of_node) {
> > +		struct device_node *node = dev->of_node;
> > +
> > +		hub_data->clk = devm_clk_get(dev, "external_clk");
> 
> Please drop such _clk suffixes. The context already makes it clear that
> it's a clock.
> 

Will change.

> > +		if (IS_ERR(hub_data->clk)) {
> > +			dev_dbg(dev, "Can't get external clock: %ld\n",
> > +					PTR_ERR(hub_data->clk));
> > +		}
> > +
> > +		/*
> > +		 * Try to get the information for HUB reset, the
> > +		 * default setting like below:
> > +		 *
> > +		 * - Reset state is low
> > +		 * - The duration is 50us
> > +		 */
> > +		if (of_find_property(node, "hub-reset-active-high", NULL))
> > +			reset_pol = 1;
> > +
> > +		of_property_read_u32(node, "hub-reset-duration-us",
> > +			&duration_us);
> > +
> > +		gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
> > +			reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
> > +		ret = PTR_ERR_OR_ZERO(gpiod_reset);
> > +		if (ret) {
> > +			dev_err(dev, "Failed to get reset gpio, err = %d\n",
> > +				ret);
> > +			return ret;
> > +		}
> > +	} else if (pdata) {
> > +		hub_data->clk = pdata->ext_clk;
> 
> Passing clocks in platform_data is a no go. clocks must always be
> retrieved with clk_get.

Yes, you are right.

> 
> > +		duration_us = pdata->gpio_reset_duration_us;
> > +		reset_pol = pdata->gpio_reset_polarity;
> > +
> > +		if (gpio_is_valid(pdata->gpio_reset)) {
> > +			ret = devm_gpio_request_one(
> > +				dev, pdata->gpio_reset, reset_pol
> > +				? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> > +				dev_name(dev));
> > +			if (!ret)
> > +				gpiod_reset = gpio_to_desc(pdata->gpio_reset);
> 
> If the gpio is valid then being unable to get it is an error.

I can't catch you, if the gpio is invalid, its descriptor will be NULL,
then, the code will not do any gpio operation.

> 
> Do you need this platform_data stuff at all?
> 

If not, how can I cover the platform which does not use dts, but still
uses these kinds of HUBs?

-- 

Best Regards,
Peter Chen

Re: [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Peter Chen]

On Tue, Dec 08, 2015 at 11:16:31AM +0800, kbuild test robot wrote:
> Hi Peter,
> 
> [auto build test ERROR on usb/usb-testing]
> [also build test ERROR on v4.4-rc4 next-20151207]
> 
> url:    https://github.com/0day-ci/linux/commits/Peter-Chen/USB-add-generic-onboard-USB-HUB-driver/20151208-094428
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
> config: tile-allmodconfig (attached as .config)
> reproduce:
>         wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=tile 
> 
> All error/warnings (new ones prefixed by >>):
> 
>    drivers/usb/misc/generic_onboard_hub.c: In function 'usb_hub_generic_probe':
> >> drivers/usb/misc/generic_onboard_hub.c:76:3: error: implicit declaration of function 'devm_gpiod_get_optional'
> >> drivers/usb/misc/generic_onboard_hub.c:77:16: error: 'GPIOD_OUT_HIGH' undeclared (first use in this function)
>    drivers/usb/misc/generic_onboard_hub.c:77:16: note: each undeclared identifier is reported only once for each function it appears in
> >> drivers/usb/misc/generic_onboard_hub.c:77:33: error: 'GPIOD_OUT_LOW' undeclared (first use in this function)
> >> drivers/usb/misc/generic_onboard_hub.c:95:5: error: implicit declaration of function 'gpio_to_desc'
> >> drivers/usb/misc/generic_onboard_hub.c:95:17: warning: assignment makes pointer from integer without a cast [enabled by default]
> >> drivers/usb/misc/generic_onboard_hub.c:114:3: error: implicit declaration of function 'gpiod_set_value'
>    cc1: some warnings being treated as errors
> 
> vim +/devm_gpiod_get_optional +76 drivers/usb/misc/generic_onboard_hub.c
> 
>     70			if (of_find_property(node, "hub-reset-active-high", NULL))
>     71				reset_pol = 1;
>     72	
>     73			of_property_read_u32(node, "hub-reset-duration-us",
>     74				&duration_us);
>     75	
>   > 76			gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
>   > 77				reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
>     78			ret = PTR_ERR_OR_ZERO(gpiod_reset);
>     79			if (ret) {
>     80				dev_err(dev, "Failed to get reset gpio, err = %d\n",
>     81					ret);
>     82				return ret;
>     83			}
>     84		} else if (pdata) {
>     85			hub_data->clk = pdata->ext_clk;
>     86			duration_us = pdata->gpio_reset_duration_us;
>     87			reset_pol = pdata->gpio_reset_polarity;
>     88	
>     89			if (gpio_is_valid(pdata->gpio_reset)) {
>     90				ret = devm_gpio_request_one(
>     91					dev, pdata->gpio_reset, reset_pol
>     92					? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
>     93					dev_name(dev));
>     94				if (!ret)
>   > 95					gpiod_reset = gpio_to_desc(pdata->gpio_reset);
>     96			}
>     97		}
>     98	
>     99		if (!IS_ERR(hub_data->clk)) {
>    100			ret = clk_prepare_enable(hub_data->clk);
>    101			if (ret) {
>    102				dev_err(dev,
>    103					"Can't enable external clock: %d\n",
>    104					ret);
>    105				return ret;
>    106			}
>    107		}
>    108	
>    109		if (gpiod_reset) {
>    110			/* Sanity check */
>    111			if (duration_us > 1000000)
>    112				duration_us = 50;
>    113			usleep_range(duration_us, duration_us + 100);
>  > 114			gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);
>    115		}
>    116	
>    117		dev_set_drvdata(dev, hub_data);
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

I will add <linux/gpio/consumer.h> at header file list.


-- 

Best Regards,
Peter Chen
--
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

[PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Peter Chen]

On Tue, Dec 08, 2015 at 11:16:31AM +0800, kbuild test robot wrote:
> Hi Peter,
> 
> [auto build test ERROR on usb/usb-testing]
> [also build test ERROR on v4.4-rc4 next-20151207]
> 
> url:    https://github.com/0day-ci/linux/commits/Peter-Chen/USB-add-generic-onboard-USB-HUB-driver/20151208-094428
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
> config: tile-allmodconfig (attached as .config)
> reproduce:
>         wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=tile 
> 
> All error/warnings (new ones prefixed by >>):
> 
>    drivers/usb/misc/generic_onboard_hub.c: In function 'usb_hub_generic_probe':
> >> drivers/usb/misc/generic_onboard_hub.c:76:3: error: implicit declaration of function 'devm_gpiod_get_optional'
> >> drivers/usb/misc/generic_onboard_hub.c:77:16: error: 'GPIOD_OUT_HIGH' undeclared (first use in this function)
>    drivers/usb/misc/generic_onboard_hub.c:77:16: note: each undeclared identifier is reported only once for each function it appears in
> >> drivers/usb/misc/generic_onboard_hub.c:77:33: error: 'GPIOD_OUT_LOW' undeclared (first use in this function)
> >> drivers/usb/misc/generic_onboard_hub.c:95:5: error: implicit declaration of function 'gpio_to_desc'
> >> drivers/usb/misc/generic_onboard_hub.c:95:17: warning: assignment makes pointer from integer without a cast [enabled by default]
> >> drivers/usb/misc/generic_onboard_hub.c:114:3: error: implicit declaration of function 'gpiod_set_value'
>    cc1: some warnings being treated as errors
> 
> vim +/devm_gpiod_get_optional +76 drivers/usb/misc/generic_onboard_hub.c
> 
>     70			if (of_find_property(node, "hub-reset-active-high", NULL))
>     71				reset_pol = 1;
>     72	
>     73			of_property_read_u32(node, "hub-reset-duration-us",
>     74				&duration_us);
>     75	
>   > 76			gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
>   > 77				reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
>     78			ret = PTR_ERR_OR_ZERO(gpiod_reset);
>     79			if (ret) {
>     80				dev_err(dev, "Failed to get reset gpio, err = %d\n",
>     81					ret);
>     82				return ret;
>     83			}
>     84		} else if (pdata) {
>     85			hub_data->clk = pdata->ext_clk;
>     86			duration_us = pdata->gpio_reset_duration_us;
>     87			reset_pol = pdata->gpio_reset_polarity;
>     88	
>     89			if (gpio_is_valid(pdata->gpio_reset)) {
>     90				ret = devm_gpio_request_one(
>     91					dev, pdata->gpio_reset, reset_pol
>     92					? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
>     93					dev_name(dev));
>     94				if (!ret)
>   > 95					gpiod_reset = gpio_to_desc(pdata->gpio_reset);
>     96			}
>     97		}
>     98	
>     99		if (!IS_ERR(hub_data->clk)) {
>    100			ret = clk_prepare_enable(hub_data->clk);
>    101			if (ret) {
>    102				dev_err(dev,
>    103					"Can't enable external clock: %d\n",
>    104					ret);
>    105				return ret;
>    106			}
>    107		}
>    108	
>    109		if (gpiod_reset) {
>    110			/* Sanity check */
>    111			if (duration_us > 1000000)
>    112				duration_us = 50;
>    113			usleep_range(duration_us, duration_us + 100);
>  > 114			gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);
>    115		}
>    116	
>    117		dev_set_drvdata(dev, hub_data);
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

I will add <linux/gpio/consumer.h> at header file list.


-- 

Best Regards,
Peter Chen

Re: [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Sascha Hauer]

On Tue, Dec 08, 2015 at 05:26:56PM +0800, Peter Chen wrote:
> On Tue, Dec 08, 2015 at 07:19:05AM +0100, Sascha Hauer wrote:
> > > +	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> > > +	if (!hub_data)
> > > +		return -ENOMEM;
> > > +
> > > +	if (dev->of_node) {
> > > +		struct device_node *node = dev->of_node;
> > > +
> > > +		hub_data->clk = devm_clk_get(dev, "external_clk");
> > 
> > Please drop such _clk suffixes. The context already makes it clear that
> > it's a clock.
> > 
> 
> Will change.
> 
> > > +		if (IS_ERR(hub_data->clk)) {
> > > +			dev_dbg(dev, "Can't get external clock: %ld\n",
> > > +					PTR_ERR(hub_data->clk));
> > > +		}
> > > +
> > > +		/*
> > > +		 * Try to get the information for HUB reset, the
> > > +		 * default setting like below:
> > > +		 *
> > > +		 * - Reset state is low
> > > +		 * - The duration is 50us
> > > +		 */
> > > +		if (of_find_property(node, "hub-reset-active-high", NULL))
> > > +			reset_pol = 1;
> > > +
> > > +		of_property_read_u32(node, "hub-reset-duration-us",
> > > +			&duration_us);
> > > +
> > > +		gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
> > > +			reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
> > > +		ret = PTR_ERR_OR_ZERO(gpiod_reset);
> > > +		if (ret) {
> > > +			dev_err(dev, "Failed to get reset gpio, err = %d\n",
> > > +				ret);
> > > +			return ret;
> > > +		}
> > > +	} else if (pdata) {
> > > +		hub_data->clk = pdata->ext_clk;
> > 
> > Passing clocks in platform_data is a no go. clocks must always be
> > retrieved with clk_get.
> 
> Yes, you are right.
> 
> > 
> > > +		duration_us = pdata->gpio_reset_duration_us;
> > > +		reset_pol = pdata->gpio_reset_polarity;
> > > +
> > > +		if (gpio_is_valid(pdata->gpio_reset)) {
> > > +			ret = devm_gpio_request_one(
> > > +				dev, pdata->gpio_reset, reset_pol
> > > +				? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> > > +				dev_name(dev));
> > > +			if (!ret)
> > > +				gpiod_reset = gpio_to_desc(pdata->gpio_reset);
> > 
> > If the gpio is valid then being unable to get it is an error.
> 
> I can't catch you, if the gpio is invalid, its descriptor will be NULL,
> then, the code will not do any gpio operation.

When the platform_data provides a gpio (gpio_is_valid() is true) then
you must use the gpio. If then devm_gpio_request_one() fails you must
bail out.

> 
> > 
> > Do you need this platform_data stuff at all?
> > 
> 
> If not, how can I cover the platform which does not use dts, but still
> uses these kinds of HUBs?

You can't, but are there any non device tree platforms which want to use
this driver?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
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

[PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Sascha Hauer]

On Tue, Dec 08, 2015 at 05:26:56PM +0800, Peter Chen wrote:
> On Tue, Dec 08, 2015 at 07:19:05AM +0100, Sascha Hauer wrote:
> > > +	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> > > +	if (!hub_data)
> > > +		return -ENOMEM;
> > > +
> > > +	if (dev->of_node) {
> > > +		struct device_node *node = dev->of_node;
> > > +
> > > +		hub_data->clk = devm_clk_get(dev, "external_clk");
> > 
> > Please drop such _clk suffixes. The context already makes it clear that
> > it's a clock.
> > 
> 
> Will change.
> 
> > > +		if (IS_ERR(hub_data->clk)) {
> > > +			dev_dbg(dev, "Can't get external clock: %ld\n",
> > > +					PTR_ERR(hub_data->clk));
> > > +		}
> > > +
> > > +		/*
> > > +		 * Try to get the information for HUB reset, the
> > > +		 * default setting like below:
> > > +		 *
> > > +		 * - Reset state is low
> > > +		 * - The duration is 50us
> > > +		 */
> > > +		if (of_find_property(node, "hub-reset-active-high", NULL))
> > > +			reset_pol = 1;
> > > +
> > > +		of_property_read_u32(node, "hub-reset-duration-us",
> > > +			&duration_us);
> > > +
> > > +		gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
> > > +			reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
> > > +		ret = PTR_ERR_OR_ZERO(gpiod_reset);
> > > +		if (ret) {
> > > +			dev_err(dev, "Failed to get reset gpio, err = %d\n",
> > > +				ret);
> > > +			return ret;
> > > +		}
> > > +	} else if (pdata) {
> > > +		hub_data->clk = pdata->ext_clk;
> > 
> > Passing clocks in platform_data is a no go. clocks must always be
> > retrieved with clk_get.
> 
> Yes, you are right.
> 
> > 
> > > +		duration_us = pdata->gpio_reset_duration_us;
> > > +		reset_pol = pdata->gpio_reset_polarity;
> > > +
> > > +		if (gpio_is_valid(pdata->gpio_reset)) {
> > > +			ret = devm_gpio_request_one(
> > > +				dev, pdata->gpio_reset, reset_pol
> > > +				? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> > > +				dev_name(dev));
> > > +			if (!ret)
> > > +				gpiod_reset = gpio_to_desc(pdata->gpio_reset);
> > 
> > If the gpio is valid then being unable to get it is an error.
> 
> I can't catch you, if the gpio is invalid, its descriptor will be NULL,
> then, the code will not do any gpio operation.

When the platform_data provides a gpio (gpio_is_valid() is true) then
you must use the gpio. If then devm_gpio_request_one() fails you must
bail out.

> 
> > 
> > Do you need this platform_data stuff at all?
> > 
> 
> If not, how can I cover the platform which does not use dts, but still
> uses these kinds of HUBs?

You can't, but are there any non device tree platforms which want to use
this driver?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/3] doc: dt-binding: generic onboard USB HUB

[Arnd Bergmann]

On Tuesday 08 December 2015 17:20:46 Peter Chen wrote:
> On Tue, Dec 08, 2015 at 12:30:59AM -0200, Fabio Estevam wrote:
> > On Mon, Dec 7, 2015 at 11:37 PM, Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > > Add dt-binding documentation for generic onboard USB HUB.
> > >
> > > Signed-off-by: Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > > ---
> > >  .../bindings/usb/generic-onboard-hub.txt           | 28 ++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > > new file mode 100644
> > > index 0000000..ea92205
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > > @@ -0,0 +1,28 @@
> > > +Generic Onboard USB HUB
> > > +
> > > +Required properties:
> > > +- compatible: should be "generic-onboard-hub"
> > > +
> > > +Optional properties:
> > > +- clocks: the input clock for HUB.
> > > +
> > > +- clock-names: Should be "external_clk"

Make this just "external", or remove the name and use it as an anonymous clock.

> > > +- hub-reset-gpios: Should specify the GPIO for reset.
> > > +
> > > +- hub-reset-active-high: the active reset signal is high, if this property is
> > > +  not set, the active reset signal is low.
> > > +
> > > +- hub-reset-duration-us: the duration for assert reset signal, the time unit
> > > +  is microsecond.

Remove the "hub-" prefix here.

> > > +Example:
> > > +
> > > +       usb_hub1 {
> > > +               compatible = "generic-onboard-hub";
> > > +               clocks = <&clks IMX6QDL_CLK_CKO>;
> > > +               clock-names = "external_clk";
> > > +               hub-reset-gpios = <&gpio7 12 0>;
> > > +               hub-reset-active-high;
> > 
> > I think you could drop the 'hub-reset-active-high' property and do
> > like this instead:
> > 
> > hub-reset-gpios = <&gpio7 12 GPIO_ACTIVE_HIGH>;
> > 
> > or
> > 
> > hub-reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;
> 
> Yes, you are right, would you have a test to see if can work for
> udoo board?

Yes, it should be done like this.

	Arnd
--
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

[PATCH 2/3] doc: dt-binding: generic onboard USB HUB

[Arnd Bergmann]

On Tuesday 08 December 2015 17:20:46 Peter Chen wrote:
> On Tue, Dec 08, 2015 at 12:30:59AM -0200, Fabio Estevam wrote:
> > On Mon, Dec 7, 2015 at 11:37 PM, Peter Chen <peter.chen@freescale.com> wrote:
> > > Add dt-binding documentation for generic onboard USB HUB.
> > >
> > > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > > ---
> > >  .../bindings/usb/generic-onboard-hub.txt           | 28 ++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > > new file mode 100644
> > > index 0000000..ea92205
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > > @@ -0,0 +1,28 @@
> > > +Generic Onboard USB HUB
> > > +
> > > +Required properties:
> > > +- compatible: should be "generic-onboard-hub"
> > > +
> > > +Optional properties:
> > > +- clocks: the input clock for HUB.
> > > +
> > > +- clock-names: Should be "external_clk"

Make this just "external", or remove the name and use it as an anonymous clock.

> > > +- hub-reset-gpios: Should specify the GPIO for reset.
> > > +
> > > +- hub-reset-active-high: the active reset signal is high, if this property is
> > > +  not set, the active reset signal is low.
> > > +
> > > +- hub-reset-duration-us: the duration for assert reset signal, the time unit
> > > +  is microsecond.

Remove the "hub-" prefix here.

> > > +Example:
> > > +
> > > +       usb_hub1 {
> > > +               compatible = "generic-onboard-hub";
> > > +               clocks = <&clks IMX6QDL_CLK_CKO>;
> > > +               clock-names = "external_clk";
> > > +               hub-reset-gpios = <&gpio7 12 0>;
> > > +               hub-reset-active-high;
> > 
> > I think you could drop the 'hub-reset-active-high' property and do
> > like this instead:
> > 
> > hub-reset-gpios = <&gpio7 12 GPIO_ACTIVE_HIGH>;
> > 
> > or
> > 
> > hub-reset-gpios = <&gpio7 12 GPIO_ACTIVE_LOW>;
> 
> Yes, you are right, would you have a test to see if can work for
> udoo board?

Yes, it should be done like this.

	Arnd

Re: [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Arnd Bergmann]

On Tuesday 08 December 2015 09:37:48 Peter Chen wrote:

> +struct usb_hub_generic_data {
> +	struct clk *clk;
> +};
> +
> +static int usb_hub_generic_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct usb_hub_generic_platform_data *pdata = dev->platform_data;
> +	struct usb_hub_generic_data *hub_data;
> +	int reset_pol = 0, duration_us = 50, ret = 0;
> +	struct gpio_desc *gpiod_reset = NULL;
> +
> +	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> +	if (!hub_data)
> +		return -ENOMEM;
> +
> +	if (dev->of_node) {

Let's not worry about the !DT case until someone adds a board file
that needs it. Just remove the if() here along and the whole else
block.

> +#ifdef CONFIG_OF
> +static const struct of_device_id usb_hub_generic_dt_ids[] = {
> +	{.compatible = "generic-onboard-hub"},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, usb_hub_generic_dt_ids);
> +#endif
> +
> +static struct platform_driver usb_hub_generic_driver = {
> +	.probe = usb_hub_generic_probe,
> +	.remove = usb_hub_generic_remove,
> +	.driver = {
> +		.name = "usb_hub_generic_onboard",
> +		.of_match_table = usb_hub_generic_dt_ids,
> +	 },
> +};

Build error when CONFIG_OF is disabled: Please remove the #ifdef around the device
table.

> diff --git a/include/linux/usb/generic_onboard_hub.h b/include/linux/usb/generic_onboard_hub.h
> new file mode 100644
> index 0000000..1b70ccc
> --- /dev/null
> +++ b/include/linux/usb/generic_onboard_hub.h
> @@ -0,0 +1,11 @@
> +#ifndef __LINUX_USB_GENERIC_HUB_H
> +#define __LINUX_USB_GENERIC_HUB_H
> +
> +struct usb_hub_generic_platform_data {
> +	int gpio_reset;
> +	int gpio_reset_polarity;
> +	int gpio_reset_duration_us;
> +	struct clk *ext_clk;
> +};

Merge this structure into struct usb_hub_generic_data and remove the header.

	ARnd
--
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

[PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Arnd Bergmann]

On Tuesday 08 December 2015 09:37:48 Peter Chen wrote:

> +struct usb_hub_generic_data {
> +	struct clk *clk;
> +};
> +
> +static int usb_hub_generic_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct usb_hub_generic_platform_data *pdata = dev->platform_data;
> +	struct usb_hub_generic_data *hub_data;
> +	int reset_pol = 0, duration_us = 50, ret = 0;
> +	struct gpio_desc *gpiod_reset = NULL;
> +
> +	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> +	if (!hub_data)
> +		return -ENOMEM;
> +
> +	if (dev->of_node) {

Let's not worry about the !DT case until someone adds a board file
that needs it. Just remove the if() here along and the whole else
block.

> +#ifdef CONFIG_OF
> +static const struct of_device_id usb_hub_generic_dt_ids[] = {
> +	{.compatible = "generic-onboard-hub"},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, usb_hub_generic_dt_ids);
> +#endif
> +
> +static struct platform_driver usb_hub_generic_driver = {
> +	.probe = usb_hub_generic_probe,
> +	.remove = usb_hub_generic_remove,
> +	.driver = {
> +		.name = "usb_hub_generic_onboard",
> +		.of_match_table = usb_hub_generic_dt_ids,
> +	 },
> +};

Build error when CONFIG_OF is disabled: Please remove the #ifdef around the device
table.

> diff --git a/include/linux/usb/generic_onboard_hub.h b/include/linux/usb/generic_onboard_hub.h
> new file mode 100644
> index 0000000..1b70ccc
> --- /dev/null
> +++ b/include/linux/usb/generic_onboard_hub.h
> @@ -0,0 +1,11 @@
> +#ifndef __LINUX_USB_GENERIC_HUB_H
> +#define __LINUX_USB_GENERIC_HUB_H
> +
> +struct usb_hub_generic_platform_data {
> +	int gpio_reset;
> +	int gpio_reset_polarity;
> +	int gpio_reset_duration_us;
> +	struct clk *ext_clk;
> +};

Merge this structure into struct usb_hub_generic_data and remove the header.

	ARnd

Re: [PATCH 2/3] doc: dt-binding: generic onboard USB HUB

[Philipp Zabel]

Hi Peter,

Am Dienstag, den 08.12.2015, 09:37 +0800 schrieb Peter Chen:
> Add dt-binding documentation for generic onboard USB HUB.
> 
> Signed-off-by: Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  .../bindings/usb/generic-onboard-hub.txt           | 28 ++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> 
> diff --git a/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> new file mode 100644
> index 0000000..ea92205
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> @@ -0,0 +1,28 @@
> +Generic Onboard USB HUB
>+
> +Required properties:
> +- compatible: should be "generic-onboard-hub"

This something we don't have to define ad-hoc. The hub hangs off an USB
controller, right? The "Open Firmware recommended practice: USB"
document already describes how to represent USB devices in a generic
manner:
http://www.firmware.org/1275/bindings/usb/usb-1_0.ps

Is there a reason not to reuse this?

The usb hub node would be a child of the usb controller node, and it
could use
	compatible = "usb,class9"; /* bDeviceClass 9 (Hub) */


> +Optional properties:
> +- clocks: the input clock for HUB.
> +
> +- clock-names: Should be "external_clk"

Is clock-names necessary for a single clock?

> +- hub-reset-gpios: Should specify the GPIO for reset.

I'd prefer that to be just "reset-gpios", it is the only reset property
in the hub node after all. And use the GPIO_ACTIVE_HIGH/LOW flags to
indicate polarity.

> +- hub-reset-active-high: the active reset signal is high, if this property is
> +  not set, the active reset signal is low.

Then this could be dropped.

> +- hub-reset-duration-us: the duration for assert reset signal, the time unit
> +  is microsecond.

And consequently, this could just be called "reset-duration-us".
It might make sense to define the same for other reset GPIOs, too.
The Freescale FEC, for example, has "phy-reset-duration" (in ms)
already.

> +
> +Example:
> +
> +	usb_hub1 {
> +		compatible = "generic-onboard-hub";
> +		clocks = <&clks IMX6QDL_CLK_CKO>;
> +		clock-names = "external_clk";
> +		hub-reset-gpios = <&gpio7 12 0>;
> +		hub-reset-active-high;
> +		hub-reset-duration-us = <2>;
> +	};

best regards
Philipp

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

[PATCH 2/3] doc: dt-binding: generic onboard USB HUB

[Philipp Zabel]

Hi Peter,

Am Dienstag, den 08.12.2015, 09:37 +0800 schrieb Peter Chen:
> Add dt-binding documentation for generic onboard USB HUB.
> 
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
>  .../bindings/usb/generic-onboard-hub.txt           | 28 ++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> 
> diff --git a/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> new file mode 100644
> index 0000000..ea92205
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> @@ -0,0 +1,28 @@
> +Generic Onboard USB HUB
>+
> +Required properties:
> +- compatible: should be "generic-onboard-hub"

This something we don't have to define ad-hoc. The hub hangs off an USB
controller, right? The "Open Firmware recommended practice: USB"
document already describes how to represent USB devices in a generic
manner:
http://www.firmware.org/1275/bindings/usb/usb-1_0.ps

Is there a reason not to reuse this?

The usb hub node would be a child of the usb controller node, and it
could use
	compatible = "usb,class9"; /* bDeviceClass 9 (Hub) */


> +Optional properties:
> +- clocks: the input clock for HUB.
> +
> +- clock-names: Should be "external_clk"

Is clock-names necessary for a single clock?

> +- hub-reset-gpios: Should specify the GPIO for reset.

I'd prefer that to be just "reset-gpios", it is the only reset property
in the hub node after all. And use the GPIO_ACTIVE_HIGH/LOW flags to
indicate polarity.

> +- hub-reset-active-high: the active reset signal is high, if this property is
> +  not set, the active reset signal is low.

Then this could be dropped.

> +- hub-reset-duration-us: the duration for assert reset signal, the time unit
> +  is microsecond.

And consequently, this could just be called "reset-duration-us".
It might make sense to define the same for other reset GPIOs, too.
The Freescale FEC, for example, has "phy-reset-duration" (in ms)
already.

> +
> +Example:
> +
> +	usb_hub1 {
> +		compatible = "generic-onboard-hub";
> +		clocks = <&clks IMX6QDL_CLK_CKO>;
> +		clock-names = "external_clk";
> +		hub-reset-gpios = <&gpio7 12 0>;
> +		hub-reset-active-high;
> +		hub-reset-duration-us = <2>;
> +	};

best regards
Philipp

Re: [PATCH 2/3] doc: dt-binding: generic onboard USB HUB

[Arnd Bergmann]

On Tuesday 08 December 2015 10:50:49 Philipp Zabel wrote:
> Am Dienstag, den 08.12.2015, 09:37 +0800 schrieb Peter Chen:
> > Add dt-binding documentation for generic onboard USB HUB.
> > 
> > Signed-off-by: Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > ---
> >  .../bindings/usb/generic-onboard-hub.txt           | 28 ++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > new file mode 100644
> > index 0000000..ea92205
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > @@ -0,0 +1,28 @@
> > +Generic Onboard USB HUB
> >+
> > +Required properties:
> > +- compatible: should be "generic-onboard-hub"
> 
> This something we don't have to define ad-hoc. The hub hangs off an USB
> controller, right? The "Open Firmware recommended practice: USB"
> document already describes how to represent USB devices in a generic
> manner:
> http://www.firmware.org/1275/bindings/usb/usb-1_0.ps
> 
> Is there a reason not to reuse this?
> 
> The usb hub node would be a child of the usb controller node, and it
> could use
> 	compatible = "usb,class9"; /* bDeviceClass 9 (Hub) */

Good point, I had not thought of that when I looked at the patches.

Yes, let's do this way. I don't know if we ever implemented the simple
patch to associate a USB device with a device_node, but if not, then
let's do it now for this driver. A lot of people have asked for it in
the past.

	Arnd
--
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

[PATCH 2/3] doc: dt-binding: generic onboard USB HUB

[Arnd Bergmann]

On Tuesday 08 December 2015 10:50:49 Philipp Zabel wrote:
> Am Dienstag, den 08.12.2015, 09:37 +0800 schrieb Peter Chen:
> > Add dt-binding documentation for generic onboard USB HUB.
> > 
> > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > ---
> >  .../bindings/usb/generic-onboard-hub.txt           | 28 ++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > new file mode 100644
> > index 0000000..ea92205
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > @@ -0,0 +1,28 @@
> > +Generic Onboard USB HUB
> >+
> > +Required properties:
> > +- compatible: should be "generic-onboard-hub"
> 
> This something we don't have to define ad-hoc. The hub hangs off an USB
> controller, right? The "Open Firmware recommended practice: USB"
> document already describes how to represent USB devices in a generic
> manner:
> http://www.firmware.org/1275/bindings/usb/usb-1_0.ps
> 
> Is there a reason not to reuse this?
> 
> The usb hub node would be a child of the usb controller node, and it
> could use
> 	compatible = "usb,class9"; /* bDeviceClass 9 (Hub) */

Good point, I had not thought of that when I looked at the patches.

Yes, let's do this way. I don't know if we ever implemented the simple
patch to associate a USB device with a device_node, but if not, then
let's do it now for this driver. A lot of people have asked for it in
the past.

	Arnd

Re: [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Felipe Balbi]

[-- Attachment #1.1: Type: text/plain, Size: 3838 bytes --]


Hi,

Peter Chen <peter.chen@freescale.com> writes:
>> seriously ? Is this really all ? What about that reset line below ?
>
> The clock is PHY input clock on the HUB, this clock may from SoC's
> PLL.

oh, you might have misunderstood my comment. I'm saying that there is
more than one thing you could cache in your private structure. That's
all.

>> > +static int usb_hub_generic_probe(struct platform_device *pdev)
>> > +{
>> > +	struct device *dev = &pdev->dev;
>> > +	struct usb_hub_generic_platform_data *pdata = dev->platform_data;
>> > +	struct usb_hub_generic_data *hub_data;
>> > +	int reset_pol = 0, duration_us = 50, ret = 0;
>> > +	struct gpio_desc *gpiod_reset = NULL;
>> > +
>> > +	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
>> > +	if (!hub_data)
>> > +		return -ENOMEM;
>> > +
>> > +	if (dev->of_node) {
>> > +		struct device_node *node = dev->of_node;
>> > +
>> > +		hub_data->clk = devm_clk_get(dev, "external_clk");
>> > +		if (IS_ERR(hub_data->clk)) {
>> > +			dev_dbg(dev, "Can't get external clock: %ld\n",
>> > +					PTR_ERR(hub_data->clk));
>> 
>> how about setting clock to NULL to here ? then you don't need IS_ERR()
>> checks anywhere else.
>> 
>> > +		}
>> 
>> braces shouldn't be used here, if you add NULL trick above,
>> however... then you can keep them.
>> 
>
> Braces aren't needed, it may not too much useful to using NULL
> as a indicator for error pointer.

heh, it's not about using it as an error pointer. I'm merely trying to
make clk optional. NULL is a valid clk, meaning you won't get NULL
pointer dereferences when passing it along clk_*() calls (if you find
any, it's likely a bug in CCF), so NULL can be used to cope with
optional clocks:

         clk = clk_get(dev, "foo");
         if (IS_ERR(clk)) {
         	if (PTR_ERR(clk) == -EPROBE_DEFER)
                	return -EPROBE_DEFER;
                else
                	clk = NULL;
         }

>> > +		/*
>> > +		 * Try to get the information for HUB reset, the
>> > +		 * default setting like below:
>> > +		 *
>> > +		 * - Reset state is low
>> > +		 * - The duration is 50us
>> > +		 */
>> > +		if (of_find_property(node, "hub-reset-active-high", NULL))
>> > +			reset_pol = 1;
>> 
>> you see, this is definitely *not* generic. You should write a generic
>> reset-gpio.c reset controller and describe the polarity on the gpio
>> binding. This driver *always* uses reset_assert(); reset_deassert(); and
>> reset-gpio implements though by gpiod_set_value() correctly.
>> 
>> Polarity _must_ be described elsewhere in DT.
>> 
>> > +		of_property_read_u32(node, "hub-reset-duration-us",
>> > +			&duration_us);
>> 
>> likewise, this should be described as a debounce time for the GPIO.
>> 
>
> Yes, if we are a reset gpio driver.

even if you use a raw GPIO, polarity and duration must come through DT.

>> > +		usleep_range(duration_us, duration_us + 100);
>> > +		gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);
>> 
>> wrong. You should _not_ have polarity checks here. You should have
>> already initialized the gpio as ACTIVE_HIGH or ACTIVE_LOW and gpiolib
>> will handle the polarity for you.
>
> Yes, you are right. I did not understand ACTIVE_LOW for gpio flag
> before.

with open source code, that's a rather poor excuse, Peter.

>> > +static int __init usb_hub_generic_init(void)
>> > +{
>> > +	return platform_driver_register(&usb_hub_generic_driver);
>> > +}
>> > +subsys_initcall(usb_hub_generic_init);
>> > +
>> > +static void __exit usb_hub_generic_exit(void)
>> > +{
>> > +	platform_driver_unregister(&usb_hub_generic_driver);
>> > +}
>> > +module_exit(usb_hub_generic_exit);
>> 
>> module_platform_driver();
>
> I want this driver to be called before module init's.

why ?

-- 
balbi

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Felipe Balbi]

Hi,

Peter Chen <peter.chen@freescale.com> writes:
>> seriously ? Is this really all ? What about that reset line below ?
>
> The clock is PHY input clock on the HUB, this clock may from SoC's
> PLL.

oh, you might have misunderstood my comment. I'm saying that there is
more than one thing you could cache in your private structure. That's
all.

>> > +static int usb_hub_generic_probe(struct platform_device *pdev)
>> > +{
>> > +	struct device *dev = &pdev->dev;
>> > +	struct usb_hub_generic_platform_data *pdata = dev->platform_data;
>> > +	struct usb_hub_generic_data *hub_data;
>> > +	int reset_pol = 0, duration_us = 50, ret = 0;
>> > +	struct gpio_desc *gpiod_reset = NULL;
>> > +
>> > +	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
>> > +	if (!hub_data)
>> > +		return -ENOMEM;
>> > +
>> > +	if (dev->of_node) {
>> > +		struct device_node *node = dev->of_node;
>> > +
>> > +		hub_data->clk = devm_clk_get(dev, "external_clk");
>> > +		if (IS_ERR(hub_data->clk)) {
>> > +			dev_dbg(dev, "Can't get external clock: %ld\n",
>> > +					PTR_ERR(hub_data->clk));
>> 
>> how about setting clock to NULL to here ? then you don't need IS_ERR()
>> checks anywhere else.
>> 
>> > +		}
>> 
>> braces shouldn't be used here, if you add NULL trick above,
>> however... then you can keep them.
>> 
>
> Braces aren't needed, it may not too much useful to using NULL
> as a indicator for error pointer.

heh, it's not about using it as an error pointer. I'm merely trying to
make clk optional. NULL is a valid clk, meaning you won't get NULL
pointer dereferences when passing it along clk_*() calls (if you find
any, it's likely a bug in CCF), so NULL can be used to cope with
optional clocks:

         clk = clk_get(dev, "foo");
         if (IS_ERR(clk)) {
         	if (PTR_ERR(clk) == -EPROBE_DEFER)
                	return -EPROBE_DEFER;
                else
                	clk = NULL;
         }

>> > +		/*
>> > +		 * Try to get the information for HUB reset, the
>> > +		 * default setting like below:
>> > +		 *
>> > +		 * - Reset state is low
>> > +		 * - The duration is 50us
>> > +		 */
>> > +		if (of_find_property(node, "hub-reset-active-high", NULL))
>> > +			reset_pol = 1;
>> 
>> you see, this is definitely *not* generic. You should write a generic
>> reset-gpio.c reset controller and describe the polarity on the gpio
>> binding. This driver *always* uses reset_assert(); reset_deassert(); and
>> reset-gpio implements though by gpiod_set_value() correctly.
>> 
>> Polarity _must_ be described elsewhere in DT.
>> 
>> > +		of_property_read_u32(node, "hub-reset-duration-us",
>> > +			&duration_us);
>> 
>> likewise, this should be described as a debounce time for the GPIO.
>> 
>
> Yes, if we are a reset gpio driver.

even if you use a raw GPIO, polarity and duration must come through DT.

>> > +		usleep_range(duration_us, duration_us + 100);
>> > +		gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);
>> 
>> wrong. You should _not_ have polarity checks here. You should have
>> already initialized the gpio as ACTIVE_HIGH or ACTIVE_LOW and gpiolib
>> will handle the polarity for you.
>
> Yes, you are right. I did not understand ACTIVE_LOW for gpio flag
> before.

with open source code, that's a rather poor excuse, Peter.

>> > +static int __init usb_hub_generic_init(void)
>> > +{
>> > +	return platform_driver_register(&usb_hub_generic_driver);
>> > +}
>> > +subsys_initcall(usb_hub_generic_init);
>> > +
>> > +static void __exit usb_hub_generic_exit(void)
>> > +{
>> > +	platform_driver_unregister(&usb_hub_generic_driver);
>> > +}
>> > +module_exit(usb_hub_generic_exit);
>> 
>> module_platform_driver();
>
> I want this driver to be called before module init's.

why ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20151208/46d4f17b/attachment.sig>

Re: [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Mathieu Poirier]

On 7 December 2015 at 18:37, Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> Current USB HUB driver lacks of platform interfaces to configure
> external signal on HUB chip, eg, the PHY input clock and gpio reset
> pin for HUB, these kinds of HUBs are usually soldered at the board,
> and they are not hot-plug USB devices.
>
> With this patch, the user can configure the HUB's pins at platform
> part (through dts or platform), and these configuration will be
> effective before the USB bus can be used, we use subsys_initcall
> for this driver.
>
> Signed-off-by: Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  MAINTAINERS                             |   7 ++
>  drivers/usb/misc/Kconfig                |   9 ++
>  drivers/usb/misc/Makefile               |   1 +
>  drivers/usb/misc/generic_onboard_hub.c  | 162 ++++++++++++++++++++++++++++++++
>  include/linux/usb/generic_onboard_hub.h |  11 +++
>  5 files changed, 190 insertions(+)
>  create mode 100644 drivers/usb/misc/generic_onboard_hub.c
>  create mode 100644 include/linux/usb/generic_onboard_hub.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e9caa4b..cc1981e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11121,6 +11121,13 @@ S:     Maintained
>  F:     Documentation/usb/ohci.txt
>  F:     drivers/usb/host/ohci*
>
> +USB Generic Onboard HUB Driver
> +M:     Peter Chen <Peter.Chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> +T:     git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
> +L:     linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> +S:     Maintained
> +F:     drivers/usb/misc/generic_onboard_hub.c
> +
>  USB OTG FSM (Finite State Machine)
>  M:     Peter Chen <Peter.Chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>  T:     git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
> diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> index f7a7fc2..8921cae 100644
> --- a/drivers/usb/misc/Kconfig
> +++ b/drivers/usb/misc/Kconfig
> @@ -268,3 +268,12 @@ config USB_CHAOSKEY
>
>           To compile this driver as a module, choose M here: the
>           module will be called chaoskey.
> +
> +config USB_ONBOARD_HUB
> +       tristate "Generic USB Onboard HUB"
> +       help
> +         Say Y here if your board has an onboard HUB, and this hub needs
> +         to control its PHY clock and reset pin through external signals.
> +         If you are not sure, say N.
> +
> +         To compile this driver as a module, choose M here.
> diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
> index 45fd4ac..da52e9a 100644
> --- a/drivers/usb/misc/Makefile
> +++ b/drivers/usb/misc/Makefile
> @@ -29,3 +29,4 @@ obj-$(CONFIG_USB_CHAOSKEY)            += chaoskey.o
>
>  obj-$(CONFIG_USB_SISUSBVGA)            += sisusbvga/
>  obj-$(CONFIG_USB_LINK_LAYER_TEST)      += lvstest.o
> +obj-$(CONFIG_USB_ONBOARD_HUB)          += generic_onboard_hub.o
> diff --git a/drivers/usb/misc/generic_onboard_hub.c b/drivers/usb/misc/generic_onboard_hub.c
> new file mode 100644
> index 0000000..df722a0
> --- /dev/null
> +++ b/drivers/usb/misc/generic_onboard_hub.c
> @@ -0,0 +1,162 @@
> +/*
> + * usb_hub_generic.c   The generic onboard USB HUB driver
> + *
> + * Copyright (C) 2015 Freescale Semiconductor, Inc.
> + * Author: Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2  of
> + * the License as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +/*
> + * This driver is only for the USB HUB devices which need to control
> + * their external pins(clock, reset, etc), and these USB HUB devices
> + * are soldered at the board.
> + */
> +
> +#define DEBUG
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/usb/generic_onboard_hub.h>
> +
> +struct usb_hub_generic_data {
> +       struct clk *clk;
> +};
> +
> +static int usb_hub_generic_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct usb_hub_generic_platform_data *pdata = dev->platform_data;
> +       struct usb_hub_generic_data *hub_data;
> +       int reset_pol = 0, duration_us = 50, ret = 0;
> +       struct gpio_desc *gpiod_reset = NULL;
> +
> +       hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> +       if (!hub_data)
> +               return -ENOMEM;
> +
> +       if (dev->of_node) {
> +               struct device_node *node = dev->of_node;
> +
> +               hub_data->clk = devm_clk_get(dev, "external_clk");
> +               if (IS_ERR(hub_data->clk)) {
> +                       dev_dbg(dev, "Can't get external clock: %ld\n",
> +                                       PTR_ERR(hub_data->clk));
> +               }

Is the intended behaviour to keep going here event when there is an
error?  Can the "hub_data" really work without a clock?

> +
> +               /*
> +                * Try to get the information for HUB reset, the
> +                * default setting like below:
> +                *
> +                * - Reset state is low
> +                * - The duration is 50us
> +                */
> +               if (of_find_property(node, "hub-reset-active-high", NULL))
> +                       reset_pol = 1;
> +
> +               of_property_read_u32(node, "hub-reset-duration-us",
> +                       &duration_us);
> +
> +               gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
> +                       reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
> +               ret = PTR_ERR_OR_ZERO(gpiod_reset);
> +               if (ret) {
> +                       dev_err(dev, "Failed to get reset gpio, err = %d\n",
> +                               ret);
> +                       return ret;
> +               }
> +       } else if (pdata) {
> +               hub_data->clk = pdata->ext_clk;
> +               duration_us = pdata->gpio_reset_duration_us;
> +               reset_pol = pdata->gpio_reset_polarity;
> +
> +               if (gpio_is_valid(pdata->gpio_reset)) {
> +                       ret = devm_gpio_request_one(
> +                               dev, pdata->gpio_reset, reset_pol
> +                               ? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> +                               dev_name(dev));
> +                       if (!ret)
> +                               gpiod_reset = gpio_to_desc(pdata->gpio_reset);
> +               }
> +       }
> +
> +       if (!IS_ERR(hub_data->clk)) {
> +               ret = clk_prepare_enable(hub_data->clk);
> +               if (ret) {
> +                       dev_err(dev,
> +                               "Can't enable external clock: %d\n",
> +                               ret);
> +                       return ret;
> +               }
> +       }
> +
> +       if (gpiod_reset) {
> +               /* Sanity check */
> +               if (duration_us > 1000000)
> +                       duration_us = 50;
> +               usleep_range(duration_us, duration_us + 100);
> +               gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);

What are these hard-coded values?  Shouldn't this be taken from the
DT?  If not then some comments explaining the values would be
appreciated.

> +       }
> +
> +       dev_set_drvdata(dev, hub_data);
> +       return ret;
> +}
> +
> +static int usb_hub_generic_remove(struct platform_device *pdev)
> +{
> +       struct usb_hub_generic_data *hub_data = platform_get_drvdata(pdev);
> +
> +       if (!IS_ERR(hub_data->clk))
> +               clk_disable_unprepare(hub_data->clk);
> +
> +       return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id usb_hub_generic_dt_ids[] = {
> +       {.compatible = "generic-onboard-hub"},
> +       { },
> +};
> +MODULE_DEVICE_TABLE(of, usb_hub_generic_dt_ids);
> +#endif
> +
> +static struct platform_driver usb_hub_generic_driver = {
> +       .probe = usb_hub_generic_probe,
> +       .remove = usb_hub_generic_remove,
> +       .driver = {
> +               .name = "usb_hub_generic_onboard",
> +               .of_match_table = usb_hub_generic_dt_ids,
> +        },
> +};
> +
> +static int __init usb_hub_generic_init(void)
> +{
> +       return platform_driver_register(&usb_hub_generic_driver);
> +}
> +subsys_initcall(usb_hub_generic_init);
> +
> +static void __exit usb_hub_generic_exit(void)
> +{
> +       platform_driver_unregister(&usb_hub_generic_driver);
> +}
> +module_exit(usb_hub_generic_exit);
> +
> +MODULE_AUTHOR("Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>");
> +MODULE_DESCRIPTION("Generic Onboard USB HUB driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/usb/generic_onboard_hub.h b/include/linux/usb/generic_onboard_hub.h
> new file mode 100644
> index 0000000..1b70ccc
> --- /dev/null
> +++ b/include/linux/usb/generic_onboard_hub.h
> @@ -0,0 +1,11 @@
> +#ifndef __LINUX_USB_GENERIC_HUB_H
> +#define __LINUX_USB_GENERIC_HUB_H
> +
> +struct usb_hub_generic_platform_data {
> +       int gpio_reset;
> +       int gpio_reset_polarity;
> +       int gpio_reset_duration_us;
> +       struct clk *ext_clk;
> +};

Please document this structure in accordance with kernel documentation
standards.

> +
> +#endif /* __LINUX_USB_GENERIC_HUB_H */
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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

[PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Mathieu Poirier]

On 7 December 2015 at 18:37, Peter Chen <peter.chen@freescale.com> wrote:
> Current USB HUB driver lacks of platform interfaces to configure
> external signal on HUB chip, eg, the PHY input clock and gpio reset
> pin for HUB, these kinds of HUBs are usually soldered at the board,
> and they are not hot-plug USB devices.
>
> With this patch, the user can configure the HUB's pins at platform
> part (through dts or platform), and these configuration will be
> effective before the USB bus can be used, we use subsys_initcall
> for this driver.
>
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
>  MAINTAINERS                             |   7 ++
>  drivers/usb/misc/Kconfig                |   9 ++
>  drivers/usb/misc/Makefile               |   1 +
>  drivers/usb/misc/generic_onboard_hub.c  | 162 ++++++++++++++++++++++++++++++++
>  include/linux/usb/generic_onboard_hub.h |  11 +++
>  5 files changed, 190 insertions(+)
>  create mode 100644 drivers/usb/misc/generic_onboard_hub.c
>  create mode 100644 include/linux/usb/generic_onboard_hub.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e9caa4b..cc1981e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11121,6 +11121,13 @@ S:     Maintained
>  F:     Documentation/usb/ohci.txt
>  F:     drivers/usb/host/ohci*
>
> +USB Generic Onboard HUB Driver
> +M:     Peter Chen <Peter.Chen@freescale.com>
> +T:     git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
> +L:     linux-usb at vger.kernel.org
> +S:     Maintained
> +F:     drivers/usb/misc/generic_onboard_hub.c
> +
>  USB OTG FSM (Finite State Machine)
>  M:     Peter Chen <Peter.Chen@freescale.com>
>  T:     git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git
> diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> index f7a7fc2..8921cae 100644
> --- a/drivers/usb/misc/Kconfig
> +++ b/drivers/usb/misc/Kconfig
> @@ -268,3 +268,12 @@ config USB_CHAOSKEY
>
>           To compile this driver as a module, choose M here: the
>           module will be called chaoskey.
> +
> +config USB_ONBOARD_HUB
> +       tristate "Generic USB Onboard HUB"
> +       help
> +         Say Y here if your board has an onboard HUB, and this hub needs
> +         to control its PHY clock and reset pin through external signals.
> +         If you are not sure, say N.
> +
> +         To compile this driver as a module, choose M here.
> diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
> index 45fd4ac..da52e9a 100644
> --- a/drivers/usb/misc/Makefile
> +++ b/drivers/usb/misc/Makefile
> @@ -29,3 +29,4 @@ obj-$(CONFIG_USB_CHAOSKEY)            += chaoskey.o
>
>  obj-$(CONFIG_USB_SISUSBVGA)            += sisusbvga/
>  obj-$(CONFIG_USB_LINK_LAYER_TEST)      += lvstest.o
> +obj-$(CONFIG_USB_ONBOARD_HUB)          += generic_onboard_hub.o
> diff --git a/drivers/usb/misc/generic_onboard_hub.c b/drivers/usb/misc/generic_onboard_hub.c
> new file mode 100644
> index 0000000..df722a0
> --- /dev/null
> +++ b/drivers/usb/misc/generic_onboard_hub.c
> @@ -0,0 +1,162 @@
> +/*
> + * usb_hub_generic.c   The generic onboard USB HUB driver
> + *
> + * Copyright (C) 2015 Freescale Semiconductor, Inc.
> + * Author: Peter Chen <peter.chen@freescale.com>
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2  of
> + * the License as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +/*
> + * This driver is only for the USB HUB devices which need to control
> + * their external pins(clock, reset, etc), and these USB HUB devices
> + * are soldered at the board.
> + */
> +
> +#define DEBUG
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/usb/generic_onboard_hub.h>
> +
> +struct usb_hub_generic_data {
> +       struct clk *clk;
> +};
> +
> +static int usb_hub_generic_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct usb_hub_generic_platform_data *pdata = dev->platform_data;
> +       struct usb_hub_generic_data *hub_data;
> +       int reset_pol = 0, duration_us = 50, ret = 0;
> +       struct gpio_desc *gpiod_reset = NULL;
> +
> +       hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> +       if (!hub_data)
> +               return -ENOMEM;
> +
> +       if (dev->of_node) {
> +               struct device_node *node = dev->of_node;
> +
> +               hub_data->clk = devm_clk_get(dev, "external_clk");
> +               if (IS_ERR(hub_data->clk)) {
> +                       dev_dbg(dev, "Can't get external clock: %ld\n",
> +                                       PTR_ERR(hub_data->clk));
> +               }

Is the intended behaviour to keep going here event when there is an
error?  Can the "hub_data" really work without a clock?

> +
> +               /*
> +                * Try to get the information for HUB reset, the
> +                * default setting like below:
> +                *
> +                * - Reset state is low
> +                * - The duration is 50us
> +                */
> +               if (of_find_property(node, "hub-reset-active-high", NULL))
> +                       reset_pol = 1;
> +
> +               of_property_read_u32(node, "hub-reset-duration-us",
> +                       &duration_us);
> +
> +               gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
> +                       reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
> +               ret = PTR_ERR_OR_ZERO(gpiod_reset);
> +               if (ret) {
> +                       dev_err(dev, "Failed to get reset gpio, err = %d\n",
> +                               ret);
> +                       return ret;
> +               }
> +       } else if (pdata) {
> +               hub_data->clk = pdata->ext_clk;
> +               duration_us = pdata->gpio_reset_duration_us;
> +               reset_pol = pdata->gpio_reset_polarity;
> +
> +               if (gpio_is_valid(pdata->gpio_reset)) {
> +                       ret = devm_gpio_request_one(
> +                               dev, pdata->gpio_reset, reset_pol
> +                               ? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> +                               dev_name(dev));
> +                       if (!ret)
> +                               gpiod_reset = gpio_to_desc(pdata->gpio_reset);
> +               }
> +       }
> +
> +       if (!IS_ERR(hub_data->clk)) {
> +               ret = clk_prepare_enable(hub_data->clk);
> +               if (ret) {
> +                       dev_err(dev,
> +                               "Can't enable external clock: %d\n",
> +                               ret);
> +                       return ret;
> +               }
> +       }
> +
> +       if (gpiod_reset) {
> +               /* Sanity check */
> +               if (duration_us > 1000000)
> +                       duration_us = 50;
> +               usleep_range(duration_us, duration_us + 100);
> +               gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);

What are these hard-coded values?  Shouldn't this be taken from the
DT?  If not then some comments explaining the values would be
appreciated.

> +       }
> +
> +       dev_set_drvdata(dev, hub_data);
> +       return ret;
> +}
> +
> +static int usb_hub_generic_remove(struct platform_device *pdev)
> +{
> +       struct usb_hub_generic_data *hub_data = platform_get_drvdata(pdev);
> +
> +       if (!IS_ERR(hub_data->clk))
> +               clk_disable_unprepare(hub_data->clk);
> +
> +       return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id usb_hub_generic_dt_ids[] = {
> +       {.compatible = "generic-onboard-hub"},
> +       { },
> +};
> +MODULE_DEVICE_TABLE(of, usb_hub_generic_dt_ids);
> +#endif
> +
> +static struct platform_driver usb_hub_generic_driver = {
> +       .probe = usb_hub_generic_probe,
> +       .remove = usb_hub_generic_remove,
> +       .driver = {
> +               .name = "usb_hub_generic_onboard",
> +               .of_match_table = usb_hub_generic_dt_ids,
> +        },
> +};
> +
> +static int __init usb_hub_generic_init(void)
> +{
> +       return platform_driver_register(&usb_hub_generic_driver);
> +}
> +subsys_initcall(usb_hub_generic_init);
> +
> +static void __exit usb_hub_generic_exit(void)
> +{
> +       platform_driver_unregister(&usb_hub_generic_driver);
> +}
> +module_exit(usb_hub_generic_exit);
> +
> +MODULE_AUTHOR("Peter Chen <peter.chen@freescale.com>");
> +MODULE_DESCRIPTION("Generic Onboard USB HUB driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/usb/generic_onboard_hub.h b/include/linux/usb/generic_onboard_hub.h
> new file mode 100644
> index 0000000..1b70ccc
> --- /dev/null
> +++ b/include/linux/usb/generic_onboard_hub.h
> @@ -0,0 +1,11 @@
> +#ifndef __LINUX_USB_GENERIC_HUB_H
> +#define __LINUX_USB_GENERIC_HUB_H
> +
> +struct usb_hub_generic_platform_data {
> +       int gpio_reset;
> +       int gpio_reset_polarity;
> +       int gpio_reset_duration_us;
> +       struct clk *ext_clk;
> +};

Please document this structure in accordance with kernel documentation
standards.

> +
> +#endif /* __LINUX_USB_GENERIC_HUB_H */
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] doc: dt-binding: generic onboard USB HUB

[Rob Herring]

On Tue, Dec 08, 2015 at 10:58:48AM +0100, Arnd Bergmann wrote:
> On Tuesday 08 December 2015 10:50:49 Philipp Zabel wrote:
> > Am Dienstag, den 08.12.2015, 09:37 +0800 schrieb Peter Chen:
> > > Add dt-binding documentation for generic onboard USB HUB.
> > > 
> > > Signed-off-by: Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > > ---
> > >  .../bindings/usb/generic-onboard-hub.txt           | 28 ++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > > new file mode 100644
> > > index 0000000..ea92205
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > > @@ -0,0 +1,28 @@
> > > +Generic Onboard USB HUB
> > >+
> > > +Required properties:
> > > +- compatible: should be "generic-onboard-hub"
> > 
> > This something we don't have to define ad-hoc. The hub hangs off an USB
> > controller, right? The "Open Firmware recommended practice: USB"
> > document already describes how to represent USB devices in a generic
> > manner:
> > http://www.firmware.org/1275/bindings/usb/usb-1_0.ps
> > 
> > Is there a reason not to reuse this?
> > 
> > The usb hub node would be a child of the usb controller node, and it
> > could use
> > 	compatible = "usb,class9"; /* bDeviceClass 9 (Hub) */
> 
> Good point, I had not thought of that when I looked at the patches.
>  
> Yes, let's do this way. I don't know if we ever implemented the simple
> patch to associate a USB device with a device_node, but if not, then
> let's do it now for this driver. A lot of people have asked for it in
> the past.

Agreed. Also, some hubs have I2C buses as well, but I still think under 
the USB bus is the right place.

However, one complication here is often (probably this case) these 
addtional signals need to be controlled before the device enumerates.

Rob
--
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

[PATCH 2/3] doc: dt-binding: generic onboard USB HUB

[Rob Herring]

On Tue, Dec 08, 2015 at 10:58:48AM +0100, Arnd Bergmann wrote:
> On Tuesday 08 December 2015 10:50:49 Philipp Zabel wrote:
> > Am Dienstag, den 08.12.2015, 09:37 +0800 schrieb Peter Chen:
> > > Add dt-binding documentation for generic onboard USB HUB.
> > > 
> > > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > > ---
> > >  .../bindings/usb/generic-onboard-hub.txt           | 28 ++++++++++++++++++++++
> > >  1 file changed, 28 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > > new file mode 100644
> > > index 0000000..ea92205
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > > @@ -0,0 +1,28 @@
> > > +Generic Onboard USB HUB
> > >+
> > > +Required properties:
> > > +- compatible: should be "generic-onboard-hub"
> > 
> > This something we don't have to define ad-hoc. The hub hangs off an USB
> > controller, right? The "Open Firmware recommended practice: USB"
> > document already describes how to represent USB devices in a generic
> > manner:
> > http://www.firmware.org/1275/bindings/usb/usb-1_0.ps
> > 
> > Is there a reason not to reuse this?
> > 
> > The usb hub node would be a child of the usb controller node, and it
> > could use
> > 	compatible = "usb,class9"; /* bDeviceClass 9 (Hub) */
> 
> Good point, I had not thought of that when I looked at the patches.
>  
> Yes, let's do this way. I don't know if we ever implemented the simple
> patch to associate a USB device with a device_node, but if not, then
> let's do it now for this driver. A lot of people have asked for it in
> the past.

Agreed. Also, some hubs have I2C buses as well, but I still think under 
the USB bus is the right place.

However, one complication here is often (probably this case) these 
addtional signals need to be controlled before the device enumerates.

Rob

Re: [PATCH 2/3] doc: dt-binding: generic onboard USB HUB

[Peter Chen]

On Tue, Dec 08, 2015 at 10:50:49AM +0100, Philipp Zabel wrote:
> Hi Peter,
> 
> Am Dienstag, den 08.12.2015, 09:37 +0800 schrieb Peter Chen:
> > Add dt-binding documentation for generic onboard USB HUB.
> > 
> > Signed-off-by: Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > ---
> >  .../bindings/usb/generic-onboard-hub.txt           | 28 ++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > new file mode 100644
> > index 0000000..ea92205
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > @@ -0,0 +1,28 @@
> > +Generic Onboard USB HUB
> >+
> > +Required properties:
> > +- compatible: should be "generic-onboard-hub"
> 
> This something we don't have to define ad-hoc. The hub hangs off an USB
> controller, right? The "Open Firmware recommended practice: USB"
> document already describes how to represent USB devices in a generic
> manner:
> http://www.firmware.org/1275/bindings/usb/usb-1_0.ps
> 
> Is there a reason not to reuse this?
> 
> The usb hub node would be a child of the usb controller node, and it
> could use
> 	compatible = "usb,class9"; /* bDeviceClass 9 (Hub) */
> 

Before writing this driver, I did consider how to let the USB core
handle device tree, but without good solution.

The controller (platform) driver should not handle specific USB device
described at device tree, since we want this handling to be generic.
And the USB device may not be recognized by controller without handling
its properties, like clock, reset pins.

With your suggestion, the dts likes below

usbh1: usb@02184000 {
	compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
	reg = <0x02184000 0x200>;
	interrupts = <0 43 IRQ_TYPE_LEVEL_HIGH>;
	clocks = <&clks IMX6QDL_CLK_USBOH3>;
	usb_hub@1 {
		compatible = "usb,class9"
		clocks = <&clks USB_HUB_CLK>;
		....
	};
};

I don't know how to handle above at USB bus, if you have any
suggestions, give me some tips please.

> 
> > +Optional properties:
> > +- clocks: the input clock for HUB.
> > +
> > +- clock-names: Should be "external_clk"
> 
> Is clock-names necessary for a single clock?
> 
> > +- hub-reset-gpios: Should specify the GPIO for reset.
> 
> I'd prefer that to be just "reset-gpios", it is the only reset property
> in the hub node after all. And use the GPIO_ACTIVE_HIGH/LOW flags to
> indicate polarity.

I will change to "clk" and "reset-gpios".

> 
> > +- hub-reset-active-high: the active reset signal is high, if this property is
> > +  not set, the active reset signal is low.
> 
> Then this could be dropped.
> 
> > +- hub-reset-duration-us: the duration for assert reset signal, the time unit
> > +  is microsecond.
> 
> And consequently, this could just be called "reset-duration-us".
> It might make sense to define the same for other reset GPIOs, too.
> The Freescale FEC, for example, has "phy-reset-duration" (in ms)
> already.
> 

Agree.

By the way: Felipe suggest using generic reset gpio driver to handle
above, I find you have written similar things before [1], why it can't be
accepted?

[1]http://comments.gmane.org/gmane.linux.ports.arm.kernel/255129
-- 

Best Regards,
Peter Chen
--
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

[PATCH 2/3] doc: dt-binding: generic onboard USB HUB

[Peter Chen]

On Tue, Dec 08, 2015 at 10:50:49AM +0100, Philipp Zabel wrote:
> Hi Peter,
> 
> Am Dienstag, den 08.12.2015, 09:37 +0800 schrieb Peter Chen:
> > Add dt-binding documentation for generic onboard USB HUB.
> > 
> > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > ---
> >  .../bindings/usb/generic-onboard-hub.txt           | 28 ++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > new file mode 100644
> > index 0000000..ea92205
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > @@ -0,0 +1,28 @@
> > +Generic Onboard USB HUB
> >+
> > +Required properties:
> > +- compatible: should be "generic-onboard-hub"
> 
> This something we don't have to define ad-hoc. The hub hangs off an USB
> controller, right? The "Open Firmware recommended practice: USB"
> document already describes how to represent USB devices in a generic
> manner:
> http://www.firmware.org/1275/bindings/usb/usb-1_0.ps
> 
> Is there a reason not to reuse this?
> 
> The usb hub node would be a child of the usb controller node, and it
> could use
> 	compatible = "usb,class9"; /* bDeviceClass 9 (Hub) */
> 

Before writing this driver, I did consider how to let the USB core
handle device tree, but without good solution.

The controller (platform) driver should not handle specific USB device
described at device tree, since we want this handling to be generic.
And the USB device may not be recognized by controller without handling
its properties, like clock, reset pins.

With your suggestion, the dts likes below

usbh1: usb at 02184000 {
	compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
	reg = <0x02184000 0x200>;
	interrupts = <0 43 IRQ_TYPE_LEVEL_HIGH>;
	clocks = <&clks IMX6QDL_CLK_USBOH3>;
	usb_hub at 1 {
		compatible = "usb,class9"
		clocks = <&clks USB_HUB_CLK>;
		....
	};
};

I don't know how to handle above at USB bus, if you have any
suggestions, give me some tips please.

> 
> > +Optional properties:
> > +- clocks: the input clock for HUB.
> > +
> > +- clock-names: Should be "external_clk"
> 
> Is clock-names necessary for a single clock?
> 
> > +- hub-reset-gpios: Should specify the GPIO for reset.
> 
> I'd prefer that to be just "reset-gpios", it is the only reset property
> in the hub node after all. And use the GPIO_ACTIVE_HIGH/LOW flags to
> indicate polarity.

I will change to "clk" and "reset-gpios".

> 
> > +- hub-reset-active-high: the active reset signal is high, if this property is
> > +  not set, the active reset signal is low.
> 
> Then this could be dropped.
> 
> > +- hub-reset-duration-us: the duration for assert reset signal, the time unit
> > +  is microsecond.
> 
> And consequently, this could just be called "reset-duration-us".
> It might make sense to define the same for other reset GPIOs, too.
> The Freescale FEC, for example, has "phy-reset-duration" (in ms)
> already.
> 

Agree.

By the way: Felipe suggest using generic reset gpio driver to handle
above, I find you have written similar things before [1], why it can't be
accepted?

[1]http://comments.gmane.org/gmane.linux.ports.arm.kernel/255129
-- 

Best Regards,
Peter Chen

Re: [PATCH 2/3] doc: dt-binding: generic onboard USB HUB

[Peter Chen]

On Tue, Dec 08, 2015 at 09:24:03PM -0600, Rob Herring wrote:
> On Tue, Dec 08, 2015 at 10:58:48AM +0100, Arnd Bergmann wrote:
> > On Tuesday 08 December 2015 10:50:49 Philipp Zabel wrote:
> > > Am Dienstag, den 08.12.2015, 09:37 +0800 schrieb Peter Chen:
> > > > Add dt-binding documentation for generic onboard USB HUB.
> > > > 
> > > > Signed-off-by: Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > > > ---
> > > >  .../bindings/usb/generic-onboard-hub.txt           | 28 ++++++++++++++++++++++
> > > >  1 file changed, 28 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > > > new file mode 100644
> > > > index 0000000..ea92205
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > > > @@ -0,0 +1,28 @@
> > > > +Generic Onboard USB HUB
> > > >+
> > > > +Required properties:
> > > > +- compatible: should be "generic-onboard-hub"
> > > 
> > > This something we don't have to define ad-hoc. The hub hangs off an USB
> > > controller, right? The "Open Firmware recommended practice: USB"
> > > document already describes how to represent USB devices in a generic
> > > manner:
> > > http://www.firmware.org/1275/bindings/usb/usb-1_0.ps
> > > 
> > > Is there a reason not to reuse this?
> > > 
> > > The usb hub node would be a child of the usb controller node, and it
> > > could use
> > > 	compatible = "usb,class9"; /* bDeviceClass 9 (Hub) */
> > 
> > Good point, I had not thought of that when I looked at the patches.
> >  
> > Yes, let's do this way. I don't know if we ever implemented the simple
> > patch to associate a USB device with a device_node, but if not, then
> > let's do it now for this driver. A lot of people have asked for it in
> > the past.
> 
> Agreed. Also, some hubs have I2C buses as well, but I still think under 
> the USB bus is the right place.
> 
> However, one complication here is often (probably this case) these 
> addtional signals need to be controlled before the device enumerates.
> 

Yes, I did not find a way to let the USB bus code handle it, so I had to
write a platform driver to do it

-- 

Best Regards,
Peter Chen
--
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

[PATCH 2/3] doc: dt-binding: generic onboard USB HUB

[Peter Chen]

On Tue, Dec 08, 2015 at 09:24:03PM -0600, Rob Herring wrote:
> On Tue, Dec 08, 2015 at 10:58:48AM +0100, Arnd Bergmann wrote:
> > On Tuesday 08 December 2015 10:50:49 Philipp Zabel wrote:
> > > Am Dienstag, den 08.12.2015, 09:37 +0800 schrieb Peter Chen:
> > > > Add dt-binding documentation for generic onboard USB HUB.
> > > > 
> > > > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > > > ---
> > > >  .../bindings/usb/generic-onboard-hub.txt           | 28 ++++++++++++++++++++++
> > > >  1 file changed, 28 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > > > new file mode 100644
> > > > index 0000000..ea92205
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/usb/generic-onboard-hub.txt
> > > > @@ -0,0 +1,28 @@
> > > > +Generic Onboard USB HUB
> > > >+
> > > > +Required properties:
> > > > +- compatible: should be "generic-onboard-hub"
> > > 
> > > This something we don't have to define ad-hoc. The hub hangs off an USB
> > > controller, right? The "Open Firmware recommended practice: USB"
> > > document already describes how to represent USB devices in a generic
> > > manner:
> > > http://www.firmware.org/1275/bindings/usb/usb-1_0.ps
> > > 
> > > Is there a reason not to reuse this?
> > > 
> > > The usb hub node would be a child of the usb controller node, and it
> > > could use
> > > 	compatible = "usb,class9"; /* bDeviceClass 9 (Hub) */
> > 
> > Good point, I had not thought of that when I looked at the patches.
> >  
> > Yes, let's do this way. I don't know if we ever implemented the simple
> > patch to associate a USB device with a device_node, but if not, then
> > let's do it now for this driver. A lot of people have asked for it in
> > the past.
> 
> Agreed. Also, some hubs have I2C buses as well, but I still think under 
> the USB bus is the right place.
> 
> However, one complication here is often (probably this case) these 
> addtional signals need to be controlled before the device enumerates.
> 

Yes, I did not find a way to let the USB bus code handle it, so I had to
write a platform driver to do it

-- 

Best Regards,
Peter Chen

Re: [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Peter Chen]

On Tue, Dec 08, 2015 at 10:48:28AM +0100, Arnd Bergmann wrote:
> On Tuesday 08 December 2015 09:37:48 Peter Chen wrote:
> 
> > +struct usb_hub_generic_data {
> > +	struct clk *clk;
> > +};
> > +
> > +static int usb_hub_generic_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct usb_hub_generic_platform_data *pdata = dev->platform_data;
> > +	struct usb_hub_generic_data *hub_data;
> > +	int reset_pol = 0, duration_us = 50, ret = 0;
> > +	struct gpio_desc *gpiod_reset = NULL;
> > +
> > +	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> > +	if (!hub_data)
> > +		return -ENOMEM;
> > +
> > +	if (dev->of_node) {
> 
> Let's not worry about the !DT case until someone adds a board file
> that needs it. Just remove the if() here along and the whole else
> block.
> 
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id usb_hub_generic_dt_ids[] = {
> > +	{.compatible = "generic-onboard-hub"},
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, usb_hub_generic_dt_ids);
> > +#endif
> > +
> > +static struct platform_driver usb_hub_generic_driver = {
> > +	.probe = usb_hub_generic_probe,
> > +	.remove = usb_hub_generic_remove,
> > +	.driver = {
> > +		.name = "usb_hub_generic_onboard",
> > +		.of_match_table = usb_hub_generic_dt_ids,
> > +	 },
> > +};
> 
> Build error when CONFIG_OF is disabled: Please remove the #ifdef around the device
> table.
> 
> > diff --git a/include/linux/usb/generic_onboard_hub.h b/include/linux/usb/generic_onboard_hub.h
> > new file mode 100644
> > index 0000000..1b70ccc
> > --- /dev/null
> > +++ b/include/linux/usb/generic_onboard_hub.h
> > @@ -0,0 +1,11 @@
> > +#ifndef __LINUX_USB_GENERIC_HUB_H
> > +#define __LINUX_USB_GENERIC_HUB_H
> > +
> > +struct usb_hub_generic_platform_data {
> > +	int gpio_reset;
> > +	int gpio_reset_polarity;
> > +	int gpio_reset_duration_us;
> > +	struct clk *ext_clk;
> > +};
> 
> Merge this structure into struct usb_hub_generic_data and remove the header.
> 
> 	ARnd

Agree.

-- 

Best Regards,
Peter Chen
--
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

[PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Peter Chen]

On Tue, Dec 08, 2015 at 10:48:28AM +0100, Arnd Bergmann wrote:
> On Tuesday 08 December 2015 09:37:48 Peter Chen wrote:
> 
> > +struct usb_hub_generic_data {
> > +	struct clk *clk;
> > +};
> > +
> > +static int usb_hub_generic_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct usb_hub_generic_platform_data *pdata = dev->platform_data;
> > +	struct usb_hub_generic_data *hub_data;
> > +	int reset_pol = 0, duration_us = 50, ret = 0;
> > +	struct gpio_desc *gpiod_reset = NULL;
> > +
> > +	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> > +	if (!hub_data)
> > +		return -ENOMEM;
> > +
> > +	if (dev->of_node) {
> 
> Let's not worry about the !DT case until someone adds a board file
> that needs it. Just remove the if() here along and the whole else
> block.
> 
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id usb_hub_generic_dt_ids[] = {
> > +	{.compatible = "generic-onboard-hub"},
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, usb_hub_generic_dt_ids);
> > +#endif
> > +
> > +static struct platform_driver usb_hub_generic_driver = {
> > +	.probe = usb_hub_generic_probe,
> > +	.remove = usb_hub_generic_remove,
> > +	.driver = {
> > +		.name = "usb_hub_generic_onboard",
> > +		.of_match_table = usb_hub_generic_dt_ids,
> > +	 },
> > +};
> 
> Build error when CONFIG_OF is disabled: Please remove the #ifdef around the device
> table.
> 
> > diff --git a/include/linux/usb/generic_onboard_hub.h b/include/linux/usb/generic_onboard_hub.h
> > new file mode 100644
> > index 0000000..1b70ccc
> > --- /dev/null
> > +++ b/include/linux/usb/generic_onboard_hub.h
> > @@ -0,0 +1,11 @@
> > +#ifndef __LINUX_USB_GENERIC_HUB_H
> > +#define __LINUX_USB_GENERIC_HUB_H
> > +
> > +struct usb_hub_generic_platform_data {
> > +	int gpio_reset;
> > +	int gpio_reset_polarity;
> > +	int gpio_reset_duration_us;
> > +	struct clk *ext_clk;
> > +};
> 
> Merge this structure into struct usb_hub_generic_data and remove the header.
> 
> 	ARnd

Agree.

-- 

Best Regards,
Peter Chen

Re: [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Peter Chen]

On Tue, Dec 08, 2015 at 10:44:02AM +0100, Sascha Hauer wrote:
> On Tue, Dec 08, 2015 at 05:26:56PM +0800, Peter Chen wrote:
> > On Tue, Dec 08, 2015 at 07:19:05AM +0100, Sascha Hauer wrote:
> > > > +	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> > > > +	if (!hub_data)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	if (dev->of_node) {
> > > > +		struct device_node *node = dev->of_node;
> > > > +
> > > > +		hub_data->clk = devm_clk_get(dev, "external_clk");
> > > 
> > > Please drop such _clk suffixes. The context already makes it clear that
> > > it's a clock.
> > > 
> > 
> > Will change.
> > 
> > > > +		if (IS_ERR(hub_data->clk)) {
> > > > +			dev_dbg(dev, "Can't get external clock: %ld\n",
> > > > +					PTR_ERR(hub_data->clk));
> > > > +		}
> > > > +
> > > > +		/*
> > > > +		 * Try to get the information for HUB reset, the
> > > > +		 * default setting like below:
> > > > +		 *
> > > > +		 * - Reset state is low
> > > > +		 * - The duration is 50us
> > > > +		 */
> > > > +		if (of_find_property(node, "hub-reset-active-high", NULL))
> > > > +			reset_pol = 1;
> > > > +
> > > > +		of_property_read_u32(node, "hub-reset-duration-us",
> > > > +			&duration_us);
> > > > +
> > > > +		gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
> > > > +			reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
> > > > +		ret = PTR_ERR_OR_ZERO(gpiod_reset);
> > > > +		if (ret) {
> > > > +			dev_err(dev, "Failed to get reset gpio, err = %d\n",
> > > > +				ret);
> > > > +			return ret;
> > > > +		}
> > > > +	} else if (pdata) {
> > > > +		hub_data->clk = pdata->ext_clk;
> > > 
> > > Passing clocks in platform_data is a no go. clocks must always be
> > > retrieved with clk_get.
> > 
> > Yes, you are right.
> > 
> > > 
> > > > +		duration_us = pdata->gpio_reset_duration_us;
> > > > +		reset_pol = pdata->gpio_reset_polarity;
> > > > +
> > > > +		if (gpio_is_valid(pdata->gpio_reset)) {
> > > > +			ret = devm_gpio_request_one(
> > > > +				dev, pdata->gpio_reset, reset_pol
> > > > +				? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> > > > +				dev_name(dev));
> > > > +			if (!ret)
> > > > +				gpiod_reset = gpio_to_desc(pdata->gpio_reset);
> > > 
> > > If the gpio is valid then being unable to get it is an error.
> > 
> > I can't catch you, if the gpio is invalid, its descriptor will be NULL,
> > then, the code will not do any gpio operation.
> 
> When the platform_data provides a gpio (gpio_is_valid() is true) then
> you must use the gpio. If then devm_gpio_request_one() fails you must
> bail out.

I see.

> 
> > 
> > > 
> > > Do you need this platform_data stuff at all?
> > > 
> > 
> > If not, how can I cover the platform which does not use dts, but still
> > uses these kinds of HUBs?
> 
> You can't, but are there any non device tree platforms which want to use
> this driver?
> 

I thought there are i386 embedded devices, it may use the HUB, like this
driver tries to handle. I agree that we only handle dt in this driver
first until the non-dt user appears.

-- 

Best Regards,
Peter Chen
--
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

[PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Peter Chen]

On Tue, Dec 08, 2015 at 10:44:02AM +0100, Sascha Hauer wrote:
> On Tue, Dec 08, 2015 at 05:26:56PM +0800, Peter Chen wrote:
> > On Tue, Dec 08, 2015 at 07:19:05AM +0100, Sascha Hauer wrote:
> > > > +	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> > > > +	if (!hub_data)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	if (dev->of_node) {
> > > > +		struct device_node *node = dev->of_node;
> > > > +
> > > > +		hub_data->clk = devm_clk_get(dev, "external_clk");
> > > 
> > > Please drop such _clk suffixes. The context already makes it clear that
> > > it's a clock.
> > > 
> > 
> > Will change.
> > 
> > > > +		if (IS_ERR(hub_data->clk)) {
> > > > +			dev_dbg(dev, "Can't get external clock: %ld\n",
> > > > +					PTR_ERR(hub_data->clk));
> > > > +		}
> > > > +
> > > > +		/*
> > > > +		 * Try to get the information for HUB reset, the
> > > > +		 * default setting like below:
> > > > +		 *
> > > > +		 * - Reset state is low
> > > > +		 * - The duration is 50us
> > > > +		 */
> > > > +		if (of_find_property(node, "hub-reset-active-high", NULL))
> > > > +			reset_pol = 1;
> > > > +
> > > > +		of_property_read_u32(node, "hub-reset-duration-us",
> > > > +			&duration_us);
> > > > +
> > > > +		gpiod_reset = devm_gpiod_get_optional(dev, "hub-reset",
> > > > +			reset_pol ? GPIOD_OUT_HIGH : GPIOD_OUT_LOW);
> > > > +		ret = PTR_ERR_OR_ZERO(gpiod_reset);
> > > > +		if (ret) {
> > > > +			dev_err(dev, "Failed to get reset gpio, err = %d\n",
> > > > +				ret);
> > > > +			return ret;
> > > > +		}
> > > > +	} else if (pdata) {
> > > > +		hub_data->clk = pdata->ext_clk;
> > > 
> > > Passing clocks in platform_data is a no go. clocks must always be
> > > retrieved with clk_get.
> > 
> > Yes, you are right.
> > 
> > > 
> > > > +		duration_us = pdata->gpio_reset_duration_us;
> > > > +		reset_pol = pdata->gpio_reset_polarity;
> > > > +
> > > > +		if (gpio_is_valid(pdata->gpio_reset)) {
> > > > +			ret = devm_gpio_request_one(
> > > > +				dev, pdata->gpio_reset, reset_pol
> > > > +				? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
> > > > +				dev_name(dev));
> > > > +			if (!ret)
> > > > +				gpiod_reset = gpio_to_desc(pdata->gpio_reset);
> > > 
> > > If the gpio is valid then being unable to get it is an error.
> > 
> > I can't catch you, if the gpio is invalid, its descriptor will be NULL,
> > then, the code will not do any gpio operation.
> 
> When the platform_data provides a gpio (gpio_is_valid() is true) then
> you must use the gpio. If then devm_gpio_request_one() fails you must
> bail out.

I see.

> 
> > 
> > > 
> > > Do you need this platform_data stuff at all?
> > > 
> > 
> > If not, how can I cover the platform which does not use dts, but still
> > uses these kinds of HUBs?
> 
> You can't, but are there any non device tree platforms which want to use
> this driver?
> 

I thought there are i386 embedded devices, it may use the HUB, like this
driver tries to handle. I agree that we only handle dt in this driver
first until the non-dt user appears.

-- 

Best Regards,
Peter Chen

Re: [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Peter Chen]

On Tue, Dec 08, 2015 at 07:55:05AM -0600, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> writes:
> >> seriously ? Is this really all ? What about that reset line below ?
> >
> > The clock is PHY input clock on the HUB, this clock may from SoC's
> > PLL.
> 
> oh, you might have misunderstood my comment. I'm saying that there is
> more than one thing you could cache in your private structure. That's
> all.
> 

How? I need to handle clock at both ->probe and ->remove.

> >> > +static int usb_hub_generic_probe(struct platform_device *pdev)
> >> > +{
> >> > +	struct device *dev = &pdev->dev;
> >> > +	struct usb_hub_generic_platform_data *pdata = dev->platform_data;
> >> > +	struct usb_hub_generic_data *hub_data;
> >> > +	int reset_pol = 0, duration_us = 50, ret = 0;
> >> > +	struct gpio_desc *gpiod_reset = NULL;
> >> > +
> >> > +	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> >> > +	if (!hub_data)
> >> > +		return -ENOMEM;
> >> > +
> >> > +	if (dev->of_node) {
> >> > +		struct device_node *node = dev->of_node;
> >> > +
> >> > +		hub_data->clk = devm_clk_get(dev, "external_clk");
> >> > +		if (IS_ERR(hub_data->clk)) {
> >> > +			dev_dbg(dev, "Can't get external clock: %ld\n",
> >> > +					PTR_ERR(hub_data->clk));
> >> 
> >> how about setting clock to NULL to here ? then you don't need IS_ERR()
> >> checks anywhere else.
> >> 
> >> > +		}
> >> 
> >> braces shouldn't be used here, if you add NULL trick above,
> >> however... then you can keep them.
> >> 
> >
> > Braces aren't needed, it may not too much useful to using NULL
> > as a indicator for error pointer.
> 
> heh, it's not about using it as an error pointer. I'm merely trying to
> make clk optional. NULL is a valid clk, meaning you won't get NULL
> pointer dereferences when passing it along clk_*() calls (if you find
> any, it's likely a bug in CCF), so NULL can be used to cope with
> optional clocks:
> 
>          clk = clk_get(dev, "foo");
>          if (IS_ERR(clk)) {
>          	if (PTR_ERR(clk) == -EPROBE_DEFER)
>                 	return -EPROBE_DEFER;
>                 else
>                 	clk = NULL;
>          }
> 

Get your point, so at coming code, we don't need to add condition
to enable optional clock.

> >> > +		/*
> >> > +		 * Try to get the information for HUB reset, the
> >> > +		 * default setting like below:
> >> > +		 *
> >> > +		 * - Reset state is low
> >> > +		 * - The duration is 50us
> >> > +		 */
> >> > +		if (of_find_property(node, "hub-reset-active-high", NULL))
> >> > +			reset_pol = 1;
> >> 
> >> you see, this is definitely *not* generic. You should write a generic
> >> reset-gpio.c reset controller and describe the polarity on the gpio
> >> binding. This driver *always* uses reset_assert(); reset_deassert(); and
> >> reset-gpio implements though by gpiod_set_value() correctly.
> >> 
> >> Polarity _must_ be described elsewhere in DT.
> >> 
> >> > +		of_property_read_u32(node, "hub-reset-duration-us",
> >> > +			&duration_us);
> >> 
> >> likewise, this should be described as a debounce time for the GPIO.
> >> 
> >
> > Yes, if we are a reset gpio driver.
> 
> even if you use a raw GPIO, polarity and duration must come through DT.
> 
> >> > +		usleep_range(duration_us, duration_us + 100);
> >> > +		gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);
> >> 
> >> wrong. You should _not_ have polarity checks here. You should have
> >> already initialized the gpio as ACTIVE_HIGH or ACTIVE_LOW and gpiolib
> >> will handle the polarity for you.
> >
> > Yes, you are right. I did not understand ACTIVE_LOW for gpio flag
> > before.
> 
> with open source code, that's a rather poor excuse, Peter.

I will pay attention to it, thanks.
At my dts example, it is like below, I just treat it at raw gpio
handling.

	usb_hub1 {
		compatible = "generic-onboard-hub";
		clocks = <&clks IMX6QDL_CLK_CKO>;
		clock-names = "external_clk";
		hub-reset-active-high;
		hub-reset-gpios = <&gpio7 12 0>;
		hub-reset-duration-us = <2>;
	};

I will change it like below:
	usb_hub1 {
		compatible = "generic-onboard-hub";
		clocks = <&clks IMX6QDL_CLK_CKO>;
		clock-names = "clk";
		reset-gpios = <&gpio7 12 GPIO_ACTIVE_HIGH>;
		reset-duration-us = <2>;
	};

> 
> >> > +static int __init usb_hub_generic_init(void)
> >> > +{
> >> > +	return platform_driver_register(&usb_hub_generic_driver);
> >> > +}
> >> > +subsys_initcall(usb_hub_generic_init);
> >> > +
> >> > +static void __exit usb_hub_generic_exit(void)
> >> > +{
> >> > +	platform_driver_unregister(&usb_hub_generic_driver);
> >> > +}
> >> > +module_exit(usb_hub_generic_exit);
> >> 
> >> module_platform_driver();
> >
> > I want this driver to be called before module init's.
> 
> why ?

The USB HUB should be in ready state before controller
tries to talk with it, otherwise, it may has noise at
the bus during the HUB reset/power on process.

-- 

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

[PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Peter Chen]

On Tue, Dec 08, 2015 at 07:55:05AM -0600, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen <peter.chen@freescale.com> writes:
> >> seriously ? Is this really all ? What about that reset line below ?
> >
> > The clock is PHY input clock on the HUB, this clock may from SoC's
> > PLL.
> 
> oh, you might have misunderstood my comment. I'm saying that there is
> more than one thing you could cache in your private structure. That's
> all.
> 

How? I need to handle clock at both ->probe and ->remove.

> >> > +static int usb_hub_generic_probe(struct platform_device *pdev)
> >> > +{
> >> > +	struct device *dev = &pdev->dev;
> >> > +	struct usb_hub_generic_platform_data *pdata = dev->platform_data;
> >> > +	struct usb_hub_generic_data *hub_data;
> >> > +	int reset_pol = 0, duration_us = 50, ret = 0;
> >> > +	struct gpio_desc *gpiod_reset = NULL;
> >> > +
> >> > +	hub_data = devm_kzalloc(dev, sizeof(*hub_data), GFP_KERNEL);
> >> > +	if (!hub_data)
> >> > +		return -ENOMEM;
> >> > +
> >> > +	if (dev->of_node) {
> >> > +		struct device_node *node = dev->of_node;
> >> > +
> >> > +		hub_data->clk = devm_clk_get(dev, "external_clk");
> >> > +		if (IS_ERR(hub_data->clk)) {
> >> > +			dev_dbg(dev, "Can't get external clock: %ld\n",
> >> > +					PTR_ERR(hub_data->clk));
> >> 
> >> how about setting clock to NULL to here ? then you don't need IS_ERR()
> >> checks anywhere else.
> >> 
> >> > +		}
> >> 
> >> braces shouldn't be used here, if you add NULL trick above,
> >> however... then you can keep them.
> >> 
> >
> > Braces aren't needed, it may not too much useful to using NULL
> > as a indicator for error pointer.
> 
> heh, it's not about using it as an error pointer. I'm merely trying to
> make clk optional. NULL is a valid clk, meaning you won't get NULL
> pointer dereferences when passing it along clk_*() calls (if you find
> any, it's likely a bug in CCF), so NULL can be used to cope with
> optional clocks:
> 
>          clk = clk_get(dev, "foo");
>          if (IS_ERR(clk)) {
>          	if (PTR_ERR(clk) == -EPROBE_DEFER)
>                 	return -EPROBE_DEFER;
>                 else
>                 	clk = NULL;
>          }
> 

Get your point, so at coming code, we don't need to add condition
to enable optional clock.

> >> > +		/*
> >> > +		 * Try to get the information for HUB reset, the
> >> > +		 * default setting like below:
> >> > +		 *
> >> > +		 * - Reset state is low
> >> > +		 * - The duration is 50us
> >> > +		 */
> >> > +		if (of_find_property(node, "hub-reset-active-high", NULL))
> >> > +			reset_pol = 1;
> >> 
> >> you see, this is definitely *not* generic. You should write a generic
> >> reset-gpio.c reset controller and describe the polarity on the gpio
> >> binding. This driver *always* uses reset_assert(); reset_deassert(); and
> >> reset-gpio implements though by gpiod_set_value() correctly.
> >> 
> >> Polarity _must_ be described elsewhere in DT.
> >> 
> >> > +		of_property_read_u32(node, "hub-reset-duration-us",
> >> > +			&duration_us);
> >> 
> >> likewise, this should be described as a debounce time for the GPIO.
> >> 
> >
> > Yes, if we are a reset gpio driver.
> 
> even if you use a raw GPIO, polarity and duration must come through DT.
> 
> >> > +		usleep_range(duration_us, duration_us + 100);
> >> > +		gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);
> >> 
> >> wrong. You should _not_ have polarity checks here. You should have
> >> already initialized the gpio as ACTIVE_HIGH or ACTIVE_LOW and gpiolib
> >> will handle the polarity for you.
> >
> > Yes, you are right. I did not understand ACTIVE_LOW for gpio flag
> > before.
> 
> with open source code, that's a rather poor excuse, Peter.

I will pay attention to it, thanks.
At my dts example, it is like below, I just treat it at raw gpio
handling.

	usb_hub1 {
		compatible = "generic-onboard-hub";
		clocks = <&clks IMX6QDL_CLK_CKO>;
		clock-names = "external_clk";
		hub-reset-active-high;
		hub-reset-gpios = <&gpio7 12 0>;
		hub-reset-duration-us = <2>;
	};

I will change it like below:
	usb_hub1 {
		compatible = "generic-onboard-hub";
		clocks = <&clks IMX6QDL_CLK_CKO>;
		clock-names = "clk";
		reset-gpios = <&gpio7 12 GPIO_ACTIVE_HIGH>;
		reset-duration-us = <2>;
	};

> 
> >> > +static int __init usb_hub_generic_init(void)
> >> > +{
> >> > +	return platform_driver_register(&usb_hub_generic_driver);
> >> > +}
> >> > +subsys_initcall(usb_hub_generic_init);
> >> > +
> >> > +static void __exit usb_hub_generic_exit(void)
> >> > +{
> >> > +	platform_driver_unregister(&usb_hub_generic_driver);
> >> > +}
> >> > +module_exit(usb_hub_generic_exit);
> >> 
> >> module_platform_driver();
> >
> > I want this driver to be called before module init's.
> 
> why ?

The USB HUB should be in ready state before controller
tries to talk with it, otherwise, it may has noise at
the bus during the HUB reset/power on process.

-- 

Best Regards,
Peter Chen

Re: [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Peter Chen]

On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> On 7 December 2015 at 18:37, Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > +
> > +       if (dev->of_node) {
> > +               struct device_node *node = dev->of_node;
> > +
> > +               hub_data->clk = devm_clk_get(dev, "external_clk");
> > +               if (IS_ERR(hub_data->clk)) {
> > +                       dev_dbg(dev, "Can't get external clock: %ld\n",
> > +                                       PTR_ERR(hub_data->clk));
> > +               }
> 
> Is the intended behaviour to keep going here event when there is an
> error?  Can the "hub_data" really work without a clock?

Yes, some HUB may work with fixed 24M OSC at the board, but they need to
reset through external IO, so the clock is not need at this case, but
reset pin is mandatory.

> > +       }
> > +
> > +       if (gpiod_reset) {
> > +               /* Sanity check */
> > +               if (duration_us > 1000000)
> > +                       duration_us = 50;
> > +               usleep_range(duration_us, duration_us + 100);
> > +               gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);
> 
> What are these hard-coded values?  Shouldn't this be taken from the
> DT?  If not then some comments explaining the values would be
> appreciated.

Yes, I did it wrongly, thanks.

> > diff --git a/include/linux/usb/generic_onboard_hub.h b/include/linux/usb/generic_onboard_hub.h
> > new file mode 100644
> > index 0000000..1b70ccc
> > --- /dev/null
> > +++ b/include/linux/usb/generic_onboard_hub.h
> > @@ -0,0 +1,11 @@
> > +#ifndef __LINUX_USB_GENERIC_HUB_H
> > +#define __LINUX_USB_GENERIC_HUB_H
> > +
> > +struct usb_hub_generic_platform_data {
> > +       int gpio_reset;
> > +       int gpio_reset_polarity;
> > +       int gpio_reset_duration_us;
> > +       struct clk *ext_clk;
> > +};
> 
> Please document this structure in accordance with kernel documentation
> standards.
> 

Thanks, it should be.

-- 

Best Regards,
Peter Chen
--
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

[PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Peter Chen]

On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> On 7 December 2015 at 18:37, Peter Chen <peter.chen@freescale.com> wrote:
> > +
> > +       if (dev->of_node) {
> > +               struct device_node *node = dev->of_node;
> > +
> > +               hub_data->clk = devm_clk_get(dev, "external_clk");
> > +               if (IS_ERR(hub_data->clk)) {
> > +                       dev_dbg(dev, "Can't get external clock: %ld\n",
> > +                                       PTR_ERR(hub_data->clk));
> > +               }
> 
> Is the intended behaviour to keep going here event when there is an
> error?  Can the "hub_data" really work without a clock?

Yes, some HUB may work with fixed 24M OSC at the board, but they need to
reset through external IO, so the clock is not need at this case, but
reset pin is mandatory.

> > +       }
> > +
> > +       if (gpiod_reset) {
> > +               /* Sanity check */
> > +               if (duration_us > 1000000)
> > +                       duration_us = 50;
> > +               usleep_range(duration_us, duration_us + 100);
> > +               gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);
> 
> What are these hard-coded values?  Shouldn't this be taken from the
> DT?  If not then some comments explaining the values would be
> appreciated.

Yes, I did it wrongly, thanks.

> > diff --git a/include/linux/usb/generic_onboard_hub.h b/include/linux/usb/generic_onboard_hub.h
> > new file mode 100644
> > index 0000000..1b70ccc
> > --- /dev/null
> > +++ b/include/linux/usb/generic_onboard_hub.h
> > @@ -0,0 +1,11 @@
> > +#ifndef __LINUX_USB_GENERIC_HUB_H
> > +#define __LINUX_USB_GENERIC_HUB_H
> > +
> > +struct usb_hub_generic_platform_data {
> > +       int gpio_reset;
> > +       int gpio_reset_polarity;
> > +       int gpio_reset_duration_us;
> > +       struct clk *ext_clk;
> > +};
> 
> Please document this structure in accordance with kernel documentation
> standards.
> 

Thanks, it should be.

-- 

Best Regards,
Peter Chen

Re: [PATCH 2/3] doc: dt-binding: generic onboard USB HUB

[Arnd Bergmann]

On Wednesday 09 December 2015 16:12:24 Peter Chen wrote:
> On Tue, Dec 08, 2015 at 09:24:03PM -0600, Rob Herring wrote:
> > On Tue, Dec 08, 2015 at 10:58:48AM +0100, Arnd Bergmann wrote:
> > > On Tuesday 08 December 2015 10:50:49 Philipp Zabel wrote:
> > > > This something we don't have to define ad-hoc. The hub hangs off an USB
> > > > controller, right? The "Open Firmware recommended practice: USB"
> > > > document already describes how to represent USB devices in a generic
> > > > manner:
> > > > http://www.firmware.org/1275/bindings/usb/usb-1_0.ps
> > > > 
> > > > Is there a reason not to reuse this?
> > > > 
> > > > The usb hub node would be a child of the usb controller node, and it
> > > > could use
> > > > 	compatible = "usb,class9"; /* bDeviceClass 9 (Hub) */
> > > 
> > > Good point, I had not thought of that when I looked at the patches.
> > >  
> > > Yes, let's do this way. I don't know if we ever implemented the simple
> > > patch to associate a USB device with a device_node, but if not, then
> > > let's do it now for this driver. A lot of people have asked for it in
> > > the past.
> > 
> > Agreed. Also, some hubs have I2C buses as well, but I still think under 
> > the USB bus is the right place.
> > 
> > However, one complication here is often (probably this case) these 
> > addtional signals need to be controlled before the device enumerates.
> > 
> 
> Yes, I did not find a way to let the USB bus code handle it, so I had to
> write a platform driver to do it

Looping in Ulf, he solved the same problem for SDIO devices recently,
and probably remembers the details best.

	Arnd
--
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

[PATCH 2/3] doc: dt-binding: generic onboard USB HUB

[Arnd Bergmann]

On Wednesday 09 December 2015 16:12:24 Peter Chen wrote:
> On Tue, Dec 08, 2015 at 09:24:03PM -0600, Rob Herring wrote:
> > On Tue, Dec 08, 2015 at 10:58:48AM +0100, Arnd Bergmann wrote:
> > > On Tuesday 08 December 2015 10:50:49 Philipp Zabel wrote:
> > > > This something we don't have to define ad-hoc. The hub hangs off an USB
> > > > controller, right? The "Open Firmware recommended practice: USB"
> > > > document already describes how to represent USB devices in a generic
> > > > manner:
> > > > http://www.firmware.org/1275/bindings/usb/usb-1_0.ps
> > > > 
> > > > Is there a reason not to reuse this?
> > > > 
> > > > The usb hub node would be a child of the usb controller node, and it
> > > > could use
> > > > 	compatible = "usb,class9"; /* bDeviceClass 9 (Hub) */
> > > 
> > > Good point, I had not thought of that when I looked at the patches.
> > >  
> > > Yes, let's do this way. I don't know if we ever implemented the simple
> > > patch to associate a USB device with a device_node, but if not, then
> > > let's do it now for this driver. A lot of people have asked for it in
> > > the past.
> > 
> > Agreed. Also, some hubs have I2C buses as well, but I still think under 
> > the USB bus is the right place.
> > 
> > However, one complication here is often (probably this case) these 
> > addtional signals need to be controlled before the device enumerates.
> > 
> 
> Yes, I did not find a way to let the USB bus code handle it, so I had to
> write a platform driver to do it

Looping in Ulf, he solved the same problem for SDIO devices recently,
and probably remembers the details best.

	Arnd

Re: [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Lucas Stach]

Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:
> On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> > On 7 December 2015 at 18:37, Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > > +
> > > +       if (dev->of_node) {
> > > +               struct device_node *node = dev->of_node;
> > > +
> > > +               hub_data->clk = devm_clk_get(dev, "external_clk");
> > > +               if (IS_ERR(hub_data->clk)) {
> > > +                       dev_dbg(dev, "Can't get external clock: %ld\n",
> > > +                                       PTR_ERR(hub_data->clk));
> > > +               }
> > 
> > Is the intended behaviour to keep going here event when there is an
> > error?  Can the "hub_data" really work without a clock?
> 
> Yes, some HUB may work with fixed 24M OSC at the board, but they need to
> reset through external IO, so the clock is not need at this case, but
> reset pin is mandatory.
> 
If the hub always requires a clock it must not be optional. If you have
a fixed 24MHz clock on board add this to the DT as a fixed-clock and use
it as an input to the hub.

> > > +       }
> > > +
> > > +       if (gpiod_reset) {
> > > +               /* Sanity check */
> > > +               if (duration_us > 1000000)
> > > +                       duration_us = 50;
> > > +               usleep_range(duration_us, duration_us + 100);
> > > +               gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);
> > 
> > What are these hard-coded values?  Shouldn't this be taken from the
> > DT?  If not then some comments explaining the values would be
> > appreciated.
> 
> Yes, I did it wrongly, thanks.
> 
> > > diff --git a/include/linux/usb/generic_onboard_hub.h b/include/linux/usb/generic_onboard_hub.h
> > > new file mode 100644
> > > index 0000000..1b70ccc
> > > --- /dev/null
> > > +++ b/include/linux/usb/generic_onboard_hub.h
> > > @@ -0,0 +1,11 @@
> > > +#ifndef __LINUX_USB_GENERIC_HUB_H
> > > +#define __LINUX_USB_GENERIC_HUB_H
> > > +
> > > +struct usb_hub_generic_platform_data {
> > > +       int gpio_reset;
> > > +       int gpio_reset_polarity;
> > > +       int gpio_reset_duration_us;
> > > +       struct clk *ext_clk;
> > > +};
> > 
> > Please document this structure in accordance with kernel documentation
> > standards.
> > 
> 
> Thanks, it should be.
> 

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

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

[PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Lucas Stach]

Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:
> On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> > On 7 December 2015 at 18:37, Peter Chen <peter.chen@freescale.com> wrote:
> > > +
> > > +       if (dev->of_node) {
> > > +               struct device_node *node = dev->of_node;
> > > +
> > > +               hub_data->clk = devm_clk_get(dev, "external_clk");
> > > +               if (IS_ERR(hub_data->clk)) {
> > > +                       dev_dbg(dev, "Can't get external clock: %ld\n",
> > > +                                       PTR_ERR(hub_data->clk));
> > > +               }
> > 
> > Is the intended behaviour to keep going here event when there is an
> > error?  Can the "hub_data" really work without a clock?
> 
> Yes, some HUB may work with fixed 24M OSC at the board, but they need to
> reset through external IO, so the clock is not need at this case, but
> reset pin is mandatory.
> 
If the hub always requires a clock it must not be optional. If you have
a fixed 24MHz clock on board add this to the DT as a fixed-clock and use
it as an input to the hub.

> > > +       }
> > > +
> > > +       if (gpiod_reset) {
> > > +               /* Sanity check */
> > > +               if (duration_us > 1000000)
> > > +                       duration_us = 50;
> > > +               usleep_range(duration_us, duration_us + 100);
> > > +               gpiod_set_value(gpiod_reset, reset_pol ? 0 : 1);
> > 
> > What are these hard-coded values?  Shouldn't this be taken from the
> > DT?  If not then some comments explaining the values would be
> > appreciated.
> 
> Yes, I did it wrongly, thanks.
> 
> > > diff --git a/include/linux/usb/generic_onboard_hub.h b/include/linux/usb/generic_onboard_hub.h
> > > new file mode 100644
> > > index 0000000..1b70ccc
> > > --- /dev/null
> > > +++ b/include/linux/usb/generic_onboard_hub.h
> > > @@ -0,0 +1,11 @@
> > > +#ifndef __LINUX_USB_GENERIC_HUB_H
> > > +#define __LINUX_USB_GENERIC_HUB_H
> > > +
> > > +struct usb_hub_generic_platform_data {
> > > +       int gpio_reset;
> > > +       int gpio_reset_polarity;
> > > +       int gpio_reset_duration_us;
> > > +       struct clk *ext_clk;
> > > +};
> > 
> > Please document this structure in accordance with kernel documentation
> > standards.
> > 
> 
> Thanks, it should be.
> 

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Peter Chen]

On Wed, Dec 09, 2015 at 09:57:40AM +0100, Lucas Stach wrote:
> Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:
> > On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> > > On 7 December 2015 at 18:37, Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > > > +
> > > > +       if (dev->of_node) {
> > > > +               struct device_node *node = dev->of_node;
> > > > +
> > > > +               hub_data->clk = devm_clk_get(dev, "external_clk");
> > > > +               if (IS_ERR(hub_data->clk)) {
> > > > +                       dev_dbg(dev, "Can't get external clock: %ld\n",
> > > > +                                       PTR_ERR(hub_data->clk));
> > > > +               }
> > > 
> > > Is the intended behaviour to keep going here event when there is an
> > > error?  Can the "hub_data" really work without a clock?
> > 
> > Yes, some HUB may work with fixed 24M OSC at the board, but they need to
> > reset through external IO, so the clock is not need at this case, but
> > reset pin is mandatory.
> > 
> If the hub always requires a clock it must not be optional. If you have
> a fixed 24MHz clock on board add this to the DT as a fixed-clock and use
> it as an input to the hub.
> 

This fixed 24MHz clock may from a fixed crystal on board, it is always
on, no software need to control it, imx51-bbg board is an example.

-- 

Best Regards,
Peter Chen
--
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

[PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Peter Chen]

On Wed, Dec 09, 2015 at 09:57:40AM +0100, Lucas Stach wrote:
> Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:
> > On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> > > On 7 December 2015 at 18:37, Peter Chen <peter.chen@freescale.com> wrote:
> > > > +
> > > > +       if (dev->of_node) {
> > > > +               struct device_node *node = dev->of_node;
> > > > +
> > > > +               hub_data->clk = devm_clk_get(dev, "external_clk");
> > > > +               if (IS_ERR(hub_data->clk)) {
> > > > +                       dev_dbg(dev, "Can't get external clock: %ld\n",
> > > > +                                       PTR_ERR(hub_data->clk));
> > > > +               }
> > > 
> > > Is the intended behaviour to keep going here event when there is an
> > > error?  Can the "hub_data" really work without a clock?
> > 
> > Yes, some HUB may work with fixed 24M OSC at the board, but they need to
> > reset through external IO, so the clock is not need at this case, but
> > reset pin is mandatory.
> > 
> If the hub always requires a clock it must not be optional. If you have
> a fixed 24MHz clock on board add this to the DT as a fixed-clock and use
> it as an input to the hub.
> 

This fixed 24MHz clock may from a fixed crystal on board, it is always
on, no software need to control it, imx51-bbg board is an example.

-- 

Best Regards,
Peter Chen

Re: [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Peter Chen]

On Wed, Dec 09, 2015 at 10:10:51AM +0100, Arnd Bergmann wrote:
> On Wednesday 09 December 2015 09:57:40 Lucas Stach wrote:
> > Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:
> > > On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> > > > On 7 December 2015 at 18:37, Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > > > > +
> > > > > +       if (dev->of_node) {
> > > > > +               struct device_node *node = dev->of_node;
> > > > > +
> > > > > +               hub_data->clk = devm_clk_get(dev, "external_clk");
> > > > > +               if (IS_ERR(hub_data->clk)) {
> > > > > +                       dev_dbg(dev, "Can't get external clock: %ld\n",
> > > > > +                                       PTR_ERR(hub_data->clk));
> > > > > +               }
> > > > 
> > > > Is the intended behaviour to keep going here event when there is an
> > > > error?  Can the "hub_data" really work without a clock?
> > > 
> > > Yes, some HUB may work with fixed 24M OSC at the board, but they need to
> > > reset through external IO, so the clock is not need at this case, but
> > > reset pin is mandatory.
> > > 
> > If the hub always requires a clock it must not be optional. If you have
> > a fixed 24MHz clock on board add this to the DT as a fixed-clock and use
> > it as an input to the hub.
> 
> I think it's fine to make the clock optional in the sense that you only
> need to list non-fixed clocks that have to be enabled and or controlled.
> 
> Which reminds me, should the driver call clk_set_rate()? It currently
> doesn't, but other machines might need that.
> 

Yes, you are right, I will add it at v2.

-- 

Best Regards,
Peter Chen
--
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

[PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Peter Chen]

On Wed, Dec 09, 2015 at 10:10:51AM +0100, Arnd Bergmann wrote:
> On Wednesday 09 December 2015 09:57:40 Lucas Stach wrote:
> > Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:
> > > On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> > > > On 7 December 2015 at 18:37, Peter Chen <peter.chen@freescale.com> wrote:
> > > > > +
> > > > > +       if (dev->of_node) {
> > > > > +               struct device_node *node = dev->of_node;
> > > > > +
> > > > > +               hub_data->clk = devm_clk_get(dev, "external_clk");
> > > > > +               if (IS_ERR(hub_data->clk)) {
> > > > > +                       dev_dbg(dev, "Can't get external clock: %ld\n",
> > > > > +                                       PTR_ERR(hub_data->clk));
> > > > > +               }
> > > > 
> > > > Is the intended behaviour to keep going here event when there is an
> > > > error?  Can the "hub_data" really work without a clock?
> > > 
> > > Yes, some HUB may work with fixed 24M OSC at the board, but they need to
> > > reset through external IO, so the clock is not need at this case, but
> > > reset pin is mandatory.
> > > 
> > If the hub always requires a clock it must not be optional. If you have
> > a fixed 24MHz clock on board add this to the DT as a fixed-clock and use
> > it as an input to the hub.
> 
> I think it's fine to make the clock optional in the sense that you only
> need to list non-fixed clocks that have to be enabled and or controlled.
> 
> Which reminds me, should the driver call clk_set_rate()? It currently
> doesn't, but other machines might need that.
> 

Yes, you are right, I will add it at v2.

-- 

Best Regards,
Peter Chen

Re: [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Arnd Bergmann]

On Wednesday 09 December 2015 09:57:40 Lucas Stach wrote:
> Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:
> > On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> > > On 7 December 2015 at 18:37, Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > > > +
> > > > +       if (dev->of_node) {
> > > > +               struct device_node *node = dev->of_node;
> > > > +
> > > > +               hub_data->clk = devm_clk_get(dev, "external_clk");
> > > > +               if (IS_ERR(hub_data->clk)) {
> > > > +                       dev_dbg(dev, "Can't get external clock: %ld\n",
> > > > +                                       PTR_ERR(hub_data->clk));
> > > > +               }
> > > 
> > > Is the intended behaviour to keep going here event when there is an
> > > error?  Can the "hub_data" really work without a clock?
> > 
> > Yes, some HUB may work with fixed 24M OSC at the board, but they need to
> > reset through external IO, so the clock is not need at this case, but
> > reset pin is mandatory.
> > 
> If the hub always requires a clock it must not be optional. If you have
> a fixed 24MHz clock on board add this to the DT as a fixed-clock and use
> it as an input to the hub.

I think it's fine to make the clock optional in the sense that you only
need to list non-fixed clocks that have to be enabled and or controlled.

Which reminds me, should the driver call clk_set_rate()? It currently
doesn't, but other machines might need that.

	Arnd
--
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

[PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Arnd Bergmann]

On Wednesday 09 December 2015 09:57:40 Lucas Stach wrote:
> Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:
> > On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> > > On 7 December 2015 at 18:37, Peter Chen <peter.chen@freescale.com> wrote:
> > > > +
> > > > +       if (dev->of_node) {
> > > > +               struct device_node *node = dev->of_node;
> > > > +
> > > > +               hub_data->clk = devm_clk_get(dev, "external_clk");
> > > > +               if (IS_ERR(hub_data->clk)) {
> > > > +                       dev_dbg(dev, "Can't get external clock: %ld\n",
> > > > +                                       PTR_ERR(hub_data->clk));
> > > > +               }
> > > 
> > > Is the intended behaviour to keep going here event when there is an
> > > error?  Can the "hub_data" really work without a clock?
> > 
> > Yes, some HUB may work with fixed 24M OSC at the board, but they need to
> > reset through external IO, so the clock is not need at this case, but
> > reset pin is mandatory.
> > 
> If the hub always requires a clock it must not be optional. If you have
> a fixed 24MHz clock on board add this to the DT as a fixed-clock and use
> it as an input to the hub.

I think it's fine to make the clock optional in the sense that you only
need to list non-fixed clocks that have to be enabled and or controlled.

Which reminds me, should the driver call clk_set_rate()? It currently
doesn't, but other machines might need that.

	Arnd

Re: [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Lucas Stach]

Am Mittwoch, den 09.12.2015, 17:00 +0800 schrieb Peter Chen:
> On Wed, Dec 09, 2015 at 09:57:40AM +0100, Lucas Stach wrote:
> > Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:
> > > On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> > > > On 7 December 2015 at 18:37, Peter Chen <peter.chen-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > > > > +
> > > > > +       if (dev->of_node) {
> > > > > +               struct device_node *node = dev->of_node;
> > > > > +
> > > > > +               hub_data->clk = devm_clk_get(dev, "external_clk");
> > > > > +               if (IS_ERR(hub_data->clk)) {
> > > > > +                       dev_dbg(dev, "Can't get external clock: %ld\n",
> > > > > +                                       PTR_ERR(hub_data->clk));
> > > > > +               }
> > > > 
> > > > Is the intended behaviour to keep going here event when there is an
> > > > error?  Can the "hub_data" really work without a clock?
> > > 
> > > Yes, some HUB may work with fixed 24M OSC at the board, but they need to
> > > reset through external IO, so the clock is not need at this case, but
> > > reset pin is mandatory.
> > > 
> > If the hub always requires a clock it must not be optional. If you have
> > a fixed 24MHz clock on board add this to the DT as a fixed-clock and use
> > it as an input to the hub.
> > 
> 
> This fixed 24MHz clock may from a fixed crystal on board, it is always
> on, no software need to control it, imx51-bbg board is an example.
> 
And that's wh it is a fixed-clock. Please look it up in the DT binding
documentation.
-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

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

[PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Lucas Stach]

Am Mittwoch, den 09.12.2015, 17:00 +0800 schrieb Peter Chen:
> On Wed, Dec 09, 2015 at 09:57:40AM +0100, Lucas Stach wrote:
> > Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:
> > > On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> > > > On 7 December 2015 at 18:37, Peter Chen <peter.chen@freescale.com> wrote:
> > > > > +
> > > > > +       if (dev->of_node) {
> > > > > +               struct device_node *node = dev->of_node;
> > > > > +
> > > > > +               hub_data->clk = devm_clk_get(dev, "external_clk");
> > > > > +               if (IS_ERR(hub_data->clk)) {
> > > > > +                       dev_dbg(dev, "Can't get external clock: %ld\n",
> > > > > +                                       PTR_ERR(hub_data->clk));
> > > > > +               }
> > > > 
> > > > Is the intended behaviour to keep going here event when there is an
> > > > error?  Can the "hub_data" really work without a clock?
> > > 
> > > Yes, some HUB may work with fixed 24M OSC at the board, but they need to
> > > reset through external IO, so the clock is not need at this case, but
> > > reset pin is mandatory.
> > > 
> > If the hub always requires a clock it must not be optional. If you have
> > a fixed 24MHz clock on board add this to the DT as a fixed-clock and use
> > it as an input to the hub.
> > 
> 
> This fixed 24MHz clock may from a fixed crystal on board, it is always
> on, no software need to control it, imx51-bbg board is an example.
> 
And that's wh it is a fixed-clock. Please look it up in the DT binding
documentation.
-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

RE: [PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Peter Chen]

Best regards,
Peter Chen

> -----Original Message-----
> From: Lucas Stach [mailto:l.stach@pengutronix.de]
> Sent: Wednesday, December 09, 2015 5:13 PM
> To: Chen Peter-B29397 <Peter.Chen@freescale.com>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>; Mark Rutland
> <mark.rutland@arm.com>; devicetree@vger.kernel.org; festevam@gmail.com;
> Philipp Zabel <p.zabel@pengutronix.de>; Paweł Moll <pawel.moll@arm.com>;
> Greg KH <gregkh@linuxfoundation.org>; linux-usb@vger.kernel.org;
> patryk@kowalczyk.ws; Rob Herring <robh+dt@kernel.org>;
> stern@rowland.harvard.edu; kernel@pengutronix.de; Shawn Guo
> <shawnguo@kernel.org>; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 1/3] usb: misc: generic_onboard_hub: add generic
> onboard USB HUB driver
> 
> Am Mittwoch, den 09.12.2015, 17:00 +0800 schrieb Peter Chen:
> > On Wed, Dec 09, 2015 at 09:57:40AM +0100, Lucas Stach wrote:
> > > Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:
> > > > On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> > > > > On 7 December 2015 at 18:37, Peter Chen <peter.chen@freescale.com>
> wrote:
> > > > > > +
> > > > > > +       if (dev->of_node) {
> > > > > > +               struct device_node *node = dev->of_node;
> > > > > > +
> > > > > > +               hub_data->clk = devm_clk_get(dev, "external_clk");
> > > > > > +               if (IS_ERR(hub_data->clk)) {
> > > > > > +                       dev_dbg(dev, "Can't get external clock: %ld\n",
> > > > > > +                                       PTR_ERR(hub_data->clk));
> > > > > > +               }
> > > > >
> > > > > Is the intended behaviour to keep going here event when there is
> > > > > an error?  Can the "hub_data" really work without a clock?
> > > >
> > > > Yes, some HUB may work with fixed 24M OSC at the board, but they
> > > > need to reset through external IO, so the clock is not need at
> > > > this case, but reset pin is mandatory.
> > > >
> > > If the hub always requires a clock it must not be optional. If you
> > > have a fixed 24MHz clock on board add this to the DT as a
> > > fixed-clock and use it as an input to the hub.
> > >
> >
> > This fixed 24MHz clock may from a fixed crystal on board, it is always
> > on, no software need to control it, imx51-bbg board is an example.
> >
> And that's wh it is a fixed-clock. Please look it up in the DT binding
> documentation.

Yes, I see similar things are described at [1], but why we need this
additional entry at dts for this case,  it is always on, no software
is needed.
 
[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
 
Peter

[PATCH 1/3] usb: misc: generic_onboard_hub: add generic onboard USB HUB driver

[Peter Chen]

Best regards,
Peter Chen

> -----Original Message-----
> From: Lucas Stach [mailto:l.stach at pengutronix.de]
> Sent: Wednesday, December 09, 2015 5:13 PM
> To: Chen Peter-B29397 <Peter.Chen@freescale.com>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>; Mark Rutland
> <mark.rutland@arm.com>; devicetree at vger.kernel.org; festevam at gmail.com;
> Philipp Zabel <p.zabel@pengutronix.de>; Pawe? Moll <pawel.moll@arm.com>;
> Greg KH <gregkh@linuxfoundation.org>; linux-usb at vger.kernel.org;
> patryk at kowalczyk.ws; Rob Herring <robh+dt@kernel.org>;
> stern at rowland.harvard.edu; kernel at pengutronix.de; Shawn Guo
> <shawnguo@kernel.org>; linux-arm-kernel at lists.infradead.org
> Subject: Re: [PATCH 1/3] usb: misc: generic_onboard_hub: add generic
> onboard USB HUB driver
> 
> Am Mittwoch, den 09.12.2015, 17:00 +0800 schrieb Peter Chen:
> > On Wed, Dec 09, 2015 at 09:57:40AM +0100, Lucas Stach wrote:
> > > Am Mittwoch, den 09.12.2015, 16:50 +0800 schrieb Peter Chen:
> > > > On Tue, Dec 08, 2015 at 08:36:00AM -0700, Mathieu Poirier wrote:
> > > > > On 7 December 2015 at 18:37, Peter Chen <peter.chen@freescale.com>
> wrote:
> > > > > > +
> > > > > > +       if (dev->of_node) {
> > > > > > +               struct device_node *node = dev->of_node;
> > > > > > +
> > > > > > +               hub_data->clk = devm_clk_get(dev, "external_clk");
> > > > > > +               if (IS_ERR(hub_data->clk)) {
> > > > > > +                       dev_dbg(dev, "Can't get external clock: %ld\n",
> > > > > > +                                       PTR_ERR(hub_data->clk));
> > > > > > +               }
> > > > >
> > > > > Is the intended behaviour to keep going here event when there is
> > > > > an error?  Can the "hub_data" really work without a clock?
> > > >
> > > > Yes, some HUB may work with fixed 24M OSC at the board, but they
> > > > need to reset through external IO, so the clock is not need at
> > > > this case, but reset pin is mandatory.
> > > >
> > > If the hub always requires a clock it must not be optional. If you
> > > have a fixed 24MHz clock on board add this to the DT as a
> > > fixed-clock and use it as an input to the hub.
> > >
> >
> > This fixed 24MHz clock may from a fixed crystal on board, it is always
> > on, no software need to control it, imx51-bbg board is an example.
> >
> And that's wh it is a fixed-clock. Please look it up in the DT binding
> documentation.

Yes, I see similar things are described at [1], but why we need this
additional entry at dts for this case,  it is always on, no software
is needed.
 
[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
 
Peter

Re: [PATCH 2/3] doc: dt-binding: generic onboard USB HUB

[Ulf Hansson]

On 9 December 2015 at 09:55, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> On Wednesday 09 December 2015 16:12:24 Peter Chen wrote:
>> On Tue, Dec 08, 2015 at 09:24:03PM -0600, Rob Herring wrote:
>> > On Tue, Dec 08, 2015 at 10:58:48AM +0100, Arnd Bergmann wrote:
>> > > On Tuesday 08 December 2015 10:50:49 Philipp Zabel wrote:
>> > > > This something we don't have to define ad-hoc. The hub hangs off an USB
>> > > > controller, right? The "Open Firmware recommended practice: USB"
>> > > > document already describes how to represent USB devices in a generic
>> > > > manner:
>> > > > http://www.firmware.org/1275/bindings/usb/usb-1_0.ps
>> > > >
>> > > > Is there a reason not to reuse this?
>> > > >
>> > > > The usb hub node would be a child of the usb controller node, and it
>> > > > could use
>> > > >         compatible = "usb,class9"; /* bDeviceClass 9 (Hub) */
>> > >
>> > > Good point, I had not thought of that when I looked at the patches.
>> > >
>> > > Yes, let's do this way. I don't know if we ever implemented the simple
>> > > patch to associate a USB device with a device_node, but if not, then
>> > > let's do it now for this driver. A lot of people have asked for it in
>> > > the past.
>> >
>> > Agreed. Also, some hubs have I2C buses as well, but I still think under
>> > the USB bus is the right place.
>> >
>> > However, one complication here is often (probably this case) these
>> > addtional signals need to be controlled before the device enumerates.
>> >
>>
>> Yes, I did not find a way to let the USB bus code handle it, so I had to
>> write a platform driver to do it
>
> Looping in Ulf, he solved the same problem for SDIO devices recently,
> and probably remembers the details best.
>

Thanks Arnd!

Yes, that was a kind of a long outstanding issue we have had for SDIO devices.

Several generic attempts was made to have a framework/library
available to support so called "power sequences" for exactly the same
reasons as above.
Others and myself failed to get those attempts accepted.

Instead, I invented a mmc subsystem specific DT based solution, the
"mmc-pwrseq".

DT documentation:
Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
Documentation/devicetree/bindings/mmc/mmc.txt

Long story short: The mmc host device may contain a phandle to a
mmc-pwrseq, which will describe the resources needed to power on/off
the SDIO card.

The code is available at:
drivers/mmc/core/pwrseq*

We didn't implement this as platform driver, but that was mostly
because I initially wanted things to be simple. Although, nothing
prevents us from converting to this as a follow up, which would make
the solution a bit less "hacky".

Kind regards
Uffe
--
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

[PATCH 2/3] doc: dt-binding: generic onboard USB HUB

[Ulf Hansson]

On 9 December 2015 at 09:55, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 09 December 2015 16:12:24 Peter Chen wrote:
>> On Tue, Dec 08, 2015 at 09:24:03PM -0600, Rob Herring wrote:
>> > On Tue, Dec 08, 2015 at 10:58:48AM +0100, Arnd Bergmann wrote:
>> > > On Tuesday 08 December 2015 10:50:49 Philipp Zabel wrote:
>> > > > This something we don't have to define ad-hoc. The hub hangs off an USB
>> > > > controller, right? The "Open Firmware recommended practice: USB"
>> > > > document already describes how to represent USB devices in a generic
>> > > > manner:
>> > > > http://www.firmware.org/1275/bindings/usb/usb-1_0.ps
>> > > >
>> > > > Is there a reason not to reuse this?
>> > > >
>> > > > The usb hub node would be a child of the usb controller node, and it
>> > > > could use
>> > > >         compatible = "usb,class9"; /* bDeviceClass 9 (Hub) */
>> > >
>> > > Good point, I had not thought of that when I looked at the patches.
>> > >
>> > > Yes, let's do this way. I don't know if we ever implemented the simple
>> > > patch to associate a USB device with a device_node, but if not, then
>> > > let's do it now for this driver. A lot of people have asked for it in
>> > > the past.
>> >
>> > Agreed. Also, some hubs have I2C buses as well, but I still think under
>> > the USB bus is the right place.
>> >
>> > However, one complication here is often (probably this case) these
>> > addtional signals need to be controlled before the device enumerates.
>> >
>>
>> Yes, I did not find a way to let the USB bus code handle it, so I had to
>> write a platform driver to do it
>
> Looping in Ulf, he solved the same problem for SDIO devices recently,
> and probably remembers the details best.
>

Thanks Arnd!

Yes, that was a kind of a long outstanding issue we have had for SDIO devices.

Several generic attempts was made to have a framework/library
available to support so called "power sequences" for exactly the same
reasons as above.
Others and myself failed to get those attempts accepted.

Instead, I invented a mmc subsystem specific DT based solution, the
"mmc-pwrseq".

DT documentation:
Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
Documentation/devicetree/bindings/mmc/mmc.txt

Long story short: The mmc host device may contain a phandle to a
mmc-pwrseq, which will describe the resources needed to power on/off
the SDIO card.

The code is available at:
drivers/mmc/core/pwrseq*

We didn't implement this as platform driver, but that was mostly
because I initially wanted things to be simple. Although, nothing
prevents us from converting to this as a follow up, which would make
the solution a bit less "hacky".

Kind regards
Uffe

Re: [PATCH 2/3] doc: dt-binding: generic onboard USB HUB

[Peter Chen]

On Tue, Dec 15, 2015 at 09:21:09PM +0100, Ulf Hansson wrote:
> On 9 December 2015 at 09:55, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wednesday 09 December 2015 16:12:24 Peter Chen wrote:
> >> On Tue, Dec 08, 2015 at 09:24:03PM -0600, Rob Herring wrote:
> >> > On Tue, Dec 08, 2015 at 10:58:48AM +0100, Arnd Bergmann wrote:
> >> > > On Tuesday 08 December 2015 10:50:49 Philipp Zabel wrote:
> >> > > > This something we don't have to define ad-hoc. The hub hangs off an USB
> >> > > > controller, right? The "Open Firmware recommended practice: USB"
> >> > > > document already describes how to represent USB devices in a generic
> >> > > > manner:
> >> > > > http://www.firmware.org/1275/bindings/usb/usb-1_0.ps
> >> > > >
> >> > > > Is there a reason not to reuse this?
> >> > > >
> >> > > > The usb hub node would be a child of the usb controller node, and it
> >> > > > could use
> >> > > >         compatible = "usb,class9"; /* bDeviceClass 9 (Hub) */
> >> > >
> >> > > Good point, I had not thought of that when I looked at the patches.
> >> > >
> >> > > Yes, let's do this way. I don't know if we ever implemented the simple
> >> > > patch to associate a USB device with a device_node, but if not, then
> >> > > let's do it now for this driver. A lot of people have asked for it in
> >> > > the past.
> >> >
> >> > Agreed. Also, some hubs have I2C buses as well, but I still think under
> >> > the USB bus is the right place.
> >> >
> >> > However, one complication here is often (probably this case) these
> >> > addtional signals need to be controlled before the device enumerates.
> >> >
> >>
> >> Yes, I did not find a way to let the USB bus code handle it, so I had to
> >> write a platform driver to do it
> >
> > Looping in Ulf, he solved the same problem for SDIO devices recently,
> > and probably remembers the details best.
> >
> 
> Thanks Arnd!
> 
> Yes, that was a kind of a long outstanding issue we have had for SDIO devices.
> 
> Several generic attempts was made to have a framework/library
> available to support so called "power sequences" for exactly the same
> reasons as above.
> Others and myself failed to get those attempts accepted.
> 
> Instead, I invented a mmc subsystem specific DT based solution, the
> "mmc-pwrseq".
> 
> DT documentation:
> Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
> Documentation/devicetree/bindings/mmc/mmc.txt
> 
> Long story short: The mmc host device may contain a phandle to a
> mmc-pwrseq, which will describe the resources needed to power on/off
> the SDIO card.
> 
> The code is available at:
> drivers/mmc/core/pwrseq*
> 
> We didn't implement this as platform driver, but that was mostly
> because I initially wanted things to be simple. Although, nothing
> prevents us from converting to this as a follow up, which would make
> the solution a bit less "hacky".
> 

Thanks for your information, Ulf.

-- 

Best Regards,
Peter Chen

[PATCH 2/3] doc: dt-binding: generic onboard USB HUB

[Peter Chen]

On Tue, Dec 15, 2015 at 09:21:09PM +0100, Ulf Hansson wrote:
> On 9 December 2015 at 09:55, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Wednesday 09 December 2015 16:12:24 Peter Chen wrote:
> >> On Tue, Dec 08, 2015 at 09:24:03PM -0600, Rob Herring wrote:
> >> > On Tue, Dec 08, 2015 at 10:58:48AM +0100, Arnd Bergmann wrote:
> >> > > On Tuesday 08 December 2015 10:50:49 Philipp Zabel wrote:
> >> > > > This something we don't have to define ad-hoc. The hub hangs off an USB
> >> > > > controller, right? The "Open Firmware recommended practice: USB"
> >> > > > document already describes how to represent USB devices in a generic
> >> > > > manner:
> >> > > > http://www.firmware.org/1275/bindings/usb/usb-1_0.ps
> >> > > >
> >> > > > Is there a reason not to reuse this?
> >> > > >
> >> > > > The usb hub node would be a child of the usb controller node, and it
> >> > > > could use
> >> > > >         compatible = "usb,class9"; /* bDeviceClass 9 (Hub) */
> >> > >
> >> > > Good point, I had not thought of that when I looked at the patches.
> >> > >
> >> > > Yes, let's do this way. I don't know if we ever implemented the simple
> >> > > patch to associate a USB device with a device_node, but if not, then
> >> > > let's do it now for this driver. A lot of people have asked for it in
> >> > > the past.
> >> >
> >> > Agreed. Also, some hubs have I2C buses as well, but I still think under
> >> > the USB bus is the right place.
> >> >
> >> > However, one complication here is often (probably this case) these
> >> > addtional signals need to be controlled before the device enumerates.
> >> >
> >>
> >> Yes, I did not find a way to let the USB bus code handle it, so I had to
> >> write a platform driver to do it
> >
> > Looping in Ulf, he solved the same problem for SDIO devices recently,
> > and probably remembers the details best.
> >
> 
> Thanks Arnd!
> 
> Yes, that was a kind of a long outstanding issue we have had for SDIO devices.
> 
> Several generic attempts was made to have a framework/library
> available to support so called "power sequences" for exactly the same
> reasons as above.
> Others and myself failed to get those attempts accepted.
> 
> Instead, I invented a mmc subsystem specific DT based solution, the
> "mmc-pwrseq".
> 
> DT documentation:
> Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt
> Documentation/devicetree/bindings/mmc/mmc.txt
> 
> Long story short: The mmc host device may contain a phandle to a
> mmc-pwrseq, which will describe the resources needed to power on/off
> the SDIO card.
> 
> The code is available at:
> drivers/mmc/core/pwrseq*
> 
> We didn't implement this as platform driver, but that was mostly
> because I initially wanted things to be simple. Although, nothing
> prevents us from converting to this as a follow up, which would make
> the solution a bit less "hacky".
> 

Thanks for your information, Ulf.

-- 

Best Regards,
Peter Chen