linux-kernel - Re: [PATCH v3 1/3] gpio: gpio-regmap: add flags to control some behaviour

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: <20250821143220.GA672670@bhelgaas>
Date: Thu, 21 Aug 2025 09:32:20 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Marcos Del Sol Vives <marcos@...a.pet>
Cc: linux-kernel@...r.kernel.org, Linus Walleij <linus.walleij@...aro.org>,
	Bartosz Golaszewski <brgl@...ev.pl>,
	Michael Walle <mwalle@...nel.org>, Lee Jones <lee@...nel.org>,
	Bjorn Helgaas <bhelgaas@...gle.com>, linux-gpio@...r.kernel.org,
	linux-pci@...r.kernel.org
Subject: Re: [PATCH v3 1/3] gpio: gpio-regmap: add flags to control some
 behaviour

Not my area, but consider making the subject more specific, e.g.,
"add flag to set direction before value"

On Thu, Aug 21, 2025 at 12:18:57PM +0200, Marcos Del Sol Vives wrote:
> The Vortex86 family of SoCs need the direction set before the value, else
> writes to the DATA ports are ignored.
> 
> This commit adds a new "flags" field plus a flag to change the default
> behaviour, which is to set first the direction and then the value.

This sounds like the default behavior is to set direction, then value.
But from the patch, it looks like:

  - default: set value, then direction

  - with GPIO_REGMAP_DIR_BEFORE_SET: set direction, then value

> Signed-off-by: Marcos Del Sol Vives <marcos@...a.pet>
> ---
>  drivers/gpio/gpio-regmap.c  | 17 ++++++++++++++++-
>  include/linux/gpio/regmap.h | 17 +++++++++++++++++
>  2 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> index e8a32dfebdcb..24cefbd57637 100644
> --- a/drivers/gpio/gpio-regmap.c
> +++ b/drivers/gpio/gpio-regmap.c
> @@ -31,6 +31,7 @@ struct gpio_regmap {
>  	unsigned int reg_clr_base;
>  	unsigned int reg_dir_in_base;
>  	unsigned int reg_dir_out_base;
> +	unsigned int flags;
>  
>  	int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base,
>  			      unsigned int offset, unsigned int *reg,
> @@ -196,7 +197,20 @@ static int gpio_regmap_direction_input(struct gpio_chip *chip,
>  static int gpio_regmap_direction_output(struct gpio_chip *chip,
>  					unsigned int offset, int value)
>  {
> -	gpio_regmap_set(chip, offset, value);
> +	struct gpio_regmap *gpio = gpiochip_get_data(chip);
> +	int ret;
> +
> +	if (gpio->flags & GPIO_REGMAP_DIR_BEFORE_SET) {
> +		ret = gpio_regmap_set_direction(chip, offset, true);
> +		if (ret)
> +			return ret;
> +
> +		return gpio_regmap_set(chip, offset, value);
> +	}
> +
> +	ret = gpio_regmap_set(chip, offset, value);
> +	if (ret)
> +		return ret;
>  
>  	return gpio_regmap_set_direction(chip, offset, true);
>  }
> @@ -247,6 +261,7 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
>  	gpio->reg_clr_base = config->reg_clr_base;
>  	gpio->reg_dir_in_base = config->reg_dir_in_base;
>  	gpio->reg_dir_out_base = config->reg_dir_out_base;
> +	gpio->flags = config->flags;
>  
>  	chip = &gpio->gpio_chip;
>  	chip->parent = config->parent;
> diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
> index c722c67668c6..aea107e71fec 100644
> --- a/include/linux/gpio/regmap.h
> +++ b/include/linux/gpio/regmap.h
> @@ -12,6 +12,20 @@ struct regmap;
>  #define GPIO_REGMAP_ADDR_ZERO ((unsigned int)(-1))
>  #define GPIO_REGMAP_ADDR(addr) ((addr) ? : GPIO_REGMAP_ADDR_ZERO)
>  
> +
> +/**
> + * enum gpio_regmap_flags - flags to control GPIO operation
> + */
> +enum gpio_regmap_flags {
> +	/**
> +	 * @GPIO_REGMAP_DIR_BEFORE_SET: when setting a pin as an output, set
> +	 * its direction before the value. The output value will be undefined
> +	 * for a short time which may have unwanted side effects, but some
> +	 * hardware requires this.
> +	 */
> +	GPIO_REGMAP_DIR_BEFORE_SET	= BIT(0),
> +};
> +
>  /**
>   * struct gpio_regmap_config - Description of a generic regmap gpio_chip.
>   * @parent:		The parent device
> @@ -23,6 +37,8 @@ struct regmap;
>   *			If not given, the name of the device is used.
>   * @ngpio:		(Optional) Number of GPIOs
>   * @names:		(Optional) Array of names for gpios
> + * @flags:		(Optional) A bitmask of flags from
> + * 			&enum gpio_regmap_flags
>   * @reg_dat_base:	(Optional) (in) register base address
>   * @reg_set_base:	(Optional) set register base address
>   * @reg_clr_base:	(Optional) clear register base address
> @@ -68,6 +84,7 @@ struct gpio_regmap_config {
>  	const char *label;
>  	int ngpio;
>  	const char *const *names;
> +	unsigned int flags;
>  
>  	unsigned int reg_dat_base;
>  	unsigned int reg_set_base;
> -- 
> 2.34.1
>