[RFC PATCH 0/2] MIIM regmap and RTL8231 GPIO expander support

[RFC PATCH 0/2] MIIM regmap and RTL8231 GPIO expander support

[Sander Vanheule]

The RTL8231 GPIO and LED expander can be configured for use as an MIIM or I2C
bus device. To provide uniform data access between these two modes, the regmap
interface is used. Since no regmap interface exists for MIIM busses, a basic
implementation is provided.

Currently outstanding questions:
- The REGMAP_MIIM symbol should depend on MDIO_DEVICE (or a better suited,
  related symbol), but this results in circular Kconfig dependencies:
    drivers/of/Kconfig:69:error: recursive dependency detected!
    drivers/of/Kconfig:69:	symbol OF_IRQ depends on IRQ_DOMAIN
    kernel/irq/Kconfig:59:	symbol IRQ_DOMAIN is selected by REGMAP
    drivers/base/regmap/Kconfig:7:	symbol REGMAP default is visible depending on REGMAP_MIIM
    drivers/base/regmap/Kconfig:39:	symbol REGMAP_MIIM depends on MDIO_DEVICE
    drivers/net/mdio/Kconfig:6:	symbol MDIO_DEVICE is selected by PHYLIB
    drivers/net/phy/Kconfig:16:	symbol PHYLIB is selected by ARC_EMAC_CORE
    drivers/net/ethernet/arc/Kconfig:19:	symbol ARC_EMAC_CORE is selected by ARC_EMAC
    drivers/net/ethernet/arc/Kconfig:25:	symbol ARC_EMAC depends on OF_IRQ
  Suggestions on how to resolve this are welcome.

- Providing no compatible for an MDIO child node is considered to be equivalent
  to a c22 ethernet phy, so one must be provided. However, this node is then
  not automatically probed. Is it okay to provide a binding without a driver?
  If some code is required, where should this be put?
  Current devicetree structure:
    mdio-bus {
        compatible = "vendor,mdio";
        ...

        expander0: expander@0 {
            /*
             * Provide compatible for working registration of mdio device.
             * Device probing happens in gpio1 node.
             */
            compatible = "realtek,rtl8231-expander";
            reg = <0>;
        };

    };
    gpio1 : ext_gpio {
        compatible = "realtek,rtl8231-mdio";
        gpio-controller;
        ...
    };

- MFD driver:
  The RTL8231 is not just a GPIO expander, but also a pin controller and LED
  matrix controller. Regmap initialisation could probably be moved to a parent
  MFD, with gpio, led, and pinctrl cells. Is this a hard requirement if only a
  GPIO controller is provided?

Sander Vanheule (2):
  regmap: add miim bus support
  gpio: Add Realtek RTL8231 support

 drivers/base/regmap/Kconfig       |   6 +-
 drivers/base/regmap/Makefile      |   1 +
 drivers/base/regmap/regmap-miim.c |  58 +++++
 drivers/gpio/Kconfig              |   9 +
 drivers/gpio/Makefile             |   1 +
 drivers/gpio/gpio-rtl8231.c       | 404 ++++++++++++++++++++++++++++++
 include/linux/regmap.h            |  36 +++
 7 files changed, 514 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/regmap/regmap-miim.c
 create mode 100644 drivers/gpio/gpio-rtl8231.c

-- 
2.30.2

[RFC PATCH 1/2] regmap: add miim bus support

[Sander Vanheule]

Basic support for MIIM bus access. Support only includes clause-22
register access, with 5-bit addresses, and 16-bit wide registers.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 drivers/base/regmap/Kconfig       |  6 +++-
 drivers/base/regmap/Makefile      |  1 +
 drivers/base/regmap/regmap-miim.c | 58 +++++++++++++++++++++++++++++++
 include/linux/regmap.h            | 36 +++++++++++++++++++
 4 files changed, 100 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/regmap/regmap-miim.c

diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
index 50b1e2d06a25..5bcd9789284e 100644
--- a/drivers/base/regmap/Kconfig
+++ b/drivers/base/regmap/Kconfig
@@ -4,7 +4,7 @@
 # subsystems should select the appropriate symbols.
 
 config REGMAP
-	default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_W1 || REGMAP_AC97 || REGMAP_MMIO || REGMAP_IRQ || REGMAP_SOUNDWIRE || REGMAP_SOUNDWIRE_MBQ || REGMAP_SCCB || REGMAP_I3C || REGMAP_SPI_AVMM)
+	default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_W1 || REGMAP_AC97 || REGMAP_MMIO || REGMAP_IRQ || REGMAP_SOUNDWIRE || REGMAP_SOUNDWIRE_MBQ || REGMAP_SCCB || REGMAP_I3C || REGMAP_SPI_AVMM || REGMAP_MIIM)
 	select IRQ_DOMAIN if REGMAP_IRQ
 	bool
 
@@ -36,6 +36,10 @@ config REGMAP_W1
 	tristate
 	depends on W1
 
+config REGMAP_MIIM
+	tristate
+	depends on MDIO_DEVICE
+
 config REGMAP_MMIO
 	tristate
 
diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
index 33f63adb5b3d..d80920bd42ce 100644
--- a/drivers/base/regmap/Makefile
+++ b/drivers/base/regmap/Makefile
@@ -19,3 +19,4 @@ obj-$(CONFIG_REGMAP_SOUNDWIRE_MBQ) += regmap-sdw-mbq.o
 obj-$(CONFIG_REGMAP_SCCB) += regmap-sccb.o
 obj-$(CONFIG_REGMAP_I3C) += regmap-i3c.o
 obj-$(CONFIG_REGMAP_SPI_AVMM) += regmap-spi-avmm.o
+obj-$(CONFIG_REGMAP_MIIM) += regmap-miim.o
diff --git a/drivers/base/regmap/regmap-miim.c b/drivers/base/regmap/regmap-miim.c
new file mode 100644
index 000000000000..a560d2910417
--- /dev/null
+++ b/drivers/base/regmap/regmap-miim.c
@@ -0,0 +1,58 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/errno.h>
+#include <linux/mdio.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+static int regmap_miim_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, reg);
+	*val = ret & 0xffff;
+
+	return ret < 0 ? ret : 0;
+}
+
+static int regmap_miim_write(void *context, unsigned int reg, unsigned int val)
+{
+	struct mdio_device *mdio_dev = context;
+
+	return mdiobus_write(mdio_dev->bus, mdio_dev->addr, reg, val);
+}
+
+static const struct regmap_bus regmap_miim_bus = {
+	.reg_write = regmap_miim_write,
+	.reg_read = regmap_miim_read,
+};
+
+struct regmap *__regmap_init_miim(struct mdio_device *mdio_dev,
+	const struct regmap_config *config, struct lock_class_key *lock_key,
+	const char *lock_name)
+{
+	if (config->reg_bits != 5 || config->val_bits != 16)
+		return ERR_PTR(-ENOTSUPP);
+
+	return __regmap_init(&mdio_dev->dev, &regmap_miim_bus, mdio_dev, config,
+		lock_key, lock_name);
+}
+EXPORT_SYMBOL_GPL(__regmap_init_miim);
+
+struct regmap *__devm_regmap_init_miim(struct mdio_device *mdio_dev,
+	const struct regmap_config *config, struct lock_class_key *lock_key,
+	const char *lock_name)
+{
+	if (config->reg_bits != 5 || config->val_bits != 16)
+		return ERR_PTR(-ENOTSUPP);
+
+	return __devm_regmap_init(&mdio_dev->dev, &regmap_miim_bus, mdio_dev,
+		config, lock_key, lock_name);
+}
+EXPORT_SYMBOL_GPL(__devm_regmap_init_miim);
+
+MODULE_AUTHOR("Sander Vanheule <sander@svanheule.net>");
+MODULE_DESCRIPTION("Regmap MIIM Module");
+MODULE_LICENSE("GPL v2");
+
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 2cc4ecd36298..f25630f793c3 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -27,6 +27,7 @@ struct device_node;
 struct i2c_client;
 struct i3c_device;
 struct irq_domain;
+struct mdio_device;
 struct slim_device;
 struct spi_device;
 struct spmi_device;
@@ -538,6 +539,10 @@ struct regmap *__regmap_init_i2c(struct i2c_client *i2c,
 				 const struct regmap_config *config,
 				 struct lock_class_key *lock_key,
 				 const char *lock_name);
+struct regmap *__regmap_init_miim(struct mdio_device *mdio_dev,
+				 const struct regmap_config *config,
+				 struct lock_class_key *lock_key,
+				 const char *lock_name);
 struct regmap *__regmap_init_sccb(struct i2c_client *i2c,
 				  const struct regmap_config *config,
 				  struct lock_class_key *lock_key,
@@ -594,6 +599,10 @@ struct regmap *__devm_regmap_init_i2c(struct i2c_client *i2c,
 				      const struct regmap_config *config,
 				      struct lock_class_key *lock_key,
 				      const char *lock_name);
+struct regmap *__devm_regmap_init_miim(struct mdio_device *mdio_dev,
+				      const struct regmap_config *config,
+				      struct lock_class_key *lock_key,
+				      const char *lock_name);
 struct regmap *__devm_regmap_init_sccb(struct i2c_client *i2c,
 				       const struct regmap_config *config,
 				       struct lock_class_key *lock_key,
@@ -697,6 +706,19 @@ int regmap_attach_dev(struct device *dev, struct regmap *map,
 	__regmap_lockdep_wrapper(__regmap_init_i2c, #config,		\
 				i2c, config)
 
+/**
+ * regmap_init_miim() - Initialise register map
+ *
+ * @mdio_dev: Device that will be interacted with
+ * @config: Configuration for register map
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer to
+ * a struct regmap.
+ */
+#define regmap_init_miim(mdio_dev, config)				\
+	__regmap_lockdep_wrapper(__regmap_init_miim, #config,		\
+				mdio_dev, config)
+
 /**
  * regmap_init_sccb() - Initialise register map
  *
@@ -888,6 +910,20 @@ bool regmap_ac97_default_volatile(struct device *dev, unsigned int reg);
 	__regmap_lockdep_wrapper(__devm_regmap_init_i2c, #config,	\
 				i2c, config)
 
+/**
+ * devm_regmap_init_miim() - Initialise managed register map
+ *
+ * @mdio_dev: Device that will be interacted with
+ * @config: Configuration for register map
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer
+ * to a struct regmap.  The regmap will be automatically freed by the
+ * device management code.
+ */
+#define devm_regmap_init_miim(mdio_dev, config)				\
+	__regmap_lockdep_wrapper(__devm_regmap_init_miim, #config,	\
+				mdio_dev, config)
+
 /**
  * devm_regmap_init_sccb() - Initialise managed register map
  *
-- 
2.30.2

Re: [RFC PATCH 1/2] regmap: add miim bus support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 514 bytes --]

On Thu, Apr 08, 2021 at 10:52:34PM +0200, Sander Vanheule wrote:
> Basic support for MIIM bus access. Support only includes clause-22
> register access, with 5-bit addresses, and 16-bit wide registers.

What is "MIIM"?  A quick search isn't showing up useful hits for that.
Why not just call this MDIO like the rest of the kernel is doing, it
seems like using something else is at best going to make it harder to
discover this code?  If MIIM is some subset or something it's not
obvious how we're limited to that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 1/2] regmap: add miim bus support

[Sander Vanheule]

Hi Mark,

On Fri, 2021-04-09 at 17:07 +0100, Mark Brown wrote:
> On Thu, Apr 08, 2021 at 10:52:34PM +0200, Sander Vanheule wrote:
> > Basic support for MIIM bus access. Support only includes clause-22
> > register access, with 5-bit addresses, and 16-bit wide registers.
> 
> What is "MIIM"?  A quick search isn't showing up useful hits for that.
> Why not just call this MDIO like the rest of the kernel is doing, it
> seems like using something else is at best going to make it harder to
> discover this code?  If MIIM is some subset or something it's not
> obvious how we're limited to that.

MIIM stands for "MII management", i.e. the management bus for devices
with some form of MII interface. MDIO is also frequently used to refer
to the data pin of the bus (there's also MDC: the clock pin), so I
wanted to make the distinction.

The kernel has the mii_bus struct to describe the bus master, but like
you noted the bus is generaly refered to as an MDIO interface. I'm fine
with naming it MDIO to make it easier to spot.

Best,
Sander

Re: [RFC PATCH 1/2] regmap: add miim bus support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 421 bytes --]

On Fri, Apr 09, 2021 at 08:14:22PM +0200, Sander Vanheule wrote:

> The kernel has the mii_bus struct to describe the bus master, but like
> you noted the bus is generaly refered to as an MDIO interface. I'm fine
> with naming it MDIO to make it easier to spot.

Either mii_bus or mdio seem like an improvement - something matching
existing kernel terminology, I guess mdio is consistent with the API it
works with so...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 1/2] regmap: add miim bus support

[Andrew Lunn]

On Fri, Apr 09, 2021 at 07:16:42PM +0100, Mark Brown wrote:
> On Fri, Apr 09, 2021 at 08:14:22PM +0200, Sander Vanheule wrote:
> 
> > The kernel has the mii_bus struct to describe the bus master, but like
> > you noted the bus is generaly refered to as an MDIO interface. I'm fine
> > with naming it MDIO to make it easier to spot.
> 
> Either mii_bus or mdio seem like an improvement - something matching
> existing kernel terminology, I guess mdio is consistent with the API it
> works with so...

I would also suggest mdio. The mii_bus code is an old framework which
does not see any work done to it.

     Andrew

[RFC PATCH 2/2] gpio: Add Realtek RTL8231 support

[Sander Vanheule]

The RTL8231 GPIO and LED expander chip can be controlled via a MIIM or
I2C bus. A regmap interface is used to allow for portable code.

This patch only provides GPIO support, since this is the only known use,
as found on commercial devices.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 drivers/gpio/Kconfig        |   9 +
 drivers/gpio/Makefile       |   1 +
 drivers/gpio/gpio-rtl8231.c | 404 ++++++++++++++++++++++++++++++++++++
 3 files changed, 414 insertions(+)
 create mode 100644 drivers/gpio/gpio-rtl8231.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 20cc0012a5ef..982c87855510 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -521,6 +521,15 @@ config GPIO_REG
 	  A 32-bit single register GPIO fixed in/out implementation.  This
 	  can be used to represent any register as a set of GPIO signals.
 
+config GPIO_RTL8231
+	tristate "Realtek RTL8231 GPIO expander"
+	select REGMAP_I2C
+	select REGMAP_MIIM
+	select OF_MDIO
+	help
+	  RTL8231 GPIO and LED expander support.
+	  When built as a module, the module will be called rtl8231_expander.
+
 config GPIO_SAMA5D2_PIOBU
 	tristate "SAMA5D2 PIOBU GPIO support"
 	depends on MFD_SYSCON
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 7ba71922817e..7f22f0e5430e 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -126,6 +126,7 @@ obj-$(CONFIG_GPIO_RDA)			+= gpio-rda.o
 obj-$(CONFIG_GPIO_RDC321X)		+= gpio-rdc321x.o
 obj-$(CONFIG_GPIO_REALTEK_OTTO)		+= gpio-realtek-otto.o
 obj-$(CONFIG_GPIO_REG)			+= gpio-reg.o
+obj-$(CONFIG_GPIO_RTL8231)		+= gpio-rtl8231.o
 obj-$(CONFIG_ARCH_SA1100)		+= gpio-sa1100.o
 obj-$(CONFIG_GPIO_SAMA5D2_PIOBU)	+= gpio-sama5d2-piobu.o
 obj-$(CONFIG_GPIO_SCH311X)		+= gpio-sch311x.o
diff --git a/drivers/gpio/gpio-rtl8231.c b/drivers/gpio/gpio-rtl8231.c
new file mode 100644
index 000000000000..e0dfee68c859
--- /dev/null
+++ b/drivers/gpio/gpio-rtl8231.c
@@ -0,0 +1,404 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/gpio/driver.h>
+#include <linux/i2c.h>
+#include <linux/mdio.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_mdio.h>
+#include <linux/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+/* RTL8231 registers for LED control */
+#define RTL8231_FUNC0			0x00
+#define RTL8231_FUNC1			0x01
+#define RTL8231_PIN_MODE0		0x02
+#define RTL8231_PIN_MODE1		0x03
+#define RTL8231_PIN_HI_CFG		0x04
+#define RTL8231_GPIO_DIR0		0x05
+#define RTL8231_GPIO_DIR1		0x06
+#define RTL8231_GPIO_INVERT0		0x07
+#define RTL8231_GPIO_INVERT1		0x08
+#define RTL8231_GPIO_DATA0		0x1c
+#define RTL8231_GPIO_DATA1		0x1d
+#define RTL8231_GPIO_DATA2		0x1e
+
+#define RTL8231_READY_CODE_VALUE	0x37
+#define RTL8231_GPIO_DIR_IN		1
+#define RTL8231_GPIO_DIR_OUT		0
+
+#define RTL8231_MAX_GPIOS		37
+
+enum rtl8231_regfield {
+	RTL8231_FIELD_LED_START,
+	RTL8231_FIELD_READY_CODE,
+	RTL8231_FIELD_SOFT_RESET,
+	RTL8231_FIELD_PIN_MODE0,
+	RTL8231_FIELD_PIN_MODE1,
+	RTL8231_FIELD_PIN_MODE2,
+	RTL8231_FIELD_GPIO_DIR0,
+	RTL8231_FIELD_GPIO_DIR1,
+	RTL8231_FIELD_GPIO_DIR2,
+	RTL8231_FIELD_GPIO_DATA0,
+	RTL8231_FIELD_GPIO_DATA1,
+	RTL8231_FIELD_GPIO_DATA2,
+	RTL8231_FIELD_MAX
+};
+
+static const struct reg_field rtl8231_fields[RTL8231_FIELD_MAX] = {
+	[RTL8231_FIELD_LED_START]   = REG_FIELD(RTL8231_FUNC0, 1, 1),
+	[RTL8231_FIELD_READY_CODE]  = REG_FIELD(RTL8231_FUNC1, 4, 9),
+	[RTL8231_FIELD_SOFT_RESET]  = REG_FIELD(RTL8231_PIN_HI_CFG, 15, 15),
+	[RTL8231_FIELD_PIN_MODE0]   = REG_FIELD(RTL8231_PIN_MODE0, 0, 15),
+	[RTL8231_FIELD_PIN_MODE1]   = REG_FIELD(RTL8231_PIN_MODE1, 0, 15),
+	[RTL8231_FIELD_PIN_MODE2]   = REG_FIELD(RTL8231_PIN_HI_CFG, 0, 4),
+	[RTL8231_FIELD_GPIO_DIR0]   = REG_FIELD(RTL8231_GPIO_DIR0, 0, 15),
+	[RTL8231_FIELD_GPIO_DIR1]   = REG_FIELD(RTL8231_GPIO_DIR1, 0, 15),
+	[RTL8231_FIELD_GPIO_DIR2]   = REG_FIELD(RTL8231_PIN_HI_CFG, 5, 9),
+	[RTL8231_FIELD_GPIO_DATA0]  = REG_FIELD(RTL8231_GPIO_DATA0, 0, 15),
+	[RTL8231_FIELD_GPIO_DATA1]  = REG_FIELD(RTL8231_GPIO_DATA1, 0, 15),
+	[RTL8231_FIELD_GPIO_DATA2]  = REG_FIELD(RTL8231_GPIO_DATA2, 0, 4),
+};
+
+/**
+ * struct rtl8231_gpio_ctrl - Control data for an RTL8231 chip
+ *
+ * @gc: Associated gpio_chip instance
+ * @dev
+ * @fields
+ */
+struct rtl8231_gpio_ctrl {
+	struct gpio_chip gc;
+	struct device *dev;
+	struct regmap_field *fields[RTL8231_FIELD_MAX];
+};
+
+static int rtl8231_pin_read(struct rtl8231_gpio_ctrl *ctrl, int base, int offset)
+{
+	int field = base + offset / 16;
+	int bit = offset % 16;
+	unsigned int v;
+	int err;
+
+	err = regmap_field_read(ctrl->fields[field], &v);
+
+	if (err)
+		return err;
+
+	return !!(v & BIT(bit));
+}
+
+static int rtl8231_pin_write(struct rtl8231_gpio_ctrl *ctrl, int base, int offset, int val)
+{
+	int field = base + offset / 16;
+	int bit = offset % 16;
+
+	return regmap_field_update_bits(ctrl->fields[field], BIT(bit), val << bit);
+}
+
+static int rtl8231_direction_input(struct gpio_chip *gc, unsigned int offset)
+{
+	struct rtl8231_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+
+	return rtl8231_pin_write(ctrl, RTL8231_FIELD_GPIO_DIR0, offset, RTL8231_GPIO_DIR_IN);
+}
+
+static int rtl8231_direction_output(struct gpio_chip *gc, unsigned int offset, int value)
+{
+	struct rtl8231_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+	int err;
+
+	err = rtl8231_pin_write(ctrl, RTL8231_FIELD_GPIO_DIR0, offset, RTL8231_GPIO_DIR_OUT);
+	if (err)
+		return err;
+
+	return rtl8231_pin_write(ctrl, RTL8231_FIELD_GPIO_DATA0, offset, value);
+}
+
+static int rtl8231_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	struct rtl8231_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+
+	return rtl8231_pin_read(ctrl, RTL8231_FIELD_GPIO_DIR0, offset);
+}
+
+static int rtl8231_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+	struct rtl8231_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+
+	return rtl8231_pin_read(ctrl, RTL8231_FIELD_GPIO_DATA0, offset);
+}
+
+static void rtl8231_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
+{
+	struct rtl8231_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+
+	rtl8231_pin_write(ctrl, RTL8231_FIELD_GPIO_DATA0, offset, value);
+}
+
+static int rtl8231_gpio_get_multiple(struct gpio_chip *gc,
+	unsigned long *mask, unsigned long *bits)
+{
+	struct rtl8231_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+	int read, field;
+	int offset, shift;
+	int sub_mask;
+	int value, err;
+
+	err = 0;
+	read = 0;
+	field = 0;
+
+	while (read < gc->ngpio) {
+		shift = read % (8 * sizeof(*bits));
+		offset = read / (8 * sizeof(*bits));
+		sub_mask = (mask[offset] >> shift) & 0xffff;
+		if (sub_mask) {
+			err = regmap_field_read(ctrl->fields[RTL8231_FIELD_GPIO_DATA0 + field],
+				&value);
+			if (err)
+				return err;
+			value = (sub_mask & value) << shift;
+			sub_mask <<= shift;
+			bits[offset] = (bits[offset] & ~sub_mask) | value;
+		}
+
+		field += 1;
+		read += 16;
+	}
+
+	return err;
+}
+
+static void rtl8231_gpio_set_multiple(struct gpio_chip *gc,
+	unsigned long *mask, unsigned long *bits)
+{
+	struct rtl8231_gpio_ctrl *ctrl = gpiochip_get_data(gc);
+	int read, field;
+	int offset, shift;
+	int sub_mask;
+	int value;
+
+	read = 0;
+	field = 0;
+
+	while (read < gc->ngpio) {
+		shift = read % (8 * sizeof(*bits));
+		offset = read / (8 * sizeof(*bits));
+		sub_mask = (mask[offset] >> shift) & 0xffff;
+		if (sub_mask) {
+			value = bits[offset] >> shift;
+			regmap_field_update_bits(ctrl->fields[RTL8231_FIELD_GPIO_DATA0 + field],
+				sub_mask, value);
+		}
+
+		field += 1;
+		read += 16;
+	}
+}
+
+static int rtl8231_init(struct rtl8231_gpio_ctrl *ctrl)
+{
+	unsigned int v;
+	int err;
+
+	err = regmap_field_read(ctrl->fields[RTL8231_FIELD_READY_CODE], &v);
+	if (err) {
+		dev_err(ctrl->dev, "failed to read READY_CODE\n");
+		return -ENODEV;
+	} else if (v != RTL8231_READY_CODE_VALUE) {
+		dev_err(ctrl->dev, "RTL8231 not present or ready 0x%x != 0x%x\n",
+			v, RTL8231_READY_CODE_VALUE);
+		return -ENODEV;
+	}
+
+	dev_info(ctrl->dev, "RTL8231 found\n");
+
+	/* If the device was already configured, just leave it alone */
+	err = regmap_field_read(ctrl->fields[RTL8231_FIELD_LED_START], &v);
+	if (err)
+		return err;
+	else if (v)
+		return 0;
+
+	regmap_field_write(ctrl->fields[RTL8231_FIELD_SOFT_RESET], 1);
+	regmap_field_write(ctrl->fields[RTL8231_FIELD_LED_START], 1);
+
+	/* Select GPIO functionality for all pins and set to input */
+	regmap_field_write(ctrl->fields[RTL8231_FIELD_PIN_MODE0], 0xffff);
+	regmap_field_write(ctrl->fields[RTL8231_FIELD_GPIO_DIR0], 0xffff);
+	regmap_field_write(ctrl->fields[RTL8231_FIELD_PIN_MODE1], 0xffff);
+	regmap_field_write(ctrl->fields[RTL8231_FIELD_GPIO_DIR1], 0xffff);
+	regmap_field_write(ctrl->fields[RTL8231_FIELD_PIN_MODE2], 0x1f);
+	regmap_field_write(ctrl->fields[RTL8231_FIELD_GPIO_DIR2], 0x1f);
+
+	return 0;
+}
+
+#define OF_COMPATIBLE_RTL8231_MDIO	"realtek,rtl8231-mdio"
+#define OF_COMPATIBLE_RTL8231_I2C	"realtek,rtl8231-i2c"
+
+static const struct of_device_id rtl8231_gpio_of_match[] = {
+	{ .compatible = OF_COMPATIBLE_RTL8231_MDIO },
+	{ .compatible = OF_COMPATIBLE_RTL8231_I2C },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, rtl8231_gpio_of_match);
+
+static struct regmap *rtl8231_gpio_regmap_mdio(struct device *dev, struct regmap_config *cfg)
+{
+	struct device_node *np = dev->of_node;
+	struct device_node *expander_np = NULL;
+	struct mdio_device *mdiodev;
+
+	expander_np = of_parse_phandle(np, "dev-handle", 0);
+	if (!expander_np) {
+		dev_err(dev, "missing dev-handle node\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	mdiodev = of_mdio_find_device(expander_np);
+	of_node_put(expander_np);
+
+	if (!mdiodev) {
+		dev_err(dev, "failed to find MDIO device\n");
+		return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	cfg->reg_bits = 5;
+	return devm_regmap_init_miim(mdiodev, cfg);
+}
+
+static struct regmap *rtl8231_gpio_regmap_i2c(struct device *dev, struct regmap_config *cfg)
+{
+	struct device_node *np = dev->of_node;
+	struct i2c_client *i2cdev;
+	struct regmap *map;
+	u32 reg_width;
+
+	// TODO untested
+	i2cdev = of_find_i2c_device_by_node(np);
+	if (IS_ERR(i2cdev)) {
+		dev_err(dev, "failed to find I2C device\n");
+		return ERR_PTR(-ENODEV);
+	}
+
+	/* Complete 7-bit I2C address is [1 0 1 0 A2 A1 A0] */
+	if ((i2cdev->addr & ~(0x7)) != 0x50) {
+		dev_err(dev, "invalid address\n");
+		map = ERR_PTR(-EINVAL);
+		goto regmap_i2c_out;
+	}
+
+	if (of_property_read_u32(np, "realtek,regnum-width", &reg_width)
+		|| reg_width != 1 || reg_width != 2) {
+		dev_err(dev, "invalid realtek,regnum-width\n");
+		map = ERR_PTR(-EINVAL);
+		goto regmap_i2c_out;
+	}
+
+	cfg->reg_bits = 8*reg_width;
+	map = devm_regmap_init_i2c(i2cdev, cfg);
+
+regmap_i2c_out:
+	put_device(&i2cdev->dev);
+	return map;
+}
+
+static int rtl8231_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct regmap *map;
+	struct regmap_config regmap_cfg = {};
+	struct rtl8231_gpio_ctrl *ctrl;
+	int field, err;
+	u32 ngpios;
+
+	if (!np) {
+		dev_err(dev, "no DT node found\n");
+		return -EINVAL;
+	}
+
+	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
+	if (!ctrl)
+		return -ENOMEM;
+
+	ngpios = RTL8231_MAX_GPIOS;
+	of_property_read_u32(np, "ngpios", &ngpios);
+	if (ngpios > RTL8231_MAX_GPIOS) {
+		dev_err(dev, "ngpios can be at most %d\n", RTL8231_MAX_GPIOS);
+		return -EINVAL;
+	}
+
+	regmap_cfg.val_bits = 16;
+	regmap_cfg.max_register = 30;
+	regmap_cfg.cache_type = REGCACHE_NONE;
+	regmap_cfg.num_ranges = 0;
+	regmap_cfg.use_single_read = true;
+	regmap_cfg.use_single_write = true;
+	regmap_cfg.reg_format_endian = REGMAP_ENDIAN_BIG;
+	regmap_cfg.val_format_endian = REGMAP_ENDIAN_BIG;
+
+	if (of_device_is_compatible(np, OF_COMPATIBLE_RTL8231_MDIO)) {
+		map = rtl8231_gpio_regmap_mdio(dev, &regmap_cfg);
+	} else if (of_device_is_compatible(np, OF_COMPATIBLE_RTL8231_I2C)) {
+		map = rtl8231_gpio_regmap_i2c(dev, &regmap_cfg);
+	} else {
+		dev_err(dev, "invalid bus type\n");
+		return -ENOTSUPP;
+	}
+
+	if (IS_ERR(map)) {
+		dev_err(dev, "failed to init regmap\n");
+		return PTR_ERR(map);
+	}
+
+	for (field = 0; field < RTL8231_FIELD_MAX; field++) {
+		ctrl->fields[field] = devm_regmap_field_alloc(dev, map, rtl8231_fields[field]);
+		if (IS_ERR(ctrl->fields[field])) {
+			dev_err(dev, "unable to allocate regmap field\n");
+			return PTR_ERR(ctrl->fields[field]);
+		}
+	}
+
+	ctrl->dev = dev;
+	err = rtl8231_init(ctrl);
+	if (err < 0)
+		return err;
+
+	ctrl->gc.base = -1;
+	ctrl->gc.ngpio = ngpios;
+	ctrl->gc.label = "rtl8231-gpio";
+	ctrl->gc.parent = dev;
+	ctrl->gc.owner = THIS_MODULE;
+	ctrl->gc.can_sleep = true;
+
+	ctrl->gc.set = rtl8231_gpio_set;
+	ctrl->gc.set_multiple = rtl8231_gpio_set_multiple;
+	ctrl->gc.get = rtl8231_gpio_get;
+	ctrl->gc.get_multiple = rtl8231_gpio_get_multiple;
+	ctrl->gc.direction_input = rtl8231_direction_input;
+	ctrl->gc.direction_output = rtl8231_direction_output;
+	ctrl->gc.get_direction = rtl8231_get_direction;
+
+	return devm_gpiochip_add_data(dev, &ctrl->gc, ctrl);
+}
+
+static struct platform_driver rtl8231_gpio_driver = {
+	.driver = {
+		.name = "rtl8231-expander",
+		.of_match_table	= rtl8231_gpio_of_match,
+	},
+	.probe = rtl8231_gpio_probe,
+};
+module_platform_driver(rtl8231_gpio_driver);
+
+MODULE_AUTHOR("Sander Vanheule <sander@svanheule.net>");
+MODULE_DESCRIPTION("Realtek RTL8231 GPIO and LED expander support");
+MODULE_LICENSE("GPL v2");
-- 
2.30.2

Re: [RFC PATCH 0/2] MIIM regmap and RTL8231 GPIO expander support

[Andrew Lunn]

> - Providing no compatible for an MDIO child node is considered to be equivalent
>   to a c22 ethernet phy, so one must be provided. However, this node is then
>   not automatically probed.

It cannot be automatically probed, since register 2 and 3 do not
contain an ID, which PHYs do. So you need to explicitly list in on the
MDIO bus, and when the of_mdiobus_register() is called, the device
will be instantiated.

Is it okay to provide a binding without a driver?
>   If some code is required, where should this be put?
>   Current devicetree structure:
>     mdio-bus {
>         compatible = "vendor,mdio";
>         ...
> 
>         expander0: expander@0 {
>             /*
>              * Provide compatible for working registration of mdio device.
>              * Device probing happens in gpio1 node.
>              */
>             compatible = "realtek,rtl8231-expander";
>             reg = <0>;
>         };
> 
>     };
>     gpio1 : ext_gpio {
>         compatible = "realtek,rtl8231-mdio";
>         gpio-controller;
>         ...
>     };

I don't understand this split. Why not

     mdio-bus {
         compatible = "vendor,mdio";
         ...
 
         expander0: expander@0 {
             /*
              * Provide compatible for working registration of mdio device.
              * Device probing happens in gpio1 node.
              */
             compatible = "realtek,rtl8231-expander";
             reg = <0>;
	     gpio-controller;
         };
     };

You can list whatever properties you need in the node. Ethernet
switches have interrupt-controller, embedded MDIO busses with PHYs on
them etc.

> - MFD driver:
>   The RTL8231 is not just a GPIO expander, but also a pin controller and LED
>   matrix controller. Regmap initialisation could probably be moved to a parent
>   MFD, with gpio, led, and pinctrl cells. Is this a hard requirement if only a
>   GPIO controller is provided?

You need to think about forward/backwards compatibility. You are
defining a binding now, which you need to keep. Do you see how an MFD
could be added without breaking backwards compatibility?

      Andrew

Re: [RFC PATCH 0/2] MIIM regmap and RTL8231 GPIO expander support

[Sander Vanheule]

Hi Andrew,

Thank you for the feedback. You can find a (leaked) datasheet at:
https://github.com/libc0607/Realtek_switch_hacking/blob/files/RTL8231_Datasheet_1.2.pdf

On Fri, 2021-04-09 at 00:18 +0200, Andrew Lunn wrote:
> > - Providing no compatible for an MDIO child node is considered to
> > be equivalent
> >   to a c22 ethernet phy, so one must be provided. However, this
> > node is then
> >   not automatically probed.
> 
> It cannot be automatically probed, since register 2 and 3 do not
> contain an ID, which PHYs do. So you need to explicitly list in on
> the
> MDIO bus, and when the of_mdiobus_register() is called, the device
> will be instantiated.
> 
> Is it okay to provide a binding without a driver?
> >   If some code is required, where should this be put?
> >   Current devicetree structure:
> >     mdio-bus {
> >         compatible = "vendor,mdio";
> >         ...
> > 
> >         expander0: expander@0 {
> >             /*
> >              * Provide compatible for working registration of mdio
> > device.
> >              * Device probing happens in gpio1 node.
> >              */
> >             compatible = "realtek,rtl8231-expander";
> >             reg = <0>;
> >         };
> > 
> >     };
> >     gpio1 : ext_gpio {
> >         compatible = "realtek,rtl8231-mdio";
> >         gpio-controller;
> >         ...
> >     };
> 
> I don't understand this split. Why not
> 
>      mdio-bus {
>          compatible = "vendor,mdio";
>          ...
>  
>          expander0: expander@0 {
>              /*
>               * Provide compatible for working registration of mdio
> device.
>               * Device probing happens in gpio1 node.
>               */
>              compatible = "realtek,rtl8231-expander";
>              reg = <0>;
>              gpio-controller;
>          };
>      };
> 
> You can list whatever properties you need in the node. Ethernet
> switches have interrupt-controller, embedded MDIO busses with PHYs on
> them etc.

This is what I tried initially, but it doesn't seem to work. The node
is probably still added as an MDIO device, but rtl8231_gpio_probe()
doesn't appear to get called at all. I do agree it would be preferable
over the split specification.

Having another look, I see mdio_device_id is used for ethernet phys,
but like you said this requires and ID in registers 2 & 3. These
registers contain pin configuration on the RTL8231, so this can't be
used.
Registering as a phy_driver appears to have the same issue, although it
looks like I could use a custom match_phy_device(). I do feel like this
would be stretching the meaning of what a PHY is.


> > - MFD driver:
> >   The RTL8231 is not just a GPIO expander, but also a pin
> > controller and LED
> >   matrix controller. Regmap initialisation could probably be moved
> > to a parent
> >   MFD, with gpio, led, and pinctrl cells. Is this a hard
> > requirement if only a
> >   GPIO controller is provided?
> 
> You need to think about forward/backwards compatibility. You are
> defining a binding now, which you need to keep. Do you see how an MFD
> could be added without breaking backwards compatibility?

There are pin-/gpio-controllers that have the gpio and pinctrl nodes in
the device's root node. So I think adding pinctrl later shouldn't be an
issue. The LED matrix description would probably need a dedicated sub-
node. I'll see if I can write some preliminary bindings later today or
this weekend.

Best,
Sander

Re: [RFC PATCH 0/2] MIIM regmap and RTL8231 GPIO expander support

[Andrew Lunn]

On Fri, Apr 09, 2021 at 07:42:32AM +0200, Sander Vanheule wrote:
> Hi Andrew,
> 
> Thank you for the feedback. You can find a (leaked) datasheet at:
> https://github.com/libc0607/Realtek_switch_hacking/blob/files/RTL8231_Datasheet_1.2.pdf

So this is not really an MFD. It has different ways of making use of
pins, which could be used for GPIO, but can also be used for LEDs. You
could look if it better fits in drivers/leds. But you can also use
GPIO drivers for LEDs via led-gpio.

> > I don't understand this split. Why not
> > 
> >      mdio-bus {
> >          compatible = "vendor,mdio";
> >          ...
> >  
> >          expander0: expander@0 {
> >              /*
> >               * Provide compatible for working registration of mdio
> > device.
> >               * Device probing happens in gpio1 node.
> >               */
> >              compatible = "realtek,rtl8231-expander";
> >              reg = <0>;
> >              gpio-controller;
> >          };
> >      };
> > 
> > You can list whatever properties you need in the node. Ethernet
> > switches have interrupt-controller, embedded MDIO busses with PHYs on
> > them etc.
> 
> This is what I tried initially, but it doesn't seem to work. The node
> is probably still added as an MDIO device, but rtl8231_gpio_probe()
> doesn't appear to get called at all. I do agree it would be preferable
> over the split specification.

Look at drivers/net/dsa/mv88e6xxx/chip.c for how to register an mdio
driver. If you still cannot get it to work, post your code and i will
take a look.

     Andrew

Re: [RFC PATCH 0/2] MIIM regmap and RTL8231 GPIO expander support

[Sander Vanheule]

Hi Andrew,


On Fri, 2021-04-09 at 22:10 +0200, Andrew Lunn wrote:
> On Fri, Apr 09, 2021 at 07:42:32AM +0200, Sander Vanheule wrote:
> > Hi Andrew,
> > 
> > Thank you for the feedback. You can find a (leaked) datasheet at:
> > https://github.com/libc0607/Realtek_switch_hacking/blob/files/RTL8231_Datasheet_1.2.pdf
> 
> So this is not really an MFD. It has different ways of making use of
> pins, which could be used for GPIO, but can also be used for LEDs. You
> could look if it better fits in drivers/leds. But you can also use
> GPIO drivers for LEDs via led-gpio.

The chip provides LED scanning matrix functionality, for which one needs to set up row and column
pins. The chip supports 3×12 + 3×12 + 2×8 (88) LEDs; a lot more than it has pins available. There is
also (limited) support for hardware-accelerated blinking.

For example, single LED color scan matrix ("group A" for ports 0-11) would be wired up as follows:
   
    Row and column pins of scan matrix for LED0, LED1, LED2
    Columns control LED0/1/2 for ports [n] and [n+6]
          L0[n]    L1[n]    L2[n]    L0[n+6]  L1[n+6]  L2[n+6]
            |        |        |        |        |        |
    P0/P6 --X--------X--------X--------X--------X--------X (3)
            |        |        |        |        |        |
    P1/P7 --X--------X--------X--------X--------X--------X (4)
            |        |        |        |        |        |
    P2/P8 --X--------X--------X--------X--------X--------X (5)
            |        |        |        |        |        |
    P3/P9 --X--------X--------X--------X--------X--------X (6)
            |        |        |        |        |        |
   P4/P10 --X--------X--------X--------X--------X--------X (7)
            |        |        |        |        |        |
   P5/P11 --X--------X--------X--------X--------X--------X (8)
           (0)      (1)      (2)      (9)      (10)     (11)

So far, I haven't seen any actual hardware implementation that uses the scanning matrix
functionality (or the buzzer control feature with frequency selection).

1:1 use of GPIO pins for LEDs is indeed trivial with led-gpio, and I am currently using this on one
of my devices.

> > > I don't understand this split. Why not
> > > 
> > >      mdio-bus {
> > >          compatible = "vendor,mdio";
> > >          ...
> > >  
> > >          expander0: expander@0 {
> > >              /*
> > >               * Provide compatible for working registration of mdio
> > > device.
> > >               * Device probing happens in gpio1 node.
> > >               */
> > >              compatible = "realtek,rtl8231-expander";
> > >              reg = <0>;
> > >              gpio-controller;
> > >          };
> > >      };
> > > 
> > > You can list whatever properties you need in the node. Ethernet
> > > switches have interrupt-controller, embedded MDIO busses with PHYs on
> > > them etc.
> > 
> > This is what I tried initially, but it doesn't seem to work. The node
> > is probably still added as an MDIO device, but rtl8231_gpio_probe()
> > doesn't appear to get called at all. I do agree it would be preferable
> > over the split specification.
> 
> Look at drivers/net/dsa/mv88e6xxx/chip.c for how to register an mdio
> driver. If you still cannot get it to work, post your code and i will
> take a look.

Thanks for the suggestion. I've managed to create a cleaner mdio_device driver with a single
corresponding DT node.

Would the following make sense for a more complete DT description? Or would I need sub-nodes to
group e.g. the pin control or LED nodes/properties?

   expander@31 {
   	/* Either "realtek,rtl8231-mdio" or "realtek,rtl8231-smi" */
   	compatible = "realtek,rtl8231-mdio";
   	reg = <31>;
   
   	/* Must be <1> (8 bit) or <2> (16 bit); only for "realtek,rtl8231-smi" */
   	realtek,smi-regnum-width = <1>;
   
   	/** GPIO controller properties **/
   	gpio-controller;
   	#gpio-cells = <2>;
   	ngpios = <37>;
   
   	poe_enable {
   		gpio-hog;
   		gpios = <10 0>;
   		output-high;
   	};
   
   	/** Pin controller properties **/
   	/* Can only set a global drive strength, 4mA or 8mA */
   	realtek,gpio-drive-strength = <4>;
   
   	/* Global LED scan matrix setting, 0 (single-color) or 1 (bi-color) */
   	realtek,led-color-scan-mode = <0>;
   
   	pinctrl-names = "default";
   	pinctrl-0 = <&user_button>, <&port_leds>;
   
   	user_button : user_button_cfg {
   		pins = "gpio31";
   		function = "gpio";
   		/* Only GPIOs 31-35 can do hardware debouncing */
   		/* Debouncing is either disabled or 100ms */
   		input-debounce = <100000>;
   	};
   
   	port_leds : port_leds_cfg {
   		/* Select two columns (LED colors) for switch ports 0-7 */
   		pins = "gpio0", "gpio1", "gpio9", "gpio10",
   		       "gpio3", "gpio4", "gpio5", "gpio6";
   		function = "led";
   	};
   	
   	/** LED config **/
   	#address-cells = <2>;
   	#size-cells = <0>;
   
   	led@0.0 {
   		/* LED0 for port 0, corresponds to bits [2:0] in regnum 0x09 */
   		reg = <0 0>;
   		...
   	};
   	led@0.1 {
   		/* LED1 for port 0 */
   		reg = <0 1>;
   		...
   	};
   	/* LEDs 1.x, 2.x, 3.x, 6.x, 7.x, 8.x, 9.x omitted */
   };
   

As a final remark, I have found out that this chip doesn't actually talk I2C, but rather Realtek's
proprietary SMI. These two protocols are very similar w.r.t. to byte framing, but SMI requires the bus
master to write the register number byte(s) on both READ and WRITE frames. I2C/SMBUS does a WRITE for
the register number first, then a separate READ for the register value. This means I can't get
regmap_i2c to work.

There is an existing, bit-banged implementation of this SMI protocol (see "realtek,smi-mdio", realtek-
smi.c). If I could re-use this in some way, there would only need to be an MDIO implementation.
However, we have noticed that the larger phy address space (7-bit in SMI vs. 5-bit in MDIO) did require
a patch to make it work on ethernet switches with more than 32 ports (and corresponding phys) on a
single SMI bus.

Best,
Sander