Re: [PATCH v9 4/6] pinctrl: Add RTL8231 pin control and GPIO support

[Bartosz Golaszewski]

On Mon, 15 Dec 2025 18:51:12 +0100, Sander Vanheule <sander@xxxxxxxxxxxxx> said:
> 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@xxxxxxxxxxxxx>
> Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---

[snip]

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

Please put this together with other global headers.

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

Newline?

> +static const unsigned int PWM_PIN = 35;

Please use the RTL8231 prefix for all symbols.

> +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);
> +	enum rtl8231_pin_function func_flag = (uintptr_t) func->data;
> +	unsigned int function_mask;
> +	unsigned int gpio_function;

Can you put these on the same line here and elsewhere?

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

Just drop the else.

> +		/*
> +		 * Set every pin that is not muxed as a GPIO to gpio-in. That
> +		 * way the pin will be high impedance when it is muxed to GPIO,
> +		 * preventing unwanted glitches.
> +		 * The pin muxes are left as-is, so there are no signal changes.
> +		 */
> +		regmap_field_write(field_dir, is_input | ~is_gpio);

This is an MDIO regmap. The operations may fail. Don't you want to check the
return values of regmap routines?

> +
> +static int rtl8231_pinctrl_init(struct device *dev, struct rtl8231_pin_ctrl *ctrl)
> +{
> +	struct pinctrl_dev *pctldev;
> +	int err;
> +
> +	err = devm_pinctrl_register_and_init(dev->parent, &rtl8231_pctl_desc, ctrl, &pctldev);
> +	if (err) {
> +		dev_err(dev, "failed to register pin controller\n");
> +		return err;

Please use dev_err_probe() here an elsewhere.

> +	}
> +
> +	err = rtl8231_pinctrl_init_functions(pctldev, &rtl8231_pctl_desc);
> +	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)

Can you align the start of the line with the opening bracket?

Bart