Message-ID: <20200418185413.07f13afc@archlinux>
Date:   Sat, 18 Apr 2020 18:54:13 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Saravanan Sekar <saravanan@...umiz.com>
Cc:     robh+dt@...nel.org, knaack.h@....de, lars@...afoo.de,
        pmeerw@...erw.net, broonie@...nel.org, lgirdwood@...il.com,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-iio@...r.kernel.org
Subject: Re: [PATCH 3/4] iio: accel: Add driver for wsen-itds accelerometer
 sensor

On Wed, 15 Apr 2020 08:55:34 +0200
Saravanan Sekar <saravanan@...umiz.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@...umiz.com>

There is a bunch of non standard looking ABI in here.  Without documentation
it is hard to review that (and docs are required anyway!)

Note that any new ABI that doesn't have good defaults is pretty much breaking
all existing userspace code. So if at all possible don't introduce any.
Sometimes that's a case of picking a sensible option for most usecase
and ignoring the many fun features of a device that no-one will use.
Sometimes it's a question of mapping to existing ABI if at all possible.

Various other comments inline.

Thanks,

Jonathan

> ---
>  drivers/iio/accel/Kconfig     |  14 +
>  drivers/iio/accel/Makefile    |   1 +
>  drivers/iio/accel/wsen-itds.c | 978 ++++++++++++++++++++++++++++++++++
>  3 files changed, 993 insertions(+)
>  create mode 100644 drivers/iio/accel/wsen-itds.c
> 
> diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> index 5d91a6dda894..9fbefc94e33e 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 provies 3-axis Acceleration, Tempreature 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..0846fb28a1b9
> --- /dev/null
> +++ b/drivers/iio/accel/wsen-itds.c
> @@ -0,0 +1,978 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * WSEN-ITDS 3-axis accel sensor driver
> + *
> + * Copyright 2020 Linumiz
> + *
> + * Author: Saravanan Sekar <saravanan@...umiz.com>
> + *
> + * TODO : events - 6D, Orientation, Free-fall, Tap, Motion
> + */
> +
> +#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/mutex.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_DEVICE_ID			0x44
> +#define	ITDS_ACCL_FIFO_SIZE		32
> +
> +#define ITDS_ACC_CHAN(_axis, _idx) {					\
> +	.type = IIO_ACCEL,						\
> +	.modified = 1,							\
> +	.channel2 = IIO_MOD_##_axis,					\
> +	.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_MODE_LOW_POWER,
> +	ITDS_MODE_NORMAL = BIT(0),
> +	ITDS_MODE_HIGH_PERFORMANCE = BIT(1),
> +	ITDS_MODE_SINGLE_SHOT = BIT(2),
> +	ITDS_MODE_INVALID = 4

INVALID is the same as SINGLE_SHOT BIT(2) == 4

Why is this a bitmap anyway?  You can't take more than one value
at a time I think?
> +};
> +
> +enum itds_fifo_mode {
> +	ITDS_MODE_BYPASS,
> +	ITDS_MODE_FIFO,
> +	ITDS_MODE_CONTINUES,
> +	ITDS_MODE_CONTINUES_TO_FIFO,
> +	ITDS_MODE_BYPASS_TO_CONTINUES,

Some of these have no standard representation in IIO. I would not
support them in initial driver.  If there are real usecases we can
work out later how to provide generic interfaces for them.

> +	ITDS_MODE_END
> +};
> +
> +struct itds_info {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct iio_trigger *trig;
> +	struct regulator *vdd_supply;
> +	struct regulator *vddio_supply;
> +	unsigned int scale;
> +	enum itds_operating_mode op_mode;
> +	enum itds_fifo_mode fifo_mode;
> +	struct mutex mutex;

All locks need documentation of what they are specifically protecting.

> +	int64_t timestamp;
> +	u8 fifo_threshold;
> +	bool is_hwfifo_enabled;
> +};
> +
> +struct itds_samp_freq {
> +	int val;
> +	int val2;
> +};
> +
> +struct itds_fifo_modes {
> +	char *str;
> +	int rval;
> +};
> +
> +static const char *operating_mode[ITDS_MODE_INVALID] = {
> +	"lowpower",
> +	"normal",
> +	"high_perf",
> +	"single_shot"

All of these should be implicit rather than directly controlled.
It will be a bit fiddly and you will fail to expose some of the
subtle nature fo the tradeoffs, but on the plus side userspace
can take advantage of the driver without having the user know 
exactly what the datasheet says.

> +};
> +
> +static const struct itds_samp_freq sample_freq[][10] = {

prefix sample_freq.  That could easily turn up in a header and cause
us trouble sometime in the future.

> +	{
> +		{0}, {1, 600000}, {12, 500000}, {25}, {50},
> +		{100}, {200}, {200}, {200}, {200}

0 is a bit pointless - I'd not expose that at all.
Also not a lot of point in exposing the multiple 200's .

> +	},
> +
> +	/* high performance mode */
> +	{

Is there an advantage in going to high performance mode for
less than 400?  Seems noise is better from datasheet but
at cost of some power.  You may want to expose less of
these subtle features in order to get a useable driver.
Perhaps so optional hint controls could be used to allow
the really interested to tweak the balance between power
and noise where the frequencies overlap.

> +		{0}, {12, 500000}, {12, 500000}, {25}, {50},
> +		{100}, {200}, {400}, {800}, {1600}
> +	}
> +};
> +
> +static const unsigned long itds_scan_masks[] = {0x7, 0};
> +
> +/* 2g, 4g, 8g, 16g */
> +unsigned int 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 itds_fifo_modes fifo_modes[ITDS_MODE_END] = {

No userspace is going to have a clue how to set these. You need to
link them in some fashion to standard concepts in IIO.

> +	{"bypass", 0},
> +	{"fifo", 1},
> +	{"continues", 3},
> +	{"continues_fifo", 4},
> +	{"bypass_continues", 6},
> +};
> +
> +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 mode;
> +
> +	mode = !!(info->op_mode & ITDS_MODE_HIGH_PERFORMANCE);
> +
> +	for (i = 0; i < ARRAY_SIZE(sensitivity_scale[0]); i++)
> +		len += sprintf(buf + len, "%d ", sensitivity_scale[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;
> +	bool mode;
> +
> +	mode = !!(info->op_mode & ITDS_MODE_HIGH_PERFORMANCE);
> +
> +	for (i = 0; i < ARRAY_SIZE(sample_freq[0]); i++) {
> +		len += sprintf(buf + len, "%d.%d ", sample_freq[mode][i].val,
> +					 sample_freq[mode][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 rval;
> +
> +	ret = regmap_read(info->regmap, ITDS_REG_STATUS_DETECT, &regval);
> +	if (ret)
> +		return ret;
> +
> +	if (!(regval & ITDS_EVENT_DRDY_T))
> +		return -EAGAIN;
> +
> +	ret = regmap_bulk_read(info->regmap, ITDS_REG_TEMP_L,
> +				   &rval, sizeof(rval));
> +	if (ret) {
> +		dev_err(info->dev, "failed to read TEMP reg\n");
> +		return ret;
> +	}
> +
> +	rval  = rval >> 4;
> +	regval = sign_extend32(le16_to_cpu(rval), 11);
> +	*val = regval * 625;

This already got comment on but should be fine as a scale value and
actually return the raw value.

> +
> +	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 reg, ret;
> +	__le16 rval;
> +
> +	if (info->is_hwfifo_enabled)
> +		return -EBUSY;

Given you enable the fifo along with the buffer a simple
iio_device_claim_direct_mode will give you race free protection
of this.

> +
> +	ret = regmap_read(info->regmap, ITDS_REG_STATUS, &regval);
> +	if (ret)
> +		return ret;
> +
> +	if (!(regval & ITDS_EVENT_DRDY))
> +		return -EAGAIN;
> +
> +	switch (chan->channel2) {
> +	case IIO_MOD_X:
> +		reg = ITDS_REG_X_OUT_L;
> +		break;
> +	case IIO_MOD_Y:
> +		reg = ITDS_REG_Y_OUT_L;
> +		break;
> +	case IIO_MOD_Z:
> +		reg = ITDS_REG_Z_OUT_L;
> +		break;
> +	default:
> +		dev_err(info->dev, "invalid axis modifier\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_bulk_read(info->regmap, reg, &rval, sizeof(rval));

bulk_read into a non dma safe buffer is a bad idea.  May
be fine on your platform but there are all sorts of fun hardware
types out that this will break.

> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&info->mutex);
> +
> +	if (info->op_mode & ITDS_MODE_HIGH_PERFORMANCE ||
> +	    info->op_mode & ITDS_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;
> +	bool mode;
> +
> +	mode = !!(info->op_mode & ITDS_MODE_HIGH_PERFORMANCE);
> +
> +	for (i = 0; i < ARRAY_SIZE(sample_freq[0]); i++) {
> +		if ((val == sample_freq[mode][i].val) &&
> +		    (val2 == sample_freq[mode][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 mode;
> +
> +	ret = regmap_read(info->regmap, ITDS_REG_CTRL1, &rval);
> +	if (ret)
> +		return ret;
> +
> +	rval = (rval & ITDS_MASK_ODR) >> 4;
> +	mode = !!(info->op_mode & ITDS_MODE_HIGH_PERFORMANCE);
> +	*val = sample_freq[mode][rval].val;
> +	*val2 = sample_freq[mode][rval].val2;
> +
> +	return IIO_VAL_INT_PLUS_MICRO;
> +}
> +
> +static int itds_set_scale(struct itds_info *info, int val)
> +{
> +	int i, ret = -EINVAL;
> +	bool mode;
> +
> +	mutex_lock(&info->mutex);
> +	mode = !!(info->op_mode & ITDS_MODE_HIGH_PERFORMANCE);
> +	for (i = 0; i < ARRAY_SIZE(sensitivity_scale[0]); i++) {
> +		if (val == sensitivity_scale[mode][i])
> +			break;
> +	}
> +
> +	if (i == ARRAY_SIZE(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 * sensitivity_scale[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 mode;
> +
> +	ret = regmap_read(info->regmap, ITDS_REG_CTRL6, &rval);
> +	if (ret)
> +		return ret;
> +
> +	rval = (rval & ITDS_MASK_SCALE) >> 4;
> +	mode = !!(info->op_mode & ITDS_MODE_HIGH_PERFORMANCE);
> +	info->scale = 1000 * sensitivity_scale[mode][rval];
> +
> +	return 0;
> +}
> +
> +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_EN : 0;
> +	ret = regmap_update_bits(info->regmap, ITDS_REG_CTRL7,
> +				 ITDS_MASK_INT_EN, enable);

Enabling and disabling global interrupts seems like a bad idea.  Perhaps
just fiddle with the mask to make this more adaptable as you add
more of the interrupt types?

> +	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;
> +	unsigned int rval;
> +	int ret, i;
> +	/* 3x16 bits data + timestamp 64 bits */

Timestamp needs to be 64 bit aligned...

> +	__le16 buffer[7];
> +	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 (!count)
> +		return 0;
> +
> +	samples = samples < count ? samples : count;
> +	for (i = 0; i < samples; i++) {
> +		ret = regmap_raw_read(info->regmap, ITDS_REG_X_OUT_L,
> +				      buffer, 3 * sizeof(u16));

Doing a raw_read means your buffer needs to be DMA safe (for spi)
It's not.

Look at the __cacheline_aligned tricks used in other drivers.
Note there is no generally safe way to do DMA to the stack so
don't.


> +		if (ret)
> +			return i;
> +
> +		iio_push_to_buffers_with_timestamp(indio_dev, buffer,
> +						   info->timestamp);

The problem here is that timestamp is going to be wrong for any but the
sample that resulted in the interrupt. 

If you are running off the dataready trigger then you 'should' only get
one sample here as long at the capture is keeping up.  For other
modes you can either not report the timestamp at all, or do some
approximation of working it out. There are various drivers doing this
with various levels of attempting to smooth the numbers out.


> +	}
> +
> +	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 ssize_t itds_accel_set_fifo_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 i;
> +
> +	mutex_lock(&info->mutex);
> +
> +	for (i = 0; i < ITDS_MODE_END; i++) {
> +		if (!strcmp(buf, fifo_modes[i].str)) {
> +			info->fifo_mode = i;
> +			goto ret_unlock;
> +		}
> +	}
> +	count = -EINVAL;
> +
> +ret_unlock:
> +	mutex_unlock(&info->mutex);
> +	return count;
> +}
> +
> +static ssize_t itds_accel_get_fifo_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->fifo_mode;
> +	mutex_unlock(&info->mutex);
> +
> +	return sprintf(buf, "%s\n", fifo_modes[mode].str);
> +}
> +
> +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_MODE_INVALID; 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(hwfifo_mode, 0644, itds_accel_get_fifo_mode,
> +			itds_accel_set_fifo_mode, 0);
> +static IIO_DEVICE_ATTR(operating_mode, 0644, itds_accel_get_operating_mode,
> +		       itds_accel_set_operating_mode, 0);

This is not standard ABI, so to even discuss this properly we need ABI docs
in Documentation/ABI/testing/sysfs-bus-iio-wsen-itds


> +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_hwfifo_mode.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->timestamp = iio_get_time_ns(indio_dev);

Note this trigger 'may' be used by other devices

> +		return IRQ_WAKE_THREAD;
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int itds_accel_buffer_postdisable(struct iio_dev *indio_dev)

Something is missbalanced as postdisable should undo what happens
in preenable not postenable as I think it does here.

> +{
> +	struct itds_info *info = iio_priv(indio_dev);
> +
> +	if (!info->is_hwfifo_enabled)
> +		return 0;

I'm fairly sure you can't get here without that being true..

> +
> +	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 = 0;

Always set so no need to initialize.

> +
> +	if (info->is_hwfifo_enabled)

Given this is function is only path that can cause that to happen
it looks to me like you can't get here with it being true.

> +		return 0;
> +
> +	if (!info->fifo_mode)
> +		return -EINVAL;
> +
> +	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)
> +		goto out_err;
> +
> +	ret = regmap_update_bits(info->regmap, ITDS_REG_FIFO_CTRL,
> +				 ITDS_MASK_FIFOMODE,
> +				 fifo_modes[info->fifo_mode].rval << 5);
> +	if (ret)
> +		goto out_err;
> +
> +	info->is_hwfifo_enabled = true;
> +	ret = regmap_update_bits(info->regmap, ITDS_REG_CTRL4,
> +				 ITDS_MASK_INT_FIFOTH,
> +				 ITDS_MASK_INT_FIFOTH);
> +	if (ret)
> +		goto disable_fifo;
> +
> +	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
> +		iio_triggered_buffer_postenable(indio_dev);
> +
> +	return 0;
> +
> +disable_fifo:
> +	regmap_update_bits(info->regmap, ITDS_REG_FIFO_CTRL,
> +				 ITDS_MASK_FIFOTH | ITDS_MASK_FIFOMODE, 0);
> +
> +out_err:
> +	return ret;
> +}
> +
> +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 = itds_get_accel(info, chan, val);
> +		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;
> +		else
> +			*val = 0;

If there is no offset we shouldn't expose the attribute in sysfs.
Hence this is probably an error path..

> +
> +		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 = iio_triggered_buffer_predisable,
> +	.postdisable = itds_accel_buffer_postdisable,
> +};
> +
> +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 int itds_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> +	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");

Not the aren't optional.  You may not have specified them (in which case
the regulator framework will provide stubs on the assumption they are
connected but just not described),  but if the thing has no power it has
no power.

> +
> +	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)
> +			goto out_err;
> +
> +		ret = regulator_set_voltage(info->vddio_supply,
> +						 1200000, 3700000);

Ah. This is why you are being very protective of your regulators.
They are stubbed out if regulator framework isn't built (return 0)

Now if they are simply not specified then I'm not totally sure what
happens.  I suspect simply dropping them from here and relying on
valid configuration is the best plan.

> +		if (ret)
> +			goto out_err;
> +
> +		ret = regulator_enable(info->vdd_supply);

Enable is fine for a stub regulator (it's a noop) so these don't need
protecting.

> +		if (ret) {
> +			dev_err(dev, "Failed to enable vdd: %d\n", ret);
> +			goto out_err;
> +		}
> +
> +		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 = regmap_read(info->regmap, ITDS_REG_DEV_ID, &rval);
> +	if (ret && (rval != ITDS_DEVICE_ID)) {
> +		dev_info(dev, "err %d device id %X not matched\n", ret, 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 = devm_iio_device_register(dev, indio_dev);
> +	if (ret) {
> +		dev_err(dev, "IIO device register fail: %d\n", ret);
> +		goto out_disable_vddio;
> +	}
> +
> +	dev_set_drvdata(dev, indio_dev);

You set clientdata above which I believe boils down to the same thing.

> +
> +	return 0;
> +
> +out_disable_vddio:
> +	if (!IS_ERR_OR_NULL(info->vddio_supply))

As below. there is no means of getting here where that condition matters
and it isn't safe to call regulator_disable (unless you have a bug :)

> +		regulator_disable(info->vddio_supply);
> +
> +out_disable_vdd:
> +	if (!IS_ERR_OR_NULL(info->vdd_supply))
> +		regulator_disable(info->vdd_supply);
> +
> +out_err:

No point in an gotos to here, just return directly where they occur.

> +	return ret;
> +}
> +
> +static int itds_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct itds_info *info = iio_priv(indio_dev);
> +
This immediately tells me you have a race condition.  The user interface
is removed when iio_device_register is unwound.  That happens 'after'
the end of remove when the managed cleanup occurs.  Hence you've turned
the power off but may still get reads from userspace.

You can't mix things that use managed cleanup with those that don't.
So 2 options.

Either call iio_device_register and then call iio_device_unregister at the
start of remove.. Or use devm_add_action_or_reset to add handlers for the
3 actions you need to do in remove immediately after their partner action
is done in probe.  Then you can get rid of the remove function entirely
and everything will be unwound automatically.

> +	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);

There should be no way of getting here that need that IS_ERR_OR_NULL protection.
Either:
1) you have regulators built in which case even without one being specified you'll
get a stub regulator and all will be fine.
2) you don't have regulators built in which case regulator_disable is a no-op
and it won't matter if you call it.
3) regulator is there and correctly enabled in which case you obviously want
to call the disable.

> +
> +	if (!IS_ERR_OR_NULL(info->vddio_supply))
> +		regulator_disable(info->vddio_supply);
> +
> +	return 0;
> +}
> +
> +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 = of_match_ptr(itds_of_match),

Don't use of_patch_ptr - it prevents ACPI PRP0001 and to do
it you would need ifdef protections on the table
https://www.kernel.org/doc/html/latest/firmware-guide/acpi/enumeration.html?highlight=PRP0001


		.of_match_table = itds_of_match, is fine.

> +	},
> +	.probe		= itds_probe,
> +	.remove		= itds_remove,
> +	.id_table	= itds_id,
> +};
> +module_i2c_driver(itds_driver);
> +
> +MODULE_AUTHOR("Saravanan Sekar <saravanan@...umiz.com>");
> +MODULE_DESCRIPTION("Würth Elektronik WSEN-ITDS 3-axis accel driver");
> +MODULE_LICENSE("GPL");

Powered by blists - more mailing lists