Message-ID: <267de96a-0129-a97d-9bf6-e1001b422a1a@theobroma-systems.com>
Date:   Thu, 3 Nov 2022 19:41:12 +0100
From:   Quentin Schulz <quentin.schulz@...obroma-systems.com>
To:     Hans de Goede <hdegoede@...hat.com>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     Quentin Schulz <foss+kernel@...il.net>, hadess@...ess.net,
        robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        shawnguo@...nel.org, s.hauer@...gutronix.de, kernel@...gutronix.de,
        festevam@...il.com, linux-imx@....com, wens@...e.org,
        jernej.skrabec@...il.com, samuel@...lland.org, agross@...nel.org,
        andersson@...nel.org, konrad.dybcio@...ainline.org,
        heiko@...ech.de, linux-input@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        arm-mail-list <linux-arm-kernel@...ts.infradead.org>,
        linux-sunxi@...ts.linux.dev, linux-arm-msm@...r.kernel.org,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>
Subject: Re: [RFC PATCH 1/7] Input: goodix - fix reset polarity

Hi all,

On 11/3/22 18:45, Hans de Goede wrote:
> Hi,
> 
> On 11/3/22 18:32, Dmitry Torokhov wrote:
>> Hi Hans,
>>
>> On Thu, Nov 03, 2022 at 03:58:47PM +0100, Hans de Goede wrote:
>>> Hi Quentin,
>>>
>>> On 11/3/22 15:43, Quentin Schulz wrote:
>>>> From: Quentin Schulz <quentin.schulz@...obroma-systems.com>
>>>>
>>>> The reset line is asserted for selecting the I2C target address and then
>>>> deasserted.
>>>
>>> It is not asserted/deasserted, asserted/deasserted is reset-controller/
>>> reset-framework (drivers/reset/*) terminology.
>>>
>>> We are driving GPIOs here and those are driven low/high.
>>
>> Not quite. GPIOD API operates on a logival level (think of them as
>> active/inactive) and allows platform/firmware to specify polarity from
>> the AP point of view (as opposed to device). This important if the
>> peripheral is not attached directly, but potentially through an inverter
>> or something similar).
> 
> Right and if a line runs through an inverting buffer then marking
> the pin as active-low in the DT makes a lot of sense here.
> 

It doesn't to me. /me shrugs

> But as I mentioned before the datasheet spells out a very specific
> init-sequence.
> 

As Dmitry pointed out, we're talking about logical vs physical level. 
The driver tries to enforce physical level (on the touchscreen 
controller side) by expecting the logical level (of the gpio controller) 
to match.

> By default marking all the direct-attached RST pin connections as
> active-low (1) to then invert the value again in the driver
> (from the datasheet init sequence specified values pov) IMHO
> just makes the driver code harder to read when putting it side
> to side by the init-sequence specified in the datasheet.
> 

When I want to put a device into reset mode, I activate/assert the line 
so that its logical state is "active". For Goodix, its reset line is 
active low. I do a "positive" action, so I activate something. If it was 
called nreset, that would be a different story. If it was named 
enable-gpios, I would understand. I just don't get the current 
implementation with reset-gpios in DT.

Reading:
reset-gpios = <&gpio0 1 GPIO_ACTIVE_HIGH>;
means that I need to set the logical output to HIGH to have a HW reset, 
which is not what happens for this driver.

> I don't see how playing this double-invert game is going to help
> us / gives us any added value, in any way.
> 

The current implementation is playing the double-invert game for me.

So clearly, we do not agree on what, at least in the DT, the level of a 
reset gpio should mean.

> And in all the ACPI tables the GPIOs are marked as active-high
> so changing this to have the driver now all of a sudden expect
> the reset-gpio to be marked as active-low at the gpio-subsys
> level will be quite cumbersome since normally the active-low vs
> -high info comes from the firmware-tables and now all of a sudden
> we need to override this.
> 

We have the information from which standard we got the GPIO, so we could 
always invert the flag we get from DT to match whatever is in ACPI.

Blindly ignoring the DT flag is not an option since the HW design could 
actually require an inversion (GPIO connected to a transistor for 
example). I'm not sure what exactly could be done on the gpio-subsys 
level to deal with this. I think we just disagree on what the reset 
"active state" should mean and no amount of code would fix that?

> During all my work on the goodix driver I have always been very
> careful to not introduce any behavior changes for the DT users
> of the drivers. It would be nice if this courtesy could also
> be extended in the other direction.
> 

This RFC is clearly breaking ACPI support. I have zero knowledge about 
ACPI and didn't know that devm_gpiod_get_optional fetches from OF or 
ACPI. It was not my intention to break ACPI, sorry if it came this way.

I frankly didn't expect this to be an easy discussion, since changing 
the DT is usually a no-go, but as is making the DT binding 
implementation-specific (which is the current state of affairs), e.g. 
we'll need U-Boot/BSD/whatever driver to also use the same logic. I want 
to be noted that I like none of the options I offered so far.

As I was surprised by the (for me) inverted logic of the GPIO state, I 
preferred fixing the driver and DT to match what my expectations were.

I'm looking for guidance on how we can deal with this, I do not claim 
what I suggest is what we should absolutely go for.

Maybe my expectations were misguided and I should just tell my brain to 
invert whatever my intuition tells me, it wouldn't be the first time.

Cheers,
Quentin

P.S.: I've been notified only the cover letter made it to the mailing 
lists, so adding the mailing lists in Cc right now. Hopefully enough 
context is left in the mail. Apologies.

Powered by blists - more mailing lists