[PATCH 0/2] Add support for Avago/Broadcom APDS9160

[PATCH 0/2] Add support for Avago/Broadcom APDS9160

[Mikael Gonella-Bolduc via B4 Relay]

APDS9160 is an ALS and proximity sensor.
https://www.broadcom.com/products/optical-sensors/integrated-ambient-light-and-proximity-sensors/apds-9160-003

Signed-off-by: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
---
Mikael Gonella-Bolduc (2):
      dt-bindings: iio: light: Add APDS9160 binding
      iio: light: Add APDS9160 ALS & Proximity sensor driver

 .../bindings/iio/light/avago,apds9160.yaml         |   50 +
 MAINTAINERS                                        |    7 +
 drivers/iio/light/Kconfig                          |   13 +
 drivers/iio/light/Makefile                         |    1 +
 drivers/iio/light/apds9160.c                       | 1420 ++++++++++++++++++++
 5 files changed, 1491 insertions(+)
---
base-commit: adc218676eef25575469234709c2d87185ca223a
change-id: 20241119-apds9160-driver-86cf5e86de35

Best regards,
-- 
Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>

[PATCH 1/2] dt-bindings: iio: light: Add APDS9160 binding

[Mikael Gonella-Bolduc via B4 Relay]

From: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>

Add device tree bindings for APDS9160 driver

Signed-off-by: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
---
 .../bindings/iio/light/avago,apds9160.yaml         | 50 ++++++++++++++++++++++
 1 file changed, 50 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/light/avago,apds9160.yaml b/Documentation/devicetree/bindings/iio/light/avago,apds9160.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..12e196b297fe523e4d324156041ef9c6900676eb
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/avago,apds9160.yaml
@@ -0,0 +1,50 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/avago,apds9160.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Broadcom Combined Proximity & Ambient light sensor
+
+maintainers:
+  - Mikael Gonella-Bolduc <m.gonella.bolduc@gmail.com>
+
+description: |
+  Datasheet: https://docs.broadcom.com/docs/APDS-9160-003-DS
+
+properties:
+  compatible:
+    enum:
+      - avago,apds9160
+      - broadmobi,apds9160
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  vdd-supply: true
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        apds9160@53 {
+          compatible = "broadmobi,apds9160";
+          reg = <0x53>;
+          interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
+          interrupt-parent = <&pinctrl>;
+        };
+    };
+...

-- 
2.34.1

Re: [PATCH 1/2] dt-bindings: iio: light: Add APDS9160 binding

[Conor Dooley]

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

On Tue, Nov 19, 2024 at 03:36:56PM -0500, Mikael Gonella-Bolduc via B4 Relay wrote:
> From: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
> 
> Add device tree bindings for APDS9160 driver
> 
> Signed-off-by: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
> ---
>  .../bindings/iio/light/avago,apds9160.yaml         | 50 ++++++++++++++++++++++
>  1 file changed, 50 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/avago,apds9160.yaml b/Documentation/devicetree/bindings/iio/light/avago,apds9160.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..12e196b297fe523e4d324156041ef9c6900676eb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/avago,apds9160.yaml
> @@ -0,0 +1,50 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/light/avago,apds9160.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Broadcom Combined Proximity & Ambient light sensor
> +
> +maintainers:
> +  - Mikael Gonella-Bolduc <m.gonella.bolduc@gmail.com>
> +
> +description: |
> +  Datasheet: https://docs.broadcom.com/docs/APDS-9160-003-DS
> +
> +properties:
> +  compatible:
> +    enum:
> +      - avago,apds9160
> +      - broadmobi,apds9160

What is the difference between these two devices? There's no match data,
makes it seem like there should be a fallback going on here.

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  vdd-supply: true
> +
> +additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        apds9160@53 {
> +          compatible = "broadmobi,apds9160";
> +          reg = <0x53>;
> +          interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
> +          interrupt-parent = <&pinctrl>;
> +        };
> +    };
> +...
> 
> -- 
> 2.34.1
> 
> 

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

Re: [PATCH 1/2] dt-bindings: iio: light: Add APDS9160 binding

[Krzysztof Kozlowski]

On 20/11/2024 18:18, Conor Dooley wrote:
> On Tue, Nov 19, 2024 at 03:36:56PM -0500, Mikael Gonella-Bolduc via B4 Relay wrote:
>> From: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
>>
>> Add device tree bindings for APDS9160 driver
>>
>> Signed-off-by: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
>> ---
>>  .../bindings/iio/light/avago,apds9160.yaml         | 50 ++++++++++++++++++++++
>>  1 file changed, 50 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/light/avago,apds9160.yaml b/Documentation/devicetree/bindings/iio/light/avago,apds9160.yaml
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..12e196b297fe523e4d324156041ef9c6900676eb
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/light/avago,apds9160.yaml
>> @@ -0,0 +1,50 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iio/light/avago,apds9160.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Broadcom Combined Proximity & Ambient light sensor
>> +
>> +maintainers:
>> +  - Mikael Gonella-Bolduc <m.gonella.bolduc@gmail.com>
>> +
>> +description: |
>> +  Datasheet: https://docs.broadcom.com/docs/APDS-9160-003-DS
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - avago,apds9160
>> +      - broadmobi,apds9160
> 
> What is the difference between these two devices? There's no match data,
> makes it seem like there should be a fallback going on here.
Same device names suggest this is some legacy. We don't take new
bindings for legacy stuff.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: iio: light: Add APDS9160 binding

[Mikael Gonella-Bolduc]

On Wed, Nov 20, 2024 at 06:23:11PM +0100, Krzysztof Kozlowski wrote:
> On 20/11/2024 18:18, Conor Dooley wrote:
> > On Tue, Nov 19, 2024 at 03:36:56PM -0500, Mikael Gonella-Bolduc via B4 Relay wrote:
> >> From: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
> >>
> >> Add device tree bindings for APDS9160 driver
> >>
> >> Signed-off-by: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
> >> ---
> >>  .../bindings/iio/light/avago,apds9160.yaml         | 50 ++++++++++++++++++++++
> >>  1 file changed, 50 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/iio/light/avago,apds9160.yaml b/Documentation/devicetree/bindings/iio/light/avago,apds9160.yaml
> >> new file mode 100644
> >> index 0000000000000000000000000000000000000000..12e196b297fe523e4d324156041ef9c6900676eb
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/iio/light/avago,apds9160.yaml
> >> @@ -0,0 +1,50 @@
> >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/iio/light/avago,apds9160.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Broadcom Combined Proximity & Ambient light sensor
> >> +
> >> +maintainers:
> >> +  - Mikael Gonella-Bolduc <m.gonella.bolduc@gmail.com>
> >> +
> >> +description: |
> >> +  Datasheet: https://docs.broadcom.com/docs/APDS-9160-003-DS
> >> +
> >> +properties:
> >> +  compatible:
> >> +    enum:
> >> +      - avago,apds9160
> >> +      - broadmobi,apds9160
> > 
> > What is the difference between these two devices? There's no match data,
> > makes it seem like there should be a fallback going on here.
> Same device names suggest this is some legacy. We don't take new
> bindings for legacy stuff.
> 
> Best regards,
> Krzysztof

Hi,
Thank you for the feedback.

There's no difference between these two devices, it's the same chip using two different names.
There's two names because the chip was first released before the Avago & Broadcom acquisition.

The datasheet available has the avago name in it and it's referenced using both names.
I did not know which name to include so I wrote both.

It's old but still being produced today and active for new designs.

Is it too old for the driver to be mainlined?
If not, which name should I use?

Best regards,
Mikael

Re: [PATCH 1/2] dt-bindings: iio: light: Add APDS9160 binding

[Krzysztof Kozlowski]

On 20/11/2024 21:26, Mikael Gonella-Bolduc wrote:
>>>> +$id: http://devicetree.org/schemas/iio/light/avago,apds9160.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Broadcom Combined Proximity & Ambient light sensor
>>>> +
>>>> +maintainers:
>>>> +  - Mikael Gonella-Bolduc <m.gonella.bolduc@gmail.com>
>>>> +
>>>> +description: |
>>>> +  Datasheet: https://docs.broadcom.com/docs/APDS-9160-003-DS
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - avago,apds9160
>>>> +      - broadmobi,apds9160
>>>
>>> What is the difference between these two devices? There's no match data,
>>> makes it seem like there should be a fallback going on here.
>> Same device names suggest this is some legacy. We don't take new
>> bindings for legacy stuff.
>>
>> Best regards,
>> Krzysztof
> 
> Hi,
> Thank you for the feedback.
> 
> There's no difference between these two devices, it's the same chip using two different names.
> There's two names because the chip was first released before the Avago & Broadcom acquisition.
> 
> The datasheet available has the avago name in it and it's referenced using both names.
> I did not know which name to include so I wrote both.

Choose only one. Preferably newer one. Just notice that broadcom and
broadmobi are a bit different entities, according to vendor prefixes.

> 
> It's old but still being produced today and active for new designs.
> 
> Is it too old for the driver to be mainlined?

No, it is fine.

> If not, which name should I use?


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: iio: light: Add APDS9160 binding

[Krzysztof Kozlowski]

On 19/11/2024 21:36, Mikael Gonella-Bolduc via B4 Relay wrote:
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  vdd-supply: true
> +
> +additionalProperties: false

This goes after required:

> +
> +required:
> +  - compatible
> +  - reg

Supply not required?

> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        apds9160@53 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +          compatible = "broadmobi,apds9160";

Use 4 spaces for example indentation. Or at least something consistent.


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: iio: light: Add APDS9160 binding

[Jonathan Cameron]

On Wed, 20 Nov 2024 18:22:43 +0100
Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On 19/11/2024 21:36, Mikael Gonella-Bolduc via B4 Relay wrote:
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  vdd-supply: true
> > +
> > +additionalProperties: false  
> 
> This goes after required:
> 
> > +
> > +required:
> > +  - compatible
> > +  - reg  
> 
> Supply not required?

Just for background on this.  Linux will happily provide
you a stub regulator if the supply is not in dt, but we decided a while ago
that it was more accurate to have the supplies as required properties as that
would allow us to distinguish between those that are needed for operation
and those that are actually optional like reference voltages where there
is the option of an on chip reference if the external one isn't connected.

So almost certainly just make it required here.

Jonathan

> 
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        apds9160@53 {  
> 
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 
> 
> > +          compatible = "broadmobi,apds9160";  
> 
> Use 4 spaces for example indentation. Or at least something consistent.
> 
> 
> Best regards,
> Krzysztof
> 

[PATCH 2/2] iio: light: Add APDS9160 ALS & Proximity sensor driver

[Mikael Gonella-Bolduc via B4 Relay]

From: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>

APDS9160 is a combination of ALS and proximity sensors.

This patch add supports for:
    - Intensity clear data and illuminance data
    - Proximity data
    - Gain control, rate control
    - Event thresholds

Signed-off-by: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
---
 MAINTAINERS                  |    7 +
 drivers/iio/light/Kconfig    |   13 +
 drivers/iio/light/Makefile   |    1 +
 drivers/iio/light/apds9160.c | 1420 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 1441 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index b878ddc99f94e7f6e8fa2c479c5a3f846c514730..5e57b4a19f2eccf317cda62c98d5e7545fdd185b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3694,6 +3694,13 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml
 F:	drivers/iio/light/apds9306.c
 
+AVAGO APDS9160 AMBIENT LIGHT SENSOR AND PROXIMITY DRIVER
+M:	Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/light/avago,apds9160.yaml
+F:	drivers/iio/light/apds9160.c
+
 AVIA HX711 ANALOG DIGITAL CONVERTER IIO DRIVER
 M:	Andreas Klinger <ak@it-klinger.de>
 L:	linux-iio@vger.kernel.org
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index f2f3e414849ab12a7c0ea2b08e9a3310eb18ebb7..69a59c6759acea89241ab76bfcdfafe3e5824066 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -63,6 +63,19 @@ config AL3320A
 	  To compile this driver as a module, choose M here: the
 	  module will be called al3320a.
 
+config APDS9160
+	tristate "APDS9160 combined als and proximity sensors"
+	select REGMAP_I2C
+	select IIO_BUFFER
+	select IIO_KFIFO_BUF
+	depends on I2C
+	help
+	   Say Y here if you want to build a driver for Broadcom APDS9160
+	   combined ambient light and proximity sensor chip.
+
+	   To compile this driver as a module, choose M here: the
+	   module will be called apds9160. If unsure, say N here.
+
 config APDS9300
 	tristate "APDS9300 ambient light sensor"
 	depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index 321010fc0b938a9a7fed6d7ec41c718f56fc83a6..6d62571ae2af9bf1edcc77d7ea244a0f10bf7b4c 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_ADJD_S311)		+= adjd_s311.o
 obj-$(CONFIG_ADUX1020)		+= adux1020.o
 obj-$(CONFIG_AL3010)		+= al3010.o
 obj-$(CONFIG_AL3320A)		+= al3320a.o
+obj-$(CONFIG_APDS9160)		+= apds9160.o
 obj-$(CONFIG_APDS9300)		+= apds9300.o
 obj-$(CONFIG_APDS9306)		+= apds9306.o
 obj-$(CONFIG_APDS9960)		+= apds9960.o
diff --git a/drivers/iio/light/apds9160.c b/drivers/iio/light/apds9160.c
new file mode 100644
index 0000000000000000000000000000000000000000..cb855f20725dba9fea1390a955889d905fd7eb4f
--- /dev/null
+++ b/drivers/iio/light/apds9160.c
@@ -0,0 +1,1420 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * This file is part of the APDS9160 sensor driver.
+ * Chip is combined proximity and ambient light sensor.
+ * Author: Mikael Gonella-Bolduc <m.gonella.bolduc@gmail.com>
+ */
+
+#include <linux/acpi.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/mutex.h>
+#include <linux/err.h>
+#include <linux/irq.h>
+#include <linux/i2c.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/events.h>
+#include <linux/iio/kfifo_buf.h>
+#include <linux/iio/sysfs.h>
+
+#define APDS9160_DRIVER_NAME "apds9160"
+#define APDS9160_REGMAP_NAME "apds9160_regmap"
+#define APDS9160_STARTUP_DELAY 25000 /* us */
+#define APDS9160_REG_CNT 37
+
+/** Main control register */
+#define APDS9160_REG_CTRL 0x00
+#define APDS9160_REG_CTRL_SWRESET BIT(4) /* 1: Activate reset */
+#define APDS9160_REG_CTRL_MODE_RGB BIT(2) /* 0: ALS & IR, 1: RGB & IR */
+#define APDS9160_REG_CTRL_EN_ALS BIT(1) /* 1: ALS active */
+#define APDS9160_REG_CTLR_EN_PS BIT(0) /* 1: PS active */
+
+/** Status register  */
+#define APDS9160_REG_SR_LS_INT BIT(3)
+#define APDS9160_REG_SR_LS_NEW_DATA BIT(2)
+#define APDS9160_REG_SR_PS_INT BIT(1)
+#define APDS9160_REG_SR_PS_NEW_DATA BIT(0)
+
+/* Interrupt configuration register */
+#define APDS9160_REG_INT_CFG 0x19
+#define APDS9160_REG_INT_CFG_EN_LS BIT(2) /* LS int enable */
+#define APDS9160_REG_INT_CFG_EN_PS BIT(0) /* PS int enable */
+#define APDS9160_REG_INT_PST 0x1A
+
+/* Proximity registers */
+#define APDS9160_REG_PS_LED 0x01
+#define APDS9160_REG_PS_PULSES 0x02
+#define APDS9160_REG_PS_MEAS_RATE 0x03
+#define APDS9160_REG_PS_THRES_HI_LSB 0x1B
+#define APDS9160_REG_PS_THRES_HI_MSB 0x1C
+#define APDS9160_REG_PS_THRES_LO_LSB 0x1D
+#define APDS9160_REG_PS_THRES_LO_MSB 0x1E
+#define APDS9160_REG_PS_DATA_LSB 0x08
+#define APDS9160_REG_PS_DATA_MSB 0x09
+#define APDS9160_REG_PS_CAN_LEVEL_DIG_LSB 0x1F
+#define APDS9160_REG_PS_CAN_LEVEL_DIG_MSB 0x20
+#define APDS9160_REG_PS_CAN_LEVEL_ANA_DUR 0x21
+#define APDS9160_REG_PS_CAN_LEVEL_ANA_CURRENT 0x22
+
+/* Light sensor registers */
+#define APDS9160_REG_LS_MEAS_RATE 0x04
+#define APDS9160_REG_LS_GAIN 0x05
+#define APDS9160_REG_LS_DATA_CLEAR_LSB 0x0A
+#define APDS9160_REG_LS_DATA_CLEAR 0x0B
+#define APDS9160_REG_LS_DATA_CLEAR_MSB 0x0C
+#define APDS9160_REG_LS_DATA_ALS_LSB 0x0D
+#define APDS9160_REG_LS_DATA_ALS 0x0E
+#define APDS9160_REG_LS_DATA_ALS_MSB 0x0F
+#define APDS9160_REG_LS_THRES_UP_LSB 0x24
+#define APDS9160_REG_LS_THRES_UP 0x25
+#define APDS9160_REG_LS_THRES_UP_MSB 0x26
+#define APDS9160_REG_LS_THRES_LO_LSB 0x27
+#define APDS9160_REG_LS_THRES_LO 0x28
+#define APDS9160_REG_LS_THRES_LO_MSB 0x29
+#define APDS9160_REG_LS_THRES_VAR 0x2A
+
+/** Part identification number register */
+#define APDS9160_REG_ID 0x06
+
+/** Status register */
+#define APDS9160_REG_SR 0x07
+#define APDS9160_REG_SR_DATA_ALS BIT(3)
+#define APDS9160_REG_SR_DATA_PS BIT(0)
+
+/* Supported ID:s */
+#define APDS9160_PART_ID_0 0x00
+#define APDS9160_PART_ID_MASK 0xF0
+#define APDS9160_PART_REV_MASK 0x0F
+
+#define APDS9160_PS_THRES_MAX 0x7FF
+#define APDS9160_LS_THRES_MAX 0xFFFFF
+#define APDS9160_CMD_LS_RESOLUTION_25MS 0x04
+#define APDS9160_CMD_LS_RESOLUTION_50MS 0x03
+#define APDS9160_CMD_LS_RESOLUTION_100MS 0x02
+#define APDS9160_CMD_LS_RESOLUTION_200MS 0x01
+#define APDS9160_PS_DATA_MASK 0x7FF
+
+#define APDS9160_DEFAULT_LS_GAIN 3
+#define APDS9160_DEFAULT_LS_RATE 100
+#define APDS9160_DEFAULT_PS_RATE 100
+#define APDS9160_DEFAULT_PS_CANCELLATION_LEVEL 0
+#define APDS9160_DEFAULT_PS_ANALOG_CANCELLATION 0
+#define APDS9160_DEFAULT_PS_GAIN 1
+#define APDS9160_DEFAULT_PS_CURRENT 100
+#define APDS9160_DEFAULT_PS_RESOLUTION 0x03 // 11 bits
+
+// clang-format off
+static const struct reg_default apds9160_reg_defaults[] = {
+	{ APDS9160_REG_CTRL, 0x00 }, /* Sensors disabled by default  */
+	{ APDS9160_REG_PS_LED, 0x33 }, /* 60 kHz frequency, 100 mA pulse current */
+	{ APDS9160_REG_PS_PULSES, 0x08 }, /* 8 pulses */
+	{ APDS9160_REG_PS_MEAS_RATE, 0x05 },
+	{ APDS9160_REG_LS_MEAS_RATE, 0x22 },
+	{ APDS9160_REG_LS_GAIN, 0x01 },
+	{ APDS9160_REG_INT_CFG, 0x10 },
+	{ APDS9160_REG_INT_PST, 0x00 },
+	{ APDS9160_REG_PS_THRES_HI_LSB, 0xFF },
+	{ APDS9160_REG_PS_THRES_HI_MSB, 0x07 },
+	{ APDS9160_REG_PS_THRES_LO_LSB, 0x00 },
+	{ APDS9160_REG_PS_THRES_LO_MSB, 0x00 },
+	{ APDS9160_REG_PS_CAN_LEVEL_DIG_LSB, 0x00 },
+	{ APDS9160_REG_PS_CAN_LEVEL_DIG_MSB, 0x00 },
+	{ APDS9160_REG_PS_CAN_LEVEL_ANA_DUR, 0x00 },
+	{ APDS9160_REG_PS_CAN_LEVEL_ANA_CURRENT, 0x00 },
+	{ APDS9160_REG_LS_THRES_UP_LSB, 0xFF },
+	{ APDS9160_REG_LS_THRES_UP, 0xFF },
+	{ APDS9160_REG_LS_THRES_UP_MSB, 0x0F },
+	{ APDS9160_REG_LS_THRES_LO_LSB, 0x00 },
+	{ APDS9160_REG_LS_THRES_LO, 0x00 },
+	{ APDS9160_REG_LS_THRES_LO_MSB, 0x00 },
+	{ APDS9160_REG_LS_THRES_VAR, 0x00 },
+};
+// clang-format on
+
+static const struct regmap_range apds9160_readable_ranges[] = {
+	regmap_reg_range(APDS9160_REG_CTRL, APDS9160_REG_LS_THRES_VAR),
+};
+
+static const struct regmap_access_table apds9160_readable_table = {
+	.yes_ranges = apds9160_readable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(apds9160_readable_ranges),
+};
+
+static const struct regmap_range apds9160_writeable_ranges[] = {
+	regmap_reg_range(APDS9160_REG_CTRL, APDS9160_REG_LS_GAIN),
+	regmap_reg_range(APDS9160_REG_INT_CFG, APDS9160_REG_LS_THRES_VAR),
+};
+
+static const struct regmap_access_table apds9160_writeable_table = {
+	.yes_ranges = apds9160_writeable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(apds9160_writeable_ranges),
+};
+
+static const struct regmap_range apds9160_volatile_ranges[] = {
+	regmap_reg_range(APDS9160_REG_SR, APDS9160_REG_LS_DATA_ALS_MSB),
+};
+
+static const struct regmap_access_table apds9160_volatile_table = {
+	.yes_ranges = apds9160_volatile_ranges,
+	.n_yes_ranges = ARRAY_SIZE(apds9160_volatile_ranges),
+};
+
+static const struct regmap_config apds9160_regmap_config = {
+	.name = APDS9160_REGMAP_NAME,
+	.reg_bits = 8,
+	.val_bits = 8,
+	.use_single_read = true,
+	.use_single_write = true,
+
+	.rd_table = &apds9160_readable_table,
+	.wr_table = &apds9160_writeable_table,
+	.volatile_table = &apds9160_volatile_table,
+
+	.reg_defaults = apds9160_reg_defaults,
+	.num_reg_defaults = ARRAY_SIZE(apds9160_reg_defaults),
+	.max_register = APDS9160_REG_CNT,
+	.cache_type = REGCACHE_RBTREE,
+};
+
+static const struct iio_event_spec apds9160_ps_event_spec[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+	},
+};
+
+static const struct iio_event_spec apds9160_als_event_spec[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+	},
+};
+
+static const struct iio_chan_spec apds9160_channels[] = {
+	{
+		/* Proximity sensor channel */
+		.type = IIO_PROXIMITY,
+		.address = APDS9160_REG_PS_DATA_LSB,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+					    BIT(IIO_CHAN_INFO_CALIBSCALE) |
+					    BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
+					    BIT(IIO_CHAN_INFO_CALIBBIAS),
+		.channel = 0,
+		.indexed = 0,
+		.scan_index = -1,
+
+		.event_spec = apds9160_ps_event_spec,
+		.num_event_specs = ARRAY_SIZE(apds9160_ps_event_spec),
+	},
+	{
+		/* Proximity sensor led current */
+		.type = IIO_CURRENT,
+		.output = 1,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.scan_index = -1,
+	},
+	{
+		/* Clear channel */
+		.type = IIO_INTENSITY,
+		.address = APDS9160_REG_LS_DATA_CLEAR_LSB,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_INT_TIME) |
+					    BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
+					    BIT(IIO_CHAN_INFO_SCALE),
+		.channel2 = IIO_MOD_LIGHT_CLEAR,
+		.modified = 1,
+		.scan_index = -1,
+
+		.event_spec = apds9160_als_event_spec,
+		.num_event_specs = ARRAY_SIZE(apds9160_als_event_spec),
+	},
+	{
+		/* Illuminance */
+		.type = IIO_LIGHT,
+		.address = APDS9160_REG_LS_DATA_ALS_LSB,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = -1,
+	}
+};
+
+/* Attributes */
+// clang-format off
+static const int apds9160_als_rate_map[][2] = {
+	{ 25, 0x00 },
+	{ 50, 0x01 },
+	{ 100, 0x02 },
+	{ 200, 0x03 }
+};
+
+static const int apds9160_als_gain_map[][2] = {
+	{ 1, 0x00 },
+	{ 3, 0x01 },
+	{ 6, 0x02 },
+	{ 18, 0x03 },
+	{ 64, 0x04 }
+};
+
+static const int apds9160_ps_gain_map[][2] = {
+	{ 1, 0x00 },
+	{ 2, 0x01},
+	{ 4, 0x02},
+	{ 8, 0x03}
+};
+
+static const int apds9160_ps_rate_map[][2] = {
+	{ 25, 0x03 },
+	{ 50, 0x04 },
+	{ 100, 0x05 },
+	{ 200, 0x06 },
+	{ 400, 0x07 }
+};
+
+static const int adps9160_ps_led_current_map[][2] = {
+	{ 10, 0x00 },
+	{ 25, 0x01 },
+	{ 50, 0x02 },
+	{ 100, 0x03 },
+	{ 150, 0x04},
+	{ 175, 0x05 },
+	{ 200, 0x06 }
+};
+// clang-format on
+
+struct apds9160_scale {
+	int itime;
+	int gain;
+	int scale1;
+	int scale2;
+};
+
+static const struct apds9160_scale apds9160_als_scale_map[] = {
+	{
+		.gain = 1,
+		.itime = 25,
+		.scale1 = 3272,
+		.scale2 = 1000,
+	},
+	{
+		.gain = 1,
+		.itime = 50,
+		.scale1 = 1639,
+		.scale2 = 1000,
+	},
+	{
+		.gain = 1,
+		.itime = 100,
+		.scale1 = 819,
+		.scale2 = 1000,
+	},
+	{
+		.gain = 3,
+		.itime = 25,
+		.scale1 = 1077,
+		.scale2 = 1000,
+	},
+	{
+		.gain = 3,
+		.itime = 50,
+		.scale1 = 538,
+		.scale2 = 1000,
+	},
+	{
+		.gain = 3,
+		.itime = 100,
+		.scale1 = 269,
+		.scale2 = 1000,
+	},
+	{
+		.gain = 6,
+		.itime = 25,
+		.scale1 = 525,
+		.scale2 = 1000,
+	},
+	{
+		.gain = 6,
+		.itime = 50,
+		.scale1 = 263,
+		.scale2 = 1000,
+	},
+	{
+		.gain = 6,
+		.itime = 100,
+		.scale1 = 131,
+		.scale2 = 1000,
+	},
+	{
+		.gain = 18,
+		.itime = 25,
+		.scale1 = 169,
+		.scale2 = 1000,
+	},
+	{
+		.gain = 18,
+		.itime = 50,
+		.scale1 = 84,
+		.scale2 = 1000,
+	},
+	{
+		.gain = 18,
+		.itime = 100,
+		.scale1 = 42,
+		.scale2 = 1000,
+	},
+	{
+		.gain = 64,
+		.itime = 25,
+		.scale1 = 49,
+		.scale2 = 1000,
+	},
+	{
+		.gain = 64,
+		.itime = 50,
+		.scale1 = 25,
+		.scale2 = 1000,
+	},
+	{
+		.gain = 64,
+		.itime = 100,
+		.scale1 = 12,
+		.scale2 = 1000,
+	},
+};
+
+static IIO_CONST_ATTR(in_intensity_integration_time_available, "25 50 100 200");
+static IIO_CONST_ATTR(in_proximity_sampling_frequency_available,
+		      "25 50 100 200 400");
+static IIO_CONST_ATTR(in_intensity_hardwaregain_available, "1 3 6 18 64");
+static IIO_CONST_ATTR(in_proximity_hardwaregain_available, "1 2 4 8");
+static IIO_CONST_ATTR(out_current_available, "10 25 50 100 150 175 200");
+
+static struct attribute *apds9160_attributes[] = {
+	&iio_const_attr_in_intensity_integration_time_available.dev_attr.attr,
+	&iio_const_attr_in_intensity_hardwaregain_available.dev_attr.attr,
+	&iio_const_attr_in_proximity_sampling_frequency_available.dev_attr.attr,
+	&iio_const_attr_in_proximity_hardwaregain_available.dev_attr.attr,
+	&iio_const_attr_out_current_available.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group apds9160_attribute_group = {
+	.attrs = apds9160_attributes,
+};
+
+static const struct reg_field apds9160_reg_field_ls_en =
+	REG_FIELD(APDS9160_REG_CTRL, 1, 1);
+
+static const struct reg_field apds9160_reg_field_ps_en =
+	REG_FIELD(APDS9160_REG_CTRL, 0, 0);
+
+static const struct reg_field apds9160_reg_field_int_ps =
+	REG_FIELD(APDS9160_REG_INT_CFG, 0, 0);
+
+static const struct reg_field apds9160_reg_field_int_als =
+	REG_FIELD(APDS9160_REG_INT_CFG, 2, 2);
+
+static const struct reg_field apds9160_reg_field_ps_overflow =
+	REG_FIELD(APDS9160_REG_PS_DATA_MSB, 3, 3);
+
+static const struct reg_field apds9160_reg_field_als_rate =
+	REG_FIELD(APDS9160_REG_LS_MEAS_RATE, 0, 2);
+
+static const struct reg_field apds9160_reg_field_als_gain =
+	REG_FIELD(APDS9160_REG_LS_GAIN, 0, 2);
+
+static const struct reg_field apds9160_reg_field_ps_rate =
+	REG_FIELD(APDS9160_REG_PS_MEAS_RATE, 0, 2);
+
+static const struct reg_field apds9160_reg_field_als_res =
+	REG_FIELD(APDS9160_REG_LS_MEAS_RATE, 4, 6);
+
+static const struct reg_field apds9160_reg_field_ps_current =
+	REG_FIELD(APDS9160_REG_PS_LED, 0, 2);
+
+static const struct reg_field apds9160_reg_field_ps_gain =
+	REG_FIELD(APDS9160_REG_PS_MEAS_RATE, 6, 7);
+
+static const struct reg_field apds9160_reg_field_ps_resolution =
+	REG_FIELD(APDS9160_REG_PS_MEAS_RATE, 3, 4);
+
+struct apds9160_chip {
+	struct i2c_client *client;
+	struct iio_dev *indio_dev;
+	struct regmap *regmap;
+	struct mutex lock; /* avoid parallel access */
+
+	struct regmap_field *reg_enable_ps;
+	struct regmap_field *reg_enable_als;
+	struct regmap_field *reg_int_ps;
+	struct regmap_field *reg_int_als;
+	struct regmap_field *reg_ps_overflow;
+	struct regmap_field *reg_als_rate;
+	struct regmap_field *reg_als_resolution;
+	struct regmap_field *reg_ps_rate;
+	struct regmap_field *reg_als_gain;
+	struct regmap_field *reg_ps_current;
+	struct regmap_field *reg_ps_gain;
+	struct regmap_field *reg_ps_resolution;
+
+	/* State data */
+	u8 revision;
+	int als_int;
+	int ps_int;
+
+	/* Configuration values */
+	int als_itime;
+	int als_hwgain;
+	int als_scale1;
+	int als_scale2;
+	int ps_rate;
+	int ps_cancellation_level;
+	int ps_cancellation_analog;
+	int ps_current;
+	int ps_gain;
+};
+
+static const struct i2c_device_id apds9160_id[] = { { APDS9160_DRIVER_NAME, 0 },
+						    {} };
+
+/** Called when mutex is locked */
+static void apds9160_set_scale(struct apds9160_chip *data)
+{
+	for (int idx = 0; idx < ARRAY_SIZE(apds9160_als_scale_map); idx++) {
+		if (data->als_hwgain == apds9160_als_scale_map[idx].gain &&
+		    data->als_itime == apds9160_als_scale_map[idx].itime) {
+			data->als_scale1 = apds9160_als_scale_map[idx].scale1;
+			data->als_scale2 = apds9160_als_scale_map[idx].scale2;
+		}
+	}
+}
+
+static int apds9160_set_ps_rate(struct apds9160_chip *data, int val)
+{
+	int ret = -EINVAL;
+	int idx;
+
+	dev_dbg(&data->client->dev, "%s - set rate to %i\n", __func__, val);
+	for (idx = 0; idx < ARRAY_SIZE(apds9160_ps_rate_map); idx++) {
+		if (apds9160_ps_rate_map[idx][0] == val) {
+			mutex_lock(&data->lock);
+			ret = regmap_field_write(data->reg_ps_rate,
+						 apds9160_ps_rate_map[idx][1]);
+			if (!ret)
+				data->ps_rate = val;
+			mutex_unlock(&data->lock);
+			break;
+		}
+	}
+
+	return ret;
+}
+
+static int apds9160_set_ps_gain(struct apds9160_chip *data, int val)
+{
+	int ret = -EINVAL;
+	int idx;
+
+	dev_dbg(&data->client->dev, "%s - set gain to %i\n", __func__, val);
+	for (idx = 0; idx < ARRAY_SIZE(apds9160_ps_gain_map); idx++) {
+		if (apds9160_ps_gain_map[idx][0] == val) {
+			mutex_lock(&data->lock);
+			ret = regmap_field_write(data->reg_ps_gain,
+						 apds9160_ps_gain_map[idx][1]);
+			if (!ret)
+				data->ps_gain = val;
+			mutex_unlock(&data->lock);
+			break;
+		}
+	}
+
+	return ret;
+}
+
+/**
+ * The PS intelligent cancellation level register allows for an on-chip substraction
+ * of the ADC count caused by unwanted reflected light from PS ADC output.
+ */
+static int apds9160_set_ps_cancellation_level(struct apds9160_chip *data,
+					      int val)
+{
+	int ret = -EINVAL;
+	__le16 buf;
+
+	dev_dbg(&data->client->dev, "%s - set cancellation level to %i\n",
+		__func__, val);
+	if (val < 0 || val > 0xFFFF)
+		return ret;
+
+	mutex_lock(&data->lock);
+	buf = cpu_to_le16(val);
+	ret = regmap_bulk_write(data->regmap, APDS9160_REG_PS_CAN_LEVEL_DIG_LSB,
+				&buf, 2);
+	if (!ret)
+		data->ps_cancellation_level = val;
+	mutex_unlock(&data->lock);
+	return ret;
+}
+
+/**
+ * This parameter determines the cancellation pulse duration in each of the PWM pulse.
+ * The cancellation is applied during the integration phase of the PS measurement.
+ * Duration is programmed in half clock cycles
+ */
+static int apds9160_set_ps_analog_cancellation(struct apds9160_chip *data,
+					       int val)
+{
+	int ret = -EINVAL;
+
+	dev_dbg(&data->client->dev, "%s - set analog cancellation to %i\n",
+		__func__, val);
+	if (val < 0 || val > 0x3F)
+		return ret;
+
+	mutex_lock(&data->lock);
+	ret = regmap_write(data->regmap, APDS9160_REG_PS_CAN_LEVEL_ANA_DUR,
+			   val);
+	if (!ret)
+		data->ps_cancellation_analog = val;
+	mutex_unlock(&data->lock);
+	return ret;
+}
+
+/**
+ * Set the proximity sensor led current
+ */
+static int apds9160_set_ps_current(struct apds9160_chip *data, int val)
+{
+	int ret = -EINVAL;
+	int idx;
+
+	dev_dbg(&data->client->dev, "%s - set current to %i\n", __func__, val);
+	for (idx = 0; idx < ARRAY_SIZE(adps9160_ps_led_current_map); idx++) {
+		if (adps9160_ps_led_current_map[idx][0] == val) {
+			mutex_lock(&data->lock);
+			ret = regmap_field_write(
+				data->reg_ps_current,
+				adps9160_ps_led_current_map[idx][1]);
+
+			if (!ret)
+				data->ps_current = val;
+
+			mutex_unlock(&data->lock);
+			break;
+		}
+	}
+	return ret;
+}
+
+static int apds9160_set_als_gain(struct apds9160_chip *data, int val)
+{
+	int ret = -EINVAL;
+	int idx;
+
+	dev_dbg(&data->client->dev, "%s - set als gain to %i\n", __func__, val);
+	for (idx = 0; idx < ARRAY_SIZE(apds9160_als_gain_map); idx++) {
+		if (apds9160_als_gain_map[idx][0] == val) {
+			mutex_lock(&data->lock);
+			ret = regmap_field_write(data->reg_als_gain,
+						 apds9160_als_gain_map[idx][1]);
+			if (!ret) {
+				data->als_hwgain = val;
+				apds9160_set_scale(data);
+			}
+			mutex_unlock(&data->lock);
+			break;
+		}
+	}
+	return ret;
+}
+
+static int apds9160_set_als_int_time(struct apds9160_chip *data, int val)
+{
+	int ret = -EINVAL;
+	int idx;
+
+	dev_dbg(&data->client->dev, "%s - set int time to %i\n", __func__, val);
+	for (idx = 0; idx < ARRAY_SIZE(apds9160_als_rate_map); idx++) {
+		if (apds9160_als_rate_map[idx][0] == val) {
+			mutex_lock(&data->lock);
+			ret = regmap_field_write(data->reg_als_rate,
+						 apds9160_als_rate_map[idx][1]);
+			if (!ret) {
+				data->als_itime = val;
+				/* Lower resolution for faster rates */
+				switch (val) {
+				case 25:
+					ret = regmap_field_write(
+						data->reg_als_resolution,
+						APDS9160_CMD_LS_RESOLUTION_25MS);
+					break;
+				case 50:
+					ret = regmap_field_write(
+						data->reg_als_resolution,
+						APDS9160_CMD_LS_RESOLUTION_50MS);
+					break;
+				case 200:
+					ret = regmap_field_write(
+						data->reg_als_resolution,
+						APDS9160_CMD_LS_RESOLUTION_200MS);
+					break;
+				default:
+					ret = regmap_field_write(
+						data->reg_als_resolution,
+						APDS9160_CMD_LS_RESOLUTION_100MS);
+				}
+				apds9160_set_scale(data);
+			}
+			mutex_unlock(&data->lock);
+			break;
+		}
+	}
+
+	return ret;
+}
+
+static int apds9160_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, int *val,
+			     int *val2, long mask)
+{
+	struct apds9160_chip *data = iio_priv(indio_dev);
+	int ret = -EINVAL;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		switch (chan->type) {
+		case IIO_PROXIMITY: {
+			__le16 buf;
+
+			ret = regmap_bulk_read(data->regmap, chan->address,
+					       &buf, 2);
+			if (!ret) {
+				ret = IIO_VAL_INT;
+				*val = le16_to_cpu(buf);
+				// Remove data overflow and ambient light overflow bits from result
+				*val &= APDS9160_PS_DATA_MASK;
+			}
+		} break;
+		case IIO_LIGHT:
+		case IIO_INTENSITY: {
+			__le32 buf = 0;
+
+			ret = regmap_bulk_read(data->regmap, chan->address,
+					       &buf, 3);
+			if (!ret) {
+				ret = IIO_VAL_INT;
+				*val = le32_to_cpu(buf);
+			}
+		} break;
+		case IIO_CURRENT: {
+			ret = IIO_VAL_INT;
+			*val = data->ps_current;
+		} break;
+		default:
+			break;
+		}
+		break;
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		switch (chan->type) {
+		case IIO_INTENSITY:
+			ret = IIO_VAL_INT;
+			*val = data->als_hwgain;
+			break;
+		case IIO_PROXIMITY:
+			ret = IIO_VAL_INT;
+			*val = data->ps_gain;
+			break;
+		default:
+			break;
+		}
+		break;
+	case IIO_CHAN_INFO_INT_TIME:
+		switch (chan->type) {
+		case IIO_INTENSITY:
+			ret = IIO_VAL_INT;
+			*val = data->als_itime;
+			break;
+		default:
+			break;
+		}
+		break;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		switch (chan->type) {
+		case IIO_PROXIMITY:
+			ret = IIO_VAL_INT;
+			*val = data->ps_rate;
+			break;
+		default:
+			break;
+		}
+		break;
+	case IIO_CHAN_INFO_CALIBSCALE:
+		switch (chan->type) {
+		case IIO_PROXIMITY:
+			ret = IIO_VAL_INT;
+			*val = data->ps_cancellation_level;
+			break;
+		default:
+			break;
+		}
+		break;
+	case IIO_CHAN_INFO_CALIBBIAS:
+		switch (chan->type) {
+		case IIO_PROXIMITY:
+			ret = IIO_VAL_INT;
+			*val = data->ps_cancellation_analog;
+			break;
+		default:
+			break;
+		}
+		break;
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_LIGHT:
+		case IIO_INTENSITY:
+			ret = IIO_VAL_FRACTIONAL;
+			*val = data->als_scale1;
+			*val2 = data->als_scale2;
+			break;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+};
+
+static int apds9160_write_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan, int val,
+			      int val2, long mask)
+{
+	struct apds9160_chip *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		if (val2 != 0)
+			return -EINVAL;
+		switch (chan->type) {
+		case IIO_INTENSITY:
+			return apds9160_set_als_int_time(data, val);
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		if (val2 != 0)
+			return -EINVAL;
+		switch (chan->type) {
+		case IIO_PROXIMITY:
+			return apds9160_set_ps_rate(data, val);
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		if (val2 != 0)
+			return -EINVAL;
+		switch (chan->type) {
+		case IIO_INTENSITY:
+			return apds9160_set_als_gain(data, val);
+		case IIO_PROXIMITY:
+			return apds9160_set_ps_gain(data, val);
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_CALIBSCALE:
+		if (val2 != 0)
+			return -EINVAL;
+		switch (chan->type) {
+		case IIO_PROXIMITY:
+			return apds9160_set_ps_cancellation_level(data, val);
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_CALIBBIAS:
+		if (val2 != 0)
+			return -EINVAL;
+		switch (chan->type) {
+		case IIO_PROXIMITY:
+			return apds9160_set_ps_analog_cancellation(data, val);
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_RAW:
+		if (val2 != 0)
+			return -EINVAL;
+		switch (chan->type) {
+		case IIO_CURRENT:
+			return apds9160_set_ps_current(data, val);
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static inline int apds9160_get_thres_reg(const struct iio_chan_spec *chan,
+					 enum iio_event_direction dir, u8 *reg)
+{
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		switch (chan->type) {
+		case IIO_PROXIMITY:
+			*reg = APDS9160_REG_PS_THRES_HI_LSB;
+			break;
+		case IIO_INTENSITY:
+			*reg = APDS9160_REG_LS_THRES_UP_LSB;
+			break;
+		default:
+			return -EINVAL;
+		}
+		break;
+	case IIO_EV_DIR_FALLING:
+		switch (chan->type) {
+		case IIO_PROXIMITY:
+			*reg = APDS9160_REG_PS_THRES_LO_LSB;
+			break;
+		case IIO_INTENSITY:
+			*reg = APDS9160_REG_LS_THRES_LO_LSB;
+			break;
+		default:
+			return -EINVAL;
+		}
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int apds9160_read_event(struct iio_dev *indio_dev,
+			       const struct iio_chan_spec *chan,
+			       enum iio_event_type type,
+			       enum iio_event_direction dir,
+			       enum iio_event_info info, int *val, int *val2)
+{
+	u8 reg;
+
+	int ret = 0;
+	struct apds9160_chip *data = iio_priv(indio_dev);
+
+	if (info != IIO_EV_INFO_VALUE)
+		return -EINVAL;
+
+	ret = apds9160_get_thres_reg(chan, dir, &reg);
+	if (ret < 0)
+		return ret;
+
+	if (chan->type == IIO_PROXIMITY) {
+		__le16 buf;
+
+		ret = regmap_bulk_read(data->regmap, reg, &buf, 2);
+		if (ret < 0)
+			return ret;
+		*val = le16_to_cpu(buf);
+	} else if (chan->type == IIO_INTENSITY) {
+		__le32 buf = 0;
+
+		ret = regmap_bulk_read(data->regmap, reg, &buf, 3);
+		if (ret < 0)
+			return ret;
+		*val = le32_to_cpu(buf);
+	} else
+		return -EINVAL;
+
+	*val2 = 0;
+
+	return IIO_VAL_INT;
+}
+
+static int apds9160_write_event(struct iio_dev *indio_dev,
+				const struct iio_chan_spec *chan,
+				enum iio_event_type type,
+				enum iio_event_direction dir,
+				enum iio_event_info info, int val, int val2)
+{
+	u8 reg;
+	int ret = 0;
+	struct apds9160_chip *data = iio_priv(indio_dev);
+
+	if (info != IIO_EV_INFO_VALUE)
+		return -EINVAL;
+
+	ret = apds9160_get_thres_reg(chan, dir, &reg);
+	if (ret < 0)
+		return ret;
+
+	if (chan->type == IIO_PROXIMITY) {
+		if (val < 0 || val > APDS9160_PS_THRES_MAX)
+			return -EINVAL;
+		__le16 buf;
+
+		buf = cpu_to_le16(val);
+		ret = regmap_bulk_write(data->regmap, reg, &buf, 2);
+		if (ret < 0)
+			return ret;
+	} else if (chan->type == IIO_INTENSITY) {
+		if (val < 0 || val > APDS9160_LS_THRES_MAX)
+			return -EINVAL;
+		__le32 buf = 0;
+
+		buf = cpu_to_le32(val);
+		ret = regmap_bulk_write(data->regmap, reg, &buf, 3);
+		if (ret < 0)
+			return ret;
+	} else
+		return -EINVAL;
+
+	return 0;
+}
+
+static int apds9160_read_event_config(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      enum iio_event_type type,
+				      enum iio_event_direction dir)
+{
+	struct apds9160_chip *data = iio_priv(indio_dev);
+
+	switch (chan->type) {
+	case IIO_PROXIMITY:
+		return data->ps_int;
+	case IIO_INTENSITY:
+		return data->als_int;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int apds9160_write_event_config(struct iio_dev *indio_dev,
+				       const struct iio_chan_spec *chan,
+				       enum iio_event_type type,
+				       enum iio_event_direction dir, int state)
+{
+	struct apds9160_chip *data = iio_priv(indio_dev);
+	int ret;
+
+	state = !!state;
+
+	switch (chan->type) {
+	case IIO_PROXIMITY:
+		if (data->ps_int == state)
+			return -EINVAL;
+
+		ret = regmap_field_write(data->reg_int_ps, state);
+		if (ret)
+			return ret;
+		data->ps_int = state;
+		break;
+	case IIO_INTENSITY:
+		if (data->als_int == state)
+			return -EINVAL;
+
+		ret = regmap_field_write(data->reg_int_als, state);
+		if (ret)
+			return ret;
+		data->als_int = state;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static irqreturn_t apds9160_irq_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct apds9160_chip *data = iio_priv(indio_dev);
+	int ret, status;
+
+	/* Reading status register clears the interrupt flag */
+	ret = regmap_read(data->regmap, APDS9160_REG_SR, &status);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "irq status reg read failed\n");
+		return IRQ_HANDLED;
+	}
+
+	if ((status & APDS9160_REG_SR_LS_INT) &&
+	    (status & APDS9160_REG_SR_LS_NEW_DATA) && data->als_int) {
+		iio_push_event(indio_dev,
+			       IIO_UNMOD_EVENT_CODE(IIO_INTENSITY, 0,
+						    IIO_EV_TYPE_THRESH,
+						    IIO_EV_DIR_EITHER),
+			       iio_get_time_ns(indio_dev));
+	}
+
+	if ((status & APDS9160_REG_SR_PS_INT) &&
+	    (status & APDS9160_REG_SR_PS_NEW_DATA) && data->ps_int) {
+		/** Interrupt flag is cleared after data read */
+		iio_push_event(indio_dev,
+			       IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
+						    IIO_EV_TYPE_THRESH,
+						    IIO_EV_DIR_EITHER),
+			       iio_get_time_ns(indio_dev));
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int apds9160_detect(struct apds9160_chip *chip)
+{
+	struct i2c_client *client = chip->client;
+	int ret;
+	u32 val;
+	u8 id;
+	u8 rev;
+
+	ret = regmap_read(chip->regmap, APDS9160_REG_ID, &val);
+	if (ret < 0) {
+		dev_err(&client->dev, "ID read failed\n");
+		return ret;
+	}
+
+	id = APDS9160_PART_ID_MASK & ((u8)val);
+	rev = APDS9160_PART_REV_MASK & ((u8)val);
+
+	switch (id) {
+	case APDS9160_PART_ID_0:
+		chip->revision = rev;
+		dev_info(&client->dev, "Device probed, rev %u\n",
+			 chip->revision);
+		break;
+	default:
+		dev_info(&client->dev, "Unsupported part id %u rev %u\n", id,
+			 rev);
+		ret = -ENODEV;
+		break;
+	}
+	return ret;
+}
+
+static int apds9160_chip_init(struct apds9160_chip *chip)
+{
+	int ret;
+
+	/* Write default values to interrupt register */
+	ret = regmap_field_write(chip->reg_int_ps, 0);
+	chip->ps_int = 0;
+	if (ret)
+		return ret;
+
+	ret = regmap_field_write(chip->reg_int_als, 0);
+	chip->als_int = 0;
+	if (ret)
+		return ret;
+
+	/* Write default values to control register */
+	ret = regmap_field_write(chip->reg_enable_als, 1);
+	if (ret)
+		return ret;
+
+	ret = regmap_field_write(chip->reg_enable_ps, 1);
+	if (ret)
+		return ret;
+
+	/* Write other default values */
+	ret = regmap_field_write(chip->reg_ps_resolution,
+				 APDS9160_DEFAULT_PS_RESOLUTION);
+	if (ret)
+		return ret;
+
+	/* Write default values to configuration registers */
+	ret = apds9160_set_ps_current(chip, APDS9160_DEFAULT_PS_CURRENT);
+	if (ret)
+		return ret;
+
+	apds9160_set_ps_rate(chip, APDS9160_DEFAULT_PS_RATE);
+	if (ret)
+		return ret;
+
+	apds9160_set_als_int_time(chip, APDS9160_DEFAULT_LS_RATE);
+	if (ret)
+		return ret;
+
+	ret = apds9160_set_als_gain(chip, APDS9160_DEFAULT_LS_GAIN);
+	if (ret)
+		return ret;
+
+	ret = apds9160_set_ps_gain(chip, APDS9160_DEFAULT_PS_GAIN);
+	if (ret)
+		return ret;
+
+	ret = apds9160_set_ps_analog_cancellation(
+		chip, APDS9160_DEFAULT_PS_ANALOG_CANCELLATION);
+	if (ret)
+		return ret;
+
+	return apds9160_set_ps_cancellation_level(
+		chip, APDS9160_DEFAULT_PS_CANCELLATION_LEVEL);
+}
+
+static int apds9160_configure(struct iio_dev *indio_dev)
+{
+	struct apds9160_chip *chip = iio_priv(indio_dev);
+
+	return apds9160_chip_init(chip);
+}
+
+static int apds9160_regfield_init(struct apds9160_chip *data)
+{
+	struct device *dev = &data->client->dev;
+	struct regmap *regmap = data->regmap;
+
+	data->reg_int_als = devm_regmap_field_alloc(dev, regmap,
+						    apds9160_reg_field_int_als);
+	if (IS_ERR(data->reg_int_als)) {
+		dev_err(dev, "INT ALS reg field init failed\n");
+		return PTR_ERR(data->reg_int_als);
+	}
+
+	data->reg_int_ps =
+		devm_regmap_field_alloc(dev, regmap, apds9160_reg_field_int_ps);
+	if (IS_ERR(data->reg_int_ps)) {
+		dev_err(dev, "INT ps reg field init failed\n");
+		return PTR_ERR(data->reg_int_ps);
+	}
+
+	data->reg_enable_als =
+		devm_regmap_field_alloc(dev, regmap, apds9160_reg_field_ls_en);
+	if (IS_ERR(data->reg_enable_als)) {
+		dev_err(dev, "Enable ALS reg field init failed\n");
+		return PTR_ERR(data->reg_enable_als);
+	}
+
+	data->reg_enable_ps =
+		devm_regmap_field_alloc(dev, regmap, apds9160_reg_field_ps_en);
+	if (IS_ERR(data->reg_enable_ps)) {
+		dev_err(dev, "Enable PS reg field init failed\n");
+		return PTR_ERR(data->reg_enable_ps);
+	}
+
+	data->reg_ps_overflow = devm_regmap_field_alloc(
+		dev, regmap, apds9160_reg_field_ps_overflow);
+	if (IS_ERR(data->reg_ps_overflow)) {
+		dev_err(dev, "PS overflow reg field init failed\n");
+		return PTR_ERR(data->reg_ps_overflow);
+	}
+
+	data->reg_als_rate = devm_regmap_field_alloc(
+		dev, regmap, apds9160_reg_field_als_rate);
+	if (IS_ERR(data->reg_als_rate)) {
+		dev_err(dev, "ALS measurement rate field init failed\n");
+		return PTR_ERR(data->reg_als_rate);
+	}
+
+	data->reg_als_resolution = devm_regmap_field_alloc(
+		dev, regmap, apds9160_reg_field_als_res);
+	if (IS_ERR(data->reg_als_resolution)) {
+		dev_err(dev, "ALS resolution field init failed\n");
+		return PTR_ERR(data->reg_als_resolution);
+	}
+
+	data->reg_ps_rate = devm_regmap_field_alloc(dev, regmap,
+						    apds9160_reg_field_ps_rate);
+	if (IS_ERR(data->reg_ps_rate)) {
+		dev_err(dev, "PS measurement rate field init failed\n");
+		return PTR_ERR(data->reg_ps_rate);
+	}
+
+	data->reg_als_gain = devm_regmap_field_alloc(
+		dev, regmap, apds9160_reg_field_als_gain);
+	if (IS_ERR(data->reg_als_gain)) {
+		dev_err(dev, "ALS gain field init failed\n");
+		return PTR_ERR(data->reg_als_gain);
+	}
+
+	data->reg_ps_current = devm_regmap_field_alloc(
+		dev, regmap, apds9160_reg_field_ps_current);
+	if (IS_ERR(data->reg_ps_current)) {
+		dev_err(dev, "PS current field init failed\n");
+		return PTR_ERR(data->reg_ps_current);
+	}
+
+	data->reg_ps_gain = devm_regmap_field_alloc(dev, regmap,
+						    apds9160_reg_field_ps_gain);
+	if (IS_ERR(data->reg_ps_gain)) {
+		dev_err(dev, "PS gain field init failed\n");
+		return PTR_ERR(data->reg_ps_gain);
+	}
+
+	data->reg_ps_resolution = devm_regmap_field_alloc(
+		dev, regmap, apds9160_reg_field_ps_resolution);
+	if (IS_ERR(data->reg_ps_resolution)) {
+		dev_err(dev, "PS resolution field init failed\n");
+		return PTR_ERR(data->reg_ps_resolution);
+	}
+
+	return 0;
+}
+
+static int apds9160_disable(struct apds9160_chip *data)
+{
+	int ret;
+
+	ret = regmap_field_write(data->reg_enable_als, 0);
+	if (ret)
+		return ret;
+
+	return regmap_field_write(data->reg_enable_ps, 0);
+}
+
+static int apds9160_shutdown(struct iio_dev *indio_dev)
+{
+	struct apds9160_chip *data = iio_priv(indio_dev);
+
+	return apds9160_disable(data);
+}
+
+static void apds9160_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+
+	struct apds9160_chip *data = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+	apds9160_disable(data);
+}
+
+static const struct iio_buffer_setup_ops apds9160_buffer_setup_ops = {
+	.postenable = apds9160_configure,
+	.predisable = apds9160_shutdown,
+};
+
+static const struct iio_info apds9160_info = {
+	.attrs = &apds9160_attribute_group,
+	.read_raw = apds9160_read_raw,
+	.write_raw = apds9160_write_raw,
+	.read_event_value = apds9160_read_event,
+	.write_event_value = apds9160_write_event,
+	.read_event_config = apds9160_read_event_config,
+	.write_event_config = apds9160_write_event_config,
+};
+
+static int apds9160_probe(struct i2c_client *client)
+{
+	struct apds9160_chip *chip;
+	struct iio_dev *indio_dev;
+	int err;
+
+	dev_info(&client->dev,
+		 "Loading proximity/ambient light sensor driver\n");
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
+	indio_dev->info = &apds9160_info;
+	indio_dev->name = APDS9160_DRIVER_NAME;
+	indio_dev->channels = apds9160_channels;
+	indio_dev->num_channels = ARRAY_SIZE(apds9160_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	err = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
+					  &apds9160_buffer_setup_ops);
+	if (err)
+		return err;
+
+	chip = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+	chip->client = client;
+	chip->regmap = devm_regmap_init_i2c(client, &apds9160_regmap_config);
+	if (IS_ERR(chip->regmap)) {
+		dev_err(&client->dev, "regmap initialization failed.\n");
+		return PTR_ERR(chip->regmap);
+	}
+
+	chip->client = client;
+	chip->indio_dev = indio_dev;
+	mutex_init(&chip->lock);
+
+	err = apds9160_detect(chip);
+	if (err < 0) {
+		dev_err(&client->dev, "apds9160 not found\n");
+		return err;
+	}
+
+	err = apds9160_regfield_init(chip);
+	if (err)
+		return err;
+
+	err = apds9160_chip_init(chip);
+	if (err)
+		return err;
+
+	if (client->irq > 0) {
+		err = devm_request_threaded_irq(
+			&client->dev, client->irq, NULL, apds9160_irq_handler,
+			IRQF_TRIGGER_FALLING | IRQF_ONESHOT, "apds9160_event",
+			indio_dev);
+		if (err) {
+			dev_err(&client->dev, "request irq (%d) failed\n",
+				client->irq);
+			goto fail;
+		}
+	} else {
+		dev_info(&client->dev,
+			 "init: no IRQ defined, ps/als interrupts disabled\n");
+	}
+
+	err = iio_device_register(indio_dev);
+	if (err)
+		goto fail;
+
+	return 0;
+fail:
+	apds9160_disable(chip);
+	return err;
+}
+
+static const struct of_device_id apds9160_of_match[] = {
+	{ .compatible = "avago,apds9160" },
+	{ .compatible = "broadmobi,apds9160" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, apds9160_of_match);
+
+static struct i2c_driver apds9160_driver = {
+	.driver	  = {
+		.name	= APDS9160_DRIVER_NAME,
+		.owner = THIS_MODULE,
+		.of_match_table = apds9160_of_match,
+	},
+	.probe    = apds9160_probe,
+	.remove	  = apds9160_remove,
+	.id_table = apds9160_id,
+};
+
+MODULE_DEVICE_TABLE(i2c, apds9160_id);
+module_i2c_driver(apds9160_driver);
+MODULE_DESCRIPTION("APDS9160 combined ALS and proximity sensor");
+MODULE_AUTHOR("Mikael Gonella-Bolduc <m.gonella.bolduc@gmail.com>");
+MODULE_LICENSE("GPL");
+MODULE_VERSION("1.0");

-- 
2.34.1

Re: [PATCH 2/2] iio: light: Add APDS9160 ALS & Proximity sensor driver

[Krzysztof Kozlowski]

On 19/11/2024 21:36, Mikael Gonella-Bolduc via B4 Relay wrote:
> From: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
> 
> APDS9160 is a combination of ALS and proximity sensors.
> 
> This patch add supports for:
>     - Intensity clear data and illuminance data
>     - Proximity data
>     - Gain control, rate control
>     - Event thresholds
> 
> Signed-off-by: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
> ---
>  MAINTAINERS                  |    7 +
>  drivers/iio/light/Kconfig    |   13 +
>  drivers/iio/light/Makefile   |    1 +
>  drivers/iio/light/apds9160.c | 1420 ++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 1441 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b878ddc99f94e7f6e8fa2c479c5a3f846c514730..5e57b4a19f2eccf317cda62c98d5e7545fdd185b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3694,6 +3694,13 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml
>  F:	drivers/iio/light/apds9306.c
>  
> +AVAGO APDS9160 AMBIENT LIGHT SENSOR AND PROXIMITY DRIVER
> +M:	Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
> +L:	linux-iio@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/iio/light/avago,apds9160.yaml
> +F:	drivers/iio/light/apds9160.c
> +
>  AVIA HX711 ANALOG DIGITAL CONVERTER IIO DRIVER
>  M:	Andreas Klinger <ak@it-klinger.de>
>  L:	linux-iio@vger.kernel.org
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index f2f3e414849ab12a7c0ea2b08e9a3310eb18ebb7..69a59c6759acea89241ab76bfcdfafe3e5824066 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -63,6 +63,19 @@ config AL3320A
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called al3320a.
>  
> +config APDS9160
> +	tristate "APDS9160 combined als and proximity sensors"
> +	select REGMAP_I2C
> +	select IIO_BUFFER
> +	select IIO_KFIFO_BUF
> +	depends on I2C
> +	help
> +	   Say Y here if you want to build a driver for Broadcom APDS9160
> +	   combined ambient light and proximity sensor chip.
> +
> +	   To compile this driver as a module, choose M here: the
> +	   module will be called apds9160. If unsure, say N here.
> +
>  config APDS9300
>  	tristate "APDS9300 ambient light sensor"
>  	depends on I2C
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index 321010fc0b938a9a7fed6d7ec41c718f56fc83a6..6d62571ae2af9bf1edcc77d7ea244a0f10bf7b4c 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_ADJD_S311)		+= adjd_s311.o
>  obj-$(CONFIG_ADUX1020)		+= adux1020.o
>  obj-$(CONFIG_AL3010)		+= al3010.o
>  obj-$(CONFIG_AL3320A)		+= al3320a.o
> +obj-$(CONFIG_APDS9160)		+= apds9160.o
>  obj-$(CONFIG_APDS9300)		+= apds9300.o
>  obj-$(CONFIG_APDS9306)		+= apds9306.o
>  obj-$(CONFIG_APDS9960)		+= apds9960.o
> diff --git a/drivers/iio/light/apds9160.c b/drivers/iio/light/apds9160.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..cb855f20725dba9fea1390a955889d905fd7eb4f
> --- /dev/null
> +++ b/drivers/iio/light/apds9160.c
> @@ -0,0 +1,1420 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * This file is part of the APDS9160 sensor driver.
> + * Chip is combined proximity and ambient light sensor.
> + * Author: Mikael Gonella-Bolduc <m.gonella.bolduc@gmail.com>
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/mutex.h>
> +#include <linux/err.h>
> +#include <linux/irq.h>
> +#include <linux/i2c.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/kfifo_buf.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define APDS9160_DRIVER_NAME "apds9160"
> +#define APDS9160_REGMAP_NAME "apds9160_regmap"
> +#define APDS9160_STARTUP_DELAY 25000 /* us */
> +#define APDS9160_REG_CNT 37
> +
> +/** Main control register */
> +#define APDS9160_REG_CTRL 0x00
> +#define APDS9160_REG_CTRL_SWRESET BIT(4) /* 1: Activate reset */
> +#define APDS9160_REG_CTRL_MODE_RGB BIT(2) /* 0: ALS & IR, 1: RGB & IR */
> +#define APDS9160_REG_CTRL_EN_ALS BIT(1) /* 1: ALS active */
> +#define APDS9160_REG_CTLR_EN_PS BIT(0) /* 1: PS active */
> +
> +/** Status register  */
> +#define APDS9160_REG_SR_LS_INT BIT(3)
> +#define APDS9160_REG_SR_LS_NEW_DATA BIT(2)
> +#define APDS9160_REG_SR_PS_INT BIT(1)
> +#define APDS9160_REG_SR_PS_NEW_DATA BIT(0)
> +
> +/* Interrupt configuration register */
> +#define APDS9160_REG_INT_CFG 0x19
> +#define APDS9160_REG_INT_CFG_EN_LS BIT(2) /* LS int enable */
> +#define APDS9160_REG_INT_CFG_EN_PS BIT(0) /* PS int enable */
> +#define APDS9160_REG_INT_PST 0x1A
> +
> +/* Proximity registers */
> +#define APDS9160_REG_PS_LED 0x01
> +#define APDS9160_REG_PS_PULSES 0x02
> +#define APDS9160_REG_PS_MEAS_RATE 0x03
> +#define APDS9160_REG_PS_THRES_HI_LSB 0x1B
> +#define APDS9160_REG_PS_THRES_HI_MSB 0x1C
> +#define APDS9160_REG_PS_THRES_LO_LSB 0x1D
> +#define APDS9160_REG_PS_THRES_LO_MSB 0x1E
> +#define APDS9160_REG_PS_DATA_LSB 0x08
> +#define APDS9160_REG_PS_DATA_MSB 0x09
> +#define APDS9160_REG_PS_CAN_LEVEL_DIG_LSB 0x1F
> +#define APDS9160_REG_PS_CAN_LEVEL_DIG_MSB 0x20
> +#define APDS9160_REG_PS_CAN_LEVEL_ANA_DUR 0x21
> +#define APDS9160_REG_PS_CAN_LEVEL_ANA_CURRENT 0x22
> +
> +/* Light sensor registers */
> +#define APDS9160_REG_LS_MEAS_RATE 0x04
> +#define APDS9160_REG_LS_GAIN 0x05
> +#define APDS9160_REG_LS_DATA_CLEAR_LSB 0x0A
> +#define APDS9160_REG_LS_DATA_CLEAR 0x0B
> +#define APDS9160_REG_LS_DATA_CLEAR_MSB 0x0C
> +#define APDS9160_REG_LS_DATA_ALS_LSB 0x0D
> +#define APDS9160_REG_LS_DATA_ALS 0x0E
> +#define APDS9160_REG_LS_DATA_ALS_MSB 0x0F
> +#define APDS9160_REG_LS_THRES_UP_LSB 0x24
> +#define APDS9160_REG_LS_THRES_UP 0x25
> +#define APDS9160_REG_LS_THRES_UP_MSB 0x26
> +#define APDS9160_REG_LS_THRES_LO_LSB 0x27
> +#define APDS9160_REG_LS_THRES_LO 0x28
> +#define APDS9160_REG_LS_THRES_LO_MSB 0x29
> +#define APDS9160_REG_LS_THRES_VAR 0x2A
> +
> +/** Part identification number register */
> +#define APDS9160_REG_ID 0x06
> +
> +/** Status register */
> +#define APDS9160_REG_SR 0x07
> +#define APDS9160_REG_SR_DATA_ALS BIT(3)
> +#define APDS9160_REG_SR_DATA_PS BIT(0)
> +
> +/* Supported ID:s */
> +#define APDS9160_PART_ID_0 0x00
> +#define APDS9160_PART_ID_MASK 0xF0
> +#define APDS9160_PART_REV_MASK 0x0F
> +
> +#define APDS9160_PS_THRES_MAX 0x7FF
> +#define APDS9160_LS_THRES_MAX 0xFFFFF
> +#define APDS9160_CMD_LS_RESOLUTION_25MS 0x04
> +#define APDS9160_CMD_LS_RESOLUTION_50MS 0x03
> +#define APDS9160_CMD_LS_RESOLUTION_100MS 0x02
> +#define APDS9160_CMD_LS_RESOLUTION_200MS 0x01
> +#define APDS9160_PS_DATA_MASK 0x7FF
> +
> +#define APDS9160_DEFAULT_LS_GAIN 3
> +#define APDS9160_DEFAULT_LS_RATE 100
> +#define APDS9160_DEFAULT_PS_RATE 100
> +#define APDS9160_DEFAULT_PS_CANCELLATION_LEVEL 0
> +#define APDS9160_DEFAULT_PS_ANALOG_CANCELLATION 0
> +#define APDS9160_DEFAULT_PS_GAIN 1
> +#define APDS9160_DEFAULT_PS_CURRENT 100
> +#define APDS9160_DEFAULT_PS_RESOLUTION 0x03 // 11 bits
> +
> +// clang-format off
> +static const struct reg_default apds9160_reg_defaults[] = {
> +	{ APDS9160_REG_CTRL, 0x00 }, /* Sensors disabled by default  */
> +	{ APDS9160_REG_PS_LED, 0x33 }, /* 60 kHz frequency, 100 mA pulse current */
> +	{ APDS9160_REG_PS_PULSES, 0x08 }, /* 8 pulses */
> +	{ APDS9160_REG_PS_MEAS_RATE, 0x05 },
> +	{ APDS9160_REG_LS_MEAS_RATE, 0x22 },
> +	{ APDS9160_REG_LS_GAIN, 0x01 },
> +	{ APDS9160_REG_INT_CFG, 0x10 },
> +	{ APDS9160_REG_INT_PST, 0x00 },
> +	{ APDS9160_REG_PS_THRES_HI_LSB, 0xFF },
> +	{ APDS9160_REG_PS_THRES_HI_MSB, 0x07 },
> +	{ APDS9160_REG_PS_THRES_LO_LSB, 0x00 },
> +	{ APDS9160_REG_PS_THRES_LO_MSB, 0x00 },
> +	{ APDS9160_REG_PS_CAN_LEVEL_DIG_LSB, 0x00 },
> +	{ APDS9160_REG_PS_CAN_LEVEL_DIG_MSB, 0x00 },
> +	{ APDS9160_REG_PS_CAN_LEVEL_ANA_DUR, 0x00 },
> +	{ APDS9160_REG_PS_CAN_LEVEL_ANA_CURRENT, 0x00 },
> +	{ APDS9160_REG_LS_THRES_UP_LSB, 0xFF },
> +	{ APDS9160_REG_LS_THRES_UP, 0xFF },
> +	{ APDS9160_REG_LS_THRES_UP_MSB, 0x0F },
> +	{ APDS9160_REG_LS_THRES_LO_LSB, 0x00 },
> +	{ APDS9160_REG_LS_THRES_LO, 0x00 },
> +	{ APDS9160_REG_LS_THRES_LO_MSB, 0x00 },
> +	{ APDS9160_REG_LS_THRES_VAR, 0x00 },
> +};
> +// clang-format on
> +
> +static const struct regmap_range apds9160_readable_ranges[] = {
> +	regmap_reg_range(APDS9160_REG_CTRL, APDS9160_REG_LS_THRES_VAR),
> +};
> +
> +static const struct regmap_access_table apds9160_readable_table = {
> +	.yes_ranges = apds9160_readable_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(apds9160_readable_ranges),
> +};
> +
> +static const struct regmap_range apds9160_writeable_ranges[] = {
> +	regmap_reg_range(APDS9160_REG_CTRL, APDS9160_REG_LS_GAIN),
> +	regmap_reg_range(APDS9160_REG_INT_CFG, APDS9160_REG_LS_THRES_VAR),
> +};
> +
> +static const struct regmap_access_table apds9160_writeable_table = {
> +	.yes_ranges = apds9160_writeable_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(apds9160_writeable_ranges),
> +};
> +
> +static const struct regmap_range apds9160_volatile_ranges[] = {
> +	regmap_reg_range(APDS9160_REG_SR, APDS9160_REG_LS_DATA_ALS_MSB),
> +};
> +
> +static const struct regmap_access_table apds9160_volatile_table = {
> +	.yes_ranges = apds9160_volatile_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(apds9160_volatile_ranges),
> +};
> +
> +static const struct regmap_config apds9160_regmap_config = {
> +	.name = APDS9160_REGMAP_NAME,
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.use_single_read = true,
> +	.use_single_write = true,
> +
> +	.rd_table = &apds9160_readable_table,
> +	.wr_table = &apds9160_writeable_table,
> +	.volatile_table = &apds9160_volatile_table,
> +
> +	.reg_defaults = apds9160_reg_defaults,
> +	.num_reg_defaults = ARRAY_SIZE(apds9160_reg_defaults),
> +	.max_register = APDS9160_REG_CNT,
> +	.cache_type = REGCACHE_RBTREE,
> +};
> +
> +static const struct iio_event_spec apds9160_ps_event_spec[] = {
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE),
> +	},
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_FALLING,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE),
> +	},
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> +	},
> +};
> +
> +static const struct iio_event_spec apds9160_als_event_spec[] = {
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE),
> +	},
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_FALLING,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE),
> +	},
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> +	},
> +};
> +
> +static const struct iio_chan_spec apds9160_channels[] = {
> +	{
> +		/* Proximity sensor channel */
> +		.type = IIO_PROXIMITY,
> +		.address = APDS9160_REG_PS_DATA_LSB,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> +					    BIT(IIO_CHAN_INFO_CALIBSCALE) |
> +					    BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
> +					    BIT(IIO_CHAN_INFO_CALIBBIAS),
> +		.channel = 0,
> +		.indexed = 0,
> +		.scan_index = -1,
> +
> +		.event_spec = apds9160_ps_event_spec,
> +		.num_event_specs = ARRAY_SIZE(apds9160_ps_event_spec),
> +	},
> +	{
> +		/* Proximity sensor led current */
> +		.type = IIO_CURRENT,
> +		.output = 1,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.scan_index = -1,
> +	},
> +	{
> +		/* Clear channel */
> +		.type = IIO_INTENSITY,
> +		.address = APDS9160_REG_LS_DATA_CLEAR_LSB,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_INT_TIME) |
> +					    BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
> +					    BIT(IIO_CHAN_INFO_SCALE),
> +		.channel2 = IIO_MOD_LIGHT_CLEAR,
> +		.modified = 1,
> +		.scan_index = -1,
> +
> +		.event_spec = apds9160_als_event_spec,
> +		.num_event_specs = ARRAY_SIZE(apds9160_als_event_spec),
> +	},
> +	{
> +		/* Illuminance */
> +		.type = IIO_LIGHT,
> +		.address = APDS9160_REG_LS_DATA_ALS_LSB,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = -1,
> +	}
> +};
> +
> +/* Attributes */
> +// clang-format off
> +static const int apds9160_als_rate_map[][2] = {
> +	{ 25, 0x00 },
> +	{ 50, 0x01 },
> +	{ 100, 0x02 },
> +	{ 200, 0x03 }
> +};
> +
> +static const int apds9160_als_gain_map[][2] = {
> +	{ 1, 0x00 },
> +	{ 3, 0x01 },
> +	{ 6, 0x02 },
> +	{ 18, 0x03 },
> +	{ 64, 0x04 }
> +};
> +
> +static const int apds9160_ps_gain_map[][2] = {
> +	{ 1, 0x00 },
> +	{ 2, 0x01},
> +	{ 4, 0x02},
> +	{ 8, 0x03}
> +};
> +
> +static const int apds9160_ps_rate_map[][2] = {
> +	{ 25, 0x03 },
> +	{ 50, 0x04 },
> +	{ 100, 0x05 },
> +	{ 200, 0x06 },
> +	{ 400, 0x07 }
> +};
> +
> +static const int adps9160_ps_led_current_map[][2] = {
> +	{ 10, 0x00 },
> +	{ 25, 0x01 },
> +	{ 50, 0x02 },
> +	{ 100, 0x03 },
> +	{ 150, 0x04},
> +	{ 175, 0x05 },
> +	{ 200, 0x06 }
> +};
> +// clang-format on
> +
> +struct apds9160_scale {
> +	int itime;
> +	int gain;
> +	int scale1;
> +	int scale2;
> +};
> +
> +static const struct apds9160_scale apds9160_als_scale_map[] = {
> +	{
> +		.gain = 1,
> +		.itime = 25,
> +		.scale1 = 3272,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 1,
> +		.itime = 50,
> +		.scale1 = 1639,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 1,
> +		.itime = 100,
> +		.scale1 = 819,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 3,
> +		.itime = 25,
> +		.scale1 = 1077,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 3,
> +		.itime = 50,
> +		.scale1 = 538,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 3,
> +		.itime = 100,
> +		.scale1 = 269,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 6,
> +		.itime = 25,
> +		.scale1 = 525,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 6,
> +		.itime = 50,
> +		.scale1 = 263,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 6,
> +		.itime = 100,
> +		.scale1 = 131,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 18,
> +		.itime = 25,
> +		.scale1 = 169,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 18,
> +		.itime = 50,
> +		.scale1 = 84,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 18,
> +		.itime = 100,
> +		.scale1 = 42,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 64,
> +		.itime = 25,
> +		.scale1 = 49,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 64,
> +		.itime = 50,
> +		.scale1 = 25,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 64,
> +		.itime = 100,
> +		.scale1 = 12,
> +		.scale2 = 1000,
> +	},
> +};
> +
> +static IIO_CONST_ATTR(in_intensity_integration_time_available, "25 50 100 200");
> +static IIO_CONST_ATTR(in_proximity_sampling_frequency_available,
> +		      "25 50 100 200 400");
> +static IIO_CONST_ATTR(in_intensity_hardwaregain_available, "1 3 6 18 64");
> +static IIO_CONST_ATTR(in_proximity_hardwaregain_available, "1 2 4 8");
> +static IIO_CONST_ATTR(out_current_available, "10 25 50 100 150 175 200");
> +
> +static struct attribute *apds9160_attributes[] = {
> +	&iio_const_attr_in_intensity_integration_time_available.dev_attr.attr,
> +	&iio_const_attr_in_intensity_hardwaregain_available.dev_attr.attr,
> +	&iio_const_attr_in_proximity_sampling_frequency_available.dev_attr.attr,
> +	&iio_const_attr_in_proximity_hardwaregain_available.dev_attr.attr,
> +	&iio_const_attr_out_current_available.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group apds9160_attribute_group = {
> +	.attrs = apds9160_attributes,
> +};
> +
> +static const struct reg_field apds9160_reg_field_ls_en =
> +	REG_FIELD(APDS9160_REG_CTRL, 1, 1);
> +
> +static const struct reg_field apds9160_reg_field_ps_en =
> +	REG_FIELD(APDS9160_REG_CTRL, 0, 0);
> +
> +static const struct reg_field apds9160_reg_field_int_ps =
> +	REG_FIELD(APDS9160_REG_INT_CFG, 0, 0);
> +
> +static const struct reg_field apds9160_reg_field_int_als =
> +	REG_FIELD(APDS9160_REG_INT_CFG, 2, 2);
> +
> +static const struct reg_field apds9160_reg_field_ps_overflow =
> +	REG_FIELD(APDS9160_REG_PS_DATA_MSB, 3, 3);
> +
> +static const struct reg_field apds9160_reg_field_als_rate =
> +	REG_FIELD(APDS9160_REG_LS_MEAS_RATE, 0, 2);
> +
> +static const struct reg_field apds9160_reg_field_als_gain =
> +	REG_FIELD(APDS9160_REG_LS_GAIN, 0, 2);
> +
> +static const struct reg_field apds9160_reg_field_ps_rate =
> +	REG_FIELD(APDS9160_REG_PS_MEAS_RATE, 0, 2);
> +
> +static const struct reg_field apds9160_reg_field_als_res =
> +	REG_FIELD(APDS9160_REG_LS_MEAS_RATE, 4, 6);
> +
> +static const struct reg_field apds9160_reg_field_ps_current =
> +	REG_FIELD(APDS9160_REG_PS_LED, 0, 2);
> +
> +static const struct reg_field apds9160_reg_field_ps_gain =
> +	REG_FIELD(APDS9160_REG_PS_MEAS_RATE, 6, 7);
> +
> +static const struct reg_field apds9160_reg_field_ps_resolution =
> +	REG_FIELD(APDS9160_REG_PS_MEAS_RATE, 3, 4);
> +
> +struct apds9160_chip {
> +	struct i2c_client *client;
> +	struct iio_dev *indio_dev;
> +	struct regmap *regmap;
> +	struct mutex lock; /* avoid parallel access */

That's terrible comment. It is obvious what mutex is doing. We all know.

Instead, say exactly which parts are protected. I see you are protecting
regmap which is obviously wrong - regmap handles this...

> +
> +	struct regmap_field *reg_enable_ps;
> +	struct regmap_field *reg_enable_als;
> +	struct regmap_field *reg_int_ps;
> +	struct regmap_field *reg_int_als;
> +	struct regmap_field *reg_ps_overflow;
> +	struct regmap_field *reg_als_rate;
> +	struct regmap_field *reg_als_resolution;
> +	struct regmap_field *reg_ps_rate;
> +	struct regmap_field *reg_als_gain;
> +	struct regmap_field *reg_ps_current;
> +	struct regmap_field *reg_ps_gain;
> +	struct regmap_field *reg_ps_resolution;
> +
> +	/* State data */
> +	u8 revision;

Drop, not used.

Drop all such unused members.

> +	int als_int;
> +	int ps_int;
> +
> +	/* Configuration values */
> +	int als_itime;
> +	int als_hwgain;
> +	int als_scale1;
> +	int als_scale2;
> +	int ps_rate;
> +	int ps_cancellation_level;
> +	int ps_cancellation_analog;
> +	int ps_current;
> +	int ps_gain;
> +};
> +
> +static const struct i2c_device_id apds9160_id[] = { { APDS9160_DRIVER_NAME, 0 },
> +						    {} };
> +
> +/** Called when mutex is locked */
> +static void apds9160_set_scale(struct apds9160_chip *data)
> +{
> +	for (int idx = 0; idx < ARRAY_SIZE(apds9160_als_scale_map); idx++) {
> +		if (data->als_hwgain == apds9160_als_scale_map[idx].gain &&
> +		    data->als_itime == apds9160_als_scale_map[idx].itime) {
> +			data->als_scale1 = apds9160_als_scale_map[idx].scale1;
> +			data->als_scale2 = apds9160_als_scale_map[idx].scale2;
> +		}
> +	}
> +}
> +
> +static int apds9160_set_ps_rate(struct apds9160_chip *data, int val)
> +{
> +	int ret = -EINVAL;
> +	int idx;
> +
> +	dev_dbg(&data->client->dev, "%s - set rate to %i\n", __func__, val);
> +	for (idx = 0; idx < ARRAY_SIZE(apds9160_ps_rate_map); idx++) {
> +		if (apds9160_ps_rate_map[idx][0] == val) {
> +			mutex_lock(&data->lock);
> +			ret = regmap_field_write(data->reg_ps_rate,
> +						 apds9160_ps_rate_map[idx][1]);
> +			if (!ret)
> +				data->ps_rate = val;
> +			mutex_unlock(&data->lock);
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static int apds9160_set_ps_gain(struct apds9160_chip *data, int val)
> +{
> +	int ret = -EINVAL;
> +	int idx;
> +
> +	dev_dbg(&data->client->dev, "%s - set gain to %i\n", __func__, val);
> +	for (idx = 0; idx < ARRAY_SIZE(apds9160_ps_gain_map); idx++) {
> +		if (apds9160_ps_gain_map[idx][0] == val) {
> +			mutex_lock(&data->lock);
> +			ret = regmap_field_write(data->reg_ps_gain,
> +						 apds9160_ps_gain_map[idx][1]);
> +			if (!ret)
> +				data->ps_gain = val;
> +			mutex_unlock(&data->lock);
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +/**
> + * The PS intelligent cancellation level register allows for an on-chip substraction
> + * of the ADC count caused by unwanted reflected light from PS ADC output.
> + */
> +static int apds9160_set_ps_cancellation_level(struct apds9160_chip *data,
> +					      int val)
> +{
> +	int ret = -EINVAL;
> +	__le16 buf;
> +
> +	dev_dbg(&data->client->dev, "%s - set cancellation level to %i\n",
> +		__func__, val);
> +	if (val < 0 || val > 0xFFFF)
> +		return ret;
> +
> +	mutex_lock(&data->lock);
> +	buf = cpu_to_le16(val);
> +	ret = regmap_bulk_write(data->regmap, APDS9160_REG_PS_CAN_LEVEL_DIG_LSB,
> +				&buf, 2);
> +	if (!ret)
> +		data->ps_cancellation_level = val;
> +	mutex_unlock(&data->lock);
> +	return ret;
> +}
> +
> +/**
> + * This parameter determines the cancellation pulse duration in each of the PWM pulse.
> + * The cancellation is applied during the integration phase of the PS measurement.
> + * Duration is programmed in half clock cycles
> + */
> +static int apds9160_set_ps_analog_cancellation(struct apds9160_chip *data,
> +					       int val)
> +{
> +	int ret = -EINVAL;
> +
> +	dev_dbg(&data->client->dev, "%s - set analog cancellation to %i\n",
> +		__func__, val);
> +	if (val < 0 || val > 0x3F)
> +		return ret;

No, return -EINVAL. This applies everywhere

> +
> +	mutex_lock(&data->lock);
> +	ret = regmap_write(data->regmap, APDS9160_REG_PS_CAN_LEVEL_ANA_DUR,
> +			   val);
> +	if (!ret)
> +		data->ps_cancellation_analog = val;
> +	mutex_unlock(&data->lock);

Missing blank line, always.

> +	return ret;
> +}
> +

All these functions look poor to me. First: dev_dbg on every entry, even
with some useful data, is sign this is just not ready yet. Second, it is
really unclear to me what mutex protects here. Jonathan probably will
clarify it better how mutex should be used in IIO drivers.


> +static int apds9160_configure(struct iio_dev *indio_dev)
> +{
> +	struct apds9160_chip *chip = iio_priv(indio_dev);
> +
> +	return apds9160_chip_init(chip);
> +}
> +
> +static int apds9160_regfield_init(struct apds9160_chip *data)
> +{
> +	struct device *dev = &data->client->dev;
> +	struct regmap *regmap = data->regmap;
> +
> +	data->reg_int_als = devm_regmap_field_alloc(dev, regmap,
> +						    apds9160_reg_field_int_als);
> +	if (IS_ERR(data->reg_int_als)) {
> +		dev_err(dev, "INT ALS reg field init failed\n");

Hm? Is this allocation error? ENOMEM must not have dev_err.

> +		return PTR_ERR(data->reg_int_als);
> +	}
> +
> +	data->reg_int_ps =
> +		devm_regmap_field_alloc(dev, regmap, apds9160_reg_field_int_ps);

Fix wrapping everywhere. See coding style.

> +	if (IS_ERR(data->reg_int_ps)) {
> +		dev_err(dev, "INT ps reg field init failed\n");
> +		return PTR_ERR(data->reg_int_ps);
> +	}
> +
> +	data->reg_enable_als =
> +		devm_regmap_field_alloc(dev, regmap, apds9160_reg_field_ls_en);
> +	if (IS_ERR(data->reg_enable_als)) {
> +		dev_err(dev, "Enable ALS reg field init failed\n");
> +		return PTR_ERR(data->reg_enable_als);
> +	}
> +
> +	data->reg_enable_ps =
> +		devm_regmap_field_alloc(dev, regmap, apds9160_reg_field_ps_en);
> +	if (IS_ERR(data->reg_enable_ps)) {
> +		dev_err(dev, "Enable PS reg field init failed\n");
> +		return PTR_ERR(data->reg_enable_ps);
> +	}
> +
> +	data->reg_ps_overflow = devm_regmap_field_alloc(
> +		dev, regmap, apds9160_reg_field_ps_overflow);
> +	if (IS_ERR(data->reg_ps_overflow)) {
> +		dev_err(dev, "PS overflow reg field init failed\n");
> +		return PTR_ERR(data->reg_ps_overflow);
> +	}
> +
> +	data->reg_als_rate = devm_regmap_field_alloc(
> +		dev, regmap, apds9160_reg_field_als_rate);
> +	if (IS_ERR(data->reg_als_rate)) {
> +		dev_err(dev, "ALS measurement rate field init failed\n");
> +		return PTR_ERR(data->reg_als_rate);
> +	}
> +
> +	data->reg_als_resolution = devm_regmap_field_alloc(
> +		dev, regmap, apds9160_reg_field_als_res);
> +	if (IS_ERR(data->reg_als_resolution)) {
> +		dev_err(dev, "ALS resolution field init failed\n");
> +		return PTR_ERR(data->reg_als_resolution);
> +	}
> +
> +	data->reg_ps_rate = devm_regmap_field_alloc(dev, regmap,
> +						    apds9160_reg_field_ps_rate);
> +	if (IS_ERR(data->reg_ps_rate)) {
> +		dev_err(dev, "PS measurement rate field init failed\n");
> +		return PTR_ERR(data->reg_ps_rate);
> +	}
> +
> +	data->reg_als_gain = devm_regmap_field_alloc(
> +		dev, regmap, apds9160_reg_field_als_gain);
> +	if (IS_ERR(data->reg_als_gain)) {
> +		dev_err(dev, "ALS gain field init failed\n");
> +		return PTR_ERR(data->reg_als_gain);
> +	}
> +
> +	data->reg_ps_current = devm_regmap_field_alloc(
> +		dev, regmap, apds9160_reg_field_ps_current);
> +	if (IS_ERR(data->reg_ps_current)) {
> +		dev_err(dev, "PS current field init failed\n");
> +		return PTR_ERR(data->reg_ps_current);
> +	}
> +
> +	data->reg_ps_gain = devm_regmap_field_alloc(dev, regmap,
> +						    apds9160_reg_field_ps_gain);
> +	if (IS_ERR(data->reg_ps_gain)) {
> +		dev_err(dev, "PS gain field init failed\n");
> +		return PTR_ERR(data->reg_ps_gain);
> +	}
> +
> +	data->reg_ps_resolution = devm_regmap_field_alloc(
> +		dev, regmap, apds9160_reg_field_ps_resolution);
> +	if (IS_ERR(data->reg_ps_resolution)) {
> +		dev_err(dev, "PS resolution field init failed\n");
> +		return PTR_ERR(data->reg_ps_resolution);
> +	}
> +
> +	return 0;
> +}
> +
> +static int apds9160_disable(struct apds9160_chip *data)
> +{
> +	int ret;
> +
> +	ret = regmap_field_write(data->reg_enable_als, 0);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_field_write(data->reg_enable_ps, 0);
> +}
> +
> +static int apds9160_shutdown(struct iio_dev *indio_dev)
> +{
> +	struct apds9160_chip *data = iio_priv(indio_dev);
> +
> +	return apds9160_disable(data);
> +}
> +
> +static void apds9160_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
> +	struct apds9160_chip *data = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);

Why not devm?

> +	apds9160_disable(data);

Remove() is always, always placed immediately after probe.

This and one more hint suggest you took some poor downstream source as
template. That is not how you are supposed to work. Take newest,
revewied driver as template instead.


> +}
> +
> +static const struct iio_buffer_setup_ops apds9160_buffer_setup_ops = {
> +	.postenable = apds9160_configure,
> +	.predisable = apds9160_shutdown,
> +};
> +
> +static const struct iio_info apds9160_info = {
> +	.attrs = &apds9160_attribute_group,
> +	.read_raw = apds9160_read_raw,
> +	.write_raw = apds9160_write_raw,
> +	.read_event_value = apds9160_read_event,
> +	.write_event_value = apds9160_write_event,
> +	.read_event_config = apds9160_read_event_config,
> +	.write_event_config = apds9160_write_event_config,
> +};
> +
> +static int apds9160_probe(struct i2c_client *client)
> +{
> +	struct apds9160_chip *chip;
> +	struct iio_dev *indio_dev;
> +	int err;
> +
> +	dev_info(&client->dev,
> +		 "Loading proximity/ambient light sensor driver\n");

Drop, driver should be silent on success.


> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> +	indio_dev->info = &apds9160_info;
> +	indio_dev->name = APDS9160_DRIVER_NAME;
> +	indio_dev->channels = apds9160_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(apds9160_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	err = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
> +					  &apds9160_buffer_setup_ops);
> +	if (err)
> +		return err;
> +
> +	chip = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +	chip->client = client;
> +	chip->regmap = devm_regmap_init_i2c(client, &apds9160_regmap_config);
> +	if (IS_ERR(chip->regmap)) {
> +		dev_err(&client->dev, "regmap initialization failed.\n");

return dev_err_probe

> +		return PTR_ERR(chip->regmap);
> +	}
> +
> +	chip->client = client;
> +	chip->indio_dev = indio_dev;
> +	mutex_init(&chip->lock);
> +
> +	err = apds9160_detect(chip);
> +	if (err < 0) {
> +		dev_err(&client->dev, "apds9160 not found\n");
> +		return err;
> +	}
> +
> +	err = apds9160_regfield_init(chip);
> +	if (err)
> +		return err;
> +
> +	err = apds9160_chip_init(chip);
> +	if (err)
> +		return err;
> +
> +	if (client->irq > 0) {
> +		err = devm_request_threaded_irq(
> +			&client->dev, client->irq, NULL, apds9160_irq_handler,

Odd wrapping.

> +			IRQF_TRIGGER_FALLING | IRQF_ONESHOT, "apds9160_event",
> +			indio_dev);
> +		if (err) {
> +			dev_err(&client->dev, "request irq (%d) failed\n",
> +				client->irq);

dev_err_probe

> +			goto fail;
> +		}
> +	} else {
> +		dev_info(&client->dev,
> +			 "init: no IRQ defined, ps/als interrupts disabled\n");

dev_dbg, unless this is important, but if user decided to skip
interrupts, why would you ping them all the time? It's concious choice.
Hardware design. You cannot change hardware once it is released...

> +	}
> +
> +	err = iio_device_register(indio_dev);
> +	if (err)
> +		goto fail;
> +
> +	return 0;

Missing blank line

> +fail:
> +	apds9160_disable(chip);
> +	return err;
> +}
> +
> +static const struct of_device_id apds9160_of_match[] = {
> +	{ .compatible = "avago,apds9160" },
> +	{ .compatible = "broadmobi,apds9160" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, apds9160_of_match);
> +
> +static struct i2c_driver apds9160_driver = {
> +	.driver	  = {
> +		.name	= APDS9160_DRIVER_NAME,
> +		.owner = THIS_MODULE,

Uh, this was dropped like 10 years ago... Please use recent driver as
your template. Otherwise you repeat all the issues we fixed.


> +		.of_match_table = apds9160_of_match,
> +	},
> +	.probe    = apds9160_probe,
> +	.remove	  = apds9160_remove,
> +	.id_table = apds9160_id,
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, apds9160_id);

This ALWAYS is next to the table.

> +module_i2c_driver(apds9160_driver);
> +MODULE_DESCRIPTION("APDS9160 combined ALS and proximity sensor");
> +MODULE_AUTHOR("Mikael Gonella-Bolduc <m.gonella.bolduc@gmail.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION("1.0");

Drop

> 


Best regards,
Krzysztof

Re: [PATCH 2/2] iio: light: Add APDS9160 ALS & Proximity sensor driver

[Javier Carrasco]

Hi Mikael,

a few comments inline to add to what Krzysztof pointed out.

On 19/11/2024 21:36, Mikael Gonella-Bolduc via B4 Relay wrote:
> From: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
> 
> APDS9160 is a combination of ALS and proximity sensors.
> 
> This patch add supports for:
>     - Intensity clear data and illuminance data
>     - Proximity data
>     - Gain control, rate control
>     - Event thresholds
> 
> Signed-off-by: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
> ---
>  MAINTAINERS                  |    7 +
>  drivers/iio/light/Kconfig    |   13 +
>  drivers/iio/light/Makefile   |    1 +
>  drivers/iio/light/apds9160.c | 1420 ++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 1441 insertions(+)
> 

...

> +config APDS9160
> +	tristate "APDS9160 combined als and proximity sensors"
> +	select REGMAP_I2C
> +	select IIO_BUFFER
> +	select IIO_KFIFO_BUF
> +	depends on I2C
> +	help
> +	   Say Y here if you want to build a driver for Broadcom APDS9160
> +	   combined ambient light and proximity sensor chip.
> +

You can drop that "If unsure, say N here." as it is not common for such
drivers in IIO. There are a couple of entries in the whole subsystem
(not in this Kconfig, though) with this sentence, and some of them could
be dropped too.

> +	   To compile this driver as a module, choose M here: the
> +	   module will be called apds9160. If unsure, say N here.
> +
>  config APDS9300
>  	tristate "APDS9300 ambient light sensor"
>  	depends on I2C

...


> +
> +static int apds9160_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan, int val,
> +			      int val2, long mask)
> +{
> +	struct apds9160_chip *data = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_INT_TIME:
> +		if (val2 != 0)
> +			return -EINVAL;
> +		switch (chan->type) {
> +		case IIO_INTENSITY:
> +			return apds9160_set_als_int_time(data, val);
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (val2 != 0)
> +			return -EINVAL;
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			return apds9160_set_ps_rate(data, val);
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_HARDWAREGAIN:
> +		if (val2 != 0)
> +			return -EINVAL;
> +		switch (chan->type) {
> +		case IIO_INTENSITY:
> +			return apds9160_set_als_gain(data, val);
> +		case IIO_PROXIMITY:
> +			return apds9160_set_ps_gain(data, val);
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		if (val2 != 0)
> +			return -EINVAL;
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			return apds9160_set_ps_cancellation_level(data, val);
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		if (val2 != 0)
> +			return -EINVAL;
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			return apds9160_set_ps_analog_cancellation(data, val);
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_RAW:
> +		if (val2 != 0)
> +			return -EINVAL;
> +		switch (chan->type) {
> +		case IIO_CURRENT:
> +			return apds9160_set_ps_current(data, val);
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +

If you only have a switch with a return in every path, this return 0
can't be reached.

> +	return 0;
> +}
> +
> +static inline int apds9160_get_thres_reg(const struct iio_chan_spec *chan,
> +					 enum iio_event_direction dir, u8 *reg)
> +{
> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			*reg = APDS9160_REG_PS_THRES_HI_LSB;
> +			break;
> +		case IIO_INTENSITY:
> +			*reg = APDS9160_REG_LS_THRES_UP_LSB;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	case IIO_EV_DIR_FALLING:
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			*reg = APDS9160_REG_PS_THRES_LO_LSB;
> +			break;
> +		case IIO_INTENSITY:
> +			*reg = APDS9160_REG_LS_THRES_LO_LSB;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int apds9160_read_event(struct iio_dev *indio_dev,
> +			       const struct iio_chan_spec *chan,
> +			       enum iio_event_type type,
> +			       enum iio_event_direction dir,
> +			       enum iio_event_info info, int *val, int *val2)
> +{
> +	u8 reg;
> +
> +	int ret = 0;
> +	struct apds9160_chip *data = iio_priv(indio_dev);
> +
> +	if (info != IIO_EV_INFO_VALUE)
> +		return -EINVAL;
> +
> +	ret = apds9160_get_thres_reg(chan, dir, &reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (chan->type == IIO_PROXIMITY) {
> +		__le16 buf;
> +
> +		ret = regmap_bulk_read(data->regmap, reg, &buf, 2);
> +		if (ret < 0)
> +			return ret;
> +		*val = le16_to_cpu(buf);
> +	} else if (chan->type == IIO_INTENSITY) {
> +		__le32 buf = 0;
> +
> +		ret = regmap_bulk_read(data->regmap, reg, &buf, 3);
> +		if (ret < 0)
> +			return ret;
> +		*val = le32_to_cpu(buf);

Missing braces for that else (use them in all arms if you need them in one).

> +	} else
> +		return -EINVAL;
> +
> +	*val2 = 0;
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int apds9160_write_event(struct iio_dev *indio_dev,
> +				const struct iio_chan_spec *chan,
> +				enum iio_event_type type,
> +				enum iio_event_direction dir,
> +				enum iio_event_info info, int val, int val2)
> +{
> +	u8 reg;
> +	int ret = 0;
> +	struct apds9160_chip *data = iio_priv(indio_dev);
> +
> +	if (info != IIO_EV_INFO_VALUE)
> +		return -EINVAL;
> +
> +	ret = apds9160_get_thres_reg(chan, dir, &reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (chan->type == IIO_PROXIMITY) {
> +		if (val < 0 || val > APDS9160_PS_THRES_MAX)
> +			return -EINVAL;
> +		__le16 buf;
> +
> +		buf = cpu_to_le16(val);
> +		ret = regmap_bulk_write(data->regmap, reg, &buf, 2);
> +		if (ret < 0)
> +			return ret;
> +	} else if (chan->type == IIO_INTENSITY) {
> +		if (val < 0 || val > APDS9160_LS_THRES_MAX)
> +			return -EINVAL;
> +		__le32 buf = 0;
> +
> +		buf = cpu_to_le32(val);
> +		ret = regmap_bulk_write(data->regmap, reg, &buf, 3);
> +		if (ret < 0)
> +		

Same here.

	return ret;
> +	} else
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int apds9160_read_event_config(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan,
> +				      enum iio_event_type type,
> +				      enum iio_event_direction dir)
> +{
> +	struct apds9160_chip *data = iio_priv(indio_dev);
> +
> +	switch (chan->type) {
> +	case IIO_PROXIMITY:
> +		return data->ps_int;
> +	case IIO_INTENSITY:
> +		return data->als_int;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +

The 'state' argument is now a bool. To avoid issues, please rebase to
newer branches like linux-next, iio/testing iio/togreg. Otherwise it
will not compile with that modification. data->ps_int should then become
a bool too.

> +static int apds9160_write_event_config(struct iio_dev *indio_dev,
> +				       const struct iio_chan_spec *chan,
> +				       enum iio_event_type type,
> +				       enum iio_event_direction dir, int state)
> +{
> +	struct apds9160_chip *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	state = !!state;
> +
> +	switch (chan->type) {
> +	case IIO_PROXIMITY:
> +		if (data->ps_int == state)
> +			return -EINVAL;
> +
> +		ret = regmap_field_write(data->reg_int_ps, state);
> +		if (ret)
> +			return ret;
> +		data->ps_int = state;
> +		break;
> +	case IIO_INTENSITY:
> +		if (data->als_int == state)
> +			return -EINVAL;
> +
> +		ret = regmap_field_write(data->reg_int_als, state);
> +		if (ret)
> +			return ret;
> +		data->als_int = state;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}


Best regards,
Javier Carrasco

Re: [PATCH 2/2] iio: light: Add APDS9160 ALS & Proximity sensor driver

[kernel test robot]

Hi Mikael,

kernel test robot noticed the following build warnings:

[auto build test WARNING on adc218676eef25575469234709c2d87185ca223a]

url:    https://github.com/intel-lab-lkp/linux/commits/Mikael-Gonella-Bolduc-via-B4-Relay/dt-bindings-iio-light-Add-APDS9160-binding/20241121-123509
base:   adc218676eef25575469234709c2d87185ca223a
patch link:    https://lore.kernel.org/r/20241119-apds9160-driver-v1-2-fa00675b4ea4%40dimonoff.com
patch subject: [PATCH 2/2] iio: light: Add APDS9160 ALS & Proximity sensor driver
config: loongarch-allyesconfig (https://download.01.org/0day-ci/archive/20241122/202411222244.oAiveIHV-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241122/202411222244.oAiveIHV-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411222244.oAiveIHV-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/iio/light/apds9160.c:3: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
    * This file is part of the APDS9160 sensor driver.
>> drivers/iio/light/apds9160.c:565: warning: Function parameter or struct member 'data' not described in 'apds9160_set_ps_cancellation_level'
>> drivers/iio/light/apds9160.c:565: warning: Function parameter or struct member 'val' not described in 'apds9160_set_ps_cancellation_level'
>> drivers/iio/light/apds9160.c:565: warning: expecting prototype for The PS intelligent cancellation level register allows for an on(). Prototype was for apds9160_set_ps_cancellation_level() instead
   drivers/iio/light/apds9160.c:585: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
    * This parameter determines the cancellation pulse duration in each of the PWM pulse.
   drivers/iio/light/apds9160.c:609: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
    * Set the proximity sensor led current


vim +3 drivers/iio/light/apds9160.c

   > 3	 * This file is part of the APDS9160 sensor driver.
     4	 * Chip is combined proximity and ambient light sensor.
     5	 * Author: Mikael Gonella-Bolduc <m.gonella.bolduc@gmail.com>
     6	 */
     7	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/2] iio: light: Add APDS9160 ALS & Proximity sensor driver

[Jonathan Cameron]

On Tue, 19 Nov 2024 15:36:57 -0500
Mikael Gonella-Bolduc via B4 Relay <devnull+mgonellabolduc.dimonoff.com@kernel.org> wrote:

> From: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
> 
> APDS9160 is a combination of ALS and proximity sensors.
> 
> This patch add supports for:
>     - Intensity clear data and illuminance data
>     - Proximity data
>     - Gain control, rate control
>     - Event thresholds
> 
> Signed-off-by: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
Hi Mikael,

I may well repeat things you have already from the other reviewers!

Various comments inline.  Big one is that you register a buffer
but no channels to be captured and never push any data to it.
That looks like a workaround for something else?

Jonathan



> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index f2f3e414849ab12a7c0ea2b08e9a3310eb18ebb7..69a59c6759acea89241ab76bfcdfafe3e5824066 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -63,6 +63,19 @@ config AL3320A
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called al3320a.
>  
> +config APDS9160
> +	tristate "APDS9160 combined als and proximity sensors"
> +	select REGMAP_I2C
> +	select IIO_BUFFER
> +	select IIO_KFIFO_BUF
Normally I'd expect a light sensor to be providing a triggered buffer
rather than using the kfifo path directly.

Also you never push anything into the buffer.

Normally we only skip the trigger (which allows other signals to be used
to start a scan of the channels, or this trigger to capture on other sensors)
if there is a hardware buffer involved so we can't identify each individual
scan of the channels.

As it turns out you aren't doing buffered capture at all.

> +	depends on I2C
> +	help
> +	   Say Y here if you want to build a driver for Broadcom APDS9160
> +	   combined ambient light and proximity sensor chip.
> +
> +	   To compile this driver as a module, choose M here: the
> +	   module will be called apds9160. If unsure, say N here.
> +

> diff --git a/drivers/iio/light/apds9160.c b/drivers/iio/light/apds9160.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..cb855f20725dba9fea1390a955889d905fd7eb4f
> --- /dev/null
> +++ b/drivers/iio/light/apds9160.c
> @@ -0,0 +1,1420 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * This file is part of the APDS9160 sensor driver.
> + * Chip is combined proximity and ambient light sensor.
> + * Author: Mikael Gonella-Bolduc <m.gonella.bolduc@gmail.com>
> + */
> +
> +#include <linux/acpi.h>
Why?
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/mutex.h>
> +#include <linux/err.h>
> +#include <linux/irq.h>
> +#include <linux/i2c.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/kfifo_buf.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define APDS9160_DRIVER_NAME "apds9160"
> +#define APDS9160_REGMAP_NAME "apds9160_regmap"
> +#define APDS9160_STARTUP_DELAY 25000 /* us */
> +#define APDS9160_REG_CNT 37
> +
> +/** Main control register */
> +#define APDS9160_REG_CTRL 0x00
> +#define APDS9160_REG_CTRL_SWRESET BIT(4) /* 1: Activate reset */
> +#define APDS9160_REG_CTRL_MODE_RGB BIT(2) /* 0: ALS & IR, 1: RGB & IR */
> +#define APDS9160_REG_CTRL_EN_ALS BIT(1) /* 1: ALS active */
> +#define APDS9160_REG_CTLR_EN_PS BIT(0) /* 1: PS active */
> +
> +/** Status register  */
Run kernel-doc over the file and fix everything. This is not
kernel-doc so should not have /**

> +#define APDS9160_REG_SR_LS_INT BIT(3)
> +#define APDS9160_REG_SR_LS_NEW_DATA BIT(2)
> +#define APDS9160_REG_SR_PS_INT BIT(1)
> +#define APDS9160_REG_SR_PS_NEW_DATA BIT(0)
> +
> +/* Interrupt configuration register */
> +#define APDS9160_REG_INT_CFG 0x19
> +#define APDS9160_REG_INT_CFG_EN_LS BIT(2) /* LS int enable */
> +#define APDS9160_REG_INT_CFG_EN_PS BIT(0) /* PS int enable */
> +#define APDS9160_REG_INT_PST 0x1A
> +
> +/* Proximity registers */
> +#define APDS9160_REG_PS_LED 0x01
> +#define APDS9160_REG_PS_PULSES 0x02
> +#define APDS9160_REG_PS_MEAS_RATE 0x03
> +#define APDS9160_REG_PS_THRES_HI_LSB 0x1B
> +#define APDS9160_REG_PS_THRES_HI_MSB 0x1C
> +#define APDS9160_REG_PS_THRES_LO_LSB 0x1D
> +#define APDS9160_REG_PS_THRES_LO_MSB 0x1E
> +#define APDS9160_REG_PS_DATA_LSB 0x08
> +#define APDS9160_REG_PS_DATA_MSB 0x09
> +#define APDS9160_REG_PS_CAN_LEVEL_DIG_LSB 0x1F
> +#define APDS9160_REG_PS_CAN_LEVEL_DIG_MSB 0x20
> +#define APDS9160_REG_PS_CAN_LEVEL_ANA_DUR 0x21
> +#define APDS9160_REG_PS_CAN_LEVEL_ANA_CURRENT 0x22
> +
> +/* Light sensor registers */
> +#define APDS9160_REG_LS_MEAS_RATE 0x04
> +#define APDS9160_REG_LS_GAIN 0x05
> +#define APDS9160_REG_LS_DATA_CLEAR_LSB 0x0A
> +#define APDS9160_REG_LS_DATA_CLEAR 0x0B
> +#define APDS9160_REG_LS_DATA_CLEAR_MSB 0x0C
> +#define APDS9160_REG_LS_DATA_ALS_LSB 0x0D
> +#define APDS9160_REG_LS_DATA_ALS 0x0E
> +#define APDS9160_REG_LS_DATA_ALS_MSB 0x0F
> +#define APDS9160_REG_LS_THRES_UP_LSB 0x24
> +#define APDS9160_REG_LS_THRES_UP 0x25
> +#define APDS9160_REG_LS_THRES_UP_MSB 0x26
> +#define APDS9160_REG_LS_THRES_LO_LSB 0x27
> +#define APDS9160_REG_LS_THRES_LO 0x28
> +#define APDS9160_REG_LS_THRES_LO_MSB 0x29
> +#define APDS9160_REG_LS_THRES_VAR 0x2A
> +
> +/** Part identification number register */
> +#define APDS9160_REG_ID 0x06
> +
> +/** Status register */
> +#define APDS9160_REG_SR 0x07
> +#define APDS9160_REG_SR_DATA_ALS BIT(3)

One trick to make it obvious what is a field and what
a register is don't include the _REG bit in
the field defines.

> +#define APDS9160_REG_SR_DATA_PS BIT(0)
> +
> +/* Supported ID:s */
> +#define APDS9160_PART_ID_0 0x00
> +#define APDS9160_PART_ID_MASK 0xF0
> +#define APDS9160_PART_REV_MASK 0x0F
> +
> +#define APDS9160_PS_THRES_MAX 0x7FF
> +#define APDS9160_LS_THRES_MAX 0xFFFFF
> +#define APDS9160_CMD_LS_RESOLUTION_25MS 0x04
> +#define APDS9160_CMD_LS_RESOLUTION_50MS 0x03
> +#define APDS9160_CMD_LS_RESOLUTION_100MS 0x02
> +#define APDS9160_CMD_LS_RESOLUTION_200MS 0x01
> +#define APDS9160_PS_DATA_MASK 0x7FF
> +
> +#define APDS9160_DEFAULT_LS_GAIN 3
> +#define APDS9160_DEFAULT_LS_RATE 100
> +#define APDS9160_DEFAULT_PS_RATE 100
> +#define APDS9160_DEFAULT_PS_CANCELLATION_LEVEL 0
> +#define APDS9160_DEFAULT_PS_ANALOG_CANCELLATION 0
> +#define APDS9160_DEFAULT_PS_GAIN 1
> +#define APDS9160_DEFAULT_PS_CURRENT 100
> +#define APDS9160_DEFAULT_PS_RESOLUTION 0x03 // 11 bits
Name so that it is obvious it is the value for 11 bits.

> +
> +// clang-format off
> +static const struct reg_default apds9160_reg_defaults[] = {
you can explicitly tell regmap to write these to the device. That will save
a bunch of init code.

> +	{ APDS9160_REG_CTRL, 0x00 }, /* Sensors disabled by default  */
> +	{ APDS9160_REG_PS_LED, 0x33 }, /* 60 kHz frequency, 100 mA pulse current */
> +	{ APDS9160_REG_PS_PULSES, 0x08 }, /* 8 pulses */
> +	{ APDS9160_REG_PS_MEAS_RATE, 0x05 },
> +	{ APDS9160_REG_LS_MEAS_RATE, 0x22 },
> +	{ APDS9160_REG_LS_GAIN, 0x01 },
> +	{ APDS9160_REG_INT_CFG, 0x10 },
> +	{ APDS9160_REG_INT_PST, 0x00 },
> +	{ APDS9160_REG_PS_THRES_HI_LSB, 0xFF },
> +	{ APDS9160_REG_PS_THRES_HI_MSB, 0x07 },
> +	{ APDS9160_REG_PS_THRES_LO_LSB, 0x00 },
> +	{ APDS9160_REG_PS_THRES_LO_MSB, 0x00 },
> +	{ APDS9160_REG_PS_CAN_LEVEL_DIG_LSB, 0x00 },
> +	{ APDS9160_REG_PS_CAN_LEVEL_DIG_MSB, 0x00 },
> +	{ APDS9160_REG_PS_CAN_LEVEL_ANA_DUR, 0x00 },
> +	{ APDS9160_REG_PS_CAN_LEVEL_ANA_CURRENT, 0x00 },
> +	{ APDS9160_REG_LS_THRES_UP_LSB, 0xFF },
> +	{ APDS9160_REG_LS_THRES_UP, 0xFF },
> +	{ APDS9160_REG_LS_THRES_UP_MSB, 0x0F },
> +	{ APDS9160_REG_LS_THRES_LO_LSB, 0x00 },
> +	{ APDS9160_REG_LS_THRES_LO, 0x00 },
> +	{ APDS9160_REG_LS_THRES_LO_MSB, 0x00 },
> +	{ APDS9160_REG_LS_THRES_VAR, 0x00 },
> +};
> +// clang-format on

> +
> +static const struct iio_event_spec apds9160_ps_event_spec[] = {
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE),
Given they are the same, why not use same event_spec array for als and ps.

> +	},
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_FALLING,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE),
> +	},
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> +	},
> +};
> +
> +static const struct iio_event_spec apds9160_als_event_spec[] = {
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE),
> +	},
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_FALLING,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE),
> +	},
> +	{
Explicitly set DIR_EITHER here.  It's not a completely obvious default
> +		.type = IIO_EV_TYPE_THRESH,
> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> +	},
> +};
> +
> +static const struct iio_chan_spec apds9160_channels[] = {
> +	{
> +		/* Proximity sensor channel */
> +		.type = IIO_PROXIMITY,
> +		.address = APDS9160_REG_PS_DATA_LSB,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> +					    BIT(IIO_CHAN_INFO_CALIBSCALE) |
> +					    BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
> +					    BIT(IIO_CHAN_INFO_CALIBBIAS),
> +		.channel = 0,
> +		.indexed = 0,
> +		.scan_index = -1,
No need to set scan_index if you don't register a buffer (which you shouldn't
without more work and these having actual values).
> +
> +		.event_spec = apds9160_ps_event_spec,
> +		.num_event_specs = ARRAY_SIZE(apds9160_ps_event_spec),
> +	},
> +	{
> +		/* Proximity sensor led current */
> +		.type = IIO_CURRENT,
> +		.output = 1,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.scan_index = -1,
> +	},
> +	{
> +		/* Clear channel */
> +		.type = IIO_INTENSITY,
> +		.address = APDS9160_REG_LS_DATA_CLEAR_LSB,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_INT_TIME) |
> +					    BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
> +					    BIT(IIO_CHAN_INFO_SCALE),
> +		.channel2 = IIO_MOD_LIGHT_CLEAR,
> +		.modified = 1,
> +		.scan_index = -1,
> +
> +		.event_spec = apds9160_als_event_spec,
> +		.num_event_specs = ARRAY_SIZE(apds9160_als_event_spec),
> +	},
> +	{
> +		/* Illuminance */
> +		.type = IIO_LIGHT,
> +		.address = APDS9160_REG_LS_DATA_ALS_LSB,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = -1,
> +	}
> +};
> +
> +/* Attributes */
> +// clang-format off
> +static const int apds9160_als_rate_map[][2] = {
> +	{ 25, 0x00 },
> +	{ 50, 0x01 },
> +	{ 100, 0x02 },
> +	{ 200, 0x03 }
> +};
> +
> +static const int apds9160_als_gain_map[][2] = {
> +	{ 1, 0x00 },
> +	{ 3, 0x01 },
> +	{ 6, 0x02 },
> +	{ 18, 0x03 },
> +	{ 64, 0x04 }
> +};
> +
> +static const int apds9160_ps_gain_map[][2] = {
> +	{ 1, 0x00 },
> +	{ 2, 0x01},
> +	{ 4, 0x02},
> +	{ 8, 0x03}
Make spacing consistent throughout. Same as first
line here looks good.

> +};
> +
> +static const int apds9160_ps_rate_map[][2] = {
> +	{ 25, 0x03 },
> +	{ 50, 0x04 },
> +	{ 100, 0x05 },
> +	{ 200, 0x06 },
> +	{ 400, 0x07 }
> +};
> +
> +static const int adps9160_ps_led_current_map[][2] = {
> +	{ 10, 0x00 },
> +	{ 25, 0x01 },
> +	{ 50, 0x02 },
> +	{ 100, 0x03 },
> +	{ 150, 0x04},
> +	{ 175, 0x05 },
> +	{ 200, 0x06 }
Add trailing commas. In theory we might have more values to add
(though very unlikely).
> +};
> +// clang-format on
No to these Just ignore what clang-format tries to do.
It should be treated as hints, not rules for kernel code.
> +
> +struct apds9160_scale {
> +	int itime;
> +	int gain;
> +	int scale1;
> +	int scale2;
> +};
> +
> +static const struct apds9160_scale apds9160_als_scale_map[] = {
> +	{
> +		.gain = 1,
> +		.itime = 25,
> +		.scale1 = 3272,
> +		.scale2 = 1000,

No way to compute scale from gain and itime?  If not I'm not sure
if we can use the gts helpers.

> +	},
> +	{
> +		.gain = 1,
> +		.itime = 50,
> +		.scale1 = 1639,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 1,
> +		.itime = 100,
> +		.scale1 = 819,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 3,
> +		.itime = 25,
> +		.scale1 = 1077,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 3,
> +		.itime = 50,
> +		.scale1 = 538,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 3,
> +		.itime = 100,
> +		.scale1 = 269,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 6,
> +		.itime = 25,
> +		.scale1 = 525,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 6,
> +		.itime = 50,
> +		.scale1 = 263,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 6,
> +		.itime = 100,
> +		.scale1 = 131,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 18,
> +		.itime = 25,
> +		.scale1 = 169,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 18,
> +		.itime = 50,
> +		.scale1 = 84,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 18,
> +		.itime = 100,
> +		.scale1 = 42,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 64,
> +		.itime = 25,
> +		.scale1 = 49,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 64,
> +		.itime = 50,
> +		.scale1 = 25,
> +		.scale2 = 1000,
> +	},
> +	{
> +		.gain = 64,
> +		.itime = 100,
> +		.scale1 = 12,
> +		.scale2 = 1000,
> +	},
> +};
> +
> +static IIO_CONST_ATTR(in_intensity_integration_time_available, "25 50 100 200");
> +static IIO_CONST_ATTR(in_proximity_sampling_frequency_available,
> +		      "25 50 100 200 400");
> +static IIO_CONST_ATTR(in_intensity_hardwaregain_available, "1 3 6 18 64");
> +static IIO_CONST_ATTR(in_proximity_hardwaregain_available, "1 2 4 8");
> +static IIO_CONST_ATTR(out_current_available, "10 25 50 100 150 175 200");
Use read_avail and appropriate masks. 

Whilst we used to do it with explicit attributes for modern drivers we like
to make the data available to in kernel users and ensure there is only one
source of the values by using the same arrays for matching write values.

> +
> +static struct attribute *apds9160_attributes[] = {
> +	&iio_const_attr_in_intensity_integration_time_available.dev_attr.attr,
> +	&iio_const_attr_in_intensity_hardwaregain_available.dev_attr.attr,
> +	&iio_const_attr_in_proximity_sampling_frequency_available.dev_attr.attr,
> +	&iio_const_attr_in_proximity_hardwaregain_available.dev_attr.attr,
> +	&iio_const_attr_out_current_available.dev_attr.attr,
> +	NULL,
No comma on NULL terminators. However, no reason any of these should
need to be handled with custom attributes.

> +};
> +
> +
> +struct apds9160_chip {
> +	struct i2c_client *client;
> +	struct iio_dev *indio_dev;
That sets alarm bells off. If you ever need to go from
iio_priv() to struct iio_dev then you almost certainly need to rethink your
design.  Thankfully I think you only ever set this.  Drop it.

> +	struct regmap *regmap;
> +	struct mutex lock; /* avoid parallel access */
You already have feedback on this comment so I won't add anything.

> +
> +	struct regmap_field *reg_enable_ps;
> +	struct regmap_field *reg_enable_als;
> +	struct regmap_field *reg_int_ps;
> +	struct regmap_field *reg_int_als;
> +	struct regmap_field *reg_ps_overflow;
> +	struct regmap_field *reg_als_rate;
> +	struct regmap_field *reg_als_resolution;
> +	struct regmap_field *reg_ps_rate;
> +	struct regmap_field *reg_als_gain;
> +	struct regmap_field *reg_ps_current;
> +	struct regmap_field *reg_ps_gain;
> +	struct regmap_field *reg_ps_resolution;
> +
> +	/* State data */
> +	u8 revision;
> +	int als_int;
> +	int ps_int;
> +
> +	/* Configuration values */
> +	int als_itime;
These are all caches of register values. Maybe simpler to just enable the regmap
cache and read them back form the cached versions of the hardware registers.
> +	int als_hwgain;
> +	int als_scale1;
> +	int als_scale2;
> +	int ps_rate;
> +	int ps_cancellation_level;
> +	int ps_cancellation_analog;
> +	int ps_current;
> +	int ps_gain;
> +};
> +
> +static const struct i2c_device_id apds9160_id[] = { { APDS9160_DRIVER_NAME, 0 },
> +						    {} };
Move this down to just before where it is used.
Also, just hard code the string. It is much easier to grep for then as no reason
the name in the device id should match the driver name.

> +
> +/** Called when mutex is locked */
Not kernel doc so /* only.
> +static void apds9160_set_scale(struct apds9160_chip *data)
> +{
> +	for (int idx = 0; idx < ARRAY_SIZE(apds9160_als_scale_map); idx++) {
> +		if (data->als_hwgain == apds9160_als_scale_map[idx].gain &&
> +		    data->als_itime == apds9160_als_scale_map[idx].itime) {
> +			data->als_scale1 = apds9160_als_scale_map[idx].scale1;
> +			data->als_scale2 = apds9160_als_scale_map[idx].scale2;
> +		}
> +	}
> +}


> +/**
> + * This parameter determines the cancellation pulse duration in each of the PWM pulse.
> + * The cancellation is applied during the integration phase of the PS measurement.
> + * Duration is programmed in half clock cycles
This is unusual. and I don't really understand how it works.
> + */
> +static int apds9160_set_ps_analog_cancellation(struct apds9160_chip *data,
> +					       int val)
> +{
> +	int ret = -EINVAL;
overwritten in all paths where it is used.
> +
> +	dev_dbg(&data->client->dev, "%s - set analog cancellation to %i\n",
> +		__func__, val);
> +	if (val < 0 || val > 0x3F)
> +		return ret;
> +
> +	mutex_lock(&data->lock);
> +	ret = regmap_write(data->regmap, APDS9160_REG_PS_CAN_LEVEL_ANA_DUR,
> +			   val);
> +	if (!ret)
Always keep error paths out of line.  It just makes things more consistent
and hence easier to review.

The cleanup.h magic like guard() can help a lot with keeping code simple when
doing early returns.

> +		data->ps_cancellation_analog = val;
> +	mutex_unlock(&data->lock);
> +	return ret;
> +}
> +
> +/**
Not kernel doc so /* only.
> + * Set the proximity sensor led current
> + */
> +static int apds9160_set_ps_current(struct apds9160_chip *data, int val)
> +{
> +	int ret = -EINVAL;
> +	int idx;
> +
> +	dev_dbg(&data->client->dev, "%s - set current to %i\n", __func__, val);
> +	for (idx = 0; idx < ARRAY_SIZE(adps9160_ps_led_current_map); idx++) {
> +		if (adps9160_ps_led_current_map[idx][0] == val) {
> +			mutex_lock(&data->lock);
I'd suggest changes similar to what I said for the next function
> +			ret = regmap_field_write(
> +				data->reg_ps_current,
> +				adps9160_ps_led_current_map[idx][1]);
> +
> +			if (!ret)
> +				data->ps_current = val;
> +
> +			mutex_unlock(&data->lock);
> +			break;
> +		}
> +	}
> +	return ret;
> +}
> +
> +static int apds9160_set_als_gain(struct apds9160_chip *data, int val)
> +{
> +	int ret = -EINVAL;
> +	int idx;
> +
> +	dev_dbg(&data->client->dev, "%s - set als gain to %i\n", __func__, val);
> +	for (idx = 0; idx < ARRAY_SIZE(apds9160_als_gain_map); idx++) {
> +		if (apds9160_als_gain_map[idx][0] == val) {

Flip this to reduce indent and give more readable code.

		if (apds9160_als_gain_map[idx][0] != val)
			continue;


> +			mutex_lock(&data->lock);
guard(mutex)(&data-<lock).
> +			ret = regmap_field_write(data->reg_als_gain,
> +						 apds9160_als_gain_map[idx][1]);
			if (ret)
				return ret;
			data->als_hwgain = val...

> +			if (!ret) {
> +				data->als_hwgain = val;
> +				apds9160_set_scale(data);
> +			}
> +			mutex_unlock(&data->lock);
> +			break;
> +		}
> +	}
> +	return ret;
> +}
> +
> +static int apds9160_set_als_int_time(struct apds9160_chip *data, int val)
> +{
> +	int ret = -EINVAL;
> +	int idx;
> +
> +	dev_dbg(&data->client->dev, "%s - set int time to %i\n", __func__, val);
> +	for (idx = 0; idx < ARRAY_SIZE(apds9160_als_rate_map); idx++) {
> +		if (apds9160_als_rate_map[idx][0] == val) {
Flip logic and use a continue to reduce indent for what follows (see example below).

> +			mutex_lock(&data->lock);
guard(mutex)(&data->lock);

> +			ret = regmap_field_write(data->reg_als_rate,
> +						 apds9160_als_rate_map[idx][1]);
> +			if (!ret) {
			if (ret)
				return ret;

			data->als_itime = val;
			
				
> +				data->als_itime = val;
> +				/* Lower resolution for faster rates */
> +				switch (val) {
> +				case 25:
> +					ret = regmap_field_write(
> +						data->reg_als_resolution,
> +						APDS9160_CMD_LS_RESOLUTION_25MS);
> +					break;
> +				case 50:
> +					ret = regmap_field_write(
> +						data->reg_als_resolution,
> +						APDS9160_CMD_LS_RESOLUTION_50MS);
> +					break;
> +				case 200:
> +					ret = regmap_field_write(
> +						data->reg_als_resolution,
> +						APDS9160_CMD_LS_RESOLUTION_200MS);
> +					break;
> +				default:
> +					ret = regmap_field_write(
> +						data->reg_als_resolution,
> +						APDS9160_CMD_LS_RESOLUTION_100MS);
> +				}
> +				apds9160_set_scale(data);
> +			}
> +			mutex_unlock(&data->lock);
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static int apds9160_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, int *val,
> +			     int *val2, long mask)
> +{
> +	struct apds9160_chip *data = iio_priv(indio_dev);
> +	int ret = -EINVAL;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_PROXIMITY: {
> +			__le16 buf;
> +
> +			ret = regmap_bulk_read(data->regmap, chan->address,
> +					       &buf, 2);
Should be a dma safe buffer for any bulk accesses.
> +			if (!ret) {
Flip logic to return in error path.
			if (ret)
				return ret;

			*val = le16_to_cpu(buf);
			*val = FIELD_GET() for what you want, not masking what you don't.

> +				ret = IIO_VAL_INT;
> +				*val = le16_to_cpu(buf);
> +				// Remove data overflow and ambient light overflow bits from result

/* */

> +				*val &= APDS9160_PS_DATA_MASK;
> +			}
> +		} break;
> +		case IIO_LIGHT:
> +		case IIO_INTENSITY: {
> +			__le32 buf = 0;
> +
> +			ret = regmap_bulk_read(data->regmap, chan->address,
> +					       &buf, 3);
as below. Use a u8 [3]  and get_unaligned_le24()

> +			if (!ret) {
> +				ret = IIO_VAL_INT;
> +				*val = le32_to_cpu(buf);
> +			}
> +		} break;
> +		case IIO_CURRENT: {
{} not needed.
> +			ret = IIO_VAL_INT;
> +			*val = data->ps_current;
> +		} break;
> +		default:
> +			break;
> +		}
> +		break;
> +	case IIO_CHAN_INFO_HARDWAREGAIN:
As for the write, this is unlikely to be the right interface
but I'm not 100% sure what the various gain related controls
are affecting.  I haven't dog into the datasheet yet and
won't get time today.

> +		switch (chan->type) {
> +		case IIO_INTENSITY:
> +			ret = IIO_VAL_INT;
> +			*val = data->als_hwgain;
> +			break;
> +		case IIO_PROXIMITY:
> +			ret = IIO_VAL_INT;
> +			*val = data->ps_gain;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
> +	case IIO_CHAN_INFO_INT_TIME:
> +		switch (chan->type) {
> +		case IIO_INTENSITY:
> +			ret = IIO_VAL_INT;
> +			*val = data->als_itime;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			ret = IIO_VAL_INT;
> +			*val = data->ps_rate;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			ret = IIO_VAL_INT;
> +			*val = data->ps_cancellation_level;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			ret = IIO_VAL_INT;
> +			*val = data->ps_cancellation_analog;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_LIGHT:
> +		case IIO_INTENSITY:
> +			ret = IIO_VAL_FRACTIONAL;
> +			*val = data->als_scale1;
> +			*val2 = data->als_scale2;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return ret;
> +};
> +
> +static int apds9160_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan, int val,
> +			      int val2, long mask)
> +{
> +	struct apds9160_chip *data = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_INT_TIME:
> +		if (val2 != 0)
> +			return -EINVAL;
> +		switch (chan->type) {
> +		case IIO_INTENSITY:
> +			return apds9160_set_als_int_time(data, val);
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (val2 != 0)
> +			return -EINVAL;
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			return apds9160_set_ps_rate(data, val);
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_HARDWAREGAIN:

This is rarely used unless for time of flight sensors.  That is where
you are controlling the gain on something not directly related to the
signal you are reading. Here if it affects the read out value such that
_raw * _scale should include it then use _scale, otherwise should be
incorporated in calibscale. 

If it is _scale then the code will be fiddly but you need to present
the scales that are appropriate for the current integration time.

It may be worth looking at the GTS helpers that assist with cases
where there are complex controls interacting around gain of the
sensor.

> +		if (val2 != 0)
> +			return -EINVAL;
> +		switch (chan->type) {
> +		case IIO_INTENSITY:
> +			return apds9160_set_als_gain(data, val);
> +		case IIO_PROXIMITY:
> +			return apds9160_set_ps_gain(data, val);
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		if (val2 != 0)
> +			return -EINVAL;
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			return apds9160_set_ps_cancellation_level(data, val);
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		if (val2 != 0)
> +			return -EINVAL;
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			return apds9160_set_ps_analog_cancellation(data, val);
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_RAW:
> +		if (val2 != 0)
> +			return -EINVAL;
> +		switch (chan->type) {
> +		case IIO_CURRENT:
> +			return apds9160_set_ps_current(data, val);
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static inline int apds9160_get_thres_reg(const struct iio_chan_spec *chan,
> +					 enum iio_event_direction dir, u8 *reg)
> +{
> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			*reg = APDS9160_REG_PS_THRES_HI_LSB;
> +			break;
> +		case IIO_INTENSITY:
> +			*reg = APDS9160_REG_LS_THRES_UP_LSB;
> +			break;
return from these rather than breaking.
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
Then this won't be reachable.
> +	case IIO_EV_DIR_FALLING:
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			*reg = APDS9160_REG_PS_THRES_LO_LSB;
> +			break;
> +		case IIO_INTENSITY:
> +			*reg = APDS9160_REG_LS_THRES_LO_LSB;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
And neither will this.
> +
> +	return 0;
> +}
> +
> +static int apds9160_read_event(struct iio_dev *indio_dev,
> +			       const struct iio_chan_spec *chan,
> +			       enum iio_event_type type,
> +			       enum iio_event_direction dir,
> +			       enum iio_event_info info, int *val, int *val2)
> +{
> +	u8 reg;
> +
> +	int ret = 0;
> +	struct apds9160_chip *data = iio_priv(indio_dev);
> +
> +	if (info != IIO_EV_INFO_VALUE)
> +		return -EINVAL;
> +
> +	ret = apds9160_get_thres_reg(chan, dir, &reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (chan->type == IIO_PROXIMITY) {

Probably clearer as a switch.

> +		__le16 buf;
> +
> +		ret = regmap_bulk_read(data->regmap, reg, &buf, 2);
> +		if (ret < 0)
> +			return ret;
> +		*val = le16_to_cpu(buf);
> +	} else if (chan->type == IIO_INTENSITY) {
> +		__le32 buf = 0;
> +
> +		ret = regmap_bulk_read(data->regmap, reg, &buf, 3);

It's not an __le32 if you are reading only 3 bytes. Use a u8 [3]
and get_unaligned_le24()

> +		if (ret < 0)
> +			return ret;
> +		*val = le32_to_cpu(buf);
> +	} else
> +		return -EINVAL;
> +
> +	*val2 = 0;
No need to set if returning IIO_VAL_INT as won't be used.

> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int apds9160_write_event(struct iio_dev *indio_dev,
> +				const struct iio_chan_spec *chan,
> +				enum iio_event_type type,
> +				enum iio_event_direction dir,
> +				enum iio_event_info info, int val, int val2)
> +{
> +	u8 reg;
> +	int ret = 0;
> +	struct apds9160_chip *data = iio_priv(indio_dev);
> +
> +	if (info != IIO_EV_INFO_VALUE)
> +		return -EINVAL;
> +
> +	ret = apds9160_get_thres_reg(chan, dir, &reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (chan->type == IIO_PROXIMITY) {
> +		if (val < 0 || val > APDS9160_PS_THRES_MAX)
> +			return -EINVAL;
> +		__le16 buf;
> +
> +		buf = cpu_to_le16(val);
> +		ret = regmap_bulk_write(data->regmap, reg, &buf, 2);
Needs to be a dma safe buffer.  In practice this probably won't cause
a bug but that is down to subtle internals of regmap and there is
no guarantee that future optimizations won't again require a DMA safe
buffer.  This what the __aligned(IIO_DMA_MINALIGN) stuff is about.

> +		if (ret < 0)
> +			return ret;
> +	} else if (chan->type == IIO_INTENSITY) {
> +		if (val < 0 || val > APDS9160_LS_THRES_MAX)
> +			return -EINVAL;
> +		__le32 buf = 0;

Variables should almost always be declared at top of scope or just put
them at top of function if that is more appropraite.

> +
> +		buf = cpu_to_le32(val);
> +		ret = regmap_bulk_write(data->regmap, reg, &buf, 3);
> +		if (ret < 0)
> +			return ret;
		return regmap_bulk_write()

> +	} else
> +		return -EINVAL;
Probably neater to use a switch statement and default to cath this last one.
> +
> +	return 0;
> +}
> +
> +static int apds9160_read_event_config(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan,
> +				      enum iio_event_type type,
> +				      enum iio_event_direction dir)
> +{
> +	struct apds9160_chip *data = iio_priv(indio_dev);
> +
> +	switch (chan->type) {
> +	case IIO_PROXIMITY:
> +		return data->ps_int;
> +	case IIO_INTENSITY:
> +		return data->als_int;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
Unreachable so drop this.

> +}
> +
> +static int apds9160_write_event_config(struct iio_dev *indio_dev,
> +				       const struct iio_chan_spec *chan,
> +				       enum iio_event_type type,
> +				       enum iio_event_direction dir, int state)
> +{
> +	struct apds9160_chip *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	state = !!state;
> +
> +	switch (chan->type) {
> +	case IIO_PROXIMITY:
> +		if (data->ps_int == state)
> +			return -EINVAL;
> +
> +		ret = regmap_field_write(data->reg_int_ps, state);
> +		if (ret)
> +			return ret;
> +		data->ps_int = state;
> +		break;
		return 0;
> +	case IIO_INTENSITY:
> +		if (data->als_int == state)
> +			return -EINVAL;
> +
> +		ret = regmap_field_write(data->reg_int_als, state);
> +		if (ret)
> +			return ret;
> +		data->als_int = state;
> +		break;
		return 0; instead of break if nothing else to do.
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
With above changes, this is unreachable so drop it.

> +}
> +
> +static irqreturn_t apds9160_irq_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct apds9160_chip *data = iio_priv(indio_dev);
> +	int ret, status;
> +
> +	/* Reading status register clears the interrupt flag */
> +	ret = regmap_read(data->regmap, APDS9160_REG_SR, &status);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "irq status reg read failed\n");
> +		return IRQ_HANDLED;
> +	}
> +
> +	if ((status & APDS9160_REG_SR_LS_INT) &&
> +	    (status & APDS9160_REG_SR_LS_NEW_DATA) && data->als_int) {
> +		iio_push_event(indio_dev,
> +			       IIO_UNMOD_EVENT_CODE(IIO_INTENSITY, 0,
> +						    IIO_EV_TYPE_THRESH,
> +						    IIO_EV_DIR_EITHER),
> +			       iio_get_time_ns(indio_dev));
> +	}
> +
> +	if ((status & APDS9160_REG_SR_PS_INT) &&
> +	    (status & APDS9160_REG_SR_PS_NEW_DATA) && data->ps_int) {
> +		/** Interrupt flag is cleared after data read */


Not kernel doc. So just /* 

However, confusing comment to have here and duplicates comment above I think.


> +		iio_push_event(indio_dev,
> +			       IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
> +						    IIO_EV_TYPE_THRESH,
> +						    IIO_EV_DIR_EITHER),
> +			       iio_get_time_ns(indio_dev));
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int apds9160_detect(struct apds9160_chip *chip)
> +{
> +	struct i2c_client *client = chip->client;
> +	int ret;
> +	u32 val;
> +	u8 id;
> +	u8 rev;
> +
> +	ret = regmap_read(chip->regmap, APDS9160_REG_ID, &val);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "ID read failed\n");
> +		return ret;
> +	}
> +
> +	id = APDS9160_PART_ID_MASK & ((u8)val);
FIELD_GET()
> +	rev = APDS9160_PART_REV_MASK & ((u8)val);
> +
> +	switch (id) {
> +	case APDS9160_PART_ID_0:
> +		chip->revision = rev;
> +		dev_info(&client->dev, "Device probed, rev %u\n",
> +			 chip->revision);
Too noisy.
return 0 here.

> +		break;
> +	default:
> +		dev_info(&client->dev, "Unsupported part id %u rev %u\n", id,
> +			 rev);
> +		ret = -ENODEV;
Fine to return an error code from here, just don't fail the probe on the
basis of an unknown part ID.

We have some older drivers that do this, but are slowly moving
to just printing an info message and moving on.

> +		break;
> +	}
> +	return ret;
> +}

> +static int apds9160_configure(struct iio_dev *indio_dev)
> +{
> +	struct apds9160_chip *chip = iio_priv(indio_dev);
> +
> +	return apds9160_chip_init(chip);
> +}
> +
> +static int apds9160_regfield_init(struct apds9160_chip *data)
> +{
> +	struct device *dev = &data->client->dev;
> +	struct regmap *regmap = data->regmap;
> +
> +	data->reg_int_als = devm_regmap_field_alloc(dev, regmap,
> +						    apds9160_reg_field_int_als);
Can you just use the bulk field allocator?

> +	if (IS_ERR(data->reg_int_als)) {
> +		dev_err(dev, "INT ALS reg field init failed\n");
> +		return PTR_ERR(data->reg_int_als);
return dev_err_probe() for all of these if only called form probe.

> +	}
> +
> +	data->reg_int_ps =
> +		devm_regmap_field_alloc(dev, regmap, apds9160_reg_field_int_ps);
> +	if (IS_ERR(data->reg_int_ps)) {
> +		dev_err(dev, "INT ps reg field init failed\n");
> +		return PTR_ERR(data->reg_int_ps);
> +	}
> +
> +	data->reg_enable_als =
> +		devm_regmap_field_alloc(dev, regmap, apds9160_reg_field_ls_en);
> +	if (IS_ERR(data->reg_enable_als)) {
> +		dev_err(dev, "Enable ALS reg field init failed\n");
> +		return PTR_ERR(data->reg_enable_als);
> +	}
> +
> +	data->reg_enable_ps =
> +		devm_regmap_field_alloc(dev, regmap, apds9160_reg_field_ps_en);
> +	if (IS_ERR(data->reg_enable_ps)) {
> +		dev_err(dev, "Enable PS reg field init failed\n");
> +		return PTR_ERR(data->reg_enable_ps);
> +	}
> +
> +	data->reg_ps_overflow = devm_regmap_field_alloc(
> +		dev, regmap, apds9160_reg_field_ps_overflow);
> +	if (IS_ERR(data->reg_ps_overflow)) {
> +		dev_err(dev, "PS overflow reg field init failed\n");
> +		return PTR_ERR(data->reg_ps_overflow);
> +	}
> +
> +	data->reg_als_rate = devm_regmap_field_alloc(
> +		dev, regmap, apds9160_reg_field_als_rate);
> +	if (IS_ERR(data->reg_als_rate)) {
> +		dev_err(dev, "ALS measurement rate field init failed\n");
> +		return PTR_ERR(data->reg_als_rate);
> +	}
> +
> +	data->reg_als_resolution = devm_regmap_field_alloc(
> +		dev, regmap, apds9160_reg_field_als_res);
> +	if (IS_ERR(data->reg_als_resolution)) {
> +		dev_err(dev, "ALS resolution field init failed\n");
> +		return PTR_ERR(data->reg_als_resolution);
> +	}
> +
> +	data->reg_ps_rate = devm_regmap_field_alloc(dev, regmap,
> +						    apds9160_reg_field_ps_rate);
> +	if (IS_ERR(data->reg_ps_rate)) {
> +		dev_err(dev, "PS measurement rate field init failed\n");
> +		return PTR_ERR(data->reg_ps_rate);
> +	}
> +
> +	data->reg_als_gain = devm_regmap_field_alloc(
> +		dev, regmap, apds9160_reg_field_als_gain);
> +	if (IS_ERR(data->reg_als_gain)) {
> +		dev_err(dev, "ALS gain field init failed\n");
> +		return PTR_ERR(data->reg_als_gain);
> +	}
> +
> +	data->reg_ps_current = devm_regmap_field_alloc(
> +		dev, regmap, apds9160_reg_field_ps_current);
> +	if (IS_ERR(data->reg_ps_current)) {
> +		dev_err(dev, "PS current field init failed\n");
> +		return PTR_ERR(data->reg_ps_current);
> +	}
> +
> +	data->reg_ps_gain = devm_regmap_field_alloc(dev, regmap,
> +						    apds9160_reg_field_ps_gain);
> +	if (IS_ERR(data->reg_ps_gain)) {
> +		dev_err(dev, "PS gain field init failed\n");
> +		return PTR_ERR(data->reg_ps_gain);
> +	}
> +
> +	data->reg_ps_resolution = devm_regmap_field_alloc(
> +		dev, regmap, apds9160_reg_field_ps_resolution);
> +	if (IS_ERR(data->reg_ps_resolution)) {
> +		dev_err(dev, "PS resolution field init failed\n");
> +		return PTR_ERR(data->reg_ps_resolution);
> +	}
> +
> +	return 0;
> +}
> +
> +static int apds9160_disable(struct apds9160_chip *data)
> +{
> +	int ret;
> +
> +	ret = regmap_field_write(data->reg_enable_als, 0);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_field_write(data->reg_enable_ps, 0);
> +}
> +
> +static int apds9160_shutdown(struct iio_dev *indio_dev)
> +{
> +	struct apds9160_chip *data = iio_priv(indio_dev);
> +
I think you are using the buffer interfaces just to provide
an enable / disable?  Don't do that - instead implement runtime
PM with autosuspend.  That way the sensor will be powered down
if it isn't accessed for a bit.  When you enable events
you just hold it on until they are disabled.


> +	return apds9160_disable(data);
> +}
> +
> +static void apds9160_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
> +	struct apds9160_chip *data = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +	apds9160_disable(data);
convention is to put this after probe()
However with changes suggested in probe() you won't need this
at all.

> +}
> +
> +static const struct iio_buffer_setup_ops apds9160_buffer_setup_ops = {
> +	.postenable = apds9160_configure,
> +	.predisable = apds9160_shutdown,
> +};
> +
> +static const struct iio_info apds9160_info = {
> +	.attrs = &apds9160_attribute_group,
> +	.read_raw = apds9160_read_raw,
> +	.write_raw = apds9160_write_raw,
> +	.read_event_value = apds9160_read_event,
> +	.write_event_value = apds9160_write_event,
> +	.read_event_config = apds9160_read_event_config,
> +	.write_event_config = apds9160_write_event_config,
> +};
> +
> +static int apds9160_probe(struct i2c_client *client)
> +{
> +	struct apds9160_chip *chip;
> +	struct iio_dev *indio_dev;
> +	int err;
> +
> +	dev_info(&client->dev,
> +		 "Loading proximity/ambient light sensor driver\n");
Far too noisy. It is easy to find out if the driver has probed from
other means so we tend not to put generic entries in the kernel log.

> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
Can fail. Check for that.

> +	indio_dev->info = &apds9160_info;
> +	indio_dev->name = APDS9160_DRIVER_NAME;

Put it here as a string. There is no particularly reason this (which is
the part number) should match the driver name.

> +	indio_dev->channels = apds9160_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(apds9160_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	err = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
> +					  &apds9160_buffer_setup_ops);
> +	if (err)
> +		return err;
> +
> +	chip = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +	chip->client = client;
> +	chip->regmap = devm_regmap_init_i2c(client, &apds9160_regmap_config);
> +	if (IS_ERR(chip->regmap)) {
> +		dev_err(&client->dev, "regmap initialization failed.\n");
> +		return PTR_ERR(chip->regmap);
		return dev_err_probe()...
For all error messages in probe.

> +	}
> +
> +	chip->client = client;
> +	chip->indio_dev = indio_dev;
> +	mutex_init(&chip->lock);
> +
> +	err = apds9160_detect(chip);
> +	if (err < 0) {
Don't fail on this. Fine to print a message though to say we have
a part we don't recognise.

Actually failing prevents the use of fallback compatibles in DT
etc that allow an older kernel to work with new devices as long
as they are backwards compatible (with exception of the ID register).
> +		dev_err(&client->dev, "apds9160 not found\n");
> +		return err;
> +	}
> +
> +	err = apds9160_regfield_init(chip);
> +	if (err)
> +		return err;
> +
> +	err = apds9160_chip_init(chip);
> +	if (err)
> +		return err;
Register a callback to turn the chip off again here with 
	ret = devm_add_action_or_reset()

That avoids mixing devm and non devm cleanup and allows direct returns.

> +
> +	if (client->irq > 0) {
> +		err = devm_request_threaded_irq(
> +			&client->dev, client->irq, NULL, apds9160_irq_handler,
> +			IRQF_TRIGGER_FALLING | IRQF_ONESHOT, "apds9160_event",
Direction needs to come from firmware so drop the FALLING part.

> +			indio_dev);
> +		if (err) {
> +			dev_err(&client->dev, "request irq (%d) failed\n",
> +				client->irq);
> +			goto fail;
> +		}
> +	} else {
> +		dev_info(&client->dev,
> +			 "init: no IRQ defined, ps/als interrupts disabled\n");
Easy to find out, so no need for print.
> +	}
> +
> +	err = iio_device_register(indio_dev);
With above changes make this a devm_call as well.
> +	if (err)
> +		goto fail;
> +
> +	return 0;
> +fail:
> +	apds9160_disable(chip);
With devm based disable this goes.
> +	return err;
> +}
> +
> +static const struct of_device_id apds9160_of_match[] = {
> +	{ .compatible = "avago,apds9160" },
> +	{ .compatible = "broadmobi,apds9160" },
> +	{}
Trivial local formatting thing but in IIO I'm trying to standardize on
	{ }
for these terminator entries.

> +};
> +MODULE_DEVICE_TABLE(of, apds9160_of_match);
> +
> +static struct i2c_driver apds9160_driver = {
> +	.driver	  = {
> +		.name	= APDS9160_DRIVER_NAME,
> +		.owner = THIS_MODULE,
> +		.of_match_table = apds9160_of_match,
> +	},
> +	.probe    = apds9160_probe,
> +	.remove	  = apds9160_remove,
> +	.id_table = apds9160_id,
> +};

Re: [PATCH 2/2] iio: light: Add APDS9160 ALS & Proximity sensor driver

[Mikael Gonella-Bolduc]

On Sun, Nov 24, 2024 at 09:15:45PM +0000, Jonathan Cameron wrote:
> On Tue, 19 Nov 2024 15:36:57 -0500
> Mikael Gonella-Bolduc via B4 Relay <devnull+mgonellabolduc.dimonoff.com@kernel.org> wrote:
> 
> > From: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
> > 
> > APDS9160 is a combination of ALS and proximity sensors.
> > 
> > This patch add supports for:
> >     - Intensity clear data and illuminance data
> >     - Proximity data
> >     - Gain control, rate control
> >     - Event thresholds
> > 
> > Signed-off-by: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
> Hi Mikael,
> 
> I may well repeat things you have already from the other reviewers!
> 
> Various comments inline.  Big one is that you register a buffer
> but no channels to be captured and never push any data to it.
> That looks like a workaround for something else?
> 
> Jonathan
> 
> 
> 
> > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> > index f2f3e414849ab12a7c0ea2b08e9a3310eb18ebb7..69a59c6759acea89241ab76bfcdfafe3e5824066 100644
> > --- a/drivers/iio/light/Kconfig
> > +++ b/drivers/iio/light/Kconfig
> > @@ -63,6 +63,19 @@ config AL3320A
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called al3320a.
> >  
> > +config APDS9160
> > +	tristate "APDS9160 combined als and proximity sensors"
> > +	select REGMAP_I2C
> > +	select IIO_BUFFER
> > +	select IIO_KFIFO_BUF
> Normally I'd expect a light sensor to be providing a triggered buffer
> rather than using the kfifo path directly.
> 
> Also you never push anything into the buffer.
> 
> Normally we only skip the trigger (which allows other signals to be used
> to start a scan of the channels, or this trigger to capture on other sensors)
> if there is a hardware buffer involved so we can't identify each individual
> scan of the channels.
> 
> As it turns out you aren't doing buffered capture at all.
> 
> > +	depends on I2C
> > +	help
> > +	   Say Y here if you want to build a driver for Broadcom APDS9160
> > +	   combined ambient light and proximity sensor chip.
> > +
> > +	   To compile this driver as a module, choose M here: the
> > +	   module will be called apds9160. If unsure, say N here.
> > +
> 
> > diff --git a/drivers/iio/light/apds9160.c b/drivers/iio/light/apds9160.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..cb855f20725dba9fea1390a955889d905fd7eb4f
> > --- /dev/null
> > +++ b/drivers/iio/light/apds9160.c
> > @@ -0,0 +1,1420 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/**
> > + * This file is part of the APDS9160 sensor driver.
> > + * Chip is combined proximity and ambient light sensor.
> > + * Author: Mikael Gonella-Bolduc <m.gonella.bolduc@gmail.com>
> > + */
> > +
> > +#include <linux/acpi.h>
> Why?
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/delay.h>
> > +#include <linux/mutex.h>
> > +#include <linux/err.h>
> > +#include <linux/irq.h>
> > +#include <linux/i2c.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regmap.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/events.h>
> > +#include <linux/iio/kfifo_buf.h>
> > +#include <linux/iio/sysfs.h>
> > +
> > +#define APDS9160_DRIVER_NAME "apds9160"
> > +#define APDS9160_REGMAP_NAME "apds9160_regmap"
> > +#define APDS9160_STARTUP_DELAY 25000 /* us */
> > +#define APDS9160_REG_CNT 37
> > +
> > +/** Main control register */
> > +#define APDS9160_REG_CTRL 0x00
> > +#define APDS9160_REG_CTRL_SWRESET BIT(4) /* 1: Activate reset */
> > +#define APDS9160_REG_CTRL_MODE_RGB BIT(2) /* 0: ALS & IR, 1: RGB & IR */
> > +#define APDS9160_REG_CTRL_EN_ALS BIT(1) /* 1: ALS active */
> > +#define APDS9160_REG_CTLR_EN_PS BIT(0) /* 1: PS active */
> > +
> > +/** Status register  */
> Run kernel-doc over the file and fix everything. This is not
> kernel-doc so should not have /**
> 
> > +#define APDS9160_REG_SR_LS_INT BIT(3)
> > +#define APDS9160_REG_SR_LS_NEW_DATA BIT(2)
> > +#define APDS9160_REG_SR_PS_INT BIT(1)
> > +#define APDS9160_REG_SR_PS_NEW_DATA BIT(0)
> > +
> > +/* Interrupt configuration register */
> > +#define APDS9160_REG_INT_CFG 0x19
> > +#define APDS9160_REG_INT_CFG_EN_LS BIT(2) /* LS int enable */
> > +#define APDS9160_REG_INT_CFG_EN_PS BIT(0) /* PS int enable */
> > +#define APDS9160_REG_INT_PST 0x1A
> > +
> > +/* Proximity registers */
> > +#define APDS9160_REG_PS_LED 0x01
> > +#define APDS9160_REG_PS_PULSES 0x02
> > +#define APDS9160_REG_PS_MEAS_RATE 0x03
> > +#define APDS9160_REG_PS_THRES_HI_LSB 0x1B
> > +#define APDS9160_REG_PS_THRES_HI_MSB 0x1C
> > +#define APDS9160_REG_PS_THRES_LO_LSB 0x1D
> > +#define APDS9160_REG_PS_THRES_LO_MSB 0x1E
> > +#define APDS9160_REG_PS_DATA_LSB 0x08
> > +#define APDS9160_REG_PS_DATA_MSB 0x09
> > +#define APDS9160_REG_PS_CAN_LEVEL_DIG_LSB 0x1F
> > +#define APDS9160_REG_PS_CAN_LEVEL_DIG_MSB 0x20
> > +#define APDS9160_REG_PS_CAN_LEVEL_ANA_DUR 0x21
> > +#define APDS9160_REG_PS_CAN_LEVEL_ANA_CURRENT 0x22
> > +
> > +/* Light sensor registers */
> > +#define APDS9160_REG_LS_MEAS_RATE 0x04
> > +#define APDS9160_REG_LS_GAIN 0x05
> > +#define APDS9160_REG_LS_DATA_CLEAR_LSB 0x0A
> > +#define APDS9160_REG_LS_DATA_CLEAR 0x0B
> > +#define APDS9160_REG_LS_DATA_CLEAR_MSB 0x0C
> > +#define APDS9160_REG_LS_DATA_ALS_LSB 0x0D
> > +#define APDS9160_REG_LS_DATA_ALS 0x0E
> > +#define APDS9160_REG_LS_DATA_ALS_MSB 0x0F
> > +#define APDS9160_REG_LS_THRES_UP_LSB 0x24
> > +#define APDS9160_REG_LS_THRES_UP 0x25
> > +#define APDS9160_REG_LS_THRES_UP_MSB 0x26
> > +#define APDS9160_REG_LS_THRES_LO_LSB 0x27
> > +#define APDS9160_REG_LS_THRES_LO 0x28
> > +#define APDS9160_REG_LS_THRES_LO_MSB 0x29
> > +#define APDS9160_REG_LS_THRES_VAR 0x2A
> > +
> > +/** Part identification number register */
> > +#define APDS9160_REG_ID 0x06
> > +
> > +/** Status register */
> > +#define APDS9160_REG_SR 0x07
> > +#define APDS9160_REG_SR_DATA_ALS BIT(3)
> 
> One trick to make it obvious what is a field and what
> a register is don't include the _REG bit in
> the field defines.
> 
> > +#define APDS9160_REG_SR_DATA_PS BIT(0)
> > +
> > +/* Supported ID:s */
> > +#define APDS9160_PART_ID_0 0x00
> > +#define APDS9160_PART_ID_MASK 0xF0
> > +#define APDS9160_PART_REV_MASK 0x0F
> > +
> > +#define APDS9160_PS_THRES_MAX 0x7FF
> > +#define APDS9160_LS_THRES_MAX 0xFFFFF
> > +#define APDS9160_CMD_LS_RESOLUTION_25MS 0x04
> > +#define APDS9160_CMD_LS_RESOLUTION_50MS 0x03
> > +#define APDS9160_CMD_LS_RESOLUTION_100MS 0x02
> > +#define APDS9160_CMD_LS_RESOLUTION_200MS 0x01
> > +#define APDS9160_PS_DATA_MASK 0x7FF
> > +
> > +#define APDS9160_DEFAULT_LS_GAIN 3
> > +#define APDS9160_DEFAULT_LS_RATE 100
> > +#define APDS9160_DEFAULT_PS_RATE 100
> > +#define APDS9160_DEFAULT_PS_CANCELLATION_LEVEL 0
> > +#define APDS9160_DEFAULT_PS_ANALOG_CANCELLATION 0
> > +#define APDS9160_DEFAULT_PS_GAIN 1
> > +#define APDS9160_DEFAULT_PS_CURRENT 100
> > +#define APDS9160_DEFAULT_PS_RESOLUTION 0x03 // 11 bits
> Name so that it is obvious it is the value for 11 bits.
> 
> > +
> > +// clang-format off
> > +static const struct reg_default apds9160_reg_defaults[] = {
> you can explicitly tell regmap to write these to the device. That will save
> a bunch of init code.
> 
> > +	{ APDS9160_REG_CTRL, 0x00 }, /* Sensors disabled by default  */
> > +	{ APDS9160_REG_PS_LED, 0x33 }, /* 60 kHz frequency, 100 mA pulse current */
> > +	{ APDS9160_REG_PS_PULSES, 0x08 }, /* 8 pulses */
> > +	{ APDS9160_REG_PS_MEAS_RATE, 0x05 },
> > +	{ APDS9160_REG_LS_MEAS_RATE, 0x22 },
> > +	{ APDS9160_REG_LS_GAIN, 0x01 },
> > +	{ APDS9160_REG_INT_CFG, 0x10 },
> > +	{ APDS9160_REG_INT_PST, 0x00 },
> > +	{ APDS9160_REG_PS_THRES_HI_LSB, 0xFF },
> > +	{ APDS9160_REG_PS_THRES_HI_MSB, 0x07 },
> > +	{ APDS9160_REG_PS_THRES_LO_LSB, 0x00 },
> > +	{ APDS9160_REG_PS_THRES_LO_MSB, 0x00 },
> > +	{ APDS9160_REG_PS_CAN_LEVEL_DIG_LSB, 0x00 },
> > +	{ APDS9160_REG_PS_CAN_LEVEL_DIG_MSB, 0x00 },
> > +	{ APDS9160_REG_PS_CAN_LEVEL_ANA_DUR, 0x00 },
> > +	{ APDS9160_REG_PS_CAN_LEVEL_ANA_CURRENT, 0x00 },
> > +	{ APDS9160_REG_LS_THRES_UP_LSB, 0xFF },
> > +	{ APDS9160_REG_LS_THRES_UP, 0xFF },
> > +	{ APDS9160_REG_LS_THRES_UP_MSB, 0x0F },
> > +	{ APDS9160_REG_LS_THRES_LO_LSB, 0x00 },
> > +	{ APDS9160_REG_LS_THRES_LO, 0x00 },
> > +	{ APDS9160_REG_LS_THRES_LO_MSB, 0x00 },
> > +	{ APDS9160_REG_LS_THRES_VAR, 0x00 },
> > +};
> > +// clang-format on
> 
> > +
> > +static const struct iio_event_spec apds9160_ps_event_spec[] = {
> > +	{
> > +		.type = IIO_EV_TYPE_THRESH,
> > +		.dir = IIO_EV_DIR_RISING,
> > +		.mask_separate = BIT(IIO_EV_INFO_VALUE),
> Given they are the same, why not use same event_spec array for als and ps.
> 
> > +	},
> > +	{
> > +		.type = IIO_EV_TYPE_THRESH,
> > +		.dir = IIO_EV_DIR_FALLING,
> > +		.mask_separate = BIT(IIO_EV_INFO_VALUE),
> > +	},
> > +	{
> > +		.type = IIO_EV_TYPE_THRESH,
> > +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> > +	},
> > +};
> > +
> > +static const struct iio_event_spec apds9160_als_event_spec[] = {
> > +	{
> > +		.type = IIO_EV_TYPE_THRESH,
> > +		.dir = IIO_EV_DIR_RISING,
> > +		.mask_separate = BIT(IIO_EV_INFO_VALUE),
> > +	},
> > +	{
> > +		.type = IIO_EV_TYPE_THRESH,
> > +		.dir = IIO_EV_DIR_FALLING,
> > +		.mask_separate = BIT(IIO_EV_INFO_VALUE),
> > +	},
> > +	{
> Explicitly set DIR_EITHER here.  It's not a completely obvious default
> > +		.type = IIO_EV_TYPE_THRESH,
> > +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> > +	},
> > +};
> > +
> > +static const struct iio_chan_spec apds9160_channels[] = {
> > +	{
> > +		/* Proximity sensor channel */
> > +		.type = IIO_PROXIMITY,
> > +		.address = APDS9160_REG_PS_DATA_LSB,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> > +					    BIT(IIO_CHAN_INFO_CALIBSCALE) |
> > +					    BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
> > +					    BIT(IIO_CHAN_INFO_CALIBBIAS),
> > +		.channel = 0,
> > +		.indexed = 0,
> > +		.scan_index = -1,
> No need to set scan_index if you don't register a buffer (which you shouldn't
> without more work and these having actual values).
> > +
> > +		.event_spec = apds9160_ps_event_spec,
> > +		.num_event_specs = ARRAY_SIZE(apds9160_ps_event_spec),
> > +	},
> > +	{
> > +		/* Proximity sensor led current */
> > +		.type = IIO_CURRENT,
> > +		.output = 1,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > +		.scan_index = -1,
> > +	},
> > +	{
> > +		/* Clear channel */
> > +		.type = IIO_INTENSITY,
> > +		.address = APDS9160_REG_LS_DATA_CLEAR_LSB,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_INT_TIME) |
> > +					    BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
> > +					    BIT(IIO_CHAN_INFO_SCALE),
> > +		.channel2 = IIO_MOD_LIGHT_CLEAR,
> > +		.modified = 1,
> > +		.scan_index = -1,
> > +
> > +		.event_spec = apds9160_als_event_spec,
> > +		.num_event_specs = ARRAY_SIZE(apds9160_als_event_spec),
> > +	},
> > +	{
> > +		/* Illuminance */
> > +		.type = IIO_LIGHT,
> > +		.address = APDS9160_REG_LS_DATA_ALS_LSB,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> > +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> > +		.scan_index = -1,
> > +	}
> > +};
> > +
> > +/* Attributes */
> > +// clang-format off
> > +static const int apds9160_als_rate_map[][2] = {
> > +	{ 25, 0x00 },
> > +	{ 50, 0x01 },
> > +	{ 100, 0x02 },
> > +	{ 200, 0x03 }
> > +};
> > +
> > +static const int apds9160_als_gain_map[][2] = {
> > +	{ 1, 0x00 },
> > +	{ 3, 0x01 },
> > +	{ 6, 0x02 },
> > +	{ 18, 0x03 },
> > +	{ 64, 0x04 }
> > +};
> > +
> > +static const int apds9160_ps_gain_map[][2] = {
> > +	{ 1, 0x00 },
> > +	{ 2, 0x01},
> > +	{ 4, 0x02},
> > +	{ 8, 0x03}
> Make spacing consistent throughout. Same as first
> line here looks good.
> 
> > +};
> > +
> > +static const int apds9160_ps_rate_map[][2] = {
> > +	{ 25, 0x03 },
> > +	{ 50, 0x04 },
> > +	{ 100, 0x05 },
> > +	{ 200, 0x06 },
> > +	{ 400, 0x07 }
> > +};
> > +
> > +static const int adps9160_ps_led_current_map[][2] = {
> > +	{ 10, 0x00 },
> > +	{ 25, 0x01 },
> > +	{ 50, 0x02 },
> > +	{ 100, 0x03 },
> > +	{ 150, 0x04},
> > +	{ 175, 0x05 },
> > +	{ 200, 0x06 }
> Add trailing commas. In theory we might have more values to add
> (though very unlikely).
> > +};
> > +// clang-format on
> No to these Just ignore what clang-format tries to do.
> It should be treated as hints, not rules for kernel code.
> > +
> > +struct apds9160_scale {
> > +	int itime;
> > +	int gain;
> > +	int scale1;
> > +	int scale2;
> > +};
> > +
> > +static const struct apds9160_scale apds9160_als_scale_map[] = {
> > +	{
> > +		.gain = 1,
> > +		.itime = 25,
> > +		.scale1 = 3272,
> > +		.scale2 = 1000,
> 
> No way to compute scale from gain and itime?  If not I'm not sure
> if we can use the gts helpers.
> 
> > +	},
> > +	{
> > +		.gain = 1,
> > +		.itime = 50,
> > +		.scale1 = 1639,
> > +		.scale2 = 1000,
> > +	},
> > +	{
> > +		.gain = 1,
> > +		.itime = 100,
> > +		.scale1 = 819,
> > +		.scale2 = 1000,
> > +	},
> > +	{
> > +		.gain = 3,
> > +		.itime = 25,
> > +		.scale1 = 1077,
> > +		.scale2 = 1000,
> > +	},
> > +	{
> > +		.gain = 3,
> > +		.itime = 50,
> > +		.scale1 = 538,
> > +		.scale2 = 1000,
> > +	},
> > +	{
> > +		.gain = 3,
> > +		.itime = 100,
> > +		.scale1 = 269,
> > +		.scale2 = 1000,
> > +	},
> > +	{
> > +		.gain = 6,
> > +		.itime = 25,
> > +		.scale1 = 525,
> > +		.scale2 = 1000,
> > +	},
> > +	{
> > +		.gain = 6,
> > +		.itime = 50,
> > +		.scale1 = 263,
> > +		.scale2 = 1000,
> > +	},
> > +	{
> > +		.gain = 6,
> > +		.itime = 100,
> > +		.scale1 = 131,
> > +		.scale2 = 1000,
> > +	},
> > +	{
> > +		.gain = 18,
> > +		.itime = 25,
> > +		.scale1 = 169,
> > +		.scale2 = 1000,
> > +	},
> > +	{
> > +		.gain = 18,
> > +		.itime = 50,
> > +		.scale1 = 84,
> > +		.scale2 = 1000,
> > +	},
> > +	{
> > +		.gain = 18,
> > +		.itime = 100,
> > +		.scale1 = 42,
> > +		.scale2 = 1000,
> > +	},
> > +	{
> > +		.gain = 64,
> > +		.itime = 25,
> > +		.scale1 = 49,
> > +		.scale2 = 1000,
> > +	},
> > +	{
> > +		.gain = 64,
> > +		.itime = 50,
> > +		.scale1 = 25,
> > +		.scale2 = 1000,
> > +	},
> > +	{
> > +		.gain = 64,
> > +		.itime = 100,
> > +		.scale1 = 12,
> > +		.scale2 = 1000,
> > +	},
> > +};
> > +
> > +static IIO_CONST_ATTR(in_intensity_integration_time_available, "25 50 100 200");
> > +static IIO_CONST_ATTR(in_proximity_sampling_frequency_available,
> > +		      "25 50 100 200 400");
> > +static IIO_CONST_ATTR(in_intensity_hardwaregain_available, "1 3 6 18 64");
> > +static IIO_CONST_ATTR(in_proximity_hardwaregain_available, "1 2 4 8");
> > +static IIO_CONST_ATTR(out_current_available, "10 25 50 100 150 175 200");
> Use read_avail and appropriate masks. 
> 
> Whilst we used to do it with explicit attributes for modern drivers we like
> to make the data available to in kernel users and ensure there is only one
> source of the values by using the same arrays for matching write values.
> 
> > +
> > +static struct attribute *apds9160_attributes[] = {
> > +	&iio_const_attr_in_intensity_integration_time_available.dev_attr.attr,
> > +	&iio_const_attr_in_intensity_hardwaregain_available.dev_attr.attr,
> > +	&iio_const_attr_in_proximity_sampling_frequency_available.dev_attr.attr,
> > +	&iio_const_attr_in_proximity_hardwaregain_available.dev_attr.attr,
> > +	&iio_const_attr_out_current_available.dev_attr.attr,
> > +	NULL,
> No comma on NULL terminators. However, no reason any of these should
> need to be handled with custom attributes.
> 
> > +};
> > +
> > +
> > +struct apds9160_chip {
> > +	struct i2c_client *client;
> > +	struct iio_dev *indio_dev;
> That sets alarm bells off. If you ever need to go from
> iio_priv() to struct iio_dev then you almost certainly need to rethink your
> design.  Thankfully I think you only ever set this.  Drop it.
> 
> > +	struct regmap *regmap;
> > +	struct mutex lock; /* avoid parallel access */
> You already have feedback on this comment so I won't add anything.
> 
> > +
> > +	struct regmap_field *reg_enable_ps;
> > +	struct regmap_field *reg_enable_als;
> > +	struct regmap_field *reg_int_ps;
> > +	struct regmap_field *reg_int_als;
> > +	struct regmap_field *reg_ps_overflow;
> > +	struct regmap_field *reg_als_rate;
> > +	struct regmap_field *reg_als_resolution;
> > +	struct regmap_field *reg_ps_rate;
> > +	struct regmap_field *reg_als_gain;
> > +	struct regmap_field *reg_ps_current;
> > +	struct regmap_field *reg_ps_gain;
> > +	struct regmap_field *reg_ps_resolution;
> > +
> > +	/* State data */
> > +	u8 revision;
> > +	int als_int;
> > +	int ps_int;
> > +
> > +	/* Configuration values */
> > +	int als_itime;
> These are all caches of register values. Maybe simpler to just enable the regmap
> cache and read them back form the cached versions of the hardware registers.
> > +	int als_hwgain;
> > +	int als_scale1;
> > +	int als_scale2;
> > +	int ps_rate;
> > +	int ps_cancellation_level;
> > +	int ps_cancellation_analog;
> > +	int ps_current;
> > +	int ps_gain;
> > +};
> > +
> > +static const struct i2c_device_id apds9160_id[] = { { APDS9160_DRIVER_NAME, 0 },
> > +						    {} };
> Move this down to just before where it is used.
> Also, just hard code the string. It is much easier to grep for then as no reason
> the name in the device id should match the driver name.
> 
> > +
> > +/** Called when mutex is locked */
> Not kernel doc so /* only.
> > +static void apds9160_set_scale(struct apds9160_chip *data)
> > +{
> > +	for (int idx = 0; idx < ARRAY_SIZE(apds9160_als_scale_map); idx++) {
> > +		if (data->als_hwgain == apds9160_als_scale_map[idx].gain &&
> > +		    data->als_itime == apds9160_als_scale_map[idx].itime) {
> > +			data->als_scale1 = apds9160_als_scale_map[idx].scale1;
> > +			data->als_scale2 = apds9160_als_scale_map[idx].scale2;
> > +		}
> > +	}
> > +}
> 
> 
> > +/**
> > + * This parameter determines the cancellation pulse duration in each of the PWM pulse.
> > + * The cancellation is applied during the integration phase of the PS measurement.
> > + * Duration is programmed in half clock cycles
> This is unusual. and I don't really understand how it works.
> > + */
> > +static int apds9160_set_ps_analog_cancellation(struct apds9160_chip *data,
> > +					       int val)
> > +{
> > +	int ret = -EINVAL;
> overwritten in all paths where it is used.
> > +
> > +	dev_dbg(&data->client->dev, "%s - set analog cancellation to %i\n",
> > +		__func__, val);
> > +	if (val < 0 || val > 0x3F)
> > +		return ret;
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = regmap_write(data->regmap, APDS9160_REG_PS_CAN_LEVEL_ANA_DUR,
> > +			   val);
> > +	if (!ret)
> Always keep error paths out of line.  It just makes things more consistent
> and hence easier to review.
> 
> The cleanup.h magic like guard() can help a lot with keeping code simple when
> doing early returns.
> 
> > +		data->ps_cancellation_analog = val;
> > +	mutex_unlock(&data->lock);
> > +	return ret;
> > +}
> > +
> > +/**
> Not kernel doc so /* only.
> > + * Set the proximity sensor led current
> > + */
> > +static int apds9160_set_ps_current(struct apds9160_chip *data, int val)
> > +{
> > +	int ret = -EINVAL;
> > +	int idx;
> > +
> > +	dev_dbg(&data->client->dev, "%s - set current to %i\n", __func__, val);
> > +	for (idx = 0; idx < ARRAY_SIZE(adps9160_ps_led_current_map); idx++) {
> > +		if (adps9160_ps_led_current_map[idx][0] == val) {
> > +			mutex_lock(&data->lock);
> I'd suggest changes similar to what I said for the next function
> > +			ret = regmap_field_write(
> > +				data->reg_ps_current,
> > +				adps9160_ps_led_current_map[idx][1]);
> > +
> > +			if (!ret)
> > +				data->ps_current = val;
> > +
> > +			mutex_unlock(&data->lock);
> > +			break;
> > +		}
> > +	}
> > +	return ret;
> > +}
> > +
> > +static int apds9160_set_als_gain(struct apds9160_chip *data, int val)
> > +{
> > +	int ret = -EINVAL;
> > +	int idx;
> > +
> > +	dev_dbg(&data->client->dev, "%s - set als gain to %i\n", __func__, val);
> > +	for (idx = 0; idx < ARRAY_SIZE(apds9160_als_gain_map); idx++) {
> > +		if (apds9160_als_gain_map[idx][0] == val) {
> 
> Flip this to reduce indent and give more readable code.
> 
> 		if (apds9160_als_gain_map[idx][0] != val)
> 			continue;
> 
> 
> > +			mutex_lock(&data->lock);
> guard(mutex)(&data-<lock).
> > +			ret = regmap_field_write(data->reg_als_gain,
> > +						 apds9160_als_gain_map[idx][1]);
> 			if (ret)
> 				return ret;
> 			data->als_hwgain = val...
> 
> > +			if (!ret) {
> > +				data->als_hwgain = val;
> > +				apds9160_set_scale(data);
> > +			}
> > +			mutex_unlock(&data->lock);
> > +			break;
> > +		}
> > +	}
> > +	return ret;
> > +}
> > +
> > +static int apds9160_set_als_int_time(struct apds9160_chip *data, int val)
> > +{
> > +	int ret = -EINVAL;
> > +	int idx;
> > +
> > +	dev_dbg(&data->client->dev, "%s - set int time to %i\n", __func__, val);
> > +	for (idx = 0; idx < ARRAY_SIZE(apds9160_als_rate_map); idx++) {
> > +		if (apds9160_als_rate_map[idx][0] == val) {
> Flip logic and use a continue to reduce indent for what follows (see example below).
> 
> > +			mutex_lock(&data->lock);
> guard(mutex)(&data->lock);
> 
> > +			ret = regmap_field_write(data->reg_als_rate,
> > +						 apds9160_als_rate_map[idx][1]);
> > +			if (!ret) {
> 			if (ret)
> 				return ret;
> 
> 			data->als_itime = val;
> 			
> 				
> > +				data->als_itime = val;
> > +				/* Lower resolution for faster rates */
> > +				switch (val) {
> > +				case 25:
> > +					ret = regmap_field_write(
> > +						data->reg_als_resolution,
> > +						APDS9160_CMD_LS_RESOLUTION_25MS);
> > +					break;
> > +				case 50:
> > +					ret = regmap_field_write(
> > +						data->reg_als_resolution,
> > +						APDS9160_CMD_LS_RESOLUTION_50MS);
> > +					break;
> > +				case 200:
> > +					ret = regmap_field_write(
> > +						data->reg_als_resolution,
> > +						APDS9160_CMD_LS_RESOLUTION_200MS);
> > +					break;
> > +				default:
> > +					ret = regmap_field_write(
> > +						data->reg_als_resolution,
> > +						APDS9160_CMD_LS_RESOLUTION_100MS);
> > +				}
> > +				apds9160_set_scale(data);
> > +			}
> > +			mutex_unlock(&data->lock);
> > +			break;
> > +		}
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int apds9160_read_raw(struct iio_dev *indio_dev,
> > +			     struct iio_chan_spec const *chan, int *val,
> > +			     int *val2, long mask)
> > +{
> > +	struct apds9160_chip *data = iio_priv(indio_dev);
> > +	int ret = -EINVAL;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		switch (chan->type) {
> > +		case IIO_PROXIMITY: {
> > +			__le16 buf;
> > +
> > +			ret = regmap_bulk_read(data->regmap, chan->address,
> > +					       &buf, 2);
> Should be a dma safe buffer for any bulk accesses.
> > +			if (!ret) {
> Flip logic to return in error path.
> 			if (ret)
> 				return ret;
> 
> 			*val = le16_to_cpu(buf);
> 			*val = FIELD_GET() for what you want, not masking what you don't.
> 
> > +				ret = IIO_VAL_INT;
> > +				*val = le16_to_cpu(buf);
> > +				// Remove data overflow and ambient light overflow bits from result
> 
> /* */
> 
> > +				*val &= APDS9160_PS_DATA_MASK;
> > +			}
> > +		} break;
> > +		case IIO_LIGHT:
> > +		case IIO_INTENSITY: {
> > +			__le32 buf = 0;
> > +
> > +			ret = regmap_bulk_read(data->regmap, chan->address,
> > +					       &buf, 3);
> as below. Use a u8 [3]  and get_unaligned_le24()
> 
> > +			if (!ret) {
> > +				ret = IIO_VAL_INT;
> > +				*val = le32_to_cpu(buf);
> > +			}
> > +		} break;
> > +		case IIO_CURRENT: {
> {} not needed.
> > +			ret = IIO_VAL_INT;
> > +			*val = data->ps_current;
> > +		} break;
> > +		default:
> > +			break;
> > +		}
> > +		break;
> > +	case IIO_CHAN_INFO_HARDWAREGAIN:
> As for the write, this is unlikely to be the right interface
> but I'm not 100% sure what the various gain related controls
> are affecting.  I haven't dog into the datasheet yet and
> won't get time today.
> 
> > +		switch (chan->type) {
> > +		case IIO_INTENSITY:
> > +			ret = IIO_VAL_INT;
> > +			*val = data->als_hwgain;
> > +			break;
> > +		case IIO_PROXIMITY:
> > +			ret = IIO_VAL_INT;
> > +			*val = data->ps_gain;
> > +			break;
> > +		default:
> > +			break;
> > +		}
> > +		break;
> > +	case IIO_CHAN_INFO_INT_TIME:
> > +		switch (chan->type) {
> > +		case IIO_INTENSITY:
> > +			ret = IIO_VAL_INT;
> > +			*val = data->als_itime;
> > +			break;
> > +		default:
> > +			break;
> > +		}
> > +		break;
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		switch (chan->type) {
> > +		case IIO_PROXIMITY:
> > +			ret = IIO_VAL_INT;
> > +			*val = data->ps_rate;
> > +			break;
> > +		default:
> > +			break;
> > +		}
> > +		break;
> > +	case IIO_CHAN_INFO_CALIBSCALE:
> > +		switch (chan->type) {
> > +		case IIO_PROXIMITY:
> > +			ret = IIO_VAL_INT;
> > +			*val = data->ps_cancellation_level;
> > +			break;
> > +		default:
> > +			break;
> > +		}
> > +		break;
> > +	case IIO_CHAN_INFO_CALIBBIAS:
> > +		switch (chan->type) {
> > +		case IIO_PROXIMITY:
> > +			ret = IIO_VAL_INT;
> > +			*val = data->ps_cancellation_analog;
> > +			break;
> > +		default:
> > +			break;
> > +		}
> > +		break;
> > +	case IIO_CHAN_INFO_SCALE:
> > +		switch (chan->type) {
> > +		case IIO_LIGHT:
> > +		case IIO_INTENSITY:
> > +			ret = IIO_VAL_FRACTIONAL;
> > +			*val = data->als_scale1;
> > +			*val2 = data->als_scale2;
> > +			break;
> > +		default:
> > +			break;
> > +		}
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return ret;
> > +};
> > +
> > +static int apds9160_write_raw(struct iio_dev *indio_dev,
> > +			      struct iio_chan_spec const *chan, int val,
> > +			      int val2, long mask)
> > +{
> > +	struct apds9160_chip *data = iio_priv(indio_dev);
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_INT_TIME:
> > +		if (val2 != 0)
> > +			return -EINVAL;
> > +		switch (chan->type) {
> > +		case IIO_INTENSITY:
> > +			return apds9160_set_als_int_time(data, val);
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		if (val2 != 0)
> > +			return -EINVAL;
> > +		switch (chan->type) {
> > +		case IIO_PROXIMITY:
> > +			return apds9160_set_ps_rate(data, val);
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	case IIO_CHAN_INFO_HARDWAREGAIN:
> 
> This is rarely used unless for time of flight sensors.  That is where
> you are controlling the gain on something not directly related to the
> signal you are reading. Here if it affects the read out value such that
> _raw * _scale should include it then use _scale, otherwise should be
> incorporated in calibscale. 
> 
> If it is _scale then the code will be fiddly but you need to present
> the scales that are appropriate for the current integration time.
> 
> It may be worth looking at the GTS helpers that assist with cases
> where there are complex controls interacting around gain of the
> sensor.
> 
> > +		if (val2 != 0)
> > +			return -EINVAL;
> > +		switch (chan->type) {
> > +		case IIO_INTENSITY:
> > +			return apds9160_set_als_gain(data, val);
> > +		case IIO_PROXIMITY:
> > +			return apds9160_set_ps_gain(data, val);
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	case IIO_CHAN_INFO_CALIBSCALE:
> > +		if (val2 != 0)
> > +			return -EINVAL;
> > +		switch (chan->type) {
> > +		case IIO_PROXIMITY:
> > +			return apds9160_set_ps_cancellation_level(data, val);
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	case IIO_CHAN_INFO_CALIBBIAS:
> > +		if (val2 != 0)
> > +			return -EINVAL;
> > +		switch (chan->type) {
> > +		case IIO_PROXIMITY:
> > +			return apds9160_set_ps_analog_cancellation(data, val);
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	case IIO_CHAN_INFO_RAW:
> > +		if (val2 != 0)
> > +			return -EINVAL;
> > +		switch (chan->type) {
> > +		case IIO_CURRENT:
> > +			return apds9160_set_ps_current(data, val);
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static inline int apds9160_get_thres_reg(const struct iio_chan_spec *chan,
> > +					 enum iio_event_direction dir, u8 *reg)
> > +{
> > +	switch (dir) {
> > +	case IIO_EV_DIR_RISING:
> > +		switch (chan->type) {
> > +		case IIO_PROXIMITY:
> > +			*reg = APDS9160_REG_PS_THRES_HI_LSB;
> > +			break;
> > +		case IIO_INTENSITY:
> > +			*reg = APDS9160_REG_LS_THRES_UP_LSB;
> > +			break;
> return from these rather than breaking.
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +		break;
> Then this won't be reachable.
> > +	case IIO_EV_DIR_FALLING:
> > +		switch (chan->type) {
> > +		case IIO_PROXIMITY:
> > +			*reg = APDS9160_REG_PS_THRES_LO_LSB;
> > +			break;
> > +		case IIO_INTENSITY:
> > +			*reg = APDS9160_REG_LS_THRES_LO_LSB;
> > +			break;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> And neither will this.
> > +
> > +	return 0;
> > +}
> > +
> > +static int apds9160_read_event(struct iio_dev *indio_dev,
> > +			       const struct iio_chan_spec *chan,
> > +			       enum iio_event_type type,
> > +			       enum iio_event_direction dir,
> > +			       enum iio_event_info info, int *val, int *val2)
> > +{
> > +	u8 reg;
> > +
> > +	int ret = 0;
> > +	struct apds9160_chip *data = iio_priv(indio_dev);
> > +
> > +	if (info != IIO_EV_INFO_VALUE)
> > +		return -EINVAL;
> > +
> > +	ret = apds9160_get_thres_reg(chan, dir, &reg);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (chan->type == IIO_PROXIMITY) {
> 
> Probably clearer as a switch.
> 
> > +		__le16 buf;
> > +
> > +		ret = regmap_bulk_read(data->regmap, reg, &buf, 2);
> > +		if (ret < 0)
> > +			return ret;
> > +		*val = le16_to_cpu(buf);
> > +	} else if (chan->type == IIO_INTENSITY) {
> > +		__le32 buf = 0;
> > +
> > +		ret = regmap_bulk_read(data->regmap, reg, &buf, 3);
> 
> It's not an __le32 if you are reading only 3 bytes. Use a u8 [3]
> and get_unaligned_le24()
> 
> > +		if (ret < 0)
> > +			return ret;
> > +		*val = le32_to_cpu(buf);
> > +	} else
> > +		return -EINVAL;
> > +
> > +	*val2 = 0;
> No need to set if returning IIO_VAL_INT as won't be used.
> 
> > +
> > +	return IIO_VAL_INT;
> > +}
> > +
> > +static int apds9160_write_event(struct iio_dev *indio_dev,
> > +				const struct iio_chan_spec *chan,
> > +				enum iio_event_type type,
> > +				enum iio_event_direction dir,
> > +				enum iio_event_info info, int val, int val2)
> > +{
> > +	u8 reg;
> > +	int ret = 0;
> > +	struct apds9160_chip *data = iio_priv(indio_dev);
> > +
> > +	if (info != IIO_EV_INFO_VALUE)
> > +		return -EINVAL;
> > +
> > +	ret = apds9160_get_thres_reg(chan, dir, &reg);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (chan->type == IIO_PROXIMITY) {
> > +		if (val < 0 || val > APDS9160_PS_THRES_MAX)
> > +			return -EINVAL;
> > +		__le16 buf;
> > +
> > +		buf = cpu_to_le16(val);
> > +		ret = regmap_bulk_write(data->regmap, reg, &buf, 2);
> Needs to be a dma safe buffer.  In practice this probably won't cause
> a bug but that is down to subtle internals of regmap and there is
> no guarantee that future optimizations won't again require a DMA safe
> buffer.  This what the __aligned(IIO_DMA_MINALIGN) stuff is about.
> 
> > +		if (ret < 0)
> > +			return ret;
> > +	} else if (chan->type == IIO_INTENSITY) {
> > +		if (val < 0 || val > APDS9160_LS_THRES_MAX)
> > +			return -EINVAL;
> > +		__le32 buf = 0;
> 
> Variables should almost always be declared at top of scope or just put
> them at top of function if that is more appropraite.
> 
> > +
> > +		buf = cpu_to_le32(val);
> > +		ret = regmap_bulk_write(data->regmap, reg, &buf, 3);
> > +		if (ret < 0)
> > +			return ret;
> 		return regmap_bulk_write()
> 
> > +	} else
> > +		return -EINVAL;
> Probably neater to use a switch statement and default to cath this last one.
> > +
> > +	return 0;
> > +}
> > +
> > +static int apds9160_read_event_config(struct iio_dev *indio_dev,
> > +				      const struct iio_chan_spec *chan,
> > +				      enum iio_event_type type,
> > +				      enum iio_event_direction dir)
> > +{
> > +	struct apds9160_chip *data = iio_priv(indio_dev);
> > +
> > +	switch (chan->type) {
> > +	case IIO_PROXIMITY:
> > +		return data->ps_int;
> > +	case IIO_INTENSITY:
> > +		return data->als_int;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> Unreachable so drop this.
> 
> > +}
> > +
> > +static int apds9160_write_event_config(struct iio_dev *indio_dev,
> > +				       const struct iio_chan_spec *chan,
> > +				       enum iio_event_type type,
> > +				       enum iio_event_direction dir, int state)
> > +{
> > +	struct apds9160_chip *data = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	state = !!state;
> > +
> > +	switch (chan->type) {
> > +	case IIO_PROXIMITY:
> > +		if (data->ps_int == state)
> > +			return -EINVAL;
> > +
> > +		ret = regmap_field_write(data->reg_int_ps, state);
> > +		if (ret)
> > +			return ret;
> > +		data->ps_int = state;
> > +		break;
> 		return 0;
> > +	case IIO_INTENSITY:
> > +		if (data->als_int == state)
> > +			return -EINVAL;
> > +
> > +		ret = regmap_field_write(data->reg_int_als, state);
> > +		if (ret)
> > +			return ret;
> > +		data->als_int = state;
> > +		break;
> 		return 0; instead of break if nothing else to do.
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> With above changes, this is unreachable so drop it.
> 
> > +}
> > +
> > +static irqreturn_t apds9160_irq_handler(int irq, void *private)
> > +{
> > +	struct iio_dev *indio_dev = private;
> > +	struct apds9160_chip *data = iio_priv(indio_dev);
> > +	int ret, status;
> > +
> > +	/* Reading status register clears the interrupt flag */
> > +	ret = regmap_read(data->regmap, APDS9160_REG_SR, &status);
> > +	if (ret < 0) {
> > +		dev_err(&data->client->dev, "irq status reg read failed\n");
> > +		return IRQ_HANDLED;
> > +	}
> > +
> > +	if ((status & APDS9160_REG_SR_LS_INT) &&
> > +	    (status & APDS9160_REG_SR_LS_NEW_DATA) && data->als_int) {
> > +		iio_push_event(indio_dev,
> > +			       IIO_UNMOD_EVENT_CODE(IIO_INTENSITY, 0,
> > +						    IIO_EV_TYPE_THRESH,
> > +						    IIO_EV_DIR_EITHER),
> > +			       iio_get_time_ns(indio_dev));
> > +	}
> > +
> > +	if ((status & APDS9160_REG_SR_PS_INT) &&
> > +	    (status & APDS9160_REG_SR_PS_NEW_DATA) && data->ps_int) {
> > +		/** Interrupt flag is cleared after data read */
> 
> 
> Not kernel doc. So just /* 
> 
> However, confusing comment to have here and duplicates comment above I think.
> 
> 
> > +		iio_push_event(indio_dev,
> > +			       IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
> > +						    IIO_EV_TYPE_THRESH,
> > +						    IIO_EV_DIR_EITHER),
> > +			       iio_get_time_ns(indio_dev));
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int apds9160_detect(struct apds9160_chip *chip)
> > +{
> > +	struct i2c_client *client = chip->client;
> > +	int ret;
> > +	u32 val;
> > +	u8 id;
> > +	u8 rev;
> > +
> > +	ret = regmap_read(chip->regmap, APDS9160_REG_ID, &val);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "ID read failed\n");
> > +		return ret;
> > +	}
> > +
> > +	id = APDS9160_PART_ID_MASK & ((u8)val);
> FIELD_GET()
> > +	rev = APDS9160_PART_REV_MASK & ((u8)val);
> > +
> > +	switch (id) {
> > +	case APDS9160_PART_ID_0:
> > +		chip->revision = rev;
> > +		dev_info(&client->dev, "Device probed, rev %u\n",
> > +			 chip->revision);
> Too noisy.
> return 0 here.
> 
> > +		break;
> > +	default:
> > +		dev_info(&client->dev, "Unsupported part id %u rev %u\n", id,
> > +			 rev);
> > +		ret = -ENODEV;
> Fine to return an error code from here, just don't fail the probe on the
> basis of an unknown part ID.
> 
> We have some older drivers that do this, but are slowly moving
> to just printing an info message and moving on.
> 
> > +		break;
> > +	}
> > +	return ret;
> > +}
> 
> > +static int apds9160_configure(struct iio_dev *indio_dev)
> > +{
> > +	struct apds9160_chip *chip = iio_priv(indio_dev);
> > +
> > +	return apds9160_chip_init(chip);
> > +}
> > +
> > +static int apds9160_regfield_init(struct apds9160_chip *data)
> > +{
> > +	struct device *dev = &data->client->dev;
> > +	struct regmap *regmap = data->regmap;
> > +
> > +	data->reg_int_als = devm_regmap_field_alloc(dev, regmap,
> > +						    apds9160_reg_field_int_als);
> Can you just use the bulk field allocator?
> 
> > +	if (IS_ERR(data->reg_int_als)) {
> > +		dev_err(dev, "INT ALS reg field init failed\n");
> > +		return PTR_ERR(data->reg_int_als);
> return dev_err_probe() for all of these if only called form probe.
> 
> > +	}
> > +
> > +	data->reg_int_ps =
> > +		devm_regmap_field_alloc(dev, regmap, apds9160_reg_field_int_ps);
> > +	if (IS_ERR(data->reg_int_ps)) {
> > +		dev_err(dev, "INT ps reg field init failed\n");
> > +		return PTR_ERR(data->reg_int_ps);
> > +	}
> > +
> > +	data->reg_enable_als =
> > +		devm_regmap_field_alloc(dev, regmap, apds9160_reg_field_ls_en);
> > +	if (IS_ERR(data->reg_enable_als)) {
> > +		dev_err(dev, "Enable ALS reg field init failed\n");
> > +		return PTR_ERR(data->reg_enable_als);
> > +	}
> > +
> > +	data->reg_enable_ps =
> > +		devm_regmap_field_alloc(dev, regmap, apds9160_reg_field_ps_en);
> > +	if (IS_ERR(data->reg_enable_ps)) {
> > +		dev_err(dev, "Enable PS reg field init failed\n");
> > +		return PTR_ERR(data->reg_enable_ps);
> > +	}
> > +
> > +	data->reg_ps_overflow = devm_regmap_field_alloc(
> > +		dev, regmap, apds9160_reg_field_ps_overflow);
> > +	if (IS_ERR(data->reg_ps_overflow)) {
> > +		dev_err(dev, "PS overflow reg field init failed\n");
> > +		return PTR_ERR(data->reg_ps_overflow);
> > +	}
> > +
> > +	data->reg_als_rate = devm_regmap_field_alloc(
> > +		dev, regmap, apds9160_reg_field_als_rate);
> > +	if (IS_ERR(data->reg_als_rate)) {
> > +		dev_err(dev, "ALS measurement rate field init failed\n");
> > +		return PTR_ERR(data->reg_als_rate);
> > +	}
> > +
> > +	data->reg_als_resolution = devm_regmap_field_alloc(
> > +		dev, regmap, apds9160_reg_field_als_res);
> > +	if (IS_ERR(data->reg_als_resolution)) {
> > +		dev_err(dev, "ALS resolution field init failed\n");
> > +		return PTR_ERR(data->reg_als_resolution);
> > +	}
> > +
> > +	data->reg_ps_rate = devm_regmap_field_alloc(dev, regmap,
> > +						    apds9160_reg_field_ps_rate);
> > +	if (IS_ERR(data->reg_ps_rate)) {
> > +		dev_err(dev, "PS measurement rate field init failed\n");
> > +		return PTR_ERR(data->reg_ps_rate);
> > +	}
> > +
> > +	data->reg_als_gain = devm_regmap_field_alloc(
> > +		dev, regmap, apds9160_reg_field_als_gain);
> > +	if (IS_ERR(data->reg_als_gain)) {
> > +		dev_err(dev, "ALS gain field init failed\n");
> > +		return PTR_ERR(data->reg_als_gain);
> > +	}
> > +
> > +	data->reg_ps_current = devm_regmap_field_alloc(
> > +		dev, regmap, apds9160_reg_field_ps_current);
> > +	if (IS_ERR(data->reg_ps_current)) {
> > +		dev_err(dev, "PS current field init failed\n");
> > +		return PTR_ERR(data->reg_ps_current);
> > +	}
> > +
> > +	data->reg_ps_gain = devm_regmap_field_alloc(dev, regmap,
> > +						    apds9160_reg_field_ps_gain);
> > +	if (IS_ERR(data->reg_ps_gain)) {
> > +		dev_err(dev, "PS gain field init failed\n");
> > +		return PTR_ERR(data->reg_ps_gain);
> > +	}
> > +
> > +	data->reg_ps_resolution = devm_regmap_field_alloc(
> > +		dev, regmap, apds9160_reg_field_ps_resolution);
> > +	if (IS_ERR(data->reg_ps_resolution)) {
> > +		dev_err(dev, "PS resolution field init failed\n");
> > +		return PTR_ERR(data->reg_ps_resolution);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int apds9160_disable(struct apds9160_chip *data)
> > +{
> > +	int ret;
> > +
> > +	ret = regmap_field_write(data->reg_enable_als, 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return regmap_field_write(data->reg_enable_ps, 0);
> > +}
> > +
> > +static int apds9160_shutdown(struct iio_dev *indio_dev)
> > +{
> > +	struct apds9160_chip *data = iio_priv(indio_dev);
> > +
> I think you are using the buffer interfaces just to provide
> an enable / disable?  Don't do that - instead implement runtime
> PM with autosuspend.  That way the sensor will be powered down
> if it isn't accessed for a bit.  When you enable events
> you just hold it on until they are disabled.
> 
> 
> > +	return apds9160_disable(data);
> > +}
> > +
> > +static void apds9160_remove(struct i2c_client *client)
> > +{
> > +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > +
> > +	struct apds9160_chip *data = iio_priv(indio_dev);
> > +
> > +	iio_device_unregister(indio_dev);
> > +	apds9160_disable(data);
> convention is to put this after probe()
> However with changes suggested in probe() you won't need this
> at all.
> 
> > +}
> > +
> > +static const struct iio_buffer_setup_ops apds9160_buffer_setup_ops = {
> > +	.postenable = apds9160_configure,
> > +	.predisable = apds9160_shutdown,
> > +};
> > +
> > +static const struct iio_info apds9160_info = {
> > +	.attrs = &apds9160_attribute_group,
> > +	.read_raw = apds9160_read_raw,
> > +	.write_raw = apds9160_write_raw,
> > +	.read_event_value = apds9160_read_event,
> > +	.write_event_value = apds9160_write_event,
> > +	.read_event_config = apds9160_read_event_config,
> > +	.write_event_config = apds9160_write_event_config,
> > +};
> > +
> > +static int apds9160_probe(struct i2c_client *client)
> > +{
> > +	struct apds9160_chip *chip;
> > +	struct iio_dev *indio_dev;
> > +	int err;
> > +
> > +	dev_info(&client->dev,
> > +		 "Loading proximity/ambient light sensor driver\n");
> Far too noisy. It is easy to find out if the driver has probed from
> other means so we tend not to put generic entries in the kernel log.
> 
> > +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> Can fail. Check for that.
> 
> > +	indio_dev->info = &apds9160_info;
> > +	indio_dev->name = APDS9160_DRIVER_NAME;
> 
> Put it here as a string. There is no particularly reason this (which is
> the part number) should match the driver name.
> 
> > +	indio_dev->channels = apds9160_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(apds9160_channels);
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +	err = devm_iio_kfifo_buffer_setup(&client->dev, indio_dev,
> > +					  &apds9160_buffer_setup_ops);
> > +	if (err)
> > +		return err;
> > +
> > +	chip = iio_priv(indio_dev);
> > +	i2c_set_clientdata(client, indio_dev);
> > +	chip->client = client;
> > +	chip->regmap = devm_regmap_init_i2c(client, &apds9160_regmap_config);
> > +	if (IS_ERR(chip->regmap)) {
> > +		dev_err(&client->dev, "regmap initialization failed.\n");
> > +		return PTR_ERR(chip->regmap);
> 		return dev_err_probe()...
> For all error messages in probe.
> 
> > +	}
> > +
> > +	chip->client = client;
> > +	chip->indio_dev = indio_dev;
> > +	mutex_init(&chip->lock);
> > +
> > +	err = apds9160_detect(chip);
> > +	if (err < 0) {
> Don't fail on this. Fine to print a message though to say we have
> a part we don't recognise.
> 
> Actually failing prevents the use of fallback compatibles in DT
> etc that allow an older kernel to work with new devices as long
> as they are backwards compatible (with exception of the ID register).
> > +		dev_err(&client->dev, "apds9160 not found\n");
> > +		return err;
> > +	}
> > +
> > +	err = apds9160_regfield_init(chip);
> > +	if (err)
> > +		return err;
> > +
> > +	err = apds9160_chip_init(chip);
> > +	if (err)
> > +		return err;
> Register a callback to turn the chip off again here with 
> 	ret = devm_add_action_or_reset()
> 
> That avoids mixing devm and non devm cleanup and allows direct returns.
> 
> > +
> > +	if (client->irq > 0) {
> > +		err = devm_request_threaded_irq(
> > +			&client->dev, client->irq, NULL, apds9160_irq_handler,
> > +			IRQF_TRIGGER_FALLING | IRQF_ONESHOT, "apds9160_event",
> Direction needs to come from firmware so drop the FALLING part.
> 
> > +			indio_dev);
> > +		if (err) {
> > +			dev_err(&client->dev, "request irq (%d) failed\n",
> > +				client->irq);
> > +			goto fail;
> > +		}
> > +	} else {
> > +		dev_info(&client->dev,
> > +			 "init: no IRQ defined, ps/als interrupts disabled\n");
> Easy to find out, so no need for print.
> > +	}
> > +
> > +	err = iio_device_register(indio_dev);
> With above changes make this a devm_call as well.
> > +	if (err)
> > +		goto fail;
> > +
> > +	return 0;
> > +fail:
> > +	apds9160_disable(chip);
> With devm based disable this goes.
> > +	return err;
> > +}
> > +
> > +static const struct of_device_id apds9160_of_match[] = {
> > +	{ .compatible = "avago,apds9160" },
> > +	{ .compatible = "broadmobi,apds9160" },
> > +	{}
> Trivial local formatting thing but in IIO I'm trying to standardize on
> 	{ }
> for these terminator entries.
> 
> > +};
> > +MODULE_DEVICE_TABLE(of, apds9160_of_match);
> > +
> > +static struct i2c_driver apds9160_driver = {
> > +	.driver	  = {
> > +		.name	= APDS9160_DRIVER_NAME,
> > +		.owner = THIS_MODULE,
> > +		.of_match_table = apds9160_of_match,
> > +	},
> > +	.probe    = apds9160_probe,
> > +	.remove	  = apds9160_remove,
> > +	.id_table = apds9160_id,
> > +};

Hi Jonathan,

Thank you for the feedback. I'm currently in the process of integrating the comments from all the reviewers for a rev 2.
However, there's still some things that are not clear for me that I'm not sure on how to handle properly.

First, regarding the integration time/gain/scale parameters. I took a look at the datasheet again as there is a table
provided to get lux/count (scale?) for the ALS sensor depending on gain and integration time. 

It looks like the correlation in the table is almost linear but it's not as there is a loss of precision.
For example, at 1x gain with integration time 100ms the lux/count is 0.819 but at 3x the table is stating 0.269 instead of exepected 0.273.

Is it still possible to use the gts helpers in that case?

Second, regarding the use of the IIO_CHAN_INFO_HARDWAREGAIN channel info.
I took a look at a couple of recent drivers, some use the IIO_CHAN_INFO_SCALE to ajust gain type registers.

In my use case, it feels like the scale is read-only as it is affected by the gain and integration time and both can be set independently
with their respective available values. How should I handle this?

Finally, you mention to use a dma safe buffer when calling regmap_bulk_read. 
I took a look at other recent drivers and I don't see any differences on how they are handling this. 
Could you provide an example of how to ensure the buffer allocated on the stack is dma safe?

Best regards,
Mikael

Re: [PATCH 2/2] iio: light: Add APDS9160 ALS & Proximity sensor driver

[Jonathan Cameron]

> > > +};
> > > +MODULE_DEVICE_TABLE(of, apds9160_of_match);
> > > +
> > > +static struct i2c_driver apds9160_driver = {
> > > +	.driver	  = {
> > > +		.name	= APDS9160_DRIVER_NAME,
> > > +		.owner = THIS_MODULE,
> > > +		.of_match_table = apds9160_of_match,
> > > +	},
> > > +	.probe    = apds9160_probe,
> > > +	.remove	  = apds9160_remove,
> > > +	.id_table = apds9160_id,
> > > +};  
> 
> Hi Jonathan,
> 

Hi Mikael,

One quick process thing first.  Please crop replies. It is far too easy to miss comments
inline (I don't think there were any?) and reviewers aren't fond of scrolling through lots
of context that isn't relevant to a particular discussion.

> Thank you for the feedback. I'm currently in the process of integrating the comments from all the reviewers for a rev 2.
> However, there's still some things that are not clear for me that I'm not sure on how to handle properly.
> 
> First, regarding the integration time/gain/scale parameters. I took a look at the datasheet again as there is a table
> provided to get lux/count (scale?) for the ALS sensor depending on gain and integration time. 
> 
> It looks like the correlation in the table is almost linear but it's not as there is a loss of precision.
> For example, at 1x gain with integration time 100ms the lux/count is 0.819 but at 3x the table is stating 0.269 instead of exepected 0.273.
> 
> Is it still possible to use the gts helpers in that case?

Ah. Probably not if it goes non linear.  Matti? (+CC)

> 
> Second, regarding the use of the IIO_CHAN_INFO_HARDWAREGAIN channel info.
> I took a look at a couple of recent drivers, some use the IIO_CHAN_INFO_SCALE to ajust gain type registers.
> 
> In my use case, it feels like the scale is read-only as it is affected by the gain and integration time and both can be set independently
> with their respective available values. How should I handle this?
The general preference is for the scale to be the primary control. 
For a light sensor assuming the device doesn't support very long integration times, the
trade off is normally set the integration time as high as possible (as that gives lowest
noise) then tune the gain as necessary.

Another model is to let the integration time be controllable and then try and adjust
the gain to keep as close as possible to a requested scale.  Matti has spent more
brain power on this than anyone so I'll over to him for more precise suggestions!

> 
> Finally, you mention to use a dma safe buffer when calling regmap_bulk_read. 
> I took a look at other recent drivers and I don't see any differences on how they are handling this. 
> Could you provide an example of how to ensure the buffer allocated on the stack is dma safe?
Gah. Before going on, I'd failed to notice this was an I2C device and I2C transports
don't need DMA safe buffers (whether or not accessed through regmap). So that comment
was spurious.  Not sure why I thought it was an SPI device :(

Anyhow for future reference:
The only way to do a safe buffer on the stack is to allocate a huge buffer and then
find an appropriate aligned padding.  We simply don't do that.  So reality is you can't use a buffer
on the stack for transfers that require DMA safe buffers.  Either kzalloc the buffer
or look at the many examples where the driver has __aligned(IIO_DMA_MINALIGN) data in iio_priv()
accessed structure.

The regmap case is a little less than clear though.  Last time I checked the reality was
that regmap always bounce buffered anyway as part of it's handling for weird register
formats.  It doesn't need to though and when I asked the maintainer a few years back the
response was that we should continue to use dma safe buffers for bulk transfers if
the underlying transport (e.g. SPI) requires them.

Jonathan



> 
> Best regards,
> Mikael

Re: [PATCH 2/2] iio: light: Add APDS9160 ALS & Proximity sensor driver

[Matti Vaittinen]

Hi Jonathan & Mikael,

On 01/12/2024 15:20, Jonathan Cameron wrote:
> 
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, apds9160_of_match);
>>>> +
>>>> +static struct i2c_driver apds9160_driver = {
>>>> +	.driver	  = {
>>>> +		.name	= APDS9160_DRIVER_NAME,
>>>> +		.owner = THIS_MODULE,
>>>> +		.of_match_table = apds9160_of_match,
>>>> +	},
>>>> +	.probe    = apds9160_probe,
>>>> +	.remove	  = apds9160_remove,
>>>> +	.id_table = apds9160_id,
>>>> +};
>> First, regarding the integration time/gain/scale parameters. I took a look at the datasheet again as there is a table
>> provided to get lux/count (scale?) for the ALS sensor depending on gain and integration time.
>>
>> It looks like the correlation in the table is almost linear but it's not as there is a loss of precision.
>> For example, at 1x gain with integration time 100ms the lux/count is 0.819 but at 3x the table is stating 0.269 instead of exepected 0.273.
>>
>> Is it still possible to use the gts helpers in that case?
> 
> Ah. Probably not if it goes non linear.  Matti? (+CC)

Disclaimer - I didn't go through the patch and I just respond from the 
top of my head :) So, please take my words with a pinch of salt.

AFAIR, it is not required that the impact of integration time is 
_linear_ through the range. The "multiplication factor" can be set for 
each integration time separately. So, it is perfectly Ok to say:

time 1 => multiply by 1
time 2 => multiply by 2
time 10 => multiply by 9 <= not linear, as linear would be 10.
time 15 => multiply by 15

...

The notable limitation of _current_ implementation is that the 
"multiplication factor" needs to be integer. So, this may result loss of 
accuracy.

>> Second, regarding the use of the IIO_CHAN_INFO_HARDWAREGAIN channel info.
>> I took a look at a couple of recent drivers, some use the IIO_CHAN_INFO_SCALE to ajust gain type registers.
>>
>> In my use case, it feels like the scale is read-only as it is affected by the gain and integration time and both can be set independently
>> with their respective available values. How should I handle this?
> The general preference is for the scale to be the primary control.
> For a light sensor assuming the device doesn't support very long integration times, the
> trade off is normally set the integration time as high as possible (as that gives lowest
> noise) then tune the gain as necessary.
> 
> Another model is to let the integration time be controllable and then try and adjust
> the gain to keep as close as possible to a requested scale.  Matti has spent more
> brain power on this than anyone so I'll over to him for more precise suggestions!

This is the very reason why I wrote the gts helpers. The recently 
removed rohm-bu27008.c light-sensor driver and the rohm-bu27034.c allow 
setting integration time. When time is set the driver will also set the 
gain, and does this so that the scale is maintained. (Eg, if gain caused 
by time is doubled by the user request, the hw-gain is halved by the 
driver).

For scale setting I used logic where the scale setting tries to maintain 
the integration time when possible, and only set the hardware gain. 
Ideology is that when the driver allows user to set the time, the user 
is expected to know whether to prefer longer time (and typically better 
accuracy), or faster measurement with more hw-gain.

There are still some corner cases in these drivers where the scale can't 
be maintained when time is set, or time can't be maintained when scale 
is set. AFAIR, These drivers chose to in any case set the requested 
time/scale - which means the userland needs to be prepared to "pick up 
the pieces" and deal with the other entity changing. Other option would 
be to deny such changes, but it would limit the use of the hardware - 
and I hate drivers trying to outsmart the users.

Where this gets really hairy is when there are multiple channels and 
some of the controls can be set per channel, and some are common for all 
channels. As an example, I've faced sensors where gain can be set 
separately for the channels but the time is common for all. Here we may 
end up in situations where part of the channels can compensate the 
changes while others can't. Furthermore, there were situations where 
some of the scales could only be achieved with a specific integration 
times. This can get very confusing for users.

In order to make it somehow tolerable the gts helpers support also 
advertising only scales which can be supported with currently selected 
integration time via the "available_scales". Also, I strongly recommend 
having at least a read-only hardwaregain to help understanding which 
part of the scale is contributed by time, and which by hardwaregain. I 
think this is very helpful when debugging ;)

I know Jonathan (and maybe rest of the world :) ) prefers keeping the 
scale as the single main way of changing the gain. Still, my personal 
opinion on this is that there are cases, where having both the 
hardwaregain and integration time directly changeable would be the 
simplest and clearest solution.

Putting my opinions aside (as this is not a battle I feel like fighting 
- nor do I feel I am the best expert on this!), with the current 'one 
scale to rule them all' approach, my recommendation is to consider if 
always using largest integration time and smallest hardwaregain to 
achieve the requested scale fits your users. (Also, I think your case 
where the impact of integration time is not strictly linear will cause 
some scales to be 'almost same' but not quite. I am not sure if it makes 
sense to support all of these, or just always round to the scale where 
the integration time is longest). If yes, then maybe do it.

If you think the longest times may cause unacceptable wait times for 
some users, or that the small scale differences really matter, then you 
may want to go the route bu27008 / bu27034 drivers took - but it may get 
hairy.

Finally, maybe taking a look if the integration time multiplier in gts 
helpers could support fixed point wouldn't be completely futile(?) No 
promises on that route though, I haven't considered this when writing 
those so it may lead to dead end - but one never knows before taking a 
look, right? :)

Ha. Looking all the letters I typed, I feel I helped really little while 
being very verbose ... Sorry bout that but I really don't have a great 
recommendation :(

Yours,
	-- Matti

Re: [PATCH 2/2] iio: light: Add APDS9160 ALS & Proximity sensor driver

[Matti Vaittinen]

On 02/12/2024 10:22, Matti Vaittinen wrote:
> Hi Jonathan & Mikael,
> 
> On 01/12/2024 15:20, Jonathan Cameron wrote:
>>
>>>>> +};
>>>>> +MODULE_DEVICE_TABLE(of, apds9160_of_match);
>>>>> +
>>>>> +static struct i2c_driver apds9160_driver = {
>>>>> +    .driver      = {
>>>>> +        .name    = APDS9160_DRIVER_NAME,
>>>>> +        .owner = THIS_MODULE,
>>>>> +        .of_match_table = apds9160_of_match,
>>>>> +    },
>>>>> +    .probe    = apds9160_probe,
>>>>> +    .remove      = apds9160_remove,
>>>>> +    .id_table = apds9160_id,
>>>>> +};
>>> First, regarding the integration time/gain/scale parameters. I took a 
>>> look at the datasheet again as there is a table
>>> provided to get lux/count (scale?) for the ALS sensor depending on 
>>> gain and integration time.
>>>
>>> It looks like the correlation in the table is almost linear but it's 
>>> not as there is a loss of precision.
>>> For example, at 1x gain with integration time 100ms the lux/count is 
>>> 0.819 but at 3x the table is stating 0.269 instead of exepected 0.273.
>>>
>>> Is it still possible to use the gts helpers in that case?
>>
>> Ah. Probably not if it goes non linear.  Matti? (+CC)
> 
> Disclaimer - I didn't go through the patch and I just respond from the 
> top of my head :) So, please take my words with a pinch of salt.
> 
> AFAIR, it is not required that the impact of integration time is 
> _linear_ through the range. The "multiplication factor" can be set for 
> each integration time separately. So, it is perfectly Ok to say:
> 
> time 1 => multiply by 1
> time 2 => multiply by 2
> time 10 => multiply by 9 <= not linear, as linear would be 10.
> time 15 => multiply by 15
> 
> ...
> 
> The notable limitation of _current_ implementation is that the 
> "multiplication factor" needs to be integer. So, this may result loss of 
> accuracy.

// Snip.

I ended up re-reading this mail as a result of running some of my 
public-inbox scripts...

...and I noticed that the non linear correlation was not about 
integration time, but about gain. Eg, if I now read you right, the 
integration time is kept constant 100mS, and gain is changed from 1x => 
3x, which actually did not bring 3x gain to the lux/count values.

If this is the case, then the GTS helpers aren't likely to help you 
much. Sorry.


Yours,
	-- Matti

Re: [PATCH 2/2] iio: light: Add APDS9160 ALS & Proximity sensor driver

[Mikael Gonella-Bolduc]

On Thu, Dec 05, 2024 at 11:42:21AM +0200, Matti Vaittinen wrote:
> On 02/12/2024 10:22, Matti Vaittinen wrote:
> > Hi Jonathan & Mikael,
> > 
> > On 01/12/2024 15:20, Jonathan Cameron wrote:
> > > 
> > > > > > +};
> > > > > > +MODULE_DEVICE_TABLE(of, apds9160_of_match);
> > > > > > +
> > > > > > +static struct i2c_driver apds9160_driver = {
> > > > > > +    .driver      = {
> > > > > > +        .name    = APDS9160_DRIVER_NAME,
> > > > > > +        .owner = THIS_MODULE,
> > > > > > +        .of_match_table = apds9160_of_match,
> > > > > > +    },
> > > > > > +    .probe    = apds9160_probe,
> > > > > > +    .remove      = apds9160_remove,
> > > > > > +    .id_table = apds9160_id,
> > > > > > +};
> > > > First, regarding the integration time/gain/scale parameters. I
> > > > took a look at the datasheet again as there is a table
> > > > provided to get lux/count (scale?) for the ALS sensor depending
> > > > on gain and integration time.
> > > > 
> > > > It looks like the correlation in the table is almost linear but
> > > > it's not as there is a loss of precision.
> > > > For example, at 1x gain with integration time 100ms the
> > > > lux/count is 0.819 but at 3x the table is stating 0.269 instead
> > > > of exepected 0.273.
> > > > 
> > > > Is it still possible to use the gts helpers in that case?
> > > 
> > > Ah. Probably not if it goes non linear.  Matti? (+CC)
> > 
> > Disclaimer - I didn't go through the patch and I just respond from the
> > top of my head :) So, please take my words with a pinch of salt.
> > 
> > AFAIR, it is not required that the impact of integration time is
> > _linear_ through the range. The "multiplication factor" can be set for
> > each integration time separately. So, it is perfectly Ok to say:
> > 
> > time 1 => multiply by 1
> > time 2 => multiply by 2
> > time 10 => multiply by 9 <= not linear, as linear would be 10.
> > time 15 => multiply by 15
> > 
> > ...
> > 
> > The notable limitation of _current_ implementation is that the
> > "multiplication factor" needs to be integer. So, this may result loss of
> > accuracy.
> 
> // Snip.
> 
> I ended up re-reading this mail as a result of running some of my
> public-inbox scripts...
> 
> ...and I noticed that the non linear correlation was not about integration
> time, but about gain. Eg, if I now read you right, the integration time is
> kept constant 100mS, and gain is changed from 1x => 3x, which actually did
> not bring 3x gain to the lux/count values.
> 
> If this is the case, then the GTS helpers aren't likely to help you much.
> Sorry.
> 
> 
> Yours,
> 	-- Matti

Hi,

Thank you for the feedback.
The datasheet is not very clear on why exactly the correlation is almost linear.
I assume it is probably to compensate for some inaccuracies and that's why they
provide the table instead of a linear formula.

I took the approach to let the user control the integration time and I ajust
the available scales depending on the selected integration time.

I kept the hardware gain read-only, as you suggested, for debugging purposes.

See changes in v2.

Best regards,
Mikael