Re: [PATCH 1/2] leds: add DT binding for BCM6328 LED controller

[Jonas Gorski <jogo@openwrt.org>]

On 03.04.2015 14:19, Álvaro Fernández Rojas wrote:
> 
>> El 3/4/2015, a las 4:06, Florian Fainelli <f.fainelli@gmail.com> escribió:
>>
>>> On 02/04/15 04:54, Álvaro Fernández Rojas wrote:
>>> This adds device tree binding documentation for the Broadcom BCM6328 LED
>>> controller.
>>
>> Looks pretty good to me, few comments below:
>>
>>>
>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>>> Signed-off-by: Jonas Gorski <jogo@openwrt.org>
>>> ---
>>> .../devicetree/bindings/leds/leds-bcm6328.txt      | 173 +++++++++++++++++++++
>>> 1 file changed, 173 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/leds/leds-bcm6328.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/leds/leds-bcm6328.txt b/Documentation/devicetree/bindings/leds/leds-bcm6328.txt
>>> new file mode 100644
>>> index 0000000..e63d27f
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/leds/leds-bcm6328.txt
>>> @@ -0,0 +1,173 @@
>>> +LEDs connected to Broadcom BCM6328 controller
>>> +
>>> +Required properties:
>>> +- compatible : should be : "brcm,bcm6328-leds".
>>
>> Something like "brcm,bcm6328-leds-ctrl" might be a bit more descriptive.
> 
> Yeah you're right, bcm6328-leds-ctrl is better.
> 
>>
>>> +- #address-cells: must be 1
>>> +- #size-cells: must be 0
>>> +- reg: BCM6328 LED controller address and size.
>>> +
>>> +Optional properties:
>>> +- brcm,serial-leds: enable Serial LEDs.
>>> +
>>> +Each led is represented as a sub-node of the brcm,bcm6328-leds device.
>>> +
>>> +LED sub-node properties:
>>> +- reg : LED pin number (could be from 0 to 23).

I think this should be a bit more strict: "Only leds 0 to 23 are valid".

>>> +- compatible : should be : "brcm,bcm6328-led".
>>
>> Do we really need to strongly type the LED child nodes here with a
>> compatible string? It is somewhat implicit that if your parent is
>> "brcm,bcm6328-leds", the child nodes are of the same "type"?
> 
> This is just a sanity check inherited from other LED controllers, but I can remove it. In fact I prefer it, because adding compatible strings on each subnode can be a bit annoying, specially if there are a lot of LEDs.

Well, you might have/want other subnodes like pinctrl configurations.
But since we have a reg-property, the existence of it is probably enough
to tell it apart from non-led-configuration nodes.

> 
>>
>>> +
>>> +Normal LED:
>>> +- label (optional) : see Documentation/devicetree/bindings/leds/common.txt
>>> +- active-low (optional) : LED is active low.
>>
>> If it is an optional property, you would want to clarify that active
>> high is the default.
> 
> Ok, I will.
> 
>>
>>> +- default-state (optional): see
>>> +  Documentation/devicetree/bindings/leds/leds-gpio.txt
>>> +- linux,default-trigger (optional): see
>>> +  Documentation/devicetree/bindings/leds/common.txt
>>> +
>>> +Hardware controlled LED:
>>> +- brcm,hardware-controlled (optional) : LED is hardware controlled.
>>
>> The type of this property is not strictly defined, but it would appear
>> to be a boolean. Reading through the driver though, it looks like the
>> number of LEDs "hardware" controlled should be something customizable at
>> some point.
> 
> Actually the number of hardware controlled LEDs depends on each SoC, but it can also depend on the specific board. There's a 32 bit register (HWDIS) which controls which LEDs are hardware controlled.

Actually this bit toggles whether the led should be controlled by the
appropriate hardware signal, or may be controlled by software through
the appropriate registers. You can enable it for all leds, but only a
subset is actually wired to an actual function.

The hardware signals aren't consecutive. E.g. on BCM6362 0 is the USB
led, 1 is the "internet" led, and 4 to 7 are the ephy leds. 2 or 3 do
not have a defined function, at least in the public sources. BCM6328 has
its ephy activity leds at 17~20, etc.

So it's technically a configuration value, but allowing this to be
changed seems quite a challenge from the current linux led apis. AFAICT
triggers have no way of only working on certain led controllers, and led
controllers have no way of knowing that a led is controlled by a certain
trigger.

Which functions maps to which led is probably something better described
in header files or default led-nodes in the dtsi files.

>>> +- brcm,link-selection (optional) : LED link selection values.
>>
>> Here you should specify exactly which values are accepted, if this is a
>> value that is directly mapping to a hardware register value, we might
>> want to create a header file for that.
> 
> The problem of this is that it depends on each SoC but also on each board, since this controls which LED from LEDs 0-7 is controlled by each ethernet activity and speed "event". For example, Jonas managed to set a specific LED to be controlled by every ethernet port on the board, so there may be boards in which Ethernet ports may not be directly assigned to the Ethernet LEDs, but swapped (it's not the most likely situtation, but since there isn't much documentation from Broadcom available it seems sensible to me to offer that configuration posibility).

A potential use case for using more than one source for a led is if you
have only one "lan" led and want to mux all four phy activity/link
signals onto it.

A better name and description might be

- brcm,link-signal-sources (optional): An array of hardware link
  signal sources. Up to four link hardware signals can get muxed into
  this leds. Only valid for leds 0 to 7, where led signals 0 to 3 may
  be muxed to leds 0 to 3, and signals 4 to 7 may be muxed to leds
  4 to 7. A signal can be muxed to more than one led, and one led can
  have more than one source signal.

> 
>>
>>> +- brcm,activity-selection (optional) : LED activity selection values.
>>
>> Ditto.

And this one called "brcm,activity-signal-sources".


Regards
Jonas