Re: [PATCH v6 5/8] mfd: Add RTL8231 core device

[Sander Vanheule <sander@svanheule.net>]

Hi Lee,

On Thu, 2025-11-06 at 16:33 +0000, Lee Jones wrote:
> On Tue, 21 Oct 2025, Sander Vanheule wrote:
> > @@ -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.

Ack

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

Moving it to the one place where it's used.

> > +static const struct mfd_cell rtl8231_cells[] = {
> > +	{
> > +		.name = "rtl8231-pinctrl",
> > +	},
> > +	{
> > +		.name = "rtl8231-leds",
> > +		.of_compatible = "realtek,rtl8231-leds",
> > +	},
> > +};
> 
> This is a pretty tenuous MFD!

I suppose two functions is the minimum to count as multi-functional :-)

I need a place to perform chip detection and to start the output drivers. The
latter could be part of a pinctrl driver, but making the led driver depend on
the pinctrl driver doesn't sit too well with me. Now the functionality is
cleanly split across the MFD core driver and pinctrl/led drivers.

> > +static int rtl8231_soft_reset(struct regmap *map)
> 
> s/map/regmap/

Ack, replaced throughout the driver.

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

Made it a bit less claustrophobic.

> > +	regcache_mark_dirty(map);

This marks the device register state as invalid, which isn't what I wanted here.
After a reset, the cache should be repopulated, so this is now:

	regcache_drop_region(0, RTL8231_REG_COUNT - 1);

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

Ack, updated the naming a bit.

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

Ack

> 
> > +		err = rtl8231_soft_reset(map);
> > +		if (err)
> > +			return err;
> 
> '\n' here.

Ack

> > +		/* LED_START enables power to output pins, and starts the
> > LED engine */
> > +		err = regmap_field_force_write(led_start, 1);

This is in a volatile register, so the driver doesn't need to force anything to
write to the device and will now use regmap_field_write().

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

The is naming taken from the datasheet. Setting LED_START actually enables both
the LED scanning output engine and the output drivers. So this is needed for
proper functioning of the GPIO functionality too.


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

Ack

> > +__maybe_unused static int rtl8231_suspend(struct device *dev)
> 
> The __maybe_unused comes after the "static int" part.

Ack.

C11 attributes like [[maybe_unused]] must come before the "static int" part, but
that doesn't line up with how most of the kernel uses the attributes.


Best,
Sander