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
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 <[email protected]>
Signed-off-by: Anderson Reis <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Anderson Reis <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Anderson Reis <[email protected]>
---
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
Invert if statement arms in line 214, in order to make the code cleaner
Signed-off-by: Lucas Oshiro <[email protected]>
Signed-off-by: Anderson Reis <[email protected]>
---
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
Add missing '\n' at the end of dev_err message on line 215.
Signed-off-by: Lucas Oshiro <[email protected]>
Signed-off-by: Anderson Reis <[email protected]>
---
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
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
On Mon, 18 Feb 2019 14:22:31 -0300
Lucas Oshiro <[email protected]> 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(-)
>
On Mon, 18 Feb 2019 13:01:23 -0800
Joe Perches <[email protected]> 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
>
On Mon, 18 Feb 2019 14:22:35 -0300
Lucas Oshiro <[email protected]> wrote:
> Invert if statement arms in line 214, in order to make the code cleaner
>
> Signed-off-by: Lucas Oshiro <[email protected]>
> Signed-off-by: Anderson Reis <[email protected]>
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;
On Wed, 2019-02-20 at 09:49 +0000, Jonathan Cameron wrote:
> On Mon, 18 Feb 2019 13:01:23 -0800 Joe Perches <[email protected]> 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
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 <[email protected]> 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
>
On Tue, 26 Feb 2019 17:15:52 -0300
Lucas Oshiro <[email protected]> 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 <[email protected]> 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
> >