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

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

Hi Alvaro,

On 04/05/2015 05:08 PM, Álvaro Fernández Rojas wrote:
> This adds device tree binding documentation for the Broadcom BCM6328 LED
> controller.
>
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> Signed-off-by: Jonas Gorski <jogo@openwrt.org>
> ---
> v2: Introduce changes suggested by Florian and Jonas.
>   - Change compatible string to "brcm,bcm6328-leds-ctrl".
>   - Make valid LEDs statement more strict.
>   - Remove compatible strings from LED subnodes.
>   - Clarify that LEDs are active high by default.
>   - Add a better description for brcm,link-signal-sources and brcm,activity-signal-sources.
>
>   .../devicetree/bindings/leds/leds-bcm6328.txt      | 162 +++++++++++++++++++++
>   1 file changed, 162 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..6e4f5563a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-bcm6328.txt
> @@ -0,0 +1,162 @@
> +LEDs connected to Broadcom BCM6328 controller

Part of description from the cover letter could find its way here.
It could be more precise though. I am wondering who controls the LEDs
through spi-gpio interface.

> +
> +Required properties:
> +- compatible: should be : "brcm,bcm6328-leds-ctrl".

I liked brcm,bcm6328-leds more. It is obvious that all LED subsystem
drivers are for LED *controllers*.

> +- #address-cells: must be 1
> +- #size-cells: must be 0
> +- reg: BCM6328 LED controller address and size.
> +
> +Optional properties:
> +- brcm,serial-leds: boolean which enables Serial LEDs.
> +
> +Each LED is represented as a sub-node of the brcm,bcm6328-leds device.
> +
> +LED sub-node properties:
> +- reg: LED pin number (only LEDs 0 to 23 are valid).
> +
> +Normal LED:

Please give it more meaningful description. Software controlled?

> +LEDs are active high by default.

This should be put next to active-low property, like:

active-low: boolean that makes LED active low.
             Default: false

> +- label (optional): see Documentation/devicetree/bindings/leds/common.txt
> +- active-low (optional): boolean that makes LED active low.
> +- 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): boolean that makes this LED hardware
> +  controlled.
> +- brcm,link-signal-sources (optional): An array of hardware link
> +  signal sources. Up to four link hardware signals can get muxed into
> +  these 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-signal-sources (optional): An array of hardware activity
> +  signal sources. Up to four activity hardware signals can get muxed into
> +  these 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.

It is more common to group properties in sections - in this case:
"Required properties for child nodes" and "Optional properties for
child nodes".


> +example 1) BCM6328
> +
> +leds0: led-controller@10000800 {
> +	compatible = "brcm,bcm6328-leds-ctrl";
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	reg = <0x10000800 0x24>;
> +
> +	alarm_red@2 {
> +		reg = <2>;
> +		active-low;
> +		label = "red:alarm";
> +	};
> +	inet_green@3 {
> +		reg = <3>;
> +		active-low;
> +		label = "green:inet";
> +	};
> +	power_green@4 {
> +		reg = <4>;
> +		active-low;
> +		label = "green:power";
> +		default-state = "on";
> +	};
> +	ephy0_spd@17 {
> +		reg = <17>;
> +		brcm,hardware-controlled;

If there is no LED class device created for hardware controlled LED,
then why does it need the node if has no brcm,link-signal-sources
or brcm,activity-signal-sources properties? There seems to be nothing
to configure then.

> +	};
> +	ephy1_spd@18 {
> +		reg = <18>;
> +		brcm,hardware-controlled;
> +	};
> +	ephy2_spd@19 {
> +		reg = <19>;
> +		brcm,hardware-controlled;
> +	};
> +	ephy3_spd@20 {
> +		reg = <20>;
> +		brcm,hardware-controlled;
> +	};
> +};
> +
> +example 2) BCM63268
> +
> +leds0: led-controller@10001900 {
> +	compatible = "brcm,bcm6328-leds-ctrl";
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	reg = <0x10001900 0x24>;
> +	brcm,serial-leds;
> +
> +	gphy0_spd0@0 {
> +		reg = <0>;
> +		brcm,hardware-controlled;
> +		brcm,link-signal-sources = <0>;

More valid examples with brcm,link-signal-sources and
brcm,activity-signal-sources properties would be useful. This is
especially vital taking into account limitations mentioned in the
description of these properties. The examples should be as
comprehensive as possible, i.e. the lists should contain more than
one element.

> +	};
> +	gphy0_spd1@1 {
> +		reg = <1>;
> +		brcm,hardware-controlled;
> +		brcm,link-signal-sources = <1>;
> +	};
> +	inet_red@2 {
> +		reg = <2>;
> +		active-low;
> +		label = "red:inet";
> +	};
> +	dsl_green@3 {
> +		reg = <3>;
> +		active-low;
> +		label = "green:dsl";
> +	};
> +	usb_green@4 {
> +		reg = <4>;
> +		active-low;
> +		label = "green:usb";
> +	};
> +	wps_green@7 {
> +		reg = <7>;
> +		active-low;
> +		label = "green:wps";
> +	};
> +	inet_green@8 {
> +		reg = <8>;
> +		active-low;
> +		label = "green:inet";
> +	};
> +	ephy0_act@9 {
> +		reg = <9>;
> +		brcm,hardware-controlled;
> +	};
> +	ephy1_act@10 {
> +		reg = <10>;
> +		brcm,hardware-controlled;
> +	};
> +	ephy2_act@11 {
> +		reg = <11>;
> +		brcm,hardware-controlled;
> +	};
> +	gphy0_act@12 {
> +		reg = <12>;
> +		brcm,hardware-controlled;
> +	};
> +	ephy0_spd@13 {
> +		reg = <13>;
> +		brcm,hardware-controlled;
> +	};
> +	ephy1_spd@14 {
> +		reg = <14>;
> +		brcm,hardware-controlled;
> +	};
> +	ephy2_spd@15 {
> +		reg = <15>;
> +		brcm,hardware-controlled;
> +	};
> +	power_green@20 {
> +		reg = <20>;
> +		active-low;
> +		label = "green:power";
> +		default-state = "on";
> +	};
> +};
>


-- 
Best Regards,
Jacek Anaszewski