Re: [PATCH] spi: imx: Fix the number of chipselects count

[Matthias Schiffer <matthias.schiffer@xxxxxxxxxxxxxxx>]

On Sun, 2020-09-13 at 11:19 -0300, Fabio Estevam wrote:
> On an imx6q-sabresd board, which has one single GPIO chipselect used
> for SPI, the SPI core now reports that it has 3 chipselects.
> 
> This happens since commit 8cdcd8aeee281 ("spi: imx/fsl-lpspi: Convert
> to
> GPIO descriptors"), which assigned master->num_chipselect as 3 when
> 'num-cs' is absent.
> 
> However, 'num-cs' property is not used in any in-tree devicetree,
> leading
> to an incorrect count of chipselects.
> 
> Fix the number of chipselects count by only assigning
> master->num_chipselect in the unlikely case when the "cs-gpios"
> property
> is absent.
> 
> Print a warning in this case, since using native chipselects in
> several i.MX SoCs is known to be problematic.
> 
> While at it, use 4 as the maximum number of native chip-select
> supported
> by this controller.
> 
> Fixes: 8cdcd8aeee281 ("spi: imx/fsl-lpspi: Convert to GPIO
> descriptors")
> Signed-off-by: Fabio Estevam <festevam@xxxxxxxxx>

Hello Fabio,

thanks for your patch. I had thought about doing something similiar,
but ultimately decided to go with the simpler patch I submitted as
"spi-imx: remove num-cs support, set num_chipselect to 4" for two
reasons:

- none of the other SPI drivers care about the number of cs-gpios when
setting num_chipselect
- AFAICT, using GPIO CS only for some indices, and native ones for the
rest should work fine; as I understand it now, there is no reason to
make num_chpselect smaller than the number of native CS (so the
'num_chipselect = max(num_chipselect, number of cs-gpios)' in the SPI
core is working as intended)

For these reasons I believe that my proposed patch is more in line with
the usual way to handle num_chipselect (adding a warning still seems
like a good idea.)


FWIW, I've researched for which usecases we use the native CS of i.MX
SoCs:

As you write, the native CS of these SPI controllers is problematic for
most usecases - it seems to me that CS is not asserted across the read
and write portions of what is supposed a single transaction.

However, there are ADCs that do not need a command, but will start
sending data as soon as CS is asserted. For this kind of communication,
the native CS works fine, and may actually be preferable for latency-
critical applications, as locking is required to set GPIOs on the i.MX
platform.

Kind regards,
Matthias




> ---
>  drivers/spi/spi-imx.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 197f60632072..968c868cf17f 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -13,6 +13,7 @@
>  #include <linux/irq.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/of_gpio.h>
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> @@ -1581,7 +1582,7 @@ static int spi_imx_probe(struct platform_device
> *pdev)
>  	const struct spi_imx_devtype_data *devtype_data = of_id ?
> of_id->data :
>  		(struct spi_imx_devtype_data *)pdev->id_entry-
> >driver_data;
>  	bool slave_mode;
> -	u32 val;
> +	int num_cs_gpios;
>  
>  	slave_mode = devtype_data->has_slavemode &&
>  			of_property_read_bool(np, "spi-slave");
> @@ -1613,16 +1614,12 @@ static int spi_imx_probe(struct
> platform_device *pdev)
>  
>  	spi_imx->devtype_data = devtype_data;
>  
> -	/*
> -	 * Get number of chip selects from device properties. This can
> be
> -	 * coming from device tree or boardfiles, if it is not defined,
> -	 * a default value of 3 chip selects will be used, as all the
> legacy
> -	 * board files have <= 3 chip selects.
> -	 */
> -	if (!device_property_read_u32(&pdev->dev, "num-cs", &val))
> -		master->num_chipselect = val;
> -	else
> -		master->num_chipselect = 3;
> +	num_cs_gpios = gpiod_count(&pdev->dev, "cs");
> +	if ((num_cs_gpios == 0) || (num_cs_gpios == -ENOENT)) {
> +		dev_warn(&pdev->dev,
> +			 "cs-gpios not found. Using native chipselect
> with this SPI controller is known to be problematic\n");
> +		master->num_chipselect = 4;
> +	}
>  
>  	spi_imx->bitbang.setup_transfer = spi_imx_setupxfer;
>  	spi_imx->bitbang.txrx_bufs = spi_imx_transfer;