Re: [PATCH 3/4] staging: iio: ad7150: simplify i2c SMBus return treatment

[Jonathan Cameron]

On Sat, 4 May 2019 13:36:43 +0300
Alexandru Ardelean <ardeleanalex@xxxxxxxxx> wrote:

> On Sat, May 4, 2019 at 1:26 AM Melissa Wen <melissa.srw@xxxxxxxxx> wrote:
> >
> > Since i2c_smbus_write_byte_data returns no-positive value, this commit
> > making the treatment of its return value less verbose.
> >
> > Signed-off-by: Melissa Wen <melissa.srw@xxxxxxxxx>
> > ---
> >  drivers/staging/iio/cdc/ad7150.c | 10 +++-------
> >  1 file changed, 3 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
> > index 4ba46fb6ac02..3a4572a9e5ec 100644
> > --- a/drivers/staging/iio/cdc/ad7150.c
> > +++ b/drivers/staging/iio/cdc/ad7150.c
> > @@ -201,16 +201,12 @@ static int ad7150_write_event_params(struct iio_dev *indio_dev,
> >         ret = i2c_smbus_write_byte_data(chip->client,
> >                                         ad7150_addresses[chan][4],
> >                                         sens);
> > -       if (ret < 0)
> > +       if (ret)  
> 
> For i2c_smbus_write_byte_data(), checking "ret < 0" or non-zero, is the same.
> Changing this doesn't have any added value.
The slight note I'd add to that is that if you are (and I think you should)
just doing
return i2c_smbus_write_byte_data()
for the last call then that effectively means we are assuming ret is never positive
in some paths and not others.  I'd encourage consistency so would would prefer
this is changed to if (ret).

As in the earlier patch the line between what is noise in a staging (or new
driver) and what is noise in a driver that has been outside staging for years
is different.  Not so good for Alex perhaps if there is a chance Analog will
backport fixes for their drivers, but tough luck :)

> 
> >                 return ret;
> > -
> > -       ret = i2c_smbus_write_byte_data(chip->client,
> > +       else
> > +               return i2c_smbus_write_byte_data(chip->client,
> >                                         ad7150_addresses[chan][5],
> >                                         timeout);  
> 
> The introduction of the "else" branch is a bit noisy.
> The code was a bit neater (and readable) before the else branch, and
> functionally identical.
> 
> Well, when I say neater before, you have to understand, that I (and I
> assume that some other people who write drivers) have a slight
> fixation for this pattern:
> 
> example1:
> ret = fn1();
> 
> if (ret < 0)  // could also be just "if (ret)"
>    return ret;
> 
> ret = fn2();
> if (ret < 0)  // could also be just "if (ret)"
>    return ret;
> 
> example1a:
> +ret = fn3();
> +if (ret < 0)  // could also be just "if (ret)"
> +    return ret;
> 
> 
> Various higher-level programming languages, will discourage this
> pattern in favor of neater patterns.
> 
> I personally, have a few arguments in favor of this pattern:
> 1) it is closer to how the machine code ; so, closer to how a
> low-level instruction looks like
> 2) if (ever) this needs to be patched, the patch could be neat (see
> example1a) ; the examle assumes that it's been added via a patch at a
> later point in time
> 3) it keeps indentation level to a minimum ; this also aligns with
> kernel-coding guidelines
> (https://www.kernel.org/doc/html/v4.10/process/coding-style.html )
>     (indentation seems a bit OCD-like when someone points it out at a
> review, but it has it's value over time)
Nicely laid out argument.  Strongest one is the maintainability and
reviewability aspect of it being how kernel code is done and hence
takes every so slightly less thought ;)

Errors paths are indented, good paths not (in general).

Jonathan


> 
> > -       if (ret < 0)
> > -               return ret;
> > -
> > -       return 0;
> >  }
> >
> >  static int ad7150_write_event_config(struct iio_dev *indio_dev,
> > --
> > 2.20.1
> >