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

[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

[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] 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