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

[Vasileios Amoiridis]

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
> 
>