[2/2] leds: meter: add leds-meter binding

Add DT binding for leds-meter.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Bjorn Andersson <bjorn@kryo.se>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Jean-Jacques Hiblot <jjhiblot@ti.com>
Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Dan Murphy <dmurphy@ti.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 .../devicetree/bindings/leds/leds-meter.yaml       | 42 ++++++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-meter.yaml

Akinobu

On 10/22/19 10:06 AM, Akinobu Mita wrote:
> Add DT binding for leds-meter.

$subject should be

  dt-bindings: leds: Add leds-meter binding

And this patch should be first in the series

> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Bjorn Andersson <bjorn@kryo.se>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Jean-Jacques Hiblot <jjhiblot@ti.com>
> Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Dan Murphy <dmurphy@ti.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>   .../devicetree/bindings/leds/leds-meter.yaml       | 42 ++++++++++++++++++++++
>   1 file changed, 42 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/leds/leds-meter.yaml
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-meter.yaml b/Documentation/devicetree/bindings/leds/leds-meter.yaml
> new file mode 100644
> index 0000000..d5dfa261
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-meter.yaml
> @@ -0,0 +1,42 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/leds-meter.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Generic LED level meter
> +
> +maintainers:
> +  - Akinobu Mita <akinobu.mita@gmail.com>
> +
> +description:
> +  Generic LED level meter consists of multiple LED devices by different drivers.
> +
> +properties:
> +  compatible:
> +    const: meter-leds
> +
> +  leds:

This seems a bit generic for the property name

> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    minItems: 1
> +    description: List of phandles to LED node that are members of a level meter.
> +
> +  brightness-weights:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 1
> +    description: Each integer represents a contribution ratio within a level
> +      meter.

This description is a bit vague I would not be sure what I would set 
this to.

Dan

> +
> +required:
> + - compatible
> + - leds
> +
> +examples:
> +  - |
> +    leds {
> +        compatible = "meter-leds";
> +        leds = <&led0>, <&led1>, <&led2>, <&led3>;
> +        brightness-weights = <3 1 1 1>;
> +    };
> +
> +...

2019年10月23日(水) 21:35 Dan Murphy <dmurphy@ti.com>:
>
> Akinobu
>
> On 10/22/19 10:06 AM, Akinobu Mita wrote:
> > Add DT binding for leds-meter.
>
> $subject should be
>
>   dt-bindings: leds: Add leds-meter binding
>
> And this patch should be first in the series

OK.

> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Bjorn Andersson <bjorn@kryo.se>
> > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > Cc: Jean-Jacques Hiblot <jjhiblot@ti.com>
> > Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> > Cc: Pavel Machek <pavel@ucw.cz>
> > Cc: Dan Murphy <dmurphy@ti.com>
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > ---
> >   .../devicetree/bindings/leds/leds-meter.yaml       | 42 ++++++++++++++++++++++
> >   1 file changed, 42 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/leds/leds-meter.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/leds/leds-meter.yaml b/Documentation/devicetree/bindings/leds/leds-meter.yaml
> > new file mode 100644
> > index 0000000..d5dfa261
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/leds/leds-meter.yaml
> > @@ -0,0 +1,42 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/leds/leds-meter.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Generic LED level meter
> > +
> > +maintainers:
> > +  - Akinobu Mita <akinobu.mita@gmail.com>
> > +
> > +description:
> > +  Generic LED level meter consists of multiple LED devices by different drivers.
> > +
> > +properties:
> > +  compatible:
> > +    const: meter-leds
> > +
> > +  leds:
>
> This seems a bit generic for the property name

I have no problem with changing to more descriptive name like "meter-leds".
But this property is parsed by of_led_get(), so we need to change of_led_get()
to take additional 'prefix' parameter.  I would like to hear led-backlight
authors' opinion.

> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +    minItems: 1
> > +    description: List of phandles to LED node that are members of a level meter.
> > +
> > +  brightness-weights:
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    minItems: 1
> > +    description: Each integer represents a contribution ratio within a level
> > +      meter.
>
> This description is a bit vague I would not be sure what I would set
> this to.

OK.  I'll add some description and examples like I wrote in the commit
message for patch 1/2.

On Tue, Oct 22, 2019 at 10:07 AM Akinobu Mita <akinobu.mita@gmail.com> wrote:
>
> Add DT binding for leds-meter.

What's an leds meter? Need a better explanation to understand if this
makes sense at all, but some comments on the schema below.

> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Bjorn Andersson <bjorn@kryo.se>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Jean-Jacques Hiblot <jjhiblot@ti.com>
> Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Dan Murphy <dmurphy@ti.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
>  .../devicetree/bindings/leds/leds-meter.yaml       | 42 ++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-meter.yaml
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-meter.yaml b/Documentation/devicetree/bindings/leds/leds-meter.yaml
> new file mode 100644
> index 0000000..d5dfa261
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-meter.yaml
> @@ -0,0 +1,42 @@
> +# SPDX-License-Identifier: GPL-2.0

(GPL-2.0-only OR BSD-2-Clause) for new bindings.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/leds-meter.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Generic LED level meter
> +
> +maintainers:
> +  - Akinobu Mita <akinobu.mita@gmail.com>
> +
> +description:
> +  Generic LED level meter consists of multiple LED devices by different drivers.
> +
> +properties:
> +  compatible:
> +    const: meter-leds
> +
> +  leds:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    minItems: 1

No need for this as the minimum for arrays is already 1. And it
doesn't work either. You'd need the $ref under an 'allOf'.

> +    description: List of phandles to LED node that are members of a level meter.
> +
> +  brightness-weights:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 1
> +    description: Each integer represents a contribution ratio within a level
> +      meter.
> +
> +required:
> + - compatible
> + - leds

Add a:

additionalProperties: false

> +
> +examples:
> +  - |
> +    leds {

Needs to be a name that's not also a property name. 'leds-meter' or
'meter-leds' perhaps. And define the name above under a $nodename
property.

> +        compatible = "meter-leds";
> +        leds = <&led0>, <&led1>, <&led2>, <&led3>;
> +        brightness-weights = <3 1 1 1>;
> +    };
> +
> +...
> --
> 2.7.4
>

On Wed 2019-10-23 09:56:03, Rob Herring wrote:
> On Tue, Oct 22, 2019 at 10:07 AM Akinobu Mita <akinobu.mita@gmail.com> wrote:
> >
> > Add DT binding for leds-meter.
> 
> What's an leds meter? Need a better explanation to understand if this
> makes sense at all, but some comments on the schema below.

It groups several LEDs into one "virtual" LED.

I'm not sure I like it. What is it good for?

We do not have many triggers that dim the LEDs, and if it is only used
from userspace, it can stay in userspace...?

Best regards,
									Pavel