| Message ID | 20240325-hsi-dt-binding-v1-1-88e8e97c3aae@collabora.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Convert MIPI HSI DT bindings to YAML | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 2 warnings, 84 lines checked |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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) but more importantly: some properties are now excluding each other. > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> > --- ... > 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@kernel.org> > + > +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. > + > + 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. > + - if: > + required: > + - hsi-rx-mode > + then: > + properties: > + hsi-mode: false > Best regards, Krzysztof
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@collabora.com> > > --- > > ... > > > 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@kernel.org> > > + > > +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
On 26/03/2024 13:45, Sebastian Reichel wrote: > >> but more importantly: some properties are now excluding each >> other. > > I think that requirement was already there. Right. ... >>> + >>> +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 Not entirely. anyOf should succeed also when none of them are present, which is not what you want in such case. > 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. Then wouldn't this work for you: https://elixir.bootlin.com/linux/v5.17-rc2/source/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml#L91 ? But if you really want them to be optional but excluding, then simpler syntax is: https://lore.kernel.org/all/20230118163208.GA117919-robh@kernel.org/ Best regards, Krzysztof
Hi, On Tue, Mar 26, 2024 at 01:56:22PM +0100, Krzysztof Kozlowski wrote: > >>> +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 > > Not entirely. anyOf should succeed also when none of them are present, > which is not what you want in such case. Right, this should be oneOf instead of anyOf. I fixed that for v2. > > 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. > > Then wouldn't this work for you: > https://elixir.bootlin.com/linux/v5.17-rc2/source/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml#L91 I suppose you mean using "then: not: required: PROPERTY" instead of "then: PROPERTY: false"? The variant using "PROPERTY: false" is what is being used in example-schema.yaml: https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/example-schema.yaml#L225 IMHO the "not: required: property" is harder to understand. I would expect that to mean "the property is not required (i.e. optional)" instead of "the property is not allowed". -- Sebastian
On 26/03/2024 16:15, Sebastian Reichel wrote: > Hi, > > On Tue, Mar 26, 2024 at 01:56:22PM +0100, Krzysztof Kozlowski wrote: >>>>> +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 >> >> Not entirely. anyOf should succeed also when none of them are present, >> which is not what you want in such case. > > Right, this should be oneOf instead of anyOf. I fixed that for v2. > >>> 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. >> >> Then wouldn't this work for you: >> https://elixir.bootlin.com/linux/v5.17-rc2/source/Documentation/devicetree/bindings/reserved-memory/reserved-memory.yaml#L91 > > I suppose you mean using "then: not: required: PROPERTY" instead of > "then: PROPERTY: false"? The variant using "PROPERTY: false" is what > is being used in example-schema.yaml: No, I pointed to specific line with code for you. > > https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/example-schema.yaml#L225 > > IMHO the "not: required: property" is harder to understand. I would > expect that to mean "the property is not required (i.e. optional)" > instead of "the property is not allowed". Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/hsi/client-devices.txt b/Documentation/devicetree/bindings/hsi/client-devices.txt deleted file mode 100644 index 104c9a3e57a4..000000000000 --- a/Documentation/devicetree/bindings/hsi/client-devices.txt +++ /dev/null @@ -1,44 +0,0 @@ -Each HSI port is supposed to have one child node, which -symbols the remote device connected to the HSI port. The -following properties are standardized for HSI clients: - -Required HSI configuration properties: - -- hsi-channel-ids: A list of channel ids - -- hsi-rx-mode: Receiver Bit transmission mode ("stream" or "frame") -- hsi-tx-mode: Transmitter Bit transmission mode ("stream" or "frame") -- hsi-mode: 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: Max bit transmission speed in kbit/s -- hsi-flow: RX flow type ("synchronized" or "pipeline") -- hsi-arb-mode: Arbitration mode for TX frame ("round-robin", "priority") - -Optional HSI configuration properties: - -- hsi-channel-names: A list with one name per channel specified in the - hsi-channel-ids property - - -Device Tree node example for an HSI client: - -hsi-controller { - hsi-port { - modem: hsi-client { - compatible = "nokia,n900-modem"; - - hsi-channel-ids = <0>, <1>, <2>, <3>; - hsi-channel-names = "mcsaab-control", - "speech-control", - "speech-data", - "mcsaab-data"; - hsi-speed-kbps = <55000>; - hsi-mode = "frame"; - hsi-flow = "synchronized"; - hsi-arb-mode = "round-robin"; - - /* more client specific properties */ - }; - }; -}; 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@kernel.org> + +properties: + $nodename: + const: hsi-client + + 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 + - if: + required: + - hsi-rx-mode + then: + properties: + hsi-mode: false
Convert the legacy txt binding to modern YAML and rename from client-devices to hsi-client. No semantic change. Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com> --- .../devicetree/bindings/hsi/client-devices.txt | 44 ------------ .../devicetree/bindings/hsi/hsi-client.yaml | 84 ++++++++++++++++++++++ 2 files changed, 84 insertions(+), 44 deletions(-)