> El 3/4/2015, a las 4:06, Florian Fainelli <f.fainelli@xxxxxxxxx> 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@xxxxxxxxx> >> Signed-off-by: Jonas Gorski <jogo@xxxxxxxxxxx> >> --- >> .../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). >> +- 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. > >> + >> +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. > >> +- 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). > >> +- brcm,activity-selection (optional) : LED activity selection values. > > Ditto. > -- > Florian I will wait some time before sending the v2 with the changes suggested by Florian, so everyone can share their opinion (just in case there are other things to modify). -- Álvaro-- To unsubscribe from this list: send the line "unsubscribe linux-leds" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
