| Message ID | 20200913141937.11588-1-festevam@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: imx: Fix the number of chipselects count | expand |
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@gmail.com> 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;
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;
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@gmail.com> --- drivers/spi/spi-imx.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)