From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753180AbcGMOsY (ORCPT ); Wed, 13 Jul 2016 10:48:24 -0400 Received: from mailout3.w1.samsung.com ([210.118.77.13]:21278 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753026AbcGMOsP (ORCPT ); Wed, 13 Jul 2016 10:48:15 -0400 MIME-version: 1.0 Content-type: text/plain; charset=UTF-8; format=flowed X-AuditID: cbfec7f4-f796c6d000001486-4b-578654a6e0dc Content-transfer-encoding: 8BIT Message-id: <578654A4.9030009@samsung.com> Date: Wed, 13 Jul 2016 16:48:04 +0200 From: Jacek Anaszewski User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130804 Thunderbird/17.0.8 To: =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= Cc: Richard Purdie , Jonathan Corbet , Ezequiel Garcia , Geert Uytterhoeven , Stephan Linz , Matthias Brugger , Boris Brezillon , "open list:DOCUMENTATION" , open list , "open list:LED SUBSYSTEM" , balbi@kernel.org Subject: Re: [PATCH] leds: trigger: Introduce an USB port trigger References: <1468239883-22695-1-git-send-email-zajec5@gmail.com> In-reply-to: <1468239883-22695-1-git-send-email-zajec5@gmail.com> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFupikeLIzCtJLcpLzFFi42I5/e/4Fd1lIW3hBpvfs1kca3vCbnHgxUIW iycH2hktNr74zGLx7NZeJouFbUtYLC7vmsNmsfXNOkaLq68XsVgsmPyE1WL3rqesFmtOpjrw eDzZdJHRY+esu+wem1Z1snnsfpvqcehwB6PH4r7JrB575v9g9Vi/5SqLR9eR62wenzfJBXBF cdmkpOZklqUW6dslcGWsu3aTpeBcQMWnrrPMDYxvHLoYOTgkBEwkbs5w62LkBDLFJC7cW8/W xcjFISSwlFFiy4MLjCAJXgFBiR+T77GA1DMLyEscuZQNEmYWMJN41LKOGaL+GaPEupk/WSHq tST+7H7EBGKzCKhKTG+cChZnEzCU+PniNVhcVCBC4s/pfWBxEQFziSd9IPVcQENvMku0TT8D ViQs4CjxbMslsCIhAQeJy/tus4PYnEDxC8f+sE5gFJiF5L5ZCPfNQnLfAkbmVYyiqaXJBcVJ 6bmGesWJucWleel6yfm5mxghMfRlB+PiY1aHGAU4GJV4eBlFWsOFWBPLiitzDzFKcDArifAa BLeFC/GmJFZWpRblxxeV5qQWH2KU5mBREuedu+t9iJBAemJJanZqakFqEUyWiYNTqoGRkXNe pKIBo4xqVpn2/SrlyY9+FZZ+/XTv74G9gaHRU/uWT4ks9Uxzf2W08HtacuuCf9pS9wNT52pF yPx374oQtFsrNkU2ZkpIw497gfFnH58qjOKT+1Oq12d/rf6rbJgoR8Pcb/cUp9ppFUgvtDBe ExlzY9q8SS2Zk38Y+K348T0h4EfaPyUlluKMREMt5qLiRAC/dHJHnQIAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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 > + * > + * 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 > +#include > +#include > +#include > +#include > +#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 "); > +MODULE_DESCRIPTION("USB port trigger"); > +MODULE_LICENSE("GPL"); > -- Best regards, Jacek Anaszewski