[PATCH 0/3] spi-gpio updates

[PATCH 0/3] spi-gpio updates

[Sascha Hauer]

Our spio-gpio driver currently only supports 8bit word width. This
series updates the driver to support arbitrary word widths up to 32bit.
Also we get rid of the deprecated GPIO binding that the driver uses.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
Sascha Hauer (3):
      spi: spi-gpio: actually delay in spidelay()
      spi: spi-gpio: support different word widths
      spi: spi-gpio: switch to new gpio binding

 arch/arm/dts/stm32mp151-mect1s.dts |  6 ++--
 drivers/spi/gpio_spi.c             | 69 +++++++++++++++++++++++++++++++-------
 2 files changed, 59 insertions(+), 16 deletions(-)
---
base-commit: 419ea9350aa083d4a2806a70132129a49a5ecf95
change-id: 20240925-spi-gpio-4e65be1f7b90

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>

[PATCH 1/3] spi: spi-gpio: actually delay in spidelay()

[Sascha Hauer]

spidelay() currently only is a no-op dummy function. Actually delay in
this function to avoid spi-gpio being faster than specified.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/spi/gpio_spi.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/gpio_spi.c b/drivers/spi/gpio_spi.c
index e5664df3fe..a71b4eddab 100644
--- a/drivers/spi/gpio_spi.c
+++ b/drivers/spi/gpio_spi.c
@@ -48,7 +48,10 @@ static inline int getmiso(const struct spi_device *spi)
 	return !!gpio_get_value(priv->data->miso);
 }
 
-#define spidelay(nsecs) do { } while (0)
+static inline void spidelay(unsigned int nsecs)
+{
+	udelay(max(1U, nsecs / 1000));
+}
 
 #include "spi-bitbang-txrx.h"
 

-- 
2.39.5

[PATCH 2/3] spi: spi-gpio: support different word widths

[Sascha Hauer]

The spi-gpio driver only supports 8bit word width. Add support for
arbitrary word widths up to 32bit.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/spi/gpio_spi.c | 58 ++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 49 insertions(+), 9 deletions(-)

diff --git a/drivers/spi/gpio_spi.c b/drivers/spi/gpio_spi.c
index a71b4eddab..c76b71f610 100644
--- a/drivers/spi/gpio_spi.c
+++ b/drivers/spi/gpio_spi.c
@@ -78,7 +78,7 @@ static inline u32 gpio_spi_txrx_word(struct spi_device *spi, unsigned nsecs,
 		return bitbang_txrx_be_cpha0(spi, nsecs, cpol, word, bits);
 }
 
-static int gpio_spi_transfer_one(struct spi_device *spi, struct spi_transfer *t)
+static int gpio_spi_transfer_one_u8(struct spi_device *spi, struct spi_transfer *t)
 {
 	bool read = (t->rx_buf) ? true : false;
 	u32 word = 0;
@@ -87,14 +87,59 @@ static int gpio_spi_transfer_one(struct spi_device *spi, struct spi_transfer *t)
 	for (n = 0; n < t->len; n++) {
 		if (!read)
 			word = ((const u8 *)t->tx_buf)[n];
-		word = gpio_spi_txrx_word(spi, 0, word, 8);
+		word = gpio_spi_txrx_word(spi, 0, word, spi->bits_per_word);
+		if (read)
+			((u8 *)t->rx_buf)[n] = word;
+	}
+
+	return 0;
+}
+
+static int gpio_spi_transfer_one_u16(struct spi_device *spi, struct spi_transfer *t)
+{
+	bool read = (t->rx_buf) ? true : false;
+	u32 word = 0;
+	int n;
+
+	for (n = 0; n < t->len / 2; n++) {
+		if (!read)
+			word = ((const u16 *)t->tx_buf)[n];
+		word = gpio_spi_txrx_word(spi, 0, word, spi->bits_per_word);
 		if (read)
-			((u8 *)t->rx_buf)[n] = word & 0xff;
+			((u16 *)t->rx_buf)[n] = word;
 	}
 
 	return 0;
 }
 
+static int gpio_spi_transfer_one_u32(struct spi_device *spi, struct spi_transfer *t)
+{
+	bool read = (t->rx_buf) ? true : false;
+	u32 word = 0;
+	int n;
+
+	for (n = 0; n < t->len / 4; n++) {
+		if (!read)
+			word = ((const u32 *)t->tx_buf)[n];
+		word = gpio_spi_txrx_word(spi, 0, word, spi->bits_per_word);
+		if (read)
+			((u32 *)t->rx_buf)[n] = word;
+	}
+
+	return 0;
+}
+
+static int gpio_spi_transfer_one(struct spi_device *spi, struct spi_transfer *t)
+{
+	if (spi->bits_per_word <= 8)
+		return gpio_spi_transfer_one_u8(spi, t);
+	if (spi->bits_per_word <= 16)
+		return gpio_spi_transfer_one_u16(spi, t);
+	if (spi->bits_per_word <= 32)
+		return gpio_spi_transfer_one_u32(spi, t);
+	return -EINVAL;
+}
+
 static int gpio_spi_transfer(struct spi_device *spi, struct spi_message *msg)
 {
 	struct spi_transfer *t;
@@ -120,12 +165,7 @@ static int gpio_spi_transfer(struct spi_device *spi, struct spi_message *msg)
 
 static int gpio_spi_setup(struct spi_device *spi)
 {
-	if (spi->bits_per_word != 8) {
-		dev_err(spi->master->dev, "master does not support %d bits per word\n",
-			spi->bits_per_word);
-		return -EINVAL;
-	}
-
+	gpio_spi_set_cs(spi, 0);
 	return 0;
 }
 

-- 
2.39.5

[PATCH 3/3] spi: spi-gpio: switch to new gpio binding

[Sascha Hauer]

The old deprecated device tree binding for the spi-gpio driver uses
"gpio-sck", "gpio-mosi" and "gpio-miso" to specify the GPIOs. Switch to
the new binding which uses the standard GPIO property names.

The old binding is still used in some device trees, but none of the SoCs
using them is actually supported in barebox, so do not bother to keep a
fallback to the old binding. The one in-tree user of the old binding is
converted in this patch.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/dts/stm32mp151-mect1s.dts | 6 +++---
 drivers/spi/gpio_spi.c             | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm/dts/stm32mp151-mect1s.dts b/arch/arm/dts/stm32mp151-mect1s.dts
index 4d0e31d189..b0dc1cfaa8 100644
--- a/arch/arm/dts/stm32mp151-mect1s.dts
+++ b/arch/arm/dts/stm32mp151-mect1s.dts
@@ -65,9 +65,9 @@ led-1 {
 
 	spi_gpio: spi-gpio-0 {
 		compatible = "spi-gpio";
-		gpio-sck = <&gpioi 1 GPIO_ACTIVE_HIGH>;
-		gpio-mosi = <&gpioi 3 GPIO_ACTIVE_HIGH>;
-		gpio-miso = <&gpioi 2 GPIO_ACTIVE_HIGH>;
+		sck-gpios = <&gpioi 1 GPIO_ACTIVE_HIGH>;
+		mosi-gpios = <&gpioi 3 GPIO_ACTIVE_HIGH>;
+		miso-gpios = <&gpioi 2 GPIO_ACTIVE_HIGH>;
 		cs-gpios = <&gpioj 3 GPIO_ACTIVE_LOW>;
 		num-chipselects = <1>;
 		#address-cells = <1>;
diff --git a/drivers/spi/gpio_spi.c b/drivers/spi/gpio_spi.c
index c76b71f610..34d74faf4c 100644
--- a/drivers/spi/gpio_spi.c
+++ b/drivers/spi/gpio_spi.c
@@ -178,7 +178,7 @@ static int gpio_spi_of_probe(struct device *dev)
 	if (!IS_ENABLED(CONFIG_OFDEVICE) || dev->platform_data)
 		return 0;
 
-	sck = of_get_named_gpio(np, "gpio-sck", 0);
+	sck = of_get_named_gpio(np, "sck-gpios", 0);
 	if (!gpio_is_valid(sck))
 		return dev_err_probe(dev, sck < 0 ? sck : -EINVAL,
 				     "missing mandatory SCK gpio\n");
@@ -187,11 +187,11 @@ static int gpio_spi_of_probe(struct device *dev)
 	pdata->sck = sck;
 	pdata->num_cs = MAX_CHIPSELECT;
 
-	pdata->miso = of_get_named_gpio(np, "gpio-miso", 0);
+	pdata->miso = of_get_named_gpio(np, "miso-gpios", 0);
 	if (!gpio_is_valid(pdata->miso))
 		pdata->miso = SPI_GPIO_NO_MISO;
 
-	pdata->mosi = of_get_named_gpio(np, "gpio-mosi", 0);
+	pdata->mosi = of_get_named_gpio(np, "mosi-gpios", 0);
 	if (!gpio_is_valid(pdata->mosi))
 		pdata->mosi = SPI_GPIO_NO_MOSI;
 

-- 
2.39.5

Re: [PATCH 1/3] spi: spi-gpio: actually delay in spidelay()

[Ahmad Fatoum]

On 25.09.24 16:06, Sascha Hauer wrote:
> spidelay() currently only is a no-op dummy function. Actually delay in
> this function to avoid spi-gpio being faster than specified.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/spi/gpio_spi.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/gpio_spi.c b/drivers/spi/gpio_spi.c
> index e5664df3fe..a71b4eddab 100644
> --- a/drivers/spi/gpio_spi.c
> +++ b/drivers/spi/gpio_spi.c
> @@ -48,7 +48,10 @@ static inline int getmiso(const struct spi_device *spi)
>  	return !!gpio_get_value(priv->data->miso);
>  }
>  
> -#define spidelay(nsecs) do { } while (0)
> +static inline void spidelay(unsigned int nsecs)
> +{
> +	udelay(max(1U, nsecs / 1000));

Why delay 1us at least? Why not use ndelay?

> +}
>  
>  #include "spi-bitbang-txrx.h"
>  
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/3] spi: spi-gpio: support different word widths

[Ahmad Fatoum]

On 25.09.24 16:06, Sascha Hauer wrote:
> The spi-gpio driver only supports 8bit word width. Add support for
> arbitrary word widths up to 32bit.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

With below issue addressed:

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

>  static int gpio_spi_setup(struct spi_device *spi)
>  {
> -	if (spi->bits_per_word != 8) {
> -		dev_err(spi->master->dev, "master does not support %d bits per word\n",
> -			spi->bits_per_word);
> -		return -EINVAL;
> -	}
> -
> +	gpio_spi_set_cs(spi, 0);

Chipselect is already asserted/deasserted in gpio_spi_transfer, why deassert it here?

>  	return 0;
>  }
>  
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/3] spi: spi-gpio: switch to new gpio binding

[Ahmad Fatoum]

Hello Sascha,

On 25.09.24 16:06, Sascha Hauer wrote:
> The old deprecated device tree binding for the spi-gpio driver uses
> "gpio-sck", "gpio-mosi" and "gpio-miso" to specify the GPIOs. Switch to
> the new binding which uses the standard GPIO property names.
> 
> The old binding is still used in some device trees, but none of the SoCs
> using them is actually supported in barebox, so do not bother to keep a
> fallback to the old binding. The one in-tree user of the old binding is
> converted in this patch.

There is one more user in sandbox-libftdi-example.dtsi. Please fix that as well.

Also, while you are it, why not switch over to the GPIOD API? :-)

Cheers,
Ahmad

> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  arch/arm/dts/stm32mp151-mect1s.dts | 6 +++---
>  drivers/spi/gpio_spi.c             | 6 +++---
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/dts/stm32mp151-mect1s.dts b/arch/arm/dts/stm32mp151-mect1s.dts
> index 4d0e31d189..b0dc1cfaa8 100644
> --- a/arch/arm/dts/stm32mp151-mect1s.dts
> +++ b/arch/arm/dts/stm32mp151-mect1s.dts
> @@ -65,9 +65,9 @@ led-1 {
>  
>  	spi_gpio: spi-gpio-0 {
>  		compatible = "spi-gpio";
> -		gpio-sck = <&gpioi 1 GPIO_ACTIVE_HIGH>;
> -		gpio-mosi = <&gpioi 3 GPIO_ACTIVE_HIGH>;
> -		gpio-miso = <&gpioi 2 GPIO_ACTIVE_HIGH>;
> +		sck-gpios = <&gpioi 1 GPIO_ACTIVE_HIGH>;
> +		mosi-gpios = <&gpioi 3 GPIO_ACTIVE_HIGH>;
> +		miso-gpios = <&gpioi 2 GPIO_ACTIVE_HIGH>;
>  		cs-gpios = <&gpioj 3 GPIO_ACTIVE_LOW>;
>  		num-chipselects = <1>;
>  		#address-cells = <1>;
> diff --git a/drivers/spi/gpio_spi.c b/drivers/spi/gpio_spi.c
> index c76b71f610..34d74faf4c 100644
> --- a/drivers/spi/gpio_spi.c
> +++ b/drivers/spi/gpio_spi.c
> @@ -178,7 +178,7 @@ static int gpio_spi_of_probe(struct device *dev)
>  	if (!IS_ENABLED(CONFIG_OFDEVICE) || dev->platform_data)
>  		return 0;
>  
> -	sck = of_get_named_gpio(np, "gpio-sck", 0);
> +	sck = of_get_named_gpio(np, "sck-gpios", 0);
>  	if (!gpio_is_valid(sck))
>  		return dev_err_probe(dev, sck < 0 ? sck : -EINVAL,
>  				     "missing mandatory SCK gpio\n");
> @@ -187,11 +187,11 @@ static int gpio_spi_of_probe(struct device *dev)
>  	pdata->sck = sck;
>  	pdata->num_cs = MAX_CHIPSELECT;
>  
> -	pdata->miso = of_get_named_gpio(np, "gpio-miso", 0);
> +	pdata->miso = of_get_named_gpio(np, "miso-gpios", 0);
>  	if (!gpio_is_valid(pdata->miso))
>  		pdata->miso = SPI_GPIO_NO_MISO;
>  
> -	pdata->mosi = of_get_named_gpio(np, "gpio-mosi", 0);
> +	pdata->mosi = of_get_named_gpio(np, "mosi-gpios", 0);
>  	if (!gpio_is_valid(pdata->mosi))
>  		pdata->mosi = SPI_GPIO_NO_MOSI;
>  
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/3] spi: spi-gpio: actually delay in spidelay()

[Sascha Hauer]

On Thu, Sep 26, 2024 at 07:55:35AM +0200, Ahmad Fatoum wrote:
> On 25.09.24 16:06, Sascha Hauer wrote:
> > spidelay() currently only is a no-op dummy function. Actually delay in
> > this function to avoid spi-gpio being faster than specified.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/spi/gpio_spi.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/spi/gpio_spi.c b/drivers/spi/gpio_spi.c
> > index e5664df3fe..a71b4eddab 100644
> > --- a/drivers/spi/gpio_spi.c
> > +++ b/drivers/spi/gpio_spi.c
> > @@ -48,7 +48,10 @@ static inline int getmiso(const struct spi_device *spi)
> >  	return !!gpio_get_value(priv->data->miso);
> >  }
> >  
> > -#define spidelay(nsecs) do { } while (0)
> > +static inline void spidelay(unsigned int nsecs)
> > +{
> > +	udelay(max(1U, nsecs / 1000));
> 
> Why delay 1us at least? Why not use ndelay?

I was under the assumption that we don't have ndelay(). We do have it,
will use it instead.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/3] spi: spi-gpio: support different word widths

[Sascha Hauer]

On Thu, Sep 26, 2024 at 08:07:49AM +0200, Ahmad Fatoum wrote:
> On 25.09.24 16:06, Sascha Hauer wrote:
> > The spi-gpio driver only supports 8bit word width. Add support for
> > arbitrary word widths up to 32bit.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> 
> With below issue addressed:
> 
> Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> 
> >  static int gpio_spi_setup(struct spi_device *spi)
> >  {
> > -	if (spi->bits_per_word != 8) {
> > -		dev_err(spi->master->dev, "master does not support %d bits per word\n",
> > -			spi->bits_per_word);
> > -		return -EINVAL;
> > -	}
> > -
> > +	gpio_spi_set_cs(spi, 0);
> 
> Chipselect is already asserted/deasserted in gpio_spi_transfer, why deassert it here?

I had a case where the CS is asserted by default during startup. I had
to deassert it once before the very first transfer to signal a new
transfer to the slave.

Anyway, this change deserves an extra patch.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/3] spi: spi-gpio: switch to new gpio binding

[Sascha Hauer]

On Thu, Sep 26, 2024 at 08:09:44AM +0200, Ahmad Fatoum wrote:
> Hello Sascha,
> 
> On 25.09.24 16:06, Sascha Hauer wrote:
> > The old deprecated device tree binding for the spi-gpio driver uses
> > "gpio-sck", "gpio-mosi" and "gpio-miso" to specify the GPIOs. Switch to
> > the new binding which uses the standard GPIO property names.
> > 
> > The old binding is still used in some device trees, but none of the SoCs
> > using them is actually supported in barebox, so do not bother to keep a
> > fallback to the old binding. The one in-tree user of the old binding is
> > converted in this patch.
> 
> There is one more user in sandbox-libftdi-example.dtsi. Please fix that as well.

Missed that, will do.

> 
> Also, while you are it, why not switch over to the GPIOD API? :-)

I'll do that for the next round.

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |