Re: [BUG] SPI broken for SPI based panel drivers

[Linus Walleij <linus.walleij@linaro.org>]

On Fri, Dec 4, 2020 at 5:52 PM H. Nikolaus Schaller <hns@goldelico.com> wrote:

> It could have been described by ACTIVE_LOW without spi-cs-high but that did
> emit a nasty and not helpful warning on each boot.
>
> Well, there are of course better and worse definitions and I agree that
> spi-cs-high to define an active-low chip select sounds strange. Still it
> is just a definition.

This has an historical background which is why we have the
problem in the first place. We ended up with two different
ways of doing polarity inversion in some subsystems because
polarity flags *and* local polarity flags such as this existed.
So the semantics became ambiguous.

commit f618ebfcbf9616a0fa9a78f5ecb69762f0fa3c59
"of/spi: Support specifying chip select as active high via device tree"
created spi-cs-high
October 2008

commit b908b53d580c3e9aba81ebe3339c5b7b4fa8031d
"of/gpio: Implement of_get_gpio_flags()"
Created of_get_gpio_flags() and OF_GPIO_ACTIVE_LOW
as an optional but strongly encouraged cell.
December 2008

What we are seeing
is the consequence of a formal language with ambiguous
semantics. It's no-ones fault other than the human error of
allowing both to exist simultaneously in the first place.

Both ways of doing things existed before I took over as GPIO
maintainer and before Mark took over as SPI maintainer.
What we try to do is contain the situation. My attempt was
to hide it inside the GPIO descriptor, which we still do,
if and only if GPIO descriptors are used.

> But what I don't know is if I can omit spi-cs-high and have to keep
> ACTIVE_HIGH (my revert patch) or also change to ACTIVE_LOW (my additional
> patch). This is arbitrary and someone has to decide what it should be.
(...)
> I'd prefer if you or maybe Linus could submit such a patch and I am happy to review it.

It seems really ill-advised to have me do that since I have not
managed very well to deal with this. Clearly better developers
are needed. But I can review a patch and see if it makes me
smarter :)

> > The reason for that is described in the commit message of
> > 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors")
>
> IMHO it could have been fixed in the gpiolib alone. In my assumption, gpiolib
> would know (or be informed) that the gpio is used by spi and could invert gpio_set_value()
> if needed. Then any SPI code would simply use gpio_set_value(true) if spi-cs-high
> is defined and gpio_set_value(false) if not to enable the chip.

That was the intention behind my code in
of_gpio_flags_quirks().

And I suppose how it finally works after the recent patches
as well, since we now pass enable1 (the non-inverted
assertion parameter) to gpiod_set_value_cansleep()
if and only if you are using GPIO descriptors.

The reason it didn't work and a lot of ill-advised patches
(mostly mine) has a lot to do with native chip selects
which both listened to the same flag "spi-cs-high"
and expected specific semantics from the spi core.

For native chip selects there is no ambiguity: they can
only be made active high using "spi-cs-high".

> The alternative would be that it is only done centrally in one place in the
> spi subsystem.

FWIW I think GPIO CS is fine to handle in the gpiolib
but then spi-cs-high also applies to native chip selects
and that makes it necessary for the SPI core to also parse
for spi-cs-high sometimes, for something that is not
a GPIO, setting SPI_CS_HIGH and calling
spi->controller->set_cs() on the controller with 0 for
active low and 1 for active high.

I think we are there now?

Yours,
Linus Walleij