Message-ID: <20230917105640.6b3fe6b4@jic23-huawei>
Date:   Sun, 17 Sep 2023 10:56:40 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc:     mazziesaccount@...il.com, ak@...klinger.de,
        andriy.shevchenko@...ux.intel.com, ang.iglesiasg@...il.com,
        bbara93@...il.com, conor+dt@...nel.org, devicetree@...r.kernel.org,
        krzysztof.kozlowski+dt@...aro.org, lars@...afoo.de,
        linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
        matti.vaittinen@...rohmeurope.com, robh+dt@...nel.org
Subject: Re: [PATCH v2 2/3] iio: pressure: Support ROHM BU1390

On Sat, 16 Sep 2023 10:01:06 +0200
Christophe JAILLET <christophe.jaillet@...adoo.fr> 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@...lic.gmane.org>
> >   
> 
> ...
> 
> > +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

Powered by blists - more mailing lists