[0/5] RTL8231 GPIO expander support

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

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

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.

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

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

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

On Mon, May 17, 2021 at 10:28 PM 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.

But it's broken hardware.
Can it be rather marked as a quirk?

> Add support for setting pin direction before value. The order defaults
> to setting the value first, but this can be reversed by setting the
> regmap_config.no_set_on_input flag, similar to the corresponding flag in
> the gpio-mmio driver.

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?

...

> +         Support for the Realtek RTL8231 GPIO and LED expander.
> +         Provides up to 37 GPIOs, 88 LEDs, and one PWM output.

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

The name is not the one it will be according to Makefile.

> +obj-$(CONFIG_MFD_RTL8231)      += rtl8231.o

...

> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mdio.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>

...

> +static int rtl8231_init(struct device *dev, struct regmap *map)
> +{
> +       unsigned int ready_code;
> +       unsigned int v;

> +       int err = 0;

Redundant assignment.

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

> +       if (err) {
> +               dev_err(dev, "failed to read READY_CODE\n");
> +               return err;

> +       } else if (ready_code != RTL8231_FUNC1_READY_CODE_VALUE) {

Redundant 'else'.

> +               dev_err(dev, "RTL8231 not present or ready 0x%x != 0x%x\n",
> +                       ready_code, RTL8231_FUNC1_READY_CODE_VALUE);
> +               return -ENODEV;
> +       }

...

> +       return err;

Is it somehow different to 0?

> +}

...

> +static int rtl8231_mdio_probe(struct mdio_device *mdiodev)
> +{
> +       struct device *dev = &mdiodev->dev;
> +       struct regmap_field *led_start;
> +       struct regmap *map;
> +       int err;
> +
> +       map = devm_regmap_init_mdio(mdiodev, &rtl8231_mdio_regmap_config);

> +

Redundant blank line.

> +       if (IS_ERR(map)) {
> +               dev_err(dev, "failed to init regmap\n");
> +               return PTR_ERR(map);
> +       }
> +
> +       led_start = devm_regmap_field_alloc(dev, map, RTL8231_FIELD_LED_START);
> +       if (IS_ERR(led_start))
> +               return PTR_ERR(led_start);
> +
> +       dev_set_drvdata(dev, led_start);
> +
> +       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.

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


> +#define RTL8231_PM_OPS (&rtl8231_pm_ops)

> +#else
> +#define RTL8231_PM_OPS NULL
> +#endif /* CONFIG_PM */

...

> +static const struct of_device_id rtl8231_of_match[] = {
> +       { .compatible = "realtek,rtl8231" },
> +       {},

No need to have a comma for the terminator line.

> +};

On Mon, May 17, 2021 at 10:28 PM 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_pin_desc {
> +       unsigned int number;
> +       const char *name;
> +       enum rtl8231_pin_function functions;
> +       u8 reg;
> +       u8 offset;
> +       u8 gpio_function_value;
> +};

I would see rather

sturct pinctrl_pin_desc desc;

Where drv_data describes the rest of the data for pin

...

> +static int rtl8231_get_group_pins(struct pinctrl_dev *pctldev, unsigned int selector,
> +       const unsigned int **pins, unsigned int *num_pins)
> +{

> +       if (selector < ARRAY_SIZE(rtl8231_pins)) {

Can we use traditional pattern, i.e.

  if (... >= ARRAY_SIZE(...))
    return -EINVAL;

  ...
  return 0;

?

> +               *pins = &rtl8231_pins[selector].number;
> +               *num_pins = 1;
> +               return 0;
> +       }
> +
> +       return -EINVAL;
> +}

...

> +static int rtl8231_set_mux(struct pinctrl_dev *pctldev, unsigned int func_selector,
> +       unsigned int group_selector)
> +{

> +       int err = 0;

Redundant variable.

> +       switch (func_flag) {
> +       case RTL8231_PIN_FUNCTION_LED:
> +       case RTL8231_PIN_FUNCTION_PWM:
> +               err = regmap_update_bits(ctrl->map, desc->reg, function_mask, ~gpio_function);
> +               break;
> +       case RTL8231_PIN_FUNCTION_GPIO:
> +               err = regmap_update_bits(ctrl->map, desc->reg, function_mask, gpio_function);
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       return err;
> +}

...

> +static const struct pinmux_ops rtl8231_pinmux_ops = {
> +       .set_mux = rtl8231_set_mux,
> +       .get_functions_count = rtl8231_get_functions_count,
> +       .get_function_name = rtl8231_get_function_name,
> +       .get_function_groups = rtl8231_get_function_groups,
> +       .gpio_request_enable = rtl8231_gpio_request_enable,

> +       .strict = true

Leave comma for non-terminator entries.

> +};
> +
> +

One blank line is enough.

...

> +static int rtl8231_pinctrl_init_functions(struct device *dev, struct rtl8231_pin_ctrl *ctrl)
> +{
> +       struct rtl8231_function *function;
> +       const char **group_name;
> +       unsigned int f_idx;
> +       unsigned int pin;
> +
> +       ctrl->nfunctions = ARRAY_SIZE(rtl8231_pin_function_names);
> +       ctrl->functions = devm_kcalloc(dev, ctrl->nfunctions, sizeof(*ctrl->functions), GFP_KERNEL);
> +       if (IS_ERR(ctrl->functions)) {

Wrong.


> +               dev_err(dev, "failed to allocate pin function descriptors\n");

Noisy message, user space will print the similar.

> +               return PTR_ERR(ctrl->functions);
> +       }
> +
> +       for (f_idx = 0; f_idx < ctrl->nfunctions; f_idx++) {
> +               function = &ctrl->functions[f_idx];
> +               function->name = rtl8231_pin_function_names[f_idx];
> +
> +               for (pin = 0; pin < ctrl->pctl_desc.npins; pin++)
> +                       if (rtl8231_pins[pin].functions & BIT(f_idx))
> +                               function->ngroups++;
> +
> +               function->groups = devm_kcalloc(dev, function->ngroups,
> +                       sizeof(*function->groups), GFP_KERNEL);

> +               if (IS_ERR(function->groups)) {
> +                       dev_err(dev, "failed to allocate pin function group names\n");
> +                       return PTR_ERR(function->groups);
> +               }

Ditto.

> +               group_name = function->groups;
> +               for (pin = 0; pin < ctrl->pctl_desc.npins; pin++)
> +                       if (rtl8231_pins[pin].functions & BIT(f_idx))
> +                               *group_name++ = rtl8231_pins[pin].name;
> +       }
> +
> +       return 0;
> +}
> +
> +static int rtl8231_pinctrl_init(struct device *dev, struct rtl8231_pin_ctrl *ctrl)
> +{
> +       struct pinctrl_dev *pctl;
> +       struct pinctrl_pin_desc *pins;
> +       unsigned int pin;

> +       int err = 0;

Check entire series for unnecessary assignments.They

> +
> +       ctrl->pctl_desc.name = "rtl8231-pinctrl",
> +       ctrl->pctl_desc.owner = THIS_MODULE,
> +       ctrl->pctl_desc.pctlops = &rtl8231_pinctrl_ops,
> +       ctrl->pctl_desc.pmxops = &rtl8231_pinmux_ops,
> +
> +       ctrl->pctl_desc.npins = ARRAY_SIZE(rtl8231_pins);
> +       pins = devm_kcalloc(dev, ctrl->pctl_desc.npins, sizeof(*pins), GFP_KERNEL);
> +       if (IS_ERR(pins)) {
> +               dev_err(dev, "failed to allocate pin descriptors\n");
> +               return PTR_ERR(pins);
> +       }

Ditto.

> +       ctrl->pctl_desc.pins = pins;
> +
> +       for (pin = 0; pin < ctrl->pctl_desc.npins; pin++) {
> +               pins[pin].number = rtl8231_pins[pin].number;
> +               pins[pin].name = rtl8231_pins[pin].name;
> +       }
> +
> +       err = rtl8231_pinctrl_init_functions(dev, ctrl);
> +       if (err)
> +               return err;
> +
> +       err = devm_pinctrl_register_and_init(dev->parent, &ctrl->pctl_desc, ctrl, &pctl);
> +       if (err) {
> +               dev_err(dev, "failed to register pin controller\n");
> +               return err;
> +       }
> +
> +       err = pinctrl_enable(pctl);
> +       if (err)
> +               dev_err(dev, "failed to enable pin controller\n");
> +
> +       return err;
> +}
> +
> +/*
> + * GPIO controller functionality
> + */
> +static int rtl8231_gpio_reg_mask_xlate(struct gpio_regmap *gpio, unsigned int base,
> +       unsigned int offset, unsigned int *reg, unsigned int *mask)
> +{
> +       unsigned int pin_mask = BIT(offset % RTL8231_BITS_VAL);
> +
> +       if (base == RTL8231_REG_GPIO_DATA0 || offset < 32) {
> +               *reg = base + offset / RTL8231_BITS_VAL;
> +               *mask = pin_mask;
> +       } else if (base == RTL8231_REG_GPIO_DIR0) {
> +               *reg = RTL8231_REG_PIN_HI_CFG;
> +               *mask = FIELD_PREP(RTL8231_PIN_HI_CFG_DIR_MASK, pin_mask);
> +       } else {
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
> +static int rtl8231_pinctrl_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct rtl8231_pin_ctrl *ctrl;
> +       struct gpio_regmap_config gpio_cfg = {};
> +       struct gpio_regmap *gr;
> +       int err;
> +
> +       ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
> +       if (!ctrl)
> +               return -ENOMEM;
> +
> +       ctrl->map = dev_get_regmap(dev->parent, NULL);
> +       if (IS_ERR_OR_NULL(ctrl->map)) {
> +               dev_err(dev, "failed to retrieve regmap\n");
> +               if (!ctrl->map)
> +                       return -ENODEV;
> +               else
> +                       return PTR_ERR(ctrl->map);
> +       }
> +
> +       err = rtl8231_pinctrl_init(dev, ctrl);
> +       if (err)
> +               return err;
> +
> +       gpio_cfg.regmap = ctrl->map;
> +       gpio_cfg.parent = dev->parent;
> +       gpio_cfg.ngpio = RTL8231_NUM_GPIOS;
> +       gpio_cfg.ngpio_per_reg = RTL8231_BITS_VAL;
> +
> +       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);
> +       gpio_cfg.no_set_on_input = true;
> +
> +       gpio_cfg.reg_mask_xlate = rtl8231_gpio_reg_mask_xlate;
> +
> +       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);
> +       }
> +
> +       return 0;
> +}
> +
> +static struct platform_driver rtl8231_pinctrl_driver = {
> +       .driver = {
> +               .name = "rtl8231-pinctrl",
> +       },
> +       .probe = rtl8231_pinctrl_probe,
> +};
> +module_platform_driver(rtl8231_pinctrl_driver);
> +
> +MODULE_AUTHOR("Sander Vanheule <sander@svanheule.net>");
> +MODULE_DESCRIPTION("Realtek RTL8231 pin control and GPIO support");
> +MODULE_LICENSE("GPL v2");
> --
> 2.31.1
>

On Tue, May 18, 2021 at 12:42 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, May 17, 2021 at 10:28 PM Sander Vanheule <sander@svanheule.net> wrote:

The rest of the review is here (hit the send before finished).

...

> > +       int err = 0;
>
> Check entire series for unnecessary assignments.They

They even may hide a mistake.

...


> > +static int rtl8231_pinctrl_probe(struct platform_device *pdev)
> > +{

> > +       ctrl->map = dev_get_regmap(dev->parent, NULL);
> > +       if (IS_ERR_OR_NULL(ctrl->map)) {
> > +               dev_err(dev, "failed to retrieve regmap\n");
> > +               if (!ctrl->map)
> > +                       return -ENODEV;
> > +               else
> > +                       return PTR_ERR(ctrl->map);
> > +       }

Simply split the outer conditional to two.

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;
> +       }

On Mon, May 17, 2021 at 09:28:04PM +0200, Sander Vanheule wrote:
> GPIO chips may not support setting the output value when a pin is
> configured as an input

Could you describe what happens with the hardware you are playing
with. Not being able to do this means you will get glitches when
enabling the output so you should not use these GPIOs with bit banging
busses like i2c.

       Andrew

Hi,

Am 2021-05-17 21:28, 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
> regmap_config.no_set_on_input flag, similar to the corresponding flag 
> in
> the gpio-mmio driver.
> 
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> ---
>  drivers/gpio/gpio-regmap.c  | 20 +++++++++++++++++---
>  include/linux/gpio/regmap.h |  3 +++
>  2 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> index 134cedf151a7..1cdb20f8f8b4 100644
> --- a/drivers/gpio/gpio-regmap.c
> +++ b/drivers/gpio/gpio-regmap.c
> @@ -170,14 +170,25 @@ static int gpio_regmap_direction_input(struct
> gpio_chip *chip,
>  	return gpio_regmap_set_direction(chip, offset, false);
>  }
> 
> -static int gpio_regmap_direction_output(struct gpio_chip *chip,
> -					unsigned int offset, int value)
> +static int gpio_regmap_dir_out_val_first(struct gpio_chip *chip,
> +					 unsigned int offset, int value)

Can we leave the name as is? TBH I find these two similar names
super confusing. Maybe its just me, though.

>  {
>  	gpio_regmap_set(chip, offset, value);
> 
>  	return gpio_regmap_set_direction(chip, offset, true);
>  }
> 
> +static int gpio_regmap_dir_out_dir_first(struct gpio_chip *chip,
> +					 unsigned int offset, int value)
> +{
> +	int err;

use ret for consistency here

> +
> +	err = gpio_regmap_set_direction(chip, offset, true);
> +	gpio_regmap_set(chip, offset, value);
> +
> +	return err;
> +}
> +

Instead of adding a new one, we can also just check no_set_on_input
in gpio_regmap_direction_output(), which I'd prefer.

static int gpio_regmap_direction_output(struct gpio_chip *chip,
					unsigned int offset, int value)
{
	struct gpio_regmap *gpio = gpiochip_get_data(chip);
	int ret;

	if (gpio->no_set_on_input) {
		/* some smart comment here, also mention gliches */
		ret = gpio_regmap_set_direction(chip, offset, true);
		gpio_regmap_set(chip, offset, value);
	} else {
		gpio_regmap_set(chip, offset, value);
		ret = gpio_regmap_set_direction(chip, offset, true);
	}

	return ret;
}

>  void gpio_regmap_set_drvdata(struct gpio_regmap *gpio, void *data)
>  {
>  	gpio->driver_data = data;
> @@ -277,7 +288,10 @@ struct gpio_regmap *gpio_regmap_register(const
> struct gpio_regmap_config *config
>  	if (gpio->reg_dir_in_base || gpio->reg_dir_out_base) {
>  		chip->get_direction = gio_regmap_get_direction;
>  		chip->direction_input = gpio_regmap_direction_input;
> -		chip->direction_output = gpio_regmap_direction_output;
> +		if (config->no_set_on_input)
> +			chip->direction_output = gpio_regmap_dir_out_dir_first;
> +		else
> +			chip->direction_output = gpio_regmap_dir_out_val_first;
>  	}
> 
>  	ret = gpiochip_add_data(chip, gpio);
> diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
> index 334dd928042b..2a732f8f23be 100644
> --- a/include/linux/gpio/regmap.h
> +++ b/include/linux/gpio/regmap.h
> @@ -30,6 +30,8 @@ struct regmap;
>   * @reg_dir_out_base:	(Optional) out setting register base address
>   * @reg_stride:		(Optional) May be set if the registers (of the
>   *			same type, dat, set, etc) are not consecutive.
> + * @no_set_on_input:	Set if output value can only be set when the 
> direction
> + *			is configured as output.

set_direction_first ?

>   * @ngpio_per_reg:	Number of GPIOs per register
>   * @irq_domain:		(Optional) IRQ domain if the controller is
>   *			interrupt-capable
> @@ -73,6 +75,7 @@ struct gpio_regmap_config {
>  	unsigned int reg_dir_out_base;
>  	int reg_stride;
>  	int ngpio_per_reg;
> +	bool no_set_on_input;
>  	struct irq_domain *irq_domain;
> 
>  	int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base,

-michael

+Matti

On Tue, May 18, 2021 at 11:39 AM Michael Walle <michael@walle.cc> wrote:
> Am 2021-05-17 21:28, schrieb Sander Vanheule:

...

> Instead of adding a new one, we can also just check no_set_on_input
> in gpio_regmap_direction_output(), which I'd prefer.

+! here.

> static int gpio_regmap_direction_output(struct gpio_chip *chip,
>                                         unsigned int offset, int value)
> {
>         struct gpio_regmap *gpio = gpiochip_get_data(chip);
>         int ret;
>
>         if (gpio->no_set_on_input) {
>                 /* some smart comment here, also mention gliches */
>                 ret = gpio_regmap_set_direction(chip, offset, true);
>                 gpio_regmap_set(chip, offset, value);
>         } else {
>                 gpio_regmap_set(chip, offset, value);
>                 ret = gpio_regmap_set_direction(chip, offset, true);
>         }
>
>         return ret;
> }

...

> > + * @no_set_on_input: Set if output value can only be set when the
> > direction
> > + *                   is configured as output.
>
> set_direction_first ?

Perhaps we need to establish rather something like

/* Broken hardware can't set value on input pin, we have to set it to
output first */
#define GPIO_REGMAP_QUIRK_...  BIT(0)

unsigned int quirks;

?

Hi Andrew,

On Tue, 2021-05-18 at 03:40 +0200, Andrew Lunn wrote:
> On Mon, May 17, 2021 at 09:28:04PM +0200, Sander Vanheule wrote:
> > GPIO chips may not support setting the output value when a pin is
> > configured as an input
> 
> Could you describe what happens with the hardware you are playing
> with. Not being able to do this means you will get glitches when
> enabling the output so you should not use these GPIOs with bit banging
> busses like i2c.

As I was testing this driver, I noticed that output settings for GPIO LEDs,
connected to the RTL8231, weren't being properly set. The actual LED brightness
didn't correspond to the one reported by sysfs. Changing the operation order
fixed this.

However, the vendor code uses I2C bitbanging quite extensively on these chips,
so I decided to have another look.

From u-boot on my device, I can manipulate the RTL8231 registeres relatively
easily. I performed the following short tests:
 * Set pin to input, pull pin high, write output low, change direction: pin
   output changes to low value
 * Set pin to input pull pin low, write output high, change direction: pin
   output changes to high value

Which seems to indicate that I _can_ set output values on input pins... I'll
need to look into this in more detail when I have a bit more time, later this
week.


Best,
Sander

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

On Mon, May 17, 2021 at 09:28:03PM +0200, Sander Vanheule wrote:
> Basic support for MDIO bus access. Support only includes clause-22
> register access, with 5-bit addresses, and 16-bit wide registers.

The following changes since commit 6efb943b8616ec53a5e444193dccf1af9ad627b5:

  Linux 5.13-rc1 (2021-05-09 14:17:44 -0700)

are available in the Git repository at:

  https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git tags/regmap-mdio

for you to fetch changes up to 1f89d2fe16072a74b34bdb895160910091427891:

  regmap: Add MDIO bus support (2021-05-19 14:19:10 +0100)

----------------------------------------------------------------
regmap: Add MDIO bus support

----------------------------------------------------------------
Sander Vanheule (1):
      regmap: Add MDIO bus support

 drivers/base/regmap/Kconfig       |  6 ++++-
 drivers/base/regmap/Makefile      |  1 +
 drivers/base/regmap/regmap-mdio.c | 57 +++++++++++++++++++++++++++++++++++++++
 include/linux/regmap.h            | 36 +++++++++++++++++++++++++
 4 files changed, 99 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/regmap/regmap-mdio.c

Hi Michael,

On Tue, 2021-05-18 at 10:39 +0200, Michael Walle wrote:
> Hi,
> 
> Am 2021-05-17 21:28, 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
> > regmap_config.no_set_on_input flag, similar to the corresponding flag 
> > in
> > the gpio-mmio driver.
> > 
> > Signed-off-by: Sander Vanheule <sander@svanheule.net>
> > ---
> >  drivers/gpio/gpio-regmap.c  | 20 +++++++++++++++++---
> >  include/linux/gpio/regmap.h |  3 +++
> >  2 files changed, 20 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> > index 134cedf151a7..1cdb20f8f8b4 100644
> > --- a/drivers/gpio/gpio-regmap.c
> > +++ b/drivers/gpio/gpio-regmap.c
> > @@ -170,14 +170,25 @@ static int gpio_regmap_direction_input(struct
> > gpio_chip *chip,
> >         return gpio_regmap_set_direction(chip, offset, false);
> >  }
> > 
> > -static int gpio_regmap_direction_output(struct gpio_chip *chip,
> > -                                       unsigned int offset, int value)
> > +static int gpio_regmap_dir_out_val_first(struct gpio_chip *chip,
> > +                                        unsigned int offset, int value)
> 
> Can we leave the name as is? TBH I find these two similar names
> super confusing. Maybe its just me, though.

Sure. This is the implementation used in gpio-mmio.c to provide the same
functionality, so I had used that for consistenty between the two drivers.

> >  {
> >         gpio_regmap_set(chip, offset, value);
> > 
> >         return gpio_regmap_set_direction(chip, offset, true);
> >  }
> > 
> > +static int gpio_regmap_dir_out_dir_first(struct gpio_chip *chip,
> > +                                        unsigned int offset, int value)
> > +{
> > +       int err;
> 
> use ret for consistency here
> 
> > +
> > +       err = gpio_regmap_set_direction(chip, offset, true);
> > +       gpio_regmap_set(chip, offset, value);
> > +
> > +       return err;
> > +}
> > +
> 
> Instead of adding a new one, we can also just check no_set_on_input
> in gpio_regmap_direction_output(), which I'd prefer.
> 
> static int gpio_regmap_direction_output(struct gpio_chip *chip,
>                                         unsigned int offset, int value)
> {
>         struct gpio_regmap *gpio = gpiochip_get_data(chip);
>         int ret;
> 
>         if (gpio->no_set_on_input) {
>                 /* some smart comment here, also mention gliches */
>                 ret = gpio_regmap_set_direction(chip, offset, true);
>                 gpio_regmap_set(chip, offset, value);
>         } else {
>                 gpio_regmap_set(chip, offset, value);
>                 ret = gpio_regmap_set_direction(chip, offset, true);
>         }
> 
>         return ret;
> }
> 

This would certainly make the code a bit easier to follow when you're not
familiar with it :-)
I also see the other functions do checks on static values too, so I'll bring
this function in line with that style.


> >  void gpio_regmap_set_drvdata(struct gpio_regmap *gpio, void *data)
> >  {
> >         gpio->driver_data = data;
> > @@ -277,7 +288,10 @@ struct gpio_regmap *gpio_regmap_register(const
> > struct gpio_regmap_config *config
> >         if (gpio->reg_dir_in_base || gpio->reg_dir_out_base) {
> >                 chip->get_direction = gio_regmap_get_direction;
> >                 chip->direction_input = gpio_regmap_direction_input;
> > -               chip->direction_output = gpio_regmap_direction_output;
> > +               if (config->no_set_on_input)
> > +                       chip->direction_output =
> > gpio_regmap_dir_out_dir_first;
> > +               else
> > +                       chip->direction_output =
> > gpio_regmap_dir_out_val_first;
> >         }
> > 
> >         ret = gpiochip_add_data(chip, gpio);
> > diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
> > index 334dd928042b..2a732f8f23be 100644
> > --- a/include/linux/gpio/regmap.h
> > +++ b/include/linux/gpio/regmap.h
> > @@ -30,6 +30,8 @@ struct regmap;
> >   * @reg_dir_out_base:  (Optional) out setting register base address
> >   * @reg_stride:                (Optional) May be set if the registers (of
> > the
> >   *                     same type, dat, set, etc) are not consecutive.
> > + * @no_set_on_input:   Set if output value can only be set when the 
> > direction
> > + *                     is configured as output.
> 
> set_direction_first ?

This negation can indeed be a bit confusing, I'll change this. As Andy
suggested, I just went for a 'quirks' field, with currently only one defined
flag.

Best,
Sander

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

Hi Andy,

On Tue, 2021-05-18 at 00:42 +0300, Andy Shevchenko wrote:
> On Mon, May 17, 2021 at 10:28 PM 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_pin_desc {
> > +       unsigned int number;
> > +       const char *name;
> > +       enum rtl8231_pin_function functions;
> > +       u8 reg;
> > +       u8 offset;
> > +       u8 gpio_function_value;
> > +};
> 
> I would see rather
> 
> sturct pinctrl_pin_desc desc;
> 
> Where drv_data describes the rest of the data for pin
> 

I've split up the definitions into two parts:
 * pinctrl_pin_desc with the standard info, which has drv_data pointing to...
 * a device-specific rtl8231_pin_desc, with the register field info and
   alternate function

So the pin descriptions are now entirely static, and only the pin functions are
assembled at runtime.

> 
> > +static int rtl8231_get_group_pins(struct pinctrl_dev *pctldev, unsigned int
> > selector,
> > +       const unsigned int **pins, unsigned int *num_pins)
> > +{
> 
> > +       if (selector < ARRAY_SIZE(rtl8231_pins)) {
> 
> Can we use traditional pattern, i.e.
> 
>   if (... >= ARRAY_SIZE(...))
>     return -EINVAL;
> 
>   ...
>   return 0;
> 
> ?

Sure. Will be implemented in v3.

> 
> > +               *pins = &rtl8231_pins[selector].number;
> > +               *num_pins = 1;
> > +               return 0;
> > +       }
> > +
> > +       return -EINVAL;
> > +}
> 
> ...
> 
> > +static int rtl8231_set_mux(struct pinctrl_dev *pctldev, unsigned int
> > func_selector,
> > +       unsigned int group_selector)
> > +{
> 
> > +       int err = 0;
> 
> Redundant variable.
> 
> > +       switch (func_flag) {
> > +       case RTL8231_PIN_FUNCTION_LED:
> > +       case RTL8231_PIN_FUNCTION_PWM:
> > +               err = regmap_update_bits(ctrl->map, desc->reg,
> > function_mask, ~gpio_function);
> > +               break;
> > +       case RTL8231_PIN_FUNCTION_GPIO:
> > +               err = regmap_update_bits(ctrl->map, desc->reg,
> > function_mask, gpio_function);
> > +               break;
> > +       default:
> > +               return -EINVAL;
> > +       }
> > +
> > +       return err;
> > +}
> 

I've reworked this whole section a bit. since a pin is either (only) GPIO, or
some alternative function, this could be done with a simpler if/else.

> 
> > +static const struct pinmux_ops rtl8231_pinmux_ops = {
> > +       .set_mux = rtl8231_set_mux,
> > +       .get_functions_count = rtl8231_get_functions_count,
> > +       .get_function_name = rtl8231_get_function_name,
> > +       .get_function_groups = rtl8231_get_function_groups,
> > +       .gpio_request_enable = rtl8231_gpio_request_enable,
> 
> > +       .strict = true
> 
> Leave comma for non-terminator entries.
> 
> > +};
> > +
> > +
> 
> One blank line is enough.
> 
> ...
> 
> > +static int rtl8231_pinctrl_init_functions(struct device *dev, struct
> > rtl8231_pin_ctrl *ctrl)
> > +{
> > +       struct rtl8231_function *function;
> > +       const char **group_name;
> > +       unsigned int f_idx;
> > +       unsigned int pin;
> > +
> > +       ctrl->nfunctions = ARRAY_SIZE(rtl8231_pin_function_names);
> > +       ctrl->functions = devm_kcalloc(dev, ctrl->nfunctions, sizeof(*ctrl-
> > >functions), GFP_KERNEL);
> > +       if (IS_ERR(ctrl->functions)) {
> 
> Wrong.

I was somehow thinking that this would either return an error value or a valid
point. Don't know where I got that, but should be fixed here and for the other
kallocs.

Best,
Sander

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

Hi Adrew,

On Tue, 2021-05-18 at 03:40 +0200, Andrew Lunn wrote:
> On Mon, May 17, 2021 at 09:28:04PM +0200, Sander Vanheule wrote:
> > GPIO chips may not support setting the output value when a pin is
> > configured as an input
> 
> Could you describe what happens with the hardware you are playing
> with. Not being able to do this means you will get glitches when
> enabling the output so you should not use these GPIOs with bit banging
> busses like i2c.

As I reported earlier, using value-before-direction breaks the GPIO driven LEDs
on one of my devices.

I've tried to use another device to test if I could reproduce this. Using the
gpioset and gpioget utilities, I can't seem to reproduce this however. Whether I
enable the (new) quirk or not, doesn't seem to make any difference.

The documentation we have on this chip is quite scarce, so I'm unaware of
different chip revisions, or how I would distinguish between revisions. As far
as I can see, Realtek's code (present in the GPL archives we got from some
vendors) set the pin direction before setting the value.

For now, I've made the implementation so that the quirk is always applied. Like
the behaviour that is implicit in the origal code. If prefered, I could also
supply a separate devicetree compatible or extra devictree flag.

Regarding bit banged I2C, glitches may not actually be an issue. If a pull-up
resistor is used for HIGH values, and an open drain for LOW values, the GPIO pin
doesn't actually have to change value, only direction (IN for HIGH, OUT for
LOW). A configuration like this would perhaps glitch once on the initial OUT-LOW
configuration. Like I mentioned, bit banged I2C is frequently used in ethernet
switches with these chips to talk to SFP modules.


Best,
Sander

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

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

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?

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

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

On Thu, Jun 3, 2021 at 1:01 PM 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.
>
> Debouncing is only available for the six highest GPIOs, and must be
> emulated when other pins are used for (button) inputs. Although
> described in the bindings, drive strength selection is currently not
> implemented.

Now it looks so nice that I have a temptation to give 2+ tags, but
let's do with one in accordance with process:
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Thanks for doing this! It's a good example of how we can do it in
other cases with regmap and this kind of hardware limitation / design.

> Signed-off-by: Sander Vanheule <sander@svanheule.net>
>
> ---
> v4:
> - Switch to pinmux_generic for pin functions
> - Add pin debounce pinconf
> - Virtual addresses and cacheing
> - Use PRT_ERR_OR_ZERO in pinctrl/gpio probe
> - Drop direction-first quirk for gpio-regmap
>
> v3:
> - Use static pin description for pin controller
> - Fix gpio consumer resource leak
>
> v2:
> - Use gpio-regmap with direction-before-value output
> ---
>  drivers/pinctrl/Kconfig           |  11 +
>  drivers/pinctrl/Makefile          |   1 +
>  drivers/pinctrl/pinctrl-rtl8231.c | 438 ++++++++++++++++++++++++++++++
>  3 files changed, 450 insertions(+)
>  create mode 100644 drivers/pinctrl/pinctrl-rtl8231.c
>
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index c2c7e7963ed0..a02c1befbee4 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -221,6 +221,17 @@ config PINCTRL_ROCKCHIP
>         help
>            This support pinctrl and gpio driver for Rockchip SoCs.
>
> +config PINCTRL_RTL8231
> +       tristate "Realtek RTL8231 GPIO expander's pin controller"
> +       depends on MFD_RTL8231
> +       default MFD_RTL8231
> +       select GPIO_REGMAP
> +       select GENERIC_PINCONF
> +       select GENERIC_PINMUX_FUNCTIONS
> +       help
> +         Support for RTL8231 expander's GPIOs and pin controller.
> +         When built as a module, the module will be called pinctrl-rtl8231.
> +
>  config PINCTRL_SINGLE
>         tristate "One-register-per-pin type device tree based pinctrl driver"
>         depends on OF
> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
> index 5ef5334a797f..239603efb317 100644
> --- a/drivers/pinctrl/Makefile
> +++ b/drivers/pinctrl/Makefile
> @@ -30,6 +30,7 @@ obj-$(CONFIG_PINCTRL_PALMAS)  += pinctrl-palmas.o
>  obj-$(CONFIG_PINCTRL_PIC32)    += pinctrl-pic32.o
>  obj-$(CONFIG_PINCTRL_PISTACHIO)        += pinctrl-pistachio.o
>  obj-$(CONFIG_PINCTRL_ROCKCHIP) += pinctrl-rockchip.o
> +obj-$(CONFIG_PINCTRL_RTL8231)  += pinctrl-rtl8231.o
>  obj-$(CONFIG_PINCTRL_SINGLE)   += pinctrl-single.o
>  obj-$(CONFIG_PINCTRL_SX150X)   += pinctrl-sx150x.o
>  obj-$(CONFIG_ARCH_TEGRA)       += tegra/
> diff --git a/drivers/pinctrl/pinctrl-rtl8231.c b/drivers/pinctrl/pinctrl-rtl8231.c
> new file mode 100644
> index 000000000000..a0f37633b744
> --- /dev/null
> +++ b/drivers/pinctrl/pinctrl-rtl8231.c
> @@ -0,0 +1,438 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/bitfield.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/gpio/regmap.h>
> +#include <linux/module.h>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include "core.h"
> +#include "pinmux.h"
> +#include <linux/mfd/rtl8231.h>
> +
> +#define RTL8231_NUM_GPIOS              37
> +#define RTL8231_DEBOUNCE_USEC          100000
> +#define RTL8231_DEBOUNCE_MIN_OFFSET    31
> +
> +struct rtl8231_pin_ctrl {
> +       struct pinctrl_desc pctl_desc;
> +       struct regmap *map;
> +};
> +
> +/*
> + * Pin controller functionality
> + */
> +static const char * const rtl8231_pin_function_names[] = {
> +       "gpio",
> +       "led",
> +       "pwm",
> +};
> +
> +enum rtl8231_pin_function {
> +       RTL8231_PIN_FUNCTION_GPIO = BIT(0),
> +       RTL8231_PIN_FUNCTION_LED = BIT(1),
> +       RTL8231_PIN_FUNCTION_PWM = BIT(2),
> +};
> +
> +struct rtl8231_pin_desc {
> +       const enum rtl8231_pin_function functions;
> +       const u8 reg;
> +       const u8 offset;
> +       const u8 gpio_function_value;
> +};
> +
> +#define RTL8231_PIN_DESC(_num, _func, _reg, _fld, _val)                \
> +       [_num] = {                                              \
> +               .functions = RTL8231_PIN_FUNCTION_GPIO | _func, \
> +               .reg = _reg,                                    \
> +               .offset = _fld,                                 \
> +               .gpio_function_value = _val,                    \
> +       }
> +#define RTL8231_GPIO_PIN_DESC(_num, _reg, _fld)                        \
> +       RTL8231_PIN_DESC(_num, 0, _reg, _fld, RTL8231_PIN_MODE_GPIO)
> +#define RTL8231_LED_PIN_DESC(_num, _reg, _fld)                 \
> +       RTL8231_PIN_DESC(_num, RTL8231_PIN_FUNCTION_LED, _reg, _fld, RTL8231_PIN_MODE_GPIO)
> +#define RTL8231_PWM_PIN_DESC(_num, _reg, _fld)                 \
> +       RTL8231_PIN_DESC(_num, RTL8231_PIN_FUNCTION_PWM, _reg, _fld, 0)
> +
> +/*
> + * All pins have a GPIO/LED mux bit, but the bits for pins 35/36 are read-only. Use this bit
> + * for the GPIO-only pin instead of a placeholder, so the rest of the logic can stay generic.
> + */
> +static struct rtl8231_pin_desc rtl8231_pin_data[RTL8231_NUM_GPIOS] = {
> +       RTL8231_LED_PIN_DESC(0, RTL8231_REG_PIN_MODE0, 0),
> +       RTL8231_LED_PIN_DESC(1, RTL8231_REG_PIN_MODE0, 1),
> +       RTL8231_LED_PIN_DESC(2, RTL8231_REG_PIN_MODE0, 2),
> +       RTL8231_LED_PIN_DESC(3, RTL8231_REG_PIN_MODE0, 3),
> +       RTL8231_LED_PIN_DESC(4, RTL8231_REG_PIN_MODE0, 4),
> +       RTL8231_LED_PIN_DESC(5, RTL8231_REG_PIN_MODE0, 5),
> +       RTL8231_LED_PIN_DESC(6, RTL8231_REG_PIN_MODE0, 6),
> +       RTL8231_LED_PIN_DESC(7, RTL8231_REG_PIN_MODE0, 7),
> +       RTL8231_LED_PIN_DESC(8, RTL8231_REG_PIN_MODE0, 8),
> +       RTL8231_LED_PIN_DESC(9, RTL8231_REG_PIN_MODE0, 9),
> +       RTL8231_LED_PIN_DESC(10, RTL8231_REG_PIN_MODE0, 10),
> +       RTL8231_LED_PIN_DESC(11, RTL8231_REG_PIN_MODE0, 11),
> +       RTL8231_LED_PIN_DESC(12, RTL8231_REG_PIN_MODE0, 12),
> +       RTL8231_LED_PIN_DESC(13, RTL8231_REG_PIN_MODE0, 13),
> +       RTL8231_LED_PIN_DESC(14, RTL8231_REG_PIN_MODE0, 14),
> +       RTL8231_LED_PIN_DESC(15, RTL8231_REG_PIN_MODE0, 15),
> +       RTL8231_LED_PIN_DESC(16, RTL8231_REG_PIN_MODE1, 0),
> +       RTL8231_LED_PIN_DESC(17, RTL8231_REG_PIN_MODE1, 1),
> +       RTL8231_LED_PIN_DESC(18, RTL8231_REG_PIN_MODE1, 2),
> +       RTL8231_LED_PIN_DESC(19, RTL8231_REG_PIN_MODE1, 3),
> +       RTL8231_LED_PIN_DESC(20, RTL8231_REG_PIN_MODE1, 4),
> +       RTL8231_LED_PIN_DESC(21, RTL8231_REG_PIN_MODE1, 5),
> +       RTL8231_LED_PIN_DESC(22, RTL8231_REG_PIN_MODE1, 6),
> +       RTL8231_LED_PIN_DESC(23, RTL8231_REG_PIN_MODE1, 7),
> +       RTL8231_LED_PIN_DESC(24, RTL8231_REG_PIN_MODE1, 8),
> +       RTL8231_LED_PIN_DESC(25, RTL8231_REG_PIN_MODE1, 9),
> +       RTL8231_LED_PIN_DESC(26, RTL8231_REG_PIN_MODE1, 10),
> +       RTL8231_LED_PIN_DESC(27, RTL8231_REG_PIN_MODE1, 11),
> +       RTL8231_LED_PIN_DESC(28, RTL8231_REG_PIN_MODE1, 12),
> +       RTL8231_LED_PIN_DESC(29, RTL8231_REG_PIN_MODE1, 13),
> +       RTL8231_LED_PIN_DESC(30, RTL8231_REG_PIN_MODE1, 14),
> +       RTL8231_LED_PIN_DESC(31, RTL8231_REG_PIN_MODE1, 15),
> +       RTL8231_LED_PIN_DESC(32, RTL8231_REG_PIN_HI_CFG, 0),
> +       RTL8231_LED_PIN_DESC(33, RTL8231_REG_PIN_HI_CFG, 1),
> +       RTL8231_LED_PIN_DESC(34, RTL8231_REG_PIN_HI_CFG, 2),
> +       RTL8231_PWM_PIN_DESC(35, RTL8231_REG_FUNC1, 3),
> +       RTL8231_GPIO_PIN_DESC(36, RTL8231_REG_PIN_HI_CFG, 4),
> +};
> +
> +#define RTL8231_PIN(_num)                              \
> +       {                                               \
> +               .number = _num,                         \
> +               .name = "gpio" #_num,                   \
> +               .drv_data = &rtl8231_pin_data[_num]     \
> +       }
> +
> +static const struct pinctrl_pin_desc rtl8231_pins[RTL8231_NUM_GPIOS] = {
> +       RTL8231_PIN(0),
> +       RTL8231_PIN(1),
> +       RTL8231_PIN(2),
> +       RTL8231_PIN(3),
> +       RTL8231_PIN(4),
> +       RTL8231_PIN(5),
> +       RTL8231_PIN(6),
> +       RTL8231_PIN(7),
> +       RTL8231_PIN(8),
> +       RTL8231_PIN(9),
> +       RTL8231_PIN(10),
> +       RTL8231_PIN(11),
> +       RTL8231_PIN(12),
> +       RTL8231_PIN(13),
> +       RTL8231_PIN(14),
> +       RTL8231_PIN(15),
> +       RTL8231_PIN(16),
> +       RTL8231_PIN(17),
> +       RTL8231_PIN(18),
> +       RTL8231_PIN(19),
> +       RTL8231_PIN(20),
> +       RTL8231_PIN(21),
> +       RTL8231_PIN(22),
> +       RTL8231_PIN(23),
> +       RTL8231_PIN(24),
> +       RTL8231_PIN(25),
> +       RTL8231_PIN(26),
> +       RTL8231_PIN(27),
> +       RTL8231_PIN(28),
> +       RTL8231_PIN(29),
> +       RTL8231_PIN(30),
> +       RTL8231_PIN(31),
> +       RTL8231_PIN(32),
> +       RTL8231_PIN(33),
> +       RTL8231_PIN(34),
> +       RTL8231_PIN(35),
> +       RTL8231_PIN(36),
> +};
> +
> +static int rtl8231_get_groups_count(struct pinctrl_dev *pctldev)
> +{
> +       return ARRAY_SIZE(rtl8231_pins);
> +}
> +
> +static const char *rtl8231_get_group_name(struct pinctrl_dev *pctldev, unsigned int selector)
> +{
> +       return rtl8231_pins[selector].name;
> +}
> +
> +static int rtl8231_get_group_pins(struct pinctrl_dev *pctldev, unsigned int selector,
> +       const unsigned int **pins, unsigned int *num_pins)
> +{
> +       if (selector >= ARRAY_SIZE(rtl8231_pins))
> +               return -EINVAL;
> +
> +       *pins = &rtl8231_pins[selector].number;
> +       *num_pins = 1;
> +
> +       return 0;
> +}
> +
> +static const struct pinctrl_ops rtl8231_pinctrl_ops = {
> +       .get_groups_count = rtl8231_get_groups_count,
> +       .get_group_name = rtl8231_get_group_name,
> +       .get_group_pins = rtl8231_get_group_pins,
> +       .dt_node_to_map = pinconf_generic_dt_node_to_map_all,
> +       .dt_free_map = pinconf_generic_dt_free_map,
> +};
> +
> +static int rtl8231_set_mux(struct pinctrl_dev *pctldev, unsigned int func_selector,
> +       unsigned int group_selector)
> +{
> +       const struct function_desc *func = pinmux_generic_get_function(pctldev, func_selector);
> +       const struct rtl8231_pin_desc *desc = rtl8231_pins[group_selector].drv_data;
> +       const struct rtl8231_pin_ctrl *ctrl = pinctrl_dev_get_drvdata(pctldev);
> +       unsigned int func_flag = (unsigned int) func->data;
> +       unsigned int function_mask;
> +       unsigned int gpio_function;
> +
> +       if (!(desc->functions & func_flag))
> +               return -EINVAL;
> +
> +       function_mask = BIT(desc->offset);
> +       gpio_function = desc->gpio_function_value << desc->offset;
> +
> +       if (func_flag == RTL8231_PIN_FUNCTION_GPIO)
> +               return regmap_update_bits(ctrl->map, desc->reg, function_mask, gpio_function);
> +       else
> +               return regmap_update_bits(ctrl->map, desc->reg, function_mask, ~gpio_function);
> +}
> +
> +static int rtl8231_gpio_request_enable(struct pinctrl_dev *pctldev,
> +       struct pinctrl_gpio_range *range, unsigned int offset)
> +{
> +       const struct rtl8231_pin_desc *desc = rtl8231_pins[offset].drv_data;
> +       struct rtl8231_pin_ctrl *ctrl = pinctrl_dev_get_drvdata(pctldev);
> +       unsigned int function_mask;
> +       unsigned int gpio_function;
> +
> +       function_mask = BIT(desc->offset);
> +       gpio_function = desc->gpio_function_value << desc->offset;
> +
> +       return regmap_update_bits(ctrl->map, desc->reg, function_mask, gpio_function);
> +}
> +
> +static const struct pinmux_ops rtl8231_pinmux_ops = {
> +       .get_functions_count = pinmux_generic_get_function_count,
> +       .get_function_name = pinmux_generic_get_function_name,
> +       .get_function_groups = pinmux_generic_get_function_groups,
> +       .set_mux = rtl8231_set_mux,
> +       .gpio_request_enable = rtl8231_gpio_request_enable,
> +       .strict = true,
> +};
> +
> +static int rtl8231_pin_config_get(struct pinctrl_dev *pctldev, unsigned int offset,
> +       unsigned long *config)
> +{
> +       struct rtl8231_pin_ctrl *ctrl = pinctrl_dev_get_drvdata(pctldev);
> +       unsigned int param = pinconf_to_config_param(*config);
> +       unsigned int arg;
> +       int err;
> +       int v;
> +
> +       switch (param) {
> +       case PIN_CONFIG_INPUT_DEBOUNCE:
> +               if (offset < RTL8231_DEBOUNCE_MIN_OFFSET)
> +                       return -EINVAL;
> +
> +               err = regmap_read(ctrl->map, RTL8231_REG_FUNC1, &v);
> +               if (err)
> +                       return err;
> +
> +               v = FIELD_GET(RTL8231_FUNC1_DEBOUNCE_MASK, v);
> +               if (v & BIT(offset - RTL8231_DEBOUNCE_MIN_OFFSET))
> +                       arg = RTL8231_DEBOUNCE_USEC;
> +               else
> +                       arg = 0;
> +               break;
> +       default:
> +               return -ENOTSUPP;
> +       }
> +
> +       *config = pinconf_to_config_packed(param, arg);
> +
> +       return 0;
> +}
> +
> +static int rtl8231_pin_config_set(struct pinctrl_dev *pctldev, unsigned int offset,
> +       unsigned long *configs, unsigned int num_configs)
> +{
> +       struct rtl8231_pin_ctrl *ctrl = pinctrl_dev_get_drvdata(pctldev);
> +       unsigned int param, arg;
> +       unsigned int pin_mask;
> +       int err;
> +       int i;
> +
> +       for (i = 0; i < num_configs; i++) {
> +               param = pinconf_to_config_param(configs[i]);
> +               arg = pinconf_to_config_argument(configs[i]);
> +
> +               switch (param) {
> +               case PIN_CONFIG_INPUT_DEBOUNCE:
> +                       if (offset < RTL8231_DEBOUNCE_MIN_OFFSET)
> +                               return -EINVAL;
> +
> +                       pin_mask = FIELD_PREP(RTL8231_FUNC1_DEBOUNCE_MASK,
> +                               BIT(offset - RTL8231_DEBOUNCE_MIN_OFFSET));
> +
> +                       switch (arg) {
> +                       case 0:
> +                               err = regmap_update_bits(ctrl->map, RTL8231_REG_FUNC1,
> +                                       pin_mask, 0);
> +                               break;
> +                       case RTL8231_DEBOUNCE_USEC:
> +                               err = regmap_update_bits(ctrl->map, RTL8231_REG_FUNC1,
> +                                       pin_mask, pin_mask);
> +                               break;
> +                       default:
> +                               return -EINVAL;
> +                       }
> +
> +                       break;
> +               default:
> +                       return -ENOTSUPP;
> +               }
> +       }
> +
> +       return err;
> +}
> +
> +static const struct pinconf_ops rtl8231_pinconf_ops = {
> +       .is_generic = true,
> +       .pin_config_get = rtl8231_pin_config_get,
> +       .pin_config_set = rtl8231_pin_config_set,
> +};
> +
> +static int rtl8231_pinctrl_init_functions(struct pinctrl_dev *pctl, struct rtl8231_pin_ctrl *ctrl)
> +{
> +       const char *function_name;
> +       const char **groups;
> +       unsigned int f_idx;
> +       unsigned int pin;
> +       int num_groups;
> +       int err;
> +
> +       for (f_idx = 0; f_idx < ARRAY_SIZE(rtl8231_pin_function_names); f_idx++) {
> +               function_name = rtl8231_pin_function_names[f_idx];
> +
> +               for (pin = 0, num_groups = 0; pin < ctrl->pctl_desc.npins; pin++)
> +                       if (rtl8231_pin_data[pin].functions & BIT(f_idx))
> +                               num_groups++;
> +
> +               groups = devm_kcalloc(pctl->dev, num_groups, sizeof(*groups), GFP_KERNEL);
> +               if (!groups)
> +                       return -ENOMEM;
> +
> +               for (pin = 0, num_groups = 0; pin < ctrl->pctl_desc.npins; pin++)
> +                       if (rtl8231_pin_data[pin].functions & BIT(f_idx))
> +                               groups[num_groups++] = rtl8231_pins[pin].name;
> +
> +               err = pinmux_generic_add_function(pctl, function_name, groups, num_groups,
> +                       (void *) BIT(f_idx));
> +               if (err < 0)
> +                       return err;
> +       }
> +
> +       return 0;
> +}
> +
> +static int rtl8231_pinctrl_init(struct device *dev, struct rtl8231_pin_ctrl *ctrl)
> +{
> +       struct pinctrl_dev *pctldev;
> +       int err;
> +
> +       ctrl->pctl_desc.name = "rtl8231-pinctrl";
> +       ctrl->pctl_desc.owner = THIS_MODULE;
> +       ctrl->pctl_desc.confops = &rtl8231_pinconf_ops;
> +       ctrl->pctl_desc.pctlops = &rtl8231_pinctrl_ops;
> +       ctrl->pctl_desc.pmxops = &rtl8231_pinmux_ops;
> +       ctrl->pctl_desc.npins = ARRAY_SIZE(rtl8231_pins);
> +       ctrl->pctl_desc.pins = rtl8231_pins;
> +
> +       err = devm_pinctrl_register_and_init(dev->parent, &ctrl->pctl_desc, ctrl, &pctldev);
> +       if (err) {
> +               dev_err(dev, "failed to register pin controller\n");
> +               return err;
> +       }
> +
> +       err = rtl8231_pinctrl_init_functions(pctldev, ctrl);
> +       if (err)
> +               return err;
> +
> +       err = pinctrl_enable(pctldev);
> +       if (err)
> +               dev_err(dev, "failed to enable pin controller\n");
> +
> +       return err;
> +}
> +
> +/*
> + * GPIO controller functionality
> + */
> +static int rtl8231_gpio_reg_mask_xlate(struct gpio_regmap *gpio, unsigned int base,
> +       unsigned int offset, unsigned int *reg, unsigned int *mask)
> +{
> +       unsigned int pin_mask = BIT(offset % RTL8231_BITS_VAL);
> +
> +       if (base == RTL8231_REG_GPIO_DATA_IN0 || base == RTL8231_VREG_GPIO_DATA_OUT0
> +               || offset < 32) {
> +               *reg = base + offset / RTL8231_BITS_VAL;
> +               *mask = pin_mask;
> +       } else if (base == RTL8231_REG_GPIO_DIR0) {
> +               *reg = RTL8231_REG_PIN_HI_CFG;
> +               *mask = FIELD_PREP(RTL8231_PIN_HI_CFG_DIR_MASK, pin_mask);
> +       } else {
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
> +static int rtl8231_pinctrl_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct rtl8231_pin_ctrl *ctrl;
> +       struct gpio_regmap_config gpio_cfg = {};
> +       int err;
> +
> +       ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
> +       if (!ctrl)
> +               return -ENOMEM;
> +
> +       ctrl->map = dev_get_regmap(dev->parent, NULL);
> +       if (!ctrl->map)
> +               return -ENODEV;
> +
> +       err = rtl8231_pinctrl_init(dev, ctrl);
> +       if (err)
> +               return err;
> +
> +       gpio_cfg.regmap = ctrl->map;
> +       gpio_cfg.parent = dev->parent;
> +       gpio_cfg.ngpio = RTL8231_NUM_GPIOS;
> +       gpio_cfg.ngpio_per_reg = RTL8231_BITS_VAL;
> +
> +       gpio_cfg.reg_dat_base = GPIO_REGMAP_ADDR(RTL8231_REG_GPIO_DATA_IN0);
> +       gpio_cfg.reg_set_base = GPIO_REGMAP_ADDR(RTL8231_VREG_GPIO_DATA_OUT0);
> +       gpio_cfg.reg_dir_in_base = GPIO_REGMAP_ADDR(RTL8231_REG_GPIO_DIR0);
> +
> +       gpio_cfg.reg_mask_xlate = rtl8231_gpio_reg_mask_xlate;
> +
> +       return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(dev, &gpio_cfg));
> +}
> +
> +static struct platform_driver rtl8231_pinctrl_driver = {
> +       .driver = {
> +               .name = "rtl8231-pinctrl",
> +       },
> +       .probe = rtl8231_pinctrl_probe,
> +};
> +module_platform_driver(rtl8231_pinctrl_driver);
> +
> +MODULE_AUTHOR("Sander Vanheule <sander@svanheule.net>");
> +MODULE_DESCRIPTION("Realtek RTL8231 pin control and GPIO support");
> +MODULE_LICENSE("GPL v2");
> --
> 2.31.1
>

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

On Thu, Jun 3, 2021 at 1:01 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 a child device with RTL8231 MFD,
> and uses the parent regmap to access the required registers.

FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Sander Vanheule <sander@svanheule.net>
>
> ---
> v4:
> - Rename variable addr_count -> err
> - Use -EINVAL instead of -ENODEV
>
> v3:
> - Rename 'interval' to 'interval_ms'
>
> v2:
> - Use fwnode-calls instead of OF-calls
> ---
>  drivers/leds/Kconfig        |  10 ++
>  drivers/leds/Makefile       |   1 +
>  drivers/leds/leds-rtl8231.c | 291 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 302 insertions(+)
>  create mode 100644 drivers/leds/leds-rtl8231.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 49d99cb084db..8cb869e8cd09 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -593,6 +593,16 @@ config LEDS_REGULATOR
>         help
>           This option enables support for regulator driven LEDs.
>
> +config LEDS_RTL8231
> +       tristate "RTL8231 LED matrix support"
> +       depends on LEDS_CLASS
> +       depends on MFD_RTL8231
> +       default MFD_RTL8231
> +       help
> +         This option enables support for using the LED scanning matrix output
> +         of the RTL8231 GPIO and LED expander chip.
> +         When built as a module, this module will be named leds-rtl8231.
> +
>  config LEDS_BD2802
>         tristate "LED driver for BD2802 RGB LED"
>         depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 7e604d3028c8..ce0f44a87dee 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -80,6 +80,7 @@ obj-$(CONFIG_LEDS_PM8058)             += leds-pm8058.o
>  obj-$(CONFIG_LEDS_POWERNV)             += leds-powernv.o
>  obj-$(CONFIG_LEDS_PWM)                 += leds-pwm.o
>  obj-$(CONFIG_LEDS_REGULATOR)           += leds-regulator.o
> +obj-$(CONFIG_LEDS_RTL8231)             += leds-rtl8231.o
>  obj-$(CONFIG_LEDS_S3C24XX)             += leds-s3c24xx.o
>  obj-$(CONFIG_LEDS_SC27XX_BLTC)         += leds-sc27xx-bltc.o
>  obj-$(CONFIG_LEDS_SGM3140)             += leds-sgm3140.o
> diff --git a/drivers/leds/leds-rtl8231.c b/drivers/leds/leds-rtl8231.c
> new file mode 100644
> index 000000000000..fb2b1ca419c9
> --- /dev/null
> +++ b/drivers/leds/leds-rtl8231.c
> @@ -0,0 +1,291 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +#include <linux/device.h>
> +#include <linux/leds.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/mfd/rtl8231.h>
> +
> +/**
> + * struct led_toggle_rate - description of an LED blinking mode
> + * @interval_ms:       LED toggle rate in milliseconds
> + * @mode:              Register field value used to activate this mode
> + *
> + * 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.
> + */
> +struct led_toggle_rate {
> +       u16 interval_ms;
> +       u8 mode;
> +};
> +
> +/**
> + * struct led_modes - description of all LED modes
> + * @toggle_rates:      Array of led_toggle_rate values, sorted by ascending interval
> + * @num_toggle_rates:  Number of elements in @led_toggle_rate
> + * @off:               Register field value to turn LED off
> + * @on:                        Register field value to turn LED on
> + */
> +struct led_modes {
> +       const struct led_toggle_rate *toggle_rates;
> +       unsigned int num_toggle_rates;
> +       u8 off;
> +       u8 on;
> +};
> +
> +struct rtl8231_led {
> +       struct led_classdev led;
> +       const struct led_modes *modes;
> +       struct regmap_field *reg_field;
> +};
> +#define to_rtl8231_led(_cdev) container_of(_cdev, struct rtl8231_led, led)
> +
> +#define RTL8231_NUM_LEDS       3
> +#define RTL8231_LED_PER_REG    5
> +#define RTL8231_BITS_PER_LED   3
> +
> +static const unsigned int rtl8231_led_port_counts_single[RTL8231_NUM_LEDS] = {32, 32, 24};
> +static const unsigned int rtl8231_led_port_counts_bicolor[RTL8231_NUM_LEDS] = {24, 24, 24};
> +
> +static const unsigned int rtl8231_led_base[RTL8231_NUM_LEDS] = {
> +       RTL8231_REG_LED0_BASE,
> +       RTL8231_REG_LED1_BASE,
> +       RTL8231_REG_LED2_BASE,
> +};
> +
> +#define RTL8231_DEFAULT_TOGGLE_INTERVAL_MS     500
> +
> +static const struct led_toggle_rate rtl8231_toggle_rates[] = {
> +       {  40, 1},
> +       {  80, 2},
> +       { 160, 3},
> +       { 320, 4},
> +       { 640, 5},
> +       {1280, 6},
> +};
> +
> +static const struct led_modes rtl8231_led_modes = {
> +       .off = 0,
> +       .on = 7,
> +       .num_toggle_rates = ARRAY_SIZE(rtl8231_toggle_rates),
> +       .toggle_rates = rtl8231_toggle_rates,
> +};
> +
> +static void rtl8231_led_brightness_set(struct led_classdev *led_cdev,
> +       enum led_brightness brightness)
> +{
> +       struct rtl8231_led *pled = to_rtl8231_led(led_cdev);
> +
> +       if (brightness)
> +               regmap_field_write(pled->reg_field, pled->modes->on);
> +       else
> +               regmap_field_write(pled->reg_field, pled->modes->off);
> +}
> +
> +static enum led_brightness rtl8231_led_brightness_get(struct led_classdev *led_cdev)
> +{
> +       struct rtl8231_led *pled = to_rtl8231_led(led_cdev);
> +       u32 current_mode = pled->modes->off;
> +
> +       regmap_field_read(pled->reg_field, &current_mode);
> +
> +       if (current_mode == pled->modes->off)
> +               return LED_OFF;
> +       else
> +               return LED_ON;
> +}
> +
> +static unsigned int rtl8231_led_current_interval(struct rtl8231_led *pled)
> +{
> +       unsigned int mode;
> +       unsigned int i;
> +
> +       if (regmap_field_read(pled->reg_field, &mode))
> +               return 0;
> +
> +       for (i = 0; i < pled->modes->num_toggle_rates; i++)
> +               if (mode == pled->modes->toggle_rates[i].mode)
> +                       return pled->modes->toggle_rates[i].interval_ms;
> +
> +       return 0;
> +}
> +
> +static int rtl8231_led_blink_set(struct led_classdev *led_cdev, unsigned long *delay_on,
> +       unsigned long *delay_off)
> +{
> +       struct rtl8231_led *pled = to_rtl8231_led(led_cdev);
> +       const struct led_toggle_rate *rates = pled->modes->toggle_rates;
> +       unsigned int num_rates = pled->modes->num_toggle_rates;
> +       unsigned int interval_ms;
> +       unsigned int i;
> +       int err;
> +
> +       if (*delay_on == 0 && *delay_off == 0) {
> +               interval_ms = RTL8231_DEFAULT_TOGGLE_INTERVAL_MS;
> +       } else {
> +               /*
> +                * If the current mode is blinking, choose the delay that (likely) changed.
> +                * Otherwise, choose the interval that would have the same total delay.
> +                */
> +               interval_ms = rtl8231_led_current_interval(pled);
> +               if (interval_ms > 0 && interval_ms == *delay_off)
> +                       interval_ms = *delay_on;
> +               else if (interval_ms > 0 && interval_ms == *delay_on)
> +                       interval_ms = *delay_off;
> +               else
> +                       interval_ms = (*delay_on + *delay_off) / 2;
> +       }
> +
> +       /* Find clamped toggle interval */
> +       for (i = 0; i < (num_rates - 1); i++)
> +               if (interval_ms > rates[i].interval_ms)
> +                       break;
> +
> +       interval_ms = rates[i].interval_ms;
> +
> +       err = regmap_field_write(pled->reg_field, rates[i].mode);
> +       if (err)
> +               return err;
> +
> +       *delay_on = interval_ms;
> +       *delay_off = interval_ms;
> +
> +       return 0;
> +}
> +
> +static int rtl8231_led_read_address(struct fwnode_handle *fwnode, unsigned int *addr_port,
> +       unsigned int *addr_led)
> +{
> +       u32 addr[2];
> +       int err;
> +
> +       err = fwnode_property_count_u32(fwnode, "reg");
> +       if (err < 0)
> +               return err;
> +       if (err != ARRAY_SIZE(addr))
> +               return -EINVAL;
> +
> +       err = fwnode_property_read_u32_array(fwnode, "reg", addr, ARRAY_SIZE(addr));
> +       if (err)
> +               return err;
> +
> +       *addr_port = addr[0];
> +       *addr_led = addr[1];
> +
> +       return 0;
> +}
> +
> +static struct reg_field rtl8231_led_get_field(unsigned int port_index, unsigned int led_index)
> +{
> +       unsigned int offset, shift;
> +       struct reg_field field;
> +
> +       offset = port_index / RTL8231_LED_PER_REG;
> +       shift = (port_index % RTL8231_LED_PER_REG) * RTL8231_BITS_PER_LED;
> +
> +       field.reg = rtl8231_led_base[led_index] + offset;
> +       field.lsb = shift;
> +       field.msb = shift + RTL8231_BITS_PER_LED - 1;
> +
> +       return field;
> +}
> +
> +static int rtl8231_led_probe_single(struct device *dev, struct regmap *map,
> +       const unsigned int *port_counts, struct fwnode_handle *fwnode)
> +{
> +       struct led_init_data init_data = {};
> +       struct rtl8231_led *pled;
> +       unsigned int port_index;
> +       unsigned int led_index;
> +       struct reg_field field;
> +       int err;
> +
> +       pled = devm_kzalloc(dev, sizeof(*pled), GFP_KERNEL);
> +       if (!pled)
> +               return -ENOMEM;
> +
> +       err = rtl8231_led_read_address(fwnode, &port_index, &led_index);
> +       if (err) {
> +               dev_err(dev, "LED address invalid\n");
> +               return err;
> +       }
> +
> +       if (led_index >= RTL8231_NUM_LEDS || port_index >= port_counts[led_index]) {
> +               dev_err(dev, "LED address (%d.%d) invalid\n", port_index, led_index);
> +               return -EINVAL;
> +       }
> +
> +       field = rtl8231_led_get_field(port_index, led_index);
> +       pled->reg_field = devm_regmap_field_alloc(dev, map, field);
> +       if (IS_ERR(pled->reg_field))
> +               return PTR_ERR(pled->reg_field);
> +
> +       pled->modes = &rtl8231_led_modes;
> +
> +       pled->led.max_brightness = 1;
> +       pled->led.brightness_get = rtl8231_led_brightness_get;
> +       pled->led.brightness_set = rtl8231_led_brightness_set;
> +       pled->led.blink_set = rtl8231_led_blink_set;
> +
> +       init_data.fwnode = fwnode;
> +
> +       return devm_led_classdev_register_ext(dev, &pled->led, &init_data);
> +}
> +
> +static int rtl8231_led_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       const unsigned int *port_counts;
> +       struct fwnode_handle *child;
> +       struct regmap *map;
> +       int err;
> +
> +       map = dev_get_regmap(dev->parent, NULL);
> +       if (!map)
> +               return -ENODEV;
> +
> +       if (device_property_match_string(dev, "realtek,led-scan-mode", "single-color") >= 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") >= 0) {
> +               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;
> +       }
> +
> +       fwnode_for_each_available_child_node(dev->fwnode, child) {
> +               err = rtl8231_led_probe_single(dev, map, port_counts, child);
> +               if (err)
> +                       dev_warn(dev, "failed to register LED %pfwP\n", child);
> +       }
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id of_rtl8231_led_match[] = {
> +       { .compatible = "realtek,rtl8231-leds" },
> +       {}
> +};
> +MODULE_DEVICE_TABLE(of, of_rtl8231_led_match);
> +
> +static struct platform_driver rtl8231_led_driver = {
> +       .driver = {
> +               .name = "rtl8231-leds",
> +               .of_match_table = of_rtl8231_led_match,
> +       },
> +       .probe = rtl8231_led_probe,
> +};
> +module_platform_driver(rtl8231_led_driver);
> +
> +MODULE_AUTHOR("Sander Vanheule <sander@svanheule.net>");
> +MODULE_DESCRIPTION("Realtek RTL8231 LED support");
> +MODULE_LICENSE("GPL v2");
> --
> 2.31.1
>

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

> 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

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

On Thu, Jun 3, 2021 at 12:01 PM 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.
>
> Debouncing is only available for the six highest GPIOs, and must be
> emulated when other pins are used for (button) inputs. Although
> described in the bindings, drive strength selection is currently not
> implemented.
>
> Signed-off-by: Sander Vanheule <sander@svanheule.net>

Wow this looks really good. Thanks for going the extra mile
and fix this. Special thanks to Andy for the thorough review.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij