i2c/busses/i2c-highlander.c: using char variable in bit operation

i2c/busses/i2c-highlander.c: using char variable in bit operation

[d binderman]

Hello there,

I just ran the sourceforge tool cppcheck over the source code of the
new Linux kernel 2.6.33-rc6

It said

[./i2c/busses/i2c-highlander.c:284]: (style) Warning - using char variable in bit operation

The source code is

static int highlander_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr,
                                  unsigned short flags, char read_write,
                                  u8 command, int size,
                                  union i2c_smbus_data *data)
{
        struct highlander_i2c_dev *dev = i2c_get_adapdata(adap);
        int read = read_write & I2C_SMBUS_READ;

In C, chars can be signed or unsigned, so the value written into
local variable read by sign extension is not certain.

Suggest new code

static int highlander_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr,
                                  unsigned short flags, char read_write,
                                  u8 command, int size,
                                  union i2c_smbus_data *data)
{
        struct highlander_i2c_dev *dev = i2c_get_adapdata(adap);
        int read = ((unsigned char) read_write) & I2C_SMBUS_READ;


Regards

David Binderman

 		 	   		  
_________________________________________________________________
Tell us your greatest, weirdest and funniest Hotmail stories
http://clk.atdmt.com/UKM/go/195013117/direct/01/--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: i2c/busses/i2c-highlander.c: using char variable in bit operation

[Jean Delvare]

On Mon, 1 Feb 2010 09:51:42 +0000, d binderman wrote:
> 
> 
> Hello there,
> 
> I just ran the sourceforge tool cppcheck over the source code of the
> new Linux kernel 2.6.33-rc6
> 
> It said
> 
> [./i2c/busses/i2c-highlander.c:284]: (style) Warning - using char variable in bit operation
> 
> The source code is
> 
> static int highlander_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr,
>                                   unsigned short flags, char read_write,
>                                   u8 command, int size,
>                                   union i2c_smbus_data *data)
> {
>         struct highlander_i2c_dev *dev = i2c_get_adapdata(adap);
>         int read = read_write & I2C_SMBUS_READ;
> 
> In C, chars can be signed or unsigned, so the value written into
> local variable read by sign extension is not certain.
> 
> Suggest new code
> 
> static int highlander_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr,
>                                   unsigned short flags, char read_write,
>                                   u8 command, int size,
>                                   union i2c_smbus_data *data)
> {
>         struct highlander_i2c_dev *dev = i2c_get_adapdata(adap);
>         int read = ((unsigned char) read_write) & I2C_SMBUS_READ;

read_write can only have value 0 or 1, so we don't care if chars are
signed or not. That being said, the code above looks odd, the value of
read_write can be used in the code directly without using an
intermediate variable:

	iowrite16((addr << 1) | read_write, dev->base + SMSMADR);

This would solve your warning issue and make the code more simple too.

-- 
Jean Delvare

Re: i2c/busses/i2c-highlander.c: using char variable in bit operation

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2034 bytes --]

On Mon, Feb 01, 2010 at 11:07:21AM +0100, Jean Delvare wrote:
> On Mon, 1 Feb 2010 09:51:42 +0000, d binderman wrote:
> > 
> > 
> > Hello there,
> > 
> > I just ran the sourceforge tool cppcheck over the source code of the
> > new Linux kernel 2.6.33-rc6
> > 
> > It said
> > 
> > [./i2c/busses/i2c-highlander.c:284]: (style) Warning - using char variable in bit operation
> > 
> > The source code is
> > 
> > static int highlander_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr,
> >                                   unsigned short flags, char read_write,
> >                                   u8 command, int size,
> >                                   union i2c_smbus_data *data)
> > {
> >         struct highlander_i2c_dev *dev = i2c_get_adapdata(adap);
> >         int read = read_write & I2C_SMBUS_READ;
> > 
> > In C, chars can be signed or unsigned, so the value written into
> > local variable read by sign extension is not certain.
> > 
> > Suggest new code
> > 
> > static int highlander_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr,
> >                                   unsigned short flags, char read_write,
> >                                   u8 command, int size,
> >                                   union i2c_smbus_data *data)
> > {
> >         struct highlander_i2c_dev *dev = i2c_get_adapdata(adap);
> >         int read = ((unsigned char) read_write) & I2C_SMBUS_READ;
> 
> read_write can only have value 0 or 1, so we don't care if chars are
> signed or not. That being said, the code above looks odd, the value of
> read_write can be used in the code directly without using an
> intermediate variable:
> 
> 	iowrite16((addr << 1) | read_write, dev->base + SMSMADR);
> 
> This would solve your warning issue and make the code more simple too.

Don't forget to convert the 'if (read)' below, too.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH] i2c/highlander: remover superflous variable

[Wolfram Sang]

When cppcheck found this flaw

[./i2c/busses/i2c-highlander.c:284]: (style) Warning - using char variable in bit operation

it was noted that the 'read'-variable could just be removed as read_write can
only be 0 or 1 anyhow. So, we remove the flaw by simplifying the code.

Reported-by: d binderman <dcb314-PkbjNfxxIARBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Cc: Paul Mundt <lethal-M7jkjyW5wf5g9hUCZPvPmw@public.gmane.org>
Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
Cc: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>
---

There was no patch following the original report, so I picked it up. Not
tested, due to no hardware.

Jean, I put you on CC as you commented on the original mail, although it is
more an embedded driver.

 drivers/i2c/busses/i2c-highlander.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-highlander.c b/drivers/i2c/busses/i2c-highlander.c
index 87ecace..db290b9 100644
--- a/drivers/i2c/busses/i2c-highlander.c
+++ b/drivers/i2c/busses/i2c-highlander.c
@@ -281,7 +281,6 @@ static int highlander_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr,
 				  union i2c_smbus_data *data)
 {
 	struct highlander_i2c_dev *dev = i2c_get_adapdata(adap);
-	int read = read_write & I2C_SMBUS_READ;
 	u16 tmp;
 
 	init_completion(&dev->cmd_complete);
@@ -336,11 +335,11 @@ static int highlander_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr,
 	highlander_i2c_done(dev);
 
 	/* Set slave address */
-	iowrite16((addr << 1) | read, dev->base + SMSMADR);
+	iowrite16((addr << 1) | read_write, dev->base + SMSMADR);
 
 	highlander_i2c_command(dev, command, dev->buf_len);
 
-	if (read)
+	if (read_write)
 		return highlander_i2c_read(dev);
 	else
 		return highlander_i2c_write(dev);
-- 
1.6.5

Re: [PATCH] i2c/highlander: remover superflous variable

[Jean Delvare]

On Tue,  2 Feb 2010 13:17:43 +0100, Wolfram Sang wrote:
> When cppcheck found this flaw
> 
> [./i2c/busses/i2c-highlander.c:284]: (style) Warning - using char variable in bit operation
> 
> it was noted that the 'read'-variable could just be removed as read_write can
> only be 0 or 1 anyhow. So, we remove the flaw by simplifying the code.
> 
> Reported-by: d binderman <dcb314-PkbjNfxxIARBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Paul Mundt <lethal-M7jkjyW5wf5g9hUCZPvPmw@public.gmane.org>
> Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Cc: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>
> ---
> 
> There was no patch following the original report, so I picked it up. Not
> tested, due to no hardware.
> 
> Jean, I put you on CC as you commented on the original mail, although it is
> more an embedded driver.
> 
>  drivers/i2c/busses/i2c-highlander.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-highlander.c b/drivers/i2c/busses/i2c-highlander.c
> index 87ecace..db290b9 100644
> --- a/drivers/i2c/busses/i2c-highlander.c
> +++ b/drivers/i2c/busses/i2c-highlander.c
> @@ -281,7 +281,6 @@ static int highlander_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr,
>  				  union i2c_smbus_data *data)
>  {
>  	struct highlander_i2c_dev *dev = i2c_get_adapdata(adap);
> -	int read = read_write & I2C_SMBUS_READ;
>  	u16 tmp;
>  
>  	init_completion(&dev->cmd_complete);
> @@ -336,11 +335,11 @@ static int highlander_i2c_smbus_xfer(struct i2c_adapter *adap, u16 addr,
>  	highlander_i2c_done(dev);
>  
>  	/* Set slave address */
> -	iowrite16((addr << 1) | read, dev->base + SMSMADR);
> +	iowrite16((addr << 1) | read_write, dev->base + SMSMADR);
>  
>  	highlander_i2c_command(dev, command, dev->buf_len);
>  
> -	if (read)
> +	if (read_write)
>  		return highlander_i2c_read(dev);
>  	else
>  		return highlander_i2c_write(dev);

Acked-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>

Thanks Wolfram.

-- 
Jean Delvare

Re: [PATCH] i2c/highlander: remover superflous variable

[Paul Mundt]

On Tue, Feb 02, 2010 at 01:17:43PM +0100, Wolfram Sang wrote:
> When cppcheck found this flaw
> 
> [./i2c/busses/i2c-highlander.c:284]: (style) Warning - using char variable in bit operation
> 
> it was noted that the 'read'-variable could just be removed as read_write can
> only be 0 or 1 anyhow. So, we remove the flaw by simplifying the code.
> 
> Reported-by: d binderman <dcb314-PkbjNfxxIARBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Cc: Paul Mundt <lethal-M7jkjyW5wf5g9hUCZPvPmw@public.gmane.org>
> Cc: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
> Cc: Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>

Fine with me.

Acked-by: Paul Mundt <lethal-M7jkjyW5wf5g9hUCZPvPmw@public.gmane.org>