Re: [PATCH v1 3/5] pinctrl: intel: Add __intel_gpio_get_direction() helper

Mika Westerberg
2024-08-29T04:46:56Z

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@linux.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