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

[PATCH v2 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>
---
Changes in v2:
- Rebased on linux-iio 20fd1383
- dt-bindings: Dropped the old Avago name and use the brcm vendor prefix
- dt-bindings: Updated example node name with a generic name
- dt-bindings: Updated example indentation to 4 spaces
- dt-bindings: Fixed element ordering
- KConfig: Dropped unsure sentences
- KConfig: Dropped unused Kfifo buffer selection
- driver: Use a more recent iio light driver as template
- driver: Remove buffer declaration
- driver: Use avail functions instead of custom iio attributes
- driver: Use scale instead of hardware gain
- driver: Removed unused members and unreachable statements
- driver: Removed unnecessary info and debug prints
- driver: Fix some coding style and line wrapping issues
- driver: Reordering of functions
- Link to v1: https://lore.kernel.org/r/20241119-apds9160-driver-v1-0-fa00675b4ea4@dimonoff.com

---
Mikael Gonella-Bolduc (2):
      dt-bindings: iio: light: Add APDS9160 binding
      iio: light: Add APDS9160 ALS & Proximity sensor driver

 .../bindings/iio/light/brcm,apds9160.yaml          |   51 +
 MAINTAINERS                                        |    7 +
 drivers/iio/light/Kconfig                          |   11 +
 drivers/iio/light/Makefile                         |    1 +
 drivers/iio/light/apds9160.c                       | 1548 ++++++++++++++++++++
 5 files changed, 1618 insertions(+)
---
base-commit: 5de07b8a24cf44cdb78adeab790704bf577c2c1d
change-id: 20241119-apds9160-driver-86cf5e86de35

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

[PATCH v2 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/brcm,apds9160.yaml          | 51 ++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/light/brcm,apds9160.yaml b/Documentation/devicetree/bindings/iio/light/brcm,apds9160.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..525fba52f156df3b78e24d7d0d445fe9d882eaa7
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/brcm,apds9160.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/brcm,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:
+      - brcm,apds9160
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  vdd-supply: true
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        light-sensor@53 {
+            compatible = "brcm,apds9160";
+            reg = <0x53>;
+            vdd-supply = <&vdd_reg>;
+            interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
+            interrupt-parent = <&pinctrl>;
+        };
+    };
+...

-- 
2.34.1

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

[Conor Dooley]

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

On Fri, Dec 06, 2024 at 11:09:56AM -0500, Mikael Gonella-Bolduc via B4 Relay wrote:
> From: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
> 
> Add device tree bindings for APDS9160 driver

Bindings are for hardware, not for drivers.

> 
> Signed-off-by: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
> ---
>  .../bindings/iio/light/brcm,apds9160.yaml          | 51 ++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/brcm,apds9160.yaml b/Documentation/devicetree/bindings/iio/light/brcm,apds9160.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..525fba52f156df3b78e24d7d0d445fe9d882eaa7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/brcm,apds9160.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/light/brcm,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>

How come this differs from your author email?

> +
> +description: |
> +  Datasheet: https://docs.broadcom.com/docs/APDS-9160-003-DS
> +
> +properties:
> +  compatible:
> +    enum:
> +      - brcm,apds9160
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  vdd-supply: true
> +
> +required:
> +  - compatible
> +  - reg
> +  - vdd-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        light-sensor@53 {
> +            compatible = "brcm,apds9160";
> +            reg = <0x53>;
> +            vdd-supply = <&vdd_reg>;
> +            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 v2 1/2] dt-bindings: iio: light: Add APDS9160 binding

[Mikael Gonella-Bolduc]

Hi Conor,

See my comments inline.

Thank you,
Mikael

On Fri, Dec 06, 2024 at 04:33:36PM +0000, Conor Dooley wrote:
> On Fri, Dec 06, 2024 at 11:09:56AM -0500, Mikael Gonella-Bolduc via B4 Relay wrote:
> > From: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
> > 
> > Add device tree bindings for APDS9160 driver
> 
> Bindings are for hardware, not for drivers.
> 

Indeed, should I change the commit message to remove the "driver" part?

> > 
> > Signed-off-by: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
> > ---
> >  .../bindings/iio/light/brcm,apds9160.yaml          | 51 ++++++++++++++++++++++
> >  1 file changed, 51 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/light/brcm,apds9160.yaml b/Documentation/devicetree/bindings/iio/light/brcm,apds9160.yaml
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..525fba52f156df3b78e24d7d0d445fe9d882eaa7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/light/brcm,apds9160.yaml
> > @@ -0,0 +1,51 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/light/brcm,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>
> 
> How come this differs from your author email?
> 

The author email is my work email, this one is my personal email.
The first one might change while the other one will not.
Is it required to have the same email here?

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

[Jonathan Cameron]

On Fri, 6 Dec 2024 15:03:17 -0500
Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com> wrote:

> Hi Conor,
> 
> See my comments inline.
> 
> Thank you,
> Mikael
> 
> On Fri, Dec 06, 2024 at 04:33:36PM +0000, Conor Dooley wrote:
> > On Fri, Dec 06, 2024 at 11:09:56AM -0500, Mikael Gonella-Bolduc via B4 Relay wrote:  
> > > From: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
> > > 
> > > Add device tree bindings for APDS9160 driver  
> > 
> > Bindings are for hardware, not for drivers.
> >   
> 
> Indeed, should I change the commit message to remove the "driver" part?
Yes.
> 
> > > 
> > > Signed-off-by: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
> > > ---
> > >  .../bindings/iio/light/brcm,apds9160.yaml          | 51 ++++++++++++++++++++++
> > >  1 file changed, 51 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/iio/light/brcm,apds9160.yaml b/Documentation/devicetree/bindings/iio/light/brcm,apds9160.yaml
> > > new file mode 100644
> > > index 0000000000000000000000000000000000000000..525fba52f156df3b78e24d7d0d445fe9d882eaa7
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/light/brcm,apds9160.yaml
> > > @@ -0,0 +1,51 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/iio/light/brcm,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>  
> > 
> > How come this differs from your author email?
> >   
> 
> The author email is my work email, this one is my personal email.
> The first one might change while the other one will not.
> Is it required to have the same email here?
I don' think there are firm rules on this, but there are processes in place
for changing email via .mailmap and where relevant patches updating
to a new email address.  A note in the commit message would be appropriate though.

Jonathan

> 

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

[Conor Dooley]

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

On Sun, Dec 08, 2024 at 11:41:41AM +0000, Jonathan Cameron wrote:
> On Fri, 6 Dec 2024 15:03:17 -0500
> Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com> wrote:
> 
> > Hi Conor,
> > 
> > See my comments inline.
> > 
> > Thank you,
> > Mikael
> > 
> > On Fri, Dec 06, 2024 at 04:33:36PM +0000, Conor Dooley wrote:
> > > On Fri, Dec 06, 2024 at 11:09:56AM -0500, Mikael Gonella-Bolduc via B4 Relay wrote:  
> > > > From: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
> > > > 
> > > > Add device tree bindings for APDS9160 driver  
> > > 
> > > Bindings are for hardware, not for drivers.
> > >   
> > 
> > Indeed, should I change the commit message to remove the "driver" part?
> Yes.
> > 
> > > > 
> > > > Signed-off-by: Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com>
> > > > ---
> > > >  .../bindings/iio/light/brcm,apds9160.yaml          | 51 ++++++++++++++++++++++
> > > >  1 file changed, 51 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/iio/light/brcm,apds9160.yaml b/Documentation/devicetree/bindings/iio/light/brcm,apds9160.yaml
> > > > new file mode 100644
> > > > index 0000000000000000000000000000000000000000..525fba52f156df3b78e24d7d0d445fe9d882eaa7
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/iio/light/brcm,apds9160.yaml
> > > > @@ -0,0 +1,51 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/iio/light/brcm,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>  
> > > 
> > > How come this differs from your author email?
> > >   
> > 
> > The author email is my work email, this one is my personal email.
> > The first one might change while the other one will not.
> > Is it required to have the same email here?
> I don' think there are firm rules on this, but there are processes in place
> for changing email via .mailmap and where relevant patches updating
> to a new email address.  A note in the commit message would be appropriate though.

Ye, I wasn't intending to raise an objection, just checking if the
difference was intentional.

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

[PATCH v2 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    |   11 +
 drivers/iio/light/Makefile   |    1 +
 drivers/iio/light/apds9160.c | 1548 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 1567 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index e69d1632c382fe0e366f7bb20e72ee0c9e91e30b..99903578e2a31dd946bd5f6722eb2aabc349a090 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3714,6 +3714,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 29ffa84919273d64b8464ab8bbf59661b0102f97..419360661d53973eda71d7d986ff7fd481c7aa2c 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -63,6 +63,17 @@ 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 sensor"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	   Say Y here if you want to build support for a Broadcom APDS9160
+		combined ambient light and proximity sensor.
+
+	   To compile this driver as a module, choose M here: the
+	   module will be called apds9160.
+
 config APDS9300
 	tristate "APDS9300 ambient light sensor"
 	depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index f14a3744271242f04fe7849b47308397ac2c9939..4a22043acdbbbed2b696a3225e4024654d5d9339 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..1cc639c31ebfef7254bf967e56a25a75f99ab05c
--- /dev/null
+++ b/drivers/iio/light/apds9160.c
@@ -0,0 +1,1548 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * APDS9160 sensor driver.
+ * Chip is combined proximity and ambient light sensor.
+ * Author: 2024 Mikael Gonella-Bolduc <m.gonella.bolduc@gmail.com>
+ */
+
+#include <linux/bits.h>
+#include <linux/cleanup.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/types.h>
+#include <linux/units.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/events.h>
+#include <linux/iio/sysfs.h>
+
+#include <linux/unaligned.h>
+
+#define APDS9160_REGMAP_NAME "apds9160_regmap"
+#define APDS9160_REG_CNT 37
+
+/* Main control register */
+#define APDS9160_REG_CTRL 0x00
+#define APDS9160_CTRL_SWRESET BIT(4) /* 1: Activate reset */
+#define APDS9160_CTRL_MODE_RGB BIT(2) /* 0: ALS & IR, 1: RGB & IR */
+#define APDS9160_CTRL_EN_ALS BIT(1) /* 1: ALS active */
+#define APDS9160_CTLR_EN_PS BIT(0) /* 1: PS active */
+
+/* Status register  */
+#define APDS9160_SR_LS_INT BIT(4)
+#define APDS9160_SR_LS_NEW_DATA BIT(3)
+#define APDS9160_SR_PS_INT BIT(1)
+#define APDS9160_SR_PS_NEW_DATA BIT(0)
+
+/* Interrupt configuration registers */
+#define APDS9160_REG_INT_CFG 0x19
+#define APDS9160_REG_INT_PST 0x1A
+#define APDS9160_INT_CFG_EN_LS BIT(2) /* LS int enable */
+#define APDS9160_INT_CFG_EN_PS BIT(0) /* PS int enable */
+
+/* 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_SR_DATA_ALS BIT(3)
+#define APDS9160_SR_DATA_PS BIT(0)
+
+/* Supported ID:s */
+#define APDS9160_PART_ID_0 0x03
+
+#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_11BITS 0x03
+
+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 */
+	{ APDS9160_REG_PS_PULSES, 0x08 }, /* 8 pulses */
+	{ APDS9160_REG_PS_MEAS_RATE, 0x05 }, /* 100ms */
+	{ APDS9160_REG_LS_MEAS_RATE, 0x22 }, /* 100ms */
+	{ APDS9160_REG_LS_GAIN, 0x01 }, /* 3x */
+	{ APDS9160_REG_INT_CFG, 0x10 }, /* Interrupts disabled */
+	{ 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 },
+};
+
+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_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,
+		.dir = IIO_EV_DIR_EITHER,
+		.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),
+		.info_mask_separate_available = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
+						BIT(IIO_CHAN_INFO_HARDWAREGAIN),
+		.event_spec = apds9160_event_spec,
+		.num_event_specs = ARRAY_SIZE(apds9160_event_spec),
+	},
+	{
+		/* Proximity sensor led current */
+		.type = IIO_CURRENT,
+		.output = 1,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_CALIBBIAS),
+		.info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW),
+	},
+	{
+		/* 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_INT_TIME) |
+					    BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
+					    BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME) |
+						BIT(IIO_CHAN_INFO_SCALE),
+		.event_spec = apds9160_event_spec,
+		.num_event_specs = ARRAY_SIZE(apds9160_event_spec),
+	},
+	{
+		/* Clear channel */
+		.type = IIO_INTENSITY,
+		.address = APDS9160_REG_LS_DATA_CLEAR_LSB,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.channel2 = IIO_MOD_LIGHT_CLEAR,
+		.modified = 1,
+	},
+};
+
+/* Attributes */
+static const int apds9160_als_rate_avail[] = {
+	25, 50, 100, 200
+};
+
+static const int apds9160_als_rate_map[][2] = {
+	{ 25, 0x00 },
+	{ 50, 0x01 },
+	{ 100, 0x02 },
+	{ 200, 0x03 },
+};
+
+static const int apds9160_als_gain_avail[] = {
+	1, 3, 6, 18, 54
+};
+
+static const int apds9160_als_gain_map[][2] = {
+	{ 1, 0x00 },
+	{ 3, 0x01 },
+	{ 6, 0x02 },
+	{ 18, 0x03 },
+	{ 54, 0x04 },
+};
+
+static const int apds9160_ps_gain_avail[] = {
+	1, 2, 4, 8
+};
+
+static const int apds9160_ps_gain_map[][2] = {
+	{ 1, 0x00 },
+	{ 2, 0x01 },
+	{ 4, 0x02 },
+	{ 8, 0x03 },
+};
+
+static const int apds9160_ps_rate_avail[] = {
+	25, 50, 100, 200, 400
+};
+
+static const int apds9160_ps_rate_map[][2] = {
+	{ 25, 0x03 },
+	{ 50, 0x04 },
+	{ 100, 0x05 },
+	{ 200, 0x06 },
+	{ 400, 0x07 },
+};
+
+static const int apds9160_ps_led_current_avail[] = {
+	10, 25, 50, 100, 150, 175, 200
+};
+
+static const int apds9160_ps_led_current_map[][2] = {
+	{ 10, 0x00 },
+	{ 25, 0x01 },
+	{ 50, 0x02 },
+	{ 100, 0x03 },
+	{ 150, 0x04 },
+	{ 175, 0x05 },
+	{ 200, 0x06 },
+};
+
+/*
+ * @itime: Integration time in ms
+ * @gain: Gain multiplier
+ * @scale1: lux/count resolution
+ * @scale2: micro lux/count
+ */
+struct apds9160_scale {
+	int itime;
+	int gain;
+	int scale1;
+	int scale2;
+};
+
+/*
+ * Scale mapping extracted from datasheet
+ */
+static const struct apds9160_scale apds9160_als_scale_map[] = {
+	{
+		.itime = 25,
+		.gain = 1,
+		.scale1 = 3,
+		.scale2 = 272000,
+	},
+	{
+		.itime = 25,
+		.gain = 3,
+		.scale1 = 1,
+		.scale2 = 77000,
+	},
+	{
+		.itime = 25,
+		.gain = 6,
+		.scale1 = 0,
+		.scale2 = 525000,
+	},
+	{
+		.itime = 25,
+		.gain = 18,
+		.scale1 = 0,
+		.scale2 = 169000,
+	},
+	{
+		.itime = 25,
+		.gain = 54,
+		.scale1 = 0,
+		.scale2 = 49000,
+	},
+	{
+		.itime = 50,
+		.gain = 1,
+		.scale1 = 1,
+		.scale2 = 639000,
+	},
+	{
+		.itime = 50,
+		.gain = 3,
+		.scale1 = 0,
+		.scale2 = 538000,
+	},
+	{
+		.itime = 50,
+		.gain = 6,
+		.scale1 = 0,
+		.scale2 = 263000,
+	},
+	{
+		.itime = 50,
+		.gain = 18,
+		.scale1 = 0,
+		.scale2 = 84000,
+	},
+	{
+		.itime = 50,
+		.gain = 54,
+		.scale1 = 0,
+		.scale2 = 25000,
+	},
+	{
+		.itime = 100,
+		.gain = 1,
+		.scale1 = 0,
+		.scale2 = 819000,
+	},
+	{
+		.itime = 100,
+		.gain = 3,
+		.scale1 = 0,
+		.scale2 = 269000,
+	},
+	{
+		.itime = 100,
+		.gain = 6,
+		.scale1 = 0,
+		.scale2 = 131000,
+	},
+	{
+		.itime = 100,
+		.gain = 18,
+		.scale1 = 0,
+		.scale2 = 42000,
+	},
+	{
+		.itime = 100,
+		.gain = 54,
+		.scale1 = 0,
+		.scale2 = 12000,
+	},
+	{
+		.itime = 200,
+		.gain = 1,
+		.scale1 = 0,
+		.scale2 = 409000,
+	},
+	{
+		.itime = 200,
+		.gain = 3,
+		.scale1 = 0,
+		.scale2 = 135000,
+	},
+	{
+		.itime = 200,
+		.gain = 6,
+		.scale1 = 0,
+		.scale2 = 66000,
+	},
+	{
+		.itime = 200,
+		.gain = 18,
+		.scale1 = 0,
+		.scale2 = 21000,
+	},
+	{
+		.itime = 200,
+		.gain = 54,
+		.scale1 = 0,
+		.scale2 = 6000,
+	},
+};
+
+static const int apds9160_25ms_avail[][2] = {
+	{ 3, 272000 },
+	{ 1, 77000 },
+	{ 0, 525000 },
+	{ 0, 169000 },
+	{ 0, 49000 },
+};
+
+static const int apds9160_50ms_avail[][2] = {
+	{ 1, 639000 },
+	{ 0, 538000 },
+	{ 0, 263000 },
+	{ 0, 84000 },
+	{ 0, 25000 },
+};
+
+static const int apds9160_100ms_avail[][2] = {
+	{ 0, 819000 },
+	{ 0, 269000 },
+	{ 0, 131000 },
+	{ 0, 42000 },
+	{ 0, 12000 },
+};
+
+static const int apds9160_200ms_avail[][2] = {
+	{ 0, 409000 },
+	{ 0, 135000 },
+	{ 0, 66000 },
+	{ 0, 21000 },
+	{ 0, 6000 },
+};
+
+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 regmap *regmap;
+
+	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;
+
+	struct mutex lock; /* protects state and config data */
+
+	/* State data */
+	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_cancellation_current;
+	int ps_current;
+	int ps_gain;
+};
+
+static int apds9160_set_ps_rate(struct apds9160_chip *data, int val)
+{
+	int ret = -EINVAL;
+	int idx;
+
+	for (idx = 0; idx < ARRAY_SIZE(apds9160_ps_rate_map); idx++) {
+		if (apds9160_ps_rate_map[idx][0] == val) {
+			ret = regmap_field_write(data->reg_ps_rate,
+						 apds9160_ps_rate_map[idx][1]);
+			if (ret)
+				return ret;
+
+			data->ps_rate = val;
+			break;
+		}
+	}
+
+	return ret;
+}
+
+static int apds9160_set_ps_gain(struct apds9160_chip *data, int val)
+{
+	int ret = -EINVAL;
+	int idx;
+
+	for (idx = 0; idx < ARRAY_SIZE(apds9160_ps_gain_map); idx++) {
+		if (apds9160_ps_gain_map[idx][0] == val) {
+			ret = regmap_field_write(data->reg_ps_gain,
+						 apds9160_ps_gain_map[idx][1]);
+			if (ret)
+				return ret;
+
+			data->ps_gain = val;
+			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;
+	__le16 buf;
+
+	if (val < 0 || val > 0xFFFF)
+		return -EINVAL;
+
+	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;
+
+	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
+ * A duration value of 0 or 1 will not generate any cancellation pulse
+ */
+static int apds9160_set_ps_analog_cancellation(struct apds9160_chip *data,
+					       int val)
+{
+	int ret;
+
+	if (val < 0 || val > 0x7F)
+		return -EINVAL;
+
+	ret = regmap_write(data->regmap, APDS9160_REG_PS_CAN_LEVEL_ANA_DUR,
+			   val);
+	if (!ret)
+		data->ps_cancellation_analog = val;
+
+	return ret;
+}
+
+/*
+ * This parameter works in conjunction with the cancellation pulse duration
+ * The value determines the current used for crosstalk cancellation
+ * B4-B5: The coarse value defines cancellation current in steps of 60nA
+ * B0-B3: The fine value adjusts the cancellation current in steps of 2.4nA
+ */
+static int apds9160_set_ps_cancellation_current(struct apds9160_chip *data,
+						int val)
+{
+	int ret;
+
+	if (val < 0 || val > 0x3F)
+		return -EINVAL;
+
+	ret = regmap_write(data->regmap, APDS9160_REG_PS_CAN_LEVEL_ANA_CURRENT,
+			   val);
+	if (!ret)
+		data->ps_cancellation_current = val;
+
+	return ret;
+}
+
+static int apds9160_set_ps_current(struct apds9160_chip *data, int val)
+{
+	int ret = -EINVAL;
+	int idx;
+
+	for (idx = 0; idx < ARRAY_SIZE(apds9160_ps_led_current_map); idx++) {
+		if (apds9160_ps_led_current_map[idx][0] == val) {
+			ret = regmap_field_write(
+				data->reg_ps_current,
+				apds9160_ps_led_current_map[idx][1]);
+
+			if (ret)
+				return ret;
+
+			data->ps_current = val;
+			break;
+		}
+	}
+
+	return ret;
+}
+
+static int apds9160_set_als_gain(struct apds9160_chip *data, int gain)
+{
+	int ret = -EINVAL;
+	int idx;
+
+	for (idx = 0; idx < ARRAY_SIZE(apds9160_als_gain_map); idx++) {
+		if (gain == apds9160_als_gain_map[idx][0]) {
+			ret = regmap_field_write(data->reg_als_gain,
+					apds9160_als_gain_map[idx][1]);
+			if (ret)
+				return ret;
+
+			data->als_hwgain = gain;
+			break;
+		}
+	}
+
+	return ret;
+}
+
+static int apds9160_set_als_scale(struct apds9160_chip *data, int val, int val2)
+{
+	int ret = -EINVAL;
+	int idx;
+
+	for (idx = 0; idx < ARRAY_SIZE(apds9160_als_scale_map); idx++) {
+		if (apds9160_als_scale_map[idx].itime == data->als_itime &&
+		    apds9160_als_scale_map[idx].scale1 == val &&
+		    apds9160_als_scale_map[idx].scale2 == val2) {
+			ret = apds9160_set_als_gain(data,
+					apds9160_als_scale_map[idx].gain);
+			if (ret)
+				return ret;
+
+			data->als_scale1 = val;
+			data->als_scale2 = val2;
+			break;
+		}
+	}
+
+	return ret;
+}
+
+static int apds9160_set_als_resolution(struct apds9160_chip *data, int val)
+{
+	switch (val) {
+	case 25:
+		return regmap_field_write(data->reg_als_resolution,
+			APDS9160_CMD_LS_RESOLUTION_25MS);
+	case 50:
+		return regmap_field_write(data->reg_als_resolution,
+			APDS9160_CMD_LS_RESOLUTION_50MS);
+	case 200:
+		return regmap_field_write(data->reg_als_resolution,
+			APDS9160_CMD_LS_RESOLUTION_200MS);
+	default:
+		return regmap_field_write(data->reg_als_resolution,
+			APDS9160_CMD_LS_RESOLUTION_100MS);
+	}
+}
+
+static int apds9160_set_als_rate(struct apds9160_chip *data, int val)
+{
+	int idx;
+
+	for (idx = 0; idx < ARRAY_SIZE(apds9160_als_rate_map); idx++) {
+		if (apds9160_als_rate_map[idx][0] == val) {
+			return regmap_field_write(data->reg_als_rate,
+					apds9160_als_rate_map[idx][1]);
+		}
+	}
+
+	return -EINVAL;
+}
+
+/*
+ *	Setting the integration time ajusts resolution, rate, scale and gain
+ */
+static int apds9160_set_als_int_time(struct apds9160_chip *data, int val)
+{
+	int ret = -EINVAL;
+	int idx;
+
+	ret = apds9160_set_als_rate(data, val);
+	if (ret)
+		return ret;
+
+	/* Match resolution register with rate */
+	ret = apds9160_set_als_resolution(data, val);
+	if (ret)
+		return ret;
+
+	data->als_itime = val;
+
+	/* Set the scale minimum gain */
+	for (idx = 0; idx < ARRAY_SIZE(apds9160_als_scale_map); idx++) {
+		if (data->als_itime == apds9160_als_scale_map[idx].itime) {
+			ret = apds9160_set_als_scale(data,
+					apds9160_als_scale_map[idx].scale1,
+					apds9160_als_scale_map[idx].scale2);
+			break;
+		}
+	}
+
+	return ret;
+}
+
+static int apds9160_read_avail(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan,
+			       const int **vals, int *type, int *length,
+			       long mask)
+{
+	struct apds9160_chip *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		switch (chan->type) {
+		case IIO_LIGHT:
+			*length = ARRAY_SIZE(apds9160_als_rate_avail);
+			*vals = (const int *)apds9160_als_rate_avail;
+			*type = IIO_VAL_INT;
+			return IIO_AVAIL_LIST;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_LIGHT:
+			/* The available scales changes depending on itime */
+			switch (data->als_itime) {
+			case 25:
+				*length = ARRAY_SIZE(apds9160_25ms_avail) * 2;
+				*vals = (const int *) apds9160_25ms_avail;
+				*type = IIO_VAL_INT_PLUS_MICRO;
+				return IIO_AVAIL_LIST;
+			case 50:
+				*length = ARRAY_SIZE(apds9160_50ms_avail) * 2;
+				*vals = (const int *) apds9160_50ms_avail;
+				*type = IIO_VAL_INT_PLUS_MICRO;
+				return IIO_AVAIL_LIST;
+			case 100:
+				*length = ARRAY_SIZE(apds9160_100ms_avail) * 2;
+				*vals = (const int *) apds9160_100ms_avail;
+				*type = IIO_VAL_INT_PLUS_MICRO;
+				return IIO_AVAIL_LIST;
+			case 200:
+				*length = ARRAY_SIZE(apds9160_200ms_avail) * 2;
+				*vals = (const int *) apds9160_200ms_avail;
+				*type = IIO_VAL_INT_PLUS_MICRO;
+				return IIO_AVAIL_LIST;
+			default:
+				return -EINVAL;
+			}
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		switch (chan->type) {
+		case IIO_PROXIMITY:
+			*length = ARRAY_SIZE(apds9160_ps_rate_avail);
+			*vals = (const int *)apds9160_ps_rate_avail;
+			*type = IIO_VAL_INT;
+			return IIO_AVAIL_LIST;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		switch (chan->type) {
+		case IIO_PROXIMITY:
+			*length = ARRAY_SIZE(apds9160_ps_gain_avail);
+			*vals = (const int *)apds9160_ps_gain_avail;
+			*type = IIO_VAL_INT;
+			return IIO_AVAIL_LIST;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_RAW:
+		switch (chan->type) {
+		case IIO_CURRENT:
+			*length = ARRAY_SIZE(apds9160_ps_led_current_avail);
+			*vals = (const int *)apds9160_ps_led_current_avail;
+			*type = IIO_VAL_INT;
+			return IIO_AVAIL_LIST;
+		default:
+			return -EINVAL;
+		}
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int apds9160_write_raw_get_fmt(struct iio_dev *indio_dev,
+				      struct iio_chan_spec const *chan,
+				      long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_RAW:
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		return -EINVAL;
+	}
+}
+
+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)
+				return ret;
+
+			ret = IIO_VAL_INT;
+			*val = le16_to_cpu(buf);
+			/* Remove overflow bits from result */
+			*val = FIELD_GET(APDS9160_PS_DATA_MASK, *val);
+		} break;
+		case IIO_LIGHT:
+		case IIO_INTENSITY: {
+			u8 buf[3];
+
+			ret = regmap_bulk_read(data->regmap, chan->address,
+					       &buf, 3);
+			if (ret)
+				return ret;
+
+			ret = IIO_VAL_INT;
+			*val = get_unaligned_le24(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_LIGHT:
+			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_LIGHT:
+			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;
+		case IIO_CURRENT:
+			ret = IIO_VAL_INT;
+			*val = data->ps_cancellation_current;
+		default:
+			break;
+		}
+		break;
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_LIGHT:
+			ret = IIO_VAL_INT_PLUS_MICRO;
+			*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);
+
+	guard(mutex)(&data->lock);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		if (val2 != 0)
+			return -EINVAL;
+		switch (chan->type) {
+		case IIO_LIGHT:
+			return apds9160_set_als_int_time(data, val);
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_LIGHT:
+			return apds9160_set_als_scale(data, val, val2);
+		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_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);
+		case IIO_CURRENT:
+			return apds9160_set_ps_cancellation_current(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;
+	}
+}
+
+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_LIGHT:
+			*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_LIGHT:
+			*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;
+
+	switch (chan->type) {
+	case IIO_PROXIMITY: {
+		__le16 buf;
+
+		ret = regmap_bulk_read(data->regmap, reg, &buf, 2);
+		if (ret < 0)
+			return ret;
+		*val = le16_to_cpu(buf);
+		return IIO_VAL_INT;
+	} break;
+	case IIO_LIGHT: {
+		u8 buf[3];
+
+		ret = regmap_bulk_read(data->regmap, reg, &buf, 3);
+		if (ret < 0)
+			return ret;
+		*val = get_unaligned_le24(buf);
+		return IIO_VAL_INT;
+	} break;
+	default:
+		return -EINVAL;
+	}
+}
+
+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;
+
+	switch (chan->type) {
+	case IIO_PROXIMITY: {
+		if (val < 0 || val > APDS9160_PS_THRES_MAX)
+			return -EINVAL;
+		__le16 buf;
+
+		buf = cpu_to_le16(val);
+		return regmap_bulk_write(data->regmap, reg, &buf, 2);
+	}
+	case IIO_LIGHT: {
+		if (val < 0 || val > APDS9160_LS_THRES_MAX)
+			return -EINVAL;
+		__le32 buf = 0;
+
+		buf = cpu_to_le32(val);
+		return regmap_bulk_write(data->regmap, reg, &buf, 3);
+	}
+	default:
+		return -EINVAL;
+	}
+}
+
+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_LIGHT:
+		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, bool state)
+{
+	struct apds9160_chip *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (chan->type) {
+	case IIO_PROXIMITY:
+		ret = regmap_field_write(data->reg_int_ps, state);
+		if (ret)
+			return ret;
+		data->ps_int = state;
+		break;
+	case IIO_LIGHT:
+		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_ratelimited(&data->client->dev,
+				    "irq status reg read failed\n");
+		return IRQ_HANDLED;
+	}
+
+	if ((status & APDS9160_SR_LS_INT) &&
+	    (status & APDS9160_SR_LS_NEW_DATA) && data->als_int) {
+		iio_push_event(indio_dev,
+			       IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0,
+						    IIO_EV_TYPE_THRESH,
+						    IIO_EV_DIR_EITHER),
+			       iio_get_time_ns(indio_dev));
+	}
+
+	if ((status & APDS9160_SR_PS_INT) &&
+	    (status & APDS9160_SR_PS_NEW_DATA) && data->ps_int) {
+		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;
+
+	ret = regmap_read(chip->regmap, APDS9160_REG_ID, &val);
+	if (ret < 0) {
+		dev_err(&client->dev, "ID read failed\n");
+		return ret;
+	}
+
+	if (val != APDS9160_PART_ID_0)
+		dev_info(&client->dev, "Unsupported part id %u\n", val);
+
+	return ret;
+}
+
+static void apds9160_disable(void *chip)
+{
+	struct apds9160_chip *data = (struct apds9160_chip *)chip;
+	int ret;
+
+	ret = regmap_field_write(data->reg_enable_als, 0);
+	if (ret)
+		return;
+
+	regmap_field_write(data->reg_enable_ps, 0);
+}
+
+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_11BITS);
+	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_scale(chip,
+				     apds9160_100ms_avail[0][0],
+				     apds9160_100ms_avail[0][1]);
+	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;
+
+	ret = apds9160_set_ps_cancellation_level(
+		chip, APDS9160_DEFAULT_PS_CANCELLATION_LEVEL);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(&chip->client->dev, apds9160_disable,
+					(void *)chip);
+}
+
+static int apds9160_regfield_init(struct apds9160_chip *data)
+{
+	struct device *dev = &data->client->dev;
+	struct regmap *regmap = data->regmap;
+	struct regmap_field *tmp;
+
+	tmp = devm_regmap_field_alloc(dev, regmap, apds9160_reg_field_int_als);
+	if (IS_ERR(tmp))
+		return PTR_ERR(tmp);
+	data->reg_int_als = tmp;
+
+	tmp = devm_regmap_field_alloc(dev, regmap, apds9160_reg_field_int_ps);
+	if (IS_ERR(tmp))
+		return PTR_ERR(tmp);
+	data->reg_int_ps = tmp;
+
+	tmp = devm_regmap_field_alloc(dev, regmap, apds9160_reg_field_ls_en);
+	if (IS_ERR(tmp))
+		return PTR_ERR(tmp);
+	data->reg_enable_als = tmp;
+
+	tmp = devm_regmap_field_alloc(dev, regmap, apds9160_reg_field_ps_en);
+	if (IS_ERR(tmp))
+		return PTR_ERR(tmp);
+	data->reg_enable_ps = tmp;
+
+	tmp = devm_regmap_field_alloc(dev, regmap,
+				      apds9160_reg_field_ps_overflow);
+	if (IS_ERR(tmp))
+		return PTR_ERR(tmp);
+	data->reg_ps_overflow = tmp;
+
+	tmp = devm_regmap_field_alloc(dev, regmap, apds9160_reg_field_als_rate);
+	if (IS_ERR(tmp))
+		return PTR_ERR(tmp);
+	data->reg_als_rate = tmp;
+
+	tmp = devm_regmap_field_alloc(dev, regmap, apds9160_reg_field_als_res);
+	if (IS_ERR(tmp))
+		return PTR_ERR(tmp);
+	data->reg_als_resolution = tmp;
+
+	tmp = devm_regmap_field_alloc(dev, regmap, apds9160_reg_field_ps_rate);
+	if (IS_ERR(tmp))
+		return PTR_ERR(tmp);
+	data->reg_ps_rate = tmp;
+
+	tmp = devm_regmap_field_alloc(dev, regmap, apds9160_reg_field_als_gain);
+	if (IS_ERR(tmp))
+		return PTR_ERR(tmp);
+	data->reg_als_gain = tmp;
+
+	tmp = devm_regmap_field_alloc(dev, regmap,
+				      apds9160_reg_field_ps_current);
+	if (IS_ERR(tmp))
+		return PTR_ERR(tmp);
+	data->reg_ps_current = tmp;
+
+	tmp = devm_regmap_field_alloc(dev, regmap, apds9160_reg_field_ps_gain);
+	if (IS_ERR(tmp))
+		return PTR_ERR(tmp);
+	data->reg_ps_gain = tmp;
+
+	tmp = devm_regmap_field_alloc(dev, regmap,
+				      apds9160_reg_field_ps_resolution);
+	if (IS_ERR(tmp))
+		return PTR_ERR(tmp);
+	data->reg_ps_resolution = tmp;
+
+	return 0;
+}
+
+static const struct iio_info apds9160_info = {
+	.read_avail = apds9160_read_avail,
+	.read_raw = apds9160_read_raw,
+	.write_raw = apds9160_write_raw,
+	.write_raw_get_fmt = apds9160_write_raw_get_fmt,
+	.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 device *dev = &client->dev;
+	struct apds9160_chip *chip;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*chip));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	ret = devm_regulator_get_enable(dev, "vdd");
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable vdd supply\n");
+
+	indio_dev->info = &apds9160_info;
+	indio_dev->name = "apds9160";
+	indio_dev->channels = apds9160_channels;
+	indio_dev->num_channels = ARRAY_SIZE(apds9160_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	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))
+		return dev_err_probe(dev, PTR_ERR(chip->regmap),
+				     "regmap initialization failed.\n");
+
+	chip->client = client;
+	mutex_init(&chip->lock);
+
+	ret = apds9160_detect(chip);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "apds9160 not found\n");
+
+	ret = apds9160_regfield_init(chip);
+	if (ret)
+		return ret;
+
+	ret = apds9160_chip_init(chip);
+	if (ret)
+		return ret;
+
+	if (client->irq > 0) {
+		ret = devm_request_threaded_irq(dev, client->irq, NULL,
+						apds9160_irq_handler,
+						IRQF_ONESHOT, "apds9160_event",
+						indio_dev);
+		if (ret) {
+			return dev_err_probe(dev, ret,
+					     "request irq (%d) failed\n",
+					     client->irq);
+		}
+	}
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed iio device registration\n");
+
+	return ret;
+}
+
+static const struct of_device_id apds9160_of_match[] = {
+	{ .compatible = "brcm,apds9160" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, apds9160_of_match);
+
+static const struct i2c_device_id apds9160_id[] = { { "apds9160", 0 }, { } };
+MODULE_DEVICE_TABLE(i2c, apds9160_id);
+
+static struct i2c_driver apds9160_driver = {
+	.driver	  = {
+		.name	= "apds9160",
+		.of_match_table = apds9160_of_match,
+	},
+	.probe    = apds9160_probe,
+	.id_table = 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");

-- 
2.34.1

Re: [PATCH v2 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 5de07b8a24cf44cdb78adeab790704bf577c2c1d]

url:    https://github.com/intel-lab-lkp/linux/commits/Mikael-Gonella-Bolduc-via-B4-Relay/dt-bindings-iio-light-Add-APDS9160-binding/20241207-001144
base:   5de07b8a24cf44cdb78adeab790704bf577c2c1d
patch link:    https://lore.kernel.org/r/20241206-apds9160-driver-v2-2-be2cb72ef8f4%40dimonoff.com
patch subject: [PATCH v2 2/2] iio: light: Add APDS9160 ALS & Proximity sensor driver
reproduce: (https://download.01.org/0day-ci/archive/20241207/202412070552.HvBGzx9l-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/202412070552.HvBGzx9l-lkp@intel.com/

All warnings (new ones prefixed by >>):

   Warning: Documentation/devicetree/bindings/nvmem/zii,rave-sp-eeprom.yaml references a file that doesn't exist: Documentation/devicetree/bindings/mfd/zii,rave-sp.yaml
   Warning: Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml references a file that doesn't exist: Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
   Warning: Documentation/hwmon/g762.rst references a file that doesn't exist: Documentation/devicetree/bindings/hwmon/g762.txt
   Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/reserved-memory/qcom
>> Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/iio/light/avago,apds9160.yaml
   Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt
   Using alabaster theme

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

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

[kernel test robot]

Hi Mikael,

kernel test robot noticed the following build errors:

[auto build test ERROR on 5de07b8a24cf44cdb78adeab790704bf577c2c1d]

url:    https://github.com/intel-lab-lkp/linux/commits/Mikael-Gonella-Bolduc-via-B4-Relay/dt-bindings-iio-light-Add-APDS9160-binding/20241207-001144
base:   5de07b8a24cf44cdb78adeab790704bf577c2c1d
patch link:    https://lore.kernel.org/r/20241206-apds9160-driver-v2-2-be2cb72ef8f4%40dimonoff.com
patch subject: [PATCH v2 2/2] iio: light: Add APDS9160 ALS & Proximity sensor driver
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20241207/202412070600.aufBle2b-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241207/202412070600.aufBle2b-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/202412070600.aufBle2b-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   drivers/iio/light/apds9160.c: In function 'apds9160_read_raw':
>> drivers/iio/light/apds9160.c:911:32: error: implicit declaration of function 'FIELD_GET' [-Wimplicit-function-declaration]
     911 |                         *val = FIELD_GET(APDS9160_PS_DATA_MASK, *val);
         |                                ^~~~~~~~~
   drivers/iio/light/apds9160.c: At top level:
>> drivers/iio/light/apds9160.c:256:18: warning: 'apds9160_als_gain_avail' defined but not used [-Wunused-const-variable=]
     256 | static const int apds9160_als_gain_avail[] = {
         |                  ^~~~~~~~~~~~~~~~~~~~~~~


vim +/FIELD_GET +911 drivers/iio/light/apds9160.c

   889	
   890	static int apds9160_read_raw(struct iio_dev *indio_dev,
   891				     struct iio_chan_spec const *chan, int *val,
   892				     int *val2, long mask)
   893	{
   894		struct apds9160_chip *data = iio_priv(indio_dev);
   895		int ret = -EINVAL;
   896	
   897		switch (mask) {
   898		case IIO_CHAN_INFO_RAW:
   899			switch (chan->type) {
   900			case IIO_PROXIMITY: {
   901				__le16 buf;
   902	
   903				ret = regmap_bulk_read(data->regmap, chan->address,
   904						       &buf, 2);
   905				if (ret)
   906					return ret;
   907	
   908				ret = IIO_VAL_INT;
   909				*val = le16_to_cpu(buf);
   910				/* Remove overflow bits from result */
 > 911				*val = FIELD_GET(APDS9160_PS_DATA_MASK, *val);
   912			} break;
   913			case IIO_LIGHT:
   914			case IIO_INTENSITY: {
   915				u8 buf[3];
   916	
   917				ret = regmap_bulk_read(data->regmap, chan->address,
   918						       &buf, 3);
   919				if (ret)
   920					return ret;
   921	
   922				ret = IIO_VAL_INT;
   923				*val = get_unaligned_le24(buf);
   924			} break;
   925			case IIO_CURRENT:
   926				ret = IIO_VAL_INT;
   927				*val = data->ps_current;
   928				break;
   929			default:
   930				break;
   931			}
   932			break;
   933		case IIO_CHAN_INFO_HARDWAREGAIN:
   934			switch (chan->type) {
   935			case IIO_LIGHT:
   936				ret = IIO_VAL_INT;
   937				*val = data->als_hwgain;
   938				break;
   939			case IIO_PROXIMITY:
   940				ret = IIO_VAL_INT;
   941				*val = data->ps_gain;
   942				break;
   943			default:
   944				break;
   945			}
   946			break;
   947		case IIO_CHAN_INFO_INT_TIME:
   948			switch (chan->type) {
   949			case IIO_LIGHT:
   950				ret = IIO_VAL_INT;
   951				*val = data->als_itime;
   952				break;
   953			default:
   954				break;
   955			}
   956			break;
   957		case IIO_CHAN_INFO_SAMP_FREQ:
   958			switch (chan->type) {
   959			case IIO_PROXIMITY:
   960				ret = IIO_VAL_INT;
   961				*val = data->ps_rate;
   962				break;
   963			default:
   964				break;
   965			}
   966			break;
   967		case IIO_CHAN_INFO_CALIBSCALE:
   968			switch (chan->type) {
   969			case IIO_PROXIMITY:
   970				ret = IIO_VAL_INT;
   971				*val = data->ps_cancellation_level;
   972				break;
   973			default:
   974				break;
   975			}
   976			break;
   977		case IIO_CHAN_INFO_CALIBBIAS:
   978			switch (chan->type) {
   979			case IIO_PROXIMITY:
   980				ret = IIO_VAL_INT;
   981				*val = data->ps_cancellation_analog;
   982				break;
   983			case IIO_CURRENT:
   984				ret = IIO_VAL_INT;
   985				*val = data->ps_cancellation_current;
   986			default:
   987				break;
   988			}
   989			break;
   990		case IIO_CHAN_INFO_SCALE:
   991			switch (chan->type) {
   992			case IIO_LIGHT:
   993				ret = IIO_VAL_INT_PLUS_MICRO;
   994				*val = data->als_scale1;
   995				*val2 = data->als_scale2;
   996				break;
   997			default:
   998				break;
   999			}
  1000			break;
  1001		default:
  1002			break;
  1003		}
  1004	
  1005		return ret;
  1006	};
  1007	

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

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

[Jonathan Cameron]

On Fri, 06 Dec 2024 11:09: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,

As the bots noted, the maintainers entry has the wrong vendor prefix,
or the binding does.

Also the issue with missing include of linux/bitfield.h

Unused gain table is less obvious. Not sure what intent was on that one.

Given the discussion with Matti about how to do the gain control, please add
some description here of the outcome.  The control scheme is not particularly
obvious and is the key bit we should be reviewing.  It feels like you've
applied the feedback on v1 to the light channel but it is equally applicable
to proximity channels when they are just measures of reflected light intensity.

Various other minor things inline.

Thanks,


Jonathan



> ---
>  MAINTAINERS                  |    7 +
>  drivers/iio/light/Kconfig    |   11 +
>  drivers/iio/light/Makefile   |    1 +
>  drivers/iio/light/apds9160.c | 1548 ++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 1567 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e69d1632c382fe0e366f7bb20e72ee0c9e91e30b..99903578e2a31dd946bd5f6722eb2aabc349a090 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3714,6 +3714,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 29ffa84919273d64b8464ab8bbf59661b0102f97..419360661d53973eda71d7d986ff7fd481c7aa2c 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -63,6 +63,17 @@ 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 sensor"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	   Say Y here if you want to build support for a Broadcom APDS9160
> +		combined ambient light and proximity sensor.
> +
> +	   To compile this driver as a module, choose M here: the
> +	   module will be called apds9160.
> +
>  config APDS9300
>  	tristate "APDS9300 ambient light sensor"
>  	depends on I2C
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index f14a3744271242f04fe7849b47308397ac2c9939..4a22043acdbbbed2b696a3225e4024654d5d9339 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..1cc639c31ebfef7254bf967e56a25a75f99ab05c
> --- /dev/null
> +++ b/drivers/iio/light/apds9160.c
> @@ -0,0 +1,1548 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * APDS9160 sensor driver.
> + * Chip is combined proximity and ambient light sensor.
> + * Author: 2024 Mikael Gonella-Bolduc <m.gonella.bolduc@gmail.com>
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/cleanup.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/sysfs.h>
> +
> +#include <linux/unaligned.h>
> +
> +#define APDS9160_REGMAP_NAME "apds9160_regmap"
> +#define APDS9160_REG_CNT 37
> +
> +/* Main control register */
> +#define APDS9160_REG_CTRL 0x00
> +#define APDS9160_CTRL_SWRESET BIT(4) /* 1: Activate reset */
> +#define APDS9160_CTRL_MODE_RGB BIT(2) /* 0: ALS & IR, 1: RGB & IR */
> +#define APDS9160_CTRL_EN_ALS BIT(1) /* 1: ALS active */
> +#define APDS9160_CTLR_EN_PS BIT(0) /* 1: PS active */
> +
> +/* Status register  */
> +#define APDS9160_SR_LS_INT BIT(4)
> +#define APDS9160_SR_LS_NEW_DATA BIT(3)
> +#define APDS9160_SR_PS_INT BIT(1)
> +#define APDS9160_SR_PS_NEW_DATA BIT(0)
> +
> +/* Interrupt configuration registers */
> +#define APDS9160_REG_INT_CFG 0x19
> +#define APDS9160_REG_INT_PST 0x1A
> +#define APDS9160_INT_CFG_EN_LS BIT(2) /* LS int enable */
> +#define APDS9160_INT_CFG_EN_PS BIT(0) /* PS int enable */
> +
> +/* 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_SR_DATA_ALS BIT(3)
> +#define APDS9160_SR_DATA_PS BIT(0)
> +
> +/* Supported ID:s */
> +#define APDS9160_PART_ID_0 0x03
> +
> +#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_11BITS 0x03
> +
> +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 */
> +	{ APDS9160_REG_PS_PULSES, 0x08 }, /* 8 pulses */
> +	{ APDS9160_REG_PS_MEAS_RATE, 0x05 }, /* 100ms */
> +	{ APDS9160_REG_LS_MEAS_RATE, 0x22 }, /* 100ms */
> +	{ APDS9160_REG_LS_GAIN, 0x01 }, /* 3x */
> +	{ APDS9160_REG_INT_CFG, 0x10 }, /* Interrupts disabled */
> +	{ 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 },
> +};
> +
> +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_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,
> +		.dir = IIO_EV_DIR_EITHER,
> +		.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),
Unusual to have integration time control on a light sensor but not in proximity
mode.

> +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> +						BIT(IIO_CHAN_INFO_HARDWAREGAIN),
> +		.event_spec = apds9160_event_spec,
> +		.num_event_specs = ARRAY_SIZE(apds9160_event_spec),

As below.  Give IRQ is optional (which is good btw) you should not register
the events if we have no way to actually get them.
So unless I'm missing something you should have a second channel array with
no events used form an iio_info structure with no event callbacks.

> +	},
> +	{
> +		/* Proximity sensor led current */
> +		.type = IIO_CURRENT,
> +		.output = 1,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_CALIBBIAS),
> +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW),
> +	},
> +	{
> +		/* 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_INT_TIME) |
> +					    BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
> +					    BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_INT_TIME) |
> +						BIT(IIO_CHAN_INFO_SCALE),
> +		.event_spec = apds9160_event_spec,
> +		.num_event_specs = ARRAY_SIZE(apds9160_event_spec),
> +	},
> +	{
> +		/* Clear channel */
> +		.type = IIO_INTENSITY,
> +		.address = APDS9160_REG_LS_DATA_CLEAR_LSB,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.channel2 = IIO_MOD_LIGHT_CLEAR,
> +		.modified = 1,
> +	},
> +};
> +
> +/* Attributes */

Random mixture of stuff somewhat related to attributes.
I'd drop the comment as to my eyes it isn't really helpful.

> +static const int apds9160_als_rate_avail[] = {
> +	25, 50, 100, 200
> +};

> +
> +/*

It is very close to kernel-doc.  I'd just make it compliant
by adding the structure name and short description.

> + * @itime: Integration time in ms
> + * @gain: Gain multiplier
> + * @scale1: lux/count resolution
> + * @scale2: micro lux/count
> + */
> +struct apds9160_scale {
> +	int itime;
> +	int gain;
> +	int scale1;
> +	int scale2;
> +};
> +
> +/*
> + * Scale mapping extracted from datasheet
> + */
Single line comment would be fine here.


> +/*
> + * This parameter works in conjunction with the cancellation pulse duration
> + * The value determines the current used for crosstalk cancellation
> + * B4-B5: The coarse value defines cancellation current in steps of 60nA
> + * B0-B3: The fine value adjusts the cancellation current in steps of 2.4nA
> + */
> +static int apds9160_set_ps_cancellation_current(struct apds9160_chip *data,
> +						int val)
> +{
> +	int ret;
> +
> +	if (val < 0 || val > 0x3F)
> +		return -EINVAL;
> +
> +	ret = regmap_write(data->regmap, APDS9160_REG_PS_CAN_LEVEL_ANA_CURRENT,
> +			   val);
> +	if (!ret)
For consistency, prefer the error cases out of line even if it adds a line or two of
code.  It just makes things a tiny bit easier to review when people are reading
lots of code if everything is the same way around.

Apply same logic in other similar cases.

	if (ret)
		return ret;

	data->ps...
	
	return 0;

> +		data->ps_cancellation_current = val;
> +
> +	return ret;
> +}

> +static int apds9160_set_als_gain(struct apds9160_chip *data, int gain)
> +{
> +	int ret = -EINVAL;
> +	int idx;
> +
> +	for (idx = 0; idx < ARRAY_SIZE(apds9160_als_gain_map); idx++) {
> +		if (gain == apds9160_als_gain_map[idx][0]) {
> +			ret = regmap_field_write(data->reg_als_gain,
> +					apds9160_als_gain_map[idx][1]);
> +			if (ret)
> +				return ret;
> +
> +			data->als_hwgain = gain;
> +			break;

As below. Early return simplifies this as the return below is then in
error case only.
Can also drag ret into this local scope. 
Make sure to apply to other similar cases.


> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static int apds9160_set_als_scale(struct apds9160_chip *data, int val, int val2)
> +{
> +	int ret = -EINVAL;
> +	int idx;
> +
> +	for (idx = 0; idx < ARRAY_SIZE(apds9160_als_scale_map); idx++) {
> +		if (apds9160_als_scale_map[idx].itime == data->als_itime &&
> +		    apds9160_als_scale_map[idx].scale1 == val &&
> +		    apds9160_als_scale_map[idx].scale2 == val2) {
> +			ret = apds9160_set_als_gain(data,
> +					apds9160_als_scale_map[idx].gain);
> +			if (ret)
> +				return ret;
> +
> +			data->als_scale1 = val;
> +			data->als_scale2 = val2;
> +			break;

As below. Might as well return.

> +		}
> +	}
> +
> +	return ret;
return -EINVAL; as with the early return above, can't be anything else.
> +}

> +/*
> + *	Setting the integration time ajusts resolution, rate, scale and gain
> + */
> +static int apds9160_set_als_int_time(struct apds9160_chip *data, int val)
> +{
> +	int ret = -EINVAL;
> +	int idx;
> +
> +	ret = apds9160_set_als_rate(data, val);
> +	if (ret)
> +		return ret;
> +
> +	/* Match resolution register with rate */
> +	ret = apds9160_set_als_resolution(data, val);
> +	if (ret)
> +		return ret;
> +
> +	data->als_itime = val;
> +
> +	/* Set the scale minimum gain */
> +	for (idx = 0; idx < ARRAY_SIZE(apds9160_als_scale_map); idx++) {
> +		if (data->als_itime == apds9160_als_scale_map[idx].itime) {
Consider flipping it for shorter lines.
		if (data->als_itime != adps9160_als_scale_map[idx].itime)
			continue;

		return apds9160...

> +			ret = apds9160_set_als_scale(data,
> +					apds9160_als_scale_map[idx].scale1,
> +					apds9160_als_scale_map[idx].scale2);
> +			break;
Might as well return here.

> +		}
> +	}
> +
> +	return ret;
return -EINVAL; 
Right now ret = 0 in this path after not matching which is not I think the intent.

> +}


> +
> +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)
> +				return ret;
> +
> +			ret = IIO_VAL_INT;
> +			*val = le16_to_cpu(buf);
> +			/* Remove overflow bits from result */
> +			*val = FIELD_GET(APDS9160_PS_DATA_MASK, *val);

As below. Return if there is nothing else to do rather than make the
reader go looking for end of the nested switch.

Also  return IIO_VAL_INT; is easier to follow than checking that the ret
value was set in all good paths.


> +		} break;
> +		case IIO_LIGHT:
> +		case IIO_INTENSITY: {
> +			u8 buf[3];
> +
> +			ret = regmap_bulk_read(data->regmap, chan->address,
> +					       &buf, 3);
> +			if (ret)
> +				return ret;
> +
> +			ret = IIO_VAL_INT;
> +			*val = get_unaligned_le24(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_LIGHT:
> +			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_LIGHT:
> +			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;
> +		case IIO_CURRENT:
> +			ret = IIO_VAL_INT;
> +			*val = data->ps_cancellation_current;
> +		default:
> +			break;
> +		}
> +		break;
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_LIGHT:
> +			ret = IIO_VAL_INT_PLUS_MICRO;
> +			*val = data->als_scale1;
> +			*val2 = data->als_scale2;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return ret;
> +};

> +
> +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_LIGHT:
> +			*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;
return in each leg to make the flow simpler to follow.
Then drop all unneeded breaks and the final return 0.


> +		case IIO_LIGHT:
> +			*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;
> +

Drop this blank line.

> +	int ret = 0;

Always set, so don't set it here.

> +	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;
> +
> +	switch (chan->type) {
> +	case IIO_PROXIMITY: {
> +		__le16 buf;
> +
> +		ret = regmap_bulk_read(data->regmap, reg, &buf, 2);
> +		if (ret < 0)
> +			return ret;
> +		*val = le16_to_cpu(buf);
> +		return IIO_VAL_INT;
> +	} break;
> +	case IIO_LIGHT: {
> +		u8 buf[3];
> +
> +		ret = regmap_bulk_read(data->regmap, reg, &buf, 3);
> +		if (ret < 0)
> +			return ret;
> +		*val = get_unaligned_le24(buf);
> +		return IIO_VAL_INT;
> +	} break;
Already returned.  The compiler better be able to figure that out!
So this break is unreachable code.  Check for others of these.


> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +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;

Set in all paths where it is used, so don't set it here.

> +	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;
> +
> +	switch (chan->type) {
> +	case IIO_PROXIMITY: {
> +		if (val < 0 || val > APDS9160_PS_THRES_MAX)
> +			return -EINVAL;
> +		__le16 buf;
> +
> +		buf = cpu_to_le16(val);
> +		return regmap_bulk_write(data->regmap, reg, &buf, 2);
> +	}
> +	case IIO_LIGHT: {
> +		if (val < 0 || val > APDS9160_LS_THRES_MAX)
> +			return -EINVAL;
> +		__le32 buf = 0;

No need to initialise.

Also, we still mostly respect old style c formtting, so that __le32 buf
belongs before the checks on val.

The only exceptions for this are when autocleanup is in use where the
declaration location matters for correctness + various things so wrapped
up in macros it's not obvious they are creating local variables.


> +
> +		buf = cpu_to_le32(val);
as with the above get...

It is 3 bytes not 4 so use a u8 buf[3] and put_unaligned_le24()
rather than a padded valued. 

> +		return regmap_bulk_write(data->regmap, reg, &buf, 3);
> +	}
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +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_LIGHT:
> +		return data->als_int;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;

Can't get here so remove this return.  If that changes in future the compiler
will complain and hence any bugs are easy to spot. In some circumstances it will
complain about unreachable code with what you have here.

> +}
> +
> +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, bool state)
> +{
> +	struct apds9160_chip *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (chan->type) {
> +	case IIO_PROXIMITY:
> +		ret = regmap_field_write(data->reg_int_ps, state);
> +		if (ret)
> +			return ret;
> +		data->ps_int = state;
> +		break;
Easier to return 0 here
> +	case IIO_LIGHT:
> +		ret = regmap_field_write(data->reg_int_als, state);
> +		if (ret)
> +			return ret;
> +		data->als_int = state;
> +		break;
and here.
> +	default:
> +		return -EINVAL;
> +	}
> +
and drop this one.
> +	return 0;
> +}

> +
> +static int apds9160_detect(struct apds9160_chip *chip)
> +{
> +	struct i2c_client *client = chip->client;
> +	int ret;
> +	u32 val;
> +
> +	ret = regmap_read(chip->regmap, APDS9160_REG_ID, &val);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "ID read failed\n");
> +		return ret;
> +	}
> +
> +	if (val != APDS9160_PART_ID_0)
> +		dev_info(&client->dev, "Unsupported part id %u\n", val);
> +
> +	return ret;

I'd return 0 to make it explicit that this is always hit in the
non error path only. Also makes it easier for the compiler to see that
ret is never positive.


> +}
> +
> +static void apds9160_disable(void *chip)
> +{
> +	struct apds9160_chip *data = (struct apds9160_chip *)chip;

No need (in C spec) to ever explicitly cast from void * to other
pointer types, so drop that case and any similar ones.

> +	int ret;
> +
> +	ret = regmap_field_write(data->reg_enable_als, 0);
> +	if (ret)
> +		return;
> +
> +	regmap_field_write(data->reg_enable_ps, 0);
> +}
> +
> +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_11BITS);
> +	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_scale(chip,
> +				     apds9160_100ms_avail[0][0],
> +				     apds9160_100ms_avail[0][1]);
> +	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;
> +
> +	ret = apds9160_set_ps_cancellation_level(
> +		chip, APDS9160_DEFAULT_PS_CANCELLATION_LEVEL);
> +	if (ret)
> +		return ret;
> +
> +	return devm_add_action_or_reset(&chip->client->dev, apds9160_disable,
> +					(void *)chip);
No need to cast explicitly from any pointer type to a void *.

> +}


> +static const struct iio_info apds9160_info = {
> +	.read_avail = apds9160_read_avail,
> +	.read_raw = apds9160_read_raw,
> +	.write_raw = apds9160_write_raw,
> +	.write_raw_get_fmt = apds9160_write_raw_get_fmt,
> +	.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 device *dev = &client->dev;
> +	struct apds9160_chip *chip;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*chip));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable vdd supply\n");
> +
> +	indio_dev->info = &apds9160_info;
> +	indio_dev->name = "apds9160";
> +	indio_dev->channels = apds9160_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(apds9160_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	chip = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);

I'm not immediately able to find where this is used. If it's not
then don't set it.

> +	chip->client = client;
> +	chip->regmap = devm_regmap_init_i2c(client, &apds9160_regmap_config);
> +	if (IS_ERR(chip->regmap))
> +		return dev_err_probe(dev, PTR_ERR(chip->regmap),
> +				     "regmap initialization failed.\n");
> +
> +	chip->client = client;
> +	mutex_init(&chip->lock);
> +
> +	ret = apds9160_detect(chip);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "apds9160 not found\n");
> +
> +	ret = apds9160_regfield_init(chip);
> +	if (ret)
> +		return ret;
> +
> +	ret = apds9160_chip_init(chip);
> +	if (ret)
> +		return ret;
> +
> +	if (client->irq > 0) {

As the interrupt is optional, we should hide all the event related elements
if there is no interrupt to signal them.  Normal way to do this is two
iio_info structures that you pick between depending on availability of the irq.


> +		ret = devm_request_threaded_irq(dev, client->irq, NULL,
> +						apds9160_irq_handler,
> +						IRQF_ONESHOT, "apds9160_event",
> +						indio_dev);
> +		if (ret) {
> +			return dev_err_probe(dev, ret,
> +					     "request irq (%d) failed\n",
> +					     client->irq);
> +		}
> +	}
> +
> +	ret = devm_iio_device_register(dev, indio_dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed iio device registration\n");
> +
> +	return ret;
> +}
> +
> +static const struct of_device_id apds9160_of_match[] = {
> +	{ .compatible = "brcm,apds9160" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, apds9160_of_match);
> +
> +static const struct i2c_device_id apds9160_id[] = { { "apds9160", 0 }, { } };

Hmm. Not sure why I missed this in v1. Format in a style that makes it easy to extend.

static const struct i2c_device_id apds9160_id[] = {
	{ "apds9160", 0 },
	{ }
};

> +MODULE_DEVICE_TABLE(i2c, apds9160_id);

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

[Mikael Gonella-Bolduc]

On Sun, Dec 08, 2024 at 12:20:38PM +0000, Jonathan Cameron wrote:
> On Fri, 06 Dec 2024 11:09: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,
> 
> As the bots noted, the maintainers entry has the wrong vendor prefix,
> or the binding does.
> 
> Also the issue with missing include of linux/bitfield.h
> 
> Unused gain table is less obvious. Not sure what intent was on that one.
> 
> Given the discussion with Matti about how to do the gain control, please add
> some description here of the outcome.  The control scheme is not particularly
> obvious and is the key bit we should be reviewing.  It feels like you've
> applied the feedback on v1 to the light channel but it is equally applicable
> to proximity channels when they are just measures of reflected light intensity.
> 
> Various other minor things inline.
> 
> Thanks,
> 
> 
> Jonathan

Hi Jonathan,

I will fix the warnings the bots noted and other inline comments for v3, sorry about that.
Regarding gain control for ALS, I kept the non-linear table provided in the datasheet.
The user can adjust the integration time and the available scales will update
depending on the value.
For example, at 100ms, you have possible scales of 0.819, 0.269, 0.131, etc. (lux/count).
The hardware gain and other relevant registers gets adjusted by the driver depending on selected scale.
The attribute is kept as read-only as Matti suggested.

Now, for proximity, again I'm confused. Please bear with me a little.
The only "scale" I see in the datasheet is that the proximity sensor is for a short distance of under 70mm.
There's nothing provided in the datasheet to convert the proximity ADC count to a distance or to anything meaningful like standard units.
I don't feel like this is really precise and the intended use case is probably like mine where you can use this to detect
if there's something covering the sensor or not.

I took a look at other light/proximity sensors, again, it's not clear for me how to handle this.
It seems that some drivers just directly control the hardware gain register with the scale even if it's not really a scale.

What should I do?

Best regards,
Mikael

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

[Jonathan Cameron]

On Mon, 9 Dec 2024 16:49:12 -0500
Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com> wrote:

> On Sun, Dec 08, 2024 at 12:20:38PM +0000, Jonathan Cameron wrote:
> > On Fri, 06 Dec 2024 11:09: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,
> > 
> > As the bots noted, the maintainers entry has the wrong vendor prefix,
> > or the binding does.
> > 
> > Also the issue with missing include of linux/bitfield.h
> > 
> > Unused gain table is less obvious. Not sure what intent was on that one.
> > 
> > Given the discussion with Matti about how to do the gain control, please add
> > some description here of the outcome.  The control scheme is not particularly
> > obvious and is the key bit we should be reviewing.  It feels like you've
> > applied the feedback on v1 to the light channel but it is equally applicable
> > to proximity channels when they are just measures of reflected light intensity.
> > 
> > Various other minor things inline.
> > 
> > Thanks,
> > 
> > 
> > Jonathan  
> 
> Hi Jonathan,
> 
> I will fix the warnings the bots noted and other inline comments for v3, sorry about that.
> Regarding gain control for ALS, I kept the non-linear table provided in the datasheet.
> The user can adjust the integration time and the available scales will update
> depending on the value.
> For example, at 100ms, you have possible scales of 0.819, 0.269, 0.131, etc. (lux/count).
> The hardware gain and other relevant registers gets adjusted by the driver depending on selected scale.
> The attribute is kept as read-only as Matti suggested.
> 
> Now, for proximity, again I'm confused. Please bear with me a little.
> The only "scale" I see in the datasheet is that the proximity sensor is for a short distance of under 70mm.

That sounds like a design point for sensitivity of sensor vs light source brightness.

> There's nothing provided in the datasheet to convert the proximity ADC count to a distance or to anything meaningful like standard units.
> I don't feel like this is really precise and the intended use case is probably like mine where you can use this to detect
> if there's something covering the sensor or not.
> 
> I took a look at other light/proximity sensors, again, it's not clear for me how to handle this.
> It seems that some drivers just directly control the hardware gain register with the scale even if it's not really a scale.
Typical case is that it is a scale, just not of distance.  But rather controls an amplifier on the light sensor,
so same as for the ambient light sensor.

The ABI docs are a little vague on this Documentation/ABI/testing/sysfs-bus-iio
has
What:		/sys/.../iio:deviceX/in_proximity_raw
What:		/sys/.../iio:deviceX/in_proximity_input
What:		/sys/.../iio:deviceX/in_proximityY_raw
KernelVersion:	3.4
Contact:	linux-iio@vger.kernel.org
Description:
		Proximity measurement indicating that some
		object is near the sensor, usually by observing
		reflectivity of infrared or ultrasound emitted.

		Often these sensors are unit less and as such conversion
		to SI units is not possible. Higher proximity measurements
		indicate closer objects, and vice versa. Units after
		application of scale and offset are meters.

So it kind of says we can't relate them to real units, but then we provide
a unit. Hmm, not our finest and clearest documentation.

Probably best bet is to follow precedence as even if we haven't tightly defined
it that is what any userspace tuning these value will be using.

Given inverse square law and different characteristics of reflective surfaces
I think it is normally a case of crank the gain up until the signal is good.

In most cases these proximity sensors aren't much more than fancy switches
though can be used for approaching vs moving away detection.

Anyhow, I haven't checked all the precedence in existing drivers but from
memory scale is the standard choice.

Hardware gain as a writable control is just rarely used and only in devices where
it doesn't affect what we are measuring. In proximity that means time of flight
sensors, not ones based on reflected intensity.

Jonathan

> 
> What should I do?
> 
> Best regards,
> Mikael

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

[Mikael Gonella-Bolduc]

On Wed, Dec 11, 2024 at 08:53:02PM +0000, Jonathan Cameron wrote:
> On Mon, 9 Dec 2024 16:49:12 -0500
> Mikael Gonella-Bolduc <mgonellabolduc@dimonoff.com> wrote:
> 
> > On Sun, Dec 08, 2024 at 12:20:38PM +0000, Jonathan Cameron wrote:
> > > On Fri, 06 Dec 2024 11:09: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,
> > > 
> > > As the bots noted, the maintainers entry has the wrong vendor prefix,
> > > or the binding does.
> > > 
> > > Also the issue with missing include of linux/bitfield.h
> > > 
> > > Unused gain table is less obvious. Not sure what intent was on that one.
> > > 
> > > Given the discussion with Matti about how to do the gain control, please add
> > > some description here of the outcome.  The control scheme is not particularly
> > > obvious and is the key bit we should be reviewing.  It feels like you've
> > > applied the feedback on v1 to the light channel but it is equally applicable
> > > to proximity channels when they are just measures of reflected light intensity.
> > > 
> > > Various other minor things inline.
> > > 
> > > Thanks,
> > > 
> > > 
> > > Jonathan  
> > 
> > Hi Jonathan,
> > 
> > I will fix the warnings the bots noted and other inline comments for v3, sorry about that.
> > Regarding gain control for ALS, I kept the non-linear table provided in the datasheet.
> > The user can adjust the integration time and the available scales will update
> > depending on the value.
> > For example, at 100ms, you have possible scales of 0.819, 0.269, 0.131, etc. (lux/count).
> > The hardware gain and other relevant registers gets adjusted by the driver depending on selected scale.
> > The attribute is kept as read-only as Matti suggested.
> > 
> > Now, for proximity, again I'm confused. Please bear with me a little.
> > The only "scale" I see in the datasheet is that the proximity sensor is for a short distance of under 70mm.
> 
> That sounds like a design point for sensitivity of sensor vs light source brightness.
> 
> > There's nothing provided in the datasheet to convert the proximity ADC count to a distance or to anything meaningful like standard units.
> > I don't feel like this is really precise and the intended use case is probably like mine where you can use this to detect
> > if there's something covering the sensor or not.
> > 
> > I took a look at other light/proximity sensors, again, it's not clear for me how to handle this.
> > It seems that some drivers just directly control the hardware gain register with the scale even if it's not really a scale.
> Typical case is that it is a scale, just not of distance.  But rather controls an amplifier on the light sensor,
> so same as for the ambient light sensor.
> 
> The ABI docs are a little vague on this Documentation/ABI/testing/sysfs-bus-iio
> has
> What:		/sys/.../iio:deviceX/in_proximity_raw
> What:		/sys/.../iio:deviceX/in_proximity_input
> What:		/sys/.../iio:deviceX/in_proximityY_raw
> KernelVersion:	3.4
> Contact:	linux-iio@vger.kernel.org
> Description:
> 		Proximity measurement indicating that some
> 		object is near the sensor, usually by observing
> 		reflectivity of infrared or ultrasound emitted.
> 
> 		Often these sensors are unit less and as such conversion
> 		to SI units is not possible. Higher proximity measurements
> 		indicate closer objects, and vice versa. Units after
> 		application of scale and offset are meters.
> 
> So it kind of says we can't relate them to real units, but then we provide
> a unit. Hmm, not our finest and clearest documentation.
> 
> Probably best bet is to follow precedence as even if we haven't tightly defined
> it that is what any userspace tuning these value will be using.
> 
> Given inverse square law and different characteristics of reflective surfaces
> I think it is normally a case of crank the gain up until the signal is good.
> 
> In most cases these proximity sensors aren't much more than fancy switches
> though can be used for approaching vs moving away detection.
> 
> Anyhow, I haven't checked all the precedence in existing drivers but from
> memory scale is the standard choice.
> 
> Hardware gain as a writable control is just rarely used and only in devices where
> it doesn't affect what we are measuring. In proximity that means time of flight
> sensors, not ones based on reflected intensity.
> 
> Jonathan
> 

Hi Jonathan,

Thank you for the clarifications.
Please see v3.

Best regards,
Mikael 

Re: [PATCH v2 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 5de07b8a24cf44cdb78adeab790704bf577c2c1d]

url:    https://github.com/intel-lab-lkp/linux/commits/Mikael-Gonella-Bolduc-via-B4-Relay/dt-bindings-iio-light-Add-APDS9160-binding/20241207-001144
base:   5de07b8a24cf44cdb78adeab790704bf577c2c1d
patch link:    https://lore.kernel.org/r/20241206-apds9160-driver-v2-2-be2cb72ef8f4%40dimonoff.com
patch subject: [PATCH v2 2/2] iio: light: Add APDS9160 ALS & Proximity sensor driver
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20241207/202412070636.sTYakaSQ-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241207/202412070636.sTYakaSQ-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/202412070636.sTYakaSQ-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/iio/light/apds9160.c:12:
   In file included from include/linux/i2c.h:13:
   In file included from include/linux/acpi.h:14:
   In file included from include/linux/device.h:32:
   In file included from include/linux/device/driver.h:21:
   In file included from include/linux/module.h:19:
   In file included from include/linux/elf.h:6:
   In file included from arch/s390/include/asm/elf.h:181:
   In file included from arch/s390/include/asm/mmu_context.h:11:
   In file included from arch/s390/include/asm/pgalloc.h:18:
   In file included from include/linux/mm.h:2213:
   include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     505 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     512 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     525 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   In file included from drivers/iio/light/apds9160.c:16:
   In file included from include/linux/regmap.h:20:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:14:
   In file included from arch/s390/include/asm/io.h:95:
   include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     548 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     561 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
         |                                                           ^
   include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
     102 | #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
         |                                                      ^
   In file included from drivers/iio/light/apds9160.c:16:
   In file included from include/linux/regmap.h:20:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:14:
   In file included from arch/s390/include/asm/io.h:95:
   include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     574 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
         |                                                           ^
   include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
     115 | #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
         |                                                      ^
   In file included from drivers/iio/light/apds9160.c:16:
   In file included from include/linux/regmap.h:20:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:14:
   In file included from arch/s390/include/asm/io.h:95:
   include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     585 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     595 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     605 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:693:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     693 |         readsb(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:701:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     701 |         readsw(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:709:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     709 |         readsl(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:718:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     718 |         writesb(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:727:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     727 |         writesw(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:736:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     736 |         writesl(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
>> drivers/iio/light/apds9160.c:986:3: warning: unannotated fall-through between switch labels [-Wimplicit-fallthrough]
     986 |                 default:
         |                 ^
   drivers/iio/light/apds9160.c:986:3: note: insert 'break;' to avoid fall-through
     986 |                 default:
         |                 ^
         |                 break; 
>> drivers/iio/light/apds9160.c:256:18: warning: unused variable 'apds9160_als_gain_avail' [-Wunused-const-variable]
     256 | static const int apds9160_als_gain_avail[] = {
         |                  ^~~~~~~~~~~~~~~~~~~~~~~
   18 warnings generated.


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

   889	
   890	static int apds9160_read_raw(struct iio_dev *indio_dev,
   891				     struct iio_chan_spec const *chan, int *val,
   892				     int *val2, long mask)
   893	{
   894		struct apds9160_chip *data = iio_priv(indio_dev);
   895		int ret = -EINVAL;
   896	
   897		switch (mask) {
   898		case IIO_CHAN_INFO_RAW:
   899			switch (chan->type) {
   900			case IIO_PROXIMITY: {
   901				__le16 buf;
   902	
   903				ret = regmap_bulk_read(data->regmap, chan->address,
   904						       &buf, 2);
   905				if (ret)
   906					return ret;
   907	
   908				ret = IIO_VAL_INT;
   909				*val = le16_to_cpu(buf);
   910				/* Remove overflow bits from result */
   911				*val = FIELD_GET(APDS9160_PS_DATA_MASK, *val);
   912			} break;
   913			case IIO_LIGHT:
   914			case IIO_INTENSITY: {
   915				u8 buf[3];
   916	
   917				ret = regmap_bulk_read(data->regmap, chan->address,
   918						       &buf, 3);
   919				if (ret)
   920					return ret;
   921	
   922				ret = IIO_VAL_INT;
   923				*val = get_unaligned_le24(buf);
   924			} break;
   925			case IIO_CURRENT:
   926				ret = IIO_VAL_INT;
   927				*val = data->ps_current;
   928				break;
   929			default:
   930				break;
   931			}
   932			break;
   933		case IIO_CHAN_INFO_HARDWAREGAIN:
   934			switch (chan->type) {
   935			case IIO_LIGHT:
   936				ret = IIO_VAL_INT;
   937				*val = data->als_hwgain;
   938				break;
   939			case IIO_PROXIMITY:
   940				ret = IIO_VAL_INT;
   941				*val = data->ps_gain;
   942				break;
   943			default:
   944				break;
   945			}
   946			break;
   947		case IIO_CHAN_INFO_INT_TIME:
   948			switch (chan->type) {
   949			case IIO_LIGHT:
   950				ret = IIO_VAL_INT;
   951				*val = data->als_itime;
   952				break;
   953			default:
   954				break;
   955			}
   956			break;
   957		case IIO_CHAN_INFO_SAMP_FREQ:
   958			switch (chan->type) {
   959			case IIO_PROXIMITY:
   960				ret = IIO_VAL_INT;
   961				*val = data->ps_rate;
   962				break;
   963			default:
   964				break;
   965			}
   966			break;
   967		case IIO_CHAN_INFO_CALIBSCALE:
   968			switch (chan->type) {
   969			case IIO_PROXIMITY:
   970				ret = IIO_VAL_INT;
   971				*val = data->ps_cancellation_level;
   972				break;
   973			default:
   974				break;
   975			}
   976			break;
   977		case IIO_CHAN_INFO_CALIBBIAS:
   978			switch (chan->type) {
   979			case IIO_PROXIMITY:
   980				ret = IIO_VAL_INT;
   981				*val = data->ps_cancellation_analog;
   982				break;
   983			case IIO_CURRENT:
   984				ret = IIO_VAL_INT;
   985				*val = data->ps_cancellation_current;
 > 986			default:
   987				break;
   988			}
   989			break;
   990		case IIO_CHAN_INFO_SCALE:
   991			switch (chan->type) {
   992			case IIO_LIGHT:
   993				ret = IIO_VAL_INT_PLUS_MICRO;
   994				*val = data->als_scale1;
   995				*val2 = data->als_scale2;
   996				break;
   997			default:
   998				break;
   999			}
  1000			break;
  1001		default:
  1002			break;
  1003		}
  1004	
  1005		return ret;
  1006	};
  1007	

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