Re: [PATCH v1] pinctrl: intel: Allow to request locked pins

[Mika Westerberg <mika.westerberg@linux.intel.com>]

On Fri, Jul 26, 2019 at 11:08:30PM +0300, Andy Shevchenko wrote:
> Some firmwares would like to protect pins from being modified by OS
> and at the same time provide them to OS as a resource. So, the driver
> in such circumstances may request pin and may not change its state.

This is definitely good idea.

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 55 +++++++++++++++++++--------
>  1 file changed, 39 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index 3a945997b8eb..567fe43b238f 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -220,22 +220,30 @@ static bool intel_pad_acpi_mode(struct intel_pinctrl *pctrl, unsigned int pin)
>  	return !(readl(hostown) & BIT(gpp_offset));
>  }
>  
> -static bool intel_pad_locked(struct intel_pinctrl *pctrl, unsigned int pin)
> +enum {
> +	PAD_UNLOCKED	= 0,
> +	PAD_LOCKED	= 1,
> +	PAD_LOCKED_TX	= 2,
> +	PAD_LOCKED_FULL	= PAD_LOCKED | PAD_LOCKED_TX,
> +};
> +
> +static int intel_pad_locked(struct intel_pinctrl *pctrl, unsigned int pin)
>  {
>  	struct intel_community *community;
>  	const struct intel_padgroup *padgrp;
>  	unsigned int offset, gpp_offset;
>  	u32 value;
> +	int ret = PAD_UNLOCKED;
>  
>  	community = intel_get_community(pctrl, pin);
>  	if (!community)
> -		return true;
> +		return PAD_LOCKED_FULL;
>  	if (!community->padcfglock_offset)
> -		return false;
> +		return PAD_UNLOCKED;
>  
>  	padgrp = intel_community_get_padgroup(community, pin);
>  	if (!padgrp)
> -		return true;
> +		return PAD_LOCKED_FULL;
>  
>  	gpp_offset = padgroup_offset(padgrp, pin);
>  
> @@ -244,23 +252,27 @@ static bool intel_pad_locked(struct intel_pinctrl *pctrl, unsigned int pin)
>  	 * the pad is considered unlocked. Any other case means that it is
>  	 * either fully or partially locked and we don't touch it.

I think you should update the above comment as well.

>  	 */
> -	offset = community->padcfglock_offset + padgrp->reg_num * 8;
> +	offset = community->padcfglock_offset + 0 + padgrp->reg_num * 8;
>  	value = readl(community->regs + offset);
>  	if (value & BIT(gpp_offset))
> -		return true;
> +		ret |= PAD_LOCKED;
>  
>  	offset = community->padcfglock_offset + 4 + padgrp->reg_num * 8;
>  	value = readl(community->regs + offset);
>  	if (value & BIT(gpp_offset))
> -		return true;
> +		ret |= PAD_LOCKED_TX;
>  
> -	return false;
> +	return ret;
> +}
> +
> +static bool intel_pad_is_unlocked(struct intel_pinctrl *pctrl, unsigned int pin)
> +{
> +	return (intel_pad_locked(pctrl, pin) & PAD_LOCKED) == PAD_UNLOCKED;
>  }
>  
>  static bool intel_pad_usable(struct intel_pinctrl *pctrl, unsigned int pin)
>  {
> -	return intel_pad_owned_by_host(pctrl, pin) &&
> -		!intel_pad_locked(pctrl, pin);
> +	return intel_pad_owned_by_host(pctrl, pin) && intel_pad_is_unlocked(pctrl, pin);
>  }
>  
>  static int intel_get_groups_count(struct pinctrl_dev *pctldev)
> @@ -294,7 +306,8 @@ static void intel_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
>  	struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
>  	void __iomem *padcfg;
>  	u32 cfg0, cfg1, mode;
> -	bool locked, acpi;
> +	int locked;
> +	bool acpi;
>  
>  	if (!intel_pad_owned_by_host(pctrl, pin)) {
>  		seq_puts(s, "not available");
> @@ -322,11 +335,16 @@ static void intel_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
>  
>  	if (locked || acpi) {
>  		seq_puts(s, " [");
> -		if (locked) {
> +		if (locked)
>  			seq_puts(s, "LOCKED");
> -			if (acpi)
> -				seq_puts(s, ", ");
> -		}
> +		if ((locked & PAD_LOCKED_FULL) == PAD_LOCKED_TX)
> +			seq_puts(s, " TX");
> +		else if ((locked & PAD_LOCKED_FULL) == PAD_LOCKED_FULL)
> +			seq_puts(s, " FULL");
> +
> +		if (locked && acpi)
> +			seq_puts(s, ", ");
> +
>  		if (acpi)
>  			seq_puts(s, "ACPI");
>  		seq_puts(s, "]");
> @@ -448,11 +466,16 @@ static int intel_gpio_request_enable(struct pinctrl_dev *pctldev,
>  
>  	raw_spin_lock_irqsave(&pctrl->lock, flags);
>  
> -	if (!intel_pad_usable(pctrl, pin)) {
> +	if (!intel_pad_owned_by_host(pctrl, pin)) {
>  		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>  		return -EBUSY;
>  	}
>  
> +	if (!intel_pad_is_unlocked(pctrl, pin)) {
> +		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> +		return 0;

Hmm, if I'm reading this right it still does not allow requesting locked
pins. What I'm missing here?

> +	}
> +
>  	padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0);
>  	intel_gpio_set_gpio_mode(padcfg0);
>  	/* Disable TX buffer and enable RX (this will be input) */
> -- 
> 2.20.1