[PATCH] pinctrl: intel: Implement intel_gpio_get_direction callback

[PATCH] pinctrl: intel: Implement intel_gpio_get_direction callback

[Javier Arteaga]

Allows querying GPIO direction from the pad config register.
If the pad is not in GPIO mode, return an error.

Signed-off-by: Javier Arteaga <javier@emutex.com>
---
This is needed by the drivers for the UP Squared board, an APL-based
platform. (For now, these drivers are out-of-tree.)

An earlier version of this patch was reviewed some time ago by Andy
Shevchenko outside of the mailing lists:
<https://github.com/emutex/ubilinux-kernel/commit/708a776f2ffd1b1c>

 drivers/pinctrl/intel/pinctrl-intel.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 96e73e30204e..1e24a6b8a64e 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -788,6 +788,24 @@ static void intel_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 }
 
+static int intel_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+	struct intel_pinctrl *pctrl = gpiochip_get_data(chip);
+	void __iomem *reg;
+	u32 padcfg0;
+
+	reg = intel_get_padcfg(pctrl, offset, PADCFG0);
+	if (!reg)
+		return -EINVAL;
+
+	padcfg0 = readl(reg);
+
+	if (padcfg0 & PADCFG0_PMODE_MASK)
+		return -EINVAL;
+
+	return !!(padcfg0 & PADCFG0_GPIOTXDIS);
+}
+
 static int intel_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 {
 	return pinctrl_gpio_direction_input(chip->base + offset);
@@ -804,6 +822,7 @@ static const struct gpio_chip intel_gpio_chip = {
 	.owner = THIS_MODULE,
 	.request = gpiochip_generic_request,
 	.free = gpiochip_generic_free,
+	.get_direction = intel_gpio_get_direction,
 	.direction_input = intel_gpio_direction_input,
 	.direction_output = intel_gpio_direction_output,
 	.get = intel_gpio_get,
-- 
2.16.2

Re: [PATCH] pinctrl: intel: Implement intel_gpio_get_direction callback

[Javier Arteaga]

(Fix Andy's Cc: - apologies)

On 06/03/2018 13:42, Javier Arteaga wrote:
> Allows querying GPIO direction from the pad config register.
> If the pad is not in GPIO mode, return an error.
> 
> Signed-off-by: Javier Arteaga <javier@emutex.com>
> ---
> This is needed by the drivers for the UP Squared board, an APL-based
> platform. (For now, these drivers are out-of-tree.)
> 
> An earlier version of this patch was reviewed some time ago by Andy
> Shevchenko outside of the mailing lists:
> <https://github.com/emutex/ubilinux-kernel/commit/708a776f2ffd1b1c>
> 
>  drivers/pinctrl/intel/pinctrl-intel.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index 96e73e30204e..1e24a6b8a64e 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -788,6 +788,24 @@ static void intel_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
>  	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
>  }
>  
> +static int intel_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct intel_pinctrl *pctrl = gpiochip_get_data(chip);
> +	void __iomem *reg;
> +	u32 padcfg0;
> +
> +	reg = intel_get_padcfg(pctrl, offset, PADCFG0);
> +	if (!reg)
> +		return -EINVAL;
> +
> +	padcfg0 = readl(reg);
> +
> +	if (padcfg0 & PADCFG0_PMODE_MASK)
> +		return -EINVAL;
> +
> +	return !!(padcfg0 & PADCFG0_GPIOTXDIS);
> +}
> +
>  static int intel_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
>  {
>  	return pinctrl_gpio_direction_input(chip->base + offset);
> @@ -804,6 +822,7 @@ static const struct gpio_chip intel_gpio_chip = {
>  	.owner = THIS_MODULE,
>  	.request = gpiochip_generic_request,
>  	.free = gpiochip_generic_free,
> +	.get_direction = intel_gpio_get_direction,
>  	.direction_input = intel_gpio_direction_input,
>  	.direction_output = intel_gpio_direction_output,
>  	.get = intel_gpio_get,
> 

Re: [PATCH] pinctrl: intel: Implement intel_gpio_get_direction callback

[Andy Shevchenko]

On Tue, 2018-03-06 at 14:31 +0000, Javier Arteaga wrote:
> (Fix Andy's Cc: - apologies)
> 
> On 06/03/2018 13:42, Javier Arteaga wrote:
> > Allows querying GPIO direction from the pad config register.
> > If the pad is not in GPIO mode, return an error.
> > 
> > Signed-off-by: Javier Arteaga <javier@emutex.com>
> > ---
> > This is needed by the drivers for the UP Squared board, an APL-based
> > platform. (For now, these drivers are out-of-tree.)
> > 
> > An earlier version of this patch was reviewed some time ago by Andy
> > Shevchenko outside of the mailing lists:
> > <https://github.com/emutex/ubilinux-kernel/commit/708a776f2ffd1b1c>
> > 

Yes, this one look good!

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> >  drivers/pinctrl/intel/pinctrl-intel.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> > 
> > diff --git a/drivers/pinctrl/intel/pinctrl-intel.c
> > b/drivers/pinctrl/intel/pinctrl-intel.c
> > index 96e73e30204e..1e24a6b8a64e 100644
> > --- a/drivers/pinctrl/intel/pinctrl-intel.c
> > +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> > @@ -788,6 +788,24 @@ static void intel_gpio_set(struct gpio_chip
> > *chip, unsigned offset, int value)
> >  	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> >  }
> >  
> > +static int intel_gpio_get_direction(struct gpio_chip *chip,
> > unsigned int offset)
> > +{
> > +	struct intel_pinctrl *pctrl = gpiochip_get_data(chip);
> > +	void __iomem *reg;
> > +	u32 padcfg0;
> > +
> > +	reg = intel_get_padcfg(pctrl, offset, PADCFG0);
> > +	if (!reg)
> > +		return -EINVAL;
> > +
> > +	padcfg0 = readl(reg);
> > +

> > +	if (padcfg0 & PADCFG0_PMODE_MASK)
> > +		return -EINVAL;

Actually we might return direction of GPIO function while pin is in some
other mode, though it would probably make not much sense in practice.

> > +
> > +	return !!(padcfg0 & PADCFG0_GPIOTXDIS);
> > +}
> > +
> >  static int intel_gpio_direction_input(struct gpio_chip *chip,
> > unsigned offset)
> >  {
> >  	return pinctrl_gpio_direction_input(chip->base + offset);
> > @@ -804,6 +822,7 @@ static const struct gpio_chip intel_gpio_chip =
> > {
> >  	.owner = THIS_MODULE,
> >  	.request = gpiochip_generic_request,
> >  	.free = gpiochip_generic_free,
> > +	.get_direction = intel_gpio_get_direction,
> >  	.direction_input = intel_gpio_direction_input,
> >  	.direction_output = intel_gpio_direction_output,
> >  	.get = intel_gpio_get,
> > 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] pinctrl: intel: Implement intel_gpio_get_direction callback

[Andy Shevchenko]

On Tue, 2018-03-06 at 16:56 +0200, Andy Shevchenko wrote:
> On Tue, 2018-03-06 at 14:31 +0000, Javier Arteaga wrote:

> > > +static int intel_gpio_get_direction(struct gpio_chip *chip,
> > > unsigned int offset)
> > > +{

> > > +	if (padcfg0 & PADCFG0_PMODE_MASK)
> > > +		return -EINVAL;
> 
> Actually we might return direction of GPIO function while pin is in
> some
> other mode, though it would probably make not much sense in practice.

One more though, this is a call back for GPIO function anyway, so, above
condition should never happen. I think it's safe to remove it
completely.

> > > +
> > > +	return !!(padcfg0 & PADCFG0_GPIOTXDIS);
> > > +}


-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH] pinctrl: intel: Implement intel_gpio_get_direction callback

[Javier Arteaga]

On 06/03/2018 14:59, Andy Shevchenko wrote:
> On Tue, 2018-03-06 at 16:56 +0200, Andy Shevchenko wrote:
>> On Tue, 2018-03-06 at 14:31 +0000, Javier Arteaga wrote:
> 
>>>> +static int intel_gpio_get_direction(struct gpio_chip *chip,
>>>> unsigned int offset)
>>>> +{
> 
>>>> +	if (padcfg0 & PADCFG0_PMODE_MASK)
>>>> +		return -EINVAL;
>>
>> Actually we might return direction of GPIO function while pin is in
>> some
>> other mode, though it would probably make not much sense in practice.
> 
> One more though, this is a call back for GPIO function anyway, so, above
> condition should never happen. I think it's safe to remove it
> completely.

The story behind that check is likely *not* a valid usecase: the current
iteration of the UP board drivers use gpiod_get_direction() *while*
requesting GPIOs to mirror SoC GPIO config on the on-board FPGA. So the
direction doesn't make sense for pins set to function mode.

As per your other feedback that driver should be reworked anyway -
that's not a reason to keep the check. I just thought it's a bit more
defensive, and saw there's some precedent of doing this:

    f002d07c56c7 ("gpio: tegra: Implement gpio_get_direction callback")

That being said I don't have a strong argument either way :)
I'll resend if you still feel it's unnecessary.

Thanks for reviewing!

Re: [PATCH] pinctrl: intel: Implement intel_gpio_get_direction callback

[Mika Westerberg]

On Tue, Mar 06, 2018 at 04:56:05PM +0200, Andy Shevchenko wrote:
> On Tue, 2018-03-06 at 14:31 +0000, Javier Arteaga wrote:
> > (Fix Andy's Cc: - apologies)
> > 
> > On 06/03/2018 13:42, Javier Arteaga wrote:
> > > Allows querying GPIO direction from the pad config register.
> > > If the pad is not in GPIO mode, return an error.
> > > 
> > > Signed-off-by: Javier Arteaga <javier@emutex.com>
> > > ---
> > > This is needed by the drivers for the UP Squared board, an APL-based
> > > platform. (For now, these drivers are out-of-tree.)
> > > 
> > > An earlier version of this patch was reviewed some time ago by Andy
> > > Shevchenko outside of the mailing lists:
> > > <https://github.com/emutex/ubilinux-kernel/commit/708a776f2ffd1b1c>
> > > 
> 
> Yes, this one look good!
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>