[1/2] dt-bindings: leds: Mention GPIO triggers

  We reuse the trigger-sources phandle to just point to
GPIOs we may want to use as LED triggers.

Example:

gpio: gpio@0 {
    compatible "my-gpio";
    gpio-controller;
    #gpio-cells = <2>;
    interrupt-controller;
    #interrupt-cells = <2>;
    #trigger-source-cells = <2>;
};

leds {
    compatible = "gpio-leds";
    led-my-gpio {
        label = "device:blue:myled";
        gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
        default-state = "off";
        linux,default-trigger = "gpio";
        trigger-sources = <&gpio 1 GPIO_ACTIVE_HIGH>;
    };
};

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 Documentation/devicetree/bindings/leds/common.yaml | 2 ++
 1 file changed, 2 insertions(+)
  

On Tue, Sep 12, 2023 at 03:44:30PM +0200, Linus Walleij wrote:
> We reuse the trigger-sources phandle to just point to
> GPIOs we may want to use as LED triggers.
> 
> Example:
> 
> gpio: gpio@0 {
>     compatible "my-gpio";
>     gpio-controller;
>     #gpio-cells = <2>;
>     interrupt-controller;
>     #interrupt-cells = <2>;
>     #trigger-source-cells = <2>;

BTW, this is not documented for any GPIO binding. If we want to specify 
the cell size, then it has to be added to every GPIO controller binding. 
If not, we then need to reference gpio.yaml in every GPIO controller 
binding (along with unevaluatedProperties). Doesn't have to be done for 
this patch to go in though.

> };
> 
> leds {
>     compatible = "gpio-leds";
>     led-my-gpio {
>         label = "device:blue:myled";
>         gpios = <&gpio 0 GPIO_ACTIVE_HIGH>;
>         default-state = "off";
>         linux,default-trigger = "gpio";
>         trigger-sources = <&gpio 1 GPIO_ACTIVE_HIGH>;
>     };
> };
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  Documentation/devicetree/bindings/leds/common.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/leds/common.yaml b/Documentation/devicetree/bindings/leds/common.yaml
> index 5fb7007f3618..b42950643b9d 100644
> --- a/Documentation/devicetree/bindings/leds/common.yaml
> +++ b/Documentation/devicetree/bindings/leds/common.yaml
> @@ -191,6 +191,8 @@ properties:
>        each of them having its own LED assigned (assuming they are not
>        hardwired). In such cases this property should contain phandle(s) of
>        related source device(s).
> +      Another example is a GPIO line that will be monitored and mirror the
> +      state of the line (with or without inversion flags) to the LED.
>        In many cases LED can be related to more than one device (e.g. one USB LED
>        vs. multiple USB ports). Each source should be represented by a node in
>        the device tree and be referenced by a phandle and a set of phandle
> 
> -- 
> 2.34.1
>
  

On Wed, Sep 13, 2023 at 3:34 PM Rob Herring <robh@kernel.org> wrote:
> On Tue, Sep 12, 2023 at 03:44:30PM +0200, Linus Walleij wrote:
> > We reuse the trigger-sources phandle to just point to
> > GPIOs we may want to use as LED triggers.
> >
> > Example:
> >
> > gpio: gpio@0 {
> >     compatible "my-gpio";
> >     gpio-controller;
> >     #gpio-cells = <2>;
> >     interrupt-controller;
> >     #interrupt-cells = <2>;
> >     #trigger-source-cells = <2>;
>
> BTW, this is not documented for any GPIO binding. If we want to specify
> the cell size, then it has to be added to every GPIO controller binding.
> If not, we then need to reference gpio.yaml in every GPIO controller
> binding (along with unevaluatedProperties). Doesn't have to be done for
> this patch to go in though.

Yeah I mean this trigger-sources = <...>; one-size-fits-all is a bit
weird in a way.

My other idea was to simply add trigger-gpios to the normal way
and be done with it, but now the trigger binding has this weird
thing.

Would trigger-gpios be better?

Yours,
Linus Walleij
  

On Thu, Sep 14, 2023 at 3:40 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Wed, Sep 13, 2023 at 3:34 PM Rob Herring <robh@kernel.org> wrote:
> > On Tue, Sep 12, 2023 at 03:44:30PM +0200, Linus Walleij wrote:
> > > We reuse the trigger-sources phandle to just point to
> > > GPIOs we may want to use as LED triggers.
> > >
> > > Example:
> > >
> > > gpio: gpio@0 {
> > >     compatible "my-gpio";
> > >     gpio-controller;
> > >     #gpio-cells = <2>;
> > >     interrupt-controller;
> > >     #interrupt-cells = <2>;
> > >     #trigger-source-cells = <2>;
> >
> > BTW, this is not documented for any GPIO binding. If we want to specify
> > the cell size, then it has to be added to every GPIO controller binding.
> > If not, we then need to reference gpio.yaml in every GPIO controller
> > binding (along with unevaluatedProperties). Doesn't have to be done for
> > this patch to go in though.
>
> Yeah I mean this trigger-sources = <...>; one-size-fits-all is a bit
> weird in a way.
>
> My other idea was to simply add trigger-gpios to the normal way
> and be done with it, but now the trigger binding has this weird
> thing.
>
> Would trigger-gpios be better?

Then GPIOs are different than everyone else. I think we have to think
about other bindings too. While we could standardize the naming here
with trigger-gpios, that won't work with the foos/foo-names style of
bindings.

trigger-sources is not widely used as it is just USB ATM and a few
platforms. We could come up with something different.
"trigger-sources-<cellname>" is the only idea I have. Then the
property name gives you the cell name to read. But variable property
names have their own challenges. We would need to look at all the
current trigger sources (i.e. the linux,default-trigger ones) and see
what else might need this.

Rob
  

On Thu, Sep 14, 2023 at 4:27 PM Rob Herring <robh@kernel.org> wrote:
> On Thu, Sep 14, 2023 at 3:40 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Wed, Sep 13, 2023 at 3:34 PM Rob Herring <robh@kernel.org> wrote:
> > > On Tue, Sep 12, 2023 at 03:44:30PM +0200, Linus Walleij wrote:
> > > > We reuse the trigger-sources phandle to just point to
> > > > GPIOs we may want to use as LED triggers.
> > > >
> > > > Example:
> > > >
> > > > gpio: gpio@0 {
> > > >     compatible "my-gpio";
> > > >     gpio-controller;
> > > >     #gpio-cells = <2>;
> > > >     interrupt-controller;
> > > >     #interrupt-cells = <2>;
> > > >     #trigger-source-cells = <2>;
> > >
> > > BTW, this is not documented for any GPIO binding. If we want to specify
> > > the cell size, then it has to be added to every GPIO controller binding.
> > > If not, we then need to reference gpio.yaml in every GPIO controller
> > > binding (along with unevaluatedProperties). Doesn't have to be done for
> > > this patch to go in though.
> >
> > Yeah I mean this trigger-sources = <...>; one-size-fits-all is a bit
> > weird in a way.
> >
> > My other idea was to simply add trigger-gpios to the normal way
> > and be done with it, but now the trigger binding has this weird
> > thing.
> >
> > Would trigger-gpios be better?
>
> Then GPIOs are different than everyone else. I think we have to think
> about other bindings too. While we could standardize the naming here
> with trigger-gpios, that won't work with the foos/foo-names style of
> bindings.
>
> trigger-sources is not widely used as it is just USB ATM and a few
> platforms. We could come up with something different.
> "trigger-sources-<cellname>" is the only idea I have. Then the
> property name gives you the cell name to read. But variable property
> names have their own challenges. We would need to look at all the
> current trigger sources (i.e. the linux,default-trigger ones) and see
> what else might need this.

I think it in a way is elegant with the trigger-sources phandle as it
is so I would stick with this.

I can just add '#trigger-source-cells' to the existing GPIO
bindings and it's a bit tedious since we don't have a common file
for the GPIO chip stuff, but it's just lots of lines.

I guess it would be better to break out gpio-common.yaml and
gpio-common-irq.yaml for GPIO controllers with or without
interrupt support and then add '#trigger-source-cells' to just
those supporting IRQs because I think only that makes sense,
polling for a line to change isn't quite a "trigger".

Yours,
Linus Walleij
  

On Fri, Sep 15, 2023 at 7:01 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Thu, Sep 14, 2023 at 4:27 PM Rob Herring <robh@kernel.org> wrote:
> > On Thu, Sep 14, 2023 at 3:40 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > > On Wed, Sep 13, 2023 at 3:34 PM Rob Herring <robh@kernel.org> wrote:
> > > > On Tue, Sep 12, 2023 at 03:44:30PM +0200, Linus Walleij wrote:
> > > > > We reuse the trigger-sources phandle to just point to
> > > > > GPIOs we may want to use as LED triggers.
> > > > >
> > > > > Example:
> > > > >
> > > > > gpio: gpio@0 {
> > > > >     compatible "my-gpio";
> > > > >     gpio-controller;
> > > > >     #gpio-cells = <2>;
> > > > >     interrupt-controller;
> > > > >     #interrupt-cells = <2>;
> > > > >     #trigger-source-cells = <2>;
> > > >
> > > > BTW, this is not documented for any GPIO binding. If we want to specify
> > > > the cell size, then it has to be added to every GPIO controller binding.
> > > > If not, we then need to reference gpio.yaml in every GPIO controller
> > > > binding (along with unevaluatedProperties). Doesn't have to be done for
> > > > this patch to go in though.
> > >
> > > Yeah I mean this trigger-sources = <...>; one-size-fits-all is a bit
> > > weird in a way.
> > >
> > > My other idea was to simply add trigger-gpios to the normal way
> > > and be done with it, but now the trigger binding has this weird
> > > thing.
> > >
> > > Would trigger-gpios be better?
> >
> > Then GPIOs are different than everyone else. I think we have to think
> > about other bindings too. While we could standardize the naming here
> > with trigger-gpios, that won't work with the foos/foo-names style of
> > bindings.
> >
> > trigger-sources is not widely used as it is just USB ATM and a few
> > platforms. We could come up with something different.
> > "trigger-sources-<cellname>" is the only idea I have. Then the
> > property name gives you the cell name to read. But variable property
> > names have their own challenges. We would need to look at all the
> > current trigger sources (i.e. the linux,default-trigger ones) and see
> > what else might need this.
>
> I think it in a way is elegant with the trigger-sources phandle as it
> is so I would stick with this.
>
> I can just add '#trigger-source-cells' to the existing GPIO
> bindings and it's a bit tedious since we don't have a common file
> for the GPIO chip stuff, but it's just lots of lines.

We do, gpio.yaml in dtschema. We just didn't reference it in all
controllers because at the time unevaluatedProperties didn't
exist/work and you still have to list most properties to define their
constraints. In general, we reference the common schema for ones with
child nodes, but not ones which are just #foo-cells and other
properties. For GPIO, it's evolved to the point that referencing the
common schema makes sense mainly because we have the hog nodes now.

I said before we still need '#trigger-source-cells' in each schema,
but really I suppose if we just set it to 1 or 2 cells in the common
schema, that would be good enough. It's so rarely used. I expect
(because there's discussions on it) someday jsonschema will support
data dependent schemas (i.e. if the value of prop is X, then apply
schema). Actually, we can do that already with if/then schemas, but it
would be kind of verbose.

> I guess it would be better to break out gpio-common.yaml and
> gpio-common-irq.yaml for GPIO controllers with or without
> interrupt support and then add '#trigger-source-cells' to just
> those supporting IRQs because I think only that makes sense,
> polling for a line to change isn't quite a "trigger".

No need to split them just for that:

dependencies:
  '#trigger-source-cells': interrupt-controller

Though maybe a split makes sense anyways.

Rob