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

[PATCH v2 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>
---
v2: refactor to avoid ternary (Mark, Sven), dropped comment changes (Mark)
 drivers/spi/spi.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index cd40421b8f55..36ee33514b40 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -820,15 +820,29 @@ 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)) {
-			if (spi->cs_gpiod)
-				/* polarity handled by gpiolib */
-				gpiod_set_value_cansleep(spi->cs_gpiod, activate);
-			else
+			if (spi->cs_gpiod) {
+				/*
+				 * 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.
+				 */
+				if (has_acpi_companion(&spi->dev))
+					gpiod_set_value_cansleep(spi->cs_gpiod, !enable);
+				else
+					/* Polarity handled by GPIO library */
+					gpiod_set_value_cansleep(spi->cs_gpiod, activate);
+			} else {
 				/*
 				 * invert the enable line, as active low is
 				 * default for SPI.
 				 */
 				gpio_set_value_cansleep(spi->cs_gpio, !enable);
+			}
 		}
 		/* Some SPI masters need both GPIO CS & slave_select */
 		if ((spi->controller->flags & SPI_MASTER_GPIO_SS) &&
-- 
2.30.2

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

[Sven Van Asbroeck]

On Tue, May 11, 2021 at 10:08 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> 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.

I don't have access to the hardware to test this right now, but the
patch is small enough to determine that the OF case will stay the
same.
LGTM.

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

[Xin Hao]

在 2021/5/11 下午10:09, 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>
> ---
> v2: refactor to avoid ternary (Mark, Sven), dropped comment changes (Mark)
>   drivers/spi/spi.c | 22 ++++++++++++++++++----
>   1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index cd40421b8f55..36ee33514b40 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -820,15 +820,29 @@ 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)) {
> -			if (spi->cs_gpiod)
> -				/* polarity handled by gpiolib */
> -				gpiod_set_value_cansleep(spi->cs_gpiod, activate);
> -			else
> +			if (spi->cs_gpiod) {
> +				/*
> +				 * 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.
> +				 */
> +				if (has_acpi_companion(&spi->dev))
> +					gpiod_set_value_cansleep(spi->cs_gpiod, !enable);

it worked and code changed minimally,  before ACPI & OF keeps no same 
rules,  this patch is ok!

> +				else
> +					/* Polarity handled by GPIO library */
> +					gpiod_set_value_cansleep(spi->cs_gpiod, activate);
> +			} else {
>   				/*
>   				 * invert the enable line, as active low is
>   				 * default for SPI.
>   				 */
>   				gpio_set_value_cansleep(spi->cs_gpio, !enable);
> +			}
>   		}
>   		/* Some SPI masters need both GPIO CS & slave_select */
>   		if ((spi->controller->flags & SPI_MASTER_GPIO_SS) &&

-- 
Best Regards!
Xin Hao

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

[Mark Brown]

On Tue, 11 May 2021 17:09:12 +0300, Andy Shevchenko wrote:
> 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.

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: Assume GPIO CS active high in ACPI case
      commit: 6b69546912a57ff8c31061f98e56383cc0beffd3

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark