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

[H. Nikolaus Schaller]

Hi Andreas,

> Am 30.03.2019 um 19:33 schrieb Andreas Kemnade <andreas@xxxxxxxxxxxx>:
> 
> On Sun, 24 Mar 2019 07:56:17 +0100
> "H. Nikolaus Schaller" <hns@xxxxxxxxxxxxx> wrote:
> 
>> 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.
>> 
> So if I get it right is a check added for undocumented properties
> (nothing about  spi-cs-high  in controller node in the bindings
> documentation, only in the slave node) in the two patches mentioned.
> 
> And then you add that undocumented property to a dts file in your "fix".
> 
> That all sounds strange to me.

> 
>>> 
>>>> 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.
>> 
> Well, it is in the list of optional properties. So the question is how
> that "optional" is interpreted. Is it optional because you only use it
> if cs is active high or is it optional because you can either indicate
> that fact here or via gpio definition.
> 
> But again that flags makes no sense in the controller node.

I have already prepared some fix for this controller/slave node issue and
a new CONFIG option to enable the legacy "spi-cs-high" only if needed.
Seems to be common practice that legacy API support (and legacy DTB
compatibility is also "API") is only enabled by CONFIGs on demand.

This can then replace our current GTA04 hack. Will post ASAP to LKML.

BR,
Nikolaus