Re: [PATCH 1/2] dt-bindings: leds: Add binding for ubnt-spi LED.

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

On 5/4/19 9:48 PM, Christian Mauderer wrote:
> On 04/05/2019 21:34, Jacek Anaszewski wrote:
>> Hi Christian,
>>
>> Thank you for the patch.
> 
> Hello Jacek,
> 
> thank you for your time to review it.

You're welcome.

>>
>> On 5/4/19 2:28 PM, list@c-mauderer.de wrote:
>>> From: Christian Mauderer <oss@c-mauderer.de>
>>>
>>> This patch adds the binding documentation for the LED controller found
>>> in Ubiquity airCube ISP devices.
>>>
>>> Signed-off-by: Christian Mauderer <oss@c-mauderer.de>
>>> ---
>>>
>>> I tested the patches with a 4.14 and a 4.19 kernel on the current
>>> OpenWRT.
>>> Although I didn't get the kernel running due to file system problems
>>> they build
>>> fine with a 5.1-rc7.
>>>
>>> I shortly described the protocol of the controller in a comment in the
>>> driver
>>> file in the second patch.
>>>
>>> Checkpatch gives the following warning for both patches:
>>>
>>>     WARNING: added, moved or deleted file(s), does MAINTAINERS need
>>> updating?
>>>
>>> To be honest: I don't know what to do with it. Please excuse my
>>> ignorance here.
>>> It's the first driver that I want to add to the Linux kernel.
>>
>> You can add yourself as a maintainer of this driver, but it is customary
>> rather for more complex drivers.
> 
> I would like to follow the best practice here. So if you say that for
> this simple driver it's not usual, I won't add me. Except if you think
> it is useful to have someone to blame for it ;-)

People will always have your email in the driver, so I'd ignore
that warning as Pavel already proposed.

>>> Please point me to some documentation if I did miss some big points for
>>> submitting patches.
>>>
>>>
>>>    .../bindings/leds/leds-ubnt-spi.txt           | 49 +++++++++++++++++++
>>>    1 file changed, 49 insertions(+)
>>>    create mode 100644
>>> Documentation/devicetree/bindings/leds/leds-ubnt-spi.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/leds/leds-ubnt-spi.txt
>>> b/Documentation/devicetree/bindings/leds/leds-ubnt-spi.txt
>>> new file mode 100644
>>> index 000000000000..ab1478cdc139
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/leds/leds-ubnt-spi.txt
>>> @@ -0,0 +1,49 @@
>>> +Binding for the controller based LED found in Ubiquity airCube ISP
>>> and most
>>> +likely some other Ubiquity devices.
>>> +
>>> +The protocol of the controller is quite simple. Only one byte will be
>>> sent. The
>>> +value of the byte can be between the ubnt-spi,off_bright value and the
>>> +ubnt-spi,max_bright value.
>>> +
>>> +The driver maybe can be used for other devices with a similar
>>> protocol too.
>>> +
>>> +Required properties:
>>> +- compatible:        Should be "ubnt,spi-led".
>>> +- spi-max-frequency:    Should be <100000> for this device.
>>> +
>>> +Optional sub-node properties:
>>> +- ubnt-spi,off_bright:    The value that will be sent if the LED
>>> should be
>>> +            switched off. Default value is 0.
>>> +- ubnt-spi,max_bright:    Value for the maximum brightness. Default
>>> value for that
>>> +            is 63.
>>> +- label:        A label for the LED. If one is given, the LED will be
>>> +            named "ubnt-spi:<label>" or "ubnt-spi::" otherwise.
>>> +
>>> +Being a SPI device this driver should be a sub-node of a SPI
>>> controller. The
>>> +controller only supports one LED. For consistence with other
>>> controllers, the
>>> +LED is defined as a sub-node.
>>> +
>>> +Example for the airCube ISP (with SPI controller matching that device):
>>> +
>>> +led_spi {
>>> +    compatible = "spi-gpio";
>>> +    #address-cells = <1>;
>>> +    #size-cells = <0>;
>>> +
>>> +    gpio-sck = <&gpio 3 GPIO_ACTIVE_HIGH>;
>>> +    gpio-mosi = <&gpio 2 GPIO_ACTIVE_HIGH>;
>>> +    cs-gpios = <&gpio 1 GPIO_ACTIVE_HIGH>;
>>> +    num-chipselects = <1>;
>>
>> SPI node implementation is out of LED bindings scope.
>> Here you're showing spi-gpio configuration, but people are free to use
>> hardware SPI module if available on the platform of their choice.
>>
>> Effectively, please remove above led_spi node. You can compare other SPI
>> based LED bindings, e.g.:
>>
>> Documentation/devicetree/bindings/leds/leds-cr0014114.txt
> 
> Reason behind adding it was that most likely the controller is only
> useful for that device. Of course that's no longer true together with
> the suggestion from Pavel Machek to make it more generic.
> 
> I'll follow your suggestion.

Thank you.
>>> +    led_ubnt@0 {
>>
>> s/led_usbnt/led-controller/
>>
>>> +        compatible = "ubnt,spi-led";
>>> +        reg = <0>;
>>> +        spi-max-frequency = <100000>;
>>> +
>>> +        led {
>>> +            label = "system";
>>
>> In label we expect "color:function" pattern. If section is to be left
>> empty than just leave it blank, e.g.:
>>
>>              label = ":system"
>>
>> But, is this LED function name telling something useful?
>> What is the actual function of this LED?
> 
> The LED is a white one. It's the only LED of a WLAN access point. There
> are not even any network LEDs. Ubiquiti uses it to show some system
> states with different pulsing patterns during boot. After that it's
> always on.
> 
> In OpenWRT it maybe could also show some network traffic (blinking
> between dim and bright). But that's user configurable. My plan was to
> just let it dim when the system is booted.

Thank you for this explanation.

>>
>> I work for some time on LED unification patch set and there is
>> a patch with common LED function names [0], but there is nothing
>> suitable for access points. There is "wlan", but is is rather for
>> wifi dongle LEDs (side note: "wifi" seems to be more ubiquitous,
>> so I will probably go for it finally).
>>
>> So, maybe we for access points "wifi-ap" would work?
>> i.e. I propose the label in the form:
>>
>>              label  = ":wifi-ap"
>>
> 
> I wasn't aware of that list. Maybe "power" or even better "status" would
> match the function.

Hmm, I've just found out that there are two "wlan-ap" occurrences in
the existing mainline bindings, so I propose to follow that.

> Should I add the color too? So "white:status"?

Yes, why not if it is known. So, having the above I propose:

		label = "white:wlan-ap";

>>
>>> +            /* keep the LED slightly on to show powered device */
>>> +            ubnt-spi,off_bright = /bits/ 8 <4>;
>>> +        };
>>> +    };
>>> +};
>>>
>>
>> [0] https://www.spinics.net/lists/kernel/msg3103824.html
>>
> 

-- 
Best regards,
Jacek Anaszewski