Re: [PATCH net-next v3 10/17] dt-bindings: net: pse-pd: Add another way of describing several PSE PIs

[Köry Maincent]

Hello Rob,

Thanks for your review!

On Fri, 9 Feb 2024 14:43:49 +0000
Rob Herring <robh@xxxxxxxxxx> wrote:

> On Thu, Feb 08, 2024 at 02:08:47PM +0100, Kory Maincent wrote:
> > Before hand we set "#pse-cell" to 1 to define a PSE controller with  
> 
> #pse-cells
> 
> > several PIs (Power Interface). The drawback of this was that we could not
> > have any information on the PI except its number.  
> 
> Then increase it to what you need. The whole point of #foo-cells is that 
> it is variable depending on what the provider needs. 
> 
> > Add support for pse_pis and pse_pi node to be able to have more information
> > on the PI like the number of pairset used and the pairset pinout.  
> 
> Please explain the problem you are trying to solve, not your solution. I 
> don't understand what the problem is to provide any useful suggestions 
> on the design.

Please see Oleksij's reply.
Thank you Oleksij, for the documentation!!

> > 
> > Sponsored-by: Dent Project <dentproject@xxxxxxxxxxxxxxxxxxx>  
> 
> Is this a recognized tag? First I've seen it.

This is not a standard tag but it has been used several times in the past.

> >  
> > -required:
> > -  - "#pse-cells"
> > +  pse_pis:
> > +    $ref: "#/$defs/pse_pis"
> > +
> > +$defs:  
> 
> $defs is for when you need multiple copies of the same thing. I don't 
> see that here.

I made this choice for better readability but indeed it is used only once.
I will remove it then.

> > +  pse_pis:
> > +    type: object
> > +    description:
> > +      Kind of a matrix to identify the concordance between a PSE Power
> > +      Interface and one or two (PoE4) physical ports.
> > +
> > +    properties:
> > +      "#address-cells":
> > +        const: 1
> > +
> > +      "#size-cells":
> > +        const: 0
> > +
> > +    patternProperties:
> > +      "^pse_pi@[0-9]+$":  
> 
> Unit-addresses are hex.

Oops sorry for the mistake.

> 
> > +        $ref: "#/$defs/pse_pi"
> > +
> > +    required:
> > +      - "#address-cells"
> > +      - "#size-cells"
> > +
> > +  pse_pi:
> > +    description:
> > +      PSE PI device for power delivery via pairsets, compliant with IEEE
> > +      802.3-2022, Section 145.2.4. Each pairset comprises a positive and a
> > +      negative VPSE pair, adhering to the pinout configurations detailed in
> > +      the standard.
> > +    type: object
> > +    properties:
> > +      reg:
> > +        maxItems: 1  
> 
> As you are defining the addressing here, you need to define what the 
> "addresses" are.

Yes I will add some documentation in next version.

> > +          values are "alternative-a" and "alternative-b". Each name should
> > +          correspond to a phandle in the 'pairset' property pointing to the
> > +          power supply for that pairset.
> > +        $ref: /schemas/types.yaml#/definitions/string-array
> > +        minItems: 1
> > +        maxItems: 2
> > +        items:
> > +          - enum:
> > +            - "alternative-a"
> > +            - "alternative-b"  
> 
> This leaves the 2nd entry undefined. You need the dictionary form of 
> 'items' rather than a list. IOW, Drop the '-' under items.

Oh thanks! That is what I was looking for. I was struggling using the right
description.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com