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

[Vladimir Oltean <olteanv@xxxxxxxxx>]

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.