Hi Jonathan,
Sorry for the continued development and resubmission. I won't write
anymore patches now. At least until these have been accepted.
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) and extended support for
a new magnetometer sensor (LSM303DLH).
Everything has been tested with Device Tree.
v2:
- Rebased onto Linux -next as requested by Jonathan
- Reworked clean-up patch to address Jonathan's concerns
- Added binding document (inc regulators)
- Extended regulator support in the driver
- Re-worked Data Ready pin handling
v3:
- Standardise sensor names: <model>_<sensor-type>
- Added error handling clean-ups
- More binding documents; for magn, accel and gyro
- Message on successful sensor bring-up; for press, magn, accel and gyro
- More device enablement for ux500 by way of CONFIGs
- New neat reorder/grouping of patches
Documentation/devicetree/bindings/iio/accel/lsm303dlh.txt | 21 ++++++++
Documentation/devicetree/bindings/iio/gyro/l3g4200d.txt | 21 ++++++++
Documentation/devicetree/bindings/iio/magnetometer/lsm303dlh.txt | 21 ++++++++
Documentation/devicetree/bindings/iio/pressure/lps001wp.txt | 21 ++++++++
arch/arm/boot/dts/ste-dbx5x0.dtsi | 5 --
arch/arm/boot/dts/ste-snowball.dts | 34 ++++++++++++
arch/arm/configs/u8500_defconfig | 5 ++
drivers/iio/accel/st_accel.h | 4 +-
drivers/iio/accel/st_accel_core.c | 28 +++++-----
drivers/iio/common/st_sensors/st_sensors_core.c | 79 +++++++++++++++-------------
drivers/iio/gyro/st_gyro.h | 8 +--
drivers/iio/gyro/st_gyro_core.c | 24 +++++----
drivers/iio/magnetometer/st_magn.h | 3 +-
drivers/iio/magnetometer/st_magn_core.c | 27 +++++-----
drivers/iio/pressure/st_pressure.h | 3 +-
drivers/iio/pressure/st_pressure_core.c | 303 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------
drivers/iio/pressure/st_pressure_i2c.c | 1 +
include/linux/iio/common/st_sensors.h | 6 +++
18 files changed, 453 insertions(+), 161 deletions(-)
After applying this node the LSM303DLH sensor chip should probe
successfully once the driver support has also been applied.
Signed-off-by: Lee Jones <[email protected]>
---
arch/arm/boot/dts/ste-snowball.dts | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/arm/boot/dts/ste-snowball.dts b/arch/arm/boot/dts/ste-snowball.dts
index d355d25..d1f4c60 100644
--- a/arch/arm/boot/dts/ste-snowball.dts
+++ b/arch/arm/boot/dts/ste-snowball.dts
@@ -161,6 +161,14 @@
vdd-supply = <&ab8500_ldo_aux1_reg>;
vddio-supply = <&db8500_vsmps2_reg>;
};
+
+ lsm303dlh_accel@19 {
+ compatible = "st,lsm303dlh_accel";
+ reg = <0x19>;
+
+ vdd-supply = <&ab8500_ldo_aux1_reg>;
+ vddio-supply = <&db8500_vsmps2_reg>;
+ };
};
uart@80120000 {
--
1.8.1.2
Signed-off-by: Lee Jones <[email protected]>
---
arch/arm/configs/u8500_defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/configs/u8500_defconfig b/arch/arm/configs/u8500_defconfig
index 4b94b42..aaa6c0a 100644
--- a/arch/arm/configs/u8500_defconfig
+++ b/arch/arm/configs/u8500_defconfig
@@ -41,6 +41,7 @@ CONFIG_IIO=y
CONFIG_IIO_ST_PRESS=y
CONFIG_IIO_ST_ACCEL_3AXIS=y
CONFIG_IIO_ST_MAGN_3AXIS=y
+CONFIG_IIO_ST_GYRO_3AXIS=y
CONFIG_NETDEVICES=y
CONFIG_SMSC911X=y
CONFIG_SMSC_PHY=y
--
1.8.1.2
LSM303DLH is a Accelerometer Sensor
Signed-off-by: Lee Jones <[email protected]>
---
.../devicetree/bindings/iio/accel/lsm303dlh.txt | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/accel/lsm303dlh.txt
diff --git a/Documentation/devicetree/bindings/iio/accel/lsm303dlh.txt b/Documentation/devicetree/bindings/iio/accel/lsm303dlh.txt
new file mode 100644
index 0000000..bb59363
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/accel/lsm303dlh.txt
@@ -0,0 +1,21 @@
+* STMicroelectronics Accelerometer Sensor
+
+Required properties:
+ - compatible: Should be "st,lsm303dlh_accel"
+ - reg: The I2C address of the sensor
+
+Optional properties:
+ - vdd-supply: Phandle to the Vdd supply regulator
+ - vddio-supply: Phandle to the Vdd-IO supply regulator
+
+Example:
+
+i2c@80128000 {
+ lsm303dlh_accel@19 {
+ compatible = "st,lsm303dlh_accel";
+ reg = <0x19>;
+
+ vdd-supply = <&ab8500_ldo_aux1_reg>;
+ vddio-supply = <&db8500_vsmps2_reg>;
+ };
+};
--
1.8.1.2
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 <[email protected]>
---
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 60c2ee4..3abada2 100644
--- a/drivers/iio/pressure/st_pressure_core.c
+++ b/drivers/iio/pressure/st_pressure_core.c
@@ -107,6 +107,7 @@ static const struct st_sensors st_press_sensors[] = {
[0] = LPS331AP_PRESS_DEV_NAME,
},
.ch = (struct iio_chan_spec *)st_press_lps331ap_channels,
+ .num_ch = ARRAY_SIZE(st_press_lps331ap_channels),
.odr = {
.addr = ST_PRESS_LPS331AP_ODR_ADDR,
.mask = ST_PRESS_LPS331AP_ODR_MASK,
@@ -245,7 +246,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_lps331ap_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 e51f654..e732fda 100644
--- a/include/linux/iio/common/st_sensors.h
+++ b/include/linux/iio/common/st_sensors.h
@@ -184,6 +184,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
Trivial patch adding the LSM303DLH to the list of already supported
Magnetometer Sensors.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/iio/magnetometer/st_magn.h | 1 +
drivers/iio/magnetometer/st_magn_core.c | 3 ++-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/magnetometer/st_magn.h b/drivers/iio/magnetometer/st_magn.h
index f185720..19b53e3 100644
--- a/drivers/iio/magnetometer/st_magn.h
+++ b/drivers/iio/magnetometer/st_magn.h
@@ -15,6 +15,7 @@
#include <linux/iio/common/st_sensors.h>
#define LSM303DLHC_MAGN_DEV_NAME "lsm303dlhc_magn"
+#define LSM303DLH_MAGN_DEV_NAME "lsm303dlh_magn"
#define LSM303DLM_MAGN_DEV_NAME "lsm303dlm_magn"
#define LIS3MDL_MAGN_DEV_NAME "lis3mdl_magn"
diff --git a/drivers/iio/magnetometer/st_magn_core.c b/drivers/iio/magnetometer/st_magn_core.c
index 12e7e79..b2e2917 100644
--- a/drivers/iio/magnetometer/st_magn_core.c
+++ b/drivers/iio/magnetometer/st_magn_core.c
@@ -151,7 +151,8 @@ static const struct st_sensors st_magn_sensors[] = {
.wai = ST_MAGN_1_WAI_EXP,
.sensors_supported = {
[0] = LSM303DLHC_MAGN_DEV_NAME,
- [1] = LSM303DLM_MAGN_DEV_NAME,
+ [1] = LSM303DLH_MAGN_DEV_NAME,
+ [0] = LSM303DLM_MAGN_DEV_NAME,
},
.ch = (struct iio_chan_spec *)st_magn_16bit_channels,
.odr = {
--
1.8.1.2
At the moment the driver is silent in some error cases and if successful.
Prior to this patch there was no clear way to know if the driver succeeded
or not without looking deep into sysfs.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/iio/magnetometer/st_magn_core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/iio/magnetometer/st_magn_core.c b/drivers/iio/magnetometer/st_magn_core.c
index c252659..12e7e79 100644
--- a/drivers/iio/magnetometer/st_magn_core.c
+++ b/drivers/iio/magnetometer/st_magn_core.c
@@ -386,6 +386,9 @@ int st_magn_common_probe(struct iio_dev *indio_dev,
if (err && irq > 0)
goto st_magn_device_register_error;
+ if (!err)
+ dev_info(&indio_dev->dev, "Successfully registered\n");
+
return err;
st_magn_device_register_error:
--
1.8.1.2
Reduce the amount of those unnecessary goto calls, as in most cases
we can simply return immediately. We also only call for the IRQ number
once and use that value throughout.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/iio/magnetometer/st_magn_core.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/iio/magnetometer/st_magn_core.c b/drivers/iio/magnetometer/st_magn_core.c
index e8d2849..c252659 100644
--- a/drivers/iio/magnetometer/st_magn_core.c
+++ b/drivers/iio/magnetometer/st_magn_core.c
@@ -348,8 +348,9 @@ static const struct iio_info magn_info = {
int st_magn_common_probe(struct iio_dev *indio_dev,
struct st_sensors_platform_data *pdata)
{
- int err;
struct st_sensor_data *mdata = iio_priv(indio_dev);
+ int irq = mdata->get_irq_data_ready(indio_dev);
+ int err;
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->info = &magn_info;
@@ -357,7 +358,7 @@ int st_magn_common_probe(struct iio_dev *indio_dev,
err = st_sensors_check_device_support(indio_dev,
ARRAY_SIZE(st_magn_sensors), st_magn_sensors);
if (err < 0)
- goto st_magn_common_probe_error;
+ return err;
mdata->num_data_channels = ST_MAGN_NUMBER_DATA_CHANNELS;
mdata->multiread_bit = mdata->sensor->multi_read_bit;
@@ -370,30 +371,28 @@ int st_magn_common_probe(struct iio_dev *indio_dev,
err = st_sensors_init_sensor(indio_dev, pdata);
if (err < 0)
- goto st_magn_common_probe_error;
+ return err;
- if (mdata->get_irq_data_ready(indio_dev) > 0) {
+ if (irq > 0) {
err = st_magn_allocate_ring(indio_dev);
if (err < 0)
- goto st_magn_common_probe_error;
+ return err;
err = st_sensors_allocate_trigger(indio_dev, NULL);
if (err < 0)
goto st_magn_probe_trigger_error;
}
err = iio_device_register(indio_dev);
- if (err)
+ if (err && irq > 0)
goto st_magn_device_register_error;
return err;
st_magn_device_register_error:
- if (mdata->get_irq_data_ready(indio_dev) > 0)
- st_sensors_deallocate_trigger(indio_dev);
+ st_sensors_deallocate_trigger(indio_dev);
st_magn_probe_trigger_error:
- if (mdata->get_irq_data_ready(indio_dev) > 0)
- st_magn_deallocate_ring(indio_dev);
-st_magn_common_probe_error:
+ st_magn_deallocate_ring(indio_dev);
+
return err;
}
EXPORT_SYMBOL(st_magn_common_probe);
--
1.8.1.2
Reduce the amount of those unnecessary goto calls, as in most cases
we can simply return immediately. We also only call for the IRQ number
once and use that value throughout.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/iio/gyro/st_gyro_core.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/iio/gyro/st_gyro_core.c b/drivers/iio/gyro/st_gyro_core.c
index e13c2b0..09e24f9 100644
--- a/drivers/iio/gyro/st_gyro_core.c
+++ b/drivers/iio/gyro/st_gyro_core.c
@@ -305,8 +305,9 @@ static const struct iio_trigger_ops st_gyro_trigger_ops = {
int st_gyro_common_probe(struct iio_dev *indio_dev,
struct st_sensors_platform_data *pdata)
{
- int err;
struct st_sensor_data *gdata = iio_priv(indio_dev);
+ int irq = gdata->get_irq_data_ready(indio_dev);
+ int err;
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->info = &gyro_info;
@@ -314,7 +315,7 @@ int st_gyro_common_probe(struct iio_dev *indio_dev,
err = st_sensors_check_device_support(indio_dev,
ARRAY_SIZE(st_gyro_sensors), st_gyro_sensors);
if (err < 0)
- goto st_gyro_common_probe_error;
+ return err;
gdata->num_data_channels = ST_GYRO_NUMBER_DATA_CHANNELS;
gdata->multiread_bit = gdata->sensor->multi_read_bit;
@@ -327,12 +328,12 @@ int st_gyro_common_probe(struct iio_dev *indio_dev,
err = st_sensors_init_sensor(indio_dev, pdata);
if (err < 0)
- goto st_gyro_common_probe_error;
+ return err;
- if (gdata->get_irq_data_ready(indio_dev) > 0) {
+ if (irq > 0) {
err = st_gyro_allocate_ring(indio_dev);
if (err < 0)
- goto st_gyro_common_probe_error;
+ return err;
err = st_sensors_allocate_trigger(indio_dev,
ST_GYRO_TRIGGER_OPS);
@@ -341,18 +342,16 @@ int st_gyro_common_probe(struct iio_dev *indio_dev,
}
err = iio_device_register(indio_dev);
- if (err)
+ if (err && irq > 0)
goto st_gyro_device_register_error;
return err;
st_gyro_device_register_error:
- if (gdata->get_irq_data_ready(indio_dev) > 0)
- st_sensors_deallocate_trigger(indio_dev);
+ st_sensors_deallocate_trigger(indio_dev);
st_gyro_probe_trigger_error:
- if (gdata->get_irq_data_ready(indio_dev) > 0)
- st_gyro_deallocate_ring(indio_dev);
-st_gyro_common_probe_error:
+ st_gyro_deallocate_ring(indio_dev);
+
return err;
}
EXPORT_SYMBOL(st_gyro_common_probe);
--
1.8.1.2
At the moment the driver is silent in some error cases and if successful.
Prior to this patch there was no clear way to know if the driver succeeded
or not without looking deep into sysfs.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/iio/gyro/st_gyro_core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/iio/gyro/st_gyro_core.c b/drivers/iio/gyro/st_gyro_core.c
index 09e24f9..e93b94c 100644
--- a/drivers/iio/gyro/st_gyro_core.c
+++ b/drivers/iio/gyro/st_gyro_core.c
@@ -345,6 +345,9 @@ int st_gyro_common_probe(struct iio_dev *indio_dev,
if (err && irq > 0)
goto st_gyro_device_register_error;
+ if (!err)
+ dev_info(&indio_dev->dev, "Successfully registered\n");
+
return err;
st_gyro_device_register_error:
--
1.8.1.2
At the moment the driver is silent in some error cases and if successful.
Prior to this patch there was no clear way to know if the driver succeeded
or not without looking deep into sysfs.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/iio/pressure/st_pressure_core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
index 6ffd949..34b3fb1 100644
--- a/drivers/iio/pressure/st_pressure_core.c
+++ b/drivers/iio/pressure/st_pressure_core.c
@@ -280,6 +280,9 @@ int st_press_common_probe(struct iio_dev *indio_dev,
if (err && irq > 0)
goto st_press_device_register_error;
+ if (!err)
+ dev_info(&indio_dev->dev, "Successfully registered\n");
+
return err;
st_press_device_register_error:
--
1.8.1.2
At the moment the driver is silent in some error cases and if successful.
Prior to this patch there was no clear way to know if the driver succeeded
or not without looking deep into sysfs.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/iio/accel/st_accel_core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
index 03a2b6b..a003f3b 100644
--- a/drivers/iio/accel/st_accel_core.c
+++ b/drivers/iio/accel/st_accel_core.c
@@ -496,6 +496,9 @@ int st_accel_common_probe(struct iio_dev *indio_dev,
if (err && irq > 0)
goto st_accel_device_register_error;
+ if (!err)
+ dev_info(&indio_dev->dev, "Successfully registered\n");
+
return err;
st_accel_device_register_error:
--
1.8.1.2
The LSM303DLH's WAI (WhoAmI) is 0x33, meaning it should be enabled by
Accel Sensor group one. For the device to probe without error, we'll
need to ensure it's registered with the correct WAI.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/iio/accel/st_accel_core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
index ea62291..03a2b6b 100644
--- a/drivers/iio/accel/st_accel_core.c
+++ b/drivers/iio/accel/st_accel_core.c
@@ -170,6 +170,7 @@ static const struct st_sensors st_accel_sensors[] = {
[2] = LSM330D_ACCEL_DEV_NAME,
[3] = LSM330DL_ACCEL_DEV_NAME,
[4] = LSM330DLC_ACCEL_DEV_NAME,
+ [5] = LSM303DLH_ACCEL_DEV_NAME,
},
.ch = (struct iio_chan_spec *)st_accel_12bit_channels,
.odr = {
@@ -238,8 +239,7 @@ static const struct st_sensors st_accel_sensors[] = {
.sensors_supported = {
[0] = LIS331DLH_ACCEL_DEV_NAME,
[1] = LSM303DL_ACCEL_DEV_NAME,
- [2] = LSM303DLH_ACCEL_DEV_NAME,
- [3] = LSM303DLM_ACCEL_DEV_NAME,
+ [2] = LSM303DLM_ACCEL_DEV_NAME,
},
.ch = (struct iio_chan_spec *)st_accel_12bit_channels,
.odr = {
--
1.8.1.2
Reduce the amount of those unnecessary goto calls, as in most cases
we can simply return immediately. We also only call for the IRQ number
once and use that value throughout.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/iio/accel/st_accel_core.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
index 1458343..ea62291 100644
--- a/drivers/iio/accel/st_accel_core.c
+++ b/drivers/iio/accel/st_accel_core.c
@@ -452,8 +452,9 @@ static const struct iio_trigger_ops st_accel_trigger_ops = {
int st_accel_common_probe(struct iio_dev *indio_dev,
struct st_sensors_platform_data *plat_data)
{
- int err;
struct st_sensor_data *adata = iio_priv(indio_dev);
+ int irq = adata->get_irq_data_ready(indio_dev);
+ int err;
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->info = &accel_info;
@@ -461,7 +462,7 @@ int st_accel_common_probe(struct iio_dev *indio_dev,
err = st_sensors_check_device_support(indio_dev,
ARRAY_SIZE(st_accel_sensors), st_accel_sensors);
if (err < 0)
- goto st_accel_common_probe_error;
+ return err;
adata->num_data_channels = ST_ACCEL_NUMBER_DATA_CHANNELS;
adata->multiread_bit = adata->sensor->multi_read_bit;
@@ -478,12 +479,12 @@ int st_accel_common_probe(struct iio_dev *indio_dev,
err = st_sensors_init_sensor(indio_dev, plat_data);
if (err < 0)
- goto st_accel_common_probe_error;
+ return err;
- if (adata->get_irq_data_ready(indio_dev) > 0) {
+ if (irq > 0) {
err = st_accel_allocate_ring(indio_dev);
if (err < 0)
- goto st_accel_common_probe_error;
+ return err;
err = st_sensors_allocate_trigger(indio_dev,
ST_ACCEL_TRIGGER_OPS);
@@ -492,18 +493,16 @@ int st_accel_common_probe(struct iio_dev *indio_dev,
}
err = iio_device_register(indio_dev);
- if (err)
+ if (err && irq > 0)
goto st_accel_device_register_error;
return err;
st_accel_device_register_error:
- if (adata->get_irq_data_ready(indio_dev) > 0)
- st_sensors_deallocate_trigger(indio_dev);
+ st_sensors_deallocate_trigger(indio_dev);
st_accel_probe_trigger_error:
- if (adata->get_irq_data_ready(indio_dev) > 0)
- st_accel_deallocate_ring(indio_dev);
-st_accel_common_probe_error:
+ st_accel_deallocate_ring(indio_dev);
+
return err;
}
EXPORT_SYMBOL(st_accel_common_probe);
--
1.8.1.2
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 <[email protected]>
---
drivers/iio/pressure/st_pressure_core.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
index 3abada2..6ffd949 100644
--- a/drivers/iio/pressure/st_pressure_core.c
+++ b/drivers/iio/pressure/st_pressure_core.c
@@ -232,21 +232,23 @@ static const struct iio_trigger_ops st_press_trigger_ops = {
int st_press_common_probe(struct iio_dev *indio_dev,
struct st_sensors_platform_data *plat_data)
{
- 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 *)
@@ -261,32 +263,30 @@ int st_press_common_probe(struct iio_dev *indio_dev,
err = st_sensors_init_sensor(indio_dev, plat_data);
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);
+ ST_PRESS_TRIGGER_OPS);
if (err < 0)
goto st_press_probe_trigger_error;
}
err = iio_device_register(indio_dev);
- if (err)
+ if (err && irq > 0)
goto st_press_device_register_error;
return err;
st_press_device_register_error:
- if (pdata->get_irq_data_ready(indio_dev) > 0)
- st_sensors_deallocate_trigger(indio_dev);
+ 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:
+ st_press_deallocate_ring(indio_dev);
+
return err;
}
EXPORT_SYMBOL(st_press_common_probe);
--
1.8.1.2
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. This patch allows for
the Vdd_IO power supply to be specified by either platform data or
Device Tree.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/iio/pressure/st_pressure_core.c | 13 ++++++++++++-
include/linux/iio/common/st_sensors.h | 2 ++
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
index d52b487..2db3b28 100644
--- a/drivers/iio/pressure/st_pressure_core.c
+++ b/drivers/iio/pressure/st_pressure_core.c
@@ -319,7 +319,7 @@ void st_press_power_enable(struct iio_dev *indio_dev)
struct st_sensor_data *pdata = iio_priv(indio_dev);
int err;
- /* Regulators not mandatory, but if requested we should enable it. */
+ /* Regulators not mandatory, but if requested we should enable them. */
pdata->vdd = devm_regulator_get_optional(&indio_dev->dev, "vdd");
if (!IS_ERR(pdata->vdd)) {
err = regulator_enable(pdata->vdd);
@@ -327,6 +327,14 @@ void st_press_power_enable(struct iio_dev *indio_dev)
dev_warn(&indio_dev->dev,
"Failed to enable specified Vdd supply\n");
}
+
+ pdata->vdd_io = devm_regulator_get_optional(&indio_dev->dev, "vddio");
+ if (!IS_ERR(pdata->vdd_io)) {
+ err = regulator_enable(pdata->vdd_io);
+ if (err != 0)
+ dev_warn(&indio_dev->dev,
+ "Failed to enable specified Vdd_IO supply\n");
+ }
}
void st_press_power_disable(struct iio_dev *indio_dev)
@@ -335,6 +343,9 @@ void st_press_power_disable(struct iio_dev *indio_dev)
if (!IS_ERR(pdata->vdd))
regulator_disable(pdata->vdd);
+
+ if (!IS_ERR(pdata->vdd_io))
+ regulator_disable(pdata->vdd_io);
}
int st_press_common_probe(struct iio_dev *indio_dev,
diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
index 968b84e..3c005eb 100644
--- a/include/linux/iio/common/st_sensors.h
+++ b/include/linux/iio/common/st_sensors.h
@@ -203,6 +203,7 @@ struct st_sensors {
* @sensor: Pointer to the current sensor struct in use.
* @current_fullscale: Maximum range of measure by the sensor.
* @vdd: Pointer to sensor's Vdd power supply
+ * @vdd_io: Pointer to sensor's Vdd-IO 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.
@@ -219,6 +220,7 @@ struct st_sensor_data {
struct st_sensors *sensor;
struct st_sensor_fullscale_avl *current_fullscale;
struct regulator *vdd;
+ struct regulator *vdd_io;
bool enabled;
bool multiread_bit;
--
1.8.1.2
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. This patch allows for
the Vdd power supply to be specified by either platform data or Device
Tree.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/iio/pressure/st_pressure_core.c | 28 ++++++++++++++++++++++++++++
include/linux/iio/common/st_sensors.h | 3 +++
2 files changed, 31 insertions(+)
diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
index b42614a..d52b487 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>
@@ -313,6 +314,29 @@ static const struct iio_trigger_ops st_press_trigger_ops = {
#define ST_PRESS_TRIGGER_OPS NULL
#endif
+void st_press_power_enable(struct iio_dev *indio_dev)
+{
+ struct st_sensor_data *pdata = iio_priv(indio_dev);
+ int err;
+
+ /* Regulators not mandatory, but if requested we should enable it. */
+ pdata->vdd = devm_regulator_get_optional(&indio_dev->dev, "vdd");
+ if (!IS_ERR(pdata->vdd)) {
+ err = regulator_enable(pdata->vdd);
+ if (err != 0)
+ dev_warn(&indio_dev->dev,
+ "Failed to enable specified Vdd supply\n");
+ }
+}
+
+void st_press_power_disable(struct iio_dev *indio_dev)
+{
+ struct st_sensor_data *pdata = iio_priv(indio_dev);
+
+ if (!IS_ERR(pdata->vdd))
+ regulator_disable(pdata->vdd);
+}
+
int st_press_common_probe(struct iio_dev *indio_dev,
struct st_sensors_platform_data *plat_data)
{
@@ -323,6 +347,8 @@ int st_press_common_probe(struct iio_dev *indio_dev,
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->info = &press_info;
+ st_press_power_enable(indio_dev);
+
err = st_sensors_check_device_support(indio_dev,
ARRAY_SIZE(st_press_sensors),
st_press_sensors);
@@ -382,6 +408,8 @@ void st_press_common_remove(struct iio_dev *indio_dev)
{
struct st_sensor_data *pdata = iio_priv(indio_dev);
+ st_press_power_disable(indio_dev);
+
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 e732fda..968b84e 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>
#include <linux/platform_data/st_sensors_pdata.h>
@@ -201,6 +202,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.
+ * @vdd: Pointer to sensor's Vdd 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.
@@ -216,6 +218,7 @@ struct st_sensor_data {
struct iio_trigger *trig;
struct st_sensors *sensor;
struct st_sensor_fullscale_avl *current_fullscale;
+ struct regulator *vdd;
bool enabled;
bool multiread_bit;
--
1.8.1.2
Here we use existing practices to introduce support for another
pressure/temperature sensor, the LPS001WP.
Signed-off-by: Lee Jones <[email protected]>
---
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 f1bebce..36b1cc6 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_press"
+#define LPS001WP_PRESS_DEV_NAME "lps001wp_press"
/**
* struct st_sensors_platform_data - default press platform data
diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
index 34b3fb1..b42614a 100644
--- a/drivers/iio/pressure/st_pressure_core.c
+++ b/drivers/iio/pressure/st_pressure_core.c
@@ -64,6 +64,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_lps331ap_channels[] = {
{
.type = IIO_PRESSURE,
@@ -100,6 +115,40 @@ static const struct iio_chan_spec st_press_lps331ap_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,
@@ -148,6 +197,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 08aac5e..ada5cbd 100644
--- a/drivers/iio/pressure/st_pressure_i2c.c
+++ b/drivers/iio/pressure/st_pressure_i2c.c
@@ -50,6 +50,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
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 <[email protected]>
---
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 965ee22..eb261a5 100644
--- a/drivers/iio/common/st_sensors/st_sensors_core.c
+++ b/drivers/iio/common/st_sensors/st_sensors_core.c
@@ -235,10 +235,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 ceebd3c..16cfbc5 100644
--- a/drivers/iio/pressure/st_pressure_core.c
+++ b/drivers/iio/pressure/st_pressure_core.c
@@ -226,8 +226,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_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;
if (!plat_data)
--
1.8.1.2
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 <[email protected]>
---
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 2ee4bcd..60c2ee4 100644
--- a/drivers/iio/pressure/st_pressure_core.c
+++ b/drivers/iio/pressure/st_pressure_core.c
@@ -64,16 +64,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_lps331ap_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)
};
@@ -83,7 +106,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_lps331ap_channels,
.odr = {
.addr = ST_PRESS_LPS331AP_ODR_ADDR,
.mask = ST_PRESS_LPS331AP_ODR_MASK,
@@ -222,7 +245,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_lps331ap_channels);
if (pdata->sensor->fs.addr != 0)
pdata->current_fullscale = (struct st_sensor_fullscale_avl *)
--
1.8.1.2
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 <[email protected]>
---
drivers/iio/pressure/st_pressure_core.c | 94 ++++++++++++++++-----------------
1 file changed, 46 insertions(+), 48 deletions(-)
diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
index 279aafd..2ee4bcd 100644
--- a/drivers/iio/pressure/st_pressure_core.c
+++ b/drivers/iio/pressure/st_pressure_core.c
@@ -36,94 +36,92 @@
ST_PRESS_LSB_PER_CELSIUS)
#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_KPASCAL_NANO_SCALE
-#define ST_PRESS_1_FS_AVL_TEMP_GAIN ST_PRESS_CELSIUS_NANO_SCALE
-#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_INT1_MASK 0x04
-#define ST_PRESS_1_DRDY_IRQ_INT2_MASK 0x20
-#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_KPASCAL_NANO_SCALE
+#define ST_PRESS_LPS331AP_FS_AVL_TEMP_GAIN ST_PRESS_CELSIUS_NANO_SCALE
+#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_INT1_MASK 0x04
+#define ST_PRESS_LPS331AP_DRDY_IRQ_INT2_MASK 0x20
+#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_int1 = ST_PRESS_1_DRDY_IRQ_INT1_MASK,
- .mask_int2 = ST_PRESS_1_DRDY_IRQ_INT2_MASK,
+ .addr = ST_PRESS_LPS331AP_DRDY_IRQ_ADDR,
+ .mask_int1 = ST_PRESS_LPS331AP_DRDY_IRQ_INT1_MASK,
+ .mask_int2 = ST_PRESS_LPS331AP_DRDY_IRQ_INT2_MASK,
},
- .multi_read_bit = ST_PRESS_1_MULTIREAD_BIT,
+ .multi_read_bit = ST_PRESS_LPS331AP_MULTIREAD_BIT,
.bootime = 2,
},
};
--
1.8.1.2
Some of ST's sensors are appended with their sensor type and some
are not. For consistency we're extending the same naming convention
throughout.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/iio/pressure/st_pressure.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/pressure/st_pressure.h b/drivers/iio/pressure/st_pressure.h
index b0b6306..f1bebce 100644
--- a/drivers/iio/pressure/st_pressure.h
+++ b/drivers/iio/pressure/st_pressure.h
@@ -14,7 +14,7 @@
#include <linux/types.h>
#include <linux/iio/common/st_sensors.h>
-#define LPS331AP_PRESS_DEV_NAME "lps331ap"
+#define LPS331AP_PRESS_DEV_NAME "lps331ap_press"
/**
* struct st_sensors_platform_data - default press platform data
--
1.8.1.2
Saving a few lines of code.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/iio/common/st_sensors/st_sensors_core.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
index 148f0e5..25d4c7e 100644
--- a/drivers/iio/common/st_sensors/st_sensors_core.c
+++ b/drivers/iio/common/st_sensors/st_sensors_core.c
@@ -359,28 +359,25 @@ int st_sensors_read_info_raw(struct iio_dev *indio_dev,
mutex_lock(&indio_dev->mlock);
if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
err = -EBUSY;
- goto read_error;
+ goto out;
} else {
err = st_sensors_set_enable(indio_dev, true);
if (err < 0)
- goto read_error;
+ goto out;
msleep((sdata->sensor->bootime * 1000) / sdata->odr);
err = st_sensors_read_axis_data(indio_dev, ch, val);
if (err < 0)
- goto read_error;
+ goto out;
*val = *val >> ch->scan_type.shift;
err = st_sensors_set_enable(indio_dev, false);
}
+out:
mutex_unlock(&indio_dev->mlock);
return err;
-
-read_error:
- mutex_unlock(&indio_dev->mlock);
- return err;
}
EXPORT_SYMBOL(st_sensors_read_info_raw);
--
1.8.1.2
Gets rid of those unnecessary gotos.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/iio/common/st_sensors/st_sensors_core.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
index 8c4c54c..148f0e5 100644
--- a/drivers/iio/common/st_sensors/st_sensors_core.c
+++ b/drivers/iio/common/st_sensors/st_sensors_core.c
@@ -331,26 +331,23 @@ static int st_sensors_read_axis_data(struct iio_dev *indio_dev,
unsigned int byte_for_channel = ch->scan_type.storagebits >> 3;
outdata = kmalloc(byte_for_channel, GFP_KERNEL);
- if (!outdata) {
- err = -EINVAL;
- goto st_sensors_read_axis_data_error;
- }
+ if (!outdata)
+ return -ENOMEM;
err = sdata->tf->read_multiple_byte(&sdata->tb, sdata->dev,
ch->address, byte_for_channel,
outdata, sdata->multiread_bit);
- if (err < 0)
- goto st_sensors_free_memory;
+ if (err < 0) {
+ kfree(outdata);
+ return err;
+ }
if (byte_for_channel == 2)
*data = (s16)get_unaligned_le16(outdata);
else if (byte_for_channel == 3)
*data = (s32)st_sensors_get_unaligned_le24(outdata);
-st_sensors_free_memory:
- kfree(outdata);
-st_sensors_read_axis_data_error:
- return err;
+ return 0;
}
int st_sensors_read_info_raw(struct iio_dev *indio_dev,
--
1.8.1.2
Strip out all those unnecessary gotos and just return the error right away.
Aids to simplicity and reduces code.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/iio/common/st_sensors/st_sensors_core.c | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
index b86cad2..8c4c54c 100644
--- a/drivers/iio/common/st_sensors/st_sensors_core.c
+++ b/drivers/iio/common/st_sensors/st_sensors_core.c
@@ -208,8 +208,7 @@ int st_sensors_set_drdy_int_pin(struct iio_dev *indio_dev,
if (sdata->sensor->drdy_irq.mask_int1 == 0) {
dev_err(&indio_dev->dev,
"DRDY on INT1 not available.\n");
- err = -EINVAL;
- goto init_error;
+ return -EINVAL;
}
sdata->drdy_int_pin = 1;
break;
@@ -217,15 +216,13 @@ int st_sensors_set_drdy_int_pin(struct iio_dev *indio_dev,
if (sdata->sensor->drdy_irq.mask_int2 == 0) {
dev_err(&indio_dev->dev,
"DRDY on INT2 not available.\n");
- err = -EINVAL;
- goto init_error;
+ return -EINVAL;
}
sdata->drdy_int_pin = 2;
break;
default:
dev_err(&indio_dev->dev, "DRDY on pdata not valid.\n");
- err = -EINVAL;
- goto init_error;
+ return -EINVAL;
}
return 0;
@@ -244,29 +241,28 @@ int st_sensors_init_sensor(struct iio_dev *indio_dev,
err = st_sensors_set_enable(indio_dev, false);
if (err < 0)
- goto init_error;
+ return err;
if (sdata->current_fullscale) {
err = st_sensors_set_fullscale(indio_dev,
sdata->current_fullscale->num);
if (err < 0)
- goto init_error;
+ return err;
} else
dev_info(&indio_dev->dev, "Full-scale not possible\n");
err = st_sensors_set_odr(indio_dev, sdata->odr);
if (err < 0)
- goto init_error;
+ return err;
/* set BDU */
err = st_sensors_write_data_with_mask(indio_dev,
sdata->sensor->bdu.addr, sdata->sensor->bdu.mask, true);
if (err < 0)
- goto init_error;
+ return err;
err = st_sensors_set_axis_enable(indio_dev, ST_SENSORS_ENABLE_ALL_AXIS);
-init_error:
return err;
}
EXPORT_SYMBOL(st_sensors_init_sensor);
--
1.8.1.2
Not all ST's sensors support data ready, so let's make the declaration
of one conditional.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/iio/common/st_sensors/st_sensors_core.c | 24 +++++++++++++++++++-----
drivers/iio/pressure/st_pressure_core.c | 3 ++-
2 files changed, 21 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 eb261a5..b86cad2 100644
--- a/drivers/iio/common/st_sensors/st_sensors_core.c
+++ b/drivers/iio/common/st_sensors/st_sensors_core.c
@@ -198,14 +198,11 @@ int st_sensors_set_axis_enable(struct iio_dev *indio_dev, u8 axis_enable)
}
EXPORT_SYMBOL(st_sensors_set_axis_enable);
-int st_sensors_init_sensor(struct iio_dev *indio_dev,
- struct st_sensors_platform_data *pdata)
+int st_sensors_set_drdy_int_pin(struct iio_dev *indio_dev,
+ struct st_sensors_platform_data *pdata)
{
- int err;
struct st_sensor_data *sdata = iio_priv(indio_dev);
- mutex_init(&sdata->tb.buf_lock);
-
switch (pdata->drdy_int_pin) {
case 1:
if (sdata->sensor->drdy_irq.mask_int1 == 0) {
@@ -231,6 +228,20 @@ int st_sensors_init_sensor(struct iio_dev *indio_dev,
goto init_error;
}
+ return 0;
+}
+
+int st_sensors_init_sensor(struct iio_dev *indio_dev,
+ struct st_sensors_platform_data *pdata)
+{
+ int err = 0;
+ struct st_sensor_data *sdata = iio_priv(indio_dev);
+
+ mutex_init(&sdata->tb.buf_lock);
+
+ if (pdata)
+ err = st_sensors_set_drdy_int_pin(indio_dev, pdata);
+
err = st_sensors_set_enable(indio_dev, false);
if (err < 0)
goto init_error;
@@ -266,6 +277,9 @@ int st_sensors_set_dataready_irq(struct iio_dev *indio_dev, bool enable)
u8 drdy_mask;
struct st_sensor_data *sdata = iio_priv(indio_dev);
+ if (!sdata->sensor->drdy_irq.addr)
+ return 0;
+
/* Enable/Disable the interrupt generator 1. */
if (sdata->sensor->drdy_irq.ig1.en_addr > 0) {
err = st_sensors_write_data_with_mask(indio_dev,
diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
index 16cfbc5..279aafd 100644
--- a/drivers/iio/pressure/st_pressure_core.c
+++ b/drivers/iio/pressure/st_pressure_core.c
@@ -232,7 +232,8 @@ int st_press_common_probe(struct iio_dev *indio_dev,
pdata->odr = pdata->sensor->odr.odr_avl[0].hz;
- if (!plat_data)
+ /* Some devices don't support a data ready pin. */
+ if (!plat_data && pdata->sensor->drdy_irq.addr)
plat_data =
(struct st_sensors_platform_data *)&default_press_pdata;
--
1.8.1.2
Some of ST's sensors are appended with their sensor type and some
are not. For consistency we're extending the same naming convention
throughout.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/iio/magnetometer/st_magn.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iio/magnetometer/st_magn.h b/drivers/iio/magnetometer/st_magn.h
index 694e33e..f185720 100644
--- a/drivers/iio/magnetometer/st_magn.h
+++ b/drivers/iio/magnetometer/st_magn.h
@@ -16,7 +16,7 @@
#define LSM303DLHC_MAGN_DEV_NAME "lsm303dlhc_magn"
#define LSM303DLM_MAGN_DEV_NAME "lsm303dlm_magn"
-#define LIS3MDL_MAGN_DEV_NAME "lis3mdl"
+#define LIS3MDL_MAGN_DEV_NAME "lis3mdl_magn"
int st_magn_common_probe(struct iio_dev *indio_dev,
struct st_sensors_platform_data *pdata);
--
1.8.1.2
Some of ST's sensors are appended with their sensor type and some
are not. For consistency we're extending the same naming convention
throughout.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/iio/gyro/st_gyro.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/gyro/st_gyro.h b/drivers/iio/gyro/st_gyro.h
index f8f2bf8..5f0ec99 100644
--- a/drivers/iio/gyro/st_gyro.h
+++ b/drivers/iio/gyro/st_gyro.h
@@ -14,13 +14,13 @@
#include <linux/types.h>
#include <linux/iio/common/st_sensors.h>
-#define L3G4200D_GYRO_DEV_NAME "l3g4200d"
+#define L3G4200D_GYRO_DEV_NAME "l3g4200d_gyro"
#define LSM330D_GYRO_DEV_NAME "lsm330d_gyro"
#define LSM330DL_GYRO_DEV_NAME "lsm330dl_gyro"
#define LSM330DLC_GYRO_DEV_NAME "lsm330dlc_gyro"
-#define L3GD20_GYRO_DEV_NAME "l3gd20"
-#define L3GD20H_GYRO_DEV_NAME "l3gd20h"
-#define L3G4IS_GYRO_DEV_NAME "l3g4is_ui"
+#define L3GD20_GYRO_DEV_NAME "l3gd20_gyro"
+#define L3GD20H_GYRO_DEV_NAME "l3gd20h_gyro"
+#define L3G4IS_GYRO_DEV_NAME "l3g4is_ui_gyro"
#define LSM330_GYRO_DEV_NAME "lsm330_gyro"
/**
--
1.8.1.2
Some of ST's sensors are appended with their sensor type and some
are not. For consistency we're extending the same naming convention
throughout.
Signed-off-by: Lee Jones <[email protected]>
---
drivers/iio/accel/st_accel.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/accel/st_accel.h b/drivers/iio/accel/st_accel.h
index c387763..d8d22e5 100644
--- a/drivers/iio/accel/st_accel.h
+++ b/drivers/iio/accel/st_accel.h
@@ -15,11 +15,11 @@
#include <linux/iio/common/st_sensors.h>
#define LSM303DLHC_ACCEL_DEV_NAME "lsm303dlhc_accel"
-#define LIS3DH_ACCEL_DEV_NAME "lis3dh"
+#define LIS3DH_ACCEL_DEV_NAME "lis3dh_accel"
#define LSM330D_ACCEL_DEV_NAME "lsm330d_accel"
#define LSM330DL_ACCEL_DEV_NAME "lsm330dl_accel"
#define LSM330DLC_ACCEL_DEV_NAME "lsm330dlc_accel"
-#define LIS331DLH_ACCEL_DEV_NAME "lis331dlh"
+#define LIS331DLH_ACCEL_DEV_NAME "lis331dlh_accel"
#define LSM303DL_ACCEL_DEV_NAME "lsm303dl_accel"
#define LSM303DLH_ACCEL_DEV_NAME "lsm303dlh_accel"
#define LSM303DLM_ACCEL_DEV_NAME "lsm303dlm_accel"
--
1.8.1.2
LSM303DLH is a Magnetometer Sensor
Signed-off-by: Lee Jones <[email protected]>
---
.../bindings/iio/magnetometer/lsm303dlh.txt | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/lsm303dlh.txt
diff --git a/Documentation/devicetree/bindings/iio/magnetometer/lsm303dlh.txt b/Documentation/devicetree/bindings/iio/magnetometer/lsm303dlh.txt
new file mode 100644
index 0000000..5938369
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/magnetometer/lsm303dlh.txt
@@ -0,0 +1,21 @@
+* STMicroelectronics Magnetometer Sensor
+
+Required properties:
+ - compatible: Should be "st,lsm303dlh_magn"
+ - reg: The I2C address of the sensor
+
+Optional properties:
+ - vdd-supply: Phandle to the Vdd supply regulator
+ - vddio-supply: Phandle to the Vdd-IO supply regulator
+
+Example:
+
+i2c@80128000 {
+ lsm303dlh_magn@1e {
+ compatible = "st,lsm303dlh_magn";
+ reg = <0x1e>;
+
+ vdd-supply = <&ab8500_ldo_aux1_reg>;
+ vddio-supply = <&db8500_vsmps2_reg>;
+ };
+};
--
1.8.1.2
L3G4200D is a Gyroscope Sensor
Signed-off-by: Lee Jones <[email protected]>
---
.../devicetree/bindings/iio/gyro/l3g4200d.txt | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/gyro/l3g4200d.txt
diff --git a/Documentation/devicetree/bindings/iio/gyro/l3g4200d.txt b/Documentation/devicetree/bindings/iio/gyro/l3g4200d.txt
new file mode 100644
index 0000000..29baf9d
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/gyro/l3g4200d.txt
@@ -0,0 +1,21 @@
+* STMicroelectronics Gyroscope Sensor
+
+Required properties:
+ - compatible: Should be "st,l3g4200d_gyro"
+ - reg: The I2C address of the sensor
+
+Optional properties:
+ - vdd-supply: Phandle to the Vdd supply regulator
+ - vddio-supply: Phandle to the Vdd-IO supply regulator
+
+Example:
+
+i2c@80128000 {
+ l3g4200d_gyro@68 {
+ compatible = "st,l3g4200d_gyro";
+ reg = <0x68>;
+
+ vdd-supply = <&ab8500_ldo_aux1_reg>;
+ vddio-supply = <&db8500_vsmps2_reg>;
+ };
+};
--
1.8.1.2
Signed-off-by: Lee Jones <[email protected]>
---
arch/arm/configs/u8500_defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/configs/u8500_defconfig b/arch/arm/configs/u8500_defconfig
index 24f88d6..4b94b42 100644
--- a/arch/arm/configs/u8500_defconfig
+++ b/arch/arm/configs/u8500_defconfig
@@ -40,6 +40,7 @@ CONFIG_SENSORS_BH1780=y
CONFIG_IIO=y
CONFIG_IIO_ST_PRESS=y
CONFIG_IIO_ST_ACCEL_3AXIS=y
+CONFIG_IIO_ST_MAGN_3AXIS=y
CONFIG_NETDEVICES=y
CONFIG_SMSC911X=y
CONFIG_SMSC_PHY=y
--
1.8.1.2
LPS001WP is a Pressure and Temperature sensor.
Signed-off-by: Lee Jones <[email protected]>
---
.../devicetree/bindings/iio/pressure/lps001wp.txt | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/pressure/lps001wp.txt
diff --git a/Documentation/devicetree/bindings/iio/pressure/lps001wp.txt b/Documentation/devicetree/bindings/iio/pressure/lps001wp.txt
new file mode 100644
index 0000000..3294a65c6
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/pressure/lps001wp.txt
@@ -0,0 +1,21 @@
+* STMicroelectronics Pressure Sensor
+
+Required properties:
+ - compatible: Should be "st,lps001wp_press"
+ - reg: The I2C address of the sensor
+
+Optional properties:
+ - vdd-supply: Phandle to the Vdd supply regulator
+ - vddio-supply: Phandle to the Vdd-IO supply regulator
+
+Example:
+
+i2c@80128000 {
+ lps001wp@5c {
+ compatible = "st,lps001wp_press";
+ reg = <0x5c>;
+
+ vdd-supply = <&ab8500_ldo_aux1_reg>;
+ vddio-supply = <&db8500_vsmps2_reg>;
+ };
+};
--
1.8.1.2
Signed-off-by: Lee Jones <[email protected]>
---
arch/arm/configs/u8500_defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/configs/u8500_defconfig b/arch/arm/configs/u8500_defconfig
index 6b29109..24f88d6 100644
--- a/arch/arm/configs/u8500_defconfig
+++ b/arch/arm/configs/u8500_defconfig
@@ -39,6 +39,7 @@ CONFIG_BLK_DEV_RAM_SIZE=65536
CONFIG_SENSORS_BH1780=y
CONFIG_IIO=y
CONFIG_IIO_ST_PRESS=y
+CONFIG_IIO_ST_ACCEL_3AXIS=y
CONFIG_NETDEVICES=y
CONFIG_SMSC911X=y
CONFIG_SMSC_PHY=y
--
1.8.1.2
Signed-off-by: Lee Jones <[email protected]>
---
arch/arm/configs/u8500_defconfig | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm/configs/u8500_defconfig b/arch/arm/configs/u8500_defconfig
index a0025dc..6b29109 100644
--- a/arch/arm/configs/u8500_defconfig
+++ b/arch/arm/configs/u8500_defconfig
@@ -37,6 +37,8 @@ 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_NETDEVICES=y
CONFIG_SMSC911X=y
CONFIG_SMSC_PHY=y
--
1.8.1.2
After applying this node the LPS001WP sensor chip should probe
successfully once the driver support has also been applied.
Signed-off-by: Lee Jones <[email protected]>
---
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 f1fc128..d355d25 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_press@5c {
+ compatible = "st,lps001wp_press";
+ reg = <0x5c>;
+
+ vdd-supply = <&ab8500_ldo_aux1_reg>;
+ vddio-supply = <&db8500_vsmps2_reg>;
+ };
+ };
+
uart@80120000 {
status = "okay";
};
--
1.8.1.2
After applying this node the L3G4200D sensor chip should probe
successfully once the driver support has also been applied.
Signed-off-by: Lee Jones <[email protected]>
---
arch/arm/boot/dts/ste-snowball.dts | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/arm/boot/dts/ste-snowball.dts b/arch/arm/boot/dts/ste-snowball.dts
index e37284e..0b53854 100644
--- a/arch/arm/boot/dts/ste-snowball.dts
+++ b/arch/arm/boot/dts/ste-snowball.dts
@@ -177,6 +177,14 @@
vdd-supply = <&ab8500_ldo_aux1_reg>;
vddio-supply = <&db8500_vsmps2_reg>;
};
+
+ l3g4200d_gyro@68 {
+ compatible = "st,l3g4200d_gyro";
+ reg = <0x68>;
+
+ vdd-supply = <&ab8500_ldo_aux1_reg>;
+ vddio-supply = <&db8500_vsmps2_reg>;
+ };
};
uart@80120000 {
--
1.8.1.2
After applying this node the LSM303DLH sensor chip should probe
successfully once the driver support has also been applied.
Signed-off-by: Lee Jones <[email protected]>
---
arch/arm/boot/dts/ste-snowball.dts | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/arm/boot/dts/ste-snowball.dts b/arch/arm/boot/dts/ste-snowball.dts
index d1f4c60..e37284e 100644
--- a/arch/arm/boot/dts/ste-snowball.dts
+++ b/arch/arm/boot/dts/ste-snowball.dts
@@ -169,6 +169,14 @@
vdd-supply = <&ab8500_ldo_aux1_reg>;
vddio-supply = <&db8500_vsmps2_reg>;
};
+
+ lsm303dlh_magn@1e {
+ compatible = "st,lsm303dlh_magn";
+ reg = <0x1e>;
+
+ vdd-supply = <&ab8500_ldo_aux1_reg>;
+ vddio-supply = <&db8500_vsmps2_reg>;
+ };
};
uart@80120000 {
--
1.8.1.2
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 <[email protected]>
---
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 1c1091e..0c1338e 100644
--- a/arch/arm/boot/dts/ste-dbx5x0.dtsi
+++ b/arch/arm/boot/dts/ste-dbx5x0.dtsi
@@ -559,7 +559,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>;
@@ -572,7 +571,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>;
@@ -585,7 +583,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>;
@@ -598,7 +595,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>;
@@ -611,7 +607,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
On Tue, Sep 10, 2013 at 5:49 AM, Lee Jones <[email protected]> wrote:
> Signed-off-by: Lee Jones <[email protected]>
> ---
> arch/arm/configs/u8500_defconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/configs/u8500_defconfig b/arch/arm/configs/u8500_defconfig
> index 6b29109..24f88d6 100644
> --- a/arch/arm/configs/u8500_defconfig
> +++ b/arch/arm/configs/u8500_defconfig
> @@ -39,6 +39,7 @@ CONFIG_BLK_DEV_RAM_SIZE=65536
> CONFIG_SENSORS_BH1780=y
> CONFIG_IIO=y
> CONFIG_IIO_ST_PRESS=y
> +CONFIG_IIO_ST_ACCEL_3AXIS=y
> CONFIG_NETDEVICES=y
> CONFIG_SMSC911X=y
> CONFIG_SMSC_PHY=y
This is ridiculous.
I know lwn.net's patch author statistics are fun to be on, but
splitting this and the other config update into two patches? Really?
-Olof
Hi,
On Tue, Sep 10, 2013 at 5:49 AM, Lee Jones <[email protected]> wrote:
> 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 <[email protected]>
How did you test this patch? What hardware did you boot it on and with
what config?
Thanks!
-Olof
> On Tue, Sep 10, 2013 at 5:49 AM, Lee Jones <[email protected]> wrote:
> > 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 <[email protected]>
>
> How did you test this patch? What hardware did you boot it on and with
> what config?
Snowball, u8500_defconfig.
Why? Don't you have the same result with yours?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tue, 10 Sep 2013, Olof Johansson wrote:
> On Tue, Sep 10, 2013 at 5:49 AM, Lee Jones <[email protected]> wrote:
> > Signed-off-by: Lee Jones <[email protected]>
> > ---
> > arch/arm/configs/u8500_defconfig | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/arm/configs/u8500_defconfig b/arch/arm/configs/u8500_defconfig
> > index 6b29109..24f88d6 100644
> > --- a/arch/arm/configs/u8500_defconfig
> > +++ b/arch/arm/configs/u8500_defconfig
> > @@ -39,6 +39,7 @@ CONFIG_BLK_DEV_RAM_SIZE=65536
> > CONFIG_SENSORS_BH1780=y
> > CONFIG_IIO=y
> > CONFIG_IIO_ST_PRESS=y
> > +CONFIG_IIO_ST_ACCEL_3AXIS=y
> > CONFIG_NETDEVICES=y
> > CONFIG_SMSC911X=y
> > CONFIG_SMSC_PHY=y
>
> This is ridiculous.
>
> I know lwn.net's patch author statistics are fun to be on, but
Jealousy won't get you anywhere... ;)
> splitting this and the other config update into two patches? Really?
I haven't split anything. I created the patches as I was
enabling/testing the drivers, which was at completely different
times. The only reason they're close together in the set is because I
reorganised it and didn't think it was necessary to squash them
together.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Lee Jones <[email protected]> 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 <[email protected]>
>---
>drivers/iio/pressure/st_pressure_core.c | 32
>++++++++++++++++----------------
> 1 file changed, 16 insertions(+), 16 deletions(-)
>
>diff --git a/drivers/iio/pressure/st_pressure_core.c
>b/drivers/iio/pressure/st_pressure_core.c
>index 3abada2..6ffd949 100644
>--- a/drivers/iio/pressure/st_pressure_core.c
>+++ b/drivers/iio/pressure/st_pressure_core.c
>@@ -232,21 +232,23 @@ static const struct iio_trigger_ops
>st_press_trigger_ops = {
> int st_press_common_probe(struct iio_dev *indio_dev,
> struct st_sensors_platform_data *plat_data)
> {
>- 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 *)
>@@ -261,32 +263,30 @@ int st_press_common_probe(struct iio_dev
>*indio_dev,
>
> err = st_sensors_init_sensor(indio_dev, plat_data);
> 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);
>+ ST_PRESS_TRIGGER_OPS);
> if (err < 0)
> goto st_press_probe_trigger_error;
> }
>
> err = iio_device_register(indio_dev);
>- if (err)
This bit of handling is confusing. I would much rather see the if IRQ at the goto. Here the first thought is why is it not an error if there is no IRQ!
>+ if (err && irq > 0)
> goto st_press_device_register_error;
>
> return err;
>
> st_press_device_register_error:
>- if (pdata->get_irq_data_ready(indio_dev) > 0)
>- st_sensors_deallocate_trigger(indio_dev);
>+ 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:
>+ st_press_deallocate_ring(indio_dev);
>+
> return err;
> }
> EXPORT_SYMBOL(st_press_common_probe);
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
Lee Jones <[email protected]> wrote:
>At the moment the driver is silent in some error cases and if
>successful.
>Prior to this patch there was no clear way to know if the driver
>succeeded
>or not without looking deep into sysfs.
>
>Signed-off-by: Lee Jones <[email protected]>
>---
> drivers/iio/pressure/st_pressure_core.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/drivers/iio/pressure/st_pressure_core.c
>b/drivers/iio/pressure/st_pressure_core.c
>index 6ffd949..34b3fb1 100644
>--- a/drivers/iio/pressure/st_pressure_core.c
>+++ b/drivers/iio/pressure/st_pressure_core.c
>@@ -280,6 +280,9 @@ int st_press_common_probe(struct iio_dev
>*indio_dev,
> if (err && irq > 0)
> goto st_press_device_register_error;
>
>+ if (!err)
>+ dev_info(&indio_dev->dev, "Successfully registered\n");
>+
Not keen. That to my mind is pointless noise. If this made sense then it would be in the driver core not here.
> return err;
>
> st_press_device_register_error:
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
On Tue, Sep 10, 2013 at 8:35 AM, Lee Jones <[email protected]> wrote:
>> splitting this and the other config update into two patches? Really?
>
> I haven't split anything. I created the patches as I was
> enabling/testing the drivers, which was at completely different
> times. The only reason they're close together in the set is because I
> reorganised it and didn't think it was necessary to squash them
> together.
In this case, squashing makes a lot of sense. But you could even have
LinusW do that for you when he pulls in the various config updates for
next release.
-Olof
[pruning out the iio list/people]
On Tue, Sep 10, 2013 at 8:30 AM, Lee Jones <[email protected]> wrote:
>> On Tue, Sep 10, 2013 at 5:49 AM, Lee Jones <[email protected]> wrote:
>> > 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 <[email protected]>
>>
>> How did you test this patch? What hardware did you boot it on and with
>> what config?
>
> Snowball, u8500_defconfig.
>
> Why? Don't you have the same result with yours?
I have a very weird experience with snowball right now. I noticed this
yesterday when I decided to look at why multi_v7_defconfig doesn't
boot on it:
* u8500_defconfig doesn't boot as a DT kernel, since the machine ID is
still enabled. If I disable the machine ID, it doesn't boot.
* Same for multi_v7_defconfig, since that is only DT: It doesn't boot.
Unfortunately I can't get to my home network right now to double-check
boot logs to see if this has changed behavior recently, I'll have to
do that later today. It'd be interesting to hear if you have the same
experience w.r.t. DT on u8500_defconfig though.
-Olof
On Tue, 10 Sep 2013, Jonathan Cameron wrote:
> Lee Jones <[email protected]> wrote:
> >At the moment the driver is silent in some error cases and if
> >successful.
> >Prior to this patch there was no clear way to know if the driver
> >succeeded
> >or not without looking deep into sysfs.
> >
> >Signed-off-by: Lee Jones <[email protected]>
> >---
> > drivers/iio/pressure/st_pressure_core.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> >diff --git a/drivers/iio/pressure/st_pressure_core.c
> >b/drivers/iio/pressure/st_pressure_core.c
> >index 6ffd949..34b3fb1 100644
> >--- a/drivers/iio/pressure/st_pressure_core.c
> >+++ b/drivers/iio/pressure/st_pressure_core.c
> >@@ -280,6 +280,9 @@ int st_press_common_probe(struct iio_dev
> >*indio_dev,
> > if (err && irq > 0)
> > goto st_press_device_register_error;
> >
> >+ if (!err)
> >+ dev_info(&indio_dev->dev, "Successfully registered\n");
> >+
> Not keen. That to my mind is pointless noise.
I think it needs to be somewhere. IIO provides no indication whether
these chips are probed/registered/whathaveyou, or even if the
subsystem is in use.
One line per hardware component is not noise, IMO it's indicative of
key functionality which is now available:
Bootlog:
<snip>
iio iio: lsm303dlh_accel: Successfully registered
iio iio: l3g4200d_gyro: Successfully registered
iio iio: lps001wp_press: Successfully registered
iio iio: lsm303dlhc_magn: Successfully registered
<snip>
> If this made sense then it would be in the driver core not here.
Also fine. Just anything but silence/NULL reporting.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On 09/11/2013 09:10 AM, Lee Jones wrote:
> On Tue, 10 Sep 2013, Jonathan Cameron wrote:
>> Lee Jones <[email protected]> wrote:
>>> At the moment the driver is silent in some error cases and if
>>> successful.
>>> Prior to this patch there was no clear way to know if the driver
>>> succeeded
>>> or not without looking deep into sysfs.
>>>
>>> Signed-off-by: Lee Jones <[email protected]>
>>> ---
>>> drivers/iio/pressure/st_pressure_core.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/iio/pressure/st_pressure_core.c
>>> b/drivers/iio/pressure/st_pressure_core.c
>>> index 6ffd949..34b3fb1 100644
>>> --- a/drivers/iio/pressure/st_pressure_core.c
>>> +++ b/drivers/iio/pressure/st_pressure_core.c
>>> @@ -280,6 +280,9 @@ int st_press_common_probe(struct iio_dev
>>> *indio_dev,
>>> if (err && irq > 0)
>>> goto st_press_device_register_error;
>>>
>>> + if (!err)
>>> + dev_info(&indio_dev->dev, "Successfully registered\n");
>>> +
>> Not keen. That to my mind is pointless noise.
>
> I think it needs to be somewhere. IIO provides no indication whether
> these chips are probed/registered/whathaveyou, or even if the
> subsystem is in use.
>
> One line per hardware component is not noise, IMO it's indicative of
> key functionality which is now available:
>
> Bootlog:
> <snip>
> iio iio: lsm303dlh_accel: Successfully registered
> iio iio: l3g4200d_gyro: Successfully registered
> iio iio: lps001wp_press: Successfully registered
> iio iio: lsm303dlhc_magn: Successfully registered
> <snip>
>
>> If this made sense then it would be in the driver core not here.
>
> Also fine. Just anything but silence/NULL reporting.
Silence means everything is good, a message means there is an error. If every
device that gets probed would spit out a message the log would be scrolling
forever and you wouldn't be able to see the error messages.
- Lars
> > err = st_sensors_init_sensor(indio_dev, plat_data);
> > 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);
> >+ ST_PRESS_TRIGGER_OPS);
> > if (err < 0)
> > goto st_press_probe_trigger_error;
> > }
> >
> > err = iio_device_register(indio_dev);
> >- if (err)
>
> This bit of handling is confusing. I would much rather see the if IRQ at the goto. Here the first thought is why is it not an error if there is no IRQ!
I certainly see your point. But surely anyone would see after a second
or two that we're returning err and not 0 in this case, so the error
would still be returned, we're not ignoring it. Adding this extra
comparison saves several lines of code.
If you think it's 'too' confusing, I'll revert the change.
Or perhaps a comment:
/* Only deallocate_[trigger|ring] if they were allocated. */
or
/* Only deallocate_[trigger|ring] if we have an IRQ line. */
or
/* If no IRQ was specified, just return the error. */
> >+ if (err && irq > 0)
> > goto st_press_device_register_error;
> >
> > return err;
> >
> > st_press_device_register_error:
> >- if (pdata->get_irq_data_ready(indio_dev) > 0)
> >- st_sensors_deallocate_trigger(indio_dev);
> >+ 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:
> >+ st_press_deallocate_ring(indio_dev);
> >+
> > return err;
> > }
> > EXPORT_SYMBOL(st_press_common_probe);
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tue, 10 Sep 2013, Olof Johansson wrote:
> On Tue, Sep 10, 2013 at 8:35 AM, Lee Jones <[email protected]> wrote:
>
> >> splitting this and the other config update into two patches? Really?
> >
> > I haven't split anything. I created the patches as I was
> > enabling/testing the drivers, which was at completely different
> > times. The only reason they're close together in the set is because I
> > reorganised it and didn't think it was necessary to squash them
> > together.
>
> In this case, squashing makes a lot of sense.
No problem. That's all you had to say. I'll do it right away.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, 11 Sep 2013, Lars-Peter Clausen wrote:
> On 09/11/2013 09:10 AM, Lee Jones wrote:
> >On Tue, 10 Sep 2013, Jonathan Cameron wrote:
> >>Lee Jones <[email protected]> wrote:
> >>>At the moment the driver is silent in some error cases and if
> >>>successful.
> >>>Prior to this patch there was no clear way to know if the driver
> >>>succeeded
> >>>or not without looking deep into sysfs.
> >>>
> >>>Signed-off-by: Lee Jones <[email protected]>
> >>>---
> >>>drivers/iio/pressure/st_pressure_core.c | 3 +++
> >>>1 file changed, 3 insertions(+)
> >>>
> >>>diff --git a/drivers/iio/pressure/st_pressure_core.c
> >>>b/drivers/iio/pressure/st_pressure_core.c
> >>>index 6ffd949..34b3fb1 100644
> >>>--- a/drivers/iio/pressure/st_pressure_core.c
> >>>+++ b/drivers/iio/pressure/st_pressure_core.c
> >>>@@ -280,6 +280,9 @@ int st_press_common_probe(struct iio_dev
> >>>*indio_dev,
> >>> if (err && irq > 0)
> >>> goto st_press_device_register_error;
> >>>
> >>>+ if (!err)
> >>>+ dev_info(&indio_dev->dev, "Successfully registered\n");
> >>>+
> >>Not keen. That to my mind is pointless noise.
> >
> >I think it needs to be somewhere. IIO provides no indication whether
> >these chips are probed/registered/whathaveyou, or even if the
> >subsystem is in use.
> >
> >One line per hardware component is not noise, IMO it's indicative of
> >key functionality which is now available:
> >
> >Bootlog:
> ><snip>
> >iio iio: lsm303dlh_accel: Successfully registered
> >iio iio: l3g4200d_gyro: Successfully registered
> >iio iio: lps001wp_press: Successfully registered
> >iio iio: lsm303dlhc_magn: Successfully registered
> ><snip>
> >
> >>If this made sense then it would be in the driver core not here.
> >
> >Also fine. Just anything but silence/NULL reporting.
>
> Silence means everything is good, a message means there is an error.
> If every device that gets probed would spit out a message the log
> would be scrolling forever and you wouldn't be able to see the error
> messages.
Only if you print out every regulator, clock, GPIO pin and things of
this nature. Key hardware blocks such as; SD, Flash, USB, Eth, HDMI,
Audio, UART, GPIO and I2C controllers and Sensors I think deserve a
one line "I'm here and working" message.
I'll not fight this for too long. This is based my experience as a
user. I tried to look at the bootlog for the sensors I'd just enabled
and there was nothing. Some of them had probed, some hadn't and there
was no clear way to distinguish between them without digging into
sysfs.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tue, Sep 10, 2013 at 6:57 PM, Olof Johansson <[email protected]> wrote:
> I have a very weird experience with snowball right now. I noticed this
> yesterday when I decided to look at why multi_v7_defconfig doesn't
> boot on it:
>
> * u8500_defconfig doesn't boot as a DT kernel, since the machine ID is
> still enabled. If I disable the machine ID, it doesn't boot.
> * Same for multi_v7_defconfig, since that is only DT: It doesn't boot.
Weird, yeah there is something wrong on Torvalds' HEAD, with
earlyprint it says:
Uncompressing Linux... done, booting the kernel.
Error: unrecognized/unsupported processor variant (0x412fc091).
This comes from arch/arm/kernel/head-common.S
I'm trying to bisect and find out what is causing this...
Yours,
Linus Walleij
On Tue, 10 Sep 2013, Olof Johansson wrote:
> [pruning out the iio list/people]
>
> On Tue, Sep 10, 2013 at 8:30 AM, Lee Jones <[email protected]> wrote:
> >> On Tue, Sep 10, 2013 at 5:49 AM, Lee Jones <[email protected]> wrote:
> >> > 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 <[email protected]>
> >>
> >> How did you test this patch? What hardware did you boot it on and with
> >> what config?
> >
> > Snowball, u8500_defconfig.
> >
> > Why? Don't you have the same result with yours?
>
> I have a very weird experience with snowball right now. I noticed this
> yesterday when I decided to look at why multi_v7_defconfig doesn't
> boot on it:
>
> * u8500_defconfig doesn't boot as a DT kernel, since the machine ID is
> still enabled. If I disable the machine ID, it doesn't boot.
> * Same for multi_v7_defconfig, since that is only DT: It doesn't boot.
I'm not entirely sure what you mean by this, as I have Snowball
booting as a Device Tree only platform:
http://www.spinics.net/lists/kernel/msg1590759.html
The Machine ID should be all F's for DT I think.
> Unfortunately I can't get to my home network right now to double-check
> boot logs to see if this has changed behavior recently, I'll have to
> do that later today. It'd be interesting to hear if you have the same
> experience w.r.t. DT on u8500_defconfig though.
I just changed SNOWBALL's machine ID to 9999 and it still boots just
fine with DT (ATAGs boot hangs). So what did you do? Can you send me
a patch of what you did, so I might reproduce your build please?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, 11 Sep 2013, Linus Walleij wrote:
> On Tue, Sep 10, 2013 at 6:57 PM, Olof Johansson <[email protected]> wrote:
>
> > I have a very weird experience with snowball right now. I noticed this
> > yesterday when I decided to look at why multi_v7_defconfig doesn't
> > boot on it:
> >
> > * u8500_defconfig doesn't boot as a DT kernel, since the machine ID is
> > still enabled. If I disable the machine ID, it doesn't boot.
> > * Same for multi_v7_defconfig, since that is only DT: It doesn't boot.
>
> Weird, yeah there is something wrong on Torvalds' HEAD, with
> earlyprint it says:
>
> Uncompressing Linux... done, booting the kernel.
> Error: unrecognized/unsupported processor variant (0x412fc091).
> This comes from arch/arm/kernel/head-common.S
>
> I'm trying to bisect and find out what is causing this...
Ah nice. Keep us informed of any progress.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Wed, Sep 11, 2013 at 10:19 AM, Lee Jones <[email protected]> wrote:
> On Wed, 11 Sep 2013, Linus Walleij wrote:
>
>> Weird, yeah there is something wrong on Torvalds' HEAD, with
>> earlyprint it says:
>>
>> Uncompressing Linux... done, booting the kernel.
>> Error: unrecognized/unsupported processor variant (0x412fc091).
>> This comes from arch/arm/kernel/head-common.S
>>
>> I'm trying to bisect and find out what is causing this...
>
> Ah nice. Keep us informed of any progress.
No it was something transient, after a clean rebuild it boots
just fine. :-/
I'll see if I can boot the same uImage on the Snowball too.
Linus Walleij
On Wed, Sep 11, 2013 at 11:39 AM, Lee Jones <[email protected]> wrote:
> On 11 Sep 2013 10:33, "Linus Walleij" <[email protected]> wrote:
>> No it was something transient, after a clean rebuild it boots
>> just fine. :-/
>>
>> I'll see if I can boot the same uImage on the Snowball too.
Yeah Snowball boots the same DT image with just some random
dmesg noise from Torvalds HEAD:
U8500 $ mmc rescan 1 ; fat load mmc 1 0x0 /uImage ; bootm 0x0
Partition info retrieved
Reading /uImage
8997342 bytes read
## Booting kernel from Legacy Image at 00000000 ...
Image Name: Ux500 Device Tree kernel
Image Type: ARM Linux Kernel Image (uncompressed)
Data Size: 8997278 Bytes = 8.6 MB
Load Address: 00008000
Entry Point: 00008000
Loading Kernel Image ... OK
OK
Starting kernel ...
Uncompressing Linux... done, booting the kernel.
Booting Linux on physical CPU 0x300
Linux version 3.11.0-09031-ga22a0fd ([email protected]) (gcc
version 4.8.2 20130805 (prerelease) (crosstool-NG
linaro-1.13.1-4.8-2013.08 - Linaro GCC 2013.08) ) #48 SMP PREEMPT Wed
Sep 11 11:14:36 CEST 2013
CPU: ARMv7 Processor [412fc091] revision 1 (ARMv7), cr=10c5387d
CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing instruction cache
Machine: ST-Ericsson Ux5x0 platform (Device Tree Support), model:
ST-Ericsson HREF (v60+) platform with Device Tree
Only cachepolicy=writeback supported on ARMv6 and later
Memory policy: ECC disabled, Data cache writealloc
DB8500 v2.1 [0x008500b1]
(...)
I don't know what is the problem with Olof's machine :-/
Olof do you want a copy of my uImage?
Yours,
Linus Walleij
On Wed, Sep 11, 2013 at 08:29:55AM +0100, Lee Jones wrote:
> Only if you print out every regulator, clock, GPIO pin and things of
> this nature. Key hardware blocks such as; SD, Flash, USB, Eth, HDMI,
> Audio, UART, GPIO and I2C controllers and Sensors I think deserve a
> one line "I'm here and working" message.
The usual rule people apply with this stuff is if the driver has read
back information it can display from the hardware (eg, a revision) then
that's OK. Things that are likely to crash the system if they fail tend
to also be OK (this is why the regualtor API is quite chatty for
example, a lot of the things it does can cause an immediate crash).
On 09/11/13 08:18, Lars-Peter Clausen wrote:
> On 09/11/2013 09:10 AM, Lee Jones wrote:
>> On Tue, 10 Sep 2013, Jonathan Cameron wrote:
>>> Lee Jones <[email protected]> wrote:
>>>> At the moment the driver is silent in some error cases and if
>>>> successful.
>>>> Prior to this patch there was no clear way to know if the driver
>>>> succeeded
>>>> or not without looking deep into sysfs.
>>>>
>>>> Signed-off-by: Lee Jones <[email protected]>
>>>> ---
>>>> drivers/iio/pressure/st_pressure_core.c | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/iio/pressure/st_pressure_core.c
>>>> b/drivers/iio/pressure/st_pressure_core.c
>>>> index 6ffd949..34b3fb1 100644
>>>> --- a/drivers/iio/pressure/st_pressure_core.c
>>>> +++ b/drivers/iio/pressure/st_pressure_core.c
>>>> @@ -280,6 +280,9 @@ int st_press_common_probe(struct iio_dev
>>>> *indio_dev,
>>>> if (err && irq > 0)
>>>> goto st_press_device_register_error;
>>>>
>>>> + if (!err)
>>>> + dev_info(&indio_dev->dev, "Successfully registered\n");
>>>> +
>>> Not keen. That to my mind is pointless noise.
>>
>> I think it needs to be somewhere. IIO provides no indication whether
>> these chips are probed/registered/whathaveyou, or even if the
>> subsystem is in use.
>>
>> One line per hardware component is not noise, IMO it's indicative of
>> key functionality which is now available:
>>
>> Bootlog:
>> <snip>
>> iio iio: lsm303dlh_accel: Successfully registered
>> iio iio: l3g4200d_gyro: Successfully registered
>> iio iio: lps001wp_press: Successfully registered
>> iio iio: lsm303dlhc_magn: Successfully registered
>> <snip>
>>
>>> If this made sense then it would be in the driver core not here.
>>
>> Also fine. Just anything but silence/NULL reporting.
>
> Silence means everything is good, a message means there is an error. If every device that gets probed would spit out a
> message the log would be scrolling forever and you wouldn't be able to see the error messages.
Exactly. If you really think this is worthwhile, then propose it is added to the relevant
place in drivers/base/ (possibly bus.c) I've just been lax in persuading people not
to put it in existing drivers rather than the other way around.
>
> - Lars
>
On Wed, Sep 11, 2013 at 4:13 AM, Linus Walleij <[email protected]> wrote:
> On Wed, Sep 11, 2013 at 11:39 AM, Lee Jones <[email protected]> wrote:
>> On 11 Sep 2013 10:33, "Linus Walleij" <[email protected]> wrote:
>
>>> No it was something transient, after a clean rebuild it boots
>>> just fine. :-/
>>>
>>> I'll see if I can boot the same uImage on the Snowball too.
>
> Yeah Snowball boots the same DT image with just some random
> dmesg noise from Torvalds HEAD:
>
> U8500 $ mmc rescan 1 ; fat load mmc 1 0x0 /uImage ; bootm 0x0
> Partition info retrieved
> Reading /uImage
>
> 8997342 bytes read
> ## Booting kernel from Legacy Image at 00000000 ...
> Image Name: Ux500 Device Tree kernel
> Image Type: ARM Linux Kernel Image (uncompressed)
> Data Size: 8997278 Bytes = 8.6 MB
> Load Address: 00008000
> Entry Point: 00008000
> Loading Kernel Image ... OK
> OK
>
> Starting kernel ...
>
> Uncompressing Linux... done, booting the kernel.
> Booting Linux on physical CPU 0x300
> Linux version 3.11.0-09031-ga22a0fd ([email protected]) (gcc
> version 4.8.2 20130805 (prerelease) (crosstool-NG
> linaro-1.13.1-4.8-2013.08 - Linaro GCC 2013.08) ) #48 SMP PREEMPT Wed
> Sep 11 11:14:36 CEST 2013
> CPU: ARMv7 Processor [412fc091] revision 1 (ARMv7), cr=10c5387d
> CPU: PIPT / VIPT nonaliasing data cache, VIPT aliasing instruction cache
> Machine: ST-Ericsson Ux5x0 platform (Device Tree Support), model:
> ST-Ericsson HREF (v60+) platform with Device Tree
> Only cachepolicy=writeback supported on ARMv6 and later
> Memory policy: ECC disabled, Data cache writealloc
> DB8500 v2.1 [0x008500b1]
> (...)
>
> I don't know what is the problem with Olof's machine :-/
Oh! Well, to start with apparantly u8500_defconfig doesn't have
APPENDED_DTB enabled, which explains some of the confusion here.
With that fixed (and the appropriate options to get the bootargs from
the atags), I get as far as console probing where output stops, if I
have DEBUG_LL on. With it disabled I get all the way up.
Not sure what was going on earlier here. Maybe just as in your case it
was a matter of cleaning out and starting fresh.
multi_v7_defconfig still doesn't boot though, but the failure seems to
be lower level than any of the others, no output with DEBUG_LL on.
-Olof
On Wed, Sep 11, 2013 at 10:36 PM, Olof Johansson <[email protected]> wrote:
> [Me]
>> I don't know what is the problem with Olof's machine :-/
>
> Oh! Well, to start with apparantly u8500_defconfig doesn't have
> APPENDED_DTB enabled, which explains some of the confusion here.
Oh. Well honestly I think we will never be able to rewrite the
U8500 bootloaders, so maybe I should just send a patch adding
this to the defconfig by default.
> With that fixed (and the appropriate options to get the bootargs from
> the atags), I get as far as console probing where output stops, if I
> have DEBUG_LL on. With it disabled I get all the way up.
Yeah somewhere the bit-banging from the LL-prints that just
hammer the hardware start conflicting with the kernel control
over the same port. I guess it's the same for all users of the
PL011.
> Not sure what was going on earlier here. Maybe just as in your case it
> was a matter of cleaning out and starting fresh.
Yeah, it's scary :-/
> multi_v7_defconfig still doesn't boot though, but the failure seems to
> be lower level than any of the others, no output with DEBUG_LL on.
Probably the usual case where one system is screwing up for
another system. We'll need to look into this.
Yours,
Linus Walleij
On Thu, Sep 12, 2013 at 02:23:48PM +0200, Linus Walleij wrote:
> Yeah somewhere the bit-banging from the LL-prints that just
> hammer the hardware start conflicting with the kernel control
> over the same port. I guess it's the same for all users of the
> PL011.
That shouldn't happen, unless the port becomes disabled (eg, you're
not using the LL-debug port as your console, and its then placed into
a low power mode.)
Again, let me re-interate what LL-debug is for: it's there to assist
getting the early boot up and running. Once the early boot is working,
it should *not* be used.
On Wednesday, September 11, 2013 3:30 AM Lee Jones wrote:
> On Wed, 11 Sep 2013, Lars-Peter Clausen wrote:
> > On 09/11/2013 09:10 AM, Lee Jones wrote:
> > >On Tue, 10 Sep 2013, Jonathan Cameron wrote:
> > >>Lee Jones <[email protected]> wrote:
> > Silence means everything is good, a message means there is an error.
> > If every device that gets probed would spit out a message the log
> > would be scrolling forever and you wouldn't be able to see the error
> > messages.
>
> Only if you print out every regulator, clock, GPIO pin and things of this nature.
> Key hardware blocks such as; SD, Flash, USB, Eth, HDMI, Audio, UART, GPIO and
> I2C controllers and Sensors I think deserve a one line "I'm here and working"
> message.
In many embedded systems -- there are lots and lots of IIO devices, and I'm not sure
I would consider very many "key". It really depends on the board.
To me it's a matter of boot time. Even memcpy to msgbuf takes time.
Sometimes unnecessary time.
> I'll not fight this for too long. This is based my experience as a user. I tried to
> look at the bootlog for the sensors I'd just enabled and there was nothing. Some
> of them had probed, some hadn't and there was no clear way to distinguish
> between them without digging into sysfs.
echo "working:"
cat /sys/bus/iio/devices/*/name
isn't too much digging. If you really want to do that on your machine - do so in rc.local
for many other subsystems, it's just not that easy, and printing from the driver is necessary.
-Robin
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
added devicetree list cc.
On 09/10/13 13:49, Lee Jones wrote:
> LPS001WP is a Pressure and Temperature sensor.
>
> Signed-off-by: Lee Jones <[email protected]>
> ---
> .../devicetree/bindings/iio/pressure/lps001wp.txt | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/pressure/lps001wp.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/pressure/lps001wp.txt b/Documentation/devicetree/bindings/iio/pressure/lps001wp.txt
> new file mode 100644
> index 0000000..3294a65c6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/pressure/lps001wp.txt
> @@ -0,0 +1,21 @@
> +* STMicroelectronics Pressure Sensor
> +
> +Required properties:
> + - compatible: Should be "st,lps001wp_press"
> + - reg: The I2C address of the sensor
> +
> +Optional properties:
> + - vdd-supply: Phandle to the Vdd supply regulator
> + - vddio-supply: Phandle to the Vdd-IO supply regulator
> +
> +Example:
> +
> +i2c@80128000 {
> + lps001wp@5c {
> + compatible = "st,lps001wp_press";
> + reg = <0x5c>;
> +
> + vdd-supply = <&ab8500_ldo_aux1_reg>;
> + vddio-supply = <&db8500_vsmps2_reg>;
> + };
> +};
> -- 1.8.1.2 -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to
> [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
add devicetree list cc
On 09/10/13 13:49, Lee Jones wrote:
> LSM303DLH is a Accelerometer Sensor
>
> Signed-off-by: Lee Jones <[email protected]>
> ---
> .../devicetree/bindings/iio/accel/lsm303dlh.txt | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/accel/lsm303dlh.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/accel/lsm303dlh.txt b/Documentation/devicetree/bindings/iio/accel/lsm303dlh.txt
> new file mode 100644
> index 0000000..bb59363
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/accel/lsm303dlh.txt
> @@ -0,0 +1,21 @@
> +* STMicroelectronics Accelerometer Sensor
> +
> +Required properties:
> + - compatible: Should be "st,lsm303dlh_accel"
> + - reg: The I2C address of the sensor
> +
> +Optional properties:
> + - vdd-supply: Phandle to the Vdd supply regulator
> + - vddio-supply: Phandle to the Vdd-IO supply regulator
> +
> +Example:
> +
> +i2c@80128000 {
> + lsm303dlh_accel@19 {
> + compatible = "st,lsm303dlh_accel";
> + reg = <0x19>;
> +
> + vdd-supply = <&ab8500_ldo_aux1_reg>;
> + vddio-supply = <&db8500_vsmps2_reg>;
> + };
> +};
>
Add devicetree list cc
On 09/10/13 13:49, Lee Jones wrote:
> L3G4200D is a Gyroscope Sensor
>
> Signed-off-by: Lee Jones <[email protected]>
> ---
> .../devicetree/bindings/iio/gyro/l3g4200d.txt | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/gyro/l3g4200d.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/gyro/l3g4200d.txt b/Documentation/devicetree/bindings/iio/gyro/l3g4200d.txt
> new file mode 100644
> index 0000000..29baf9d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/gyro/l3g4200d.txt
> @@ -0,0 +1,21 @@
> +* STMicroelectronics Gyroscope Sensor
> +
> +Required properties:
> + - compatible: Should be "st,l3g4200d_gyro"
> + - reg: The I2C address of the sensor
> +
> +Optional properties:
> + - vdd-supply: Phandle to the Vdd supply regulator
> + - vddio-supply: Phandle to the Vdd-IO supply regulator
> +
> +Example:
> +
> +i2c@80128000 {
> + l3g4200d_gyro@68 {
> + compatible = "st,l3g4200d_gyro";
> + reg = <0x68>;
> +
> + vdd-supply = <&ab8500_ldo_aux1_reg>;
> + vddio-supply = <&db8500_vsmps2_reg>;
> + };
> +};
>
Add devicetree list cc.
On 09/10/13 13:49, Lee Jones wrote:
> LSM303DLH is a Magnetometer Sensor
>
> Signed-off-by: Lee Jones <[email protected]>
> ---
> .../bindings/iio/magnetometer/lsm303dlh.txt | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/magnetometer/lsm303dlh.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/magnetometer/lsm303dlh.txt b/Documentation/devicetree/bindings/iio/magnetometer/lsm303dlh.txt
> new file mode 100644
> index 0000000..5938369
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/magnetometer/lsm303dlh.txt
> @@ -0,0 +1,21 @@
> +* STMicroelectronics Magnetometer Sensor
> +
> +Required properties:
> + - compatible: Should be "st,lsm303dlh_magn"
> + - reg: The I2C address of the sensor
> +
> +Optional properties:
> + - vdd-supply: Phandle to the Vdd supply regulator
> + - vddio-supply: Phandle to the Vdd-IO supply regulator
> +
> +Example:
> +
> +i2c@80128000 {
> + lsm303dlh_magn@1e {
> + compatible = "st,lsm303dlh_magn";
> + reg = <0x1e>;
> +
> + vdd-supply = <&ab8500_ldo_aux1_reg>;
> + vddio-supply = <&db8500_vsmps2_reg>;
> + };
> +};
>
On 09/10/13 13:49, Lee Jones wrote:
> Some of ST's sensors are appended with their sensor type and some
> are not. For consistency we're extending the same naming convention
> throughout.
>
> Signed-off-by: Lee Jones <[email protected]>
Honestly I don't care either way on these, but consistency would definitely
be good so applied to the togreg branch of iio.git
Thanks,
> ---
> drivers/iio/accel/st_accel.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/accel/st_accel.h b/drivers/iio/accel/st_accel.h
> index c387763..d8d22e5 100644
> --- a/drivers/iio/accel/st_accel.h
> +++ b/drivers/iio/accel/st_accel.h
> @@ -15,11 +15,11 @@
> #include <linux/iio/common/st_sensors.h>
>
> #define LSM303DLHC_ACCEL_DEV_NAME "lsm303dlhc_accel"
> -#define LIS3DH_ACCEL_DEV_NAME "lis3dh"
> +#define LIS3DH_ACCEL_DEV_NAME "lis3dh_accel"
> #define LSM330D_ACCEL_DEV_NAME "lsm330d_accel"
> #define LSM330DL_ACCEL_DEV_NAME "lsm330dl_accel"
> #define LSM330DLC_ACCEL_DEV_NAME "lsm330dlc_accel"
> -#define LIS331DLH_ACCEL_DEV_NAME "lis331dlh"
> +#define LIS331DLH_ACCEL_DEV_NAME "lis331dlh_accel"
> #define LSM303DL_ACCEL_DEV_NAME "lsm303dl_accel"
> #define LSM303DLH_ACCEL_DEV_NAME "lsm303dlh_accel"
> #define LSM303DLM_ACCEL_DEV_NAME "lsm303dlm_accel"
>
On 09/14/13 13:14, Jonathan Cameron wrote:
> On 09/10/13 13:49, Lee Jones wrote:
>> Some of ST's sensors are appended with their sensor type and some
>> are not. For consistency we're extending the same naming convention
>> throughout.
>>
>> Signed-off-by: Lee Jones <[email protected]>
> Honestly I don't care either way on these, but consistency would definitely
> be good so applied to the togreg branch of iio.git
>
> Thanks,
Actually change of plan. I'm going to hold off on these as this an ABI change.
Iritating though having these as completely inconsistent is, changing this
will change device identification from userspace which is not a good idea.
Sorry Lee, but we really shouldn't do this. I should have picked up on this
in the original driver reviews but that's hindsight for you.
>> ---
>> drivers/iio/accel/st_accel.h | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iio/accel/st_accel.h b/drivers/iio/accel/st_accel.h
>> index c387763..d8d22e5 100644
>> --- a/drivers/iio/accel/st_accel.h
>> +++ b/drivers/iio/accel/st_accel.h
>> @@ -15,11 +15,11 @@
>> #include <linux/iio/common/st_sensors.h>
>>
>> #define LSM303DLHC_ACCEL_DEV_NAME "lsm303dlhc_accel"
>> -#define LIS3DH_ACCEL_DEV_NAME "lis3dh"
>> +#define LIS3DH_ACCEL_DEV_NAME "lis3dh_accel"
>> #define LSM330D_ACCEL_DEV_NAME "lsm330d_accel"
>> #define LSM330DL_ACCEL_DEV_NAME "lsm330dl_accel"
>> #define LSM330DLC_ACCEL_DEV_NAME "lsm330dlc_accel"
>> -#define LIS331DLH_ACCEL_DEV_NAME "lis331dlh"
>> +#define LIS331DLH_ACCEL_DEV_NAME "lis331dlh_accel"
>> #define LSM303DL_ACCEL_DEV_NAME "lsm303dl_accel"
>> #define LSM303DLH_ACCEL_DEV_NAME "lsm303dlh_accel"
>> #define LSM303DLM_ACCEL_DEV_NAME "lsm303dlm_accel"
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On 09/10/13 13:49, Lee Jones wrote:
> 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 <[email protected]>
Applied to the togreg branch of iio.git
Thanks
> ---
> 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 965ee22..eb261a5 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> @@ -235,10 +235,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 ceebd3c..16cfbc5 100644
> --- a/drivers/iio/pressure/st_pressure_core.c
> +++ b/drivers/iio/pressure/st_pressure_core.c
> @@ -226,8 +226,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_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;
>
> if (!plat_data)
>
On 09/10/13 13:49, Lee Jones wrote:
> Not all ST's sensors support data ready, so let's make the declaration
> of one conditional.
>
> Signed-off-by: Lee Jones <[email protected]>
Hi Lee,
This one doesn't actually build:
CC [M] drivers/iio/common/st_sensors/st_sensors_core.o
drivers/iio/common/st_sensors/st_sensors_core.c: In function 'st_sensors_set_drdy_int_pin':
drivers/iio/common/st_sensors/st_sensors_core.c:211:4: error: 'err' undeclared (first use in this function)
drivers/iio/common/st_sensors/st_sensors_core.c:211:4: note: each undeclared identifier is reported only once for each
function it appears in
drivers/iio/common/st_sensors/st_sensors_core.c:228:3: error: label 'init_error' used but not defined
make[4]: *** [drivers/iio/common/st_sensors/st_sensors_core.o] Error 1
The following patch gets rid of these lines so I'm guessing you reorganised your patch
series then didn't check buildling them in the new order.
Please fix this up before reposting the remainder of the series.
> ---
> drivers/iio/common/st_sensors/st_sensors_core.c | 24 +++++++++++++++++++-----
> drivers/iio/pressure/st_pressure_core.c | 3 ++-
> 2 files changed, 21 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 eb261a5..b86cad2 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> @@ -198,14 +198,11 @@ int st_sensors_set_axis_enable(struct iio_dev *indio_dev, u8 axis_enable)
> }
> EXPORT_SYMBOL(st_sensors_set_axis_enable);
>
> -int st_sensors_init_sensor(struct iio_dev *indio_dev,
> - struct st_sensors_platform_data *pdata)
> +int st_sensors_set_drdy_int_pin(struct iio_dev *indio_dev,
> + struct st_sensors_platform_data *pdata)
> {
> - int err;
> struct st_sensor_data *sdata = iio_priv(indio_dev);
>
> - mutex_init(&sdata->tb.buf_lock);
> -
> switch (pdata->drdy_int_pin) {
> case 1:
> if (sdata->sensor->drdy_irq.mask_int1 == 0) {
> @@ -231,6 +228,20 @@ int st_sensors_init_sensor(struct iio_dev *indio_dev,
> goto init_error;
> }
>
> + return 0;
> +}
> +
> +int st_sensors_init_sensor(struct iio_dev *indio_dev,
> + struct st_sensors_platform_data *pdata)
> +{
> + int err = 0;
> + struct st_sensor_data *sdata = iio_priv(indio_dev);
> +
> + mutex_init(&sdata->tb.buf_lock);
> +
> + if (pdata)
> + err = st_sensors_set_drdy_int_pin(indio_dev, pdata);
> +
> err = st_sensors_set_enable(indio_dev, false);
> if (err < 0)
> goto init_error;
> @@ -266,6 +277,9 @@ int st_sensors_set_dataready_irq(struct iio_dev *indio_dev, bool enable)
> u8 drdy_mask;
> struct st_sensor_data *sdata = iio_priv(indio_dev);
>
> + if (!sdata->sensor->drdy_irq.addr)
> + return 0;
> +
> /* Enable/Disable the interrupt generator 1. */
> if (sdata->sensor->drdy_irq.ig1.en_addr > 0) {
> err = st_sensors_write_data_with_mask(indio_dev,
> diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
> index 16cfbc5..279aafd 100644
> --- a/drivers/iio/pressure/st_pressure_core.c
> +++ b/drivers/iio/pressure/st_pressure_core.c
> @@ -232,7 +232,8 @@ int st_press_common_probe(struct iio_dev *indio_dev,
>
> pdata->odr = pdata->sensor->odr.odr_avl[0].hz;
>
> - if (!plat_data)
> + /* Some devices don't support a data ready pin. */
> + if (!plat_data && pdata->sensor->drdy_irq.addr)
> plat_data =
> (struct st_sensors_platform_data *)&default_press_pdata;
>
>
On 09/10/13 13:49, Lee Jones wrote:
> Strip out all those unnecessary gotos and just return the error right away.
>
> Aids to simplicity and reduces code.
>
> Signed-off-by: Lee Jones <[email protected]>
This is fine except in that it is intertwined with the previous patch.
> ---
> drivers/iio/common/st_sensors/st_sensors_core.c | 18 +++++++-----------
> 1 file changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
> index b86cad2..8c4c54c 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> @@ -208,8 +208,7 @@ int st_sensors_set_drdy_int_pin(struct iio_dev *indio_dev,
> if (sdata->sensor->drdy_irq.mask_int1 == 0) {
> dev_err(&indio_dev->dev,
> "DRDY on INT1 not available.\n");
> - err = -EINVAL;
> - goto init_error;
> + return -EINVAL;
> }
> sdata->drdy_int_pin = 1;
> break;
> @@ -217,15 +216,13 @@ int st_sensors_set_drdy_int_pin(struct iio_dev *indio_dev,
> if (sdata->sensor->drdy_irq.mask_int2 == 0) {
> dev_err(&indio_dev->dev,
> "DRDY on INT2 not available.\n");
> - err = -EINVAL;
> - goto init_error;
> + return -EINVAL;
> }
> sdata->drdy_int_pin = 2;
> break;
> default:
> dev_err(&indio_dev->dev, "DRDY on pdata not valid.\n");
> - err = -EINVAL;
> - goto init_error;
> + return -EINVAL;
> }
>
> return 0;
> @@ -244,29 +241,28 @@ int st_sensors_init_sensor(struct iio_dev *indio_dev,
>
> err = st_sensors_set_enable(indio_dev, false);
> if (err < 0)
> - goto init_error;
> + return err;
>
> if (sdata->current_fullscale) {
> err = st_sensors_set_fullscale(indio_dev,
> sdata->current_fullscale->num);
> if (err < 0)
> - goto init_error;
> + return err;
> } else
> dev_info(&indio_dev->dev, "Full-scale not possible\n");
>
> err = st_sensors_set_odr(indio_dev, sdata->odr);
> if (err < 0)
> - goto init_error;
> + return err;
>
> /* set BDU */
> err = st_sensors_write_data_with_mask(indio_dev,
> sdata->sensor->bdu.addr, sdata->sensor->bdu.mask, true);
> if (err < 0)
> - goto init_error;
> + return err;
>
> err = st_sensors_set_axis_enable(indio_dev, ST_SENSORS_ENABLE_ALL_AXIS);
>
> -init_error:
> return err;
> }
> EXPORT_SYMBOL(st_sensors_init_sensor);
>
On 09/10/13 13:49, Lee Jones wrote:
> Gets rid of those unnecessary gotos.
Unfortunately it introduced a bug whilst it is at it. Sometimes
those gotos are necessary and the 'right' way to do things.
>
> Signed-off-by: Lee Jones <[email protected]>
> ---
> drivers/iio/common/st_sensors/st_sensors_core.c | 17 +++++++----------
> 1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
> index 8c4c54c..148f0e5 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> @@ -331,26 +331,23 @@ static int st_sensors_read_axis_data(struct iio_dev *indio_dev,
> unsigned int byte_for_channel = ch->scan_type.storagebits >> 3;
>
> outdata = kmalloc(byte_for_channel, GFP_KERNEL);
> - if (!outdata) {
> - err = -EINVAL;
> - goto st_sensors_read_axis_data_error;
> - }
> + if (!outdata)
> + return -ENOMEM;
I agree this change makes complete sense.
>
> err = sdata->tf->read_multiple_byte(&sdata->tb, sdata->dev,
> ch->address, byte_for_channel,
> outdata, sdata->multiread_bit);
> - if (err < 0)
> - goto st_sensors_free_memory;
> + if (err < 0) {
> + kfree(outdata);
> + return err;
> + }
I don't like this change as....
>
> if (byte_for_channel == 2)
> *data = (s16)get_unaligned_le16(outdata);
> else if (byte_for_channel == 3)
> *data = (s32)st_sensors_get_unaligned_le24(outdata);
>
> -st_sensors_free_memory:
> - kfree(outdata);
you now don't free out data in the case of no error. This is precisely the case
where a goto is the correct way to handle things as the free need to occur whether
or not the error occurs.
> -st_sensors_read_axis_data_error:
> - return err;
> + return 0;
> }
>
> int st_sensors_read_info_raw(struct iio_dev *indio_dev,
>
On 09/10/13 13:49, Lee Jones wrote:
> Saving a few lines of code.
>
> Signed-off-by: Lee Jones <[email protected]>
Applied to the togreg branch of iio.git.
To my mind the key thing here is that the error paths were previous
inconsistent in that all but the last one went via a separate cleanup path
whereas the last one went straight through.
Now they are consistent and that is more important than saving a few lines of code.
Thanks,
> ---
> drivers/iio/common/st_sensors/st_sensors_core.c | 11 ++++-------
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
> index 148f0e5..25d4c7e 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> @@ -359,28 +359,25 @@ int st_sensors_read_info_raw(struct iio_dev *indio_dev,
> mutex_lock(&indio_dev->mlock);
> if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> err = -EBUSY;
> - goto read_error;
> + goto out;
> } else {
> err = st_sensors_set_enable(indio_dev, true);
> if (err < 0)
> - goto read_error;
> + goto out;
>
> msleep((sdata->sensor->bootime * 1000) / sdata->odr);
> err = st_sensors_read_axis_data(indio_dev, ch, val);
> if (err < 0)
> - goto read_error;
> + goto out;
>
> *val = *val >> ch->scan_type.shift;
>
> err = st_sensors_set_enable(indio_dev, false);
> }
> +out:
> mutex_unlock(&indio_dev->mlock);
>
> return err;
> -
> -read_error:
> - mutex_unlock(&indio_dev->mlock);
> - return err;
> }
> EXPORT_SYMBOL(st_sensors_read_info_raw);
>
>
On 09/14/13 17:45, Jonathan Cameron wrote:
> On 09/10/13 13:49, Lee Jones wrote:
>> 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 <[email protected]>
> Applied to the togreg branch of iio.git
>
Note that Denis Acked this in v1 and it hasn't changed substantially that I can see.
Hence you should have added that ack to this reposting. I nearly missed it.
> Thanks
>> ---
>> 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 965ee22..eb261a5 100644
>> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
>> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
>> @@ -235,10 +235,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 ceebd3c..16cfbc5 100644
>> --- a/drivers/iio/pressure/st_pressure_core.c
>> +++ b/drivers/iio/pressure/st_pressure_core.c
>> @@ -226,8 +226,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_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;
>>
>> if (!plat_data)
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On 09/10/13 13:49, Lee Jones wrote:
> 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 <[email protected]>
Very happy to see this patch. The _1_ stuff should never have gotten through
the initial review of the driver (oops). Even in cases where it might be shared
by multiple devices, it is better to have it named after one choice.
Anyhow, applied to the togreg branch of iio.git
Thanks
> ---
> drivers/iio/pressure/st_pressure_core.c | 94 ++++++++++++++++-----------------
> 1 file changed, 46 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
> index 279aafd..2ee4bcd 100644
> --- a/drivers/iio/pressure/st_pressure_core.c
> +++ b/drivers/iio/pressure/st_pressure_core.c
> @@ -36,94 +36,92 @@
> ST_PRESS_LSB_PER_CELSIUS)
> #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_KPASCAL_NANO_SCALE
> -#define ST_PRESS_1_FS_AVL_TEMP_GAIN ST_PRESS_CELSIUS_NANO_SCALE
> -#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_INT1_MASK 0x04
> -#define ST_PRESS_1_DRDY_IRQ_INT2_MASK 0x20
> -#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_KPASCAL_NANO_SCALE
> +#define ST_PRESS_LPS331AP_FS_AVL_TEMP_GAIN ST_PRESS_CELSIUS_NANO_SCALE
> +#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_INT1_MASK 0x04
> +#define ST_PRESS_LPS331AP_DRDY_IRQ_INT2_MASK 0x20
> +#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_int1 = ST_PRESS_1_DRDY_IRQ_INT1_MASK,
> - .mask_int2 = ST_PRESS_1_DRDY_IRQ_INT2_MASK,
> + .addr = ST_PRESS_LPS331AP_DRDY_IRQ_ADDR,
> + .mask_int1 = ST_PRESS_LPS331AP_DRDY_IRQ_INT1_MASK,
> + .mask_int2 = ST_PRESS_LPS331AP_DRDY_IRQ_INT2_MASK,
> },
> - .multi_read_bit = ST_PRESS_1_MULTIREAD_BIT,
> + .multi_read_bit = ST_PRESS_LPS331AP_MULTIREAD_BIT,
> .bootime = 2,
> },
> };
>
On 09/10/13 13:49, Lee Jones 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.
>
> Signed-off-by: Lee Jones <[email protected]>
Agreed that in this case it is clearer not to use a macro. When you have lots
of repeats (e.g. an adc with 16 channels) they can make sense, but here as
you say a few extra lines of code make it much easier to follow.
My only slight addition here would be to drop the IIO_NO_MOD and modified=0 bits
on the basis those are both the obvious defaults (and happen to be equal to 0).
Perhaps worth leaving them in this case as it makes the patch more obviously correct.
Applied to the togreg branch of iio.git
Thanks,
Jonathan
> ---
> 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 2ee4bcd..60c2ee4 100644
> --- a/drivers/iio/pressure/st_pressure_core.c
> +++ b/drivers/iio/pressure/st_pressure_core.c
> @@ -64,16 +64,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_lps331ap_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)
> };
>
> @@ -83,7 +106,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_lps331ap_channels,
> .odr = {
> .addr = ST_PRESS_LPS331AP_ODR_ADDR,
> .mask = ST_PRESS_LPS331AP_ODR_MASK,
> @@ -222,7 +245,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_lps331ap_channels);
>
> if (pdata->sensor->fs.addr != 0)
> pdata->current_fullscale = (struct st_sensor_fullscale_avl *)
>
On 09/10/13 13:49, Lee Jones wrote:
> 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 <[email protected]>
Applied with Denis ack (again it should have been here)
> ---
> 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 60c2ee4..3abada2 100644
> --- a/drivers/iio/pressure/st_pressure_core.c
> +++ b/drivers/iio/pressure/st_pressure_core.c
> @@ -107,6 +107,7 @@ static const struct st_sensors st_press_sensors[] = {
> [0] = LPS331AP_PRESS_DEV_NAME,
> },
> .ch = (struct iio_chan_spec *)st_press_lps331ap_channels,
> + .num_ch = ARRAY_SIZE(st_press_lps331ap_channels),
> .odr = {
> .addr = ST_PRESS_LPS331AP_ODR_ADDR,
> .mask = ST_PRESS_LPS331AP_ODR_MASK,
> @@ -245,7 +246,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_lps331ap_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 e51f654..e732fda 100644
> --- a/include/linux/iio/common/st_sensors.h
> +++ b/include/linux/iio/common/st_sensors.h
> @@ -184,6 +184,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;
>
On 09/11/13 08:19, Lee Jones wrote:
>>> err = st_sensors_init_sensor(indio_dev, plat_data);
>>> 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);
>>> + ST_PRESS_TRIGGER_OPS);
>>> if (err < 0)
>>> goto st_press_probe_trigger_error;
>>> }
>>>
>>> err = iio_device_register(indio_dev);
>>> - if (err)
>>
>> This bit of handling is confusing. I would much rather see the if IRQ at the goto. Here the first thought is why is it not an error if there is no IRQ!
>
> I certainly see your point. But surely anyone would see after a second
> or two that we're returning err and not 0 in this case, so the error
> would still be returned, we're not ignoring it. Adding this extra
> comparison saves several lines of code.
>
> If you think it's 'too' confusing, I'll revert the change.
>
> Or perhaps a comment:
>
> /* Only deallocate_[trigger|ring] if they were allocated. */
> or
> /* Only deallocate_[trigger|ring] if we have an IRQ line. */
> or
> /* If no IRQ was specified, just return the error. */
>
Revert the change. Whilst not complex to follow it is non obvious to save only
a couple of lines. Adding a comment would take almost as much space as just doing
it the 'easy way'.
>>> + if (err && irq > 0)
>>> goto st_press_device_register_error;
>>>
>>> return err;
>>>
>>> st_press_device_register_error:
>>> - if (pdata->get_irq_data_ready(indio_dev) > 0)
>>> - st_sensors_deallocate_trigger(indio_dev);
>>> + 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:
>>> + st_press_deallocate_ring(indio_dev);
>>> +
>>> return err;
>>> }
>>> EXPORT_SYMBOL(st_press_common_probe);
>>
>
On 09/10/13 13:49, Lee Jones wrote:
> Here we use existing practices to introduce support for another
> pressure/temperature sensor, the LPS001WP.
>
> Signed-off-by: Lee Jones <[email protected]>
Looks fine to me, will pick this up when the precursors are all sorted.
> ---
> 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 f1bebce..36b1cc6 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_press"
> +#define LPS001WP_PRESS_DEV_NAME "lps001wp_press"
>
> /**
> * struct st_sensors_platform_data - default press platform data
> diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
> index 34b3fb1..b42614a 100644
> --- a/drivers/iio/pressure/st_pressure_core.c
> +++ b/drivers/iio/pressure/st_pressure_core.c
> @@ -64,6 +64,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_lps331ap_channels[] = {
> {
> .type = IIO_PRESSURE,
> @@ -100,6 +115,40 @@ static const struct iio_chan_spec st_press_lps331ap_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,
> @@ -148,6 +197,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 08aac5e..ada5cbd 100644
> --- a/drivers/iio/pressure/st_pressure_i2c.c
> +++ b/drivers/iio/pressure/st_pressure_i2c.c
> @@ -50,6 +50,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);
>
On 09/10/13 13:49, 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. This patch allows for
> the Vdd power supply to be specified by either platform data or Device
> Tree.
>
> Signed-off-by: Lee Jones <[email protected]>
Fine, will pick up with the rest. This optional regulator stuff is nice as
it gets around the annoying platform specific callbacks that tend to do stuff
like this.
If anyone is bored, I suspect there are quite a few cases where this makes sense
in other IIO drivers!
> ---
> drivers/iio/pressure/st_pressure_core.c | 28 ++++++++++++++++++++++++++++
> include/linux/iio/common/st_sensors.h | 3 +++
> 2 files changed, 31 insertions(+)
>
> diff --git a/drivers/iio/pressure/st_pressure_core.c b/drivers/iio/pressure/st_pressure_core.c
> index b42614a..d52b487 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>
> @@ -313,6 +314,29 @@ static const struct iio_trigger_ops st_press_trigger_ops = {
> #define ST_PRESS_TRIGGER_OPS NULL
> #endif
>
> +void st_press_power_enable(struct iio_dev *indio_dev)
> +{
> + struct st_sensor_data *pdata = iio_priv(indio_dev);
> + int err;
> +
> + /* Regulators not mandatory, but if requested we should enable it. */
> + pdata->vdd = devm_regulator_get_optional(&indio_dev->dev, "vdd");
> + if (!IS_ERR(pdata->vdd)) {
> + err = regulator_enable(pdata->vdd);
> + if (err != 0)
> + dev_warn(&indio_dev->dev,
> + "Failed to enable specified Vdd supply\n");
> + }
> +}
> +
> +void st_press_power_disable(struct iio_dev *indio_dev)
> +{
> + struct st_sensor_data *pdata = iio_priv(indio_dev);
> +
> + if (!IS_ERR(pdata->vdd))
> + regulator_disable(pdata->vdd);
> +}
> +
> int st_press_common_probe(struct iio_dev *indio_dev,
> struct st_sensors_platform_data *plat_data)
> {
> @@ -323,6 +347,8 @@ int st_press_common_probe(struct iio_dev *indio_dev,
> indio_dev->modes = INDIO_DIRECT_MODE;
> indio_dev->info = &press_info;
>
> + st_press_power_enable(indio_dev);
> +
> err = st_sensors_check_device_support(indio_dev,
> ARRAY_SIZE(st_press_sensors),
> st_press_sensors);
> @@ -382,6 +408,8 @@ void st_press_common_remove(struct iio_dev *indio_dev)
> {
> struct st_sensor_data *pdata = iio_priv(indio_dev);
>
> + st_press_power_disable(indio_dev);
> +
> 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 e732fda..968b84e 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>
>
> #include <linux/platform_data/st_sensors_pdata.h>
>
> @@ -201,6 +202,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.
> + * @vdd: Pointer to sensor's Vdd 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.
> @@ -216,6 +218,7 @@ struct st_sensor_data {
> struct iio_trigger *trig;
> struct st_sensors *sensor;
> struct st_sensor_fullscale_avl *current_fullscale;
> + struct regulator *vdd;
>
> bool enabled;
> bool multiread_bit;
>
On 09/10/13 13:49, Lee Jones wrote:
> Reduce the amount of those unnecessary goto calls, as in most cases
> we can simply return immediately. We also only call for the IRQ number
> once and use that value throughout.
>
> Signed-off-by: Lee Jones <[email protected]>
...
> - if (adata->get_irq_data_ready(indio_dev) > 0) {
> + if (irq > 0) {
> err = st_accel_allocate_ring(indio_dev);
> if (err < 0)
> - goto st_accel_common_probe_error;
> + return err;
>
> err = st_sensors_allocate_trigger(indio_dev,
> ST_ACCEL_TRIGGER_OPS);
> @@ -492,18 +493,16 @@ int st_accel_common_probe(struct iio_dev *indio_dev,
> }
>
> err = iio_device_register(indio_dev);
> - if (err)
> + if (err && irq > 0)
> goto st_accel_device_register_error;
This is the same structure I moaned about earlier. Again, put the if (irq > 0) in the error handling
not here.
>
> return err;
>
> st_accel_device_register_error:
> - if (adata->get_irq_data_ready(indio_dev) > 0)
> - st_sensors_deallocate_trigger(indio_dev);
> + st_sensors_deallocate_trigger(indio_dev);
> st_accel_probe_trigger_error:
> - if (adata->get_irq_data_ready(indio_dev) > 0)
> - st_accel_deallocate_ring(indio_dev);
> -st_accel_common_probe_error:
> + st_accel_deallocate_ring(indio_dev);
> +
> return err;
> }
> EXPORT_SYMBOL(st_accel_common_probe);
>
On 09/10/13 13:49, Lee Jones wrote:
> The LSM303DLH's WAI (WhoAmI) is 0x33, meaning it should be enabled by
> Accel Sensor group one. For the device to probe without error, we'll
> need to ensure it's registered with the correct WAI.
>
> Signed-off-by: Lee Jones <[email protected]>
You clearly have a better datasheet than I have as for that part it doesn't even claim to
have the relevant register to read a who am I from.
Now that datasheet does list odr values as 50, 100, 400 1000 which would put it where it originally
was in these tables.
http://www.st.com/web/en/resource/technical/document/datasheet/CD00260288.pdf
I haven't checked other elements...
I'm confused but suspect we may need another type entry to deal with this one.
> ---
> drivers/iio/accel/st_accel_core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
> index ea62291..03a2b6b 100644
> --- a/drivers/iio/accel/st_accel_core.c
> +++ b/drivers/iio/accel/st_accel_core.c
> @@ -170,6 +170,7 @@ static const struct st_sensors st_accel_sensors[] = {
> [2] = LSM330D_ACCEL_DEV_NAME,
> [3] = LSM330DL_ACCEL_DEV_NAME,
> [4] = LSM330DLC_ACCEL_DEV_NAME,
> + [5] = LSM303DLH_ACCEL_DEV_NAME,
> },
> .ch = (struct iio_chan_spec *)st_accel_12bit_channels,
> .odr = {
> @@ -238,8 +239,7 @@ static const struct st_sensors st_accel_sensors[] = {
> .sensors_supported = {
> [0] = LIS331DLH_ACCEL_DEV_NAME,
> [1] = LSM303DL_ACCEL_DEV_NAME,
> - [2] = LSM303DLH_ACCEL_DEV_NAME,
> - [3] = LSM303DLM_ACCEL_DEV_NAME,
> + [2] = LSM303DLM_ACCEL_DEV_NAME,
> },
> .ch = (struct iio_chan_spec *)st_accel_12bit_channels,
> .odr = {
>
On 09/10/13 13:49, Lee Jones wrote:
> Trivial patch adding the LSM303DLH to the list of already supported
> Magnetometer Sensors.
>
> Signed-off-by: Lee Jones <[email protected]>
err.
> index 12e7e79..b2e2917 100644
> --- a/drivers/iio/magnetometer/st_magn_core.c
> +++ b/drivers/iio/magnetometer/st_magn_core.c
> @@ -151,7 +151,8 @@ static const struct st_sensors st_magn_sensors[] = {
> .wai = ST_MAGN_1_WAI_EXP,
> .sensors_supported = {
> [0] = LSM303DLHC_MAGN_DEV_NAME,
> - [1] = LSM303DLM_MAGN_DEV_NAME,
> + [1] = LSM303DLH_MAGN_DEV_NAME,
> + [0] = LSM303DLM_MAGN_DEV_NAME,
[2]?
> },
> .ch = (struct iio_chan_spec *)st_magn_16bit_channels,
> .odr = {
>
On Sat, 14 Sep 2013, Jonathan Cameron wrote:
> On 09/10/13 13:49, Lee Jones wrote:
> > Trivial patch adding the LSM303DLH to the list of already supported
> > Magnetometer Sensors.
> >
> > Signed-off-by: Lee Jones <[email protected]>
> err.
?
> > index 12e7e79..b2e2917 100644
> > --- a/drivers/iio/magnetometer/st_magn_core.c
> > +++ b/drivers/iio/magnetometer/st_magn_core.c
> > @@ -151,7 +151,8 @@ static const struct st_sensors st_magn_sensors[] = {
> > .wai = ST_MAGN_1_WAI_EXP,
> > .sensors_supported = {
> > [0] = LSM303DLHC_MAGN_DEV_NAME,
> > - [1] = LSM303DLM_MAGN_DEV_NAME,
> > + [1] = LSM303DLH_MAGN_DEV_NAME,
> > + [0] = LSM303DLM_MAGN_DEV_NAME,
> [2]?
Yes, good spot, thanks.
> > },
> > .ch = (struct iio_chan_spec *)st_magn_16bit_channels,
> > .odr = {
> >
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Sat, 14 Sep 2013, Jonathan Cameron wrote:
> On 09/10/13 13:49, Lee Jones wrote:
> > The LSM303DLH's WAI (WhoAmI) is 0x33, meaning it should be enabled by
> > Accel Sensor group one. For the device to probe without error, we'll
> > need to ensure it's registered with the correct WAI.
> >
> > Signed-off-by: Lee Jones <[email protected]>
> You clearly have a better datasheet than I have as for that part it doesn't even claim to
> have the relevant register to read a who am I from.
>
> Now that datasheet does list odr values as 50, 100, 400 1000 which would put it where it originally
> was in these tables.
>
> http://www.st.com/web/en/resource/technical/document/datasheet/CD00260288.pdf
>
> I haven't checked other elements...
>
> I'm confused but suspect we may need another type entry to deal with this one.
Hmmm... on initial thought, I don't know how we can handle this one. I
assumed the WAIs were a one-stop-shop for device identification. I also
thought that they would only match if the functionality and register
maps would be the same.
I'll touch base with Denis on this, perhaps he can shed some light.
> > ---
> > drivers/iio/accel/st_accel_core.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
> > index ea62291..03a2b6b 100644
> > --- a/drivers/iio/accel/st_accel_core.c
> > +++ b/drivers/iio/accel/st_accel_core.c
> > @@ -170,6 +170,7 @@ static const struct st_sensors st_accel_sensors[] = {
> > [2] = LSM330D_ACCEL_DEV_NAME,
> > [3] = LSM330DL_ACCEL_DEV_NAME,
> > [4] = LSM330DLC_ACCEL_DEV_NAME,
> > + [5] = LSM303DLH_ACCEL_DEV_NAME,
> > },
> > .ch = (struct iio_chan_spec *)st_accel_12bit_channels,
> > .odr = {
> > @@ -238,8 +239,7 @@ static const struct st_sensors st_accel_sensors[] = {
> > .sensors_supported = {
> > [0] = LIS331DLH_ACCEL_DEV_NAME,
> > [1] = LSM303DL_ACCEL_DEV_NAME,
> > - [2] = LSM303DLH_ACCEL_DEV_NAME,
> > - [3] = LSM303DLM_ACCEL_DEV_NAME,
> > + [2] = LSM303DLM_ACCEL_DEV_NAME,
> > },
> > .ch = (struct iio_chan_spec *)st_accel_12bit_channels,
> > .odr = {
> >
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Sat, 14 Sep 2013, Jonathan Cameron wrote:
> On 09/10/13 13:49, Lee Jones wrote:
> > 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 <[email protected]>
> Applied with Denis ack (again it should have been here)
Ah, you're going to take the patches one-by-one?
After amending the drivers as per your review I was going to send a
final time with all Acks applied. Then, once all maintainer Acks were
obtained I was going to send you a pull-request.
Perhaps I should have communicated the intent sooner.
Future submissions will have all Acks applied in this case.
> > ---
> > 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 60c2ee4..3abada2 100644
> > --- a/drivers/iio/pressure/st_pressure_core.c
> > +++ b/drivers/iio/pressure/st_pressure_core.c
> > @@ -107,6 +107,7 @@ static const struct st_sensors st_press_sensors[] = {
> > [0] = LPS331AP_PRESS_DEV_NAME,
> > },
> > .ch = (struct iio_chan_spec *)st_press_lps331ap_channels,
> > + .num_ch = ARRAY_SIZE(st_press_lps331ap_channels),
> > .odr = {
> > .addr = ST_PRESS_LPS331AP_ODR_ADDR,
> > .mask = ST_PRESS_LPS331AP_ODR_MASK,
> > @@ -245,7 +246,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_lps331ap_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 e51f654..e732fda 100644
> > --- a/include/linux/iio/common/st_sensors.h
> > +++ b/include/linux/iio/common/st_sensors.h
> > @@ -184,6 +184,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;
> >
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Sat, 14 Sep 2013, Jonathan Cameron wrote:
> On 09/10/13 13:49, Lee Jones wrote:
> > Gets rid of those unnecessary gotos.
>
> Unfortunately it introduced a bug whilst it is at it. Sometimes
> those gotos are necessary and the 'right' way to do things.
> >
> > Signed-off-by: Lee Jones <[email protected]>
> > ---
> > drivers/iio/common/st_sensors/st_sensors_core.c | 17 +++++++----------
> > 1 file changed, 7 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
> > index 8c4c54c..148f0e5 100644
> > --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> > +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> > @@ -331,26 +331,23 @@ static int st_sensors_read_axis_data(struct iio_dev *indio_dev,
> > unsigned int byte_for_channel = ch->scan_type.storagebits >> 3;
> >
> > outdata = kmalloc(byte_for_channel, GFP_KERNEL);
> > - if (!outdata) {
> > - err = -EINVAL;
> > - goto st_sensors_read_axis_data_error;
> > - }
> > + if (!outdata)
> > + return -ENOMEM;
> I agree this change makes complete sense.
> >
> > err = sdata->tf->read_multiple_byte(&sdata->tb, sdata->dev,
> > ch->address, byte_for_channel,
> > outdata, sdata->multiread_bit);
> > - if (err < 0)
> > - goto st_sensors_free_memory;
> > + if (err < 0) {
> > + kfree(outdata);
> > + return err;
> > + }
> I don't like this change as....
> >
> > if (byte_for_channel == 2)
> > *data = (s16)get_unaligned_le16(outdata);
> > else if (byte_for_channel == 3)
> > *data = (s32)st_sensors_get_unaligned_le24(outdata);
> >
> > -st_sensors_free_memory:
> > - kfree(outdata);
> you now don't free out data in the case of no error. This is precisely the case
> where a goto is the correct way to handle things as the free need to occur whether
> or not the error occurs.
Ah, you're quite right. I'll fix up.
> > -st_sensors_read_axis_data_error:
> > - return err;
> > + return 0;
> > }
> >
> > int st_sensors_read_info_raw(struct iio_dev *indio_dev,
> >
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On 09/14/2013 02:27 PM, Jonathan Cameron wrote:
> On 09/14/13 13:14, Jonathan Cameron wrote:
>> On 09/10/13 13:49, Lee Jones wrote:
>>> Some of ST's sensors are appended with their sensor type and some
>>> are not. For consistency we're extending the same naming convention
>>> throughout.
>>>
>>> Signed-off-by: Lee Jones <[email protected]>
>> Honestly I don't care either way on these, but consistency would definitely
>> be good so applied to the togreg branch of iio.git
>>
>> Thanks,
>
> Actually change of plan. I'm going to hold off on these as this an ABI change.
> Iritating though having these as completely inconsistent is, changing this
> will change device identification from userspace which is not a good idea.
>
> Sorry Lee, but we really shouldn't do this. I should have picked up on this
> in the original driver reviews but that's hindsight for you.
I think those with the suffix are the ones where you have accel and gyro in
the same package, the ones without the suffix are pure accelerometers.
- Lars
Hi Lee,
>> On 09/10/13 13:49, Lee Jones wrote:
>>> The LSM303DLH's WAI (WhoAmI) is 0x33, meaning it should be enabled by
>>> Accel Sensor group one. For the device to probe without error, we'll
>>> need to ensure it's registered with the correct WAI.
>>>
>>> Signed-off-by: Lee Jones <[email protected]>
>> You clearly have a better datasheet than I have as for that part it doesn't even claim to
>> have the relevant register to read a who am I from.
>>
>> Now that datasheet does list odr values as 50, 100, 400 1000 which would put it where it originally
>> was in these tables.
>>
>> http://www.st.com/web/en/resource/technical/document/datasheet/CD00260288.pdf
>>
>> I haven't checked other elements...
>>
>> I'm confused but suspect we may need another type entry to deal with this one.
> Hmmm... on initial thought, I don't know how we can handle this one. I
> assumed the WAIs were a one-stop-shop for device identification. I also
> thought that they would only match if the functionality and register
> maps would be the same.
>
> I'll touch base with Denis on this, perhaps he can shed some light.
>
I've just checked, the WAI is 0x32. It was correct...Are you sure that
you use the LSM303DLH?
Denis
>>> ---
>>> drivers/iio/accel/st_accel_core.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
>>> index ea62291..03a2b6b 100644
>>> --- a/drivers/iio/accel/st_accel_core.c
>>> +++ b/drivers/iio/accel/st_accel_core.c
>>> @@ -170,6 +170,7 @@ static const struct st_sensors st_accel_sensors[] = {
>>> [2] = LSM330D_ACCEL_DEV_NAME,
>>> [3] = LSM330DL_ACCEL_DEV_NAME,
>>> [4] = LSM330DLC_ACCEL_DEV_NAME,
>>> + [5] = LSM303DLH_ACCEL_DEV_NAME,
>>> },
>>> .ch = (struct iio_chan_spec *)st_accel_12bit_channels,
>>> .odr = {
>>> @@ -238,8 +239,7 @@ static const struct st_sensors st_accel_sensors[] = {
>>> .sensors_supported = {
>>> [0] = LIS331DLH_ACCEL_DEV_NAME,
>>> [1] = LSM303DL_ACCEL_DEV_NAME,
>>> - [2] = LSM303DLH_ACCEL_DEV_NAME,
>>> - [3] = LSM303DLM_ACCEL_DEV_NAME,
>>> + [2] = LSM303DLM_ACCEL_DEV_NAME,
>>> },
>>> .ch = (struct iio_chan_spec *)st_accel_12bit_channels,
>>> .odr = {
>>>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
Hi Lars,
> On 09/14/2013 02:27 PM, Jonathan Cameron wrote:
>> On 09/14/13 13:14, Jonathan Cameron wrote:
>>> On 09/10/13 13:49, Lee Jones wrote:
>>>> Some of ST's sensors are appended with their sensor type and some
>>>> are not. For consistency we're extending the same naming convention
>>>> throughout.
>>>>
>>>> Signed-off-by: Lee Jones <[email protected]>
>>> Honestly I don't care either way on these, but consistency would definitely
>>> be good so applied to the togreg branch of iio.git
>>>
>>> Thanks,
>> Actually change of plan. I'm going to hold off on these as this an ABI change.
>> Iritating though having these as completely inconsistent is, changing this
>> will change device identification from userspace which is not a good idea.
>>
>> Sorry Lee, but we really shouldn't do this. I should have picked up on this
>> in the original driver reviews but that's hindsight for you.
> I think those with the suffix are the ones where you have accel and gyro in
> the same package, the ones without the suffix are pure accelerometers.
Yes You are right. I've used this kind of logic... What do you think about?
Denis
Hi Lee,
> index 12e7e79..b2e2917 100644
> --- a/drivers/iio/magnetometer/st_magn_core.c
> +++ b/drivers/iio/magnetometer/st_magn_core.c
> @@ -151,7 +151,8 @@ static const struct st_sensors st_magn_sensors[] = {
> .wai = ST_MAGN_1_WAI_EXP,
> .sensors_supported = {
> [0] = LSM303DLHC_MAGN_DEV_NAME,
> - [1] = LSM303DLM_MAGN_DEV_NAME,
> + [1] = LSM303DLH_MAGN_DEV_NAME,
> + [0] = LSM303DLM_MAGN_DEV_NAME,
Are you sure that it works?
Because LSM303DLH magnetometer use different kind of WAI registers. This
driver read 0x0f and in this case the value is 0 and the driver do not
probe...You have to read the identification registers (0x0a, 0x0b, 0x0c)
in that case.
Denis????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Mon, 16 Sep 2013, Denis CIOCCA wrote:
> Hi Lee,
> >> On 09/10/13 13:49, Lee Jones wrote:
> >>> The LSM303DLH's WAI (WhoAmI) is 0x33, meaning it should be enabled by
> >>> Accel Sensor group one. For the device to probe without error, we'll
> >>> need to ensure it's registered with the correct WAI.
> >>>
> >>> Signed-off-by: Lee Jones <[email protected]>
> >> You clearly have a better datasheet than I have as for that part it doesn't even claim to
> >> have the relevant register to read a who am I from.
> >>
> >> Now that datasheet does list odr values as 50, 100, 400 1000 which would put it where it originally
> >> was in these tables.
> >>
> >> http://www.st.com/web/en/resource/technical/document/datasheet/CD00260288.pdf
> >>
> >> I haven't checked other elements...
> >>
> >> I'm confused but suspect we may need another type entry to deal with this one.
> > Hmmm... on initial thought, I don't know how we can handle this one. I
> > assumed the WAIs were a one-stop-shop for device identification. I also
> > thought that they would only match if the functionality and register
> > maps would be the same.
> >
> > I'll touch base with Denis on this, perhaps he can shed some light.
> >
> I've just checked, the WAI is 0x32. It was correct...Are you sure that
> you use the LSM303DLH?
That's what the datasheet for the board says. Perhaps it's that that's
incorrect? Annoyingly, instead of printing the device name on the
package, ST put some non-Googleable nonsense is there instead
(probably the serial number).
Nevertheless, I'll revert the patch and work something else out.
> >>> ---
> >>> drivers/iio/accel/st_accel_core.c | 4 ++--
> >>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/accel/st_accel_core.c b/drivers/iio/accel/st_accel_core.c
> >>> index ea62291..03a2b6b 100644
> >>> --- a/drivers/iio/accel/st_accel_core.c
> >>> +++ b/drivers/iio/accel/st_accel_core.c
> >>> @@ -170,6 +170,7 @@ static const struct st_sensors st_accel_sensors[] = {
> >>> [2] = LSM330D_ACCEL_DEV_NAME,
> >>> [3] = LSM330DL_ACCEL_DEV_NAME,
> >>> [4] = LSM330DLC_ACCEL_DEV_NAME,
> >>> + [5] = LSM303DLH_ACCEL_DEV_NAME,
> >>> },
> >>> .ch = (struct iio_chan_spec *)st_accel_12bit_channels,
> >>> .odr = {
> >>> @@ -238,8 +239,7 @@ static const struct st_sensors st_accel_sensors[] = {
> >>> .sensors_supported = {
> >>> [0] = LIS331DLH_ACCEL_DEV_NAME,
> >>> [1] = LSM303DL_ACCEL_DEV_NAME,
> >>> - [2] = LSM303DLH_ACCEL_DEV_NAME,
> >>> - [3] = LSM303DLM_ACCEL_DEV_NAME,
> >>> + [2] = LSM303DLM_ACCEL_DEV_NAME,
> >>> },
> >>> .ch = (struct iio_chan_spec *)st_accel_12bit_channels,
> >>> .odr = {
> >>>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
> echo "working:"
> cat /sys/bus/iio/devices/*/name
>
> isn't too much digging. If you really want to do that on your machine - do so in rc.local
Okay, I was going round the long way, which is why is was difficult:
/sys/class/i2c-adapter/i2c-2/2-005c/iio\:device2/name
I will revert the "registered" patches.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Hi Lee,
> That's what the datasheet for the board says. Perhaps it's that that's
> incorrect? Annoyingly, instead of printing the device name on the
> package, ST put some non-Googleable nonsense is there instead
> (probably the serial number). Nevertheless, I'll revert the patch and
> work something else out.
I've tried with real device...Unfortunately, you are right for name on
the package...
So, are you using the LSM303DLHC?
Denis????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Mon, 16 Sep 2013, Denis CIOCCA wrote:
> Hi Lee,
> > index 12e7e79..b2e2917 100644
> > --- a/drivers/iio/magnetometer/st_magn_core.c
> > +++ b/drivers/iio/magnetometer/st_magn_core.c
> > @@ -151,7 +151,8 @@ static const struct st_sensors st_magn_sensors[] = {
> > .wai = ST_MAGN_1_WAI_EXP,
> > .sensors_supported = {
> > [0] = LSM303DLHC_MAGN_DEV_NAME,
> > - [1] = LSM303DLM_MAGN_DEV_NAME,
> > + [1] = LSM303DLH_MAGN_DEV_NAME,
> > + [0] = LSM303DLM_MAGN_DEV_NAME,
> Are you sure that it works?
Well I have tested it working.
> Because LSM303DLH magnetometer use different kind of WAI registers. This
> driver read 0x0f and in this case the value is 0 and the driver do not
> probe...You have to read the identification registers (0x0a, 0x0b, 0x0c)
> in that case.
The driver mentions that the first group of sensors have a WAI of 0x3c:
#define ST_MAGN_1_WAI_EXP 0x3c
And when I print out the WAI read from the device:
Requested device: lsm303dlh_magn - read WAI: 0x3c
I guess I could have been lied to again by the board's datasheet again
and the attached device isn't actually a LSM303DLH, but how can I tell
for sure?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Hi Lee,
> The driver mentions that the first group of sensors have a WAI of
> 0x3c: #define ST_MAGN_1_WAI_EXP 0x3c And when I print out the WAI read
> from the device: Requested device: lsm303dlh_magn - read WAI: 0x3c I
> guess I could have been lied to again by the board's datasheet again
> and the attached device isn't actually a LSM303DLH, but how can I tell
> for sure?
On LSM303DLH you have to read 0x0a 0x0b and 0x0c registers, you have to
find 0x48, 0x34 and 0x33. I think in the snowball there is the LSM303DLHC.
Denis????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
> > That's what the datasheet for the board says. Perhaps it's that that's
> > incorrect? Annoyingly, instead of printing the device name on the
> > package, ST put some non-Googleable nonsense is there instead
> > (probably the serial number). Nevertheless, I'll revert the patch and
> > work something else out.
>
> I've tried with real device...Unfortunately, you are right for name on
> the package...
> So, are you using the LSM303DLHC?
According to my datasheet [1], I have the LSM303DLH.
[1] http://www.calao-systems.com/repository/pub/EMBEDDED%20COMPUTERS/SKY-S9500-ULP-XXX/C12-SDK/SCH-00103-B11.pdf
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
>> I've tried with real device...Unfortunately, you are right for name on
>> the package...
>> So, are you using the LSM303DLHC?
> According to my datasheet [1], I have the LSM303DLH.
>
> [1] http://www.calao-systems.com/repository/pub/EMBEDDED%20COMPUTERS/SKY-S9500-ULP-XXX/C12-SDK/SCH-00103-B11.pdf
>
Mhh...I saw the LSM303DLHC...
The LSM303DLH is not populated, there is only the footprint but is not
soldered...
Denis????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Mon, 16 Sep 2013, Denis CIOCCA wrote:
> >> I've tried with real device...Unfortunately, you are right for name on
> >> the package...
> >> So, are you using the LSM303DLHC?
> > According to my datasheet [1], I have the LSM303DLH.
> >
> > [1] http://www.calao-systems.com/repository/pub/EMBEDDED%20COMPUTERS/SKY-S9500-ULP-XXX/C12-SDK/SCH-00103-B11.pdf
> >
> Mhh...I saw the LSM303DLHC...
> The LSM303DLH is not populated, there is only the footprint but is not
> soldered...
The documentation says is the other way round.
See the big "Not populated" box around the LSM303DLHC?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, 16 Sep 2013, Denis CIOCCA wrote:
> Hi Lee,
>
> > The driver mentions that the first group of sensors have a WAI of
> > 0x3c: #define ST_MAGN_1_WAI_EXP 0x3c And when I print out the WAI read
> > from the device: Requested device: lsm303dlh_magn - read WAI: 0x3c I
> > guess I could have been lied to again by the board's datasheet again
> > and the attached device isn't actually a LSM303DLH, but how can I tell
> > for sure?
> On LSM303DLH you have to read 0x0a 0x0b and 0x0c registers, you have to
> find 0x48, 0x34 and 0x33. I think in the snowball there is the LSM303DLHC.
I'm inclined to agree with you. I'll make the necessary changes.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, 16 Sep 2013, Denis CIOCCA wrote:
> Hi Lee,
>
> > The driver mentions that the first group of sensors have a WAI of
> > 0x3c: #define ST_MAGN_1_WAI_EXP 0x3c And when I print out the WAI read
> > from the device: Requested device: lsm303dlh_magn - read WAI: 0x3c I
> > guess I could have been lied to again by the board's datasheet again
> > and the attached device isn't actually a LSM303DLH, but how can I tell
> > for sure?
> On LSM303DLH you have to read 0x0a 0x0b and 0x0c registers, you have to
> find 0x48, 0x34 and 0x33. I think in the snowball there is the LSM303DLHC.
I just did that too:
Device: lsm303dlh_magn: WAI: (0x3c)
st_magn_common_probe(): IR: ID1 (0x48) ID2 (0x34) ID3 (0x33)
It appears the LSM303DLH does have a populated WAI register after all.
Now what? :)
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On 09/16/2013 11:21 AM, Denis CIOCCA wrote:
> Hi Lars,
>> On 09/14/2013 02:27 PM, Jonathan Cameron wrote:
>>> On 09/14/13 13:14, Jonathan Cameron wrote:
>>>> On 09/10/13 13:49, Lee Jones wrote:
>>>>> Some of ST's sensors are appended with their sensor type and some
>>>>> are not. For consistency we're extending the same naming convention
>>>>> throughout.
>>>>>
>>>>> Signed-off-by: Lee Jones <[email protected]>
>>>> Honestly I don't care either way on these, but consistency would definitely
>>>> be good so applied to the togreg branch of iio.git
>>>>
>>>> Thanks,
>>> Actually change of plan. I'm going to hold off on these as this an ABI change.
>>> Iritating though having these as completely inconsistent is, changing this
>>> will change device identification from userspace which is not a good idea.
>>>
>>> Sorry Lee, but we really shouldn't do this. I should have picked up on this
>>> in the original driver reviews but that's hindsight for you.
>> I think those with the suffix are the ones where you have accel and gyro in
>> the same package, the ones without the suffix are pure accelerometers.
> Yes You are right. I've used this kind of logic... What do you think about?
>
> Denis
Seems sensible to me.
> Honestly I don't care either way on these, but consistency would definitely
> be good so applied to the togreg branch of iio.git
For clarification, is togreg due for the v3.12 merge window or will be
get a settling period in -next?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On 09/16/2013 03:31 PM, Lee Jones wrote:
>> Honestly I don't care either way on these, but consistency would definitely
>> be good so applied to the togreg branch of iio.git
>
> For clarification, is togreg due for the v3.12 merge window or will be
> get a settling period in -next?
>
Well everything up to the 'iio-for-3.12c' tag will be in v3.12, everything
else not.
- Lars
Lee Jones <[email protected]> wrote:
>On Sat, 14 Sep 2013, Jonathan Cameron wrote:
>
>> On 09/10/13 13:49, Lee Jones wrote:
>> > 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 <[email protected]>
>> Applied with Denis ack (again it should have been here)
>
>Ah, you're going to take the patches one-by-one?
>
>After amending the drivers as per your review I was going to send a
>final time with all Acks applied. Then, once all maintainer Acks were
>obtained I was going to send you a pull-request.
>
>Perhaps I should have communicated the intent sooner.
>
>Future submissions will have all Acks applied in this case.
Quite a few of this series stood alone just fine so I grabbed them :)
>
>> > ---
>> > 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 60c2ee4..3abada2 100644
>> > --- a/drivers/iio/pressure/st_pressure_core.c
>> > +++ b/drivers/iio/pressure/st_pressure_core.c
>> > @@ -107,6 +107,7 @@ static const struct st_sensors
>st_press_sensors[] = {
>> > [0] = LPS331AP_PRESS_DEV_NAME,
>> > },
>> > .ch = (struct iio_chan_spec *)st_press_lps331ap_channels,
>> > + .num_ch = ARRAY_SIZE(st_press_lps331ap_channels),
>> > .odr = {
>> > .addr = ST_PRESS_LPS331AP_ODR_ADDR,
>> > .mask = ST_PRESS_LPS331AP_ODR_MASK,
>> > @@ -245,7 +246,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_lps331ap_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 e51f654..e732fda 100644
>> > --- a/include/linux/iio/common/st_sensors.h
>> > +++ b/include/linux/iio/common/st_sensors.h
>> > @@ -184,6 +184,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;
>> >
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
On 09/16/13 13:03, Lars-Peter Clausen wrote:
> On 09/16/2013 11:21 AM, Denis CIOCCA wrote:
>> Hi Lars,
>>> On 09/14/2013 02:27 PM, Jonathan Cameron wrote:
>>>> On 09/14/13 13:14, Jonathan Cameron wrote:
>>>>> On 09/10/13 13:49, Lee Jones wrote:
>>>>>> Some of ST's sensors are appended with their sensor type and some
>>>>>> are not. For consistency we're extending the same naming convention
>>>>>> throughout.
>>>>>>
>>>>>> Signed-off-by: Lee Jones <[email protected]>
>>>>> Honestly I don't care either way on these, but consistency would definitely
>>>>> be good so applied to the togreg branch of iio.git
>>>>>
>>>>> Thanks,
>>>> Actually change of plan. I'm going to hold off on these as this an ABI change.
>>>> Iritating though having these as completely inconsistent is, changing this
>>>> will change device identification from userspace which is not a good idea.
>>>>
>>>> Sorry Lee, but we really shouldn't do this. I should have picked up on this
>>>> in the original driver reviews but that's hindsight for you.
>>> I think those with the suffix are the ones where you have accel and gyro in
>>> the same package, the ones without the suffix are pure accelerometers.
>> Yes You are right. I've used this kind of logic... What do you think about?
>>
>> Denis
>
> Seems sensible to me.
How about a comment to this effect in the code so that we keep it like this?
On 09/16/13 14:43, Lars-Peter Clausen wrote:
> On 09/16/2013 03:31 PM, Lee Jones wrote:
>>> Honestly I don't care either way on these, but consistency would definitely
>>> be good so applied to the togreg branch of iio.git
>>
>> For clarification, is togreg due for the v3.12 merge window or will be
>> get a settling period in -next?
>>
>
> Well everything up to the 'iio-for-3.12c' tag will be in v3.12, everything
> else not.
>
> - Lars
>
Yes. At the moment IIO is routed through Greg KHs staging tree. His merge
window closes 1 week before Linus opens the main one (when Greg can figure
out what Linus is intending!) Hence I close the IIO one whenever I manage
to get a last pull request in before that.
Hence all 3.13 for the current togreg branch.
Jonathan
On Mon, 16 Sep 2013, Jonathan Cameron wrote:
> On 09/16/13 14:43, Lars-Peter Clausen wrote:
> > On 09/16/2013 03:31 PM, Lee Jones wrote:
> >>> Honestly I don't care either way on these, but consistency would definitely
> >>> be good so applied to the togreg branch of iio.git
> >>
> >> For clarification, is togreg due for the v3.12 merge window or will be
> >> get a settling period in -next?
> >>
> >
> > Well everything up to the 'iio-for-3.12c' tag will be in v3.12, everything
> > else not.
> >
> > - Lars
> >
> Yes. At the moment IIO is routed through Greg KHs staging tree. His merge
> window closes 1 week before Linus opens the main one (when Greg can figure
> out what Linus is intending!) Hence I close the IIO one whenever I manage
> to get a last pull request in before that.
>
> Hence all 3.13 for the current togreg branch.
Perfect, thanks both of you.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog