linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] iio: imu: Add support for Kionix KMX61 sensor
@ 2014-11-18 16:47 Daniel Baluta
  2014-11-22 10:46 ` Jonathan Cameron
  2014-11-25  0:45 ` Hartmut Knaack
  0 siblings, 2 replies; 7+ messages in thread
From: Daniel Baluta @ 2014-11-18 16:47 UTC (permalink / raw)
  To: jic23, pmeerw, srinivas.pandruvada; +Cc: linux-iio, linux-kernel

Minimal implementation for KMX61 6-axis accelerometer/magnetometer. It exports
raw accel/magn readings together with scale and sampling frequency.

Datasheet will be available at:
http://www.kionix.com/6-axis-accelerometer-magnetometer/kmx61

Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
---
Changes since v2:
	* mostly style fixes after comments received from Jonathan and Peter
		* http://marc.info/?l=linux-iio&m=141564019602400&w=2
	* implemented PM runtime and PM sleep resume/suspend hooks
	* introduced new parameter for kmx61_set_mode to help updating
	stby bits in kmx61_data.
	* use kmx6111021 instead of kmx61 for i2c_device_id.

 drivers/iio/imu/Kconfig  |   9 +
 drivers/iio/imu/Makefile |   2 +
 drivers/iio/imu/kmx61.c  | 766 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 777 insertions(+)
 create mode 100644 drivers/iio/imu/kmx61.c

diff --git a/drivers/iio/imu/Kconfig b/drivers/iio/imu/Kconfig
index 2b0e451..d675f43 100644
--- a/drivers/iio/imu/Kconfig
+++ b/drivers/iio/imu/Kconfig
@@ -25,6 +25,15 @@ config ADIS16480
 	  Say yes here to build support for Analog Devices ADIS16375, ADIS16480,
 	  ADIS16485, ADIS16488 inertial sensors.
 
+config KMX61
+	tristate "Kionix KMX61 6-axis accelerometer and magnetometer"
+	depends on I2C
+	help
+	  Say Y here if you want to build a driver for Kionix KMX61 6-axis accelerometer
+	  and magnetometer.
+	  To compile this driver as module, choose M here: the module will be called
+	  kmx61.
+
 source "drivers/iio/imu/inv_mpu6050/Kconfig"
 
 endmenu
diff --git a/drivers/iio/imu/Makefile b/drivers/iio/imu/Makefile
index 114d2c1..e1e6e3d 100644
--- a/drivers/iio/imu/Makefile
+++ b/drivers/iio/imu/Makefile
@@ -14,3 +14,5 @@ adis_lib-$(CONFIG_IIO_ADIS_LIB_BUFFER) += adis_buffer.o
 obj-$(CONFIG_IIO_ADIS_LIB) += adis_lib.o
 
 obj-y += inv_mpu6050/
+
+obj-$(CONFIG_KMX61) += kmx61.o
diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c
new file mode 100644
index 0000000..f68b3ef
--- /dev/null
+++ b/drivers/iio/imu/kmx61.c
@@ -0,0 +1,766 @@
+/*
+ * KMX61 - Kionix 6-axis Accelerometer/Magnetometer
+ *
+ * Copyright (c) 2014, 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.
+ *
+ * IIO driver for KMX61 (7-bit I2C slave address 0x0E or 0x0F).
+ *
+ * TODO: buffer, interrupt, thresholds, acpi, temperature sensor
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define KMX61_DRV_NAME "kmx61"
+
+#define KMX61_REG_WHO_AM_I	0x00
+
+/*
+ * three 16-bit accelerometer output registers for X/Y/Z axis
+ * we use only XOUT_L as a base register, all other addresses
+ * can be obtained by applying an offset and are provided here
+ * only for clarity.
+ */
+#define KMX61_ACC_XOUT_L	0x0A
+#define KMX61_ACC_XOUT_H	0x0B
+#define KMX61_ACC_YOUT_L	0x0C
+#define KMX61_ACC_YOUT_H	0x0D
+#define KMX61_ACC_ZOUT_L	0x0E
+#define KMX61_ACC_ZOUT_H	0x0F
+
+/*
+ * one 16-bit temperature output register
+ */
+#define KMX61_TEMP_L		0x10
+#define KMX61_TEMP_H		0x11
+
+/*
+ * three 16-bit magnetometer output registers for X/Y/Z axis
+ */
+#define KMX61_MAG_XOUT_L	0x12
+#define KMX61_MAG_XOUT_H	0x13
+#define KMX61_MAG_YOUT_L	0x14
+#define KMX61_MAG_YOUT_H	0x15
+#define KMX61_MAG_ZOUT_L	0x16
+#define KMX61_MAG_ZOUT_H	0x17
+
+#define KMX61_REG_ODCNTL	0x2C
+#define KMX61_REG_STBY		0x29
+#define KMX61_REG_CTRL1		0x2A
+
+#define KMX61_ACC_STBY_BIT	BIT(0)
+#define KMX61_MAG_STBY_BIT	BIT(1)
+#define KMX61_ACT_STBY_BIT	BIT(7)
+
+#define KMX61_ALL_STBY		(KMX61_ACC_STBY_BIT | KMX61_MAG_STBY_BIT)
+
+#define KMX61_REG_CTRL1_GSEL0_SHIFT	0
+#define KMX61_REG_CTRL1_GSEL1_SHIFT	1
+#define KMX61_REG_CTRL1_GSEL0_MASK	0x01
+#define KMX61_REG_CTRL1_GSEL1_MASK	0x02
+
+#define KMX61_REG_CTRL1_BIT_RES		BIT(4)
+
+#define KMX61_ACC_ODR_SHIFT	0
+#define KMX61_MAG_ODR_SHIFT	4
+#define KMX61_ACC_ODR_MASK	0x0F
+#define KMX61_MAG_ODR_MASK	0xF0
+
+#define KMX61_SLEEP_DELAY_MS	2000
+
+#define KMX61_CHIP_ID		0x12
+
+struct kmx61_data {
+	struct i2c_client *client;
+
+	/* serialize access to non-atomic ops, e.g set_mode */
+	struct mutex lock;
+	u8 range;
+	u8 odr_bits;
+
+	/* standby state */
+	u8 acc_stby;
+	u8 mag_stby;
+
+	/* power state */
+	bool acc_ps;
+	bool mag_ps;
+};
+
+enum kmx61_range {
+	KMX61_RANGE_2G,
+	KMX61_RANGE_4G,
+	KMX61_RANGE_8G,
+};
+
+enum kmx61_scan {
+	KMX61_SCAN_ACC_X,
+	KMX61_SCAN_ACC_Y,
+	KMX61_SCAN_ACC_Z,
+	KMX61_SCAN_TEMP,
+	KMX61_SCAN_MAG_X,
+	KMX61_SCAN_MAG_Y,
+	KMX61_SCAN_MAG_Z,
+};
+
+static const struct {
+	u16 uscale;
+	u8 gsel0;
+	u8 gsel1;
+} kmx61_scale_table[] = {
+	{9582, 0, 0},
+	{19163, 1, 0},
+	{38326, 0, 1},
+};
+
+/* KMX61 devices */
+#define KMX61_ACC	0x01
+#define KMX61_MAG	0x02
+
+static const struct {
+	int val;
+	int val2;
+	u8 odr_bits;
+} kmx61_samp_freq_table[] = { {12, 500000, 0x00},
+			{25, 0, 0x01},
+			{50, 0, 0x02},
+			{100, 0, 0x03},
+			{200, 0, 0x04},
+			{400, 0, 0x05},
+			{800, 0, 0x06},
+			{1600, 0, 0x07},
+			{0, 781000, 0x08},
+			{1, 563000, 0x09},
+			{3, 125000, 0x0A},
+			{6, 250000, 0x0B} };
+
+static IIO_CONST_ATTR(accel_scale_available, "0.009582 0.019163 0.038326");
+static IIO_CONST_ATTR(magn_scale_available, "0.001465");
+static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
+	"0.781000 1.563000 3.125000 6.250000 12.500000 25 50 100 200 400 800");
+
+static struct attribute *kmx61_attributes[] = {
+	&iio_const_attr_accel_scale_available.dev_attr.attr,
+	&iio_const_attr_magn_scale_available.dev_attr.attr,
+	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group kmx61_attribute_group = {
+	.attrs = kmx61_attributes,
+};
+
+#define KMX61_ACC_CHAN(_axis, _index) { \
+	.type = IIO_ACCEL, \
+	.modified = 1, \
+	.channel2 = IIO_MOD_ ## _axis, \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
+				BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+	.address = KMX61_ACC, \
+	.scan_index = _index, \
+	.scan_type = { \
+		.sign = 's', \
+		.realbits = 12, \
+		.storagebits = 16, \
+		.shift = 4, \
+		.endianness = IIO_LE, \
+	}, \
+}
+
+#define KMX61_MAG_CHAN(_axis, _index) { \
+	.type = IIO_MAGN, \
+	.modified = 1, \
+	.channel2 = IIO_MOD_ ## _axis, \
+	.address = KMX61_MAG, \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
+				BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+	.scan_index = _index, \
+	.scan_type = { \
+		.sign = 's', \
+		.realbits = 14, \
+		.storagebits = 16, \
+		.shift = 2, \
+		.endianness = IIO_LE, \
+	}, \
+}
+
+static const struct iio_chan_spec kmx61_channels[] = {
+	KMX61_ACC_CHAN(X, KMX61_SCAN_ACC_X),
+	KMX61_ACC_CHAN(Y, KMX61_SCAN_ACC_Y),
+	KMX61_ACC_CHAN(Z, KMX61_SCAN_ACC_Z),
+	KMX61_MAG_CHAN(X, KMX61_SCAN_MAG_X),
+	KMX61_MAG_CHAN(Y, KMX61_SCAN_MAG_Y),
+	KMX61_MAG_CHAN(Z, KMX61_SCAN_MAG_Z),
+};
+
+static int kmx61_convert_freq_to_bit(int val, int val2)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(kmx61_samp_freq_table); i++)
+		if (val == kmx61_samp_freq_table[i].val &&
+		    val2 == kmx61_samp_freq_table[i].val2)
+			return kmx61_samp_freq_table[i].odr_bits;
+	return -EINVAL;
+}
+/**
+ * kmx61_set_mode() - set KMX61 device operating mode
+ * @data - kmx61 device private data pointer
+ * @mode - bitmask, indicating operating mode for @device
+ * @device - bitmask, indicating device for which @mode needs to be set
+ * @update - update stby bits stored in device's private  @data
+ *
+ * For each sensor (accelerometer/magnetometer) there are two operating modes
+ * STANDBY and OPERATION. Neither accel nor magn can be disabled independently
+ * if they are both enabled. Internal sensors state is saved in acc_stby and
+ * mag_stby members of driver's private @data.
+ */
+static int kmx61_set_mode(struct kmx61_data *data, u8 mode, u8 device,
+			  bool update)
+{
+	int ret;
+	int acc_stby = -1, mag_stby = -1;
+
+	ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_STBY);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Error reading reg_stby\n");
+		return ret;
+	}
+	if (device & KMX61_ACC) {
+		if (mode & KMX61_ACC_STBY_BIT) {
+			ret |= KMX61_ACC_STBY_BIT;
+			acc_stby = 1;
+		} else {
+			ret &= ~KMX61_ACC_STBY_BIT;
+			acc_stby = 0;
+		}
+	}
+
+	if (device & KMX61_MAG) {
+		if (mode & KMX61_MAG_STBY_BIT) {
+			ret |= KMX61_MAG_STBY_BIT;
+			mag_stby = 1;
+		} else {
+			ret &= ~KMX61_MAG_STBY_BIT;
+			mag_stby = 0;
+		}
+	}
+
+	ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_STBY, ret);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Error writing reg_stby\n");
+		return ret;
+	}
+
+	if (acc_stby != -1 && update)
+		data->acc_stby = !!acc_stby;
+	if (mag_stby != -1 && update)
+		data->mag_stby = !!mag_stby;
+
+	return ret;
+}
+
+static int kmx61_get_mode(struct kmx61_data *data, u8 *mode, u8 device)
+{
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_STBY);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Error reading reg_stby\n");
+		return ret;
+	}
+	*mode = 0;
+
+	if (device & KMX61_ACC) {
+		if (ret & KMX61_ACC_STBY_BIT)
+			*mode |= KMX61_ACC_STBY_BIT;
+		else
+			*mode &= ~KMX61_ACC_STBY_BIT;
+	}
+
+	if (device & KMX61_MAG) {
+		if (ret & KMX61_MAG_STBY_BIT)
+			*mode |= KMX61_MAG_STBY_BIT;
+		else
+			*mode &= ~KMX61_MAG_STBY_BIT;
+	}
+
+	return 0;
+}
+
+static int kmx61_set_odr(struct kmx61_data *data, int val, int val2, u8 device)
+{
+	int ret;
+	u8 mode;
+	int lodr_bits, odr_bits;
+
+	ret = kmx61_get_mode(data, &mode, KMX61_ACC | KMX61_MAG);
+	if (ret < 0)
+		return ret;
+
+	lodr_bits = kmx61_convert_freq_to_bit(val, val2);
+	if (lodr_bits < 0)
+		return lodr_bits;
+
+	/* To change ODR, accel and magn must be in STDBY */
+	ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG,
+			     true);
+	if (ret < 0)
+		return ret;
+
+	odr_bits = 0;
+	if (device & KMX61_ACC)
+		odr_bits |= lodr_bits;
+	if (device & KMX61_MAG)
+		odr_bits |= (lodr_bits << KMX61_MAG_ODR_SHIFT);
+
+	ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_ODCNTL,
+					odr_bits);
+	if (ret < 0)
+		return ret;
+
+	ret = kmx61_set_mode(data, mode, KMX61_ACC | KMX61_MAG, true);
+	if (ret < 0)
+		return ret;
+
+	data->odr_bits = lodr_bits;
+
+	return 0;
+}
+
+static
+int kmx61_get_odr(struct kmx61_data *data, int *val, int *val2, u8 device)
+{	int i;
+	u8 lodr_bits;
+
+	if (device & KMX61_ACC)
+		lodr_bits = (data->odr_bits >> KMX61_ACC_ODR_SHIFT) &
+			     KMX61_ACC_ODR_MASK;
+	else if (device & KMX61_MAG)
+		lodr_bits = (data->odr_bits >> KMX61_MAG_ODR_SHIFT) &
+			     KMX61_MAG_ODR_MASK;
+	else
+		return -EINVAL;
+
+	for (i = 0; i < ARRAY_SIZE(kmx61_samp_freq_table); i++)
+		if (lodr_bits == kmx61_samp_freq_table[i].odr_bits) {
+			*val = kmx61_samp_freq_table[i].val;
+			*val2 = kmx61_samp_freq_table[i].val2;
+			return 0;
+		}
+	return -EINVAL;
+}
+
+static int kmx61_set_range(struct kmx61_data *data, int range)
+{
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_CTRL1);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
+		return ret;
+	}
+
+	ret &= ~(KMX61_REG_CTRL1_GSEL0_MASK | KMX61_REG_CTRL1_GSEL1_MASK);
+	ret |= kmx61_scale_table[range].gsel0 << KMX61_REG_CTRL1_GSEL0_SHIFT;
+	ret |= kmx61_scale_table[range].gsel1 << KMX61_REG_CTRL1_GSEL1_SHIFT;
+
+	ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_CTRL1, ret);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Error writing reg_ctrl1\n");
+		return ret;
+	}
+
+	data->range = range;
+
+	return 0;
+}
+
+static int kmx61_set_scale(struct kmx61_data *data, int uscale)
+{
+	int ret, i;
+	u8  mode;
+
+	for (i = 0; i < ARRAY_SIZE(kmx61_scale_table); i++) {
+		if (kmx61_scale_table[i].uscale == uscale) {
+			ret = kmx61_get_mode(data, &mode,
+					     KMX61_ACC | KMX61_MAG);
+			if (ret < 0)
+				return ret;
+
+			ret = kmx61_set_mode(data, KMX61_ALL_STBY,
+					     KMX61_ACC | KMX61_MAG, true);
+			if (ret < 0)
+				return ret;
+
+			ret = kmx61_set_range(data, i);
+			if (ret < 0)
+				return ret;
+
+			return  kmx61_set_mode(data, mode,
+					       KMX61_ACC | KMX61_MAG, true);
+		}
+	}
+	return -EINVAL;
+}
+
+static int kmx61_chip_init(struct kmx61_data *data)
+{
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_WHO_AM_I);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Error reading who_am_i\n");
+		return ret;
+	}
+
+	if (ret != KMX61_CHIP_ID) {
+		dev_err(&data->client->dev,
+			"Wrong chip id, got %x expected %x\n",
+			 ret, KMX61_CHIP_ID);
+		return -EINVAL;
+	}
+
+	/* set accel 12bit, 4g range */
+	ret = kmx61_set_range(data, KMX61_RANGE_4G);
+	if (ret < 0)
+		return ret;
+
+	/* set acc/magn to OPERATION mode */
+	ret = kmx61_set_mode(data, 0, KMX61_ACC | KMX61_MAG, true);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+/**
+ * kmx61_set_power_state() - set power state for kmx61 @device
+ * @data - kmx61 device private pointer
+ * @on - power state to be set for @device
+ * @device - bitmask indicating device for which @on state needs to be set
+ *
+ * Notice that when ACC power state needs to be set to ON and MAG is in
+ * OPERATION then we know that kmx61_runtime_resume was already called
+ * so we must set ACC OPERATION mode here. The same happens when MAG power
+ * state needs to be set to ON and ACC is in OPERATION.
+ */
+static int kmx61_set_power_state(struct kmx61_data *data, bool on, u8 device)
+{
+#ifdef CONFIG_PM_RUNTIME
+	int ret;
+
+	if (device & KMX61_ACC) {
+		if (on && !data->acc_ps && !data->mag_stby)
+			kmx61_set_mode(data, 0, KMX61_ACC, true);
+		data->acc_ps = on;
+	}
+	if (device & KMX61_MAG) {
+		if (on && !data->mag_ps && !data->acc_stby)
+			kmx61_set_mode(data, 0, KMX61_MAG, true);
+		data->mag_ps = on;
+	}
+
+	if (on) {
+		ret = pm_runtime_get_sync(&data->client->dev);
+	} else {
+		pm_runtime_mark_last_busy(&data->client->dev);
+		ret = pm_runtime_put_autosuspend(&data->client->dev);
+	}
+	if (ret < 0) {
+		dev_err(&data->client->dev,
+			"Failed: kmx61_set_power_state for %d, ret %d\n",
+			on, ret);
+		return ret;
+	}
+#endif
+	return 0;
+}
+
+static int kmx61_read_measurement(struct kmx61_data *data, int base, int offset)
+{
+	int ret;
+	u8 reg = base + offset * 2;
+
+	ret = i2c_smbus_read_word_data(data->client, reg);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "failed to read reg at %x\n", reg);
+		return ret;
+	}
+
+	return ret;
+}
+
+static int kmx61_read_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan, int *val,
+			      int *val2, long mask)
+{
+	struct kmx61_data *data = iio_priv(indio_dev);
+	int ret;
+	u8 base_reg;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		switch (chan->type) {
+		case IIO_ACCEL:
+		case IIO_MAGN:
+			base_reg = KMX61_ACC_XOUT_L;
+			break;
+		default:
+			return -EINVAL;
+		}
+		mutex_lock(&data->lock);
+
+		kmx61_set_power_state(data, true, chan->address);
+		ret = kmx61_read_measurement(data, base_reg, chan->scan_index);
+		if (ret < 0) {
+			kmx61_set_power_state(data, false, chan->address);
+			mutex_unlock(&data->lock);
+			return ret;
+		}
+		*val = sign_extend32(ret >> chan->scan_type.shift,
+				     chan->scan_type.realbits - 1);
+		kmx61_set_power_state(data, false, chan->address);
+
+		mutex_unlock(&data->lock);
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_ACCEL:
+			*val = 0;
+			*val2 = kmx61_scale_table[data->range].uscale;
+			return IIO_VAL_INT_PLUS_MICRO;
+		case IIO_MAGN:
+			/* 14 bits res, 1465 microGauss per magn count */
+			*val = 0;
+			*val2 = 1465;
+			return IIO_VAL_INT_PLUS_MICRO;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		if (chan->type != IIO_ACCEL && chan->type != IIO_MAGN)
+			return -EINVAL;
+
+		mutex_lock(&data->lock);
+		ret = kmx61_get_odr(data, val, val2, chan->address);
+		mutex_unlock(&data->lock);
+		if (ret)
+			return -EINVAL;
+		return IIO_VAL_INT_PLUS_MICRO;
+	}
+	return -EINVAL;
+}
+
+static int kmx61_write_raw(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan, int val,
+			       int val2, long mask)
+{
+	struct kmx61_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		if (chan->type != IIO_ACCEL && chan->type != IIO_MAGN)
+			return -EINVAL;
+
+		mutex_lock(&data->lock);
+		ret = kmx61_set_odr(data, val, val2, chan->address);
+		mutex_unlock(&data->lock);
+		return ret;
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_ACCEL:
+			if (val != 0)
+				return -EINVAL;
+			mutex_lock(&data->lock);
+			ret = kmx61_set_scale(data, val2);
+			mutex_unlock(&data->lock);
+			return ret;
+		default:
+			return -EINVAL;
+		}
+		return ret;
+	default:
+		return -EINVAL;
+	}
+	return ret;
+}
+
+static const struct iio_info kmx61_info = {
+	.driver_module		= THIS_MODULE,
+	.read_raw		= kmx61_read_raw,
+	.write_raw		= kmx61_write_raw,
+	.attrs			= &kmx61_attribute_group,
+};
+
+static int kmx61_probe(struct i2c_client *client,
+		       const struct i2c_device_id *id)
+{
+	struct kmx61_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+	const char *name = NULL;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+	data->client = client;
+
+	if (id)
+		name = id->name;
+
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->channels = kmx61_channels;
+	indio_dev->num_channels = ARRAY_SIZE(kmx61_channels);
+	indio_dev->name = name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &kmx61_info;
+
+	mutex_init(&data->lock);
+
+	ret = kmx61_chip_init(data);
+	if (ret < 0)
+		return ret;
+
+	ret = iio_device_register(indio_dev);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to register iio device\n");
+		goto err_iio_device_register;
+	}
+
+	ret = pm_runtime_set_active(&client->dev);
+	if (ret < 0)
+		goto err_pm_runtime_set_active;
+
+	pm_runtime_enable(&client->dev);
+	pm_runtime_set_autosuspend_delay(&client->dev, KMX61_SLEEP_DELAY_MS);
+	pm_runtime_use_autosuspend(&client->dev);
+
+	return 0;
+
+err_pm_runtime_set_active:
+	iio_device_unregister(indio_dev);
+err_iio_device_register:
+	kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
+	return ret;
+}
+
+static int kmx61_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct kmx61_data *data = iio_priv(indio_dev);
+	int ret;
+
+	pm_runtime_disable(&client->dev);
+	pm_runtime_set_suspended(&client->dev);
+	pm_runtime_put_noidle(&client->dev);
+
+	iio_device_unregister(indio_dev);
+
+	mutex_lock(&data->lock);
+	ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int kmx61_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct kmx61_data *data = iio_priv(indio_dev);
+	int ret;
+
+	mutex_lock(&data->lock);
+	ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG,
+			     false);
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int kmx61_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct kmx61_data *data = iio_priv(indio_dev);
+	u8 stby = 0;
+
+	if (data->acc_stby)
+		stby |= KMX61_ACC_STBY_BIT;
+	if (data->mag_stby)
+		stby |= KMX61_MAG_STBY_BIT;
+
+	return kmx61_set_mode(data, stby, KMX61_ACC | KMX61_MAG, true);
+}
+#endif
+
+#ifdef CONFIG_PM_RUNTIME
+static int kmx61_runtime_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct kmx61_data *data = iio_priv(indio_dev);
+	int ret;
+
+	mutex_lock(&data->lock);
+	ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int kmx61_runtime_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct kmx61_data *data = iio_priv(indio_dev);
+	u8 stby = 0;
+
+	if (!data->acc_ps)
+		stby |= KMX61_ACC_STBY_BIT;
+	if (!data->mag_ps)
+		stby |= KMX61_MAG_STBY_BIT;
+
+	return kmx61_set_mode(data, stby, KMX61_ACC | KMX61_MAG, true);
+}
+#endif
+
+static const struct dev_pm_ops kmx61_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(kmx61_suspend, kmx61_resume)
+	SET_RUNTIME_PM_OPS(kmx61_runtime_suspend, kmx61_runtime_resume, NULL)
+};
+
+static const struct i2c_device_id kmx61_id[] = {
+	{"kmx611021", 0},
+	{}
+};
+
+MODULE_DEVICE_TABLE(i2c, kmx61_id);
+
+static struct i2c_driver kmx61_driver = {
+	.driver = {
+		.name = KMX61_DRV_NAME,
+		.pm = &kmx61_pm_ops,
+	},
+	.probe		= kmx61_probe,
+	.remove		= kmx61_remove,
+	.id_table	= kmx61_id,
+};
+
+module_i2c_driver(kmx61_driver);
+
+MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com>");
+MODULE_DESCRIPTION("KMX61 accelerometer/magnetometer driver");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] iio: imu: Add support for Kionix KMX61 sensor
  2014-11-18 16:47 [PATCH v3] iio: imu: Add support for Kionix KMX61 sensor Daniel Baluta
@ 2014-11-22 10:46 ` Jonathan Cameron
  2014-11-25  0:45 ` Hartmut Knaack
  1 sibling, 0 replies; 7+ messages in thread
From: Jonathan Cameron @ 2014-11-22 10:46 UTC (permalink / raw)
  To: Daniel Baluta, pmeerw, srinivas.pandruvada; +Cc: linux-iio, linux-kernel

On 18/11/14 16:47, Daniel Baluta wrote:
> Minimal implementation for KMX61 6-axis accelerometer/magnetometer. It exports
> raw accel/magn readings together with scale and sampling frequency.
> 
> Datasheet will be available at:
> http://www.kionix.com/6-axis-accelerometer-magnetometer/kmx61
> 
> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
I would slightly have prefered the runtime suspend stuff as a follow up
patch for ease fo reviewing, but it seems straight forward and correct
so fair enough.,

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

Jonathan
> ---
> Changes since v2:
> 	* mostly style fixes after comments received from Jonathan and Peter
> 		* http://marc.info/?l=linux-iio&m=141564019602400&w=2
> 	* implemented PM runtime and PM sleep resume/suspend hooks
> 	* introduced new parameter for kmx61_set_mode to help updating
> 	stby bits in kmx61_data.
> 	* use kmx6111021 instead of kmx61 for i2c_device_id.
> 
>  drivers/iio/imu/Kconfig  |   9 +
>  drivers/iio/imu/Makefile |   2 +
>  drivers/iio/imu/kmx61.c  | 766 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 777 insertions(+)
>  create mode 100644 drivers/iio/imu/kmx61.c
> 
> diff --git a/drivers/iio/imu/Kconfig b/drivers/iio/imu/Kconfig
> index 2b0e451..d675f43 100644
> --- a/drivers/iio/imu/Kconfig
> +++ b/drivers/iio/imu/Kconfig
> @@ -25,6 +25,15 @@ config ADIS16480
>  	  Say yes here to build support for Analog Devices ADIS16375, ADIS16480,
>  	  ADIS16485, ADIS16488 inertial sensors.
>  
> +config KMX61
> +	tristate "Kionix KMX61 6-axis accelerometer and magnetometer"
> +	depends on I2C
> +	help
> +	  Say Y here if you want to build a driver for Kionix KMX61 6-axis accelerometer
> +	  and magnetometer.
> +	  To compile this driver as module, choose M here: the module will be called
> +	  kmx61.
> +
>  source "drivers/iio/imu/inv_mpu6050/Kconfig"
>  
>  endmenu
> diff --git a/drivers/iio/imu/Makefile b/drivers/iio/imu/Makefile
> index 114d2c1..e1e6e3d 100644
> --- a/drivers/iio/imu/Makefile
> +++ b/drivers/iio/imu/Makefile
> @@ -14,3 +14,5 @@ adis_lib-$(CONFIG_IIO_ADIS_LIB_BUFFER) += adis_buffer.o
>  obj-$(CONFIG_IIO_ADIS_LIB) += adis_lib.o
>  
>  obj-y += inv_mpu6050/
> +
> +obj-$(CONFIG_KMX61) += kmx61.o
> diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c
> new file mode 100644
> index 0000000..f68b3ef
> --- /dev/null
> +++ b/drivers/iio/imu/kmx61.c
> @@ -0,0 +1,766 @@
> +/*
> + * KMX61 - Kionix 6-axis Accelerometer/Magnetometer
> + *
> + * Copyright (c) 2014, 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.
> + *
> + * IIO driver for KMX61 (7-bit I2C slave address 0x0E or 0x0F).
> + *
> + * TODO: buffer, interrupt, thresholds, acpi, temperature sensor
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define KMX61_DRV_NAME "kmx61"
> +
> +#define KMX61_REG_WHO_AM_I	0x00
> +
> +/*
> + * three 16-bit accelerometer output registers for X/Y/Z axis
> + * we use only XOUT_L as a base register, all other addresses
> + * can be obtained by applying an offset and are provided here
> + * only for clarity.
> + */
> +#define KMX61_ACC_XOUT_L	0x0A
> +#define KMX61_ACC_XOUT_H	0x0B
> +#define KMX61_ACC_YOUT_L	0x0C
> +#define KMX61_ACC_YOUT_H	0x0D
> +#define KMX61_ACC_ZOUT_L	0x0E
> +#define KMX61_ACC_ZOUT_H	0x0F
> +
> +/*
> + * one 16-bit temperature output register
> + */
> +#define KMX61_TEMP_L		0x10
> +#define KMX61_TEMP_H		0x11
> +
> +/*
> + * three 16-bit magnetometer output registers for X/Y/Z axis
> + */
> +#define KMX61_MAG_XOUT_L	0x12
> +#define KMX61_MAG_XOUT_H	0x13
> +#define KMX61_MAG_YOUT_L	0x14
> +#define KMX61_MAG_YOUT_H	0x15
> +#define KMX61_MAG_ZOUT_L	0x16
> +#define KMX61_MAG_ZOUT_H	0x17
> +
> +#define KMX61_REG_ODCNTL	0x2C
> +#define KMX61_REG_STBY		0x29
> +#define KMX61_REG_CTRL1		0x2A
> +
> +#define KMX61_ACC_STBY_BIT	BIT(0)
> +#define KMX61_MAG_STBY_BIT	BIT(1)
> +#define KMX61_ACT_STBY_BIT	BIT(7)
> +
> +#define KMX61_ALL_STBY		(KMX61_ACC_STBY_BIT | KMX61_MAG_STBY_BIT)
> +
> +#define KMX61_REG_CTRL1_GSEL0_SHIFT	0
> +#define KMX61_REG_CTRL1_GSEL1_SHIFT	1
> +#define KMX61_REG_CTRL1_GSEL0_MASK	0x01
> +#define KMX61_REG_CTRL1_GSEL1_MASK	0x02
> +
> +#define KMX61_REG_CTRL1_BIT_RES		BIT(4)
> +
> +#define KMX61_ACC_ODR_SHIFT	0
> +#define KMX61_MAG_ODR_SHIFT	4
> +#define KMX61_ACC_ODR_MASK	0x0F
> +#define KMX61_MAG_ODR_MASK	0xF0
> +
> +#define KMX61_SLEEP_DELAY_MS	2000
> +
> +#define KMX61_CHIP_ID		0x12
> +
> +struct kmx61_data {
> +	struct i2c_client *client;
> +
> +	/* serialize access to non-atomic ops, e.g set_mode */
> +	struct mutex lock;
> +	u8 range;
> +	u8 odr_bits;
> +
> +	/* standby state */
> +	u8 acc_stby;
> +	u8 mag_stby;
> +
> +	/* power state */
> +	bool acc_ps;
> +	bool mag_ps;
> +};
> +
> +enum kmx61_range {
> +	KMX61_RANGE_2G,
> +	KMX61_RANGE_4G,
> +	KMX61_RANGE_8G,
> +};
> +
> +enum kmx61_scan {
> +	KMX61_SCAN_ACC_X,
> +	KMX61_SCAN_ACC_Y,
> +	KMX61_SCAN_ACC_Z,
> +	KMX61_SCAN_TEMP,
> +	KMX61_SCAN_MAG_X,
> +	KMX61_SCAN_MAG_Y,
> +	KMX61_SCAN_MAG_Z,
> +};
> +
> +static const struct {
> +	u16 uscale;
> +	u8 gsel0;
> +	u8 gsel1;
> +} kmx61_scale_table[] = {
> +	{9582, 0, 0},
> +	{19163, 1, 0},
> +	{38326, 0, 1},
> +};
> +
> +/* KMX61 devices */
> +#define KMX61_ACC	0x01
> +#define KMX61_MAG	0x02
> +
> +static const struct {
> +	int val;
> +	int val2;
> +	u8 odr_bits;
> +} kmx61_samp_freq_table[] = { {12, 500000, 0x00},
> +			{25, 0, 0x01},
> +			{50, 0, 0x02},
> +			{100, 0, 0x03},
> +			{200, 0, 0x04},
> +			{400, 0, 0x05},
> +			{800, 0, 0x06},
> +			{1600, 0, 0x07},
> +			{0, 781000, 0x08},
> +			{1, 563000, 0x09},
> +			{3, 125000, 0x0A},
> +			{6, 250000, 0x0B} };
> +
> +static IIO_CONST_ATTR(accel_scale_available, "0.009582 0.019163 0.038326");
> +static IIO_CONST_ATTR(magn_scale_available, "0.001465");
> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
> +	"0.781000 1.563000 3.125000 6.250000 12.500000 25 50 100 200 400 800");
> +
> +static struct attribute *kmx61_attributes[] = {
> +	&iio_const_attr_accel_scale_available.dev_attr.attr,
> +	&iio_const_attr_magn_scale_available.dev_attr.attr,
> +	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group kmx61_attribute_group = {
> +	.attrs = kmx61_attributes,
> +};
> +
> +#define KMX61_ACC_CHAN(_axis, _index) { \
> +	.type = IIO_ACCEL, \
> +	.modified = 1, \
> +	.channel2 = IIO_MOD_ ## _axis, \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
> +				BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> +	.address = KMX61_ACC, \
> +	.scan_index = _index, \
> +	.scan_type = { \
> +		.sign = 's', \
> +		.realbits = 12, \
> +		.storagebits = 16, \
> +		.shift = 4, \
> +		.endianness = IIO_LE, \
> +	}, \
> +}
> +
> +#define KMX61_MAG_CHAN(_axis, _index) { \
> +	.type = IIO_MAGN, \
> +	.modified = 1, \
> +	.channel2 = IIO_MOD_ ## _axis, \
> +	.address = KMX61_MAG, \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
> +				BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> +	.scan_index = _index, \
> +	.scan_type = { \
> +		.sign = 's', \
> +		.realbits = 14, \
> +		.storagebits = 16, \
> +		.shift = 2, \
> +		.endianness = IIO_LE, \
> +	}, \
> +}
> +
> +static const struct iio_chan_spec kmx61_channels[] = {
> +	KMX61_ACC_CHAN(X, KMX61_SCAN_ACC_X),
> +	KMX61_ACC_CHAN(Y, KMX61_SCAN_ACC_Y),
> +	KMX61_ACC_CHAN(Z, KMX61_SCAN_ACC_Z),
> +	KMX61_MAG_CHAN(X, KMX61_SCAN_MAG_X),
> +	KMX61_MAG_CHAN(Y, KMX61_SCAN_MAG_Y),
> +	KMX61_MAG_CHAN(Z, KMX61_SCAN_MAG_Z),
> +};
> +
> +static int kmx61_convert_freq_to_bit(int val, int val2)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(kmx61_samp_freq_table); i++)
> +		if (val == kmx61_samp_freq_table[i].val &&
> +		    val2 == kmx61_samp_freq_table[i].val2)
> +			return kmx61_samp_freq_table[i].odr_bits;
> +	return -EINVAL;
> +}
> +/**
> + * kmx61_set_mode() - set KMX61 device operating mode
> + * @data - kmx61 device private data pointer
> + * @mode - bitmask, indicating operating mode for @device
> + * @device - bitmask, indicating device for which @mode needs to be set
> + * @update - update stby bits stored in device's private  @data
> + *
> + * For each sensor (accelerometer/magnetometer) there are two operating modes
> + * STANDBY and OPERATION. Neither accel nor magn can be disabled independently
> + * if they are both enabled. Internal sensors state is saved in acc_stby and
> + * mag_stby members of driver's private @data.
> + */
> +static int kmx61_set_mode(struct kmx61_data *data, u8 mode, u8 device,
> +			  bool update)
> +{
> +	int ret;
> +	int acc_stby = -1, mag_stby = -1;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_STBY);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error reading reg_stby\n");
> +		return ret;
> +	}
> +	if (device & KMX61_ACC) {
> +		if (mode & KMX61_ACC_STBY_BIT) {
> +			ret |= KMX61_ACC_STBY_BIT;
> +			acc_stby = 1;
> +		} else {
> +			ret &= ~KMX61_ACC_STBY_BIT;
> +			acc_stby = 0;
> +		}
> +	}
> +
> +	if (device & KMX61_MAG) {
> +		if (mode & KMX61_MAG_STBY_BIT) {
> +			ret |= KMX61_MAG_STBY_BIT;
> +			mag_stby = 1;
> +		} else {
> +			ret &= ~KMX61_MAG_STBY_BIT;
> +			mag_stby = 0;
> +		}
> +	}
> +
> +	ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_STBY, ret);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error writing reg_stby\n");
> +		return ret;
> +	}
> +
> +	if (acc_stby != -1 && update)
> +		data->acc_stby = !!acc_stby;
> +	if (mag_stby != -1 && update)
> +		data->mag_stby = !!mag_stby;
> +
> +	return ret;
> +}
> +
> +static int kmx61_get_mode(struct kmx61_data *data, u8 *mode, u8 device)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_STBY);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error reading reg_stby\n");
> +		return ret;
> +	}
> +	*mode = 0;
> +
> +	if (device & KMX61_ACC) {
> +		if (ret & KMX61_ACC_STBY_BIT)
> +			*mode |= KMX61_ACC_STBY_BIT;
> +		else
> +			*mode &= ~KMX61_ACC_STBY_BIT;
> +	}
> +
> +	if (device & KMX61_MAG) {
> +		if (ret & KMX61_MAG_STBY_BIT)
> +			*mode |= KMX61_MAG_STBY_BIT;
> +		else
> +			*mode &= ~KMX61_MAG_STBY_BIT;
> +	}
> +
> +	return 0;
> +}
> +
> +static int kmx61_set_odr(struct kmx61_data *data, int val, int val2, u8 device)
> +{
> +	int ret;
> +	u8 mode;
> +	int lodr_bits, odr_bits;
> +
> +	ret = kmx61_get_mode(data, &mode, KMX61_ACC | KMX61_MAG);
> +	if (ret < 0)
> +		return ret;
> +
> +	lodr_bits = kmx61_convert_freq_to_bit(val, val2);
> +	if (lodr_bits < 0)
> +		return lodr_bits;
> +
> +	/* To change ODR, accel and magn must be in STDBY */
> +	ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG,
> +			     true);
> +	if (ret < 0)
> +		return ret;
> +
> +	odr_bits = 0;
> +	if (device & KMX61_ACC)
> +		odr_bits |= lodr_bits;
> +	if (device & KMX61_MAG)
> +		odr_bits |= (lodr_bits << KMX61_MAG_ODR_SHIFT);
> +
> +	ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_ODCNTL,
> +					odr_bits);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = kmx61_set_mode(data, mode, KMX61_ACC | KMX61_MAG, true);
> +	if (ret < 0)
> +		return ret;
> +
> +	data->odr_bits = lodr_bits;
> +
> +	return 0;
> +}
> +
> +static
> +int kmx61_get_odr(struct kmx61_data *data, int *val, int *val2, u8 device)
> +{	int i;
> +	u8 lodr_bits;
> +
> +	if (device & KMX61_ACC)
> +		lodr_bits = (data->odr_bits >> KMX61_ACC_ODR_SHIFT) &
> +			     KMX61_ACC_ODR_MASK;
> +	else if (device & KMX61_MAG)
> +		lodr_bits = (data->odr_bits >> KMX61_MAG_ODR_SHIFT) &
> +			     KMX61_MAG_ODR_MASK;
> +	else
> +		return -EINVAL;
> +
> +	for (i = 0; i < ARRAY_SIZE(kmx61_samp_freq_table); i++)
> +		if (lodr_bits == kmx61_samp_freq_table[i].odr_bits) {
> +			*val = kmx61_samp_freq_table[i].val;
> +			*val2 = kmx61_samp_freq_table[i].val2;
> +			return 0;
> +		}
> +	return -EINVAL;
> +}
> +
> +static int kmx61_set_range(struct kmx61_data *data, int range)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_CTRL1);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
> +		return ret;
> +	}
> +
> +	ret &= ~(KMX61_REG_CTRL1_GSEL0_MASK | KMX61_REG_CTRL1_GSEL1_MASK);
> +	ret |= kmx61_scale_table[range].gsel0 << KMX61_REG_CTRL1_GSEL0_SHIFT;
> +	ret |= kmx61_scale_table[range].gsel1 << KMX61_REG_CTRL1_GSEL1_SHIFT;
> +
> +	ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_CTRL1, ret);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error writing reg_ctrl1\n");
> +		return ret;
> +	}
> +
> +	data->range = range;
> +
> +	return 0;
> +}
> +
> +static int kmx61_set_scale(struct kmx61_data *data, int uscale)
> +{
> +	int ret, i;
> +	u8  mode;
> +
> +	for (i = 0; i < ARRAY_SIZE(kmx61_scale_table); i++) {
> +		if (kmx61_scale_table[i].uscale == uscale) {
> +			ret = kmx61_get_mode(data, &mode,
> +					     KMX61_ACC | KMX61_MAG);
> +			if (ret < 0)
> +				return ret;
> +
> +			ret = kmx61_set_mode(data, KMX61_ALL_STBY,
> +					     KMX61_ACC | KMX61_MAG, true);
> +			if (ret < 0)
> +				return ret;
> +
> +			ret = kmx61_set_range(data, i);
> +			if (ret < 0)
> +				return ret;
> +
> +			return  kmx61_set_mode(data, mode,
> +					       KMX61_ACC | KMX61_MAG, true);
> +		}
> +	}
> +	return -EINVAL;
> +}
> +
> +static int kmx61_chip_init(struct kmx61_data *data)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_WHO_AM_I);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error reading who_am_i\n");
> +		return ret;
> +	}
> +
> +	if (ret != KMX61_CHIP_ID) {
> +		dev_err(&data->client->dev,
> +			"Wrong chip id, got %x expected %x\n",
> +			 ret, KMX61_CHIP_ID);
> +		return -EINVAL;
> +	}
> +
> +	/* set accel 12bit, 4g range */
> +	ret = kmx61_set_range(data, KMX61_RANGE_4G);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* set acc/magn to OPERATION mode */
> +	ret = kmx61_set_mode(data, 0, KMX61_ACC | KMX61_MAG, true);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +/**
> + * kmx61_set_power_state() - set power state for kmx61 @device
> + * @data - kmx61 device private pointer
> + * @on - power state to be set for @device
> + * @device - bitmask indicating device for which @on state needs to be set
> + *
> + * Notice that when ACC power state needs to be set to ON and MAG is in
> + * OPERATION then we know that kmx61_runtime_resume was already called
> + * so we must set ACC OPERATION mode here. The same happens when MAG power
> + * state needs to be set to ON and ACC is in OPERATION.
> + */
> +static int kmx61_set_power_state(struct kmx61_data *data, bool on, u8 device)
> +{
> +#ifdef CONFIG_PM_RUNTIME
> +	int ret;
> +
> +	if (device & KMX61_ACC) {
> +		if (on && !data->acc_ps && !data->mag_stby)
> +			kmx61_set_mode(data, 0, KMX61_ACC, true);
> +		data->acc_ps = on;
> +	}
> +	if (device & KMX61_MAG) {
> +		if (on && !data->mag_ps && !data->acc_stby)
> +			kmx61_set_mode(data, 0, KMX61_MAG, true);
> +		data->mag_ps = on;
> +	}
> +
> +	if (on) {
> +		ret = pm_runtime_get_sync(&data->client->dev);
> +	} else {
> +		pm_runtime_mark_last_busy(&data->client->dev);
> +		ret = pm_runtime_put_autosuspend(&data->client->dev);
> +	}
> +	if (ret < 0) {
> +		dev_err(&data->client->dev,
> +			"Failed: kmx61_set_power_state for %d, ret %d\n",
> +			on, ret);
> +		return ret;
> +	}
> +#endif
> +	return 0;
> +}
> +
> +static int kmx61_read_measurement(struct kmx61_data *data, int base, int offset)
> +{
> +	int ret;
> +	u8 reg = base + offset * 2;
> +
> +	ret = i2c_smbus_read_word_data(data->client, reg);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "failed to read reg at %x\n", reg);
> +		return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +static int kmx61_read_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan, int *val,
> +			      int *val2, long mask)
> +{
> +	struct kmx61_data *data = iio_priv(indio_dev);
> +	int ret;
> +	u8 base_reg;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_ACCEL:
> +		case IIO_MAGN:
> +			base_reg = KMX61_ACC_XOUT_L;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		mutex_lock(&data->lock);
> +
> +		kmx61_set_power_state(data, true, chan->address);
> +		ret = kmx61_read_measurement(data, base_reg, chan->scan_index);
> +		if (ret < 0) {
> +			kmx61_set_power_state(data, false, chan->address);
> +			mutex_unlock(&data->lock);
> +			return ret;
> +		}
> +		*val = sign_extend32(ret >> chan->scan_type.shift,
> +				     chan->scan_type.realbits - 1);
> +		kmx61_set_power_state(data, false, chan->address);
> +
> +		mutex_unlock(&data->lock);
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_ACCEL:
> +			*val = 0;
> +			*val2 = kmx61_scale_table[data->range].uscale;
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		case IIO_MAGN:
> +			/* 14 bits res, 1465 microGauss per magn count */
> +			*val = 0;
> +			*val2 = 1465;
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (chan->type != IIO_ACCEL && chan->type != IIO_MAGN)
> +			return -EINVAL;
> +
> +		mutex_lock(&data->lock);
> +		ret = kmx61_get_odr(data, val, val2, chan->address);
> +		mutex_unlock(&data->lock);
> +		if (ret)
> +			return -EINVAL;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	}
> +	return -EINVAL;
> +}
> +
> +static int kmx61_write_raw(struct iio_dev *indio_dev,
> +			       struct iio_chan_spec const *chan, int val,
> +			       int val2, long mask)
> +{
> +	struct kmx61_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (chan->type != IIO_ACCEL && chan->type != IIO_MAGN)
> +			return -EINVAL;
> +
> +		mutex_lock(&data->lock);
> +		ret = kmx61_set_odr(data, val, val2, chan->address);
> +		mutex_unlock(&data->lock);
> +		return ret;
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_ACCEL:
> +			if (val != 0)
> +				return -EINVAL;
> +			mutex_lock(&data->lock);
> +			ret = kmx61_set_scale(data, val2);
> +			mutex_unlock(&data->lock);
> +			return ret;
> +		default:
> +			return -EINVAL;
> +		}
> +		return ret;
> +	default:
> +		return -EINVAL;
> +	}
> +	return ret;
> +}
> +
> +static const struct iio_info kmx61_info = {
> +	.driver_module		= THIS_MODULE,
> +	.read_raw		= kmx61_read_raw,
> +	.write_raw		= kmx61_write_raw,
> +	.attrs			= &kmx61_attribute_group,
> +};
> +
> +static int kmx61_probe(struct i2c_client *client,
> +		       const struct i2c_device_id *id)
> +{
> +	struct kmx61_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +	const char *name = NULL;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +	data->client = client;
> +
> +	if (id)
> +		name = id->name;
> +
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->channels = kmx61_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(kmx61_channels);
> +	indio_dev->name = name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &kmx61_info;
> +
> +	mutex_init(&data->lock);
> +
> +	ret = kmx61_chip_init(data);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Failed to register iio device\n");
> +		goto err_iio_device_register;
> +	}
> +
> +	ret = pm_runtime_set_active(&client->dev);
> +	if (ret < 0)
> +		goto err_pm_runtime_set_active;
> +
> +	pm_runtime_enable(&client->dev);
> +	pm_runtime_set_autosuspend_delay(&client->dev, KMX61_SLEEP_DELAY_MS);
> +	pm_runtime_use_autosuspend(&client->dev);
> +
> +	return 0;
> +
> +err_pm_runtime_set_active:
> +	iio_device_unregister(indio_dev);
> +err_iio_device_register:
> +	kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
> +	return ret;
> +}
> +
> +static int kmx61_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct kmx61_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	pm_runtime_disable(&client->dev);
> +	pm_runtime_set_suspended(&client->dev);
> +	pm_runtime_put_noidle(&client->dev);
> +
> +	iio_device_unregister(indio_dev);
> +
> +	mutex_lock(&data->lock);
> +	ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int kmx61_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct kmx61_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&data->lock);
> +	ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG,
> +			     false);
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
> +
> +static int kmx61_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct kmx61_data *data = iio_priv(indio_dev);
> +	u8 stby = 0;
> +
> +	if (data->acc_stby)
> +		stby |= KMX61_ACC_STBY_BIT;
> +	if (data->mag_stby)
> +		stby |= KMX61_MAG_STBY_BIT;
> +
> +	return kmx61_set_mode(data, stby, KMX61_ACC | KMX61_MAG, true);
> +}
> +#endif
> +
> +#ifdef CONFIG_PM_RUNTIME
> +static int kmx61_runtime_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct kmx61_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&data->lock);
> +	ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
> +
> +static int kmx61_runtime_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct kmx61_data *data = iio_priv(indio_dev);
> +	u8 stby = 0;
> +
> +	if (!data->acc_ps)
> +		stby |= KMX61_ACC_STBY_BIT;
> +	if (!data->mag_ps)
> +		stby |= KMX61_MAG_STBY_BIT;
> +
> +	return kmx61_set_mode(data, stby, KMX61_ACC | KMX61_MAG, true);
> +}
> +#endif
> +
> +static const struct dev_pm_ops kmx61_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(kmx61_suspend, kmx61_resume)
> +	SET_RUNTIME_PM_OPS(kmx61_runtime_suspend, kmx61_runtime_resume, NULL)
> +};
> +
> +static const struct i2c_device_id kmx61_id[] = {
> +	{"kmx611021", 0},
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, kmx61_id);
> +
> +static struct i2c_driver kmx61_driver = {
> +	.driver = {
> +		.name = KMX61_DRV_NAME,
> +		.pm = &kmx61_pm_ops,
> +	},
> +	.probe		= kmx61_probe,
> +	.remove		= kmx61_remove,
> +	.id_table	= kmx61_id,
> +};
> +
> +module_i2c_driver(kmx61_driver);
> +
> +MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com>");
> +MODULE_DESCRIPTION("KMX61 accelerometer/magnetometer driver");
> +MODULE_LICENSE("GPL v2");
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] iio: imu: Add support for Kionix KMX61 sensor
  2014-11-18 16:47 [PATCH v3] iio: imu: Add support for Kionix KMX61 sensor Daniel Baluta
  2014-11-22 10:46 ` Jonathan Cameron
@ 2014-11-25  0:45 ` Hartmut Knaack
  2014-11-25  9:54   ` Daniel Baluta
  1 sibling, 1 reply; 7+ messages in thread
From: Hartmut Knaack @ 2014-11-25  0:45 UTC (permalink / raw)
  To: Daniel Baluta, jic23, pmeerw, srinivas.pandruvada; +Cc: linux-iio, linux-kernel

Daniel Baluta schrieb am 18.11.2014 17:47:
> Minimal implementation for KMX61 6-axis accelerometer/magnetometer. It exports
> raw accel/magn readings together with scale and sampling frequency.
> 
> Datasheet will be available at:
> http://www.kionix.com/6-axis-accelerometer-magnetometer/kmx61
> 
Finally managed to have a close look, and found a few issues.
> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
> ---
> Changes since v2:
> 	* mostly style fixes after comments received from Jonathan and Peter
> 		* http://marc.info/?l=linux-iio&m=141564019602400&w=2
> 	* implemented PM runtime and PM sleep resume/suspend hooks
> 	* introduced new parameter for kmx61_set_mode to help updating
> 	stby bits in kmx61_data.
> 	* use kmx6111021 instead of kmx61 for i2c_device_id.
> 
>  drivers/iio/imu/Kconfig  |   9 +
>  drivers/iio/imu/Makefile |   2 +
>  drivers/iio/imu/kmx61.c  | 766 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 777 insertions(+)
>  create mode 100644 drivers/iio/imu/kmx61.c
> 
> diff --git a/drivers/iio/imu/Kconfig b/drivers/iio/imu/Kconfig
> index 2b0e451..d675f43 100644
> --- a/drivers/iio/imu/Kconfig
> +++ b/drivers/iio/imu/Kconfig
> @@ -25,6 +25,15 @@ config ADIS16480
>  	  Say yes here to build support for Analog Devices ADIS16375, ADIS16480,
>  	  ADIS16485, ADIS16488 inertial sensors.
>  
> +config KMX61
> +	tristate "Kionix KMX61 6-axis accelerometer and magnetometer"
> +	depends on I2C
> +	help
> +	  Say Y here if you want to build a driver for Kionix KMX61 6-axis accelerometer
> +	  and magnetometer.
> +	  To compile this driver as module, choose M here: the module will be called
> +	  kmx61.
> +
This help text is more than 80 characters wide and may better be wrapped a bit differently.
>  source "drivers/iio/imu/inv_mpu6050/Kconfig"
>  
>  endmenu
> diff --git a/drivers/iio/imu/Makefile b/drivers/iio/imu/Makefile
> index 114d2c1..e1e6e3d 100644
> --- a/drivers/iio/imu/Makefile
> +++ b/drivers/iio/imu/Makefile
> @@ -14,3 +14,5 @@ adis_lib-$(CONFIG_IIO_ADIS_LIB_BUFFER) += adis_buffer.o
>  obj-$(CONFIG_IIO_ADIS_LIB) += adis_lib.o
>  
>  obj-y += inv_mpu6050/
> +
> +obj-$(CONFIG_KMX61) += kmx61.o
> diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c
> new file mode 100644
> index 0000000..f68b3ef
> --- /dev/null
> +++ b/drivers/iio/imu/kmx61.c
> @@ -0,0 +1,766 @@
> +/*
> + * KMX61 - Kionix 6-axis Accelerometer/Magnetometer
> + *
> + * Copyright (c) 2014, 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.
> + *
> + * IIO driver for KMX61 (7-bit I2C slave address 0x0E or 0x0F).
> + *
> + * TODO: buffer, interrupt, thresholds, acpi, temperature sensor
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define KMX61_DRV_NAME "kmx61"
> +
> +#define KMX61_REG_WHO_AM_I	0x00
> +
> +/*
> + * three 16-bit accelerometer output registers for X/Y/Z axis
> + * we use only XOUT_L as a base register, all other addresses
> + * can be obtained by applying an offset and are provided here
> + * only for clarity.
> + */
> +#define KMX61_ACC_XOUT_L	0x0A
> +#define KMX61_ACC_XOUT_H	0x0B
> +#define KMX61_ACC_YOUT_L	0x0C
> +#define KMX61_ACC_YOUT_H	0x0D
> +#define KMX61_ACC_ZOUT_L	0x0E
> +#define KMX61_ACC_ZOUT_H	0x0F
> +
> +/*
> + * one 16-bit temperature output register
> + */
> +#define KMX61_TEMP_L		0x10
> +#define KMX61_TEMP_H		0x11
> +
> +/*
> + * three 16-bit magnetometer output registers for X/Y/Z axis
> + */
> +#define KMX61_MAG_XOUT_L	0x12
> +#define KMX61_MAG_XOUT_H	0x13
> +#define KMX61_MAG_YOUT_L	0x14
> +#define KMX61_MAG_YOUT_H	0x15
> +#define KMX61_MAG_ZOUT_L	0x16
> +#define KMX61_MAG_ZOUT_H	0x17
> +
> +#define KMX61_REG_ODCNTL	0x2C
> +#define KMX61_REG_STBY		0x29
> +#define KMX61_REG_CTRL1		0x2A
> +
> +#define KMX61_ACC_STBY_BIT	BIT(0)
> +#define KMX61_MAG_STBY_BIT	BIT(1)
> +#define KMX61_ACT_STBY_BIT	BIT(7)
> +
> +#define KMX61_ALL_STBY		(KMX61_ACC_STBY_BIT | KMX61_MAG_STBY_BIT)
> +
> +#define KMX61_REG_CTRL1_GSEL0_SHIFT	0
> +#define KMX61_REG_CTRL1_GSEL1_SHIFT	1
> +#define KMX61_REG_CTRL1_GSEL0_MASK	0x01
> +#define KMX61_REG_CTRL1_GSEL1_MASK	0x02
I don't see the benefit of separating GSEL0 and GSEL1.
> +
> +#define KMX61_REG_CTRL1_BIT_RES		BIT(4)
> +
> +#define KMX61_ACC_ODR_SHIFT	0
> +#define KMX61_MAG_ODR_SHIFT	4
> +#define KMX61_ACC_ODR_MASK	0x0F
> +#define KMX61_MAG_ODR_MASK	0xF0
> +
> +#define KMX61_SLEEP_DELAY_MS	2000
> +
> +#define KMX61_CHIP_ID		0x12
> +
> +struct kmx61_data {
> +	struct i2c_client *client;
> +
> +	/* serialize access to non-atomic ops, e.g set_mode */
> +	struct mutex lock;
> +	u8 range;
> +	u8 odr_bits;
> +
> +	/* standby state */
> +	u8 acc_stby;
> +	u8 mag_stby;
Why u8 instead of bool?
> +
> +	/* power state */
> +	bool acc_ps;
> +	bool mag_ps;
> +};
> +
> +enum kmx61_range {
> +	KMX61_RANGE_2G,
> +	KMX61_RANGE_4G,
> +	KMX61_RANGE_8G,
> +};
> +
> +enum kmx61_scan {
> +	KMX61_SCAN_ACC_X,
> +	KMX61_SCAN_ACC_Y,
> +	KMX61_SCAN_ACC_Z,
> +	KMX61_SCAN_TEMP,
> +	KMX61_SCAN_MAG_X,
> +	KMX61_SCAN_MAG_Y,
> +	KMX61_SCAN_MAG_Z,
> +};
> +
> +static const struct {
> +	u16 uscale;
> +	u8 gsel0;
> +	u8 gsel1;
> +} kmx61_scale_table[] = {
> +	{9582, 0, 0},
> +	{19163, 1, 0},
> +	{38326, 0, 1},
Why not consolidate gsel0 and gsel1 to gsel? Then this could become u16 kmx61_scale[] = {9582, 19163, 38326} with gsel as index.
> +};
> +
> +/* KMX61 devices */
> +#define KMX61_ACC	0x01
> +#define KMX61_MAG	0x02
> +
> +static const struct {
> +	int val;
> +	int val2;
> +	u8 odr_bits;
> +} kmx61_samp_freq_table[] = { {12, 500000, 0x00},
> +			{25, 0, 0x01},
> +			{50, 0, 0x02},
> +			{100, 0, 0x03},
> +			{200, 0, 0x04},
> +			{400, 0, 0x05},
> +			{800, 0, 0x06},
> +			{1600, 0, 0x07},
> +			{0, 781000, 0x08},
> +			{1, 563000, 0x09},
> +			{3, 125000, 0x0A},
> +			{6, 250000, 0x0B} };
> +
> +static IIO_CONST_ATTR(accel_scale_available, "0.009582 0.019163 0.038326");
> +static IIO_CONST_ATTR(magn_scale_available, "0.001465");
> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
> +	"0.781000 1.563000 3.125000 6.250000 12.500000 25 50 100 200 400 800");
> +
> +static struct attribute *kmx61_attributes[] = {
> +	&iio_const_attr_accel_scale_available.dev_attr.attr,
> +	&iio_const_attr_magn_scale_available.dev_attr.attr,
> +	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group kmx61_attribute_group = {
> +	.attrs = kmx61_attributes,
> +};
> +
> +#define KMX61_ACC_CHAN(_axis, _index) { \
> +	.type = IIO_ACCEL, \
> +	.modified = 1, \
> +	.channel2 = IIO_MOD_ ## _axis, \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
> +				BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> +	.address = KMX61_ACC, \
> +	.scan_index = _index, \
> +	.scan_type = { \
> +		.sign = 's', \
> +		.realbits = 12, \
> +		.storagebits = 16, \
> +		.shift = 4, \
> +		.endianness = IIO_LE, \
> +	}, \
> +}
> +
> +#define KMX61_MAG_CHAN(_axis, _index) { \
> +	.type = IIO_MAGN, \
> +	.modified = 1, \
> +	.channel2 = IIO_MOD_ ## _axis, \
> +	.address = KMX61_MAG, \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
> +				BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> +	.scan_index = _index, \
> +	.scan_type = { \
> +		.sign = 's', \
> +		.realbits = 14, \
> +		.storagebits = 16, \
> +		.shift = 2, \
> +		.endianness = IIO_LE, \
> +	}, \
> +}
> +
> +static const struct iio_chan_spec kmx61_channels[] = {
> +	KMX61_ACC_CHAN(X, KMX61_SCAN_ACC_X),
> +	KMX61_ACC_CHAN(Y, KMX61_SCAN_ACC_Y),
> +	KMX61_ACC_CHAN(Z, KMX61_SCAN_ACC_Z),
> +	KMX61_MAG_CHAN(X, KMX61_SCAN_MAG_X),
> +	KMX61_MAG_CHAN(Y, KMX61_SCAN_MAG_Y),
> +	KMX61_MAG_CHAN(Z, KMX61_SCAN_MAG_Z),
> +};
> +
> +static int kmx61_convert_freq_to_bit(int val, int val2)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(kmx61_samp_freq_table); i++)
> +		if (val == kmx61_samp_freq_table[i].val &&
> +		    val2 == kmx61_samp_freq_table[i].val2)
> +			return kmx61_samp_freq_table[i].odr_bits;
> +	return -EINVAL;
> +}
> +/**
Like here, the first line of the following multiline comments have one asterisk too much.
> + * kmx61_set_mode() - set KMX61 device operating mode
> + * @data - kmx61 device private data pointer
> + * @mode - bitmask, indicating operating mode for @device
> + * @device - bitmask, indicating device for which @mode needs to be set
> + * @update - update stby bits stored in device's private  @data
> + *
> + * For each sensor (accelerometer/magnetometer) there are two operating modes
> + * STANDBY and OPERATION. Neither accel nor magn can be disabled independently
> + * if they are both enabled. Internal sensors state is saved in acc_stby and
> + * mag_stby members of driver's private @data.
> + */
> +static int kmx61_set_mode(struct kmx61_data *data, u8 mode, u8 device,
> +			  bool update)
> +{
> +	int ret;
> +	int acc_stby = -1, mag_stby = -1;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_STBY);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error reading reg_stby\n");
> +		return ret;
> +	}
> +	if (device & KMX61_ACC) {
> +		if (mode & KMX61_ACC_STBY_BIT) {
> +			ret |= KMX61_ACC_STBY_BIT;
> +			acc_stby = 1;
> +		} else {
> +			ret &= ~KMX61_ACC_STBY_BIT;
> +			acc_stby = 0;
> +		}
> +	}
> +
> +	if (device & KMX61_MAG) {
> +		if (mode & KMX61_MAG_STBY_BIT) {
> +			ret |= KMX61_MAG_STBY_BIT;
> +			mag_stby = 1;
> +		} else {
> +			ret &= ~KMX61_MAG_STBY_BIT;
> +			mag_stby = 0;
> +		}
> +	}
> +
> +	ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_STBY, ret);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error writing reg_stby\n");
> +		return ret;
> +	}
> +
> +	if (acc_stby != -1 && update)
> +		data->acc_stby = !!acc_stby;
> +	if (mag_stby != -1 && update)
> +		data->mag_stby = !!mag_stby;
Why is there a need for double negation?
> +
> +	return ret;
Since no error occured until this point, just return 0.
> +}
> +
> +static int kmx61_get_mode(struct kmx61_data *data, u8 *mode, u8 device)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_STBY);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error reading reg_stby\n");
> +		return ret;
> +	}
> +	*mode = 0;
> +
> +	if (device & KMX61_ACC) {
> +		if (ret & KMX61_ACC_STBY_BIT)
> +			*mode |= KMX61_ACC_STBY_BIT;
> +		else
> +			*mode &= ~KMX61_ACC_STBY_BIT;
Simply:
		*mode |= ret & KMX61_ACC_STBY_BIT;
> +	}
> +
> +	if (device & KMX61_MAG) {
> +		if (ret & KMX61_MAG_STBY_BIT)
> +			*mode |= KMX61_MAG_STBY_BIT;
> +		else
> +			*mode &= ~KMX61_MAG_STBY_BIT;
Same here:
		*mode |= ret & KMX61_MAG_STBY_BIT;
> +	}
> +
> +	return 0;
> +}
> +
> +static int kmx61_set_odr(struct kmx61_data *data, int val, int val2, u8 device)
> +{
> +	int ret;
> +	u8 mode;
> +	int lodr_bits, odr_bits;
> +
> +	ret = kmx61_get_mode(data, &mode, KMX61_ACC | KMX61_MAG);
> +	if (ret < 0)
> +		return ret;
> +
> +	lodr_bits = kmx61_convert_freq_to_bit(val, val2);
> +	if (lodr_bits < 0)
> +		return lodr_bits;
> +
> +	/* To change ODR, accel and magn must be in STDBY */
> +	ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG,
> +			     true);
> +	if (ret < 0)
> +		return ret;
> +
> +	odr_bits = 0;
> +	if (device & KMX61_ACC)
> +		odr_bits |= lodr_bits;
> +	if (device & KMX61_MAG)
> +		odr_bits |= (lodr_bits << KMX61_MAG_ODR_SHIFT);
No need for parenthesis.
> +
> +	ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_ODCNTL,
> +					odr_bits);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = kmx61_set_mode(data, mode, KMX61_ACC | KMX61_MAG, true);
> +	if (ret < 0)
> +		return ret;
> +
> +	data->odr_bits = lodr_bits;
> +
> +	return 0;
> +}
> +
> +static
> +int kmx61_get_odr(struct kmx61_data *data, int *val, int *val2, u8 device)
Usually wrap parameters? And don't put code on line with {.
> +{	int i;
> +	u8 lodr_bits;
> +
> +	if (device & KMX61_ACC)
> +		lodr_bits = (data->odr_bits >> KMX61_ACC_ODR_SHIFT) &
> +			     KMX61_ACC_ODR_MASK;
KMX61_ACC_ODR_SHIFT is just a placebo and not used in kmx61_set_odr. Choose one way and stay consistent.
> +	else if (device & KMX61_MAG)
> +		lodr_bits = (data->odr_bits >> KMX61_MAG_ODR_SHIFT) &
> +			     KMX61_MAG_ODR_MASK;
> +	else
> +		return -EINVAL;
> +
> +	for (i = 0; i < ARRAY_SIZE(kmx61_samp_freq_table); i++)
> +		if (lodr_bits == kmx61_samp_freq_table[i].odr_bits) {
> +			*val = kmx61_samp_freq_table[i].val;
> +			*val2 = kmx61_samp_freq_table[i].val2;
> +			return 0;
return IIO_VAL_INT_PLUS_MICRO allows you to faciliate _read_raw a bit to just return ret there.
> +		}
> +	return -EINVAL;
> +}
> +
> +static int kmx61_set_range(struct kmx61_data *data, int range)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_CTRL1);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
> +		return ret;
> +	}
> +
> +	ret &= ~(KMX61_REG_CTRL1_GSEL0_MASK | KMX61_REG_CTRL1_GSEL1_MASK);
> +	ret |= kmx61_scale_table[range].gsel0 << KMX61_REG_CTRL1_GSEL0_SHIFT;
> +	ret |= kmx61_scale_table[range].gsel1 << KMX61_REG_CTRL1_GSEL1_SHIFT;
> +
> +	ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_CTRL1, ret);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error writing reg_ctrl1\n");
> +		return ret;
> +	}
> +
> +	data->range = range;
> +
> +	return 0;
> +}
> +
> +static int kmx61_set_scale(struct kmx61_data *data, int uscale)
> +{
> +	int ret, i;
> +	u8  mode;
> +
> +	for (i = 0; i < ARRAY_SIZE(kmx61_scale_table); i++) {
> +		if (kmx61_scale_table[i].uscale == uscale) {
> +			ret = kmx61_get_mode(data, &mode,
> +					     KMX61_ACC | KMX61_MAG);
> +			if (ret < 0)
> +				return ret;
> +
> +			ret = kmx61_set_mode(data, KMX61_ALL_STBY,
> +					     KMX61_ACC | KMX61_MAG, true);
> +			if (ret < 0)
> +				return ret;
> +
> +			ret = kmx61_set_range(data, i);
> +			if (ret < 0)
> +				return ret;
> +
> +			return  kmx61_set_mode(data, mode,
> +					       KMX61_ACC | KMX61_MAG, true);
> +		}
> +	}
> +	return -EINVAL;
> +}
> +
> +static int kmx61_chip_init(struct kmx61_data *data)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_WHO_AM_I);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "Error reading who_am_i\n");
> +		return ret;
> +	}
> +
> +	if (ret != KMX61_CHIP_ID) {
> +		dev_err(&data->client->dev,
> +			"Wrong chip id, got %x expected %x\n",
> +			 ret, KMX61_CHIP_ID);
> +		return -EINVAL;
> +	}
> +
> +	/* set accel 12bit, 4g range */
> +	ret = kmx61_set_range(data, KMX61_RANGE_4G);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* set acc/magn to OPERATION mode */
> +	ret = kmx61_set_mode(data, 0, KMX61_ACC | KMX61_MAG, true);
Just return kmx61_set_mode(data, 0, KMX61_ACC | KMX61_MAG, true);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +/**
> + * kmx61_set_power_state() - set power state for kmx61 @device
> + * @data - kmx61 device private pointer
> + * @on - power state to be set for @device
> + * @device - bitmask indicating device for which @on state needs to be set
> + *
> + * Notice that when ACC power state needs to be set to ON and MAG is in
> + * OPERATION then we know that kmx61_runtime_resume was already called
> + * so we must set ACC OPERATION mode here. The same happens when MAG power
> + * state needs to be set to ON and ACC is in OPERATION.
> + */
> +static int kmx61_set_power_state(struct kmx61_data *data, bool on, u8 device)
> +{
> +#ifdef CONFIG_PM_RUNTIME
> +	int ret;
> +
> +	if (device & KMX61_ACC) {
> +		if (on && !data->acc_ps && !data->mag_stby)
> +			kmx61_set_mode(data, 0, KMX61_ACC, true);
> +		data->acc_ps = on;
> +	}
> +	if (device & KMX61_MAG) {
> +		if (on && !data->mag_ps && !data->acc_stby)
> +			kmx61_set_mode(data, 0, KMX61_MAG, true);
> +		data->mag_ps = on;
> +	}
> +
> +	if (on) {
> +		ret = pm_runtime_get_sync(&data->client->dev);
> +	} else {
> +		pm_runtime_mark_last_busy(&data->client->dev);
> +		ret = pm_runtime_put_autosuspend(&data->client->dev);
> +	}
> +	if (ret < 0) {
> +		dev_err(&data->client->dev,
> +			"Failed: kmx61_set_power_state for %d, ret %d\n",
> +			on, ret);
> +		return ret;
> +	}
> +#endif
> +	return 0;
> +}
> +
> +static int kmx61_read_measurement(struct kmx61_data *data, int base, int offset)
u8 base, u8 offset?
> +{
> +	int ret;
> +	u8 reg = base + offset * 2;
> +
> +	ret = i2c_smbus_read_word_data(data->client, reg);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "failed to read reg at %x\n", reg);
> +		return ret;
This return is unnecessary.
> +	}
> +
> +	return ret;
> +}
> +
> +static int kmx61_read_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan, int *val,
> +			      int *val2, long mask)
Indentation of the parameters is a bit too high.
> +{
> +	struct kmx61_data *data = iio_priv(indio_dev);
> +	int ret;
> +	u8 base_reg;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_ACCEL:
> +		case IIO_MAGN:
> +			base_reg = KMX61_ACC_XOUT_L;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		mutex_lock(&data->lock);
> +
> +		kmx61_set_power_state(data, true, chan->address);
> +		ret = kmx61_read_measurement(data, base_reg, chan->scan_index);
> +		if (ret < 0) {
> +			kmx61_set_power_state(data, false, chan->address);
> +			mutex_unlock(&data->lock);
> +			return ret;
> +		}
> +		*val = sign_extend32(ret >> chan->scan_type.shift,
> +				     chan->scan_type.realbits - 1);
> +		kmx61_set_power_state(data, false, chan->address);
> +
> +		mutex_unlock(&data->lock);
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_ACCEL:
> +			*val = 0;
> +			*val2 = kmx61_scale_table[data->range].uscale;
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		case IIO_MAGN:
> +			/* 14 bits res, 1465 microGauss per magn count */
> +			*val = 0;
> +			*val2 = 1465;
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (chan->type != IIO_ACCEL && chan->type != IIO_MAGN)
> +			return -EINVAL;
> +
> +		mutex_lock(&data->lock);
> +		ret = kmx61_get_odr(data, val, val2, chan->address);
> +		mutex_unlock(&data->lock);
Could be just return ret here, if kmx61_get_odr() returns IIO_VAL_INT_PLUS_MICRO on success and -EINVAL on error.
> +		if (ret)
> +			return -EINVAL;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	}
> +	return -EINVAL;
> +}
> +
> +static int kmx61_write_raw(struct iio_dev *indio_dev,
> +			       struct iio_chan_spec const *chan, int val,
> +			       int val2, long mask)
Indentation of parameters is too high.
> +{
> +	struct kmx61_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (chan->type != IIO_ACCEL && chan->type != IIO_MAGN)
> +			return -EINVAL;
> +
> +		mutex_lock(&data->lock);
> +		ret = kmx61_set_odr(data, val, val2, chan->address);
> +		mutex_unlock(&data->lock);
> +		return ret;
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_ACCEL:
> +			if (val != 0)
> +				return -EINVAL;
> +			mutex_lock(&data->lock);
> +			ret = kmx61_set_scale(data, val2);
> +			mutex_unlock(&data->lock);
> +			return ret;
> +		default:
> +			return -EINVAL;
> +		}
> +		return ret;
This won't be reached (if so, ret would not be initialized).
> +	default:
> +		return -EINVAL;
> +	}
> +	return ret;
Same here.
> +}
> +
> +static const struct iio_info kmx61_info = {
> +	.driver_module		= THIS_MODULE,
> +	.read_raw		= kmx61_read_raw,
> +	.write_raw		= kmx61_write_raw,
> +	.attrs			= &kmx61_attribute_group,
> +};
> +
> +static int kmx61_probe(struct i2c_client *client,
> +		       const struct i2c_device_id *id)
> +{
> +	struct kmx61_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +	const char *name = NULL;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +	data->client = client;
> +
> +	if (id)
> +		name = id->name;
> +
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->channels = kmx61_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(kmx61_channels);
> +	indio_dev->name = name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &kmx61_info;
> +
> +	mutex_init(&data->lock);
> +
> +	ret = kmx61_chip_init(data);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Failed to register iio device\n");
> +		goto err_iio_device_register;
> +	}
> +
> +	ret = pm_runtime_set_active(&client->dev);
> +	if (ret < 0)
> +		goto err_pm_runtime_set_active;
> +
> +	pm_runtime_enable(&client->dev);
> +	pm_runtime_set_autosuspend_delay(&client->dev, KMX61_SLEEP_DELAY_MS);
> +	pm_runtime_use_autosuspend(&client->dev);
> +
> +	return 0;
> +
> +err_pm_runtime_set_active:
> +	iio_device_unregister(indio_dev);
> +err_iio_device_register:
> +	kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
> +	return ret;
> +}
> +
> +static int kmx61_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct kmx61_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	pm_runtime_disable(&client->dev);
> +	pm_runtime_set_suspended(&client->dev);
> +	pm_runtime_put_noidle(&client->dev);
> +
> +	iio_device_unregister(indio_dev);
> +
> +	mutex_lock(&data->lock);
> +	ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int kmx61_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct kmx61_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&data->lock);
> +	ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG,
> +			     false);
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
> +
> +static int kmx61_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct kmx61_data *data = iio_priv(indio_dev);
> +	u8 stby = 0;
> +
> +	if (data->acc_stby)
> +		stby |= KMX61_ACC_STBY_BIT;
> +	if (data->mag_stby)
> +		stby |= KMX61_MAG_STBY_BIT;
> +
> +	return kmx61_set_mode(data, stby, KMX61_ACC | KMX61_MAG, true);
> +}
> +#endif
> +
> +#ifdef CONFIG_PM_RUNTIME
> +static int kmx61_runtime_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct kmx61_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&data->lock);
> +	ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
> +
> +static int kmx61_runtime_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct kmx61_data *data = iio_priv(indio_dev);
> +	u8 stby = 0;
> +
> +	if (!data->acc_ps)
> +		stby |= KMX61_ACC_STBY_BIT;
> +	if (!data->mag_ps)
> +		stby |= KMX61_MAG_STBY_BIT;
> +
> +	return kmx61_set_mode(data, stby, KMX61_ACC | KMX61_MAG, true);
> +}
> +#endif
> +
> +static const struct dev_pm_ops kmx61_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(kmx61_suspend, kmx61_resume)
> +	SET_RUNTIME_PM_OPS(kmx61_runtime_suspend, kmx61_runtime_resume, NULL)
> +};
> +
> +static const struct i2c_device_id kmx61_id[] = {
> +	{"kmx611021", 0},
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, kmx61_id);
> +
> +static struct i2c_driver kmx61_driver = {
> +	.driver = {
> +		.name = KMX61_DRV_NAME,
> +		.pm = &kmx61_pm_ops,
> +	},
> +	.probe		= kmx61_probe,
> +	.remove		= kmx61_remove,
> +	.id_table	= kmx61_id,
> +};
> +
> +module_i2c_driver(kmx61_driver);
> +
> +MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com>");
> +MODULE_DESCRIPTION("KMX61 accelerometer/magnetometer driver");
> +MODULE_LICENSE("GPL v2");
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] iio: imu: Add support for Kionix KMX61 sensor
  2014-11-25  0:45 ` Hartmut Knaack
@ 2014-11-25  9:54   ` Daniel Baluta
  2014-11-25 22:31     ` Hartmut Knaack
  0 siblings, 1 reply; 7+ messages in thread
From: Daniel Baluta @ 2014-11-25  9:54 UTC (permalink / raw)
  To: Hartmut Knaack
  Cc: Daniel Baluta, Jonathan Cameron, Peter Meerwald,
	Srinivas Pandruvada, linux-iio, Linux Kernel Mailing List

On Tue, Nov 25, 2014 at 2:45 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
> Daniel Baluta schrieb am 18.11.2014 17:47:
>> Minimal implementation for KMX61 6-axis accelerometer/magnetometer. It exports
>> raw accel/magn readings together with scale and sampling frequency.
>>
>> Datasheet will be available at:
>> http://www.kionix.com/6-axis-accelerometer-magnetometer/kmx61
>>
> Finally managed to have a close look, and found a few issues.
>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>> ---
>> Changes since v2:
>>       * mostly style fixes after comments received from Jonathan and Peter
>>               * http://marc.info/?l=linux-iio&m=141564019602400&w=2
>>       * implemented PM runtime and PM sleep resume/suspend hooks
>>       * introduced new parameter for kmx61_set_mode to help updating
>>       stby bits in kmx61_data.
>>       * use kmx6111021 instead of kmx61 for i2c_device_id.
>>
>>  drivers/iio/imu/Kconfig  |   9 +
>>  drivers/iio/imu/Makefile |   2 +
>>  drivers/iio/imu/kmx61.c  | 766 +++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 777 insertions(+)
>>  create mode 100644 drivers/iio/imu/kmx61.c
>>
>> diff --git a/drivers/iio/imu/Kconfig b/drivers/iio/imu/Kconfig
>> index 2b0e451..d675f43 100644
>> --- a/drivers/iio/imu/Kconfig
>> +++ b/drivers/iio/imu/Kconfig
>> @@ -25,6 +25,15 @@ config ADIS16480
>>         Say yes here to build support for Analog Devices ADIS16375, ADIS16480,
>>         ADIS16485, ADIS16488 inertial sensors.
>>
>> +config KMX61
>> +     tristate "Kionix KMX61 6-axis accelerometer and magnetometer"
>> +     depends on I2C
>> +     help
>> +       Say Y here if you want to build a driver for Kionix KMX61 6-axis accelerometer
>> +       and magnetometer.
>> +       To compile this driver as module, choose M here: the module will be called
>> +       kmx61.
>> +
> This help text is more than 80 characters wide and may better be wrapped a bit differently.

Good catch. checkpatch.pl didn't complain.

>>  source "drivers/iio/imu/inv_mpu6050/Kconfig"
>>
>>  endmenu
>> diff --git a/drivers/iio/imu/Makefile b/drivers/iio/imu/Makefile
>> index 114d2c1..e1e6e3d 100644
>> --- a/drivers/iio/imu/Makefile
>> +++ b/drivers/iio/imu/Makefile
>> @@ -14,3 +14,5 @@ adis_lib-$(CONFIG_IIO_ADIS_LIB_BUFFER) += adis_buffer.o
>>  obj-$(CONFIG_IIO_ADIS_LIB) += adis_lib.o
>>
>>  obj-y += inv_mpu6050/
>> +
>> +obj-$(CONFIG_KMX61) += kmx61.o
>> diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c
>> new file mode 100644
>> index 0000000..f68b3ef
>> --- /dev/null
>> +++ b/drivers/iio/imu/kmx61.c
>> @@ -0,0 +1,766 @@
>> +/*
>> + * KMX61 - Kionix 6-axis Accelerometer/Magnetometer
>> + *
>> + * Copyright (c) 2014, 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.
>> + *
>> + * IIO driver for KMX61 (7-bit I2C slave address 0x0E or 0x0F).
>> + *
>> + * TODO: buffer, interrupt, thresholds, acpi, temperature sensor
>> + *
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/i2c.h>
>> +#include <linux/pm.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +
>> +#define KMX61_DRV_NAME "kmx61"
>> +
>> +#define KMX61_REG_WHO_AM_I   0x00
>> +
>> +/*
>> + * three 16-bit accelerometer output registers for X/Y/Z axis
>> + * we use only XOUT_L as a base register, all other addresses
>> + * can be obtained by applying an offset and are provided here
>> + * only for clarity.
>> + */
>> +#define KMX61_ACC_XOUT_L     0x0A
>> +#define KMX61_ACC_XOUT_H     0x0B
>> +#define KMX61_ACC_YOUT_L     0x0C
>> +#define KMX61_ACC_YOUT_H     0x0D
>> +#define KMX61_ACC_ZOUT_L     0x0E
>> +#define KMX61_ACC_ZOUT_H     0x0F
>> +
>> +/*
>> + * one 16-bit temperature output register
>> + */
>> +#define KMX61_TEMP_L         0x10
>> +#define KMX61_TEMP_H         0x11
>> +
>> +/*
>> + * three 16-bit magnetometer output registers for X/Y/Z axis
>> + */
>> +#define KMX61_MAG_XOUT_L     0x12
>> +#define KMX61_MAG_XOUT_H     0x13
>> +#define KMX61_MAG_YOUT_L     0x14
>> +#define KMX61_MAG_YOUT_H     0x15
>> +#define KMX61_MAG_ZOUT_L     0x16
>> +#define KMX61_MAG_ZOUT_H     0x17
>> +
>> +#define KMX61_REG_ODCNTL     0x2C
>> +#define KMX61_REG_STBY               0x29
>> +#define KMX61_REG_CTRL1              0x2A
>> +
>> +#define KMX61_ACC_STBY_BIT   BIT(0)
>> +#define KMX61_MAG_STBY_BIT   BIT(1)
>> +#define KMX61_ACT_STBY_BIT   BIT(7)
>> +
>> +#define KMX61_ALL_STBY               (KMX61_ACC_STBY_BIT | KMX61_MAG_STBY_BIT)
>> +
>> +#define KMX61_REG_CTRL1_GSEL0_SHIFT  0
>> +#define KMX61_REG_CTRL1_GSEL1_SHIFT  1
>> +#define KMX61_REG_CTRL1_GSEL0_MASK   0x01
>> +#define KMX61_REG_CTRL1_GSEL1_MASK   0x02
> I don't see the benefit of separating GSEL0 and GSEL1.
>> +
>> +#define KMX61_REG_CTRL1_BIT_RES              BIT(4)
>> +
>> +#define KMX61_ACC_ODR_SHIFT  0
>> +#define KMX61_MAG_ODR_SHIFT  4
>> +#define KMX61_ACC_ODR_MASK   0x0F
>> +#define KMX61_MAG_ODR_MASK   0xF0
>> +
>> +#define KMX61_SLEEP_DELAY_MS 2000
>> +
>> +#define KMX61_CHIP_ID                0x12
>> +
>> +struct kmx61_data {
>> +     struct i2c_client *client;
>> +
>> +     /* serialize access to non-atomic ops, e.g set_mode */
>> +     struct mutex lock;
>> +     u8 range;
>> +     u8 odr_bits;
>> +
>> +     /* standby state */
>> +     u8 acc_stby;
>> +     u8 mag_stby;
> Why u8 instead of bool?

Hmm, correct. I was thinking bitwise, but you are right. Will fix.

>> +
>> +     /* power state */
>> +     bool acc_ps;
>> +     bool mag_ps;
>> +};
>> +
>> +enum kmx61_range {
>> +     KMX61_RANGE_2G,
>> +     KMX61_RANGE_4G,
>> +     KMX61_RANGE_8G,
>> +};
>> +
>> +enum kmx61_scan {
>> +     KMX61_SCAN_ACC_X,
>> +     KMX61_SCAN_ACC_Y,
>> +     KMX61_SCAN_ACC_Z,
>> +     KMX61_SCAN_TEMP,
>> +     KMX61_SCAN_MAG_X,
>> +     KMX61_SCAN_MAG_Y,
>> +     KMX61_SCAN_MAG_Z,
>> +};
>> +
>> +static const struct {
>> +     u16 uscale;
>> +     u8 gsel0;
>> +     u8 gsel1;
>> +} kmx61_scale_table[] = {
>> +     {9582, 0, 0},
>> +     {19163, 1, 0},
>> +     {38326, 0, 1},
> Why not consolidate gsel0 and gsel1 to gsel? Then this could become u16 kmx61_scale[] = {9582, 19163, 38326} with gsel as index.

True. Will fix.

>> +};
>> +
>> +/* KMX61 devices */
>> +#define KMX61_ACC    0x01
>> +#define KMX61_MAG    0x02
>> +
>> +static const struct {
>> +     int val;
>> +     int val2;
>> +     u8 odr_bits;
>> +} kmx61_samp_freq_table[] = { {12, 500000, 0x00},
>> +                     {25, 0, 0x01},
>> +                     {50, 0, 0x02},
>> +                     {100, 0, 0x03},
>> +                     {200, 0, 0x04},
>> +                     {400, 0, 0x05},
>> +                     {800, 0, 0x06},
>> +                     {1600, 0, 0x07},
>> +                     {0, 781000, 0x08},
>> +                     {1, 563000, 0x09},
>> +                     {3, 125000, 0x0A},
>> +                     {6, 250000, 0x0B} };
>> +
>> +static IIO_CONST_ATTR(accel_scale_available, "0.009582 0.019163 0.038326");
>> +static IIO_CONST_ATTR(magn_scale_available, "0.001465");
>> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
>> +     "0.781000 1.563000 3.125000 6.250000 12.500000 25 50 100 200 400 800");
>> +
>> +static struct attribute *kmx61_attributes[] = {
>> +     &iio_const_attr_accel_scale_available.dev_attr.attr,
>> +     &iio_const_attr_magn_scale_available.dev_attr.attr,
>> +     &iio_const_attr_sampling_frequency_available.dev_attr.attr,
>> +     NULL,
>> +};
>> +
>> +static const struct attribute_group kmx61_attribute_group = {
>> +     .attrs = kmx61_attributes,
>> +};
>> +
>> +#define KMX61_ACC_CHAN(_axis, _index) { \
>> +     .type = IIO_ACCEL, \
>> +     .modified = 1, \
>> +     .channel2 = IIO_MOD_ ## _axis, \
>> +     .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>> +     .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
>> +                             BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>> +     .address = KMX61_ACC, \
>> +     .scan_index = _index, \
>> +     .scan_type = { \
>> +             .sign = 's', \
>> +             .realbits = 12, \
>> +             .storagebits = 16, \
>> +             .shift = 4, \
>> +             .endianness = IIO_LE, \
>> +     }, \
>> +}
>> +
>> +#define KMX61_MAG_CHAN(_axis, _index) { \
>> +     .type = IIO_MAGN, \
>> +     .modified = 1, \
>> +     .channel2 = IIO_MOD_ ## _axis, \
>> +     .address = KMX61_MAG, \
>> +     .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>> +     .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
>> +                             BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>> +     .scan_index = _index, \
>> +     .scan_type = { \
>> +             .sign = 's', \
>> +             .realbits = 14, \
>> +             .storagebits = 16, \
>> +             .shift = 2, \
>> +             .endianness = IIO_LE, \
>> +     }, \
>> +}
>> +
>> +static const struct iio_chan_spec kmx61_channels[] = {
>> +     KMX61_ACC_CHAN(X, KMX61_SCAN_ACC_X),
>> +     KMX61_ACC_CHAN(Y, KMX61_SCAN_ACC_Y),
>> +     KMX61_ACC_CHAN(Z, KMX61_SCAN_ACC_Z),
>> +     KMX61_MAG_CHAN(X, KMX61_SCAN_MAG_X),
>> +     KMX61_MAG_CHAN(Y, KMX61_SCAN_MAG_Y),
>> +     KMX61_MAG_CHAN(Z, KMX61_SCAN_MAG_Z),
>> +};
>> +
>> +static int kmx61_convert_freq_to_bit(int val, int val2)
>> +{
>> +     int i;
>> +
>> +     for (i = 0; i < ARRAY_SIZE(kmx61_samp_freq_table); i++)
>> +             if (val == kmx61_samp_freq_table[i].val &&
>> +                 val2 == kmx61_samp_freq_table[i].val2)
>> +                     return kmx61_samp_freq_table[i].odr_bits;
>> +     return -EINVAL;
>> +}
>> +/**
> Like here, the first line of the following multiline comments have one asterisk too much.

I think this is the convention, when adding doxygen style documentation:
http://lxr.free-electrons.com/source/fs/eventpoll.c#L382

>> + * kmx61_set_mode() - set KMX61 device operating mode
>> + * @data - kmx61 device private data pointer
>> + * @mode - bitmask, indicating operating mode for @device
>> + * @device - bitmask, indicating device for which @mode needs to be set
>> + * @update - update stby bits stored in device's private  @data
>> + *
>> + * For each sensor (accelerometer/magnetometer) there are two operating modes
>> + * STANDBY and OPERATION. Neither accel nor magn can be disabled independently
>> + * if they are both enabled. Internal sensors state is saved in acc_stby and
>> + * mag_stby members of driver's private @data.
>> + */
>> +static int kmx61_set_mode(struct kmx61_data *data, u8 mode, u8 device,
>> +                       bool update)
>> +{
>> +     int ret;
>> +     int acc_stby = -1, mag_stby = -1;
>> +
>> +     ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_STBY);
>> +     if (ret < 0) {
>> +             dev_err(&data->client->dev, "Error reading reg_stby\n");
>> +             return ret;
>> +     }
>> +     if (device & KMX61_ACC) {
>> +             if (mode & KMX61_ACC_STBY_BIT) {
>> +                     ret |= KMX61_ACC_STBY_BIT;
>> +                     acc_stby = 1;
>> +             } else {
>> +                     ret &= ~KMX61_ACC_STBY_BIT;
>> +                     acc_stby = 0;
>> +             }
>> +     }
>> +
>> +     if (device & KMX61_MAG) {
>> +             if (mode & KMX61_MAG_STBY_BIT) {
>> +                     ret |= KMX61_MAG_STBY_BIT;
>> +                     mag_stby = 1;
>> +             } else {
>> +                     ret &= ~KMX61_MAG_STBY_BIT;
>> +                     mag_stby = 0;
>> +             }
>> +     }
>> +
>> +     ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_STBY, ret);
>> +     if (ret < 0) {
>> +             dev_err(&data->client->dev, "Error writing reg_stby\n");
>> +             return ret;
>> +     }
>> +
>> +     if (acc_stby != -1 && update)
>> +             data->acc_stby = !!acc_stby;
>> +     if (mag_stby != -1 && update)
>> +             data->mag_stby = !!mag_stby;
> Why is there a need for double negation?

Correct. Now it doesn't make sense since data->acc_stby is u8.

But, with your suggestion if I will use bool to represent data->acc_stby
then I think the code should use double negation to make it clear
that data->acc_stby is bool and acc_stby is int.

>> +
>> +     return ret;
> Since no error occured until this point, just return 0.

Ok. Will fix.

>> +}
>> +
>> +static int kmx61_get_mode(struct kmx61_data *data, u8 *mode, u8 device)
>> +{
>> +     int ret;
>> +
>> +     ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_STBY);
>> +     if (ret < 0) {
>> +             dev_err(&data->client->dev, "Error reading reg_stby\n");
>> +             return ret;
>> +     }
>> +     *mode = 0;
>> +
>> +     if (device & KMX61_ACC) {
>> +             if (ret & KMX61_ACC_STBY_BIT)
>> +                     *mode |= KMX61_ACC_STBY_BIT;
>> +             else
>> +                     *mode &= ~KMX61_ACC_STBY_BIT;
> Simply:
>                 *mode |= ret & KMX61_ACC_STBY_BIT;
Ok.

>> +     }
>> +
>> +     if (device & KMX61_MAG) {
>> +             if (ret & KMX61_MAG_STBY_BIT)
>> +                     *mode |= KMX61_MAG_STBY_BIT;
>> +             else
>> +                     *mode &= ~KMX61_MAG_STBY_BIT;
> Same here:
Ok.

>                 *mode |= ret & KMX61_MAG_STBY_BIT;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int kmx61_set_odr(struct kmx61_data *data, int val, int val2, u8 device)
>> +{
>> +     int ret;
>> +     u8 mode;
>> +     int lodr_bits, odr_bits;
>> +
>> +     ret = kmx61_get_mode(data, &mode, KMX61_ACC | KMX61_MAG);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     lodr_bits = kmx61_convert_freq_to_bit(val, val2);
>> +     if (lodr_bits < 0)
>> +             return lodr_bits;
>> +
>> +     /* To change ODR, accel and magn must be in STDBY */
>> +     ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG,
>> +                          true);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     odr_bits = 0;
>> +     if (device & KMX61_ACC)
>> +             odr_bits |= lodr_bits;
>> +     if (device & KMX61_MAG)
>> +             odr_bits |= (lodr_bits << KMX61_MAG_ODR_SHIFT);
> No need for parenthesis.
ok.

>> +
>> +     ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_ODCNTL,
>> +                                     odr_bits);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = kmx61_set_mode(data, mode, KMX61_ACC | KMX61_MAG, true);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     data->odr_bits = lodr_bits;
>> +
>> +     return 0;
>> +}
>> +
>> +static
>> +int kmx61_get_odr(struct kmx61_data *data, int *val, int *val2, u8 device)
> Usually wrap parameters? And don't put code on line with {.

Will fix.

>> +{    int i;
>> +     u8 lodr_bits;
>> +
>> +     if (device & KMX61_ACC)
>> +             lodr_bits = (data->odr_bits >> KMX61_ACC_ODR_SHIFT) &
>> +                          KMX61_ACC_ODR_MASK;
> KMX61_ACC_ODR_SHIFT is just a placebo and not used in kmx61_set_odr. Choose one way and stay consistent.

placebo is good.  will fix kmx61_set_odr :)

>> +     else if (device & KMX61_MAG)
>> +             lodr_bits = (data->odr_bits >> KMX61_MAG_ODR_SHIFT) &
>> +                          KMX61_MAG_ODR_MASK;
>> +     else
>> +             return -EINVAL;
>> +
>> +     for (i = 0; i < ARRAY_SIZE(kmx61_samp_freq_table); i++)
>> +             if (lodr_bits == kmx61_samp_freq_table[i].odr_bits) {
>> +                     *val = kmx61_samp_freq_table[i].val;
>> +                     *val2 = kmx61_samp_freq_table[i].val2;
>> +                     return 0;
> return IIO_VAL_INT_PLUS_MICRO allows you to faciliate _read_raw a bit to just return ret there.

Hmm, If you don't mind I want to keep this as it is, just to
be consistent with the rest of the functions. The convention
used is to return 0 in case of success.

>> +             }
>> +     return -EINVAL;
>> +}
>> +
>> +static int kmx61_set_range(struct kmx61_data *data, int range)
>> +{
>> +     int ret;
>> +
>> +     ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_CTRL1);
>> +     if (ret < 0) {
>> +             dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
>> +             return ret;
>> +     }
>> +
>> +     ret &= ~(KMX61_REG_CTRL1_GSEL0_MASK | KMX61_REG_CTRL1_GSEL1_MASK);
>> +     ret |= kmx61_scale_table[range].gsel0 << KMX61_REG_CTRL1_GSEL0_SHIFT;
>> +     ret |= kmx61_scale_table[range].gsel1 << KMX61_REG_CTRL1_GSEL1_SHIFT;
>> +
>> +     ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_CTRL1, ret);
>> +     if (ret < 0) {
>> +             dev_err(&data->client->dev, "Error writing reg_ctrl1\n");
>> +             return ret;
>> +     }
>> +
>> +     data->range = range;
>> +
>> +     return 0;
>> +}
>> +
>> +static int kmx61_set_scale(struct kmx61_data *data, int uscale)
>> +{
>> +     int ret, i;
>> +     u8  mode;
>> +
>> +     for (i = 0; i < ARRAY_SIZE(kmx61_scale_table); i++) {
>> +             if (kmx61_scale_table[i].uscale == uscale) {
>> +                     ret = kmx61_get_mode(data, &mode,
>> +                                          KMX61_ACC | KMX61_MAG);
>> +                     if (ret < 0)
>> +                             return ret;
>> +
>> +                     ret = kmx61_set_mode(data, KMX61_ALL_STBY,
>> +                                          KMX61_ACC | KMX61_MAG, true);
>> +                     if (ret < 0)
>> +                             return ret;
>> +
>> +                     ret = kmx61_set_range(data, i);
>> +                     if (ret < 0)
>> +                             return ret;
>> +
>> +                     return  kmx61_set_mode(data, mode,
>> +                                            KMX61_ACC | KMX61_MAG, true);
>> +             }
>> +     }
>> +     return -EINVAL;
>> +}
>> +
>> +static int kmx61_chip_init(struct kmx61_data *data)
>> +{
>> +     int ret;
>> +
>> +     ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_WHO_AM_I);
>> +     if (ret < 0) {
>> +             dev_err(&data->client->dev, "Error reading who_am_i\n");
>> +             return ret;
>> +     }
>> +
>> +     if (ret != KMX61_CHIP_ID) {
>> +             dev_err(&data->client->dev,
>> +                     "Wrong chip id, got %x expected %x\n",
>> +                      ret, KMX61_CHIP_ID);
>> +             return -EINVAL;
>> +     }
>> +
>> +     /* set accel 12bit, 4g range */
>> +     ret = kmx61_set_range(data, KMX61_RANGE_4G);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     /* set acc/magn to OPERATION mode */
>> +     ret = kmx61_set_mode(data, 0, KMX61_ACC | KMX61_MAG, true);
> Just return kmx61_set_mode(data, 0, KMX61_ACC | KMX61_MAG, true);

Ok.

>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     return 0;
>> +}
>> +/**
>> + * kmx61_set_power_state() - set power state for kmx61 @device
>> + * @data - kmx61 device private pointer
>> + * @on - power state to be set for @device
>> + * @device - bitmask indicating device for which @on state needs to be set
>> + *
>> + * Notice that when ACC power state needs to be set to ON and MAG is in
>> + * OPERATION then we know that kmx61_runtime_resume was already called
>> + * so we must set ACC OPERATION mode here. The same happens when MAG power
>> + * state needs to be set to ON and ACC is in OPERATION.
>> + */
>> +static int kmx61_set_power_state(struct kmx61_data *data, bool on, u8 device)
>> +{
>> +#ifdef CONFIG_PM_RUNTIME
>> +     int ret;
>> +
>> +     if (device & KMX61_ACC) {
>> +             if (on && !data->acc_ps && !data->mag_stby)
>> +                     kmx61_set_mode(data, 0, KMX61_ACC, true);
>> +             data->acc_ps = on;
>> +     }
>> +     if (device & KMX61_MAG) {
>> +             if (on && !data->mag_ps && !data->acc_stby)
>> +                     kmx61_set_mode(data, 0, KMX61_MAG, true);
>> +             data->mag_ps = on;
>> +     }
>> +
>> +     if (on) {
>> +             ret = pm_runtime_get_sync(&data->client->dev);
>> +     } else {
>> +             pm_runtime_mark_last_busy(&data->client->dev);
>> +             ret = pm_runtime_put_autosuspend(&data->client->dev);
>> +     }
>> +     if (ret < 0) {
>> +             dev_err(&data->client->dev,
>> +                     "Failed: kmx61_set_power_state for %d, ret %d\n",
>> +                     on, ret);
>> +             return ret;
>> +     }
>> +#endif
>> +     return 0;
>> +}
>> +
>> +static int kmx61_read_measurement(struct kmx61_data *data, int base, int offset)
> u8 base, u8 offset?

Correct.

>> +{
>> +     int ret;
>> +     u8 reg = base + offset * 2;
>> +
>> +     ret = i2c_smbus_read_word_data(data->client, reg);
>> +     if (ret < 0) {
>> +             dev_err(&data->client->dev, "failed to read reg at %x\n", reg);
>> +             return ret;
> This return is unnecessary.

Ok.

>> +     }
>> +
>> +     return ret;
>> +}
>> +
>> +static int kmx61_read_raw(struct iio_dev *indio_dev,
>> +                           struct iio_chan_spec const *chan, int *val,
>> +                           int *val2, long mask)
> Indentation of the parameters is a bit too high.

Ok, will fix.

>> +{
>> +     struct kmx61_data *data = iio_priv(indio_dev);
>> +     int ret;
>> +     u8 base_reg;
>> +
>> +     switch (mask) {
>> +     case IIO_CHAN_INFO_RAW:
>> +             switch (chan->type) {
>> +             case IIO_ACCEL:
>> +             case IIO_MAGN:
>> +                     base_reg = KMX61_ACC_XOUT_L;
>> +                     break;
>> +             default:
>> +                     return -EINVAL;
>> +             }
>> +             mutex_lock(&data->lock);
>> +
>> +             kmx61_set_power_state(data, true, chan->address);
>> +             ret = kmx61_read_measurement(data, base_reg, chan->scan_index);
>> +             if (ret < 0) {
>> +                     kmx61_set_power_state(data, false, chan->address);
>> +                     mutex_unlock(&data->lock);
>> +                     return ret;
>> +             }
>> +             *val = sign_extend32(ret >> chan->scan_type.shift,
>> +                                  chan->scan_type.realbits - 1);
>> +             kmx61_set_power_state(data, false, chan->address);
>> +
>> +             mutex_unlock(&data->lock);
>> +             return IIO_VAL_INT;
>> +     case IIO_CHAN_INFO_SCALE:
>> +             switch (chan->type) {
>> +             case IIO_ACCEL:
>> +                     *val = 0;
>> +                     *val2 = kmx61_scale_table[data->range].uscale;
>> +                     return IIO_VAL_INT_PLUS_MICRO;
>> +             case IIO_MAGN:
>> +                     /* 14 bits res, 1465 microGauss per magn count */
>> +                     *val = 0;
>> +                     *val2 = 1465;
>> +                     return IIO_VAL_INT_PLUS_MICRO;
>> +             default:
>> +                     return -EINVAL;
>> +             }
>> +     case IIO_CHAN_INFO_SAMP_FREQ:
>> +             if (chan->type != IIO_ACCEL && chan->type != IIO_MAGN)
>> +                     return -EINVAL;
>> +
>> +             mutex_lock(&data->lock);
>> +             ret = kmx61_get_odr(data, val, val2, chan->address);
>> +             mutex_unlock(&data->lock);
> Could be just return ret here, if kmx61_get_odr() returns IIO_VAL_INT_PLUS_MICRO on success and -EINVAL on error.

As explained above, I prefer not to do this. More than that, I can't do ret here
because the data->lock is held.

>> +             if (ret)
>> +                     return -EINVAL;
>> +             return IIO_VAL_INT_PLUS_MICRO;
>> +     }
>> +     return -EINVAL;
>> +}
>> +
>> +static int kmx61_write_raw(struct iio_dev *indio_dev,
>> +                            struct iio_chan_spec const *chan, int val,
>> +                            int val2, long mask)
> Indentation of parameters is too high.
Ok.
>> +{
>> +     struct kmx61_data *data = iio_priv(indio_dev);
>> +     int ret;
>> +
>> +     switch (mask) {
>> +     case IIO_CHAN_INFO_SAMP_FREQ:
>> +             if (chan->type != IIO_ACCEL && chan->type != IIO_MAGN)
>> +                     return -EINVAL;
>> +
>> +             mutex_lock(&data->lock);
>> +             ret = kmx61_set_odr(data, val, val2, chan->address);
>> +             mutex_unlock(&data->lock);
>> +             return ret;
>> +     case IIO_CHAN_INFO_SCALE:
>> +             switch (chan->type) {
>> +             case IIO_ACCEL:
>> +                     if (val != 0)
>> +                             return -EINVAL;
>> +                     mutex_lock(&data->lock);
>> +                     ret = kmx61_set_scale(data, val2);
>> +                     mutex_unlock(&data->lock);
>> +                     return ret;
>> +             default:
>> +                     return -EINVAL;
>> +             }
>> +             return ret;
> This won't be reached (if so, ret would not be initialized).
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +     return ret;
> Same here.

I know. Would initializing ret with 0 will be enough?

>> +}
>> +
>> +static const struct iio_info kmx61_info = {
>> +     .driver_module          = THIS_MODULE,
>> +     .read_raw               = kmx61_read_raw,
>> +     .write_raw              = kmx61_write_raw,
>> +     .attrs                  = &kmx61_attribute_group,
>> +};
>> +
>> +static int kmx61_probe(struct i2c_client *client,
>> +                    const struct i2c_device_id *id)
>> +{
>> +     struct kmx61_data *data;
>> +     struct iio_dev *indio_dev;
>> +     int ret;
>> +     const char *name = NULL;
>> +
>> +     indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>> +     if (!indio_dev)
>> +             return -ENOMEM;
>> +
>> +     data = iio_priv(indio_dev);
>> +     i2c_set_clientdata(client, indio_dev);
>> +     data->client = client;
>> +
>> +     if (id)
>> +             name = id->name;
>> +
>> +     indio_dev->dev.parent = &client->dev;
>> +     indio_dev->channels = kmx61_channels;
>> +     indio_dev->num_channels = ARRAY_SIZE(kmx61_channels);
>> +     indio_dev->name = name;
>> +     indio_dev->modes = INDIO_DIRECT_MODE;
>> +     indio_dev->info = &kmx61_info;
>> +
>> +     mutex_init(&data->lock);
>> +
>> +     ret = kmx61_chip_init(data);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = iio_device_register(indio_dev);
>> +     if (ret < 0) {
>> +             dev_err(&client->dev, "Failed to register iio device\n");
>> +             goto err_iio_device_register;
>> +     }
>> +
>> +     ret = pm_runtime_set_active(&client->dev);
>> +     if (ret < 0)
>> +             goto err_pm_runtime_set_active;
>> +
>> +     pm_runtime_enable(&client->dev);
>> +     pm_runtime_set_autosuspend_delay(&client->dev, KMX61_SLEEP_DELAY_MS);
>> +     pm_runtime_use_autosuspend(&client->dev);
>> +
>> +     return 0;
>> +
>> +err_pm_runtime_set_active:
>> +     iio_device_unregister(indio_dev);
>> +err_iio_device_register:
>> +     kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
>> +     return ret;
>> +}
>> +
>> +static int kmx61_remove(struct i2c_client *client)
>> +{
>> +     struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> +     struct kmx61_data *data = iio_priv(indio_dev);
>> +     int ret;
>> +
>> +     pm_runtime_disable(&client->dev);
>> +     pm_runtime_set_suspended(&client->dev);
>> +     pm_runtime_put_noidle(&client->dev);
>> +
>> +     iio_device_unregister(indio_dev);
>> +
>> +     mutex_lock(&data->lock);
>> +     ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
>> +     mutex_unlock(&data->lock);
>> +
>> +     return ret;
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int kmx61_suspend(struct device *dev)
>> +{
>> +     struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>> +     struct kmx61_data *data = iio_priv(indio_dev);
>> +     int ret;
>> +
>> +     mutex_lock(&data->lock);
>> +     ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG,
>> +                          false);
>> +     mutex_unlock(&data->lock);
>> +
>> +     return ret;
>> +}
>> +
>> +static int kmx61_resume(struct device *dev)
>> +{
>> +     struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>> +     struct kmx61_data *data = iio_priv(indio_dev);
>> +     u8 stby = 0;
>> +
>> +     if (data->acc_stby)
>> +             stby |= KMX61_ACC_STBY_BIT;
>> +     if (data->mag_stby)
>> +             stby |= KMX61_MAG_STBY_BIT;
>> +
>> +     return kmx61_set_mode(data, stby, KMX61_ACC | KMX61_MAG, true);
>> +}
>> +#endif
>> +
>> +#ifdef CONFIG_PM_RUNTIME
>> +static int kmx61_runtime_suspend(struct device *dev)
>> +{
>> +     struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>> +     struct kmx61_data *data = iio_priv(indio_dev);
>> +     int ret;
>> +
>> +     mutex_lock(&data->lock);
>> +     ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
>> +     mutex_unlock(&data->lock);
>> +
>> +     return ret;
>> +}
>> +
>> +static int kmx61_runtime_resume(struct device *dev)
>> +{
>> +     struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>> +     struct kmx61_data *data = iio_priv(indio_dev);
>> +     u8 stby = 0;
>> +
>> +     if (!data->acc_ps)
>> +             stby |= KMX61_ACC_STBY_BIT;
>> +     if (!data->mag_ps)
>> +             stby |= KMX61_MAG_STBY_BIT;
>> +
>> +     return kmx61_set_mode(data, stby, KMX61_ACC | KMX61_MAG, true);
>> +}
>> +#endif
>> +
>> +static const struct dev_pm_ops kmx61_pm_ops = {
>> +     SET_SYSTEM_SLEEP_PM_OPS(kmx61_suspend, kmx61_resume)
>> +     SET_RUNTIME_PM_OPS(kmx61_runtime_suspend, kmx61_runtime_resume, NULL)
>> +};
>> +
>> +static const struct i2c_device_id kmx61_id[] = {
>> +     {"kmx611021", 0},
>> +     {}
>> +};
>> +
>> +MODULE_DEVICE_TABLE(i2c, kmx61_id);
>> +
>> +static struct i2c_driver kmx61_driver = {
>> +     .driver = {
>> +             .name = KMX61_DRV_NAME,
>> +             .pm = &kmx61_pm_ops,
>> +     },
>> +     .probe          = kmx61_probe,
>> +     .remove         = kmx61_remove,
>> +     .id_table       = kmx61_id,
>> +};
>> +
>> +module_i2c_driver(kmx61_driver);
>> +
>> +MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com>");
>> +MODULE_DESCRIPTION("KMX61 accelerometer/magnetometer driver");
>> +MODULE_LICENSE("GPL v2");

Hartmut, thanks a lot for your reviews. I will send a patch series
with these cleanups together with changes for ACPI, buffer and trigger
support.

Daniel.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] iio: imu: Add support for Kionix KMX61 sensor
  2014-11-25  9:54   ` Daniel Baluta
@ 2014-11-25 22:31     ` Hartmut Knaack
  2014-11-25 22:44       ` Daniel Baluta
  0 siblings, 1 reply; 7+ messages in thread
From: Hartmut Knaack @ 2014-11-25 22:31 UTC (permalink / raw)
  To: Daniel Baluta
  Cc: Jonathan Cameron, Peter Meerwald, Srinivas Pandruvada, linux-iio,
	Linux Kernel Mailing List

Daniel Baluta schrieb am 25.11.2014 10:54:
> On Tue, Nov 25, 2014 at 2:45 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
>> Daniel Baluta schrieb am 18.11.2014 17:47:
>>> Minimal implementation for KMX61 6-axis accelerometer/magnetometer. It exports
>>> raw accel/magn readings together with scale and sampling frequency.
>>>
>>> Datasheet will be available at:
>>> http://www.kionix.com/6-axis-accelerometer-magnetometer/kmx61
>>>
>> Finally managed to have a close look, and found a few issues.
>>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>>> ---
>>> Changes since v2:
>>>       * mostly style fixes after comments received from Jonathan and Peter
>>>               * http://marc.info/?l=linux-iio&m=141564019602400&w=2
>>>       * implemented PM runtime and PM sleep resume/suspend hooks
>>>       * introduced new parameter for kmx61_set_mode to help updating
>>>       stby bits in kmx61_data.
>>>       * use kmx6111021 instead of kmx61 for i2c_device_id.
>>>
>>>  drivers/iio/imu/Kconfig  |   9 +
>>>  drivers/iio/imu/Makefile |   2 +
>>>  drivers/iio/imu/kmx61.c  | 766 +++++++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 777 insertions(+)
>>>  create mode 100644 drivers/iio/imu/kmx61.c
>>>
>>> diff --git a/drivers/iio/imu/Kconfig b/drivers/iio/imu/Kconfig
>>> index 2b0e451..d675f43 100644
>>> --- a/drivers/iio/imu/Kconfig
>>> +++ b/drivers/iio/imu/Kconfig
>>> @@ -25,6 +25,15 @@ config ADIS16480
>>>         Say yes here to build support for Analog Devices ADIS16375, ADIS16480,
>>>         ADIS16485, ADIS16488 inertial sensors.
>>>
>>> +config KMX61
>>> +     tristate "Kionix KMX61 6-axis accelerometer and magnetometer"
>>> +     depends on I2C
>>> +     help
>>> +       Say Y here if you want to build a driver for Kionix KMX61 6-axis accelerometer
>>> +       and magnetometer.
>>> +       To compile this driver as module, choose M here: the module will be called
>>> +       kmx61.
>>> +
>> This help text is more than 80 characters wide and may better be wrapped a bit differently.
> 
> Good catch. checkpatch.pl didn't complain.
> 
>>>  source "drivers/iio/imu/inv_mpu6050/Kconfig"
>>>
>>>  endmenu
>>> diff --git a/drivers/iio/imu/Makefile b/drivers/iio/imu/Makefile
>>> index 114d2c1..e1e6e3d 100644
>>> --- a/drivers/iio/imu/Makefile
>>> +++ b/drivers/iio/imu/Makefile
>>> @@ -14,3 +14,5 @@ adis_lib-$(CONFIG_IIO_ADIS_LIB_BUFFER) += adis_buffer.o
>>>  obj-$(CONFIG_IIO_ADIS_LIB) += adis_lib.o
>>>
>>>  obj-y += inv_mpu6050/
>>> +
>>> +obj-$(CONFIG_KMX61) += kmx61.o
>>> diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c
>>> new file mode 100644
>>> index 0000000..f68b3ef
>>> --- /dev/null
>>> +++ b/drivers/iio/imu/kmx61.c
>>> @@ -0,0 +1,766 @@
>>> +/*
>>> + * KMX61 - Kionix 6-axis Accelerometer/Magnetometer
>>> + *
>>> + * Copyright (c) 2014, 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.
>>> + *
>>> + * IIO driver for KMX61 (7-bit I2C slave address 0x0E or 0x0F).
>>> + *
>>> + * TODO: buffer, interrupt, thresholds, acpi, temperature sensor
>>> + *
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/pm.h>
>>> +#include <linux/pm_runtime.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/sysfs.h>
>>> +
>>> +#define KMX61_DRV_NAME "kmx61"
>>> +
>>> +#define KMX61_REG_WHO_AM_I   0x00
>>> +
>>> +/*
>>> + * three 16-bit accelerometer output registers for X/Y/Z axis
>>> + * we use only XOUT_L as a base register, all other addresses
>>> + * can be obtained by applying an offset and are provided here
>>> + * only for clarity.
>>> + */
>>> +#define KMX61_ACC_XOUT_L     0x0A
>>> +#define KMX61_ACC_XOUT_H     0x0B
>>> +#define KMX61_ACC_YOUT_L     0x0C
>>> +#define KMX61_ACC_YOUT_H     0x0D
>>> +#define KMX61_ACC_ZOUT_L     0x0E
>>> +#define KMX61_ACC_ZOUT_H     0x0F
>>> +
>>> +/*
>>> + * one 16-bit temperature output register
>>> + */
>>> +#define KMX61_TEMP_L         0x10
>>> +#define KMX61_TEMP_H         0x11
>>> +
>>> +/*
>>> + * three 16-bit magnetometer output registers for X/Y/Z axis
>>> + */
>>> +#define KMX61_MAG_XOUT_L     0x12
>>> +#define KMX61_MAG_XOUT_H     0x13
>>> +#define KMX61_MAG_YOUT_L     0x14
>>> +#define KMX61_MAG_YOUT_H     0x15
>>> +#define KMX61_MAG_ZOUT_L     0x16
>>> +#define KMX61_MAG_ZOUT_H     0x17
>>> +
>>> +#define KMX61_REG_ODCNTL     0x2C
>>> +#define KMX61_REG_STBY               0x29
>>> +#define KMX61_REG_CTRL1              0x2A
>>> +
>>> +#define KMX61_ACC_STBY_BIT   BIT(0)
>>> +#define KMX61_MAG_STBY_BIT   BIT(1)
>>> +#define KMX61_ACT_STBY_BIT   BIT(7)
>>> +
>>> +#define KMX61_ALL_STBY               (KMX61_ACC_STBY_BIT | KMX61_MAG_STBY_BIT)
>>> +
>>> +#define KMX61_REG_CTRL1_GSEL0_SHIFT  0
>>> +#define KMX61_REG_CTRL1_GSEL1_SHIFT  1
>>> +#define KMX61_REG_CTRL1_GSEL0_MASK   0x01
>>> +#define KMX61_REG_CTRL1_GSEL1_MASK   0x02
>> I don't see the benefit of separating GSEL0 and GSEL1.
>>> +
>>> +#define KMX61_REG_CTRL1_BIT_RES              BIT(4)
>>> +
>>> +#define KMX61_ACC_ODR_SHIFT  0
>>> +#define KMX61_MAG_ODR_SHIFT  4
>>> +#define KMX61_ACC_ODR_MASK   0x0F
>>> +#define KMX61_MAG_ODR_MASK   0xF0
>>> +
>>> +#define KMX61_SLEEP_DELAY_MS 2000
>>> +
>>> +#define KMX61_CHIP_ID                0x12
>>> +
>>> +struct kmx61_data {
>>> +     struct i2c_client *client;
>>> +
>>> +     /* serialize access to non-atomic ops, e.g set_mode */
>>> +     struct mutex lock;
>>> +     u8 range;
>>> +     u8 odr_bits;
>>> +
>>> +     /* standby state */
>>> +     u8 acc_stby;
>>> +     u8 mag_stby;
>> Why u8 instead of bool?
> 
> Hmm, correct. I was thinking bitwise, but you are right. Will fix.
> 
>>> +
>>> +     /* power state */
>>> +     bool acc_ps;
>>> +     bool mag_ps;
>>> +};
>>> +
>>> +enum kmx61_range {
>>> +     KMX61_RANGE_2G,
>>> +     KMX61_RANGE_4G,
>>> +     KMX61_RANGE_8G,
>>> +};
>>> +
>>> +enum kmx61_scan {
>>> +     KMX61_SCAN_ACC_X,
>>> +     KMX61_SCAN_ACC_Y,
>>> +     KMX61_SCAN_ACC_Z,
>>> +     KMX61_SCAN_TEMP,
>>> +     KMX61_SCAN_MAG_X,
>>> +     KMX61_SCAN_MAG_Y,
>>> +     KMX61_SCAN_MAG_Z,
>>> +};
>>> +
>>> +static const struct {
>>> +     u16 uscale;
>>> +     u8 gsel0;
>>> +     u8 gsel1;
>>> +} kmx61_scale_table[] = {
>>> +     {9582, 0, 0},
>>> +     {19163, 1, 0},
>>> +     {38326, 0, 1},
>> Why not consolidate gsel0 and gsel1 to gsel? Then this could become u16 kmx61_scale[] = {9582, 19163, 38326} with gsel as index.
> 
> True. Will fix.
> 
>>> +};
>>> +
>>> +/* KMX61 devices */
>>> +#define KMX61_ACC    0x01
>>> +#define KMX61_MAG    0x02
>>> +
>>> +static const struct {
>>> +     int val;
>>> +     int val2;
>>> +     u8 odr_bits;
>>> +} kmx61_samp_freq_table[] = { {12, 500000, 0x00},
>>> +                     {25, 0, 0x01},
>>> +                     {50, 0, 0x02},
>>> +                     {100, 0, 0x03},
>>> +                     {200, 0, 0x04},
>>> +                     {400, 0, 0x05},
>>> +                     {800, 0, 0x06},
>>> +                     {1600, 0, 0x07},
>>> +                     {0, 781000, 0x08},
>>> +                     {1, 563000, 0x09},
>>> +                     {3, 125000, 0x0A},
>>> +                     {6, 250000, 0x0B} };
>>> +
>>> +static IIO_CONST_ATTR(accel_scale_available, "0.009582 0.019163 0.038326");
>>> +static IIO_CONST_ATTR(magn_scale_available, "0.001465");
>>> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
>>> +     "0.781000 1.563000 3.125000 6.250000 12.500000 25 50 100 200 400 800");
>>> +
>>> +static struct attribute *kmx61_attributes[] = {
>>> +     &iio_const_attr_accel_scale_available.dev_attr.attr,
>>> +     &iio_const_attr_magn_scale_available.dev_attr.attr,
>>> +     &iio_const_attr_sampling_frequency_available.dev_attr.attr,
>>> +     NULL,
>>> +};
>>> +
>>> +static const struct attribute_group kmx61_attribute_group = {
>>> +     .attrs = kmx61_attributes,
>>> +};
>>> +
>>> +#define KMX61_ACC_CHAN(_axis, _index) { \
>>> +     .type = IIO_ACCEL, \
>>> +     .modified = 1, \
>>> +     .channel2 = IIO_MOD_ ## _axis, \
>>> +     .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>>> +     .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
>>> +                             BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>>> +     .address = KMX61_ACC, \
>>> +     .scan_index = _index, \
>>> +     .scan_type = { \
>>> +             .sign = 's', \
>>> +             .realbits = 12, \
>>> +             .storagebits = 16, \
>>> +             .shift = 4, \
>>> +             .endianness = IIO_LE, \
>>> +     }, \
>>> +}
>>> +
>>> +#define KMX61_MAG_CHAN(_axis, _index) { \
>>> +     .type = IIO_MAGN, \
>>> +     .modified = 1, \
>>> +     .channel2 = IIO_MOD_ ## _axis, \
>>> +     .address = KMX61_MAG, \
>>> +     .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>>> +     .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
>>> +                             BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>>> +     .scan_index = _index, \
>>> +     .scan_type = { \
>>> +             .sign = 's', \
>>> +             .realbits = 14, \
>>> +             .storagebits = 16, \
>>> +             .shift = 2, \
>>> +             .endianness = IIO_LE, \
>>> +     }, \
>>> +}
>>> +
>>> +static const struct iio_chan_spec kmx61_channels[] = {
>>> +     KMX61_ACC_CHAN(X, KMX61_SCAN_ACC_X),
>>> +     KMX61_ACC_CHAN(Y, KMX61_SCAN_ACC_Y),
>>> +     KMX61_ACC_CHAN(Z, KMX61_SCAN_ACC_Z),
>>> +     KMX61_MAG_CHAN(X, KMX61_SCAN_MAG_X),
>>> +     KMX61_MAG_CHAN(Y, KMX61_SCAN_MAG_Y),
>>> +     KMX61_MAG_CHAN(Z, KMX61_SCAN_MAG_Z),
>>> +};
>>> +
>>> +static int kmx61_convert_freq_to_bit(int val, int val2)
>>> +{
>>> +     int i;
>>> +
>>> +     for (i = 0; i < ARRAY_SIZE(kmx61_samp_freq_table); i++)
>>> +             if (val == kmx61_samp_freq_table[i].val &&
>>> +                 val2 == kmx61_samp_freq_table[i].val2)
>>> +                     return kmx61_samp_freq_table[i].odr_bits;
>>> +     return -EINVAL;
>>> +}
>>> +/**
>> Like here, the first line of the following multiline comments have one asterisk too much.
> 
> I think this is the convention, when adding doxygen style documentation:
> http://lxr.free-electrons.com/source/fs/eventpoll.c#L382
> 
>>> + * kmx61_set_mode() - set KMX61 device operating mode
>>> + * @data - kmx61 device private data pointer
>>> + * @mode - bitmask, indicating operating mode for @device
>>> + * @device - bitmask, indicating device for which @mode needs to be set
>>> + * @update - update stby bits stored in device's private  @data
>>> + *
>>> + * For each sensor (accelerometer/magnetometer) there are two operating modes
>>> + * STANDBY and OPERATION. Neither accel nor magn can be disabled independently
>>> + * if they are both enabled. Internal sensors state is saved in acc_stby and
>>> + * mag_stby members of driver's private @data.
>>> + */
>>> +static int kmx61_set_mode(struct kmx61_data *data, u8 mode, u8 device,
>>> +                       bool update)
>>> +{
>>> +     int ret;
>>> +     int acc_stby = -1, mag_stby = -1;
>>> +
>>> +     ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_STBY);
>>> +     if (ret < 0) {
>>> +             dev_err(&data->client->dev, "Error reading reg_stby\n");
>>> +             return ret;
>>> +     }
>>> +     if (device & KMX61_ACC) {
>>> +             if (mode & KMX61_ACC_STBY_BIT) {
>>> +                     ret |= KMX61_ACC_STBY_BIT;
>>> +                     acc_stby = 1;
>>> +             } else {
>>> +                     ret &= ~KMX61_ACC_STBY_BIT;
>>> +                     acc_stby = 0;
>>> +             }
>>> +     }
>>> +
>>> +     if (device & KMX61_MAG) {
>>> +             if (mode & KMX61_MAG_STBY_BIT) {
>>> +                     ret |= KMX61_MAG_STBY_BIT;
>>> +                     mag_stby = 1;
>>> +             } else {
>>> +                     ret &= ~KMX61_MAG_STBY_BIT;
>>> +                     mag_stby = 0;
>>> +             }
>>> +     }
>>> +
>>> +     ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_STBY, ret);
>>> +     if (ret < 0) {
>>> +             dev_err(&data->client->dev, "Error writing reg_stby\n");
>>> +             return ret;
>>> +     }
>>> +
>>> +     if (acc_stby != -1 && update)
>>> +             data->acc_stby = !!acc_stby;
>>> +     if (mag_stby != -1 && update)
>>> +             data->mag_stby = !!mag_stby;
>> Why is there a need for double negation?
> 
> Correct. Now it doesn't make sense since data->acc_stby is u8.
> 
> But, with your suggestion if I will use bool to represent data->acc_stby
> then I think the code should use double negation to make it clear
> that data->acc_stby is bool and acc_stby is int.
> 
Well, if it makes you happy, then go ahead. My interpretation of a bool is, that an assignment of 0 becomes 0/false, and an assignment of anything else becomes 1/true. So, I wouldn't see a need here (especially since acc_stby/mag_stby were distinctly assigned either 0 or 1).
>>> +
>>> +     return ret;
>> Since no error occured until this point, just return 0.
> 
> Ok. Will fix.
> 
>>> +}
>>> +
>>> +static int kmx61_get_mode(struct kmx61_data *data, u8 *mode, u8 device)
>>> +{
>>> +     int ret;
>>> +
>>> +     ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_STBY);
>>> +     if (ret < 0) {
>>> +             dev_err(&data->client->dev, "Error reading reg_stby\n");
>>> +             return ret;
>>> +     }
>>> +     *mode = 0;
>>> +
>>> +     if (device & KMX61_ACC) {
>>> +             if (ret & KMX61_ACC_STBY_BIT)
>>> +                     *mode |= KMX61_ACC_STBY_BIT;
>>> +             else
>>> +                     *mode &= ~KMX61_ACC_STBY_BIT;
>> Simply:
>>                 *mode |= ret & KMX61_ACC_STBY_BIT;
> Ok.
> 
>>> +     }
>>> +
>>> +     if (device & KMX61_MAG) {
>>> +             if (ret & KMX61_MAG_STBY_BIT)
>>> +                     *mode |= KMX61_MAG_STBY_BIT;
>>> +             else
>>> +                     *mode &= ~KMX61_MAG_STBY_BIT;
>> Same here:
> Ok.
> 
>>                 *mode |= ret & KMX61_MAG_STBY_BIT;
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int kmx61_set_odr(struct kmx61_data *data, int val, int val2, u8 device)
>>> +{
>>> +     int ret;
>>> +     u8 mode;
>>> +     int lodr_bits, odr_bits;
>>> +
>>> +     ret = kmx61_get_mode(data, &mode, KMX61_ACC | KMX61_MAG);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     lodr_bits = kmx61_convert_freq_to_bit(val, val2);
>>> +     if (lodr_bits < 0)
>>> +             return lodr_bits;
>>> +
>>> +     /* To change ODR, accel and magn must be in STDBY */
>>> +     ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG,
>>> +                          true);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     odr_bits = 0;
>>> +     if (device & KMX61_ACC)
>>> +             odr_bits |= lodr_bits;
>>> +     if (device & KMX61_MAG)
>>> +             odr_bits |= (lodr_bits << KMX61_MAG_ODR_SHIFT);
>> No need for parenthesis.
> ok.
> 
>>> +
>>> +     ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_ODCNTL,
>>> +                                     odr_bits);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     ret = kmx61_set_mode(data, mode, KMX61_ACC | KMX61_MAG, true);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     data->odr_bits = lodr_bits;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static
>>> +int kmx61_get_odr(struct kmx61_data *data, int *val, int *val2, u8 device)
>> Usually wrap parameters? And don't put code on line with {.
> 
> Will fix.
> 
>>> +{    int i;
>>> +     u8 lodr_bits;
>>> +
>>> +     if (device & KMX61_ACC)
>>> +             lodr_bits = (data->odr_bits >> KMX61_ACC_ODR_SHIFT) &
>>> +                          KMX61_ACC_ODR_MASK;
>> KMX61_ACC_ODR_SHIFT is just a placebo and not used in kmx61_set_odr. Choose one way and stay consistent.
> 
> placebo is good.  will fix kmx61_set_odr :)
> 
>>> +     else if (device & KMX61_MAG)
>>> +             lodr_bits = (data->odr_bits >> KMX61_MAG_ODR_SHIFT) &
>>> +                          KMX61_MAG_ODR_MASK;
>>> +     else
>>> +             return -EINVAL;
>>> +
>>> +     for (i = 0; i < ARRAY_SIZE(kmx61_samp_freq_table); i++)
>>> +             if (lodr_bits == kmx61_samp_freq_table[i].odr_bits) {
>>> +                     *val = kmx61_samp_freq_table[i].val;
>>> +                     *val2 = kmx61_samp_freq_table[i].val2;
>>> +                     return 0;
>> return IIO_VAL_INT_PLUS_MICRO allows you to faciliate _read_raw a bit to just return ret there.
> 
> Hmm, If you don't mind I want to keep this as it is, just to
> be consistent with the rest of the functions. The convention
> used is to return 0 in case of success.
> 
Up to you. Though kmx61_read_raw() would be an example of a function that doesn't follow this convention ;-) And there is no real benefit of checking an error code there, which is passed up anyway.
>>> +             }
>>> +     return -EINVAL;
>>> +}
>>> +
>>> +static int kmx61_set_range(struct kmx61_data *data, int range)
>>> +{
>>> +     int ret;
>>> +
>>> +     ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_CTRL1);
>>> +     if (ret < 0) {
>>> +             dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
>>> +             return ret;
>>> +     }
>>> +
>>> +     ret &= ~(KMX61_REG_CTRL1_GSEL0_MASK | KMX61_REG_CTRL1_GSEL1_MASK);
>>> +     ret |= kmx61_scale_table[range].gsel0 << KMX61_REG_CTRL1_GSEL0_SHIFT;
>>> +     ret |= kmx61_scale_table[range].gsel1 << KMX61_REG_CTRL1_GSEL1_SHIFT;
>>> +
>>> +     ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_CTRL1, ret);
>>> +     if (ret < 0) {
>>> +             dev_err(&data->client->dev, "Error writing reg_ctrl1\n");
>>> +             return ret;
>>> +     }
>>> +
>>> +     data->range = range;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int kmx61_set_scale(struct kmx61_data *data, int uscale)
>>> +{
>>> +     int ret, i;
>>> +     u8  mode;
>>> +
>>> +     for (i = 0; i < ARRAY_SIZE(kmx61_scale_table); i++) {
>>> +             if (kmx61_scale_table[i].uscale == uscale) {
>>> +                     ret = kmx61_get_mode(data, &mode,
>>> +                                          KMX61_ACC | KMX61_MAG);
>>> +                     if (ret < 0)
>>> +                             return ret;
>>> +
>>> +                     ret = kmx61_set_mode(data, KMX61_ALL_STBY,
>>> +                                          KMX61_ACC | KMX61_MAG, true);
>>> +                     if (ret < 0)
>>> +                             return ret;
>>> +
>>> +                     ret = kmx61_set_range(data, i);
>>> +                     if (ret < 0)
>>> +                             return ret;
>>> +
>>> +                     return  kmx61_set_mode(data, mode,
>>> +                                            KMX61_ACC | KMX61_MAG, true);
>>> +             }
>>> +     }
>>> +     return -EINVAL;
>>> +}
>>> +
>>> +static int kmx61_chip_init(struct kmx61_data *data)
>>> +{
>>> +     int ret;
>>> +
>>> +     ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_WHO_AM_I);
>>> +     if (ret < 0) {
>>> +             dev_err(&data->client->dev, "Error reading who_am_i\n");
>>> +             return ret;
>>> +     }
>>> +
>>> +     if (ret != KMX61_CHIP_ID) {
>>> +             dev_err(&data->client->dev,
>>> +                     "Wrong chip id, got %x expected %x\n",
>>> +                      ret, KMX61_CHIP_ID);
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     /* set accel 12bit, 4g range */
>>> +     ret = kmx61_set_range(data, KMX61_RANGE_4G);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     /* set acc/magn to OPERATION mode */
>>> +     ret = kmx61_set_mode(data, 0, KMX61_ACC | KMX61_MAG, true);
>> Just return kmx61_set_mode(data, 0, KMX61_ACC | KMX61_MAG, true);
> 
> Ok.
> 
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     return 0;
>>> +}
>>> +/**
>>> + * kmx61_set_power_state() - set power state for kmx61 @device
>>> + * @data - kmx61 device private pointer
>>> + * @on - power state to be set for @device
>>> + * @device - bitmask indicating device for which @on state needs to be set
>>> + *
>>> + * Notice that when ACC power state needs to be set to ON and MAG is in
>>> + * OPERATION then we know that kmx61_runtime_resume was already called
>>> + * so we must set ACC OPERATION mode here. The same happens when MAG power
>>> + * state needs to be set to ON and ACC is in OPERATION.
>>> + */
>>> +static int kmx61_set_power_state(struct kmx61_data *data, bool on, u8 device)
>>> +{
>>> +#ifdef CONFIG_PM_RUNTIME
>>> +     int ret;
>>> +
>>> +     if (device & KMX61_ACC) {
>>> +             if (on && !data->acc_ps && !data->mag_stby)
>>> +                     kmx61_set_mode(data, 0, KMX61_ACC, true);
>>> +             data->acc_ps = on;
>>> +     }
>>> +     if (device & KMX61_MAG) {
>>> +             if (on && !data->mag_ps && !data->acc_stby)
>>> +                     kmx61_set_mode(data, 0, KMX61_MAG, true);
>>> +             data->mag_ps = on;
>>> +     }
>>> +
>>> +     if (on) {
>>> +             ret = pm_runtime_get_sync(&data->client->dev);
>>> +     } else {
>>> +             pm_runtime_mark_last_busy(&data->client->dev);
>>> +             ret = pm_runtime_put_autosuspend(&data->client->dev);
>>> +     }
>>> +     if (ret < 0) {
>>> +             dev_err(&data->client->dev,
>>> +                     "Failed: kmx61_set_power_state for %d, ret %d\n",
>>> +                     on, ret);
>>> +             return ret;
>>> +     }
>>> +#endif
>>> +     return 0;
>>> +}
>>> +
>>> +static int kmx61_read_measurement(struct kmx61_data *data, int base, int offset)
>> u8 base, u8 offset?
> 
> Correct.
> 
>>> +{
>>> +     int ret;
>>> +     u8 reg = base + offset * 2;
>>> +
>>> +     ret = i2c_smbus_read_word_data(data->client, reg);
>>> +     if (ret < 0) {
>>> +             dev_err(&data->client->dev, "failed to read reg at %x\n", reg);
>>> +             return ret;
>> This return is unnecessary.
> 
> Ok.
> 
>>> +     }
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +static int kmx61_read_raw(struct iio_dev *indio_dev,
>>> +                           struct iio_chan_spec const *chan, int *val,
>>> +                           int *val2, long mask)
>> Indentation of the parameters is a bit too high.
> 
> Ok, will fix.
> 
>>> +{
>>> +     struct kmx61_data *data = iio_priv(indio_dev);
>>> +     int ret;
>>> +     u8 base_reg;
>>> +
>>> +     switch (mask) {
>>> +     case IIO_CHAN_INFO_RAW:
>>> +             switch (chan->type) {
>>> +             case IIO_ACCEL:
>>> +             case IIO_MAGN:
>>> +                     base_reg = KMX61_ACC_XOUT_L;
>>> +                     break;
>>> +             default:
>>> +                     return -EINVAL;
>>> +             }
>>> +             mutex_lock(&data->lock);
>>> +
>>> +             kmx61_set_power_state(data, true, chan->address);
>>> +             ret = kmx61_read_measurement(data, base_reg, chan->scan_index);
>>> +             if (ret < 0) {
>>> +                     kmx61_set_power_state(data, false, chan->address);
>>> +                     mutex_unlock(&data->lock);
>>> +                     return ret;
>>> +             }
>>> +             *val = sign_extend32(ret >> chan->scan_type.shift,
>>> +                                  chan->scan_type.realbits - 1);
>>> +             kmx61_set_power_state(data, false, chan->address);
>>> +
>>> +             mutex_unlock(&data->lock);
>>> +             return IIO_VAL_INT;
>>> +     case IIO_CHAN_INFO_SCALE:
>>> +             switch (chan->type) {
>>> +             case IIO_ACCEL:
>>> +                     *val = 0;
>>> +                     *val2 = kmx61_scale_table[data->range].uscale;
>>> +                     return IIO_VAL_INT_PLUS_MICRO;
>>> +             case IIO_MAGN:
>>> +                     /* 14 bits res, 1465 microGauss per magn count */
>>> +                     *val = 0;
>>> +                     *val2 = 1465;
>>> +                     return IIO_VAL_INT_PLUS_MICRO;
>>> +             default:
>>> +                     return -EINVAL;
>>> +             }
>>> +     case IIO_CHAN_INFO_SAMP_FREQ:
>>> +             if (chan->type != IIO_ACCEL && chan->type != IIO_MAGN)
>>> +                     return -EINVAL;
>>> +
>>> +             mutex_lock(&data->lock);
>>> +             ret = kmx61_get_odr(data, val, val2, chan->address);
>>> +             mutex_unlock(&data->lock);
>> Could be just return ret here, if kmx61_get_odr() returns IIO_VAL_INT_PLUS_MICRO on success and -EINVAL on error.
> 
> As explained above, I prefer not to do this. More than that, I can't do ret here
> because the data->lock is held.
> 
Well, I actually intended to have an unconditional "return ret;" right after the mutex_unlock. But up to you. I also see a good point to make it obvious here, that the return value is IIO_VAL_INT_PLUS_MICRO. So, up to you.
>>> +             if (ret)
>>> +                     return -EINVAL;
>>> +             return IIO_VAL_INT_PLUS_MICRO;
>>> +     }
>>> +     return -EINVAL;
>>> +}
>>> +
>>> +static int kmx61_write_raw(struct iio_dev *indio_dev,
>>> +                            struct iio_chan_spec const *chan, int val,
>>> +                            int val2, long mask)
>> Indentation of parameters is too high.
> Ok.
>>> +{
>>> +     struct kmx61_data *data = iio_priv(indio_dev);
>>> +     int ret;
>>> +
>>> +     switch (mask) {
>>> +     case IIO_CHAN_INFO_SAMP_FREQ:
>>> +             if (chan->type != IIO_ACCEL && chan->type != IIO_MAGN)
>>> +                     return -EINVAL;
>>> +
>>> +             mutex_lock(&data->lock);
>>> +             ret = kmx61_set_odr(data, val, val2, chan->address);
>>> +             mutex_unlock(&data->lock);
>>> +             return ret;
>>> +     case IIO_CHAN_INFO_SCALE:
>>> +             switch (chan->type) {
>>> +             case IIO_ACCEL:
>>> +                     if (val != 0)
>>> +                             return -EINVAL;
>>> +                     mutex_lock(&data->lock);
>>> +                     ret = kmx61_set_scale(data, val2);
>>> +                     mutex_unlock(&data->lock);
>>> +                     return ret;
>>> +             default:
>>> +                     return -EINVAL;
>>> +             }
>>> +             return ret;
>> This won't be reached (if so, ret would not be initialized).
>>> +     default:
>>> +             return -EINVAL;
>>> +     }
>>> +     return ret;
>> Same here.
> 
> I know. Would initializing ret with 0 will be enough?
Since the switch already catches all cases and returns, there is no need for return ret, here. But if you prefer to have a return at the end of the function, you could drop the default case, or leave it empty, or just put a break in there - and return -EINVAL at the end.
> 
>>> +}
>>> +
>>> +static const struct iio_info kmx61_info = {
>>> +     .driver_module          = THIS_MODULE,
>>> +     .read_raw               = kmx61_read_raw,
>>> +     .write_raw              = kmx61_write_raw,
>>> +     .attrs                  = &kmx61_attribute_group,
>>> +};
>>> +
>>> +static int kmx61_probe(struct i2c_client *client,
>>> +                    const struct i2c_device_id *id)
>>> +{
>>> +     struct kmx61_data *data;
>>> +     struct iio_dev *indio_dev;
>>> +     int ret;
>>> +     const char *name = NULL;
>>> +
>>> +     indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>>> +     if (!indio_dev)
>>> +             return -ENOMEM;
>>> +
>>> +     data = iio_priv(indio_dev);
>>> +     i2c_set_clientdata(client, indio_dev);
>>> +     data->client = client;
>>> +
>>> +     if (id)
>>> +             name = id->name;
>>> +
>>> +     indio_dev->dev.parent = &client->dev;
>>> +     indio_dev->channels = kmx61_channels;
>>> +     indio_dev->num_channels = ARRAY_SIZE(kmx61_channels);
>>> +     indio_dev->name = name;
>>> +     indio_dev->modes = INDIO_DIRECT_MODE;
>>> +     indio_dev->info = &kmx61_info;
>>> +
>>> +     mutex_init(&data->lock);
>>> +
>>> +     ret = kmx61_chip_init(data);
>>> +     if (ret < 0)
>>> +             return ret;
>>> +
>>> +     ret = iio_device_register(indio_dev);
>>> +     if (ret < 0) {
>>> +             dev_err(&client->dev, "Failed to register iio device\n");
>>> +             goto err_iio_device_register;
>>> +     }
>>> +
>>> +     ret = pm_runtime_set_active(&client->dev);
>>> +     if (ret < 0)
>>> +             goto err_pm_runtime_set_active;
>>> +
>>> +     pm_runtime_enable(&client->dev);
>>> +     pm_runtime_set_autosuspend_delay(&client->dev, KMX61_SLEEP_DELAY_MS);
>>> +     pm_runtime_use_autosuspend(&client->dev);
>>> +
>>> +     return 0;
>>> +
>>> +err_pm_runtime_set_active:
>>> +     iio_device_unregister(indio_dev);
>>> +err_iio_device_register:
>>> +     kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
>>> +     return ret;
>>> +}
>>> +
>>> +static int kmx61_remove(struct i2c_client *client)
>>> +{
>>> +     struct iio_dev *indio_dev = i2c_get_clientdata(client);
>>> +     struct kmx61_data *data = iio_priv(indio_dev);
>>> +     int ret;
>>> +
>>> +     pm_runtime_disable(&client->dev);
>>> +     pm_runtime_set_suspended(&client->dev);
>>> +     pm_runtime_put_noidle(&client->dev);
>>> +
>>> +     iio_device_unregister(indio_dev);
>>> +
>>> +     mutex_lock(&data->lock);
>>> +     ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
>>> +     mutex_unlock(&data->lock);
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +#ifdef CONFIG_PM_SLEEP
>>> +static int kmx61_suspend(struct device *dev)
>>> +{
>>> +     struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>>> +     struct kmx61_data *data = iio_priv(indio_dev);
>>> +     int ret;
>>> +
>>> +     mutex_lock(&data->lock);
>>> +     ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG,
>>> +                          false);
>>> +     mutex_unlock(&data->lock);
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +static int kmx61_resume(struct device *dev)
>>> +{
>>> +     struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>>> +     struct kmx61_data *data = iio_priv(indio_dev);
>>> +     u8 stby = 0;
>>> +
>>> +     if (data->acc_stby)
>>> +             stby |= KMX61_ACC_STBY_BIT;
>>> +     if (data->mag_stby)
>>> +             stby |= KMX61_MAG_STBY_BIT;
>>> +
>>> +     return kmx61_set_mode(data, stby, KMX61_ACC | KMX61_MAG, true);
>>> +}
>>> +#endif
>>> +
>>> +#ifdef CONFIG_PM_RUNTIME
>>> +static int kmx61_runtime_suspend(struct device *dev)
>>> +{
>>> +     struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>>> +     struct kmx61_data *data = iio_priv(indio_dev);
>>> +     int ret;
>>> +
>>> +     mutex_lock(&data->lock);
>>> +     ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
>>> +     mutex_unlock(&data->lock);
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +static int kmx61_runtime_resume(struct device *dev)
>>> +{
>>> +     struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>>> +     struct kmx61_data *data = iio_priv(indio_dev);
>>> +     u8 stby = 0;
>>> +
>>> +     if (!data->acc_ps)
>>> +             stby |= KMX61_ACC_STBY_BIT;
>>> +     if (!data->mag_ps)
>>> +             stby |= KMX61_MAG_STBY_BIT;
>>> +
>>> +     return kmx61_set_mode(data, stby, KMX61_ACC | KMX61_MAG, true);
>>> +}
>>> +#endif
>>> +
>>> +static const struct dev_pm_ops kmx61_pm_ops = {
>>> +     SET_SYSTEM_SLEEP_PM_OPS(kmx61_suspend, kmx61_resume)
>>> +     SET_RUNTIME_PM_OPS(kmx61_runtime_suspend, kmx61_runtime_resume, NULL)
>>> +};
>>> +
>>> +static const struct i2c_device_id kmx61_id[] = {
>>> +     {"kmx611021", 0},
>>> +     {}
>>> +};
>>> +
>>> +MODULE_DEVICE_TABLE(i2c, kmx61_id);
>>> +
>>> +static struct i2c_driver kmx61_driver = {
>>> +     .driver = {
>>> +             .name = KMX61_DRV_NAME,
>>> +             .pm = &kmx61_pm_ops,
>>> +     },
>>> +     .probe          = kmx61_probe,
>>> +     .remove         = kmx61_remove,
>>> +     .id_table       = kmx61_id,
>>> +};
>>> +
>>> +module_i2c_driver(kmx61_driver);
>>> +
>>> +MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com>");
>>> +MODULE_DESCRIPTION("KMX61 accelerometer/magnetometer driver");
>>> +MODULE_LICENSE("GPL v2");
> 
> Hartmut, thanks a lot for your reviews. I will send a patch series
> with these cleanups together with changes for ACPI, buffer and trigger
> support.
Don't know if Jonathan would like to see it smashed together, already.
Take care

Hartmut
> 
> Daniel.
> --
> 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
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] iio: imu: Add support for Kionix KMX61 sensor
  2014-11-25 22:31     ` Hartmut Knaack
@ 2014-11-25 22:44       ` Daniel Baluta
  2014-11-25 23:03         ` Daniel Baluta
  0 siblings, 1 reply; 7+ messages in thread
From: Daniel Baluta @ 2014-11-25 22:44 UTC (permalink / raw)
  To: Hartmut Knaack
  Cc: Daniel Baluta, Jonathan Cameron, Peter Meerwald,
	Srinivas Pandruvada, linux-iio, Linux Kernel Mailing List

On Wed, Nov 26, 2014 at 12:31 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
> Daniel Baluta schrieb am 25.11.2014 10:54:
>> On Tue, Nov 25, 2014 at 2:45 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
>>> Daniel Baluta schrieb am 18.11.2014 17:47:
>>>> Minimal implementation for KMX61 6-axis accelerometer/magnetometer. It exports
>>>> raw accel/magn readings together with scale and sampling frequency.
>>>>
>>>> Datasheet will be available at:
>>>> http://www.kionix.com/6-axis-accelerometer-magnetometer/kmx61
>>>>
>>> Finally managed to have a close look, and found a few issues.
>>>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>>>> ---
>>>> Changes since v2:
>>>>       * mostly style fixes after comments received from Jonathan and Peter
>>>>               * http://marc.info/?l=linux-iio&m=141564019602400&w=2
>>>>       * implemented PM runtime and PM sleep resume/suspend hooks
>>>>       * introduced new parameter for kmx61_set_mode to help updating
>>>>       stby bits in kmx61_data.
>>>>       * use kmx6111021 instead of kmx61 for i2c_device_id.
>>>>
>>>>  drivers/iio/imu/Kconfig  |   9 +
>>>>  drivers/iio/imu/Makefile |   2 +
>>>>  drivers/iio/imu/kmx61.c  | 766 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 777 insertions(+)
>>>>  create mode 100644 drivers/iio/imu/kmx61.c
>>>>
>>>> diff --git a/drivers/iio/imu/Kconfig b/drivers/iio/imu/Kconfig
>>>> index 2b0e451..d675f43 100644
>>>> --- a/drivers/iio/imu/Kconfig
>>>> +++ b/drivers/iio/imu/Kconfig
>>>> @@ -25,6 +25,15 @@ config ADIS16480
>>>>         Say yes here to build support for Analog Devices ADIS16375, ADIS16480,
>>>>         ADIS16485, ADIS16488 inertial sensors.
>>>>
>>>> +config KMX61
>>>> +     tristate "Kionix KMX61 6-axis accelerometer and magnetometer"
>>>> +     depends on I2C
>>>> +     help
>>>> +       Say Y here if you want to build a driver for Kionix KMX61 6-axis accelerometer
>>>> +       and magnetometer.
>>>> +       To compile this driver as module, choose M here: the module will be called
>>>> +       kmx61.
>>>> +
>>> This help text is more than 80 characters wide and may better be wrapped a bit differently.
>>
>> Good catch. checkpatch.pl didn't complain.
>>
>>>>  source "drivers/iio/imu/inv_mpu6050/Kconfig"
>>>>
>>>>  endmenu
>>>> diff --git a/drivers/iio/imu/Makefile b/drivers/iio/imu/Makefile
>>>> index 114d2c1..e1e6e3d 100644
>>>> --- a/drivers/iio/imu/Makefile
>>>> +++ b/drivers/iio/imu/Makefile
>>>> @@ -14,3 +14,5 @@ adis_lib-$(CONFIG_IIO_ADIS_LIB_BUFFER) += adis_buffer.o
>>>>  obj-$(CONFIG_IIO_ADIS_LIB) += adis_lib.o
>>>>
>>>>  obj-y += inv_mpu6050/
>>>> +
>>>> +obj-$(CONFIG_KMX61) += kmx61.o
>>>> diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c
>>>> new file mode 100644
>>>> index 0000000..f68b3ef
>>>> --- /dev/null
>>>> +++ b/drivers/iio/imu/kmx61.c
>>>> @@ -0,0 +1,766 @@
>>>> +/*
>>>> + * KMX61 - Kionix 6-axis Accelerometer/Magnetometer
>>>> + *
>>>> + * Copyright (c) 2014, 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.
>>>> + *
>>>> + * IIO driver for KMX61 (7-bit I2C slave address 0x0E or 0x0F).
>>>> + *
>>>> + * TODO: buffer, interrupt, thresholds, acpi, temperature sensor
>>>> + *
>>>> + */
>>>> +
>>>> +#include <linux/module.h>
>>>> +#include <linux/i2c.h>
>>>> +#include <linux/pm.h>
>>>> +#include <linux/pm_runtime.h>
>>>> +#include <linux/iio/iio.h>
>>>> +#include <linux/iio/sysfs.h>
>>>> +
>>>> +#define KMX61_DRV_NAME "kmx61"
>>>> +
>>>> +#define KMX61_REG_WHO_AM_I   0x00
>>>> +
>>>> +/*
>>>> + * three 16-bit accelerometer output registers for X/Y/Z axis
>>>> + * we use only XOUT_L as a base register, all other addresses
>>>> + * can be obtained by applying an offset and are provided here
>>>> + * only for clarity.
>>>> + */
>>>> +#define KMX61_ACC_XOUT_L     0x0A
>>>> +#define KMX61_ACC_XOUT_H     0x0B
>>>> +#define KMX61_ACC_YOUT_L     0x0C
>>>> +#define KMX61_ACC_YOUT_H     0x0D
>>>> +#define KMX61_ACC_ZOUT_L     0x0E
>>>> +#define KMX61_ACC_ZOUT_H     0x0F
>>>> +
>>>> +/*
>>>> + * one 16-bit temperature output register
>>>> + */
>>>> +#define KMX61_TEMP_L         0x10
>>>> +#define KMX61_TEMP_H         0x11
>>>> +
>>>> +/*
>>>> + * three 16-bit magnetometer output registers for X/Y/Z axis
>>>> + */
>>>> +#define KMX61_MAG_XOUT_L     0x12
>>>> +#define KMX61_MAG_XOUT_H     0x13
>>>> +#define KMX61_MAG_YOUT_L     0x14
>>>> +#define KMX61_MAG_YOUT_H     0x15
>>>> +#define KMX61_MAG_ZOUT_L     0x16
>>>> +#define KMX61_MAG_ZOUT_H     0x17
>>>> +
>>>> +#define KMX61_REG_ODCNTL     0x2C
>>>> +#define KMX61_REG_STBY               0x29
>>>> +#define KMX61_REG_CTRL1              0x2A
>>>> +
>>>> +#define KMX61_ACC_STBY_BIT   BIT(0)
>>>> +#define KMX61_MAG_STBY_BIT   BIT(1)
>>>> +#define KMX61_ACT_STBY_BIT   BIT(7)
>>>> +
>>>> +#define KMX61_ALL_STBY               (KMX61_ACC_STBY_BIT | KMX61_MAG_STBY_BIT)
>>>> +
>>>> +#define KMX61_REG_CTRL1_GSEL0_SHIFT  0
>>>> +#define KMX61_REG_CTRL1_GSEL1_SHIFT  1
>>>> +#define KMX61_REG_CTRL1_GSEL0_MASK   0x01
>>>> +#define KMX61_REG_CTRL1_GSEL1_MASK   0x02
>>> I don't see the benefit of separating GSEL0 and GSEL1.
>>>> +
>>>> +#define KMX61_REG_CTRL1_BIT_RES              BIT(4)
>>>> +
>>>> +#define KMX61_ACC_ODR_SHIFT  0
>>>> +#define KMX61_MAG_ODR_SHIFT  4
>>>> +#define KMX61_ACC_ODR_MASK   0x0F
>>>> +#define KMX61_MAG_ODR_MASK   0xF0
>>>> +
>>>> +#define KMX61_SLEEP_DELAY_MS 2000
>>>> +
>>>> +#define KMX61_CHIP_ID                0x12
>>>> +
>>>> +struct kmx61_data {
>>>> +     struct i2c_client *client;
>>>> +
>>>> +     /* serialize access to non-atomic ops, e.g set_mode */
>>>> +     struct mutex lock;
>>>> +     u8 range;
>>>> +     u8 odr_bits;
>>>> +
>>>> +     /* standby state */
>>>> +     u8 acc_stby;
>>>> +     u8 mag_stby;
>>> Why u8 instead of bool?
>>
>> Hmm, correct. I was thinking bitwise, but you are right. Will fix.
>>
>>>> +
>>>> +     /* power state */
>>>> +     bool acc_ps;
>>>> +     bool mag_ps;
>>>> +};
>>>> +
>>>> +enum kmx61_range {
>>>> +     KMX61_RANGE_2G,
>>>> +     KMX61_RANGE_4G,
>>>> +     KMX61_RANGE_8G,
>>>> +};
>>>> +
>>>> +enum kmx61_scan {
>>>> +     KMX61_SCAN_ACC_X,
>>>> +     KMX61_SCAN_ACC_Y,
>>>> +     KMX61_SCAN_ACC_Z,
>>>> +     KMX61_SCAN_TEMP,
>>>> +     KMX61_SCAN_MAG_X,
>>>> +     KMX61_SCAN_MAG_Y,
>>>> +     KMX61_SCAN_MAG_Z,
>>>> +};
>>>> +
>>>> +static const struct {
>>>> +     u16 uscale;
>>>> +     u8 gsel0;
>>>> +     u8 gsel1;
>>>> +} kmx61_scale_table[] = {
>>>> +     {9582, 0, 0},
>>>> +     {19163, 1, 0},
>>>> +     {38326, 0, 1},
>>> Why not consolidate gsel0 and gsel1 to gsel? Then this could become u16 kmx61_scale[] = {9582, 19163, 38326} with gsel as index.
>>
>> True. Will fix.
>>
>>>> +};
>>>> +
>>>> +/* KMX61 devices */
>>>> +#define KMX61_ACC    0x01
>>>> +#define KMX61_MAG    0x02
>>>> +
>>>> +static const struct {
>>>> +     int val;
>>>> +     int val2;
>>>> +     u8 odr_bits;
>>>> +} kmx61_samp_freq_table[] = { {12, 500000, 0x00},
>>>> +                     {25, 0, 0x01},
>>>> +                     {50, 0, 0x02},
>>>> +                     {100, 0, 0x03},
>>>> +                     {200, 0, 0x04},
>>>> +                     {400, 0, 0x05},
>>>> +                     {800, 0, 0x06},
>>>> +                     {1600, 0, 0x07},
>>>> +                     {0, 781000, 0x08},
>>>> +                     {1, 563000, 0x09},
>>>> +                     {3, 125000, 0x0A},
>>>> +                     {6, 250000, 0x0B} };
>>>> +
>>>> +static IIO_CONST_ATTR(accel_scale_available, "0.009582 0.019163 0.038326");
>>>> +static IIO_CONST_ATTR(magn_scale_available, "0.001465");
>>>> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
>>>> +     "0.781000 1.563000 3.125000 6.250000 12.500000 25 50 100 200 400 800");
>>>> +
>>>> +static struct attribute *kmx61_attributes[] = {
>>>> +     &iio_const_attr_accel_scale_available.dev_attr.attr,
>>>> +     &iio_const_attr_magn_scale_available.dev_attr.attr,
>>>> +     &iio_const_attr_sampling_frequency_available.dev_attr.attr,
>>>> +     NULL,
>>>> +};
>>>> +
>>>> +static const struct attribute_group kmx61_attribute_group = {
>>>> +     .attrs = kmx61_attributes,
>>>> +};
>>>> +
>>>> +#define KMX61_ACC_CHAN(_axis, _index) { \
>>>> +     .type = IIO_ACCEL, \
>>>> +     .modified = 1, \
>>>> +     .channel2 = IIO_MOD_ ## _axis, \
>>>> +     .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>>>> +     .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
>>>> +                             BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>>>> +     .address = KMX61_ACC, \
>>>> +     .scan_index = _index, \
>>>> +     .scan_type = { \
>>>> +             .sign = 's', \
>>>> +             .realbits = 12, \
>>>> +             .storagebits = 16, \
>>>> +             .shift = 4, \
>>>> +             .endianness = IIO_LE, \
>>>> +     }, \
>>>> +}
>>>> +
>>>> +#define KMX61_MAG_CHAN(_axis, _index) { \
>>>> +     .type = IIO_MAGN, \
>>>> +     .modified = 1, \
>>>> +     .channel2 = IIO_MOD_ ## _axis, \
>>>> +     .address = KMX61_MAG, \
>>>> +     .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>>>> +     .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
>>>> +                             BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>>>> +     .scan_index = _index, \
>>>> +     .scan_type = { \
>>>> +             .sign = 's', \
>>>> +             .realbits = 14, \
>>>> +             .storagebits = 16, \
>>>> +             .shift = 2, \
>>>> +             .endianness = IIO_LE, \
>>>> +     }, \
>>>> +}
>>>> +
>>>> +static const struct iio_chan_spec kmx61_channels[] = {
>>>> +     KMX61_ACC_CHAN(X, KMX61_SCAN_ACC_X),
>>>> +     KMX61_ACC_CHAN(Y, KMX61_SCAN_ACC_Y),
>>>> +     KMX61_ACC_CHAN(Z, KMX61_SCAN_ACC_Z),
>>>> +     KMX61_MAG_CHAN(X, KMX61_SCAN_MAG_X),
>>>> +     KMX61_MAG_CHAN(Y, KMX61_SCAN_MAG_Y),
>>>> +     KMX61_MAG_CHAN(Z, KMX61_SCAN_MAG_Z),
>>>> +};
>>>> +
>>>> +static int kmx61_convert_freq_to_bit(int val, int val2)
>>>> +{
>>>> +     int i;
>>>> +
>>>> +     for (i = 0; i < ARRAY_SIZE(kmx61_samp_freq_table); i++)
>>>> +             if (val == kmx61_samp_freq_table[i].val &&
>>>> +                 val2 == kmx61_samp_freq_table[i].val2)
>>>> +                     return kmx61_samp_freq_table[i].odr_bits;
>>>> +     return -EINVAL;
>>>> +}
>>>> +/**
>>> Like here, the first line of the following multiline comments have one asterisk too much.
>>
>> I think this is the convention, when adding doxygen style documentation:
>> http://lxr.free-electrons.com/source/fs/eventpoll.c#L382
>>
>>>> + * kmx61_set_mode() - set KMX61 device operating mode
>>>> + * @data - kmx61 device private data pointer
>>>> + * @mode - bitmask, indicating operating mode for @device
>>>> + * @device - bitmask, indicating device for which @mode needs to be set
>>>> + * @update - update stby bits stored in device's private  @data
>>>> + *
>>>> + * For each sensor (accelerometer/magnetometer) there are two operating modes
>>>> + * STANDBY and OPERATION. Neither accel nor magn can be disabled independently
>>>> + * if they are both enabled. Internal sensors state is saved in acc_stby and
>>>> + * mag_stby members of driver's private @data.
>>>> + */
>>>> +static int kmx61_set_mode(struct kmx61_data *data, u8 mode, u8 device,
>>>> +                       bool update)
>>>> +{
>>>> +     int ret;
>>>> +     int acc_stby = -1, mag_stby = -1;
>>>> +
>>>> +     ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_STBY);
>>>> +     if (ret < 0) {
>>>> +             dev_err(&data->client->dev, "Error reading reg_stby\n");
>>>> +             return ret;
>>>> +     }
>>>> +     if (device & KMX61_ACC) {
>>>> +             if (mode & KMX61_ACC_STBY_BIT) {
>>>> +                     ret |= KMX61_ACC_STBY_BIT;
>>>> +                     acc_stby = 1;
>>>> +             } else {
>>>> +                     ret &= ~KMX61_ACC_STBY_BIT;
>>>> +                     acc_stby = 0;
>>>> +             }
>>>> +     }
>>>> +
>>>> +     if (device & KMX61_MAG) {
>>>> +             if (mode & KMX61_MAG_STBY_BIT) {
>>>> +                     ret |= KMX61_MAG_STBY_BIT;
>>>> +                     mag_stby = 1;
>>>> +             } else {
>>>> +                     ret &= ~KMX61_MAG_STBY_BIT;
>>>> +                     mag_stby = 0;
>>>> +             }
>>>> +     }
>>>> +
>>>> +     ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_STBY, ret);
>>>> +     if (ret < 0) {
>>>> +             dev_err(&data->client->dev, "Error writing reg_stby\n");
>>>> +             return ret;
>>>> +     }
>>>> +
>>>> +     if (acc_stby != -1 && update)
>>>> +             data->acc_stby = !!acc_stby;
>>>> +     if (mag_stby != -1 && update)
>>>> +             data->mag_stby = !!mag_stby;
>>> Why is there a need for double negation?
>>
>> Correct. Now it doesn't make sense since data->acc_stby is u8.
>>
>> But, with your suggestion if I will use bool to represent data->acc_stby
>> then I think the code should use double negation to make it clear
>> that data->acc_stby is bool and acc_stby is int.
>>
> Well, if it makes you happy, then go ahead. My interpretation of a bool is, that an assignment of 0 becomes 0/false, and an assignment of anything else becomes 1/true. So, I wouldn't see a need here (especially since acc_stby/mag_stby were distinctly assigned either 0 or 1).

All right. It makes sense.

>>>> +
>>>> +     return ret;
>>> Since no error occured until this point, just return 0.
>>
>> Ok. Will fix.
>>
>>>> +}
>>>> +
>>>> +static int kmx61_get_mode(struct kmx61_data *data, u8 *mode, u8 device)
>>>> +{
>>>> +     int ret;
>>>> +
>>>> +     ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_STBY);
>>>> +     if (ret < 0) {
>>>> +             dev_err(&data->client->dev, "Error reading reg_stby\n");
>>>> +             return ret;
>>>> +     }
>>>> +     *mode = 0;
>>>> +
>>>> +     if (device & KMX61_ACC) {
>>>> +             if (ret & KMX61_ACC_STBY_BIT)
>>>> +                     *mode |= KMX61_ACC_STBY_BIT;
>>>> +             else
>>>> +                     *mode &= ~KMX61_ACC_STBY_BIT;
>>> Simply:
>>>                 *mode |= ret & KMX61_ACC_STBY_BIT;
>> Ok.
>>
>>>> +     }
>>>> +
>>>> +     if (device & KMX61_MAG) {
>>>> +             if (ret & KMX61_MAG_STBY_BIT)
>>>> +                     *mode |= KMX61_MAG_STBY_BIT;
>>>> +             else
>>>> +                     *mode &= ~KMX61_MAG_STBY_BIT;
>>> Same here:
>> Ok.
>>
>>>                 *mode |= ret & KMX61_MAG_STBY_BIT;
>>>> +     }
>>>> +
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static int kmx61_set_odr(struct kmx61_data *data, int val, int val2, u8 device)
>>>> +{
>>>> +     int ret;
>>>> +     u8 mode;
>>>> +     int lodr_bits, odr_bits;
>>>> +
>>>> +     ret = kmx61_get_mode(data, &mode, KMX61_ACC | KMX61_MAG);
>>>> +     if (ret < 0)
>>>> +             return ret;
>>>> +
>>>> +     lodr_bits = kmx61_convert_freq_to_bit(val, val2);
>>>> +     if (lodr_bits < 0)
>>>> +             return lodr_bits;
>>>> +
>>>> +     /* To change ODR, accel and magn must be in STDBY */
>>>> +     ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG,
>>>> +                          true);
>>>> +     if (ret < 0)
>>>> +             return ret;
>>>> +
>>>> +     odr_bits = 0;
>>>> +     if (device & KMX61_ACC)
>>>> +             odr_bits |= lodr_bits;
>>>> +     if (device & KMX61_MAG)
>>>> +             odr_bits |= (lodr_bits << KMX61_MAG_ODR_SHIFT);
>>> No need for parenthesis.
>> ok.
>>
>>>> +
>>>> +     ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_ODCNTL,
>>>> +                                     odr_bits);
>>>> +     if (ret < 0)
>>>> +             return ret;
>>>> +
>>>> +     ret = kmx61_set_mode(data, mode, KMX61_ACC | KMX61_MAG, true);
>>>> +     if (ret < 0)
>>>> +             return ret;
>>>> +
>>>> +     data->odr_bits = lodr_bits;
>>>> +
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static
>>>> +int kmx61_get_odr(struct kmx61_data *data, int *val, int *val2, u8 device)
>>> Usually wrap parameters? And don't put code on line with {.
>>
>> Will fix.
>>
>>>> +{    int i;
>>>> +     u8 lodr_bits;
>>>> +
>>>> +     if (device & KMX61_ACC)
>>>> +             lodr_bits = (data->odr_bits >> KMX61_ACC_ODR_SHIFT) &
>>>> +                          KMX61_ACC_ODR_MASK;
>>> KMX61_ACC_ODR_SHIFT is just a placebo and not used in kmx61_set_odr. Choose one way and stay consistent.
>>
>> placebo is good.  will fix kmx61_set_odr :)
>>
>>>> +     else if (device & KMX61_MAG)
>>>> +             lodr_bits = (data->odr_bits >> KMX61_MAG_ODR_SHIFT) &
>>>> +                          KMX61_MAG_ODR_MASK;
>>>> +     else
>>>> +             return -EINVAL;
>>>> +
>>>> +     for (i = 0; i < ARRAY_SIZE(kmx61_samp_freq_table); i++)
>>>> +             if (lodr_bits == kmx61_samp_freq_table[i].odr_bits) {
>>>> +                     *val = kmx61_samp_freq_table[i].val;
>>>> +                     *val2 = kmx61_samp_freq_table[i].val2;
>>>> +                     return 0;
>>> return IIO_VAL_INT_PLUS_MICRO allows you to faciliate _read_raw a bit to just return ret there.
>>
>> Hmm, If you don't mind I want to keep this as it is, just to
>> be consistent with the rest of the functions. The convention
>> used is to return 0 in case of success.
>>
> Up to you. Though kmx61_read_raw() would be an example of a function that doesn't follow this convention ;-) And there is no real benefit of checking an error code there, which is passed up anyway.
>>>> +             }
>>>> +     return -EINVAL;
>>>> +}
>>>> +
>>>> +static int kmx61_set_range(struct kmx61_data *data, int range)
>>>> +{
>>>> +     int ret;
>>>> +
>>>> +     ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_CTRL1);
>>>> +     if (ret < 0) {
>>>> +             dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
>>>> +             return ret;
>>>> +     }
>>>> +
>>>> +     ret &= ~(KMX61_REG_CTRL1_GSEL0_MASK | KMX61_REG_CTRL1_GSEL1_MASK);
>>>> +     ret |= kmx61_scale_table[range].gsel0 << KMX61_REG_CTRL1_GSEL0_SHIFT;
>>>> +     ret |= kmx61_scale_table[range].gsel1 << KMX61_REG_CTRL1_GSEL1_SHIFT;
>>>> +
>>>> +     ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_CTRL1, ret);
>>>> +     if (ret < 0) {
>>>> +             dev_err(&data->client->dev, "Error writing reg_ctrl1\n");
>>>> +             return ret;
>>>> +     }
>>>> +
>>>> +     data->range = range;
>>>> +
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static int kmx61_set_scale(struct kmx61_data *data, int uscale)
>>>> +{
>>>> +     int ret, i;
>>>> +     u8  mode;
>>>> +
>>>> +     for (i = 0; i < ARRAY_SIZE(kmx61_scale_table); i++) {
>>>> +             if (kmx61_scale_table[i].uscale == uscale) {
>>>> +                     ret = kmx61_get_mode(data, &mode,
>>>> +                                          KMX61_ACC | KMX61_MAG);
>>>> +                     if (ret < 0)
>>>> +                             return ret;
>>>> +
>>>> +                     ret = kmx61_set_mode(data, KMX61_ALL_STBY,
>>>> +                                          KMX61_ACC | KMX61_MAG, true);
>>>> +                     if (ret < 0)
>>>> +                             return ret;
>>>> +
>>>> +                     ret = kmx61_set_range(data, i);
>>>> +                     if (ret < 0)
>>>> +                             return ret;
>>>> +
>>>> +                     return  kmx61_set_mode(data, mode,
>>>> +                                            KMX61_ACC | KMX61_MAG, true);
>>>> +             }
>>>> +     }
>>>> +     return -EINVAL;
>>>> +}
>>>> +
>>>> +static int kmx61_chip_init(struct kmx61_data *data)
>>>> +{
>>>> +     int ret;
>>>> +
>>>> +     ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_WHO_AM_I);
>>>> +     if (ret < 0) {
>>>> +             dev_err(&data->client->dev, "Error reading who_am_i\n");
>>>> +             return ret;
>>>> +     }
>>>> +
>>>> +     if (ret != KMX61_CHIP_ID) {
>>>> +             dev_err(&data->client->dev,
>>>> +                     "Wrong chip id, got %x expected %x\n",
>>>> +                      ret, KMX61_CHIP_ID);
>>>> +             return -EINVAL;
>>>> +     }
>>>> +
>>>> +     /* set accel 12bit, 4g range */
>>>> +     ret = kmx61_set_range(data, KMX61_RANGE_4G);
>>>> +     if (ret < 0)
>>>> +             return ret;
>>>> +
>>>> +     /* set acc/magn to OPERATION mode */
>>>> +     ret = kmx61_set_mode(data, 0, KMX61_ACC | KMX61_MAG, true);
>>> Just return kmx61_set_mode(data, 0, KMX61_ACC | KMX61_MAG, true);
>>
>> Ok.
>>
>>>> +     if (ret < 0)
>>>> +             return ret;
>>>> +
>>>> +     return 0;
>>>> +}
>>>> +/**
>>>> + * kmx61_set_power_state() - set power state for kmx61 @device
>>>> + * @data - kmx61 device private pointer
>>>> + * @on - power state to be set for @device
>>>> + * @device - bitmask indicating device for which @on state needs to be set
>>>> + *
>>>> + * Notice that when ACC power state needs to be set to ON and MAG is in
>>>> + * OPERATION then we know that kmx61_runtime_resume was already called
>>>> + * so we must set ACC OPERATION mode here. The same happens when MAG power
>>>> + * state needs to be set to ON and ACC is in OPERATION.
>>>> + */
>>>> +static int kmx61_set_power_state(struct kmx61_data *data, bool on, u8 device)
>>>> +{
>>>> +#ifdef CONFIG_PM_RUNTIME
>>>> +     int ret;
>>>> +
>>>> +     if (device & KMX61_ACC) {
>>>> +             if (on && !data->acc_ps && !data->mag_stby)
>>>> +                     kmx61_set_mode(data, 0, KMX61_ACC, true);
>>>> +             data->acc_ps = on;
>>>> +     }
>>>> +     if (device & KMX61_MAG) {
>>>> +             if (on && !data->mag_ps && !data->acc_stby)
>>>> +                     kmx61_set_mode(data, 0, KMX61_MAG, true);
>>>> +             data->mag_ps = on;
>>>> +     }
>>>> +
>>>> +     if (on) {
>>>> +             ret = pm_runtime_get_sync(&data->client->dev);
>>>> +     } else {
>>>> +             pm_runtime_mark_last_busy(&data->client->dev);
>>>> +             ret = pm_runtime_put_autosuspend(&data->client->dev);
>>>> +     }
>>>> +     if (ret < 0) {
>>>> +             dev_err(&data->client->dev,
>>>> +                     "Failed: kmx61_set_power_state for %d, ret %d\n",
>>>> +                     on, ret);
>>>> +             return ret;
>>>> +     }
>>>> +#endif
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static int kmx61_read_measurement(struct kmx61_data *data, int base, int offset)
>>> u8 base, u8 offset?
>>
>> Correct.
>>
>>>> +{
>>>> +     int ret;
>>>> +     u8 reg = base + offset * 2;
>>>> +
>>>> +     ret = i2c_smbus_read_word_data(data->client, reg);
>>>> +     if (ret < 0) {
>>>> +             dev_err(&data->client->dev, "failed to read reg at %x\n", reg);
>>>> +             return ret;
>>> This return is unnecessary.
>>
>> Ok.
>>
>>>> +     }
>>>> +
>>>> +     return ret;
>>>> +}
>>>> +
>>>> +static int kmx61_read_raw(struct iio_dev *indio_dev,
>>>> +                           struct iio_chan_spec const *chan, int *val,
>>>> +                           int *val2, long mask)
>>> Indentation of the parameters is a bit too high.
>>
>> Ok, will fix.
>>
>>>> +{
>>>> +     struct kmx61_data *data = iio_priv(indio_dev);
>>>> +     int ret;
>>>> +     u8 base_reg;
>>>> +
>>>> +     switch (mask) {
>>>> +     case IIO_CHAN_INFO_RAW:
>>>> +             switch (chan->type) {
>>>> +             case IIO_ACCEL:
>>>> +             case IIO_MAGN:
>>>> +                     base_reg = KMX61_ACC_XOUT_L;
>>>> +                     break;
>>>> +             default:
>>>> +                     return -EINVAL;
>>>> +             }
>>>> +             mutex_lock(&data->lock);
>>>> +
>>>> +             kmx61_set_power_state(data, true, chan->address);
>>>> +             ret = kmx61_read_measurement(data, base_reg, chan->scan_index);
>>>> +             if (ret < 0) {
>>>> +                     kmx61_set_power_state(data, false, chan->address);
>>>> +                     mutex_unlock(&data->lock);
>>>> +                     return ret;
>>>> +             }
>>>> +             *val = sign_extend32(ret >> chan->scan_type.shift,
>>>> +                                  chan->scan_type.realbits - 1);
>>>> +             kmx61_set_power_state(data, false, chan->address);
>>>> +
>>>> +             mutex_unlock(&data->lock);
>>>> +             return IIO_VAL_INT;
>>>> +     case IIO_CHAN_INFO_SCALE:
>>>> +             switch (chan->type) {
>>>> +             case IIO_ACCEL:
>>>> +                     *val = 0;
>>>> +                     *val2 = kmx61_scale_table[data->range].uscale;
>>>> +                     return IIO_VAL_INT_PLUS_MICRO;
>>>> +             case IIO_MAGN:
>>>> +                     /* 14 bits res, 1465 microGauss per magn count */
>>>> +                     *val = 0;
>>>> +                     *val2 = 1465;
>>>> +                     return IIO_VAL_INT_PLUS_MICRO;
>>>> +             default:
>>>> +                     return -EINVAL;
>>>> +             }
>>>> +     case IIO_CHAN_INFO_SAMP_FREQ:
>>>> +             if (chan->type != IIO_ACCEL && chan->type != IIO_MAGN)
>>>> +                     return -EINVAL;
>>>> +
>>>> +             mutex_lock(&data->lock);
>>>> +             ret = kmx61_get_odr(data, val, val2, chan->address);
>>>> +             mutex_unlock(&data->lock);
>>> Could be just return ret here, if kmx61_get_odr() returns IIO_VAL_INT_PLUS_MICRO on success and -EINVAL on error.
>>
>> As explained above, I prefer not to do this. More than that, I can't do ret here
>> because the data->lock is held.
>>
> Well, I actually intended to have an unconditional "return ret;" right after the mutex_unlock. But up to you. I also see a good point to make it obvious here, that the return value is IIO_VAL_INT_PLUS_MICRO. So, up to you.
>>>> +             if (ret)
>>>> +                     return -EINVAL;
>>>> +             return IIO_VAL_INT_PLUS_MICRO;
>>>> +     }
>>>> +     return -EINVAL;
>>>> +}
>>>> +
>>>> +static int kmx61_write_raw(struct iio_dev *indio_dev,
>>>> +                            struct iio_chan_spec const *chan, int val,
>>>> +                            int val2, long mask)
>>> Indentation of parameters is too high.
>> Ok.
>>>> +{
>>>> +     struct kmx61_data *data = iio_priv(indio_dev);
>>>> +     int ret;
>>>> +
>>>> +     switch (mask) {
>>>> +     case IIO_CHAN_INFO_SAMP_FREQ:
>>>> +             if (chan->type != IIO_ACCEL && chan->type != IIO_MAGN)
>>>> +                     return -EINVAL;
>>>> +
>>>> +             mutex_lock(&data->lock);
>>>> +             ret = kmx61_set_odr(data, val, val2, chan->address);
>>>> +             mutex_unlock(&data->lock);
>>>> +             return ret;
>>>> +     case IIO_CHAN_INFO_SCALE:
>>>> +             switch (chan->type) {
>>>> +             case IIO_ACCEL:
>>>> +                     if (val != 0)
>>>> +                             return -EINVAL;
>>>> +                     mutex_lock(&data->lock);
>>>> +                     ret = kmx61_set_scale(data, val2);
>>>> +                     mutex_unlock(&data->lock);
>>>> +                     return ret;
>>>> +             default:
>>>> +                     return -EINVAL;
>>>> +             }
>>>> +             return ret;
>>> This won't be reached (if so, ret would not be initialized).
>>>> +     default:
>>>> +             return -EINVAL;
>>>> +     }
>>>> +     return ret;
>>> Same here.
>>
>> I know. Would initializing ret with 0 will be enough?
> Since the switch already catches all cases and returns, there is no need for return ret, here. But if you prefer to have a return at the end of the function, you could drop the default case, or leave it empty, or just put a break in there - and return -EINVAL at the end.

Ok. I will remove the the unnecessary return.

>>
>>>> +}
>>>> +
>>>> +static const struct iio_info kmx61_info = {
>>>> +     .driver_module          = THIS_MODULE,
>>>> +     .read_raw               = kmx61_read_raw,
>>>> +     .write_raw              = kmx61_write_raw,
>>>> +     .attrs                  = &kmx61_attribute_group,
>>>> +};
>>>> +
>>>> +static int kmx61_probe(struct i2c_client *client,
>>>> +                    const struct i2c_device_id *id)
>>>> +{
>>>> +     struct kmx61_data *data;
>>>> +     struct iio_dev *indio_dev;
>>>> +     int ret;
>>>> +     const char *name = NULL;
>>>> +
>>>> +     indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>>>> +     if (!indio_dev)
>>>> +             return -ENOMEM;
>>>> +
>>>> +     data = iio_priv(indio_dev);
>>>> +     i2c_set_clientdata(client, indio_dev);
>>>> +     data->client = client;
>>>> +
>>>> +     if (id)
>>>> +             name = id->name;
>>>> +
>>>> +     indio_dev->dev.parent = &client->dev;
>>>> +     indio_dev->channels = kmx61_channels;
>>>> +     indio_dev->num_channels = ARRAY_SIZE(kmx61_channels);
>>>> +     indio_dev->name = name;
>>>> +     indio_dev->modes = INDIO_DIRECT_MODE;
>>>> +     indio_dev->info = &kmx61_info;
>>>> +
>>>> +     mutex_init(&data->lock);
>>>> +
>>>> +     ret = kmx61_chip_init(data);
>>>> +     if (ret < 0)
>>>> +             return ret;
>>>> +
>>>> +     ret = iio_device_register(indio_dev);
>>>> +     if (ret < 0) {
>>>> +             dev_err(&client->dev, "Failed to register iio device\n");
>>>> +             goto err_iio_device_register;
>>>> +     }
>>>> +
>>>> +     ret = pm_runtime_set_active(&client->dev);
>>>> +     if (ret < 0)
>>>> +             goto err_pm_runtime_set_active;
>>>> +
>>>> +     pm_runtime_enable(&client->dev);
>>>> +     pm_runtime_set_autosuspend_delay(&client->dev, KMX61_SLEEP_DELAY_MS);
>>>> +     pm_runtime_use_autosuspend(&client->dev);
>>>> +
>>>> +     return 0;
>>>> +
>>>> +err_pm_runtime_set_active:
>>>> +     iio_device_unregister(indio_dev);
>>>> +err_iio_device_register:
>>>> +     kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
>>>> +     return ret;
>>>> +}
>>>> +
>>>> +static int kmx61_remove(struct i2c_client *client)
>>>> +{
>>>> +     struct iio_dev *indio_dev = i2c_get_clientdata(client);
>>>> +     struct kmx61_data *data = iio_priv(indio_dev);
>>>> +     int ret;
>>>> +
>>>> +     pm_runtime_disable(&client->dev);
>>>> +     pm_runtime_set_suspended(&client->dev);
>>>> +     pm_runtime_put_noidle(&client->dev);
>>>> +
>>>> +     iio_device_unregister(indio_dev);
>>>> +
>>>> +     mutex_lock(&data->lock);
>>>> +     ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
>>>> +     mutex_unlock(&data->lock);
>>>> +
>>>> +     return ret;
>>>> +}
>>>> +
>>>> +#ifdef CONFIG_PM_SLEEP
>>>> +static int kmx61_suspend(struct device *dev)
>>>> +{
>>>> +     struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>>>> +     struct kmx61_data *data = iio_priv(indio_dev);
>>>> +     int ret;
>>>> +
>>>> +     mutex_lock(&data->lock);
>>>> +     ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG,
>>>> +                          false);
>>>> +     mutex_unlock(&data->lock);
>>>> +
>>>> +     return ret;
>>>> +}
>>>> +
>>>> +static int kmx61_resume(struct device *dev)
>>>> +{
>>>> +     struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>>>> +     struct kmx61_data *data = iio_priv(indio_dev);
>>>> +     u8 stby = 0;
>>>> +
>>>> +     if (data->acc_stby)
>>>> +             stby |= KMX61_ACC_STBY_BIT;
>>>> +     if (data->mag_stby)
>>>> +             stby |= KMX61_MAG_STBY_BIT;
>>>> +
>>>> +     return kmx61_set_mode(data, stby, KMX61_ACC | KMX61_MAG, true);
>>>> +}
>>>> +#endif
>>>> +
>>>> +#ifdef CONFIG_PM_RUNTIME
>>>> +static int kmx61_runtime_suspend(struct device *dev)
>>>> +{
>>>> +     struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>>>> +     struct kmx61_data *data = iio_priv(indio_dev);
>>>> +     int ret;
>>>> +
>>>> +     mutex_lock(&data->lock);
>>>> +     ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
>>>> +     mutex_unlock(&data->lock);
>>>> +
>>>> +     return ret;
>>>> +}
>>>> +
>>>> +static int kmx61_runtime_resume(struct device *dev)
>>>> +{
>>>> +     struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>>>> +     struct kmx61_data *data = iio_priv(indio_dev);
>>>> +     u8 stby = 0;
>>>> +
>>>> +     if (!data->acc_ps)
>>>> +             stby |= KMX61_ACC_STBY_BIT;
>>>> +     if (!data->mag_ps)
>>>> +             stby |= KMX61_MAG_STBY_BIT;
>>>> +
>>>> +     return kmx61_set_mode(data, stby, KMX61_ACC | KMX61_MAG, true);
>>>> +}
>>>> +#endif
>>>> +
>>>> +static const struct dev_pm_ops kmx61_pm_ops = {
>>>> +     SET_SYSTEM_SLEEP_PM_OPS(kmx61_suspend, kmx61_resume)
>>>> +     SET_RUNTIME_PM_OPS(kmx61_runtime_suspend, kmx61_runtime_resume, NULL)
>>>> +};
>>>> +
>>>> +static const struct i2c_device_id kmx61_id[] = {
>>>> +     {"kmx611021", 0},
>>>> +     {}
>>>> +};
>>>> +
>>>> +MODULE_DEVICE_TABLE(i2c, kmx61_id);
>>>> +
>>>> +static struct i2c_driver kmx61_driver = {
>>>> +     .driver = {
>>>> +             .name = KMX61_DRV_NAME,
>>>> +             .pm = &kmx61_pm_ops,
>>>> +     },
>>>> +     .probe          = kmx61_probe,
>>>> +     .remove         = kmx61_remove,
>>>> +     .id_table       = kmx61_id,
>>>> +};
>>>> +
>>>> +module_i2c_driver(kmx61_driver);
>>>> +
>>>> +MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com>");
>>>> +MODULE_DESCRIPTION("KMX61 accelerometer/magnetometer driver");
>>>> +MODULE_LICENSE("GPL v2");
>>
>> Hartmut, thanks a lot for your reviews. I will send a patch series
>> with these cleanups together with changes for ACPI, buffer and trigger
>> support.
> Don't know if Jonathan would like to see it smashed together, already.
> Take care

By together I mean separate patches in the same patch set.

I will take
Daniel.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3] iio: imu: Add support for Kionix KMX61 sensor
  2014-11-25 22:44       ` Daniel Baluta
@ 2014-11-25 23:03         ` Daniel Baluta
  0 siblings, 0 replies; 7+ messages in thread
From: Daniel Baluta @ 2014-11-25 23:03 UTC (permalink / raw)
  To: Daniel Baluta
  Cc: Hartmut Knaack, Jonathan Cameron, Peter Meerwald,
	Srinivas Pandruvada, linux-iio, Linux Kernel Mailing List

On Wed, Nov 26, 2014 at 12:44 AM, Daniel Baluta <daniel.baluta@intel.com> wrote:
> On Wed, Nov 26, 2014 at 12:31 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
>> Daniel Baluta schrieb am 25.11.2014 10:54:
>>> On Tue, Nov 25, 2014 at 2:45 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
>>>> Daniel Baluta schrieb am 18.11.2014 17:47:
>>>>> Minimal implementation for KMX61 6-axis accelerometer/magnetometer. It exports
>>>>> raw accel/magn readings together with scale and sampling frequency.
>>>>>
>>>>> Datasheet will be available at:
>>>>> http://www.kionix.com/6-axis-accelerometer-magnetometer/kmx61
>>>>>
>>>> Finally managed to have a close look, and found a few issues.
>>>>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>>>>> ---
>>>>> Changes since v2:
>>>>>       * mostly style fixes after comments received from Jonathan and Peter
>>>>>               * http://marc.info/?l=linux-iio&m=141564019602400&w=2
>>>>>       * implemented PM runtime and PM sleep resume/suspend hooks
>>>>>       * introduced new parameter for kmx61_set_mode to help updating
>>>>>       stby bits in kmx61_data.
>>>>>       * use kmx6111021 instead of kmx61 for i2c_device_id.
>>>>>
>>>>>  drivers/iio/imu/Kconfig  |   9 +
>>>>>  drivers/iio/imu/Makefile |   2 +
>>>>>  drivers/iio/imu/kmx61.c  | 766 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  3 files changed, 777 insertions(+)
>>>>>  create mode 100644 drivers/iio/imu/kmx61.c
>>>>>
>>>>> diff --git a/drivers/iio/imu/Kconfig b/drivers/iio/imu/Kconfig
>>>>> index 2b0e451..d675f43 100644
>>>>> --- a/drivers/iio/imu/Kconfig
>>>>> +++ b/drivers/iio/imu/Kconfig
>>>>> @@ -25,6 +25,15 @@ config ADIS16480
>>>>>         Say yes here to build support for Analog Devices ADIS16375, ADIS16480,
>>>>>         ADIS16485, ADIS16488 inertial sensors.
>>>>>
>>>>> +config KMX61
>>>>> +     tristate "Kionix KMX61 6-axis accelerometer and magnetometer"
>>>>> +     depends on I2C
>>>>> +     help
>>>>> +       Say Y here if you want to build a driver for Kionix KMX61 6-axis accelerometer
>>>>> +       and magnetometer.
>>>>> +       To compile this driver as module, choose M here: the module will be called
>>>>> +       kmx61.
>>>>> +
>>>> This help text is more than 80 characters wide and may better be wrapped a bit differently.
>>>
>>> Good catch. checkpatch.pl didn't complain.
>>>
>>>>>  source "drivers/iio/imu/inv_mpu6050/Kconfig"
>>>>>
>>>>>  endmenu
>>>>> diff --git a/drivers/iio/imu/Makefile b/drivers/iio/imu/Makefile
>>>>> index 114d2c1..e1e6e3d 100644
>>>>> --- a/drivers/iio/imu/Makefile
>>>>> +++ b/drivers/iio/imu/Makefile
>>>>> @@ -14,3 +14,5 @@ adis_lib-$(CONFIG_IIO_ADIS_LIB_BUFFER) += adis_buffer.o
>>>>>  obj-$(CONFIG_IIO_ADIS_LIB) += adis_lib.o
>>>>>
>>>>>  obj-y += inv_mpu6050/
>>>>> +
>>>>> +obj-$(CONFIG_KMX61) += kmx61.o
>>>>> diff --git a/drivers/iio/imu/kmx61.c b/drivers/iio/imu/kmx61.c
>>>>> new file mode 100644
>>>>> index 0000000..f68b3ef
>>>>> --- /dev/null
>>>>> +++ b/drivers/iio/imu/kmx61.c
>>>>> @@ -0,0 +1,766 @@
>>>>> +/*
>>>>> + * KMX61 - Kionix 6-axis Accelerometer/Magnetometer
>>>>> + *
>>>>> + * Copyright (c) 2014, 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.
>>>>> + *
>>>>> + * IIO driver for KMX61 (7-bit I2C slave address 0x0E or 0x0F).
>>>>> + *
>>>>> + * TODO: buffer, interrupt, thresholds, acpi, temperature sensor
>>>>> + *
>>>>> + */
>>>>> +
>>>>> +#include <linux/module.h>
>>>>> +#include <linux/i2c.h>
>>>>> +#include <linux/pm.h>
>>>>> +#include <linux/pm_runtime.h>
>>>>> +#include <linux/iio/iio.h>
>>>>> +#include <linux/iio/sysfs.h>
>>>>> +
>>>>> +#define KMX61_DRV_NAME "kmx61"
>>>>> +
>>>>> +#define KMX61_REG_WHO_AM_I   0x00
>>>>> +
>>>>> +/*
>>>>> + * three 16-bit accelerometer output registers for X/Y/Z axis
>>>>> + * we use only XOUT_L as a base register, all other addresses
>>>>> + * can be obtained by applying an offset and are provided here
>>>>> + * only for clarity.
>>>>> + */
>>>>> +#define KMX61_ACC_XOUT_L     0x0A
>>>>> +#define KMX61_ACC_XOUT_H     0x0B
>>>>> +#define KMX61_ACC_YOUT_L     0x0C
>>>>> +#define KMX61_ACC_YOUT_H     0x0D
>>>>> +#define KMX61_ACC_ZOUT_L     0x0E
>>>>> +#define KMX61_ACC_ZOUT_H     0x0F
>>>>> +
>>>>> +/*
>>>>> + * one 16-bit temperature output register
>>>>> + */
>>>>> +#define KMX61_TEMP_L         0x10
>>>>> +#define KMX61_TEMP_H         0x11
>>>>> +
>>>>> +/*
>>>>> + * three 16-bit magnetometer output registers for X/Y/Z axis
>>>>> + */
>>>>> +#define KMX61_MAG_XOUT_L     0x12
>>>>> +#define KMX61_MAG_XOUT_H     0x13
>>>>> +#define KMX61_MAG_YOUT_L     0x14
>>>>> +#define KMX61_MAG_YOUT_H     0x15
>>>>> +#define KMX61_MAG_ZOUT_L     0x16
>>>>> +#define KMX61_MAG_ZOUT_H     0x17
>>>>> +
>>>>> +#define KMX61_REG_ODCNTL     0x2C
>>>>> +#define KMX61_REG_STBY               0x29
>>>>> +#define KMX61_REG_CTRL1              0x2A
>>>>> +
>>>>> +#define KMX61_ACC_STBY_BIT   BIT(0)
>>>>> +#define KMX61_MAG_STBY_BIT   BIT(1)
>>>>> +#define KMX61_ACT_STBY_BIT   BIT(7)
>>>>> +
>>>>> +#define KMX61_ALL_STBY               (KMX61_ACC_STBY_BIT | KMX61_MAG_STBY_BIT)
>>>>> +
>>>>> +#define KMX61_REG_CTRL1_GSEL0_SHIFT  0
>>>>> +#define KMX61_REG_CTRL1_GSEL1_SHIFT  1
>>>>> +#define KMX61_REG_CTRL1_GSEL0_MASK   0x01
>>>>> +#define KMX61_REG_CTRL1_GSEL1_MASK   0x02
>>>> I don't see the benefit of separating GSEL0 and GSEL1.
>>>>> +
>>>>> +#define KMX61_REG_CTRL1_BIT_RES              BIT(4)
>>>>> +
>>>>> +#define KMX61_ACC_ODR_SHIFT  0
>>>>> +#define KMX61_MAG_ODR_SHIFT  4
>>>>> +#define KMX61_ACC_ODR_MASK   0x0F
>>>>> +#define KMX61_MAG_ODR_MASK   0xF0
>>>>> +
>>>>> +#define KMX61_SLEEP_DELAY_MS 2000
>>>>> +
>>>>> +#define KMX61_CHIP_ID                0x12
>>>>> +
>>>>> +struct kmx61_data {
>>>>> +     struct i2c_client *client;
>>>>> +
>>>>> +     /* serialize access to non-atomic ops, e.g set_mode */
>>>>> +     struct mutex lock;
>>>>> +     u8 range;
>>>>> +     u8 odr_bits;
>>>>> +
>>>>> +     /* standby state */
>>>>> +     u8 acc_stby;
>>>>> +     u8 mag_stby;
>>>> Why u8 instead of bool?
>>>
>>> Hmm, correct. I was thinking bitwise, but you are right. Will fix.
>>>
>>>>> +
>>>>> +     /* power state */
>>>>> +     bool acc_ps;
>>>>> +     bool mag_ps;
>>>>> +};
>>>>> +
>>>>> +enum kmx61_range {
>>>>> +     KMX61_RANGE_2G,
>>>>> +     KMX61_RANGE_4G,
>>>>> +     KMX61_RANGE_8G,
>>>>> +};
>>>>> +
>>>>> +enum kmx61_scan {
>>>>> +     KMX61_SCAN_ACC_X,
>>>>> +     KMX61_SCAN_ACC_Y,
>>>>> +     KMX61_SCAN_ACC_Z,
>>>>> +     KMX61_SCAN_TEMP,
>>>>> +     KMX61_SCAN_MAG_X,
>>>>> +     KMX61_SCAN_MAG_Y,
>>>>> +     KMX61_SCAN_MAG_Z,
>>>>> +};
>>>>> +
>>>>> +static const struct {
>>>>> +     u16 uscale;
>>>>> +     u8 gsel0;
>>>>> +     u8 gsel1;
>>>>> +} kmx61_scale_table[] = {
>>>>> +     {9582, 0, 0},
>>>>> +     {19163, 1, 0},
>>>>> +     {38326, 0, 1},
>>>> Why not consolidate gsel0 and gsel1 to gsel? Then this could become u16 kmx61_scale[] = {9582, 19163, 38326} with gsel as index.
>>>
>>> True. Will fix.
>>>
>>>>> +};
>>>>> +
>>>>> +/* KMX61 devices */
>>>>> +#define KMX61_ACC    0x01
>>>>> +#define KMX61_MAG    0x02
>>>>> +
>>>>> +static const struct {
>>>>> +     int val;
>>>>> +     int val2;
>>>>> +     u8 odr_bits;
>>>>> +} kmx61_samp_freq_table[] = { {12, 500000, 0x00},
>>>>> +                     {25, 0, 0x01},
>>>>> +                     {50, 0, 0x02},
>>>>> +                     {100, 0, 0x03},
>>>>> +                     {200, 0, 0x04},
>>>>> +                     {400, 0, 0x05},
>>>>> +                     {800, 0, 0x06},
>>>>> +                     {1600, 0, 0x07},
>>>>> +                     {0, 781000, 0x08},
>>>>> +                     {1, 563000, 0x09},
>>>>> +                     {3, 125000, 0x0A},
>>>>> +                     {6, 250000, 0x0B} };
>>>>> +
>>>>> +static IIO_CONST_ATTR(accel_scale_available, "0.009582 0.019163 0.038326");
>>>>> +static IIO_CONST_ATTR(magn_scale_available, "0.001465");
>>>>> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
>>>>> +     "0.781000 1.563000 3.125000 6.250000 12.500000 25 50 100 200 400 800");
>>>>> +
>>>>> +static struct attribute *kmx61_attributes[] = {
>>>>> +     &iio_const_attr_accel_scale_available.dev_attr.attr,
>>>>> +     &iio_const_attr_magn_scale_available.dev_attr.attr,
>>>>> +     &iio_const_attr_sampling_frequency_available.dev_attr.attr,
>>>>> +     NULL,
>>>>> +};
>>>>> +
>>>>> +static const struct attribute_group kmx61_attribute_group = {
>>>>> +     .attrs = kmx61_attributes,
>>>>> +};
>>>>> +
>>>>> +#define KMX61_ACC_CHAN(_axis, _index) { \
>>>>> +     .type = IIO_ACCEL, \
>>>>> +     .modified = 1, \
>>>>> +     .channel2 = IIO_MOD_ ## _axis, \
>>>>> +     .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>>>>> +     .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
>>>>> +                             BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>>>>> +     .address = KMX61_ACC, \
>>>>> +     .scan_index = _index, \
>>>>> +     .scan_type = { \
>>>>> +             .sign = 's', \
>>>>> +             .realbits = 12, \
>>>>> +             .storagebits = 16, \
>>>>> +             .shift = 4, \
>>>>> +             .endianness = IIO_LE, \
>>>>> +     }, \
>>>>> +}
>>>>> +
>>>>> +#define KMX61_MAG_CHAN(_axis, _index) { \
>>>>> +     .type = IIO_MAGN, \
>>>>> +     .modified = 1, \
>>>>> +     .channel2 = IIO_MOD_ ## _axis, \
>>>>> +     .address = KMX61_MAG, \
>>>>> +     .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>>>>> +     .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
>>>>> +                             BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>>>>> +     .scan_index = _index, \
>>>>> +     .scan_type = { \
>>>>> +             .sign = 's', \
>>>>> +             .realbits = 14, \
>>>>> +             .storagebits = 16, \
>>>>> +             .shift = 2, \
>>>>> +             .endianness = IIO_LE, \
>>>>> +     }, \
>>>>> +}
>>>>> +
>>>>> +static const struct iio_chan_spec kmx61_channels[] = {
>>>>> +     KMX61_ACC_CHAN(X, KMX61_SCAN_ACC_X),
>>>>> +     KMX61_ACC_CHAN(Y, KMX61_SCAN_ACC_Y),
>>>>> +     KMX61_ACC_CHAN(Z, KMX61_SCAN_ACC_Z),
>>>>> +     KMX61_MAG_CHAN(X, KMX61_SCAN_MAG_X),
>>>>> +     KMX61_MAG_CHAN(Y, KMX61_SCAN_MAG_Y),
>>>>> +     KMX61_MAG_CHAN(Z, KMX61_SCAN_MAG_Z),
>>>>> +};
>>>>> +
>>>>> +static int kmx61_convert_freq_to_bit(int val, int val2)
>>>>> +{
>>>>> +     int i;
>>>>> +
>>>>> +     for (i = 0; i < ARRAY_SIZE(kmx61_samp_freq_table); i++)
>>>>> +             if (val == kmx61_samp_freq_table[i].val &&
>>>>> +                 val2 == kmx61_samp_freq_table[i].val2)
>>>>> +                     return kmx61_samp_freq_table[i].odr_bits;
>>>>> +     return -EINVAL;
>>>>> +}
>>>>> +/**
>>>> Like here, the first line of the following multiline comments have one asterisk too much.
>>>
>>> I think this is the convention, when adding doxygen style documentation:
>>> http://lxr.free-electrons.com/source/fs/eventpoll.c#L382
>>>
>>>>> + * kmx61_set_mode() - set KMX61 device operating mode
>>>>> + * @data - kmx61 device private data pointer
>>>>> + * @mode - bitmask, indicating operating mode for @device
>>>>> + * @device - bitmask, indicating device for which @mode needs to be set
>>>>> + * @update - update stby bits stored in device's private  @data
>>>>> + *
>>>>> + * For each sensor (accelerometer/magnetometer) there are two operating modes
>>>>> + * STANDBY and OPERATION. Neither accel nor magn can be disabled independently
>>>>> + * if they are both enabled. Internal sensors state is saved in acc_stby and
>>>>> + * mag_stby members of driver's private @data.
>>>>> + */
>>>>> +static int kmx61_set_mode(struct kmx61_data *data, u8 mode, u8 device,
>>>>> +                       bool update)
>>>>> +{
>>>>> +     int ret;
>>>>> +     int acc_stby = -1, mag_stby = -1;
>>>>> +
>>>>> +     ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_STBY);
>>>>> +     if (ret < 0) {
>>>>> +             dev_err(&data->client->dev, "Error reading reg_stby\n");
>>>>> +             return ret;
>>>>> +     }
>>>>> +     if (device & KMX61_ACC) {
>>>>> +             if (mode & KMX61_ACC_STBY_BIT) {
>>>>> +                     ret |= KMX61_ACC_STBY_BIT;
>>>>> +                     acc_stby = 1;
>>>>> +             } else {
>>>>> +                     ret &= ~KMX61_ACC_STBY_BIT;
>>>>> +                     acc_stby = 0;
>>>>> +             }
>>>>> +     }
>>>>> +
>>>>> +     if (device & KMX61_MAG) {
>>>>> +             if (mode & KMX61_MAG_STBY_BIT) {
>>>>> +                     ret |= KMX61_MAG_STBY_BIT;
>>>>> +                     mag_stby = 1;
>>>>> +             } else {
>>>>> +                     ret &= ~KMX61_MAG_STBY_BIT;
>>>>> +                     mag_stby = 0;
>>>>> +             }
>>>>> +     }
>>>>> +
>>>>> +     ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_STBY, ret);
>>>>> +     if (ret < 0) {
>>>>> +             dev_err(&data->client->dev, "Error writing reg_stby\n");
>>>>> +             return ret;
>>>>> +     }
>>>>> +
>>>>> +     if (acc_stby != -1 && update)
>>>>> +             data->acc_stby = !!acc_stby;
>>>>> +     if (mag_stby != -1 && update)
>>>>> +             data->mag_stby = !!mag_stby;
>>>> Why is there a need for double negation?
>>>
>>> Correct. Now it doesn't make sense since data->acc_stby is u8.
>>>
>>> But, with your suggestion if I will use bool to represent data->acc_stby
>>> then I think the code should use double negation to make it clear
>>> that data->acc_stby is bool and acc_stby is int.
>>>
>> Well, if it makes you happy, then go ahead. My interpretation of a bool is, that an assignment of 0 becomes 0/false, and an assignment of anything else becomes 1/true. So, I wouldn't see a need here (especially since acc_stby/mag_stby were distinctly assigned either 0 or 1).
>
> All right. It makes sense.
>
>>>>> +
>>>>> +     return ret;
>>>> Since no error occured until this point, just return 0.
>>>
>>> Ok. Will fix.
>>>
>>>>> +}
>>>>> +
>>>>> +static int kmx61_get_mode(struct kmx61_data *data, u8 *mode, u8 device)
>>>>> +{
>>>>> +     int ret;
>>>>> +
>>>>> +     ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_STBY);
>>>>> +     if (ret < 0) {
>>>>> +             dev_err(&data->client->dev, "Error reading reg_stby\n");
>>>>> +             return ret;
>>>>> +     }
>>>>> +     *mode = 0;
>>>>> +
>>>>> +     if (device & KMX61_ACC) {
>>>>> +             if (ret & KMX61_ACC_STBY_BIT)
>>>>> +                     *mode |= KMX61_ACC_STBY_BIT;
>>>>> +             else
>>>>> +                     *mode &= ~KMX61_ACC_STBY_BIT;
>>>> Simply:
>>>>                 *mode |= ret & KMX61_ACC_STBY_BIT;
>>> Ok.
>>>
>>>>> +     }
>>>>> +
>>>>> +     if (device & KMX61_MAG) {
>>>>> +             if (ret & KMX61_MAG_STBY_BIT)
>>>>> +                     *mode |= KMX61_MAG_STBY_BIT;
>>>>> +             else
>>>>> +                     *mode &= ~KMX61_MAG_STBY_BIT;
>>>> Same here:
>>> Ok.
>>>
>>>>                 *mode |= ret & KMX61_MAG_STBY_BIT;
>>>>> +     }
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static int kmx61_set_odr(struct kmx61_data *data, int val, int val2, u8 device)
>>>>> +{
>>>>> +     int ret;
>>>>> +     u8 mode;
>>>>> +     int lodr_bits, odr_bits;
>>>>> +
>>>>> +     ret = kmx61_get_mode(data, &mode, KMX61_ACC | KMX61_MAG);
>>>>> +     if (ret < 0)
>>>>> +             return ret;
>>>>> +
>>>>> +     lodr_bits = kmx61_convert_freq_to_bit(val, val2);
>>>>> +     if (lodr_bits < 0)
>>>>> +             return lodr_bits;
>>>>> +
>>>>> +     /* To change ODR, accel and magn must be in STDBY */
>>>>> +     ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG,
>>>>> +                          true);
>>>>> +     if (ret < 0)
>>>>> +             return ret;
>>>>> +
>>>>> +     odr_bits = 0;
>>>>> +     if (device & KMX61_ACC)
>>>>> +             odr_bits |= lodr_bits;
>>>>> +     if (device & KMX61_MAG)
>>>>> +             odr_bits |= (lodr_bits << KMX61_MAG_ODR_SHIFT);
>>>> No need for parenthesis.
>>> ok.
>>>
>>>>> +
>>>>> +     ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_ODCNTL,
>>>>> +                                     odr_bits);
>>>>> +     if (ret < 0)
>>>>> +             return ret;
>>>>> +
>>>>> +     ret = kmx61_set_mode(data, mode, KMX61_ACC | KMX61_MAG, true);
>>>>> +     if (ret < 0)
>>>>> +             return ret;
>>>>> +
>>>>> +     data->odr_bits = lodr_bits;
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static
>>>>> +int kmx61_get_odr(struct kmx61_data *data, int *val, int *val2, u8 device)
>>>> Usually wrap parameters? And don't put code on line with {.
>>>
>>> Will fix.
>>>
>>>>> +{    int i;
>>>>> +     u8 lodr_bits;
>>>>> +
>>>>> +     if (device & KMX61_ACC)
>>>>> +             lodr_bits = (data->odr_bits >> KMX61_ACC_ODR_SHIFT) &
>>>>> +                          KMX61_ACC_ODR_MASK;
>>>> KMX61_ACC_ODR_SHIFT is just a placebo and not used in kmx61_set_odr. Choose one way and stay consistent.
>>>
>>> placebo is good.  will fix kmx61_set_odr :)
>>>
>>>>> +     else if (device & KMX61_MAG)
>>>>> +             lodr_bits = (data->odr_bits >> KMX61_MAG_ODR_SHIFT) &
>>>>> +                          KMX61_MAG_ODR_MASK;
>>>>> +     else
>>>>> +             return -EINVAL;
>>>>> +
>>>>> +     for (i = 0; i < ARRAY_SIZE(kmx61_samp_freq_table); i++)
>>>>> +             if (lodr_bits == kmx61_samp_freq_table[i].odr_bits) {
>>>>> +                     *val = kmx61_samp_freq_table[i].val;
>>>>> +                     *val2 = kmx61_samp_freq_table[i].val2;
>>>>> +                     return 0;
>>>> return IIO_VAL_INT_PLUS_MICRO allows you to faciliate _read_raw a bit to just return ret there.
>>>
>>> Hmm, If you don't mind I want to keep this as it is, just to
>>> be consistent with the rest of the functions. The convention
>>> used is to return 0 in case of success.
>>>
>> Up to you. Though kmx61_read_raw() would be an example of a function that doesn't follow this convention ;-) And there is no real benefit of checking an error code there, which is passed up anyway.
>>>>> +             }
>>>>> +     return -EINVAL;
>>>>> +}
>>>>> +
>>>>> +static int kmx61_set_range(struct kmx61_data *data, int range)
>>>>> +{
>>>>> +     int ret;
>>>>> +
>>>>> +     ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_CTRL1);
>>>>> +     if (ret < 0) {
>>>>> +             dev_err(&data->client->dev, "Error reading reg_ctrl1\n");
>>>>> +             return ret;
>>>>> +     }
>>>>> +
>>>>> +     ret &= ~(KMX61_REG_CTRL1_GSEL0_MASK | KMX61_REG_CTRL1_GSEL1_MASK);
>>>>> +     ret |= kmx61_scale_table[range].gsel0 << KMX61_REG_CTRL1_GSEL0_SHIFT;
>>>>> +     ret |= kmx61_scale_table[range].gsel1 << KMX61_REG_CTRL1_GSEL1_SHIFT;
>>>>> +
>>>>> +     ret = i2c_smbus_write_byte_data(data->client, KMX61_REG_CTRL1, ret);
>>>>> +     if (ret < 0) {
>>>>> +             dev_err(&data->client->dev, "Error writing reg_ctrl1\n");
>>>>> +             return ret;
>>>>> +     }
>>>>> +
>>>>> +     data->range = range;
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static int kmx61_set_scale(struct kmx61_data *data, int uscale)
>>>>> +{
>>>>> +     int ret, i;
>>>>> +     u8  mode;
>>>>> +
>>>>> +     for (i = 0; i < ARRAY_SIZE(kmx61_scale_table); i++) {
>>>>> +             if (kmx61_scale_table[i].uscale == uscale) {
>>>>> +                     ret = kmx61_get_mode(data, &mode,
>>>>> +                                          KMX61_ACC | KMX61_MAG);
>>>>> +                     if (ret < 0)
>>>>> +                             return ret;
>>>>> +
>>>>> +                     ret = kmx61_set_mode(data, KMX61_ALL_STBY,
>>>>> +                                          KMX61_ACC | KMX61_MAG, true);
>>>>> +                     if (ret < 0)
>>>>> +                             return ret;
>>>>> +
>>>>> +                     ret = kmx61_set_range(data, i);
>>>>> +                     if (ret < 0)
>>>>> +                             return ret;
>>>>> +
>>>>> +                     return  kmx61_set_mode(data, mode,
>>>>> +                                            KMX61_ACC | KMX61_MAG, true);
>>>>> +             }
>>>>> +     }
>>>>> +     return -EINVAL;
>>>>> +}
>>>>> +
>>>>> +static int kmx61_chip_init(struct kmx61_data *data)
>>>>> +{
>>>>> +     int ret;
>>>>> +
>>>>> +     ret = i2c_smbus_read_byte_data(data->client, KMX61_REG_WHO_AM_I);
>>>>> +     if (ret < 0) {
>>>>> +             dev_err(&data->client->dev, "Error reading who_am_i\n");
>>>>> +             return ret;
>>>>> +     }
>>>>> +
>>>>> +     if (ret != KMX61_CHIP_ID) {
>>>>> +             dev_err(&data->client->dev,
>>>>> +                     "Wrong chip id, got %x expected %x\n",
>>>>> +                      ret, KMX61_CHIP_ID);
>>>>> +             return -EINVAL;
>>>>> +     }
>>>>> +
>>>>> +     /* set accel 12bit, 4g range */
>>>>> +     ret = kmx61_set_range(data, KMX61_RANGE_4G);
>>>>> +     if (ret < 0)
>>>>> +             return ret;
>>>>> +
>>>>> +     /* set acc/magn to OPERATION mode */
>>>>> +     ret = kmx61_set_mode(data, 0, KMX61_ACC | KMX61_MAG, true);
>>>> Just return kmx61_set_mode(data, 0, KMX61_ACC | KMX61_MAG, true);
>>>
>>> Ok.
>>>
>>>>> +     if (ret < 0)
>>>>> +             return ret;
>>>>> +
>>>>> +     return 0;
>>>>> +}
>>>>> +/**
>>>>> + * kmx61_set_power_state() - set power state for kmx61 @device
>>>>> + * @data - kmx61 device private pointer
>>>>> + * @on - power state to be set for @device
>>>>> + * @device - bitmask indicating device for which @on state needs to be set
>>>>> + *
>>>>> + * Notice that when ACC power state needs to be set to ON and MAG is in
>>>>> + * OPERATION then we know that kmx61_runtime_resume was already called
>>>>> + * so we must set ACC OPERATION mode here. The same happens when MAG power
>>>>> + * state needs to be set to ON and ACC is in OPERATION.
>>>>> + */
>>>>> +static int kmx61_set_power_state(struct kmx61_data *data, bool on, u8 device)
>>>>> +{
>>>>> +#ifdef CONFIG_PM_RUNTIME
>>>>> +     int ret;
>>>>> +
>>>>> +     if (device & KMX61_ACC) {
>>>>> +             if (on && !data->acc_ps && !data->mag_stby)
>>>>> +                     kmx61_set_mode(data, 0, KMX61_ACC, true);
>>>>> +             data->acc_ps = on;
>>>>> +     }
>>>>> +     if (device & KMX61_MAG) {
>>>>> +             if (on && !data->mag_ps && !data->acc_stby)
>>>>> +                     kmx61_set_mode(data, 0, KMX61_MAG, true);
>>>>> +             data->mag_ps = on;
>>>>> +     }
>>>>> +
>>>>> +     if (on) {
>>>>> +             ret = pm_runtime_get_sync(&data->client->dev);
>>>>> +     } else {
>>>>> +             pm_runtime_mark_last_busy(&data->client->dev);
>>>>> +             ret = pm_runtime_put_autosuspend(&data->client->dev);
>>>>> +     }
>>>>> +     if (ret < 0) {
>>>>> +             dev_err(&data->client->dev,
>>>>> +                     "Failed: kmx61_set_power_state for %d, ret %d\n",
>>>>> +                     on, ret);
>>>>> +             return ret;
>>>>> +     }
>>>>> +#endif
>>>>> +     return 0;
>>>>> +}
>>>>> +
>>>>> +static int kmx61_read_measurement(struct kmx61_data *data, int base, int offset)
>>>> u8 base, u8 offset?
>>>
>>> Correct.
>>>
>>>>> +{
>>>>> +     int ret;
>>>>> +     u8 reg = base + offset * 2;
>>>>> +
>>>>> +     ret = i2c_smbus_read_word_data(data->client, reg);
>>>>> +     if (ret < 0) {
>>>>> +             dev_err(&data->client->dev, "failed to read reg at %x\n", reg);
>>>>> +             return ret;
>>>> This return is unnecessary.
>>>
>>> Ok.
>>>
>>>>> +     }
>>>>> +
>>>>> +     return ret;
>>>>> +}
>>>>> +
>>>>> +static int kmx61_read_raw(struct iio_dev *indio_dev,
>>>>> +                           struct iio_chan_spec const *chan, int *val,
>>>>> +                           int *val2, long mask)
>>>> Indentation of the parameters is a bit too high.
>>>
>>> Ok, will fix.
>>>
>>>>> +{
>>>>> +     struct kmx61_data *data = iio_priv(indio_dev);
>>>>> +     int ret;
>>>>> +     u8 base_reg;
>>>>> +
>>>>> +     switch (mask) {
>>>>> +     case IIO_CHAN_INFO_RAW:
>>>>> +             switch (chan->type) {
>>>>> +             case IIO_ACCEL:
>>>>> +             case IIO_MAGN:
>>>>> +                     base_reg = KMX61_ACC_XOUT_L;
>>>>> +                     break;
>>>>> +             default:
>>>>> +                     return -EINVAL;
>>>>> +             }
>>>>> +             mutex_lock(&data->lock);
>>>>> +
>>>>> +             kmx61_set_power_state(data, true, chan->address);
>>>>> +             ret = kmx61_read_measurement(data, base_reg, chan->scan_index);
>>>>> +             if (ret < 0) {
>>>>> +                     kmx61_set_power_state(data, false, chan->address);
>>>>> +                     mutex_unlock(&data->lock);
>>>>> +                     return ret;
>>>>> +             }
>>>>> +             *val = sign_extend32(ret >> chan->scan_type.shift,
>>>>> +                                  chan->scan_type.realbits - 1);
>>>>> +             kmx61_set_power_state(data, false, chan->address);
>>>>> +
>>>>> +             mutex_unlock(&data->lock);
>>>>> +             return IIO_VAL_INT;
>>>>> +     case IIO_CHAN_INFO_SCALE:
>>>>> +             switch (chan->type) {
>>>>> +             case IIO_ACCEL:
>>>>> +                     *val = 0;
>>>>> +                     *val2 = kmx61_scale_table[data->range].uscale;
>>>>> +                     return IIO_VAL_INT_PLUS_MICRO;
>>>>> +             case IIO_MAGN:
>>>>> +                     /* 14 bits res, 1465 microGauss per magn count */
>>>>> +                     *val = 0;
>>>>> +                     *val2 = 1465;
>>>>> +                     return IIO_VAL_INT_PLUS_MICRO;
>>>>> +             default:
>>>>> +                     return -EINVAL;
>>>>> +             }
>>>>> +     case IIO_CHAN_INFO_SAMP_FREQ:
>>>>> +             if (chan->type != IIO_ACCEL && chan->type != IIO_MAGN)
>>>>> +                     return -EINVAL;
>>>>> +
>>>>> +             mutex_lock(&data->lock);
>>>>> +             ret = kmx61_get_odr(data, val, val2, chan->address);
>>>>> +             mutex_unlock(&data->lock);
>>>> Could be just return ret here, if kmx61_get_odr() returns IIO_VAL_INT_PLUS_MICRO on success and -EINVAL on error.
>>>
>>> As explained above, I prefer not to do this. More than that, I can't do ret here
>>> because the data->lock is held.
>>>
>> Well, I actually intended to have an unconditional "return ret;" right after the mutex_unlock. But up to you. I also see a good point to make it obvious here, that the return value is IIO_VAL_INT_PLUS_MICRO. So, up to you.
>>>>> +             if (ret)
>>>>> +                     return -EINVAL;
>>>>> +             return IIO_VAL_INT_PLUS_MICRO;
>>>>> +     }
>>>>> +     return -EINVAL;
>>>>> +}
>>>>> +
>>>>> +static int kmx61_write_raw(struct iio_dev *indio_dev,
>>>>> +                            struct iio_chan_spec const *chan, int val,
>>>>> +                            int val2, long mask)
>>>> Indentation of parameters is too high.
>>> Ok.
>>>>> +{
>>>>> +     struct kmx61_data *data = iio_priv(indio_dev);
>>>>> +     int ret;
>>>>> +
>>>>> +     switch (mask) {
>>>>> +     case IIO_CHAN_INFO_SAMP_FREQ:
>>>>> +             if (chan->type != IIO_ACCEL && chan->type != IIO_MAGN)
>>>>> +                     return -EINVAL;
>>>>> +
>>>>> +             mutex_lock(&data->lock);
>>>>> +             ret = kmx61_set_odr(data, val, val2, chan->address);
>>>>> +             mutex_unlock(&data->lock);
>>>>> +             return ret;
>>>>> +     case IIO_CHAN_INFO_SCALE:
>>>>> +             switch (chan->type) {
>>>>> +             case IIO_ACCEL:
>>>>> +                     if (val != 0)
>>>>> +                             return -EINVAL;
>>>>> +                     mutex_lock(&data->lock);
>>>>> +                     ret = kmx61_set_scale(data, val2);
>>>>> +                     mutex_unlock(&data->lock);
>>>>> +                     return ret;
>>>>> +             default:
>>>>> +                     return -EINVAL;
>>>>> +             }
>>>>> +             return ret;
>>>> This won't be reached (if so, ret would not be initialized).
>>>>> +     default:
>>>>> +             return -EINVAL;
>>>>> +     }
>>>>> +     return ret;
>>>> Same here.
>>>
>>> I know. Would initializing ret with 0 will be enough?
>> Since the switch already catches all cases and returns, there is no need for return ret, here. But if you prefer to have a return at the end of the function, you could drop the default case, or leave it empty, or just put a break in there - and return -EINVAL at the end.
>
> Ok. I will remove the the unnecessary return.
>
>>>
>>>>> +}
>>>>> +
>>>>> +static const struct iio_info kmx61_info = {
>>>>> +     .driver_module          = THIS_MODULE,
>>>>> +     .read_raw               = kmx61_read_raw,
>>>>> +     .write_raw              = kmx61_write_raw,
>>>>> +     .attrs                  = &kmx61_attribute_group,
>>>>> +};
>>>>> +
>>>>> +static int kmx61_probe(struct i2c_client *client,
>>>>> +                    const struct i2c_device_id *id)
>>>>> +{
>>>>> +     struct kmx61_data *data;
>>>>> +     struct iio_dev *indio_dev;
>>>>> +     int ret;
>>>>> +     const char *name = NULL;
>>>>> +
>>>>> +     indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>>>>> +     if (!indio_dev)
>>>>> +             return -ENOMEM;
>>>>> +
>>>>> +     data = iio_priv(indio_dev);
>>>>> +     i2c_set_clientdata(client, indio_dev);
>>>>> +     data->client = client;
>>>>> +
>>>>> +     if (id)
>>>>> +             name = id->name;
>>>>> +
>>>>> +     indio_dev->dev.parent = &client->dev;
>>>>> +     indio_dev->channels = kmx61_channels;
>>>>> +     indio_dev->num_channels = ARRAY_SIZE(kmx61_channels);
>>>>> +     indio_dev->name = name;
>>>>> +     indio_dev->modes = INDIO_DIRECT_MODE;
>>>>> +     indio_dev->info = &kmx61_info;
>>>>> +
>>>>> +     mutex_init(&data->lock);
>>>>> +
>>>>> +     ret = kmx61_chip_init(data);
>>>>> +     if (ret < 0)
>>>>> +             return ret;
>>>>> +
>>>>> +     ret = iio_device_register(indio_dev);
>>>>> +     if (ret < 0) {
>>>>> +             dev_err(&client->dev, "Failed to register iio device\n");
>>>>> +             goto err_iio_device_register;
>>>>> +     }
>>>>> +
>>>>> +     ret = pm_runtime_set_active(&client->dev);
>>>>> +     if (ret < 0)
>>>>> +             goto err_pm_runtime_set_active;
>>>>> +
>>>>> +     pm_runtime_enable(&client->dev);
>>>>> +     pm_runtime_set_autosuspend_delay(&client->dev, KMX61_SLEEP_DELAY_MS);
>>>>> +     pm_runtime_use_autosuspend(&client->dev);
>>>>> +
>>>>> +     return 0;
>>>>> +
>>>>> +err_pm_runtime_set_active:
>>>>> +     iio_device_unregister(indio_dev);
>>>>> +err_iio_device_register:
>>>>> +     kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
>>>>> +     return ret;
>>>>> +}
>>>>> +
>>>>> +static int kmx61_remove(struct i2c_client *client)
>>>>> +{
>>>>> +     struct iio_dev *indio_dev = i2c_get_clientdata(client);
>>>>> +     struct kmx61_data *data = iio_priv(indio_dev);
>>>>> +     int ret;
>>>>> +
>>>>> +     pm_runtime_disable(&client->dev);
>>>>> +     pm_runtime_set_suspended(&client->dev);
>>>>> +     pm_runtime_put_noidle(&client->dev);
>>>>> +
>>>>> +     iio_device_unregister(indio_dev);
>>>>> +
>>>>> +     mutex_lock(&data->lock);
>>>>> +     ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
>>>>> +     mutex_unlock(&data->lock);
>>>>> +
>>>>> +     return ret;
>>>>> +}
>>>>> +
>>>>> +#ifdef CONFIG_PM_SLEEP
>>>>> +static int kmx61_suspend(struct device *dev)
>>>>> +{
>>>>> +     struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>>>>> +     struct kmx61_data *data = iio_priv(indio_dev);
>>>>> +     int ret;
>>>>> +
>>>>> +     mutex_lock(&data->lock);
>>>>> +     ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG,
>>>>> +                          false);
>>>>> +     mutex_unlock(&data->lock);
>>>>> +
>>>>> +     return ret;
>>>>> +}
>>>>> +
>>>>> +static int kmx61_resume(struct device *dev)
>>>>> +{
>>>>> +     struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>>>>> +     struct kmx61_data *data = iio_priv(indio_dev);
>>>>> +     u8 stby = 0;
>>>>> +
>>>>> +     if (data->acc_stby)
>>>>> +             stby |= KMX61_ACC_STBY_BIT;
>>>>> +     if (data->mag_stby)
>>>>> +             stby |= KMX61_MAG_STBY_BIT;
>>>>> +
>>>>> +     return kmx61_set_mode(data, stby, KMX61_ACC | KMX61_MAG, true);
>>>>> +}
>>>>> +#endif
>>>>> +
>>>>> +#ifdef CONFIG_PM_RUNTIME
>>>>> +static int kmx61_runtime_suspend(struct device *dev)
>>>>> +{
>>>>> +     struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>>>>> +     struct kmx61_data *data = iio_priv(indio_dev);
>>>>> +     int ret;
>>>>> +
>>>>> +     mutex_lock(&data->lock);
>>>>> +     ret = kmx61_set_mode(data, KMX61_ALL_STBY, KMX61_ACC | KMX61_MAG, true);
>>>>> +     mutex_unlock(&data->lock);
>>>>> +
>>>>> +     return ret;
>>>>> +}
>>>>> +
>>>>> +static int kmx61_runtime_resume(struct device *dev)
>>>>> +{
>>>>> +     struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>>>>> +     struct kmx61_data *data = iio_priv(indio_dev);
>>>>> +     u8 stby = 0;
>>>>> +
>>>>> +     if (!data->acc_ps)
>>>>> +             stby |= KMX61_ACC_STBY_BIT;
>>>>> +     if (!data->mag_ps)
>>>>> +             stby |= KMX61_MAG_STBY_BIT;
>>>>> +
>>>>> +     return kmx61_set_mode(data, stby, KMX61_ACC | KMX61_MAG, true);
>>>>> +}
>>>>> +#endif
>>>>> +
>>>>> +static const struct dev_pm_ops kmx61_pm_ops = {
>>>>> +     SET_SYSTEM_SLEEP_PM_OPS(kmx61_suspend, kmx61_resume)
>>>>> +     SET_RUNTIME_PM_OPS(kmx61_runtime_suspend, kmx61_runtime_resume, NULL)
>>>>> +};
>>>>> +
>>>>> +static const struct i2c_device_id kmx61_id[] = {
>>>>> +     {"kmx611021", 0},
>>>>> +     {}
>>>>> +};
>>>>> +
>>>>> +MODULE_DEVICE_TABLE(i2c, kmx61_id);
>>>>> +
>>>>> +static struct i2c_driver kmx61_driver = {
>>>>> +     .driver = {
>>>>> +             .name = KMX61_DRV_NAME,
>>>>> +             .pm = &kmx61_pm_ops,
>>>>> +     },
>>>>> +     .probe          = kmx61_probe,
>>>>> +     .remove         = kmx61_remove,
>>>>> +     .id_table       = kmx61_id,
>>>>> +};
>>>>> +
>>>>> +module_i2c_driver(kmx61_driver);
>>>>> +
>>>>> +MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com>");
>>>>> +MODULE_DESCRIPTION("KMX61 accelerometer/magnetometer driver");
>>>>> +MODULE_LICENSE("GPL v2");
>>>
>>> Hartmut, thanks a lot for your reviews. I will send a patch series
>>> with these cleanups together with changes for ACPI, buffer and trigger
>>> support.
>> Don't know if Jonathan would like to see it smashed together, already.
>> Take care
>
> By together I mean separate patches in the same patch set.
>
> I will take

Hmm, pressed send to early.

I will take any suggestion on how to send this. Jonathan what do you think?

Daniel.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2014-11-25 23:03 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-18 16:47 [PATCH v3] iio: imu: Add support for Kionix KMX61 sensor Daniel Baluta
2014-11-22 10:46 ` Jonathan Cameron
2014-11-25  0:45 ` Hartmut Knaack
2014-11-25  9:54   ` Daniel Baluta
2014-11-25 22:31     ` Hartmut Knaack
2014-11-25 22:44       ` Daniel Baluta
2014-11-25 23:03         ` Daniel Baluta

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).