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_buf { > + u32 pressure; > + __be16 temp; > + s64 ts __aligned(8); Would like to see aligned_s64, but it will depend on my series, so not your problem and not right now :-) > +}; ... > +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 and previous branch has 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? -- With Best Regards, Andy Shevchenko