Support ROHM BU27008 RGB sensor

<[email protected]>,[email protected],[email protected]

Add myself as a maintainer for ROHM BU27008 color sensor driver.

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

---
  MAINTAINERS | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

MAINTAINERS +2 -1

index ac81383129a6..6c0775aabade 100644
@@ -18099,10 +18099,11 @@ S:	Maintained
  F:	Documentation/devicetree/bindings/iio/light/bh1750.yaml
  F:	drivers/iio/light/bh1750.c
 
-ROHM BU27034 AMBIENT LIGHT SENSOR DRIVER
+ROHM BU270xx LIGHT SENSOR DRIVERs
  M:	Matti Vaittinen <[email protected]>
  L:	[email protected]
  S:	Supported
+F:	drivers/iio/light/rohm-bu27008.c
  F:	drivers/iio/light/rohm-bu27034.c
 
  ROHM MULTIFUNCTION BD9571MWV-M PMIC DEVICE DRIVERS

Add support for 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.

This series supports reading the RGBC and IR channels using IIO frameeork. However, only two of the BC+IR can be enabled at the same

time. Series adds also support for scale and integration time configuration, where scale consists of impact of both the integration time and hardware gain. The gain and time support is backed by the newly introduced IIO GTS helper. This series depends on GTS helper patches added in BU27034 support series which is already merged in iio/togreg which this series is based on.

The hardware allows configuring gain setting by writing a 5-bit gain selector value to a register. Part of the gain setting is common for all channels (RGBC + IR) but part of the selector value can be set separately for RGBC and IR:

MODE_CONTROL2 REG:
bit 7 6 5 4 3 2 1 0
-----------------------------------------------------------------

|	RGB	selector		|
+---------------------------------------+
-----------------------------------------------------------------
| high bits IR	|			| low bits IR selector	|
+---------------+			+-----------------------+

In theory it would be possible to set certain separate gain values for RGBC and IR channels, but this gets pretty confusing because there are a few 'unsupported' selector values. If only RGBC or IR was set, some extra handling should be done to prevent the other channel from getting unsupported value due to change in high-bits. Furthermore, allowing the channels to be set different gain values (in some cases when gains are such the HW supports it) would make the cases where also integration time is changed to achieve correct scale ... interesting. It might also be confusing for user to try predicting when setting different scales succeeds and when it does not. Furthermore, if for example the scale setting for RGBC caused IR selector to be invalid - it could also cause the IR scale to "jump" very far from previous value.

To make the code simpler and more predictable for users, the current logic is as follows:

1. Prevent setting IR scale. (My assumption is IR is less used than RGBC)
2. When RGBC scale is set, set also the IR-selector to the same value. This prevents unsupported selector values and makes the IR scale changes predictable.

The 2) could mean we effectively have the same scale for all channels. Unfortunately, the HW design is slightly peculiar and selector 0 means gain 1X on RGBC but gain 2X on IR. Rest of the selectors equal same gain values on RGBC and IR. The result is that while changin selector from 0 => 1 causes RGBC gain to go from 1X => 4X, it causes IR gain to go from 2X => 4X.

So, the driver provides separate scale entries for all channels (also RGB and C will have separate gain entries because these channels are of same type as IR channel). This makes it possible for user applications to go read the scales for all channels after setting scale for one (in order to detect the IR scale difference).

Having the separate IR scale entry which applications can read to detect "arbitrary scale changes" makes it possible for applications to be written so they can cope if we need to implement the 'allow setting some different gains for IR and RGBC' - later.

Finally, the scales_available is also provided for all other channels except the IR channel, which does not allow the scale to be changed.

The sensor provides a data-ready IRQ and the driver implements a triggered buffer mode using this IRQ as a trigger.

---

Matti Vaittinen (3):
dt-bindings: iio: light: ROHM BU27008
iio: light: ROHM BU27008 color sensor
MAINTAINERS: Add ROHM BU27008

 .../bindings/iio/light/rohm-bu27008.yaml      |   49 +
 MAINTAINERS                                   |    3 +-
 drivers/iio/light/Kconfig                     |   14 +
 drivers/iio/light/Makefile                    |    1 +
 drivers/iio/light/rohm-bu27008.c              | 1028 +++++++++++++++++
 5 files changed, 1094 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/iio/light/rohm-bu27008.yaml
 create mode 100644 drivers/iio/light/rohm-bu27008.c

base-commit: 52cc189b4fc6af6accc45fe7b7053d76d8724059

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..6fca193eeb9e
@@ -0,0 +1,1028 @@

drivers/iio/light/rohm-bu27008.c new file +13 -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
+};
+
+enum {
+	BU27008_DATA0, /* Always RED */
+	BU27008_DATA1, /* Always GREEN */

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

+};
 
+
+static int bu27008_get_gain(struct bu27008_data *data, struct iio_gts *gts, int *gain)
+{

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

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

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

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

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

+	data->timestamp = iio_get_time_ns(idev);
+
+	if (data->trigger_enabled)
+		return IRQ_WAKE_THREAD;

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

+	 */
+	ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL3, &dummy);
+	if (ret < 0)
+		goto err_read;
+

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

+
+	/*
+	 * We need to check the scan mask to determine which of the
+	 * BLUE/CLEAR/IR are enabled so we know which channel is used to
+	 * measure which data.
+	 */
+	if (*idev->active_scan_mask & BIT(BU27008_BLUE)) {

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

+
+	iio_push_to_buffers_with_timestamp(idev, raw, pf->timestamp);
+err_read:
+	iio_trigger_notify_done(idev->trig);
+
+	return IRQ_HANDLED;
+}

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

+		ret = IRQ_HANDLED;
+	}
+	mutex_unlock(&data->mutex);
+
+	return ret;

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

+			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, true);
+}
+
+static int bu27008_buffer_postdisable(struct iio_dev *idev)
+{
+	struct bu27008_data *data = iio_priv(idev);
+
+	return bu27008_meas_set(data, false);
+}
+
+static const struct iio_buffer_setup_ops bu27008_buffer_ops = {
+	.preenable = bu27008_buffer_preenable,
+	.postdisable = bu27008_buffer_postdisable,
+};
+
+static int bu27008_probe(struct i2c_client *i2c)
+{
+	struct device *dev = &i2c->dev;

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

+		return -EINVAL;
+	}
+
+	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, &reg);
+	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;
+

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

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

Quoted 8 lines · On Fri, 21 Apr 2023 12:37:30 +0300 Matti Vaittinen <[email protected]>…

Add support for 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.

Quoted 2 lines

This series supports reading the RGBC and IR channels using IIO frameeork. However, only two of the BC+IR can be enabled at the same

framework

Quoted 57 lines

time. Series adds also support for scale and integration time configuration, where scale consists of impact of both the integration time and hardware gain. The gain and time support is backed by the newly introduced IIO GTS helper. This series depends on GTS helper patches added in BU27034 support series which is already merged in iio/togreg which this series is based on.

The hardware allows configuring gain setting by writing a 5-bit gain selector value to a register. Part of the gain setting is common for all channels (RGBC + IR) but part of the selector value can be set separately for RGBC and IR:

MODE_CONTROL2 REG:
bit 7 6 5 4 3 2 1 0
-----------------------------------------------------------------

|	RGB	selector		|
+---------------------------------------+
-----------------------------------------------------------------
| high bits IR	|			| low bits IR selector	|
+---------------+			+-----------------------+

In theory it would be possible to set certain separate gain values for RGBC and IR channels, but this gets pretty confusing because there are a few 'unsupported' selector values. If only RGBC or IR was set, some extra handling should be done to prevent the other channel from getting unsupported value due to change in high-bits. Furthermore, allowing the channels to be set different gain values (in some cases when gains are such the HW supports it) would make the cases where also integration time is changed to achieve correct scale ... interesting. It might also be confusing for user to try predicting when setting different scales succeeds and when it does not. Furthermore, if for example the scale setting for RGBC caused IR selector to be invalid - it could also cause the IR scale to "jump" very far from previous value.

To make the code simpler and more predictable for users, the current logic is as follows:

1. Prevent setting IR scale. (My assumption is IR is less used than RGBC)
2. When RGBC scale is set, set also the IR-selector to the same value. This prevents unsupported selector values and makes the IR scale changes predictable.

The 2) could mean we effectively have the same scale for all channels. Unfortunately, the HW design is slightly peculiar and selector 0 means gain 1X on RGBC but gain 2X on IR. Rest of the selectors equal same gain values on RGBC and IR. The result is that while changin selector from 0 => 1 causes RGBC gain to go from 1X => 4X, it causes IR gain to go from 2X => 4X.

So, the driver provides separate scale entries for all channels (also RGB and C will have separate gain entries because these channels are of same type as IR channel). This makes it possible for user applications to go read the scales for all channels after setting scale for one (in order to detect the IR scale difference).

Quoted 4 lines

Having the separate IR scale entry which applications can read to detect "arbitrary scale changes" makes it possible for applications to be written so they can cope if we need to implement the 'allow setting some different gains for IR and RGBC' - later.

Hmm. I'm not sure preventing it is the best approach. That makes for an interface that is perhaps even less intuitive than having it affect the scales of the other channels. Still having it configurable at all is optional from an ABI point of view, so we could go with what you have here and see if anyone shouts about it in future.

Quoted 11 lines · On Fri, 21 Apr 2023 12:39:36 +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,

Biggest issue in here is some confusion over data packing when some channels are enabled. The driver must pack the channels that are enabled (seen from active_scan_mask) according to general IIO rules. So naturally aligned buffers. Thus for this device it should always be packed into

struct scan {
	__le16 chans[4];
	s64 ts __aligned(8); /*it's aligned anyway but better not to make reviewers think ;) */
};

Even though there are 5 possible channels. If one in the middle isnt' enabled (e.g. blue) then clear and IR shift to lower words. of the buffer.

The demux code in the IIO core then deals with userspace requesting less than this
by repacking the data if needed. That allows it to present different views to different
consumers (e.g. userspace IIO access, and an inkernel buffer user - though there aren't any of those for light sensors AFAIK.

I'd not thought about the issue of the weird scales interacting with someone changing the integration time. That's nasty and I don't have a better suggestion than what you have currently.

Otherwise some trivial stuff inline.

Jonathan

Quoted 6 lines

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

new file mode 100644
index 000000000000..6fca193eeb9e
@@ -0,0 +1,1028 @@

...

Quoted 13 lines

drivers/iio/light/rohm-bu27008.c new file +13 -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
+};
+
+enum {
+	BU27008_DATA0, /* Always RED */
+	BU27008_DATA1, /* Always GREEN */

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

Quoted 10 lines

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

+	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))
+
+#define BU27008_CHAN_DATA_SIZE		2 /* Each channel has 16bits of data */
+#define BU27008_BUF_DATA_SIZE (BU27008_NUM_CHANS * BU27008_CHAN_DATA_SIZE)

Buffer should be same size as the number you can grab in one read. So it should be same as BU27008_HW_DATA_SIZE. The data is packed when only some channels are enabled.

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

Quoted 7 lines

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

+#define BU27008_HW_DATA_SIZE (BU27008_NUM_HW_CHANS * BU27008_CHAN_DATA_SIZE)
+#define NUM_U16_IN_TSTAMP (sizeof(s64) / sizeof(u16))
+
+static const unsigned long bu27008_scan_masks[] = {
+	ALWAYS_SCANNABLE | BIT(BU27008_CLEAR) | BIT(BU27008_IR),
+	ALWAYS_SCANNABLE | BIT(BU27008_CLEAR) | BIT(BU27008_BLUE),
+	ALWAYS_SCANNABLE | BIT(BU27008_BLUE) | BIT(BU27008_IR),

Good, this looks correct for allowing the IIO core to deal with demuxing the data as necessary. Note that this is saying that if one of these modes is active scan mask it will be backed into lowers set of __le16 buffer elements.

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

Quoted 8 lines

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

+static int bu27008_validate_trigger(struct iio_dev *idev,
+				   struct iio_trigger *trig)
+{
+	struct bu27008_data *data = iio_priv(idev);
+
+	if (data->trig != trig)

Hmm. I thought we had a standard function for this, but turns out we only have the one that validates in the other direction. Perhaps it's wroth something generic like iio_device_validate_own_trigger which would be similar to iio_trigger_validate_own_device in that it would use the common parentage of the trigger and iio_dev to check the match.

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

Quoted 6 lines

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

+	ret = regmap_update_bits(data->regmap, BU27008_REG_SYSTEM_CONTROL,
+			   BU27008_MASK_SW_RESET, BU27008_MASK_SW_RESET);
+	if (ret)
+		return dev_err_probe(data->dev, ret, "Sensor reset failed\n");
+
+	msleep(1);

Good to state what reset time min is from datasheet.

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

Quoted 4 lines

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

+
+	/*
+	 * After some measurements, it seems reading the
+	 * BU27008_REG_MODE_CONTROL3 debounces the IRQ line

'it seems' worries me. In docs? Not that we have them but would be nice if this statement was stronger!

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

Quoted 3 lines

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

+
+	if (!i2c->irq) {
+		dev_err(dev, "No IRQ configured\n");

If it's possible to relax this requirement for an IRQ you should definitely do so, even if you loose a lot of functionality. It's annoyingly common for board designers to think it's not necessary to wire up IRQs.

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 BU27008 dt-bindings.

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

---
 .../bindings/iio/light/rohm-bu27008.yaml      | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/rohm-bu27008.yaml

Quoted 10 lines · On Fri, 21 Apr 2023 12:39:36 +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

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

Quoted 10 lines

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

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

+
+static int bu27008_meas_set(struct bu27008_data *data, bool enable)
+{
+	if (enable)
+		return regmap_set_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
+				       BU27008_MASK_MEAS_EN);
+
+	return regmap_clear_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
+				 BU27008_MASK_MEAS_EN);

Might be cleaner with regmap_update_bits()

Quoted 1 line

+}

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

Quoted 8 lines

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

+
+static int bu27008_set_drdy_irq(struct bu27008_data *data, bool state)
+{
+	if (state)
+		return regmap_set_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
+					BU27008_MASK_INT_EN);
+	return regmap_clear_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
+				 BU27008_MASK_INT_EN);

regmap_update_bits() maybe with the mask and value supplied.

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

Quoted 9 lines

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

+}
+
 
+static irqreturn_t bu27008_irq_handler(int irq, void *private)
+{
+	struct iio_dev *idev = private;
+	struct bu27008_data *data = iio_priv(idev);
+
+	data->old_timestamp = data->timestamp;

What is old_timestamp for? Without out setting that, this is the same as iio_pollfunc_store_time() with the timestamp stored in a slightly difference place and always waking the thread (which probably doesn't matter)

Quoted 5 lines

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

+
+static irqreturn_t bu27008_irq_thread_handler(int irq, void *private)
+{
+	struct iio_dev *idev = private;
+	struct bu27008_data *data = iio_priv(idev);

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

Quoted 5 lines

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

+	irqreturn_t ret = IRQ_NONE;
+
+	mutex_lock(&data->mutex);
+	if (data->trigger_enabled) {
+		iio_trigger_poll_nested(data->trig);

Add a comment here on why it makes sense to hold the mutex whilst calling this.

Quoted 8 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 BU27008 dt-bindings.

Quoted 5 lines

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

---
  .../bindings/iio/light/rohm-bu27008.yaml      | 49 +++++++++++++++++++
  1 file changed, 49 insertions(+)

create mode 100644 Documentation/devicetree/bindings/iio/light/rohm-bu27008.yaml

Filename like compatible, so rohm,bu27008.yaml

Documentation/devicetree/bindings/iio/light/rohm-bu27008.yaml line 4

Quoted 12 lines

Documentation/devicetree/bindings/iio/light/rohm-bu27008.yaml new file +4 -0

new file mode 100644
index 000000000000..d942c2817680
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/bu27008.yaml#

Does not look like you tested the bindings. Please run make dt_binding_check (see Documentation/devicetree/bindings/writing-schema.rst for instructions).

Documentation/devicetree/bindings/iio/light/rohm-bu27008.yaml line 12

Quoted 9 lines

Documentation/devicetree/bindings/iio/light/rohm-bu27008.yaml new file +8 -0

+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ROHM BU27008 color sensor
+
+maintainers:
+  - Matti Vaittinen <[email protected]>
+
+description: |

Do not need '|' unless you need to preserve formatting.

Quoted 22 lines · Jonathan Cameron

Quoted 11 lines · On Fri, 21 Apr 2023 12:39:36 +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,

Biggest issue in here is some confusion over data packing when some channels are enabled. The driver must pack the channels that are enabled (seen from active_scan_mask) according to general IIO rules. So naturally aligned buffers. Thus for this device it should always be packed into

Quoted 7 lines

struct scan {
	__le16 chans[4];
	s64 ts __aligned(8); /*it's aligned anyway but better not to make reviewers think ;) */
};

Even though there are 5 possible channels. If one in the middle isnt' enabled (e.g. blue) then clear and IR shift to lower words. of the buffer.

Ah, right. So I had misunderstood how the buffer works. I thought the scan_mask was only used to disallow unsupported channel-enabling configurations. If I understand your statement correctly, the scan_mask is used to determine the 'place of data' in the buffer when certain configuration is used. (I'll check this from the code but if the IIO handles data as I now think - that's cool! It should indeed simplify the buffer in driver side!).

Quoted 13 lines · Jonathan Cameron

Quoted 10 lines · On Fri, 21 Apr 2023 12:39:36 +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

Quoted 14 lines

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

Quoted 10 lines

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

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

+
+static int bu27008_meas_set(struct bu27008_data *data, bool enable)
+{
+	if (enable)
+		return regmap_set_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
+				       BU27008_MASK_MEAS_EN);
+
+	return regmap_clear_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
+				 BU27008_MASK_MEAS_EN);  

Might be cleaner with regmap_update_bits()

Quoted 1 line

+}

Hm. I need to disagree on this although I think it depends on what one is used to :)

For me adding a variable for value to be used is slightly more complex than just using clear or set function depending on the enable/disable. I remember thinking the same as you and preferring the update_bits also on enable/disable cases - until I wrote my first power-supply driver and Sebasian Reichel told me to not do:

int val;

if (foo)

	val = mask;
else
	val = 0;

return regmap_update_bits(regmap, reg, mask, val);

but use set/clear bits. This allows killing the 'int val;'. I remember I had to sleep over night on it but I later started seeing the set/clear bits as a simpler thing.

Sure we could also do

if (foo)

	return regmap_update_bits(map, reg, mask, mask);
else
	return regmap_update_bits(map, reg, mask, 0);

- but here we just replace:

regmap_set_bits(map, reg, mask) with
regmap_update_bits(map, reg, mask, mask)

and

regmap_clear_bits(map, reg, mask)
regmap_update_bits(map, reg, mask, 0)

with longer but functionally same variants - which kind of says "I think the "regmap_set_bits() and regmap_clear_bits()" are useless ;)

After saying this - I can use the regmap_update_bits() if you insist, but in my (not always so) humble opinion this does not improve the function.

Quoted 11 lines

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

Quoted 10 lines

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

+
+static int bu27008_set_drdy_irq(struct bu27008_data *data, bool state)
+{
+	if (state)
+		return regmap_set_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
+					BU27008_MASK_INT_EN);
+	return regmap_clear_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
+				 BU27008_MASK_INT_EN);  

regmap_update_bits() maybe with the mask and value supplied.

Same weak objection here as was with the bu27008_meas_set(). Eg, can change if required but please reconsider :)

Quoted 5 lines

Quoted 3 lines

(quoted diff) +2 -0

+}
+  

Quoted 13 lines

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

Quoted 10 lines

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

+
+static irqreturn_t bu27008_irq_thread_handler(int irq, void *private)
+{
+	struct iio_dev *idev = private;
+	struct bu27008_data *data = iio_priv(idev);
+	irqreturn_t ret = IRQ_NONE;
+
+	mutex_lock(&data->mutex);
+	if (data->trigger_enabled) {
+		iio_trigger_poll_nested(data->trig);  

Add a comment here on why it makes sense to hold the mutex whilst calling this.

After revising this - I don't think it makes. Nor do I think we need the trigger_enable flag so we don't propably need the mutex in buffer enable either as all raw-write configs are claiming the direct mode.