[PATCH RFT] gpio: zevio: Get correct gpio output value

[PATCH RFT] gpio: zevio: Get correct gpio output value

[Axel Lin]

Read gpio output value from ZEVIO_GPIO_OUTPUT.
The spin_lock is required to ensure the direction is not changed before reading
input/output value.

Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
Hi Fabian,
The document only mentions:
  +10 (R/W): Direction (set bit to 0 for output, 1 for input)
  +14 (R/W): GPIO output bit
  +18 (R): Reads GPIO input bit.
So I'm not sure if reading ZEVIO_GPIO_INPUT returns correct status when the
direction is output. So I think this needs test.

Regards,
Axel
 drivers/gpio/gpio-zevio.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-zevio.c b/drivers/gpio/gpio-zevio.c
index 9bf5034..29b0150 100644
--- a/drivers/gpio/gpio-zevio.c
+++ b/drivers/gpio/gpio-zevio.c
@@ -81,9 +81,15 @@ static inline void zevio_gpio_port_set(struct zevio_gpio *c, unsigned pin,
 static int zevio_gpio_get(struct gpio_chip *chip, unsigned pin)
 {
 	struct zevio_gpio *controller = to_zevio_gpio(chip);
+	u32 val, dir;
 
-	/* Only reading allowed, so no spinlock needed */
-	u32 val = zevio_gpio_port_get(controller, pin, ZEVIO_GPIO_INPUT);
+	spin_lock(&controller->lock);
+	dir = zevio_gpio_port_get(controller, pin, ZEVIO_GPIO_DIRECTION);
+	if (dir | BIT(ZEVIO_GPIO_BIT(pin)))
+		val = zevio_gpio_port_get(controller, pin, ZEVIO_GPIO_INPUT);
+	else
+		val = zevio_gpio_port_get(controller, pin, ZEVIO_GPIO_OUTPUT);
+	spin_unlock(&controller->lock);
 
 	return (val >> ZEVIO_GPIO_BIT(pin)) & 0x1;
 }
-- 
1.8.3.2

Re: [PATCH RFT] gpio: zevio: Get correct gpio output value

[Fabian Vogt]

Hi,

IIRC I tested that and input and output are independant of each other.
If you set a pin as output and write a 1 into the output register, but you connect the pin to ground physically output is "1" and input "0".
Which behaviour is the correct one for gpio_get?

Bye,
Fabian

Am Montag, 7. April 2014, 14:34:52 schrieb Axel Lin:
> Read gpio output value from ZEVIO_GPIO_OUTPUT.
> The spin_lock is required to ensure the direction is not changed before reading
> input/output value.
> 
> Signed-off-by: Axel Lin <axel.lin@ingics.com>
> ---
> Hi Fabian,
> The document only mentions:
>   +10 (R/W): Direction (set bit to 0 for output, 1 for input)
>   +14 (R/W): GPIO output bit
>   +18 (R): Reads GPIO input bit.
> So I'm not sure if reading ZEVIO_GPIO_INPUT returns correct status when the
> direction is output. So I think this needs test.
> 
> Regards,
> Axel
>  drivers/gpio/gpio-zevio.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-zevio.c b/drivers/gpio/gpio-zevio.c
> index 9bf5034..29b0150 100644
> --- a/drivers/gpio/gpio-zevio.c
> +++ b/drivers/gpio/gpio-zevio.c
> @@ -81,9 +81,15 @@ static inline void zevio_gpio_port_set(struct zevio_gpio *c, unsigned pin,
>  static int zevio_gpio_get(struct gpio_chip *chip, unsigned pin)
>  {
>  	struct zevio_gpio *controller = to_zevio_gpio(chip);
> +	u32 val, dir;
>  
> -	/* Only reading allowed, so no spinlock needed */
> -	u32 val = zevio_gpio_port_get(controller, pin, ZEVIO_GPIO_INPUT);
> +	spin_lock(&controller->lock);
> +	dir = zevio_gpio_port_get(controller, pin, ZEVIO_GPIO_DIRECTION);
> +	if (dir | BIT(ZEVIO_GPIO_BIT(pin)))
> +		val = zevio_gpio_port_get(controller, pin, ZEVIO_GPIO_INPUT);
> +	else
> +		val = zevio_gpio_port_get(controller, pin, ZEVIO_GPIO_OUTPUT);
> +	spin_unlock(&controller->lock);
>  
>  	return (val >> ZEVIO_GPIO_BIT(pin)) & 0x1;
>  }
> 

Re: [PATCH RFT] gpio: zevio: Get correct gpio output value

[Gerhard Sittig]

On Mon, 2014-04-07 at 17:17 +0200, Fabian Vogt wrote:
> 
> Hi,
> 
> IIRC I tested that and input and output are independant of each other.
> If you set a pin as output and write a 1 into the output register, but you connect the pin to ground physically output is "1" and input "0".
> Which behaviour is the correct one for gpio_get?

Please don't top-post, and wrap overly long lines!  Thanks.

I'd assume that gpio_get() should return the information about
the pin level ("is"), regardless of internal "want" state.  But
that's a guess (gut feeling), can't tell whether there is a
specified semantics, or whether this is supposed to be
"implementation defined".  Different hardware may have different
capabilites (like lack of support to read back what's on the
line, thus having to rely on internal registers which may differ
from the pin level).

> Am Montag, 7. April 2014, 14:34:52 schrieb Axel Lin:
> > Read gpio output value from ZEVIO_GPIO_OUTPUT.
> > The spin_lock is required to ensure the direction is not changed before reading
> > input/output value.
> > 
> > [ ... ]
> > @@ -81,9 +81,15 @@ static inline void zevio_gpio_port_set(struct zevio_gpio *c, unsigned pin,
> >  static int zevio_gpio_get(struct gpio_chip *chip, unsigned pin)
> >  {
> >  	struct zevio_gpio *controller = to_zevio_gpio(chip);
> > +	u32 val, dir;
> >  
> > -	/* Only reading allowed, so no spinlock needed */
> > -	u32 val = zevio_gpio_port_get(controller, pin, ZEVIO_GPIO_INPUT);
> > +	spin_lock(&controller->lock);
> > +	dir = zevio_gpio_port_get(controller, pin, ZEVIO_GPIO_DIRECTION);
> > +	if (dir | BIT(ZEVIO_GPIO_BIT(pin)))
> > +		val = zevio_gpio_port_get(controller, pin, ZEVIO_GPIO_INPUT);
> > +	else
> > +		val = zevio_gpio_port_get(controller, pin, ZEVIO_GPIO_OUTPUT);
> > +	spin_unlock(&controller->lock);

As a side effect of your making me look at the code:

Is the if() not an "always true" condition?  Should this be
s/|/\&/ instead?


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

Re: [PATCH RFT] gpio: zevio: Get correct gpio output value

[Axel Lin]

>> > @@ -81,9 +81,15 @@ static inline void zevio_gpio_port_set(struct zevio_gpio *c, unsigned pin,
>> >  static int zevio_gpio_get(struct gpio_chip *chip, unsigned pin)
>> >  {
>> >     struct zevio_gpio *controller = to_zevio_gpio(chip);
>> > +   u32 val, dir;
>> >
>> > -   /* Only reading allowed, so no spinlock needed */
>> > -   u32 val = zevio_gpio_port_get(controller, pin, ZEVIO_GPIO_INPUT);
>> > +   spin_lock(&controller->lock);
>> > +   dir = zevio_gpio_port_get(controller, pin, ZEVIO_GPIO_DIRECTION);
>> > +   if (dir | BIT(ZEVIO_GPIO_BIT(pin)))
>> > +           val = zevio_gpio_port_get(controller, pin, ZEVIO_GPIO_INPUT);
>> > +   else
>> > +           val = zevio_gpio_port_get(controller, pin, ZEVIO_GPIO_OUTPUT);
>> > +   spin_unlock(&controller->lock);
>
> As a side effect of your making me look at the code:
>
> Is the if() not an "always true" condition?  Should this be
> s/|/\&/ instead?
You are right, I'm sending a v2 now.

Thanks for the review,
Axel