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

[PATCH 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
- Pocessed data (Lux) values for als channel
- Up to 20 bit resolution
- 20 bit data register for each channel
- Common Configurable items for both channels
    - Integration Time
    - Measurement Frequency
    - 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

Link: https://lore.kernel.org/all/20230411011203.5013-1-subhajit.ghosh@tweaklogic.com/
Link: https://patchwork.kernel.org/project/linux-iio/cover/20230411011203.5013-1-subhajit.ghosh@tweaklogic.com/

Sysfs structure:
root@stm32mp1:~# tree -I 'dev|name|of_node|power|subsystem|uevent' \
> /sys/bus/iio/devices/iio:device0/
/sys/bus/iio/devices/iio:device0/
|-- 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_input
|-- in_illuminance_raw
|-- in_intensity_clear_raw
|-- integration_time
|-- integration_time_available
|-- sampling_frequency
|-- sampling_frequency_available
|-- scale
|-- scale_available
`-- waiting_for_supplier

RFC -> v0
 - DT binding review by Rob and Krzysztof:
  - Verified with dt_binding_check
  - Removed the last/redundant "bindings" word

 - Review by Andy:
  - Dropped blank line and reordered initial comments
  - Sorted header files
  - Added kernel-doc for the private structure
  - Removed regmap internal lock
  - Used regmap_read_poll_timeout() function instead of while loop
    in apds9306_read_data()
  - Applied fixes as per review

 - Review by Jonathan:
  - Updated apds9306_read_data() and apds9306_irq_handler() as per
    review. If interrupts are enabled together with userspace read
    of raw and processed adc values, then events can be pushed both
    by the interrupt handler and apds9306_read_data(). If the
    interrupt handler gets a data ready for read flag then it sets
    a flag in the private data structure which is used by 
    apds9306_read_data().
  - ABI update - In events, per cnannel enable for both
    channels, removing custom sysfs attributes for channel selection.
  - Added lux calculation. Page 4 of the datasheet has test results
    for only the default integration time at the default hardware gain.
    Normalized the values for all hardware gains and all supported
    integration times as per the part ID. I got consistent results
    when compared with a lux meter in the default range.
  - Other fixes as commented
  - Implemented IIO_GTS_HELPER
  - Shuffled functions for logical split and readability

Apologies for this huge delay in submitting this patch from RFC to v0.
I had a change of job and subsequent relocation.
Future revisions will not be delayed.

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

 .../bindings/iio/light/avago,apds9306.yaml    |   49 +
 drivers/iio/light/Kconfig                     |   12 +
 drivers/iio/light/Makefile                    |    1 +
 drivers/iio/light/apds9306.c                  | 1381 +++++++++++++++++
 4 files changed, 1443 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds9306.yaml
 create mode 100644 drivers/iio/light/apds9306.c


base-commit: b9ddbb0cde2adcedda26045cc58f31316a492215
-- 
2.34.1

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

[Subhajit Ghosh]

Add devicetree bindings for Avago APDS9306 Ambient Light Sensor.

Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
---
 .../bindings/iio/light/avago,apds9306.yaml    | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds9306.yaml

diff --git a/Documentation/devicetree/bindings/iio/light/avago,apds9306.yaml b/Documentation/devicetree/bindings/iio/light/avago,apds9306.yaml
new file mode 100644
index 000000000000..e8bb897782fc
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/avago,apds9306.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/avago,apds9306.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Avago APDS9306 Ambient Light Sensor
+
+maintainers:
+  - Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
+
+description:
+  Datasheet at https://docs.broadcom.com/doc/AV02-4755EN
+
+properties:
+  compatible:
+    const: avago,apds9306
+
+  reg:
+    maxItems: 1
+
+  vin-supply:
+    description: Regulator supply to the sensor
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        light-sensor@52 {
+            compatible = "avago,apds9306";
+            reg = <0x52>;
+            interrupt-parent = <&gpiof>;
+            interrupts = <6 IRQ_TYPE_LEVEL_LOW>;
+        };
+    };
+...
-- 
2.34.1

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

[Krzysztof Kozlowski]

On 08/10/2023 17:48, Subhajit Ghosh wrote:
> Add devicetree bindings for Avago APDS9306 Ambient Light Sensor.
> 
> Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
> ---
>  .../bindings/iio/light/avago,apds9306.yaml    | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds9306.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/avago,apds9306.yaml b/Documentation/devicetree/bindings/iio/light/avago,apds9306.yaml
> new file mode 100644
> index 000000000000..e8bb897782fc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/avago,apds9306.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/light/avago,apds9306.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Avago APDS9306 Ambient Light Sensor
> +
> +maintainers:
> +  - Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
> +
> +description:
> +  Datasheet at https://docs.broadcom.com/doc/AV02-4755EN

This is exactly the same as two other Avago devices. It should be
squashed - first two device schemas squashed, then add new device support.

Also, the supply is not vin, but vdd.

Best regards,
Krzysztof

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

[Subhajit Ghosh]

>> +description:
>> +  Datasheet at https://docs.broadcom.com/doc/AV02-4755EN
> 
> This is exactly the same as two other Avago devices. It should be
> squashed - first two device schemas squashed, then add new device support.
> 
> Also, the supply is not vin, but vdd.
>

Yes, they look similar. I will combine them all in a single yaml file in
the next revision. Thank you Krzysztof.

Regards,
Subhajit Ghosh

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

[Matti Vaittinen]

On 10/8/23 18:48, Subhajit Ghosh wrote:
> Add devicetree bindings for Avago APDS9306 Ambient Light Sensor.
> 
> Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
> ---
>   .../bindings/iio/light/avago,apds9306.yaml    | 49 +++++++++++++++++++
>   1 file changed, 49 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds9306.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/avago,apds9306.yaml b/Documentation/devicetree/bindings/iio/light/avago,apds9306.yaml
> new file mode 100644
> index 000000000000..e8bb897782fc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/avago,apds9306.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/light/avago,apds9306.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Avago APDS9306 Ambient Light Sensor
> +
> +maintainers:
> +  - Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
> +
> +description:
> +  Datasheet at https://docs.broadcom.com/doc/AV02-4755EN
> +
> +properties:
> +  compatible:
> +    const: avago,apds9306

I see the driver supports two different variants of this IC, 
differentiated by the part-ID register. Variants are named as apds9306 
and apds9306-065. I wonder if we could/should have different compatibles 
for them?

Yours,
	-- Matti

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

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

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

[Subhajit Ghosh]

On 10/10/23 19:22, Matti Vaittinen wrote:

>> +properties:
>> +  compatible:
>> +    const: avago,apds9306
> 
> I see the driver supports two different variants of this IC, differentiated by the part-ID register. Variants are named as apds9306 and apds9306-065. I wonder if we could/should have different compatibles for them?
> 

Yes, we can. It makes sense. I'll implement that.

Regards,
Subhajit Ghosh

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

[Jonathan Cameron]

On Tue, 10 Oct 2023 22:48:43 +1030
Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:

> On 10/10/23 19:22, Matti Vaittinen wrote:
> 
> >> +properties:
> >> +  compatible:
> >> +    const: avago,apds9306  
> > 
> > I see the driver supports two different variants of this IC, differentiated by the part-ID register. Variants are named as apds9306 and apds9306-065. I wonder if we could/should have different compatibles for them?
> >   
> 
> Yes, we can. It makes sense. I'll implement that.
We could.  The reason to do so is that we might in future want to use
fallback compatibles.  So we want to allow a new DT to work with older
kernel by saying - I have a new device, but it is fully compatible with
this earlier one.  In those cases we check the ID as your driver current
does, but just print a warning that we aren't sure what the device is so
are going with what the DT told us to fall back to.

Jonathan

> 
> Regards,
> Subhajit Ghosh
> 

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

[Rob Herring]

On Tue, Oct 10, 2023 at 11:52:28AM +0300, Matti Vaittinen wrote:
> On 10/8/23 18:48, Subhajit Ghosh wrote:
> > Add devicetree bindings for Avago APDS9306 Ambient Light Sensor.
> > 
> > Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
> > ---
> >   .../bindings/iio/light/avago,apds9306.yaml    | 49 +++++++++++++++++++
> >   1 file changed, 49 insertions(+)
> >   create mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds9306.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/light/avago,apds9306.yaml b/Documentation/devicetree/bindings/iio/light/avago,apds9306.yaml
> > new file mode 100644
> > index 000000000000..e8bb897782fc
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/light/avago,apds9306.yaml
> > @@ -0,0 +1,49 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/light/avago,apds9306.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Avago APDS9306 Ambient Light Sensor
> > +
> > +maintainers:
> > +  - Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
> > +
> > +description:
> > +  Datasheet at https://docs.broadcom.com/doc/AV02-4755EN
> > +
> > +properties:
> > +  compatible:
> > +    const: avago,apds9306
> 
> I see the driver supports two different variants of this IC, differentiated
> by the part-ID register. Variants are named as apds9306 and apds9306-065. I
> wonder if we could/should have different compatibles for them?

If 1 compatible is sufficient to know how to power on both devices and 
read the part-ID register, then no need for different compatibles.

Rob

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

[Subhajit Ghosh]

On 11/10/23 02:49, Rob Herring wrote:

>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: avago,apds9306
>>
>> I see the driver supports two different variants of this IC, differentiated
>> by the part-ID register. Variants are named as apds9306 and apds9306-065. I
>> wonder if we could/should have different compatibles for them?
> 
> If 1 compatible is sufficient to know how to power on both devices and
> read the part-ID register, then no need for different compatibles.
> 
> Rob
Understood. Thanks Rob.

Regards,
Subhajit Ghosh

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

[Jonathan Cameron]

On Mon,  9 Oct 2023 02:18:56 +1030
Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:

> Add devicetree bindings for Avago APDS9306 Ambient Light Sensor.
> 
> Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
> ---
>  .../bindings/iio/light/avago,apds9306.yaml    | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds9306.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/avago,apds9306.yaml b/Documentation/devicetree/bindings/iio/light/avago,apds9306.yaml
> new file mode 100644
> index 000000000000..e8bb897782fc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/avago,apds9306.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/light/avago,apds9306.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Avago APDS9306 Ambient Light Sensor
> +
> +maintainers:
> +  - Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
> +
> +description:
> +  Datasheet at https://docs.broadcom.com/doc/AV02-4755EN
> +
> +properties:
> +  compatible:
> +    const: avago,apds9306
> +
> +  reg:
> +    maxItems: 1
> +
> +  vin-supply:
> +    description: Regulator supply to the sensor

Why vin?  It seems to be vdd on the datasheet.
We tend to match the datasheet naming for power supplies as that is normally
what is seen on circuit board schematics.


> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        light-sensor@52 {
> +            compatible = "avago,apds9306";
> +            reg = <0x52>;
> +            interrupt-parent = <&gpiof>;
> +            interrupts = <6 IRQ_TYPE_LEVEL_LOW>;
> +        };
> +    };
> +...

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

[Subhajit Ghosh]

On 11/10/23 00:21, Jonathan Cameron wrote:

>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  vin-supply:
>> +    description: Regulator supply to the sensor
> 
> Why vin?  It seems to be vdd on the datasheet.
> We tend to match the datasheet naming for power supplies as that is normally
> what is seen on circuit board schematics.
Got it, I'll fix it. Thanks for looking into it.

Regards,
Subhajit Ghosh

[PATCH 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.
This driver exposes raw values for both the channels and
processed(lux) values for the als channel.
Support for both with or without hardware interrupt
configurations are provided.

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

Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
---
 drivers/iio/light/Kconfig    |   12 +
 drivers/iio/light/Makefile   |    1 +
 drivers/iio/light/apds9306.c | 1381 ++++++++++++++++++++++++++++++++++
 3 files changed, 1394 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..02c8285b230b
--- /dev/null
+++ b/drivers/iio/light/apds9306.c
@@ -0,0 +1,1381 @@
+// 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,
+};
+
+enum apds9306_int_channels {
+	CLEAR,
+	ALS,
+};
+
+/**
+ * struct part_id_nlux_per_count - Part no. & corresponding nano lux per count
+ * @part_id: Part ID of the device
+ * @nlux_per_count: Nano lux per ADC count
+ */
+struct part_id_nlux_per_count {
+	int part_id;
+	int nlux_per_count;
+};
+
+/*
+ * 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,
+ * saving in nano lux per count.
+ * Nano Lux per count = (340.134 * 1000000000)/ (32 * 3 * 2000) for apds9306
+ * Nano Lux per count = (293.69 * 1000000000)/ (32 * 3 * 2000) for apds9306-065
+ */
+static struct part_id_nlux_per_count apds9306_part_id_nlux_per_count[] = {
+	{.part_id = 0xB1, .nlux_per_count = 1787156},
+	{.part_id = 0xB3, .nlux_per_count = 1529635},
+};
+
+/**
+ * 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_THRESH_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;
+	/*
+	 * Guard register access, single data reads, power up and down sequence.
+	 */
+	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
+ *
+ * Max total gain = HW gain * Intg time gain (18 * 128) = 2304
+ *
+ * For least precision loss, multiplier of 4 is required with
+ * 1736111.1 nanos (0.001736111).
+ */
+#define APDS9306_SCALE_1X	4
+
+#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) |
+			BIT(IIO_CHAN_INFO_PROCESSED),
+		.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) |
+			BIT(IIO_CHAN_INFO_PROCESSED),
+	}, {
+		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 15]");
+
+/* ALS_THRESH_VAR available */
+IIO_CONST_ATTR(thresh_adaptive_either_values_available, "[0 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,
+	.disable_locking = true,
+};
+
+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
+	 * 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_read_lux(struct apds9306_data *data, int *val)
+{
+	int ret, raw_count, gain, itime, scale, scale2;
+	u64 lux, new_scale;
+
+	itime = iio_gts_find_int_time_by_sel(&data->gts, data->intg_time_idx);
+	if (itime < 0)
+		return itime;
+
+	gain = iio_gts_find_gain_by_sel(&data->gts, data->gain_idx);
+	if (gain < 0)
+		return gain;
+
+	ret = iio_gts_get_scale(&data->gts, gain, itime, &scale, &scale2);
+	if (ret)
+		return ret;
+
+	ret = apds9306_read_data(data, &raw_count, APDS9306_ALS_DATA_0);
+	if (ret)
+		return ret;
+
+	lux = (u64)raw_count * data->nlux_per_count;
+
+	if (scale) {
+		new_scale = ((u64)scale * NANO) + scale2;
+		/* NANO scale is greater than max int, so reduce it */
+		do_div(new_scale, 10);
+		scale2 = (int)new_scale;
+		lux *= 10;
+	}
+
+	lux *= APDS9306_SCALE_1X;
+	do_div(lux, scale2);
+	*val = lux;
+
+	return 0;
+}
+
+static int apds9306_intg_time_get(struct apds9306_data *data, int *val2)
+{
+	*val2 = iio_gts_find_int_time_by_sel(&data->gts, data->intg_time_idx);
+	if (*val2 < 0)
+		return *val2;
+
+	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;
+			break;
+		}
+
+	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;
+
+	mutex_lock(&data->mutex);
+	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;
+		ret = apds9306_read_data(data, val, reg);
+		if (ret)
+			break;
+		ret = IIO_VAL_INT;
+		*val2 = 0;
+		break;
+	case IIO_CHAN_INFO_PROCESSED:
+		ret = apds9306_read_lux(data, val);
+		if (ret)
+			break;
+		*val2 = 0;
+		ret = IIO_VAL_INT;
+		break;
+	case IIO_CHAN_INFO_INT_TIME:
+		*val = 0;
+		ret = apds9306_intg_time_get(data, val2);
+		if (ret)
+			break;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = apds9306_sampling_freq_get(data, val, val2);
+		if (ret)
+			break;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+	case IIO_CHAN_INFO_SCALE:
+		ret = apds9306_scale_get(data, val, val2);
+		if (ret)
+			break;
+		ret = IIO_VAL_INT_PLUS_NANO;
+		break;
+	default:
+		ret = -EINVAL;
+	}
+	mutex_unlock(&data->mutex);
+
+	return ret;
+};
+
+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;
+
+	mutex_lock(&data->mutex);
+	switch (type) {
+	case IIO_EV_TYPE_THRESH:
+		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);
+		break;
+	case IIO_EV_TYPE_THRESH_ADAPTIVE:
+		ret = apds9306_event_thresh_adaptive_get(data, val);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+	mutex_unlock(&data->mutex);
+
+	if (ret)
+		return ret;
+
+	*val2 = 0;
+	return IIO_VAL_INT;
+}
+
+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;
+
+	mutex_lock(&data->mutex);
+	switch (type) {
+	case IIO_EV_TYPE_THRESH:
+		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);
+		break;
+	case IIO_EV_TYPE_THRESH_ADAPTIVE:
+		ret = apds9306_event_thresh_adaptive_set(data, val);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+	mutex_unlock(&data->mutex);
+	return ret;
+}
+
+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 val, val2;
+	int ret;
+
+	mutex_lock(&data->mutex);
+	switch (type) {
+	case IIO_EV_TYPE_THRESH:
+		ret = regmap_field_read(data->regfield_int_en, &val);
+		if (ret)
+			break;
+		ret = regmap_field_read(data->regfield_int_src, &val2);
+		if (ret)
+			break;
+		if (chan->type == IIO_LIGHT)
+			ret = val & val2;
+		else if (chan->type == IIO_INTENSITY)
+			ret = val & !val2;
+		else
+			ret = -EINVAL;
+		break;
+	case IIO_EV_TYPE_THRESH_ADAPTIVE:
+		ret = regmap_field_read(data->regfield_int_thresh_var_en,
+					&val);
+		if (ret)
+			break;
+		ret = val;
+		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;
+
+	state = !!state;
+	mutex_lock(&data->mutex);
+	switch (type) {
+	case IIO_EV_TYPE_THRESH:
+		if (state) {
+			if (chan->type == IIO_LIGHT) {
+				ret = regmap_field_write(data->regfield_int_src, 1);
+				if (ret)
+					break;
+			} else if (chan->type == IIO_INTENSITY) {
+				ret = regmap_field_write(data->regfield_int_src, 0);
+				if (ret)
+					break;
+			} else {
+				ret = -EINVAL;
+				break;
+			}
+		}
+		ret = regmap_field_write(data->regfield_int_en, state);
+		if (ret)
+			break;
+		ret = apds9306_runtime_power(data, state);
+		break;
+	case IIO_EV_TYPE_THRESH_ADAPTIVE:
+		ret = regmap_field_write(data->regfield_int_thresh_var_en,
+					 state);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+#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 get_device_id_lux_per_count(struct apds9306_data *data)
+{
+	int ret, part_id;
+
+	ret = regmap_read(data->regmap, APDS9306_PART_ID, &part_id);
+	if (ret)
+		return ret;
+
+	if (part_id == apds9306_part_id_nlux_per_count[0].part_id)
+		data->nlux_per_count =
+			apds9306_part_id_nlux_per_count[0].nlux_per_count;
+	else if (part_id == apds9306_part_id_nlux_per_count[1].part_id)
+		data->nlux_per_count =
+			apds9306_part_id_nlux_per_count[1].nlux_per_count;
+	else
+		return -ENXIO;
+
+	return 0;
+}
+
+static void apds9306_powerdown(void *ptr)
+{
+	struct apds9306_data *data = (struct apds9306_data *)ptr;
+	struct device *dev = data->dev;
+	int ret;
+
+	/* Disable interrupts */
+	ret = regmap_field_write(data->regfield_int_thresh_var_en, 0);
+	if (ret)
+		dev_err(dev, "Failed to disable variance interrupts\n");
+	ret = regmap_field_write(data->regfield_int_en, 0);
+	if (ret)
+		dev_err(dev, "Failed to disable interrupts\n");
+	/* Put the device in standby mode */
+	ret = apds9306_power_state(data, STANDBY);
+	if (ret)
+		dev_err(dev, "Failed to power down device\n");
+}
+
+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);
+
+	ret = regmap_reinit_cache(data->regmap, &apds9306_regmap);
+	if (ret)
+		return ret;
+
+	/* Get lux per count value according to the device ID */
+	ret = get_device_id_lux_per_count(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, "vin");
+	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_TRIGGER_FALLING | 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 = devm_iio_init_iio_gts(dev, APDS9306_SCALE_1X, 0, apds9306_gains,
+				    ARRAY_SIZE(apds9306_gains), apds9306_itimes,
+				    ARRAY_SIZE(apds9306_itimes), &data->gts);
+	if (ret)
+		return ret;
+
+	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");
+
+	ret = apds9306_init_device(data);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to init device\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);
+	int ret;
+
+	ret = apds9306_power_state(data, STANDBY);
+	if (ret)
+		regcache_cache_only(data->regmap, true);
+
+	return ret;
+}
+
+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);
+	int ret;
+
+	regcache_cache_only(data->regmap, false);
+	ret = regcache_sync(data->regmap);
+	if (ret)
+		return ret;
+	ret = apds9306_power_state(data, ACTIVE);
+	if (ret)
+		regcache_cache_only(data->regmap, true);
+
+	return 0;
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(apds9306_pm_ops,
+				 apds9306_runtime_suspend,
+				 apds9306_runtime_resume,
+				 NULL);
+
+static const struct i2c_device_id apds9306_id[] = {
+	{ "apds9306" }, { }
+};
+MODULE_DEVICE_TABLE(i2c, apds9306_id);
+
+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_new = apds9306_probe,
+	.id_table = apds9306_id,
+};
+
+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 2/2] iio: light: Add support for APDS9306 Light Sensor

[Matti Vaittinen]

On 10/8/23 18:48, Subhajit Ghosh wrote:
> Driver support for Avago (Broadcom) APDS9306
> Ambient Light Sensor with als and clear channels.
> This driver exposes raw values for both the channels and
> processed(lux) values for the als channel.
> Support for both with or without hardware interrupt
> configurations are provided.
> 
> Datasheet at https://docs.broadcom.com/doc/AV02-4755EN
> 
> Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>

Hi Subhajit,

To my eyes this driver looks nice. Just spotted two minor things.

> ---
>   drivers/iio/light/Kconfig    |   12 +
>   drivers/iio/light/Makefile   |    1 +
>   drivers/iio/light/apds9306.c | 1381 ++++++++++++++++++++++++++++++++++
>   3 files changed, 1394 insertions(+)
>   create mode 100644 drivers/iio/light/apds9306.c
> 
...

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

Returning the truth value on purpose? :)

> +	}
> +	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 get_device_id_lux_per_count(struct apds9306_data *data)
> +{
> +	int ret, part_id;
> +
> +	ret = regmap_read(data->regmap, APDS9306_PART_ID, &part_id);
> +	if (ret)
> +		return ret;
> +
> +	if (part_id == apds9306_part_id_nlux_per_count[0].part_id)
> +		data->nlux_per_count =
> +			apds9306_part_id_nlux_per_count[0].nlux_per_count;
> +	else if (part_id == apds9306_part_id_nlux_per_count[1].part_id)
> +		data->nlux_per_count =
> +			apds9306_part_id_nlux_per_count[1].nlux_per_count;
> +	else
> +		return -ENXIO;

I think we should be able to differentiate between the IC variants by DT 
compatible. (Commented that on bindings patch). Not sure if we need to 
support cases where the sensor is instantiated without device-tree. I am 
not super happy when code requires the part-id to be known if we have 
separate compatibles for variants. Can we in dt-case just print a 
warning if the part-ID is not what we expect - and proceed assuming the 
nlux_per_count based on the DT information? (Sometimes we see new 
variants with same part-IDs - or many part-IDs with no SW changes 
needed. Hence maintaining the part-ID lists may be tedious). This is 
just some pondering though, no strong requirements from my side

> +
> +	return 0;
> +}
> +

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 2/2] iio: light: Add support for APDS9306 Light Sensor

[Subhajit Ghosh]

On 10/10/23 20:15, Matti Vaittinen wrote:

> 
> To my eyes this driver looks nice. Just spotted two minor things.
> 

Thanks Matti. Nice is one nice thing I heard after some time!

>> +    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;
> 
> Returning the truth value on purpose? :)

Nope, it's a bug. I'll fix it.

>> +static int get_device_id_lux_per_count(struct apds9306_data *data)
>> +{
>> +    int ret, part_id;
>> +
>> +    ret = regmap_read(data->regmap, APDS9306_PART_ID, &part_id);
>> +    if (ret)
>> +        return ret;
>> +
>> +    if (part_id == apds9306_part_id_nlux_per_count[0].part_id)
>> +        data->nlux_per_count =
>> +            apds9306_part_id_nlux_per_count[0].nlux_per_count;
>> +    else if (part_id == apds9306_part_id_nlux_per_count[1].part_id)
>> +        data->nlux_per_count =
>> +            apds9306_part_id_nlux_per_count[1].nlux_per_count;
>> +    else
>> +        return -ENXIO;
> 
> I think we should be able to differentiate between the IC variants by DT compatible. (Commented that on bindings patch). Not sure if we need to support cases where the sensor is instantiated without device-tree. I am not super happy when code requires the part-id to be known if we have separate compatibles for variants. Can we in dt-case just print a warning if the part-ID is not what we expect - and proceed assuming the nlux_per_count based on the DT information? (Sometimes we see new variants with same part-IDs - or many part-IDs with no SW changes needed. Hence maintaining the part-ID lists may be tedious). This is just some pondering though, no strong requirements from my side

Yes, I agree with you. The purpose of DT is to provide proper hardware descriptions.
I will throw a warning as well as implement a compatibility match.

Regards,
Subhajit Ghosh

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

[Jonathan Cameron]

On Mon,  9 Oct 2023 02:18:57 +1030
Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:

> Driver support for Avago (Broadcom) APDS9306
> Ambient Light Sensor with als and clear channels.
> This driver exposes raw values for both the channels and
> processed(lux) values for the als channel.
> Support for both with or without hardware interrupt
> configurations are provided.
Hi Subhajit,

No need to wrap the patch description quite so short. Aim
for up to 75 char for a commit message (and 80 for the code)
Here you are under 60.

> 
> Datasheet at https://docs.broadcom.com/doc/AV02-4755EN
> 
There is a tag for datasheets in the format tags block so
Datasheet: https://docs.broadcom.com/doc/AV02-4755EN
> Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>

I took a quick look at the most similar part number adps9300 and
this does look substantially different but could you confirm you've
taken a look at the plausible drivers to which support for this part
could be added and perhaps mention why that doesn't make sense
I think it will be mainly feature set being different here, but also
it seems they have completely different register maps despite similar
part numbers!


A few other comments inline.

Thanks,

Jonathan

> diff --git a/drivers/iio/light/apds9306.c b/drivers/iio/light/apds9306.c
> new file mode 100644
> index 000000000000..02c8285b230b
> --- /dev/null
> +++ b/drivers/iio/light/apds9306.c


...


> +};
> +
> +enum apds9306_int_channels {
> +	CLEAR,
> +	ALS,
> +};

Is this used?

> +
> +/**
> + * struct part_id_nlux_per_count - Part no. & corresponding nano lux per count
> + * @part_id: Part ID of the device
> + * @nlux_per_count: Nano lux per ADC count
> + */
> +struct part_id_nlux_per_count {
> +	int part_id;
> +	int nlux_per_count;
> +};
> +
> +/*
> + * 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,
> + * saving in nano lux per count.
> + * Nano Lux per count = (340.134 * 1000000000)/ (32 * 3 * 2000) for apds9306
> + * Nano Lux per count = (293.69 * 1000000000)/ (32 * 3 * 2000) for apds9306-065

Even though it's a comment stick to kernel maths syntax and put a space before the /
Otherwise some script will complain it's not correctly formatted code :)

> + */
> +static struct part_id_nlux_per_count apds9306_part_id_nlux_per_count[] = {
> +	{.part_id = 0xB1, .nlux_per_count = 1787156},
> +	{.part_id = 0xB3, .nlux_per_count = 1529635},

Prefer { .part_id = 0xB3, .nlux_per_count = 1629635 },
for tables liek this as it ends up a tiny bit easier to read.

> +};

...

> +#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) |
> +			BIT(IIO_CHAN_INFO_PROCESSED),
> +		.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) |
> +			BIT(IIO_CHAN_INFO_PROCESSED),

This needs an explanation for why as a comment in the code.
We very rarely support both raw and processed for the same channel and
mostly when we do it is due to some historical changes.

You are using the gts stuff here so it should be possible to expose
the controls for scale necessary to have userspace perform the raw to processed
conversion.  

> +	}, {
> +		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 15]");
> +
> +/* ALS_THRESH_VAR available */
> +IIO_CONST_ATTR(thresh_adaptive_either_values_available, "[0 7]");
Not valid range syntax for IIO attributes, you need to include
the step.

[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 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,
> +	.disable_locking = true,
This normally deserves a statement of what you are doing about locking instead.

The interrupt controller for starters takes to no locks and can run concurrently
with other accesses from other CPUs.  That seems unwise.

> +};
+
...

> +
> +static int apds9306_intg_time_get(struct apds9306_data *data, int *val2)
> +{
> +	*val2 = iio_gts_find_int_time_by_sel(&data->gts, data->intg_time_idx);
> +	if (*val2 < 0)
> +		return *val2;

You shouldn't have side effects on *val2 if an error occurs.
Its not a bug in this case, but it is generally something to avoid

	int ret;

	ret = iio_gts...
	if (ret < 0)
		return ret;

	*val2 = ret;

	return 0;

> +
> +	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_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;
> +			break;
Might as well return here instead of break as nothing else to do.

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

> +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;
> +
> +	mutex_lock(&data->mutex);
> +	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;
> +		ret = apds9306_read_data(data, val, reg);
> +		if (ret)
> +			break;
> +		ret = IIO_VAL_INT;
> +		*val2 = 0;

As below.  No need to set *val2 to 0 if returning IIO_VAL_INT.

> +		break;
> +	case IIO_CHAN_INFO_PROCESSED:
> +		ret = apds9306_read_lux(data, val);
> +		if (ret)
> +			break;
> +		*val2 = 0;
> +		ret = IIO_VAL_INT;
> +		break;
> +	case IIO_CHAN_INFO_INT_TIME:
> +		*val = 0;
> +		ret = apds9306_intg_time_get(data, val2);
> +		if (ret)
> +			break;
> +		ret = IIO_VAL_INT_PLUS_MICRO;
> +		break;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = apds9306_sampling_freq_get(data, val, val2);
> +		if (ret)
> +			break;
> +		ret = IIO_VAL_INT_PLUS_MICRO;
> +		break;
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = apds9306_scale_get(data, val, val2);
> +		if (ret)
> +			break;
> +		ret = IIO_VAL_INT_PLUS_NANO;
> +		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;
> +
> +	mutex_lock(&data->mutex);
> +	switch (type) {
> +	case IIO_EV_TYPE_THRESH:
> +		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);
> +		break;
> +	case IIO_EV_TYPE_THRESH_ADAPTIVE:
> +		ret = apds9306_event_thresh_adaptive_get(data, val);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +	mutex_unlock(&data->mutex);
> +
> +	if (ret)
> +		return ret;
> +
> +	*val2 = 0;

The IIO core won't use val2 if you return IIO_VAL_INT, so don't bother setting it.

> +	return IIO_VAL_INT;
> +}
> +
> +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;
> +
> +	mutex_lock(&data->mutex);
> +	switch (type) {
> +	case IIO_EV_TYPE_THRESH:
> +		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);
> +		break;
> +	case IIO_EV_TYPE_THRESH_ADAPTIVE:
> +		ret = apds9306_event_thresh_adaptive_set(data, val);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +	mutex_unlock(&data->mutex);
> +	return ret;
> +}
> +
> +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 val, val2;
> +	int ret;
> +
> +	mutex_lock(&data->mutex);
As below
	guard(mutex)(&data->mutex);

should simplify this - I won't comment on this one above this point (reviewing backwards
through the code).

> +	switch (type) {
> +	case IIO_EV_TYPE_THRESH:
> +		ret = regmap_field_read(data->regfield_int_en, &val);
> +		if (ret)
> +			break;
> +		ret = regmap_field_read(data->regfield_int_src, &val2);
> +		if (ret)
> +			break;
> +		if (chan->type == IIO_LIGHT)
> +			ret = val & val2;
> +		else if (chan->type == IIO_INTENSITY)
> +			ret = val & !val2;

This logic would benefit from better variable naming.
en and src for example..

> +		else
> +			ret = -EINVAL;
> +		break;
> +	case IIO_EV_TYPE_THRESH_ADAPTIVE:
> +		ret = regmap_field_read(data->regfield_int_thresh_var_en,
> +					&val);
> +		if (ret)
> +			break;
> +		ret = val;
> +		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;
> +
> +	state = !!state;
> +	mutex_lock(&data->mutex);

Perfect place to use the new cleanup.h trickery here.

guard(mutex)(&data->mutex); 

and then you can just return in error paths which will simplify this code

> +	switch (type) {
> +	case IIO_EV_TYPE_THRESH:
> +		if (state) {
> +			if (chan->type == IIO_LIGHT) {
> +				ret = regmap_field_write(data->regfield_int_src, 1);
> +				if (ret)
> +					break;
> +			} else if (chan->type == IIO_INTENSITY) {
> +				ret = regmap_field_write(data->regfield_int_src, 0);
> +				if (ret)
> +					break;
> +			} else {
> +				ret = -EINVAL;
> +				break;
> +			}
> +		}
> +		ret = regmap_field_write(data->regfield_int_en, state);
> +		if (ret)
> +			break;
> +		ret = apds9306_runtime_power(data, state);
> +		break;
> +	case IIO_EV_TYPE_THRESH_ADAPTIVE:
> +		ret = regmap_field_write(data->regfield_int_thresh_var_en,
> +					 state);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}
> +
> +#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 get_device_id_lux_per_count(struct apds9306_data *data)
> +{
> +	int ret, part_id;
> +
> +	ret = regmap_read(data->regmap, APDS9306_PART_ID, &part_id);
> +	if (ret)
> +		return ret;
> +
> +	if (part_id == apds9306_part_id_nlux_per_count[0].part_id)
> +		data->nlux_per_count =
> +			apds9306_part_id_nlux_per_count[0].nlux_per_count;
> +	else if (part_id == apds9306_part_id_nlux_per_count[1].part_id)
> +		data->nlux_per_count =
> +			apds9306_part_id_nlux_per_count[1].nlux_per_count;

For loop over ARRAY_SIZE(apds9306_part_id_nlux_per_count)
would be more extensible with a return on match, so that if you
don't we just return -ENXIO on exit from the loop.


> +	else
> +		return -ENXIO;
> +
> +	return 0;
> +}
> +
> +static void apds9306_powerdown(void *ptr)
> +{
> +	struct apds9306_data *data = (struct apds9306_data *)ptr;
> +	struct device *dev = data->dev;
> +	int ret;
> +
> +	/* Disable interrupts */
> +	ret = regmap_field_write(data->regfield_int_thresh_var_en, 0);
> +	if (ret)
> +		dev_err(dev, "Failed to disable variance interrupts\n");

Muddling on when things are failing is probably not worthwhile. I'd be
tempted to just error out of here.  Worst that happens is we leave the
device partly enabled which is a bit of a power waste, but it's not expected
to happen so I don't think we care.  Much easier to follow code if we
always return on error.

> +	ret = regmap_field_write(data->regfield_int_en, 0);
> +	if (ret)
> +		dev_err(dev, "Failed to disable interrupts\n");
> +	/* Put the device in standby mode */
> +	ret = apds9306_power_state(data, STANDBY);
> +	if (ret)
> +		dev_err(dev, "Failed to power down device\n");
> +}

> +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, "vin");
> +	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_TRIGGER_FALLING | IRQF_ONESHOT,

The direction of the interrupt should come from device tree.  Sometimes people
use level conversion by using an not gate and that flips the logic of the
interrupt in a way that the driver can't see.  Hence we leave that
detail for firmware, not the driver.

> +					"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 = devm_iio_init_iio_gts(dev, APDS9306_SCALE_1X, 0, apds9306_gains,
> +				    ARRAY_SIZE(apds9306_gains), apds9306_itimes,
> +				    ARRAY_SIZE(apds9306_itimes), &data->gts);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(dev, apds9306_powerdown, data);

Why at this point? I'd have thought it wasn't powered up until init_device()
which follows?  So I'd expect to see this call after that, not before.

> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				"failed to add action on driver unwind\n");
> +
> +	ret = apds9306_init_device(data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to init device\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);
> +	int ret;
> +
> +	ret = apds9306_power_state(data, STANDBY);
> +	if (ret)
> +		regcache_cache_only(data->regmap, true);

What is the logic of putting the regcache into cache only mode
if we fail to power down the device?  

> +
> +	return ret;
> +}
> +
> +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);
> +	int ret;
> +
> +	regcache_cache_only(data->regmap, false);
> +	ret = regcache_sync(data->regmap);
> +	if (ret)
> +		return ret;
> +	ret = apds9306_power_state(data, ACTIVE);
> +	if (ret)
> +		regcache_cache_only(data->regmap, true);

If you get here an this failed we are in an unknown state where
the device is effectively dead anyway.  I'd not bother
with juggling the state of the regcache.  Or am I missing some path
in which this regcache_cache_only() is called that isn't
an error path?

> +
> +	return 0;
> +}
> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(apds9306_pm_ops,
> +				 apds9306_runtime_suspend,
> +				 apds9306_runtime_resume,
> +				 NULL);
> +
> +static const struct i2c_device_id apds9306_id[] = {
> +	{ "apds9306" }, { }

Put the terminator on a new line because it reduces the noise if we ever add
more devices by removing the need to reformat this first.

> +};
> +MODULE_DEVICE_TABLE(i2c, apds9306_id);
> +
> +static const struct of_device_id apds9306_of_match[] = {
> +	{ .compatible = "avago,apds9306" }, { }

Same as above.

> +};
> +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_new = apds9306_probe,
> +	.id_table = apds9306_id,
> +};
> +
> +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 2/2] iio: light: Add support for APDS9306 Light Sensor

[Subhajit Ghosh]

On 11/10/23 01:08, Jonathan Cameron wrote:
> 
> No need to wrap the patch description quite so short. Aim
> for up to 75 char for a commit message (and 80 for the code)
> Here you are under 60.
> 
Thank you for taking time to point out these small issues.

>>
>> Datasheet at https://docs.broadcom.com/doc/AV02-4755EN
>>
> There is a tag for datasheets in the format tags block so
> Datasheet: https://docs.broadcom.com/doc/AV02-4755EN
>> Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
> 
> I took a quick look at the most similar part number adps9300 and
> this does look substantially different but could you confirm you've
> taken a look at the plausible drivers to which support for this part
> could be added and perhaps mention why that doesn't make sense
> I think it will be mainly feature set being different here, but also
> it seems they have completely different register maps despite similar
> part numbers!
I have taken a look at quiet a few light sensor drivers including
apds9960 and apds9300, as you said that they are different. There are
another two drivers apds990x and apds9802als in drivers/misc which are
also very different but I can't say that I have been through all the
driver files.

>> +
>> +enum apds9306_int_channels {
>> +	CLEAR,
>> +	ALS,
>> +};
> 
> Is this used?
>
Something left from the old code. It is not needed.
  

>> + * Nano Lux per count = (340.134 * 1000000000)/ (32 * 3 * 2000) for apds9306
>> + * Nano Lux per count = (293.69 * 1000000000)/ (32 * 3 * 2000) for apds9306-065
> 
> Even though it's a comment stick to kernel maths syntax and put a space before the /
> Otherwise some script will complain it's not correctly formatted code :)
Ok, understood.

> 
>> + */
>> +static struct part_id_nlux_per_count apds9306_part_id_nlux_per_count[] = {
>> +	{.part_id = 0xB1, .nlux_per_count = 1787156},
>> +	{.part_id = 0xB3, .nlux_per_count = 1529635},
> 
> Prefer { .part_id = 0xB3, .nlux_per_count = 1629635 },
> for tables liek this as it ends up a tiny bit easier to read.
Ok.
> 
>> +};
>> +static struct iio_chan_spec apds9306_channels_without_events[] = {
>> +	{
>> +		APDS9306_CHANNEL(IIO_LIGHT)
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> +			BIT(IIO_CHAN_INFO_PROCESSED),
> 
> This needs an explanation for why as a comment in the code.
> We very rarely support both raw and processed for the same channel and
> mostly when we do it is due to some historical changes.
> 
Thanks for pointing it out.
> You are using the gts stuff here so it should be possible to expose
> the controls for scale necessary to have userspace perform the raw to processed
> conversion.
Yes, processed = (raw + offset) * scale. No need to do calculations in kernel
space. Ok. I will reimplement it.

>> +
>> +/* INT_PERSISTENCE available */
>> +IIO_CONST_ATTR(thresh_either_period_available, "[0 15]");
>> +
>> +/* ALS_THRESH_VAR available */
>> +IIO_CONST_ATTR(thresh_adaptive_either_values_available, "[0 7]");
> Not valid range syntax for IIO attributes, you need to include
> the step.
> 
> [0 1 7]
Got it.
> 

>> +	.cache_type = REGCACHE_RBTREE,
>> +	.disable_locking = true,
> This normally deserves a statement of what you are doing about locking instead.

I'll put it in the next version.

> The interrupt controller for starters takes to no locks and can run concurrently
> with other accesses from other CPUs.  That seems unwise.
>
Well, regarding device access, interrupt handler just reads the status registers
thereby clearing the interrupt status flag and releasing the physical interrupt line.
What can be the issue if I don't use a lock?


>> +static int apds9306_intg_time_get(struct apds9306_data *data, int *val2)
>> +{
>> +	*val2 = iio_gts_find_int_time_by_sel(&data->gts, data->intg_time_idx);
>> +	if (*val2 < 0)
>> +		return *val2;
> 
> You shouldn't have side effects on *val2 if an error occurs.
> Its not a bug in this case, but it is generally something to avoid
> 
Ok.

> 
>> +
>> +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;
>> +			break;
> Might as well return here instead of break as nothing else to do.
Ok.
> 
....
>> +		ret = IIO_VAL_INT;
>> +		*val2 = 0;
> 
> As below.  No need to set *val2 to 0 if returning IIO_VAL_INT.
Ok.

>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	*val2 = 0;
> 
> The IIO core won't use val2 if you return IIO_VAL_INT, so don't bother setting it.

Ok. Got it.
> 
>> +	return IIO_VAL_INT;
>> +}
>> +

>> +
>> +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 val, val2;
>> +	int ret;
>> +
>> +	mutex_lock(&data->mutex);
> As below
> 	guard(mutex)(&data->mutex);
> 
> should simplify this - I won't comment on this one above this point (reviewing backwards
> through the code).
Ok.
> 
>> +	switch (type) {
>> +	case IIO_EV_TYPE_THRESH:
>> +		ret = regmap_field_read(data->regfield_int_en, &val);
>> +		if (ret)
>> +			break;
>> +		ret = regmap_field_read(data->regfield_int_src, &val2);
>> +		if (ret)
>> +			break;
>> +		if (chan->type == IIO_LIGHT)
>> +			ret = val & val2;
>> +		else if (chan->type == IIO_INTENSITY)
>> +			ret = val & !val2;
> 
> This logic would benefit from better variable naming.
> en and src for example..
Sure.
> 
>> +		else
>> +			ret = -EINVAL;
>> +		break;
>> +	case IIO_EV_TYPE_THRESH_ADAPTIVE:
>> +		ret = regmap_field_read(data->regfield_int_thresh_var_en,
>> +					&val);
>> +		if (ret)
>> +			break;
>> +		ret = val;
>> +		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;
>> +
>> +	state = !!state;
>> +	mutex_lock(&data->mutex);
> 
> Perfect place to use the new cleanup.h trickery here.
:) Absolutely...
> 
> guard(mutex)(&data->mutex);
> 
> and then you can just return in error paths which will simplify this code
> 
Got your point.
>> +	switch (type) {
>> +	case IIO_EV_TYPE_THRESH:

>> +static int get_device_id_lux_per_count(struct apds9306_data *data)
>> +{
>> +	int ret, part_id;
>> +
>> +	ret = regmap_read(data->regmap, APDS9306_PART_ID, &part_id);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (part_id == apds9306_part_id_nlux_per_count[0].part_id)
>> +		data->nlux_per_count =
>> +			apds9306_part_id_nlux_per_count[0].nlux_per_count;
>> +	else if (part_id == apds9306_part_id_nlux_per_count[1].part_id)
>> +		data->nlux_per_count =
>> +			apds9306_part_id_nlux_per_count[1].nlux_per_count;
> 
> For loop over ARRAY_SIZE(apds9306_part_id_nlux_per_count)
> would be more extensible with a return on match, so that if you
> don't we just return -ENXIO on exit from the loop.
Yes. Got it.
> 
> 
>> +	else
>> +		return -ENXIO;
>> +
>> +	return 0;
>> +}
>> +
>> +static void apds9306_powerdown(void *ptr)
>> +{
>> +	struct apds9306_data *data = (struct apds9306_data *)ptr;
>> +	struct device *dev = data->dev;
>> +	int ret;
>> +
>> +	/* Disable interrupts */
>> +	ret = regmap_field_write(data->regfield_int_thresh_var_en, 0);
>> +	if (ret)
>> +		dev_err(dev, "Failed to disable variance interrupts\n");
> 
> Muddling on when things are failing is probably not worthwhile. I'd be
> tempted to just error out of here.  Worst that happens is we leave the
> device partly enabled which is a bit of a power waste, but it's not expected
> to happen so I don't think we care.  Much easier to follow code if we
> always return on error.
Ok, makes sense. I'll do that.
> 

>> +	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_TRIGGER_FALLING | IRQF_ONESHOT,
> 
> The direction of the interrupt should come from device tree.  Sometimes people
> use level conversion by using an not gate and that flips the logic of the
> interrupt in a way that the driver can't see.  Hence we leave that
> detail for firmware, not the driver.
Ok, understood.
> 
>> +	ret = devm_add_action_or_reset(dev, apds9306_powerdown, data);
> 
> Why at this point? I'd have thought it wasn't powered up until init_device()
> which follows?  So I'd expect to see this call after that, not before.
> 
Right. I will do a bit more reading on this before using this. I assumed this
functions registers the callback which gets called at driver release by the
subsystem similar to release().

>> +	if (ret)
>> +		return dev_err_probe(dev, ret,
>> +				"failed to add action on driver unwind\n");
>> +
>> +	ret = apds9306_init_device(data);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to init device\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);
>> +	int ret;
>> +
>> +	ret = apds9306_power_state(data, STANDBY);
>> +	if (ret)
>> +		regcache_cache_only(data->regmap, true);
> 
> What is the logic of putting the regcache into cache only mode
> if we fail to power down the device?
Yes, true. Real regs are available why use fake ones. I'll fix it.


>> +	ret = apds9306_power_state(data, ACTIVE);
>> +	if (ret)
>> +		regcache_cache_only(data->regmap, true);
> 
> If you get here an this failed we are in an unknown state where
> the device is effectively dead anyway.  I'd not bother
> with juggling the state of the regcache.  Or am I missing some path
> in which this regcache_cache_only() is called that isn't
> an error path?
> 
Yes, this is an error. I'll simply return error.

>> +
>> +static const struct i2c_device_id apds9306_id[] = {
>> +	{ "apds9306" }, { }
> 
> Put the terminator on a new line because it reduces the noise if we ever add
> more devices by removing the need to reformat this first.
> 
Ok.
>> +};
>> +MODULE_DEVICE_TABLE(i2c, apds9306_id);
>> +
>> +static const struct of_device_id apds9306_of_match[] = {
>> +	{ .compatible = "avago,apds9306" }, { }
> 
> Same as above.
> 
Ok.

Thank you Jonathan for the review. I'll get the changes done in the next version.

Regards,
Subhajit Ghosh

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

[Jonathan Cameron]

On Thu, 12 Oct 2023 01:07:10 +1030
Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:

> On 11/10/23 01:08, Jonathan Cameron wrote:
> > 
> > No need to wrap the patch description quite so short. Aim
> > for up to 75 char for a commit message (and 80 for the code)
> > Here you are under 60.
> >   
> Thank you for taking time to point out these small issues.
> 
> >>
> >> Datasheet at https://docs.broadcom.com/doc/AV02-4755EN
> >>  
> > There is a tag for datasheets in the format tags block so
> > Datasheet: https://docs.broadcom.com/doc/AV02-4755EN  
> >> Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>  
> > 
> > I took a quick look at the most similar part number adps9300 and
> > this does look substantially different but could you confirm you've
> > taken a look at the plausible drivers to which support for this part
> > could be added and perhaps mention why that doesn't make sense
> > I think it will be mainly feature set being different here, but also
> > it seems they have completely different register maps despite similar
> > part numbers!  
> I have taken a look at quiet a few light sensor drivers including
> apds9960 and apds9300, as you said that they are different. There are
> another two drivers apds990x and apds9802als in drivers/misc which are
> also very different but I can't say that I have been through all the
> driver files.
> 

Great.  Then as expected this separate driver make sense even if the
DT bindings can be combined.  Would be nice if they standardised
the interface, but some companies seem to feel the need to start from
scratch for each device they produce :(


> 
> > The interrupt controller for starters takes to no locks and can run concurrently
> > with other accesses from other CPUs.  That seems unwise.
> >  
> Well, regarding device access, interrupt handler just reads the status registers
> thereby clearing the interrupt status flag and releasing the physical interrupt line.
> What can be the issue if I don't use a lock?
Gah. I was far too sleepy that day.  Glad you interpreted my comment as intended :)

Hmm. You will be relying on the internal implementation of the regmap bus interface
resulting in locks being taken in the i2c controller.  That may be fine, but
it makes me a little nervous that it's relying on a particular implementation.

My normal assumption is that any driver that turns off locking in regmap is doing
so because it has various complex read modify write cycles so needs to have it's
own locks - but that it also applies those locks everywhere regmap would have
done (so for duration of every regmap call).

You may be fine, but you aren't meeting the requirements documented.
The flag to disable locking in regmap states:

 * @disable_locking: This regmap is either protected by external means or
 *                   is guaranteed not to be accessed from multiple threads.
 *                   Don't use any locking mechanisms.

It doesn't say you are fine for simple accesses and there are multiple threads
accessing 'the regmap'.

Unless you really care about it, I'd just leave regmap locking enabled.
The likely performance hit on a device on a slow bus is low and it avoids
us having to think too hard about this.


> >> +	ret = devm_add_action_or_reset(dev, apds9306_powerdown, data);  
> > 
> > Why at this point? I'd have thought it wasn't powered up until init_device()
> > which follows?  So I'd expect to see this call after that, not before.
> >   
> Right. I will do a bit more reading on this before using this. I assumed this
> functions registers the callback which gets called at driver release by the
> subsystem similar to release().

That's true, but with the addition that it is called in the reverse order of
being add to the devm managed release list.  So ordering matters.


> 
> Thank you Jonathan for the review. I'll get the changes done in the next version.
> 
No problem.   As a side note, feel free to just crop out any responses where
you agree with a review.  Default assumption is that if you don't comment that
is the case and it cuts down on scrolling when reviewer next looks.
They are also much more likely to take a look at a short reply than a long one!

Jonathan

> Regards,
> Subhajit Ghosh
> 
> 
> 
> 

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

[Subhajit Ghosh]

>> Thank you Jonathan for the review. I'll get the changes done in the next version.
>>
> No problem.   As a side note, feel free to just crop out any responses where
> you agree with a review.  Default assumption is that if you don't comment that
> is the case and it cuts down on scrolling when reviewer next looks.
> They are also much more likely to take a look at a short reply than a long one!
> 
> Jonathan
>
Understood. Thank you Jonathan.

Regards,
Subhajit Ghosh