Received-SPF: pass (google.com: domain of linux-kernel+bounces-169840-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 6 May 2024 13:50:10 +0100
From: Jonathan Cameron <jic23@kernel.org>
To: Vasileios Amoiridis <vassilisamir@gmail.com>
Cc: lars@metafoo.de, andriy.shevchenko@linux.intel.com,
 ang.iglesiasg@gmail.com, mazziesaccount@gmail.com, ak@it-klinger.de,
 petre.rodan@subdimension.ro, phil@raspberrypi.com, 579lpy@gmail.com,
 linus.walleij@linaro.org, semen.protsenko@linaro.org,
 linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 10/10] iio: pressure: bmp280: Add triggered buffer
 support
Message-ID: <20240506135010.5f7de250@jic23-huawei>
In-Reply-To: <20240505235755.GC17986@vamoiridPC>
References: <20240429190046.24252-1-vassilisamir@gmail.com>
	<20240429190046.24252-11-vassilisamir@gmail.com>
	<20240505203456.0c4c0c90@jic23-huawei>
	<20240505235755.GC17986@vamoiridPC>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Mon, 6 May 2024 01:57:55 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> On Sun, May 05, 2024 at 08:34:56PM +0100, Jonathan Cameron wrote:
> > On Mon, 29 Apr 2024 21:00:46 +0200
> > Vasileios Amoiridis <vassilisamir@gmail.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@gmail.com>
> > > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>  
> > Hi Vasileois,
> > 
> > Just one question on this inline. (patches 8 and 9 look good to me)
> > 
> > For v6, only need to send the patches that I haven't already applied.
> > 
> > Thanks,
> > 
> > Jonathan
> >   
> > >  
> > > +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 < 0)
> > > +		return IRQ_HANDLED;
> > > +
> > > +	data->sensor_data[1] = chan_value;
> > > +
> > > +	ret = bmp180_read_press(data, &chan_value);  
> > 
> > So I 'think' that after all the refactoring you end up reading the temperature
> > twice.  To avoid that you need to pull the read_temp() and read_press()
> > function implementations here and only do the (currently duplicated) steps once.
> > 
> > You seem to have done this for the other case, but missed the bmp180?
> > Maybe I'm missing some reason it doesn't work for this one!
> >   
> 
> Hi Jonathan!
> 
> So, I didn't miss it. This is an old sensor and in order to get data out, the
> procedure is much more constrained. As you can see in the datasheet [1] in page
> 11 there is a well defined process on how to read the data out. It's not
> possible to make a burst read here. Hence, the strange bmp180_measure() function
> in order to wait for an EOC before reading the values. Indeed I am reading the
> temperature 2 times which is not optimal but in order to read both of them I
> would have to:
> 
> a) either get the temperature out of the bmp180_read_press() function
> (which would ruin a bit consistency with the other bmpxxx_read_press() functions)
> 
> b) make a bmp180_get_sensor_data() which would look like bmp180_get_press() but
> also gives temperature (which won't look that good).
> 
> That's why I didn't touch it. If you think it makes more sense to do it, I can
> follow one of the 2 approaches, whichever you think would make more sense.

Ok. As you say, old sensor so fine to not optimize it. If anyone else cares
they can do it ;)

Jonathan

> 
> Cheers,
> Vasilis
> 
> [1]: https://cdn-shop.adafruit.com/datasheets/BST-BMP180-DS000-09.pdf
> 
> > > +	if (ret < 0)
> > > +		return IRQ_HANDLED;
> > > +
> > > +	data->sensor_data[0] = chan_value;
> > > +
> > > +	iio_push_to_buffers_with_timestamp(indio_dev, &data->sensor_data,
> > > +					   iio_get_time_ns(indio_dev));
> > > +
> > > +	iio_trigger_notify_done(indio_dev->trig);
> > > +
> > > +	return IRQ_HANDLED;
> > > +}