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

From: Andy Shevchenko
Date: Mon Aug 26 2024 - 06:17:49 EST


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