* [PATCH] staging/iio/meter: fix the coding style problem
@ 2012-04-30 18:12 joseph daniel
0 siblings, 0 replies; 11+ messages in thread
From: joseph daniel @ 2012-04-30 18:12 UTC (permalink / raw)
To: Jonathan Cameron, Greg Kroah-Hartman, Wolfram Sang,
Paul Gortmaker, Lars-Peter Clausen, linux-iio, devel,
linux-kernel
Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH] staging/iio/meter: fix the coding style problem
@ 2012-04-30 18:21 joseph daniel
2012-04-30 18:32 ` Lars-Peter Clausen
0 siblings, 1 reply; 11+ messages in thread
From: joseph daniel @ 2012-04-30 18:21 UTC (permalink / raw)
To: Jonathan Cameron, Greg Kroah-Hartman, Wolfram Sang,
Paul Gortmaker, Lars-Peter Clausen, linux-iio, devel,
linux-kernel
Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] staging/iio/meter: fix the coding style problem
2012-04-30 18:21 joseph daniel
@ 2012-04-30 18:32 ` Lars-Peter Clausen
0 siblings, 0 replies; 11+ messages in thread
From: Lars-Peter Clausen @ 2012-04-30 18:32 UTC (permalink / raw)
To: joseph daniel
Cc: Jonathan Cameron, Greg Kroah-Hartman, Wolfram Sang,
Paul Gortmaker, linux-iio, devel, linux-kernel
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;
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] staging/iio/meter: fix the coding style problem
@ 2012-04-30 19:26 joseph daniel
2012-04-30 20:01 ` Lars-Peter Clausen
2012-04-30 20:04 ` Dan Carpenter
0 siblings, 2 replies; 11+ messages in thread
From: joseph daniel @ 2012-04-30 19:26 UTC (permalink / raw)
To: Jonathan Cameron, Greg Kroah-Hartman, Wolfram Sang,
Paul Gortmaker, Lars-Peter Clausen, linux-iio, devel,
linux-kernel
Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] staging/iio/meter: fix the coding style problem
2012-04-30 19:26 joseph daniel
@ 2012-04-30 20:01 ` Lars-Peter Clausen
2012-05-01 4:08 ` joseph daniel
2012-05-01 7:56 ` Jonathan Cameron
2012-04-30 20:04 ` Dan Carpenter
1 sibling, 2 replies; 11+ messages in thread
From: Lars-Peter Clausen @ 2012-04-30 20:01 UTC (permalink / raw)
To: joseph daniel
Cc: Jonathan Cameron, Greg Kroah-Hartman, Wolfram Sang,
Paul Gortmaker, linux-iio, devel, linux-kernel
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;
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] staging/iio/meter: fix the coding style problem
2012-04-30 20:01 ` Lars-Peter Clausen
@ 2012-05-01 4:08 ` joseph daniel
2012-05-01 7:56 ` Jonathan Cameron
1 sibling, 0 replies; 11+ messages in thread
From: joseph daniel @ 2012-05-01 4:08 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Jonathan Cameron, Greg Kroah-Hartman, Wolfram Sang,
Paul Gortmaker, linux-iio, devel, linux-kernel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] staging/iio/meter: fix the coding style problem
2012-04-30 20:01 ` Lars-Peter Clausen
2012-05-01 4:08 ` joseph daniel
@ 2012-05-01 7:56 ` Jonathan Cameron
2012-05-01 8:50 ` Lars-Peter Clausen
1 sibling, 1 reply; 11+ messages in thread
From: Jonathan Cameron @ 2012-05-01 7:56 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: joseph daniel, Greg Kroah-Hartman, Wolfram Sang, Paul Gortmaker,
linux-iio, devel, linux-kernel
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;
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] staging/iio/meter: fix the coding style problem
2012-05-01 7:56 ` Jonathan Cameron
@ 2012-05-01 8:50 ` Lars-Peter Clausen
0 siblings, 0 replies; 11+ messages in thread
From: Lars-Peter Clausen @ 2012-05-01 8:50 UTC (permalink / raw)
To: Jonathan Cameron
Cc: joseph daniel, Greg Kroah-Hartman, Wolfram Sang, Paul Gortmaker,
linux-iio, devel, linux-kernel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] staging/iio/meter: fix the coding style problem
2012-04-30 19:26 joseph daniel
2012-04-30 20:01 ` Lars-Peter Clausen
@ 2012-04-30 20:04 ` Dan Carpenter
2012-04-30 20:18 ` Dan Carpenter
1 sibling, 1 reply; 11+ messages in thread
From: Dan Carpenter @ 2012-04-30 20:04 UTC (permalink / raw)
To: joseph daniel
Cc: Jonathan Cameron, Greg Kroah-Hartman, Wolfram Sang,
Paul Gortmaker, Lars-Peter Clausen, linux-iio, devel,
linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] staging/iio/meter: fix the coding style problem
2012-04-30 20:04 ` Dan Carpenter
@ 2012-04-30 20:18 ` Dan Carpenter
2012-05-01 4:09 ` joseph daniel
0 siblings, 1 reply; 11+ messages in thread
From: Dan Carpenter @ 2012-04-30 20:18 UTC (permalink / raw)
To: joseph daniel
Cc: devel, Lars-Peter Clausen, linux-iio, Greg Kroah-Hartman,
Wolfram Sang, linux-kernel, Paul Gortmaker, Jonathan Cameron
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] staging/iio/meter: fix the coding style problem
2012-04-30 20:18 ` Dan Carpenter
@ 2012-05-01 4:09 ` joseph daniel
0 siblings, 0 replies; 11+ messages in thread
From: joseph daniel @ 2012-05-01 4:09 UTC (permalink / raw)
To: Dan Carpenter
Cc: devel, Lars-Peter Clausen, linux-iio, Greg Kroah-Hartman,
Wolfram Sang, linux-kernel, Paul Gortmaker, Jonathan Cameron
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2012-05-01 8:50 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-04-30 18:12 [PATCH] staging/iio/meter: fix the coding style problem joseph daniel
-- strict thread matches above, loose matches on Subject: below --
2012-04-30 18:21 joseph daniel
2012-04-30 18:32 ` Lars-Peter Clausen
2012-04-30 19:26 joseph daniel
2012-04-30 20:01 ` Lars-Peter Clausen
2012-05-01 4:08 ` joseph daniel
2012-05-01 7:56 ` Jonathan Cameron
2012-05-01 8:50 ` Lars-Peter Clausen
2012-04-30 20:04 ` Dan Carpenter
2012-04-30 20:18 ` Dan Carpenter
2012-05-01 4:09 ` joseph daniel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).