From: Sander Vanheule <sander@svanheule.net>
To: Michael Walle <michael@walle.cc>,
Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Andrew Lunn <andrew@lunn.ch>, Pavel Machek <pavel@ucw.cz>,
Rob Herring <robh+dt@kernel.org>,
Lee Jones <lee.jones@linaro.org>, Mark Brown <broonie@kernel.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
"Rafael J . Wysocki" <rafael@kernel.org>,
Linus Walleij <linus.walleij@linaro.org>,
Bartosz Golaszewski <bgolaszewski@baylibre.com>,
Linux LED Subsystem <linux-leds@vger.kernel.org>,
devicetree <devicetree@vger.kernel.org>,
"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Hans de Goede <hdegoede@redhat.com>
Subject: Re: [PATCH v3 0/6] RTL8231 GPIO expander support
Date: Sun, 30 May 2021 18:19:21 +0200 [thread overview]
Message-ID: <84352c93f27d7c8b7afea54f3932020e9cd97d02.camel@svanheule.net> (raw)
In-Reply-To: <f03d5cdc958110fc7d95cfc4258dac4e@walle.cc>
Hi Michael, Andy,
On Fri, 2021-05-28 at 08:37 +0200, Michael Walle wrote:
> Am 2021-05-24 13:41, schrieb Sander Vanheule:
> > Hi Andy, Andrew,
> >
> > On Mon, 2021-05-24 at 10:53 +0300, Andy Shevchenko wrote:
> > > On Mon, May 24, 2021 at 4:11 AM Andrew Lunn <andrew@lunn.ch> wrote:
> > > >
> > > > > Changes since v2:
> > > > > - MDIO regmap support was merged, so patch is dropped here
> > > >
> > > > Do you have any idea how this will get merged. It sounds like one of
> > > > the Maintainers will need a stable branch of regmap.
> > >
> > > This is not a problem if Mark provides an immutable branch to pull
> > > from.
> >
> > Mark has a tag (regmap-mdio) for this patch:
> > https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git/tag/?h=regmap-mdio
> >
> > >
> > > > > - Introduce GPIO regmap quirks to set output direction first
> > > >
> > > > I thought you had determined it was possible to set output before
> > > > direction?
> > >
> > > Same thoughts when I saw an updated version of that patch. My
> > > anticipation was to not see it at all.
> >
> > The two devices I've been trying to test the behaviour on are:
> > * Netgear GS110TPP: has an RTL8231 with three LEDs, each driven via a
> > pin
> > configured as (active-low) GPIO. The LEDs are easy for a quick
> > visual check.
> > * Zyxel GS1900-8: RTL8231 used for the front panel button, and an
> > active-low
> > GPIO used to hard reset the main SoC (an RTL8380). I've modified
> > this board
> > to change some of the strapping pin values, but testing with the
> > jumpers and
> > pull-up/down resistors is a bit more tedious.
> >
> > On the Netgear, I tested the following with and without the quirk:
> >
> > # Set as OUT-LOW twice, to avoid the quirk. Always turns the LED on
> > gpioset 1 32=0; gpioset 1 32=0
> > # Get value to change to input, turns the LED off (high impedance)
> > # Will return 1 due to (weak) internal pull-up
> > gpioget 1 32
> > # Set as OUT-HIGH, should result in LED off
> > # When the quirk is disabled, the LED turns on (i.e. old OUT-LOW
> > value)
> > # When the quirk is enabled, the LED remains off (i.e. correct
> > OUT-HIGH value)
> > gpioset 1 32=1
> >
> > Now, what's confusing (to me) is that the inverse doesn't depend on the
> > quirk:
> >
> > # Set as OUT-HIGH twice
> > gpioset 1 32=1; gpioset 1 32=1
> > # Change to high-Z
> > gpioget 1 32
> > # Set to OUT-LOW, always results in LED on, with or without quirk
> > gpioset 1 32=0
> >
> > Any idea why this would be (or appear) broken on the former case, but
> > not on the
> > latter?
>
> Before reading this, I'd have guessed that they switch the internal
> register
> depending on the GPIO direction; I mean there is only one register
> address
> for both the input and the output register. Hm.
>
> Did you try playing around with raw register accesses and see if the
> value
> of the GPIO data register is changing when you switch GPIOs to
> input/output.
>
> Eg. you could try https://github.com/kontron/miitool to access the
> registers
> from userspace (your ethernet controller has to have support for the
> ioctl's
> though, see commit a613bafec516 ("enetc: add ioctl() support for
> PHY-related
> ops") for an example).
I think I found a solution!
As Michael suggested, I tried raw register reads and writes, to eliminate any
side effects of the intermediate code. I didn't use the ioctls (this isn't a
netdev), but I found regmap's debugfs write functionality, which allowed me to
do the same.
I was trying to reproduce the behaviour I reported earlier, but couldn't. The
output levels were always the intended ones. At some point I realised that the
regmap_update_bits function does a read-modify-write, which might shadow the
actual current output value.
For example:
* Set output low: current out is low
* Change to input with pull-up: current out is still low, but DATAx reads high
* Set output high: RMW reads a high value (the input), so assumes a write is
not necessary, leaving the old output value (low).
Currently, I see two options:
* Use regmap_update_bits_base to avoid the lazy RMW behaviour
* Add a cache for the output data values to the driver, and only use these
values to write to the output registers. This would allow keeping lazy RMW
behaviour, which may be a benefit on slow busses.
With either of these implemented, if I set the output value before the
direction, everything works! :-)
Would you like this to be added to regmap-gpio, or should I revert back to a
device-specific implementation?
Best,
Sander
next prev parent reply other threads:[~2021-05-30 16:19 UTC|newest]
Thread overview: 114+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-11 12:25 [PATCH 0/5] RTL8231 GPIO expander support Sander Vanheule
2021-05-11 12:25 ` [PATCH 1/5] dt-bindings: leds: Binding for RTL8231 scan matrix Sander Vanheule
2021-05-17 22:31 ` Rob Herring
2021-05-19 16:39 ` Sander Vanheule
2021-05-11 12:25 ` [PATCH 2/5] dt-bindings: mfd: Binding for RTL8231 Sander Vanheule
2021-05-17 22:38 ` Rob Herring
2021-05-19 16:53 ` Sander Vanheule
2021-05-11 12:25 ` [PATCH 3/5] mfd: Add RTL8231 core device Sander Vanheule
2021-05-12 12:29 ` kernel test robot
2021-05-12 13:13 ` kernel test robot
2021-05-19 14:58 ` Lee Jones
2021-05-19 15:11 ` Sander Vanheule
2021-05-11 12:25 ` [PATCH 4/5] pinctrl: Add RTL8231 pin control and GPIO support Sander Vanheule
2021-05-11 12:25 ` [PATCH 5/5] leds: Add support for RTL8231 LED scan matrix Sander Vanheule
[not found] ` <CAHp75VffoKyyPJbdtKMLx575c9LT0S8+EHOk7Mw36j=aTL6Q4Q@mail.gmail.com>
2021-05-16 21:40 ` [PATCH 0/5] RTL8231 GPIO expander support Sander Vanheule
2021-05-17 8:13 ` Andy Shevchenko
2021-05-17 8:50 ` Sander Vanheule
2021-05-17 19:32 ` Sander Vanheule
2021-05-17 19:28 ` [PATCH v2 0/7] " Sander Vanheule
2021-05-17 19:28 ` [PATCH v2 1/7] regmap: Add MDIO bus support Sander Vanheule
2021-05-19 16:12 ` Mark Brown
2021-05-17 19:28 ` [PATCH v2 2/7] gpio: regmap: Add configurable dir/value order Sander Vanheule
2021-05-17 21:06 ` Andy Shevchenko
2021-05-18 1:40 ` Andrew Lunn
2021-05-18 11:39 ` Sander Vanheule
2021-05-23 22:21 ` Sander Vanheule
2021-05-18 8:39 ` Michael Walle
2021-05-18 10:39 ` Andy Shevchenko
2021-05-23 21:19 ` Sander Vanheule
2021-05-17 19:28 ` [PATCH v2 3/7] dt-bindings: leds: Binding for RTL8231 scan matrix Sander Vanheule
2021-05-17 19:28 ` [PATCH v2 4/7] dt-bindings: mfd: Binding for RTL8231 Sander Vanheule
2021-05-18 22:02 ` Linus Walleij
2021-05-17 19:28 ` [PATCH v2 5/7] mfd: Add RTL8231 core device Sander Vanheule
2021-05-17 21:18 ` Andy Shevchenko
2021-05-23 21:28 ` Sander Vanheule
2021-05-24 7:49 ` Andy Shevchenko
2021-05-24 7:50 ` Sander Vanheule
2021-05-24 7:55 ` Andy Shevchenko
2021-05-24 8:04 ` Sander Vanheule
2021-05-17 19:28 ` [PATCH v2 6/7] pinctrl: Add RTL8231 pin control and GPIO support Sander Vanheule
2021-05-17 21:42 ` Andy Shevchenko
2021-05-17 21:46 ` Andy Shevchenko
2021-05-23 21:42 ` Sander Vanheule
2021-05-17 19:28 ` [PATCH v2 7/7] leds: Add support for RTL8231 LED scan matrix Sander Vanheule
2021-05-17 22:00 ` Andy Shevchenko
2021-05-23 21:53 ` Sander Vanheule
2021-05-19 16:10 ` (subset) [PATCH v2 0/7] RTL8231 GPIO expander support Mark Brown
2021-05-23 22:33 ` [PATCH v3 0/6] " Sander Vanheule
2021-05-23 22:33 ` [PATCH v3 1/6] gpio: regmap: Add quirk for output data register Sander Vanheule
2021-05-28 6:40 ` Michael Walle
2021-06-03 10:03 ` Sander Vanheule
2021-05-31 7:25 ` Bartosz Golaszewski
2021-05-23 22:34 ` [PATCH v3 2/6] dt-bindings: leds: Binding for RTL8231 scan matrix Sander Vanheule
2021-06-02 18:58 ` Rob Herring
2021-05-23 22:34 ` [PATCH v3 3/6] dt-bindings: mfd: Binding for RTL8231 Sander Vanheule
2021-05-27 23:31 ` Linus Walleij
2021-06-02 19:02 ` Rob Herring
2021-05-23 22:34 ` [PATCH v3 4/6] mfd: Add RTL8231 core device Sander Vanheule
2021-05-24 8:02 ` Andy Shevchenko
2021-05-24 8:23 ` Sander Vanheule
2021-05-24 10:18 ` Andy Shevchenko
2021-05-24 11:41 ` Sander Vanheule
2021-05-23 22:34 ` [PATCH v3 5/6] pinctrl: Add RTL8231 pin control and GPIO support Sander Vanheule
2021-05-28 6:29 ` Michael Walle
2021-05-28 6:42 ` Sander Vanheule
2021-05-28 6:43 ` Michael Walle
2021-05-23 22:34 ` [PATCH v3 6/6] leds: Add support for RTL8231 LED scan matrix Sander Vanheule
2021-05-24 10:24 ` Andy Shevchenko
2021-05-24 12:04 ` Sander Vanheule
2021-05-24 12:47 ` Andy Shevchenko
2021-05-24 15:30 ` Sander Vanheule
2021-05-24 1:10 ` [PATCH v3 0/6] RTL8231 GPIO expander support Andrew Lunn
2021-05-24 7:53 ` Andy Shevchenko
2021-05-24 11:41 ` Sander Vanheule
2021-05-24 12:54 ` Andy Shevchenko
2021-05-24 15:03 ` Sander Vanheule
2021-05-24 16:30 ` Andy Shevchenko
2021-05-25 17:11 ` Andy Shevchenko
2021-05-25 18:00 ` Sander Vanheule
2021-05-26 21:02 ` Sander Vanheule
2021-05-27 10:38 ` Andy Shevchenko
2021-05-27 10:41 ` Hans de Goede
2021-05-24 15:20 ` Sander Vanheule
2021-05-28 6:37 ` Michael Walle
2021-05-30 16:19 ` Sander Vanheule [this message]
2021-05-30 16:51 ` Hans de Goede
2021-05-30 18:16 ` Andy Shevchenko
2021-05-30 21:22 ` Michael Walle
2021-05-31 8:36 ` Sander Vanheule
2021-05-31 10:02 ` Michael Walle
[not found] ` <CAHp75VfOrUBRQH1vrXEwHN4ZPojQfQju-_wp_3djZeozEaatug@mail.gmail.com>
2021-05-31 15:33 ` [PATCH 0/5] " Sander Vanheule
2021-05-31 15:48 ` Andy Shevchenko
2021-06-01 11:49 ` Michael Walle
2021-06-01 15:24 ` Andy Shevchenko
2021-06-02 20:20 ` Sander Vanheule
2021-06-01 9:59 ` [PATCH v3 0/6] " Linus Walleij
2021-06-01 10:18 ` Michael Walle
2021-06-01 10:51 ` Linus Walleij
2021-06-01 11:41 ` Michael Walle
2021-06-01 11:48 ` Linus Walleij
2021-06-03 10:00 ` [PATCH v4 0/5] " Sander Vanheule
2021-06-03 10:00 ` [PATCH v4 1/5] dt-bindings: leds: Binding for RTL8231 scan matrix Sander Vanheule
2021-06-03 10:00 ` [PATCH v4 2/5] dt-bindings: mfd: Binding for RTL8231 Sander Vanheule
2021-06-03 10:00 ` [PATCH v4 3/5] mfd: Add RTL8231 core device Sander Vanheule
2021-06-03 10:58 ` Andy Shevchenko
2021-06-03 11:28 ` Sander Vanheule
2021-06-03 14:03 ` Andrew Lunn
2021-06-03 15:20 ` Sander Vanheule
2021-06-05 11:07 ` Sander Vanheule
2021-06-03 10:00 ` [PATCH v4 4/5] pinctrl: Add RTL8231 pin control and GPIO support Sander Vanheule
2021-06-03 10:18 ` Andy Shevchenko
2021-06-04 22:10 ` Linus Walleij
2021-06-03 10:00 ` [PATCH v4 5/5] leds: Add support for RTL8231 LED scan matrix Sander Vanheule
2021-06-03 11:01 ` Andy Shevchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=84352c93f27d7c8b7afea54f3932020e9cd97d02.camel@svanheule.net \
--to=sander@svanheule.net \
--cc=andrew@lunn.ch \
--cc=andy.shevchenko@gmail.com \
--cc=bgolaszewski@baylibre.com \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=hdegoede@redhat.com \
--cc=lee.jones@linaro.org \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=michael@walle.cc \
--cc=pavel@ucw.cz \
--cc=rafael@kernel.org \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).