| Message ID | ZRq4pdDn6N73n7BO@dc78bmyyyyyyyyyyyyydt-3.rev.dnainternet.fi (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: bu27008: Add processed illuminance channel | expand |
On Mon, 2 Oct 2023 15:33:41 +0300 Matti Vaittinen <mazziesaccount@gmail.com> 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. > > 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@gmail.com> > A few comments inline, but in general looks fine to me. 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. > + > +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. Also when doing normalization I'd used fixed with types so there is no confusion over what was intended (here u32) > + > + 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. > + c1 = -22370; > + c2 = 321900; > + c3 = -120371; > + } else { > + c1 = -10740; > + c2 = 305415; > + c3 = -129367; > + } > + nlux = c1 * red + c2 * green + c3 * blue; > + if (nlux < 0) > + nlux = 0; return max(0, nlux); is a bit neater and makes it clear this is simple clamping to possible values given unlikely we'll see negative light sources :) > + > + return nlux; > +} > + > +static int bu27008_get_time_n_gains(struct bu27008_data *data, > + unsigned int *gain, unsigned int *gain_ir, unsigned int *time) > +{ > + int ret; > + > + ret = bu27008_get_gain(data, &data->gts, gain); > + if (ret < 0) > + return ret; > + > + ret = bu27008_get_gain(data, &data->gts_ir, gain_ir); > + if (ret < 0) > + return ret; > + > + ret = bu27008_get_int_time_us(data); > + if (ret < 0) > + return ret; > + > + /* Max integration time is 400000i. Fits in signed int. */ > + *time = ret; > + > + return 0; > +} > + > +struct bu27008_buf { > + __le16 chan[BU27008_NUM_HW_CHANS]; > + u64 lux __aligned(8); > + s64 ts __aligned(8); > +}; > + > +static int bu27008_buffer_get_lux(struct bu27008_data *data, > + struct bu27008_buf *raw) > +{ > + unsigned int red, green, blue, ir, gain, gain_ir, time; > + int ret; > + > + red = le16_to_cpu(raw->chan[0]); > + green = le16_to_cpu(raw->chan[1]); > + blue = le16_to_cpu(raw->chan[2]); > + ir = le16_to_cpu(raw->chan[3]); > + ret = bu27008_get_time_n_gains(data, &gain, &gain_ir, &time); > + if (ret) > + return ret; > + > + raw->lux = bu27008_calc_nlux(data, red, green, blue, ir, gain, gain_ir, > + time); Probably call this function *fill_in_lux() or something like that because I'd expect a *get_lux() function to return the lux value. > + > + return 0; > +} > + > +static int bu27008_read_lux(struct bu27008_data *data, struct iio_dev *idev, > + struct iio_chan_spec const *chan, > + int *val, int *val2) > +{ > + unsigned int red, green, blue, ir, gain, gain_ir, time; > + u64 nlux; > + int ret; > + > + ret = bu27008_get_rgb_ir(data, &red, &green, &blue, &ir); > + if (ret) > + return ret; > + > + ret = bu27008_get_time_n_gains(data, &gain, &gain_ir, &time); > + if (ret) > + return ret; > + > + nlux = bu27008_calc_nlux(data, red, green, blue, ir, gain, gain_ir, > + time); > + *val = (int)nlux; > + *val2 = nlux >> 32LLU; > + > + return IIO_VAL_INT_64; > +} > + > static int bu27008_read_raw(struct iio_dev *idev, > struct iio_chan_spec const *chan, > int *val, int *val2, long mask) > @@ -1012,13 +1206,17 @@ static int bu27008_read_raw(struct iio_dev *idev, > int busy, ret; > > switch (mask) { > + Unwanted change. > case IIO_CHAN_INFO_RAW: No IIO_CHAN_INFO_PROCESSED so I'm a bit surprised if this o > busy = iio_device_claim_direct_mode(idev); > if (busy) > return -EBUSY; > > mutex_lock(&data->mutex); > - ret = bu27008_read_one(data, idev, chan, val, val2); > + if (chan->type == IIO_LIGHT) > + ret = bu27008_read_lux(data, idev, chan, val, val2); > + else > + ret = bu27008_read_one(data, idev, chan, val, val2); > mutex_unlock(&data->mutex); > > iio_device_release_direct_mode(idev); > @@ -1026,6 +1224,11 @@ static int bu27008_read_raw(struct iio_dev *idev, > return ret; > > case IIO_CHAN_INFO_SCALE: > + if (chan->type == IIO_LIGHT) { > + *val = 0; > + *val2 = 1; > + return IIO_VAL_INT_PLUS_NANO; > + } > ret = bu27008_get_scale(data, chan->scan_index == BU27008_IR, > val, val2); > if (ret) > @@ -1231,15 +1434,13 @@ static const struct iio_trigger_ops bu27008_trigger_ops = { > .reenable = bu27008_trigger_reenable, > }; > > + Unrelated and probably unwanted change. > static irqreturn_t bu27008_trigger_handler(int irq, void *p) > { > struct iio_poll_func *pf = p; > struct iio_dev *idev = pf->indio_dev; > struct bu27008_data *data = iio_priv(idev); > - struct { > - __le16 chan[BU27008_NUM_HW_CHANS]; > - s64 ts __aligned(8); > - } raw; > + struct bu27008_buf raw; > int ret, dummy; > > memset(&raw, 0, sizeof(raw)); > @@ -1256,6 +1457,11 @@ static irqreturn_t bu27008_trigger_handler(int irq, void *p) > sizeof(raw.chan)); > if (ret < 0) > goto err_read; Blank line here appropriate given no block of unrelated code. > + if (test_bit(BU27008_LUX, idev->active_scan_mask)) { > + ret = bu27008_buffer_get_lux(data, &raw); > + if (ret) > + goto err_read; > + } > > iio_push_to_buffers_with_timestamp(idev, &raw, pf->timestamp); > err_read: > > base-commit: 5e99f692d4e32e3250ab18d511894ca797407aec
On 10/5/23 18:14, Jonathan Cameron wrote: > On Mon, 2 Oct 2023 15:33:41 +0300 > Matti Vaittinen <mazziesaccount@gmail.com> 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@gmail.com> >> > > 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. 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. >> + >> +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... > 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: IRratio = (IR > 0.18 * Green) ? 0 : 1 formula I got from HW colleagues and added in the comment preceding the function. > >> + c1 = -22370; >> + c2 = 321900; >> + c3 = -120371; >> + } else { >> + c1 = -10740; >> + c2 = 305415; >> + c3 = -129367; >> + } >> + nlux = c1 * red + c2 * green + c3 * blue; >> + if (nlux < 0) >> + nlux = 0; > > return max(0, nlux); is a bit neater and makes > it clear this is simple clamping to possible values given unlikely we'll see > negative light sources :) Ok. I should've remembered how you prefered max()/min() when clamping :) >> + >> + return nlux; >> +} >> + >> +static int bu27008_get_time_n_gains(struct bu27008_data *data, >> + unsigned int *gain, unsigned int *gain_ir, unsigned int *time) >> +{ >> + int ret; >> + >> + ret = bu27008_get_gain(data, &data->gts, gain); >> + if (ret < 0) >> + return ret; >> + >> + ret = bu27008_get_gain(data, &data->gts_ir, gain_ir); >> + if (ret < 0) >> + return ret; >> + >> + ret = bu27008_get_int_time_us(data); >> + if (ret < 0) >> + return ret; >> + >> + /* Max integration time is 400000i. Fits in signed int. */ >> + *time = ret; >> + >> + return 0; >> +} >> + >> +struct bu27008_buf { >> + __le16 chan[BU27008_NUM_HW_CHANS]; >> + u64 lux __aligned(8); >> + s64 ts __aligned(8); >> +}; >> + >> +static int bu27008_buffer_get_lux(struct bu27008_data *data, >> + struct bu27008_buf *raw) >> +{ >> + unsigned int red, green, blue, ir, gain, gain_ir, time; >> + int ret; >> + >> + red = le16_to_cpu(raw->chan[0]); >> + green = le16_to_cpu(raw->chan[1]); >> + blue = le16_to_cpu(raw->chan[2]); >> + ir = le16_to_cpu(raw->chan[3]); >> + ret = bu27008_get_time_n_gains(data, &gain, &gain_ir, &time); >> + if (ret) >> + return ret; >> + >> + raw->lux = bu27008_calc_nlux(data, red, green, blue, ir, gain, gain_ir, >> + time); > > Probably call this function *fill_in_lux() or something like that because I'd expect > a *get_lux() function to return the lux value. > Ok. Makes sense. Thanks for the review again! Yours, -- Matti
On Fri, 6 Oct 2023 08:01:15 +0300 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > On 10/5/23 18:14, Jonathan Cameron wrote: > > On Mon, 2 Oct 2023 15:33:41 +0300 > > Matti Vaittinen <mazziesaccount@gmail.com> 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@gmail.com> > >> > > > > 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. >
On 10/10/23 12:40, Jonathan Cameron wrote: > On Fri, 6 Oct 2023 08:01:15 +0300 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >> On 10/5/23 18:14, Jonathan Cameron wrote: >>> On Mon, 2 Oct 2023 15:33:41 +0300 >>> Matti Vaittinen <mazziesaccount@gmail.com> 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@gmail.com> >>>> >>> >>> 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
On 10/10/23 13:01, Matti Vaittinen wrote: > On 10/10/23 12:40, Jonathan Cameron wrote: >> On Fri, 6 Oct 2023 08:01:15 +0300 >> Matti Vaittinen <mazziesaccount@gmail.com> wrote: >> >>> On 10/5/23 18:14, Jonathan Cameron wrote: >>>> On Mon, 2 Oct 2023 15:33:41 +0300 >>>> Matti Vaittinen <mazziesaccount@gmail.com> wrote: > //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). I should've re-read the whole driver code before replying. I see we already have an enum for colours (with comments telling that some of the colours do not have fixed channel). So, my point objecting the enum is kind of moot. Unfortunately I chose the clear to be before IR, which means that if we used existing enum we would have a gap in the array (because clear is not used for lux computation). Not sure it matters though :) Yours, -- Matti
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]); + +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); + + if ((u64)ir * 100LLU > 18LLU * (u64)green) { + c1 = -22370; + c2 = 321900; + c3 = -120371; + } else { + c1 = -10740; + c2 = 305415; + c3 = -129367; + } + nlux = c1 * red + c2 * green + c3 * blue; + if (nlux < 0) + nlux = 0; + + return nlux; +} + +static int bu27008_get_time_n_gains(struct bu27008_data *data, + unsigned int *gain, unsigned int *gain_ir, unsigned int *time) +{ + int ret; + + ret = bu27008_get_gain(data, &data->gts, gain); + if (ret < 0) + return ret; + + ret = bu27008_get_gain(data, &data->gts_ir, gain_ir); + if (ret < 0) + return ret; + + ret = bu27008_get_int_time_us(data); + if (ret < 0) + return ret; + + /* Max integration time is 400000i. Fits in signed int. */ + *time = ret; + + return 0; +} + +struct bu27008_buf { + __le16 chan[BU27008_NUM_HW_CHANS]; + u64 lux __aligned(8); + s64 ts __aligned(8); +}; + +static int bu27008_buffer_get_lux(struct bu27008_data *data, + struct bu27008_buf *raw) +{ + unsigned int red, green, blue, ir, gain, gain_ir, time; + int ret; + + red = le16_to_cpu(raw->chan[0]); + green = le16_to_cpu(raw->chan[1]); + blue = le16_to_cpu(raw->chan[2]); + ir = le16_to_cpu(raw->chan[3]); + ret = bu27008_get_time_n_gains(data, &gain, &gain_ir, &time); + if (ret) + return ret; + + raw->lux = bu27008_calc_nlux(data, red, green, blue, ir, gain, gain_ir, + time); + + return 0; +} + +static int bu27008_read_lux(struct bu27008_data *data, struct iio_dev *idev, + struct iio_chan_spec const *chan, + int *val, int *val2) +{ + unsigned int red, green, blue, ir, gain, gain_ir, time; + u64 nlux; + int ret; + + ret = bu27008_get_rgb_ir(data, &red, &green, &blue, &ir); + if (ret) + return ret; + + ret = bu27008_get_time_n_gains(data, &gain, &gain_ir, &time); + if (ret) + return ret; + + nlux = bu27008_calc_nlux(data, red, green, blue, ir, gain, gain_ir, + time); + *val = (int)nlux; + *val2 = nlux >> 32LLU; + + return IIO_VAL_INT_64; +} + static int bu27008_read_raw(struct iio_dev *idev, struct iio_chan_spec const *chan, int *val, int *val2, long mask) @@ -1012,13 +1206,17 @@ static int bu27008_read_raw(struct iio_dev *idev, int busy, ret; switch (mask) { + case IIO_CHAN_INFO_RAW: busy = iio_device_claim_direct_mode(idev); if (busy) return -EBUSY; mutex_lock(&data->mutex); - ret = bu27008_read_one(data, idev, chan, val, val2); + if (chan->type == IIO_LIGHT) + ret = bu27008_read_lux(data, idev, chan, val, val2); + else + ret = bu27008_read_one(data, idev, chan, val, val2); mutex_unlock(&data->mutex); iio_device_release_direct_mode(idev); @@ -1026,6 +1224,11 @@ static int bu27008_read_raw(struct iio_dev *idev, return ret; case IIO_CHAN_INFO_SCALE: + if (chan->type == IIO_LIGHT) { + *val = 0; + *val2 = 1; + return IIO_VAL_INT_PLUS_NANO; + } ret = bu27008_get_scale(data, chan->scan_index == BU27008_IR, val, val2); if (ret) @@ -1231,15 +1434,13 @@ static const struct iio_trigger_ops bu27008_trigger_ops = { .reenable = bu27008_trigger_reenable, }; + static irqreturn_t bu27008_trigger_handler(int irq, void *p) { struct iio_poll_func *pf = p; struct iio_dev *idev = pf->indio_dev; struct bu27008_data *data = iio_priv(idev); - struct { - __le16 chan[BU27008_NUM_HW_CHANS]; - s64 ts __aligned(8); - } raw; + struct bu27008_buf raw; int ret, dummy; memset(&raw, 0, sizeof(raw)); @@ -1256,6 +1457,11 @@ static irqreturn_t bu27008_trigger_handler(int irq, void *p) sizeof(raw.chan)); if (ret < 0) goto err_read; + if (test_bit(BU27008_LUX, idev->active_scan_mask)) { + ret = bu27008_buffer_get_lux(data, &raw); + if (ret) + goto err_read; + } iio_push_to_buffers_with_timestamp(idev, &raw, pf->timestamp); err_read:
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. 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@gmail.com> --- 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(-) base-commit: 5e99f692d4e32e3250ab18d511894ca797407aec