On 9/24/23 19:29, Jonathan Cameron wrote:
On Fri, 22 Sep 2023 14:19:10 +0300
Matti Vaittinen <mazziesaccount@xxxxxxxxx> 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 <mazziesaccount@xxxxxxxxx>
Main question here is whether the fifo mode is useable if now interrupt
is wired up?
I don't think it is.
I'm guessing not really as it's not worth dealing with making
it work if someone can't be bothered to connect the wire. In which case
I'd expect few more things to be disable if no IRQ.
Also, didn't think we'd have a validate_own_trigger callback set
as that means the buffer is potentially also something that should go
away if no interrupts are wired.
This was what I originally had been thinking. You made me think that we
should perhaps allow using an external trigger as well - and I tried
doing that (but obviously didn't test it as I forgot the
validate_own_trigger in place). I don't really know the potential
applications for this sensor so I can't imagine if it'd be useful to
support external triggers. Still, if it is just a matter of dropping the
validate_own_trigger - then, why limiting the users?
Anyhow, other than that a few trivial things inline.
---
Revision history:
v2 => v3:
- Read temperature only after FIFO is read to overcome a HW quirck
- Drop unused defines
- Allow scanning the pressure only
- Some clarifying comments added, some made less verbose
- warn if measurement stp fails
- use IIO_VAL_FRACTIONAL for pressure scale
- don't disable IRQ but use timestamp from stack
- fix amount of samples to read
- minor styling
- better separate buffer and trigger parts
- allow buffer even when there is no IRQ
with external trigger to be supported.
- add completely, utterly useless NULL check because we have the cycles
to waste (grumbles)
)Smiles)
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..82a0cd61d215
--- /dev/null
+++ b/drivers/iio/pressure/rohm-bm1390.c
+
+/*
+ * 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, warn;
+
+ 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;
+ }
+ warn = bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);
+ if (warn)
+ dev_warn(data->dev, "Failed to stop measurementi (%d)\n", warn);
measurement
Thanks! Seems like an useless vim 'insert' mode change ;)
+unlock_out:
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static int __bm1390_fifo_flush(struct iio_dev *idev, unsigned int samples,
+ s64 timestamp)
+{
+ /* BM1390_FIFO_LENGTH is small so we shouldn't run out of stack */
+ struct bm1390_data_buf buffer[BM1390_FIFO_LENGTH];
+ struct bm1390_data *data = iio_priv(idev);
+ int smp_lvl, ret, i, warn, dummy;
+ u64 sample_period;
+ __be16 temp = 0;
+
+ 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 > BM1390_FIFO_LENGTH) {
+ /*
+ * 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 = BM1390_FIFO_LENGTH;
+ }
+
+ sample_period = timestamp - data->old_timestamp;
+ do_div(sample_period, smp_lvl);
+
+ if (samples && smp_lvl > samples)
+ smp_lvl = samples;
+
+
+ /*
+ * After some testing it appears that the temperature is not readable
+ * untill the FIFO access has been done after the WMI. Thus, we need
Spell check. until (Why it doesn't have 2 ls is beyond me but that's English being
annoyingly irregular)
Thanks. I keep mistyping it. I think I've seen checkpatch warnings on
this kind of mistakes before. Makes me wonder if I forgot to run the
checkpatch after latest modifications...
+ * to read the all pressure values to memory and read the temperature
+ * only after that.
+ */
+ for (i = 0; i < smp_lvl; i++) {
+ /*
+ * When we start reading data from the FIFO the sensor goes to
+ * special FIFO reading mode. If any other register is accessed
+ * during the FIFO read, samples can be dropped. Prevent access
+ * until FIFO_LVL is read. We have mutex locked and we do also
+ * go performing reading of FIFO_LVL even if this read fails.
+ */
+ if (test_bit(BM1390_CHAN_PRESSURE, idev->active_scan_mask)) {
+ ret = bm1390_pressure_read(data, &buffer[i].pressure);
+ if (ret)
+ break;
+ }
+
+ /*
+ * 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;
+ buffer[i].ts = data->old_timestamp;
+ }
+ /* Reading the FIFO_LVL closes the FIFO access sequence */
+ warn = regmap_read(data->regmap, BM1390_REG_FIFO_LVL, &dummy);
+ if (warn)
+ dev_warn(data->dev, "Closing FIFO sequence failed\n");
+
+ if (ret)
+ return ret;
+
+ 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;
+ pr_info("Temp before reading the FIFO %u\n", be16_to_cpu(temp));
Why this print?
Thanks! Forgot a debug phase print here...
+ }
+
+ if (ret)
+ return ret;
+
+ for (i = 0; i < smp_lvl; i++) {
+ buffer[i].temp = temp;
+ iio_push_to_buffers_with_timestamp(idev, &buffer[i],
+ buffer[i].ts);
You can just use iio_push_to_buffers() if you've already filled
in the timestamp by hand. The _with_timestamp() version was just to reduce
boilerplate (and given the main it has caused over the years, I'm not
sure it was a good idea!)
Right. I had a brainfart on this one. Thought that the
iio_push_to_buffers_with_timestamp() would help in case where timestamp
was disabled by the user. Now I see this thought was very much not
correct. That's what I get when writing code at the afternoon, and
especially at Friday afternoon :) My brains are shutting down early :)
+ }
+
+ return smp_lvl;
+}
+
+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,
Given my (possibly incorrect assumption) that the fifo is useless
without the interrupt, I'd expect to see another version of this
that has read_raw only set.
Yes...
Also, why do we need validate_own_trigger. I thought this could
be used with other triggers. If not, then don't register the buffer
for the case with no interrupt either.
... and yes. Thank you :)
+};
+
+
+static int bm1390_fifo_set_wmi(struct bm1390_data *data)
+{
+ u8 regval;
+
+ regval = data->watermark - BM1390_WMI_MIN;
Trivial: I'd rather we didn't put stuff that clearly isn't the register
value in a variable called regval. I'd just go directly to
regval = FIELD_PREP(BM1390_MASK_FIFO_LEN,
data->watermark - BMI1390_WMI_MIN);
and avoid that first 'mis'use.
Ok.
+ 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;
+
+ /* We can't do buffered stuff without IRQ as we never get WMI */
+ if (data->irq <= 0)
+ return -EINVAL;
+
+ 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);
I'm sure it works in this order but to my mind it would make more sense
(and might still work) for fifo_disable() to be reverse of steps
in fifo_enable(). So I'd expect the mode change first.
I think stopping the masurement should indeed be done first. I'm not
sure why the order is this. I know I tried suffling the order of
starting the measurement while trying to figure out the temperature
measurements for first FIFO sample - but I don't think I touched the
disabling. So, it has probably been like this from the v1. I'll revise
this, thanks!
+
+unlock_out:
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+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");
+
+ ret = devm_regulator_get_enable(dev, "vdd");
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to get regulator\n");
+
+ 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);
+
+ idev = devm_iio_device_alloc(dev, sizeof(*data));
+ if (!idev)
+ return -ENOMEM;
+
+ data = iio_priv(idev);
+ 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;
Silly question that I might resolve as I read on.
If we don't have the WMI interrupt, do we have buffer_software support?
It could be made to work with a timer, but do you do so?
No. I can't say the exact meaning of all these flags is 100% clear to me
- but I think we only have the INDIO_BUFFER_TRIGGERED if we don't have
the IRQ. Thanks!
+
+ ret = bm1390_chip_init(data);
+ if (ret)
+ return dev_err_probe(dev, ret, "sensor init failed\n");
+
+ ret = bm1390_setup_buffer(data, idev);
+ if (ret)
+ return ret;
+
+ /* No trigger if we don't have IRQ for data-ready and WMI */
+ if (i2c->irq > 0) {
+ 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;
+}
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
