From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752045AbcIBGyz (ORCPT ); Fri, 2 Sep 2016 02:54:55 -0400 Received: from mailout2.w1.samsung.com ([210.118.77.12]:54845 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750975AbcIBGyx (ORCPT ); Fri, 2 Sep 2016 02:54:53 -0400 MIME-version: 1.0 Content-type: text/plain; charset=utf-8; format=flowed X-AuditID: cbfec7f4-f79cb6d000001359-b4-57c922381e6a Subject: Re: [PATCH V4] leds: trigger: Introduce an USB port trigger To: Alan Stern References: Cc: =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= , Greg KH , Richard Purdie , Felipe Balbi , Peter Chen , "linux-usb@vger.kernel.org" , =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= , Jonathan Corbet , Ezequiel Garcia , Boris Brezillon , Geert Uytterhoeven , Stephan Linz , open list , "open list:DOCUMENTATION" , "open list:LED SUBSYSTEM" From: Jacek Anaszewski Message-id: Date: Fri, 02 Sep 2016 08:54:47 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 In-reply-to: Content-transfer-encoding: 8bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrOIsWRmVeSWpSXmKPExsVy+t/xq7oWSifDDXb2Glsca3vCbnHgxUIW iycH2hktNr74zGLx7NZeJovmxevZLLqubmKzWNi2hMXi8q45bBZb36xjtFi0rJXZ4urrRSwW u9cuYrLYvespq8WE3xfYLNacTHUQ8Hiy6SKjx85Zd9k9Nq3qZPPY/TbV49DhDkaP/XPXsHss 7pvM6nF+qqfH7Ls/GD32zP/B6tF15Dqbx+dNcgE8UVw2Kak5mWWpRfp2CVwZ12atYC1YoVLx 9HNQA+Mp2S5GDg4JAROJg78Euhg5gUwxiQv31rN1MXJxCAksZZSY/HY3K0iCV0BQ4sfkeywg NrOAmcSXl4dZIYqeMUpM+vSJCSQhLOAiMf/QUzYQW0RAS2Jz00tmEFtIwEdi46m17CANzALb WSUmHvkLNpVNwFDi54vXTBAb7CRO/N7CAnIRi4CqxP0l9SBhUYEIiVurPjKC2JwCvhKXl3yC OkJe4uCV5ywTGAVmIblvFpL7ZiEpW8DIvIpRNLU0uaA4KT3XUK84Mbe4NC9dLzk/dxMjJPq+ 7GBcfMzqEKMAB6MSDy+H0YlwIdbEsuLK3EOMEhzMSiK8f+VPhgvxpiRWVqUW5ccXleakFh9i lOZgURLnnbvrfYiQQHpiSWp2ampBahFMlomDU6qBUf1MQtqeNTGfH+c3ZHu98djYc62WT/NN omrmkaOqbZVHde8nru3/vva9YsHP7Jz+6EW3JqZVMtpFnRM4Wt3Ev+qDj79Qq0vLmgXqyxZd +yf9j+WXuYfA1MXBu6fMvO066wPz4fvufS/vB35QY1N++UJ20+b5dlEPzE++l1y9LvYxj2/F 8d7mViWW4oxEQy3mouJEAK9ArO+6AgAA Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/01/2016 04:36 PM, Alan Stern wrote: > On Thu, 1 Sep 2016, Jacek Anaszewski wrote: > >> On 09/01/2016 07:25 AM, Rafał Miłecki wrote: >>> On 31 August 2016 at 21:00, Rafał Miłecki wrote: >>>> On 31 August 2016 at 20:23, Alan Stern wrote: >>>>> On Tue, 30 Aug 2016, Rafał Miłecki wrote: >>>>>> Not really as it won't cover some pretty common use cases. Many home >>>>>> routers have few USB ports (2-5) and only 1 USB LED. It has to be >>>>>> possible to assign few USB ports to a single LED (trigger). That way >>>>>> LED should be turned on (and kept on) if there is at least 1 USB >>>>>> device connected. You obviously can't do: >>>>>> echo "usb1-1 usb1-2 usb2-1" > /sys/class/leds/foo/trigger >>>>>> >>>>>> This was already brought up by Rob (who mentioned CPU trigger) and I >>>>>> replied him pretty much the same way in: >>>>>> https://lkml.org/lkml/2016/7/29/38 >>>>>> (reply starts with "Anyway, the serious limitation I see"). >>>>> >>>>> The code for a bunch of triggers must already be written. What would >>>>> the user do if he wanted to flash a single LED in response to both >>>>> CPU activity and MTD activity? If not >>>>> >>>>> echo "cpu mtd" >/sys/class/leds/foo/trigger >>>>> >>>>> then what? >>>> >>>> Well, it sounds like a new feature then. Shall we add an extra API >>>> with a request function for turning LED on? It could internally count >>>> how many requests were raised and keep LED on as long as there is at >>>> least 1 left. I guess we should implement it in trigger "subsystem" >>>> (if I can call it so). Does it sound like a good plan? >>> >>> I'm pretty sure noone ever planned to have more than 1 trigger >>> assigned to a single LED. I just realized there will be a problem with >>> proposed solution: sysfs files conflict. >>> >>> Consider 2 existing triggers for a moment: >>> 1) oneshot: it creates following sysfs files: >>> /sys/class/leds/foo/delay_on >>> /sys/class/leds/foo/delay_off >>> /sys/class/leds/foo/invert >>> /sys/class/leds/foo/shot >>> 2) timer: it creates following sysfs files: >>> /sys/class/leds/foo/delay_on >>> /sys/class/leds/foo/delay_off >>> >>> Activating both of them will probably cause a WARNING in sysfs. They >>> can't coexist :( >>> >>> We should probably have per-trigger subdirs, e.g.: >>> /sys/class/leds/foo/trigger-oneshot/delay_on >>> /sys/class/leds/foo/trigger-oneshot/delay_off >>> /sys/class/leds/foo/trigger-oneshot/invert >>> /sys/class/leds/foo/trigger-oneshot/shot >>> /sys/class/leds/foo/trigger-timer/delay_on >>> /sys/class/leds/foo/trigger-timer/delay_off >>> but implementing it now would break the ABI. >>> >>> One workaround I can see is doing triggers V2, they: >>> 1) Would put sysfs files in /sys/class/leds/foo/trigger-bar/ >>> 2) Use new API for *requesting* LED to be on/off >>> 3) There would be a counter of requests in V2 API >>> 4) Multiple triggers V2 would be allowed to be used (assigned) at the same time >> >> Currently we support only triggers dedicated to specific type of >> devices. Even in case of triggers that don't expose custom sysfs >> attributes, registered with led_trigger_register_simple(), device >> drivers have to generate trigger event with dedicated function, e.g: >> >> - ledtrig-cpu: void ledtrig_cpu(enum cpu_led_event ledevt) >> - ledtrig-disk: void ledtrig_disk_activity(void) >> - ledtrig-mtd: void ledtrig_mtd_activity(void) >> >> If one wanted to have the LED notified by different type of devices, >> then they would have to implement a trigger that would exposed all >> required types of API. >> >> Unfortunately, there are many possible combinations of >> triggers and that doesn't sound sane to add a new one when someone >> will find it useful. Of course it would entail adding a call to the >> new trigger API in the drivers, which doesn't seem like something >> acceptable in the mainline. > > Maybe it would make more sense, in this case, to allow only three > possibilities for a USB port activity trigger. Toggle the LED > whenever: > > There is activity on the specified port, or > > There is any activity on any port on the specified hub, or > > There is any USB activity on any port. > > That ought to cover most of the normal use cases, and it would be > simple enough to implement. What would be the benefit of having a USB port activity trigger, for which we would be specifying the port to observe, but in the same time we would react on any activity on any port (cases 1 and 3)? -- Best regards, Jacek Anaszewski