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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a8063a3a5eb8055bf88976f9da07175528502144.camel@svanheule.net>
Date: Fri, 19 Dec 2025 14:52:08 +0100
From: Sander Vanheule <sander@...nheule.net>
To: Bartosz Golaszewski <brgl@...nel.org>
Cc: Lee Jones <lee@...nel.org>, 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 v9 4/6] pinctrl: Add RTL8231 pin control and GPIO support

Hi Bartosz,

On Thu, 2025-12-18 at 01:15 -0800, Bartosz Golaszewski wrote:
> On Mon, 15 Dec 2025 18:51:12 +0100, Sander Vanheule <sander@...nheule.net>
> 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@...nheule.net>
> > Reviewed-by: Linus Walleij <linus.walleij@...aro.org>
> > ---
> 
> [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.

Updated here and in the other driver files.

> > +static const unsigned int PWM_PIN = 35;
> 
> Please use the RTL8231 prefix for all symbols.

Added the prefix here and also for a struct defined in this file (rtl8231_...).

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

I am aware this is often done, and also mentioned in the kernel docs [1], but I
actually prefer this style and have always used it in my kernel patches.
IMHO placing these on separate lines helps with:
 * readability: every line is either [type] [name] or [type] [name] = [value]
 * reducing churn: changing the type of one variable doesn't impact other lines
 * quickly assessing code complexity: number of local variables is simply the
   number of lines

Editors aren't typically 80x24 anymore. The maximum line length has been relaxed
to 100 characters, so I feel like a developer can also be expected to have more
lines available on their display nowadays.

[1]
https://docs.kernel.org/6.18/process/maintainer-tip.html#variable-declarations

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

Error checking was indeed not done consistently, so I've now added checks to all
regmap_*() calls that return an error code. Also updated in the other drivers.

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

Updated here and in the other drivers.

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

Will do.

Best,
Sander