[PATCH] MIPS: rb532: fix bit swapping in rb532_set_bit()

[PATCH] MIPS: rb532: fix bit swapping in rb532_set_bit()

[Phil Sutter]

This is a simplified version of the original fix, thanks to Atsushi Nemoto for
the hint.

Greetings, Phil

---

The algorithm works unconditionally. If bitval is one, the first line is
a no op and the second line sets the bit at offset position. Vice versa,
if bitval is zero, the first line clears the bit at offset position and
the second line is a no op.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
---
 arch/mips/rb532/gpio.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/mips/rb532/gpio.c b/arch/mips/rb532/gpio.c
index 0e84c8a..e35cb75 100644
--- a/arch/mips/rb532/gpio.c
+++ b/arch/mips/rb532/gpio.c
@@ -119,13 +119,11 @@ static inline void rb532_set_bit(unsigned bitval,
 	unsigned long flags;
 	u32 val;
 
-	bitval = !!bitval;              /* map parameter to {0,1} */
-
 	local_irq_save(flags);
 
 	val = readl(ioaddr);
-	val &= ~( ~bitval << offset );   /* unset bit if bitval == 0 */
-	val |=  (  bitval << offset );   /* set bit if bitval == 1 */
+	val &= ~(!bitval << offset);   /* unset bit if bitval == 0 */
+	val |= (!!bitval << offset);   /* set bit if bitval == 1 */
 	writel(val, ioaddr);
 
 	local_irq_restore(flags);
-- 
1.5.6.4

Re: [PATCH] MIPS: rb532: fix bit swapping in rb532_set_bit()

[Phil Sutter]

On Wed, Nov 12, 2008 at 12:14:40AM +0100, Phil Sutter wrote:
> This is a simplified version of the original fix, thanks to Atsushi Nemoto for
> the hint.

Shame on me, I got the wrong patch and so this is just a copy of the
mail sent before, but with a missing In-Reply-To header.

Sorry for that, Phil

Re: [PATCH] provide functions for gpio configuration

[Phil Sutter]

Hi,

On Mon, Nov 03, 2008 at 12:56:16AM +0300, Sergei Shtylyov wrote:
> >+	val &= ~( ~bitval << offset );   /* unset bit if bitval == 0 */
> >  
> 
>   If bitval == 0, ~bitval evaluates to all ones, and the final AND mask 
> ~(0xffffffff << offset) clears 32-offset MSBs. What you want is simply 
> ~(1 << offset).

Right, I messed up boolean and bitwise inverting. The correct line is:

| val &= ~(!bitval << offset);   /* unset bit if bitval == 0 */

That's the same as ~(1 << offset) when bitval is zero, but turns into a
no op when it's one. That's what I want here, successfully removing the
need for the if-else case.

> >@@ -176,117 +184,60 @@ static int rb532_gpio_direction_input(struct 
> >gpio_chip *chip, unsigned offset)
> > static int rb532_gpio_direction_output(struct gpio_chip *chip,
> > 					unsigned offset, int value)
> > {
> >-	unsigned long		flags;
> >-	u32			mask = 1 << offset;
> >-	u32			tmp;
> > 	struct rb532_gpio_chip	*gpch;
> >-	void __iomem		*gpdr;
> > 
> > 	gpch = container_of(chip, struct rb532_gpio_chip, chip);
> >-	writel(mask, gpch->regbase + GPIOD);
> >  
> 
>   Hm, the old code seems really borked here...

It's not as bad as this may look like. In fact I could just simplify
lots of the functions by having them call rb532_set_bit().

A patch fixing the above mentioned issue follows, thanks for your help!

Greetings, Phil

[PATCH] MIPS: rb532: fix bit swapping in rb532_set_bit()

[Phil Sutter]

The algorithm works unconditionally. If bitval is one, the first line is
a no op and the second line sets the bit at offset position. Vice versa,
if bitval is zero, the first line clears the bit at offset position and
the second line is a no op.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
---
 arch/mips/rb532/gpio.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/mips/rb532/gpio.c b/arch/mips/rb532/gpio.c
index 0e84c8a..ba9a0c4 100644
--- a/arch/mips/rb532/gpio.c
+++ b/arch/mips/rb532/gpio.c
@@ -124,8 +124,8 @@ static inline void rb532_set_bit(unsigned bitval,
 	local_irq_save(flags);
 
 	val = readl(ioaddr);
-	val &= ~( ~bitval << offset );   /* unset bit if bitval == 0 */
-	val |=  (  bitval << offset );   /* set bit if bitval == 1 */
+	val &= ~(!bitval << offset);   /* unset bit if bitval == 0 */
+	val |=  (bitval << offset);   /* set bit if bitval == 1 */
 	writel(val, ioaddr);
 
 	local_irq_restore(flags);
-- 
1.5.6.4

Re: [PATCH] MIPS: rb532: fix bit swapping in rb532_set_bit()

[Atsushi Nemoto]

On Mon,  3 Nov 2008 15:30:25 +0100, Phil Sutter <n0-1@freewrt.org> wrote:
> The algorithm works unconditionally. If bitval is one, the first line is
> a no op and the second line sets the bit at offset position. Vice versa,
> if bitval is zero, the first line clears the bit at offset position and
> the second line is a no op.

Well, the linux gpio framework uses 0 for low, _nonzero_ for high.
You should not assume the bitval is 0 or 1.

	val &= ~(!bitval << offset);   /* unset bit if bitval == 0 */
	val |= (!!bitval << offset);   /* set bit if bitval != 0 */

would be safe here.  Or you should ensure the bitval is 0 or 1
somewhere.

---
Atsushi Nemoto

Re: [PATCH] MIPS: rb532: fix bit swapping in rb532_set_bit()

[Phil Sutter]

Hi,

On Mon, Nov 03, 2008 at 11:48:56PM +0900, Atsushi Nemoto wrote:
> Well, the linux gpio framework uses 0 for low, _nonzero_ for high.
> You should not assume the bitval is 0 or 1.

Good point. I was thinking about this potential problem, too. This is
why the mentioned function contains the following line (sadly too far
off to occur in the patch):

| bitval = !!bitval;              /* map parameter to {0,1} */

I put that separately to not make the below lines even more unreadable.

> 	val &= ~(!bitval << offset);   /* unset bit if bitval == 0 */
> 	val |= (!!bitval << offset);   /* set bit if bitval != 0 */

But as a boolean inverse has to be used anyway, your solution looks more
elegant than mine.

Greetings, Phil

[PATCH] MIPS: rb532: fix bit swapping in rb532_set_bit()

[Phil Sutter]

This is a simplified version of the original fix, thanks to Atsushi Nemoto for
the hint.

Greetings, Phil

---

The algorithm works unconditionally. If bitval is one, the first line is
a no op and the second line sets the bit at offset position. Vice versa,
if bitval is zero, the first line clears the bit at offset position and
the second line is a no op.

Signed-off-by: Phil Sutter <n0-1@freewrt.org>
---
 arch/mips/rb532/gpio.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/mips/rb532/gpio.c b/arch/mips/rb532/gpio.c
index 0e84c8a..e35cb75 100644
--- a/arch/mips/rb532/gpio.c
+++ b/arch/mips/rb532/gpio.c
@@ -119,13 +119,11 @@ static inline void rb532_set_bit(unsigned bitval,
 	unsigned long flags;
 	u32 val;
 
-	bitval = !!bitval;              /* map parameter to {0,1} */
-
 	local_irq_save(flags);
 
 	val = readl(ioaddr);
-	val &= ~( ~bitval << offset );   /* unset bit if bitval == 0 */
-	val |=  (  bitval << offset );   /* set bit if bitval == 1 */
+	val &= ~(!bitval << offset);   /* unset bit if bitval == 0 */
+	val |= (!!bitval << offset);   /* set bit if bitval == 1 */
 	writel(val, ioaddr);
 
 	local_irq_restore(flags);
-- 
1.5.6.4

Re: [PATCH] MIPS: rb532: fix bit swapping in rb532_set_bit()

[Ralf Baechle]

On Wed, Nov 12, 2008 at 12:09:30AM +0100, Phil Sutter wrote:

thanks, applied.

  Ralf