[PATCH] staging/iio/meter: fix the coding style problem

[PATCH] staging/iio/meter: fix the coding style problem

[joseph daniel]

Signed-off-by: joseph daniel <josephdanielwalter@gmail.com>
---
 Please consider this patch, previous one is wrong because my
 tabspace in vim was 4, instead 8. sorry..

 drivers/staging/iio/meter/ade7854-i2c.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
index 1e1faa0..ba97d6e 100644
--- a/drivers/staging/iio/meter/ade7854-i2c.c
+++ b/drivers/staging/iio/meter/ade7854-i2c.c
@@ -195,7 +195,10 @@ static int ade7854_i2c_read_reg_32(struct device *dev,
 	if (ret)
 		goto out;
 
-	*val = (st->rx[0] << 24) | (st->rx[1] << 16) | (st->rx[2] << 8) | st->rx[3];
+	*val =  (st->rx[0] << 24) |
+		(st->rx[1] << 16) |
+		(st->rx[2] << 8)  |
+		 st->rx[3];
 out:
 	mutex_unlock(&st->buf_lock);
 	return ret;
-- 
1.7.9.5

Re: [PATCH] staging/iio/meter: fix the coding style problem

[Lars-Peter Clausen]

On 04/30/2012 08:21 PM, joseph daniel wrote:
> Signed-off-by: joseph daniel <josephdanielwalter@gmail.com>
> ---
>  Please consider this patch, previous one is wrong because my
>  tabspace in vim was 4, instead 8. sorry..
> 
>  drivers/staging/iio/meter/ade7854-i2c.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
> index 1e1faa0..ba97d6e 100644
> --- a/drivers/staging/iio/meter/ade7854-i2c.c
> +++ b/drivers/staging/iio/meter/ade7854-i2c.c
> @@ -195,7 +195,10 @@ static int ade7854_i2c_read_reg_32(struct device *dev,
>  	if (ret)
>  		goto out;
>  
> -	*val = (st->rx[0] << 24) | (st->rx[1] << 16) | (st->rx[2] << 8) | st->rx[3];
> +	*val =  (st->rx[0] << 24) |
> +		(st->rx[1] << 16) |
> +		(st->rx[2] << 8)  |
> +		 st->rx[3];

Nack. In my opinion this doesn't make the code more readable. 80 chars per line
should be though of as a softlimit.

But this looks like something that could use be32_to_cpu though.

>  out:
>  	mutex_unlock(&st->buf_lock);
>  	return ret;

Re: [PATCH] staging/iio/meter: fix the coding style problem

[Lars-Peter Clausen]

On 05/01/2012 09:56 AM, Jonathan Cameron wrote:
> On 4/30/2012 9:01 PM, Lars-Peter Clausen wrote:
>> On 04/30/2012 09:26 PM, joseph daniel wrote:
>>> Signed-off-by: joseph daniel<josephdanielwalter@gmail.com>
>>> ---
>>> Hi Lars,
>>>     Thanks for review. how about the below change?
>>>   drivers/staging/iio/meter/ade7854-i2c.c |    6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/iio/meter/ade7854-i2c.c
>>> b/drivers/staging/iio/meter/ade7854-i2c.c
>>> index 1e1faa0..52bdb05 100644
>>> --- a/drivers/staging/iio/meter/ade7854-i2c.c
>>> +++ b/drivers/staging/iio/meter/ade7854-i2c.c
>>> @@ -181,6 +181,7 @@ static int ade7854_i2c_read_reg_32(struct device
>>> *dev,
>>>   {
>>>       struct iio_dev *indio_dev = dev_get_drvdata(dev);
>>>       struct ade7854_state *st = iio_priv(indio_dev);
>>> +    uint32_t value;
>>>       int ret;
>>>
>>>       mutex_lock(&st->buf_lock);
>>> @@ -195,7 +196,10 @@ static int ade7854_i2c_read_reg_32(struct device
>>> *dev,
>>>       if (ret)
>>>           goto out;
>>>
>>> -    *val = (st->rx[0]<<  24) | (st->rx[1]<<  16) | (st->rx[2]<<  8)
>>> | st->rx[3];
>>> +    memcpy(&value, st->rx, sizeof(uint32_t));
>>> +
>> Uhm, yes, you are right st->rx is unaligned. The memcpy is not
>> necessary though
>> if you use get_unaligned_be32. Sorry for the pointer to the wrong
>> function.
> Or we could just force the alignment of st->rx? Might not be worth
> bothering.

Yes, but be32_to_cpu wants a be32 and there is another functions which only
does a 16 bit access on rx, so we'd need to cast and things get messy. Also for
tx we store first a 16bit value and then a 32bit value and while it is possible
to get this properly aligned this gets a bit messy too. I think we are fine
with {get,set}_unaligned for now.

Re: [PATCH] staging/iio/meter: fix the coding style problem

[Jonathan Cameron]

On 4/30/2012 9:01 PM, Lars-Peter Clausen wrote:
> On 04/30/2012 09:26 PM, joseph daniel wrote:
>> Signed-off-by: joseph daniel<josephdanielwalter@gmail.com>
>> ---
>> Hi Lars,
>> 	Thanks for review. how about the below change?
>>   drivers/staging/iio/meter/ade7854-i2c.c |    6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
>> index 1e1faa0..52bdb05 100644
>> --- a/drivers/staging/iio/meter/ade7854-i2c.c
>> +++ b/drivers/staging/iio/meter/ade7854-i2c.c
>> @@ -181,6 +181,7 @@ static int ade7854_i2c_read_reg_32(struct device *dev,
>>   {
>>   	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>>   	struct ade7854_state *st = iio_priv(indio_dev);
>> +	uint32_t value;
>>   	int ret;
>>
>>   	mutex_lock(&st->buf_lock);
>> @@ -195,7 +196,10 @@ static int ade7854_i2c_read_reg_32(struct device *dev,
>>   	if (ret)
>>   		goto out;
>>
>> -	*val = (st->rx[0]<<  24) | (st->rx[1]<<  16) | (st->rx[2]<<  8) | st->rx[3];
>> +	memcpy(&value, st->rx, sizeof(uint32_t));
>> +
> Uhm, yes, you are right st->rx is unaligned. The memcpy is not necessary though
> if you use get_unaligned_be32. Sorry for the pointer to the wrong function.
Or we could just force the alignment of st->rx? Might not be worth 
bothering.
>
> Btw. there are a few other locations in this driver which could use the
> put_unaligned_be* and get_unaligned_be* functions. Would be great if you could
> convert these as well.
>
>> +	*val = be32_to_cpu(value);
>> +
>>   out:
>>   	mutex_unlock(&st->buf_lock);
>>   	return ret;

Re: [PATCH] staging/iio/meter: fix the coding style problem

[joseph daniel]

On Tue, May 1, 2012 at 2:18 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Mon, Apr 30, 2012 at 11:04:44PM +0300, Dan Carpenter wrote:
>> On Tue, May 01, 2012 at 01:26:17AM +0600, joseph daniel wrote:
>> >
>> > Signed-off-by: joseph daniel <josephdanielwalter@gmail.com>
>> > ---
>> > Hi Lars,
>> >     Thanks for review. how about the below change?
>
> Lar's review is the correct one.  It's cleaner to use
> get_unaligned_be32().  You'll need to include <asm/unaligned.h>
>
Thanks For the info Dan. i will be taking your comments in the code change.
> regards,
> dan carpenter
>
Thanks,
Joseph.

Re: [PATCH] staging/iio/meter: fix the coding style problem

[joseph daniel]

On Tue, May 1, 2012 at 2:18 AM, Dan Carpenter <dan.carpenter@oracle.com> wr=
ote:
> On Mon, Apr 30, 2012 at 11:04:44PM +0300, Dan Carpenter wrote:
>> On Tue, May 01, 2012 at 01:26:17AM +0600, joseph daniel wrote:
>> >
>> > Signed-off-by: joseph daniel <josephdanielwalter@gmail.com>
>> > ---
>> > Hi Lars,
>> > =A0 =A0 Thanks for review. how about the below change?
>
> Lar's review is the correct one. =A0It's cleaner to use
> get_unaligned_be32(). =A0You'll need to include <asm/unaligned.h>
>
Thanks For the info Dan. i will be taking your comments in the code change.
> regards,
> dan carpenter
>
Thanks,
Joseph.

Re: [PATCH] staging/iio/meter: fix the coding style problem

[joseph daniel]

On Tue, May 1, 2012 at 2:01 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 04/30/2012 09:26 PM, joseph daniel wrote:
>> Signed-off-by: joseph daniel <josephdanielwalter@gmail.com>
>> ---
>> Hi Lars,
>>       Thanks for review. how about the below change?
>>  drivers/staging/iio/meter/ade7854-i2c.c |    6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
>> index 1e1faa0..52bdb05 100644
>> --- a/drivers/staging/iio/meter/ade7854-i2c.c
>> +++ b/drivers/staging/iio/meter/ade7854-i2c.c
>> @@ -181,6 +181,7 @@ static int ade7854_i2c_read_reg_32(struct device *dev,
>>  {
>>       struct iio_dev *indio_dev = dev_get_drvdata(dev);
>>       struct ade7854_state *st = iio_priv(indio_dev);
>> +     uint32_t value;
>>       int ret;
>>
>>       mutex_lock(&st->buf_lock);
>> @@ -195,7 +196,10 @@ static int ade7854_i2c_read_reg_32(struct device *dev,
>>       if (ret)
>>               goto out;
>>
>> -     *val = (st->rx[0] << 24) | (st->rx[1] << 16) | (st->rx[2] << 8) | st->rx[3];
>> +     memcpy(&value, st->rx, sizeof(uint32_t));
>> +
>
> Uhm, yes, you are right st->rx is unaligned. The memcpy is not necessary though
> if you use get_unaligned_be32. Sorry for the pointer to the wrong function.
>
> Btw. there are a few other locations in this driver which could use the
> put_unaligned_be* and get_unaligned_be* functions. Would be great if you could
> convert these as well.
>
>> +     *val = be32_to_cpu(value);
>> +
>>  out:
>>       mutex_unlock(&st->buf_lock);
>>       return ret;
>

Thanks for review. i will be glad doing those changes. i will send
patch as soon as i get free.

Thanks.
Joseph.

Re: [PATCH] staging/iio/meter: fix the coding style problem

[joseph daniel]

On Tue, May 1, 2012 at 2:01 AM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 04/30/2012 09:26 PM, joseph daniel wrote:
>> Signed-off-by: joseph daniel <josephdanielwalter@gmail.com>
>> ---
>> Hi Lars,
>> =A0 =A0 =A0 Thanks for review. how about the below change?
>> =A0drivers/staging/iio/meter/ade7854-i2c.c | =A0 =A06 +++++-
>> =A01 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/i=
io/meter/ade7854-i2c.c
>> index 1e1faa0..52bdb05 100644
>> --- a/drivers/staging/iio/meter/ade7854-i2c.c
>> +++ b/drivers/staging/iio/meter/ade7854-i2c.c
>> @@ -181,6 +181,7 @@ static int ade7854_i2c_read_reg_32(struct device *de=
v,
>> =A0{
>> =A0 =A0 =A0 struct iio_dev *indio_dev =3D dev_get_drvdata(dev);
>> =A0 =A0 =A0 struct ade7854_state *st =3D iio_priv(indio_dev);
>> + =A0 =A0 uint32_t value;
>> =A0 =A0 =A0 int ret;
>>
>> =A0 =A0 =A0 mutex_lock(&st->buf_lock);
>> @@ -195,7 +196,10 @@ static int ade7854_i2c_read_reg_32(struct device *d=
ev,
>> =A0 =A0 =A0 if (ret)
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto out;
>>
>> - =A0 =A0 *val =3D (st->rx[0] << 24) | (st->rx[1] << 16) | (st->rx[2] <<=
 8) | st->rx[3];
>> + =A0 =A0 memcpy(&value, st->rx, sizeof(uint32_t));
>> +
>
> Uhm, yes, you are right st->rx is unaligned. The memcpy is not necessary =
though
> if you use get_unaligned_be32. Sorry for the pointer to the wrong functio=
n.
>
> Btw. there are a few other locations in this driver which could use the
> put_unaligned_be* and get_unaligned_be* functions. Would be great if you =
could
> convert these as well.
>
>> + =A0 =A0 *val =3D be32_to_cpu(value);
>> +
>> =A0out:
>> =A0 =A0 =A0 mutex_unlock(&st->buf_lock);
>> =A0 =A0 =A0 return ret;
>

Thanks for review. i will be glad doing those changes. i will send
patch as soon as i get free.

Thanks.
Joseph.

Re: [PATCH] staging/iio/meter: fix the coding style problem

[Dan Carpenter]

On Mon, Apr 30, 2012 at 11:04:44PM +0300, Dan Carpenter wrote:
> On Tue, May 01, 2012 at 01:26:17AM +0600, joseph daniel wrote:
> > 
> > Signed-off-by: joseph daniel <josephdanielwalter@gmail.com>
> > ---
> > Hi Lars,
> > 	Thanks for review. how about the below change?

Lar's review is the correct one.  It's cleaner to use
get_unaligned_be32().  You'll need to include <asm/unaligned.h>

regards,
dan carpenter

Re: [PATCH] staging/iio/meter: fix the coding style problem

[Dan Carpenter]

On Tue, May 01, 2012 at 01:26:17AM +0600, joseph daniel wrote:
> 
> Signed-off-by: joseph daniel <josephdanielwalter@gmail.com>
> ---
> Hi Lars,
> 	Thanks for review. how about the below change?

Slow down.  There is no rush.  It's better to sit on patches
overnight so you can review them carefully the next day.  Take the
time to write a proper changelog...

>  drivers/staging/iio/meter/ade7854-i2c.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
> index 1e1faa0..52bdb05 100644
> --- a/drivers/staging/iio/meter/ade7854-i2c.c
> +++ b/drivers/staging/iio/meter/ade7854-i2c.c
> @@ -181,6 +181,7 @@ static int ade7854_i2c_read_reg_32(struct device *dev,
>  {
>  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>  	struct ade7854_state *st = iio_priv(indio_dev);
> +	uint32_t value;

Use the endian notation here:

	__be32 value;

http://lwn.net/Articles/205624/

The rest looks fine to me.  Package it up like a proper patch and
resend.

regards,
dan carpenter

Re: [PATCH] staging/iio/meter: fix the coding style problem

[Lars-Peter Clausen]

On 04/30/2012 09:26 PM, joseph daniel wrote:
> Signed-off-by: joseph daniel <josephdanielwalter@gmail.com>
> ---
> Hi Lars,
> 	Thanks for review. how about the below change?
>  drivers/staging/iio/meter/ade7854-i2c.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
> index 1e1faa0..52bdb05 100644
> --- a/drivers/staging/iio/meter/ade7854-i2c.c
> +++ b/drivers/staging/iio/meter/ade7854-i2c.c
> @@ -181,6 +181,7 @@ static int ade7854_i2c_read_reg_32(struct device *dev,
>  {
>  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>  	struct ade7854_state *st = iio_priv(indio_dev);
> +	uint32_t value;
>  	int ret;
>  
>  	mutex_lock(&st->buf_lock);
> @@ -195,7 +196,10 @@ static int ade7854_i2c_read_reg_32(struct device *dev,
>  	if (ret)
>  		goto out;
>  
> -	*val = (st->rx[0] << 24) | (st->rx[1] << 16) | (st->rx[2] << 8) | st->rx[3];
> +	memcpy(&value, st->rx, sizeof(uint32_t));
> +

Uhm, yes, you are right st->rx is unaligned. The memcpy is not necessary though
if you use get_unaligned_be32. Sorry for the pointer to the wrong function.

Btw. there are a few other locations in this driver which could use the
put_unaligned_be* and get_unaligned_be* functions. Would be great if you could
convert these as well.

> +	*val = be32_to_cpu(value);
> +
>  out:
>  	mutex_unlock(&st->buf_lock);
>  	return ret;

[PATCH] staging/iio/meter: fix the coding style problem

[joseph daniel]

Signed-off-by: joseph daniel <josephdanielwalter@gmail.com>
---
Hi Lars,
	Thanks for review. how about the below change?
 drivers/staging/iio/meter/ade7854-i2c.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
index 1e1faa0..52bdb05 100644
--- a/drivers/staging/iio/meter/ade7854-i2c.c
+++ b/drivers/staging/iio/meter/ade7854-i2c.c
@@ -181,6 +181,7 @@ static int ade7854_i2c_read_reg_32(struct device *dev,
 {
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct ade7854_state *st = iio_priv(indio_dev);
+	uint32_t value;
 	int ret;
 
 	mutex_lock(&st->buf_lock);
@@ -195,7 +196,10 @@ static int ade7854_i2c_read_reg_32(struct device *dev,
 	if (ret)
 		goto out;
 
-	*val = (st->rx[0] << 24) | (st->rx[1] << 16) | (st->rx[2] << 8) | st->rx[3];
+	memcpy(&value, st->rx, sizeof(uint32_t));
+
+	*val = be32_to_cpu(value);
+
 out:
 	mutex_unlock(&st->buf_lock);
 	return ret;
-- 
1.7.9.5

[PATCH] staging/iio/meter: fix the coding style problem

[joseph daniel]

Signed-off-by: joseph daniel <josephdanielwalter@gmail.com>
---
 drivers/staging/iio/meter/ade7854-i2c.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/iio/meter/ade7854-i2c.c b/drivers/staging/iio/meter/ade7854-i2c.c
index 1e1faa0..d55cb87 100644
--- a/drivers/staging/iio/meter/ade7854-i2c.c
+++ b/drivers/staging/iio/meter/ade7854-i2c.c
@@ -195,7 +195,10 @@ static int ade7854_i2c_read_reg_32(struct device *dev,
 	if (ret)
 		goto out;
 
-	*val = (st->rx[0] << 24) | (st->rx[1] << 16) | (st->rx[2] << 8) | st->rx[3];
+	*val = (st->rx[0] << 24) |
+		   (st->rx[1] << 16) |
+		   (st->rx[2] << 8)  |
+		   st->rx[3];
 out:
 	mutex_unlock(&st->buf_lock);
 	return ret;
-- 
1.7.9.5