Support ROHM BU27008 RGB 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.

Add initial support for the ROHM BU27008 color sensor.
- raw_read() of RGB and clear channels
- triggered buffer w/ DRDY interrtupt

+};

+};

drivers/iio/light/rohm-bu27008.c new file +8 -0

+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;

drivers/iio/light/rohm-bu27008.c new file +11 -0

+	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;
+}

drivers/iio/light/rohm-bu27008.c new file +3 -0

+	}
+
+	return ret;

drivers/iio/light/rohm-bu27008.c new file +38 -0

+		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");
+	}

Quoted 9 lines · Matti Vaittinen

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

...

drivers/iio/light/Kconfig config JSA1212 · line 304

Quoted 13 lines

drivers/iio/light/Kconfig +13 -0

+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?

...

Quoted 7 lines

drivers/iio/light/rohm-bu27008.c new file +7 -0

+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,

Review

Quoted 1 line

(quoted diff) +1 -0

+	}

+ trailing comma for consistency?

Quoted 1 line

+};

...

drivers/iio/light/rohm-bu27008.c line 269

Quoted 7 lines

drivers/iio/light/rohm-bu27008.c new file +7 -0

+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)?

Quoted 2 lines

+};

...

Quoted 8 lines

drivers/iio/light/rohm-bu27008.c new file +8 -0

+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;

drivers/iio/light/rohm-bu27008.c line 556

Quoted 10 lines

drivers/iio/light/rohm-bu27008.c new file +10 -0

+
+	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.

Quoted 12 lines

drivers/iio/light/rohm-bu27008.c new file +11 -0

+	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;
+}

...

drivers/iio/light/rohm-bu27008.c line 748

Quoted 5 lines

drivers/iio/light/rohm-bu27008.c new file +4 -0

+	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.

Quoted 4 lines

drivers/iio/light/rohm-bu27008.c new file +3 -0

+	}
+
+	return ret;

...

drivers/iio/light/rohm-bu27008.c line 927

Quoted 1 line

drivers/iio/light/rohm-bu27008.c new file +1 -0

+	if (i2c->irq) {

Instead of a long body, I would rather see a call to

		ret = ..._setup_irq();
		if (ret)
			return ret;

Quoted 30 lines

drivers/iio/light/rohm-bu27008.c new file +30 -0

+		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)

drivers/iio/light/rohm-bu27008.c line 965

Quoted 8 lines

drivers/iio/light/rohm-bu27008.c new file +8 -0

+			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");
+	}

Some triggers can only be attached to the IIO device that corresponds to the same physical device. Implement generic helper which can be used as a validate_trigger callback for such devices.

Suggested-by Jonathan Cameron <[email protected]>

Signed-off-by Matti Vaittinen <[email protected]>

---
Revision history
v2: => v3:
- Fix title (space after iio:)
v2: New patch

---
 drivers/iio/industrialio-trigger.c | 22 +++++++++++++++++++++-
 include/linux/iio/trigger.h        |  1 +
 2 files changed, 22 insertions(+), 1 deletion(-)

drivers/iio/industrialio-trigger.c +1 -1

index 784dc1e00310..c616297aa754 100644
@@ -322,7 +322,7 @@ int iio_trigger_attach_poll_func(struct iio_trigger *trig,
 	 * this is the case if the IIO device and the trigger device share the
 	 * same parent device.
 	 */
-	if (pf->indio_dev->dev.parent == trig->dev.parent)
+	if (iio_validate_own_trigger(pf->indio_dev, trig))
 		trig->attached_own_device = true;
 
 	return ret;

drivers/iio/industrialio-trigger.c +15 -0

+ *
+ * This function can be used as the validate_trigger callback for triggers that
+ * can only be attached to their own device.
+ *
+ * Return: 0 if both the trigger and the IIO device belong to the same
+ * device, -EINVAL otherwise.
+ */
+int iio_validate_own_trigger(struct iio_dev *idev, struct iio_trigger *trig)
+{
+	if (idev->dev.parent != trig->dev.parent)
+		return -EINVAL;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(iio_validate_own_trigger);
+
 /**
  * iio_trigger_validate_own_device - Check if a trigger and IIO device belong to
  *  the same device

include/linux/iio/trigger.h +1 -0

index 51f52c5c6092..bce3b1788199 100644
@@ -171,6 +171,7 @@ void iio_trigger_free(struct iio_trigger *trig);
  */
 bool iio_trigger_using_own(struct iio_dev *indio_dev);
 
+int iio_validate_own_trigger(struct iio_dev *idev, struct iio_trigger *trig);
 int iio_trigger_validate_own_device(struct iio_trigger *trig,
 				     struct iio_dev *indio_dev);
 

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 <[email protected]>

drivers/iio/light/rohm-bu27008.c new file +0 -0

new file mode 100644
index 000000000000..c04d845062ba
@@ -0,0 +1,993 @@

drivers/iio/light/rohm-bu27008.c new file +13 -0

+
+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)

drivers/iio/light/rohm-bu27008.c new file +35 -0

+	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);

drivers/iio/light/rohm-bu27008.c new file +32 -0

+		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;
+

drivers/iio/light/rohm-bu27008.c new file +15 -0

+
+	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;

drivers/iio/light/rohm-bu27008.c new file +13 -0

+
+	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,

drivers/iio/light/rohm-bu27008.c new file +4 -0

+	iio_trigger_poll(private);
+
+	return IRQ_HANDLED;
+}

Quoted 6 lines · On Wed, 3 May 2023 12:50:14 +0300 Matti Vaittinen <[email protected]>…

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.

Quoted 6 lines

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 <[email protected]>

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.

Quoted 29 lines

drivers/iio/light/rohm-bu27008.c new file +27 -0

+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);

drivers/iio/light/rohm-bu27008.c line 810

Quoted 4 lines

drivers/iio/light/rohm-bu27008.c new file +4 -0

+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).

Quoted 22 lines

drivers/iio/light/rohm-bu27008.c new file +21 -0

+
+	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);
+

drivers/iio/light/rohm-bu27008.c line 835

Quoted 4 lines

drivers/iio/light/rohm-bu27008.c new file +4 -0

+	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.

Quoted 19 lines

drivers/iio/light/rohm-bu27008.c new file +19 -0

+
+	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)
+{
+	/*

drivers/iio/light/rohm-bu27008.c line 858

Quoted 4 lines

drivers/iio/light/rohm-bu27008.c new file +4 -0

+	 * 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.

Quoted 1 line · Andy Shevchenko

On Wed, May 03, 2023 at 12:50:14PM +0300, Matti Vaittinen wrote:

Quoted 7 lines

Quoted 7 lines

drivers/iio/light/Kconfig +7 -0

+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.

Quoted 8 lines

drivers/iio/light/Kconfig config JSA1212 · line 304

Quoted 6 lines

drivers/iio/light/Kconfig +6 -0

+	  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/[email protected]/

Module name is completely irrelevant when selecting a kernel configuration.

Quoted 1 line

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.

Quoted 2 lines · Matti

Quoted 1 line · Andy Shevchenko

On Wed, May 03, 2023 at 12:50:14PM +0300, Matti Vaittinen wrote:

...

Quoted 16 lines

Quoted 15 lines

drivers/iio/light/Kconfig config JSA1212 · line 304

Quoted 13 lines

drivers/iio/light/Kconfig +13 -0

+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?

Quoted 5 lines

We have discussed this several times already.

https://lore.kernel.org/all/[email protected]/

Module name is completely irrelevant when selecting a kernel configuration.

This option is also selectable by user.

...

Quoted 5 lines

Quoted 1 line

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.

Quoted 8 lines

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).

Quoted 8 lines

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 Jonathan,

Quoted 15 lines · Jonathan Cameron

Quoted 6 lines · On Wed, 3 May 2023 12:50:14 +0300 Matti Vaittinen <[email protected]>…

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.

Quoted 6 lines

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 <[email protected]>

Quoted 7 lines

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?

Quoted 29 lines

Quoted 29 lines

drivers/iio/light/rohm-bu27008.c new file +27 -0

+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);

Quoted 8 lines

drivers/iio/light/rohm-bu27008.c line 810

Quoted 4 lines

drivers/iio/light/rohm-bu27008.c new file +4 -0

+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!

Quoted 23 lines

Quoted 23 lines

drivers/iio/light/rohm-bu27008.c new file +21 -0

+
+	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);
+

Quoted 8 lines

drivers/iio/light/rohm-bu27008.c line 835

Quoted 4 lines

drivers/iio/light/rohm-bu27008.c new file +4 -0

+	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.

Quoted 30 lines

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.

Quoted 19 lines

drivers/iio/light/rohm-bu27008.c new file +19 -0

+
+	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)
+{
+	/*

Quoted 8 lines

drivers/iio/light/rohm-bu27008.c line 858

Quoted 4 lines

drivers/iio/light/rohm-bu27008.c new file +4 -0

+	 * 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

Quoted 74 lines · On Mon, 8 May 2023 09:32:28 +0300 Matti Vaittinen <[email protected]>…

Hi Jonathan,

Quoted 15 lines · Jonathan Cameron

Quoted 6 lines · On Wed, 3 May 2023 12:50:14 +0300 Matti Vaittinen <[email protected]>…

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.

Quoted 6 lines

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 <[email protected]>

Quoted 7 lines

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;
}

Quoted 7 lines

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 :()

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.