Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
MIME-Version: 1.0
References: <20230807174159.1140957-1-kyletso@google.com> <20230807174159.1140957-2-kyletso@google.com>
 <20230821162517.GA1832382-robh@kernel.org> <CAGZ6i=0ey+94YYgc=os5iQ_pNmNVB=qZQD0=jB4fRTKggZPSWA@mail.gmail.com>
 <CAL_JsqLjAooY27pG4GQHRdpiB_=0RCYrkKOjzAvmvz=5yU71Og@mail.gmail.com>
In-Reply-To: <CAL_JsqLjAooY27pG4GQHRdpiB_=0RCYrkKOjzAvmvz=5yU71Og@mail.gmail.com>
From:   Kyle Tso <kyletso@google.com>
Date:   Tue, 5 Dec 2023 01:24:09 +0800
Message-ID: <CAGZ6i=2qoi12OJGNmuvVm7wDgbGTnuywpuC=EYZ_MfdNPazNqw@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] dt-bindings: connector: Add child nodes for
 multiple PD capabilities
To:     Rob Herring <robh@kernel.org>
Cc:     krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
        linux@roeck-us.net, heikki.krogerus@linux.intel.com,
        gregkh@linuxfoundation.org, badhri@google.com,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-usb@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Nov 22, 2023 at 11:36=E2=80=AFPM Rob Herring <robh@kernel.org> wrot=
e:
>
> On Wed, Oct 18, 2023 at 12:19=E2=80=AFPM Kyle Tso <kyletso@google.com> wr=
ote:
> >
> > On Tue, Aug 22, 2023 at 12:25=E2=80=AFAM Rob Herring <robh@kernel.org> =
wrote:
> > >
> >
> > Sorry for the late response...
> >
> > > On Tue, Aug 08, 2023 at 01:41:58AM +0800, Kyle Tso wrote:
> > > > Define a new optional property "capabilities" which is a child node
> > > > under connector to contain multiple USB Power Delivery capabilities=
.
> > > >
> > > > Define a new property with pattern (e.g. caps-0, caps-1) which is a
> > >
> > > A property in json-schema terms, but for DT it's a node not a propert=
y.
> > > 'Define a child node ...' would be clearer.
> > >
> >
> > Will do in the future patch
> >
> > > > child node under "capabilities". Each node contains PDO data of a
> > > > selectable Power Delivery capability.
> > > >
> > > > Also define common properties for source-pdos, sink-pdos, and
> > > > op-sink-microwatt that can be referenced.
> > >
> > > Why do we need this? What issue does this solve? The commit message
> > > should answer those questions (always).
> > >
> >
> > Will state the rationale in the future patch
> >
> > > > Signed-off-by: Kyle Tso <kyletso@google.com>
> > > > ---
> > > > v1 -> v2:
> > > > - move source/sink-pdos to $defs and reference them in properties
> > > >
> > > >  .../bindings/connector/usb-connector.yaml     | 80 +++++++++++++--=
----
> > > >  1 file changed, 57 insertions(+), 23 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/connector/usb-connec=
tor.yaml b/Documentation/devicetree/bindings/connector/usb-connector.yaml
> > > > index 1c4d3eb87763..c6b02dbda83f 100644
> > > > --- a/Documentation/devicetree/bindings/connector/usb-connector.yam=
l
> > > > +++ b/Documentation/devicetree/bindings/connector/usb-connector.yam=
l
> > > > @@ -117,28 +117,10 @@ properties:
> > > >    # The following are optional properties for "usb-c-connector" wi=
th power
> > > >    # delivery support.
> > > >    source-pdos:
> > > > -    description: An array of u32 with each entry providing support=
ed power
> > > > -      source data object(PDO), the detailed bit definitions of PDO=
 can be found
> > > > -      in "Universal Serial Bus Power Delivery Specification" chapt=
er 6.4.1.2
> > > > -      Source_Capabilities Message, the order of each entry(PDO) sh=
ould follow
> > > > -      the PD spec chapter 6.4.1. Required for power source and pow=
er dual role.
> > > > -      User can specify the source PDO array via PDO_FIXED/BATT/VAR=
/PPS_APDO()
> > > > -      defined in dt-bindings/usb/pd.h.
> > > > -    minItems: 1
> > > > -    maxItems: 7
> > > > -    $ref: /schemas/types.yaml#/definitions/uint32-array
> > > > +    $ref: "#/$defs/source-pdos"
> > > >
> > > >    sink-pdos:
> > > > -    description: An array of u32 with each entry providing support=
ed power sink
> > > > -      data object(PDO), the detailed bit definitions of PDO can be=
 found in
> > > > -      "Universal Serial Bus Power Delivery Specification" chapter =
6.4.1.3
> > > > -      Sink Capabilities Message, the order of each entry(PDO) shou=
ld follow the
> > > > -      PD spec chapter 6.4.1. Required for power sink and power dua=
l role. User
> > > > -      can specify the sink PDO array via PDO_FIXED/BATT/VAR/PPS_AP=
DO() defined
> > > > -      in dt-bindings/usb/pd.h.
> > > > -    minItems: 1
> > > > -    maxItems: 7
> > > > -    $ref: /schemas/types.yaml#/definitions/uint32-array
> > > > +    $ref: "#/$defs/sink-pdos"
> > > >
> > > >    sink-vdos:
> > > >      description: An array of u32 with each entry, a Vendor Defined=
 Message Object (VDO),
> > > > @@ -164,9 +146,7 @@ properties:
> > > >      $ref: /schemas/types.yaml#/definitions/uint32-array
> > > >
> > > >    op-sink-microwatt:
> > > > -    description: Sink required operating power in microwatt, if so=
urce can't
> > > > -      offer the power, Capability Mismatch is set. Required for po=
wer sink and
> > > > -      power dual role.
> > > > +    $ref: "#/$defs/op-sink-microwatt"
> > > >
> > > >    port:
> > > >      $ref: /schemas/graph.yaml#/properties/port
> > > > @@ -228,6 +208,30 @@ properties:
> > > >        SNK_READY for non-pd link.
> > > >      type: boolean
> > > >
> > > > +  capabilities:
> > > > +    description: A child node to contain all the selectable USB Po=
wer Delivery capabilities.
> > > > +    type: object
> > > > +
> > > > +    patternProperties:
> > > > +      "^caps-[0-9]+$":
> > > > +        description: Child nodes under "capabilities" node. Each n=
ode contains a selectable USB
> > > > +          Power Delivery capability.
> > > > +        type: object
> > > > +
> > > > +        properties:
> > > > +          source-pdos:
> > > > +            $ref: "#/$defs/source-pdos"
> > > > +
> > > > +          sink-pdos:
> > > > +            $ref: "#/$defs/sink-pdos"
> > > > +
> > > > +          op-sink-microwatt:
> > > > +            $ref: "#/$defs/op-sink-microwatt"
> > > > +
> > > > +        additionalProperties: false
> > > > +
> > > > +    additionalProperties: false
> > > > +
> > > >  dependencies:
> > > >    sink-vdos-v1: [ 'sink-vdos' ]
> > > >    sink-vdos: [ 'sink-vdos-v1' ]
> > > > @@ -235,6 +239,36 @@ dependencies:
> > > >  required:
> > > >    - compatible
> > > >
> > > > +$defs:
> > >
> > > Make this:
> > >
> > > $defs:
> > >   capabilities:
> > >     properties:
> > >       ...
> > >
> > > And then just reference "#/$defs/capabilities" at the top-level and i=
n
> > > caps-[0-9] node schema.
> > >
> > > You'll need to use unevaluatedProperties instead of additionalPropert=
ies
> > > as well.
> > >
> >
> > I am sorry I don't understand this part. Could you explain more?
> >
> > In $defs, define a whole structure of "capabilities" which has
> > patternProperties "caps-[0-9]+", and inside "caps-[0-9]+" there are
> > properties "source/sink-pdos" ?
>
> No, define a list of properties under a def. Here's a more complete examp=
le:
>
> $defs:
>   capabilities:
>     properties:
>       source-pdos:
>         ...
>
>       sink-pdos:
>         ...
>
> And then add "$ref: '#/$defs/capabilities'" at the top-level of this
> schema doc and under "caps-[0-9]+":
>
>     patternProperties:
>       "^caps-[0-9]+$":
>         description: Child nodes under "capabilities" node. Each node
> contains a selectable USB
>           Power Delivery capability.
>         type: object
>         $ref: "#/$defs/capabilities"
>         unevaluatedProperties: false
>
> unevaluatedProperties is needed because it can "see" properties
> defined in a $ref to get the full list of properties for a node.
>
> Rob

Thanks! Will update in v4

Kyle