On Sat, 16 Sep 2023 10:01:06 +0200
Christophe JAILLET <christophe.jaillet@xxxxxxxxxx> 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 <mazziesaccount-Re5JQEeQqe8AvxtiuMwx3w@xxxxxxxxxxxxxxxx>
> >
>
> ...
>
> > +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
