[PATCH v2 0/2] Support for Avago APDS9306 Ambient Light Sensor

[PATCH v2 0/2] Support for Avago APDS9306 Ambient Light Sensor

[Subhajit Ghosh]

This series adds support for Avago (Broadcom) APDS9306 Ambient Light
Sensor.

Datasheet: https://docs.broadcom.com/doc/AV02-4755EN

Following features are supported:
  - I2C interface
  - 2 channels - als and clear
  - Raw data for als and clear channels
  - Up to 20 bit resolution
  - 20 bit data register for each channel
  - Common Configurable items for both channels
    - Integration Time
    - Scale
  - High and Low threshold interrupts for each channel
  - Selection of interrupt channels - als or clear
  - Selection of interrupt mode - threshold or adaptive
  - Level selection for adaptive threshold interrupts
  - Persistence (Period) level selection for interrupts
  
This driver also uses the IIO GTS Helpers Namespace for Scales, Gains
and Integration time implementation.

root@stm32mp1:~# tree -I 'dev|name|of_node|power|subsystem|uevent' \
> /sys/bus/iio/devices/iio:device1/
/sys/bus/iio/devices/iio:device1/
|-- events
|   |-- in_illuminance_thresh_either_en
|   |-- in_intensity_clear_thresh_either_en
|   |-- thresh_adaptive_either_en
|   |-- thresh_adaptive_either_value
|   |-- thresh_adaptive_either_values_available
|   |-- thresh_either_period
|   |-- thresh_either_period_available
|   |-- thresh_falling_value
|   `-- thresh_rising_value
|-- in_illuminance_raw
|-- in_intensity_clear_raw
|-- integration_time
|-- integration_time_available
|-- sampling_frequency
|-- sampling_frequency_available
|-- scale
|-- scale_available
`-- waiting_for_supplier

1 directory, 18 files

v1 -> v2
  - apds9306 driver
   - Fixed kernel build failure due to probe_new(), replaced with probe()
v0 -> v1
  - dt_bindings
   - Squashed apds9300 and apds9600 dt bindings, added apds9306 bindings on
     top of that
   - Added detailed commit message for dt_bindings
  - apds9306 driver
   - Fixes as per review
   - Not disabling the regmap internal lock
   - Removing processed attribute for als channel which exposes raw values
   - Modified the iio gts scale implementation for above change
   - Not implementing a fallback and warning for compatibility and part ID 
     mismatch as suggested by Matti and Jonathan as Rob insisted on having
     a single compatible string for the driver (if the device can power up
     with a single compatible string, which it does).

Subhajit Ghosh (2):
  dt-bindings: iio: light: Avago APDS9306
  iio: light: Add support for APDS9306 Light Sensor

 .../bindings/iio/light/avago,apds9300.yaml    |   35 +-
 .../bindings/iio/light/avago,apds9960.yaml    |   44 -
 drivers/iio/light/Kconfig                     |   12 +
 drivers/iio/light/Makefile                    |    1 +
 drivers/iio/light/apds9306.c                  | 1326 +++++++++++++++++
 5 files changed, 1369 insertions(+), 49 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds9960.yaml
 create mode 100644 drivers/iio/light/apds9306.c


base-commit: 611da07b89fdd53f140d7b33013f255bf0ed8f34
-- 
2.34.1

[PATCH v2 1/2] dt-bindings: iio: light: Avago APDS9306

[Subhajit Ghosh]

v1 -> v2
- No change

v0 -> v1
- Squashing Avago (Broadcom) APDS9300 and APDS9960 schemas into one as
  they look similar
- Adding support for APDS9306 in the same schema file
- Adding mandatory interrupt property requirement for APDS9960 as per the
  driver's probe method which fails if interrupt bindings are not defined.
  Both APDS9300 and APDS9306 (this patch set) supports sensors with and
  without hardware interrupt bindings
- In the device tree example, replacing interrupt type number with macro
  from irq.h
- Updated the vin to vdd which is the same for all the three sensors
- Used proper "Datasheet:" tags

Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
---
 .../bindings/iio/light/avago,apds9300.yaml    | 35 ++++++++++++---
 .../bindings/iio/light/avago,apds9960.yaml    | 44 -------------------
 2 files changed, 30 insertions(+), 49 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds9960.yaml

diff --git a/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml b/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml
index 206af44f2c43..7a24a97d0594 100644
--- a/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml
+++ b/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml
@@ -4,17 +4,26 @@
 $id: http://devicetree.org/schemas/iio/light/avago,apds9300.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Avago APDS9300 ambient light sensor
+title: Avago Gesture, RGB, ALS and Proximity sensors
 
 maintainers:
   - Jonathan Cameron <jic23@kernel.org>
+  - Matt Ranostay <matt@ranostay.sg>
+  - Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
 
 description: |
-  Datasheet at https://www.avagotech.com/docs/AV02-1077EN
+  Avago (Broadcom) optical and proximity sensors with I2C interfaces.
+  Datasheet: https://docs.broadcom.com/doc/AV02-1077EN
+  Datasheet: https://docs.broadcom.com/doc/AV02-4191EN
+  Datasheet: https://docs.broadcom.com/doc/AV02-4755EN
 
 properties:
   compatible:
-    const: avago,apds9300
+    oneOf:
+      - enum:
+          - avago,apds9300
+          - avago,apds9306
+          - avago,apds9960
 
   reg:
     maxItems: 1
@@ -22,14 +31,30 @@ properties:
   interrupts:
     maxItems: 1
 
-additionalProperties: false
+  vdd-supply: true
 
 required:
   - compatible
   - reg
 
+allOf:
+  - $ref: ../common.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - avago,apds9960
+    then:
+      required:
+        - interrupts
+
+additionalProperties: false
+
 examples:
   - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
     i2c {
         #address-cells = <1>;
         #size-cells = <0>;
@@ -38,7 +63,7 @@ examples:
             compatible = "avago,apds9300";
             reg = <0x39>;
             interrupt-parent = <&gpio2>;
-            interrupts = <29 8>;
+            interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
         };
     };
 ...
diff --git a/Documentation/devicetree/bindings/iio/light/avago,apds9960.yaml b/Documentation/devicetree/bindings/iio/light/avago,apds9960.yaml
deleted file mode 100644
index f06e0fda5629..000000000000
--- a/Documentation/devicetree/bindings/iio/light/avago,apds9960.yaml
+++ /dev/null
@@ -1,44 +0,0 @@
-# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
-%YAML 1.2
----
-$id: http://devicetree.org/schemas/iio/light/avago,apds9960.yaml#
-$schema: http://devicetree.org/meta-schemas/core.yaml#
-
-title: Avago APDS9960 gesture/RGB/ALS/proximity sensor
-
-maintainers:
-  - Matt Ranostay <matt.ranostay@konsulko.com>
-
-description: |
-  Datasheet at https://www.avagotech.com/docs/AV02-4191EN
-
-properties:
-  compatible:
-    const: avago,apds9960
-
-  reg:
-    maxItems: 1
-
-  interrupts:
-    maxItems: 1
-
-additionalProperties: false
-
-required:
-  - compatible
-  - reg
-
-examples:
-  - |
-    i2c {
-        #address-cells = <1>;
-        #size-cells = <0>;
-
-        light-sensor@39 {
-            compatible = "avago,apds9960";
-            reg = <0x39>;
-            interrupt-parent = <&gpio1>;
-            interrupts = <16 1>;
-        };
-    };
-...
-- 
2.34.1

Re: [PATCH v2 1/2] dt-bindings: iio: light: Avago APDS9306

[Krzysztof Kozlowski]

On 27/10/2023 09:45, Subhajit Ghosh wrote:
> v1 -> v2
> - No change
> 


Missing commit msg.

> v0 -> v1
> - Squashing Avago (Broadcom) APDS9300 and APDS9960 schemas into one as
>   they look similar
> - Adding support for APDS9306 in the same schema file
> - Adding mandatory interrupt property requirement for APDS9960 as per the
>   driver's probe method which fails if interrupt bindings are not defined.
>   Both APDS9300 and APDS9306 (this patch set) supports sensors with and
>   without hardware interrupt bindings
> - In the device tree example, replacing interrupt type number with macro
>   from irq.h
> - Updated the vin to vdd which is the same for all the three sensors
> - Used proper "Datasheet:" tags

This is changelog, so after ---.

> 
> Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
> ---
>  .../bindings/iio/light/avago,apds9300.yaml    | 35 ++++++++++++---
>  .../bindings/iio/light/avago,apds9960.yaml    | 44 -------------------
>  2 files changed, 30 insertions(+), 49 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds9960.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml b/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml
> index 206af44f2c43..7a24a97d0594 100644
> --- a/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml
> +++ b/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml
> @@ -4,17 +4,26 @@
>  $id: http://devicetree.org/schemas/iio/light/avago,apds9300.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
> -title: Avago APDS9300 ambient light sensor
> +title: Avago Gesture, RGB, ALS and Proximity sensors
>  
>  maintainers:
>    - Jonathan Cameron <jic23@kernel.org>
> +  - Matt Ranostay <matt@ranostay.sg>
> +  - Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
>  
>  description: |
> -  Datasheet at https://www.avagotech.com/docs/AV02-1077EN
> +  Avago (Broadcom) optical and proximity sensors with I2C interfaces.
> +  Datasheet: https://docs.broadcom.com/doc/AV02-1077EN
> +  Datasheet: https://docs.broadcom.com/doc/AV02-4191EN
> +  Datasheet: https://docs.broadcom.com/doc/AV02-4755EN
>  
>  properties:
>    compatible:
> -    const: avago,apds9300
> +    oneOf:

Drop

> +      - enum:
> +          - avago,apds9300
> +          - avago,apds9306
> +          - avago,apds9960

I don't understand what is happening here. At all. Some compatibles are
moved, some are added, nothing explains why you are doing it.


Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: iio: light: Avago APDS9306

[Subhajit Ghosh]

>>   
>>   properties:
>>     compatible:
>> -    const: avago,apds9300
>> +    oneOf:
> 
> Drop
Sorry, do you mean I should not use the "oneOf"?
> 
>> +      - enum:
>> +          - avago,apds9300
>> +          - avago,apds9306
>> +          - avago,apds9960
> 
> I don't understand what is happening here. At all. Some compatibles are
> moved, some are added, nothing explains why you are doing it.
> 
> 
> Best regards,
> Krzysztof
> 
I have formatted the commit message wrongly, it will be fixed at once.
I am trying to add the support for all the three sensors in the same schema file.

Regards,
Subhajit Ghosh

Re: [PATCH v2 1/2] dt-bindings: iio: light: Avago APDS9306

[Krzysztof Kozlowski]

On 27/10/2023 10:55, Subhajit Ghosh wrote:
> 
>>>   
>>>   properties:
>>>     compatible:
>>> -    const: avago,apds9300
>>> +    oneOf:
>>
>> Drop
> Sorry, do you mean I should not use the "oneOf"?

Yep. You do not need it - you do not have more than one item.


>>
>>> +      - enum:
>>> +          - avago,apds9300
>>> +          - avago,apds9306
>>> +          - avago,apds9960
>>
>> I don't understand what is happening here. At all. Some compatibles are
>> moved, some are added, nothing explains why you are doing it.
>>
>>
>> Best regards,
>> Krzysztof
>>
> I have formatted the commit message wrongly, it will be fixed at once.
> I am trying to add the support for all the three sensors in the same schema file.

One commit does one logical thing. Moving things is one thing. Adding
things is another.

Best regards,
Krzysztof

Re: [PATCH v2 1/2] dt-bindings: iio: light: Avago APDS9306

[Jonathan Cameron]

On Fri, 27 Oct 2023 18:15:44 +1030
Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:

> v1 -> v2
> - No change
> 
> v0 -> v1
> - Squashing Avago (Broadcom) APDS9300 and APDS9960 schemas into one as
>   they look similar
> - Adding support for APDS9306 in the same schema file
The fact this belongs below the --- has already been mentioned.

These first two things are different types of change.
Patch 1: Combine bindings.
Patch 2: Add new device entry to combined binding
Patch 3: Add driver support.

> - Adding mandatory interrupt property requirement for APDS9960 as per the
>   driver's probe method which fails if interrupt bindings are not defined.
>   Both APDS9300 and APDS9306 (this patch set) supports sensors with and
>   without hardware interrupt bindings
> - In the device tree example, replacing interrupt type number with macro
>   from irq.h
> - Updated the vin to vdd which is the same for all the three sensors
> - Used proper "Datasheet:" tags
> 
> Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
> ---
>  .../bindings/iio/light/avago,apds9300.yaml    | 35 ++++++++++++---
>  .../bindings/iio/light/avago,apds9960.yaml    | 44 -------------------
>  2 files changed, 30 insertions(+), 49 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds9960.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml b/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml
> index 206af44f2c43..7a24a97d0594 100644
> --- a/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml
> +++ b/Documentation/devicetree/bindings/iio/light/avago,apds9300.yaml
> @@ -4,17 +4,26 @@
>  $id: http://devicetree.org/schemas/iio/light/avago,apds9300.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
> -title: Avago APDS9300 ambient light sensor
> +title: Avago Gesture, RGB, ALS and Proximity sensors
>  
>  maintainers:
>    - Jonathan Cameron <jic23@kernel.org>
> +  - Matt Ranostay <matt@ranostay.sg>
> +  - Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
>  
>  description: |
> -  Datasheet at https://www.avagotech.com/docs/AV02-1077EN
> +  Avago (Broadcom) optical and proximity sensors with I2C interfaces.
> +  Datasheet: https://docs.broadcom.com/doc/AV02-1077EN
> +  Datasheet: https://docs.broadcom.com/doc/AV02-4191EN
> +  Datasheet: https://docs.broadcom.com/doc/AV02-4755EN
>  
>  properties:
>    compatible:
> -    const: avago,apds9300
> +    oneOf:
Don't need the oneOf.  Lots of examples of this in tree!

       enum:
         - ...

> +      - enum:
> +          - avago,apds9300
> +          - avago,apds9306
> +          - avago,apds9960
>  
>    reg:
>      maxItems: 1
> @@ -22,14 +31,30 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> -additionalProperties: false
> +  vdd-supply: true
>  
>  required:
>    - compatible
>    - reg
>  
> +allOf:
> +  - $ref: ../common.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - avago,apds9960
> +    then:
> +      required:
> +        - interrupts

It is very rare that a binding for a sensor 'requires' interrupts.
They are more commonly a nice to have.

Note that the driver failing doesn't mean the binding should change.
Unless there is a very strong requirement (polling can't be done for
some reason - or device is largely pointless with out the interrupt)
then we normally keep it optional in the binding.

Someone can fix the driver later if they care about it!

Also, if this fix is correct, it needs to be a precursor patch handing
just this tightening of the binding.  The rest of the series then goes
on top of that.

Jonathan


> +
> +additionalProperties: false
> +
>  examples:
>    - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
>      i2c {
>          #address-cells = <1>;
>          #size-cells = <0>;
> @@ -38,7 +63,7 @@ examples:
>              compatible = "avago,apds9300";
>              reg = <0x39>;
>              interrupt-parent = <&gpio2>;
> -            interrupts = <29 8>;
> +            interrupts = <29 IRQ_TYPE_EDGE_FALLING>;
>          };
>      };
>  ...
> diff --git a/Documentation/devicetree/bindings/iio/light/avago,apds9960.yaml b/Documentation/devicetree/bindings/iio/light/avago,apds9960.yaml
> deleted file mode 100644
> index f06e0fda5629..000000000000
> --- a/Documentation/devicetree/bindings/iio/light/avago,apds9960.yaml
> +++ /dev/null
> @@ -1,44 +0,0 @@
> -# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> -%YAML 1.2
> ----
> -$id: http://devicetree.org/schemas/iio/light/avago,apds9960.yaml#
> -$schema: http://devicetree.org/meta-schemas/core.yaml#
> -
> -title: Avago APDS9960 gesture/RGB/ALS/proximity sensor
> -
> -maintainers:
> -  - Matt Ranostay <matt.ranostay@konsulko.com>
> -
> -description: |
> -  Datasheet at https://www.avagotech.com/docs/AV02-4191EN
> -
> -properties:
> -  compatible:
> -    const: avago,apds9960
> -
> -  reg:
> -    maxItems: 1
> -
> -  interrupts:
> -    maxItems: 1
> -
> -additionalProperties: false
> -
> -required:
> -  - compatible
> -  - reg
> -
> -examples:
> -  - |
> -    i2c {
> -        #address-cells = <1>;
> -        #size-cells = <0>;
> -
> -        light-sensor@39 {
> -            compatible = "avago,apds9960";
> -            reg = <0x39>;
> -            interrupt-parent = <&gpio1>;
> -            interrupts = <16 1>;
> -        };
> -    };
> -...

[PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[Subhajit Ghosh]

Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor with als
and clear channels with i2c interface. Hardware interrupt configuration is
optional. It is a low power device with 20 bit resolution and has
configurable adaptive interrupt mode and interrupt persistence mode.
The device also features inbuilt hardware gain, multiple integration time
selection options and sampling frequency selection options.

v1 -> v2
- Renamed probe_new to probe
- Removed module id table

v0 -> v1
- Fixed errors as per previous review
- Longer commit messages and descriptions
- Updated scale calculations as per iio gts scheme to export proper scale
  values and tables to userspace
- Removed processed attribute for the same channel for which raw is
  provided, instead, exporting proper scale and scale table to userspace so
  that userspace can do "(raw + offset) * scale" and derive Lux values
- Fixed IIO attribute range syntax
- Keeping the regmap lock enabled as the driver uses unlocked regfield
  accesses from interrupt handler
- Several levels of cleanups by placing guard mutexes in proper places and
  returning immediately in case of an error
- Using iio_device_claim_direct_mode() during raw reads so that
  configurations could not be changed during an adc conversion period
- In case of a powerdown error, returning immediately
- Removing the definition of direction of the hardware interrupt and
  leaving it on to device tree
- Adding the powerdown callback after doing device initialization
- Removed the regcache_cache_only() implementation

Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
---
 drivers/iio/light/Kconfig    |   12 +
 drivers/iio/light/Makefile   |    1 +
 drivers/iio/light/apds9306.c | 1326 ++++++++++++++++++++++++++++++++++
 3 files changed, 1339 insertions(+)
 create mode 100644 drivers/iio/light/apds9306.c

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 45edba797e4c..04e7d10f1470 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -73,6 +73,18 @@ config APDS9300
 	  To compile this driver as a module, choose M here: the
 	  module will be called apds9300.
 
+config APDS9306
+	tristate "Avago APDS9306 Ambient Light Sensor"
+	depends on I2C
+	select REGMAP_I2C
+	select IIO_GTS_HELPER
+	help
+	  If you say Y or M here, you get support for Avago APDS9306
+	  Ambient Light Sensor.
+
+	  If built as a dynamically linked module, it will be called
+	  apds9306.
+
 config APDS9960
 	tristate "Avago APDS9960 gesture/RGB/ALS/proximity sensor"
 	select REGMAP_I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index c0db4c4c36ec..ab94eac04db0 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_ADUX1020)		+= adux1020.o
 obj-$(CONFIG_AL3010)		+= al3010.o
 obj-$(CONFIG_AL3320A)		+= al3320a.o
 obj-$(CONFIG_APDS9300)		+= apds9300.o
+obj-$(CONFIG_APDS9306)		+= apds9306.o
 obj-$(CONFIG_APDS9960)		+= apds9960.o
 obj-$(CONFIG_AS73211)		+= as73211.o
 obj-$(CONFIG_BH1750)		+= bh1750.o
diff --git a/drivers/iio/light/apds9306.c b/drivers/iio/light/apds9306.c
new file mode 100644
index 000000000000..ae79d3b1129d
--- /dev/null
+++ b/drivers/iio/light/apds9306.c
@@ -0,0 +1,1326 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * APDS-9306/APDS-9306-065 Ambient Light Sensor
+ * I2C Address: 0x52
+ * Datasheet: https://docs.broadcom.com/doc/AV02-4755EN
+ *
+ * Copyright (C) 2023 Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
+ */
+
+#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/pm.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/units.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/iio-gts-helper.h>
+#include <linux/iio/events.h>
+#include <linux/iio/sysfs.h>
+
+#include <asm/unaligned.h>
+
+#define APDS9306_MAIN_CTRL		0x00
+#define APDS9306_ALS_MEAS_RATE		0x04
+#define APDS9306_ALS_GAIN		0x05
+#define APDS9306_PART_ID		0x06
+#define APDS9306_MAIN_STATUS		0x07
+#define APDS9306_CLEAR_DATA_0		0x0A
+#define APDS9306_CLEAR_DATA_1		0x0B
+#define APDS9306_CLEAR_DATA_2		0x0C
+#define APDS9306_ALS_DATA_0		0x0D
+#define APDS9306_ALS_DATA_1		0x0E
+#define APDS9306_ALS_DATA_2		0x0F
+#define APDS9306_INT_CFG		0x19
+#define APDS9306_INT_PERSISTENCE	0x1A
+#define APDS9306_ALS_THRES_UP_0		0x21
+#define APDS9306_ALS_THRES_UP_1		0x22
+#define APDS9306_ALS_THRES_UP_2		0x23
+#define APDS9306_ALS_THRES_LOW_0	0x24
+#define APDS9306_ALS_THRES_LOW_1	0x25
+#define APDS9306_ALS_THRES_LOW_2	0x26
+#define APDS9306_ALS_THRES_VAR		0x27
+
+#define APDS9306_ALS_INT_STAT_MASK	BIT(4)
+#define APDS9306_ALS_DATA_STAT_MASK	BIT(3)
+
+#define APDS9306_ALS_THRES_VAL_MAX	0xFFFFF
+#define APDS9306_ALS_THRES_VAR_VAL_MAX	7
+#define APDS9306_ALS_PERSIST_VAL_MAX	15
+#define APDS9306_ALS_READ_DATA_DELAY_US	20000
+
+enum apds9306_power_states {
+	STANDBY,
+	ACTIVE,
+};
+
+/**
+ * struct part_id_gts_multiplier - Part no. & corresponding gts multiplier
+ * @part_id: Part ID of the device
+ * @max_scale_int: Multiplier for iio_init_iio_gts()
+ * @max_scale_nano: Multiplier for iio_init_iio_gts()
+ */
+struct part_id_gts_multiplier {
+	int part_id;
+	int max_scale_int;
+	int max_scale_nano;
+};
+
+/*
+ * As per the datasheet, at HW Gain = 3x, Integration time 100mS (32x),
+ * typical 2000 ADC counts are observed for 49.8 uW per sq cm (340.134 lux)
+ * for apds9306 and 43 uW per sq cm (293.69 lux) for apds9306-065.
+ * Assuming lux per count is linear across all integration time ranges.
+ *
+ * Lux = (raw + offset) * scale; offset can be any value by userspace.
+ * HG = Hardware Gain; ITG = Gain by changing integration time.
+ * Scale table by IIO GTS Helpers = (1 / HG) * (1 / ITG) * Multiplier.
+ *
+ * The Lux values provided in the datasheet are at ITG=32x and HG=3x,
+ * at typical 2000 count.
+ *
+ * Lux per ADC count at 3x and 32x for apds9306 = 340.134 / 2000
+ * Lux per ADC count at 3x and 32x for apds9306-065 = 293.69 / 2000
+ *
+ * The Multiplier for the scale table provided to userspace:
+ * IIO GTS scale Multiplier for apds9306 = (340.134 / 2000) * 32 * 3
+ * IIO GTS scale Multiplier for apds9306-065 = (293.69 / 2000) * 32 * 3
+ */
+static struct part_id_gts_multiplier apds9306_gts_mul[] = {
+	{
+		.part_id = 0xB1,
+		.max_scale_int = 16,
+		.max_scale_nano = 3264320,
+	}, {
+		.part_id = 0xB3,
+		.max_scale_int = 14,
+		.max_scale_nano = 9712000,
+	},
+};
+
+/**
+ * apds9306_repeat_rate_freq - Sampling Frequency in uHz
+ */
+static const int apds9306_repeat_rate_freq[][2] = {
+	{40, 0},
+	{20, 0},
+	{10, 0},
+	{5,  0},
+	{2,  0},
+	{1,  0},
+	{0, 500000},
+};
+
+/**
+ * apds9306_repeat_rate_period - Sampling period in uSec
+ */
+static const int apds9306_repeat_rate_period[] = {
+	25000, 50000, 100000, 200000, 500000, 1000000, 2000000
+};
+static_assert(ARRAY_SIZE(apds9306_repeat_rate_freq) ==
+	      ARRAY_SIZE(apds9306_repeat_rate_period));
+
+/**
+ * struct apds9306_data - apds9306 private data and registers definitions
+ *
+ * All regfield definitions are named exactly according to datasheet for easy
+ * search
+ *
+ * @dev:	Pointer to the device structure
+ * @gts:	IIO Gain Time Scale structure
+ * @mutex:	Lock for protecting register access, adc reads and power
+ * @regmap:	Regmap structure pointer
+ * @regfield_sw_reset:	Reg: MAIN_CTRL, Field: SW_Reset
+ * @regfield_en:	Reg: MAIN_CTRL, Field: ALS_EN
+ * @regfield_intg_time:	Reg: ALS_MEAS_RATE, Field: ALS Resolution/Bit Width
+ * @regfield_repeat_rate:	Reg: ALS_MEAS_RATE, Field: ALS Measurement Rate
+ * @regfield_scale:	Reg: ALS_GAIN, Field: ALS Gain Range
+ * @regfield_int_src:	Reg: INT_CFG, Field: ALS Interrupt Source
+ * @regfield_int_thresh_var_en:	Reg: INT_CFG, Field: ALS Var Interrupt Mode
+ * @regfield_int_en:	Reg: INT_CFG, Field: ALS Interrupt Enable
+ * @regfield_int_persist_val:	Reg: INT_PERSISTENCE, Field: ALS_PERSIST
+ * @regfield_int_thresh_var_val:	Reg: ALS_THRSH_VAR, Field: ALS_THRES_VAR
+ * @nlux_per_count:	nano lux per ADC count for a particular model
+ * @read_data_available:	Flag set by IRQ handler for ADC data available
+ * @intg_time_idx:	Array index for integration times
+ * @repeat_rate_idx:	Array index for sampling frequency
+ * @gain_idx:	Array index for gain
+ * @int_ch:	Currently selected Interrupt channel
+ */
+struct apds9306_data {
+	struct device *dev;
+	struct iio_gts gts;
+	/*
+	 * Protects device settings changes where some calculations are required
+	 * before or after setting or getting the raw settings values from regmap
+	 * writes or reads respectively.
+	 */
+	struct mutex mutex;
+
+	struct regmap *regmap;
+	struct regmap_field *regfield_sw_reset;
+	struct regmap_field *regfield_en;
+	struct regmap_field *regfield_intg_time;
+	struct regmap_field *regfield_repeat_rate;
+	struct regmap_field *regfield_scale;
+	struct regmap_field *regfield_int_src;
+	struct regmap_field *regfield_int_thresh_var_en;
+	struct regmap_field *regfield_int_en;
+	struct regmap_field *regfield_int_persist_val;
+	struct regmap_field *regfield_int_thresh_var_val;
+
+	int nlux_per_count;
+	int read_data_available;
+	u8 intg_time_idx;
+	u8 repeat_rate_idx;
+	u8 gain_idx;
+	u8 int_ch;
+};
+
+/*
+ * Available scales with gain 1x - 18x, timings 3.125, 25, 50, 100, 200,
+ * 400 mS
+ * Time impacts to gain: 1x, 8x, 16x, 32x, 64x, 128x
+ */
+
+#define APDS9306_GSEL_1X	0x00
+#define APDS9306_GSEL_3X	0x01
+#define APDS9306_GSEL_6X	0x02
+#define APDS9306_GSEL_9X	0x03
+#define APDS9306_GSEL_18X	0x04
+
+static const struct iio_gain_sel_pair apds9306_gains[] = {
+	GAIN_SCALE_GAIN(1, APDS9306_GSEL_1X),
+	GAIN_SCALE_GAIN(3, APDS9306_GSEL_3X),
+	GAIN_SCALE_GAIN(6, APDS9306_GSEL_6X),
+	GAIN_SCALE_GAIN(9, APDS9306_GSEL_9X),
+	GAIN_SCALE_GAIN(18, APDS9306_GSEL_18X),
+};
+
+#define APDS9306_MEAS_MODE_400MS	0x00
+#define APDS9306_MEAS_MODE_200MS	0x01
+#define APDS9306_MEAS_MODE_100MS	0x02
+#define APDS9306_MEAS_MODE_50MS		0x03
+#define APDS9306_MEAS_MODE_25MS		0x04
+#define APDS9306_MEAS_MODE_3125US	0x05
+
+static const struct iio_itime_sel_mul apds9306_itimes[] = {
+	GAIN_SCALE_ITIME_US(400000, APDS9306_MEAS_MODE_400MS, 128),
+	GAIN_SCALE_ITIME_US(200000, APDS9306_MEAS_MODE_200MS, 64),
+	GAIN_SCALE_ITIME_US(100000, APDS9306_MEAS_MODE_100MS, 32),
+	GAIN_SCALE_ITIME_US(50000, APDS9306_MEAS_MODE_50MS, 16),
+	GAIN_SCALE_ITIME_US(25000, APDS9306_MEAS_MODE_25MS, 8),
+	GAIN_SCALE_ITIME_US(3125, APDS9306_MEAS_MODE_3125US, 1),
+};
+
+static struct iio_event_spec apds9306_event_spec_als[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
+	}, {
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
+	}, {
+		.type = IIO_EV_TYPE_THRESH,
+		.mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD),
+	}, {
+		.type = IIO_EV_TYPE_THRESH_ADAPTIVE,
+		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
+			BIT(IIO_EV_INFO_ENABLE),
+	}, {
+		.type = IIO_EV_TYPE_THRESH,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+	},
+};
+
+static struct iio_event_spec apds9306_event_spec_clear[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+	},
+};
+
+#define APDS9306_CHANNEL(_type) \
+	.type = _type, \
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) | \
+		BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+	.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | \
+		BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+
+static struct iio_chan_spec apds9306_channels_with_events[] = {
+	{
+		APDS9306_CHANNEL(IIO_LIGHT)
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.event_spec = apds9306_event_spec_als,
+		.num_event_specs = ARRAY_SIZE(apds9306_event_spec_als),
+	}, {
+		APDS9306_CHANNEL(IIO_INTENSITY)
+		.channel2 = IIO_MOD_LIGHT_CLEAR,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.modified = 1,
+		.event_spec = apds9306_event_spec_clear,
+		.num_event_specs = ARRAY_SIZE(apds9306_event_spec_clear),
+	},
+};
+
+static struct iio_chan_spec apds9306_channels_without_events[] = {
+	{
+		APDS9306_CHANNEL(IIO_LIGHT)
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	}, {
+		APDS9306_CHANNEL(IIO_INTENSITY)
+		.channel2 = IIO_MOD_LIGHT_CLEAR,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.modified = 1,
+	},
+};
+
+/* INT_PERSISTENCE available */
+IIO_CONST_ATTR(thresh_either_period_available, "[0 1 15]");
+
+/* ALS_THRESH_VAR available */
+IIO_CONST_ATTR(thresh_adaptive_either_values_available, "[0 1 7]");
+
+static struct attribute *apds9306_event_attributes[] = {
+	&iio_const_attr_thresh_either_period_available.dev_attr.attr,
+	&iio_const_attr_thresh_adaptive_either_values_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group apds9306_event_attr_group = {
+	.attrs = apds9306_event_attributes,
+};
+
+static const struct regmap_range apds9306_readable_ranges[] = {
+	regmap_reg_range(APDS9306_MAIN_CTRL, APDS9306_ALS_THRES_VAR)
+};
+
+static const struct regmap_range apds9306_writable_ranges[] = {
+	regmap_reg_range(APDS9306_MAIN_CTRL, APDS9306_ALS_GAIN),
+	regmap_reg_range(APDS9306_INT_CFG, APDS9306_ALS_THRES_VAR)
+};
+
+static const struct regmap_range apds9306_volatile_ranges[] = {
+	regmap_reg_range(APDS9306_MAIN_STATUS, APDS9306_MAIN_STATUS),
+	regmap_reg_range(APDS9306_CLEAR_DATA_0, APDS9306_ALS_DATA_2)
+};
+
+static const struct regmap_range apds9306_precious_ranges[] = {
+	regmap_reg_range(APDS9306_MAIN_STATUS, APDS9306_MAIN_STATUS)
+};
+
+static const struct regmap_access_table apds9306_readable_table = {
+	.yes_ranges = apds9306_readable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(apds9306_readable_ranges)
+};
+
+static const struct regmap_access_table apds9306_writable_table = {
+	.yes_ranges = apds9306_writable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(apds9306_writable_ranges)
+};
+
+static const struct regmap_access_table apds9306_volatile_table = {
+	.yes_ranges = apds9306_volatile_ranges,
+	.n_yes_ranges = ARRAY_SIZE(apds9306_volatile_ranges)
+};
+
+static const struct regmap_access_table apds9306_precious_table = {
+	.yes_ranges = apds9306_precious_ranges,
+	.n_yes_ranges = ARRAY_SIZE(apds9306_precious_ranges)
+};
+
+static const struct regmap_config apds9306_regmap = {
+	.name = "apds9306_regmap",
+	.reg_bits = 8,
+	.val_bits = 8,
+	.rd_table = &apds9306_readable_table,
+	.wr_table = &apds9306_writable_table,
+	.volatile_table = &apds9306_volatile_table,
+	.precious_table = &apds9306_precious_table,
+	.max_register = APDS9306_ALS_THRES_VAR,
+	.cache_type = REGCACHE_RBTREE,
+	/*
+	 * Leaving the regmap lock enabled as regfield accesses are everywhere
+	 * which are read modify writes and data mutex is not used in the
+	 * interrupt handler.
+	 */
+};
+
+static const struct reg_field apds9306_regfield_sw_reset =
+	REG_FIELD(APDS9306_MAIN_CTRL, 4, 4);
+
+static const struct reg_field apds9306_regfield_en =
+	REG_FIELD(APDS9306_MAIN_CTRL, 1, 1);
+
+static const struct reg_field apds9306_regfield_intg_time =
+	REG_FIELD(APDS9306_ALS_MEAS_RATE, 4, 6);
+
+static const struct reg_field apds9306_regfield_repeat_rate =
+	REG_FIELD(APDS9306_ALS_MEAS_RATE, 0, 2);
+
+static const struct reg_field apds9306_regfield_scale =
+	REG_FIELD(APDS9306_ALS_GAIN, 0, 2);
+
+static const struct reg_field apds9306_regfield_int_src =
+	REG_FIELD(APDS9306_INT_CFG, 4, 5);
+
+static const struct reg_field apds9306_regfield_int_thresh_var_en =
+	REG_FIELD(APDS9306_INT_CFG, 3, 3);
+
+static const struct reg_field apds9306_regfield_int_en =
+	REG_FIELD(APDS9306_INT_CFG, 2, 2);
+
+static const struct reg_field apds9306_regfield_int_persist_val =
+	REG_FIELD(APDS9306_INT_PERSISTENCE, 4, 7);
+
+static const struct reg_field apds9306_regfield_int_thresh_var_val =
+	REG_FIELD(APDS9306_ALS_THRES_VAR, 0, 2);
+
+static int apds9306_regfield_init(struct apds9306_data *data)
+{
+	struct device *dev = data->dev;
+	struct regmap *regmap = data->regmap;
+
+	data->regfield_sw_reset = devm_regmap_field_alloc(dev, regmap,
+			apds9306_regfield_sw_reset);
+	if (IS_ERR(data->regfield_sw_reset))
+		return PTR_ERR(data->regfield_sw_reset);
+
+	data->regfield_en = devm_regmap_field_alloc(dev, regmap,
+			apds9306_regfield_en);
+	if (IS_ERR(data->regfield_en))
+		return PTR_ERR(data->regfield_en);
+
+	data->regfield_intg_time = devm_regmap_field_alloc(dev, regmap,
+			apds9306_regfield_intg_time);
+	if (IS_ERR(data->regfield_intg_time))
+		return PTR_ERR(data->regfield_intg_time);
+
+	data->regfield_repeat_rate = devm_regmap_field_alloc(dev, regmap,
+			apds9306_regfield_repeat_rate);
+	if (IS_ERR(data->regfield_repeat_rate))
+		return PTR_ERR(data->regfield_repeat_rate);
+
+	data->regfield_scale = devm_regmap_field_alloc(dev, regmap,
+			apds9306_regfield_scale);
+	if (IS_ERR(data->regfield_scale))
+		return PTR_ERR(data->regfield_scale);
+
+	data->regfield_int_src = devm_regmap_field_alloc(dev, regmap,
+			apds9306_regfield_int_src);
+	if (IS_ERR(data->regfield_int_src))
+		return PTR_ERR(data->regfield_int_src);
+
+	data->regfield_int_thresh_var_en = devm_regmap_field_alloc(dev, regmap,
+			apds9306_regfield_int_thresh_var_en);
+	if (IS_ERR(data->regfield_int_thresh_var_en))
+		return PTR_ERR(data->regfield_int_thresh_var_en);
+
+	data->regfield_int_en = devm_regmap_field_alloc(dev, regmap,
+			apds9306_regfield_int_en);
+	if (IS_ERR(data->regfield_int_en))
+		return PTR_ERR(data->regfield_int_en);
+
+	data->regfield_int_persist_val = devm_regmap_field_alloc(dev, regmap,
+			apds9306_regfield_int_persist_val);
+	if (IS_ERR(data->regfield_int_persist_val))
+		return PTR_ERR(data->regfield_int_en);
+
+	data->regfield_int_thresh_var_val = devm_regmap_field_alloc(dev, regmap,
+			apds9306_regfield_int_thresh_var_val);
+	if (IS_ERR(data->regfield_int_thresh_var_val))
+		return PTR_ERR(data->regfield_int_thresh_var_en);
+
+	return 0;
+}
+
+static int apds9306_power_state(struct apds9306_data *data,
+				enum apds9306_power_states state)
+{
+	int ret;
+
+	/* Reset not included as it causes ugly I2C bus error */
+	switch (state) {
+	case STANDBY:
+		return regmap_field_write(data->regfield_en, 0);
+	case ACTIVE:
+		ret = regmap_field_write(data->regfield_en, 1);
+		if (ret)
+			return ret;
+		/* 5ms wake up time */
+		usleep_range(5000, 10000);
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int apds9306_runtime_power(struct apds9306_data *data, int en)
+{
+	struct device *dev = data->dev;
+	int ret;
+
+	if (en) {
+		ret = pm_runtime_resume_and_get(dev);
+		if (ret < 0) {
+			dev_err(dev, "runtime resume failed: %d\n", ret);
+			return ret;
+		}
+		return 0;
+	}
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+	return 0;
+}
+
+static int apds9306_read_data(struct apds9306_data *data, int *val, int reg)
+{
+	struct device *dev = data->dev;
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	int ret, delay, intg_time, status = 0;
+	u8 buff[3];
+
+	ret = apds9306_runtime_power(data, 1);
+	if (ret)
+		return ret;
+
+	intg_time = iio_gts_find_int_time_by_sel(&data->gts,
+						 data->intg_time_idx);
+	if (intg_time < 0)
+		delay = apds9306_repeat_rate_period[data->repeat_rate_idx];
+
+	/*
+	 * Whichever is greater - integration time period or
+	 * sampling period.
+	 */
+	delay = max(intg_time,
+		    apds9306_repeat_rate_period[data->repeat_rate_idx]);
+
+
+	/*
+	 * Clear stale data flag that might have been set by the interrupt
+	 * handler if it got data available flag set in the status reg.
+	 */
+	data->read_data_available = 0;
+
+	/*
+	 * If this function runs parallel with the interrupt handler, either
+	 * this reads and clears the status registers or the interrupt handler
+	 * does. The interrupt handler sets a flag for read data available
+	 * in our private structure which we read here.
+	 */
+	ret = regmap_read_poll_timeout(data->regmap, APDS9306_MAIN_STATUS,
+				status, (status & (APDS9306_ALS_DATA_STAT_MASK |
+				APDS9306_ALS_INT_STAT_MASK)) ||
+				data->read_data_available,
+				APDS9306_ALS_READ_DATA_DELAY_US, delay * 2);
+
+	if (ret)
+		return ret;
+
+	/* If we reach here before the interrupt handler we push an event */
+	if ((status & APDS9306_ALS_INT_STAT_MASK)) {
+		iio_push_event(indio_dev, IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
+			       data->int_ch,
+			       IIO_EV_TYPE_THRESH, IIO_EV_DIR_EITHER),
+			       iio_get_time_ns(indio_dev));
+	}
+
+	if ((status & APDS9306_ALS_DATA_STAT_MASK) ||
+		data->read_data_available) {
+		ret = regmap_bulk_read(data->regmap, reg, buff, sizeof(buff));
+		if (ret) {
+			dev_err(dev, "read data failed\n");
+			return ret;
+		}
+		*val = get_unaligned_le24(&buff);
+	}
+
+	return apds9306_runtime_power(data, 0);
+}
+
+static int apds9306_intg_time_get(struct apds9306_data *data, int *val2)
+{
+	int ret;
+
+	ret = iio_gts_find_int_time_by_sel(&data->gts, data->intg_time_idx);
+	if (ret < 0)
+		return ret;
+
+	*val2 = ret;
+
+	return 0;
+}
+
+static int apds9306_intg_time_set_hw(struct apds9306_data *data, int sel)
+{
+	int ret;
+
+	ret = regmap_field_write(data->regfield_intg_time, sel);
+	if (ret)
+		return ret;
+
+	data->intg_time_idx = sel;
+
+	return ret;
+}
+
+static int apds9306_gain_set_hw(struct apds9306_data *data, int sel)
+{
+	int ret;
+
+	ret = regmap_field_write(data->regfield_scale, sel);
+	if (ret)
+		return ret;
+
+	data->gain_idx = sel;
+
+	return ret;
+}
+
+static int apds9306_intg_time_set(struct apds9306_data *data, int val2)
+{
+	struct device *dev = data->dev;
+	int ret, intg_old, gain_old, gain_new, gain_new_closest;
+	bool ok;
+
+	if (!iio_gts_valid_time(&data->gts, val2)) {
+		dev_err(dev, "Unsupported integration time %u\n", val2);
+		return ret;
+	}
+
+	intg_old = iio_gts_find_int_time_by_sel(&data->gts,
+						data->intg_time_idx);
+	if (ret < 0)
+		return ret;
+
+	if (intg_old == val2)
+		return 0;
+
+	gain_old = iio_gts_find_gain_by_sel(&data->gts, data->gain_idx);
+	if (gain_old < 0)
+		return gain_old;
+
+	ret = iio_gts_find_new_gain_by_old_gain_time(&data->gts, gain_old,
+						     intg_old, val2, &gain_new);
+	if (gain_new < 0) {
+		dev_err(dev, "Unsupported gain with time\n");
+		return gain_new;
+	}
+
+	gain_new_closest = iio_find_closest_gain_low(&data->gts, gain_new, &ok);
+	if (gain_new_closest < 0) {
+		gain_new_closest = iio_gts_get_min_gain(&data->gts);
+		if (gain_new_closest < 0)
+			return gain_new_closest < 0;
+	}
+	if (!ok)
+		dev_dbg(dev, "Unable to find optimum gain, setting minimum");
+
+	ret = iio_gts_find_sel_by_int_time(&data->gts, val2);
+	if (ret < 0)
+		return ret;
+
+	ret = apds9306_intg_time_set_hw(data, ret);
+	if (ret)
+		return ret;
+
+	ret = iio_gts_find_sel_by_gain(&data->gts, gain_new_closest);
+	if (ret < 0)
+		return ret;
+
+	return apds9306_gain_set_hw(data, ret);
+}
+
+static int apds9306_sampling_freq_get(struct apds9306_data *data, int *val,
+				int *val2)
+{
+	if (data->repeat_rate_idx > ARRAY_SIZE(apds9306_repeat_rate_freq))
+		return -EINVAL;
+
+	*val = apds9306_repeat_rate_freq[data->repeat_rate_idx][0];
+	*val2 = apds9306_repeat_rate_freq[data->repeat_rate_idx][1];
+
+	return 0;
+}
+
+static int apds9306_sampling_freq_set(struct apds9306_data *data, int val,
+				int val2)
+{
+	int i, ret = -EINVAL;
+
+	for (i = 0; i < ARRAY_SIZE(apds9306_repeat_rate_freq); i++)
+		if (apds9306_repeat_rate_freq[i][0] == val &&
+				apds9306_repeat_rate_freq[i][1] == val2) {
+			ret = regmap_field_write(data->regfield_repeat_rate, i);
+			if (ret)
+				return ret;
+			data->repeat_rate_idx = i;
+			return ret;
+		}
+
+	return ret;
+}
+
+static int apds9306_scale_get(struct apds9306_data *data, int *val, int *val2)
+{
+	int gain, intg;
+
+	gain = iio_gts_find_gain_by_sel(&data->gts, data->gain_idx);
+	if (gain < 0)
+		return gain;
+
+	intg = iio_gts_find_int_time_by_sel(&data->gts, data->intg_time_idx);
+	if (intg < 0)
+		return intg;
+
+	return iio_gts_get_scale(&data->gts, gain, intg, val, val2);
+}
+
+static int apds9306_scale_set(struct apds9306_data *data, int val, int val2)
+{
+	int i, ret, time_sel, gain_sel;
+
+	/* Rounding up the last digit by one, otherwise matching table fails! */
+	if (val2 % 10)
+		val2 += 1;
+
+	ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts,
+				     data->intg_time_idx, val, val2, &gain_sel);
+	if (ret) {
+		for (i = 0; i < data->gts.num_itime; i++) {
+			time_sel = data->gts.itime_table[i].sel;
+
+			if (time_sel == data->intg_time_idx)
+				continue;
+
+			ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts,
+						time_sel, val, val2, &gain_sel);
+			if (!ret)
+				break;
+		}
+		if (ret)
+			return -EINVAL;
+
+		ret = apds9306_intg_time_set_hw(data, time_sel);
+		if (ret)
+			return ret;
+	}
+
+	return apds9306_gain_set_hw(data, gain_sel);
+}
+
+static int apds9306_event_period_get(struct apds9306_data *data, int *val)
+{
+	int period, ret;
+
+	ret = regmap_field_read(data->regfield_int_persist_val, &period);
+	if (ret)
+		return ret;
+
+	if (period > APDS9306_ALS_PERSIST_VAL_MAX)
+		return -EINVAL;
+
+	*val = period;
+
+	return ret;
+}
+
+static int apds9306_event_period_set(struct apds9306_data *data, int val)
+{
+	if (val < 0 || val > APDS9306_ALS_PERSIST_VAL_MAX)
+		return -EINVAL;
+
+	return regmap_field_write(data->regfield_int_persist_val, val);
+}
+
+static int apds9306_event_thresh_get(struct apds9306_data *data, int dir,
+				     int *val)
+{
+	int var, ret;
+	u8 buff[3];
+
+	if (dir == IIO_EV_DIR_RISING)
+		var = APDS9306_ALS_THRES_UP_0;
+	else if (dir == IIO_EV_DIR_FALLING)
+		var = APDS9306_ALS_THRES_LOW_0;
+	else
+		return -EINVAL;
+
+	ret = regmap_bulk_read(data->regmap, var, buff, sizeof(buff));
+	if (ret)
+		return ret;
+	*val = get_unaligned_le24(&buff);
+	return 0;
+}
+
+static int apds9306_event_thresh_set(struct apds9306_data *data, int dir,
+				     int val)
+{
+	int var;
+	u8 buff[3];
+
+	if (dir == IIO_EV_DIR_RISING)
+		var = APDS9306_ALS_THRES_UP_0;
+	else if (dir == IIO_EV_DIR_FALLING)
+		var = APDS9306_ALS_THRES_LOW_0;
+	else
+		return -EINVAL;
+
+	if (val < 0 || val > APDS9306_ALS_THRES_VAL_MAX)
+		return -EINVAL;
+
+	put_unaligned_le24(val, buff);
+	return regmap_bulk_write(data->regmap, var, buff, sizeof(buff));
+}
+
+static int apds9306_event_thresh_adaptive_get(struct apds9306_data *data,
+					      int *val)
+{
+	int thad, ret;
+
+	ret = regmap_field_read(data->regfield_int_thresh_var_val, &thad);
+	if (ret)
+		return ret;
+
+	if (thad > APDS9306_ALS_THRES_VAR_VAL_MAX)
+		return -EINVAL;
+
+	*val = thad;
+
+	return ret;
+}
+
+static int apds9306_event_thresh_adaptive_set(struct apds9306_data *data,
+		int val)
+{
+	if (val < 0 || val > APDS9306_ALS_THRES_VAR_VAL_MAX)
+		return -EINVAL;
+
+	return regmap_field_write(data->regfield_int_thresh_var_val, val);
+}
+
+static int apds9306_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, int *val,
+			     int *val2, long mask)
+{
+	struct apds9306_data *data = iio_priv(indio_dev);
+	int ret, reg;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (chan->channel2 == IIO_MOD_LIGHT_CLEAR)
+			reg = APDS9306_CLEAR_DATA_0;
+		else
+			reg = APDS9306_ALS_DATA_0;
+		/*
+		 * Changing device parameters during adc operation, resets
+		 * the ADC which has to avoided.
+		 */
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+		ret = apds9306_read_data(data, val, reg);
+		iio_device_release_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_INT_TIME:
+		ret = apds9306_intg_time_get(data, val2);
+		if (ret)
+			return ret;
+		*val = 0;
+		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = apds9306_sampling_freq_get(data, val, val2);
+		if (ret)
+			return ret;
+		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_SCALE:
+		ret = apds9306_scale_get(data, val, val2);
+		if (ret)
+			return ret;
+		return IIO_VAL_INT_PLUS_NANO;
+	default:
+		return -EINVAL;
+	}
+};
+
+static int apds9306_read_avail(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan,
+			       const int **vals, int *type, int *length,
+			       long mask)
+{
+	struct apds9306_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		return iio_gts_avail_times(&data->gts, vals, type, length);
+	case IIO_CHAN_INFO_SCALE:
+		return iio_gts_all_avail_scales(&data->gts, vals, type,
+						length);
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*length = ARRAY_SIZE(apds9306_repeat_rate_freq) * 2;
+		*vals = (const int *)apds9306_repeat_rate_freq;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int apds9306_write_raw_get_fmt(struct iio_dev *indio_dev,
+				      struct iio_chan_spec const *chan,
+				      long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		return IIO_VAL_INT_PLUS_NANO;
+	case IIO_CHAN_INFO_INT_TIME:
+		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int apds9306_write_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan, int val,
+			      int val2, long mask)
+{
+	struct apds9306_data *data = iio_priv(indio_dev);
+	int ret;
+
+	mutex_lock(&data->mutex);
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		if (!val)
+			ret = apds9306_intg_time_set(data, val2);
+		else
+			ret = -EINVAL;
+		break;
+	case IIO_CHAN_INFO_SCALE:
+		ret = apds9306_scale_set(data, val, val2);
+		break;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = apds9306_sampling_freq_set(data, val, val2);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static irqreturn_t apds9306_irq_handler(int irq, void *priv)
+{
+	struct iio_dev *indio_dev = priv;
+	struct apds9306_data *data = iio_priv(indio_dev);
+	int ret, status;
+
+	/*
+	 * The interrupt line is released and the interrupt flag is
+	 * cleared as a result of reading the status register. All the
+	 * status flags are cleared as a result of this read.
+	 */
+	ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS, &status);
+	if (ret < 0) {
+		dev_err(data->dev, "status reg read failed\n");
+		return IRQ_HANDLED;
+	}
+
+	if ((status & APDS9306_ALS_INT_STAT_MASK)) {
+		iio_push_event(indio_dev, IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
+			       data->int_ch, IIO_EV_TYPE_THRESH,
+			       IIO_EV_DIR_EITHER), iio_get_time_ns(indio_dev));
+	}
+
+	/*
+	 * If a one-shot read through sysfs is underway at the same time
+	 * as this interrupt handler is executing and a read data available
+	 * flag was set, this flag is set to inform read_poll_timeout()
+	 * to exit.
+	 */
+	if ((status & APDS9306_ALS_DATA_STAT_MASK))
+		data->read_data_available = 1;
+
+	return IRQ_HANDLED;
+}
+
+static int apds9306_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)
+{
+	struct apds9306_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (type) {
+	case IIO_EV_TYPE_THRESH:
+		mutex_lock(&data->mutex);
+		if (dir == IIO_EV_DIR_EITHER && info == IIO_EV_INFO_PERIOD)
+			ret = apds9306_event_period_get(data, val);
+		else
+			ret = apds9306_event_thresh_get(data, dir, val);
+		mutex_unlock(&data->mutex);
+		if (ret)
+			return ret;
+		return IIO_VAL_INT;
+	case IIO_EV_TYPE_THRESH_ADAPTIVE:
+		mutex_lock(&data->mutex);
+		ret = apds9306_event_thresh_adaptive_get(data, val);
+		mutex_unlock(&data->mutex);
+		if (ret)
+			return ret;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int apds9306_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)
+{
+	struct apds9306_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (type) {
+	case IIO_EV_TYPE_THRESH:
+		mutex_lock(&data->mutex);
+		if (dir == IIO_EV_DIR_EITHER && info == IIO_EV_INFO_PERIOD)
+			ret = apds9306_event_period_set(data, val);
+		else
+			ret = apds9306_event_thresh_set(data, dir, val);
+		mutex_unlock(&data->mutex);
+		return ret;
+	case IIO_EV_TYPE_THRESH_ADAPTIVE:
+		mutex_lock(&data->mutex);
+		ret = apds9306_event_thresh_adaptive_set(data, val);
+		mutex_unlock(&data->mutex);
+		return ret;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int apds9306_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 apds9306_data *data = iio_priv(indio_dev);
+	unsigned int int_en;
+	int ret;
+
+	mutex_lock(&data->mutex);
+	switch (type) {
+	case IIO_EV_TYPE_THRESH:
+		ret = regmap_field_read(data->regfield_int_en, &int_en);
+		if (ret)
+			break;
+		if (chan->type == IIO_LIGHT)
+			ret = int_en & data->int_ch;
+		else if (chan->type == IIO_INTENSITY)
+			ret = int_en & !data->int_ch;
+		else
+			ret = -EINVAL;
+		break;
+	case IIO_EV_TYPE_THRESH_ADAPTIVE:
+		ret = regmap_field_read(data->regfield_int_thresh_var_en,
+					&int_en);
+		if (ret)
+			break;
+		ret = int_en;
+		break;
+	default:
+		ret = -EINVAL;
+	}
+	mutex_unlock(&data->mutex);
+	return ret;
+}
+
+static int apds9306_write_event_config(struct iio_dev *indio_dev,
+				       const struct iio_chan_spec *chan,
+				       enum iio_event_type type,
+				       enum iio_event_direction dir,
+				       int state)
+{
+	struct apds9306_data *data = iio_priv(indio_dev);
+	int ret, int_sel;
+
+	state = !!state;
+	switch (type) {
+	case IIO_EV_TYPE_THRESH:
+		/*
+		 * If interrupt is enabled, the channel is set before enabling
+		 * the interrupt. In case of disable, no need to switch
+		 * channels. In case of different channel is selected while
+		 * interrupt in on, just changing the channel.
+		 */
+		if (state) {
+			if (chan->type == IIO_LIGHT)
+				int_sel = 1;
+			else if (chan->type == IIO_INTENSITY)
+				int_sel = 0;
+			else
+				return -EINVAL;
+
+			ret = regmap_field_write(data->regfield_int_src,
+						 int_sel);
+			if (ret)
+				return ret;
+			data->int_ch = int_sel;
+		}
+		ret = regmap_field_read(data->regfield_int_en, &int_sel);
+		if (ret)
+			return ret;
+		if (int_sel == state)
+			return 0;
+		mutex_lock(&data->mutex);
+		ret = regmap_field_write(data->regfield_int_en, state);
+		if (!ret)
+			ret = apds9306_runtime_power(data, state);
+		mutex_unlock(&data->mutex);
+		return ret;
+	case IIO_EV_TYPE_THRESH_ADAPTIVE:
+		return regmap_field_write(data->regfield_int_thresh_var_en,
+					  state);
+	default:
+		return -EINVAL;
+	}
+}
+
+#define APDS9306_IIO_INFO \
+	.read_avail = apds9306_read_avail, \
+	.read_raw = apds9306_read_raw, \
+	.write_raw = apds9306_write_raw, \
+	.write_raw_get_fmt = apds9306_write_raw_get_fmt,
+
+static const struct iio_info apds9306_info_no_events = {
+	APDS9306_IIO_INFO
+};
+
+static const struct iio_info apds9306_info = {
+	APDS9306_IIO_INFO
+	.event_attrs = &apds9306_event_attr_group,
+	.read_event_value = apds9306_read_event,
+	.write_event_value = apds9306_write_event,
+	.read_event_config = apds9306_read_event_config,
+	.write_event_config = apds9306_write_event_config,
+};
+
+static int apds9306_init_iio_gts(struct apds9306_data *data)
+{
+	int i, ret, part_id;
+
+	ret = regmap_read(data->regmap, APDS9306_PART_ID, &part_id);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < ARRAY_SIZE(apds9306_gts_mul); i++) {
+		if (part_id == apds9306_gts_mul[i].part_id)
+			return devm_iio_init_iio_gts(data->dev,
+				 apds9306_gts_mul[i].max_scale_int,
+				 apds9306_gts_mul[i].max_scale_nano,
+				 apds9306_gains, ARRAY_SIZE(apds9306_gains),
+				 apds9306_itimes, ARRAY_SIZE(apds9306_itimes),
+				 &data->gts);
+	}
+
+	return -ENXIO;
+}
+
+static void apds9306_powerdown(void *ptr)
+{
+	struct apds9306_data *data = (struct apds9306_data *)ptr;
+	int ret;
+
+	/* Disable interrupts */
+	ret = regmap_field_write(data->regfield_int_thresh_var_en, 0);
+	if (ret)
+		return;
+	ret = regmap_field_write(data->regfield_int_en, 0);
+	if (ret)
+		return;
+	/* Put the device in standby mode */
+	apds9306_power_state(data, STANDBY);
+}
+
+static int apds9306_init_device(struct apds9306_data *data)
+{
+	struct device *dev = data->dev;
+	int ret;
+
+	ret = apds9306_power_state(data, ACTIVE);
+	if (ret)
+		return ret;
+
+	ret = pm_runtime_set_active(dev);
+	if (ret)
+		return ret;
+
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return ret;
+
+	pm_runtime_set_autosuspend_delay(dev, 5000);
+	pm_runtime_use_autosuspend(dev);
+
+	/* Init GTS multiplier values according to Part ID */
+	ret = apds9306_init_iio_gts(data);
+	if (ret)
+		return ret;
+
+	/* Integration time: 100 msec */
+	ret = apds9306_intg_time_set_hw(data, 2);
+	if (ret)
+		return ret;
+
+	/* Sampling frequency: 100 msec, 10 Hz */
+	ret = apds9306_sampling_freq_set(data, 10, 0);
+	if (ret)
+		return ret;
+
+	/* Scale: x3 */
+	ret = apds9306_gain_set_hw(data, 1);
+	if (ret)
+		return ret;
+
+	/* Interrupt source channel: als */
+	ret = regmap_field_write(data->regfield_int_src, 1);
+	if (ret)
+		return ret;
+	data->int_ch = 1;
+
+	/* Interrupts disabled */
+	ret = regmap_field_write(data->regfield_int_en, 0);
+	if (ret)
+		return ret;
+
+	/* Threshold Variance disabled */
+	return regmap_field_write(data->regfield_int_thresh_var_en, 0);
+}
+
+static int apds9306_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct apds9306_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+
+	mutex_init(&data->mutex);
+
+	data->regmap = devm_regmap_init_i2c(client, &apds9306_regmap);
+	if (IS_ERR(data->regmap))
+		return dev_err_probe(dev, PTR_ERR(data->regmap),
+				     "regmap initialization failed\n");
+
+	data->dev = dev;
+	i2c_set_clientdata(client, indio_dev);
+
+	ret = apds9306_regfield_init(data);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "regfield initialization failed\n");
+
+	ret = devm_regulator_get_enable(dev, "vdd");
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable regulator\n");
+
+	indio_dev->name = "apds9306";
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	if (client->irq) {
+		indio_dev->info = &apds9306_info;
+		indio_dev->channels = apds9306_channels_with_events;
+		indio_dev->num_channels =
+				ARRAY_SIZE(apds9306_channels_with_events);
+		ret = devm_request_threaded_irq(dev, client->irq, NULL,
+					apds9306_irq_handler, IRQF_ONESHOT,
+					"apds9306_event", indio_dev);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					"failed to assign interrupt.\n");
+	} else {
+		indio_dev->info = &apds9306_info_no_events;
+		indio_dev->channels = apds9306_channels_without_events;
+		indio_dev->num_channels =
+				ARRAY_SIZE(apds9306_channels_without_events);
+	}
+
+	ret = apds9306_init_device(data);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to init device\n");
+
+	ret = devm_add_action_or_reset(dev, apds9306_powerdown, data);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				"failed to add action on driver unwind\n");
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static int apds9306_runtime_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct apds9306_data *data = iio_priv(indio_dev);
+
+	return apds9306_power_state(data, STANDBY);
+}
+
+static int apds9306_runtime_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct apds9306_data *data = iio_priv(indio_dev);
+
+	return apds9306_power_state(data, ACTIVE);
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(apds9306_pm_ops,
+				 apds9306_runtime_suspend,
+				 apds9306_runtime_resume,
+				 NULL);
+
+static const struct of_device_id apds9306_of_match[] = {
+	{ .compatible = "avago,apds9306" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, apds9306_of_match);
+
+static struct i2c_driver apds9306_driver = {
+	.driver = {
+		.name = "apds9306",
+		.pm = pm_ptr(&apds9306_pm_ops),
+		.of_match_table = apds9306_of_match,
+		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
+	},
+	.probe = apds9306_probe,
+};
+module_i2c_driver(apds9306_driver);
+
+MODULE_AUTHOR("Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>");
+MODULE_DESCRIPTION("APDS9306 Ambient Light Sensor driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_GTS_HELPER);
-- 
2.34.1

Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[Krzysztof Kozlowski]

On 27/10/2023 09:45, Subhajit Ghosh wrote:
> Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor with als
> and clear channels with i2c interface. Hardware interrupt configuration is
> optional. It is a low power device with 20 bit resolution and has
> configurable adaptive interrupt mode and interrupt persistence mode.
> The device also features inbuilt hardware gain, multiple integration time
> selection options and sampling frequency selection options.
> 
> v1 -> v2
> - Renamed probe_new to probe
> - Removed module id table

This is fourth version of this patchset, not second, so v4.

Best regards,
Krzysztof

Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[Subhajit Ghosh]

On 27/10/23 18:43, Krzysztof Kozlowski wrote:
> On 27/10/2023 09:45, Subhajit Ghosh wrote:
>> Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor with als
>> and clear channels with i2c interface. Hardware interrupt configuration is
>> optional. It is a low power device with 20 bit resolution and has
>> configurable adaptive interrupt mode and interrupt persistence mode.
>> The device also features inbuilt hardware gain, multiple integration time
>> selection options and sampling frequency selection options.
>>
>> v1 -> v2
>> - Renamed probe_new to probe
>> - Removed module id table
> 
> This is fourth version of this patchset, not second, so v4.
> 
> Best regards,
> Krzysztof
> 
Hi Krzysztof,

1. This patch set adds support for APDS9306 Light sensor,
2. In v0 of this patch series I was told to combine the avago,apds9300.yaml
    and avago,apds9960.yaml as they look similar, then add my apds9306
    support into it.
3. I assumed that the squashing operation of apds9300 and apds9960 schemas
    are a separate operation and submitted a separate patch to do that, please
    follow the link
    Link: https://lore.kernel.org/all/20231019-hurry-eagle-0ffa95b1a026@spud/
4. Conor reviewed the patch and said that it would be better that I handle all
    these operations in apds9306 driver (this) patch series rather than submitting
    a new patch.
    "Ahh apologies then. The best course of action would likely be to include
     the patch merging the two bindings in your series adding the third user."
5. As per this patch series -- RFC->v0->v1-v2

I have formatted the commit messages wrongly which might be the source of all the
confusion. I'll fix it. Please let me know the best course of action, I am not well
versed with this process. Thank you for reviewing.

Regards,
Subhajit Ghosh

Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[Krzysztof Kozlowski]

On 27/10/2023 10:42, Subhajit Ghosh wrote:
> 4. Conor reviewed the patch and said that it would be better that I handle all
>     these operations in apds9306 driver (this) patch series rather than submitting
>     a new patch.
>     "Ahh apologies then. The best course of action would likely be to include
>      the patch merging the two bindings in your series adding the third user."
> 5. As per this patch series -- RFC->v0->v1-v2

RFC was the first version sent to mailing list. So after RFC there is
second version - v2. This is v4.

> 
> I have formatted the commit messages wrongly which might be the source of all the
> confusion. I'll fix it. Please let me know the best course of action, I am not well
> versed with this process. Thank you for reviewing.

Best regards,
Krzysztof

Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[Subhajit Ghosh]

On 27/10/23 21:34, Krzysztof Kozlowski wrote:
> On 27/10/2023 10:42, Subhajit Ghosh wrote:
>> 4. Conor reviewed the patch and said that it would be better that I handle all
>>      these operations in apds9306 driver (this) patch series rather than submitting
>>      a new patch.
>>      "Ahh apologies then. The best course of action would likely be to include
>>       the patch merging the two bindings in your series adding the third user."
>> 5. As per this patch series -- RFC->v0->v1-v2
> 
> RFC was the first version sent to mailing list. So after RFC there is
> second version - v2. This is v4.

Acknowledging all your other comments. Appreciate your time and effort in reviewing
this. One last question on this - So what version should I use for the patchset
which I will submit next - "v3" or "v5" in the Subject of the emails?

Regards,
Subhajit Ghosh

Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[Jonathan Cameron]

On Fri, 27 Oct 2023 22:12:11 +1030
Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:

> On 27/10/23 21:34, Krzysztof Kozlowski wrote:
> > On 27/10/2023 10:42, Subhajit Ghosh wrote:  
> >> 4. Conor reviewed the patch and said that it would be better that I handle all
> >>      these operations in apds9306 driver (this) patch series rather than submitting
> >>      a new patch.
> >>      "Ahh apologies then. The best course of action would likely be to include
> >>       the patch merging the two bindings in your series adding the third user."
> >> 5. As per this patch series -- RFC->v0->v1-v2  
> > 
> > RFC was the first version sent to mailing list. So after RFC there is
> > second version - v2. This is v4.  
> 
> Acknowledging all your other comments. Appreciate your time and effort in reviewing
> this. One last question on this - So what version should I use for the patchset
> which I will submit next - "v3" or "v5" in the Subject of the emails?
Go with v5 and play it safe given possible confusion.

Numbering when there has previously been one or more RFC versions is always rather
confusing, but we tend not to have a v0!  So RFC-> v1 -> v2 -> V3 would have
been fine in my opinion, or
RFC -> V2 -> v3 
With a note in v2 cover letter saying changes from RFC to make it clear there
was not a separate v1.

Jonathan

> 
> Regards,
> Subhajit Ghosh
> 

Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[Andy Shevchenko]

On Fri, Oct 27, 2023 at 06:15:45PM +1030, Subhajit Ghosh wrote:
> Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor with als
> and clear channels with i2c interface. Hardware interrupt configuration is
> optional. It is a low power device with 20 bit resolution and has
> configurable adaptive interrupt mode and interrupt persistence mode.
> The device also features inbuilt hardware gain, multiple integration time
> selection options and sampling frequency selection options.

Please, do not send so often this big patches (~1.5kLoC!), let reviewers a bit
of time, please.

Since I have my message postponed, I finish review on the v1, and I believe
100% of those comments will be applicable here.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[Subhajit Ghosh]

> Please, do not send so often this big patches (~1.5kLoC!), let reviewers a bit
> of time, please.
> 
> Since I have my message postponed, I finish review on the v1, and I believe
> 100% of those comments will be applicable here.
> 
Hi Andy,
v1 broke the kernel build, which was bit embarrassing as I skipped compilation on
latest kernel and only tested on my embedded test bench, so I submitted v2 that early,
sorry about that, I understand, won't happen again. I am not in a hurry either!

Regards,
Subhajit Ghosh

Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[kernel test robot]

Hi Subhajit,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 611da07b89fdd53f140d7b33013f255bf0ed8f34]

url:    https://github.com/intel-lab-lkp/linux/commits/Subhajit-Ghosh/dt-bindings-iio-light-Avago-APDS9306/20231027-154954
base:   611da07b89fdd53f140d7b33013f255bf0ed8f34
patch link:    https://lore.kernel.org/r/20231027074545.6055-3-subhajit.ghosh%40tweaklogic.com
patch subject: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor
config: alpha-allmodconfig (https://download.01.org/0day-ci/archive/20231028/202310281420.see2fNLh-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231028/202310281420.see2fNLh-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/202310281420.see2fNLh-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/iio/light/apds9306.c:110: warning: cannot understand function prototype: 'const int apds9306_repeat_rate_freq[][2] = '
>> drivers/iio/light/apds9306.c:123: warning: cannot understand function prototype: 'const int apds9306_repeat_rate_period[] = '


vim +110 drivers/iio/light/apds9306.c

   106	
   107	/**
   108	 * apds9306_repeat_rate_freq - Sampling Frequency in uHz
   109	 */
 > 110	static const int apds9306_repeat_rate_freq[][2] = {
   111		{40, 0},
   112		{20, 0},
   113		{10, 0},
   114		{5,  0},
   115		{2,  0},
   116		{1,  0},
   117		{0, 500000},
   118	};
   119	
   120	/**
   121	 * apds9306_repeat_rate_period - Sampling period in uSec
   122	 */
 > 123	static const int apds9306_repeat_rate_period[] = {
   124		25000, 50000, 100000, 200000, 500000, 1000000, 2000000
   125	};
   126	static_assert(ARRAY_SIZE(apds9306_repeat_rate_freq) ==
   127		      ARRAY_SIZE(apds9306_repeat_rate_period));
   128	

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

Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[Jonathan Cameron]

On Fri, 27 Oct 2023 18:15:45 +1030
Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:

> Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor with als
> and clear channels with i2c interface. Hardware interrupt configuration is
> optional. It is a low power device with 20 bit resolution and has
> configurable adaptive interrupt mode and interrupt persistence mode.
> The device also features inbuilt hardware gain, multiple integration time
> selection options and sampling frequency selection options.

Hi Subhajit,


> 
Change log below the --- 

We don't generally want to end up with this information in the git log
and anything above the --- is used for the commit message.

Note that if you want to keep notes in your local git it is fine adding

Signed-of-by...

---

Version notes
etc


As then git am will drop them anyway when your patches are picked up.


> v1 -> v2
> - Renamed probe_new to probe
> - Removed module id table
> 
> v0 -> v1
> - Fixed errors as per previous review
> - Longer commit messages and descriptions
> - Updated scale calculations as per iio gts scheme to export proper scale
>   values and tables to userspace
> - Removed processed attribute for the same channel for which raw is
>   provided, instead, exporting proper scale and scale table to userspace so
>   that userspace can do "(raw + offset) * scale" and derive Lux values
> - Fixed IIO attribute range syntax
> - Keeping the regmap lock enabled as the driver uses unlocked regfield
>   accesses from interrupt handler
> - Several levels of cleanups by placing guard mutexes in proper places and
>   returning immediately in case of an error
> - Using iio_device_claim_direct_mode() during raw reads so that
>   configurations could not be changed during an adc conversion period
> - In case of a powerdown error, returning immediately
> - Removing the definition of direction of the hardware interrupt and
>   leaving it on to device tree
> - Adding the powerdown callback after doing device initialization
> - Removed the regcache_cache_only() implementation
> 
> Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>

More comments inline.

> diff --git a/drivers/iio/light/apds9306.c b/drivers/iio/light/apds9306.c
> new file mode 100644
> index 000000000000..ae79d3b1129d
> --- /dev/null
> +++ b/drivers/iio/light/apds9306.c
> @@ -0,0 +1,1326 @@

...

> +#define APDS9306_INT_CFG		0x19
> +#define APDS9306_INT_PERSISTENCE	0x1A
> +#define APDS9306_ALS_THRES_UP_0		0x21
> +#define APDS9306_ALS_THRES_UP_1		0x22
> +#define APDS9306_ALS_THRES_UP_2		0x23
> +#define APDS9306_ALS_THRES_LOW_0	0x24
> +#define APDS9306_ALS_THRES_LOW_1	0x25
> +#define APDS9306_ALS_THRES_LOW_2	0x26
> +#define APDS9306_ALS_THRES_VAR		0x27

Naming doesn't make it clear what is a field and what is a register address
_REG or _ADDR as a postfix for register addresses tends to make that clear.

> +
> +#define APDS9306_ALS_INT_STAT_MASK	BIT(4)
> +#define APDS9306_ALS_DATA_STAT_MASK	BIT(3)
Naming doesn't make it clear which register these are in and
it definitely should.

> +
> +#define APDS9306_ALS_THRES_VAL_MAX	0xFFFFF
> +#define APDS9306_ALS_THRES_VAR_VAL_MAX	7
> +#define APDS9306_ALS_PERSIST_VAL_MAX	15
> +#define APDS9306_ALS_READ_DATA_DELAY_US	20000
> +
> +enum apds9306_power_states {
> +	STANDBY,
> +	ACTIVE,

Effectively on vs off: I'd just use a boolean.

> +};

> +/**
> + * apds9306_repeat_rate_freq - Sampling Frequency in uHz
> + */
> +static const int apds9306_repeat_rate_freq[][2] = {
> +	{40, 0},
> +	{20, 0},
> +	{10, 0},
> +	{5,  0},
> +	{2,  0},
> +	{1,  0},
> +	{0, 500000},
> +};
> +
> +/**
> + * apds9306_repeat_rate_period - Sampling period in uSec
> + */
> +static const int apds9306_repeat_rate_period[] = {
> +	25000, 50000, 100000, 200000, 500000, 1000000, 2000000
> +};
> +static_assert(ARRAY_SIZE(apds9306_repeat_rate_freq) ==
> +	      ARRAY_SIZE(apds9306_repeat_rate_period));
> +
> +/**
> + * struct apds9306_data - apds9306 private data and registers definitions
> + *
> + * All regfield definitions are named exactly according to datasheet for easy
> + * search
> + *
> + * @dev:	Pointer to the device structure
> + * @gts:	IIO Gain Time Scale structure
> + * @mutex:	Lock for protecting register access, adc reads and power
> + * @regmap:	Regmap structure pointer
> + * @regfield_sw_reset:	Reg: MAIN_CTRL, Field: SW_Reset
> + * @regfield_en:	Reg: MAIN_CTRL, Field: ALS_EN
> + * @regfield_intg_time:	Reg: ALS_MEAS_RATE, Field: ALS Resolution/Bit Width
> + * @regfield_repeat_rate:	Reg: ALS_MEAS_RATE, Field: ALS Measurement Rate
> + * @regfield_scale:	Reg: ALS_GAIN, Field: ALS Gain Range
> + * @regfield_int_src:	Reg: INT_CFG, Field: ALS Interrupt Source
> + * @regfield_int_thresh_var_en:	Reg: INT_CFG, Field: ALS Var Interrupt Mode
> + * @regfield_int_en:	Reg: INT_CFG, Field: ALS Interrupt Enable
> + * @regfield_int_persist_val:	Reg: INT_PERSISTENCE, Field: ALS_PERSIST
> + * @regfield_int_thresh_var_val:	Reg: ALS_THRSH_VAR, Field: ALS_THRES_VAR
> + * @nlux_per_count:	nano lux per ADC count for a particular model
> + * @read_data_available:	Flag set by IRQ handler for ADC data available
> + * @intg_time_idx:	Array index for integration times
> + * @repeat_rate_idx:	Array index for sampling frequency
> + * @gain_idx:	Array index for gain
> + * @int_ch:	Currently selected Interrupt channel
> + */
> +struct apds9306_data {
> +	struct device *dev;
> +	struct iio_gts gts;
> +	/*
> +	 * Protects device settings changes where some calculations are required
> +	 * before or after setting or getting the raw settings values from regmap
> +	 * writes or reads respectively.
> +	 */
> +	struct mutex mutex;
> +
> +	struct regmap *regmap;
> +	struct regmap_field *regfield_sw_reset;
> +	struct regmap_field *regfield_en;
> +	struct regmap_field *regfield_intg_time;
> +	struct regmap_field *regfield_repeat_rate;
> +	struct regmap_field *regfield_scale;
> +	struct regmap_field *regfield_int_src;
> +	struct regmap_field *regfield_int_thresh_var_en;
> +	struct regmap_field *regfield_int_en;
> +	struct regmap_field *regfield_int_persist_val;
> +	struct regmap_field *regfield_int_thresh_var_val;
> +
> +	int nlux_per_count;
> +	int read_data_available;
> +	u8 intg_time_idx;
> +	u8 repeat_rate_idx;
> +	u8 gain_idx;

As mentioned below, I'm not sure why you need to cache these instead
of just reading the fields back from the regmap cache or the device.
 
> +	u8 int_ch;
> +};
> +
...

> +static struct iio_event_spec apds9306_event_spec_als[] = {
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
> +	}, {
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_FALLING,
> +		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
> +	}, {
> +		.type = IIO_EV_TYPE_THRESH,
> +		.mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD),
> +	}, {
> +		.type = IIO_EV_TYPE_THRESH_ADAPTIVE,
> +		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
> +			BIT(IIO_EV_INFO_ENABLE),
> +	}, {
> +		.type = IIO_EV_TYPE_THRESH,
> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
This matches an entry above for type. Don't have separate entries.
> +	},
> +};
> +
> +static struct iio_event_spec apds9306_event_spec_clear[] = {
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> +	},
> +};
> +
> +#define APDS9306_CHANNEL(_type) \
> +	.type = _type, \
> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) | \
> +		BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> +	.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | \
> +		BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), \

Scale on the intensity channel is interesting...  What are the units?
There tend not to be any well defined units for intensity (as opposed
to illuminance).  There may be gain on the signal, but it won't be in untils
that map directly to a scale userspace should apply.  This is one of the
rare reasons for using the HARDWARE_GAIN element of the ABI.

A tricky corner however as relationship between raw value and hardwaregain
is not tightly defined (as it can be really weird!)

> +
> +static struct iio_chan_spec apds9306_channels_without_events[] = {
> +	{
> +		APDS9306_CHANNEL(IIO_LIGHT)
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +	}, {
> +		APDS9306_CHANNEL(IIO_INTENSITY)
> +		.channel2 = IIO_MOD_LIGHT_CLEAR,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.modified = 1,
> +	},
> +};



> +
> +static int apds9306_runtime_power(struct apds9306_data *data, int en)

Why int?  bool seems a better fit.

> +{
> +	struct device *dev = data->dev;
> +	int ret;
> +
> +	if (en) {
> +		ret = pm_runtime_resume_and_get(dev);
> +		if (ret < 0) {
> +			dev_err(dev, "runtime resume failed: %d\n", ret);
> +			return ret;
> +		}
> +		return 0;
The two paths are entirely unrelated so split power up from power down as two
different functions. Only the power up can actually fail.

> +	}
> +
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);

Feels like we should suggest a generic version of this given it's such a common pair
I guess the biggest problem would be naming it.

	pm_runtime_mark_and_put_autosuspend() maybe?

Anyhow, that's a question for another day.

> +	return 0;
> +}
> +
> +static int apds9306_read_data(struct apds9306_data *data, int *val, int reg)
> +{
> +	struct device *dev = data->dev;
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	int ret, delay, intg_time, status = 0;
> +	u8 buff[3];
> +
> +	ret = apds9306_runtime_power(data, 1);
> +	if (ret)
> +		return ret;
> +
> +	intg_time = iio_gts_find_int_time_by_sel(&data->gts,
> +						 data->intg_time_idx);

Feels like caching the intg_time is potentially useful unlike the idx
which can be gotten easily from the register cache.  I haven't checked
how complex looking it up like this is though, so may not be worth bothering.

> +	if (intg_time < 0)
> +		delay = apds9306_repeat_rate_period[data->repeat_rate_idx];
> +
> +	/*
> +	 * Whichever is greater - integration time period or
> +	 * sampling period.
> +	 */
> +	delay = max(intg_time,
> +		    apds9306_repeat_rate_period[data->repeat_rate_idx]);
> +
> +
> +	/*
> +	 * Clear stale data flag that might have been set by the interrupt
> +	 * handler if it got data available flag set in the status reg.
> +	 */
> +	data->read_data_available = 0;
> +
> +	/*
> +	 * If this function runs parallel with the interrupt handler, either
> +	 * this reads and clears the status registers or the interrupt handler
> +	 * does. The interrupt handler sets a flag for read data available
> +	 * in our private structure which we read here.
> +	 */
> +	ret = regmap_read_poll_timeout(data->regmap, APDS9306_MAIN_STATUS,
> +				status, (status & (APDS9306_ALS_DATA_STAT_MASK |
> +				APDS9306_ALS_INT_STAT_MASK)) ||
> +				data->read_data_available,
> +				APDS9306_ALS_READ_DATA_DELAY_US, delay * 2);
> +
> +	if (ret)
> +		return ret;
> +
> +	/* If we reach here before the interrupt handler we push an event */
> +	if ((status & APDS9306_ALS_INT_STAT_MASK)) {
> +		iio_push_event(indio_dev, IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
> +			       data->int_ch,
> +			       IIO_EV_TYPE_THRESH, IIO_EV_DIR_EITHER),
> +			       iio_get_time_ns(indio_dev));
> +	}
> +
> +	if ((status & APDS9306_ALS_DATA_STAT_MASK) ||
> +		data->read_data_available) {

If these aren't true isn't it an error that we should report?
	if (!(status & APDS9306_ALS_DATA_STAT_MASK) &&
            !data->read_data_available)
		return -EIO; //maybe indicated timed out?

	ret = regmap
	
> +		ret = regmap_bulk_read(data->regmap, reg, buff, sizeof(buff));
> +		if (ret) {
> +			dev_err(dev, "read data failed\n");
> +			return ret;
> +		}
> +		*val = get_unaligned_le24(&buff);
> +	}
> +
> +	return apds9306_runtime_power(data, 0);
The fact this looks like it can fail is an artefact of combining power up and power down.
	apds9306_runtime_power_down(data);
	
	return 0; 

would make it clear that it can't.

> +}
> +

> +
> +static int apds9306_intg_time_set_hw(struct apds9306_data *data, int sel)
> +{
> +	int ret;
> +
> +	ret = regmap_field_write(data->regfield_intg_time, sel);

Given regmap comes with excellent caching and you have that turned on,
what is the advantage of having your own cache of the value?

I'd just use regmap_field_read() to get it back where you need it.
Having made that change, this wrapper becomes unnecessary and yuo can
just call regmap_field_write() directly wherever it is currently called.

Same applies for all the other caching of status in the driver. If you
can rely on the generic register caching it normally ends ups simpler
than rolling your own.

> +	if (ret)
> +		return ret;
> +
> +	data->intg_time_idx = sel;
> +
> +	return ret;
> +}


> +static int apds9306_sampling_freq_set(struct apds9306_data *data, int val,
> +				int val2)
> +{
> +	int i, ret = -EINVAL;
> +
> +	for (i = 0; i < ARRAY_SIZE(apds9306_repeat_rate_freq); i++)
> +		if (apds9306_repeat_rate_freq[i][0] == val &&
> +				apds9306_repeat_rate_freq[i][1] == val2) {

Align the second line of this with the first one
		if (apds9306_repeat_rate_freq[i][0] == val &&
		    apds9306_repeat_rate_freq[i][1] == val2) {

> +			ret = regmap_field_write(data->regfield_repeat_rate, i);
> +			if (ret)
> +				return ret;
> +			data->repeat_rate_idx = i;
> +			return ret;
> +		}
> +
> +	return ret;
> +}

> +static int apds9306_scale_set(struct apds9306_data *data, int val, int val2)
> +{
> +	int i, ret, time_sel, gain_sel;
> +
> +	/* Rounding up the last digit by one, otherwise matching table fails! */

Interesting.  Sounds like a question for Matti?

> +	if (val2 % 10)
> +		val2 += 1;
> +
> +	ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts,
> +				     data->intg_time_idx, val, val2, &gain_sel);
> +	if (ret) {
> +		for (i = 0; i < data->gts.num_itime; i++) {
> +			time_sel = data->gts.itime_table[i].sel;
> +
> +			if (time_sel == data->intg_time_idx)
> +				continue;
> +
> +			ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts,
> +						time_sel, val, val2, &gain_sel);
> +			if (!ret)
> +				break;
> +		}
> +		if (ret)
> +			return -EINVAL;
> +
> +		ret = apds9306_intg_time_set_hw(data, time_sel);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return apds9306_gain_set_hw(data, gain_sel);
> +}

> +
> +static int apds9306_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)
> +{
> +	struct apds9306_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (type) {
> +	case IIO_EV_TYPE_THRESH:
> +		mutex_lock(&data->mutex);

Lock taken in all 'real' paths as the default won't actually happen.
So take it outside the switch and use guard() to all direct returns.

> +		if (dir == IIO_EV_DIR_EITHER && info == IIO_EV_INFO_PERIOD)
> +			ret = apds9306_event_period_set(data, val);
> +		else
> +			ret = apds9306_event_thresh_set(data, dir, val);
> +		mutex_unlock(&data->mutex);
> +		return ret;
> +	case IIO_EV_TYPE_THRESH_ADAPTIVE:
> +		mutex_lock(&data->mutex);
> +		ret = apds9306_event_thresh_adaptive_set(data, val);
> +		mutex_unlock(&data->mutex);
> +		return ret;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int apds9306_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 apds9306_data *data = iio_priv(indio_dev);
> +	unsigned int int_en;
> +	int ret;
> +
> +	mutex_lock(&data->mutex);
guard()

and direct returns to simplify this.

> +	switch (type) {
> +	case IIO_EV_TYPE_THRESH:
> +		ret = regmap_field_read(data->regfield_int_en, &int_en);
> +		if (ret)
> +			break;
> +		if (chan->type == IIO_LIGHT)
> +			ret = int_en & data->int_ch;
> +		else if (chan->type == IIO_INTENSITY)
> +			ret = int_en & !data->int_ch;
> +		else
> +			ret = -EINVAL;
> +		break;
> +	case IIO_EV_TYPE_THRESH_ADAPTIVE:
> +		ret = regmap_field_read(data->regfield_int_thresh_var_en,
> +					&int_en);
> +		if (ret)
> +			break;
> +		ret = int_en;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +	mutex_unlock(&data->mutex);
> +	return ret;
> +}
> +
> +static int apds9306_write_event_config(struct iio_dev *indio_dev,
> +				       const struct iio_chan_spec *chan,
> +				       enum iio_event_type type,
> +				       enum iio_event_direction dir,
> +				       int state)
> +{
> +	struct apds9306_data *data = iio_priv(indio_dev);
> +	int ret, int_sel;
> +
> +	state = !!state;
> +	switch (type) {
> +	case IIO_EV_TYPE_THRESH:
> +		/*
> +		 * If interrupt is enabled, the channel is set before enabling
> +		 * the interrupt. In case of disable, no need to switch
> +		 * channels. In case of different channel is selected while
> +		 * interrupt in on, just changing the channel.
> +		 */
> +		if (state) {
> +			if (chan->type == IIO_LIGHT)
> +				int_sel = 1;
> +			else if (chan->type == IIO_INTENSITY)
> +				int_sel = 0;
> +			else
> +				return -EINVAL;
> +
> +			ret = regmap_field_write(data->regfield_int_src,
> +						 int_sel);
> +			if (ret)
> +				return ret;
> +			data->int_ch = int_sel;
> +		}
> +		ret = regmap_field_read(data->regfield_int_en, &int_sel);
> +		if (ret)
> +			return ret;
> +		if (int_sel == state)
Not obvious why you are comparing the enabled state of a channel with the
interrupt selection.  Which might not be set by here anyway.

> +			return 0;
> +		mutex_lock(&data->mutex);
> +		ret = regmap_field_write(data->regfield_int_en, state);
> +		if (!ret)
> +			ret = apds9306_runtime_power(data, state);

This bit is confusing in a way that would be cleaned up by...

		scoped_guard(mutex, &data->mutex) {
			ret = regmap_field_write();
			if (ret)
				return ret;
			return apds9360_runtime_power();
		}

		or simply
		guard(mutex)(&data->mutex);
		ret = regmap_field_write();
		if (ret)
			return ret;
	...


		but I think you need to add brackets to whole case statement for that
		to work as expected.


> +		mutex_unlock(&data->mutex);
> +		return ret;
> +	case IIO_EV_TYPE_THRESH_ADAPTIVE:
> +		return regmap_field_write(data->regfield_int_thresh_var_en,
> +					  state);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +#define APDS9306_IIO_INFO \
> +	.read_avail = apds9306_read_avail, \
> +	.read_raw = apds9306_read_raw, \
> +	.write_raw = apds9306_write_raw, \
> +	.write_raw_get_fmt = apds9306_write_raw_get_fmt,

I'd not bother with this macro.  It saves very little actual
code and makes things a tiny bit harder to read.

> +
> +static const struct iio_info apds9306_info_no_events = {
> +	APDS9306_IIO_INFO
> +};
> +
> +static const struct iio_info apds9306_info = {
> +	APDS9306_IIO_INFO
> +	.event_attrs = &apds9306_event_attr_group,
> +	.read_event_value = apds9306_read_event,
> +	.write_event_value = apds9306_write_event,
> +	.read_event_config = apds9306_read_event_config,
> +	.write_event_config = apds9306_write_event_config,
> +};

> +static void apds9306_powerdown(void *ptr)
> +{
> +	struct apds9306_data *data = (struct apds9306_data *)ptr;
> +	int ret;
> +
> +	/* Disable interrupts */

As below. The field names hopefully make that obvious so the comment not needed
and just becomes something that can become wrong as the driver evolves.

> +	ret = regmap_field_write(data->regfield_int_thresh_var_en, 0);
> +	if (ret)
> +		return;
> +	ret = regmap_field_write(data->regfield_int_en, 0);
> +	if (ret)
> +		return;

> +	/* Put the device in standby mode */

This one is even more obvious.

> +	apds9306_power_state(data, STANDBY);
> +}
> +
> +static int apds9306_init_device(struct apds9306_data *data)
> +{
> +	struct device *dev = data->dev;
> +	int ret;
> +
> +	ret = apds9306_power_state(data, ACTIVE);
> +	if (ret)
> +		return ret;
> +
> +	ret = pm_runtime_set_active(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_set_autosuspend_delay(dev, 5000);
> +	pm_runtime_use_autosuspend(dev);
this lot of runtime pm stuff isn't initializing the device, so I don't
see it as making sense in here. I'd push it out to the caller with
the power up before init and the autosuspend etc after.
I'll note that I'd expect to see a a pm_runtime_put_autosuspend()
at the end of probe to put device to sleep soon after loading.

> +
> +	/* Init GTS multiplier values according to Part ID */
> +	ret = apds9306_init_iio_gts(data);
> +	if (ret)
> +		return ret;
> +
> +	/* Integration time: 100 msec */
> +	ret = apds9306_intg_time_set_hw(data, 2);
Define as below.

> +	if (ret)
> +		return ret;
> +
> +	/* Sampling frequency: 100 msec, 10 Hz */
> +	ret = apds9306_sampling_freq_set(data, 10, 0);
> +	if (ret)
> +		return ret;
> +
> +	/* Scale: x3 */
> +	ret = apds9306_gain_set_hw(data, 1);
As per note below, this is one where the value written isn't 'obvious'.
A good indication that the code could be made more readable.
	ret = apds9306_gain_set_hw(data, APDS9306_GSEL_3X);

for example.

> +	if (ret)
> +		return ret;
> +
> +	/* Interrupt source channel: als */
> +	ret = regmap_field_write(data->regfield_int_src, 1);
I've not checked the datasheet, but this feels like the 1 is
probably a selection value that should have a define like you
have for the GSEL values.  Do that and hopefully the comment
is no longer necessary.

> +	if (ret)
> +		return ret;
> +	data->int_ch = 1;
> +
> +	/* Interrupts disabled */
> +	ret = regmap_field_write(data->regfield_int_en, 0);
> +	if (ret)
> +		return ret;
> +
> +	/* Threshold Variance disabled */
Ideal situation is that the code itself makes it obvious what is going on,
reducing need for comments.  Long experience tells me that comments that
add little information and just an opportunity for code rot!

This is a good example. Setting a field called int_thresh_var_en to 0
seems pretty clear without the comment.

Consider all the other similar cases and just keep the ones that non obvious.
In some cases that means using some defines to make the code obvious.

> +	return regmap_field_write(data->regfield_int_thresh_var_en, 0);
> +}


> +
> +static int apds9306_runtime_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct apds9306_data *data = iio_priv(indio_dev);

As below.

> +
> +	return apds9306_power_state(data, STANDBY);
> +}
> +
> +static int apds9306_runtime_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct apds9306_data *data = iio_priv(indio_dev);

You could combine these lines without significant loss of readability.

	struct apds9306_data *data = iio_priv(dev_get_drvdata(dev));

> +
> +	return apds9306_power_state(data, ACTIVE);
> +}
> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(apds9306_pm_ops,
> +				 apds9306_runtime_suspend,
> +				 apds9306_runtime_resume,
> +				 NULL);


> +static struct i2c_driver apds9306_driver = {
> +	.driver = {
> +		.name = "apds9306",
> +		.pm = pm_ptr(&apds9306_pm_ops),
> +		.of_match_table = apds9306_of_match,
> +		.probe_type = PROBE_PREFER_ASYNCHRONOUS,

Async probe may make sense, but it's not necessarily an obvious choice.
Add a brief comment on why.

> +	},
> +	.probe = apds9306_probe,
> +};
> +module_i2c_driver(apds9306_driver);
> +
> +MODULE_AUTHOR("Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>");
> +MODULE_DESCRIPTION("APDS9306 Ambient Light Sensor driver");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(IIO_GTS_HELPER);

Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[Matti Vaittinen]

On 10/28/23 18:20, Jonathan Cameron wrote:
> On Fri, 27 Oct 2023 18:15:45 +1030
> Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:
> 
>> Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor with als
>> and clear channels with i2c interface. Hardware interrupt configuration is
>> optional. It is a low power device with 20 bit resolution and has
>> configurable adaptive interrupt mode and interrupt persistence mode.
>> The device also features inbuilt hardware gain, multiple integration time
>> selection options and sampling frequency selection options.
> 
> Hi Subhajit,
> 
> 
>>
> Change log below the ---
> 
> We don't generally want to end up with this information in the git log
> and anything above the --- is used for the commit message.
> 
> Note that if you want to keep notes in your local git it is fine adding
> 
> Signed-of-by...
> 
> ---
> 
> Version notes
> etc
> 
> 
> As then git am will drop them anyway when your patches are picked up.
> 
> 
>> v1 -> v2
>> - Renamed probe_new to probe
>> - Removed module id table
>>
>> v0 -> v1
>> - Fixed errors as per previous review
>> - Longer commit messages and descriptions
>> - Updated scale calculations as per iio gts scheme to export proper scale
>>    values and tables to userspace
>> - Removed processed attribute for the same channel for which raw is
>>    provided, instead, exporting proper scale and scale table to userspace so
>>    that userspace can do "(raw + offset) * scale" and derive Lux values
>> - Fixed IIO attribute range syntax
>> - Keeping the regmap lock enabled as the driver uses unlocked regfield
>>    accesses from interrupt handler
>> - Several levels of cleanups by placing guard mutexes in proper places and
>>    returning immediately in case of an error
>> - Using iio_device_claim_direct_mode() during raw reads so that
>>    configurations could not be changed during an adc conversion period
>> - In case of a powerdown error, returning immediately
>> - Removing the definition of direction of the hardware interrupt and
>>    leaving it on to device tree
>> - Adding the powerdown callback after doing device initialization
>> - Removed the regcache_cache_only() implementation
>>
>> Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
> 

...

> 
>> +static int apds9306_scale_set(struct apds9306_data *data, int val, int val2)
>> +{
>> +	int i, ret, time_sel, gain_sel;
>> +
>> +	/* Rounding up the last digit by one, otherwise matching table fails! */
> 
> Interesting.  Sounds like a question for Matti?

Sounds odd indeed. I assume this happens when scale setting is requested 
using one of the exact values advertised by the available scales from 
the GTS? This does not feel right and the +1 does not ring a bell to me. 
I need to investigate what's going on. It would help if you could 
provide the values used as val and val2 for the setting.

This will take a while from me though - I'll try to get to this next 
week. Thanks for pointing out the anomaly!

> 
>> +	if (val2 % 10)
>> +		val2 += 1;
>> +
>> +	ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts,
>> +				     data->intg_time_idx, val, val2, &gain_sel);
>> +	if (ret) {
>> +		for (i = 0; i < data->gts.num_itime; i++) {
>> +			time_sel = data->gts.itime_table[i].sel;
>> +
>> +			if (time_sel == data->intg_time_idx)
>> +				continue;
>> +
>> +			ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts,
>> +						time_sel, val, val2, &gain_sel);
>> +			if (!ret)
>> +				break;
>> +		}
>> +		if (ret)
>> +			return -EINVAL;
>> +
>> +		ret = apds9306_intg_time_set_hw(data, time_sel);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return apds9306_gain_set_hw(data, gain_sel);
>> +}

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

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

Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[Matti Vaittinen]

Hi dee Ho peeps,

On 10/29/23 17:51, Matti Vaittinen wrote:
> On 10/28/23 18:20, Jonathan Cameron wrote:
>> On Fri, 27 Oct 2023 18:15:45 +1030
>> Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:
>>
>>> Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor 
>>> with als
>>> and clear channels with i2c interface. Hardware interrupt 
>>> configuration is
>>> optional. It is a low power device with 20 bit resolution and has
>>> configurable adaptive interrupt mode and interrupt persistence mode.
>>> The device also features inbuilt hardware gain, multiple integration 
>>> time
>>> selection options and sampling frequency selection options.
>>
>> Hi Subhajit,
>>
>>
>>>
>> Change log below the ---
>>
>> We don't generally want to end up with this information in the git log
>> and anything above the --- is used for the commit message.
>>
>> Note that if you want to keep notes in your local git it is fine adding
>>
>> Signed-of-by...
>>
>> ---
>>
>> Version notes
>> etc
>>
>>
>> As then git am will drop them anyway when your patches are picked up.
>>
>>
>>> v1 -> v2
>>> - Renamed probe_new to probe
>>> - Removed module id table
>>>
>>> v0 -> v1
>>> - Fixed errors as per previous review
>>> - Longer commit messages and descriptions
>>> - Updated scale calculations as per iio gts scheme to export proper 
>>> scale
>>>    values and tables to userspace
>>> - Removed processed attribute for the same channel for which raw is
>>>    provided, instead, exporting proper scale and scale table to 
>>> userspace so
>>>    that userspace can do "(raw + offset) * scale" and derive Lux values
>>> - Fixed IIO attribute range syntax
>>> - Keeping the regmap lock enabled as the driver uses unlocked regfield
>>>    accesses from interrupt handler
>>> - Several levels of cleanups by placing guard mutexes in proper 
>>> places and
>>>    returning immediately in case of an error
>>> - Using iio_device_claim_direct_mode() during raw reads so that
>>>    configurations could not be changed during an adc conversion period
>>> - In case of a powerdown error, returning immediately
>>> - Removing the definition of direction of the hardware interrupt and
>>>    leaving it on to device tree
>>> - Adding the powerdown callback after doing device initialization
>>> - Removed the regcache_cache_only() implementation
>>>
>>> Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
>>
> 
> ...
> 
>>
>>> +static int apds9306_scale_set(struct apds9306_data *data, int val, 
>>> int val2)
>>> +{
>>> +    int i, ret, time_sel, gain_sel;
>>> +
>>> +    /* Rounding up the last digit by one, otherwise matching table 
>>> fails! */
>>
>> Interesting.  Sounds like a question for Matti?
> 
> Sounds odd indeed. I assume this happens when scale setting is requested 
> using one of the exact values advertised by the available scales from 
> the GTS? This does not feel right and the +1 does not ring a bell to me. 
> I need to investigate what's going on. It would help if you could 
> provide the values used as val and val2 for the setting.
> 
> This will take a while from me though - I'll try to get to this next 
> week. Thanks for pointing out the anomaly!
> 

I think I have a rough understanding. I did a Kunit test which goes 
through all the available scales values from the 
gts->avail_all_scales_table and all integration times, and feeds them to 
the logic below. It seems the first culprit is hit by:
val = 0, val2 = 125025502.

Problem is that the 125025502 is rounded. The exact linearized NANO 
scale resulting from time multiplier 128, gain multiplier 1 is 
125025502.5 - which means we will see rounding.

>>
>>> +    if (val2 % 10)
>>> +        val2 += 1;

For a while I was unsure if this check works for all cases because I see 
linearized scales:
250051005 - multipliers 1x, 64x
83350335 - multipliers 3x, 64x and 6x, 32x
27783445 - multipliers 9x, 64x.

For those we will get + 1 added to val2 even though there is no 
rounding. It appears this is not a problem because the 
iio_gts_get_gain() (which is used to figure out the required total gain 
to get the desired scale) does not require the scale to be formed by 
exact multiples of gain.

Still, the check:
 >>> +    if (val2 % 10)
 >>> +        val2 += 1;

seems a bit misleading because only the scales where the NANO accuracy 
is not enough need the + 1. I would at least ask for a comment 
explaining this a bit better :)

Another quick'n dirty way is to simply check if requested scale is one 
of the magic scales where the NANO accuracy is not enough:
41675167(.5)
125025502(.5)
20837583(.75)
13891722(.5)

and handle the rounding only for those (instead of the val2 % 10) - 
still with appropriate comment.

I think it would be very nice if the gts-helpers could do the rounding 
when computing the available scales, but that'd require some thinking. 
Fixup patch is still very welcome ;)

I did avoid this issue with BU27* drivers earlier because I was able to 
select the maximum scale so that the NANO accuracy was enough since I 
used these helpers for the intensity channels.

>>> +
>>> +    ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts,
>>> +                     data->intg_time_idx, val, val2, &gain_sel);
>>> +    if (ret) {
>>> +        for (i = 0; i < data->gts.num_itime; i++) {
>>> +            time_sel = data->gts.itime_table[i].sel;
>>> +
>>> +            if (time_sel == data->intg_time_idx)
>>> +                continue;
>>> +
>>> +            ret = 
>>> iio_gts_find_gain_sel_for_scale_using_time(&data->gts,
>>> +                        time_sel, val, val2, &gain_sel);
>>> +            if (!ret)
>>> +                break;
>>> +        }
>>> +        if (ret)
>>> +            return -EINVAL;
>>> +
>>> +        ret = apds9306_intg_time_set_hw(data, time_sel);
>>> +        if (ret)
>>> +            return ret;
>>> +    }
>>> +
>>> +    return apds9306_gain_set_hw(data, gain_sel);
>>> +}
> 

Yours,
	-- Matti

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

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

Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[Matti Vaittinen]

On 10/30/23 12:21, Matti Vaittinen wrote:
> Hi dee Ho peeps,
> 
> On 10/29/23 17:51, Matti Vaittinen wrote:
>> On 10/28/23 18:20, Jonathan Cameron wrote:
>>> On Fri, 27 Oct 2023 18:15:45 +1030
>>> Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:
>>>
>>>> Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor 
>>>> with als
>>>> and clear channels with i2c interface. Hardware interrupt 
>>>> configuration is
>>>> optional. It is a low power device with 20 bit resolution and has
>>>> configurable adaptive interrupt mode and interrupt persistence mode.
>>>> The device also features inbuilt hardware gain, multiple integration 
>>>> time
>>>> selection options and sampling frequency selection options.

...

>>>> +static int apds9306_scale_set(struct apds9306_data *data, int val, 
>>>> int val2)
>>>> +{
>>>> +    int i, ret, time_sel, gain_sel;
>>>> +
>>>> +    /* Rounding up the last digit by one, otherwise matching table 
>>>> fails! */
>>>
>>> Interesting.  Sounds like a question for Matti?
>>
>> Sounds odd indeed. I assume this happens when scale setting is 
>> requested using one of the exact values advertised by the available 
>> scales from the GTS? This does not feel right and the +1 does not ring 
>> a bell to me. I need to investigate what's going on. It would help if 
>> you could provide the values used as val and val2 for the setting.
>>
>> This will take a while from me though - I'll try to get to this next 
>> week. Thanks for pointing out the anomaly!
>>
> 
> I think I have a rough understanding. I did a Kunit test which goes 
> through all the available scales values from the 
> gts->avail_all_scales_table and all integration times, and feeds them to 
> the logic below. It seems the first culprit is hit by:
> val = 0, val2 = 125025502.
> 
> Problem is that the 125025502 is rounded. The exact linearized NANO 
> scale resulting from time multiplier 128, gain multiplier 1 is 
> 125025502.5 - which means we will see rounding.
> 
>>>
>>>> +    if (val2 % 10)
>>>> +        val2 += 1;
> 
> For a while I was unsure if this check works for all cases because I see 
> linearized scales:
> 250051005 - multipliers 1x, 64x
> 83350335 - multipliers 3x, 64x and 6x, 32x
> 27783445 - multipliers 9x, 64x.
> 
> For those we will get + 1 added to val2 even though there is no 
> rounding. It appears this is not a problem because the 
> iio_gts_get_gain() (which is used to figure out the required total gain 
> to get the desired scale) does not require the scale to be formed by 
> exact multiples of gain.

...

> I think it would be very nice if the gts-helpers could do the rounding 
> when computing the available scales, but that'd require some thinking. 
> Fixup patch is still very welcome ;)

I did some further experimenting. Basically, I did a "hack" which always 
rounds up the available-scales values if division results a remainder. 
This way the values advertised by the available_scales did find the 
matching table.

It is a tiny bit icky because for example the scale 6945861.25 becomes 
6945862 in available-scales. Also, I assume that if we "hack" just the 
available-scales and don't fix the rest of the logic, setting 6945862 
will read back as 6945861 (I haven't tested this though). Also, the 
20837583.75 will be 20837583 in available-scales but 20837582 when read 
back, resulting small error. (I haven't tested this either but I assume 
the current GTS code is flooring the 20837583.75 to 20837583.

I am wondering if changing the iio_gts_get_gain() to do rounding instead 
of flooring and changing also the iio_gts_total_gain_to_scale() to 
something like:

int iio_gts_total_gain_to_scale(struct iio_gts *gts, int total_gain,
				int *scale_int, int *scale_nano)
{
	u64 tmp;
	int rem;

	tmp = gts->max_scale;

	rem = do_div(tmp, total_gain);
	if (total_gain > 1 && rem >= total_gain / 2)
		tmp += 1ULL;

	return iio_gts_delinearize(tmp, NANO, scale_int, scale_nano);
}

would do the trick. It's just that I'm a bit afraid of touching the 
iio_gts_get_gain() - by the very least I need to fire up the GTS tests 
which I implemented but are not in-tree due to the test-device 
dependency... :/

Any thoughts?

>>>> +
>>>> +    ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts,
>>>> +                     data->intg_time_idx, val, val2, &gain_sel);
>>>> +    if (ret) {
>>>> +        for (i = 0; i < data->gts.num_itime; i++) {
>>>> +            time_sel = data->gts.itime_table[i].sel;
>>>> +
>>>> +            if (time_sel == data->intg_time_idx)
>>>> +                continue;
>>>> +
>>>> +            ret = 
>>>> iio_gts_find_gain_sel_for_scale_using_time(&data->gts,
>>>> +                        time_sel, val, val2, &gain_sel);
>>>> +            if (!ret)
>>>> +                break;
>>>> +        }
>>>> +        if (ret)
>>>> +            return -EINVAL;
>>>> +
>>>> +        ret = apds9306_intg_time_set_hw(data, time_sel);
>>>> +        if (ret)
>>>> +            return ret;
>>>> +    }
>>>> +
>>>> +    return apds9306_gain_set_hw(data, gain_sel);
>>>> +}

Yours,
	-- Matti

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

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

Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[Subhajit Ghosh]

On 31/10/23 17:41, Matti Vaittinen wrote:
> On 10/30/23 12:21, Matti Vaittinen wrote:
>> Hi dee Ho peeps,
>>
>> On 10/29/23 17:51, Matti Vaittinen wrote:
>>> On 10/28/23 18:20, Jonathan Cameron wrote:
>>>> On Fri, 27 Oct 2023 18:15:45 +1030
>>>> Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:
>>>>
>>>>> Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor with als
>>>>> and clear channels with i2c interface. Hardware interrupt configuration is
>>>>> optional. It is a low power device with 20 bit resolution and has
>>>>> configurable adaptive interrupt mode and interrupt persistence mode.
>>>>> The device also features inbuilt hardware gain, multiple integration time
>>>>> selection options and sampling frequency selection options.
> 
> ...
> 
>>>>> +static int apds9306_scale_set(struct apds9306_data *data, int val, int val2)
>>>>> +{
>>>>> +    int i, ret, time_sel, gain_sel;
>>>>> +
>>>>> +    /* Rounding up the last digit by one, otherwise matching table fails! */
>>>>
>>>> Interesting.  Sounds like a question for Matti?
>>>
>>> Sounds odd indeed. I assume this happens when scale setting is requested using one of the exact values advertised by the available scales from the GTS? This does not feel right and the +1 does not ring a bell to me. I need to investigate what's going on. It would help if you could provide the values used as val and val2 for the setting.
>>>
>>> This will take a while from me though - I'll try to get to this next week. Thanks for pointing out the anomaly!
>>>
>>
>> I think I have a rough understanding. I did a Kunit test which goes through all the available scales values from the gts->avail_all_scales_table and all integration times, and feeds them to the logic below. It seems the first culprit is hit by:
>> val = 0, val2 = 125025502.
>>
>> Problem is that the 125025502 is rounded. The exact linearized NANO scale resulting from time multiplier 128, gain multiplier 1 is 125025502.5 - which means we will see rounding.
>>
>>>>
>>>>> +    if (val2 % 10)
>>>>> +        val2 += 1;
>>
>> For a while I was unsure if this check works for all cases because I see linearized scales:
>> 250051005 - multipliers 1x, 64x
>> 83350335 - multipliers 3x, 64x and 6x, 32x
>> 27783445 - multipliers 9x, 64x.
>>
>> For those we will get + 1 added to val2 even though there is no rounding. It appears this is not a problem because the iio_gts_get_gain() (which is used to figure out the required total gain to get the desired scale) does not require the scale to be formed by exact multiples of gain.
> 
> ...
> 
>> I think it would be very nice if the gts-helpers could do the rounding when computing the available scales, but that'd require some thinking. Fixup patch is still very welcome ;)
> 
> I did some further experimenting. Basically, I did a "hack" which always rounds up the available-scales values if division results a remainder. This way the values advertised by the available_scales did find the matching table.
> 
> It is a tiny bit icky because for example the scale 6945861.25 becomes 6945862 in available-scales. Also, I assume that if we "hack" just the available-scales and don't fix the rest of the logic, setting 6945862 will read back as 6945861 (I haven't tested this though). Also, the 20837583.75 will be 20837583 in available-scales but 20837582 when read back, resulting small error. (I haven't tested this either but I assume the current GTS code is flooring the 20837583.75 to 20837583.
> 
> I am wondering if changing the iio_gts_get_gain() to do rounding instead of flooring and changing also the iio_gts_total_gain_to_scale() to something like:
> 
> int iio_gts_total_gain_to_scale(struct iio_gts *gts, int total_gain,
>                  int *scale_int, int *scale_nano)
> {
>      u64 tmp;
>      int rem;
> 
>      tmp = gts->max_scale;
> 
>      rem = do_div(tmp, total_gain);
>      if (total_gain > 1 && rem >= total_gain / 2)
>          tmp += 1ULL;
> 
>      return iio_gts_delinearize(tmp, NANO, scale_int, scale_nano);
> }
> 
> would do the trick. It's just that I'm a bit afraid of touching the iio_gts_get_gain() - by the very least I need to fire up the GTS tests which I implemented but are not in-tree due to the test-device dependency... :/
> 
> Any thoughts?
> 
Hi Matti,
Sorry, got busy with my full time job.
It's nice to see that you have found the issue without my test results:)

Please find below my tests -

root@stm32mp1:/sys/bus/iio/devices/iio:device1# cat scale_available
14.009712000 4.669904000 2.334952000 1.751214000 1.556634666 0.875607000 0.778317333 0.583738000 0.437803500 0.291869000 0.218901750 0.194579333 0.145934500 0.109450875 0.097289666 0.072967250 0.048644833 0.036483625
0.024322416 0.018241812 0.012161208 0.006080604
root@stm32mp1:/sys/bus/iio/devices/iio:device1# echo 0.875607000 > scale ## This works
root@stm32mp1:/sys/bus/iio/devices/iio:device1# echo 0.097289666 > scale ## This fails
root@stm32mp1:/sys/bus/iio/devices/iio:device1# echo 0.097289667 > scale ## However if I add 1, it works! I figured, its a rounding issue so used this trick: "if (val2 % 10) val2 += 1;"
I am sorry, I haven't gone through the full gts internals and only used your driver as a reference to understand it's implementation. I do not have any thoughts on top of my head now but let me go through the code.
   
Regards,
Subhajit Ghosh

Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[Andy Shevchenko]

On Tue, Oct 31, 2023 at 09:11:37AM +0200, Matti Vaittinen wrote:
> On 10/30/23 12:21, Matti Vaittinen wrote:
> > On 10/29/23 17:51, Matti Vaittinen wrote:
> > > On 10/28/23 18:20, Jonathan Cameron wrote:

...

> 	tmp = gts->max_scale;
> 
> 	rem = do_div(tmp, total_gain);
> 	if (total_gain > 1 && rem >= total_gain / 2)
> 		tmp += 1ULL;

...which is NIH DIV_ROUND_CLOSEST_ULL()

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[Matti Vaittinen]

On 10/31/23 12:38, Andy Shevchenko wrote:
> On Tue, Oct 31, 2023 at 09:11:37AM +0200, Matti Vaittinen wrote:
>> On 10/30/23 12:21, Matti Vaittinen wrote:
>>> On 10/29/23 17:51, Matti Vaittinen wrote:
>>>> On 10/28/23 18:20, Jonathan Cameron wrote:
> 
> ...
> 
>> 	tmp = gts->max_scale;
>>
>> 	rem = do_div(tmp, total_gain);
>> 	if (total_gain > 1 && rem >= total_gain / 2)
>> 		tmp += 1ULL;
> 
> ...which is NIH DIV_ROUND_CLOSEST_ULL()

Thanks for the hint Andy. I do very much prefer using stuff like the 
DIV_ROUND_CLOSEST_ULL().

I will use this - do you have other suggestions for me, especially 
regarding the division u64 / u64?

I would appreciate if you found the time and energy to look at:
https://lore.kernel.org/all/ZUDN9n8iXoNwzifQ@dc78bmyyyyyyyyyyyyyyt-3.rev.dnainternet.fi/ 

as well. I feel like I am doing something someone else has already done. 
(Yes, DIV_ROUND_CLOSEST_ULL() can be used there too).

Anyways, Thanks for this!

Yours,
	-- Matti

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

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

Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[Matti Vaittinen]

On 10/31/23 12:38, Andy Shevchenko wrote:
> On Tue, Oct 31, 2023 at 09:11:37AM +0200, Matti Vaittinen wrote:
>> On 10/30/23 12:21, Matti Vaittinen wrote:
>>> On 10/29/23 17:51, Matti Vaittinen wrote:
>>>> On 10/28/23 18:20, Jonathan Cameron wrote:
> 
> ...
> 
>> 	tmp = gts->max_scale;
>>
>> 	rem = do_div(tmp, total_gain);
>> 	if (total_gain > 1 && rem >= total_gain / 2)
>> 		tmp += 1ULL;
> 
> ...which is NIH DIV_ROUND_CLOSEST_ULL()

There is a difference though. The DIV_ROUND_CLOSEST_ULL() does

tmp + total_gain / 2;

before division - which in theory may overflow.

Yours,
	-- Matti

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

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

Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[Andy Shevchenko]

On Tue, Oct 31, 2023 at 02:07:46PM +0200, Matti Vaittinen wrote:
> On 10/31/23 12:38, Andy Shevchenko wrote:
> > On Tue, Oct 31, 2023 at 09:11:37AM +0200, Matti Vaittinen wrote:
> > > On 10/30/23 12:21, Matti Vaittinen wrote:
> > > > On 10/29/23 17:51, Matti Vaittinen wrote:
> > > > > On 10/28/23 18:20, Jonathan Cameron wrote:

...

> > > 	tmp = gts->max_scale;
> > > 
> > > 	rem = do_div(tmp, total_gain);
> > > 	if (total_gain > 1 && rem >= total_gain / 2)
> > > 		tmp += 1ULL;
> > 
> > ...which is NIH DIV_ROUND_CLOSEST_ULL()
> 
> There is a difference though. The DIV_ROUND_CLOSEST_ULL() does
> 
> tmp + total_gain / 2;
> 
> before division - which in theory may overflow.

Then you can fix it there for everybody, no?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[Matti Vaittinen]

On 10/31/23 15:42, Andy Shevchenko wrote:
> On Tue, Oct 31, 2023 at 02:07:46PM +0200, Matti Vaittinen wrote:
>> On 10/31/23 12:38, Andy Shevchenko wrote:
>>> On Tue, Oct 31, 2023 at 09:11:37AM +0200, Matti Vaittinen wrote:
>>>> On 10/30/23 12:21, Matti Vaittinen wrote:
>>>>> On 10/29/23 17:51, Matti Vaittinen wrote:
>>>>>> On 10/28/23 18:20, Jonathan Cameron wrote:
> 
> ...
> 
>>>> 	tmp = gts->max_scale;
>>>>
>>>> 	rem = do_div(tmp, total_gain);
>>>> 	if (total_gain > 1 && rem >= total_gain / 2)
>>>> 		tmp += 1ULL;
>>>
>>> ...which is NIH DIV_ROUND_CLOSEST_ULL()
>>
>> There is a difference though. The DIV_ROUND_CLOSEST_ULL() does
>>
>> tmp + total_gain / 2;
>>
>> before division - which in theory may overflow.
> 
> Then you can fix it there for everybody, no?

Now that I know of this macro - Maybe. It's just always scary to touch 
things which seem like fundamental building blocks and which may be used 
by many. Odds are something breaks, so I tend to be very conservative 
when suggesting changes to widely used stuff. Especially when I have no 
idea when and why the API has been added - and if the thing I'm trying 
to "fix" has been a deliberate choice.

Yours,
	-- Matti.

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

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

Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[Andy Shevchenko]

On Wed, Nov 01, 2023 at 08:16:32AM +0200, Matti Vaittinen wrote:
> On 10/31/23 15:42, Andy Shevchenko wrote:
> > On Tue, Oct 31, 2023 at 02:07:46PM +0200, Matti Vaittinen wrote:
> > > On 10/31/23 12:38, Andy Shevchenko wrote:
> > > > On Tue, Oct 31, 2023 at 09:11:37AM +0200, Matti Vaittinen wrote:
> > > > > On 10/30/23 12:21, Matti Vaittinen wrote:
> > > > > > On 10/29/23 17:51, Matti Vaittinen wrote:
> > > > > > > On 10/28/23 18:20, Jonathan Cameron wrote:

...

> > > > > 	tmp = gts->max_scale;
> > > > > 
> > > > > 	rem = do_div(tmp, total_gain);
> > > > > 	if (total_gain > 1 && rem >= total_gain / 2)
> > > > > 		tmp += 1ULL;
> > > > 
> > > > ...which is NIH DIV_ROUND_CLOSEST_ULL()
> > > 
> > > There is a difference though. The DIV_ROUND_CLOSEST_ULL() does
> > > 
> > > tmp + total_gain / 2;
> > > 
> > > before division - which in theory may overflow.
> > 
> > Then you can fix it there for everybody, no?
> 
> Now that I know of this macro - Maybe. It's just always scary to touch
> things which seem like fundamental building blocks and which may be used by
> many. Odds are something breaks, so I tend to be very conservative when
> suggesting changes to widely used stuff. Especially when I have no idea when
> and why the API has been added - and if the thing I'm trying to "fix" has
> been a deliberate choice.

Welcome to the club of the div overflow in macros discussion:

https://lore.kernel.org/linux-clk/20231024161931.78567-1-sebastian.reichel@collabora.com/T/#t

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[Subhajit Ghosh]

> 
>> +static struct iio_event_spec apds9306_event_spec_als[] = {
>> +	{
>> +		.type = IIO_EV_TYPE_THRESH,
>> +		.dir = IIO_EV_DIR_RISING,
>> +		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
>> +	}, {
>> +		.type = IIO_EV_TYPE_THRESH,
>> +		.dir = IIO_EV_DIR_FALLING,
>> +		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
>> +	}, {
>> +		.type = IIO_EV_TYPE_THRESH,
>> +		.mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD),
>> +	}, {
>> +		.type = IIO_EV_TYPE_THRESH_ADAPTIVE,
>> +		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
>> +			BIT(IIO_EV_INFO_ENABLE),
>> +	}, {
>> +		.type = IIO_EV_TYPE_THRESH,
>> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> This matches an entry above for type. Don't have separate entries.
>> +	},
>> +};
>> +
>> +static struct iio_event_spec apds9306_event_spec_clear[] = {
>> +	{
>> +		.type = IIO_EV_TYPE_THRESH,
>> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
>> +	},
>> +};
>> +
>> +#define APDS9306_CHANNEL(_type) \
>> +	.type = _type, \
>> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) | \
>> +		BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>> +	.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | \
>> +		BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> 
> Scale on the intensity channel is interesting...  What are the units?
> There tend not to be any well defined units for intensity (as opposed
> to illuminance).  There may be gain on the signal, but it won't be in untils
> that map directly to a scale userspace should apply.  This is one of the
> rare reasons for using the HARDWARE_GAIN element of the ABI.
> 
> A tricky corner however as relationship between raw value and hardwaregain
> is not tightly defined (as it can be really weird!)
Hi Jonathan,

Thank you for taking time for reviewing and clearing all my tiny doubts and
queries especially for the dt and versioning part. Much appreciated.

In the above case, should I not expose scale for the "clear" channel? Rather,
how should I expose the "clear" channel to userspace?

Regards,
Subhajit Ghosh

> 
>> +
>> +static struct iio_chan_spec apds9306_channels_without_events[] = {
>> +	{
>> +		APDS9306_CHANNEL(IIO_LIGHT)
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> +	}, {
>> +		APDS9306_CHANNEL(IIO_INTENSITY)
>> +		.channel2 = IIO_MOD_LIGHT_CLEAR,
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> +		.modified = 1,
>> +	},
>> +};
> 

Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[Jonathan Cameron]

On Tue, 31 Oct 2023 19:08:08 +1030
Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:

> >   
> >> +static struct iio_event_spec apds9306_event_spec_als[] = {
> >> +	{
> >> +		.type = IIO_EV_TYPE_THRESH,
> >> +		.dir = IIO_EV_DIR_RISING,
> >> +		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
> >> +	}, {
> >> +		.type = IIO_EV_TYPE_THRESH,
> >> +		.dir = IIO_EV_DIR_FALLING,
> >> +		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
> >> +	}, {
> >> +		.type = IIO_EV_TYPE_THRESH,
> >> +		.mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD),
> >> +	}, {
> >> +		.type = IIO_EV_TYPE_THRESH_ADAPTIVE,
> >> +		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
> >> +			BIT(IIO_EV_INFO_ENABLE),
> >> +	}, {
> >> +		.type = IIO_EV_TYPE_THRESH,
> >> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),  
> > This matches an entry above for type. Don't have separate entries.  
> >> +	},
> >> +};
> >> +
> >> +static struct iio_event_spec apds9306_event_spec_clear[] = {
> >> +	{
> >> +		.type = IIO_EV_TYPE_THRESH,
> >> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
> >> +	},
> >> +};
> >> +
> >> +#define APDS9306_CHANNEL(_type) \
> >> +	.type = _type, \
> >> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) | \
> >> +		BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> >> +	.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | \
> >> +		BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), \  
> > 
> > Scale on the intensity channel is interesting...  What are the units?
> > There tend not to be any well defined units for intensity (as opposed
> > to illuminance).  There may be gain on the signal, but it won't be in untils
> > that map directly to a scale userspace should apply.  This is one of the
> > rare reasons for using the HARDWARE_GAIN element of the ABI.
> > 
> > A tricky corner however as relationship between raw value and hardwaregain
> > is not tightly defined (as it can be really weird!)  
> Hi Jonathan,
> 
> Thank you for taking time for reviewing and clearing all my tiny doubts and
> queries especially for the dt and versioning part. Much appreciated.
> 
> In the above case, should I not expose scale for the "clear" channel? Rather,
> how should I expose the "clear" channel to userspace?
What is the scale?  What units to you get after applying it?

I suspect it's not well defined.

Not sure what we've done in similar cases in the past.  Perhaps have
a look at those.  There may be precedence for still using scale.

I'm about to jump on a long haul flight (delayed - sigh) so don't have
time to look myself!

Jonathan

> 
> Regards,
> Subhajit Ghosh
> 
> >   
> >> +
> >> +static struct iio_chan_spec apds9306_channels_without_events[] = {
> >> +	{
> >> +		APDS9306_CHANNEL(IIO_LIGHT)
> >> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> >> +	}, {
> >> +		APDS9306_CHANNEL(IIO_INTENSITY)
> >> +		.channel2 = IIO_MOD_LIGHT_CLEAR,
> >> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> >> +		.modified = 1,
> >> +	},
> >> +};  
> >   
> 
> 

Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[Subhajit Ghosh]

On 6/11/23 21:43, Jonathan Cameron wrote:
> On Tue, 31 Oct 2023 19:08:08 +1030
> Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:
> 
>>>    
>>>> +static struct iio_event_spec apds9306_event_spec_als[] = {
>>>> +	{
>>>> +		.type = IIO_EV_TYPE_THRESH,
>>>> +		.dir = IIO_EV_DIR_RISING,
>>>> +		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
>>>> +	}, {
>>>> +		.type = IIO_EV_TYPE_THRESH,
>>>> +		.dir = IIO_EV_DIR_FALLING,
>>>> +		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
>>>> +	}, {
>>>> +		.type = IIO_EV_TYPE_THRESH,
>>>> +		.mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD),
>>>> +	}, {
>>>> +		.type = IIO_EV_TYPE_THRESH_ADAPTIVE,
>>>> +		.mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
>>>> +			BIT(IIO_EV_INFO_ENABLE),
>>>> +	}, {
>>>> +		.type = IIO_EV_TYPE_THRESH,
>>>> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
>>> This matches an entry above for type. Don't have separate entries.
>>>> +	},
>>>> +};
>>>> +
>>>> +static struct iio_event_spec apds9306_event_spec_clear[] = {
>>>> +	{
>>>> +		.type = IIO_EV_TYPE_THRESH,
>>>> +		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
>>>> +	},
>>>> +};
>>>> +
>>>> +#define APDS9306_CHANNEL(_type) \
>>>> +	.type = _type, \
>>>> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) | \
>>>> +		BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>>>> +	.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | \
>>>> +		BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>>>
>>> Scale on the intensity channel is interesting...  What are the units?
>>> There tend not to be any well defined units for intensity (as opposed
>>> to illuminance).  There may be gain on the signal, but it won't be in untils
>>> that map directly to a scale userspace should apply.  This is one of the
>>> rare reasons for using the HARDWARE_GAIN element of the ABI.
>>>
>>> A tricky corner however as relationship between raw value and hardwaregain
>>> is not tightly defined (as it can be really weird!)
>> Hi Jonathan,
>>
>> Thank you for taking time for reviewing and clearing all my tiny doubts and
>> queries especially for the dt and versioning part. Much appreciated.
>>
>> In the above case, should I not expose scale for the "clear" channel? Rather,
>> how should I expose the "clear" channel to userspace?
> What is the scale?  What units to you get after applying it?
The scale is in Lux. The output after applying is Lux.
> 
> I suspect it's not well defined.
> 
> Not sure what we've done in similar cases in the past.  Perhaps have
> a look at those.  There may be precedence for still using scale.
Sure, I'll look it up.
> 
> I'm about to jump on a long haul flight (delayed - sigh) so don't have
> time to look myself!
> 
No worries. I will update notes and application in next version.
Have a good trip to where ever you are going. Are you coming to Australia
by any chance?
Thanks for the reply.

Regards,
Subhajit Ghosh

Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[Matti Vaittinen]

On 11/6/23 14:04, Subhajit Ghosh wrote:
> On 6/11/23 21:43, Jonathan Cameron wrote:
>> On Tue, 31 Oct 2023 19:08:08 +1030
>> Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:
>>
>>>> Scale on the intensity channel is interesting...  What are the units?
>>>> There tend not to be any well defined units for intensity (as opposed
>>>> to illuminance).  There may be gain on the signal, but it won't be 
>>>> in untils
>>>> that map directly to a scale userspace should apply.  This is one of 
>>>> the
>>>> rare reasons for using the HARDWARE_GAIN element of the ABI.
>>>>
>>>> A tricky corner however as relationship between raw value and 
>>>> hardwaregain
>>>> is not tightly defined (as it can be really weird!)
>>> Hi Jonathan,
>>>
>>> Thank you for taking time for reviewing and clearing all my tiny 
>>> doubts and
>>> queries especially for the dt and versioning part. Much appreciated.
>>>
>>> In the above case, should I not expose scale for the "clear" channel? 
>>> Rather,
>>> how should I expose the "clear" channel to userspace?
>> What is the scale?  What units to you get after applying it?
> The scale is in Lux. The output after applying is Lux.

Hi Subhajit,

I am by no means an expert here but maybe you could check if the channel 
should be of type 'illuminance'? (To me 'Lux' sounds like an unit of 
illuminance rather than intensity).

Yours,
	-- Matti

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

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

Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[Jonathan Cameron]

On Mon, 6 Nov 2023 14:10:43 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 11/6/23 14:04, Subhajit Ghosh wrote:
> > On 6/11/23 21:43, Jonathan Cameron wrote:  
> >> On Tue, 31 Oct 2023 19:08:08 +1030
> >> Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:
> >>  
> >>>> Scale on the intensity channel is interesting...  What are the units?
> >>>> There tend not to be any well defined units for intensity (as opposed
> >>>> to illuminance).  There may be gain on the signal, but it won't be 
> >>>> in untils
> >>>> that map directly to a scale userspace should apply.  This is one of 
> >>>> the
> >>>> rare reasons for using the HARDWARE_GAIN element of the ABI.
> >>>>
> >>>> A tricky corner however as relationship between raw value and 
> >>>> hardwaregain
> >>>> is not tightly defined (as it can be really weird!)  
> >>> Hi Jonathan,
> >>>
> >>> Thank you for taking time for reviewing and clearing all my tiny 
> >>> doubts and
> >>> queries especially for the dt and versioning part. Much appreciated.
> >>>
> >>> In the above case, should I not expose scale for the "clear" channel? 
> >>> Rather,
> >>> how should I expose the "clear" channel to userspace?  
> >> What is the scale?  What units to you get after applying it?  
> > The scale is in Lux. The output after applying is Lux.  
> 
> Hi Subhajit,
> 
> I am by no means an expert here but maybe you could check if the channel 
> should be of type 'illuminance'? (To me 'Lux' sounds like an unit of 
> illuminance rather than intensity).

Absolutely. Light measurements are 'weird'. Lux is a measurement of
light as if the human eye were looking at it. Unfortunately light sensors
don't have the same sensitivity curves as the eye so instead they tend
to do it either by some careful choice of filters, a horrible approximation
based on assumption of day light, or most commonly by combining the inputs
of several different light sensors.

Clear normally means sensitive to both visible light and infrared which
means you need to remove the infrared part to get closer to human
eye response.   Thus in other light sensors, we can't assign standard
units to the intensity channels (and so don't provide a scale).
We just don't have enough information to work out what they are measuring.
In theory we could provide the full sensitivity curve (usually somewhere
in the datasheet) but they can be complex multi-peak things so we don't
so far.

Jonathan


> 
> Yours,
> 	-- Matti
> 

Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[kernel test robot]

Hi Subhajit,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 611da07b89fdd53f140d7b33013f255bf0ed8f34]

url:    https://github.com/intel-lab-lkp/linux/commits/Subhajit-Ghosh/dt-bindings-iio-light-Avago-APDS9306/20231027-154954
base:   611da07b89fdd53f140d7b33013f255bf0ed8f34
patch link:    https://lore.kernel.org/r/20231027074545.6055-3-subhajit.ghosh%40tweaklogic.com
patch subject: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor
config: powerpc64-allyesconfig (https://download.01.org/0day-ci/archive/20231105/202311052102.1GrBH0gk-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231105/202311052102.1GrBH0gk-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/202311052102.1GrBH0gk-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/iio/light/apds9306.c:598:10: warning: variable 'ret' is uninitialized when used here [-Wuninitialized]
     598 |                 return ret;
         |                        ^~~
   drivers/iio/light/apds9306.c:593:9: note: initialize the variable 'ret' to silence this warning
     593 |         int ret, intg_old, gain_old, gain_new, gain_new_closest;
         |                ^
         |                 = 0
   1 warning generated.


vim +/ret +598 drivers/iio/light/apds9306.c

   589	
   590	static int apds9306_intg_time_set(struct apds9306_data *data, int val2)
   591	{
   592		struct device *dev = data->dev;
   593		int ret, intg_old, gain_old, gain_new, gain_new_closest;
   594		bool ok;
   595	
   596		if (!iio_gts_valid_time(&data->gts, val2)) {
   597			dev_err(dev, "Unsupported integration time %u\n", val2);
 > 598			return ret;
   599		}
   600	
   601		intg_old = iio_gts_find_int_time_by_sel(&data->gts,
   602							data->intg_time_idx);
   603		if (ret < 0)
   604			return ret;
   605	
   606		if (intg_old == val2)
   607			return 0;
   608	
   609		gain_old = iio_gts_find_gain_by_sel(&data->gts, data->gain_idx);
   610		if (gain_old < 0)
   611			return gain_old;
   612	
   613		ret = iio_gts_find_new_gain_by_old_gain_time(&data->gts, gain_old,
   614							     intg_old, val2, &gain_new);
   615		if (gain_new < 0) {
   616			dev_err(dev, "Unsupported gain with time\n");
   617			return gain_new;
   618		}
   619	
   620		gain_new_closest = iio_find_closest_gain_low(&data->gts, gain_new, &ok);
   621		if (gain_new_closest < 0) {
   622			gain_new_closest = iio_gts_get_min_gain(&data->gts);
   623			if (gain_new_closest < 0)
   624				return gain_new_closest < 0;
   625		}
   626		if (!ok)
   627			dev_dbg(dev, "Unable to find optimum gain, setting minimum");
   628	
   629		ret = iio_gts_find_sel_by_int_time(&data->gts, val2);
   630		if (ret < 0)
   631			return ret;
   632	
   633		ret = apds9306_intg_time_set_hw(data, ret);
   634		if (ret)
   635			return ret;
   636	
   637		ret = iio_gts_find_sel_by_gain(&data->gts, gain_new_closest);
   638		if (ret < 0)
   639			return ret;
   640	
   641		return apds9306_gain_set_hw(data, ret);
   642	}
   643	

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

Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[Andy Shevchenko]

On Sun, Nov 05, 2023 at 10:22:07PM +0800, kernel test robot wrote:

> >> drivers/iio/light/apds9306.c:598:10: warning: variable 'ret' is uninitialized when used here [-Wuninitialized]
>      598 |                 return ret;
>          |                        ^~~
>    drivers/iio/light/apds9306.c:593:9: note: initialize the variable 'ret' to silence this warning
>      593 |         int ret, intg_old, gain_old, gain_new, gain_new_closest;
>          |                ^
>          |                 = 0
>    1 warning generated.

Bad advice, just use correct error code instead of ret.

>    590	static int apds9306_intg_time_set(struct apds9306_data *data, int val2)
>    591	{
>    592		struct device *dev = data->dev;
>    593		int ret, intg_old, gain_old, gain_new, gain_new_closest;
>    594		bool ok;
>    595	
>    596		if (!iio_gts_valid_time(&data->gts, val2)) {
>    597			dev_err(dev, "Unsupported integration time %u\n", val2);
>  > 598			return ret;
>    599		}
>    600	
>    601		intg_old = iio_gts_find_int_time_by_sel(&data->gts,
>    602							data->intg_time_idx);
>    603		if (ret < 0)
>    604			return ret;
>    605	
>    606		if (intg_old == val2)
>    607			return 0;
>    608	
>    609		gain_old = iio_gts_find_gain_by_sel(&data->gts, data->gain_idx);
>    610		if (gain_old < 0)
>    611			return gain_old;
>    612	
>    613		ret = iio_gts_find_new_gain_by_old_gain_time(&data->gts, gain_old,
>    614							     intg_old, val2, &gain_new);
>    615		if (gain_new < 0) {
>    616			dev_err(dev, "Unsupported gain with time\n");
>    617			return gain_new;
>    618		}
>    619	
>    620		gain_new_closest = iio_find_closest_gain_low(&data->gts, gain_new, &ok);
>    621		if (gain_new_closest < 0) {
>    622			gain_new_closest = iio_gts_get_min_gain(&data->gts);
>    623			if (gain_new_closest < 0)
>    624				return gain_new_closest < 0;
>    625		}
>    626		if (!ok)
>    627			dev_dbg(dev, "Unable to find optimum gain, setting minimum");
>    628	
>    629		ret = iio_gts_find_sel_by_int_time(&data->gts, val2);
>    630		if (ret < 0)
>    631			return ret;
>    632	
>    633		ret = apds9306_intg_time_set_hw(data, ret);
>    634		if (ret)
>    635			return ret;
>    636	
>    637		ret = iio_gts_find_sel_by_gain(&data->gts, gain_new_closest);
>    638		if (ret < 0)
>    639			return ret;
>    640	
>    641		return apds9306_gain_set_hw(data, ret);
>    642	}

-- 
With Best Regards,
Andy Shevchenko