[PATCH 0/8] bmc150_magn fix and cleanup

[PATCH 0/8] bmc150_magn fix and cleanup

[Hartmut Knaack]

Some issues I spotted during review of this driver. First three are quite
obvious problems, the rest may be matter of taste or intended to stay 
in the original form. Opinions welcome.

Hartmut Knaack (8):
  iio:magnetometer:bmc150_magn: add regmap dependency
  iio:magnetometer:bmc150_magn: sort entry alphabetically
  iio:magnetometer:bmc150_magn: output intended variable
  iio:magnetometer:bmc150_magn: replace magic value
  iio:magnetometer:bmc150_magn: add error handling on gpio probe
  iio:magnetometer:bmc150_magn: protect runtime_resume with mutex
  iio:magnetometer:bmc150_magn: expand mutex in trigger_handler
  iio:magnetometer:bmc150_magn: use descriptive name for mask

 drivers/iio/magnetometer/Kconfig       | 32 ++++++++++++++++++--------------
 drivers/iio/magnetometer/Makefile      |  3 +--
 drivers/iio/magnetometer/bmc150_magn.c | 31 ++++++++++++++++++++++---------
 3 files changed, 41 insertions(+), 25 deletions(-)

-- 
2.4.3

[PATCH 1/8] iio:magnetometer:bmc150_magn: add regmap dependency

[Hartmut Knaack]

bmc150_magn makes use of REGMAP_I2C, so select it to build always without
errors.

Fixes: c91746a2361d ("iio: magn: Add support for BMC150 magnetometer")

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/magnetometer/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
index dcadfc4f0661..efb9350b0d76 100644
--- a/drivers/iio/magnetometer/Kconfig
+++ b/drivers/iio/magnetometer/Kconfig
@@ -90,6 +90,7 @@ config IIO_ST_MAGN_SPI_3AXIS
 config BMC150_MAGN
 	tristate "Bosch BMC150 Magnetometer Driver"
 	depends on I2C
+	select REGMAP_I2C
 	select IIO_BUFFER
 	select IIO_TRIGGERED_BUFFER
 	help
-- 
2.4.3

[PATCH 2/8] iio:magnetometer:bmc150_magn: sort entry alphabetically

[Hartmut Knaack]

Sort the entry for bmc105_magn in Kconfig and Makefile to its correct
position. Also add the minor module information for completeness.

Fixes: c91746a2361d ("iio: magn: Add support for BMC150 magnetometer")

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/magnetometer/Kconfig  | 33 ++++++++++++++++++---------------
 drivers/iio/magnetometer/Makefile |  3 +--
 2 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
index efb9350b0d76..868abada3409 100644
--- a/drivers/iio/magnetometer/Kconfig
+++ b/drivers/iio/magnetometer/Kconfig
@@ -24,6 +24,24 @@ config AK09911
 	help
 	  Deprecated: AK09911 is now supported by AK8975 driver.
 
+config BMC150_MAGN
+	tristate "Bosch BMC150 Magnetometer Driver"
+	depends on I2C
+	select REGMAP_I2C
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	help
+	  Say yes here to build support for the BMC150 magnetometer.
+
+	  Currently this only supports the device via an i2c interface.
+
+	  This is a combo module with both accelerometer and magnetometer.
+	  This driver is only implementing magnetometer part, which has
+	  its own address and register map.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called bmc150_magn.
+
 config MAG3110
 	tristate "Freescale MAG3110 3-Axis Magnetometer"
 	depends on I2C
@@ -87,19 +105,4 @@ config IIO_ST_MAGN_SPI_3AXIS
 	depends on IIO_ST_MAGN_3AXIS
 	depends on IIO_ST_SENSORS_SPI
 
-config BMC150_MAGN
-	tristate "Bosch BMC150 Magnetometer Driver"
-	depends on I2C
-	select REGMAP_I2C
-	select IIO_BUFFER
-	select IIO_TRIGGERED_BUFFER
-	help
-	  Say yes here to build support for the BMC150 magnetometer.
-
-	  Currently this only supports the device via an i2c interface.
-
-	  This is a combo module with both accelerometer and magnetometer.
-	  This driver is only implementing magnetometer part, which has
-	  its own address and register map.
-
 endmenu
diff --git a/drivers/iio/magnetometer/Makefile b/drivers/iio/magnetometer/Makefile
index 33b1d4d54ee7..2c72df458ec2 100644
--- a/drivers/iio/magnetometer/Makefile
+++ b/drivers/iio/magnetometer/Makefile
@@ -4,6 +4,7 @@
 
 # When adding new entries keep the list in alphabetical order
 obj-$(CONFIG_AK8975)	+= ak8975.o
+obj-$(CONFIG_BMC150_MAGN) += bmc150_magn.o
 obj-$(CONFIG_MAG3110)	+= mag3110.o
 obj-$(CONFIG_HID_SENSOR_MAGNETOMETER_3D) += hid-sensor-magn-3d.o
 obj-$(CONFIG_MMC35240)	+= mmc35240.o
@@ -14,5 +15,3 @@ st_magn-$(CONFIG_IIO_BUFFER) += st_magn_buffer.o
 
 obj-$(CONFIG_IIO_ST_MAGN_I2C_3AXIS) += st_magn_i2c.o
 obj-$(CONFIG_IIO_ST_MAGN_SPI_3AXIS) += st_magn_spi.o
-
-obj-$(CONFIG_BMC150_MAGN) += bmc150_magn.o
-- 
2.4.3

[PATCH 3/8] iio:magnetometer:bmc150_magn: output intended variable

[Hartmut Knaack]

According to the debug/error string, the content of chip_id is supposed to
be output, rather than the return value of the previous operation.

Fixes: c91746a2361d ("iio: magn: Add support for BMC150 magnetometer")

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/magnetometer/bmc150_magn.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
index cd002710dd02..7da4ce9409e8 100644
--- a/drivers/iio/magnetometer/bmc150_magn.c
+++ b/drivers/iio/magnetometer/bmc150_magn.c
@@ -694,11 +694,11 @@ static int bmc150_magn_init(struct bmc150_magn_data *data)
 		goto err_poweroff;
 	}
 	if (chip_id != BMC150_MAGN_CHIP_ID_VAL) {
-		dev_err(&data->client->dev, "Invalid chip id 0x%x\n", ret);
+		dev_err(&data->client->dev, "Invalid chip id 0x%x\n", chip_id);
 		ret = -ENODEV;
 		goto err_poweroff;
 	}
-	dev_dbg(&data->client->dev, "Chip id %x\n", ret);
+	dev_dbg(&data->client->dev, "Chip id %x\n", chip_id);
 
 	preset = bmc150_magn_presets_table[BMC150_MAGN_DEFAULT_PRESET];
 	ret = bmc150_magn_set_odr(data, preset.odr);
-- 
2.4.3

[PATCH 4/8] iio:magnetometer:bmc150_magn: replace magic value

[Hartmut Knaack]

Construct the scanmask using its descriptive axis names (as used in
iio_chan_spec) instead of a 'magic' value.

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
---
Does this look better?

 drivers/iio/magnetometer/bmc150_magn.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
index 7da4ce9409e8..f1b6a4dc40ea 100644
--- a/drivers/iio/magnetometer/bmc150_magn.c
+++ b/drivers/iio/magnetometer/bmc150_magn.c
@@ -651,7 +651,9 @@ static const struct iio_info bmc150_magn_info = {
 	.driver_module = THIS_MODULE,
 };
 
-static const unsigned long bmc150_magn_scan_masks[] = {0x07, 0};
+static const unsigned long bmc150_magn_scan_masks[] = {
+					BIT(AXIS_X) | BIT(AXIS_Y) | BIT(AXIS_Z),
+					0};
 
 static irqreturn_t bmc150_magn_trigger_handler(int irq, void *p)
 {
-- 
2.4.3

[PATCH 5/8] iio:magnetometer:bmc150_magn: add error handling on gpio probe

[Hartmut Knaack]

bmc150_magn_gpio_probe() may return some error codes, so better check
them.

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/magnetometer/bmc150_magn.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
index f1b6a4dc40ea..e1f804b2b244 100644
--- a/drivers/iio/magnetometer/bmc150_magn.c
+++ b/drivers/iio/magnetometer/bmc150_magn.c
@@ -920,8 +920,13 @@ static int bmc150_magn_probe(struct i2c_client *client,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &bmc150_magn_info;
 
-	if (client->irq <= 0)
+	if (client->irq <= 0) {
 		client->irq = bmc150_magn_gpio_probe(client);
+		if (client->irq < 0) {
+			ret = client->irq;
+			goto err_poweroff;
+		}
+	}
 
 	if (client->irq > 0) {
 		data->dready_trig = devm_iio_trigger_alloc(&client->dev,
-- 
2.4.3

[PATCH 6/8] iio:magnetometer:bmc150_magn: protect runtime_resume with mutex

[Hartmut Knaack]

Protect bmc150_magn_runtime_resume() with a mutex, as done with other
suspend/resume functions.

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/magnetometer/bmc150_magn.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
index e1f804b2b244..ac0cdbde813b 100644
--- a/drivers/iio/magnetometer/bmc150_magn.c
+++ b/drivers/iio/magnetometer/bmc150_magn.c
@@ -1052,9 +1052,14 @@ static int bmc150_magn_runtime_resume(struct device *dev)
 {
 	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
 	struct bmc150_magn_data *data = iio_priv(indio_dev);
+	int ret;
 
-	return bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_NORMAL,
-					  true);
+	mutex_lock(&data->mutex);
+	ret = bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_NORMAL,
+					 true);
+	mutex_unlock(&data->mutex);
+
+	return ret;
 }
 #endif
 
-- 
2.4.3

[PATCH 7/8] iio:magnetometer:bmc150_magn: expand mutex in trigger_handler

[Hartmut Knaack]

Keep the mutex locked, until the content of data->buffer has been pushed
out.

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/magnetometer/bmc150_magn.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
index ac0cdbde813b..bdcf49ee75df 100644
--- a/drivers/iio/magnetometer/bmc150_magn.c
+++ b/drivers/iio/magnetometer/bmc150_magn.c
@@ -664,7 +664,6 @@ static irqreturn_t bmc150_magn_trigger_handler(int irq, void *p)
 
 	mutex_lock(&data->mutex);
 	ret = bmc150_magn_read_xyz(data, data->buffer);
-	mutex_unlock(&data->mutex);
 	if (ret < 0)
 		goto err;
 
@@ -672,6 +671,7 @@ static irqreturn_t bmc150_magn_trigger_handler(int irq, void *p)
 					   pf->timestamp);
 
 err:
+	mutex_unlock(&data->mutex);
 	iio_trigger_notify_done(indio_dev->trig);
 
 	return IRQ_HANDLED;
-- 
2.4.3

[PATCH 8/8] iio:magnetometer:bmc150_magn: use descriptive name for mask

[Hartmut Knaack]

Define and use a descriptive name for the repetition registers data mask,
instead of a 'magic' value.

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/magnetometer/bmc150_magn.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
index bdcf49ee75df..e8cf08ee2d15 100644
--- a/drivers/iio/magnetometer/bmc150_magn.c
+++ b/drivers/iio/magnetometer/bmc150_magn.c
@@ -85,6 +85,7 @@
 #define BMC150_MAGN_REG_HIGH_THRESH		0x50
 #define BMC150_MAGN_REG_REP_XY			0x51
 #define BMC150_MAGN_REG_REP_Z			0x52
+#define BMC150_MAGN_REG_REP_DATAMASK		GENMASK(7, 0)
 
 #define BMC150_MAGN_REG_TRIM_START		0x5D
 #define BMC150_MAGN_REG_TRIM_END		0x71
@@ -559,7 +560,7 @@ static int bmc150_magn_write_raw(struct iio_dev *indio_dev,
 			}
 			ret = regmap_update_bits(data->regmap,
 						 BMC150_MAGN_REG_REP_XY,
-						 0xFF,
+						 BMC150_MAGN_REG_REP_DATAMASK,
 						 BMC150_MAGN_REPXY_TO_REGVAL
 						 (val));
 			mutex_unlock(&data->mutex);
@@ -575,7 +576,7 @@ static int bmc150_magn_write_raw(struct iio_dev *indio_dev,
 			}
 			ret = regmap_update_bits(data->regmap,
 						 BMC150_MAGN_REG_REP_Z,
-						 0xFF,
+						 BMC150_MAGN_REG_REP_DATAMASK,
 						 BMC150_MAGN_REPZ_TO_REGVAL
 						 (val));
 			mutex_unlock(&data->mutex);
-- 
2.4.3

Re: [PATCH 1/8] iio:magnetometer:bmc150_magn: add regmap dependency

[Jonathan Cameron]

On 17/07/15 09:52, Hartmut Knaack wrote:
> bmc150_magn makes use of REGMAP_I2C, so select it to build always without
> errors.
> 
> Fixes: c91746a2361d ("iio: magn: Add support for BMC150 magnetometer")
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
Applied to the fixes-togreg branch of iio.git

Thanks,

Jonathan
> ---
>  drivers/iio/magnetometer/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
> index dcadfc4f0661..efb9350b0d76 100644
> --- a/drivers/iio/magnetometer/Kconfig
> +++ b/drivers/iio/magnetometer/Kconfig
> @@ -90,6 +90,7 @@ config IIO_ST_MAGN_SPI_3AXIS
>  config BMC150_MAGN
>  	tristate "Bosch BMC150 Magnetometer Driver"
>  	depends on I2C
> +	select REGMAP_I2C
>  	select IIO_BUFFER
>  	select IIO_TRIGGERED_BUFFER
>  	help
> 

Re: [PATCH 2/8] iio:magnetometer:bmc150_magn: sort entry alphabetically

[Jonathan Cameron]

On 17/07/15 09:52, Hartmut Knaack wrote:
> Sort the entry for bmc105_magn in Kconfig and Makefile to its correct
> position. Also add the minor module information for completeness.
> 
> Fixes: c91746a2361d ("iio: magn: Add support for BMC150 magnetometer")
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
Technically this isn't a fix so will have to go the slow way.  Obviously it's
dependent on the previous patch, so if I forget about it do poke me!

Jonathan
> ---
>  drivers/iio/magnetometer/Kconfig  | 33 ++++++++++++++++++---------------
>  drivers/iio/magnetometer/Makefile |  3 +--
>  2 files changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
> index efb9350b0d76..868abada3409 100644
> --- a/drivers/iio/magnetometer/Kconfig
> +++ b/drivers/iio/magnetometer/Kconfig
> @@ -24,6 +24,24 @@ config AK09911
>  	help
>  	  Deprecated: AK09911 is now supported by AK8975 driver.
>  
> +config BMC150_MAGN
> +	tristate "Bosch BMC150 Magnetometer Driver"
> +	depends on I2C
> +	select REGMAP_I2C
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
> +	help
> +	  Say yes here to build support for the BMC150 magnetometer.
> +
> +	  Currently this only supports the device via an i2c interface.
> +
> +	  This is a combo module with both accelerometer and magnetometer.
> +	  This driver is only implementing magnetometer part, which has
> +	  its own address and register map.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called bmc150_magn.
> +
>  config MAG3110
>  	tristate "Freescale MAG3110 3-Axis Magnetometer"
>  	depends on I2C
> @@ -87,19 +105,4 @@ config IIO_ST_MAGN_SPI_3AXIS
>  	depends on IIO_ST_MAGN_3AXIS
>  	depends on IIO_ST_SENSORS_SPI
>  
> -config BMC150_MAGN
> -	tristate "Bosch BMC150 Magnetometer Driver"
> -	depends on I2C
> -	select REGMAP_I2C
> -	select IIO_BUFFER
> -	select IIO_TRIGGERED_BUFFER
> -	help
> -	  Say yes here to build support for the BMC150 magnetometer.
> -
> -	  Currently this only supports the device via an i2c interface.
> -
> -	  This is a combo module with both accelerometer and magnetometer.
> -	  This driver is only implementing magnetometer part, which has
> -	  its own address and register map.
> -
>  endmenu
> diff --git a/drivers/iio/magnetometer/Makefile b/drivers/iio/magnetometer/Makefile
> index 33b1d4d54ee7..2c72df458ec2 100644
> --- a/drivers/iio/magnetometer/Makefile
> +++ b/drivers/iio/magnetometer/Makefile
> @@ -4,6 +4,7 @@
>  
>  # When adding new entries keep the list in alphabetical order
>  obj-$(CONFIG_AK8975)	+= ak8975.o
> +obj-$(CONFIG_BMC150_MAGN) += bmc150_magn.o
>  obj-$(CONFIG_MAG3110)	+= mag3110.o
>  obj-$(CONFIG_HID_SENSOR_MAGNETOMETER_3D) += hid-sensor-magn-3d.o
>  obj-$(CONFIG_MMC35240)	+= mmc35240.o
> @@ -14,5 +15,3 @@ st_magn-$(CONFIG_IIO_BUFFER) += st_magn_buffer.o
>  
>  obj-$(CONFIG_IIO_ST_MAGN_I2C_3AXIS) += st_magn_i2c.o
>  obj-$(CONFIG_IIO_ST_MAGN_SPI_3AXIS) += st_magn_spi.o
> -
> -obj-$(CONFIG_BMC150_MAGN) += bmc150_magn.o
> 

Re: [PATCH 3/8] iio:magnetometer:bmc150_magn: output intended variable

[Jonathan Cameron]

On 17/07/15 09:52, Hartmut Knaack wrote:
> According to the debug/error string, the content of chip_id is supposed to
> be output, rather than the return value of the previous operation.
> 
> Fixes: c91746a2361d ("iio: magn: Add support for BMC150 magnetometer")
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
Applied to the fixes-togreg branch of iio.git.

Thanks,

Jonathan
> ---
>  drivers/iio/magnetometer/bmc150_magn.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
> index cd002710dd02..7da4ce9409e8 100644
> --- a/drivers/iio/magnetometer/bmc150_magn.c
> +++ b/drivers/iio/magnetometer/bmc150_magn.c
> @@ -694,11 +694,11 @@ static int bmc150_magn_init(struct bmc150_magn_data *data)
>  		goto err_poweroff;
>  	}
>  	if (chip_id != BMC150_MAGN_CHIP_ID_VAL) {
> -		dev_err(&data->client->dev, "Invalid chip id 0x%x\n", ret);
> +		dev_err(&data->client->dev, "Invalid chip id 0x%x\n", chip_id);
>  		ret = -ENODEV;
>  		goto err_poweroff;
>  	}
> -	dev_dbg(&data->client->dev, "Chip id %x\n", ret);
> +	dev_dbg(&data->client->dev, "Chip id %x\n", chip_id);
>  
>  	preset = bmc150_magn_presets_table[BMC150_MAGN_DEFAULT_PRESET];
>  	ret = bmc150_magn_set_odr(data, preset.odr);
> 

Re: [PATCH 4/8] iio:magnetometer:bmc150_magn: replace magic value

[Jonathan Cameron]

On 17/07/15 09:52, Hartmut Knaack wrote:
> Construct the scanmask using its descriptive axis names (as used in
> iio_chan_spec) instead of a 'magic' value.
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
Applied to the togreg branch of iio.git, pushed out as
testing for the autobuilders to play with it.

Thanks,

Jonathan
> ---
> Does this look better?
Certainly slightly clearer what is going on.
> 
>  drivers/iio/magnetometer/bmc150_magn.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
> index 7da4ce9409e8..f1b6a4dc40ea 100644
> --- a/drivers/iio/magnetometer/bmc150_magn.c
> +++ b/drivers/iio/magnetometer/bmc150_magn.c
> @@ -651,7 +651,9 @@ static const struct iio_info bmc150_magn_info = {
>  	.driver_module = THIS_MODULE,
>  };
>  
> -static const unsigned long bmc150_magn_scan_masks[] = {0x07, 0};
> +static const unsigned long bmc150_magn_scan_masks[] = {
> +					BIT(AXIS_X) | BIT(AXIS_Y) | BIT(AXIS_Z),
> +					0};
>  
>  static irqreturn_t bmc150_magn_trigger_handler(int irq, void *p)
>  {
> 

Re: [PATCH 5/8] iio:magnetometer:bmc150_magn: add error handling on gpio probe

[Jonathan Cameron]

On 17/07/15 09:52, Hartmut Knaack wrote:
> bmc150_magn_gpio_probe() may return some error codes, so better check
> them.
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
Another kind of obvious improvement.  Obviously comments still welcome
from others.  Applied to the togreg branch of iio.git

Thanks,

Jonathan
> ---
>  drivers/iio/magnetometer/bmc150_magn.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
> index f1b6a4dc40ea..e1f804b2b244 100644
> --- a/drivers/iio/magnetometer/bmc150_magn.c
> +++ b/drivers/iio/magnetometer/bmc150_magn.c
> @@ -920,8 +920,13 @@ static int bmc150_magn_probe(struct i2c_client *client,
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->info = &bmc150_magn_info;
>  
> -	if (client->irq <= 0)
> +	if (client->irq <= 0) {
>  		client->irq = bmc150_magn_gpio_probe(client);
> +		if (client->irq < 0) {
> +			ret = client->irq;
> +			goto err_poweroff;
> +		}
> +	}
>  
>  	if (client->irq > 0) {
>  		data->dready_trig = devm_iio_trigger_alloc(&client->dev,
> 

Re: [PATCH 6/8] iio:magnetometer:bmc150_magn: protect runtime_resume with mutex

[Jonathan Cameron]

On 17/07/15 09:52, Hartmut Knaack wrote:
> Protect bmc150_magn_runtime_resume() with a mutex, as done with other
> suspend/resume functions.
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
Looks fine to me, but given we are getting into more complex patches, I'll
let this sit on the list for Irina and others to have a look at.

Jonathan
> ---
>  drivers/iio/magnetometer/bmc150_magn.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
> index e1f804b2b244..ac0cdbde813b 100644
> --- a/drivers/iio/magnetometer/bmc150_magn.c
> +++ b/drivers/iio/magnetometer/bmc150_magn.c
> @@ -1052,9 +1052,14 @@ static int bmc150_magn_runtime_resume(struct device *dev)
>  {
>  	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>  	struct bmc150_magn_data *data = iio_priv(indio_dev);
> +	int ret;
>  
> -	return bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_NORMAL,
> -					  true);
> +	mutex_lock(&data->mutex);
> +	ret = bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_NORMAL,
> +					 true);
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
>  }
>  #endif
>  
> 

RE: [PATCH 0/8] bmc150_magn fix and cleanup

[Tirdea, Irina]

> -----Original Message-----
> From: Hartmut Knaack [mailto:knaack.h@gmx.de]
> Sent: 17 July, 2015 11:52
> To: linux-iio@vger.kernel.org
> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Tirdea, Irina; Dogaru, Vlad
> Subject: [PATCH 0/8] bmc150_magn fix and cleanup
> 
> Some issues I spotted during review of this driver. First three are quite
> obvious problems, the rest may be matter of taste or intended to stay
> in the original form. Opinions welcome.
> 

Hi Hartmut,

Thanks for taking the time to cleanup these issues. I know you did not get
to review this code before merge, so I was expecting your feedback :)

I see Jonathan has already merged some of the changes, so I'm just going
to reply to the rest.
I am fine with the first 4 changes which already got merged anyway.

Thanks,
Irina

> Hartmut Knaack (8):
>   iio:magnetometer:bmc150_magn: add regmap dependency
>   iio:magnetometer:bmc150_magn: sort entry alphabetically
>   iio:magnetometer:bmc150_magn: output intended variable
>   iio:magnetometer:bmc150_magn: replace magic value
>   iio:magnetometer:bmc150_magn: add error handling on gpio probe
>   iio:magnetometer:bmc150_magn: protect runtime_resume with mutex
>   iio:magnetometer:bmc150_magn: expand mutex in trigger_handler
>   iio:magnetometer:bmc150_magn: use descriptive name for mask
> 
>  drivers/iio/magnetometer/Kconfig       | 32 ++++++++++++++++++--------------
>  drivers/iio/magnetometer/Makefile      |  3 +--
>  drivers/iio/magnetometer/bmc150_magn.c | 31 ++++++++++++++++++++++---------
>  3 files changed, 41 insertions(+), 25 deletions(-)
> 
> --
> 2.4.3

RE: [PATCH 5/8] iio:magnetometer:bmc150_magn: add error handling on gpio probe

[Tirdea, Irina]

> -----Original Message-----
> From: Hartmut Knaack [mailto:knaack.h@gmx.de]
> Sent: 17 July, 2015 11:52
> To: linux-iio@vger.kernel.org
> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Tirdea, Irina; Dogaru, Vlad
> Subject: [PATCH 5/8] iio:magnetometer:bmc150_magn: add error handling on gpio probe
> 
> bmc150_magn_gpio_probe() may return some error codes, so better check
> them.
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> ---
>  drivers/iio/magnetometer/bmc150_magn.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
> index f1b6a4dc40ea..e1f804b2b244 100644
> --- a/drivers/iio/magnetometer/bmc150_magn.c
> +++ b/drivers/iio/magnetometer/bmc150_magn.c
> @@ -920,8 +920,13 @@ static int bmc150_magn_probe(struct i2c_client *client,
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->info = &bmc150_magn_info;
> 
> -	if (client->irq <= 0)
> +	if (client->irq <= 0) {
>  		client->irq = bmc150_magn_gpio_probe(client);
> +		if (client->irq < 0) {
> +			ret = client->irq;
> +			goto err_poweroff;
> +		}
> +	}

The error code for client->irq was not checked on purpose. We would like to be able to use this driver
even if the interrupt pin is not connected (reading from sysfs in poll mode or by using iio hrtimers). 

If the i2c core does not initialize the irq, we try to initialize it through ACPI in bmc150_magn_gpio_probe
and if that fails too we do not register the interrupt and iio trigger (see check for client->irq >0 below).
In this way we could still use the device even if we don't have interrupts.

> 
>  	if (client->irq > 0) {
>  		data->dready_trig = devm_iio_trigger_alloc(&client->dev,
> --
> 2.4.3

Re: [PATCH 5/8] iio:magnetometer:bmc150_magn: add error handling on gpio probe

[Jonathan Cameron]

On 20/07/15 18:36, Tirdea, Irina wrote:
> 
> 
>> -----Original Message-----
>> From: Hartmut Knaack [mailto:knaack.h@gmx.de]
>> Sent: 17 July, 2015 11:52
>> To: linux-iio@vger.kernel.org
>> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Tirdea, Irina; Dogaru, Vlad
>> Subject: [PATCH 5/8] iio:magnetometer:bmc150_magn: add error handling on gpio probe
>>
>> bmc150_magn_gpio_probe() may return some error codes, so better check
>> them.
>>
>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
>> ---
>>  drivers/iio/magnetometer/bmc150_magn.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
>> index f1b6a4dc40ea..e1f804b2b244 100644
>> --- a/drivers/iio/magnetometer/bmc150_magn.c
>> +++ b/drivers/iio/magnetometer/bmc150_magn.c
>> @@ -920,8 +920,13 @@ static int bmc150_magn_probe(struct i2c_client *client,
>>  	indio_dev->modes = INDIO_DIRECT_MODE;
>>  	indio_dev->info = &bmc150_magn_info;
>>
>> -	if (client->irq <= 0)
>> +	if (client->irq <= 0) {
>>  		client->irq = bmc150_magn_gpio_probe(client);
>> +		if (client->irq < 0) {
>> +			ret = client->irq;
>> +			goto err_poweroff;
>> +		}
>> +	}
> 
> The error code for client->irq was not checked on purpose. We would like to be able to use this driver
> even if the interrupt pin is not connected (reading from sysfs in poll mode or by using iio hrtimers). 
> 
> If the i2c core does not initialize the irq, we try to initialize it through ACPI in bmc150_magn_gpio_probe
> and if that fails too we do not register the interrupt and iio trigger (see check for client->irq >0 below).
> In this way we could still use the device even if we don't have interrupts.
> 
One of the advantages of a pre push to testing is I can still rebase when
I jump the gun and mess up like this ;)

Anyhow, patch dropped and testing re-pushed out.

Sorry about that, should have left these a while on the list before applying.

Jonathan

>>
>>  	if (client->irq > 0) {
>>  		data->dready_trig = devm_iio_trigger_alloc(&client->dev,
>> --
>> 2.4.3
> 
> --
> 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
> 

RE: [PATCH 6/8] iio:magnetometer:bmc150_magn: protect runtime_resume with mutex

[Tirdea, Irina]

> -----Original Message-----
> From: Hartmut Knaack [mailto:knaack.h@gmx.de]
> Sent: 17 July, 2015 11:52
> To: linux-iio@vger.kernel.org
> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Tirdea, Irina; =
Dogaru, Vlad
> Subject: [PATCH 6/8] iio:magnetometer:bmc150_magn: protect runtime_resume=
 with mutex
>=20
> Protect bmc150_magn_runtime_resume() with a mutex, as done with other
> suspend/resume functions.
>=20
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> ---
>  drivers/iio/magnetometer/bmc150_magn.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>=20
> diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magneto=
meter/bmc150_magn.c
> index e1f804b2b244..ac0cdbde813b 100644
> --- a/drivers/iio/magnetometer/bmc150_magn.c
> +++ b/drivers/iio/magnetometer/bmc150_magn.c
> @@ -1052,9 +1052,14 @@ static int bmc150_magn_runtime_resume(struct devic=
e *dev)
>  {
>  	struct iio_dev *indio_dev =3D i2c_get_clientdata(to_i2c_client(dev));
>  	struct bmc150_magn_data *data =3D iio_priv(indio_dev);
> +	int ret;
>=20
> -	return bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_NORMAL,
> -					  true);
> +	mutex_lock(&data->mutex);
> +	ret =3D bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_NORMAL,
> +					 true);
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;

This function does not have mutex_lock/unlock because it is called with dat=
a->mutex locked.
I should have added a comment to specify this.

Whenever we wake up the device, bmc150_magn_set_power_state is called after=
 locking
data->mutex. This in turn calls pm_runtime_get_sync which will immediately =
call
bmc150_magn_runtime_resume. Trying to lock the mutex here would lead to a d=
eadlock
(e.g. when trying to read a value through sysfs _raw).

Since this driver uses autosuspend delay, the situation is different for ru=
ntime suspend since
that is called after 2 seconds in a separate thread. This is why there is a=
 mutex locked in runtime
suspend but not in runtime resume.

>  }
>  #endif
>=20
> --
> 2.4.3

RE: [PATCH 7/8] iio:magnetometer:bmc150_magn: expand mutex in trigger_handler

[Tirdea, Irina]

> -----Original Message-----
> From: Hartmut Knaack [mailto:knaack.h@gmx.de]
> Sent: 17 July, 2015 11:53
> To: linux-iio@vger.kernel.org
> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Tirdea, Irina; Dogaru, Vlad
> Subject: [PATCH 7/8] iio:magnetometer:bmc150_magn: expand mutex in trigger_handler
> 
> Keep the mutex locked, until the content of data->buffer has been pushed
> out.
> 

Looks good to me.

Acked-by: Irina Tirdea <irina.tirdea@intel.com>

> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> ---
>  drivers/iio/magnetometer/bmc150_magn.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
> index ac0cdbde813b..bdcf49ee75df 100644
> --- a/drivers/iio/magnetometer/bmc150_magn.c
> +++ b/drivers/iio/magnetometer/bmc150_magn.c
> @@ -664,7 +664,6 @@ static irqreturn_t bmc150_magn_trigger_handler(int irq, void *p)
> 
>  	mutex_lock(&data->mutex);
>  	ret = bmc150_magn_read_xyz(data, data->buffer);
> -	mutex_unlock(&data->mutex);
>  	if (ret < 0)
>  		goto err;
> 
> @@ -672,6 +671,7 @@ static irqreturn_t bmc150_magn_trigger_handler(int irq, void *p)
>  					   pf->timestamp);
> 
>  err:
> +	mutex_unlock(&data->mutex);
>  	iio_trigger_notify_done(indio_dev->trig);
> 
>  	return IRQ_HANDLED;
> --
> 2.4.3

Re: [PATCH 0/8] bmc150_magn fix and cleanup

[Jonathan Cameron]

On 20/07/15 18:30, Tirdea, Irina wrote:
> 
> 
>> -----Original Message-----
>> From: Hartmut Knaack [mailto:knaack.h@gmx.de]
>> Sent: 17 July, 2015 11:52
>> To: linux-iio@vger.kernel.org
>> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Tirdea, Irina; Dogaru, Vlad
>> Subject: [PATCH 0/8] bmc150_magn fix and cleanup
>>
>> Some issues I spotted during review of this driver. First three are quite
>> obvious problems, the rest may be matter of taste or intended to stay
>> in the original form. Opinions welcome.
>>
> 
> Hi Hartmut,
> 
> Thanks for taking the time to cleanup these issues. I know you did not get
> to review this code before merge, so I was expecting your feedback :)
> 
> I see Jonathan has already merged some of the changes, so I'm just going
> to reply to the rest.
> I am fine with the first 4 changes which already got merged anyway.
Feel free to offer comments on the rest as well.  I was on a bit of a roll
and not giving sufficient time to others to review these.  The
'obviously' correct are not always so obvious given patch 5!

Sorry about that

Jonathan
> 
> Thanks,
> Irina
> 
>> Hartmut Knaack (8):
>>   iio:magnetometer:bmc150_magn: add regmap dependency
>>   iio:magnetometer:bmc150_magn: sort entry alphabetically
>>   iio:magnetometer:bmc150_magn: output intended variable
>>   iio:magnetometer:bmc150_magn: replace magic value
>>   iio:magnetometer:bmc150_magn: add error handling on gpio probe
>>   iio:magnetometer:bmc150_magn: protect runtime_resume with mutex
>>   iio:magnetometer:bmc150_magn: expand mutex in trigger_handler
>>   iio:magnetometer:bmc150_magn: use descriptive name for mask
>>
>>  drivers/iio/magnetometer/Kconfig       | 32 ++++++++++++++++++--------------
>>  drivers/iio/magnetometer/Makefile      |  3 +--
>>  drivers/iio/magnetometer/bmc150_magn.c | 31 ++++++++++++++++++++++---------
>>  3 files changed, 41 insertions(+), 25 deletions(-)
>>
>> --
>> 2.4.3
> 
> --
> 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
> 

RE: [PATCH 8/8] iio:magnetometer:bmc150_magn: use descriptive name for mask

[Tirdea, Irina]

> -----Original Message-----
> From: Hartmut Knaack [mailto:knaack.h@gmx.de]
> Sent: 17 July, 2015 11:53
> To: linux-iio@vger.kernel.org
> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Tirdea, Irina; Dogaru, Vlad
> Subject: [PATCH 8/8] iio:magnetometer:bmc150_magn: use descriptive name for mask
> 
> Define and use a descriptive name for the repetition registers data mask,
> instead of a 'magic' value.
>

Missed this hardcoded value. Thanks for catching this!

Acked-by: Irina Tirdea <irina.tirdea@intel.com>
 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> ---
>  drivers/iio/magnetometer/bmc150_magn.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
> index bdcf49ee75df..e8cf08ee2d15 100644
> --- a/drivers/iio/magnetometer/bmc150_magn.c
> +++ b/drivers/iio/magnetometer/bmc150_magn.c
> @@ -85,6 +85,7 @@
>  #define BMC150_MAGN_REG_HIGH_THRESH		0x50
>  #define BMC150_MAGN_REG_REP_XY			0x51
>  #define BMC150_MAGN_REG_REP_Z			0x52
> +#define BMC150_MAGN_REG_REP_DATAMASK		GENMASK(7, 0)
> 
>  #define BMC150_MAGN_REG_TRIM_START		0x5D
>  #define BMC150_MAGN_REG_TRIM_END		0x71
> @@ -559,7 +560,7 @@ static int bmc150_magn_write_raw(struct iio_dev *indio_dev,
>  			}
>  			ret = regmap_update_bits(data->regmap,
>  						 BMC150_MAGN_REG_REP_XY,
> -						 0xFF,
> +						 BMC150_MAGN_REG_REP_DATAMASK,
>  						 BMC150_MAGN_REPXY_TO_REGVAL
>  						 (val));
>  			mutex_unlock(&data->mutex);
> @@ -575,7 +576,7 @@ static int bmc150_magn_write_raw(struct iio_dev *indio_dev,
>  			}
>  			ret = regmap_update_bits(data->regmap,
>  						 BMC150_MAGN_REG_REP_Z,
> -						 0xFF,
> +						 BMC150_MAGN_REG_REP_DATAMASK,
>  						 BMC150_MAGN_REPZ_TO_REGVAL
>  						 (val));
>  			mutex_unlock(&data->mutex);
> --
> 2.4.3

RE: [PATCH 0/8] bmc150_magn fix and cleanup

[Tirdea, Irina]

> -----Original Message-----
> From: linux-iio-owner@vger.kernel.org [mailto:linux-iio-owner@vger.kernel=
.org] On Behalf Of Jonathan Cameron
> Sent: 20 July, 2015 20:49
> To: Tirdea, Irina; Hartmut Knaack; linux-iio@vger.kernel.org
> Cc: Lars-Peter Clausen; Peter Meerwald; Dogaru, Vlad
> Subject: Re: [PATCH 0/8] bmc150_magn fix and cleanup
>=20
> On 20/07/15 18:30, Tirdea, Irina wrote:
> >
> >
> >> -----Original Message-----
> >> From: Hartmut Knaack [mailto:knaack.h@gmx.de]
> >> Sent: 17 July, 2015 11:52
> >> To: linux-iio@vger.kernel.org
> >> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Tirdea, Irin=
a; Dogaru, Vlad
> >> Subject: [PATCH 0/8] bmc150_magn fix and cleanup
> >>
> >> Some issues I spotted during review of this driver. First three are qu=
ite
> >> obvious problems, the rest may be matter of taste or intended to stay
> >> in the original form. Opinions welcome.
> >>
> >
> > Hi Hartmut,
> >
> > Thanks for taking the time to cleanup these issues. I know you did not =
get
> > to review this code before merge, so I was expecting your feedback :)
> >
> > I see Jonathan has already merged some of the changes, so I'm just goin=
g
> > to reply to the rest.
> > I am fine with the first 4 changes which already got merged anyway.
> Feel free to offer comments on the rest as well.  I was on a bit of a rol=
l
> and not giving sufficient time to others to review these.  The
> 'obviously' correct are not always so obvious given patch 5!
>=20
> Sorry about that
>=20

No problem, Jonathan.=20
I did review the rest of the patches as well and they look good to me.

Thanks,
Irina

> Jonathan
> >
> > Thanks,
> > Irina
> >
> >> Hartmut Knaack (8):
> >>   iio:magnetometer:bmc150_magn: add regmap dependency
> >>   iio:magnetometer:bmc150_magn: sort entry alphabetically
> >>   iio:magnetometer:bmc150_magn: output intended variable
> >>   iio:magnetometer:bmc150_magn: replace magic value
> >>   iio:magnetometer:bmc150_magn: add error handling on gpio probe
> >>   iio:magnetometer:bmc150_magn: protect runtime_resume with mutex
> >>   iio:magnetometer:bmc150_magn: expand mutex in trigger_handler
> >>   iio:magnetometer:bmc150_magn: use descriptive name for mask
> >>
> >>  drivers/iio/magnetometer/Kconfig       | 32 ++++++++++++++++++-------=
-------
> >>  drivers/iio/magnetometer/Makefile      |  3 +--
> >>  drivers/iio/magnetometer/bmc150_magn.c | 31 ++++++++++++++++++++++---=
------
> >>  3 files changed, 41 insertions(+), 25 deletions(-)
> >>
> >> --
> >> 2.4.3
> >
> > --
> > 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
> >
>=20
> --
> 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

Re: [PATCH 6/8] iio:magnetometer:bmc150_magn: protect runtime_resume with mutex

[Hartmut Knaack]

Tirdea, Irina schrieb am 20.07.2015 um 19:45:
> 
> 
>> -----Original Message-----
>> From: Hartmut Knaack [mailto:knaack.h@gmx.de]
>> Sent: 17 July, 2015 11:52
>> To: linux-iio@vger.kernel.org
>> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Tirdea, Irina; Dogaru, Vlad
>> Subject: [PATCH 6/8] iio:magnetometer:bmc150_magn: protect runtime_resume with mutex
>>
>> Protect bmc150_magn_runtime_resume() with a mutex, as done with other
>> suspend/resume functions.
>>
>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
>> ---
>>  drivers/iio/magnetometer/bmc150_magn.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
>> index e1f804b2b244..ac0cdbde813b 100644
>> --- a/drivers/iio/magnetometer/bmc150_magn.c
>> +++ b/drivers/iio/magnetometer/bmc150_magn.c
>> @@ -1052,9 +1052,14 @@ static int bmc150_magn_runtime_resume(struct device *dev)
>>  {
>>  	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>>  	struct bmc150_magn_data *data = iio_priv(indio_dev);
>> +	int ret;
>>
>> -	return bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_NORMAL,
>> -					  true);
>> +	mutex_lock(&data->mutex);
>> +	ret = bmc150_magn_set_power_mode(data, BMC150_MAGN_POWER_MODE_NORMAL,
>> +					 true);
>> +	mutex_unlock(&data->mutex);
>> +
>> +	return ret;
> 
> This function does not have mutex_lock/unlock because it is called with data->mutex locked.
> I should have added a comment to specify this.
> 
> Whenever we wake up the device, bmc150_magn_set_power_state is called after locking
> data->mutex. This in turn calls pm_runtime_get_sync which will immediately call
> bmc150_magn_runtime_resume. Trying to lock the mutex here would lead to a deadlock
> (e.g. when trying to read a value through sysfs _raw).
> 
> Since this driver uses autosuspend delay, the situation is different for runtime suspend since
> that is called after 2 seconds in a separate thread. This is why there is a mutex locked in runtime
> suspend but not in runtime resume.
> 

Yes, please add such a comment. As for me, it seems I have to dig a bit
deeper into power management magic - maybe another day ;-)
Thanks,

Hartmut

>>  }
>>  #endif
>>
>> --
>> 2.4.3
> 
> --
> 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
> 

Re: [PATCH 7/8] iio:magnetometer:bmc150_magn: expand mutex in trigger_handler

[Jonathan Cameron]

On 20/07/15 18:48, Tirdea, Irina wrote:
> 
> 
>> -----Original Message-----
>> From: Hartmut Knaack [mailto:knaack.h@gmx.de]
>> Sent: 17 July, 2015 11:53
>> To: linux-iio@vger.kernel.org
>> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Tirdea, Irina; Dogaru, Vlad
>> Subject: [PATCH 7/8] iio:magnetometer:bmc150_magn: expand mutex in trigger_handler
>>
>> Keep the mutex locked, until the content of data->buffer has been pushed
>> out.
>>
> 
> Looks good to me.
> 
> Acked-by: Irina Tirdea <irina.tirdea@intel.com>
Applied to the togreg branch of iio.git.

Thanks,

> 
>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
>> ---
>>  drivers/iio/magnetometer/bmc150_magn.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
>> index ac0cdbde813b..bdcf49ee75df 100644
>> --- a/drivers/iio/magnetometer/bmc150_magn.c
>> +++ b/drivers/iio/magnetometer/bmc150_magn.c
>> @@ -664,7 +664,6 @@ static irqreturn_t bmc150_magn_trigger_handler(int irq, void *p)
>>
>>  	mutex_lock(&data->mutex);
>>  	ret = bmc150_magn_read_xyz(data, data->buffer);
>> -	mutex_unlock(&data->mutex);
>>  	if (ret < 0)
>>  		goto err;
>>
>> @@ -672,6 +671,7 @@ static irqreturn_t bmc150_magn_trigger_handler(int irq, void *p)
>>  					   pf->timestamp);
>>
>>  err:
>> +	mutex_unlock(&data->mutex);
>>  	iio_trigger_notify_done(indio_dev->trig);
>>
>>  	return IRQ_HANDLED;
>> --
>> 2.4.3
> 
> --
> 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
> 

Re: [PATCH 8/8] iio:magnetometer:bmc150_magn: use descriptive name for mask

[Jonathan Cameron]

On 20/07/15 18:52, Tirdea, Irina wrote:
> 
> 
>> -----Original Message-----
>> From: Hartmut Knaack [mailto:knaack.h@gmx.de]
>> Sent: 17 July, 2015 11:53
>> To: linux-iio@vger.kernel.org
>> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Tirdea, Irina; Dogaru, Vlad
>> Subject: [PATCH 8/8] iio:magnetometer:bmc150_magn: use descriptive name for mask
>>
>> Define and use a descriptive name for the repetition registers data mask,
>> instead of a 'magic' value.
>>
> 
> Missed this hardcoded value. Thanks for catching this!
> 
> Acked-by: Irina Tirdea <irina.tirdea@intel.com>
Applied, thanks!

Jonathan
>  
>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
>> ---
>>  drivers/iio/magnetometer/bmc150_magn.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iio/magnetometer/bmc150_magn.c b/drivers/iio/magnetometer/bmc150_magn.c
>> index bdcf49ee75df..e8cf08ee2d15 100644
>> --- a/drivers/iio/magnetometer/bmc150_magn.c
>> +++ b/drivers/iio/magnetometer/bmc150_magn.c
>> @@ -85,6 +85,7 @@
>>  #define BMC150_MAGN_REG_HIGH_THRESH		0x50
>>  #define BMC150_MAGN_REG_REP_XY			0x51
>>  #define BMC150_MAGN_REG_REP_Z			0x52
>> +#define BMC150_MAGN_REG_REP_DATAMASK		GENMASK(7, 0)
>>
>>  #define BMC150_MAGN_REG_TRIM_START		0x5D
>>  #define BMC150_MAGN_REG_TRIM_END		0x71
>> @@ -559,7 +560,7 @@ static int bmc150_magn_write_raw(struct iio_dev *indio_dev,
>>  			}
>>  			ret = regmap_update_bits(data->regmap,
>>  						 BMC150_MAGN_REG_REP_XY,
>> -						 0xFF,
>> +						 BMC150_MAGN_REG_REP_DATAMASK,
>>  						 BMC150_MAGN_REPXY_TO_REGVAL
>>  						 (val));
>>  			mutex_unlock(&data->mutex);
>> @@ -575,7 +576,7 @@ static int bmc150_magn_write_raw(struct iio_dev *indio_dev,
>>  			}
>>  			ret = regmap_update_bits(data->regmap,
>>  						 BMC150_MAGN_REG_REP_Z,
>> -						 0xFF,
>> +						 BMC150_MAGN_REG_REP_DATAMASK,
>>  						 BMC150_MAGN_REPZ_TO_REGVAL
>>  						 (val));
>>  			mutex_unlock(&data->mutex);
>> --
>> 2.4.3
> 
> --
> 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
> 

Re: [PATCH 2/8] iio:magnetometer:bmc150_magn: sort entry alphabetically

[Jonathan Cameron]

On 19/07/15 11:03, Jonathan Cameron wrote:
> On 17/07/15 09:52, Hartmut Knaack wrote:
>> Sort the entry for bmc105_magn in Kconfig and Makefile to its correct
>> position. Also add the minor module information for completeness.
>>
>> Fixes: c91746a2361d ("iio: magn: Add support for BMC150 magnetometer")
>>
>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> Technically this isn't a fix so will have to go the slow way.  Obviously it's
> dependent on the previous patch, so if I forget about it do poke me!
> 
> Jonathan
Applied.
>> ---
>>  drivers/iio/magnetometer/Kconfig  | 33 ++++++++++++++++++---------------
>>  drivers/iio/magnetometer/Makefile |  3 +--
>>  2 files changed, 19 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
>> index efb9350b0d76..868abada3409 100644
>> --- a/drivers/iio/magnetometer/Kconfig
>> +++ b/drivers/iio/magnetometer/Kconfig
>> @@ -24,6 +24,24 @@ config AK09911
>>  	help
>>  	  Deprecated: AK09911 is now supported by AK8975 driver.
>>  
>> +config BMC150_MAGN
>> +	tristate "Bosch BMC150 Magnetometer Driver"
>> +	depends on I2C
>> +	select REGMAP_I2C
>> +	select IIO_BUFFER
>> +	select IIO_TRIGGERED_BUFFER
>> +	help
>> +	  Say yes here to build support for the BMC150 magnetometer.
>> +
>> +	  Currently this only supports the device via an i2c interface.
>> +
>> +	  This is a combo module with both accelerometer and magnetometer.
>> +	  This driver is only implementing magnetometer part, which has
>> +	  its own address and register map.
>> +
>> +	  To compile this driver as a module, choose M here: the module will be
>> +	  called bmc150_magn.
>> +
>>  config MAG3110
>>  	tristate "Freescale MAG3110 3-Axis Magnetometer"
>>  	depends on I2C
>> @@ -87,19 +105,4 @@ config IIO_ST_MAGN_SPI_3AXIS
>>  	depends on IIO_ST_MAGN_3AXIS
>>  	depends on IIO_ST_SENSORS_SPI
>>  
>> -config BMC150_MAGN
>> -	tristate "Bosch BMC150 Magnetometer Driver"
>> -	depends on I2C
>> -	select REGMAP_I2C
>> -	select IIO_BUFFER
>> -	select IIO_TRIGGERED_BUFFER
>> -	help
>> -	  Say yes here to build support for the BMC150 magnetometer.
>> -
>> -	  Currently this only supports the device via an i2c interface.
>> -
>> -	  This is a combo module with both accelerometer and magnetometer.
>> -	  This driver is only implementing magnetometer part, which has
>> -	  its own address and register map.
>> -
>>  endmenu
>> diff --git a/drivers/iio/magnetometer/Makefile b/drivers/iio/magnetometer/Makefile
>> index 33b1d4d54ee7..2c72df458ec2 100644
>> --- a/drivers/iio/magnetometer/Makefile
>> +++ b/drivers/iio/magnetometer/Makefile
>> @@ -4,6 +4,7 @@
>>  
>>  # When adding new entries keep the list in alphabetical order
>>  obj-$(CONFIG_AK8975)	+= ak8975.o
>> +obj-$(CONFIG_BMC150_MAGN) += bmc150_magn.o
>>  obj-$(CONFIG_MAG3110)	+= mag3110.o
>>  obj-$(CONFIG_HID_SENSOR_MAGNETOMETER_3D) += hid-sensor-magn-3d.o
>>  obj-$(CONFIG_MMC35240)	+= mmc35240.o
>> @@ -14,5 +15,3 @@ st_magn-$(CONFIG_IIO_BUFFER) += st_magn_buffer.o
>>  
>>  obj-$(CONFIG_IIO_ST_MAGN_I2C_3AXIS) += st_magn_i2c.o
>>  obj-$(CONFIG_IIO_ST_MAGN_SPI_3AXIS) += st_magn_spi.o
>> -
>> -obj-$(CONFIG_BMC150_MAGN) += bmc150_magn.o
>>
> 
> --
> 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
>