| Message ID | 1409659354-23553-3-git-send-email-linus.walleij@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Tuesday 02 September 2014 14:02:28 Linus Walleij wrote: > +LED sub-node properties: > + > +Required properties: > +- compatible : must be "register-bit-led" > +- 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 ... > How does the driver know whether to do byte or word sized accesses? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-leds" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Linus, On Tue, Sep 2, 2014 at 2:02 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > +LED sub-node properties: > + > +Required properties: > +- compatible : must be "register-bit-led" > +- offset : register offset to the register controlling this LED > +- mask : bit mask for the bit controlling this LED in the register Why don't you use a "reg" property with "#address-cells = <2>" and "#size-cells = <1>", so you can store offset and mask there? > +syscon: syscon@10000000 { > + compatible = "arm,realview-pb1176-syscon", "syscon"; > + reg = <0x10000000 0x1000>; > + > + led@08.0 { > + compatible = "register-bit-led"; > + offset = <0x08>; > + mask = <0x01>; > + label = "versatile:0"; > + linux,default-trigger = "heartbeat"; > + default-state = "on"; > + }; ePAPR v1.1 says: "The unit-address must match the first address specified in the reg property of the node. If the node has no reg property, the @ and unit-address must be omitted ..." So you cannot have the "@8.0" without a "reg" property. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- 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
On Tue, Sep 2, 2014 at 4:22 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Tue, Sep 2, 2014 at 2:02 PM, Linus Walleij <linus.walleij@linaro.org> wrote: >> +LED sub-node properties: >> + >> +Required properties: >> +- compatible : must be "register-bit-led" >> +- offset : register offset to the register controlling this LED >> +- mask : bit mask for the bit controlling this LED in the register > > Why don't you use a "reg" property with "#address-cells = <2>" and > "#size-cells = <1>", so you can store offset and mask there? Because "reg" means a register range not based off another range, i.e. not relative, and the OF cores does not allow overlapping reg ranges as would be the case here IIRC. >> +syscon: syscon@10000000 { >> + compatible = "arm,realview-pb1176-syscon", "syscon"; >> + reg = <0x10000000 0x1000>; >> + >> + led@08.0 { >> + compatible = "register-bit-led"; >> + offset = <0x08>; >> + mask = <0x01>; >> + label = "versatile:0"; >> + linux,default-trigger = "heartbeat"; >> + default-state = "on"; >> + }; > > ePAPR v1.1 says: > > "The unit-address must match the first address specified in the reg property > of the node. If the node has no reg property, the @ and unit-address must > be omitted ..." > > So you cannot have the "@8.0" without a "reg" property. This terminology was suggested by Rob Herring due to the fact that there were no previous examples. Rob: care to comment? Right now I have that distinct feeling of despair as the v8 patch set is still stuck in DT syntax discussions... Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Sep 2, 2014 at 2:24 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tuesday 02 September 2014 14:02:28 Linus Walleij wrote: >> +LED sub-node properties: >> + >> +Required properties: >> +- compatible : must be "register-bit-led" >> +- 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 ... >> > > How does the driver know whether to do byte or word sized accesses? Like with all other bindings based on Documentation/devicetree/bindings/mfd/syscon.txt it is implicit that the access size is 32 bit words. Just grep for syscon in the bindings and you will see none define the access width. If this should be fixed, the parent syscon binding should be altered to cover access width for all children as this is a property of the entire register range. So for example: gpr: iomuxc-gpr@020e0000 { compatible = "fsl,imx6q-iomuxc-gpr", "syscon"; reg = <0x020e0000 0x38>; register-width = <32>; register-valid = <32>; register-stride = <4>; }; So as to indicate that this register range is made up of 32 bit words, all 32 bits are valid in each word, and the stride between words i 4 bytes (i.e. tightly packed). As mentioned by Rob, the "syscon" driver should be renamed "register-range" and "syscon" deprecated as binding, as it is Linux terminology. I guess this adds up to the conclusion that the "syscon" binding was badly reviewed and merged, and now we're suffering from it. ---- Not that anyone should care when reviewing device tree bindings, but as it happens, the Linux driver drivers/mfd/syscon.c explicitly assumes all sycons are 32bit: static struct regmap_config syscon_regmap_config = { .reg_bits = 32, .val_bits = 32, .reg_stride = 4, }; If one happens to work on Linux (we would never assume such a thing, but if one by infinitesimal chance would still happen to do that) we can leave a note in this posting that the syscon driver would need to be altered to marshall it's regmap differently on stumbling onto this new property. Yours, 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 --git a/Documentation/devicetree/bindings/leds/register-bit-led.txt b/Documentation/devicetree/bindings/leds/register-bit-led.txt new file mode 100644 index 000000000000..379cefdc0bda --- /dev/null +++ b/Documentation/devicetree/bindings/leds/register-bit-led.txt @@ -0,0 +1,99 @@ +Device Tree Bindings for Register Bit LEDs + +Register bit leds are used with syscon multifunctional devices +where single bits in a certain register can turn on/off a +single LED. The register bit LEDs appear as children to the +syscon device, with the proper compatible string. For the +syscon bindings see: +Documentation/devicetree/bindings/mfd/syscon.txt + +Each LED is represented as a sub-node of the syscon device. Each +node's name represents the name of the corresponding LED. + +LED sub-node properties: + +Required properties: +- compatible : must be "register-bit-led" +- 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 ... + +Optional properties: +- 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: + +syscon: syscon@10000000 { + compatible = "arm,realview-pb1176-syscon", "syscon"; + reg = <0x10000000 0x1000>; + + led@08.0 { + compatible = "register-bit-led"; + offset = <0x08>; + mask = <0x01>; + label = "versatile:0"; + linux,default-trigger = "heartbeat"; + default-state = "on"; + }; + led@08.1 { + compatible = "register-bit-led"; + offset = <0x08>; + mask = <0x02>; + label = "versatile:1"; + linux,default-trigger = "mmc0"; + default-state = "off"; + }; + led@08.2 { + compatible = "register-bit-led"; + offset = <0x08>; + mask = <0x04>; + label = "versatile:2"; + linux,default-trigger = "cpu0"; + default-state = "off"; + }; + led@08.3 { + compatible = "register-bit-led"; + offset = <0x08>; + mask = <0x08>; + label = "versatile:3"; + default-state = "off"; + }; + led@08.4 { + compatible = "register-bit-led"; + offset = <0x08>; + mask = <0x10>; + label = "versatile:4"; + default-state = "off"; + }; + led@08.5 { + compatible = "register-bit-led"; + offset = <0x08>; + mask = <0x20>; + label = "versatile:5"; + default-state = "off"; + }; + led@08.6 { + compatible = "register-bit-led"; + offset = <0x08>; + mask = <0x40>; + label = "versatile:6"; + default-state = "off"; + }; + led@08.7 { + compatible = "register-bit-led"; + offset = <0x08>; + mask = <0x80>; + label = "versatile:7"; + default-state = "off"; + }; +};
This adds the device tree bindings used by register bit 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> --- ChangeLog v1->v2: - Rename binding "register-bit-led" - Define that such nodes appear only as children of syscon - Group required and optional parameters - Number nodes as foo@<offset>.<bit> - Update example --- .../devicetree/bindings/leds/register-bit-led.txt | 99 ++++++++++++++++++++++ 1 file changed, 99 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/register-bit-led.txt