[PATCH] gpio-sch: set output level after configuration of direction

[PATCH] gpio-sch: set output level after configuration of direction

[Alexander Stein]

From: Daniel Krueger <daniel.krueger@systec-electronic.com>

According to the datasheet, writing to the level register has no effect
when GPIO is programmed as input. Hence we set the level after configuring
the GPIO as output. But we cannot prevent a short low pulse if direction is
set to high and an external pull-up is connected.

Signed-off-by: Daniel Krueger <daniel.krueger@systec-electronic.com>
Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 drivers/gpio/gpio-sch.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
index 5af6571..e5f3e2e 100644
--- a/drivers/gpio/gpio-sch.c
+++ b/drivers/gpio/gpio-sch.c
@@ -97,8 +97,6 @@ static int sch_gpio_core_direction_out(struct gpio_chip *gc,
 	u8 curr_dirs;
 	unsigned short offset, bit;
 
-	sch_gpio_core_set(gc, gpio_num, val);
-
 	spin_lock(&gpio_lock);
 
 	offset = CGIO + gpio_num / 8;
@@ -109,6 +107,15 @@ static int sch_gpio_core_direction_out(struct gpio_chip *gc,
 		outb(curr_dirs & ~(1 << bit), gpio_ba + offset);
 
 	spin_unlock(&gpio_lock);
+
+	/*
+	 * according to the datasheet, writing to the level register has no
+	 * effect when GPIO is programmed as input.
+	 * Hence we set the level after configuring the GPIO as output.
+	 * But we cannot prevent a short low pulse if direction is set to high
+	 * and an external pull-up is connected.
+	 */
+	sch_gpio_core_set(gc, gpio_num, val);
 	return 0;
 }
 
@@ -178,8 +185,6 @@ static int sch_gpio_resume_direction_out(struct gpio_chip *gc,
 	u8 curr_dirs;
 	unsigned short offset, bit;
 
-	sch_gpio_resume_set(gc, gpio_num, val);
-
 	offset = RGIO + gpio_num / 8;
 	bit = gpio_num % 8;
 
@@ -190,6 +195,15 @@ static int sch_gpio_resume_direction_out(struct gpio_chip *gc,
 		outb(curr_dirs & ~(1 << bit), gpio_ba + offset);
 
 	spin_unlock(&gpio_lock);
+
+	/*
+	 * according to the datasheet, writing to the level register has no
+	 * effect when GPIO is programmed as input.
+	 * Hence we set the level after configuring the GPIO as output.
+	 * But we cannot prevent a short low pulse if direction is set to high
+	 * and an external pull-up is connected.
+	 */
+	sch_gpio_resume_set(gc, gpio_num, val);
 	return 0;
 }
 
-- 
1.8.3.2

Re: [PATCH] gpio-sch: set output level after configuration of direction

[Gerhard Sittig]

On Tue, 2014-03-25 at 10:33 +0100, Alexander Stein wrote:
> 
> According to the datasheet, writing to the level register has no effect
> when GPIO is programmed as input. Hence we set the level after configuring
> the GPIO as output. But we cannot prevent a short low pulse if direction is
> set to high and an external pull-up is connected.

Does "will have no effect" actually translate into "should not be
done" or even "must not be done"?  Doesn't sound like it.

I understood that setting the data register for input pins does
nothing in that very moment, yet when switching to output the
value immediately gets used.  So this is the most appropriate
thing to do.


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] gpio-sch: set output level after configuration of direction

[Alexander Stein]

On Wednesday 26 March 2014 20:05:43, Gerhard Sittig wrote:
> On Tue, 2014-03-25 at 10:33 +0100, Alexander Stein wrote:
> > 
> > According to the datasheet, writing to the level register has no effect
> > when GPIO is programmed as input. Hence we set the level after configuring
> > the GPIO as output. But we cannot prevent a short low pulse if direction is
> > set to high and an external pull-up is connected.
> 
> Does "will have no effect" actually translate into "should not be
> done" or even "must not be done"?  Doesn't sound like it.

I would translate that into 'won't do anything', it's read-only, thus the written value is lost. It just reflects the current signal when configured as input.

> I understood that setting the data register for input pins does
> nothing in that very moment, yet when switching to output the
> value immediately gets used.  So this is the most appropriate
> thing to do.

That would be the correct way, if hardware can be programmed that way. But if the data register is set when GPIO is confgured as input the data is lost. So when switching to output it does _NOT_ have the previously set value.
That's why the direction is set first and data afterwards. Awkward, but yet the only way to set an "initial" output value

Regards,
Alexander
-- 
Dipl.-Inf. Alexander Stein

SYS TEC electronic GmbH
Am Windrad 2
08468 Heinsdorfergrund
Tel.: 03765 38600-1156
Fax: 03765 38600-4100
Email: alexander.stein@systec-electronic.com
Website: www.systec-electronic.com
 
Managing Director: Dipl.-Phys. Siegmar Schmidt
Commercial registry: Amtsgericht Chemnitz, HRB 28082

Re: [PATCH] gpio-sch: set output level after configuration of direction

[Gerhard Sittig]

On Thu, 2014-03-27 at 14:10 +0100, Alexander Stein wrote:
> 
> On Wednesday 26 March 2014 20:05:43, Gerhard Sittig wrote:
> > On Tue, 2014-03-25 at 10:33 +0100, Alexander Stein wrote:
> > > 
> > > According to the datasheet, writing to the level register has no effect
> > > when GPIO is programmed as input. Hence we set the level after configuring
> > > the GPIO as output. But we cannot prevent a short low pulse if direction is
> > > set to high and an external pull-up is connected.
> > 
> > Does "will have no effect" actually translate into "should not be
> > done" or even "must not be done"?  Doesn't sound like it.
> 
> I would translate that into 'won't do anything', it's read-only, thus the written value is lost. It just reflects the current signal when configured as input.

This is new information (both the "read-only" thing and "data
will be lost").  I'd suggest to update the commit message then.

BTW does reflecting input data and accepting output data (which
may or may not apply at the moment since it's only used in output
mode) not necessarily conflict.  Hardware may have two separate
registers (one for read, one for write) at the same address.
GPIO data registers often are implemented that way.  Which is why
you usually can "queue" the output value and then switch
directions.  And in the output case it's valid as well that input
sensed at the wire and output data dictated by the CPU might
differ (think open drain or bus keeper).

> > I understood that setting the data register for input pins does
> > nothing in that very moment, yet when switching to output the
> > value immediately gets used.  So this is the most appropriate
> > thing to do.
> 
> That would be the correct way, if hardware can be programmed that way. But if the data register is set when GPIO is confgured as input the data is lost. So when switching to output it does _NOT_ have the previously set value.
> That's why the direction is set first and data afterwards. Awkward, but yet the only way to set an "initial" output value

This information should end up in the source comment, I guess, in
this very strength.  It's an unusual limitation in a block that
implements a commodity feature.  The previous "soft" form made me
ask those questions, as will the next person to look ask again.

You keep referring to the documentation, did you check that the
hardware behaves the way you interpret the document?  Just to
make sure ...  This question might be avoided by saying "the
datasheet states and observation of hardware acknowledges ...".
It's not unusual that documentation and implementation differ. :)


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

[PATCH v2] gpio-sch: set output level after configuration of direction

[Alexander Stein]

From: Daniel Krueger <daniel.krueger@systec-electronic.com>

According to the datasheet, writing to the level register has no effect
when GPIO is programmed as input. Actually the the level register is
read-only when configured as input. Thus presetting the output level
before switching to output is _NOT_ possible. Any writes are lost!
Hence we set the level after configuring the GPIO as output.
But we cannot prevent a short low pulse if direction is set to high and
an external  pull-up is connected.

Signed-off-by: Daniel Krueger <daniel.krueger@systec-electronic.com>
Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
Changes in v2:
* Adjusted commited message to make clear that writes are lost when GPIO
is configured as input.

 drivers/gpio/gpio-sch.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-sch.c b/drivers/gpio/gpio-sch.c
index 5af6571..a9b1cd1 100644
--- a/drivers/gpio/gpio-sch.c
+++ b/drivers/gpio/gpio-sch.c
@@ -97,8 +97,6 @@ static int sch_gpio_core_direction_out(struct gpio_chip *gc,
 	u8 curr_dirs;
 	unsigned short offset, bit;
 
-	sch_gpio_core_set(gc, gpio_num, val);
-
 	spin_lock(&gpio_lock);
 
 	offset = CGIO + gpio_num / 8;
@@ -109,6 +107,17 @@ static int sch_gpio_core_direction_out(struct gpio_chip *gc,
 		outb(curr_dirs & ~(1 << bit), gpio_ba + offset);
 
 	spin_unlock(&gpio_lock);
+
+	/*
+	 * according to the datasheet, writing to the level register has no
+	 * effect when GPIO is programmed as input.
+	 * Actually the the level register is read-only when configured as input.
+	 * Thus presetting the output level before switching to output is _NOT_ possible.
+	 * Hence we set the level after configuring the GPIO as output.
+	 * But we cannot prevent a short low pulse if direction is set to high
+	 * and an external pull-up is connected.
+	 */
+	sch_gpio_core_set(gc, gpio_num, val);
 	return 0;
 }
 
@@ -178,8 +187,6 @@ static int sch_gpio_resume_direction_out(struct gpio_chip *gc,
 	u8 curr_dirs;
 	unsigned short offset, bit;
 
-	sch_gpio_resume_set(gc, gpio_num, val);
-
 	offset = RGIO + gpio_num / 8;
 	bit = gpio_num % 8;
 
@@ -190,6 +197,17 @@ static int sch_gpio_resume_direction_out(struct gpio_chip *gc,
 		outb(curr_dirs & ~(1 << bit), gpio_ba + offset);
 
 	spin_unlock(&gpio_lock);
+
+	/*
+	* according to the datasheet, writing to the level register has no
+	* effect when GPIO is programmed as input.
+	* Actually the the level register is read-only when configured as input.
+	* Thus presetting the output level before switching to output is _NOT_ possible.
+	* Hence we set the level after configuring the GPIO as output.
+	* But we cannot prevent a short low pulse if direction is set to high
+	* and an external pull-up is connected.
+	*/
+	sch_gpio_resume_set(gc, gpio_num, val);
 	return 0;
 }
 
-- 
1.8.3.2

Re: [PATCH v2] gpio-sch: set output level after configuration of direction

[Linus Walleij]

On Mon, Apr 7, 2014 at 2:20 PM, Alexander Stein
<alexander.stein@systec-electronic.com> wrote:

> From: Daniel Krueger <daniel.krueger@systec-electronic.com>
>
> According to the datasheet, writing to the level register has no effect
> when GPIO is programmed as input. Actually the the level register is
> read-only when configured as input. Thus presetting the output level
> before switching to output is _NOT_ possible. Any writes are lost!
> Hence we set the level after configuring the GPIO as output.
> But we cannot prevent a short low pulse if direction is set to high and
> an external  pull-up is connected.
>
> Signed-off-by: Daniel Krueger <daniel.krueger@systec-electronic.com>
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> ---
> Changes in v2:
> * Adjusted commited message to make clear that writes are lost when GPIO
> is configured as input.

No further comments, so I have applied this v2 version for v3.16.

Yours,
Linus Walleij