[PATCH 0/5] iio:potentiostat:lmp91000: Adjust codestyle, and minor cleanup changes

[PATCH 0/5] iio:potentiostat:lmp91000: Adjust codestyle, and minor cleanup changes

[Lucas Oshiro]

Hi,

We solved some checkpath.el CHECKs and WARNINGs. We also inverted the arms of
an if statement, in order to make the code smaller as the else statement was
supressed. We added a missing '\n' on a dev_err message.

Lucas Oshiro (5):
  iio:potentiostat:lmp91000: remove unnecessary parentheses
  iio:potentiostat:lmp91000: insert braces around if arms
  iio:potentiostat:lmp91000: reduce line width and remove blank line
  iio:potentiostat:lmp91000: invert if statement
  iio:potentiostat:lmp91000: add '\n' on dev_err

 drivers/iio/potentiostat/lmp91000.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

-- 
2.20.1

[PATCH 1/5] iio:potentiostat:lmp91000: remove unnecessary parentheses

[Lucas Oshiro]

Remove unnecessary parentheses on line 116, and solve these checkpatch.pl
CHECKs:

- lmp91000.c:116: CHECK: Unnecessary parentheses around 'state != channel'
- lmp91000.c:116: CHECK: Unnecessary parentheses around 'channel == LMP91000_REG_MODECN_TEMP'

Signed-off-by: Lucas Oshiro <lucasseikioshiro@gmail.com>
Signed-off-by: Anderson Reis <andersonreisrosa@gmail.com>
---
 drivers/iio/potentiostat/lmp91000.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c
index 90e895adf997..03d277621861 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;
-- 
2.20.1

[PATCH 2/5] iio:potentiostat:lmp91000: insert braces around if arms

[Lucas Oshiro]

Insert braces around all arms of if statement starting on line 214,
and solve these checkpath.el CHECKs:

- lmp91000.c:214: CHECK: braces {} should be used on all arms of this statement
- lmp91000.c:216: CHECK: Unbalanced braces around else statement

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

diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c
index 03d277621861..ca31be510940 100644
--- 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;
 		}
-- 
2.20.1

[PATCH 3/5] iio:potentiostat:lmp91000: reduce line width and remove blank line

[Lucas Oshiro]

Break the line 258 in order fit the line width on 80 characters. Remove
the blank line 279, as the line before is also a blank line. Solve these
checkpath.el WARNING and CHECK:

- 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 | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c
index ca31be510940..6dba26121a62 100644
--- a/drivers/iio/potentiostat/lmp91000.c
+++ b/drivers/iio/potentiostat/lmp91000.c
@@ -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

[PATCH 4/5] iio:potentiostat:lmp91000: invert if statement

[Lucas Oshiro]

Invert if statement arms in line 214, in order to make the code cleaner

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

diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c
index 6dba26121a62..7229ef59590a 100644
--- a/drivers/iio/potentiostat/lmp91000.c
+++ b/drivers/iio/potentiostat/lmp91000.c
@@ -211,12 +211,11 @@ 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")) {
-			val = 0;
-		} else {
+		if (!of_property_read_bool(np, "ti,external-tia-resistor")) {
 			dev_err(dev, "no ti,tia-gain-ohm defined");
 			return ret;
 		}
+		val = 0;
 	}
 
 	ret = -EINVAL;
-- 
2.20.1

[PATCH 5/5] iio:potentiostat:lmp91000: add '\n' on dev_err

[Lucas Oshiro]

Add missing '\n' at the end of dev_err message on line 215.

Signed-off-by: Lucas Oshiro <lucasseikioshiro@gmail.com>
Signed-off-by: Anderson Reis <andersonreisrosa@gmail.com>
---
 drivers/iio/potentiostat/lmp91000.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c
index 7229ef59590a..aecdda757586 100644
--- a/drivers/iio/potentiostat/lmp91000.c
+++ b/drivers/iio/potentiostat/lmp91000.c
@@ -212,7 +212,7 @@ 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")) {
-			dev_err(dev, "no ti,tia-gain-ohm defined");
+			dev_err(dev, "no ti,tia-gain-ohm defined\n");
 			return ret;
 		}
 		val = 0;
-- 
2.20.1

Re: [PATCH 5/5] iio:potentiostat:lmp91000: add '\n' on dev_err

[Joe Perches]

On Mon, 2019-02-18 at 14:22 -0300, Lucas Oshiro wrote:
> Add missing '\n' at the end of dev_err message on line 215.
[]
> diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c
[]
> @@ -212,7 +212,7 @@ 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")) {
> -			dev_err(dev, "no ti,tia-gain-ohm defined");
> +			dev_err(dev, "no ti,tia-gain-ohm defined\n");

Perhaps a copy/paste error as the test is for
external-tia-resistor and not tia-gain-ohm

Re: [PATCH 0/5] iio:potentiostat:lmp91000: Adjust codestyle, and minor cleanup changes

[Jonathan Cameron]

On Mon, 18 Feb 2019 14:22:31 -0300
Lucas Oshiro <lucasseikioshiro@gmail.com> wrote:

> Hi,
> 
> We solved some checkpath.el CHECKs and WARNINGs. We also inverted the arms of
> an if statement, in order to make the code smaller as the else statement was
> supressed. We added a missing '\n' on a dev_err message.

Please try to identify the original author.  Matt is still definitely
about, and he is obviously likely to be able to provide the most detailed
review of changes to this driver.

Thanks,

Jonathan

> 
> Lucas Oshiro (5):
>   iio:potentiostat:lmp91000: remove unnecessary parentheses
>   iio:potentiostat:lmp91000: insert braces around if arms
>   iio:potentiostat:lmp91000: reduce line width and remove blank line
>   iio:potentiostat:lmp91000: invert if statement
>   iio:potentiostat:lmp91000: add '\n' on dev_err
> 
>  drivers/iio/potentiostat/lmp91000.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 

Re: [PATCH 5/5] iio:potentiostat:lmp91000: add '\n' on dev_err

[Jonathan Cameron]

On Mon, 18 Feb 2019 13:01:23 -0800
Joe Perches <joe@perches.com> wrote:

> On Mon, 2019-02-18 at 14:22 -0300, Lucas Oshiro wrote:
> > Add missing '\n' at the end of dev_err message on line 215.  
> []
> > diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c  
> []
> > @@ -212,7 +212,7 @@ 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")) {
> > -			dev_err(dev, "no ti,tia-gain-ohm defined");
> > +			dev_err(dev, "no ti,tia-gain-ohm defined\n");  
> 
> Perhaps a copy/paste error as the test is for
> external-tia-resistor and not tia-gain-ohm
> 
It is an odd construct, but I think this is correct.  What it is actually
saying is that, given that we don't have an external resistor, we care
that the tia-gain-ohm isn't set (otherwise it wouldn't matter).

From the docs
  - ti,external-tia-resistor: if the property ti,tia-gain-ohm is not defined this
    needs to be set to signal that an external resistor value is being used.

So, it might be ideal to say that tia-gain-ohm is not defined and we do
not have an external resistor specified.

So not wrong, but could be more informative!  So perhaps a follow up patch
to tidy that up would be good.

Jonathan
> 

Re: [PATCH 4/5] iio:potentiostat:lmp91000: invert if statement

[Jonathan Cameron]

On Mon, 18 Feb 2019 14:22:35 -0300
Lucas Oshiro <lucasseikioshiro@gmail.com> wrote:

> Invert if statement arms in line 214, in order to make the code cleaner
> 
> Signed-off-by: Lucas Oshiro <lucasseikioshiro@gmail.com>
> Signed-off-by: Anderson Reis <andersonreisrosa@gmail.com>

Given this undoes one of the earlier changes, please merge them.

Jonathan

> ---
>  drivers/iio/potentiostat/lmp91000.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c
> index 6dba26121a62..7229ef59590a 100644
> --- a/drivers/iio/potentiostat/lmp91000.c
> +++ b/drivers/iio/potentiostat/lmp91000.c
> @@ -211,12 +211,11 @@ 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")) {
> -			val = 0;
> -		} else {
> +		if (!of_property_read_bool(np, "ti,external-tia-resistor")) {
>  			dev_err(dev, "no ti,tia-gain-ohm defined");
>  			return ret;
>  		}
> +		val = 0;
>  	}
>  
>  	ret = -EINVAL;

Re: [PATCH 5/5] iio:potentiostat:lmp91000: add '\n' on dev_err

[Joe Perches]

On Wed, 2019-02-20 at 09:49 +0000, Jonathan Cameron wrote:
> On Mon, 18 Feb 2019 13:01:23 -0800 Joe Perches <joe@perches.com> wrote:
> > On Mon, 2019-02-18 at 14:22 -0300, Lucas Oshiro wrote:
> > > Add missing '\n' at the end of dev_err message on line 215.  
> > []
> > > diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c  
> > []
> > > @@ -212,7 +212,7 @@ 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")) {
> > > -			dev_err(dev, "no ti,tia-gain-ohm defined");
> > > +			dev_err(dev, "no ti,tia-gain-ohm defined\n");  
> > 
> > Perhaps a copy/paste error as the test is for
> > external-tia-resistor and not tia-gain-ohm
> > 
> It is an odd construct, but I think this is correct.  What it is actually
> saying is that, given that we don't have an external resistor, we care
> that the tia-gain-ohm isn't set (otherwise it wouldn't matter).
> 
> From the docs
>   - ti,external-tia-resistor: if the property ti,tia-gain-ohm is not defined this
>     needs to be set to signal that an external resistor value is being used.
> 
> So, it might be ideal to say that tia-gain-ohm is not defined and we do
> not have an external resistor specified.
> 
> So not wrong, but could be more informative!  So perhaps a follow up patch
> to tidy that up would be good.

Then thanks in advance for doing that.
cheers, Joe

Re: [PATCH 5/5] iio:potentiostat:lmp91000: add '\n' on dev_err

[Lucas Oshiro]

Thanks for the review!

On 20/02/2019 19:22, Joe Perches wrote:
> On Wed, 2019-02-20 at 09:49 +0000, Jonathan Cameron wrote:
>> On Mon, 18 Feb 2019 13:01:23 -0800 Joe Perches <joe@perches.com> wrote:
>>> On Mon, 2019-02-18 at 14:22 -0300, Lucas Oshiro wrote:
>>>> Add missing '\n' at the end of dev_err message on line 215.
>>> []
>>>> diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c
>>> []
>>>> @@ -212,7 +212,7 @@ 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")) {
>>>> -			dev_err(dev, "no ti,tia-gain-ohm defined");
>>>> +			dev_err(dev, "no ti,tia-gain-ohm defined\n");
>>>
>>> Perhaps a copy/paste error as the test is for
>>> external-tia-resistor and not tia-gain-ohm
>>>
>> It is an odd construct, but I think this is correct.  What it is actually
>> saying is that, given that we don't have an external resistor, we care
>> that the tia-gain-ohm isn't set (otherwise it wouldn't matter).
>>
>>  From the docs
>>    - ti,external-tia-resistor: if the property ti,tia-gain-ohm is not defined this
>>      needs to be set to signal that an external resistor value is being used.
>>
>> So, it might be ideal to say that tia-gain-ohm is not defined and we do
>> not have an external resistor specified.
>>
>> So not wrong, but could be more informative!  So perhaps a follow up patch
>> to tidy that up would be good.

So, this means that it's a good idea to change the dev_err message to something
like "no ti,tia-gain-ohm defined and external resistor not specified"?

> 
> Then thanks in advance for doing that.
> cheers, Joe
> 

Re: [PATCH 5/5] iio:potentiostat:lmp91000: add '\n' on dev_err

[Jonathan Cameron]

On Tue, 26 Feb 2019 17:15:52 -0300
Lucas Oshiro <lucasseikioshiro@gmail.com> wrote:

> Thanks for the review!
> 
> On 20/02/2019 19:22, Joe Perches wrote:
> > On Wed, 2019-02-20 at 09:49 +0000, Jonathan Cameron wrote:  
> >> On Mon, 18 Feb 2019 13:01:23 -0800 Joe Perches <joe@perches.com> wrote:  
> >>> On Mon, 2019-02-18 at 14:22 -0300, Lucas Oshiro wrote:  
> >>>> Add missing '\n' at the end of dev_err message on line 215.  
> >>> []  
> >>>> diff --git a/drivers/iio/potentiostat/lmp91000.c b/drivers/iio/potentiostat/lmp91000.c  
> >>> []  
> >>>> @@ -212,7 +212,7 @@ 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")) {
> >>>> -			dev_err(dev, "no ti,tia-gain-ohm defined");
> >>>> +			dev_err(dev, "no ti,tia-gain-ohm defined\n");  
> >>>
> >>> Perhaps a copy/paste error as the test is for
> >>> external-tia-resistor and not tia-gain-ohm
> >>>  
> >> It is an odd construct, but I think this is correct.  What it is actually
> >> saying is that, given that we don't have an external resistor, we care
> >> that the tia-gain-ohm isn't set (otherwise it wouldn't matter).
> >>
> >>  From the docs
> >>    - ti,external-tia-resistor: if the property ti,tia-gain-ohm is not defined this
> >>      needs to be set to signal that an external resistor value is being used.
> >>
> >> So, it might be ideal to say that tia-gain-ohm is not defined and we do
> >> not have an external resistor specified.
> >>
> >> So not wrong, but could be more informative!  So perhaps a follow up patch
> >> to tidy that up would be good.  
> 
> So, this means that it's a good idea to change the dev_err message to something
> like "no ti,tia-gain-ohm defined and external resistor not specified"?

Exactly.  Thanks,

Jonathan
> 
> > 
> > Then thanks in advance for doing that.
> > cheers, Joe
> >