[v4,4/5] iio: light: ROHM BU27008 color sensor
Commit Message
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
v3 => v4:
- use regmap_write_bits() for SWRESET
- reorder Makefile entries
- styling + impoved comments
- drop bu27008_irq_thread_handler() and call iio_trigger_poll() from IRQ
top half
- use devm_request_irq() instead of devm_request_threaded_irq()
- Explicitly disable the IRQ from top-half instead of thread + IRQF_ONESHOT
(I hope this is now implemented in a way it should)
- drop print severity
- reinit regmap cache after reset
- avoid unnecessary division when waiting measurement to complete
- use kernel-doc for enums
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 | 993 +++++++++++++++++++++++++++++++
3 files changed, 1008 insertions(+)
create mode 100644 drivers/iio/light/rohm-bu27008.c
Comments
On Wed, May 03, 2023 at 12:50:14PM +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
...
> +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.
Module name?
...
> +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,
> + }
+ trailing comma for consistency?
> +};
...
> +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,
Do you need regmap lock? If so, why (since you have mutex)?
> +};
...
> +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 = BU27008_MEAS_TIME_MAX_MS;
> + else
> + int_time /= USEC_PER_MSEC;
The above function returns an error code when negative, so I would rather see
ret = bu27008_get_int_time_us(data);
if (ret < 0)
int_time = BU27008_MEAS_TIME_MAX_MS;
else
int_time = ret / USEC_PER_MSEC;
at least this explicitly shows the semantics of the "negative" time.
> + msleep(int_time);
> +
> + 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;
> +}
...
> + ret = regmap_reinit_cache(data->regmap, &bu27008_regmap);
> + if (ret) {
> + dev_err(data->dev, "Failed to reinit reg cache\n");
> + return ret;
Dup is not needed.
> + }
> +
> + return ret;
...
> + if (i2c->irq) {
Instead of a long body, I would rather see a call to
ret = ..._setup_irq();
if (ret)
return ret;
> + 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_irq(dev, i2c->irq,
> + &bu27008_data_rdy_poll,
> + 0, name, itrig);
> + 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");
> +
> + /* set default trigger */
> + idev->trig = iio_trigger_get(itrig);
> + } else {
> + dev_info(dev, "No IRQ, buffered mode disabled\n");
> + }
On 5/4/23 17:33, Andy Shevchenko wrote:
> On Wed, May 03, 2023 at 12:50:14PM +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
>
> ...
>
>> +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.
>
> Module name?
We have discussed this several times already.
https://lore.kernel.org/all/10c4663b-dd65-a545-786d-10aed6e6e5e9@fi.rohmeurope.com/
Module name is completely irrelevant when selecting a kernel configuration.
>
> ...
>
>> +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,
>
>> + }
>
> + trailing comma for consistency?
Thanks.
>> +};
>
> ...
>
>> +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,
>
> Do you need regmap lock? If so, why (since you have mutex)?
I believe you know that regmap uses a default lock when no external lock
is given. So, I assume you mean that maybe we could set
'disable_locking' for the regmap here.
It's nice to be occasionally pushed to think "out of the box". And yes,
disabling regmap lock is really out of my "normal box" :)
I didn't go through all of the code yet, but I think pretty much all of
the sequences which end up to register writes are indeed protected by
the mutex. (Well, probe is not but it is expected to only update one bit
while rest of the register should stay fixed).
It may be we could live without regmap_lock when driver is in it's
current state, but I am not convinced the performance improvement is
worth the risk. Having regmap unprotected is not common, and it is also
not easy to spot when making changes to the driver. In my opinion it is
a bit like asking for a nose-bleed unless there is really heavy reasons
to drop the lock... In this case, having the regmap_lock (which is
pretty much never locked because we have the mutex as you said) is
probably not a penalty that matters.
>
>> +};
>
> ...
>
>> +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 = BU27008_MEAS_TIME_MAX_MS;
>> + else
>> + int_time /= USEC_PER_MSEC;
>
> The above function returns an error code when negative, so I would rather see
>
> ret = bu27008_get_int_time_us(data);
> if (ret < 0)
> int_time = BU27008_MEAS_TIME_MAX_MS;
> else
> int_time = ret / USEC_PER_MSEC;
>
> at least this explicitly shows the semantics of the "negative" time.
Ok.
>
>> + msleep(int_time);
>> +
>> + 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;
>> +}
>
> ...
>
>> + ret = regmap_reinit_cache(data->regmap, &bu27008_regmap);
>> + if (ret) {
>> + dev_err(data->dev, "Failed to reinit reg cache\n");
>
>> + return ret;
>
> Dup is not needed.
Thanks.
>
>> + }
>> +
>> + return ret;
>
> ...
>
>> + if (i2c->irq) {
>
> Instead of a long body, I would rather see a call to
Could make sense indeed. I'll see how it would look like, thanks.
>
> ret = ..._setup_irq();
> if (ret)
> return ret;
>
>> + 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_irq(dev, i2c->irq,
>> + &bu27008_data_rdy_poll,
>> + 0, name, itrig);
>> + 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");
>> +
>> + /* set default trigger */
>> + idev->trig = iio_trigger_get(itrig);
>> + } else {
>> + dev_info(dev, "No IRQ, buffered mode disabled\n");
>> + }
>
>
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On Wed, 3 May 2023 12:50:14 +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>
>
Mostly stuff that you asked about in response to earlier version but
which I hadn't replied to until today.
Upshot, don't need the manual irq handling in here.
Whilst you aren't setting IRQF_ONESHOT for the pollfunc side of the trigger
(the downstream IRQ / IRQ thread) the IIO utility functions are.
Jonathan
> diff --git a/drivers/iio/light/rohm-bu27008.c b/drivers/iio/light/rohm-bu27008.c
> new file mode 100644
> index 000000000000..c04d845062ba
> --- /dev/null
> +++ b/drivers/iio/light/rohm-bu27008.c
> @@ -0,0 +1,993 @@
...
> +
> +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;
> + }
Why {}?
I guess there was a local variable, but isn't any more?
> + 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_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;
> +
> + /*
> + * Do not allow changing scale when measurement is ongoing as doing so
> + * could make values in the 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;
Slight preference for
if (val) {
ret = -EINVAL;
break;
}
ret = bu27008....
break;
so that the error path is out of line of the main flow.
Ever so slightly helps people follow this stuff if they are reading 'a lot'
of code (every little helps!). I have to look closer at anything other than
an obvious direct break / goto / return on error paths to make sure nothing
else snuck in that shouldn't be there.
> + 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_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);
> +
> + enable_irq(data->irq);
As below. This shouldn't be needed (and if it was it should be in the
reenable path that is ultimately a result of that notify_done above and
some reference counting fun).
> +
> + 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;
Hmm. I'd missed this before but. This is in the wrong place really
(though it probably doesn't make much difference), stuff related to
enabling particular channels should be in iio_info->update_scan_mode()
It's arguable that the actual measurement mode setting might come
in the postenable callback (after the update_scan_mode() call which
in turn follows the preenable callback).
All these callbacks have become a bit blurry over time as we end
up with devices that need to do nasty thing in one place. In this
particular case it's pretty simple though, so nicer to move
the scan mask stuff to the callback that is given the active_scan
mask as a parameter.
> +
> + 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 irqreturn_t bu27008_data_rdy_poll(int irq, void *private)
> +{
> + /*
> + * The BU27008 keeps IRQ asserted until we read the VALID bit from
> + * a register. We need to keep the IRQ disabled until this
> + */
> + disable_irq_nosync(irq);
As per my late reply to your question on this, shouldn't be needed
as IRQF_ONESHOT is ultimately set for the interrupts nested below this
so they'll get the resulting queuing on the threads which is fine.
Note if this were needed, you might want to provide the reenable callback
for the trigger to do the irq reenabling and to implement that correctly
against some race conditions, that might need to do another read of the
register. That handles weird ordering conditions where another consumer
of the trigger is enabled before the device acting as a consumer has
it's own buffer enabled.
Anyhow, I don't think it's relevant here and you shouldn't need this unless
I've miss understood / remembered how all that interrupt logic is working.
Jonathan
> + iio_trigger_poll(private);
> +
> + return IRQ_HANDLED;
> +}
Hi Jonathan,
On 5/7/23 17:54, Jonathan Cameron wrote:
> On Wed, 3 May 2023 12:50:14 +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>
>>
> Mostly stuff that you asked about in response to earlier version but
> which I hadn't replied to until today.
>
> Upshot, don't need the manual irq handling in here.
>
> Whilst you aren't setting IRQF_ONESHOT for the pollfunc side of the trigger
> (the downstream IRQ / IRQ thread) the IIO utility functions are.
I tried doing:
static int bu27008_setup_trigger(struct bu27008_data *data, struct
iio_dev *idev)
{
struct iio_trigger *itrig;
char *name;
int ret;
ret = devm_iio_triggered_buffer_setup(data->dev, idev,
&iio_pollfunc_store_time,
bu27008_trigger_handler,
&bu27008_buffer_ops);
if (ret)
return dev_err_probe(data->dev, ret,
"iio_triggered_buffer_setup_ext FAIL\n");
itrig = devm_iio_trigger_alloc(data->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(data->dev, GFP_KERNEL, "%s-bu27008",
dev_name(data->dev));
ret = devm_request_irq(data->dev, data->irq,
/* No IRQ disabling */
&iio_trigger_generic_data_rdy_poll,
0, name, itrig);
if (ret)
return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
ret = devm_iio_trigger_register(data->dev, itrig);
if (ret)
return dev_err_probe(data->dev, ret,
"Trigger registration failed\n");
/* set default trigger */
idev->trig = iio_trigger_get(itrig);
return 0;
}
It seems to me we get IRQ storm out of it, bu27008_trigger_handler never
being called. My assumption is that as soon as the IRQ handling code
exits the iio_trigger_generic_data_rdy_poll, it re-enables the IRQ - and
because we have level active IRQ and because the
bu27008_trigger_handler() has not yet had a chance to read the VALID bit
which restores the IRQ-line - we will immediately enter back to the IRQ
handling.
This problem does not occur when I use bu27008_data_rdy_poll() (which is
the same but disables the IRQ) instead of
iio_trigger_generic_data_rdy_poll(), and re-enable the IRQ only after
the handler bu27008_trigger_handler() has restored the IRQ line.
Does the sequence above (bu27008_setup_trigger()) look sane?
>
>
>> +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);
>> +
>> + enable_irq(data->irq);
>
> As below. This shouldn't be needed (and if it was it should be in the
> reenable path that is ultimately a result of that notify_done above and
> some reference counting fun).
I will see the reenable callback, thanks!
>
>> +
>> + 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;
>
> Hmm. I'd missed this before but. This is in the wrong place really
> (though it probably doesn't make much difference), stuff related to
> enabling particular channels should be in iio_info->update_scan_mode()
Oh. I'll check this out as well.
>
> It's arguable that the actual measurement mode setting might come
> in the postenable callback (after the update_scan_mode() call which
> in turn follows the preenable callback).
>
> All these callbacks have become a bit blurry over time as we end
> up with devices that need to do nasty thing in one place. In this
> particular case it's pretty simple though, so nicer to move
> the scan mask stuff to the callback that is given the active_scan
> mask as a parameter.
>
>> +
>> + 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 irqreturn_t bu27008_data_rdy_poll(int irq, void *private)
>> +{
>> + /*
>> + * The BU27008 keeps IRQ asserted until we read the VALID bit from
>> + * a register. We need to keep the IRQ disabled until this
>> + */
>> + disable_irq_nosync(irq);
>
> As per my late reply to your question on this, shouldn't be needed
> as IRQF_ONESHOT is ultimately set for the interrupts nested below this
> so they'll get the resulting queuing on the threads which is fine.
I see an IRQ storm if I omit this. The threaded trigger handler which
'ACKs' the IRQ gets never ran. I'll see the reenable though! Thanks!
Yours,
-- Matti
On Fri, May 05, 2023 at 04:56:47AM +0000, Vaittinen, Matti wrote:
> On 5/4/23 17:33, Andy Shevchenko wrote:
> > On Wed, May 03, 2023 at 12:50:14PM +0300, Matti Vaittinen wrote:
...
> >> +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.
> >
> > Module name?
>
> We have discussed this several times already.
>
> https://lore.kernel.org/all/10c4663b-dd65-a545-786d-10aed6e6e5e9@fi.rohmeurope.com/
>
> Module name is completely irrelevant when selecting a kernel configuration.
This option is also selectable by user.
...
> > Do you need regmap lock? If so, why (since you have mutex)?
>
> I believe you know that regmap uses a default lock when no external lock
> is given. So, I assume you mean that maybe we could set
> 'disable_locking' for the regmap here.
Correct.
> It's nice to be occasionally pushed to think "out of the box". And yes,
> disabling regmap lock is really out of my "normal box" :)
>
> I didn't go through all of the code yet, but I think pretty much all of
> the sequences which end up to register writes are indeed protected by
> the mutex. (Well, probe is not but it is expected to only update one bit
> while rest of the register should stay fixed).
>
> It may be we could live without regmap_lock when driver is in it's
> current state, but I am not convinced the performance improvement is
> worth the risk. Having regmap unprotected is not common, and it is also
> not easy to spot when making changes to the driver. In my opinion it is
> a bit like asking for a nose-bleed unless there is really heavy reasons
> to drop the lock... In this case, having the regmap_lock (which is
> pretty much never locked because we have the mutex as you said) is
> probably not a penalty that matters.
Basically you try to justify a hidden mine field in case somebody will think
"oh, we are protected by regmap lock, so why to bother call mutex_lock()" and
at the end it become a subtle bugs in the code. With disable_locking = true
I can see that code author _carefully thought through_ the locking schema and
understands the hardware and the code.
P.S. I'm wondering why your lines of text have a single trailing whitespace
but the last line.
Hi Andy,
On 5/8/23 15:23, Andy Shevchenko wrote:
> On Fri, May 05, 2023 at 04:56:47AM +0000, Vaittinen, Matti wrote:
>> On 5/4/23 17:33, Andy Shevchenko wrote:
>>> On Wed, May 03, 2023 at 12:50:14PM +0300, Matti Vaittinen wrote:
>
> ...
>
>>>> +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.
>>>
>>> Module name?
>>
>> We have discussed this several times already.
>>
>> https://lore.kernel.org/all/10c4663b-dd65-a545-786d-10aed6e6e5e9@fi.rohmeurope.com/
>>
>> Module name is completely irrelevant when selecting a kernel configuration.
>
> This option is also selectable by user.
I don't think the name is selectable. Yes, user selects whether to
compile driver as a module or in-kernel - but the module name is
completely irrelevant what comes to this decision.
> ...
>
>>> Do you need regmap lock? If so, why (since you have mutex)?
>>
>> I believe you know that regmap uses a default lock when no external lock
>> is given. So, I assume you mean that maybe we could set
>> 'disable_locking' for the regmap here.
>
> Correct.
>
>> It's nice to be occasionally pushed to think "out of the box". And yes,
>> disabling regmap lock is really out of my "normal box" :)
>>
>> I didn't go through all of the code yet, but I think pretty much all of
>> the sequences which end up to register writes are indeed protected by
>> the mutex. (Well, probe is not but it is expected to only update one bit
>> while rest of the register should stay fixed).
>>
>> It may be we could live without regmap_lock when driver is in it's
>> current state, but I am not convinced the performance improvement is
>> worth the risk. Having regmap unprotected is not common, and it is also
>> not easy to spot when making changes to the driver. In my opinion it is
>> a bit like asking for a nose-bleed unless there is really heavy reasons
>> to drop the lock... In this case, having the regmap_lock (which is
>> pretty much never locked because we have the mutex as you said) is
>> probably not a penalty that matters.
>
> Basically you try to justify a hidden mine field in case somebody will think
> "oh, we are protected by regmap lock, so why to bother call mutex_lock()" and
> at the end it become a subtle bugs in the code. With disable_locking = true
> I can see that code author _carefully thought through_ the locking schema and
> understands the hardware and the code.
I added the disable_locking = true in v5 - but I am not convinced that
was a great idea. I am afraid disabling regmap lock is the hidden
minefield for average users. I didn't grep the kernel for it but I am
afraid the percentage of regmap users who disable locking is very low.
Thus, I'd say this is unexpected to many and may lead to bugs although I
try to watch out for them. Well, time will tell.
> P.S. I'm wondering why your lines of text have a single trailing whitespace
> but the last line.
I guess it must be Thunderbird client then. Well, at least it can send
out plain-text decently well while working with the exchange servers
used by ROHM as well as with the gmail. I am not super happy with
Thunderbird, it tends to eat way more resources I wished it did, but it
is a working compromise for me. I am interested in hearing if anyone
knows a way to configure the Thunderbird to drop these extra spaces.
Yours,
-- Matti
On Mon, 8 May 2023 09:32:28 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> Hi Jonathan,
>
> On 5/7/23 17:54, Jonathan Cameron wrote:
> > On Wed, 3 May 2023 12:50:14 +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>
> >>
> > Mostly stuff that you asked about in response to earlier version but
> > which I hadn't replied to until today.
> >
> > Upshot, don't need the manual irq handling in here.
> >
> > Whilst you aren't setting IRQF_ONESHOT for the pollfunc side of the trigger
> > (the downstream IRQ / IRQ thread) the IIO utility functions are.
>
> I tried doing:
>
> static int bu27008_setup_trigger(struct bu27008_data *data, struct
> iio_dev *idev)
> {
> struct iio_trigger *itrig;
> char *name;
> int ret;
>
> ret = devm_iio_triggered_buffer_setup(data->dev, idev,
> &iio_pollfunc_store_time,
> bu27008_trigger_handler,
> &bu27008_buffer_ops);
> if (ret)
> return dev_err_probe(data->dev, ret,
> "iio_triggered_buffer_setup_ext FAIL\n");
>
> itrig = devm_iio_trigger_alloc(data->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(data->dev, GFP_KERNEL, "%s-bu27008",
> dev_name(data->dev));
>
> ret = devm_request_irq(data->dev, data->irq,
> /* No IRQ disabling */
> &iio_trigger_generic_data_rdy_poll,
> 0, name, itrig);
> if (ret)
> return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
>
> ret = devm_iio_trigger_register(data->dev, itrig);
> if (ret)
> return dev_err_probe(data->dev, ret,
> "Trigger registration failed\n");
>
> /* set default trigger */
> idev->trig = iio_trigger_get(itrig);
>
> return 0;
> }
>
> It seems to me we get IRQ storm out of it, bu27008_trigger_handler never
> being called. My assumption is that as soon as the IRQ handling code
> exits the iio_trigger_generic_data_rdy_poll, it re-enables the IRQ - and
> because we have level active IRQ and because the
> bu27008_trigger_handler() has not yet had a chance to read the VALID bit
> which restores the IRQ-line - we will immediately enter back to the IRQ
> handling.
Ah. I'd miss understood what was going on here. I thought we were talking
race conditions only - not a level interrupt. Sorry for confusion / being
half asleep. If it has an Ack like this I'd argue this is really an edge
interrupt but that would require a guaranteed drop in the signal.
I am assuming the sensor merrily carries on grabbing data, whether or
not anyone reads it and so if we treated this as an edge interrupt then
the clear to set cycle could be very short (and hence not detected).
If it instead doesn't read new data until previous has been read, then things
are much simpler.
Hmm. How to make this work cleanly assuming it's case 1. It might be that your
current approach is the best though it would be nice to do something in the
IIO code (with risk of breaking everyone :() I don't think we can though
as we have no way from the trigger implementation side to know if we might
get threaded interrupt handling or not on the downstream side.
We have reference counting to reenable a trigger that actually has a hardware
mask at the device end when all consumers are done - that should be used for
the reenable, not do it in the pollfunc handler. As it's a level interrupt
you avoid need to do a bonus read in there I think (sometimes that's necessary
because of an edge trigger and a slow read back on a possible unrelated device).
The subtle difference between IRQF_ONESHOT and irq_disable is one uses
the irq_mask / unmask callbacks on the irq chip and the other is using
the enable / disable ones. That may make no practical difference - I'm not
entirely sure. A quick glance at some drivers suggests masking is usually
lighter weight as less state is rewrite on reenable.
So in short, move the irq_enable() into the iio_trig->reenable() callback.
>
> This problem does not occur when I use bu27008_data_rdy_poll() (which is
> the same but disables the IRQ) instead of
> iio_trigger_generic_data_rdy_poll(), and re-enable the IRQ only after
> the handler bu27008_trigger_handler() has restored the IRQ line.
>
> Does the sequence above (bu27008_setup_trigger()) look sane?
>
> >
> >
> >> +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);
> >> +
> >> + enable_irq(data->irq);
> >
> > As below. This shouldn't be needed (and if it was it should be in the
> > reenable path that is ultimately a result of that notify_done above and
> > some reference counting fun).
>
> I will see the reenable callback, thanks!
Great
>
> >
> >> +
> >> + 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;
> >
> > Hmm. I'd missed this before but. This is in the wrong place really
> > (though it probably doesn't make much difference), stuff related to
> > enabling particular channels should be in iio_info->update_scan_mode()
>
> Oh. I'll check this out as well.
>
> >
> > It's arguable that the actual measurement mode setting might come
> > in the postenable callback (after the update_scan_mode() call which
> > in turn follows the preenable callback).
> >
> > All these callbacks have become a bit blurry over time as we end
> > up with devices that need to do nasty thing in one place. In this
> > particular case it's pretty simple though, so nicer to move
> > the scan mask stuff to the callback that is given the active_scan
> > mask as a parameter.
> >
> >> +
> >> + 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 irqreturn_t bu27008_data_rdy_poll(int irq, void *private)
> >> +{
> >> + /*
> >> + * The BU27008 keeps IRQ asserted until we read the VALID bit from
> >> + * a register. We need to keep the IRQ disabled until this
> >> + */
> >> + disable_irq_nosync(irq);
> >
> > As per my late reply to your question on this, shouldn't be needed
> > as IRQF_ONESHOT is ultimately set for the interrupts nested below this
> > so they'll get the resulting queuing on the threads which is fine.
>
> I see an IRQ storm if I omit this. The threaded trigger handler which
> 'ACKs' the IRQ gets never ran. I'll see the reenable though! Thanks!
As you probably figured, I was wrong. Reenable bit stands though!.
Jonathan
>
> Yours,
> -- Matti
>
On 5/13/23 20:52, Jonathan Cameron wrote:
> On Mon, 8 May 2023 09:32:28 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>
>> Hi Jonathan,
>>
>> On 5/7/23 17:54, Jonathan Cameron wrote:
>>> On Wed, 3 May 2023 12:50:14 +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>
>>>>
>>> Mostly stuff that you asked about in response to earlier version but
>>> which I hadn't replied to until today.
>>>
>>> Upshot, don't need the manual irq handling in here.
>>>
>>> Whilst you aren't setting IRQF_ONESHOT for the pollfunc side of the trigger
>>> (the downstream IRQ / IRQ thread) the IIO utility functions are.
>>
>> I tried doing:
>>
>> static int bu27008_setup_trigger(struct bu27008_data *data, struct
>> iio_dev *idev)
>> {
>> struct iio_trigger *itrig;
>> char *name;
>> int ret;
>>
>> ret = devm_iio_triggered_buffer_setup(data->dev, idev,
>> &iio_pollfunc_store_time,
>> bu27008_trigger_handler,
>> &bu27008_buffer_ops);
>> if (ret)
>> return dev_err_probe(data->dev, ret,
>> "iio_triggered_buffer_setup_ext FAIL\n");
>>
>> itrig = devm_iio_trigger_alloc(data->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(data->dev, GFP_KERNEL, "%s-bu27008",
>> dev_name(data->dev));
>>
>> ret = devm_request_irq(data->dev, data->irq,
>> /* No IRQ disabling */
>> &iio_trigger_generic_data_rdy_poll,
>> 0, name, itrig);
>> if (ret)
>> return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
>>
>> ret = devm_iio_trigger_register(data->dev, itrig);
>> if (ret)
>> return dev_err_probe(data->dev, ret,
>> "Trigger registration failed\n");
>>
>> /* set default trigger */
>> idev->trig = iio_trigger_get(itrig);
>>
>> return 0;
>> }
>>
>> It seems to me we get IRQ storm out of it, bu27008_trigger_handler never
>> being called. My assumption is that as soon as the IRQ handling code
>> exits the iio_trigger_generic_data_rdy_poll, it re-enables the IRQ - and
>> because we have level active IRQ and because the
>> bu27008_trigger_handler() has not yet had a chance to read the VALID bit
>> which restores the IRQ-line - we will immediately enter back to the IRQ
>> handling.
>
> Ah. I'd miss understood what was going on here. I thought we were talking
> race conditions only - not a level interrupt. Sorry for confusion / being
> half asleep. If it has an Ack like this I'd argue this is really an edge
> interrupt but that would require a guaranteed drop in the signal.
Yes. A failure to detect the edge (and skip acking) would leave the IRQ
no longer working. I think we have both seen some examples of that in
the past ;)
> I am assuming the sensor merrily carries on grabbing data, whether or
> not anyone reads it
This is also my understanding.
> and so if we treated this as an edge interrupt then
> the clear to set cycle could be very short (and hence not detected).
> If it instead doesn't read new data until previous has been read, then things
> are much simpler.
I think this is not how BU27008 works.
I think we could probably go on with edge IRQs and cook-up some "re-read
the VALID-bit again after the IRQ is for sure enabled to ensure the IRQ
does not go unasserted" - mechanism, which would work on 99.99% of the
cases. Problem is that some device always handles the 10000th
measurement ;) To tell the truth, I never really thought of that.
> Hmm. How to make this work cleanly assuming it's case 1. It might be that your
> current approach is the best though it would be nice to do something in the
> IIO code (with risk of breaking everyone :()
I didn't check if this would be doable.
I don't think we can though
> as we have no way from the trigger implementation side to know if we might
> get threaded interrupt handling or not on the downstream side.
>
> We have reference counting to reenable a trigger that actually has a hardware
> mask at the device end when all consumers are done - that should be used for
> the reenable, not do it in the pollfunc handler. As it's a level interrupt
> you avoid need to do a bonus read in there I think (sometimes that's necessary
> because of an edge trigger and a slow read back on a possible unrelated device).
>
> The subtle difference between IRQF_ONESHOT and irq_disable is one uses
> the irq_mask / unmask callbacks on the irq chip and the other is using
> the enable / disable ones. That may make no practical difference - I'm not
> entirely sure. A quick glance at some drivers suggests masking is usually
> lighter weight as less state is rewrite on reenable.
>
> So in short, move the irq_enable() into the iio_trig->reenable() callback.
>
This should be what I did at v5 :) Thanks for the help!
Yours,
-- Matti
@@ -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
@@ -38,6 +38,7 @@ obj-$(CONFIG_MAX44009) += max44009.o
obj-$(CONFIG_NOA1305) += noa1305.o
obj-$(CONFIG_OPT3001) += opt3001.o
obj-$(CONFIG_PA12203001) += pa12203001.o
+obj-$(CONFIG_ROHM_BU27008) += rohm-bu27008.o
obj-$(CONFIG_ROHM_BU27034) += rohm-bu27034.o
obj-$(CONFIG_RPR0521) += rpr0521.o
obj-$(CONFIG_SI1133) += si1133.o
new file mode 100644
@@ -0,0 +1,993 @@
+// 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_chan_type - BU27008 channel types
+ * @BU27008_RED: Red channel. Always via data0.
+ * @BU27008_GREEN: Green channel. Always via data1.
+ * @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_NUM_CHANS: Number of channel types.
+ */
+enum bu27008_chan_type {
+ BU27008_RED,
+ BU27008_GREEN,
+ BU27008_BLUE,
+ BU27008_CLEAR,
+ BU27008_IR,
+ BU27008_NUM_CHANS
+};
+
+/**
+ * enum bu27008_chan - BU27008 physical data channel
+ * @BU27008_DATA0: Always red.
+ * @BU27008_DATA0: Always green.
+ * @BU27008_DATA0: Blue or clear.
+ * @BU27008_DATA0: IR or clear.
+ * @BU27008_NUM_HW_CHANS: Number of physical channels
+ */
+enum bu27008_chan {
+ BU27008_DATA0,
+ BU27008_DATA1,
+ BU27008_DATA2,
+ BU27008_DATA3,
+ 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
+#define BU27008_MEAS_TIME_MAX_MS 400
+
+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);
+}
+
+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 = BU27008_MEAS_TIME_MAX_MS;
+ else
+ int_time /= USEC_PER_MSEC;
+
+ msleep(int_time);
+
+ 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;
+ }
+}
+
+/* Called if the new scale could not be supported with existing int-time */
+static int bu27008_try_find_new_time_gain(struct bu27008_data *data, int val,
+ int val2, int *gain_sel)
+{
+ 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)
+ goto unlock_out;
+
+ }
+ 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;
+
+ /*
+ * Do not allow changing scale when measurement is ongoing as doing so
+ * could make values in the 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_write_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);
+
+ ret = regmap_reinit_cache(data->regmap, &bu27008_regmap);
+ if (ret) {
+ dev_err(data->dev, "Failed to reinit reg cache\n");
+ return ret;
+ }
+
+ 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);
+
+ enable_irq(data->irq);
+
+ 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 irqreturn_t bu27008_data_rdy_poll(int irq, void *private)
+{
+ /*
+ * The BU27008 keeps IRQ asserted until we read the VALID bit from
+ * a register. We need to keep the IRQ disabled until this
+ */
+ disable_irq_nosync(irq);
+ iio_trigger_poll(private);
+
+ return IRQ_HANDLED;
+}
+
+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_irq(dev, i2c->irq,
+ &bu27008_data_rdy_poll,
+ 0, name, itrig);
+ 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");
+
+ /* set default trigger */
+ idev->trig = iio_trigger_get(itrig);
+ } else {
+ dev_info(dev, "No IRQ, buffered mode disabled\n");
+ }
+
+ ret = devm_iio_device_register(dev, idev);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Unable to register iio device\n");
+
+ return 0;
+}
+
+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);