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 <[email protected]>
>>>>
>>>> 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.
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 <[email protected]>
>>>>>
>>>>> 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
Hi Hans,
Sorry for the delay.
On 11/3/22 20:28, Hans de Goede wrote:
[...]
> 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.
>
I just read the kernel docs about GPIO and ACPI and I'm not entirely
sure this is always 100% safe to do:
https://docs.kernel.org/firmware-guide/acpi/gpio-properties.html
Specifically:
"""
The GpioIo() resource unfortunately doesn't explicitly provide an
initial state of the output pin which driver should use during its
initialization.
Linux tries to use common sense here and derives the state from the bias
and polarity settings. The table below shows the expectations:
========= ============= ==============
Pull Bias Polarity Requested...
========= ============= ==============
Implicit x AS IS (assumed firmware configured for us)
Explicit x (no _DSD) as Pull Bias (Up == High, Down == Low),
assuming non-active (Polarity = !Pull Bias)
Down Low as low, assuming active
Down High as low, assuming non-active
Up Low as high, assuming non-active
Up High as high, assuming active
========= ============= ==============
"""
But since we actually override this during our devm_gpiod_get_optional
by passing forcing the flag to be either GPIOD_IN or GPIOD_ASIS, we
should be good for this driver IIUC?
Thanks for the pointers,
Cheers,
Quentin
Hi,
On 11/21/22 16:06, Quentin Schulz wrote:
> Hi Hans,
>
> Sorry for the delay.
>
> On 11/3/22 20:28, Hans de Goede wrote:
> [...]
>> 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.
>>
>
> I just read the kernel docs about GPIO and ACPI and I'm not entirely sure this is always 100% safe to do:
>
> https://docs.kernel.org/firmware-guide/acpi/gpio-properties.html
>
> Specifically:
> """
> The GpioIo() resource unfortunately doesn't explicitly provide an initial state of the output pin which driver should use during its initialization.
>
> Linux tries to use common sense here and derives the state from the bias and polarity settings. The table below shows the expectations:
>
> ========= ============= ==============
> Pull Bias Polarity Requested...
> ========= ============= ==============
> Implicit x AS IS (assumed firmware configured for us)
> Explicit x (no _DSD) as Pull Bias (Up == High, Down == Low),
> assuming non-active (Polarity = !Pull Bias)
> Down Low as low, assuming active
> Down High as low, assuming non-active
> Up Low as high, assuming non-active
> Up High as high, assuming active
> ========= ============= ==============
> """
>
> But since we actually override this during our devm_gpiod_get_optional by passing forcing the flag to be either GPIOD_IN or GPIOD_ASIS, we should be good for this driver IIUC?
Not entirely I just checked and for some reason the ACPI GPIO
lookup code will override the gpiod_flags passed to gpiod_get()
if it can determine a set of flags from the ACPI GpioIo entry.
For output pins like the reset pin, this requores a pull bias
to be set, which often is not the case, so then the GPIOD_ASIS
which we pass in is used.
But if a pull bias is specified in the ACPI GpioIo entry for
the reset pin then thats get translated to GPIOD_OUT_LOW or
GPIOD_OUT_HIGH and setting acpi_gpio_params.active_low as
your patch do will flip those.
So this may cause unintended side-effects.
This is something which we can fix though, we can force
the ACPI GPIO code to honor the GPIOD_ASIS we pass in
by changing:
static const struct acpi_gpio_params first_gpio = { 0, 0, false };
static const struct acpi_gpio_params second_gpio = { 1, 0, false };
to:
static const struct acpi_gpio_params first_gpio = { 0, 0, false, ACPI_GPIO_QUIRK_NO_IO_RESTRICTION };
static const struct acpi_gpio_params second_gpio = { 1, 0, false, ACPI_GPIO_QUIRK_NO_IO_RESTRICTION };
Which will make gpiod_get honor the GPIOD_ASIS for the reset pin
and the GPIOD_IN for the IRQ pin.
It would be good to do this as a preparation patch, because
this will be good to have regardless of the rest of your series
because the gpiolib-acpi behavior without the
ACPI_GPIO_QUIRK_NO_IO_RESTRICTION flag may already cause the
reset GPIO to get its value changed at probe time which we
do not want to happen in the Goodix driver.
If you can send me such a preparation patch + a new 1/7
on top, then I can give this a test on a x86/ACPI device
with a goodix touchscreen.
(Maybe send them offlist if you don't want to send out another
version this quickly.
Regards,
Hans