Hi Linus, I am sorry to report that your patch (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. The symptom was easy to see but really difficult to understand: the LCD panel stopped working (did only show white pixels) after upgrading from v5.0 to v5.1-rc1. In a long bisect session, I was able to identify patch (1) as the first bad commit and started to analyse what the code is supposed to do and then found the older patch (2). 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 (3) c2e138bc8ed80 ARM: dts: omap3-gta04: Add display support So it has no spi-cs-high property and just defines constant 0 for the flags (GPIO_ACTIVE_HIGH after some fix from 2015) assuming that this is sufficient. And it was until before v5.1-rc1. This is now wrongly turned into an GPIO_ACTIVE_LOW by your legacy fix which makes the panel stop feeling responsible to receive any initialization data over SPI. Yes, there is a new message in the boot log for this case, but to be honest, nobody did see it before I found the code that does print it. A non-working LCD panel was a too distracting indicator and could have been everything (most likely drm or the panel driver or clock or regulator or dma setup or whatever)... So your assumption that this case is the exception as stated in commit message of (2) seems not to be true. IMHO it should be standard (at least since 2015) that there is GPIO_ACTIVE_HIGH and no spi-cs-high. So your patch (2) gives a legacy property a new priority over modern style. BTW: the spi-cs-high is described as optional and indicative in the bindings document. But it now appears to be required to get a cs-high. By the way, the detection of spi-cs-high still seems to be wrong as of v5.1-rc1. Shouldn't it look at the child node for "spi-cs-high" and not the controller node pointer? As far as I understand the current code, all such legacy flags are therefore ignored and should break all boards that use spi-cs-high and GPIO_ACTIVE_HIGH. Of course, adding spi-cs-high; (at the controller node!) of our device tree can make it work, but I do not like to be forced to introduce a legacy property to our DTS file, just because gpio-lib tries to fix a legacy problem in the SPI subsystem (quite mind-twisting cross-subsystem dependencies, aren't they?). The real problem seems to be that the legacy spi-cs-high property itself still exists although it should not be needed for ca. 5 years. From all this I conclude that nobody really needs this legacy support or does test it. And if he/she does, they should better fix the cs-gpios entries in the DTS and remove spi-cs-high completely. 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. Then we can leave our GTA04 display and device tree untouched as from v3.16-rc1 up to v5.0.x. BR and thanks, Nikolaus
