[PATCH v3 0/3] spi: fsl-dspi: Convert to yaml format and use common SPI property

[PATCH v3 0/3] spi: fsl-dspi: Convert to yaml format and use common SPI property

[Frank Li]

Convert fsl-dspi binding to to yaml format.
Using common SPI property spi-cs-setup-delay-ns and spi-cs-hold-delay-ns.
Update driver and ls1043 dts file.

To: Vladimir Oltean <olteanv@gmail.com>
To: Mark Brown <broonie@kernel.org>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
To: Shawn Guo <shawnguo@kernel.org>
Cc: linux-spi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: imx@lists.linux.dev
Cc: olteanv@gmail.com

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Changes in v3:
- using Vladimir Oltean's https://lore.kernel.org/imx/20240613-ls_qspi-v2-0-b288f6f5b736@nxp.com/T/#t
  for fsl periphal part to keep compatiblity.

- Add common property and depericated fsl private property.
- Link to v2: https://lore.kernel.org/r/20240613-ls_qspi-v2-0-b288f6f5b736@nxp.com

---
Frank Li (3):
      spi: fsl-dspi: use common proptery 'spi-cs-setup(hold)-delay-ns'
      spi: dt-bindings: fsl-dspi: Convert to yaml format
      arm64: dts: fsl-ls1043a-rdb: use common spi-cs-setup(hold)-delay-ns

 .../devicetree/bindings/spi/fsl,dspi.yaml          | 115 +++++++++++++++++++++
 .../spi/fsl,spi-dspi-peripheral-props.yaml         |  28 +++++
 .../devicetree/bindings/spi/spi-fsl-dspi.txt       |  65 ------------
 .../bindings/spi/spi-peripheral-props.yaml         |   1 +
 arch/arm64/boot/dts/freescale/fsl-ls1043a-rdb.dts  |  15 +++
 drivers/spi/spi-fsl-dspi.c                         |  14 ++-
 6 files changed, 168 insertions(+), 70 deletions(-)
---
base-commit: 03d44168cbd7fc57d5de56a3730427db758fc7f6
change-id: 20240613-ls_qspi-bdced20e235e

Best regards,
---
Frank Li <Frank.Li@nxp.com>

[PATCH v3 1/3] spi: fsl-dspi: use common proptery 'spi-cs-setup(hold)-delay-ns'

[Frank Li]

Use SPI common DT binding properties 'spi-cs-setup-delay-ns' and
'spi-cs-hold-delay-ns'. If these properties do not exist, fall back to
legacy 'fsl,spi-cs-sck-delay' and 'fsl,spi-sck-cs-delay'.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/spi/spi-fsl-dspi.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 0a2730cd07c6a..7c1f8af9d215e 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -1018,11 +1018,15 @@ static int dspi_setup(struct spi_device *spi)
 	pdata = dev_get_platdata(&dspi->pdev->dev);
 
 	if (!pdata) {
-		of_property_read_u32(spi->dev.of_node, "fsl,spi-cs-sck-delay",
-				     &cs_sck_delay);
-
-		of_property_read_u32(spi->dev.of_node, "fsl,spi-sck-cs-delay",
-				     &sck_cs_delay);
+		cs_sck_delay = spi_delay_to_ns(&spi->cs_setup, NULL);
+		if (!cs_sck_delay)
+			of_property_read_u32(spi->dev.of_node, "fsl,spi-cs-sck-delay",
+					     &cs_sck_delay);
+
+		sck_cs_delay = spi_delay_to_ns(&spi->cs_hold, NULL);
+		if (!sck_cs_delay)
+			of_property_read_u32(spi->dev.of_node, "fsl,spi-sck-cs-delay",
+					     &sck_cs_delay);
 	} else {
 		cs_sck_delay = pdata->cs_sck_delay;
 		sck_cs_delay = pdata->sck_cs_delay;

-- 
2.34.1

RE: [PATCH v3 1/3] spi: fsl-dspi: use common proptery 'spi-cs-setup(hold)-delay-ns'

[Peng Fan]

> Subject: [PATCH v3 1/3] spi: fsl-dspi: use common proptery 'spi-cs-
> setup(hold)-delay-ns'
> 
> Use SPI common DT binding properties 'spi-cs-setup-delay-ns' and 'spi-
> cs-hold-delay-ns'. If these properties do not exist, fall back to legacy
> 'fsl,spi-cs-sck-delay' and 'fsl,spi-sck-cs-delay'.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/spi/spi-fsl-dspi.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c index
> 0a2730cd07c6a..7c1f8af9d215e 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -1018,11 +1018,15 @@ static int dspi_setup(struct spi_device
> *spi)
>  	pdata = dev_get_platdata(&dspi->pdev->dev);
> 
>  	if (!pdata) {
> -		of_property_read_u32(spi->dev.of_node, "fsl,spi-cs-
> sck-delay",
> -				     &cs_sck_delay);
> -
> -		of_property_read_u32(spi->dev.of_node, "fsl,spi-sck-
> cs-delay",
> -				     &sck_cs_delay);
> +		cs_sck_delay = spi_delay_to_ns(&spi->cs_setup, NULL);
> +		if (!cs_sck_delay)

`if (cs_sck_delay <= 0)` ?

> +			of_property_read_u32(spi->dev.of_node,
> "fsl,spi-cs-sck-delay",
> +					     &cs_sck_delay);
> +
> +		sck_cs_delay = spi_delay_to_ns(&spi->cs_hold, NULL);
> +		if (!sck_cs_delay)

` if (sck_cs_delay <= 0)`?

Regards,
Peng.

> +			of_property_read_u32(spi->dev.of_node,
> "fsl,spi-sck-cs-delay",
> +					     &sck_cs_delay);
>  	} else {
>  		cs_sck_delay = pdata->cs_sck_delay;
>  		sck_cs_delay = pdata->sck_cs_delay;
> 
> --
> 2.34.1
> 

Re: [PATCH v3 1/3] spi: fsl-dspi: use common proptery 'spi-cs-setup(hold)-delay-ns'

[Vladimir Oltean]

On Fri, Jun 21, 2024 at 01:28:28AM +0000, Peng Fan wrote:
> > +		cs_sck_delay = spi_delay_to_ns(&spi->cs_setup, NULL);
> > +		if (!cs_sck_delay)
> 
> `if (cs_sck_delay <= 0)` ?

spi_delay_to_ns() returns error only for SPI_DELAY_UNIT_SCK and for
unknown units.

The first case never appears to be set by the core. Only spi-dw-core.c
and spi-dw-dma.c set SPI_DELAY_UNIT_SCK.

The latter case seems to be mostly avoidable defensive programming.
spi_alloc_device() gives you zero-initialized memory, which means
spi->cs_hold.unit is by default SPI_DELAY_UNIT_USECS (0) and so is
spi->cs_setup.unit. Nothing seems to set the unit to an invalid value,
so the default case appears dead code. If "u8 unit" from within
struct spi_delay was an enum type, it would have likely been fine to
even omit the default case altogether.

There's also the curious case of integer type (signedness) mismatch
between:
- the u32 type of "delay" processed and returned by spi_delay_to_ns()
- the int return type of spi_delay_to_ns()
- the u32 type of the "cs_sck_delay" and "sck_cs_delay" variables used
  by Frank to store the output from spi_delay_to_ns() inside the dspi
  driver

The interaction between these data types means that:
- The "if (cs_sck_delay <= 0)" snippet you suggest will simply not work,
  because the spi_delay_to_ns() function output is assigned to an
  unsigned variable, which is never negative.
- There is a theoretical possibility that a large u32 delay returned by
  spi_delay_to_ns() is misinterpreted as an error by a caller which does
  make an attempt to check for negative values. However, simply casting
  the value back to unsigned as Frank does eliminates that possibility.
  Given that ultimately, the setup and hold times come from u32 device
  tree properties which aren't range-checked, it might just well happen
  for someone who does check for < 0 to trip over this. It might be
  worth somebody having a closer look at this situation.

I don't think that your suggestion will produce better code.

Re: [PATCH v3 1/3] spi: fsl-dspi: use common proptery 'spi-cs-setup(hold)-delay-ns'

[Vladimir Oltean]

Typo in title: property

On Thu, Jun 20, 2024 at 12:58:27PM -0400, Frank Li wrote:
> Use SPI common DT binding properties 'spi-cs-setup-delay-ns' and
> 'spi-cs-hold-delay-ns'. If these properties do not exist, fall back to
> legacy 'fsl,spi-cs-sck-delay' and 'fsl,spi-sck-cs-delay'.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/spi/spi-fsl-dspi.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index 0a2730cd07c6a..7c1f8af9d215e 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -1018,11 +1018,15 @@ static int dspi_setup(struct spi_device *spi)
>  	pdata = dev_get_platdata(&dspi->pdev->dev);
>  
>  	if (!pdata) {
> -		of_property_read_u32(spi->dev.of_node, "fsl,spi-cs-sck-delay",
> -				     &cs_sck_delay);
> -
> -		of_property_read_u32(spi->dev.of_node, "fsl,spi-sck-cs-delay",
> -				     &sck_cs_delay);
> +		cs_sck_delay = spi_delay_to_ns(&spi->cs_setup, NULL);
> +		if (!cs_sck_delay)
> +			of_property_read_u32(spi->dev.of_node, "fsl,spi-cs-sck-delay",
> +					     &cs_sck_delay);
> +
> +		sck_cs_delay = spi_delay_to_ns(&spi->cs_hold, NULL);
> +		if (!sck_cs_delay)
> +			of_property_read_u32(spi->dev.of_node, "fsl,spi-sck-cs-delay",
> +					     &sck_cs_delay);

Keep the 80 character line limit please.

[PATCH v3 2/3] spi: dt-bindings: fsl-dspi: Convert to yaml format

[Frank Li]

Convert dt-binding spi-fsl-dspi.txt to yaml format.

Addtional changes during convert:
- compatible string "fsl,ls1028a-dspi" can be followed by
fsl,ls1021a-v1.0-dspi.
- Change "dspi0@4002c000" to "spi@4002c000" in example.
- Reorder properties in example.
- Use GIC include in example.
- Remove fsl,spi-cs-sck-delay and fsl,spi-sck-cs-delay by use common SPI
property.
- Use compatible string 'jedec,spi-nor' in example.
- Split peripheral part to fsl,spi-dspi-peripheral-props.yaml

Signed-off-by: Frank Li <Frank.Li@nxp.com>

---
Use part of Vladimir Oltean's work at
https://lore.kernel.org/linux-spi/20221111224651.577729-1-vladimir.oltean@nxp.com/
---
 .../devicetree/bindings/spi/fsl,dspi.yaml          | 115 +++++++++++++++++++++
 .../spi/fsl,spi-dspi-peripheral-props.yaml         |  28 +++++
 .../devicetree/bindings/spi/spi-fsl-dspi.txt       |  65 ------------
 .../bindings/spi/spi-peripheral-props.yaml         |   1 +
 4 files changed, 144 insertions(+), 65 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/fsl,dspi.yaml b/Documentation/devicetree/bindings/spi/fsl,dspi.yaml
new file mode 100644
index 0000000000000..924ba19aea017
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/fsl,dspi.yaml
@@ -0,0 +1,115 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spi/fsl,dspi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ARM Freescale DSPI controller
+
+maintainers:
+  - Frank Li <Frank.Li@nxp.com>
+
+properties:
+  compatible:
+    oneOf:
+      - enum:
+          - fsl,vf610-dspi
+          - fsl,ls1021a-v1.0-dspi
+          - fsl,ls1012a-dspi
+          - fsl,ls1028a-dspi
+          - fsl,ls1043a-dspi
+          - fsl,ls1046a-dspi
+          - fsl,ls1088a-dspi
+          - fsl,ls2080a-dspi
+          - fsl,ls2085a-dspi
+          - fsl,lx2160a-dspi
+      - items:
+          - enum:
+              - fsl,ls1012a-dspi
+              - fsl,ls1028a-dspi
+              - fsl,ls1043a-dspi
+              - fsl,ls1046a-dspi
+              - fsl,ls1088a-dspi
+          - const: fsl,ls1021a-v1.0-dspi
+      - items:
+          - const: fsl,ls2080a-dspi
+          - const: fsl,ls2085a-dspi
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: dspi
+
+  pinctrl-0: true
+
+  pinctrl-names:
+    items:
+      - const: default
+
+  spi-num-chipselects:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: the number of the chipselect signals.
+
+  big-endian:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      If present the dspi device's registers are implemented
+      in big endian mode.
+
+  bus-num:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: the slave chip chipselect signal number.
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - interrupts
+  - pinctrl-0
+  - pinctrl-names
+  - spi-num-chipselects
+
+allOf:
+  - $ref: spi-controller.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/vf610-clock.h>
+
+    spi@4002c000 {
+        compatible = "fsl,vf610-dspi";
+        reg = <0x4002c000 0x1000>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+        interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&clks VF610_CLK_DSPI0>;
+        clock-names = "dspi";
+        spi-num-chipselects = <5>;
+        bus-num = <0>;
+        pinctrl-names = "default";
+        pinctrl-0 = <&pinctrl_dspi0_1>;
+        big-endian;
+
+        flash@0 {
+                compatible = "jedec,spi-nor";
+                reg = <0>;
+                spi-max-frequency = <16000000>;
+                spi-cpol;
+                spi-cpha;
+                spi-cs-setup-delay-ns = <100>;
+                spi-cs-hold-delay-ns = <50>;
+        };
+    };
+
diff --git a/Documentation/devicetree/bindings/spi/fsl,spi-dspi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/fsl,spi-dspi-peripheral-props.yaml
new file mode 100644
index 0000000000000..ea9c7c52c1883
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/fsl,spi-dspi-peripheral-props.yaml
@@ -0,0 +1,28 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/spi/fsl,spi-dspi-peripheral-props.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Peripheral-specific properties for Freescale DSPI controller
+
+maintainers:
+  - Vladimir Oltean <olteanv@gmail.com>
+
+description:
+  See spi-peripheral-props.yaml for more info.
+
+properties:
+  fsl,spi-cs-sck-delay:
+    description:
+      Delay in nanoseconds between activating chip select and the start of
+      clock signal, at the start of a transfer.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  fsl,spi-sck-cs-delay:
+    description:
+      Delay in nanoseconds between stopping the clock signal and
+      deactivating chip select, at the end of a transfer.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+additionalProperties: true
diff --git a/Documentation/devicetree/bindings/spi/spi-fsl-dspi.txt b/Documentation/devicetree/bindings/spi/spi-fsl-dspi.txt
deleted file mode 100644
index 30a79da9c039d..0000000000000
--- a/Documentation/devicetree/bindings/spi/spi-fsl-dspi.txt
+++ /dev/null
@@ -1,65 +0,0 @@
-ARM Freescale DSPI controller
-
-Required properties:
-- compatible : must be one of:
-	"fsl,vf610-dspi",
-	"fsl,ls1021a-v1.0-dspi",
-	"fsl,ls1012a-dspi" (optionally followed by "fsl,ls1021a-v1.0-dspi"),
-	"fsl,ls1028a-dspi",
-	"fsl,ls1043a-dspi" (optionally followed by "fsl,ls1021a-v1.0-dspi"),
-	"fsl,ls1046a-dspi" (optionally followed by "fsl,ls1021a-v1.0-dspi"),
-	"fsl,ls1088a-dspi" (optionally followed by "fsl,ls1021a-v1.0-dspi"),
-	"fsl,ls2080a-dspi" (optionally followed by "fsl,ls2085a-dspi"),
-	"fsl,ls2085a-dspi",
-	"fsl,lx2160a-dspi",
-- reg : Offset and length of the register set for the device
-- interrupts : Should contain SPI controller interrupt
-- clocks: from common clock binding: handle to dspi clock.
-- clock-names: from common clock binding: Shall be "dspi".
-- pinctrl-0: pin control group to be used for this controller.
-- pinctrl-names: must contain a "default" entry.
-- spi-num-chipselects : the number of the chipselect signals.
-
-Optional property:
-- big-endian: If present the dspi device's registers are implemented
-  in big endian mode.
-- bus-num : the slave chip chipselect signal number.
-
-Optional SPI slave node properties:
-- fsl,spi-cs-sck-delay: a delay in nanoseconds between activating chip
-  select and the start of clock signal, at the start of a transfer.
-- fsl,spi-sck-cs-delay: a delay in nanoseconds between stopping the clock
-  signal and deactivating chip select, at the end of a transfer.
-
-Example:
-
-dspi0@4002c000 {
-	#address-cells = <1>;
-	#size-cells = <0>;
-	compatible = "fsl,vf610-dspi";
-	reg = <0x4002c000 0x1000>;
-	interrupts = <0 67 0x04>;
-	clocks = <&clks VF610_CLK_DSPI0>;
-	clock-names = "dspi";
-	spi-num-chipselects = <5>;
-	bus-num = <0>;
-	pinctrl-names = "default";
-	pinctrl-0 = <&pinctrl_dspi0_1>;
-	big-endian;
-
-	sflash: at26df081a@0 {
-		#address-cells = <1>;
-		#size-cells = <1>;
-		compatible = "atmel,at26df081a";
-		spi-max-frequency = <16000000>;
-		spi-cpol;
-		spi-cpha;
-		reg = <0>;
-		linux,modalias = "m25p80";
-		modal = "at26df081a";
-		fsl,spi-cs-sck-delay = <100>;
-		fsl,spi-sck-cs-delay = <50>;
-	};
-};
-
-
diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
index 15938f81fdce2..fcc39a04a8b7a 100644
--- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
@@ -122,6 +122,7 @@ properties:
 allOf:
   - $ref: arm,pl022-peripheral-props.yaml#
   - $ref: cdns,qspi-nor-peripheral-props.yaml#
+  - $ref: fsl,spi-dspi-peripheral-props.yaml#
   - $ref: samsung,spi-peripheral-props.yaml#
   - $ref: nvidia,tegra210-quad-peripheral-props.yaml#
 

-- 
2.34.1

Re: [PATCH v3 2/3] spi: dt-bindings: fsl-dspi: Convert to yaml format

[Vladimir Oltean]

On Thu, Jun 20, 2024 at 12:58:28PM -0400, Frank Li wrote:
> Convert dt-binding spi-fsl-dspi.txt to yaml format.
> 
> Addtional changes during convert:
> - compatible string "fsl,ls1028a-dspi" can be followed by
> fsl,ls1021a-v1.0-dspi.
> - Change "dspi0@4002c000" to "spi@4002c000" in example.
> - Reorder properties in example.
> - Use GIC include in example.
> - Remove fsl,spi-cs-sck-delay and fsl,spi-sck-cs-delay by use common SPI
> property.
> - Use compatible string 'jedec,spi-nor' in example.
> - Split peripheral part to fsl,spi-dspi-peripheral-props.yaml
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> 
> ---
> Use part of Vladimir Oltean's work at
> https://lore.kernel.org/linux-spi/20221111224651.577729-1-vladimir.oltean@nxp.com/

Hm, you took part of that but gave no attribution? The portion below ---
is also discarded when the patch is applied, so even the link is lost,
FYI.

> ---
>  .../devicetree/bindings/spi/fsl,dspi.yaml          | 115 +++++++++++++++++++++
>  .../spi/fsl,spi-dspi-peripheral-props.yaml         |  28 +++++

For consistency, could you name this fsl,dspi-peripheral-props.yaml?

>  .../devicetree/bindings/spi/spi-fsl-dspi.txt       |  65 ------------
>  .../bindings/spi/spi-peripheral-props.yaml         |   1 +

No MAINTAINERS change for the schema path? There was a discussion with
Krzysztof in the old thread.

>  4 files changed, 144 insertions(+), 65 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/spi/fsl,dspi.yaml b/Documentation/devicetree/bindings/spi/fsl,dspi.yaml
> new file mode 100644
> index 0000000000000..924ba19aea017
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spi/fsl,dspi.yaml
> @@ -0,0 +1,115 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/spi/fsl,dspi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ARM Freescale DSPI controller
> +
> +maintainers:
> +  - Frank Li <Frank.Li@nxp.com>
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - enum:
> +          - fsl,vf610-dspi
> +          - fsl,ls1021a-v1.0-dspi
> +          - fsl,ls1012a-dspi
> +          - fsl,ls1028a-dspi
> +          - fsl,ls1043a-dspi
> +          - fsl,ls1046a-dspi
> +          - fsl,ls1088a-dspi
> +          - fsl,ls2080a-dspi
> +          - fsl,ls2085a-dspi
> +          - fsl,lx2160a-dspi
> +      - items:
> +          - enum:
> +              - fsl,ls1012a-dspi
> +              - fsl,ls1028a-dspi
> +              - fsl,ls1043a-dspi
> +              - fsl,ls1046a-dspi
> +              - fsl,ls1088a-dspi
> +          - const: fsl,ls1021a-v1.0-dspi
> +      - items:
> +          - const: fsl,ls2080a-dspi
> +          - const: fsl,ls2085a-dspi
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: dspi
> +
> +  pinctrl-0: true
> +
> +  pinctrl-names:
> +    items:
> +      - const: default

I don't think that pinctrl properties need to be specified in the
schema. Somehow, I think dt-schema applies
dtschema/schemas/pinctrl/pinctrl-consumer.yaml by default every time.

> +
> +  spi-num-chipselects:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: the number of the chipselect signals.

Worth mentioning that this is about _native_ chip select signals.
cs-gpios don't count against this number.

> +
> +  big-endian:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      If present the dspi device's registers are implemented
> +      in big endian mode.

I'm not sure that this needs an explanation, it is an absolutely generic
property with a universal meaning.

> +
> +  bus-num:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: the slave chip chipselect signal number.

In fact, no, this is not a chip select number, the old documentation is
wrong. It just gets assigned to the struct spi_controller :: bus_num.
In my last submitted version I wrote "SoC-specific identifier for the
SPI controller", that seems perfectly adequate.

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - interrupts
> +  - pinctrl-0
> +  - pinctrl-names

interrupts and pinctrl are not required.

> +  - spi-num-chipselects
> +
> +allOf:
> +  - $ref: spi-controller.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/vf610-clock.h>
> +
> +    spi@4002c000 {
> +        compatible = "fsl,vf610-dspi";
> +        reg = <0x4002c000 0x1000>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>;
> +        clocks = <&clks VF610_CLK_DSPI0>;
> +        clock-names = "dspi";
> +        spi-num-chipselects = <5>;
> +        bus-num = <0>;
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&pinctrl_dspi0_1>;
> +        big-endian;
> +
> +        flash@0 {
> +                compatible = "jedec,spi-nor";
> +                reg = <0>;
> +                spi-max-frequency = <16000000>;
> +                spi-cpol;
> +                spi-cpha;
> +                spi-cs-setup-delay-ns = <100>;
> +                spi-cs-hold-delay-ns = <50>;
> +        };
> +    };
> +

Please remove newline at end of file.

Re: [PATCH v3 2/3] spi: dt-bindings: fsl-dspi: Convert to yaml format

[Frank Li]

On Fri, Jun 21, 2024 at 03:42:11PM +0300, Vladimir Oltean wrote:
> On Thu, Jun 20, 2024 at 12:58:28PM -0400, Frank Li wrote:
> > Convert dt-binding spi-fsl-dspi.txt to yaml format.
> > 
> > Addtional changes during convert:
> > - compatible string "fsl,ls1028a-dspi" can be followed by
> > fsl,ls1021a-v1.0-dspi.
> > - Change "dspi0@4002c000" to "spi@4002c000" in example.
> > - Reorder properties in example.
> > - Use GIC include in example.
> > - Remove fsl,spi-cs-sck-delay and fsl,spi-sck-cs-delay by use common SPI
> > property.
> > - Use compatible string 'jedec,spi-nor' in example.
> > - Split peripheral part to fsl,spi-dspi-peripheral-props.yaml
> > 
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > 
> > ---
> > Use part of Vladimir Oltean's work at
> > https://lore.kernel.org/linux-spi/20221111224651.577729-1-vladimir.oltean@nxp.com/
> 
> Hm, you took part of that but gave no attribution? The portion below ---
> is also discarded when the patch is applied, so even the link is lost,
> FYI.

I am not sure what should be added in comments? 

Ref part of Vladimir Oltean's work at https:// ... (I am not sure if allow
http link).
 
Or coworked-with Vladimir Oltean ...
Or use seperated patch for your part.

What do you like?

Frank

> 
> > ---
> >  .../devicetree/bindings/spi/fsl,dspi.yaml          | 115 +++++++++++++++++++++
> >  .../spi/fsl,spi-dspi-peripheral-props.yaml         |  28 +++++
> 
> For consistency, could you name this fsl,dspi-peripheral-props.yaml?
> 
> >  .../devicetree/bindings/spi/spi-fsl-dspi.txt       |  65 ------------
> >  .../bindings/spi/spi-peripheral-props.yaml         |   1 +
> 
> No MAINTAINERS change for the schema path? There was a discussion with
> Krzysztof in the old thread.
> 
> >  4 files changed, 144 insertions(+), 65 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/spi/fsl,dspi.yaml b/Documentation/devicetree/bindings/spi/fsl,dspi.yaml
> > new file mode 100644
> > index 0000000000000..924ba19aea017
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/spi/fsl,dspi.yaml
> > @@ -0,0 +1,115 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/spi/fsl,dspi.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ARM Freescale DSPI controller
> > +
> > +maintainers:
> > +  - Frank Li <Frank.Li@nxp.com>
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - enum:
> > +          - fsl,vf610-dspi
> > +          - fsl,ls1021a-v1.0-dspi
> > +          - fsl,ls1012a-dspi
> > +          - fsl,ls1028a-dspi
> > +          - fsl,ls1043a-dspi
> > +          - fsl,ls1046a-dspi
> > +          - fsl,ls1088a-dspi
> > +          - fsl,ls2080a-dspi
> > +          - fsl,ls2085a-dspi
> > +          - fsl,lx2160a-dspi
> > +      - items:
> > +          - enum:
> > +              - fsl,ls1012a-dspi
> > +              - fsl,ls1028a-dspi
> > +              - fsl,ls1043a-dspi
> > +              - fsl,ls1046a-dspi
> > +              - fsl,ls1088a-dspi
> > +          - const: fsl,ls1021a-v1.0-dspi
> > +      - items:
> > +          - const: fsl,ls2080a-dspi
> > +          - const: fsl,ls2085a-dspi
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    items:
> > +      - const: dspi
> > +
> > +  pinctrl-0: true
> > +
> > +  pinctrl-names:
> > +    items:
> > +      - const: default
> 
> I don't think that pinctrl properties need to be specified in the
> schema. Somehow, I think dt-schema applies
> dtschema/schemas/pinctrl/pinctrl-consumer.yaml by default every time.
> 
> > +
> > +  spi-num-chipselects:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: the number of the chipselect signals.
> 
> Worth mentioning that this is about _native_ chip select signals.
> cs-gpios don't count against this number.
> 
> > +
> > +  big-endian:
> > +    $ref: /schemas/types.yaml#/definitions/flag
> > +    description:
> > +      If present the dspi device's registers are implemented
> > +      in big endian mode.
> 
> I'm not sure that this needs an explanation, it is an absolutely generic
> property with a universal meaning.
> 
> > +
> > +  bus-num:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: the slave chip chipselect signal number.
> 
> In fact, no, this is not a chip select number, the old documentation is
> wrong. It just gets assigned to the struct spi_controller :: bus_num.
> In my last submitted version I wrote "SoC-specific identifier for the
> SPI controller", that seems perfectly adequate.
> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - interrupts
> > +  - pinctrl-0
> > +  - pinctrl-names
> 
> interrupts and pinctrl are not required.
> 
> > +  - spi-num-chipselects
> > +
> > +allOf:
> > +  - $ref: spi-controller.yaml#
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/clock/vf610-clock.h>
> > +
> > +    spi@4002c000 {
> > +        compatible = "fsl,vf610-dspi";
> > +        reg = <0x4002c000 0x1000>;
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +        interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>;
> > +        clocks = <&clks VF610_CLK_DSPI0>;
> > +        clock-names = "dspi";
> > +        spi-num-chipselects = <5>;
> > +        bus-num = <0>;
> > +        pinctrl-names = "default";
> > +        pinctrl-0 = <&pinctrl_dspi0_1>;
> > +        big-endian;
> > +
> > +        flash@0 {
> > +                compatible = "jedec,spi-nor";
> > +                reg = <0>;
> > +                spi-max-frequency = <16000000>;
> > +                spi-cpol;
> > +                spi-cpha;
> > +                spi-cs-setup-delay-ns = <100>;
> > +                spi-cs-hold-delay-ns = <50>;
> > +        };
> > +    };
> > +
> 
> Please remove newline at end of file.

Re: [PATCH v3 2/3] spi: dt-bindings: fsl-dspi: Convert to yaml format

[Vladimir Oltean]

On Fri, Jun 21, 2024 at 10:39:33AM -0400, Frank Li wrote:
> On Fri, Jun 21, 2024 at 03:42:11PM +0300, Vladimir Oltean wrote:
> > > Use part of Vladimir Oltean's work at
> > > https://lore.kernel.org/linux-spi/20221111224651.577729-1-vladimir.oltean@nxp.com/
> > 
> > Hm, you took part of that but gave no attribution? The portion below ---
> > is also discarded when the patch is applied, so even the link is lost,
> > FYI.
> 
> I am not sure what should be added in comments? 
> 
> Ref part of Vladimir Oltean's work at https:// ... (I am not sure if allow
> http link).
>  
> Or coworked-with Vladimir Oltean ...
> Or use seperated patch for your part.
> 
> What do you like?
> 
> Frank

Well, it depends on how much from that work you end up taking.
I would recommend taking all of it :) and completing the unfinalized
review feedback with your work, plus some additions like the standard
CS timing parameters. Then, you could keep that sign off chain and add
yours at the bottom. At least that was my approach when I tried to
continue Kuldeep's work.

If you take a very small portion, I guess you could at least move the
link inside the portion of the commit message that remains visible in
the commit log.

And for a significant portion, Documentation/process/submitting-patches.rst
says you could add Co-developed-by + Signed-off-by, followed by your
sign off.

[PATCH v3 3/3] arm64: dts: fsl-ls1043a-rdb: use common spi-cs-setup(hold)-delay-ns

[Frank Li]

Use SPI common propteries 'spi-cs-setup-delay-ns' and
'spi-cs-hold-delay-ns', mark private properties 'fsl,spi-cs-sck-delay'
and 'fsl,spi-sck-cs-delay' as depericated.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 arch/arm64/boot/dts/freescale/fsl-ls1043a-rdb.dts | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043a-rdb.dts b/arch/arm64/boot/dts/freescale/fsl-ls1043a-rdb.dts
index 26f8540cb101b..fef3534157a1d 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1043a-rdb.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1043a-rdb.dts
@@ -104,6 +104,11 @@ flash@0 {
 		compatible = "n25q128a13", "jedec,spi-nor";  /* 16MB */
 		reg = <0>;
 		spi-max-frequency = <1000000>; /* input clock */
+		/* Add common SPI property */
+		spi-cs-setup-delay-ns = <100>;
+		spi-cs-hold-delay-ns = <100>;
+
+		/* Deprecated below property */
 		fsl,spi-cs-sck-delay = <100>;
 		fsl,spi-sck-cs-delay = <100>;
 	};
@@ -112,6 +117,11 @@ slic@2 {
 		compatible = "maxim,ds26522";
 		reg = <2>;
 		spi-max-frequency = <2000000>;
+		/* Add common SPI property */
+		spi-cs-setup-delay-ns = <100>;
+		spi-cs-hold-delay-ns = <50>;
+
+		/* Deprecated below property */
 		fsl,spi-cs-sck-delay = <100>;
 		fsl,spi-sck-cs-delay = <50>;
 	};
@@ -120,6 +130,11 @@ slic@3 {
 		compatible = "maxim,ds26522";
 		reg = <3>;
 		spi-max-frequency = <2000000>;
+		/* Add common SPI property */
+		spi-cs-setup-delay-ns = <100>;
+		spi-cs-hold-delay-ns = <50>;
+
+		/* Deprecated below property */
 		fsl,spi-cs-sck-delay = <100>;
 		fsl,spi-sck-cs-delay = <50>;
 	};

-- 
2.34.1

Re: [PATCH v3 3/3] arm64: dts: fsl-ls1043a-rdb: use common spi-cs-setup(hold)-delay-ns

[Vladimir Oltean]

On Thu, Jun 20, 2024 at 12:58:29PM -0400, Frank Li wrote:
> Use SPI common propteries 'spi-cs-setup-delay-ns' and

properties

> 'spi-cs-hold-delay-ns', mark private properties 'fsl,spi-cs-sck-delay'
> and 'fsl,spi-sck-cs-delay' as depericated.

deprecated

> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  arch/arm64/boot/dts/freescale/fsl-ls1043a-rdb.dts | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1043a-rdb.dts b/arch/arm64/boot/dts/freescale/fsl-ls1043a-rdb.dts
> index 26f8540cb101b..fef3534157a1d 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls1043a-rdb.dts
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1043a-rdb.dts
> @@ -104,6 +104,11 @@ flash@0 {
>  		compatible = "n25q128a13", "jedec,spi-nor";  /* 16MB */
>  		reg = <0>;
>  		spi-max-frequency = <1000000>; /* input clock */
> +		/* Add common SPI property */

I guess if you do insist on adding comments for this (not necessary IMO),
we could improve on the stylistic elegance by saying "Standard CS timing
properties replace the deprecated vendor variants below" or something
like that. But currently it doesn't look great when there is a verb in
the present tense and another in the past tense. Also, the plural form
should have been used for 'property'.

Additionally, I don't believe that a new line between the groups of
properties is necessary.

> +		spi-cs-setup-delay-ns = <100>;
> +		spi-cs-hold-delay-ns = <100>;

Do you plan on deprecating the fsl, CS timing parameters tree-wide, or
are you going to stop at the LS1043A-RDB?