linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/4] iio: gyro: add support for fxas21002c
@ 2018-08-25 21:19 Afonso Bordado
  2018-08-25 21:19 ` [PATCH 2/4] iio: gyro: add device tree " Afonso Bordado
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Afonso Bordado @ 2018-08-25 21:19 UTC (permalink / raw)
  To: jic23, knaack.h, lars, pmeerw; +Cc: linux-iio, linux-kernel, devicetree

FXAS21002C is a 3 axis gyroscope with integrated temperature sensor

Signed-off-by: Afonso Bordado <afonsobordado@az8.co>
---
 drivers/iio/gyro/Kconfig      |  11 ++
 drivers/iio/gyro/Makefile     |   1 +
 drivers/iio/gyro/fxas21002c.c | 363 ++++++++++++++++++++++++++++++++++
 3 files changed, 375 insertions(+)
 create mode 100644 drivers/iio/gyro/fxas21002c.c

diff --git a/drivers/iio/gyro/Kconfig b/drivers/iio/gyro/Kconfig
index 3126cf05e6b9..d71e33ea9fa4 100644
--- a/drivers/iio/gyro/Kconfig
+++ b/drivers/iio/gyro/Kconfig
@@ -73,6 +73,17 @@ config BMG160_SPI
 	tristate
 	select REGMAP_SPI
 
+config FXAS21002C
+	tristate "Freescale FXAS21002C Gyroscope"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	  Say yes here to build support for the Freescale FXAS21002C Gyroscope
+	  driver connected via I2C.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called fxas21002c.
+
 config HID_SENSOR_GYRO_3D
 	depends on HID_SENSOR_HUB
 	select IIO_BUFFER
diff --git a/drivers/iio/gyro/Makefile b/drivers/iio/gyro/Makefile
index 295ec780c4eb..ec3e2aeae92a 100644
--- a/drivers/iio/gyro/Makefile
+++ b/drivers/iio/gyro/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_ADXRS450) += adxrs450.o
 obj-$(CONFIG_BMG160) += bmg160_core.o
 obj-$(CONFIG_BMG160_I2C) += bmg160_i2c.o
 obj-$(CONFIG_BMG160_SPI) += bmg160_spi.o
+obj-$(CONFIG_FXAS21002C) += fxas21002c.o
 
 obj-$(CONFIG_HID_SENSOR_GYRO_3D) += hid-sensor-gyro-3d.o
 
diff --git a/drivers/iio/gyro/fxas21002c.c b/drivers/iio/gyro/fxas21002c.c
new file mode 100644
index 000000000000..7626b2f88d72
--- /dev/null
+++ b/drivers/iio/gyro/fxas21002c.c
@@ -0,0 +1,363 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * FXAS21002C - Digital Angular Rate Gyroscope driver
+ *
+ * Copyright (c) 2018, Afonso Bordado <afonsobordado@az8.co>
+ *
+ * IIO driver for FXAS21002C (7-bit I2C slave address 0x20 or 0x21).
+ * Datasheet: https://www.nxp.com/docs/en/data-sheet/FXAS21002.pdf
+ * TODO:
+ *        ODR / Scale Support
+ *        Devicetree
+ *        Power management
+ *        LowPass/HighPass Filters
+ *        Buffers
+ *        Interrupts
+ *        Alarms
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/regmap.h>
+
+#define FXAS21002C_DRV_NAME "fxas21002c"
+
+#define FXAS21002C_MAX_TRANSITION_TIME_MS 61
+
+#define FXAS21002C_CHIP_ID             0xD7
+
+#define FXAS21002C_REG_STATUS          0x00
+#define FXAS21002C_REG_OUT_X_MSB       0x01
+#define FXAS21002C_REG_OUT_X_LSB       0x02
+#define FXAS21002C_REG_OUT_Y_MSB       0x03
+#define FXAS21002C_REG_OUT_Y_LSB       0x04
+#define FXAS21002C_REG_OUT_Z_MSB       0x05
+#define FXAS21002C_REG_OUT_Z_LSB       0x06
+#define FXAS21002C_REG_DR_STATUS       0x07
+#define FXAS21002C_REG_F_STATUS        0x08
+#define FXAS21002C_REG_F_SETUP         0x09
+#define FXAS21002C_REG_F_EVENT         0x0A
+#define FXAS21002C_REG_INT_SRC_FLAG    0x0B
+#define FXAS21002C_REG_WHO_AM_I        0x0C
+#define FXAS21002C_REG_CTRL_REG0       0x0D
+#define FXAS21002C_REG_RT_CFG          0x0E
+#define FXAS21002C_REG_RT_SRC          0x0F
+#define FXAS21002C_REG_RT_THS          0x10
+#define FXAS21002C_REG_RT_COUNT        0x11
+#define FXAS21002C_REG_TEMP            0x12
+
+#define FXAS21002C_REG_CTRL_REG1       0x13
+#define FXAS21002C_RST_BIT             BIT(6)
+#define FXAS21002C_ACTIVE_BIT          BIT(1)
+#define FXAS21002C_READY_BIT           BIT(0)
+
+#define FXAS21002C_REG_CTRL_REG2       0x14
+#define FXAS21002C_REG_CTRL_REG3       0x15
+
+#define FXAS21002C_DEFAULT_ODR_HZ      800
+
+// 0.0625 deg/s
+#define FXAS21002C_DEFAULT_SENSITIVITY IIO_DEGREE_TO_RAD(62500)
+
+enum fxas21002c_id {
+	ID_FXAS21002C,
+};
+
+enum fxas21002c_operating_mode {
+	FXAS21002C_OM_BOOT,
+	FXAS21002C_OM_STANDBY,
+	FXAS21002C_OM_READY,
+	FXAS21002C_OM_ACTIVE,
+};
+
+struct fxas21002c_data {
+	struct i2c_client *client;
+	struct regmap *regmap;
+};
+
+static const struct regmap_range fxas21002c_writable_ranges[] = {
+	regmap_reg_range(FXAS21002C_REG_F_SETUP, FXAS21002C_REG_F_SETUP),
+	regmap_reg_range(FXAS21002C_REG_CTRL_REG0, FXAS21002C_REG_RT_CFG),
+	regmap_reg_range(FXAS21002C_REG_RT_THS, FXAS21002C_REG_RT_COUNT),
+	regmap_reg_range(FXAS21002C_REG_CTRL_REG1, FXAS21002C_REG_CTRL_REG3),
+};
+
+static const struct regmap_access_table fxas21002c_writable_table = {
+	.yes_ranges = fxas21002c_writable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(fxas21002c_writable_ranges),
+};
+
+static const struct regmap_range fxas21002c_volatile_ranges[] = {
+	regmap_reg_range(FXAS21002C_REG_STATUS, FXAS21002C_REG_F_STATUS),
+	regmap_reg_range(FXAS21002C_REG_F_EVENT, FXAS21002C_REG_INT_SRC_FLAG),
+	regmap_reg_range(FXAS21002C_REG_RT_COUNT, FXAS21002C_REG_CTRL_REG1),
+};
+
+static const struct regmap_access_table fxas21002c_volatile_table = {
+	.yes_ranges = fxas21002c_volatile_ranges,
+	.n_yes_ranges = ARRAY_SIZE(fxas21002c_volatile_ranges),
+};
+
+const struct regmap_config fxas21002c_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.max_register = FXAS21002C_REG_CTRL_REG3,
+	// We don't specify a .rd_table because everything is readable
+	.wr_table = &fxas21002c_writable_table,
+	.volatile_table = &fxas21002c_volatile_table,
+};
+EXPORT_SYMBOL(fxas21002c_regmap_config);
+
+#define FXAS21002C_GYRO_CHAN(_axis) {					\
+	.type = IIO_ANGL_VEL,						\
+	.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 = FXAS21002C_REG_OUT_ ## _axis ## _MSB,		\
+}
+
+static const struct iio_chan_spec fxas21002c_channels[] = {
+	{
+		.type = IIO_TEMP,
+		.address = FXAS21002C_REG_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	},
+	FXAS21002C_GYRO_CHAN(X),
+	FXAS21002C_GYRO_CHAN(Y),
+	FXAS21002C_GYRO_CHAN(Z),
+	IIO_CHAN_SOFT_TIMESTAMP(3),
+};
+
+static int fxas21002c_set_operating_mode(struct fxas21002c_data *data,
+					 enum fxas21002c_operating_mode om)
+{
+	int ret;
+	int mask = 0;
+
+	switch (om) {
+	case FXAS21002C_OM_STANDBY:
+		break;
+	case FXAS21002C_OM_READY:
+		mask |= FXAS21002C_READY_BIT;
+		break;
+	case FXAS21002C_OM_ACTIVE:
+		mask |= FXAS21002C_ACTIVE_BIT;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	ret = regmap_write(data->regmap, FXAS21002C_REG_CTRL_REG1, mask);
+	if (ret) {
+		dev_err(&data->client->dev,
+			"could not switch operating mode\n");
+		return ret;
+	}
+
+	msleep(FXAS21002C_MAX_TRANSITION_TIME_MS);
+
+	return 0;
+}
+
+static int fxas21002c_reset(struct fxas21002c_data *data)
+{
+	int ret;
+
+	/*
+	 * On issuing a Software Reset command over an I2C interface,
+	 * the device immediately resets and does not send any
+	 * acknowledgment (ACK) of the written byte to the Master.
+	 *
+	 * This is documented in table 46 on the datasheet. Due to this
+	 * the write will fail with EREMOTEIO.
+	 */
+	ret = regmap_write(data->regmap,
+			   FXAS21002C_REG_CTRL_REG1, FXAS21002C_RST_BIT);
+
+	if (ret != -EREMOTEIO) {
+		dev_err(&data->client->dev, "could not reset device\n");
+		return ret;
+	}
+
+	regcache_mark_dirty(data->regmap);
+
+	// Wait for device to boot up
+	msleep(FXAS21002C_MAX_TRANSITION_TIME_MS);
+
+	return 0;
+}
+
+static int fxas21002c_verify_chip(struct fxas21002c_data *data)
+{
+	int ret;
+	int chip_id;
+
+	ret = regmap_read(data->regmap, FXAS21002C_REG_WHO_AM_I, &chip_id);
+	if (ret) {
+		dev_err(&data->client->dev, "could not read device id\n");
+		return ret;
+	}
+
+	if (chip_id != FXAS21002C_CHIP_ID) {
+		dev_err(&data->client->dev,
+			"unsupported chip id %02x\n", chip_id);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int fxas21002c_read_oneshot(struct fxas21002c_data *data,
+				   struct iio_chan_spec const *chan, int *val)
+{
+	int ret;
+	int raw;
+	__be16 bulk_raw;
+
+	switch (chan->type) {
+	case IIO_ANGL_VEL:
+		ret = regmap_bulk_read(data->regmap, chan->address,
+				       &bulk_raw, sizeof(bulk_raw));
+		if (ret)
+			return ret;
+
+		*val = sign_extend32(be16_to_cpu(bulk_raw), 15);
+		break;
+	case IIO_TEMP:
+		ret = regmap_read(data->regmap, chan->address, &raw);
+		if (ret)
+			return ret;
+
+		*val = raw * 1000; // Convert to millicelsius
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return IIO_VAL_INT;
+}
+
+static int fxas21002c_read_raw(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan, int *val,
+			       int *val2, long mask)
+{
+	struct fxas21002c_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		return fxas21002c_read_oneshot(data, chan, val);
+	case IIO_CHAN_INFO_SCALE:
+		if (chan->type != IIO_ANGL_VEL)
+			return -EINVAL;
+
+		*val = 0;
+		*val2 = FXAS21002C_DEFAULT_SENSITIVITY;
+
+		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		if (chan->type != IIO_ANGL_VEL)
+			return -EINVAL;
+
+		*val = FXAS21002C_DEFAULT_ODR_HZ;
+
+		return IIO_VAL_INT;
+	}
+
+	return -EINVAL;
+}
+
+static const struct iio_info fxas21002c_info = {
+	.read_raw		= fxas21002c_read_raw,
+};
+
+static int fxas21002c_probe(struct i2c_client *client,
+			    const struct i2c_device_id *id)
+{
+	int ret;
+	struct iio_dev *indio_dev;
+	struct fxas21002c_data *data;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, indio_dev);
+	data = iio_priv(indio_dev);
+	data->client = client;
+
+	data->regmap = devm_regmap_init_i2c(client, &fxas21002c_regmap_config);
+	if (IS_ERR(data->regmap)) {
+		ret = PTR_ERR(data->regmap);
+		dev_err(&client->dev,
+			"Failed to allocate regmap, err: %d\n", ret);
+		return ret;
+	}
+
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->channels = fxas21002c_channels;
+	indio_dev->num_channels = ARRAY_SIZE(fxas21002c_channels);
+	indio_dev->name = id->name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &fxas21002c_info;
+
+	ret = fxas21002c_verify_chip(data);
+	if (ret < 0)
+		return ret;
+
+	ret = fxas21002c_reset(data);
+	if (ret < 0)
+		return ret;
+
+	ret = fxas21002c_set_operating_mode(data, FXAS21002C_OM_ACTIVE);
+	if (ret < 0)
+		return ret;
+
+	ret = iio_device_register(indio_dev);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to register iio device\n");
+		fxas21002c_set_operating_mode(data, FXAS21002C_OM_STANDBY);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int fxas21002c_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct fxas21002c_data *data = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+
+	fxas21002c_set_operating_mode(data, FXAS21002C_OM_STANDBY);
+
+	return 0;
+}
+
+static const struct i2c_device_id fxas21002c_id[] = {
+	{"fxas21002c", ID_FXAS21002C},
+	{}
+};
+
+MODULE_DEVICE_TABLE(i2c, fxas21002c_id);
+
+static struct i2c_driver fxas21002c_driver = {
+	.driver = {
+		.name = FXAS21002C_DRV_NAME,
+	},
+	.probe		= fxas21002c_probe,
+	.remove		= fxas21002c_remove,
+	.id_table	= fxas21002c_id,
+};
+
+module_i2c_driver(fxas21002c_driver);
+
+MODULE_AUTHOR("Afonso Bordado <afonsobordado@az8.co>");
+MODULE_DESCRIPTION("FXAS21002C Digital Angular Rate Gyroscope driver");
+MODULE_LICENSE("GPL v2");
-- 
2.18.0



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

* [PATCH 2/4] iio: gyro: add device tree support for fxas21002c
  2018-08-25 21:19 [PATCH 1/4] iio: gyro: add support for fxas21002c Afonso Bordado
@ 2018-08-25 21:19 ` Afonso Bordado
  2018-08-27 17:13   ` Jonathan Cameron
  2018-08-25 21:19 ` [PATCH 3/4] iio: fxas21002c: add ODR/Scale support Afonso Bordado
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Afonso Bordado @ 2018-08-25 21:19 UTC (permalink / raw)
  To: jic23, knaack.h, lars, pmeerw; +Cc: linux-iio, linux-kernel, devicetree

This patch adds device tree support for the fxas21002c driver, including
bindings.

Signed-off-by: Afonso Bordado <afonsobordado@az8.co>
---
 .../bindings/iio/gyroscope/fsl,fxas21002c.txt        | 12 ++++++++++++
 drivers/iio/gyro/fxas21002c.c                        | 10 +++++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/iio/gyroscope/fsl,fxas21002c.txt

diff --git a/Documentation/devicetree/bindings/iio/gyroscope/fsl,fxas21002c.txt b/Documentation/devicetree/bindings/iio/gyroscope/fsl,fxas21002c.txt
new file mode 100644
index 000000000000..62f8c1bad85a
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/gyroscope/fsl,fxas21002c.txt
@@ -0,0 +1,12 @@
+* Freescale FXAS21002C Digital Angular Rate Gyroscope
+
+Required properties:
+
+  - compatible: must be "fsl,fxas21002c"
+  - reg : the I2C address of the sensor
+
+Example:
+gyroscope@0 {
+       compatible = "fsl,fxas21002c";
+       reg = <0x20>;
+};
diff --git a/drivers/iio/gyro/fxas21002c.c b/drivers/iio/gyro/fxas21002c.c
index 7626b2f88d72..6fef210630e0 100644
--- a/drivers/iio/gyro/fxas21002c.c
+++ b/drivers/iio/gyro/fxas21002c.c
@@ -8,7 +8,6 @@
  * Datasheet: https://www.nxp.com/docs/en/data-sheet/FXAS21002.pdf
  * TODO:
  *        ODR / Scale Support
- *        Devicetree
  *        Power management
  *        LowPass/HighPass Filters
  *        Buffers
@@ -340,6 +339,14 @@ static int fxas21002c_remove(struct i2c_client *client)
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id fxas21002c_of_ids[] = {
+	{.compatible = "fsl,fxas21002c"},
+	{}
+};
+MODULE_DEVICE_TABLE(of, fxas21002c_of_ids);
+#endif
+
 static const struct i2c_device_id fxas21002c_id[] = {
 	{"fxas21002c", ID_FXAS21002C},
 	{}
@@ -350,6 +357,7 @@ MODULE_DEVICE_TABLE(i2c, fxas21002c_id);
 static struct i2c_driver fxas21002c_driver = {
 	.driver = {
 		.name = FXAS21002C_DRV_NAME,
+		.of_match_table = of_match_ptr(fxas21002c_of_ids),
 	},
 	.probe		= fxas21002c_probe,
 	.remove		= fxas21002c_remove,
-- 
2.18.0



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

* [PATCH 3/4] iio: fxas21002c: add ODR/Scale support
  2018-08-25 21:19 [PATCH 1/4] iio: gyro: add support for fxas21002c Afonso Bordado
  2018-08-25 21:19 ` [PATCH 2/4] iio: gyro: add device tree " Afonso Bordado
@ 2018-08-25 21:19 ` Afonso Bordado
  2018-08-27 17:18   ` Jonathan Cameron
  2018-08-25 21:19 ` [PATCH 4/4] MAINTAINERS: add entry for fxas21002c gyro driver Afonso Bordado
  2018-08-27 17:08 ` [PATCH 1/4] iio: gyro: add support for fxas21002c Jonathan Cameron
  3 siblings, 1 reply; 12+ messages in thread
From: Afonso Bordado @ 2018-08-25 21:19 UTC (permalink / raw)
  To: jic23, knaack.h, lars, pmeerw; +Cc: linux-iio, linux-kernel, devicetree

This patch adds support for reading/writing ODR/Scale

We don't support the scale boost modes.

Signed-off-by: Afonso Bordado <afonsobordado@az8.co>
---
 drivers/iio/gyro/fxas21002c.c | 162 +++++++++++++++++++++++++++++++---
 1 file changed, 149 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/gyro/fxas21002c.c b/drivers/iio/gyro/fxas21002c.c
index 6fef210630e0..dc0cb9848386 100644
--- a/drivers/iio/gyro/fxas21002c.c
+++ b/drivers/iio/gyro/fxas21002c.c
@@ -7,7 +7,7 @@
  * IIO driver for FXAS21002C (7-bit I2C slave address 0x20 or 0x21).
  * Datasheet: https://www.nxp.com/docs/en/data-sheet/FXAS21002.pdf
  * TODO:
- *        ODR / Scale Support
+ *        Scale boost mode
  *        Power management
  *        LowPass/HighPass Filters
  *        Buffers
@@ -40,7 +40,10 @@
 #define FXAS21002C_REG_F_EVENT         0x0A
 #define FXAS21002C_REG_INT_SRC_FLAG    0x0B
 #define FXAS21002C_REG_WHO_AM_I        0x0C
+
 #define FXAS21002C_REG_CTRL_REG0       0x0D
+#define FXAS21002C_SCALE_MASK          (BIT(0) | BIT(1))
+
 #define FXAS21002C_REG_RT_CFG          0x0E
 #define FXAS21002C_REG_RT_SRC          0x0F
 #define FXAS21002C_REG_RT_THS          0x10
@@ -52,13 +55,12 @@
 #define FXAS21002C_ACTIVE_BIT          BIT(1)
 #define FXAS21002C_READY_BIT           BIT(0)
 
-#define FXAS21002C_REG_CTRL_REG2       0x14
-#define FXAS21002C_REG_CTRL_REG3       0x15
+#define FXAS21002C_ODR_SHIFT           2
+#define FXAS21002C_ODR_MASK            (BIT(2) | BIT(3) | BIT(4))
 
-#define FXAS21002C_DEFAULT_ODR_HZ      800
 
-// 0.0625 deg/s
-#define FXAS21002C_DEFAULT_SENSITIVITY IIO_DEGREE_TO_RAD(62500)
+#define FXAS21002C_REG_CTRL_REG2       0x14
+#define FXAS21002C_REG_CTRL_REG3       0x15
 
 enum fxas21002c_id {
 	ID_FXAS21002C,
@@ -76,6 +78,40 @@ struct fxas21002c_data {
 	struct regmap *regmap;
 };
 
+enum fxas21002c_scale {
+	FXAS21002C_SCALE_62MDPS,
+	FXAS21002C_SCALE_31MDPS,
+	FXAS21002C_SCALE_15MDPS,
+	FXAS21002C_SCALE_7MDPS,
+};
+
+static const int fxas21002c_anglevel_scale_avail[4][2] = {
+	[FXAS21002C_SCALE_62MDPS] = { 0, IIO_DEGREE_TO_RAD(62500) },
+	[FXAS21002C_SCALE_31MDPS] = { 0, IIO_DEGREE_TO_RAD(31250) },
+	[FXAS21002C_SCALE_15MDPS] = { 0, IIO_DEGREE_TO_RAD(15625) },
+	[FXAS21002C_SCALE_7MDPS]  = { 0, IIO_DEGREE_TO_RAD(7812) },
+};
+
+enum fxas21002c_odr {
+	FXAS21002C_ODR_800,
+	FXAS21002C_ODR_400,
+	FXAS21002C_ODR_200,
+	FXAS21002C_ODR_100,
+	FXAS21002C_ODR_50,
+	FXAS21002C_ODR_25,
+	FXAS21002C_ODR_12_5,
+};
+
+static const int fxas21002c_sample_freq_avail[7][2] = {
+	[FXAS21002C_ODR_800]  = { 800, 0 },
+	[FXAS21002C_ODR_400]  = { 400, 0 },
+	[FXAS21002C_ODR_200]  = { 200, 0 },
+	[FXAS21002C_ODR_100]  = { 100, 0 },
+	[FXAS21002C_ODR_50]   = { 50, 0 },
+	[FXAS21002C_ODR_25]   = { 25, 0 },
+	[FXAS21002C_ODR_12_5] = { 12, 500000 },
+};
+
 static const struct regmap_range fxas21002c_writable_ranges[] = {
 	regmap_reg_range(FXAS21002C_REG_F_SETUP, FXAS21002C_REG_F_SETUP),
 	regmap_reg_range(FXAS21002C_REG_CTRL_REG0, FXAS21002C_REG_RT_CFG),
@@ -242,6 +278,47 @@ static int fxas21002c_read_oneshot(struct fxas21002c_data *data,
 	return IIO_VAL_INT;
 }
 
+static int fxas21002c_scale_read(struct fxas21002c_data *data, int *val,
+				 int *val2)
+{
+	int ret;
+	unsigned int raw;
+
+	ret = regmap_read(data->regmap, FXAS21002C_REG_CTRL_REG0, &raw);
+	if (ret)
+		return ret;
+
+	raw &= FXAS21002C_SCALE_MASK;
+
+	*val = fxas21002c_anglevel_scale_avail[raw][0];
+	*val2 = fxas21002c_anglevel_scale_avail[raw][1];
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int fxas21002c_odr_read(struct fxas21002c_data *data, int *val,
+			       int *val2)
+{
+	int ret;
+	unsigned int raw;
+
+	ret = regmap_read(data->regmap, FXAS21002C_REG_CTRL_REG1, &raw);
+	if (ret)
+		return ret;
+
+	raw = (raw & FXAS21002C_ODR_MASK) >> FXAS21002C_ODR_SHIFT;
+
+	// We don't use this mode but according to the datasheet its
+	// also a 12.5Hz
+	if (raw == 7)
+		raw = FXAS21002C_ODR_12_5;
+
+	*val = fxas21002c_sample_freq_avail[raw][0];
+	*val2 = fxas21002c_sample_freq_avail[raw][1];
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
 static int fxas21002c_read_raw(struct iio_dev *indio_dev,
 			       struct iio_chan_spec const *chan, int *val,
 			       int *val2, long mask)
@@ -255,24 +332,83 @@ static int fxas21002c_read_raw(struct iio_dev *indio_dev,
 		if (chan->type != IIO_ANGL_VEL)
 			return -EINVAL;
 
-		*val = 0;
-		*val2 = FXAS21002C_DEFAULT_SENSITIVITY;
-
-		return IIO_VAL_INT_PLUS_MICRO;
+		return fxas21002c_scale_read(data, val, val2);
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		if (chan->type != IIO_ANGL_VEL)
 			return -EINVAL;
 
-		*val = FXAS21002C_DEFAULT_ODR_HZ;
-
-		return IIO_VAL_INT;
+		return fxas21002c_odr_read(data, val, val2);
 	}
 
 	return -EINVAL;
 }
 
+static int fxas21002c_write_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan, int val,
+				int val2, long mask)
+{
+	struct fxas21002c_data *data = iio_priv(indio_dev);
+	int ret = -EINVAL;
+	int i;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		for (i = 0; i < ARRAY_SIZE(fxas21002c_sample_freq_avail); i++) {
+			if (fxas21002c_sample_freq_avail[i][0] == val &&
+			    fxas21002c_sample_freq_avail[i][1] == val2)
+				break;
+		}
+
+		if (i == ARRAY_SIZE(fxas21002c_sample_freq_avail))
+			break;
+
+		ret = regmap_update_bits(data->regmap, FXAS21002C_REG_CTRL_REG1,
+					 FXAS21002C_ODR_MASK,
+					 i << FXAS21002C_ODR_SHIFT);
+
+		break;
+	case IIO_CHAN_INFO_SCALE:
+		for (i = 0; i < ARRAY_SIZE(fxas21002c_anglevel_scale_avail);
+		     i++) {
+			if (fxas21002c_anglevel_scale_avail[i][0] == val &&
+			    fxas21002c_anglevel_scale_avail[i][1] == val2)
+				break;
+		}
+
+		if (i == ARRAY_SIZE(fxas21002c_anglevel_scale_avail))
+			break;
+
+		ret = regmap_update_bits(data->regmap, FXAS21002C_REG_CTRL_REG0,
+					 FXAS21002C_SCALE_MASK, i);
+
+		break;
+	}
+
+	return ret;
+}
+
+static IIO_CONST_ATTR(anglevel_scale_available,
+		      "0.001090831 "  // 62.5    mdps
+		      "0.000545415 "  // 31.25   mdps
+		      "0.000272708 "  // 15.625  mdps
+		      "0.000136354"); //  7.8125 mdps
+
+static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("800 400 200 100 50 25 12.5");
+
+static struct attribute *fxas21002c_attributes[] = {
+	&iio_const_attr_anglevel_scale_available.dev_attr.attr,
+	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group fxas21002c_attribute_group = {
+	.attrs = fxas21002c_attributes,
+};
+
 static const struct iio_info fxas21002c_info = {
 	.read_raw		= fxas21002c_read_raw,
+	.write_raw              = fxas21002c_write_raw,
+	.attrs                  = &fxas21002c_attribute_group,
 };
 
 static int fxas21002c_probe(struct i2c_client *client,
-- 
2.18.0



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

* [PATCH 4/4] MAINTAINERS: add entry for fxas21002c gyro driver
  2018-08-25 21:19 [PATCH 1/4] iio: gyro: add support for fxas21002c Afonso Bordado
  2018-08-25 21:19 ` [PATCH 2/4] iio: gyro: add device tree " Afonso Bordado
  2018-08-25 21:19 ` [PATCH 3/4] iio: fxas21002c: add ODR/Scale support Afonso Bordado
@ 2018-08-25 21:19 ` Afonso Bordado
  2018-08-27 17:08 ` [PATCH 1/4] iio: gyro: add support for fxas21002c Jonathan Cameron
  3 siblings, 0 replies; 12+ messages in thread
From: Afonso Bordado @ 2018-08-25 21:19 UTC (permalink / raw)
  To: jic23, knaack.h, lars, pmeerw; +Cc: linux-iio, linux-kernel, devicetree

Add entry for fxas21002c gyroscope driver and add myself as
maintainer of this driver.

Signed-off-by: Afonso Bordado <afonsobordado@az8.co>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 2b7b24b145f0..faf5f41b1465 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5838,6 +5838,13 @@ L:	linuxppc-dev@lists.ozlabs.org
 S:	Maintained
 F:	drivers/usb/gadget/udc/fsl*
 
+FREESCALE FXAS21002C
+M:	Afonso Bordado <afonsobordado@az8.co>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	drivers/iio/gyro/fxas21002.c
+F:	Documentation/devicetree/bindings/iio/gyroscope/fsl,fxas21002c.txt
+
 FREEVXFS FILESYSTEM
 M:	Christoph Hellwig <hch@infradead.org>
 W:	ftp://ftp.openlinux.org/pub/people/hch/vxfs
-- 
2.18.0



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

* Re: [PATCH 1/4] iio: gyro: add support for fxas21002c
  2018-08-25 21:19 [PATCH 1/4] iio: gyro: add support for fxas21002c Afonso Bordado
                   ` (2 preceding siblings ...)
  2018-08-25 21:19 ` [PATCH 4/4] MAINTAINERS: add entry for fxas21002c gyro driver Afonso Bordado
@ 2018-08-27 17:08 ` Jonathan Cameron
  2018-08-27 20:50   ` Joe Perches
  2018-08-29  6:43   ` Afonso Bordado
  3 siblings, 2 replies; 12+ messages in thread
From: Jonathan Cameron @ 2018-08-27 17:08 UTC (permalink / raw)
  To: Afonso Bordado
  Cc: knaack.h, lars, pmeerw, linux-iio, linux-kernel, devicetree

On Sat, 25 Aug 2018 22:19:07 +0100
Afonso Bordado <afonsobordado@az8.co> wrote:

> FXAS21002C is a 3 axis gyroscope with integrated temperature sensor
> 
> Signed-off-by: Afonso Bordado <afonsobordado@az8.co>

Hi,

Driver is pretty clean so only a few minor comments inline.
If we were late in a cycle I'd probably just have taken it and fixed up
but as we have lots of time and I'm inherently lazy I'll let you do a v2 :)

Good job, thanks!

Jonathan


> ---
>  drivers/iio/gyro/Kconfig      |  11 ++
>  drivers/iio/gyro/Makefile     |   1 +
>  drivers/iio/gyro/fxas21002c.c | 363 ++++++++++++++++++++++++++++++++++
>  3 files changed, 375 insertions(+)
>  create mode 100644 drivers/iio/gyro/fxas21002c.c
> 
> diff --git a/drivers/iio/gyro/Kconfig b/drivers/iio/gyro/Kconfig
> index 3126cf05e6b9..d71e33ea9fa4 100644
> --- a/drivers/iio/gyro/Kconfig
> +++ b/drivers/iio/gyro/Kconfig
> @@ -73,6 +73,17 @@ config BMG160_SPI
>  	tristate
>  	select REGMAP_SPI
>  
> +config FXAS21002C
> +	tristate "Freescale FXAS21002C Gyroscope"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  Say yes here to build support for the Freescale FXAS21002C Gyroscope
> +	  driver connected via I2C.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called fxas21002c.
> +
>  config HID_SENSOR_GYRO_3D
>  	depends on HID_SENSOR_HUB
>  	select IIO_BUFFER
> diff --git a/drivers/iio/gyro/Makefile b/drivers/iio/gyro/Makefile
> index 295ec780c4eb..ec3e2aeae92a 100644
> --- a/drivers/iio/gyro/Makefile
> +++ b/drivers/iio/gyro/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_ADXRS450) += adxrs450.o
>  obj-$(CONFIG_BMG160) += bmg160_core.o
>  obj-$(CONFIG_BMG160_I2C) += bmg160_i2c.o
>  obj-$(CONFIG_BMG160_SPI) += bmg160_spi.o
> +obj-$(CONFIG_FXAS21002C) += fxas21002c.o
>  
>  obj-$(CONFIG_HID_SENSOR_GYRO_3D) += hid-sensor-gyro-3d.o
>  
> diff --git a/drivers/iio/gyro/fxas21002c.c b/drivers/iio/gyro/fxas21002c.c
> new file mode 100644
> index 000000000000..7626b2f88d72
> --- /dev/null
> +++ b/drivers/iio/gyro/fxas21002c.c
> @@ -0,0 +1,363 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * FXAS21002C - Digital Angular Rate Gyroscope driver
> + *
> + * Copyright (c) 2018, Afonso Bordado <afonsobordado@az8.co>
> + *
> + * IIO driver for FXAS21002C (7-bit I2C slave address 0x20 or 0x21).
> + * Datasheet: https://www.nxp.com/docs/en/data-sheet/FXAS21002.pdf
> + * TODO:
> + *        ODR / Scale Support
> + *        Devicetree
> + *        Power management
> + *        LowPass/HighPass Filters
> + *        Buffers
> + *        Interrupts
> + *        Alarms
> + */
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/regmap.h>
> +
> +#define FXAS21002C_DRV_NAME "fxas21002c"
> +
> +#define FXAS21002C_MAX_TRANSITION_TIME_MS 61
> +
> +#define FXAS21002C_CHIP_ID             0xD7
> +
> +#define FXAS21002C_REG_STATUS          0x00
> +#define FXAS21002C_REG_OUT_X_MSB       0x01
> +#define FXAS21002C_REG_OUT_X_LSB       0x02
> +#define FXAS21002C_REG_OUT_Y_MSB       0x03
> +#define FXAS21002C_REG_OUT_Y_LSB       0x04
> +#define FXAS21002C_REG_OUT_Z_MSB       0x05
> +#define FXAS21002C_REG_OUT_Z_LSB       0x06
> +#define FXAS21002C_REG_DR_STATUS       0x07
> +#define FXAS21002C_REG_F_STATUS        0x08
> +#define FXAS21002C_REG_F_SETUP         0x09
> +#define FXAS21002C_REG_F_EVENT         0x0A
> +#define FXAS21002C_REG_INT_SRC_FLAG    0x0B
> +#define FXAS21002C_REG_WHO_AM_I        0x0C
> +#define FXAS21002C_REG_CTRL_REG0       0x0D
> +#define FXAS21002C_REG_RT_CFG          0x0E
> +#define FXAS21002C_REG_RT_SRC          0x0F
> +#define FXAS21002C_REG_RT_THS          0x10
> +#define FXAS21002C_REG_RT_COUNT        0x11
> +#define FXAS21002C_REG_TEMP            0x12
> +
> +#define FXAS21002C_REG_CTRL_REG1       0x13
> +#define FXAS21002C_RST_BIT             BIT(6)
> +#define FXAS21002C_ACTIVE_BIT          BIT(1)
> +#define FXAS21002C_READY_BIT           BIT(0)
> +
> +#define FXAS21002C_REG_CTRL_REG2       0x14
> +#define FXAS21002C_REG_CTRL_REG3       0x15
> +
> +#define FXAS21002C_DEFAULT_ODR_HZ      800
> +
> +// 0.0625 deg/s
/* .. */
> +#define FXAS21002C_DEFAULT_SENSITIVITY IIO_DEGREE_TO_RAD(62500)
> +
> +enum fxas21002c_id {
> +	ID_FXAS21002C,
> +};
> +
> +enum fxas21002c_operating_mode {
> +	FXAS21002C_OM_BOOT,
> +	FXAS21002C_OM_STANDBY,
> +	FXAS21002C_OM_READY,
> +	FXAS21002C_OM_ACTIVE,
> +};
> +
> +struct fxas21002c_data {
> +	struct i2c_client *client;
> +	struct regmap *regmap;
> +};
> +
> +static const struct regmap_range fxas21002c_writable_ranges[] = {
> +	regmap_reg_range(FXAS21002C_REG_F_SETUP, FXAS21002C_REG_F_SETUP),
> +	regmap_reg_range(FXAS21002C_REG_CTRL_REG0, FXAS21002C_REG_RT_CFG),
> +	regmap_reg_range(FXAS21002C_REG_RT_THS, FXAS21002C_REG_RT_COUNT),
> +	regmap_reg_range(FXAS21002C_REG_CTRL_REG1, FXAS21002C_REG_CTRL_REG3),
> +};
> +
> +static const struct regmap_access_table fxas21002c_writable_table = {
> +	.yes_ranges = fxas21002c_writable_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(fxas21002c_writable_ranges),
> +};
> +
> +static const struct regmap_range fxas21002c_volatile_ranges[] = {
> +	regmap_reg_range(FXAS21002C_REG_STATUS, FXAS21002C_REG_F_STATUS),
> +	regmap_reg_range(FXAS21002C_REG_F_EVENT, FXAS21002C_REG_INT_SRC_FLAG),
> +	regmap_reg_range(FXAS21002C_REG_RT_COUNT, FXAS21002C_REG_CTRL_REG1),
> +};
> +
> +static const struct regmap_access_table fxas21002c_volatile_table = {
> +	.yes_ranges = fxas21002c_volatile_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(fxas21002c_volatile_ranges),
> +};
> +
> +const struct regmap_config fxas21002c_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +
> +	.max_register = FXAS21002C_REG_CTRL_REG3,
> +	// We don't specify a .rd_table because everything is readable
/* ... */

Please run checkpatch as IIRC it complains about this.

> +	.wr_table = &fxas21002c_writable_table,
> +	.volatile_table = &fxas21002c_volatile_table,
> +};
> +EXPORT_SYMBOL(fxas21002c_regmap_config);
> +
> +#define FXAS21002C_GYRO_CHAN(_axis) {					\
> +	.type = IIO_ANGL_VEL,						\
> +	.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 = FXAS21002C_REG_OUT_ ## _axis ## _MSB,		\
> +}
> +
> +static const struct iio_chan_spec fxas21002c_channels[] = {
> +	{
> +		.type = IIO_TEMP,
> +		.address = FXAS21002C_REG_TEMP,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),

As it currently stands it is IIO_CHAN_PROCESSED but I'd prefer you provided
the scale and kept it as _RAW.

> +	},
> +	FXAS21002C_GYRO_CHAN(X),
> +	FXAS21002C_GYRO_CHAN(Y),
> +	FXAS21002C_GYRO_CHAN(Z),
> +	IIO_CHAN_SOFT_TIMESTAMP(3),
> +};
> +
> +static int fxas21002c_set_operating_mode(struct fxas21002c_data *data,
> +					 enum fxas21002c_operating_mode om)
> +{
> +	int ret;
> +	int mask = 0;
Might be clearer to not set this here and...
> +
> +	switch (om) {
> +	case FXAS21002C_OM_STANDBY:
		mask = 0;
> +		break;
> +	case FXAS21002C_OM_READY:
> +		mask |= FXAS21002C_READY_BIT;
		mask = FXA210002C_READY_BIT;
> +		break;
> +	case FXAS21002C_OM_ACTIVE:
> +		mask |= FXAS21002C_ACTIVE_BIT;
		mask = FXA21002C_ACTIVE_BIT;

Slightly more readable I think...
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_write(data->regmap, FXAS21002C_REG_CTRL_REG1, mask);
> +	if (ret) {
> +		dev_err(&data->client->dev,
> +			"could not switch operating mode\n");
> +		return ret;
> +	}
> +
> +	msleep(FXAS21002C_MAX_TRANSITION_TIME_MS);
> +
> +	return 0;
> +}
> +
> +static int fxas21002c_reset(struct fxas21002c_data *data)
> +{
> +	int ret;
> +
> +	/*
> +	 * On issuing a Software Reset command over an I2C interface,
> +	 * the device immediately resets and does not send any
> +	 * acknowledgment (ACK) of the written byte to the Master.
> +	 *
> +	 * This is documented in table 46 on the datasheet. Due to this
> +	 * the write will fail with EREMOTEIO.
> +	 */
> +	ret = regmap_write(data->regmap,
> +			   FXAS21002C_REG_CTRL_REG1, FXAS21002C_RST_BIT);
> +
> +	if (ret != -EREMOTEIO) {
> +		dev_err(&data->client->dev, "could not reset device\n");
> +		return ret;
> +	}
> +
> +	regcache_mark_dirty(data->regmap);
> +
> +	// Wait for device to boot up

/* Wait... */

> +	msleep(FXAS21002C_MAX_TRANSITION_TIME_MS);
> +
> +	return 0;
> +}
> +
> +static int fxas21002c_verify_chip(struct fxas21002c_data *data)
> +{
> +	int ret;
> +	int chip_id;
> +
> +	ret = regmap_read(data->regmap, FXAS21002C_REG_WHO_AM_I, &chip_id);
> +	if (ret) {
> +		dev_err(&data->client->dev, "could not read device id\n");
> +		return ret;
> +	}
> +
> +	if (chip_id != FXAS21002C_CHIP_ID) {
> +		dev_err(&data->client->dev,
> +			"unsupported chip id %02x\n", chip_id);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static int fxas21002c_read_oneshot(struct fxas21002c_data *data,
> +				   struct iio_chan_spec const *chan, int *val)
> +{
> +	int ret;
> +	int raw;
> +	__be16 bulk_raw;
> +
> +	switch (chan->type) {
> +	case IIO_ANGL_VEL:
> +		ret = regmap_bulk_read(data->regmap, chan->address,
> +				       &bulk_raw, sizeof(bulk_raw));
> +		if (ret)
> +			return ret;
> +
> +		*val = sign_extend32(be16_to_cpu(bulk_raw), 15);
> +		break;

return IIO_VAL_INT directly here.

> +	case IIO_TEMP:
> +		ret = regmap_read(data->regmap, chan->address, &raw);
> +		if (ret)
> +			return ret;
> +
> +		*val = raw * 1000; // Convert to millicelsius

Don't use c++ style comments in kernel code please.

Also I wouldn't do this multiplier in here, I'd provide the scale attribute.
The reason is that we may later have support for buffered reads from this
device - at that point we will want to have nice 8 bit data rather than having
a scaled value that isn't quite a whole number of bits.


> +		break;
return IIO_VAL_INT; directly here.
> +	default:
> +		return -EINVAL;
> +	}
> +

With the above two changes in place this return is never reached.

> +	return IIO_VAL_INT;
> +}
> +
> +static int fxas21002c_read_raw(struct iio_dev *indio_dev,
> +			       struct iio_chan_spec const *chan, int *val,
> +			       int *val2, long mask)
> +{
> +	struct fxas21002c_data *data = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		return fxas21002c_read_oneshot(data, chan, val);
> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type != IIO_ANGL_VEL)
> +			return -EINVAL;
This check is a bit over paranoid given there is no way
this would be called in with a different type.

Doesn't do any real harm though.

> +
> +		*val = 0;
> +		*val2 = FXAS21002C_DEFAULT_SENSITIVITY;
> +
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (chan->type != IIO_ANGL_VEL)
> +			return -EINVAL;
> +
> +		*val = FXAS21002C_DEFAULT_ODR_HZ;
> +
> +		return IIO_VAL_INT;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct iio_info fxas21002c_info = {
> +	.read_raw		= fxas21002c_read_raw,
> +};
> +
> +static int fxas21002c_probe(struct i2c_client *client,
> +			    const struct i2c_device_id *id)
> +{
> +	int ret;
> +	struct iio_dev *indio_dev;
> +	struct fxas21002c_data *data;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	i2c_set_clientdata(client, indio_dev);
> +	data = iio_priv(indio_dev);
> +	data->client = client;
> +
> +	data->regmap = devm_regmap_init_i2c(client, &fxas21002c_regmap_config);
> +	if (IS_ERR(data->regmap)) {
> +		ret = PTR_ERR(data->regmap);
> +		dev_err(&client->dev,
> +			"Failed to allocate regmap, err: %d\n", ret);
> +		return ret;
> +	}
> +
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->channels = fxas21002c_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(fxas21002c_channels);
> +	indio_dev->name = id->name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &fxas21002c_info;
> +
> +	ret = fxas21002c_verify_chip(data);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = fxas21002c_reset(data);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = fxas21002c_set_operating_mode(data, FXAS21002C_OM_ACTIVE);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Failed to register iio device\n");
> +		fxas21002c_set_operating_mode(data, FXAS21002C_OM_STANDBY);
> +		return ret;

We may potentially get a patch as one of the static analysis tools will point
out that return ret could be dropped out of the brackets without changing the
code (ret is 0 in this case as iio_device_register never returns positive)

Really minor though!

> +	}
> +
> +	return 0;
> +}
> +
> +static int fxas21002c_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct fxas21002c_data *data = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +
> +	fxas21002c_set_operating_mode(data, FXAS21002C_OM_STANDBY);
You could have used the devm_add_action to allow the managed cleanup to handle
this and hence gotten rid of the remove function.

(minor suggestion and somewhat a matter of personal taste).

> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id fxas21002c_id[] = {
> +	{"fxas21002c", ID_FXAS21002C},
> +	{}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, fxas21002c_id);
> +
> +static struct i2c_driver fxas21002c_driver = {
> +	.driver = {
> +		.name = FXAS21002C_DRV_NAME,
> +	},
> +	.probe		= fxas21002c_probe,
> +	.remove		= fxas21002c_remove,
> +	.id_table	= fxas21002c_id,
> +};
> +
> +module_i2c_driver(fxas21002c_driver);
> +
> +MODULE_AUTHOR("Afonso Bordado <afonsobordado@az8.co>");
> +MODULE_DESCRIPTION("FXAS21002C Digital Angular Rate Gyroscope driver");
> +MODULE_LICENSE("GPL v2");


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

* Re: [PATCH 2/4] iio: gyro: add device tree support for fxas21002c
  2018-08-25 21:19 ` [PATCH 2/4] iio: gyro: add device tree " Afonso Bordado
@ 2018-08-27 17:13   ` Jonathan Cameron
  2018-08-29  6:43     ` Afonso Bordado
  0 siblings, 1 reply; 12+ messages in thread
From: Jonathan Cameron @ 2018-08-27 17:13 UTC (permalink / raw)
  To: Afonso Bordado
  Cc: knaack.h, lars, pmeerw, linux-iio, linux-kernel, devicetree

On Sat, 25 Aug 2018 22:19:08 +0100
Afonso Bordado <afonsobordado@az8.co> wrote:

> This patch adds device tree support for the fxas21002c driver, including
> bindings.
> 
> Signed-off-by: Afonso Bordado <afonsobordado@az8.co>
Now, the devicetree bindings should not reflect just what the driver
uses right now, but rather describe the hardware.

There are interrupts on there for starters that definitely want to be
described from the start.  Also there is a reset line that should probably
be here from the start.

Potentially also the two power supplies though that's less critical
(nice to have though)

It is also an i2c and spi part though that can probably be added later as
we can argue we are only documenting the bindings for the device in i2c mode
for now.

So what is here is fine, but I think we need to describe more.

It's all well understood details of how it is connected so no need to
have tested it with a driver to be sure it will be right.

Jonathan

> ---
>  .../bindings/iio/gyroscope/fsl,fxas21002c.txt        | 12 ++++++++++++
>  drivers/iio/gyro/fxas21002c.c                        | 10 +++++++++-
>  2 files changed, 21 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/iio/gyroscope/fsl,fxas21002c.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/gyroscope/fsl,fxas21002c.txt b/Documentation/devicetree/bindings/iio/gyroscope/fsl,fxas21002c.txt
> new file mode 100644
> index 000000000000..62f8c1bad85a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/gyroscope/fsl,fxas21002c.txt
> @@ -0,0 +1,12 @@
> +* Freescale FXAS21002C Digital Angular Rate Gyroscope
> +
> +Required properties:
> +
> +  - compatible: must be "fsl,fxas21002c"
> +  - reg : the I2C address of the sensor
> +
> +Example:
> +gyroscope@0 {
> +       compatible = "fsl,fxas21002c";
> +       reg = <0x20>;
> +};
> diff --git a/drivers/iio/gyro/fxas21002c.c b/drivers/iio/gyro/fxas21002c.c
> index 7626b2f88d72..6fef210630e0 100644
> --- a/drivers/iio/gyro/fxas21002c.c
> +++ b/drivers/iio/gyro/fxas21002c.c
> @@ -8,7 +8,6 @@
>   * Datasheet: https://www.nxp.com/docs/en/data-sheet/FXAS21002.pdf
>   * TODO:
>   *        ODR / Scale Support
> - *        Devicetree
>   *        Power management
>   *        LowPass/HighPass Filters
>   *        Buffers
> @@ -340,6 +339,14 @@ static int fxas21002c_remove(struct i2c_client *client)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id fxas21002c_of_ids[] = {
> +	{.compatible = "fsl,fxas21002c"},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, fxas21002c_of_ids);
> +#endif
> +
>  static const struct i2c_device_id fxas21002c_id[] = {
>  	{"fxas21002c", ID_FXAS21002C},
>  	{}
> @@ -350,6 +357,7 @@ MODULE_DEVICE_TABLE(i2c, fxas21002c_id);
>  static struct i2c_driver fxas21002c_driver = {
>  	.driver = {
>  		.name = FXAS21002C_DRV_NAME,
> +		.of_match_table = of_match_ptr(fxas21002c_of_ids),
>  	},
>  	.probe		= fxas21002c_probe,
>  	.remove		= fxas21002c_remove,


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

* Re: [PATCH 3/4] iio: fxas21002c: add ODR/Scale support
  2018-08-25 21:19 ` [PATCH 3/4] iio: fxas21002c: add ODR/Scale support Afonso Bordado
@ 2018-08-27 17:18   ` Jonathan Cameron
  0 siblings, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2018-08-27 17:18 UTC (permalink / raw)
  To: Afonso Bordado
  Cc: knaack.h, lars, pmeerw, linux-iio, linux-kernel, devicetree

On Sat, 25 Aug 2018 22:19:09 +0100
Afonso Bordado <afonsobordado@az8.co> wrote:

> This patch adds support for reading/writing ODR/Scale
> 
> We don't support the scale boost modes.
> 
> Signed-off-by: Afonso Bordado <afonsobordado@az8.co>

A few trivial bits in here.

Jonathan

> ---
>  drivers/iio/gyro/fxas21002c.c | 162 +++++++++++++++++++++++++++++++---
>  1 file changed, 149 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/gyro/fxas21002c.c b/drivers/iio/gyro/fxas21002c.c
> index 6fef210630e0..dc0cb9848386 100644
> --- a/drivers/iio/gyro/fxas21002c.c
> +++ b/drivers/iio/gyro/fxas21002c.c
> @@ -7,7 +7,7 @@
>   * IIO driver for FXAS21002C (7-bit I2C slave address 0x20 or 0x21).
>   * Datasheet: https://www.nxp.com/docs/en/data-sheet/FXAS21002.pdf
>   * TODO:
> - *        ODR / Scale Support
> + *        Scale boost mode
>   *        Power management
>   *        LowPass/HighPass Filters
>   *        Buffers
> @@ -40,7 +40,10 @@
>  #define FXAS21002C_REG_F_EVENT         0x0A
>  #define FXAS21002C_REG_INT_SRC_FLAG    0x0B
>  #define FXAS21002C_REG_WHO_AM_I        0x0C
> +
>  #define FXAS21002C_REG_CTRL_REG0       0x0D
> +#define FXAS21002C_SCALE_MASK          (BIT(0) | BIT(1))

This is a 2 bit mask, not a direct combination of two
one bit different things, so better as GENMASK(1, 0) to
make that implicit.

> +
>  #define FXAS21002C_REG_RT_CFG          0x0E
>  #define FXAS21002C_REG_RT_SRC          0x0F
>  #define FXAS21002C_REG_RT_THS          0x10
> @@ -52,13 +55,12 @@
>  #define FXAS21002C_ACTIVE_BIT          BIT(1)
>  #define FXAS21002C_READY_BIT           BIT(0)
>  
> -#define FXAS21002C_REG_CTRL_REG2       0x14
> -#define FXAS21002C_REG_CTRL_REG3       0x15
> +#define FXAS21002C_ODR_SHIFT           2
> +#define FXAS21002C_ODR_MASK            (BIT(2) | BIT(3) | BIT(4))
>  
> -#define FXAS21002C_DEFAULT_ODR_HZ      800
>  
> -// 0.0625 deg/s
> -#define FXAS21002C_DEFAULT_SENSITIVITY IIO_DEGREE_TO_RAD(62500)
> +#define FXAS21002C_REG_CTRL_REG2       0x14
> +#define FXAS21002C_REG_CTRL_REG3       0x15
>  
>  enum fxas21002c_id {
>  	ID_FXAS21002C,
> @@ -76,6 +78,40 @@ struct fxas21002c_data {
>  	struct regmap *regmap;
>  };
>  
> +enum fxas21002c_scale {
> +	FXAS21002C_SCALE_62MDPS,
> +	FXAS21002C_SCALE_31MDPS,
> +	FXAS21002C_SCALE_15MDPS,
> +	FXAS21002C_SCALE_7MDPS,
> +};
> +
> +static const int fxas21002c_anglevel_scale_avail[4][2] = {
> +	[FXAS21002C_SCALE_62MDPS] = { 0, IIO_DEGREE_TO_RAD(62500) },
> +	[FXAS21002C_SCALE_31MDPS] = { 0, IIO_DEGREE_TO_RAD(31250) },
> +	[FXAS21002C_SCALE_15MDPS] = { 0, IIO_DEGREE_TO_RAD(15625) },
> +	[FXAS21002C_SCALE_7MDPS]  = { 0, IIO_DEGREE_TO_RAD(7812) },
> +};
> +
> +enum fxas21002c_odr {
> +	FXAS21002C_ODR_800,
> +	FXAS21002C_ODR_400,
> +	FXAS21002C_ODR_200,
> +	FXAS21002C_ODR_100,
> +	FXAS21002C_ODR_50,
> +	FXAS21002C_ODR_25,
> +	FXAS21002C_ODR_12_5,
> +};
> +
> +static const int fxas21002c_sample_freq_avail[7][2] = {
> +	[FXAS21002C_ODR_800]  = { 800, 0 },
> +	[FXAS21002C_ODR_400]  = { 400, 0 },
> +	[FXAS21002C_ODR_200]  = { 200, 0 },
> +	[FXAS21002C_ODR_100]  = { 100, 0 },
> +	[FXAS21002C_ODR_50]   = { 50, 0 },
> +	[FXAS21002C_ODR_25]   = { 25, 0 },
> +	[FXAS21002C_ODR_12_5] = { 12, 500000 },
> +};
> +
>  static const struct regmap_range fxas21002c_writable_ranges[] = {
>  	regmap_reg_range(FXAS21002C_REG_F_SETUP, FXAS21002C_REG_F_SETUP),
>  	regmap_reg_range(FXAS21002C_REG_CTRL_REG0, FXAS21002C_REG_RT_CFG),
> @@ -242,6 +278,47 @@ static int fxas21002c_read_oneshot(struct fxas21002c_data *data,
>  	return IIO_VAL_INT;
>  }
>  
> +static int fxas21002c_scale_read(struct fxas21002c_data *data, int *val,
> +				 int *val2)
> +{
> +	int ret;
> +	unsigned int raw;
> +
> +	ret = regmap_read(data->regmap, FXAS21002C_REG_CTRL_REG0, &raw);
> +	if (ret)
> +		return ret;
> +
> +	raw &= FXAS21002C_SCALE_MASK;
> +
> +	*val = fxas21002c_anglevel_scale_avail[raw][0];
> +	*val2 = fxas21002c_anglevel_scale_avail[raw][1];
> +
> +	return IIO_VAL_INT_PLUS_MICRO;
> +}
> +
> +static int fxas21002c_odr_read(struct fxas21002c_data *data, int *val,
> +			       int *val2)
> +{
> +	int ret;
> +	unsigned int raw;
> +
> +	ret = regmap_read(data->regmap, FXAS21002C_REG_CTRL_REG1, &raw);
> +	if (ret)
> +		return ret;
> +
> +	raw = (raw & FXAS21002C_ODR_MASK) >> FXAS21002C_ODR_SHIFT;
> +
> +	// We don't use this mode but according to the datasheet its
> +	// also a 12.5Hz
/*
 * We...
 * also..
 */

The kernel style is very fussy about comment syntax.  It may seem silly but when
you read a lot of code these little thing being consistent help.

> +	if (raw == 7)
> +		raw = FXAS21002C_ODR_12_5;
> +
> +	*val = fxas21002c_sample_freq_avail[raw][0];
> +	*val2 = fxas21002c_sample_freq_avail[raw][1];
> +
> +	return IIO_VAL_INT_PLUS_MICRO;
> +}
> +
>  static int fxas21002c_read_raw(struct iio_dev *indio_dev,
>  			       struct iio_chan_spec const *chan, int *val,
>  			       int *val2, long mask)
> @@ -255,24 +332,83 @@ static int fxas21002c_read_raw(struct iio_dev *indio_dev,
>  		if (chan->type != IIO_ANGL_VEL)
>  			return -EINVAL;
>  
> -		*val = 0;
> -		*val2 = FXAS21002C_DEFAULT_SENSITIVITY;
> -
> -		return IIO_VAL_INT_PLUS_MICRO;
> +		return fxas21002c_scale_read(data, val, val2);
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		if (chan->type != IIO_ANGL_VEL)
>  			return -EINVAL;
>  
> -		*val = FXAS21002C_DEFAULT_ODR_HZ;
> -
> -		return IIO_VAL_INT;
> +		return fxas21002c_odr_read(data, val, val2);
>  	}
>  
>  	return -EINVAL;
>  }
>  
> +static int fxas21002c_write_raw(struct iio_dev *indio_dev,
> +				struct iio_chan_spec const *chan, int val,
> +				int val2, long mask)
> +{
> +	struct fxas21002c_data *data = iio_priv(indio_dev);
> +	int ret = -EINVAL;
> +	int i;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		for (i = 0; i < ARRAY_SIZE(fxas21002c_sample_freq_avail); i++) {
> +			if (fxas21002c_sample_freq_avail[i][0] == val &&
> +			    fxas21002c_sample_freq_avail[i][1] == val2)
> +				break;
> +		}
> +
> +		if (i == ARRAY_SIZE(fxas21002c_sample_freq_avail))
> +			break;
> +
> +		ret = regmap_update_bits(data->regmap, FXAS21002C_REG_CTRL_REG1,
return regmap_update_bits...

> +					 FXAS21002C_ODR_MASK,
> +					 i << FXAS21002C_ODR_SHIFT);
> +
> +		break;
> +	case IIO_CHAN_INFO_SCALE:
> +		for (i = 0; i < ARRAY_SIZE(fxas21002c_anglevel_scale_avail);
> +		     i++) {
> +			if (fxas21002c_anglevel_scale_avail[i][0] == val &&
> +			    fxas21002c_anglevel_scale_avail[i][1] == val2)
> +				break;
> +		}
> +
> +		if (i == ARRAY_SIZE(fxas21002c_anglevel_scale_avail))
> +			break;
> +
> +		ret = regmap_update_bits(data->regmap, FXAS21002C_REG_CTRL_REG0,
return regmap_update_bits.
> +					 FXAS21002C_SCALE_MASK, i);
> +
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static IIO_CONST_ATTR(anglevel_scale_available,
> +		      "0.001090831 "  // 62.5    mdps
/* ... */
> +		      "0.000545415 "  // 31.25   mdps
> +		      "0.000272708 "  // 15.625  mdps
> +		      "0.000136354"); //  7.8125 mdps
> +
> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("800 400 200 100 50 25 12.5");
> +
> +static struct attribute *fxas21002c_attributes[] = {
> +	&iio_const_attr_anglevel_scale_available.dev_attr.attr,
> +	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group fxas21002c_attribute_group = {
> +	.attrs = fxas21002c_attributes,
> +};
> +
>  static const struct iio_info fxas21002c_info = {
>  	.read_raw		= fxas21002c_read_raw,
> +	.write_raw              = fxas21002c_write_raw,
> +	.attrs                  = &fxas21002c_attribute_group,
>  };
>  
>  static int fxas21002c_probe(struct i2c_client *client,


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

* Re: [PATCH 1/4] iio: gyro: add support for fxas21002c
  2018-08-27 17:08 ` [PATCH 1/4] iio: gyro: add support for fxas21002c Jonathan Cameron
@ 2018-08-27 20:50   ` Joe Perches
  2018-08-29  6:43   ` Afonso Bordado
  1 sibling, 0 replies; 12+ messages in thread
From: Joe Perches @ 2018-08-27 20:50 UTC (permalink / raw)
  To: Jonathan Cameron, Afonso Bordado
  Cc: knaack.h, lars, pmeerw, linux-iio, linux-kernel, devicetree

On Mon, 2018-08-27 at 18:08 +0100, Jonathan Cameron wrote:
> On Sat, 25 Aug 2018 22:19:07 +0100
> Afonso Bordado <afonsobordado@az8.co> wrote:
> 
> > FXAS21002C is a 3 axis gyroscope with integrated temperature sensor
> > 
> > Signed-off-by: Afonso Bordado <afonsobordado@az8.co>
> 
> Hi,
> 
> Driver is pretty clean so only a few minor comments inline.
> If we were late in a cycle I'd probably just have taken it and fixed up
> but as we have lots of time and I'm inherently lazy I'll let you do a v2 :)
> 
> Good job, thanks!
[]
> > diff --git a/drivers/iio/gyro/fxas21002c.c b/drivers/iio/gyro/fxas21002c.c
[]
> /* ... */
> 
> Please run checkpatch as IIRC it complains about this.

Running checkpatch and paying attention to what it bleats
is generally a good idea, as long as its bleats aren't
mindlessly followed.

And c99 style // comments have been ignored since:

commit dadf680de3c2eb4cba9840619991eda0cfe98778
Author: Joe Perches <joe@perches.com>
Date:   Tue Aug 2 14:04:33 2016 -0700

    checkpatch: allow c99 style // comments
    
    Sanitise the lines that contain c99 comments so that the error doesn't
    get emitted.


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

* Re: [PATCH 1/4] iio: gyro: add support for fxas21002c
  2018-08-27 17:08 ` [PATCH 1/4] iio: gyro: add support for fxas21002c Jonathan Cameron
  2018-08-27 20:50   ` Joe Perches
@ 2018-08-29  6:43   ` Afonso Bordado
  2018-09-02  9:18     ` Jonathan Cameron
  1 sibling, 1 reply; 12+ messages in thread
From: Afonso Bordado @ 2018-08-29  6:43 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: knaack.h, lars, pmeerw, linux-iio, linux-kernel, devicetree

On Mon, 2018-08-27 at 18:08 +0100, Jonathan Cameron wrote:
> On Sat, 25 Aug 2018 22:19:07 +0100
> Afonso Bordado <afonsobordado@az8.co> wrote:
> 
> > FXAS21002C is a 3 axis gyroscope with integrated temperature sensor
> > 
> > Signed-off-by: Afonso Bordado <afonsobordado@az8.co>
> 
> Hi,
> 
> Driver is pretty clean so only a few minor comments inline.
> If we were late in a cycle I'd probably just have taken it and fixed
> up
> but as we have lots of time and I'm inherently lazy I'll let you do a
> v2 :)
> 
> Good job, thanks!
> 
> Jonathan

Great!


> > +
> > +static const struct regmap_access_table fxas21002c_volatile_table
> > = {
> > +	.yes_ranges = fxas21002c_volatile_ranges,
> > +	.n_yes_ranges = ARRAY_SIZE(fxas21002c_volatile_ranges),
> > +};
> > +
> > +const struct regmap_config fxas21002c_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +
> > +	.max_register = FXAS21002C_REG_CTRL_REG3,
> > +	// We don't specify a .rd_table because everything is readable
> 
> /* ... */
> 
> Please run checkpatch as IIRC it complains about this.

I've replaced all instances of C99 comments with ANSI comments.
However, has Joe Perches mentioned. Checkpatch did not warn me about
this.

> > +	.wr_table = &fxas21002c_writable_table,
> > +	.volatile_table = &fxas21002c_volatile_table,
> > +};
> > +EXPORT_SYMBOL(fxas21002c_regmap_config);
> > +
> > +#define FXAS21002C_GYRO_CHAN(_axis) {				
> > 	\
> > +	.type = IIO_ANGL_VEL,						
> > \
> > +	.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 = FXAS21002C_REG_OUT_ ## _axis ## _MSB,		\
> > +}
> > +
> > +static const struct iio_chan_spec fxas21002c_channels[] = {
> > +	{
> > +		.type = IIO_TEMP,
> > +		.address = FXAS21002C_REG_TEMP,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> 
> As it currently stands it is IIO_CHAN_PROCESSED but I'd prefer you
> provided
> the scale and kept it as _RAW.

Changed in v2. I'll provide scale + raw.

> > +	},
> > +	FXAS21002C_GYRO_CHAN(X),
> > +	FXAS21002C_GYRO_CHAN(Y),
> > +	FXAS21002C_GYRO_CHAN(Z),
> > +	IIO_CHAN_SOFT_TIMESTAMP(3),
> > +};
> > +
> > +static int fxas21002c_set_operating_mode(struct fxas21002c_data
> > *data,
> > +					 enum fxas21002c_operating_mode
> > om)
> > +{
> > +	int ret;
> > +	int mask = 0;
> 
> Might be clearer to not set this here and...
> > +
> > +	switch (om) {
> > +	case FXAS21002C_OM_STANDBY:
> 
> 		mask = 0;
> > +		break;
> > +	case FXAS21002C_OM_READY:
> > +		mask |= FXAS21002C_READY_BIT;
> 
> 		mask = FXA210002C_READY_BIT;
> > +		break;
> > +	case FXAS21002C_OM_ACTIVE:
> > +		mask |= FXAS21002C_ACTIVE_BIT;
> 
> 		mask = FXA21002C_ACTIVE_BIT;
> 
> Slightly more readable I think...

I Agree.

> > +static int fxas21002c_read_oneshot(struct fxas21002c_data *data,
> > +				   struct iio_chan_spec const *chan,
> > int *val)
> > +{
> > +	int ret;
> > +	int raw;
> > +	__be16 bulk_raw;
> > +
> > +	switch (chan->type) {
> > +	case IIO_ANGL_VEL:
> > +		ret = regmap_bulk_read(data->regmap, chan->address,
> > +				       &bulk_raw, sizeof(bulk_raw));
> > +		if (ret)
> > +			return ret;
> > +
> > +		*val = sign_extend32(be16_to_cpu(bulk_raw), 15);
> > +		break;
> 
> return IIO_VAL_INT directly here.

Sure

> > +	case IIO_TEMP:
> > +		ret = regmap_read(data->regmap, chan->address, &raw);
> > +		if (ret)
> > +			return ret;
> > +
> > +		*val = raw * 1000; // Convert to millicelsius
> 
> Don't use c++ style comments in kernel code please.
> 
> Also I wouldn't do this multiplier in here, I'd provide the scale
> attribute.
> The reason is that we may later have support for buffered reads from
> this
> device - at that point we will want to have nice 8 bit data rather
> than having
> a scaled value that isn't quite a whole number of bits.

This makes sense, I'll put in the SCALE bit for temp.

> > +		break;
> 
> return IIO_VAL_INT; directly here.
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> 
> With the above two changes in place this return is never reached.

Changed in v2.

> > +	return IIO_VAL_INT;
> > +}
> > +
> > +static int fxas21002c_read_raw(struct iio_dev *indio_dev,
> > +			       struct iio_chan_spec const *chan, int
> > *val,
> > +			       int *val2, long mask)
> > +{
> > +	struct fxas21002c_data *data = iio_priv(indio_dev);
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		return fxas21002c_read_oneshot(data, chan, val);
> > +	case IIO_CHAN_INFO_SCALE:
> > +		if (chan->type != IIO_ANGL_VEL)
> > +			return -EINVAL;
> 
> This check is a bit over paranoid given there is no way
> this would be called in with a different type.
> 
> Doesn't do any real harm though.

This no longer applies with the temp changes included in v2.
However, i'd still like to leave it for IIO_CHAN_INFO_SAMP_FREQ.

> > +
> > +		*val = 0;
> > +		*val2 = FXAS21002C_DEFAULT_SENSITIVITY;
> > +
> > +		return IIO_VAL_INT_PLUS_MICRO;
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		if (chan->type != IIO_ANGL_VEL)
> > +			return -EINVAL;
> > +
> > +		*val = FXAS21002C_DEFAULT_ODR_HZ;
> > +
> > +		return IIO_VAL_INT;
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static const struct iio_info fxas21002c_info = {
> > +	.read_raw		= fxas21002c_read_raw,
> > +};
> > +
> > +static int fxas21002c_probe(struct i2c_client *client,
> > +			    const struct i2c_device_id *id)
> > +{
> > +	int ret;
> > +	struct iio_dev *indio_dev;
> > +	struct fxas21002c_data *data;
> > +
> > +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	i2c_set_clientdata(client, indio_dev);
> > +	data = iio_priv(indio_dev);
> > +	data->client = client;
> > +
> > +	data->regmap = devm_regmap_init_i2c(client,
> > &fxas21002c_regmap_config);
> > +	if (IS_ERR(data->regmap)) {
> > +		ret = PTR_ERR(data->regmap);
> > +		dev_err(&client->dev,
> > +			"Failed to allocate regmap, err: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	indio_dev->dev.parent = &client->dev;
> > +	indio_dev->channels = fxas21002c_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(fxas21002c_channels);
> > +	indio_dev->name = id->name;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->info = &fxas21002c_info;
> > +
> > +	ret = fxas21002c_verify_chip(data);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = fxas21002c_reset(data);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = fxas21002c_set_operating_mode(data,
> > FXAS21002C_OM_ACTIVE);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = iio_device_register(indio_dev);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "Failed to register iio
> > device\n");
> > +		fxas21002c_set_operating_mode(data,
> > FXAS21002C_OM_STANDBY);
> > +		return ret;
> 
> We may potentially get a patch as one of the static analysis tools
> will point
> out that return ret could be dropped out of the brackets without
> changing the
> code (ret is 0 in this case as iio_device_register never returns
> positive)
> 
> Really minor though!

Will be fixed in V2.


> > +
> > +static int fxas21002c_remove(struct i2c_client *client)
> > +{
> > +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > +	struct fxas21002c_data *data = iio_priv(indio_dev);
> > +
> > +	iio_device_unregister(indio_dev);
> > +
> > +	fxas21002c_set_operating_mode(data, FXAS21002C_OM_STANDBY);
> 
> You could have used the devm_add_action to allow the managed cleanup
> to handle
> this and hence gotten rid of the remove function.
> 
> (minor suggestion and somewhat a matter of personal taste).

I didn't know this existed! Changed in v2.




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

* Re: [PATCH 2/4] iio: gyro: add device tree support for fxas21002c
  2018-08-27 17:13   ` Jonathan Cameron
@ 2018-08-29  6:43     ` Afonso Bordado
  2018-09-02  9:15       ` Jonathan Cameron
  0 siblings, 1 reply; 12+ messages in thread
From: Afonso Bordado @ 2018-08-29  6:43 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: knaack.h, lars, pmeerw, linux-iio, linux-kernel, devicetree

On Mon, 2018-08-27 at 18:13 +0100, Jonathan Cameron wrote:
> On Sat, 25 Aug 2018 22:19:08 +0100
> Afonso Bordado <afonsobordado@az8.co> wrote:
> 
> > This patch adds device tree support for the fxas21002c driver,
> > including
> > bindings.
> > 
> > Signed-off-by: Afonso Bordado <afonsobordado@az8.co>
> 
> Now, the devicetree bindings should not reflect just what the driver
> uses right now, but rather describe the hardware.
> 
> There are interrupts on there for starters that definitely want to be
> described from the start.  Also there is a reset line that should
> probably
> be here from the start.
> 
> Potentially also the two power supplies though that's less critical
> (nice to have though)
> 
> It is also an i2c and spi part though that can probably be added
> later as
> we can argue we are only documenting the bindings for the device in
> i2c mode
> for now.
> 
> So what is here is fine, but I think we need to describe more.
> 
> It's all well understood details of how it is connected so no need to
> have tested it with a driver to be sure it will be right.
> 
> Jonathan

Ok, so i'm thinking about adding both interrupts, the reset line and
the regulators. If i say they are optional in the device tree document,
i shouldn't need to add any more code immediately and could just
implement support for it later right?



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

* Re: [PATCH 2/4] iio: gyro: add device tree support for fxas21002c
  2018-08-29  6:43     ` Afonso Bordado
@ 2018-09-02  9:15       ` Jonathan Cameron
  0 siblings, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2018-09-02  9:15 UTC (permalink / raw)
  To: Afonso Bordado
  Cc: knaack.h, lars, pmeerw, linux-iio, linux-kernel, devicetree

On Wed, 29 Aug 2018 07:43:53 +0100
Afonso Bordado <afonsobordado@az8.co> wrote:

> On Mon, 2018-08-27 at 18:13 +0100, Jonathan Cameron wrote:
> > On Sat, 25 Aug 2018 22:19:08 +0100
> > Afonso Bordado <afonsobordado@az8.co> wrote:
> >   
> > > This patch adds device tree support for the fxas21002c driver,
> > > including
> > > bindings.
> > > 
> > > Signed-off-by: Afonso Bordado <afonsobordado@az8.co>  
> > 
> > Now, the devicetree bindings should not reflect just what the driver
> > uses right now, but rather describe the hardware.
> > 
> > There are interrupts on there for starters that definitely want to be
> > described from the start.  Also there is a reset line that should
> > probably
> > be here from the start.
> > 
> > Potentially also the two power supplies though that's less critical
> > (nice to have though)
> > 
> > It is also an i2c and spi part though that can probably be added
> > later as
> > we can argue we are only documenting the bindings for the device in
> > i2c mode
> > for now.
> > 
> > So what is here is fine, but I think we need to describe more.
> > 
> > It's all well understood details of how it is connected so no need to
> > have tested it with a driver to be sure it will be right.
> > 
> > Jonathan  
> 
> Ok, so i'm thinking about adding both interrupts, the reset line and
> the regulators. If i say they are optional in the device tree document,
> i shouldn't need to add any more code immediately and could just
> implement support for it later right?
> 
Absolutely, as long as you are happy to keep them optional as you
add the features to the driver.

Regulators make that easy by providing stubs (as long as you don't
need to read their voltages).  Reset and interrupts will need to be
handled explicitly by the driver.

Thanks,

Jonathan
> 


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

* Re: [PATCH 1/4] iio: gyro: add support for fxas21002c
  2018-08-29  6:43   ` Afonso Bordado
@ 2018-09-02  9:18     ` Jonathan Cameron
  0 siblings, 0 replies; 12+ messages in thread
From: Jonathan Cameron @ 2018-09-02  9:18 UTC (permalink / raw)
  To: Afonso Bordado
  Cc: knaack.h, lars, pmeerw, linux-iio, linux-kernel, devicetree

On Wed, 29 Aug 2018 07:43:48 +0100
Afonso Bordado <afonsobordado@az8.co> wrote:

> On Mon, 2018-08-27 at 18:08 +0100, Jonathan Cameron wrote:
> > On Sat, 25 Aug 2018 22:19:07 +0100
> > Afonso Bordado <afonsobordado@az8.co> wrote:
> >   
> > > FXAS21002C is a 3 axis gyroscope with integrated temperature sensor
> > > 
> > > Signed-off-by: Afonso Bordado <afonsobordado@az8.co>  
> > 
> > Hi,
> > 
> > Driver is pretty clean so only a few minor comments inline.
> > If we were late in a cycle I'd probably just have taken it and fixed
> > up
> > but as we have lots of time and I'm inherently lazy I'll let you do a
> > v2 :)
> > 
> > Good job, thanks!
> > 
> > Jonathan  
> 
> Great!
> 
> 
> > > +
> > > +static const struct regmap_access_table fxas21002c_volatile_table
> > > = {
> > > +	.yes_ranges = fxas21002c_volatile_ranges,
> > > +	.n_yes_ranges = ARRAY_SIZE(fxas21002c_volatile_ranges),
> > > +};
> > > +
> > > +const struct regmap_config fxas21002c_regmap_config = {
> > > +	.reg_bits = 8,
> > > +	.val_bits = 8,
> > > +
> > > +	.max_register = FXAS21002C_REG_CTRL_REG3,
> > > +	// We don't specify a .rd_table because everything is readable  
> > 
> > /* ... */
> > 
> > Please run checkpatch as IIRC it complains about this.  
> 
> I've replaced all instances of C99 comments with ANSI comments.
> However, has Joe Perches mentioned. Checkpatch did not warn me about
> this.
> 
Yup, thanks to Joe for clarifying this I had missed the change.
> 
> > > +
> > > +static int fxas21002c_remove(struct i2c_client *client)
> > > +{
> > > +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > > +	struct fxas21002c_data *data = iio_priv(indio_dev);
> > > +
> > > +	iio_device_unregister(indio_dev);
> > > +
> > > +	fxas21002c_set_operating_mode(data, FXAS21002C_OM_STANDBY);  
> > 
> > You could have used the devm_add_action to allow the managed cleanup
> > to handle
> > this and hence gotten rid of the remove function.
> > 
> > (minor suggestion and somewhat a matter of personal taste).  
> 
> I didn't know this existed! Changed in v2.

Nor me until someone used it in a driver a few months back ;)
One of the advantages of doing a lot of review is that you get to see
any new stuff other people use.

Jonathan

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

end of thread, other threads:[~2018-09-02  9:18 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-25 21:19 [PATCH 1/4] iio: gyro: add support for fxas21002c Afonso Bordado
2018-08-25 21:19 ` [PATCH 2/4] iio: gyro: add device tree " Afonso Bordado
2018-08-27 17:13   ` Jonathan Cameron
2018-08-29  6:43     ` Afonso Bordado
2018-09-02  9:15       ` Jonathan Cameron
2018-08-25 21:19 ` [PATCH 3/4] iio: fxas21002c: add ODR/Scale support Afonso Bordado
2018-08-27 17:18   ` Jonathan Cameron
2018-08-25 21:19 ` [PATCH 4/4] MAINTAINERS: add entry for fxas21002c gyro driver Afonso Bordado
2018-08-27 17:08 ` [PATCH 1/4] iio: gyro: add support for fxas21002c Jonathan Cameron
2018-08-27 20:50   ` Joe Perches
2018-08-29  6:43   ` Afonso Bordado
2018-09-02  9:18     ` Jonathan Cameron

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).