Received-SPF: pass (google.com: domain of linux-kernel+bounces-32059-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Sun, 21 Jan 2024 14:52:54 +0200
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc: subhajit.ghosh@tweaklogic.com, anshulusr@gmail.com, conor+dt@kernel.org,
	devicetree@vger.kernel.org, javier.carrasco.cruz@gmail.com,
	jic23@kernel.org, krzysztof.kozlowski+dt@linaro.org,
	lars@metafoo.de, linux-iio@vger.kernel.org,
	linux-kernel@vger.kernel.org, marex@denx.de, matt@ranostay.sg,
	mazziesaccount@gmail.com, robh+dt@kernel.org,
	stefan.windfeldt-prytz@axis.com
Subject: Re: [PATCH v5 3/3] iio: light: Add support for APDS9306 Light Sensor
Message-ID: <Za0TpiKjDD27Vh_x@smile.fi.intel.com>
References: <20240121051735.32246-1-subhajit.ghosh@tweaklogic.com>
 <20240121051735.32246-4-subhajit.ghosh@tweaklogic.com>
 <8a7f03b6-caca-4fbb-8093-0ba87bd2e850@wanadoo.fr>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <8a7f03b6-caca-4fbb-8093-0ba87bd2e850@wanadoo.fr>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo

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