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

H. Nikolaus Schaller hns at goldelico.com
Sun Mar 24 07:56:17 CET 2019


Hi Linus,

> Am 24.03.2019 um 05:15 schrieb Linus Walleij <linus.walleij at linaro.org>:
> 
> On Sat, Mar 23, 2019 at 3:40 PM H. Nikolaus Schaller <hns at goldelico.com> 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 at 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



More information about the Letux-kernel mailing list