On Fri, 1 Feb 2019 12:29:11 -0200
LSO <lucasseikioshiro@xxxxxxxxx> 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
> >
> >
