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

From: Sebastian Reichel
Date: Tue Mar 26 2024 - 08:46:07 EST


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