mbox series

[0/5] RTL8231 GPIO expander support

Message ID cover.1620735871.git.sander@svanheule.net
Headers show
Series RTL8231 GPIO expander support | expand

Message

Sander Vanheule May 11, 2021, 12:25 p.m. UTC
The RTL8231 GPIO and LED expander can be configured for use as an MDIO or SMI
bus device. Currently only the MDIO mode is supported, although SMI mode
support should be fairly straightforward, once an SMI bus driver is available.

Provided features by the RTL8231:
  - Up to 37 GPIOs
    - Configurable drive strength: 8mA or 4mA (currently unsupported)
    - Input debouncing on high GPIOs (currently unsupported)
  - Up to 88 LEDs in multiple scan matrix groups
    - On, off, or one of six toggling intervals
    - "single-color mode": 2×36 single color LEDs + 8 bi-color LEDs
    - "bi-color mode": (12 + 2×6) bi-color LEDs + 24 single color LEDs
  - Up to one PWM output (currently unsupported)
    - Fixed duty cycle, 8 selectable frequencies (1.2kHz - 4.8kHz)

There remain some log warnings when probing the device, possibly due to the way
I'm using the MFD subsystem. Would it be possible to avoid these?
[    2.602242] rtl8231-pinctrl: Failed to locate of_node [id: -2]
[    2.609380] rtl8231-pinctrl rtl8231-pinctrl.0.auto: no of_node; not parsing pinctrl DT

When no 'leds' sub-node is specified:
[    2.922262] rtl8231-leds: Failed to locate of_node [id: -2]
[    2.967149] rtl8231-leds rtl8231-leds.1.auto: no of_node; not parsing pinctrl DT
[    2.975673] rtl8231-leds rtl8231-leds.1.auto: scan mode missing or invalid
[    2.983531] rtl8231-leds: probe of rtl8231-leds.1.auto failed with error -22

Changes since RFC:
  - Dropped MDIO regmap interface. I was unable to resolve the Kconfig
    dependency issue, so have reverted to using regmap_config.reg_read/write.
  - Added pinctrl support
  - Added LED support
  - Changed root device to MFD, with pinctrl and leds child devices. Root
    device is now an mdio_device driver.

Sander Vanheule (5):
  dt-bindings: leds: Binding for RTL8231 scan matrix
  dt-bindings: mfd: Binding for RTL8231
  mfd: Add RTL8231 core device
  pinctrl: Add RTL8231 pin control and GPIO support
  leds: Add support for RTL8231 LED scan matrix

 .../bindings/leds/realtek,rtl8231-leds.yaml   | 159 ++++++
 .../bindings/mfd/realtek,rtl8231.yaml         | 202 +++++++
 drivers/leds/Kconfig                          |  10 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-rtl8231.c                   | 281 ++++++++++
 drivers/mfd/Kconfig                           |   9 +
 drivers/mfd/Makefile                          |   1 +
 drivers/mfd/rtl8231.c                         | 163 ++++++
 drivers/pinctrl/Kconfig                       |  10 +
 drivers/pinctrl/Makefile                      |   1 +
 drivers/pinctrl/pinctrl-rtl8231.c             | 497 ++++++++++++++++++
 include/linux/mfd/rtl8231.h                   |  49 ++
 12 files changed, 1383 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/realtek,rtl8231-leds.yaml
 create mode 100644 Documentation/devicetree/bindings/mfd/realtek,rtl8231.yaml
 create mode 100644 drivers/leds/leds-rtl8231.c
 create mode 100644 drivers/mfd/rtl8231.c
 create mode 100644 drivers/pinctrl/pinctrl-rtl8231.c
 create mode 100644 include/linux/mfd/rtl8231.h

Comments

Sander Vanheule May 16, 2021, 9:40 p.m. UTC | #1
On Wed, 2021-05-12 at 18:29 +0300, Andy Shevchenko wrote:
> 
> 
> On Tuesday, May 11, 2021, Sander Vanheule <sander@svanheule.net> wrote:
> > The RTL8231 GPIO and LED expander can be configured for use as an MDIO or
> > SMI
> > bus device. Currently only the MDIO mode is supported, although SMI mode
> > support should be fairly straightforward, once an SMI bus driver is
> > available.
> > 
> > Provided features by the RTL8231:
> >   - Up to 37 GPIOs
> >     - Configurable drive strength: 8mA or 4mA (currently unsupported)
> >     - Input debouncing on high GPIOs (currently unsupported)
> >   - Up to 88 LEDs in multiple scan matrix groups
> >     - On, off, or one of six toggling intervals
> >     - "single-color mode": 2×36 single color LEDs + 8 bi-color LEDs
> >     - "bi-color mode": (12 + 2×6) bi-color LEDs + 24 single color LEDs
> >   - Up to one PWM output (currently unsupported)
> >     - Fixed duty cycle, 8 selectable frequencies (1.2kHz - 4.8kHz)
> > 
> > There remain some log warnings when probing the device, possibly due to the
> > way
> > I'm using the MFD subsystem. Would it be possible to avoid these?
> > [    2.602242] rtl8231-pinctrl: Failed to locate of_node [id: -2]
> > [    2.609380] rtl8231-pinctrl rtl8231-pinctrl.0.auto: no of_node; not
> > parsing pinctrl DT
> > 
> > When no 'leds' sub-node is specified:
> > [    2.922262] rtl8231-leds: Failed to locate of_node [id: -2]
> > [    2.967149] rtl8231-leds rtl8231-leds.1.auto: no of_node; not parsing
> > pinctrl DT
> > [    2.975673] rtl8231-leds rtl8231-leds.1.auto: scan mode missing or
> > invalid
> > [    2.983531] rtl8231-leds: probe of rtl8231-leds.1.auto failed with error
> > -22
> > 
> > 
> 
> 
> I have several comments to the series, but I may give them next week.
> 
> Just couple here:
> 1. If subsystem provides a regmap API I would suggest to use it, I.o.w. try
> again to understand what is wrong with MDIO case.

Are you referring to the MDIO regmap interface, or the GPIO regmap interface?

For the MDIO regmap interface, I have been able to resolve the Kconfig
dependency issue. So I can reintroduce that, if that's preferred over the
solution in this v1.

With an extra patch, I was able to use the gpio-regmap interface, dropping most
of the GPIO code. The current gpio-regmap implementation makes the assumption
that an output value can be set while a pin is configured as an input. That
assumption is invalid for this chip, so I had to provide an extra flag for
gpio_regmap_config, similar to how this is handled in gpio-mmio.


> 2. Please, switch to fwnode API in LED driver

Since you had the same comment on my previous patch set, I had already tried to
this this into account as much as possible.

There's a few things I couldn't find the fwnode-equivalent for:
 * I use of_node_name_prefix to enforce the naming required by the binding. I
   could just walk over all (available) child nodes, which would be mostly
   equivalent.
 * To get the address of an LED child node, I use of_get_address, since this
   appeared to provide what I want to do: get the address of the node. I know
   next to nothing about ACPI. Does the equivalent exist there? Or am I taking
   the wrong approach?


I have updated patches ready, if you would rather just review a v2.


Best,
Sander
Andy Shevchenko May 17, 2021, 8:13 a.m. UTC | #2
On Mon, May 17, 2021 at 12:40 AM Sander Vanheule <sander@svanheule.net> wrote:
> On Wed, 2021-05-12 at 18:29 +0300, Andy Shevchenko wrote:

> > On Tuesday, May 11, 2021, Sander Vanheule <sander@svanheule.net> wrote:


...

> > I have several comments to the series, but I may give them next week.

> >

> > Just couple here:

> > 1. If subsystem provides a regmap API I would suggest to use it, I.o.w. try

> > again to understand what is wrong with MDIO case.

>

> Are you referring to the MDIO regmap interface, or the GPIO regmap interface?


MDIO

> For the MDIO regmap interface, I have been able to resolve the Kconfig

> dependency issue. So I can reintroduce that, if that's preferred over the

> solution in this v1.

>

> With an extra patch, I was able to use the gpio-regmap interface, dropping most

> of the GPIO code. The current gpio-regmap implementation makes the assumption

> that an output value can be set while a pin is configured as an input. That

> assumption is invalid for this chip, so I had to provide an extra flag for

> gpio_regmap_config, similar to how this is handled in gpio-mmio.

>

>

> > 2. Please, switch to fwnode API in LED driver

>

> Since you had the same comment on my previous patch set, I had already tried to

> this this into account as much as possible.

>

> There's a few things I couldn't find the fwnode-equivalent for:

>  * I use of_node_name_prefix to enforce the naming required by the binding. I

>    could just walk over all (available) child nodes, which would be mostly

>    equivalent.


AFAIU the LED traditional bindings is that you define LED compatible
nodes and all child nodes of it are the one-per-LED ones, there
shouldn't be others.

>  * To get the address of an LED child node, I use of_get_address, since this

>    appeared to provide what I want to do: get the address of the node. I know

>    next to nothing about ACPI. Does the equivalent exist there? Or am I taking

>    the wrong approach?


What are the means of an address in this case?

> I have updated patches ready, if you would rather just review a v2.


-- 
With Best Regards,
Andy Shevchenko
Sander Vanheule May 17, 2021, 8:50 a.m. UTC | #3
On Mon, 2021-05-17 at 11:13 +0300, Andy Shevchenko wrote:
> On Mon, May 17, 2021 at 12:40 AM Sander Vanheule <sander@svanheule.net> wrote:

> > On Wed, 2021-05-12 at 18:29 +0300, Andy Shevchenko wrote:

> > > On Tuesday, May 11, 2021, Sander Vanheule <sander@svanheule.net> wrote:

> 

> ...

> 

> > >  * > > > I have several comments to the series, but I may give them next

> > >    week.

> > > 

> > > Just couple here:

> > > 1. If subsystem provides a regmap API I would suggest to use it, I.o.w.

> > > try

> > > again to understand what is wrong with MDIO case.

> > 

> > Are you referring to the MDIO regmap interface, or the GPIO regmap

> > interface?

> 

> MDIO

> 

> > For the MDIO regmap interface, I have been able to resolve the Kconfig

> > dependency issue. So I can reintroduce that, if that's preferred over the

> > solution in this v1.

> > 

> > With an extra patch, I was able to use the gpio-regmap interface, dropping

> > most

> > of the GPIO code. The current gpio-regmap implementation makes the

> > assumption

> > that an output value can be set while a pin is configured as an input. That

> > assumption is invalid for this chip, so I had to provide an extra flag for

> > gpio_regmap_config, similar to how this is handled in gpio-mmio.

> > 

> > 

> > > 2. Please, switch to fwnode API in LED driver

> > 

> > Since you had the same comment on my previous patch set, I had already tried

> > to

> > this this into account as much as possible.

> > 

> > There's a few things I couldn't find the fwnode-equivalent for:

> >  * I use of_node_name_prefix to enforce the naming required by the binding.

> > I

> >    could just walk over all (available) child nodes, which would be mostly

> >    equivalent.

> 

> AFAIU the LED traditional bindings is that you define LED compatible

> nodes and all child nodes of it are the one-per-LED ones, there

> shouldn't be others.


OK, then I can just iterate over all child fwnodes.


> >  * To get the address of an LED child node, I use of_get_address, since this

> >    appeared to provide what I want to do: get the address of the node. I

> > know

> >    next to nothing about ACPI. Does the equivalent exist there? Or am I

> > taking

> >    the wrong approach?

> 

> What are the means of an address in this case?


The chip appears to be intended for use with ethernet switches. The registers
are organised to into a few groups, to provide 2 or 3 status LEDs per switch
port:

 * "LED0" group for 32 ports,
 * "LED1" group for 32 ports,
 * "LED2" group for 24 ports

The number of LEDs that can be used depends on the output mode, so I use a two-
part <#PORT #LED> address, resembling how this is defined by Realtek.

A single linear LED address space would get awkward gaps in bi-color mode (where
only the lower 24 ports can be used), but would still require addresses to be
able to specify which LED is where. For example in case the user want to link
them to a phy trigger for a specific switch port.

Best,
Sander
Sander Vanheule May 17, 2021, 7:28 p.m. UTC | #4
The RTL8231 GPIO and LED expander can be configured for use as an MDIO or SMI
bus device. Currently only the MDIO mode is supported, although SMI mode
support should be fairly straightforward, once an SMI bus driver is available.

Provided features by the RTL8231:
  - Up to 37 GPIOs
    - Configurable drive strength: 8mA or 4mA (currently unsupported)
    - Input debouncing on high GPIOs (currently unsupported)
  - Up to 88 LEDs in multiple scan matrix groups
    - On, off, or one of six toggling intervals
    - "single-color mode": 2×36 single color LEDs + 8 bi-color LEDs
    - "bi-color mode": (12 + 2×6) bi-color LEDs + 24 single color LEDs
  - Up to one PWM output (currently unsupported)
    - Fixed duty cycle, 8 selectable frequencies (1.2kHz - 4.8kHz)

Register access is provided through a new MDIO regmap provider. The GPIO
controller uses gpio-regmap, although a patch is required to support a
limitation of the chip.

There remain some log warnings when probing the device, possibly due to the way
I'm using the MFD subsystem. Would it be possible to avoid these?
[    2.602242] rtl8231-pinctrl: Failed to locate of_node [id: -2]
[    2.609380] rtl8231-pinctrl rtl8231-pinctrl.0.auto: no of_node; not parsing pinctrl DT

When no 'leds' sub-node is specified:
[    2.922262] rtl8231-leds: Failed to locate of_node [id: -2]
[    2.967149] rtl8231-leds rtl8231-leds.1.auto: no of_node; not parsing pinctrl DT
[    2.975673] rtl8231-leds rtl8231-leds.1.auto: scan mode missing or invalid
[    2.983531] rtl8231-leds: probe of rtl8231-leds.1.auto failed with error -22

Changes since v1:
  - Reintroduce MDIO regmap, with fixed Kconfig dependencies
  - Add configurable dir/value order for gpio-regmap direction_out call
  - Drop allocations for regmap fields that are used only on init
  - Move some definitions to MFD header
  - Add PM ops to replace driver remove for MFD
  - Change pinctrl driver to (modified) gpio-regmap
  - Change leds driver to use fwnode
Link: https://lore.kernel.org/lkml/cover.1620735871.git.sander@svanheule.net/

Changes since RFC:
  - Dropped MDIO regmap interface. I was unable to resolve the Kconfig
    dependency issue, so have reverted to using regmap_config.reg_read/write.
  - Added pinctrl support
  - Added LED support
  - Changed root device to MFD, with pinctrl and leds child devices. Root
    device is now an mdio_device driver.
Link: https://lore.kernel.org/linux-gpio/cover.1617914861.git.sander@svanheule.net/

Sander Vanheule (7):
  regmap: Add MDIO bus support
  gpio: regmap: Add configurable dir/value order
  dt-bindings: leds: Binding for RTL8231 scan matrix
  dt-bindings: mfd: Binding for RTL8231
  mfd: Add RTL8231 core device
  pinctrl: Add RTL8231 pin control and GPIO support
  leds: Add support for RTL8231 LED scan matrix

 .../bindings/leds/realtek,rtl8231-leds.yaml   | 159 ++++++++
 .../bindings/mfd/realtek,rtl8231.yaml         | 202 ++++++++++
 drivers/base/regmap/Kconfig                   |   6 +-
 drivers/base/regmap/Makefile                  |   1 +
 drivers/base/regmap/regmap-mdio.c             |  57 +++
 drivers/gpio/gpio-regmap.c                    |  20 +-
 drivers/leds/Kconfig                          |  10 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-rtl8231.c                   | 293 ++++++++++++++
 drivers/mfd/Kconfig                           |   9 +
 drivers/mfd/Makefile                          |   1 +
 drivers/mfd/rtl8231.c                         | 153 +++++++
 drivers/pinctrl/Kconfig                       |  11 +
 drivers/pinctrl/Makefile                      |   1 +
 drivers/pinctrl/pinctrl-rtl8231.c             | 377 ++++++++++++++++++
 include/linux/gpio/regmap.h                   |   3 +
 include/linux/mfd/rtl8231.h                   |  57 +++
 include/linux/regmap.h                        |  36 ++
 18 files changed, 1393 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/leds/realtek,rtl8231-leds.yaml
 create mode 100644 Documentation/devicetree/bindings/mfd/realtek,rtl8231.yaml
 create mode 100644 drivers/base/regmap/regmap-mdio.c
 create mode 100644 drivers/leds/leds-rtl8231.c
 create mode 100644 drivers/mfd/rtl8231.c
 create mode 100644 drivers/pinctrl/pinctrl-rtl8231.c
 create mode 100644 include/linux/mfd/rtl8231.h
Sander Vanheule May 17, 2021, 7:32 p.m. UTC | #5
On Sun, 2021-05-16 at 23:40 +0200, Sander Vanheule wrote:
> > 2. Please, switch to fwnode API in LED driver

> 

> Since you had the same comment on my previous patch set, I had already tried to

> this this into account as much as possible.

> 

> There's a few things I couldn't find the fwnode-equivalent for:

>  * I use of_node_name_prefix to enforce the naming required by the binding. I

>    could just walk over all (available) child nodes, which would be mostly

>    equivalent.

>  * To get the address of an LED child node, I use of_get_address, since this

>    appeared to provide what I want to do: get the address of the node. I know

>    next to nothing about ACPI. Does the equivalent exist there? Or am I taking

>    the wrong approach?


Hi Andy,

I found this:
https://www.kernel.org/doc/html/latest/firmware-guide/acpi/dsd/leds.html

So instead of of_address, I now just read the "reg" property. The v2 I just sent
should be fwnode-only.

Best,
Sander
Andy Shevchenko May 17, 2021, 10 p.m. UTC | #6
On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@svanheule.net> wrote:
>
> Both single and bi-color scanning modes are supported. The driver will
> verify that the addresses are valid for the current mode, before
> registering the LEDs. LEDs can be turned on, off, or toggled at one of
> six predefined rates from 40ms to 1280ms.
>
> Implements a platform device for use as child device with RTL8231 MFD,

as a child

> and uses the parent regmap to access the required registers.

...

> +         When built as a module, this module will be named rtl8231_leds.

Again, what it's written here is not what is in Makefile.

> +obj-$(CONFIG_LEDS_RTL8231)             += leds-rtl8231.o

...

> +/**
> + * struct led_toggle_rate - description of an LED blinking mode
> + * @interval:  LED toggle rate in ms
> + * @mode:      Register field value used to active this mode

activate

> + *
> + * For LED hardware accelerated blinking, with equal on and off delay.
> + * Both delays are given by @interval, so the interval at which the LED blinks
> + * (i.e. turn on and off once) is double this value.
> + */

...

> +static unsigned int rtl8231_led_current_interval(struct rtl8231_led *pled)
> +{
> +       unsigned int mode;

> +       unsigned int i = 0;

This...

> +       if (regmap_field_read(pled->reg_field, &mode))
> +               return 0;
> +
> +       while (i < pled->modes->num_toggle_rates && mode != pled->modes->toggle_rates[i].mode)
> +               i++;

...and this will be better as a for-loop.

> +       if (i < pled->modes->num_toggle_rates)
> +               return pled->modes->toggle_rates[i].interval;

> +       else

Redundant.

> +               return 0;
> +}

...

> +       unsigned int i = 0;

As per above.

...

> +               interval = 500;

interval_ms

> +               /*
> +                * If the current mode is blinking, choose the delay that (likely) changed.
> +                * Otherwise, choose the interval that would have the same total delay.
> +                */
> +               interval = rtl8231_led_current_interval(pled);

> +

Redundant blank line.

> +               if (interval > 0 && interval == *delay_off)
> +                       interval = *delay_on;
> +               else if (interval > 0 && interval == *delay_on)
> +                       interval = *delay_off;
> +               else
> +                       interval = (*delay_on + *delay_off) / 2;
> +       }

...

> +       u32 addr[2];
> +       int err;
> +

> +       if (!fwnode_property_count_u32(fwnode, "reg"))

err = fwnode_property_count_u32(...);
if (err < 0)
  return err;
if (err == 0)
  return -ENODEV;

> +               return -ENODEV;
> +
> +       err = fwnode_property_read_u32_array(fwnode, "reg", addr, ARRAY_SIZE(addr));

If count returns 1? What's the point of counting if you always want two?

> +       if (err)
> +               return err;
> +
> +       *addr_port = addr[0];
> +       *addr_led = addr[1];
> +
> +       return 0;
> +}

...

> +       pled = devm_kzalloc(dev, sizeof(*pled), GFP_KERNEL);
> +       if (IS_ERR(pled))

Wrong.

> +               return PTR_ERR(pled);

...

> +       err = rtl8231_led_read_address(fwnode, &port_index, &led_index);
> +

Redundant blank line.

> +       if (err) {
> +               dev_err(dev, "LED address invalid\n");
> +               return err;

> +       } else if (led_index >= RTL8231_NUM_LEDS || port_index >= port_counts[led_index]) {

Redundant 'else'

> +               dev_err(dev, "LED address (%d.%d) invalid\n", port_index, led_index);
> +               return -ENODEV;
> +       }

...

> +       map = dev_get_regmap(dev->parent, NULL);
> +       if (IS_ERR_OR_NULL(map)) {

Split it into two conditionals.

> +               dev_err(dev, "failed to retrieve regmap\n");
> +               if (!map)
> +                       return -ENODEV;
> +               else
> +                       return PTR_ERR(map);
> +       }

...

> +       if (!device_property_match_string(dev, "realtek,led-scan-mode", "single-color")) {

It seems that device_property_match_string() and accompanying
functions have wrong description of returned codes, i.e. it returns
the index of the matched string. It's possible that some APIs are
broken (but I believe that the former is the case).

That said, I think the proper comparison should be >= 0.

> +               port_counts = rtl8231_led_port_counts_single;
> +               regmap_update_bits(map, RTL8231_REG_FUNC0,
> +                       RTL8231_FUNC0_SCAN_MODE, RTL8231_FUNC0_SCAN_SINGLE);
> +       } else if (!device_property_match_string(dev, "realtek,led-scan-mode", "bi-color")) {

Ditto.

> +               port_counts = rtl8231_led_port_counts_bicolor;
> +               regmap_update_bits(map, RTL8231_REG_FUNC0,
> +                       RTL8231_FUNC0_SCAN_MODE, RTL8231_FUNC0_SCAN_BICOLOR);
> +       } else {
> +               dev_err(dev, "scan mode missing or invalid\n");
> +               return -EINVAL;
> +       }
Rob Herring (Arm) May 17, 2021, 10:38 p.m. UTC | #7
On Tue, May 11, 2021 at 02:25:20PM +0200, Sander Vanheule wrote:
> Add a binding description for the Realtek RTL8231, a GPIO and LED
> expander chip commonly used in ethernet switches based on a Realtek
> switch SoC. These chips can be addressed via an MDIO or SMI bus, or used
> as a plain 36-bit shift register.
> 
> This binding only describes the feature set provided by the MDIO/SMI
> configuration, and covers the GPIO, PWM, and pin control properties. The
> LED properties are defined in a separate binding.
> 
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> ---
>  .../bindings/mfd/realtek,rtl8231.yaml         | 202 ++++++++++++++++++
>  1 file changed, 202 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/realtek,rtl8231.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/realtek,rtl8231.yaml b/Documentation/devicetree/bindings/mfd/realtek,rtl8231.yaml
> new file mode 100644
> index 000000000000..2023cfa887a3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/realtek,rtl8231.yaml
> @@ -0,0 +1,202 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/realtek,rtl8231.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Realtek RTL8231 GPIO and LED expander.
> +
> +maintainers:
> +  - Sander Vanheule <sander@svanheule.net>
> +
> +description: |
> +  The RTL8231 is a GPIO and LED expander chip, providing up to 37 GPIOs, up to
> +  88 LEDs, and up to one PWM output. This device is frequently used alongside
> +  Realtek switch SoCs, to provide additional I/O capabilities.
> +
> +  To manage the RTL8231's features, its strapping pins can be used to configure
> +  it in one of three modes: shift register, MDIO device, or SMI device. The
> +  shift register mode does not need special support. In MDIO or SMI mode, most
> +  pins can be configured as a GPIO output, LED matrix scan line/column, or as a
> +  PWM output.
> +
> +  The GPIO and pin control are part of the main node. PWM and LED support are
> +  configured as sub-nodes.
> +
> +properties:
> +  compatible:
> +    const: realtek,rtl8231
> +
> +  reg:
> +    description: MDIO or SMI device address.
> +    maxItems: 1
> +
> +  # GPIO support
> +  gpio-controller: true
> +
> +  "#gpio-cells":
> +    const: 2
> +    description: |
> +      The first cell is the pin number and the second cell is used to specify
> +      the gpio active state.
> +
> +  gpio-ranges:
> +    description: |
> +      Must reference itself, and provide a zero-based mapping for 37 pins.
> +    maxItems: 1
> +
> +  # Pin muxing and configuration
> +  realtek,drive-strength:
> +    $ref: /schemas/types.yaml#/definitions/uint32

Use the standard 'drive-strength' property.

> +    description: |
> +      Common drive strength used for all GPIO output pins, must be 4mA or 8mA.
> +      On reset, this value will default to 8mA.
> +    enum: [4, 8]
> +
> +  # LED scanning matrix
> +  leds:
> +    $ref: ../leds/realtek,rtl8231-leds.yaml#
> +
> +  # PWM output
> +  pwm:
> +    type: object
> +    description: |
> +      Subnode describing the PWM peripheral. To use the PWM output, gpio35 must
> +      be muxed to its 'pwm' function. Valid frequency values for consumers are
> +      1200, 1600, 2000, 2400, 2800, 3200, 4000, and 4800.
> +
> +    properties:
> +      "#pwm-cells":
> +        description: |
> +          Twos cells with PWM index (must be 0) and PWM frequency in Hz.
> +        const: 2
> +
> +    required:
> +      - "#pwm-cells"

Just move this to the parent node. No reason for a child node or that 1 
node can't be 2 providers.

> +
> +patternProperties:
> +  "-pins$":
> +    type: object
> +    $ref: ../pinctrl/pinmux-node.yaml#
> +
> +    properties:
> +      pins:
> +        items:
> +          oneOf:

No need for oneOf when there's only 1 entry.

> +            - enum: ["gpio0", "gpio1", "gpio2", "gpio3", "gpio4", "gpio5", "gpio6",
> +                     "gpio7", "gpio8", "gpio9", "gpio10", "gpio11", "gpio12", "gpio13",
> +                     "gpio14", "gpio15", "gpio16", "gpio17", "gpio18", "gpio19", "gpio20",
> +                     "gpio21", "gpio22", "gpio23", "gpio24", "gpio25", "gpio26", "gpio27",
> +                     "gpio28", "gpio29", "gpio30", "gpio31", "gpio32", "gpio33", "gpio34",
> +                     "gpio35", "gpio36"]
> +        minItems: 1
> +        maxItems: 37
> +      function:
> +        description: |
> +          Select which function to use. "gpio" is supported for all pins, "led" is supported
> +          for pins 0-34, "pwm" is supported for for pin 35.
> +        enum: ["gpio", "led", "pwm"]
> +
> +    required:
> +      - pins
> +      - function
> +
> +required:
> +  - compatible
> +  - reg
> +  - gpio-controller
> +  - "#gpio-cells"
> +  - gpio-ranges
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    // Minimal example
> +    mdio {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        expander0: expander@0 {
> +            compatible = "realtek,rtl8231";
> +            reg = <0>;
> +
> +            gpio-controller;
> +            #gpio-cells = <2>;
> +            gpio-ranges = <&expander0 0 0 37>;
> +        };
> +    };
> +  - |
> +    // All bells and whistles included
> +    #include <dt-bindings/leds/common.h>
> +    mdio {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        expander1: expander@1 {
> +            compatible = "realtek,rtl8231";
> +            reg = <1>;
> +
> +            gpio-controller;
> +            #gpio-cells = <2>;
> +            gpio-ranges = <&expander1 0 0 37>;
> +
> +            realtek,drive-strength = <4>;
> +
> +            button-pins {
> +                pins = "gpio36";
> +                function = "gpio";
> +                input-debounce = "100000";
> +            };
> +
> +            pwm-pins {
> +                pins = "gpio35";
> +                function = "pwm";
> +            };
> +
> +            led-pins {
> +                pins = "gpio0", "gpio1", "gpio3", "gpio4";
> +                function = "led";
> +            };
> +
> +            pwm {
> +                #pwm-cells = <2>;
> +            };
> +
> +            leds {
> +                compatible = "realtek,rtl8231-leds";
> +                #address-cells = <2>;
> +                #size-cells = <0>;
> +
> +                realtek,led-scan-mode = "single-color";
> +
> +                led@0,0 {
> +                    reg = <0 0>;
> +                    color = <LED_COLOR_ID_GREEN>;
> +                    function = LED_FUNCTION_LAN;
> +                    function-enumerator = <0>;
> +                };
> +
> +                led@0,1 {
> +                    reg = <0 1>;
> +                    color = <LED_COLOR_ID_AMBER>;
> +                    function = LED_FUNCTION_LAN;
> +                    function-enumerator = <0>;
> +                };
> +
> +                led@1,0 {
> +                    reg = <1 0>;
> +                    color = <LED_COLOR_ID_GREEN>;
> +                    function = LED_FUNCTION_LAN;
> +                    function-enumerator = <1>;
> +                };
> +
> +                led@1,1 {
> +                    reg = <1 1>;
> +                    color = <LED_COLOR_ID_AMBER>;
> +                    function = LED_FUNCTION_LAN;
> +                    function-enumerator = <1>;
> +                };
> +            };
> +        };
> +    };
> -- 
> 2.31.1
>
Mark Brown May 19, 2021, 4:10 p.m. UTC | #8
On Mon, 17 May 2021 21:28:02 +0200, Sander Vanheule wrote:
> The RTL8231 GPIO and LED expander can be configured for use as an MDIO or SMI

> bus device. Currently only the MDIO mode is supported, although SMI mode

> support should be fairly straightforward, once an SMI bus driver is available.

> 

> Provided features by the RTL8231:

>   - Up to 37 GPIOs

>     - Configurable drive strength: 8mA or 4mA (currently unsupported)

>     - Input debouncing on high GPIOs (currently unsupported)

>   - Up to 88 LEDs in multiple scan matrix groups

>     - On, off, or one of six toggling intervals

>     - "single-color mode": 2×36 single color LEDs + 8 bi-color LEDs

>     - "bi-color mode": (12 + 2×6) bi-color LEDs + 24 single color LEDs

>   - Up to one PWM output (currently unsupported)

>     - Fixed duty cycle, 8 selectable frequencies (1.2kHz - 4.8kHz)

> 

> [...]


Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next

Thanks!

[1/7] regmap: Add MDIO bus support
      commit: 1f89d2fe16072a74b34bdb895160910091427891

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark
Sander Vanheule May 23, 2021, 9:28 p.m. UTC | #9
Hi Andy,

I've implemented the minor remarks (redundant assignments, if/else code
structure...). Some extra details below.

On Tue, 2021-05-18 at 00:18 +0300, Andy Shevchenko wrote:
> On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@svanheule.net> wrote:
> > 
> > The RTL8231 is implemented as an MDIO device, and provides a regmap
> > interface for register access by the core and child devices.
> > 
> > The chip can also be a device on an SMI bus, an I2C-like bus by Realtek.
> > Since kernel support for SMI is limited, and no real-world SMI
> > implementations have been encountered for this device, this is currently
> > unimplemented. The use of the regmap interface should make any future
> > support relatively straightforward.
> > 
> > After reset, all pins are muxed to GPIO inputs before the pin drivers
> > are enabled. This is done to prevent accidental system resets, when a
> > pin is connected to the parent SoC's reset line.
> 
> > [missing MDIO_BUS dependency, provided via REGMAP_MDIO]
> > Reported-by: kernel test robot <lkp@intel.com>
> 
> What is the culprit? Shouldn't this have a Fixes tag?

But it doesn't actually fix an issue created by an existing commit, just
something that was wrong in the first version of the patch.  This patch is not
dedicated to fixing that single issue though, it's just a part of it. Hence the
note above the Reported-by tag.

> > 
> > +       mdiodev->reset_gpio = gpiod_get_optional(dev, "reset",
> > GPIOD_OUT_LOW);
> > +       device_property_read_u32(dev, "reset-assert-delay", &mdiodev-
> > >reset_assert_delay);
> > +       device_property_read_u32(dev, "reset-deassert-delay", &mdiodev-
> > >reset_deassert_delay);
> > +
> > +       err = rtl8231_init(dev, map);
> > +       if (err)
> 
> Resource leakage.

Replaced gpiod_get_optional by devm_gpiod_get_optional.

> 
> > +               return err;
> > +
> > +       /* LED_START enables power to output pins, and starts the LED engine
> > */
> > +       regmap_field_write(led_start, 1);
> 
> > +       return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, rtl8231_cells,
> > +               ARRAY_SIZE(rtl8231_cells), NULL, 0, NULL);
> 
> Ditto.
> 
> > +}
> 
> ...
> 
> > +#ifdef CONFIG_PM
> 
> Replace this with __maybe_unused attribute.

Done. I've also used a few extra macros from PM header to clean this part up a
bit more.



Best,
Sander
Sander Vanheule May 23, 2021, 9:53 p.m. UTC | #10
Hi Andy,

Also here I've tried to address your remarks for v3, some extra details below.

On Tue, 2021-05-18 at 01:00 +0300, Andy Shevchenko wrote:
> On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@svanheule.net> wrote:
> > +static unsigned int rtl8231_led_current_interval(struct rtl8231_led *pled)
> > +{
> > +       unsigned int mode;
> 
> > +       unsigned int i = 0;
> 
> This...
> 
> > +       if (regmap_field_read(pled->reg_field, &mode))
> > +               return 0;
> > +
> > +       while (i < pled->modes->num_toggle_rates && mode != pled->modes-
> > >toggle_rates[i].mode)
> > +               i++;
> 
> ...and this will be better as a for-loop.
> 
> > +       if (i < pled->modes->num_toggle_rates)
> > +               return pled->modes->toggle_rates[i].interval;
> 
> > +       else
> 
> Redundant.
> 
> > +               return 0;
> > +}

Shrunk down to "for (...) if (...) return ...;" in v3.


> 
> > +               interval = 500;
> 
> interval_ms

Good suggestion, thanks. Don't need those comments in the code then.


> 
> > +       u32 addr[2];
> > +       int err;
> > +
> 
> > +       if (!fwnode_property_count_u32(fwnode, "reg"))
> 
> err = fwnode_property_count_u32(...);
> if (err < 0)
>   return err;
> if (err == 0)
>   return -ENODEV;
> 
> > +               return -ENODEV;
> > +
> > +       err = fwnode_property_read_u32_array(fwnode, "reg", addr,
> > ARRAY_SIZE(addr));
> 
> If count returns 1? What's the point of counting if you always want two?

If count returns 1, or more than 2, that's an error. But this check was missing
in v2, so I added it in v3.


> 
> > +       if (!device_property_match_string(dev, "realtek,led-scan-mode",
> > "single-color")) {
> 
> It seems that device_property_match_string() and accompanying
> functions have wrong description of returned codes, i.e. it returns
> the index of the matched string. It's possible that some APIs are
> broken (but I believe that the former is the case).
> 
> That said, I think the proper comparison should be >= 0.

Thanks, fixed.


Best,
Sander
Sander Vanheule May 23, 2021, 10:33 p.m. UTC | #11
The RTL8231 GPIO and LED expander can be configured for use as an MDIO or SMI
bus device. Currently only the MDIO mode is supported, although SMI mode
support should be fairly straightforward, once an SMI bus driver is available.

Provided features by the RTL8231:
  - Up to 37 GPIOs
    - Configurable drive strength: 8mA or 4mA (currently unsupported)
    - Input debouncing on high GPIOs (currently unsupported)
  - Up to 88 LEDs in multiple scan matrix groups
    - On, off, or one of six toggling intervals
    - "single-color mode": 2×36 single color LEDs + 8 bi-color LEDs
    - "bi-color mode": (12 + 2×6) bi-color LEDs + 24 single color LEDs
  - Up to one PWM output (currently unsupported)
    - Fixed duty cycle, 8 selectable frequencies (1.2kHz - 4.8kHz)

Register access is provided through a new MDIO regmap provider. The GPIO
controller uses gpio-regmap, although a patch is required to support a
limitation of the chip.

Changes since v2:
  - MDIO regmap support was merged, so patch is dropped here
  - Implement feedback for DT bindings
  - Use correct module names in Kconfigs
  - Fix k*alloc return value checks
  - Introduce GPIO regmap quirks to set output direction first
  - pinctrl: Use static pin descriptions for pin controller
  - pinctrl: Fix gpio consumer resource leak
  - mfd: Replace CONFIG_PM-ifdef'ery
  - leds: Rename interval to interval_ms

Changes since v1:
  - Reintroduce MDIO regmap, with fixed Kconfig dependencies
  - Add configurable dir/value order for gpio-regmap direction_out call
  - Drop allocations for regmap fields that are used only on init
  - Move some definitions to MFD header
  - Add PM ops to replace driver remove for MFD
  - Change pinctrl driver to (modified) gpio-regmap
  - Change leds driver to use fwnode

Changes since RFC:
  - Dropped MDIO regmap interface. I was unable to resolve the Kconfig
    dependency issue, so have reverted to using regmap_config.reg_read/write.
  - Added pinctrl support
  - Added LED support
  - Changed root device to MFD, with pinctrl and leds child devices. Root
    device is now an mdio_device driver.

Sander Vanheule (6):
  gpio: regmap: Add quirk for output data register
  dt-bindings: leds: Binding for RTL8231 scan matrix
  dt-bindings: mfd: Binding for RTL8231
  mfd: Add RTL8231 core device
  pinctrl: Add RTL8231 pin control and GPIO support
  leds: Add support for RTL8231 LED scan matrix

 .../bindings/leds/realtek,rtl8231-leds.yaml   | 166 ++++++++
 .../bindings/mfd/realtek,rtl8231.yaml         | 190 +++++++++
 drivers/gpio/gpio-regmap.c                    |  15 +-
 drivers/leds/Kconfig                          |  10 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-rtl8231.c                   | 291 +++++++++++++
 drivers/mfd/Kconfig                           |   9 +
 drivers/mfd/Makefile                          |   1 +
 drivers/mfd/rtl8231.c                         | 143 +++++++
 drivers/pinctrl/Kconfig                       |  11 +
 drivers/pinctrl/Makefile                      |   1 +
 drivers/pinctrl/pinctrl-rtl8231.c             | 398 ++++++++++++++++++
 include/linux/gpio/regmap.h                   |  13 +
 include/linux/mfd/rtl8231.h                   |  57 +++
 14 files changed, 1304 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/leds/realtek,rtl8231-leds.yaml
 create mode 100644 Documentation/devicetree/bindings/mfd/realtek,rtl8231.yaml
 create mode 100644 drivers/leds/leds-rtl8231.c
 create mode 100644 drivers/mfd/rtl8231.c
 create mode 100644 drivers/pinctrl/pinctrl-rtl8231.c
 create mode 100644 include/linux/mfd/rtl8231.h
Andrew Lunn May 24, 2021, 1:10 a.m. UTC | #12
> 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.

>   - Introduce GPIO regmap quirks to set output direction first


I thought you had determined it was possible to set output before
direction?

	Andrew
Andy Shevchenko May 24, 2021, 7:49 a.m. UTC | #13
On Mon, May 24, 2021 at 12:28 AM Sander Vanheule <sander@svanheule.net> wrote:
> On Tue, 2021-05-18 at 00:18 +0300, Andy Shevchenko wrote:

> > On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@svanheule.net> wrote:

> > >

> > > The RTL8231 is implemented as an MDIO device, and provides a regmap

> > > interface for register access by the core and child devices.

> > >

> > > The chip can also be a device on an SMI bus, an I2C-like bus by Realtek.

> > > Since kernel support for SMI is limited, and no real-world SMI

> > > implementations have been encountered for this device, this is currently

> > > unimplemented. The use of the regmap interface should make any future

> > > support relatively straightforward.

> > >

> > > After reset, all pins are muxed to GPIO inputs before the pin drivers

> > > are enabled. This is done to prevent accidental system resets, when a

> > > pin is connected to the parent SoC's reset line.

> >

> > > [missing MDIO_BUS dependency, provided via REGMAP_MDIO]

> > > Reported-by: kernel test robot <lkp@intel.com>

> >

> > What is the culprit? Shouldn't this have a Fixes tag?

>

> But it doesn't actually fix an issue created by an existing commit, just

> something that was wrong in the first version of the patch.


Then why is it in the tag block?
If you want to give a credit to LKP, do it in the comments block
(after '---' cutter line).

>  This patch is not

> dedicated to fixing that single issue though, it's just a part of it. Hence the

> note above the Reported-by tag.


--
With Best Regards,
Andy Shevchenko
Sander Vanheule May 24, 2021, 7:50 a.m. UTC | #14
Hi Andy,

I forgot to address one of your questions, so here's a short follow up.

On Tue, 2021-05-18 at 00:18 +0300, Andy Shevchenko wrote:
> On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@svanheule.net> wrote:

> > +       err = regmap_read(map, RTL8231_REG_FUNC1, &v);

> 

> > +       ready_code = FIELD_GET(RTL8231_FUNC1_READY_CODE_MASK, v);

> 

> If we got an error why we need a read_core, what for?


The chip has a static 5-bit field in register 0x01, called READY_CODE according
to the datasheet. If a device is present, and a read from register 0x01
succeeds, I still check that this field has the correct value. For the RTL8231,
it should return 0x37. If this isn't the case, I assume this isn't an RTL8231,
so the driver probe stops and returns an error value.

Best,
Sander
Andy Shevchenko May 24, 2021, 7:53 a.m. UTC | #15
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.

> >   - 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.
Andy Shevchenko May 24, 2021, 7:55 a.m. UTC | #16
On Mon, May 24, 2021 at 10:50 AM Sander Vanheule <sander@svanheule.net> wrote:
> On Tue, 2021-05-18 at 00:18 +0300, Andy Shevchenko wrote:

> > On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@svanheule.net> wrote:

> > > +       err = regmap_read(map, RTL8231_REG_FUNC1, &v);

> >

> > > +       ready_code = FIELD_GET(RTL8231_FUNC1_READY_CODE_MASK, v);

> >

> > If we got an error why we need a read_core, what for?

>

> The chip has a static 5-bit field in register 0x01, called READY_CODE according

> to the datasheet. If a device is present, and a read from register 0x01

> succeeds, I still check that this field has the correct value. For the RTL8231,

> it should return 0x37. If this isn't the case, I assume this isn't an RTL8231,

> so the driver probe stops and returns an error value.


Right. And why do you get ready_code if you know that there is an error?

-- 
With Best Regards,
Andy Shevchenko
Sander Vanheule May 24, 2021, 8:04 a.m. UTC | #17
On Mon, 2021-05-24 at 10:55 +0300, Andy Shevchenko wrote:
> On Mon, May 24, 2021 at 10:50 AM Sander Vanheule <sander@svanheule.net> wrote:
> > On Tue, 2021-05-18 at 00:18 +0300, Andy Shevchenko wrote:
> > > On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@svanheule.net>
> > > wrote:
> > > > +       err = regmap_read(map, RTL8231_REG_FUNC1, &v);
> > > 
> > > > +       ready_code = FIELD_GET(RTL8231_FUNC1_READY_CODE_MASK, v);
> > > 
> > > If we got an error why we need a read_core, what for?
> > 
> > The chip has a static 5-bit field in register 0x01, called READY_CODE
> > according
> > to the datasheet. If a device is present, and a read from register 0x01
> > succeeds, I still check that this field has the correct value. For the
> > RTL8231,
> > it should return 0x37. If this isn't the case, I assume this isn't an
> > RTL8231,
> > so the driver probe stops and returns an error value.
> 
> Right. And why do you get ready_code if you know that there is an error?

This has changed in v3. I now check if there was an error reading the register,
and return if there was. Only if there wasn't an error, the code continues to
extract and verify the READY_CODE value.

Best,
Sander
Sander Vanheule May 24, 2021, 11:39 a.m. UTC | #18
On Mon, 2021-05-24 at 14:32 +0300, Andy Shevchenko wrote:
> Oops, I had sent this privately, Cc'ing to ML.

I'll repeat my replies here then.

> 
> On Mon, May 24, 2021 at 12:08 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > 
> > On Mon, May 24, 2021 at 1:34 AM Sander Vanheule <sander@svanheule.net> wrote:
> > > 
> > > This driver implements the GPIO and pin muxing features provided by the
> > > RTL8231. The device should be instantiated as an MFD child, where the
> > > parent device has already configured the regmap used for register
> > > access.
> > > 
> > > Although described in the bindings, pin debouncing and drive strength
> > > selection are currently not implemented. Debouncing is only available
> > > for the six highest GPIOs, and must be emulated when other pins are used
> > > for (button) inputs anyway.
> > 
> > ...
> > 
> > > +struct rtl8231_function {
> > > +       const char *name;
> > > +       unsigned int ngroups;
> > > +       const char **groups;
> > 
> > const char * const * groups?
> > (Double check this, because I don't know if it's really const in your case)
> > 

I had to rework rtl8231_pinctrl_init_functions a bit, but outside of that
function this string array is indeed constant.


> > > +};
> > 
> > ...
> > 
> > > +       const struct rtl8231_pin_desc *desc =
> > > +               (struct rtl8231_pin_desc *)
> > > &rtl8231_pins[group_selector].drv_data;
> > 
> > Casting from/to void * is redundant in C.
> > 
> > ...
> > 
> > > +       struct rtl8231_pin_desc *desc =
> > > +               (struct rtl8231_pin_desc *) &rtl8231_pins[offset].drv_data;
> > 
> > Ditto.

Ok, changed.


> > 
> > ...
> > 
> > > +       ctrl->nfunctions = ARRAY_SIZE(rtl8231_pin_function_names);
> > > +       ctrl->functions = devm_kcalloc(dev, ctrl->nfunctions, sizeof(*ctrl-
> > > >functions), GFP_KERNEL);
> > > +       if (!ctrl->functions) {
> > 
> > > +               dev_err(dev, "failed to allocate pin function
> > > descriptors\n");
> > 
> > Dtop this noisy message, user space will print the similar one.
> > 
> > > +               return -ENOMEM;
> > > +       }
> > 
> > ...
> > 
> > > +       ctrl->map = dev_get_regmap(dev->parent, NULL);
> > > +       if (!ctrl->map)
> > > +               return -ENODEV;
> > > +
> > > +       if (IS_ERR(ctrl->map))
> > > +               return PTR_ERR(ctrl->map);
> > 
> > Hmm... Is it really the case that you have to check for different values?
> > What does NULL mean? Optional?
> > 

Checked the documentation again, and this actually doesn't return error values.
Only valid pointers or NULL. Will change accordingly here, and also in the LED
driver.


> > ...
> > 
> > > +       gr = devm_gpio_regmap_register(dev, &gpio_cfg);
> > > +       if (IS_ERR(gr)) {
> > 
> > > +               dev_err(dev, "failed to register gpio controller\n");
> > > +               return PTR_ERR(gr);
> > 
> > Is it possible to get a deferred probe here? If so, use dev_err_probe()
> > 

gpiochip_add_data_with_key can indeed return EPROBE_DEFER (when gpiolib isn't
loaded yet, if I understand correctly). I'll replace dev_err by dev_err_probe.


Best,
Sander
Sander Vanheule May 24, 2021, 11:41 a.m. UTC | #19
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?

I was trying to reproduce this behaviour on the Zyxel, but using the strapping
pins that are also used to configure the device's address. So perhaps the pull-
ups/-downs were confusing the results. Using a separate pin on the Zyxel's
RTL8231, I've now been able to confirm the same behaviour as on the Netgear,
including capturing the resulting glitch (with my simple logic analyser) when
enabling the quirk in the first test case.

I hope this explains why I've still included the quirk in this revision. If not,
please let me know what isn't clear.


Best,
Sander
Andy Shevchenko May 24, 2021, 12:54 p.m. UTC | #20
On Mon, May 24, 2021 at 2:41 PM Sander Vanheule <sander@svanheule.net> wrote:
> 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


Also works but you have to provide this information in the cover letter.

...

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


GPIO tools for the shell are context-less. Can you reproduce this with
the legacy sysfs interface?

> I was trying to reproduce this behaviour on the Zyxel, but using the strapping

> pins that are also used to configure the device's address. So perhaps the pull-

> ups/-downs were confusing the results. Using a separate pin on the Zyxel's

> RTL8231, I've now been able to confirm the same behaviour as on the Netgear,

> including capturing the resulting glitch (with my simple logic analyser) when

> enabling the quirk in the first test case.

>

> I hope this explains why I've still included the quirk in this revision. If not,

> please let me know what isn't clear.


Do you possess a schematic of either of the devices and a link to the
RTL datasheet (Btw, if it's publicly available, or you have a link
that will ask for necessary sign-in it would be nice to include the
link to it as a Datasheet: tag)?

-- 
With Best Regards,
Andy Shevchenko
Sander Vanheule May 24, 2021, 3:03 p.m. UTC | #21
On Mon, 2021-05-24 at 15:54 +0300, Andy Shevchenko wrote:
> On Mon, May 24, 2021 at 2:41 PM Sander Vanheule <sander@svanheule.net> wrote:

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

> 

> Also works but you have to provide this information in the cover letter.

> 


Ok, I will add the link to the cover letter for the next version. Does it need
to be in a Link-tag, or can just be a reference?


> ...

> 

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

> 

> GPIO tools for the shell are context-less. Can you reproduce this with

> the legacy sysfs interface?

> 

> > I was trying to reproduce this behaviour on the Zyxel, but using the

> > strapping

> > pins that are also used to configure the device's address. So perhaps the

> > pull-

> > ups/-downs were confusing the results. Using a separate pin on the Zyxel's

> > RTL8231, I've now been able to confirm the same behaviour as on the Netgear,

> > including capturing the resulting glitch (with my simple logic analyser)

> > when

> > enabling the quirk in the first test case.

> > 

> > I hope this explains why I've still included the quirk in this revision. If

> > not,

> > please let me know what isn't clear.

> 

> Do you possess a schematic of either of the devices and a link to the

> RTL datasheet (Btw, if it's publicly available, or you have a link

> that will ask for necessary sign-in it would be nice to include the

> link to it as a Datasheet: tag)?


Sadly, I don't. Most of the info we have comes from code archives of switch
vendors (Zyxel, Cisco etc). Boards need to be reverse engineered, and the few
leaked datasheets that can be found on the internet aren't exactly thick in
information.

The RTL8231 datasheet is actually quite useful, but makes no mention of the
output value isse. Since this isn't an official resource, I don't think it would
be appropriate to link it via a Datasheet: tag.
https://github.com/libc0607/Realtek_switch_hacking/blob/files/RTL8231_Datasheet_
1.2.pdf

Looking at the datasheet again, I came up with a... terrible hack to work around
the output value issue.

The chip also has GPIO_INVERT registers that I hadn't used until now, because
the logical inversion is handled in the kernel. However, these inversion
registers only apply to the output values. So, I could implement glitch-free
output behaviour in the following way:
 * After chip reset, and before enabling the output driver (MFD initialisation):
    - Mux all pins as GPIO
    - Change all pins to outputs, so the data registers (0x1c-0x1e) become writable
    - Write value 0 to all pins
    - Change all pins to GPI to change them into high-Z
 * In the pinctrl/gpio driver:
    - Use data registers as input-only
    - Use inversion register to determine output value (can be written any time)

The above gives glitch-free outputs, but the values that are read back (when
configured as output), come from the data registers. They should now be coming
from the inversion (reg_set_base) registers, but the code prefers to use the
data registers (reg_dat_base).


Best,
Sander
Sander Vanheule May 24, 2021, 3:20 p.m. UTC | #22
Hi Andy,

Forgot to reply to the sysfs suggestion.

On Mon, 2021-05-24 at 15:54 +0300, Andy Shevchenko wrote:
> On Mon, May 24, 2021 at 2:41 PM Sander Vanheule <sander@svanheule.net> wrote:

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

> 

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

> 

> GPIO tools for the shell are context-less. Can you reproduce this with

> the legacy sysfs interface?


Using the sysfs interface produced the same behaviour for both test cases.

E.g. case 1:
   # Set to output low
   echo out > direction; echo 0 > value
   # Change to input (with weak pull-up)
   echo in > direction
   # Try to set to output high
   # Fails to go high if the pin value is set before the direction
   echo high > direction


Best,
Sander
Andy Shevchenko May 24, 2021, 4:30 p.m. UTC | #23
On Mon, May 24, 2021 at 6:03 PM Sander Vanheule <sander@svanheule.net> wrote:
> On Mon, 2021-05-24 at 15:54 +0300, Andy Shevchenko wrote:

> > On Mon, May 24, 2021 at 2:41 PM Sander Vanheule <sander@svanheule.net> wrote:

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


...

> Ok, I will add the link to the cover letter for the next version. Does it need

> to be in a Link-tag, or can just be a reference?


Some kind of reference, no need to have a special tag in the cover letter.


...

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

> >

> > GPIO tools for the shell are context-less. Can you reproduce this with

> > the legacy sysfs interface?

> >

> > > I was trying to reproduce this behaviour on the Zyxel, but using the

> > > strapping

> > > pins that are also used to configure the device's address. So perhaps the

> > > pull-

> > > ups/-downs were confusing the results. Using a separate pin on the Zyxel's

> > > RTL8231, I've now been able to confirm the same behaviour as on the Netgear,

> > > including capturing the resulting glitch (with my simple logic analyser)

> > > when

> > > enabling the quirk in the first test case.

> > >

> > > I hope this explains why I've still included the quirk in this revision. If

> > > not,

> > > please let me know what isn't clear.

> >

> > Do you possess a schematic of either of the devices and a link to the

> > RTL datasheet (Btw, if it's publicly available, or you have a link

> > that will ask for necessary sign-in it would be nice to include the

> > link to it as a Datasheet: tag)?

>

> Sadly, I don't. Most of the info we have comes from code archives of switch

> vendors (Zyxel, Cisco etc). Boards need to be reverse engineered, and the few

> leaked datasheets that can be found on the internet aren't exactly thick in

> information.

>

> The RTL8231 datasheet is actually quite useful, but makes no mention of the

> output value isse. Since this isn't an official resource, I don't think it would

> be appropriate to link it via a Datasheet: tag.

> https://github.com/libc0607/Realtek_switch_hacking/blob/files/RTL8231_Datasheet_

> 1.2.pdf

>

> Looking at the datasheet again, I came up with a... terrible hack to work around

> the output value issue.

>

> The chip also has GPIO_INVERT registers that I hadn't used until now, because

> the logical inversion is handled in the kernel. However, these inversion

> registers only apply to the output values. So, I could implement glitch-free

> output behaviour in the following way:

>  * After chip reset, and before enabling the output driver (MFD initialisation):

>     - Mux all pins as GPIO

>     - Change all pins to outputs,


No. no, no. This is much worse than the glitches. You never know what
the hardware is connected there and it's potential breakage (on hw
level) possible.

>  so the data registers (0x1c-0x1e) become writable

>     - Write value 0 to all pins

>     - Change all pins to GPI to change them into high-Z

>  * In the pinctrl/gpio driver:

>     - Use data registers as input-only

>     - Use inversion register to determine output value (can be written any time)

>

> The above gives glitch-free outputs, but the values that are read back (when

> configured as output), come from the data registers. They should now be coming

> from the inversion (reg_set_base) registers, but the code prefers to use the

> data registers (reg_dat_base).


Lemme read the datasheet and see if I find any clue for the hw behaviour.

--
With Best Regards,
Andy Shevchenko
Andy Shevchenko May 25, 2021, 5:11 p.m. UTC | #24
On Mon, May 24, 2021 at 7:30 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, May 24, 2021 at 6:03 PM Sander Vanheule <sander@svanheule.net> wrote:

> > On Mon, 2021-05-24 at 15:54 +0300, Andy Shevchenko wrote:


...

> > Sadly, I don't. Most of the info we have comes from code archives of switch

> > vendors (Zyxel, Cisco etc). Boards need to be reverse engineered, and the few

> > leaked datasheets that can be found on the internet aren't exactly thick in

> > information.

> >

> > The RTL8231 datasheet is actually quite useful, but makes no mention of the

> > output value isse. Since this isn't an official resource, I don't think it would

> > be appropriate to link it via a Datasheet: tag.

> > https://github.com/libc0607/Realtek_switch_hacking/blob/files/RTL8231_Datasheet_

> > 1.2.pdf

> >

> > Looking at the datasheet again, I came up with a... terrible hack to work around

> > the output value issue.

> >

> > The chip also has GPIO_INVERT registers that I hadn't used until now, because

> > the logical inversion is handled in the kernel. However, these inversion

> > registers only apply to the output values. So, I could implement glitch-free

> > output behaviour in the following way:

> >  * After chip reset, and before enabling the output driver (MFD initialisation):

> >     - Mux all pins as GPIO

> >     - Change all pins to outputs,

>

> No. no, no. This is much worse than the glitches. You never know what

> the hardware is connected there and it's potential breakage (on hw

> level) possible.

>

> >  so the data registers (0x1c-0x1e) become writable

> >     - Write value 0 to all pins

> >     - Change all pins to GPI to change them into high-Z

> >  * In the pinctrl/gpio driver:

> >     - Use data registers as input-only

> >     - Use inversion register to determine output value (can be written any time)

> >

> > The above gives glitch-free outputs, but the values that are read back (when

> > configured as output), come from the data registers. They should now be coming

> > from the inversion (reg_set_base) registers, but the code prefers to use the

> > data registers (reg_dat_base).

>

> Lemme read the datasheet and see if I find any clue for the hw behaviour.


Thank you for your patience!

Have you explored the possibility of using En_Sync_GPIO?

-- 
With Best Regards,
Andy Shevchenko
Sander Vanheule May 25, 2021, 6 p.m. UTC | #25
On Tue, 2021-05-25 at 20:11 +0300, Andy Shevchenko wrote:
> On Mon, May 24, 2021 at 7:30 PM Andy Shevchenko

> <andy.shevchenko@gmail.com> wrote:

> > On Mon, May 24, 2021 at 6:03 PM Sander Vanheule <sander@svanheule.net>

> > wrote:

> > > On Mon, 2021-05-24 at 15:54 +0300, Andy Shevchenko wrote:

> 

> ...

> 

> > > Sadly, I don't. Most of the info we have comes from code archives of

> > > switch

> > > vendors (Zyxel, Cisco etc). Boards need to be reverse engineered, and the

> > > few

> > > leaked datasheets that can be found on the internet aren't exactly thick

> > > in

> > > information.

> > > 

> > > The RTL8231 datasheet is actually quite useful, but makes no mention of

> > > the

> > > output value isse. Since this isn't an official resource, I don't think it

> > > would

> > > be appropriate to link it via a Datasheet: tag.

> > > https://github.com/libc0607/Realtek_switch_hacking/blob/files/RTL8231_Datasheet_

> > > 1.2.pdf

> > > 

> > > Looking at the datasheet again, I came up with a... terrible hack to work

> > > around

> > > the output value issue.

> > > 

> > > The chip also has GPIO_INVERT registers that I hadn't used until now,

> > > because

> > > the logical inversion is handled in the kernel. However, these inversion

> > > registers only apply to the output values. So, I could implement glitch-

> > > free

> > > output behaviour in the following way:

> > >  * After chip reset, and before enabling the output driver (MFD

> > > initialisation):

> > >     - Mux all pins as GPIO

> > >     - Change all pins to outputs,

> > 

> > No. no, no. This is much worse than the glitches. You never know what

> > the hardware is connected there and it's potential breakage (on hw

> > level) possible.

> > 

> > >  so the data registers (0x1c-0x1e) become writable

> > >     - Write value 0 to all pins

> > >     - Change all pins to GPI to change them into high-Z

> > >  * In the pinctrl/gpio driver:

> > >     - Use data registers as input-only

> > >     - Use inversion register to determine output value (can be written any

> > > time)

> > > 

> > > The above gives glitch-free outputs, but the values that are read back

> > > (when

> > > configured as output), come from the data registers. They should now be

> > > coming

> > > from the inversion (reg_set_base) registers, but the code prefers to use

> > > the

> > > data registers (reg_dat_base).

> > 

> > Lemme read the datasheet and see if I find any clue for the hw behaviour.

> 

> Thank you for your patience!

> 

> Have you explored the possibility of using En_Sync_GPIO?


I haven't (output latching doesn't really appear to be a thing in the gpio
framework?), but I did notice that the main SoC's RTL8231 integration uses it.

Let me play around with it to see if it also latches the pin direction, or if
that's always an immediate change.


Best,
Sander
Sander Vanheule May 26, 2021, 9:02 p.m. UTC | #26
On Tue, 2021-05-25 at 20:11 +0300, Andy Shevchenko wrote:
> On Mon, May 24, 2021 at 7:30 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Mon, May 24, 2021 at 6:03 PM Sander Vanheule <sander@svanheule.net>
> > wrote:
> > > On Mon, 2021-05-24 at 15:54 +0300, Andy Shevchenko wrote:
> 
> ...
> 
> > > Sadly, I don't. Most of the info we have comes from code archives of
> > > switch
> > > vendors (Zyxel, Cisco etc). Boards need to be reverse engineered, and the
> > > few
> > > leaked datasheets that can be found on the internet aren't exactly thick
> > > in
> > > information.
> > > 
> > > The RTL8231 datasheet is actually quite useful, but makes no mention of
> > > the
> > > output value isse. Since this isn't an official resource, I don't think it
> > > would
> > > be appropriate to link it via a Datasheet: tag.
> > > https://github.com/libc0607/Realtek_switch_hacking/blob/files/RTL8231_Datasheet_
> > > 1.2.pdf
> > > 
> > > Looking at the datasheet again, I came up with a... terrible hack to work
> > > around
> > > the output value issue.
> > > 
> > > The chip also has GPIO_INVERT registers that I hadn't used until now,
> > > because
> > > the logical inversion is handled in the kernel. However, these inversion
> > > registers only apply to the output values. So, I could implement glitch-
> > > free
> > > output behaviour in the following way:
> > >  * After chip reset, and before enabling the output driver (MFD
> > > initialisation):
> > >     - Mux all pins as GPIO
> > >     - Change all pins to outputs,
> > 
> > No. no, no. This is much worse than the glitches. You never know what
> > the hardware is connected there and it's potential breakage (on hw
> > level) possible.
> > 
> > >  so the data registers (0x1c-0x1e) become writable
> > >     - Write value 0 to all pins
> > >     - Change all pins to GPI to change them into high-Z
> > >  * In the pinctrl/gpio driver:
> > >     - Use data registers as input-only
> > >     - Use inversion register to determine output value (can be written any
> > > time)
> > > 
> > > The above gives glitch-free outputs, but the values that are read back
> > > (when
> > > configured as output), come from the data registers. They should now be
> > > coming
> > > from the inversion (reg_set_base) registers, but the code prefers to use
> > > the
> > > data registers (reg_dat_base).
> > 
> > Lemme read the datasheet and see if I find any clue for the hw behaviour.
> 
> Thank you for your patience!
> 
> Have you explored the possibility of using En_Sync_GPIO?

Got around to testing things.

If En_Sync_GPIO is enabled, it's still possible to change the pin direction
without also writing the Sync_GPIO bit. So even with the latching, glitches are
still produced.

As long as Sync_GPIO is not set to latch the new values, it also appears that
reads of the data registers result in the current output value, not the new one.

As a different test, I've added a pull-down, to make the input level low. Now I
see the opposite behaviour as before (with set-value-before-direction):
 * OUT-HIGH > IN (low) > OUT-LOW: results in a high level (i.e. old value)
 * OUT-HIGH > IN (low) > OUT-HIGH: results in a high level (new/old value)
 * OUT-LOW > IN (low) > OUT-HIGH: results in a high level (new value, or toggled
   old value?)
 * OUT-LOW > IN (low) > OUT-LOW: results in a low level (new/old value)

For reference, with a pull-up:
 * OUT-HIGH > IN (high) > OUT-HIGH: high result
 * OUT-HIGH > IN (high) > OUT-LOW: low result
 * OUT-LOW > IN (high) > OUT-HIGH: low result
 * OUT-LOW > IN (high) > OUT-LOW: low result

I've only tested this with the sysfs interface, so I don't know what the result
would be on multiple writes to the data register (during input, but probably not
very relevant). Nor have I tested direction changes if the input has changed
between two output values.

I may have some time tomorrow for more testing, but otherwise it'll have to wait
until the weekend. Any other ideas in the meantime?


Best,
Sander
Andy Shevchenko May 27, 2021, 10:38 a.m. UTC | #27
+Cc: Hans

Hans, sorry for disturbing you later too much. Here we have "nice"
hardware which can't be used in a glitch-free mode (somehow it reminds
me lynxpoint, baytrail, cherryview designs). If you have any ideas to
share (no need to dive deep or look at it if you have no time), you're
welcome.

On Thu, May 27, 2021 at 12:02 AM Sander Vanheule <sander@svanheule.net> wrote:
>

> On Tue, 2021-05-25 at 20:11 +0300, Andy Shevchenko wrote:

> > On Mon, May 24, 2021 at 7:30 PM Andy Shevchenko

> > <andy.shevchenko@gmail.com> wrote:

> > > On Mon, May 24, 2021 at 6:03 PM Sander Vanheule <sander@svanheule.net>

> > > wrote:

> > > > On Mon, 2021-05-24 at 15:54 +0300, Andy Shevchenko wrote:

> >

> > ...

> >

> > > > Sadly, I don't. Most of the info we have comes from code archives of

> > > > switch

> > > > vendors (Zyxel, Cisco etc). Boards need to be reverse engineered, and the

> > > > few

> > > > leaked datasheets that can be found on the internet aren't exactly thick

> > > > in

> > > > information.

> > > >

> > > > The RTL8231 datasheet is actually quite useful, but makes no mention of

> > > > the

> > > > output value isse. Since this isn't an official resource, I don't think it

> > > > would

> > > > be appropriate to link it via a Datasheet: tag.

> > > > https://github.com/libc0607/Realtek_switch_hacking/blob/files/RTL8231_Datasheet_

> > > > 1.2.pdf

> > > >

> > > > Looking at the datasheet again, I came up with a... terrible hack to work

> > > > around

> > > > the output value issue.

> > > >

> > > > The chip also has GPIO_INVERT registers that I hadn't used until now,

> > > > because

> > > > the logical inversion is handled in the kernel. However, these inversion

> > > > registers only apply to the output values. So, I could implement glitch-

> > > > free

> > > > output behaviour in the following way:

> > > >  * After chip reset, and before enabling the output driver (MFD

> > > > initialisation):

> > > >     - Mux all pins as GPIO

> > > >     - Change all pins to outputs,

> > >

> > > No. no, no. This is much worse than the glitches. You never know what

> > > the hardware is connected there and it's potential breakage (on hw

> > > level) possible.

> > >

> > > >  so the data registers (0x1c-0x1e) become writable

> > > >     - Write value 0 to all pins

> > > >     - Change all pins to GPI to change them into high-Z

> > > >  * In the pinctrl/gpio driver:

> > > >     - Use data registers as input-only

> > > >     - Use inversion register to determine output value (can be written any

> > > > time)

> > > >

> > > > The above gives glitch-free outputs, but the values that are read back

> > > > (when

> > > > configured as output), come from the data registers. They should now be

> > > > coming

> > > > from the inversion (reg_set_base) registers, but the code prefers to use

> > > > the

> > > > data registers (reg_dat_base).

> > >

> > > Lemme read the datasheet and see if I find any clue for the hw behaviour.

> >

> > Thank you for your patience!

> >

> > Have you explored the possibility of using En_Sync_GPIO?

>

> Got around to testing things.

>

> If En_Sync_GPIO is enabled, it's still possible to change the pin direction

> without also writing the Sync_GPIO bit. So even with the latching, glitches are

> still produced.

>

> As long as Sync_GPIO is not set to latch the new values, it also appears that

> reads of the data registers result in the current output value, not the new one.

>

> As a different test, I've added a pull-down, to make the input level low. Now I

> see the opposite behaviour as before (with set-value-before-direction):

>  * OUT-HIGH > IN (low) > OUT-LOW: results in a high level (i.e. old value)

>  * OUT-HIGH > IN (low) > OUT-HIGH: results in a high level (new/old value)

>  * OUT-LOW > IN (low) > OUT-HIGH: results in a high level (new value, or toggled

>    old value?)

>  * OUT-LOW > IN (low) > OUT-LOW: results in a low level (new/old value)

>

> For reference, with a pull-up:

>  * OUT-HIGH > IN (high) > OUT-HIGH: high result

>  * OUT-HIGH > IN (high) > OUT-LOW: low result

>  * OUT-LOW > IN (high) > OUT-HIGH: low result

>  * OUT-LOW > IN (high) > OUT-LOW: low result

>

> I've only tested this with the sysfs interface, so I don't know what the result

> would be on multiple writes to the data register (during input, but probably not

> very relevant). Nor have I tested direction changes if the input has changed

> between two output values.

>

> I may have some time tomorrow for more testing, but otherwise it'll have to wait

> until the weekend. Any other ideas in the meantime?


No ideas so far. In x86 we used to have something similar (baytrail,
cherryview, lynxpoint), but it's firmware assisted. I think that this
hardware (realtek) is supposed either
- to be firmware / bootloader assisted, so in a way that platform is
preconfigured when Linux starts and any GPIO request won't be harmful
as long as it doesn't change direction on the pins (which is usually
guaranteed by DT and corresponding drivers to do the correct things)
- be used for glitch-tolerant hardware (LEDs, for example, where
nobody usually will noticed 1ms blink)

That said, I have not been convinced we have to quirk gpio-regmap for
this one. Just describe the issues with hardware in the accompanying
documentation.

But if maintainers or somebody comes with a better / different
approach I am all ears.

-- 
With Best Regards,
Andy Shevchenko
Hans de Goede May 27, 2021, 10:41 a.m. UTC | #28
Hi,

On 5/27/21 12:38 PM, Andy Shevchenko wrote:
> +Cc: Hans
> 
> Hans, sorry for disturbing you later too much. Here we have "nice"
> hardware which can't be used in a glitch-free mode (somehow it reminds
> me lynxpoint, baytrail, cherryview designs). If you have any ideas to
> share (no need to dive deep or look at it if you have no time), you're
> welcome.

I'm afraid I've no ideas how to solve this nicely. Documenting the
issue might be the best we can do.

Regards,

Hans



> 
> On Thu, May 27, 2021 at 12:02 AM Sander Vanheule <sander@svanheule.net> wrote:
>>
>> On Tue, 2021-05-25 at 20:11 +0300, Andy Shevchenko wrote:
>>> On Mon, May 24, 2021 at 7:30 PM Andy Shevchenko
>>> <andy.shevchenko@gmail.com> wrote:
>>>> On Mon, May 24, 2021 at 6:03 PM Sander Vanheule <sander@svanheule.net>
>>>> wrote:
>>>>> On Mon, 2021-05-24 at 15:54 +0300, Andy Shevchenko wrote:
>>>
>>> ...
>>>
>>>>> Sadly, I don't. Most of the info we have comes from code archives of
>>>>> switch
>>>>> vendors (Zyxel, Cisco etc). Boards need to be reverse engineered, and the
>>>>> few
>>>>> leaked datasheets that can be found on the internet aren't exactly thick
>>>>> in
>>>>> information.
>>>>>
>>>>> The RTL8231 datasheet is actually quite useful, but makes no mention of
>>>>> the
>>>>> output value isse. Since this isn't an official resource, I don't think it
>>>>> would
>>>>> be appropriate to link it via a Datasheet: tag.
>>>>> https://github.com/libc0607/Realtek_switch_hacking/blob/files/RTL8231_Datasheet_
>>>>> 1.2.pdf
>>>>>
>>>>> Looking at the datasheet again, I came up with a... terrible hack to work
>>>>> around
>>>>> the output value issue.
>>>>>
>>>>> The chip also has GPIO_INVERT registers that I hadn't used until now,
>>>>> because
>>>>> the logical inversion is handled in the kernel. However, these inversion
>>>>> registers only apply to the output values. So, I could implement glitch-
>>>>> free
>>>>> output behaviour in the following way:
>>>>>  * After chip reset, and before enabling the output driver (MFD
>>>>> initialisation):
>>>>>     - Mux all pins as GPIO
>>>>>     - Change all pins to outputs,
>>>>
>>>> No. no, no. This is much worse than the glitches. You never know what
>>>> the hardware is connected there and it's potential breakage (on hw
>>>> level) possible.
>>>>
>>>>>  so the data registers (0x1c-0x1e) become writable
>>>>>     - Write value 0 to all pins
>>>>>     - Change all pins to GPI to change them into high-Z
>>>>>  * In the pinctrl/gpio driver:
>>>>>     - Use data registers as input-only
>>>>>     - Use inversion register to determine output value (can be written any
>>>>> time)
>>>>>
>>>>> The above gives glitch-free outputs, but the values that are read back
>>>>> (when
>>>>> configured as output), come from the data registers. They should now be
>>>>> coming
>>>>> from the inversion (reg_set_base) registers, but the code prefers to use
>>>>> the
>>>>> data registers (reg_dat_base).
>>>>
>>>> Lemme read the datasheet and see if I find any clue for the hw behaviour.
>>>
>>> Thank you for your patience!
>>>
>>> Have you explored the possibility of using En_Sync_GPIO?
>>
>> Got around to testing things.
>>
>> If En_Sync_GPIO is enabled, it's still possible to change the pin direction
>> without also writing the Sync_GPIO bit. So even with the latching, glitches are
>> still produced.
>>
>> As long as Sync_GPIO is not set to latch the new values, it also appears that
>> reads of the data registers result in the current output value, not the new one.
>>
>> As a different test, I've added a pull-down, to make the input level low. Now I
>> see the opposite behaviour as before (with set-value-before-direction):
>>  * OUT-HIGH > IN (low) > OUT-LOW: results in a high level (i.e. old value)
>>  * OUT-HIGH > IN (low) > OUT-HIGH: results in a high level (new/old value)
>>  * OUT-LOW > IN (low) > OUT-HIGH: results in a high level (new value, or toggled
>>    old value?)
>>  * OUT-LOW > IN (low) > OUT-LOW: results in a low level (new/old value)
>>
>> For reference, with a pull-up:
>>  * OUT-HIGH > IN (high) > OUT-HIGH: high result
>>  * OUT-HIGH > IN (high) > OUT-LOW: low result
>>  * OUT-LOW > IN (high) > OUT-HIGH: low result
>>  * OUT-LOW > IN (high) > OUT-LOW: low result
>>
>> I've only tested this with the sysfs interface, so I don't know what the result
>> would be on multiple writes to the data register (during input, but probably not
>> very relevant). Nor have I tested direction changes if the input has changed
>> between two output values.
>>
>> I may have some time tomorrow for more testing, but otherwise it'll have to wait
>> until the weekend. Any other ideas in the meantime?
> 
> No ideas so far. In x86 we used to have something similar (baytrail,
> cherryview, lynxpoint), but it's firmware assisted. I think that this
> hardware (realtek) is supposed either
> - to be firmware / bootloader assisted, so in a way that platform is
> preconfigured when Linux starts and any GPIO request won't be harmful
> as long as it doesn't change direction on the pins (which is usually
> guaranteed by DT and corresponding drivers to do the correct things)
> - be used for glitch-tolerant hardware (LEDs, for example, where
> nobody usually will noticed 1ms blink)
> 
> That said, I have not been convinced we have to quirk gpio-regmap for
> this one. Just describe the issues with hardware in the accompanying
> documentation.
> 
> But if maintainers or somebody comes with a better / different
> approach I am all ears.
>
Linus Walleij May 27, 2021, 11:31 p.m. UTC | #29
On Mon, May 24, 2021 at 12:34 AM Sander Vanheule <sander@svanheule.net> wrote:

> Add a binding description for the Realtek RTL8231, a GPIO and LED
> expander chip commonly used in ethernet switches based on a Realtek
> switch SoC. These chips can be addressed via an MDIO or SMI bus, or used
> as a plain 36-bit shift register.
>
> This binding only describes the feature set provided by the MDIO/SMI
> configuration, and covers the GPIO, PWM, and pin control properties. The
> LED properties are defined in a separate binding.
>
> Signed-off-by: Sander Vanheule <sander@svanheule.net>

This looks good to me from a GPIO and pin control PoV:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
Michael Walle May 28, 2021, 6:29 a.m. UTC | #30
> +	gpio_cfg.reg_dat_base = GPIO_REGMAP_ADDR(RTL8231_REG_GPIO_DATA0);
> +	gpio_cfg.reg_set_base = GPIO_REGMAP_ADDR(RTL8231_REG_GPIO_DATA0);
> +	gpio_cfg.reg_dir_in_base = GPIO_REGMAP_ADDR(RTL8231_REG_GPIO_DIR0);

Btw. you'd only need GPIO_REGMAP_ADDR(x) if x might be 0. Because you 
have
a constant != 0 there, you could save the GPIO_REGMAP_ADDR() call. You
could drop this if you like, but no need to respin the series for this.

-michael
Michael Walle May 28, 2021, 6:37 a.m. UTC | #31
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).

-michael
Michael Walle May 28, 2021, 6:40 a.m. UTC | #32
Am 2021-05-24 00:33, schrieb Sander Vanheule:
> GPIO chips may not support setting the output value when a pin is

> configured as an input, although the current implementation assumes 

> this

> is always possible.

> 

> Add support for setting pin direction before value. The order defaults

> to setting the value first, but this can be reversed by setting the

> GPIO_REGMAP_QUIRK_SET_DIRECTION_FIRST flag in regmap_config.quirks.


Nice! If this is really needed:

Reviewed-by: Michael Walle <michael@walle.cc>


-michael
Sander Vanheule May 28, 2021, 6:42 a.m. UTC | #33
Hi Michael,

On Fri, 2021-05-28 at 08:29 +0200, Michael Walle wrote:
> > +       gpio_cfg.reg_dat_base = GPIO_REGMAP_ADDR(RTL8231_REG_GPIO_DATA0);
> > +       gpio_cfg.reg_set_base = GPIO_REGMAP_ADDR(RTL8231_REG_GPIO_DATA0);
> > +       gpio_cfg.reg_dir_in_base = GPIO_REGMAP_ADDR(RTL8231_REG_GPIO_DIR0);
> 
> Btw. you'd only need GPIO_REGMAP_ADDR(x) if x might be 0. Because you 
> have
> a constant != 0 there, you could save the GPIO_REGMAP_ADDR() call. You
> could drop this if you like, but no need to respin the series for this.

I will need to respin this series anyway, so I can drop the GPIO_REGMAP_ADDR()
calls. I was aware they are no-ops in this case, as register address 0 is not
used for the GPIO functions, so mainly included them as a form of documentation.

Best,
Sander
Michael Walle May 28, 2021, 6:43 a.m. UTC | #34
Am 2021-05-28 08:42, schrieb Sander Vanheule:
> On Fri, 2021-05-28 at 08:29 +0200, Michael Walle wrote:

>> > +       gpio_cfg.reg_dat_base = GPIO_REGMAP_ADDR(RTL8231_REG_GPIO_DATA0);

>> > +       gpio_cfg.reg_set_base = GPIO_REGMAP_ADDR(RTL8231_REG_GPIO_DATA0);

>> > +       gpio_cfg.reg_dir_in_base = GPIO_REGMAP_ADDR(RTL8231_REG_GPIO_DIR0);

>> 

>> Btw. you'd only need GPIO_REGMAP_ADDR(x) if x might be 0. Because you

>> have

>> a constant != 0 there, you could save the GPIO_REGMAP_ADDR() call. You

>> could drop this if you like, but no need to respin the series for 

>> this.

> 

> I will need to respin this series anyway, so I can drop the 

> GPIO_REGMAP_ADDR()

> calls. I was aware they are no-ops in this case, as register address 0 

> is not

> used for the GPIO functions, so mainly included them as a form of 

> documentation.


It's up to you if you like to change it or keep it.

-michael
Sander Vanheule May 30, 2021, 4:19 p.m. UTC | #35
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
Hans de Goede May 30, 2021, 4:51 p.m. UTC | #36
Hi,

On 5/30/21 6:19 PM, Sander Vanheule wrote:
> 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?

Regmap allows you to mark certain ranges as volatile, so that they will not
be cached, these GPIO registers containing the current pin value seems like
a good candidate for this. This is also necessary to make reading the GPIO
work without getting back a stale, cached value.

Regards,

Hans
Andy Shevchenko May 30, 2021, 6:16 p.m. UTC | #37
On Sun, May 30, 2021 at 7:51 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 5/30/21 6:19 PM, Sander Vanheule wrote:

> > On Fri, 2021-05-28 at 08:37 +0200, Michael Walle wrote:


...

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

>

> Regmap allows you to mark certain ranges as volatile, so that they will not

> be cached, these GPIO registers containing the current pin value seems like

> a good candidate for this. This is also necessary to make reading the GPIO

> work without getting back a stale, cached value.


After all it seems a simple missed proper register configuration in
the driver for regmap.
Oh, as usual something easy-to-solve requires tons of time to find it. :-)

Sander, I think you may look at gpio-pca953x.c to understand how it
works (volatility of registers).

-- 
With Best Regards,
Andy Shevchenko
Michael Walle May 30, 2021, 9:22 p.m. UTC | #38
Am 2021-05-30 20:16, schrieb Andy Shevchenko:
> On Sun, May 30, 2021 at 7:51 PM Hans de Goede <hdegoede@redhat.com> 

> wrote:

>> On 5/30/21 6:19 PM, Sander Vanheule wrote:

>> > On Fri, 2021-05-28 at 08:37 +0200, Michael Walle wrote:

> 

> ...

> 

>> > I think I found a solution!


nice!

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

>> 

>> Regmap allows you to mark certain ranges as volatile, so that they 

>> will not

>> be cached, these GPIO registers containing the current pin value seems 

>> like

>> a good candidate for this. This is also necessary to make reading the 

>> GPIO

>> work without getting back a stale, cached value.

> 

> After all it seems a simple missed proper register configuration in

> the driver for regmap.

> Oh, as usual something easy-to-solve requires tons of time to find it. 

> :-)

> 

> Sander, I think you may look at gpio-pca953x.c to understand how it

> works (volatility of registers).


But as far as I see is the regmap instantiated without a cache?

-michael
Bartosz Golaszewski May 31, 2021, 7:25 a.m. UTC | #39
On Mon, May 24, 2021 at 12:34 AM Sander Vanheule <sander@svanheule.net> wrote:
>

> GPIO chips may not support setting the output value when a pin is

> configured as an input, although the current implementation assumes this

> is always possible.

>

> Add support for setting pin direction before value. The order defaults

> to setting the value first, but this can be reversed by setting the

> GPIO_REGMAP_QUIRK_SET_DIRECTION_FIRST flag in regmap_config.quirks.

>

> Signed-off-by: Sander Vanheule <sander@svanheule.net>

> ---


Acked-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Sander Vanheule May 31, 2021, 8:36 a.m. UTC | #40
On Sun, 2021-05-30 at 23:22 +0200, Michael Walle wrote:
> Am 2021-05-30 20:16, schrieb Andy Shevchenko:
> > On Sun, May 30, 2021 at 7:51 PM Hans de Goede <hdegoede@redhat.com> 
> > wrote:
> > > On 5/30/21 6:19 PM, Sander Vanheule wrote:
> > > > 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?
> > > 
> > > Regmap allows you to mark certain ranges as volatile, so that they 
> > > will not
> > > be cached, these GPIO registers containing the current pin value seems 
> > > like
> > > a good candidate for this. This is also necessary to make reading the 
> > > GPIO
> > > work without getting back a stale, cached value.
> > 
> > After all it seems a simple missed proper register configuration in
> > the driver for regmap.
> > Oh, as usual something easy-to-solve requires tons of time to find it. 
> > :-)
> > 
> > Sander, I think you may look at gpio-pca953x.c to understand how it
> > works (volatility of registers).
> 
> But as far as I see is the regmap instantiated without a cache?

That's correct, there currently is no cache, although I could add one.

The data register rather appears to be implemented as a read-only (pin inputs)
register and a write-only (pin outputs) register, aliased on the same register
address.

As I understand, marking the DATA registers as volatile wouldn't help. With a
cache this would force reads to not use the cache, which is indeed required for
the pin input values (DATA register reads). However, the output values (DATA
register writes) can in fact be cached.
Looking at _regmap_update_bits(), marking a register as volatile would only make
a difference if regmap.reg_update_bits is implemented. On an MDIO bus, this
would also be emulated with a lazy RMW (see mdiobus_modify()), which is why I
chose not to implement it for regmap-mdio.

So, I still think the issue lies with the lazy RMW behaviour. The patch below
would force a register update when reg_set_base (the data output register) and
reg_dat_base (the data input register) are identical. Otherwise the two
registers are assumed to have conventional RW behaviour. I'm just not entirely
sure gpio-regmap.c is the right place for this.

---8<---

diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
index 95553734e169..c2fccd19548a 100644
--- a/drivers/gpio/gpio-regmap.c
+++ b/drivers/gpio/gpio-regmap.c
@@ -81,13 +81,16 @@ static void gpio_regmap_set(struct gpio_chip *chip, unsigned
int offset,
 {
        struct gpio_regmap *gpio = gpiochip_get_data(chip);
        unsigned int base = gpio_regmap_addr(gpio->reg_set_base);
+       bool force = gpio->reg_set_base == gpio->reg_dat_base;
        unsigned int reg, mask;
 
        gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
        if (val)
-               regmap_update_bits(gpio->regmap, reg, mask, mask);
+               regmap_update_bits_base(gpio->regmap, reg, mask, mask, NULL,
+                                       false, force);
        else
-               regmap_update_bits(gpio->regmap, reg, mask, 0);
+               regmap_update_bits_base(gpio->regmap, reg, mask, 0, NULL,
+                                       false, force);
 }
 
 static void gpio_regmap_set_with_clear(struct gpio_chip *chip,

--
Best,
Sander
Sander Vanheule May 31, 2021, 3:33 p.m. UTC | #41
On Mon, 2021-05-31 at 14:16 +0300, Andy Shevchenko wrote:
> 

> 

> On Monday, May 31, 2021, Michael Walle <michael@walle.cc> wrote:

> > Am 2021-05-31 10:36, schrieb Sander Vanheule:


...

> > > The data register rather appears to be implemented as a read-only (pin

> > > inputs)

> > > register and a write-only (pin outputs) register, aliased on the same

> > > register

> > > address.

> > > 

> > 

> > Ahh so this makes more sense. If the data register is really write only

> > regardless of the direction mode, then RMW doesn't make any sense at all.

> > Please note, that even if regmap caches values, it might be marked as dirty

> > and it will re-read the values from hardware. So I don't know if that will

> > help you.

> > 

> > So a possible quirk could be

> >  GPIO_REGMAP_WRITE_ONLY_DATA_REG (or something like that)

> > 

> > 

> 

> Isn’t regmap property to do a such? I don’t think any quirks are needed since hw

> works as expected.


The HW works as expected, but it is regmap-gpio's assumption that values read
from reg_set_base reflect the current output value that fails.

I looked a bit more at the provided interface, to see if this can be done with
existing regmap functionality.

The data registers must not be marked volatile, to ensure cached reads. The pin
set function can wrap the RMW in regcache_cache_only + regcache_sync, but this
causes visible glitching on my device.

The pin input values can be read by wrapping the regmap_read in
regcache_cache_bypass guards.

If only the regmap's internal lock is used, the RMW cycle is no longer atomic.
Inside the cache_only guards you can't read the input data, and inside the
cache_bypass guards other register writes cannot be performed, or the cache may
get out of sync. regcache_sync_region could be used, but maybe we would then
miss other registers that were updated in the meantime.

Am I missing something here? It seems to me like the regmap interface can't
really accommodate what's required, unless maybe the rtl8231 regmap users
perform some manual locking. This all seems terribly complicated compared to
using an internal output-value cache inside regmap-gpio.


Best,
Sander
Linus Walleij June 1, 2021, 9:59 a.m. UTC | #42
On Sun, May 30, 2021 at 8:16 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Sun, May 30, 2021 at 7:51 PM Hans de Goede <hdegoede@redhat.com> wrote:

> > Regmap allows you to mark certain ranges as volatile, so that they will not
> > be cached, these GPIO registers containing the current pin value seems like
> > a good candidate for this. This is also necessary to make reading the GPIO
> > work without getting back a stale, cached value.
>
> After all it seems a simple missed proper register configuration in
> the driver for regmap.
> Oh, as usual something easy-to-solve requires tons of time to find it. :-)

This is actually quite interesting.

In the discussion around adding Rust support for the Linux kernel
what I came to realize was that the memory safety that Rust adds is
similar in application and ambition to what e.g. regmap-mmio provides.

One aspect of writing kernel drivers in Rust is to always have
something like regmap between your code and the hardware to
strictly control the memory access pattern.

After all regmap is "memory safety implemented in C".

What we see in cases like this is that not only does that make
things more strict and controlled (after all we have regmap for
a reason) but also makes it possible to generate a whole new
set of bugs by doing an error in how you specify the memory
semantics. As all other paradigms, memory safety thinking
implies that never specify anything wrong.

Just regarding all registers/memory cells in a register page
as default volatile (which is what we do a lot of the time)
has its upsides: bugs like this doesn't happen.

(Just some sidetracking...)

Yours,
Linus Walleij
Michael Walle June 1, 2021, 10:18 a.m. UTC | #43
Am 2021-06-01 11:59, schrieb Linus Walleij:
> Just regarding all registers/memory cells in a register page
> as default volatile (which is what we do a lot of the time)
> has its upsides: bugs like this doesn't happen.

I don't think this is the bug here. If it is really a write-only 
register
the problem is the read in RMW. Because reading the register will return
the input value instead of the (previously written) output value.

-michael
Linus Walleij June 1, 2021, 10:51 a.m. UTC | #44
On Tue, Jun 1, 2021 at 12:18 PM Michael Walle <michael@walle.cc> wrote:
> Am 2021-06-01 11:59, schrieb Linus Walleij:

> > Just regarding all registers/memory cells in a register page
> > as default volatile (which is what we do a lot of the time)
> > has its upsides: bugs like this doesn't happen.
>
> I don't think this is the bug here. If it is really a write-only
> register
> the problem is the read in RMW. Because reading the register will return
> the input value instead of the (previously written) output value.

True that. Write and read semantics differ on the register.

Volatile is used for this and some other things,
like for example interrupts being cleared when a register
is read so it is strictly read-once.

So the regmap config is really important to get right.

IIUC one of the ambitions around Rust is to encode this
in how memory is specified in the language. (I am still
thinking about whether that is really a good idea or not.)

Yours,
Linus Walleij
Michael Walle June 1, 2021, 11:41 a.m. UTC | #45
Am 2021-06-01 12:51, schrieb Linus Walleij:
> On Tue, Jun 1, 2021 at 12:18 PM Michael Walle <michael@walle.cc> wrote:

>> Am 2021-06-01 11:59, schrieb Linus Walleij:

> 

>> > Just regarding all registers/memory cells in a register page

>> > as default volatile (which is what we do a lot of the time)

>> > has its upsides: bugs like this doesn't happen.

>> 

>> I don't think this is the bug here. If it is really a write-only

>> register

>> the problem is the read in RMW. Because reading the register will 

>> return

>> the input value instead of the (previously written) output value.

> 

> True that. Write and read semantics differ on the register.

> 

> Volatile is used for this and some other things,

> like for example interrupts being cleared when a register

> is read so it is strictly read-once.


Isn't that what precious is for?

> So the regmap config is really important to get right.

> 

> IIUC one of the ambitions around Rust is to encode this

> in how memory is specified in the language. (I am still

> thinking about whether that is really a good idea or not.)


-- 
-michael
Linus Walleij June 1, 2021, 11:48 a.m. UTC | #46
On Tue, Jun 1, 2021 at 1:41 PM Michael Walle <michael@walle.cc> wrote:
> Am 2021-06-01 12:51, schrieb Linus Walleij:
> > On Tue, Jun 1, 2021 at 12:18 PM Michael Walle <michael@walle.cc> wrote:
> >> Am 2021-06-01 11:59, schrieb Linus Walleij:
> >
> >> > Just regarding all registers/memory cells in a register page
> >> > as default volatile (which is what we do a lot of the time)
> >> > has its upsides: bugs like this doesn't happen.
> >>
> >> I don't think this is the bug here. If it is really a write-only
> >> register
> >> the problem is the read in RMW. Because reading the register will
> >> return
> >> the input value instead of the (previously written) output value.
> >
> > True that. Write and read semantics differ on the register.
> >
> > Volatile is used for this and some other things,
> > like for example interrupts being cleared when a register
> > is read so it is strictly read-once.
>
> Isn't that what precious is for?

I never figured that one out. But I assume you are right.

Proper regmap semantics documentation is forthcoming! ;)

Yours,
Linus Walleij
Michael Walle June 1, 2021, 11:49 a.m. UTC | #47
Am 2021-05-31 17:48, schrieb Andy Shevchenko:
> On Mon, May 31, 2021 at 6:33 PM Sander Vanheule <sander@svanheule.net> 

> wrote:

>> On Mon, 2021-05-31 at 14:16 +0300, Andy Shevchenko wrote:

>> > On Monday, May 31, 2021, Michael Walle <michael@walle.cc> wrote:

>> > > Am 2021-05-31 10:36, schrieb Sander Vanheule:

> 

>> Am I missing something here? It seems to me like the regmap interface 

>> can't

>> really accommodate what's required, unless maybe the rtl8231 regmap 

>> users

>> perform some manual locking. This all seems terribly complicated 

>> compared to

>> using an internal output-value cache inside regmap-gpio.

> 

> Have you had a chance to look into the PCA953x driver?

> Sounds to me that you are missing the APIs that regmap provides.


What would that be? The register cache? We need to cache the
value somehow, because (still assuming it is write only) we cannot
read it back. Thus the read of the RMW, would need get the
value from the cache. Thus the user of gpio-regmap would need
to make sure, to (a) use a cache for the regmap supplied to
gpio-regmap and (b) populate its initial values correctly. Is
that what you are suggesting? And hopefully, no other user
of the regmap will call regcache_mark_dirty() or something
like that.

I had a quick look at the PCA953x driver but it all its
registers are readable according to the comment on the top
of the file.

-michael
Andy Shevchenko June 1, 2021, 3:24 p.m. UTC | #48
On Tue, Jun 1, 2021 at 2:49 PM Michael Walle <michael@walle.cc> wrote:
> Am 2021-05-31 17:48, schrieb Andy Shevchenko:
> > On Mon, May 31, 2021 at 6:33 PM Sander Vanheule <sander@svanheule.net>
> > wrote:
> >> On Mon, 2021-05-31 at 14:16 +0300, Andy Shevchenko wrote:
> >> > On Monday, May 31, 2021, Michael Walle <michael@walle.cc> wrote:
> >> > > Am 2021-05-31 10:36, schrieb Sander Vanheule:
> >
> >> Am I missing something here? It seems to me like the regmap interface
> >> can't
> >> really accommodate what's required, unless maybe the rtl8231 regmap
> >> users
> >> perform some manual locking. This all seems terribly complicated
> >> compared to
> >> using an internal output-value cache inside regmap-gpio.
> >
> > Have you had a chance to look into the PCA953x driver?
> > Sounds to me that you are missing the APIs that regmap provides.
>
> What would that be? The register cache? We need to cache the
> value somehow, because (still assuming it is write only) we cannot
> read it back. Thus the read of the RMW, would need get the
> value from the cache. Thus the user of gpio-regmap would need
> to make sure, to (a) use a cache for the regmap supplied to
> gpio-regmap and (b) populate its initial values correctly. Is
> that what you are suggesting? And hopefully, no other user
> of the regmap will call regcache_mark_dirty() or something
> like that.
>
> I had a quick look at the PCA953x driver but it all its
> registers are readable according to the comment on the top
> of the file.

Since regmap doesn't have a facility to support the registers _at the
same offset_ with different meaning (depending on data direction), the
only way to handle this (without redesign regmap internals) is to add
specific "pages" via additional bits in the address space. So, let's
say 0 = RW, 1 = RO, 2 = WO. In this case see the following offset
mapping of the hypothetical hardware registers:

REG1 (RW)   0x00 -> 0x000
REG2 (RW)   0x04 -> 0x004
REG3 (RO)   0x08 -> 0x108
REG3 (RW)   0x08 -> 0x208

Then these bits should be masked out. Something similar is done in the
PCA953x driver for extended addresses and autoincrement.

This is what I propose to look at as the starter.
Rob Herring (Arm) June 2, 2021, 7:02 p.m. UTC | #49
On Mon, 24 May 2021 00:34:01 +0200, Sander Vanheule wrote:
> Add a binding description for the Realtek RTL8231, a GPIO and LED
> expander chip commonly used in ethernet switches based on a Realtek
> switch SoC. These chips can be addressed via an MDIO or SMI bus, or used
> as a plain 36-bit shift register.
> 
> This binding only describes the feature set provided by the MDIO/SMI
> configuration, and covers the GPIO, PWM, and pin control properties. The
> LED properties are defined in a separate binding.
> 
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> ---
>  .../bindings/mfd/realtek,rtl8231.yaml         | 190 ++++++++++++++++++
>  1 file changed, 190 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/realtek,rtl8231.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>
Sander Vanheule June 2, 2021, 8:20 p.m. UTC | #50
Hi Andy,

On Tue, 2021-06-01 at 18:24 +0300, Andy Shevchenko wrote:
> On Tue, Jun 1, 2021 at 2:49 PM Michael Walle <michael@walle.cc> wrote:

> > Am 2021-05-31 17:48, schrieb Andy Shevchenko:

> > > On Mon, May 31, 2021 at 6:33 PM Sander Vanheule <sander@svanheule.net>

> > > wrote:

> > > > On Mon, 2021-05-31 at 14:16 +0300, Andy Shevchenko wrote:

> > > > > On Monday, May 31, 2021, Michael Walle <michael@walle.cc> wrote:

> > > > > > Am 2021-05-31 10:36, schrieb Sander Vanheule:

> > > 

> > > > Am I missing something here? It seems to me like the regmap interface

> > > > can't

> > > > really accommodate what's required, unless maybe the rtl8231 regmap

> > > > users

> > > > perform some manual locking. This all seems terribly complicated

> > > > compared to

> > > > using an internal output-value cache inside regmap-gpio.

> > > 

> > > Have you had a chance to look into the PCA953x driver?

> > > Sounds to me that you are missing the APIs that regmap provides.

> > 

> > What would that be? The register cache? We need to cache the

> > value somehow, because (still assuming it is write only) we cannot

> > read it back. Thus the read of the RMW, would need get the

> > value from the cache. Thus the user of gpio-regmap would need

> > to make sure, to (a) use a cache for the regmap supplied to

> > gpio-regmap and (b) populate its initial values correctly. Is

> > that what you are suggesting? And hopefully, no other user

> > of the regmap will call regcache_mark_dirty() or something

> > like that.

> > 

> > I had a quick look at the PCA953x driver but it all its

> > registers are readable according to the comment on the top

> > of the file.

> 

> Since regmap doesn't have a facility to support the registers _at the

> same offset_ with different meaning (depending on data direction), the

> only way to handle this (without redesign regmap internals) is to add

> specific "pages" via additional bits in the address space. So, let's

> say 0 = RW, 1 = RO, 2 = WO. In this case see the following offset

> mapping of the hypothetical hardware registers:

> 

> REG1 (RW)   0x00 -> 0x000

> REG2 (RW)   0x04 -> 0x004

> REG3 (RO)   0x08 -> 0x108

> REG3 (RW)   0x08 -> 0x208

> 

> Then these bits should be masked out. Something similar is done in the

> PCA953x driver for extended addresses and autoincrement.

> 

> This is what I propose to look at as the starter.


Thank you for clarifying. Essentialy this is then the same solution as an extra
cache in gpio-regmap for the output values, except the cacheing is handled by
the regmap layer.

I think this was the last issue standing, so after I implement this, I'll spin a
v4.

Best,
Sander
Sander Vanheule June 3, 2021, 10 a.m. UTC | #51
The RTL8231 GPIO and LED expander can be configured for use as an MDIO or SMI
bus device. Currently only the MDIO mode is supported, although SMI mode
support should be fairly straightforward, once an SMI bus driver is available.

Provided features by the RTL8231:
  - Up to 37 GPIOs
    - Configurable drive strength: 8mA or 4mA (currently unsupported)
    - Input debouncing on high GPIOs (currently unsupported)
  - Up to 88 LEDs in multiple scan matrix groups
    - On, off, or one of six toggling intervals
    - "single-color mode": 2×36 single color LEDs + 8 bi-color LEDs
    - "bi-color mode": (12 + 2×6) bi-color LEDs + 24 single color LEDs
  - Up to one PWM output (currently unsupported)
    - Fixed duty cycle, 8 selectable frequencies (1.2kHz - 4.8kHz)

The GPIO controller uses gpio-regmap. The assumed read-modify-write behaviour
of the data output is provided by using a cached virtual address range for the
output values.

Register access is provided through a new MDIO regmap provider. The required
MDIO regmap support was merged in Mark Brown's regmap repository, and can be
found under the regmap-mdio tag:
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git/tag/?h=regmap-mdio

Changes since v3:
  - Drop gpio-regmap direction-before-value quirk
  - Use secondary virtual register range to enable proper read-modify-write
    behaviour on GPIO output values
  - Add pin debounce support
  - Switch to generic pinmux functions

Changes since v2:
  - MDIO regmap support was merged, so patch is dropped here
  - Implement feedback for DT bindings
  - Use correct module names in Kconfigs
  - Fix k*alloc return value checks
  - Introduce GPIO regmap quirks to set output direction first
  - pinctrl: Use static pin descriptions for pin controller
  - pinctrl: Fix gpio consumer resource leak
  - mfd: Replace CONFIG_PM-ifdef'ery
  - leds: Rename interval to interval_ms

Changes since v1:
  - Reintroduce MDIO regmap, with fixed Kconfig dependencies
  - Add configurable dir/value order for gpio-regmap direction_out call
  - Drop allocations for regmap fields that are used only on init
  - Move some definitions to MFD header
  - Add PM ops to replace driver remove for MFD
  - Change pinctrl driver to (modified) gpio-regmap
  - Change leds driver to use fwnode

Changes since RFC:
  - Dropped MDIO regmap interface. I was unable to resolve the Kconfig
    dependency issue, so have reverted to using regmap_config.reg_read/write.
  - Added pinctrl support
  - Added LED support
  - Changed root device to MFD, with pinctrl and leds child devices. Root
    device is now an mdio_device driver.

Sander Vanheule (5):
  dt-bindings: leds: Binding for RTL8231 scan matrix
  dt-bindings: mfd: Binding for RTL8231
  mfd: Add RTL8231 core device
  pinctrl: Add RTL8231 pin control and GPIO support
  leds: Add support for RTL8231 LED scan matrix

 .../bindings/leds/realtek,rtl8231-leds.yaml   | 166 +++++++
 .../bindings/mfd/realtek,rtl8231.yaml         | 190 ++++++++
 drivers/leds/Kconfig                          |  10 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-rtl8231.c                   | 291 ++++++++++++
 drivers/mfd/Kconfig                           |   9 +
 drivers/mfd/Makefile                          |   1 +
 drivers/mfd/rtl8231.c                         | 240 ++++++++++
 drivers/pinctrl/Kconfig                       |  11 +
 drivers/pinctrl/Makefile                      |   1 +
 drivers/pinctrl/pinctrl-rtl8231.c             | 438 ++++++++++++++++++
 include/linux/mfd/rtl8231.h                   |  78 ++++
 12 files changed, 1436 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/realtek,rtl8231-leds.yaml
 create mode 100644 Documentation/devicetree/bindings/mfd/realtek,rtl8231.yaml
 create mode 100644 drivers/leds/leds-rtl8231.c
 create mode 100644 drivers/mfd/rtl8231.c
 create mode 100644 drivers/pinctrl/pinctrl-rtl8231.c
 create mode 100644 include/linux/mfd/rtl8231.h
Sander Vanheule June 3, 2021, 10:03 a.m. UTC | #52
Hi Michael, Bartosz,

On Fri, 2021-05-28 at 08:40 +0200, Michael Walle wrote:
> Am 2021-05-24 00:33, schrieb Sander Vanheule:
> > GPIO chips may not support setting the output value when a pin is
> > configured as an input, although the current implementation assumes 
> > this
> > is always possible.
> > 
> > Add support for setting pin direction before value. The order defaults
> > to setting the value first, but this can be reversed by setting the
> > GPIO_REGMAP_QUIRK_SET_DIRECTION_FIRST flag in regmap_config.quirks.
> 
> Nice! If this is really needed:
> 
> Reviewed-by: Michael Walle <michael@walle.cc>

Looks like the quirk won't be needed for this series, but I can always resubmit
it separately if needed.


Best,
Sander
Andy Shevchenko June 3, 2021, 10:58 a.m. UTC | #53
On Thu, Jun 3, 2021 at 1:01 PM Sander Vanheule <sander@svanheule.net> wrote:
>
> The RTL8231 is implemented as an MDIO device, and provides a regmap
> interface for register access by the core and child devices.
>
> The chip can also be a device on an SMI bus, an I2C-like bus by Realtek.
> Since kernel support for SMI is limited, and no real-world SMI
> implementations have been encountered for this device, this is currently
> unimplemented. The use of the regmap interface should make any future
> support relatively straightforward.
>
> After reset, all pins are muxed to GPIO inputs before the pin drivers
> are enabled. This is done to prevent accidental system resets, when a
> pin is connected to the parent SoC's reset line.
>
> To provide different read and write semantics for the GPIO data
> registers, a secondary virtual register range is used to enable separate
> cacheing properties of pin input and output values.

caching

...


> +static int rtl8231_reg_read(void *context, unsigned int reg, unsigned int *val)
> +{
> +       struct mdio_device *mdio_dev = context;
> +       int ret;
> +
> +       ret = mdiobus_read(mdio_dev->bus, mdio_dev->addr, RTL8231_REAL_REG(reg));
> +
> +       if (ret < 0)
> +               return ret;
> +
> +       *val = ret & 0xffff;
> +
> +       return 0;
> +}
> +
> +static int rtl8231_reg_write(void *context, unsigned int reg, unsigned int val)
> +{
> +       struct mdio_device *mdio_dev = context;
> +
> +       return mdiobus_write(mdio_dev->bus, mdio_dev->addr, RTL8231_REAL_REG(reg), val);
> +}

Hmm... Maybe we can amend regmap-mdio to avoid duplication of the
above? Something like xlate in gpio-regmap or so?

...

> +       mdiodev->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);

Missed

  if (IS_ERR(mdiodev->reset_gpio))
    return PTR_ERR(mdiodev->reset_gpio);
Sander Vanheule June 3, 2021, 11:28 a.m. UTC | #54
On Thu, 2021-06-03 at 13:58 +0300, Andy Shevchenko wrote:
> On Thu, Jun 3, 2021 at 1:01 PM Sander Vanheule <sander@svanheule.net> wrote:
> > 
> > The RTL8231 is implemented as an MDIO device, and provides a regmap
> > interface for register access by the core and child devices.
> > 
> > The chip can also be a device on an SMI bus, an I2C-like bus by Realtek.
> > Since kernel support for SMI is limited, and no real-world SMI
> > implementations have been encountered for this device, this is currently
> > unimplemented. The use of the regmap interface should make any future
> > support relatively straightforward.
> > 
> > After reset, all pins are muxed to GPIO inputs before the pin drivers
> > are enabled. This is done to prevent accidental system resets, when a
> > pin is connected to the parent SoC's reset line.
> > 
> > To provide different read and write semantics for the GPIO data
> > registers, a secondary virtual register range is used to enable separate
> > cacheing properties of pin input and output values.
> 
> caching
> 
> ...
> 
> 
> > +static int rtl8231_reg_read(void *context, unsigned int reg, unsigned int
> > *val)
> > +{
> > +       struct mdio_device *mdio_dev = context;
> > +       int ret;
> > +
> > +       ret = mdiobus_read(mdio_dev->bus, mdio_dev->addr,
> > RTL8231_REAL_REG(reg));
> > +
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       *val = ret & 0xffff;
> > +
> > +       return 0;
> > +}
> > +
> > +static int rtl8231_reg_write(void *context, unsigned int reg, unsigned int
> > val)
> > +{
> > +       struct mdio_device *mdio_dev = context;
> > +
> > +       return mdiobus_write(mdio_dev->bus, mdio_dev->addr,
> > RTL8231_REAL_REG(reg), val);
> > +}
> 
> Hmm... Maybe we can amend regmap-mdio to avoid duplication of the
> above? Something like xlate in gpio-regmap or so?
> 

I wanted to make the masking explicit, but since regmap-mdio currently requires
a register address width of 5 bit, it could move there.

Actually, can we safely assume that any MDIO driver implementing clause-22
access (5-bit register address width) will just ignore higher bits? In that
case, I could just drop these functions and not even modify regmap-mdio. It
appears to work for bitbanged MDIO.


> > +       mdiodev->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > GPIOD_OUT_LOW);
> 
> Missed
> 
>   if (IS_ERR(mdiodev->reset_gpio))
>     return PTR_ERR(mdiodev->reset_gpio);
> 

Will fix.

Best,
Sander
Andrew Lunn June 3, 2021, 2:03 p.m. UTC | #55
> I wanted to make the masking explicit, but since regmap-mdio currently requires

> a register address width of 5 bit, it could move there.

> 

> Actually, can we safely assume that any MDIO driver implementing clause-22

> access (5-bit register address width) will just ignore higher bits? In that

> case, I could just drop these functions and not even modify regmap-mdio. It

> appears to work for bitbanged MDIO.


How are C45 addresses handled? The API to the MDIO bus driver uses a
register value which is 32 bits in width. Bit 30 indicates the address
is a C45 address, and then you have 21 bits of actual address.
regmap-mdio needs to be generic and support both C22 and C45.

   Andrew
Sander Vanheule June 3, 2021, 3:20 p.m. UTC | #56
On Thu, 2021-06-03 at 16:03 +0200, Andrew Lunn wrote:
> > I wanted to make the masking explicit, but since regmap-mdio currently

> > requires

> > a register address width of 5 bit, it could move there.

> > 

> > Actually, can we safely assume that any MDIO driver implementing clause-22

> > access (5-bit register address width) will just ignore higher bits? In that

> > case, I could just drop these functions and not even modify regmap-mdio. It

> > appears to work for bitbanged MDIO.

> 

> How are C45 addresses handled? The API to the MDIO bus driver uses a

> register value which is 32 bits in width. Bit 30 indicates the address

> is a C45 address, and then you have 21 bits of actual address.

> regmap-mdio needs to be generic and support both C22 and C45.


Currently regmap-mdio will only accept regmap_config structs where the register
width is 5 bit, but this is not enforced for the reg_read/reg_write functions
and the regnum is passed verbatim to mdiobus_read/mdiobus_write.

So, if I understand correctly:
 * for a regmap configured for C22 access, register addresses should be masked
   with 0x1f to create a C22 regnum
 * for a regmap configured for C45 access, register addresses should be masked
   and formatted with 
   (MII_ADDR_C45 | (mdiodev->addr << MII_DEVADDR_C45_SHIFT) | (reg_addr 0xffff))

I would think that a device that supports both C22 and C45 access, can then just
be set up to have two regmaps. If they can be considered to be independent
register spaces, one regmap for each access type would make sense to me.

I'll cook up a patch for this.

Best,
Sander
Sander Vanheule June 5, 2021, 11:07 a.m. UTC | #57
On Thu, 2021-06-03 at 13:58 +0300, Andy Shevchenko wrote:
> On Thu, Jun 3, 2021 at 1:01 PM Sander Vanheule <sander@svanheule.net> wrote:

> > +static int rtl8231_reg_read(void *context, unsigned int reg, unsigned int

> > *val)

> > +{

> > +       struct mdio_device *mdio_dev = context;

> > +       int ret;

> > +

> > +       ret = mdiobus_read(mdio_dev->bus, mdio_dev->addr,

> > RTL8231_REAL_REG(reg));

> > +

> > +       if (ret < 0)

> > +               return ret;

> > +

> > +       *val = ret & 0xffff;

> > +

> > +       return 0;

> > +}

> > +

> > +static int rtl8231_reg_write(void *context, unsigned int reg, unsigned int

> > val)

> > +{

> > +       struct mdio_device *mdio_dev = context;

> > +

> > +       return mdiobus_write(mdio_dev->bus, mdio_dev->addr,

> > RTL8231_REAL_REG(reg), val);

> > +}

> 

> Hmm... Maybe we can amend regmap-mdio to avoid duplication of the

> above? Something like xlate in gpio-regmap or so?


(+Cc Mark for the regmap discussion)

It turns out that I can't use both devm_regmap_init_mdio and the overrides
regmap_config.reg_read/write. This appears to be in contrast to what the
documentation for these two overrides suggest. devm_regmap_init_mdio provides a
bus for the regmap, which causes the overrides to be ignored in regmap.c

Then I tried to use the paging support provided by regmap, by adding the
following config:
   
      static struct regmap_range_cfg rtl8231_reg_ranges[] = {
             {
                     /* Specify an unused register with an empty mask */
                     .selector_reg = 0x1f,
                     .selector_mask = 0x00,
                     .selector_shift = 0,
                     .range_min = 0x00,
                     .range_max = RTL8231_VREG(RTL8231_REG_COUNT - 1),
                     .window_start = 0x00,
                     .window_len = 0x20,
             },
      };


This also doesn't work, because the used _regmap_bus_reg_read/write don't
resolve register pages. The patch below fixes this, but maybe this missing
functionality is intentional, and I should actually implement
regmap_bus.read/write?

----8<----
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 0d185ec018a5..20b6a2e0d2e3 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -1881,6 +1881,15 @@ static int _regmap_bus_reg_write(void *context, unsigned
int reg,
                                 unsigned int val)
 {
        struct regmap *map = context;
+       struct regmap_range_node *range;
+       int ret;
+
+       range = _regmap_range_lookup(map, reg);
+       if (range) {
+               ret = _regmap_select_page(map, &reg, range, 1);
+               if (ret != 0)
+                       return ret;
+       }
 
        return map->bus->reg_write(map->bus_context, reg, val);
 }
@@ -2651,6 +2660,15 @@ static int _regmap_bus_reg_read(void *context, unsigned
int reg,
                                unsigned int *val)
 {
        struct regmap *map = context;
+       struct regmap_range_node *range;
+       int ret;
+
+       range = _regmap_range_lookup(map, reg);
+       if (range) {
+               ret = _regmap_select_page(map, &reg, range, 1);
+               if (ret != 0)
+                       return ret;
+       }
 
        return map->bus->reg_read(map->bus_context, reg, val);
 }

--
Best,
Sander