Date: Sat, 22 Jun 2024 16:09:11 +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.
> 
> 
> 
> > +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);

With the bulk read here, we have 24 bits of temperature and 24 bits 
of pressure, so in total 6 bytes. The only way I can see to not do
the endian conversion is that I memcpy the first 3 bytes to the
data->sensor_data[1], and the last 3 bytes to the data->sensor_data[0].

If you can see any other better way please let me know, otherwise the
other solution is to use get_unaligned_24(). Still, we won't do the
skipping part.
 
> > +	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.
> 
> > +
> > +	/* 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