On Wed, Sep 06, 2023 at 03:37:48PM +0300, Matti Vaittinen 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.
...
+struct bm1390_data {
+ int64_t timestamp, old_timestamp;
Out of a sudden int64_t instead of u64?
+ 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;
Or u8 instead of uint8_t?
+ /* Prevent accessing sensor during FIFO read sequence */
+ struct mutex mutex;
+};
...
+static int bm1390_read_temp(struct bm1390_data *data, int *temp)
+{
+ u8 temp_reg[2] __aligned(2);
Why?! Just use proper bitwise type.
+ __be16 *temp_raw;
+ int ret;
+ s16 val;
+ bool negative;
+
+ ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp_reg,
+ sizeof(temp_reg));
+ if (ret)
+ return ret;
+
+ if (temp_reg[0] & 0x80)
+ negative = true;
+ else
+ negative = false;
+ temp_raw = (__be16 *)&temp_reg[0];
Heck no. Make temp_reg be properly typed.
+ val = be16_to_cpu(*temp_raw);
+
+ if (negative) {
+ /*
+ * Two's complement. I am not sure if all supported
+ * architectures actually use 2's complement represantation of
+ * signed ints. If yes, then we could just do the endianes
+ * conversion and say this is the s16 value. However, as I
+ * don't know, and as the conversion is pretty simple. let's
+ * just convert the signed 2's complement to absolute value and
+ * multiply by -1.
+ */
+ val = ~val + 1;
+ val *= -1;
+ }
+
+ *temp = val;
+
+ 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[0], 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 */
In this case assign _all_ values explicitly. Currently it's prone to errors
if somebody squeezed a value in between.
+enum bm1390_meas_mode {
+ BM1390_MEAS_MODE_STOP = 0x0,
+ BM1390_MEAS_MODE_1SHOT,
+ BM1390_MEAS_MODE_CONTINUOUS,
+};
...
+ mutex_lock(&data->mutex);
Wouldn't you like to start using cleanup.h?
...
+ /*
+ * 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
Missing period at the end.
+ */
...
+ goto unlock_out;
cleanup.h makes these goto:s unneeded.
...
+ 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;
Why not switch-case like other drivers do?
+ 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 IIO_VAL_INT;
+
+ return ret;
Why not usual pattern?
if (ret)
return ret;
+ default:
+ return -EINVAL;
+ }
...
+ smp_lvl = FIELD_GET(BM1390_MASK_FIFO_LVL, smp_lvl);
+
Unneeded blank line.
+ if (smp_lvl > 4) {
+ /*
+ * 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;
+ }
+ if (!smp_lvl)
+ return ret;
This can be checked first as it's and error condition
no side effects on this. Also, wouldn't be better to use error code explicitly?
...
+static int bm1390_write_raw(struct iio_dev *idev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ int ret;
+
+ ret = iio_device_claim_direct_mode(idev);
+ if (ret)
+ return ret;
+ switch (mask) {
+ default:
+ ret = -EINVAL;
+ }
This needs a comment: Why we have a dead code.
+ iio_device_release_direct_mode(idev);
+
+ return ret;
+}
...
+ /*
+ * Default to use IIR filter in "middle" mode. Also the AVE_NUM must
+ * be fixed when IIR is in use
Missing period.
+ */
...
+ ret = regmap_read(data->regmap, BM1390_REG_STATUS,
+ &dummy);
This is perfectly one line even for fanatics of 80 characters limitation.
+ if (ret || !dummy)
+ return IRQ_NONE;
...
+ if (state) {
+ ret = bm1390_meas_set(data, BM1390_MEAS_MODE_CONTINUOUS);
This ret is never used, what's going on here?
+ } else {
+ int dummy;
+
+ ret = bm1390_meas_set(data, BM1390_MEAS_MODE_STOP);
+
+ /*
+ * 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.
+ */
+ if (!ret)
+ ret = regmap_read(data->regmap, BM1390_REG_STATUS,
+ &dummy);
+ }
+
+ ret = bm1390_set_drdy_irq(data, state);
+ if (ret)
+ goto unlock_out;
+
+
One blank line is enough.
+unlock_out:
+ mutex_unlock(&data->mutex);
+
+ return ret;
+
We do not put blank lines at the end of functions.
+}
...
+ ret = devm_iio_triggered_buffer_setup(data->dev, idev,
+ &iio_pollfunc_store_time,
+ &bm1390_trigger_handler,
+ &bm1390_buffer_ops);
+
Yet another redundant blank line. I dunno how you manage to almost in every
second attempt to randomly place blank lines here and there... I feel like
a conspiracy theory against myself :-)
+ if (ret)
+ return dev_err_probe(data->dev, ret,
+ "iio_triggered_buffer_setup FAIL\n");
...
+
+
Ditto.
+ ret = devm_iio_trigger_register(data->dev, itrig);
+ if (ret)
+ return dev_err_probe(data->dev, ret,
+ "Trigger registration failed\n");
+
+
Ditto.
+ 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 ret;
Do you expect anything than 0 here?