linux-kernel - Re: [PATCH 1/4] ARM: dts: omap3-n900: fix LCD reset line polarity

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <41373c20-3b97-ac47-81c8-75bf1bbe3a38@ideasonboard.com>
Date:   Wed, 12 Oct 2022 13:58:15 +0300
From:   Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To:     Tony Lindgren <tony@...mide.com>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     Sebastian Reichel <sre@...nel.org>,
        Benoît Cousson <bcousson@...libre.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org,
        linux-omap@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] ARM: dts: omap3-n900: fix LCD reset line polarity

Hi,

On 11/10/2022 17:47, Tony Lindgren wrote:
> * Dmitry Torokhov <dmitry.torokhov@...il.com> [221011 14:30]:
>> On Tue, Oct 11, 2022 at 05:30:12PM +0300, Tony Lindgren wrote:
>>> * Dmitry Torokhov <dmitry.torokhov@...il.com> [221011 13:57]:
>>>> Hi Sebastian,
>>>>
>>>> On Tue, Oct 11, 2022 at 02:37:26PM +0200, Sebastian Reichel wrote:
>>>>> Hi,
>>>>>
>>>>> On Tue, Oct 11, 2022 at 08:45:54AM +0300, Tony Lindgren wrote:
>>>>>> * Dmitry Torokhov <dmitry.torokhov@...il.com> [221004 21:26]:
>>>>>>> The LCD driver (panel-sony-acx565akm), when probing, starts with line
>>>>>>> driven low, and then toggles it to high and keeps it there. Also, the
>>>>>>> line is driven low when powering off the device, and ls released when
>>>>>>> powering it back on. This means that the reset line should be described
>>>>>>> as "active low" in DTS. This will be important when the driver is
>>>>>>> converted to gpiod API which respects the polarity declared in DTS.
>>>>>>
>>>>>> We should ensure these patches get merged together with the driver
>>>>>> change to avoid breaking LCD for booting. Probably no need to have
>>>>>> the driver quirk handling for inverted polartity in this case.
>>>>>>
>>>>>> It's probably easiest to have an immutable branch for the driver
>>>>>> changes I can base the dts changes on. Or I can ack the dts changes
>>>>>> if they get merged with the driver.
>>>>>
>>>>> Both drivers are already using gpiod API:
>>>>>
>>>>> drivers/gpu/drm/panel/panel-sony-acx565akm.c
>>>>> drivers/gpu/drm/panel/panel-dsi-cm.c
>>>>
>>>> I was looking at
>>>>
>>>> drivers/video/fbdev/omap2/omapfb/displays/panel-sony-acx565akm.c
>>>> drivers/video/fbdev/omap2/omapfb/displays/panel-dsi-cm.c
>>>
>>> Ah OK that explains :)
>>>
>>>> which are not using gpiod. Should they be retired?
>>>
>>> Yes we should just get rid of them with omapdrm working just fine.
>>
>> Will you be submitting such patches? I'd like to get rid of
>> of_get_named_gpio() and friends if I can...
> 
> Adding Tomi to Cc, my guess is he already has such patches and knows
> better which ones can go :)

To be honest, I haven't really even had a glance towards fbdev for a 
long time.

There is one thing that omapdrm doesn't support, which is VRFB rotation. 
I cannot say if the users of those above-mentioned panels require VRFB.

>>>>> So this just breaks things.
>>>>
>>>> I missed the drivers in drivers/gpu/... and I see that they essentially
>>>> abuse gpiod API as gpiod_set_value() operates on logical level
>>>> (active/inactive) and not absolute (high/low). They should either use
>>>> the gpiod_*_raw() variants, or they should be adjusted to do the proper
>>>> thing together with the accompanying DTS change.
>>>>
>>>> What are your preferences?
>>>
>>> Seems like high/low at the connected device end is what we should use,
>>> right? Otherwise things will misbehave if the panel is connected to
>>> some other SoC possibly.
>>
>> It is exactly because of this case the driver should use active/inactive
>> and follow polarity described in DTS. If the driver does:
>>
>> 	gpiod_set_value_cansleep(d->reset, 1);
>>
>> then if DTS is saying that the reset line is active low, under the wraps
>> the line will be driven to "0", but if DTS is saying that the line is
>> active high, then the very same call will drive the line to "1".
>>
>> This allows accommodating different designs without having to change the
>> driver code.

Isn't breaking an old dts file quite a bad thing? Why not just add a 
comment to the .dts and to the driver about the situation. I don't quite 
see that the fixing the dts (And, if done properly, adding a boot time 
fixup for old dtbs) and changing the drivers is worth the hassle.

Unless we see new users for these drivers, which would require the new 
users to write broken dts files.

  Tomi