Re: [PATCH v1 4/5] pinctrl: intel: Implement high impedance support

Mika Westerberg
2024-08-29T04:48:22Z

On Wed, Aug 28, 2024 at 09:38:37PM +0300, Andy Shevchenko wrote:
> Implement high impedance support for Intel pin control hardware.
> It allows to set high impedance and check it.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> drivers/pinctrl/intel/pinctrl-intel.c | 46 +++++++++++++++++++++++++++
> 1 file changed, 46 insertions(+)
>
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index 3a135cfe435f..ae30969b2dee 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -78,6 +78,7 @@
> #define PADCFG0_GPIODIS_FULL 3
> #define PADCFG0_GPIORXDIS BIT(9)
> #define PADCFG0_GPIOTXDIS BIT(8)
> +#define PADCFG0_GPIODIS (BIT(9) | BIT(8))
> #define PADCFG0_GPIORXSTATE BIT(1)
> #define PADCFG0_GPIOTXSTATE BIT(0)
>
> @@ -654,6 +655,23 @@ static int intel_config_get_pull(struct intel_pinctrl *pctrl, unsigned int pin,
> return 0;
> }
>
> +static int intel_config_get_high_impedance(struct intel_pinctrl *pctrl, unsigned int pin,
> + enum pin_config_param param, u32 *arg)
> +{
> + void __iomem *padcfg0;
> + u32 value;
> +
> + padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0);
> +
> + scoped_guard(raw_spinlock_irqsave, &pctrl->lock)
> + value = readl(padcfg0);
> +
> + if (__intel_gpio_get_direction(value) != PAD_CONNECT_NONE)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> static int intel_config_get_debounce(struct intel_pinctrl *pctrl, unsigned int pin,
> enum pin_config_param param, u32 *arg)
> {
> @@ -697,6 +715,12 @@ static int intel_config_get(struct pinctrl_dev *pctldev, unsigned int pin,
> return ret;
> break;
>
> + case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
> + ret = intel_config_get_high_impedance(pctrl, pin, param, &arg);
> + if (ret)
> + return ret;
> + break;
> +
> case PIN_CONFIG_INPUT_DEBOUNCE:
> ret = intel_config_get_debounce(pctrl, pin, param, &arg);
> if (ret)
> @@ -795,6 +819,22 @@ static int intel_config_set_pull(struct intel_pinctrl *pctrl, unsigned int pin,
> return 0;
> }
>
> +static int intel_gpio_set_high_impedance(struct intel_pinctrl *pctrl, unsigned int pin)
> +{
> + void __iomem *padcfg0;
> + u32 value;
> +
> + padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0);
> +
> + guard(raw_spinlock_irqsave)(&pctrl->lock);
> +
> + value = readl(padcfg0);
> + value = __intel_gpio_set_direction(value, false, false);
> + writel(value, padcfg0);
> +
> + return 0;

Why not make this return void?

> +}
> +
> static int intel_config_set_debounce(struct intel_pinctrl *pctrl,
> unsigned int pin, unsigned int debounce)
> {
> @@ -857,6 +897,12 @@ static int intel_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
> return ret;
> break;
>
> + case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
> + ret = intel_gpio_set_high_impedance(pctrl, pin);
> + if (ret)
> + return ret;

Then this becomes simpler too.

> + break;
> +
> case PIN_CONFIG_INPUT_DEBOUNCE:
> ret = intel_config_set_debounce(pctrl, pin,
> pinconf_to_config_argument(configs[i]));
> --
> 2.43.0.rc1.1336.g36b5255a03ac