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

[Andy Shevchenko]

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