[PATCH v3 0/2] Add support for the Bosch BMA220 accelerometer

[PATCH v3 0/2] Add support for the Bosch BMA220 accelerometer

[Tiberiu Breana]

This patch set adds support for the Bosch Sensortec BMA220 accelerometer.
Patch 1 adds basic support, raw readings and ACPI detection.
Patch 2 adds triggered buffer support.

v3:
Added link to datasheet.

v2 change log:
patch 1:
- addressed Peter's comments
- added the bma220_init function that handles chip id
  checking and waking up the device at probing time

patch 2:
- addressed Peter's comments

Tiberiu Breana (2):
  iio: accel: Add support for Bosch BMA220
  iio: accel: Add triggered buffer support for BMA220

 drivers/iio/accel/Kconfig      |  10 ++
 drivers/iio/accel/Makefile     |   1 +
 drivers/iio/accel/bma220_spi.c | 319 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 330 insertions(+)
 create mode 100644 drivers/iio/accel/bma220_spi.c

-- 
1.9.1

[PATCH v3 1/2] iio: accel: Add support for Bosch BMA220

[Tiberiu Breana]

This commit adds basic support for the Bosch Sensortec BMA220
digital triaxial acceleration sensor.
The device datasheet can be found here:

http://www.mouser.com/pdfdocs/BSTBMA220DS00308.PDF

Includes:
- raw readings
- ACPI detection
- power management

Signed-off-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
---
 drivers/iio/accel/Kconfig      |  10 ++
 drivers/iio/accel/Makefile     |   1 +
 drivers/iio/accel/bma220_spi.c | 257 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 268 insertions(+)
 create mode 100644 drivers/iio/accel/bma220_spi.c

diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index e4a758c..98740c7 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -17,6 +17,16 @@ config BMA180
 	  To compile this driver as a module, choose M here: the
 	  module will be called bma180.
 
+config BMA220
+    tristate "Bosch BMA220 3-Axis Accelerometer Driver"
+	depends on SPI
+    help
+      Say yes here to add support for the Bosch BMA220 triaxial
+      acceleration sensor.
+
+      To compile this driver as a module, choose M here: the
+      module will be called bma220_spi.
+
 config BMC150_ACCEL
 	tristate "Bosch BMC150 Accelerometer Driver"
 	select IIO_BUFFER
diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
index 71b6794..9ae7820e 100644
--- a/drivers/iio/accel/Makefile
+++ b/drivers/iio/accel/Makefile
@@ -4,6 +4,7 @@
 
 # When adding new entries keep the list in alphabetical order
 obj-$(CONFIG_BMA180) += bma180.o
+obj-$(CONFIG_BMA220) += bma220_spi.o
 obj-$(CONFIG_BMC150_ACCEL) += bmc150-accel-core.o
 obj-$(CONFIG_BMC150_ACCEL_I2C) += bmc150-accel-i2c.o
 obj-$(CONFIG_BMC150_ACCEL_SPI) += bmc150-accel-spi.o
diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
new file mode 100644
index 0000000..5df5594
--- /dev/null
+++ b/drivers/iio/accel/bma220_spi.c
@@ -0,0 +1,257 @@
+/**
+ * BMA220 Digital triaxial acceleration sensor driver
+ *
+ * Copyright (c) 2016, Intel Corporation.
+ *
+ * This file is subject to the terms and conditions of version 2 of
+ * the GNU General Public License. See the file COPYING in the main
+ * directory of this archive for more details.
+ */
+
+#include <linux/acpi.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/spi/spi.h>
+
+#define BMA220_REG_ID				0x00
+#define BMA220_REG_ACCEL_X			0x02
+#define BMA220_REG_ACCEL_Y			0x03
+#define BMA220_REG_ACCEL_Z			0x04
+#define BMA220_REG_RANGE			0x11
+#define BMA220_REG_SUSPEND			0x18
+
+#define BMA220_CHIP_ID				0xDD
+#define BMA220_READ_MASK			0x80
+#define BMA220_RANGE_MASK			0x03
+#define BMA220_DATA_SHIFT			2
+#define BMA220_SUSPEND_SLEEP			0xFF
+#define BMA220_SUSPEND_WAKE			0x00
+
+#define BMA220_DEVICE_NAME			"bma220"
+#define BMA220_SCALE_AVAILABLE			"0.623 1.248 2.491 4.983"
+
+#define BMA220_ACCEL_CHANNEL(index, reg, axis) {			\
+	.type = IIO_ACCEL,						\
+	.address = reg,							\
+	.modified = 1,							\
+	.channel2 = IIO_MOD_##axis,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
+}
+
+static IIO_CONST_ATTR(in_accel_scale_available, BMA220_SCALE_AVAILABLE);
+
+static struct attribute *bma220_attributes[] = {
+	&iio_const_attr_in_accel_scale_available.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group bma220_attribute_group = {
+	.attrs = bma220_attributes,
+};
+
+static const int bma220_scale_table[][4] = {
+	{0, 623000}, {1, 248000}, {2, 491000}, {4, 983000}
+};
+
+struct bma220_data {
+	struct spi_device *spi_device;
+};
+
+static const struct iio_chan_spec bma220_channels[] = {
+	BMA220_ACCEL_CHANNEL(0, BMA220_REG_ACCEL_X, X),
+	BMA220_ACCEL_CHANNEL(1, BMA220_REG_ACCEL_Y, Y),
+	BMA220_ACCEL_CHANNEL(2, BMA220_REG_ACCEL_Z, Z),
+};
+
+static inline int bma220_read_reg(struct spi_device *spi, u8 reg)
+{
+	return spi_w8r8(spi, reg | BMA220_READ_MASK);
+}
+
+static int bma220_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	int ret;
+	u8 range_idx;
+	struct bma220_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = bma220_read_reg(data->spi_device, chan->address);
+		if (ret < 0)
+			return -EINVAL;
+		*val = sign_extend32(ret >> BMA220_DATA_SHIFT, 5);
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		ret = bma220_read_reg(data->spi_device, BMA220_REG_RANGE);
+		if (ret < 0)
+			return ret;
+		range_idx = ret & BMA220_RANGE_MASK;
+		*val = bma220_scale_table[range_idx][0];
+		*val2 = bma220_scale_table[range_idx][1];
+		return IIO_VAL_INT_PLUS_MICRO;
+	}
+
+	return -EINVAL;
+}
+
+static int bma220_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long mask)
+{
+	int i;
+	int ret;
+	u8 buf[2];
+	int index = -1;
+	struct bma220_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		for (i = 0; i < ARRAY_SIZE(bma220_scale_table); i++)
+			if (val == bma220_scale_table[i][0] &&
+			    val2 == bma220_scale_table[i][1]) {
+				index = i;
+				break;
+			}
+		if (index < 0)
+			return -EINVAL;
+
+		buf[0] = BMA220_REG_RANGE;
+		buf[1] = index;
+		ret = spi_write(data->spi_device, &buf, sizeof(buf));
+		if (ret < 0)
+			dev_err(&data->spi_device->dev,
+				"failed to set measurement range\n");
+
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static const struct iio_info bma220_info = {
+	.driver_module		= THIS_MODULE,
+	.read_raw		= bma220_read_raw,
+	.write_raw		= bma220_write_raw,
+	.attrs			= &bma220_attribute_group,
+};
+
+static int bma220_init(struct spi_device *spi)
+{
+	int ret;
+
+	ret = bma220_read_reg(spi, BMA220_REG_ID);
+	if (ret != BMA220_CHIP_ID)
+		return -ENODEV;
+
+	/* Make sure the chip is powered on */
+	ret = bma220_read_reg(spi, BMA220_REG_SUSPEND);
+	if (ret < 0)
+		return ret;
+	else if (ret == BMA220_SUSPEND_WAKE)
+		return bma220_read_reg(spi, BMA220_REG_SUSPEND);
+
+	return 0;
+}
+
+static int bma220_probe(struct spi_device *spi)
+{
+	int ret;
+	struct iio_dev *indio_dev;
+	struct bma220_data *data;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*data));
+	if (!indio_dev) {
+		dev_err(&spi->dev, "iio allocation failed!\n");
+		return -ENOMEM;
+	}
+
+	data = iio_priv(indio_dev);
+	data->spi_device = spi;
+	spi_set_drvdata(spi, indio_dev);
+
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->info = &bma220_info;
+	indio_dev->name = BMA220_DEVICE_NAME;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = bma220_channels;
+	indio_dev->num_channels = ARRAY_SIZE(bma220_channels);
+
+	ret = bma220_init(data->spi_device);
+	if (ret < 0)
+		return ret;
+
+	ret = iio_device_register(indio_dev);
+	if (ret < 0)
+		dev_err(&spi->dev, "iio_device_register failed\n");
+
+	return ret;
+}
+
+static int bma220_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+	struct bma220_data *data = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+
+	return bma220_read_reg(data->spi_device, BMA220_REG_SUSPEND);
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int bma220_suspend(struct device *dev)
+{
+	struct bma220_data *data =
+			iio_priv(spi_get_drvdata(to_spi_device(dev)));
+
+	/* The chip can be suspended/woken up by a simple register read. */
+	return bma220_read_reg(data->spi_device, BMA220_REG_SUSPEND);
+}
+
+static int bma220_resume(struct device *dev)
+{
+	struct bma220_data *data =
+			iio_priv(spi_get_drvdata(to_spi_device(dev)));
+
+	return bma220_read_reg(data->spi_device, BMA220_REG_SUSPEND);
+}
+
+static SIMPLE_DEV_PM_OPS(bma220_pm_ops, bma220_suspend, bma220_resume);
+
+#define BMA220_PM_OPS (&bma220_pm_ops)
+#else
+#define BMA220_PM_OPS NULL
+#endif
+
+static const struct spi_device_id bma220_spi_id[] = {
+	{"bma220", 0},
+	{}
+};
+
+static const struct acpi_device_id bma220_acpi_id[] = {
+	{"BMA0220", 0},
+	{}
+};
+
+MODULE_DEVICE_TABLE(spi, bma220_spi_id);
+
+static struct spi_driver bma220_driver = {
+	.driver = {
+		.name = "bma220_spi",
+		.pm = BMA220_PM_OPS,
+		.acpi_match_table = ACPI_PTR(bma220_acpi_id),
+	},
+	.probe =            bma220_probe,
+	.remove =           bma220_remove,
+	.id_table =         bma220_spi_id,
+};
+
+module_spi_driver(bma220_driver);
+
+MODULE_AUTHOR("Tiberiu Breana <tiberiu.a.breana@intel.com>");
+MODULE_DESCRIPTION("BMA220 acceleration sensor driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [PATCH v3 1/2] iio: accel: Add support for Bosch BMA220

[Jonathan Cameron]

On 29/04/16 14:32, Tiberiu Breana wrote:
> This commit adds basic support for the Bosch Sensortec BMA220
> digital triaxial acceleration sensor.
> The device datasheet can be found here:
> 
> http://www.mouser.com/pdfdocs/BSTBMA220DS00308.PDF
> 
> Includes:
> - raw readings
> - ACPI detection
> - power management
> 
> Signed-off-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
Mainly a case of fixing the possible dma cacheline corruption.
> ---
>  drivers/iio/accel/Kconfig      |  10 ++
>  drivers/iio/accel/Makefile     |   1 +
>  drivers/iio/accel/bma220_spi.c | 257 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 268 insertions(+)
>  create mode 100644 drivers/iio/accel/bma220_spi.c
> 
> diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> index e4a758c..98740c7 100644
> --- a/drivers/iio/accel/Kconfig
> +++ b/drivers/iio/accel/Kconfig
> @@ -17,6 +17,16 @@ config BMA180
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called bma180.
>  
> +config BMA220
> +    tristate "Bosch BMA220 3-Axis Accelerometer Driver"
> +	depends on SPI
> +    help
> +      Say yes here to add support for the Bosch BMA220 triaxial
> +      acceleration sensor.
> +
> +      To compile this driver as a module, choose M here: the
> +      module will be called bma220_spi.
> +
>  config BMC150_ACCEL
>  	tristate "Bosch BMC150 Accelerometer Driver"
>  	select IIO_BUFFER
> diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
> index 71b6794..9ae7820e 100644
> --- a/drivers/iio/accel/Makefile
> +++ b/drivers/iio/accel/Makefile
> @@ -4,6 +4,7 @@
>  
>  # When adding new entries keep the list in alphabetical order
>  obj-$(CONFIG_BMA180) += bma180.o
> +obj-$(CONFIG_BMA220) += bma220_spi.o
>  obj-$(CONFIG_BMC150_ACCEL) += bmc150-accel-core.o
>  obj-$(CONFIG_BMC150_ACCEL_I2C) += bmc150-accel-i2c.o
>  obj-$(CONFIG_BMC150_ACCEL_SPI) += bmc150-accel-spi.o
> diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
> new file mode 100644
> index 0000000..5df5594
> --- /dev/null
> +++ b/drivers/iio/accel/bma220_spi.c
> @@ -0,0 +1,257 @@
> +/**
> + * BMA220 Digital triaxial acceleration sensor driver
> + *
> + * Copyright (c) 2016, Intel Corporation.
> + *
> + * This file is subject to the terms and conditions of version 2 of
> + * the GNU General Public License. See the file COPYING in the main
> + * directory of this archive for more details.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/spi/spi.h>
> +
> +#define BMA220_REG_ID				0x00
> +#define BMA220_REG_ACCEL_X			0x02
> +#define BMA220_REG_ACCEL_Y			0x03
> +#define BMA220_REG_ACCEL_Z			0x04
> +#define BMA220_REG_RANGE			0x11
> +#define BMA220_REG_SUSPEND			0x18
> +
> +#define BMA220_CHIP_ID				0xDD
> +#define BMA220_READ_MASK			0x80
> +#define BMA220_RANGE_MASK			0x03
> +#define BMA220_DATA_SHIFT			2
> +#define BMA220_SUSPEND_SLEEP			0xFF
> +#define BMA220_SUSPEND_WAKE			0x00
> +
> +#define BMA220_DEVICE_NAME			"bma220"
> +#define BMA220_SCALE_AVAILABLE			"0.623 1.248 2.491 4.983"
> +
> +#define BMA220_ACCEL_CHANNEL(index, reg, axis) {			\
> +	.type = IIO_ACCEL,						\
> +	.address = reg,							\
> +	.modified = 1,							\
> +	.channel2 = IIO_MOD_##axis,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
> +}
> +
> +static IIO_CONST_ATTR(in_accel_scale_available, BMA220_SCALE_AVAILABLE);
> +
> +static struct attribute *bma220_attributes[] = {
> +	&iio_const_attr_in_accel_scale_available.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group bma220_attribute_group = {
> +	.attrs = bma220_attributes,
> +};
> +
> +static const int bma220_scale_table[][4] = {
> +	{0, 623000}, {1, 248000}, {2, 491000}, {4, 983000}
> +};
> +
> +struct bma220_data {
> +	struct spi_device *spi_device;
> +};
> +
> +static const struct iio_chan_spec bma220_channels[] = {
> +	BMA220_ACCEL_CHANNEL(0, BMA220_REG_ACCEL_X, X),
> +	BMA220_ACCEL_CHANNEL(1, BMA220_REG_ACCEL_Y, Y),
> +	BMA220_ACCEL_CHANNEL(2, BMA220_REG_ACCEL_Z, Z),
> +};
> +
> +static inline int bma220_read_reg(struct spi_device *spi, u8 reg)
> +{
> +	return spi_w8r8(spi, reg | BMA220_READ_MASK);
> +}
> +
> +static int bma220_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	int ret;
> +	u8 range_idx;
> +	struct bma220_data *data = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = bma220_read_reg(data->spi_device, chan->address);
> +		if (ret < 0)
> +			return -EINVAL;
> +		*val = sign_extend32(ret >> BMA220_DATA_SHIFT, 5);
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = bma220_read_reg(data->spi_device, BMA220_REG_RANGE);
> +		if (ret < 0)
> +			return ret;
> +		range_idx = ret & BMA220_RANGE_MASK;
> +		*val = bma220_scale_table[range_idx][0];
> +		*val2 = bma220_scale_table[range_idx][1];
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int bma220_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	int i;
> +	int ret;
> +	u8 buf[2];
The whole cacheline alignment thing needs sorting...
> +	int index = -1;
> +	struct bma220_data *data = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		for (i = 0; i < ARRAY_SIZE(bma220_scale_table); i++)
> +			if (val == bma220_scale_table[i][0] &&
> +			    val2 == bma220_scale_table[i][1]) {
> +				index = i;
> +				break;
> +			}
> +		if (index < 0)
> +			return -EINVAL;
> +
> +		buf[0] = BMA220_REG_RANGE;
> +		buf[1] = index;
> +		ret = spi_write(data->spi_device, &buf, sizeof(buf));
> +		if (ret < 0)
> +			dev_err(&data->spi_device->dev,
> +				"failed to set measurement range\n");
> +
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct iio_info bma220_info = {
> +	.driver_module		= THIS_MODULE,
> +	.read_raw		= bma220_read_raw,
> +	.write_raw		= bma220_write_raw,
> +	.attrs			= &bma220_attribute_group,
> +};
> +
> +static int bma220_init(struct spi_device *spi)
> +{
> +	int ret;
> +
> +	ret = bma220_read_reg(spi, BMA220_REG_ID);
> +	if (ret != BMA220_CHIP_ID)
> +		return -ENODEV;
> +
> +	/* Make sure the chip is powered on */
> +	ret = bma220_read_reg(spi, BMA220_REG_SUSPEND);
> +	if (ret < 0)
> +		return ret;
> +	else if (ret == BMA220_SUSPEND_WAKE)
> +		return bma220_read_reg(spi, BMA220_REG_SUSPEND);
> +
> +	return 0;
> +}
> +
> +static int bma220_probe(struct spi_device *spi)
> +{
> +	int ret;
> +	struct iio_dev *indio_dev;
> +	struct bma220_data *data;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*data));
> +	if (!indio_dev) {
> +		dev_err(&spi->dev, "iio allocation failed!\n");
> +		return -ENOMEM;
> +	}
> +
> +	data = iio_priv(indio_dev);
> +	data->spi_device = spi;
> +	spi_set_drvdata(spi, indio_dev);
> +
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->info = &bma220_info;
> +	indio_dev->name = BMA220_DEVICE_NAME;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = bma220_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(bma220_channels);
> +
> +	ret = bma220_init(data->spi_device);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret < 0)
> +		dev_err(&spi->dev, "iio_device_register failed\n");
> +
> +	return ret;
> +}
> +
> +static int bma220_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct bma220_data *data = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +
> +	return bma220_read_reg(data->spi_device, BMA220_REG_SUSPEND);
I'd have a slight preference here for a wrapper to match against  _init
as then it would be clear that you 'unsuspend' in there which took me a
moment to notice.  Gives us a nice matched pair of functions rather than
a seemingly unmatched suspend...
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int bma220_suspend(struct device *dev)
> +{
> +	struct bma220_data *data =
> +			iio_priv(spi_get_drvdata(to_spi_device(dev)));
> +
> +	/* The chip can be suspended/woken up by a simple register read. */
Weird, you would have thought a write would have taken no more hardware and been
slightly more intuitive.
> +	return bma220_read_reg(data->spi_device, BMA220_REG_SUSPEND);
> +}
> +
> +static int bma220_resume(struct device *dev)
> +{
> +	struct bma220_data *data =
> +			iio_priv(spi_get_drvdata(to_spi_device(dev)));
> +
> +	return bma220_read_reg(data->spi_device, BMA220_REG_SUSPEND);
> +}
> +
> +static SIMPLE_DEV_PM_OPS(bma220_pm_ops, bma220_suspend, bma220_resume);
> +
> +#define BMA220_PM_OPS (&bma220_pm_ops)
> +#else
> +#define BMA220_PM_OPS NULL
> +#endif
> +
> +static const struct spi_device_id bma220_spi_id[] = {
> +	{"bma220", 0},
> +	{}
> +};
> +
> +static const struct acpi_device_id bma220_acpi_id[] = {
> +	{"BMA0220", 0},
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(spi, bma220_spi_id);
> +
> +static struct spi_driver bma220_driver = {
> +	.driver = {
> +		.name = "bma220_spi",
> +		.pm = BMA220_PM_OPS,
> +		.acpi_match_table = ACPI_PTR(bma220_acpi_id),
> +	},
> +	.probe =            bma220_probe,
> +	.remove =           bma220_remove,
> +	.id_table =         bma220_spi_id,
> +};
> +
> +module_spi_driver(bma220_driver);
> +
> +MODULE_AUTHOR("Tiberiu Breana <tiberiu.a.breana@intel.com>");
> +MODULE_DESCRIPTION("BMA220 acceleration sensor driver");
> +MODULE_LICENSE("GPL v2");
> 

[PATCH v3 2/2] iio: accel: Add triggered buffer support for BMA220

[Tiberiu Breana]

Signed-off-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
---
 drivers/iio/accel/bma220_spi.c | 66 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 64 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
index 5df5594..cc305aa 100644
--- a/drivers/iio/accel/bma220_spi.c
+++ b/drivers/iio/accel/bma220_spi.c
@@ -11,9 +11,12 @@
 #include <linux/acpi.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/iio/buffer.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/spi/spi.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
 
 #define BMA220_REG_ID				0x00
 #define BMA220_REG_ACCEL_X			0x02
@@ -39,6 +42,14 @@
 	.channel2 = IIO_MOD_##axis,					\
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
+	.scan_index = index,						\
+	.scan_type = {							\
+		.sign = 's',						\
+		.realbits = 6,						\
+		.storagebits = 8,					\
+		.shift = BMA220_DATA_SHIFT,				\
+		.endianness = IIO_CPU,					\
+	},								\
 }
 
 static IIO_CONST_ATTR(in_accel_scale_available, BMA220_SCALE_AVAILABLE);
@@ -58,12 +69,16 @@ static const int bma220_scale_table[][4] = {
 
 struct bma220_data {
 	struct spi_device *spi_device;
+	struct mutex lock;
+	s8 buffer[16]; /* 3x8-bit channels + 5x8 padding + 8x8 timestamp */
+
 };
 
 static const struct iio_chan_spec bma220_channels[] = {
 	BMA220_ACCEL_CHANNEL(0, BMA220_REG_ACCEL_X, X),
 	BMA220_ACCEL_CHANNEL(1, BMA220_REG_ACCEL_Y, Y),
 	BMA220_ACCEL_CHANNEL(2, BMA220_REG_ACCEL_Z, Z),
+	IIO_CHAN_SOFT_TIMESTAMP(3),
 };
 
 static inline int bma220_read_reg(struct spi_device *spi, u8 reg)
@@ -71,6 +86,40 @@ static inline int bma220_read_reg(struct spi_device *spi, u8 reg)
 	return spi_w8r8(spi, reg | BMA220_READ_MASK);
 }
 
+static irqreturn_t bma220_trigger_handler(int irq, void *p)
+{
+	int i;
+	int ret;
+	int bit;
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct bma220_data *data = iio_priv(indio_dev);
+	struct spi_device *spi = data->spi_device;
+	u8 tx_buf = BMA220_REG_ACCEL_X | BMA220_READ_MASK;
+	u8 rx_buf[3];
+
+	/* Do a bulk read, then pick out what we need. */
+	mutex_lock(&data->lock);
+	ret = spi_write_then_read(spi, &tx_buf, sizeof(tx_buf),
+					rx_buf, sizeof(rx_buf));
+	if (ret < 0)
+		goto err;
+
+	i = 0;
+	for_each_set_bit(bit, indio_dev->active_scan_mask,
+			      indio_dev->masklength) {
+		data->buffer[i] = rx_buf[bit];
+		i++;
+	}
+
+	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
+					   pf->timestamp);
+err:
+	mutex_unlock(&data->lock);
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
+
 static int bma220_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan,
 			   int *val, int *val2, long mask)
@@ -173,6 +222,7 @@ static int bma220_probe(struct spi_device *spi)
 	data = iio_priv(indio_dev);
 	data->spi_device = spi;
 	spi_set_drvdata(spi, indio_dev);
+	mutex_init(&data->lock);
 
 	indio_dev->dev.parent = &spi->dev;
 	indio_dev->info = &bma220_info;
@@ -185,11 +235,22 @@ static int bma220_probe(struct spi_device *spi)
 	if (ret < 0)
 		return ret;
 
+	ret = iio_triggered_buffer_setup(indio_dev, NULL,
+					 bma220_trigger_handler, NULL);
+	if (ret < 0) {
+		dev_err(&data->spi_device->dev,
+			"iio triggered buffer setup failed\n");
+		return ret;
+	}
+
 	ret = iio_device_register(indio_dev);
-	if (ret < 0)
+	if (ret < 0) {
 		dev_err(&spi->dev, "iio_device_register failed\n");
+		iio_triggered_buffer_cleanup(indio_dev);
+		return ret;
+	}
 
-	return ret;
+	return 0;
 }
 
 static int bma220_remove(struct spi_device *spi)
@@ -198,6 +259,7 @@ static int bma220_remove(struct spi_device *spi)
 	struct bma220_data *data = iio_priv(indio_dev);
 
 	iio_device_unregister(indio_dev);
+	iio_triggered_buffer_cleanup(indio_dev);
 
 	return bma220_read_reg(data->spi_device, BMA220_REG_SUSPEND);
 }
-- 
1.9.1

Re: [PATCH v3 2/2] iio: accel: Add triggered buffer support for BMA220

[Jonathan Cameron]

On 29/04/16 14:32, Tiberiu Breana wrote:
> Signed-off-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
Again, dma cacheline stuff to fix. Plus one comment that should have been
in my review of patch 1. oops.

Jonathan
> ---
>  drivers/iio/accel/bma220_spi.c | 66 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 64 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
> index 5df5594..cc305aa 100644
> --- a/drivers/iio/accel/bma220_spi.c
> +++ b/drivers/iio/accel/bma220_spi.c
> @@ -11,9 +11,12 @@
>  #include <linux/acpi.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/iio/buffer.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
>  #include <linux/spi/spi.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>  
>  #define BMA220_REG_ID				0x00
>  #define BMA220_REG_ACCEL_X			0x02
> @@ -39,6 +42,14 @@
>  	.channel2 = IIO_MOD_##axis,					\
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
>  	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
> +	.scan_index = index,						\
> +	.scan_type = {							\
> +		.sign = 's',						\
> +		.realbits = 6,						\
> +		.storagebits = 8,					\
> +		.shift = BMA220_DATA_SHIFT,				\
> +		.endianness = IIO_CPU,					\
> +	},								\
>  }
>  
>  static IIO_CONST_ATTR(in_accel_scale_available, BMA220_SCALE_AVAILABLE);
> @@ -58,12 +69,16 @@ static const int bma220_scale_table[][4] = {
>  
>  struct bma220_data {
>  	struct spi_device *spi_device;
> +	struct mutex lock;
> +	s8 buffer[16]; /* 3x8-bit channels + 5x8 padding + 8x8 timestamp */
> +
>  };
>  
>  static const struct iio_chan_spec bma220_channels[] = {
>  	BMA220_ACCEL_CHANNEL(0, BMA220_REG_ACCEL_X, X),
>  	BMA220_ACCEL_CHANNEL(1, BMA220_REG_ACCEL_Y, Y),
>  	BMA220_ACCEL_CHANNEL(2, BMA220_REG_ACCEL_Z, Z),
> +	IIO_CHAN_SOFT_TIMESTAMP(3),
>  };
>  
>  static inline int bma220_read_reg(struct spi_device *spi, u8 reg)
> @@ -71,6 +86,40 @@ static inline int bma220_read_reg(struct spi_device *spi, u8 reg)
>  	return spi_w8r8(spi, reg | BMA220_READ_MASK);
>  }
>  
> +static irqreturn_t bma220_trigger_handler(int irq, void *p)
> +{
> +	int i;
> +	int ret;
> +	int bit;
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct bma220_data *data = iio_priv(indio_dev);
> +	struct spi_device *spi = data->spi_device;
> +	u8 tx_buf = BMA220_REG_ACCEL_X | BMA220_READ_MASK;
> +	u8 rx_buf[3];
> +
> +	/* Do a bulk read, then pick out what we need. */
> +	mutex_lock(&data->lock);
> +	ret = spi_write_then_read(spi, &tx_buf, sizeof(tx_buf),
> +					rx_buf, sizeof(rx_buf));
> +	if (ret < 0)
> +		goto err;
> +
> +	i = 0;
> +	for_each_set_bit(bit, indio_dev->active_scan_mask,
> +			      indio_dev->masklength) {
> +		data->buffer[i] = rx_buf[bit];
> +		i++;
> +	}
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> +					   pf->timestamp);
> +err:
> +	mutex_unlock(&data->lock);
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}
> +
>  static int bma220_read_raw(struct iio_dev *indio_dev,
>  			   struct iio_chan_spec const *chan,
>  			   int *val, int *val2, long mask)
> @@ -173,6 +222,7 @@ static int bma220_probe(struct spi_device *spi)
>  	data = iio_priv(indio_dev);
>  	data->spi_device = spi;
>  	spi_set_drvdata(spi, indio_dev);
> +	mutex_init(&data->lock);
>  
>  	indio_dev->dev.parent = &spi->dev;
>  	indio_dev->info = &bma220_info;
> @@ -185,11 +235,22 @@ static int bma220_probe(struct spi_device *spi)
>  	if (ret < 0)
>  		return ret;
>  
> +	ret = iio_triggered_buffer_setup(indio_dev, NULL,
> +					 bma220_trigger_handler, NULL);
> +	if (ret < 0) {
> +		dev_err(&data->spi_device->dev,
> +			"iio triggered buffer setup failed\n");
> +		return ret;
> +	}
> +
>  	ret = iio_device_register(indio_dev);
> -	if (ret < 0)
> +	if (ret < 0) {
>  		dev_err(&spi->dev, "iio_device_register failed\n");
> +		iio_triggered_buffer_cleanup(indio_dev);
> +		return ret;
> +	}
>  
> -	return ret;
Just noticed - in patch one you should be suspending the device on probe error (or trying
to anyway) 

As such I'd be tempted to have a goto type handling of the error path in here.
> +	return 0;
>  }
>  
>  static int bma220_remove(struct spi_device *spi)
> @@ -198,6 +259,7 @@ static int bma220_remove(struct spi_device *spi)
>  	struct bma220_data *data = iio_priv(indio_dev);
>  
>  	iio_device_unregister(indio_dev);
> +	iio_triggered_buffer_cleanup(indio_dev);
>  
>  	return bma220_read_reg(data->spi_device, BMA220_REG_SUSPEND);
>  }
>