[v2,2/3] iio: pressure: Support ROHM BU1390

Commit Message

Matti Vaittinen Sept. 15, 2023, 6:56 a.m. UTC

Support for the ROHM BM1390 pressure sensor. The BM1390GLV-Z can measure
pressures ranging from 300 hPa to 1300 hPa with configurable measurement
averaging and internal FIFO. The sensor does also provide temperature
measurements.

Sensor does also contain IIR filter implemented in HW. The data-sheet
says the IIR filter can be configured to be "weak", "middle" or
"strong". Some RMS noise figures are provided in data sheet but no
accurate maths for the filter configurations is provided. Hence, the IIR
filter configuration is not supported by this driver and the filter is
configured to the "middle" setting (at least not for now).

The FIFO measurement mode is only measuring the pressure and not the
temperature. The driver measures temperature when FIFO is flushed and
simply uses the same measured temperature value to all reported
temperatures. This should not be a problem when temperature is not
changing very rapidly (several degrees C / second) but allows users to
get the temperature measurements from sensor without any additional logic.

This driver allows the sensor to be used in two muitually exclusive ways,

1. With trigger (data-ready IRQ).
In this case the FIFO is not used as we get data ready for each collected
sample. Instead, for each data-ready IRQ we read the sample from sensor
and push it to the IIO buffer.

2. With hardware FIFO and watermark IRQ.
In this case the data-ready is not used but we enable watermark IRQ. At
each watermark IRQ we go and read all samples in FIFO and push them to the
IIO buffer.

Signed-off-by: Matti Vaittinen <[e-mail]>

---
Revision history:
v1 => v2:
- prefer s64 over int64_t
- drop not needed handling of 2's complements
- plenty of styling changes
- drop dead code (write_raw)
- fix typos in comments
- explain trigger and FIFO usage in commit message
- do better job at cheking the return values
- ensure there's no race when checking if triggered buffer is used
  before enabling the FIFO
- print warning if register read fails at IRQ handler
- drop unnecessary warning if IRQ is not given
- explain why we prefer asynchronous probing
---
 drivers/iio/pressure/Kconfig       |   9 +
 drivers/iio/pressure/Makefile      |   1 +
 drivers/iio/pressure/rohm-bm1390.c | 899 +++++++++++++++++++++++++++++
 3 files changed, 909 insertions(+)
 create mode 100644 drivers/iio/pressure/rohm-bm1390.c

Comments

Christophe JAILLET Sept. 16, 2023, 8:01 a.m. UTC | #1

Le 15/09/2023 à 08:56, Matti Vaittinen a écrit :
> Support for the ROHM BM1390 pressure sensor. The BM1390GLV-Z can measure
> pressures ranging from 300 hPa to 1300 hPa with configurable measurement
> averaging and internal FIFO. The sensor does also provide temperature
> measurements.
> 
> Sensor does also contain IIR filter implemented in HW. The data-sheet
> says the IIR filter can be configured to be "weak", "middle" or
> "strong". Some RMS noise figures are provided in data sheet but no
> accurate maths for the filter configurations is provided. Hence, the IIR
> filter configuration is not supported by this driver and the filter is
> configured to the "middle" setting (at least not for now).
> 
> The FIFO measurement mode is only measuring the pressure and not the
> temperature. The driver measures temperature when FIFO is flushed and
> simply uses the same measured temperature value to all reported
> temperatures. This should not be a problem when temperature is not
> changing very rapidly (several degrees C / second) but allows users to
> get the temperature measurements from sensor without any additional logic.
> 
> This driver allows the sensor to be used in two muitually exclusive ways,
> 
> 1. With trigger (data-ready IRQ).
> In this case the FIFO is not used as we get data ready for each collected
> sample. Instead, for each data-ready IRQ we read the sample from sensor
> and push it to the IIO buffer.
> 
> 2. With hardware FIFO and watermark IRQ.
> In this case the data-ready is not used but we enable watermark IRQ. At
> each watermark IRQ we go and read all samples in FIFO and push them to the
> IIO buffer.
> 
> Signed-off-by: Matti Vaittinen <[e-mail]>
> 

...

> +struct bm1390_data_buf {
> +	u32 pressure;
> +	__be16 temp;

I've not looked in details so I'm not sure if related, but 
bm1390_read_temp() seems to use int.

> +	s64 ts __aligned(8);
> +};
> +
> +/* Pressure data is in 3 8-bit registers */
> +#define BM1390_PRESSURE_SIZE	3

Unused? (see other comment below)

> +
> +/* BM1390 has FIFO for 4 pressure samples */
> +#define BM1390_FIFO_LENGTH	4
> +
> +/* Temperature data is in 2 8-bit registers */
> +#define BM1390_TEMP_SIZE	2

Unused? (see other comment below)

...

> +static int bm1390_read_temp(struct bm1390_data *data, int *temp)
> +{
> +	__be16 temp_raw;

Something to do with BM1390_TEMP_SIZE?

> +	int ret;
> +
> +	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp_raw,
> +			       sizeof(temp_raw));
> +	if (ret)
> +		return ret;
> +
> +	*temp = be16_to_cpu(temp_raw);

See potential link with the comment above related to 
bm1390_data_buf.temp being a __be16 an temp being a int.

> +
> +	return 0;
> +}
> +
> +static int bm1390_pressure_read(struct bm1390_data *data, u32 *pressure)
> +{
> +	int ret;
> +	u8 raw[3];

BM1390_PRESSURE_SIZE?

(not sure if it make sense because we still have [0..2] below, so having 
3 here looks useful)

> +
> +	ret = regmap_bulk_read(data->regmap, BM1390_REG_PRESSURE_BASE,
> +			       raw, sizeof(raw));
> +	if (ret < 0)
> +		return ret;
> +
> +	*pressure = (u32)(raw[2] >> 2 | raw[1] << 6 | raw[0] << 14);
> +
> +	return 0;
> +}

...

> +static int bm1390_read_data(struct bm1390_data *data,
> +			struct iio_chan_spec const *chan, int *val, int *val2)
> +{
> +	int ret;
> +
> +	mutex_lock(&data->mutex);
> +	/*
> +	 * We use 'continuous mode' even for raw read because according to the
> +	 * data-sheet an one-shot mode can't be used with IIR filter.
> +	 */
> +	ret = bm1390_meas_set(data, BM1390_MEAS_MODE_CONTINUOUS);
> +	if (ret)
> +		goto unlock_out;
> +
> +	switch (chan->type) {
> +	case IIO_PRESSURE:
> +		msleep(BM1390_MAX_MEAS_TIME_MS);
> +		ret = bm1390_pressure_read(data, val);
> +		break;
> +	case IIO_TEMP:
> +		msleep(BM1390_MAX_MEAS_TIME_MS);
> +		ret = bm1390_read_temp(data, val);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +	bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);

"ret =" missing, or done on purpose?

> +unlock_out:
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}
> +
> +static int bm1390_read_raw(struct iio_dev *idev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct bm1390_data *data = iio_priv(idev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type == IIO_TEMP) {
> +			*val = 31;
> +			*val2 = 250000;
> +
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		} else if (chan->type == IIO_PRESSURE) {
> +			*val = 0;
> +			/*
> +			 * pressure in hPa is register value divided by 2048.
> +			 * This means kPa is 1/20480 times the register value,
> +			 * which equals to 48828.125 * 10 ^ -9
> +			 * This is 48828.125 nano kPa.
> +			 *
> +			 * When we scale this using IIO_VAL_INT_PLUS_NANO we
> +			 * get 48828 - which means we lose some accuracy. Well,
> +			 * let's try to live with that.
> +			 */
> +			*val2 = 48828;
> +
> +			return IIO_VAL_INT_PLUS_NANO;
> +		}
> +
> +		return -EINVAL;
> +	case IIO_CHAN_INFO_RAW:
> +		ret = iio_device_claim_direct_mode(idev);
> +		if (ret)
> +			return ret;
> +
> +		ret = bm1390_read_data(data, chan, val, val2);
> +		iio_device_release_direct_mode(idev);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;

Certainly useless, but should we break and return -EINVAL after the 
switch, so that it is more explicit that bm1390_read_raw() always 
returns a value?

> +	}
> +}
> +
> +static int __bm1390_fifo_flush(struct iio_dev *idev, unsigned int samples,
> +			       bool irq)
> +{
> +	struct bm1390_data *data = iio_priv(idev);
> +	struct bm1390_data_buf buffer;
> +	int smp_lvl, ret, i, warn;
> +	u64 sample_period;
> +	__be16 temp = 0;
> +
> +	/*
> +	 * If the IC is accessed during FIFO read samples can be dropped.
> +	 * Prevent access until FIFO_LVL is read
> +	 */
> +	if (test_bit(BM1390_CHAN_TEMP, idev->active_scan_mask)) {
> +		ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp,
> +				       sizeof(temp));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = regmap_read(data->regmap, BM1390_REG_FIFO_LVL, &smp_lvl);
> +	if (ret)
> +		return ret;
> +
> +	smp_lvl = FIELD_GET(BM1390_MASK_FIFO_LVL, smp_lvl);
> +	if (!smp_lvl)
> +		return 0;
> +
> +	if (smp_lvl > 4) {
> +		/*
> +		 * The fifo holds maximum of 4 samples so valid values
> +		 * should be 0, 1, 2, 3, 4 - rest are probably bit errors
> +		 * in I2C line. Don't overflow if this happens.
> +		 */
> +		dev_err(data->dev, "bad FIFO level %d\n", smp_lvl);
> +		smp_lvl = 4;
> +	}
> +
> +	sample_period = data->timestamp - data->old_timestamp;
> +	do_div(sample_period, smp_lvl);
> +
> +	if (samples && smp_lvl > samples)
> +		smp_lvl = samples;
> +
> +	for (i = 0; i < smp_lvl; i++) {
> +		ret = bm1390_pressure_read(data, &buffer.pressure);
> +		if (ret)
> +			break;
> +
> +		buffer.temp = temp;
> +		/*
> +		 * Old timestamp is either the previous sample IRQ time,
> +		 * previous flush-time or, if this was first sample, the enable
> +		 * time. When we add a sample period to that we should get the
> +		 * best approximation of the time-stamp we are handling.
> +		 *
> +		 * Idea is to always keep the "old_timestamp" matching the
> +		 * timestamp which we are currently handling.
> +		 */
> +		data->old_timestamp += sample_period;
> +
> +		iio_push_to_buffers_with_timestamp(idev, &buffer,
> +						   data->old_timestamp);
> +	}
> +	/* Reading the FIFO_LVL closes the FIFO access sequence */
> +	warn = regmap_read(data->regmap, BM1390_REG_FIFO_LVL, &smp_lvl);
> +	if (warn)
> +		dev_warn(data->dev, "Closing FIFO sequence failed\n");
> +
> +	if (!ret)

if (ret)?
If done on purpose "return 0;" would be more explicit.

> +		return ret;
> +
> +	return smp_lvl;
> +}

...

> +static int bm1390_setup_trigger(struct bm1390_data *data, struct iio_dev *idev,
> +				int irq)
> +{
> +	struct iio_trigger *itrig;
> +	char *name;
> +	int ret;
> +
> +	/* Nothing to do if we don't have IRQ for data-ready and WMI */
> +	if (irq < 0)
> +		return 0;
> +
> +	ret = devm_iio_triggered_buffer_setup(data->dev, idev,
> +					      &iio_pollfunc_store_time,
> +					      &bm1390_trigger_handler,
> +					      &bm1390_buffer_ops);
> +
> +	if (ret)
> +		return dev_err_probe(data->dev, ret,
> +				     "iio_triggered_buffer_setup FAIL\n");
> +
> +	itrig = devm_iio_trigger_alloc(data->dev, "%sdata-rdy-dev%d", idev->name,
> +					    iio_device_id(idev));
> +	if (!itrig)
> +		return -ENOMEM;
> +
> +	data->trig = itrig;
> +	idev->available_scan_masks = bm1390_scan_masks;
> +
> +	itrig->ops = &bm1390_trigger_ops;
> +	iio_trigger_set_drvdata(itrig, data);
> +
> +	name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-bm1390",
> +			      dev_name(data->dev));

Missing NULL check?

> +
> +	ret = devm_request_threaded_irq(data->dev, irq, bm1390_irq_handler,
> +					&bm1390_irq_thread_handler,
> +					IRQF_ONESHOT, name, idev);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
> +
> +
> +	ret = devm_iio_trigger_register(data->dev, itrig);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret,
> +				     "Trigger registration failed\n");
> +
> +	return 0;

...

Jonathan Cameron Sept. 17, 2023, 9:56 a.m. UTC | #2

On Sat, 16 Sep 2023 10:01:06 +0200
Christophe JAILLET <[e-mail]> wrote:

> Le 15/09/2023 à 08:56, Matti Vaittinen a écrit :
> > Support for the ROHM BM1390 pressure sensor. The BM1390GLV-Z can measure
> > pressures ranging from 300 hPa to 1300 hPa with configurable measurement
> > averaging and internal FIFO. The sensor does also provide temperature
> > measurements.
> > 
> > Sensor does also contain IIR filter implemented in HW. The data-sheet
> > says the IIR filter can be configured to be "weak", "middle" or
> > "strong". Some RMS noise figures are provided in data sheet but no
> > accurate maths for the filter configurations is provided. Hence, the IIR
> > filter configuration is not supported by this driver and the filter is
> > configured to the "middle" setting (at least not for now).
> > 
> > The FIFO measurement mode is only measuring the pressure and not the
> > temperature. The driver measures temperature when FIFO is flushed and
> > simply uses the same measured temperature value to all reported
> > temperatures. This should not be a problem when temperature is not
> > changing very rapidly (several degrees C / second) but allows users to
> > get the temperature measurements from sensor without any additional logic.
> > 
> > This driver allows the sensor to be used in two muitually exclusive ways,
> > 
> > 1. With trigger (data-ready IRQ).
> > In this case the FIFO is not used as we get data ready for each collected
> > sample. Instead, for each data-ready IRQ we read the sample from sensor
> > and push it to the IIO buffer.
> > 
> > 2. With hardware FIFO and watermark IRQ.
> > In this case the data-ready is not used but we enable watermark IRQ. At
> > each watermark IRQ we go and read all samples in FIFO and push them to the
> > IIO buffer.
> > 
> > Signed-off-by: Matti Vaittinen <[e-mail]>
> >   
> 
> ...
> 
> > +struct bm1390_data_buf {
> > +	u32 pressure;
> > +	__be16 temp;  
> 
> I've not looked in details so I'm not sure if related, but 
> bm1390_read_temp() seems to use int.
> 

I'll comment on this one below..

> > +	s64 ts __aligned(8);
> > +};
> > +
> > +/* Pressure data is in 3 8-bit registers */
> > +#define BM1390_PRESSURE_SIZE	3  
> 
> Unused? (see other comment below)
> 
> > +
> > +/* BM1390 has FIFO for 4 pressure samples */
> > +#define BM1390_FIFO_LENGTH	4
> > +
> > +/* Temperature data is in 2 8-bit registers */
> > +#define BM1390_TEMP_SIZE	2  
> 
> Unused? (see other comment below)
> 
> ...
> 
> > +static int bm1390_read_temp(struct bm1390_data *data, int *temp)
> > +{
> > +	__be16 temp_raw;  
> 
> Something to do with BM1390_TEMP_SIZE?

Yeah, better to drop that define as doesn't add anything.
Possibly the one for the 24bit (ish) field does given we can't just
use a type for that.

> 
> > +	int ret;
> > +
> > +	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp_raw,
> > +			       sizeof(temp_raw));
> > +	if (ret)
> > +		return ret;
> > +
> > +	*temp = be16_to_cpu(temp_raw);  
> 
> See potential link with the comment above related to 
> bm1390_data_buf.temp being a __be16 an temp being a int.

That is fine. They two are used in very different paths.
The hardware definition is indeed a __be16 and when pushed via the IIO
buffered interface we leave it like that.  See the fifo draining code.

This function is for read_raw() which is ultimately just used to provide
the sysfs reads.  As such, we just want he value to 'fit' and be in a form
that is printable (needs to match cpu endianness) as such this function
does the conversions - whereas for the buffered interface fed by the
fifo we make that a userspace problem.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int bm1390_pressure_read(struct bm1390_data *data, u32 *pressure)
> > +{
> > +	int ret;
> > +	u8 raw[3];  
> 
> BM1390_PRESSURE_SIZE?
> 
> (not sure if it make sense because we still have [0..2] below, so having 
> 3 here looks useful)
> 
> > +
> > +	ret = regmap_bulk_read(data->regmap, BM1390_REG_PRESSURE_BASE,
> > +			       raw, sizeof(raw));
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	*pressure = (u32)(raw[2] >> 2 | raw[1] << 6 | raw[0] << 14);
> > +
> > +	return 0;
> > +}  
> 
> ...
> 
> > +static int bm1390_read_data(struct bm1390_data *data,
> > +			struct iio_chan_spec const *chan, int *val, int *val2)
> > +{
> > +	int ret;
> > +
> > +	mutex_lock(&data->mutex);
> > +	/*
> > +	 * We use 'continuous mode' even for raw read because according to the
> > +	 * data-sheet an one-shot mode can't be used with IIR filter.
> > +	 */
> > +	ret = bm1390_meas_set(data, BM1390_MEAS_MODE_CONTINUOUS);
> > +	if (ret)
> > +		goto unlock_out;
> > +
> > +	switch (chan->type) {
> > +	case IIO_PRESSURE:
> > +		msleep(BM1390_MAX_MEAS_TIME_MS);
> > +		ret = bm1390_pressure_read(data, val);
> > +		break;
> > +	case IIO_TEMP:
> > +		msleep(BM1390_MAX_MEAS_TIME_MS);
> > +		ret = bm1390_read_temp(data, val);
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> > +	}
> > +	bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);  
> 
> "ret =" missing, or done on purpose?
> 
> > +unlock_out:
> > +	mutex_unlock(&data->mutex);
> > +
> > +	return ret;
> > +}
> > +
> > +static int bm1390_read_raw(struct iio_dev *idev,
> > +			   struct iio_chan_spec const *chan,
> > +			   int *val, int *val2, long mask)
> > +{
> > +	struct bm1390_data *data = iio_priv(idev);
> > +	int ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_SCALE:
> > +		if (chan->type == IIO_TEMP) {
> > +			*val = 31;
> > +			*val2 = 250000;
> > +
> > +			return IIO_VAL_INT_PLUS_MICRO;
> > +		} else if (chan->type == IIO_PRESSURE) {
> > +			*val = 0;
> > +			/*
> > +			 * pressure in hPa is register value divided by 2048.
> > +			 * This means kPa is 1/20480 times the register value,
> > +			 * which equals to 48828.125 * 10 ^ -9
> > +			 * This is 48828.125 nano kPa.
> > +			 *
> > +			 * When we scale this using IIO_VAL_INT_PLUS_NANO we
> > +			 * get 48828 - which means we lose some accuracy. Well,
> > +			 * let's try to live with that.
> > +			 */
> > +			*val2 = 48828;
> > +
> > +			return IIO_VAL_INT_PLUS_NANO;
> > +		}
> > +
> > +		return -EINVAL;
> > +	case IIO_CHAN_INFO_RAW:
> > +		ret = iio_device_claim_direct_mode(idev);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = bm1390_read_data(data, chan, val, val2);
> > +		iio_device_release_direct_mode(idev);
> > +		if (ret)
> > +			return ret;
> > +
> > +		return IIO_VAL_INT;
> > +	default:
> > +		return -EINVAL;  
> 
> Certainly useless, but should we break and return -EINVAL after the 
> switch, so that it is more explicit that bm1390_read_raw() always 
> returns a value?

I prefer this as it stands..  Compiler will catch a failure to return a value,
and this way it is clearer what we are basing decision for it being invalid on.

Jonathan

Jonathan Cameron Sept. 17, 2023, 10:35 a.m. UTC | #3

On Fri, 15 Sep 2023 09:56:19 +0300
Matti Vaittinen <[e-mail]> wrote:

> Support for the ROHM BM1390 pressure sensor. The BM1390GLV-Z can measure
> pressures ranging from 300 hPa to 1300 hPa with configurable measurement
> averaging and internal FIFO. The sensor does also provide temperature
> measurements.
> 
> Sensor does also contain IIR filter implemented in HW. The data-sheet
> says the IIR filter can be configured to be "weak", "middle" or
> "strong". Some RMS noise figures are provided in data sheet but no
> accurate maths for the filter configurations is provided. Hence, the IIR
> filter configuration is not supported by this driver and the filter is
> configured to the "middle" setting (at least not for now).
> 
> The FIFO measurement mode is only measuring the pressure and not the
> temperature. The driver measures temperature when FIFO is flushed and
> simply uses the same measured temperature value to all reported
> temperatures. This should not be a problem when temperature is not
> changing very rapidly (several degrees C / second) but allows users to
> get the temperature measurements from sensor without any additional logic.
> 
> This driver allows the sensor to be used in two muitually exclusive ways,
> 
> 1. With trigger (data-ready IRQ).
> In this case the FIFO is not used as we get data ready for each collected
> sample. Instead, for each data-ready IRQ we read the sample from sensor
> and push it to the IIO buffer.
> 
> 2. With hardware FIFO and watermark IRQ.
> In this case the data-ready is not used but we enable watermark IRQ. At
> each watermark IRQ we go and read all samples in FIFO and push them to the
> IIO buffer.
> 
> Signed-off-by: Matti Vaittinen <[e-mail]>

Hi Matti,

I think this is coming together nicely. A few comments inline.

Jonathan

> 
> ---
> Revision history:
> v1 => v2:
> - prefer s64 over int64_t
> - drop not needed handling of 2's complements
> - plenty of styling changes
> - drop dead code (write_raw)
> - fix typos in comments
> - explain trigger and FIFO usage in commit message
> - do better job at cheking the return values
> - ensure there's no race when checking if triggered buffer is used
>   before enabling the FIFO
> - print warning if register read fails at IRQ handler
> - drop unnecessary warning if IRQ is not given
> - explain why we prefer asynchronous probing
> ---

> diff --git a/drivers/iio/pressure/rohm-bm1390.c b/drivers/iio/pressure/rohm-bm1390.c
> new file mode 100644
> index 000000000000..d3cc229d1688
> --- /dev/null
> +++ b/drivers/iio/pressure/rohm-bm1390.c
> @@ -0,0 +1,899 @@

...

> +
> +static const unsigned long bm1390_scan_masks[] = {
> +	BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0
Why?  Doesn't look hard to support just one or the other?
Normally we only do this sort of limitation when there is a heavily
optimized read routine for a set of channels and it is better
to grab them all and throw away the ones we don't care about.
That doesn't seem to be true here. So if the fifo grabbed both
temp and pressure it would makes sense here, but doesn't seem
like it does.

> +};

> +static int bm1390_read_temp(struct bm1390_data *data, int *temp)
> +{
> +	__be16 temp_raw;
> +	int ret;
> +
> +	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp_raw,
> +			       sizeof(temp_raw));
> +	if (ret)
> +		return ret;
> +
> +	*temp = be16_to_cpu(temp_raw);
> +
> +	return 0;
> +}

> +static int bm1390_read_raw(struct iio_dev *idev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct bm1390_data *data = iio_priv(idev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type == IIO_TEMP) {
> +			*val = 31;
> +			*val2 = 250000;
> +
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		} else if (chan->type == IIO_PRESSURE) {
> +			*val = 0;
> +			/*
> +			 * pressure in hPa is register value divided by 2048.
> +			 * This means kPa is 1/20480 times the register value,
> +			 * which equals to 48828.125 * 10 ^ -9
> +			 * This is 48828.125 nano kPa.
> +			 *
> +			 * When we scale this using IIO_VAL_INT_PLUS_NANO we
> +			 * get 48828 - which means we lose some accuracy. Well,
> +			 * let's try to live with that.
> +			 */
> +			*val2 = 48828;
> +
> +			return IIO_VAL_INT_PLUS_NANO;

IIO_VAL_FRACTIONAL?  Mind you I'm not sure that will result in enough precision
either here.   For in kernel use it will have full precision though as will be
kept as a fraction.  I guess question of whether it is worse than what you have
here.  There hasn't been much demand for IIO_VAL_INTO_PLUS_PICO, but if we have
to look at that we can - with proviso that existing userspace software won't know
anything about it.

> +		}
> +
> +		return -EINVAL;
> +	case IIO_CHAN_INFO_RAW:
> +		ret = iio_device_claim_direct_mode(idev);
> +		if (ret)
> +			return ret;
> +
> +		ret = bm1390_read_data(data, chan, val, val2);
> +		iio_device_release_direct_mode(idev);
> +		if (ret)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int __bm1390_fifo_flush(struct iio_dev *idev, unsigned int samples,
> +			       bool irq)
> +{
> +	struct bm1390_data *data = iio_priv(idev);
> +	struct bm1390_data_buf buffer;
> +	int smp_lvl, ret, i, warn;
> +	u64 sample_period;
> +	__be16 temp = 0;
> +
> +	/*
> +	 * If the IC is accessed during FIFO read samples can be dropped.
> +	 * Prevent access until FIFO_LVL is read
Comment doesn't seem to have much to do with code that follows it?
Maybe needs more detail given you clearly access the IC before reading
FIFO_LVL.

> +	 */
> +	if (test_bit(BM1390_CHAN_TEMP, idev->active_scan_mask)) {
> +		ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp,
> +				       sizeof(temp));
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = regmap_read(data->regmap, BM1390_REG_FIFO_LVL, &smp_lvl);
> +	if (ret)
> +		return ret;
> +
> +	smp_lvl = FIELD_GET(BM1390_MASK_FIFO_LVL, smp_lvl);
> +	if (!smp_lvl)
> +		return 0;
> +
> +	if (smp_lvl > 4) {
> +		/*
> +		 * The fifo holds maximum of 4 samples so valid values
> +		 * should be 0, 1, 2, 3, 4 - rest are probably bit errors
> +		 * in I2C line. Don't overflow if this happens.
> +		 */
> +		dev_err(data->dev, "bad FIFO level %d\n", smp_lvl);
> +		smp_lvl = 4;
> +	}
> +
> +	sample_period = data->timestamp - data->old_timestamp;
> +	do_div(sample_period, smp_lvl);
> +
> +	if (samples && smp_lvl > samples)
> +		smp_lvl = samples;
> +
> +	for (i = 0; i < smp_lvl; i++) {
> +		ret = bm1390_pressure_read(data, &buffer.pressure);
> +		if (ret)
> +			break;
> +
> +		buffer.temp = temp;
> +		/*
> +		 * Old timestamp is either the previous sample IRQ time,
> +		 * previous flush-time or, if this was first sample, the enable
> +		 * time. When we add a sample period to that we should get the
> +		 * best approximation of the time-stamp we are handling.
> +		 *
> +		 * Idea is to always keep the "old_timestamp" matching the
> +		 * timestamp which we are currently handling.
> +		 */
> +		data->old_timestamp += sample_period;
> +
> +		iio_push_to_buffers_with_timestamp(idev, &buffer,
> +						   data->old_timestamp);
> +	}
> +	/* Reading the FIFO_LVL closes the FIFO access sequence */
> +	warn = regmap_read(data->regmap, BM1390_REG_FIFO_LVL, &smp_lvl);
> +	if (warn)
> +		dev_warn(data->dev, "Closing FIFO sequence failed\n");
> +
> +	if (!ret)
> +		return ret;
> +
> +	return smp_lvl;
> +}
> +
> +static int bm1390_fifo_flush(struct iio_dev *idev, unsigned int samples)
> +{
> +	struct bm1390_data *data = iio_priv(idev);
> +	int ret;
> +
> +	/*
> +	 * If fifo_flush is being called from IRQ handler we know the stored timestamp
> +	 * is fairly accurate for the last stored sample. If we are

Odd line wrapping.  Tidy this up to go near but not over 80 chars

> +	 * called as a result of a read operation from userspace and hence
> +	 * before the watermark interrupt was triggered, take a timestamp
> +	 * now. We can fall anywhere in between two samples so the error in this
> +	 * case is at most one sample period.
> +	 * We need to have the IRQ disabled or we risk of messing-up
> +	 * the timestamps. If we are ran from IRQ, then the
> +	 * IRQF_ONESHOT has us covered - but if we are ran by the
> +	 * user-space read we need to disable the IRQ to be on a safe
> +	 * side. We do this usng synchronous disable so that if the
> +	 * IRQ thread is being ran on other CPU we wait for it to be
> +	 * finished.

That irq disable is potentially expensive.
Why not just pass the current timestamp into the __bm1390_fifo_flush?

The locks should prevent other races I think..   
> +	 */
> +	disable_irq(data->irq);
> +	data->timestamp = iio_get_time_ns(idev);
> +
> +	mutex_lock(&data->mutex);

scoped_guard() will work nicely here

> +	ret = __bm1390_fifo_flush(idev, samples, false);
> +	mutex_unlock(&data->mutex);
> +
> +	enable_irq(data->irq);
> +
> +	return ret;
> +}

> +
> +static irqreturn_t bm1390_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *idev = pf->indio_dev;
> +	struct bm1390_data *data = iio_priv(idev);
> +	int ret, status;
> +
> +	/* DRDY is acked by reading status reg */
> +	ret = regmap_read(data->regmap, BM1390_REG_STATUS, &status);
> +	if (ret || !status)
> +		return IRQ_NONE;
> +
> +	dev_dbg(data->dev, "DRDY trig status 0x%x\n", status);
> +
> +	ret = bm1390_pressure_read(data, &data->buf.pressure);
> +	if (ret) {
> +		dev_warn(data->dev, "sample read failed %d\n", ret);
> +		return IRQ_NONE;
> +	}
> +
> +	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI,
> +			       &data->buf.temp, BM1390_TEMP_SIZE);
> +	if (ret)
> +		dev_warn(data->dev, "temp read failed %d\n", ret);

I don't want to see garbage (or a zero :) pushed into the buffer. So
if you get a read fail, don't push to buffers.

> +
> +	iio_push_to_buffers_with_timestamp(idev, &data->buf, data->timestamp);
> +	iio_trigger_notify_done(idev->trig);
> +
> +	return IRQ_HANDLED;
> +}


> +static int bm1390_trigger_set_state(struct iio_trigger *trig,
> +				    bool state)
> +{
> +	struct bm1390_data *data = iio_trigger_get_drvdata(trig);
> +	int ret = 0;
> +
> +	mutex_lock(&data->mutex);

Nice case for the new guard() calls which do automatic release of locks
on exit from a function - we also have scoped_guard for stuff inside
functions.

Here it will let you use direct returns throughout as lock will always
be released for you.


> +
> +	if (data->trigger_enabled == state)
> +		goto unlock_out;
> +
> +	if (data->state == BM1390_STATE_FIFO) {
> +		dev_warn(data->dev, "Can't set trigger when FIFO enabled\n");
> +		ret = -EBUSY;
> +		goto unlock_out;
> +	}
> +
> +	data->trigger_enabled = state;
> +
> +	if (state) {
> +		ret = bm1390_meas_set(data, BM1390_MEAS_MODE_CONTINUOUS);
> +		if (ret)
> +			goto unlock_out;
> +	} else {
> +		int dummy;
> +
> +		ret = bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);
> +		if (ret)
> +			goto unlock_out;
> +
> +		/*
> +		 * We need to read the status register in order to ACK the
> +		 * data-ready which may have been generated just before we
> +		 * disabled the measurement.
> +		 */
> +		ret = regmap_read(data->regmap, BM1390_REG_STATUS, &dummy);
> +		if (ret)
> +			dev_warn(data->dev, "status read failed\n");
> +	}
> +
> +	ret = bm1390_set_drdy_irq(data, state);
> +
> +unlock_out:
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}
> +
> +static const struct iio_trigger_ops bm1390_trigger_ops = {
> +	.set_trigger_state = bm1390_trigger_set_state,
> +};
> +
> +static int bm1390_setup_trigger(struct bm1390_data *data, struct iio_dev *idev,
> +				int irq)
> +{
> +	struct iio_trigger *itrig;
> +	char *name;
> +	int ret;
> +
> +	/* Nothing to do if we don't have IRQ for data-ready and WMI */
> +	if (irq < 0)
> +		return 0;

Matter of taste, so I won't argue strongly but I'd prefer to see this
check at the call site, not down in the function.  Makes the flow more
obvious and we don't have usual reasons for doing it this way
(either too much indenting at the call site, or lots of similar calls).

> +
> +	ret = devm_iio_triggered_buffer_setup(data->dev, idev,
> +					      &iio_pollfunc_store_time,
> +					      &bm1390_trigger_handler,
> +					      &bm1390_buffer_ops);

Why doesn't this still apply even if we don't have an irq for this device?
Can use a sysfs or hrtimer trigger or an irq on another device.

> +
> +	if (ret)
> +		return dev_err_probe(data->dev, ret,
> +				     "iio_triggered_buffer_setup FAIL\n");
> +
> +	itrig = devm_iio_trigger_alloc(data->dev, "%sdata-rdy-dev%d", idev->name,
> +					    iio_device_id(idev));
> +	if (!itrig)
> +		return -ENOMEM;
> +
> +	data->trig = itrig;
> +	idev->available_scan_masks = bm1390_scan_masks;

Mixing trigger and buffer stuff in here. I'd rather see them
separate - so move this up to where you set the buffer up.

> +
> +	itrig->ops = &bm1390_trigger_ops;
> +	iio_trigger_set_drvdata(itrig, data);
> +
> +	name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-bm1390",
> +			      dev_name(data->dev));

Check the allocation.  Definitely don't want name not defined here.

> +
> +	ret = devm_request_threaded_irq(data->dev, irq, bm1390_irq_handler,
> +					&bm1390_irq_thread_handler,
> +					IRQF_ONESHOT, name, idev);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
> +
> +
> +	ret = devm_iio_trigger_register(data->dev, itrig);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret,
> +				     "Trigger registration failed\n");
> +
> +	return 0;
> +}
> +
> +static int bm1390_probe(struct i2c_client *i2c)
> +{
> +	struct bm1390_data *data;
> +	struct regmap *regmap;
> +	struct iio_dev *idev;
> +	struct device *dev;
> +	unsigned int part_id;
> +	int ret;
> +
> +	dev = &i2c->dev;

Given it's unconditionally set and no line length issue, I'd prefer this
done on the local variable definitions block above.

	struct device *dev = &i2c->dev;
> +
> +	regmap = devm_regmap_init_i2c(i2c, &bm1390_regmap);
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(dev, PTR_ERR(regmap),
> +				     "Failed to initialize Regmap\n");
> +
> +	idev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!idev)
> +		return -ENOMEM;
> +
> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get regulator\n");
> +
> +	data = iio_priv(idev);
Why this ordering?  I'd expect this either:
1) Immediately after it becomes available so above the regulator enable,
or
2) immediately before it is used, so a few lines down above data->regmap...
Here it just looks a bit lost :)

> +
> +	ret = regmap_read(regmap, BM1390_REG_PART_ID, &part_id);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to access sensor\n");
> +
> +	if (part_id != BM1390_ID)
> +		dev_warn(dev, "unknown device 0x%x\n", part_id);
> +
> +	data->regmap = regmap;
> +	data->dev = dev;
> +	data->irq = i2c->irq;
> +	/*
> +	 * For now we just allow BM1390_WMI_MIN to BM1390_WMI_MAX and
> +	 * discard every other configuration when triggered mode is not used.
> +	 */
> +	data->watermark = BM1390_WMI_MAX;
> +	mutex_init(&data->mutex);
> +
> +	idev->channels = bm1390_channels;
> +	idev->num_channels = ARRAY_SIZE(bm1390_channels);
> +	idev->name = "bm1390";
> +	idev->info = &bm1390_info;
> +	idev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> +
> +	ret = bm1390_chip_init(data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "sensor init failed\n");
> +
> +	ret = bm1390_setup_trigger(data, idev, i2c->irq);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_iio_device_register(dev, idev);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret,
> +				     "Unable to register iio device\n");
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id bm1390_of_match[] = {
> +	{ .compatible = "rohm,bm1390glv-z" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, bm1390_of_match);
> +
> +static const struct i2c_device_id bm1390_id[] = {
> +	{ "bm1390glv-z", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, bm1390_id);
> +
> +static struct i2c_driver bm1390_driver = {
> +	.driver = {
> +		.name = "bm1390",
> +		.of_match_table = bm1390_of_match,
> +		/*
> +		 * The probe issues a few msleep()s - which can result
> +		 * measurable delays. Additionally, enabling the VDD may cause
> +		 * some (slight) delay depending on the ramp-rate of the
> +		 * regulator. Delays are propably magnitude of tens of
> +		 * milliseconds - but async probing should not be a problem so
> +		 * we should have nothing to lose here. Let's revise this if
> +		 * async probing proves to cause problems here.
> +		 */
Perhaps we can summarise this with just the sleep info? - don't need to
predict the future :)
		/*
		 * Probing explicitly requires a few millisecond of sleep.
	 	 * Enabling the VDD regulator may include ramp up rates.
		 */
> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,

Matti Vaittinen Sept. 18, 2023, 11:39 a.m. UTC | #4

Hi Christophe,

Thank you for taking a look at this! Much appreciated :)

On 9/16/23 11:01, Christophe JAILLET wrote:
> Le 15/09/2023 à 08:56, Matti Vaittinen a écrit :
>> Support for the ROHM BM1390 pressure sensor. The BM1390GLV-Z can measure
>> pressures ranging from 300 hPa to 1300 hPa with configurable measurement
>> averaging and internal FIFO. The sensor does also provide temperature
>> measurements.
>>
>> Sensor does also contain IIR filter implemented in HW. The data-sheet
>> says the IIR filter can be configured to be "weak", "middle" or
>> "strong". Some RMS noise figures are provided in data sheet but no
>> accurate maths for the filter configurations is provided. Hence, the IIR
>> filter configuration is not supported by this driver and the filter is
>> configured to the "middle" setting (at least not for now).
>>
>> The FIFO measurement mode is only measuring the pressure and not the
>> temperature. The driver measures temperature when FIFO is flushed and
>> simply uses the same measured temperature value to all reported
>> temperatures. This should not be a problem when temperature is not
>> changing very rapidly (several degrees C / second) but allows users to
>> get the temperature measurements from sensor without any additional 
>> logic.
>>
>> This driver allows the sensor to be used in two muitually exclusive ways,
>>
>> 1. With trigger (data-ready IRQ).
>> In this case the FIFO is not used as we get data ready for each collected
>> sample. Instead, for each data-ready IRQ we read the sample from sensor
>> and push it to the IIO buffer.
>>
>> 2. With hardware FIFO and watermark IRQ.
>> In this case the data-ready is not used but we enable watermark IRQ. At
>> each watermark IRQ we go and read all samples in FIFO and push them to 
>> the
>> IIO buffer.
>>
>> Signed-off-by: Matti Vaittinen 
>> <[e-mail]>
>>
> 
> ...
> 
>> +struct bm1390_data_buf {
>> +    u32 pressure;
>> +    __be16 temp;
> 
> I've not looked in details so I'm not sure if related, but 
> bm1390_read_temp() seems to use int.

The bm1390_read_temp() is only used for read_raw, and the u16 gets 
converted to int because this is what the IIO API uses for read_raw. 
Even the bm1390_read_temp() gets this into an u16 when data is read from 
hardware - because the (temperature) data in the hardware is 16 bits.

> 
>> +    s64 ts __aligned(8);
>> +};
>> +
>> +/* Pressure data is in 3 8-bit registers */
>> +#define BM1390_PRESSURE_SIZE    3
> 
> Unused? (see other comment below)
> 
>> +
>> +/* BM1390 has FIFO for 4 pressure samples */
>> +#define BM1390_FIFO_LENGTH    4
>> +
>> +/* Temperature data is in 2 8-bit registers */
>> +#define BM1390_TEMP_SIZE    2
> 
> Unused? (see other comment below)
> 
> ...
> 
>> +static int bm1390_read_temp(struct bm1390_data *data, int *temp)
>> +{
>> +    __be16 temp_raw;
> 
> Something to do with BM1390_TEMP_SIZE?
> 
>> +    int ret;
>> +
>> +    ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp_raw,
>> +                   sizeof(temp_raw));
>> +    if (ret)
>> +        return ret;
>> +
>> +    *temp = be16_to_cpu(temp_raw);
> 
> See potential link with the comment above related to 
> bm1390_data_buf.temp being a __be16 an temp being a int.

Thanks. And yes. I have (most likely) originally thought of reading the 
data in u8 arrays with the size of the defined data sizes. This idea was 
probably abandoned when discussing about this approach during v1 review 
- feedback from Andy and Jonathan was that the temperature data should 
be directly read to an u16 - and at this point using sizeof() seems more 
natural. So - this define is indeed useless now. Thanks for pointing it out!

>> +
>> +    return 0;
>> +}
>> +
>> +static int bm1390_pressure_read(struct bm1390_data *data, u32 *pressure)
>> +{
>> +    int ret;
>> +    u8 raw[3];
> 
> BM1390_PRESSURE_SIZE?
> 
> (not sure if it make sense because we still have [0..2] below, so having 
> 3 here looks useful)
> 

Umm... I see what you mean. Not sure which is better - so, unless there 
is strong opinion(s) I'll just drop the define.

>> +
>> +    ret = regmap_bulk_read(data->regmap, BM1390_REG_PRESSURE_BASE,
>> +                   raw, sizeof(raw));
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    *pressure = (u32)(raw[2] >> 2 | raw[1] << 6 | raw[0] << 14);
>> +
>> +    return 0;
>> +}
> 
> ...
> 
>> +static int bm1390_read_data(struct bm1390_data *data,
>> +            struct iio_chan_spec const *chan, int *val, int *val2)
>> +{
>> +    int ret;
>> +
>> +    mutex_lock(&data->mutex);
>> +    /*
>> +     * We use 'continuous mode' even for raw read because according 
>> to the
>> +     * data-sheet an one-shot mode can't be used with IIR filter.
>> +     */
>> +    ret = bm1390_meas_set(data, BM1390_MEAS_MODE_CONTINUOUS);
>> +    if (ret)
>> +        goto unlock_out;
>> +
>> +    switch (chan->type) {
>> +    case IIO_PRESSURE:
>> +        msleep(BM1390_MAX_MEAS_TIME_MS);
>> +        ret = bm1390_pressure_read(data, val);
>> +        break;
>> +    case IIO_TEMP:
>> +        msleep(BM1390_MAX_MEAS_TIME_MS);
>> +        ret = bm1390_read_temp(data, val);
>> +        break;
>> +    default:
>> +        ret = -EINVAL;
>> +    }
>> +    bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);
> 
> "ret =" missing, or done on purpose?

Done on purpose as the actual data was read successfully and should be 
correct. The measurement will be left running though - and it would 
probably warrant at least a warning. Thanks!

> 
>> +unlock_out:
>> +    mutex_unlock(&data->mutex);
>> +
>> +    return ret;
>> +}
>> +
>> +static int bm1390_read_raw(struct iio_dev *idev,
>> +               struct iio_chan_spec const *chan,
>> +               int *val, int *val2, long mask)
>> +{
>> +    struct bm1390_data *data = iio_priv(idev);
>> +    int ret;
>> +
>> +    switch (mask) {
>> +    case IIO_CHAN_INFO_SCALE:
>> +        if (chan->type == IIO_TEMP) {
>> +            *val = 31;
>> +            *val2 = 250000;
>> +
>> +            return IIO_VAL_INT_PLUS_MICRO;
>> +        } else if (chan->type == IIO_PRESSURE) {
>> +            *val = 0;
>> +            /*
>> +             * pressure in hPa is register value divided by 2048.
>> +             * This means kPa is 1/20480 times the register value,
>> +             * which equals to 48828.125 * 10 ^ -9
>> +             * This is 48828.125 nano kPa.
>> +             *
>> +             * When we scale this using IIO_VAL_INT_PLUS_NANO we
>> +             * get 48828 - which means we lose some accuracy. Well,
>> +             * let's try to live with that.
>> +             */
>> +            *val2 = 48828;
>> +
>> +            return IIO_VAL_INT_PLUS_NANO;
>> +        }
>> +
>> +        return -EINVAL;
>> +    case IIO_CHAN_INFO_RAW:
>> +        ret = iio_device_claim_direct_mode(idev);
>> +        if (ret)
>> +            return ret;
>> +
>> +        ret = bm1390_read_data(data, chan, val, val2);
>> +        iio_device_release_direct_mode(idev);
>> +        if (ret)
>> +            return ret;
>> +
>> +        return IIO_VAL_INT;
>> +    default:
>> +        return -EINVAL;
> 
> Certainly useless, but should we break and return -EINVAL after the 
> switch, so that it is more explicit that bm1390_read_raw() always 
> returns a value?

I think there is also opposite opinions on this. For my eyes the return 
at the end of the function would also be clearer - but I think I have 
been asked to drop the useless return when I've been working with other 
sensors in IIO domain :) My personal preference would definitely be:

int ret;

switch (foo)
{
case BAR:
	ret = func1();
	if (ret)
		break;

	ret = func2();
	if (ret)
		break;

...
	break;

case BAZ:
	ret = -EINVAL;
	break;
}

return ret;

- but I've learned to think this is not the IIO preference.

> 
>> +    }
>> +}
>> +
>> +static int __bm1390_fifo_flush(struct iio_dev *idev, unsigned int 
>> samples,
>> +                   bool irq)
>> +{
>> +    struct bm1390_data *data = iio_priv(idev);
>> +    struct bm1390_data_buf buffer;
>> +    int smp_lvl, ret, i, warn;
>> +    u64 sample_period;
>> +    __be16 temp = 0;
>> +
>> +    /*
>> +     * If the IC is accessed during FIFO read samples can be dropped.
>> +     * Prevent access until FIFO_LVL is read
>> +     */
>> +    if (test_bit(BM1390_CHAN_TEMP, idev->active_scan_mask)) {
>> +        ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp,
>> +                       sizeof(temp));
>> +        if (ret)
>> +            return ret;
>> +    }
>> +
>> +    ret = regmap_read(data->regmap, BM1390_REG_FIFO_LVL, &smp_lvl);
>> +    if (ret)
>> +        return ret;
>> +
>> +    smp_lvl = FIELD_GET(BM1390_MASK_FIFO_LVL, smp_lvl);
>> +    if (!smp_lvl)
>> +        return 0;
>> +
>> +    if (smp_lvl > 4) {
>> +        /*
>> +         * The fifo holds maximum of 4 samples so valid values
>> +         * should be 0, 1, 2, 3, 4 - rest are probably bit errors
>> +         * in I2C line. Don't overflow if this happens.
>> +         */
>> +        dev_err(data->dev, "bad FIFO level %d\n", smp_lvl);
>> +        smp_lvl = 4;
>> +    }
>> +
>> +    sample_period = data->timestamp - data->old_timestamp;
>> +    do_div(sample_period, smp_lvl);
>> +
>> +    if (samples && smp_lvl > samples)
>> +        smp_lvl = samples;
>> +
>> +    for (i = 0; i < smp_lvl; i++) {
>> +        ret = bm1390_pressure_read(data, &buffer.pressure);
>> +        if (ret)
>> +            break;
>> +
>> +        buffer.temp = temp;
>> +        /*
>> +         * Old timestamp is either the previous sample IRQ time,
>> +         * previous flush-time or, if this was first sample, the enable
>> +         * time. When we add a sample period to that we should get the
>> +         * best approximation of the time-stamp we are handling.
>> +         *
>> +         * Idea is to always keep the "old_timestamp" matching the
>> +         * timestamp which we are currently handling.
>> +         */
>> +        data->old_timestamp += sample_period;
>> +
>> +        iio_push_to_buffers_with_timestamp(idev, &buffer,
>> +                           data->old_timestamp);
>> +    }
>> +    /* Reading the FIFO_LVL closes the FIFO access sequence */
>> +    warn = regmap_read(data->regmap, BM1390_REG_FIFO_LVL, &smp_lvl);
>> +    if (warn)
>> +        dev_warn(data->dev, "Closing FIFO sequence failed\n");
>> +
>> +    if (!ret)
> 
> if (ret)?
> If done on purpose "return 0;" would be more explicit.
> 

I'm having a deja-vu. I may have missed similar comment when fixing 
things mentioned by Andy... I'll revise this - thanks!

>> +        return ret;
>> +
>> +    return smp_lvl;
>> +}
> 
> ...
> 
>> +static int bm1390_setup_trigger(struct bm1390_data *data, struct 
>> iio_dev *idev,
>> +                int irq)
>> +{
>> +    struct iio_trigger *itrig;
>> +    char *name;
>> +    int ret;
>> +
>> +    /* Nothing to do if we don't have IRQ for data-ready and WMI */
>> +    if (irq < 0)
>> +        return 0;
>> +
>> +    ret = devm_iio_triggered_buffer_setup(data->dev, idev,
>> +                          &iio_pollfunc_store_time,
>> +                          &bm1390_trigger_handler,
>> +                          &bm1390_buffer_ops);
>> +
>> +    if (ret)
>> +        return dev_err_probe(data->dev, ret,
>> +                     "iio_triggered_buffer_setup FAIL\n");
>> +
>> +    itrig = devm_iio_trigger_alloc(data->dev, "%sdata-rdy-dev%d", 
>> idev->name,
>> +                        iio_device_id(idev));
>> +    if (!itrig)
>> +        return -ENOMEM;
>> +
>> +    data->trig = itrig;
>> +    idev->available_scan_masks = bm1390_scan_masks;
>> +
>> +    itrig->ops = &bm1390_trigger_ops;
>> +    iio_trigger_set_drvdata(itrig, data);
>> +
>> +    name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-bm1390",
>> +                  dev_name(data->dev));
> 
> Missing NULL check?

Actually not. If name is NULL, the dev_name() will be used by the 
(devm_)request_threaded_irq(). I actually used to have a comment 
mentioning this but I was told the comment was longer than the NULL check...
...which was true. Still, the comment won't get compiled to code so I 
liked it more than the check.

Problem I have with the check is that this alloc returning NULL is 
extremely unlikely. So, it is going to be useless most of (all?) the 
time. And - if we for some reason did get NULL here - then it would not 
cause problems as the devm_request_threaded_irq() can handle NULL just fine.

I think Jonathan did also ask for this check to be added, so it is 
likely I am forced to do so even if I don't like it :) Still, in my 
books, the NULL check here is useless code.

> 
>> +
>> +    ret = devm_request_threaded_irq(data->dev, irq, bm1390_irq_handler,
>> +                    &bm1390_irq_thread_handler,
>> +                    IRQF_ONESHOT, name, idev);
>> +    if (ret)
>> +        return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
>> +
>> +
>> +    ret = devm_iio_trigger_register(data->dev, itrig);
>> +    if (ret)
>> +        return dev_err_probe(data->dev, ret,
>> +                     "Trigger registration failed\n");
>> +
>> +    return 0;

Thanks for taking the time to do the review! Much appreciated!

Yours,
	-- Matti

Matti Vaittinen Sept. 18, 2023, 12:56 p.m. UTC | #5

On 9/17/23 13:35, Jonathan Cameron wrote:
> On Fri, 15 Sep 2023 09:56:19 +0300
> Matti Vaittinen <[e-mail]> wrote:
> 
>> Support for the ROHM BM1390 pressure sensor. The BM1390GLV-Z can measure
>> pressures ranging from 300 hPa to 1300 hPa with configurable measurement
>> averaging and internal FIFO. The sensor does also provide temperature
>> measurements.
>>
>> Sensor does also contain IIR filter implemented in HW. The data-sheet
>> says the IIR filter can be configured to be "weak", "middle" or
>> "strong". Some RMS noise figures are provided in data sheet but no
>> accurate maths for the filter configurations is provided. Hence, the IIR
>> filter configuration is not supported by this driver and the filter is
>> configured to the "middle" setting (at least not for now).
>>
>> The FIFO measurement mode is only measuring the pressure and not the
>> temperature. The driver measures temperature when FIFO is flushed and
>> simply uses the same measured temperature value to all reported
>> temperatures. This should not be a problem when temperature is not
>> changing very rapidly (several degrees C / second) but allows users to
>> get the temperature measurements from sensor without any additional logic.
>>
>> This driver allows the sensor to be used in two muitually exclusive ways,
>>
>> 1. With trigger (data-ready IRQ).
>> In this case the FIFO is not used as we get data ready for each collected
>> sample. Instead, for each data-ready IRQ we read the sample from sensor
>> and push it to the IIO buffer.
>>
>> 2. With hardware FIFO and watermark IRQ.
>> In this case the data-ready is not used but we enable watermark IRQ. At
>> each watermark IRQ we go and read all samples in FIFO and push them to the
>> IIO buffer.
>>
>> Signed-off-by: Matti Vaittinen <[e-mail]>
> 
> Hi Matti,
> 
> I think this is coming together nicely. A few comments inline.

Thanks for the review (once again) :)

> 
>>
>> ---
>> Revision history:
>> v1 => v2:
>> - prefer s64 over int64_t
>> - drop not needed handling of 2's complements
>> - plenty of styling changes
>> - drop dead code (write_raw)
>> - fix typos in comments
>> - explain trigger and FIFO usage in commit message
>> - do better job at cheking the return values
>> - ensure there's no race when checking if triggered buffer is used
>>    before enabling the FIFO
>> - print warning if register read fails at IRQ handler
>> - drop unnecessary warning if IRQ is not given
>> - explain why we prefer asynchronous probing
>> ---
> 
>> diff --git a/drivers/iio/pressure/rohm-bm1390.c b/drivers/iio/pressure/rohm-bm1390.c
>> new file mode 100644
>> index 000000000000..d3cc229d1688
>> --- /dev/null
>> +++ b/drivers/iio/pressure/rohm-bm1390.c
>> @@ -0,0 +1,899 @@
> 
> ...
> 
>> +
>> +static const unsigned long bm1390_scan_masks[] = {
>> +	BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0
> Why?  Doesn't look hard to support just one or the other?
> Normally we only do this sort of limitation when there is a heavily
> optimized read routine for a set of channels and it is better
> to grab them all and throw away the ones we don't care about.
> That doesn't seem to be true here. So if the fifo grabbed both
> temp and pressure it would makes sense here, but doesn't seem
> like it does.

I have a feeling I have misunderstood how this mask works. I have 
assumed all the channels with corresponding mask bit _can_ be enabled 
simultaneously, but I have not understood they _must_ all be enabled. I 
think I must go back studying this, but if all channels really _must_ be 
enabled, then you are correct. It actually makes a lot of sense to 
support the pressure values alone, as, according to the data-sheet, the 
HW is doing a "MEMS temperature compensation" to the pressure values. 
So, my assuimption is the temperature data may not be required to be 
captured.

This also means I should revise the scan masks for the BU27008, BU27010 
and BU27034 light sensors as I don't think all the users want all the 
channels enabled. I wonder how I have not noticed any problems when I 
tested those things - did I really always enable all the channels...? @_@

Anyways, Thanks.

>> +};
> 
>> +static int bm1390_read_temp(struct bm1390_data *data, int *temp)
>> +{
>> +	__be16 temp_raw;
>> +	int ret;
>> +
>> +	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp_raw,
>> +			       sizeof(temp_raw));
>> +	if (ret)
>> +		return ret;
>> +
>> +	*temp = be16_to_cpu(temp_raw);
>> +
>> +	return 0;
>> +}
> 
>> +static int bm1390_read_raw(struct iio_dev *idev,
>> +			   struct iio_chan_spec const *chan,
>> +			   int *val, int *val2, long mask)
>> +{
>> +	struct bm1390_data *data = iio_priv(idev);
>> +	int ret;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_SCALE:
>> +		if (chan->type == IIO_TEMP) {
>> +			*val = 31;
>> +			*val2 = 250000;
>> +
>> +			return IIO_VAL_INT_PLUS_MICRO;
>> +		} else if (chan->type == IIO_PRESSURE) {
>> +			*val = 0;
>> +			/*
>> +			 * pressure in hPa is register value divided by 2048.
>> +			 * This means kPa is 1/20480 times the register value,
>> +			 * which equals to 48828.125 * 10 ^ -9
>> +			 * This is 48828.125 nano kPa.
>> +			 *
>> +			 * When we scale this using IIO_VAL_INT_PLUS_NANO we
>> +			 * get 48828 - which means we lose some accuracy. Well,
>> +			 * let's try to live with that.
>> +			 */
>> +			*val2 = 48828;
>> +
>> +			return IIO_VAL_INT_PLUS_NANO;
> 
> IIO_VAL_FRACTIONAL?  Mind you I'm not sure that will result in enough precision
> either here.   For in kernel use it will have full precision though as will be
> kept as a fraction.  I guess question of whether it is worse than what you have
> here.  There hasn't been much demand for IIO_VAL_INTO_PLUS_PICO, but if we have
> to look at that we can - with proviso that existing userspace software won't know
> anything about it.

I must take a look at the IIO_VAL_FRACTIONAL. I think either it or the 
NANO at this point. I have no understanding what is the precision that 
is enough. Hmm.. Would adding the PICO really require a change in 
user-space or could we hide it in-kernel when formatting the value we 
emit via sysfs?

> 
>> +		}
>> +
>> +		return -EINVAL;
>> +	case IIO_CHAN_INFO_RAW:
>> +		ret = iio_device_claim_direct_mode(idev);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = bm1390_read_data(data, chan, val, val2);
>> +		iio_device_release_direct_mode(idev);
>> +		if (ret)
>> +			return ret;
>> +
>> +		return IIO_VAL_INT;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int __bm1390_fifo_flush(struct iio_dev *idev, unsigned int samples,
>> +			       bool irq)
>> +{
>> +	struct bm1390_data *data = iio_priv(idev);
>> +	struct bm1390_data_buf buffer;
>> +	int smp_lvl, ret, i, warn;
>> +	u64 sample_period;
>> +	__be16 temp = 0;
>> +
>> +	/*
>> +	 * If the IC is accessed during FIFO read samples can be dropped.
>> +	 * Prevent access until FIFO_LVL is read
> Comment doesn't seem to have much to do with code that follows it?
> Maybe needs more detail given you clearly access the IC before reading
> FIFO_LVL.

I see now the comment is vague. I think I may originally had the mutex 
locked in this function and the comment has been preceding the lock.

Anyways, how the HW operates is that reading the first pressure value 
from the FIFO makes the sensor to 'start a FIFO access sequence'. So, 
after we read the first pressure value from the FIFO, we must not read 
any other register untill we have read all the pressure values and 
closed the 'FIFO access sequence' by reading the FIFO_LVL. If we do we 
may lose samples. I'll see how to explain this better.

> 
>> +	 */
>> +	if (test_bit(BM1390_CHAN_TEMP, idev->active_scan_mask)) {
>> +		ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp,
>> +				       sizeof(temp));
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	ret = regmap_read(data->regmap, BM1390_REG_FIFO_LVL, &smp_lvl);
>> +	if (ret)
>> +		return ret;
>> +
>> +	smp_lvl = FIELD_GET(BM1390_MASK_FIFO_LVL, smp_lvl);
>> +	if (!smp_lvl)
>> +		return 0;
>> +
>> +	if (smp_lvl > 4) {
>> +		/*
>> +		 * The fifo holds maximum of 4 samples so valid values
>> +		 * should be 0, 1, 2, 3, 4 - rest are probably bit errors
>> +		 * in I2C line. Don't overflow if this happens.
>> +		 */
>> +		dev_err(data->dev, "bad FIFO level %d\n", smp_lvl);
>> +		smp_lvl = 4;
>> +	}
>> +
>> +	sample_period = data->timestamp - data->old_timestamp;
>> +	do_div(sample_period, smp_lvl);
>> +
>> +	if (samples && smp_lvl > samples)
>> +		smp_lvl = samples;
>> +
>> +	for (i = 0; i < smp_lvl; i++) {
>> +		ret = bm1390_pressure_read(data, &buffer.pressure);
>> +		if (ret)
>> +			break;
>> +
>> +		buffer.temp = temp;
>> +		/*
>> +		 * Old timestamp is either the previous sample IRQ time,
>> +		 * previous flush-time or, if this was first sample, the enable
>> +		 * time. When we add a sample period to that we should get the
>> +		 * best approximation of the time-stamp we are handling.
>> +		 *
>> +		 * Idea is to always keep the "old_timestamp" matching the
>> +		 * timestamp which we are currently handling.
>> +		 */
>> +		data->old_timestamp += sample_period;
>> +
>> +		iio_push_to_buffers_with_timestamp(idev, &buffer,
>> +						   data->old_timestamp);
>> +	}
>> +	/* Reading the FIFO_LVL closes the FIFO access sequence */
>> +	warn = regmap_read(data->regmap, BM1390_REG_FIFO_LVL, &smp_lvl);
>> +	if (warn)
>> +		dev_warn(data->dev, "Closing FIFO sequence failed\n");
>> +
>> +	if (!ret)
>> +		return ret;
>> +
>> +	return smp_lvl;
>> +}
>> +
>> +static int bm1390_fifo_flush(struct iio_dev *idev, unsigned int samples)
>> +{
>> +	struct bm1390_data *data = iio_priv(idev);
>> +	int ret;
>> +
>> +	/*
>> +	 * If fifo_flush is being called from IRQ handler we know the stored timestamp
>> +	 * is fairly accurate for the last stored sample. If we are
> 
> Odd line wrapping.  Tidy this up to go near but not over 80 chars

Thanks!

> 
>> +	 * called as a result of a read operation from userspace and hence
>> +	 * before the watermark interrupt was triggered, take a timestamp
>> +	 * now. We can fall anywhere in between two samples so the error in this
>> +	 * case is at most one sample period.
>> +	 * We need to have the IRQ disabled or we risk of messing-up
>> +	 * the timestamps. If we are ran from IRQ, then the
>> +	 * IRQF_ONESHOT has us covered - but if we are ran by the
>> +	 * user-space read we need to disable the IRQ to be on a safe
>> +	 * side. We do this usng synchronous disable so that if the
>> +	 * IRQ thread is being ran on other CPU we wait for it to be
>> +	 * finished.
> 
> That irq disable is potentially expensive.
> Why not just pass the current timestamp into the __bm1390_fifo_flush >
> The locks should prevent other races I think..

Gah. I hate you Jonathan ;) (Not really!)

Actually, thank you (as always) for pointing this out. I don't instantly 
see why it wouldn't work, but going throught the IRQ races is never 
trivial (for me). It's work I've learned not to do at afternoon as my 
brains work better at the morning :) So, I will still go through this as 
a first thing tomorrow when I start my work day...


>> +	 */
>> +	disable_irq(data->irq);
>> +	data->timestamp = iio_get_time_ns(idev);
>> +
>> +	mutex_lock(&data->mutex);
> 
> scoped_guard() will work nicely here

Please, see my reply to Andy's comment. I'll paste it to this message 
below where you're also suggesting using these new 'autofreed when out 
of scope' thingies.

> 
>> +	ret = __bm1390_fifo_flush(idev, samples, false);
>> +	mutex_unlock(&data->mutex);
>> +
>> +	enable_irq(data->irq);
>> +
>> +	return ret;
>> +}
> 
>> +
>> +static irqreturn_t bm1390_trigger_handler(int irq, void *p)
>> +{
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *idev = pf->indio_dev;
>> +	struct bm1390_data *data = iio_priv(idev);
>> +	int ret, status;
>> +
>> +	/* DRDY is acked by reading status reg */
>> +	ret = regmap_read(data->regmap, BM1390_REG_STATUS, &status);
>> +	if (ret || !status)
>> +		return IRQ_NONE;
>> +
>> +	dev_dbg(data->dev, "DRDY trig status 0x%x\n", status);
>> +
>> +	ret = bm1390_pressure_read(data, &data->buf.pressure);
>> +	if (ret) {
>> +		dev_warn(data->dev, "sample read failed %d\n", ret);
>> +		return IRQ_NONE;
>> +	}
>> +
>> +	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI,
>> +			       &data->buf.temp, BM1390_TEMP_SIZE);
>> +	if (ret)
>> +		dev_warn(data->dev, "temp read failed %d\n", ret);
> 
> I don't want to see garbage (or a zero :) pushed into the buffer. So
> if you get a read fail, don't push to buffers.

Ok, makes sense.

> 
>> +
>> +	iio_push_to_buffers_with_timestamp(idev, &data->buf, data->timestamp);
>> +	iio_trigger_notify_done(idev->trig);
>> +
>> +	return IRQ_HANDLED;
>> +}
> 
> 
>> +static int bm1390_trigger_set_state(struct iio_trigger *trig,
>> +				    bool state)
>> +{
>> +	struct bm1390_data *data = iio_trigger_get_drvdata(trig);
>> +	int ret = 0;
>> +
>> +	mutex_lock(&data->mutex);
> 
> Nice case for the new guard() calls which do automatic release of locks
> on exit from a function - we also have scoped_guard for stuff inside
> functions.
> 
> Here it will let you use direct returns throughout as lock will always
> be released for you.
> 

Andy did also suggest using them to me. This is what I wrote as a reply 
to him - I'll copy it as a rationale why I'd rather did the good old 
goto-unwinding here at least for now...


 >> Wouldn't you like to start using cleanup.h?
 >
 > The new macro "thingee" for C++ destructor like constructs?

I was talking about these: guard() / scoped_guard().

 > TBH, I am not really in a rush with it for two reasons.
 > 1) The syntax looks very alien to me. I would like to get some time 
to get
 > used to it before voluntarily ending up maintaining a code using it. (I
 > don't like practicing things upstream as practicing tends to include 
making
 > more errors).
 >
 > 2. Related to 1). I am not (yet) convinced incorporating changes in stuff
 > using these cleanups is easy. I'm a bit reserved and would like to 
see how
 > things play out.
 >
 > 3. I expect I will get a few requests to backport the code to some older
 > kernels used by some big customers. (I've been doing plenty of extra work
 > when backporting my kernel improvements like regmap_irq stuff, linear
 > ranges, regulator pickable ranges, gts-helpers to customer kernels to 
get my
 > drivers working - or working around the lack of thiose features. I 
have been
 > willing to pay this prize to get those helpers upstream for everyone 
to use.
 > The cleanup.h however is there so it does not _need_ new users. I don't
 > think _all_ existing drivers will be converted to use it so one more 
should
 > probably not hurt. I think that in a year at least some customers will be
 > using kernel containing the cleanup.h - so by latest then it is time 
for me
 > to jump on that train. I hope I am used to reading those macros by then).

OK.


>> +
>> +	if (data->trigger_enabled == state)
>> +		goto unlock_out;
>> +
>> +	if (data->state == BM1390_STATE_FIFO) {
>> +		dev_warn(data->dev, "Can't set trigger when FIFO enabled\n");
>> +		ret = -EBUSY;
>> +		goto unlock_out;
>> +	}
>> +
>> +	data->trigger_enabled = state;
>> +
>> +	if (state) {
>> +		ret = bm1390_meas_set(data, BM1390_MEAS_MODE_CONTINUOUS);
>> +		if (ret)
>> +			goto unlock_out;
>> +	} else {
>> +		int dummy;
>> +
>> +		ret = bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);
>> +		if (ret)
>> +			goto unlock_out;
>> +
>> +		/*
>> +		 * We need to read the status register in order to ACK the
>> +		 * data-ready which may have been generated just before we
>> +		 * disabled the measurement.
>> +		 */
>> +		ret = regmap_read(data->regmap, BM1390_REG_STATUS, &dummy);
>> +		if (ret)
>> +			dev_warn(data->dev, "status read failed\n");
>> +	}
>> +
>> +	ret = bm1390_set_drdy_irq(data, state);
>> +
>> +unlock_out:
>> +	mutex_unlock(&data->mutex);
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct iio_trigger_ops bm1390_trigger_ops = {
>> +	.set_trigger_state = bm1390_trigger_set_state,
>> +};
>> +
>> +static int bm1390_setup_trigger(struct bm1390_data *data, struct iio_dev *idev,
>> +				int irq)
>> +{
>> +	struct iio_trigger *itrig;
>> +	char *name;
>> +	int ret;
>> +
>> +	/* Nothing to do if we don't have IRQ for data-ready and WMI */
>> +	if (irq < 0)
>> +		return 0;
> 
> Matter of taste, so I won't argue strongly but I'd prefer to see this
> check at the call site, not down in the function.  Makes the flow more
> obvious and we don't have usual reasons for doing it this way
> (either too much indenting at the call site, or lots of similar calls).

I think I can live with what you suggest here :)
> 
>> +
>> +	ret = devm_iio_triggered_buffer_setup(data->dev, idev,
>> +					      &iio_pollfunc_store_time,
>> +					      &bm1390_trigger_handler,
>> +					      &bm1390_buffer_ops);
> 
> Why doesn't this still apply even if we don't have an irq for this device?
> Can use a sysfs or hrtimer trigger or an irq on another device.
> 

Another good question. I know I wrote the code only thinking that when 
the trigger is used, it will be the data-ready from the device. I think 
I even used the trigger validation function which requires us to use the 
device's own trigger.

Now, whether it would make sense to allow other triggers, and whether 
the trigger handler has any data-ready specific handling here is 
something I need to revise. What does slightly bug me with allowing 
other triggers is that as far as I remmeber the BM1390 had no way to 
indicate that a single sample was actually ready. So, when we use any 
other trigger it may be we don't have a valid sample to read. (The 
read_raw uses a delay which should be long enough to guarantee a sample).

>> +
>> +	if (ret)
>> +		return dev_err_probe(data->dev, ret,
>> +				     "iio_triggered_buffer_setup FAIL\n");
>> +
>> +	itrig = devm_iio_trigger_alloc(data->dev, "%sdata-rdy-dev%d", idev->name,
>> +					    iio_device_id(idev));
>> +	if (!itrig)
>> +		return -ENOMEM;
>> +
>> +	data->trig = itrig;
>> +	idev->available_scan_masks = bm1390_scan_masks;
> 
> Mixing trigger and buffer stuff in here. I'd rather see them
> separate - so move this up to where you set the buffer up.

Thanks for pointing this out. In my mind the trigger and buffer went 
hand to hand with this device. Now, after your previous comment, I need 
to rethink if they really can be sepated. Anyways, moving this makes sense.

> 
>> +
>> +	itrig->ops = &bm1390_trigger_ops;
>> +	iio_trigger_set_drvdata(itrig, data);
>> +
>> +	name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-bm1390",
>> +			      dev_name(data->dev));
> 
> Check the allocation.  Definitely don't want name not defined here.

The request_threaded_irq will use the dev_name() if NULL is passed to 
it. I think it should be fine - but I think this is not worth fighting 
if you strongly prefer the check :)
> 
>> +
>> +	ret = devm_request_threaded_irq(data->dev, irq, bm1390_irq_handler,
>> +					&bm1390_irq_thread_handler,
>> +					IRQF_ONESHOT, name, idev);
>> +	if (ret)
>> +		return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
>> +
>> +
>> +	ret = devm_iio_trigger_register(data->dev, itrig);
>> +	if (ret)
>> +		return dev_err_probe(data->dev, ret,
>> +				     "Trigger registration failed\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static int bm1390_probe(struct i2c_client *i2c)
>> +{
>> +	struct bm1390_data *data;
>> +	struct regmap *regmap;
>> +	struct iio_dev *idev;
>> +	struct device *dev;
>> +	unsigned int part_id;
>> +	int ret;
>> +
>> +	dev = &i2c->dev;
> 
> Given it's unconditionally set and no line length issue, I'd prefer this
> done on the local variable definitions block above.

I can change this if you feel strong about it. Still, I personally find 
assignments in declarations less clear and so I tend to avoid them. 
(There's a few exceptions like repeated private data pointer getting).

> 
> 	struct device *dev = &i2c->dev;
>> +
>> +	regmap = devm_regmap_init_i2c(i2c, &bm1390_regmap);
>> +	if (IS_ERR(regmap))
>> +		return dev_err_probe(dev, PTR_ERR(regmap),
>> +				     "Failed to initialize Regmap\n");
>> +
>> +	idev = devm_iio_device_alloc(dev, sizeof(*data));
>> +	if (!idev)
>> +		return -ENOMEM;
>> +
>> +	ret = devm_regulator_get_enable(dev, "vdd");
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to get regulator\n");
>> +
>> +	data = iio_priv(idev);
> Why this ordering?  I'd expect this either:
> 1) Immediately after it becomes available so above the regulator enable,
> or
> 2) immediately before it is used, so a few lines down above data->regmap...
> Here it just looks a bit lost :)
> 

I agree. I may have just accidentally added the regulator stuff in 
between device alloc and getting the 'data'-pointer.

>> +
>> +	ret = regmap_read(regmap, BM1390_REG_PART_ID, &part_id);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to access sensor\n");
>> +
>> +	if (part_id != BM1390_ID)
>> +		dev_warn(dev, "unknown device 0x%x\n", part_id);
>> +
>> +	data->regmap = regmap;
>> +	data->dev = dev;
>> +	data->irq = i2c->irq;
>> +	/*
>> +	 * For now we just allow BM1390_WMI_MIN to BM1390_WMI_MAX and
>> +	 * discard every other configuration when triggered mode is not used.
>> +	 */
>> +	data->watermark = BM1390_WMI_MAX;
>> +	mutex_init(&data->mutex);
>> +
>> +	idev->channels = bm1390_channels;
>> +	idev->num_channels = ARRAY_SIZE(bm1390_channels);
>> +	idev->name = "bm1390";
>> +	idev->info = &bm1390_info;
>> +	idev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
>> +
>> +	ret = bm1390_chip_init(data);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "sensor init failed\n");
>> +
>> +	ret = bm1390_setup_trigger(data, idev, i2c->irq);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = devm_iio_device_register(dev, idev);
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret,
>> +				     "Unable to register iio device\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id bm1390_of_match[] = {
>> +	{ .compatible = "rohm,bm1390glv-z" },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, bm1390_of_match);
>> +
>> +static const struct i2c_device_id bm1390_id[] = {
>> +	{ "bm1390glv-z", },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, bm1390_id);
>> +
>> +static struct i2c_driver bm1390_driver = {
>> +	.driver = {
>> +		.name = "bm1390",
>> +		.of_match_table = bm1390_of_match,
>> +		/*
>> +		 * The probe issues a few msleep()s - which can result
>> +		 * measurable delays. Additionally, enabling the VDD may cause
>> +		 * some (slight) delay depending on the ramp-rate of the
>> +		 * regulator. Delays are propably magnitude of tens of
>> +		 * milliseconds - but async probing should not be a problem so
>> +		 * we should have nothing to lose here. Let's revise this if
>> +		 * async probing proves to cause problems here.
>> +		 */
> Perhaps we can summarise this with just the sleep info? - don't need to
> predict the future :)
> 		/*
> 		 * Probing explicitly requires a few millisecond of sleep.
> 	 	 * Enabling the VDD regulator may include ramp up rates.
> 		 */

I really seem to be babbling... :) Thanks for the distilled version :)

>> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> 

Much appreciated feedback!

Yours,
	-- Matti

Matti Vaittinen Sept. 19, 2023, 11:28 a.m. UTC | #6

On 9/18/23 15:56, Matti Vaittinen wrote:
> On 9/17/23 13:35, Jonathan Cameron wrote:
>> On Fri, 15 Sep 2023 09:56:19 +0300
>> Matti Vaittinen <[e-mail]> wrote:
>>
>>> Support for the ROHM BM1390 pressure sensor. The BM1390GLV-Z can measure
>>> pressures ranging from 300 hPa to 1300 hPa with configurable measurement
>>> averaging and internal FIFO. The sensor does also provide temperature
>>> measurements.
>>>
>>> Sensor does also contain IIR filter implemented in HW. The data-sheet
>>> says the IIR filter can be configured to be "weak", "middle" or
>>> "strong". Some RMS noise figures are provided in data sheet but no
>>> accurate maths for the filter configurations is provided. Hence, the IIR
>>> filter configuration is not supported by this driver and the filter is
>>> configured to the "middle" setting (at least not for now).
>>>
>>> The FIFO measurement mode is only measuring the pressure and not the
>>> temperature. The driver measures temperature when FIFO is flushed and
>>> simply uses the same measured temperature value to all reported
>>> temperatures. This should not be a problem when temperature is not
>>> changing very rapidly (several degrees C / second) but allows users to
>>> get the temperature measurements from sensor without any additional 
>>> logic.
>>>
>>> This driver allows the sensor to be used in two muitually exclusive 
>>> ways,
>>>
>>> 1. With trigger (data-ready IRQ).
>>> In this case the FIFO is not used as we get data ready for each 
>>> collected
>>> sample. Instead, for each data-ready IRQ we read the sample from sensor
>>> and push it to the IIO buffer.
>>>
>>> 2. With hardware FIFO and watermark IRQ.
>>> In this case the data-ready is not used but we enable watermark IRQ. At
>>> each watermark IRQ we go and read all samples in FIFO and push them 
>>> to the
>>> IIO buffer.
>>>
>>> Signed-off-by: Matti Vaittinen <[e-mail]>

...

>>> +
>>> +static const unsigned long bm1390_scan_masks[] = {
>>> +    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0
>> Why?  Doesn't look hard to support just one or the other?
>> Normally we only do this sort of limitation when there is a heavily
>> optimized read routine for a set of channels and it is better
>> to grab them all and throw away the ones we don't care about.
>> That doesn't seem to be true here. So if the fifo grabbed both
>> temp and pressure it would makes sense here, but doesn't seem
>> like it does.
> 
> I have a feeling I have misunderstood how this mask works. I have 
> assumed all the channels with corresponding mask bit _can_ be enabled 
> simultaneously, but I have not understood they _must_ all be enabled. I 
> think I must go back studying this, but if all channels really _must_ be 
> enabled, then you are correct. It actually makes a lot of sense to 
> support the pressure values alone, as, according to the data-sheet, the 
> HW is doing a "MEMS temperature compensation" to the pressure values. 
> So, my assuimption is the temperature data may not be required to be 
> captured.
> 
> This also means I should revise the scan masks for the BU27008, BU27010 
> and BU27034 light sensors as I don't think all the users want all the 
> channels enabled. I wonder how I have not noticed any problems when I 
> tested those things - did I really always enable all the channels...? @_@
> 
> Anyways, Thanks.

Hi Jonathan,

There's something in IIO scan_masks / buffer demuxing that I don't quite 
understand. I noticed following things:

1) Strict available scan mask check seems to be in use only for 
INDIO_BUFFER_HARDWARE stuff.

https://elixir.bootlin.com/linux/v6.6-rc2/source/drivers/iio/industrialio-buffer.c#L881

So, the:

 >>> +static const unsigned long bm1390_scan_masks[] = {
 >>> +    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0

is not exclusive for BM1390 as long as it is not setting the 
INDIO_BUFFER_HARDWARE.

My test seems to agree with the code:

// Only enable the temperature:
root@arm:/sys/bus/iio/devices/iio:device0# echo 1 > 
scan_elements/in_temp_en

// Run the geneeric buffer without -a:
root@arm:/sys/bus/iio/devices/iio:device0# /iio_generic_buffer -c 20 -N 
0 -t bm1390data-rdy-dev0
iio device number being used is 0
iio trigger number being used is 0
/sys/bus/iio/devices/iio:device0 bm1390data-rdy-dev0
23000.000000
23000.000000
23000.000000
23000.000000
23000.000000
23000.000000
23000.000000
23000.000000
23000.000000
22968.750000
23000.000000
23000.000000
23000.000000
23000.000000
23000.000000
23000.000000
23000.000000
23000.000000
23000.000000
23031.250000
23000.000000
23000.000000
root@arm:/sys/bus/iio/devices/iio:device0#

In above case the temperature values and only the temperature values 
were shown. I must admit I did not spend enough time with the 
iio_generic_buffer.c or IIO demuxing code to really understand all the 
details (I got headache very quickly ;) ). I still believe the 
iio_generic_buffer expects to see only the enabled channel data in the 
buffer - so, it seems to me the kernel is also only adding the enabled 
channel data to the buffer. Also, judging the values, the demuxing is 
correctly extracting the temperature data from data the driver pushes here:

iio_push_to_buffers_with_timestamp(idev, &data->buf, data->timestamp);

The bm1390 driver as sent in v2 does not do demuxing but always pushes 
whole chunk of data and trusts IIO to do demuxing.

2) I noticed the 'available_scan_masks' was marked as an optional field. 
So, I think that if there is no restrictions to which of the channels 
can be enabled, then we can omit setting it. This is what I tried.

It appears that when we do not populate the 'available_scan_masks' with the:
 >>> +static const unsigned long bm1390_scan_masks[] = {
 >>> +    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0

things change. When I tested enabling only temperature and ran the 
iio_generic_buffer -c 20 -N 0 -t bm1390data-rdy-dev0 - the reported 
values seemed completely random.

When I initialized the pressure data in driver:
data->buf.pressure = 1;
before doing the:
iio_push_to_buffers_with_timestamp(idev, &data->buf, data->timestamp);

I saw following:

root@arm:/# cd /sys/bus/iio/devices/iio\:device0
root@arm:/sys/bus/iio/devices/iio:device0# echo 1 > 
scan_elements/in_temp_en
root@arm:/sys/bus/iio/devices/iio:device0# /iio_generic_buffer -c 20 -N 
0 -t bm1390data-rdy-dev0
iio device number being used is 0
iio trigger number being used is 0
/sys/bus/iio/devices/iio:device0 bm1390data-rdy-dev0
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
8000.000000
root@arm:/sys/bus/iio/devices/iio:device0# cat in_temp_scale
31.250000

If we calculate 8000/31.250000 we will get value 256. This looks like 
value '1' in BE16 format.

Based on this experimenting (and headache obtained from reading the 
demuxing code) - the IIO framework does not do channel demuxing if the 
'available_scan_masks' is not given? To me this was somewhat unexpected.

Finally, when the watermark IRQ is used, we can't omit reading the 
pressure data because clearing the WMI is done based on the pressure 
data in FIFO.

So, I would propose we do:

static const unsigned long bm1390_scan_masks[] = {
	BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP),
	BIT(BM1390_CHAN_PRESSURE), 0

which better reflects what the hardware is capable of - and, unless I am 
missing something, also allows us to offload the buffer demuxing to the IIO.

Still, as mentioned in 1), the

 >>> +static const unsigned long bm1390_scan_masks[] = {
 >>> +    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0

does not seem to prevent enabling only the temperature channel - so in 
the driver buffer handler we still must unconditionally read the 
pressure data regardles the active_scan_mask.

>>
>>> +     * called as a result of a read operation from userspace and hence
>>> +     * before the watermark interrupt was triggered, take a timestamp
>>> +     * now. We can fall anywhere in between two samples so the error 
>>> in this
>>> +     * case is at most one sample period.
>>> +     * We need to have the IRQ disabled or we risk of messing-up
>>> +     * the timestamps. If we are ran from IRQ, then the
>>> +     * IRQF_ONESHOT has us covered - but if we are ran by the
>>> +     * user-space read we need to disable the IRQ to be on a safe
>>> +     * side. We do this usng synchronous disable so that if the
>>> +     * IRQ thread is being ran on other CPU we wait for it to be
>>> +     * finished.
>>
>> That irq disable is potentially expensive.
>> Why not just pass the current timestamp into the __bm1390_fifo_flush >
>> The locks should prevent other races I think..
> 
> Gah. I hate you Jonathan ;) (Not really!)
> 
> Actually, thank you (as always) for pointing this out. I don't instantly 
> see why it wouldn't work, but going throught the IRQ races is never 
> trivial (for me). It's work I've learned not to do at afternoon as my 
> brains work better at the morning :) So, I will still go through this as 
> a first thing tomorrow when I start my work day...

After staring this for a while, I see no reason why we couldn't do as 
you suggested. Thanks! It really improves this :)

Yours,
	-- Matti

Jonathan Cameron Sept. 19, 2023, 2:32 p.m. UTC | #7

> >> +static int bm1390_read_raw(struct iio_dev *idev,
> >> +               struct iio_chan_spec const *chan,
> >> +               int *val, int *val2, long mask)
> >> +{
> >> +    struct bm1390_data *data = iio_priv(idev);
> >> +    int ret;
> >> +
> >> +    switch (mask) {
> >> +    case IIO_CHAN_INFO_SCALE:
> >> +        if (chan->type == IIO_TEMP) {
> >> +            *val = 31;
> >> +            *val2 = 250000;
> >> +
> >> +            return IIO_VAL_INT_PLUS_MICRO;
> >> +        } else if (chan->type == IIO_PRESSURE) {
> >> +            *val = 0;
> >> +            /*
> >> +             * pressure in hPa is register value divided by 2048.
> >> +             * This means kPa is 1/20480 times the register value,
> >> +             * which equals to 48828.125 * 10 ^ -9
> >> +             * This is 48828.125 nano kPa.
> >> +             *
> >> +             * When we scale this using IIO_VAL_INT_PLUS_NANO we
> >> +             * get 48828 - which means we lose some accuracy. Well,
> >> +             * let's try to live with that.
> >> +             */
> >> +            *val2 = 48828;
> >> +
> >> +            return IIO_VAL_INT_PLUS_NANO;
> >> +        }
> >> +
> >> +        return -EINVAL;
> >> +    case IIO_CHAN_INFO_RAW:
> >> +        ret = iio_device_claim_direct_mode(idev);
> >> +        if (ret)
> >> +            return ret;
> >> +
> >> +        ret = bm1390_read_data(data, chan, val, val2);
> >> +        iio_device_release_direct_mode(idev);
> >> +        if (ret)
> >> +            return ret;
> >> +
> >> +        return IIO_VAL_INT;
> >> +    default:
> >> +        return -EINVAL;  
> > 
> > Certainly useless, but should we break and return -EINVAL after the 
> > switch, so that it is more explicit that bm1390_read_raw() always 
> > returns a value?  
> 
> I think there is also opposite opinions on this. For my eyes the return 
> at the end of the function would also be clearer - but I think I have 
> been asked to drop the useless return when I've been working with other 
> sensors in IIO domain :) My personal preference would definitely be:
> 
> int ret;
> 
> switch (foo)
> {
> case BAR:
> 	ret = func1();
> 	if (ret)
> 		break;
> 
> 	ret = func2();
> 	if (ret)
> 		break;
> 
> ...
> 	break;
> 
> case BAZ:
> 	ret = -EINVAL;
> 	break;
> }
> 
> return ret;
> 
> - but I've learned to think this is not the IIO preference.

Some static analyzers get confused (probably when there is a little
bit more going on after the function) by that and moan that some
cases are not considered in the switch.  I got annoyed enough with the
noise they were generating to advocate always having explicit defaults.


> 
>

Jonathan Cameron Sept. 19, 2023, 2:45 p.m. UTC | #8

On Mon, 18 Sep 2023 15:56:42 +0300
Matti Vaittinen <[e-mail]> wrote:

> On 9/17/23 13:35, Jonathan Cameron wrote:
> > On Fri, 15 Sep 2023 09:56:19 +0300
> > Matti Vaittinen <[e-mail]> wrote:
> >   
> >> Support for the ROHM BM1390 pressure sensor. The BM1390GLV-Z can measure
> >> pressures ranging from 300 hPa to 1300 hPa with configurable measurement
> >> averaging and internal FIFO. The sensor does also provide temperature
> >> measurements.
> >>
> >> Sensor does also contain IIR filter implemented in HW. The data-sheet
> >> says the IIR filter can be configured to be "weak", "middle" or
> >> "strong". Some RMS noise figures are provided in data sheet but no
> >> accurate maths for the filter configurations is provided. Hence, the IIR
> >> filter configuration is not supported by this driver and the filter is
> >> configured to the "middle" setting (at least not for now).
> >>
> >> The FIFO measurement mode is only measuring the pressure and not the
> >> temperature. The driver measures temperature when FIFO is flushed and
> >> simply uses the same measured temperature value to all reported
> >> temperatures. This should not be a problem when temperature is not
> >> changing very rapidly (several degrees C / second) but allows users to
> >> get the temperature measurements from sensor without any additional logic.
> >>
> >> This driver allows the sensor to be used in two muitually exclusive ways,
> >>
> >> 1. With trigger (data-ready IRQ).
> >> In this case the FIFO is not used as we get data ready for each collected
> >> sample. Instead, for each data-ready IRQ we read the sample from sensor
> >> and push it to the IIO buffer.
> >>
> >> 2. With hardware FIFO and watermark IRQ.
> >> In this case the data-ready is not used but we enable watermark IRQ. At
> >> each watermark IRQ we go and read all samples in FIFO and push them to the
> >> IIO buffer.
> >>
> >> Signed-off-by: Matti Vaittinen <[e-mail]>  
> > 
> > Hi Matti,
> > 
> > I think this is coming together nicely. A few comments inline.  
> 
> Thanks for the review (once again) :)
> 
> >   
> >>
> >> ---
> >> Revision history:
> >> v1 => v2:
> >> - prefer s64 over int64_t
> >> - drop not needed handling of 2's complements
> >> - plenty of styling changes
> >> - drop dead code (write_raw)
> >> - fix typos in comments
> >> - explain trigger and FIFO usage in commit message
> >> - do better job at cheking the return values
> >> - ensure there's no race when checking if triggered buffer is used
> >>    before enabling the FIFO
> >> - print warning if register read fails at IRQ handler
> >> - drop unnecessary warning if IRQ is not given
> >> - explain why we prefer asynchronous probing
> >> ---  
> >   
> >> diff --git a/drivers/iio/pressure/rohm-bm1390.c b/drivers/iio/pressure/rohm-bm1390.c
> >> new file mode 100644
> >> index 000000000000..d3cc229d1688
> >> --- /dev/null
> >> +++ b/drivers/iio/pressure/rohm-bm1390.c
> >> @@ -0,0 +1,899 @@  
> > 
> > ...
> >   
> >> +
> >> +static const unsigned long bm1390_scan_masks[] = {
> >> +	BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0  
> > Why?  Doesn't look hard to support just one or the other?
> > Normally we only do this sort of limitation when there is a heavily
> > optimized read routine for a set of channels and it is better
> > to grab them all and throw away the ones we don't care about.
> > That doesn't seem to be true here. So if the fifo grabbed both
> > temp and pressure it would makes sense here, but doesn't seem
> > like it does.  
> 
> I have a feeling I have misunderstood how this mask works. I have 
> assumed all the channels with corresponding mask bit _can_ be enabled 
> simultaneously, but I have not understood they _must_ all be enabled. I 
> think I must go back studying this, but if all channels really _must_ be 
> enabled, then you are correct. It actually makes a lot of sense to 
> support the pressure values alone, as, according to the data-sheet, the 
> HW is doing a "MEMS temperature compensation" to the pressure values. 
> So, my assuimption is the temperature data may not be required to be 
> captured.
> 
> This also means I should revise the scan masks for the BU27008, BU27010 
> and BU27034 light sensors as I don't think all the users want all the 
> channels enabled. I wonder how I have not noticed any problems when I 
> tested those things - did I really always enable all the channels...? @_@

If you didn't enable them all, the IIO core would then grab them all anyway
but demux the data out that you actually wanted.  So you would only notice
if you looked at the data transfers happening which were probably larger
than necessary.


> 
> Anyways, Thanks.

> >> +static int bm1390_read_raw(struct iio_dev *idev,
> >> +			   struct iio_chan_spec const *chan,
> >> +			   int *val, int *val2, long mask)
> >> +{
> >> +	struct bm1390_data *data = iio_priv(idev);
> >> +	int ret;
> >> +
> >> +	switch (mask) {
> >> +	case IIO_CHAN_INFO_SCALE:
> >> +		if (chan->type == IIO_TEMP) {
> >> +			*val = 31;
> >> +			*val2 = 250000;
> >> +
> >> +			return IIO_VAL_INT_PLUS_MICRO;
> >> +		} else if (chan->type == IIO_PRESSURE) {
> >> +			*val = 0;
> >> +			/*
> >> +			 * pressure in hPa is register value divided by 2048.
> >> +			 * This means kPa is 1/20480 times the register value,
> >> +			 * which equals to 48828.125 * 10 ^ -9
> >> +			 * This is 48828.125 nano kPa.
> >> +			 *
> >> +			 * When we scale this using IIO_VAL_INT_PLUS_NANO we
> >> +			 * get 48828 - which means we lose some accuracy. Well,
> >> +			 * let's try to live with that.
> >> +			 */
> >> +			*val2 = 48828;
> >> +
> >> +			return IIO_VAL_INT_PLUS_NANO;  
> > 
> > IIO_VAL_FRACTIONAL?  Mind you I'm not sure that will result in enough precision
> > either here.   For in kernel use it will have full precision though as will be
> > kept as a fraction.  I guess question of whether it is worse than what you have
> > here.  There hasn't been much demand for IIO_VAL_INTO_PLUS_PICO, but if we have
> > to look at that we can - with proviso that existing userspace software won't know
> > anything about it.  
> 
> I must take a look at the IIO_VAL_FRACTIONAL. I think either it or the 
> NANO at this point. I have no understanding what is the precision that 
> is enough. Hmm.. Would adding the PICO really require a change in 
> user-space or could we hide it in-kernel when formatting the value we 
> emit via sysfs?

More digits would turn up :) Otherwise you are correct and I wasn't thinking
this through. You'd hope that the code reading them would be fine.

I had a patch adding to precision of IIO_VAL_FRACTION_LOG2 a while
back on basis we'd 'get away with it'.
https://lore.kernel.org/linux-iio/[e-mail]/

Not sure I ever picked it up though :( 


...

> 
> >   
> >> +	 * called as a result of a read operation from userspace and hence
> >> +	 * before the watermark interrupt was triggered, take a timestamp
> >> +	 * now. We can fall anywhere in between two samples so the error in this
> >> +	 * case is at most one sample period.
> >> +	 * We need to have the IRQ disabled or we risk of messing-up
> >> +	 * the timestamps. If we are ran from IRQ, then the
> >> +	 * IRQF_ONESHOT has us covered - but if we are ran by the
> >> +	 * user-space read we need to disable the IRQ to be on a safe
> >> +	 * side. We do this usng synchronous disable so that if the
> >> +	 * IRQ thread is being ran on other CPU we wait for it to be
> >> +	 * finished.  
> > 
> > That irq disable is potentially expensive.
> > Why not just pass the current timestamp into the __bm1390_fifo_flush >
> > The locks should prevent other races I think..  
> 
> Gah. I hate you Jonathan ;) (Not really!)
> 
> Actually, thank you (as always) for pointing this out. I don't instantly 
> see why it wouldn't work, but going throught the IRQ races is never 
> trivial (for me). It's work I've learned not to do at afternoon as my 
> brains work better at the morning :) So, I will still go through this as 
> a first thing tomorrow when I start my work day...

...


> >   
> >> +
> >> +	ret = devm_iio_triggered_buffer_setup(data->dev, idev,
> >> +					      &iio_pollfunc_store_time,
> >> +					      &bm1390_trigger_handler,
> >> +					      &bm1390_buffer_ops);  
> > 
> > Why doesn't this still apply even if we don't have an irq for this device?
> > Can use a sysfs or hrtimer trigger or an irq on another device.
> >   
> 
> Another good question. I know I wrote the code only thinking that when 
> the trigger is used, it will be the data-ready from the device. I think 
> I even used the trigger validation function which requires us to use the 
> device's own trigger.
> 
> Now, whether it would make sense to allow other triggers, and whether 
> the trigger handler has any data-ready specific handling here is 
> something I need to revise. What does slightly bug me with allowing 
> other triggers is that as far as I remmeber the BM1390 had no way to 
> indicate that a single sample was actually ready. So, when we use any 
> other trigger it may be we don't have a valid sample to read. (The 
> read_raw uses a delay which should be long enough to guarantee a sample).

That is a fairly common problem with using other triggers. Will we get no
data, or stale data?  If just stale, but some guarantee on not too stale
then usually fine to just return that.

> 
> >> +
> >> +	if (ret)
> >> +		return dev_err_probe(data->dev, ret,
> >> +				     "iio_triggered_buffer_setup FAIL\n");
> >> +
> >> +	itrig = devm_iio_trigger_alloc(data->dev, "%sdata-rdy-dev%d", idev->name,
> >> +					    iio_device_id(idev));
> >> +	if (!itrig)
> >> +		return -ENOMEM;
> >> +
> >> +	data->trig = itrig;
> >> +	idev->available_scan_masks = bm1390_scan_masks;  
> > 
> > Mixing trigger and buffer stuff in here. I'd rather see them
> > separate - so move this up to where you set the buffer up.  
> 
> Thanks for pointing this out. In my mind the trigger and buffer went 
> hand to hand with this device. Now, after your previous comment, I need 
> to rethink if they really can be sepated. Anyways, moving this makes sense.
> 
That separation is often messy / unclear.  So it is kind of best effort
+ making it work with external triggers if possible.

> >   
> >> +
> >> +	itrig->ops = &bm1390_trigger_ops;
> >> +	iio_trigger_set_drvdata(itrig, data);
> >> +
> >> +	name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-bm1390",
> >> +			      dev_name(data->dev));  
> > 
> > Check the allocation.  Definitely don't want name not defined here.  
> 
> The request_threaded_irq will use the dev_name() if NULL is passed to 
> it. I think it should be fine - but I think this is not worth fighting 
> if you strongly prefer the check :)

I do :)  Mostly because I don't want readers to have to know it doesn't
matter.  It will never fail anyway in reasonable systems.


> >   
> >> +
> >> +	ret = devm_request_threaded_irq(data->dev, irq, bm1390_irq_handler,
> >> +					&bm1390_irq_thread_handler,
> >> +					IRQF_ONESHOT, name, idev);
> >> +	if (ret)
> >> +		return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
> >> +
> >> +
> >> +	ret = devm_iio_trigger_register(data->dev, itrig);
> >> +	if (ret)
> >> +		return dev_err_probe(data->dev, ret,
> >> +				     "Trigger registration failed\n");
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int bm1390_probe(struct i2c_client *i2c)
> >> +{
> >> +	struct bm1390_data *data;
> >> +	struct regmap *regmap;
> >> +	struct iio_dev *idev;
> >> +	struct device *dev;
> >> +	unsigned int part_id;
> >> +	int ret;
> >> +
> >> +	dev = &i2c->dev;  
> > 
> > Given it's unconditionally set and no line length issue, I'd prefer this
> > done on the local variable definitions block above.  
> 
> I can change this if you feel strong about it. Still, I personally find 
> assignments in declarations less clear and so I tend to avoid them. 
> (There's a few exceptions like repeated private data pointer getting).

Don't care that much.  Though I'll probably forget I said that
and moan about it in next version :)



Jonathan

Jonathan Cameron Sept. 19, 2023, 2:53 p.m. UTC | #9

On Tue, 19 Sep 2023 14:28:29 +0300
Matti Vaittinen <[e-mail]> wrote:

> On 9/18/23 15:56, Matti Vaittinen wrote:
> > On 9/17/23 13:35, Jonathan Cameron wrote:  
> >> On Fri, 15 Sep 2023 09:56:19 +0300
> >> Matti Vaittinen <[e-mail]> wrote:
> >>  
> >>> Support for the ROHM BM1390 pressure sensor. The BM1390GLV-Z can measure
> >>> pressures ranging from 300 hPa to 1300 hPa with configurable measurement
> >>> averaging and internal FIFO. The sensor does also provide temperature
> >>> measurements.
> >>>
> >>> Sensor does also contain IIR filter implemented in HW. The data-sheet
> >>> says the IIR filter can be configured to be "weak", "middle" or
> >>> "strong". Some RMS noise figures are provided in data sheet but no
> >>> accurate maths for the filter configurations is provided. Hence, the IIR
> >>> filter configuration is not supported by this driver and the filter is
> >>> configured to the "middle" setting (at least not for now).
> >>>
> >>> The FIFO measurement mode is only measuring the pressure and not the
> >>> temperature. The driver measures temperature when FIFO is flushed and
> >>> simply uses the same measured temperature value to all reported
> >>> temperatures. This should not be a problem when temperature is not
> >>> changing very rapidly (several degrees C / second) but allows users to
> >>> get the temperature measurements from sensor without any additional 
> >>> logic.
> >>>
> >>> This driver allows the sensor to be used in two muitually exclusive 
> >>> ways,
> >>>
> >>> 1. With trigger (data-ready IRQ).
> >>> In this case the FIFO is not used as we get data ready for each 
> >>> collected
> >>> sample. Instead, for each data-ready IRQ we read the sample from sensor
> >>> and push it to the IIO buffer.
> >>>
> >>> 2. With hardware FIFO and watermark IRQ.
> >>> In this case the data-ready is not used but we enable watermark IRQ. At
> >>> each watermark IRQ we go and read all samples in FIFO and push them 
> >>> to the
> >>> IIO buffer.
> >>>
> >>> Signed-off-by: Matti Vaittinen <[e-mail]>  
> 
> ...
> 
> >>> +
> >>> +static const unsigned long bm1390_scan_masks[] = {
> >>> +    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0  
> >> Why?  Doesn't look hard to support just one or the other?
> >> Normally we only do this sort of limitation when there is a heavily
> >> optimized read routine for a set of channels and it is better
> >> to grab them all and throw away the ones we don't care about.
> >> That doesn't seem to be true here. So if the fifo grabbed both
> >> temp and pressure it would makes sense here, but doesn't seem
> >> like it does.  
> > 
> > I have a feeling I have misunderstood how this mask works. I have 
> > assumed all the channels with corresponding mask bit _can_ be enabled 
> > simultaneously, but I have not understood they _must_ all be enabled. I 
> > think I must go back studying this, but if all channels really _must_ be 
> > enabled, then you are correct. It actually makes a lot of sense to 
> > support the pressure values alone, as, according to the data-sheet, the 
> > HW is doing a "MEMS temperature compensation" to the pressure values. 
> > So, my assuimption is the temperature data may not be required to be 
> > captured.
> > 
> > This also means I should revise the scan masks for the BU27008, BU27010 
> > and BU27034 light sensors as I don't think all the users want all the 
> > channels enabled. I wonder how I have not noticed any problems when I 
> > tested those things - did I really always enable all the channels...? @_@
> > 
> > Anyways, Thanks.  
> 
> Hi Jonathan,
> 
> There's something in IIO scan_masks / buffer demuxing that I don't quite 
> understand. I noticed following things:
> 
> 1) Strict available scan mask check seems to be in use only for 
> INDIO_BUFFER_HARDWARE stuff.
> 
> https://elixir.bootlin.com/linux/v6.6-rc2/source/drivers/iio/industrialio-buffer.c#L881
> 

That is referring to the fact that you can't start the buffer if it
doesn't match (because we can't mess with the data stream in
that type of buffer as we don't necessarily see it in software!)

> So, the:
> 
>  >>> +static const unsigned long bm1390_scan_masks[] = {
>  >>> +    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0  
> 
> is not exclusive for BM1390 as long as it is not setting the 
> INDIO_BUFFER_HARDWARE.
> 
> My test seems to agree with the code:
> 
> // Only enable the temperature:
> root@arm:/sys/bus/iio/devices/iio:device0# echo 1 > 
> scan_elements/in_temp_en
> 
> // Run the geneeric buffer without -a:
> root@arm:/sys/bus/iio/devices/iio:device0# /iio_generic_buffer -c 20 -N 
> 0 -t bm1390data-rdy-dev0
> iio device number being used is 0
> iio trigger number being used is 0
> /sys/bus/iio/devices/iio:device0 bm1390data-rdy-dev0
> 23000.000000
> 23000.000000
> 23000.000000
> 23000.000000
> 23000.000000
> 23000.000000
> 23000.000000
> 23000.000000
> 23000.000000
> 22968.750000
> 23000.000000
> 23000.000000
> 23000.000000
> 23000.000000
> 23000.000000
> 23000.000000
> 23000.000000
> 23000.000000
> 23000.000000
> 23031.250000
> 23000.000000
> 23000.000000
> root@arm:/sys/bus/iio/devices/iio:device0#

> 
> In above case the temperature values and only the temperature values 
> were shown. I must admit I did not spend enough time with the 
> iio_generic_buffer.c or IIO demuxing code to really understand all the 
> details (I got headache very quickly ;) ). I still believe the 
> iio_generic_buffer expects to see only the enabled channel data in the 
> buffer - so, it seems to me the kernel is also only adding the enabled 
> channel data to the buffer. Also, judging the values, the demuxing is 
> correctly extracting the temperature data from data the driver pushes here:
> 
> iio_push_to_buffers_with_timestamp(idev, &data->buf, data->timestamp);
> 
> The bm1390 driver as sent in v2 does not do demuxing but always pushes 
> whole chunk of data and trusts IIO to do demuxing.

Yup. That should work.  But in this case you can trivially decide not to read
or fill in the temperature and hence save some bus cycles.

> 
> 2) I noticed the 'available_scan_masks' was marked as an optional field. 
> So, I think that if there is no restrictions to which of the channels 
> can be enabled, then we can omit setting it. This is what I tried.
> 
> It appears that when we do not populate the 'available_scan_masks' with the:
>  >>> +static const unsigned long bm1390_scan_masks[] = {
>  >>> +    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0  
> 
> things change. When I tested enabling only temperature and ran the 
> iio_generic_buffer -c 20 -N 0 -t bm1390data-rdy-dev0 - the reported 
> values seemed completely random.

You need to pack the data correctly yourself in the driver.
So it adds small amount of code complexity but potentially saves bus
traffic.


> 
> When I initialized the pressure data in driver:
> data->buf.pressure = 1;
> before doing the:
> iio_push_to_buffers_with_timestamp(idev, &data->buf, data->timestamp);
> 
> I saw following:
> 
> root@arm:/# cd /sys/bus/iio/devices/iio\:device0
> root@arm:/sys/bus/iio/devices/iio:device0# echo 1 > 
> scan_elements/in_temp_en
> root@arm:/sys/bus/iio/devices/iio:device0# /iio_generic_buffer -c 20 -N 
> 0 -t bm1390data-rdy-dev0
> iio device number being used is 0
> iio trigger number being used is 0
> /sys/bus/iio/devices/iio:device0 bm1390data-rdy-dev0
> 8000.000000
> 8000.000000
> 8000.000000
> 8000.000000
> 8000.000000
> 8000.000000
> 8000.000000
> 8000.000000
> 8000.000000
> 8000.000000
> 8000.000000
> 8000.000000
> 8000.000000
> 8000.000000
> 8000.000000
> 8000.000000
> 8000.000000
> 8000.000000
> 8000.000000
> 8000.000000
> root@arm:/sys/bus/iio/devices/iio:device0# cat in_temp_scale
> 31.250000
> 
> If we calculate 8000/31.250000 we will get value 256. This looks like 
> value '1' in BE16 format.

> 
> Based on this experimenting (and headache obtained from reading the 
> demuxing code) - the IIO framework does not do channel demuxing if the 
> 'available_scan_masks' is not given? To me this was somewhat unexpected.

Yes.  If you don't tell it what channel setups are available (note there can
be lots) you are saying that we support any random combination and have
to do the data packing by hand.

> 
> Finally, when the watermark IRQ is used, we can't omit reading the 
> pressure data because clearing the WMI is done based on the pressure 
> data in FIFO.

Hmm. That is a good reason to keep the available scan masks set.
Add a comment on that next to the mask.

> 
> So, I would propose we do:
> 
> static const unsigned long bm1390_scan_masks[] = {
> 	BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP),
> 	BIT(BM1390_CHAN_PRESSURE), 0

Makes sense given need to read the pressure channel.
> 
> which better reflects what the hardware is capable of - and, unless I am 
> missing something, also allows us to offload the buffer demuxing to the IIO.
> 
> Still, as mentioned in 1), the
> 
>  >>> +static const unsigned long bm1390_scan_masks[] = {
>  >>> +    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0  
> 
> does not seem to prevent enabling only the temperature channel - so in 
> the driver buffer handler we still must unconditionally read the 
> pressure data regardles the active_scan_mask.

active_scan_mask should match the above - it's separate from what is enabled.
active_scan_mask is on the data capture side of the demux - the buffers
are then fed repacked data reflecting what is enabled.

> 
> >>  
> >>> +     * called as a result of a read operation from userspace and hence
> >>> +     * before the watermark interrupt was triggered, take a timestamp
> >>> +     * now. We can fall anywhere in between two samples so the error 
> >>> in this
> >>> +     * case is at most one sample period.
> >>> +     * We need to have the IRQ disabled or we risk of messing-up
> >>> +     * the timestamps. If we are ran from IRQ, then the
> >>> +     * IRQF_ONESHOT has us covered - but if we are ran by the
> >>> +     * user-space read we need to disable the IRQ to be on a safe
> >>> +     * side. We do this usng synchronous disable so that if the
> >>> +     * IRQ thread is being ran on other CPU we wait for it to be
> >>> +     * finished.  
> >>
> >> That irq disable is potentially expensive.
> >> Why not just pass the current timestamp into the __bm1390_fifo_flush >
> >> The locks should prevent other races I think..  
> > 
> > Gah. I hate you Jonathan ;) (Not really!)
> > 
> > Actually, thank you (as always) for pointing this out. I don't instantly 
> > see why it wouldn't work, but going throught the IRQ races is never 
> > trivial (for me). It's work I've learned not to do at afternoon as my 
> > brains work better at the morning :) So, I will still go through this as 
> > a first thing tomorrow when I start my work day...  
> 
> After staring this for a while, I see no reason why we couldn't do as 
> you suggested. Thanks! It really improves this :)

No problem.

Jonathan

> 
> Yours,
> 	-- Matti
>

Matti Vaittinen Sept. 21, 2023, 8:17 a.m. UTC | #10

On 9/19/23 17:53, Jonathan Cameron wrote:
> On Tue, 19 Sep 2023 14:28:29 +0300
> Matti Vaittinen <[e-mail]> wrote:
> 
>> On 9/18/23 15:56, Matti Vaittinen wrote:
>>> On 9/17/23 13:35, Jonathan Cameron wrote:
>>>> On Fri, 15 Sep 2023 09:56:19 +0300
>>>> Matti Vaittinen <[e-mail]> wrote:
>>>>   
>>>>> Support for the ROHM BM1390 pressure sensor. The BM1390GLV-Z can measure
>>>>> pressures ranging from 300 hPa to 1300 hPa with configurable measurement
>>>>> averaging and internal FIFO. The sensor does also provide temperature
>>>>> measurements.
>>>>>
>>>>> Sensor does also contain IIR filter implemented in HW. The data-sheet
>>>>> says the IIR filter can be configured to be "weak", "middle" or
>>>>> "strong". Some RMS noise figures are provided in data sheet but no
>>>>> accurate maths for the filter configurations is provided. Hence, the IIR
>>>>> filter configuration is not supported by this driver and the filter is
>>>>> configured to the "middle" setting (at least not for now).
>>>>>
>>>>> The FIFO measurement mode is only measuring the pressure and not the
>>>>> temperature. The driver measures temperature when FIFO is flushed and
>>>>> simply uses the same measured temperature value to all reported
>>>>> temperatures. This should not be a problem when temperature is not
>>>>> changing very rapidly (several degrees C / second) but allows users to
>>>>> get the temperature measurements from sensor without any additional
>>>>> logic.
>>>>>
>>>>> This driver allows the sensor to be used in two muitually exclusive
>>>>> ways,
>>>>>
>>>>> 1. With trigger (data-ready IRQ).
>>>>> In this case the FIFO is not used as we get data ready for each
>>>>> collected
>>>>> sample. Instead, for each data-ready IRQ we read the sample from sensor
>>>>> and push it to the IIO buffer.
>>>>>
>>>>> 2. With hardware FIFO and watermark IRQ.
>>>>> In this case the data-ready is not used but we enable watermark IRQ. At
>>>>> each watermark IRQ we go and read all samples in FIFO and push them
>>>>> to the
>>>>> IIO buffer.
>>>>>
>>>>> Signed-off-by: Matti Vaittinen <[e-mail]>
>>
>> ...
>>
>>>>> +
>>>>> +static const unsigned long bm1390_scan_masks[] = {
>>>>> +    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0
>>>> Why?  Doesn't look hard to support just one or the other?
>>>> Normally we only do this sort of limitation when there is a heavily
>>>> optimized read routine for a set of channels and it is better
>>>> to grab them all and throw away the ones we don't care about.
>>>> That doesn't seem to be true here. So if the fifo grabbed both
>>>> temp and pressure it would makes sense here, but doesn't seem
>>>> like it does.
>>>
>>> I have a feeling I have misunderstood how this mask works. I have
>>> assumed all the channels with corresponding mask bit _can_ be enabled
>>> simultaneously, but I have not understood they _must_ all be enabled. I
>>> think I must go back studying this, but if all channels really _must_ be
>>> enabled, then you are correct. It actually makes a lot of sense to
>>> support the pressure values alone, as, according to the data-sheet, the
>>> HW is doing a "MEMS temperature compensation" to the pressure values.
>>> So, my assuimption is the temperature data may not be required to be
>>> captured.
>>>
>>> This also means I should revise the scan masks for the BU27008, BU27010
>>> and BU27034 light sensors as I don't think all the users want all the
>>> channels enabled. I wonder how I have not noticed any problems when I
>>> tested those things - did I really always enable all the channels...? @_@
>>>
>>> Anyways, Thanks.
>>
>> Hi Jonathan,
>>
>> There's something in IIO scan_masks / buffer demuxing that I don't quite
>> understand

Thank You for the patience and the explanation Jonathan. I have a - 
hopefully not totally wrong - feeling I understand this better. I didn't 
understand that IIO framework has this extra logic and that 
available_scan_masks was not really meant for telling what IIO allows 
_users_ to enable - but it was for driver to tell IIO core what the 
driver can give. (Naturally this impacts to what IIO allows users to 
enable, but not directly). Eg, if I now get it right, the 
'available_scan_masks' is information what data flows between driver and 
IIO. Similarly, 'active_scan_mask' tells what the IIO core 'expects' to 
get from the driver, right? So, in bm1390 case, if I set both:

BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP),
and
BIT(BM1390_CHAN_PRESSURE),

to 'available_scan_masks', then it means driver is telling IIO it can 
give both pressure and temperature, or temperature alone. As a result, 
IIO can set either both temp and pressure to 'active_scan_mask' - in 
which case driver should push both in the buffers - or just the 
pressure, in which case the driver should not push the temperature.

>>
>> The bm1390 driver as sent in v2 does not do demuxing but always pushes
>> whole chunk of data and trusts IIO to do demuxing.
> 
> Yup. That should work.  But in this case you can trivially decide not to read
> or fill in the temperature and hence save some bus cycles.
> 
>>
>> 2) I noticed the 'available_scan_masks' was marked as an optional field.
>> So, I think that if there is no restrictions to which of the channels
>> can be enabled, then we can omit setting it. This is what I tried.
>>
>> It appears that when we do not populate the 'available_scan_masks' with the:
>>   >>> +static const unsigned long bm1390_scan_masks[] = {
>>   >>> +    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0
>>
>> things change. When I tested enabling only temperature and ran the
>> iio_generic_buffer -c 20 -N 0 -t bm1390data-rdy-dev0 - the reported
>> values seemed completely random.
> 
> You need to pack the data correctly yourself in the driver.
> So it adds small amount of code complexity but potentially saves bus
> traffic.

...

>>
>> Based on this experimenting (and headache obtained from reading the
>> demuxing code) - the IIO framework does not do channel demuxing if the
>> 'available_scan_masks' is not given? To me this was somewhat unexpected.
> 
> Yes.  If you don't tell it what channel setups are available (note there can
> be lots) you are saying that we support any random combination and have
> to do the data packing by hand.
> 

Okay... I think it kind of makes sense to leave an option where the 
driver can create buffer as it wants. I am not sure I can wrap my head 
around this to the extent that I knew when the IIO does not know what 
channels the driver has enabled - and thus, what types of data there 
will be - but I can accept the answer that such situations can exist :) 
Besides, allowing the driver to do the buffer formatting may allow some 
'use-case specific, custom optimizations' when user-space knows what to 
expect (for example, regarding the data alignments and ordering to for 
example save space).

...This leads to another thing I noticed - and to another question :)

I was experimenting with the bm1390 scan masks and iio_generic_buffer 
tool. It seems to me that the iio_generic_buffer does not expect the 
padding when the data in buffer is u32 + u16. When I enable the pressure 
(32bits) and temperature (16 bits) in bm1390 and keep the time-stamp 
disabled, the user-space will get buffer where 2 bytes of padding is 
added to each temperature sample so the next pressure sample stays 
correctly aligned. I believe kernel side does sane job here.

This, however, did result (at least my version of) the 
iio_generic_buffer tool to read garbage values after the first sample. 
My assumption is that the iio_generic_buffer does not add the padding 
bytes to the end of last data in the scan in order to ensure also the 
next scan will be correctly aligned.

I did following change to the iio_generic_buffer:

Author: Matti Vaittinen <[e-mail]>
Date:   Thu Sep 21 10:15:19 2023 +0300

     tools: iio: iio_generic_buffer ensure alignment

     The iio_generic_buffer can return garbage values when the total size of
     scan data is not a multiple of largest element in the scan. This can be
     demonstrated by reading a scan consisting for example of one 4 byte and
     one 2 byte element, where the 4 byte elemnt is first in the buffer.

     The IIO generic buffert code does not take into accunt the last two
     padding bytes that are needed to ensure that the 4byte data for next
     scan is correctly aligned.

     Add padding bytes required to align the next sample into the scan size.

     Signed-off-by: Matti Vaittinen <[e-mail]>

diff --git a/tools/iio/iio_generic_buffer.c b/tools/iio/iio_generic_buffer.c
index 44bbf80f0cfd..fc562799a109 100644
--- a/tools/iio/iio_generic_buffer.c
+++ b/tools/iio/iio_generic_buffer.c
@@ -54,9 +54,12 @@ enum autochan {
  static unsigned int size_from_channelarray(struct iio_channel_info 
*channels, int num_channels)
  {
         unsigned int bytes = 0;
-       int i = 0;
+       int i = 0, max = 0;
+       unsigned int misalignment;

         while (i < num_channels) {
+               if (channels[i].bytes > max)
+                       max = channels[i].bytes;
                 if (bytes % channels[i].bytes == 0)
                         channels[i].location = bytes;
                 else
@@ -66,6 +69,16 @@ static unsigned int size_from_channelarray(struct 
iio_channel_info *channels, in
                 bytes = channels[i].location + channels[i].bytes;
                 i++;
         }
+       /*
+        * We wan't the data in next sample to also be properly aligned so
+        * we'll add padding at the end if needed. TODO: should we use fixed
+        * 8 byte alignment instead of the size of the biggest samnple?
+        */
+       misalignment = bytes % max;
+       if (misalignment) {
+               printf("Misalignment %u. Adding Padding %u\n", 
misalignment,  max - misalignment);
+               bytes += max - misalignment;
+       }

         return bytes;
  }

I can send this as a proper patch if you guys think it is correct.


>>
>> Finally, when the watermark IRQ is used, we can't omit reading the
>> pressure data because clearing the WMI is done based on the pressure
>> data in FIFO.
> 
> Hmm. That is a good reason to keep the available scan masks set.
> Add a comment on that next to the mask.
> 
>>
>> So, I would propose we do:
>>
>> static const unsigned long bm1390_scan_masks[] = {
>> 	BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP),
>> 	BIT(BM1390_CHAN_PRESSURE), 0
> 
> Makes sense given need to read the pressure channel.
>>
>> which better reflects what the hardware is capable of - and, unless I am
>> missing something, also allows us to offload the buffer demuxing to the IIO.
>>
>> Still, as mentioned in 1), the
>>
>>   >>> +static const unsigned long bm1390_scan_masks[] = {
>>   >>> +    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0
>>
>> does not seem to prevent enabling only the temperature channel - so in
>> the driver buffer handler we still must unconditionally read the
>> pressure data regardles the active_scan_mask.
> 
> active_scan_mask should match the above - it's separate from what is enabled.
> active_scan_mask is on the data capture side of the demux - the buffers
> are then fed repacked data reflecting what is enabled.

Cool! So, the driver can rely on IIO asking for the pressure (in 
active_scan_mask) after it has correctly set the available scan mask. It 
may not be a big thing but I like it. It is enough to take care of 
ensuring all required stuff is read from HW in one place (in available 
scan masks) and not to worry about it in actual data reading path but 
just read stuff IIO is asking to be read. I like it.

Another thing to note is that, when we build the available_scan_mask 
array - we should either pay attention to the order of masks - or change 
the iio_scan_mask_match() to not accept first matching subset but to go 
through all of the masks unless it finds and exactly matching one (and 
in general prefer the smallest subset). Not sure this is worth the extra 
cycles though.

Thanks again - feel like I've learned something today :)

Yours,
	-- Matti Vaittinen

Matti Vaittinen Sept. 21, 2023, 9 a.m. UTC | #11

On 9/21/23 11:17, Matti Vaittinen wrote:

> Another thing to note is that, when we build the available_scan_mask 
> array - we should either pay attention to the order of masks - or change 
> the iio_scan_mask_match() to not accept first matching subset but to go 
> through all of the masks unless it finds and exactly matching one (and 
> in general prefer the smallest subset). Not sure this is worth the extra 
> cycles though.

Replying to myself and to those who I perhaps managed to confuse :)

As a result of above pondering I wrote this:

@@ -411,6 +418,8 @@ static const unsigned long 
*iio_scan_mask_match(const unsigned long *av_masks,
                                                 const unsigned long *mask,
                                                 bool strict)
  {
+       const unsigned long *smallest = NULL;
+
         if (bitmap_empty(mask, masklength))
                 return NULL;
         while (*av_masks) {
@@ -418,12 +427,16 @@ static const unsigned long 
*iio_scan_mask_match(const unsigned long *av_masks,
                         if (bitmap_equal(mask, av_masks, masklength))
                                 return av_masks;
                 } else {
-                       if (bitmap_subset(mask, av_masks, masklength))
-                               return av_masks;
+                       if (bitmap_subset(mask, av_masks, masklength)) {
+                               if (!smallest ||
+                                   bitmap_weight(av_masks, BITS_PER_LONG) <
+                                   bitmap_weight(smallest, BITS_PER_LONG))
+                                       smallest = av_masks;
+                       }
                 }
                 av_masks += BITS_TO_LONGS(masklength);
         }
-       return NULL;
+       return smallest;
  }

but ...
... I see a problem that some of the channels may be more costly to 
access than the other. It could be that reading some of the channels is 
just a matter of getting a cached value, while other could require a 
long measurement time and access to significant amount of registers. So, 
the knowledge of preferred scan masks should indeed be on the driver 
side. Hence, the ordering of the masks in the order of preference makes 
perfect sense. What we could do in the IIO core side is still go through 
all of the available masks to see if we find an exact match. I guess we 
could also document the fact that the order of masks matters.

Thanks for listening - and sorry for the noise :)

Yours,
	-- Matti

Matti Vaittinen Sept. 22, 2023, 6:07 a.m. UTC | #12

On 9/19/23 17:32, Jonathan Cameron wrote:
> 
>>>> +static int bm1390_read_raw(struct iio_dev *idev,
>>>> +               struct iio_chan_spec const *chan,
>>>> +               int *val, int *val2, long mask)
>>>> +{
>>>> +    struct bm1390_data *data = iio_priv(idev);
>>>> +    int ret;
>>>> +
>>>> +    switch (mask) {
>>>> +    case IIO_CHAN_INFO_SCALE:
>>>> +        if (chan->type == IIO_TEMP) {
>>>> +            *val = 31;
>>>> +            *val2 = 250000;
>>>> +
>>>> +            return IIO_VAL_INT_PLUS_MICRO;
>>>> +        } else if (chan->type == IIO_PRESSURE) {
>>>> +            *val = 0;
>>>> +            /*
>>>> +             * pressure in hPa is register value divided by 2048.
>>>> +             * This means kPa is 1/20480 times the register value,
>>>> +             * which equals to 48828.125 * 10 ^ -9
>>>> +             * This is 48828.125 nano kPa.
>>>> +             *
>>>> +             * When we scale this using IIO_VAL_INT_PLUS_NANO we
>>>> +             * get 48828 - which means we lose some accuracy. Well,
>>>> +             * let's try to live with that.
>>>> +             */
>>>> +            *val2 = 48828;
>>>> +
>>>> +            return IIO_VAL_INT_PLUS_NANO;
>>>> +        }
>>>> +
>>>> +        return -EINVAL;
>>>> +    case IIO_CHAN_INFO_RAW:
>>>> +        ret = iio_device_claim_direct_mode(idev);
>>>> +        if (ret)
>>>> +            return ret;
>>>> +
>>>> +        ret = bm1390_read_data(data, chan, val, val2);
>>>> +        iio_device_release_direct_mode(idev);
>>>> +        if (ret)
>>>> +            return ret;
>>>> +
>>>> +        return IIO_VAL_INT;
>>>> +    default:
>>>> +        return -EINVAL;
>>>
>>> Certainly useless, but should we break and return -EINVAL after the
>>> switch, so that it is more explicit that bm1390_read_raw() always
>>> returns a value?
>>
>> I think there is also opposite opinions on this. For my eyes the return
>> at the end of the function would also be clearer - but I think I have
>> been asked to drop the useless return when I've been working with other
>> sensors in IIO domain :) My personal preference would definitely be:
>>
>> int ret;
>>
>> switch (foo)
>> {
>> case BAR:
>> 	ret = func1();
>> 	if (ret)
>> 		break;
>>
>> 	ret = func2();
>> 	if (ret)
>> 		break;
>>
>> ...
>> 	break;
>>
>> case BAZ:
>> 	ret = -EINVAL;
>> 	break;
>> }
>>
>> return ret;
>>
>> - but I've learned to think this is not the IIO preference.
> 
> Some static analyzers get confused (probably when there is a little
> bit more going on after the function) by that and moan that some
> cases are not considered in the switch.  I got annoyed enough with the
> noise they were generating to advocate always having explicit defaults.

Oh, yes. I see I omitted the default from the example - but this was not 
what I tried to highlight ;) With a bit more thought I would've added:

default:
	ret = -EINVAL;
	break;

As you probably guess, what I was after is that for a simple (not deeply 
nested) cases like this, I would rather use a variable for return value 
and a single point of exit at the end of the function - instead of 
having returns in the switch-case. That'd suit better _my_ taste.

Yours,
	-- Matti

Jonathan Cameron Sept. 24, 2023, 12:12 p.m. UTC | #13

On Thu, 21 Sep 2023 11:17:41 +0300
Matti Vaittinen <[e-mail]> wrote:

> On 9/19/23 17:53, Jonathan Cameron wrote:
> > On Tue, 19 Sep 2023 14:28:29 +0300
> > Matti Vaittinen <[e-mail]> wrote:
> >   
> >> On 9/18/23 15:56, Matti Vaittinen wrote:  
> >>> On 9/17/23 13:35, Jonathan Cameron wrote:  
> >>>> On Fri, 15 Sep 2023 09:56:19 +0300
> >>>> Matti Vaittinen <[e-mail]> wrote:
> >>>>     
> >>>>> Support for the ROHM BM1390 pressure sensor. The BM1390GLV-Z can measure
> >>>>> pressures ranging from 300 hPa to 1300 hPa with configurable measurement
> >>>>> averaging and internal FIFO. The sensor does also provide temperature
> >>>>> measurements.
> >>>>>
> >>>>> Sensor does also contain IIR filter implemented in HW. The data-sheet
> >>>>> says the IIR filter can be configured to be "weak", "middle" or
> >>>>> "strong". Some RMS noise figures are provided in data sheet but no
> >>>>> accurate maths for the filter configurations is provided. Hence, the IIR
> >>>>> filter configuration is not supported by this driver and the filter is
> >>>>> configured to the "middle" setting (at least not for now).
> >>>>>
> >>>>> The FIFO measurement mode is only measuring the pressure and not the
> >>>>> temperature. The driver measures temperature when FIFO is flushed and
> >>>>> simply uses the same measured temperature value to all reported
> >>>>> temperatures. This should not be a problem when temperature is not
> >>>>> changing very rapidly (several degrees C / second) but allows users to
> >>>>> get the temperature measurements from sensor without any additional
> >>>>> logic.
> >>>>>
> >>>>> This driver allows the sensor to be used in two muitually exclusive
> >>>>> ways,
> >>>>>
> >>>>> 1. With trigger (data-ready IRQ).
> >>>>> In this case the FIFO is not used as we get data ready for each
> >>>>> collected
> >>>>> sample. Instead, for each data-ready IRQ we read the sample from sensor
> >>>>> and push it to the IIO buffer.
> >>>>>
> >>>>> 2. With hardware FIFO and watermark IRQ.
> >>>>> In this case the data-ready is not used but we enable watermark IRQ. At
> >>>>> each watermark IRQ we go and read all samples in FIFO and push them
> >>>>> to the
> >>>>> IIO buffer.
> >>>>>
> >>>>> Signed-off-by: Matti Vaittinen <[e-mail]>  
> >>
> >> ...
> >>  
> >>>>> +
> >>>>> +static const unsigned long bm1390_scan_masks[] = {
> >>>>> +    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0  
> >>>> Why?  Doesn't look hard to support just one or the other?
> >>>> Normally we only do this sort of limitation when there is a heavily
> >>>> optimized read routine for a set of channels and it is better
> >>>> to grab them all and throw away the ones we don't care about.
> >>>> That doesn't seem to be true here. So if the fifo grabbed both
> >>>> temp and pressure it would makes sense here, but doesn't seem
> >>>> like it does.  
> >>>
> >>> I have a feeling I have misunderstood how this mask works. I have
> >>> assumed all the channels with corresponding mask bit _can_ be enabled
> >>> simultaneously, but I have not understood they _must_ all be enabled. I
> >>> think I must go back studying this, but if all channels really _must_ be
> >>> enabled, then you are correct. It actually makes a lot of sense to
> >>> support the pressure values alone, as, according to the data-sheet, the
> >>> HW is doing a "MEMS temperature compensation" to the pressure values.
> >>> So, my assuimption is the temperature data may not be required to be
> >>> captured.
> >>>
> >>> This also means I should revise the scan masks for the BU27008, BU27010
> >>> and BU27034 light sensors as I don't think all the users want all the
> >>> channels enabled. I wonder how I have not noticed any problems when I
> >>> tested those things - did I really always enable all the channels...? @_@
> >>>
> >>> Anyways, Thanks.  
> >>
> >> Hi Jonathan,
> >>
> >> There's something in IIO scan_masks / buffer demuxing that I don't quite
> >> understand  
> 
> Thank You for the patience and the explanation Jonathan. I have a - 
> hopefully not totally wrong - feeling I understand this better. I didn't 
> understand that IIO framework has this extra logic and that 
> available_scan_masks was not really meant for telling what IIO allows 
> _users_ to enable - but it was for driver to tell IIO core what the 
> driver can give. (Naturally this impacts to what IIO allows users to 
> enable, but not directly). Eg, if I now get it right, the 
> 'available_scan_masks' is information what data flows between driver and 
> IIO. Similarly, 'active_scan_mask' tells what the IIO core 'expects' to 
> get from the driver, right? So, in bm1390 case, if I set both:
> 
> BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP),
> and
> BIT(BM1390_CHAN_PRESSURE),
> 
> to 'available_scan_masks', then it means driver is telling IIO it can 
> give both pressure and temperature, or temperature alone. As a result, 
> IIO can set either both temp and pressure to 'active_scan_mask' - in 
> which case driver should push both in the buffers - or just the 
> pressure, in which case the driver should not push the temperature.

I think you have understood this from the comment further down.  But
key is active_scan_masks is telling the driver what to put in the
buffer via iio_push_to_buffer*().  The driver has to have told
the core what it can provide (available scan masks) and the core is
picking one of those.  So active_scan_mask is always an entry
of available_scan_masks with the special case of no available_scan_masks
meaning any combination of channels is fine.
The chosen value is made up of a superset of the channels enable by
the various consumers: userspace and in kernel consumers. A typical
complex example being a SoC integrated ADCs where a couple of channels
are used for the touch screen and the rest for more general purposes.
In those cases, the in kernel touch screen driver enables the channels
it wants, and the userspace IIO interface might enable some more.
Each consumer just sees what it asks for as the IIO core repacks the
requested channels for each consumer dropping the others (maybe
leaving some valid data in the padding. I can't remember if we let
that happen or not and that code is complex so I'm not going to go
figure it out again today :)  There are some subtle cases around
consumers requesting mutually exclusive sets. We also never really
dealt with how to negotiates sample rates etc, so this isn't
quite as intuitive as it would ideally be.  E.g. the core could
subsample to meet a lower sampling rate request, but IIRC we just
let the multiple consumers fight it out for control :(


> 
> >>
> >> The bm1390 driver as sent in v2 does not do demuxing but always pushes
> >> whole chunk of data and trusts IIO to do demuxing.  
> > 
> > Yup. That should work.  But in this case you can trivially decide not to read
> > or fill in the temperature and hence save some bus cycles.
> >   
> >>
> >> 2) I noticed the 'available_scan_masks' was marked as an optional field.
> >> So, I think that if there is no restrictions to which of the channels
> >> can be enabled, then we can omit setting it. This is what I tried.
> >>
> >> It appears that when we do not populate the 'available_scan_masks' with the:  
> >>   >>> +static const unsigned long bm1390_scan_masks[] = {
> >>   >>> +    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0  
> >>
> >> things change. When I tested enabling only temperature and ran the
> >> iio_generic_buffer -c 20 -N 0 -t bm1390data-rdy-dev0 - the reported
> >> values seemed completely random.  
> > 
> > You need to pack the data correctly yourself in the driver.
> > So it adds small amount of code complexity but potentially saves bus
> > traffic.  
> 
> ...
> 
> >>
> >> Based on this experimenting (and headache obtained from reading the
> >> demuxing code) - the IIO framework does not do channel demuxing if the
> >> 'available_scan_masks' is not given? To me this was somewhat unexpected.  
> > 
> > Yes.  If you don't tell it what channel setups are available (note there can
> > be lots) you are saying that we support any random combination and have
> > to do the data packing by hand.
> >   
> 
> Okay... I think it kind of makes sense to leave an option where the 
> driver can create buffer as it wants. I am not sure I can wrap my head 
> around this to the extent that I knew when the IIO does not know what 
> channels the driver has enabled - and thus, what types of data there 
> will be - but I can accept the answer that such situations can exist :) 
> Besides, allowing the driver to do the buffer formatting may allow some 
> 'use-case specific, custom optimizations' when user-space knows what to 
> expect (for example, regarding the data alignments and ordering to for 
> example save space).
> 
> ...This leads to another thing I noticed - and to another question :)
> 
> I was experimenting with the bm1390 scan masks and iio_generic_buffer 
> tool. It seems to me that the iio_generic_buffer does not expect the 
> padding when the data in buffer is u32 + u16. When I enable the pressure 
> (32bits) and temperature (16 bits) in bm1390 and keep the time-stamp 
> disabled, the user-space will get buffer where 2 bytes of padding is 
> added to each temperature sample so the next pressure sample stays 
> correctly aligned. I believe kernel side does sane job here.
> 
> This, however, did result (at least my version of) the 
> iio_generic_buffer tool to read garbage values after the first sample. 
> My assumption is that the iio_generic_buffer does not add the padding 
> bytes to the end of last data in the scan in order to ensure also the 
> next scan will be correctly aligned.
> 
Ah. That's definitely possible. We've found similar problems with the logic
those example tools before and I can see that this case might not have turned
up much.

> I did following change to the iio_generic_buffer:
> 
> Author: Matti Vaittinen <[e-mail]>
> Date:   Thu Sep 21 10:15:19 2023 +0300
> 
>      tools: iio: iio_generic_buffer ensure alignment
> 
>      The iio_generic_buffer can return garbage values when the total size of
>      scan data is not a multiple of largest element in the scan. This can be
>      demonstrated by reading a scan consisting for example of one 4 byte and
>      one 2 byte element, where the 4 byte elemnt is first in the buffer.
> 
>      The IIO generic buffert code does not take into accunt the last two
>      padding bytes that are needed to ensure that the 4byte data for next
>      scan is correctly aligned.
> 
>      Add padding bytes required to align the next sample into the scan size.
> 
>      Signed-off-by: Matti Vaittinen <[e-mail]>
> 
> diff --git a/tools/iio/iio_generic_buffer.c b/tools/iio/iio_generic_buffer.c
> index 44bbf80f0cfd..fc562799a109 100644
> --- a/tools/iio/iio_generic_buffer.c
> +++ b/tools/iio/iio_generic_buffer.c
> @@ -54,9 +54,12 @@ enum autochan {
>   static unsigned int size_from_channelarray(struct iio_channel_info 
> *channels, int num_channels)
>   {
>          unsigned int bytes = 0;
> -       int i = 0;
> +       int i = 0, max = 0;
> +       unsigned int misalignment;
> 
>          while (i < num_channels) {
> +               if (channels[i].bytes > max)
> +                       max = channels[i].bytes;
>                  if (bytes % channels[i].bytes == 0)
>                          channels[i].location = bytes;
>                  else
> @@ -66,6 +69,16 @@ static unsigned int size_from_channelarray(struct 
> iio_channel_info *channels, in
>                  bytes = channels[i].location + channels[i].bytes;
>                  i++;
>          }
> +       /*
> +        * We wan't the data in next sample to also be properly aligned so
> +        * we'll add padding at the end if needed. TODO: should we use fixed
> +        * 8 byte alignment instead of the size of the biggest samnple?

Nope, biggest sample is correct.

> +        */
> +       misalignment = bytes % max;
> +       if (misalignment) {
> +               printf("Misalignment %u. Adding Padding %u\n", 
> misalignment,  max - misalignment);
> +               bytes += max - misalignment;
> +       }
> 
>          return bytes;
>   }
> 
> I can send this as a proper patch if you guys think it is correct.
Please do.


> 
> 
> >>
> >> Finally, when the watermark IRQ is used, we can't omit reading the
> >> pressure data because clearing the WMI is done based on the pressure
> >> data in FIFO.  
> > 
> > Hmm. That is a good reason to keep the available scan masks set.
> > Add a comment on that next to the mask.
> >   
> >>
> >> So, I would propose we do:
> >>
> >> static const unsigned long bm1390_scan_masks[] = {
> >> 	BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP),
> >> 	BIT(BM1390_CHAN_PRESSURE), 0  
> > 
> > Makes sense given need to read the pressure channel.  
> >>
> >> which better reflects what the hardware is capable of - and, unless I am
> >> missing something, also allows us to offload the buffer demuxing to the IIO.
> >>
> >> Still, as mentioned in 1), the
> >>  
> >>   >>> +static const unsigned long bm1390_scan_masks[] = {
> >>   >>> +    BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0  
> >>
> >> does not seem to prevent enabling only the temperature channel - so in
> >> the driver buffer handler we still must unconditionally read the
> >> pressure data regardles the active_scan_mask.  
> > 
> > active_scan_mask should match the above - it's separate from what is enabled.
> > active_scan_mask is on the data capture side of the demux - the buffers
> > are then fed repacked data reflecting what is enabled.  
> 
> Cool! So, the driver can rely on IIO asking for the pressure (in 
> active_scan_mask) after it has correctly set the available scan mask. It 
> may not be a big thing but I like it. It is enough to take care of 
> ensuring all required stuff is read from HW in one place (in available 
> scan masks) and not to worry about it in actual data reading path but 
> just read stuff IIO is asking to be read. I like it.
> 
> Another thing to note is that, when we build the available_scan_mask 
> array - we should either pay attention to the order of masks - or change 
> the iio_scan_mask_match() to not accept first matching subset but to go 
> through all of the masks unless it finds and exactly matching one (and 
> in general prefer the smallest subset). Not sure this is worth the extra 
> cycles though.

Yes, they do need to be ordered.  I'm not sure if that's clearly documented
or not :(  If not, a patch to add docs on that would be welcome.
I don't think it's worth likely complexity of finding the 'smallest match'
and that's not always obvious.  Imagine a driver that as options

[A, B, C, D] and [A, B, C, E] - how to chose?  May well be more efficient
to do one option than the other (require fewer address writes for the in
order one.  Hence order in will matter anyway so we make it matter completely
with a policy of first match.


> 
> Thanks again - feel like I've learned something today :)
No problem.  Was quite a while ago that we figured out how to make all this
stuff work and I remember it being very fiddle.

Hohum. I should take another spin at our documentation at some point as
I'm sure some of this is less clear than it should be (or not mentioned) :)

Have a few big docs jobs to do (non IIO) at the moment so this will probably
be a 'while'.

Jonathan

> 
> Yours,
> 	-- Matti Vaittinen
>

Jonathan Cameron Sept. 24, 2023, 12:14 p.m. UTC | #14

On Thu, 21 Sep 2023 12:00:39 +0300
Matti Vaittinen <[e-mail]> wrote:

> On 9/21/23 11:17, Matti Vaittinen wrote:
> 
> > Another thing to note is that, when we build the available_scan_mask 
> > array - we should either pay attention to the order of masks - or change 
> > the iio_scan_mask_match() to not accept first matching subset but to go 
> > through all of the masks unless it finds and exactly matching one (and 
> > in general prefer the smallest subset). Not sure this is worth the extra 
> > cycles though.  
> 
> Replying to myself and to those who I perhaps managed to confuse :)
> 
> As a result of above pondering I wrote this:
> 
> @@ -411,6 +418,8 @@ static const unsigned long 
> *iio_scan_mask_match(const unsigned long *av_masks,
>                                                  const unsigned long *mask,
>                                                  bool strict)
>   {
> +       const unsigned long *smallest = NULL;
> +
>          if (bitmap_empty(mask, masklength))
>                  return NULL;
>          while (*av_masks) {
> @@ -418,12 +427,16 @@ static const unsigned long 
> *iio_scan_mask_match(const unsigned long *av_masks,
>                          if (bitmap_equal(mask, av_masks, masklength))
>                                  return av_masks;
>                  } else {
> -                       if (bitmap_subset(mask, av_masks, masklength))
> -                               return av_masks;
> +                       if (bitmap_subset(mask, av_masks, masklength)) {
> +                               if (!smallest ||
> +                                   bitmap_weight(av_masks, BITS_PER_LONG) <
> +                                   bitmap_weight(smallest, BITS_PER_LONG))
> +                                       smallest = av_masks;
> +                       }
>                  }
>                  av_masks += BITS_TO_LONGS(masklength);
>          }
> -       return NULL;
> +       return smallest;
>   }
> 
> but ...
> ... I see a problem that some of the channels may be more costly to 
> access than the other. It could be that reading some of the channels is 
> just a matter of getting a cached value, while other could require a 
> long measurement time and access to significant amount of registers. So, 
> the knowledge of preferred scan masks should indeed be on the driver 
> side. Hence, the ordering of the masks in the order of preference makes 
> perfect sense. What we could do in the IIO core side is still go through 
> all of the available masks to see if we find an exact match. I guess we 
> could also document the fact that the order of masks matters.

I should have read on in the thread. Indeed - ordering of preferences needs
to be in driver control for exactly the reason you came up with!

Thanks,

Jonathan


> 
> Thanks for listening - and sorry for the noise :)
> 
> Yours,
> 	-- Matti
>

Patch

diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
index 7b4c2af32852..95efa32e4289 100644
--- a/drivers/iio/pressure/Kconfig
+++ b/drivers/iio/pressure/Kconfig
@@ -16,6 +16,15 @@  config ABP060MG
 	  To compile this driver as a module, choose M here: the module
 	  will be called abp060mg.
 
+config ROHM_BM1390
+	tristate "ROHM BM1390GLV-Z pressure sensor driver"
+	depends on I2C
+	help
+	  Support for the ROHM BM1390 pressure sensor. The BM1390GLV-Z
+	  can measure pressures ranging from 300 hPa to 1300 hPa with
+	  configurable measurement averaging and internal FIFO. The
+	  sensor does also provide temperature measurements.
+
 config BMP280
 	tristate "Bosch Sensortec BMP180/BMP280/BMP380/BMP580 pressure sensor driver"
 	depends on (I2C || SPI_MASTER)
diff --git a/drivers/iio/pressure/Makefile b/drivers/iio/pressure/Makefile
index c90f77210e94..436aec7e65f3 100644
--- a/drivers/iio/pressure/Makefile
+++ b/drivers/iio/pressure/Makefile
@@ -5,6 +5,7 @@ 
 
 # When adding new entries keep the list in alphabetical order
 obj-$(CONFIG_ABP060MG) += abp060mg.o
+obj-$(CONFIG_ROHM_BM1390) += rohm-bm1390.o
 obj-$(CONFIG_BMP280) += bmp280.o
 bmp280-objs := bmp280-core.o bmp280-regmap.o
 obj-$(CONFIG_BMP280_I2C) += bmp280-i2c.o
diff --git a/drivers/iio/pressure/rohm-bm1390.c b/drivers/iio/pressure/rohm-bm1390.c
new file mode 100644
index 000000000000..d3cc229d1688
--- /dev/null
+++ b/drivers/iio/pressure/rohm-bm1390.c
@@ -0,0 +1,899 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * BM1390 ROHM pressure sensor
+ *
+ * Copyright (c) 2023, ROHM Semiconductor.
+ * https://fscdn.rohm.com/en/products/databook/datasheet/ic/sensor/pressure/bm1390glv-z-e.pdf
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#define BM1390_REG_MANUFACT_ID		0x0f
+#define BM1390_REG_PART_ID		0x10
+#define BM1390_REG_POWER		0x12
+#define BM1390_MASK_POWER		BIT(0)
+#define BM1390_POWER_ON			BM1390_MASK_POWER
+#define BM1390_POWER_OFF		0x00
+#define BM1390_REG_RESET		0x13
+#define BM1390_MASK_RESET		BIT(0)
+#define BM1390_RESET_RELEASE		BM1390_MASK_RESET
+#define BM1390_RESET			0x00
+#define BM1390_REG_MODE_CTRL		0x14
+#define BM1390_MASK_MEAS_MODE		GENMASK(1, 0)
+#define BM1390_MASK_DRDY_EN		BIT(4)
+#define BM1390_MASK_WMI_EN		BIT(2)
+#define BM1390_MASK_AVE_NUM		GENMASK(7, 5)
+
+/*
+ * Data-sheet states that when the IIR is used, the AVE_NUM must be set to
+ * value 110b
+ */
+#define BM1390_IIR_AVE_NUM		0x06
+#define BM1390_REG_FIFO_CTRL		0x15
+#define BM1390_MASK_IIR_MODE		GENMASK(1, 0)
+#define BM1390_IIR_MODE_OFF		0x0
+#define BM1390_IIR_MODE_WEAK		0x1
+#define BM1390_IIR_MODE_MID		0x2
+#define BM1390_IIR_MODE_STRONG		0x3
+
+#define BM1390_MASK_FIFO_LEN		BIT(6)
+#define BM1390_MASK_FIFO_EN		BIT(7)
+#define BM1390_WMI_MIN			2
+#define BM1390_WMI_MAX			3
+
+#define BM1390_REG_FIFO_LVL		0x18
+#define BM1390_MASK_FIFO_LVL		GENMASK(2, 0)
+#define BM1390_REG_STATUS		0x19
+#define BM1390_REG_PRESSURE_BASE	0x1a
+#define BM1390_REG_TEMP_HI		0x1d
+#define BM1390_REG_TEMP_LO		0x1e
+#define BM1390_MAX_REGISTER		BM1390_REG_TEMP_LO
+
+#define BM1390_ID			0x34
+
+/* Regmap configs */
+static const struct regmap_range bm1390_volatile_ranges[] = {
+	{
+		.range_min = BM1390_REG_STATUS,
+		.range_max = BM1390_REG_STATUS,
+	},
+	{
+		.range_min = BM1390_REG_FIFO_LVL,
+		.range_max = BM1390_REG_TEMP_LO,
+	},
+};
+
+static const struct regmap_access_table bm1390_volatile_regs = {
+	.yes_ranges = &bm1390_volatile_ranges[0],
+	.n_yes_ranges = ARRAY_SIZE(bm1390_volatile_ranges),
+};
+
+static const struct regmap_range bm1390_precious_ranges[] = {
+	{
+		.range_min = BM1390_REG_STATUS,
+		.range_max = BM1390_REG_STATUS,
+	},
+};
+
+static const struct regmap_access_table bm1390_precious_regs = {
+	.yes_ranges = &bm1390_precious_ranges[0],
+	.n_yes_ranges = ARRAY_SIZE(bm1390_precious_ranges),
+};
+
+static const struct regmap_range bm1390_read_only_ranges[] = {
+	{
+		.range_min = BM1390_REG_MANUFACT_ID,
+		.range_max = BM1390_REG_PART_ID,
+	}, {
+		.range_min = BM1390_REG_FIFO_LVL,
+		.range_max = BM1390_REG_TEMP_LO,
+	},
+};
+
+static const struct regmap_access_table bm1390_ro_regs = {
+	.no_ranges = &bm1390_read_only_ranges[0],
+	.n_no_ranges = ARRAY_SIZE(bm1390_read_only_ranges),
+};
+
+static const struct regmap_range bm1390_noinc_read_ranges[] = {
+	{
+		.range_min = BM1390_REG_PRESSURE_BASE,
+		.range_max = BM1390_REG_TEMP_LO,
+	},
+};
+
+static const struct regmap_access_table bm1390_nir_regs = {
+	.yes_ranges = &bm1390_noinc_read_ranges[0],
+	.n_yes_ranges = ARRAY_SIZE(bm1390_noinc_read_ranges),
+};
+
+static const struct regmap_config bm1390_regmap = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.volatile_table = &bm1390_volatile_regs,
+	.wr_table = &bm1390_ro_regs,
+	.rd_noinc_table = &bm1390_nir_regs,
+	.precious_table = &bm1390_precious_regs,
+	.max_register = BM1390_MAX_REGISTER,
+	.cache_type = REGCACHE_RBTREE,
+	.disable_locking = true,
+};
+
+enum {
+	BM1390_STATE_SAMPLE,
+	BM1390_STATE_FIFO,
+};
+
+struct bm1390_data_buf {
+	u32 pressure;
+	__be16 temp;
+	s64 ts __aligned(8);
+};
+
+/* Pressure data is in 3 8-bit registers */
+#define BM1390_PRESSURE_SIZE	3
+
+/* BM1390 has FIFO for 4 pressure samples */
+#define BM1390_FIFO_LENGTH	4
+
+/* Temperature data is in 2 8-bit registers */
+#define BM1390_TEMP_SIZE	2
+
+struct bm1390_data {
+	s64 timestamp, old_timestamp;
+	struct iio_trigger *trig;
+	struct regmap *regmap;
+	struct device *dev;
+	struct bm1390_data_buf buf;
+	int irq;
+	unsigned int state;
+	bool trigger_enabled;
+	u8 watermark;
+
+	/* Prevent accessing sensor during FIFO read sequence */
+	struct mutex mutex;
+};
+
+enum {
+	BM1390_CHAN_PRESSURE,
+	BM1390_CHAN_TEMP,
+};
+
+static const struct iio_chan_spec bm1390_channels[] = {
+	{
+		.type = IIO_PRESSURE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		/*
+		 * When IIR is used, we must fix amount of averaged samples.
+		 * Thus we don't allow setting oversampling ratio.
+		 */
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = BM1390_CHAN_PRESSURE,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 22,
+			.storagebits = 32,
+			.endianness = IIO_LE,
+		},
+	},
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = BM1390_CHAN_TEMP,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_BE,
+		},
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(2),
+};
+
+static const unsigned long bm1390_scan_masks[] = {
+	BIT(BM1390_CHAN_PRESSURE) | BIT(BM1390_CHAN_TEMP), 0
+};
+
+static int bm1390_read_temp(struct bm1390_data *data, int *temp)
+{
+	__be16 temp_raw;
+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp_raw,
+			       sizeof(temp_raw));
+	if (ret)
+		return ret;
+
+	*temp = be16_to_cpu(temp_raw);
+
+	return 0;
+}
+
+static int bm1390_pressure_read(struct bm1390_data *data, u32 *pressure)
+{
+	int ret;
+	u8 raw[3];
+
+	ret = regmap_bulk_read(data->regmap, BM1390_REG_PRESSURE_BASE,
+			       raw, sizeof(raw));
+	if (ret < 0)
+		return ret;
+
+	*pressure = (u32)(raw[2] >> 2 | raw[1] << 6 | raw[0] << 14);
+
+	return 0;
+}
+
+ /* The enum values map directly to register bits */
+enum bm1390_meas_mode {
+	BM1390_MEAS_MODE_STOP = 0x0,
+	BM1390_MEAS_MODE_1SHOT = 0x1,
+	BM1390_MEAS_MODE_CONTINUOUS = 0x2,
+};
+
+static int bm1390_meas_set(struct bm1390_data *data, enum bm1390_meas_mode mode)
+{
+	return regmap_update_bits(data->regmap, BM1390_REG_MODE_CTRL,
+				  BM1390_MASK_MEAS_MODE, mode);
+}
+
+/*
+ * If the trigger is not used we just wait until the measurement has
+ * completed. The data-sheet says maximum measurement cycle (regardless
+ * the AVE_NUM) is 200 mS so let's just sleep at least that long. If speed
+ * is needed the trigger should be used.
+ */
+#define BM1390_MAX_MEAS_TIME_MS 205
+
+static int bm1390_read_data(struct bm1390_data *data,
+			struct iio_chan_spec const *chan, int *val, int *val2)
+{
+	int ret;
+
+	mutex_lock(&data->mutex);
+	/*
+	 * We use 'continuous mode' even for raw read because according to the
+	 * data-sheet an one-shot mode can't be used with IIR filter.
+	 */
+	ret = bm1390_meas_set(data, BM1390_MEAS_MODE_CONTINUOUS);
+	if (ret)
+		goto unlock_out;
+
+	switch (chan->type) {
+	case IIO_PRESSURE:
+		msleep(BM1390_MAX_MEAS_TIME_MS);
+		ret = bm1390_pressure_read(data, val);
+		break;
+	case IIO_TEMP:
+		msleep(BM1390_MAX_MEAS_TIME_MS);
+		ret = bm1390_read_temp(data, val);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+	bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int bm1390_read_raw(struct iio_dev *idev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	struct bm1390_data *data = iio_priv(idev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		if (chan->type == IIO_TEMP) {
+			*val = 31;
+			*val2 = 250000;
+
+			return IIO_VAL_INT_PLUS_MICRO;
+		} else if (chan->type == IIO_PRESSURE) {
+			*val = 0;
+			/*
+			 * pressure in hPa is register value divided by 2048.
+			 * This means kPa is 1/20480 times the register value,
+			 * which equals to 48828.125 * 10 ^ -9
+			 * This is 48828.125 nano kPa.
+			 *
+			 * When we scale this using IIO_VAL_INT_PLUS_NANO we
+			 * get 48828 - which means we lose some accuracy. Well,
+			 * let's try to live with that.
+			 */
+			*val2 = 48828;
+
+			return IIO_VAL_INT_PLUS_NANO;
+		}
+
+		return -EINVAL;
+	case IIO_CHAN_INFO_RAW:
+		ret = iio_device_claim_direct_mode(idev);
+		if (ret)
+			return ret;
+
+		ret = bm1390_read_data(data, chan, val, val2);
+		iio_device_release_direct_mode(idev);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int __bm1390_fifo_flush(struct iio_dev *idev, unsigned int samples,
+			       bool irq)
+{
+	struct bm1390_data *data = iio_priv(idev);
+	struct bm1390_data_buf buffer;
+	int smp_lvl, ret, i, warn;
+	u64 sample_period;
+	__be16 temp = 0;
+
+	/*
+	 * If the IC is accessed during FIFO read samples can be dropped.
+	 * Prevent access until FIFO_LVL is read
+	 */
+	if (test_bit(BM1390_CHAN_TEMP, idev->active_scan_mask)) {
+		ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp,
+				       sizeof(temp));
+		if (ret)
+			return ret;
+	}
+
+	ret = regmap_read(data->regmap, BM1390_REG_FIFO_LVL, &smp_lvl);
+	if (ret)
+		return ret;
+
+	smp_lvl = FIELD_GET(BM1390_MASK_FIFO_LVL, smp_lvl);
+	if (!smp_lvl)
+		return 0;
+
+	if (smp_lvl > 4) {
+		/*
+		 * The fifo holds maximum of 4 samples so valid values
+		 * should be 0, 1, 2, 3, 4 - rest are probably bit errors
+		 * in I2C line. Don't overflow if this happens.
+		 */
+		dev_err(data->dev, "bad FIFO level %d\n", smp_lvl);
+		smp_lvl = 4;
+	}
+
+	sample_period = data->timestamp - data->old_timestamp;
+	do_div(sample_period, smp_lvl);
+
+	if (samples && smp_lvl > samples)
+		smp_lvl = samples;
+
+	for (i = 0; i < smp_lvl; i++) {
+		ret = bm1390_pressure_read(data, &buffer.pressure);
+		if (ret)
+			break;
+
+		buffer.temp = temp;
+		/*
+		 * Old timestamp is either the previous sample IRQ time,
+		 * previous flush-time or, if this was first sample, the enable
+		 * time. When we add a sample period to that we should get the
+		 * best approximation of the time-stamp we are handling.
+		 *
+		 * Idea is to always keep the "old_timestamp" matching the
+		 * timestamp which we are currently handling.
+		 */
+		data->old_timestamp += sample_period;
+
+		iio_push_to_buffers_with_timestamp(idev, &buffer,
+						   data->old_timestamp);
+	}
+	/* Reading the FIFO_LVL closes the FIFO access sequence */
+	warn = regmap_read(data->regmap, BM1390_REG_FIFO_LVL, &smp_lvl);
+	if (warn)
+		dev_warn(data->dev, "Closing FIFO sequence failed\n");
+
+	if (!ret)
+		return ret;
+
+	return smp_lvl;
+}
+
+static int bm1390_fifo_flush(struct iio_dev *idev, unsigned int samples)
+{
+	struct bm1390_data *data = iio_priv(idev);
+	int ret;
+
+	/*
+	 * If fifo_flush is being called from IRQ handler we know the stored timestamp
+	 * is fairly accurate for the last stored sample. If we are
+	 * called as a result of a read operation from userspace and hence
+	 * before the watermark interrupt was triggered, take a timestamp
+	 * now. We can fall anywhere in between two samples so the error in this
+	 * case is at most one sample period.
+	 * We need to have the IRQ disabled or we risk of messing-up
+	 * the timestamps. If we are ran from IRQ, then the
+	 * IRQF_ONESHOT has us covered - but if we are ran by the
+	 * user-space read we need to disable the IRQ to be on a safe
+	 * side. We do this usng synchronous disable so that if the
+	 * IRQ thread is being ran on other CPU we wait for it to be
+	 * finished.
+	 */
+	disable_irq(data->irq);
+	data->timestamp = iio_get_time_ns(idev);
+
+	mutex_lock(&data->mutex);
+	ret = __bm1390_fifo_flush(idev, samples, false);
+	mutex_unlock(&data->mutex);
+
+	enable_irq(data->irq);
+
+	return ret;
+}
+
+static int bm1390_set_watermark(struct iio_dev *idev, unsigned int val)
+{
+	struct bm1390_data *data = iio_priv(idev);
+
+	if (val < BM1390_WMI_MIN || val > BM1390_WMI_MAX)
+		return -EINVAL;
+
+	mutex_lock(&data->mutex);
+	data->watermark = val;
+	mutex_unlock(&data->mutex);
+
+	return 0;
+}
+
+static const struct iio_info bm1390_info = {
+	.read_raw = &bm1390_read_raw,
+	.validate_trigger = iio_validate_own_trigger,
+	.hwfifo_set_watermark = bm1390_set_watermark,
+	.hwfifo_flush_to_buffer = bm1390_fifo_flush,
+};
+
+static int bm1390_chip_init(struct bm1390_data *data)
+{
+	int ret;
+
+	ret = regmap_write_bits(data->regmap, BM1390_REG_POWER,
+				BM1390_MASK_POWER, BM1390_POWER_ON);
+	if (ret)
+		return ret;
+
+	msleep(1);
+
+	ret = regmap_write_bits(data->regmap, BM1390_REG_RESET,
+				BM1390_MASK_RESET, BM1390_RESET);
+	if (ret)
+		return ret;
+
+	msleep(1);
+
+	ret = regmap_write_bits(data->regmap, BM1390_REG_RESET,
+				BM1390_MASK_RESET, BM1390_RESET_RELEASE);
+	if (ret)
+		return ret;
+
+	msleep(1);
+
+	ret = regmap_reinit_cache(data->regmap, &bm1390_regmap);
+	if (ret) {
+		dev_err(data->dev, "Failed to reinit reg cache\n");
+		return ret;
+	}
+
+	/*
+	 * Default to use IIR filter in "middle" mode. Also the AVE_NUM must
+	 * be fixed when IIR is in use.
+	 */
+	ret = regmap_update_bits(data->regmap, BM1390_REG_MODE_CTRL,
+				 BM1390_MASK_AVE_NUM, BM1390_IIR_AVE_NUM);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(data->regmap, BM1390_REG_FIFO_CTRL,
+				  BM1390_MASK_IIR_MODE, BM1390_IIR_MODE_MID);
+}
+
+static int bm1390_fifo_set_wmi(struct bm1390_data *data)
+{
+	u8 regval;
+
+	regval = data->watermark - BM1390_WMI_MIN;
+	regval = FIELD_PREP(BM1390_MASK_FIFO_LEN, regval);
+
+	return regmap_update_bits(data->regmap, BM1390_REG_FIFO_CTRL,
+				  BM1390_MASK_FIFO_LEN, regval);
+}
+
+static int bm1390_fifo_enable(struct iio_dev *idev)
+{
+	struct bm1390_data *data = iio_priv(idev);
+	int ret;
+
+	mutex_lock(&data->mutex);
+	if (data->trigger_enabled) {
+		ret = -EBUSY;
+		goto unlock_out;
+	}
+
+	/* Update watermark to HW */
+	ret = bm1390_fifo_set_wmi(data);
+	if (ret)
+		goto unlock_out;
+
+	/* Enable WMI_IRQ */
+	ret = regmap_set_bits(data->regmap, BM1390_REG_MODE_CTRL,
+				 BM1390_MASK_WMI_EN);
+	if (ret)
+		goto unlock_out;
+
+	/* Enable FIFO */
+	ret = regmap_set_bits(data->regmap, BM1390_REG_FIFO_CTRL,
+			      BM1390_MASK_FIFO_EN);
+	if (ret)
+		goto unlock_out;
+
+	data->state = BM1390_STATE_FIFO;
+
+	data->old_timestamp = iio_get_time_ns(idev);
+	ret = bm1390_meas_set(data, BM1390_MEAS_MODE_CONTINUOUS);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int bm1390_fifo_disable(struct iio_dev *idev)
+{
+	struct bm1390_data *data = iio_priv(idev);
+	int ret;
+
+	msleep(1);
+
+	mutex_lock(&data->mutex);
+	/* Disable FIFO */
+	ret = regmap_clear_bits(data->regmap, BM1390_REG_FIFO_CTRL,
+				BM1390_MASK_FIFO_EN);
+	if (ret)
+		goto unlock_out;
+
+	data->state = BM1390_STATE_SAMPLE;
+
+	/* Disable WMI_IRQ */
+	ret = regmap_clear_bits(data->regmap, BM1390_REG_MODE_CTRL,
+				 BM1390_MASK_WMI_EN);
+	if (ret)
+		goto unlock_out;
+
+	ret = bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int bm1390_buffer_postenable(struct iio_dev *idev)
+{
+	/*
+	 * If we use data-ready trigger, then the IRQ masks should be handled by
+	 * trigger enable and the hardware buffer is not used but we just update
+	 * results to the IIO FIFO when data-ready triggers.
+	 */
+	if (iio_device_get_current_mode(idev) == INDIO_BUFFER_TRIGGERED)
+		return 0;
+
+	return bm1390_fifo_enable(idev);
+}
+
+static int bm1390_buffer_predisable(struct iio_dev *idev)
+{
+	if (iio_device_get_current_mode(idev) == INDIO_BUFFER_TRIGGERED)
+		return 0;
+
+	return bm1390_fifo_disable(idev);
+}
+
+static const struct iio_buffer_setup_ops bm1390_buffer_ops = {
+	.postenable = bm1390_buffer_postenable,
+	.predisable = bm1390_buffer_predisable,
+};
+
+static irqreturn_t bm1390_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *idev = pf->indio_dev;
+	struct bm1390_data *data = iio_priv(idev);
+	int ret, status;
+
+	/* DRDY is acked by reading status reg */
+	ret = regmap_read(data->regmap, BM1390_REG_STATUS, &status);
+	if (ret || !status)
+		return IRQ_NONE;
+
+	dev_dbg(data->dev, "DRDY trig status 0x%x\n", status);
+
+	ret = bm1390_pressure_read(data, &data->buf.pressure);
+	if (ret) {
+		dev_warn(data->dev, "sample read failed %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI,
+			       &data->buf.temp, BM1390_TEMP_SIZE);
+	if (ret)
+		dev_warn(data->dev, "temp read failed %d\n", ret);
+
+	iio_push_to_buffers_with_timestamp(idev, &data->buf, data->timestamp);
+	iio_trigger_notify_done(idev->trig);
+
+	return IRQ_HANDLED;
+}
+
+/* Get timestamps and wake the thread if we need to read data */
+static irqreturn_t bm1390_irq_handler(int irq, void *private)
+{
+	struct iio_dev *idev = private;
+	struct bm1390_data *data = iio_priv(idev);
+
+	data->timestamp = iio_get_time_ns(idev);
+
+	if (data->state == BM1390_STATE_FIFO || data->trigger_enabled)
+		return IRQ_WAKE_THREAD;
+
+	return IRQ_NONE;
+}
+
+static irqreturn_t bm1390_irq_thread_handler(int irq, void *private)
+{
+	struct iio_dev *idev = private;
+	struct bm1390_data *data = iio_priv(idev);
+	int ret = IRQ_NONE;
+
+	mutex_lock(&data->mutex);
+
+	if (data->trigger_enabled) {
+		iio_trigger_poll_nested(data->trig);
+		ret = IRQ_HANDLED;
+	}
+
+	if (data->state == BM1390_STATE_FIFO) {
+		int ok;
+
+		ok = __bm1390_fifo_flush(idev, BM1390_TEMP_SIZE, true);
+		if (ok > 0)
+			ret = IRQ_HANDLED;
+	}
+
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int bm1390_set_drdy_irq(struct bm1390_data *data, bool en)
+{
+	if (en)
+		return regmap_set_bits(data->regmap, BM1390_REG_MODE_CTRL,
+				       BM1390_MASK_DRDY_EN);
+	return regmap_clear_bits(data->regmap, BM1390_REG_MODE_CTRL,
+				 BM1390_MASK_DRDY_EN);
+}
+
+static int bm1390_trigger_set_state(struct iio_trigger *trig,
+				    bool state)
+{
+	struct bm1390_data *data = iio_trigger_get_drvdata(trig);
+	int ret = 0;
+
+	mutex_lock(&data->mutex);
+
+	if (data->trigger_enabled == state)
+		goto unlock_out;
+
+	if (data->state == BM1390_STATE_FIFO) {
+		dev_warn(data->dev, "Can't set trigger when FIFO enabled\n");
+		ret = -EBUSY;
+		goto unlock_out;
+	}
+
+	data->trigger_enabled = state;
+
+	if (state) {
+		ret = bm1390_meas_set(data, BM1390_MEAS_MODE_CONTINUOUS);
+		if (ret)
+			goto unlock_out;
+	} else {
+		int dummy;
+
+		ret = bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);
+		if (ret)
+			goto unlock_out;
+
+		/*
+		 * We need to read the status register in order to ACK the
+		 * data-ready which may have been generated just before we
+		 * disabled the measurement.
+		 */
+		ret = regmap_read(data->regmap, BM1390_REG_STATUS, &dummy);
+		if (ret)
+			dev_warn(data->dev, "status read failed\n");
+	}
+
+	ret = bm1390_set_drdy_irq(data, state);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static const struct iio_trigger_ops bm1390_trigger_ops = {
+	.set_trigger_state = bm1390_trigger_set_state,
+};
+
+static int bm1390_setup_trigger(struct bm1390_data *data, struct iio_dev *idev,
+				int irq)
+{
+	struct iio_trigger *itrig;
+	char *name;
+	int ret;
+
+	/* Nothing to do if we don't have IRQ for data-ready and WMI */
+	if (irq < 0)
+		return 0;
+
+	ret = devm_iio_triggered_buffer_setup(data->dev, idev,
+					      &iio_pollfunc_store_time,
+					      &bm1390_trigger_handler,
+					      &bm1390_buffer_ops);
+
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "iio_triggered_buffer_setup FAIL\n");
+
+	itrig = devm_iio_trigger_alloc(data->dev, "%sdata-rdy-dev%d", idev->name,
+					    iio_device_id(idev));
+	if (!itrig)
+		return -ENOMEM;
+
+	data->trig = itrig;
+	idev->available_scan_masks = bm1390_scan_masks;
+
+	itrig->ops = &bm1390_trigger_ops;
+	iio_trigger_set_drvdata(itrig, data);
+
+	name = devm_kasprintf(data->dev, GFP_KERNEL, "%s-bm1390",
+			      dev_name(data->dev));
+
+	ret = devm_request_threaded_irq(data->dev, irq, bm1390_irq_handler,
+					&bm1390_irq_thread_handler,
+					IRQF_ONESHOT, name, idev);
+	if (ret)
+		return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
+
+
+	ret = devm_iio_trigger_register(data->dev, itrig);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "Trigger registration failed\n");
+
+	return 0;
+}
+
+static int bm1390_probe(struct i2c_client *i2c)
+{
+	struct bm1390_data *data;
+	struct regmap *regmap;
+	struct iio_dev *idev;
+	struct device *dev;
+	unsigned int part_id;
+	int ret;
+
+	dev = &i2c->dev;
+
+	regmap = devm_regmap_init_i2c(i2c, &bm1390_regmap);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "Failed to initialize Regmap\n");
+
+	idev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!idev)
+		return -ENOMEM;
+
+	ret = devm_regulator_get_enable(dev, "vdd");
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get regulator\n");
+
+	data = iio_priv(idev);
+
+	ret = regmap_read(regmap, BM1390_REG_PART_ID, &part_id);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to access sensor\n");
+
+	if (part_id != BM1390_ID)
+		dev_warn(dev, "unknown device 0x%x\n", part_id);
+
+	data->regmap = regmap;
+	data->dev = dev;
+	data->irq = i2c->irq;
+	/*
+	 * For now we just allow BM1390_WMI_MIN to BM1390_WMI_MAX and
+	 * discard every other configuration when triggered mode is not used.
+	 */
+	data->watermark = BM1390_WMI_MAX;
+	mutex_init(&data->mutex);
+
+	idev->channels = bm1390_channels;
+	idev->num_channels = ARRAY_SIZE(bm1390_channels);
+	idev->name = "bm1390";
+	idev->info = &bm1390_info;
+	idev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
+
+	ret = bm1390_chip_init(data);
+	if (ret)
+		return dev_err_probe(dev, ret, "sensor init failed\n");
+
+	ret = bm1390_setup_trigger(data, idev, i2c->irq);
+	if (ret)
+		return ret;
+
+	ret = devm_iio_device_register(dev, idev);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Unable to register iio device\n");
+
+	return 0;
+}
+
+static const struct of_device_id bm1390_of_match[] = {
+	{ .compatible = "rohm,bm1390glv-z" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, bm1390_of_match);
+
+static const struct i2c_device_id bm1390_id[] = {
+	{ "bm1390glv-z", },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, bm1390_id);
+
+static struct i2c_driver bm1390_driver = {
+	.driver = {
+		.name = "bm1390",
+		.of_match_table = bm1390_of_match,
+		/*
+		 * The probe issues a few msleep()s - which can result
+		 * measurable delays. Additionally, enabling the VDD may cause
+		 * some (slight) delay depending on the ramp-rate of the
+		 * regulator. Delays are propably magnitude of tens of
+		 * milliseconds - but async probing should not be a problem so
+		 * we should have nothing to lose here. Let's revise this if
+		 * async probing proves to cause problems here.
+		 */
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+	},
+	.probe = bm1390_probe,
+	.id_table = bm1390_id,
+};
+module_i2c_driver(bm1390_driver);
+
+MODULE_AUTHOR("Matti Vaittinen <[e-mail]>");
+MODULE_DESCRIPTION("Driver for ROHM BM1390 pressure sensor");
+MODULE_LICENSE("GPL");