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: <9380CE00-9CE6-4E0B-B2E1-1B534F85E47D@goldelico.com>
Date:   Tue, 1 Dec 2020 17:39:11 +0100
From:   "H. Nikolaus Schaller" <hns@...delico.com>
To:     Sven Van Asbroeck <thesven73@...il.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        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

> Am 01.12.2020 um 17:10 schrieb Sven Van Asbroeck <thesven73@...il.com>:
> 
> Nikolaus,
> 
> On Tue, Dec 1, 2020 at 9:38 AM H. Nikolaus Schaller <hns@...delico.com> wrote:
>> 
>> Let's work on a fix for the fix now.
>> 
> 
> Are you quite sure the chip-select of the tpo,td028ttec1 panel
> is active-high? A quick google produced a datasheet which
> seems to indicate that XCS is active-low?
> 
> See page 17 here:
> http://www.lcd-source.com/datasheet/TPO/TD028TTEC1.pdf
> 
> It is of course possible that you are driving that line behind
> some inverting circuitry. Hardware designers seem to do that
> all the time, if they need to go from one voltage domain to
> the other, etc.

You are right. It is active low.

But that is the start of a long story...

It was introduced in DT by c2e138bc8ed80ae
with cs-gpios = <&gpio1 19 0>;

because the third parameter did not have any meaning
back then, AFAIR. It was ignored and drivers did initialize
gpios with inversion if needed.

Missing spi-cs-high; did define active low back then for spi-gpio.

In 3a637e008e542b8ebdc2ceed616b229af0462b14
the "0" was replaced by the constant 
cs-gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>;

keeping the DTB unchanged and compatible.

This did work because there was still no spi-cs-high;

Then came 6953c57ab172 which mixed cs-gpios and spi-cs-high;
and made use of the now wrong GPIO_ACTIVE_HIGH;
A special logic converted GPIO_ACTIVE_HIGH to GPIO_ACTIVE_LOW
in case of spi-cs-high; being present.

A long discussion revealed that the only solution to
stay compatible with old and new DT/kernels was to
add spi-cs-high; and keep the wrong <&gpio1 19 GPIO_ACTIVE_HIGH>;

f1f028ff89cb0d3 did "fix" this by adding spi-cs-high to the DT.

Please note that apparently we were already confused about what
the right value should be (ACTIVE_HIGH or ACTIVE_LOW) in 2019 but
the result was working until your new patch appeared.

Yes, it could be that the problem introduced by 6953c57ab172
is just coming up again with your new change.

On the other hand I remember from the old discussion that changing
a DT is "forbidden" if it is not upwards compatible with earlier
kernels. The driver must take what it gets from (legacy) DT.

BR,
Nikolaus