Re: [PATCH v5 7/8] iio: light: ROHM BU27034 Ambient Light Sensor

["Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>]

On 3/26/23 19:19, Jonathan Cameron wrote:
> On Wed, 22 Mar 2023 11:07:56 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> ROHM BU27034 is an ambient light sensor with 3 channels and 3 photo diodes
>> capable of detecting a very wide range of illuminance. Typical application
>> is adjusting LCD and backlight power of TVs and mobile phones.
>>
>> Add initial  support for the ROHM BU27034 ambient light sensor.
>>
>> NOTE:
>> 	- Driver exposes 4 channels. One IIO_LIGHT channel providing the
>> 	  calculated lux values based on measured data from diodes #0 and
>> 	  #1. In addition, 3 IIO_INTENSITY channels are emitting the raw
>> 	  register data from all diodes for more intense user-space
>> 	  computations.
>> 	- Sensor has GAIN values that can be adjusted from 1x to 4096x.
>> 	- Sensor has adjustible measurement times of 5, 55, 100, 200 and
>> 	  400 mS. Driver does not support 5 mS which has special
>> 	  limitations.
>> 	- Driver exposes standard 'scale' adjustment which is
>> 	  implemented by:
>> 		1) Trying to adjust only the GAIN
>> 		2) If GAIN adjustment alone can't provide requested
>> 		   scale, adjusting both the time and the gain is
>> 		   attempted.
>> 	- Driver exposes writable INT_TIME property that can be used
>> 	  for adjusting the measurement time. Time adjustment will also
>> 	  cause the driver to try to adjust the GAIN so that the
>> 	  overall scale is kept as close to the original as possible.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>
> Hi Matti,
> 
> A few minor comments inline.  I'll take a closer look at the rest of the
> series when the discussions around the tests and devices to be used
> for them settle down.
> 
> Thanks,
> 
> Jonathan
> 
>> +
>> +static u64 bu27034_fixp_calc_t1(unsigned int coeff, unsigned int ch0,
>> +				unsigned int ch1, unsigned int gain0,
>> +				unsigned int gain1)
>> +{
>> +	unsigned int helper, tmp;
>> +	u64 helper64;
>> +
>> +	/*
>> +	 * Here we could overflow even the 64bit value. Hence we
>> +	 * multiply with gain0 only after the divisions - even though
>> +	 * it may result loss of accuracy
>> +	 */
>> +	helper64 = (u64)coeff * (u64)ch1 * (u64)ch1;
>> +	helper = coeff * ch1 * ch1;
>> +	tmp = helper * gain0;
>> +
>> +	if (helper == helper64 && (tmp / gain0 == helper))
> 
> Similar to below.  Don't bother with the non 64 bit version.
> 
>> +		return tmp / (gain1 * gain1) / ch0;
>> +
>> +	helper = gain1 * gain1;
>> +	if (helper > ch0) {
>> +		do_div(helper64, helper);
>> +
>> +		return gain_mul_div_helper(helper64, gain0, ch0);
>> +	}
>> +
>> +	do_div(helper64, ch0);
>> +
>> +	return gain_mul_div_helper(helper64, gain0, helper);
>> +}
>> +
>> +static u64 bu27034_fixp_calc_t23(unsigned int coeff, unsigned int ch,
>> +				 unsigned int gain)
>> +{
>> +	unsigned int helper;
>> +	u64 helper64;
>> +
>> +	helper64 = (u64)coeff * (u64)ch;
>> +	helper = coeff * ch;
>> +
>> +	if (helper == helper64)
>> +		return helper / gain;
>> +
>> +	do_div(helper64, gain);
>> +
>> +	return helper64;
> 
> I suspect that this is a premature bit of optimization so I'd just
> do it in 64 bits always.

Probably so.

> 
> Also, if you did want to do this, check_mul_overflow() etc would help.
> (linux/overflow.h)

Thanks! I'll check the overflow.h

The only reason why I did it like this is that I've been bitten badly by 
the do_div() in the past. It was actually quite expensive payment for a 
lesson learnt - my do_div() usage ended up in a customer devices in the 
field - and with a bit of bad luck we got a huge number to be divided 
with a small one... And the do_div() implementation for the architecture 
was a loop subtracting the divider.

The thing ended up halting the customer devices for many seconds, 
messing up lots of things. On top of that the project was huge - Amount 
of SW-developers was four figures. It took weeks until the bug report 
found it's way also to my desk - at which point I finally found the 
mistake I had made... And I didn't feel proud of it :|

And yes, we don't prevent the "divide big number by a tiny one" - issue 
by this check here. OTOH, I think I didn't see the loop-based do_div() 
implementation anymore either. It's just the habit of only using 
do_div() as a last resort. But yes, we probably can unconditionally use 
the do_div() here. I'll see what we have in the overflow.h though.

Thanks for the review again!

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~