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
> >
> >> ---
> >>
> >> I did very dummy testing at very normal daylight inside a building. No
> >> special equipments were used - I simply compared values computed from
> >> BU27008 RGB+IR channels, to values displayed by the ALS in my mobile
> >> phone. Results were roughly the same (around 400 lux). Couldn't repeat
> >> test on BU27010, but the data it outputs should be same format as
> >> BU27008 data so equation should work for both sensors.
> >> ---
> >> drivers/iio/light/rohm-bu27008.c | 216 ++++++++++++++++++++++++++++++-
> >> 1 file changed, 211 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/iio/light/rohm-bu27008.c b/drivers/iio/light/rohm-bu27008.c
> >> index 6a6d77805091..d480cf761377 100644
> >> --- a/drivers/iio/light/rohm-bu27008.c
> >> +++ b/drivers/iio/light/rohm-bu27008.c
> >> @@ -130,6 +130,7 @@
> >> * @BU27008_BLUE: Blue channel. Via data2 (when used).
> >> * @BU27008_CLEAR: Clear channel. Via data2 or data3 (when used).
> >> * @BU27008_IR: IR channel. Via data3 (when used).
> >> + * @BU27008_LUX: Illuminance channel, computed using RGB and IR.
> >> * @BU27008_NUM_CHANS: Number of channel types.
> >> */
> >> enum bu27008_chan_type {
> >> @@ -138,6 +139,7 @@ enum bu27008_chan_type {
> >> BU27008_BLUE,
> >> BU27008_CLEAR,
> >> BU27008_IR,
> >> + BU27008_LUX,
> >> BU27008_NUM_CHANS
> >> };
> >>
> >> @@ -172,6 +174,8 @@ static const unsigned long bu27008_scan_masks[] = {
> >> ALWAYS_SCANNABLE | BIT(BU27008_CLEAR) | BIT(BU27008_IR),
> >> /* buffer is R, G, B, IR */
> >> ALWAYS_SCANNABLE | BIT(BU27008_BLUE) | BIT(BU27008_IR),
> >> + /* buffer is R, G, B, IR, LUX */
> >> + ALWAYS_SCANNABLE | BIT(BU27008_BLUE) | BIT(BU27008_IR) | BIT(BU27008_LUX),
> >> 0
> >> };
> >>
> >> @@ -331,6 +335,19 @@ static const struct iio_chan_spec bu27008_channels[] = {
> >> * Hence we don't advertise available ones either.
> >> */
> >> BU27008_CHAN(IR, DATA3, 0),
> >> + {
> >> + .type = IIO_LIGHT,
> >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> >> + BIT(IIO_CHAN_INFO_SCALE),
> >> + .channel = BU27008_LUX,
> >> + .scan_index = BU27008_LUX,
> >> + .scan_type = {
> >> + .sign = 'u',
> >> + .realbits = 64,
> >> + .storagebits = 64,
> >> + .endianness = IIO_CPU,
> >> + },
> >> + },
> >> IIO_CHAN_SOFT_TIMESTAMP(BU27008_NUM_CHANS),
> >> };
> >>
> >> @@ -1004,6 +1021,183 @@ static int bu27008_read_one(struct bu27008_data *data, struct iio_dev *idev,
> >> return ret;
> >> }
> >>
> >> +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.
>
> >> +
> >> +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'.
>
> > 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).
>
> IRratio = (IR > 0.18 * Green) ? 0 : 1
> formula I got from HW colleagues and added in the comment preceding the
> function.
>
