Re: [PATCH v1 12/13] iio: chemical: bme680: Add triggered buffer support

[Jonathan Cameron]

On Sat, 12 Oct 2024 13:04:02 +0100
Jonathan Cameron <jic23@xxxxxxxxxx> wrote:

> On Fri, 11 Oct 2024 21:07:20 +0200
> Vasileios Aoiridis <vassilisamir@xxxxxxxxx> wrote:
> 
> > On Fri, Oct 11, 2024 at 01:37:56PM +0300, Andy Shevchenko wrote:  
> > > On Thu, Oct 10, 2024 at 11:00:29PM +0200, vamoirid wrote:    
> > > > From: Vasileios Amoiridis <vassilisamir@xxxxxxxxx>
> > > > 
> > > > Add triggered buffer and soft timestamp support. The available scan mask
> > > > enables all the channels of the sensor in order to follow the operation of
> > > > the sensor. The sensor basically starts to capture from all channels
> > > > as long as it enters into FORCED mode.    
> > > 
> > > ...
> > >     
> > > >  	struct regulator_bulk_data supplies[BME680_NUM_SUPPLIES];
> > > >  	int ambient_temp;
> > > >  
> > > > +	u8 buffer[ALIGN(sizeof(s32) * BME680_NUM_CHANNELS, sizeof(s64))
> > > > +		  + sizeof(s64)] __aligned(sizeof(s64));    
> > > 
> > > Can it be represented as a structure?
> > > We also have aligned_s64 for the timestamp.
> > >    
> > 
> > Hi Andy,
> > 
> > The same approach was used also for the bmp280 driver and since I was
> > working on the bmp280 as well, I did it here. You think the
> > representation as a struct would look better? Personally I like the
> > nature of this one because of the ALIGN() but I have no problem of using
> > a struct here.  
> 
> Depends if you can enable sufficiently few channels that the timestamp
> moves.  If that is the case, a structure is missleading as a representation
> of this buffer so I prefer the above fun as it doesn't give the wrong
> impression (by giving no impression at all of the data layout!)
Looks like you always write them all to the buffer. So structure indeed
appropriate here. (I should have read the code before commenting!)

J
> 
> Jonathan