Re: [BUG] gpiolib: spi chip select legacy support breaks modern chip select and whitens the GTA04 LCD panel

[H. Nikolaus Schaller]

Hi Linus,

> Am 24.03.2019 um 05:15 schrieb Linus Walleij <linus.walleij@xxxxxxxxxx>:
> 
> On Sat, Mar 23, 2019 at 3:40 PM H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> wrote:
> 
>> (1) c1c04cea13dc gpio: of: Fix logic inversion
>> 
>> together with the basic patch
>> 
>> (2) 6953c57ab172 gpio: of: Handle SPI chipselect legacy bindings
>> 
>> leads to a severe regression for our GTA04 board.
> 
> Sorry! :(
> 
> I found the same problem on my Nomadik board.
> 
> But I fixed it in that case by introducing a spi-cs-high into the DTS file:
> https://marc.info/?l=linux-arm-kernel&m=155292310015309&w=2

Yes, that of course works and is our temporary solution.

And I see that you also have it on the controller node and not the slave node.

> 
>> I learned that it tries to handle a legacy "spi-cs-high" property of SPI slaves, but was stopped
>> from doing so by a bug (1). So only with both patches, the legacy handler becomes active which
>> explains why it was not noticed earlier.
>> 
>> Now, our GTA04 device tree from 2014 (v3.16-rc1) was already written without any legacy spi properties
>> in mind
> 
> I'm sorry about that, however if you look at the DT binding document:
> Documentation/devicetree/bindings/spi/spi-bus.txt

Shouldn't it be a property of the slave node and not the controller node?

> 
> You will see that spi-cs-high is mandatory. So these DTS files are
> incorrect.

How do you read that it is mandatory from

"All slave nodes can contain the following optional properties:
- spi-cs-high     - Empty property indicating device requires chip select
		    active high."

I read it as optional and indicative. Equal priority to cs-gpios.

> 
>> Therefore I would suggest:
>> * revert both patches as soon as possible (v5.1-rc series) to remove the unexpected spi legacy
>>  code handler from the gpio subsystem.
>> * replace all uses of spi-cs-high by correct cs-gpios flags - unless they already are there.
>>  fgrep spi-cs-high arch/*/boot/dts/*.dts* shows only a handful of DTS candidates.
>> * fix spi-bus.txt documentation to describe this potential pitfall.
> 
> This does not work because there are devices that requires spi-cs-high to be
> respected and the DTS second cell GPIO flag to be ignored.

Then, those should be fixed...

> 
> Jan Kotas reported this problem.

Thanks for adding him to the discussion.

> 
> They might have deployed DTB binaries that need to keep working,

Well, that is a weak argument. What if the GTA04 would have the DTB in FLASH
and would need it working (fortunately we always reflash kernel + dtbs)?

> so we
> cannot change it to ignore spi-cs-high like this. (I might give in if it can be
> proven that all of them just recompile the DTS all the time and no
> DTBs are in flash.)

BTW, on which node do these invariable DTBs have the spi-cs-high flag?
In the controller node (IMHO wrong) or the slave node (according to bindings doc)?

I have checked with randomly picked imx51-babbage.dts and it has it in the
slave node (pmic: mc13892@0). It also seems to be an example where different
CS lines want different settings.

If the all these DTBs have spi-cs-high in the slave node, none of them will
be fixed by the current code.

> 
> I think in this case the oldest binding wins.

Ok, it is the question when such deprecated things are really removed.
There is no clear answer...

> The spi-cs-high was there before
> we came up with the scheme to use the flags cell with GPIO phandles.

> 
> I think you simply have to patch these GTA04 DTS files to use
> spi-cs-high.

Ugly... Well, if DTS maintainers accept that?

> 
> But I'm open to other ideas, let's discuss this.

What about a CONFIG to explicitly enable/disable this legacy support?

IMHO it will need to be enabled for les than 1% of the kernel builds and
therefore also keeps the zImage smaller for all others. And avoids DTB
changes where the gpio flags are already correct.

BR and thanks,
Nikolaus