Re: [PATCH v3 4/7] iio: pressure: bmp280: Use sleep and forced mode for oneshot captures

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Sat, Aug 24, 2024 at 01:29:24PM +0200, Vasileios Amoiridis wrote:
> On Fri, Aug 23, 2024 at 10:25:01PM +0300, Andy Shevchenko wrote:
> > On Fri, Aug 23, 2024 at 08:17:11PM +0200, Vasileios Amoiridis wrote:

...

> > > +	meas_time = 4000 + time_conv_temp[data->oversampling_temp] +
> > > +			   time_conv_press[data->oversampling_press];
> > 
> > 4 * USEC_PER_MSEC ?
> 
> Since the previous values in the arrays are all in thousands, why should
> I make this different?

When I read the code (and mind that we write code for humans), I don't
have a clue about the order of the values in use. Also it's hard to get from
the line the meaning of both sides of the formula. Using named definitions
helps a lot in understanding this line without reading and analysing code in
full.

...

> > > +	usleep_range(meas_time, meas_time * 12 / 10);
> > 
> > Comment? fsleep() ?
> 
> The usleep here is for waiting for the sensor to make the conversion,
> as the function name points out as well? Should I put it as a comment?
> 
> In general, is it considered good practice to add comments above all
> sleep functions?

Yes, it's even a requirement (not sure if it's documented anywhere) to comment
over long enough delays.

> I don't think it's a bad idea, I just didn't notice
> it somewhere.
> 
> > > +	return 0;
> > > +}

...

> > > +	usleep_range(2500, 3000);
> > 
> > fsleep() ?
> > 
> 
> ACK.

Also a comment, since it's milliseconds range which might be considered long
enough.

-- 
With Best Regards,
Andy Shevchenko