Re: [Outreachy kernel] [PATCH 1/3] Staging: iio: Place driver in sleep mode on error

[Cristina Opriceana <cristina.opriceana@gmail.com>]

On Vi, 2015-03-20 at 11:13 +0200, Daniel Baluta wrote:
> On Thu, Mar 19, 2015 at 9:19 PM, Cristina Opriceana
> <cristina.opriceana@gmail.com> wrote:
> > Added sleep_mode label, on which the device enters sleep mode if
> > encounters an error after initialization. This helps saving energy
> > consumption.
> 
> Commit message shouldn't tell "how" it does something, this should
> be obvious from the code. We should insist more on "why" we need
> this change.
> 
> So, the fact that you added "sleep_mode" label shouldn't be present
> in commit message.
> 
> Better say:
> 
> Put device in sleep mode if we encounter an error after initialization
> to avoid wasting power.
> 
> >
> > Signed-off-by: Cristina Opriceana <cristina.opriceana@gmail.com>
> > ---
> >  drivers/staging/iio/magnetometer/hmc5843_core.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/staging/iio/magnetometer/hmc5843_core.c b/drivers/staging/iio/magnetometer/hmc5843_core.c
> > index 90cc18b..59afede 100644
> > --- a/drivers/staging/iio/magnetometer/hmc5843_core.c
> > +++ b/drivers/staging/iio/magnetometer/hmc5843_core.c
> > @@ -606,16 +606,17 @@ int hmc5843_common_probe(struct device *dev, struct regmap *regmap,
> >         ret = iio_triggered_buffer_setup(indio_dev, NULL,
> >                 hmc5843_trigger_handler, NULL);
> >         if (ret < 0)
> > -               return ret;
> > +               goto sleep_mode;
> >
> >         ret = iio_device_register(indio_dev);
> > -       if (ret < 0)
> > -               goto buffer_cleanup;
> > -
> > +       if (ret < 0) {
> > +               iio_triggered_buffer_cleanup(indio_dev);
> > +               goto sleep_mode;
> > +       }
> 
> Why this change here? The initial goto buffer_cleanup will work just fine.

I wanted to avoid duplicating code, but yes, apart from that, the
initial goto would work just fine.

> 
> >         return 0;
> >
> > -buffer_cleanup:
> > -       iio_triggered_buffer_cleanup(indio_dev);
> > +sleep_mode:
> > +       hmc5843_set_mode(iio_priv(indio_dev), HMC5843_MODE_SLEEP);
> >         return ret;
> >  }
> >  EXPORT_SYMBOL(hmc5843_common_probe);
> > --
> 
> Nice patch. A good idea would be to also add module authors/reviewers for this.
> 
> Thanks,
> Daniel.