diff mbox series

[3/4] iio: pressure: Add timestamp and scan_masks for BMP280 driver

Message ID 20240303165300.468011-4-vassilisamir@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series Series to add triggered buffer support to BMP280 driver | expand

Commit Message

Vasileios Amoiridis March 3, 2024, 4:52 p.m. UTC 
The scan mask for the BME280 device which contains humidity
measurement needs to become different in order for the timestamp
to be able to work. Scan masks are added for different combinations
of measurements. The temperature measurement is needed for either
pressure or humidity measurements.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/pressure/bmp280-core.c | 113 +++++++++++++++++++++++++----
 drivers/iio/pressure/bmp280.h      |   1 +
 2 files changed, 100 insertions(+), 14 deletions(-)

Comments

Andy Shevchenko March 4, 2024, 11:47 a.m. UTC | #1 
On Sun, Mar 03, 2024 at 05:52:59PM +0100, Vasileios Amoiridis wrote:
> The scan mask for the BME280 device which contains humidity
> measurement needs to become different in order for the timestamp
> to be able to work. Scan masks are added for different combinations
> of measurements. The temperature measurement is needed for either
> pressure or humidity measurements.

...

> +enum bmp280_scan {
> +	BMP280_TEMP,
> +	BMP280_PRESS,
> +	BME280_HUMID,
> +};

Hmm... Why do we need to actually copy the IIO ones? Can't we use IIO ones
directly (or in some way)?

...

> +static const unsigned long bmp280_avail_scan_masks[] = {
> +	BIT(BMP280_TEMP),
> +	BIT(BMP280_PRESS) | BIT(BMP280_TEMP),
> +	0,

No comma for the terminator line.

> +};

> +static const unsigned long bme280_avail_scan_masks[] = {
> +	BIT(BMP280_TEMP),
> +	BIT(BMP280_PRESS) | BIT(BMP280_TEMP),
> +	BIT(BME280_HUMID) | BIT(BMP280_TEMP),
> +	BIT(BME280_HUMID) | BIT(BMP280_PRESS) | BIT(BMP280_TEMP),
> +	0,

Ditto.

> +};

...

>  	const struct iio_chan_spec *channels;
>  	int num_channels;
> +	const unsigned long *avail_scan_masks;
>  	unsigned int start_up_time;

Please, run `pahole` every time you are changing data structure layout.
Here you efficiently wasted 8 bytes of memory AFAICS.
Vasileios Amoiridis March 4, 2024, 6:50 p.m. UTC | #2 
On Mon, Mar 04, 2024 at 01:47:47PM +0200, Andy Shevchenko wrote:
> On Sun, Mar 03, 2024 at 05:52:59PM +0100, Vasileios Amoiridis wrote:
> > The scan mask for the BME280 device which contains humidity
> > measurement needs to become different in order for the timestamp
> > to be able to work. Scan masks are added for different combinations
> > of measurements. The temperature measurement is needed for either
> > pressure or humidity measurements.
> 
> ...
> 
> > +enum bmp280_scan {
> > +	BMP280_TEMP,
> > +	BMP280_PRESS,
> > +	BME280_HUMID,
> > +};
> 
> Hmm... Why do we need to actually copy the IIO ones? Can't we use IIO ones
> directly (or in some way)?
> 

What do you mean exactly by copying the IIO ones? These values are used as
indexes to enable channels in the avail_scan_masks below. 
> ...
> 
> > +static const unsigned long bmp280_avail_scan_masks[] = {
> > +	BIT(BMP280_TEMP),
> > +	BIT(BMP280_PRESS) | BIT(BMP280_TEMP),
> > +	0,
> 
> No comma for the terminator line.
> 
> > +};
> 
> > +static const unsigned long bme280_avail_scan_masks[] = {
> > +	BIT(BMP280_TEMP),
> > +	BIT(BMP280_PRESS) | BIT(BMP280_TEMP),
> > +	BIT(BME280_HUMID) | BIT(BMP280_TEMP),
> > +	BIT(BME280_HUMID) | BIT(BMP280_PRESS) | BIT(BMP280_TEMP),
> > +	0,
> 
> Ditto.
> 
> > +};
> 
> ...
> 
> >  	const struct iio_chan_spec *channels;
> >  	int num_channels;
> > +	const unsigned long *avail_scan_masks;
> >  	unsigned int start_up_time;
> 
> Please, run `pahole` every time you are changing data structure layout.
> Here you efficiently wasted 8 bytes of memory AFAICS.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>
Andy Shevchenko March 4, 2024, 7:07 p.m. UTC | #3 
On Mon, Mar 04, 2024 at 07:50:17PM +0100, Vasileios Amoiridis wrote:
> On Mon, Mar 04, 2024 at 01:47:47PM +0200, Andy Shevchenko wrote:
> > On Sun, Mar 03, 2024 at 05:52:59PM +0100, Vasileios Amoiridis wrote:

...

> > > +enum bmp280_scan {
> > > +	BMP280_TEMP,
> > > +	BMP280_PRESS,
> > > +	BME280_HUMID,
> > > +};
> > 
> > Hmm... Why do we need to actually copy the IIO ones? Can't we use IIO ones
> > directly (or in some way)?
> 
> What do you mean exactly by copying the IIO ones? These values are used as
> indexes to enable channels in the avail_scan_masks below.

Yeah, I have now an answer to my question. The IIO drivers provide these lists
as mapping between available channels (as starting from 0) and real channels
as per IIO specifications (which can be anything, although limited currently
by 40 or so).
Jonathan Cameron March 9, 2024, 6:12 p.m. UTC | #4 
On Mon, 4 Mar 2024 21:07:09 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Mon, Mar 04, 2024 at 07:50:17PM +0100, Vasileios Amoiridis wrote:
> > On Mon, Mar 04, 2024 at 01:47:47PM +0200, Andy Shevchenko wrote:  
> > > On Sun, Mar 03, 2024 at 05:52:59PM +0100, Vasileios Amoiridis wrote:  
> 
> ...
> 
> > > > +enum bmp280_scan {
> > > > +	BMP280_TEMP,
> > > > +	BMP280_PRESS,
> > > > +	BME280_HUMID,
> > > > +};  
> > > 
> > > Hmm... Why do we need to actually copy the IIO ones? Can't we use IIO ones
> > > directly (or in some way)?  
> > 
> > What do you mean exactly by copying the IIO ones? These values are used as
> > indexes to enable channels in the avail_scan_masks below.  
> 
> Yeah, I have now an answer to my question. The IIO drivers provide these lists
> as mapping between available channels (as starting from 0) and real channels
> as per IIO specifications (which can be anything, although limited currently
> by 40 or so).
> 

These are the scan indexes. It would be better to specify them as such
in the channel definitions so that the two sets of values are forced to match.

Jonathan
diff mbox series

Patch

diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index acdf6138d317..3bdf6002983f 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -134,36 +134,86 @@  enum {
 	BMP380_P11 = 20,
 };
 
+enum bmp280_scan {
+	BMP280_TEMP,
+	BMP280_PRESS,
+	BME280_HUMID,
+};
+
 static const struct iio_chan_spec bmp280_channels[] = {
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
+				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 32,
+			.storagebits = 32,
+			.endianness = IIO_CPU,
+		},
+	},
 	{
 		.type = IIO_PRESSURE,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
 				      BIT(IIO_CHAN_INFO_SCALE) |
 				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.scan_index = 1,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 32,
+			.storagebits = 32,
+			.endianness = IIO_CPU,
+		},
 	},
+	IIO_CHAN_SOFT_TIMESTAMP(2),
+};
+
+static const struct iio_chan_spec bme280_channels[] = {
 	{
 		.type = IIO_TEMP,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
 				      BIT(IIO_CHAN_INFO_SCALE) |
 				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 32,
+			.storagebits = 32,
+			.endianness = IIO_CPU,
+		},
 	},
 	{
-		.type = IIO_HUMIDITYRELATIVE,
+		.type = IIO_PRESSURE,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
 				      BIT(IIO_CHAN_INFO_SCALE) |
 				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
+		.scan_index = 1,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 32,
+			.storagebits = 32,
+			.endianness = IIO_CPU,
+		},
 	},
-};
-
-static const struct iio_chan_spec bmp380_channels[] = {
 	{
-		.type = IIO_PRESSURE,
+		.type = IIO_HUMIDITYRELATIVE,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
 				      BIT(IIO_CHAN_INFO_SCALE) |
 				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
-		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
-					   BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
+		.scan_index = 2,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 32,
+			.storagebits = 32,
+			.endianness = IIO_CPU,
+		},
 	},
+	IIO_CHAN_SOFT_TIMESTAMP(3),
+};
+
+static const struct iio_chan_spec bmp380_channels[] = {
 	{
 		.type = IIO_TEMP,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
@@ -171,15 +221,30 @@  static const struct iio_chan_spec bmp380_channels[] = {
 				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
 					   BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 's',
+			.realbits = 32,
+			.storagebits = 32,
+			.endianness = IIO_CPU,
+		},
 	},
 	{
-		.type = IIO_HUMIDITYRELATIVE,
+		.type = IIO_PRESSURE,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
 				      BIT(IIO_CHAN_INFO_SCALE) |
 				      BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
 					   BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY),
+		.scan_index = 1,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 32,
+			.storagebits = 32,
+			.endianness = IIO_CPU,
+		},
 	},
+	IIO_CHAN_SOFT_TIMESTAMP(2),
 };
 
 static int bmp280_read_calib(struct bmp280_data *data)
@@ -835,6 +900,20 @@  static const struct iio_info bmp280_info = {
 	.write_raw = &bmp280_write_raw,
 };
 
+static const unsigned long bmp280_avail_scan_masks[] = {
+	BIT(BMP280_TEMP),
+	BIT(BMP280_PRESS) | BIT(BMP280_TEMP),
+	0,
+};
+
+static const unsigned long bme280_avail_scan_masks[] = {
+	BIT(BMP280_TEMP),
+	BIT(BMP280_PRESS) | BIT(BMP280_TEMP),
+	BIT(BME280_HUMID) | BIT(BMP280_TEMP),
+	BIT(BME280_HUMID) | BIT(BMP280_PRESS) | BIT(BMP280_TEMP),
+	0,
+};
+
 static int bmp280_chip_config(struct bmp280_data *data)
 {
 	u8 osrs = FIELD_PREP(BMP280_OSRS_TEMP_MASK, data->oversampling_temp + 1) |
@@ -874,7 +953,8 @@  const struct bmp280_chip_info bmp280_chip_info = {
 	.regmap_config = &bmp280_regmap_config,
 	.start_up_time = 2000,
 	.channels = bmp280_channels,
-	.num_channels = 2,
+	.num_channels = 3,
+	.avail_scan_masks = bmp280_avail_scan_masks,
 
 	.oversampling_temp_avail = bmp280_oversampling_avail,
 	.num_oversampling_temp_avail = ARRAY_SIZE(bmp280_oversampling_avail),
@@ -927,8 +1007,9 @@  const struct bmp280_chip_info bme280_chip_info = {
 	.num_chip_id = ARRAY_SIZE(bme280_chip_ids),
 	.regmap_config = &bmp280_regmap_config,
 	.start_up_time = 2000,
-	.channels = bmp280_channels,
-	.num_channels = 3,
+	.channels = bme280_channels,
+	.num_channels = 4,
+	.avail_scan_masks = bme280_avail_scan_masks,
 
 	.oversampling_temp_avail = bmp280_oversampling_avail,
 	.num_oversampling_temp_avail = ARRAY_SIZE(bmp280_oversampling_avail),
@@ -1305,7 +1386,8 @@  const struct bmp280_chip_info bmp380_chip_info = {
 	.regmap_config = &bmp380_regmap_config,
 	.start_up_time = 2000,
 	.channels = bmp380_channels,
-	.num_channels = 2,
+	.num_channels = 3,
+	.avail_scan_masks = bmp280_avail_scan_masks,
 
 	.oversampling_temp_avail = bmp380_oversampling_avail,
 	.num_oversampling_temp_avail = ARRAY_SIZE(bmp380_oversampling_avail),
@@ -1807,7 +1889,8 @@  const struct bmp280_chip_info bmp580_chip_info = {
 	.regmap_config = &bmp580_regmap_config,
 	.start_up_time = 2000,
 	.channels = bmp380_channels,
-	.num_channels = 2,
+	.num_channels = 3,
+	.avail_scan_masks = bmp280_avail_scan_masks,
 
 	.oversampling_temp_avail = bmp580_oversampling_avail,
 	.num_oversampling_temp_avail = ARRAY_SIZE(bmp580_oversampling_avail),
@@ -2072,7 +2155,8 @@  const struct bmp280_chip_info bmp180_chip_info = {
 	.regmap_config = &bmp180_regmap_config,
 	.start_up_time = 2000,
 	.channels = bmp280_channels,
-	.num_channels = 2,
+	.num_channels = 3,
+	.avail_scan_masks = bmp280_avail_scan_masks,
 
 	.oversampling_temp_avail = bmp180_oversampling_temp_avail,
 	.num_oversampling_temp_avail =
@@ -2179,6 +2263,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;
diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
index 4012387d7956..d77402cb3121 100644
--- a/drivers/iio/pressure/bmp280.h
+++ b/drivers/iio/pressure/bmp280.h
@@ -426,6 +426,7 @@  struct bmp280_chip_info {
 
 	const struct iio_chan_spec *channels;
 	int num_channels;
+	const unsigned long *avail_scan_masks;
 	unsigned int start_up_time;
 
 	const int *oversampling_temp_avail;