[PATCH] mxc/gpio: make _set_value work with values != 0/1

[PATCH] mxc/gpio: make _set_value work with values != 0/1

[Peter Korsgaard]

Documentation/gpio.txt specifies that the value argument to
gpio_set_value() should be handled as a boolean (E.G. != 0 is high),
so use the same logic as in _set_direction().

Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
---
 arch/arm/plat-mxc/gpio.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/arm/plat-mxc/gpio.c b/arch/arm/plat-mxc/gpio.c
index 57ec4a8..e226801 100644
--- a/arch/arm/plat-mxc/gpio.c
+++ b/arch/arm/plat-mxc/gpio.c
@@ -234,8 +234,13 @@ static void mxc_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 	u32 l;
 	unsigned long flags;
 
+
 	spin_lock_irqsave(&port->lock, flags);
-	l = (__raw_readl(reg) & (~(1 << offset))) | (value << offset);
+	l = __raw_readl(reg);
+	if (value)
+		l |= 1 << offset;
+	else
+		l &= ~(1 << offset);
 	__raw_writel(l, reg);
 	spin_unlock_irqrestore(&port->lock, flags);
 }
-- 
1.7.1

[PATCH] mxc/gpio: make _set_value work with values != 0/1

[Marc Kleine-Budde]

On 10/11/2010 01:59 PM, Peter Korsgaard wrote:
> Documentation/gpio.txt specifies that the value argument to
> gpio_set_value() should be handled as a boolean (E.G. != 0 is high),
> so use the same logic as in _set_direction().
> 
> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
> ---
>  arch/arm/plat-mxc/gpio.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/plat-mxc/gpio.c b/arch/arm/plat-mxc/gpio.c
> index 57ec4a8..e226801 100644
> --- a/arch/arm/plat-mxc/gpio.c
> +++ b/arch/arm/plat-mxc/gpio.c
> @@ -234,8 +234,13 @@ static void mxc_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
>  	u32 l;
>  	unsigned long flags;
>  
> +
>  	spin_lock_irqsave(&port->lock, flags);
> -	l = (__raw_readl(reg) & (~(1 << offset))) | (value << offset);

what about using "!!value" instead of just "value".

> +	l = __raw_readl(reg);
> +	if (value)
> +		l |= 1 << offset;
> +	else
> +		l &= ~(1 << offset);
>  	__raw_writel(l, reg);
>  	spin_unlock_irqrestore(&port->lock, flags);
>  }

cheers, Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 262 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20101011/2c692258/attachment.sig>

[PATCH] mxc/gpio: make _set_value work with values != 0/1

[Peter Korsgaard]

>>>>> "Marc" == Marc Kleine-Budde <mkl@pengutronix.de> writes:

Hi,

 Marc> On 10/11/2010 01:59 PM, Peter Korsgaard wrote:
 >> Documentation/gpio.txt specifies that the value argument to
 >> gpio_set_value() should be handled as a boolean (E.G. != 0 is high),
 >> so use the same logic as in _set_direction().
 >> 
 >> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
 >> ---
 >> arch/arm/plat-mxc/gpio.c |    7 ++++++-
 >> 1 files changed, 6 insertions(+), 1 deletions(-)
 >> 
 >> diff --git a/arch/arm/plat-mxc/gpio.c b/arch/arm/plat-mxc/gpio.c
 >> index 57ec4a8..e226801 100644
 >> --- a/arch/arm/plat-mxc/gpio.c
 >> +++ b/arch/arm/plat-mxc/gpio.c
 >> @@ -234,8 +234,13 @@ static void mxc_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 >> u32 l;
 >> unsigned long flags;
 >> 
 >> +
 >> spin_lock_irqsave(&port->lock, flags);
 >> -	l = (__raw_readl(reg) & (~(1 << offset))) | (value << offset);

 Marc> what about using "!!value" instead of just "value".

Sure, that's equivalent, but I found that harder to read. My form has
the extra advantage that it matches the structure of
_set_gpio_direction() just above.

-- 
Bye, Peter Korsgaard

[PATCH] mxc/gpio: make _set_value work with values != 0/1

[Uwe Kleine-König]

Hallali,

On Mon, Oct 11, 2010 at 01:59:05PM +0200, Peter Korsgaard wrote:
> Documentation/gpio.txt specifies that the value argument to
> gpio_set_value() should be handled as a boolean (E.G. != 0 is high),
> so use the same logic as in _set_direction().
> 
> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
> ---
>  arch/arm/plat-mxc/gpio.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/plat-mxc/gpio.c b/arch/arm/plat-mxc/gpio.c
> index 57ec4a8..e226801 100644
> --- a/arch/arm/plat-mxc/gpio.c
> +++ b/arch/arm/plat-mxc/gpio.c
> @@ -234,8 +234,13 @@ static void mxc_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
>  	u32 l;
>  	unsigned long flags;
>  
> +
>  	spin_lock_irqsave(&port->lock, flags);
> -	l = (__raw_readl(reg) & (~(1 << offset))) | (value << offset);
> +	l = __raw_readl(reg);
> +	if (value)
> +		l |= 1 << offset;
> +	else
> +		l &= ~(1 << offset);
Why not just

	l = (__raw_readl(reg) & (~(1 << offset))) | (!!value << offset);

?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] mxc/gpio: make _set_value work with values != 0/1

[Peter Korsgaard]

>>>>> "Uwe" == Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de> writes:

Hi,

 Uwe> On Mon, Oct 11, 2010 at 01:59:05PM +0200, Peter Korsgaard wrote:
 >> @@ -234,8 +234,13 @@ static void mxc_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 >> u32 l;
 >> unsigned long flags;
 >> 
 >> +

Ups, that extra line shouldn't have been there.

 >> spin_lock_irqsave(&port->lock, flags);
 >> -	l = (__raw_readl(reg) & (~(1 << offset))) | (value << offset);
 >> +	l = __raw_readl(reg);
 >> +	if (value)
 >> +		l |= 1 << offset;
 >> +	else
 >> +		l &= ~(1 << offset);
 Uwe> Why not just

 Uwe> 	l = (__raw_readl(reg) & (~(1 << offset))) | (!!value << offset);

Se my other reply. I don't feel strongly about it, but I for one don't
remember the precedence rules for !! and << offhand.

-- 
Bye, Peter Korsgaard

[PATCH] mxc/gpio: make _set_value work with values != 0/1

[Michał Nazarewicz]

On Mon, 11 Oct 2010 14:21:30 +0200, Peter Korsgaard <jacmet@sunsite.dk> wrote:
> Se my other reply. I don't feel strongly about it, but I for one don't
> remember the precedence rules for !! and << offhand.

A bit off-topic, but hopefuly it'll help someone:

1. (In C,) one argument operators have binding tighter than two (or three) argument
    operators. (The only exception to this rule is the :: operator in C++ which has
    the tighest binding of all operators.)
2. The next rule, is that one argument postfix operators has tigter binding
    then prefix operators, thus *a++ is *(a++).

I find those two rules let me understand most operatrs use cases.  Hope this
helps. :)


Also, I haven't followed the patch from the beginnig (just stumbled accross
some mails), but anoter possible solution to the problem would be to change
the type of "value" to "bool".

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Micha? "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--

[PATCH] mxc/gpio: make _set_value work with values != 0/1

[Baruch Siach]

Hi Peter,

On Mon, Oct 11, 2010 at 01:59:05PM +0200, Peter Korsgaard wrote:
> Documentation/gpio.txt specifies that the value argument to
> gpio_set_value() should be handled as a boolean (E.G. != 0 is high),
> so use the same logic as in _set_direction().
> 
> Signed-off-by: Peter Korsgaard <jacmet@sunsite.dk>
> ---
>  arch/arm/plat-mxc/gpio.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/plat-mxc/gpio.c b/arch/arm/plat-mxc/gpio.c
> index 57ec4a8..e226801 100644
> --- a/arch/arm/plat-mxc/gpio.c
> +++ b/arch/arm/plat-mxc/gpio.c
> @@ -234,8 +234,13 @@ static void mxc_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
>  	u32 l;
>  	unsigned long flags;
>  
> +

Unneeded empty line.

>  	spin_lock_irqsave(&port->lock, flags);
> -	l = (__raw_readl(reg) & (~(1 << offset))) | (value << offset);
> +	l = __raw_readl(reg);
> +	if (value)
> +		l |= 1 << offset;
> +	else
> +		l &= ~(1 << offset);

Alternative shorter version:

l = (__raw_readl(reg) & (~(1 << offset))) | (!!value << offset);

baruch

>  	__raw_writel(l, reg);
>  	spin_unlock_irqrestore(&port->lock, flags);
>  }
> -- 
> 1.7.1

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH] mxc/gpio: make _set_value work with values != 0/1

[Peter Korsgaard]

>>>>> "Baruch" == Baruch Siach <baruch@tkos.co.il> writes:

Hi,

 >> +

 Baruch> Unneeded empty line.

Yeah, just noticed as well.

 >> spin_lock_irqsave(&port->lock, flags);
 >> -	l = (__raw_readl(reg) & (~(1 << offset))) | (value << offset);
 >> +	l = __raw_readl(reg);
 >> +	if (value)
 >> +		l |= 1 << offset;
 >> +	else
 >> +		l &= ~(1 << offset);

 Baruch> Alternative shorter version:

 Baruch> l = (__raw_readl(reg) & (~(1 << offset))) | (!!value << offset);

Well, what do you know - I seem to be outnumbered ;)

Sasha, do you want the !! version instead? Then I'll resend.

-- 
Bye, Peter Korsgaard

[PATCH] mxc/gpio: make _set_value work with values != 0/1

[Sascha Hauer]

On Mon, Oct 11, 2010 at 02:23:00PM +0200, Peter Korsgaard wrote:
> >>>>> "Baruch" == Baruch Siach <baruch@tkos.co.il> writes:
> 
> Hi,
> 
>  >> +
> 
>  Baruch> Unneeded empty line.
> 
> Yeah, just noticed as well.
> 
>  >> spin_lock_irqsave(&port->lock, flags);
>  >> -	l = (__raw_readl(reg) & (~(1 << offset))) | (value << offset);
>  >> +	l = __raw_readl(reg);
>  >> +	if (value)
>  >> +		l |= 1 << offset;
>  >> +	else
>  >> +		l &= ~(1 << offset);
> 
>  Baruch> Alternative shorter version:
> 
>  Baruch> l = (__raw_readl(reg) & (~(1 << offset))) | (!!value << offset);

This is shorter but I find this significantly harder to read and I bet
the compiler generates the same code from both versions.

> 
> Well, what do you know - I seem to be outnumbered ;)
> 
> Sasha, do you want the !! version instead? Then I'll resend.

I like the !! version. The only problem with this is that people tend
to try to remove the !! as it looks like a noop at first sight.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH] mxc/gpio: make _set_value work with values != 0/1

[Peter Korsgaard]

>>>>> "Sascha" == Sascha Hauer <s.hauer@pengutronix.de> writes:

Hi,

 Baruch> Alternative shorter version:
 Baruch> l = (__raw_readl(reg) & (~(1 << offset))) | (!!value << offset);

 Sascha> This is shorter but I find this significantly harder to read and I bet
 Sascha> the compiler generates the same code from both versions.

Agreed.

 >> Well, what do you know - I seem to be outnumbered ;)
 >> 
 >> Sasha, do you want the !! version instead? Then I'll resend.

 Sascha> I like the !! version. The only problem with this is that people tend
 Sascha> to try to remove the !! as it looks like a noop at first sight.

So what then? Do you want me to change to !!, resend with the extra
empty line removed or just let you remove the line when you apply it?

-- 
Bye, Peter Korsgaard

[PATCH] mxc/gpio: make _set_value work with values != 0/1

[Sascha Hauer]

On Mon, Oct 11, 2010 at 02:47:41PM +0200, Peter Korsgaard wrote:
> >>>>> "Sascha" == Sascha Hauer <s.hauer@pengutronix.de> writes:
> 
> Hi,
> 
>  Baruch> Alternative shorter version:
>  Baruch> l = (__raw_readl(reg) & (~(1 << offset))) | (!!value << offset);
> 
>  Sascha> This is shorter but I find this significantly harder to read and I bet
>  Sascha> the compiler generates the same code from both versions.
> 
> Agreed.
> 
>  >> Well, what do you know - I seem to be outnumbered ;)
>  >> 
>  >> Sasha, do you want the !! version instead? Then I'll resend.
> 
>  Sascha> I like the !! version. The only problem with this is that people tend
>  Sascha> to try to remove the !! as it looks like a noop at first sight.
> 
> So what then? Do you want me to change to !!, resend with the extra
> empty line removed or just let you remove the line when you apply it?

If noone replies with '!! totally sucks' within 2 minutes just send a
patch which changes it to !! and removes the extra empty line.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |