[net-next,v5,10/15] dt-bindings: net: ethernet-controller: Document support for LEDs node

  Document support for LEDs node in ethernet-controller.
Ethernet Controller may support different LEDs that can be configured
for different operation like blinking on traffic event or port link.

Also add some Documentation to describe the difference of these nodes
compared to PHY LEDs, since ethernet-controller LEDs are controllable
by the ethernet controller regs and the possible intergated PHY doesn't
have control on them.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 .../bindings/net/ethernet-controller.yaml     | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)
  

On Sun, Mar 19, 2023 at 08:18:09PM +0100, Christian Marangi wrote:
> Document support for LEDs node in ethernet-controller.
> Ethernet Controller may support different LEDs that can be configured
> for different operation like blinking on traffic event or port link.
> 
> Also add some Documentation to describe the difference of these nodes
> compared to PHY LEDs, since ethernet-controller LEDs are controllable
> by the ethernet controller regs and the possible intergated PHY doesn't
> have control on them.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  .../bindings/net/ethernet-controller.yaml     | 21 +++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> index 00be387984ac..a93673592314 100644
> --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> @@ -222,6 +222,27 @@ properties:
>          required:
>            - speed
>  
> +  leds:
> +    type: object
> +    description:
> +      Describes the LEDs associated by Ethernet Controller.
> +      These LEDs are not integrated in the PHY and PHY doesn't have any
> +      control on them. Ethernet Controller regs are used to control
> +      these defined LEDs.
> +
> +    properties:
> +      '#address-cells':
> +        const: 1
> +
> +      '#size-cells':
> +        const: 0
> +
> +    patternProperties:
> +      '^led(@[a-f0-9]+)?$':
> +        $ref: /schemas/leds/common.yaml#

Are specific ethernet controllers allowed to add their own properties in 
led nodes? If so, this doesn't work. As-is, this allows any other 
properties. You need 'unevaluatedProperties: false' here to prevent 
that. But then no one can add properties. If you want to support that, 
then you need this to be a separate schema that devices can optionally 
include if they don't extend the properties, and then devices that 
extend the binding would essentially have the above with:

$ref: /schemas/leds/common.yaml#
unevaluatedProperties: false
properties:
  a-custom-device-prop: ...


If you wanted to define both common ethernet LED properties and 
device specific properties, then you'd need to replace leds/common.yaml 
above  with the ethernet one.

This is all the same reasons the DSA/switch stuff and graph bindings are 
structured the way they are.

Rob
  

On Tue, Mar 21, 2023 at 04:19:53PM -0500, Rob Herring wrote:
> On Sun, Mar 19, 2023 at 08:18:09PM +0100, Christian Marangi wrote:
> > Document support for LEDs node in ethernet-controller.
> > Ethernet Controller may support different LEDs that can be configured
> > for different operation like blinking on traffic event or port link.
> > 
> > Also add some Documentation to describe the difference of these nodes
> > compared to PHY LEDs, since ethernet-controller LEDs are controllable
> > by the ethernet controller regs and the possible intergated PHY doesn't
> > have control on them.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  .../bindings/net/ethernet-controller.yaml     | 21 +++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > index 00be387984ac..a93673592314 100644
> > --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > @@ -222,6 +222,27 @@ properties:
> >          required:
> >            - speed
> >  
> > +  leds:
> > +    type: object
> > +    description:
> > +      Describes the LEDs associated by Ethernet Controller.
> > +      These LEDs are not integrated in the PHY and PHY doesn't have any
> > +      control on them. Ethernet Controller regs are used to control
> > +      these defined LEDs.
> > +
> > +    properties:
> > +      '#address-cells':
> > +        const: 1
> > +
> > +      '#size-cells':
> > +        const: 0
> > +
> > +    patternProperties:
> > +      '^led(@[a-f0-9]+)?$':
> > +        $ref: /schemas/leds/common.yaml#
> 
> Are specific ethernet controllers allowed to add their own properties in 
> led nodes? If so, this doesn't work. As-is, this allows any other 
> properties. You need 'unevaluatedProperties: false' here to prevent 
> that. But then no one can add properties. If you want to support that, 
> then you need this to be a separate schema that devices can optionally 
> include if they don't extend the properties, and then devices that 
> extend the binding would essentially have the above with:
> 
> $ref: /schemas/leds/common.yaml#
> unevaluatedProperties: false
> properties:
>   a-custom-device-prop: ...
> 
> 
> If you wanted to define both common ethernet LED properties and 
> device specific properties, then you'd need to replace leds/common.yaml 
> above  with the ethernet one.
> 
> This is all the same reasons the DSA/switch stuff and graph bindings are 
> structured the way they are.
> 

Hi Rob, thanks for the review/questions.

The idea of all of this is to keep leds node as standard as possible.
It was asked to add unevaluatedProperties: False but I didn't understood
it was needed also for the led nodes.

leds/common.yaml have additionalProperties set to true but I guess that
is not OK for the final schema and we need something more specific.

Looking at the common.yaml schema reg binding is missing so an
additional schema is needed.

Reg is needed for ethernet LEDs and PHY but I think we should also permit
to skip that if the device actually have just one LED. (if this wouldn't
complicate the implementation. Maybe some hints from Andrew about this
decision?)

If we decide that reg is a must, if I understood it correctly we should
create something like leds-ethernet.yaml that would reference common and
add reg binding? Is it correct? This schema should be laded in leds
directory and not in the net/ethernet.

Also with setting reg mandatory I will have to fix the regex to require
@ in the node name.


Also also if we decide for a more specific schema, I guess I can
reference that directly in ethernet-phy.yaml and ethernet-controller.yaml
with something like:

leds:
  $ref: /schemas/leds/leds-ethernet.yaml#

Again thanks for the review and hope you can give some
hint/clarification if I got everything right.
  

> > Are specific ethernet controllers allowed to add their own properties in 
> > led nodes? If so, this doesn't work. As-is, this allows any other 
> > properties. You need 'unevaluatedProperties: false' here to prevent 
> > that. But then no one can add properties. If you want to support that, 
> > then you need this to be a separate schema that devices can optionally 
> > include if they don't extend the properties, and then devices that 
> > extend the binding would essentially have the above with:
> > 
> > $ref: /schemas/leds/common.yaml#
> > unevaluatedProperties: false
> > properties:
> >   a-custom-device-prop: ...
> > 
> > 
> > If you wanted to define both common ethernet LED properties and 
> > device specific properties, then you'd need to replace leds/common.yaml 
> > above  with the ethernet one.
> > 
> > This is all the same reasons the DSA/switch stuff and graph bindings are 
> > structured the way they are.
> > 
> 
> Hi Rob, thanks for the review/questions.
> 
> The idea of all of this is to keep leds node as standard as possible.
> It was asked to add unevaluatedProperties: False but I didn't understood
> it was needed also for the led nodes.
> 
> leds/common.yaml have additionalProperties set to true but I guess that
> is not OK for the final schema and we need something more specific.
> 
> Looking at the common.yaml schema reg binding is missing so an
> additional schema is needed.
> 
> Reg is needed for ethernet LEDs and PHY but I think we should also permit
> to skip that if the device actually have just one LED. (if this wouldn't
> complicate the implementation. Maybe some hints from Andrew about this
> decision?)

I would make reg mandatory.

We should not encourage additional properties, but i also think we
cannot block it.

The problem we have is that there is absolutely no standardisation
here. Vendors are free to do whatever they want, and they do. So i
would not be too surprised if some vendor properties are needed
eventually.

	Andrew
  

On Wed, Mar 22, 2023 at 12:23:59AM +0100, Andrew Lunn wrote:
> > > Are specific ethernet controllers allowed to add their own properties in 
> > > led nodes? If so, this doesn't work. As-is, this allows any other 
> > > properties. You need 'unevaluatedProperties: false' here to prevent 
> > > that. But then no one can add properties. If you want to support that, 
> > > then you need this to be a separate schema that devices can optionally 
> > > include if they don't extend the properties, and then devices that 
> > > extend the binding would essentially have the above with:
> > > 
> > > $ref: /schemas/leds/common.yaml#
> > > unevaluatedProperties: false
> > > properties:
> > >   a-custom-device-prop: ...
> > > 
> > > 
> > > If you wanted to define both common ethernet LED properties and 
> > > device specific properties, then you'd need to replace leds/common.yaml 
> > > above  with the ethernet one.
> > > 
> > > This is all the same reasons the DSA/switch stuff and graph bindings are 
> > > structured the way they are.
> > > 
> > 
> > Hi Rob, thanks for the review/questions.
> > 
> > The idea of all of this is to keep leds node as standard as possible.
> > It was asked to add unevaluatedProperties: False but I didn't understood
> > it was needed also for the led nodes.
> > 
> > leds/common.yaml have additionalProperties set to true but I guess that
> > is not OK for the final schema and we need something more specific.
> > 
> > Looking at the common.yaml schema reg binding is missing so an
> > additional schema is needed.
> > 
> > Reg is needed for ethernet LEDs and PHY but I think we should also permit
> > to skip that if the device actually have just one LED. (if this wouldn't
> > complicate the implementation. Maybe some hints from Andrew about this
> > decision?)
> 
> I would make reg mandatory.
>

Ok will add a new schema and change the regex.

> We should not encourage additional properties, but i also think we
> cannot block it.
> 
> The problem we have is that there is absolutely no standardisation
> here. Vendors are free to do whatever they want, and they do. So i
> would not be too surprised if some vendor properties are needed
> eventually.
>

Think that will come later with defining a more specific schema. But I
honestly think most of the special implementation will be handled to the
driver internally and not with special binding in DT.
  

On Wed, Mar 22, 2023 at 12:39:48AM +0100, Christian Marangi wrote:
> On Wed, Mar 22, 2023 at 12:23:59AM +0100, Andrew Lunn wrote:
> > > > Are specific ethernet controllers allowed to add their own properties in 
> > > > led nodes? If so, this doesn't work. As-is, this allows any other 
> > > > properties. You need 'unevaluatedProperties: false' here to prevent 
> > > > that. But then no one can add properties. If you want to support that, 
> > > > then you need this to be a separate schema that devices can optionally 
> > > > include if they don't extend the properties, and then devices that 
> > > > extend the binding would essentially have the above with:
> > > > 
> > > > $ref: /schemas/leds/common.yaml#
> > > > unevaluatedProperties: false
> > > > properties:
> > > >   a-custom-device-prop: ...
> > > > 
> > > > 
> > > > If you wanted to define both common ethernet LED properties and 
> > > > device specific properties, then you'd need to replace leds/common.yaml 
> > > > above  with the ethernet one.
> > > > 
> > > > This is all the same reasons the DSA/switch stuff and graph bindings are 
> > > > structured the way they are.
> > > > 
> > > 
> > > Hi Rob, thanks for the review/questions.
> > > 
> > > The idea of all of this is to keep leds node as standard as possible.
> > > It was asked to add unevaluatedProperties: False but I didn't understood
> > > it was needed also for the led nodes.
> > > 
> > > leds/common.yaml have additionalProperties set to true but I guess that
> > > is not OK for the final schema and we need something more specific.
> > > 
> > > Looking at the common.yaml schema reg binding is missing so an
> > > additional schema is needed.
> > > 
> > > Reg is needed for ethernet LEDs and PHY but I think we should also permit
> > > to skip that if the device actually have just one LED. (if this wouldn't
> > > complicate the implementation. Maybe some hints from Andrew about this
> > > decision?)
> > 
> > I would make reg mandatory.
> >
> 
> Ok will add a new schema and change the regex.
> 
> > We should not encourage additional properties, but i also think we
> > cannot block it.
> > 
> > The problem we have is that there is absolutely no standardisation
> > here. Vendors are free to do whatever they want, and they do. So i
> > would not be too surprised if some vendor properties are needed
> > eventually.
> >
> 
> Think that will come later with defining a more specific schema. But I
> honestly think most of the special implementation will be handled to the
> driver internally and not with special binding in DT.

Then encourage no additional properties by letting whomever wants to add 
them to restructure the schema. ;)

Rob
  

On Tue, Mar 21, 2023 at 11:54:46PM +0100, Christian Marangi wrote:
> On Tue, Mar 21, 2023 at 04:19:53PM -0500, Rob Herring wrote:
> > On Sun, Mar 19, 2023 at 08:18:09PM +0100, Christian Marangi wrote:
> > > Document support for LEDs node in ethernet-controller.
> > > Ethernet Controller may support different LEDs that can be configured
> > > for different operation like blinking on traffic event or port link.
> > > 
> > > Also add some Documentation to describe the difference of these nodes
> > > compared to PHY LEDs, since ethernet-controller LEDs are controllable
> > > by the ethernet controller regs and the possible intergated PHY doesn't
> > > have control on them.
> > > 
> > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > ---
> > >  .../bindings/net/ethernet-controller.yaml     | 21 +++++++++++++++++++
> > >  1 file changed, 21 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > > index 00be387984ac..a93673592314 100644
> > > --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > > +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > > @@ -222,6 +222,27 @@ properties:
> > >          required:
> > >            - speed
> > >  
> > > +  leds:
> > > +    type: object
> > > +    description:
> > > +      Describes the LEDs associated by Ethernet Controller.
> > > +      These LEDs are not integrated in the PHY and PHY doesn't have any
> > > +      control on them. Ethernet Controller regs are used to control
> > > +      these defined LEDs.
> > > +
> > > +    properties:
> > > +      '#address-cells':
> > > +        const: 1
> > > +
> > > +      '#size-cells':
> > > +        const: 0
> > > +
> > > +    patternProperties:
> > > +      '^led(@[a-f0-9]+)?$':
> > > +        $ref: /schemas/leds/common.yaml#
> > 
> > Are specific ethernet controllers allowed to add their own properties in 
> > led nodes? If so, this doesn't work. As-is, this allows any other 
> > properties. You need 'unevaluatedProperties: false' here to prevent 
> > that. But then no one can add properties. If you want to support that, 
> > then you need this to be a separate schema that devices can optionally 
> > include if they don't extend the properties, and then devices that 
> > extend the binding would essentially have the above with:
> > 
> > $ref: /schemas/leds/common.yaml#
> > unevaluatedProperties: false
> > properties:
> >   a-custom-device-prop: ...
> > 
> > 
> > If you wanted to define both common ethernet LED properties and 
> > device specific properties, then you'd need to replace leds/common.yaml 
> > above  with the ethernet one.
> > 
> > This is all the same reasons the DSA/switch stuff and graph bindings are 
> > structured the way they are.
> > 
> 
> Hi Rob, thanks for the review/questions.
> 
> The idea of all of this is to keep leds node as standard as possible.
> It was asked to add unevaluatedProperties: False but I didn't understood
> it was needed also for the led nodes.
> 
> leds/common.yaml have additionalProperties set to true but I guess that
> is not OK for the final schema and we need something more specific.

Yes, every node needs a schema with all possible properties and then 
'unevaluatedProperties: false' to not allow any other properties.

> Looking at the common.yaml schema reg binding is missing so an
> additional schema is needed.
> 
> Reg is needed for ethernet LEDs and PHY but I think we should also permit
> to skip that if the device actually have just one LED. (if this wouldn't
> complicate the implementation. Maybe some hints from Andrew about this
> decision?)
> 
> If we decide that reg is a must, if I understood it correctly we should
> create something like leds-ethernet.yaml that would reference common and
> add reg binding? Is it correct? This schema should be laded in leds
> directory and not in the net/ethernet.

You need 'reg' in properties, but whether it is required or not just 
depends on putting it in 'required'. I don't have a strong opinion on 
that, but generally it's only use 'reg' when there's more than 1.

Rob