Message-ID: <20251106163316.GV8064@google.com>
Date: Thu, 6 Nov 2025 16:33:16 +0000
From: Lee Jones <lee@...nel.org>
To: Sander Vanheule <sander@...nheule.net>
Cc: Michael Walle <mwalle@...nel.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Bartosz Golaszewski <brgl@...ev.pl>, linux-gpio@...r.kernel.org,
	Pavel Machek <pavel@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, linux-leds@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 5/8] mfd: Add RTL8231 core device

On Tue, 21 Oct 2025, Sander Vanheule 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 a soft 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 main SoC's reset line.
> 
> Signed-off-by: Sander Vanheule <sander@...nheule.net>
> ---
>  drivers/mfd/Kconfig         |   9 ++
>  drivers/mfd/Makefile        |   1 +
>  drivers/mfd/rtl8231.c       | 193 ++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/rtl8231.h |  71 +++++++++++++
>  4 files changed, 274 insertions(+)
>  create mode 100644 drivers/mfd/rtl8231.c
>  create mode 100644 include/linux/mfd/rtl8231.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 6cec1858947b..e13e2df63fee 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1301,6 +1301,15 @@ config MFD_RDC321X
>  	  southbridge which provides access to GPIOs and Watchdog using the
>  	  southbridge PCI device configuration space.
>  
> +config MFD_RTL8231
> +	tristate "Realtek RTL8231 GPIO and LED expander"
> +	select MFD_CORE
> +	select REGMAP_MDIO
> +	help
> +	  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.
> +
>  config MFD_RT4831
>  	tristate "Richtek RT4831 four channel WLED and Display Bias Voltage"
>  	depends on I2C
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 865e9f12faff..ba973382a20f 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -252,6 +252,7 @@ obj-$(CONFIG_MFD_HI6421_PMIC)	+= hi6421-pmic-core.o
>  obj-$(CONFIG_MFD_HI6421_SPMI)	+= hi6421-spmi-pmic.o
>  obj-$(CONFIG_MFD_HI655X_PMIC)   += hi655x-pmic.o
>  obj-$(CONFIG_MFD_DLN2)		+= dln2.o
> +obj-$(CONFIG_MFD_RTL8231)	+= rtl8231.o
>  obj-$(CONFIG_MFD_RT4831)	+= rt4831.o
>  obj-$(CONFIG_MFD_RT5033)	+= rt5033.o
>  obj-$(CONFIG_MFD_RT5120)	+= rt5120.o
> diff --git a/drivers/mfd/rtl8231.c b/drivers/mfd/rtl8231.c
> new file mode 100644
> index 000000000000..60d4a0feea5c
> --- /dev/null
> +++ b/drivers/mfd/rtl8231.c
> @@ -0,0 +1,193 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#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>

Alphabetical please.

> +#include <linux/mfd/rtl8231.h>
> +
> +static bool rtl8231_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	/*
> +	 * Registers with self-clearing bits, strapping pin values.
> +	 * Don't mark the data registers as volatile, since we need
> +	 * caching for the output values.
> +	 */
> +	case RTL8231_REG_FUNC0:
> +	case RTL8231_REG_FUNC1:
> +	case RTL8231_REG_PIN_HI_CFG:
> +	case RTL8231_REG_LED_END:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static const struct reg_field RTL8231_FIELD_LED_START = REG_FIELD(RTL8231_REG_FUNC0, 1, 1);

Why does this have to be global?

Variables should be lowercase.

> +static const struct mfd_cell rtl8231_cells[] = {
> +	{
> +		.name = "rtl8231-pinctrl",
> +	},
> +	{
> +		.name = "rtl8231-leds",
> +		.of_compatible = "realtek,rtl8231-leds",
> +	},
> +};

This is a pretty tenuous MFD!

> +static int rtl8231_soft_reset(struct regmap *map)

s/map/regmap/

> +{
> +	const unsigned int all_pins_mask = GENMASK(RTL8231_BITS_VAL - 1, 0);
> +	unsigned int val;
> +	int err;
> +
> +	/* SOFT_RESET bit self-clears when done */
> +	regmap_write_bits(map, RTL8231_REG_PIN_HI_CFG,
> +		RTL8231_PIN_HI_CFG_SOFT_RESET, RTL8231_PIN_HI_CFG_SOFT_RESET);
> +	err = regmap_read_poll_timeout(map, RTL8231_REG_PIN_HI_CFG, val,
> +		!(val & RTL8231_PIN_HI_CFG_SOFT_RESET), 50, 1000);
> +	if (err)
> +		return err;

Do we have to scrunch these calls together?

> +	regcache_mark_dirty(map);
> +
> +	/*
> +	 * Chip reset results in a pin configuration that is a mix of LED and GPIO outputs.
> +	 * Select GPI functionality for all pins before enabling pin outputs.
> +	 */
> +	regmap_write(map, RTL8231_REG_PIN_MODE0, all_pins_mask);
> +	regmap_write(map, RTL8231_REG_GPIO_DIR0, all_pins_mask);
> +	regmap_write(map, RTL8231_REG_PIN_MODE1, all_pins_mask);
> +	regmap_write(map, RTL8231_REG_GPIO_DIR1, all_pins_mask);
> +	regmap_write(map, RTL8231_REG_PIN_HI_CFG,
> +		RTL8231_PIN_HI_CFG_MODE_MASK | RTL8231_PIN_HI_CFG_DIR_MASK);
> +
> +	return 0;
> +}
> +
> +static int rtl8231_init(struct device *dev, struct regmap *map)
> +{
> +	struct regmap_field *led_start;
> +	unsigned int started;
> +	unsigned int val;

If this is used for only one thing, it makes sense to use better
nomenclature that refers to that thing.

> +	int err;
> +
> +	err = regmap_read(map, RTL8231_REG_FUNC1, &val);
> +	if (err) {
> +		dev_err(dev, "failed to read READY_CODE\n");
> +		return err;
> +	}
> +
> +	val = FIELD_GET(RTL8231_FUNC1_READY_CODE_MASK, val);
> +	if (val != RTL8231_FUNC1_READY_CODE_VALUE) {
> +		dev_err(dev, "RTL8231 not present or ready 0x%x != 0x%x\n",
> +			val, RTL8231_FUNC1_READY_CODE_VALUE);
> +		return -ENODEV;
> +	}
> +
> +	led_start = dev_get_drvdata(dev);
> +	err = regmap_field_read(led_start, &started);
> +	if (err)
> +		return err;
> +
> +	if (!started) {

Reverse the logic and exit early if 'started'.

> +		err = rtl8231_soft_reset(map);
> +		if (err)
> +			return err;

'\n' here.

> +		/* LED_START enables power to output pins, and starts the LED engine */
> +		err = regmap_field_force_write(led_start, 1);
> +	}
> +
> +	return err;
> +}
> +
> +static const struct regmap_config rtl8231_mdio_regmap_config = {
> +	.val_bits = RTL8231_BITS_VAL,
> +	.reg_bits = RTL8231_BITS_REG,
> +	.volatile_reg = rtl8231_volatile_reg,
> +	.max_register = RTL8231_REG_COUNT - 1,
> +	.use_single_read = true,
> +	.use_single_write = true,
> +	.reg_format_endian = REGMAP_ENDIAN_BIG,
> +	.val_format_endian = REGMAP_ENDIAN_BIG,
> +	/* Cannot use REGCACHE_FLAT because it's not smart enough about cache invalidation  */
> +	.cache_type = REGCACHE_MAPLE,
> +};
> +
> +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);
> +	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);

Why do we need to do LED specific actions in the core driver?

> +	dev_set_drvdata(dev, led_start);
> +
> +	mdiodev->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(mdiodev->reset_gpio))
> +		return PTR_ERR(mdiodev->reset_gpio);
> +
> +	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)
> +		return err;
> +
> +	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, rtl8231_cells,
> +		ARRAY_SIZE(rtl8231_cells), NULL, 0, NULL);

Odd tabbing.  Please line-up with the '(' like usual.

> +}
> +
> +__maybe_unused static int rtl8231_suspend(struct device *dev)

The __maybe_unused comes after the "static int" part.

> +{
> +	struct regmap_field *led_start = dev_get_drvdata(dev);
> +
> +	return regmap_field_force_write(led_start, 0);
> +}
> +
> +__maybe_unused static int rtl8231_resume(struct device *dev)
> +{
> +	struct regmap_field *led_start = dev_get_drvdata(dev);
> +
> +	return regmap_field_force_write(led_start, 1);
> +}
> +
> +static SIMPLE_DEV_PM_OPS(rtl8231_pm_ops, rtl8231_suspend, rtl8231_resume);
> +
> +static const struct of_device_id rtl8231_of_match[] = {
> +	{ .compatible = "realtek,rtl8231" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, rtl8231_of_match);
> +
> +static struct mdio_driver rtl8231_mdio_driver = {
> +	.mdiodrv.driver = {
> +		.name = "rtl8231-expander",
> +		.of_match_table	= rtl8231_of_match,
> +		.pm = pm_ptr(&rtl8231_pm_ops),
> +	},
> +	.probe = rtl8231_mdio_probe,
> +};
> +mdio_module_driver(rtl8231_mdio_driver);
> +
> +MODULE_AUTHOR("Sander Vanheule <sander@...nheule.net>");
> +MODULE_DESCRIPTION("Realtek RTL8231 GPIO and LED expander");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/rtl8231.h b/include/linux/mfd/rtl8231.h
> new file mode 100644
> index 000000000000..003eda3797a3
> --- /dev/null
> +++ b/include/linux/mfd/rtl8231.h
> @@ -0,0 +1,71 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Register definitions the RTL8231 GPIO and LED expander chip
> + */
> +
> +#ifndef __LINUX_MFD_RTL8231_H
> +#define __LINUX_MFD_RTL8231_H
> +
> +#include <linux/bits.h>
> +
> +/*
> + * Registers addresses are 5 bit, values are 16 bit
> + * Also define a duplicated range of virtual addresses, to enable
> + * different read/write behaviour on the GPIO data registers
> + */
> +#define RTL8231_BITS_VAL		16
> +#define RTL8231_BITS_REG		5
> +
> +/* Chip control */
> +#define RTL8231_REG_FUNC0		0x00
> +#define RTL8231_FUNC0_SCAN_MODE		BIT(0)
> +#define RTL8231_FUNC0_SCAN_SINGLE	0
> +#define RTL8231_FUNC0_SCAN_BICOLOR	BIT(0)
> +
> +#define RTL8231_REG_FUNC1		0x01
> +#define RTL8231_FUNC1_READY_CODE_VALUE	0x37
> +#define RTL8231_FUNC1_READY_CODE_MASK	GENMASK(9, 4)
> +#define RTL8231_FUNC1_DEBOUNCE_MASK	GENMASK(15, 10)
> +
> +/* Pin control */
> +#define RTL8231_REG_PIN_MODE0		0x02
> +#define RTL8231_REG_PIN_MODE1		0x03
> +
> +#define RTL8231_PIN_MODE_LED		0
> +#define RTL8231_PIN_MODE_GPIO		1
> +
> +/* Pin high config: pin and GPIO control for pins 32-26 */
> +#define RTL8231_REG_PIN_HI_CFG		0x04
> +#define RTL8231_PIN_HI_CFG_MODE_MASK	GENMASK(4, 0)
> +#define RTL8231_PIN_HI_CFG_DIR_MASK	GENMASK(9, 5)
> +#define RTL8231_PIN_HI_CFG_INV_MASK	GENMASK(14, 10)
> +#define RTL8231_PIN_HI_CFG_SOFT_RESET	BIT(15)
> +
> +/* GPIO control registers */
> +#define RTL8231_REG_GPIO_DIR0		0x05
> +#define RTL8231_REG_GPIO_DIR1		0x06
> +#define RTL8231_REG_GPIO_INVERT0	0x07
> +#define RTL8231_REG_GPIO_INVERT1	0x08
> +
> +#define RTL8231_GPIO_DIR_IN		1
> +#define RTL8231_GPIO_DIR_OUT		0
> +
> +/*
> + * GPIO data registers
> + * Only the output data can be written to these registers, and only the input
> + * data can be read.
> + */
> +#define RTL8231_REG_GPIO_DATA0		0x1c
> +#define RTL8231_REG_GPIO_DATA1		0x1d
> +#define RTL8231_REG_GPIO_DATA2		0x1e
> +#define RTL8231_PIN_HI_DATA_MASK	GENMASK(4, 0)
> +
> +/* LED control base registers */
> +#define RTL8231_REG_LED0_BASE		0x09
> +#define RTL8231_REG_LED1_BASE		0x10
> +#define RTL8231_REG_LED2_BASE		0x17
> +#define RTL8231_REG_LED_END		0x1b
> +
> +#define RTL8231_REG_COUNT		0x1f
> +
> +#endif /* __LINUX_MFD_RTL8231_H */
> -- 
> 2.51.0
> 
> 

-- 
Lee Jones [李琼斯]

Powered by blists - more mailing lists