Re: [PATCH 2/3] iio: pressure: Support ROHM BU1390

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 7 Sep 2023 08:57:17 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Morning Andy,
> 
> Thanks for the review.
> 
> On 9/6/23 18:48, Andy Shevchenko wrote:
> > On Wed, Sep 06, 2023 at 03:37:48PM +0300, Matti Vaittinen wrote:  
> >> Support for the ROHM BM1390 pressure sensor. The BM1390GLV-Z can measure
> >> pressures ranging from 300 hPa to 1300 hPa with configurable measurement
> >> averaging and internal FIFO. The sensor does also provide temperature
> >> measurements.
> >>
> >> Sensor does also contain IIR filter implemented in HW. The data-sheet
> >> says the IIR filter can be configured to be "weak", "middle" or
> >> "strong". Some RMS noise figures are provided in data sheet but no
> >> accurate maths for the filter configurations is provided. Hence, the IIR
> >> filter configuration is not supported by this driver and the filter is
> >> configured to the "middle" setting (at least not for now).
> >>
> >> The FIFO measurement mode is only measuring the pressure and not the
> >> temperature. The driver measures temperature when FIFO is flushed and
> >> simply uses the same measured temperature value to all reported
> >> temperatures. This should not be a problem when temperature is not
> >> changing very rapidly (several degrees C / second) but allows users to
> >> get the temperature measurements from sensor without any additional logic.  
> > 
> > ...
> > 
> >   
> >> +struct bm1390_data {
> >> +	int64_t timestamp, old_timestamp;  
> > 
> > Out of a sudden int64_t instead of u64?  
> 
> Judging the iio_push_to_buffers_with_timestamp() and iio_get_time_ns(), 
> IIO operates with signed timestamps. One being s64, the other int64_t.

That's odd. Ah well.  Should both be s64 as internal to the kernel only.


> 
> >> +	struct iio_trigger *trig;
> >> +	struct regmap *regmap;
> >> +	struct device *dev;
> >> +	struct bm1390_data_buf buf;
> >> +	int irq;
> >> +	unsigned int state;
> >> +	bool trigger_enabled;  
> >   
> >> +	u8 watermark;  
> > 
> > Or u8 instead of uint8_t?  
> 
> So, uint8_t is preferred? I don't really care all that much which of 
> these to use - which may even show up as a lack of consistency... I 
> think I did use uint8_t when I learned about it - but at some point 
> someone somewhere asked me to use u8 instead.. This somewhere might have 
> been u-boot though...
> 
> So, are you Suggesting I should replace u8 with uint8_t? Can do if it 
> matters.
u8 preferred for internal to kernel stuff, uint8_t if a userspace header.