| Message ID | fb35de40a3908988f5f83e25d17119e6944d289b.1682495921.git.mazziesaccount@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Support ROHM BU27008 RGB sensor | expand |
On Wed, 26 Apr 2023 11:08:17 +0300 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > The ROHM BU27008 is a sensor with 5 photodiodes (red, green, blue, clear > and IR) with four configurable channels. Red and green being always > available and two out of the rest three (blue, clear, IR) can be > selected to be simultaneously measured. Typical application is adjusting > LCD backlight of TVs, mobile phones and tablet PCs. > > Add initial support for the ROHM BU27008 color sensor. > - raw_read() of RGB and clear channels > - triggered buffer w/ DRDY interrtupt > > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> Hi Matti, Mostly trivial stuff, but some confusion has occurred with respect to the two interrupts involve in an IIO trigger. Specifically the pollfunc stuff occurs on the downward side of the irqchip that hides on the consumer side of an IIO trigger) and is set up by devm_iio_trigered_buffer_setup() not of the interrupt that calls iio_trigger_poll[_nested]() Jonathan > > --- > Revision history > v2 => v3: > - drop bits.h > - drop unnecessary comma after compatible > - Styling / cleaning > - Simplify sleep time computation > - rename bu27008_get_int_time() => bu27008_get_int_time_us() > > v1 => v2: > - Fix buffered data demuxing > - Use generic trigger functions instead of rolling own ones > - Drop unnecessary locking > - Use generic iio_validate_own_trigger() > - Some other more trivial fixes for review comments > - use defines for [enable/disable] [measurement/data-ready IRQ] reg values > and use regmap_update_bits() directly instead of regamap_[set/clear]_bits() > --- > drivers/iio/light/Kconfig | 14 + > drivers/iio/light/Makefile | 1 + > drivers/iio/light/rohm-bu27008.c | 963 +++++++++++++++++++++++++++++++ > 3 files changed, 978 insertions(+) > create mode 100644 drivers/iio/light/rohm-bu27008.c > > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig > index 6fa31fcd71a1..7888fc439b2f 100644 > --- a/drivers/iio/light/Kconfig > +++ b/drivers/iio/light/Kconfig > @@ -289,6 +289,20 @@ config JSA1212 > To compile this driver as a module, choose M here: > the module will be called jsa1212. > > +config ROHM_BU27008 > + tristate "ROHM BU27008 color (RGB+C/IR) sensor" > + depends on I2C > + select REGMAP_I2C > + select IIO_GTS_HELPER > + help > + Enable support for the ROHM BU27008 color sensor. > + The ROHM BU27008 is a sensor with 5 photodiodes (red, green, > + blue, clear and IR) with four configurable channels. Red and > + green being always available and two out of the rest three > + (blue, clear, IR) can be selected to be simultaneously measured. > + Typical application is adjusting LCD backlight of TVs, > + mobile phones and tablet PCs. > + > config ROHM_BU27034 > tristate "ROHM BU27034 ambient light sensor" > depends on I2C > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile > index 985f6feaccd4..881173952301 100644 > --- a/drivers/iio/light/Makefile > +++ b/drivers/iio/light/Makefile > @@ -39,6 +39,7 @@ obj-$(CONFIG_NOA1305) += noa1305.o > obj-$(CONFIG_OPT3001) += opt3001.o > obj-$(CONFIG_PA12203001) += pa12203001.o > obj-$(CONFIG_ROHM_BU27034) += rohm-bu27034.o > +obj-$(CONFIG_ROHM_BU27008) += rohm-bu27008.o Numeric order in Makefile as well as Kconfig. > obj-$(CONFIG_RPR0521) += rpr0521.o > obj-$(CONFIG_SI1133) += si1133.o > obj-$(CONFIG_SI1145) += si1145.o > diff --git a/drivers/iio/light/rohm-bu27008.c b/drivers/iio/light/rohm-bu27008.c > new file mode 100644 > index 000000000000..2728e6d5a321 > --- /dev/null > +++ b/drivers/iio/light/rohm-bu27008.c > + > +enum { > + BU27008_RED, /* Always data0 */ > + BU27008_GREEN, /* Always data1 */ > + BU27008_BLUE, /* data2, configurable (blue / clear) */ > + BU27008_CLEAR, /* data2 or data3 */ > + BU27008_IR, /* data3 */ > + BU27008_NUM_CHANS > +}; > + > +enum { > + BU27008_DATA0, /* Always RED */ > + BU27008_DATA1, /* Always GREEN */ > + BU27008_DATA2, /* Blue or Clear */ > + BU27008_DATA3, /* IR or Clear */ > + BU27008_NUM_HW_CHANS > +}; > + > +/* We can always measure red and green at same time */ > +#define ALWAYS_SCANNABLE (BIT(BU27008_RED) | BIT(BU27008_GREEN)) > + > +/* We use these data channel configs. Ensure scan_masks below follow them too */ > +#define BU27008_BLUE2_CLEAR3 0x0 /* buffer is R, G, B, C */ > +#define BU27008_CLEAR2_IR3 0x1 /* buffer is R, G, C, IR */ > +#define BU27008_BLUE2_IR3 0x2 /* buffer is R, G, B, IR */ > + > +static const unsigned long bu27008_scan_masks[] = { > + /* buffer is R, G, B, C */ > + ALWAYS_SCANNABLE | BIT(BU27008_BLUE) | BIT(BU27008_CLEAR), > + /* buffer is R, G, C, IR */ > + ALWAYS_SCANNABLE | BIT(BU27008_CLEAR) | BIT(BU27008_IR), > + /* buffer is R, G, B, IR */ > + ALWAYS_SCANNABLE | BIT(BU27008_BLUE) | BIT(BU27008_IR), > + 0 > +}; > + > +static int bu27008_get_int_time_sel(struct bu27008_data *data, int *sel) > +{ > + int ret, val; > + > + ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL1, &val); > + *sel = FIELD_GET(BU27008_MASK_MEAS_MODE, val); > + > + return ret; > +} > + > +static int bu27008_set_int_time_sel(struct bu27008_data *data, int sel) > +{ > + return regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL1, > + BU27008_MASK_MEAS_MODE, sel); > +} > + > +static int bu27008_get_int_time_us(struct bu27008_data *data) > +{ > + int ret, sel; > + > + ret = bu27008_get_int_time_sel(data, &sel); > + if (ret) > + return ret; > + > + return iio_gts_find_int_time_by_sel(&data->gts, > + sel & BU27008_MASK_MEAS_MODE); sel already masked in bu27008_get_int_time_sel(). No point in doing it again (was going to say use FIELD_GET() but then noticed you did above!) > +} > + > +static int bu27008_try_find_new_time_gain(struct bu27008_data *data, int val, > + int val2, int *gain_sel) > +{ > + /* Could not support new scale with existing int-time */ I'd move that above the function and change it to /* Called if the new scale could not be supported with existing int-time */ Down here it is not clear that this applies to the whole funciton. > + int i, ret, new_time_sel; > + > + for (i = 0; i < data->gts.num_itime; i++) { > + new_time_sel = data->gts.itime_table[i].sel; > + ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts, > + new_time_sel, val, val2 * 1000, gain_sel); > + if (!ret) > + break; > + } > + if (i == data->gts.num_itime) { > + dev_err(data->dev, "Can't support scale %u %u\n", val, > + val2); Line wrapping inconsistent. I like short lines with appropriate flexibility where longer ones are more readable. However, I am fairly sure this one fits under 80 chars as a single line. > + > + return -EINVAL; > + } > + > + return bu27008_set_int_time_sel(data, new_time_sel); > +} > + > +static int bu27008_set_scale(struct bu27008_data *data, > + struct iio_chan_spec const *chan, > + int val, int val2) > +{ > + int ret, gain_sel, time_sel; > + > + if (chan->scan_index == BU27008_IR) > + return -EINVAL; > + > + mutex_lock(&data->mutex); > + > + ret = bu27008_get_int_time_sel(data, &time_sel); > + if (ret < 0) > + goto unlock_out; > + > + ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts, time_sel, > + val, val2 * 1000, &gain_sel); > + if (ret) > + ret = bu27008_try_find_new_time_gain(data, val, val2, &gain_sel); Obviously it is code that doesn't make any functional difference, but I'd prefer to see if (ret) { ret = bu27.... if (ret) goto unlock_out; } ret = bu27008_write_gain_sel(); so that each error path is out of line, but the good path is the linear flow. > + > + if (!ret) > + ret = bu27008_write_gain_sel(data, gain_sel); > + > +unlock_out: > + mutex_unlock(&data->mutex); > + > + return ret; > +} > + > +static int bu27008_write_raw(struct iio_dev *idev, > + struct iio_chan_spec const *chan, > + int val, int val2, long mask) > +{ > + struct bu27008_data *data = iio_priv(idev); > + int ret; > + > + /* > + * We should not allow changing scale when measurement is ongoing. "We should" is a statement of a desire, whereas here you want to say it is prevented. * Do not allow changing scale when measurement is ongoing as * doing so could make values in the buffer inconsistent. */ > + * This could make values in buffer inconsistent. > + */ > + ret = iio_device_claim_direct_mode(idev); > + if (ret) > + return ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_SCALE: > + ret = bu27008_set_scale(data, chan, val, val2); > + break; > + case IIO_CHAN_INFO_INT_TIME: > + if (val) > + ret = -EINVAL; > + else > + ret = bu27008_try_set_int_time(data, val2); > + break; > + default: > + ret = -EINVAL; > + break; > + } > + iio_device_release_direct_mode(idev); > + > + return ret; > +} > +static irqreturn_t bu27008_irq_thread_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); > + > + iio_trigger_poll_nested(data->trig); See below but this is what alerted me to something unusual. It never makes sense to have iio_trigger_poll_nested() called unless there is a check on whether it should be called! If there isn't iio_trigger_poll() in the top half is the right thing to do. > + > + return IRQ_HANDLED; > +} > + > +static int bu27008_probe(struct i2c_client *i2c) > +{ ... > + > + if (i2c->irq) { > + ret = devm_iio_triggered_buffer_setup(dev, idev, > + &iio_pollfunc_store_time, > + bu27008_trigger_handler, > + &bu27008_buffer_ops); > + if (ret) > + return dev_err_probe(dev, ret, > + "iio_triggered_buffer_setup_ext FAIL\n"); > + > + itrig = devm_iio_trigger_alloc(dev, "%sdata-rdy-dev%d", > + idev->name, iio_device_id(idev)); > + if (!itrig) > + return -ENOMEM; > + > + data->trig = itrig; > + > + itrig->ops = &bu27008_trigger_ops; > + iio_trigger_set_drvdata(itrig, data); > + > + name = devm_kasprintf(dev, GFP_KERNEL, "%s-bu27008", > + dev_name(dev)); > + > + ret = devm_request_threaded_irq(dev, i2c->irq, > + iio_pollfunc_store_time, This is on the wrong irq. iio_pollfunc_store_time is used with the trigger not here. Basically what happens is the caller of iio_poll_trigger() fires the input to a software irq chip that then signals all the of the downstream irqs (which are the individual consumers of the triggers). If that's triggered from the top half / non threaded bit of the interrupt the iio_pollfunc_store_time() will be called in that non threaded context before the individual threads for the trigger consumer are started. If there is nothing to do in the actual interrupt as it's a data ready only signal, then you should just call iio_trigger_poll() in the top half and use devm_request_irq() only as there is no thread in this interrupt (though there is one for the interrupt below the software interrupt chip). > + &bu27008_irq_thread_handler, > + IRQF_ONESHOT, name, idev->pollfunc); > + if (ret) > + return dev_err_probe(dev, ret, > + "Could not request IRQ\n"); > + > + > + ret = devm_iio_trigger_register(dev, itrig); > + if (ret) > + return dev_err_probe(dev, ret, > + "Trigger registration failed\n"); > + } else { > + dev_warn(dev, "No IRQ configured\n"); Why is it a warning? Either driver works without an IRQ, or it doesn't. dev_dbg() or dev_info() at most. > + } > + > + ret = devm_iio_device_register(dev, idev); > + if (ret) > + return dev_err_probe(dev, ret, > + "Unable to register iio device\n"); > + > + return ret; > +}
On 5/1/23 17:20, Jonathan Cameron wrote: > On Wed, 26 Apr 2023 11:08:17 +0300 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >> The ROHM BU27008 is a sensor with 5 photodiodes (red, green, blue, clear >> and IR) with four configurable channels. Red and green being always >> available and two out of the rest three (blue, clear, IR) can be >> selected to be simultaneously measured. Typical application is adjusting >> LCD backlight of TVs, mobile phones and tablet PCs. >> >> Add initial support for the ROHM BU27008 color sensor. >> - raw_read() of RGB and clear channels >> - triggered buffer w/ DRDY interrtupt >> >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> > > Hi Matti, > > Mostly trivial stuff, but some confusion has occurred with respect to the > two interrupts involve in an IIO trigger. Specifically the pollfunc stuff > occurs on the downward side of the irqchip that hides on the consumer side > of an IIO trigger) and is set up by devm_iio_trigered_buffer_setup() not > of the interrupt that calls iio_trigger_poll[_nested]() > > >> + >> +static int bu27008_try_find_new_time_gain(struct bu27008_data *data, int val, >> + int val2, int *gain_sel) >> +{ >> + /* Could not support new scale with existing int-time */ > > I'd move that above the function and change it to > /* Called if the new scale could not be supported with existing int-time */ > Down here it is not clear that this applies to the whole funciton. > > >> + int i, ret, new_time_sel; >> + >> + for (i = 0; i < data->gts.num_itime; i++) { >> + new_time_sel = data->gts.itime_table[i].sel; >> + ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts, >> + new_time_sel, val, val2 * 1000, gain_sel); >> + if (!ret) >> + break; >> + } >> + if (i == data->gts.num_itime) { >> + dev_err(data->dev, "Can't support scale %u %u\n", val, >> + val2); > > Line wrapping inconsistent. I like short lines with appropriate flexibility where > longer ones are more readable. However, I am fairly sure this one fits under 80 > chars as a single line. Thanks! Both the comment and the line-wrapping were just left like this when I pulled this part of a functionality into this new function. Well spotted! > >> + >> + return -EINVAL; >> + } >> + >> + return bu27008_set_int_time_sel(data, new_time_sel); >> +} >> + >> +static int bu27008_set_scale(struct bu27008_data *data, >> + struct iio_chan_spec const *chan, >> + int val, int val2) >> +{ >> + int ret, gain_sel, time_sel; >> + >> + if (chan->scan_index == BU27008_IR) >> + return -EINVAL; >> + >> + mutex_lock(&data->mutex); >> + >> + ret = bu27008_get_int_time_sel(data, &time_sel); >> + if (ret < 0) >> + goto unlock_out; >> + >> + ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts, time_sel, >> + val, val2 * 1000, &gain_sel); >> + if (ret) >> + ret = bu27008_try_find_new_time_gain(data, val, val2, &gain_sel); > > Obviously it is code that doesn't make any functional difference, but I'd prefer to see > if (ret) { > ret = bu27.... > if (ret) > goto unlock_out; > } > ret = bu27008_write_gain_sel(); > > so that each error path is out of line, but the good path is the linear flow. ... Yuck! The difference of tastes ... ;) Well, not worth fighting I guess. > >> + >> + if (!ret) >> + ret = bu27008_write_gain_sel(data, gain_sel); >> + >> +unlock_out: >> + mutex_unlock(&data->mutex); >> + >> + return ret; >> +} > > >> +static irqreturn_t bu27008_irq_thread_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); >> + >> + iio_trigger_poll_nested(data->trig); > > See below but this is what alerted me to something unusual. > It never makes sense to have iio_trigger_poll_nested() called unless > there is a check on whether it should be called! If there isn't > iio_trigger_poll() in the top half is the right thing to do. > >> + >> + return IRQ_HANDLED; >> +} >> + > > >> +static int bu27008_probe(struct i2c_client *i2c) >> +{ > > ... > >> + >> + if (i2c->irq) { >> + ret = devm_iio_triggered_buffer_setup(dev, idev, >> + &iio_pollfunc_store_time, >> + bu27008_trigger_handler, >> + &bu27008_buffer_ops); >> + if (ret) >> + return dev_err_probe(dev, ret, >> + "iio_triggered_buffer_setup_ext FAIL\n"); >> + >> + itrig = devm_iio_trigger_alloc(dev, "%sdata-rdy-dev%d", >> + idev->name, iio_device_id(idev)); >> + if (!itrig) >> + return -ENOMEM; >> + >> + data->trig = itrig; >> + >> + itrig->ops = &bu27008_trigger_ops; >> + iio_trigger_set_drvdata(itrig, data); >> + >> + name = devm_kasprintf(dev, GFP_KERNEL, "%s-bu27008", >> + dev_name(dev)); >> + >> + ret = devm_request_threaded_irq(dev, i2c->irq, >> + iio_pollfunc_store_time, > > This is on the wrong irq. Seems like I have some homework to do :) Right. I now see I pass the iio_pollfunc_store_time() as top half for both the "real IRQ" generated by the device (here), as well as a top-half for the devm_iio_triggered_buffer_setup(). Ideally I like the idea of taking the timestamp in the top half for the device-generated IRQ as it is closer the moment HW did acquire the sample - but it really would make no difference here (even if I did it correctly). iio_pollfunc_store_time is used with the trigger not > here. Basically what happens is the caller of iio_poll_trigger() fires the input > to a software irq chip that then signals all the of the downstream irqs (which > are the individual consumers of the triggers). If that's triggered from the > top half / non threaded bit of the interrupt the iio_pollfunc_store_time() > will be called in that non threaded context before the individual threads > for the trigger consumer are started. Oh. So, you mean the iio_pollfunc_store_time() is automatically called already before kicking the SW-IRQ? So we don't need it in devm_iio_triggered_buffer_setup() anymore? > If there is nothing to do in the actual interrupt as it's a data ready > only signal, then you should just call iio_trigger_poll() in the top half and > use devm_request_irq() only as there is no thread in this interrupt (though > there is one for the interrupt below the software interrupt chip). I haven't tested this yet so please ignore me if I am writing nonsense - but... The BU27008 will keep the IRQ line asserted until a register is read. We can't read the register form HW-IRQ so we need to keep the IRQ disabled until the threaded trigger handler is ran. With the setup we have here, the IRQF_ONESHOT, took care of this. I assume that changing to call the iio_poll_trigger() from top-half means I need to explicitly disable the IRQ and re-enable it at the end of the trigger thread after reading the register which debounces the IRQ line? > > >> + &bu27008_irq_thread_handler, >> + IRQF_ONESHOT, name, idev->pollfunc); >> + if (ret) >> + return dev_err_probe(dev, ret, >> + "Could not request IRQ\n"); >> + >> + >> + ret = devm_iio_trigger_register(dev, itrig); >> + if (ret) >> + return dev_err_probe(dev, ret, >> + "Trigger registration failed\n"); >> + } else { >> + dev_warn(dev, "No IRQ configured\n"); > > Why is it a warning? Either driver works without an IRQ, or it doesn't. > dev_dbg() or dev_info() at most. Some of it works. Well, maybe I'll change it to tell that device works in raw_read only mode. Thanks again for the help! Yours, -- Matti
On Wed, Apr 26, 2023 at 11:08:17AM +0300, Matti Vaittinen wrote: > The ROHM BU27008 is a sensor with 5 photodiodes (red, green, blue, clear > and IR) with four configurable channels. Red and green being always > available and two out of the rest three (blue, clear, IR) can be > selected to be simultaneously measured. Typical application is adjusting > LCD backlight of TVs, mobile phones and tablet PCs. > > Add initial support for the ROHM BU27008 color sensor. > - raw_read() of RGB and clear channels > - triggered buffer w/ DRDY interrtupt ... > +enum { > + BU27008_RED, /* Always data0 */ > + BU27008_GREEN, /* Always data1 */ > + BU27008_BLUE, /* data2, configurable (blue / clear) */ > + BU27008_CLEAR, /* data2 or data3 */ > + BU27008_IR, /* data3 */ > + BU27008_NUM_CHANS Why not converting comments to a kernel-doc? > +}; > + > +enum { > + BU27008_DATA0, /* Always RED */ > + BU27008_DATA1, /* Always GREEN */ > + BU27008_DATA2, /* Blue or Clear */ > + BU27008_DATA3, /* IR or Clear */ > + BU27008_NUM_HW_CHANS > +}; Ditto. ... > + if (int_time < 0) > + int_time = 400000; Adding 3 0:s to drop them below with a heavy division operation? Well done! Or did I miss anything? > + msleep(int_time / 1000); USEC_PER_MSEC ? ... > + ret = devm_iio_device_register(dev, idev); > + if (ret) > + return dev_err_probe(dev, ret, > + "Unable to register iio device\n"); > + > + return ret; return 0 will suffice.
Hi Andy Thanks for the review. On 5/2/23 23:40, Andy Shevchenko wrote: > On Wed, Apr 26, 2023 at 11:08:17AM +0300, Matti Vaittinen wrote: >> The ROHM BU27008 is a sensor with 5 photodiodes (red, green, blue, clear >> and IR) with four configurable channels. Red and green being always >> available and two out of the rest three (blue, clear, IR) can be >> selected to be simultaneously measured. Typical application is adjusting >> LCD backlight of TVs, mobile phones and tablet PCs. >> >> Add initial support for the ROHM BU27008 color sensor. >> - raw_read() of RGB and clear channels >> - triggered buffer w/ DRDY interrtupt > > ... > >> +enum { >> + BU27008_RED, /* Always data0 */ >> + BU27008_GREEN, /* Always data1 */ >> + BU27008_BLUE, /* data2, configurable (blue / clear) */ >> + BU27008_CLEAR, /* data2 or data3 */ >> + BU27008_IR, /* data3 */ >> + BU27008_NUM_CHANS > > Why not converting comments to a kernel-doc? > >> +}; >> + >> +enum { >> + BU27008_DATA0, /* Always RED */ >> + BU27008_DATA1, /* Always GREEN */ >> + BU27008_DATA2, /* Blue or Clear */ >> + BU27008_DATA3, /* IR or Clear */ >> + BU27008_NUM_HW_CHANS >> +}; > > Ditto. I see no value having entities which are not intended to be used outside this file documented in any "global" documentation. One who is ever going to use these or wonder what these are - will most likely be watching this file. My personal view is that the generated docs should be kept lean. In my opinion the problem of the day is the time we spend looking for a needle hidden in a haystack. In my opinion adding this to kernel-doc just adds hay :) I still can do this if no-one else objects. I almost never look at the generated docs myself. Usually I just look the docs from code files - and kernel-doc format is not any worse for me to read. Still, I can imagine including this type of stuff to generic doc just bloats them and my not serve well those who use them. > > ... > >> + if (int_time < 0) >> + int_time = 400000; > > Adding 3 0:s to drop them below with a heavy division operation? Well done! > Or did I miss anything? No. You did not miss anything. This can be improved. > >> + msleep(int_time / 1000); > > USEC_PER_MSEC ? Ok. > ... > >> + ret = devm_iio_device_register(dev, idev); >> + if (ret) >> + return dev_err_probe(dev, ret, >> + "Unable to register iio device\n"); >> + >> + return ret; > > return 0 will suffice. Ok. Thanks, -- Matti
> > > >> + > >> + if (i2c->irq) { > >> + ret = devm_iio_triggered_buffer_setup(dev, idev, > >> + &iio_pollfunc_store_time, > >> + bu27008_trigger_handler, > >> + &bu27008_buffer_ops); > >> + if (ret) > >> + return dev_err_probe(dev, ret, > >> + "iio_triggered_buffer_setup_ext FAIL\n"); > >> + > >> + itrig = devm_iio_trigger_alloc(dev, "%sdata-rdy-dev%d", > >> + idev->name, iio_device_id(idev)); > >> + if (!itrig) > >> + return -ENOMEM; > >> + > >> + data->trig = itrig; > >> + > >> + itrig->ops = &bu27008_trigger_ops; > >> + iio_trigger_set_drvdata(itrig, data); > >> + > >> + name = devm_kasprintf(dev, GFP_KERNEL, "%s-bu27008", > >> + dev_name(dev)); > >> + > >> + ret = devm_request_threaded_irq(dev, i2c->irq, > >> + iio_pollfunc_store_time, > > > > This is on the wrong irq. > > Seems like I have some homework to do :) > > Right. I now see I pass the iio_pollfunc_store_time() as top half for > both the "real IRQ" generated by the device (here), as well as a > top-half for the devm_iio_triggered_buffer_setup(). Ideally I like the > idea of taking the timestamp in the top half for the device-generated > IRQ as it is closer the moment HW did acquire the sample - but it really > would make no difference here (even if I did it correctly). It's the matter of few function calls later. So trivial timing wise. > > iio_pollfunc_store_time is used with the trigger not > > here. Basically what happens is the caller of iio_poll_trigger() fires the input > > to a software irq chip that then signals all the of the downstream irqs (which > > are the individual consumers of the triggers). If that's triggered from the > > top half / non threaded bit of the interrupt the iio_pollfunc_store_time() > > will be called in that non threaded context before the individual threads > > for the trigger consumer are started. > > Oh. So, you mean the iio_pollfunc_store_time() is automatically called > already before kicking the SW-IRQ? So we don't need it in > devm_iio_triggered_buffer_setup() anymore? No. Whatever you register as the top half of the poll_func in the call to devm_iio_triggered_buffer_setup() will be called as part of the SW irq chip handling - not this isn't a SW-IRQ at all, it's just some demultiplexing code. The top half of an IIO poll func runs in the the trigger interrupt handler (under iio_trigger_poll) before those in turn start their own interrupt threads to handle whatever you registered as the threads in devm_iio_trigger_buffer_setup() > > > If there is nothing to do in the actual interrupt as it's a data ready > > only signal, then you should just call iio_trigger_poll() in the top half and > > use devm_request_irq() only as there is no thread in this interrupt (though > > there is one for the interrupt below the software interrupt chip). > > I haven't tested this yet so please ignore me if I am writing nonsense - > but... The BU27008 will keep the IRQ line asserted until a register is > read. We can't read the register form HW-IRQ so we need to keep the IRQ > disabled until the threaded trigger handler is ran. With the setup we > have here, the IRQF_ONESHOT, took care of this. I assume that changing > to call the iio_poll_trigger() from top-half means I need to explicitly > disable the IRQ and re-enable it at the end of the trigger thread after > reading the register which debounces the IRQ line? Hmm. I'm trying to remember how this works (wrote this a very long time ago). I'm fairly sure it's not an issue because we use IRQF_ONESHOT down one level so exercise the same prevention of the threads triggering multiple times etc. https://elixir.bootlin.com/linux/latest/source/drivers/iio/buffer/industrialio-triggered-buffer.c#L57 It doesn't matter if the device interrupt fires again as it will still be masked at our software irqchip level and will then get queued up and the thread will run again. > > > > > > >> + &bu27008_irq_thread_handler, > >> + IRQF_ONESHOT, name, idev->pollfunc); > >> + if (ret) > >> + return dev_err_probe(dev, ret, > >> + "Could not request IRQ\n"); > >> + > >> + > >> + ret = devm_iio_trigger_register(dev, itrig); > >> + if (ret) > >> + return dev_err_probe(dev, ret, > >> + "Trigger registration failed\n"); > >> + } else { > >> + dev_warn(dev, "No IRQ configured\n"); > > > > Why is it a warning? Either driver works without an IRQ, or it doesn't. > > dev_dbg() or dev_info() at most. > > Some of it works. Well, maybe I'll change it to tell that device works > in raw_read only mode. > > Thanks again for the help! > > Yours, > -- Matti >
On Wed, 3 May 2023 05:11:53 +0000 "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com> wrote: > Hi Andy > > Thanks for the review. > > On 5/2/23 23:40, Andy Shevchenko wrote: > > On Wed, Apr 26, 2023 at 11:08:17AM +0300, Matti Vaittinen wrote: > >> The ROHM BU27008 is a sensor with 5 photodiodes (red, green, blue, clear > >> and IR) with four configurable channels. Red and green being always > >> available and two out of the rest three (blue, clear, IR) can be > >> selected to be simultaneously measured. Typical application is adjusting > >> LCD backlight of TVs, mobile phones and tablet PCs. > >> > >> Add initial support for the ROHM BU27008 color sensor. > >> - raw_read() of RGB and clear channels > >> - triggered buffer w/ DRDY interrtupt > > > > ... > > > >> +enum { > >> + BU27008_RED, /* Always data0 */ > >> + BU27008_GREEN, /* Always data1 */ > >> + BU27008_BLUE, /* data2, configurable (blue / clear) */ > >> + BU27008_CLEAR, /* data2 or data3 */ > >> + BU27008_IR, /* data3 */ > >> + BU27008_NUM_CHANS > > > > Why not converting comments to a kernel-doc? > > > >> +}; > >> + > >> +enum { > >> + BU27008_DATA0, /* Always RED */ > >> + BU27008_DATA1, /* Always GREEN */ > >> + BU27008_DATA2, /* Blue or Clear */ > >> + BU27008_DATA3, /* IR or Clear */ > >> + BU27008_NUM_HW_CHANS > >> +}; > > > > Ditto. > > I see no value having entities which are not intended to be used outside > this file documented in any "global" documentation. One who is ever > going to use these or wonder what these are - will most likely be > watching this file. My personal view is that the generated docs should > be kept lean. In my opinion the problem of the day is the time we spend > looking for a needle hidden in a haystack. In my opinion adding this to > kernel-doc just adds hay :) > > I still can do this if no-one else objects. I almost never look at the > generated docs myself. Usually I just look the docs from code files - > and kernel-doc format is not any worse for me to read. Still, I can > imagine including this type of stuff to generic doc just bloats them and > my not serve well those who use them. Unless someone specifically adds this doc to the main docs build, the kernel-doc won't end up in the docs anyway. It just provides a nice bit of consistent formatting. Even if they do add this for some reason, there are controls on internal vs external (exported stuff) being added to the docs. > > > > > ... > > > >> + if (int_time < 0) > >> + int_time = 400000; > > > > Adding 3 0:s to drop them below with a heavy division operation? Well done! > > Or did I miss anything? > > No. You did not miss anything. This can be improved. > > > > >> + msleep(int_time / 1000); > > > > USEC_PER_MSEC ? > > Ok. > > > ... > > > >> + ret = devm_iio_device_register(dev, idev); > >> + if (ret) > >> + return dev_err_probe(dev, ret, > >> + "Unable to register iio device\n"); > >> + > >> + return ret; > > > > return 0 will suffice. > > Ok. > > Thanks, > -- Matti >
On 5/7/23 17:24, Jonathan Cameron wrote: > On Wed, 3 May 2023 05:11:53 +0000 > "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com> wrote: > >> Hi Andy >> >> Thanks for the review. >> >> On 5/2/23 23:40, Andy Shevchenko wrote: >>> On Wed, Apr 26, 2023 at 11:08:17AM +0300, Matti Vaittinen wrote: >>>> The ROHM BU27008 is a sensor with 5 photodiodes (red, green, blue, clear >>>> and IR) with four configurable channels. Red and green being always >>>> available and two out of the rest three (blue, clear, IR) can be >>>> selected to be simultaneously measured. Typical application is adjusting >>>> LCD backlight of TVs, mobile phones and tablet PCs. >>>> >>>> Add initial support for the ROHM BU27008 color sensor. >>>> - raw_read() of RGB and clear channels >>>> - triggered buffer w/ DRDY interrtupt >>> >>> ... >>> >>>> +enum { >>>> + BU27008_RED, /* Always data0 */ >>>> + BU27008_GREEN, /* Always data1 */ >>>> + BU27008_BLUE, /* data2, configurable (blue / clear) */ >>>> + BU27008_CLEAR, /* data2 or data3 */ >>>> + BU27008_IR, /* data3 */ >>>> + BU27008_NUM_CHANS >>> >>> Why not converting comments to a kernel-doc? >>> >>>> +}; >>>> + >>>> +enum { >>>> + BU27008_DATA0, /* Always RED */ >>>> + BU27008_DATA1, /* Always GREEN */ >>>> + BU27008_DATA2, /* Blue or Clear */ >>>> + BU27008_DATA3, /* IR or Clear */ >>>> + BU27008_NUM_HW_CHANS >>>> +}; >>> >>> Ditto. >> >> I see no value having entities which are not intended to be used outside >> this file documented in any "global" documentation. One who is ever >> going to use these or wonder what these are - will most likely be >> watching this file. My personal view is that the generated docs should >> be kept lean. In my opinion the problem of the day is the time we spend >> looking for a needle hidden in a haystack. In my opinion adding this to >> kernel-doc just adds hay :) > >> >> I still can do this if no-one else objects. I almost never look at the >> generated docs myself. Usually I just look the docs from code files - >> and kernel-doc format is not any worse for me to read. Still, I can >> imagine including this type of stuff to generic doc just bloats them and >> my not serve well those who use them. > > > Unless someone specifically adds this doc to the main docs build, the > kernel-doc won't end up in the docs anyway. Ah! This makes sense. Thanks for correcting me! > It just provides a nice > bit of consistent formatting. Even if they do add this for some reason, > there are controls on internal vs external (exported stuff) being added > to the docs. I'll use kernel-doc for this then. Yours, -- Matti
On 5/7/23 17:22, Jonathan Cameron wrote: >>> If there is nothing to do in the actual interrupt as it's a data ready >>> only signal, then you should just call iio_trigger_poll() in the top half and >>> use devm_request_irq() only as there is no thread in this interrupt (though >>> there is one for the interrupt below the software interrupt chip). >> >> I haven't tested this yet so please ignore me if I am writing nonsense - >> but... The BU27008 will keep the IRQ line asserted until a register is >> read. We can't read the register form HW-IRQ so we need to keep the IRQ >> disabled until the threaded trigger handler is ran. With the setup we >> have here, the IRQF_ONESHOT, took care of this. I assume that changing >> to call the iio_poll_trigger() from top-half means I need to explicitly >> disable the IRQ and re-enable it at the end of the trigger thread after >> reading the register which debounces the IRQ line? > > Hmm. I'm trying to remember how this works (wrote this a very long time ago). > I'm fairly sure it's not an issue because we use IRQF_ONESHOT down one level > so exercise the same prevention of the threads triggering multiple times etc > https://elixir.bootlin.com/linux/latest/source/drivers/iio/buffer/industrialio-triggered-buffer.c#L57 > > It doesn't matter if the device interrupt fires again as it will still be masked > at our software irqchip level and will then get queued up and the thread will > run again. After reading this I am not at all sure I am using the trigger correctly. I see the iio_trigger_attach_poll_func() registering threaded handler with the IRQF_ONESHOT which is stored in the iio_alloc_pollfunc() as you pointed above. The iio_trigger_attach_poll_func() is called from sysfs callback when trigger is enabled. So, if this is supposed to be the device IRQ, then I am not at all sure the driver should be requesting the IRQ at the probe(). If it is not the device's IRQ, then I guess the IRQF_ONESHOT passed in here won't help. I need to try seeing some examples how other drivers are using the triggers. Getting back to this tomorrow... Yours, -- Matti.
On Sun, May 07, 2023 at 03:24:43PM +0100, Jonathan Cameron wrote: > On Wed, 3 May 2023 05:11:53 +0000 > "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com> wrote: > > On 5/2/23 23:40, Andy Shevchenko wrote: > > > On Wed, Apr 26, 2023 at 11:08:17AM +0300, Matti Vaittinen wrote: ... > > >> +enum { > > >> + BU27008_RED, /* Always data0 */ > > >> + BU27008_GREEN, /* Always data1 */ > > >> + BU27008_BLUE, /* data2, configurable (blue / clear) */ > > >> + BU27008_CLEAR, /* data2 or data3 */ > > >> + BU27008_IR, /* data3 */ > > >> + BU27008_NUM_CHANS > > > > > > Why not converting comments to a kernel-doc? > > > > > >> +}; > > >> + > > >> +enum { > > >> + BU27008_DATA0, /* Always RED */ > > >> + BU27008_DATA1, /* Always GREEN */ > > >> + BU27008_DATA2, /* Blue or Clear */ > > >> + BU27008_DATA3, /* IR or Clear */ > > >> + BU27008_NUM_HW_CHANS > > >> +}; > > > > > > Ditto. > > > > I see no value having entities which are not intended to be used outside > > this file documented in any "global" documentation. One who is ever > > going to use these or wonder what these are - will most likely be > > watching this file. My personal view is that the generated docs should > > be kept lean. In my opinion the problem of the day is the time we spend > > looking for a needle hidden in a haystack. In my opinion adding this to > > kernel-doc just adds hay :) > > > > > I still can do this if no-one else objects. I almost never look at the > > generated docs myself. Usually I just look the docs from code files - > > and kernel-doc format is not any worse for me to read. Still, I can > > imagine including this type of stuff to generic doc just bloats them and > > my not serve well those who use them. > > > Unless someone specifically adds this doc to the main docs build, the > kernel-doc won't end up in the docs anyway. It just provides a nice > bit of consistent formatting. Even if they do add this for some reason, > there are controls on internal vs external (exported stuff) being added > to the docs. I can run it manually and see in a nice form instead of browsing file for that, so there is still a usefulness in my opinion. Esp. taking into account that comments are already there. It's just different and helpful form of representation. No?
On 5/8/23 15:41, Andy Shevchenko wrote: > On Sun, May 07, 2023 at 03:24:43PM +0100, Jonathan Cameron wrote: >> On Wed, 3 May 2023 05:11:53 +0000 >> "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com> wrote: >>> On 5/2/23 23:40, Andy Shevchenko wrote: >>>> On Wed, Apr 26, 2023 at 11:08:17AM +0300, Matti Vaittinen wrote: > > ... > >>>>> +enum { >>>>> + BU27008_RED, /* Always data0 */ >>>>> + BU27008_GREEN, /* Always data1 */ >>>>> + BU27008_BLUE, /* data2, configurable (blue / clear) */ >>>>> + BU27008_CLEAR, /* data2 or data3 */ >>>>> + BU27008_IR, /* data3 */ >>>>> + BU27008_NUM_CHANS >>>> >>>> Why not converting comments to a kernel-doc? >>>> >>>>> +}; >>>>> + >>>>> +enum { >>>>> + BU27008_DATA0, /* Always RED */ >>>>> + BU27008_DATA1, /* Always GREEN */ >>>>> + BU27008_DATA2, /* Blue or Clear */ >>>>> + BU27008_DATA3, /* IR or Clear */ >>>>> + BU27008_NUM_HW_CHANS >>>>> +}; >>>> >>>> Ditto. >>> >>> I see no value having entities which are not intended to be used outside >>> this file documented in any "global" documentation. One who is ever >>> going to use these or wonder what these are - will most likely be >>> watching this file. My personal view is that the generated docs should >>> be kept lean. In my opinion the problem of the day is the time we spend >>> looking for a needle hidden in a haystack. In my opinion adding this to >>> kernel-doc just adds hay :) >> >>> >>> I still can do this if no-one else objects. I almost never look at the >>> generated docs myself. Usually I just look the docs from code files - >>> and kernel-doc format is not any worse for me to read. Still, I can >>> imagine including this type of stuff to generic doc just bloats them and >>> my not serve well those who use them. >> >> >> Unless someone specifically adds this doc to the main docs build, the >> kernel-doc won't end up in the docs anyway. It just provides a nice >> bit of consistent formatting. Even if they do add this for some reason, >> there are controls on internal vs external (exported stuff) being added >> to the docs. > > I can run it manually and see in a nice form instead of browsing file for that, > so there is still a usefulness in my opinion. Esp. taking into account that > comments are already there. It's just different and helpful form of > representation. No? My main objection was a _misunderstanding_ that the kernel-doc formatted comments would automatically end up in generated docs. As I wrote, I rarely (never) generate the docs. I use the docs from sources, hence it is not easy for me to see this value. Nevertheless, I also wrote >>> and kernel-doc format is not any worse for me to read. Hence, I did format these comments as kernel-doc in v5. The only slight disadvantage (from my perspective) in using the kernel-doc is increased amount of lines with pretty much no additional information. I can live with that though. Yours, -- Matti
On Sun, 7 May 2023 19:13:38 +0300 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > On 5/7/23 17:22, Jonathan Cameron wrote: > > >>> If there is nothing to do in the actual interrupt as it's a data ready > >>> only signal, then you should just call iio_trigger_poll() in the top half and > >>> use devm_request_irq() only as there is no thread in this interrupt (though > >>> there is one for the interrupt below the software interrupt chip). > >> > >> I haven't tested this yet so please ignore me if I am writing nonsense - > >> but... The BU27008 will keep the IRQ line asserted until a register is > >> read. We can't read the register form HW-IRQ so we need to keep the IRQ > >> disabled until the threaded trigger handler is ran. With the setup we > >> have here, the IRQF_ONESHOT, took care of this. I assume that changing > >> to call the iio_poll_trigger() from top-half means I need to explicitly > >> disable the IRQ and re-enable it at the end of the trigger thread after > >> reading the register which debounces the IRQ line? > > > > Hmm. I'm trying to remember how this works (wrote this a very long time ago). > > I'm fairly sure it's not an issue because we use IRQF_ONESHOT down one level > > so exercise the same prevention of the threads triggering multiple times etc > > https://elixir.bootlin.com/linux/latest/source/drivers/iio/buffer/industrialio-triggered-buffer.c#L57 > > > > It doesn't matter if the device interrupt fires again as it will still be masked > > at our software irqchip level and will then get queued up and the thread will > > run again. > > After reading this I am not at all sure I am using the trigger > correctly. I see the iio_trigger_attach_poll_func() registering threaded > handler with the IRQF_ONESHOT which is stored in the > iio_alloc_pollfunc() as you pointed above. > > The iio_trigger_attach_poll_func() is called from sysfs callback when > trigger is enabled. So, if this is supposed to be the device IRQ, then I > am not at all sure the driver should be requesting the IRQ at the > probe(). If it is not the device's IRQ, then I guess the IRQF_ONESHOT > passed in here won't help. I need to try seeing some examples how other > drivers are using the triggers. Getting back to this tomorrow... I think you are confusing two different irqs here / I managed to confuse you even more. The one in iio_trigger_attach_pollfunc() is a child of the trigger - it's called when the trigger 'fires' along with any other children attached (if multiple IIO devices have current_trigger set tot his one). The one you are registering for is the parent of the trigger and calls iio_poll_trigger() in it's handler which in turn will result in interrupts for any children of the trigger (that is the pollfunc's which are interrupt handlers underneath). Not sure this horrible diagram will help Data Ready Interrupt | iio_trigger_poll() after checking it's right int etc. Easy here. | ________|__________________________________________________________________ | | | | Pollfunc 1 Int Pollfunc 2 Int | | Top half of PF 1 Top half of PF2 return IRQ_THREAD etc | | ..................................... | | Thread PF 1 Thread PF 2 | | iio_push_to_buffers..() iio_push_to_bufffers..() For trigger consumer For trigger consumer device 1 device 2. So in the pollfunc interrupts have IRQF_ONESHOT set so the masking happens in the internal irq_chip that IIO uses to handle this demux of the trigger signal to multiple different sensor drivers to make them grab data to push into buffers. This whole thing is built to enable sync capture of multiple signals where possible (hardware restrictions mean this only works for some combinations). Years ago, we had our own registration scheme that didn't use interrupts internally and it was pointed out (Arnd Bergmann I think...) that we were just replicating what goes on in an interrupt chip so should use that instead. Jonathan > > Yours, > -- Matti. >
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig index 6fa31fcd71a1..7888fc439b2f 100644 --- a/drivers/iio/light/Kconfig +++ b/drivers/iio/light/Kconfig @@ -289,6 +289,20 @@ config JSA1212 To compile this driver as a module, choose M here: the module will be called jsa1212. +config ROHM_BU27008 + tristate "ROHM BU27008 color (RGB+C/IR) sensor" + depends on I2C + select REGMAP_I2C + select IIO_GTS_HELPER + help + Enable support for the ROHM BU27008 color sensor. + The ROHM BU27008 is a sensor with 5 photodiodes (red, green, + blue, clear and IR) with four configurable channels. Red and + green being always available and two out of the rest three + (blue, clear, IR) can be selected to be simultaneously measured. + Typical application is adjusting LCD backlight of TVs, + mobile phones and tablet PCs. + config ROHM_BU27034 tristate "ROHM BU27034 ambient light sensor" depends on I2C diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile index 985f6feaccd4..881173952301 100644 --- a/drivers/iio/light/Makefile +++ b/drivers/iio/light/Makefile @@ -39,6 +39,7 @@ obj-$(CONFIG_NOA1305) += noa1305.o obj-$(CONFIG_OPT3001) += opt3001.o obj-$(CONFIG_PA12203001) += pa12203001.o obj-$(CONFIG_ROHM_BU27034) += rohm-bu27034.o +obj-$(CONFIG_ROHM_BU27008) += rohm-bu27008.o obj-$(CONFIG_RPR0521) += rpr0521.o obj-$(CONFIG_SI1133) += si1133.o obj-$(CONFIG_SI1145) += si1145.o diff --git a/drivers/iio/light/rohm-bu27008.c b/drivers/iio/light/rohm-bu27008.c new file mode 100644 index 000000000000..2728e6d5a321 --- /dev/null +++ b/drivers/iio/light/rohm-bu27008.c @@ -0,0 +1,963 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * BU27008 ROHM Colour Sensor + * + * Copyright (c) 2023, ROHM Semiconductor. + */ + +#include <linux/bitfield.h> +#include <linux/bitops.h> +#include <linux/device.h> +#include <linux/i2c.h> +#include <linux/interrupt.h> +#include <linux/module.h> +#include <linux/property.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> +#include <linux/units.h> + +#include <linux/iio/iio.h> +#include <linux/iio/iio-gts-helper.h> +#include <linux/iio/trigger.h> +#include <linux/iio/trigger_consumer.h> +#include <linux/iio/triggered_buffer.h> + +#define BU27008_REG_SYSTEM_CONTROL 0x40 +#define BU27008_MASK_SW_RESET BIT(7) +#define BU27008_MASK_PART_ID GENMASK(5, 0) +#define BU27008_ID 0x1a +#define BU27008_REG_MODE_CONTROL1 0x41 +#define BU27008_MASK_MEAS_MODE GENMASK(2, 0) +#define BU27008_MASK_CHAN_SEL GENMASK(3, 2) + +#define BU27008_REG_MODE_CONTROL2 0x42 +#define BU27008_MASK_RGBC_GAIN GENMASK(7, 3) +#define BU27008_MASK_IR_GAIN_LO GENMASK(2, 0) +#define BU27008_SHIFT_IR_GAIN 3 + +#define BU27008_REG_MODE_CONTROL3 0x43 +#define BU27008_MASK_VALID BIT(7) +#define BU27008_MASK_INT_EN BIT(1) +#define BU27008_INT_EN BU27008_MASK_INT_EN +#define BU27008_INT_DIS 0 +#define BU27008_MASK_MEAS_EN BIT(0) +#define BU27008_MEAS_EN BIT(0) +#define BU27008_MEAS_DIS 0 + +#define BU27008_REG_DATA0_LO 0x50 +#define BU27008_REG_DATA1_LO 0x52 +#define BU27008_REG_DATA2_LO 0x54 +#define BU27008_REG_DATA3_LO 0x56 +#define BU27008_REG_DATA3_HI 0x57 +#define BU27008_REG_MANUFACTURER_ID 0x92 +#define BU27008_REG_MAX BU27008_REG_MANUFACTURER_ID + +enum { + BU27008_RED, /* Always data0 */ + BU27008_GREEN, /* Always data1 */ + BU27008_BLUE, /* data2, configurable (blue / clear) */ + BU27008_CLEAR, /* data2 or data3 */ + BU27008_IR, /* data3 */ + BU27008_NUM_CHANS +}; + +enum { + BU27008_DATA0, /* Always RED */ + BU27008_DATA1, /* Always GREEN */ + BU27008_DATA2, /* Blue or Clear */ + BU27008_DATA3, /* IR or Clear */ + BU27008_NUM_HW_CHANS +}; + +/* We can always measure red and green at same time */ +#define ALWAYS_SCANNABLE (BIT(BU27008_RED) | BIT(BU27008_GREEN)) + +/* We use these data channel configs. Ensure scan_masks below follow them too */ +#define BU27008_BLUE2_CLEAR3 0x0 /* buffer is R, G, B, C */ +#define BU27008_CLEAR2_IR3 0x1 /* buffer is R, G, C, IR */ +#define BU27008_BLUE2_IR3 0x2 /* buffer is R, G, B, IR */ + +static const unsigned long bu27008_scan_masks[] = { + /* buffer is R, G, B, C */ + ALWAYS_SCANNABLE | BIT(BU27008_BLUE) | BIT(BU27008_CLEAR), + /* buffer is R, G, C, IR */ + ALWAYS_SCANNABLE | BIT(BU27008_CLEAR) | BIT(BU27008_IR), + /* buffer is R, G, B, IR */ + ALWAYS_SCANNABLE | BIT(BU27008_BLUE) | BIT(BU27008_IR), + 0 +}; + +/* + * Available scales with gain 1x - 1024x, timings 55, 100, 200, 400 mS + * Time impacts to gain: 1x, 2x, 4x, 8x. + * + * => Max total gain is HWGAIN * gain by integration time (8 * 1024) = 8192 + * + * Max amplification is (HWGAIN * MAX integration-time multiplier) 1024 * 8 + * = 8192. With NANO scale we get rid of accuracy loss when we start with the + * scale 16.0 for HWGAIN1, INT-TIME 55 mS. This way the nano scale for MAX + * total gain 8192 will be 1953125 + */ +#define BU27008_SCALE_1X 16 + +/* See the data sheet for the "Gain Setting" table */ +#define BU27008_GSEL_1X 0x00 +#define BU27008_GSEL_4X 0x08 +#define BU27008_GSEL_8X 0x09 +#define BU27008_GSEL_16X 0x0a +#define BU27008_GSEL_32X 0x0b +#define BU27008_GSEL_64X 0x0c +#define BU27008_GSEL_256X 0x18 +#define BU27008_GSEL_512X 0x19 +#define BU27008_GSEL_1024X 0x1a + +static const struct iio_gain_sel_pair bu27008_gains[] = { + GAIN_SCALE_GAIN(1, BU27008_GSEL_1X), + GAIN_SCALE_GAIN(4, BU27008_GSEL_4X), + GAIN_SCALE_GAIN(8, BU27008_GSEL_8X), + GAIN_SCALE_GAIN(16, BU27008_GSEL_16X), + GAIN_SCALE_GAIN(32, BU27008_GSEL_32X), + GAIN_SCALE_GAIN(64, BU27008_GSEL_64X), + GAIN_SCALE_GAIN(256, BU27008_GSEL_256X), + GAIN_SCALE_GAIN(512, BU27008_GSEL_512X), + GAIN_SCALE_GAIN(1024, BU27008_GSEL_1024X), +}; + +static const struct iio_gain_sel_pair bu27008_gains_ir[] = { + GAIN_SCALE_GAIN(2, BU27008_GSEL_1X), + GAIN_SCALE_GAIN(4, BU27008_GSEL_4X), + GAIN_SCALE_GAIN(8, BU27008_GSEL_8X), + GAIN_SCALE_GAIN(16, BU27008_GSEL_16X), + GAIN_SCALE_GAIN(32, BU27008_GSEL_32X), + GAIN_SCALE_GAIN(64, BU27008_GSEL_64X), + GAIN_SCALE_GAIN(256, BU27008_GSEL_256X), + GAIN_SCALE_GAIN(512, BU27008_GSEL_512X), + GAIN_SCALE_GAIN(1024, BU27008_GSEL_1024X), +}; + +#define BU27008_MEAS_MODE_100MS 0x00 +#define BU27008_MEAS_MODE_55MS 0x01 +#define BU27008_MEAS_MODE_200MS 0x02 +#define BU27008_MEAS_MODE_400MS 0x04 + +static const struct iio_itime_sel_mul bu27008_itimes[] = { + GAIN_SCALE_ITIME_US(400000, BU27008_MEAS_MODE_400MS, 8), + GAIN_SCALE_ITIME_US(200000, BU27008_MEAS_MODE_200MS, 4), + GAIN_SCALE_ITIME_US(100000, BU27008_MEAS_MODE_100MS, 2), + GAIN_SCALE_ITIME_US(55000, BU27008_MEAS_MODE_55MS, 1), +}; + +/* + * All the RGBC channels share the same gain. + * IR gain can be fine-tuned from the gain set for the RGBC by 2 bit, but this + * would yield quite complex gain setting. Especially since not all bit + * compinations are supported. And in any case setting GAIN for RGBC will + * always also change the IR-gain. + * + * On top of this, the selector '0' which corresponds to hw-gain 1X on RGBC, + * corresponds to gain 2X on IR. Rest of the selctors correspond to same gains + * though. This, however, makes it not possible to use shared gain for all + * RGBC and IR settings even though they are all changed at the one go. + */ +#define BU27008_CHAN(color, data, separate_avail) \ +{ \ + .type = IIO_INTENSITY, \ + .modified = 1, \ + .channel2 = IIO_MOD_LIGHT_##color, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ + BIT(IIO_CHAN_INFO_SCALE), \ + .info_mask_separate_available = (separate_avail), \ + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), \ + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME), \ + .address = BU27008_REG_##data##_LO, \ + .scan_index = BU27008_##color, \ + .scan_type = { \ + .sign = 's', \ + .realbits = 16, \ + .storagebits = 16, \ + .endianness = IIO_LE, \ + }, \ +} + +/* For raw reads we always configure DATA3 for CLEAR */ +static const struct iio_chan_spec bu27008_channels[] = { + BU27008_CHAN(RED, DATA0, BIT(IIO_CHAN_INFO_SCALE)), + BU27008_CHAN(GREEN, DATA1, BIT(IIO_CHAN_INFO_SCALE)), + BU27008_CHAN(BLUE, DATA2, BIT(IIO_CHAN_INFO_SCALE)), + BU27008_CHAN(CLEAR, DATA2, BIT(IIO_CHAN_INFO_SCALE)), + /* + * We don't allow setting scale for IR (because of shared gain bits). + * Hence we don't advertise available ones either. + */ + BU27008_CHAN(IR, DATA3, 0), + IIO_CHAN_SOFT_TIMESTAMP(BU27008_NUM_CHANS), +}; + +struct bu27008_data { + struct regmap *regmap; + struct iio_trigger *trig; + struct device *dev; + struct iio_gts gts; + struct iio_gts gts_ir; + int irq; + + /* + * Prevent changing gain/time config when scale is read/written. + * Similarly, protect the integration_time read/change sequence. + * Prevent changing gain/time when data is read. + */ + struct mutex mutex; +}; + +static const struct regmap_range bu27008_volatile_ranges[] = { + { + .range_min = BU27008_REG_SYSTEM_CONTROL, /* SWRESET */ + .range_max = BU27008_REG_SYSTEM_CONTROL, + }, { + .range_min = BU27008_REG_MODE_CONTROL3, /* VALID */ + .range_max = BU27008_REG_MODE_CONTROL3, + }, { + .range_min = BU27008_REG_DATA0_LO, /* DATA */ + .range_max = BU27008_REG_DATA3_HI, + }, +}; + +static const struct regmap_access_table bu27008_volatile_regs = { + .yes_ranges = &bu27008_volatile_ranges[0], + .n_yes_ranges = ARRAY_SIZE(bu27008_volatile_ranges), +}; + +static const struct regmap_range bu27008_read_only_ranges[] = { + { + .range_min = BU27008_REG_DATA0_LO, + .range_max = BU27008_REG_DATA3_HI, + }, { + .range_min = BU27008_REG_MANUFACTURER_ID, + .range_max = BU27008_REG_MANUFACTURER_ID, + } +}; + +static const struct regmap_access_table bu27008_ro_regs = { + .no_ranges = &bu27008_read_only_ranges[0], + .n_no_ranges = ARRAY_SIZE(bu27008_read_only_ranges), +}; + +static const struct regmap_config bu27008_regmap = { + .reg_bits = 8, + .val_bits = 8, + .max_register = BU27008_REG_MAX, + .cache_type = REGCACHE_RBTREE, + .volatile_table = &bu27008_volatile_regs, + .wr_table = &bu27008_ro_regs, +}; + +#define BU27008_MAX_VALID_RESULT_WAIT_US 50000 +#define BU27008_VALID_RESULT_WAIT_QUANTA_US 1000 + +static int bu27008_chan_read_data(struct bu27008_data *data, int reg, int *val) +{ + int ret, valid; + __le16 tmp; + + ret = regmap_read_poll_timeout(data->regmap, BU27008_REG_MODE_CONTROL3, + valid, (valid & BU27008_MASK_VALID), + BU27008_VALID_RESULT_WAIT_QUANTA_US, + BU27008_MAX_VALID_RESULT_WAIT_US); + if (ret) + return ret; + + ret = regmap_bulk_read(data->regmap, reg, &tmp, sizeof(tmp)); + if (ret) + dev_err(data->dev, "Reading channel data failed\n"); + + *val = le16_to_cpu(tmp); + + return ret; +} + +static int bu27008_get_gain(struct bu27008_data *data, struct iio_gts *gts, int *gain) +{ + int ret, sel; + + ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL2, &sel); + if (ret) + return ret; + + sel = FIELD_GET(BU27008_MASK_RGBC_GAIN, sel); + + ret = iio_gts_find_gain_by_sel(gts, sel); + if (ret < 0) { + dev_err(data->dev, "unknown gain value 0x%x\n", sel); + return ret; + } + + *gain = ret; + + return 0; +} + +static int bu27008_write_gain_sel(struct bu27008_data *data, int sel) +{ + int regval; + + regval = FIELD_PREP(BU27008_MASK_RGBC_GAIN, sel); + + /* + * We do always set also the LOW bits of IR-gain because othervice we + * would risk resulting an invalid GAIN register value. + * + * We could allow setting separate gains for RGBC and IR when the + * values were such that HW could support both gain settings. + * Eg, when the shared bits were same for both gain values. + * + * This, however, has a negligible benefit compared to the increased + * software complexity when we would need to go through the gains + * for both channels separately when the integration time changes. + * This would end up with nasty logic for computing gain values for + * both channels - and rejecting them if shared bits changed. + * + * We should then build the logic by guessing what a user prefers. + * RGBC or IR gains correctly set while other jumps to odd value? + * Maybe look-up a value where both gains are somehow optimized + * <what this somehow is, is ATM unknown to us>. Or maybe user would + * expect us to reject changes when optimal gains can't be set to both + * channels w/given integration time. At best that would result + * solution that works well for a very specific subset of + * configurations but causes unexpected corner-cases. + * + * So, we keep it simple. Always set same selector to IR and RGBC. + * We disallow setting IR (as I expect that most of the users are + * interested in RGBC). This way we can show the user that the scales + * for RGBC and IR channels are different (1X Vs 2X with sel 0) while + * still keeping the operation deterministic. + */ + regval |= FIELD_PREP(BU27008_MASK_IR_GAIN_LO, sel); + + return regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL2, + BU27008_MASK_RGBC_GAIN, regval); +} + +static int bu27008_set_gain(struct bu27008_data *data, int gain) +{ + int ret; + + ret = iio_gts_find_sel_by_gain(&data->gts, gain); + if (ret < 0) + return ret; + + return bu27008_write_gain_sel(data, ret); +} + +static int bu27008_get_int_time_sel(struct bu27008_data *data, int *sel) +{ + int ret, val; + + ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL1, &val); + *sel = FIELD_GET(BU27008_MASK_MEAS_MODE, val); + + return ret; +} + +static int bu27008_set_int_time_sel(struct bu27008_data *data, int sel) +{ + return regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL1, + BU27008_MASK_MEAS_MODE, sel); +} + +static int bu27008_get_int_time_us(struct bu27008_data *data) +{ + int ret, sel; + + ret = bu27008_get_int_time_sel(data, &sel); + if (ret) + return ret; + + return iio_gts_find_int_time_by_sel(&data->gts, + sel & BU27008_MASK_MEAS_MODE); +} + +static int _bu27008_get_scale(struct bu27008_data *data, bool ir, int *val, + int *val2) +{ + struct iio_gts *gts; + int gain, ret; + + if (ir) + gts = &data->gts_ir; + else + gts = &data->gts; + + ret = bu27008_get_gain(data, gts, &gain); + if (ret) + return ret; + + ret = bu27008_get_int_time_us(data); + if (ret < 0) + return ret; + + return iio_gts_get_scale(gts, gain, ret, val, val2); +} + +static int bu27008_get_scale(struct bu27008_data *data, bool ir, int *val, + int *val2) +{ + int ret; + + mutex_lock(&data->mutex); + ret = _bu27008_get_scale(data, ir, val, val2); + mutex_unlock(&data->mutex); + + return ret; +} + +static int bu27008_set_int_time(struct bu27008_data *data, int time) +{ + int ret; + + ret = iio_gts_find_sel_by_int_time(&data->gts, time); + if (ret < 0) + return ret; + + return regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL1, + BU27008_MASK_MEAS_MODE, ret); +} + +/* Try to change the time so that the scale is maintained */ +static int bu27008_try_set_int_time(struct bu27008_data *data, int int_time_new) +{ + int ret, old_time_sel, new_time_sel, old_gain, new_gain; + + mutex_lock(&data->mutex); + + ret = bu27008_get_int_time_sel(data, &old_time_sel); + if (ret < 0) + goto unlock_out; + + if (!iio_gts_valid_time(&data->gts, int_time_new)) { + dev_dbg(data->dev, "Unsupported integration time %u\n", + int_time_new); + + ret = -EINVAL; + goto unlock_out; + } + + /* If we already use requested time, then we're done */ + new_time_sel = iio_gts_find_sel_by_int_time(&data->gts, int_time_new); + if (new_time_sel == old_time_sel) + goto unlock_out; + + ret = bu27008_get_gain(data, &data->gts, &old_gain); + if (ret) + goto unlock_out; + + ret = iio_gts_find_new_gain_sel_by_old_gain_time(&data->gts, old_gain, + old_time_sel, new_time_sel, &new_gain); + if (ret) { + int scale1, scale2; + bool ok; + + _bu27008_get_scale(data, false, &scale1, &scale2); + dev_dbg(data->dev, + "Can't support time %u with current scale %u %u\n", + int_time_new, scale1, scale2); + + if (new_gain < 0) + goto unlock_out; + + /* + * If caller requests for integration time change and we + * can't support the scale - then the caller should be + * prepared to 'pick up the pieces and deal with the + * fact that the scale changed'. + */ + ret = iio_find_closest_gain_low(&data->gts, new_gain, &ok); + if (!ok) + dev_dbg(data->dev, "optimal gain out of range\n"); + + if (ret < 0) { + dev_dbg(data->dev, + "Total gain increase. Risk of saturation"); + ret = iio_gts_get_min_gain(&data->gts); + if (ret < 0) + goto unlock_out; + } + new_gain = ret; + dev_dbg(data->dev, "scale changed, new gain %u\n", new_gain); + } + + ret = bu27008_set_gain(data, new_gain); + if (ret) + goto unlock_out; + + ret = bu27008_set_int_time(data, int_time_new); + +unlock_out: + mutex_unlock(&data->mutex); + + return ret; +} + +static int bu27008_meas_set(struct bu27008_data *data, int state) +{ + return regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL3, + BU27008_MASK_MEAS_EN, state); +} + +static int bu27008_chan_cfg(struct bu27008_data *data, + struct iio_chan_spec const *chan) +{ + int chan_sel; + + if (chan->scan_index == BU27008_BLUE) + chan_sel = BU27008_BLUE2_CLEAR3; + else + chan_sel = BU27008_CLEAR2_IR3; + + chan_sel = FIELD_PREP(BU27008_MASK_CHAN_SEL, chan_sel); + + return regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL3, + BU27008_MASK_CHAN_SEL, chan_sel); +} + +static int bu27008_read_one(struct bu27008_data *data, struct iio_dev *idev, + struct iio_chan_spec const *chan, int *val, int *val2) +{ + int ret, int_time; + + ret = bu27008_chan_cfg(data, chan); + if (ret) + return ret; + + ret = bu27008_meas_set(data, BU27008_MEAS_EN); + if (ret) + return ret; + + int_time = bu27008_get_int_time_us(data); + if (int_time < 0) + int_time = 400000; + + msleep(int_time / 1000); + + ret = bu27008_chan_read_data(data, chan->address, val); + if (!ret) + ret = IIO_VAL_INT; + + if (bu27008_meas_set(data, BU27008_MEAS_DIS)) + dev_warn(data->dev, "measurement disabling failed\n"); + + return ret; +} + +static int bu27008_read_raw(struct iio_dev *idev, + struct iio_chan_spec const *chan, + int *val, int *val2, long mask) +{ + struct bu27008_data *data = iio_priv(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); + mutex_unlock(&data->mutex); + + iio_device_release_direct_mode(idev); + + return ret; + } + case IIO_CHAN_INFO_SCALE: + ret = bu27008_get_scale(data, chan->scan_index == BU27008_IR, + val, val2); + if (ret) + return ret; + + return IIO_VAL_INT_PLUS_NANO; + + case IIO_CHAN_INFO_INT_TIME: + ret = bu27008_get_int_time_us(data); + if (ret < 0) + return ret; + + *val = 0; + *val2 = ret; + + return IIO_VAL_INT_PLUS_MICRO; + + default: + return -EINVAL; + } +} + +static int bu27008_try_find_new_time_gain(struct bu27008_data *data, int val, + int val2, int *gain_sel) +{ + /* Could not support new scale with existing int-time */ + int i, ret, new_time_sel; + + for (i = 0; i < data->gts.num_itime; i++) { + new_time_sel = data->gts.itime_table[i].sel; + ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts, + new_time_sel, val, val2 * 1000, gain_sel); + if (!ret) + break; + } + if (i == data->gts.num_itime) { + dev_err(data->dev, "Can't support scale %u %u\n", val, + val2); + + return -EINVAL; + } + + return bu27008_set_int_time_sel(data, new_time_sel); +} + +static int bu27008_set_scale(struct bu27008_data *data, + struct iio_chan_spec const *chan, + int val, int val2) +{ + int ret, gain_sel, time_sel; + + if (chan->scan_index == BU27008_IR) + return -EINVAL; + + mutex_lock(&data->mutex); + + ret = bu27008_get_int_time_sel(data, &time_sel); + if (ret < 0) + goto unlock_out; + + ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts, time_sel, + val, val2 * 1000, &gain_sel); + if (ret) + ret = bu27008_try_find_new_time_gain(data, val, val2, &gain_sel); + + if (!ret) + ret = bu27008_write_gain_sel(data, gain_sel); + +unlock_out: + mutex_unlock(&data->mutex); + + return ret; +} + +static int bu27008_write_raw(struct iio_dev *idev, + struct iio_chan_spec const *chan, + int val, int val2, long mask) +{ + struct bu27008_data *data = iio_priv(idev); + int ret; + + /* + * We should not allow changing scale when measurement is ongoing. + * This could make values in buffer inconsistent. + */ + ret = iio_device_claim_direct_mode(idev); + if (ret) + return ret; + + switch (mask) { + case IIO_CHAN_INFO_SCALE: + ret = bu27008_set_scale(data, chan, val, val2); + break; + case IIO_CHAN_INFO_INT_TIME: + if (val) + ret = -EINVAL; + else + ret = bu27008_try_set_int_time(data, val2); + break; + default: + ret = -EINVAL; + break; + } + iio_device_release_direct_mode(idev); + + return ret; +} + +static int bu27008_read_avail(struct iio_dev *idev, + struct iio_chan_spec const *chan, const int **vals, + int *type, int *length, long mask) +{ + struct bu27008_data *data = iio_priv(idev); + + switch (mask) { + case IIO_CHAN_INFO_INT_TIME: + return iio_gts_avail_times(&data->gts, vals, type, length); + case IIO_CHAN_INFO_SCALE: + if (chan->channel2 == IIO_MOD_LIGHT_IR) + return iio_gts_all_avail_scales(&data->gts_ir, vals, + type, length); + return iio_gts_all_avail_scales(&data->gts, vals, type, length); + default: + return -EINVAL; + } +} + +static const struct iio_info bu27008_info = { + .read_raw = &bu27008_read_raw, + .write_raw = &bu27008_write_raw, + .read_avail = &bu27008_read_avail, + .validate_trigger = iio_validate_own_trigger, +}; + +static int bu27008_chip_init(struct bu27008_data *data) +{ + int ret; + + ret = regmap_update_bits(data->regmap, BU27008_REG_SYSTEM_CONTROL, + BU27008_MASK_SW_RESET, BU27008_MASK_SW_RESET); + if (ret) + return dev_err_probe(data->dev, ret, "Sensor reset failed\n"); + + /* + * The data-sheet does not tell how long performing the IC reset takes. + * However, the data-sheet says the minimum time it takes the IC to be + * able to take inputs after power is applied, is 100 uS. I'd assume + * > 1 mS is enough. + */ + msleep(1); + + return ret; +} + +static int bu27008_set_drdy_irq(struct bu27008_data *data, int state) +{ + return regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL3, + BU27008_MASK_INT_EN, state); +} + +static int bu27008_trigger_set_state(struct iio_trigger *trig, + bool state) +{ + struct bu27008_data *data = iio_trigger_get_drvdata(trig); + int ret = 0; + + if (state) + ret = bu27008_set_drdy_irq(data, BU27008_INT_EN); + else + ret = bu27008_set_drdy_irq(data, BU27008_INT_DIS); + if (ret) + dev_err(data->dev, "Failed to set trigger state\n"); + + return ret; +} + +static const struct iio_trigger_ops bu27008_trigger_ops = { + .set_trigger_state = bu27008_trigger_set_state, +}; + +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; + int ret, dummy; + + memset(&raw, 0, sizeof(raw)); + + /* + * After some measurements, it seems reading the + * BU27008_REG_MODE_CONTROL3 debounces the IRQ line + */ + ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL3, &dummy); + if (ret < 0) + goto err_read; + + ret = regmap_bulk_read(data->regmap, BU27008_REG_DATA0_LO, &raw.chan, + sizeof(raw.chan)); + if (ret < 0) + goto err_read; + + iio_push_to_buffers_with_timestamp(idev, &raw, pf->timestamp); +err_read: + iio_trigger_notify_done(idev->trig); + + return IRQ_HANDLED; +} + +static irqreturn_t bu27008_irq_thread_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); + + iio_trigger_poll_nested(data->trig); + + return IRQ_HANDLED; +} + +static int bu27008_buffer_preenable(struct iio_dev *idev) +{ + struct bu27008_data *data = iio_priv(idev); + int chan_sel, ret; + + /* Configure channel selection */ + if (test_bit(BU27008_BLUE, idev->active_scan_mask)) { + if (test_bit(BU27008_CLEAR, idev->active_scan_mask)) + chan_sel = BU27008_BLUE2_CLEAR3; + else + chan_sel = BU27008_BLUE2_IR3; + } else { + chan_sel = BU27008_CLEAR2_IR3; + } + + chan_sel = FIELD_PREP(BU27008_MASK_CHAN_SEL, chan_sel); + + ret = regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL3, + BU27008_MASK_CHAN_SEL, chan_sel); + if (ret) + return ret; + + return bu27008_meas_set(data, BU27008_MEAS_EN); +} + +static int bu27008_buffer_postdisable(struct iio_dev *idev) +{ + struct bu27008_data *data = iio_priv(idev); + + return bu27008_meas_set(data, BU27008_MEAS_DIS); +} + +static const struct iio_buffer_setup_ops bu27008_buffer_ops = { + .preenable = bu27008_buffer_preenable, + .postdisable = bu27008_buffer_postdisable, +}; + +static int bu27008_probe(struct i2c_client *i2c) +{ + struct device *dev = &i2c->dev; + struct iio_trigger *itrig; + struct bu27008_data *data; + struct regmap *regmap; + unsigned int part_id, reg; + struct iio_dev *idev; + char *name; + int ret; + + regmap = devm_regmap_init_i2c(i2c, &bu27008_regmap); + if (IS_ERR(regmap)) + return dev_err_probe(dev, PTR_ERR(regmap), + "Failed to initialize Regmap\n"); + + idev = devm_iio_device_alloc(dev, sizeof(*data)); + if (!idev) + return -ENOMEM; + + ret = devm_regulator_get_enable(dev, "vdd"); + if (ret) + return dev_err_probe(dev, ret, "Failed to get regulator\n"); + + data = iio_priv(idev); + + ret = regmap_read(regmap, BU27008_REG_SYSTEM_CONTROL, ®); + if (ret) + return dev_err_probe(dev, ret, "Failed to access sensor\n"); + + part_id = FIELD_GET(BU27008_MASK_PART_ID, reg); + + if (part_id != BU27008_ID) + dev_warn(dev, "unknown device 0x%x\n", part_id); + + ret = devm_iio_init_iio_gts(dev, BU27008_SCALE_1X, 0, bu27008_gains, + ARRAY_SIZE(bu27008_gains), bu27008_itimes, + ARRAY_SIZE(bu27008_itimes), &data->gts); + if (ret) + return ret; + + ret = devm_iio_init_iio_gts(dev, BU27008_SCALE_1X, 0, bu27008_gains_ir, + ARRAY_SIZE(bu27008_gains_ir), bu27008_itimes, + ARRAY_SIZE(bu27008_itimes), &data->gts_ir); + if (ret) + return ret; + + mutex_init(&data->mutex); + data->regmap = regmap; + data->dev = dev; + data->irq = i2c->irq; + + idev->channels = bu27008_channels; + idev->num_channels = ARRAY_SIZE(bu27008_channels); + idev->name = "bu27008"; + idev->info = &bu27008_info; + idev->modes = INDIO_DIRECT_MODE; + idev->available_scan_masks = bu27008_scan_masks; + + ret = bu27008_chip_init(data); + if (ret) + return ret; + + if (i2c->irq) { + ret = devm_iio_triggered_buffer_setup(dev, idev, + &iio_pollfunc_store_time, + bu27008_trigger_handler, + &bu27008_buffer_ops); + if (ret) + return dev_err_probe(dev, ret, + "iio_triggered_buffer_setup_ext FAIL\n"); + + itrig = devm_iio_trigger_alloc(dev, "%sdata-rdy-dev%d", + idev->name, iio_device_id(idev)); + if (!itrig) + return -ENOMEM; + + data->trig = itrig; + + itrig->ops = &bu27008_trigger_ops; + iio_trigger_set_drvdata(itrig, data); + + name = devm_kasprintf(dev, GFP_KERNEL, "%s-bu27008", + dev_name(dev)); + + ret = devm_request_threaded_irq(dev, i2c->irq, + iio_pollfunc_store_time, + &bu27008_irq_thread_handler, + IRQF_ONESHOT, name, idev->pollfunc); + if (ret) + return dev_err_probe(dev, ret, + "Could not request IRQ\n"); + + + ret = devm_iio_trigger_register(dev, itrig); + if (ret) + return dev_err_probe(dev, ret, + "Trigger registration failed\n"); + } else { + dev_warn(dev, "No IRQ configured\n"); + } + + ret = devm_iio_device_register(dev, idev); + if (ret) + return dev_err_probe(dev, ret, + "Unable to register iio device\n"); + + return ret; +} + +static const struct of_device_id bu27008_of_match[] = { + { .compatible = "rohm,bu27008" }, + { } +}; +MODULE_DEVICE_TABLE(of, bu27008_of_match); + +static struct i2c_driver bu27008_i2c_driver = { + .driver = { + .name = "bu27008", + .of_match_table = bu27008_of_match, + }, + .probe_new = bu27008_probe, +}; +module_i2c_driver(bu27008_i2c_driver); + +MODULE_DESCRIPTION("ROHM BU27008 colour sensor driver"); +MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>"); +MODULE_LICENSE("GPL"); +MODULE_IMPORT_NS(IIO_GTS_HELPER);
The ROHM BU27008 is a sensor with 5 photodiodes (red, green, blue, clear and IR) with four configurable channels. Red and green being always available and two out of the rest three (blue, clear, IR) can be selected to be simultaneously measured. Typical application is adjusting LCD backlight of TVs, mobile phones and tablet PCs. Add initial support for the ROHM BU27008 color sensor. - raw_read() of RGB and clear channels - triggered buffer w/ DRDY interrtupt Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com> --- Revision history v2 => v3: - drop bits.h - drop unnecessary comma after compatible - Styling / cleaning - Simplify sleep time computation - rename bu27008_get_int_time() => bu27008_get_int_time_us() v1 => v2: - Fix buffered data demuxing - Use generic trigger functions instead of rolling own ones - Drop unnecessary locking - Use generic iio_validate_own_trigger() - Some other more trivial fixes for review comments - use defines for [enable/disable] [measurement/data-ready IRQ] reg values and use regmap_update_bits() directly instead of regamap_[set/clear]_bits() --- drivers/iio/light/Kconfig | 14 + drivers/iio/light/Makefile | 1 + drivers/iio/light/rohm-bu27008.c | 963 +++++++++++++++++++++++++++++++ 3 files changed, 978 insertions(+) create mode 100644 drivers/iio/light/rohm-bu27008.c