Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1440521 > unrolled thread

[PATCH] leds: trigger: Introduce an USB port trigger

Back to article view | Back to linux.kernel

Contents

  [PATCH] leds: trigger: Introduce an USB port trigger Rafał Miłecki <zajec5@gmail.com> - 2016-07-11 14:30 +0200
    Re: [PATCH] leds: trigger: Introduce an USB port trigger Jacek Anaszewski <j.anaszewski@samsung.com> - 2016-07-13 17:00 +0200
    [PATCH V2] leds: trigger: Introduce an USB port trigger Rafał Miłecki <zajec5@gmail.com> - 2016-07-15 23:20 +0200
      Re: [PATCH V2] leds: trigger: Introduce an USB port trigger Peter Chen <hzpeterchen@gmail.com> - 2016-07-18 04:40 +0200
        Re: [PATCH V2] leds: trigger: Introduce an USB port trigger Rafał Miłecki <zajec5@gmail.com> - 2016-07-18 06:50 +0200
          Re: [PATCH V2] leds: trigger: Introduce an USB port trigger Peter Chen <hzpeterchen@gmail.com> - 2016-07-18 07:50 +0200
            Re: [PATCH V2] leds: trigger: Introduce an USB port trigger Rafał Miłecki <zajec5@gmail.com> - 2016-07-18 08:00 +0200
              Re: [PATCH V2] leds: trigger: Introduce an USB port trigger Peter Chen <hzpeterchen@gmail.com> - 2016-07-18 08:10 +0200
                Re: [PATCH V2] leds: trigger: Introduce an USB port trigger Rafał Miłecki <zajec5@gmail.com> - 2016-07-18 09:00 +0200
                  Re: [PATCH V2] leds: trigger: Introduce an USB port trigger Pavel Machek <pavel@ucw.cz> - 2016-07-20 10:10 +0200
        Re: [PATCH V2] leds: trigger: Introduce an USB port trigger Pavel Machek <pavel@ucw.cz> - 2016-07-20 10:10 +0200
      Re: [PATCH V2] leds: trigger: Introduce an USB port trigger Rob Herring <robh@kernel.org> - 2016-07-20 03:10 +0200
        Re: [PATCH V2] leds: trigger: Introduce an USB port trigger Rafał Miłecki <zajec5@gmail.com> - 2016-07-20 10:10 +0200
          Re: [PATCH V2] leds: trigger: Introduce an USB port trigger Rob Herring <robh@kernel.org> - 2016-07-21 22:50 +0200

#1440521 — [PATCH] leds: trigger: Introduce an USB port trigger

This commit adds a new trigger that can turn on LED when USB device gets
connected to the USB port. This can can useful for various home routers
that have USB port and a proper LED telling user a device is connected.

The trigger gets its documentation file but basically it just requires
specifying USB port in a Linux format (e.g. echo 1-1 > new_port).

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
 Documentation/leds/ledtrig-usbport.txt |  47 ++++++++
 drivers/leds/trigger/Kconfig           |   8 ++
 drivers/leds/trigger/Makefile          |   1 +
 drivers/leds/trigger/ledtrig-usbport.c | 196 +++++++++++++++++++++++++++++++++
 4 files changed, 252 insertions(+)
 create mode 100644 Documentation/leds/ledtrig-usbport.txt
 create mode 100644 drivers/leds/trigger/ledtrig-usbport.c

diff --git a/Documentation/leds/ledtrig-usbport.txt b/Documentation/leds/ledtrig-usbport.txt
new file mode 100644
index 0000000..d31dba2
--- /dev/null
+++ b/Documentation/leds/ledtrig-usbport.txt
@@ -0,0 +1,47 @@
+USB port LED trigger
+====================
+
+This LED trigger can be used for signaling user a presence of USB device in a
+given port. It simply turns on LED when device appears and turns it off when it
+disappears.
+
+It requires specifying a list of USB ports that should be observed. Used format
+matches Linux kernel format and consists of a root hub number and a hub port
+separated by a dash (e.g. 3-1).
+
+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.).
+
+Please note that this trigger allows assigning multiple USB ports to a single
+LED. This can be useful in two cases:
+
+1) Device with single USB LED and few physical ports
+
+In such case LED will be turned on as long as there is at least one connected
+USB device.
+
+2) Device with a physical port handled by few controllers
+
+Some devices have e.g. one controller per PHY standard. E.g. USB 2.0 physical
+port may be handled by ohci-platform, ehci-platform and xhci-hcd. If there is
+only one LED user will most likely want to assign ports from all 3 hubs.
+
+
+This trigger can be activated from user space on led class devices as shown
+below:
+
+  echo usbport > trigger
+
+This adds the following sysfs attributes to the LED:
+
+  ports - allows listing all USB ports assigned to the trigger.
+
+  new_port - allows adding a new USB port by simply writing to it.
+
+Example use-case:
+
+  echo usbport > trigger
+  echo 4-1 > new_port
+  echo 2-1 > new_port
+  cat ports
diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index 3f9ddb9..bdd6fd2 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -126,4 +126,12 @@ config LEDS_TRIGGER_PANIC
 	  a different trigger.
 	  If unsure, say Y.
 
+config LEDS_TRIGGER_USBPORT
+	tristate "USB port LED trigger"
+	depends on LEDS_TRIGGERS && USB
+	help
+	  This allows LEDs to be controlled by USB events. Enabling this option
+	  allows specifying list of USB ports that should turn on LED when some
+	  USB device gets connected.
+
 endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index a72c43c..56e1741 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -10,3 +10,4 @@ obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON)	+= ledtrig-default-on.o
 obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)	+= ledtrig-transient.o
 obj-$(CONFIG_LEDS_TRIGGER_CAMERA)	+= ledtrig-camera.o
 obj-$(CONFIG_LEDS_TRIGGER_PANIC)	+= ledtrig-panic.o
+obj-$(CONFIG_LEDS_TRIGGER_USBPORT)	+= ledtrig-usbport.o
diff --git a/drivers/leds/trigger/ledtrig-usbport.c b/drivers/leds/trigger/ledtrig-usbport.c
new file mode 100644
index 0000000..eb20a8f
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-usbport.c
@@ -0,0 +1,196 @@
+/*
+ * USB port LED trigger
+ *
+ * Copyright (C) 2016 Rafał Miłecki <rafal.milecki@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or (at
+ * your option) any later version.
+ */
+
+#include <linux/device.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/usb.h>
+#include "../leds.h"
+
+struct usbport_trig_port {
+	char *name;
+	struct list_head list;
+};
+
+struct usbport_trig_data {
+	struct led_classdev *led_cdev;
+	struct list_head ports;
+	struct notifier_block nb;
+	int count; /* Amount of connected matching devices */
+};
+
+static ssize_t ports_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct usbport_trig_data *usbport_data = led_cdev->trigger_data;
+	struct usbport_trig_port *port;
+	ssize_t ret = 0;
+	int len;
+
+	list_for_each_entry(port, &usbport_data->ports, list) {
+		len = sprintf(buf + ret, "%s\n", port->name);
+		if (len >= 0)
+			ret += len;
+	}
+
+	return ret;
+}
+
+static DEVICE_ATTR(ports, S_IRUSR, ports_show, NULL);
+
+static ssize_t new_port_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t size)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct usbport_trig_data *usbport_data = led_cdev->trigger_data;
+	struct usbport_trig_port *port;
+	size_t len;
+
+	port = kzalloc(sizeof(*port), GFP_KERNEL);
+	if (!port)
+		return -ENOMEM;
+
+	len = strlen(buf);
+	if (len && buf[len - 1] == '\n')
+		len--;
+	if (!len)
+		return -EINVAL;
+
+	port->name = kzalloc(strlen(buf), GFP_KERNEL);
+	if (!port->name) {
+		kfree(port);
+		return -ENOMEM;
+	}
+	strncpy(port->name, buf, len);
+
+	list_add_tail(&port->list, &usbport_data->ports);
+
+	return size;
+}
+
+static DEVICE_ATTR(new_port, S_IWUSR, NULL, new_port_store);
+
+static bool usbport_trig_match(struct usbport_trig_data *usbport_data,
+			       struct usb_device *usb_dev)
+{
+	struct usbport_trig_port *port;
+	const char *name = dev_name(&usb_dev->dev);
+
+	list_for_each_entry(port, &usbport_data->ports, list) {
+		if (!strcmp(port->name, name))
+			return true;
+	}
+
+	return false;
+}
+
+static int usbport_trig_notify(struct notifier_block *nb, unsigned long action,
+			       void *data)
+{
+	struct usbport_trig_data *usbport_data =
+		container_of(nb, struct usbport_trig_data, nb);
+	struct led_classdev *led_cdev = usbport_data->led_cdev;
+
+	switch (action) {
+	case USB_DEVICE_ADD:
+		if (usbport_trig_match(usbport_data, data)) {
+			if (usbport_data->count++ == 0)
+				led_set_brightness_nosleep(led_cdev, LED_FULL);
+		}
+		break;
+	case USB_DEVICE_REMOVE:
+		if (usbport_trig_match(usbport_data, data)) {
+			if (--usbport_data->count == 0)
+				led_set_brightness_nosleep(led_cdev, LED_OFF);
+		}
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static void usbport_trig_activate(struct led_classdev *led_cdev)
+{
+	struct usbport_trig_data *usbport_data;
+	int err;
+
+	usbport_data = kzalloc(sizeof(*usbport_data), GFP_KERNEL);
+	if (!usbport_data)
+		return;
+	usbport_data->led_cdev = led_cdev;
+	INIT_LIST_HEAD(&usbport_data->ports);
+	usbport_data->nb.notifier_call = usbport_trig_notify,
+	led_cdev->trigger_data = usbport_data;
+
+	err = device_create_file(led_cdev->dev, &dev_attr_ports);
+	if (err)
+		goto err_free;
+	err = device_create_file(led_cdev->dev, &dev_attr_new_port);
+	if (err)
+		goto err_remove_ports;
+
+	usb_register_notify(&usbport_data->nb);
+
+	led_cdev->activated = true;
+	return;
+
+err_free:
+	kfree(usbport_data);
+err_remove_ports:
+	device_remove_file(led_cdev->dev, &dev_attr_ports);
+}
+
+static void usbport_trig_deactivate(struct led_classdev *led_cdev)
+{
+	struct usbport_trig_data *usbport_data = led_cdev->trigger_data;
+	struct usbport_trig_port *port, *tmp;
+
+	if (!led_cdev->activated)
+		return;
+
+	usb_unregister_notify(&usbport_data->nb);
+
+	list_for_each_entry_safe(port, tmp, &usbport_data->ports, list) {
+		kfree(port->name);
+		list_del(&port->list);
+	}
+
+	device_remove_file(led_cdev->dev, &dev_attr_new_port);
+	device_remove_file(led_cdev->dev, &dev_attr_ports);
+	kfree(usbport_data);
+
+	led_cdev->activated = false;
+}
+
+static struct led_trigger usbport_led_trigger = {
+	.name     = "usbport",
+	.activate = usbport_trig_activate,
+	.deactivate = usbport_trig_deactivate,
+};
+
+static int __init usbport_trig_init(void)
+{
+	return led_trigger_register(&usbport_led_trigger);
+}
+
+static void __exit usbport_trig_exit(void)
+{
+	led_trigger_unregister(&usbport_led_trigger);
+}
+
+module_init(usbport_trig_init);
+module_exit(usbport_trig_exit);
+
+MODULE_AUTHOR("Rafał Miłecki <rafal.milecki@gmail.com>");
+MODULE_DESCRIPTION("USB port trigger");
+MODULE_LICENSE("GPL");
-- 
1.8.4.5

[toc] | [next] | [standalone]

#1442529

Hi Rafał,

Thanks for the patch.

On 07/11/2016 02:24 PM, Rafał Miłecki wrote:
> This commit adds a new trigger that can turn on LED when USB device gets
> connected to the USB port. This can can useful for various home routers

s/can can/can be/

> that have USB port and a proper LED telling user a device is connected.
>
> The trigger gets its documentation file but basically it just requires
> specifying USB port in a Linux format (e.g. echo 1-1 > new_port).
>
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> ---
>   Documentation/leds/ledtrig-usbport.txt |  47 ++++++++
>   drivers/leds/trigger/Kconfig           |   8 ++
>   drivers/leds/trigger/Makefile          |   1 +
>   drivers/leds/trigger/ledtrig-usbport.c | 196 +++++++++++++++++++++++++++++++++
>   4 files changed, 252 insertions(+)
>   create mode 100644 Documentation/leds/ledtrig-usbport.txt
>   create mode 100644 drivers/leds/trigger/ledtrig-usbport.c
>
> diff --git a/Documentation/leds/ledtrig-usbport.txt b/Documentation/leds/ledtrig-usbport.txt
> new file mode 100644
> index 0000000..d31dba2
> --- /dev/null
> +++ b/Documentation/leds/ledtrig-usbport.txt
> @@ -0,0 +1,47 @@
> +USB port LED trigger
> +====================
> +
> +This LED trigger can be used for signaling user a presence of USB device in a

s/signaling/signalling
s/user/to the user/

> +given port. It simply turns on LED when device appears and turns it off when it
> +disappears.
> +
> +It requires specifying a list of USB ports that should be observed. Used format
> +matches Linux kernel format and consists of a root hub number and a hub port
> +separated by a dash (e.g. 3-1).
> +
> +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

Maybe it would be possible to rearrange this so that there was no need
for nesting expression in brackets?

> +ports then (e.g. 1-1.1, 1-1.2, etc.).
> +
> +Please note that this trigger allows assigning multiple USB ports to a single
> +LED. This can be useful in two cases:
> +
> +1) Device with single USB LED and few physical ports
> +
> +In such case LED will be turned on as long as there is at least one connected

s/such/such a/

> +USB device.
> +
> +2) Device with a physical port handled by few controllers
> +
> +Some devices have e.g. one controller per PHY standard. E.g. USB 2.0 physical
> +port may be handled by ohci-platform, ehci-platform and xhci-hcd. If there is
> +only one LED user will most likely want to assign ports from all 3 hubs.
> +
> +
> +This trigger can be activated from user space on led class devices as shown
> +below:
> +
> +  echo usbport > trigger
> +
> +This adds the following sysfs attributes to the LED:
> +
> +  ports - allows listing all USB ports assigned to the trigger.
> +
> +  new_port - allows adding a new USB port by simply writing to it.
> +
> +Example use-case:
> +
> +  echo usbport > trigger
> +  echo 4-1 > new_port
> +  echo 2-1 > new_port
> +  cat ports

I will need USB maintainer's ack for this patch.
Adding Felipe.

> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
> index 3f9ddb9..bdd6fd2 100644
> --- a/drivers/leds/trigger/Kconfig
> +++ b/drivers/leds/trigger/Kconfig
> @@ -126,4 +126,12 @@ config LEDS_TRIGGER_PANIC
>   	  a different trigger.
>   	  If unsure, say Y.
>
> +config LEDS_TRIGGER_USBPORT
> +	tristate "USB port LED trigger"
> +	depends on LEDS_TRIGGERS && USB
> +	help
> +	  This allows LEDs to be controlled by USB events. Enabling this option
> +	  allows specifying list of USB ports that should turn on LED when some
> +	  USB device gets connected.
> +
>   endif # LEDS_TRIGGERS
> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
> index a72c43c..56e1741 100644
> --- a/drivers/leds/trigger/Makefile
> +++ b/drivers/leds/trigger/Makefile
> @@ -10,3 +10,4 @@ obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON)	+= ledtrig-default-on.o
>   obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)	+= ledtrig-transient.o
>   obj-$(CONFIG_LEDS_TRIGGER_CAMERA)	+= ledtrig-camera.o
>   obj-$(CONFIG_LEDS_TRIGGER_PANIC)	+= ledtrig-panic.o
> +obj-$(CONFIG_LEDS_TRIGGER_USBPORT)	+= ledtrig-usbport.o
> diff --git a/drivers/leds/trigger/ledtrig-usbport.c b/drivers/leds/trigger/ledtrig-usbport.c
> new file mode 100644
> index 0000000..eb20a8f
> --- /dev/null
> +++ b/drivers/leds/trigger/ledtrig-usbport.c
> @@ -0,0 +1,196 @@
> +/*
> + * USB port LED trigger
> + *
> + * Copyright (C) 2016 Rafał Miłecki <rafal.milecki@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or (at
> + * your option) any later version.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/usb.h>
> +#include "../leds.h"
> +
> +struct usbport_trig_port {
> +	char *name;
> +	struct list_head list;
> +};
> +
> +struct usbport_trig_data {
> +	struct led_classdev *led_cdev;
> +	struct list_head ports;
> +	struct notifier_block nb;
> +	int count; /* Amount of connected matching devices */
> +};
> +
> +static ssize_t ports_show(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct usbport_trig_data *usbport_data = led_cdev->trigger_data;
> +	struct usbport_trig_port *port;
> +	ssize_t ret = 0;
> +	int len;
> +
> +	list_for_each_entry(port, &usbport_data->ports, list) {
> +		len = sprintf(buf + ret, "%s\n", port->name);
> +		if (len >= 0)
> +			ret += len;
> +	}
> +
> +	return ret;
> +}
> +
> +static DEVICE_ATTR(ports, S_IRUSR, ports_show, NULL);
> +
> +static ssize_t new_port_store(struct device *dev, struct device_attribute *attr,
> +			      const char *buf, size_t size)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct usbport_trig_data *usbport_data = led_cdev->trigger_data;
> +	struct usbport_trig_port *port;
> +	size_t len;
> +
> +	port = kzalloc(sizeof(*port), GFP_KERNEL);
> +	if (!port)
> +		return -ENOMEM;
> +
> +	len = strlen(buf);
> +	if (len && buf[len - 1] == '\n')
> +		len--;
> +	if (!len)
> +		return -EINVAL;
> +
> +	port->name = kzalloc(strlen(buf), GFP_KERNEL);
> +	if (!port->name) {
> +		kfree(port);
> +		return -ENOMEM;
> +	}
> +	strncpy(port->name, buf, len);
> +
> +	list_add_tail(&port->list, &usbport_data->ports);
> +
> +	return size;
> +}
> +
> +static DEVICE_ATTR(new_port, S_IWUSR, NULL, new_port_store);
> +
> +static bool usbport_trig_match(struct usbport_trig_data *usbport_data,
> +			       struct usb_device *usb_dev)
> +{
> +	struct usbport_trig_port *port;
> +	const char *name = dev_name(&usb_dev->dev);
> +
> +	list_for_each_entry(port, &usbport_data->ports, list) {
> +		if (!strcmp(port->name, name))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static int usbport_trig_notify(struct notifier_block *nb, unsigned long action,
> +			       void *data)
> +{
> +	struct usbport_trig_data *usbport_data =
> +		container_of(nb, struct usbport_trig_data, nb);
> +	struct led_classdev *led_cdev = usbport_data->led_cdev;
> +
> +	switch (action) {
> +	case USB_DEVICE_ADD:
> +		if (usbport_trig_match(usbport_data, data)) {
> +			if (usbport_data->count++ == 0)
> +				led_set_brightness_nosleep(led_cdev, LED_FULL);
> +		}
> +		break;
> +	case USB_DEVICE_REMOVE:
> +		if (usbport_trig_match(usbport_data, data)) {
> +			if (--usbport_data->count == 0)
> +				led_set_brightness_nosleep(led_cdev, LED_OFF);
> +		}
> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +static void usbport_trig_activate(struct led_classdev *led_cdev)
> +{
> +	struct usbport_trig_data *usbport_data;
> +	int err;
> +
> +	usbport_data = kzalloc(sizeof(*usbport_data), GFP_KERNEL);
> +	if (!usbport_data)
> +		return;
> +	usbport_data->led_cdev = led_cdev;
> +	INIT_LIST_HEAD(&usbport_data->ports);
> +	usbport_data->nb.notifier_call = usbport_trig_notify,
> +	led_cdev->trigger_data = usbport_data;
> +
> +	err = device_create_file(led_cdev->dev, &dev_attr_ports);
> +	if (err)
> +		goto err_free;
> +	err = device_create_file(led_cdev->dev, &dev_attr_new_port);
> +	if (err)
> +		goto err_remove_ports;

Couldn't we live without new_port attr? Writing the ports attribute
would add the port. We should have a means for removing the port too,
possibly prepended with some character, like '-'.

> +
> +	usb_register_notify(&usbport_data->nb);
> +
> +	led_cdev->activated = true;
> +	return;
> +
> +err_free:
> +	kfree(usbport_data);
> +err_remove_ports:
> +	device_remove_file(led_cdev->dev, &dev_attr_ports);
> +}
> +
> +static void usbport_trig_deactivate(struct led_classdev *led_cdev)
> +{
> +	struct usbport_trig_data *usbport_data = led_cdev->trigger_data;
> +	struct usbport_trig_port *port, *tmp;
> +
> +	if (!led_cdev->activated)
> +		return;
> +
> +	usb_unregister_notify(&usbport_data->nb);
> +
> +	list_for_each_entry_safe(port, tmp, &usbport_data->ports, list) {
> +		kfree(port->name);
> +		list_del(&port->list);
> +	}
> +
> +	device_remove_file(led_cdev->dev, &dev_attr_new_port);
> +	device_remove_file(led_cdev->dev, &dev_attr_ports);
> +	kfree(usbport_data);
> +
> +	led_cdev->activated = false;
> +}
> +
> +static struct led_trigger usbport_led_trigger = {
> +	.name     = "usbport",
> +	.activate = usbport_trig_activate,
> +	.deactivate = usbport_trig_deactivate,
> +};
> +
> +static int __init usbport_trig_init(void)
> +{
> +	return led_trigger_register(&usbport_led_trigger);
> +}
> +
> +static void __exit usbport_trig_exit(void)
> +{
> +	led_trigger_unregister(&usbport_led_trigger);
> +}
> +
> +module_init(usbport_trig_init);
> +module_exit(usbport_trig_exit);
> +
> +MODULE_AUTHOR("Rafał Miłecki <rafal.milecki@gmail.com>");
> +MODULE_DESCRIPTION("USB port trigger");
> +MODULE_LICENSE("GPL");
>


-- 
Best regards,
Jacek Anaszewski

[toc] | [prev] | [next] | [standalone]

#1444596 — [PATCH V2] leds: trigger: Introduce an USB port trigger

This commit adds a new trigger that can turn on LED when USB device gets
connected to the USB port. This can be useful for various home routers
that have USB port and a proper LED telling user a device is connected.

Right now this trigger is usable with a proper DT only, there isn't a
way to specify USB ports from user space. This may change in a future.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
V2: The first version got support for specifying list of USB ports from
    user space only. There was a (big try &) discussion on adding DT
    support. It led to a pretty simple solution of comparing of_node of
    usb_device to of_node specified in usb-ports property.
    Since it appeared DT support may be simpler and non-DT a bit more
    complex, this version drops previous support for "ports" and
    "new_port" and focuses on DT only. The plan is to see if this
    solution with DT is OK, get it accepted and then work on non-DT.

Felipe: if there won't be any objections I'd like to ask for your Ack.
---
 Documentation/devicetree/bindings/leds/common.txt |  11 ++
 Documentation/leds/ledtrig-usbport.txt            |  19 ++
 drivers/leds/trigger/Kconfig                      |   8 +
 drivers/leds/trigger/Makefile                     |   1 +
 drivers/leds/trigger/ledtrig-usbport.c            | 206 ++++++++++++++++++++++
 5 files changed, 245 insertions(+)
 create mode 100644 Documentation/leds/ledtrig-usbport.txt
 create mode 100644 drivers/leds/trigger/ledtrig-usbport.c

diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
index af10678..75536f7 100644
--- a/Documentation/devicetree/bindings/leds/common.txt
+++ b/Documentation/devicetree/bindings/leds/common.txt
@@ -50,6 +50,12 @@ property can be omitted.
 For controllers that have no configurable timeout the flash-max-timeout-us
 property can be omitted.
 
+Trigger specific properties for child nodes:
+
+usbport trigger:
+- usb-ports : List of USB ports that usbport should observed for turning on a
+	      given LED.
+
 Examples:
 
 system-status {
@@ -58,6 +64,11 @@ system-status {
 	...
 };
 
+usb {
+	label = "USB";
+	usb-ports = <&ohci_port1>, <&ehci_port1>;
+};
+
 camera-flash {
 	label = "Flash";
 	led-sources = <0>, <1>;
diff --git a/Documentation/leds/ledtrig-usbport.txt b/Documentation/leds/ledtrig-usbport.txt
new file mode 100644
index 0000000..642c4cd
--- /dev/null
+++ b/Documentation/leds/ledtrig-usbport.txt
@@ -0,0 +1,19 @@
+USB port LED trigger
+====================
+
+This LED trigger can be used for signaling user a presence of USB device in a
+given port. It simply turns on LED when device appears and turns it off when it
+disappears.
+
+It requires specifying a list of USB ports that should be observed. This can be
+done in DT by setting a proper property with list of a phandles. If more than
+one port is specified, LED will be turned on as along as there is at least one
+device connected to any of ports.
+
+This trigger can be activated from user space on led class devices as shown
+below:
+
+  echo usbport > trigger
+
+Nevertheless, current there isn't a way to specify list of USB ports from user
+space.
diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index 9893d91..5b8e7c7 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -126,4 +126,12 @@ config LEDS_TRIGGER_PANIC
 	  a different trigger.
 	  If unsure, say Y.
 
+config LEDS_TRIGGER_USBPORT
+	tristate "USB port LED trigger"
+	depends on LEDS_TRIGGERS && USB && OF
+	help
+	  This allows LEDs to be controlled by USB events. This trigger will
+	  enable LED if some USB device gets connected to any of ports specified
+	  in DT.
+
 endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index 8cc64a4..80e2494 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -10,3 +10,4 @@ obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON)	+= ledtrig-default-on.o
 obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)	+= ledtrig-transient.o
 obj-$(CONFIG_LEDS_TRIGGER_CAMERA)	+= ledtrig-camera.o
 obj-$(CONFIG_LEDS_TRIGGER_PANIC)	+= ledtrig-panic.o
+obj-$(CONFIG_LEDS_TRIGGER_USBPORT)	+= ledtrig-usbport.o
diff --git a/drivers/leds/trigger/ledtrig-usbport.c b/drivers/leds/trigger/ledtrig-usbport.c
new file mode 100644
index 0000000..97b064c
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-usbport.c
@@ -0,0 +1,206 @@
+/*
+ * USB port LED trigger
+ *
+ * Copyright (C) 2016 Rafał Miłecki <rafal.milecki@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or (at
+ * your option) any later version.
+ */
+
+#include <linux/device.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/slab.h>
+#include <linux/usb.h>
+#include "../leds.h"
+
+struct usbport_trig_port {
+	struct device_node *np;
+	struct list_head list;
+};
+
+struct usbport_trig_data {
+	struct led_classdev *led_cdev;
+	struct list_head ports;
+	struct notifier_block nb;
+	int count; /* Amount of connected matching devices */
+};
+
+static int usbport_trig_check_usb_dev(struct usb_device *usb_dev, void *data)
+{
+	struct usbport_trig_data *usbport_data = data;
+	struct device_node *np = usb_dev->dev.of_node;
+	struct usbport_trig_port *port;
+
+	list_for_each_entry(port, &usbport_data->ports, list) {
+		if (port->np == np) {
+			usbport_data->count++;
+			break;
+		}
+	}
+
+	return 0;
+}
+
+static int usbport_trig_get_ports(struct usbport_trig_data *usbport_data)
+{
+	struct led_classdev *led_cdev = usbport_data->led_cdev;
+	struct device *dev = led_cdev->dev;
+	struct device_node *np = dev->of_node;
+	struct of_phandle_args args;
+	struct usbport_trig_port *port;
+	int count, i;
+
+	if (!np)
+		return -ENOENT;
+
+	count = of_count_phandle_with_args(np, "usb-ports", NULL);
+	if (count < 0) {
+		dev_warn(dev, "Failed to get USB ports for %s\n",
+			 np->full_name);
+		return count;
+	}
+
+	if (!count) {
+		dev_warn(dev, "There aren't USB ports assigned to the %s\n",
+			 np->full_name);
+		return -ENOENT;
+	}
+
+	for (i = 0; i < count; i++) {
+		int err;
+
+		err = of_parse_phandle_with_args(np, "usb-ports", NULL, i,
+						 &args);
+		if (err) {
+			dev_err(dev, "Failed to get USB port phandle at index %d: %d\n",
+				i, err);
+			continue;
+		}
+
+		port = kzalloc(sizeof(*port), GFP_KERNEL);
+		if (port) {
+			port->np = args.np;
+			list_add_tail(&port->list, &usbport_data->ports);
+		}
+
+		of_node_put(args.np);
+	}
+
+	usb_for_each_dev(usbport_data, usbport_trig_check_usb_dev);
+	if (usbport_data->count)
+		led_set_brightness_nosleep(led_cdev, LED_FULL);
+
+	list_for_each_entry(port, &usbport_data->ports, list) {
+		dev_dbg(dev, "Added USB port %s\n", port->np->full_name);
+	}
+
+	return 0;
+}
+
+static bool usbport_trig_match(struct usbport_trig_data *usbport_data,
+			       struct usb_device *usb_dev)
+{
+	struct device_node *np = usb_dev->dev.of_node;
+	struct usbport_trig_port *port;
+
+	if (!np)
+		return false;
+
+	list_for_each_entry(port, &usbport_data->ports, list) {
+		if (port->np == np)
+			return true;
+	}
+
+	return false;
+}
+
+static int usbport_trig_notify(struct notifier_block *nb, unsigned long action,
+			       void *data)
+{
+	struct usbport_trig_data *usbport_data = container_of(nb, struct usbport_trig_data, nb);
+	struct led_classdev *led_cdev = usbport_data->led_cdev;
+
+	switch (action) {
+	case USB_DEVICE_ADD:
+		if (usbport_trig_match(usbport_data, data)) {
+			if (usbport_data->count++ == 0)
+				led_set_brightness_nosleep(led_cdev, LED_FULL);
+		}
+		break;
+	case USB_DEVICE_REMOVE:
+		if (usbport_trig_match(usbport_data, data)) {
+			if (--usbport_data->count == 0)
+				led_set_brightness_nosleep(led_cdev, LED_OFF);
+		}
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static void usbport_trig_activate(struct led_classdev *led_cdev)
+{
+	struct usbport_trig_data *usbport_data;
+
+	usbport_data = kzalloc(sizeof(*usbport_data), GFP_KERNEL);
+	if (!usbport_data)
+		return;
+	led_cdev->trigger_data = usbport_data;
+
+	usbport_data->led_cdev = led_cdev;
+
+	INIT_LIST_HEAD(&usbport_data->ports);
+	usbport_trig_get_ports(usbport_data);
+
+	usbport_data->nb.notifier_call = usbport_trig_notify,
+	usb_register_notify(&usbport_data->nb);
+
+	led_cdev->activated = true;
+}
+
+static void usbport_trig_deactivate(struct led_classdev *led_cdev)
+{
+	struct usbport_trig_data *usbport_data = led_cdev->trigger_data;
+	struct usbport_trig_port *port, *tmp;
+
+	if (!led_cdev->activated)
+		return;
+
+	usb_unregister_notify(&usbport_data->nb);
+
+	list_for_each_entry_safe(port, tmp, &usbport_data->ports, list) {
+		list_del(&port->list);
+		kfree(port);
+	}
+
+	kfree(usbport_data);
+
+	led_cdev->activated = false;
+}
+
+static struct led_trigger usbport_led_trigger = {
+	.name     = "usbport",
+	.activate = usbport_trig_activate,
+	.deactivate = usbport_trig_deactivate,
+};
+
+static int __init usbport_trig_init(void)
+{
+	return led_trigger_register(&usbport_led_trigger);
+}
+
+static void __exit usbport_trig_exit(void)
+{
+	led_trigger_unregister(&usbport_led_trigger);
+}
+
+module_init(usbport_trig_init);
+module_exit(usbport_trig_exit);
+
+MODULE_AUTHOR("Rafał Miłecki <rafal.milecki@gmail.com>");
+MODULE_DESCRIPTION("USB port trigger");
+MODULE_LICENSE("GPL");
-- 
1.8.4.5

[toc] | [prev] | [next] | [standalone]

#1445169 — Re: [PATCH V2] leds: trigger: Introduce an USB port trigger

On Fri, Jul 15, 2016 at 11:10:45PM +0200, Rafał Miłecki wrote:
> +
> +usbport trigger:
> +- usb-ports : List of USB ports that usbport should observed for turning on a
> +	      given LED.
> +

%s/should/should be

>  Examples:
>  
>  system-status {
> @@ -58,6 +64,11 @@ system-status {
>  	...
>  };
>  
> +usb {
> +	label = "USB";
> +	usb-ports = <&ohci_port1>, <&ehci_port1>;
> +};
> +
>  camera-flash {
>  	label = "Flash";
>  	led-sources = <0>, <1>;
> diff --git a/Documentation/leds/ledtrig-usbport.txt b/Documentation/leds/ledtrig-usbport.txt
> new file mode 100644
> index 0000000..642c4cd
> --- /dev/null
> +++ b/Documentation/leds/ledtrig-usbport.txt
> @@ -0,0 +1,19 @@
> +USB port LED trigger
> +====================
> +
> +This LED trigger can be used for signaling user a presence of USB device in a
> +given port. It simply turns on LED when device appears and turns it off when it
> +disappears.
> +
> +It requires specifying a list of USB ports that should be observed. This can be
> +done in DT by setting a proper property with list of a phandles. If more than
> +one port is specified, LED will be turned on as along as there is at least one
> +device connected to any of ports.
> +
> +This trigger can be activated from user space on led class devices as shown
> +below:
> +
> +  echo usbport > trigger
> +
> +Nevertheless, current there isn't a way to specify list of USB ports from user
> +space.
> diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
> index 9893d91..5b8e7c7 100644
> --- a/drivers/leds/trigger/Kconfig
> +++ b/drivers/leds/trigger/Kconfig
> @@ -126,4 +126,12 @@ config LEDS_TRIGGER_PANIC
>  	  a different trigger.
>  	  If unsure, say Y.
>  
> +config LEDS_TRIGGER_USBPORT
> +	tristate "USB port LED trigger"
> +	depends on LEDS_TRIGGERS && USB && OF
> +	help
> +	  This allows LEDs to be controlled by USB events. This trigger will
> +	  enable LED if some USB device gets connected to any of ports specified
> +	  in DT.
> +
>  endif # LEDS_TRIGGERS
> diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
> index 8cc64a4..80e2494 100644
> --- a/drivers/leds/trigger/Makefile
> +++ b/drivers/leds/trigger/Makefile
> @@ -10,3 +10,4 @@ obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON)	+= ledtrig-default-on.o
>  obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT)	+= ledtrig-transient.o
>  obj-$(CONFIG_LEDS_TRIGGER_CAMERA)	+= ledtrig-camera.o
>  obj-$(CONFIG_LEDS_TRIGGER_PANIC)	+= ledtrig-panic.o
> +obj-$(CONFIG_LEDS_TRIGGER_USBPORT)	+= ledtrig-usbport.o
> diff --git a/drivers/leds/trigger/ledtrig-usbport.c b/drivers/leds/trigger/ledtrig-usbport.c
> new file mode 100644
> index 0000000..97b064c
> --- /dev/null
> +++ b/drivers/leds/trigger/ledtrig-usbport.c
> @@ -0,0 +1,206 @@
> +/*
> + * USB port LED trigger
> + *
> + * Copyright (C) 2016 Rafał Miłecki <rafal.milecki@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or (at
> + * your option) any later version.
> + */

GPL v2 only.

> +MODULE_AUTHOR("Rafał Miłecki <rafal.milecki@gmail.com>");
> +MODULE_DESCRIPTION("USB port trigger");
> +MODULE_LICENSE("GPL");

GPL v2

After you fix above, feel free to add:
Reviewed-by: Peter Chen <peter.chen@nxp.com>

-- 

Best Regards,
Peter Chen

[toc] | [prev] | [next] | [standalone]

#1445203 — Re: [PATCH V2] leds: trigger: Introduce an USB port trigger

On 18 July 2016 at 04:31, Peter Chen <hzpeterchen@gmail.com> wrote:
> On Fri, Jul 15, 2016 at 11:10:45PM +0200, Rafał Miłecki wrote:
>> +
>> +usbport trigger:
>> +- usb-ports : List of USB ports that usbport should observed for turning on a
>> +           given LED.
>> +
>
> %s/should/should be

Thanks.


>> diff --git a/drivers/leds/trigger/ledtrig-usbport.c b/drivers/leds/trigger/ledtrig-usbport.c
>> new file mode 100644
>> index 0000000..97b064c
>> --- /dev/null
>> +++ b/drivers/leds/trigger/ledtrig-usbport.c
>> @@ -0,0 +1,206 @@
>> +/*
>> + * USB port LED trigger
>> + *
>> + * Copyright (C) 2016 Rafał Miłecki <rafal.milecki@gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or (at
>> + * your option) any later version.
>> + */
>
> GPL v2 only.
>
>> +MODULE_AUTHOR("Rafał Miłecki <rafal.milecki@gmail.com>");
>> +MODULE_DESCRIPTION("USB port trigger");
>> +MODULE_LICENSE("GPL");
>
> GPL v2

What's the reason for this? I don't have any real preference, but I
never heard heard about kernel/Linux preference neither.

-- 
Rafał

[toc] | [prev] | [next] | [standalone]

#1445219 — Re: [PATCH V2] leds: trigger: Introduce an USB port trigger

On Mon, Jul 18, 2016 at 06:44:49AM +0200, Rafał Miłecki wrote:
> On 18 July 2016 at 04:31, Peter Chen <hzpeterchen@gmail.com> wrote:
> > On Fri, Jul 15, 2016 at 11:10:45PM +0200, Rafał Miłecki wrote:
> >> +
> >> +usbport trigger:
> >> +- usb-ports : List of USB ports that usbport should observed for turning on a
> >> +           given LED.
> >> +
> >
> > %s/should/should be
> 
> Thanks.
> 
> 
> >> diff --git a/drivers/leds/trigger/ledtrig-usbport.c b/drivers/leds/trigger/ledtrig-usbport.c
> >> new file mode 100644
> >> index 0000000..97b064c
> >> --- /dev/null
> >> +++ b/drivers/leds/trigger/ledtrig-usbport.c
> >> @@ -0,0 +1,206 @@
> >> +/*
> >> + * USB port LED trigger
> >> + *
> >> + * Copyright (C) 2016 Rafał Miłecki <rafal.milecki@gmail.com>
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License as published by
> >> + * the Free Software Foundation; either version 2 of the License, or (at
> >> + * your option) any later version.
> >> + */
> >
> > GPL v2 only.
> >
> >> +MODULE_AUTHOR("Rafał Miłecki <rafal.milecki@gmail.com>");
> >> +MODULE_DESCRIPTION("USB port trigger");
> >> +MODULE_LICENSE("GPL");
> >
> > GPL v2
> 
> What's the reason for this? I don't have any real preference, but I
> never heard heard about kernel/Linux preference neither.
> 

https://en.wikipedia.org/wiki/Linux_kernel

-- 

Best Regards,
Peter Chen

[toc] | [prev] | [next] | [standalone]

#1445228 — Re: [PATCH V2] leds: trigger: Introduce an USB port trigger

On 18 July 2016 at 07:40, Peter Chen <hzpeterchen@gmail.com> wrote:
> On Mon, Jul 18, 2016 at 06:44:49AM +0200, Rafał Miłecki wrote:
>> On 18 July 2016 at 04:31, Peter Chen <hzpeterchen@gmail.com> wrote:
>> > On Fri, Jul 15, 2016 at 11:10:45PM +0200, Rafał Miłecki wrote:
>> >> +
>> >> +usbport trigger:
>> >> +- usb-ports : List of USB ports that usbport should observed for turning on a
>> >> +           given LED.
>> >> +
>> >
>> > %s/should/should be
>>
>> Thanks.
>>
>>
>> >> diff --git a/drivers/leds/trigger/ledtrig-usbport.c b/drivers/leds/trigger/ledtrig-usbport.c
>> >> new file mode 100644
>> >> index 0000000..97b064c
>> >> --- /dev/null
>> >> +++ b/drivers/leds/trigger/ledtrig-usbport.c
>> >> @@ -0,0 +1,206 @@
>> >> +/*
>> >> + * USB port LED trigger
>> >> + *
>> >> + * Copyright (C) 2016 Rafał Miłecki <rafal.milecki@gmail.com>
>> >> + *
>> >> + * This program is free software; you can redistribute it and/or modify
>> >> + * it under the terms of the GNU General Public License as published by
>> >> + * the Free Software Foundation; either version 2 of the License, or (at
>> >> + * your option) any later version.
>> >> + */
>> >
>> > GPL v2 only.
>> >
>> >> +MODULE_AUTHOR("Rafał Miłecki <rafal.milecki@gmail.com>");
>> >> +MODULE_DESCRIPTION("USB port trigger");
>> >> +MODULE_LICENSE("GPL");
>> >
>> > GPL v2
>>
>> What's the reason for this? I don't have any real preference, but I
>> never heard heard about kernel/Linux preference neither.
>>
>
> https://en.wikipedia.org/wiki/Linux_kernel

Well, Linux is released under GPL v2, I'm well aware of that. It means
all its code needs to be GPL v2 compatible. There are multiple
compatible licenses: MIT, BSD 3-clause, BSD 2-clause. The one I used
allows treating code as GPL V2 as well. I could release this code
using MIT and it should be acceptable as well.

I still don't see what's wrong with the picked license.

-- 
Rafał

[toc] | [prev] | [next] | [standalone]

#1445231 — Re: [PATCH V2] leds: trigger: Introduce an USB port trigger

On Mon, Jul 18, 2016 at 07:57:34AM +0200, Rafał Miłecki wrote:
> On 18 July 2016 at 07:40, Peter Chen <hzpeterchen@gmail.com> wrote:
> > On Mon, Jul 18, 2016 at 06:44:49AM +0200, Rafał Miłecki wrote:
> >> On 18 July 2016 at 04:31, Peter Chen <hzpeterchen@gmail.com> wrote:
> >> > On Fri, Jul 15, 2016 at 11:10:45PM +0200, Rafał Miłecki wrote:
> >> >> +
> >> >> +usbport trigger:
> >> >> +- usb-ports : List of USB ports that usbport should observed for turning on a
> >> >> +           given LED.
> >> >> +
> >> >
> >> > %s/should/should be
> >>
> >> Thanks.
> >>
> >>
> >> >> diff --git a/drivers/leds/trigger/ledtrig-usbport.c b/drivers/leds/trigger/ledtrig-usbport.c
> >> >> new file mode 100644
> >> >> index 0000000..97b064c
> >> >> --- /dev/null
> >> >> +++ b/drivers/leds/trigger/ledtrig-usbport.c
> >> >> @@ -0,0 +1,206 @@
> >> >> +/*
> >> >> + * USB port LED trigger
> >> >> + *
> >> >> + * Copyright (C) 2016 Rafał Miłecki <rafal.milecki@gmail.com>
> >> >> + *
> >> >> + * This program is free software; you can redistribute it and/or modify
> >> >> + * it under the terms of the GNU General Public License as published by
> >> >> + * the Free Software Foundation; either version 2 of the License, or (at
> >> >> + * your option) any later version.
> >> >> + */
> >> >

In your COPYRIGHT, it says "or any later version". But afaik, It should
not be on GPL v3.

Peter
> >> > GPL v2 only.
> >> >
> >> >> +MODULE_AUTHOR("Rafał Miłecki <rafal.milecki@gmail.com>");
> >> >> +MODULE_DESCRIPTION("USB port trigger");
> >> >> +MODULE_LICENSE("GPL");
> >> >
> >> > GPL v2
> >>
> >> What's the reason for this? I don't have any real preference, but I
> >> never heard heard about kernel/Linux preference neither.
> >>
> >
> > https://en.wikipedia.org/wiki/Linux_kernel
> 
> Well, Linux is released under GPL v2, I'm well aware of that. It means
> all its code needs to be GPL v2 compatible. There are multiple
> compatible licenses: MIT, BSD 3-clause, BSD 2-clause. The one I used
> allows treating code as GPL V2 as well. I could release this code
> using MIT and it should be acceptable as well.
> 
> I still don't see what's wrong with the picked license.
> 
> -- 
> Rafał

-- 

Best Regards,
Peter Chen

[toc] | [prev] | [next] | [standalone]

#1445249 — Re: [PATCH V2] leds: trigger: Introduce an USB port trigger

On 18 July 2016 at 07:53, Peter Chen <hzpeterchen@gmail.com> wrote:
> On Mon, Jul 18, 2016 at 07:57:34AM +0200, Rafał Miłecki wrote:
>> On 18 July 2016 at 07:40, Peter Chen <hzpeterchen@gmail.com> wrote:
>> > On Mon, Jul 18, 2016 at 06:44:49AM +0200, Rafał Miłecki wrote:
>> >> On 18 July 2016 at 04:31, Peter Chen <hzpeterchen@gmail.com> wrote:
>> >> > On Fri, Jul 15, 2016 at 11:10:45PM +0200, Rafał Miłecki wrote:
>> >> >> +
>> >> >> +usbport trigger:
>> >> >> +- usb-ports : List of USB ports that usbport should observed for turning on a
>> >> >> +           given LED.
>> >> >> +
>> >> >
>> >> > %s/should/should be
>> >>
>> >> Thanks.
>> >>
>> >>
>> >> >> diff --git a/drivers/leds/trigger/ledtrig-usbport.c b/drivers/leds/trigger/ledtrig-usbport.c
>> >> >> new file mode 100644
>> >> >> index 0000000..97b064c
>> >> >> --- /dev/null
>> >> >> +++ b/drivers/leds/trigger/ledtrig-usbport.c
>> >> >> @@ -0,0 +1,206 @@
>> >> >> +/*
>> >> >> + * USB port LED trigger
>> >> >> + *
>> >> >> + * Copyright (C) 2016 Rafał Miłecki <rafal.milecki@gmail.com>
>> >> >> + *
>> >> >> + * This program is free software; you can redistribute it and/or modify
>> >> >> + * it under the terms of the GNU General Public License as published by
>> >> >> + * the Free Software Foundation; either version 2 of the License, or (at
>> >> >> + * your option) any later version.
>> >> >> + */
>> >> >
>
> In your COPYRIGHT, it says "or any later version". But afaik, It should
> not be on GPL v3.

If one picks my code, modifies it, relicenses it using GPL v3, we
can't include it anymore. It's the same story as with BSD systems and
their BSD licenses. If one picks e.g. BSD 3-clause licensed code,
makes it GPL, releases it (e.g. by putting in Linux), BSD can't use it
anymore.

Still, this license is GPL v2 compatible and as is it can be included
in the Linux.

-- 
Rafał

[toc] | [prev] | [next] | [standalone]

#1447062 — Re: [PATCH V2] leds: trigger: Introduce an USB port trigger

On Mon 2016-07-18 08:55:52, Rafał Miłecki wrote:
> On 18 July 2016 at 07:53, Peter Chen <hzpeterchen@gmail.com> wrote:
> > On Mon, Jul 18, 2016 at 07:57:34AM +0200, Rafał Miłecki wrote:
> >> On 18 July 2016 at 07:40, Peter Chen <hzpeterchen@gmail.com> wrote:
> >> > On Mon, Jul 18, 2016 at 06:44:49AM +0200, Rafał Miłecki wrote:
> >> >> On 18 July 2016 at 04:31, Peter Chen <hzpeterchen@gmail.com> wrote:
> >> >> > On Fri, Jul 15, 2016 at 11:10:45PM +0200, Rafał Miłecki wrote:
> >> >> >> +
> >> >> >> +usbport trigger:
> >> >> >> +- usb-ports : List of USB ports that usbport should observed for turning on a
> >> >> >> +           given LED.
> >> >> >> +
> >> >> >
> >> >> > %s/should/should be
> >> >>
> >> >> Thanks.
> >> >>
> >> >>
> >> >> >> diff --git a/drivers/leds/trigger/ledtrig-usbport.c b/drivers/leds/trigger/ledtrig-usbport.c
> >> >> >> new file mode 100644
> >> >> >> index 0000000..97b064c
> >> >> >> --- /dev/null
> >> >> >> +++ b/drivers/leds/trigger/ledtrig-usbport.c
> >> >> >> @@ -0,0 +1,206 @@
> >> >> >> +/*
> >> >> >> + * USB port LED trigger
> >> >> >> + *
> >> >> >> + * Copyright (C) 2016 Rafał Miłecki <rafal.milecki@gmail.com>
> >> >> >> + *
> >> >> >> + * This program is free software; you can redistribute it and/or modify
> >> >> >> + * it under the terms of the GNU General Public License as published by
> >> >> >> + * the Free Software Foundation; either version 2 of the License, or (at
> >> >> >> + * your option) any later version.
> >> >> >> + */
> >> >> >
> >
> > In your COPYRIGHT, it says "or any later version". But afaik, It should
> > not be on GPL v3.
> 
> If one picks my code, modifies it, relicenses it using GPL v3, we
> can't include it anymore. It's the same story as with BSD systems and
> their BSD licenses. If one picks e.g. BSD 3-clause licensed code,
> makes it GPL, releases it (e.g. by putting in Linux), BSD can't use it
> anymore.
> 
> Still, this license is GPL v2 compatible and as is it can be included
> in the Linux.

Anything GPLv2 compatible is fair game for the kernel. GPLv2+ is very
common for the kernel.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1447064 — Re: [PATCH V2] leds: trigger: Introduce an USB port trigger

Hi!

> > @@ -0,0 +1,206 @@
> > +/*
> > + * USB port LED trigger
> > + *
> > + * Copyright (C) 2016 Rafał Miłecki <rafal.milecki@gmail.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or (at
> > + * your option) any later version.
> > + */
> 
> GPL v2 only.

No, you are not going to tell people which license they can use for
their kernel code.

GPL v2+ is perfectly fine for the kernel.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1446847 — Re: [PATCH V2] leds: trigger: Introduce an USB port trigger

On Fri, Jul 15, 2016 at 11:10:45PM +0200, Rafał Miłecki wrote:
> This commit adds a new trigger that can turn on LED when USB device gets
> connected to the USB port. This can be useful for various home routers
> that have USB port and a proper LED telling user a device is connected.
> 
> Right now this trigger is usable with a proper DT only, there isn't a
> way to specify USB ports from user space. This may change in a future.
> 
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> ---
> V2: The first version got support for specifying list of USB ports from
>     user space only. There was a (big try &) discussion on adding DT
>     support. It led to a pretty simple solution of comparing of_node of
>     usb_device to of_node specified in usb-ports property.
>     Since it appeared DT support may be simpler and non-DT a bit more
>     complex, this version drops previous support for "ports" and
>     "new_port" and focuses on DT only. The plan is to see if this
>     solution with DT is OK, get it accepted and then work on non-DT.
> 
> Felipe: if there won't be any objections I'd like to ask for your Ack.
> ---
>  Documentation/devicetree/bindings/leds/common.txt |  11 ++
>  Documentation/leds/ledtrig-usbport.txt            |  19 ++
>  drivers/leds/trigger/Kconfig                      |   8 +
>  drivers/leds/trigger/Makefile                     |   1 +
>  drivers/leds/trigger/ledtrig-usbport.c            | 206 ++++++++++++++++++++++
>  5 files changed, 245 insertions(+)
>  create mode 100644 Documentation/leds/ledtrig-usbport.txt
>  create mode 100644 drivers/leds/trigger/ledtrig-usbport.c
> 
> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
> index af10678..75536f7 100644
> --- a/Documentation/devicetree/bindings/leds/common.txt
> +++ b/Documentation/devicetree/bindings/leds/common.txt
> @@ -50,6 +50,12 @@ property can be omitted.
>  For controllers that have no configurable timeout the flash-max-timeout-us
>  property can be omitted.
>  
> +Trigger specific properties for child nodes:
> +
> +usbport trigger:
> +- usb-ports : List of USB ports that usbport should observed for turning on a
> +	      given LED.

I think this should be more generic such that it could work for disk or 
network LEDs too. Perhaps 'led-triggers = <nodes>'? trigger is a bit of 
a Linux name, but I haven't thought of anything better. Really, I'd 
prefer the link in the other direction (e.g. port node have a 'leds" or 
'*-leds' property), but that's maybe harder to parse.

> +
>  Examples:
>  
>  system-status {
> @@ -58,6 +64,11 @@ system-status {
>  	...
>  };
>  
> +usb {
> +	label = "USB";

It's not really clear in the example this is an LED node as it is 
incomplete.

> +	usb-ports = <&ohci_port1>, <&ehci_port1>;
> +};
> +
>  camera-flash {
>  	label = "Flash";
>  	led-sources = <0>, <1>;
> diff --git a/Documentation/leds/ledtrig-usbport.txt b/Documentation/leds/ledtrig-usbport.txt
> new file mode 100644
> index 0000000..642c4cd
> --- /dev/null
> +++ b/Documentation/leds/ledtrig-usbport.txt
> @@ -0,0 +1,19 @@
> +USB port LED trigger
> +====================
> +
> +This LED trigger can be used for signaling user a presence of USB device in a
> +given port. It simply turns on LED when device appears and turns it off when it
> +disappears.
> +
> +It requires specifying a list of USB ports that should be observed. This can be
> +done in DT by setting a proper property with list of a phandles. If more than
> +one port is specified, LED will be turned on as along as there is at least one
> +device connected to any of ports.
> +
> +This trigger can be activated from user space on led class devices as shown
> +below:
> +
> +  echo usbport > trigger

Why do I have to do this (by default)? I already specified in the DT 
what the connection is. It should come up working OOTB, or don't put it 
in DT.

> +Nevertheless, current there isn't a way to specify list of USB ports from user
> +space.

s/current/currently/

This is a problem since if it works by default and you switch to a 
different trigger, there's no way to get back to the default (unless 
you remember the ports).

Rob

[toc] | [prev] | [next] | [standalone]

#1447067 — Re: [PATCH V2] leds: trigger: Introduce an USB port trigger

On 20 July 2016 at 03:02, Rob Herring <robh@kernel.org> wrote:
> On Fri, Jul 15, 2016 at 11:10:45PM +0200, Rafał Miłecki wrote:
>> This commit adds a new trigger that can turn on LED when USB device gets
>> connected to the USB port. This can be useful for various home routers
>> that have USB port and a proper LED telling user a device is connected.
>>
>> Right now this trigger is usable with a proper DT only, there isn't a
>> way to specify USB ports from user space. This may change in a future.
>>
>> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
>> ---
>> V2: The first version got support for specifying list of USB ports from
>>     user space only. There was a (big try &) discussion on adding DT
>>     support. It led to a pretty simple solution of comparing of_node of
>>     usb_device to of_node specified in usb-ports property.
>>     Since it appeared DT support may be simpler and non-DT a bit more
>>     complex, this version drops previous support for "ports" and
>>     "new_port" and focuses on DT only. The plan is to see if this
>>     solution with DT is OK, get it accepted and then work on non-DT.
>>
>> Felipe: if there won't be any objections I'd like to ask for your Ack.
>> ---
>>  Documentation/devicetree/bindings/leds/common.txt |  11 ++
>>  Documentation/leds/ledtrig-usbport.txt            |  19 ++
>>  drivers/leds/trigger/Kconfig                      |   8 +
>>  drivers/leds/trigger/Makefile                     |   1 +
>>  drivers/leds/trigger/ledtrig-usbport.c            | 206 ++++++++++++++++++++++
>>  5 files changed, 245 insertions(+)
>>  create mode 100644 Documentation/leds/ledtrig-usbport.txt
>>  create mode 100644 drivers/leds/trigger/ledtrig-usbport.c
>>
>> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
>> index af10678..75536f7 100644
>> --- a/Documentation/devicetree/bindings/leds/common.txt
>> +++ b/Documentation/devicetree/bindings/leds/common.txt
>> @@ -50,6 +50,12 @@ property can be omitted.
>>  For controllers that have no configurable timeout the flash-max-timeout-us
>>  property can be omitted.
>>
>> +Trigger specific properties for child nodes:
>> +
>> +usbport trigger:
>> +- usb-ports : List of USB ports that usbport should observed for turning on a
>> +           given LED.
>
> I think this should be more generic such that it could work for disk or
> network LEDs too. Perhaps 'led-triggers = <nodes>'? trigger is a bit of
> a Linux name, but I haven't thought of anything better. Really, I'd
> prefer the link in the other direction (e.g. port node have a 'leds" or
> '*-leds' property), but that's maybe harder to parse.

I was already thinking on this as I plan to add "netdev" trigger at
some point in the future. I'd like to use "net-devices" for it (as a
equivalent or "usb-ports").

However there is a problem with your idea of sharing "led-triggers"
between triggers.. Consider device with generic purpose LED that
should be controllable by a user. I believe we should let user switch
it's state, e.g. with:
echo usbport > trigger
echo netdev > trigger
with a shared property I don't see how we couldn't handle it properly.
I don't think we can use anything like:
led-triggers = <&ohci_port1>, <&ehci_port1>, <&ethmac0>;
I'd get even more complicated if we ever need cells for any trigger.
AFAIK it's not allowed to mix handles with different cells like:
led-triggers = <&ohci_port1>, <&foo 5>, <&ethmac0>;

These problems made me use trigger-specific properies for specifying
LED triggers. Do you have any other idea for sharing a property &
avoiding above problems at the same time?


>> +
>>  Examples:
>>
>>  system-status {
>> @@ -58,6 +64,11 @@ system-status {
>>       ...
>>  };
>>
>> +usb {
>> +     label = "USB";
>
> It's not really clear in the example this is an LED node as it is
> incomplete.

What do you mean by incomplete? Should I include e.g. ohci_port1 in
this example?


>> +     usb-ports = <&ohci_port1>, <&ehci_port1>;
>> +};
>> +
>>  camera-flash {
>>       label = "Flash";
>>       led-sources = <0>, <1>;
>> diff --git a/Documentation/leds/ledtrig-usbport.txt b/Documentation/leds/ledtrig-usbport.txt
>> new file mode 100644
>> index 0000000..642c4cd
>> --- /dev/null
>> +++ b/Documentation/leds/ledtrig-usbport.txt
>> @@ -0,0 +1,19 @@
>> +USB port LED trigger
>> +====================
>> +
>> +This LED trigger can be used for signaling user a presence of USB device in a
>> +given port. It simply turns on LED when device appears and turns it off when it
>> +disappears.
>> +
>> +It requires specifying a list of USB ports that should be observed. This can be
>> +done in DT by setting a proper property with list of a phandles. If more than
>> +one port is specified, LED will be turned on as along as there is at least one
>> +device connected to any of ports.
>> +
>> +This trigger can be activated from user space on led class devices as shown
>> +below:
>> +
>> +  echo usbport > trigger
>
> Why do I have to do this (by default)? I already specified in the DT
> what the connection is. It should come up working OOTB, or don't put it
> in DT.

Just specifying connection with "usb-ports" (or "led-triggers") won't
enable a given trigger and it shouldn't. There is already a way to
specify default trigger using property "linux,default-trigger". So you
can specify:
1) Default one with:
linux,default-trigger = "usbport";
2) On runtime:
echo usbport > trigger


>> +Nevertheless, current there isn't a way to specify list of USB ports from user
>> +space.
>
> s/current/currently/
>
> This is a problem since if it works by default and you switch to a
> different trigger, there's no way to get back to the default (unless
> you remember the ports).

There is no such problem. You can freely do:
echo none > trigger
(e.g. to disable LED during night or whatever)
and then:
echo usbport > trigger

This will make "usbport" use triggers specified in DT as expected.

-- 
Rafał

[toc] | [prev] | [next] | [standalone]

#1448131 — Re: [PATCH V2] leds: trigger: Introduce an USB port trigger

On Wed, Jul 20, 2016 at 10:06:23AM +0200, Rafał Miłecki wrote:
> On 20 July 2016 at 03:02, Rob Herring <robh@kernel.org> wrote:
> > On Fri, Jul 15, 2016 at 11:10:45PM +0200, Rafał Miłecki wrote:
> >> This commit adds a new trigger that can turn on LED when USB device gets
> >> connected to the USB port. This can be useful for various home routers
> >> that have USB port and a proper LED telling user a device is connected.
> >>
> >> Right now this trigger is usable with a proper DT only, there isn't a
> >> way to specify USB ports from user space. This may change in a future.
> >>
> >> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> >> ---
> >> V2: The first version got support for specifying list of USB ports from
> >>     user space only. There was a (big try &) discussion on adding DT
> >>     support. It led to a pretty simple solution of comparing of_node of
> >>     usb_device to of_node specified in usb-ports property.
> >>     Since it appeared DT support may be simpler and non-DT a bit more
> >>     complex, this version drops previous support for "ports" and
> >>     "new_port" and focuses on DT only. The plan is to see if this
> >>     solution with DT is OK, get it accepted and then work on non-DT.
> >>
> >> Felipe: if there won't be any objections I'd like to ask for your Ack.
> >> ---
> >>  Documentation/devicetree/bindings/leds/common.txt |  11 ++
> >>  Documentation/leds/ledtrig-usbport.txt            |  19 ++
> >>  drivers/leds/trigger/Kconfig                      |   8 +
> >>  drivers/leds/trigger/Makefile                     |   1 +
> >>  drivers/leds/trigger/ledtrig-usbport.c            | 206 ++++++++++++++++++++++
> >>  5 files changed, 245 insertions(+)
> >>  create mode 100644 Documentation/leds/ledtrig-usbport.txt
> >>  create mode 100644 drivers/leds/trigger/ledtrig-usbport.c
> >>
> >> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
> >> index af10678..75536f7 100644
> >> --- a/Documentation/devicetree/bindings/leds/common.txt
> >> +++ b/Documentation/devicetree/bindings/leds/common.txt
> >> @@ -50,6 +50,12 @@ property can be omitted.
> >>  For controllers that have no configurable timeout the flash-max-timeout-us
> >>  property can be omitted.
> >>
> >> +Trigger specific properties for child nodes:
> >> +
> >> +usbport trigger:
> >> +- usb-ports : List of USB ports that usbport should observed for turning on a
> >> +           given LED.
> >
> > I think this should be more generic such that it could work for disk or
> > network LEDs too. Perhaps 'led-triggers = <nodes>'? trigger is a bit of
> > a Linux name, but I haven't thought of anything better. Really, I'd
> > prefer the link in the other direction (e.g. port node have a 'leds" or
> > '*-leds' property), but that's maybe harder to parse.
> 
> I was already thinking on this as I plan to add "netdev" trigger at
> some point in the future. I'd like to use "net-devices" for it (as a
> equivalent or "usb-ports").
> 
> However there is a problem with your idea of sharing "led-triggers"
> between triggers.. Consider device with generic purpose LED that
> should be controllable by a user. I believe we should let user switch
> it's state, e.g. with:
> echo usbport > trigger
> echo netdev > trigger
> with a shared property I don't see how we couldn't handle it properly.

Well, the userspace interface and DT bindings are independent things. 
You can't argue for what the DT binding should look like based on the 
userspace interface.

Perhaps we need a "device trigger" where you echo the device to 
be the trigger (similar to how bind works). If you have something to id 
the device, then you can lookup its struct device and then its of_node 
ptr.

The other problem with your userspace interface is it only works if 
the trigger is defined in DT. It doesn't allow for specifying which usb 
port or which netdev. Any trigger source should be assignable to any 
LED? I can assign the disk activity trigger to the numlock LED if I 
want to...

> I don't think we can use anything like:
> led-triggers = <&ohci_port1>, <&ehci_port1>, <&ethmac0>;
> I'd get even more complicated if we ever need cells for any trigger.
> AFAIK it's not allowed to mix handles with different cells like:
> led-triggers = <&ohci_port1>, <&foo 5>, <&ethmac0>;

It is allowed, but you would have to have a '#led-trigger-cells' 
property in each phandle target.

> These problems made me use trigger-specific properies for specifying
> LED triggers. Do you have any other idea for sharing a property &
> avoiding above problems at the same time?
> 
> 
> >> +
> >>  Examples:
> >>
> >>  system-status {
> >> @@ -58,6 +64,11 @@ system-status {
> >>       ...
> >>  };
> >>
> >> +usb {
> >> +     label = "USB";
> >
> > It's not really clear in the example this is an LED node as it is
> > incomplete.
> 
> What do you mean by incomplete? Should I include e.g. ohci_port1 in
> this example?

label and usb-ports alone in this node does not make an LED node.

> 
> 
> >> +     usb-ports = <&ohci_port1>, <&ehci_port1>;
> >> +};
> >> +
> >>  camera-flash {
> >>       label = "Flash";
> >>       led-sources = <0>, <1>;
> >> diff --git a/Documentation/leds/ledtrig-usbport.txt b/Documentation/leds/ledtrig-usbport.txt
> >> new file mode 100644
> >> index 0000000..642c4cd
> >> --- /dev/null
> >> +++ b/Documentation/leds/ledtrig-usbport.txt
> >> @@ -0,0 +1,19 @@
> >> +USB port LED trigger
> >> +====================
> >> +
> >> +This LED trigger can be used for signaling user a presence of USB device in a
> >> +given port. It simply turns on LED when device appears and turns it off when it
> >> +disappears.
> >> +
> >> +It requires specifying a list of USB ports that should be observed. This can be
> >> +done in DT by setting a proper property with list of a phandles. If more than
> >> +one port is specified, LED will be turned on as along as there is at least one
> >> +device connected to any of ports.
> >> +
> >> +This trigger can be activated from user space on led class devices as shown
> >> +below:
> >> +
> >> +  echo usbport > trigger
> >
> > Why do I have to do this (by default)? I already specified in the DT
> > what the connection is. It should come up working OOTB, or don't put it
> > in DT.
> 
> Just specifying connection with "usb-ports" (or "led-triggers") won't
> enable a given trigger and it shouldn't. There is already a way to
> specify default trigger using property "linux,default-trigger". So you
> can specify:
> 1) Default one with:
> linux,default-trigger = "usbport";
> 2) On runtime:
> echo usbport > trigger

IMO, these should be mutually exclusive. Either you specify a DT node as 
the trigger or you specify a linux specific trigger.

> >> +Nevertheless, current there isn't a way to specify list of USB ports from user
> >> +space.
> >
> > s/current/currently/
> >
> > This is a problem since if it works by default and you switch to a
> > different trigger, there's no way to get back to the default (unless
> > you remember the ports).
> 
> There is no such problem. You can freely do:
> echo none > trigger
> (e.g. to disable LED during night or whatever)
> and then:
> echo usbport > trigger
> 
> This will make "usbport" use triggers specified in DT as expected.

I think the singular "usbport" trigger is problematic. Look at how cpu 
triggers are done. I can specify which cpu is the trigger. You should be 
able specify which port is the trigger, too.

In summary, I guess what I'm saying is don't push the problems of the 
current userspace interface down to DT.

Rob

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web