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

[Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxx>
> ---
>  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