Received-SPF: pass (google.com: domain of linux-kernel+bounces-91147-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Vasileios Amoiridis <vassilisamir@gmail.com>
Date: Mon, 4 Mar 2024 20:08:38 +0100
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Vasileios Amoiridis <vassilisamir@gmail.com>, jic23@kernel.org,
	lars@metafoo.de, ang.iglesiasg@gmail.com, mazziesaccount@gmail.com,
	ak@it-klinger.de, petre.rodan@subdimension.ro, phil@raspberrypi.com,
	579lpy@gmail.com, linux-iio@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 4/4] iio: pressure: Add triggered buffer support for
 BMP280 driver
Message-ID: <20240304190838.GA4431@vamoiridPC>
References: <20240303165300.468011-1-vassilisamir@gmail.com>
 <20240303165300.468011-5-vassilisamir@gmail.com>
 <ZeW15Q2juyQuTw5q@smile.fi.intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZeW15Q2juyQuTw5q@smile.fi.intel.com>

On Mon, Mar 04, 2024 at 01:52:05PM +0200, Andy Shevchenko wrote:
> On Sun, Mar 03, 2024 at 05:53:00PM +0100, Vasileios Amoiridis wrote:
> > Add a buffer struct that will hold the values of the measurements
> > and will be pushed to userspace. Modify all read_* functions in order
> > to just read and compensate the data without though converting to the
> > required IIO measurement units which are used for the oneshot captures.
> 
> > +#include <linux/iio/buffer.h>
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> > +#include <linux/iio/trigger.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> 
> Yes, taking into account the comment against patch 1, this will become the part
> of iio/* group.
> 
> ...
> 
> > +	/* val might be NULL if we're called by the buffer handler */
> > +	if (val) {
> > +		*val = comp_press;
> > +		/* Compensated pressure is in cPa (centipascals) */
> > +		*val2 = 100000;
> 
> Here and everywhere else where it makes sense
> 
> 		*val2 = CENTI * 1000; // (What is 1000 here?)
> 
> from units.h?
> 

I didn't change these values, the addition here is that I put them under an
if statement that checks if we were called by the buffer handler or by the
oneshot capture read_raw function. The point is that every sensor provides
values that need different scaling in order to have the IIO standard
measurement units. In the above code I guess since 1kPa=100000cPa that's
why *val2=100000.

The *val and *val2 values could be moved to the read_raw function as it will
already happen for the IIO_CHAN_INFO_SCALE values from chip_info arrays as
you proposed in Patch No.2. This would require though that all the functions
like this one you commented would need to change. Is that something that you
think as better?

> > +		return IIO_VAL_FRACTIONAL;
> > +	}
> 
> ...
> 
> > +	struct {
> > +		s32 temperature;
> > +		u32 pressure;
> > +		u32 humidity;
> 
> > +		s64 timestamp;
> 
> Shouldn't this be aligned properly?
> 

I saw that in some drivers it was added and in some it was not. What is the
difference of aligning just the timestamp of the kernel?
> > +	} iio_buffer;
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>