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

[PATCH v4 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.

Change log:
v4:
- addressed Jonathan's comments
- added the bma220_deinit function for suspending the device
- the SPI tx/rx buffers are now cacheline aligned and part of
  the driver's private data

v3:
- added link to datasheet.

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

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 v4 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 | 277 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 288 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..7343575
--- /dev/null
+++ b/drivers/iio/accel/bma220_spi.c
@@ -0,0 +1,277 @@
+/**
+ * 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;
+	struct mutex lock;
+	u8 tx_buf[2] ____cacheline_aligned;
+};
+
+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;
+	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;
+
+		mutex_lock(&data->lock);
+		data->tx_buf[0] = BMA220_REG_RANGE;
+		data->tx_buf[1] = index;
+		ret = spi_write(data->spi_device, data->tx_buf,
+				sizeof(data->tx_buf));
+		if (ret < 0)
+			dev_err(&data->spi_device->dev,
+				"failed to set measurement range\n");
+		mutex_unlock(&data->lock);
+
+		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_deinit(struct spi_device *spi)
+{
+	int ret;
+
+	/* Make sure the chip is powered off */
+	ret = bma220_read_reg(spi, BMA220_REG_SUSPEND);
+	if (ret < 0)
+		return ret;
+	else if (ret == BMA220_SUSPEND_SLEEP)
+		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);
+	mutex_init(&data->lock);
+
+	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 bma220_deinit(spi);
+	}
+
+	return ret;
+}
+
+static int bma220_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+
+	iio_device_unregister(indio_dev);
+
+	return bma220_deinit(spi);
+}
+
+#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

[PATCH v4 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 | 64 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 62 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
index 7343575..6c0bbd8 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);
@@ -59,13 +70,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 */
 	u8 tx_buf[2] ____cacheline_aligned;
+	u8 rx_buf[3];
 };
 
 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)
@@ -73,6 +87,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;
+
+	/* Do a bulk read, then pick out what we need. */
+	mutex_lock(&data->lock);
+	data->tx_buf[0] = BMA220_REG_ACCEL_X | BMA220_READ_MASK;
+	ret = spi_write_then_read(spi, data->tx_buf, 1,
+				  data->rx_buf, sizeof(data->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] = data->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)
@@ -204,13 +252,24 @@ 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(&spi->dev, "iio triggered buffer setup failed\n");
+		goto err_suspend;
+	}
+
 	ret = iio_device_register(indio_dev);
 	if (ret < 0) {
 		dev_err(&spi->dev, "iio_device_register failed\n");
-		return bma220_deinit(spi);
+		iio_triggered_buffer_cleanup(indio_dev);
+		goto err_suspend;
 	}
 
-	return ret;
+	return 0;
+
+err_suspend:
+	return bma220_deinit(spi);
 }
 
 static int bma220_remove(struct spi_device *spi)
@@ -218,6 +277,7 @@ static int bma220_remove(struct spi_device *spi)
 	struct iio_dev *indio_dev = spi_get_drvdata(spi);
 
 	iio_device_unregister(indio_dev);
+	iio_triggered_buffer_cleanup(indio_dev);
 
 	return bma220_deinit(spi);
 }
-- 
1.9.1

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

[Jonathan Cameron]

On 05/05/16 16:48, 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>
Looks good.

Applied to the togreg branch of iio.git - initially pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan
> ---
>  drivers/iio/accel/Kconfig      |  10 ++
>  drivers/iio/accel/Makefile     |   1 +
>  drivers/iio/accel/bma220_spi.c | 277 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 288 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..7343575
> --- /dev/null
> +++ b/drivers/iio/accel/bma220_spi.c
> @@ -0,0 +1,277 @@
> +/**
> + * 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;
> +	struct mutex lock;
> +	u8 tx_buf[2] ____cacheline_aligned;
> +};
> +
> +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;
> +	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;
> +
> +		mutex_lock(&data->lock);
> +		data->tx_buf[0] = BMA220_REG_RANGE;
> +		data->tx_buf[1] = index;
> +		ret = spi_write(data->spi_device, data->tx_buf,
> +				sizeof(data->tx_buf));
> +		if (ret < 0)
> +			dev_err(&data->spi_device->dev,
> +				"failed to set measurement range\n");
> +		mutex_unlock(&data->lock);
> +
> +		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_deinit(struct spi_device *spi)
> +{
> +	int ret;
> +
> +	/* Make sure the chip is powered off */
> +	ret = bma220_read_reg(spi, BMA220_REG_SUSPEND);
> +	if (ret < 0)
> +		return ret;
> +	else if (ret == BMA220_SUSPEND_SLEEP)
> +		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);
> +	mutex_init(&data->lock);
> +
> +	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 bma220_deinit(spi);
> +	}
> +
> +	return ret;
> +}
> +
> +static int bma220_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +
> +	iio_device_unregister(indio_dev);
> +
> +	return bma220_deinit(spi);
> +}
> +
> +#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");
> 

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

[Jonathan Cameron]

On 05/05/16 16:48, Tiberiu Breana wrote:
> Signed-off-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
Hi Tiberiu,

A few optimization suggestions inline...

Otherwise looks pretty good to me.  As I've applied patch 1 now, just
send new versions of this patch on their own.  Plenty of time this coming
cycle so I'm being fussier than I might otherwise have been.

Thanks

Jonathan
> ---
>  drivers/iio/accel/bma220_spi.c | 64 ++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 62 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/accel/bma220_spi.c b/drivers/iio/accel/bma220_spi.c
> index 7343575..6c0bbd8 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);
> @@ -59,13 +70,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 */
>  	u8 tx_buf[2] ____cacheline_aligned;
> +	u8 rx_buf[3];
>  };
>  
>  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)
> @@ -73,6 +87,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;
> +
> +	/* Do a bulk read, then pick out what we need. */
Why not provide available_scan_masks and let the core demux handle the
picking out of relevant data? (there are cases where that doesn't make
sense but I'm not seeing why here).  The reasons it does make sense
are primarily a case of not reinventing the wheel and because the
core can be demuxing into multiple client buffers - if you have
it in the driver as well you do all the moving of data twice.

Also - side note, rx_buf doesn't need to be cacheline_aligned as
spi_write_then_read uses buffers
from spi.h  
/* this copies txbuf and rxbuf data; for small transfers only! */
Which means that here you can just use the buffer
directly in the call and save any explicit copying at all.
> +	mutex_lock(&data->lock);
> +	data->tx_buf[0] = BMA220_REG_ACCEL_X | BMA220_READ_MASK;
> +	ret = spi_write_then_read(spi, data->tx_buf, 1,
> +				  data->rx_buf, sizeof(data->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] = data->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)
> @@ -204,13 +252,24 @@ 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(&spi->dev, "iio triggered buffer setup failed\n");
> +		goto err_suspend;
> +	}
> +
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0) {
>  		dev_err(&spi->dev, "iio_device_register failed\n");
> -		return bma220_deinit(spi);
> +		iio_triggered_buffer_cleanup(indio_dev);
> +		goto err_suspend;
>  	}
>  
> -	return ret;
> +	return 0;
> +
> +err_suspend:
> +	return bma220_deinit(spi);
>  }
>  
>  static int bma220_remove(struct spi_device *spi)
> @@ -218,6 +277,7 @@ static int bma220_remove(struct spi_device *spi)
>  	struct iio_dev *indio_dev = spi_get_drvdata(spi);
>  
>  	iio_device_unregister(indio_dev);
> +	iio_triggered_buffer_cleanup(indio_dev);
>  
>  	return bma220_deinit(spi);
>  }
>