diff mbox

[RFC,2/2] dt-bindings: spi: QuadSPI driver for Atmel SAMA5D2 documentation

Message ID 20180618162124.21749-3-bugalski.piotr@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Piotr Bugalski June 18, 2018, 4:21 p.m. UTC
Documentation for DT-binding change.

Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com>
Signed-off-by: Piotr Bugalski <pbu@cryptera.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

Comments

Boris Brezillon June 20, 2018, 2:47 p.m. UTC | #1
Hi Piotr,

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

> Documentation for DT-binding change.
> 
> Suggested-by: Boris Brezillon <boris.brezillon@bootlin.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@cryptera.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@f0024000 {
> +	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
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Piotr Bugalski June 21, 2018, 10:56 a.m. UTC | #2
Hi Boris,

On Wed, 20 Jun 2018, Boris Brezillon wrote:

> Hi Piotr,
>
> On Mon, 18 Jun 2018 18:21:24 +0200
> Piotr Bugalski <bugalski.piotr@gmail.com> wrote:
>
>> Documentation for DT-binding change.
>>
>> Suggested-by: Boris Brezillon <boris.brezillon@bootlin.com>
>
> I'm pretty sure I didn't make a single suggestion about the DT
> bindings you use here ;-).
>

Ok, I misunderstood a bit your idea, but I think from next release this
field will be in good place. So it was just prepared for the future ;-)

>> Signed-off-by: Piotr Bugalski <pbu@cryptera.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.

I'll change it in next version.

>
>>
>> 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@f0024000 {
>> +	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.
>

I was mainly focusing on NAND-flash with QSPI inteface so I took
example from tested configuration. Next time I'll use NOR-flash.

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

Thanks,
Piotr

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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@f0024000 {
+	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";
+		reg = <0>;
+		spi-max-frequency = <83000000>;
+		spi-rx-bus-width = <4>;
+		spi-tx-bus-width = <4>;
+
+		...
+	};
+};
+