| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZPifWlRvX5hLFPvG@smile.fi.intel.com>
Date: Wed, 6 Sep 2023 18:48:42 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Matti Vaittinen <mazziesaccount@...il.com>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
Jonathan Cameron <jic23@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Angel Iglesias <ang.iglesiasg@...il.com>,
Andreas Klinger <ak@...klinger.de>, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] iio: pressure: Support ROHM BU1390
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
Powered by blists - more mailing lists