Message-ID: <20240622123251.GB123707@vamoiridPC>
Date: Sat, 22 Jun 2024 14:32:51 +0200
From: Vasileios Amoiridis <vassilisamir@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Vasileios Amoiridis <vassilisamir@...il.com>, lars@...afoo.de,
	andriy.shevchenko@...ux.intel.com, ang.iglesiasg@...il.com,
	mazziesaccount@...il.com, ak@...klinger.de,
	petre.rodan@...dimension.ro, phil@...pberrypi.com, 579lpy@...il.com,
	linus.walleij@...aro.org, semen.protsenko@...aro.org,
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
	Adam Rizkalla <ajarizzo@...il.com>
Subject: Re: [PATCH v8 3/3] iio: pressure: bmp280: Add triggered buffer
 support

On Sat, Jun 22, 2024 at 10:40:39AM +0100, Jonathan Cameron wrote:
> On Tue, 18 Jun 2024 01:05:40 +0200
> Vasileios Amoiridis <vassilisamir@...il.com> wrote:
> 
> > BMP2xx, BME280, BMP3xx, and BMP5xx use continuous buffers for their
> > temperature, pressure and humidity readings. This facilitates the
> > use of burst/bulk reads in order to acquire data faster. The
> > approach is different from the one used in oneshot captures.
> > 
> > BMP085 & BMP1xx devices use a completely different measurement
> > process that is well defined and is used in their buffer_handler().
> > 
> > Suggested-by: Angel Iglesias <ang.iglesiasg@...il.com>
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@...il.com>
> > Link: https://lore.kernel.org/r/20240512230524.53990-6-vassilisamir@gmail.com
> > ---
> The sign extend in buffered path doesn't make much sense as we should be
> advertising the correct bit depth for the channel and making that a userspace
> problem.
> 
> I'd failed to notice you are doing endian conversions just to check
> the skipped values. Ideally we'd leave the channels little endian
> and include that in the channel spec.
> 
> Hmm. I guess this works and if we have to do the endian conversion
> anyway isn't too bad.  It does provide slightly wrong information
> to userspace though.
> 
> So even with this in place I think these channels should be real_bits 24.
> 

Well, I totally get your point. Actually, I think that it makes much more
sense, to check the skipped values in userspace. These are information that
come from the datasheet, and I think that if it is important to someone to
check those values, they can do it. The point is to get the data to
userspace as soon as possible and then it is on the hands of user to do
what they want with that. So I agree that the implementation can be
simplified a lot.

As for the real_bits 24, I kind of get what you mean. I will fix this as
well. I will wait for Adam's comment on the first patch as well, and
then I will send a v9.

> 
> 
> > +static irqreturn_t bmp580_buffer_handler(int irq, void *p)
> > +{
> > +	struct iio_poll_func *pf = p;
> > +	struct iio_dev *indio_dev = pf->indio_dev;
> > +	struct bmp280_data *data = iio_priv(indio_dev);
> > +	s32 adc_temp, adc_press;
> > +	int ret;
> > +
> > +	guard(mutex)(&data->lock);
> > +
> > +	/* Burst read data registers */
> > +	ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB,
> > +			       data->buf, BMP280_BURST_READ_BYTES);
> > +	if (ret) {
> > +		dev_err(data->dev, "failed to burst read sensor data\n");
> > +		goto out;
> > +	}
> > +
> > +	/* Temperature calculations */
> > +	adc_temp = get_unaligned_le24(&data->buf[0]);
> > +	if (adc_temp == BMP580_TEMP_SKIPPED) {
> > +		dev_err(data->dev, "reading temperature skipped\n");
> > +		goto out;
> > +	}
> > +
> > +	data->sensor_data[1] = sign_extend32(adc_temp, 23);
> 
> the channel type should indicate that it's a 24 bit value. Not our
> problem to sign extend.  Leave that to userspace.
> 

Ok, I understand.


> > +
> > +	/* Pressure calculations */
> > +	adc_press = get_unaligned_le24(&data->buf[3]);
> > +	if (adc_press == BMP380_PRESS_SKIPPED) {
> > +		dev_err(data->dev, "reading pressure skipped\n");
> > +		goto out;
> > +	}
> > +
> > +	data->sensor_data[0] = adc_press;
> > +
> > +	iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data,
> > +					   iio_get_time_ns(indio_dev));
> > +
> > +out:
> > +	iio_trigger_notify_done(indio_dev->trig);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  static const int bmp580_oversampling_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128 };
> >  static const u8 bmp580_chip_ids[] = { BMP580_CHIP_ID, BMP580_CHIP_ID_ALT };
> >  static const int bmp580_temp_coeffs[] = { 1000, 16 };
> > @@ -1929,6 +2204,7 @@ const struct bmp280_chip_info bmp580_chip_info = {
> >  	.start_up_time = 2000,
> >  	.channels = bmp380_channels,
> >  	.num_channels = ARRAY_SIZE(bmp380_channels),
> > +	.avail_scan_masks = bmp280_avail_scan_masks,
> >  
> >  	.oversampling_temp_avail = bmp580_oversampling_avail,
> >  	.num_oversampling_temp_avail = ARRAY_SIZE(bmp580_oversampling_avail),
> > @@ -1955,6 +2231,8 @@ const struct bmp280_chip_info bmp580_chip_info = {
> >  	.read_temp = bmp580_read_temp,
> >  	.read_press = bmp580_read_press,
> >  	.preinit = bmp580_preinit,
> > +
> > +	.buffer_handler = bmp580_buffer_handler,
> >  };
> >  EXPORT_SYMBOL_NS(bmp580_chip_info, IIO_BMP280);
> >  
> > @@ -2133,7 +2411,7 @@ static int bmp180_read_press_adc(struct bmp280_data *data, u32 *adc_press)
> >  		return ret;
> >  
> >  	ret = regmap_bulk_read(data->regmap, BMP180_REG_OUT_MSB,
> > -			       data->buf, sizeof(data->buf));
> > +			       data->buf, BMP280_NUM_PRESS_BYTES);
> >  	if (ret) {
> >  		dev_err(data->dev, "failed to read pressure\n");
> >  		return ret;
> > @@ -2204,6 +2482,36 @@ static int bmp180_chip_config(struct bmp280_data *data)
> >  	return 0;
> >  }
> >  
> > +static irqreturn_t bmp180_buffer_handler(int irq, void *p)
> > +{
> > +	struct iio_poll_func *pf = p;
> > +	struct iio_dev *indio_dev = pf->indio_dev;
> > +	struct bmp280_data *data = iio_priv(indio_dev);
> > +	int ret, chan_value;
> > +
> > +	guard(mutex)(&data->lock);
> > +
> > +	ret = bmp180_read_temp(data, &chan_value);
> > +	if (ret)
> > +		goto out;
> > +
> > +	data->sensor_data[1] = chan_value;
> > +
> > +	ret = bmp180_read_press(data, &chan_value);
> > +	if (ret)
> > +		goto out;
> > +
> > +	data->sensor_data[0] = chan_value;
> > +
> > +	iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data,
> > +					   iio_get_time_ns(indio_dev));
> > +
> > +out:
> > +	iio_trigger_notify_done(indio_dev->trig);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> >  static const int bmp180_oversampling_temp_avail[] = { 1 };
> >  static const int bmp180_oversampling_press_avail[] = { 1, 2, 4, 8 };
> >  static const u8 bmp180_chip_ids[] = { BMP180_CHIP_ID };
> > @@ -2218,6 +2526,7 @@ const struct bmp280_chip_info bmp180_chip_info = {
> >  	.start_up_time = 2000,
> >  	.channels = bmp280_channels,
> >  	.num_channels = ARRAY_SIZE(bmp280_channels),
> > +	.avail_scan_masks = bmp280_avail_scan_masks,
> >  
> >  	.oversampling_temp_avail = bmp180_oversampling_temp_avail,
> >  	.num_oversampling_temp_avail =
> > @@ -2238,6 +2547,8 @@ const struct bmp280_chip_info bmp180_chip_info = {
> >  	.read_temp = bmp180_read_temp,
> >  	.read_press = bmp180_read_press,
> >  	.read_calib = bmp180_read_calib,
> > +
> > +	.buffer_handler = bmp180_buffer_handler,
> >  };
> >  EXPORT_SYMBOL_NS(bmp180_chip_info, IIO_BMP280);
> >  
> > @@ -2283,6 +2594,30 @@ static int bmp085_fetch_eoc_irq(struct device *dev,
> >  	return 0;
> >  }
> >  
> > +static int bmp280_buffer_preenable(struct iio_dev *indio_dev)
> > +{
> > +	struct bmp280_data *data = iio_priv(indio_dev);
> > +
> > +	pm_runtime_get_sync(data->dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static int bmp280_buffer_postdisable(struct iio_dev *indio_dev)
> > +{
> > +	struct bmp280_data *data = iio_priv(indio_dev);
> > +
> > +	pm_runtime_mark_last_busy(data->dev);
> > +	pm_runtime_put_autosuspend(data->dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct iio_buffer_setup_ops bmp280_buffer_setup_ops = {
> > +	.preenable = bmp280_buffer_preenable,
> > +	.postdisable = bmp280_buffer_postdisable,
> > +};
> > +
> >  static void bmp280_pm_disable(void *data)
> >  {
> >  	struct device *dev = data;
> > @@ -2329,6 +2664,7 @@ int bmp280_common_probe(struct device *dev,
> >  	/* Apply initial values from chip info structure */
> >  	indio_dev->channels = chip_info->channels;
> >  	indio_dev->num_channels = chip_info->num_channels;
> > +	indio_dev->available_scan_masks = chip_info->avail_scan_masks;
> >  	data->oversampling_press = chip_info->oversampling_press_default;
> >  	data->oversampling_humid = chip_info->oversampling_humid_default;
> >  	data->oversampling_temp = chip_info->oversampling_temp_default;
> > @@ -2414,6 +2750,14 @@ int bmp280_common_probe(struct device *dev,
> >  					     "failed to read calibration coefficients\n");
> >  	}
> >  
> > +	ret = devm_iio_triggered_buffer_setup(data->dev, indio_dev,
> > +					      iio_pollfunc_store_time,
> > +					      data->chip_info->buffer_handler,
> > +					      &bmp280_buffer_setup_ops);
> > +	if (ret)
> > +		return dev_err_probe(data->dev, ret,
> > +				     "iio triggered buffer setup failed\n");
> > +
> >  	/*
> >  	 * Attempt to grab an optional EOC IRQ - only the BMP085 has this
> >  	 * however as it happens, the BMP085 shares the chip ID of BMP180
> > diff --git a/drivers/iio/pressure/bmp280-spi.c b/drivers/iio/pressure/bmp280-spi.c
> > index 62b4e58104cf..e5abee15950e 100644
> > --- a/drivers/iio/pressure/bmp280-spi.c
> > +++ b/drivers/iio/pressure/bmp280-spi.c
> > @@ -40,14 +40,10 @@ static int bmp380_regmap_spi_read(void *context, const void *reg,
> >  				  size_t reg_size, void *val, size_t val_size)
> >  {
> >  	struct spi_device *spi = to_spi_device(context);
> > -	u8 rx_buf[4];
> > +	u8 rx_buf[BME280_BURST_READ_BYTES + 1];
> >  	ssize_t status;
> >  
> > -	/*
> > -	 * Maximum number of consecutive bytes read for a temperature or
> > -	 * pressure measurement is 3.
> > -	 */
> > -	if (val_size > 3)
> > +	if (val_size > BME280_BURST_READ_BYTES)
> >  		return -EINVAL;
> >  
> >  	/*
> > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> > index a3d2cd722760..756c644354c2 100644
> > --- a/drivers/iio/pressure/bmp280.h
> > +++ b/drivers/iio/pressure/bmp280.h
> > @@ -304,6 +304,16 @@
> >  #define BMP280_PRESS_SKIPPED		0x80000
> >  #define BMP280_HUMIDITY_SKIPPED		0x8000
> >  
> > +/* Number of bytes for each value */
> > +#define BMP280_NUM_PRESS_BYTES		3
> > +#define BMP280_NUM_TEMP_BYTES		3
> > +#define BME280_NUM_HUMIDITY_BYTES	2
> > +#define BMP280_BURST_READ_BYTES		(BMP280_NUM_PRESS_BYTES + \
> > +					 BMP280_NUM_TEMP_BYTES)
> > +#define BME280_BURST_READ_BYTES		(BMP280_NUM_PRESS_BYTES + \
> > +					 BMP280_NUM_TEMP_BYTES + \
> > +					 BME280_NUM_HUMIDITY_BYTES)
> > +
> >  /* Core exported structs */
> >  
> >  static const char *const bmp280_supply_names[] = {
> > @@ -397,13 +407,19 @@ struct bmp280_data {
> >  	 */
> >  	int sampling_freq;
> >  
> > +	/*
> > +	 * Data to push to userspace triggered buffer. Up to 3 channels and
> > +	 * s64 timestamp, aligned.
> > +	 */
> > +	s32 sensor_data[6] __aligned(8);
> > +
> >  	/*
> >  	 * DMA (thus cache coherency maintenance) may require the
> >  	 * transfer buffers to live in their own cache lines.
> >  	 */
> >  	union {
> >  		/* Sensor data buffer */
> > -		u8 buf[3];
> > +		u8 buf[BME280_BURST_READ_BYTES];
> >  		/* Calibration data buffers */
> >  		__le16 bmp280_cal_buf[BMP280_CONTIGUOUS_CALIB_REGS / 2];
> >  		__be16 bmp180_cal_buf[BMP180_REG_CALIB_COUNT / 2];
> > @@ -425,6 +441,7 @@ struct bmp280_chip_info {
> >  	const struct iio_chan_spec *channels;
> >  	int num_channels;
> >  	unsigned int start_up_time;
> > +	const unsigned long *avail_scan_masks;
> >  
> >  	const int *oversampling_temp_avail;
> >  	int num_oversampling_temp_avail;
> > @@ -459,6 +476,8 @@ struct bmp280_chip_info {
> >  	int (*read_humid)(struct bmp280_data *data, u32 *adc_humidity);
> >  	int (*read_calib)(struct bmp280_data *data);
> >  	int (*preinit)(struct bmp280_data *data);
> > +
> > +	irqreturn_t (*buffer_handler)(int irq, void *p);
> >  };
> >  
> >  /* Chip infos for each variant */
> 

Powered by blists - more mailing lists