| Message ID | 20220706112828.27278-1-pali@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] dt-bindings: leds: register-bit-led: Add value property | expand |
On Wed, Jul 6, 2022 at 6:23 PM Pali Rohár <pali@kernel.org> wrote: > On Wednesday 06 July 2022 10:21:11 Rob Herring wrote: > > On Wed, Jul 06, 2022 at 01:28:27PM +0200, Pali Rohár wrote: > > > Allow to define inverted logic (0 - enable LED, 1 - disable LED) via value > > > property. This property name is already used by other syscon drivers, e.g. > > > syscon-reboot. > > > > Yes, but those are potentially multi-bit values. This is a single bit > > value, and the only value that's ever needed is 0. Why not just use > > 'active-low' here? > > Just because to have uniform definitions across more syscon nodes. But what happens if he mask and value don't line up? mask = 0x10; value = 0x08; If you just state active-low; this kind of mistake is not possible to make. So I'd rather go for active-low; Yours, Linus Walleij
On Monday 11 July 2022 14:06:50 Linus Walleij wrote: > On Wed, Jul 6, 2022 at 6:23 PM Pali Rohár <pali@kernel.org> wrote: > > On Wednesday 06 July 2022 10:21:11 Rob Herring wrote: > > > On Wed, Jul 06, 2022 at 01:28:27PM +0200, Pali Rohár wrote: > > > > Allow to define inverted logic (0 - enable LED, 1 - disable LED) via value > > > > property. This property name is already used by other syscon drivers, e.g. > > > > syscon-reboot. > > > > > > Yes, but those are potentially multi-bit values. This is a single bit > > > value, and the only value that's ever needed is 0. Why not just use > > > 'active-low' here? > > > > Just because to have uniform definitions across more syscon nodes. > > But what happens if he mask and value don't line up? > > mask = 0x10; > value = 0x08; Same what would happen in other drivers, no? Only those value bits are take into account which are also sets in the mask. > If you just state active-low; this kind of mistake is not possible to make. > > So I'd rather go for active-low; > > Yours, > Linus Walleij
On Mon, Jul 11, 2022 at 2:10 PM Pali Rohár <pali@kernel.org> wrote: > On Monday 11 July 2022 14:06:50 Linus Walleij wrote: > > On Wed, Jul 6, 2022 at 6:23 PM Pali Rohár <pali@kernel.org> wrote: > > > On Wednesday 06 July 2022 10:21:11 Rob Herring wrote: > > > > On Wed, Jul 06, 2022 at 01:28:27PM +0200, Pali Rohár wrote: > > > > > Allow to define inverted logic (0 - enable LED, 1 - disable LED) via value > > > > > property. This property name is already used by other syscon drivers, e.g. > > > > > syscon-reboot. > > > > > > > > Yes, but those are potentially multi-bit values. This is a single bit > > > > value, and the only value that's ever needed is 0. Why not just use > > > > 'active-low' here? > > > > > > Just because to have uniform definitions across more syscon nodes. > > > > But what happens if he mask and value don't line up? > > > > mask = 0x10; > > value = 0x08; > > Same what would happen in other drivers, no? > > Only those value bits are take into account which are also sets in the mask. Two wrongs does not make one right. I.e. just because this bad pattern is used in other syscon drivers we do not need to repeat their mistakes. Also, in this case we can only specify one bit due to the way the enum is designed, other drivers may specify multiple bits. This also becomes a bit confusing: + enum: + [ 0x0, What you do is add 0 at the beginning of the enum but that represent a low bit of any from 0-31. I just get confused, active-low is crystal clear. Yours, Linus Walleij
diff --git a/Documentation/devicetree/bindings/leds/register-bit-led.yaml b/Documentation/devicetree/bindings/leds/register-bit-led.yaml index 79b8fc0f9d23..d6054a3f9087 100644 --- a/Documentation/devicetree/bindings/leds/register-bit-led.yaml +++ b/Documentation/devicetree/bindings/leds/register-bit-led.yaml @@ -43,6 +43,17 @@ properties: 0x100000, 0x200000, 0x400000, 0x800000, 0x1000000, 0x2000000, 0x4000000, 0x8000000, 0x10000000, 0x20000000, 0x40000000, 0x80000000 ] + value: + description: + bit value of ON state for the bit controlling this LED in the register + when not specified it is same as the mask + $ref: /schemas/types.yaml#/definitions/uint32 + enum: + [ 0x0, 0x1, 0x2, 0x4, 0x8, 0x10, 0x20, 0x40, 0x80, 0x100, 0x200, 0x400, 0x800, + 0x1000, 0x2000, 0x4000, 0x8000, 0x10000, 0x20000, 0x40000, 0x80000, + 0x100000, 0x200000, 0x400000, 0x800000, 0x1000000, 0x2000000, 0x4000000, + 0x8000000, 0x10000000, 0x20000000, 0x40000000, 0x80000000 ] + offset: description: register offset to the register controlling this LED
Allow to define inverted logic (0 - enable LED, 1 - disable LED) via value property. This property name is already used by other syscon drivers, e.g. syscon-reboot. Signed-off-by: Pali Rohár <pali@kernel.org> --- .../devicetree/bindings/leds/register-bit-led.yaml | 11 +++++++++++ 1 file changed, 11 insertions(+)