[PATCH v2 0/3] iio: accel: add support for Memsic MXC4005

[PATCH v2 0/3] iio: accel: add support for Memsic MXC4005

[Teodora Baluta]

This patch set adds support for Memsic MXC4005XC 3-axis accelerometer
sensor. The first patch adds basic support for the accelerometer, with
raw readings, the second patch adds support for buffer mode and the last
one adds the data ready trigger.

---
Changes since v1:
- fixes for the comments (coding style, proper buffer length,
  available_scan_masks)
- remove unnecessary locking for regmap operations since regmap has an
  internal locking mechanism
- keep the mutex for set_trigger_state operation to protect the state
  variable trigger_enabled

Teodora Baluta (3):
  iio: accel: add support for mxc4005 accelerometer
  iio: mxc4005: add triggered buffer mode for mxc4005
  iio: mxc4005: add data ready trigger for mxc4005

 drivers/iio/accel/Kconfig   |  13 +
 drivers/iio/accel/Makefile  |   2 +
 drivers/iio/accel/mxc4005.c | 586 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 601 insertions(+)
 create mode 100644 drivers/iio/accel/mxc4005.c

-- 
1.9.1

[PATCH v2 1/3] iio: accel: add support for mxc4005 accelerometer

[Teodora Baluta]

This patch adds support for Memsic MXC4005XC 3-axis accelerometer. The
current implementation is a minimal one as it adds raw readings for the
three axes and setting scale from userspace.

Signed-off-by: Teodora Baluta <teodora.baluta@intel.com>
---
 drivers/iio/accel/Kconfig   |  11 ++
 drivers/iio/accel/Makefile  |   2 +
 drivers/iio/accel/mxc4005.c | 358 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 371 insertions(+)
 create mode 100644 drivers/iio/accel/mxc4005.c

diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index a59047d..69302be 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -137,6 +137,17 @@ config MMA9553
 	  To compile this driver as a module, choose M here: the module
 	  will be called mma9553.
 
+config MXC4005
+	tristate "Memsic MXC4005XC 3-Axis Accelerometer Driver"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  Say yes here to build support for the Memsic MXC4005XC 3-axis
+	  accelerometer.
+
+	  To compile this driver as a module, choose M. The module will be
+	  called mxc4005.
+
 config STK8312
 	tristate "Sensortek STK8312 3-Axis Accelerometer Driver"
 	depends on I2C
diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
index ebd2675..020dda0 100644
--- a/drivers/iio/accel/Makefile
+++ b/drivers/iio/accel/Makefile
@@ -14,6 +14,8 @@ obj-$(CONFIG_MMA9551_CORE)	+= mma9551_core.o
 obj-$(CONFIG_MMA9551)		+= mma9551.o
 obj-$(CONFIG_MMA9553)		+= mma9553.o
 
+obj-$(CONFIG_MXC4005)		+= mxc4005.o
+
 obj-$(CONFIG_STK8312)		+= stk8312.o
 obj-$(CONFIG_STK8BA50)		+= stk8ba50.o
 
diff --git a/drivers/iio/accel/mxc4005.c b/drivers/iio/accel/mxc4005.c
new file mode 100644
index 0000000..76a7954
--- /dev/null
+++ b/drivers/iio/accel/mxc4005.c
@@ -0,0 +1,358 @@
+/*
+ * 3-axis accelerometer driver for MXC4005XC Memsic sensor
+ *
+ * Copyright (c) 2014, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/acpi.h>
+#include <linux/regmap.h>
+#include <linux/iio/sysfs.h>
+
+#define MXC4005_DRV_NAME		"mxc4005"
+#define MXC4005_REGMAP_NAME		"mxc4005_regmap"
+
+#define MXC4005_REG_XOUT_UPPER		0x03
+#define MXC4005_REG_XOUT_LOWER		0x04
+#define MXC4005_REG_YOUT_UPPER		0x05
+#define MXC4005_REG_YOUT_LOWER		0x06
+#define MXC4005_REG_ZOUT_UPPER		0x07
+#define MXC4005_REG_ZOUT_LOWER		0x08
+
+#define MXC4005_REG_CONTROL		0x0D
+#define MXC4005_REG_CONTROL_MASK_FSR	GENMASK(6, 5)
+#define MXC4005_CONTROL_FSR_SHIFT	5
+
+#define MXC4005_REG_DEVICE_ID		0x0E
+
+enum mxc4005_axis {
+	AXIS_X,
+	AXIS_Y,
+	AXIS_Z,
+};
+
+enum mxc4005_range {
+	MXC4005_RANGE_2G,
+	MXC4005_RANGE_4G,
+	MXC4005_RANGE_8G,
+};
+
+struct mxc4005_data {
+	struct i2c_client *client;
+	struct mutex mutex;
+	struct regmap *regmap;
+};
+
+/*
+ * MXC4005 can operate in the following ranges:
+ * +/- 2G, 4G, 8G (the default +/-2G)
+ *
+ * (2 + 2) * 9.81 / (2^12 - 1) = 0.009582
+ * (4 + 4) * 9.81 / (2^12 - 1) = 0.019164
+ * (8 + 8) * 9.81 / (2^12 - 1) = 0.038329
+ */
+static const struct {
+	u8 range;
+	int scale;
+} mxc4005_scale_table[] = {
+	{MXC4005_RANGE_2G, 9582},
+	{MXC4005_RANGE_4G, 19164},
+	{MXC4005_RANGE_8G, 38329},
+};
+
+
+static IIO_CONST_ATTR(in_accel_scale_available, "0.009582 0.019164 0.038329");
+
+static struct attribute *mxc4005_attributes[] = {
+	&iio_const_attr_in_accel_scale_available.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group mxc4005_attrs_group = {
+	.attrs = mxc4005_attributes,
+};
+
+static bool mxc4005_is_readable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case MXC4005_REG_XOUT_UPPER:
+	case MXC4005_REG_XOUT_LOWER:
+	case MXC4005_REG_YOUT_UPPER:
+	case MXC4005_REG_YOUT_LOWER:
+	case MXC4005_REG_ZOUT_UPPER:
+	case MXC4005_REG_ZOUT_LOWER:
+	case MXC4005_REG_DEVICE_ID:
+	case MXC4005_REG_CONTROL:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool mxc4005_is_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case MXC4005_REG_CONTROL:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config mxc4005_regmap_config = {
+	.name = MXC4005_DRV_NAME,
+
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.max_register = MXC4005_REG_DEVICE_ID,
+	.cache_type = REGCACHE_NONE,
+
+	.readable_reg = mxc4005_is_readable_reg,
+	.writeable_reg = mxc4005_is_writeable_reg,
+};
+
+static int mxc4005_read_axis(struct mxc4005_data *data,
+			     unsigned int addr)
+{
+	__be16 reg;
+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, addr, (u8 *) &reg, sizeof(reg));
+	if (ret < 0) {
+		dev_err(&data->client->dev, "failed to read reg %02x\n", addr);
+		return ret;
+	}
+
+	return be16_to_cpu(reg);
+}
+
+static int mxc4005_read_scale(struct mxc4005_data *data)
+{
+	unsigned int reg;
+	int ret;
+	int i;
+
+	ret = regmap_read(data->regmap, MXC4005_REG_CONTROL, &reg);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "failed to read reg_control\n");
+		return ret;
+	}
+
+	i = reg >> MXC4005_CONTROL_FSR_SHIFT;
+
+	if (i < 0 || i >= ARRAY_SIZE(mxc4005_scale_table))
+		return -EINVAL;
+
+	return mxc4005_scale_table[i].scale;
+}
+
+static int mxc4005_set_scale(struct mxc4005_data *data, int val)
+{
+	unsigned int reg;
+	int i;
+	int ret;
+
+	for (i = 0; i < ARRAY_SIZE(mxc4005_scale_table); i++) {
+		if (mxc4005_scale_table[i].scale == val) {
+			reg = i << MXC4005_CONTROL_FSR_SHIFT;
+			ret = regmap_update_bits(data->regmap,
+						 MXC4005_REG_CONTROL,
+						 MXC4005_REG_CONTROL_MASK_FSR,
+						 reg);
+			if (ret < 0)
+				dev_err(&data->client->dev,
+					"failed to write reg_control\n");
+			return ret;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static int mxc4005_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct mxc4005_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		switch (chan->type) {
+		case IIO_ACCEL:
+			if (iio_buffer_enabled(indio_dev))
+				return -EBUSY;
+
+			ret = mxc4005_read_axis(data, chan->address);
+			if (ret < 0)
+				return ret;
+			*val = sign_extend32(ret >> 4, 11);
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_SCALE:
+		ret = mxc4005_read_scale(data);
+		if (ret < 0)
+			return ret;
+
+		*val = 0;
+		*val2 = ret;
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int mxc4005_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	struct mxc4005_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		if (val != 0)
+			return -EINVAL;
+
+		return mxc4005_set_scale(data, val2);
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info mxc4005_info = {
+	.driver_module	= THIS_MODULE,
+	.read_raw	= mxc4005_read_raw,
+	.write_raw	= mxc4005_write_raw,
+	.attrs		= &mxc4005_attrs_group,
+};
+
+#define MXC4005_CHANNEL(_axis, _addr) {				\
+	.type = IIO_ACCEL,					\
+	.modified = 1,						\
+	.channel2 = IIO_MOD_##_axis,				\
+	.address = _addr,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+}
+
+static const struct iio_chan_spec mxc4005_channels[] = {
+	MXC4005_CHANNEL(X, MXC4005_REG_XOUT_UPPER),
+	MXC4005_CHANNEL(Y, MXC4005_REG_YOUT_UPPER),
+	MXC4005_CHANNEL(Z, MXC4005_REG_ZOUT_UPPER),
+};
+
+static int mxc4005_chip_init(struct mxc4005_data *data)
+{
+	int ret;
+	unsigned int reg;
+
+	ret = regmap_read(data->regmap, MXC4005_REG_DEVICE_ID, &reg);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "failed to read chip id\n");
+		return ret;
+	}
+
+	dev_dbg(&data->client->dev, "MXC4005 chip id %02x\n", reg);
+
+	return 0;
+}
+
+static int mxc4005_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct mxc4005_data *data;
+	struct iio_dev *indio_dev;
+	struct regmap *regmap;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	regmap = devm_regmap_init_i2c(client, &mxc4005_regmap_config);
+	if (IS_ERR(regmap)) {
+		dev_err(&client->dev, "failed to initialize regmap\n");
+		return PTR_ERR(regmap);
+	}
+
+	data = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+	data->client = client;
+	data->regmap = regmap;
+
+	ret = mxc4005_chip_init(data);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to init chip\n");
+		return ret;
+	}
+
+	mutex_init(&data->mutex);
+
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->channels = mxc4005_channels;
+	indio_dev->num_channels = ARRAY_SIZE(mxc4005_channels);
+	indio_dev->name = MXC4005_DRV_NAME;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &mxc4005_info;
+
+	ret = iio_device_register(indio_dev);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"unable to register iio device %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int mxc4005_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+
+	iio_device_unregister(indio_dev);
+
+	return 0;
+}
+
+static const struct acpi_device_id mxc4005_acpi_match[] = {
+	{"MXC4005",	0},
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, mxc4005_acpi_match);
+
+static const struct i2c_device_id mxc4005_id[] = {
+	{"mxc4005",	0},
+	{ },
+};
+
+MODULE_DEVICE_TABLE(i2c, mxc4005_id);
+
+static struct i2c_driver mxc4005_driver = {
+	.driver = {
+		.name = MXC4005_DRV_NAME,
+		.acpi_match_table = ACPI_PTR(mxc4005_acpi_match),
+	},
+	.probe		= mxc4005_probe,
+	.remove		= mxc4005_remove,
+	.id_table	= mxc4005_id,
+};
+
+module_i2c_driver(mxc4005_driver);
+
+MODULE_AUTHOR("Teodora Baluta <teodora.baluta@intel.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("MXC4005 3-axis accelerometer driver");
-- 
1.9.1

Re: [PATCH v2 1/3] iio: accel: add support for mxc4005 accelerometer

[Jonathan Cameron]

On 13/08/15 18:31, Teodora Baluta wrote:
> This patch adds support for Memsic MXC4005XC 3-axis accelerometer. The
> current implementation is a minimal one as it adds raw readings for the
> three axes and setting scale from userspace.
> 
> Signed-off-by: Teodora Baluta <teodora.baluta@intel.com>
Looking pretty good.  Some real nitpicks and minor suggestions
inline.  I was going to leave this on the list anyway to give
Peter and others time for another look so thought I might as
well be fussy.  What can I say. It's Sunday morning and the
coffee hasn't kicked in yet :)

Jonathan
> ---
>  drivers/iio/accel/Kconfig   |  11 ++
>  drivers/iio/accel/Makefile  |   2 +
>  drivers/iio/accel/mxc4005.c | 358 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 371 insertions(+)
>  create mode 100644 drivers/iio/accel/mxc4005.c
> 
> diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> index a59047d..69302be 100644
> --- a/drivers/iio/accel/Kconfig
> +++ b/drivers/iio/accel/Kconfig
> @@ -137,6 +137,17 @@ config MMA9553
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called mma9553.
>  
> +config MXC4005
> +	tristate "Memsic MXC4005XC 3-Axis Accelerometer Driver"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  Say yes here to build support for the Memsic MXC4005XC 3-axis
> +	  accelerometer.
> +
> +	  To compile this driver as a module, choose M. The module will be
> +	  called mxc4005.
> +
>  config STK8312
>  	tristate "Sensortek STK8312 3-Axis Accelerometer Driver"
>  	depends on I2C
> diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
> index ebd2675..020dda0 100644
> --- a/drivers/iio/accel/Makefile
> +++ b/drivers/iio/accel/Makefile
> @@ -14,6 +14,8 @@ obj-$(CONFIG_MMA9551_CORE)	+= mma9551_core.o
>  obj-$(CONFIG_MMA9551)		+= mma9551.o
>  obj-$(CONFIG_MMA9553)		+= mma9553.o
>  
> +obj-$(CONFIG_MXC4005)		+= mxc4005.o
> +
>  obj-$(CONFIG_STK8312)		+= stk8312.o
>  obj-$(CONFIG_STK8BA50)		+= stk8ba50.o
>  
> diff --git a/drivers/iio/accel/mxc4005.c b/drivers/iio/accel/mxc4005.c
> new file mode 100644
> index 0000000..76a7954
> --- /dev/null
> +++ b/drivers/iio/accel/mxc4005.c
> @@ -0,0 +1,358 @@
> +/*
> + * 3-axis accelerometer driver for MXC4005XC Memsic sensor
> + *
> + * Copyright (c) 2014, Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/acpi.h>
> +#include <linux/regmap.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define MXC4005_DRV_NAME		"mxc4005"
> +#define MXC4005_REGMAP_NAME		"mxc4005_regmap"
You don't actually use the above...  Guessing you did mean
to but used drv_name instead in the regmap configuration.
> +
> +#define MXC4005_REG_XOUT_UPPER		0x03
> +#define MXC4005_REG_XOUT_LOWER		0x04
> +#define MXC4005_REG_YOUT_UPPER		0x05
> +#define MXC4005_REG_YOUT_LOWER		0x06
> +#define MXC4005_REG_ZOUT_UPPER		0x07
> +#define MXC4005_REG_ZOUT_LOWER		0x08
> +
> +#define MXC4005_REG_CONTROL		0x0D
> +#define MXC4005_REG_CONTROL_MASK_FSR	GENMASK(6, 5)
> +#define MXC4005_CONTROL_FSR_SHIFT	5
> +
> +#define MXC4005_REG_DEVICE_ID		0x0E
> +
> +enum mxc4005_axis {
> +	AXIS_X,
> +	AXIS_Y,
> +	AXIS_Z,
> +};
> +
> +enum mxc4005_range {
> +	MXC4005_RANGE_2G,
> +	MXC4005_RANGE_4G,
> +	MXC4005_RANGE_8G,
> +};
> +
> +struct mxc4005_data {
> +	struct i2c_client *client;
In a regmap cleanup series someone posted the other day,
they just put struct device *dev in here instead and
assigned data.dev = client->dev;

Result is slightly cleaner code.  Worth doing here?

> +	struct mutex mutex;
> +	struct regmap *regmap;
> +};
> +
> +/*
> + * MXC4005 can operate in the following ranges:
> + * +/- 2G, 4G, 8G (the default +/-2G)
> + *
> + * (2 + 2) * 9.81 / (2^12 - 1) = 0.009582
> + * (4 + 4) * 9.81 / (2^12 - 1) = 0.019164
> + * (8 + 8) * 9.81 / (2^12 - 1) = 0.038329
> + */
> +static const struct {
> +	u8 range;
> +	int scale;
> +} mxc4005_scale_table[] = {
> +	{MXC4005_RANGE_2G, 9582},
> +	{MXC4005_RANGE_4G, 19164},
> +	{MXC4005_RANGE_8G, 38329},
> +};
> +
> +
> +static IIO_CONST_ATTR(in_accel_scale_available, "0.009582 0.019164 0.038329");
> +
> +static struct attribute *mxc4005_attributes[] = {
> +	&iio_const_attr_in_accel_scale_available.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group mxc4005_attrs_group = {
> +	.attrs = mxc4005_attributes,
> +};
> +
> +static bool mxc4005_is_readable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case MXC4005_REG_XOUT_UPPER:
> +	case MXC4005_REG_XOUT_LOWER:
> +	case MXC4005_REG_YOUT_UPPER:
> +	case MXC4005_REG_YOUT_LOWER:
> +	case MXC4005_REG_ZOUT_UPPER:
> +	case MXC4005_REG_ZOUT_LOWER:
> +	case MXC4005_REG_DEVICE_ID:
> +	case MXC4005_REG_CONTROL:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool mxc4005_is_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case MXC4005_REG_CONTROL:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static const struct regmap_config mxc4005_regmap_config = {
> +	.name = MXC4005_DRV_NAME,
> +
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +
> +	.max_register = MXC4005_REG_DEVICE_ID,
> +	.cache_type = REGCACHE_NONE,
REGCACHE_NONE is the default. No need to specify it here.
(enum value of 0)
> +
> +	.readable_reg = mxc4005_is_readable_reg,
> +	.writeable_reg = mxc4005_is_writeable_reg,
> +};
> +
> +static int mxc4005_read_axis(struct mxc4005_data *data,
> +			     unsigned int addr)
> +{
> +	__be16 reg;
> +	int ret;
> +
> +	ret = regmap_bulk_read(data->regmap, addr, (u8 *) &reg, sizeof(reg));
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "failed to read reg %02x\n", addr);
> +		return ret;
> +	}
> +
> +	return be16_to_cpu(reg);
> +}
> +
> +static int mxc4005_read_scale(struct mxc4005_data *data)
> +{
> +	unsigned int reg;
> +	int ret;
> +	int i;
> +
> +	ret = regmap_read(data->regmap, MXC4005_REG_CONTROL, &reg);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "failed to read reg_control\n");
> +		return ret;
> +	}
> +
> +	i = reg >> MXC4005_CONTROL_FSR_SHIFT;
> +
> +	if (i < 0 || i >= ARRAY_SIZE(mxc4005_scale_table))
> +		return -EINVAL;
> +
> +	return mxc4005_scale_table[i].scale;
> +}
> +
> +static int mxc4005_set_scale(struct mxc4005_data *data, int val)
> +{
> +	unsigned int reg;
> +	int i;
> +	int ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(mxc4005_scale_table); i++) {
> +		if (mxc4005_scale_table[i].scale == val) {
> +			reg = i << MXC4005_CONTROL_FSR_SHIFT;
> +			ret = regmap_update_bits(data->regmap,
> +						 MXC4005_REG_CONTROL,
> +						 MXC4005_REG_CONTROL_MASK_FSR,
> +						 reg);
> +			if (ret < 0)
> +				dev_err(&data->client->dev,
> +					"failed to write reg_control\n");
> +			return ret;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int mxc4005_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct mxc4005_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_ACCEL:
> +			if (iio_buffer_enabled(indio_dev))
> +				return -EBUSY;
> +
> +			ret = mxc4005_read_axis(data, chan->address);
> +			if (ret < 0)
> +				return ret;
> +			*val = sign_extend32(ret >> 4, 11);
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = mxc4005_read_scale(data);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = 0;
> +		*val2 = ret;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int mxc4005_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int val, int val2, long mask)
> +{
> +	struct mxc4005_data *data = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		if (val != 0)
> +			return -EINVAL;
> +
> +		return mxc4005_set_scale(data, val2);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info mxc4005_info = {
> +	.driver_module	= THIS_MODULE,
> +	.read_raw	= mxc4005_read_raw,
> +	.write_raw	= mxc4005_write_raw,
> +	.attrs		= &mxc4005_attrs_group,
> +};
> +
> +#define MXC4005_CHANNEL(_axis, _addr) {				\
> +	.type = IIO_ACCEL,					\
> +	.modified = 1,						\
> +	.channel2 = IIO_MOD_##_axis,				\
> +	.address = _addr,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +}
> +
> +static const struct iio_chan_spec mxc4005_channels[] = {
> +	MXC4005_CHANNEL(X, MXC4005_REG_XOUT_UPPER),
> +	MXC4005_CHANNEL(Y, MXC4005_REG_YOUT_UPPER),
> +	MXC4005_CHANNEL(Z, MXC4005_REG_ZOUT_UPPER),
> +};
> +
> +static int mxc4005_chip_init(struct mxc4005_data *data)
> +{
> +	int ret;
> +	unsigned int reg;
> +
> +	ret = regmap_read(data->regmap, MXC4005_REG_DEVICE_ID, &reg);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "failed to read chip id\n");
> +		return ret;
> +	}
> +
> +	dev_dbg(&data->client->dev, "MXC4005 chip id %02x\n", reg);
> +
> +	return 0;
> +}
> +
> +static int mxc4005_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct mxc4005_data *data;
> +	struct iio_dev *indio_dev;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	regmap = devm_regmap_init_i2c(client, &mxc4005_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		dev_err(&client->dev, "failed to initialize regmap\n");
> +		return PTR_ERR(regmap);
> +	}
> +
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +	data->client = client;
> +	data->regmap = regmap;
> +
> +	ret = mxc4005_chip_init(data);
> +	if (ret < 0) {
For consistency with previous error message (yeah I'm getting
picky ;) - initialize
> +		dev_err(&client->dev, "failed to init chip\n");
> +		return ret;
> +	}
> +
> +	mutex_init(&data->mutex);
> +
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->channels = mxc4005_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(mxc4005_channels);
> +	indio_dev->name = MXC4005_DRV_NAME;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &mxc4005_info;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret < 0) {
> +		dev_err(&client->dev,
> +			"unable to register iio device %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mxc4005_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
Roll the line above and the one below into one.  No point in the
separation.
> +	iio_device_unregister(indio_dev);
> +
> +	return 0;
> +}
> +
> +static const struct acpi_device_id mxc4005_acpi_match[] = {
> +	{"MXC4005",	0},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(acpi, mxc4005_acpi_match);
> +
> +static const struct i2c_device_id mxc4005_id[] = {
> +	{"mxc4005",	0},
> +	{ },
> +};
For consistency no blank line here.
> +
> +MODULE_DEVICE_TABLE(i2c, mxc4005_id);
> +
> +static struct i2c_driver mxc4005_driver = {
> +	.driver = {
> +		.name = MXC4005_DRV_NAME,
> +		.acpi_match_table = ACPI_PTR(mxc4005_acpi_match),
> +	},
> +	.probe		= mxc4005_probe,
> +	.remove		= mxc4005_remove,
> +	.id_table	= mxc4005_id,
> +};
> +
> +module_i2c_driver(mxc4005_driver);
> +
> +MODULE_AUTHOR("Teodora Baluta <teodora.baluta@intel.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("MXC4005 3-axis accelerometer driver");
> 

Re: [PATCH v2 1/3] iio: accel: add support for mxc4005 accelerometer

[Teodora Baluta]

On Sun, Aug 16, 2015 at 08:31:54AM +0100, Jonathan Cameron wrote:
> On 13/08/15 18:31, Teodora Baluta wrote:
> > This patch adds support for Memsic MXC4005XC 3-axis accelerometer. The
> > current implementation is a minimal one as it adds raw readings for the
> > three axes and setting scale from userspace.
> > 
> > Signed-off-by: Teodora Baluta <teodora.baluta@intel.com>
> Looking pretty good.  Some real nitpicks and minor suggestions
> inline.  I was going to leave this on the list anyway to give
> Peter and others time for another look so thought I might as
> well be fussy.  What can I say. It's Sunday morning and the
> coffee hasn't kicked in yet :)

Thanks, it's very nice to get some more feedback. :) I have missed a
couple of things and I'll send another patch set asap.

> 
> Jonathan
> > ---
> >  drivers/iio/accel/Kconfig   |  11 ++
> >  drivers/iio/accel/Makefile  |   2 +
> >  drivers/iio/accel/mxc4005.c | 358 ++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 371 insertions(+)
> >  create mode 100644 drivers/iio/accel/mxc4005.c
> > 
> > diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> > index a59047d..69302be 100644
> > --- a/drivers/iio/accel/Kconfig
> > +++ b/drivers/iio/accel/Kconfig
> > @@ -137,6 +137,17 @@ config MMA9553
> >  	  To compile this driver as a module, choose M here: the module
> >  	  will be called mma9553.
> >  
> > +config MXC4005
> > +	tristate "Memsic MXC4005XC 3-Axis Accelerometer Driver"
> > +	depends on I2C
> > +	select REGMAP_I2C
> > +	help
> > +	  Say yes here to build support for the Memsic MXC4005XC 3-axis
> > +	  accelerometer.
> > +
> > +	  To compile this driver as a module, choose M. The module will be
> > +	  called mxc4005.
> > +
> >  config STK8312
> >  	tristate "Sensortek STK8312 3-Axis Accelerometer Driver"
> >  	depends on I2C
> > diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
> > index ebd2675..020dda0 100644
> > --- a/drivers/iio/accel/Makefile
> > +++ b/drivers/iio/accel/Makefile
> > @@ -14,6 +14,8 @@ obj-$(CONFIG_MMA9551_CORE)	+= mma9551_core.o
> >  obj-$(CONFIG_MMA9551)		+= mma9551.o
> >  obj-$(CONFIG_MMA9553)		+= mma9553.o
> >  
> > +obj-$(CONFIG_MXC4005)		+= mxc4005.o
> > +
> >  obj-$(CONFIG_STK8312)		+= stk8312.o
> >  obj-$(CONFIG_STK8BA50)		+= stk8ba50.o
> >  
> > diff --git a/drivers/iio/accel/mxc4005.c b/drivers/iio/accel/mxc4005.c
> > new file mode 100644
> > index 0000000..76a7954
> > --- /dev/null
> > +++ b/drivers/iio/accel/mxc4005.c
> > @@ -0,0 +1,358 @@
> > +/*
> > + * 3-axis accelerometer driver for MXC4005XC Memsic sensor
> > + *
> > + * Copyright (c) 2014, Intel Corporation.
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> > + * more details.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/i2c.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/acpi.h>
> > +#include <linux/regmap.h>
> > +#include <linux/iio/sysfs.h>
> > +
> > +#define MXC4005_DRV_NAME		"mxc4005"
> > +#define MXC4005_REGMAP_NAME		"mxc4005_regmap"
> You don't actually use the above...  Guessing you did mean
> to but used drv_name instead in the regmap configuration.

That's right. :(

> > +
> > +#define MXC4005_REG_XOUT_UPPER		0x03
> > +#define MXC4005_REG_XOUT_LOWER		0x04
> > +#define MXC4005_REG_YOUT_UPPER		0x05
> > +#define MXC4005_REG_YOUT_LOWER		0x06
> > +#define MXC4005_REG_ZOUT_UPPER		0x07
> > +#define MXC4005_REG_ZOUT_LOWER		0x08
> > +
> > +#define MXC4005_REG_CONTROL		0x0D
> > +#define MXC4005_REG_CONTROL_MASK_FSR	GENMASK(6, 5)
> > +#define MXC4005_CONTROL_FSR_SHIFT	5
> > +
> > +#define MXC4005_REG_DEVICE_ID		0x0E
> > +
> > +enum mxc4005_axis {
> > +	AXIS_X,
> > +	AXIS_Y,
> > +	AXIS_Z,
> > +};
> > +
> > +enum mxc4005_range {
> > +	MXC4005_RANGE_2G,
> > +	MXC4005_RANGE_4G,
> > +	MXC4005_RANGE_8G,
> > +};
> > +
> > +struct mxc4005_data {
> > +	struct i2c_client *client;
> In a regmap cleanup series someone posted the other day,
> they just put struct device *dev in here instead and
> assigned data.dev = client->dev;
> 
> Result is slightly cleaner code.  Worth doing here?
> 

Worth a change for this since I am always doing client->dev.

> > +	struct mutex mutex;
> > +	struct regmap *regmap;
> > +};
> > +
> > +/*
> > + * MXC4005 can operate in the following ranges:
> > + * +/- 2G, 4G, 8G (the default +/-2G)
> > + *
> > + * (2 + 2) * 9.81 / (2^12 - 1) = 0.009582
> > + * (4 + 4) * 9.81 / (2^12 - 1) = 0.019164
> > + * (8 + 8) * 9.81 / (2^12 - 1) = 0.038329
> > + */
> > +static const struct {
> > +	u8 range;
> > +	int scale;
> > +} mxc4005_scale_table[] = {
> > +	{MXC4005_RANGE_2G, 9582},
> > +	{MXC4005_RANGE_4G, 19164},
> > +	{MXC4005_RANGE_8G, 38329},
> > +};
> > +
> > +
> > +static IIO_CONST_ATTR(in_accel_scale_available, "0.009582 0.019164 0.038329");
> > +
> > +static struct attribute *mxc4005_attributes[] = {
> > +	&iio_const_attr_in_accel_scale_available.dev_attr.attr,
> > +	NULL,
> > +};
> > +
> > +static const struct attribute_group mxc4005_attrs_group = {
> > +	.attrs = mxc4005_attributes,
> > +};
> > +
> > +static bool mxc4005_is_readable_reg(struct device *dev, unsigned int reg)
> > +{
> > +	switch (reg) {
> > +	case MXC4005_REG_XOUT_UPPER:
> > +	case MXC4005_REG_XOUT_LOWER:
> > +	case MXC4005_REG_YOUT_UPPER:
> > +	case MXC4005_REG_YOUT_LOWER:
> > +	case MXC4005_REG_ZOUT_UPPER:
> > +	case MXC4005_REG_ZOUT_LOWER:
> > +	case MXC4005_REG_DEVICE_ID:
> > +	case MXC4005_REG_CONTROL:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> > +static bool mxc4005_is_writeable_reg(struct device *dev, unsigned int reg)
> > +{
> > +	switch (reg) {
> > +	case MXC4005_REG_CONTROL:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> > +static const struct regmap_config mxc4005_regmap_config = {
> > +	.name = MXC4005_DRV_NAME,
> > +
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +
> > +	.max_register = MXC4005_REG_DEVICE_ID,
> > +	.cache_type = REGCACHE_NONE,
> REGCACHE_NONE is the default. No need to specify it here.
> (enum value of 0)

Ok. Will remove this.

> > +
> > +	.readable_reg = mxc4005_is_readable_reg,
> > +	.writeable_reg = mxc4005_is_writeable_reg,
> > +};
> > +
> > +static int mxc4005_read_axis(struct mxc4005_data *data,
> > +			     unsigned int addr)
> > +{
> > +	__be16 reg;
> > +	int ret;
> > +
> > +	ret = regmap_bulk_read(data->regmap, addr, (u8 *) &reg, sizeof(reg));
> > +	if (ret < 0) {
> > +		dev_err(&data->client->dev, "failed to read reg %02x\n", addr);
> > +		return ret;
> > +	}
> > +
> > +	return be16_to_cpu(reg);
> > +}
> > +
> > +static int mxc4005_read_scale(struct mxc4005_data *data)
> > +{
> > +	unsigned int reg;
> > +	int ret;
> > +	int i;
> > +
> > +	ret = regmap_read(data->regmap, MXC4005_REG_CONTROL, &reg);
> > +	if (ret < 0) {
> > +		dev_err(&data->client->dev, "failed to read reg_control\n");
> > +		return ret;
> > +	}
> > +
> > +	i = reg >> MXC4005_CONTROL_FSR_SHIFT;
> > +
> > +	if (i < 0 || i >= ARRAY_SIZE(mxc4005_scale_table))
> > +		return -EINVAL;
> > +
> > +	return mxc4005_scale_table[i].scale;
> > +}
> > +
> > +static int mxc4005_set_scale(struct mxc4005_data *data, int val)
> > +{
> > +	unsigned int reg;
> > +	int i;
> > +	int ret;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(mxc4005_scale_table); i++) {
> > +		if (mxc4005_scale_table[i].scale == val) {
> > +			reg = i << MXC4005_CONTROL_FSR_SHIFT;
> > +			ret = regmap_update_bits(data->regmap,
> > +						 MXC4005_REG_CONTROL,
> > +						 MXC4005_REG_CONTROL_MASK_FSR,
> > +						 reg);
> > +			if (ret < 0)
> > +				dev_err(&data->client->dev,
> > +					"failed to write reg_control\n");
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int mxc4005_read_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *chan,
> > +			    int *val, int *val2, long mask)
> > +{
> > +	struct mxc4005_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		switch (chan->type) {
> > +		case IIO_ACCEL:
> > +			if (iio_buffer_enabled(indio_dev))
> > +				return -EBUSY;
> > +
> > +			ret = mxc4005_read_axis(data, chan->address);
> > +			if (ret < 0)
> > +				return ret;
> > +			*val = sign_extend32(ret >> 4, 11);
> > +			return IIO_VAL_INT;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	case IIO_CHAN_INFO_SCALE:
> > +		ret = mxc4005_read_scale(data);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		*val = 0;
> > +		*val2 = ret;
> > +		return IIO_VAL_INT_PLUS_MICRO;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int mxc4005_write_raw(struct iio_dev *indio_dev,
> > +			     struct iio_chan_spec const *chan,
> > +			     int val, int val2, long mask)
> > +{
> > +	struct mxc4005_data *data = iio_priv(indio_dev);
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_SCALE:
> > +		if (val != 0)
> > +			return -EINVAL;
> > +
> > +		return mxc4005_set_scale(data, val2);
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static const struct iio_info mxc4005_info = {
> > +	.driver_module	= THIS_MODULE,
> > +	.read_raw	= mxc4005_read_raw,
> > +	.write_raw	= mxc4005_write_raw,
> > +	.attrs		= &mxc4005_attrs_group,
> > +};
> > +
> > +#define MXC4005_CHANNEL(_axis, _addr) {				\
> > +	.type = IIO_ACCEL,					\
> > +	.modified = 1,						\
> > +	.channel2 = IIO_MOD_##_axis,				\
> > +	.address = _addr,					\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> > +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> > +}
> > +
> > +static const struct iio_chan_spec mxc4005_channels[] = {
> > +	MXC4005_CHANNEL(X, MXC4005_REG_XOUT_UPPER),
> > +	MXC4005_CHANNEL(Y, MXC4005_REG_YOUT_UPPER),
> > +	MXC4005_CHANNEL(Z, MXC4005_REG_ZOUT_UPPER),
> > +};
> > +
> > +static int mxc4005_chip_init(struct mxc4005_data *data)
> > +{
> > +	int ret;
> > +	unsigned int reg;
> > +
> > +	ret = regmap_read(data->regmap, MXC4005_REG_DEVICE_ID, &reg);
> > +	if (ret < 0) {
> > +		dev_err(&data->client->dev, "failed to read chip id\n");
> > +		return ret;
> > +	}
> > +
> > +	dev_dbg(&data->client->dev, "MXC4005 chip id %02x\n", reg);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mxc4005_probe(struct i2c_client *client,
> > +			 const struct i2c_device_id *id)
> > +{
> > +	struct mxc4005_data *data;
> > +	struct iio_dev *indio_dev;
> > +	struct regmap *regmap;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	regmap = devm_regmap_init_i2c(client, &mxc4005_regmap_config);
> > +	if (IS_ERR(regmap)) {
> > +		dev_err(&client->dev, "failed to initialize regmap\n");
> > +		return PTR_ERR(regmap);
> > +	}
> > +
> > +	data = iio_priv(indio_dev);
> > +	i2c_set_clientdata(client, indio_dev);
> > +	data->client = client;
> > +	data->regmap = regmap;
> > +
> > +	ret = mxc4005_chip_init(data);
> > +	if (ret < 0) {
> For consistency with previous error message (yeah I'm getting
> picky ;) - initialize

Yes, true. :)

> > +		dev_err(&client->dev, "failed to init chip\n");
> > +		return ret;
> > +	}
> > +
> > +	mutex_init(&data->mutex);
> > +
> > +	indio_dev->dev.parent = &client->dev;
> > +	indio_dev->channels = mxc4005_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(mxc4005_channels);
> > +	indio_dev->name = MXC4005_DRV_NAME;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->info = &mxc4005_info;
> > +
> > +	ret = iio_device_register(indio_dev);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev,
> > +			"unable to register iio device %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int mxc4005_remove(struct i2c_client *client)
> > +{
> > +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > +
> Roll the line above and the one below into one.  No point in the
> separation.

Ok.

> > +	iio_device_unregister(indio_dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct acpi_device_id mxc4005_acpi_match[] = {
> > +	{"MXC4005",	0},
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(acpi, mxc4005_acpi_match);
> > +
> > +static const struct i2c_device_id mxc4005_id[] = {
> > +	{"mxc4005",	0},
> > +	{ },
> > +};
> For consistency no blank line here.

Ok.

> > +
> > +MODULE_DEVICE_TABLE(i2c, mxc4005_id);
> > +
> > +static struct i2c_driver mxc4005_driver = {
> > +	.driver = {
> > +		.name = MXC4005_DRV_NAME,
> > +		.acpi_match_table = ACPI_PTR(mxc4005_acpi_match),
> > +	},
> > +	.probe		= mxc4005_probe,
> > +	.remove		= mxc4005_remove,
> > +	.id_table	= mxc4005_id,
> > +};
> > +
> > +module_i2c_driver(mxc4005_driver);
> > +
> > +MODULE_AUTHOR("Teodora Baluta <teodora.baluta@intel.com>");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("MXC4005 3-axis accelerometer driver");
> > 
> 
> --
> 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 v2 2/3] iio: mxc4005: add triggered buffer mode for mxc4005

[Teodora Baluta]

This patch adds support for buffered readings for the 3-axis
accelerometer mxc4005.

Signed-off-by: Teodora Baluta <teodora.baluta@intel.com>
---
 drivers/iio/accel/Kconfig   |  2 ++
 drivers/iio/accel/mxc4005.c | 75 +++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index 69302be..cd5cd24 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -140,6 +140,8 @@ config MMA9553
 config MXC4005
 	tristate "Memsic MXC4005XC 3-Axis Accelerometer Driver"
 	depends on I2C
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
 	select REGMAP_I2C
 	help
 	  Say yes here to build support for the Memsic MXC4005XC 3-axis
diff --git a/drivers/iio/accel/mxc4005.c b/drivers/iio/accel/mxc4005.c
index 76a7954..5f4813d 100644
--- a/drivers/iio/accel/mxc4005.c
+++ b/drivers/iio/accel/mxc4005.c
@@ -19,6 +19,9 @@
 #include <linux/acpi.h>
 #include <linux/regmap.h>
 #include <linux/iio/sysfs.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
 
 #define MXC4005_DRV_NAME		"mxc4005"
 #define MXC4005_REGMAP_NAME		"mxc4005_regmap"
@@ -52,6 +55,7 @@ struct mxc4005_data {
 	struct i2c_client *client;
 	struct mutex mutex;
 	struct regmap *regmap;
+	__be16 buffer[3];
 };
 
 /*
@@ -123,6 +127,20 @@ static const struct regmap_config mxc4005_regmap_config = {
 	.writeable_reg = mxc4005_is_writeable_reg,
 };
 
+static int mxc4005_read_xyz(struct mxc4005_data *data)
+{
+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, MXC4005_REG_XOUT_UPPER,
+			       (u8 *) data->buffer, sizeof(data->buffer));
+	if (ret < 0) {
+		dev_err(&data->client->dev, "failed to read axes\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 static int mxc4005_read_axis(struct mxc4005_data *data,
 			     unsigned int addr)
 {
@@ -198,7 +216,8 @@ static int mxc4005_read_raw(struct iio_dev *indio_dev,
 			ret = mxc4005_read_axis(data, chan->address);
 			if (ret < 0)
 				return ret;
-			*val = sign_extend32(ret >> 4, 11);
+			*val = sign_extend32(ret >> chan->scan_type.shift,
+					     chan->scan_type.realbits - 1);
 			return IIO_VAL_INT;
 		default:
 			return -EINVAL;
@@ -240,6 +259,11 @@ static const struct iio_info mxc4005_info = {
 	.attrs		= &mxc4005_attrs_group,
 };
 
+static const unsigned long mxc4005_scan_masks[] = {
+	BIT(AXIS_X) | BIT(AXIS_Y) | BIT(AXIS_Z),
+	0
+};
+
 #define MXC4005_CHANNEL(_axis, _addr) {				\
 	.type = IIO_ACCEL,					\
 	.modified = 1,						\
@@ -247,14 +271,43 @@ static const struct iio_info mxc4005_info = {
 	.address = _addr,					\
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+	.scan_index = AXIS_##_axis,				\
+	.scan_type = {						\
+		.sign = 's',					\
+		.realbits = 12,					\
+		.storagebits = 16,				\
+		.shift = 4,					\
+		.endianness = IIO_BE,				\
+	},							\
 }
 
 static const struct iio_chan_spec mxc4005_channels[] = {
 	MXC4005_CHANNEL(X, MXC4005_REG_XOUT_UPPER),
 	MXC4005_CHANNEL(Y, MXC4005_REG_YOUT_UPPER),
 	MXC4005_CHANNEL(Z, MXC4005_REG_ZOUT_UPPER),
+	IIO_CHAN_SOFT_TIMESTAMP(3),
 };
 
+static irqreturn_t mxc4005_trigger_handler(int irq, void *private)
+{
+	struct iio_poll_func *pf = private;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct mxc4005_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = mxc4005_read_xyz(data);
+	if (ret < 0)
+		goto err;
+
+	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
+					   iio_get_time_ns());
+
+err:
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
 static int mxc4005_chip_init(struct mxc4005_data *data)
 {
 	int ret;
@@ -305,18 +358,34 @@ static int mxc4005_probe(struct i2c_client *client,
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->channels = mxc4005_channels;
 	indio_dev->num_channels = ARRAY_SIZE(mxc4005_channels);
+	indio_dev->available_scan_masks = mxc4005_scan_masks;
 	indio_dev->name = MXC4005_DRV_NAME;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &mxc4005_info;
 
+	ret = iio_triggered_buffer_setup(indio_dev,
+					 &iio_pollfunc_store_time,
+					 mxc4005_trigger_handler,
+					 NULL);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"failed to setup iio triggered buffer\n");
+		return ret;
+	}
+
 	ret = iio_device_register(indio_dev);
 	if (ret < 0) {
 		dev_err(&client->dev,
 			"unable to register iio device %d\n", ret);
-		return ret;
+		goto err_buffer_cleanup;
 	}
 
 	return 0;
+
+err_buffer_cleanup:
+	iio_triggered_buffer_cleanup(indio_dev);
+
+	return ret;
 }
 
 static int mxc4005_remove(struct i2c_client *client)
@@ -325,6 +394,8 @@ static int mxc4005_remove(struct i2c_client *client)
 
 	iio_device_unregister(indio_dev);
 
+	iio_triggered_buffer_cleanup(indio_dev);
+
 	return 0;
 }
 
-- 
1.9.1

Re: [PATCH v2 2/3] iio: mxc4005: add triggered buffer mode for mxc4005

[Jonathan Cameron]

On 13/08/15 18:31, Teodora Baluta wrote:
> This patch adds support for buffered readings for the 3-axis
> accelerometer mxc4005.
> 
> Signed-off-by: Teodora Baluta <teodora.baluta@intel.com>
One big one (buffer isn't big enough) in here.
Otherwise, looking good.

Jonathan
> ---
>  drivers/iio/accel/Kconfig   |  2 ++
>  drivers/iio/accel/mxc4005.c | 75 +++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 75 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> index 69302be..cd5cd24 100644
> --- a/drivers/iio/accel/Kconfig
> +++ b/drivers/iio/accel/Kconfig
> @@ -140,6 +140,8 @@ config MMA9553
>  config MXC4005
>  	tristate "Memsic MXC4005XC 3-Axis Accelerometer Driver"
>  	depends on I2C
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
>  	select REGMAP_I2C
>  	help
>  	  Say yes here to build support for the Memsic MXC4005XC 3-axis
> diff --git a/drivers/iio/accel/mxc4005.c b/drivers/iio/accel/mxc4005.c
> index 76a7954..5f4813d 100644
> --- a/drivers/iio/accel/mxc4005.c
> +++ b/drivers/iio/accel/mxc4005.c
> @@ -19,6 +19,9 @@
>  #include <linux/acpi.h>
>  #include <linux/regmap.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
>  
>  #define MXC4005_DRV_NAME		"mxc4005"
>  #define MXC4005_REGMAP_NAME		"mxc4005_regmap"
> @@ -52,6 +55,7 @@ struct mxc4005_data {
>  	struct i2c_client *client;
>  	struct mutex mutex;
>  	struct regmap *regmap;
> +	__be16 buffer[3];
Not big enough as needs to have space for the timestamp as well.
(noticing this does require reading the fine print for
iio_push_to_buffers_with_timestamp).

Should be 3*2 bytes + 8 bytes (aligned) so will be 16 bytes, or 8 __be16s.

>  };
>  
>  /*
> @@ -123,6 +127,20 @@ static const struct regmap_config mxc4005_regmap_config = {
>  	.writeable_reg = mxc4005_is_writeable_reg,
>  };
>  
> +static int mxc4005_read_xyz(struct mxc4005_data *data)
> +{
> +	int ret;
> +
> +	ret = regmap_bulk_read(data->regmap, MXC4005_REG_XOUT_UPPER,
> +			       (u8 *) data->buffer, sizeof(data->buffer));
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "failed to read axes\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int mxc4005_read_axis(struct mxc4005_data *data,
>  			     unsigned int addr)
>  {
> @@ -198,7 +216,8 @@ static int mxc4005_read_raw(struct iio_dev *indio_dev,
>  			ret = mxc4005_read_axis(data, chan->address);
>  			if (ret < 0)
>  				return ret;
> -			*val = sign_extend32(ret >> 4, 11);
> +			*val = sign_extend32(ret >> chan->scan_type.shift,
> +					     chan->scan_type.realbits - 1);
>  			return IIO_VAL_INT;
>  		default:
>  			return -EINVAL;
> @@ -240,6 +259,11 @@ static const struct iio_info mxc4005_info = {
>  	.attrs		= &mxc4005_attrs_group,
>  };
>  
> +static const unsigned long mxc4005_scan_masks[] = {
> +	BIT(AXIS_X) | BIT(AXIS_Y) | BIT(AXIS_Z),
> +	0
> +};
> +
>  #define MXC4005_CHANNEL(_axis, _addr) {				\
>  	.type = IIO_ACCEL,					\
>  	.modified = 1,						\
> @@ -247,14 +271,43 @@ static const struct iio_info mxc4005_info = {
>  	.address = _addr,					\
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
>  	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +	.scan_index = AXIS_##_axis,				\
> +	.scan_type = {						\
> +		.sign = 's',					\
> +		.realbits = 12,					\
> +		.storagebits = 16,				\
> +		.shift = 4,					\
> +		.endianness = IIO_BE,				\
> +	},							\
>  }
>  
>  static const struct iio_chan_spec mxc4005_channels[] = {
>  	MXC4005_CHANNEL(X, MXC4005_REG_XOUT_UPPER),
>  	MXC4005_CHANNEL(Y, MXC4005_REG_YOUT_UPPER),
>  	MXC4005_CHANNEL(Z, MXC4005_REG_ZOUT_UPPER),
> +	IIO_CHAN_SOFT_TIMESTAMP(3),
>  };
>  
> +static irqreturn_t mxc4005_trigger_handler(int irq, void *private)
> +{
> +	struct iio_poll_func *pf = private;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct mxc4005_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = mxc4005_read_xyz(data);
> +	if (ret < 0)
> +		goto err;
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> +					   iio_get_time_ns());
> +
> +err:
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int mxc4005_chip_init(struct mxc4005_data *data)
>  {
>  	int ret;
> @@ -305,18 +358,34 @@ static int mxc4005_probe(struct i2c_client *client,
>  	indio_dev->dev.parent = &client->dev;
>  	indio_dev->channels = mxc4005_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(mxc4005_channels);
> +	indio_dev->available_scan_masks = mxc4005_scan_masks;
>  	indio_dev->name = MXC4005_DRV_NAME;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->info = &mxc4005_info;
>  
> +	ret = iio_triggered_buffer_setup(indio_dev,
> +					 &iio_pollfunc_store_time,
> +					 mxc4005_trigger_handler,
> +					 NULL);
> +	if (ret < 0) {
> +		dev_err(&client->dev,
> +			"failed to setup iio triggered buffer\n");
> +		return ret;
> +	}
> +
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0) {
>  		dev_err(&client->dev,
>  			"unable to register iio device %d\n", ret);
> -		return ret;
> +		goto err_buffer_cleanup;
>  	}
>  
>  	return 0;
> +
> +err_buffer_cleanup:
> +	iio_triggered_buffer_cleanup(indio_dev);
> +
> +	return ret;
>  }
>  
>  static int mxc4005_remove(struct i2c_client *client)
> @@ -325,6 +394,8 @@ static int mxc4005_remove(struct i2c_client *client)
>  
>  	iio_device_unregister(indio_dev);
>  
> +	iio_triggered_buffer_cleanup(indio_dev);
> +
>  	return 0;
>  }
>  
> 

Re: [PATCH v2 2/3] iio: mxc4005: add triggered buffer mode for mxc4005

[Teodora Baluta]

On Sun, Aug 16, 2015 at 08:37:52AM +0100, Jonathan Cameron wrote:
> On 13/08/15 18:31, Teodora Baluta wrote:
> > This patch adds support for buffered readings for the 3-axis
> > accelerometer mxc4005.
> > 
> > Signed-off-by: Teodora Baluta <teodora.baluta@intel.com>
> One big one (buffer isn't big enough) in here.
> Otherwise, looking good.

Yes, thanks. :)

> 
> Jonathan
> > ---
> >  drivers/iio/accel/Kconfig   |  2 ++
> >  drivers/iio/accel/mxc4005.c | 75 +++++++++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 75 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> > index 69302be..cd5cd24 100644
> > --- a/drivers/iio/accel/Kconfig
> > +++ b/drivers/iio/accel/Kconfig
> > @@ -140,6 +140,8 @@ config MMA9553
> >  config MXC4005
> >  	tristate "Memsic MXC4005XC 3-Axis Accelerometer Driver"
> >  	depends on I2C
> > +	select IIO_BUFFER
> > +	select IIO_TRIGGERED_BUFFER
> >  	select REGMAP_I2C
> >  	help
> >  	  Say yes here to build support for the Memsic MXC4005XC 3-axis
> > diff --git a/drivers/iio/accel/mxc4005.c b/drivers/iio/accel/mxc4005.c
> > index 76a7954..5f4813d 100644
> > --- a/drivers/iio/accel/mxc4005.c
> > +++ b/drivers/iio/accel/mxc4005.c
> > @@ -19,6 +19,9 @@
> >  #include <linux/acpi.h>
> >  #include <linux/regmap.h>
> >  #include <linux/iio/sysfs.h>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +#include <linux/iio/trigger_consumer.h>
> >  
> >  #define MXC4005_DRV_NAME		"mxc4005"
> >  #define MXC4005_REGMAP_NAME		"mxc4005_regmap"
> > @@ -52,6 +55,7 @@ struct mxc4005_data {
> >  	struct i2c_client *client;
> >  	struct mutex mutex;
> >  	struct regmap *regmap;
> > +	__be16 buffer[3];
> Not big enough as needs to have space for the timestamp as well.
> (noticing this does require reading the fine print for
> iio_push_to_buffers_with_timestamp).
> 
> Should be 3*2 bytes + 8 bytes (aligned) so will be 16 bytes, or 8 __be16s.
> 
> >  };
> >  
> >  /*
> > @@ -123,6 +127,20 @@ static const struct regmap_config mxc4005_regmap_config = {
> >  	.writeable_reg = mxc4005_is_writeable_reg,
> >  };
> >  
> > +static int mxc4005_read_xyz(struct mxc4005_data *data)
> > +{
> > +	int ret;
> > +
> > +	ret = regmap_bulk_read(data->regmap, MXC4005_REG_XOUT_UPPER,
> > +			       (u8 *) data->buffer, sizeof(data->buffer));
> > +	if (ret < 0) {
> > +		dev_err(&data->client->dev, "failed to read axes\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int mxc4005_read_axis(struct mxc4005_data *data,
> >  			     unsigned int addr)
> >  {
> > @@ -198,7 +216,8 @@ static int mxc4005_read_raw(struct iio_dev *indio_dev,
> >  			ret = mxc4005_read_axis(data, chan->address);
> >  			if (ret < 0)
> >  				return ret;
> > -			*val = sign_extend32(ret >> 4, 11);
> > +			*val = sign_extend32(ret >> chan->scan_type.shift,
> > +					     chan->scan_type.realbits - 1);
> >  			return IIO_VAL_INT;
> >  		default:
> >  			return -EINVAL;
> > @@ -240,6 +259,11 @@ static const struct iio_info mxc4005_info = {
> >  	.attrs		= &mxc4005_attrs_group,
> >  };
> >  
> > +static const unsigned long mxc4005_scan_masks[] = {
> > +	BIT(AXIS_X) | BIT(AXIS_Y) | BIT(AXIS_Z),
> > +	0
> > +};
> > +
> >  #define MXC4005_CHANNEL(_axis, _addr) {				\
> >  	.type = IIO_ACCEL,					\
> >  	.modified = 1,						\
> > @@ -247,14 +271,43 @@ static const struct iio_info mxc4005_info = {
> >  	.address = _addr,					\
> >  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> >  	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> > +	.scan_index = AXIS_##_axis,				\
> > +	.scan_type = {						\
> > +		.sign = 's',					\
> > +		.realbits = 12,					\
> > +		.storagebits = 16,				\
> > +		.shift = 4,					\
> > +		.endianness = IIO_BE,				\
> > +	},							\
> >  }
> >  
> >  static const struct iio_chan_spec mxc4005_channels[] = {
> >  	MXC4005_CHANNEL(X, MXC4005_REG_XOUT_UPPER),
> >  	MXC4005_CHANNEL(Y, MXC4005_REG_YOUT_UPPER),
> >  	MXC4005_CHANNEL(Z, MXC4005_REG_ZOUT_UPPER),
> > +	IIO_CHAN_SOFT_TIMESTAMP(3),
> >  };
> >  
> > +static irqreturn_t mxc4005_trigger_handler(int irq, void *private)
> > +{
> > +	struct iio_poll_func *pf = private;
> > +	struct iio_dev *indio_dev = pf->indio_dev;
> > +	struct mxc4005_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	ret = mxc4005_read_xyz(data);
> > +	if (ret < 0)
> > +		goto err;
> > +
> > +	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> > +					   iio_get_time_ns());
> > +
> > +err:
> > +	iio_trigger_notify_done(indio_dev->trig);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  static int mxc4005_chip_init(struct mxc4005_data *data)
> >  {
> >  	int ret;
> > @@ -305,18 +358,34 @@ static int mxc4005_probe(struct i2c_client *client,
> >  	indio_dev->dev.parent = &client->dev;
> >  	indio_dev->channels = mxc4005_channels;
> >  	indio_dev->num_channels = ARRAY_SIZE(mxc4005_channels);
> > +	indio_dev->available_scan_masks = mxc4005_scan_masks;
> >  	indio_dev->name = MXC4005_DRV_NAME;
> >  	indio_dev->modes = INDIO_DIRECT_MODE;
> >  	indio_dev->info = &mxc4005_info;
> >  
> > +	ret = iio_triggered_buffer_setup(indio_dev,
> > +					 &iio_pollfunc_store_time,
> > +					 mxc4005_trigger_handler,
> > +					 NULL);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev,
> > +			"failed to setup iio triggered buffer\n");
> > +		return ret;
> > +	}
> > +
> >  	ret = iio_device_register(indio_dev);
> >  	if (ret < 0) {
> >  		dev_err(&client->dev,
> >  			"unable to register iio device %d\n", ret);
> > -		return ret;
> > +		goto err_buffer_cleanup;
> >  	}
> >  
> >  	return 0;
> > +
> > +err_buffer_cleanup:
> > +	iio_triggered_buffer_cleanup(indio_dev);
> > +
> > +	return ret;
> >  }
> >  
> >  static int mxc4005_remove(struct i2c_client *client)
> > @@ -325,6 +394,8 @@ static int mxc4005_remove(struct i2c_client *client)
> >  
> >  	iio_device_unregister(indio_dev);
> >  
> > +	iio_triggered_buffer_cleanup(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

[PATCH v2 3/3] iio: mxc4005: add data ready trigger for mxc4005

[Teodora Baluta]

Add iio trigger for the data ready interrupt that signals new
measurements for the X, Y and Z axes.

Signed-off-by: Teodora Baluta <teodora.baluta@intel.com>
---
 drivers/iio/accel/mxc4005.c | 159 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 158 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/accel/mxc4005.c b/drivers/iio/accel/mxc4005.c
index 5f4813d..0c47bfe 100644
--- a/drivers/iio/accel/mxc4005.c
+++ b/drivers/iio/accel/mxc4005.c
@@ -17,13 +17,16 @@
 #include <linux/i2c.h>
 #include <linux/iio/iio.h>
 #include <linux/acpi.h>
+#include <linux/gpio/consumer.h>
 #include <linux/regmap.h>
 #include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
 #include <linux/iio/buffer.h>
 #include <linux/iio/triggered_buffer.h>
 #include <linux/iio/trigger_consumer.h>
 
 #define MXC4005_DRV_NAME		"mxc4005"
+#define MXC4005_IRQ_NAME		"mxc4005_event"
 #define MXC4005_REGMAP_NAME		"mxc4005_regmap"
 
 #define MXC4005_REG_XOUT_UPPER		0x03
@@ -33,6 +36,15 @@
 #define MXC4005_REG_ZOUT_UPPER		0x07
 #define MXC4005_REG_ZOUT_LOWER		0x08
 
+#define MXC4005_REG_INT_SRC1		0x01
+#define MXC4005_REG_INT_SRC2_BIT_DRDY	0x01
+
+#define MXC4005_REG_INT_MASK1		0x0B
+#define MXC4005_REG_INT_MASK1_BIT_DRDYE	0x01
+
+#define MXC4005_REG_INT_CLR1		0x01
+#define MXC4005_REG_INT_CLR1_BIT_DRDYC	0x01
+
 #define MXC4005_REG_CONTROL		0x0D
 #define MXC4005_REG_CONTROL_MASK_FSR	GENMASK(6, 5)
 #define MXC4005_CONTROL_FSR_SHIFT	5
@@ -55,7 +67,10 @@ struct mxc4005_data {
 	struct i2c_client *client;
 	struct mutex mutex;
 	struct regmap *regmap;
+	struct iio_trigger *dready_trig;
+	int64_t timestamp;
 	__be16 buffer[3];
+	bool trigger_enabled;
 };
 
 /*
@@ -97,6 +112,7 @@ static bool mxc4005_is_readable_reg(struct device *dev, unsigned int reg)
 	case MXC4005_REG_ZOUT_UPPER:
 	case MXC4005_REG_ZOUT_LOWER:
 	case MXC4005_REG_DEVICE_ID:
+	case MXC4005_REG_INT_SRC1:
 	case MXC4005_REG_CONTROL:
 		return true;
 	default:
@@ -107,6 +123,8 @@ static bool mxc4005_is_readable_reg(struct device *dev, unsigned int reg)
 static bool mxc4005_is_writeable_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
+	case MXC4005_REG_INT_CLR1:
+	case MXC4005_REG_INT_MASK1:
 	case MXC4005_REG_CONTROL:
 		return true;
 	default:
@@ -300,7 +318,7 @@ static irqreturn_t mxc4005_trigger_handler(int irq, void *private)
 		goto err;
 
 	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
-					   iio_get_time_ns());
+					   data->timestamp);
 
 err:
 	iio_trigger_notify_done(indio_dev->trig);
@@ -308,6 +326,103 @@ err:
 	return IRQ_HANDLED;
 }
 
+static int mxc4005_set_trigger_state(struct iio_trigger *trig,
+				     bool state)
+{
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+	struct mxc4005_data *data = iio_priv(indio_dev);
+	int ret;
+
+	mutex_lock(&data->mutex);
+	if (state) {
+		ret = regmap_write(data->regmap, MXC4005_REG_INT_MASK1,
+				   MXC4005_REG_INT_MASK1_BIT_DRDYE);
+	} else {
+		ret = regmap_write(data->regmap, MXC4005_REG_INT_MASK1,
+				   ~MXC4005_REG_INT_MASK1_BIT_DRDYE);
+	}
+
+	if (ret < 0) {
+		mutex_unlock(&data->mutex);
+		dev_err(&data->client->dev,
+			"failed to update reg_int_mask1");
+		return ret;
+	}
+
+	data->trigger_enabled = state;
+	mutex_unlock(&data->mutex);
+
+	return 0;
+}
+
+static const struct iio_trigger_ops mxc4005_trigger_ops = {
+	.set_trigger_state = mxc4005_set_trigger_state,
+	.owner = THIS_MODULE,
+};
+
+static irqreturn_t mxc4005_irq_thrd_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct mxc4005_data *data = iio_priv(indio_dev);
+	unsigned int reg;
+	int ret;
+
+	ret = regmap_read(data->regmap, MXC4005_REG_INT_SRC1, &reg);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "failed to read reg_int_src1\n");
+		goto exit;
+	}
+
+	/* clear interrupt */
+	ret = regmap_write(data->regmap, MXC4005_REG_INT_CLR1,
+			   MXC4005_REG_INT_CLR1_BIT_DRDYC);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "failed to write to reg_int_clr1\n");
+		goto exit;
+	}
+
+exit:
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t mxc4005_irq_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct mxc4005_data *data = iio_priv(indio_dev);
+
+	data->timestamp = iio_get_time_ns();
+
+	if (data->trigger_enabled)
+		iio_trigger_poll(data->dready_trig);
+
+	return IRQ_WAKE_THREAD;
+}
+
+static int mxc4005_gpio_probe(struct i2c_client *client,
+			      struct mxc4005_data *data)
+{
+	struct device *dev;
+	struct gpio_desc *gpio;
+	int ret;
+
+	if (!client)
+		return -EINVAL;
+
+	dev = &client->dev;
+
+	gpio = devm_gpiod_get_index(dev, "mxc4005_int", 0, GPIOD_IN);
+	if (IS_ERR(gpio)) {
+		dev_err(dev, "failed to get acpi gpio index\n");
+		return PTR_ERR(gpio);
+	}
+
+	ret = gpiod_to_irq(gpio);
+
+	dev_dbg(dev, "GPIO resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);
+
+	return ret;
+}
+
 static int mxc4005_chip_init(struct mxc4005_data *data)
 {
 	int ret;
@@ -373,6 +488,43 @@ static int mxc4005_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	if (client->irq < 0)
+		client->irq = mxc4005_gpio_probe(client, data);
+
+	if (client->irq >= 0) {
+		ret = devm_request_threaded_irq(&client->dev, client->irq,
+						mxc4005_irq_handler,
+						mxc4005_irq_thrd_handler,
+						IRQF_TRIGGER_FALLING |
+						IRQF_ONESHOT,
+						MXC4005_IRQ_NAME,
+						indio_dev);
+		if (ret) {
+			dev_err(&client->dev,
+				"failed to init threaded irq\n");
+			goto err_buffer_cleanup;
+		}
+
+		data->dready_trig = devm_iio_trigger_alloc(&client->dev,
+							   "%s-dev%d",
+							   indio_dev->name,
+							   indio_dev->id);
+		if (!data->dready_trig)
+			return -ENOMEM;
+
+		data->dready_trig->dev.parent = &client->dev;
+		data->dready_trig->ops = &mxc4005_trigger_ops;
+		iio_trigger_set_drvdata(data->dready_trig, indio_dev);
+		indio_dev->trig = data->dready_trig;
+		iio_trigger_get(indio_dev->trig);
+		ret = iio_trigger_register(data->dready_trig);
+		if (ret) {
+			dev_err(&client->dev,
+				"failed to register trigger\n");
+			goto err_trigger_unregister;
+		}
+	}
+
 	ret = iio_device_register(indio_dev);
 	if (ret < 0) {
 		dev_err(&client->dev,
@@ -382,6 +534,8 @@ static int mxc4005_probe(struct i2c_client *client,
 
 	return 0;
 
+err_trigger_unregister:
+	iio_trigger_unregister(data->dready_trig);
 err_buffer_cleanup:
 	iio_triggered_buffer_cleanup(indio_dev);
 
@@ -391,10 +545,13 @@ err_buffer_cleanup:
 static int mxc4005_remove(struct i2c_client *client)
 {
 	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct mxc4005_data *data = iio_priv(indio_dev);
 
 	iio_device_unregister(indio_dev);
 
 	iio_triggered_buffer_cleanup(indio_dev);
+	if (data->dready_trig)
+		iio_trigger_unregister(data->dready_trig);
 
 	return 0;
 }
-- 
1.9.1

Re: [PATCH v2 3/3] iio: mxc4005: add data ready trigger for mxc4005

[Jonathan Cameron]

On 13/08/15 18:31, Teodora Baluta wrote:
> Add iio trigger for the data ready interrupt that signals new
> measurements for the X, Y and Z axes.
> 
> Signed-off-by: Teodora Baluta <teodora.baluta@intel.com>
Various bits inline.

Jonathan
> ---
>  drivers/iio/accel/mxc4005.c | 159 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 158 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/mxc4005.c b/drivers/iio/accel/mxc4005.c
> index 5f4813d..0c47bfe 100644
> --- a/drivers/iio/accel/mxc4005.c
> +++ b/drivers/iio/accel/mxc4005.c
> @@ -17,13 +17,16 @@
>  #include <linux/i2c.h>
>  #include <linux/iio/iio.h>
>  #include <linux/acpi.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/regmap.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/triggered_buffer.h>
>  #include <linux/iio/trigger_consumer.h>
>  
>  #define MXC4005_DRV_NAME		"mxc4005"
> +#define MXC4005_IRQ_NAME		"mxc4005_event"
>  #define MXC4005_REGMAP_NAME		"mxc4005_regmap"
>  
>  #define MXC4005_REG_XOUT_UPPER		0x03
> @@ -33,6 +36,15 @@
>  #define MXC4005_REG_ZOUT_UPPER		0x07
>  #define MXC4005_REG_ZOUT_LOWER		0x08
>  
> +#define MXC4005_REG_INT_SRC1		0x01
> +#define MXC4005_REG_INT_SRC2_BIT_DRDY	0x01
> +
> +#define MXC4005_REG_INT_MASK1		0x0B
> +#define MXC4005_REG_INT_MASK1_BIT_DRDYE	0x01
> +
> +#define MXC4005_REG_INT_CLR1		0x01
> +#define MXC4005_REG_INT_CLR1_BIT_DRDYC	0x01
> +
>  #define MXC4005_REG_CONTROL		0x0D
>  #define MXC4005_REG_CONTROL_MASK_FSR	GENMASK(6, 5)
>  #define MXC4005_CONTROL_FSR_SHIFT	5
> @@ -55,7 +67,10 @@ struct mxc4005_data {
>  	struct i2c_client *client;
>  	struct mutex mutex;
>  	struct regmap *regmap;
> +	struct iio_trigger *dready_trig;

As mentioned below, use the pf->timestamp or you'll get in a mess if for
example you driver the capture from this via a slow sysfs trigger.
> +	int64_t timestamp;
>  	__be16 buffer[3];
> +	bool trigger_enabled;
>  };
>  
>  /*
> @@ -97,6 +112,7 @@ static bool mxc4005_is_readable_reg(struct device *dev, unsigned int reg)
>  	case MXC4005_REG_ZOUT_UPPER:
>  	case MXC4005_REG_ZOUT_LOWER:
>  	case MXC4005_REG_DEVICE_ID:
> +	case MXC4005_REG_INT_SRC1:
>  	case MXC4005_REG_CONTROL:
>  		return true;
>  	default:
> @@ -107,6 +123,8 @@ static bool mxc4005_is_readable_reg(struct device *dev, unsigned int reg)
>  static bool mxc4005_is_writeable_reg(struct device *dev, unsigned int reg)
>  {
>  	switch (reg) {
> +	case MXC4005_REG_INT_CLR1:
> +	case MXC4005_REG_INT_MASK1:
>  	case MXC4005_REG_CONTROL:
>  		return true;
>  	default:
> @@ -300,7 +318,7 @@ static irqreturn_t mxc4005_trigger_handler(int irq, void *private)
>  		goto err;
>  
>  	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> -					   iio_get_time_ns());
> +					   data->timestamp);
Who said this was being triggered by the devices own trigger?  If
you want to do this, then use pull the timestamp save in the pollfunc
top half. (iio_pollfunc_store_time is available for this).

>  
>  err:
>  	iio_trigger_notify_done(indio_dev->trig);
> @@ -308,6 +326,103 @@ err:
>  	return IRQ_HANDLED;
>  }
>  
> +static int mxc4005_set_trigger_state(struct iio_trigger *trig,
> +				     bool state)
> +{
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct mxc4005_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&data->mutex);
> +	if (state) {
> +		ret = regmap_write(data->regmap, MXC4005_REG_INT_MASK1,
> +				   MXC4005_REG_INT_MASK1_BIT_DRDYE);
> +	} else {
> +		ret = regmap_write(data->regmap, MXC4005_REG_INT_MASK1,
> +				   ~MXC4005_REG_INT_MASK1_BIT_DRDYE);
> +	}
> +
> +	if (ret < 0) {
> +		mutex_unlock(&data->mutex);
> +		dev_err(&data->client->dev,
> +			"failed to update reg_int_mask1");
> +		return ret;
> +	}
> +
> +	data->trigger_enabled = state;
> +	mutex_unlock(&data->mutex);
> +
> +	return 0;
> +}
> +
> +static const struct iio_trigger_ops mxc4005_trigger_ops = {
> +	.set_trigger_state = mxc4005_set_trigger_state,
> +	.owner = THIS_MODULE,
> +};
> +
> +static irqreturn_t mxc4005_irq_thrd_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct mxc4005_data *data = iio_priv(indio_dev);
> +	unsigned int reg;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, MXC4005_REG_INT_SRC1, &reg);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "failed to read reg_int_src1\n");
> +		goto exit;
> +	}
Guessing you have to read the interrupt source to get it clear?  If so
please comment it.  If not, don't bother reading it :)
> +
> +	/* clear interrupt */
> +	ret = regmap_write(data->regmap, MXC4005_REG_INT_CLR1,
> +			   MXC4005_REG_INT_CLR1_BIT_DRDYC);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "failed to write to reg_int_clr1\n");
> +		goto exit;
> +	}

This is what the try_to_reenable callback is meant for.  That will clear
the interrupt, only when all child interrupts are done (i.e. all
triggers consumers are ready for the next one).  Here you clear it immediately
and hence could end up feeding interrupts into that system faster than they
can be handled (though the masking that occurs should prevent that actually
causing too much trouble).

> +
> +exit:
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t mxc4005_irq_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct mxc4005_data *data = iio_priv(indio_dev);
> +
> +	data->timestamp = iio_get_time_ns();
> +
Right now you don't need this as this is the only supported interrupt.
I'm guessing you have some events to come though?
> +	if (data->trigger_enabled)
> +		iio_trigger_poll(data->dready_trig);
> +
Unusual to have a thread handler here when only one interrupt type is possible.
> +	return IRQ_WAKE_THREAD;
> +}
> +
> +static int mxc4005_gpio_probe(struct i2c_client *client,
> +			      struct mxc4005_data *data)
> +{
> +	struct device *dev;
> +	struct gpio_desc *gpio;
> +	int ret;
> +
> +	if (!client)
> +		return -EINVAL;
> +
> +	dev = &client->dev;
> +
> +	gpio = devm_gpiod_get_index(dev, "mxc4005_int", 0, GPIOD_IN);
> +	if (IS_ERR(gpio)) {
> +		dev_err(dev, "failed to get acpi gpio index\n");
> +		return PTR_ERR(gpio);
> +	}
> +
> +	ret = gpiod_to_irq(gpio);
> +
> +	dev_dbg(dev, "GPIO resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);
> +
> +	return ret;
> +}
> +
>  static int mxc4005_chip_init(struct mxc4005_data *data)
>  {
>  	int ret;
> @@ -373,6 +488,43 @@ static int mxc4005_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> +	if (client->irq < 0)
> +		client->irq = mxc4005_gpio_probe(client, data);
> +
> +	if (client->irq >= 0) {
Should be > 0.

IRQ of 0 is nolonger considered valid (used to indicate a specified not
connected IRQ which isn't going to be useful here!)

Had a patch set correcting all remaining cases of this in IIO the other
day.  Quite a few years ago now, IRQ 0 was valid on arm, but that was
changed.

> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> +						mxc4005_irq_handler,
> +						mxc4005_irq_thrd_handler,
> +						IRQF_TRIGGER_FALLING |
> +						IRQF_ONESHOT,
> +						MXC4005_IRQ_NAME,
> +						indio_dev);
> +		if (ret) {
> +			dev_err(&client->dev,
> +				"failed to init threaded irq\n");
> +			goto err_buffer_cleanup;
> +		}
> +
> +		data->dready_trig = devm_iio_trigger_alloc(&client->dev,
> +							   "%s-dev%d",
> +							   indio_dev->name,
> +							   indio_dev->id);
> +		if (!data->dready_trig)
> +			return -ENOMEM;
> +
> +		data->dready_trig->dev.parent = &client->dev;
> +		data->dready_trig->ops = &mxc4005_trigger_ops;
> +		iio_trigger_set_drvdata(data->dready_trig, indio_dev);
> +		indio_dev->trig = data->dready_trig;
> +		iio_trigger_get(indio_dev->trig);
> +		ret = iio_trigger_register(data->dready_trig);
> +		if (ret) {
> +			dev_err(&client->dev,
> +				"failed to register trigger\n");
> +			goto err_trigger_unregister;
> +		}
> +	}
> +
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0) {
>  		dev_err(&client->dev,
> @@ -382,6 +534,8 @@ static int mxc4005_probe(struct i2c_client *client,
>  
>  	return 0;
>  
> +err_trigger_unregister:
> +	iio_trigger_unregister(data->dready_trig);
>  err_buffer_cleanup:
>  	iio_triggered_buffer_cleanup(indio_dev);
>  
> @@ -391,10 +545,13 @@ err_buffer_cleanup:
>  static int mxc4005_remove(struct i2c_client *client)
>  {
>  	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct mxc4005_data *data = iio_priv(indio_dev);
>  
>  	iio_device_unregister(indio_dev);
>  
>  	iio_triggered_buffer_cleanup(indio_dev);
> +	if (data->dready_trig)
> +		iio_trigger_unregister(data->dready_trig);
>  
>  	return 0;
>  }
> 

Re: [PATCH v2 3/3] iio: mxc4005: add data ready trigger for mxc4005

[Teodora Baluta]

On Sun, Aug 16, 2015 at 09:01:39AM +0100, Jonathan Cameron wrote:
> On 13/08/15 18:31, Teodora Baluta wrote:
> > Add iio trigger for the data ready interrupt that signals new
> > measurements for the X, Y and Z axes.
> > 
> > Signed-off-by: Teodora Baluta <teodora.baluta@intel.com>
> Various bits inline.

Thanks for the review!

> 
> Jonathan
> > ---
> >  drivers/iio/accel/mxc4005.c | 159 +++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 158 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iio/accel/mxc4005.c b/drivers/iio/accel/mxc4005.c
> > index 5f4813d..0c47bfe 100644
> > --- a/drivers/iio/accel/mxc4005.c
> > +++ b/drivers/iio/accel/mxc4005.c
> > @@ -17,13 +17,16 @@
> >  #include <linux/i2c.h>
> >  #include <linux/iio/iio.h>
> >  #include <linux/acpi.h>
> > +#include <linux/gpio/consumer.h>
> >  #include <linux/regmap.h>
> >  #include <linux/iio/sysfs.h>
> > +#include <linux/iio/trigger.h>
> >  #include <linux/iio/buffer.h>
> >  #include <linux/iio/triggered_buffer.h>
> >  #include <linux/iio/trigger_consumer.h>
> >  
> >  #define MXC4005_DRV_NAME		"mxc4005"
> > +#define MXC4005_IRQ_NAME		"mxc4005_event"
> >  #define MXC4005_REGMAP_NAME		"mxc4005_regmap"
> >  
> >  #define MXC4005_REG_XOUT_UPPER		0x03
> > @@ -33,6 +36,15 @@
> >  #define MXC4005_REG_ZOUT_UPPER		0x07
> >  #define MXC4005_REG_ZOUT_LOWER		0x08
> >  
> > +#define MXC4005_REG_INT_SRC1		0x01
> > +#define MXC4005_REG_INT_SRC2_BIT_DRDY	0x01
> > +
> > +#define MXC4005_REG_INT_MASK1		0x0B
> > +#define MXC4005_REG_INT_MASK1_BIT_DRDYE	0x01
> > +
> > +#define MXC4005_REG_INT_CLR1		0x01
> > +#define MXC4005_REG_INT_CLR1_BIT_DRDYC	0x01
> > +
> >  #define MXC4005_REG_CONTROL		0x0D
> >  #define MXC4005_REG_CONTROL_MASK_FSR	GENMASK(6, 5)
> >  #define MXC4005_CONTROL_FSR_SHIFT	5
> > @@ -55,7 +67,10 @@ struct mxc4005_data {
> >  	struct i2c_client *client;
> >  	struct mutex mutex;
> >  	struct regmap *regmap;
> > +	struct iio_trigger *dready_trig;
> 
> As mentioned below, use the pf->timestamp or you'll get in a mess if for
> example you driver the capture from this via a slow sysfs trigger.
> > +	int64_t timestamp;
> >  	__be16 buffer[3];
> > +	bool trigger_enabled;
> >  };
> >  
> >  /*
> > @@ -97,6 +112,7 @@ static bool mxc4005_is_readable_reg(struct device *dev, unsigned int reg)
> >  	case MXC4005_REG_ZOUT_UPPER:
> >  	case MXC4005_REG_ZOUT_LOWER:
> >  	case MXC4005_REG_DEVICE_ID:
> > +	case MXC4005_REG_INT_SRC1:
> >  	case MXC4005_REG_CONTROL:
> >  		return true;
> >  	default:
> > @@ -107,6 +123,8 @@ static bool mxc4005_is_readable_reg(struct device *dev, unsigned int reg)
> >  static bool mxc4005_is_writeable_reg(struct device *dev, unsigned int reg)
> >  {
> >  	switch (reg) {
> > +	case MXC4005_REG_INT_CLR1:
> > +	case MXC4005_REG_INT_MASK1:
> >  	case MXC4005_REG_CONTROL:
> >  		return true;
> >  	default:
> > @@ -300,7 +318,7 @@ static irqreturn_t mxc4005_trigger_handler(int irq, void *private)
> >  		goto err;
> >  
> >  	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> > -					   iio_get_time_ns());
> > +					   data->timestamp);
> Who said this was being triggered by the devices own trigger?  If
> you want to do this, then use pull the timestamp save in the pollfunc
> top half. (iio_pollfunc_store_time is available for this).
> 

Ok, thanks!

> >  
> >  err:
> >  	iio_trigger_notify_done(indio_dev->trig);
> > @@ -308,6 +326,103 @@ err:
> >  	return IRQ_HANDLED;
> >  }
> >  
> > +static int mxc4005_set_trigger_state(struct iio_trigger *trig,
> > +				     bool state)
> > +{
> > +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> > +	struct mxc4005_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	mutex_lock(&data->mutex);
> > +	if (state) {
> > +		ret = regmap_write(data->regmap, MXC4005_REG_INT_MASK1,
> > +				   MXC4005_REG_INT_MASK1_BIT_DRDYE);
> > +	} else {
> > +		ret = regmap_write(data->regmap, MXC4005_REG_INT_MASK1,
> > +				   ~MXC4005_REG_INT_MASK1_BIT_DRDYE);
> > +	}
> > +
> > +	if (ret < 0) {
> > +		mutex_unlock(&data->mutex);
> > +		dev_err(&data->client->dev,
> > +			"failed to update reg_int_mask1");
> > +		return ret;
> > +	}
> > +
> > +	data->trigger_enabled = state;
> > +	mutex_unlock(&data->mutex);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct iio_trigger_ops mxc4005_trigger_ops = {
> > +	.set_trigger_state = mxc4005_set_trigger_state,
> > +	.owner = THIS_MODULE,
> > +};
> > +
> > +static irqreturn_t mxc4005_irq_thrd_handler(int irq, void *private)
> > +{
> > +	struct iio_dev *indio_dev = private;
> > +	struct mxc4005_data *data = iio_priv(indio_dev);
> > +	unsigned int reg;
> > +	int ret;
> > +
> > +	ret = regmap_read(data->regmap, MXC4005_REG_INT_SRC1, &reg);
> > +	if (ret < 0) {
> > +		dev_err(&data->client->dev, "failed to read reg_int_src1\n");
> > +		goto exit;
> > +	}
> Guessing you have to read the interrupt source to get it clear?  If so
> please comment it.  If not, don't bother reading it :)

The datasheet says that just reading the interrupt source doesn't clear
the interrupt but I'll have to test this. There could be other interrupt
sources - the device has support for shake and tilt events. I guess when
support for these is added it will make sense to read the INT_SRCx
registers.

> > +
> > +	/* clear interrupt */
> > +	ret = regmap_write(data->regmap, MXC4005_REG_INT_CLR1,
> > +			   MXC4005_REG_INT_CLR1_BIT_DRDYC);
> > +	if (ret < 0) {
> > +		dev_err(&data->client->dev, "failed to write to reg_int_clr1\n");
> > +		goto exit;
> > +	}
> 
> This is what the try_to_reenable callback is meant for.  That will clear
> the interrupt, only when all child interrupts are done (i.e. all
> triggers consumers are ready for the next one).  Here you clear it immediately
> and hence could end up feeding interrupts into that system faster than they
> can be handled (though the masking that occurs should prevent that actually
> causing too much trouble).
> 

I'll look into this and fix it.

> > +
> > +exit:
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t mxc4005_irq_handler(int irq, void *private)
> > +{
> > +	struct iio_dev *indio_dev = private;
> > +	struct mxc4005_data *data = iio_priv(indio_dev);
> > +
> > +	data->timestamp = iio_get_time_ns();
> > +
> Right now you don't need this as this is the only supported interrupt.
> I'm guessing you have some events to come though?
> > +	if (data->trigger_enabled)
> > +		iio_trigger_poll(data->dready_trig);
> > +
> Unusual to have a thread handler here when only one interrupt type is possible.

There are the shake, tilt functions of the device which make sense to be
events, but I am not going to add these anytime soon so you're right.
At this level, I don't think the handler is necessary. I'll rework this
code as per your suggestion.

> > +	return IRQ_WAKE_THREAD;
> > +}
> > +
> > +static int mxc4005_gpio_probe(struct i2c_client *client,
> > +			      struct mxc4005_data *data)
> > +{
> > +	struct device *dev;
> > +	struct gpio_desc *gpio;
> > +	int ret;
> > +
> > +	if (!client)
> > +		return -EINVAL;
> > +
> > +	dev = &client->dev;
> > +
> > +	gpio = devm_gpiod_get_index(dev, "mxc4005_int", 0, GPIOD_IN);
> > +	if (IS_ERR(gpio)) {
> > +		dev_err(dev, "failed to get acpi gpio index\n");
> > +		return PTR_ERR(gpio);
> > +	}
> > +
> > +	ret = gpiod_to_irq(gpio);
> > +
> > +	dev_dbg(dev, "GPIO resource, no:%d irq:%d\n", desc_to_gpio(gpio), ret);
> > +
> > +	return ret;
> > +}
> > +
> >  static int mxc4005_chip_init(struct mxc4005_data *data)
> >  {
> >  	int ret;
> > @@ -373,6 +488,43 @@ static int mxc4005_probe(struct i2c_client *client,
> >  		return ret;
> >  	}
> >  
> > +	if (client->irq < 0)
> > +		client->irq = mxc4005_gpio_probe(client, data);
> > +
> > +	if (client->irq >= 0) {
> Should be > 0.
> 
> IRQ of 0 is nolonger considered valid (used to indicate a specified not
> connected IRQ which isn't going to be useful here!)
> 
> Had a patch set correcting all remaining cases of this in IIO the other
> day.  Quite a few years ago now, IRQ 0 was valid on arm, but that was
> changed.
> 

Ok.

> > +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> > +						mxc4005_irq_handler,
> > +						mxc4005_irq_thrd_handler,
> > +						IRQF_TRIGGER_FALLING |
> > +						IRQF_ONESHOT,
> > +						MXC4005_IRQ_NAME,
> > +						indio_dev);
> > +		if (ret) {
> > +			dev_err(&client->dev,
> > +				"failed to init threaded irq\n");
> > +			goto err_buffer_cleanup;
> > +		}
> > +
> > +		data->dready_trig = devm_iio_trigger_alloc(&client->dev,
> > +							   "%s-dev%d",
> > +							   indio_dev->name,
> > +							   indio_dev->id);
> > +		if (!data->dready_trig)
> > +			return -ENOMEM;
> > +
> > +		data->dready_trig->dev.parent = &client->dev;
> > +		data->dready_trig->ops = &mxc4005_trigger_ops;
> > +		iio_trigger_set_drvdata(data->dready_trig, indio_dev);
> > +		indio_dev->trig = data->dready_trig;
> > +		iio_trigger_get(indio_dev->trig);
> > +		ret = iio_trigger_register(data->dready_trig);
> > +		if (ret) {
> > +			dev_err(&client->dev,
> > +				"failed to register trigger\n");
> > +			goto err_trigger_unregister;
> > +		}
> > +	}
> > +
> >  	ret = iio_device_register(indio_dev);
> >  	if (ret < 0) {
> >  		dev_err(&client->dev,
> > @@ -382,6 +534,8 @@ static int mxc4005_probe(struct i2c_client *client,
> >  
> >  	return 0;
> >  
> > +err_trigger_unregister:
> > +	iio_trigger_unregister(data->dready_trig);
> >  err_buffer_cleanup:
> >  	iio_triggered_buffer_cleanup(indio_dev);
> >  
> > @@ -391,10 +545,13 @@ err_buffer_cleanup:
> >  static int mxc4005_remove(struct i2c_client *client)
> >  {
> >  	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > +	struct mxc4005_data *data = iio_priv(indio_dev);
> >  
> >  	iio_device_unregister(indio_dev);
> >  
> >  	iio_triggered_buffer_cleanup(indio_dev);
> > +	if (data->dready_trig)
> > +		iio_trigger_unregister(data->dready_trig);
> >  
> >  	return 0;
> >  }
> > 
>