[PATCH v1 0/3] Support ROHM BU27008 RGB sensor

[PATCH v1 0/3] Support ROHM BU27008 RGB sensor

[Matti Vaittinen]

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

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


-- 
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 v1 1/3] dt-bindings: iio: light: ROHM BU27008

[Matti Vaittinen]

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

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 <mazziesaccount@gmail.com>
---
 .../bindings/iio/light/rohm-bu27008.yaml      | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/rohm-bu27008.yaml

diff --git a/Documentation/devicetree/bindings/iio/light/rohm-bu27008.yaml b/Documentation/devicetree/bindings/iio/light/rohm-bu27008.yaml
new file mode 100644
index 000000000000..d942c2817680
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/rohm-bu27008.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/bu27008.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ROHM BU27008 color sensor
+
+maintainers:
+  - Matti Vaittinen <mazziesaccount@gmail.com>
+
+description: |
+  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.
+
+properties:
+  compatible:
+    const: rohm,bu27008
+
+  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,bu27008";
+        reg = <0x38>;
+      };
+    };
+
+...
-- 
2.40.0


-- 
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 v1 0/3] Support ROHM BU27008 RGB sensor

[Jonathan Cameron]

On Fri, 21 Apr 2023 12:37:30 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> 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

framework

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

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.

Jonathan

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

Re: [PATCH v1 0/3] Support ROHM BU27008 RGB sensor

[Vaittinen, Matti]

On 4/23/23 15:04, Jonathan Cameron wrote:
> On Fri, 21 Apr 2023 12:37:30 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> 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
> 
> framework

Thanks!

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

I am not sure either. This is really a dance between making driver which 
is still understandable and maintainable, ABI which allows us to improve 
things if needed and providing users the features they need.

So. let's begin with this - and as you say, if it appears to be 
suboptimal, then we can re-work it :)

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 v1 1/3] dt-bindings: iio: light: ROHM BU27008

[Krzysztof Kozlowski]

On 21/04/2023 11:38, Matti Vaittinen wrote:
> The ROHM BU27008 is a sensor with 5 photodiodes (red, green, blue, clear
> and IR) with four configurable channels. Red and green being always
> available and two out of the rest three (blue, clear, IR) can be
> selected to be simultaneously measured. Typical application is adjusting
> LCD backlight of TVs, mobile phones and tablet PCs.
> 
> Add BU27008 dt-bindings.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> ---
>  .../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

> 
> diff --git a/Documentation/devicetree/bindings/iio/light/rohm-bu27008.yaml b/Documentation/devicetree/bindings/iio/light/rohm-bu27008.yaml
> new file mode 100644
> index 000000000000..d942c2817680
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/rohm-bu27008.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/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).


> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ROHM BU27008 color sensor
> +
> +maintainers:
> +  - Matti Vaittinen <mazziesaccount@gmail.com>
> +
> +description: |

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

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


Best regards,
Krzysztof

Re: [PATCH v1 1/3] dt-bindings: iio: light: ROHM BU27008

[Matti Vaittinen]

On 4/24/23 13:12, Krzysztof Kozlowski wrote:
> On 21/04/2023 11:38, Matti Vaittinen wrote:
>> The ROHM BU27008 is a sensor with 5 photodiodes (red, green, blue, clear
>> and IR) with four configurable channels. Red and green being always
>> available and two out of the rest three (blue, clear, IR) can be
>> selected to be simultaneously measured. Typical application is adjusting
>> LCD backlight of TVs, mobile phones and tablet PCs.
>>
>> Add BU27008 dt-bindings.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>> ---
>>   .../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

Thanks Krzysztof. I should've remembered this as you told me the same 
thing during the bu27034 review. Feel free to kick me atthe ELC-E if I 
do the same mistake with bu27010 as well ^^;
> 
>>
>> diff --git a/Documentation/devicetree/bindings/iio/light/rohm-bu27008.yaml b/Documentation/devicetree/bindings/iio/light/rohm-bu27008.yaml
>> new file mode 100644
>> index 000000000000..d942c2817680
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/light/rohm-bu27008.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/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).
> 

I actually did. But I did first run the dt_binding_check without 
filename - causing it to check all the in-tree bindings - which took a 
while. So, I went to have a lunch. When I came back I re-ran the check 
with the filename (DT_SCHEMA_FILES=...) - which gave me no errors.

I _assume_ this is because running the check for all bindings had 
already done <add step here> generating the warning, while re-running 
the check with the filename omitted the <add step here> and no longer 
displayed the warning.

In any case, I missed warning from full-check, and checker missed the 
warning when re-ran.

> 
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: ROHM BU27008 color sensor
>> +
>> +maintainers:
>> +  - Matti Vaittinen <mazziesaccount@gmail.com>
>> +
>> +description: |
> 
> Do not need '|' unless you need to preserve formatting.

Ok, thanks.

I'll fix these for v2 :)

Yours,
	-- Matti

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

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