Linux-IIO Archive on lore.kernel.org
 help / Atom feed
* [PATCH] iio:potentiostat:lmp91000: solve codestyle WARNINGs and CHECKs
@ 2019-01-29 18:36 Lucas Oshiro
  2019-01-29 22:48 ` Joe Perches
  0 siblings, 1 reply; 5+ messages in thread
From: Lucas Oshiro @ 2019-01-29 18:36 UTC (permalink / raw)
  To: jic23, knaack.h, lars, pmeerw
  Cc: linux-iio, linux-kernel, kernel-usp, Anderson Reis

Solve most of the checkpatch.pl WARNINGs and CHECKs on lmp9100.c. They
are the following:

lmp91000.c:116: CHECK: Unnecessary parentheses around 'state != channel'
lmp91000.c:116: CHECK: Unnecessary parentheses around 'channel == LMP91000_REG_MODECN_TEMP'
lmp91000.c:214: CHECK: braces {} should be used on all arms of this statement
lmp91000.c:216: CHECK: Unbalanced braces around else statement
lmp91000.c:258: WARNING: line over 80 characters
lmp91000.c:279: CHECK: Please don't use multiple blank lines

Signed-off-by: Lucas Oshiro <lucasseikioshiro@gmail.com>
Signed-off-by: Anderson Reis <andersonreisrosa@gmail.com>
---
 drivers/iio/potentiostat/lmp91000.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c
index 90e895adf997..6dba26121a62 100644
--- a/drivers/iio/potentiostat/lmp91000.c
+++ b/drivers/iio/potentiostat/lmp91000.c
@@ -113,7 +113,7 @@ static int lmp91000_read(struct lmp91000_data *data, int channel, int *val)
 		return -EINVAL;
 
 	/* delay till first temperature reading is complete */
-	if ((state != channel) && (channel == LMP91000_REG_MODECN_TEMP))
+	if (state != channel && channel == LMP91000_REG_MODECN_TEMP)
 		usleep_range(3000, 4000);
 
 	data->chan_select = channel != LMP91000_REG_MODECN_3LEAD;
@@ -211,9 +211,9 @@ static int lmp91000_read_config(struct lmp91000_data *data)
 
 	ret = of_property_read_u32(np, "ti,tia-gain-ohm", &val);
 	if (ret) {
-		if (of_property_read_bool(np, "ti,external-tia-resistor"))
+		if (of_property_read_bool(np, "ti,external-tia-resistor")) {
 			val = 0;
-		else {
+		} else {
 			dev_err(dev, "no ti,tia-gain-ohm defined");
 			return ret;
 		}
@@ -255,8 +255,8 @@ static int lmp91000_read_config(struct lmp91000_data *data)
 
 	regmap_write(data->regmap, LMP91000_REG_LOCK, 0);
 	regmap_write(data->regmap, LMP91000_REG_TIACN, reg);
-	regmap_write(data->regmap, LMP91000_REG_REFCN, LMP91000_REG_REFCN_EXT_REF
-					| LMP91000_REG_REFCN_50_ZERO);
+	regmap_write(data->regmap, LMP91000_REG_REFCN,
+		     LMP91000_REG_REFCN_EXT_REF	| LMP91000_REG_REFCN_50_ZERO);
 	regmap_write(data->regmap, LMP91000_REG_LOCK, 1);
 
 	return 0;
@@ -276,7 +276,6 @@ static int lmp91000_buffer_cb(const void *val, void *private)
 static const struct iio_trigger_ops lmp91000_trigger_ops = {
 };
 
-
 static int lmp91000_buffer_preenable(struct iio_dev *indio_dev)
 {
 	struct lmp91000_data *data = iio_priv(indio_dev);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] iio:potentiostat:lmp91000: solve codestyle WARNINGs and CHECKs
  2019-01-29 18:36 [PATCH] iio:potentiostat:lmp91000: solve codestyle WARNINGs and CHECKs Lucas Oshiro
@ 2019-01-29 22:48 ` Joe Perches
  2019-02-01 14:29   ` LSO
  0 siblings, 1 reply; 5+ messages in thread
From: Joe Perches @ 2019-01-29 22:48 UTC (permalink / raw)
  To: Lucas Oshiro, jic23, knaack.h, lars, pmeerw
  Cc: linux-iio, linux-kernel, kernel-usp, Anderson Reis

On Tue, 2019-01-29 at 16:36 -0200, Lucas Oshiro wrote:
> Solve most of the checkpatch.pl WARNINGs and CHECKs on lmp9100.c. They
> are the following:
> 
> lmp91000.c:116: CHECK: Unnecessary parentheses around 'state != channel'
> lmp91000.c:116: CHECK: Unnecessary parentheses around 'channel == LMP91000_REG_MODECN_TEMP'
> lmp91000.c:214: CHECK: braces {} should be used on all arms of this statement
> lmp91000.c:216: CHECK: Unbalanced braces around else statement
> lmp91000.c:258: WARNING: line over 80 characters
> lmp91000.c:279: CHECK: Please don't use multiple blank lines

Some will say this is too many things to do at once.
I think it's mostly fine, but there are a few nits
that also could use fixing.

> diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c
[]
> @@ -211,9 +211,9 @@ static int lmp91000_read_config(struct lmp91000_data *data)
>  
>  	ret = of_property_read_u32(np, "ti,tia-gain-ohm", &val);
>  	if (ret) {
> -		if (of_property_read_bool(np, "ti,external-tia-resistor"))
> +		if (of_property_read_bool(np, "ti,external-tia-resistor")) {
>  			val = 0;
> -		else {
> +		} else {
>  			dev_err(dev, "no ti,tia-gain-ohm defined");
>  			return ret;
>  		}

This could use inverting the test

	if (ret) {
		if (!of_property_read_bool(...)) {
			dev_err(dev, "no ti,ti-gain-ohm defined\n");
			return ret;
		}
		val = 0;
	}

Also the dev_err is missing a '\n' termination



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] iio:potentiostat:lmp91000: solve codestyle WARNINGs and CHECKs
  2019-01-29 22:48 ` Joe Perches
@ 2019-02-01 14:29   ` LSO
  2019-02-02 10:00     ` Jonathan Cameron
  0 siblings, 1 reply; 5+ messages in thread
From: LSO @ 2019-02-01 14:29 UTC (permalink / raw)
  To: Joe Perches, jic23, knaack.h, lars, pmeerw
  Cc: linux-iio, linux-kernel, kernel-usp, Anderson Reis

Thanks for the review!

On 29/01/2019 20:48, Joe Perches wrote:
> On Tue, 2019-01-29 at 16:36 -0200, Lucas Oshiro wrote:
>> Solve most of the checkpatch.pl WARNINGs and CHECKs on lmp9100.c. They
>> are the following:
>>
>> lmp91000.c:116: CHECK: Unnecessary parentheses around 'state != channel'
>> lmp91000.c:116: CHECK: Unnecessary parentheses around 'channel == LMP91000_REG_MODECN_TEMP'
>> lmp91000.c:214: CHECK: braces {} should be used on all arms of this statement
>> lmp91000.c:216: CHECK: Unbalanced braces around else statement
>> lmp91000.c:258: WARNING: line over 80 characters
>> lmp91000.c:279: CHECK: Please don't use multiple blank lines
> 
> Some will say this is too many things to do at once.
> I think it's mostly fine, but there are a few nits
> that also could use fixing.
> 
>> diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c
> []
>> @@ -211,9 +211,9 @@ static int lmp91000_read_config(struct lmp91000_data *data)
>>   
>>   	ret = of_property_read_u32(np, "ti,tia-gain-ohm", &val);
>>   	if (ret) {
>> -		if (of_property_read_bool(np, "ti,external-tia-resistor"))
>> +		if (of_property_read_bool(np, "ti,external-tia-resistor")) {
>>   			val = 0;
>> -		else {
>> +		} else {
>>   			dev_err(dev, "no ti,tia-gain-ohm defined");
>>   			return ret;
>>   		}
> 
> This could use inverting the test
> 
> 	if (ret) {
> 		if (!of_property_read_bool(...)) {
> 			dev_err(dev, "no ti,ti-gain-ohm defined\n");
> 			return ret;
> 		}
> 		val = 0;
> 	}
>
Thanks for the suggestion, I'll do that in the next version.

> Also the dev_err is missing a '\n' termination

My aim in this patch was only solve style problems, but I
can put that missing '\n' too. Do you think it could be done
in the same commit or it's a better idea do it in another
commit and send both as a patchset?
> 
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] iio:potentiostat:lmp91000: solve codestyle WARNINGs and CHECKs
  2019-02-01 14:29   ` LSO
@ 2019-02-02 10:00     ` Jonathan Cameron
  2019-02-10  0:05       ` Lucas Oshiro
  0 siblings, 1 reply; 5+ messages in thread
From: Jonathan Cameron @ 2019-02-02 10:00 UTC (permalink / raw)
  To: LSO
  Cc: Joe Perches, knaack.h, lars, pmeerw, linux-iio, linux-kernel,
	kernel-usp, Anderson Reis

On Fri, 1 Feb 2019 12:29:11 -0200
LSO <lucasseikioshiro@gmail.com> wrote:

> Thanks for the review!
> 
> On 29/01/2019 20:48, Joe Perches wrote:
> > On Tue, 2019-01-29 at 16:36 -0200, Lucas Oshiro wrote:  
> >> Solve most of the checkpatch.pl WARNINGs and CHECKs on lmp9100.c. They
> >> are the following:
> >>
> >> lmp91000.c:116: CHECK: Unnecessary parentheses around 'state != channel'
> >> lmp91000.c:116: CHECK: Unnecessary parentheses around 'channel == LMP91000_REG_MODECN_TEMP'
> >> lmp91000.c:214: CHECK: braces {} should be used on all arms of this statement
> >> lmp91000.c:216: CHECK: Unbalanced braces around else statement
> >> lmp91000.c:258: WARNING: line over 80 characters
> >> lmp91000.c:279: CHECK: Please don't use multiple blank lines  
> > 
> > Some will say this is too many things to do at once.
> > I think it's mostly fine, but there are a few nits
> > that also could use fixing.

Always a case of personal judgement.
I agree that this one 'just' falls on the side of not too many things for one
patch.  If there had been a few more items then it would have been too much.

I would also have been happy with it broken out.  If I had been spinning
it myself, I would have done it as 3 patches in pairs from your list
above with the last one grouping the white space changes.

The test inversion below is also stretching beyond simple style
so probably should be broken out.

> >   
> >> diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c  
> > []  
> >> @@ -211,9 +211,9 @@ static int lmp91000_read_config(struct lmp91000_data *data)
> >>   
> >>   	ret = of_property_read_u32(np, "ti,tia-gain-ohm", &val);
> >>   	if (ret) {
> >> -		if (of_property_read_bool(np, "ti,external-tia-resistor"))
> >> +		if (of_property_read_bool(np, "ti,external-tia-resistor")) {
> >>   			val = 0;
> >> -		else {
> >> +		} else {
> >>   			dev_err(dev, "no ti,tia-gain-ohm defined");
> >>   			return ret;
> >>   		}  
> > 
> > This could use inverting the test
> > 
> > 	if (ret) {
> > 		if (!of_property_read_bool(...)) {
> > 			dev_err(dev, "no ti,ti-gain-ohm defined\n");
> > 			return ret;
> > 		}
> > 		val = 0;
> > 	}
> >  
> Thanks for the suggestion, I'll do that in the next version.
> 
> > Also the dev_err is missing a '\n' termination  
> 
> My aim in this patch was only solve style problems, but I
> can put that missing '\n' too. Do you think it could be done
> in the same commit or it's a better idea do it in another
> commit and send both as a patchset?

Separate commit given as you say it's not style and this one has
enough different things in it already!

Thanks,

Jonathan

> > 
> >   


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] iio:potentiostat:lmp91000: solve codestyle WARNINGs and CHECKs
  2019-02-02 10:00     ` Jonathan Cameron
@ 2019-02-10  0:05       ` Lucas Oshiro
  0 siblings, 0 replies; 5+ messages in thread
From: Lucas Oshiro @ 2019-02-10  0:05 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Joe Perches, knaack.h, lars, pmeerw, linux-iio, linux-kernel,
	kernel-usp, Anderson Reis

Thanks! I'll send those changes in my next patchset.

On 02/02/2019 08:00, Jonathan Cameron wrote:
> On Fri, 1 Feb 2019 12:29:11 -0200
> LSO <lucasseikioshiro@gmail.com> wrote:
> 
>> Thanks for the review!
>>
>> On 29/01/2019 20:48, Joe Perches wrote:
>>> On Tue, 2019-01-29 at 16:36 -0200, Lucas Oshiro wrote:
>>>> Solve most of the checkpatch.pl WARNINGs and CHECKs on lmp9100.c. They
>>>> are the following:
>>>>
>>>> lmp91000.c:116: CHECK: Unnecessary parentheses around 'state != channel'
>>>> lmp91000.c:116: CHECK: Unnecessary parentheses around 'channel == LMP91000_REG_MODECN_TEMP'
>>>> lmp91000.c:214: CHECK: braces {} should be used on all arms of this statement
>>>> lmp91000.c:216: CHECK: Unbalanced braces around else statement
>>>> lmp91000.c:258: WARNING: line over 80 characters
>>>> lmp91000.c:279: CHECK: Please don't use multiple blank lines
>>>
>>> Some will say this is too many things to do at once.
>>> I think it's mostly fine, but there are a few nits
>>> that also could use fixing.
> 
> Always a case of personal judgement.
> I agree that this one 'just' falls on the side of not too many things for one
> patch.  If there had been a few more items then it would have been too much.
> 
> I would also have been happy with it broken out.  If I had been spinning
> it myself, I would have done it as 3 patches in pairs from your list
> above with the last one grouping the white space changes.
> 
> The test inversion below is also stretching beyond simple style
> so probably should be broken out.
> 
>>>    
>>>> diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c
>>> []
>>>> @@ -211,9 +211,9 @@ static int lmp91000_read_config(struct lmp91000_data *data)
>>>>    
>>>>    	ret = of_property_read_u32(np, "ti,tia-gain-ohm", &val);
>>>>    	if (ret) {
>>>> -		if (of_property_read_bool(np, "ti,external-tia-resistor"))
>>>> +		if (of_property_read_bool(np, "ti,external-tia-resistor")) {
>>>>    			val = 0;
>>>> -		else {
>>>> +		} else {
>>>>    			dev_err(dev, "no ti,tia-gain-ohm defined");
>>>>    			return ret;
>>>>    		}
>>>
>>> This could use inverting the test
>>>
>>> 	if (ret) {
>>> 		if (!of_property_read_bool(...)) {
>>> 			dev_err(dev, "no ti,ti-gain-ohm defined\n");
>>> 			return ret;
>>> 		}
>>> 		val = 0;
>>> 	}
>>>   
>> Thanks for the suggestion, I'll do that in the next version.
>>
>>> Also the dev_err is missing a '\n' termination
>>
>> My aim in this patch was only solve style problems, but I
>> can put that missing '\n' too. Do you think it could be done
>> in the same commit or it's a better idea do it in another
>> commit and send both as a patchset?
> 
> Separate commit given as you say it's not style and this one has
> enough different things in it already!
> 
> Thanks,
> 
> Jonathan
> 
>>>
>>>    
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-29 18:36 [PATCH] iio:potentiostat:lmp91000: solve codestyle WARNINGs and CHECKs Lucas Oshiro
2019-01-29 22:48 ` Joe Perches
2019-02-01 14:29   ` LSO
2019-02-02 10:00     ` Jonathan Cameron
2019-02-10  0:05       ` Lucas Oshiro

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org linux-iio@archiver.kernel.org
	public-inbox-index linux-iio


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/ public-inbox