Date:   Thu, 3 Nov 2022 20:28:09 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Quentin Schulz <quentin.schulz@...obroma-systems.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 Quentin,

On 11/3/22 19:41, Quentin Schulz wrote:
> 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.

It would seem that way. Anyways lets agree to disagree here.

It seems that Dmitry is in favor of the change you suggest, so
lets just focus on making sure these changes don't break AcPI
support.

>> 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?

I would prefer for the gpiod_direction_output(ts->gpiod_rst, x)
calls to have x actually matching the timing diagrams in
the datasheet.

At a minimum when you invert those from the datasheet, please
add a comment that the values are inverted from the timing
diagram because the GPIO is marked as active-low in their
gpio_desc ?

>> 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.

Yes breaking the existing DT bindings / existing DTB files is
probably also going to be a problem. I'm going to defer reviewing
that part of these changes to other people.

> 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.

Ok, so I've been taking a look at how we can invert the 'x' passed
to the gpiod_direction_output(ts->gpiod_rst, x) calls and not break
things with ACPI.

The rst pin is looked up through a acpi_gpio_mapping which
contains acpi_gpio_params as one of the per pin parameters
and that does have an active_low flag.

After (re)reading the gpiolib code to fresh up my memory
of how this all fits together that flag should do what it
says on the tin.

So if we want to revert the value of x for all the:

gpiod_direction_output(ts->gpiod_rst, x);

calls, then something like the following should work to get
gpiolib to invert that again to turn it into a no-op:

diff --git a/drivers/input/touchscreen/goodix.c b/drivers/input/touchscreen/goodix.c
index a33cc7950cf5..5c294c56a20d 100644
--- a/drivers/input/touchscreen/goodix.c
+++ b/drivers/input/touchscreen/goodix.c
@@ -797,23 +797,26 @@ static int goodix_reset(struct goodix_ts_data *ts)
 }
 
 #ifdef ACPI_GPIO_SUPPORT
-static const struct acpi_gpio_params first_gpio = { 0, 0, false };
-static const struct acpi_gpio_params second_gpio = { 1, 0, false };
+static const struct acpi_gpio_params int_first_gpio = { 0, 0, false };
+static const struct acpi_gpio_params int_second_gpio = { 1, 0, false };
+
+static const struct acpi_gpio_params rst_first_gpio = { 0, 0, true };
+static const struct acpi_gpio_params rst_second_gpio = { 1, 0, true };
 
 static const struct acpi_gpio_mapping acpi_goodix_int_first_gpios[] = {
-	{ GOODIX_GPIO_INT_NAME "-gpios", &first_gpio, 1 },
-	{ GOODIX_GPIO_RST_NAME "-gpios", &second_gpio, 1 },
+	{ GOODIX_GPIO_INT_NAME "-gpios", &int_first_gpio, 1 },
+	{ GOODIX_GPIO_RST_NAME "-gpios", &rst_second_gpio, 1 },
 	{ },
 };
 
 static const struct acpi_gpio_mapping acpi_goodix_int_last_gpios[] = {
-	{ GOODIX_GPIO_RST_NAME "-gpios", &first_gpio, 1 },
-	{ GOODIX_GPIO_INT_NAME "-gpios", &second_gpio, 1 },
+	{ GOODIX_GPIO_RST_NAME "-gpios", &rst_first_gpio, 1 },
+	{ GOODIX_GPIO_INT_NAME "-gpios", &int_second_gpio, 1 },
 	{ },
 };
 
 static const struct acpi_gpio_mapping acpi_goodix_reset_only_gpios[] = {
-	{ GOODIX_GPIO_RST_NAME "-gpios", &first_gpio, 1 },
+	{ GOODIX_GPIO_RST_NAME "-gpios", &rst_first_gpio, 1 },
 	{ },
 };
 
Note this is missing the actual inverting of the
gpiod_direction_output(ts->gpiod_rst, x) calls!

Regards,

Hans

Powered by blists - more mailing lists