[PATCH 1/2] usb: core: add support for HCD providers

[PATCH 1/2] usb: core: add support for HCD providers

[Rafał Miłecki]

When working with Device Tree we may need to reference controllers
(their nodes) and query for HCDs. This is useful for getting some
runtime info about host controllers like e.g. assigned bus number.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 drivers/usb/core/Makefile    |  1 +
 drivers/usb/core/provider.c  | 79 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb/provider.h | 39 ++++++++++++++++++++++
 3 files changed, 119 insertions(+)
 create mode 100644 drivers/usb/core/provider.c
 create mode 100644 include/linux/usb/provider.h

diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
index 9780877..20b91d1 100644
--- a/drivers/usb/core/Makefile
+++ b/drivers/usb/core/Makefile
@@ -9,5 +9,6 @@ usbcore-y += port.o of.o
 
 usbcore-$(CONFIG_PCI)		+= hcd-pci.o
 usbcore-$(CONFIG_ACPI)		+= usb-acpi.o
+usbcore-$(CONFIG_OF)		+= provider.o
 
 obj-$(CONFIG_USB)		+= usbcore.o
diff --git a/drivers/usb/core/provider.c b/drivers/usb/core/provider.c
new file mode 100644
index 0000000..4b9165a
--- /dev/null
+++ b/drivers/usb/core/provider.c
@@ -0,0 +1,79 @@
+#include <linux/slab.h>
+#include <linux/usb/provider.h>
+
+static DEFINE_MUTEX(hcd_provider_mutex);
+static LIST_HEAD(hcd_provider_list);
+
+struct hcd_provider {
+	struct device_node *np;
+	struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data);
+	void *data;
+	struct list_head list;
+};
+
+struct hcd_provider *of_hcd_provider_register(struct device_node *np,
+					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),
+					      void *data)
+{
+	struct hcd_provider *hcd_provider;
+
+	if (!np)
+		return ERR_PTR(-EINVAL);
+
+	hcd_provider = kzalloc(sizeof(*hcd_provider), GFP_KERNEL);
+	if (!hcd_provider)
+		return ERR_PTR(-ENOMEM);
+
+	hcd_provider->np = np;
+	hcd_provider->of_xlate = of_xlate;
+	hcd_provider->data = data;
+
+	mutex_lock(&hcd_provider_mutex);
+	list_add_tail(&hcd_provider->list, &hcd_provider_list);
+	mutex_unlock(&hcd_provider_mutex);
+
+	return hcd_provider;
+}
+EXPORT_SYMBOL_GPL(of_hcd_provider_register);
+
+void of_hcd_provider_unregister(struct hcd_provider *hcd_provider)
+{
+	if (IS_ERR(hcd_provider))
+		return;
+
+	mutex_lock(&hcd_provider_mutex);
+	list_del(&hcd_provider->list);
+	mutex_unlock(&hcd_provider_mutex);
+
+	kfree(hcd_provider);
+}
+EXPORT_SYMBOL_GPL(of_hcd_provider_unregister);
+
+struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args, void *data)
+{
+	if (args->args_count != 0)
+		return ERR_PTR(-EINVAL);
+	return data;
+}
+EXPORT_SYMBOL_GPL(of_hcd_xlate_simple);
+
+struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args)
+{
+	struct usb_hcd *hcd = ERR_PTR(-ENOENT);
+	struct hcd_provider *provider;
+
+	if (!args)
+		return ERR_PTR(-EINVAL);
+
+	mutex_lock(&hcd_provider_mutex);
+	list_for_each_entry(provider, &hcd_provider_list, list) {
+		if (provider->np == args->np) {
+			hcd = provider->of_xlate(args, provider->data);
+			break;
+		}
+	}
+	mutex_unlock(&hcd_provider_mutex);
+
+	return hcd;
+}
+EXPORT_SYMBOL_GPL(of_hcd_get_from_provider);
diff --git a/include/linux/usb/provider.h b/include/linux/usb/provider.h
new file mode 100644
index 0000000..c66e006
--- /dev/null
+++ b/include/linux/usb/provider.h
@@ -0,0 +1,39 @@
+#ifndef __USB_CORE_PROVIDER_H
+#define __USB_CORE_PROVIDER_H
+
+#include <linux/of.h>
+#include <linux/usb.h>
+#include <linux/usb/hcd.h>
+
+struct hcd_provider;
+
+#ifdef CONFIG_OF
+struct hcd_provider *of_hcd_provider_register(struct device_node *np,
+					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),
+					      void *data);
+void of_hcd_provider_unregister(struct hcd_provider *hcd_provider);
+struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args, void *data);
+struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args);
+#else
+static inline
+struct hcd_provider *of_hcd_provider_register(struct device_node *np,
+					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),
+					      void *data)
+{
+	return ERR_PTR(-ENOSYS);
+}
+static inline void of_hcd_provider_unregister(struct hcd_provider *hcd_provider)
+{
+}
+static inline struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args,
+						  void *data)
+{
+	return ERR_PTR(-ENOSYS);
+}
+static inline struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args)
+{
+	return NULL;
+}
+#endif
+
+#endif /* __USB_CORE_PROVIDER_H */
-- 
1.8.4.5

[PATCH 2/2] ohci-platform: register HCD provider

[Rafał Miłecki]

This allows platforms using e.g. "generic-ohci" to reference HCD using
recently introduced providers mechanism

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 drivers/usb/host/ohci-platform.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c
index 898b740..57be81c 100644
--- a/drivers/usb/host/ohci-platform.c
+++ b/drivers/usb/host/ohci-platform.c
@@ -28,6 +28,7 @@
 #include <linux/usb/ohci_pdriver.h>
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
+#include <linux/usb/provider.h>
 
 #include "ohci.h"
 
@@ -40,6 +41,7 @@ struct ohci_platform_priv {
 	struct clk *clks[OHCI_MAX_CLKS];
 	struct reset_control *resets[OHCI_MAX_RESETS];
 	struct phy **phys;
+	struct hcd_provider *hcd_provider;
 	int num_phys;
 };
 
@@ -258,6 +260,11 @@ static int ohci_platform_probe(struct platform_device *dev)
 	if (err)
 		goto err_power;
 
+	if (dev->dev.of_node)
+		priv->hcd_provider = of_hcd_provider_register(dev->dev.of_node,
+							      of_hcd_xlate_simple,
+							      hcd);
+
 	device_wakeup_enable(hcd->self.controller);
 
 	platform_set_drvdata(dev, hcd);
@@ -289,6 +296,8 @@ static int ohci_platform_remove(struct platform_device *dev)
 	struct ohci_platform_priv *priv = hcd_to_ohci_priv(hcd);
 	int clk, rst;
 
+	of_hcd_provider_unregister(priv->hcd_provider);
+
 	usb_remove_hcd(hcd);
 
 	if (pdata->power_off)
-- 
1.8.4.5

[PATCH PROOF OF CONCEPT 3/2] trigger: ledtrig-usbport: read initial state from DT

[Rafał Miłecki]

This allows specifying USB ports that should be observed by a trigger
right after activating it. Example:

usb {
	gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
	linux,default-trigger = "usbport";
	usb-controllers = <&ohci>, <&ehci>, <&xhci>;
	usb-ports = "1", "1", "1";
};

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 drivers/leds/trigger/ledtrig-usbport.c | 72 ++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-usbport.c b/drivers/leds/trigger/ledtrig-usbport.c
index eb20a8f..5724f63 100644
--- a/drivers/leds/trigger/ledtrig-usbport.c
+++ b/drivers/leds/trigger/ledtrig-usbport.c
@@ -12,8 +12,10 @@
 #include <linux/device.h>
 #include <linux/leds.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/slab.h>
 #include <linux/usb.h>
+#include <linux/usb/provider.h>
 #include "../leds.h"
 
 struct usbport_trig_port {
@@ -94,6 +96,75 @@ static bool usbport_trig_match(struct usbport_trig_data *usbport_data,
 	return false;
 }
 
+static int usbport_trig_new_port(struct usbport_trig_data *usbport_data,
+				 int busnum, const char *suffix)
+{
+	struct usbport_trig_port *port;
+	size_t len;
+	int tmp;
+
+	tmp = busnum;
+	len = 1;
+	while (tmp >= 10) {
+		tmp /= 10;
+		len++;
+	}
+	len++; /* '-' */
+	len += strlen(suffix);
+	len++; /* '\0' */
+
+	port = kzalloc(sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return -ENOMEM;
+
+	port->name = kzalloc(len, GFP_KERNEL);
+	if (!port->name) {
+		kfree(port);
+		return -ENOMEM;
+	}
+	snprintf(port->name, len, "%d-%s", busnum, suffix);
+
+	list_add_tail(&port->list, &usbport_data->ports);
+
+	return 0;
+}
+
+static int usbport_trig_fill(struct usbport_trig_data *usbport_data)
+{
+	struct device_node *np = usbport_data->led_cdev->dev->of_node;
+	struct of_phandle_args args;
+	int count, i;
+	int err = 0;
+
+	if (!np)
+		return -ENOENT;
+
+	count = of_property_count_strings(np, "usb-ports");
+	if (count < 0)
+		return count;
+
+	for (i = 0; i < count; i++) {
+		const char *port;
+		struct usb_hcd *hcd;
+
+		err = of_property_read_string_index(np, "usb-ports", i, &port);
+		if (err)
+			continue;
+
+		err = of_parse_phandle_with_args(np, "usb-controllers", "#usb-cells", i, &args);
+		if (err)
+			continue;
+
+		hcd = of_hcd_get_from_provider(&args);
+		if (!IS_ERR(hcd))
+			usbport_trig_new_port(usbport_data, hcd->self.busnum, port);
+
+		of_node_put(args.np);
+	}
+
+	return err;
+}
+
 static int usbport_trig_notify(struct notifier_block *nb, unsigned long action,
 			       void *data)
 {
@@ -129,6 +200,7 @@ static void usbport_trig_activate(struct led_classdev *led_cdev)
 		return;
 	usbport_data->led_cdev = led_cdev;
 	INIT_LIST_HEAD(&usbport_data->ports);
+	usbport_trig_fill(usbport_data);
 	usbport_data->nb.notifier_call = usbport_trig_notify,
 	led_cdev->trigger_data = usbport_data;
 
-- 
1.8.4.5

[PATCH V2 1/1] usb: core: add support for HCD providers

[Rafał Miłecki]

When working with Device Tree we may need to reference controllers
(their nodes) and query for HCDs. This is useful for getting some
runtime info about host controllers like e.g. assigned bus number.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 drivers/usb/core/Makefile     |  1 +
 drivers/usb/core/hcd.c        |  8 ++++
 drivers/usb/core/provider.c   | 99 +++++++++++++++++++++++++++++++++++++++++++
 include/dt-bindings/usb/usb.h |  7 +++
 include/linux/usb/hcd.h       |  2 +
 include/linux/usb/provider.h  | 39 +++++++++++++++++
 6 files changed, 156 insertions(+)
 create mode 100644 drivers/usb/core/provider.c
 create mode 100644 include/dt-bindings/usb/usb.h
 create mode 100644 include/linux/usb/provider.h

diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
index 9780877..20b91d1 100644
--- a/drivers/usb/core/Makefile
+++ b/drivers/usb/core/Makefile
@@ -9,5 +9,6 @@ usbcore-y += port.o of.o
 
 usbcore-$(CONFIG_PCI)		+= hcd-pci.o
 usbcore-$(CONFIG_ACPI)		+= usb-acpi.o
+usbcore-$(CONFIG_OF)		+= provider.o
 
 obj-$(CONFIG_USB)		+= usbcore.o
diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index d2e3f65..55079a0 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -46,6 +46,7 @@
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
 #include <linux/usb/phy.h>
+#include <linux/usb/provider.h>
 
 #include "usb.h"
 
@@ -2712,6 +2713,7 @@ static void usb_put_invalidate_rhdev(struct usb_hcd *hcd)
 int usb_add_hcd(struct usb_hcd *hcd,
 		unsigned int irqnum, unsigned long irqflags)
 {
+	struct device *dev = hcd->self.controller;
 	int retval;
 	struct usb_device *rhdev;
 
@@ -2885,6 +2887,11 @@ int usb_add_hcd(struct usb_hcd *hcd,
 	if (hcd->uses_new_polling && HCD_POLL_RH(hcd))
 		usb_hcd_poll_rh_status(hcd);
 
+	if (usb_hcd_is_primary_hcd(hcd) && dev->of_node)
+		hcd->provider = of_hcd_provider_register(dev->of_node,
+							 of_hcd_xlate_simple,
+							 hcd);
+
 	return retval;
 
 error_create_attr_group:
@@ -2952,6 +2959,7 @@ void usb_remove_hcd(struct usb_hcd *hcd)
 	dev_info(hcd->self.controller, "remove, state %x\n", hcd->state);
 
 	usb_get_dev(rhdev);
+	of_hcd_provider_unregister(hcd->provider);
 	sysfs_remove_group(&rhdev->dev.kobj, &usb_bus_attr_group);
 
 	clear_bit(HCD_FLAG_RH_RUNNING, &hcd->flags);
diff --git a/drivers/usb/core/provider.c b/drivers/usb/core/provider.c
new file mode 100644
index 0000000..0de769a
--- /dev/null
+++ b/drivers/usb/core/provider.c
@@ -0,0 +1,99 @@
+#include <dt-bindings/usb/usb.h>
+#include <linux/slab.h>
+#include <linux/usb/provider.h>
+
+static DEFINE_MUTEX(hcd_provider_mutex);
+static LIST_HEAD(hcd_provider_list);
+
+struct hcd_provider {
+	struct device_node *np;
+	struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data);
+	void *data;
+	struct list_head list;
+};
+
+struct hcd_provider *of_hcd_provider_register(struct device_node *np,
+					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),
+					      void *data)
+{
+	struct hcd_provider *hcd_provider;
+
+	if (!np)
+		return ERR_PTR(-EINVAL);
+
+	hcd_provider = kzalloc(sizeof(*hcd_provider), GFP_KERNEL);
+	if (!hcd_provider)
+		return ERR_PTR(-ENOMEM);
+
+	hcd_provider->np = np;
+	hcd_provider->of_xlate = of_xlate;
+	hcd_provider->data = data;
+
+	mutex_lock(&hcd_provider_mutex);
+	list_add_tail(&hcd_provider->list, &hcd_provider_list);
+	mutex_unlock(&hcd_provider_mutex);
+
+	pr_debug("Registered provider for %s\n", np->full_name);
+
+	return hcd_provider;
+}
+
+void of_hcd_provider_unregister(struct hcd_provider *hcd_provider)
+{
+	if (IS_ERR(hcd_provider))
+		return;
+
+	mutex_lock(&hcd_provider_mutex);
+	list_del(&hcd_provider->list);
+	mutex_unlock(&hcd_provider_mutex);
+
+	kfree(hcd_provider);
+}
+EXPORT_SYMBOL_GPL(of_hcd_provider_unregister);
+
+struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args, void *data)
+{
+	struct usb_hcd *hcd = data;
+	int type = USB_HCD_PRIMARY;
+
+	if (args->args_count >= 2)
+		return ERR_PTR(-EINVAL);
+	else if (args->args_count >= 1)
+		type = args->args[0];
+
+	pr_debug("Looking for HCD type %d of %s\n", type, args->np->full_name);
+
+	switch (type) {
+	case USB_HCD_PRIMARY:
+		return hcd;
+	case USB_HCD_SHARED:
+		if (!hcd->shared_hcd)
+			return ERR_PTR(-ENOENT);
+		return hcd->shared_hcd;
+	default:
+		return ERR_PTR(-EINVAL);
+	}
+}
+
+struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args)
+{
+	struct usb_hcd *hcd = ERR_PTR(-ENOENT);
+	struct hcd_provider *provider;
+
+	if (!args)
+		return ERR_PTR(-EINVAL);
+
+	pr_debug("Looking for provider for %s\n", args->np->full_name);
+
+	mutex_lock(&hcd_provider_mutex);
+	list_for_each_entry(provider, &hcd_provider_list, list) {
+		if (provider->np == args->np) {
+			hcd = provider->of_xlate(args, provider->data);
+			break;
+		}
+	}
+	mutex_unlock(&hcd_provider_mutex);
+
+	return hcd;
+}
+EXPORT_SYMBOL_GPL(of_hcd_get_from_provider);
diff --git a/include/dt-bindings/usb/usb.h b/include/dt-bindings/usb/usb.h
new file mode 100644
index 0000000..adec026
--- /dev/null
+++ b/include/dt-bindings/usb/usb.h
@@ -0,0 +1,7 @@
+#ifndef _DT_BINDINGS_USB_USB_H
+#define _DT_BINDINGS_USB_USB_H
+
+#define USB_HCD_PRIMARY			0
+#define USB_HCD_SHARED			1
+
+#endif
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 66fc137..d064613 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -210,6 +210,8 @@ struct usb_hcd {
 	 * (ohci 32, uhci 1024, ehci 256/512/1024).
 	 */
 
+	struct hcd_provider	*provider;
+
 	/* The HC driver's private data is stored at the end of
 	 * this structure.
 	 */
diff --git a/include/linux/usb/provider.h b/include/linux/usb/provider.h
new file mode 100644
index 0000000..c66e006
--- /dev/null
+++ b/include/linux/usb/provider.h
@@ -0,0 +1,39 @@
+#ifndef __USB_CORE_PROVIDER_H
+#define __USB_CORE_PROVIDER_H
+
+#include <linux/of.h>
+#include <linux/usb.h>
+#include <linux/usb/hcd.h>
+
+struct hcd_provider;
+
+#ifdef CONFIG_OF
+struct hcd_provider *of_hcd_provider_register(struct device_node *np,
+					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),
+					      void *data);
+void of_hcd_provider_unregister(struct hcd_provider *hcd_provider);
+struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args, void *data);
+struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args);
+#else
+static inline
+struct hcd_provider *of_hcd_provider_register(struct device_node *np,
+					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),
+					      void *data)
+{
+	return ERR_PTR(-ENOSYS);
+}
+static inline void of_hcd_provider_unregister(struct hcd_provider *hcd_provider)
+{
+}
+static inline struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args,
+						  void *data)
+{
+	return ERR_PTR(-ENOSYS);
+}
+static inline struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args)
+{
+	return NULL;
+}
+#endif
+
+#endif /* __USB_CORE_PROVIDER_H */
-- 
1.8.4.5

[PATCH V2 PROOF OF CONCEPT 2/1] trigger: ledtrig-usbport: read initial state from DT

[Rafał Miłecki]

This allows specifying USB ports that should be observed by a trigger
right after activating it. Example:

usb {
	gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
	linux,default-trigger = "usbport";
	usb-controllers = <&ohci>, <&ehci>, <&xhci USB_HCD_SHARED>;
	usb-ports = "1", "1", "1";
};

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 drivers/leds/trigger/ledtrig-usbport.c | 72 ++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/drivers/leds/trigger/ledtrig-usbport.c b/drivers/leds/trigger/ledtrig-usbport.c
index eb20a8f..5724f63 100644
--- a/drivers/leds/trigger/ledtrig-usbport.c
+++ b/drivers/leds/trigger/ledtrig-usbport.c
@@ -12,8 +12,10 @@
 #include <linux/device.h>
 #include <linux/leds.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/slab.h>
 #include <linux/usb.h>
+#include <linux/usb/provider.h>
 #include "../leds.h"
 
 struct usbport_trig_port {
@@ -94,6 +96,75 @@ static bool usbport_trig_match(struct usbport_trig_data *usbport_data,
 	return false;
 }
 
+static int usbport_trig_new_port(struct usbport_trig_data *usbport_data,
+				 int busnum, const char *suffix)
+{
+	struct usbport_trig_port *port;
+	size_t len;
+	int tmp;
+
+	tmp = busnum;
+	len = 1;
+	while (tmp >= 10) {
+		tmp /= 10;
+		len++;
+	}
+	len++; /* '-' */
+	len += strlen(suffix);
+	len++; /* '\0' */
+
+	port = kzalloc(sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return -ENOMEM;
+
+	port->name = kzalloc(len, GFP_KERNEL);
+	if (!port->name) {
+		kfree(port);
+		return -ENOMEM;
+	}
+	snprintf(port->name, len, "%d-%s", busnum, suffix);
+
+	list_add_tail(&port->list, &usbport_data->ports);
+
+	return 0;
+}
+
+static int usbport_trig_fill(struct usbport_trig_data *usbport_data)
+{
+	struct device_node *np = usbport_data->led_cdev->dev->of_node;
+	struct of_phandle_args args;
+	int count, i;
+	int err = 0;
+
+	if (!np)
+		return -ENOENT;
+
+	count = of_property_count_strings(np, "usb-ports");
+	if (count < 0)
+		return count;
+
+	for (i = 0; i < count; i++) {
+		const char *port;
+		struct usb_hcd *hcd;
+
+		err = of_property_read_string_index(np, "usb-ports", i, &port);
+		if (err)
+			continue;
+
+		err = of_parse_phandle_with_args(np, "usb-controllers", "#usb-cells", i, &args);
+		if (err)
+			continue;
+
+		hcd = of_hcd_get_from_provider(&args);
+		if (!IS_ERR(hcd))
+			usbport_trig_new_port(usbport_data, hcd->self.busnum, port);
+
+		of_node_put(args.np);
+	}
+
+	return err;
+}
+
 static int usbport_trig_notify(struct notifier_block *nb, unsigned long action,
 			       void *data)
 {
@@ -129,6 +200,7 @@ static void usbport_trig_activate(struct led_classdev *led_cdev)
 		return;
 	usbport_data->led_cdev = led_cdev;
 	INIT_LIST_HEAD(&usbport_data->ports);
+	usbport_trig_fill(usbport_data);
 	usbport_data->nb.notifier_call = usbport_trig_notify,
 	led_cdev->trigger_data = usbport_data;
 
-- 
1.8.4.5

Re: [PATCH V2 PROOF OF CONCEPT 2/1] trigger: ledtrig-usbport: read initial state from DT

[Jacek Anaszewski]

Hi Rafał,

On 07/13/2016 02:42 PM, Rafał Miłecki wrote:
> This allows specifying USB ports that should be observed by a trigger
> right after activating it. Example:
>
> usb {
> 	gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
> 	linux,default-trigger = "usbport";
> 	usb-controllers = <&ohci>, <&ehci>, <&xhci USB_HCD_SHARED>;
> 	usb-ports = "1", "1", "1";

Port is a numerical value, right?
Wouldn't it be better to define it as an array of integers?

e.g.: usb-ports = <1>. <1>, <1>;

> };

Please provide a patch with DT bindings documentation for this
trigger. I infer that usb-controllers and usb-ports are new
properties.

Adding also Felipe, Mark and Rob.

>
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> ---
>   drivers/leds/trigger/ledtrig-usbport.c | 72 ++++++++++++++++++++++++++++++++++
>   1 file changed, 72 insertions(+)
>
> diff --git a/drivers/leds/trigger/ledtrig-usbport.c b/drivers/leds/trigger/ledtrig-usbport.c
> index eb20a8f..5724f63 100644
> --- a/drivers/leds/trigger/ledtrig-usbport.c
> +++ b/drivers/leds/trigger/ledtrig-usbport.c
> @@ -12,8 +12,10 @@
>   #include <linux/device.h>
>   #include <linux/leds.h>
>   #include <linux/module.h>
> +#include <linux/of.h>
>   #include <linux/slab.h>
>   #include <linux/usb.h>
> +#include <linux/usb/provider.h>
>   #include "../leds.h"
>
>   struct usbport_trig_port {
> @@ -94,6 +96,75 @@ static bool usbport_trig_match(struct usbport_trig_data *usbport_data,
>   	return false;
>   }
>
> +static int usbport_trig_new_port(struct usbport_trig_data *usbport_data,
> +				 int busnum, const char *suffix)
> +{
> +	struct usbport_trig_port *port;
> +	size_t len;
> +	int tmp;
> +
> +	tmp = busnum;
> +	len = 1;
> +	while (tmp >= 10) {
> +		tmp /= 10;
> +		len++;
> +	}
> +	len++; /* '-' */
> +	len += strlen(suffix);
> +	len++; /* '\0' */
> +
> +	port = kzalloc(sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return -ENOMEM;
> +
> +	port->name = kzalloc(len, GFP_KERNEL);
> +	if (!port->name) {
> +		kfree(port);
> +		return -ENOMEM;
> +	}
> +	snprintf(port->name, len, "%d-%s", busnum, suffix);
> +
> +	list_add_tail(&port->list, &usbport_data->ports);
> +
> +	return 0;
> +}
> +
> +static int usbport_trig_fill(struct usbport_trig_data *usbport_data)
> +{
> +	struct device_node *np = usbport_data->led_cdev->dev->of_node;
> +	struct of_phandle_args args;
> +	int count, i;
> +	int err = 0;
> +
> +	if (!np)
> +		return -ENOENT;
> +
> +	count = of_property_count_strings(np, "usb-ports");
> +	if (count < 0)
> +		return count;
> +
> +	for (i = 0; i < count; i++) {
> +		const char *port;
> +		struct usb_hcd *hcd;
> +
> +		err = of_property_read_string_index(np, "usb-ports", i, &port);
> +		if (err)
> +			continue;
> +
> +		err = of_parse_phandle_with_args(np, "usb-controllers", "#usb-cells", i, &args);
> +		if (err)
> +			continue;
> +
> +		hcd = of_hcd_get_from_provider(&args);
> +		if (!IS_ERR(hcd))
> +			usbport_trig_new_port(usbport_data, hcd->self.busnum, port);
> +
> +		of_node_put(args.np);
> +	}
> +
> +	return err;
> +}
> +
>   static int usbport_trig_notify(struct notifier_block *nb, unsigned long action,
>   			       void *data)
>   {
> @@ -129,6 +200,7 @@ static void usbport_trig_activate(struct led_classdev *led_cdev)
>   		return;
>   	usbport_data->led_cdev = led_cdev;
>   	INIT_LIST_HEAD(&usbport_data->ports);
> +	usbport_trig_fill(usbport_data);
>   	usbport_data->nb.notifier_call = usbport_trig_notify,
>   	led_cdev->trigger_data = usbport_data;
>
>


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH V2 PROOF OF CONCEPT 2/1] trigger: ledtrig-usbport: read initial state from DT

[Rafał Miłecki]

On 13 July 2016 at 16:48, Jacek Anaszewski <j.anaszewski@samsung.com> wrote:
> On 07/13/2016 02:42 PM, Rafał Miłecki wrote:
>>
>> This allows specifying USB ports that should be observed by a trigger
>> right after activating it. Example:
>>
>> usb {
>>         gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
>>         linux,default-trigger = "usbport";
>>         usb-controllers = <&ohci>, <&ehci>, <&xhci USB_HCD_SHARED>;
>>         usb-ports = "1", "1", "1";
>
>
> Port is a numerical value, right?
> Wouldn't it be better to define it as an array of integers?
>
> e.g.: usb-ports = <1>. <1>, <1>;

Not always. Let me quote this part of "usbport" documentation:

> This also allows handling devices with internal hubs (when root hub's port has
> always a device (hub) connected). User can simply specify specify internal hub
> ports then (e.g. 1-1.1, 1-1.2, etc.).

In such case we'd need usb-ports = "1.1", "1.2";

Anyway, there is a discussion ongoing in:
[PATCH V2 0/1] usb: add HCD providers
so let's see if someone will have a better idea for referencing USB
ports inside DT.

Re: [PATCH 1/2] usb: core: add support for HCD providers

[Greg Kroah-Hartman]

On Tue, Jul 12, 2016 at 02:35:19PM +0200, Rafał Miłecki wrote:
> When working with Device Tree we may need to reference controllers
> (their nodes) and query for HCDs. This is useful for getting some
> runtime info about host controllers like e.g. assigned bus number.
> 
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> ---
>  drivers/usb/core/Makefile    |  1 +
>  drivers/usb/core/provider.c  | 79 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/usb/provider.h | 39 ++++++++++++++++++++++
>  3 files changed, 119 insertions(+)
>  create mode 100644 drivers/usb/core/provider.c
>  create mode 100644 include/linux/usb/provider.h
> 
> diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
> index 9780877..20b91d1 100644
> --- a/drivers/usb/core/Makefile
> +++ b/drivers/usb/core/Makefile
> @@ -9,5 +9,6 @@ usbcore-y += port.o of.o
>  
>  usbcore-$(CONFIG_PCI)		+= hcd-pci.o
>  usbcore-$(CONFIG_ACPI)		+= usb-acpi.o
> +usbcore-$(CONFIG_OF)		+= provider.o
>  
>  obj-$(CONFIG_USB)		+= usbcore.o
> diff --git a/drivers/usb/core/provider.c b/drivers/usb/core/provider.c
> new file mode 100644
> index 0000000..4b9165a
> --- /dev/null
> +++ b/drivers/usb/core/provider.c
> @@ -0,0 +1,79 @@
> +#include <linux/slab.h>
> +#include <linux/usb/provider.h>
> +
> +static DEFINE_MUTEX(hcd_provider_mutex);
> +static LIST_HEAD(hcd_provider_list);
> +
> +struct hcd_provider {
> +	struct device_node *np;
> +	struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data);
> +	void *data;
> +	struct list_head list;
> +};
> +
> +struct hcd_provider *of_hcd_provider_register(struct device_node *np,
> +					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),

Typedef for the function pointer?


> +					      void *data)
> +{
> +	struct hcd_provider *hcd_provider;
> +
> +	if (!np)
> +		return ERR_PTR(-EINVAL);

How can that be true?

> +
> +	hcd_provider = kzalloc(sizeof(*hcd_provider), GFP_KERNEL);
> +	if (!hcd_provider)
> +		return ERR_PTR(-ENOMEM);
> +
> +	hcd_provider->np = np;
> +	hcd_provider->of_xlate = of_xlate;
> +	hcd_provider->data = data;
> +
> +	mutex_lock(&hcd_provider_mutex);
> +	list_add_tail(&hcd_provider->list, &hcd_provider_list);
> +	mutex_unlock(&hcd_provider_mutex);
> +
> +	return hcd_provider;
> +}
> +EXPORT_SYMBOL_GPL(of_hcd_provider_register);
> +
> +void of_hcd_provider_unregister(struct hcd_provider *hcd_provider)
> +{
> +	if (IS_ERR(hcd_provider))
> +		return;
> +
> +	mutex_lock(&hcd_provider_mutex);
> +	list_del(&hcd_provider->list);
> +	mutex_unlock(&hcd_provider_mutex);
> +
> +	kfree(hcd_provider);
> +}
> +EXPORT_SYMBOL_GPL(of_hcd_provider_unregister);
> +
> +struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args, void *data)
> +{
> +	if (args->args_count != 0)
> +		return ERR_PTR(-EINVAL);

Huh?

> +	return data;

What is this function for?  Why even have it?

> +}
> +EXPORT_SYMBOL_GPL(of_hcd_xlate_simple);
> +
> +struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args)
> +{
> +	struct usb_hcd *hcd = ERR_PTR(-ENOENT);
> +	struct hcd_provider *provider;
> +
> +	if (!args)
> +		return ERR_PTR(-EINVAL);

How is args not set?

> +
> +	mutex_lock(&hcd_provider_mutex);
> +	list_for_each_entry(provider, &hcd_provider_list, list) {
> +		if (provider->np == args->np) {
> +			hcd = provider->of_xlate(args, provider->data);
> +			break;
> +		}
> +	}
> +	mutex_unlock(&hcd_provider_mutex);
> +
> +	return hcd;
> +}
> +EXPORT_SYMBOL_GPL(of_hcd_get_from_provider);
> diff --git a/include/linux/usb/provider.h b/include/linux/usb/provider.h
> new file mode 100644
> index 0000000..c66e006
> --- /dev/null
> +++ b/include/linux/usb/provider.h
> @@ -0,0 +1,39 @@
> +#ifndef __USB_CORE_PROVIDER_H
> +#define __USB_CORE_PROVIDER_H
> +
> +#include <linux/of.h>
> +#include <linux/usb.h>
> +#include <linux/usb/hcd.h>
> +
> +struct hcd_provider;
> +
> +#ifdef CONFIG_OF
> +struct hcd_provider *of_hcd_provider_register(struct device_node *np,
> +					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),
> +					      void *data);
> +void of_hcd_provider_unregister(struct hcd_provider *hcd_provider);
> +struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args, void *data);
> +struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args);
> +#else
> +static inline
> +struct hcd_provider *of_hcd_provider_register(struct device_node *np,
> +					      struct usb_hcd *(*of_xlate)(struct of_phandle_args *args, void *data),
> +					      void *data)
> +{
> +	return ERR_PTR(-ENOSYS);
> +}
> +static inline void of_hcd_provider_unregister(struct hcd_provider *hcd_provider)
> +{
> +}
> +static inline struct usb_hcd *of_hcd_xlate_simple(struct of_phandle_args *args,
> +						  void *data)
> +{
> +	return ERR_PTR(-ENOSYS);
> +}
> +static inline struct usb_hcd *of_hcd_get_from_provider(struct of_phandle_args *args)
> +{
> +	return NULL;
> +}
> +#endif

Why all of the "of" stuff?  Why not make it generic for any firmware
type (acpi, OF, etc.)?

And I really don't like this, isn't there other ways to get this
information if you really need it?

thanks,

greg k-h