[PATCH] spi: stm32_spi: Fix GPIO chipselect polarity handling

[PATCH] spi: stm32_spi: Fix GPIO chipselect polarity handling

[Marek Vasut]

The GPIO chipselect signal polarity is handled by the GPIO core code,
the driver code is only responsible for asserting and deasserting the
GPIO. Do not invert the GPIO polarity in the driver code.

For example, In case CS GPIO is active low, then the DT must contain
GPIO_ACTIVE_LOW flag and the GPIO core code would set the GPIO accordingly.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>
---
 drivers/spi/stm32_spi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/stm32_spi.c b/drivers/spi/stm32_spi.c
index fe5419e8518..0cb24546ca9 100644
--- a/drivers/spi/stm32_spi.c
+++ b/drivers/spi/stm32_spi.c
@@ -434,7 +434,7 @@ static int stm32_spi_xfer(struct udevice *slave, unsigned int bitlen,
 
 	slave_plat = dev_get_parent_plat(slave);
 	if (flags & SPI_XFER_BEGIN)
-		stm32_spi_set_cs(bus, slave_plat->cs, false);
+		stm32_spi_set_cs(bus, slave_plat->cs, true);
 
 	/* Be sure to have data in fifo before starting data transfer */
 	if (priv->tx_buf)
@@ -485,7 +485,7 @@ static int stm32_spi_xfer(struct udevice *slave, unsigned int bitlen,
 	stm32_spi_stopxfer(bus);
 
 	if (flags & SPI_XFER_END)
-		stm32_spi_set_cs(bus, slave_plat->cs, true);
+		stm32_spi_set_cs(bus, slave_plat->cs, false);
 
 	return xfer_status;
 }
-- 
2.35.1

Re: [PATCH] spi: stm32_spi: Fix GPIO chipselect polarity handling

[Patrice CHOTARD]

Hi Marek

On 8/23/22 19:07, Marek Vasut wrote:
> The GPIO chipselect signal polarity is handled by the GPIO core code,
> the driver code is only responsible for asserting and deasserting the
> GPIO. Do not invert the GPIO polarity in the driver code.
> 
> For example, In case CS GPIO is active low, then the DT must contain
> GPIO_ACTIVE_LOW flag and the GPIO core code would set the GPIO accordingly.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
> ---
>  drivers/spi/stm32_spi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/stm32_spi.c b/drivers/spi/stm32_spi.c
> index fe5419e8518..0cb24546ca9 100644
> --- a/drivers/spi/stm32_spi.c
> +++ b/drivers/spi/stm32_spi.c
> @@ -434,7 +434,7 @@ static int stm32_spi_xfer(struct udevice *slave, unsigned int bitlen,
>  
>  	slave_plat = dev_get_parent_plat(slave);
>  	if (flags & SPI_XFER_BEGIN)
> -		stm32_spi_set_cs(bus, slave_plat->cs, false);
> +		stm32_spi_set_cs(bus, slave_plat->cs, true);
>  
>  	/* Be sure to have data in fifo before starting data transfer */
>  	if (priv->tx_buf)
> @@ -485,7 +485,7 @@ static int stm32_spi_xfer(struct udevice *slave, unsigned int bitlen,
>  	stm32_spi_stopxfer(bus);
>  
>  	if (flags & SPI_XFER_END)
> -		stm32_spi_set_cs(bus, slave_plat->cs, true);
> +		stm32_spi_set_cs(bus, slave_plat->cs, false);
>  
>  	return xfer_status;
>  }

It's a bit confusing as there is also the spi property "spi-cs-high" which also invert the polarity
in stm32_spi_set_cs() :

	if (priv->cs_high)
		enable = !enable;

	return dm_gpio_set_value(&plat->cs_gpios[cs], enable ? 1 : 0);

It's seems that chip select polarity can be managed at two different places.

Patrice

Re: [PATCH] spi: stm32_spi: Fix GPIO chipselect polarity handling

[Marek Vasut]

On 8/24/22 16:04, Patrice CHOTARD wrote:
> Hi Marek

Hi,

> On 8/23/22 19:07, Marek Vasut wrote:
>> The GPIO chipselect signal polarity is handled by the GPIO core code,
>> the driver code is only responsible for asserting and deasserting the
>> GPIO. Do not invert the GPIO polarity in the driver code.
>>
>> For example, In case CS GPIO is active low, then the DT must contain
>> GPIO_ACTIVE_LOW flag and the GPIO core code would set the GPIO accordingly.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
>> Cc: Patrice Chotard <patrice.chotard@foss.st.com>
>> ---
>>   drivers/spi/stm32_spi.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/spi/stm32_spi.c b/drivers/spi/stm32_spi.c
>> index fe5419e8518..0cb24546ca9 100644
>> --- a/drivers/spi/stm32_spi.c
>> +++ b/drivers/spi/stm32_spi.c
>> @@ -434,7 +434,7 @@ static int stm32_spi_xfer(struct udevice *slave, unsigned int bitlen,
>>   
>>   	slave_plat = dev_get_parent_plat(slave);
>>   	if (flags & SPI_XFER_BEGIN)
>> -		stm32_spi_set_cs(bus, slave_plat->cs, false);
>> +		stm32_spi_set_cs(bus, slave_plat->cs, true);
>>   
>>   	/* Be sure to have data in fifo before starting data transfer */
>>   	if (priv->tx_buf)
>> @@ -485,7 +485,7 @@ static int stm32_spi_xfer(struct udevice *slave, unsigned int bitlen,
>>   	stm32_spi_stopxfer(bus);
>>   
>>   	if (flags & SPI_XFER_END)
>> -		stm32_spi_set_cs(bus, slave_plat->cs, true);
>> +		stm32_spi_set_cs(bus, slave_plat->cs, false);
>>   
>>   	return xfer_status;
>>   }
> 
> It's a bit confusing as there is also the spi property "spi-cs-high" which also invert the polarity
> in stm32_spi_set_cs() :
> 
> 	if (priv->cs_high)
> 		enable = !enable;
> 
> 	return dm_gpio_set_value(&plat->cs_gpios[cs], enable ? 1 : 0);
> 
> It's seems that chip select polarity can be managed at two different places.

Urgh, all right, drop this patch for now. This clearly needs closer look.

Also, as a bonus, 'sspi' command allows us to override the bus flags, so 
that's place number 3 where things can go confusing.