| Message ID | 20240828184018.3097386-4-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: intel: High impedance impl. and cleanups | expand |
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
On Thu, Aug 29, 2024 at 07:46:53AM +0300, Mika Westerberg wrote: > On Wed, Aug 28, 2024 at 09:38:36PM +0300, Andy Shevchenko wrote: ... > > + * enum - possible pad physical configurations > > + * > > Start with capital letter as others: > > enum - Possible.. Fixed! > 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. We have the other enum with a blank line. Perhaps removing them in a separate patch?
On Thu, Aug 29, 2024 at 01:49:38PM +0300, Andy Shevchenko wrote: > On Thu, Aug 29, 2024 at 07:46:53AM +0300, Mika Westerberg wrote: > > On Wed, Aug 28, 2024 at 09:38:36PM +0300, Andy Shevchenko wrote: ... > > > + * enum - possible pad physical configurations > > > + * > > > > Start with capital letter as others: > > > > enum - Possible.. > > Fixed! > > > 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. > > We have the other enum with a blank line. Perhaps removing them in a separate > patch? Or I can do it "while at it", it seems to be quite small and likely never conflicting change...
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 + * + * @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); }
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(-)