| Message ID | 20190129183656.15656-1-lucasseikioshiro@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio:potentiostat:lmp91000: solve codestyle WARNINGs and CHECKs | expand |
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
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? > >
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 > > > >
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 > >>> >>> >
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);
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(-)