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

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

---
Revision history
v2 => v3:
- drop bits.h
- drop unnecessary comma after compatible
- Styling / cleaning
- Simplify sleep time computation
- rename bu27008_get_int_time() => bu27008_get_int_time_us()

v1 => v2:
- Fix buffered data demuxing
- Use generic trigger functions instead of rolling own ones
- Drop unnecessary locking
- Use generic iio_validate_own_trigger()
- Some other more trivial fixes for review comments
- use defines for [enable/disable] [measurement/data-ready IRQ] reg values and use regmap_update_bits() directly instead of regamap_[set/clear]_bits()

---
 drivers/iio/light/Kconfig        |  14 +
 drivers/iio/light/Makefile       |   1 +
 drivers/iio/light/rohm-bu27008.c | 963 +++++++++++++++++++++++++++++++
 3 files changed, 978 insertions(+)
 create mode 100644 drivers/iio/light/rohm-bu27008.c

drivers/iio/light/Kconfig +14 -0

index 6fa31fcd71a1..7888fc439b2f 100644
@@ -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

drivers/iio/light/Makefile +0 -0

index 985f6feaccd4..881173952301 100644

drivers/iio/light/Makefile +0 -0

 obj-$(CONFIG_RPR0521)		+= rpr0521.o
 obj-$(CONFIG_SI1133)		+= si1133.o
 obj-$(CONFIG_SI1145)		+= si1145.o

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

new file mode 100644
index 000000000000..2728e6d5a321
 
+
+enum {
+	BU27008_RED,	/* Always data0 */
+	BU27008_GREEN,	/* Always data1 */
+	BU27008_BLUE,	/* data2, configurable (blue / clear) */
+	BU27008_CLEAR,	/* data2 or data3 */
+	BU27008_IR,	/* data3 */
+	BU27008_NUM_CHANS
+};
+
+enum {
+	BU27008_DATA0, /* Always RED */
+	BU27008_DATA1, /* Always GREEN */
+	BU27008_DATA2, /* Blue or Clear */
+	BU27008_DATA3, /* IR or Clear */
+	BU27008_NUM_HW_CHANS
+};
+
+/* We can always measure red and green at same time */
+#define ALWAYS_SCANNABLE (BIT(BU27008_RED) | BIT(BU27008_GREEN))
+
+/* We use these data channel configs. Ensure scan_masks below follow them too */
+#define BU27008_BLUE2_CLEAR3		0x0 /* buffer is R, G, B, C */
+#define BU27008_CLEAR2_IR3		0x1 /* buffer is R, G, C, IR */
+#define BU27008_BLUE2_IR3		0x2 /* buffer is R, G, B, IR */
+
+static const unsigned long bu27008_scan_masks[] = {
+	/* buffer is R, G, B, C */
+	ALWAYS_SCANNABLE | BIT(BU27008_BLUE) | BIT(BU27008_CLEAR),
+	/* buffer is R, G, C, IR */
+	ALWAYS_SCANNABLE | BIT(BU27008_CLEAR) | BIT(BU27008_IR),
+	/* buffer is R, G, B, IR */
+	ALWAYS_SCANNABLE | BIT(BU27008_BLUE) | BIT(BU27008_IR),
+	0
+};
 
 
+
+static int bu27008_get_int_time_sel(struct bu27008_data *data, int *sel)
+{
+	int ret, val;
+
+	ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL1, &val);
+	*sel = FIELD_GET(BU27008_MASK_MEAS_MODE, val);
+
+	return ret;
+}
+
+static int bu27008_set_int_time_sel(struct bu27008_data *data, int sel)
+{
+	return regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL1,
+				  BU27008_MASK_MEAS_MODE, sel);
+}
+

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

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

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

+
+	if (!ret)
+		ret = bu27008_write_gain_sel(data, gain_sel);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}

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

+	 * This could make values in buffer inconsistent.
+	 */
+	ret = iio_device_claim_direct_mode(idev);
+	if (ret)
+		return ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		ret = bu27008_set_scale(data, chan, val, val2);
+		break;
+	case IIO_CHAN_INFO_INT_TIME:
+		if (val)
+			ret = -EINVAL;
+		else
+			ret = bu27008_try_set_int_time(data, val2);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	iio_device_release_direct_mode(idev);
+
+	return ret;

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

+
+	return IRQ_HANDLED;
+}
+
 
 
+static int bu27008_probe(struct i2c_client *i2c)
+{

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

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

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

+	}
+
+	ret = devm_iio_device_register(dev, idev);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Unable to register iio device\n");
+
+	return ret;
+}

Quoted 11 lines · On Wed, 26 Apr 2023 11:08:17 +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.

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]>

Hi Matti,

Mostly trivial stuff, but some confusion has occurred with respect to the two interrupts involve in an IIO trigger. Specifically the pollfunc stuff occurs on the downward side of the irqchip that hides on the consumer side of an IIO trigger) and is set up by devm_iio_trigered_buffer_setup() not of the interrupt that calls iio_trigger_poll[_nested]()

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

Quoted 5 lines

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

+
+static int bu27008_try_find_new_time_gain(struct bu27008_data *data, int val,
+					  int val2, int *gain_sel)
+{
+	/* Could not support new scale with existing int-time */

I'd move that above the function and change it to

	/* Called if the new scale could not be supported with existing int-time */

Down here it is not clear that this applies to the whole funciton.

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

Quoted 12 lines

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

+	int i, ret, new_time_sel;
+
+	for (i = 0; i < data->gts.num_itime; i++) {
+		new_time_sel = data->gts.itime_table[i].sel;
+		ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts,
+					new_time_sel, val, val2 * 1000, gain_sel);
+		if (!ret)
+			break;
+	}
+	if (i == data->gts.num_itime) {
+		dev_err(data->dev, "Can't support scale %u %u\n", val,
+			val2);

Line wrapping inconsistent. I like short lines with appropriate flexibility where longer ones are more readable. However, I am fairly sure this one fits under 80 chars as a single line.

Quoted 17 lines

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

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

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

Quoted 10 lines

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

+	mutex_lock(&data->mutex);
+
+	ret = bu27008_get_int_time_sel(data, &time_sel);
+	if (ret < 0)
+		goto unlock_out;
+
+	ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts, time_sel,
+						val, val2 * 1000, &gain_sel);
+	if (ret)
+		ret = bu27008_try_find_new_time_gain(data, val, val2, &gain_sel);

Obviously it is code that doesn't make any functional difference, but I'd prefer to see

	if (ret) {
		ret = bu27....
		if (ret)
			goto unlock_out;
	}
	ret = bu27008_write_gain_sel();

so that each error path is out of line, but the good path is the linear flow.

Quoted 9 lines

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

+
+	if (!ret)
+		ret = bu27008_write_gain_sel(data, gain_sel);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+
+	return ret;

drivers/iio/light/rohm-bu27008.c

Quoted 10 lines

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

+}
 
 
+static irqreturn_t bu27008_irq_thread_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *idev = pf->indio_dev;
+	struct bu27008_data *data = iio_priv(idev);
+
+	iio_trigger_poll_nested(data->trig);

See below but this is what alerted me to something unusual. It never makes sense to have iio_trigger_poll_nested() called unless there is a check on whether it should be called! If there isn't iio_trigger_poll() in the top half is the right thing to do. >

Quoted 8 lines

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

+
+	return IRQ_HANDLED;
+}
+
 
 
+static int bu27008_probe(struct i2c_client *i2c)
+{

...

Quoted 19 lines

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

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

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

Quoted 6 lines

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

+
+		name = devm_kasprintf(dev, GFP_KERNEL, "%s-bu27008",
+				      dev_name(dev));
+
+		ret = devm_request_threaded_irq(dev, i2c->irq,
+						iio_pollfunc_store_time,

This is on the wrong irq.

here. Basically what happens is the caller of iio_poll_trigger() fires the input to a software irq chip that then signals all the of the downstream irqs (which are the individual consumers of the triggers). If that's triggered from the top half / non threaded bit of the interrupt the iio_pollfunc_store_time() will be called in that non threaded context before the individual threads for the trigger consumer are started.

If there is nothing to do in the actual interrupt as it's a data ready only signal, then you should just call iio_trigger_poll() in the top half and use devm_request_irq() only as there is no thread in this interrupt (though there is one for the interrupt below the software interrupt chip).

Quoted 5 lines

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

+						&bu27008_irq_thread_handler,
+						IRQF_ONESHOT, name, idev->pollfunc);
+		if (ret)

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

Quoted 10 lines

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

+			return dev_err_probe(dev, ret,
+					     "Could not request IRQ\n");
+
+
+		ret = devm_iio_trigger_register(dev, itrig);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Trigger registration failed\n");
+	} else {
+		dev_warn(dev, "No IRQ configured\n");

Why is it a warning? Either driver works without an IRQ, or it doesn't. dev_dbg() or dev_info() at most.

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

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/rohm-bu27008.c line 61

Quoted 7 lines

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

+enum {
+	BU27008_RED,	/* Always data0 */
+	BU27008_GREEN,	/* Always data1 */
+	BU27008_BLUE,	/* data2, configurable (blue / clear) */
+	BU27008_CLEAR,	/* data2 or data3 */
+	BU27008_IR,	/* data3 */
+	BU27008_NUM_CHANS

Why not converting comments to a kernel-doc?

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

Quoted 9 lines

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

+};
+
+enum {
+	BU27008_DATA0, /* Always RED */
+	BU27008_DATA1, /* Always GREEN */
+	BU27008_DATA2, /* Blue or Clear */
+	BU27008_DATA3, /* IR or Clear */
+	BU27008_NUM_HW_CHANS
+};

Ditto.

...

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

Quoted 2 lines

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

+	if (int_time < 0)
+		int_time = 400000;

Adding 3 0:s to drop them below with a heavy division operation? Well done! Or did I miss anything?

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

Quoted 2 lines

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

+	msleep(int_time / 1000);

USEC_PER_MSEC ?

...

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

Quoted 6 lines

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

+	ret = devm_iio_device_register(dev, idev);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Unable to register iio device\n");
+
+	return ret;

return 0 will suffice.

Quoted 21 lines

Quoted 21 lines

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

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

Quoted 8 lines

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

Quoted 6 lines

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

+
+		name = devm_kasprintf(dev, GFP_KERNEL, "%s-bu27008",
+				      dev_name(dev));
+
+		ret = devm_request_threaded_irq(dev, i2c->irq,
+						iio_pollfunc_store_time,  

This is on the wrong irq.

Seems like I have some homework to do :)

Right. I now see I pass the iio_pollfunc_store_time() as top half for both the "real IRQ" generated by the device (here), as well as a top-half for the devm_iio_triggered_buffer_setup(). Ideally I like the idea of taking the timestamp in the top half for the device-generated IRQ as it is closer the moment HW did acquire the sample - but it really would make no difference here (even if I did it correctly).

iio_pollfunc_store_time is used with the trigger not

Quoted 6 lines

here. Basically what happens is the caller of iio_poll_trigger() fires the input to a software irq chip that then signals all the of the downstream irqs (which are the individual consumers of the triggers). If that's triggered from the top half / non threaded bit of the interrupt the iio_pollfunc_store_time() will be called in that non threaded context before the individual threads for the trigger consumer are started.

Oh. So, you mean the iio_pollfunc_store_time() is automatically called already before kicking the SW-IRQ? So we don't need it in devm_iio_triggered_buffer_setup() anymore?

Quoted 5 lines

If there is nothing to do in the actual interrupt as it's a data ready only signal, then you should just call iio_trigger_poll() in the top half and use devm_request_irq() only as there is no thread in this interrupt (though there is one for the interrupt below the software interrupt chip).

I haven't tested this yet so please ignore me if I am writing nonsense - but... The BU27008 will keep the IRQ line asserted until a register is read. We can't read the register form HW-IRQ so we need to keep the IRQ disabled until the threaded trigger handler is ran. With the setup we have here, the IRQF_ONESHOT, took care of this. I assume that changing to call the iio_poll_trigger() from top-half means I need to explicitly disable the IRQ and re-enable it at the end of the trigger thread after reading the register which debounces the IRQ line?

Quoted 19 lines

Quoted 6 lines

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

+						&bu27008_irq_thread_handler,
+						IRQF_ONESHOT, name, idev->pollfunc);
+		if (ret)

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

Quoted 10 lines

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

+			return dev_err_probe(dev, ret,
+					     "Could not request IRQ\n");
+
+
+		ret = devm_iio_trigger_register(dev, itrig);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Trigger registration failed\n");
+	} else {
+		dev_warn(dev, "No IRQ configured\n");  

Why is it a warning? Either driver works without an IRQ, or it doesn't. dev_dbg() or dev_info() at most.

Some of it works. Well, maybe I'll change it to tell that device works in raw_read only mode.

Thanks again for the help!

Yours,
-- Matti

Hi Andy

Thanks for the review.

Quoted 23 lines · Andy Shevchenko

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/rohm-bu27008.c line 61

Quoted 7 lines

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

+enum {
+	BU27008_RED,	/* Always data0 */
+	BU27008_GREEN,	/* Always data1 */
+	BU27008_BLUE,	/* data2, configurable (blue / clear) */
+	BU27008_CLEAR,	/* data2 or data3 */
+	BU27008_IR,	/* data3 */
+	BU27008_NUM_CHANS  

Why not converting comments to a kernel-doc?

Quoted 11 lines

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

Quoted 9 lines

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

+};
+
+enum {
+	BU27008_DATA0, /* Always RED */
+	BU27008_DATA1, /* Always GREEN */
+	BU27008_DATA2, /* Blue or Clear */
+	BU27008_DATA3, /* IR or Clear */
+	BU27008_NUM_HW_CHANS
+};  

Ditto.

I see no value having entities which are not intended to be used outside this file documented in any "global" documentation. One who is ever going to use these or wonder what these are - will most likely be watching this file. My personal view is that the generated docs should be kept lean. In my opinion the problem of the day is the time we spend looking for a needle hidden in a haystack. In my opinion adding this to kernel-doc just adds hay :)

I still can do this if no-one else objects. I almost never look at the generated docs myself. Usually I just look the docs from code files - and kernel-doc format is not any worse for me to read. Still, I can imagine including this type of stuff to generic doc just bloats them and my not serve well those who use them.

Quoted 9 lines

...

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

Quoted 2 lines

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

+	if (int_time < 0)
+		int_time = 400000;  

Adding 3 0:s to drop them below with a heavy division operation? Well done! Or did I miss anything?

No. You did not miss anything. This can be improved.

Quoted 4 lines

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

Quoted 2 lines

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

+	msleep(int_time / 1000);  

USEC_PER_MSEC ?

Ok.

Quoted 10 lines

...

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

Quoted 6 lines

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

+	ret = devm_iio_device_register(dev, idev);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Unable to register iio device\n");
+
+	return ret;  

return 0 will suffice.

Ok.

Thanks,
-- Matti

Quoted 49 lines · Matti

Hi Andy

Thanks for the review.

Quoted 23 lines · Andy Shevchenko

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/rohm-bu27008.c line 61

Quoted 7 lines

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

+enum {
+	BU27008_RED,	/* Always data0 */
+	BU27008_GREEN,	/* Always data1 */
+	BU27008_BLUE,	/* data2, configurable (blue / clear) */
+	BU27008_CLEAR,	/* data2 or data3 */
+	BU27008_IR,	/* data3 */
+	BU27008_NUM_CHANS

Why not converting comments to a kernel-doc?

Quoted 11 lines

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

Quoted 9 lines

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

+};
+
+enum {
+	BU27008_DATA0, /* Always RED */
+	BU27008_DATA1, /* Always GREEN */
+	BU27008_DATA2, /* Blue or Clear */
+	BU27008_DATA3, /* IR or Clear */
+	BU27008_NUM_HW_CHANS
+};

Ditto.

I see no value having entities which are not intended to be used outside this file documented in any "global" documentation. One who is ever going to use these or wonder what these are - will most likely be watching this file. My personal view is that the generated docs should be kept lean. In my opinion the problem of the day is the time we spend looking for a needle hidden in a haystack. In my opinion adding this to kernel-doc just adds hay :)

Quoted 5 lines

I still can do this if no-one else objects. I almost never look at the generated docs myself. Usually I just look the docs from code files - and kernel-doc format is not any worse for me to read. Still, I can imagine including this type of stuff to generic doc just bloats them and my not serve well those who use them.

Unless someone specifically adds this doc to the main docs build, the kernel-doc won't end up in the docs anyway.

It just provides a nice
bit of consistent formatting. Even if they do add this for some reason, there are controls on internal vs external (exported stuff) being added to the docs.

Quoted 5 lines

Quoted 4 lines

If there is nothing to do in the actual interrupt as it's a data ready only signal, then you should just call iio_trigger_poll() in the top half and use devm_request_irq() only as there is no thread in this interrupt (though there is one for the interrupt below the software interrupt chip).

Quoted 8 lines

I haven't tested this yet so please ignore me if I am writing nonsense - but... The BU27008 will keep the IRQ line asserted until a register is read. We can't read the register form HW-IRQ so we need to keep the IRQ disabled until the threaded trigger handler is ran. With the setup we have here, the IRQF_ONESHOT, took care of this. I assume that changing to call the iio_poll_trigger() from top-half means I need to explicitly disable the IRQ and re-enable it at the end of the trigger thread after reading the register which debounces the IRQ line?

Hmm. I'm trying to remember how this works (wrote this a very long time ago). I'm fairly sure it's not an issue because we use IRQF_ONESHOT down one level so exercise the same prevention of the threads triggering multiple times etc >

It doesn't matter if the device interrupt fires again as it will still be masked at our software irqchip level and will then get queued up and the thread will run again.

Quoted 2 lines · Matti

Quoted 1 line · Andy Shevchenko

On Wed, Apr 26, 2023 at 11:08:17AM +0300, Matti Vaittinen wrote:

Quoted 31 lines

Quoted 10 lines

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

Quoted 7 lines

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

+enum {
+	BU27008_RED,	/* Always data0 */
+	BU27008_GREEN,	/* Always data1 */
+	BU27008_BLUE,	/* data2, configurable (blue / clear) */
+	BU27008_CLEAR,	/* data2 or data3 */
+	BU27008_IR,	/* data3 */
+	BU27008_NUM_CHANS  

Why not converting comments to a kernel-doc?

Quoted 11 lines

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

Quoted 9 lines

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

+};
+
+enum {
+	BU27008_DATA0, /* Always RED */
+	BU27008_DATA1, /* Always GREEN */
+	BU27008_DATA2, /* Blue or Clear */
+	BU27008_DATA3, /* IR or Clear */
+	BU27008_NUM_HW_CHANS
+};  

Ditto.

I see no value having entities which are not intended to be used outside this file documented in any "global" documentation. One who is ever going to use these or wonder what these are - will most likely be watching this file. My personal view is that the generated docs should be kept lean. In my opinion the problem of the day is the time we spend looking for a needle hidden in a haystack. In my opinion adding this to kernel-doc just adds hay :)

Quoted 5 lines

I still can do this if no-one else objects. I almost never look at the generated docs myself. Usually I just look the docs from code files - and kernel-doc format is not any worse for me to read. Still, I can imagine including this type of stuff to generic doc just bloats them and my not serve well those who use them.

Unless someone specifically adds this doc to the main docs build, the kernel-doc won't end up in the docs anyway. It just provides a nice bit of consistent formatting. Even if they do add this for some reason, there are controls on internal vs external (exported stuff) being added to the docs.

Quoted 4 lines · Jonathan Cameron

Quoted 2 lines · Matti

Quoted 1 line · Andy Shevchenko

On Wed, Apr 26, 2023 at 11:08:17AM +0300, Matti Vaittinen wrote:

...

Quoted 38 lines

Quoted 31 lines

Quoted 10 lines

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

Quoted 7 lines

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

+enum {
+	BU27008_RED,	/* Always data0 */
+	BU27008_GREEN,	/* Always data1 */
+	BU27008_BLUE,	/* data2, configurable (blue / clear) */
+	BU27008_CLEAR,	/* data2 or data3 */
+	BU27008_IR,	/* data3 */
+	BU27008_NUM_CHANS

Why not converting comments to a kernel-doc?

Quoted 11 lines

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

Quoted 9 lines

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

+};
+
+enum {
+	BU27008_DATA0, /* Always RED */
+	BU27008_DATA1, /* Always GREEN */
+	BU27008_DATA2, /* Blue or Clear */
+	BU27008_DATA3, /* IR or Clear */
+	BU27008_NUM_HW_CHANS
+};

Ditto.

I see no value having entities which are not intended to be used outside this file documented in any "global" documentation. One who is ever going to use these or wonder what these are - will most likely be watching this file. My personal view is that the generated docs should be kept lean. In my opinion the problem of the day is the time we spend looking for a needle hidden in a haystack. In my opinion adding this to kernel-doc just adds hay :)

Quoted 5 lines

I still can do this if no-one else objects. I almost never look at the generated docs myself. Usually I just look the docs from code files - and kernel-doc format is not any worse for me to read. Still, I can imagine including this type of stuff to generic doc just bloats them and my not serve well those who use them.

Quoted 5 lines

Unless someone specifically adds this doc to the main docs build, the kernel-doc won't end up in the docs anyway. It just provides a nice bit of consistent formatting. Even if they do add this for some reason, there are controls on internal vs external (exported stuff) being added to the docs.

I can run it manually and see in a nice form instead of browsing file for that, so there is still a usefulness in my opinion. Esp. taking into account that comments are already there. It's just different and helpful form of representation. No?

Quoted 1 line

Quoted 1 line

and kernel-doc format is not any worse for me to read.

Quoted 14 lines · Jonathan Cameron

Quoted 5 lines

Quoted 4 lines

If there is nothing to do in the actual interrupt as it's a data ready only signal, then you should just call iio_trigger_poll() in the top half and use devm_request_irq() only as there is no thread in this interrupt (though there is one for the interrupt below the software interrupt chip).

Quoted 8 lines

I haven't tested this yet so please ignore me if I am writing nonsense - but... The BU27008 will keep the IRQ line asserted until a register is read. We can't read the register form HW-IRQ so we need to keep the IRQ disabled until the threaded trigger handler is ran. With the setup we have here, the IRQF_ONESHOT, took care of this. I assume that changing to call the iio_poll_trigger() from top-half means I need to explicitly disable the IRQ and re-enable it at the end of the trigger thread after reading the register which debounces the IRQ line?

Quoted 3 lines

Hmm. I'm trying to remember how this works (wrote this a very long time ago). I'm fairly sure it's not an issue because we use IRQF_ONESHOT down one level so exercise the same prevention of the threads triggering multiple times etc >

https://elixir.bootlin.com/linux/latest/source/drivers/iio/buffer/industrialio-triggered-buffer.c#L57

Quoted 4 lines

It doesn't matter if the device interrupt fires again as it will still be masked at our software irqchip level and will then get queued up and the thread will run again.

After reading this I am not at all sure I am using the trigger correctly. I see the iio_trigger_attach_poll_func() registering threaded handler with the IRQF_ONESHOT which is stored in the iio_alloc_pollfunc() as you pointed above.

The iio_trigger_attach_poll_func() is called from sysfs callback when trigger is enabled. So, if this is supposed to be the device IRQ, then I am not at all sure the driver should be requesting the IRQ at the probe(). If it is not the device's IRQ, then I guess the IRQF_ONESHOT passed in here won't help. I need to try seeing some examples how other drivers are using the triggers. Getting back to this tomorrow...

Yours,

	-- Matti.