Re: [PATCH RFC V3.5] leds: trigger: Introduce an USB port trigger

[Jacek Anaszewski <j.anaszewski@samsung.com>]

On 08/25/2016 10:29 AM, Rafał Miłecki wrote:
> On 25 August 2016 at 10:03, Jacek Anaszewski <j.anaszewski@samsung.com> wrote:
>> On 08/24/2016 07:52 PM, Rafał Miłecki wrote:
>>>
>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>
>>> This commit adds a new trigger responsible for turning on LED when USB
>>> device gets connected to the specified USB port. This can can useful for
>>> various home routers that have USB port(s) 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).
>>>
>>> During work on this trigger there was a plan to add DT bindings for it,
>>> but there wasn't an agreement on the format yet. This can be worked on
>>> later, a sysfs interface is needed anyway for platforms not using DT.
>>>
>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>> ---
>>> V2: Trying to add DT support, idea postponed as it will take more time
>>>     to discuss the bindings.
>>> V3: Fix typos in commit and Documentation (thanks Jacek!)
>>>     Use "ports" sysfs file for adding and removing USB ports (thx Jacek)
>>>     Check if there is USB device connected after adding new USB port
>>>     Fix memory leak or two
>>> V3.5: Fix e-mail address (thanks Matthias)
>>>       Simplify conditions in usbport_trig_notify (thx Matthias)
>>>       Make "ports" a subdirectory with file per port, to match one value
>>>       per file sysfs rule (thanks Greg)
>>>       As "ports" couldn't be used for adding and removing ports anymore,
>>>       there are now "new_port" and "remove_port". Having them makes this
>>>       API also common with e.g. pci and usb buses.
>>
>>
>> Now writing new_port with "1-1" produces a file named "1-1" in the
>> ports directory with 000 permissions. I think that what Greg had
>> on mind by referring to "one value per file" rule was a set of
>> files representing ports, like "1-1 1-2 2-1", and each file should be
>> readable/writeable.
>>
>> For instance "echo 1 > 1-1" would enable the trigger for the port 1-1
>> and "echo 0 > 1-1" would disable it. The problem is that we don't know
>> the number of required ports at compilation time and the sysfs
>> attributes would have to be dynamically created on driver instantiation.
>> What is more, as the USB ports can dynamically appear/disappear in the
>> system, the files would have to be created/removed accordingly during
>> LED class device lifetime, which is not the best design for the sysfs
>> interface I think.
>>
>> Therefore, maybe it would be good to follow the "triggers" sysfs
>> attribute pattern, where it lists the available LED triggers?
>>
>> The question is whether there is some mechanism available for
>> notifying addition/removal of a USB port?
>
> Every port is part of some hub and every hub (even root one) is a USB
> device. So I could just get struct usb_device and check its "maxchild"
> property. If e.g. I get USB device "1-1" with maxchild 4, I know there
> are:
> 1-1.1
> 1-1.2
> 1-1.3
> 1-1.4
>
> So the sysfs structure you suggested would be possible, I just don't
> know if it's preferred one or not.

It would require an ack from Greg.

I'd see it as follows:

#cat available_ports
#1-1 1-2 2-1

#echo "1-1" > new_port

#cat observed_ports
#1-1

#echo "2-1" > new_port

#cat observed_ports
#1-1 2-1

We've already had few discussions about the sysfs designs trying
to break the one-value-per-file rule for LED class device, and
there was always strong resistance against.

Cc Pavel.

> Confirmation: yes, right now I create simple files with chmod 000 for
> every port added to the usbport observable list.
>
>
>> Also a description of the device connected to the port would be a nice
>> feature, however I am not certain about the feasibility thereof.
>
> What kind of description do you mean? Where should it be used / where
> should it appear?
>

Product name/symbol. Actually it should be USB subsystem responsibility
to provide the means for querying the product name by port id, if it
is possible at all.

-- 
Best regards,
Jacek Anaszewski