linux-kernel - Re: [BUG] SPI broken for SPI based panel drivers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201209090459.60ecb5ea@aktux>
Date:   Wed, 9 Dec 2020 09:04:59 +0100
From:   Andreas Kemnade <andreas@...nade.info>
To:     "H. Nikolaus Schaller" <hns@...delico.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Sven Van Asbroeck <thesven73@...il.com>,
        Lukas Wunner <lukas@...ner.de>,
        Mark Brown <broonie@...nel.org>,
        kernel list <linux-kernel@...r.kernel.org>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Discussions about the Letux Kernel 
        <letux-kernel@...nphoenux.org>
Subject: Re: [BUG] SPI broken for SPI based panel drivers

Hi,

On Sat, 5 Dec 2020 08:04:25 +0100
"H. Nikolaus Schaller" <hns@...delico.com> wrote:

> Hi Linus,
> 
> > Am 05.12.2020 um 01:25 schrieb Linus Walleij <linus.walleij@...aro.org>:
> > 
> > On Fri, Dec 4, 2020 at 5:52 PM H. Nikolaus Schaller <hns@...delico.com> wrote:
> >   
> >> 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 :) 
 
Hmm, if those developers are not available, then probably finding
those bugs has to be time-optimized, like establishing better automatic
display testing.

> 
> I find it interesting that so far nobody wants to take responsibility
> for a decision and to write down the behaviour really should be. Coding
> is the second step then.
> 
well, the interesting people are not involved yet (DTML) because no
patch is sent.

> Anyways you did not cite the really important part of my mail. So let me
> copy it back. Here it is again:
> 
> > 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.  
> 
> Please take the attached diff, comment it here and define the question marks
> according to your intention and then make a patch for the YAML bindings out
> of it. (I can't do because I don't know your intentions and what to write into
> the commit message).
> 
Well, I the easiest step forward is just to document clearer how things
behave now, so the commit message is just something like

"Behavior of CS signal is not clearly documented, clarify the
documentation". And then send the patch to the proper mailing lists
including devicetree folks.

Regards,
Andreas

> As soon as we have settled this, we can check if code is correct and really
> define if my device tree fits and which change it needs exactly.
> 
> BR and thanks,
> Nikolaus
> 
> [slightly edited]
> 
> 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 of a gpio descriptor can be GPIO_ACTIVE_HIGH/0
> +      or GPIO_ACTIVE_LOW/1.
> +
> +      There is a 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 potential 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:
> 
> 
>