Message-ID: <38c9b9e77029f894dd186305a275231aaa502664.camel@svanheule.net>
Date: Wed, 26 Nov 2025 20:54:44 +0100
From: Sander Vanheule <sander@...nheule.net>
To: Lee Jones <lee@...nel.org>
Cc: Pavel Machek <pavel@...nel.org>, Rob Herring <robh@...nel.org>, 
 Krzysztof Kozlowski	 <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Linus Walleij	 <linus.walleij@...aro.org>, Michael
 Walle <mwalle@...nel.org>, Bartosz Golaszewski <brgl@...ev.pl>,
 linux-leds@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org
Subject: Re: [PATCH v8 3/6] mfd: Add RTL8231 core device

Hi Lee,

On Wed, 2025-11-26 at 13:04 +0000, Lee Jones wrote:
> On Wed, 19 Nov 2025, Sander Vanheule wrote:
> > --- /dev/null
> > +++ b/drivers/mfd/rtl8231.c
> > @@ -0,0 +1,193 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> 
> No company copyright or author tags?
> 

I'm writing this driver on my own time, so there's no company. My name is at the
bottom of the driver, in the MODULE_AUTHOR() macro. That's enough for me.


> > +static int rtl8231_soft_reset(struct regmap *regmap)
> > +{
> > +	const unsigned int all_pins_mask = GENMASK(RTL8231_BITS_VAL - 1,
> > 0);
> > +	unsigned int cfg;
> > +	int err;
> > +
> > +	/* SOFT_RESET bit self-clears when done */
> > +	regmap_write_bits(regmap, RTL8231_REG_PIN_HI_CFG,
> > +			  RTL8231_PIN_HI_CFG_SOFT_RESET,
> > RTL8231_PIN_HI_CFG_SOFT_RESET);
> > +
> > +	err = regmap_read_poll_timeout(regmap, RTL8231_REG_PIN_HI_CFG, cfg,
> > +				       !(cfg &
> > RTL8231_PIN_HI_CFG_SOFT_RESET), 50, 1000);
> > +	if (err)
> > +		return err;
> > +
> > +	regcache_drop_region(regmap, 0, RTL8231_REG_COUNT - 1);
> 
> Nit:
> 
> RTL8231_REG_FUNC0, RTL8231_REG_GPIO_DATA2
> 
> I'll also accept additional devices for each of these which would make
> things very clear.
> 
> RTL8231_REG_START	0
> RTL8231_REG_END		RTL8231_REG_COUNT - 1
> 
> Or similar.

I went with:

#define RTL8231_REG_START	0x00
#define RTL8231_REG_END		0x1e

I will also use RTL8231_REG_END for the regmap config's max_register, so
RTL8231_REG_COUNT can even be dropped.


> > +static int rtl8231_init(struct device *dev, struct regmap *regmap)
> > +{
> > +	struct regmap_field *led_start;
> > +	unsigned int ready_code;
> > +	unsigned int started;
> > +	unsigned int status;
> > +	int err;
> > +
> > +	err = regmap_read(regmap, RTL8231_REG_FUNC1, &status);
> > +	if (err) {
> > +		dev_err(dev, "failed to read READY_CODE\n");
> > +		return err;
> > +	}
> > +
> > +	ready_code = FIELD_GET(RTL8231_FUNC1_READY_CODE_MASK, status);
> > +	if (ready_code != RTL8231_FUNC1_READY_CODE_VALUE) {
> > +		dev_err(dev, "RTL8231 not present or ready 0x%x != 0x%x\n",
> > +			ready_code, 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)
> > +		return 0;
> > +
> > +	err = rtl8231_soft_reset(regmap);
> > +	if (err)
> > +		return err;
> > +
> > +	/* LED_START enables power to output pins, and starts the LED
> > engine */
> > +	return regmap_field_write(led_start, 1);
> 
> Why can't the dedicated LED driver initialise itself?

Perhaps I didn't explain this clearly enough in my previous message, but the
"led_start" bit must also be set if all pins are configured for use as GPIO and
the LED driver isn't even loaded in the kernel. Otherwise the output drivers of
the pins remain disabled.

I can rename "led_start" to "output_enable" if that would make things clearer.

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

Changed to RTL8231_REG_END.

> > +	regmap = devm_regmap_init_mdio(mdiodev,
> > &rtl8231_mdio_regmap_config);
> > +	if (IS_ERR(regmap)) {
> > +		dev_err(dev, "failed to init regmap\n");
> 
> Nit: Let's not shorten user messages - "initialise".

Ack.


> > +		return PTR_ERR(regmap);
> > +	}
> > +
> > +	led_start = devm_regmap_field_alloc(dev, regmap, field_led_start);
> > +	if (IS_ERR(led_start))
> > +		return PTR_ERR(led_start);
> > +
> > +	dev_set_drvdata(dev, led_start);
> 
> I'd pass the whole Regmap through and let the LED extract its own part.
> 
> > +	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, regmap);
> 
> ... then you can omit the 'regmap' argument.

Sure, that's one less allocation too. The field writes become a bit more
verbose, but it's not too unwieldy.


> > +	if (err)
> > +		return err;
> > +
> > +	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
> > rtl8231_cells,
> > +				    ARRAY_SIZE(rtl8231_cells), NULL, 0,
> > NULL);
> > +}
> > +
> > +static int __maybe_unused rtl8231_suspend(struct device *dev)

I've learned the __maybe_unused is (no longer) required, so I dropped these.

> > +{
> > +	struct regmap_field *led_start = dev_get_drvdata(dev);
> > +
> > +	return regmap_field_write(led_start, 0);
> 
> The LED driver doesn't have its own suspend support?

Because this affects both the GPIO outputs and the LED scanning feature, this
does belong in the core driver IMHO.

> 
> > +}
> > +
> > +static int __maybe_unused rtl8231_resume(struct device *dev)
> > +{
> > +	struct regmap_field *led_start = dev_get_drvdata(dev);
> > +
> > +	return regmap_field_write(led_start, 1);
> > +}
> > +
> 
> Nit: Remove this line.

You mean the blank line above "SIMPLE_DEV_PM_OPS"? AFAICT this is standard style
for this kind of statement. It feels "ugly" to me to glue the PM_OPS macro to
the resume function, while the suspend function is separated from it.

> 
Best,
Sander

Powered by blists - more mailing lists