Re: [PATCH] iio: bu27008: Add processed illuminance channel

[Matti Vaittinen]

On 10/10/23 12:40, Jonathan Cameron wrote:
> On Fri, 6 Oct 2023 08:01:15 +0300
> Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
>
> > On 10/5/23 18:14, Jonathan Cameron wrote:
> > > On Mon, 2 Oct 2023 15:33:41 +0300
> > > Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
> > >    
> > > > The RGB + IR data can be used to calculate illuminance value (Luxes).
> > > > Implement the equation obtained from the ROHM HW colleagues and add a
> > > > light data channel outputting illuminance values in (nano) Luxes.
> > > Units in the ABI doc for illuminance are Lux, not nanolux.
> > > I'm guessing that you actually provide it in Lux but via scale.
> > >
> > > Make that clearer in this description if so.
> >
> > Yep. Also, the "processed" is misleading as I implement a raw channel. I
> > did originally think I'll only implement the read_raw (as I thought
> > we'll need all RGBC + IR and end up doing two accesses - which wouldn't
> > be nice due to the doubled measurement time). I actually did that and
> > used INT_PLUS_NANO. While implementing this I noticed the 'clear' data
> > was not used - and thought I might as well support buffering when RGB+IR
> > are enabled. I needed the scale to get the buffered values to decent
> > format though - so I converted channel to raw one and added scale. The
> > commit title still contains the 'processed' which reflects the original
> > thinking. Thanks for pointing out the confusion.
> >
> > > > Both the read_raw and buffering values is supported, with the limitation
> > > > that buffering is only allowed when suitable scan-mask is used. (RGB+IR,
> > > > no clear).
> > > >
> > > > The equation has been developed by ROHM HW colleagues for open air sensor.
> > > > Adding any lens to the sensor is likely to impact to the used c1, c2, c3
> > > > coefficients. Also, The output values have only been tested on BU27008.
> > > >
> > > > According to the HW colleagues, the very same equation should work also
> > > > on BU27010.
> > > >
> > > > Calculate and output illuminance values from BU27008 and BU27010.
> > > >
> > > > Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
> > > >   
> > >
> > > A few comments inline, but in general looks fine to me.
> >
> > Thanks Jonathan. I had to give also the BU27008 sensor away for a while.
> > I guess I won't send the next version until I am able to do some very
> > basic testing even if the changes were minor. That's probably sometime
> > next week.
> >
> > > Jonathan
> > >    
> > > > ---

//snip

> > > >    
> > > > +static int bu27008_get_rgb_ir(struct bu27008_data *data, unsigned int *red,
> > > > +		    unsigned int *green, unsigned int *blue, unsigned int *ir)
> > > > +{
> > > > +	int ret, chan_sel, int_time, tmpret, valid;
> > > > +	__le16 chans[BU27008_NUM_HW_CHANS];
> > > > +
> > > > +	chan_sel = BU27008_BLUE2_IR3 << (ffs(data->cd->chan_sel_mask) - 1);
> > > > +
> > > > +	ret = regmap_update_bits(data->regmap, data->cd->chan_sel_reg,
> > > > +				 data->cd->chan_sel_mask, chan_sel);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = bu27008_meas_set(data, true);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = bu27008_get_int_time_us(data);
> > > > +	if (ret < 0)
> > > > +		int_time = BU27008_MEAS_TIME_MAX_MS;
> > > > +	else
> > > > +		int_time = ret / USEC_PER_MSEC;
> > > > +
> > > > +	msleep(int_time);
> > > > +
> > > > +	ret = regmap_read_poll_timeout(data->regmap, data->cd->valid_reg,
> > > > +				       valid, (valid & BU27008_MASK_VALID),
> > > > +				       BU27008_VALID_RESULT_WAIT_QUANTA_US,
> > > > +				       BU27008_MAX_VALID_RESULT_WAIT_US);
> > > > +	if (ret)
> > > > +		goto out;
> > > > +
> > > > +	ret = regmap_bulk_read(data->regmap, BU27008_REG_DATA0_LO, chans,
> > > > +			       sizeof(chans));
> > > > +	if (ret)
> > > > +		goto out;
> > > > +
> > > > +	*red = le16_to_cpu(chans[0]);
> > > > +	*green = le16_to_cpu(chans[1]);
> > > > +	*blue = le16_to_cpu(chans[2]);
> > > > +	*ir = le16_to_cpu(chans[3]);
> > >
> > > I'd be tempted to use an array + definitely pass them as u16 rather
> > > than unsigned int.
> >
> > I'm not really convinced the u16 is better here. We need the 32 bits
> > later for the calculations - and (afaics) using natural size int for
> > arguments shouldn't harm. We read the channel data to correct type array
> > so code should be pretty clear as to what we have in HW.
>
> ok.  I don't like lack of range clamping - so at the point of the caller
> I can't immediately see that these will be sub 16bit value.  Not htat
> important I guess.
>
> > Also, I think that having an array obfuscates what element is which
> > channel because these ICs didn't have the 1 to 1 mapping from channel
> > index to colour. I was thinking of adding a struct for this but decided
> > to just keep it simple and clear.
> A struct or array + enum would work.
> I just don't much like lots of very similar parameters.

Right. A struct is not a problem. I am less fond with an enum because 
the HW channel which carries a specific color may change. I think adding 
an enum which indicates a place where a color is in array may be 
misleading one to think that HW has a fixed channel (data register 
address) for a colour. (I think that is quite usual while ICs where one 
color may be in different channels depending on the config are likely to 
be rare... I haven't read too many light sensor specs/drivers to know 
for sure though).

> > > > +
> > > > +out:
> > > > +	tmpret = bu27008_meas_set(data, false);
> > > > +	if (tmpret)
> > > > +		dev_warn(data->dev, "Stopping measurement failed\n");
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Following equation for computing lux out of register values was given by
> > > > + * ROHM HW colleagues;
> > > > + *
> > > > + * Red = RedData*1024 / Gain * 20 / meas_mode
> > > > + * Green = GreenData* 1024 / Gain * 20 / meas_mode
> > > > + * Blue = BlueData* 1024 / Gain * 20 / meas_mode
> > > > + * IR = IrData* 1024 / Gain * 20 / meas_mode
> > > > + *
> > > > + * where meas_mode is the integration time in mS / 10
> > > > + *
> > > > + * IRratio = (IR > 0.18 * Green) ? 0 : 1
> > > > + *
> > > > + * Lx = max(c1*Red + c2*Green + c3*Blue,0)
> > > > + *
> > > > + * for
> > > > + * IRratio 0: c1 = -0.00002237, c2 = 0.0003219, c3 = -0.000120371
> > > > + * IRratio 1: c1 = -0.00001074, c2 = 0.000305415, c3 = -0.000129367
> > > > + */
> > > > +
> > > > +/*
> > > > + * The max chan data is 0xffff. When we multiply it by 1024 * 20, we'll get
> > > > + * 0x4FFFB000 which still fits in 32-bit integer. So this can't overflow.
> > > > + */
> > > > +#define NORM_CHAN_DATA_FOR_LX_CALC(chan, gain, time) ((chan) * 1024 * 20 / \
> > > > +				   (gain) / (time))
> > > > +static u64 bu27008_calc_nlux(struct bu27008_data *data, unsigned int red,
> > > > +		unsigned int green, unsigned int blue,  unsigned int ir,
> > > > +		unsigned int gain, unsigned int gain_ir, unsigned int time)
> > > > +{
> > > > +	s64 c1, c2, c3, nlux;
> > > > +
> > > > +	time /= 10000;
> > > > +	ir = NORM_CHAN_DATA_FOR_LX_CALC(ir, gain_ir, time);
> > > > +	red = NORM_CHAN_DATA_FOR_LX_CALC(red, gain, time);
> > > > +	green = NORM_CHAN_DATA_FOR_LX_CALC(green, gain, time);
> > > > +	blue = NORM_CHAN_DATA_FOR_LX_CALC(blue, gain, time);
> >
> > > I'd prefer to see the inputs parameters and the normalized version given different
> > > names. Also the inputs are still u16, so nice to reflect that here.
> >
> > So, you suggest we bring the data as u16 until here and only here we
> > assign it into 32bit variables when doing the 'normalization'? I'm sure
> > it works, but I dislike doing computations like multiplying u16 by u32
> > as I never know (out of my head) how the implicit type conversions work
> > and if we get some results cropped. Adding the casts to computation make
> > it less pretty for my eyes while having all variables in large enough
> > types does not leave me wondering if it works correctly and if explicit
> > casts are needed.
> >
> > I am not strongly opposing this though if you insist - I am sure I can
> > at the end of the day get the code right - but I am afraid I will later
> > look at the code and wonder if it contains hideous issues...
>
> This isn't particularly important either way.  My gut would have
> been to keep them as __le16 to the point where the maths happens
> but I don't mind it happening elsewhere.
>
> I do want different names though given the inputs and outputs are
> different 'things'.

I'll try improving this and hopefully send a new version later this 
week. I should get the sensor at my hands latest at Thursday.

> > > Also when doing normalization I'd used fixed with types so there is no
> > > confusion over what was intended (here u32)
> >
> > Ok.
> >
> > >    
> > > > +
> > > > +	if ((u64)ir * 100LLU > 18LLU * (u64)green) {
> > >
> > > Putting scaling for ir to the right and green to the left is
> > > unusual. I'd chose one side and stick to it.
> >
> > Sorry Jonathan. I must be a bit slow today but I just seem to not be
> > able to think how you would like to have this? I think this line is
> > somehow mappable to the:
>
> if ((u64)ir * 100LLU > (u64)green * 18LLU)
> or
> if ((100LLU * (u64)ir > 18LLU * (u64)green)
>
> Either is fine.  Just don't like the scaling from different sides of
> the variable.  I can see how you got there from 0.18 * Green but equally
> valid to premultiply by 100 as it is to post multiply (when doing the
> maths on paper).

Now I see what you meant :) I misread your comment to meant that you 
didn't like the scaling on both sides of the '>'. /me feels slightly stupid.

Thanks again!

> > IRratio = (IR > 0.18 * Green) ? 0 : 1
> > formula I got from HW colleagues and added in the comment preceding the
> > function.

Yours,
	-- Matti

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

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