| Message ID | 20240325-hsi-dt-binding-v1-1-88e8e97c3aae@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Convert MIPI HSI DT bindings to YAML | expand |
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(-)