Re: [PATCH 1/4] iio: gyro: add support for fxas21002c

From: Jonathan Cameron
Date: Mon Aug 27 2018 - 13:08:17 EST

Next message: Jan-Marek Glogowski: "Re: [PATCH v4] drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse""
Previous message: Jiri Olsa: "[PATCHv3] perf tools: Add struct ordered_events_buffer layer"
In reply to: Afonso Bordado: "[PATCH 4/4] MAINTAINERS: add entry for fxas21002c gyro driver"
Next in thread: Joe Perches: "Re: [PATCH 1/4] iio: gyro: add support for fxas21002c"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

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

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@xxxxxx>
> + *
> + * 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@xxxxxx>");
> +MODULE_DESCRIPTION("FXAS21002C Digital Angular Rate Gyroscope driver");
> +MODULE_LICENSE("GPL v2");

Next message: Jan-Marek Glogowski: "Re: [PATCH v4] drm/i915: Re-apply "Perform link quality check, unconditionally during long pulse""
Previous message: Jiri Olsa: "[PATCHv3] perf tools: Add struct ordered_events_buffer layer"
In reply to: Afonso Bordado: "[PATCH 4/4] MAINTAINERS: add entry for fxas21002c gyro driver"
Next in thread: Joe Perches: "Re: [PATCH 1/4] iio: gyro: add support for fxas21002c"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]