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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZsxWMSv5e5ZWOlai@smile.fi.intel.com>
Date: Mon, 26 Aug 2024 13:17:21 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Vasileios Amoiridis <vassilisamir@...il.com>
Cc: jic23@...nel.org, lars@...afoo.de, robh@...nel.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, ang.iglesiasg@...il.com,
	linus.walleij@...aro.org, biju.das.jz@...renesas.com,
	javier.carrasco.cruz@...il.com, semen.protsenko@...aro.org,
	579lpy@...il.com, ak@...klinger.de, linux-iio@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 4/7] iio: pressure: bmp280: Use sleep and forced mode
 for oneshot captures

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