| Message ID | 18725f7ddc3ac42b1c781b1848b05fabd4bd8320.1556919363.git.melissa.srw@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | staging: iio: ad7150: improve driver readability | expand |
On Sat, May 4, 2019 at 1:26 AM Melissa Wen <melissa.srw@gmail.com> 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@gmail.com> > --- > 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. > 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) > - if (ret < 0) > - return ret; > - > - return 0; > } > > static int ad7150_write_event_config(struct iio_dev *indio_dev, > -- > 2.20.1 >
On Sat, 4 May 2019 13:36:43 +0300 Alexandru Ardelean <ardeleanalex@gmail.com> wrote: > On Sat, May 4, 2019 at 1:26 AM Melissa Wen <melissa.srw@gmail.com> 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@gmail.com> > > --- > > 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 > >
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) return ret; - - ret = i2c_smbus_write_byte_data(chip->client, + else + return i2c_smbus_write_byte_data(chip->client, ad7150_addresses[chan][5], timeout); - if (ret < 0) - return ret; - - return 0; } static int ad7150_write_event_config(struct iio_dev *indio_dev,
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@gmail.com> --- drivers/staging/iio/cdc/ad7150.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)