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

[RFC 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
- 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 channel - als or clear
- Selection of interrupt mode - threshold or adaptive
- Level selection for adaptive threshold interrupts
- Persistence (Period) level selection for interrupts

Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>

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

 .../bindings/iio/light/avago,apds9306.yaml    |   47 +
 drivers/iio/light/Kconfig                     |   11 +
 drivers/iio/light/Makefile                    |    1 +
 drivers/iio/light/apds9306.c                  | 1146 +++++++++++++++++
 4 files changed, 1205 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds9306.yaml
 create mode 100644 drivers/iio/light/apds9306.c


base-commit: 0d3eb744aed40ffce820cded61d7eac515199165
-- 
2.34.1

[RFC PATCH 1/2] dt-bindings: Document APDS9306 Light Sensor bindings

[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    | 47 +++++++++++++++++++
 1 file changed, 47 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..f0547771bb1f
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/avago,apds9306.yaml
@@ -0,0 +1,47 @@
+# 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:
+  - |
+    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: [RFC PATCH 1/2] dt-bindings: Document APDS9306 Light Sensor bindings

[Rob Herring]

On Tue, 11 Apr 2023 09:12:02 +0800, 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    | 47 +++++++++++++++++++
>  1 file changed, 47 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds9306.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/iio/light/avago,apds9306.example.dts:29.33-34 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/iio/light/avago,apds9306.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1512: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230411011203.5013-2-subhajit.ghosh@tweaklogic.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [RFC PATCH 1/2] dt-bindings: Document APDS9306 Light Sensor bindings

[Subhajit Ghosh]

Hi Rob,
Thank you very much for reviewing the patch.

> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
> 
Thank you for the above steps, I did not perform them. It will
be fixed in the next revision.

Regards,
Subhajit Ghosh

Re: [RFC PATCH 1/2] dt-bindings: Document APDS9306 Light Sensor bindings

[Krzysztof Kozlowski]

On 11/04/2023 03:12, Subhajit Ghosh wrote:
> Add devicetree bindings for Avago APDS9306 Ambient Light Sensor.

Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching).

Subject: drop second/last, redundant "bindings". The "dt-bindings"
prefix is already stating that these are bindings.


Best regards,
Krzysztof

Re: [RFC PATCH 1/2] dt-bindings: Document APDS9306 Light Sensor bindings

[Subhajit Ghosh]

Hi Krzysztof,

Thank you for the feedback.
I will change the subject line.

Regards,
Subhajit Ghosh

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

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

Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>

---

Software reset feature is not implemented as it causes I2C bus error,
the I2C bus driver throws an ugly error message.

Could not locate the Lux calculations from datasheet, only exporting
raw values.

Reading of the Status register clears the Data Ready and the Interrupt
Status flags. It makes it tricky to read oneshot values together with
interrupts enabled as the IRQ handler clears the status on receipt
of an interrupt signal.
Not checking the status in IRQ handler will make the interrupt line
unsharable and it does not reset the interrupt line if the Interrupt
status flag is not cleared.

Many applications don't need interrupts. It is good to have support
for both in the driver.

Sysfs structure with interrupt line defined in DT:
root@stm32mp1:~# tree -I 'dev|name|of_node|power|subsystem|uevent' \
  /sys/bus/iio/devices/iio:device0/
/sys/bus/iio/devices/iio:device0/
|-- events
|   |-- thresh_adaptive_either_en
|   |-- thresh_adaptive_either_value
|   |-- thresh_adaptive_either_values_available
|   |-- thresh_channel_select
|   |-- thresh_channels_available
|   |-- thresh_either_en
|   |-- thresh_either_period
|   |-- thresh_either_period_available
|   |-- thresh_falling_value
|   `-- thresh_rising_value
|-- in_illuminance_raw
|-- in_intensity_clear_raw
|-- integration_time
|-- integration_time_available
|-- sampling_frequency
|-- sampling_frequency_available
|-- scale
`-- scale_available

Sysfs structure with no interrupt:
root@stm32mp1:~# tree -I 'dev|name|of_node|power|subsystem|uevent' \
  /sys/bus/iio/devices/iio:device0/
/sys/bus/iio/devices/iio:device0/
|-- in_illuminance_raw
|-- in_intensity_clear_raw
|-- integration_time
|-- integration_time_available
|-- sampling_frequency
|-- sampling_frequency_available
|-- scale
`-- scale_available

As there is a single interrupt enable option, using the existing
IIO bitmasks to cater for channel selection - als or clear and
interrupt type selection - threshold or adaptive
was looking ambiguous. I am open to any other implementations.

I could not find any _available implementations for the event
interface.

Created one sysfs attribute for interrupt channel selection
and one RO attribute for channel availability.

Created two RO attributes for interrupt period and threshold
adaptive value available ranges.

The ALS Measurement Rate (Sampling Frequency) has max values
of 2000ms and 2000ms for both 110b and 111b respectively.
This is not a typo. Found out experimentally that assigning
either values, interrupt is generated 2 secs apart when
interrupt conditions are met.

 drivers/iio/light/Kconfig    |   11 +
 drivers/iio/light/Makefile   |    1 +
 drivers/iio/light/apds9306.c | 1146 ++++++++++++++++++++++++++++++++++
 3 files changed, 1158 insertions(+)
 create mode 100644 drivers/iio/light/apds9306.c

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 0d4447df7200..8bd5aa13f49e 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -73,6 +73,17 @@ 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
+	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 d74d2b5ff14c..978ec4b3de4f 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..d57fb7aaa24d
--- /dev/null
+++ b/drivers/iio/light/apds9306.c
@@ -0,0 +1,1146 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * APDS-9306/APDS-9306-065 Ambient Light Sensor
+ *
+ * Datasheet: https://docs.broadcom.com/doc/AV02-4755EN
+ *
+ * I2C Address: 0x52
+ *
+ * Copyright (C) 2023 Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
+ */
+
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/delay.h>
+#include <linux/regmap.h>
+#include <linux/interrupt.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/events.h>
+#include <linux/regulator/consumer.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)
+
+enum apds9306_power_states {
+	standby,
+	active,
+};
+
+enum apds9306_int_channels {
+	clear,
+	als,
+};
+
+/* Integration Time (ALS Resolution) uSec */
+static const int apds9306_intg_time[][2] = {
+	{0, 400000},
+	{0, 200000},
+	{0, 100000},
+	{0, 50000},
+	{0, 25000},
+	{0, 3125},
+};
+
+/* 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},
+};
+
+/* Sampling Frequency in uSec */
+static const int apds9306_repeat_rate_period[] = {
+	25000, 50000, 100000, 200000, 500000, 1000000,
+	2000000
+};
+
+/* ALS Gain Range (Scale) */
+static const int apds9306_gain[] = {
+	1, 3, 6, 9, 18
+};
+
+struct apds9306_data {
+	struct i2c_client *client;
+	struct iio_dev *indio_dev;
+	/*
+	 * Protect single als reads and device
+	 * power states.
+	 */
+	struct mutex mutex;
+
+	struct regmap *regmap;
+	/* Reg: MAIN_CTRL, Field: SW_Reset */
+	struct regmap_field *regfield_sw_reset;
+	/* Reg: MAIN_CTRL, Field: ALS_EN */
+	struct regmap_field *regfield_en;
+	/* Reg: ALS_MEAS_RATE, Field: ALS Resolution/Bit Width */
+	struct regmap_field *regfield_intg_time;
+	/* Reg: ALS_MEAS_RATE, Field: ALS Measurement Rate */
+	struct regmap_field *regfield_repeat_rate;
+	/* Reg: ALS_GAIN, Field: ALS Gain Range */
+	struct regmap_field *regfield_scale;
+	/* Reg: INT_CFG, Field: ALS Interrupt Source */
+	struct regmap_field *regfield_int_src;
+	/* Reg: INT_CFG, Field: ALS Variation Interrupt Mode */
+	struct regmap_field *regfield_int_thresh_var_en;
+	/* Reg: INT_CFG, Field: ALS Interrupt Enable */
+	struct regmap_field *regfield_int_en;
+	/* Reg: INT_PERSISTENCE, Field: ALS_PERSIST */
+	struct regmap_field *regfield_int_persist_val;
+	/* Reg: ALS_THRESH_VAR, Field: ALS_THRES_VAR */
+	struct regmap_field *regfield_int_thresh_var_val;
+
+	u8 intg_time_idx;
+	u8 repeat_rate_idx;
+	u8 gain_idx;
+	u8 int_ch;
+};
+
+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,
+};
+
+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 struct iio_event_spec apds9306_event_spec[] = {
+	{
+		.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_shared_by_all = BIT(IIO_EV_INFO_ENABLE),
+	},
+};
+
+static struct iio_chan_spec apds9306_channels[] = {
+	{
+		.type = IIO_LIGHT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.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),
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 20,
+			.storagebits = 32,
+		},
+		.event_spec = apds9306_event_spec,
+		.num_event_specs = ARRAY_SIZE(apds9306_event_spec),
+	},
+	{
+		.type = IIO_INTENSITY,
+		.channel2 = IIO_MOD_LIGHT_CLEAR,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.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),
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 20,
+			.storagebits = 32,
+		},
+		.modified = 1,
+	},
+};
+
+static struct iio_chan_spec apds9306_channels_no_events[] = {
+	{
+		.type = IIO_LIGHT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.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),
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 20,
+			.storagebits = 32,
+		},
+	},
+	{
+		.type = IIO_INTENSITY,
+		.channel2 = IIO_MOD_LIGHT_CLEAR,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.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),
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 20,
+			.storagebits = 32,
+		},
+		.modified = 1,
+	},
+};
+
+static ssize_t thresh_channel_select_show(struct device *dev,
+		struct device_attribute *attr, char *buff)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct apds9306_data *data = iio_priv(indio_dev);
+	int ret;
+
+	if (data->int_ch == 1)
+		ret = sprintf(buff, "als\n");
+	else if (data->int_ch == 0)
+		ret = sprintf(buff, "clear\n");
+	else
+		ret = -EINVAL;
+
+	return ret;
+}
+
+static ssize_t thresh_channel_select_store(struct device *dev,
+		struct device_attribute *attr, const char *buff, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct apds9306_data *data = iio_priv(indio_dev);
+	int ret, channel;
+
+	if (len <= 0 || len > 6)
+		return -EINVAL;
+
+	if (!strncmp(buff, "als", 3))
+		channel = 1;
+	else if (!strncmp(buff, "clear", 5))
+		channel = 0;
+	else
+		return -EINVAL;
+
+	ret = regmap_field_write(data->regfield_int_src, channel);
+	if (ret)
+		return ret;
+
+	data->int_ch = channel;
+
+	return len;
+}
+
+/* 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]");
+
+/* Interrupt channel selection available */
+IIO_CONST_ATTR(thresh_channels_available, "clear als");
+IIO_DEVICE_ATTR_RW(thresh_channel_select, 0);
+
+static struct attribute *apds9306_event_attributes[] = {
+	&iio_const_attr_thresh_either_period_available.dev_attr.attr,
+	&iio_const_attr_thresh_channels_available.dev_attr.attr,
+	&iio_const_attr_thresh_adaptive_either_values_available.dev_attr.attr,
+	&iio_dev_attr_thresh_channel_select.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group apds9306_event_attr_group = {
+	.attrs = apds9306_event_attributes,
+};
+
+static int apds9306_regfield_init(struct apds9306_data *data)
+{
+	struct device *dev = &data->client->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);
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int apds9306_runtime_power(struct apds9306_data *data, int en)
+{
+	struct device *dev = &data->client->dev;
+	int ret = 0;
+
+	if (en) {
+		ret = pm_runtime_resume_and_get(dev);
+		if (ret < 0) {
+			dev_err(dev, "runtime resume failed: %d\n", ret);
+			return ret;
+		}
+		ret = 0;
+	} else {
+		pm_runtime_mark_last_busy(dev);
+		pm_runtime_put_autosuspend(dev);
+	}
+
+	return ret;
+}
+
+static int apds9306_read_data(struct apds9306_data *data, int *val, int reg)
+{
+	struct device *dev = &data->client->dev;
+	int ret, delay, status, int_en;
+	int retries = 4;
+	u8 buff[3];
+
+	ret = apds9306_runtime_power(data, 1);
+	if (ret)
+		return ret;
+
+	/*
+	 * Whichever is greater - integration time period or
+	 * sampling period.
+	 */
+	delay = max(apds9306_intg_time[data->intg_time_idx][1],
+			apds9306_repeat_rate_period[data->repeat_rate_idx]);
+
+	/*
+	 * If interrupts are enabled then Status resistor cannot be
+	 * relied upon as all the status bits are cleared by the
+	 * interrupt handler in case of an event.
+	 */
+	ret = regmap_field_read(data->regfield_int_en, &int_en);
+	if (ret) {
+		dev_err(dev, "read interrupt status failed: %d\n", ret);
+		return ret;
+	}
+
+	if (!int_en) {
+		while (retries--) {
+			ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS,
+					&status);
+			if (ret) {
+				dev_err(dev, "read status failed: %d\n", ret);
+				return ret;
+			}
+			if (status & APDS9306_ALS_DATA_STAT_MASK)
+				break;
+			/*
+			 * In case of continuous one-shot read from userspace,
+			 * new data is available after sampling period.
+			 * Delays are in the range of 25ms to 2secs.
+			 */
+			fsleep(delay);
+		}
+	} else
+		fsleep(delay);
+
+	if (!retries)
+		return -EBUSY;
+
+	ret = regmap_bulk_read(data->regmap, reg, buff, sizeof(buff));
+	if (ret) {
+		dev_err(&data->client->dev, "read data failed\n");
+		return ret;
+	}
+
+	*val = get_unaligned_le24(&buff[0]);
+
+	ret = apds9306_runtime_power(data, 0);
+	if (ret)
+		return ret;
+
+	return ret;
+}
+
+static int apds9306_intg_time_get(struct apds9306_data *data, int *val,
+		int *val2)
+{
+	if (data->intg_time_idx > ARRAY_SIZE(apds9306_intg_time))
+		return -EINVAL;
+
+	*val = apds9306_intg_time[data->intg_time_idx][0];
+	*val2 = apds9306_intg_time[data->intg_time_idx][1];
+
+	return 0;
+}
+
+static int apds9306_intg_time_set(struct apds9306_data *data, int val,
+		int val2)
+{
+	int i, ret = -EINVAL;
+
+	for (i = 0; i < ARRAY_SIZE(apds9306_intg_time); i++)
+		if (apds9306_intg_time[i][0] ==  val &&
+				apds9306_intg_time[i][1] ==  val2) {
+			ret = regmap_field_write(data->regfield_intg_time, i);
+			if (ret)
+				return ret;
+			data->intg_time_idx = i;
+			break;
+		}
+
+	return 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_gain_get(struct apds9306_data *data, int *val)
+{
+	if (data->gain_idx > ARRAY_SIZE(apds9306_gain))
+		return -EINVAL;
+
+	*val = apds9306_gain[data->gain_idx];
+
+	return 0;
+}
+
+static int apds9306_gain_set(struct apds9306_data *data, int val)
+{
+	int i, ret;
+
+	for (i = 0; i < ARRAY_SIZE(apds9306_gain); i++)
+		if (apds9306_gain[i] ==  val) {
+			ret = regmap_field_write(data->regfield_scale, i);
+			if (ret)
+				return ret;
+			data->gain_idx = i;
+		}
+
+	return 0;
+}
+
+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 > 15)
+		return -EINVAL;
+
+	*val = period;
+
+	return ret;
+}
+
+static int apds9306_event_period_set(struct apds9306_data *data, int val)
+{
+	if (val < 0 || val > 15)
+		return -EINVAL;
+
+	return regmap_field_write(data->regfield_int_persist_val, val);
+}
+
+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 > 7)
+		return -EINVAL;
+
+	*val = thad;
+
+	return ret;
+}
+
+static int apds9306_event_thresh_adaptive_set(struct apds9306_data *data,
+		int val)
+{
+	if (val < 0 || val > 7)
+		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)
+			ret = IIO_VAL_INT;
+		*val2 = 0;
+		break;
+	case IIO_CHAN_INFO_INT_TIME:
+		ret = apds9306_intg_time_get(data, val, val2);
+		if (!ret)
+			ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = apds9306_sampling_freq_get(data, val, val2);
+		if (!ret)
+			ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+	case IIO_CHAN_INFO_SCALE:
+		ret = apds9306_gain_get(data, val);
+		if (!ret)
+			ret = IIO_VAL_INT;
+		*val2 = 0;
+		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)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		*length = ARRAY_SIZE(apds9306_intg_time) * 2;
+		*vals = (const int *)apds9306_intg_time;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+	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;
+	case IIO_CHAN_INFO_SCALE:
+		*length = ARRAY_SIZE(apds9306_gain);
+		*vals = apds9306_gain;
+		*type = IIO_VAL_INT;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+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 = -EINVAL;
+
+	mutex_lock(&data->mutex);
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		ret = apds9306_intg_time_set(data, val, val2);
+		break;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = apds9306_sampling_freq_set(data, val, val2);
+		break;
+	case IIO_CHAN_INFO_SCALE:
+		if (val2 != 0)
+			break;
+		ret = apds9306_gain_set(data, val);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static const struct iio_info apds9306_info_no_events = {
+		.read_raw = apds9306_read_raw,
+		.read_avail = apds9306_read_avail,
+		.write_raw = apds9306_write_raw,
+};
+
+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
+	 */
+	ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS, &status);
+	if (ret < 0) {
+		dev_err(&data->client->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));
+	}
+
+	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);
+	u8 buff[3];
+	int var, ret;
+
+	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);
+			if (ret)
+				return ret;
+			break;
+		}
+
+		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[0]);
+	break;
+	case IIO_EV_TYPE_THRESH_ADAPTIVE:
+		ret = apds9306_event_thresh_adaptive_get(data, val);
+		if (ret)
+			return ret;
+	break;
+	default:
+		return -EINVAL;
+	}
+
+	*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);
+	unsigned int var;
+	u8 buff[3];
+
+	switch (type) {
+	case IIO_EV_TYPE_THRESH:
+		if (dir == IIO_EV_DIR_EITHER && info == IIO_EV_INFO_PERIOD)
+			return apds9306_event_period_set(data, val);
+		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 > 0xFFFFF)
+			return -EINVAL;
+
+		put_unaligned_le24(val, buff);
+		return regmap_bulk_write(data->regmap, var, buff, sizeof(buff));
+	case IIO_EV_TYPE_THRESH_ADAPTIVE:
+		return apds9306_event_thresh_adaptive_set(data, val);
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+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;
+	int ret;
+
+	switch (type) {
+	case IIO_EV_TYPE_THRESH:
+		ret = regmap_field_read(data->regfield_int_en, &val);
+		if (ret)
+			return ret;
+		return val;
+	case IIO_EV_TYPE_THRESH_ADAPTIVE:
+		ret = regmap_field_read(data->regfield_int_thresh_var_en, &val);
+		if (ret)
+			return ret;
+		return val;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+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, curr_state;
+
+	state = !!state;
+	switch (type) {
+	case IIO_EV_TYPE_THRESH:
+		ret = regmap_field_read(data->regfield_int_en, &curr_state);
+		if (ret)
+			return ret;
+		if (curr_state == state)
+			return 0;
+		ret = regmap_field_write(data->regfield_int_en, state);
+		if (ret)
+			return ret;
+		mutex_lock(&data->mutex);
+		ret = apds9306_runtime_power(data, state);
+		mutex_unlock(&data->mutex);
+		if (ret)
+			return ret;
+		break;
+	case IIO_EV_TYPE_THRESH_ADAPTIVE:
+		ret = regmap_field_write(data->regfield_int_thresh_var_en, state);
+		if (ret)
+			return ret;
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+static const struct iio_info apds9306_info = {
+		.event_attrs = &apds9306_event_attr_group,
+		.read_raw = apds9306_read_raw,
+		.read_avail = apds9306_read_avail,
+		.write_raw = apds9306_write_raw,
+		.read_event_value = apds9306_read_event,
+		.write_event_value = apds9306_write_event,
+		.read_event_config = apds9306_read_event_config,
+		.write_event_config = apds9306_write_event_config,
+};
+
+static void apds9306_powerdown(void *ptr)
+{
+	struct apds9306_data *data = (struct apds9306_data *)ptr;
+	int status;
+
+	/* Disable interrupts */
+	regmap_field_write(data->regfield_int_thresh_var_en, 0);
+	regmap_field_write(data->regfield_int_en, 0);
+	/* Clear status */
+	regmap_read(data->regmap, APDS9306_MAIN_STATUS, &status);
+	/* Put the device in standby mode */
+	apds9306_power_state(data, standby);
+}
+
+static int apds9306_init_device(struct apds9306_data *data)
+{
+	struct device *dev = &data->client->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;
+
+	/* Integration time: 100 msec */
+	ret = apds9306_intg_time_set(data, 0, 100000);
+	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(data, 3);
+	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 */
+	ret = regmap_field_write(data->regfield_int_thresh_var_en, 0);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int apds9306_probe(struct i2c_client *client)
+{
+	struct apds9306_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	data->indio_dev = indio_dev;
+
+	mutex_init(&data->mutex);
+
+	data->regmap = devm_regmap_init_i2c(client, &apds9306_regmap);
+	if (IS_ERR(data->regmap))
+		return dev_err_probe(&client->dev, PTR_ERR(data->regmap),
+					"regmap initialization failed\n");
+
+	i2c_set_clientdata(client, indio_dev);
+	data->client = client;
+
+	ret = apds9306_regfield_init(data);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+					"regfield initialization failed\n");
+
+	ret = devm_regulator_get_enable(&client->dev, "vin");
+	if (ret)
+		return dev_err_probe(&client->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;
+		indio_dev->num_channels = ARRAY_SIZE(apds9306_channels);
+		ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
+				apds9306_irq_handler, IRQF_TRIGGER_FALLING |
+				IRQF_ONESHOT, "apds9306_event", indio_dev);
+		if (ret)
+			return dev_err_probe(&client->dev, ret,
+					"failed to assign interrupt.\n");
+	} else {
+		indio_dev->info = &apds9306_info_no_events;
+		indio_dev->channels = apds9306_channels_no_events;
+		indio_dev->num_channels = ARRAY_SIZE(apds9306_channels_no_events);
+	}
+
+	ret = devm_add_action_or_reset(&client->dev, apds9306_powerdown, data);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				"failed to add action on driver unwind\n");
+
+	ret = apds9306_init_device(data);
+	if (ret)
+		return dev_err_probe(&client->dev, ret, "failed to init device\n");
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+static int apds9306_runtime_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(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 = i2c_get_clientdata(to_i2c_client(dev));
+	struct apds9306_data *data = iio_priv(indio_dev);
+	int ret;
+
+	regcache_cache_only(data->regmap, false);
+	ret = regcache_sync(data->regmap);
+	if (!ret) {
+		ret = apds9306_power_state(data, active);
+		if (ret)
+			regcache_cache_only(data->regmap, true);
+	}
+
+	return ret;
+}
+
+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_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");
-- 
2.34.1

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

[Andy Shevchenko]

On Tue, Apr 11, 2023 at 09:12:03AM +0800, 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.

...

> Datasheet at https://docs.broadcom.com/doc/AV02-4755EN
> 
> Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>

Drop a blank line and make Datasheet: to be a tag.
Datasheet: https://docs.broadcom.com/doc/AV02-4755EN

...

> +/*
> + * APDS-9306/APDS-9306-065 Ambient Light Sensor
> + *
> + * Datasheet: https://docs.broadcom.com/doc/AV02-4755EN
> + *
> + * I2C Address: 0x52

I guess this can be reordered and condensed a bit:

 * 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/pm.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/delay.h>
> +#include <linux/regmap.h>
> +#include <linux/interrupt.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/events.h>
> +#include <linux/regulator/consumer.h>

Sorted?

+ Blank line.

> +#include <asm/unaligned.h>

...

> +enum apds9306_power_states {
> +	standby,
> +	active,

Namespace? Capital letters?

> +};
> +
> +enum apds9306_int_channels {
> +	clear,
> +	als,

Ditto.

> +};

...

> +/* Sampling Frequency in uSec */
> +static const int apds9306_repeat_rate_period[] = {
> +	25000, 50000, 100000, 200000, 500000, 1000000,
> +	2000000

Can be on a single line.

> +};

...

Perhaps you want to add a few static_asserts() to be sure that the ARRAY_SIZE()
of the tables are all greater or equal to each others.

...

> +struct apds9306_data {
> +	struct i2c_client *client;
> +	struct iio_dev *indio_dev;
> +	/*
> +	 * Protect single als reads and device
> +	 * power states.
> +	 */
> +	struct mutex mutex;
> +
> +	struct regmap *regmap;
> +	/* Reg: MAIN_CTRL, Field: SW_Reset */
> +	struct regmap_field *regfield_sw_reset;
> +	/* Reg: MAIN_CTRL, Field: ALS_EN */
> +	struct regmap_field *regfield_en;
> +	/* Reg: ALS_MEAS_RATE, Field: ALS Resolution/Bit Width */

Why not converting all comments to a kernel-doc for the entire structure?

> +	struct regmap_field *regfield_intg_time;
> +	/* Reg: ALS_MEAS_RATE, Field: ALS Measurement Rate */
> +	struct regmap_field *regfield_repeat_rate;
> +	/* Reg: ALS_GAIN, Field: ALS Gain Range */
> +	struct regmap_field *regfield_scale;
> +	/* Reg: INT_CFG, Field: ALS Interrupt Source */
> +	struct regmap_field *regfield_int_src;
> +	/* Reg: INT_CFG, Field: ALS Variation Interrupt Mode */
> +	struct regmap_field *regfield_int_thresh_var_en;
> +	/* Reg: INT_CFG, Field: ALS Interrupt Enable */
> +	struct regmap_field *regfield_int_en;
> +	/* Reg: INT_PERSISTENCE, Field: ALS_PERSIST */
> +	struct regmap_field *regfield_int_persist_val;
> +	/* Reg: ALS_THRESH_VAR, Field: ALS_THRES_VAR */
> +	struct regmap_field *regfield_int_thresh_var_val;
> +
> +	u8 intg_time_idx;
> +	u8 repeat_rate_idx;
> +	u8 gain_idx;
> +	u8 int_ch;
> +};

...

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

Do you need an internal regmap lock? If so, why?

> +};

...

> +static ssize_t thresh_channel_select_show(struct device *dev,
> +		struct device_attribute *attr, char *buff)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct apds9306_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (data->int_ch == 1)
> +		ret = sprintf(buff, "als\n");

Must be sysfs_emit().

> +	else if (data->int_ch == 0)
> +		ret = sprintf(buff, "clear\n");

Must be sysfs_emit().

> +	else
> +		ret = -EINVAL;
> +
> +	return ret;

Make the string literal array for these and...

> +}
> +
> +static ssize_t thresh_channel_select_store(struct device *dev,
> +		struct device_attribute *attr, const char *buff, size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct apds9306_data *data = iio_priv(indio_dev);
> +	int ret, channel;
> +
> +	if (len <= 0 || len > 6)
> +		return -EINVAL;
> +
> +	if (!strncmp(buff, "als", 3))
> +		channel = 1;
> +	else if (!strncmp(buff, "clear", 5))
> +		channel = 0;
> +	else
> +		return -EINVAL;

...use sysfs_match_string().

> +
> +	ret = regmap_field_write(data->regfield_int_src, channel);
> +	if (ret)
> +		return ret;
> +
> +	data->int_ch = channel;
> +
> +	return len;
> +}

...

> +static struct attribute *apds9306_event_attributes[] = {
> +	&iio_const_attr_thresh_either_period_available.dev_attr.attr,
> +	&iio_const_attr_thresh_channels_available.dev_attr.attr,
> +	&iio_const_attr_thresh_adaptive_either_values_available.dev_attr.attr,
> +	&iio_dev_attr_thresh_channel_select.dev_attr.attr,
> +	NULL,

No comma for a terminator entry.

> +};

...

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

> +	return 0;

Move that to a single user of this line inside the switch-case.

> +}

...

> +	struct device *dev = &data->client->dev;

Why data contains I²C client pointer, what for?

...

> +	int ret = 0;

Unneeded assignment...

> +	if (en) {
> +		ret = pm_runtime_resume_and_get(dev);
> +		if (ret < 0) {
> +			dev_err(dev, "runtime resume failed: %d\n", ret);
> +			return ret;
> +		}
> +		ret = 0;
> +	} else {
> +		pm_runtime_mark_last_busy(dev);
> +		pm_runtime_put_autosuspend(dev);
> +	}
> +
> +	return ret;

...just return 0 here.

...

> +		while (retries--) {
> +			ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS,
> +					&status);
> +			if (ret) {
> +				dev_err(dev, "read status failed: %d\n", ret);
> +				return ret;
> +			}
> +			if (status & APDS9306_ALS_DATA_STAT_MASK)
> +				break;
> +			/*
> +			 * In case of continuous one-shot read from userspace,
> +			 * new data is available after sampling period.
> +			 * Delays are in the range of 25ms to 2secs.
> +			 */
> +			fsleep(delay);
> +		}

regmap_read_poll_timeout().

...

> +	*val = get_unaligned_le24(&buff[0]);

buff is enough, but if you want to be too explicit...

...

> +	ret = apds9306_runtime_power(data, 0);
> +	if (ret)
> +		return ret;
> +
> +	return ret;

	return apds...(...);

...

> +		if (apds9306_intg_time[i][0] ==  val &&
> +				apds9306_intg_time[i][1] ==  val2) {

Too many spaces and wrong indentation.

...

> +		if (apds9306_repeat_rate_freq[i][0] ==  val &&
> +				apds9306_repeat_rate_freq[i][1] ==  val2) {

Ditto.

...

> +		if (apds9306_gain[i] ==  val) {

Too many spaces.

...

> +	if (thad > 7)
> +		return -EINVAL;

This 7 should be defined with a meaningful name.

...

> +	if (val < 0 || val > 7)
> +		return -EINVAL;

Ditto.

...

> +	struct apds9306_data *data = iio_priv(indio_dev);
> +	int ret = -EINVAL;

This assignment is used only once, so make it explicit in that user below.

> +	mutex_lock(&data->mutex);
> +	switch (mask) {
> +	case IIO_CHAN_INFO_INT_TIME:
> +		ret = apds9306_intg_time_set(data, val, val2);
> +		break;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = apds9306_sampling_freq_set(data, val, val2);
> +		break;
> +	case IIO_CHAN_INFO_SCALE:
> +		if (val2 != 0)
> +			break;
> +		ret = apds9306_gain_set(data, val);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}

...

> +	/* The interrupt line is released and the interrupt flag is
> +	 * cleared as a result of reading the status register
> +	 */

/*
 * Style of the multi-line comment
 * is wrong.
 */

...

> +	break;
> +	case IIO_EV_TYPE_THRESH_ADAPTIVE:
> +		ret = apds9306_event_thresh_adaptive_get(data, val);
> +		if (ret)
> +			return ret;

> +	break;

Wrong indentation of this in entire switch-case. Why the style is different
from piece of code to piece of code?

...

> +		if (val < 0 || val > 0xFFFFF)
> +			return -EINVAL;

Definition and use some plain decimal number if it's about the upper limit of
the respective non-bitwise value.

> +		put_unaligned_le24(val, buff);
> +		return regmap_bulk_write(data->regmap, var, buff, sizeof(buff));
> +	case IIO_EV_TYPE_THRESH_ADAPTIVE:
> +		return apds9306_event_thresh_adaptive_set(data, val);
> +	default:
> +		return -EINVAL;
> +	}

> +	return 0;

Is it dead code?

> +}

...

> +	case IIO_EV_TYPE_THRESH:
> +		ret = regmap_field_read(data->regfield_int_en, &val);
> +		if (ret)
> +			return ret;
> +		return val;
> +	case IIO_EV_TYPE_THRESH_ADAPTIVE:
> +		ret = regmap_field_read(data->regfield_int_thresh_var_en, &val);
> +		if (ret)
> +			return ret;
> +		return val;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;

Ditto.

...

> +	switch (type) {
> +	case IIO_EV_TYPE_THRESH:
> +		ret = regmap_field_read(data->regfield_int_en, &curr_state);
> +		if (ret)
> +			return ret;
> +		if (curr_state == state)
> +			return 0;
> +		ret = regmap_field_write(data->regfield_int_en, state);
> +		if (ret)
> +			return ret;
> +		mutex_lock(&data->mutex);
> +		ret = apds9306_runtime_power(data, state);
> +		mutex_unlock(&data->mutex);
> +		if (ret)
> +			return ret;
> +		break;
> +	case IIO_EV_TYPE_THRESH_ADAPTIVE:
> +		ret = regmap_field_write(data->regfield_int_thresh_var_en, state);
> +		if (ret)
> +			return ret;
> +		break;
> +	default:
> +		ret = -EINVAL;

Again, use the same style, here you have no lock, so you may return directly.
No need to have this.

> +	}
> +
> +	return ret;

Why ret?

...

> +	regmap_field_write(data->regfield_int_en, 0);
> +	/* Clear status */
> +	regmap_read(data->regmap, APDS9306_MAIN_STATUS, &status);
> +	/* Put the device in standby mode */
> +	apds9306_power_state(data, standby);


No error checks at all?

...

> +		ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +				apds9306_irq_handler, IRQF_TRIGGER_FALLING |
> +				IRQF_ONESHOT, "apds9306_event", indio_dev);

Re-indent them to have logical split on the lines.

> +		if (ret)
> +			return dev_err_probe(&client->dev, ret,
> +					"failed to assign interrupt.\n");

...

> +static int apds9306_runtime_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));

WHy? Use dev_get_drvdata() directly.

> +	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 = i2c_get_clientdata(to_i2c_client(dev));

Ditto.

> +	struct apds9306_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	regcache_cache_only(data->regmap, false);
> +	ret = regcache_sync(data->regmap);

> +	if (!ret) {

	if (ret)
		return ret;

> +		ret = apds9306_power_state(data, active);
> +		if (ret)
> +			regcache_cache_only(data->regmap, true);
> +	}
> +
> +	return ret;
> +}

...

> +static DEFINE_RUNTIME_DEV_PM_OPS(apds9306_pm_ops, apds9306_runtime_suspend,
> +				apds9306_runtime_resume, NULL);

Do a logical split between lines.

static DEFINE_RUNTIME_DEV_PM_OPS(apds9306_pm_ops,
				 apds9306_runtime_suspend, apds9306_runtime_resume, NULL);

Alternatively

static DEFINE_RUNTIME_DEV_PM_OPS(apds9306_pm_ops,
				 apds9306_runtime_suspend,
				 apds9306_runtime_resume,
				 NULL);

...

> +static struct i2c_driver apds9306_driver = {
> +	.driver = {
> +		.name = "apds9306",
> +		.pm = pm_ptr(&apds9306_pm_ops),
> +		.of_match_table = apds9306_of_match,
> +	},
> +	.probe_new = apds9306_probe,
> +	.id_table = apds9306_id,
> +};

> +

Redundant blank line.

> +module_i2c_driver(apds9306_driver);

-- 
With Best Regards,
Andy Shevchenko

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

[Subhajit Ghosh]

Hi Andy,
Thank you so much for the detailed review.
Appreciate you taking time to do this.
It would be helpful if you could answers few questions inline?

> 
>> +/*
>> + * APDS-9306/APDS-9306-065 Ambient Light Sensor
>> + *
>> + * Datasheet: https://docs.broadcom.com/doc/AV02-4755EN
>> + *
>> + * I2C Address: 0x52
> 
> I guess this can be reordered and condensed a bit:
> 
>   * 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>
>> + */
> 
Understood. It will be fixed.

>> +#include <linux/iio/events.h>
>> +#include <linux/regulator/consumer.h>
> 
> Sorted?
> 
> + Blank line.
> 
>> +#include <asm/unaligned.h>
> 
Understood. It will be fixed.

> Namespace? Capital letters?
> 
>> +};
>> +
>> +enum apds9306_int_channels {
>> +	clear,
>> +	als,
> 
> Ditto.
> 
>> +};
> 
Understood. It will be fixed.

> 
>> +/* Sampling Frequency in uSec */
>> +static const int apds9306_repeat_rate_period[] = {
>> +	25000, 50000, 100000, 200000, 500000, 1000000,
>> +	2000000
> 
> Can be on a single line.
> 
>> +};
> 
> ...
> 
> Perhaps you want to add a few static_asserts() to be sure that the ARRAY_SIZE()
> of the tables are all greater or equal to each others.
> 
> ...
Ok. I'll do that.

> 
> Why not converting all comments to a kernel-doc for the entire structure?
>
Yes. Sure.
  
> 
> ...
> 
>> +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,
> 
> Do you need an internal regmap lock? If so, why?
For event interface - interrupt enable, adaptive interrupt enable,
upper and lower threshold values, selection of clear or als
channels for interrupt, the mutex in the driver's private data structure
is not used.
I thought to use the regmap's internal locking mechanism for
mutual exclusion as the values are directly written to or read from
the device registers form the write_event(), read_event(),
write_event_config() and read_event_config().
What do you think?
> 
>> +	else if (data->int_ch == 0)
>> +		ret = sprintf(buff, "clear\n");
> 
> Must be sysfs_emit().
Sure. I'll use that.
> 
>> +	else
>> +		ret = -EINVAL;
>> +
>> +	return ret;
> 
> Make the string literal array for these and...
> ...
>> +		channel = 1;
>> +	else if (!strncmp(buff, "clear", 5))
>> +		channel = 0;
>> +	else
>> +		return -EINVAL;
> 
> ...use sysfs_match_string().
Understood. It will be done.

>> +	NULL,
> 
> No comma for a terminator entry.
> 
>> +};
> 
Sorry. Mistake. It will be fixed.
> 
>> +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);
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
> 
>> +	return 0;
> 
> Move that to a single user of this line inside the switch-case.
Sorry, I did not get you. Can you please elaborate?

>> +	struct device *dev = &data->client->dev;
> 
> Why data contains I²C client pointer, what for?
I copied the implementation. It will be re-implemented.
> 
> ...
> 
>> +	int ret = 0;
> 
> Unneeded assignment...
Yes. It will be fixed.
> 
>> +	if (en) {
>> +		ret = pm_runtime_resume_and_get(dev);
>> +		if (ret < 0) {
>> +			dev_err(dev, "runtime resume failed: %d\n", ret);
>> +			return ret;
>> +		}
>> +		ret = 0;
>> +	} else {
>> +		pm_runtime_mark_last_busy(dev);
>> +		pm_runtime_put_autosuspend(dev);
>> +	}
>> +
>> +	return ret;
> 
> ...just return 0 here.
Ok.
> 
> ...
> 
>> +		while (retries--) {
>> +			ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS,
>> +					&status);
>> +			if (ret) {
>> +				dev_err(dev, "read status failed: %d\n", ret);
>> +				return ret;
>> +			}
>> +			if (status & APDS9306_ALS_DATA_STAT_MASK)
>> +				break;
>> +			/*
>> +			 * In case of continuous one-shot read from userspace,
>> +			 * new data is available after sampling period.
>> +			 * Delays are in the range of 25ms to 2secs.
>> +			 */
>> +			fsleep(delay);
>> +		}
> 
> regmap_read_poll_timeout().
According to the regmap_read_poll_timeout() documentation, the maximum time
to sleep between reads should be less than ~20ms as it uses usleep_range().

If userspace is doing continuous reads, then data is available after sampling
period (25ms to 2sec) or integration time (3.125ms to 400ms) whichever is
greater.

The runtime_suspend() function is called after 5 seconds, so the device is
still active and running.

If the ALS data bit is not set in status reg, it is efficient to sleep for
one sampling period rather than continuously checking the status reg
within ~20ms if we use regmap_read_poll_timeout().

Do you have any suggestions?

> 
> ...
> 
>> +	*val = get_unaligned_le24(&buff[0]);
> 
> buff is enough, but if you want to be too explicit...
Ok. It will be done.
> 
> ...
> 
>> +	ret = apds9306_runtime_power(data, 0);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return ret;
> 
> 	return apds...(...);
Oh yes.
> 
> ...
> 
>> +		if (apds9306_intg_time[i][0] ==  val &&
>> +				apds9306_intg_time[i][1] ==  val2) {
> 
> Too many spaces and wrong indentation.
> 
> ...
> 
>> +		if (apds9306_repeat_rate_freq[i][0] ==  val &&
>> +				apds9306_repeat_rate_freq[i][1] ==  val2) {
> 
> Ditto.
> 
> ...
> 
>> +		if (apds9306_gain[i] ==  val) {
> 
> Too many spaces.
Thank you. It will be fixed.
> 
> ...
> 
>> +	if (thad > 7)
>> +		return -EINVAL;
> 
> This 7 should be defined with a meaningful name.
> 
> ...
> 
>> +	if (val < 0 || val > 7)
>> +		return -EINVAL;
> 
> Ditto.
>
Understood.
> ...
> 
>> +	struct apds9306_data *data = iio_priv(indio_dev);
>> +	int ret = -EINVAL;
> 
> This assignment is used only once, so make it explicit in that user below.
>
Got it.

> 
> ...
> 
>> +	/* The interrupt line is released and the interrupt flag is
>> +	 * cleared as a result of reading the status register
>> +	 */
> 
> /*
>   * Style of the multi-line comment
>   * is wrong.
>   */
> 
Thank you for this. I've been scratching my head for some time on the
style of commenting.
> ...
> 
>> +	break;
>> +	case IIO_EV_TYPE_THRESH_ADAPTIVE:
>> +		ret = apds9306_event_thresh_adaptive_get(data, val);
>> +		if (ret)
>> +			return ret;
> 
>> +	break;
> 
> Wrong indentation of this in entire switch-case. Why the style is different
> from piece of code to piece of code?
> 
> ...
> 
>> +		if (val < 0 || val > 0xFFFFF)
>> +			return -EINVAL;
> 
> Definition and use some plain decimal number if it's about the upper limit of
> the respective non-bitwise value.
Ok, it will be done
> 
>> +	default:
>> +		return -EINVAL;
>> +	}
> 
>> +	return 0;
> 
> Is it dead code?
Yes, seems to be. It will be fixed.

>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return 0;
> 
> Ditto.
Yes, seems to be. It will be fixed.
> 
> ...
> 
>> +	switch (type) {
>> +	case IIO_EV_TYPE_THRESH:
>> +		ret = regmap_field_read(data->regfield_int_en, &curr_state);
>> +		if (ret)
>> +			return ret;
>> +		if (curr_state == state)
>> +			return 0;
>> +		ret = regmap_field_write(data->regfield_int_en, state);
>> +		if (ret)
>> +			return ret;
>> +		mutex_lock(&data->mutex);
>> +		ret = apds9306_runtime_power(data, state);
>> +		mutex_unlock(&data->mutex);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	case IIO_EV_TYPE_THRESH_ADAPTIVE:
>> +		ret = regmap_field_write(data->regfield_int_thresh_var_en, state);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
> 
> Again, use the same style, here you have no lock, so you may return directly.
> No need to have this.
> 
>> +	}
>> +
>> +	return ret;
> 
> Why ret?
Once, it is clear whether regmap's internal locking should be used or not,
this function will be re-implemented.
> 
> ...
> 
>> +	regmap_field_write(data->regfield_int_en, 0);
>> +	/* Clear status */
>> +	regmap_read(data->regmap, APDS9306_MAIN_STATUS, &status);
>> +	/* Put the device in standby mode */
>> +	apds9306_power_state(data, standby);
> 
> 
> No error checks at all?
Missed it. I'll do that.
> 
> ...
> 
>> +		ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
>> +				apds9306_irq_handler, IRQF_TRIGGER_FALLING |
>> +				IRQF_ONESHOT, "apds9306_event", indio_dev);
> 
> Re-indent them to have logical split on the lines.
Yes.

> 
>> +static int apds9306_runtime_suspend(struct device *dev)
>> +{
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> 
> WHy? Use dev_get_drvdata() directly.
Thanks. I'll use that function.

>> +
>> +static int apds9306_runtime_resume(struct device *dev)
>> +{
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> 
> Ditto.
Yes.

> Alternatively
> 
> static DEFINE_RUNTIME_DEV_PM_OPS(apds9306_pm_ops,
> 				 apds9306_runtime_suspend,
> 				 apds9306_runtime_resume,
> 				 NULL);
> 
Thank you. Second option looks nice. I'll stick to that.
> 
> Redundant blank line.
It will be fixed.


Regards,
Subhajit Ghosh
  

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

[Andy Shevchenko]

On Wed, Apr 12, 2023 at 12:29:15PM +0800, Subhajit Ghosh wrote:

...

> > > +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,
> > 
> > Do you need an internal regmap lock? If so, why?
> For event interface - interrupt enable, adaptive interrupt enable,
> upper and lower threshold values, selection of clear or als
> channels for interrupt, the mutex in the driver's private data structure
> is not used.
> I thought to use the regmap's internal locking mechanism for
> mutual exclusion as the values are directly written to or read from
> the device registers form the write_event(), read_event(),
> write_event_config() and read_event_config().
> What do you think?

I didn't get. If you have a sequence of registers to be read/write/modified/etc
in IRQ handler and/or elsewhere and at the same time in IRQ or elsewhere you
have even a single IO access to the hardware you have to be sure that the IO
ordering has no side effects. regmap API does not guarantee that. It only works
on a simple read/write/modify of a _single_ register, or a coupled group of
registers (like bulk ops), if your case is sparse, you on your own and probably
lucky enough not to have an issue during the testing. So, take your time and
think more about what you are doing in the driver and what locking schema
should take place.

...

> > > +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);
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > 
> > > +	return 0;
> > 
> > Move that to a single user of this line inside the switch-case.
> Sorry, I did not get you. Can you please elaborate?

The user of this return is only one case in the switch. Instead of breaking
the switch-case, just move this return statement to there.

...

> > > +	struct device *dev = &data->client->dev;
> > 
> > Why data contains I²C client pointer, what for?
> I copied the implementation. It will be re-implemented.

I mean, how client pointer is used in comparison to the plain pointer to the
generic device object.

...

> > > +		while (retries--) {
> > > +			ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS,
> > > +					&status);
> > > +			if (ret) {
> > > +				dev_err(dev, "read status failed: %d\n", ret);
> > > +				return ret;
> > > +			}
> > > +			if (status & APDS9306_ALS_DATA_STAT_MASK)
> > > +				break;
> > > +			/*
> > > +			 * In case of continuous one-shot read from userspace,
> > > +			 * new data is available after sampling period.
> > > +			 * Delays are in the range of 25ms to 2secs.
> > > +			 */
> > > +			fsleep(delay);
> > > +		}
> > 
> > regmap_read_poll_timeout().
> According to the regmap_read_poll_timeout() documentation, the maximum time
> to sleep between reads should be less than ~20ms as it uses usleep_range().
> 
> If userspace is doing continuous reads, then data is available after sampling
> period (25ms to 2sec) or integration time (3.125ms to 400ms) whichever is
> greater.
> 
> The runtime_suspend() function is called after 5 seconds, so the device is
> still active and running.
> 
> If the ALS data bit is not set in status reg, it is efficient to sleep for
> one sampling period rather than continuously checking the status reg
> within ~20ms if we use regmap_read_poll_timeout().
> 
> Do you have any suggestions?

Yes, Use proposed API. It takes _two_ timeout parameters, one of which is the
same as your delay. You may actually resplit it by multiplying retries and
decreasing delay to satisfy the regmap_read_poll_timeout() recommendation.

-- 
With Best Regards,
Andy Shevchenko

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

[Subhajit Ghosh]

On 12/4/23 21:37, Andy Shevchenko wrote:
> On Wed, Apr 12, 2023 at 12:29:15PM +0800, Subhajit Ghosh wrote:
> 
> ...
> 
>>>> +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,
>>>
>>> Do you need an internal regmap lock? If so, why?
>> For event interface - interrupt enable, adaptive interrupt enable,
>> upper and lower threshold values, selection of clear or als
>> channels for interrupt, the mutex in the driver's private data structure
>> is not used.
>> I thought to use the regmap's internal locking mechanism for
>> mutual exclusion as the values are directly written to or read from
>> the device registers form the write_event(), read_event(),
>> write_event_config() and read_event_config().
>> What do you think?
> 
> I didn't get. If you have a sequence of registers to be read/write/modified/etc
> in IRQ handler and/or elsewhere and at the same time in IRQ or elsewhere you
> have even a single IO access to the hardware you have to be sure that the IO
> ordering has no side effects. regmap API does not guarantee that. It only works
> on a simple read/write/modify of a _single_ register, or a coupled group of
> registers (like bulk ops), if your case is sparse, you on your own and probably
> lucky enough not to have an issue during the testing. So, take your time and
> think more about what you are doing in the driver and what locking schema
> should take place.
> 
> ...
Agree. I have to rethink and re-implement the locking mechanism.

> 
>>>> +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);
>>>> +		break;
>>>> +	default:
>>>> +		return -EINVAL;
>>>> +	}
>>>
>>>> +	return 0;
>>>
>>> Move that to a single user of this line inside the switch-case.
>> Sorry, I did not get you. Can you please elaborate?
> 
> The user of this return is only one case in the switch. Instead of breaking
> the switch-case, just move this return statement to there.
> 
Ok. It will be done.

> ...
> 
>>>> +	struct device *dev = &data->client->dev;
>>>
>>> Why data contains I²C client pointer, what for?
>> I copied the implementation. It will be re-implemented.
> 
> I mean, how client pointer is used in comparison to the plain pointer to the
> generic device object.
> 
> ...
> 
>>>> +		while (retries--) {
>>>> +			ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS,
>>>> +					&status);
>>>> +			if (ret) {
>>>> +				dev_err(dev, "read status failed: %d\n", ret);
>>>> +				return ret;
>>>> +			}
>>>> +			if (status & APDS9306_ALS_DATA_STAT_MASK)
>>>> +				break;
>>>> +			/*
>>>> +			 * In case of continuous one-shot read from userspace,
>>>> +			 * new data is available after sampling period.
>>>> +			 * Delays are in the range of 25ms to 2secs.
>>>> +			 */
>>>> +			fsleep(delay);
>>>> +		}
>>>
>>> regmap_read_poll_timeout().
>> According to the regmap_read_poll_timeout() documentation, the maximum time
>> to sleep between reads should be less than ~20ms as it uses usleep_range().
>>
>> If userspace is doing continuous reads, then data is available after sampling
>> period (25ms to 2sec) or integration time (3.125ms to 400ms) whichever is
>> greater.
>>
>> The runtime_suspend() function is called after 5 seconds, so the device is
>> still active and running.
>>
>> If the ALS data bit is not set in status reg, it is efficient to sleep for
>> one sampling period rather than continuously checking the status reg
>> within ~20ms if we use regmap_read_poll_timeout().
>>
>> Do you have any suggestions?
> 
> Yes, Use proposed API. It takes _two_ timeout parameters, one of which is the
> same as your delay. You may actually resplit it by multiplying retries and
> decreasing delay to satisfy the regmap_read_poll_timeout() recommendation.
> 
Yes, that can be done. I will re-write this function in the next patch.

Thanks once again Andy for the detailed review.

Regards,
Subhajit Ghosh

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

[Jonathan Cameron]

On Tue, 11 Apr 2023 09:12:03 +0800
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.
> 
> Datasheet at https://docs.broadcom.com/doc/AV02-4755EN
> 
> Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
> 
> ---
> 
> Software reset feature is not implemented as it causes I2C bus error,
> the I2C bus driver throws an ugly error message.

That's unfortunate and perhaps something we should consider fixing
at the i2c layer.  Could you point to where it happens?

We have a lot of drivers where reset causes an error (Ack missing normally
due to simple state machines in the devices).

> 
> Could not locate the Lux calculations from datasheet, only exporting
> raw values.

Ah. That's annoying as userspace is generally not able to do much with
the raw values.  Any other known code supporting this device that you
can raid for info?

If not, then this ist he best we can do.

> 
> Reading of the Status register clears the Data Ready and the Interrupt
> Status flags. It makes it tricky to read oneshot values together with
> interrupts enabled as the IRQ handler clears the status on receipt
> of an interrupt signal.
>  
> Not checking the status in IRQ handler will make the interrupt line
> unsharable and it does not reset the interrupt line if the Interrupt
> status flag is not cleared.

Definitely need to check it but I'm not sure I follow why you can't
use it for both purposes with a slightly complex interrupt handler design.
Maybe the code makes it clear what the issue is here.

> 
> Many applications don't need interrupts. It is good to have support
> for both in the driver.

Agreed.
> 
> Sysfs structure with interrupt line defined in DT:
> root@stm32mp1:~# tree -I 'dev|name|of_node|power|subsystem|uevent' \
>   /sys/bus/iio/devices/iio:device0/
> /sys/bus/iio/devices/iio:device0/
> |-- events

These should include the channel they apply to...

> |   |-- thresh_adaptive_either_en
> |   |-- thresh_adaptive_either_value
> |   |-- thresh_adaptive_either_values_available
> |   |-- thresh_channel_select

Non standards ABI needs documentation.
Guessing somewhat but do you have a single set of threshold detectors
that are fed by a mux off the channels?  So can only check one channel
at a time?  If so then you present one set of controls for each channel
and either
a) return -EBUSY if someone tries to turn more on than you support
b) operating in a fifo way - so always enable last one that was enabled.
   reading back values of the attributes will allow userspace to see what
   is turned on.


> |   |-- thresh_channels_available
> |   |-- thresh_either_en
> |   |-- thresh_either_period
> |   |-- thresh_either_period_available
> |   |-- thresh_falling_value
> |   `-- thresh_rising_value
> |-- in_illuminance_raw
> |-- in_intensity_clear_raw
> |-- integration_time
> |-- integration_time_available
> |-- sampling_frequency
> |-- sampling_frequency_available
> |-- scale
> `-- scale_available
> 
> Sysfs structure with no interrupt:
> root@stm32mp1:~# tree -I 'dev|name|of_node|power|subsystem|uevent' \
>   /sys/bus/iio/devices/iio:device0/
> /sys/bus/iio/devices/iio:device0/
> |-- in_illuminance_raw
> |-- in_intensity_clear_raw
> |-- integration_time
> |-- integration_time_available
> |-- sampling_frequency
> |-- sampling_frequency_available
> |-- scale
> `-- scale_available
> 
> As there is a single interrupt enable option, using the existing
> IIO bitmasks to cater for channel selection - als or clear and
> interrupt type selection - threshold or adaptive
> was looking ambiguous. I am open to any other implementations.

This sort of dependency is common but it is effectively impossible
to design a usable ABI that covers all combinations that can occur.
As a result the ABI defines that a write to any sysfs attribute
'might' change the value of any other.   Hence you just have two enables
and enabling 1 turns them both on.

> 
> I could not find any _available implementations for the event
> interface.

Yes - we haven't yet added support except by creating the attributes
manually.  It might be worth doing. I've never checked how useful
it would be.

> 
> Created one sysfs attribute for interrupt channel selection
> and one RO attribute for channel availability.
> 
> Created two RO attributes for interrupt period and threshold
> adaptive value available ranges.
> 
> The ALS Measurement Rate (Sampling Frequency) has max values
> of 2000ms and 2000ms for both 110b and 111b respectively.
> This is not a typo. Found out experimentally that assigning
> either values, interrupt is generated 2 secs apart when
> interrupt conditions are met.
> 

Some comments inline.  Given we have these big questions, I didn't focus
too much on details. That can come later once the big stuff looks right.

Thanks,

Jonathan

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

> +static struct iio_event_spec apds9306_event_spec[] = {
> +	{
> +		.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_shared_by_all = BIT(IIO_EV_INFO_ENABLE),
> +	},
> +};
> +
> +static struct iio_chan_spec apds9306_channels[] = {
> +	{
> +		.type = IIO_LIGHT,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.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),
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 20,
> +			.storagebits = 32,
> +		},
> +		.event_spec = apds9306_event_spec,
> +		.num_event_specs = ARRAY_SIZE(apds9306_event_spec),
You'll end up with event specs for both channels.

> +	},
> +	{
	}, { 

is fine and more compact. Can use this in all similar places.

> +		.type = IIO_INTENSITY,
> +		.channel2 = IIO_MOD_LIGHT_CLEAR,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.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),
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 20,
> +			.storagebits = 32,
> +		},
> +		.modified = 1,
> +	},
> +};
> +
> +static struct iio_chan_spec apds9306_channels_no_events[] = {
> +	{
> +		.type = IIO_LIGHT,

LIGHT is an illumiance measure.  Given you don't have the info to get anywhere
near that and the absorption characteristics are unlikely to be such that
this channel alone can be used to work out the value LIGHT is not the right
type.  This is just another INTENSITY channel. 

> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.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),
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 20,
> +			.storagebits = 32,
> +		},
> +	},
> +	{
> +		.type = IIO_INTENSITY,
> +		.channel2 = IIO_MOD_LIGHT_CLEAR,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.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),
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 20,
> +			.storagebits = 32,
> +		},
> +		.modified = 1,
> +	},
> +};
> +
> +static ssize_t thresh_channel_select_show(struct device *dev,
> +		struct device_attribute *attr, char *buff)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct apds9306_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (data->int_ch == 1)
> +		ret = sprintf(buff, "als\n");
> +	else if (data->int_ch == 0)
> +		ret = sprintf(buff, "clear\n");
> +	else
> +		ret = -EINVAL;
> +
> +	return ret;
> +}
> +
> +static ssize_t thresh_channel_select_store(struct device *dev,
> +		struct device_attribute *attr, const char *buff, size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct apds9306_data *data = iio_priv(indio_dev);
> +	int ret, channel;
> +
> +	if (len <= 0 || len > 6)
> +		return -EINVAL;
> +
> +	if (!strncmp(buff, "als", 3))
> +		channel = 1;
> +	else if (!strncmp(buff, "clear", 5))
> +		channel = 0;

As above, provide an event for each channel.  Only one can be enabled
at a time.

> +	else
> +		return -EINVAL;
> +
> +	ret = regmap_field_write(data->regfield_int_src, channel);
> +	if (ret)
> +		return ret;
> +
> +	data->int_ch = channel;
> +
> +	return len;
> +}

...


>
> +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);
> +		break;
		return 0;

> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static int apds9306_runtime_power(struct apds9306_data *data, int en)
> +{
> +	struct device *dev = &data->client->dev;
> +	int ret = 0;
> +
> +	if (en) {
> +		ret = pm_runtime_resume_and_get(dev);
> +		if (ret < 0) {
> +			dev_err(dev, "runtime resume failed: %d\n", ret);
> +			return ret;
> +		}
> +		ret = 0;
		return 0;
> +	} else {
> +		pm_runtime_mark_last_busy(dev);
> +		pm_runtime_put_autosuspend(dev);
		return 0;
> +	}
> +
> +	return ret;
> +}
> +
> +static int apds9306_read_data(struct apds9306_data *data, int *val, int reg)
> +{
> +	struct device *dev = &data->client->dev;
> +	int ret, delay, status, int_en;
> +	int retries = 4;
> +	u8 buff[3];
> +
> +	ret = apds9306_runtime_power(data, 1);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Whichever is greater - integration time period or
> +	 * sampling period.
> +	 */
> +	delay = max(apds9306_intg_time[data->intg_time_idx][1],
> +			apds9306_repeat_rate_period[data->repeat_rate_idx]);
> +
> +	/*
> +	 * If interrupts are enabled then Status resistor cannot be
> +	 * relied upon as all the status bits are cleared by the
> +	 * interrupt handler in case of an event.

Ah.  I was assuming sane hardware (always an error :) that would issue
an interrupt on the data being ready.  I think we can make this work
but it is ugly.  Add some flags to the state structure. Then whenever
you read this register, set whether the two status flags are set of not.
Thus in the interrupt handler you can tell if this got there first and
here you can tell if the interrupt handler got their first.

One messy corner.  A status read resets the interrupt line, potentially before
we saw the interrupt. Oh goody - normally this silliness only happens as
a result of complex interrupt migration or errata. However it is understood
what to do about it.

If you see the interrupt status flag here, you have no way of knowing
if the interrupt line was high for long enough that the interrupt controller
saw it.  As such your only option is to assume it didn't and inject an extra
one.  Given a passing of the threshold could in theory have been noisy enough
to trigger two actual interrupts very close together userspace should be fine
with the extra event - we probably just wasted some cycles doing something twice.

The annoying bit will be testing as these races will be somewhat rare.

> +	 */
> +	ret = regmap_field_read(data->regfield_int_en, &int_en);
> +	if (ret) {
> +		dev_err(dev, "read interrupt status failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (!int_en) {
> +		while (retries--) {
> +			ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS,
> +					&status);
> +			if (ret) {
> +				dev_err(dev, "read status failed: %d\n", ret);
> +				return ret;
> +			}
> +			if (status & APDS9306_ALS_DATA_STAT_MASK)
> +				break;
> +			/*
> +			 * In case of continuous one-shot read from userspace,
> +			 * new data is available after sampling period.
> +			 * Delays are in the range of 25ms to 2secs.
> +			 */
> +			fsleep(delay);
> +		}
> +	} else
> +		fsleep(delay);
> +
> +	if (!retries)
> +		return -EBUSY;
> +
> +	ret = regmap_bulk_read(data->regmap, reg, buff, sizeof(buff));
> +	if (ret) {
> +		dev_err(&data->client->dev, "read data failed\n");
> +		return ret;
> +	}
> +
> +	*val = get_unaligned_le24(&buff[0]);
> +
> +	ret = apds9306_runtime_power(data, 0);
> +	if (ret)
> +		return ret;
> +
> +	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)
> +			ret = IIO_VAL_INT;
> +		*val2 = 0;
> +		break;
> +	case IIO_CHAN_INFO_INT_TIME:
> +		ret = apds9306_intg_time_get(data, val, val2);
> +		if (!ret)
> +			ret = IIO_VAL_INT_PLUS_MICRO;
> +		break;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = apds9306_sampling_freq_get(data, val, val2);
> +		if (!ret)
> +			ret = IIO_VAL_INT_PLUS_MICRO;
> +		break;
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = apds9306_gain_get(data, val);
> +		if (!ret)

Please keep the errors out of line.
		if (ret)
			break;
		ret = IIO_VAL_INT;
		*val2 = 0;
		break;

> +			ret = IIO_VAL_INT;
> +		*val2 = 0;
> +		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)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_INT_TIME:
> +		*length = ARRAY_SIZE(apds9306_intg_time) * 2;
> +		*vals = (const int *)apds9306_intg_time;
> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		return IIO_AVAIL_LIST;
> +	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;
> +	case IIO_CHAN_INFO_SCALE:
> +		*length = ARRAY_SIZE(apds9306_gain);
> +		*vals = apds9306_gain;
> +		*type = IIO_VAL_INT;
> +		return IIO_AVAIL_LIST;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
Can't get here.

> +}
> +
> +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 = -EINVAL;
> +
> +	mutex_lock(&data->mutex);
> +	switch (mask) {
> +	case IIO_CHAN_INFO_INT_TIME:
> +		ret = apds9306_intg_time_set(data, val, val2);
> +		break;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = apds9306_sampling_freq_set(data, val, val2);
> +		break;
> +	case IIO_CHAN_INFO_SCALE:
> +		if (val2 != 0)
> +			break;
> +		ret = apds9306_gain_set(data, val);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}
> +
> +static const struct iio_info apds9306_info_no_events = {
> +		.read_raw = apds9306_read_raw,
> +		.read_avail = apds9306_read_avail,
> +		.write_raw = apds9306_write_raw,
> +};
> +
> +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

	/*
	 * The interrupt.

> +	 * cleared as a result of reading the status register
> +	 */
> +	ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS, &status);
> +	if (ret < 0) {
> +		dev_err(&data->client->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));
> +	}
> +
> +	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);
> +	u8 buff[3];
> +	int var, ret;
> +
> +	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);
> +			if (ret)
> +				return ret;
> +			break;
> +		}
> +
> +		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[0]);
> +	break;
> +	case IIO_EV_TYPE_THRESH_ADAPTIVE:
> +		ret = apds9306_event_thresh_adaptive_get(data, val);
> +		if (ret)
> +			return ret;
> +	break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	*val2 = 0;
> +	return IIO_VAL_INT;

Pull this into the cases above so you can return directly.

> +}
> +
> +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);
> +	unsigned int var;
> +	u8 buff[3];
> +
> +	switch (type) {
> +	case IIO_EV_TYPE_THRESH:
> +		if (dir == IIO_EV_DIR_EITHER && info == IIO_EV_INFO_PERIOD)
> +			return apds9306_event_period_set(data, val);
> +		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 > 0xFFFFF)
> +			return -EINVAL;
> +
> +		put_unaligned_le24(val, buff);
> +		return regmap_bulk_write(data->regmap, var, buff, sizeof(buff));
> +	case IIO_EV_TYPE_THRESH_ADAPTIVE:
> +		return apds9306_event_thresh_adaptive_set(data, val);
> +	default:
> +		return -EINVAL;
> +	}
> +

Can't get here.

> +	return 0;
> +}
> +
> +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;
> +	int ret;
> +
> +	switch (type) {
> +	case IIO_EV_TYPE_THRESH:
> +		ret = regmap_field_read(data->regfield_int_en, &val);
> +		if (ret)
> +			return ret;
> +		return val;
> +	case IIO_EV_TYPE_THRESH_ADAPTIVE:
> +		ret = regmap_field_read(data->regfield_int_thresh_var_en, &val);
> +		if (ret)
> +			return ret;
> +		return val;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;

Can't get here so don't have this final return. 

> +}
> +
> +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, curr_state;
> +
> +	state = !!state;
> +	switch (type) {
> +	case IIO_EV_TYPE_THRESH:
> +		ret = regmap_field_read(data->regfield_int_en, &curr_state);
> +		if (ret)
> +			return ret;
> +		if (curr_state == state)
> +			return 0;
> +		ret = regmap_field_write(data->regfield_int_en, state);
> +		if (ret)
> +			return ret;
> +		mutex_lock(&data->mutex);

This seems unusual.  I don't follow why lock is needed here.

> +		ret = apds9306_runtime_power(data, state);
> +		mutex_unlock(&data->mutex);
> +		if (ret)
> +			return ret;

		return ret;

> +		break;
> +	case IIO_EV_TYPE_THRESH_ADAPTIVE:
> +		ret = regmap_field_write(data->regfield_int_thresh_var_en, state);
> +		if (ret)
> +			return ret;

		return regmap...

> +		break;
> +	default:
> +		ret = -EINVAL;

Early returns are more readable.
		return -EINVAL;

> +	}
> +
> +	return ret;
> +}
> +
> +static const struct iio_info apds9306_info = {
> +		.event_attrs = &apds9306_event_attr_group,

Indented one tab more than needed.

> +		.read_raw = apds9306_read_raw,
> +		.read_avail = apds9306_read_avail,
> +		.write_raw = apds9306_write_raw,
> +		.read_event_value = apds9306_read_event,
> +		.write_event_value = apds9306_write_event,
> +		.read_event_config = apds9306_read_event_config,
> +		.write_event_config = apds9306_write_event_config,
> +};
> +
> +static void apds9306_powerdown(void *ptr)
> +{
> +	struct apds9306_data *data = (struct apds9306_data *)ptr;
> +	int status;
> +
> +	/* Disable interrupts */
> +	regmap_field_write(data->regfield_int_thresh_var_en, 0);
> +	regmap_field_write(data->regfield_int_en, 0);
> +	/* Clear status */

Why?

> +	regmap_read(data->regmap, APDS9306_MAIN_STATUS, &status);
> +	/* Put the device in standby mode */
> +	apds9306_power_state(data, standby);
> +}
> +
> +static int apds9306_init_device(struct apds9306_data *data)

If an error occurs, this function should be as side effect free
as we can make it easily.  So I'd expect to see the power
state disabled on error. Some of the register writes look harmless
to leave in place on error though so can skip those.



> +{
> +	struct device *dev = &data->client->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;
> +
> +	/* Integration time: 100 msec */
> +	ret = apds9306_intg_time_set(data, 0, 100000);
> +	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(data, 3);
> +	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 */
> +	ret = regmap_field_write(data->regfield_int_thresh_var_en, 0);
> +	if (ret)
> +		return ret;
return regmap_field_write()

one of the static analysis tools loves this, so if we don't tidy it up
we'll probably get a patch soon to 'fix' it.  Better to do it from the
start.
> +
> +	return 0;
> +}
> +
> +static int apds9306_probe(struct i2c_client *client)
> +{
> +	struct apds9306_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	data->indio_dev = indio_dev;
> +
> +	mutex_init(&data->mutex);
> +
> +	data->regmap = devm_regmap_init_i2c(client, &apds9306_regmap);
> +	if (IS_ERR(data->regmap))
> +		return dev_err_probe(&client->dev, PTR_ERR(data->regmap),
> +					"regmap initialization failed\n");
> +
> +	i2c_set_clientdata(client, indio_dev);
> +	data->client = client;
> +
> +	ret = apds9306_regfield_init(data);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +					"regfield initialization failed\n");
> +
> +	ret = devm_regulator_get_enable(&client->dev, "vin");
> +	if (ret)
> +		return dev_err_probe(&client->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;
> +		indio_dev->num_channels = ARRAY_SIZE(apds9306_channels);
> +		ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> +				apds9306_irq_handler, IRQF_TRIGGER_FALLING |
> +				IRQF_ONESHOT, "apds9306_event", indio_dev);
> +		if (ret)
> +			return dev_err_probe(&client->dev, ret,
> +					"failed to assign interrupt.\n");
> +	} else {
> +		indio_dev->info = &apds9306_info_no_events;
> +		indio_dev->channels = apds9306_channels_no_events;
> +		indio_dev->num_channels = ARRAY_SIZE(apds9306_channels_no_events);
> +	}
> +
> +	ret = devm_add_action_or_reset(&client->dev, apds9306_powerdown, data);
What is this paired with?  Normally a devm action has to be undoing something
that it follows.  Here I'm not immediately seeing a power up.  Perhaps it should
follow init below?  

> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				"failed to add action on driver unwind\n");
> +
> +	ret = apds9306_init_device(data);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "failed to init device\n");
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +
> +static int apds9306_runtime_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(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 = i2c_get_clientdata(to_i2c_client(dev));
> +	struct apds9306_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	regcache_cache_only(data->regmap, false);
> +	ret = regcache_sync(data->regmap);
> +	if (!ret) {
> +		ret = apds9306_power_state(data, active);

Add a comment on why this might be needed only sometimes. Not obvious to me.

> +		if (ret)
> +			regcache_cache_only(data->regmap, true);
> +	}
> +
> +	return ret;
> +}

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

[Subhajit Ghosh]

Thank you Jonathan for the review.
Answering only to the Big questions.
>>
>> Software reset feature is not implemented as it causes I2C bus error,
>> the I2C bus driver throws an ugly error message.
> 
> That's unfortunate and perhaps something we should consider fixing
> at the i2c layer.  Could you point to where it happens?
> 
> We have a lot of drivers where reset causes an error (Ack missing normally
> due to simple state machines in the devices).

Below function which cause the error:
regmap_field_write(data->reg_sw_reset, 1);
regmap_write(data->regmap, APDS9306_MAIN_CTRL, 0x10);
i2c_smbus_write_byte_data(data->client, APDS9306_MAIN_CTRL, 0x10);

Error messages:
[ 3340.426180] stm32f7-i2c 40012000.i2c: <stm32f7_i2c_isr_error>: Bus error accessing addr 0x52
[ 3340.433880] stm32f7-i2c 40012000.i2c: Trying to recover bus

The function which gets called:
https://elixir.bootlin.com/linux/latest/source/drivers/i2c/busses/i2c-stm32f7.c#L1622

There is an errata associated with I2C for STM32MP157C (Section 2.19.2, Pg 35):
https://www.st.com/resource/en/errata_sheet/es0438-stm32mp151x3x7x-device-errata-stmicroelectronics.pdf
It speaks about - "Spurious bus error detection in master mode". But I
don't think it has got anything to do with our case.

I use STM32MP157C-DK2 board as my reference device.
The Reference manual to the STM32MP157C:
https://www.st.com/resource/en/reference_manual/DM00327659.pdf

stm32f7_i2c_isr_error() handler gets called because a control bit is set
ERRIE which enables interrupts from the I2C controller for Buss errors,
Arbitration losses, Overrun/Underrun PEC error, Timeout, etc.

I am not sure about other chips.
A possible way to mitigate these kind of issues would be to pass a flag
from upper layers to the i2c bus driver (I2C_SMBUS_REPORT_ERR_OFF or
something on those lines). The drivers can then implement in
struct i2c_algorithm in smbus_xfer() function where they can check for
the flag and disable error checking.

I don't have in depth knowledge on this subject so excuse my lack
of understanding.

> 
>>
>> Could not locate the Lux calculations from datasheet, only exporting
>> raw values.
> 
> Ah. That's annoying as userspace is generally not able to do much with
> the raw values.  Any other known code supporting this device that you
> can raid for info?
> 
> If not, then this ist he best we can do.
> 
This device is similar to LTRF216A.
If I use the calculation in ltrf216a then I would have to verify them
with Lux meter and controlled light source or ltrf216a.
This will be bit difficult for me at this moment.

>>
>> Reading of the Status register clears the Data Ready and the Interrupt
>> Status flags. It makes it tricky to read oneshot values together with
>> interrupts enabled as the IRQ handler clears the status on receipt
>> of an interrupt signal.
>>   
>> Not checking the status in IRQ handler will make the interrupt line
>> unsharable and it does not reset the interrupt line if the Interrupt
>> status flag is not cleared.
> 
> Definitely need to check it but I'm not sure I follow why you can't
> use it for both purposes with a slightly complex interrupt handler design.
> Maybe the code makes it clear what the issue is here.
Answers are in below comment.

>> +
>> +static int apds9306_read_data(struct apds9306_data *data, int *val, int reg)
>> +{
>> +	struct device *dev = &data->client->dev;
>> +	int ret, delay, status, int_en;
>> +	int retries = 4;
>> +	u8 buff[3];
>> +
>> +	ret = apds9306_runtime_power(data, 1);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * Whichever is greater - integration time period or
>> +	 * sampling period.
>> +	 */
>> +	delay = max(apds9306_intg_time[data->intg_time_idx][1],
>> +			apds9306_repeat_rate_period[data->repeat_rate_idx]);
>> +
>> +	/*
>> +	 * If interrupts are enabled then Status resistor cannot be
>> +	 * relied upon as all the status bits are cleared by the
>> +	 * interrupt handler in case of an event.
> 
> Ah.  I was assuming sane hardware (always an error :) that would issue
> an interrupt on the data being ready.  I think we can make this work
> but it is ugly.  Add some flags to the state structure. Then whenever
> you read this register, set whether the two status flags are set of not.
> Thus in the interrupt handler you can tell if this got there first and
> here you can tell if the interrupt handler got their first.
If I have a flag in the state structure for this, then a timestamp would
also be required or may be just a timestamp.

> 
> One messy corner.  A status read resets the interrupt line, potentially before
> we saw the interrupt. Oh goody - normally this silliness only happens as
> a result of complex interrupt migration or errata. However it is understood
> what to do about it.
> 
> If you see the interrupt status flag here, you have no way of knowing
> if the interrupt line was high for long enough that the interrupt controller
> saw it.  As such your only option is to assume it didn't and inject an extra
> one.  Given a passing of the threshold could in theory have been noisy enough
> to trigger two actual interrupts very close together userspace should be fine
> with the extra event - we probably just wasted some cycles doing something twice.
> 
> The annoying bit will be testing as these races will be somewhat rare.

As per my understanding the Status register has - "Observer effect"

The Status register has got 3 bits - Power On Status, ALS Interrupt
and ALS Data.
A single read of the register clears all three bits.

In case of an interrupt event, the interrupt line is held low (Active) by the
device till the Status register is read. As this is an edge triggered interrupt,
we stop receiving interrupts till we read the status register.
This is why it is good to acknowledge the interrupt by reading the Status register
in the ISR.

Reading the Status register in the ISR inadvertently clears the "ALS Data" bit
as well.

What you are asking is to have a read flag in the static struct and a timestamp
flag may be. In the isr, we check the flag and if the timestamp is within one
integration period, if yes, we don't read the Status register any more from the
isr.
And in this function we read the Status register and update the read flag and
the timestamp with current time.

Not sure if my understanding is correct but I will try the above.

> 
>> +	 */
>> +	ret = regmap_field_read(data->regfield_int_en, &int_en);
>> +	if (ret) {
>> +		dev_err(dev, "read interrupt status failed: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	if (!int_en) {
>> +		while (retries--) {
>> +			ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS,
>> +					&status);
>> +			if (ret) {
>> +				dev_err(dev, "read status failed: %d\n", ret);
>> +				return ret;
>> +			}
>> +			if (status & APDS9306_ALS_DATA_STAT_MASK)
>> +				break;
>> +			/*
>> +			 * In case of continuous one-shot read from userspace,
>> +			 * new data is available after sampling period.
>> +			 * Delays are in the range of 25ms to 2secs.
>> +			 */
>> +			fsleep(delay);
>> +		}
>> +	} else
>> +		fsleep(delay);
>> +
>> +	if (!retries)
>> +		return -EBUSY;
>> +
>> +	ret = regmap_bulk_read(data->regmap, reg, buff, sizeof(buff));
>> +	if (ret) {
>> +		dev_err(&data->client->dev, "read data failed\n");
>> +		return ret;
>> +	}
>> +
>> +	*val = get_unaligned_le24(&buff[0]);
>> +
>> +	ret = apds9306_runtime_power(data, 0);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return ret;
>> +}
>> +

Regards,
Subhajit Ghosh

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

[Jonathan Cameron]

On Mon, 17 Apr 2023 17:25:50 +0800
Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:

> Thank you Jonathan for the review.
> Answering only to the Big questions.
> >>
> >> Software reset feature is not implemented as it causes I2C bus error,
> >> the I2C bus driver throws an ugly error message.  
> > 
> > That's unfortunate and perhaps something we should consider fixing
> > at the i2c layer.  Could you point to where it happens?
> > 
> > We have a lot of drivers where reset causes an error (Ack missing normally
> > due to simple state machines in the devices).  
> 
> Below function which cause the error:
> regmap_field_write(data->reg_sw_reset, 1);
> regmap_write(data->regmap, APDS9306_MAIN_CTRL, 0x10);
> i2c_smbus_write_byte_data(data->client, APDS9306_MAIN_CTRL, 0x10);
> 
> Error messages:
> [ 3340.426180] stm32f7-i2c 40012000.i2c: <stm32f7_i2c_isr_error>: Bus error accessing addr 0x52
> [ 3340.433880] stm32f7-i2c 40012000.i2c: Trying to recover bus
> 
> The function which gets called:
> https://elixir.bootlin.com/linux/latest/source/drivers/i2c/busses/i2c-stm32f7.c#L1622
> 
> There is an errata associated with I2C for STM32MP157C (Section 2.19.2, Pg 35):
> https://www.st.com/resource/en/errata_sheet/es0438-stm32mp151x3x7x-device-errata-stmicroelectronics.pdf
> It speaks about - "Spurious bus error detection in master mode". But I
> don't think it has got anything to do with our case.
> 
> I use STM32MP157C-DK2 board as my reference device.
> The Reference manual to the STM32MP157C:
> https://www.st.com/resource/en/reference_manual/DM00327659.pdf
> 
> stm32f7_i2c_isr_error() handler gets called because a control bit is set
> ERRIE which enables interrupts from the I2C controller for Buss errors,
> Arbitration losses, Overrun/Underrun PEC error, Timeout, etc.
> 
> I am not sure about other chips.
> A possible way to mitigate these kind of issues would be to pass a flag
> from upper layers to the i2c bus driver (I2C_SMBUS_REPORT_ERR_OFF or
> something on those lines). The drivers can then implement in
> struct i2c_algorithm in smbus_xfer() function where they can check for
> the flag and disable error checking.
> 
> I don't have in depth knowledge on this subject so excuse my lack
> of understanding.

No problem and thanks for the details.

So there is an existing flag: I2C_M_IGNORE_NAK but I'm not sure that's
suitable and isn't supported by this particular bus driver.
https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/msp3400-driver.c#L100
is an example of it being used in a reset routine.

It might be sufficient to just check that flag in the stm32f7 i2c driver and not
print an error message if it is set.

+CC Wolfram for input on this.




> 
> >   
> >>
> >> Could not locate the Lux calculations from datasheet, only exporting
> >> raw values.  
> > 
> > Ah. That's annoying as userspace is generally not able to do much with
> > the raw values.  Any other known code supporting this device that you
> > can raid for info?
> > 
> > If not, then this ist he best we can do.
> >   
> This device is similar to LTRF216A.
> If I use the calculation in ltrf216a then I would have to verify them
> with Lux meter and controlled light source or ltrf216a.
> This will be bit difficult for me at this moment.

no problem. We can add that later if needed.
> 
> >>
> >> Reading of the Status register clears the Data Ready and the Interrupt
> >> Status flags. It makes it tricky to read oneshot values together with
> >> interrupts enabled as the IRQ handler clears the status on receipt
> >> of an interrupt signal.
> >>   
> >> Not checking the status in IRQ handler will make the interrupt line
> >> unsharable and it does not reset the interrupt line if the Interrupt
> >> status flag is not cleared.  
> > 
> > Definitely need to check it but I'm not sure I follow why you can't
> > use it for both purposes with a slightly complex interrupt handler design.
> > Maybe the code makes it clear what the issue is here.  
> Answers are in below comment.
> 
> >> +
> >> +static int apds9306_read_data(struct apds9306_data *data, int *val, int reg)
> >> +{
> >> +	struct device *dev = &data->client->dev;
> >> +	int ret, delay, status, int_en;
> >> +	int retries = 4;
> >> +	u8 buff[3];
> >> +
> >> +	ret = apds9306_runtime_power(data, 1);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	/*
> >> +	 * Whichever is greater - integration time period or
> >> +	 * sampling period.
> >> +	 */
> >> +	delay = max(apds9306_intg_time[data->intg_time_idx][1],
> >> +			apds9306_repeat_rate_period[data->repeat_rate_idx]);
> >> +
> >> +	/*
> >> +	 * If interrupts are enabled then Status resistor cannot be
> >> +	 * relied upon as all the status bits are cleared by the
> >> +	 * interrupt handler in case of an event.  
> > 
> > Ah.  I was assuming sane hardware (always an error :) that would issue
> > an interrupt on the data being ready.  I think we can make this work
> > but it is ugly.  Add some flags to the state structure. Then whenever
> > you read this register, set whether the two status flags are set of not.
> > Thus in the interrupt handler you can tell if this got there first and
> > here you can tell if the interrupt handler got their first.  
> If I have a flag in the state structure for this, then a timestamp would
> also be required or may be just a timestamp.

True - though given we are dealing with a race here, I'd imagine any wrong
timestamp issues will only make a small difference.

> 
> > 
> > One messy corner.  A status read resets the interrupt line, potentially before
> > we saw the interrupt. Oh goody - normally this silliness only happens as
> > a result of complex interrupt migration or errata. However it is understood
> > what to do about it.
> > 
> > If you see the interrupt status flag here, you have no way of knowing
> > if the interrupt line was high for long enough that the interrupt controller
> > saw it.  As such your only option is to assume it didn't and inject an extra
> > one.  Given a passing of the threshold could in theory have been noisy enough
> > to trigger two actual interrupts very close together userspace should be fine
> > with the extra event - we probably just wasted some cycles doing something twice.
> > 
> > The annoying bit will be testing as these races will be somewhat rare.  
> 
> As per my understanding the Status register has - "Observer effect"
> 
> The Status register has got 3 bits - Power On Status, ALS Interrupt
> and ALS Data.
> A single read of the register clears all three bits.
> 
> In case of an interrupt event, the interrupt line is held low (Active) by the
> device till the Status register is read. As this is an edge triggered interrupt,
> we stop receiving interrupts till we read the status register.
> This is why it is good to acknowledge the interrupt by reading the Status register
> in the ISR.
> 
> Reading the Status register in the ISR inadvertently clears the "ALS Data" bit
> as well.
> 
> What you are asking is to have a read flag in the static struct and a timestamp
> flag may be. In the isr, we check the flag and if the timestamp is within one
> integration period, if yes, we don't read the Status register any more from the
> isr.

Not quite. Reading the ISR should be safe even if we already read it from the read_raw()
path.  The worst that happens is we report one event rather than two, but they will
close enough together that won't matter.  Potentially races may mean we signal 2
events, but that's fine too. Just looks like a bit of noise on the analog signal
so shouldn't confuse software anyway.  To ensure the read raw path doesn't wait
for ever you'll also need to store a flag to say the data is good which is then
used to detect we have wiped that bit before read_raw saw it.  The read_raw path
can clear that flag so we know it's fresh if the interrupt sets it later.

> And in this function we read the Status register and update the read flag and
> the timestamp with current time.

If you see the bit that corresponds to the interrupt here, then you might never
see the interrupt (it might be too short to rise properly).   As such I think you'll
need to push the event from the read_raw path.  If the interrupt is seen then
we may find no bits set and that will be fine.

Need a bit of scribbling of timing diagrams to get all paths covered, but
I'm fairly sure it is possible.  Main challenge will be to keep it minimal and
not over engineer the solution!

> 
> Not sure if my understanding is correct but I will try the above.
> 
> >   
> >> +	 */
> >> +	ret = regmap_field_read(data->regfield_int_en, &int_en);
> >> +	if (ret) {
> >> +		dev_err(dev, "read interrupt status failed: %d\n", ret);
> >> +		return ret;
> >> +	}
> >> +
> >> +	if (!int_en) {
> >> +		while (retries--) {
> >> +			ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS,
> >> +					&status);
> >> +			if (ret) {
> >> +				dev_err(dev, "read status failed: %d\n", ret);
> >> +				return ret;
> >> +			}
> >> +			if (status & APDS9306_ALS_DATA_STAT_MASK)
> >> +				break;
> >> +			/*
> >> +			 * In case of continuous one-shot read from userspace,
> >> +			 * new data is available after sampling period.
> >> +			 * Delays are in the range of 25ms to 2secs.
> >> +			 */
> >> +			fsleep(delay);
> >> +		}
> >> +	} else
> >> +		fsleep(delay);
> >> +
> >> +	if (!retries)
> >> +		return -EBUSY;
> >> +
> >> +	ret = regmap_bulk_read(data->regmap, reg, buff, sizeof(buff));
> >> +	if (ret) {
> >> +		dev_err(&data->client->dev, "read data failed\n");
> >> +		return ret;
> >> +	}
> >> +
> >> +	*val = get_unaligned_le24(&buff[0]);
> >> +
> >> +	ret = apds9306_runtime_power(data, 0);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	return ret;
> >> +}
> >> +  
> 
> Regards,
> Subhajit Ghosh

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

[Jonathan Cameron]

On Tue, 11 Apr 2023 09:12:01 +0800
Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:

> 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
> - 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 channel - als or clear
> - Selection of interrupt mode - threshold or adaptive
> - Level selection for adaptive threshold interrupts
> - Persistence (Period) level selection for interrupts
> 
> Signed-off-by: Subhajit Ghosh <subhajit.ghosh@tweaklogic.com>
Hi Subhajit,

No need to sign off a cover letter.  The content isn't captured in the
git tree anyway.

For an RFC, I'd expect to see a clear statement in the cover letter of
why it is an RFC rather than a formal patch submission.  What specifically
are you looking for comments on?

Point us in the right direction and we might answer the questions quicker.

Thanks,

Jonathan

> 
> Subhajit Ghosh (2):
>   dt-bindings: Document APDS9306 Light Sensor bindings
>   iio: light: Add support for APDS9306 Light Sensor
> 
>  .../bindings/iio/light/avago,apds9306.yaml    |   47 +
>  drivers/iio/light/Kconfig                     |   11 +
>  drivers/iio/light/Makefile                    |    1 +
>  drivers/iio/light/apds9306.c                  | 1146 +++++++++++++++++
>  4 files changed, 1205 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/light/avago,apds9306.yaml
>  create mode 100644 drivers/iio/light/apds9306.c
> 
> 
> base-commit: 0d3eb744aed40ffce820cded61d7eac515199165

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

[Subhajit Ghosh]

Hi Jonathan,
Thank you for getting back.

> Hi Subhajit,
> 
> No need to sign off a cover letter.  The content isn't captured in the
> git tree anyway.
> 
> For an RFC, I'd expect to see a clear statement in the cover letter of
> why it is an RFC rather than a formal patch submission.  What specifically
> are you looking for comments on?
> 
> Point us in the right direction and we might answer the questions quicker.
> 
> Thanks,
> 
> Jonathan
Thank you for clearing it up.
Next version of RFC I will put specific reasons.
Before submitting a formal patch I wanted to check if my implementation of
single reads of ALS data raw values from userspace when interrupts are
enabled is the right thing to do or not. Also wanted to check if my event
related userspace ABI implementation is in line with IIO subsystem.
I will put it into better words in the next cover letter.

Can you also help me out with the git tree I should use to format the
patches? As per my understanding it is the subsystem maintainer tree
and the main branch but the macros and functions which you have suggested
in other reviews are available in Linux mainline.

Regards,
Subhajit Ghosh

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

[Jonathan Cameron]

On Wed, 12 Apr 2023 12:46:49 +0800
Subhajit Ghosh <subhajit.ghosh@tweaklogic.com> wrote:

> Hi Jonathan,
> Thank you for getting back.
> 
> > Hi Subhajit,
> > 
> > No need to sign off a cover letter.  The content isn't captured in the
> > git tree anyway.
> > 
> > For an RFC, I'd expect to see a clear statement in the cover letter of
> > why it is an RFC rather than a formal patch submission.  What specifically
> > are you looking for comments on?
> > 
> > Point us in the right direction and we might answer the questions quicker.
> > 
> > Thanks,
> > 
> > Jonathan  
> Thank you for clearing it up.
> Next version of RFC I will put specific reasons.
> Before submitting a formal patch I wanted to check if my implementation of
> single reads of ALS data raw values from userspace when interrupts are
> enabled is the right thing to do or not. Also wanted to check if my event
> related userspace ABI implementation is in line with IIO subsystem.
> I will put it into better words in the next cover letter.
> 
> Can you also help me out with the git tree I should use to format the
> patches? As per my understanding it is the subsystem maintainer tree
> and the main branch but the macros and functions which you have suggested
> in other reviews are available in Linux mainline.

For a new driver it rarely matters and I'd advise simply using
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
which is the mainline tree.  Please base either on the previous
release (currently 6.2) or rc1 of the current release (v6.3-rc1)
if doing this.

If you need a feature that has only been applied in the same cycle, or
are building on recent work that has been applied to the iio tree then
for fixes you want:
https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git fixes-togreg
for new stuff you want:
https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg

The IIO tree routes through Greg KH's char-misc tree so will see the togreg
branch move forwards to be based on that as Greg takes pull requests from me.
Usually this happens once or twice a kernel cycle.  Don't worry too much about
this. If it should affect a patch because some changes crossed I'll generally
fix it up whilst applying whichever gets applied second and ask the
authors to check I didn't make a mistake.

Joanthan
> 
> Regards,
> Subhajit Ghosh
> 

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

[Subhajit Ghosh]

>> Can you also help me out with the git tree I should use to format the
>> patches? As per my understanding it is the subsystem maintainer tree
>> and the main branch but the macros and functions which you have suggested
>> in other reviews are available in Linux mainline.
> 
> For a new driver it rarely matters and I'd advise simply using
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> which is the mainline tree.  Please base either on the previous
> release (currently 6.2) or rc1 of the current release (v6.3-rc1)
> if doing this.
> 
> If you need a feature that has only been applied in the same cycle, or
> are building on recent work that has been applied to the iio tree then
> for fixes you want:
> https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git fixes-togreg
> for new stuff you want:
> https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
> 
> The IIO tree routes through Greg KH's char-misc tree so will see the togreg
> branch move forwards to be based on that as Greg takes pull requests from me.
> Usually this happens once or twice a kernel cycle.  Don't worry too much about
> this. If it should affect a patch because some changes crossed I'll generally
> fix it up whilst applying whichever gets applied second and ask the
> authors to check I didn't make a mistake.
> 
> Joanthan
>>
>> Regards,
>> Subhajit Ghosh
>>
> 
Thank you for the detailed information. Appreciate you help.

Regards,
Subhajit Ghosh