[PATCH 00/11] iio: ST clean-ups and new pressure sensor device

[PATCH 00/11] iio: ST clean-ups and new pressure sensor device

[Lee Jones]

This patch-set includes a few clean-ups surrounding error handling and
non-mandatory functionality along with regulator support and the addition
of a new pressure/temperature sensor (LPS001WP). Everything has been
tested with Device Tree.

 arch/arm/boot/dts/ste-dbx5x0.dtsi               |   5 --
 arch/arm/boot/dts/ste-snowball.dts              |  10 ++++
 arch/arm/configs/u8500_defconfig                |   4 ++
 drivers/iio/common/st_sensors/st_sensors_core.c |  11 ++--
 drivers/iio/pressure/st_pressure.h              |   1 +
 drivers/iio/pressure/st_pressure_core.c         | 276 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------
 drivers/iio/pressure/st_pressure_i2c.c          |  18 +++----
 include/linux/iio/common/st_sensors.h           |   4 ++
 8 files changed, 230 insertions(+), 99 deletions(-)

[PATCH 01/11] ARM: ux500: Remove PrimeCell IDs from Nomadik I2C DT nodes

[Lee Jones]

Turns out that they're actually not required and the driver probes just
fine without them. The ID is incorrect at the moment anyway. They actually
currently specify the stn8815.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/ste-dbx5x0.dtsi | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/arch/arm/boot/dts/ste-dbx5x0.dtsi b/arch/arm/boot/dts/ste-dbx5x0.dtsi
index a152945..9c01d66 100644
--- a/arch/arm/boot/dts/ste-dbx5x0.dtsi
+++ b/arch/arm/boot/dts/ste-dbx5x0.dtsi
@@ -531,7 +531,6 @@
 			compatible = "stericsson,db8500-i2c", "st,nomadik-i2c", "arm,primecell";
 			reg = <0x80004000 0x1000>;
 			interrupts = <0 21 IRQ_TYPE_LEVEL_HIGH>;
-			arm,primecell-periphid = <0x180024>;
 
 			#address-cells = <1>;
 			#size-cells = <0>;
@@ -544,7 +543,6 @@
 			compatible = "stericsson,db8500-i2c", "st,nomadik-i2c", "arm,primecell";
 			reg = <0x80122000 0x1000>;
 			interrupts = <0 22 IRQ_TYPE_LEVEL_HIGH>;
-			arm,primecell-periphid = <0x180024>;
 
 			#address-cells = <1>;
 			#size-cells = <0>;
@@ -557,7 +555,6 @@
 			compatible = "stericsson,db8500-i2c", "st,nomadik-i2c", "arm,primecell";
 			reg = <0x80128000 0x1000>;
 			interrupts = <0 55 IRQ_TYPE_LEVEL_HIGH>;
-			arm,primecell-periphid = <0x180024>;
 
 			#address-cells = <1>;
 			#size-cells = <0>;
@@ -570,7 +567,6 @@
 			compatible = "stericsson,db8500-i2c", "st,nomadik-i2c", "arm,primecell";
 			reg = <0x80110000 0x1000>;
 			interrupts = <0 12 IRQ_TYPE_LEVEL_HIGH>;
-			arm,primecell-periphid = <0x180024>;
 
 			#address-cells = <1>;
 			#size-cells = <0>;
@@ -583,7 +579,6 @@
 			compatible = "stericsson,db8500-i2c", "st,nomadik-i2c", "arm,primecell";
 			reg = <0x8012a000 0x1000>;
 			interrupts = <0 51 IRQ_TYPE_LEVEL_HIGH>;
-			arm,primecell-periphid = <0x180024>;
 
 			#address-cells = <1>;
 			#size-cells = <0>;
-- 
1.8.1.2

[PATCH 02/11] ARM: ux500: Enable the LPS001WP Pressure & Temperature sensor from DT

[Lee Jones]

After applying this node the LPS001WP sensor chip should probe
successfully once the driver support has also been applied.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/boot/dts/ste-snowball.dts | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/ste-snowball.dts b/arch/arm/boot/dts/ste-snowball.dts
index cf9b16e..aad8f54 100644
--- a/arch/arm/boot/dts/ste-snowball.dts
+++ b/arch/arm/boot/dts/ste-snowball.dts
@@ -153,6 +153,16 @@
 			status = "okay";
 		};
 
+		i2c@80128000 {
+			lps001wp@5c {
+				compatible = "lps001wp";
+				reg = <0x5c>;
+
+				vdd-supply = <&ab8500_ldo_aux1_reg>;
+				vms-supply = <&db8500_vsmps2_reg>;
+			};
+		};
+
 		uart@80120000 {
 			status = "okay";
 		};
-- 
1.8.1.2

[PATCH 03/11] ARM: ux500: CONFIG: Enable ST's IIO Pressure Sensors by default

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 arch/arm/configs/u8500_defconfig | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/configs/u8500_defconfig b/arch/arm/configs/u8500_defconfig
index a0025dc..d77aa57 100644
--- a/arch/arm/configs/u8500_defconfig
+++ b/arch/arm/configs/u8500_defconfig
@@ -37,6 +37,10 @@ CONFIG_UEVENT_HELPER_PATH="/sbin/hotplug"
 CONFIG_BLK_DEV_RAM=y
 CONFIG_BLK_DEV_RAM_SIZE=65536
 CONFIG_SENSORS_BH1780=y
+CONFIG_IIO=y
+CONFIG_IIO_ST_PRESS=y
+CONFIG_IIO_ST_PRESS_I2C=y
+CONFIG_IIO_ST_SENSORS_I2C=y
 CONFIG_NETDEVICES=y
 CONFIG_SMSC911X=y
 CONFIG_SMSC_PHY=y
-- 
1.8.1.2

[PATCH 04/11] iio: pressure-i2c: st: Simplify error checking in probe()

[Lee Jones]

Strip out all the unnecessary gotos and check for NULL returns in the
usual manner.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/iio/pressure/st_pressure_i2c.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/pressure/st_pressure_i2c.c b/drivers/iio/pressure/st_pressure_i2c.c
index 7cebcc7..2ace770 100644
--- a/drivers/iio/pressure/st_pressure_i2c.c
+++ b/drivers/iio/pressure/st_pressure_i2c.c
@@ -26,10 +26,8 @@ static int st_press_i2c_probe(struct i2c_client *client,
 	int err;
 
 	indio_dev = iio_device_alloc(sizeof(*pdata));
-	if (indio_dev == NULL) {
-		err = -ENOMEM;
-		goto iio_device_alloc_error;
-	}
+	if (!indio_dev)
+		return -ENOMEM;
 
 	pdata = iio_priv(indio_dev);
 	pdata->dev = &client->dev;
@@ -37,15 +35,12 @@ static int st_press_i2c_probe(struct i2c_client *client,
 	st_sensors_i2c_configure(indio_dev, client, pdata);
 
 	err = st_press_common_probe(indio_dev);
-	if (err < 0)
-		goto st_press_common_probe_error;
+	if (err < 0) {
+		iio_device_free(indio_dev);
+		return err;
+	}
 
 	return 0;
-
-st_press_common_probe_error:
-	iio_device_free(indio_dev);
-iio_device_alloc_error:
-	return err;
 }
 
 static int st_press_i2c_remove(struct i2c_client *client)
-- 
1.8.1.2

[PATCH 05/11] iio: pressure-core: st: Describe LPS331AP defines by name

[Lee Jones]

They're currently named *_1_*, for 'Sensor 1', but the code will be much
more readable if we use the naming convention *_LPS331AP_* instead.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/iio/pressure/st_pressure_core.c | 90 ++++++++++++++++-----------------
 1 file changed, 44 insertions(+), 46 deletions(-)

diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
index 9c343b4..becfb25 100644
--- a/drivers/iio/pressure/st_pressure_core.c
+++ b/drivers/iio/pressure/st_pressure_core.c
@@ -31,92 +31,90 @@
 #define ST_PRESS_MBAR_TO_KPASCAL(x)		(x * 10)
 #define ST_PRESS_NUMBER_DATA_CHANNELS		1
 
-/* DEFAULT VALUE FOR SENSORS */
-#define ST_PRESS_DEFAULT_OUT_XL_ADDR		0x28
-#define ST_TEMP_DEFAULT_OUT_L_ADDR		0x2b
-
 /* FULLSCALE */
 #define ST_PRESS_FS_AVL_1260MB			1260
 
-/* CUSTOM VALUES FOR SENSOR 1 */
-#define ST_PRESS_1_WAI_EXP			0xbb
-#define ST_PRESS_1_ODR_ADDR			0x20
-#define ST_PRESS_1_ODR_MASK			0x70
-#define ST_PRESS_1_ODR_AVL_1HZ_VAL		0x01
-#define ST_PRESS_1_ODR_AVL_7HZ_VAL		0x05
-#define ST_PRESS_1_ODR_AVL_13HZ_VAL		0x06
-#define ST_PRESS_1_ODR_AVL_25HZ_VAL		0x07
-#define ST_PRESS_1_PW_ADDR			0x20
-#define ST_PRESS_1_PW_MASK			0x80
-#define ST_PRESS_1_FS_ADDR			0x23
-#define ST_PRESS_1_FS_MASK			0x30
-#define ST_PRESS_1_FS_AVL_1260_VAL		0x00
-#define ST_PRESS_1_FS_AVL_1260_GAIN		ST_PRESS_MBAR_TO_KPASCAL(244141)
-#define ST_PRESS_1_FS_AVL_TEMP_GAIN		2083000
-#define ST_PRESS_1_BDU_ADDR			0x20
-#define ST_PRESS_1_BDU_MASK			0x04
-#define ST_PRESS_1_DRDY_IRQ_ADDR		0x22
-#define ST_PRESS_1_DRDY_IRQ_MASK		0x04
-#define ST_PRESS_1_MULTIREAD_BIT		true
-#define ST_PRESS_1_TEMP_OFFSET			42500
+/* CUSTOM VALUES FOR LPS331AP SENSOR */
+#define ST_PRESS_LPS331AP_WAI_EXP		0xbb
+#define ST_PRESS_LPS331AP_ODR_ADDR		0x20
+#define ST_PRESS_LPS331AP_ODR_MASK		0x70
+#define ST_PRESS_LPS331AP_ODR_AVL_1HZ_VAL	0x01
+#define ST_PRESS_LPS331AP_ODR_AVL_7HZ_VAL	0x05
+#define ST_PRESS_LPS331AP_ODR_AVL_13HZ_VAL	0x06
+#define ST_PRESS_LPS331AP_ODR_AVL_25HZ_VAL	0x07
+#define ST_PRESS_LPS331AP_PW_ADDR		0x20
+#define ST_PRESS_LPS331AP_PW_MASK		0x80
+#define ST_PRESS_LPS331AP_FS_ADDR		0x23
+#define ST_PRESS_LPS331AP_FS_MASK		0x30
+#define ST_PRESS_LPS331AP_FS_AVL_1260_VAL	0x00
+#define ST_PRESS_LPS331AP_FS_AVL_1260_GAIN	ST_PRESS_MBAR_TO_KPASCAL(244141)
+#define ST_PRESS_LPS331AP_FS_AVL_TEMP_GAIN	2083000
+#define ST_PRESS_LPS331AP_BDU_ADDR		0x20
+#define ST_PRESS_LPS331AP_BDU_MASK		0x04
+#define ST_PRESS_LPS331AP_DRDY_IRQ_ADDR		0x22
+#define ST_PRESS_LPS331AP_DRDY_IRQ_MASK		0x04
+#define ST_PRESS_LPS331AP_MULTIREAD_BIT		true
+#define ST_PRESS_LPS331AP_TEMP_OFFSET		42500
+#define ST_PRESS_LPS331AP_OUT_XL_ADDR		0x28
+#define ST_TEMP_LPS331AP_OUT_L_ADDR		0x2b
 
 static const struct iio_chan_spec st_press_channels[] = {
 	ST_SENSORS_LSM_CHANNELS(IIO_PRESSURE,
 			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
 			ST_SENSORS_SCAN_X, 0, IIO_NO_MOD, 'u', IIO_LE, 24, 24,
-			ST_PRESS_DEFAULT_OUT_XL_ADDR),
+			ST_PRESS_LPS331AP_OUT_XL_ADDR),
 	ST_SENSORS_LSM_CHANNELS(IIO_TEMP,
 			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE) |
 						BIT(IIO_CHAN_INFO_OFFSET),
 			-1, 0, IIO_NO_MOD, 's', IIO_LE, 16, 16,
-			ST_TEMP_DEFAULT_OUT_L_ADDR),
+			ST_TEMP_LPS331AP_OUT_L_ADDR),
 	IIO_CHAN_SOFT_TIMESTAMP(1)
 };
 
 static const struct st_sensors st_press_sensors[] = {
 	{
-		.wai = ST_PRESS_1_WAI_EXP,
+		.wai = ST_PRESS_LPS331AP_WAI_EXP,
 		.sensors_supported = {
 			[0] = LPS331AP_PRESS_DEV_NAME,
 		},
 		.ch = (struct iio_chan_spec *)st_press_channels,
 		.odr = {
-			.addr = ST_PRESS_1_ODR_ADDR,
-			.mask = ST_PRESS_1_ODR_MASK,
+			.addr = ST_PRESS_LPS331AP_ODR_ADDR,
+			.mask = ST_PRESS_LPS331AP_ODR_MASK,
 			.odr_avl = {
-				{ 1, ST_PRESS_1_ODR_AVL_1HZ_VAL, },
-				{ 7, ST_PRESS_1_ODR_AVL_7HZ_VAL, },
-				{ 13, ST_PRESS_1_ODR_AVL_13HZ_VAL, },
-				{ 25, ST_PRESS_1_ODR_AVL_25HZ_VAL, },
+				{ 1, ST_PRESS_LPS331AP_ODR_AVL_1HZ_VAL, },
+				{ 7, ST_PRESS_LPS331AP_ODR_AVL_7HZ_VAL, },
+				{ 13, ST_PRESS_LPS331AP_ODR_AVL_13HZ_VAL, },
+				{ 25, ST_PRESS_LPS331AP_ODR_AVL_25HZ_VAL, },
 			},
 		},
 		.pw = {
-			.addr = ST_PRESS_1_PW_ADDR,
-			.mask = ST_PRESS_1_PW_MASK,
+			.addr = ST_PRESS_LPS331AP_PW_ADDR,
+			.mask = ST_PRESS_LPS331AP_PW_MASK,
 			.value_on = ST_SENSORS_DEFAULT_POWER_ON_VALUE,
 			.value_off = ST_SENSORS_DEFAULT_POWER_OFF_VALUE,
 		},
 		.fs = {
-			.addr = ST_PRESS_1_FS_ADDR,
-			.mask = ST_PRESS_1_FS_MASK,
+			.addr = ST_PRESS_LPS331AP_FS_ADDR,
+			.mask = ST_PRESS_LPS331AP_FS_MASK,
 			.fs_avl = {
 				[0] = {
 					.num = ST_PRESS_FS_AVL_1260MB,
-					.value = ST_PRESS_1_FS_AVL_1260_VAL,
-					.gain = ST_PRESS_1_FS_AVL_1260_GAIN,
-					.gain2 = ST_PRESS_1_FS_AVL_TEMP_GAIN,
+					.value = ST_PRESS_LPS331AP_FS_AVL_1260_VAL,
+					.gain = ST_PRESS_LPS331AP_FS_AVL_1260_GAIN,
+					.gain2 = ST_PRESS_LPS331AP_FS_AVL_TEMP_GAIN,
 				},
 			},
 		},
 		.bdu = {
-			.addr = ST_PRESS_1_BDU_ADDR,
-			.mask = ST_PRESS_1_BDU_MASK,
+			.addr = ST_PRESS_LPS331AP_BDU_ADDR,
+			.mask = ST_PRESS_LPS331AP_BDU_MASK,
 		},
 		.drdy_irq = {
-			.addr = ST_PRESS_1_DRDY_IRQ_ADDR,
-			.mask = ST_PRESS_1_DRDY_IRQ_MASK,
+			.addr = ST_PRESS_LPS331AP_DRDY_IRQ_ADDR,
+			.mask = ST_PRESS_LPS331AP_DRDY_IRQ_MASK,
 		},
-		.multi_read_bit = ST_PRESS_1_MULTIREAD_BIT,
+		.multi_read_bit = ST_PRESS_LPS331AP_MULTIREAD_BIT,
 		.bootime = 2,
 	},
 };
-- 
1.8.1.2

[PATCH 06/11] iio: pressure-core: st: Expand and rename LPS331AP's channel descriptor

[Lee Jones]

Due to the MACRO used, the task of reading, understanding and maintaining
the LPS331AP's channel descriptor is substantially difficult. This patch
is based on the view that it's better to have easy to read, maintainable
code than to save a few lines here and there. For that reason we're
expanding the array so initialisation is completed in full.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/iio/pressure/st_pressure_core.c | 45 +++++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
index becfb25..7ba9299 100644
--- a/drivers/iio/pressure/st_pressure_core.c
+++ b/drivers/iio/pressure/st_pressure_core.c
@@ -58,16 +58,39 @@
 #define ST_PRESS_LPS331AP_OUT_XL_ADDR		0x28
 #define ST_TEMP_LPS331AP_OUT_L_ADDR		0x2b
 
-static const struct iio_chan_spec st_press_channels[] = {
-	ST_SENSORS_LSM_CHANNELS(IIO_PRESSURE,
+static const struct iio_chan_spec st_press_lsp331ap_channels[] = {
+	{
+		.type = IIO_PRESSURE,
+		.channel2 = IIO_NO_MOD,
+		.address = ST_PRESS_LPS331AP_OUT_XL_ADDR,
+		.scan_index = ST_SENSORS_SCAN_X,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 24,
+			.storagebits = 24,
+			.endianness = IIO_LE,
+		},
+		.info_mask_separate =
 			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
-			ST_SENSORS_SCAN_X, 0, IIO_NO_MOD, 'u', IIO_LE, 24, 24,
-			ST_PRESS_LPS331AP_OUT_XL_ADDR),
-	ST_SENSORS_LSM_CHANNELS(IIO_TEMP,
-			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE) |
-						BIT(IIO_CHAN_INFO_OFFSET),
-			-1, 0, IIO_NO_MOD, 's', IIO_LE, 16, 16,
-			ST_TEMP_LPS331AP_OUT_L_ADDR),
+		.modified = 0,
+	},
+	{
+		.type = IIO_TEMP,
+		.channel2 = IIO_NO_MOD,
+		.address = ST_TEMP_LPS331AP_OUT_L_ADDR,
+		.scan_index = -1,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_LE,
+		},
+		.info_mask_separate =
+			BIT(IIO_CHAN_INFO_RAW) |
+			BIT(IIO_CHAN_INFO_SCALE) |
+			BIT(IIO_CHAN_INFO_OFFSET),
+		.modified = 0,
+	},
 	IIO_CHAN_SOFT_TIMESTAMP(1)
 };
 
@@ -77,7 +100,7 @@ static const struct st_sensors st_press_sensors[] = {
 		.sensors_supported = {
 			[0] = LPS331AP_PRESS_DEV_NAME,
 		},
-		.ch = (struct iio_chan_spec *)st_press_channels,
+		.ch = (struct iio_chan_spec *)st_press_lsp331ap_channels,
 		.odr = {
 			.addr = ST_PRESS_LPS331AP_ODR_ADDR,
 			.mask = ST_PRESS_LPS331AP_ODR_MASK,
@@ -214,7 +237,7 @@ int st_press_common_probe(struct iio_dev *indio_dev)
 	pdata->num_data_channels = ST_PRESS_NUMBER_DATA_CHANNELS;
 	pdata->multiread_bit = pdata->sensor->multi_read_bit;
 	indio_dev->channels = pdata->sensor->ch;
-	indio_dev->num_channels = ARRAY_SIZE(st_press_channels);
+	indio_dev->num_channels = ARRAY_SIZE(st_press_lsp331ap_channels);
 
 	pdata->current_fullscale = (struct st_sensor_fullscale_avl *)
 						&pdata->sensor->fs.fs_avl[0];
-- 
1.8.1.2

[PATCH 07/11] iio: sensors-core: st: Allow full-scale to be an optional feature

[Lee Jones]

Some chips either don't support it or fail to provide adequate documentation,
so sometimes it's impossible to enable the feature even if it is supported.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/iio/common/st_sensors/st_sensors_core.c | 11 +++++++----
 drivers/iio/pressure/st_pressure_core.c         |  6 ++++--
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
index 865b178..fc9acb7 100644
--- a/drivers/iio/common/st_sensors/st_sensors_core.c
+++ b/drivers/iio/common/st_sensors/st_sensors_core.c
@@ -209,10 +209,13 @@ int st_sensors_init_sensor(struct iio_dev *indio_dev)
 	if (err < 0)
 		goto init_error;
 
-	err = st_sensors_set_fullscale(indio_dev,
-						sdata->current_fullscale->num);
-	if (err < 0)
-		goto init_error;
+	if (sdata->current_fullscale) {
+		err = st_sensors_set_fullscale(indio_dev,
+					       sdata->current_fullscale->num);
+		if (err < 0)
+			goto init_error;
+	} else
+		dev_info(&indio_dev->dev, "Full-scale not possible\n");
 
 	err = st_sensors_set_odr(indio_dev, sdata->odr);
 	if (err < 0)
diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
index 7ba9299..423ed6a 100644
--- a/drivers/iio/pressure/st_pressure_core.c
+++ b/drivers/iio/pressure/st_pressure_core.c
@@ -239,8 +239,10 @@ int st_press_common_probe(struct iio_dev *indio_dev)
 	indio_dev->channels = pdata->sensor->ch;
 	indio_dev->num_channels = ARRAY_SIZE(st_press_lsp331ap_channels);
 
-	pdata->current_fullscale = (struct st_sensor_fullscale_avl *)
-						&pdata->sensor->fs.fs_avl[0];
+	if (pdata->sensor->fs.addr != 0)
+		pdata->current_fullscale = (struct st_sensor_fullscale_avl *)
+			&pdata->sensor->fs.fs_avl[0];
+
 	pdata->odr = pdata->sensor->odr.odr_avl[0].hz;
 
 	err = st_sensors_init_sensor(indio_dev);
-- 
1.8.1.2

[PATCH 08/11] iio: pressure-core: st: Allow for number of channels to vary

[Lee Jones]

At the moment the number of channels specified is dictated by the first
sensor supported by the driver. As we add support for more sensors this
is likely to vary. Instead of using the ARRAY_SIZE() of the LPS331AP's
channel specifier we'll use a new adaptable 'struct st_sensors' element
instead.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/iio/pressure/st_pressure_core.c | 3 ++-
 include/linux/iio/common/st_sensors.h   | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
index 423ed6a..3cf54ed 100644
--- a/drivers/iio/pressure/st_pressure_core.c
+++ b/drivers/iio/pressure/st_pressure_core.c
@@ -101,6 +101,7 @@ static const struct st_sensors st_press_sensors[] = {
 			[0] = LPS331AP_PRESS_DEV_NAME,
 		},
 		.ch = (struct iio_chan_spec *)st_press_lsp331ap_channels,
+		.num_ch = ARRAY_SIZE(st_press_lsp331ap_channels),
 		.odr = {
 			.addr = ST_PRESS_LPS331AP_ODR_ADDR,
 			.mask = ST_PRESS_LPS331AP_ODR_MASK,
@@ -237,7 +238,7 @@ int st_press_common_probe(struct iio_dev *indio_dev)
 	pdata->num_data_channels = ST_PRESS_NUMBER_DATA_CHANNELS;
 	pdata->multiread_bit = pdata->sensor->multi_read_bit;
 	indio_dev->channels = pdata->sensor->ch;
-	indio_dev->num_channels = ARRAY_SIZE(st_press_lsp331ap_channels);
+	indio_dev->num_channels = pdata->sensor->num_ch;
 
 	if (pdata->sensor->fs.addr != 0)
 		pdata->current_fullscale = (struct st_sensor_fullscale_avl *)
diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
index 72b2694..4aef925 100644
--- a/include/linux/iio/common/st_sensors.h
+++ b/include/linux/iio/common/st_sensors.h
@@ -180,6 +180,7 @@ struct st_sensors {
 	u8 wai;
 	char sensors_supported[ST_SENSORS_MAX_4WAI][ST_SENSORS_MAX_NAME];
 	struct iio_chan_spec *ch;
+	int num_ch;
 	struct st_sensor_odr odr;
 	struct st_sensor_power pw;
 	struct st_sensor_axis enable_axis;
-- 
1.8.1.2

[PATCH 09/11] iio: pressure-core: st: Clean-up error handling in probe function

[Lee Jones]

This patch contains some pretty basic clean-ups in probe() pertaining to
the simplification of error handling and a couple of readability adaptions.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/iio/pressure/st_pressure_core.c | 41 +++++++++++++++------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
index 3cf54ed..2893d08 100644
--- a/drivers/iio/pressure/st_pressure_core.c
+++ b/drivers/iio/pressure/st_pressure_core.c
@@ -224,21 +224,23 @@ static const struct iio_trigger_ops st_press_trigger_ops = {
 
 int st_press_common_probe(struct iio_dev *indio_dev)
 {
-	int err;
 	struct st_sensor_data *pdata = iio_priv(indio_dev);
+	int irq = pdata->get_irq_data_ready(indio_dev);
+	int err;
 
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &press_info;
 
 	err = st_sensors_check_device_support(indio_dev,
-				ARRAY_SIZE(st_press_sensors), st_press_sensors);
+					      ARRAY_SIZE(st_press_sensors),
+					      st_press_sensors);
 	if (err < 0)
-		goto st_press_common_probe_error;
+		return err;
 
 	pdata->num_data_channels = ST_PRESS_NUMBER_DATA_CHANNELS;
-	pdata->multiread_bit = pdata->sensor->multi_read_bit;
-	indio_dev->channels = pdata->sensor->ch;
-	indio_dev->num_channels = pdata->sensor->num_ch;
+	pdata->multiread_bit     = pdata->sensor->multi_read_bit;
+	indio_dev->channels      = pdata->sensor->ch;
+	indio_dev->num_channels  = pdata->sensor->num_ch;
 
 	if (pdata->sensor->fs.addr != 0)
 		pdata->current_fullscale = (struct st_sensor_fullscale_avl *)
@@ -248,32 +250,27 @@ int st_press_common_probe(struct iio_dev *indio_dev)
 
 	err = st_sensors_init_sensor(indio_dev);
 	if (err < 0)
-		goto st_press_common_probe_error;
+		return err;
 
-	if (pdata->get_irq_data_ready(indio_dev) > 0) {
+	if (irq > 0) {
 		err = st_press_allocate_ring(indio_dev);
 		if (err < 0)
-			goto st_press_common_probe_error;
+			return err;
 
 		err = st_sensors_allocate_trigger(indio_dev,
-							ST_PRESS_TRIGGER_OPS);
-		if (err < 0)
-			goto st_press_probe_trigger_error;
+						  ST_PRESS_TRIGGER_OPS);
+		if (err < 0) {
+			st_press_deallocate_ring(indio_dev);
+			return err;
+		}
 	}
 
 	err = iio_device_register(indio_dev);
-	if (err)
-		goto st_press_device_register_error;
-
-	return err;
-
-st_press_device_register_error:
-	if (pdata->get_irq_data_ready(indio_dev) > 0)
+	if (err && irq > 0) {
 		st_sensors_deallocate_trigger(indio_dev);
-st_press_probe_trigger_error:
-	if (pdata->get_irq_data_ready(indio_dev) > 0)
 		st_press_deallocate_ring(indio_dev);
-st_press_common_probe_error:
+	}
+
 	return err;
 }
 EXPORT_SYMBOL(st_press_common_probe);
-- 
1.8.1.2

[PATCH 10/11] iio: pressure: st: Add support for new LPS001WP pressure sensor

[Lee Jones]

Here we use existing practices to introduce support for another
pressure/temperature sensor, the LPS001WP.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/iio/pressure/st_pressure.h      |  1 +
 drivers/iio/pressure/st_pressure_core.c | 84 +++++++++++++++++++++++++++++++++
 drivers/iio/pressure/st_pressure_i2c.c  |  1 +
 3 files changed, 86 insertions(+)

diff --git a/drivers/iio/pressure/st_pressure.h b/drivers/iio/pressure/st_pressure.h
index 414e45a..5b42aa2 100644
--- a/drivers/iio/pressure/st_pressure.h
+++ b/drivers/iio/pressure/st_pressure.h
@@ -15,6 +15,7 @@
 #include <linux/iio/common/st_sensors.h>
 
 #define LPS331AP_PRESS_DEV_NAME		"lps331ap"
+#define LPS001WP_PRESS_DEV_NAME		"lps001wp"
 
 int st_press_common_probe(struct iio_dev *indio_dev);
 void st_press_common_remove(struct iio_dev *indio_dev);
diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
index 2893d08..f452417 100644
--- a/drivers/iio/pressure/st_pressure_core.c
+++ b/drivers/iio/pressure/st_pressure_core.c
@@ -58,6 +58,21 @@
 #define ST_PRESS_LPS331AP_OUT_XL_ADDR		0x28
 #define ST_TEMP_LPS331AP_OUT_L_ADDR		0x2b
 
+/* CUSTOM VALUES FOR LPS001WP SENSOR */
+#define ST_PRESS_LPS001WP_WAI_EXP		0xba
+#define ST_PRESS_LPS001WP_ODR_ADDR		0x20
+#define ST_PRESS_LPS001WP_ODR_MASK		0x30
+#define ST_PRESS_LPS001WP_ODR_AVL_1HZ_VAL	0x01
+#define ST_PRESS_LPS001WP_ODR_AVL_7HZ_VAL	0x02
+#define ST_PRESS_LPS001WP_ODR_AVL_13HZ_VAL	0x03
+#define ST_PRESS_LPS001WP_PW_ADDR		0x20
+#define ST_PRESS_LPS001WP_PW_MASK		0x40
+#define ST_PRESS_LPS001WP_BDU_ADDR		0x20
+#define ST_PRESS_LPS001WP_BDU_MASK		0x04
+#define ST_PRESS_LPS001WP_MULTIREAD_BIT		true
+#define ST_PRESS_LPS001WP_OUT_L_ADDR		0x28
+#define ST_TEMP_LPS001WP_OUT_L_ADDR		0x2a
+
 static const struct iio_chan_spec st_press_lsp331ap_channels[] = {
 	{
 		.type = IIO_PRESSURE,
@@ -94,6 +109,40 @@ static const struct iio_chan_spec st_press_lsp331ap_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(1)
 };
 
+static const struct iio_chan_spec st_press_lps001wp_channels[] = {
+	{
+		.type = IIO_PRESSURE,
+		.channel2 = IIO_NO_MOD,
+		.address = ST_PRESS_LPS001WP_OUT_L_ADDR,
+		.scan_index = ST_SENSORS_SCAN_X,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_LE,
+		},
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.modified = 0,
+	},
+	{
+		.type = IIO_TEMP,
+		.channel2 = IIO_NO_MOD,
+		.address = ST_TEMP_LPS001WP_OUT_L_ADDR,
+		.scan_index = -1,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_LE,
+		},
+		.info_mask_separate =
+			BIT(IIO_CHAN_INFO_RAW) |
+			BIT(IIO_CHAN_INFO_OFFSET),
+		.modified = 0,
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(1)
+};
+
 static const struct st_sensors st_press_sensors[] = {
 	{
 		.wai = ST_PRESS_LPS331AP_WAI_EXP,
@@ -141,6 +190,41 @@ static const struct st_sensors st_press_sensors[] = {
 		.multi_read_bit = ST_PRESS_LPS331AP_MULTIREAD_BIT,
 		.bootime = 2,
 	},
+	{
+		.wai = ST_PRESS_LPS001WP_WAI_EXP,
+		.sensors_supported = {
+			[0] = LPS001WP_PRESS_DEV_NAME,
+		},
+		.ch = (struct iio_chan_spec *)st_press_lps001wp_channels,
+		.num_ch = ARRAY_SIZE(st_press_lps001wp_channels),
+		.odr = {
+			.addr = ST_PRESS_LPS001WP_ODR_ADDR,
+			.mask = ST_PRESS_LPS001WP_ODR_MASK,
+			.odr_avl = {
+				{ 1, ST_PRESS_LPS001WP_ODR_AVL_1HZ_VAL, },
+				{ 7, ST_PRESS_LPS001WP_ODR_AVL_7HZ_VAL, },
+				{ 13, ST_PRESS_LPS001WP_ODR_AVL_13HZ_VAL, },
+			},
+		},
+		.pw = {
+			.addr = ST_PRESS_LPS001WP_PW_ADDR,
+			.mask = ST_PRESS_LPS001WP_PW_MASK,
+			.value_on = ST_SENSORS_DEFAULT_POWER_ON_VALUE,
+			.value_off = ST_SENSORS_DEFAULT_POWER_OFF_VALUE,
+		},
+		.fs = {
+			.addr = 0,
+		},
+		.bdu = {
+			.addr = ST_PRESS_LPS001WP_BDU_ADDR,
+			.mask = ST_PRESS_LPS001WP_BDU_MASK,
+		},
+		.drdy_irq = {
+			.addr = 0,
+		},
+		.multi_read_bit = ST_PRESS_LPS001WP_MULTIREAD_BIT,
+		.bootime = 2,
+	},
 };
 
 static int st_press_read_raw(struct iio_dev *indio_dev,
diff --git a/drivers/iio/pressure/st_pressure_i2c.c b/drivers/iio/pressure/st_pressure_i2c.c
index 2ace770..c5c96ac 100644
--- a/drivers/iio/pressure/st_pressure_i2c.c
+++ b/drivers/iio/pressure/st_pressure_i2c.c
@@ -52,6 +52,7 @@ static int st_press_i2c_remove(struct i2c_client *client)
 
 static const struct i2c_device_id st_press_id_table[] = {
 	{ LPS331AP_PRESS_DEV_NAME },
+	{ LPS001WP_PRESS_DEV_NAME },
 	{},
 };
 MODULE_DEVICE_TABLE(i2c, st_press_id_table);
-- 
1.8.1.2

[PATCH 11/11] iio: pressure-core: st: Provide correct regulator support

[Lee Jones]

The power to some of the sensors are controlled by regulators. In most
cases these are 'always on', but if not they will fail to work until
the regulator is enabled using the relevant APIs.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/iio/pressure/st_pressure_core.c | 13 +++++++++++++
 include/linux/iio/common/st_sensors.h   |  3 +++
 2 files changed, 16 insertions(+)

diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
index f452417..7beed89 100644
--- a/drivers/iio/pressure/st_pressure_core.c
+++ b/drivers/iio/pressure/st_pressure_core.c
@@ -23,6 +23,7 @@
 #include <linux/iio/sysfs.h>
 #include <linux/iio/trigger.h>
 #include <linux/iio/buffer.h>
+#include <linux/regulator/consumer.h>
 #include <asm/unaligned.h>
 
 #include <linux/iio/common/st_sensors.h>
@@ -315,6 +316,15 @@ int st_press_common_probe(struct iio_dev *indio_dev)
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &press_info;
 
+	/* Regulator not mandatory, but if requested we should enable it. */
+	pdata->regulator = regulator_get(&indio_dev->dev, "vdd");
+	if (!IS_ERR_OR_NULL(pdata->regulator)) {
+		err = regulator_enable(pdata->regulator);
+		if (err != 0)
+			dev_warn(&indio_dev->dev,
+				 "Failed to enable specified vdd regulator\n");
+	}
+
 	err = st_sensors_check_device_support(indio_dev,
 					      ARRAY_SIZE(st_press_sensors),
 					      st_press_sensors);
@@ -363,6 +373,9 @@ void st_press_common_remove(struct iio_dev *indio_dev)
 {
 	struct st_sensor_data *pdata = iio_priv(indio_dev);
 
+	if (!IS_ERR_OR_NULL(pdata->regulator))
+		regulator_disable(pdata->regulator);
+
 	iio_device_unregister(indio_dev);
 	if (pdata->get_irq_data_ready(indio_dev) > 0) {
 		st_sensors_deallocate_trigger(indio_dev);
diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
index 4aef925..eb6ef3f 100644
--- a/include/linux/iio/common/st_sensors.h
+++ b/include/linux/iio/common/st_sensors.h
@@ -16,6 +16,7 @@
 #include <linux/irqreturn.h>
 #include <linux/iio/trigger.h>
 #include <linux/bitops.h>
+#include <linux/regulator/consumer.h>
 
 #define ST_SENSORS_TX_MAX_LENGTH		2
 #define ST_SENSORS_RX_MAX_LENGTH		6
@@ -197,6 +198,7 @@ struct st_sensors {
  * @trig: The trigger in use by the core driver.
  * @sensor: Pointer to the current sensor struct in use.
  * @current_fullscale: Maximum range of measure by the sensor.
+ * @regulator: Pointer to sensor's power supply
  * @enabled: Status of the sensor (false->off, true->on).
  * @multiread_bit: Use or not particular bit for [I2C/SPI] multiread.
  * @buffer_data: Data used by buffer part.
@@ -211,6 +213,7 @@ struct st_sensor_data {
 	struct iio_trigger *trig;
 	struct st_sensors *sensor;
 	struct st_sensor_fullscale_avl *current_fullscale;
+	struct regulator *regulator;
 
 	bool enabled;
 	bool multiread_bit;
-- 
1.8.1.2

Re: [PATCH 02/11] ARM: ux500: Enable the LPS001WP Pressure & Temperature sensor from DT

[Mark Rutland]

Hi Lee,

On Wed, Sep 04, 2013 at 10:31:34AM +0100, Lee Jones wrote:
> After applying this node the LPS001WP sensor chip should probe
> successfully once the driver support has also been applied.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  arch/arm/boot/dts/ste-snowball.dts | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/ste-snowball.dts b/arch/arm/boot/dts/ste-snowball.dts
> index cf9b16e..aad8f54 100644
> --- a/arch/arm/boot/dts/ste-snowball.dts
> +++ b/arch/arm/boot/dts/ste-snowball.dts
> @@ -153,6 +153,16 @@
>  			status = "okay";
>  		};
>  
> +		i2c@80128000 {
> +			lps001wp@5c {
> +				compatible = "lps001wp";
> +				reg = <0x5c>;
> +
> +				vdd-supply = <&ab8500_ldo_aux1_reg>;
> +				vms-supply = <&db8500_vsmps2_reg>;
> +			};
> +		};
> +

This appears to be missing a binding document. I couldn't see one
anywhere in this series, or in mainline already).

As far as I can see, the compatible string should be "st,lps001wp".

Please produce a binding document.

Is there any publicly available documentation on the chip?

Thanks,
Mark.

>  		uart@80120000 {
>  			status = "okay";
>  		};
> -- 
> 1.8.1.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH 11/11] iio: pressure-core: st: Provide correct regulator support

[Mark Rutland]

Hi Lee,

On Wed, Sep 04, 2013 at 10:31:43AM +0100, Lee Jones wrote:
> The power to some of the sensors are controlled by regulators. In most
> cases these are 'always on', but if not they will fail to work until
> the regulator is enabled using the relevant APIs.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/iio/pressure/st_pressure_core.c | 13 +++++++++++++
>  include/linux/iio/common/st_sensors.h   |  3 +++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
> index f452417..7beed89 100644
> --- a/drivers/iio/pressure/st_pressure_core.c
> +++ b/drivers/iio/pressure/st_pressure_core.c
> @@ -23,6 +23,7 @@
>  #include <linux/iio/sysfs.h>
>  #include <linux/iio/trigger.h>
>  #include <linux/iio/buffer.h>
> +#include <linux/regulator/consumer.h>
>  #include <asm/unaligned.h>
>  
>  #include <linux/iio/common/st_sensors.h>
> @@ -315,6 +316,15 @@ int st_press_common_probe(struct iio_dev *indio_dev)
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->info = &press_info;
>  
> +	/* Regulator not mandatory, but if requested we should enable it. */
> +	pdata->regulator = regulator_get(&indio_dev->dev, "vdd");
> +	if (!IS_ERR_OR_NULL(pdata->regulator)) {

Can regulator_get return NULL? As far as I can see, it either returns a
valid reulator pointer or an ERR_PTR value.

When you say "if requested", do you mean "if described in the dt"? If
so, the above doesn't distunguish between a regulator not being listed
and one failing to be got (e.g. if we got EPROBE_DEFER from
regulator_get).

I think this would be better handled with something like Mark Brown's
suggested regulator_get_optional [1,2].

Thanks,
Mark.

[1] https://lkml.org/lkml/2013/7/30/328 (cover)
[2] https://lkml.org/lkml/2013/7/30/334 (patches)

> +		err = regulator_enable(pdata->regulator);
> +		if (err != 0)
> +			dev_warn(&indio_dev->dev,
> +				 "Failed to enable specified vdd regulator\n");
> +	}
> +
>  	err = st_sensors_check_device_support(indio_dev,
>  					      ARRAY_SIZE(st_press_sensors),
>  					      st_press_sensors);
> @@ -363,6 +373,9 @@ void st_press_common_remove(struct iio_dev *indio_dev)
>  {
>  	struct st_sensor_data *pdata = iio_priv(indio_dev);
>  
> +	if (!IS_ERR_OR_NULL(pdata->regulator))
> +		regulator_disable(pdata->regulator);
> +
>  	iio_device_unregister(indio_dev);
>  	if (pdata->get_irq_data_ready(indio_dev) > 0) {
>  		st_sensors_deallocate_trigger(indio_dev);
> diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
> index 4aef925..eb6ef3f 100644
> --- a/include/linux/iio/common/st_sensors.h
> +++ b/include/linux/iio/common/st_sensors.h
> @@ -16,6 +16,7 @@
>  #include <linux/irqreturn.h>
>  #include <linux/iio/trigger.h>
>  #include <linux/bitops.h>
> +#include <linux/regulator/consumer.h>
>  
>  #define ST_SENSORS_TX_MAX_LENGTH		2
>  #define ST_SENSORS_RX_MAX_LENGTH		6
> @@ -197,6 +198,7 @@ struct st_sensors {
>   * @trig: The trigger in use by the core driver.
>   * @sensor: Pointer to the current sensor struct in use.
>   * @current_fullscale: Maximum range of measure by the sensor.
> + * @regulator: Pointer to sensor's power supply
>   * @enabled: Status of the sensor (false->off, true->on).
>   * @multiread_bit: Use or not particular bit for [I2C/SPI] multiread.
>   * @buffer_data: Data used by buffer part.
> @@ -211,6 +213,7 @@ struct st_sensor_data {
>  	struct iio_trigger *trig;
>  	struct st_sensors *sensor;
>  	struct st_sensor_fullscale_avl *current_fullscale;
> +	struct regulator *regulator;
>  
>  	bool enabled;
>  	bool multiread_bit;
> -- 
> 1.8.1.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH 11/11] iio: pressure-core: st: Provide correct regulator support

[Lars-Peter Clausen]

On 09/04/2013 03:11 PM, Mark Rutland wrote:
> Hi Lee,
> 
> On Wed, Sep 04, 2013 at 10:31:43AM +0100, Lee Jones wrote:
>> The power to some of the sensors are controlled by regulators. In most
>> cases these are 'always on', but if not they will fail to work until
>> the regulator is enabled using the relevant APIs.
>>
>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>> ---
>>  drivers/iio/pressure/st_pressure_core.c | 13 +++++++++++++
>>  include/linux/iio/common/st_sensors.h   |  3 +++
>>  2 files changed, 16 insertions(+)
>>
>> diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
>> index f452417..7beed89 100644
>> --- a/drivers/iio/pressure/st_pressure_core.c
>> +++ b/drivers/iio/pressure/st_pressure_core.c
>> @@ -23,6 +23,7 @@
>>  #include <linux/iio/sysfs.h>
>>  #include <linux/iio/trigger.h>
>>  #include <linux/iio/buffer.h>
>> +#include <linux/regulator/consumer.h>
>>  #include <asm/unaligned.h>
>>  
>>  #include <linux/iio/common/st_sensors.h>
>> @@ -315,6 +316,15 @@ int st_press_common_probe(struct iio_dev *indio_dev)
>>  	indio_dev->modes = INDIO_DIRECT_MODE;
>>  	indio_dev->info = &press_info;
>>  
>> +	/* Regulator not mandatory, but if requested we should enable it. */
>> +	pdata->regulator = regulator_get(&indio_dev->dev, "vdd");
>> +	if (!IS_ERR_OR_NULL(pdata->regulator)) {
> 
> Can regulator_get return NULL? As far as I can see, it either returns a
> valid reulator pointer or an ERR_PTR value.
> 
> When you say "if requested", do you mean "if described in the dt"? If
> so, the above doesn't distunguish between a regulator not being listed
> and one failing to be got (e.g. if we got EPROBE_DEFER from
> regulator_get).
> 
> I think this would be better handled with something like Mark Brown's
> suggested regulator_get_optional [1,2].

It can return NULL, but NULL is actually a valid regulator in that case, so
the check should only be IS_ERR. And yes regulator_get_optional is what
should be used here.

- Lars

Re: [PATCH 11/11] iio: pressure-core: st: Provide correct regulator support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1090 bytes --]

On Wed, Sep 04, 2013 at 02:11:11PM +0100, Mark Rutland wrote:
> On Wed, Sep 04, 2013 at 10:31:43AM +0100, Lee Jones wrote:

> > +	/* Regulator not mandatory, but if requested we should enable it. */
> > +	pdata->regulator = regulator_get(&indio_dev->dev, "vdd");
> > +	if (!IS_ERR_OR_NULL(pdata->regulator)) {

> Can regulator_get return NULL? As far as I can see, it either returns a
> valid reulator pointer or an ERR_PTR value.

Yes, NULL is a valid regulator.

> When you say "if requested", do you mean "if described in the dt"? If
> so, the above doesn't distunguish between a regulator not being listed
> and one failing to be got (e.g. if we got EPROBE_DEFER from
> regulator_get).

> I think this would be better handled with something like Mark Brown's
> suggested regulator_get_optional [1,2].

If the regulator may genuinely be absent from the system then it should
be being requested using regulator_get_optional().  Otherwise it should
be being requested using regulator_get().  In both cases it is important 
that the driver pays attention to errors.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 11/11] iio: pressure-core: st: Provide correct regulator support

[Lee Jones]

On Wed, 04 Sep 2013, Lars-Peter Clausen wrote:

> On 09/04/2013 03:11 PM, Mark Rutland wrote:
> > Hi Lee,
> > 
> > On Wed, Sep 04, 2013 at 10:31:43AM +0100, Lee Jones wrote:
> >> The power to some of the sensors are controlled by regulators. In most
> >> cases these are 'always on', but if not they will fail to work until
> >> the regulator is enabled using the relevant APIs.
> >>
> >> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> >> ---
> >>  drivers/iio/pressure/st_pressure_core.c | 13 +++++++++++++
> >>  include/linux/iio/common/st_sensors.h   |  3 +++
> >>  2 files changed, 16 insertions(+)
> >>
> >> diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
> >> index f452417..7beed89 100644
> >> --- a/drivers/iio/pressure/st_pressure_core.c
> >> +++ b/drivers/iio/pressure/st_pressure_core.c
> >> @@ -23,6 +23,7 @@
> >>  #include <linux/iio/sysfs.h>
> >>  #include <linux/iio/trigger.h>
> >>  #include <linux/iio/buffer.h>
> >> +#include <linux/regulator/consumer.h>
> >>  #include <asm/unaligned.h>
> >>  
> >>  #include <linux/iio/common/st_sensors.h>
> >> @@ -315,6 +316,15 @@ int st_press_common_probe(struct iio_dev *indio_dev)
> >>  	indio_dev->modes = INDIO_DIRECT_MODE;
> >>  	indio_dev->info = &press_info;
> >>  
> >> +	/* Regulator not mandatory, but if requested we should enable it. */
> >> +	pdata->regulator = regulator_get(&indio_dev->dev, "vdd");
> >> +	if (!IS_ERR_OR_NULL(pdata->regulator)) {
> > 
> > Can regulator_get return NULL? As far as I can see, it either returns a
> > valid reulator pointer or an ERR_PTR value.
> > 
> > When you say "if requested", do you mean "if described in the dt"? If
> > so, the above doesn't distunguish between a regulator not being listed
> > and one failing to be got (e.g. if we got EPROBE_DEFER from
> > regulator_get).
> > 
> > I think this would be better handled with something like Mark Brown's
> > suggested regulator_get_optional [1,2].

Thanks Mark, I didn't know that existed.

> It can return NULL, but NULL is actually a valid regulator in that case, so
> the check should only be IS_ERR. And yes regulator_get_optional is what
> should be used here.

Okay, I'll use that instead.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 02/11] ARM: ux500: Enable the LPS001WP Pressure & Temperature sensor from DT

[Lee Jones]

On Wed, 04 Sep 2013, Mark Rutland wrote:

> Hi Lee,
> 
> On Wed, Sep 04, 2013 at 10:31:34AM +0100, Lee Jones wrote:
> > After applying this node the LPS001WP sensor chip should probe
> > successfully once the driver support has also been applied.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  arch/arm/boot/dts/ste-snowball.dts | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/ste-snowball.dts b/arch/arm/boot/dts/ste-snowball.dts
> > index cf9b16e..aad8f54 100644
> > --- a/arch/arm/boot/dts/ste-snowball.dts
> > +++ b/arch/arm/boot/dts/ste-snowball.dts
> > @@ -153,6 +153,16 @@
> >  			status = "okay";
> >  		};
> >  
> > +		i2c@80128000 {
> > +			lps001wp@5c {
> > +				compatible = "lps001wp";
> > +				reg = <0x5c>;
> > +
> > +				vdd-supply = <&ab8500_ldo_aux1_reg>;
> > +				vms-supply = <&db8500_vsmps2_reg>;
> > +			};
> > +		};
> > +
> 
> This appears to be missing a binding document. I couldn't see one
> anywhere in this series, or in mainline already).
> 
> As far as I can see, the compatible string should be "st,lps001wp".

The I2C subsystem doesn't actually care about vendor prefixes. They
are all stripped before use in all cases. However, I'm happy to
provide one if for no other reason than to stick to convention.

> Please produce a binding document.

Again, I'm happy to provide a boilerplate document, but there's
nothing special happening here.

> Is there any publicly available documentation on the chip?

If you Google the device name, it will be the first hit.

> >  		uart@80120000 {
> >  			status = "okay";
> >  		};

Kind regards,
Lee

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 02/11] ARM: ux500: Enable the LPS001WP Pressure & Temperature sensor from DT

[Lee Jones]

On Wed, 04 Sep 2013, Mark Rutland wrote:

> Hi Lee,
> 
> On Wed, Sep 04, 2013 at 10:31:34AM +0100, Lee Jones wrote:
> > After applying this node the LPS001WP sensor chip should probe
> > successfully once the driver support has also been applied.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  arch/arm/boot/dts/ste-snowball.dts | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/ste-snowball.dts b/arch/arm/boot/dts/ste-snowball.dts
> > index cf9b16e..aad8f54 100644
> > --- a/arch/arm/boot/dts/ste-snowball.dts
> > +++ b/arch/arm/boot/dts/ste-snowball.dts
> > @@ -153,6 +153,16 @@
> >  			status = "okay";
> >  		};
> >  
> > +		i2c@80128000 {
> > +			lps001wp@5c {
> > +				compatible = "lps001wp";
> > +				reg = <0x5c>;
> > +
> > +				vdd-supply = <&ab8500_ldo_aux1_reg>;
> > +				vms-supply = <&db8500_vsmps2_reg>;
> > +			};
> > +		};
> > +
> 
> This appears to be missing a binding document. I couldn't see one
> anywhere in this series, or in mainline already).
> 
> As far as I can see, the compatible string should be "st,lps001wp".
> 
> Please produce a binding document.

Patch sent, most of you are on CC.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v2 02/11] ARM: ux500: Enable the LPS001WP Pressure & Temperature sensor from DT

[Lee Jones]

After applying this node the LPS001WP sensor chip should probe
successfully once the driver support has also been applied.
    
Signed-off-by: Lee Jones <lee.jones@linaro.org>

diff --git a/arch/arm/boot/dts/ste-snowball.dts b/arch/arm/boot/dts/ste-snowball.dts
index cf9b16e..dc556d1 100644
--- a/arch/arm/boot/dts/ste-snowball.dts
+++ b/arch/arm/boot/dts/ste-snowball.dts
@@ -153,6 +153,16 @@
                        status = "okay";
                };
 
+               i2c@80128000 {
+                       lps001wp@5c {
+                               compatible = "st,lps001wp";
+                               reg = <0x5c>;
+
+                               vdd-supply = <&ab8500_ldo_aux1_reg>;
+                               vms-supply = <&db8500_vsmps2_reg>;
+                       };
+               };
+
                uart@80120000 {
                        status = "okay";
                };

Re: [PATCH 02/11] ARM: ux500: Enable the LPS001WP Pressure & Temperature sensor from DT

[Mark Rutland]

On Wed, Sep 04, 2013 at 02:36:54PM +0100, Lee Jones wrote:
> On Wed, 04 Sep 2013, Mark Rutland wrote:
> 
> > Hi Lee,
> > 
> > On Wed, Sep 04, 2013 at 10:31:34AM +0100, Lee Jones wrote:
> > > After applying this node the LPS001WP sensor chip should probe
> > > successfully once the driver support has also been applied.
> > > 
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  arch/arm/boot/dts/ste-snowball.dts | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/arch/arm/boot/dts/ste-snowball.dts b/arch/arm/boot/dts/ste-snowball.dts
> > > index cf9b16e..aad8f54 100644
> > > --- a/arch/arm/boot/dts/ste-snowball.dts
> > > +++ b/arch/arm/boot/dts/ste-snowball.dts
> > > @@ -153,6 +153,16 @@
> > >  			status = "okay";
> > >  		};
> > >  
> > > +		i2c@80128000 {
> > > +			lps001wp@5c {
> > > +				compatible = "lps001wp";
> > > +				reg = <0x5c>;
> > > +
> > > +				vdd-supply = <&ab8500_ldo_aux1_reg>;
> > > +				vms-supply = <&db8500_vsmps2_reg>;
> > > +			};
> > > +		};
> > > +
> > 
> > This appears to be missing a binding document. I couldn't see one
> > anywhere in this series, or in mainline already).
> > 
> > As far as I can see, the compatible string should be "st,lps001wp".
> 
> The I2C subsystem doesn't actually care about vendor prefixes. They
> are all stripped before use in all cases. However, I'm happy to
> provide one if for no other reason than to stick to convention.

I'd prefer if we did document this. Just because Linux doesn't care
about this at the moment doesn't mean we (or another OS) might want to
handle it differently in future. It also gives us a semblance of
consistency.

> 
> > Please produce a binding document.
> 
> Again, I'm happy to provide a boilerplate document, but there's
> nothing special happening here.

That's not true. The names of the regulators should be documented. One
seems to be optional for some reason.

> 
> > Is there any publicly available documentation on the chip?
> 
> If you Google the device name, it will be the first hit.

Ok, I'll take a look.

> 
> > >  		uart@80120000 {
> > >  			status = "okay";
> > >  		};
> 
> Kind regards,
> Lee

Cheers,
Mark.

Re: [PATCH 11/11] iio: pressure-core: st: Provide correct regulator support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 349 bytes --]

On Wed, Sep 04, 2013 at 02:26:38PM +0100, Lee Jones wrote:
> On Wed, 04 Sep 2013, Lars-Peter Clausen wrote:

> > > I think this would be better handled with something like Mark Brown's
> > > suggested regulator_get_optional [1,2].

> Thanks Mark, I didn't know that existed.

It's only just gone into Linus' tree this merge window so it's very new.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 04/11] iio: pressure-i2c: st: Simplify error checking in probe()

[Jonathan Cameron]

Hi Lee. This won't apply as this driver has already been cleaned up using devm_iio_device_alloc.

Guessing you are basing on an old tree? 
Always use at least staging-next for IIO patches.

I think this one will hit mainline in next few days.

Jonathan

Lee Jones <lee.jones@linaro.org> wrote:
>Strip out all the unnecessary gotos and check for NULL returns in the
>usual manner.
>
>Signed-off-by: Lee Jones <lee.jones@linaro.org>
>---
> drivers/iio/pressure/st_pressure_i2c.c | 17 ++++++-----------
> 1 file changed, 6 insertions(+), 11 deletions(-)
>
>diff --git a/drivers/iio/pressure/st_pressure_i2c.c
>b/drivers/iio/pressure/st_pressure_i2c.c
>index 7cebcc7..2ace770 100644
>--- a/drivers/iio/pressure/st_pressure_i2c.c
>+++ b/drivers/iio/pressure/st_pressure_i2c.c
>@@ -26,10 +26,8 @@ static int st_press_i2c_probe(struct i2c_client
>*client,
> 	int err;
> 
> 	indio_dev = iio_device_alloc(sizeof(*pdata));
>-	if (indio_dev == NULL) {
>-		err = -ENOMEM;
>-		goto iio_device_alloc_error;
>-	}
>+	if (!indio_dev)
>+		return -ENOMEM;
> 
> 	pdata = iio_priv(indio_dev);
> 	pdata->dev = &client->dev;
>@@ -37,15 +35,12 @@ static int st_press_i2c_probe(struct i2c_client
>*client,
> 	st_sensors_i2c_configure(indio_dev, client, pdata);
> 
> 	err = st_press_common_probe(indio_dev);
>-	if (err < 0)
>-		goto st_press_common_probe_error;
>+	if (err < 0) {
>+		iio_device_free(indio_dev);
>+		return err;
>+	}
> 
> 	return 0;
>-
>-st_press_common_probe_error:
>-	iio_device_free(indio_dev);
>-iio_device_alloc_error:
>-	return err;
> }
> 
> static int st_press_i2c_remove(struct i2c_client *client)

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Re: [PATCH 04/11] iio: pressure-i2c: st: Simplify error checking in probe()

[Lee Jones]

> Hi Lee. This won't apply as this driver has already been cleaned up using devm_iio_device_alloc.
> 
> Guessing you are basing on an old tree? 
> Always use at least staging-next for IIO patches.

Sure, I'll rebase and resubmit the set once all the other creases have
been ironed out.

> I think this one will hit mainline in next few days.

Okay, great.

> Lee Jones <lee.jones@linaro.org> wrote:
> >Strip out all the unnecessary gotos and check for NULL returns in the
> >usual manner.
> >
> >Signed-off-by: Lee Jones <lee.jones@linaro.org>
> >---
> > drivers/iio/pressure/st_pressure_i2c.c | 17 ++++++-----------
> > 1 file changed, 6 insertions(+), 11 deletions(-)
> >
> >diff --git a/drivers/iio/pressure/st_pressure_i2c.c
> >b/drivers/iio/pressure/st_pressure_i2c.c
> >index 7cebcc7..2ace770 100644
> >--- a/drivers/iio/pressure/st_pressure_i2c.c
> >+++ b/drivers/iio/pressure/st_pressure_i2c.c
> >@@ -26,10 +26,8 @@ static int st_press_i2c_probe(struct i2c_client
> >*client,
> > 	int err;
> > 
> > 	indio_dev = iio_device_alloc(sizeof(*pdata));
> >-	if (indio_dev == NULL) {
> >-		err = -ENOMEM;
> >-		goto iio_device_alloc_error;
> >-	}
> >+	if (!indio_dev)
> >+		return -ENOMEM;
> > 
> > 	pdata = iio_priv(indio_dev);
> > 	pdata->dev = &client->dev;
> >@@ -37,15 +35,12 @@ static int st_press_i2c_probe(struct i2c_client
> >*client,
> > 	st_sensors_i2c_configure(indio_dev, client, pdata);
> > 
> > 	err = st_press_common_probe(indio_dev);
> >-	if (err < 0)
> >-		goto st_press_common_probe_error;
> >+	if (err < 0) {
> >+		iio_device_free(indio_dev);
> >+		return err;
> >+	}
> > 
> > 	return 0;
> >-
> >-st_press_common_probe_error:
> >-	iio_device_free(indio_dev);
> >-iio_device_alloc_error:
> >-	return err;
> > }
> > 
> > static int st_press_i2c_remove(struct i2c_client *client)
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 09/11] iio: pressure-core: st: Clean-up error handling in probe function

[Jonathan Cameron]

Lee Jones <lee.jones@linaro.org> wrote:
>This patch contains some pretty basic clean-ups in probe() pertaining
>to
>the simplification of error handling and a couple of readability
>adaptions.
>
>Signed-off-by: Lee Jones <lee.jones@linaro.org>

Hi Lee

Most of this series look fine but I am not that keen on some of this one.

The white space changes are definitely a matter of taste and honestly I don't care either way.

The returns are fine when there is no cleanup to do.

GOTOs and a clean unwind of the function is to my eye easier to review than unwinding directly after each error.

If reviewing original driver I would probably not bother commenting but for a cleanup patch the cleanup wants to be a clear improvement!  

Out of time now so will have to get to the rest of the series sometime soon.  

>---
>drivers/iio/pressure/st_pressure_core.c | 41
>+++++++++++++++------------------
> 1 file changed, 19 insertions(+), 22 deletions(-)
>
>diff --git a/drivers/iio/pressure/st_pressure_core.c
>b/drivers/iio/pressure/st_pressure_core.c
>index 3cf54ed..2893d08 100644
>--- a/drivers/iio/pressure/st_pressure_core.c
>+++ b/drivers/iio/pressure/st_pressure_core.c
>@@ -224,21 +224,23 @@ static const struct iio_trigger_ops
>st_press_trigger_ops = {
> 
> int st_press_common_probe(struct iio_dev *indio_dev)
> {
>-	int err;
> 	struct st_sensor_data *pdata = iio_priv(indio_dev);
>+	int irq = pdata->get_irq_data_ready(indio_dev);
>+	int err;
> 
> 	indio_dev->modes = INDIO_DIRECT_MODE;
> 	indio_dev->info = &press_info;
> 
> 	err = st_sensors_check_device_support(indio_dev,
>-				ARRAY_SIZE(st_press_sensors), st_press_sensors);
>+					      ARRAY_SIZE(st_press_sensors),
>+					      st_press_sensors);
> 	if (err < 0)
>-		goto st_press_common_probe_error;
>+		return err;
> 
> 	pdata->num_data_channels = ST_PRESS_NUMBER_DATA_CHANNELS;
>-	pdata->multiread_bit = pdata->sensor->multi_read_bit;
>-	indio_dev->channels = pdata->sensor->ch;
>-	indio_dev->num_channels = pdata->sensor->num_ch;
>+	pdata->multiread_bit     = pdata->sensor->multi_read_bit;
>+	indio_dev->channels      = pdata->sensor->ch;
>+	indio_dev->num_channels  = pdata->sensor->num_ch;
> 
> 	if (pdata->sensor->fs.addr != 0)
> 		pdata->current_fullscale = (struct st_sensor_fullscale_avl *)
>@@ -248,32 +250,27 @@ int st_press_common_probe(struct iio_dev
>*indio_dev)
> 
> 	err = st_sensors_init_sensor(indio_dev);
> 	if (err < 0)
>-		goto st_press_common_probe_error;
>+		return err;
> 
>-	if (pdata->get_irq_data_ready(indio_dev) > 0) {
>+	if (irq > 0) {
> 		err = st_press_allocate_ring(indio_dev);
> 		if (err < 0)
>-			goto st_press_common_probe_error;
>+			return err;
> 
> 		err = st_sensors_allocate_trigger(indio_dev,
>-							ST_PRESS_TRIGGER_OPS);
>-		if (err < 0)
>-			goto st_press_probe_trigger_error;
>+						  ST_PRESS_TRIGGER_OPS);
>+		if (err < 0) {
>+			st_press_deallocate_ring(indio_dev);
>+			return err;
>+		}
> 	}
> 
> 	err = iio_device_register(indio_dev);
>-	if (err)
>-		goto st_press_device_register_error;
>-
>-	return err;
>-
>-st_press_device_register_error:
>-	if (pdata->get_irq_data_ready(indio_dev) > 0)
>+	if (err && irq > 0) {
> 		st_sensors_deallocate_trigger(indio_dev);
>-st_press_probe_trigger_error:
>-	if (pdata->get_irq_data_ready(indio_dev) > 0)
> 		st_press_deallocate_ring(indio_dev);
>-st_press_common_probe_error:
>+	}
>+
> 	return err;
> }
> EXPORT_SYMBOL(st_press_common_probe);

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Re: [PATCH 05/11] iio: pressure-core: st: Describe LPS331AP defines by name

[Denis CIOCCA]

Hi Lee,
> They're currently named *_1_*, for 'Sensor 1', but the code will be much
> more readable if we use the naming convention *_LPS331AP_* instead.
You are right, but the reason is to maintain the same structure of the 
other sensors drivers (like accel, gyro and magn). Often some sensors 
can use the same data and using 1,2,... I think it's more general.

Denis

> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>   drivers/iio/pressure/st_pressure_core.c | 90 ++++++++++++++++-----------------
>   1 file changed, 44 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
> index 9c343b4..becfb25 100644
> --- a/drivers/iio/pressure/st_pressure_core.c
> +++ b/drivers/iio/pressure/st_pressure_core.c
> @@ -31,92 +31,90 @@
>   #define ST_PRESS_MBAR_TO_KPASCAL(x)		(x * 10)
>   #define ST_PRESS_NUMBER_DATA_CHANNELS		1
>   
> -/* DEFAULT VALUE FOR SENSORS */
> -#define ST_PRESS_DEFAULT_OUT_XL_ADDR		0x28
> -#define ST_TEMP_DEFAULT_OUT_L_ADDR		0x2b
> -
>   /* FULLSCALE */
>   #define ST_PRESS_FS_AVL_1260MB			1260
>   
> -/* CUSTOM VALUES FOR SENSOR 1 */
> -#define ST_PRESS_1_WAI_EXP			0xbb
> -#define ST_PRESS_1_ODR_ADDR			0x20
> -#define ST_PRESS_1_ODR_MASK			0x70
> -#define ST_PRESS_1_ODR_AVL_1HZ_VAL		0x01
> -#define ST_PRESS_1_ODR_AVL_7HZ_VAL		0x05
> -#define ST_PRESS_1_ODR_AVL_13HZ_VAL		0x06
> -#define ST_PRESS_1_ODR_AVL_25HZ_VAL		0x07
> -#define ST_PRESS_1_PW_ADDR			0x20
> -#define ST_PRESS_1_PW_MASK			0x80
> -#define ST_PRESS_1_FS_ADDR			0x23
> -#define ST_PRESS_1_FS_MASK			0x30
> -#define ST_PRESS_1_FS_AVL_1260_VAL		0x00
> -#define ST_PRESS_1_FS_AVL_1260_GAIN		ST_PRESS_MBAR_TO_KPASCAL(244141)
> -#define ST_PRESS_1_FS_AVL_TEMP_GAIN		2083000
> -#define ST_PRESS_1_BDU_ADDR			0x20
> -#define ST_PRESS_1_BDU_MASK			0x04
> -#define ST_PRESS_1_DRDY_IRQ_ADDR		0x22
> -#define ST_PRESS_1_DRDY_IRQ_MASK		0x04
> -#define ST_PRESS_1_MULTIREAD_BIT		true
> -#define ST_PRESS_1_TEMP_OFFSET			42500
> +/* CUSTOM VALUES FOR LPS331AP SENSOR */
> +#define ST_PRESS_LPS331AP_WAI_EXP		0xbb
> +#define ST_PRESS_LPS331AP_ODR_ADDR		0x20
> +#define ST_PRESS_LPS331AP_ODR_MASK		0x70
> +#define ST_PRESS_LPS331AP_ODR_AVL_1HZ_VAL	0x01
> +#define ST_PRESS_LPS331AP_ODR_AVL_7HZ_VAL	0x05
> +#define ST_PRESS_LPS331AP_ODR_AVL_13HZ_VAL	0x06
> +#define ST_PRESS_LPS331AP_ODR_AVL_25HZ_VAL	0x07
> +#define ST_PRESS_LPS331AP_PW_ADDR		0x20
> +#define ST_PRESS_LPS331AP_PW_MASK		0x80
> +#define ST_PRESS_LPS331AP_FS_ADDR		0x23
> +#define ST_PRESS_LPS331AP_FS_MASK		0x30
> +#define ST_PRESS_LPS331AP_FS_AVL_1260_VAL	0x00
> +#define ST_PRESS_LPS331AP_FS_AVL_1260_GAIN	ST_PRESS_MBAR_TO_KPASCAL(244141)
> +#define ST_PRESS_LPS331AP_FS_AVL_TEMP_GAIN	2083000
> +#define ST_PRESS_LPS331AP_BDU_ADDR		0x20
> +#define ST_PRESS_LPS331AP_BDU_MASK		0x04
> +#define ST_PRESS_LPS331AP_DRDY_IRQ_ADDR		0x22
> +#define ST_PRESS_LPS331AP_DRDY_IRQ_MASK		0x04
> +#define ST_PRESS_LPS331AP_MULTIREAD_BIT		true
> +#define ST_PRESS_LPS331AP_TEMP_OFFSET		42500
> +#define ST_PRESS_LPS331AP_OUT_XL_ADDR		0x28
> +#define ST_TEMP_LPS331AP_OUT_L_ADDR		0x2b
>   
>   static const struct iio_chan_spec st_press_channels[] = {
>   	ST_SENSORS_LSM_CHANNELS(IIO_PRESSURE,
>   			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
>   			ST_SENSORS_SCAN_X, 0, IIO_NO_MOD, 'u', IIO_LE, 24, 24,
> -			ST_PRESS_DEFAULT_OUT_XL_ADDR),
> +			ST_PRESS_LPS331AP_OUT_XL_ADDR),
>   	ST_SENSORS_LSM_CHANNELS(IIO_TEMP,
>   			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE) |
>   						BIT(IIO_CHAN_INFO_OFFSET),
>   			-1, 0, IIO_NO_MOD, 's', IIO_LE, 16, 16,
> -			ST_TEMP_DEFAULT_OUT_L_ADDR),
> +			ST_TEMP_LPS331AP_OUT_L_ADDR),
>   	IIO_CHAN_SOFT_TIMESTAMP(1)
>   };
>   
>   static const struct st_sensors st_press_sensors[] = {
>   	{
> -		.wai = ST_PRESS_1_WAI_EXP,
> +		.wai = ST_PRESS_LPS331AP_WAI_EXP,
>   		.sensors_supported = {
>   			[0] = LPS331AP_PRESS_DEV_NAME,
>   		},
>   		.ch = (struct iio_chan_spec *)st_press_channels,
>   		.odr = {
> -			.addr = ST_PRESS_1_ODR_ADDR,
> -			.mask = ST_PRESS_1_ODR_MASK,
> +			.addr = ST_PRESS_LPS331AP_ODR_ADDR,
> +			.mask = ST_PRESS_LPS331AP_ODR_MASK,
>   			.odr_avl = {
> -				{ 1, ST_PRESS_1_ODR_AVL_1HZ_VAL, },
> -				{ 7, ST_PRESS_1_ODR_AVL_7HZ_VAL, },
> -				{ 13, ST_PRESS_1_ODR_AVL_13HZ_VAL, },
> -				{ 25, ST_PRESS_1_ODR_AVL_25HZ_VAL, },
> +				{ 1, ST_PRESS_LPS331AP_ODR_AVL_1HZ_VAL, },
> +				{ 7, ST_PRESS_LPS331AP_ODR_AVL_7HZ_VAL, },
> +				{ 13, ST_PRESS_LPS331AP_ODR_AVL_13HZ_VAL, },
> +				{ 25, ST_PRESS_LPS331AP_ODR_AVL_25HZ_VAL, },
>   			},
>   		},
>   		.pw = {
> -			.addr = ST_PRESS_1_PW_ADDR,
> -			.mask = ST_PRESS_1_PW_MASK,
> +			.addr = ST_PRESS_LPS331AP_PW_ADDR,
> +			.mask = ST_PRESS_LPS331AP_PW_MASK,
>   			.value_on = ST_SENSORS_DEFAULT_POWER_ON_VALUE,
>   			.value_off = ST_SENSORS_DEFAULT_POWER_OFF_VALUE,
>   		},
>   		.fs = {
> -			.addr = ST_PRESS_1_FS_ADDR,
> -			.mask = ST_PRESS_1_FS_MASK,
> +			.addr = ST_PRESS_LPS331AP_FS_ADDR,
> +			.mask = ST_PRESS_LPS331AP_FS_MASK,
>   			.fs_avl = {
>   				[0] = {
>   					.num = ST_PRESS_FS_AVL_1260MB,
> -					.value = ST_PRESS_1_FS_AVL_1260_VAL,
> -					.gain = ST_PRESS_1_FS_AVL_1260_GAIN,
> -					.gain2 = ST_PRESS_1_FS_AVL_TEMP_GAIN,
> +					.value = ST_PRESS_LPS331AP_FS_AVL_1260_VAL,
> +					.gain = ST_PRESS_LPS331AP_FS_AVL_1260_GAIN,
> +					.gain2 = ST_PRESS_LPS331AP_FS_AVL_TEMP_GAIN,
>   				},
>   			},
>   		},
>   		.bdu = {
> -			.addr = ST_PRESS_1_BDU_ADDR,
> -			.mask = ST_PRESS_1_BDU_MASK,
> +			.addr = ST_PRESS_LPS331AP_BDU_ADDR,
> +			.mask = ST_PRESS_LPS331AP_BDU_MASK,
>   		},
>   		.drdy_irq = {
> -			.addr = ST_PRESS_1_DRDY_IRQ_ADDR,
> -			.mask = ST_PRESS_1_DRDY_IRQ_MASK,
> +			.addr = ST_PRESS_LPS331AP_DRDY_IRQ_ADDR,
> +			.mask = ST_PRESS_LPS331AP_DRDY_IRQ_MASK,
>   		},
> -		.multi_read_bit = ST_PRESS_1_MULTIREAD_BIT,
> +		.multi_read_bit = ST_PRESS_LPS331AP_MULTIREAD_BIT,
>   		.bootime = 2,
>   	},
>   };

Re: [PATCH 06/11] iio: pressure-core: st: Expand and rename LPS331AP's channel descriptor

[Denis CIOCCA]

> Due to the MACRO used, the task of reading, understanding and maintaining
> the LPS331AP's channel descriptor is substantially difficult. This patch
> is based on the view that it's better to have easy to read, maintainable
> code than to save a few lines here and there. For that reason we're
> expanding the array so initialisation is completed in full.
Also for this one, the channel names are general and can be shared 
between different sensors. For the channel definition it's not a problem 
for me, but I think it's not necessary adds all that code...

Denis

> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>   drivers/iio/pressure/st_pressure_core.c | 45 +++++++++++++++++++++++++--------
>   1 file changed, 34 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
> index becfb25..7ba9299 100644
> --- a/drivers/iio/pressure/st_pressure_core.c
> +++ b/drivers/iio/pressure/st_pressure_core.c
> @@ -58,16 +58,39 @@
>   #define ST_PRESS_LPS331AP_OUT_XL_ADDR		0x28
>   #define ST_TEMP_LPS331AP_OUT_L_ADDR		0x2b
>   
> -static const struct iio_chan_spec st_press_channels[] = {
> -	ST_SENSORS_LSM_CHANNELS(IIO_PRESSURE,
> +static const struct iio_chan_spec st_press_lsp331ap_channels[] = {
> +	{
> +		.type = IIO_PRESSURE,
> +		.channel2 = IIO_NO_MOD,
> +		.address = ST_PRESS_LPS331AP_OUT_XL_ADDR,
> +		.scan_index = ST_SENSORS_SCAN_X,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 24,
> +			.storagebits = 24,
> +			.endianness = IIO_LE,
> +		},
> +		.info_mask_separate =
>   			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> -			ST_SENSORS_SCAN_X, 0, IIO_NO_MOD, 'u', IIO_LE, 24, 24,
> -			ST_PRESS_LPS331AP_OUT_XL_ADDR),
> -	ST_SENSORS_LSM_CHANNELS(IIO_TEMP,
> -			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE) |
> -						BIT(IIO_CHAN_INFO_OFFSET),
> -			-1, 0, IIO_NO_MOD, 's', IIO_LE, 16, 16,
> -			ST_TEMP_LPS331AP_OUT_L_ADDR),
> +		.modified = 0,
> +	},
> +	{
> +		.type = IIO_TEMP,
> +		.channel2 = IIO_NO_MOD,
> +		.address = ST_TEMP_LPS331AP_OUT_L_ADDR,
> +		.scan_index = -1,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.endianness = IIO_LE,
> +		},
> +		.info_mask_separate =
> +			BIT(IIO_CHAN_INFO_RAW) |
> +			BIT(IIO_CHAN_INFO_SCALE) |
> +			BIT(IIO_CHAN_INFO_OFFSET),
> +		.modified = 0,
> +	},
>   	IIO_CHAN_SOFT_TIMESTAMP(1)
>   };
>   
> @@ -77,7 +100,7 @@ static const struct st_sensors st_press_sensors[] = {
>   		.sensors_supported = {
>   			[0] = LPS331AP_PRESS_DEV_NAME,
>   		},
> -		.ch = (struct iio_chan_spec *)st_press_channels,
> +		.ch = (struct iio_chan_spec *)st_press_lsp331ap_channels,
>   		.odr = {
>   			.addr = ST_PRESS_LPS331AP_ODR_ADDR,
>   			.mask = ST_PRESS_LPS331AP_ODR_MASK,
> @@ -214,7 +237,7 @@ int st_press_common_probe(struct iio_dev *indio_dev)
>   	pdata->num_data_channels = ST_PRESS_NUMBER_DATA_CHANNELS;
>   	pdata->multiread_bit = pdata->sensor->multi_read_bit;
>   	indio_dev->channels = pdata->sensor->ch;
> -	indio_dev->num_channels = ARRAY_SIZE(st_press_channels);
> +	indio_dev->num_channels = ARRAY_SIZE(st_press_lsp331ap_channels);
>   
>   	pdata->current_fullscale = (struct st_sensor_fullscale_avl *)
>   						&pdata->sensor->fs.fs_avl[0];

Re: [PATCH 07/11] iio: sensors-core: st: Allow full-scale to be an optional feature

[Denis CIOCCA]

Acked-by: Denis Ciocca <denis.ciocca@st.com>
> Some chips either don't support it or fail to provide adequate documentation,
> so sometimes it's impossible to enable the feature even if it is supported.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>   drivers/iio/common/st_sensors/st_sensors_core.c | 11 +++++++----
>   drivers/iio/pressure/st_pressure_core.c         |  6 ++++--
>   2 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
> index 865b178..fc9acb7 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> @@ -209,10 +209,13 @@ int st_sensors_init_sensor(struct iio_dev *indio_dev)
>   	if (err < 0)
>   		goto init_error;
>   
> -	err = st_sensors_set_fullscale(indio_dev,
> -						sdata->current_fullscale->num);
> -	if (err < 0)
> -		goto init_error;
> +	if (sdata->current_fullscale) {
> +		err = st_sensors_set_fullscale(indio_dev,
> +					       sdata->current_fullscale->num);
> +		if (err < 0)
> +			goto init_error;
> +	} else
> +		dev_info(&indio_dev->dev, "Full-scale not possible\n");
>   
>   	err = st_sensors_set_odr(indio_dev, sdata->odr);
>   	if (err < 0)
> diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
> index 7ba9299..423ed6a 100644
> --- a/drivers/iio/pressure/st_pressure_core.c
> +++ b/drivers/iio/pressure/st_pressure_core.c
> @@ -239,8 +239,10 @@ int st_press_common_probe(struct iio_dev *indio_dev)
>   	indio_dev->channels = pdata->sensor->ch;
>   	indio_dev->num_channels = ARRAY_SIZE(st_press_lsp331ap_channels);
>   
> -	pdata->current_fullscale = (struct st_sensor_fullscale_avl *)
> -						&pdata->sensor->fs.fs_avl[0];
> +	if (pdata->sensor->fs.addr != 0)
> +		pdata->current_fullscale = (struct st_sensor_fullscale_avl *)
> +			&pdata->sensor->fs.fs_avl[0];
> +
>   	pdata->odr = pdata->sensor->odr.odr_avl[0].hz;
>   
>   	err = st_sensors_init_sensor(indio_dev);

Re: [PATCH 08/11] iio: pressure-core: st: Allow for number of channels to vary

[Denis CIOCCA]

Acked-by: Denis Ciocca <denis.ciocca@st.com>
> At the moment the number of channels specified is dictated by the first
> sensor supported by the driver. As we add support for more sensors this
> is likely to vary. Instead of using the ARRAY_SIZE() of the LPS331AP's
> channel specifier we'll use a new adaptable 'struct st_sensors' element
> instead.
>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>   drivers/iio/pressure/st_pressure_core.c | 3 ++-
>   include/linux/iio/common/st_sensors.h   | 1 +
>   2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
> index 423ed6a..3cf54ed 100644
> --- a/drivers/iio/pressure/st_pressure_core.c
> +++ b/drivers/iio/pressure/st_pressure_core.c
> @@ -101,6 +101,7 @@ static const struct st_sensors st_press_sensors[] = {
>   			[0] = LPS331AP_PRESS_DEV_NAME,
>   		},
>   		.ch = (struct iio_chan_spec *)st_press_lsp331ap_channels,
> +		.num_ch = ARRAY_SIZE(st_press_lsp331ap_channels),
>   		.odr = {
>   			.addr = ST_PRESS_LPS331AP_ODR_ADDR,
>   			.mask = ST_PRESS_LPS331AP_ODR_MASK,
> @@ -237,7 +238,7 @@ int st_press_common_probe(struct iio_dev *indio_dev)
>   	pdata->num_data_channels = ST_PRESS_NUMBER_DATA_CHANNELS;
>   	pdata->multiread_bit = pdata->sensor->multi_read_bit;
>   	indio_dev->channels = pdata->sensor->ch;
> -	indio_dev->num_channels = ARRAY_SIZE(st_press_lsp331ap_channels);
> +	indio_dev->num_channels = pdata->sensor->num_ch;
>   
>   	if (pdata->sensor->fs.addr != 0)
>   		pdata->current_fullscale = (struct st_sensor_fullscale_avl *)
> diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
> index 72b2694..4aef925 100644
> --- a/include/linux/iio/common/st_sensors.h
> +++ b/include/linux/iio/common/st_sensors.h
> @@ -180,6 +180,7 @@ struct st_sensors {
>   	u8 wai;
>   	char sensors_supported[ST_SENSORS_MAX_4WAI][ST_SENSORS_MAX_NAME];
>   	struct iio_chan_spec *ch;
> +	int num_ch;
>   	struct st_sensor_odr odr;
>   	struct st_sensor_power pw;
>   	struct st_sensor_axis enable_axis;

Re: [PATCH 06/11] iio: pressure-core: st: Expand and rename LPS331AP's channel descriptor

[Lee Jones]

On Wed, 04 Sep 2013, Denis CIOCCA wrote:

> > Due to the MACRO used, the task of reading, understanding and maintaining
> > the LPS331AP's channel descriptor is substantially difficult. This patch
> > is based on the view that it's better to have easy to read, maintainable
> > code than to save a few lines here and there. For that reason we're
> > expanding the array so initialisation is completed in full.
> Also for this one, the channel names are general and can be shared 
> between different sensors. For the channel definition it's not a problem 
> for me, but I think it's not necessary adds all that code...

I'm not sure what you mean by this. Would you be kind enough to
explain it in a different way please?

> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >   drivers/iio/pressure/st_pressure_core.c | 45 +++++++++++++++++++++++++--------
> >   1 file changed, 34 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
> > index becfb25..7ba9299 100644
> > --- a/drivers/iio/pressure/st_pressure_core.c
> > +++ b/drivers/iio/pressure/st_pressure_core.c
> > @@ -58,16 +58,39 @@
> >   #define ST_PRESS_LPS331AP_OUT_XL_ADDR		0x28
> >   #define ST_TEMP_LPS331AP_OUT_L_ADDR		0x2b
> >   
> > -static const struct iio_chan_spec st_press_channels[] = {
> > -	ST_SENSORS_LSM_CHANNELS(IIO_PRESSURE,
> > +static const struct iio_chan_spec st_press_lsp331ap_channels[] = {
> > +	{
> > +		.type = IIO_PRESSURE,
> > +		.channel2 = IIO_NO_MOD,
> > +		.address = ST_PRESS_LPS331AP_OUT_XL_ADDR,
> > +		.scan_index = ST_SENSORS_SCAN_X,
> > +		.scan_type = {
> > +			.sign = 'u',
> > +			.realbits = 24,
> > +			.storagebits = 24,
> > +			.endianness = IIO_LE,
> > +		},
> > +		.info_mask_separate =
> >   			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> > -			ST_SENSORS_SCAN_X, 0, IIO_NO_MOD, 'u', IIO_LE, 24, 24,
> > -			ST_PRESS_LPS331AP_OUT_XL_ADDR),
> > -	ST_SENSORS_LSM_CHANNELS(IIO_TEMP,
> > -			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE) |
> > -						BIT(IIO_CHAN_INFO_OFFSET),
> > -			-1, 0, IIO_NO_MOD, 's', IIO_LE, 16, 16,
> > -			ST_TEMP_LPS331AP_OUT_L_ADDR),
> > +		.modified = 0,
> > +	},
> > +	{
> > +		.type = IIO_TEMP,
> > +		.channel2 = IIO_NO_MOD,
> > +		.address = ST_TEMP_LPS331AP_OUT_L_ADDR,
> > +		.scan_index = -1,
> > +		.scan_type = {
> > +			.sign = 'u',
> > +			.realbits = 16,
> > +			.storagebits = 16,
> > +			.endianness = IIO_LE,
> > +		},
> > +		.info_mask_separate =
> > +			BIT(IIO_CHAN_INFO_RAW) |
> > +			BIT(IIO_CHAN_INFO_SCALE) |
> > +			BIT(IIO_CHAN_INFO_OFFSET),
> > +		.modified = 0,
> > +	},
> >   	IIO_CHAN_SOFT_TIMESTAMP(1)
> >   };
> >   
> > @@ -77,7 +100,7 @@ static const struct st_sensors st_press_sensors[] = {
> >   		.sensors_supported = {
> >   			[0] = LPS331AP_PRESS_DEV_NAME,
> >   		},
> > -		.ch = (struct iio_chan_spec *)st_press_channels,
> > +		.ch = (struct iio_chan_spec *)st_press_lsp331ap_channels,
> >   		.odr = {
> >   			.addr = ST_PRESS_LPS331AP_ODR_ADDR,
> >   			.mask = ST_PRESS_LPS331AP_ODR_MASK,
> > @@ -214,7 +237,7 @@ int st_press_common_probe(struct iio_dev *indio_dev)
> >   	pdata->num_data_channels = ST_PRESS_NUMBER_DATA_CHANNELS;
> >   	pdata->multiread_bit = pdata->sensor->multi_read_bit;
> >   	indio_dev->channels = pdata->sensor->ch;
> > -	indio_dev->num_channels = ARRAY_SIZE(st_press_channels);
> > +	indio_dev->num_channels = ARRAY_SIZE(st_press_lsp331ap_channels);
> >   
> >   	pdata->current_fullscale = (struct st_sensor_fullscale_avl *)
> >   						&pdata->sensor->fs.fs_avl[0];

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 06/11] iio: pressure-core: st: Expand and rename LPS331AP's channel descriptor

[Denis CIOCCA]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4209 bytes --]


>>> Due to the MACRO used, the task of reading, understanding and maintaining
>>> the LPS331AP's channel descriptor is substantially difficult. This patch
>>> is based on the view that it's better to have easy to read, maintainable
>>> code than to save a few lines here and there. For that reason we're
>>> expanding the array so initialisation is completed in full.
>> Also for this one, the channel names are general and can be shared
>> between different sensors. For the channel definition it's not a problem
>> for me, but I think it's not necessary adds all that code...
> I'm not sure what you mean by this. Would you be kind enough to
> explain it in a different way please?
The channel name (in this case st_press_channels) is not only specific 
for one sensor but can be shared. Ok in this driver now is used only for 
the lps331ap but for example in accelerometer driver is used by several 
sensors. It's possible in the future for new pressure sensors use the 
same channels definition.
The channel definition is intended the switch by macro 
ST_SENSORS_LSM_CHANNELS to the full definition, for me is not a problem 
but I think it's not necessary.

Denis

>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>>> ---
>>>    drivers/iio/pressure/st_pressure_core.c | 45 +++++++++++++++++++++++++--------
>>>    1 file changed, 34 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
>>> index becfb25..7ba9299 100644
>>> --- a/drivers/iio/pressure/st_pressure_core.c
>>> +++ b/drivers/iio/pressure/st_pressure_core.c
>>> @@ -58,16 +58,39 @@
>>>    #define ST_PRESS_LPS331AP_OUT_XL_ADDR		0x28
>>>    #define ST_TEMP_LPS331AP_OUT_L_ADDR		0x2b
>>>    
>>> -static const struct iio_chan_spec st_press_channels[] = {
>>> -	ST_SENSORS_LSM_CHANNELS(IIO_PRESSURE,
>>> +static const struct iio_chan_spec st_press_lsp331ap_channels[] = {
>>> +	{
>>> +		.type = IIO_PRESSURE,
>>> +		.channel2 = IIO_NO_MOD,
>>> +		.address = ST_PRESS_LPS331AP_OUT_XL_ADDR,
>>> +		.scan_index = ST_SENSORS_SCAN_X,
>>> +		.scan_type = {
>>> +			.sign = 'u',
>>> +			.realbits = 24,
>>> +			.storagebits = 24,
>>> +			.endianness = IIO_LE,
>>> +		},
>>> +		.info_mask_separate =
>>>    			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
>>> -			ST_SENSORS_SCAN_X, 0, IIO_NO_MOD, 'u', IIO_LE, 24, 24,
>>> -			ST_PRESS_LPS331AP_OUT_XL_ADDR),
>>> -	ST_SENSORS_LSM_CHANNELS(IIO_TEMP,
>>> -			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE) |
>>> -						BIT(IIO_CHAN_INFO_OFFSET),
>>> -			-1, 0, IIO_NO_MOD, 's', IIO_LE, 16, 16,
>>> -			ST_TEMP_LPS331AP_OUT_L_ADDR),
>>> +		.modified = 0,
>>> +	},
>>> +	{
>>> +		.type = IIO_TEMP,
>>> +		.channel2 = IIO_NO_MOD,
>>> +		.address = ST_TEMP_LPS331AP_OUT_L_ADDR,
>>> +		.scan_index = -1,
>>> +		.scan_type = {
>>> +			.sign = 'u',
>>> +			.realbits = 16,
>>> +			.storagebits = 16,
>>> +			.endianness = IIO_LE,
>>> +		},
>>> +		.info_mask_separate =
>>> +			BIT(IIO_CHAN_INFO_RAW) |
>>> +			BIT(IIO_CHAN_INFO_SCALE) |
>>> +			BIT(IIO_CHAN_INFO_OFFSET),
>>> +		.modified = 0,
>>> +	},
>>>    	IIO_CHAN_SOFT_TIMESTAMP(1)
>>>    };
>>>    
>>> @@ -77,7 +100,7 @@ static const struct st_sensors st_press_sensors[] = {
>>>    		.sensors_supported = {
>>>    			[0] = LPS331AP_PRESS_DEV_NAME,
>>>    		},
>>> -		.ch = (struct iio_chan_spec *)st_press_channels,
>>> +		.ch = (struct iio_chan_spec *)st_press_lsp331ap_channels,
>>>    		.odr = {
>>>    			.addr = ST_PRESS_LPS331AP_ODR_ADDR,
>>>    			.mask = ST_PRESS_LPS331AP_ODR_MASK,
>>> @@ -214,7 +237,7 @@ int st_press_common_probe(struct iio_dev *indio_dev)
>>>    	pdata->num_data_channels = ST_PRESS_NUMBER_DATA_CHANNELS;
>>>    	pdata->multiread_bit = pdata->sensor->multi_read_bit;
>>>    	indio_dev->channels = pdata->sensor->ch;
>>> -	indio_dev->num_channels = ARRAY_SIZE(st_press_channels);
>>> +	indio_dev->num_channels = ARRAY_SIZE(st_press_lsp331ap_channels);
>>>    
>>>    	pdata->current_fullscale = (struct st_sensor_fullscale_avl *)
>>>    						&pdata->sensor->fs.fs_avl[0];
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 05/11] iio: pressure-core: st: Describe LPS331AP defines by name

[Lee Jones]

> > They're currently named *_1_*, for 'Sensor 1', but the code will be much
> > more readable if we use the naming convention *_LPS331AP_* instead.
> You are right, but the reason is to maintain the same structure of the 
> other sensors drivers (like accel, gyro and magn). Often some sensors 
> can use the same data and using 1,2,... I think it's more general.

I've just coded this up for the pressure sensor [1] and in my opinion it
looks pretty ugly. We save 5 lines in this usecase, but we're
separating out some of the register addresses from the masks
etc.

Besides some code space #defines don't actually cost anything, so
unless the register set is almost identical I'd suggest putting them
in a header file out the way and separating them out like in the
original patch. I personally think it makes things so much clearer for
the reader.

I haven't looked at the other sensors yet, so perhaps we need to
evaluate this on a case by case basis, but certainly for this driver,
as we only support two sensors currently and for the sake of 5 lines I
think it's better to have them clearly defined per-sensor.

What do you think?

[1]:

/* GROUP VALUES SENSOR */
#define ST_PRESS_1_ODR_ADDR			0x20
#define ST_PRESS_1_ODR_AVL_1HZ_VAL		0x01
#define ST_PRESS_1_PW_ADDR			0x20
#define ST_PRESS_1_BDU_ADDR			0x20
#define ST_PRESS_1_MULTIREAD_BIT		true

/* CUSTOM VALUES FOR LPS331AP SENSOR */
#define ST_PRESS_LPS331AP_WAI_EXP		0xbb
#define ST_PRESS_LPS331AP_ODR_MASK		0x70
#define ST_PRESS_LPS331AP_ODR_AVL_7HZ_VAL	0x05
#define ST_PRESS_LPS331AP_ODR_AVL_13HZ_VAL	0x06
#define ST_PRESS_LPS331AP_ODR_AVL_25HZ_VAL	0x07
#define ST_PRESS_LPS331AP_PW_MASK		0x80
#define ST_PRESS_LPS331AP_FS_ADDR		0x23
#define ST_PRESS_LPS331AP_FS_MASK		0x30
#define ST_PRESS_LPS331AP_FS_AVL_1260_VAL	0x00
#define ST_PRESS_LPS331AP_FS_AVL_1260_GAIN	ST_PRESS_MBAR_TO_KPASCAL(244141)
#define ST_PRESS_LPS331AP_FS_AVL_TEMP_GAIN	2083000
#define ST_PRESS_LPS331AP_BDU_MASK		0x04
#define ST_PRESS_LPS331AP_DRDY_IRQ_ADDR		0x22
#define ST_PRESS_LPS331AP_DRDY_IRQ_MASK		0x04
#define ST_PRESS_LPS331AP_TEMP_OFFSET		42500
#define ST_PRESS_LPS331AP_OUT_XL_ADDR		0x28
#define ST_TEMP_LPS331AP_OUT_L_ADDR		0x2b

/* CUSTOM VALUES FOR LPS001WP SENSOR */
#define ST_PRESS_LPS001WP_WAI_EXP		0xba
#define ST_PRESS_LPS001WP_ODR_MASK		0x30
#define ST_PRESS_LPS001WP_ODR_AVL_7HZ_VAL	0x02
#define ST_PRESS_LPS001WP_ODR_AVL_13HZ_VAL	0x03
#define ST_PRESS_LPS001WP_PW_MASK		0x40
#define ST_PRESS_LPS001WP_BDU_MASK		0x04
#define ST_PRESS_LPS001WP_OUT_L_ADDR		0x28
#define ST_TEMP_LPS001WP_OUT_L_ADDR		0x2a

static const struct iio_chan_spec st_press_lsp331ap_channels[] = {
	{
		.type = IIO_PRESSURE,
		.channel2 = IIO_NO_MOD,
		.address = ST_PRESS_LPS331AP_OUT_XL_ADDR,
		.scan_index = ST_SENSORS_SCAN_X,
		.scan_type = {
			.sign = 'u',
			.realbits = 24,
			.storagebits = 24,
			.endianness = IIO_LE,
		},
		.info_mask_separate =
			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
		.modified = 0,
	},
	{
		.type = IIO_TEMP,
		.channel2 = IIO_NO_MOD,
		.address = ST_TEMP_LPS331AP_OUT_L_ADDR,
		.scan_index = -1,
		.scan_type = {
			.sign = 'u',
			.realbits = 16,
			.storagebits = 16,
			.endianness = IIO_LE,
		},
		.info_mask_separate =
			BIT(IIO_CHAN_INFO_RAW) |
			BIT(IIO_CHAN_INFO_SCALE) |
			BIT(IIO_CHAN_INFO_OFFSET),
		.modified = 0,
	},
	IIO_CHAN_SOFT_TIMESTAMP(1)
};

static const struct iio_chan_spec st_press_lps001wp_channels[] = {
	{
		.type = IIO_PRESSURE,
		.channel2 = IIO_NO_MOD,
		.address = ST_PRESS_LPS001WP_OUT_L_ADDR,
		.scan_index = ST_SENSORS_SCAN_X,
		.scan_type = {
			.sign = 'u',
			.realbits = 16,
			.storagebits = 16,
			.endianness = IIO_LE,
		},
		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
		.modified = 0,
	},
	{
		.type = IIO_TEMP,
		.channel2 = IIO_NO_MOD,
		.address = ST_TEMP_LPS001WP_OUT_L_ADDR,
		.scan_index = -1,
		.scan_type = {
			.sign = 'u',
			.realbits = 16,
			.storagebits = 16,
			.endianness = IIO_LE,
		},
		.info_mask_separate =
			BIT(IIO_CHAN_INFO_RAW) |
			BIT(IIO_CHAN_INFO_OFFSET),
		.modified = 0,
	},
	IIO_CHAN_SOFT_TIMESTAMP(1)
};

static const struct st_sensors st_press_sensors[] = {
	{
		.wai = ST_PRESS_LPS331AP_WAI_EXP,
		.sensors_supported = {
			[0] = LPS331AP_PRESS_DEV_NAME,
		},
		.ch = (struct iio_chan_spec *)st_press_lsp331ap_channels,
		.num_ch = ARRAY_SIZE(st_press_lsp331ap_channels),
		.odr = {
			.addr = ST_PRESS_1_ODR_ADDR,
			.mask = ST_PRESS_LPS331AP_ODR_MASK,
			.odr_avl = {
				{ 1, ST_PRESS_1_ODR_AVL_1HZ_VAL, },
				{ 7, ST_PRESS_LPS331AP_ODR_AVL_7HZ_VAL, },
				{ 13, ST_PRESS_LPS331AP_ODR_AVL_13HZ_VAL, },
				{ 25, ST_PRESS_LPS331AP_ODR_AVL_25HZ_VAL, },
			},
		},
		.pw = {
			.addr = ST_PRESS_1_PW_ADDR,
			.mask = ST_PRESS_LPS331AP_PW_MASK,
			.value_on = ST_SENSORS_DEFAULT_POWER_ON_VALUE,
			.value_off = ST_SENSORS_DEFAULT_POWER_OFF_VALUE,
		},
		.fs = {
			.addr = ST_PRESS_LPS331AP_FS_ADDR,
			.mask = ST_PRESS_LPS331AP_FS_MASK,
			.fs_avl = {
				[0] = {
					.num = ST_PRESS_FS_AVL_1260MB,
					.value = ST_PRESS_LPS331AP_FS_AVL_1260_VAL,
					.gain = ST_PRESS_LPS331AP_FS_AVL_1260_GAIN,
					.gain2 = ST_PRESS_LPS331AP_FS_AVL_TEMP_GAIN,
				},
			},
		},
		.bdu = {
			.addr = ST_PRESS_1_BDU_ADDR,
			.mask = ST_PRESS_LPS331AP_BDU_MASK,
		},
		.drdy_irq = {
			.addr = ST_PRESS_LPS331AP_DRDY_IRQ_ADDR,
			.mask = ST_PRESS_LPS331AP_DRDY_IRQ_MASK,
		},
		.multi_read_bit = ST_PRESS_1_MULTIREAD_BIT,
		.bootime = 2,
	},
	{
		.wai = ST_PRESS_LPS001WP_WAI_EXP,
		.sensors_supported = {
			[0] = LPS001WP_PRESS_DEV_NAME,
		},
		.ch = (struct iio_chan_spec *)st_press_lps001wp_channels,
		.num_ch = ARRAY_SIZE(st_press_lps001wp_channels),
		.odr = {
			.addr = ST_PRESS_1_ODR_ADDR,
			.mask = ST_PRESS_LPS001WP_ODR_MASK,
			.odr_avl = {
				{ 1, ST_PRESS_1_ODR_AVL_1HZ_VAL, },
				{ 7, ST_PRESS_LPS001WP_ODR_AVL_7HZ_VAL, },
				{ 13, ST_PRESS_LPS001WP_ODR_AVL_13HZ_VAL, },
			},
		},
		.pw = {
			.addr = ST_PRESS_1_PW_ADDR,
			.mask = ST_PRESS_LPS001WP_PW_MASK,
			.value_on = ST_SENSORS_DEFAULT_POWER_ON_VALUE,
			.value_off = ST_SENSORS_DEFAULT_POWER_OFF_VALUE,
		},
		.fs = {
			.addr = 0,
		},
		.bdu = {
			.addr = ST_PRESS_1_BDU_ADDR,
			.mask = ST_PRESS_LPS001WP_BDU_MASK,
		},
		.drdy_irq = {
			.addr = 0,
		},
		.multi_read_bit = ST_PRESS_1_MULTIREAD_BIT,
		.bootime = 2,
	},
};


> > -/* CUSTOM VALUES FOR SENSOR 1 */
> > -#define ST_PRESS_1_WAI_EXP			0xbb
> > -#define ST_PRESS_1_ODR_ADDR			0x20
> > -#define ST_PRESS_1_ODR_MASK			0x70
> > -#define ST_PRESS_1_ODR_AVL_1HZ_VAL		0x01
> > -#define ST_PRESS_1_ODR_AVL_7HZ_VAL		0x05
> > -#define ST_PRESS_1_ODR_AVL_13HZ_VAL		0x06
> > -#define ST_PRESS_1_ODR_AVL_25HZ_VAL		0x07
> > -#define ST_PRESS_1_PW_ADDR			0x20
> > -#define ST_PRESS_1_PW_MASK			0x80
> > -#define ST_PRESS_1_FS_ADDR			0x23
> > -#define ST_PRESS_1_FS_MASK			0x30
> > -#define ST_PRESS_1_FS_AVL_1260_VAL		0x00
> > -#define ST_PRESS_1_FS_AVL_1260_GAIN		ST_PRESS_MBAR_TO_KPASCAL(244141)
> > -#define ST_PRESS_1_FS_AVL_TEMP_GAIN		2083000
> > -#define ST_PRESS_1_BDU_ADDR			0x20
> > -#define ST_PRESS_1_BDU_MASK			0x04
> > -#define ST_PRESS_1_DRDY_IRQ_ADDR		0x22
> > -#define ST_PRESS_1_DRDY_IRQ_MASK		0x04
> > -#define ST_PRESS_1_MULTIREAD_BIT		true
> > -#define ST_PRESS_1_TEMP_OFFSET			42500
> > +/* CUSTOM VALUES FOR LPS331AP SENSOR */
> > +#define ST_PRESS_LPS331AP_WAI_EXP		0xbb
> > +#define ST_PRESS_LPS331AP_ODR_ADDR		0x20
> > +#define ST_PRESS_LPS331AP_ODR_MASK		0x70
> > +#define ST_PRESS_LPS331AP_ODR_AVL_1HZ_VAL	0x01
> > +#define ST_PRESS_LPS331AP_ODR_AVL_7HZ_VAL	0x05
> > +#define ST_PRESS_LPS331AP_ODR_AVL_13HZ_VAL	0x06
> > +#define ST_PRESS_LPS331AP_ODR_AVL_25HZ_VAL	0x07
> > +#define ST_PRESS_LPS331AP_PW_ADDR		0x20
> > +#define ST_PRESS_LPS331AP_PW_MASK		0x80
> > +#define ST_PRESS_LPS331AP_FS_ADDR		0x23
> > +#define ST_PRESS_LPS331AP_FS_MASK		0x30
> > +#define ST_PRESS_LPS331AP_FS_AVL_1260_VAL	0x00
> > +#define ST_PRESS_LPS331AP_FS_AVL_1260_GAIN	ST_PRESS_MBAR_TO_KPASCAL(244141)
> > +#define ST_PRESS_LPS331AP_FS_AVL_TEMP_GAIN	2083000
> > +#define ST_PRESS_LPS331AP_BDU_ADDR		0x20
> > +#define ST_PRESS_LPS331AP_BDU_MASK		0x04
> > +#define ST_PRESS_LPS331AP_DRDY_IRQ_ADDR		0x22
> > +#define ST_PRESS_LPS331AP_DRDY_IRQ_MASK		0x04
> > +#define ST_PRESS_LPS331AP_MULTIREAD_BIT		true
> > +#define ST_PRESS_LPS331AP_TEMP_OFFSET		42500
> > +#define ST_PRESS_LPS331AP_OUT_XL_ADDR		0x28
> > +#define ST_TEMP_LPS331AP_OUT_L_ADDR		0x2b
> >   
> >   static const struct iio_chan_spec st_press_channels[] = {
> >   	ST_SENSORS_LSM_CHANNELS(IIO_PRESSURE,
> >   			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> >   			ST_SENSORS_SCAN_X, 0, IIO_NO_MOD, 'u', IIO_LE, 24, 24,
> > -			ST_PRESS_DEFAULT_OUT_XL_ADDR),
> > +			ST_PRESS_LPS331AP_OUT_XL_ADDR),
> >   	ST_SENSORS_LSM_CHANNELS(IIO_TEMP,
> >   			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE) |
> >   						BIT(IIO_CHAN_INFO_OFFSET),
> >   			-1, 0, IIO_NO_MOD, 's', IIO_LE, 16, 16,
> > -			ST_TEMP_DEFAULT_OUT_L_ADDR),
> > +			ST_TEMP_LPS331AP_OUT_L_ADDR),
> >   	IIO_CHAN_SOFT_TIMESTAMP(1)
> >   };
> >   
> >   static const struct st_sensors st_press_sensors[] = {
> >   	{
> > -		.wai = ST_PRESS_1_WAI_EXP,
> > +		.wai = ST_PRESS_LPS331AP_WAI_EXP,
> >   		.sensors_supported = {
> >   			[0] = LPS331AP_PRESS_DEV_NAME,
> >   		},
> >   		.ch = (struct iio_chan_spec *)st_press_channels,
> >   		.odr = {
> > -			.addr = ST_PRESS_1_ODR_ADDR,
> > -			.mask = ST_PRESS_1_ODR_MASK,
> > +			.addr = ST_PRESS_LPS331AP_ODR_ADDR,
> > +			.mask = ST_PRESS_LPS331AP_ODR_MASK,
> >   			.odr_avl = {
> > -				{ 1, ST_PRESS_1_ODR_AVL_1HZ_VAL, },
> > -				{ 7, ST_PRESS_1_ODR_AVL_7HZ_VAL, },
> > -				{ 13, ST_PRESS_1_ODR_AVL_13HZ_VAL, },
> > -				{ 25, ST_PRESS_1_ODR_AVL_25HZ_VAL, },
> > +				{ 1, ST_PRESS_LPS331AP_ODR_AVL_1HZ_VAL, },
> > +				{ 7, ST_PRESS_LPS331AP_ODR_AVL_7HZ_VAL, },
> > +				{ 13, ST_PRESS_LPS331AP_ODR_AVL_13HZ_VAL, },
> > +				{ 25, ST_PRESS_LPS331AP_ODR_AVL_25HZ_VAL, },
> >   			},
> >   		},
> >   		.pw = {
> > -			.addr = ST_PRESS_1_PW_ADDR,
> > -			.mask = ST_PRESS_1_PW_MASK,
> > +			.addr = ST_PRESS_LPS331AP_PW_ADDR,
> > +			.mask = ST_PRESS_LPS331AP_PW_MASK,
> >   			.value_on = ST_SENSORS_DEFAULT_POWER_ON_VALUE,
> >   			.value_off = ST_SENSORS_DEFAULT_POWER_OFF_VALUE,
> >   		},
> >   		.fs = {
> > -			.addr = ST_PRESS_1_FS_ADDR,
> > -			.mask = ST_PRESS_1_FS_MASK,
> > +			.addr = ST_PRESS_LPS331AP_FS_ADDR,
> > +			.mask = ST_PRESS_LPS331AP_FS_MASK,
> >   			.fs_avl = {
> >   				[0] = {
> >   					.num = ST_PRESS_FS_AVL_1260MB,
> > -					.value = ST_PRESS_1_FS_AVL_1260_VAL,
> > -					.gain = ST_PRESS_1_FS_AVL_1260_GAIN,
> > -					.gain2 = ST_PRESS_1_FS_AVL_TEMP_GAIN,
> > +					.value = ST_PRESS_LPS331AP_FS_AVL_1260_VAL,
> > +					.gain = ST_PRESS_LPS331AP_FS_AVL_1260_GAIN,
> > +					.gain2 = ST_PRESS_LPS331AP_FS_AVL_TEMP_GAIN,
> >   				},
> >   			},
> >   		},
> >   		.bdu = {
> > -			.addr = ST_PRESS_1_BDU_ADDR,
> > -			.mask = ST_PRESS_1_BDU_MASK,
> > +			.addr = ST_PRESS_LPS331AP_BDU_ADDR,
> > +			.mask = ST_PRESS_LPS331AP_BDU_MASK,
> >   		},
> >   		.drdy_irq = {
> > -			.addr = ST_PRESS_1_DRDY_IRQ_ADDR,
> > -			.mask = ST_PRESS_1_DRDY_IRQ_MASK,
> > +			.addr = ST_PRESS_LPS331AP_DRDY_IRQ_ADDR,
> > +			.mask = ST_PRESS_LPS331AP_DRDY_IRQ_MASK,
> >   		},
> > -		.multi_read_bit = ST_PRESS_1_MULTIREAD_BIT,
> > +		.multi_read_bit = ST_PRESS_LPS331AP_MULTIREAD_BIT,
> >   		.bootime = 2,
> >   	},
> >   };

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 06/11] iio: pressure-core: st: Expand and rename LPS331AP's channel descriptor

[Lee Jones]

On Thu, 05 Sep 2013, Denis CIOCCA wrote:

> 
> >>> Due to the MACRO used, the task of reading, understanding and maintaining
> >>> the LPS331AP's channel descriptor is substantially difficult. This patch
> >>> is based on the view that it's better to have easy to read, maintainable
> >>> code than to save a few lines here and there. For that reason we're
> >>> expanding the array so initialisation is completed in full.
> >> Also for this one, the channel names are general and can be shared
> >> between different sensors. For the channel definition it's not a problem
> >> for me, but I think it's not necessary adds all that code...
> > I'm not sure what you mean by this. Would you be kind enough to
> > explain it in a different way please?
> The channel name (in this case st_press_channels) is not only specific 
> for one sensor but can be shared. Ok in this driver now is used only for 
> the lps331ap but for example in accelerometer driver is used by several 
> sensors. It's possible in the future for new pressure sensors use the 
> same channels definition.

Ah yes I see what you mean. Well as you say, for the moment, as
they're separated, this naming convention seems the most
appropriate. If we add anymore devices which share the definition, we
can pick the best naming convention for the situation I think. For
instance, I like that you've split the channels up into the number of
bits they support in the gyro and accel cases, so something of that
nature could be utilised if other device support is added.

> The channel definition is intended the switch by macro 
> ST_SENSORS_LSM_CHANNELS to the full definition, for me is not a problem 
> but I think it's not necessary.

If you are familiar with the macro I guess you could get used to
working with it, but coming from in as a first time reader, adding a
new device was pretty difficult. I had to look up the macro in the
header file, then have the original struct open too and cross
reference in 3 different places. It's made even more difficult by the
macro being in a different order to the original struct.

Now I've had time to work with it, I could probably work with it as
well. I was just thinking about helping out any new person that comes
along and tries to add support for a new sensor.

> >>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> >>> ---
> >>>    drivers/iio/pressure/st_pressure_core.c | 45 +++++++++++++++++++++++++--------
> >>>    1 file changed, 34 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
> >>> index becfb25..7ba9299 100644
> >>> --- a/drivers/iio/pressure/st_pressure_core.c
> >>> +++ b/drivers/iio/pressure/st_pressure_core.c
> >>> @@ -58,16 +58,39 @@
> >>>    #define ST_PRESS_LPS331AP_OUT_XL_ADDR		0x28
> >>>    #define ST_TEMP_LPS331AP_OUT_L_ADDR		0x2b
> >>>    
> >>> -static const struct iio_chan_spec st_press_channels[] = {
> >>> -	ST_SENSORS_LSM_CHANNELS(IIO_PRESSURE,
> >>> +static const struct iio_chan_spec st_press_lsp331ap_channels[] = {
> >>> +	{
> >>> +		.type = IIO_PRESSURE,
> >>> +		.channel2 = IIO_NO_MOD,
> >>> +		.address = ST_PRESS_LPS331AP_OUT_XL_ADDR,
> >>> +		.scan_index = ST_SENSORS_SCAN_X,
> >>> +		.scan_type = {
> >>> +			.sign = 'u',
> >>> +			.realbits = 24,
> >>> +			.storagebits = 24,
> >>> +			.endianness = IIO_LE,
> >>> +		},
> >>> +		.info_mask_separate =
> >>>    			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> >>> -			ST_SENSORS_SCAN_X, 0, IIO_NO_MOD, 'u', IIO_LE, 24, 24,
> >>> -			ST_PRESS_LPS331AP_OUT_XL_ADDR),
> >>> -	ST_SENSORS_LSM_CHANNELS(IIO_TEMP,
> >>> -			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE) |
> >>> -						BIT(IIO_CHAN_INFO_OFFSET),
> >>> -			-1, 0, IIO_NO_MOD, 's', IIO_LE, 16, 16,
> >>> -			ST_TEMP_LPS331AP_OUT_L_ADDR),
> >>> +		.modified = 0,
> >>> +	},
> >>> +	{
> >>> +		.type = IIO_TEMP,
> >>> +		.channel2 = IIO_NO_MOD,
> >>> +		.address = ST_TEMP_LPS331AP_OUT_L_ADDR,
> >>> +		.scan_index = -1,
> >>> +		.scan_type = {
> >>> +			.sign = 'u',
> >>> +			.realbits = 16,
> >>> +			.storagebits = 16,
> >>> +			.endianness = IIO_LE,
> >>> +		},
> >>> +		.info_mask_separate =
> >>> +			BIT(IIO_CHAN_INFO_RAW) |
> >>> +			BIT(IIO_CHAN_INFO_SCALE) |
> >>> +			BIT(IIO_CHAN_INFO_OFFSET),
> >>> +		.modified = 0,
> >>> +	},
> >>>    	IIO_CHAN_SOFT_TIMESTAMP(1)
> >>>    };
> >>>    
> >>> @@ -77,7 +100,7 @@ static const struct st_sensors st_press_sensors[] = {
> >>>    		.sensors_supported = {
> >>>    			[0] = LPS331AP_PRESS_DEV_NAME,
> >>>    		},
> >>> -		.ch = (struct iio_chan_spec *)st_press_channels,
> >>> +		.ch = (struct iio_chan_spec *)st_press_lsp331ap_channels,
> >>>    		.odr = {
> >>>    			.addr = ST_PRESS_LPS331AP_ODR_ADDR,
> >>>    			.mask = ST_PRESS_LPS331AP_ODR_MASK,
> >>> @@ -214,7 +237,7 @@ int st_press_common_probe(struct iio_dev *indio_dev)
> >>>    	pdata->num_data_channels = ST_PRESS_NUMBER_DATA_CHANNELS;
> >>>    	pdata->multiread_bit = pdata->sensor->multi_read_bit;
> >>>    	indio_dev->channels = pdata->sensor->ch;
> >>> -	indio_dev->num_channels = ARRAY_SIZE(st_press_channels);
> >>> +	indio_dev->num_channels = ARRAY_SIZE(st_press_lsp331ap_channels);
> >>>    
> >>>    	pdata->current_fullscale = (struct st_sensor_fullscale_avl *)
> >>>    						&pdata->sensor->fs.fs_avl[0];

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 06/11] iio: pressure-core: st: Expand and rename LPS331AP's channel descriptor

[Denis CIOCCA]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 5572 bytes --]

Lee, I got your point. For me is ok...

Denis
> On Thu, 05 Sep 2013, Denis CIOCCA wrote:
>
>>>>> Due to the MACRO used, the task of reading, understanding and maintaining
>>>>> the LPS331AP's channel descriptor is substantially difficult. This patch
>>>>> is based on the view that it's better to have easy to read, maintainable
>>>>> code than to save a few lines here and there. For that reason we're
>>>>> expanding the array so initialisation is completed in full.
>>>> Also for this one, the channel names are general and can be shared
>>>> between different sensors. For the channel definition it's not a problem
>>>> for me, but I think it's not necessary adds all that code...
>>> I'm not sure what you mean by this. Would you be kind enough to
>>> explain it in a different way please?
>> The channel name (in this case st_press_channels) is not only specific
>> for one sensor but can be shared. Ok in this driver now is used only for
>> the lps331ap but for example in accelerometer driver is used by several
>> sensors. It's possible in the future for new pressure sensors use the
>> same channels definition.
> Ah yes I see what you mean. Well as you say, for the moment, as
> they're separated, this naming convention seems the most
> appropriate. If we add anymore devices which share the definition, we
> can pick the best naming convention for the situation I think. For
> instance, I like that you've split the channels up into the number of
> bits they support in the gyro and accel cases, so something of that
> nature could be utilised if other device support is added.
>
>> The channel definition is intended the switch by macro
>> ST_SENSORS_LSM_CHANNELS to the full definition, for me is not a problem
>> but I think it's not necessary.
> If you are familiar with the macro I guess you could get used to
> working with it, but coming from in as a first time reader, adding a
> new device was pretty difficult. I had to look up the macro in the
> header file, then have the original struct open too and cross
> reference in 3 different places. It's made even more difficult by the
> macro being in a different order to the original struct.
>
> Now I've had time to work with it, I could probably work with it as
> well. I was just thinking about helping out any new person that comes
> along and tries to add support for a new sensor.
>
>>>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>>>>> ---
>>>>>     drivers/iio/pressure/st_pressure_core.c | 45 +++++++++++++++++++++++++--------
>>>>>     1 file changed, 34 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
>>>>> index becfb25..7ba9299 100644
>>>>> --- a/drivers/iio/pressure/st_pressure_core.c
>>>>> +++ b/drivers/iio/pressure/st_pressure_core.c
>>>>> @@ -58,16 +58,39 @@
>>>>>     #define ST_PRESS_LPS331AP_OUT_XL_ADDR		0x28
>>>>>     #define ST_TEMP_LPS331AP_OUT_L_ADDR		0x2b
>>>>>     
>>>>> -static const struct iio_chan_spec st_press_channels[] = {
>>>>> -	ST_SENSORS_LSM_CHANNELS(IIO_PRESSURE,
>>>>> +static const struct iio_chan_spec st_press_lsp331ap_channels[] = {
>>>>> +	{
>>>>> +		.type = IIO_PRESSURE,
>>>>> +		.channel2 = IIO_NO_MOD,
>>>>> +		.address = ST_PRESS_LPS331AP_OUT_XL_ADDR,
>>>>> +		.scan_index = ST_SENSORS_SCAN_X,
>>>>> +		.scan_type = {
>>>>> +			.sign = 'u',
>>>>> +			.realbits = 24,
>>>>> +			.storagebits = 24,
>>>>> +			.endianness = IIO_LE,
>>>>> +		},
>>>>> +		.info_mask_separate =
>>>>>     			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
>>>>> -			ST_SENSORS_SCAN_X, 0, IIO_NO_MOD, 'u', IIO_LE, 24, 24,
>>>>> -			ST_PRESS_LPS331AP_OUT_XL_ADDR),
>>>>> -	ST_SENSORS_LSM_CHANNELS(IIO_TEMP,
>>>>> -			BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE) |
>>>>> -						BIT(IIO_CHAN_INFO_OFFSET),
>>>>> -			-1, 0, IIO_NO_MOD, 's', IIO_LE, 16, 16,
>>>>> -			ST_TEMP_LPS331AP_OUT_L_ADDR),
>>>>> +		.modified = 0,
>>>>> +	},
>>>>> +	{
>>>>> +		.type = IIO_TEMP,
>>>>> +		.channel2 = IIO_NO_MOD,
>>>>> +		.address = ST_TEMP_LPS331AP_OUT_L_ADDR,
>>>>> +		.scan_index = -1,
>>>>> +		.scan_type = {
>>>>> +			.sign = 'u',
>>>>> +			.realbits = 16,
>>>>> +			.storagebits = 16,
>>>>> +			.endianness = IIO_LE,
>>>>> +		},
>>>>> +		.info_mask_separate =
>>>>> +			BIT(IIO_CHAN_INFO_RAW) |
>>>>> +			BIT(IIO_CHAN_INFO_SCALE) |
>>>>> +			BIT(IIO_CHAN_INFO_OFFSET),
>>>>> +		.modified = 0,
>>>>> +	},
>>>>>     	IIO_CHAN_SOFT_TIMESTAMP(1)
>>>>>     };
>>>>>     
>>>>> @@ -77,7 +100,7 @@ static const struct st_sensors st_press_sensors[] = {
>>>>>     		.sensors_supported = {
>>>>>     			[0] = LPS331AP_PRESS_DEV_NAME,
>>>>>     		},
>>>>> -		.ch = (struct iio_chan_spec *)st_press_channels,
>>>>> +		.ch = (struct iio_chan_spec *)st_press_lsp331ap_channels,
>>>>>     		.odr = {
>>>>>     			.addr = ST_PRESS_LPS331AP_ODR_ADDR,
>>>>>     			.mask = ST_PRESS_LPS331AP_ODR_MASK,
>>>>> @@ -214,7 +237,7 @@ int st_press_common_probe(struct iio_dev *indio_dev)
>>>>>     	pdata->num_data_channels = ST_PRESS_NUMBER_DATA_CHANNELS;
>>>>>     	pdata->multiread_bit = pdata->sensor->multi_read_bit;
>>>>>     	indio_dev->channels = pdata->sensor->ch;
>>>>> -	indio_dev->num_channels = ARRAY_SIZE(st_press_channels);
>>>>> +	indio_dev->num_channels = ARRAY_SIZE(st_press_lsp331ap_channels);
>>>>>     
>>>>>     	pdata->current_fullscale = (struct st_sensor_fullscale_avl *)
>>>>>     						&pdata->sensor->fs.fs_avl[0];
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥