[PATCH] iio:potentiostat:lmp91000: solve codestyle WARNINGs and CHECKs

[PATCH] iio:potentiostat:lmp91000: solve codestyle WARNINGs and CHECKs

[Lucas Oshiro]

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

Re: [PATCH] iio:potentiostat:lmp91000: solve codestyle WARNINGs and CHECKs

[Joe Perches]

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

Re: [PATCH] iio:potentiostat:lmp91000: solve codestyle WARNINGs and CHECKs

[LSO]

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?
> 
> 

Re: [PATCH] iio:potentiostat:lmp91000: solve codestyle WARNINGs and CHECKs

[Jonathan Cameron]

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

> > 
> >   

Re: [PATCH] iio:potentiostat:lmp91000: solve codestyle WARNINGs and CHECKs

[Lucas Oshiro]

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
> 
>>>
>>>    
>