linux-kernel - Re: [RFC PATCH] spi: dt-bindings: ti,qspi: convert to dtschema

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20240429162638.GA2066527-robh@kernel.org>
Date: Mon, 29 Apr 2024 11:26:38 -0500
From: Rob Herring <robh@...nel.org>
To: Kousik Sanagavarapu <five231003@...il.com>
Cc: Mark Brown <broonie@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, linux-spi@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	Shuah Khan <skhan@...uxfoundation.org>,
	Javier Carrasco <javier.carrasco.cruz@...il.com>
Subject: Re: [RFC PATCH] spi: dt-bindings: ti,qspi: convert to dtschema

On Sun, Apr 28, 2024 at 12:28:59PM +0530, Kousik Sanagavarapu wrote:
> Convert txt binding of TI's qspi controller (found on their omap SoCs) to
> dtschema to allow for validation.
> 
> It is however to be noted that it is not a one-to-one conversion, in the
> sense that the original txt binding needed to be updated, but these
> changes are included in the dtschema and are mentioned below.
> 
> The changes, w.r.t. the original txt binding, are:
> 
> - Introduce "clocks" and "clock-names" which was never mentioned.
> - Reflect that "ti,hwmods" is deprecated and is not a "required"
>   property anymore.
> - Introduce "num-cs" which allows for setting the number of chip
>   selects.
> 
> Signed-off-by: Kousik Sanagavarapu <five231003@...il.com>
> ---
> I'm a bit iffy about this one as I guess the original txt binding failed
> to cover some things about the properties.  I added the properties based
> on their use in the *.dtsi files when I grepped for the compatible string
> 
>         arch/arm/boot/dts/ti/omap/dra7.dtsi
>         arch/arm/boot/dts/ti/omap/am4372.dtsi
> 
> I also looked at the probe function in the driver for it, which can be
> found at
> 
>         drivers/spi/spi-ti-qspi.c
> 
>  .../devicetree/bindings/spi/ti,qspi.yaml      | 94 +++++++++++++++++++
>  .../devicetree/bindings/spi/ti_qspi.txt       | 53 -----------
>  2 files changed, 94 insertions(+), 53 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/spi/ti,qspi.yaml
>  delete mode 100644 Documentation/devicetree/bindings/spi/ti_qspi.txt
> 
> diff --git a/Documentation/devicetree/bindings/spi/ti,qspi.yaml b/Documentation/devicetree/bindings/spi/ti,qspi.yaml
> new file mode 100644
> index 000000000000..77cabd7158f5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/ti,qspi.yaml
> @@ -0,0 +1,94 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spi/ti,qspi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TI QSPI controller
> +
> +maintainers:
> +  - Kousik Sanagavarapu <five231003@...il.com>
> +
> +allOf:
> +  - $ref: spi-controller.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,am4372-qspi
> +      - ti,dra7xxx-qspi
> +
> +  reg:
> +    items:
> +      - description: base registers
> +      - description: mapped memory
> +
> +  reg-names:
> +    items:
> +      - const: qspi_base
> +      - const: qspi_mmap
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: fck
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  num-cs:
> +    maxItems: 1

This is not an array, so maxItems is not appropriate. If there are only 
certain values supported (default is up to 2^32), then you should 
define them with enum or maximum.

> +
> +  ti,hwmods:
> +    description:
> +      Name of the hwmod associated to the QSPI.  This is for legacy
> +      platforms only.
> +    $ref: /schemas/types.yaml#/definitions/string
> +    deprecated: true
> +
> +  syscon-chipselects:
> +    description:
> +      Handle to system control region contains QSPI chipselect register
> +      and offset of that register.
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      items:
> +        - description: phandle to system control register
> +        - description: register offset

This allows any number of phandle+offset entries. Is that what you want? 
If not, you need either '- items' for the 2nd 'items' or 'maxItems: 1'.

> +
> +  spi-max-frequency:
> +    description: Maximum SPI clocking speed of the controller in Hz.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - clocks
> +  - clock-names
> +  - interrupts
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/dra7.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    qspi: spi@0 {

Drop unused labels.

Unit-address is wrong.

> +        compatible = "ti,dra7xxx-qspi";
> +        reg = <0x4b300000 0x100>,
> +              <0x5c000000 0x4000000>;
> +        reg-names = "qspi_base", "qspi_mmap";
> +        syscon-chipselects = <&scm_conf 0x558>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        clocks = <&l4per2_clkctrl DRA7_L4PER2_QSPI_CLKCTRL 25>;
> +        clock-names = "fck";
> +        num-cs = <4>;
> +        spi-max-frequency = <48000000>;
> +        interrupts = <GIC_SPI 343 IRQ_TYPE_LEVEL_HIGH>;
> +    };
> +...