linux-kernel - Re: [PATCH v5 3/3] iio: light: Add support for APDS9306 Light Sensor

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: <Za0TpiKjDD27Vh_x@smile.fi.intel.com>
Date: Sun, 21 Jan 2024 14:52:54 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc: subhajit.ghosh@...aklogic.com, anshulusr@...il.com, conor+dt@...nel.org,
	devicetree@...r.kernel.org, javier.carrasco.cruz@...il.com,
	jic23@...nel.org, krzysztof.kozlowski+dt@...aro.org,
	lars@...afoo.de, linux-iio@...r.kernel.org,
	linux-kernel@...r.kernel.org, marex@...x.de, matt@...ostay.sg,
	mazziesaccount@...il.com, robh+dt@...nel.org,
	stefan.windfeldt-prytz@...s.com
Subject: Re: [PATCH v5 3/3] iio: light: Add support for APDS9306 Light Sensor

On Sun, Jan 21, 2024 at 10:22:50AM +0100, Christophe JAILLET wrote:
> Le 21/01/2024 à 06:17, Subhajit Ghosh a écrit :

..

> > +#define APDS9306_ALS_THRES_VAL_MAX	(BIT(20) - 1)
> > +#define APDS9306_ALS_THRES_VAR_VAL_MAX	(BIT(3) - 1)
> > +#define APDS9306_ALS_PERSIST_VAL_MAX	(BIT(4) - 1)
> 
> Nit: GENMASK()?

I think no, these are plain numbers given in the form of BIT(x) - 1 to show the
HW limit on the values (in bits).

..

> > +	intg_time = iio_gts_find_int_time_by_sel(&data->gts, intg_time_idx);
> > +	if (intg_time < 0)
> > +		delay = apds9306_repeat_rate_period[repeat_rate_idx];
> 
> 'delay' is always overwritten by the line below.

Seems like entire conditional here is not needed as it's implied in the max().
And max() should take both signed or unsigned types, different types might have
a side effect.

> > +	/*
> > +	 * Whichever is greater - integration time period or
> > +	 * sampling period.
> > +	 */
> > +	delay = max(intg_time,
> > +		    apds9306_repeat_rate_period[repeat_rate_idx]);

-- 
With Best Regards,
Andy Shevchenko