Add small improvements to iio/magnetometer hmc5843 driver

PATCH Superseded Cristina Opriceana 3 participants 9 messages Mar 19, 2015 – Mar 20, 2015

Subsystems (3)

Staging IIO Staging - industrial io

lore

Share & tools

lore

Cristina Opriceana

author

2015-03-19 19:18 UTC #0 lore raw

cover Add small improvements to iio/magnetometer hmc5843 driver

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(-)

Signature · 1 line

1.9.1

Cristina Opriceana

author

2015-03-19 19:19 UTC #1 lore raw

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

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 <[email protected]>

---
 drivers/staging/iio/magnetometer/hmc5843_core.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

drivers/staging/iio/magnetometer/hmc5843_core.c +7 -6

index 90cc18b..59afede 100644
@@ -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);

Signature · 1 line

1.9.1

Daniel Baluta

2015-03-20 09:13 UTC #2 lore raw

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

Quoted 3 lines · Cristina Opriceana

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.

Quoted 15 lines

Signed-off-by Cristina Opriceana <[email protected]>

---
 drivers/staging/iio/magnetometer/hmc5843_core.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

drivers/staging/iio/magnetometer/hmc5843_core.c +0 -1

index 90cc18b..59afede 100644
@@ -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;

drivers/staging/iio/magnetometer/hmc5843_core.c int hmc5843_common_probe(struct device *dev, struct regmap *regmap, · line 615

Quoted 10 lines

drivers/staging/iio/magnetometer/hmc5843_core.c +5 -3

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

drivers/staging/iio/magnetometer/hmc5843_core.c int hmc5843_common_probe(struct device *dev, struct regmap *regmap,

Quoted 10 lines

drivers/staging/iio/magnetometer/hmc5843_core.c +2 -3

        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.

Cristina Opriceana

author

2015-03-20 09:31 UTC #3 lore raw

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

Quoted 35 lines · Daniel Baluta

Quoted 3 lines · Cristina Opriceana
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.
Quoted 17 lines
Signed-off-by Cristina Opriceana <[email protected]>
---
 drivers/staging/iio/magnetometer/hmc5843_core.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
drivers/staging/iio/magnetometer/hmc5843_core.c +1 -1
index 90cc18b..59afede 100644
@@ -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;
 

Quoted 10 lines

drivers/staging/iio/magnetometer/hmc5843_core.c int hmc5843_common_probe(struct device *dev, struct regmap *regmap, · line 615
Quoted 8 lines
drivers/staging/iio/magnetometer/hmc5843_core.c +4 -3
        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.

Quoted 16 lines

drivers/staging/iio/magnetometer/hmc5843_core.c int hmc5843_common_probe(struct device *dev, struct regmap *regmap,
Quoted 11 lines
drivers/staging/iio/magnetometer/hmc5843_core.c +2 -3
        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.

Greg KH

maintainer

2015-03-20 14:41 UTC #4 lore raw

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

In reply to Cristina Opriceana

Quoted 8 lines · 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 <[email protected]>
---
 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

Cristina Opriceana

author

2015-03-20 15:35 UTC #5 lore raw

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

Quoted 5 lines · Greg KH

Quoted 4 lines · Cristina Opriceana
Added sleep_mode label, on which the device enters sleep mode if encounters an error after initialization. This helps saving energy consumption.

Quoted 9 lines

Quoted 4 lines
Signed-off-by Cristina Opriceana <[email protected]>
---
 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.

Quoted 3 lines

thanks,
greg k-h

Cristina Opriceana

author

2015-03-19 19:20 UTC #6 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/staging/iio/magnetometer/hmc5843_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/staging/iio/magnetometer/hmc5843_core.c +1 -1

index 59afede..3898690 100644
@@ -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);

Signature · 1 line

1.9.1

Cristina Opriceana

author

2015-03-19 19:22 UTC #7 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/staging/iio/magnetometer/hmc5843_core.c | 44 ++++++++++++-------------
 1 file changed, 22 insertions(+), 22 deletions(-)

drivers/staging/iio/magnetometer/hmc5843_core.c +22 ~~-22~~

index 3898690..d03833b 100644
@@ -1,25 +1,25 @@
-/*  Copyright (C) 2010 Texas Instruments
-    Author: Shubhrajyoti Datta <[email protected]>
-    Acknowledgement: Jonathan Cameron <[email protected]> for valuable inputs.
-
-    Support for HMC5883 and HMC5883L by Peter Meerwald <[email protected]>.
-
-    Split to multiple files by Josef Gajdusek <[email protected]> - 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 <[email protected]>
+ * Acknowledgment: Jonathan Cameron <[email protected]> for valuable inputs.
+ * Support for HMC5883 and HMC5883L by Peter Meerwald <[email protected]>.
+ * Split to multiple files by Josef Gajdusek <[email protected]> - 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>

Signature · 1 line

1.9.1

Daniel Baluta

2015-03-20 08:58 UTC #8 lore raw

In reply to Cristina Opriceana

Quoted 7 lines · 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

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.