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