Messages in this thread | | | Subject | Re: [BUG] gpiolib: spi chip select legacy support breaks modern chip select and whitens the GTA04 LCD panel | From | "H. Nikolaus Schaller" <> | Date | Sun, 24 Mar 2019 07:56:17 +0100 |
| |
Hi Linus,
> Am 24.03.2019 um 05:15 schrieb Linus Walleij <linus.walleij@linaro.org>: > > On Sat, Mar 23, 2019 at 3:40 PM H. Nikolaus Schaller <hns@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@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
| |