Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210510141022.56499-1-andriy.shevchenko@linux.intel.com>
In-Reply-To: <20210510141022.56499-1-andriy.shevchenko@linux.intel.com>
From:   Sven Van Asbroeck <thesven73@gmail.com>
Date:   Mon, 10 May 2021 19:36:21 -0400
Message-ID: <CAGngYiVHq=TxVL5gs19EXHSobCmQ7Pw0v9Oe4XceLRboj8u+3A@mail.gmail.com>
Subject: Re: [PATCH v1 1/1] spi: Assume GPIO CS active high in ACPI case
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     Mark Brown <broonie@kernel.org>,
        linux-spi <linux-spi@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        Liguang Zhang <zhangliguang@linux.alibaba.com>,
        Jay Fang <f.fangjian@huawei.com>,
        Xin Hao <xhao@linux.alibaba.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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);
}