Add small improvements to iio/magnetometer hmc5843 driver

PATCH Superseded Cristina Opriceana 3 participants 7 messages Mar 22, 2015 – Mar 28, 2015 1 message not in the archive

Subsystems (3)

Staging IIO Staging - industrial io

lore

Share & tools

lore

Cristina Opriceana

author

2015-03-22 00:20 UTC #0 lore raw

v2 cover Add small improvements to iio/magnetometer hmc5843 driver

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

Changes in v2:
- drop the change of number of channels hardcoded value, since this value cannot be accesed through an id for each of the hmc5843_channels and hmc5883_channels arrays.
- keep 2 goto labels for sleep mode on error in PATCH v2 1/2
- update commit message for PATCH v2 1/2 to describe better why the change is needed

Cristina Opriceana (2):
Staging: iio: Place driver in sleep mode on error
Staging: iio: Fix file header to match standards

 drivers/staging/iio/magnetometer/hmc5843_core.c | 50 +++++++++++++------------
 1 file changed, 27 insertions(+), 23 deletions(-)

Signature · 1 line

1.9.1

Cristina Opriceana

author

2015-03-22 00:27 UTC #1 lore raw

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

Put device in sleep mode if an error is encountered after initialization in order to avoid wasting power.

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

---
 drivers/staging/iio/magnetometer/hmc5843_core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

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

index 0074af2..7de19c2 100644
@@ -611,7 +611,7 @@ 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 buffer_setup_err;
 
 	ret = iio_device_register(indio_dev);
 	if (ret < 0)

@@ -619,8 +619,12 @@ int hmc5843_common_probe(struct device *dev, struct regmap *regmap,
 
 	return 0;
 
+buffer_setup_err:
+	hmc5843_set_mode(iio_priv(indio_dev), HMC5843_MODE_SLEEP);
+	return ret;
 buffer_cleanup:
 	iio_triggered_buffer_cleanup(indio_dev);
+	hmc5843_set_mode(iio_priv(indio_dev), HMC5843_MODE_SLEEP);
 	return ret;
 }
 EXPORT_SYMBOL(hmc5843_common_probe);

Signature · 1 line

1.9.1

A message that is not in the archive (sent off-list or not yet received): CAEnQRZDkMngLNm61qLdEA8_4reJ+VzQKhxaJxXu4r5drUPaP6w@mail.gmail.com

Daniel Baluta

2015-03-22 10:16 UTC #3 lore raw

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

Quoted 48 lines · Daniel Baluta

Quoted 24 lines · Cristina Opriceana

Put device in sleep mode if an error is encountered after initialization in order to avoid wasting power.

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

---
 drivers/staging/iio/magnetometer/hmc5843_core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

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

index 0074af2..7de19c2 100644
@@ -611,7 +611,7 @@ 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 buffer_setup_err;
 
        ret = iio_device_register(indio_dev);
        if (ret < 0)

@@ -619,8 +619,12 @@ int hmc5843_common_probe(struct device *dev, struct regmap *regmap,
 
        return 0;

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

Quoted 10 lines

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

 
+buffer_setup_err:
+       hmc5843_set_mode(iio_priv(indio_dev), HMC5843_MODE_SLEEP);
+       return ret;
 buffer_cleanup:
        iio_triggered_buffer_cleanup(indio_dev);
+       hmc5843_set_mode(iio_priv(indio_dev), HMC5843_MODE_SLEEP);
        return ret;
 }
 EXPORT_SYMBOL(hmc5843_common_probe);

What about:

buffer_cleanup:

         iio_triggered_buffer_cleanup(indio_dev);
buffer_setup_err:
       hmc5843_set_mode(iio_priv(indio_dev), HMC5843_MODE_SLEEP);
       return ret;

In this way you avoid duplicating code.

Daniel.

Jonathan Cameron

maintainer

2015-03-28 12:13 UTC #4 lore raw

Re: [PATCH v2 1/2] Staging: iio: Place driver in sleep mode on error

In reply to Cristina Opriceana

Quoted 2 lines · Cristina Opriceana

Put device in sleep mode if an error is encountered after initialization in order to avoid wasting power.

The patch title should definitely include the name of the driver in question! Otherwise you'll not generally get reviews from the relevant people. Staging:iio:hmc5843 Place driver in sleep mode on error during probe.

Also try to cc relevant people and the relevant list.
See MAINTAINERS and you'll notice we have both a list and a set of designated reviewers who would like to be cc'd on IIO patches. They may not respond, but if you don't send it to them then they definitely won't!

Jonathan

Quoted 20 lines

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

---
 drivers/staging/iio/magnetometer/hmc5843_core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

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

index 0074af2..7de19c2 100644
@@ -611,7 +611,7 @@ 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 buffer_setup_err;
 
 	ret = iio_device_register(indio_dev);
 	if (ret < 0)

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

Quoted 10 lines

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

@@ -619,8 +619,12 @@ int hmc5843_common_probe(struct device *dev, struct regmap *regmap,
 
 	return 0;
 
+buffer_setup_err:
+	hmc5843_set_mode(iio_priv(indio_dev), HMC5843_MODE_SLEEP);
+	return ret;
 buffer_cleanup:
 	iio_triggered_buffer_cleanup(indio_dev);
+	hmc5843_set_mode(iio_priv(indio_dev), HMC5843_MODE_SLEEP);
 	return ret;

Single exit path that reverses the ordering of creation.

e.g.
buffer_setup_err:

	hmc_set_mode(iio_priv(indio_dev), HMC5843_MODE_SLEEP);
buffer_cleanup:
	iio_triggered_buffer_cleanup(indio_dev);
	return ret;

Quoted 3 lines

drivers/staging/iio/magnetometer/hmc5843_core.c +0 -0
 }
 EXPORT_SYMBOL(hmc5843_common_probe);

Cristina Georgiana Opriceana

author

2015-03-28 13:10 UTC #5 lore raw

Re: [PATCH v2 1/2] Staging: iio: Place driver in sleep mode on error

Quoted 10 lines · Jonathan Cameron

Quoted 2 lines · Cristina Opriceana
Put device in sleep mode if an error is encountered after initialization in order to avoid wasting power.
The patch title should definitely include the name of the driver in question! Otherwise you'll not generally get reviews from the relevant people. Staging:iio:hmc5843 Place driver in sleep mode on error during probe.
Also try to cc relevant people and the relevant list.
See MAINTAINERS and you'll notice we have both a list and a set of designated reviewers who would like to be cc'd on IIO patches.

Okay, thanks for your feedback, I'll keep that in mind for the future work because Greg has already merged this on his staging-next branch.

Thanks,
Cristina

Jonathan Cameron

maintainer

2015-03-28 13:45 UTC #6 lore raw

Re: [PATCH v2 1/2] Staging: iio: Place driver in sleep mode on error

Quoted 8 lines · Cristina Georgiana Opriceana

Quoted 7 lines · Jonathan Cameron
Quoted 2 lines · Cristina Opriceana
Put device in sleep mode if an error is encountered after initialization in order to avoid wasting power.
The patch title should definitely include the name of the driver in question! Otherwise you'll not generally get reviews from the relevant people. Staging:iio:hmc5843 Place driver in sleep mode on error during probe.

Quoted 6 lines

Quoted 3 lines
Also try to cc relevant people and the relevant list.
See MAINTAINERS and you'll notice we have both a list and a set of designated reviewers who would like to be cc'd on IIO patches.
Okay, thanks for your feedback, I'll keep that in mind for the future work because Greg has already merged this on his staging-next branch.

Fair enough!

Quoted 8 lines

Thanks,
Cristina
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Cristina Opriceana

author

2015-03-22 00:28 UTC #7 lore raw

PATCH v2 2/2 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 7de19c2..9664ac7 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