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