diff mbox

[2/6] leds: add device tree bindings for syscon LEDs

Message ID 1406294628-16079-3-git-send-email-linus.walleij@linaro.org
State New
Headers show

Commit Message

Linus Walleij July 25, 2014, 1:23 p.m. UTC
This adds the device tree bindings used by syscon-based LEDs.

Cc: devicetree@vger.kernel.org
Cc: Bryan Wu <cooloney@gmail.com>
Cc: Richard Purdie <rpurdie@rpsys.net>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 .../devicetree/bindings/leds/leds-syscon.txt       | 83 ++++++++++++++++++++++
 1 file changed, 83 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-syscon.txt

Comments

Rob Herring (Arm) July 25, 2014, 2:07 p.m. UTC | #1
On Fri, Jul 25, 2014 at 8:23 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> This adds the device tree bindings used by syscon-based LEDs.
>
> Cc: devicetree@vger.kernel.org
> Cc: Bryan Wu <cooloney@gmail.com>
> Cc: Richard Purdie <rpurdie@rpsys.net>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  .../devicetree/bindings/leds/leds-syscon.txt       | 83 ++++++++++++++++++++++
>  1 file changed, 83 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-syscon.txt
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-syscon.txt b/Documentation/devicetree/bindings/leds/leds-syscon.txt
> new file mode 100644
> index 000000000000..460b9c3d1bd3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-syscon.txt
> @@ -0,0 +1,83 @@
> +Device Tree Bindings for Syscon LEDs
> +
> +Required properties:
> +- compatible : must be "syscon-leds".

Not really happy with the name... More below.

> +- regmap : a phandle to a syscon node containing a regmap
> +
> +Each LED is represented as a sub-node of the syscon-leds device. Each
> +node's name represents the name of the corresponding LED.
> +
> +LED sub-node properties:
> +- offset : register offset to the register controlling this LED
> +- mask : bit mask for the bit controlling this LED in the register
> +  typically 0x01, 0x02, 0x04 ...

This would be a single bit, right? What about inverted bits (i.e. 0 is
on or 1 is on)?

> +- label : (optional)

Please group all required and optional properties under those headings.

> +  see Documentation/devicetree/bindings/leds/common.txt
> +- linux,default-trigger : (optional)
> +  see Documentation/devicetree/bindings/leds/common.txt
> +- default-state: (optional) The initial state of the LED. Valid
> +  values are "on", "off", and "keep". If the LED is already on or off
> +  and the default-state property is set the to same value, then no
> +  glitch should be produced where the LED momentarily turns off (or
> +  on). The "keep" setting will keep the LED at whatever its current
> +  state is, without producing a glitch.  The default is off if this
> +  property is not present.
> +
> +Example:
> +
> +leds: leds@08 {
> +       compatible = "syscon-leds";
> +       regmap = <&syscon>;
> +
> +       led@bit0 {

Perhaps we can define a way to express unit address as offset+bit like
<offset>_<bit> or <offset>.<bit>.

I think we should get rid of the leds node and put this within the
syscon device node and each node here should have a compatible
property. I think the compatible should be something like
"register-bit-led" (perhaps someone has a better name) as syscon is
somewhat linux specific term and you could use this binding for any
LEDs that have a single register bit control.

Rob

> +               offset = <0x08>;
> +               mask = <0x01>;
> +               label = "versatile:0";
> +               linux,default-trigger = "heartbeat";
> +               default-state = "on";
> +       };
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Aug. 13, 2014, 9:31 a.m. UTC | #2
On Fri, Jul 25, 2014 at 4:07 PM, Rob Herring <robh@kernel.org> wrote:
> On Fri, Jul 25, 2014 at 8:23 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> This adds the device tree bindings used by syscon-based LEDs.
(...)
>> +Device Tree Bindings for Syscon LEDs
>> +
>> +Required properties:
>> +- compatible : must be "syscon-leds".
>
> Not really happy with the name... More below.

[copy paste]

> I think the compatible should be something like
> "register-bit-led" (perhaps someone has a better name) as syscon is
> somewhat linux specific term and you could use this binding for any
> LEDs that have a single register bit control.

Hm... this driver:
drivers/gpio/gpio-syscon.c

Has some bindings specific to a certain controller here:
Documentation/devicetree/bindings/gpio/cirrus,clps711x-mctrl-gpio.txt

Maybe I should think of something generic for that one
along the same lines then, so we get some consensus on
this.

Documentation/devicetree/bindings/mfd/syscon.txt
Specifies the string "syscon" for such regmap ranges, maybe
it should rather use the compatible string "misc-register-range"
or something.

>> +LED sub-node properties:
>> +- offset : register offset to the register controlling this LED
>> +- mask : bit mask for the bit controlling this LED in the register
>> +  typically 0x01, 0x02, 0x04 ...
>
> This would be a single bit, right? What about inverted bits (i.e. 0 is
> on or 1 is on)?

I can of course add inversion bindings, but cannot add it to
the driver as I can't test it. Just an "active-low;" string would
do I guess.

>> +- label : (optional)
>
> Please group all required and optional properties under those headings.

OK.

>> +Example:
>> +
>> +leds: leds@08 {
>> +       compatible = "syscon-leds";
>> +       regmap = <&syscon>;
>> +
>> +       led@bit0 {
>
> Perhaps we can define a way to express unit address as offset+bit like
> <offset>_<bit> or <offset>.<bit>.

I'll come up with something. We don't really have a place to
document such schemas do we?

> I think we should get rid of the leds node and put this within the
> syscon device node and each node here should have a compatible
> property.

OK. But then I guess you also expect to get rid of the
phandle <&syscon> from the node, so that it implicitly uses
the syscon it's embedded in? Or is it OK to keep? Or should
it be optional, so the (syscon) driver will look upward when
locating the syscon for a node?

Like I need to add a new function
syscon_regmap_lookup_by_hierarchy() to syscon.c...
It already has like three ways to look up syscons, now
we add another one.

I know that is a Linux implementation detail but it reflects the
fact that similar code will be needed in all operating systems
using syscon-type things, if it's a case-by-case question
whether a phandle or hierarchy traversal is to be used for
getting hold of the interface.

Your,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/leds/leds-syscon.txt b/Documentation/devicetree/bindings/leds/leds-syscon.txt
new file mode 100644
index 000000000000..460b9c3d1bd3
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-syscon.txt
@@ -0,0 +1,83 @@ 
+Device Tree Bindings for Syscon LEDs
+
+Required properties:
+- compatible : must be "syscon-leds".
+- regmap : a phandle to a syscon node containing a regmap
+
+Each LED is represented as a sub-node of the syscon-leds device. Each
+node's name represents the name of the corresponding LED.
+
+LED sub-node properties:
+- offset : register offset to the register controlling this LED
+- mask : bit mask for the bit controlling this LED in the register
+  typically 0x01, 0x02, 0x04 ...
+- label : (optional)
+  see Documentation/devicetree/bindings/leds/common.txt
+- linux,default-trigger : (optional)
+  see Documentation/devicetree/bindings/leds/common.txt
+- default-state: (optional) The initial state of the LED. Valid
+  values are "on", "off", and "keep". If the LED is already on or off
+  and the default-state property is set the to same value, then no
+  glitch should be produced where the LED momentarily turns off (or
+  on). The "keep" setting will keep the LED at whatever its current
+  state is, without producing a glitch.  The default is off if this
+  property is not present.
+
+Example:
+
+leds: leds@08 {
+	compatible = "syscon-leds";
+	regmap = <&syscon>;
+
+	led@bit0 {
+		offset = <0x08>;
+		mask = <0x01>;
+		label = "versatile:0";
+		linux,default-trigger = "heartbeat";
+		default-state = "on";
+	};
+	led@bit1 {
+		offset = <0x08>;
+		mask = <0x02>;
+		label = "versatile:1";
+		linux,default-trigger = "mmc0";
+		default-state = "off";
+	};
+	led@bit2 {
+		offset = <0x08>;
+		mask = <0x04>;
+		label = "versatile:2";
+		linux,default-trigger = "cpu0";
+		default-state = "off";
+	};
+	led@bit3 {
+		offset = <0x08>;
+		mask = <0x08>;
+		label = "versatile:3";
+		default-state = "off";
+	};
+	led@bit4 {
+		offset = <0x08>;
+		mask = <0x10>;
+		label = "versatile:4";
+		default-state = "off";
+	};
+	led@bit5 {
+		offset = <0x08>;
+		mask = <0x20>;
+		label = "versatile:5";
+		default-state = "off";
+	};
+	led@bit6 {
+		offset = <0x08>;
+		mask = <0x40>;
+		label = "versatile:6";
+		default-state = "off";
+	};
+	led@bit7 {
+		offset = <0x08>;
+		mask = <0x80>;
+		label = "versatile:7";
+		default-state = "off";
+	};
+};