linux-kernel - Re: [PATCH v1 3/5] pinctrl: intel: Add __intel_gpio_get

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: <20240829044653.GR1532424@black.fi.intel.com>
Date: Thu, 29 Aug 2024 07:46:53 +0300
From: Mika Westerberg <mika.westerberg@...ux.intel.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
	Andy Shevchenko <andy@...nel.org>,
	Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [PATCH v1 3/5] pinctrl: intel: Add __intel_gpio_get_direction()
 helper

On Wed, Aug 28, 2024 at 09:38:36PM +0300, Andy Shevchenko wrote:
> Add __intel_gpio_get_direction() helper which provides all possible
> physical states of the pad.
> 
> With that done, update current users and make the respective checks
> consistent.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 48 ++++++++++++++++++++++++---
>  1 file changed, 43 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index 2a3d44f35348..3a135cfe435f 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -70,6 +70,12 @@
>  #define PADCFG0_PMODE_SHIFT		10
>  #define PADCFG0_PMODE_MASK		GENMASK(13, 10)
>  #define PADCFG0_PMODE_GPIO		0
> +#define PADCFG0_GPIODIS_SHIFT		8
> +#define PADCFG0_GPIODIS_MASK		GENMASK(9, 8)
> +#define PADCFG0_GPIODIS_NONE		0
> +#define PADCFG0_GPIODIS_OUTPUT		1
> +#define PADCFG0_GPIODIS_INPUT		2
> +#define PADCFG0_GPIODIS_FULL		3
>  #define PADCFG0_GPIORXDIS		BIT(9)
>  #define PADCFG0_GPIOTXDIS		BIT(8)
>  #define PADCFG0_GPIORXSTATE		BIT(1)
> @@ -429,6 +435,37 @@ static int intel_pinmux_set_mux(struct pinctrl_dev *pctldev,
>  	return 0;
>  }
>  
> +/**
> + * enum - possible pad physical configurations
> + *

Start with capital letter as others:

enum - Possible..

Also I think we should follow the structs and drop the empty line here
(well and for other enums, I don't know how they got there ;-) but it
looks better without.

Other than this, looks good to me.

> + * @PAD_CONNECT_NONE:	pad is fully disconnected
> + * @PAD_CONNECT_INPUT:	pad is in input only mode
> + * @PAD_CONNECT_OUTPUT:	pad is in output only mode
> + * @PAD_CONNECT_FULL:	pad is fully connected
> + */
> +enum {
> +	PAD_CONNECT_NONE	= 0,
> +	PAD_CONNECT_INPUT	= 1,
> +	PAD_CONNECT_OUTPUT	= 2,
> +	PAD_CONNECT_FULL	= PAD_CONNECT_INPUT | PAD_CONNECT_OUTPUT,
> +};
> +
> +static int __intel_gpio_get_direction(u32 value)
> +{
> +	switch ((value & PADCFG0_GPIODIS_MASK) >> PADCFG0_GPIODIS_SHIFT) {
> +	case PADCFG0_GPIODIS_FULL:
> +		return PAD_CONNECT_NONE;
> +	case PADCFG0_GPIODIS_OUTPUT:
> +		return PAD_CONNECT_INPUT;
> +	case PADCFG0_GPIODIS_INPUT:
> +		return PAD_CONNECT_OUTPUT;
> +	case PADCFG0_GPIODIS_NONE:
> +		return PAD_CONNECT_FULL;
> +	default:
> +		return PAD_CONNECT_FULL;
> +	};
> +}
> +
>  static u32 __intel_gpio_set_direction(u32 value, bool input, bool output)
>  {
>  	if (input)
> @@ -937,7 +974,7 @@ static int intel_gpio_get(struct gpio_chip *chip, unsigned int offset)
>  		return -EINVAL;
>  
>  	padcfg0 = readl(reg);
> -	if (!(padcfg0 & PADCFG0_GPIOTXDIS))
> +	if (__intel_gpio_get_direction(padcfg0) & PAD_CONNECT_OUTPUT)
>  		return !!(padcfg0 & PADCFG0_GPIOTXSTATE);
>  
>  	return !!(padcfg0 & PADCFG0_GPIORXSTATE);
> @@ -990,10 +1027,10 @@ static int intel_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
>  	if (padcfg0 & PADCFG0_PMODE_MASK)
>  		return -EINVAL;
>  
> -	if (padcfg0 & PADCFG0_GPIOTXDIS)
> -		return GPIO_LINE_DIRECTION_IN;
> +	if (__intel_gpio_get_direction(padcfg0) & PAD_CONNECT_OUTPUT)
> +		return GPIO_LINE_DIRECTION_OUT;
>  
> -	return GPIO_LINE_DIRECTION_OUT;
> +	return GPIO_LINE_DIRECTION_IN;
>  }
>  
>  static int intel_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
> @@ -1690,7 +1727,8 @@ EXPORT_SYMBOL_NS_GPL(intel_pinctrl_get_soc_data, PINCTRL_INTEL);
>  
>  static bool __intel_gpio_is_direct_irq(u32 value)
>  {
> -	return (value & PADCFG0_GPIROUTIOXAPIC) && (value & PADCFG0_GPIOTXDIS) &&
> +	return (value & PADCFG0_GPIROUTIOXAPIC) &&
> +	       (__intel_gpio_get_direction(value) == PAD_CONNECT_INPUT) &&
>  	       (__intel_gpio_get_gpio_mode(value) == PADCFG0_PMODE_GPIO);
>  }
>  
> -- 
> 2.43.0.rc1.1336.g36b5255a03ac