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

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

[Cristina Opriceana]

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

-- 
1.9.1

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

[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 <cristina.opriceana@gmail.com>
---
 drivers/staging/iio/magnetometer/hmc5843_core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/iio/magnetometer/hmc5843_core.c b/drivers/staging/iio/magnetometer/hmc5843_core.c
index 0074af2..7de19c2 100644
--- a/drivers/staging/iio/magnetometer/hmc5843_core.c
+++ b/drivers/staging/iio/magnetometer/hmc5843_core.c
@@ -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);
-- 
1.9.1

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

[Daniel Baluta]

On Sun, Mar 22, 2015 at 12:16 PM, Daniel Baluta <daniel.baluta@gmail.com> wrote:
> On Sun, Mar 22, 2015 at 2:26 AM, Cristina Opriceana
> <cristina.opriceana@gmail.com> wrote:
>> Put device in sleep mode if an error is encountered after
>> initialization in order to avoid wasting power.
>>
>> Signed-off-by: Cristina Opriceana <cristina.opriceana@gmail.com>
>> ---
>>  drivers/staging/iio/magnetometer/hmc5843_core.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/iio/magnetometer/hmc5843_core.c b/drivers/staging/iio/magnetometer/hmc5843_core.c
>> index 0074af2..7de19c2 100644
>> --- a/drivers/staging/iio/magnetometer/hmc5843_core.c
>> +++ b/drivers/staging/iio/magnetometer/hmc5843_core.c
>> @@ -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);
>
> 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.

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

[Jonathan Cameron]

On 22/03/15 00:26, Cristina Opriceana wrote:
> 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

> 
> Signed-off-by: Cristina Opriceana <cristina.opriceana@gmail.com>
> ---
>  drivers/staging/iio/magnetometer/hmc5843_core.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/magnetometer/hmc5843_core.c b/drivers/staging/iio/magnetometer/hmc5843_core.c
> index 0074af2..7de19c2 100644
> --- a/drivers/staging/iio/magnetometer/hmc5843_core.c
> +++ b/drivers/staging/iio/magnetometer/hmc5843_core.c
> @@ -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;
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;
>  }
>  EXPORT_SYMBOL(hmc5843_common_probe);
> 

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

[Cristina Georgiana Opriceana]

On Sat, Mar 28, 2015 at 2:13 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 22/03/15 00:26, Cristina Opriceana wrote:
>> 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

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

[Jonathan Cameron]

On 28/03/15 13:10, Cristina Georgiana Opriceana wrote:
> On Sat, Mar 28, 2015 at 2:13 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 22/03/15 00:26, Cristina Opriceana wrote:
>>> 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.
Fair enough!

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

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