Re: [PATCH 1/3] dt-bindings: hsi: hsi-client: convert to YAML

[Sebastian Reichel]

Hi,

On Tue, Mar 26, 2024 at 08:18:39AM +0100, Krzysztof Kozlowski wrote:
> On 25/03/2024 22:45, Sebastian Reichel wrote:
> > Convert the legacy txt binding to modern YAML and rename from
> > client-devices to hsi-client. No semantic change.
> 
> There is semantic change: missing example (which is reasonable for
> shared schema)

Right, I should have mentioned that.

> but more importantly: some properties are now excluding each
> other.

I think that requirement was already there.

> > Signed-off-by: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxx>
> > ---
> 
> ...
> 
> > diff --git a/Documentation/devicetree/bindings/hsi/hsi-client.yaml b/Documentation/devicetree/bindings/hsi/hsi-client.yaml
> > new file mode 100644
> > index 000000000000..df6e1fdd2702
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hsi/hsi-client.yaml
> > @@ -0,0 +1,84 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/hsi/hsi-client.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: HSI bus peripheral
> > +
> > +description:
> > +  Each HSI port is supposed to have one child node, which
> > +  symbols the remote device connected to the HSI port.
> > +
> > +maintainers:
> > +  - Sebastian Reichel <sre@xxxxxxxxxx>
> > +
> > +properties:
> > +  $nodename:
> > +    const: hsi-client
> 
> Why? Does anything depend on this? It breaks generic-node-name rule. It
> seems you need it only to match the schema, but this just point to main
> problem - missing bus schema.

Ah, that's a good point. It makes a lot more sense to get the
nodename from the actual client. I will work this over.

> > +
> > +  hsi-channel-ids:
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    minItems: 1
> > +    maxItems: 8
> > +
> > +  hsi-channel-names:
> > +    minItems: 1
> > +    maxItems: 8
> > +
> > +  hsi-rx-mode:
> > +    enum: [stream, frame]
> > +    description: Receiver Bit transmission mode
> > +
> > +  hsi-tx-mode:
> > +    enum: [stream, frame]
> > +    description: Transmitter Bit transmission mode
> > +
> > +  hsi-mode:
> > +    enum: [stream, frame]
> > +    description:
> > +      May be used instead hsi-rx-mode and hsi-tx-mode if the
> > +      transmission mode is the same for receiver and transmitter.
> > +
> > +  hsi-speed-kbps:
> > +    description: Max bit transmission speed in kbit/s
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +
> > +  hsi-flow:
> > +    enum: [synchronized, pipeline]
> > +    description: RX flow type
> > +
> > +  hsi-arb-mode:
> > +    enum: [round-robin, priority]
> > +    description: Arbitration mode for TX frame
> > +
> > +additionalProperties: true
> > +
> > +required:
> > +  - compatible
> > +  - hsi-channel-ids
> > +  - hsi-speed-kbps
> > +  - hsi-flow
> > +  - hsi-arb-mode
> > +
> > +anyOf:
> > +  - required:
> > +      - hsi-mode
> > +  - required:
> > +      - hsi-rx-mode
> > +      - hsi-tx-mode
> > +
> > +allOf:
> > +  - if:
> > +      required:
> > +        - hsi-mode
> > +    then:
> > +      properties:
> > +        hsi-rx-mode: false
> > +        hsi-tx-mode: false
> 
> I don't understand what you are trying to achieve here and with anyOf.
> It looks like just oneOf. OTOH, old binding did not exclude these
> properties.

So the anyOf ensures, that either hsi-mode or hsi-rx-mode +
hsi-tx-mode are specified. Those properties were previously
listed as required and they are indeed mandatory by the Linux
kernel implementation.

The old binding also has this:

hsi-mode:		May be used ***instead*** hsi-rx-mode and hsi-tx-mode

So it's either hsi-rx-mode + hsi-tx-mode OR hsi-mode, but not
all properties at the same time. That's what the allOf ensures:
if hsi-mode is specified, then hsi-rx-mode and hsi-tx-mode may
not be specified.

> > +  - if:
> > +      required:
> > +        - hsi-rx-mode
> > +    then:
> > +      properties:
> > +        hsi-mode: false
> > 

Thanks for the review,

-- Sebastian

Attachment: signature.asc
Description: PGP signature