[PATCH V2 0/8] iio: mma8452 enhancements

[PATCH V2 0/8] 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)

The highpass filter attributes are added to the core.

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

V2: Address review comments from Peter Meerwald

[PATCH V2 1/8] 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)

[PATCH V2 2/8] 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,
 };
 

[PATCH V2 3/8] 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 |  203 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 202 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index c46df4e..3415b36 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,117 @@ 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_push_transient_event(struct iio_dev *indio_dev,
+						int chan, u64 ts)
+{
+	iio_push_event(indio_dev,
+		IIO_UNMOD_EVENT_CODE(IIO_ACCEL,
+			chan,
+			IIO_EV_TYPE_THRESH,
+			IIO_EV_DIR_RISING),
+		ts);
+}
+
+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)
+		mma8452_push_transient_event(indio_dev, 0, ts);
+
+	if (src & MMA8452_TRANSIENT_SRC_YTRANSE)
+		mma8452_push_transient_event(indio_dev, 1, ts);
+
+	if (src & MMA8452_TRANSIENT_SRC_ZTRANSE)
+		mma8452_push_transient_event(indio_dev, 2, 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 +447,31 @@ 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),
+	},
+};
+
+/**
+ * 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 +488,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 +513,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 +588,31 @@ 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) {
+		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,
@@ -440,8 +628,21 @@ static int mma8452_probe(struct i2c_client *client,
 	ret = iio_device_register(indio_dev);
 	if (ret < 0)
 		goto buffer_cleanup;
+
+	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 device_cleanup;
+	}
 	return 0;
 
+device_cleanup:
+	iio_device_unregister(indio_dev);
+
 buffer_cleanup:
 	iio_triggered_buffer_cleanup(indio_dev);
 	return ret;

[PATCH V2 4/8] 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 |   77 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 75 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 3415b36..1d1e41b 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;
@@ -356,6 +373,54 @@ static int mma8452_write_event_config(struct iio_dev *indio_dev,
 	return mma8452_change_config(data, MMA8452_TRANSIENT_CFG, val);
 }
 
+static ssize_t mma8452_query_transient_period(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 us, steps;
+
+	steps = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_COUNT);
+	if (steps < 0)
+		return steps;
+
+	us = mma8452_transient_time_step_us[mma8452_get_odr_index(data)]
+								* steps;
+
+	return sprintf(buf, "%ld.%06ld\n",
+			us / USEC_PER_SEC, us % USEC_PER_SEC);
+}
+
+static ssize_t mma8452_set_transient_period(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);
+	int i, f, us, steps, ret;
+
+	ret = iio_str_to_fixpoint(buf, USEC_PER_SEC / 10, &i, &f);
+	if (ret)
+		return ret;
+	if (ret < 0)
+		return -EINVAL;
+
+	us = i * USEC_PER_SEC + f;
+	steps = us / mma8452_transient_time_step_us[
+				mma8452_get_odr_index(data)];
+
+	if (steps > 0xff)
+		return -EINVAL;
+
+	ret = mma8452_change_config(data, MMA8452_TRANSIENT_COUNT, steps);
+	if (ret)
+		return ret;
+
+	return len;
+}
+
 static void mma8452_push_transient_event(struct iio_dev *indio_dev,
 						int chan, u64 ts)
 {
@@ -462,8 +527,16 @@ static const struct iio_event_spec mma8452_transient_event[] = {
  */
 static IIO_CONST_ATTR_NAMED(accel_transient_scale, in_accel_scale, "0.617742");
 
+static IIO_DEVICE_ATTR_NAMED(accel_transient_period,
+			     in_accel_thresh_rising_period,
+			     S_IRUGO | S_IWUSR,
+			     mma8452_query_transient_period,
+			     mma8452_set_transient_period,
+			     0);
+
 static struct attribute *mma8452_event_attributes[] = {
 	&iio_const_attr_accel_transient_scale.dev_attr.attr,
+	&iio_dev_attr_accel_transient_period.dev_attr.attr,
 	NULL,
 };
 

[PATCH V2 5/8] iio: core: add high pass filter attributes

[Martin Fuzzey]

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

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index a9757dc..4825139 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -340,6 +340,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.17
+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
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index fa06197..967468d 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -103,6 +103,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/include/linux/iio/iio.h b/include/linux/iio/iio.h
index ccde917..88a255d 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -31,6 +31,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,

[PATCH V2 6/8] io: 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 |   65 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 63 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 1d1e41b..eb68f9a 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -29,6 +29,8 @@
 #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)
@@ -162,6 +164,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 +190,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 +221,15 @@ 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_raw(struct iio_dev *indio_dev,
 			    struct iio_chan_spec const *chan,
 			    int *val, int *val2, long mask)
@@ -232,6 +268,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:
+		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;
+		*val = mma8452_hp_filter_cutoff[i][ret][0];
+		*val2 = mma8452_hp_filter_cutoff[i][ret][1];
+		return IIO_VAL_INT_PLUS_MICRO;
 	}
 	return -EINVAL;
 }
@@ -278,7 +324,7 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
 			     int val, int val2, long mask)
 {
 	struct mma8452_data *data = iio_priv(indio_dev);
-	int i;
+	int i, reg;
 
 	if (iio_buffer_enabled(indio_dev))
 		return -EBUSY;
@@ -306,6 +352,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:
+		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);
 	default:
 		return -EINVAL;
 	}
@@ -552,7 +611,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', \
@@ -575,6 +635,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
 };
 

[PATCH V2 7/8] iio: mma8452: add an attribute to enable the highpass filter

[Martin Fuzzey]

The hardware contains a single configurable highpass filter which
is normally used for transient detection (event).

However it is also possible to enable this filter for normal channel
reading. Add a new attribute in_accel_high_pass_filter_en to do this.

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

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index eb68f9a..62589f9 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -62,6 +62,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)
@@ -106,6 +107,43 @@ static int mma8452_read(struct mma8452_data *data, __be16 buf[3])
 		MMA8452_OUT_X, 3 * sizeof(__be16), (u8 *) buf);
 }
 
+static int mma8452_standby(struct mma8452_data *data)
+{
+	return i2c_smbus_write_byte_data(data->client, MMA8452_CTRL_REG1,
+		data->ctrl_reg1 & ~MMA8452_CTRL_ACTIVE);
+}
+
+static int mma8452_active(struct mma8452_data *data)
+{
+	return i2c_smbus_write_byte_data(data->client, MMA8452_CTRL_REG1,
+		data->ctrl_reg1);
+}
+
+static int mma8452_change_config(struct mma8452_data *data, u8 reg, u8 val)
+{
+	int ret;
+
+	mutex_lock(&data->lock);
+
+	/* config can only be changed when in standby */
+	ret = mma8452_standby(data);
+	if (ret < 0)
+		goto fail;
+
+	ret = i2c_smbus_write_byte_data(data->client, reg, val);
+	if (ret < 0)
+		goto fail;
+
+	ret = mma8452_active(data);
+	if (ret < 0)
+		goto fail;
+
+	ret = 0;
+fail:
+	mutex_unlock(&data->lock);
+	return ret;
+}
+
 static ssize_t mma8452_show_int_plus_micros(char *buf,
 	const int (*vals)[2], int n)
 {
@@ -201,11 +239,50 @@ static ssize_t mma8452_show_hp_cutoff_avail(struct device *dev,
 		ARRAY_SIZE(mma8452_hp_filter_cutoff[0]));
 }
 
+static ssize_t mma8452_show_hp_cutoff_en(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);
+
+	return sprintf(buf, "%d\n",
+		data->data_cfg & MMA8452_DATA_CFG_HPF_MASK ? 1 : 0);
+}
+
+static ssize_t mma8452_store_hp_cutoff_en(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;
+
+	ret = strtobool(buf, &state);
+	if (ret < 0)
+		return ret;
+
+	if (state)
+		data->data_cfg |= MMA8452_DATA_CFG_HPF_MASK;
+	else
+		data->data_cfg &= ~MMA8452_DATA_CFG_HPF_MASK;
+
+	ret = mma8452_change_config(data, MMA8452_DATA_CFG, data->data_cfg);
+	if (ret)
+		return ret;
+
+	return len;
+}
+
 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 IIO_DEVICE_ATTR(in_accel_filter_high_pass_en,
+			 S_IRUGO | S_IWUSR,
+			 mma8452_show_hp_cutoff_en,
+			 mma8452_store_hp_cutoff_en, 0);
 
 static int mma8452_get_samp_freq_index(struct mma8452_data *data,
 	int val, int val2)
@@ -282,43 +359,6 @@ static int mma8452_read_raw(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
-static int mma8452_standby(struct mma8452_data *data)
-{
-	return i2c_smbus_write_byte_data(data->client, MMA8452_CTRL_REG1,
-		data->ctrl_reg1 & ~MMA8452_CTRL_ACTIVE);
-}
-
-static int mma8452_active(struct mma8452_data *data)
-{
-	return i2c_smbus_write_byte_data(data->client, MMA8452_CTRL_REG1,
-		data->ctrl_reg1);
-}
-
-static int mma8452_change_config(struct mma8452_data *data, u8 reg, u8 val)
-{
-	int ret;
-
-	mutex_lock(&data->lock);
-
-	/* config can only be changed when in standby */
-	ret = mma8452_standby(data);
-	if (ret < 0)
-		goto fail;
-
-	ret = i2c_smbus_write_byte_data(data->client, reg, val);
-	if (ret < 0)
-		goto fail;
-
-	ret = mma8452_active(data);
-	if (ret < 0)
-		goto fail;
-
-	ret = 0;
-fail:
-	mutex_unlock(&data->lock);
-	return ret;
-}
-
 static int mma8452_write_raw(struct iio_dev *indio_dev,
 			     struct iio_chan_spec const *chan,
 			     int val, int val2, long mask)
@@ -636,6 +676,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_in_accel_filter_high_pass_en.dev_attr.attr,
 	NULL
 };
 

[PATCH V2 8/8] 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 |   91 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 85 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 62589f9..7ccc369 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>
@@ -555,18 +557,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, iio_get_time_ns());
+		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)
@@ -699,6 +707,67 @@ 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 = iio_trigger_alloc("%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)
+		goto err_trigger_free;
+
+	indio_dev->trig = trig;
+
+	return 0;
+
+err_trigger_free:
+	iio_trigger_free(trig);
+
+	return ret;
+}
+
+static void mma8452_trigger_cleanup(struct iio_dev *indio_dev)
+{
+	if (indio_dev->trig) {
+		iio_trigger_unregister(indio_dev->trig);
+		iio_trigger_free(indio_dev->trig);
+	}
+}
+
 static int mma8452_reset(struct i2c_client *client)
 {
 	int i;
@@ -772,7 +841,8 @@ static int mma8452_probe(struct i2c_client *client,
 		return ret;
 
 	if (client->irq) {
-		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,
@@ -783,7 +853,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;
 	}
@@ -793,12 +867,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;
 
 	ret = iio_device_register(indio_dev);
 	if (ret < 0)
@@ -820,6 +894,10 @@ device_cleanup:
 
 buffer_cleanup:
 	iio_triggered_buffer_cleanup(indio_dev);
+
+trigger_cleanup:
+	mma8452_trigger_cleanup(indio_dev);
+
 	return ret;
 }
 
@@ -829,6 +907,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;

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

[Jonathan Cameron]

On 29/07/14 10:01, 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 fine to me but I'd like an OK from Peter for this series.

J
> ---
>  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}
>  };
>
> -/*
> +/*
This ought to be in a separate cleanup patch.  Just adds noise to the important
stuff in here!  Not important though so don't bother rerolling the series for this.

>   * 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
>

Re: [PATCH V2 3/8] iio: mma8452: Basic support for transient events.

[Jonathan Cameron]

On 29/07/14 10:01, Martin Fuzzey wrote:
> The event is triggered when the highpass filtered absolute acceleration
> exceeds the threshold.
>
> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
I am unsure of the best way to handle this high pass filtered threshold.
Suggestions inline.  I'm not happy just throwing it through the
current interface and ignoring the filter...

J
> ---
>  drivers/iio/accel/mma8452.c |  203 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 202 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index c46df4e..3415b36 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,117 @@ 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_push_transient_event(struct iio_dev *indio_dev,
> +						int chan, u64 ts)
> +{
> +	iio_push_event(indio_dev,
> +		IIO_UNMOD_EVENT_CODE(IIO_ACCEL,
> +			chan,
> +			IIO_EV_TYPE_THRESH,
> +			IIO_EV_DIR_RISING),
> +		ts);
> +}
I'd prefer it if you dropped the wrapper.  Makes it more obvious what
is going on below.

Also it should be IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, IIO_MOD_X, IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING).
> +
> +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)
> +		mma8452_push_transient_event(indio_dev, 0, ts);
> +
> +	if (src & MMA8452_TRANSIENT_SRC_YTRANSE)
> +		mma8452_push_transient_event(indio_dev, 1, ts);
> +
> +	if (src & MMA8452_TRANSIENT_SRC_ZTRANSE)
> +		mma8452_push_transient_event(indio_dev, 2, 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 +447,31 @@ static int mma8452_reg_access_dbg(struct iio_dev *indio_dev,
>  	return ret;
>  }
>
> +static const struct iio_event_spec mma8452_transient_event[] = {
> +	{
This is an interesting new event type - or at least it needs some
indication that it is high pass filtered...

It is closer to a rate of change detector than a threshold, but
not quite the same.

We could add an additional IIO_EV_INFO_HIGH_PASS_FILTER_3DB element
to the event to indicate the event is based on high pass filtered data.

Things will get interesting if there is a second low pass filtered
event of otherwise the same type. I guess we'll then just have to have
indexed events (we have hardware that would allow these, but it has never
made much sense to enable it).

> +		.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),
> +	},
> +};
> +
> +/**
> + * 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,
This needs documenting under Documentation/ABI/testing/
> +	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 +488,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 +513,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 +588,31 @@ 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
> +	 */
Why are we enabling it in the probe?  Would normally expect events to
only be enabled from userspace once the device is up and running...

> +	ret = i2c_smbus_write_byte_data(client, MMA8452_TRANSIENT_THS, 0x7f);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (client->irq) {
> +		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,
> @@ -440,8 +628,21 @@ static int mma8452_probe(struct i2c_client *client,
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0)
>  		goto buffer_cleanup;
We'd normally do this in the other order.  Hence have everything up
and running before we expose the userspace interfaces in iio_device_register.
> +
> +	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 device_cleanup;
> +	}
>  	return 0;
>
> +device_cleanup:
> +	iio_device_unregister(indio_dev);
> +
>  buffer_cleanup:
>  	iio_triggered_buffer_cleanup(indio_dev);
>  	return ret;
>
> --
> 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 V2 4/8] iio: mma8452: Add support for transient event debouncing

[Jonathan Cameron]

On 29/07/14 10:01, Martin Fuzzey wrote:
> 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>
Once we have figured out how to describe this event this wants to be done
via the IIO_EV_INFO_PERIOD approach rather than a custom attribute.
> ---
>  drivers/iio/accel/mma8452.c |   77 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 75 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 3415b36..1d1e41b 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;
> @@ -356,6 +373,54 @@ static int mma8452_write_event_config(struct iio_dev *indio_dev,
>  	return mma8452_change_config(data, MMA8452_TRANSIENT_CFG, val);
>  }
>  
> +static ssize_t mma8452_query_transient_period(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 us, steps;
> +
> +	steps = i2c_smbus_read_byte_data(data->client, MMA8452_TRANSIENT_COUNT);
> +	if (steps < 0)
> +		return steps;
> +
> +	us = mma8452_transient_time_step_us[mma8452_get_odr_index(data)]
> +								* steps;
> +
> +	return sprintf(buf, "%ld.%06ld\n",
> +			us / USEC_PER_SEC, us % USEC_PER_SEC);
> +}
> +
> +static ssize_t mma8452_set_transient_period(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);
> +	int i, f, us, steps, ret;
> +
> +	ret = iio_str_to_fixpoint(buf, USEC_PER_SEC / 10, &i, &f);
> +	if (ret)
> +		return ret;
> +	if (ret < 0)
> +		return -EINVAL;
> +
> +	us = i * USEC_PER_SEC + f;
> +	steps = us / mma8452_transient_time_step_us[
> +				mma8452_get_odr_index(data)];
> +
> +	if (steps > 0xff)
> +		return -EINVAL;
> +
> +	ret = mma8452_change_config(data, MMA8452_TRANSIENT_COUNT, steps);
> +	if (ret)
> +		return ret;
> +
> +	return len;
> +}
> +
>  static void mma8452_push_transient_event(struct iio_dev *indio_dev,
>  						int chan, u64 ts)
>  {
> @@ -462,8 +527,16 @@ static const struct iio_event_spec mma8452_transient_event[] = {
>   */
>  static IIO_CONST_ATTR_NAMED(accel_transient_scale, in_accel_scale, "0.617742");
>  
> +static IIO_DEVICE_ATTR_NAMED(accel_transient_period,
> +			     in_accel_thresh_rising_period,
> +			     S_IRUGO | S_IWUSR,
> +			     mma8452_query_transient_period,
> +			     mma8452_set_transient_period,
> +			     0);
> +
>  static struct attribute *mma8452_event_attributes[] = {
>  	&iio_const_attr_accel_transient_scale.dev_attr.attr,
> +	&iio_dev_attr_accel_transient_period.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
> 

Re: [PATCH V2 5/8] iio: core: add high pass filter attributes

[Jonathan Cameron]

On 29/07/14 10:01, Martin Fuzzey wrote:
> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
Looks good.
> ---
>  Documentation/ABI/testing/sysfs-bus-iio |   10 ++++++++++
>  drivers/iio/industrialio-core.c         |    2 ++
>  include/linux/iio/iio.h                 |    1 +
>  3 files changed, 13 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index a9757dc..4825139 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -340,6 +340,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.17
> +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
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index fa06197..967468d 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -103,6 +103,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/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index ccde917..88a255d 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -31,6 +31,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,
> 
> --
> 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 V2 6/8] io: mma8452: Add highpass filter configuration.

[Jonathan Cameron]

On 29/07/14 10:01, Martin Fuzzey wrote:
> Allow the cutoff frequency of the high pass filter to be configured.
> 
> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
Good - I'll take these once we've sorted the earlier patches.

> ---
>  drivers/iio/accel/mma8452.c |   65 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 63 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 1d1e41b..eb68f9a 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -29,6 +29,8 @@
>  #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)
> @@ -162,6 +164,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 +190,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 +221,15 @@ 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_raw(struct iio_dev *indio_dev,
>  			    struct iio_chan_spec const *chan,
>  			    int *val, int *val2, long mask)
> @@ -232,6 +268,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:
> +		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;
> +		*val = mma8452_hp_filter_cutoff[i][ret][0];
> +		*val2 = mma8452_hp_filter_cutoff[i][ret][1];
> +		return IIO_VAL_INT_PLUS_MICRO;
>  	}
>  	return -EINVAL;
>  }
> @@ -278,7 +324,7 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
>  			     int val, int val2, long mask)
>  {
>  	struct mma8452_data *data = iio_priv(indio_dev);
> -	int i;
> +	int i, reg;
>  
>  	if (iio_buffer_enabled(indio_dev))
>  		return -EBUSY;
> @@ -306,6 +352,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:
> +		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);
>  	default:
>  		return -EINVAL;
>  	}
> @@ -552,7 +611,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', \
> @@ -575,6 +635,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
> 

Re: [PATCH V2 7/8] iio: mma8452: add an attribute to enable the highpass filter

[Jonathan Cameron]

On 29/07/14 10:01, Martin Fuzzey wrote:
> The hardware contains a single configurable highpass filter which
> is normally used for transient detection (event).
> 
> However it is also possible to enable this filter for normal channel
> reading. Add a new attribute in_accel_high_pass_filter_en to do this.
> 
> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
The obvious answer to this would be to set the cuttoff to 0.
Presumably the complexity here is there  is only one high pass filter
implementation, both for the data and for the event?

If so I would probably still prefer it was done via the 3db point
setting, just that you have two attributes to access that - one for
the channel and one for the event.  Setting the channel version to
non zero would change both.  Setting the event one to any value
would change both only if the channel version is not zero.

It's a little fiddly, perhaps having an enable on a filter is
the way to go... Anyone else have any thought on this?

> ---
>  drivers/iio/accel/mma8452.c |  115 +++++++++++++++++++++++++++++--------------
>  1 file changed, 78 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index eb68f9a..62589f9 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -62,6 +62,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)
> @@ -106,6 +107,43 @@ static int mma8452_read(struct mma8452_data *data, __be16 buf[3])
>  		MMA8452_OUT_X, 3 * sizeof(__be16), (u8 *) buf);
>  }
>  
> +static int mma8452_standby(struct mma8452_data *data)
> +{
> +	return i2c_smbus_write_byte_data(data->client, MMA8452_CTRL_REG1,
> +		data->ctrl_reg1 & ~MMA8452_CTRL_ACTIVE);
> +}
> +
> +static int mma8452_active(struct mma8452_data *data)
> +{
> +	return i2c_smbus_write_byte_data(data->client, MMA8452_CTRL_REG1,
> +		data->ctrl_reg1);
> +}
> +
> +static int mma8452_change_config(struct mma8452_data *data, u8 reg, u8 val)
> +{
> +	int ret;
> +
> +	mutex_lock(&data->lock);
> +
> +	/* config can only be changed when in standby */
> +	ret = mma8452_standby(data);
> +	if (ret < 0)
> +		goto fail;
> +
> +	ret = i2c_smbus_write_byte_data(data->client, reg, val);
> +	if (ret < 0)
> +		goto fail;
> +
> +	ret = mma8452_active(data);
> +	if (ret < 0)
> +		goto fail;
> +
> +	ret = 0;
> +fail:
> +	mutex_unlock(&data->lock);
> +	return ret;
> +}
> +
>  static ssize_t mma8452_show_int_plus_micros(char *buf,
>  	const int (*vals)[2], int n)
>  {
> @@ -201,11 +239,50 @@ static ssize_t mma8452_show_hp_cutoff_avail(struct device *dev,
>  		ARRAY_SIZE(mma8452_hp_filter_cutoff[0]));
>  }
>  
> +static ssize_t mma8452_show_hp_cutoff_en(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);
> +
> +	return sprintf(buf, "%d\n",
> +		data->data_cfg & MMA8452_DATA_CFG_HPF_MASK ? 1 : 0);
> +}
> +
> +static ssize_t mma8452_store_hp_cutoff_en(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;
> +
> +	ret = strtobool(buf, &state);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (state)
> +		data->data_cfg |= MMA8452_DATA_CFG_HPF_MASK;
> +	else
> +		data->data_cfg &= ~MMA8452_DATA_CFG_HPF_MASK;
> +
> +	ret = mma8452_change_config(data, MMA8452_DATA_CFG, data->data_cfg);
> +	if (ret)
> +		return ret;
> +
> +	return len;
> +}
> +
>  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 IIO_DEVICE_ATTR(in_accel_filter_high_pass_en,
> +			 S_IRUGO | S_IWUSR,
> +			 mma8452_show_hp_cutoff_en,
> +			 mma8452_store_hp_cutoff_en, 0);
>  
>  static int mma8452_get_samp_freq_index(struct mma8452_data *data,
>  	int val, int val2)
> @@ -282,43 +359,6 @@ static int mma8452_read_raw(struct iio_dev *indio_dev,
>  	return -EINVAL;
>  }
>  
> -static int mma8452_standby(struct mma8452_data *data)
> -{
> -	return i2c_smbus_write_byte_data(data->client, MMA8452_CTRL_REG1,
> -		data->ctrl_reg1 & ~MMA8452_CTRL_ACTIVE);
> -}
> -
> -static int mma8452_active(struct mma8452_data *data)
> -{
> -	return i2c_smbus_write_byte_data(data->client, MMA8452_CTRL_REG1,
> -		data->ctrl_reg1);
> -}
> -
> -static int mma8452_change_config(struct mma8452_data *data, u8 reg, u8 val)
> -{
> -	int ret;
> -
> -	mutex_lock(&data->lock);
> -
> -	/* config can only be changed when in standby */
> -	ret = mma8452_standby(data);
> -	if (ret < 0)
> -		goto fail;
> -
> -	ret = i2c_smbus_write_byte_data(data->client, reg, val);
> -	if (ret < 0)
> -		goto fail;
> -
> -	ret = mma8452_active(data);
> -	if (ret < 0)
> -		goto fail;
> -
> -	ret = 0;
> -fail:
> -	mutex_unlock(&data->lock);
> -	return ret;
> -}
> -
>  static int mma8452_write_raw(struct iio_dev *indio_dev,
>  			     struct iio_chan_spec const *chan,
>  			     int val, int val2, long mask)
> @@ -636,6 +676,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_in_accel_filter_high_pass_en.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
> 

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

[Jonathan Cameron]

On 29/07/14 10:01, Martin Fuzzey wrote:
> Implement interrupt driven trigger for data ready.
> This allows more efficient access to the sample data.
>
> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
More or less fine - few little bits inline.

J
> ---
>  drivers/iio/accel/mma8452.c |   91 ++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 85 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 62589f9..7ccc369 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>
> @@ -555,18 +557,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, iio_get_time_ns());
> +		ret = IRQ_HANDLED;
> +	}
> +
>  	if (src & MMA8452_INT_TRANS) {
>  		mma8452_transient_interrupt(indio_dev);
> -		return IRQ_HANDLED;
> +		ret = IRQ_HANDLED;
>  	}
>
> -	return IRQ_NONE;
Leave the returns as they were and just return from the new if statement...
It's neater and there is no reason to do a single point of exit if
there is no cleaning up to do.
> +	return ret;
>  }
>
>  static irqreturn_t mma8452_trigger_handler(int irq, void *p)
> @@ -699,6 +707,67 @@ 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 = iio_trigger_alloc("%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)
> +		goto err_trigger_free;
> +
> +	indio_dev->trig = trig;
> +
> +	return 0;
> +
> +err_trigger_free:
> +	iio_trigger_free(trig);
> +
> +	return ret;
> +}
> +
> +static void mma8452_trigger_cleanup(struct iio_dev *indio_dev)
> +{
> +	if (indio_dev->trig) {
> +		iio_trigger_unregister(indio_dev->trig);
Probably no reason not use devm_iio_trigger_alloc and avoid the frees.
Doesn't save much, but someone else will only propose a patch 10 minutes
after this goes in if you don't do it...
> +		iio_trigger_free(indio_dev->trig);
> +	}
> +}
> +
>  static int mma8452_reset(struct i2c_client *client)
>  {
>  	int i;
> @@ -772,7 +841,8 @@ static int mma8452_probe(struct i2c_client *client,
>  		return ret;
>
>  	if (client->irq) {
> -		int supported_interrupts = MMA8452_INT_TRANS;
> +		int supported_interrupts = MMA8452_INT_DRDY | MMA8452_INT_TRANS;
> +		int enabled_interrupts = MMA8452_INT_TRANS;
Note again, I'm very much against coming up with any events enabled....
It's not what people expect so all sorts of messy things could occur.
>
>  		/* Assume wired to INT1 pin */
>  		ret = i2c_smbus_write_byte_data(client,
> @@ -783,7 +853,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;
>  	}
> @@ -793,12 +867,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;
>
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0)
> @@ -820,6 +894,10 @@ device_cleanup:
>
>  buffer_cleanup:
>  	iio_triggered_buffer_cleanup(indio_dev);
> +
> +trigger_cleanup:
> +	mma8452_trigger_cleanup(indio_dev);
> +
>  	return ret;
>  }
>
> @@ -829,6 +907,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;
>
> --
> 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 V2 8/8] iio: mma8452: Add support for interrupt driven triggers.

[Martin Fuzzey]

Hi Jonathan and thanks for the review.

I'll send my comments but I'll be on holiday from tomorrow evening until 
beginning of september so won't be able to send a V3 until later.

On 07/08/14 18:34, Jonathan Cameron wrote:
> ---
> @@ -555,18 +557,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, iio_get_time_ns());
> +		ret = IRQ_HANDLED;
> +	}
> +
>   	if (src & MMA8452_INT_TRANS) {
>   		mma8452_transient_interrupt(indio_dev);
> -		return IRQ_HANDLED;
> +		ret = IRQ_HANDLED;
>   	}
>
> -	return IRQ_NONE;
> Leave the returns as they were and just return from the new if statement...
> It's neater and there is no reason to do a single point of exit if
> there is no cleaning up to do.

The final return still has to be changed so that the function correctly 
handles the case of both interrupt bits being set.
The idea wasn't so much as to create a single point of return as to keep 
the two handler statements symmetrical and not have the second one a 
special case just because it's the last.
The hardware supports other interrupts and there's a risk of someone 
adding a third one later and forgetting to modify the second one.

But if you really insist I can do it that way.
>> +	return ret;
>>   }

+static void mma8452_trigger_cleanup(struct iio_dev *indio_dev)
+{
+	if (indio_dev->trig) {
+		iio_trigger_unregister(indio_dev->trig);

> Probably no reason not use devm_iio_trigger_alloc and avoid the frees.
> Doesn't save much, but someone else will only propose a patch 10 minutes
> after this goes in if you don't do it...

Ok will do for V3

  	if (client->irq) {
-		int supported_interrupts = MMA8452_INT_TRANS;
+		int supported_interrupts = MMA8452_INT_DRDY | MMA8452_INT_TRANS;
+		int enabled_interrupts = MMA8452_INT_TRANS;

> Note again, I'm very much against coming up with any events enabled....
> It's not what people expect so all sorts of messy things could occur.

Yes, quite agree but actually they aren't enabled.

The bit allowing any interrupts generated by the transient detection 
module to be routed to the interrupt pin is enabled.
The threshold is set to the maximum value (your remark on a previous patch).
BUT the bit that actually enables the transient detection (and hence the 
events) is not set until userspace asks for it (by 
mma8452_write_event_config())

The reason for setting the initial threshold is for the case where 
userspace enables the event before writing the threshold value.
It seemed safer to default to a high threshold than a low one.

Regards,

Martin

Re: [PATCH V2 7/8] iio: mma8452: add an attribute to enable the highpass filter

[Martin Fuzzey]

On 07/08/14 18:30, Jonathan Cameron wrote:
> On 29/07/14 10:01, Martin Fuzzey wrote:
>> The hardware contains a single configurable highpass filter which
>> is normally used for transient detection (event).
>>
>> However it is also possible to enable this filter for normal channel
>> reading. Add a new attribute in_accel_high_pass_filter_en to do this.
>>
>> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
> The obvious answer to this would be to set the cuttoff to 0.
> Presumably the complexity here is there  is only one high pass filter
> implementation, both for the data and for the event?
Yes exactly

> If so I would probably still prefer it was done via the 3db point
> setting, just that you have two attributes to access that - one for
> the channel and one for the event.  Setting the channel version to
> non zero would change both.  Setting the event one to any value
> would change both only if the channel version is not zero.
But I don't think that would handle the use case of
* No filter for channel data
* Filter for event

The hardware has
1) A register to set the filter frequency (one of 4 possible values, 
with the allowed set depending on the current sampling frequency)
2) A bit to enable the filter for value readings  (default disabled)
3) A bit to disable the filter for transient detection (default enabled)

My current patchset does not support 3) - the filter is always enabled 
for transient events

> It's a little fiddly, perhaps having an enable on a filter is
> the way to go... Anyone else have any thought on this?
>

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

[Peter Meerwald]

> > 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 fine to me but I'd like an OK from Peter for this series.

looks good, no objection, I haven't test yet however

patch 1/8 is a bug fix but not worth for -stable

in patch 2, the local variable ret could be dropped and replaced with 
returns

patch 3, what is the local variable supported_interrupts good for?

patch 6/8 still has wrong subject: io -> iio

> >  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}
> >  };
> >
> > -/*
> > +/*
> This ought to be in a separate cleanup patch.  Just adds noise to the important
> stuff in here!  Not important though so don't bother rerolling the series for this.
> 
> >   * 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
> >
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: [PATCH V2 7/8] iio: mma8452: add an attribute to enable the highpass filter

[Jonathan Cameron]

On 07/08/14 18:36, Martin Fuzzey wrote:
> On 07/08/14 18:30, Jonathan Cameron wrote:
>> On 29/07/14 10:01, Martin Fuzzey wrote:
>>> The hardware contains a single configurable highpass filter which
>>> is normally used for transient detection (event).
>>>
>>> However it is also possible to enable this filter for normal channel
>>> reading. Add a new attribute in_accel_high_pass_filter_en to do this.
>>>
>>> Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com>
>> The obvious answer to this would be to set the cuttoff to 0.
>> Presumably the complexity here is there  is only one high pass filter
>> implementation, both for the data and for the event?
> Yes exactly
> 
>> If so I would probably still prefer it was done via the 3db point
>> setting, just that you have two attributes to access that - one for
>> the channel and one for the event.  Setting the channel version to
>> non zero would change both.  Setting the event one to any value
>> would change both only if the channel version is not zero.
> But I don't think that would handle the use case of
> * No filter for channel data
> * Filter for event
That was what the nasty bit (which I also don't like!) in my suggestion
was meant to handle.  Any time the channel version is 0 then a write
to the event one doesn't change it.  If it is non zero then it does.
(uggly use of 0 to specify that we have disabled the filter).
> 
> The hardware has
> 1) A register to set the filter frequency (one of 4 possible values, with the allowed set depending on the current
> sampling frequency)
> 2) A bit to enable the filter for value readings  (default disabled)
> 3) A bit to disable the filter for transient detection (default enabled)
> 
> My current patchset does not support 3) - the filter is always enabled for transient events
Ah. In that case perhaps a slight extension of the above:

Again two filter setting attributes, one for the event and one for the channel.
Both default to off
event : 0, channel : 0
write event version with value A

event : A, channel : 0

write channel version with value B
event : B, channel : B

write event version with value C

event : C, channel : C

write event version with 0

event : 0, channel : C

write event version with D

event : D, channel : D

write channel version with 0 to disable

event : D, channel : 0

write event version with 0 to disable

event : 0, channel 0 - no filters on.

Should be relatively easy to implement.  When you get a write simply check if the
other cached value is currently 0.  If it is, don't update it.  If it isn't then
do.  Unless a 0 is being written, in which case only disable which ever one has
been written to.

Mind you this logic will get interesting for low pass filters as setting the
cut off to 0 will end up letting through everything (somewhat counterintuitive!)


> 
>> It's a little fiddly, perhaps having an enable on a filter is
>> the way to go... Anyone else have any thought on this?
>>
> 
> -- 
> 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 V2 8/8] iio: mma8452: Add support for interrupt driven triggers.

[Jonathan Cameron]

On 07/08/14 18:20, Martin Fuzzey wrote:
> Hi Jonathan and thanks for the review.
> 
> I'll send my comments but I'll be on holiday from tomorrow evening until beginning of september so won't be able to send
> a V3 until later.
> 
Hope you had a good time!
> On 07/08/14 18:34, Jonathan Cameron wrote:
>> ---
>> @@ -555,18 +557,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, iio_get_time_ns());
>> +        ret = IRQ_HANDLED;
>> +    }
>> +
>>       if (src & MMA8452_INT_TRANS) {
>>           mma8452_transient_interrupt(indio_dev);
>> -        return IRQ_HANDLED;
>> +        ret = IRQ_HANDLED;
>>       }
>>
>> -    return IRQ_NONE;
>> Leave the returns as they were and just return from the new if statement...
>> It's neater and there is no reason to do a single point of exit if
>> there is no cleaning up to do.
> 
> The final return still has to be changed so that the function correctly handles the case of both interrupt bits being set.
> The idea wasn't so much as to create a single point of return as to keep the two handler statements symmetrical and not
> have the second one a special case just because it's the last.
> The hardware supports other interrupts and there's a risk of someone adding a third one later and forgetting to modify
> the second one.
> 
> But if you really insist I can do it that way.
Leave it be, I'd missed the possiblity of both.
>>> +    return ret;
>>>   }
> 
> +static void mma8452_trigger_cleanup(struct iio_dev *indio_dev)
> +{
> +    if (indio_dev->trig) {
> +        iio_trigger_unregister(indio_dev->trig);
> 
>> Probably no reason not use devm_iio_trigger_alloc and avoid the frees.
>> Doesn't save much, but someone else will only propose a patch 10 minutes
>> after this goes in if you don't do it...
> 
> Ok will do for V3
> 
>      if (client->irq) {
> -        int supported_interrupts = MMA8452_INT_TRANS;
> +        int supported_interrupts = MMA8452_INT_DRDY | MMA8452_INT_TRANS;
> +        int enabled_interrupts = MMA8452_INT_TRANS;
> 
>> Note again, I'm very much against coming up with any events enabled....
>> It's not what people expect so all sorts of messy things could occur.
> 
> Yes, quite agree but actually they aren't enabled.
> 
> The bit allowing any interrupts generated by the transient detection module to be routed to the interrupt pin is enabled.
> The threshold is set to the maximum value (your remark on a previous patch).
> BUT the bit that actually enables the transient detection (and hence the events) is not set until userspace asks for it
> (by mma8452_write_event_config())
Fair enough. Thanks for the explanation.
> 
> The reason for setting the initial threshold is for the case where userspace enables the event before writing the
> threshold value.
> It seemed safer to default to a high threshold than a low one.
Hmm.. More fool them if they don't check ;)

J
> 
> Regards,
> 
> Martin
> 
> -- 
> 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 V2 1/8] iio: mma8452: Initialise before activating

[Jonathan Cameron]

On 07/08/14 18:47, Peter Meerwald 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 fine to me but I'd like an OK from Peter for this series.
> 
> looks good, no objection, I haven't test yet however
No rush given timing.  Just let me know if you aren't going
to test them for a VERY long time ;)
> 
> patch 1/8 is a bug fix but not worth for -stable
> 
> in patch 2, the local variable ret could be dropped and replaced with 
> returns
> 
> patch 3, what is the local variable supported_interrupts good for?
> 
> patch 6/8 still has wrong subject: io -> iio
> 
>>>  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}
>>>  };
>>>
>>> -/*
>>> +/*
>> This ought to be in a separate cleanup patch.  Just adds noise to the important
>> stuff in here!  Not important though so don't bother rerolling the series for this.
>>
>>>   * 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
>>>
>>
>