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

[H. Nikolaus Schaller]

Hi Sven,

> Am 04.12.2020 um 14:46 schrieb Sven Van Asbroeck <thesven73@xxxxxxxxx>:
> 
> On Fri, Dec 4, 2020 at 5:11 AM H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> wrote:
>> 
>> Anyways it is debatable if this is a bug at all. It is just a definition.
> 
> I respectfully disagree. Prior to the fix, your panel's active-low chip select
> needed to be described in the devicetree with 'spi-cs-high'. That sounds
> very much like a bug to me.

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.

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.

Then, the gpiolib / spi driver code should be adapted if necessary and a
unit-test or real-hardware test with all possible combinations would prove
it for all other devices as well. So testing against a specification does
not need /my/ hardware.

> 
>> Which is not well documented anywhere.
> 
> I agree that documentation can be improved here.
> Would you like to submit a patch that improves:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/spi/spi-controller.yaml?h=v5.10-rc6#n28
> ?

Yes, that is the right location.

Basically it involves adding a table like in my previous mail to the comment
so that it also covers all cases where the second parameter is not 0.

I'd prefer if you or maybe Linus could submit such a patch and I am happy to review it.

The reason is that I am neither involved in gpiolib nor spi subsystem development
 so I neither know what your intended setup is. I may define a wrong table.

I just need a stable definition by the subsystem maintainers so that I can
finish the device tree I am responsible for.

What I can do is to provide just a skeleton for the table that you or Linus
can fix/fill in and make a patch out of it. Is attached and the ??? is
something you should discuss and define.

> This way, we also get Rob Herring involved, which may lead to more elegant
> documentation. He is more likely to respond to a patch than to a question.
> 
>> 
>> What I especially wonder is why you fix that at all in drivers/spi/spi.c
>> if polarity inversion is handled in gpiolib.
> 
> 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.

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

But I may be wrong, because the real architecture of the spi and gpiolib code
is quite new to me. My focus is on very different things (e.g. camera driver,
drm panel drivers) which is already complex enough.

BR and thanks,
Nikolaus

diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml
index 1b56d5e40f1f..4f8755dabecc 100644
--- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
@@ -42,6 +42,30 @@ properties:
         cs2 : &gpio1 1 0
         cs3 : &gpio1 2 0
 
+      The second flag can be GPIO_ACTIVE_HIGH/0 or GPIO_ACTIVE_LOW/1.
+
+      There is special rule set for combining the second flag of an
+      cs-gpio with the optional spi-cs-high flag for SPI slaves.
+
+      Each table entry defines how the CS pin is physically driven
+      (not considering gpio inversions by pinmux):
+
+      device node     | cs-gpio       | CS pin state active | Note
+      ================+===============+=====================+=====
+      spi-cs-high     | -             | H                   |
+      -               | -             | L                   |
+      spi-cs-high     | ACTIVE_HIGH   | H                   |
+      -               | ACTIVE_HIGH   | L (or H ???)        | 1
+      spi-cs-high     | ACTIVE_LOW    | H (or L ???)        | 2
+      -               | ACTIVE_LOW    | L                   |
+
+      Notes:
+      1) should print a warning about polarity inversion
+         because here it would be wise to define the gpio as ACTIVE_LOW
+      2) could print a warning about polarity inversion
+         because ACTIVE_LOW is overridden by spi-cs-high
+      3) Effectively this rule defines that the ACTIVE level of the
+         gpio has to be ignored
+
   num-cs:
     $ref: /schemas/types.yaml#/definitions/uint32
     description: