[PATCH 0/3] Add small improvements to iio/magnetometer hmc5843 driver

[PATCH 0/3] Add small improvements to iio/magnetometer hmc5843 driver

[Cristina Opriceana]

This patchset reviews the iio/magnetometer hmc5843 driver and fixes incorrect
file header, hardcoded values and reconfigures some small error checks.

Cristina Opriceana (3):
  Staging: iio: Place driver in sleep mode on error
  Staging: iio: Replace hardcoded value with number of channels
  Staging: iio: Fix file header to match standards

 drivers/staging/iio/magnetometer/hmc5843_core.c | 59 +++++++++++++------------
 1 file changed, 30 insertions(+), 29 deletions(-)

-- 
1.9.1

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

[Cristina Opriceana]

Added sleep_mode label, on which the device enters sleep mode if
encounters an error after initialization. This helps saving energy
consumption.

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;
+	}
 	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);
-- 
1.9.1

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

[Daniel Baluta]

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.

>         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.

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

[Cristina Opriceana]

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.

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

[Greg KH]

On Thu, Mar 19, 2015 at 09:19:08PM +0200, Cristina Opriceana wrote:
> Added sleep_mode label, on which the device enters sleep mode if
> encounters an error after initialization. This helps saving energy
> consumption.
> 
> Signed-off-by: Cristina Opriceana <cristina.opriceana@gmail.com>
> ---
>  drivers/staging/iio/magnetometer/hmc5843_core.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)

This series doesn't apply, and also, if forced, breaks the build :(

Please fix up and resend.

thanks,

greg k-h

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

[Cristina Opriceana]

On Vi, 2015-03-20 at 13:39 +0100, Greg KH wrote:
> On Thu, Mar 19, 2015 at 09:19:08PM +0200, Cristina Opriceana wrote:
> > Added sleep_mode label, on which the device enters sleep mode if
> > encounters an error after initialization. This helps saving energy
> > consumption.
> > 
> > Signed-off-by: Cristina Opriceana <cristina.opriceana@gmail.com>
> > ---
> >  drivers/staging/iio/magnetometer/hmc5843_core.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> This series doesn't apply, and also, if forced, breaks the build :(
> 
> Please fix up and resend.
> 
I'll fix and resend. Sorry about that, I changed some .config options
and forgot to put them back.


> thanks,
> 
> greg k-h

[PATCH 2/3] Staging: iio: Replace hardcoded value with number of channels

[Cristina Opriceana]

This patch replaces the hardcoded value 4 for the number of available
channels and computes it with the ARRAY_SIZE macro.

Signed-off-by: Cristina Opriceana <cristina.opriceana@gmail.com>
---
 drivers/staging/iio/magnetometer/hmc5843_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/magnetometer/hmc5843_core.c b/drivers/staging/iio/magnetometer/hmc5843_core.c
index 59afede..3898690 100644
--- a/drivers/staging/iio/magnetometer/hmc5843_core.c
+++ b/drivers/staging/iio/magnetometer/hmc5843_core.c
@@ -596,7 +596,7 @@ int hmc5843_common_probe(struct device *dev, struct regmap *regmap,
 	indio_dev->info = &hmc5843_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = data->variant->channels;
-	indio_dev->num_channels = 4;
+	indio_dev->num_channels = ARRAY_SIZE(data->variant->channels);
 	indio_dev->available_scan_masks = hmc5843_scan_masks;

 	ret = hmc5843_init(data);
-- 
1.9.1

[PATCH 3/3] Staging: iio: Fix file header to match standards

[Cristina Opriceana]

Fix file header to match Linux Kernel style. Remove Free Software
Foundation reference to silence the checkpatch.pl warning.
Add driver description and correct spelling mistakes.

Signed-off-by: Cristina Opriceana <cristina.opriceana@gmail.com>
---
 drivers/staging/iio/magnetometer/hmc5843_core.c | 44 ++++++++++++-------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/iio/magnetometer/hmc5843_core.c b/drivers/staging/iio/magnetometer/hmc5843_core.c
index 3898690..d03833b 100644
--- a/drivers/staging/iio/magnetometer/hmc5843_core.c
+++ b/drivers/staging/iio/magnetometer/hmc5843_core.c
@@ -1,25 +1,25 @@
-/*  Copyright (C) 2010 Texas Instruments
-    Author: Shubhrajyoti Datta <shubhrajyoti@ti.com>
-    Acknowledgement: Jonathan Cameron <jic23@kernel.org> for valuable inputs.
-
-    Support for HMC5883 and HMC5883L by Peter Meerwald <pmeerw@pmeerw.net>.
-
-    Split to multiple files by Josef Gajdusek <atx@atx.name> - 2014
-
-    This program is free software; you can redistribute it and/or modify
-    it under the terms of the GNU General Public License as published by
-    the Free Software Foundation; either version 2 of the License, or
-    (at your option) any later version.
-
-    This program is distributed in the hope that it will be useful,
-    but WITHOUT ANY WARRANTY; without even the implied warranty of
-    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-    GNU General Public License for more details.
-
-    You should have received a copy of the GNU General Public License
-    along with this program; if not, write to the Free Software
-    Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
-*/
+/*
+ * Device driver for the the HMC5843 multi-chip module designed
+ * for low field magnetic sensing.
+ *
+ * Copyright (C) 2010 Texas Instruments
+ *
+ * Author: Shubhrajyoti Datta <shubhrajyoti@ti.com>
+ * Acknowledgment: Jonathan Cameron <jic23@kernel.org> for valuable inputs.
+ * Support for HMC5883 and HMC5883L by Peter Meerwald <pmeerw@pmeerw.net>.
+ * Split to multiple files by Josef Gajdusek <atx@atx.name> - 2014
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
 
 #include <linux/module.h>
 #include <linux/regmap.h>
-- 
1.9.1

Re: [Outreachy kernel] [PATCH 0/3] Add small improvements to iio/magnetometer hmc5843 driver

[Daniel Baluta]

On Thu, Mar 19, 2015 at 9:17 PM, Cristina Opriceana
<cristina.opriceana@gmail.com> wrote:
> This patchset reviews the iio/magnetometer hmc5843 driver and fixes incorrect
> file header, hardcoded values and reconfigures some small error checks.
>
> Cristina Opriceana (3):
>   Staging: iio: Place driver in sleep mode on error
>   Staging: iio: Replace hardcoded value with number of channels
>   Staging: iio: Fix file header to match standards

Not sure if this patch series depends on  [PATCH 0/5] Fix
iio/magnetometer style warnings

If so, please merge these into one. It will be easier for Greg to merge them.

Daniel.