linux-kernel - Re: [RFC PATCH 2/2] dt-bindings: spi: QuadSPI driver for Atmel SAMA5D2 documentation

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 20 Jun 2018 16:47:41 +0200
From:   Boris Brezillon <boris.brezillon@...tlin.com>
To:     Piotr Bugalski <bugalski.piotr@...il.com>
Cc:     Mark Brown <broonie@...nel.org>, linux-spi@...r.kernel.org,
        David Woodhouse <dwmw2@...radead.org>,
        Brian Norris <computersforpeace@...il.com>,
        Marek Vasut <marek.vasut@...il.com>,
        Richard Weinberger <richard@....at>,
        linux-mtd@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Cyrille Pitchen <cyrille.pitchen@...rochip.com>,
        Tudor Ambarus <tudor.ambarus@...rochip.com>,
        Piotr Bugalski <pbu@...ptera.com>
Subject: Re: [RFC PATCH 2/2] dt-bindings: spi: QuadSPI driver for Atmel
 SAMA5D2 documentation

Hi Piotr,

On Mon, 18 Jun 2018 18:21:24 +0200
Piotr Bugalski <bugalski.piotr@...il.com> wrote:

> Documentation for DT-binding change.
> 
> Suggested-by: Boris Brezillon <boris.brezillon@...tlin.com>

I'm pretty sure I didn't make a single suggestion about the DT
bindings you use here ;-).

> Signed-off-by: Piotr Bugalski <pbu@...ptera.com>
> 
> ---
>  .../devicetree/bindings/spi/spi_atmel-qspi.txt     | 41 ++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/spi/spi_atmel-qspi.txt

I'll comment on this aspect in more details when replying to the cover
letter, but I think you should re-use the bindings defined in
Documentation/devicetree/bindings/mtd/atmel-quadspi.txt (IOW, move the
existing file to the Documentation/devicetree/bindings/spi directory).

It's the same HW block, and just because you develop a new driver to
replace the old one doesn't mean you should have 2 different bindings in
parallel.

> 
> diff --git a/Documentation/devicetree/bindings/spi/spi_atmel-qspi.txt b/Documentation/devicetree/bindings/spi/spi_atmel-qspi.txt
> new file mode 100644
> index 000000000000..d52b534c9c2b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/spi_atmel-qspi.txt
> @@ -0,0 +1,41 @@
> +* Atmel Quad Serial Peripheral Interface (QSPI)
> +
> +Required properties:
> +- compatible:     Should be "atmel,sama5d2-spi-qspi".
> +- reg:            Should contain the locations and lengths of the base registers
> +                  and the mapped memory.
> +- reg-names:      Should contain the resource reg names:
> +                  - qspi_base: configuration register address space
> +                  - qspi_mmap: memory mapped address space
> +- interrupts:     Should contain the interrupt for the device.
> +- clocks:         The phandle of the clock needed by the QSPI controller.
> +- #address-cells: Should be <1>.
> +- #size-cells:    Should be <0>.
> +
> +Example:
> +
> +qspi1: spi@...24000 {
> +	compatible = "atmel,sama5d2-spi-qspi";
> +	reg = <0xf0024000 0x100>, <0xd8000000 0x08000000>;
> +	reg-names = "qspi_base", "qspi_mmap";
> +	interrupts = <53 IRQ_TYPE_LEVEL_HIGH 7>;
> +	clocks = <&qspi1_clk>;
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_qspi1_default>;
> +	status = "okay";
> +
> +	flash@0 {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		compatible = "winbond,w25m02gv", "spi-nand";

"winbond,w25m02gv" is undocumented and unnecessary since SPI NANDs are
automatically detected. Also, maybe you should declare a SPI NOR in the
example since SPI NAND support has not yet been merged.

> +		reg = <0>;
> +		spi-max-frequency = <83000000>;
> +		spi-rx-bus-width = <4>;
> +		spi-tx-bus-width = <4>;
> +
> +		...
> +	};
> +};
> +

Regards,

Boris