* [PATCH v2 0/4] Add driver for wsen-itds accelerometer sensor @ 2020-04-29 13:39 Saravanan Sekar 2020-04-29 13:39 ` [PATCH v2 1/4] dt-bindings: iio: add document bindings for wsen-itds accel sensor Saravanan Sekar ` (3 more replies) 0 siblings, 4 replies; 10+ messages in thread From: Saravanan Sekar @ 2020-04-29 13:39 UTC (permalink / raw) To: robh+dt, jic23, knaack.h, lars, pmeerw, broonie, lgirdwood, saravanan Cc: devicetree, linux-kernel, linux-iio changes in v2: - added ABI document - fixed review comments in v1 - removed vendor entry changes (merged in v1) The patch series add support for wurth elektronic wsen-itds 3-axis accelerometer. Driver supports the acceleration, temperature data reading and supports configuring of output data rate, operating mode and scale. Thanks, Saravanan Saravanan Sekar (4): dt-bindings: iio: add document bindings for wsen-itds accel sensor iio: accel: Add driver for wsen-itds accelerometer sensor iio: accel: wsen-itds accel documentation MAINTAINERS: Add entry for wsen-itds accelerometer sensor .../ABI/testing/sysfs-bus-iio-wsen-itds | 23 + .../bindings/iio/accel/we,wsen-itds.yaml | 55 + MAINTAINERS | 7 + drivers/iio/accel/Kconfig | 14 + drivers/iio/accel/Makefile | 1 + drivers/iio/accel/wsen-itds.c | 947 ++++++++++++++++++ 6 files changed, 1047 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-wsen-itds create mode 100644 Documentation/devicetree/bindings/iio/accel/we,wsen-itds.yaml create mode 100644 drivers/iio/accel/wsen-itds.c -- 2.17.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/4] dt-bindings: iio: add document bindings for wsen-itds accel sensor 2020-04-29 13:39 [PATCH v2 0/4] Add driver for wsen-itds accelerometer sensor Saravanan Sekar @ 2020-04-29 13:39 ` Saravanan Sekar 2020-05-12 14:45 ` Rob Herring 2020-04-29 13:39 ` [PATCH v2 2/4] iio: accel: Add driver for wsen-itds accelerometer sensor Saravanan Sekar ` (2 subsequent siblings) 3 siblings, 1 reply; 10+ messages in thread From: Saravanan Sekar @ 2020-04-29 13:39 UTC (permalink / raw) To: robh+dt, jic23, knaack.h, lars, pmeerw, broonie, lgirdwood, saravanan Cc: devicetree, linux-kernel, linux-iio [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset=y, Size: 1997 bytes --] Add device tree binding information for wsen-itds accel sensor driver. Signed-off-by: Saravanan Sekar <saravanan@linumiz.com> --- .../bindings/iio/accel/we,wsen-itds.yaml | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/accel/we,wsen-itds.yaml diff --git a/Documentation/devicetree/bindings/iio/accel/we,wsen-itds.yaml b/Documentation/devicetree/bindings/iio/accel/we,wsen-itds.yaml new file mode 100644 index 000000000000..eb01ed88091b --- /dev/null +++ b/Documentation/devicetree/bindings/iio/accel/we,wsen-itds.yaml @@ -0,0 +1,55 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/accel/we,wsen-itds.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Würth Elektronik WSEN-ITDS triaxial acceleration sensor + +maintainers: + - Saravanan Sekar <saravanan@linumiz.com> + +description: | + Acceleration and temperature iio sensor with an i2c interface. + The sensor provides additional application specific features like + tap detection, 6D orientation, Free-fall, Motion and Activity. + +properties: + compatible: + enum: + - we,wsen-itds + + reg: + maxItems: 1 + + vdd-supply: + description: phandle to the regulator that provides power to the accelerometer + + vddio-supply: + description: phandle to the regulator that provides power to the sensor's IO + + interrupts: + maxItems: 1 + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + i2c { + #address-cells = <1>; + #size-cells = <0>; + accelerometer@18 { + compatible = "we,wsen-itds"; + reg = <0x18>; + vdd-supply = <&vdd>; + vddio-supply = <&vddio>; + interrupt-parent = <&gpio0>; + interrupts = <0 IRQ_TYPE_LEVEL_HIGH>; + }; + }; -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/4] dt-bindings: iio: add document bindings for wsen-itds accel sensor 2020-04-29 13:39 ` [PATCH v2 1/4] dt-bindings: iio: add document bindings for wsen-itds accel sensor Saravanan Sekar @ 2020-05-12 14:45 ` Rob Herring 0 siblings, 0 replies; 10+ messages in thread From: Rob Herring @ 2020-05-12 14:45 UTC (permalink / raw) To: Saravanan Sekar Cc: robh+dt, linux-kernel, devicetree, linux-iio, knaack.h, pmeerw, jic23, broonie, lars, lgirdwood On Wed, 29 Apr 2020 15:39:40 +0200, Saravanan Sekar wrote: > Add device tree binding information for wsen-itds accel sensor driver. > > Signed-off-by: Saravanan Sekar <saravanan@linumiz.com> > --- > .../bindings/iio/accel/we,wsen-itds.yaml | 55 +++++++++++++++++++ > 1 file changed, 55 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/accel/we,wsen-itds.yaml > Reviewed-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/4] iio: accel: Add driver for wsen-itds accelerometer sensor 2020-04-29 13:39 [PATCH v2 0/4] Add driver for wsen-itds accelerometer sensor Saravanan Sekar 2020-04-29 13:39 ` [PATCH v2 1/4] dt-bindings: iio: add document bindings for wsen-itds accel sensor Saravanan Sekar @ 2020-04-29 13:39 ` Saravanan Sekar 2020-05-03 12:42 ` Jonathan Cameron 2020-04-29 13:39 ` [PATCH v2 3/4] iio: accel: wsen-itds accel documentation Saravanan Sekar 2020-04-29 13:39 ` [PATCH v2 4/4] MAINTAINERS: Add entry for wsen-itds accelerometer sensor Saravanan Sekar 3 siblings, 1 reply; 10+ messages in thread From: Saravanan Sekar @ 2020-04-29 13:39 UTC (permalink / raw) To: robh+dt, jic23, knaack.h, lars, pmeerw, broonie, lgirdwood, saravanan Cc: devicetree, linux-kernel, linux-iio [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset=y, Size: 25548 bytes --] Add an iio driver for the wurth elektronic wsen-itds 3-axis accelerometer. The driver supports the acceleration, temperature data reading and supports configuring of output data rate, operating mode and scale. Signed-off-by: Saravanan Sekar <saravanan@linumiz.com> --- drivers/iio/accel/Kconfig | 14 + drivers/iio/accel/Makefile | 1 + drivers/iio/accel/wsen-itds.c | 947 ++++++++++++++++++++++++++++++++++ 3 files changed, 962 insertions(+) create mode 100644 drivers/iio/accel/wsen-itds.c diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig index 5d91a6dda894..ad96ba2a9e38 100644 --- a/drivers/iio/accel/Kconfig +++ b/drivers/iio/accel/Kconfig @@ -450,4 +450,18 @@ config STK8BA50 Choosing M will build the driver as a module. If so, the module will be called stk8ba50. +config WSEN_ITDS + tristate "Wurth Elektronik WSEN-ITDS 3-Axis Accelerometer Driver" + depends on I2C + select IIO_BUFFER + select IIO_TRIGGERED_BUFFER + select REGMAP_I2C + help + Say yes here to build support for the WSEN-ITDS 3-axis + accelerometer. + + The sensor provides 3-axis Acceleration, Temperature data and + application specific features like tap, 6D Orinetation, Free-fall + Motion, Activity etc + endmenu diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile index 3a051cf37f40..942cab3e1d16 100644 --- a/drivers/iio/accel/Makefile +++ b/drivers/iio/accel/Makefile @@ -61,3 +61,4 @@ st_accel-$(CONFIG_IIO_BUFFER) += st_accel_buffer.o obj-$(CONFIG_IIO_ST_ACCEL_I2C_3AXIS) += st_accel_i2c.o obj-$(CONFIG_IIO_ST_ACCEL_SPI_3AXIS) += st_accel_spi.o +obj-$(CONFIG_WSEN_ITDS) += wsen-itds.o diff --git a/drivers/iio/accel/wsen-itds.c b/drivers/iio/accel/wsen-itds.c new file mode 100644 index 000000000000..d7bc961176c3 --- /dev/null +++ b/drivers/iio/accel/wsen-itds.c @@ -0,0 +1,947 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * WSEN-ITDS 3-axis accel sensor driver + * + * Copyright 2020 Linumiz + * + * Author: Saravanan Sekar <saravanan@linumiz.com> + * + * TODO : events - 6D, Orientation, Free-fall, Tap, Motion + */ + +#include <linux/cache.h> +#include <linux/device.h> +#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/iio/buffer.h> +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> +#include <linux/iio/trigger.h> +#include <linux/iio/trigger_consumer.h> +#include <linux/iio/triggered_buffer.h> +#include <linux/interrupt.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> +#include <linux/slab.h> + +#define ITDS_REG_TEMP_L 0x0d +#define ITDS_REG_DEV_ID 0x0f +#define ITDS_REG_CTRL1 0x20 +#define ITDS_REG_CTRL2 0x21 +#define ITDS_REG_CTRL3 0x22 +#define ITDS_REG_CTRL4 0x23 +#define ITDS_REG_CTRL5 0x24 +#define ITDS_REG_CTRL6 0x25 +#define ITDS_REG_STATUS 0x27 +#define ITDS_REG_X_OUT_L 0x28 +#define ITDS_REG_Y_OUT_L 0x2a +#define ITDS_REG_Z_OUT_L 0x2c +#define ITDS_REG_FIFO_CTRL 0x2e +#define ITDS_REG_FIFO_SAMPLES 0x2f +#define ITDS_REG_STATUS_DETECT 0x37 +#define ITDS_REG_WAKEUP_EVENT 0x38 +#define ITDS_REG_CTRL7 0x3f + +#define ITDS_MASK_SCALE GENMASK(5, 4) +#define ITDS_MASK_BDU_INC_ADD GENMASK(3, 2) +#define ITDS_MASK_FIFOTH GENMASK(4, 0) +#define ITDS_MASK_FIFOMODE GENMASK(7, 5) +#define ITDS_MASK_MODE GENMASK(3, 0) +#define ITDS_MASK_SAMPLES_COUNT GENMASK(5, 0) +#define ITDS_MASK_ODR GENMASK(7, 4) +#define ITDS_MASK_INT_DRDY BIT(0) +#define ITDS_MASK_INT_FIFOTH BIT(1) +#define ITDS_MASK_INT_EN BIT(5) + +#define ITDS_EVENT_DRDY BIT(0) +#define ITDS_EVENT_DRDY_T BIT(6) +#define ITDS_EVENT_FIFO_TH BIT(7) +#define ITDS_FIFO_MODE_BYPASS 0 +#define ITDS_FIFO_MODE_FIFO BIT(5) +#define ITDS_DEVICE_ID 0x44 +#define ITDS_ACCL_FIFO_SIZE 32 + +#define ITDS_ACC_CHAN(_axis, _idx) { \ + .type = IIO_ACCEL, \ + .modified = 1, \ + .channel2 = IIO_MOD_##_axis, \ + .address = ITDS_REG_##_axis##_OUT_L, \ + .scan_index = _idx, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \ + BIT(IIO_CHAN_INFO_SCALE), \ + .scan_type = { \ + .sign = 's', \ + .realbits = 14, \ + .storagebits = 16, \ + .shift = 2, \ + .endianness = IIO_LE, \ + }, \ +} + +enum itds_operating_mode { + ITDS_OP_MODE_LOW_POWER, + ITDS_OP_MODE_NORMAL = BIT(0), + ITDS_OP_MODE_HIGH_PERFORMANCE = BIT(1), + ITDS_OP_MODE_SINGLE_SHOT = BIT(2), + ITDS_OP_MODE_MAX = 4 +}; + +enum itds_odr { + ITDS_ODR_0, + ITDS_ODR_1_6, + ITDS_ODR_12_5, + ITDS_ODR_25, + ITDS_ODR_50, + ITDS_ODR_100, + ITDS_ODR_200, + ITDS_ODR_400, + ITDS_ODR_800, + ITDS_ODR_1600, + ITDS_ODR_MAX +}; + +struct itds_info { + struct device *dev; + struct regmap *regmap; + struct iio_trigger *trig; + struct regulator *vdd_supply; + struct regulator *vddio_supply; + struct mutex mutex; /* itds info struct member */ + int64_t timestamp; + int64_t old_timestamp; + unsigned int scale; + enum itds_operating_mode op_mode; + u8 fifo_threshold; + bool is_hwfifo_enabled; +}; + +struct itds_sample_freqency { + int val; + int val2; +}; + +static const char *operating_mode[ITDS_OP_MODE_MAX] = { + "lowpower", + "normal", + "high_perf", + "single_shot" +}; + +static const struct itds_sample_freqency itds_sample_freq[ITDS_ODR_MAX] = { + {0}, {1, 600000}, {12, 500000}, {25}, {50}, {100}, {200}, + {400}, {800}, {1600} +}; + +static const unsigned long itds_scan_masks[] = {0x7, 0}; + +/* 2g, 4g, 8g, 16g */ +static const unsigned int itds_sensitivity_scale[][4] = { + {976, 1952, 3904, 7808}, + + /* high performance mode */ + {244, 488, 976, 1952} +}; + +static const struct iio_chan_spec itds_channels[] = { + ITDS_ACC_CHAN(X, 0), + ITDS_ACC_CHAN(Y, 1), + ITDS_ACC_CHAN(Z, 2), + { + .type = IIO_TEMP, + .scan_index = 3, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | + BIT(IIO_CHAN_INFO_OFFSET), + } +}; + +static struct regmap_config itds_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + .max_register = 0x3f, +}; + +static ssize_t itds_accel_scale_avail(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct itds_info *info = iio_priv(indio_dev); + int len = 0, i; + bool hp_mode; + + hp_mode = !!(info->op_mode & ITDS_OP_MODE_HIGH_PERFORMANCE); + + for (i = 0; i < ARRAY_SIZE(itds_sensitivity_scale[0]); i++) + len += sprintf(buf + len, "%d ", + itds_sensitivity_scale[hp_mode][i]); + + buf[len - 1] = '\n'; + + return len; +} + +static ssize_t itds_samp_freq_avail(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct itds_info *info = iio_priv(indio_dev); + int len = 0, i; + int start, end; + bool hp_mode; + + hp_mode = !!(info->op_mode & ITDS_OP_MODE_HIGH_PERFORMANCE); + if (hp_mode) { + start = ITDS_ODR_12_5; + end = ITDS_ODR_1600; + } else { + start = ITDS_ODR_1_6; + end = ITDS_ODR_200; + } + + for (i = start; i <= end; i++) { + len += sprintf(buf + len, "%d.%d ", itds_sample_freq[i].val, + itds_sample_freq[i].val2); + } + buf[len - 1] = '\n'; + + return len; +} + +static int itds_get_temp(struct itds_info *info, int *val) +{ + unsigned int regval; + int ret; + __le16 tempval; + + ret = regmap_read(info->regmap, ITDS_REG_STATUS_DETECT, ®val); + if (ret) + return ret; + + if (!(regval & ITDS_EVENT_DRDY_T)) + return -EAGAIN; + + ret = regmap_bulk_read(info->regmap, ITDS_REG_TEMP_L, + &tempval, sizeof(tempval)); + if (ret) { + dev_err(info->dev, "failed to read TEMP reg\n"); + return ret; + } + + tempval = tempval >> 4; + regval = sign_extend32(le16_to_cpu(tempval), 11); + *val = regval * 625; + + return IIO_VAL_INT; +} + +static int itds_get_accel(struct itds_info *info, + const struct iio_chan_spec *chan, + int *val) +{ + unsigned int regval; + int ret; + __le16 rval; + + if ((chan->channel2 != IIO_MOD_X) && + (chan->channel2 != IIO_MOD_Y) && + (chan->channel2 != IIO_MOD_Z)) { + dev_err(info->dev, "invalid axis modifier\n"); + return -EINVAL; + } + + ret = regmap_read(info->regmap, ITDS_REG_CTRL1, ®val); + if (ret) + return ret; + + /* data won't available in power down */ + if (!(regval & ITDS_MASK_ODR)) + return -EINVAL; + + ret = regmap_read(info->regmap, ITDS_REG_STATUS, ®val); + if (ret) + return ret; + + if (!(regval & ITDS_EVENT_DRDY)) + return -EAGAIN; + + ret = regmap_bulk_read(info->regmap, chan->address, &rval, + sizeof(rval)); + if (ret) + return ret; + + mutex_lock(&info->mutex); + + if (info->op_mode & ITDS_OP_MODE_HIGH_PERFORMANCE || + info->op_mode & ITDS_OP_MODE_NORMAL) { + rval = rval >> chan->scan_type.shift; + *val = sign_extend32(le16_to_cpu(rval), + chan->scan_type.realbits - 1); + } else { + rval = rval >> (chan->scan_type.shift + 2); + *val = sign_extend32(le16_to_cpu(rval), + chan->scan_type.realbits - 3); + } + + mutex_unlock(&info->mutex); + + return IIO_VAL_INT; +} + +static int itds_set_odr(struct itds_info *info, int val, int val2) +{ + int ret, i; + int start, end; + bool hp_mode; + + hp_mode = !!(info->op_mode & ITDS_OP_MODE_HIGH_PERFORMANCE); + if (hp_mode) { + start = ITDS_ODR_12_5; + end = ITDS_ODR_1600; + } else { + start = ITDS_ODR_1_6; + end = ITDS_ODR_200; + } + + for (i = start; i <= end; i++) { + if ((val == itds_sample_freq[i].val) && + (val2 == itds_sample_freq[i].val2)) { + + ret = regmap_update_bits(info->regmap, ITDS_REG_CTRL1, + ITDS_MASK_ODR, i << 4); + return ret; + } + } + + return -EINVAL; +} + +static int itds_get_odr(struct itds_info *info, int *val, int *val2) +{ + int ret; + unsigned int rval; + bool hp_mode; + + ret = regmap_read(info->regmap, ITDS_REG_CTRL1, &rval); + if (ret) + return ret; + + rval = (rval & ITDS_MASK_ODR) >> 4; + hp_mode = !!(info->op_mode & ITDS_OP_MODE_HIGH_PERFORMANCE); + + if (rval) { + if (hp_mode && (rval < ITDS_ODR_12_5)) + rval = ITDS_ODR_12_5; + else if (!hp_mode && (rval > ITDS_ODR_200)) + rval = ITDS_ODR_200; + } + + *val = itds_sample_freq[rval].val; + *val2 = itds_sample_freq[rval].val2; + + return IIO_VAL_INT_PLUS_MICRO; +} + +static int itds_set_scale(struct itds_info *info, int val) +{ + int i, ret = -EINVAL; + bool hp_mode; + + mutex_lock(&info->mutex); + hp_mode = !!(info->op_mode & ITDS_OP_MODE_HIGH_PERFORMANCE); + for (i = 0; i < ARRAY_SIZE(itds_sensitivity_scale[0]); i++) { + if (val == itds_sensitivity_scale[hp_mode][i]) + break; + } + + if (i == ARRAY_SIZE(itds_sensitivity_scale[0])) + goto ret_unlock; + + ret = regmap_update_bits(info->regmap, ITDS_REG_CTRL6, + ITDS_MASK_SCALE, i << 4); + if (ret) + goto ret_unlock; + + info->scale = 1000 * itds_sensitivity_scale[hp_mode][i]; + +ret_unlock: + mutex_unlock(&info->mutex); + return ret; +} + +static int itds_get_scale(struct itds_info *info) +{ + int ret; + unsigned int rval; + bool hp_mode; + + ret = regmap_read(info->regmap, ITDS_REG_CTRL6, &rval); + if (ret) + return ret; + + rval = (rval & ITDS_MASK_SCALE) >> 4; + hp_mode = !!(info->op_mode & ITDS_OP_MODE_HIGH_PERFORMANCE); + info->scale = 1000 * itds_sensitivity_scale[hp_mode][rval]; + + return 0; +} + +static int itds_set_trigger_state(struct iio_trigger *trig, bool state) +{ + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig); + struct itds_info *info = iio_priv(indio_dev); + unsigned int enable; + int ret; + + enable = state ? ITDS_MASK_INT_FIFOTH : 0; + ret = regmap_update_bits(info->regmap, ITDS_REG_CTRL4, + ITDS_MASK_INT_FIFOTH, + enable); + if (ret) + dev_err(info->dev, "interrupt en/dis fail %d\n", ret); + + return ret; +} + +static int itds_fifo_flush(struct iio_dev *indio_dev, unsigned int count) +{ + struct itds_info *info = iio_priv(indio_dev); + struct device *dev = info->dev; + uint64_t sample_period, ts_per_data; + unsigned int rval; + int ret, i; + /* 3x16 bits data + 1x16 padding + timestamp 64 bits */ + __le16 buffer[8] ____cacheline_aligned; + u8 samples; + + ret = regmap_read(info->regmap, ITDS_REG_FIFO_SAMPLES, &rval); + if (ret < 0) { + dev_err(dev, "FIFO count err %d\n", ret); + return ret; + } + + if (!(rval & ITDS_EVENT_FIFO_TH)) + return 0; + + samples = rval & ITDS_MASK_SAMPLES_COUNT; + if (!samples) + return 0; + + samples = samples < count ? samples : count; + + sample_period = info->timestamp - info->old_timestamp; + do_div(sample_period, samples); + ts_per_data = info->timestamp - (samples - 1) * sample_period; + + for (i = 0; i < samples; i++) { + ret = regmap_raw_read(info->regmap, ITDS_REG_X_OUT_L, + buffer, 3 * sizeof(__le16)); + if (ret) + return i; + + iio_push_to_buffers_with_timestamp(indio_dev, buffer, + ts_per_data); + ts_per_data += sample_period; + } + + return samples; +} + +static ssize_t itds_accel_get_fifo_threshold(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct itds_info *info = iio_priv(indio_dev); + int wm; + + mutex_lock(&info->mutex); + wm = info->fifo_threshold; + mutex_unlock(&info->mutex); + + return sprintf(buf, "%d\n", wm); +} + +static int itds_accel_set_fifo_threshold(struct iio_dev *indio_dev, + unsigned int val) +{ + struct itds_info *info = iio_priv(indio_dev); + + if (val >= ITDS_ACCL_FIFO_SIZE) + val = ITDS_ACCL_FIFO_SIZE - 1; + + mutex_lock(&info->mutex); + info->fifo_threshold = val; + mutex_unlock(&info->mutex); + + return 0; +} + +static int itds_set_operating_mode(struct itds_info *info, + enum itds_operating_mode mode) +{ + int ret; + + ret = regmap_update_bits(info->regmap, ITDS_REG_CTRL1, + ITDS_MASK_MODE, 1 << mode); + if (!ret) { + info->op_mode = mode; + + /* update scale according to new operating mode */ + itds_get_scale(info); + } + + return ret; +} + +static ssize_t itds_accel_set_operating_mode(struct device *dev, + struct device_attribute *attr, + const char *buf, + size_t count) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct itds_info *info = iio_priv(indio_dev); + int ret = -EINVAL; + int i; + + mutex_lock(&info->mutex); + + for (i = 0; i < ITDS_OP_MODE_MAX; i++) { + if (!strcmp(buf, operating_mode[i])) { + ret = itds_set_operating_mode(info, i); + if (!ret) + ret = count; + + break; + } + } + + mutex_unlock(&info->mutex); + return ret; +} + +static ssize_t itds_accel_get_operating_mode(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct itds_info *info = iio_priv(indio_dev); + int mode; + + mutex_lock(&info->mutex); + mode = info->op_mode; + mutex_unlock(&info->mutex); + + return sprintf(buf, "%s\n", operating_mode[mode]); +} + +static ssize_t itds_accel_get_fifo_state(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct itds_info *info = iio_priv(indio_dev); + bool is_enabled; + + mutex_lock(&info->mutex); + is_enabled = info->is_hwfifo_enabled; + mutex_unlock(&info->mutex); + + return sprintf(buf, "%d\n", is_enabled); +} + +static IIO_CONST_ATTR(hwfifo_threshold_min, "1"); +static IIO_CONST_ATTR(hwfifo_threshold_max, __stringify(ITDS_ACCL_FIFO_SIZE)); +static IIO_DEVICE_ATTR(hwfifo_enabled, 0444, + itds_accel_get_fifo_state, NULL, 0); +static IIO_DEVICE_ATTR(hwfifo_threshold, 0444, + itds_accel_get_fifo_threshold, NULL, 0); +static IIO_DEVICE_ATTR(operating_mode, 0644, itds_accel_get_operating_mode, + itds_accel_set_operating_mode, 0); +static IIO_DEVICE_ATTR(in_accel_samp_freq_available, 0444, + itds_samp_freq_avail, NULL, 0); +static IIO_DEVICE_ATTR(in_accel_scale_available, 0444, + itds_accel_scale_avail, NULL, 0); + +static struct attribute *itds_accel_fifo_attributes[] = { + &iio_const_attr_hwfifo_threshold_min.dev_attr.attr, + &iio_const_attr_hwfifo_threshold_max.dev_attr.attr, + &iio_dev_attr_hwfifo_threshold.dev_attr.attr, + &iio_dev_attr_hwfifo_enabled.dev_attr.attr, + &iio_dev_attr_operating_mode.dev_attr.attr, + &iio_dev_attr_in_accel_samp_freq_available.dev_attr.attr, + &iio_dev_attr_in_accel_scale_available.dev_attr.attr, + NULL, +}; + +static struct attribute_group itds_attrs_group = { + .attrs = itds_accel_fifo_attributes, +}; + +static irqreturn_t itds_trigger_handler(int irq, void *p) +{ + struct iio_poll_func *pf = p; + struct iio_dev *indio_dev = pf->indio_dev; + struct itds_info *info = iio_priv(indio_dev); + + if (info->is_hwfifo_enabled) + itds_fifo_flush(indio_dev, ITDS_ACCL_FIFO_SIZE); + + iio_trigger_notify_done(indio_dev->trig); + + return IRQ_HANDLED; +} + +static irqreturn_t itds_accel_irq_thread_handler(int irq, void *private) +{ + struct iio_dev *indio_dev = private; + struct itds_info *info = iio_priv(indio_dev); + unsigned int rval; + int ret; + + ret = regmap_read(info->regmap, ITDS_REG_STATUS, &rval); + if (ret) + return IRQ_NONE; + + if (rval & ITDS_EVENT_FIFO_TH) + iio_trigger_poll_chained(info->trig); + + return IRQ_HANDLED; +} + +static irqreturn_t itds_accel_irq_handler(int irq, void *private) +{ + struct iio_dev *indio_dev = private; + struct itds_info *info = iio_priv(indio_dev); + + if (info->is_hwfifo_enabled) { + info->old_timestamp = info->timestamp; + info->timestamp = iio_get_time_ns(indio_dev); + return IRQ_WAKE_THREAD; + } + + return IRQ_HANDLED; +} + +static int itds_accel_buffer_predisable(struct iio_dev *indio_dev) +{ + struct itds_info *info = iio_priv(indio_dev); + + if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) + iio_triggered_buffer_predisable(indio_dev); + + regmap_update_bits(info->regmap, ITDS_REG_FIFO_CTRL, + ITDS_MASK_FIFOTH | ITDS_MASK_FIFOMODE, 0); + regmap_update_bits(info->regmap, ITDS_REG_CTRL4, + ITDS_MASK_INT_FIFOTH, 0); + + info->is_hwfifo_enabled = false; + + return 0; +} + +static int itds_accel_buffer_postenable(struct iio_dev *indio_dev) +{ + struct itds_info *info = iio_priv(indio_dev); + unsigned int rval; + int ret; + + ret = regmap_read(info->regmap, ITDS_REG_CTRL1, &rval); + if (ret) + return ret; + + if (!(rval & ITDS_MASK_ODR)) + return -EINVAL; + + ret = regmap_update_bits(info->regmap, ITDS_REG_FIFO_CTRL, + ITDS_MASK_FIFOTH, info->fifo_threshold); + if (ret) + return ret; + + ret = regmap_update_bits(info->regmap, ITDS_REG_FIFO_CTRL, + ITDS_MASK_FIFOMODE, + ITDS_FIFO_MODE_FIFO); + if (ret) + return ret; + + info->is_hwfifo_enabled = true; + if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) + iio_triggered_buffer_postenable(indio_dev); + + return 0; +} + +static int itds_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int *val, + int *val2, long mask) +{ + struct itds_info *info = iio_priv(indio_dev); + int ret; + + switch (mask) { + case IIO_CHAN_INFO_RAW: + if (chan->type == IIO_ACCEL) { + ret = iio_device_claim_direct_mode(indio_dev); + if (ret) + return ret; + + ret = itds_get_accel(info, chan, val); + iio_device_release_direct_mode(indio_dev); + } else { + ret = itds_get_temp(info, val); + } + return ret; + + case IIO_CHAN_INFO_SCALE: + *val = 0; + if (chan->type == IIO_ACCEL) + *val2 = info->scale; + else + *val2 = 100; + return IIO_VAL_INT_PLUS_MICRO; + + case IIO_CHAN_INFO_SAMP_FREQ: + ret = itds_get_odr(info, val, val2); + return ret; + + case IIO_CHAN_INFO_OFFSET: + if (chan->type == IIO_TEMP) + *val = 250000; + return IIO_VAL_INT; + + default: + return -EINVAL; + } +} + +static int itds_write_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int val, int val2, long mask) +{ + struct itds_info *info = iio_priv(indio_dev); + + switch (mask) { + case IIO_CHAN_INFO_SAMP_FREQ: + return itds_set_odr(info, val, val2); + + case IIO_CHAN_INFO_SCALE: + return itds_set_scale(info, val); + + default: + return -EINVAL; + } +} + +static const struct iio_trigger_ops itds_trigger_ops = { + .set_trigger_state = itds_set_trigger_state, +}; + +static const struct iio_buffer_setup_ops itds_accel_buffer_ops = { + .postenable = itds_accel_buffer_postenable, + .predisable = itds_accel_buffer_predisable, +}; + +static const struct iio_info itds_accel_info = { + .attrs = &itds_attrs_group, + .read_raw = itds_read_raw, + .write_raw = itds_write_raw, + .hwfifo_flush_to_buffer = itds_fifo_flush, + .hwfifo_set_watermark = itds_accel_set_fifo_threshold, +}; + +static void itds_regulator_disable(void *data) +{ + struct itds_info *info = data; + + regmap_update_bits(info->regmap, ITDS_REG_CTRL7, ITDS_MASK_INT_EN, 0); + regmap_update_bits(info->regmap, ITDS_REG_CTRL1, ITDS_MASK_MODE, 0); + if (!IS_ERR_OR_NULL(info->vdd_supply)) + regulator_disable(info->vdd_supply); + + if (!IS_ERR_OR_NULL(info->vddio_supply)) + regulator_disable(info->vddio_supply); +} + +static int itds_probe(struct i2c_client *client) +{ + struct device *dev = &client->dev; + struct iio_dev *indio_dev; + struct itds_info *info; + struct regmap *regmap; + unsigned int rval; + int ret; + + indio_dev = devm_iio_device_alloc(dev, sizeof(*info)); + if (!indio_dev) + return -ENOMEM; + + regmap = devm_regmap_init_i2c(client, &itds_regmap_config); + if (IS_ERR(regmap)) { + dev_err(dev, "Failed to allocate regmap!\n"); + return PTR_ERR(regmap); + } + + info = iio_priv(indio_dev); + info->regmap = regmap; + info->dev = dev; + i2c_set_clientdata(client, indio_dev); + mutex_init(&info->mutex); + + info->vdd_supply = devm_regulator_get_optional(dev, "vdd"); + info->vddio_supply = devm_regulator_get_optional(dev, "vddio"); + + if (!IS_ERR_OR_NULL(info->vdd_supply) && + !IS_ERR_OR_NULL(info->vddio_supply)) { + + ret = regulator_set_voltage(info->vdd_supply, 1700000, 3600000); + if (ret) + return ret; + + ret = regulator_set_voltage(info->vddio_supply, + 1200000, 3700000); + if (ret) + return ret; + + ret = regulator_enable(info->vdd_supply); + if (ret) { + dev_err(dev, "Failed to enable vdd: %d\n", ret); + return ret; + } + + ret = regulator_enable(info->vddio_supply); + if (ret) { + dev_err(dev, "Failed to enable vddio: %d\n", ret); + goto out_disable_vdd; + } + + /* chip boot sequence takes 20ms */ + usleep_range(20000, 21000); + } + + ret = devm_add_action_or_reset(dev, itds_regulator_disable, info); + if (ret) + return ret; + + ret = regmap_read(info->regmap, ITDS_REG_DEV_ID, &rval); + if (ret) { + dev_info(dev, "device id read err %d\n", ret); + goto out_disable_vddio; + } + + if (rval != ITDS_DEVICE_ID) { + dev_info(dev, "device id %X not matched\n", rval); + goto out_disable_vddio; + } + + ret = regmap_update_bits(info->regmap, ITDS_REG_CTRL2, + ITDS_MASK_BDU_INC_ADD, ITDS_MASK_BDU_INC_ADD); + if (ret) { + dev_err(dev, "unable to set block data update!\n"); + goto out_disable_vddio; + } + + ret = regmap_update_bits(info->regmap, ITDS_REG_WAKEUP_EVENT, 0xff, 0); + if (ret) { + dev_err(dev, "disable wakeup event fail!\n"); + goto out_disable_vddio; + } + + indio_dev->name = dev_name(dev); + indio_dev->dev.parent = dev; + indio_dev->channels = itds_channels; + indio_dev->num_channels = ARRAY_SIZE(itds_channels); + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->available_scan_masks = itds_scan_masks; + indio_dev->info = &itds_accel_info; + + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, + NULL, + itds_trigger_handler, + &itds_accel_buffer_ops); + if (ret) { + dev_err(dev, "unable to setup iio triggered buffer\n"); + goto out_disable_vddio; + } + + if (client->irq > 0) { + ret = devm_request_threaded_irq(dev, client->irq, + itds_accel_irq_handler, + itds_accel_irq_thread_handler, + IRQF_TRIGGER_RISING, + "itds_event", indio_dev); + if (ret) { + dev_err(dev, "unable to request IRQ\n"); + goto out_disable_vddio; + } + + info->trig = devm_iio_trigger_alloc(dev, "%s-dev%d", + indio_dev->name, indio_dev->id); + if (!info->trig) { + ret = -ENOMEM; + goto out_disable_vddio; + } + + info->trig->dev.parent = dev; + info->trig->ops = &itds_trigger_ops; + iio_trigger_set_drvdata(info->trig, indio_dev); + indio_dev->trig = iio_trigger_get(info->trig); + + ret = devm_iio_trigger_register(dev, info->trig); + if (ret) + goto out_disable_vddio; + + ret = regmap_update_bits(info->regmap, ITDS_REG_CTRL7, + ITDS_MASK_INT_EN, ITDS_MASK_INT_EN); + if (ret) + goto out_disable_vddio; + } + + dev_set_drvdata(dev, indio_dev); + ret = devm_iio_device_register(dev, indio_dev); + if (ret) { + dev_err(dev, "IIO device register fail: %d\n", ret); + goto out_disable_vddio; + } + + return 0; + +out_disable_vddio: + if (!IS_ERR_OR_NULL(info->vddio_supply)) + regulator_disable(info->vddio_supply); + +out_disable_vdd: + if (!IS_ERR_OR_NULL(info->vdd_supply)) + regulator_disable(info->vdd_supply); + + return ret; +} + +static const struct of_device_id itds_of_match[] = { + { .compatible = "we,wsen-itds"}, + {} +}; +MODULE_DEVICE_TABLE(of, itds_of_match); + +static const struct i2c_device_id itds_id[] = { + { "wsen-itds", }, + {} +}; +MODULE_DEVICE_TABLE(i2c, itds_id); + +static struct i2c_driver itds_driver = { + .driver = { + .name = "wsen-itds", + .of_match_table = itds_of_match, + }, + .probe_new = itds_probe, + .id_table = itds_id, +}; +module_i2c_driver(itds_driver); + +MODULE_AUTHOR("Saravanan Sekar <saravanan@linumiz.com>"); +MODULE_DESCRIPTION("Würth Elektronik WSEN-ITDS 3-axis accel driver"); +MODULE_LICENSE("GPL"); -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/4] iio: accel: Add driver for wsen-itds accelerometer sensor 2020-04-29 13:39 ` [PATCH v2 2/4] iio: accel: Add driver for wsen-itds accelerometer sensor Saravanan Sekar @ 2020-05-03 12:42 ` Jonathan Cameron 0 siblings, 0 replies; 10+ messages in thread From: Jonathan Cameron @ 2020-05-03 12:42 UTC (permalink / raw) To: Saravanan Sekar Cc: robh+dt, knaack.h, lars, pmeerw, broonie, lgirdwood, devicetree, linux-kernel, linux-iio, Alexandru Ardelean On Wed, 29 Apr 2020 15:39:41 +0200 Saravanan Sekar <saravanan@linumiz.com> wrote: > Add an iio driver for the wurth elektronic wsen-itds 3-axis accelerometer. > The driver supports the acceleration, temperature data reading and > supports configuring of output data rate, operating mode and scale. > > Signed-off-by: Saravanan Sekar <saravanan@linumiz.com> +CC Alex. This one has some unusual buffer handling please, sanity check that it isn't going to cause problem for your core rework. Various comments inline. Note I left the ABI discussion for the documentation patch. Thanks, Jonathan > --- > drivers/iio/accel/Kconfig | 14 + > drivers/iio/accel/Makefile | 1 + > drivers/iio/accel/wsen-itds.c | 947 ++++++++++++++++++++++++++++++++++ > 3 files changed, 962 insertions(+) > create mode 100644 drivers/iio/accel/wsen-itds.c > > diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig > index 5d91a6dda894..ad96ba2a9e38 100644 > --- a/drivers/iio/accel/Kconfig > +++ b/drivers/iio/accel/Kconfig > @@ -450,4 +450,18 @@ config STK8BA50 > Choosing M will build the driver as a module. If so, the module > will be called stk8ba50. > > +config WSEN_ITDS > + tristate "Wurth Elektronik WSEN-ITDS 3-Axis Accelerometer Driver" > + depends on I2C > + select IIO_BUFFER > + select IIO_TRIGGERED_BUFFER > + select REGMAP_I2C > + help > + Say yes here to build support for the WSEN-ITDS 3-axis > + accelerometer. > + > + The sensor provides 3-axis Acceleration, Temperature data and > + application specific features like tap, 6D Orinetation, Free-fall > + Motion, Activity etc > + > endmenu > diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile > index 3a051cf37f40..942cab3e1d16 100644 > --- a/drivers/iio/accel/Makefile > +++ b/drivers/iio/accel/Makefile > @@ -61,3 +61,4 @@ st_accel-$(CONFIG_IIO_BUFFER) += st_accel_buffer.o > > obj-$(CONFIG_IIO_ST_ACCEL_I2C_3AXIS) += st_accel_i2c.o > obj-$(CONFIG_IIO_ST_ACCEL_SPI_3AXIS) += st_accel_spi.o > +obj-$(CONFIG_WSEN_ITDS) += wsen-itds.o > diff --git a/drivers/iio/accel/wsen-itds.c b/drivers/iio/accel/wsen-itds.c > new file mode 100644 > index 000000000000..d7bc961176c3 > --- /dev/null > +++ b/drivers/iio/accel/wsen-itds.c > @@ -0,0 +1,947 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * WSEN-ITDS 3-axis accel sensor driver > + * > + * Copyright 2020 Linumiz > + * > + * Author: Saravanan Sekar <saravanan@linumiz.com> > + * > + * TODO : events - 6D, Orientation, Free-fall, Tap, Motion > + */ > + > +#include <linux/cache.h> > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/i2c.h> > +#include <linux/iio/buffer.h> > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> > +#include <linux/iio/trigger.h> > +#include <linux/iio/trigger_consumer.h> > +#include <linux/iio/triggered_buffer.h> > +#include <linux/interrupt.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > +#include <linux/slab.h> > + > +#define ITDS_REG_TEMP_L 0x0d > +#define ITDS_REG_DEV_ID 0x0f > +#define ITDS_REG_CTRL1 0x20 > +#define ITDS_REG_CTRL2 0x21 > +#define ITDS_REG_CTRL3 0x22 > +#define ITDS_REG_CTRL4 0x23 > +#define ITDS_REG_CTRL5 0x24 > +#define ITDS_REG_CTRL6 0x25 > +#define ITDS_REG_STATUS 0x27 > +#define ITDS_REG_X_OUT_L 0x28 > +#define ITDS_REG_Y_OUT_L 0x2a > +#define ITDS_REG_Z_OUT_L 0x2c > +#define ITDS_REG_FIFO_CTRL 0x2e > +#define ITDS_REG_FIFO_SAMPLES 0x2f > +#define ITDS_REG_STATUS_DETECT 0x37 > +#define ITDS_REG_WAKEUP_EVENT 0x38 > +#define ITDS_REG_CTRL7 0x3f > + > +#define ITDS_MASK_SCALE GENMASK(5, 4) > +#define ITDS_MASK_BDU_INC_ADD GENMASK(3, 2) > +#define ITDS_MASK_FIFOTH GENMASK(4, 0) > +#define ITDS_MASK_FIFOMODE GENMASK(7, 5) > +#define ITDS_MASK_MODE GENMASK(3, 0) > +#define ITDS_MASK_SAMPLES_COUNT GENMASK(5, 0) > +#define ITDS_MASK_ODR GENMASK(7, 4) > +#define ITDS_MASK_INT_DRDY BIT(0) > +#define ITDS_MASK_INT_FIFOTH BIT(1) > +#define ITDS_MASK_INT_EN BIT(5) > + > +#define ITDS_EVENT_DRDY BIT(0) > +#define ITDS_EVENT_DRDY_T BIT(6) > +#define ITDS_EVENT_FIFO_TH BIT(7) > +#define ITDS_FIFO_MODE_BYPASS 0 > +#define ITDS_FIFO_MODE_FIFO BIT(5) > +#define ITDS_DEVICE_ID 0x44 > +#define ITDS_ACCL_FIFO_SIZE 32 > + > +#define ITDS_ACC_CHAN(_axis, _idx) { \ > + .type = IIO_ACCEL, \ > + .modified = 1, \ > + .channel2 = IIO_MOD_##_axis, \ > + .address = ITDS_REG_##_axis##_OUT_L, \ > + .scan_index = _idx, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \ > + BIT(IIO_CHAN_INFO_SCALE), \ > + .scan_type = { \ > + .sign = 's', \ > + .realbits = 14, \ > + .storagebits = 16, \ > + .shift = 2, \ > + .endianness = IIO_LE, \ > + }, \ > +} > + > +enum itds_operating_mode { > + ITDS_OP_MODE_LOW_POWER, > + ITDS_OP_MODE_NORMAL = BIT(0), > + ITDS_OP_MODE_HIGH_PERFORMANCE = BIT(1), > + ITDS_OP_MODE_SINGLE_SHOT = BIT(2), > + ITDS_OP_MODE_MAX = 4 Odd mix of bitmap and a max value. That's hard to read so don't do it. Given the value 3 is missing, and you are using this max to set the size of an array I guess aligns with this below, things will not work as expected... > +}; > + > +enum itds_odr { > + ITDS_ODR_0, > + ITDS_ODR_1_6, > + ITDS_ODR_12_5, > + ITDS_ODR_25, > + ITDS_ODR_50, > + ITDS_ODR_100, > + ITDS_ODR_200, > + ITDS_ODR_400, > + ITDS_ODR_800, > + ITDS_ODR_1600, > + ITDS_ODR_MAX > +}; > + > +struct itds_info { > + struct device *dev; > + struct regmap *regmap; > + struct iio_trigger *trig; > + struct regulator *vdd_supply; > + struct regulator *vddio_supply; > + struct mutex mutex; /* itds info struct member */ > + int64_t timestamp; > + int64_t old_timestamp; > + unsigned int scale; > + enum itds_operating_mode op_mode; > + u8 fifo_threshold; > + bool is_hwfifo_enabled; > +}; > + > +struct itds_sample_freqency { > + int val; > + int val2; > +}; > + > +static const char *operating_mode[ITDS_OP_MODE_MAX] = { > + "lowpower", > + "normal", > + "high_perf", > + "single_shot" > +}; > + > +static const struct itds_sample_freqency itds_sample_freq[ITDS_ODR_MAX] = { > + {0}, {1, 600000}, {12, 500000}, {25}, {50}, {100}, {200}, > + {400}, {800}, {1600} > +}; > + > +static const unsigned long itds_scan_masks[] = {0x7, 0}; > + > +/* 2g, 4g, 8g, 16g */ > +static const unsigned int itds_sensitivity_scale[][4] = { > + {976, 1952, 3904, 7808}, > + > + /* high performance mode */ > + {244, 488, 976, 1952} > +}; > + > +static const struct iio_chan_spec itds_channels[] = { > + ITDS_ACC_CHAN(X, 0), > + ITDS_ACC_CHAN(Y, 1), > + ITDS_ACC_CHAN(Z, 2), > + { > + .type = IIO_TEMP, > + .scan_index = 3, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | > + BIT(IIO_CHAN_INFO_OFFSET), > + } > +}; > + > +static struct regmap_config itds_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .max_register = 0x3f, > +}; > + > +static ssize_t itds_accel_scale_avail(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct itds_info *info = iio_priv(indio_dev); > + int len = 0, i; > + bool hp_mode; > + > + hp_mode = !!(info->op_mode & ITDS_OP_MODE_HIGH_PERFORMANCE); > + > + for (i = 0; i < ARRAY_SIZE(itds_sensitivity_scale[0]); i++) > + len += sprintf(buf + len, "%d ", > + itds_sensitivity_scale[hp_mode][i]); > + > + buf[len - 1] = '\n'; > + > + return len; > +} > + > +static ssize_t itds_samp_freq_avail(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct itds_info *info = iio_priv(indio_dev); > + int len = 0, i; > + int start, end; > + bool hp_mode; > + > + hp_mode = !!(info->op_mode & ITDS_OP_MODE_HIGH_PERFORMANCE); > + if (hp_mode) { > + start = ITDS_ODR_12_5; > + end = ITDS_ODR_1600; > + } else { > + start = ITDS_ODR_1_6; > + end = ITDS_ODR_200; > + } > + > + for (i = start; i <= end; i++) { > + len += sprintf(buf + len, "%d.%d ", itds_sample_freq[i].val, > + itds_sample_freq[i].val2); > + } > + buf[len - 1] = '\n'; > + > + return len; > +} > + > +static int itds_get_temp(struct itds_info *info, int *val) > +{ > + unsigned int regval; > + int ret; > + __le16 tempval; > + > + ret = regmap_read(info->regmap, ITDS_REG_STATUS_DETECT, ®val); > + if (ret) > + return ret; > + > + if (!(regval & ITDS_EVENT_DRDY_T)) > + return -EAGAIN; > + > + ret = regmap_bulk_read(info->regmap, ITDS_REG_TEMP_L, > + &tempval, sizeof(tempval)); > + if (ret) { > + dev_err(info->dev, "failed to read TEMP reg\n"); > + return ret; > + } > + > + tempval = tempval >> 4; > + regval = sign_extend32(le16_to_cpu(tempval), 11); > + *val = regval * 625; > + > + return IIO_VAL_INT; > +} > + > +static int itds_get_accel(struct itds_info *info, > + const struct iio_chan_spec *chan, > + int *val) > +{ > + unsigned int regval; > + int ret; > + __le16 rval; > + > + if ((chan->channel2 != IIO_MOD_X) && > + (chan->channel2 != IIO_MOD_Y) && > + (chan->channel2 != IIO_MOD_Z)) { > + dev_err(info->dev, "invalid axis modifier\n"); Given this is a paranoid check anyway, I'd argue the print is excessive.. You can't get here unless we have a bug. > + return -EINVAL; > + } > + > + ret = regmap_read(info->regmap, ITDS_REG_CTRL1, ®val); > + if (ret) > + return ret; > + > + /* data won't available in power down */ > + if (!(regval & ITDS_MASK_ODR)) How did you get here in power down mode? Sounds like a bug. If someone is reading you should be powered up, or power up when they try to read. > + return -EINVAL; > + > + ret = regmap_read(info->regmap, ITDS_REG_STATUS, ®val); > + if (ret) > + return ret; > + > + if (!(regval & ITDS_EVENT_DRDY)) > + return -EAGAIN; > + > + ret = regmap_bulk_read(info->regmap, chan->address, &rval, > + sizeof(rval)); > + if (ret) > + return ret; > + > + mutex_lock(&info->mutex); > + > + if (info->op_mode & ITDS_OP_MODE_HIGH_PERFORMANCE || > + info->op_mode & ITDS_OP_MODE_NORMAL) { > + rval = rval >> chan->scan_type.shift; > + *val = sign_extend32(le16_to_cpu(rval), > + chan->scan_type.realbits - 1); > + } else { > + rval = rval >> (chan->scan_type.shift + 2); > + *val = sign_extend32(le16_to_cpu(rval), > + chan->scan_type.realbits - 3); > + } > + > + mutex_unlock(&info->mutex); > + > + return IIO_VAL_INT; > +} > + > +static int itds_set_odr(struct itds_info *info, int val, int val2) > +{ > + int ret, i; > + int start, end; > + bool hp_mode; > + > + hp_mode = !!(info->op_mode & ITDS_OP_MODE_HIGH_PERFORMANCE); > + if (hp_mode) { > + start = ITDS_ODR_12_5; > + end = ITDS_ODR_1600; > + } else { > + start = ITDS_ODR_1_6; > + end = ITDS_ODR_200; > + } > + > + for (i = start; i <= end; i++) { > + if ((val == itds_sample_freq[i].val) && > + (val2 == itds_sample_freq[i].val2)) { > + > + ret = regmap_update_bits(info->regmap, ITDS_REG_CTRL1, > + ITDS_MASK_ODR, i << 4); > + return ret; > + } > + } > + > + return -EINVAL; > +} > + > +static int itds_get_odr(struct itds_info *info, int *val, int *val2) > +{ > + int ret; > + unsigned int rval; > + bool hp_mode; > + > + ret = regmap_read(info->regmap, ITDS_REG_CTRL1, &rval); > + if (ret) > + return ret; > + > + rval = (rval & ITDS_MASK_ODR) >> 4; > + hp_mode = !!(info->op_mode & ITDS_OP_MODE_HIGH_PERFORMANCE); > + > + if (rval) { > + if (hp_mode && (rval < ITDS_ODR_12_5)) > + rval = ITDS_ODR_12_5; > + else if (!hp_mode && (rval > ITDS_ODR_200)) > + rval = ITDS_ODR_200; > + } > + > + *val = itds_sample_freq[rval].val; > + *val2 = itds_sample_freq[rval].val2; > + > + return IIO_VAL_INT_PLUS_MICRO; > +} > + > +static int itds_set_scale(struct itds_info *info, int val) > +{ > + int i, ret = -EINVAL; > + bool hp_mode; > + > + mutex_lock(&info->mutex); > + hp_mode = !!(info->op_mode & ITDS_OP_MODE_HIGH_PERFORMANCE); > + for (i = 0; i < ARRAY_SIZE(itds_sensitivity_scale[0]); i++) { > + if (val == itds_sensitivity_scale[hp_mode][i]) > + break; > + } > + > + if (i == ARRAY_SIZE(itds_sensitivity_scale[0])) > + goto ret_unlock; > + > + ret = regmap_update_bits(info->regmap, ITDS_REG_CTRL6, > + ITDS_MASK_SCALE, i << 4); > + if (ret) > + goto ret_unlock; > + > + info->scale = 1000 * itds_sensitivity_scale[hp_mode][i]; > + > +ret_unlock: > + mutex_unlock(&info->mutex); > + return ret; > +} > + > +static int itds_get_scale(struct itds_info *info) > +{ > + int ret; > + unsigned int rval; > + bool hp_mode; > + > + ret = regmap_read(info->regmap, ITDS_REG_CTRL6, &rval); > + if (ret) > + return ret; > + > + rval = (rval & ITDS_MASK_SCALE) >> 4; > + hp_mode = !!(info->op_mode & ITDS_OP_MODE_HIGH_PERFORMANCE); > + info->scale = 1000 * itds_sensitivity_scale[hp_mode][rval]; > + > + return 0; > +} > + > +static int itds_set_trigger_state(struct iio_trigger *trig, bool state) > +{ > + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig); > + struct itds_info *info = iio_priv(indio_dev); > + unsigned int enable; > + int ret; > + > + enable = state ? ITDS_MASK_INT_FIFOTH : 0; > + ret = regmap_update_bits(info->regmap, ITDS_REG_CTRL4, > + ITDS_MASK_INT_FIFOTH, > + enable); > + if (ret) > + dev_err(info->dev, "interrupt en/dis fail %d\n", ret); > + > + return ret; > +} > + > +static int itds_fifo_flush(struct iio_dev *indio_dev, unsigned int count) > +{ > + struct itds_info *info = iio_priv(indio_dev); > + struct device *dev = info->dev; > + uint64_t sample_period, ts_per_data; > + unsigned int rval; > + int ret, i; > + /* 3x16 bits data + 1x16 padding + timestamp 64 bits */ > + __le16 buffer[8] ____cacheline_aligned; Doesn't work. Look at what the macro does. This needs to be on the heap, not the stack. It is extremely difficult to force any alignment on the stack beyond that of individual elements. So like most other IIO drivers, just add that buffer to your info structure. Also if you would need to zero it each time to avoid leaking arbitary kernel memory. If it's in the info structure the worst that can happen is you leak previous samples and those are unlikely to be sensitive info ;) We have a few cases of this leak and alignment problems to clean up based on the adis16475 review thread. Note however, this is using i2c which doesn't require dma safe buffers. So the alignment we need is most likely 8 not a cacheline. > + u8 samples; > + > + ret = regmap_read(info->regmap, ITDS_REG_FIFO_SAMPLES, &rval); > + if (ret < 0) { > + dev_err(dev, "FIFO count err %d\n", ret); > + return ret; > + } > + > + if (!(rval & ITDS_EVENT_FIFO_TH)) > + return 0; > + > + samples = rval & ITDS_MASK_SAMPLES_COUNT; > + if (!samples) > + return 0; > + > + samples = samples < count ? samples : count; > + > + sample_period = info->timestamp - info->old_timestamp; > + do_div(sample_period, samples); > + ts_per_data = info->timestamp - (samples - 1) * sample_period; > + > + for (i = 0; i < samples; i++) { > + ret = regmap_raw_read(info->regmap, ITDS_REG_X_OUT_L, > + buffer, 3 * sizeof(__le16)); > + if (ret) > + return i; > + > + iio_push_to_buffers_with_timestamp(indio_dev, buffer, > + ts_per_data); > + ts_per_data += sample_period; > + } > + > + return samples; > +} > + > +static ssize_t itds_accel_get_fifo_threshold(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct itds_info *info = iio_priv(indio_dev); > + int wm; > + > + mutex_lock(&info->mutex); > + wm = info->fifo_threshold; > + mutex_unlock(&info->mutex); > + > + return sprintf(buf, "%d\n", wm); > +} > + > +static int itds_accel_set_fifo_threshold(struct iio_dev *indio_dev, > + unsigned int val) > +{ > + struct itds_info *info = iio_priv(indio_dev); > + > + if (val >= ITDS_ACCL_FIFO_SIZE) > + val = ITDS_ACCL_FIFO_SIZE - 1; > + > + mutex_lock(&info->mutex); > + info->fifo_threshold = val; > + mutex_unlock(&info->mutex); > + > + return 0; > +} > + > +static int itds_set_operating_mode(struct itds_info *info, > + enum itds_operating_mode mode) > +{ > + int ret; > + > + ret = regmap_update_bits(info->regmap, ITDS_REG_CTRL1, > + ITDS_MASK_MODE, 1 << mode); > + if (!ret) { > + info->op_mode = mode; > + > + /* update scale according to new operating mode */ > + itds_get_scale(info); > + } > + > + return ret; > +} > + > +static ssize_t itds_accel_set_operating_mode(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t count) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct itds_info *info = iio_priv(indio_dev); > + int ret = -EINVAL; > + int i; > + > + mutex_lock(&info->mutex); > + > + for (i = 0; i < ITDS_OP_MODE_MAX; i++) { > + if (!strcmp(buf, operating_mode[i])) { > + ret = itds_set_operating_mode(info, i); > + if (!ret) > + ret = count; > + > + break; > + } > + } > + > + mutex_unlock(&info->mutex); > + return ret; > +} > + > +static ssize_t itds_accel_get_operating_mode(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct itds_info *info = iio_priv(indio_dev); > + int mode; > + > + mutex_lock(&info->mutex); > + mode = info->op_mode; > + mutex_unlock(&info->mutex); > + > + return sprintf(buf, "%s\n", operating_mode[mode]); > +} > + > +static ssize_t itds_accel_get_fifo_state(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct itds_info *info = iio_priv(indio_dev); > + bool is_enabled; > + > + mutex_lock(&info->mutex); > + is_enabled = info->is_hwfifo_enabled; > + mutex_unlock(&info->mutex); > + > + return sprintf(buf, "%d\n", is_enabled); > +} > + > +static IIO_CONST_ATTR(hwfifo_threshold_min, "1"); > +static IIO_CONST_ATTR(hwfifo_threshold_max, __stringify(ITDS_ACCL_FIFO_SIZE)); > +static IIO_DEVICE_ATTR(hwfifo_enabled, 0444, > + itds_accel_get_fifo_state, NULL, 0); > +static IIO_DEVICE_ATTR(hwfifo_threshold, 0444, > + itds_accel_get_fifo_threshold, NULL, 0); > +static IIO_DEVICE_ATTR(operating_mode, 0644, itds_accel_get_operating_mode, > + itds_accel_set_operating_mode, 0); > +static IIO_DEVICE_ATTR(in_accel_samp_freq_available, 0444, > + itds_samp_freq_avail, NULL, 0); > +static IIO_DEVICE_ATTR(in_accel_scale_available, 0444, > + itds_accel_scale_avail, NULL, 0); > + > +static struct attribute *itds_accel_fifo_attributes[] = { > + &iio_const_attr_hwfifo_threshold_min.dev_attr.attr, > + &iio_const_attr_hwfifo_threshold_max.dev_attr.attr, > + &iio_dev_attr_hwfifo_threshold.dev_attr.attr, > + &iio_dev_attr_hwfifo_enabled.dev_attr.attr, > + &iio_dev_attr_operating_mode.dev_attr.attr, > + &iio_dev_attr_in_accel_samp_freq_available.dev_attr.attr, > + &iio_dev_attr_in_accel_scale_available.dev_attr.attr, > + NULL, > +}; > + > +static struct attribute_group itds_attrs_group = { > + .attrs = itds_accel_fifo_attributes, > +}; > + > +static irqreturn_t itds_trigger_handler(int irq, void *p) > +{ > + struct iio_poll_func *pf = p; > + struct iio_dev *indio_dev = pf->indio_dev; > + struct itds_info *info = iio_priv(indio_dev); > + > + if (info->is_hwfifo_enabled) > + itds_fifo_flush(indio_dev, ITDS_ACCL_FIFO_SIZE); > + > + iio_trigger_notify_done(indio_dev->trig); > + > + return IRQ_HANDLED; > +} > + > +static irqreturn_t itds_accel_irq_thread_handler(int irq, void *private) > +{ > + struct iio_dev *indio_dev = private; > + struct itds_info *info = iio_priv(indio_dev); > + unsigned int rval; > + int ret; > + > + ret = regmap_read(info->regmap, ITDS_REG_STATUS, &rval); > + if (ret) > + return IRQ_NONE; > + > + if (rval & ITDS_EVENT_FIFO_TH) > + iio_trigger_poll_chained(info->trig); > + > + return IRQ_HANDLED; > +} > + > +static irqreturn_t itds_accel_irq_handler(int irq, void *private) > +{ > + struct iio_dev *indio_dev = private; > + struct itds_info *info = iio_priv(indio_dev); > + > + if (info->is_hwfifo_enabled) { > + info->old_timestamp = info->timestamp; > + info->timestamp = iio_get_time_ns(indio_dev); > + return IRQ_WAKE_THREAD; > + } > + > + return IRQ_HANDLED; > +} > + > +static int itds_accel_buffer_predisable(struct iio_dev *indio_dev) > +{ > + struct itds_info *info = iio_priv(indio_dev); > + > + if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) > + iio_triggered_buffer_predisable(indio_dev); > + > + regmap_update_bits(info->regmap, ITDS_REG_FIFO_CTRL, > + ITDS_MASK_FIFOTH | ITDS_MASK_FIFOMODE, 0); > + regmap_update_bits(info->regmap, ITDS_REG_CTRL4, > + ITDS_MASK_INT_FIFOTH, 0); > + > + info->is_hwfifo_enabled = false; > + > + return 0; > +} > + > +static int itds_accel_buffer_postenable(struct iio_dev *indio_dev) > +{ > + struct itds_info *info = iio_priv(indio_dev); > + unsigned int rval; > + int ret; > + > + ret = regmap_read(info->regmap, ITDS_REG_CTRL1, &rval); > + if (ret) > + return ret; > + > + if (!(rval & ITDS_MASK_ODR)) > + return -EINVAL; > + > + ret = regmap_update_bits(info->regmap, ITDS_REG_FIFO_CTRL, > + ITDS_MASK_FIFOTH, info->fifo_threshold); > + if (ret) > + return ret; > + > + ret = regmap_update_bits(info->regmap, ITDS_REG_FIFO_CTRL, > + ITDS_MASK_FIFOMODE, > + ITDS_FIFO_MODE_FIFO); > + if (ret) > + return ret; > + > + info->is_hwfifo_enabled = true; > + if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) > + iio_triggered_buffer_postenable(indio_dev); > + > + return 0; > +} > + > +static int itds_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int *val, > + int *val2, long mask) > +{ > + struct itds_info *info = iio_priv(indio_dev); > + int ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + if (chan->type == IIO_ACCEL) { > + ret = iio_device_claim_direct_mode(indio_dev); > + if (ret) > + return ret; > + > + ret = itds_get_accel(info, chan, val); > + iio_device_release_direct_mode(indio_dev); > + } else { > + ret = itds_get_temp(info, val); > + } > + return ret; > + > + case IIO_CHAN_INFO_SCALE: > + *val = 0; > + if (chan->type == IIO_ACCEL) > + *val2 = info->scale; > + else > + *val2 = 100; > + return IIO_VAL_INT_PLUS_MICRO; > + > + case IIO_CHAN_INFO_SAMP_FREQ: > + ret = itds_get_odr(info, val, val2); > + return ret; > + > + case IIO_CHAN_INFO_OFFSET: > + if (chan->type == IIO_TEMP) > + *val = 250000; If other channel types don't have an offset they shouldn't register one or call this function. Hence that check should either be resulting in an error return for != IIO_TEMP (if you are feeling paranoid) or not there at all. > + return IIO_VAL_INT; > + > + default: > + return -EINVAL; > + } > +} > + > +static int itds_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int val, int val2, long mask) > +{ > + struct itds_info *info = iio_priv(indio_dev); > + > + switch (mask) { > + case IIO_CHAN_INFO_SAMP_FREQ: > + return itds_set_odr(info, val, val2); > + > + case IIO_CHAN_INFO_SCALE: > + return itds_set_scale(info, val); > + > + default: > + return -EINVAL; > + } > +} > + > +static const struct iio_trigger_ops itds_trigger_ops = { > + .set_trigger_state = itds_set_trigger_state, > +}; > + > +static const struct iio_buffer_setup_ops itds_accel_buffer_ops = { > + .postenable = itds_accel_buffer_postenable, > + .predisable = itds_accel_buffer_predisable, > +}; > + > +static const struct iio_info itds_accel_info = { > + .attrs = &itds_attrs_group, > + .read_raw = itds_read_raw, > + .write_raw = itds_write_raw, > + .hwfifo_flush_to_buffer = itds_fifo_flush, > + .hwfifo_set_watermark = itds_accel_set_fifo_threshold, > +}; > + > +static void itds_regulator_disable(void *data) > +{ > + struct itds_info *info = data; > + > + regmap_update_bits(info->regmap, ITDS_REG_CTRL7, ITDS_MASK_INT_EN, 0); > + regmap_update_bits(info->regmap, ITDS_REG_CTRL1, ITDS_MASK_MODE, 0); Every step in here is the result of a different part of probe. Hence you register a separate callback for each one so that we don't have to mess around cleaning up 'parts' of it on an error in probe. Key thing is that you don't want a reviewer to have to think 'are there any race conditions in here'. Right now btw you have paths with unbalanced enable and disable of regulators.. > + if (!IS_ERR_OR_NULL(info->vdd_supply)) > + regulator_disable(info->vdd_supply); > + > + if (!IS_ERR_OR_NULL(info->vddio_supply)) > + regulator_disable(info->vddio_supply); > +} > + > +static int itds_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct iio_dev *indio_dev; > + struct itds_info *info; > + struct regmap *regmap; > + unsigned int rval; > + int ret; > + > + indio_dev = devm_iio_device_alloc(dev, sizeof(*info)); > + if (!indio_dev) > + return -ENOMEM; > + > + regmap = devm_regmap_init_i2c(client, &itds_regmap_config); > + if (IS_ERR(regmap)) { > + dev_err(dev, "Failed to allocate regmap!\n"); > + return PTR_ERR(regmap); > + } > + > + info = iio_priv(indio_dev); > + info->regmap = regmap; > + info->dev = dev; > + i2c_set_clientdata(client, indio_dev); > + mutex_init(&info->mutex); > + > + info->vdd_supply = devm_regulator_get_optional(dev, "vdd"); > + info->vddio_supply = devm_regulator_get_optional(dev, "vddio"); I'm still really not happy with this. They are not optional regulators, but we may not be able to set their voltages. I'd rather see us just handle the error case if that happens at the set voltage stage. > + > + if (!IS_ERR_OR_NULL(info->vdd_supply) && > + !IS_ERR_OR_NULL(info->vddio_supply)) { > + > + ret = regulator_set_voltage(info->vdd_supply, 1700000, 3600000); > + if (ret) > + return ret; > + > + ret = regulator_set_voltage(info->vddio_supply, > + 1200000, 3700000); > + if (ret) > + return ret; > + > + ret = regulator_enable(info->vdd_supply); > + if (ret) { > + dev_err(dev, "Failed to enable vdd: %d\n", ret); > + return ret; > + } Should have a separate calls to devm_add_action_or_reset for each regulator. That way the code is 'obviously' correct and there is no need to do manual cleanup in the error paths in probe. > + > + ret = regulator_enable(info->vddio_supply); > + if (ret) { > + dev_err(dev, "Failed to enable vddio: %d\n", ret); > + goto out_disable_vdd; > + } > + > + /* chip boot sequence takes 20ms */ > + usleep_range(20000, 21000); > + } > + > + ret = devm_add_action_or_reset(dev, itds_regulator_disable, info); > + if (ret) > + return ret; > + > + ret = regmap_read(info->regmap, ITDS_REG_DEV_ID, &rval); > + if (ret) { > + dev_info(dev, "device id read err %d\n", ret); > + goto out_disable_vddio; > + } > + > + if (rval != ITDS_DEVICE_ID) { > + dev_info(dev, "device id %X not matched\n", rval); > + goto out_disable_vddio; > + } > + > + ret = regmap_update_bits(info->regmap, ITDS_REG_CTRL2, > + ITDS_MASK_BDU_INC_ADD, ITDS_MASK_BDU_INC_ADD); > + if (ret) { > + dev_err(dev, "unable to set block data update!\n"); > + goto out_disable_vddio; > + } > + > + ret = regmap_update_bits(info->regmap, ITDS_REG_WAKEUP_EVENT, 0xff, 0); > + if (ret) { > + dev_err(dev, "disable wakeup event fail!\n"); > + goto out_disable_vddio; > + } > + > + indio_dev->name = dev_name(dev); > + indio_dev->dev.parent = dev; > + indio_dev->channels = itds_channels; > + indio_dev->num_channels = ARRAY_SIZE(itds_channels); > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->available_scan_masks = itds_scan_masks; > + indio_dev->info = &itds_accel_info; > + > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, > + NULL, > + itds_trigger_handler, > + &itds_accel_buffer_ops); > + if (ret) { > + dev_err(dev, "unable to setup iio triggered buffer\n"); > + goto out_disable_vddio; > + } > + > + if (client->irq > 0) { > + ret = devm_request_threaded_irq(dev, client->irq, > + itds_accel_irq_handler, > + itds_accel_irq_thread_handler, > + IRQF_TRIGGER_RISING, > + "itds_event", indio_dev); > + if (ret) { > + dev_err(dev, "unable to request IRQ\n"); > + goto out_disable_vddio; > + } > + > + info->trig = devm_iio_trigger_alloc(dev, "%s-dev%d", > + indio_dev->name, indio_dev->id); > + if (!info->trig) { > + ret = -ENOMEM; > + goto out_disable_vddio; > + } > + > + info->trig->dev.parent = dev; > + info->trig->ops = &itds_trigger_ops; > + iio_trigger_set_drvdata(info->trig, indio_dev); > + indio_dev->trig = iio_trigger_get(info->trig); > + > + ret = devm_iio_trigger_register(dev, info->trig); > + if (ret) > + goto out_disable_vddio; > + > + ret = regmap_update_bits(info->regmap, ITDS_REG_CTRL7, > + ITDS_MASK_INT_EN, ITDS_MASK_INT_EN); > + if (ret) > + goto out_disable_vddio; > + } > + > + dev_set_drvdata(dev, indio_dev); > + ret = devm_iio_device_register(dev, indio_dev); > + if (ret) { > + dev_err(dev, "IIO device register fail: %d\n", ret); > + goto out_disable_vddio; > + } > + > + return 0; > + > +out_disable_vddio: > + if (!IS_ERR_OR_NULL(info->vddio_supply)) > + regulator_disable(info->vddio_supply); You shouldn't need this error handling. You have registered the regulator disables with the device managed framework so they'll be disabled automatically on error return from the probe function. > + > +out_disable_vdd: > + if (!IS_ERR_OR_NULL(info->vdd_supply)) > + regulator_disable(info->vdd_supply); > + > + return ret; > +} > + > +static const struct of_device_id itds_of_match[] = { > + { .compatible = "we,wsen-itds"}, > + {} > +}; > +MODULE_DEVICE_TABLE(of, itds_of_match); > + > +static const struct i2c_device_id itds_id[] = { > + { "wsen-itds", }, > + {} So the id table is for old style probing from board files or the sysfs interface. Now, in current kernel we have i2c_of_match_device which will successfully match a device against the bit after the compatible in the of_table above. So I haven't tested bit but I 'think' you are fine to drop the itds_id table entirely. (I came across this code in another review today so not totally sure I'm right here ;) > +}; > +MODULE_DEVICE_TABLE(i2c, itds_id); > + > +static struct i2c_driver itds_driver = { > + .driver = { > + .name = "wsen-itds", > + .of_match_table = itds_of_match, > + }, > + .probe_new = itds_probe, > + .id_table = itds_id, > +}; > +module_i2c_driver(itds_driver); > + > +MODULE_AUTHOR("Saravanan Sekar <saravanan@linumiz.com>"); > +MODULE_DESCRIPTION("Würth Elektronik WSEN-ITDS 3-axis accel driver"); > +MODULE_LICENSE("GPL"); ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 3/4] iio: accel: wsen-itds accel documentation 2020-04-29 13:39 [PATCH v2 0/4] Add driver for wsen-itds accelerometer sensor Saravanan Sekar 2020-04-29 13:39 ` [PATCH v2 1/4] dt-bindings: iio: add document bindings for wsen-itds accel sensor Saravanan Sekar 2020-04-29 13:39 ` [PATCH v2 2/4] iio: accel: Add driver for wsen-itds accelerometer sensor Saravanan Sekar @ 2020-04-29 13:39 ` Saravanan Sekar 2020-05-03 12:01 ` Jonathan Cameron 2020-04-29 13:39 ` [PATCH v2 4/4] MAINTAINERS: Add entry for wsen-itds accelerometer sensor Saravanan Sekar 3 siblings, 1 reply; 10+ messages in thread From: Saravanan Sekar @ 2020-04-29 13:39 UTC (permalink / raw) To: robh+dt, jic23, knaack.h, lars, pmeerw, broonie, lgirdwood, saravanan Cc: devicetree, linux-kernel, linux-iio Add documentation about device operating mode and output data range supported according to operating mode Signed-off-by: Saravanan Sekar <saravanan@linumiz.com> --- .../ABI/testing/sysfs-bus-iio-wsen-itds | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-wsen-itds diff --git a/Documentation/ABI/testing/sysfs-bus-iio-wsen-itds b/Documentation/ABI/testing/sysfs-bus-iio-wsen-itds new file mode 100644 index 000000000000..5979f2b8aa1a --- /dev/null +++ b/Documentation/ABI/testing/sysfs-bus-iio-wsen-itds @@ -0,0 +1,23 @@ +What: /sys/bus/iio/devices/iio\:device0/in_accel_samp_freq_available +KernelVersion: 5.7 +Contact: linux-iio@vger.kernel.org +Description: + Reading gives range of sample frequencies available for current operating mode + after one data has generated. + + Access: Read + Valid values: represented in Hz + - range [12.5, 1600] for high permormance mode + - range [1.6, 200] for normal/low power mode + +What: /sys/bus/iio/devices/iio\:device0/operating_mode +KernelVersion: 5.7 +Contact: linux-iio@vger.kernel.org +Description: + Represents the device operating mode. High performance mode gives high output + data rate and low noise compared to normal mode. Normal mode consumes less + current. In single shot device enters to lowpower after one data has + generated. + + Access: Read, Write + Valid values: "lowpower", "normal", "high_perf", "single_shot" -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/4] iio: accel: wsen-itds accel documentation 2020-04-29 13:39 ` [PATCH v2 3/4] iio: accel: wsen-itds accel documentation Saravanan Sekar @ 2020-05-03 12:01 ` Jonathan Cameron 2020-05-10 18:11 ` Saravanan Sekar 0 siblings, 1 reply; 10+ messages in thread From: Jonathan Cameron @ 2020-05-03 12:01 UTC (permalink / raw) To: Saravanan Sekar Cc: robh+dt, knaack.h, lars, pmeerw, broonie, lgirdwood, devicetree, linux-kernel, linux-iio On Wed, 29 Apr 2020 15:39:42 +0200 Saravanan Sekar <saravanan@linumiz.com> wrote: > Add documentation about device operating mode and output data range > supported according to operating mode > > Signed-off-by: Saravanan Sekar <saravanan@linumiz.com> > --- > .../ABI/testing/sysfs-bus-iio-wsen-itds | 23 +++++++++++++++++++ > 1 file changed, 23 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-wsen-itds > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-wsen-itds b/Documentation/ABI/testing/sysfs-bus-iio-wsen-itds > new file mode 100644 > index 000000000000..5979f2b8aa1a > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-bus-iio-wsen-itds > @@ -0,0 +1,23 @@ > +What: /sys/bus/iio/devices/iio\:device0/in_accel_samp_freq_available > +KernelVersion: 5.7 > +Contact: linux-iio@vger.kernel.org > +Description: > + Reading gives range of sample frequencies available for current operating mode > + after one data has generated. This is standard ABI so should be the main docs, not here. It also takes absolute precedence over the power modes (as mentioned below, no standard userspace will be able to use those). So if the frequency is only available in high perf mode, then we change to high perf mode. > + > + Access: Read > + Valid values: represented in Hz > + - range [12.5, 1600] for high permormance mode > + - range [1.6, 200] for normal/low power mode > + > +What: /sys/bus/iio/devices/iio\:device0/operating_mode > +KernelVersion: 5.7 > +Contact: linux-iio@vger.kernel.org > +Description: > + Represents the device operating mode. High performance mode gives high output > + data rate and low noise compared to normal mode. Normal mode consumes less > + current. In single shot device enters to lowpower after one data has > + generated. > + > + Access: Read, Write > + Valid values: "lowpower", "normal", "high_perf", "single_shot" The issue with these sort of 'mode' interface is almost no userspace will ever use them. They are too unpredictable across different types of devices. Some of these should also not be exposed to userspace anyway as they are about 'how' you are using the driver. For example, if you aren't doing triggered capture then single_shot is almost always the correct option. Annoyingly I see high performance mode gives lower noise... So no need to expose single_shot to userspace. For the others we are just looking at different power vs speed and accuracy trade offs. Those are better exposed by what they effect. Here the big control for that is sampling frequency. So if we assume the user is never going to touch this control (if it's even there) then we probably want to assume they want the best possible accuracy for whatever frequency they are running at. So transition across the modes to provide that. Should we ever support low power mode? It sounds nice on paper, but in reality userspace won't use so I suspect we should just drop it - certainly in an initial patch submission (as it will hold up acceptance). Even if we did support it, as mentioned above ABI controls will take precedence so we are looking at a 'hint' not a control of mode. ABI is a pain, and we will put a lot of effort into not expanding it unless there is a good usecase plus no way of mapping to existing ABI. Jonathan ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/4] iio: accel: wsen-itds accel documentation 2020-05-03 12:01 ` Jonathan Cameron @ 2020-05-10 18:11 ` Saravanan Sekar 2020-05-11 17:30 ` Jonathan Cameron 0 siblings, 1 reply; 10+ messages in thread From: Saravanan Sekar @ 2020-05-10 18:11 UTC (permalink / raw) To: Jonathan Cameron Cc: robh+dt, knaack.h, lars, pmeerw, broonie, lgirdwood, devicetree, linux-kernel, linux-iio Hi Jonathan, On 03/05/20 2:01 pm, Jonathan Cameron wrote: > On Wed, 29 Apr 2020 15:39:42 +0200 > Saravanan Sekar<saravanan@linumiz.com> wrote: > >> Add documentation about device operating mode and output data range >> supported according to operating mode >> >> Signed-off-by: Saravanan Sekar<saravanan@linumiz.com> >> --- >> .../ABI/testing/sysfs-bus-iio-wsen-itds | 23 +++++++++++++++++++ >> 1 file changed, 23 insertions(+) >> create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-wsen-itds >> >> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-wsen-itds b/Documentation/ABI/testing/sysfs-bus-iio-wsen-itds >> new file mode 100644 >> index 000000000000..5979f2b8aa1a >> --- /dev/null >> +++ b/Documentation/ABI/testing/sysfs-bus-iio-wsen-itds >> @@ -0,0 +1,23 @@ >> +What: /sys/bus/iio/devices/iio\:device0/in_accel_samp_freq_available >> +KernelVersion: 5.7 >> +Contact: linux-iio@vger.kernel.org >> +Description: >> + Reading gives range of sample frequencies available for current operating mode >> + after one data has generated. > This is standard ABI so should be the main docs, not here. > It also takes absolute precedence over the power modes (as mentioned below, no > standard userspace will be able to use those). So if the frequency is > only available in high perf mode, then we change to high perf mode. > >> + >> + Access: Read >> + Valid values: represented in Hz >> + - range [12.5, 1600] for high permormance mode >> + - range [1.6, 200] for normal/low power mode >> + >> +What: /sys/bus/iio/devices/iio\:device0/operating_mode >> +KernelVersion: 5.7 >> +Contact: linux-iio@vger.kernel.org >> +Description: >> + Represents the device operating mode. High performance mode gives high output >> + data rate and low noise compared to normal mode. Normal mode consumes less >> + current. In single shot device enters to lowpower after one data has >> + generated. >> + >> + Access: Read, Write >> + Valid values: "lowpower", "normal", "high_perf", "single_shot" > The issue with these sort of 'mode' interface is almost no userspace will ever use them. > They are too unpredictable across different types of devices. I don't understand how can we assume or say no one will use this. The device supports multiple features and my understanding is driver should support according to device, not reverse. This is more or less device specific and no sure idea about bring all the device in one umbrella. > Some of these should also not be exposed to userspace anyway as they are about 'how' > you are using the driver. For example, if you aren't doing triggered capture then > single_shot is almost always the correct option. Annoyingly I see high performance > mode gives lower noise... > > So no need to expose single_shot to userspace. > > For the others we are just looking at different power vs speed and accuracy trade offs. > Those are better exposed by what they effect. Here the big control for that is > sampling frequency. > > So if we assume the user is never going to touch this control (if it's even there) > then we probably want to assume they want the best possible accuracy for whatever > frequency they are running at. So transition across the modes to provide that. > > Should we ever support low power mode? It sounds nice on paper, but in reality > userspace won't use so I suspect we should just drop it - certainly in an initial > patch submission (as it will hold up acceptance). Even if we did support > it, as mentioned above ABI controls will take precedence so we are looking > at a 'hint' not a control of mode. > > ABI is a pain, and we will put a lot of effort into not expanding it unless > there is a good usecase plus no way of mapping to existing ABI. Obviously without any reason or requirement device manufacture won't come-up these kind of feature. I will change the driver as you don't accept at least for initial version. Thanks, Saravanan ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/4] iio: accel: wsen-itds accel documentation 2020-05-10 18:11 ` Saravanan Sekar @ 2020-05-11 17:30 ` Jonathan Cameron 0 siblings, 0 replies; 10+ messages in thread From: Jonathan Cameron @ 2020-05-11 17:30 UTC (permalink / raw) To: Saravanan Sekar Cc: Jonathan Cameron, robh+dt, knaack.h, lars, pmeerw, broonie, lgirdwood, devicetree, linux-kernel, linux-iio On Sun, 10 May 2020 20:11:55 +0200 Saravanan Sekar <saravanan@linumiz.com> wrote: > Hi Jonathan, > > On 03/05/20 2:01 pm, Jonathan Cameron wrote: > > On Wed, 29 Apr 2020 15:39:42 +0200 > > Saravanan Sekar<saravanan@linumiz.com> wrote: > > > >> Add documentation about device operating mode and output data range > >> supported according to operating mode > >> > >> Signed-off-by: Saravanan Sekar<saravanan@linumiz.com> > >> --- > >> .../ABI/testing/sysfs-bus-iio-wsen-itds | 23 +++++++++++++++++++ > >> 1 file changed, 23 insertions(+) > >> create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-wsen-itds > >> > >> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-wsen-itds b/Documentation/ABI/testing/sysfs-bus-iio-wsen-itds > >> new file mode 100644 > >> index 000000000000..5979f2b8aa1a > >> --- /dev/null > >> +++ b/Documentation/ABI/testing/sysfs-bus-iio-wsen-itds > >> @@ -0,0 +1,23 @@ > >> +What: /sys/bus/iio/devices/iio\:device0/in_accel_samp_freq_available > >> +KernelVersion: 5.7 > >> +Contact: linux-iio@vger.kernel.org > >> +Description: > >> + Reading gives range of sample frequencies available for current operating mode > >> + after one data has generated. > > This is standard ABI so should be the main docs, not here. > > It also takes absolute precedence over the power modes (as mentioned below, no > > standard userspace will be able to use those). So if the frequency is > > only available in high perf mode, then we change to high perf mode. > > > >> + > >> + Access: Read > >> + Valid values: represented in Hz > >> + - range [12.5, 1600] for high permormance mode > >> + - range [1.6, 200] for normal/low power mode > >> + > >> +What: /sys/bus/iio/devices/iio\:device0/operating_mode > >> +KernelVersion: 5.7 > >> +Contact: linux-iio@vger.kernel.org > >> +Description: > >> + Represents the device operating mode. High performance mode gives high output > >> + data rate and low noise compared to normal mode. Normal mode consumes less > >> + current. In single shot device enters to lowpower after one data has > >> + generated. > >> + > >> + Access: Read, Write > >> + Valid values: "lowpower", "normal", "high_perf", "single_shot" > > The issue with these sort of 'mode' interface is almost no userspace will ever use them. > > They are too unpredictable across different types of devices. > I don't understand how can we assume or say no one will use this. The > device supports multiple features > and my understanding is driver should support according to device, not > reverse. The aim of a subsystem and an ABI definition for such is to 'unify' and provide standard ways of doing things. It is against that 'standard' that userspace code will generally be written. So every time we define custom ABI for a device, we have basically written something that the vast majority of userspace code will never use. The only exception is code written for a particular device. So we have two options for features like this: 1) Map them to existing ABI wherever possible - ensure the code people have written already for userspace will do what they would expect. Here it's a case of providing 'reasonable' configuration for the full range of sampling frequencies. That may miss some corners that are optimal but we may have to live with that. 2) Define new ABI that is 'generic' so that future user space code can use it. The problem with power modes is they are extremely device specific, so this second option is one we have never succeeded with. In theory you could define a set of modes, but they would need to fully describing. As in you would need a record that tells userspace every characteristic that is affected by each mode. To put it bluntly software doesn't read data sheets. (nor for that matter to most userspace code developers :) > This is more or > less device specific and no sure idea about bring all the device in one > umbrella. We have existing userspace code - quite a bit of it. That code should work for your device. It doesn't know about your own magic interface, so it won't use it. That's the point of standardization around one interface, we want it to work. Note that the only way we make that happen is to review new ABI very carefully and often block it entirely. > > Some of these should also not be exposed to userspace anyway as they are about 'how' > > you are using the driver. For example, if you aren't doing triggered capture then > > single_shot is almost always the correct option. Annoyingly I see high performance > > mode gives lower noise... > > > > So no need to expose single_shot to userspace. > > > > For the others we are just looking at different power vs speed and accuracy trade offs. > > Those are better exposed by what they effect. Here the big control for that is > > sampling frequency. > > > > So if we assume the user is never going to touch this control (if it's even there) > > then we probably want to assume they want the best possible accuracy for whatever > > frequency they are running at. So transition across the modes to provide that. > > > > Should we ever support low power mode? It sounds nice on paper, but in reality > > userspace won't use so I suspect we should just drop it - certainly in an initial > > patch submission (as it will hold up acceptance). Even if we did support > > it, as mentioned above ABI controls will take precedence so we are looking > > at a 'hint' not a control of mode. > > > > ABI is a pain, and we will put a lot of effort into not expanding it unless > > there is a good usecase plus no way of mapping to existing ABI. > Obviously without any reason or requirement device manufacture won't > come-up these kind of feature. Agreed, there are clear reasons for doing this, but until we understand the target use case it is very hard to have a discussion about how an ABI will be expanded. > I will change the driver as you don't accept at least for initial version. Great. Thanks, Jonathan > > > Thanks, > Saravanan > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 4/4] MAINTAINERS: Add entry for wsen-itds accelerometer sensor 2020-04-29 13:39 [PATCH v2 0/4] Add driver for wsen-itds accelerometer sensor Saravanan Sekar ` (2 preceding siblings ...) 2020-04-29 13:39 ` [PATCH v2 3/4] iio: accel: wsen-itds accel documentation Saravanan Sekar @ 2020-04-29 13:39 ` Saravanan Sekar 3 siblings, 0 replies; 10+ messages in thread From: Saravanan Sekar @ 2020-04-29 13:39 UTC (permalink / raw) To: robh+dt, jic23, knaack.h, lars, pmeerw, broonie, lgirdwood, saravanan Cc: devicetree, linux-kernel, linux-iio Add MAINTAINERS entry for wsen-itds accelerometer sensor driver. Signed-off-by: Saravanan Sekar <saravanan@linumiz.com> --- MAINTAINERS | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 32a95d162f06..d70b31910cdb 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -18167,6 +18167,13 @@ F: drivers/watchdog/ F: include/linux/watchdog.h F: include/uapi/linux/watchdog.h +WSEN-ITDS THREE-AXIS DIGITAL ACCELEROMETER DRIVER +M: Saravanan Sekar <saravanan@linumiz.com> +S: Maintained +F: Documentation/ABI/testing/sysfs-bus-iio-wsen-itds +F: Documentation/devicetree/bindings/iio/accel/we,wsen-itds.yaml +F: drivers/iio/accel/wsen-itds.c + WHISKEYCOVE PMIC GPIO DRIVER M: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> L: linux-gpio@vger.kernel.org -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-05-12 14:45 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-29 13:39 [PATCH v2 0/4] Add driver for wsen-itds accelerometer sensor Saravanan Sekar 2020-04-29 13:39 ` [PATCH v2 1/4] dt-bindings: iio: add document bindings for wsen-itds accel sensor Saravanan Sekar 2020-05-12 14:45 ` Rob Herring 2020-04-29 13:39 ` [PATCH v2 2/4] iio: accel: Add driver for wsen-itds accelerometer sensor Saravanan Sekar 2020-05-03 12:42 ` Jonathan Cameron 2020-04-29 13:39 ` [PATCH v2 3/4] iio: accel: wsen-itds accel documentation Saravanan Sekar 2020-05-03 12:01 ` Jonathan Cameron 2020-05-10 18:11 ` Saravanan Sekar 2020-05-11 17:30 ` Jonathan Cameron 2020-04-29 13:39 ` [PATCH v2 4/4] MAINTAINERS: Add entry for wsen-itds accelerometer sensor Saravanan Sekar
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).