Re: [PATCH 3/5] spi: core: allow defining time that cs is deasserted as a multiple of SCK

[Lukas Wunner <lukas@xxxxxxxxx>]

On Sat, Feb 23, 2019 at 08:49:50AM +0000, kernel@xxxxxxxxxxxxxxxx wrote:
> Support setting a delay between cs assert and deassert as
> a multiple of spi clock length.

To what end?


> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -1092,6 +1092,7 @@ static void _spi_transfer_cs_change_delay(struct spi_message *msg,
>  {
>  	u32 delay = xfer->cs_change_delay;
>  	u32 unit = xfer->cs_change_delay_unit;
> +	u32 hz;
> 
>  	/* return early on "fast" mode - for everything but USECS */
>  	if (!delay && unit != SPI_DELAY_UNIT_USECS)
> @@ -1107,6 +1108,13 @@ static void _spi_transfer_cs_change_delay(struct spi_message *msg,
>  		break;
>  	case SPI_DELAY_UNIT_NSECS: /* nothing to do here */
>  		break;
> +	case SPI_DELAY_UNIT_SCK:
> +		/* if there is no effective speed know, then approximate
> +		 * by underestimating with half the requested hz
> +		 */
> +		hz = xfer->effective_speed_hz ?: xfer->speed_hz / 2;
> +		delay *= DIV_ROUND_UP(1000000000, hz);
> +		break;
>  	default:
>  		dev_err_once(&msg->spi->dev,
>  			     "Use of unsupported delay unit %i, using default of 10us\n",
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index f503a712423d..0b1d5e2b8b8b 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -799,6 +799,7 @@ struct spi_transfer {
>  	u8		cs_change_delay_unit;
>  #define SPI_DELAY_UNIT_USECS	0
>  #define SPI_DELAY_UNIT_NSECS	1
> +#define SPI_DELAY_UNIT_SCK	2

The ability to define a unit seems somewhat over-engineered to me.
You're introducing lots of branching and integer divisions (both
comparatively expensive operations) which are executed for every
single transfer (i.e. in a hot path).  I'd recommend standardizing
on a single unit.  If 1 usec is too coarse-grained for your use case,
convert everything to nsec.

Patch [5/5] in this series didn't make it through to linux-rpi-kernel,
this has happened to my own patches as well in the past:  They're put in
a moderation queue because the number of recipients exceeds a limit,
but noone ever releases them from the moderation queue, which is
unfortunate.

Thanks,

Lukas