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.
