diff mbox series

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

Message ID 20240620-ls_qspi-v3-1-1a2afcf417e4@nxp.com (mailing list archive)
State Superseded
Headers show
Series spi: fsl-dspi: Convert to yaml format and use common SPI property | expand

Commit Message

Frank Li June 20, 2024, 4:58 p.m. UTC
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(-)

Comments

Peng Fan June 21, 2024, 1:28 a.m. UTC | #1
> 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
>
Vladimir Oltean June 21, 2024, 10:22 a.m. UTC | #2
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.
Vladimir Oltean June 21, 2024, 11:26 a.m. UTC | #3
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.
diff mbox series

Patch

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;