[PATCH 0/3] Support ROHM BU27010 RGBC sensor

[PATCH 0/3] Support ROHM BU27010 RGBC sensor

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2422 bytes --]

Support ROHM BU27010 RGBC + flickering sensor.

Following description copied from commit log:

> The ROHM BU27010 is an RGBC sensor with a flickering detection FIFO. The
> RGBC+IR sensor functionality is largely similar to what the BU27008 has.
> There are some notable things though:
>  - gain setting is once again new and exotic. Now, there is 6bit gain
>    setting where 4 of the bits are common to all channels and 2 bits
>    can be configured separately for each channel. The BU27010 has
>    similar "1X on other channels vs 2X on IR when selector is 0x0"
>    gain design as BU27008 had. So, we use same gain setting policy for
>    BU27010 as we did for BU27008 - driver sets same gain selector for
>    all channels but shows the gains separately for all channels so users
>    can (at least in theory) detect this 1X vs 2X madness...
>  - BU27010 has suffled all the control register bitfields to new
>    addresses and bit positions while still keeping the register naming
>    same.
>  - Some more power/reset control is added.
>  - FIFO for "flickering detection" is added.
>
> The control register suffling made this slightly nasty. Still, it is
> easier for maintenance perspective to add the BU27010 support in BU27008
> driver because - even though the bit positions/addresses were changed -
> most of the driver structure can be re-used. Writing own driver for
> BU27010 would mean plenty of duplicate code albeit a tad more clarity.

This series is done on top of the iio-for-6.5a + this fix-up series:
https://lore.kernel.org/all/cover.1686648422.git.mazziesaccount@gmail.com/

---

Matti Vaittinen (3):
  dt-bindings: ROHM BU27010 RGBC + flickering sensor
  iio: light: bu27008: add chip info
  iio: light: bd27008: Support BD27010 RGB

 .../bindings/iio/light/rohm,bu27010.yaml      |  50 ++
 drivers/iio/light/rohm-bu27008.c              | 504 ++++++++++++++++--
 2 files changed, 500 insertions(+), 54 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/light/rohm,bu27010.yaml

-- 
2.40.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 1/3] dt-bindings: ROHM BU27010 RGBC + flickering sensor

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2497 bytes --]

The ROHM BU27010 is a sensor with 6 photodiodes (red, green, blue, clear,
IR and flickering detection) with five configurable channels. Red, green
and flickering detection 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/OLED backlight of TVs, mobile phones
and tablet PCs.

Add binding document for ROHM BU27010.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 .../bindings/iio/light/rohm,bu27010.yaml      | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/rohm,bu27010.yaml

diff --git a/Documentation/devicetree/bindings/iio/light/rohm,bu27010.yaml b/Documentation/devicetree/bindings/iio/light/rohm,bu27010.yaml
new file mode 100644
index 000000000000..2bde9d2f1def
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/rohm,bu27010.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/rohm,bu27010.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ROHM BU27010 color sensor
+
+maintainers:
+  - Matti Vaittinen <mazziesaccount@gmail.com>
+
+description: |
+  The ROHM BU27010 is a sensor with 6 photodiodes (red, green, blue, clear,
+  IR and flickering detection) with five configurable channels. Red, green
+  and flickering detection 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/OLED backlight of TVs, mobile phones
+  and tablet PCs.
+
+properties:
+  compatible:
+    const: rohm,bu27010
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  vdd-supply: true
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      light-sensor@38 {
+        compatible = "rohm,bu27010";
+        reg = <0x38>;
+      };
+    };
+
-- 
2.40.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] dt-bindings: ROHM BU27010 RGBC + flickering sensor

[Krzysztof Kozlowski]

On 13/06/2023 12:19, Matti Vaittinen wrote:
> The ROHM BU27010 is a sensor with 6 photodiodes (red, green, blue, clear,
> IR and flickering detection) with five configurable channels. Red, green
> and flickering detection 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/OLED backlight of TVs, mobile phones
> and tablet PCs.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.
e.g.: "dt-bindings: iio:"


> 
> Add binding document for ROHM BU27010.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> ---
>  .../bindings/iio/light/rohm,bu27010.yaml      | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/light/rohm,bu27010.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/rohm,bu27010.yaml b/Documentation/devicetree/bindings/iio/light/rohm,bu27010.yaml
> new file mode 100644
> index 000000000000..2bde9d2f1def
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/rohm,bu27010.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/light/rohm,bu27010.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ROHM BU27010 color sensor
> +
> +maintainers:
> +  - Matti Vaittinen <mazziesaccount@gmail.com>
> +
> +description: |
> +  The ROHM BU27010 is a sensor with 6 photodiodes (red, green, blue, clear,
> +  IR and flickering detection) with five configurable channels. Red, green
> +  and flickering detection 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/OLED backlight of TVs, mobile phones
> +  and tablet PCs.
> +
> +properties:
> +  compatible:
> +    const: rohm,bu27010
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  vdd-supply: true

Isn't vdd-supply required for the hardware to work? How does it get the
power otherwise?

> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      light-sensor@38 {
> +        compatible = "rohm,bu27010";
> +        reg = <0x38>;
> +      };
> +    };
> +

Trailing blank line.

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: ROHM BU27010 RGBC + flickering sensor

[Matti Vaittinen]

On 6/13/23 21:42, Krzysztof Kozlowski wrote:
> On 13/06/2023 12:19, Matti Vaittinen wrote:
>> The ROHM BU27010 is a sensor with 6 photodiodes (red, green, blue, clear,
>> IR and flickering detection) with five configurable channels. Red, green
>> and flickering detection 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/OLED backlight of TVs, mobile phones
>> and tablet PCs.
> 
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching.
> e.g.: "dt-bindings: iio:"
> 

Right, thanks!

> 
>>
>> Add binding document for ROHM BU27010.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>> ---
>>   .../bindings/iio/light/rohm,bu27010.yaml      | 49 +++++++++++++++++++
>>   1 file changed, 49 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/iio/light/rohm,bu27010.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/iio/light/rohm,bu27010.yaml b/Documentation/devicetree/bindings/iio/light/rohm,bu27010.yaml
>> new file mode 100644
>> index 000000000000..2bde9d2f1def
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/light/rohm,bu27010.yaml
>> @@ -0,0 +1,49 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iio/light/rohm,bu27010.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: ROHM BU27010 color sensor
>> +
>> +maintainers:
>> +  - Matti Vaittinen <mazziesaccount@gmail.com>
>> +
>> +description: |
>> +  The ROHM BU27010 is a sensor with 6 photodiodes (red, green, blue, clear,
>> +  IR and flickering detection) with five configurable channels. Red, green
>> +  and flickering detection 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/OLED backlight of TVs, mobile phones
>> +  and tablet PCs.
>> +
>> +properties:
>> +  compatible:
>> +    const: rohm,bu27010
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  vdd-supply: true
> 
> Isn't vdd-supply required for the hardware to work? How does it get the
> power otherwise?

The BU27010 works by magic smoke. When smoke leaks out the sensor no 
longer works.

Ehh, I think you are right. My thinking was that it is not uncommon for 
people to not have all fixed regulators present in device-tree. But I 
agree, this does not mean we should encourage that. I'll add supply to 
the list of the required properties.

> 
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      light-sensor@38 {
>> +        compatible = "rohm,bu27010";
>> +        reg = <0x38>;
>> +      };
>> +    };
>> +
> 
> Trailing blank line.

Indeed. Thanks.

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

Re: [PATCH 1/3] dt-bindings: ROHM BU27010 RGBC + flickering sensor

[Krzysztof Kozlowski]

On 14/06/2023 07:32, Matti Vaittinen wrote:
>>> +properties:
>>> +  compatible:
>>> +    const: rohm,bu27010
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  vdd-supply: true
>>
>> Isn't vdd-supply required for the hardware to work? How does it get the
>> power otherwise?
> 
> The BU27010 works by magic smoke. When smoke leaks out the sensor no 
> longer works.
> 
> Ehh, I think you are right. My thinking was that it is not uncommon for 
> people to not have all fixed regulators present in device-tree. But I 
> agree, this does not mean we should encourage that. I'll add supply to 
> the list of the required properties.
> 
If the device actually requires a regulator, it should be required by
the bindings, even if it leads to dummies or fixed regulators on the
board DTS.

Best regards,
Krzysztof

[PATCH 2/3] iio: light: bu27008: add chip info

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 13027 bytes --]

The ROHM BU27010 RGB + flickering sensor is in many regards similar to
the BU27008. Prepare for adding support for BU27010 by allowing
chip-specific properties to be brought from the of_device_id data.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
 drivers/iio/light/rohm-bu27008.c | 185 +++++++++++++++++++++++--------
 1 file changed, 138 insertions(+), 47 deletions(-)

diff --git a/drivers/iio/light/rohm-bu27008.c b/drivers/iio/light/rohm-bu27008.c
index b50bf8973d9a..8c7f6f20a523 100644
--- a/drivers/iio/light/rohm-bu27008.c
+++ b/drivers/iio/light/rohm-bu27008.c
@@ -211,7 +211,33 @@ static const struct iio_chan_spec bu27008_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(BU27008_NUM_CHANS),
 };
 
+struct bu27008_data;
+
+struct bu27_chip_data {
+	const char *name;
+	int (*chip_init)(struct bu27008_data *data);
+	int (*get_gain_sel)(struct bu27008_data *data, int *sel);
+	int (*write_gain_sel)(struct bu27008_data *data, int sel);
+	const struct regmap_config *regmap_cfg;
+	const struct iio_gain_sel_pair *gains;
+	const struct iio_gain_sel_pair *gains_ir;
+	int num_gains;
+	int num_gains_ir;
+	int scale1x;
+
+	int drdy_en_reg;
+	int drdy_en_mask;
+	int meas_en_reg;
+	int meas_en_mask;
+	int valid_reg;
+	int chan_sel_reg;
+	int chan_sel_mask;
+	int int_time_mask;
+	u8 part_id;
+};
+
 struct bu27008_data {
+	const struct bu27_chip_data *cd;
 	struct regmap *regmap;
 	struct iio_trigger *trig;
 	struct device *dev;
@@ -282,6 +308,32 @@ static const struct regmap_config bu27008_regmap = {
 	.disable_locking = true,
 };
 
+static int bu27008_chip_init(struct bu27008_data *data);
+static int bu27008_write_gain_sel(struct bu27008_data *data, int sel);
+static int bu27008_get_gain_sel(struct bu27008_data *data, int *sel);
+
+static const struct bu27_chip_data bu27008_chip = {
+	.name = "bu27008",
+	.chip_init = bu27008_chip_init,
+	.scale1x = BU27008_SCALE_1X,
+	.get_gain_sel = bu27008_get_gain_sel,
+	.write_gain_sel = bu27008_write_gain_sel,
+	.part_id = BU27008_ID,
+	.regmap_cfg = &bu27008_regmap,
+	.drdy_en_reg = BU27008_REG_MODE_CONTROL3,
+	.drdy_en_mask = BU27008_MASK_INT_EN,
+	.valid_reg = BU27008_REG_MODE_CONTROL3,
+	.meas_en_reg = BU27008_REG_MODE_CONTROL3,
+	.meas_en_mask = BU27008_MASK_MEAS_EN,
+	.chan_sel_reg = BU27008_REG_MODE_CONTROL3,
+	.chan_sel_mask = BU27008_MASK_CHAN_SEL,
+	.int_time_mask = BU27008_MASK_MEAS_MODE,
+	.gains = &bu27008_gains[0],
+	.num_gains = ARRAY_SIZE(bu27008_gains),
+	.gains_ir = &bu27008_gains_ir[0],
+	.num_gains_ir = ARRAY_SIZE(bu27008_gains_ir),
+};
+
 #define BU27008_MAX_VALID_RESULT_WAIT_US	50000
 #define BU27008_VALID_RESULT_WAIT_QUANTA_US	1000
 
@@ -290,7 +342,7 @@ static int bu27008_chan_read_data(struct bu27008_data *data, int reg, int *val)
 	int ret, valid;
 	__le16 tmp;
 
-	ret = regmap_read_poll_timeout(data->regmap, BU27008_REG_MODE_CONTROL3,
+	ret = regmap_read_poll_timeout(data->regmap, data->cd->valid_reg,
 				       valid, (valid & BU27008_MASK_VALID),
 				       BU27008_VALID_RESULT_WAIT_QUANTA_US,
 				       BU27008_MAX_VALID_RESULT_WAIT_US);
@@ -306,16 +358,40 @@ static int bu27008_chan_read_data(struct bu27008_data *data, int reg, int *val)
 	return ret;
 }
 
+static int bu27008_get_gain_sel(struct bu27008_data *data, int *sel)
+{
+	int ret;
+
+	/*
+	 * If we always "lock" the gain selectors for all channels to prevent
+	 * unsupported configs, then it does not matter which channel is used
+	 * we can just return selector from any of them.
+	 *
+	 * This, however is not true if we decide to support only 4X and 16X
+	 * and then individual gains for channels. Currently this is not the
+	 * case.
+	 *
+	 * If we some day decide to support individual gains, then we need to
+	 * have channel information here.
+	 */
+
+	ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL2, sel);
+	if (ret)
+		return ret;
+
+	*sel = FIELD_GET(BU27008_MASK_RGBC_GAIN, *sel);
+
+	return 0;
+}
+
 static int bu27008_get_gain(struct bu27008_data *data, struct iio_gts *gts, int *gain)
 {
 	int ret, sel;
 
-	ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL2, &sel);
+	ret = bu27008_get_gain_sel(data, &sel);
 	if (ret)
 		return ret;
 
-	sel = FIELD_GET(BU27008_MASK_RGBC_GAIN, sel);
-
 	ret = iio_gts_find_gain_by_sel(gts, sel);
 	if (ret < 0) {
 		dev_err(data->dev, "unknown gain value 0x%x\n", sel);
@@ -384,15 +460,21 @@ 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);
+
+	val &= data->cd->int_time_mask;
+	val >>= ffs(data->cd->int_time_mask) - 1;
+
+	*sel = val;
 
 	return ret;
 }
 
 static int bu27008_set_int_time_sel(struct bu27008_data *data, int sel)
 {
+	sel <<= ffs(data->cd->int_time_mask) - 1;
+
 	return regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL1,
-				  BU27008_MASK_MEAS_MODE, sel);
+				  data->cd->int_time_mask, sel);
 }
 
 static int bu27008_get_int_time_us(struct bu27008_data *data)
@@ -448,8 +530,7 @@ static int bu27008_set_int_time(struct bu27008_data *data, int time)
 	if (ret < 0)
 		return ret;
 
-	return regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL1,
-				  BU27008_MASK_MEAS_MODE, ret);
+	return bu27008_set_int_time_sel(data, ret);
 }
 
 /* Try to change the time so that the scale is maintained */
@@ -527,10 +608,13 @@ static int bu27008_try_set_int_time(struct bu27008_data *data, int int_time_new)
 	return ret;
 }
 
-static int bu27008_meas_set(struct bu27008_data *data, int state)
+static int bu27008_meas_set(struct bu27008_data *data, bool enable)
 {
-	return regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
-				  BU27008_MASK_MEAS_EN, state);
+	if (enable)
+		return regmap_set_bits(data->regmap, data->cd->meas_en_reg,
+				       data->cd->meas_en_mask);
+	return regmap_clear_bits(data->regmap, data->cd->meas_en_reg,
+				 data->cd->meas_en_mask);
 }
 
 static int bu27008_chan_cfg(struct bu27008_data *data,
@@ -543,9 +627,15 @@ static int bu27008_chan_cfg(struct bu27008_data *data,
 	else
 		chan_sel = BU27008_CLEAR2_IR3;
 
-	chan_sel = FIELD_PREP(BU27008_MASK_CHAN_SEL, chan_sel);
+	/*
+	 * prepare bitfield for channel sel. The FIELD_PREP works only when
+	 * mask is constant. In our case the mask is assigned based on the
+	 * chip type. Hence the open-coded FIELD_PREP here. We don't bother
+	 * zeroing the irrelevant bits though - update_bits takes care of that.
+	 */
+	chan_sel <<= ffs(data->cd->chan_sel_mask) - 1;
 
-	return regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
+	return regmap_update_bits(data->regmap, data->cd->chan_sel_reg,
 				  BU27008_MASK_CHAN_SEL, chan_sel);
 }
 
@@ -558,7 +648,7 @@ static int bu27008_read_one(struct bu27008_data *data, struct iio_dev *idev,
 	if (ret)
 		return ret;
 
-	ret = bu27008_meas_set(data, BU27008_MEAS_EN);
+	ret = bu27008_meas_set(data, true);
 	if (ret)
 		return ret;
 
@@ -574,7 +664,7 @@ static int bu27008_read_one(struct bu27008_data *data, struct iio_dev *idev,
 	if (!ret)
 		ret = IIO_VAL_INT;
 
-	if (bu27008_meas_set(data, BU27008_MEAS_DIS))
+	if (bu27008_meas_set(data, false))
 		dev_warn(data->dev, "measurement disabling failed\n");
 
 	return ret;
@@ -762,10 +852,10 @@ static int bu27008_update_scan_mode(struct iio_dev *idev,
 		chan_sel = BU27008_CLEAR2_IR3;
 	}
 
-	chan_sel = FIELD_PREP(BU27008_MASK_CHAN_SEL, chan_sel);
+	chan_sel <<= ffs(data->cd->chan_sel_mask) - 1;
 
-	return regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
-				 BU27008_MASK_CHAN_SEL, chan_sel);
+	return regmap_update_bits(data->regmap, data->cd->chan_sel_reg,
+				  data->cd->chan_sel_mask, chan_sel);
 }
 
 static const struct iio_info bu27008_info = {
@@ -794,29 +884,25 @@ static int bu27008_chip_init(struct bu27008_data *data)
 	 */
 	msleep(1);
 
-	ret = regmap_reinit_cache(data->regmap, &bu27008_regmap);
+	ret = regmap_reinit_cache(data->regmap, data->cd->regmap_cfg);
 	if (ret)
 		dev_err(data->dev, "Failed to reinit reg cache\n");
 
 	return ret;
 }
 
-static int bu27008_set_drdy_irq(struct bu27008_data *data, int state)
-{
-	return regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
-				 BU27008_MASK_INT_EN, state);
-}
-
-static int bu27008_trigger_set_state(struct iio_trigger *trig,
-				     bool state)
+static int bu27008_trigger_set_state(struct iio_trigger *trig, bool state)
 {
 	struct bu27008_data *data = iio_trigger_get_drvdata(trig);
 	int ret;
 
+
 	if (state)
-		ret = bu27008_set_drdy_irq(data, BU27008_INT_EN);
+		ret = regmap_set_bits(data->regmap, data->cd->drdy_en_reg,
+				      data->cd->drdy_en_mask);
 	else
-		ret = bu27008_set_drdy_irq(data, BU27008_INT_DIS);
+		ret = regmap_clear_bits(data->regmap, data->cd->drdy_en_reg,
+					data->cd->drdy_en_mask);
 	if (ret)
 		dev_err(data->dev, "Failed to set trigger state\n");
 
@@ -852,7 +938,7 @@ static irqreturn_t bu27008_trigger_handler(int irq, void *p)
 	 * 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);
+	ret = regmap_read(data->regmap, data->cd->valid_reg, &dummy);
 	if (ret < 0)
 		goto err_read;
 
@@ -872,14 +958,14 @@ static int bu27008_buffer_preenable(struct iio_dev *idev)
 {
 	struct bu27008_data *data = iio_priv(idev);
 
-	return bu27008_meas_set(data, BU27008_MEAS_EN);
+	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, BU27008_MEAS_DIS);
+	return bu27008_meas_set(data, false);
 }
 
 static const struct iio_buffer_setup_ops bu27008_buffer_ops = {
@@ -952,11 +1038,6 @@ static int bu27008_probe(struct i2c_client *i2c)
 	struct iio_dev *idev;
 	int ret;
 
-	regmap = devm_regmap_init_i2c(i2c, &bu27008_regmap);
-	if (IS_ERR(regmap))
-		return dev_err_probe(dev, PTR_ERR(regmap),
-				     "Failed to initialize Regmap\n");
-
 	idev = devm_iio_device_alloc(dev, sizeof(*data));
 	if (!idev)
 		return -ENOMEM;
@@ -967,23 +1048,33 @@ static int bu27008_probe(struct i2c_client *i2c)
 
 	data = iio_priv(idev);
 
+	data->cd = device_get_match_data(&i2c->dev);
+	if (!data->cd)
+		return -ENODEV;
+
+	regmap = devm_regmap_init_i2c(i2c, data->cd->regmap_cfg);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "Failed to initialize Regmap\n");
+
+
 	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)
+	if (part_id != data->cd->part_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,
+	ret = devm_iio_init_iio_gts(dev, data->cd->scale1x, 0, data->cd->gains,
+				    data->cd->num_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,
+	ret = devm_iio_init_iio_gts(dev, data->cd->scale1x, 0, data->cd->gains_ir,
+				    data->cd->num_gains_ir, bu27008_itimes,
 				    ARRAY_SIZE(bu27008_itimes), &data->gts_ir);
 	if (ret)
 		return ret;
@@ -993,14 +1084,14 @@ static int bu27008_probe(struct i2c_client *i2c)
 	data->dev = dev;
 	data->irq = i2c->irq;
 
-	idev->channels = bu27008_channels;
-	idev->num_channels = ARRAY_SIZE(bu27008_channels);
-	idev->name = "bu27008";
+	idev->channels = /* data->cd->cspec; */ &bu27008_channels[0];
+	idev->num_channels = /* data->cd->num_channels; */ ARRAY_SIZE(bu27008_channels);
+	idev->name = data->cd->name;
 	idev->info = &bu27008_info;
 	idev->modes = INDIO_DIRECT_MODE;
 	idev->available_scan_masks = bu27008_scan_masks;
 
-	ret = bu27008_chip_init(data);
+	ret = data->cd->chip_init(data);
 	if (ret)
 		return ret;
 
@@ -1021,7 +1112,7 @@ static int bu27008_probe(struct i2c_client *i2c)
 }
 
 static const struct of_device_id bu27008_of_match[] = {
-	{ .compatible = "rohm,bu27008" },
+	{ .compatible = "rohm,bu27008", .data = &bu27008_chip },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, bu27008_of_match);
-- 
2.40.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/3] iio: light: bu27008: add chip info

[Jonathan Cameron]

On Tue, 13 Jun 2023 13:20:07 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> The ROHM BU27010 RGB + flickering sensor is in many regards similar to
> the BU27008. Prepare for adding support for BU27010 by allowing
> chip-specific properties to be brought from the of_device_id data.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
A few things inline - including some commented out code you missed
when tidying up before sending.

Jonathan

> ---
>  drivers/iio/light/rohm-bu27008.c | 185 +++++++++++++++++++++++--------
>  1 file changed, 138 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/iio/light/rohm-bu27008.c b/drivers/iio/light/rohm-bu27008.c
> index b50bf8973d9a..8c7f6f20a523 100644
> --- a/drivers/iio/light/rohm-bu27008.c
> +++ b/drivers/iio/light/rohm-bu27008.c
> @@ -211,7 +211,33 @@ static const struct iio_chan_spec bu27008_channels[] = {
>  	IIO_CHAN_SOFT_TIMESTAMP(BU27008_NUM_CHANS),
>  };
>  
> +struct bu27008_data;
> +
> +struct bu27_chip_data {
> +	const char *name;
> +	int (*chip_init)(struct bu27008_data *data);
> +	int (*get_gain_sel)(struct bu27008_data *data, int *sel);
> +	int (*write_gain_sel)(struct bu27008_data *data, int sel);
> +	const struct regmap_config *regmap_cfg;
> +	const struct iio_gain_sel_pair *gains;
> +	const struct iio_gain_sel_pair *gains_ir;
> +	int num_gains;
> +	int num_gains_ir;
> +	int scale1x;
> +
> +	int drdy_en_reg;
> +	int drdy_en_mask;
> +	int meas_en_reg;
> +	int meas_en_mask;
> +	int valid_reg;
> +	int chan_sel_reg;
> +	int chan_sel_mask;
> +	int int_time_mask;
> +	u8 part_id;
> +};
> +
>  struct bu27008_data {
> +	const struct bu27_chip_data *cd;
>  	struct regmap *regmap;
>  	struct iio_trigger *trig;
>  	struct device *dev;
> @@ -282,6 +308,32 @@ static const struct regmap_config bu27008_regmap = {
>  	.disable_locking = true,
>  };
>  
> +static int bu27008_chip_init(struct bu27008_data *data);
> +static int bu27008_write_gain_sel(struct bu27008_data *data, int sel);
> +static int bu27008_get_gain_sel(struct bu27008_data *data, int *sel);
> +
> +static const struct bu27_chip_data bu27008_chip = {
> +	.name = "bu27008",
> +	.chip_init = bu27008_chip_init,
> +	.scale1x = BU27008_SCALE_1X,

I'd keep this in same order as the definition unless there is a
strong reason for a different ordering (perhaps the structure
is ordered for packing purposes or something like that and assigning
can be done in an order that groups things better?)
Cost of out of order is that it's hard to check if everything is assigned.

> +	.get_gain_sel = bu27008_get_gain_sel,
> +	.write_gain_sel = bu27008_write_gain_sel,
> +	.part_id = BU27008_ID,
> +	.regmap_cfg = &bu27008_regmap,
> +	.drdy_en_reg = BU27008_REG_MODE_CONTROL3,
> +	.drdy_en_mask = BU27008_MASK_INT_EN,
> +	.valid_reg = BU27008_REG_MODE_CONTROL3,
> +	.meas_en_reg = BU27008_REG_MODE_CONTROL3,
> +	.meas_en_mask = BU27008_MASK_MEAS_EN,
> +	.chan_sel_reg = BU27008_REG_MODE_CONTROL3,
> +	.chan_sel_mask = BU27008_MASK_CHAN_SEL,
> +	.int_time_mask = BU27008_MASK_MEAS_MODE,
> +	.gains = &bu27008_gains[0],
> +	.num_gains = ARRAY_SIZE(bu27008_gains),
> +	.gains_ir = &bu27008_gains_ir[0],
> +	.num_gains_ir = ARRAY_SIZE(bu27008_gains_ir),
> +};

Could you move this down to below all the callbacks so that no need for forward
definitions of the functions? 

> +
>  #define BU27008_MAX_VALID_RESULT_WAIT_US	50000
>  #define BU27008_VALID_RESULT_WAIT_QUANTA_US	1000

> -	idev->channels = bu27008_channels;
> -	idev->num_channels = ARRAY_SIZE(bu27008_channels);
> -	idev->name = "bu27008";
> +	idev->channels = /* data->cd->cspec; */ &bu27008_channels[0];
?   Seems you didn't put the channels cd

> +	idev->num_channels = /* data->cd->num_channels; */ ARRAY_SIZE(bu27008_channels);
?


> +	idev->name = data->cd->name;
>  	idev->info = &bu27008_info;
>  	idev->modes = INDIO_DIRECT_MODE;
>  	idev->available_scan_masks = bu27008_scan_masks;
>  
> -	ret = bu27008_chip_init(data);
> +	ret = data->cd->chip_init(data);
>  	if (ret)
>  		return ret;
>  
> @@ -1021,7 +1112,7 @@ static int bu27008_probe(struct i2c_client *i2c)
>  }
>  
>  static const struct of_device_id bu27008_of_match[] = {
> -	{ .compatible = "rohm,bu27008" },
> +	{ .compatible = "rohm,bu27008", .data = &bu27008_chip },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, bu27008_of_match);

Re: [PATCH 2/3] iio: light: bu27008: add chip info

[Matti Vaittinen]

Thanks for the reviews Jonathan!

I am a bit of overloaded right now so it may be reworking this series 
will be postponed. Let's see. I will in any case take your feedback with 
me and come back with the V2 of this series - later if not sooner :)

On 6/17/23 22:48, Jonathan Cameron wrote:
> On Tue, 13 Jun 2023 13:20:07 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> The ROHM BU27010 RGB + flickering sensor is in many regards similar to
>> the BU27008. Prepare for adding support for BU27010 by allowing
>> chip-specific properties to be brought from the of_device_id data.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> A few things inline - including some commented out code you missed
> when tidying up before sending.

Ouch. I must've done some of the tidying in latter patches. I'll do the 
necessary cleanups and re-spin.

> 
> Jonathan
> 
>> ---
>>   drivers/iio/light/rohm-bu27008.c | 185 +++++++++++++++++++++++--------
>>   1 file changed, 138 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/iio/light/rohm-bu27008.c b/drivers/iio/light/rohm-bu27008.c
>> index b50bf8973d9a..8c7f6f20a523 100644
>> --- a/drivers/iio/light/rohm-bu27008.c
>> +++ b/drivers/iio/light/rohm-bu27008.c
>> @@ -211,7 +211,33 @@ static const struct iio_chan_spec bu27008_channels[] = {
>>   	IIO_CHAN_SOFT_TIMESTAMP(BU27008_NUM_CHANS),
>>   };
>>   
>> +struct bu27008_data;
>> +
>> +struct bu27_chip_data {
>> +	const char *name;
>> +	int (*chip_init)(struct bu27008_data *data);
>> +	int (*get_gain_sel)(struct bu27008_data *data, int *sel);
>> +	int (*write_gain_sel)(struct bu27008_data *data, int sel);
>> +	const struct regmap_config *regmap_cfg;
>> +	const struct iio_gain_sel_pair *gains;
>> +	const struct iio_gain_sel_pair *gains_ir;
>> +	int num_gains;
>> +	int num_gains_ir;
>> +	int scale1x;
>> +
>> +	int drdy_en_reg;
>> +	int drdy_en_mask;
>> +	int meas_en_reg;
>> +	int meas_en_mask;
>> +	int valid_reg;
>> +	int chan_sel_reg;
>> +	int chan_sel_mask;
>> +	int int_time_mask;
>> +	u8 part_id;
>> +};
>> +
>>   struct bu27008_data {
>> +	const struct bu27_chip_data *cd;
>>   	struct regmap *regmap;
>>   	struct iio_trigger *trig;
>>   	struct device *dev;
>> @@ -282,6 +308,32 @@ static const struct regmap_config bu27008_regmap = {
>>   	.disable_locking = true,
>>   };
>>   
>> +static int bu27008_chip_init(struct bu27008_data *data);
>> +static int bu27008_write_gain_sel(struct bu27008_data *data, int sel);
>> +static int bu27008_get_gain_sel(struct bu27008_data *data, int *sel);
>> +
>> +static const struct bu27_chip_data bu27008_chip = {
>> +	.name = "bu27008",
>> +	.chip_init = bu27008_chip_init,
>> +	.scale1x = BU27008_SCALE_1X,
> 
> I'd keep this in same order as the definition unless there is a
> strong reason for a different ordering (perhaps the structure
> is ordered for packing purposes or something like that 

I tried avoid adding much of padding. Didn't go through the embedded 
structs to see alignment though.

I don't think this is a strong reason though. I don't expect many copies 
of these structs being instantiated.

> and assigning
> can be done in an order that groups things better?)

Yes. I do like having some grouping there.

> Cost of out of order is that it's hard to check if everything is assigned.

I'll revise the order. Thanks for pointing this out.

>> +	.get_gain_sel = bu27008_get_gain_sel,
>> +	.write_gain_sel = bu27008_write_gain_sel,
>> +	.part_id = BU27008_ID,
>> +	.regmap_cfg = &bu27008_regmap,
>> +	.drdy_en_reg = BU27008_REG_MODE_CONTROL3,
>> +	.drdy_en_mask = BU27008_MASK_INT_EN,
>> +	.valid_reg = BU27008_REG_MODE_CONTROL3,
>> +	.meas_en_reg = BU27008_REG_MODE_CONTROL3,
>> +	.meas_en_mask = BU27008_MASK_MEAS_EN,
>> +	.chan_sel_reg = BU27008_REG_MODE_CONTROL3,
>> +	.chan_sel_mask = BU27008_MASK_CHAN_SEL,
>> +	.int_time_mask = BU27008_MASK_MEAS_MODE,
>> +	.gains = &bu27008_gains[0],
>> +	.num_gains = ARRAY_SIZE(bu27008_gains),
>> +	.gains_ir = &bu27008_gains_ir[0],
>> +	.num_gains_ir = ARRAY_SIZE(bu27008_gains_ir),
>> +};
> 
> Could you move this down to below all the callbacks so that no need for forward
> definitions of the functions?

Well, I will see how it works. I think there were some dependency - 
chip_info is probably embedded in struct bu27008_data - which is needed 
in these functions - but I'll check this.


Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

[PATCH 3/3] iio: light: bd27008: Support BD27010 RGB

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 20500 bytes --]

The ROHM BU27010 is an RGBC sensor with a flickering detection FIFO. The
RGBC+IR sensor functionality is largely similar to what the BU27008 has.
There are some notable things though:
  - gain setting is once again new and exotic. Now, there is 6bit gain
    setting where 4 of the bits are common to all channels and 2 bits
    can be configured separately for each channel. The BU27010 has
    similar "1X on other channels vs 2X on IR when selector is 0x0"
    gain design as BU27008 had. So, we use same gain setting policy for
    BU27010 as we did for BU27008 - driver sets same gain selector for all
    channels but shows the gains separately for all channels so users
    can (at least in theory) detect this 1X vs 2X madness...
  - BU27010 has suffled all the control register bitfields to new
    addresses and bit positions while still keeping the register naming
    same.
  - Some more power/reset control is added.
  - FIFO for "flickering detection" is added.

The control register suffling made this slightly nasty. Still, it is
easier for maintenance perspective to add the BU27010 support in BU27008
driver because - even though the bit positions/addresses were changed -
most of the driver structure can be re-used. Writing own driver for
BU27010 would mean plenty of duplicate code albeit a tad more clarity.

The flickering FIFO is not supported by the driver.

Add BU27010 RGBC+IR support to rohm-bu27008 driver.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

---
This is my 4.th re-write. My first idea was to write separate drivers
for the BU27008 and BU27010. I abandoned this as I ended up copying most
of the functions and just changing some of the bit offsets / register
addresses. This felt like a plenty of copy-paste.

So, 2.nd attempt was to combine BU27008 and BU27010. I hit the wall
as I tried to support setting RGBC / IR gains independently for BU27008
and setting the gain independently for each channel on BU27010 when
gains were such that the HW could support them independently. This went
to complete madness.

This is when I asked for a second opinion - which was to clear the mess
by separating the BU27008 and BU27010 in own drivers. After a while it
become obvious that supporting independet gains for the channels when HW
supported it required decisions which could not predictably be done for
all users. There is a comment explaining some of this in
bu27008_write_gain_sel().

After the complexity of shared gain bits was killed, the two separate
drivers started to look like a copy-paste again - and all the bugs had
to be fixed in two places... As a result I again pulled it all in one
driver. As a result of this experimenting, I firmly believe supporting
both ICs in the same driver is a correct design.
---
 drivers/iio/light/rohm-bu27008.c | 325 ++++++++++++++++++++++++++++++-
 1 file changed, 315 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/light/rohm-bu27008.c b/drivers/iio/light/rohm-bu27008.c
index 8c7f6f20a523..ed14fd9397e8 100644
--- a/drivers/iio/light/rohm-bu27008.c
+++ b/drivers/iio/light/rohm-bu27008.c
@@ -1,6 +1,8 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
- * BU27008 ROHM Colour Sensor
+ * ROHM Colour Sensor driver for
+ * - BU27008 RGBC sensor
+ * - BU27010 RGBC + Flickering sensor
  *
  * Copyright (c) 2023, ROHM Semiconductor.
  */
@@ -22,6 +24,25 @@
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/triggered_buffer.h>
 
+/*
+ * A word about register address and mask definitions.
+ *
+ * At a quick glance to the data-sheet register tables, the BU27010 has all the
+ * registers that the BU27008 has. On top of that the BU27010 adds couple of new
+ * ones.
+ *
+ * So, all definitions BU27008_REG_* are there also for BU27010 but none of the
+ * BU27010_REG_* are present on BU27008. This makes sense as BU27010 just adds
+ * some features (Flicker FIFO, more power control) on top of the BU27008.
+ *
+ * Unfortunately, some of the wheel has been re-invented. Even though the names
+ * of the registers have stayed the same, pretty much all of the functionality
+ * provided by the registers has changed place. Contents of all MODE_CONTROL
+ * registers on BU27008 and BU27010 are different.
+ *
+ * Chip-specific mapping from register addresses/bits to functionality is done
+ * in bu27_chip_data structures.
+ */
 #define BU27008_REG_SYSTEM_CONTROL	0x40
 #define BU27008_MASK_SW_RESET		BIT(7)
 #define BU27008_MASK_PART_ID		GENMASK(5, 0)
@@ -52,6 +73,56 @@
 #define BU27008_REG_MANUFACTURER_ID	0x92
 #define BU27008_REG_MAX BU27008_REG_MANUFACTURER_ID
 
+/* BU27010 specific definitions */
+
+#define BU27010_MASK_SW_RESET		BIT(7)
+#define BU27010_ID			0x1b
+#define BU27010_REG_POWER		0x3e
+#define BU27010_MASK_POWER		BIT(0)
+
+#define BU27010_REG_RESET		0x3f
+#define BU27010_MASK_RESET		BIT(0)
+#define BU27010_RESET_RELEASE		BU27010_MASK_RESET
+
+#define BU27010_MASK_MEAS_EN		BIT(1)
+
+#define BU27010_MASK_CHAN_SEL		GENMASK(7, 6)
+#define BU27010_MASK_MEAS_MODE		GENMASK(5, 4)
+#define BU27010_MASK_RGBC_GAIN		GENMASK(3, 0)
+
+#define BU27010_MASK_DATA3_GAIN		GENMASK(7, 6)
+#define BU27010_MASK_DATA2_GAIN		GENMASK(5, 4)
+#define BU27010_MASK_DATA1_GAIN		GENMASK(3, 2)
+#define BU27010_MASK_DATA0_GAIN		GENMASK(1, 0)
+
+#define BU27010_MASK_FLC_MODE		BIT(7)
+#define BU27010_MASK_FLC_GAIN		GENMASK(4, 0)
+
+#define BU27010_REG_MODE_CONTROL4	0x44
+/* If flicker is ever to be supported the IRQ must be handled as a field */
+#define BU27010_IRQ_DIS_ALL		GENMASK(1, 0)
+#define BU27010_DRDY_EN			BIT(0)
+#define BU27010_MASK_INT_SEL		GENMASK(1, 0)
+
+#define BU27010_REG_MODE_CONTROL5	0x45
+#define BU27010_MASK_RGB_VALID		BIT(7)
+#define BU27010_MASK_FLC_VALID		BIT(6)
+#define BU27010_MASK_WAIT_EN		BIT(3)
+#define BU27010_MASK_FIFO_EN		BIT(2)
+#define BU27010_MASK_RGB_EN		BIT(1)
+#define BU27010_MASK_FLC_EN		BIT(0)
+
+#define BU27010_REG_DATA_FLICKER_LO	0x56
+#define BU27010_MASK_DATA_FLICKER_HI	GENMASK(2, 0)
+#define BU27010_REG_FLICKER_COUNT	0x5a
+#define BU27010_REG_FIFO_LEVEL_LO	0x5b
+#define BU27010_MASK_FIFO_LEVEL_HI	BIT(0)
+#define BU27010_REG_FIFO_DATA_LO	0x5d
+#define BU27010_REG_FIFO_DATA_HI	0x5e
+#define BU27010_MASK_FIFO_DATA_HI	GENMASK(2, 0)
+#define BU27010_REG_MANUFACTURER_ID	0x92
+#define BU27010_REG_MAX BU27010_REG_MANUFACTURER_ID
+
 /**
  * enum bu27008_chan_type - BU27008 channel types
  * @BU27008_RED:	Red channel. Always via data0.
@@ -117,6 +188,17 @@ static const unsigned long bu27008_scan_masks[] = {
  */
 #define BU27008_SCALE_1X 16
 
+/*
+ * On BU27010 available scales with gain 1x - 4096x,
+ * timings 55, 100, 200, 400 mS. Time impacts to gain: 1x, 2x, 4x, 8x.
+ *
+ * => Max total gain is HWGAIN * gain by integration time (8 * 4096)
+ *
+ * Using NANO precision for scale we must use scale 64x corresponding gain 1x
+ * to avoid precision loss.
+ */
+#define BU27010_SCALE_1X 64
+
 /* See the data sheet for the "Gain Setting" table */
 #define BU27008_GSEL_1X		0x00
 #define BU27008_GSEL_4X		0x08
@@ -152,10 +234,44 @@ static const struct iio_gain_sel_pair bu27008_gains_ir[] = {
 	GAIN_SCALE_GAIN(1024, BU27008_GSEL_1024X),
 };
 
+#define BU27010_GSEL_1X		0x00	/* 000000 */
+#define BU27010_GSEL_4X		0x08	/* 001000 */
+#define BU27010_GSEL_16X	0x09	/* 001001 */
+#define BU27010_GSEL_64X	0x0e	/* 001110 */
+#define BU27010_GSEL_256X	0x1e	/* 011110 */
+#define BU27010_GSEL_1024X	0x2e	/* 101110 */
+#define BU27010_GSEL_4096X	0x3f	/* 111111 */
+
+static const struct iio_gain_sel_pair bu27010_gains[] = {
+	GAIN_SCALE_GAIN(1, BU27010_GSEL_1X),
+	GAIN_SCALE_GAIN(4, BU27010_GSEL_4X),
+	GAIN_SCALE_GAIN(16, BU27010_GSEL_16X),
+	GAIN_SCALE_GAIN(64, BU27010_GSEL_64X),
+	GAIN_SCALE_GAIN(256, BU27010_GSEL_256X),
+	GAIN_SCALE_GAIN(1024, BU27010_GSEL_1024X),
+	GAIN_SCALE_GAIN(4096, BU27010_GSEL_4096X),
+};
+
+static const struct iio_gain_sel_pair bu27010_gains_ir[] = {
+	GAIN_SCALE_GAIN(2, BU27010_GSEL_1X),
+	GAIN_SCALE_GAIN(4, BU27010_GSEL_4X),
+	GAIN_SCALE_GAIN(16, BU27010_GSEL_16X),
+	GAIN_SCALE_GAIN(64, BU27010_GSEL_64X),
+	GAIN_SCALE_GAIN(256, BU27010_GSEL_256X),
+	GAIN_SCALE_GAIN(1024, BU27010_GSEL_1024X),
+	GAIN_SCALE_GAIN(4096, BU27010_GSEL_4096X),
+};
+
 #define BU27008_MEAS_MODE_100MS		0x00
 #define BU27008_MEAS_MODE_55MS		0x01
 #define BU27008_MEAS_MODE_200MS		0x02
 #define BU27008_MEAS_MODE_400MS		0x04
+
+#define BU27010_MEAS_MODE_100MS		0x00
+#define BU27010_MEAS_MODE_55MS		0x03
+#define BU27010_MEAS_MODE_200MS		0x01
+#define BU27010_MEAS_MODE_400MS		0x02
+
 #define BU27008_MEAS_TIME_MAX_MS	400
 
 static const struct iio_itime_sel_mul bu27008_itimes[] = {
@@ -165,6 +281,13 @@ static const struct iio_itime_sel_mul bu27008_itimes[] = {
 	GAIN_SCALE_ITIME_US(55000, BU27008_MEAS_MODE_55MS, 1),
 };
 
+static const struct iio_itime_sel_mul bu27010_itimes[] = {
+	GAIN_SCALE_ITIME_US(400000, BU27010_MEAS_MODE_400MS, 8),
+	GAIN_SCALE_ITIME_US(200000, BU27010_MEAS_MODE_200MS, 4),
+	GAIN_SCALE_ITIME_US(100000, BU27010_MEAS_MODE_100MS, 2),
+	GAIN_SCALE_ITIME_US(55000, BU27010_MEAS_MODE_55MS, 1),
+};
+
 /*
  * All the RGBC channels share the same gain.
  * IR gain can be fine-tuned from the gain set for the RGBC by 2 bit, but this
@@ -221,8 +344,10 @@ struct bu27_chip_data {
 	const struct regmap_config *regmap_cfg;
 	const struct iio_gain_sel_pair *gains;
 	const struct iio_gain_sel_pair *gains_ir;
+	const struct iio_itime_sel_mul *itimes;
 	int num_gains;
 	int num_gains_ir;
+	int num_itimes;
 	int scale1x;
 
 	int drdy_en_reg;
@@ -266,11 +391,29 @@ static const struct regmap_range bu27008_volatile_ranges[] = {
 	},
 };
 
+static const struct regmap_range bu27010_volatile_ranges[] = {
+	{
+		.range_min = BU27010_REG_RESET,			/* RSTB */
+		.range_max = BU27008_REG_SYSTEM_CONTROL,	/* RESET */
+	}, {
+		.range_min = BU27010_REG_MODE_CONTROL5,		/* VALID bits */
+		.range_max = BU27010_REG_MODE_CONTROL5,
+	}, {
+		.range_min = BU27008_REG_DATA0_LO,
+		.range_max = BU27010_REG_FIFO_DATA_HI,
+	},
+};
+
 static const struct regmap_access_table bu27008_volatile_regs = {
 	.yes_ranges = &bu27008_volatile_ranges[0],
 	.n_yes_ranges = ARRAY_SIZE(bu27008_volatile_ranges),
 };
 
+static const struct regmap_access_table bu27010_volatile_regs = {
+	.yes_ranges = &bu27010_volatile_ranges[0],
+	.n_yes_ranges = ARRAY_SIZE(bu27010_volatile_ranges),
+};
+
 static const struct regmap_range bu27008_read_only_ranges[] = {
 	{
 		.range_min = BU27008_REG_DATA0_LO,
@@ -281,11 +424,26 @@ static const struct regmap_range bu27008_read_only_ranges[] = {
 	},
 };
 
+static const struct regmap_range bu27010_read_only_ranges[] = {
+	{
+		.range_min = BU27008_REG_DATA0_LO,
+		.range_max = BU27010_REG_FIFO_DATA_HI,
+	}, {
+		.range_min = BU27010_REG_MANUFACTURER_ID,
+		.range_max = BU27010_REG_MANUFACTURER_ID,
+	}
+};
+
 static const struct regmap_access_table bu27008_ro_regs = {
 	.no_ranges = &bu27008_read_only_ranges[0],
 	.n_no_ranges = ARRAY_SIZE(bu27008_read_only_ranges),
 };
 
+static const struct regmap_access_table bu27010_ro_regs = {
+	.no_ranges = &bu27010_read_only_ranges[0],
+	.n_no_ranges = ARRAY_SIZE(bu27010_read_only_ranges),
+};
+
 static const struct regmap_config bu27008_regmap = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -308,10 +466,49 @@ static const struct regmap_config bu27008_regmap = {
 	.disable_locking = true,
 };
 
+static const struct regmap_config bu27010_regmap = {
+	.reg_bits	= 8,
+	.val_bits	= 8,
+
+	.max_register	= BU27010_REG_MAX,
+	.cache_type	= REGCACHE_RBTREE,
+	.volatile_table = &bu27010_volatile_regs,
+	.wr_table	= &bu27010_ro_regs,
+	.disable_locking = true,
+};
+
 static int bu27008_chip_init(struct bu27008_data *data);
 static int bu27008_write_gain_sel(struct bu27008_data *data, int sel);
 static int bu27008_get_gain_sel(struct bu27008_data *data, int *sel);
 
+static int bu27010_chip_init(struct bu27008_data *data);
+static int bu27010_get_gain_sel(struct bu27008_data *data, int *sel);
+static int bu27010_write_gain_sel(struct bu27008_data *data, int sel);
+
+static const struct bu27_chip_data bu27010_chip = {
+	.name = "bu27010",
+	.chip_init = bu27010_chip_init,
+	.get_gain_sel = bu27010_get_gain_sel,
+	.write_gain_sel = bu27010_write_gain_sel,
+	.scale1x = BU27010_SCALE_1X,
+	.part_id = BU27010_ID,
+	.regmap_cfg = &bu27010_regmap,
+	.drdy_en_reg = BU27010_REG_MODE_CONTROL4,
+	.drdy_en_mask = BU27010_DRDY_EN,
+	.valid_reg = BU27010_REG_MODE_CONTROL5,
+	.meas_en_reg = BU27010_REG_MODE_CONTROL5,
+	.meas_en_mask = BU27010_MASK_MEAS_EN,
+	.chan_sel_reg = BU27008_REG_MODE_CONTROL1,
+	.chan_sel_mask = BU27010_MASK_CHAN_SEL,
+	.int_time_mask = BU27010_MASK_MEAS_MODE,
+	.gains = &bu27010_gains[0],
+	.num_gains = ARRAY_SIZE(bu27010_gains),
+	.gains_ir = &bu27010_gains_ir[0],
+	.num_gains_ir = ARRAY_SIZE(bu27010_gains_ir),
+	.itimes = &bu27010_itimes[0],
+	.num_itimes = ARRAY_SIZE(bu27010_itimes),
+};
+
 static const struct bu27_chip_data bu27008_chip = {
 	.name = "bu27008",
 	.chip_init = bu27008_chip_init,
@@ -332,6 +529,8 @@ static const struct bu27_chip_data bu27008_chip = {
 	.num_gains = ARRAY_SIZE(bu27008_gains),
 	.gains_ir = &bu27008_gains_ir[0],
 	.num_gains_ir = ARRAY_SIZE(bu27008_gains_ir),
+	.itimes = &bu27008_itimes[0],
+	.num_itimes = ARRAY_SIZE(bu27008_itimes),
 };
 
 #define BU27008_MAX_VALID_RESULT_WAIT_US	50000
@@ -358,6 +557,31 @@ static int bu27008_chan_read_data(struct bu27008_data *data, int reg, int *val)
 	return ret;
 }
 
+static int bu27010_get_gain_sel(struct bu27008_data *data, int *sel)
+{
+	int ret;
+
+	/*
+	 * We always "lock" the gain selectors for all channels to prevent
+	 * unsupported configs. It does not matter which channel is used
+	 * we can just return selector from any of them.
+	 */
+	ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL2, sel);
+	if (!ret) {
+		int tmp;
+
+		*sel = FIELD_GET(BU27010_MASK_DATA0_GAIN, *sel);
+
+		ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL1, &tmp);
+		if (ret)
+			return ret;
+
+		*sel |= FIELD_GET(BU27010_MASK_RGBC_GAIN, tmp) << fls(BU27010_MASK_DATA0_GAIN);
+	}
+
+	return ret;
+}
+
 static int bu27008_get_gain_sel(struct bu27008_data *data, int *sel)
 {
 	int ret;
@@ -388,7 +612,7 @@ static int bu27008_get_gain(struct bu27008_data *data, struct iio_gts *gts, int
 {
 	int ret, sel;
 
-	ret = bu27008_get_gain_sel(data, &sel);
+	ret = data->cd->get_gain_sel(data, &sel);
 	if (ret)
 		return ret;
 
@@ -403,6 +627,35 @@ static int bu27008_get_gain(struct bu27008_data *data, struct iio_gts *gts, int
 	return 0;
 }
 
+static int bu27010_write_gain_sel(struct bu27008_data *data, int sel)
+{
+	unsigned int regval;
+	int ret;
+
+	/*
+	 * Gain 'selector' is composed of two registers. Selector is 6bit value,
+	 * 4 high bits being the RGBC gain fieild in MODE_CONTROL1 register and
+	 * two low bits being the channel specific gain in MODE_CONTROL2.
+	 *
+	 * Let's take the 4 high bits of whole 6 bit selector, and prepare
+	 * the MODE_CONTROL1 value (RGBC gain part).
+	 */
+	regval = FIELD_PREP(BU27010_MASK_RGBC_GAIN, (sel >> 2));
+
+	ret = regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL1,
+				  BU27010_MASK_RGBC_GAIN, regval);
+	/*
+	 * Two low two bits must be set for all 4 channels in the
+	 * MODE_CONTROL2 register. Copy these two bits for all channels.
+	 */
+	regval = sel & GENMASK(1, 0);
+	regval = regval | regval >> 2 | regval >> 4 | regval >> 6;
+
+	ret = regmap_write(data->regmap, BU27008_REG_MODE_CONTROL2, regval);
+
+	return ret;
+}
+
 static int bu27008_write_gain_sel(struct bu27008_data *data, int sel)
 {
 	int regval;
@@ -452,7 +705,7 @@ static int bu27008_set_gain(struct bu27008_data *data, int gain)
 	if (ret < 0)
 		return ret;
 
-	return bu27008_write_gain_sel(data, ret);
+	return data->cd->write_gain_sel(data, ret);
 }
 
 static int bu27008_get_int_time_sel(struct bu27008_data *data, int *sel)
@@ -460,13 +713,15 @@ 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);
+	if (ret)
+		return ret;
 
 	val &= data->cd->int_time_mask;
 	val >>= ffs(data->cd->int_time_mask) - 1;
 
 	*sel = val;
 
-	return ret;
+	return 0;
 }
 
 static int bu27008_set_int_time_sel(struct bu27008_data *data, int sel)
@@ -759,7 +1014,7 @@ static int bu27008_set_scale(struct bu27008_data *data,
 			goto unlock_out;
 
 	}
-	ret = bu27008_write_gain_sel(data, gain_sel);
+	ret = data->cd->write_gain_sel(data, gain_sel);
 
 unlock_out:
 	mutex_unlock(&data->mutex);
@@ -867,6 +1122,55 @@ static const struct iio_info bu27008_info = {
 	.validate_trigger = iio_validate_own_trigger,
 };
 
+static int bu27010_chip_init(struct bu27008_data *data)
+{
+	int ret;
+
+	/* Reset the IC to initial power-off state */
+	ret = regmap_write_bits(data->regmap, BU27008_REG_SYSTEM_CONTROL,
+				BU27010_MASK_SW_RESET, BU27010_MASK_SW_RESET);
+	if (ret)
+		return dev_err_probe(data->dev, ret, "Sensor reset failed\n");
+
+	msleep(1);
+
+	/* Power ON*/
+	ret = regmap_write_bits(data->regmap, BU27010_REG_POWER,
+				BU27010_MASK_POWER, BU27010_MASK_POWER);
+	if (ret)
+		return dev_err_probe(data->dev, ret, "Sensor power-on failed\n");
+
+	msleep(1);
+
+	/* Release blocks from reset */
+	ret = regmap_write_bits(data->regmap, BU27010_REG_RESET,
+				BU27010_MASK_RESET, BU27010_RESET_RELEASE);
+	if (ret)
+		return dev_err_probe(data->dev, ret, "Sensor powering failed\n");
+
+	msleep(1);
+
+	/*
+	 * The IRQ enabling on BU27010 is done in a peculiar way. The IRQ
+	 * enabling is not a bit mask where individual IRQs could be enabled but
+	 * a field which values are:
+	 * 00 => IRQs disabled
+	 * 01 => Data-ready (RGBC/IR)
+	 * 10 => Data-ready (flicker)
+	 * 11 => Flicker FIFO
+	 *
+	 * So, only one IRQ can be enabled at a time and enabling for example
+	 * flicker FIFO would automagically disable data-ready IRQ.
+	 *
+	 * Currently the driver does not support the flicker. Hence, we can
+	 * just treat the RGBC data-ready as single bit which can be enabled /
+	 * disabled. This works for as long as the second bit in the field
+	 * stays zero. Here we ensure it gets zeroed.
+	 */
+	return regmap_clear_bits(data->regmap, BU27010_REG_MODE_CONTROL4,
+				 BU27010_IRQ_DIS_ALL);
+}
+
 static int bu27008_chip_init(struct bu27008_data *data)
 {
 	int ret;
@@ -1068,14 +1372,14 @@ static int bu27008_probe(struct i2c_client *i2c)
 		dev_warn(dev, "unknown device 0x%x\n", part_id);
 
 	ret = devm_iio_init_iio_gts(dev, data->cd->scale1x, 0, data->cd->gains,
-				    data->cd->num_gains, bu27008_itimes,
-				    ARRAY_SIZE(bu27008_itimes), &data->gts);
+				    data->cd->num_gains, data->cd->itimes,
+				    data->cd->num_itimes, &data->gts);
 	if (ret)
 		return ret;
 
 	ret = devm_iio_init_iio_gts(dev, data->cd->scale1x, 0, data->cd->gains_ir,
-				    data->cd->num_gains_ir, bu27008_itimes,
-				    ARRAY_SIZE(bu27008_itimes), &data->gts_ir);
+				    data->cd->num_gains_ir, data->cd->itimes,
+				    data->cd->num_itimes, &data->gts_ir);
 	if (ret)
 		return ret;
 
@@ -1113,6 +1417,7 @@ static int bu27008_probe(struct i2c_client *i2c)
 
 static const struct of_device_id bu27008_of_match[] = {
 	{ .compatible = "rohm,bu27008", .data = &bu27008_chip },
+	{ .compatible = "rohm,bu27010", .data = &bu27010_chip },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, bu27008_of_match);
@@ -1127,7 +1432,7 @@ static struct i2c_driver bu27008_i2c_driver = {
 };
 module_i2c_driver(bu27008_i2c_driver);
 
-MODULE_DESCRIPTION("ROHM BU27008 colour sensor driver");
+MODULE_DESCRIPTION("ROHM BU27008 and BU27010 colour sensor driver");
 MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
 MODULE_LICENSE("GPL");
 MODULE_IMPORT_NS(IIO_GTS_HELPER);
-- 
2.40.1


-- 
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] iio: light: bd27008: Support BD27010 RGB

[Jonathan Cameron]

On Tue, 13 Jun 2023 13:20:26 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> The ROHM BU27010 is an RGBC sensor with a flickering detection FIFO. The
> RGBC+IR sensor functionality is largely similar to what the BU27008 has.
> There are some notable things though:
>   - gain setting is once again new and exotic. Now, there is 6bit gain
>     setting where 4 of the bits are common to all channels and 2 bits
>     can be configured separately for each channel. The BU27010 has
>     similar "1X on other channels vs 2X on IR when selector is 0x0"
>     gain design as BU27008 had. So, we use same gain setting policy for
>     BU27010 as we did for BU27008 - driver sets same gain selector for all
>     channels but shows the gains separately for all channels so users
>     can (at least in theory) detect this 1X vs 2X madness...
>   - BU27010 has suffled all the control register bitfields to new
>     addresses and bit positions while still keeping the register naming
>     same.
>   - Some more power/reset control is added.
>   - FIFO for "flickering detection" is added.
> 
> The control register suffling made this slightly nasty. Still, it is
> easier for maintenance perspective to add the BU27010 support in BU27008
> driver because - even though the bit positions/addresses were changed -
> most of the driver structure can be re-used. Writing own driver for
> BU27010 would mean plenty of duplicate code albeit a tad more clarity.
> 
> The flickering FIFO is not supported by the driver.
> 
> Add BU27010 RGBC+IR support to rohm-bu27008 driver.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 

Resulting code looks more or less fine, but there is stuff in here that
belongs in previous patch - so send a v2 with the refactors all done
there and just support for the new part in here.

Thanks,

Jonathan

...


> +
>  static int bu27008_chip_init(struct bu27008_data *data);
>  static int bu27008_write_gain_sel(struct bu27008_data *data, int sel);
>  static int bu27008_get_gain_sel(struct bu27008_data *data, int *sel);
>  
> +static int bu27010_chip_init(struct bu27008_data *data);
> +static int bu27010_get_gain_sel(struct bu27008_data *data, int *sel);
> +static int bu27010_write_gain_sel(struct bu27008_data *data, int sel);
> +
> +static const struct bu27_chip_data bu27010_chip = {
> +	.name = "bu27010",
> +	.chip_init = bu27010_chip_init,
> +	.get_gain_sel = bu27010_get_gain_sel,
> +	.write_gain_sel = bu27010_write_gain_sel,
> +	.scale1x = BU27010_SCALE_1X,
> +	.part_id = BU27010_ID,
> +	.regmap_cfg = &bu27010_regmap,
> +	.drdy_en_reg = BU27010_REG_MODE_CONTROL4,
> +	.drdy_en_mask = BU27010_DRDY_EN,
> +	.valid_reg = BU27010_REG_MODE_CONTROL5,
> +	.meas_en_reg = BU27010_REG_MODE_CONTROL5,
> +	.meas_en_mask = BU27010_MASK_MEAS_EN,
> +	.chan_sel_reg = BU27008_REG_MODE_CONTROL1,
> +	.chan_sel_mask = BU27010_MASK_CHAN_SEL,
> +	.int_time_mask = BU27010_MASK_MEAS_MODE,
> +	.gains = &bu27010_gains[0],
> +	.num_gains = ARRAY_SIZE(bu27010_gains),
> +	.gains_ir = &bu27010_gains_ir[0],
> +	.num_gains_ir = ARRAY_SIZE(bu27010_gains_ir),
> +	.itimes = &bu27010_itimes[0],
> +	.num_itimes = ARRAY_SIZE(bu27010_itimes),
> +};
> +
>  static const struct bu27_chip_data bu27008_chip = {
>  	.name = "bu27008",
>  	.chip_init = bu27008_chip_init,
> @@ -332,6 +529,8 @@ static const struct bu27_chip_data bu27008_chip = {
>  	.num_gains = ARRAY_SIZE(bu27008_gains),
>  	.gains_ir = &bu27008_gains_ir[0],
>  	.num_gains_ir = ARRAY_SIZE(bu27008_gains_ir),
> +	.itimes = &bu27008_itimes[0],
> +	.num_itimes = ARRAY_SIZE(bu27008_itimes),
>  };
>  
>  #define BU27008_MAX_VALID_RESULT_WAIT_US	50000
> @@ -358,6 +557,31 @@ static int bu27008_chan_read_data(struct bu27008_data *data, int reg, int *val)
>  	return ret;
>  }
>  
> +static int bu27010_get_gain_sel(struct bu27008_data *data, int *sel)
> +{
> +	int ret;
> +
> +	/*
> +	 * We always "lock" the gain selectors for all channels to prevent
> +	 * unsupported configs. It does not matter which channel is used
> +	 * we can just return selector from any of them.
> +	 */
> +	ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL2, sel);
> +	if (!ret) {
	if (ret)
		return ret;

	....

> +		int tmp;
> +
> +		*sel = FIELD_GET(BU27010_MASK_DATA0_GAIN, *sel);
> +
> +		ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL1, &tmp);
> +		if (ret)
> +			return ret;
> +
> +		*sel |= FIELD_GET(BU27010_MASK_RGBC_GAIN, tmp) << fls(BU27010_MASK_DATA0_GAIN);
> +	}
> +
> +	return ret;
> +}
> +
>  static int bu27008_get_gain_sel(struct bu27008_data *data, int *sel)
>  {
>  	int ret;
> @@ -388,7 +612,7 @@ static int bu27008_get_gain(struct bu27008_data *data, struct iio_gts *gts, int
>  {
>  	int ret, sel;
>  
> -	ret = bu27008_get_gain_sel(data, &sel);
> +	ret = data->cd->get_gain_sel(data, &sel);
Again, belongs in previous patch.
>  	if (ret)
>  		return ret;
>  
> @@ -403,6 +627,35 @@ static int bu27008_get_gain(struct bu27008_data *data, struct iio_gts *gts, int
>  	return 0;
>  }
>  
> +static int bu27010_write_gain_sel(struct bu27008_data *data, int sel)
> +{
> +	unsigned int regval;
> +	int ret;
> +
> +	/*
> +	 * Gain 'selector' is composed of two registers. Selector is 6bit value,
> +	 * 4 high bits being the RGBC gain fieild in MODE_CONTROL1 register and
> +	 * two low bits being the channel specific gain in MODE_CONTROL2.
> +	 *
> +	 * Let's take the 4 high bits of whole 6 bit selector, and prepare
> +	 * the MODE_CONTROL1 value (RGBC gain part).
> +	 */
> +	regval = FIELD_PREP(BU27010_MASK_RGBC_GAIN, (sel >> 2));
> +
> +	ret = regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL1,
> +				  BU27010_MASK_RGBC_GAIN, regval);

ret not checked.

> +	/*
> +	 * Two low two bits must be set for all 4 channels in the
> +	 * MODE_CONTROL2 register. Copy these two bits for all channels.
> +	 */
> +	regval = sel & GENMASK(1, 0);

FIELD_PREP and all fields explicitly set.

> +	regval = regval | regval >> 2 | regval >> 4 | regval >> 6;
> +
> +	ret = regmap_write(data->regmap, BU27008_REG_MODE_CONTROL2, regval);
return regmap_write...

> +
> +	return ret;
> +}
> +
>  static int bu27008_write_gain_sel(struct bu27008_data *data, int sel)
>  {
>  	int regval;
> @@ -452,7 +705,7 @@ static int bu27008_set_gain(struct bu27008_data *data, int gain)
>  	if (ret < 0)
>  		return ret;
>  
> -	return bu27008_write_gain_sel(data, ret);
> +	return data->cd->write_gain_sel(data, ret);
Another one that wants to be in the refactor patch  - not here.

>  }
>  
>  static int bu27008_get_int_time_sel(struct bu27008_data *data, int *sel)
> @@ -460,13 +713,15 @@ 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);
> +	if (ret)
> +		return ret;
>  
>  	val &= data->cd->int_time_mask;
>  	val >>= ffs(data->cd->int_time_mask) - 1;
>  
>  	*sel = val;
>  
> -	return ret;
> +	return 0;

This looks unrelated / fix for previous patch?

>  }
>  
>  static int bu27008_set_int_time_sel(struct bu27008_data *data, int sel)
> @@ -759,7 +1014,7 @@ static int bu27008_set_scale(struct bu27008_data *data,
>  			goto unlock_out;
>  
>  	}
> -	ret = bu27008_write_gain_sel(data, gain_sel);
> +	ret = data->cd->write_gain_sel(data, gain_sel);

As below - make all these sort of refactoring changes in previous patch.

>  
>  unlock_out:
>  	mutex_unlock(&data->mutex);
> @@ -867,6 +1122,55 @@ static const struct iio_info bu27008_info = {
>  	.validate_trigger = iio_validate_own_trigger,
>  };
>  
> +static int bu27010_chip_init(struct bu27008_data *data)
> +{
> +	int ret;
> +
> +	/* Reset the IC to initial power-off state */

I'd argue the code is clear enough the comment adds little.

> +	ret = regmap_write_bits(data->regmap, BU27008_REG_SYSTEM_CONTROL,
> +				BU27010_MASK_SW_RESET, BU27010_MASK_SW_RESET);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret, "Sensor reset failed\n");
> +
> +	msleep(1);
> +
> +	/* Power ON*/
> +	ret = regmap_write_bits(data->regmap, BU27010_REG_POWER,
> +				BU27010_MASK_POWER, BU27010_MASK_POWER);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret, "Sensor power-on failed\n");
> +
> +	msleep(1);
> +
> +	/* Release blocks from reset */
> +	ret = regmap_write_bits(data->regmap, BU27010_REG_RESET,
> +				BU27010_MASK_RESET, BU27010_RESET_RELEASE);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret, "Sensor powering failed\n");
> +
> +	msleep(1);
> +
> +	/*
> +	 * The IRQ enabling on BU27010 is done in a peculiar way. The IRQ
> +	 * enabling is not a bit mask where individual IRQs could be enabled but
> +	 * a field which values are:
> +	 * 00 => IRQs disabled
> +	 * 01 => Data-ready (RGBC/IR)
> +	 * 10 => Data-ready (flicker)
> +	 * 11 => Flicker FIFO
> +	 *

That's nasty :)

> +	 * So, only one IRQ can be enabled at a time and enabling for example
> +	 * flicker FIFO would automagically disable data-ready IRQ.
> +	 *
> +	 * Currently the driver does not support the flicker. Hence, we can
> +	 * just treat the RGBC data-ready as single bit which can be enabled /
> +	 * disabled. This works for as long as the second bit in the field
> +	 * stays zero. Here we ensure it gets zeroed.
> +	 */
> +	return regmap_clear_bits(data->regmap, BU27010_REG_MODE_CONTROL4,
> +				 BU27010_IRQ_DIS_ALL);
> +}
> +
>  static int bu27008_chip_init(struct bu27008_data *data)
>  {
>  	int ret;
> @@ -1068,14 +1372,14 @@ static int bu27008_probe(struct i2c_client *i2c)
>  		dev_warn(dev, "unknown device 0x%x\n", part_id);
>  
>  	ret = devm_iio_init_iio_gts(dev, data->cd->scale1x, 0, data->cd->gains,
> -				    data->cd->num_gains, bu27008_itimes,
> -				    ARRAY_SIZE(bu27008_itimes), &data->gts);
> +				    data->cd->num_gains, data->cd->itimes,
> +				    data->cd->num_itimes, &data->gts);
>  	if (ret)
>  		return ret;
>  
>  	ret = devm_iio_init_iio_gts(dev, data->cd->scale1x, 0, data->cd->gains_ir,
> -				    data->cd->num_gains_ir, bu27008_itimes,
> -				    ARRAY_SIZE(bu27008_itimes), &data->gts_ir);
> +				    data->cd->num_gains_ir, data->cd->itimes,
> +				    data->cd->num_itimes, &data->gts_ir);

I'd expect the changes to make things part specific to all be in previous patch
- not mixed across that one and this one.


>  	if (ret)
>  		return ret;

Re: [PATCH 3/3] iio: light: bd27008: Support BD27010 RGB

[Matti Vaittinen]

On 6/17/23 22:57, Jonathan Cameron wrote:
> On Tue, 13 Jun 2023 13:20:26 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> The ROHM BU27010 is an RGBC sensor with a flickering detection FIFO. The
>> RGBC+IR sensor functionality is largely similar to what the BU27008 has.
>> There are some notable things though:
>>    - gain setting is once again new and exotic. Now, there is 6bit gain
>>      setting where 4 of the bits are common to all channels and 2 bits
>>      can be configured separately for each channel. The BU27010 has
>>      similar "1X on other channels vs 2X on IR when selector is 0x0"
>>      gain design as BU27008 had. So, we use same gain setting policy for
>>      BU27010 as we did for BU27008 - driver sets same gain selector for all
>>      channels but shows the gains separately for all channels so users
>>      can (at least in theory) detect this 1X vs 2X madness...
>>    - BU27010 has suffled all the control register bitfields to new
>>      addresses and bit positions while still keeping the register naming
>>      same.
>>    - Some more power/reset control is added.
>>    - FIFO for "flickering detection" is added.
>>
>> The control register suffling made this slightly nasty. Still, it is
>> easier for maintenance perspective to add the BU27010 support in BU27008
>> driver because - even though the bit positions/addresses were changed -
>> most of the driver structure can be re-used. Writing own driver for
>> BU27010 would mean plenty of duplicate code albeit a tad more clarity.
>>
>> The flickering FIFO is not supported by the driver.
>>
>> Add BU27010 RGBC+IR support to rohm-bu27008 driver.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>
> 
> Resulting code looks more or less fine, but there is stuff in here that
> belongs in previous patch - so send a v2 with the refactors all done
> there and just support for the new part in here.

Thanks for the review! I appreciate it. And sorry for sending a messy 
version. I'll try re-organizing the stuff between these two patches when 
re-spinning. A bit busy now as I said when replying to review of PATCH 
2/3 - but I'll see when I get a moment to rework this :) Thanks!

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~