[PATCH V3 0/9] iio: mma8452 enhancements

[PATCH V3 0/9] iio: mma8452 enhancements

[Martin Fuzzey]

This series adds some additional features to the mma8452 accelerometer driver
	* debugfs register access
	* transient threshold events
	* highpass filter
	* interrupt driven sampling (trigger)
	* self test mode

Attributes are added to the core for:
* highpass filter for readings
* both high and lowpass filters for events

In addition a latent bug in the device initialisation is fixed.

V3 changes:
	Use 3db frequency for filter enable / disable (0=disable)
	Support enabling filter for readings in addition to events
	Use IIO_EV_INFO_PERIOD rather than custom attribute
	Use devm_iio_trigger_alloc()
	Add comment to explain irq activation during probe()
	iio_device_register() only after all setup
	Drop iio_push_event() wrapper
	Correct parameters to iio_push_event()
	Document missing attributes
	Add self test mode

[PATCH 1/9] iio: mma8452: Initialise before activating

[Martin Fuzzey]

Many of the hardware configuration registers may only be modified while the
device is inactive.

Currently the probe code first activates the device and then modifies the
registers (eg to set the scale). This doesn't actually work but is not
noticed since the scale used is the default value.

While at it also issue a hardware reset command at probe time.

Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
---
 drivers/iio/accel/mma8452.c |   39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 3c12d49..04a4f34 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -32,6 +32,7 @@
 #define MMA8452_OFF_Z 0x31
 #define MMA8452_CTRL_REG1 0x2a
 #define MMA8452_CTRL_REG2 0x2b
+#define MMA8452_CTRL_REG2_RST		BIT(6)
 
 #define MMA8452_STATUS_DRDY (BIT(2) | BIT(1) | BIT(0))
 
@@ -111,7 +112,7 @@ static const int mma8452_samp_freq[8][2] = {
 	{6, 250000}, {1, 560000}
 };
 
-/* 
+/*
  * Hardware has fullscale of -2G, -4G, -8G corresponding to raw value -2048
  * The userspace interface uses m/s^2 and we declare micro units
  * So scale factor is given by:
@@ -335,6 +336,30 @@ static const struct iio_info mma8452_info = {
 
 static const unsigned long mma8452_scan_masks[] = {0x7, 0};
 
+static int mma8452_reset(struct i2c_client *client)
+{
+	int i;
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(client,	MMA8452_CTRL_REG2,
+						MMA8452_CTRL_REG2_RST);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < 10; i++) {
+		udelay(100);
+		ret = i2c_smbus_read_byte_data(client, MMA8452_CTRL_REG2);
+		if (ret == -EIO)
+			continue; /* I2C comm reset */
+		if (ret < 0)
+			return ret;
+		if (!(ret & MMA8452_CTRL_REG2_RST))
+			return 0;
+	}
+
+	return -ETIMEDOUT;
+}
+
 static int mma8452_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -365,10 +390,7 @@ static int mma8452_probe(struct i2c_client *client,
 	indio_dev->num_channels = ARRAY_SIZE(mma8452_channels);
 	indio_dev->available_scan_masks = mma8452_scan_masks;
 
-	data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
-		(MMA8452_CTRL_DR_DEFAULT << MMA8452_CTRL_DR_SHIFT);
-	ret = i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG1,
-		data->ctrl_reg1);
+	ret = mma8452_reset(client);
 	if (ret < 0)
 		return ret;
 
@@ -378,6 +400,13 @@ static int mma8452_probe(struct i2c_client *client,
 	if (ret < 0)
 		return ret;
 
+	data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
+		(MMA8452_CTRL_DR_DEFAULT << MMA8452_CTRL_DR_SHIFT);
+	ret = i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG1,
+		data->ctrl_reg1);
+	if (ret < 0)
+		return ret;
+
 	ret = iio_triggered_buffer_setup(indio_dev, NULL,
 		mma8452_trigger_handler, NULL);
 	if (ret < 0)

Re: [PATCH 1/9] iio: mma8452: Initialise before activating

[Jonathan Cameron]

On 19/02/15 14:15, Martin Fuzzey wrote:
> Many of the hardware configuration registers may only be modified while the
> device is inactive.
> 
> Currently the probe code first activates the device and then modifies the
> registers (eg to set the scale). This doesn't actually work but is not
> noticed since the scale used is the default value.
> 
> While at it also issue a hardware reset command at probe time.
> 
> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
Looks good to me, but I'd like Peters input on this as it's his driver.
> ---
>  drivers/iio/accel/mma8452.c |   39 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 3c12d49..04a4f34 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -32,6 +32,7 @@
>  #define MMA8452_OFF_Z 0x31
>  #define MMA8452_CTRL_REG1 0x2a
>  #define MMA8452_CTRL_REG2 0x2b
> +#define MMA8452_CTRL_REG2_RST		BIT(6)
>  
>  #define MMA8452_STATUS_DRDY (BIT(2) | BIT(1) | BIT(0))
>  
> @@ -111,7 +112,7 @@ static const int mma8452_samp_freq[8][2] = {
>  	{6, 250000}, {1, 560000}
>  };
>  
> -/* 
> +/*
>   * Hardware has fullscale of -2G, -4G, -8G corresponding to raw value -2048
>   * The userspace interface uses m/s^2 and we declare micro units
>   * So scale factor is given by:
> @@ -335,6 +336,30 @@ static const struct iio_info mma8452_info = {
>  
>  static const unsigned long mma8452_scan_masks[] = {0x7, 0};
>  
> +static int mma8452_reset(struct i2c_client *client)
> +{
> +	int i;
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(client,	MMA8452_CTRL_REG2,
> +						MMA8452_CTRL_REG2_RST);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < 10; i++) {
> +		udelay(100);
> +		ret = i2c_smbus_read_byte_data(client, MMA8452_CTRL_REG2);
> +		if (ret == -EIO)
> +			continue; /* I2C comm reset */
> +		if (ret < 0)
> +			return ret;
> +		if (!(ret & MMA8452_CTRL_REG2_RST))
> +			return 0;
> +	}
> +
> +	return -ETIMEDOUT;
> +}
> +
>  static int mma8452_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
> @@ -365,10 +390,7 @@ static int mma8452_probe(struct i2c_client *client,
>  	indio_dev->num_channels = ARRAY_SIZE(mma8452_channels);
>  	indio_dev->available_scan_masks = mma8452_scan_masks;
>  
> -	data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
> -		(MMA8452_CTRL_DR_DEFAULT << MMA8452_CTRL_DR_SHIFT);
> -	ret = i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG1,
> -		data->ctrl_reg1);
> +	ret = mma8452_reset(client);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -378,6 +400,13 @@ static int mma8452_probe(struct i2c_client *client,
>  	if (ret < 0)
>  		return ret;
>  
> +	data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
> +		(MMA8452_CTRL_DR_DEFAULT << MMA8452_CTRL_DR_SHIFT);
> +	ret = i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG1,
> +		data->ctrl_reg1);
> +	if (ret < 0)
> +		return ret;
> +
>  	ret = iio_triggered_buffer_setup(indio_dev, NULL,
>  		mma8452_trigger_handler, NULL);
>  	if (ret < 0)
> 

Re: [PATCH 1/9] iio: mma8452: Initialise before activating

[Peter Meerwald]

On Thu, 19 Feb 2015, Martin Fuzzey wrote:

> Many of the hardware configuration registers may only be modified while the
> device is inactive.
> 
> Currently the probe code first activates the device and then modifies the
> registers (eg to set the scale). This doesn't actually work but is not
> noticed since the scale used is the default value.
> 
> While at it also issue a hardware reset command at probe time.

comment below
 
> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
> ---
>  drivers/iio/accel/mma8452.c |   39 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 3c12d49..04a4f34 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -32,6 +32,7 @@
>  #define MMA8452_OFF_Z 0x31
>  #define MMA8452_CTRL_REG1 0x2a
>  #define MMA8452_CTRL_REG2 0x2b
> +#define MMA8452_CTRL_REG2_RST		BIT(6)
>  
>  #define MMA8452_STATUS_DRDY (BIT(2) | BIT(1) | BIT(0))
>  
> @@ -111,7 +112,7 @@ static const int mma8452_samp_freq[8][2] = {
>  	{6, 250000}, {1, 560000}
>  };
>  
> -/* 
> +/*
>   * Hardware has fullscale of -2G, -4G, -8G corresponding to raw value -2048
>   * The userspace interface uses m/s^2 and we declare micro units
>   * So scale factor is given by:
> @@ -335,6 +336,30 @@ static const struct iio_info mma8452_info = {
>  
>  static const unsigned long mma8452_scan_masks[] = {0x7, 0};
>  
> +static int mma8452_reset(struct i2c_client *client)
> +{
> +	int i;
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(client,	MMA8452_CTRL_REG2,
> +						MMA8452_CTRL_REG2_RST);
> +	if (ret)

maybe ret < 0 for consistency

> +		return ret;
> +
> +	for (i = 0; i < 10; i++) {
> +		udelay(100);
> +		ret = i2c_smbus_read_byte_data(client, MMA8452_CTRL_REG2);
> +		if (ret == -EIO)
> +			continue; /* I2C comm reset */
> +		if (ret < 0)
> +			return ret;
> +		if (!(ret & MMA8452_CTRL_REG2_RST))
> +			return 0;
> +	}
> +
> +	return -ETIMEDOUT;
> +}
> +
>  static int mma8452_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
> @@ -365,10 +390,7 @@ static int mma8452_probe(struct i2c_client *client,
>  	indio_dev->num_channels = ARRAY_SIZE(mma8452_channels);
>  	indio_dev->available_scan_masks = mma8452_scan_masks;
>  
> -	data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
> -		(MMA8452_CTRL_DR_DEFAULT << MMA8452_CTRL_DR_SHIFT);
> -	ret = i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG1,
> -		data->ctrl_reg1);
> +	ret = mma8452_reset(client);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -378,6 +400,13 @@ static int mma8452_probe(struct i2c_client *client,
>  	if (ret < 0)
>  		return ret;
>  
> +	data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
> +		(MMA8452_CTRL_DR_DEFAULT << MMA8452_CTRL_DR_SHIFT);
> +	ret = i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG1,
> +		data->ctrl_reg1);
> +	if (ret < 0)
> +		return ret;
> +
>  	ret = iio_triggered_buffer_setup(indio_dev, NULL,
>  		mma8452_trigger_handler, NULL);
>  	if (ret < 0)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: [PATCH 1/9] iio: mma8452: Initialise before activating

[Hartmut Knaack]

Martin Fuzzey schrieb am 19.02.2015 um 15:15:
> Many of the hardware configuration registers may only be modified while the
> device is inactive.
> 
> Currently the probe code first activates the device and then modifies the
> registers (eg to set the scale). This doesn't actually work but is not
> noticed since the scale used is the default value.
> 
> While at it also issue a hardware reset command at probe time.
> 

Hi Martin,
checkpatch.pl with strict option recommends the following:

CHECK: Alignment should match open parenthesis
#48: FILE: drivers/iio/accel/mma8452.c:345:
+       ret = i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG2,
+                                               MMA8452_CTRL_REG2_RST);

CHECK: usleep_range is preferred over udelay; see Documentation/timers/timers-howto.txt
#53: FILE: drivers/iio/accel/mma8452.c:350:
+               udelay(100);

CHECK: Alignment should match open parenthesis
#88: FILE: drivers/iio/accel/mma8452.c:406:
+       ret = i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG1,
+               data->ctrl_reg1);

Since your use of udelay is not critically in regard of timing, checkpatchs 
recommendation seems valid.
Thanks,
Hartmut

> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
> ---
>  drivers/iio/accel/mma8452.c |   39 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 3c12d49..04a4f34 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -32,6 +32,7 @@
>  #define MMA8452_OFF_Z 0x31
>  #define MMA8452_CTRL_REG1 0x2a
>  #define MMA8452_CTRL_REG2 0x2b
> +#define MMA8452_CTRL_REG2_RST		BIT(6)
>  
>  #define MMA8452_STATUS_DRDY (BIT(2) | BIT(1) | BIT(0))
>  
> @@ -111,7 +112,7 @@ static const int mma8452_samp_freq[8][2] = {
>  	{6, 250000}, {1, 560000}
>  };
>  
> -/* 
> +/*
>   * Hardware has fullscale of -2G, -4G, -8G corresponding to raw value -2048
>   * The userspace interface uses m/s^2 and we declare micro units
>   * So scale factor is given by:
> @@ -335,6 +336,30 @@ static const struct iio_info mma8452_info = {
>  
>  static const unsigned long mma8452_scan_masks[] = {0x7, 0};
>  
> +static int mma8452_reset(struct i2c_client *client)
> +{
> +	int i;
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(client,	MMA8452_CTRL_REG2,
> +						MMA8452_CTRL_REG2_RST);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < 10; i++) {
> +		udelay(100);
> +		ret = i2c_smbus_read_byte_data(client, MMA8452_CTRL_REG2);
> +		if (ret == -EIO)
> +			continue; /* I2C comm reset */
> +		if (ret < 0)
> +			return ret;
> +		if (!(ret & MMA8452_CTRL_REG2_RST))
> +			return 0;
> +	}
> +
> +	return -ETIMEDOUT;
> +}
> +
>  static int mma8452_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
> @@ -365,10 +390,7 @@ static int mma8452_probe(struct i2c_client *client,
>  	indio_dev->num_channels = ARRAY_SIZE(mma8452_channels);
>  	indio_dev->available_scan_masks = mma8452_scan_masks;
>  
> -	data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
> -		(MMA8452_CTRL_DR_DEFAULT << MMA8452_CTRL_DR_SHIFT);
> -	ret = i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG1,
> -		data->ctrl_reg1);
> +	ret = mma8452_reset(client);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -378,6 +400,13 @@ static int mma8452_probe(struct i2c_client *client,
>  	if (ret < 0)
>  		return ret;
>  
> +	data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
> +		(MMA8452_CTRL_DR_DEFAULT << MMA8452_CTRL_DR_SHIFT);
> +	ret = i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG1,
> +		data->ctrl_reg1);
> +	if (ret < 0)
> +		return ret;
> +
>  	ret = iio_triggered_buffer_setup(indio_dev, NULL,
>  		mma8452_trigger_handler, NULL);
>  	if (ret < 0)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 2/9] iio: mma8452: Add access to registers via DebugFS

[Martin Fuzzey]

Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
---
 drivers/iio/accel/mma8452.c |   25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 04a4f34..c46df4e 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -34,6 +34,8 @@
 #define MMA8452_CTRL_REG2 0x2b
 #define MMA8452_CTRL_REG2_RST		BIT(6)
 
+#define MMA8452_MAX_REG 0x31
+
 #define MMA8452_STATUS_DRDY (BIT(2) | BIT(1) | BIT(0))
 
 #define MMA8452_CTRL_DR_MASK (BIT(5) | BIT(4) | BIT(3))
@@ -292,6 +294,28 @@ done:
 	return IRQ_HANDLED;
 }
 
+static int mma8452_reg_access_dbg(struct iio_dev *indio_dev,
+			      unsigned reg, unsigned writeval,
+			      unsigned *readval)
+{
+	int ret;
+	struct mma8452_data *data = iio_priv(indio_dev);
+
+	if (reg > MMA8452_MAX_REG)
+		return -EINVAL;
+
+	if (readval == NULL) {
+		ret = mma8452_change_config(data, reg, writeval);
+	} else {
+		ret = i2c_smbus_read_byte_data(data->client, reg);
+		if (ret < 0)
+			return ret;
+		*readval = ret;
+		ret = 0;
+	}
+	return ret;
+}
+
 #define MMA8452_CHANNEL(axis, idx) { \
 	.type = IIO_ACCEL, \
 	.modified = 1, \
@@ -331,6 +355,7 @@ static const struct iio_info mma8452_info = {
 	.attrs = &mma8452_group,
 	.read_raw = &mma8452_read_raw,
 	.write_raw = &mma8452_write_raw,
+	.debugfs_reg_access = &mma8452_reg_access_dbg,
 	.driver_module = THIS_MODULE,
 };
 

Re: [PATCH 2/9] iio: mma8452: Add access to registers via DebugFS

[Hartmut Knaack]

Martin Fuzzey schrieb am 19.02.2015 um 15:15:
> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
A few comments inline.

> ---
>  drivers/iio/accel/mma8452.c |   25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 04a4f34..c46df4e 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -34,6 +34,8 @@
>  #define MMA8452_CTRL_REG2 0x2b
>  #define MMA8452_CTRL_REG2_RST		BIT(6)
>  
> +#define MMA8452_MAX_REG 0x31
> +
>  #define MMA8452_STATUS_DRDY (BIT(2) | BIT(1) | BIT(0))
>  
>  #define MMA8452_CTRL_DR_MASK (BIT(5) | BIT(4) | BIT(3))
> @@ -292,6 +294,28 @@ done:
>  	return IRQ_HANDLED;
>  }
>  
> +static int mma8452_reg_access_dbg(struct iio_dev *indio_dev,
> +			      unsigned reg, unsigned writeval,
> +			      unsigned *readval)
As before, parameters should be aligned with open parenthesis.

> +{
> +	int ret;
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +
> +	if (reg > MMA8452_MAX_REG)
> +		return -EINVAL;
> +
> +	if (readval == NULL) {
> +		ret = mma8452_change_config(data, reg, writeval);
You could return straight here without using ret.

> +	} else {
> +		ret = i2c_smbus_read_byte_data(data->client, reg);
> +		if (ret < 0)
> +			return ret;
> +		*readval = ret;
> +		ret = 0;
Same here.

> +	}
> +	return ret;
> +}
> +
>  #define MMA8452_CHANNEL(axis, idx) { \
>  	.type = IIO_ACCEL, \
>  	.modified = 1, \
> @@ -331,6 +355,7 @@ static const struct iio_info mma8452_info = {
>  	.attrs = &mma8452_group,
>  	.read_raw = &mma8452_read_raw,
>  	.write_raw = &mma8452_write_raw,
> +	.debugfs_reg_access = &mma8452_reg_access_dbg,
>  	.driver_module = THIS_MODULE,
>  };
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 3/9] iio: core: add high pass filter attributes

[Martin Fuzzey]

Add a high pass filter attribute for measurements (like the existing low pass)

Also add both high and low pass attibutes for events.

Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
---
 Documentation/ABI/testing/sysfs-bus-iio |   30 ++++++++++++++++++++++++++++++
 drivers/iio/industrialio-core.c         |    2 ++
 drivers/iio/industrialio-event.c        |    2 ++
 include/linux/iio/iio.h                 |    1 +
 include/linux/iio/types.h               |    2 ++
 5 files changed, 37 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 117521d..ab592dd 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -358,6 +358,16 @@ Description:
 		to the underlying data channel, then this parameter
 		gives the 3dB frequency of the filter in Hz.
 
+What:		/sys/.../in_accel_filter_high_pass_3db_frequency
+What:		/sys/.../in_magn_filter_high_pass_3db_frequency
+What:		/sys/.../in_anglvel_filter_high_pass_3db_frequency
+KernelVersion:	3.21
+Contact:	linux-iio@vger.kernel.org
+Description:
+		If a known or controllable high pass filter is applied
+		to the underlying data channel, then this parameter
+		gives the 3dB frequency of the filter in Hz.
+
 What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_raw
 What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_raw
 KernelVersion:	2.6.37
@@ -790,6 +800,26 @@ Description:
 		met before an event is generated. If direction is not
 		specified then this period applies to both directions.
 
+What:		/sys/.../events/in_accel_thresh_rising_low_pass_filter_3db
+What:		/sys/.../events/in_anglvel_thresh_rising_low_pass_filter_3db
+What:		/sys/.../events/in_magn_thresh_rising_low_pass_filter_3db
+KernelVersion:	3.21
+Contact:	linux-iio@vger.kernel.org
+Description:
+		If a low pass filter can be applied to the event generation
+		this property gives its 3db frequency in Hz.
+		A value of zero disables the filter.
+
+What:		/sys/.../events/in_accel_thresh_rising_high_pass_filter_3db
+What:		/sys/.../events/in_anglvel_thresh_rising_high_pass_filter_3db
+What:		/sys/.../events/in_magn_thresh_rising_high_pass_filter_3db
+KernelVersion:	3.21
+Contact:	linux-iio@vger.kernel.org
+Description:
+		If a high pass filter can be applied to the event generation
+		this property gives its 3db frequency in Hz.
+		A value of zero disables the filter.
+
 What:		/sys/.../iio:deviceX/events/in_accel_mag_en
 What:		/sys/.../iio:deviceX/events/in_accel_mag_rising_en
 What:		/sys/.../iio:deviceX/events/in_accel_mag_falling_en
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index f009d05..94b1b28 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -107,6 +107,8 @@ static const char * const iio_chan_info_postfix[] = {
 	[IIO_CHAN_INFO_AVERAGE_RAW] = "mean_raw",
 	[IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY]
 	= "filter_low_pass_3db_frequency",
+	[IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY]
+	= "filter_high_pass_3db_frequency",
 	[IIO_CHAN_INFO_SAMP_FREQ] = "sampling_frequency",
 	[IIO_CHAN_INFO_FREQUENCY] = "frequency",
 	[IIO_CHAN_INFO_PHASE] = "phase",
diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
index 35c02ae..4321380 100644
--- a/drivers/iio/industrialio-event.c
+++ b/drivers/iio/industrialio-event.c
@@ -210,6 +210,8 @@ static const char * const iio_ev_info_text[] = {
 	[IIO_EV_INFO_VALUE] = "value",
 	[IIO_EV_INFO_HYSTERESIS] = "hysteresis",
 	[IIO_EV_INFO_PERIOD] = "period",
+	[IIO_EV_INFO_HIGH_PASS_FILTER_3DB] = "high_pass_filter_3db",
+	[IIO_EV_INFO_LOW_PASS_FILTER_3DB] = "low_pass_filter_3db",
 };
 
 static enum iio_event_direction iio_ev_attr_dir(struct iio_dev_attr *attr)
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 3642ce7..3ce2793 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -32,6 +32,7 @@ enum iio_chan_info_enum {
 	IIO_CHAN_INFO_QUADRATURE_CORRECTION_RAW,
 	IIO_CHAN_INFO_AVERAGE_RAW,
 	IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY,
+	IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY,
 	IIO_CHAN_INFO_SAMP_FREQ,
 	IIO_CHAN_INFO_FREQUENCY,
 	IIO_CHAN_INFO_PHASE,
diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
index 4a2af8a..fc5c123 100644
--- a/include/linux/iio/types.h
+++ b/include/linux/iio/types.h
@@ -75,6 +75,8 @@ enum iio_event_info {
 	IIO_EV_INFO_VALUE,
 	IIO_EV_INFO_HYSTERESIS,
 	IIO_EV_INFO_PERIOD,
+	IIO_EV_INFO_HIGH_PASS_FILTER_3DB,
+	IIO_EV_INFO_LOW_PASS_FILTER_3DB,
 };
 
 enum iio_event_direction {

Re: [PATCH 3/9] iio: core: add high pass filter attributes

[Hartmut Knaack]

Martin Fuzzey schrieb am 19.02.2015 um 15:16:
> Add a high pass filter attribute for measurements (like the existing low pass)
> 
> Also add both high and low pass attibutes for events.
> 
Some comments inline.

> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-iio |   30 ++++++++++++++++++++++++++++++
>  drivers/iio/industrialio-core.c         |    2 ++
>  drivers/iio/industrialio-event.c        |    2 ++
>  include/linux/iio/iio.h                 |    1 +
>  include/linux/iio/types.h               |    2 ++
>  5 files changed, 37 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 117521d..ab592dd 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -358,6 +358,16 @@ Description:
>  		to the underlying data channel, then this parameter
>  		gives the 3dB frequency of the filter in Hz.
>  
> +What:		/sys/.../in_accel_filter_high_pass_3db_frequency
> +What:		/sys/.../in_magn_filter_high_pass_3db_frequency
> +What:		/sys/.../in_anglvel_filter_high_pass_3db_frequency
You mind alphabetic order of the filenames below, maybe do so over here, as well?

> +KernelVersion:	3.21
Some folks preferred a change in kernel version counting, now we have to deal
with it. Would 4.1 be appropriate? Same for the two instances below.

> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		If a known or controllable high pass filter is applied
> +		to the underlying data channel, then this parameter
> +		gives the 3dB frequency of the filter in Hz.
> +
>  What:		/sys/bus/iio/devices/iio:deviceX/out_voltageY_raw
>  What:		/sys/bus/iio/devices/iio:deviceX/out_altvoltageY_raw
>  KernelVersion:	2.6.37
> @@ -790,6 +800,26 @@ Description:
>  		met before an event is generated. If direction is not
>  		specified then this period applies to both directions.
>  
> +What:		/sys/.../events/in_accel_thresh_rising_low_pass_filter_3db
> +What:		/sys/.../events/in_anglvel_thresh_rising_low_pass_filter_3db
> +What:		/sys/.../events/in_magn_thresh_rising_low_pass_filter_3db
> +KernelVersion:	3.21
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		If a low pass filter can be applied to the event generation
> +		this property gives its 3db frequency in Hz.
> +		A value of zero disables the filter.
> +
> +What:		/sys/.../events/in_accel_thresh_rising_high_pass_filter_3db
> +What:		/sys/.../events/in_anglvel_thresh_rising_high_pass_filter_3db
> +What:		/sys/.../events/in_magn_thresh_rising_high_pass_filter_3db
> +KernelVersion:	3.21
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		If a high pass filter can be applied to the event generation
> +		this property gives its 3db frequency in Hz.
> +		A value of zero disables the filter.
> +
>  What:		/sys/.../iio:deviceX/events/in_accel_mag_en
>  What:		/sys/.../iio:deviceX/events/in_accel_mag_rising_en
>  What:		/sys/.../iio:deviceX/events/in_accel_mag_falling_en
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index f009d05..94b1b28 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -107,6 +107,8 @@ static const char * const iio_chan_info_postfix[] = {
>  	[IIO_CHAN_INFO_AVERAGE_RAW] = "mean_raw",
>  	[IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY]
>  	= "filter_low_pass_3db_frequency",
> +	[IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY]
> +	= "filter_high_pass_3db_frequency",
>  	[IIO_CHAN_INFO_SAMP_FREQ] = "sampling_frequency",
>  	[IIO_CHAN_INFO_FREQUENCY] = "frequency",
>  	[IIO_CHAN_INFO_PHASE] = "phase",
> diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
> index 35c02ae..4321380 100644
> --- a/drivers/iio/industrialio-event.c
> +++ b/drivers/iio/industrialio-event.c
> @@ -210,6 +210,8 @@ static const char * const iio_ev_info_text[] = {
>  	[IIO_EV_INFO_VALUE] = "value",
>  	[IIO_EV_INFO_HYSTERESIS] = "hysteresis",
>  	[IIO_EV_INFO_PERIOD] = "period",
> +	[IIO_EV_INFO_HIGH_PASS_FILTER_3DB] = "high_pass_filter_3db",
> +	[IIO_EV_INFO_LOW_PASS_FILTER_3DB] = "low_pass_filter_3db",
>  };
>  
>  static enum iio_event_direction iio_ev_attr_dir(struct iio_dev_attr *attr)
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 3642ce7..3ce2793 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -32,6 +32,7 @@ enum iio_chan_info_enum {
>  	IIO_CHAN_INFO_QUADRATURE_CORRECTION_RAW,
>  	IIO_CHAN_INFO_AVERAGE_RAW,
>  	IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY,
> +	IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY,
>  	IIO_CHAN_INFO_SAMP_FREQ,
>  	IIO_CHAN_INFO_FREQUENCY,
>  	IIO_CHAN_INFO_PHASE,
> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
> index 4a2af8a..fc5c123 100644
> --- a/include/linux/iio/types.h
> +++ b/include/linux/iio/types.h
> @@ -75,6 +75,8 @@ enum iio_event_info {
>  	IIO_EV_INFO_VALUE,
>  	IIO_EV_INFO_HYSTERESIS,
>  	IIO_EV_INFO_PERIOD,
> +	IIO_EV_INFO_HIGH_PASS_FILTER_3DB,
> +	IIO_EV_INFO_LOW_PASS_FILTER_3DB,
>  };
>  
>  enum iio_event_direction {
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 4/9] iio: mma8452: Basic support for transient events.

[Martin Fuzzey]

The event is triggered when the highpass filtered absolute acceleration
exceeds the threshold.

Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
---
 drivers/iio/accel/mma8452.c |  212 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 211 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index c46df4e..d44044f 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -9,7 +9,7 @@
  *
  * 7-bit I2C slave address 0x1c/0x1d (pin selectable)
  *
- * TODO: interrupt, thresholding, orientation / freefall events, autosleep
+ * TODO: orientation / freefall events, autosleep
  */
 
 #include <linux/module.h>
@@ -19,20 +19,33 @@
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/buffer.h>
 #include <linux/iio/triggered_buffer.h>
+#include <linux/iio/events.h>
 #include <linux/delay.h>
 
 #define MMA8452_STATUS 0x00
 #define MMA8452_OUT_X 0x01 /* MSB first, 12-bit  */
 #define MMA8452_OUT_Y 0x03
 #define MMA8452_OUT_Z 0x05
+#define MMA8452_INT_SRC 0x0c
 #define MMA8452_WHO_AM_I 0x0d
 #define MMA8452_DATA_CFG 0x0e
+#define MMA8452_TRANSIENT_CFG 0x1d
+#define MMA8452_TRANSIENT_CFG_ELE		BIT(4)
+#define MMA8452_TRANSIENT_CFG_CHAN_MASK(chan)	(BIT(1) << chan)
+#define MMA8452_TRANSIENT_SRC 0x1e
+#define MMA8452_TRANSIENT_SRC_XTRANSE		BIT(1)
+#define MMA8452_TRANSIENT_SRC_YTRANSE		BIT(3)
+#define MMA8452_TRANSIENT_SRC_ZTRANSE		BIT(5)
+#define MMA8452_TRANSIENT_THS 0x1f
+#define MMA8452_TRANSIENT_COUNT 0x20
 #define MMA8452_OFF_X 0x2f
 #define MMA8452_OFF_Y 0x30
 #define MMA8452_OFF_Z 0x31
 #define MMA8452_CTRL_REG1 0x2a
 #define MMA8452_CTRL_REG2 0x2b
 #define MMA8452_CTRL_REG2_RST		BIT(6)
+#define MMA8452_CTRL_REG4 0x2d
+#define MMA8452_CTRL_REG5 0x2e
 
 #define MMA8452_MAX_REG 0x31
 
@@ -48,6 +61,13 @@
 #define MMA8452_DATA_CFG_FS_4G 1
 #define MMA8452_DATA_CFG_FS_8G 2
 
+#define MMA8452_INT_DRDY	BIT(0)
+#define MMA8452_INT_FF_MT	BIT(2)
+#define MMA8452_INT_PULSE	BIT(3)
+#define MMA8452_INT_LNDPRT	BIT(4)
+#define MMA8452_INT_TRANS	BIT(5)
+#define MMA8452_INT_ASLP	BIT(7)
+
 #define MMA8452_DEVICE_ID 0x2a
 
 struct mma8452_data {
@@ -274,6 +294,121 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
 	}
 }
 
+static int mma8452_read_thresh(struct iio_dev *indio_dev,
+	const struct iio_chan_spec *chan, enum iio_event_type type,
+	enum iio_event_direction dir, enum iio_event_info info, int *val,
+	int *val2)
+{
+	struct mma8452_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_THS);
+	if (ret < 0)
+		return ret;
+
+	*val = ret & 0x7f;
+
+	return IIO_VAL_INT;
+}
+
+static int mma8452_write_thresh(struct iio_dev *indio_dev,
+	const struct iio_chan_spec *chan, enum iio_event_type type,
+	enum iio_event_direction dir, enum iio_event_info info, int val,
+	int val2)
+{
+	struct mma8452_data *data = iio_priv(indio_dev);
+
+	return mma8452_change_config(data, MMA8452_TRANSIENT_THS, val & 0x7f);
+}
+
+static int mma8452_read_event_config(struct iio_dev *indio_dev,
+	const struct iio_chan_spec *chan, enum iio_event_type type,
+	enum iio_event_direction dir)
+{
+	struct mma8452_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_CFG);
+	if (ret < 0)
+		return ret;
+
+	return ret & MMA8452_TRANSIENT_CFG_CHAN_MASK(chan->scan_index) ? 1 : 0;
+}
+
+static int mma8452_write_event_config(struct iio_dev *indio_dev,
+	const struct iio_chan_spec *chan, enum iio_event_type type,
+	enum iio_event_direction dir, int state)
+{
+	struct mma8452_data *data = iio_priv(indio_dev);
+	int val;
+
+	val = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_CFG);
+	if (val < 0)
+		return val;
+
+	if (state)
+		val |= MMA8452_TRANSIENT_CFG_CHAN_MASK(chan->scan_index);
+	else
+		val &= ~MMA8452_TRANSIENT_CFG_CHAN_MASK(chan->scan_index);
+
+	val |= MMA8452_TRANSIENT_CFG_ELE;
+
+	return mma8452_change_config(data, MMA8452_TRANSIENT_CFG, val);
+}
+
+static void mma8452_transient_interrupt(struct iio_dev *indio_dev)
+{
+	struct mma8452_data *data = iio_priv(indio_dev);
+	s64 ts = iio_get_time_ns();
+	int src;
+
+	src = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_SRC);
+	if (src < 0)
+		return;
+
+	if (src & MMA8452_TRANSIENT_SRC_XTRANSE)
+		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL,
+							0,
+							IIO_MOD_X,
+							IIO_EV_TYPE_THRESH,
+							IIO_EV_DIR_RISING),
+							ts);
+
+	if (src & MMA8452_TRANSIENT_SRC_YTRANSE)
+		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL,
+							0,
+							IIO_MOD_Y,
+							IIO_EV_TYPE_THRESH,
+							IIO_EV_DIR_RISING),
+							ts);
+
+	if (src & MMA8452_TRANSIENT_SRC_ZTRANSE)
+		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL,
+							0,
+							IIO_MOD_Z,
+							IIO_EV_TYPE_THRESH,
+							IIO_EV_DIR_RISING),
+							ts);
+}
+
+static irqreturn_t mma8452_interrupt(int irq, void *p)
+{
+	struct iio_dev *indio_dev = p;
+	struct mma8452_data *data = iio_priv(indio_dev);
+	int src;
+
+	src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
+	if (src < 0)
+		return IRQ_NONE;
+
+	if (src & MMA8452_INT_TRANS) {
+		mma8452_transient_interrupt(indio_dev);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
 static irqreturn_t mma8452_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
@@ -316,6 +451,32 @@ static int mma8452_reg_access_dbg(struct iio_dev *indio_dev,
 	return ret;
 }
 
+static const struct iio_event_spec mma8452_transient_event[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
+					BIT(IIO_EV_INFO_HIGH_PASS_FILTER_3DB),
+	},
+};
+
+/**
+ * Threshold is configured in fixed 8G/127 steps regardless of
+ * currently selected scale for measurement.
+ */
+static IIO_CONST_ATTR_NAMED(accel_transient_scale, in_accel_scale, "0.617742");
+
+static struct attribute *mma8452_event_attributes[] = {
+	&iio_const_attr_accel_transient_scale.dev_attr.attr,
+	NULL,
+};
+
+static struct attribute_group mma8452_event_attribute_group = {
+	.attrs = mma8452_event_attributes,
+	.name = "events",
+};
+
 #define MMA8452_CHANNEL(axis, idx) { \
 	.type = IIO_ACCEL, \
 	.modified = 1, \
@@ -332,6 +493,8 @@ static int mma8452_reg_access_dbg(struct iio_dev *indio_dev,
 		.shift = 4, \
 		.endianness = IIO_BE, \
 	}, \
+	.event_spec = mma8452_transient_event, \
+	.num_event_specs = ARRAY_SIZE(mma8452_transient_event), \
 }
 
 static const struct iio_chan_spec mma8452_channels[] = {
@@ -355,6 +518,11 @@ static const struct iio_info mma8452_info = {
 	.attrs = &mma8452_group,
 	.read_raw = &mma8452_read_raw,
 	.write_raw = &mma8452_write_raw,
+	.event_attrs = &mma8452_event_attribute_group,
+	.read_event_value = &mma8452_read_thresh,
+	.write_event_value = &mma8452_write_thresh,
+	.read_event_config = &mma8452_read_event_config,
+	.write_event_config = &mma8452_write_event_config,
 	.debugfs_reg_access = &mma8452_reg_access_dbg,
 	.driver_module = THIS_MODULE,
 };
@@ -425,6 +593,37 @@ static int mma8452_probe(struct i2c_client *client,
 	if (ret < 0)
 		return ret;
 
+	/*
+	 * By default set transient threshold to max to avoid events if
+	 * enabling without configuring threshold
+	 */
+	ret = i2c_smbus_write_byte_data(client, MMA8452_TRANSIENT_THS, 0x7f);
+	if (ret < 0)
+		return ret;
+
+	if (client->irq) {
+		/*
+		 * Although we enable the transient interrupt source once and
+		 * for all here the transient event detection itself is not
+		 * enabled until userspace asks for it by
+		 * mma8452_write_event_config()
+		 */
+		int supported_interrupts = MMA8452_INT_TRANS;
+
+		/* Assume wired to INT1 pin */
+		ret = i2c_smbus_write_byte_data(client,
+						MMA8452_CTRL_REG5,
+						supported_interrupts);
+		if (ret < 0)
+			return ret;
+
+		ret = i2c_smbus_write_byte_data(client,
+						MMA8452_CTRL_REG4,
+						supported_interrupts);
+		if (ret < 0)
+			return ret;
+	}
+
 	data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
 		(MMA8452_CTRL_DR_DEFAULT << MMA8452_CTRL_DR_SHIFT);
 	ret = i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG1,
@@ -437,9 +636,20 @@ static int mma8452_probe(struct i2c_client *client,
 	if (ret < 0)
 		return ret;
 
+	if (client->irq) {
+		ret = devm_request_threaded_irq(&client->dev,
+						client->irq,
+						NULL, mma8452_interrupt,
+						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+						client->name, indio_dev);
+		if (ret)
+			goto buffer_cleanup;
+	}
+
 	ret = iio_device_register(indio_dev);
 	if (ret < 0)
 		goto buffer_cleanup;
+
 	return 0;
 
 buffer_cleanup:

Re: [PATCH 4/9] iio: mma8452: Basic support for transient events.

[Jonathan Cameron]

On 19/02/15 14:16, Martin Fuzzey wrote:
> The event is triggered when the highpass filtered absolute acceleration
> exceeds the threshold.
> 
> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
you in_accel_transient_scale isn't documented.
Why do we need this naming at all?
Ah. It's not clear this is just the event rather than the underlying channel.
Perhaps, make it an event parameter instead... e.g. add scale to the ev_info
array like we do for hysteresis etc.

Otherwise, looks good to me.

Again, Peter's input would be good.

> ---
>  drivers/iio/accel/mma8452.c |  212 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 211 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index c46df4e..d44044f 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -9,7 +9,7 @@
>   *
>   * 7-bit I2C slave address 0x1c/0x1d (pin selectable)
>   *
> - * TODO: interrupt, thresholding, orientation / freefall events, autosleep
> + * TODO: orientation / freefall events, autosleep
>   */
>  
>  #include <linux/module.h>
> @@ -19,20 +19,33 @@
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/events.h>
>  #include <linux/delay.h>
>  
>  #define MMA8452_STATUS 0x00
>  #define MMA8452_OUT_X 0x01 /* MSB first, 12-bit  */
>  #define MMA8452_OUT_Y 0x03
>  #define MMA8452_OUT_Z 0x05
> +#define MMA8452_INT_SRC 0x0c
>  #define MMA8452_WHO_AM_I 0x0d
>  #define MMA8452_DATA_CFG 0x0e
> +#define MMA8452_TRANSIENT_CFG 0x1d
> +#define MMA8452_TRANSIENT_CFG_ELE		BIT(4)
> +#define MMA8452_TRANSIENT_CFG_CHAN_MASK(chan)	(BIT(1) << chan)
> +#define MMA8452_TRANSIENT_SRC 0x1e
> +#define MMA8452_TRANSIENT_SRC_XTRANSE		BIT(1)
> +#define MMA8452_TRANSIENT_SRC_YTRANSE		BIT(3)
> +#define MMA8452_TRANSIENT_SRC_ZTRANSE		BIT(5)
> +#define MMA8452_TRANSIENT_THS 0x1f
> +#define MMA8452_TRANSIENT_COUNT 0x20
>  #define MMA8452_OFF_X 0x2f
>  #define MMA8452_OFF_Y 0x30
>  #define MMA8452_OFF_Z 0x31
>  #define MMA8452_CTRL_REG1 0x2a
>  #define MMA8452_CTRL_REG2 0x2b
>  #define MMA8452_CTRL_REG2_RST		BIT(6)
> +#define MMA8452_CTRL_REG4 0x2d
> +#define MMA8452_CTRL_REG5 0x2e
>  
>  #define MMA8452_MAX_REG 0x31
>  
> @@ -48,6 +61,13 @@
>  #define MMA8452_DATA_CFG_FS_4G 1
>  #define MMA8452_DATA_CFG_FS_8G 2
>  
> +#define MMA8452_INT_DRDY	BIT(0)
> +#define MMA8452_INT_FF_MT	BIT(2)
> +#define MMA8452_INT_PULSE	BIT(3)
> +#define MMA8452_INT_LNDPRT	BIT(4)
> +#define MMA8452_INT_TRANS	BIT(5)
> +#define MMA8452_INT_ASLP	BIT(7)
> +
>  #define MMA8452_DEVICE_ID 0x2a
>  
>  struct mma8452_data {
> @@ -274,6 +294,121 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static int mma8452_read_thresh(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, enum iio_event_type type,
> +	enum iio_event_direction dir, enum iio_event_info info, int *val,
> +	int *val2)
> +{
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_THS);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = ret & 0x7f;
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int mma8452_write_thresh(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, enum iio_event_type type,
> +	enum iio_event_direction dir, enum iio_event_info info, int val,
> +	int val2)
> +{
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +
> +	return mma8452_change_config(data, MMA8452_TRANSIENT_THS, val & 0x7f);
> +}
> +
> +static int mma8452_read_event_config(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, enum iio_event_type type,
> +	enum iio_event_direction dir)
> +{
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_CFG);
> +	if (ret < 0)
> +		return ret;
> +
> +	return ret & MMA8452_TRANSIENT_CFG_CHAN_MASK(chan->scan_index) ? 1 : 0;
> +}
> +
> +static int mma8452_write_event_config(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, enum iio_event_type type,
> +	enum iio_event_direction dir, int state)
> +{
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +	int val;
> +
> +	val = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_CFG);
> +	if (val < 0)
> +		return val;
> +
> +	if (state)
> +		val |= MMA8452_TRANSIENT_CFG_CHAN_MASK(chan->scan_index);
> +	else
> +		val &= ~MMA8452_TRANSIENT_CFG_CHAN_MASK(chan->scan_index);
> +
> +	val |= MMA8452_TRANSIENT_CFG_ELE;
> +
> +	return mma8452_change_config(data, MMA8452_TRANSIENT_CFG, val);
> +}
> +
> +static void mma8452_transient_interrupt(struct iio_dev *indio_dev)
> +{
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +	s64 ts = iio_get_time_ns();
> +	int src;
> +
> +	src = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_SRC);
> +	if (src < 0)
> +		return;
> +
> +	if (src & MMA8452_TRANSIENT_SRC_XTRANSE)
> +		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL,
> +							0,
> +							IIO_MOD_X,
> +							IIO_EV_TYPE_THRESH,
> +							IIO_EV_DIR_RISING),
> +							ts);
> +
> +	if (src & MMA8452_TRANSIENT_SRC_YTRANSE)
> +		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL,
> +							0,
> +							IIO_MOD_Y,
> +							IIO_EV_TYPE_THRESH,
> +							IIO_EV_DIR_RISING),
> +							ts);
> +
> +	if (src & MMA8452_TRANSIENT_SRC_ZTRANSE)
> +		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL,
> +							0,
> +							IIO_MOD_Z,
> +							IIO_EV_TYPE_THRESH,
> +							IIO_EV_DIR_RISING),
> +							ts);
> +}
> +
> +static irqreturn_t mma8452_interrupt(int irq, void *p)
> +{
> +	struct iio_dev *indio_dev = p;
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +	int src;
> +
> +	src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
> +	if (src < 0)
> +		return IRQ_NONE;
> +
> +	if (src & MMA8452_INT_TRANS) {
> +		mma8452_transient_interrupt(indio_dev);
> +		return IRQ_HANDLED;
> +	}
> +
> +	return IRQ_NONE;
> +}
> +
>  static irqreturn_t mma8452_trigger_handler(int irq, void *p)
>  {
>  	struct iio_poll_func *pf = p;
> @@ -316,6 +451,32 @@ static int mma8452_reg_access_dbg(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> +static const struct iio_event_spec mma8452_transient_event[] = {
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> +		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> +					BIT(IIO_EV_INFO_HIGH_PASS_FILTER_3DB),
> +	},
> +};
> +
> +/**
> + * Threshold is configured in fixed 8G/127 steps regardless of
> + * currently selected scale for measurement.
> + */
> +static IIO_CONST_ATTR_NAMED(accel_transient_scale, in_accel_scale, "0.617742");
> +
> +static struct attribute *mma8452_event_attributes[] = {
> +	&iio_const_attr_accel_transient_scale.dev_attr.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group mma8452_event_attribute_group = {
> +	.attrs = mma8452_event_attributes,
> +	.name = "events",
> +};
> +
>  #define MMA8452_CHANNEL(axis, idx) { \
>  	.type = IIO_ACCEL, \
>  	.modified = 1, \
> @@ -332,6 +493,8 @@ static int mma8452_reg_access_dbg(struct iio_dev *indio_dev,
>  		.shift = 4, \
>  		.endianness = IIO_BE, \
>  	}, \
> +	.event_spec = mma8452_transient_event, \
> +	.num_event_specs = ARRAY_SIZE(mma8452_transient_event), \
>  }
>  
>  static const struct iio_chan_spec mma8452_channels[] = {
> @@ -355,6 +518,11 @@ static const struct iio_info mma8452_info = {
>  	.attrs = &mma8452_group,
>  	.read_raw = &mma8452_read_raw,
>  	.write_raw = &mma8452_write_raw,
> +	.event_attrs = &mma8452_event_attribute_group,
> +	.read_event_value = &mma8452_read_thresh,
> +	.write_event_value = &mma8452_write_thresh,
> +	.read_event_config = &mma8452_read_event_config,
> +	.write_event_config = &mma8452_write_event_config,
>  	.debugfs_reg_access = &mma8452_reg_access_dbg,
>  	.driver_module = THIS_MODULE,
>  };
> @@ -425,6 +593,37 @@ static int mma8452_probe(struct i2c_client *client,
>  	if (ret < 0)
>  		return ret;
>  
> +	/*
> +	 * By default set transient threshold to max to avoid events if
> +	 * enabling without configuring threshold
> +	 */
> +	ret = i2c_smbus_write_byte_data(client, MMA8452_TRANSIENT_THS, 0x7f);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (client->irq) {
> +		/*
> +		 * Although we enable the transient interrupt source once and
> +		 * for all here the transient event detection itself is not
> +		 * enabled until userspace asks for it by
> +		 * mma8452_write_event_config()
> +		 */
> +		int supported_interrupts = MMA8452_INT_TRANS;
> +
> +		/* Assume wired to INT1 pin */
> +		ret = i2c_smbus_write_byte_data(client,
> +						MMA8452_CTRL_REG5,
> +						supported_interrupts);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = i2c_smbus_write_byte_data(client,
> +						MMA8452_CTRL_REG4,
> +						supported_interrupts);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
>  		(MMA8452_CTRL_DR_DEFAULT << MMA8452_CTRL_DR_SHIFT);
>  	ret = i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG1,
> @@ -437,9 +636,20 @@ static int mma8452_probe(struct i2c_client *client,
>  	if (ret < 0)
>  		return ret;
>  
> +	if (client->irq) {
> +		ret = devm_request_threaded_irq(&client->dev,
> +						client->irq,
> +						NULL, mma8452_interrupt,
> +						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> +						client->name, indio_dev);
> +		if (ret)
> +			goto buffer_cleanup;
> +	}
> +
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0)
>  		goto buffer_cleanup;
> +
>  	return 0;
>  
>  buffer_cleanup:
> 

Re: [PATCH 4/9] iio: mma8452: Basic support for transient events.

[Martin Fuzzey]

On 25/02/15 13:25, Jonathan Cameron wrote:
> On 19/02/15 14:16, Martin Fuzzey wrote:
>> The event is triggered when the highpass filtered absolute acceleration
>> exceeds the threshold.
>>
>> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
> you in_accel_transient_scale isn't documented.
> Why do we need this naming at all?
> Ah. It's not clear this is just the event rather than the underlying channel.

Sorry, not understanding you here.

Are you saying it's not clear in the documentation (which is the next 
patch "iio: doc: Describe scale attributes for event thresholds" that 
you've already applied) or in the code?

The code is:
/*
  * Threshold is configured in fixed 8G/127 steps regardless of
  * currently selected scale for measurement.
  */
static IIO_CONST_ATTR_NAMED(accel_transient_scale, in_accel_scale, 
"0.617742");

static struct attribute *mma8452_event_attributes[] = {
     &iio_const_attr_accel_transient_scale.dev_attr.attr,
     NULL,
};

static struct attribute_group mma8452_event_attribute_group = {
     .attrs = mma8452_event_attributes,
     .name = "events",
};

#define MMA8452_CHANNEL(axis, idx) { \
     .type = IIO_ACCEL, \
...
     .num_event_specs = ARRAY_SIZE(mma8452_transient_event), \
}


That seems fairly clear to me.

> Perhaps, make it an event parameter instead... e.g. add scale to the ev_info
> array like we do for hysteresis etc.

But doing that would create a read/write attribute in sysfs whereas the 
above code creates a read only attribute and makes it clear that the 
attribute is a constant through the use of IIO_CONST_ATTR_NAMED

Once this is sorted out I'll send another (hopefully last) respin with 
the remaining remarks fixed.

Regards,

Martin

> Otherwise, looks good to me.
>
> Again, Peter's input would be good.
>
>> ---
>>   drivers/iio/accel/mma8452.c |  212 +++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 211 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>> index c46df4e..d44044f 100644
>> --- a/drivers/iio/accel/mma8452.c
>> +++ b/drivers/iio/accel/mma8452.c
>> @@ -9,7 +9,7 @@
>>    *
>>    * 7-bit I2C slave address 0x1c/0x1d (pin selectable)
>>    *
>> - * TODO: interrupt, thresholding, orientation / freefall events, autosleep
>> + * TODO: orientation / freefall events, autosleep
>>    */
>>   
>>   #include <linux/module.h>
>> @@ -19,20 +19,33 @@
>>   #include <linux/iio/trigger_consumer.h>
>>   #include <linux/iio/buffer.h>
>>   #include <linux/iio/triggered_buffer.h>
>> +#include <linux/iio/events.h>
>>   #include <linux/delay.h>
>>   
>>   #define MMA8452_STATUS 0x00
>>   #define MMA8452_OUT_X 0x01 /* MSB first, 12-bit  */
>>   #define MMA8452_OUT_Y 0x03
>>   #define MMA8452_OUT_Z 0x05
>> +#define MMA8452_INT_SRC 0x0c
>>   #define MMA8452_WHO_AM_I 0x0d
>>   #define MMA8452_DATA_CFG 0x0e
>> +#define MMA8452_TRANSIENT_CFG 0x1d
>> +#define MMA8452_TRANSIENT_CFG_ELE		BIT(4)
>> +#define MMA8452_TRANSIENT_CFG_CHAN_MASK(chan)	(BIT(1) << chan)
>> +#define MMA8452_TRANSIENT_SRC 0x1e
>> +#define MMA8452_TRANSIENT_SRC_XTRANSE		BIT(1)
>> +#define MMA8452_TRANSIENT_SRC_YTRANSE		BIT(3)
>> +#define MMA8452_TRANSIENT_SRC_ZTRANSE		BIT(5)
>> +#define MMA8452_TRANSIENT_THS 0x1f
>> +#define MMA8452_TRANSIENT_COUNT 0x20
>>   #define MMA8452_OFF_X 0x2f
>>   #define MMA8452_OFF_Y 0x30
>>   #define MMA8452_OFF_Z 0x31
>>   #define MMA8452_CTRL_REG1 0x2a
>>   #define MMA8452_CTRL_REG2 0x2b
>>   #define MMA8452_CTRL_REG2_RST		BIT(6)
>> +#define MMA8452_CTRL_REG4 0x2d
>> +#define MMA8452_CTRL_REG5 0x2e
>>   
>>   #define MMA8452_MAX_REG 0x31
>>   
>> @@ -48,6 +61,13 @@
>>   #define MMA8452_DATA_CFG_FS_4G 1
>>   #define MMA8452_DATA_CFG_FS_8G 2
>>   
>> +#define MMA8452_INT_DRDY	BIT(0)
>> +#define MMA8452_INT_FF_MT	BIT(2)
>> +#define MMA8452_INT_PULSE	BIT(3)
>> +#define MMA8452_INT_LNDPRT	BIT(4)
>> +#define MMA8452_INT_TRANS	BIT(5)
>> +#define MMA8452_INT_ASLP	BIT(7)
>> +
>>   #define MMA8452_DEVICE_ID 0x2a
>>   
>>   struct mma8452_data {
>> @@ -274,6 +294,121 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
>>   	}
>>   }
>>   
>> +static int mma8452_read_thresh(struct iio_dev *indio_dev,
>> +	const struct iio_chan_spec *chan, enum iio_event_type type,
>> +	enum iio_event_direction dir, enum iio_event_info info, int *val,
>> +	int *val2)
>> +{
>> +	struct mma8452_data *data = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	ret = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_THS);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	*val = ret & 0x7f;
>> +
>> +	return IIO_VAL_INT;
>> +}
>> +
>> +static int mma8452_write_thresh(struct iio_dev *indio_dev,
>> +	const struct iio_chan_spec *chan, enum iio_event_type type,
>> +	enum iio_event_direction dir, enum iio_event_info info, int val,
>> +	int val2)
>> +{
>> +	struct mma8452_data *data = iio_priv(indio_dev);
>> +
>> +	return mma8452_change_config(data, MMA8452_TRANSIENT_THS, val & 0x7f);
>> +}
>> +
>> +static int mma8452_read_event_config(struct iio_dev *indio_dev,
>> +	const struct iio_chan_spec *chan, enum iio_event_type type,
>> +	enum iio_event_direction dir)
>> +{
>> +	struct mma8452_data *data = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	ret = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_CFG);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return ret & MMA8452_TRANSIENT_CFG_CHAN_MASK(chan->scan_index) ? 1 : 0;
>> +}
>> +
>> +static int mma8452_write_event_config(struct iio_dev *indio_dev,
>> +	const struct iio_chan_spec *chan, enum iio_event_type type,
>> +	enum iio_event_direction dir, int state)
>> +{
>> +	struct mma8452_data *data = iio_priv(indio_dev);
>> +	int val;
>> +
>> +	val = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_CFG);
>> +	if (val < 0)
>> +		return val;
>> +
>> +	if (state)
>> +		val |= MMA8452_TRANSIENT_CFG_CHAN_MASK(chan->scan_index);
>> +	else
>> +		val &= ~MMA8452_TRANSIENT_CFG_CHAN_MASK(chan->scan_index);
>> +
>> +	val |= MMA8452_TRANSIENT_CFG_ELE;
>> +
>> +	return mma8452_change_config(data, MMA8452_TRANSIENT_CFG, val);
>> +}
>> +
>> +static void mma8452_transient_interrupt(struct iio_dev *indio_dev)
>> +{
>> +	struct mma8452_data *data = iio_priv(indio_dev);
>> +	s64 ts = iio_get_time_ns();
>> +	int src;
>> +
>> +	src = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_SRC);
>> +	if (src < 0)
>> +		return;
>> +
>> +	if (src & MMA8452_TRANSIENT_SRC_XTRANSE)
>> +		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL,
>> +							0,
>> +							IIO_MOD_X,
>> +							IIO_EV_TYPE_THRESH,
>> +							IIO_EV_DIR_RISING),
>> +							ts);
>> +
>> +	if (src & MMA8452_TRANSIENT_SRC_YTRANSE)
>> +		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL,
>> +							0,
>> +							IIO_MOD_Y,
>> +							IIO_EV_TYPE_THRESH,
>> +							IIO_EV_DIR_RISING),
>> +							ts);
>> +
>> +	if (src & MMA8452_TRANSIENT_SRC_ZTRANSE)
>> +		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL,
>> +							0,
>> +							IIO_MOD_Z,
>> +							IIO_EV_TYPE_THRESH,
>> +							IIO_EV_DIR_RISING),
>> +							ts);
>> +}
>> +
>> +static irqreturn_t mma8452_interrupt(int irq, void *p)
>> +{
>> +	struct iio_dev *indio_dev = p;
>> +	struct mma8452_data *data = iio_priv(indio_dev);
>> +	int src;
>> +
>> +	src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
>> +	if (src < 0)
>> +		return IRQ_NONE;
>> +
>> +	if (src & MMA8452_INT_TRANS) {
>> +		mma8452_transient_interrupt(indio_dev);
>> +		return IRQ_HANDLED;
>> +	}
>> +
>> +	return IRQ_NONE;
>> +}
>> +
>>   static irqreturn_t mma8452_trigger_handler(int irq, void *p)
>>   {
>>   	struct iio_poll_func *pf = p;
>> @@ -316,6 +451,32 @@ static int mma8452_reg_access_dbg(struct iio_dev *indio_dev,
>>   	return ret;
>>   }
>>   
>> +static const struct iio_event_spec mma8452_transient_event[] = {
>> +	{
>> +		.type = IIO_EV_TYPE_THRESH,
>> +		.dir = IIO_EV_DIR_RISING,
>> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
>> +		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
>> +					BIT(IIO_EV_INFO_HIGH_PASS_FILTER_3DB),
>> +	},
>> +};
>> +
>> +/**
>> + * Threshold is configured in fixed 8G/127 steps regardless of
>> + * currently selected scale for measurement.
>> + */
>> +static IIO_CONST_ATTR_NAMED(accel_transient_scale, in_accel_scale, "0.617742");
>> +
>> +static struct attribute *mma8452_event_attributes[] = {
>> +	&iio_const_attr_accel_transient_scale.dev_attr.attr,
>> +	NULL,
>> +};
>> +
>> +static struct attribute_group mma8452_event_attribute_group = {
>> +	.attrs = mma8452_event_attributes,
>> +	.name = "events",
>> +};
>> +
>>   #define MMA8452_CHANNEL(axis, idx) { \
>>   	.type = IIO_ACCEL, \
>>   	.modified = 1, \
>> @@ -332,6 +493,8 @@ static int mma8452_reg_access_dbg(struct iio_dev *indio_dev,
>>   		.shift = 4, \
>>   		.endianness = IIO_BE, \
>>   	}, \
>> +	.event_spec = mma8452_transient_event, \
>> +	.num_event_specs = ARRAY_SIZE(mma8452_transient_event), \
>>   }
>>   
>>   static const struct iio_chan_spec mma8452_channels[] = {
>> @@ -355,6 +518,11 @@ static const struct iio_info mma8452_info = {
>>   	.attrs = &mma8452_group,
>>   	.read_raw = &mma8452_read_raw,
>>   	.write_raw = &mma8452_write_raw,
>> +	.event_attrs = &mma8452_event_attribute_group,
>> +	.read_event_value = &mma8452_read_thresh,
>> +	.write_event_value = &mma8452_write_thresh,
>> +	.read_event_config = &mma8452_read_event_config,
>> +	.write_event_config = &mma8452_write_event_config,
>>   	.debugfs_reg_access = &mma8452_reg_access_dbg,
>>   	.driver_module = THIS_MODULE,
>>   };
>> @@ -425,6 +593,37 @@ static int mma8452_probe(struct i2c_client *client,
>>   	if (ret < 0)
>>   		return ret;
>>   
>> +	/*
>> +	 * By default set transient threshold to max to avoid events if
>> +	 * enabling without configuring threshold
>> +	 */
>> +	ret = i2c_smbus_write_byte_data(client, MMA8452_TRANSIENT_THS, 0x7f);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (client->irq) {
>> +		/*
>> +		 * Although we enable the transient interrupt source once and
>> +		 * for all here the transient event detection itself is not
>> +		 * enabled until userspace asks for it by
>> +		 * mma8452_write_event_config()
>> +		 */
>> +		int supported_interrupts = MMA8452_INT_TRANS;
>> +
>> +		/* Assume wired to INT1 pin */
>> +		ret = i2c_smbus_write_byte_data(client,
>> +						MMA8452_CTRL_REG5,
>> +						supported_interrupts);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		ret = i2c_smbus_write_byte_data(client,
>> +						MMA8452_CTRL_REG4,
>> +						supported_interrupts);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
>>   	data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
>>   		(MMA8452_CTRL_DR_DEFAULT << MMA8452_CTRL_DR_SHIFT);
>>   	ret = i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG1,
>> @@ -437,9 +636,20 @@ static int mma8452_probe(struct i2c_client *client,
>>   	if (ret < 0)
>>   		return ret;
>>   
>> +	if (client->irq) {
>> +		ret = devm_request_threaded_irq(&client->dev,
>> +						client->irq,
>> +						NULL, mma8452_interrupt,
>> +						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
>> +						client->name, indio_dev);
>> +		if (ret)
>> +			goto buffer_cleanup;
>> +	}
>> +
>>   	ret = iio_device_register(indio_dev);
>>   	if (ret < 0)
>>   		goto buffer_cleanup;
>> +
>>   	return 0;
>>   
>>   buffer_cleanup:
>>

Re: [PATCH 4/9] iio: mma8452: Basic support for transient events.

[Jonathan Cameron]

On 29/04/15 08:52, Martin Fuzzey wrote:
> On 25/02/15 13:25, Jonathan Cameron wrote:
>> On 19/02/15 14:16, Martin Fuzzey wrote:
>>> The event is triggered when the highpass filtered absolute acceleration
>>> exceeds the threshold.
>>>
>>> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
>> you in_accel_transient_scale isn't documented.
>> Why do we need this naming at all?
>> Ah. It's not clear this is just the event rather than the underlying channel.
> 
> Sorry, not understanding you here.
> 
> Are you saying it's not clear in the documentation (which is the next
> patch "iio: doc: Describe scale attributes for event thresholds" that
> you've already applied) or in the code?
oops, I suspect I didn't like the naming of the attribute being somewhat
ambiguous.

Also, whilst the next patch is documentation, it doesn't seem to include
this particular attribute...
> 
> The code is:
> /*
>  * Threshold is configured in fixed 8G/127 steps regardless of
>  * currently selected scale for measurement.
>  */
> static IIO_CONST_ATTR_NAMED(accel_transient_scale, in_accel_scale, "0.617742");
> 
> static struct attribute *mma8452_event_attributes[] = {
>     &iio_const_attr_accel_transient_scale.dev_attr.attr,
>     NULL,
> };
> 
> static struct attribute_group mma8452_event_attribute_group = {
>     .attrs = mma8452_event_attributes,
>     .name = "events",
> };
> 
> #define MMA8452_CHANNEL(axis, idx) { \
>     .type = IIO_ACCEL, \
> ...
>     .num_event_specs = ARRAY_SIZE(mma8452_transient_event), \
> }
> 
> 
> That seems fairly clear to me.
> 
>> Perhaps, make it an event parameter instead... e.g. add scale to the ev_info
>> array like we do for hysteresis etc.
> 
> But doing that would create a read/write attribute in sysfs whereas the above code creates a read only attribute and makes it clear that the attribute is a constant through the use of IIO_CONST_ATTR_NAMED
> 
> Once this is sorted out I'll send another (hopefully last) respin with the remaining remarks fixed.
> 
> Regards,
> 
> Martin
> 
>> Otherwise, looks good to me.
>>
>> Again, Peter's input would be good.
>>
>>> ---
>>>   drivers/iio/accel/mma8452.c |  212 +++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 211 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>>> index c46df4e..d44044f 100644
>>> --- a/drivers/iio/accel/mma8452.c
>>> +++ b/drivers/iio/accel/mma8452.c
>>> @@ -9,7 +9,7 @@
>>>    *
>>>    * 7-bit I2C slave address 0x1c/0x1d (pin selectable)
>>>    *
>>> - * TODO: interrupt, thresholding, orientation / freefall events, autosleep
>>> + * TODO: orientation / freefall events, autosleep
>>>    */
>>>     #include <linux/module.h>
>>> @@ -19,20 +19,33 @@
>>>   #include <linux/iio/trigger_consumer.h>
>>>   #include <linux/iio/buffer.h>
>>>   #include <linux/iio/triggered_buffer.h>
>>> +#include <linux/iio/events.h>
>>>   #include <linux/delay.h>
>>>     #define MMA8452_STATUS 0x00
>>>   #define MMA8452_OUT_X 0x01 /* MSB first, 12-bit  */
>>>   #define MMA8452_OUT_Y 0x03
>>>   #define MMA8452_OUT_Z 0x05
>>> +#define MMA8452_INT_SRC 0x0c
>>>   #define MMA8452_WHO_AM_I 0x0d
>>>   #define MMA8452_DATA_CFG 0x0e
>>> +#define MMA8452_TRANSIENT_CFG 0x1d
>>> +#define MMA8452_TRANSIENT_CFG_ELE        BIT(4)
>>> +#define MMA8452_TRANSIENT_CFG_CHAN_MASK(chan)    (BIT(1) << chan)
>>> +#define MMA8452_TRANSIENT_SRC 0x1e
>>> +#define MMA8452_TRANSIENT_SRC_XTRANSE        BIT(1)
>>> +#define MMA8452_TRANSIENT_SRC_YTRANSE        BIT(3)
>>> +#define MMA8452_TRANSIENT_SRC_ZTRANSE        BIT(5)
>>> +#define MMA8452_TRANSIENT_THS 0x1f
>>> +#define MMA8452_TRANSIENT_COUNT 0x20
>>>   #define MMA8452_OFF_X 0x2f
>>>   #define MMA8452_OFF_Y 0x30
>>>   #define MMA8452_OFF_Z 0x31
>>>   #define MMA8452_CTRL_REG1 0x2a
>>>   #define MMA8452_CTRL_REG2 0x2b
>>>   #define MMA8452_CTRL_REG2_RST        BIT(6)
>>> +#define MMA8452_CTRL_REG4 0x2d
>>> +#define MMA8452_CTRL_REG5 0x2e
>>>     #define MMA8452_MAX_REG 0x31
>>>   @@ -48,6 +61,13 @@
>>>   #define MMA8452_DATA_CFG_FS_4G 1
>>>   #define MMA8452_DATA_CFG_FS_8G 2
>>>   +#define MMA8452_INT_DRDY    BIT(0)
>>> +#define MMA8452_INT_FF_MT    BIT(2)
>>> +#define MMA8452_INT_PULSE    BIT(3)
>>> +#define MMA8452_INT_LNDPRT    BIT(4)
>>> +#define MMA8452_INT_TRANS    BIT(5)
>>> +#define MMA8452_INT_ASLP    BIT(7)
>>> +
>>>   #define MMA8452_DEVICE_ID 0x2a
>>>     struct mma8452_data {
>>> @@ -274,6 +294,121 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
>>>       }
>>>   }
>>>   +static int mma8452_read_thresh(struct iio_dev *indio_dev,
>>> +    const struct iio_chan_spec *chan, enum iio_event_type type,
>>> +    enum iio_event_direction dir, enum iio_event_info info, int *val,
>>> +    int *val2)
>>> +{
>>> +    struct mma8452_data *data = iio_priv(indio_dev);
>>> +    int ret;
>>> +
>>> +    ret = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_THS);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    *val = ret & 0x7f;
>>> +
>>> +    return IIO_VAL_INT;
>>> +}
>>> +
>>> +static int mma8452_write_thresh(struct iio_dev *indio_dev,
>>> +    const struct iio_chan_spec *chan, enum iio_event_type type,
>>> +    enum iio_event_direction dir, enum iio_event_info info, int val,
>>> +    int val2)
>>> +{
>>> +    struct mma8452_data *data = iio_priv(indio_dev);
>>> +
>>> +    return mma8452_change_config(data, MMA8452_TRANSIENT_THS, val & 0x7f);
>>> +}
>>> +
>>> +static int mma8452_read_event_config(struct iio_dev *indio_dev,
>>> +    const struct iio_chan_spec *chan, enum iio_event_type type,
>>> +    enum iio_event_direction dir)
>>> +{
>>> +    struct mma8452_data *data = iio_priv(indio_dev);
>>> +    int ret;
>>> +
>>> +    ret = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_CFG);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    return ret & MMA8452_TRANSIENT_CFG_CHAN_MASK(chan->scan_index) ? 1 : 0;
>>> +}
>>> +
>>> +static int mma8452_write_event_config(struct iio_dev *indio_dev,
>>> +    const struct iio_chan_spec *chan, enum iio_event_type type,
>>> +    enum iio_event_direction dir, int state)
>>> +{
>>> +    struct mma8452_data *data = iio_priv(indio_dev);
>>> +    int val;
>>> +
>>> +    val = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_CFG);
>>> +    if (val < 0)
>>> +        return val;
>>> +
>>> +    if (state)
>>> +        val |= MMA8452_TRANSIENT_CFG_CHAN_MASK(chan->scan_index);
>>> +    else
>>> +        val &= ~MMA8452_TRANSIENT_CFG_CHAN_MASK(chan->scan_index);
>>> +
>>> +    val |= MMA8452_TRANSIENT_CFG_ELE;
>>> +
>>> +    return mma8452_change_config(data, MMA8452_TRANSIENT_CFG, val);
>>> +}
>>> +
>>> +static void mma8452_transient_interrupt(struct iio_dev *indio_dev)
>>> +{
>>> +    struct mma8452_data *data = iio_priv(indio_dev);
>>> +    s64 ts = iio_get_time_ns();
>>> +    int src;
>>> +
>>> +    src = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_SRC);
>>> +    if (src < 0)
>>> +        return;
>>> +
>>> +    if (src & MMA8452_TRANSIENT_SRC_XTRANSE)
>>> +        iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL,
>>> +                            0,
>>> +                            IIO_MOD_X,
>>> +                            IIO_EV_TYPE_THRESH,
>>> +                            IIO_EV_DIR_RISING),
>>> +                            ts);
>>> +
>>> +    if (src & MMA8452_TRANSIENT_SRC_YTRANSE)
>>> +        iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL,
>>> +                            0,
>>> +                            IIO_MOD_Y,
>>> +                            IIO_EV_TYPE_THRESH,
>>> +                            IIO_EV_DIR_RISING),
>>> +                            ts);
>>> +
>>> +    if (src & MMA8452_TRANSIENT_SRC_ZTRANSE)
>>> +        iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL,
>>> +                            0,
>>> +                            IIO_MOD_Z,
>>> +                            IIO_EV_TYPE_THRESH,
>>> +                            IIO_EV_DIR_RISING),
>>> +                            ts);
>>> +}
>>> +
>>> +static irqreturn_t mma8452_interrupt(int irq, void *p)
>>> +{
>>> +    struct iio_dev *indio_dev = p;
>>> +    struct mma8452_data *data = iio_priv(indio_dev);
>>> +    int src;
>>> +
>>> +    src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
>>> +    if (src < 0)
>>> +        return IRQ_NONE;
>>> +
>>> +    if (src & MMA8452_INT_TRANS) {
>>> +        mma8452_transient_interrupt(indio_dev);
>>> +        return IRQ_HANDLED;
>>> +    }
>>> +
>>> +    return IRQ_NONE;
>>> +}
>>> +
>>>   static irqreturn_t mma8452_trigger_handler(int irq, void *p)
>>>   {
>>>       struct iio_poll_func *pf = p;
>>> @@ -316,6 +451,32 @@ static int mma8452_reg_access_dbg(struct iio_dev *indio_dev,
>>>       return ret;
>>>   }
>>>   +static const struct iio_event_spec mma8452_transient_event[] = {
>>> +    {
>>> +        .type = IIO_EV_TYPE_THRESH,
>>> +        .dir = IIO_EV_DIR_RISING,
>>> +        .mask_separate = BIT(IIO_EV_INFO_ENABLE),
>>> +        .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
>>> +                    BIT(IIO_EV_INFO_HIGH_PASS_FILTER_3DB),
>>> +    },
>>> +};
>>> +
>>> +/**
>>> + * Threshold is configured in fixed 8G/127 steps regardless of
>>> + * currently selected scale for measurement.
>>> + */
>>> +static IIO_CONST_ATTR_NAMED(accel_transient_scale, in_accel_scale, "0.617742");
>>> +
>>> +static struct attribute *mma8452_event_attributes[] = {
>>> +    &iio_const_attr_accel_transient_scale.dev_attr.attr,
>>> +    NULL,
>>> +};
>>> +
>>> +static struct attribute_group mma8452_event_attribute_group = {
>>> +    .attrs = mma8452_event_attributes,
>>> +    .name = "events",
>>> +};
>>> +
>>>   #define MMA8452_CHANNEL(axis, idx) { \
>>>       .type = IIO_ACCEL, \
>>>       .modified = 1, \
>>> @@ -332,6 +493,8 @@ static int mma8452_reg_access_dbg(struct iio_dev *indio_dev,
>>>           .shift = 4, \
>>>           .endianness = IIO_BE, \
>>>       }, \
>>> +    .event_spec = mma8452_transient_event, \
>>> +    .num_event_specs = ARRAY_SIZE(mma8452_transient_event), \
>>>   }
>>>     static const struct iio_chan_spec mma8452_channels[] = {
>>> @@ -355,6 +518,11 @@ static const struct iio_info mma8452_info = {
>>>       .attrs = &mma8452_group,
>>>       .read_raw = &mma8452_read_raw,
>>>       .write_raw = &mma8452_write_raw,
>>> +    .event_attrs = &mma8452_event_attribute_group,
>>> +    .read_event_value = &mma8452_read_thresh,
>>> +    .write_event_value = &mma8452_write_thresh,
>>> +    .read_event_config = &mma8452_read_event_config,
>>> +    .write_event_config = &mma8452_write_event_config,
>>>       .debugfs_reg_access = &mma8452_reg_access_dbg,
>>>       .driver_module = THIS_MODULE,
>>>   };
>>> @@ -425,6 +593,37 @@ static int mma8452_probe(struct i2c_client *client,
>>>       if (ret < 0)
>>>           return ret;
>>>   +    /*
>>> +     * By default set transient threshold to max to avoid events if
>>> +     * enabling without configuring threshold
>>> +     */
>>> +    ret = i2c_smbus_write_byte_data(client, MMA8452_TRANSIENT_THS, 0x7f);
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    if (client->irq) {
>>> +        /*
>>> +         * Although we enable the transient interrupt source once and
>>> +         * for all here the transient event detection itself is not
>>> +         * enabled until userspace asks for it by
>>> +         * mma8452_write_event_config()
>>> +         */
>>> +        int supported_interrupts = MMA8452_INT_TRANS;
>>> +
>>> +        /* Assume wired to INT1 pin */
>>> +        ret = i2c_smbus_write_byte_data(client,
>>> +                        MMA8452_CTRL_REG5,
>>> +                        supported_interrupts);
>>> +        if (ret < 0)
>>> +            return ret;
>>> +
>>> +        ret = i2c_smbus_write_byte_data(client,
>>> +                        MMA8452_CTRL_REG4,
>>> +                        supported_interrupts);
>>> +        if (ret < 0)
>>> +            return ret;
>>> +    }
>>> +
>>>       data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
>>>           (MMA8452_CTRL_DR_DEFAULT << MMA8452_CTRL_DR_SHIFT);
>>>       ret = i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG1,
>>> @@ -437,9 +636,20 @@ static int mma8452_probe(struct i2c_client *client,
>>>       if (ret < 0)
>>>           return ret;
>>>   +    if (client->irq) {
>>> +        ret = devm_request_threaded_irq(&client->dev,
>>> +                        client->irq,
>>> +                        NULL, mma8452_interrupt,
>>> +                        IRQF_TRIGGER_LOW | IRQF_ONESHOT,
>>> +                        client->name, indio_dev);
>>> +        if (ret)
>>> +            goto buffer_cleanup;
>>> +    }
>>> +
>>>       ret = iio_device_register(indio_dev);
>>>       if (ret < 0)
>>>           goto buffer_cleanup;
>>> +
>>>       return 0;
>>>     buffer_cleanup:
>>>
> 

Re: [PATCH 4/9] iio: mma8452: Basic support for transient events.

[Martin Fuzzey]

On 08/05/15 15:58, Jonathan Cameron wrote:
> On 29/04/15 08:52, Martin Fuzzey wrote:
>> On 25/02/15 13:25, Jonathan Cameron wrote:
>>> On 19/02/15 14:16, Martin Fuzzey wrote:
>>>> The event is triggered when the highpass filtered absolute acceleration
>>>> exceeds the threshold.
>>>>
>>>> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
>>> you in_accel_transient_scale isn't documented.
>>> Why do we need this naming at all?
>>> Ah. It's not clear this is just the event rather than the underlying channel.
>> Sorry, not understanding you here.
>>
>> Are you saying it's not clear in the documentation (which is the next
>> patch "iio: doc: Describe scale attributes for event thresholds" that
>> you've already applied) or in the code?
> oops, I suspect I didn't like the naming of the attribute being somewhat
> ambiguous.
>
> Also, whilst the next patch is documentation, it doesn't seem to include
> this particular attribute...

Ah, I think you are confusing the attribute name and the local variable 
name.
in_accel_transient_scale is just the local variable name.

The attribute name is events/in_accel_scale and that is documented in 
the patch you have already merged.


$ git show d1bd4867b0959d5221dc528ccf60c8534aae865d
commit d1bd4867b0959d5221dc528ccf60c8534aae865d
Author: Martin Fuzzey <mfuzzey@parkeon.com>
Date:   Thu Feb 19 15:16:04 2015 +0100

     iio: doc: Describe scale attributes for event thresholds

     Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
     Signed-off-by: Jonathan Cameron <jic23@kernel.org>

diff --git a/Documentation/ABI/testing/sysfs-bus-iio 
b/Documentation/ABI/testing/sysfs-bus-iio
index 9a70c31..9230709 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -661,6 +661,24 @@ Description:
                 value is in raw device units or in processed units (as _raw
                 and _input do on sysfs direct channel read attributes).

+What:          /sys/.../events/in_accel_scale
...


>> The code is:
>> /*
>>   * Threshold is configured in fixed 8G/127 steps regardless of
>>   * currently selected scale for measurement.
>>   */
>> static IIO_CONST_ATTR_NAMED(accel_transient_scale, in_accel_scale, "0.617742");
>>
>> static struct attribute *mma8452_event_attributes[] = {
>>      &iio_const_attr_accel_transient_scale.dev_attr.attr,
>>      NULL,
>> };
>>
>> static struct attribute_group mma8452_event_attribute_group = {
>>      .attrs = mma8452_event_attributes,
>>      .name = "events",
>> };
>>
>> #define MMA8452_CHANNEL(axis, idx) { \
>>      .type = IIO_ACCEL, \
>> ...
>>      .num_event_specs = ARRAY_SIZE(mma8452_transient_event), \
>> }
>>
>>
>> That seems fairly clear to me.
>>
>>> Perhaps, make it an event parameter instead... e.g. add scale to the ev_info
>>> array like we do for hysteresis etc.
>> But doing that would create a read/write attribute in sysfs whereas the above code creates a read only attribute and makes it clear that the attribute is a constant through the use of IIO_CONST_ATTR_NAMED
>>
>> Once this is sorted out I'll send another (hopefully last) respin with the remaining remarks fixed.
>>
>> Regards,
>>
>> Martin
>>
>>> Otherwise, looks good to me.
>>>
>>> Again, Peter's input would be good.
>>>
>>>

Re: [PATCH 4/9] iio: mma8452: Basic support for transient events.

[Jonathan Cameron]

On 12/05/15 15:14, Martin Fuzzey wrote:
> On 08/05/15 15:58, Jonathan Cameron wrote:
>> On 29/04/15 08:52, Martin Fuzzey wrote:
>>> On 25/02/15 13:25, Jonathan Cameron wrote:
>>>> On 19/02/15 14:16, Martin Fuzzey wrote:
>>>>> The event is triggered when the highpass filtered absolute acceleration
>>>>> exceeds the threshold.
>>>>>
>>>>> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
>>>> you in_accel_transient_scale isn't documented.
>>>> Why do we need this naming at all?
>>>> Ah. It's not clear this is just the event rather than the underlying channel.
>>> Sorry, not understanding you here.
>>>
>>> Are you saying it's not clear in the documentation (which is the next
>>> patch "iio: doc: Describe scale attributes for event thresholds" that
>>> you've already applied) or in the code?
>> oops, I suspect I didn't like the naming of the attribute being somewhat
>> ambiguous.
>>
>> Also, whilst the next patch is documentation, it doesn't seem to include
>> this particular attribute...
> 
> Ah, I think you are confusing the attribute name and the local variable name.
> in_accel_transient_scale is just the local variable name.
> 
> The attribute name is events/in_accel_scale and that is documented in the patch you have already merged.
Gah! fair enough.  I'm easily confused ;)
> 
> 
> $ git show d1bd4867b0959d5221dc528ccf60c8534aae865d
> commit d1bd4867b0959d5221dc528ccf60c8534aae865d
> Author: Martin Fuzzey <mfuzzey@parkeon.com>
> Date:   Thu Feb 19 15:16:04 2015 +0100
> 
>     iio: doc: Describe scale attributes for event thresholds
> 
>     Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
>     Signed-off-by: Jonathan Cameron <jic23@kernel.org>
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 9a70c31..9230709 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -661,6 +661,24 @@ Description:
>                 value is in raw device units or in processed units (as _raw
>                 and _input do on sysfs direct channel read attributes).
> 
> +What:          /sys/.../events/in_accel_scale
> ...
> 
> 
>>> The code is:
>>> /*
>>>   * Threshold is configured in fixed 8G/127 steps regardless of
>>>   * currently selected scale for measurement.
>>>   */
>>> static IIO_CONST_ATTR_NAMED(accel_transient_scale, in_accel_scale, "0.617742");
>>>
>>> static struct attribute *mma8452_event_attributes[] = {
>>>      &iio_const_attr_accel_transient_scale.dev_attr.attr,
>>>      NULL,
>>> };
>>>
>>> static struct attribute_group mma8452_event_attribute_group = {
>>>      .attrs = mma8452_event_attributes,
>>>      .name = "events",
>>> };
>>>
>>> #define MMA8452_CHANNEL(axis, idx) { \
>>>      .type = IIO_ACCEL, \
>>> ...
>>>      .num_event_specs = ARRAY_SIZE(mma8452_transient_event), \
>>> }
>>>
>>>
>>> That seems fairly clear to me.
>>>
>>>> Perhaps, make it an event parameter instead... e.g. add scale to the ev_info
>>>> array like we do for hysteresis etc.
>>> But doing that would create a read/write attribute in sysfs whereas the above code creates a read only attribute and makes it clear that the attribute is a constant through the use of IIO_CONST_ATTR_NAMED
>>>
>>> Once this is sorted out I'll send another (hopefully last) respin with the remaining remarks fixed.
>>>
>>> Regards,
>>>
>>> Martin
>>>
>>>> Otherwise, looks good to me.
>>>>
>>>> Again, Peter's input would be good.
>>>>
>>>>
> 

Re: [PATCH 4/9] iio: mma8452: Basic support for transient events.

[Peter Meerwald]

On Thu, 19 Feb 2015, Martin Fuzzey wrote:

> The event is triggered when the highpass filtered absolute acceleration
> exceeds the threshold.

comment below
 
> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
> ---
>  drivers/iio/accel/mma8452.c |  212 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 211 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index c46df4e..d44044f 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -9,7 +9,7 @@
>   *
>   * 7-bit I2C slave address 0x1c/0x1d (pin selectable)
>   *
> - * TODO: interrupt, thresholding, orientation / freefall events, autosleep
> + * TODO: orientation / freefall events, autosleep
>   */
>  
>  #include <linux/module.h>
> @@ -19,20 +19,33 @@
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/events.h>
>  #include <linux/delay.h>
>  
>  #define MMA8452_STATUS 0x00
>  #define MMA8452_OUT_X 0x01 /* MSB first, 12-bit  */
>  #define MMA8452_OUT_Y 0x03
>  #define MMA8452_OUT_Z 0x05
> +#define MMA8452_INT_SRC 0x0c
>  #define MMA8452_WHO_AM_I 0x0d
>  #define MMA8452_DATA_CFG 0x0e
> +#define MMA8452_TRANSIENT_CFG 0x1d
> +#define MMA8452_TRANSIENT_CFG_ELE		BIT(4)
> +#define MMA8452_TRANSIENT_CFG_CHAN_MASK(chan)	(BIT(1) << chan)
> +#define MMA8452_TRANSIENT_SRC 0x1e
> +#define MMA8452_TRANSIENT_SRC_XTRANSE		BIT(1)
> +#define MMA8452_TRANSIENT_SRC_YTRANSE		BIT(3)
> +#define MMA8452_TRANSIENT_SRC_ZTRANSE		BIT(5)
> +#define MMA8452_TRANSIENT_THS 0x1f
> +#define MMA8452_TRANSIENT_COUNT 0x20
>  #define MMA8452_OFF_X 0x2f
>  #define MMA8452_OFF_Y 0x30
>  #define MMA8452_OFF_Z 0x31
>  #define MMA8452_CTRL_REG1 0x2a
>  #define MMA8452_CTRL_REG2 0x2b
>  #define MMA8452_CTRL_REG2_RST		BIT(6)
> +#define MMA8452_CTRL_REG4 0x2d
> +#define MMA8452_CTRL_REG5 0x2e
>  
>  #define MMA8452_MAX_REG 0x31
>  
> @@ -48,6 +61,13 @@
>  #define MMA8452_DATA_CFG_FS_4G 1
>  #define MMA8452_DATA_CFG_FS_8G 2
>  
> +#define MMA8452_INT_DRDY	BIT(0)
> +#define MMA8452_INT_FF_MT	BIT(2)
> +#define MMA8452_INT_PULSE	BIT(3)
> +#define MMA8452_INT_LNDPRT	BIT(4)
> +#define MMA8452_INT_TRANS	BIT(5)
> +#define MMA8452_INT_ASLP	BIT(7)
> +
>  #define MMA8452_DEVICE_ID 0x2a
>  
>  struct mma8452_data {
> @@ -274,6 +294,121 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static int mma8452_read_thresh(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, enum iio_event_type type,
> +	enum iio_event_direction dir, enum iio_event_info info, int *val,
> +	int *val2)
> +{
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_THS);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = ret & 0x7f;
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int mma8452_write_thresh(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, enum iio_event_type type,
> +	enum iio_event_direction dir, enum iio_event_info info, int val,
> +	int val2)
> +{
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +
> +	return mma8452_change_config(data, MMA8452_TRANSIENT_THS, val & 0x7f);
> +}
> +
> +static int mma8452_read_event_config(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, enum iio_event_type type,
> +	enum iio_event_direction dir)
> +{
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_CFG);
> +	if (ret < 0)
> +		return ret;
> +
> +	return ret & MMA8452_TRANSIENT_CFG_CHAN_MASK(chan->scan_index) ? 1 : 0;
> +}
> +
> +static int mma8452_write_event_config(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, enum iio_event_type type,
> +	enum iio_event_direction dir, int state)
> +{
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +	int val;
> +
> +	val = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_CFG);
> +	if (val < 0)
> +		return val;
> +
> +	if (state)
> +		val |= MMA8452_TRANSIENT_CFG_CHAN_MASK(chan->scan_index);
> +	else
> +		val &= ~MMA8452_TRANSIENT_CFG_CHAN_MASK(chan->scan_index);
> +
> +	val |= MMA8452_TRANSIENT_CFG_ELE;
> +
> +	return mma8452_change_config(data, MMA8452_TRANSIENT_CFG, val);
> +}
> +
> +static void mma8452_transient_interrupt(struct iio_dev *indio_dev)
> +{
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +	s64 ts = iio_get_time_ns();
> +	int src;
> +
> +	src = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_SRC);
> +	if (src < 0)
> +		return;
> +
> +	if (src & MMA8452_TRANSIENT_SRC_XTRANSE)
> +		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL,
> +							0,
> +							IIO_MOD_X,
> +							IIO_EV_TYPE_THRESH,
> +							IIO_EV_DIR_RISING),
> +							ts);
> +
> +	if (src & MMA8452_TRANSIENT_SRC_YTRANSE)
> +		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL,
> +							0,
> +							IIO_MOD_Y,
> +							IIO_EV_TYPE_THRESH,
> +							IIO_EV_DIR_RISING),
> +							ts);
> +
> +	if (src & MMA8452_TRANSIENT_SRC_ZTRANSE)
> +		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL,
> +							0,
> +							IIO_MOD_Z,
> +							IIO_EV_TYPE_THRESH,
> +							IIO_EV_DIR_RISING),
> +							ts);
> +}
> +
> +static irqreturn_t mma8452_interrupt(int irq, void *p)
> +{
> +	struct iio_dev *indio_dev = p;
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +	int src;
> +
> +	src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
> +	if (src < 0)
> +		return IRQ_NONE;
> +
> +	if (src & MMA8452_INT_TRANS) {
> +		mma8452_transient_interrupt(indio_dev);
> +		return IRQ_HANDLED;
> +	}
> +
> +	return IRQ_NONE;
> +}
> +
>  static irqreturn_t mma8452_trigger_handler(int irq, void *p)
>  {
>  	struct iio_poll_func *pf = p;
> @@ -316,6 +451,32 @@ static int mma8452_reg_access_dbg(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> +static const struct iio_event_spec mma8452_transient_event[] = {
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> +		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> +					BIT(IIO_EV_INFO_HIGH_PASS_FILTER_3DB),
> +	},
> +};
> +
> +/**

just /*

> + * Threshold is configured in fixed 8G/127 steps regardless of
> + * currently selected scale for measurement.
> + */
> +static IIO_CONST_ATTR_NAMED(accel_transient_scale, in_accel_scale, "0.617742");
> +
> +static struct attribute *mma8452_event_attributes[] = {
> +	&iio_const_attr_accel_transient_scale.dev_attr.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group mma8452_event_attribute_group = {
> +	.attrs = mma8452_event_attributes,
> +	.name = "events",
> +};
> +
>  #define MMA8452_CHANNEL(axis, idx) { \
>  	.type = IIO_ACCEL, \
>  	.modified = 1, \
> @@ -332,6 +493,8 @@ static int mma8452_reg_access_dbg(struct iio_dev *indio_dev,
>  		.shift = 4, \
>  		.endianness = IIO_BE, \
>  	}, \
> +	.event_spec = mma8452_transient_event, \
> +	.num_event_specs = ARRAY_SIZE(mma8452_transient_event), \
>  }
>  
>  static const struct iio_chan_spec mma8452_channels[] = {
> @@ -355,6 +518,11 @@ static const struct iio_info mma8452_info = {
>  	.attrs = &mma8452_group,
>  	.read_raw = &mma8452_read_raw,
>  	.write_raw = &mma8452_write_raw,
> +	.event_attrs = &mma8452_event_attribute_group,
> +	.read_event_value = &mma8452_read_thresh,
> +	.write_event_value = &mma8452_write_thresh,
> +	.read_event_config = &mma8452_read_event_config,
> +	.write_event_config = &mma8452_write_event_config,
>  	.debugfs_reg_access = &mma8452_reg_access_dbg,
>  	.driver_module = THIS_MODULE,
>  };
> @@ -425,6 +593,37 @@ static int mma8452_probe(struct i2c_client *client,
>  	if (ret < 0)
>  		return ret;
>  
> +	/*
> +	 * By default set transient threshold to max to avoid events if
> +	 * enabling without configuring threshold
> +	 */
> +	ret = i2c_smbus_write_byte_data(client, MMA8452_TRANSIENT_THS, 0x7f);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (client->irq) {
> +		/*
> +		 * Although we enable the transient interrupt source once and
> +		 * for all here the transient event detection itself is not
> +		 * enabled until userspace asks for it by
> +		 * mma8452_write_event_config()
> +		 */
> +		int supported_interrupts = MMA8452_INT_TRANS;
> +
> +		/* Assume wired to INT1 pin */
> +		ret = i2c_smbus_write_byte_data(client,
> +						MMA8452_CTRL_REG5,
> +						supported_interrupts);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = i2c_smbus_write_byte_data(client,
> +						MMA8452_CTRL_REG4,
> +						supported_interrupts);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
>  		(MMA8452_CTRL_DR_DEFAULT << MMA8452_CTRL_DR_SHIFT);
>  	ret = i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG1,
> @@ -437,9 +636,20 @@ static int mma8452_probe(struct i2c_client *client,
>  	if (ret < 0)
>  		return ret;
>  
> +	if (client->irq) {
> +		ret = devm_request_threaded_irq(&client->dev,
> +						client->irq,
> +						NULL, mma8452_interrupt,
> +						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> +						client->name, indio_dev);
> +		if (ret)
> +			goto buffer_cleanup;
> +	}
> +
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0)
>  		goto buffer_cleanup;
> +
>  	return 0;
>  
>  buffer_cleanup:
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: [PATCH 4/9] iio: mma8452: Basic support for transient events.

[Hartmut Knaack]

Martin Fuzzey schrieb am 19.02.2015 um 15:16:
> The event is triggered when the highpass filtered absolute acceleration
> exceeds the threshold.
> 
The strict checks of checkpatch.pl complain about several alignment issues
of function parameters. Besides, I have put a few comments inline.

> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
> ---
>  drivers/iio/accel/mma8452.c |  212 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 211 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index c46df4e..d44044f 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -9,7 +9,7 @@
>   *
>   * 7-bit I2C slave address 0x1c/0x1d (pin selectable)
>   *
> - * TODO: interrupt, thresholding, orientation / freefall events, autosleep
> + * TODO: orientation / freefall events, autosleep
>   */
>  
>  #include <linux/module.h>
> @@ -19,20 +19,33 @@
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/events.h>
>  #include <linux/delay.h>
>  
>  #define MMA8452_STATUS 0x00
>  #define MMA8452_OUT_X 0x01 /* MSB first, 12-bit  */
>  #define MMA8452_OUT_Y 0x03
>  #define MMA8452_OUT_Z 0x05
> +#define MMA8452_INT_SRC 0x0c
>  #define MMA8452_WHO_AM_I 0x0d
>  #define MMA8452_DATA_CFG 0x0e
> +#define MMA8452_TRANSIENT_CFG 0x1d
> +#define MMA8452_TRANSIENT_CFG_ELE		BIT(4)
> +#define MMA8452_TRANSIENT_CFG_CHAN_MASK(chan)	(BIT(1) << chan)
This is not a mask but a channel selection, so that part of the name can
be dropped. Besides, I would define it as BIT(chan + 1)

> +#define MMA8452_TRANSIENT_SRC 0x1e
> +#define MMA8452_TRANSIENT_SRC_XTRANSE		BIT(1)
> +#define MMA8452_TRANSIENT_SRC_YTRANSE		BIT(3)
> +#define MMA8452_TRANSIENT_SRC_ZTRANSE		BIT(5)
> +#define MMA8452_TRANSIENT_THS 0x1f
> +#define MMA8452_TRANSIENT_COUNT 0x20
This one is unused.

>  #define MMA8452_OFF_X 0x2f
>  #define MMA8452_OFF_Y 0x30
>  #define MMA8452_OFF_Z 0x31
>  #define MMA8452_CTRL_REG1 0x2a
>  #define MMA8452_CTRL_REG2 0x2b
>  #define MMA8452_CTRL_REG2_RST		BIT(6)
> +#define MMA8452_CTRL_REG4 0x2d
> +#define MMA8452_CTRL_REG5 0x2e
>  
>  #define MMA8452_MAX_REG 0x31
>  
> @@ -48,6 +61,13 @@
>  #define MMA8452_DATA_CFG_FS_4G 1
>  #define MMA8452_DATA_CFG_FS_8G 2
>  
> +#define MMA8452_INT_DRDY	BIT(0)
> +#define MMA8452_INT_FF_MT	BIT(2)
> +#define MMA8452_INT_PULSE	BIT(3)
> +#define MMA8452_INT_LNDPRT	BIT(4)
> +#define MMA8452_INT_TRANS	BIT(5)
> +#define MMA8452_INT_ASLP	BIT(7)
All except MMA8452_INT_TRANS are unused.

> +
>  #define MMA8452_DEVICE_ID 0x2a
>  
>  struct mma8452_data {
> @@ -274,6 +294,121 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static int mma8452_read_thresh(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, enum iio_event_type type,
> +	enum iio_event_direction dir, enum iio_event_info info, int *val,
> +	int *val2)
> +{
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_THS);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = ret & 0x7f;
It would be better to define the threshold mask and then make use of it here
and in those other instances below.

> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int mma8452_write_thresh(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, enum iio_event_type type,
> +	enum iio_event_direction dir, enum iio_event_info info, int val,
> +	int val2)
> +{
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +
> +	return mma8452_change_config(data, MMA8452_TRANSIENT_THS, val & 0x7f);
Like here.

> +}
> +
> +static int mma8452_read_event_config(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, enum iio_event_type type,
> +	enum iio_event_direction dir)
> +{
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_CFG);
> +	if (ret < 0)
> +		return ret;
> +
> +	return ret & MMA8452_TRANSIENT_CFG_CHAN_MASK(chan->scan_index) ? 1 : 0;
return !!(ret & MMA8452_TRANSIENT_CFG_CHAN_MASK(chan->scan_index)); does the same.

> +}
> +
> +static int mma8452_write_event_config(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, enum iio_event_type type,
> +	enum iio_event_direction dir, int state)
> +{
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +	int val;
Better use ret for consistency?
> +
> +	val = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_CFG);
> +	if (val < 0)
> +		return val;
> +
> +	if (state)
> +		val |= MMA8452_TRANSIENT_CFG_CHAN_MASK(chan->scan_index);
> +	else
> +		val &= ~MMA8452_TRANSIENT_CFG_CHAN_MASK(chan->scan_index);
> +
> +	val |= MMA8452_TRANSIENT_CFG_ELE;
> +
> +	return mma8452_change_config(data, MMA8452_TRANSIENT_CFG, val);
> +}
> +
> +static void mma8452_transient_interrupt(struct iio_dev *indio_dev)
> +{
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +	s64 ts = iio_get_time_ns();
> +	int src;
> +
> +	src = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_SRC);
> +	if (src < 0)
> +		return;
> +
> +	if (src & MMA8452_TRANSIENT_SRC_XTRANSE)
> +		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL,
> +							0,
> +							IIO_MOD_X,
> +							IIO_EV_TYPE_THRESH,
> +							IIO_EV_DIR_RISING),
> +							ts);
I would use the following indentation:
		iio_push_event(indio_dev,
			       IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, IIO_MOD_X,
						  IIO_EV_TYPE_THRESH,
						  IIO_EV_DIR_RISING),
			       ts);

> +
> +	if (src & MMA8452_TRANSIENT_SRC_YTRANSE)
> +		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL,
> +							0,
> +							IIO_MOD_Y,
> +							IIO_EV_TYPE_THRESH,
> +							IIO_EV_DIR_RISING),
> +							ts);
> +
> +	if (src & MMA8452_TRANSIENT_SRC_ZTRANSE)
> +		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL,
> +							0,
> +							IIO_MOD_Z,
> +							IIO_EV_TYPE_THRESH,
> +							IIO_EV_DIR_RISING),
> +							ts);
> +}
> +
> +static irqreturn_t mma8452_interrupt(int irq, void *p)
> +{
> +	struct iio_dev *indio_dev = p;
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +	int src;
> +
> +	src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
> +	if (src < 0)
You could combine this check with !(src & MMA8452_INT_TRANS).

> +		return IRQ_NONE;
> +
> +	if (src & MMA8452_INT_TRANS) {
> +		mma8452_transient_interrupt(indio_dev);
> +		return IRQ_HANDLED;
> +	}
> +
> +	return IRQ_NONE;
> +}
> +
>  static irqreturn_t mma8452_trigger_handler(int irq, void *p)
>  {
>  	struct iio_poll_func *pf = p;
> @@ -316,6 +451,32 @@ static int mma8452_reg_access_dbg(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> +static const struct iio_event_spec mma8452_transient_event[] = {
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> +		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> +					BIT(IIO_EV_INFO_HIGH_PASS_FILTER_3DB),
This would look better if you align the bottom line one space to the left.

> +	},
> +};
> +
> +/**
> + * Threshold is configured in fixed 8G/127 steps regardless of
> + * currently selected scale for measurement.
> + */
> +static IIO_CONST_ATTR_NAMED(accel_transient_scale, in_accel_scale, "0.617742");
what's this 0.617742 based on?

> +
> +static struct attribute *mma8452_event_attributes[] = {
> +	&iio_const_attr_accel_transient_scale.dev_attr.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group mma8452_event_attribute_group = {
> +	.attrs = mma8452_event_attributes,
> +	.name = "events",
> +};
> +
>  #define MMA8452_CHANNEL(axis, idx) { \
>  	.type = IIO_ACCEL, \
>  	.modified = 1, \
> @@ -332,6 +493,8 @@ static int mma8452_reg_access_dbg(struct iio_dev *indio_dev,
>  		.shift = 4, \
>  		.endianness = IIO_BE, \
>  	}, \
> +	.event_spec = mma8452_transient_event, \
> +	.num_event_specs = ARRAY_SIZE(mma8452_transient_event), \
>  }
>  
>  static const struct iio_chan_spec mma8452_channels[] = {
> @@ -355,6 +518,11 @@ static const struct iio_info mma8452_info = {
>  	.attrs = &mma8452_group,
>  	.read_raw = &mma8452_read_raw,
>  	.write_raw = &mma8452_write_raw,
> +	.event_attrs = &mma8452_event_attribute_group,
> +	.read_event_value = &mma8452_read_thresh,
> +	.write_event_value = &mma8452_write_thresh,
> +	.read_event_config = &mma8452_read_event_config,
> +	.write_event_config = &mma8452_write_event_config,
>  	.debugfs_reg_access = &mma8452_reg_access_dbg,
>  	.driver_module = THIS_MODULE,
>  };
> @@ -425,6 +593,37 @@ static int mma8452_probe(struct i2c_client *client,
>  	if (ret < 0)
>  		return ret;
>  
> +	/*
> +	 * By default set transient threshold to max to avoid events if
> +	 * enabling without configuring threshold
This sentence should be finished with a full stop.

> +	 */
> +	ret = i2c_smbus_write_byte_data(client, MMA8452_TRANSIENT_THS, 0x7f);
Yet another instance, which would benefit from the mentioned definition.

> +	if (ret < 0)
> +		return ret;
> +
> +	if (client->irq) {
> +		/*
> +		 * Although we enable the transient interrupt source once and
> +		 * for all here the transient event detection itself is not
> +		 * enabled until userspace asks for it by
> +		 * mma8452_write_event_config()
> +		 */
> +		int supported_interrupts = MMA8452_INT_TRANS;
> +
> +		/* Assume wired to INT1 pin */
> +		ret = i2c_smbus_write_byte_data(client,
> +						MMA8452_CTRL_REG5,
> +						supported_interrupts);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = i2c_smbus_write_byte_data(client,
> +						MMA8452_CTRL_REG4,
> +						supported_interrupts);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
>  		(MMA8452_CTRL_DR_DEFAULT << MMA8452_CTRL_DR_SHIFT);
>  	ret = i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG1,
> @@ -437,9 +636,20 @@ static int mma8452_probe(struct i2c_client *client,
>  	if (ret < 0)
>  		return ret;
>  
> +	if (client->irq) {
> +		ret = devm_request_threaded_irq(&client->dev,
> +						client->irq,
> +						NULL, mma8452_interrupt,
> +						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> +						client->name, indio_dev);
> +		if (ret)
> +			goto buffer_cleanup;
> +	}
> +
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0)
>  		goto buffer_cleanup;
> +
>  	return 0;
>  
>  buffer_cleanup:
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 5/9] iio: doc: Describe scale attributes for event thresholds

[Martin Fuzzey]

Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
---
 Documentation/ABI/testing/sysfs-bus-iio |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index ab592dd..d15a61d 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -613,6 +613,24 @@ Description:
 		value is in raw device units or in processed units (as _raw
 		and _input do on sysfs direct channel read attributes).
 
+What:		/sys/.../events/in_accel_scale
+What:		/sys/.../events/in_accel_peak_scale
+What:		/sys/.../events/in_anglvel_scale
+What:		/sys/.../events/in_magn_scale
+What:		/sys/.../events/in_rot_from_north_magnetic_scale
+What:		/sys/.../events/in_rot_from_north_true_scale
+What:		/sys/.../events/in_voltage_scale
+What:		/sys/.../events/in_voltage_supply_scale
+What:		/sys/.../events/in_temp_scale
+What:		/sys/.../events/in_illuminance_scale
+What:		/sys/.../events/in_proximity_scale
+KernelVersion:	3.21
+Contact:	linux-iio@vger.kernel.org
+Description:
+                Specifies the conversion factor from the standard units
+                to device specific units used to set the event trigger
+                threshold.
+
 What:		/sys/.../events/in_accel_x_thresh_rising_hysteresis
 What:		/sys/.../events/in_accel_x_thresh_falling_hysteresis
 What:		/sys/.../events/in_accel_x_thresh_either_hysteresis

Re: [PATCH 5/9] iio: doc: Describe scale attributes for event thresholds

[Jonathan Cameron]

On 19/02/15 14:16, Martin Fuzzey wrote:
> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
Applied to the togreg branch of iio.git. 
These have been in the effective ABI for a while even if
we didn't have them documented so this patch stands
fine on it's own.

Jonathan
> ---
>  Documentation/ABI/testing/sysfs-bus-iio |   18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index ab592dd..d15a61d 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -613,6 +613,24 @@ Description:
>  		value is in raw device units or in processed units (as _raw
>  		and _input do on sysfs direct channel read attributes).
>  
> +What:		/sys/.../events/in_accel_scale
> +What:		/sys/.../events/in_accel_peak_scale
> +What:		/sys/.../events/in_anglvel_scale
> +What:		/sys/.../events/in_magn_scale
> +What:		/sys/.../events/in_rot_from_north_magnetic_scale
> +What:		/sys/.../events/in_rot_from_north_true_scale
> +What:		/sys/.../events/in_voltage_scale
> +What:		/sys/.../events/in_voltage_supply_scale
> +What:		/sys/.../events/in_temp_scale
> +What:		/sys/.../events/in_illuminance_scale
> +What:		/sys/.../events/in_proximity_scale
> +KernelVersion:	3.21
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +                Specifies the conversion factor from the standard units
> +                to device specific units used to set the event trigger
> +                threshold.
> +
>  What:		/sys/.../events/in_accel_x_thresh_rising_hysteresis
>  What:		/sys/.../events/in_accel_x_thresh_falling_hysteresis
>  What:		/sys/.../events/in_accel_x_thresh_either_hysteresis
> 

Re: [PATCH 5/9] iio: doc: Describe scale attributes for event thresholds

[Hartmut Knaack]

Jonathan Cameron schrieb am 09.03.2015 um 14:31:
> On 19/02/15 14:16, Martin Fuzzey wrote:
>> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
> Applied to the togreg branch of iio.git. 
> These have been in the effective ABI for a while even if
> we didn't have them documented so this patch stands
> fine on it's own.
> 
Annoyingly, there is a new version scheme. Who likes to adjust
this one here?

> Jonathan
>> ---
>>  Documentation/ABI/testing/sysfs-bus-iio |   18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
>> index ab592dd..d15a61d 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>> @@ -613,6 +613,24 @@ Description:
>>  		value is in raw device units or in processed units (as _raw
>>  		and _input do on sysfs direct channel read attributes).
>>  
>> +What:		/sys/.../events/in_accel_scale
>> +What:		/sys/.../events/in_accel_peak_scale
>> +What:		/sys/.../events/in_anglvel_scale
>> +What:		/sys/.../events/in_magn_scale
>> +What:		/sys/.../events/in_rot_from_north_magnetic_scale
>> +What:		/sys/.../events/in_rot_from_north_true_scale
>> +What:		/sys/.../events/in_voltage_scale
>> +What:		/sys/.../events/in_voltage_supply_scale
>> +What:		/sys/.../events/in_temp_scale
>> +What:		/sys/.../events/in_illuminance_scale
>> +What:		/sys/.../events/in_proximity_scale
>> +KernelVersion:	3.21
>> +Contact:	linux-iio@vger.kernel.org
>> +Description:
>> +                Specifies the conversion factor from the standard units
>> +                to device specific units used to set the event trigger
>> +                threshold.
>> +
>>  What:		/sys/.../events/in_accel_x_thresh_rising_hysteresis
>>  What:		/sys/.../events/in_accel_x_thresh_falling_hysteresis
>>  What:		/sys/.../events/in_accel_x_thresh_either_hysteresis
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 5/9] iio: doc: Describe scale attributes for event thresholds

[Jonathan Cameron]

On 16/04/15 23:36, Hartmut Knaack wrote:
> Jonathan Cameron schrieb am 09.03.2015 um 14:31:
>> On 19/02/15 14:16, Martin Fuzzey wrote:
>>> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
>> Applied to the togreg branch of iio.git. 
>> These have been in the effective ABI for a while even if
>> we didn't have them documented so this patch stands
>> fine on it's own.
>>
> Annoyingly, there is a new version scheme. Who likes to adjust
> this one here?
> 
I'm expressing my irritation at Linus by ignoring these,
though will happily take other's patches for them ;)
>> Jonathan
>>> ---
>>>  Documentation/ABI/testing/sysfs-bus-iio |   18 ++++++++++++++++++
>>>  1 file changed, 18 insertions(+)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
>>> index ab592dd..d15a61d 100644
>>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>>> @@ -613,6 +613,24 @@ Description:
>>>  		value is in raw device units or in processed units (as _raw
>>>  		and _input do on sysfs direct channel read attributes).
>>>  
>>> +What:		/sys/.../events/in_accel_scale
>>> +What:		/sys/.../events/in_accel_peak_scale
>>> +What:		/sys/.../events/in_anglvel_scale
>>> +What:		/sys/.../events/in_magn_scale
>>> +What:		/sys/.../events/in_rot_from_north_magnetic_scale
>>> +What:		/sys/.../events/in_rot_from_north_true_scale
>>> +What:		/sys/.../events/in_voltage_scale
>>> +What:		/sys/.../events/in_voltage_supply_scale
>>> +What:		/sys/.../events/in_temp_scale
>>> +What:		/sys/.../events/in_illuminance_scale
>>> +What:		/sys/.../events/in_proximity_scale
>>> +KernelVersion:	3.21
>>> +Contact:	linux-iio@vger.kernel.org
>>> +Description:
>>> +                Specifies the conversion factor from the standard units
>>> +                to device specific units used to set the event trigger
>>> +                threshold.
>>> +
>>>  What:		/sys/.../events/in_accel_x_thresh_rising_hysteresis
>>>  What:		/sys/.../events/in_accel_x_thresh_falling_hysteresis
>>>  What:		/sys/.../events/in_accel_x_thresh_either_hysteresis
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 

[PATCH 6/9] iio: mma8452: Add support for transient event debouncing

[Martin Fuzzey]

Allow the debouce counter for transient events to be configured
using the sysfs attribute events/in_accel_thresh_rising_period

Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
---
 drivers/iio/accel/mma8452.c |   74 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 64 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index d44044f..b7f13dd 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -129,6 +129,12 @@ static int mma8452_get_int_plus_micros_index(const int (*vals)[2], int n,
 	return -EINVAL;
 }
 
+static int mma8452_get_odr_index(struct mma8452_data *data)
+{
+	return (data->ctrl_reg1 & MMA8452_CTRL_DR_MASK) >>
+			MMA8452_CTRL_DR_SHIFT;
+}
+
 static const int mma8452_samp_freq[8][2] = {
 	{800, 0}, {400, 0}, {200, 0}, {100, 0}, {50, 0}, {12, 500000},
 	{6, 250000}, {1, 560000}
@@ -144,6 +150,18 @@ static const int mma8452_scales[3][2] = {
 	{0, 9577}, {0, 19154}, {0, 38307}
 };
 
+/* Datasheet table 35  (step time vs sample frequency) */
+static const int mma8452_transient_time_step_us[8] = {
+	1250,
+	2500,
+	5000,
+	10000,
+	20000,
+	20000,
+	20000,
+	20000
+};
+
 static ssize_t mma8452_show_samp_freq_avail(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
@@ -203,8 +221,7 @@ static int mma8452_read_raw(struct iio_dev *indio_dev,
 		*val2 = mma8452_scales[i][1];
 		return IIO_VAL_INT_PLUS_MICRO;
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		i = (data->ctrl_reg1 & MMA8452_CTRL_DR_MASK) >>
-			MMA8452_CTRL_DR_SHIFT;
+		i = mma8452_get_odr_index(data);
 		*val = mma8452_samp_freq[i][0];
 		*val2 = mma8452_samp_freq[i][1];
 		return IIO_VAL_INT_PLUS_MICRO;
@@ -300,15 +317,33 @@ static int mma8452_read_thresh(struct iio_dev *indio_dev,
 	int *val2)
 {
 	struct mma8452_data *data = iio_priv(indio_dev);
-	int ret;
+	int ret, us;
 
-	ret = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_THS);
-	if (ret < 0)
-		return ret;
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		ret = i2c_smbus_read_byte_data(data->client,
+						MMA8452_TRANSIENT_THS);
+		if (ret < 0)
+			return ret;
+
+		*val = ret & 0x7f;
+		return IIO_VAL_INT;
 
-	*val = ret & 0x7f;
+	case IIO_EV_INFO_PERIOD:
+		ret = i2c_smbus_read_byte_data(data->client,
+						MMA8452_TRANSIENT_COUNT);
+		if (ret < 0)
+			return ret;
+
+		us = ret * mma8452_transient_time_step_us[
+				mma8452_get_odr_index(data)];
+		*val = us / USEC_PER_SEC;
+		*val2 = us % USEC_PER_SEC;
+		return IIO_VAL_INT_PLUS_MICRO;
 
-	return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
 }
 
 static int mma8452_write_thresh(struct iio_dev *indio_dev,
@@ -317,8 +352,26 @@ static int mma8452_write_thresh(struct iio_dev *indio_dev,
 	int val2)
 {
 	struct mma8452_data *data = iio_priv(indio_dev);
+	int steps;
 
-	return mma8452_change_config(data, MMA8452_TRANSIENT_THS, val & 0x7f);
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		return mma8452_change_config(data,
+			MMA8452_TRANSIENT_THS, val & 0x7f);
+
+	case IIO_EV_INFO_PERIOD:
+		steps = (val * USEC_PER_SEC + val2) /
+				mma8452_transient_time_step_us[
+					mma8452_get_odr_index(data)];
+
+		if (steps > 0xff)
+			return -EINVAL;
+
+		return mma8452_change_config(data,
+				MMA8452_TRANSIENT_COUNT, steps);
+	default:
+		return -EINVAL;
+	}
 }
 
 static int mma8452_read_event_config(struct iio_dev *indio_dev,
@@ -457,7 +510,8 @@ static const struct iio_event_spec mma8452_transient_event[] = {
 		.dir = IIO_EV_DIR_RISING,
 		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
 		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
-					BIT(IIO_EV_INFO_HIGH_PASS_FILTER_3DB),
+					BIT(IIO_EV_INFO_HIGH_PASS_FILTER_3DB) |
+					BIT(IIO_EV_INFO_PERIOD),
 	},
 };
 

Re: [PATCH 6/9] iio: mma8452: Add support for transient event debouncing

[Hartmut Knaack]

Martin Fuzzey schrieb am 19.02.2015 um 15:16:
> Allow the debouce counter for transient events to be configured
> using the sysfs attribute events/in_accel_thresh_rising_period
> 

The same issue about parameter alignment applies here as well. Besides, I put
some comments inline.

> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
> ---
>  drivers/iio/accel/mma8452.c |   74 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 64 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index d44044f..b7f13dd 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -129,6 +129,12 @@ static int mma8452_get_int_plus_micros_index(const int (*vals)[2], int n,
>  	return -EINVAL;
>  }
>  
> +static int mma8452_get_odr_index(struct mma8452_data *data)
What about u8 as return type, since data->ctrl_reg1 is u8, too?

> +{
> +	return (data->ctrl_reg1 & MMA8452_CTRL_DR_MASK) >>
> +			MMA8452_CTRL_DR_SHIFT;
> +}
> +
>  static const int mma8452_samp_freq[8][2] = {
>  	{800, 0}, {400, 0}, {200, 0}, {100, 0}, {50, 0}, {12, 500000},
>  	{6, 250000}, {1, 560000}
> @@ -144,6 +150,18 @@ static const int mma8452_scales[3][2] = {
>  	{0, 9577}, {0, 19154}, {0, 38307}
>  };
>  
> +/* Datasheet table 35  (step time vs sample frequency) */
> +static const int mma8452_transient_time_step_us[8] = {
This could be unsigned.

> +	1250,
> +	2500,
> +	5000,
> +	10000,
> +	20000,
> +	20000,
> +	20000,
> +	20000
> +};
> +
>  static ssize_t mma8452_show_samp_freq_avail(struct device *dev,
>  				struct device_attribute *attr, char *buf)
>  {
> @@ -203,8 +221,7 @@ static int mma8452_read_raw(struct iio_dev *indio_dev,
>  		*val2 = mma8452_scales[i][1];
>  		return IIO_VAL_INT_PLUS_MICRO;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		i = (data->ctrl_reg1 & MMA8452_CTRL_DR_MASK) >>
> -			MMA8452_CTRL_DR_SHIFT;
> +		i = mma8452_get_odr_index(data);
>  		*val = mma8452_samp_freq[i][0];
>  		*val2 = mma8452_samp_freq[i][1];
>  		return IIO_VAL_INT_PLUS_MICRO;
> @@ -300,15 +317,33 @@ static int mma8452_read_thresh(struct iio_dev *indio_dev,
>  	int *val2)
>  {
>  	struct mma8452_data *data = iio_priv(indio_dev);
> -	int ret;
> +	int ret, us;
>  
> -	ret = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_THS);
> -	if (ret < 0)
> -		return ret;
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		ret = i2c_smbus_read_byte_data(data->client,
> +						MMA8452_TRANSIENT_THS);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = ret & 0x7f;
> +		return IIO_VAL_INT;
>  
> -	*val = ret & 0x7f;
> +	case IIO_EV_INFO_PERIOD:
> +		ret = i2c_smbus_read_byte_data(data->client,
> +						MMA8452_TRANSIENT_COUNT);
> +		if (ret < 0)
> +			return ret;
> +
> +		us = ret * mma8452_transient_time_step_us[
> +				mma8452_get_odr_index(data)];
> +		*val = us / USEC_PER_SEC;
> +		*val2 = us % USEC_PER_SEC;
> +		return IIO_VAL_INT_PLUS_MICRO;
You could make use of core calculations and also make us obsolete:
		*val = ret * mma8452_transient_time_step_us[
						mma8452_get_odr_index(data)];
		*val2 = USEC_PER_SEC;
		return IIO_VAL_FRACTIONAL;

>  
> -	return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
>  }
>  
>  static int mma8452_write_thresh(struct iio_dev *indio_dev,
> @@ -317,8 +352,26 @@ static int mma8452_write_thresh(struct iio_dev *indio_dev,
>  	int val2)
>  {
>  	struct mma8452_data *data = iio_priv(indio_dev);
> +	int steps;
>  
> -	return mma8452_change_config(data, MMA8452_TRANSIENT_THS, val & 0x7f);
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		return mma8452_change_config(data,
> +			MMA8452_TRANSIENT_THS, val & 0x7f);
Instead of masking val into the right range, better do a boundary check and
report an error on failure?

> +
> +	case IIO_EV_INFO_PERIOD:
> +		steps = (val * USEC_PER_SEC + val2) /
> +				mma8452_transient_time_step_us[
> +					mma8452_get_odr_index(data)];
> +
> +		if (steps > 0xff)
Use something like MMA8452_TRANSIENT_COUNT_MASK instead of magic number?

> +			return -EINVAL;
> +
> +		return mma8452_change_config(data,
> +				MMA8452_TRANSIENT_COUNT, steps);
> +	default:
> +		return -EINVAL;
> +	}
>  }
>  
>  static int mma8452_read_event_config(struct iio_dev *indio_dev,
> @@ -457,7 +510,8 @@ static const struct iio_event_spec mma8452_transient_event[] = {
>  		.dir = IIO_EV_DIR_RISING,
>  		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
>  		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> -					BIT(IIO_EV_INFO_HIGH_PASS_FILTER_3DB),
> +					BIT(IIO_EV_INFO_HIGH_PASS_FILTER_3DB) |
> +					BIT(IIO_EV_INFO_PERIOD),
>  	},
>  };
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 7/9] iio: mma8452: Add highpass filter configuration.

[Martin Fuzzey]

Allow the cutoff frequency of the high pass filter to be configured.

Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
---
 drivers/iio/accel/mma8452.c |  135 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 132 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index b7f13dd..fe17c42 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -29,9 +29,12 @@
 #define MMA8452_INT_SRC 0x0c
 #define MMA8452_WHO_AM_I 0x0d
 #define MMA8452_DATA_CFG 0x0e
+#define MMA8452_HP_FILTER_CUTOFF 0x0f
+#define MMA8452_HP_FILTER_CUTOFF_SEL_MASK	(BIT(0) | BIT(1))
 #define MMA8452_TRANSIENT_CFG 0x1d
 #define MMA8452_TRANSIENT_CFG_ELE		BIT(4)
 #define MMA8452_TRANSIENT_CFG_CHAN_MASK(chan)	(BIT(1) << chan)
+#define MMA8452_TRANSIENT_CFG_HPF_BYP		BIT(0)
 #define MMA8452_TRANSIENT_SRC 0x1e
 #define MMA8452_TRANSIENT_SRC_XTRANSE		BIT(1)
 #define MMA8452_TRANSIENT_SRC_YTRANSE		BIT(3)
@@ -60,6 +63,7 @@
 #define MMA8452_DATA_CFG_FS_2G 0
 #define MMA8452_DATA_CFG_FS_4G 1
 #define MMA8452_DATA_CFG_FS_8G 2
+#define MMA8452_DATA_CFG_HPF_MASK BIT(4)
 
 #define MMA8452_INT_DRDY	BIT(0)
 #define MMA8452_INT_FF_MT	BIT(2)
@@ -162,6 +166,18 @@ static const int mma8452_transient_time_step_us[8] = {
 	20000
 };
 
+/* Datasheet table 18 (normal mode) */
+static const int mma8452_hp_filter_cutoff[8][4][2] = {
+	{ {16, 0}, {8, 0}, {4, 0}, {2, 0} },		/* 800 Hz sample */
+	{ {16, 0}, {8, 0}, {4, 0}, {2, 0} },		/* 400 Hz sample */
+	{ {8, 0}, {4, 0}, {2, 0}, {1, 0} },		/* 200 Hz sample */
+	{ {4, 0}, {2, 0}, {1, 0}, {0, 500000} },	/* 100 Hz sample */
+	{ {2, 0}, {1, 0}, {0, 500000}, {0, 250000} },	/* 50 Hz sample */
+	{ {2, 0}, {1, 0}, {0, 500000}, {0, 250000} },	/* 12.5 Hz sample */
+	{ {2, 0}, {1, 0}, {0, 500000}, {0, 250000} },	/* 6.25 Hz sample */
+	{ {2, 0}, {1, 0}, {0, 500000}, {0, 250000} }	/* 1.56 Hz sample */
+};
+
 static ssize_t mma8452_show_samp_freq_avail(struct device *dev,
 				struct device_attribute *attr, char *buf)
 {
@@ -176,9 +192,22 @@ static ssize_t mma8452_show_scale_avail(struct device *dev,
 		ARRAY_SIZE(mma8452_scales));
 }
 
+static ssize_t mma8452_show_hp_cutoff_avail(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct mma8452_data *data = iio_priv(indio_dev);
+	int i = mma8452_get_odr_index(data);
+
+	return mma8452_show_int_plus_micros(buf, mma8452_hp_filter_cutoff[i],
+		ARRAY_SIZE(mma8452_hp_filter_cutoff[0]));
+}
+
 static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(mma8452_show_samp_freq_avail);
 static IIO_DEVICE_ATTR(in_accel_scale_available, S_IRUGO,
 	mma8452_show_scale_avail, NULL, 0);
+static IIO_DEVICE_ATTR(in_accel_filter_high_pass_3db_frequency_available,
+			S_IRUGO, mma8452_show_hp_cutoff_avail, NULL, 0);
 
 static int mma8452_get_samp_freq_index(struct mma8452_data *data,
 	int val, int val2)
@@ -194,6 +223,30 @@ static int mma8452_get_scale_index(struct mma8452_data *data,
 		ARRAY_SIZE(mma8452_scales), val, val2);
 }
 
+static int mma8452_get_hp_filter_index(struct mma8452_data *data,
+	int val, int val2)
+{
+	int i = mma8452_get_odr_index(data);
+
+	return mma8452_get_int_plus_micros_index(mma8452_hp_filter_cutoff[i],
+		ARRAY_SIZE(mma8452_scales[0]), val, val2);
+}
+
+static int mma8452_read_hp_filter(struct mma8452_data *data, int *hz, int *uHz)
+{
+	int i, ret;
+
+	ret = i2c_smbus_read_byte_data(data->client, MMA8452_HP_FILTER_CUTOFF);
+	if (ret < 0)
+		return ret;
+
+	i = mma8452_get_odr_index(data);
+	ret &= MMA8452_HP_FILTER_CUTOFF_SEL_MASK;
+	*hz = mma8452_hp_filter_cutoff[i][ret][0];
+	*uHz = mma8452_hp_filter_cutoff[i][ret][1];
+	return 0;
+}
+
 static int mma8452_read_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int *val, int *val2, long mask)
@@ -232,6 +285,16 @@ static int mma8452_read_raw(struct iio_dev *indio_dev,
 			return ret;
 		*val = sign_extend32(ret, 7);
 		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
+		if (data->data_cfg & MMA8452_DATA_CFG_HPF_MASK) {
+			ret = mma8452_read_hp_filter(data, val, val2);
+			if (ret < 0)
+				return ret;
+		} else {
+			*val = 0;
+			*val2 = 0;
+		}
+		return IIO_VAL_INT_PLUS_MICRO;
 	}
 	return -EINVAL;
 }
@@ -273,12 +336,30 @@ fail:
 	return ret;
 }
 
+static int mma8452_set_hp_filter_frequency(struct mma8452_data *data,
+						int val, int val2)
+{
+	int i, reg;
+
+	i = mma8452_get_hp_filter_index(data, val, val2);
+	if (i < 0)
+		return -EINVAL;
+
+	reg = i2c_smbus_read_byte_data(data->client,
+					MMA8452_HP_FILTER_CUTOFF);
+	if (reg < 0)
+		return reg;
+	reg &= ~MMA8452_HP_FILTER_CUTOFF_SEL_MASK;
+	reg |= i;
+	return mma8452_change_config(data, MMA8452_HP_FILTER_CUTOFF, reg);
+}
+
 static int mma8452_write_raw(struct iio_dev *indio_dev,
 			     struct iio_chan_spec const *chan,
 			     int val, int val2, long mask)
 {
 	struct mma8452_data *data = iio_priv(indio_dev);
-	int i;
+	int i, ret;
 
 	if (iio_buffer_enabled(indio_dev))
 		return -EBUSY;
@@ -306,6 +387,19 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
 			return -EINVAL;
 		return mma8452_change_config(data, MMA8452_OFF_X +
 			chan->scan_index, val);
+
+	case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
+		if (val == 0 && val2 == 0) {
+			data->data_cfg &= ~MMA8452_DATA_CFG_HPF_MASK;
+		} else {
+			data->data_cfg |= MMA8452_DATA_CFG_HPF_MASK;
+			ret = mma8452_set_hp_filter_frequency(data, val, val2);
+			if (ret < 0)
+				return ret;
+		}
+		return mma8452_change_config(data, MMA8452_DATA_CFG,
+						data->data_cfg);
+
 	default:
 		return -EINVAL;
 	}
@@ -341,6 +435,22 @@ static int mma8452_read_thresh(struct iio_dev *indio_dev,
 		*val2 = us % USEC_PER_SEC;
 		return IIO_VAL_INT_PLUS_MICRO;
 
+	case IIO_EV_INFO_HIGH_PASS_FILTER_3DB:
+		ret = i2c_smbus_read_byte_data(data->client,
+						MMA8452_TRANSIENT_CFG);
+		if (ret < 0)
+			return ret;
+
+		if (ret & MMA8452_TRANSIENT_CFG_HPF_BYP) {
+			*val = 0;
+			*val2 = 0;
+		} else {
+			ret = mma8452_read_hp_filter(data, val, val2);
+			if (ret < 0)
+				return ret;
+		}
+		return IIO_VAL_INT_PLUS_MICRO;
+
 	default:
 		return -EINVAL;
 	}
@@ -352,7 +462,7 @@ static int mma8452_write_thresh(struct iio_dev *indio_dev,
 	int val2)
 {
 	struct mma8452_data *data = iio_priv(indio_dev);
-	int steps;
+	int ret, reg, steps;
 
 	switch (info) {
 	case IIO_EV_INFO_VALUE:
@@ -369,6 +479,23 @@ static int mma8452_write_thresh(struct iio_dev *indio_dev,
 
 		return mma8452_change_config(data,
 				MMA8452_TRANSIENT_COUNT, steps);
+
+	case IIO_EV_INFO_HIGH_PASS_FILTER_3DB:
+		reg = i2c_smbus_read_byte_data(data->client,
+						MMA8452_TRANSIENT_CFG);
+		if (reg < 0)
+			return reg;
+
+		if (val == 0 && val2 == 0) {
+			reg |= MMA8452_TRANSIENT_CFG_HPF_BYP;
+		} else {
+			reg &= ~MMA8452_TRANSIENT_CFG_HPF_BYP;
+			ret = mma8452_set_hp_filter_frequency(data, val, val2);
+			if (ret < 0)
+				return ret;
+		}
+		return mma8452_change_config(data, MMA8452_TRANSIENT_CFG, reg);
+
 	default:
 		return -EINVAL;
 	}
@@ -538,7 +665,8 @@ static struct attribute_group mma8452_event_attribute_group = {
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
 		BIT(IIO_CHAN_INFO_CALIBBIAS), \
 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
-		BIT(IIO_CHAN_INFO_SCALE), \
+		BIT(IIO_CHAN_INFO_SCALE) | \
+		BIT(IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY), \
 	.scan_index = idx, \
 	.scan_type = { \
 		.sign = 's', \
@@ -561,6 +689,7 @@ static const struct iio_chan_spec mma8452_channels[] = {
 static struct attribute *mma8452_attributes[] = {
 	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
 	&iio_dev_attr_in_accel_scale_available.dev_attr.attr,
+	&iio_dev_attr_in_accel_filter_high_pass_3db_frequency_available.dev_attr.attr,
 	NULL
 };
 

Re: [PATCH 7/9] iio: mma8452: Add highpass filter configuration.

[Peter Meerwald]

> Allow the cutoff frequency of the high pass filter to be configured.

looks good, probably a few newlines would make it look even nicer (see 
below)
 
> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
> ---
>  drivers/iio/accel/mma8452.c |  135 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 132 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index b7f13dd..fe17c42 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -29,9 +29,12 @@
>  #define MMA8452_INT_SRC 0x0c
>  #define MMA8452_WHO_AM_I 0x0d
>  #define MMA8452_DATA_CFG 0x0e
> +#define MMA8452_HP_FILTER_CUTOFF 0x0f
> +#define MMA8452_HP_FILTER_CUTOFF_SEL_MASK	(BIT(0) | BIT(1))
>  #define MMA8452_TRANSIENT_CFG 0x1d
>  #define MMA8452_TRANSIENT_CFG_ELE		BIT(4)
>  #define MMA8452_TRANSIENT_CFG_CHAN_MASK(chan)	(BIT(1) << chan)
> +#define MMA8452_TRANSIENT_CFG_HPF_BYP		BIT(0)
>  #define MMA8452_TRANSIENT_SRC 0x1e
>  #define MMA8452_TRANSIENT_SRC_XTRANSE		BIT(1)
>  #define MMA8452_TRANSIENT_SRC_YTRANSE		BIT(3)
> @@ -60,6 +63,7 @@
>  #define MMA8452_DATA_CFG_FS_2G 0
>  #define MMA8452_DATA_CFG_FS_4G 1
>  #define MMA8452_DATA_CFG_FS_8G 2
> +#define MMA8452_DATA_CFG_HPF_MASK BIT(4)
>  
>  #define MMA8452_INT_DRDY	BIT(0)
>  #define MMA8452_INT_FF_MT	BIT(2)
> @@ -162,6 +166,18 @@ static const int mma8452_transient_time_step_us[8] = {
>  	20000
>  };
>  
> +/* Datasheet table 18 (normal mode) */
> +static const int mma8452_hp_filter_cutoff[8][4][2] = {
> +	{ {16, 0}, {8, 0}, {4, 0}, {2, 0} },		/* 800 Hz sample */
> +	{ {16, 0}, {8, 0}, {4, 0}, {2, 0} },		/* 400 Hz sample */
> +	{ {8, 0}, {4, 0}, {2, 0}, {1, 0} },		/* 200 Hz sample */
> +	{ {4, 0}, {2, 0}, {1, 0}, {0, 500000} },	/* 100 Hz sample */
> +	{ {2, 0}, {1, 0}, {0, 500000}, {0, 250000} },	/* 50 Hz sample */
> +	{ {2, 0}, {1, 0}, {0, 500000}, {0, 250000} },	/* 12.5 Hz sample */
> +	{ {2, 0}, {1, 0}, {0, 500000}, {0, 250000} },	/* 6.25 Hz sample */
> +	{ {2, 0}, {1, 0}, {0, 500000}, {0, 250000} }	/* 1.56 Hz sample */
> +};
> +
>  static ssize_t mma8452_show_samp_freq_avail(struct device *dev,
>  				struct device_attribute *attr, char *buf)
>  {
> @@ -176,9 +192,22 @@ static ssize_t mma8452_show_scale_avail(struct device *dev,
>  		ARRAY_SIZE(mma8452_scales));
>  }
>  
> +static ssize_t mma8452_show_hp_cutoff_avail(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +	int i = mma8452_get_odr_index(data);
> +
> +	return mma8452_show_int_plus_micros(buf, mma8452_hp_filter_cutoff[i],
> +		ARRAY_SIZE(mma8452_hp_filter_cutoff[0]));
> +}
> +
>  static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(mma8452_show_samp_freq_avail);
>  static IIO_DEVICE_ATTR(in_accel_scale_available, S_IRUGO,
>  	mma8452_show_scale_avail, NULL, 0);
> +static IIO_DEVICE_ATTR(in_accel_filter_high_pass_3db_frequency_available,
> +			S_IRUGO, mma8452_show_hp_cutoff_avail, NULL, 0);
>  
>  static int mma8452_get_samp_freq_index(struct mma8452_data *data,
>  	int val, int val2)
> @@ -194,6 +223,30 @@ static int mma8452_get_scale_index(struct mma8452_data *data,
>  		ARRAY_SIZE(mma8452_scales), val, val2);
>  }
>  
> +static int mma8452_get_hp_filter_index(struct mma8452_data *data,
> +	int val, int val2)
> +{
> +	int i = mma8452_get_odr_index(data);
> +
> +	return mma8452_get_int_plus_micros_index(mma8452_hp_filter_cutoff[i],
> +		ARRAY_SIZE(mma8452_scales[0]), val, val2);
> +}
> +
> +static int mma8452_read_hp_filter(struct mma8452_data *data, int *hz, int *uHz)
> +{
> +	int i, ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, MMA8452_HP_FILTER_CUTOFF);
> +	if (ret < 0)
> +		return ret;
> +
> +	i = mma8452_get_odr_index(data);
> +	ret &= MMA8452_HP_FILTER_CUTOFF_SEL_MASK;
> +	*hz = mma8452_hp_filter_cutoff[i][ret][0];
> +	*uHz = mma8452_hp_filter_cutoff[i][ret][1];

newline

> +	return 0;
> +}
> +
>  static int mma8452_read_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan,
>  			    int *val, int *val2, long mask)
> @@ -232,6 +285,16 @@ static int mma8452_read_raw(struct iio_dev *indio_dev,
>  			return ret;
>  		*val = sign_extend32(ret, 7);
>  		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
> +		if (data->data_cfg & MMA8452_DATA_CFG_HPF_MASK) {
> +			ret = mma8452_read_hp_filter(data, val, val2);
> +			if (ret < 0)
> +				return ret;
> +		} else {
> +			*val = 0;
> +			*val2 = 0;
> +		}
> +		return IIO_VAL_INT_PLUS_MICRO;
>  	}
>  	return -EINVAL;
>  }
> @@ -273,12 +336,30 @@ fail:
>  	return ret;
>  }
>  
> +static int mma8452_set_hp_filter_frequency(struct mma8452_data *data,
> +						int val, int val2)
> +{
> +	int i, reg;
> +
> +	i = mma8452_get_hp_filter_index(data, val, val2);
> +	if (i < 0)
> +		return -EINVAL;
> +
> +	reg = i2c_smbus_read_byte_data(data->client,
> +					MMA8452_HP_FILTER_CUTOFF);
> +	if (reg < 0)
> +		return reg;
> +	reg &= ~MMA8452_HP_FILTER_CUTOFF_SEL_MASK;
> +	reg |= i;

newline

> +	return mma8452_change_config(data, MMA8452_HP_FILTER_CUTOFF, reg);
> +}
> +
>  static int mma8452_write_raw(struct iio_dev *indio_dev,
>  			     struct iio_chan_spec const *chan,
>  			     int val, int val2, long mask)
>  {
>  	struct mma8452_data *data = iio_priv(indio_dev);
> -	int i;
> +	int i, ret;
>  
>  	if (iio_buffer_enabled(indio_dev))
>  		return -EBUSY;
> @@ -306,6 +387,19 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
>  			return -EINVAL;
>  		return mma8452_change_config(data, MMA8452_OFF_X +
>  			chan->scan_index, val);
> +
> +	case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
> +		if (val == 0 && val2 == 0) {
> +			data->data_cfg &= ~MMA8452_DATA_CFG_HPF_MASK;
> +		} else {
> +			data->data_cfg |= MMA8452_DATA_CFG_HPF_MASK;
> +			ret = mma8452_set_hp_filter_frequency(data, val, val2);
> +			if (ret < 0)
> +				return ret;
> +		}
> +		return mma8452_change_config(data, MMA8452_DATA_CFG,
> +						data->data_cfg);
> +
>  	default:
>  		return -EINVAL;
>  	}
> @@ -341,6 +435,22 @@ static int mma8452_read_thresh(struct iio_dev *indio_dev,
>  		*val2 = us % USEC_PER_SEC;
>  		return IIO_VAL_INT_PLUS_MICRO;
>  
> +	case IIO_EV_INFO_HIGH_PASS_FILTER_3DB:
> +		ret = i2c_smbus_read_byte_data(data->client,
> +						MMA8452_TRANSIENT_CFG);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (ret & MMA8452_TRANSIENT_CFG_HPF_BYP) {
> +			*val = 0;
> +			*val2 = 0;
> +		} else {
> +			ret = mma8452_read_hp_filter(data, val, val2);
> +			if (ret < 0)
> +				return ret;
> +		}
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
>  	default:
>  		return -EINVAL;
>  	}
> @@ -352,7 +462,7 @@ static int mma8452_write_thresh(struct iio_dev *indio_dev,
>  	int val2)
>  {
>  	struct mma8452_data *data = iio_priv(indio_dev);
> -	int steps;
> +	int ret, reg, steps;
>  
>  	switch (info) {
>  	case IIO_EV_INFO_VALUE:
> @@ -369,6 +479,23 @@ static int mma8452_write_thresh(struct iio_dev *indio_dev,
>  
>  		return mma8452_change_config(data,
>  				MMA8452_TRANSIENT_COUNT, steps);
> +
> +	case IIO_EV_INFO_HIGH_PASS_FILTER_3DB:
> +		reg = i2c_smbus_read_byte_data(data->client,
> +						MMA8452_TRANSIENT_CFG);
> +		if (reg < 0)
> +			return reg;
> +
> +		if (val == 0 && val2 == 0) {
> +			reg |= MMA8452_TRANSIENT_CFG_HPF_BYP;
> +		} else {
> +			reg &= ~MMA8452_TRANSIENT_CFG_HPF_BYP;
> +			ret = mma8452_set_hp_filter_frequency(data, val, val2);
> +			if (ret < 0)
> +				return ret;
> +		}
> +		return mma8452_change_config(data, MMA8452_TRANSIENT_CFG, reg);
> +
>  	default:
>  		return -EINVAL;
>  	}
> @@ -538,7 +665,8 @@ static struct attribute_group mma8452_event_attribute_group = {
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
>  		BIT(IIO_CHAN_INFO_CALIBBIAS), \
>  	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> -		BIT(IIO_CHAN_INFO_SCALE), \
> +		BIT(IIO_CHAN_INFO_SCALE) | \
> +		BIT(IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY), \
>  	.scan_index = idx, \
>  	.scan_type = { \
>  		.sign = 's', \
> @@ -561,6 +689,7 @@ static const struct iio_chan_spec mma8452_channels[] = {
>  static struct attribute *mma8452_attributes[] = {
>  	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
>  	&iio_dev_attr_in_accel_scale_available.dev_attr.attr,
> +	&iio_dev_attr_in_accel_filter_high_pass_3db_frequency_available.dev_attr.attr,
>  	NULL
>  };
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)

[PATCH 8/9] iio: mma8452: Add support for interrupt driven triggers.

[Martin Fuzzey]

Implement interrupt driven trigger for data ready.
This allows more efficient access to the sample data.

Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
---
 drivers/iio/accel/mma8452.c |   84 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 78 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index fe17c42..6277b5d 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -18,6 +18,8 @@
 #include <linux/iio/sysfs.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/buffer.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
 #include <linux/iio/triggered_buffer.h>
 #include <linux/iio/events.h>
 #include <linux/delay.h>
@@ -575,18 +577,24 @@ static irqreturn_t mma8452_interrupt(int irq, void *p)
 {
 	struct iio_dev *indio_dev = p;
 	struct mma8452_data *data = iio_priv(indio_dev);
+	int ret = IRQ_NONE;
 	int src;
 
 	src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
 	if (src < 0)
 		return IRQ_NONE;
 
+	if (src & MMA8452_INT_DRDY) {
+		iio_trigger_poll_chained(indio_dev->trig);
+		ret = IRQ_HANDLED;
+	}
+
 	if (src & MMA8452_INT_TRANS) {
 		mma8452_transient_interrupt(indio_dev);
-		return IRQ_HANDLED;
+		ret = IRQ_HANDLED;
 	}
 
-	return IRQ_NONE;
+	return ret;
 }
 
 static irqreturn_t mma8452_trigger_handler(int irq, void *p)
@@ -712,6 +720,60 @@ static const struct iio_info mma8452_info = {
 
 static const unsigned long mma8452_scan_masks[] = {0x7, 0};
 
+static int mma8452_data_rdy_trigger_set_state(struct iio_trigger *trig,
+		bool state)
+{
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+	struct mma8452_data *data = iio_priv(indio_dev);
+	int reg;
+
+	reg = i2c_smbus_read_byte_data(data->client, MMA8452_CTRL_REG4);
+	if (reg < 0)
+		return reg;
+
+	if (state)
+		reg |= MMA8452_INT_DRDY;
+	else
+		reg &= ~MMA8452_INT_DRDY;
+
+	return mma8452_change_config(data, MMA8452_CTRL_REG4, reg);
+}
+
+static const struct iio_trigger_ops mma8452_trigger_ops = {
+	.set_trigger_state = mma8452_data_rdy_trigger_set_state,
+	.owner = THIS_MODULE,
+};
+
+static int mma8452_trigger_setup(struct iio_dev *indio_dev)
+{
+	struct mma8452_data *data = iio_priv(indio_dev);
+	struct iio_trigger *trig;
+	int ret;
+
+	trig = devm_iio_trigger_alloc(&data->client->dev, "%s-dev%d",
+					indio_dev->name,
+					indio_dev->id);
+	if (!trig)
+		return -ENOMEM;
+
+	trig->dev.parent = &data->client->dev;
+	trig->ops = &mma8452_trigger_ops;
+	iio_trigger_set_drvdata(trig, indio_dev);
+
+	ret = iio_trigger_register(trig);
+	if (ret)
+		return ret;
+
+	indio_dev->trig = trig;
+	return 0;
+}
+
+static void mma8452_trigger_cleanup(struct iio_dev *indio_dev)
+{
+	if (indio_dev->trig)
+		iio_trigger_unregister(indio_dev->trig);
+}
+
 static int mma8452_reset(struct i2c_client *client)
 {
 	int i;
@@ -791,7 +853,8 @@ static int mma8452_probe(struct i2c_client *client,
 		 * enabled until userspace asks for it by
 		 * mma8452_write_event_config()
 		 */
-		int supported_interrupts = MMA8452_INT_TRANS;
+		int supported_interrupts = MMA8452_INT_DRDY | MMA8452_INT_TRANS;
+		int enabled_interrupts = MMA8452_INT_TRANS;
 
 		/* Assume wired to INT1 pin */
 		ret = i2c_smbus_write_byte_data(client,
@@ -802,7 +865,11 @@ static int mma8452_probe(struct i2c_client *client,
 
 		ret = i2c_smbus_write_byte_data(client,
 						MMA8452_CTRL_REG4,
-						supported_interrupts);
+						enabled_interrupts);
+		if (ret < 0)
+			return ret;
+
+		ret = mma8452_trigger_setup(indio_dev);
 		if (ret < 0)
 			return ret;
 	}
@@ -812,12 +879,12 @@ static int mma8452_probe(struct i2c_client *client,
 	ret = i2c_smbus_write_byte_data(client, MMA8452_CTRL_REG1,
 		data->ctrl_reg1);
 	if (ret < 0)
-		return ret;
+		goto trigger_cleanup;
 
 	ret = iio_triggered_buffer_setup(indio_dev, NULL,
 		mma8452_trigger_handler, NULL);
 	if (ret < 0)
-		return ret;
+		goto trigger_cleanup;
 
 	if (client->irq) {
 		ret = devm_request_threaded_irq(&client->dev,
@@ -837,6 +904,10 @@ static int mma8452_probe(struct i2c_client *client,
 
 buffer_cleanup:
 	iio_triggered_buffer_cleanup(indio_dev);
+
+trigger_cleanup:
+	mma8452_trigger_cleanup(indio_dev);
+
 	return ret;
 }
 
@@ -846,6 +917,7 @@ static int mma8452_remove(struct i2c_client *client)
 
 	iio_device_unregister(indio_dev);
 	iio_triggered_buffer_cleanup(indio_dev);
+	mma8452_trigger_cleanup(indio_dev);
 	mma8452_standby(iio_priv(indio_dev));
 
 	return 0;

[PATCH 9/9] iio: mma8452: add support for self test.

[Martin Fuzzey]

Add a new attribute to activate the self test mode.

When self test is activated, an electrostatic actuation force is
applied to the sensor, simulating a small acceleration.

Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
---
 .../ABI/testing/sysfs-bus-iio-accel-mma8452        |   10 ++++
 drivers/iio/accel/mma8452.c                        |   52 ++++++++++++++++++++
 2 files changed, 62 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-accel-mma8452

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-accel-mma8452 b/Documentation/ABI/testing/sysfs-bus-iio-accel-mma8452
new file mode 100644
index 0000000..387ed38
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-accel-mma8452
@@ -0,0 +1,10 @@
+What:		/sys/bus/iio/devices/iio:deviceX/self_test
+KernelVersion:	3.21
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Allow self test mode to be controlled:
+			"0" = inactive
+			"1" = active
+		When self test is activated, an electrostatic actuation force is
+		applied to the sensor, simulating a small acceleration.
+
diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 6277b5d..75dcc6e 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -49,6 +49,7 @@
 #define MMA8452_CTRL_REG1 0x2a
 #define MMA8452_CTRL_REG2 0x2b
 #define MMA8452_CTRL_REG2_RST		BIT(6)
+#define MMA8452_CTRL_REG2_SELFTEST	BIT(7)
 #define MMA8452_CTRL_REG4 0x2d
 #define MMA8452_CTRL_REG5 0x2e
 
@@ -639,6 +640,56 @@ static int mma8452_reg_access_dbg(struct iio_dev *indio_dev,
 	return ret;
 }
 
+static ssize_t mma8452_show_selftest(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct mma8452_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(data->client, MMA8452_CTRL_REG2);
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "%d\n", ret & MMA8452_CTRL_REG2_SELFTEST ? 1 : 0);
+}
+
+static ssize_t mma8452_store_selftest(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct mma8452_data *data = iio_priv(indio_dev);
+	bool state;
+	int ret;
+	u8 val;
+
+	ret = strtobool(buf, &state);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_smbus_read_byte_data(data->client, MMA8452_CTRL_REG2);
+	if (ret < 0)
+		return ret;
+
+	val = ret;
+	if (state)
+		val |= MMA8452_CTRL_REG2_SELFTEST;
+	else
+		val &= ~MMA8452_CTRL_REG2_SELFTEST;
+
+	ret = mma8452_change_config(data, MMA8452_CTRL_REG2, val);
+	if (ret)
+		return ret;
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR(self_test,
+			 S_IRUGO | S_IWUSR,
+			 mma8452_show_selftest,
+			 mma8452_store_selftest, 0);
+
 static const struct iio_event_spec mma8452_transient_event[] = {
 	{
 		.type = IIO_EV_TYPE_THRESH,
@@ -698,6 +749,7 @@ static struct attribute *mma8452_attributes[] = {
 	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
 	&iio_dev_attr_in_accel_scale_available.dev_attr.attr,
 	&iio_dev_attr_in_accel_filter_high_pass_3db_frequency_available.dev_attr.attr,
+	&iio_dev_attr_self_test.dev_attr.attr,
 	NULL
 };
 

Re: [PATCH 9/9] iio: mma8452: add support for self test.

[Peter Meerwald]

On Thu, 19 Feb 2015, Martin Fuzzey wrote:

> Add a new attribute to activate the self test mode.
> 
> When self test is activated, an electrostatic actuation force is
> applied to the sensor, simulating a small acceleration.

comment below

> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
> ---
>  .../ABI/testing/sysfs-bus-iio-accel-mma8452        |   10 ++++
>  drivers/iio/accel/mma8452.c                        |   52 ++++++++++++++++++++
>  2 files changed, 62 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-accel-mma8452
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-accel-mma8452 b/Documentation/ABI/testing/sysfs-bus-iio-accel-mma8452
> new file mode 100644
> index 0000000..387ed38
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-accel-mma8452
> @@ -0,0 +1,10 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/self_test
> +KernelVersion:	3.21

bad timing :)

> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Allow self test mode to be controlled:
> +			"0" = inactive
> +			"1" = active
> +		When self test is activated, an electrostatic actuation force is
> +		applied to the sensor, simulating a small acceleration.
> +
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 6277b5d..75dcc6e 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -49,6 +49,7 @@
>  #define MMA8452_CTRL_REG1 0x2a
>  #define MMA8452_CTRL_REG2 0x2b
>  #define MMA8452_CTRL_REG2_RST		BIT(6)
> +#define MMA8452_CTRL_REG2_SELFTEST	BIT(7)
>  #define MMA8452_CTRL_REG4 0x2d
>  #define MMA8452_CTRL_REG5 0x2e
>  
> @@ -639,6 +640,56 @@ static int mma8452_reg_access_dbg(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> +static ssize_t mma8452_show_selftest(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, MMA8452_CTRL_REG2);
> +	if (ret < 0)
> +		return ret;
> +
> +	return sprintf(buf, "%d\n", ret & MMA8452_CTRL_REG2_SELFTEST ? 1 : 0);
> +}
> +
> +static ssize_t mma8452_store_selftest(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +	bool state;
> +	int ret;
> +	u8 val;
> +
> +	ret = strtobool(buf, &state);
> +	if (ret < 0)
> +		return ret;
> +

take mutex?

> +	ret = i2c_smbus_read_byte_data(data->client, MMA8452_CTRL_REG2);
> +	if (ret < 0)
> +		return ret;
> +
> +	val = ret;
> +	if (state)
> +		val |= MMA8452_CTRL_REG2_SELFTEST;
> +	else
> +		val &= ~MMA8452_CTRL_REG2_SELFTEST;
> +
> +	ret = mma8452_change_config(data, MMA8452_CTRL_REG2, val);
> +	if (ret)
> +		return ret;
> +
> +	return len;
> +}
> +
> +static IIO_DEVICE_ATTR(self_test,
> +			 S_IRUGO | S_IWUSR,
> +			 mma8452_show_selftest,
> +			 mma8452_store_selftest, 0);
> +
>  static const struct iio_event_spec mma8452_transient_event[] = {
>  	{
>  		.type = IIO_EV_TYPE_THRESH,
> @@ -698,6 +749,7 @@ static struct attribute *mma8452_attributes[] = {
>  	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
>  	&iio_dev_attr_in_accel_scale_available.dev_attr.attr,
>  	&iio_dev_attr_in_accel_filter_high_pass_3db_frequency_available.dev_attr.attr,
> +	&iio_dev_attr_self_test.dev_attr.attr,
>  	NULL
>  };
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: [PATCH 9/9] iio: mma8452: add support for self test.

[Jonathan Cameron]

On 19/02/15 14:16, Martin Fuzzey wrote:
> Add a new attribute to activate the self test mode.
> 
> When self test is activated, an electrostatic actuation force is
> applied to the sensor, simulating a small acceleration.
> 
> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
The most common option for self tests is to do them on startup
and spit out an error if they fail to give the expected
result.  The advantage is that we avoid a little used and
not terribly well defined (in general!) ABI element.
This is far from the only part supporting self tests
so we need to be careful with adding a new ABI.

Would doing it on startup work here?
Clearly you'd have to apply the self test and define some boundaries
on the expected result for correct operation (vs not having it on)
and hope no one starts the device up whilst accelerating a lot.

I guess on this one a warning would be the way to go rather than
an error as huge real accelerations are more than possible.

Jonathan
> ---
>  .../ABI/testing/sysfs-bus-iio-accel-mma8452        |   10 ++++
>  drivers/iio/accel/mma8452.c                        |   52 ++++++++++++++++++++
>  2 files changed, 62 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-accel-mma8452
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-accel-mma8452 b/Documentation/ABI/testing/sysfs-bus-iio-accel-mma8452
> new file mode 100644
> index 0000000..387ed38
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-accel-mma8452
> @@ -0,0 +1,10 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/self_test
> +KernelVersion:	3.21
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Allow self test mode to be controlled:
> +			"0" = inactive
> +			"1" = active
> +		When self test is activated, an electrostatic actuation force is
> +		applied to the sensor, simulating a small acceleration.
> +
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 6277b5d..75dcc6e 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -49,6 +49,7 @@
>  #define MMA8452_CTRL_REG1 0x2a
>  #define MMA8452_CTRL_REG2 0x2b
>  #define MMA8452_CTRL_REG2_RST		BIT(6)
> +#define MMA8452_CTRL_REG2_SELFTEST	BIT(7)
>  #define MMA8452_CTRL_REG4 0x2d
>  #define MMA8452_CTRL_REG5 0x2e
>  
> @@ -639,6 +640,56 @@ static int mma8452_reg_access_dbg(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> +static ssize_t mma8452_show_selftest(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, MMA8452_CTRL_REG2);
> +	if (ret < 0)
> +		return ret;
> +
> +	return sprintf(buf, "%d\n", ret & MMA8452_CTRL_REG2_SELFTEST ? 1 : 0);
> +}
> +
> +static ssize_t mma8452_store_selftest(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct mma8452_data *data = iio_priv(indio_dev);
> +	bool state;
> +	int ret;
> +	u8 val;
> +
> +	ret = strtobool(buf, &state);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, MMA8452_CTRL_REG2);
> +	if (ret < 0)
> +		return ret;
> +
> +	val = ret;
> +	if (state)
> +		val |= MMA8452_CTRL_REG2_SELFTEST;
> +	else
> +		val &= ~MMA8452_CTRL_REG2_SELFTEST;
> +
> +	ret = mma8452_change_config(data, MMA8452_CTRL_REG2, val);
> +	if (ret)
> +		return ret;
> +
> +	return len;
> +}
> +
> +static IIO_DEVICE_ATTR(self_test,
> +			 S_IRUGO | S_IWUSR,
> +			 mma8452_show_selftest,
> +			 mma8452_store_selftest, 0);
> +
>  static const struct iio_event_spec mma8452_transient_event[] = {
>  	{
>  		.type = IIO_EV_TYPE_THRESH,
> @@ -698,6 +749,7 @@ static struct attribute *mma8452_attributes[] = {
>  	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
>  	&iio_dev_attr_in_accel_scale_available.dev_attr.attr,
>  	&iio_dev_attr_in_accel_filter_high_pass_3db_frequency_available.dev_attr.attr,
> +	&iio_dev_attr_self_test.dev_attr.attr,
>  	NULL
>  };
>  
> 

Re: [PATCH 9/9] iio: mma8452: add support for self test.

[Martin Fuzzey]

On 09/03/15 14:37, Jonathan Cameron wrote:
> On 19/02/15 14:16, Martin Fuzzey wrote:
>> Add a new attribute to activate the self test mode.
>>
>> When self test is activated, an electrostatic actuation force is
>> applied to the sensor, simulating a small acceleration.
>>
>> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
> The most common option for self tests is to do them on startup
> and spit out an error if they fail to give the expected
> result.  The advantage is that we avoid a little used and
> not terribly well defined (in general!) ABI element.
> This is far from the only part supporting self tests
> so we need to be careful with adding a new ABI.
>
> Would doing it on startup work here?
> Clearly you'd have to apply the self test and define some boundaries
> on the expected result for correct operation (vs not having it on)
> and hope no one starts the device up whilst accelerating a lot.
>
> I guess on this one a warning would be the way to go rather than
> an error as huge real accelerations are more than possible.

The problem with doing it on startup is there is no way for userspace to 
get the result, except by parsing the kernel logs (yuk).
And it would more or less preclude implementing a "maintenance mode" 
test menu.

So, at least for my usecase, doing it *only* on startup wouldn't work (a 
case could be made for having it done on startup *and* on explicit request).

Note that I'm not adding a new *generic* ABI here, but a device specific 
one (there are other devices that have specific attributes too).
I agree that a generic ABI would be better but is it possible to define 
one that would suite all devices?
I don't know enough about the other devices to answer that question.

I'm also not doing a complete test giving a "GO / NO GO" result but 
rather just providing a means of switching the actuator on and letting 
userspace observe the difference via the standard ABI (which avoids the 
problems with defining thresholds etc - or at least pushes them to 
userspace).

But, if we don't (yet?), have a consensus for a self test ABI I'll drop 
this patch for the moment and keep it in my tree for now.

Regards,

Martin

Re: [PATCH 9/9] iio: mma8452: add support for self test.

[Jonathan Cameron]

On 29/04/15 09:05, Martin Fuzzey wrote:
> On 09/03/15 14:37, Jonathan Cameron wrote:
>> On 19/02/15 14:16, Martin Fuzzey wrote:
>>> Add a new attribute to activate the self test mode.
>>>
>>> When self test is activated, an electrostatic actuation force is
>>> applied to the sensor, simulating a small acceleration.
>>>
>>> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
>> The most common option for self tests is to do them on startup
>> and spit out an error if they fail to give the expected
>> result.  The advantage is that we avoid a little used and
>> not terribly well defined (in general!) ABI element.
>> This is far from the only part supporting self tests
>> so we need to be careful with adding a new ABI.
>>
>> Would doing it on startup work here?
>> Clearly you'd have to apply the self test and define some boundaries
>> on the expected result for correct operation (vs not having it on)
>> and hope no one starts the device up whilst accelerating a lot.
>>
>> I guess on this one a warning would be the way to go rather than
>> an error as huge real accelerations are more than possible.
> 
>The problem with doing it on startup is there is no way for userspace to get the result, except by parsing the kernel logs (yuk).
Agreed.  The ability to report hardware failures in a more coherent
way is something a lot of people have been asking for for ages, but
I don't think anyone has yet come up with a better option.
> And it would more or less preclude implementing a "maintenance mode"
> test menu.
You could remove and reprobe the device, but that would be hideous!
Seems like a good usecase for lots of devices.
> 
> So, at least for my usecase, doing it *only* on startup wouldn't work
> (a case could be made for having it done on startup *and* on explicit
> request).
The both option sounds sensible.
> 
> Note that I'm not adding a new *generic* ABI here, but a device
> specific one (there are other devices that have specific attributes
> too). I agree that a generic ABI would be better but is it possible
> to define one that would suite all devices? I don't know enough about
> the other devices to answer that question.
Hmm.. Interesting question. Mostly device specific ABIs exist for the
really unusual elements (and often they end up becoming common in the
future!).
> 
> I'm also not doing a complete test giving a "GO / NO GO" result but
> rather just providing a means of switching the actuator on and
> letting userspace observe the difference via the standard ABI (which
> avoids the problems with defining thresholds etc - or at least pushes
> them to userspace).
I wonder how generic an ABI we can come up with. 
Ultimately you tend to have some 'magic setting' that leads to a known
change in the value read.  There might be more than one of these, but
that's about it.  Doesn't sound implausible to have a generic ABI for this
to me...

Lars, quite a few Analog parts have self tests.  What do you think of
such an interface?
> 
> But, if we don't (yet?), have a consensus for a self test ABI I'll
> drop this patch for the moment and keep it in my tree for now.

You are convincing me that this is a lot more interesting than it
seems at first glance :)
> 
> Regards,
> 
> Martin

Re: [PATCH V3 0/9] iio: mma8452 enhancements

[Peter Meerwald]

> This series adds some additional features to the mma8452 accelerometer driver
> 	* debugfs register access
> 	* transient threshold events
> 	* highpass filter
> 	* interrupt driven sampling (trigger)
> 	* self test mode

looks good except some very minor things;
great to see the code is used and enhanced
 
> Attributes are added to the core for:
> * highpass filter for readings
> * both high and lowpass filters for events
> 
> In addition a latent bug in the device initialisation is fixed.
> 
> V3 changes:
> 	Use 3db frequency for filter enable / disable (0=disable)
> 	Support enabling filter for readings in addition to events
> 	Use IIO_EV_INFO_PERIOD rather than custom attribute
> 	Use devm_iio_trigger_alloc()
> 	Add comment to explain irq activation during probe()
> 	iio_device_register() only after all setup
> 	Drop iio_push_event() wrapper
> 	Correct parameters to iio_push_event()
> 	Document missing attributes
> 	Add self test mode
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: [PATCH V3 0/9] iio: mma8452 enhancements

[Jonathan Cameron]

On 25/02/15 23:11, Peter Meerwald wrote:
> 
>> This series adds some additional features to the mma8452 accelerometer driver
>> 	* debugfs register access
>> 	* transient threshold events
>> 	* highpass filter
>> 	* interrupt driven sampling (trigger)
>> 	* self test mode
> 
> looks good except some very minor things;
> great to see the code is used and enhanced
I'm happy with all the patches I didn't comment on.

Looks good Martin, not much to tidy up!
>  
>> Attributes are added to the core for:
>> * highpass filter for readings
>> * both high and lowpass filters for events
>>
>> In addition a latent bug in the device initialisation is fixed.
>>
>> V3 changes:
>> 	Use 3db frequency for filter enable / disable (0=disable)
>> 	Support enabling filter for readings in addition to events
>> 	Use IIO_EV_INFO_PERIOD rather than custom attribute
>> 	Use devm_iio_trigger_alloc()
>> 	Add comment to explain irq activation during probe()
>> 	iio_device_register() only after all setup
>> 	Drop iio_push_event() wrapper
>> 	Correct parameters to iio_push_event()
>> 	Document missing attributes
>> 	Add self test mode
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>