[PATCH v1 1/1] spi: Assume GPIO CS active high in ACPI case

[PATCH v1 1/1] spi: Assume GPIO CS active high in ACPI case

[Andy Shevchenko]

Currently GPIO CS handling, when descriptors are in use, doesn't
take into consideration that in ACPI case the default polarity
is active high and can't be altered. Instead we have to use the
per-chip definition provided by SPISerialBus() resource.

Fixes: 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors")
Cc: Liguang Zhang <zhangliguang@linux.alibaba.com>
Cc: Jay Fang <f.fangjian@huawei.com>
Cc: Sven Van Asbroeck <thesven73@gmail.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Xin Hao <xhao@linux.alibaba.com>
---
 drivers/spi/spi.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 956dce3aafca..f56e0212ee97 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -820,12 +820,23 @@ static void spi_set_cs(struct spi_device *spi, bool enable, bool force)
 
 	if (spi->cs_gpiod || gpio_is_valid(spi->cs_gpio)) {
 		if (!(spi->mode & SPI_NO_CS)) {
+			/*
+			 * Historically ACPI has no means of the GPIO polarity and thus
+			 * the SPISerialBus() resource defines it on the per-chip basis.
+			 * In order to avoid a chain of negations, the GPIO polarity is
+			 * considered being Active High. Even for the cases when _DSD()
+			 * is involved (in the updated versions of ACPI) the GPIO CS
+			 * polarity must be defined Active High to avoid ambiguity.
+			 * That's why we use enable, that takes SPI_CS_HIGH into account.
+			 */
+			bool value = has_acpi_companion(&spi->dev) ? !enable : activate;
+
 			if (spi->cs_gpiod)
-				/* polarity handled by gpiolib */
-				gpiod_set_value_cansleep(spi->cs_gpiod, activate);
+				/* Polarity handled by GPIO library */
+				gpiod_set_value_cansleep(spi->cs_gpiod, value);
 			else
 				/*
-				 * invert the enable line, as active low is
+				 * Invert the enable line, as active low is
 				 * default for SPI.
 				 */
 				gpio_set_value_cansleep(spi->cs_gpio, !enable);
-- 
2.30.2

Re: [PATCH v1 1/1] spi: Assume GPIO CS active high in ACPI case

[Sven Van Asbroeck]

Hi Andy, see below.

On Mon, May 10, 2021 at 10:10 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
>         if (spi->cs_gpiod || gpio_is_valid(spi->cs_gpio)) {
>                 if (!(spi->mode & SPI_NO_CS)) {
> +                       /*
> +                        * Historically ACPI has no means of the GPIO polarity and thus
> +                        * the SPISerialBus() resource defines it on the per-chip basis.
> +                        * In order to avoid a chain of negations, the GPIO polarity is
> +                        * considered being Active High. Even for the cases when _DSD()
> +                        * is involved (in the updated versions of ACPI) the GPIO CS
> +                        * polarity must be defined Active High to avoid ambiguity.
> +                        * That's why we use enable, that takes SPI_CS_HIGH into account.
> +                        */
> +                       bool value = has_acpi_companion(&spi->dev) ? !enable : activate;

There are three booleans involved now: "value", "enable" and
"activate". It might be quite hard for someone reading this code
later, to work out what's going on? I've got to admit that my previous
choice of "enable1" was also not perfect, to say the least...

AFAIK there are two basic concepts in this function:
- enable: indicates chip-select enabled or disabled (independent of
gpio polarity)
- level : indicates the required level of the chip-select gpio

So maybe we can simplify like this?

static void spi_set_cs(struct spi_device *spi, bool enable, bool force)
{
        bool level = (spi->mode & SPI_CS_HIGH) ? enable : !enable;

        if cs_gpio              => use level  : gpio_set_value_cansleep(level)
        else if cs_gpiod(acpi)  => use level  : gpiod_set_value_cansleep(level)
        else if cs_gpiod(_)     => use enable :
gpiod_set_value_cansleep(enable);
}

Re: [PATCH v1 1/1] spi: Assume GPIO CS active high in ACPI case

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 682 bytes --]

On Mon, May 10, 2021 at 05:10:22PM +0300, Andy Shevchenko wrote:

> +			bool value = has_acpi_companion(&spi->dev) ? !enable : activate;

Please write normal conditional statements to improve legibility.

>  			if (spi->cs_gpiod)
> -				/* polarity handled by gpiolib */
> -				gpiod_set_value_cansleep(spi->cs_gpiod, activate);
> +				/* Polarity handled by GPIO library */
> +				gpiod_set_value_cansleep(spi->cs_gpiod, value);
>  			else
>  				/*
> -				 * invert the enable line, as active low is
> +				 * Invert the enable line, as active low is
>  				 * default for SPI.

The change would be clearer with the documentation formatting changes
split out from the rest of it.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/1] spi: Assume GPIO CS active high in ACPI case

[Andy Shevchenko]

On Tue, May 11, 2021 at 12:13:30PM +0100, Mark Brown wrote:
> On Mon, May 10, 2021 at 05:10:22PM +0300, Andy Shevchenko wrote:
> 
> > +			bool value = has_acpi_companion(&spi->dev) ? !enable : activate;
> 
> Please write normal conditional statements to improve legibility.

OK!

> >  			if (spi->cs_gpiod)
> > -				/* polarity handled by gpiolib */
> > -				gpiod_set_value_cansleep(spi->cs_gpiod, activate);
> > +				/* Polarity handled by GPIO library */
> > +				gpiod_set_value_cansleep(spi->cs_gpiod, value);
> >  			else
> >  				/*
> > -				 * invert the enable line, as active low is
> > +				 * Invert the enable line, as active low is
> >  				 * default for SPI.
> 
> The change would be clearer with the documentation formatting changes
> split out from the rest of it.

Will remove these changes from a fix in v2.

P.S> something is odd about the series I have submitted, i.e. half of the
patches simply didn't make it. I would wait with any new submission until it
will be clarified, otherwise it's a chance to miss more patches without any
reason why.

-- 
With Best Regards,
Andy Shevchenko