[PATCH 0/2] Add support for ADUX1020 sensor

[PATCH 0/2] Add support for ADUX1020 sensor

[Manivannan Sadhasivam]

Hello,

This patchset adds initial IIO driver support for ADUX1020 Photometric
sensor from Analog Devices. This sensor is usable for multiple optical
measurement applications, including gesture control and proximity sensing.

This initial driver includes support for only proximity mode with event
based interrupt. The driver validation has been performed using Shiratech
LTE mezzanine [1] connected to 96Boards Dragonboard410c [2].

Thanks,
Mani

[1] https://www.96boards.org/product/shiratech-lte/
[2] https://www.96boards.org/product/dragonboard410c/

Manivannan Sadhasivam (2):
  dt-bindings: iio: light: Add binding for ADUX1020
  iio: light: Add support for ADUX1020 sensor

 .../bindings/iio/light/adux1020.txt           |  22 +
 drivers/iio/light/Kconfig                     |  11 +
 drivers/iio/light/Makefile                    |   1 +
 drivers/iio/light/adux1020.c                  | 783 ++++++++++++++++++
 4 files changed, 817 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/adux1020.txt
 create mode 100644 drivers/iio/light/adux1020.c

-- 
2.17.1

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

[Manivannan Sadhasivam]

Add devicetree binding for Analog Devices ADUX1020 Photometric
sensor.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 .../bindings/iio/light/adux1020.txt           | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/adux1020.txt

diff --git a/Documentation/devicetree/bindings/iio/light/adux1020.txt b/Documentation/devicetree/bindings/iio/light/adux1020.txt
new file mode 100644
index 000000000000..e896dda30e36
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/adux1020.txt
@@ -0,0 +1,22 @@
+Analog Devices ADUX1020 Photometric sensor
+
+Link to datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ADUX1020.pdf
+
+Required properties:
+
+ - compatible: should be "adi,adux1020"
+ - reg: the I2C address of the sensor
+
+Optional properties:
+
+ - interrupts: interrupt mapping for IRQ as documented in
+   Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
+
+Example:
+
+adux1020@64 {
+	compatible = "adi,adux1020";
+	reg = <0x64>;
+	interrupt-parent = <&msmgpio>;
+	interrupts = <24 IRQ_TYPE_LEVEL_HIGH>;
+};
-- 
2.17.1

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

[Ardelean, Alexandru]

On Mon, 2019-10-07 at 15:40 +0530, Manivannan Sadhasivam wrote:
> [External]
> 
> Add devicetree binding for Analog Devices ADUX1020 Photometric
> sensor.
> 

Hey,

Thanks for the patches.

This dt-binding docs is in text format.
dt-binding docs now need to be in YAML format.

Also, patches for dt-bindings docs usually come after the driver is added.
So, this patch should be the second in the series, not the first.

Alex

> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  .../bindings/iio/light/adux1020.txt           | 22 +++++++++++++++++++
>  1 file changed, 22 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/iio/light/adux1020.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/adux1020.txt
> b/Documentation/devicetree/bindings/iio/light/adux1020.txt
> new file mode 100644
> index 000000000000..e896dda30e36
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/adux1020.txt
> @@ -0,0 +1,22 @@
> +Analog Devices ADUX1020 Photometric sensor
> +
> +Link to datasheet: 
> https://www.analog.com/media/en/technical-documentation/data-sheets/ADUX1020.pdf
> +
> +Required properties:
> +
> + - compatible: should be "adi,adux1020"
> + - reg: the I2C address of the sensor
> +
> +Optional properties:
> +
> + - interrupts: interrupt mapping for IRQ as documented in
> +   Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> +
> +Example:
> +
> +adux1020@64 {
> +	compatible = "adi,adux1020";
> +	reg = <0x64>;
> +	interrupt-parent = <&msmgpio>;
> +	interrupts = <24 IRQ_TYPE_LEVEL_HIGH>;
> +};

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

[Manivannan Sadhasivam]

Hi Ardelean, 

On 7 October 2019 3:51:16 PM IST, "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
>On Mon, 2019-10-07 at 15:40 +0530, Manivannan Sadhasivam wrote:
>> [External]
>> 
>> Add devicetree binding for Analog Devices ADUX1020 Photometric
>> sensor.
>> 
>
>Hey,
>
>Thanks for the patches.
>
>This dt-binding docs is in text format.
>dt-binding docs now need to be in YAML format.
>

Sure. I can convert to YAML binding. 

>Also, patches for dt-bindings docs usually come after the driver is
>added.
>So, this patch should be the second in the series, not the first.
>

I don't think so. The convention is to put dt-bindings patch upfront for all subsystems. Not sure if IIO differs here. 

Thanks, 
Mani
>Alex
>
>> Signed-off-by: Manivannan Sadhasivam
><manivannan.sadhasivam@linaro.org>
>> ---
>>  .../bindings/iio/light/adux1020.txt           | 22
>+++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>  create mode 100644
>> Documentation/devicetree/bindings/iio/light/adux1020.txt
>> 
>> diff --git a/Documentation/devicetree/bindings/iio/light/adux1020.txt
>> b/Documentation/devicetree/bindings/iio/light/adux1020.txt
>> new file mode 100644
>> index 000000000000..e896dda30e36
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/light/adux1020.txt
>> @@ -0,0 +1,22 @@
>> +Analog Devices ADUX1020 Photometric sensor
>> +
>> +Link to datasheet: 
>>
>https://www.analog.com/media/en/technical-documentation/data-sheets/ADUX1020.pdf
>> +
>> +Required properties:
>> +
>> + - compatible: should be "adi,adux1020"
>> + - reg: the I2C address of the sensor
>> +
>> +Optional properties:
>> +
>> + - interrupts: interrupt mapping for IRQ as documented in
>> +  
>Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>> +
>> +Example:
>> +
>> +adux1020@64 {
>> +	compatible = "adi,adux1020";
>> +	reg = <0x64>;
>> +	interrupt-parent = <&msmgpio>;
>> +	interrupts = <24 IRQ_TYPE_LEVEL_HIGH>;
>> +};

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

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

[Ardelean, Alexandru]

On Mon, 2019-10-07 at 18:10 +0530, Manivannan Sadhasivam wrote:
> [External]
> 
> Hi Ardelean, 
> 
> On 7 October 2019 3:51:16 PM IST, "Ardelean, Alexandru" <
> alexandru.Ardelean@analog.com> wrote:
> > On Mon, 2019-10-07 at 15:40 +0530, Manivannan Sadhasivam wrote:
> > > [External]
> > > 
> > > Add devicetree binding for Analog Devices ADUX1020 Photometric
> > > sensor.
> > > 
> > 
> > Hey,
> > 
> > Thanks for the patches.
> > 
> > This dt-binding docs is in text format.
> > dt-binding docs now need to be in YAML format.
> > 
> 
> Sure. I can convert to YAML binding. 
> 
> > Also, patches for dt-bindings docs usually come after the driver is
> > added.
> > So, this patch should be the second in the series, not the first.
> > 
> 
> I don't think so. The convention is to put dt-bindings patch upfront for
> all subsystems. Not sure if IIO differs here. 

Now that you mention, I'm not sure either.
We typically sent the dt-bindings one last, so I assumed it was the
default.

> 
> Thanks, 
> Mani
> > Alex
> > 
> > > Signed-off-by: Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org>
> > > ---
> > >  .../bindings/iio/light/adux1020.txt           | 22
> > +++++++++++++++++++
> > >  1 file changed, 22 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/iio/light/adux1020.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/iio/light/adux1020.txt
> > > b/Documentation/devicetree/bindings/iio/light/adux1020.txt
> > > new file mode 100644
> > > index 000000000000..e896dda30e36
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/light/adux1020.txt
> > > @@ -0,0 +1,22 @@
> > > +Analog Devices ADUX1020 Photometric sensor
> > > +
> > > +Link to datasheet: 
> > > 
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ADUX1020.pdf
> > > +
> > > +Required properties:
> > > +
> > > + - compatible: should be "adi,adux1020"
> > > + - reg: the I2C address of the sensor
> > > +
> > > +Optional properties:
> > > +
> > > + - interrupts: interrupt mapping for IRQ as documented in
> > > +  
> > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> > > +
> > > +Example:
> > > +
> > > +adux1020@64 {
> > > +	compatible = "adi,adux1020";
> > > +	reg = <0x64>;
> > > +	interrupt-parent = <&msmgpio>;
> > > +	interrupts = <24 IRQ_TYPE_LEVEL_HIGH>;
> > > +};

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

[Jonathan Cameron]

On Mon, 7 Oct 2019 13:21:50 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Mon, 2019-10-07 at 18:10 +0530, Manivannan Sadhasivam wrote:
> > [External]
> > 
> > Hi Ardelean, 
> > 
> > On 7 October 2019 3:51:16 PM IST, "Ardelean, Alexandru" <  
> > alexandru.Ardelean@analog.com> wrote:
> > > On Mon, 2019-10-07 at 15:40 +0530, Manivannan Sadhasivam wrote:  
> > > > [External]
> > > > 
> > > > Add devicetree binding for Analog Devices ADUX1020 Photometric
> > > > sensor.
> > > >   
> > > 
> > > Hey,
> > > 
> > > Thanks for the patches.
> > > 
> > > This dt-binding docs is in text format.
> > > dt-binding docs now need to be in YAML format.
> > >   
> > 
> > Sure. I can convert to YAML binding. 
> >   
> > > Also, patches for dt-bindings docs usually come after the driver is
> > > added.
> > > So, this patch should be the second in the series, not the first.
> > >   
> > 
> > I don't think so. The convention is to put dt-bindings patch upfront for
> > all subsystems. Not sure if IIO differs here.   
> 
> Now that you mention, I'm not sure either.
> We typically sent the dt-bindings one last, so I assumed it was the
> default.

I don't care either way so never comment on it :)

Jonathan

> 
> > 
> > Thanks, 
> > Mani  
> > > Alex
> > >   
> > > > Signed-off-by: Manivannan Sadhasivam  
> > > <manivannan.sadhasivam@linaro.org>  
> > > > ---
> > > >  .../bindings/iio/light/adux1020.txt           | 22  
> > > +++++++++++++++++++  
> > > >  1 file changed, 22 insertions(+)
> > > >  create mode 100644
> > > > Documentation/devicetree/bindings/iio/light/adux1020.txt
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/iio/light/adux1020.txt
> > > > b/Documentation/devicetree/bindings/iio/light/adux1020.txt
> > > > new file mode 100644
> > > > index 000000000000..e896dda30e36
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/iio/light/adux1020.txt
> > > > @@ -0,0 +1,22 @@
> > > > +Analog Devices ADUX1020 Photometric sensor
> > > > +
> > > > +Link to datasheet: 
> > > >   
> > > https://www.analog.com/media/en/technical-documentation/data-sheets/ADUX1020.pdf  
> > > > +
> > > > +Required properties:
> > > > +
> > > > + - compatible: should be "adi,adux1020"
> > > > + - reg: the I2C address of the sensor
> > > > +
> > > > +Optional properties:
> > > > +
> > > > + - interrupts: interrupt mapping for IRQ as documented in
> > > > +    
> > > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt  
> > > > +
> > > > +Example:
> > > > +
> > > > +adux1020@64 {
> > > > +	compatible = "adi,adux1020";
> > > > +	reg = <0x64>;
> > > > +	interrupt-parent = <&msmgpio>;
> > > > +	interrupts = <24 IRQ_TYPE_LEVEL_HIGH>;
> > > > +};  

[PATCH 2/2] iio: light: Add support for ADUX1020 sensor

[Manivannan Sadhasivam]

Add initial support for Analog Devices ADUX1020 Photometric sensor.
Only proximity mode has been enabled for now.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/iio/light/Kconfig    |  11 +
 drivers/iio/light/Makefile   |   1 +
 drivers/iio/light/adux1020.c | 783 +++++++++++++++++++++++++++++++++++
 3 files changed, 795 insertions(+)
 create mode 100644 drivers/iio/light/adux1020.c

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 08d7e1ef2186..3f8c8689cd89 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -32,6 +32,17 @@ config ADJD_S311
 	  This driver can also be built as a module.  If so, the module
 	  will be called adjd_s311.
 
+config ADUX1020
+	tristate "ADUX1020 photometric sensor"
+	select REGMAP_I2C
+	depends on I2C
+	help
+	 Say Y here if you want to build a driver for the Analog Devices
+	 ADUX1020 photometric sensor.
+
+	 To compile this driver as a module, choose M here: the
+	 module will be called adux1020.
+
 config AL3320A
 	tristate "AL3320A ambient light sensor"
 	depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index 00d1f9b98f39..5d650ce46a40 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -6,6 +6,7 @@
 # When adding new entries keep the list in alphabetical order
 obj-$(CONFIG_ACPI_ALS)		+= acpi-als.o
 obj-$(CONFIG_ADJD_S311)		+= adjd_s311.o
+obj-$(CONFIG_ADUX1020)		+= adux1020.o
 obj-$(CONFIG_AL3320A)		+= al3320a.o
 obj-$(CONFIG_APDS9300)		+= apds9300.o
 obj-$(CONFIG_APDS9960)		+= apds9960.o
diff --git a/drivers/iio/light/adux1020.c b/drivers/iio/light/adux1020.c
new file mode 100644
index 000000000000..d0b76e5b44f1
--- /dev/null
+++ b/drivers/iio/light/adux1020.c
@@ -0,0 +1,783 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * adux1020.c - Support for Analog Devices ADUX1020 photometric sensor
+ *
+ * Copyright (C) 2019 Linaro Ltd.
+ * Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
+ *
+ * TODO: Triggered buffer support
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/events.h>
+
+#define ADUX1020_REGMAP_NAME		"adux1020_regmap"
+#define ADUX1020_DRV_NAME		"adux1020"
+
+/* System registers */
+#define ADUX1020_REG_CHIP_ID		0x08
+#define ADUX1020_REG_SLAVE_ADDRESS	0x09
+
+#define ADUX1020_REG_SW_RESET		0x0f
+#define ADUX1020_REG_INT_ENABLE		0x1c
+#define ADUX1020_REG_INT_POLARITY	0x1d
+#define ADUX1020_REG_PROX_TH_ON1	0x2a
+#define ADUX1020_REG_PROX_TH_OFF1	0x2b
+#define	ADUX1020_REG_PROX_TYPE		0x2f
+#define	ADUX1020_REG_TEST_MODES_3	0x32
+#define	ADUX1020_REG_FORCE_MODE		0x33
+#define	ADUX1020_REG_FREQUENCY		0x40
+#define ADUX1020_REG_LED_CURRENT	0x41
+#define	ADUX1020_REG_OP_MODE		0x45
+#define	ADUX1020_REG_INT_MASK		0x48
+#define	ADUX1020_REG_INT_STATUS		0x49
+#define	ADUX1020_REG_DATA_BUFFER	0x60
+
+/* Chip ID bits */
+#define ADUX1020_CHIP_ID_MASK		GENMASK(11, 0)
+#define ADUX1020_CHIP_ID		0x03fc
+
+#define ADUX1020_MODE_OUT_SHIFT		4
+#define ADUX1020_MODE_OUT_PROX_I	1
+#define ADUX1020_MODE_OUT_PROX_XY	3
+
+#define ADUX1020_SW_RESET		BIT(1)
+#define ADUX1020_FIFO_FLUSH		BIT(15)
+#define ADUX1020_OP_MODE_MASK		GENMASK(3, 0)
+#define ADUX1020_DATA_OUT_MODE_MASK	GENMASK(7, 4)
+
+#define ADUX1020_MODE_INT_MASK		GENMASK(7, 0)
+#define ADUX1020_INT_ENABLE		0x2096
+#define ADUX1020_INT_DISABLE		0x2090
+#define ADUX1020_PROX_INT_ENABLE	0x00f0
+#define ADUX1020_PROX_ON1_INT		BIT(0)
+#define ADUX1020_PROX_OFF1_INT		BIT(1)
+#define ADUX1020_FIFO_INT_ENABLE	0x7f
+#define ADUX1020_MODE_INT_DISABLE	0xff
+#define ADUX1020_MODE_INT_STATUS_MASK	GENMASK(7, 0)
+#define ADUX1020_FIFO_STATUS_MASK	GENMASK(15, 8)
+#define ADUX1020_PROX_TYPE		BIT(15)
+
+#define ADUX1020_INT_PROX_ON1		BIT(0)
+#define ADUX1020_INT_PROX_OFF1		BIT(1)
+
+#define	ADUX1020_FORCE_CLOCK_ON		0x0f4f
+#define	ADUX1020_FORCE_CLOCK_RESET	0x0040
+#define ADUX1020_ACTIVE_4_STATE		0x0008
+
+#define ADUX1020_PROX_FREQ_MASK		GENMASK(7, 4)
+#define ADUX1020_PROX_FREQ_SHIFT	4
+
+#define ADUX1020_LED_CURRENT_MASK	GENMASK(3, 0)
+#define ADUX1020_LED_PIREF_EN		BIT(12)
+
+/* Operating modes */
+enum adux1020_op_modes {
+	ADUX1020_MODE_STANDBY,
+	ADUX1020_MODE_PROX_I,
+	ADUX1020_MODE_PROX_XY,
+	ADUX1020_MODE_GEST,
+	ADUX1020_MODE_SAMPLE,
+	ADUX1020_MODE_FORCE = 0x0e,
+	ADUX1020_MODE_IDLE = 0x0f,
+};
+
+struct adux1020_data {
+	struct i2c_client *client;
+	struct iio_dev *indio_dev;
+	struct mutex lock;
+	struct regmap *regmap;
+};
+
+struct adux1020_mode_data {
+	u8 bytes;
+	u8 buf_len;
+	u16 int_en;
+};
+
+static const struct adux1020_mode_data adux1020_modes[] = {
+	[ADUX1020_MODE_PROX_I] = {
+		.bytes = 2,
+		.buf_len = 1,
+		.int_en = ADUX1020_PROX_INT_ENABLE,
+	},
+};
+
+static const struct regmap_config adux1020_regmap_config = {
+	.name = ADUX1020_REGMAP_NAME,
+	.reg_bits = 8,
+	.val_bits = 16,
+	.max_register = 0x6F,
+	.cache_type = REGCACHE_NONE,
+};
+
+static const int adux1020_def_conf[][2] = {
+	{ 0x000c, 0x000f },
+	{ 0x0010, 0x1010 },
+	{ 0x0011, 0x004c },
+	{ 0x0012, 0x5f0c },
+	{ 0x0013, 0xada5 },
+	{ 0x0014, 0x0080 },
+	{ 0x0015, 0x0000 },
+	{ 0x0016, 0x0600 },
+	{ 0x0017, 0x0000 },
+	{ 0x0018, 0x2693 },
+	{ 0x0019, 0x0004 },
+	{ 0x001a, 0x4280 },
+	{ 0x001b, 0x0060 },
+	{ 0x001c, 0x2094 },
+	{ 0x001d, 0x0020 },
+	{ 0x001e, 0x0001 },
+	{ 0x001f, 0x0100 },
+	{ 0x0020, 0x0320 },
+	{ 0x0021, 0x0A13 },
+	{ 0x0022, 0x0320 },
+	{ 0x0023, 0x0113 },
+	{ 0x0024, 0x0000 },
+	{ 0x0025, 0x2412 },
+	{ 0x0026, 0x2412 },
+	{ 0x0027, 0x0022 },
+	{ 0x0028, 0x0000 },
+	{ 0x0029, 0x0300 },
+	{ 0x002a, 0x0700 },
+	{ 0x002b, 0x0600 },
+	{ 0x002c, 0x6000 },
+	{ 0x002d, 0x4000 },
+	{ 0x002e, 0x0000 },
+	{ 0x002f, 0x0000 },
+	{ 0x0030, 0x0000 },
+	{ 0x0031, 0x0000 },
+	{ 0x0032, 0x0040 },
+	{ 0x0033, 0x0008 },
+	{ 0x0034, 0xE400 },
+	{ 0x0038, 0x8080 },
+	{ 0x0039, 0x8080 },
+	{ 0x003a, 0x2000 },
+	{ 0x003b, 0x1f00 },
+	{ 0x003c, 0x2000 },
+	{ 0x003d, 0x2000 },
+	{ 0x003e, 0x0000 },
+	{ 0x0040, 0x8069 },
+	{ 0x0041, 0x1f2f },
+	{ 0x0042, 0x4000 },
+	{ 0x0043, 0x0000 },
+	{ 0x0044, 0x0008 },
+	{ 0x0046, 0x0000 },
+	{ 0x0048, 0x00ef },
+	{ 0x0049, 0x0000 },
+	{ 0x0045, 0x0000 },
+};
+
+static const int adux1020_rate[][2] = {
+	{ 0, 100000 },
+	{ 0, 200000 },
+	{ 0, 500000 },
+	{ 1, 0 },
+	{ 2, 0 },
+	{ 5, 0 },
+	{ 10, 0 },
+	{ 20, 0 },
+	{ 50, 0 },
+	{ 100, 0 },
+	{ 190, 0 },
+	{ 450, 0 },
+	{ 820, 0 },
+	{ 1400, 0 },
+};
+
+static const int adux1020_led_current[][2] = {
+	{ 0, 25000 },
+	{ 0, 40000 },
+	{ 0, 55000 },
+	{ 0, 70000 },
+	{ 0, 85000 },
+	{ 0, 100000 },
+	{ 0, 115000 },
+	{ 0, 130000 },
+	{ 0, 145000 },
+	{ 0, 160000 },
+	{ 0, 175000 },
+	{ 0, 190000 },
+	{ 0, 205000 },
+	{ 0, 220000 },
+	{ 0, 235000 },
+	{ 0, 250000 },
+};
+
+static void adux1020_flush_fifo(struct adux1020_data *data)
+{
+	/* Force Idle mode */
+	regmap_write(data->regmap, ADUX1020_REG_FORCE_MODE,
+		     ADUX1020_ACTIVE_4_STATE);
+	regmap_update_bits(data->regmap, ADUX1020_REG_OP_MODE,
+			   ADUX1020_OP_MODE_MASK, ADUX1020_MODE_FORCE);
+	regmap_update_bits(data->regmap, ADUX1020_REG_OP_MODE,
+			   ADUX1020_OP_MODE_MASK, ADUX1020_MODE_IDLE);
+
+	/* Flush FIFO */
+	regmap_write(data->regmap, ADUX1020_REG_TEST_MODES_3,
+		     ADUX1020_FORCE_CLOCK_ON);
+	regmap_write(data->regmap, ADUX1020_REG_INT_STATUS,
+		     ADUX1020_FIFO_FLUSH);
+	regmap_write(data->regmap, ADUX1020_REG_TEST_MODES_3,
+		     ADUX1020_FORCE_CLOCK_RESET);
+}
+
+static int adux1020_read_fifo(struct adux1020_data *data, u16 *buf, u8 buf_len)
+{
+	int i, ret = -EINVAL;
+	unsigned int regval;
+
+	/* Enable 32MHz clock */
+	regmap_write(data->regmap, ADUX1020_REG_TEST_MODES_3,
+		     ADUX1020_FORCE_CLOCK_ON);
+
+	for (i = 0; i < buf_len; i++) {
+		ret = regmap_read(data->regmap, ADUX1020_REG_DATA_BUFFER,
+				      &regval);
+		if (ret < 0)
+			goto err_out;
+
+		buf[i] = regval;
+	}
+
+	/* Set 32MHz clock to be controlled by internal state machine */
+	regmap_write(data->regmap, ADUX1020_REG_TEST_MODES_3,
+		     ADUX1020_FORCE_CLOCK_RESET);
+
+err_out:
+	return ret;
+}
+
+static void adux1020_set_mode(struct adux1020_data *data,
+			      enum adux1020_op_modes mode)
+{
+	/* Switch to standby mode before changing the mode */
+	regmap_write(data->regmap, ADUX1020_REG_OP_MODE, ADUX1020_MODE_STANDBY);
+
+	/* Set data out and switch to the desired mode */
+	if (mode == ADUX1020_MODE_PROX_I) {
+		regmap_update_bits(data->regmap, ADUX1020_REG_OP_MODE,
+			ADUX1020_DATA_OUT_MODE_MASK,
+			ADUX1020_MODE_OUT_PROX_I << ADUX1020_MODE_OUT_SHIFT);
+		regmap_update_bits(data->regmap, ADUX1020_REG_OP_MODE,
+			ADUX1020_OP_MODE_MASK, ADUX1020_MODE_PROX_I);
+	}
+}
+
+static int adux1020_measure(struct adux1020_data *data,
+			    enum adux1020_op_modes mode,
+			    u16 *val)
+{
+	int ret, tries = 50;
+	unsigned int status;
+
+	mutex_lock(&data->lock);
+
+	/* Disable INT pin as polling is going to be used */
+	regmap_write(data->regmap, ADUX1020_REG_INT_ENABLE,
+		     ADUX1020_INT_DISABLE);
+
+	/* Enable mode interrupt */
+	regmap_update_bits(data->regmap, ADUX1020_REG_INT_MASK,
+			   ADUX1020_MODE_INT_MASK,
+			   adux1020_modes[mode].int_en);
+
+	while (tries--) {
+		ret = regmap_read(data->regmap, ADUX1020_REG_INT_STATUS,
+				  &status);
+		if (ret < 0)
+			goto fail;
+
+		status &= ADUX1020_FIFO_STATUS_MASK;
+		if (status >= adux1020_modes[mode].bytes)
+			break;
+		msleep(20);
+	}
+
+	if (tries < 0) {
+		ret = -EIO;
+		goto fail;
+	}
+
+	ret = adux1020_read_fifo(data, val, adux1020_modes[mode].buf_len);
+	if (ret < 0)
+		goto fail;
+
+	/* Clear mode interrupt */
+	regmap_write(data->regmap, ADUX1020_REG_INT_STATUS,
+			   (~adux1020_modes[mode].int_en));
+	/* Disable mode interrupts */
+	regmap_update_bits(data->regmap, ADUX1020_REG_INT_MASK,
+			   ADUX1020_MODE_INT_MASK, ADUX1020_MODE_INT_DISABLE);
+
+fail:
+	mutex_unlock(&data->lock);
+
+	return ret;
+}
+
+static int adux1020_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int *val, int *val2, long mask)
+{
+	struct adux1020_data *data = iio_priv(indio_dev);
+	u16 buf[3];
+	int ret = -EINVAL;
+	unsigned int regval;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		switch (chan->type) {
+		case IIO_PROXIMITY:
+			adux1020_set_mode(data, ADUX1020_MODE_PROX_I);
+			ret = adux1020_measure(data, ADUX1020_MODE_PROX_I, buf);
+			if (ret < 0)
+				return ret;
+
+			*val = buf[0];
+			ret = IIO_VAL_INT;
+			break;
+		default:
+			break;
+		}
+		break;
+	case IIO_CHAN_INFO_PROCESSED:
+		switch (chan->type) {
+		case IIO_CURRENT:
+			ret = regmap_read(data->regmap,
+					  ADUX1020_REG_LED_CURRENT, &regval);
+			if (ret < 0)
+				return ret;
+
+			regval = regval & ADUX1020_LED_CURRENT_MASK;
+
+			*val = adux1020_led_current[regval][0];
+			*val2 = adux1020_led_current[regval][1];
+
+			ret = IIO_VAL_INT_PLUS_MICRO;
+			break;
+		default:
+			break;
+		}
+		break;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		switch (chan->type) {
+		case IIO_PROXIMITY:
+			ret = regmap_read(data->regmap, ADUX1020_REG_FREQUENCY,
+					  &regval);
+			if (ret < 0)
+				return ret;
+
+			regval = (regval & ADUX1020_PROX_FREQ_MASK) >>
+				  ADUX1020_PROX_FREQ_SHIFT;
+
+			*val = adux1020_rate[regval][0];
+			*val2 = adux1020_rate[regval][1];
+
+			ret = IIO_VAL_INT_PLUS_MICRO;
+			break;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+};
+
+static int adux1020_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	struct adux1020_data *data = iio_priv(indio_dev);
+	int i, ret = -EINVAL;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		if (chan->type == IIO_PROXIMITY) {
+			for (i = 0; i < ARRAY_SIZE(adux1020_rate); i++) {
+				if ((val == adux1020_rate[i][0]) &&
+				     (val2 == adux1020_rate[i][1])) {
+					ret = regmap_update_bits(data->regmap,
+						ADUX1020_REG_FREQUENCY,
+						ADUX1020_PROX_FREQ_MASK,
+						i << ADUX1020_PROX_FREQ_SHIFT);
+				}
+			}
+		}
+		break;
+	case IIO_CHAN_INFO_PROCESSED:
+		if (chan->type == IIO_CURRENT) {
+			for (i = 0; i < ARRAY_SIZE(adux1020_led_current); i++) {
+				if ((val == adux1020_led_current[i][0]) &&
+				     (val2 == adux1020_led_current[i][1])) {
+					ret = regmap_update_bits(data->regmap,
+						ADUX1020_REG_LED_CURRENT,
+						ADUX1020_LED_CURRENT_MASK, i);
+				}
+			}
+		}
+		break;
+	default:
+		break;
+	}
+
+	return ret;
+}
+
+static int adux1020_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 adux1020_data *data = iio_priv(indio_dev);
+
+	regmap_write(data->regmap, ADUX1020_REG_INT_ENABLE,
+		     ADUX1020_INT_ENABLE);
+
+	regmap_write(data->regmap, ADUX1020_REG_INT_POLARITY, 0);
+
+	switch (chan->type) {
+	case IIO_PROXIMITY:
+		if (dir == IIO_EV_DIR_RISING) {
+			regmap_update_bits(data->regmap, ADUX1020_REG_INT_MASK,
+					   ADUX1020_PROX_ON1_INT,
+					   state ? 0 : ADUX1020_PROX_ON1_INT);
+		} else {
+			regmap_update_bits(data->regmap, ADUX1020_REG_INT_MASK,
+					   ADUX1020_PROX_OFF1_INT,
+					   state ? 0 : ADUX1020_PROX_OFF1_INT);
+		}
+
+		/*
+		 * Trigger proximity interrupt when the intensity is above
+		 * or below threshold
+		 */
+		regmap_update_bits(data->regmap, ADUX1020_REG_PROX_TYPE,
+				   ADUX1020_PROX_TYPE, ADUX1020_PROX_TYPE);
+
+		/* Set proximity mode */
+		adux1020_set_mode(data, ADUX1020_MODE_PROX_I);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int adux1020_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 adux1020_data *data = iio_priv(indio_dev);
+	int ret, mask;
+	unsigned int regval;
+
+	switch (chan->type) {
+	case IIO_PROXIMITY:
+		if (dir == IIO_EV_DIR_RISING)
+			mask = ADUX1020_PROX_ON1_INT;
+		else
+			mask = ADUX1020_PROX_OFF1_INT;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = regmap_read(data->regmap, ADUX1020_REG_INT_MASK, &regval);
+	if (ret < 0)
+		return ret;
+
+	return !(regval & mask);
+}
+
+static int adux1020_read_thresh(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 adux1020_data *data = iio_priv(indio_dev);
+	u8 reg;
+	int ret;
+	unsigned int regval;
+
+	switch (chan->type) {
+	case IIO_PROXIMITY:
+		if (dir == IIO_EV_DIR_RISING)
+			reg = ADUX1020_REG_PROX_TH_ON1;
+		else
+			reg = ADUX1020_REG_PROX_TH_OFF1;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = regmap_read(data->regmap, reg, &regval);
+	if (ret < 0)
+		return ret;
+
+	*val = regval;
+
+	return IIO_VAL_INT;
+}
+
+static int adux1020_write_thresh(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 adux1020_data *data = iio_priv(indio_dev);
+	u8 reg;
+
+	switch (chan->type) {
+	case IIO_PROXIMITY:
+		if (dir == IIO_EV_DIR_RISING)
+			reg = ADUX1020_REG_PROX_TH_ON1;
+		else
+			reg = ADUX1020_REG_PROX_TH_OFF1;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* Full scale threshold value is 0-65535  */
+	if (val < 0 || val > 65535)
+		return -EINVAL;
+
+	return regmap_write(data->regmap, reg, val);
+}
+
+static const struct iio_event_spec adux1020_proximity_event[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+			BIT(IIO_EV_INFO_ENABLE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+			BIT(IIO_EV_INFO_ENABLE),
+	},
+};
+
+static const struct iio_chan_spec adux1020_channels[] = {
+	{
+		.type = IIO_PROXIMITY,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.event_spec = adux1020_proximity_event,
+		.num_event_specs = ARRAY_SIZE(adux1020_proximity_event),
+	},
+	{
+		.type = IIO_CURRENT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+		.extend_name = "led",
+	},
+};
+
+static IIO_CONST_ATTR(sampling_frequency_available,
+		      "0.1 0.2 0.5 1 2 5 10 20 50 100 190 450 820 1400");
+
+static struct attribute *adux1020_attributes[] = {
+	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group adux1020_attribute_group = {
+	.attrs = adux1020_attributes,
+};
+
+static const struct iio_info adux1020_info = {
+	.attrs = &adux1020_attribute_group,
+	.read_raw = adux1020_read_raw,
+	.write_raw = adux1020_write_raw,
+	.read_event_config = adux1020_read_event_config,
+	.write_event_config = adux1020_write_event_config,
+	.read_event_value = adux1020_read_thresh,
+	.write_event_value = adux1020_write_thresh,
+};
+
+static irqreturn_t adux1020_interrupt_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct adux1020_data *data = iio_priv(indio_dev);
+	int ret, status;
+
+	ret = regmap_read(data->regmap, ADUX1020_REG_INT_STATUS, &status);
+	if (ret < 0)
+		return ret;
+
+	status &= ADUX1020_MODE_INT_STATUS_MASK;
+
+	if (status & ADUX1020_INT_PROX_ON1) {
+		iio_push_event(indio_dev,
+				IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
+					IIO_EV_TYPE_THRESH,
+					IIO_EV_DIR_RISING),
+				iio_get_time_ns(indio_dev));
+	}
+
+	if (status & ADUX1020_INT_PROX_OFF1) {
+		iio_push_event(indio_dev,
+				IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
+					IIO_EV_TYPE_THRESH,
+					IIO_EV_DIR_FALLING),
+				iio_get_time_ns(indio_dev));
+	}
+
+	regmap_update_bits(data->regmap, ADUX1020_REG_INT_STATUS,
+			   ADUX1020_MODE_INT_MASK, status);
+
+	return IRQ_HANDLED;
+}
+
+static int adux1020_chip_init(struct adux1020_data *data)
+{
+	struct i2c_client *client = data->client;
+	int ret, i;
+	unsigned int val;
+
+	ret = regmap_read(data->regmap, ADUX1020_REG_CHIP_ID, &val);
+	if (ret < 0)
+		return ret;
+
+	val &= ADUX1020_CHIP_ID_MASK;
+
+	if (val != ADUX1020_CHIP_ID) {
+		dev_err(&client->dev, "invalid chip id 0x%04x\n", val);
+		return -ENODEV;
+	};
+
+	dev_dbg(&client->dev, "Detected ADUX1020 with chip id: 0x%04x\n", val);
+
+	/* Perform software reset */
+	regmap_update_bits(data->regmap, ADUX1020_REG_SW_RESET,
+			   ADUX1020_SW_RESET, ADUX1020_SW_RESET);
+
+	/* Load default configuration */
+	for (i = 0; i < ARRAY_SIZE(adux1020_def_conf); i++)
+		regmap_write(data->regmap, adux1020_def_conf[i][0],
+			     adux1020_def_conf[i][1]);
+
+	adux1020_flush_fifo(data);
+
+	/* Use LED_IREF for proximity mode */
+	regmap_update_bits(data->regmap, ADUX1020_REG_LED_CURRENT,
+			   ADUX1020_LED_PIREF_EN, 0);
+
+	/* Mask all interrupts */
+	regmap_update_bits(data->regmap, ADUX1020_REG_INT_MASK,
+			   ADUX1020_MODE_INT_MASK, ADUX1020_MODE_INT_DISABLE);
+
+	return 0;
+}
+
+static int adux1020_probe(struct i2c_client *client,
+			  const struct i2c_device_id *id)
+{
+	struct adux1020_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->info = &adux1020_info;
+	indio_dev->name = ADUX1020_DRV_NAME;
+	indio_dev->channels = adux1020_channels;
+	indio_dev->num_channels = ARRAY_SIZE(adux1020_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	data = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+
+	data->regmap = devm_regmap_init_i2c(client, &adux1020_regmap_config);
+	if (IS_ERR(data->regmap)) {
+		dev_err(&client->dev, "regmap initialization failed.\n");
+		return PTR_ERR(data->regmap);
+	}
+
+	data->client = client;
+	data->indio_dev = indio_dev;
+	mutex_init(&data->lock);
+
+	ret = adux1020_chip_init(data);
+	if (ret)
+		goto err_out;
+
+	if (client->irq) {
+		ret = devm_request_threaded_irq(&client->dev, client->irq,
+				NULL, adux1020_interrupt_handler,
+				IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
+				ADUX1020_DRV_NAME, indio_dev);
+		if (ret) {
+			dev_err(&client->dev, "irq request error %d\n", -ret);
+			goto err_out;
+		}
+	}
+
+	ret = iio_device_register(indio_dev);
+	if (ret) {
+		dev_err(&client->dev, "Failed to register IIO device\n");
+		goto err_out;
+	}
+
+	return 0;
+
+err_out:
+	return ret;
+}
+
+static int adux1020_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+
+	iio_device_unregister(indio_dev);
+
+	return 0;
+}
+
+static const struct i2c_device_id adux1020_id[] = {
+	{ "adux1020", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, adux1020_id);
+
+static const struct of_device_id adux1020_of_match[] = {
+	{ .compatible = "adi,adux1020" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, adux1020_of_match);
+
+static struct i2c_driver adux1020_driver = {
+	.driver = {
+		.name	= ADUX1020_DRV_NAME,
+		.of_match_table = adux1020_of_match,
+	},
+	.probe		= adux1020_probe,
+	.remove		= adux1020_remove,
+	.id_table	= adux1020_id,
+};
+module_i2c_driver(adux1020_driver);
+
+MODULE_AUTHOR("Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>");
+MODULE_DESCRIPTION("ADUX1020 photometric sensor");
+MODULE_LICENSE("GPL");
-- 
2.17.1

Re: [PATCH 2/2] iio: light: Add support for ADUX1020 sensor

[Ardelean, Alexandru]

On Mon, 2019-10-07 at 15:40 +0530, Manivannan Sadhasivam wrote:
> [External]
> 

Hey,

Comments inline.

I thought I sent an initial review, but seems to have gotten lost [maybe in
my email client].
Oh well. I managed to re-do it anyway.

I tried to group them this time.

The more prominent part is [3]; this driver needs a bit more error checking
on regmap_() returns.

Generally some notes:
- Is there a need to implement the 32Khz or 32Mhz clock calibration
routines on startup? Some drivers need this, some don't/
- From the functional diagram, it looks like maybe the VREF would be needed
to be hooked via a regulator framework; but this could be done later
- Just curios here: there is gesture mode as well; will that be implemented
later? Or will there be other modes implemented?

If I remember anything else I may come back with a reply.

Thanks
Alex

> Add initial support for Analog Devices ADUX1020 Photometric sensor.
> Only proximity mode has been enabled for now.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/iio/light/Kconfig    |  11 +
>  drivers/iio/light/Makefile   |   1 +
>  drivers/iio/light/adux1020.c | 783 +++++++++++++++++++++++++++++++++++

Does MAINTAINERS need updating as well?

>  3 files changed, 795 insertions(+)
>  create mode 100644 drivers/iio/light/adux1020.c
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 08d7e1ef2186..3f8c8689cd89 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -32,6 +32,17 @@ config ADJD_S311
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called adjd_s311.
>  
> +config ADUX1020
> +	tristate "ADUX1020 photometric sensor"
> +	select REGMAP_I2C
> +	depends on I2C
> +	help
> +	 Say Y here if you want to build a driver for the Analog Devices
> +	 ADUX1020 photometric sensor.
> +
> +	 To compile this driver as a module, choose M here: the
> +	 module will be called adux1020.
> +
>  config AL3320A
>  	tristate "AL3320A ambient light sensor"
>  	depends on I2C
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index 00d1f9b98f39..5d650ce46a40 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -6,6 +6,7 @@
>  # When adding new entries keep the list in alphabetical order
>  obj-$(CONFIG_ACPI_ALS)		+= acpi-als.o
>  obj-$(CONFIG_ADJD_S311)		+= adjd_s311.o
> +obj-$(CONFIG_ADUX1020)		+= adux1020.o
>  obj-$(CONFIG_AL3320A)		+= al3320a.o
>  obj-$(CONFIG_APDS9300)		+= apds9300.o
>  obj-$(CONFIG_APDS9960)		+= apds9960.o
> diff --git a/drivers/iio/light/adux1020.c b/drivers/iio/light/adux1020.c
> new file mode 100644
> index 000000000000..d0b76e5b44f1
> --- /dev/null
> +++ b/drivers/iio/light/adux1020.c
> @@ -0,0 +1,783 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * adux1020.c - Support for Analog Devices ADUX1020 photometric sensor

Maybe drop the adux1020.c part?
I think something like this should be sufficient:
"Analog Devices ADUX1020 photometric sensor"

> + *
> + * Copyright (C) 2019 Linaro Ltd.
> + * Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> + *
> + * TODO: Triggered buffer support

Maybe drop the TODO?
It's not needed for mainline.

> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/events.h>
> +
> +#define ADUX1020_REGMAP_NAME		"adux1020_regmap"
> +#define ADUX1020_DRV_NAME		"adux1020"
> +
> +/* System registers */
> +#define ADUX1020_REG_CHIP_ID		0x08
> +#define ADUX1020_REG_SLAVE_ADDRESS	0x09
> +
> +#define ADUX1020_REG_SW_RESET		0x0f
> +#define ADUX1020_REG_INT_ENABLE		0x1c
> +#define ADUX1020_REG_INT_POLARITY	0x1d
> +#define ADUX1020_REG_PROX_TH_ON1	0x2a
> +#define ADUX1020_REG_PROX_TH_OFF1	0x2b
> +#define	ADUX1020_REG_PROX_TYPE		0x2f
> +#define	ADUX1020_REG_TEST_MODES_3	0x32
> +#define	ADUX1020_REG_FORCE_MODE		0x33
> +#define	ADUX1020_REG_FREQUENCY		0x40
> +#define ADUX1020_REG_LED_CURRENT	0x41
> +#define	ADUX1020_REG_OP_MODE		0x45
> +#define	ADUX1020_REG_INT_MASK		0x48
> +#define	ADUX1020_REG_INT_STATUS		0x49
> +#define	ADUX1020_REG_DATA_BUFFER	0x60
> +
> +/* Chip ID bits */
> +#define ADUX1020_CHIP_ID_MASK		GENMASK(11, 0)
> +#define ADUX1020_CHIP_ID		0x03fc
> +
> +#define ADUX1020_MODE_OUT_SHIFT		4

I'm seeing a few _SHIFT macros.
Maybe use the FIELD_PREP() macro where possible? [1]

> +#define ADUX1020_MODE_OUT_PROX_I	1
> +#define ADUX1020_MODE_OUT_PROX_XY	3
> +
> +#define ADUX1020_SW_RESET		BIT(1)
> +#define ADUX1020_FIFO_FLUSH		BIT(15)
> +#define ADUX1020_OP_MODE_MASK		GENMASK(3, 0)
> +#define ADUX1020_DATA_OUT_MODE_MASK	GENMASK(7, 4)
> +
> +#define ADUX1020_MODE_INT_MASK		GENMASK(7, 0)
> +#define ADUX1020_INT_ENABLE		0x2096
> +#define ADUX1020_INT_DISABLE		0x2090
> +#define ADUX1020_PROX_INT_ENABLE	0x00f0
> +#define ADUX1020_PROX_ON1_INT		BIT(0)
> +#define ADUX1020_PROX_OFF1_INT		BIT(1)
> +#define ADUX1020_FIFO_INT_ENABLE	0x7f
> +#define ADUX1020_MODE_INT_DISABLE	0xff
> +#define ADUX1020_MODE_INT_STATUS_MASK	GENMASK(7, 0)
> +#define ADUX1020_FIFO_STATUS_MASK	GENMASK(15, 8)
> +#define ADUX1020_PROX_TYPE		BIT(15)
> +
> +#define ADUX1020_INT_PROX_ON1		BIT(0)
> +#define ADUX1020_INT_PROX_OFF1		BIT(1)
> +
> +#define	ADUX1020_FORCE_CLOCK_ON		0x0f4f
> +#define	ADUX1020_FORCE_CLOCK_RESET	0x0040

nitpick: indentation seems inconsistent here

> +#define ADUX1020_ACTIVE_4_STATE		0x0008
> +
> +#define ADUX1020_PROX_FREQ_MASK		GENMASK(7, 4)
> +#define ADUX1020_PROX_FREQ_SHIFT	4

Same as [1]. Maybe use FIELD_PREP() together with ADUX1020_PROX_FREQ_MASK?
See other examples, but we sometimes use them as:

#define ADUX_PROX_FREQ_SET(x)    FIELD_PREP(ADUX1020_PROX_FREQ_MASK, (x))

> +
> +#define ADUX1020_LED_CURRENT_MASK	GENMASK(3, 0)
> +#define ADUX1020_LED_PIREF_EN		BIT(12)
> +
> +/* Operating modes */
> +enum adux1020_op_modes {
> +	ADUX1020_MODE_STANDBY,
> +	ADUX1020_MODE_PROX_I,
> +	ADUX1020_MODE_PROX_XY,
> +	ADUX1020_MODE_GEST,
> +	ADUX1020_MODE_SAMPLE,
> +	ADUX1020_MODE_FORCE = 0x0e,
> +	ADUX1020_MODE_IDLE = 0x0f,
> +};
> +
> +struct adux1020_data {
> +	struct i2c_client *client;
> +	struct iio_dev *indio_dev;
> +	struct mutex lock;
> +	struct regmap *regmap;
> +};
> +
> +struct adux1020_mode_data {
> +	u8 bytes;
> +	u8 buf_len;
> +	u16 int_en;
> +};
> +
> +static const struct adux1020_mode_data adux1020_modes[] = {
> +	[ADUX1020_MODE_PROX_I] = {
> +		.bytes = 2,
> +		.buf_len = 1,
> +		.int_en = ADUX1020_PROX_INT_ENABLE,
> +	},
> +};
> +
> +static const struct regmap_config adux1020_regmap_config = {
> +	.name = ADUX1020_REGMAP_NAME,
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.max_register = 0x6F,
> +	.cache_type = REGCACHE_NONE,
> +};
> +
> +static const int adux1020_def_conf[][2] = {

Maybe use "struct reg_sequence" here?[2]
You could then use "regmap_multi_reg_write()"

> +	{ 0x000c, 0x000f },
> +	{ 0x0010, 0x1010 },
> +	{ 0x0011, 0x004c },
> +	{ 0x0012, 0x5f0c },
> +	{ 0x0013, 0xada5 },
> +	{ 0x0014, 0x0080 },
> +	{ 0x0015, 0x0000 },
> +	{ 0x0016, 0x0600 },
> +	{ 0x0017, 0x0000 },
> +	{ 0x0018, 0x2693 },
> +	{ 0x0019, 0x0004 },
> +	{ 0x001a, 0x4280 },
> +	{ 0x001b, 0x0060 },
> +	{ 0x001c, 0x2094 },
> +	{ 0x001d, 0x0020 },
> +	{ 0x001e, 0x0001 },
> +	{ 0x001f, 0x0100 },
> +	{ 0x0020, 0x0320 },
> +	{ 0x0021, 0x0A13 },
> +	{ 0x0022, 0x0320 },
> +	{ 0x0023, 0x0113 },
> +	{ 0x0024, 0x0000 },
> +	{ 0x0025, 0x2412 },
> +	{ 0x0026, 0x2412 },
> +	{ 0x0027, 0x0022 },
> +	{ 0x0028, 0x0000 },
> +	{ 0x0029, 0x0300 },
> +	{ 0x002a, 0x0700 },
> +	{ 0x002b, 0x0600 },
> +	{ 0x002c, 0x6000 },
> +	{ 0x002d, 0x4000 },
> +	{ 0x002e, 0x0000 },
> +	{ 0x002f, 0x0000 },
> +	{ 0x0030, 0x0000 },
> +	{ 0x0031, 0x0000 },
> +	{ 0x0032, 0x0040 },
> +	{ 0x0033, 0x0008 },
> +	{ 0x0034, 0xE400 },
> +	{ 0x0038, 0x8080 },
> +	{ 0x0039, 0x8080 },
> +	{ 0x003a, 0x2000 },
> +	{ 0x003b, 0x1f00 },
> +	{ 0x003c, 0x2000 },
> +	{ 0x003d, 0x2000 },
> +	{ 0x003e, 0x0000 },
> +	{ 0x0040, 0x8069 },
> +	{ 0x0041, 0x1f2f },
> +	{ 0x0042, 0x4000 },
> +	{ 0x0043, 0x0000 },
> +	{ 0x0044, 0x0008 },
> +	{ 0x0046, 0x0000 },
> +	{ 0x0048, 0x00ef },
> +	{ 0x0049, 0x0000 },
> +	{ 0x0045, 0x0000 },
> +};
> +
> +static const int adux1020_rate[][2] = {
> +	{ 0, 100000 },
> +	{ 0, 200000 },
> +	{ 0, 500000 },
> +	{ 1, 0 },
> +	{ 2, 0 },
> +	{ 5, 0 },
> +	{ 10, 0 },
> +	{ 20, 0 },
> +	{ 50, 0 },
> +	{ 100, 0 },
> +	{ 190, 0 },
> +	{ 450, 0 },
> +	{ 820, 0 },
> +	{ 1400, 0 },
> +};
> +
> +static const int adux1020_led_current[][2] = {
> +	{ 0, 25000 },
> +	{ 0, 40000 },
> +	{ 0, 55000 },
> +	{ 0, 70000 },
> +	{ 0, 85000 },
> +	{ 0, 100000 },
> +	{ 0, 115000 },
> +	{ 0, 130000 },
> +	{ 0, 145000 },
> +	{ 0, 160000 },
> +	{ 0, 175000 },
> +	{ 0, 190000 },
> +	{ 0, 205000 },
> +	{ 0, 220000 },
> +	{ 0, 235000 },
> +	{ 0, 250000 },
> +};
> +
> +static void adux1020_flush_fifo(struct adux1020_data *data)
> +{
> +	/* Force Idle mode */
> +	regmap_write(data->regmap, ADUX1020_REG_FORCE_MODE,
> +		     ADUX1020_ACTIVE_4_STATE);
> +	regmap_update_bits(data->regmap, ADUX1020_REG_OP_MODE,
> +			   ADUX1020_OP_MODE_MASK, ADUX1020_MODE_FORCE);
> +	regmap_update_bits(data->regmap, ADUX1020_REG_OP_MODE,
> +			   ADUX1020_OP_MODE_MASK, ADUX1020_MODE_IDLE);
> +
> +	/* Flush FIFO */
> +	regmap_write(data->regmap, ADUX1020_REG_TEST_MODES_3,
> +		     ADUX1020_FORCE_CLOCK_ON);
> +	regmap_write(data->regmap, ADUX1020_REG_INT_STATUS,
> +		     ADUX1020_FIFO_FLUSH);
> +	regmap_write(data->regmap, ADUX1020_REG_TEST_MODES_3,
> +		     ADUX1020_FORCE_CLOCK_RESET);

These bits could use with some minimal error checking.[3]
Maybe also convert to return int and check errors.

> +}
> +
> +static int adux1020_read_fifo(struct adux1020_data *data, u16 *buf, u8
> buf_len)
> +{
> +	int i, ret = -EINVAL;
> +	unsigned int regval;
> +
> +	/* Enable 32MHz clock */
> +	regmap_write(data->regmap, ADUX1020_REG_TEST_MODES_3,
> +		     ADUX1020_FORCE_CLOCK_ON);

This looks weird [the FORCE_CLOCK_ON], but it is what the datasheet says.
Also, related to [3]: some error checking this return would be useful.

> +
> +	for (i = 0; i < buf_len; i++) {
> +		ret = regmap_read(data->regmap, ADUX1020_REG_DATA_BUFFER,
> +				      &regval);
> +		if (ret < 0)
> +			goto err_out;
> +
> +		buf[i] = regval;
> +	}
> +
> +	/* Set 32MHz clock to be controlled by internal state machine */
> +	regmap_write(data->regmap, ADUX1020_REG_TEST_MODES_3,
> +		     ADUX1020_FORCE_CLOCK_RESET);
> +
> +err_out:
> +	return ret;
> +}
> +
> +static void adux1020_set_mode(struct adux1020_data *data,
> +			      enum adux1020_op_modes mode)
> +{
> +	/* Switch to standby mode before changing the mode */
> +	regmap_write(data->regmap, ADUX1020_REG_OP_MODE,
> ADUX1020_MODE_STANDBY);

[3] error checking maybe? and maybe int return?

> +
> +	/* Set data out and switch to the desired mode */
> +	if (mode == ADUX1020_MODE_PROX_I) {

This could become a switch() statement, so that when/if other modes get
added, the patch looks cleaner.

> +		regmap_update_bits(data->regmap, ADUX1020_REG_OP_MODE,
> +			ADUX1020_DATA_OUT_MODE_MASK,
> +			ADUX1020_MODE_OUT_PROX_I <<
> ADUX1020_MODE_OUT_SHIFT);

Related to [1]: a FIELD_PREP() macro would be useful.

> +		regmap_update_bits(data->regmap, ADUX1020_REG_OP_MODE,
> +			ADUX1020_OP_MODE_MASK, ADUX1020_MODE_PROX_I);
> +	}
> +}
> +
> +static int adux1020_measure(struct adux1020_data *data,
> +			    enum adux1020_op_modes mode,
> +			    u16 *val)
> +{
> +	int ret, tries = 50;
> +	unsigned int status;
> +
> +	mutex_lock(&data->lock);

The scope of this lock looks like it would need to be extended a bit.
See [4] on some more notes for this.

> +
> +	/* Disable INT pin as polling is going to be used */
> +	regmap_write(data->regmap, ADUX1020_REG_INT_ENABLE,
> +		     ADUX1020_INT_DISABLE);
> +
> +	/* Enable mode interrupt */
> +	regmap_update_bits(data->regmap, ADUX1020_REG_INT_MASK,
> +			   ADUX1020_MODE_INT_MASK,
> +			   adux1020_modes[mode].int_en);

[3] maybe some more error checking?

> +
> +	while (tries--) {
> +		ret = regmap_read(data->regmap, ADUX1020_REG_INT_STATUS,
> +				  &status);
> +		if (ret < 0)
> +			goto fail;
> +
> +		status &= ADUX1020_FIFO_STATUS_MASK;
> +		if (status >= adux1020_modes[mode].bytes)
> +			break;
> +		msleep(20);
> +	}
> +
> +	if (tries < 0) {
> +		ret = -EIO;
> +		goto fail;
> +	}
> +

Regarding [8], the buffer is passed from adux1020_read_raw(), so maybe just
move the buffer here, since the information about size is near the buffer
definition.
Then do *val = buf[0];

> +	ret = adux1020_read_fifo(data, val, adux1020_modes[mode].buf_len);
> +	if (ret < 0)
> +		goto fail;
> +
> +	/* Clear mode interrupt */
> +	regmap_write(data->regmap, ADUX1020_REG_INT_STATUS,
> +			   (~adux1020_modes[mode].int_en));
> +	/* Disable mode interrupts */
> +	regmap_update_bits(data->regmap, ADUX1020_REG_INT_MASK,
> +			   ADUX1020_MODE_INT_MASK,
> ADUX1020_MODE_INT_DISABLE);
> +
> +fail:
> +	mutex_unlock(&data->lock);
> +
> +	return ret;
> +}
> +
> +static int adux1020_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	struct adux1020_data *data = iio_priv(indio_dev);
> +	u16 buf[3];

This buffer looks a bit weird. [8]
It's 3 elements-wide and passed without any information about size.
And only the first element is used.
So, maybe just convert u16 buf[3] -> u16 buf?

> +	int ret = -EINVAL;
> +	unsigned int regval;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			adux1020_set_mode(data, ADUX1020_MODE_PROX_I);
> +			ret = adux1020_measure(data, ADUX1020_MODE_PROX_I,
> buf);

Regarding [4]: I'm thinking that the lock could be extended to
adux1020_{read,write}_raw() & adux1020_{read,write}_event_config()
functions, especialy [or only in places] where functions seem to do
consecutive R/W ops.

> +			if (ret < 0)
> +				return ret;
> +
> +			*val = buf[0];
> +			ret = IIO_VAL_INT;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
> +	case IIO_CHAN_INFO_PROCESSED:
> +		switch (chan->type) {
> +		case IIO_CURRENT:
> +			ret = regmap_read(data->regmap,
> +					  ADUX1020_REG_LED_CURRENT,
> &regval);
> +			if (ret < 0)
> +				return ret;
> +
> +			regval = regval & ADUX1020_LED_CURRENT_MASK;
> +
> +			*val = adux1020_led_current[regval][0];
> +			*val2 = adux1020_led_current[regval][1];
> +
> +			ret = IIO_VAL_INT_PLUS_MICRO;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		switch (chan->type) {
> +		case IIO_PROXIMITY:
> +			ret = regmap_read(data->regmap,
> ADUX1020_REG_FREQUENCY,
> +					  &regval);
> +			if (ret < 0)
> +				return ret;
> +
> +			regval = (regval & ADUX1020_PROX_FREQ_MASK) >>
> +				  ADUX1020_PROX_FREQ_SHIFT;

Related to [1]: a FIELD_PREP() macro would be useful.

> +
> +			*val = adux1020_rate[regval][0];
> +			*val2 = adux1020_rate[regval][1];
> +
> +			ret = IIO_VAL_INT_PLUS_MICRO;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return ret;
> +};
> +
> +static int adux1020_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int val, int val2, long mask)
> +{
> +	struct adux1020_data *data = iio_priv(indio_dev);
> +	int i, ret = -EINVAL;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (chan->type == IIO_PROXIMITY) {
> +			for (i = 0; i < ARRAY_SIZE(adux1020_rate); i++) {
> +				if ((val == adux1020_rate[i][0]) &&
> +				     (val2 == adux1020_rate[i][1])) {
> +					ret = regmap_update_bits(data-
> >regmap,
> +						ADUX1020_REG_FREQUENCY,
> +						ADUX1020_PROX_FREQ_MASK,
> +						i <<
> ADUX1020_PROX_FREQ_SHIFT);
> +				}
> +			}

[5] This looks as it has too many level of indentations.
And could be reworked with either using the "find_closest()" macro, or at
least split into a helper that finds the index in the "adux1020_rate" array
based on val & val2.

Then you get something like:
------------------------------------------------------------
i = adux1020_find_index(adux1020_rate, val, val2);
if (i < 0)
    return i;

return regmap_update_bits(data->regmap,
                          ADUX1020_REG_FREQUENCY,
                          ADUX1020_PROX_FREQ_SET(i));
------------------------------------------------------------

> +		}
> +		break;
> +	case IIO_CHAN_INFO_PROCESSED:
> +		if (chan->type == IIO_CURRENT) {
> +			for (i = 0; i < ARRAY_SIZE(adux1020_led_current);
> i++) {
> +				if ((val == adux1020_led_current[i][0]) &&
> +				     (val2 == adux1020_led_current[i][1]))
> {
> +					ret = regmap_update_bits(data-
> >regmap,
> +						ADUX1020_REG_LED_CURRENT,
> +						ADUX1020_LED_CURRENT_MASK,
> i);
> +				}
> +			}

Same comment as [5]

> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static int adux1020_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 adux1020_data *data = iio_priv(indio_dev);
> +
> +	regmap_write(data->regmap, ADUX1020_REG_INT_ENABLE,
> +		     ADUX1020_INT_ENABLE);
> +

Regarding [4]: it looks like the lock could be useful here.
Regarding [3]: more error checking could be useful.


> +	regmap_write(data->regmap, ADUX1020_REG_INT_POLARITY, 0);
> +
> +	switch (chan->type) {
> +	case IIO_PROXIMITY:
> +		if (dir == IIO_EV_DIR_RISING) {
> +			regmap_update_bits(data->regmap,
> ADUX1020_REG_INT_MASK,
> +					   ADUX1020_PROX_ON1_INT,
> +					   state ? 0 :
> ADUX1020_PROX_ON1_INT);
> +		} else {
> +			regmap_update_bits(data->regmap,
> ADUX1020_REG_INT_MASK,
> +					   ADUX1020_PROX_OFF1_INT,
> +					   state ? 0 :
> ADUX1020_PROX_OFF1_INT);
> +		}

[6] More about style/preference.
I like the part in adux1020_read_event_config(), where

-------------------------------------------------
  if (dir == IIO_EV_DIR_RISING)
      mask = ADUX1020_PROX_ON1_INT;
  else
      mask = ADUX1020_PROX_OFF1_INT;
-------------------------------------------------

Then this could become:

-------------------------------------------------
if (state)
    state = mask;
else
    state = 0;

ret = regmap_update_bits(data->regmap,
                         ADUX1020_REG_INT_MASK,
                         mask, state);
-------------------------------------------------


> +
> +		/*
> +		 * Trigger proximity interrupt when the intensity is above
> +		 * or below threshold
> +		 */
> +		regmap_update_bits(data->regmap, ADUX1020_REG_PROX_TYPE,
> +				   ADUX1020_PROX_TYPE, ADUX1020_PROX_TYPE);
> +
> +		/* Set proximity mode */
> +		adux1020_set_mode(data, ADUX1020_MODE_PROX_I);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int adux1020_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 adux1020_data *data = iio_priv(indio_dev);
> +	int ret, mask;
> +	unsigned int regval;
> +
> +	switch (chan->type) {
> +	case IIO_PROXIMITY:
> +		if (dir == IIO_EV_DIR_RISING)
> +			mask = ADUX1020_PROX_ON1_INT;
> +		else
> +			mask = ADUX1020_PROX_OFF1_INT;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_read(data->regmap, ADUX1020_REG_INT_MASK, &regval);
> +	if (ret < 0)
> +		return ret;
> +
> +	return !(regval & mask);
> +}
> +
> +static int adux1020_read_thresh(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 adux1020_data *data = iio_priv(indio_dev);
> +	u8 reg;
> +	int ret;
> +	unsigned int regval;
> +
> +	switch (chan->type) {
> +	case IIO_PROXIMITY:
> +		if (dir == IIO_EV_DIR_RISING)
> +			reg = ADUX1020_REG_PROX_TH_ON1;
> +		else
> +			reg = ADUX1020_REG_PROX_TH_OFF1;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_read(data->regmap, reg, &regval);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = regval;
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int adux1020_write_thresh(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 adux1020_data *data = iio_priv(indio_dev);
> +	u8 reg;
> +
> +	switch (chan->type) {
> +	case IIO_PROXIMITY:
> +		if (dir == IIO_EV_DIR_RISING)
> +			reg = ADUX1020_REG_PROX_TH_ON1;
> +		else
> +			reg = ADUX1020_REG_PROX_TH_OFF1;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/* Full scale threshold value is 0-65535  */
> +	if (val < 0 || val > 65535)
> +		return -EINVAL;
> +
> +	return regmap_write(data->regmap, reg, val);
> +}
> +
> +static const struct iio_event_spec adux1020_proximity_event[] = {
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> +			BIT(IIO_EV_INFO_ENABLE),
> +	},
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_FALLING,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> +			BIT(IIO_EV_INFO_ENABLE),
> +	},
> +};
> +
> +static const struct iio_chan_spec adux1020_channels[] = {
> +	{
> +		.type = IIO_PROXIMITY,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +		.event_spec = adux1020_proximity_event,
> +		.num_event_specs = ARRAY_SIZE(adux1020_proximity_event),
> +	},
> +	{
> +		.type = IIO_CURRENT,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> +		.extend_name = "led",
> +	},
> +};
> +
> +static IIO_CONST_ATTR(sampling_frequency_available,
> +		      "0.1 0.2 0.5 1 2 5 10 20 50 100 190 450 820 1400");

This could re-use the IIO_CONST_ATTR_SAMP_FREQ_AVAIL() macro.

> +
> +static struct attribute *adux1020_attributes[] = {
> +	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group adux1020_attribute_group = {
> +	.attrs = adux1020_attributes,
> +};
> +
> +static const struct iio_info adux1020_info = {
> +	.attrs = &adux1020_attribute_group,
> +	.read_raw = adux1020_read_raw,
> +	.write_raw = adux1020_write_raw,
> +	.read_event_config = adux1020_read_event_config,
> +	.write_event_config = adux1020_write_event_config,
> +	.read_event_value = adux1020_read_thresh,
> +	.write_event_value = adux1020_write_thresh,
> +};
> +
> +static irqreturn_t adux1020_interrupt_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct adux1020_data *data = iio_priv(indio_dev);
> +	int ret, status;
> +
> +	ret = regmap_read(data->regmap, ADUX1020_REG_INT_STATUS, &status);
> +	if (ret < 0)
> +		return ret;
> +
> +	status &= ADUX1020_MODE_INT_STATUS_MASK;
> +
> +	if (status & ADUX1020_INT_PROX_ON1) {
> +		iio_push_event(indio_dev,
> +				IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
> +					IIO_EV_TYPE_THRESH,
> +					IIO_EV_DIR_RISING),
> +				iio_get_time_ns(indio_dev));
> +	}
> +
> +	if (status & ADUX1020_INT_PROX_OFF1) {
> +		iio_push_event(indio_dev,
> +				IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
> +					IIO_EV_TYPE_THRESH,
> +					IIO_EV_DIR_FALLING),
> +				iio_get_time_ns(indio_dev));
> +	}
> +
> +	regmap_update_bits(data->regmap, ADUX1020_REG_INT_STATUS,
> +			   ADUX1020_MODE_INT_MASK, status);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int adux1020_chip_init(struct adux1020_data *data)
> +{
> +	struct i2c_client *client = data->client;
> +	int ret, i;
> +	unsigned int val;
> +
> +	ret = regmap_read(data->regmap, ADUX1020_REG_CHIP_ID, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	val &= ADUX1020_CHIP_ID_MASK;
> +
> +	if (val != ADUX1020_CHIP_ID) {
> +		dev_err(&client->dev, "invalid chip id 0x%04x\n", val);
> +		return -ENODEV;
> +	};
> +
> +	dev_dbg(&client->dev, "Detected ADUX1020 with chip id: 0x%04x\n",
> val);
> +
> +	/* Perform software reset */
> +	regmap_update_bits(data->regmap, ADUX1020_REG_SW_RESET,
> +			   ADUX1020_SW_RESET, ADUX1020_SW_RESET);
> +
> +	/* Load default configuration */
> +	for (i = 0; i < ARRAY_SIZE(adux1020_def_conf); i++)
> +		regmap_write(data->regmap, adux1020_def_conf[i][0],
> +			     adux1020_def_conf[i][1]);
> +
> +	adux1020_flush_fifo(data);
> +
> +	/* Use LED_IREF for proximity mode */
> +	regmap_update_bits(data->regmap, ADUX1020_REG_LED_CURRENT,
> +			   ADUX1020_LED_PIREF_EN, 0);
> +
> +	/* Mask all interrupts */
> +	regmap_update_bits(data->regmap, ADUX1020_REG_INT_MASK,
> +			   ADUX1020_MODE_INT_MASK,
> ADUX1020_MODE_INT_DISABLE);

Same as [3]: a bit more error checking here could be useful.

> +
> +	return 0;
> +}
> +
> +static int adux1020_probe(struct i2c_client *client,
> +			  const struct i2c_device_id *id)
> +{
> +	struct adux1020_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->info = &adux1020_info;
> +	indio_dev->name = ADUX1020_DRV_NAME;
> +	indio_dev->channels = adux1020_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(adux1020_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +
> +	data->regmap = devm_regmap_init_i2c(client,
> &adux1020_regmap_config);
> +	if (IS_ERR(data->regmap)) {
> +		dev_err(&client->dev, "regmap initialization failed.\n");
> +		return PTR_ERR(data->regmap);
> +	}
> +
> +	data->client = client;
> +	data->indio_dev = indio_dev;
> +	mutex_init(&data->lock);
> +
> +	ret = adux1020_chip_init(data);
> +	if (ret)
> +		goto err_out;
> +
> +	if (client->irq) {
> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> +				NULL, adux1020_interrupt_handler,
> +				IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> +				ADUX1020_DRV_NAME, indio_dev);
> +		if (ret) {
> +			dev_err(&client->dev, "irq request error %d\n",
> -ret);
> +			goto err_out;
> +		}
> +	}
> +
> +	ret = iio_device_register(indio_dev);

[7] Jonathan may correct me here.
If this doesn't need to do anything more later, this could use
devm_iio_device_register(), and then the  adux1020_remove() hook could be
removed.

> +	if (ret) {
> +		dev_err(&client->dev, "Failed to register IIO device\n");
> +		goto err_out;
> +	}
> +
> +	return 0;
> +
> +err_out:
> +	return ret;
> +}
> +
> +static int adux1020_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
> +	iio_device_unregister(indio_dev);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id adux1020_id[] = {
> +	{ "adux1020", 0 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, adux1020_id);
> +
> +static const struct of_device_id adux1020_of_match[] = {
> +	{ .compatible = "adi,adux1020" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, adux1020_of_match);
> +
> +static struct i2c_driver adux1020_driver = {
> +	.driver = {
> +		.name	= ADUX1020_DRV_NAME,
> +		.of_match_table = adux1020_of_match,
> +	},
> +	.probe		= adux1020_probe,
> +	.remove		= adux1020_remove,
> +	.id_table	= adux1020_id,
> +};
> +module_i2c_driver(adux1020_driver);
> +
> +MODULE_AUTHOR("Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>"
> );
> +MODULE_DESCRIPTION("ADUX1020 photometric sensor");
> +MODULE_LICENSE("GPL");

Re: [PATCH 2/2] iio: light: Add support for ADUX1020 sensor

[Manivannan Sadhasivam]

Hi Ardelean,

Thanks for the quick review!

On Tue, Oct 08, 2019 at 06:52:50AM +0000, Ardelean, Alexandru wrote:
> On Mon, 2019-10-07 at 15:40 +0530, Manivannan Sadhasivam wrote:
> > [External]
> > 
> 
> Hey,
> 
> Comments inline.
> 
> I thought I sent an initial review, but seems to have gotten lost [maybe in
> my email client].
> Oh well. I managed to re-do it anyway.
> 
> I tried to group them this time.
> 
> The more prominent part is [3]; this driver needs a bit more error checking
> on regmap_() returns.
> 

Yes, agree. I forgot that I'm not working on memory mapped region ;-)

> Generally some notes:
> - Is there a need to implement the 32Khz or 32Mhz clock calibration
> routines on startup? Some drivers need this, some don't/

Calibration is required to have the precise reading but it is not a blocker.
We can add it later.

> - From the functional diagram, it looks like maybe the VREF would be needed
> to be hooked via a regulator framework; but this could be done later

Right but the reference board schematics is not very clear about VREF and
neither the sensor datasheet. That's why I intentionally left it. Will get
in touch with the board vendor to figure out what is the recommended VREF
voltage and submit a patch later.

> - Just curios here: there is gesture mode as well; will that be implemented
> later? Or will there be other modes implemented?

Currently only proximity mode is implemented. There are gesture and sample
modes and I left those as a TODO. But I'm not sure whether IIO is supporting
gesture mode properly or not.

> 
> If I remember anything else I may come back with a reply.
> 

Sure.

> Thanks
> Alex
> 
> > Add initial support for Analog Devices ADUX1020 Photometric sensor.
> > Only proximity mode has been enabled for now.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/iio/light/Kconfig    |  11 +
> >  drivers/iio/light/Makefile   |   1 +
> >  drivers/iio/light/adux1020.c | 783 +++++++++++++++++++++++++++++++++++
> 
> Does MAINTAINERS need updating as well?
> 

I don't prefer to have MAINTAINERS entry for small drivers like this.
Anyway, get_maintainers will return my mailing address based on the
commit signing.

> >  3 files changed, 795 insertions(+)
> >  create mode 100644 drivers/iio/light/adux1020.c
> > 
> > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> > index 08d7e1ef2186..3f8c8689cd89 100644
> > --- a/drivers/iio/light/Kconfig
> > +++ b/drivers/iio/light/Kconfig
> > @@ -32,6 +32,17 @@ config ADJD_S311
> >  	  This driver can also be built as a module.  If so, the module
> >  	  will be called adjd_s311.
> >  
> > +config ADUX1020
> > +	tristate "ADUX1020 photometric sensor"
> > +	select REGMAP_I2C
> > +	depends on I2C
> > +	help
> > +	 Say Y here if you want to build a driver for the Analog Devices
> > +	 ADUX1020 photometric sensor.
> > +
> > +	 To compile this driver as a module, choose M here: the
> > +	 module will be called adux1020.
> > +
> >  config AL3320A
> >  	tristate "AL3320A ambient light sensor"
> >  	depends on I2C
> > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> > index 00d1f9b98f39..5d650ce46a40 100644
> > --- a/drivers/iio/light/Makefile
> > +++ b/drivers/iio/light/Makefile
> > @@ -6,6 +6,7 @@
> >  # When adding new entries keep the list in alphabetical order
> >  obj-$(CONFIG_ACPI_ALS)		+= acpi-als.o
> >  obj-$(CONFIG_ADJD_S311)		+= adjd_s311.o
> > +obj-$(CONFIG_ADUX1020)		+= adux1020.o
> >  obj-$(CONFIG_AL3320A)		+= al3320a.o
> >  obj-$(CONFIG_APDS9300)		+= apds9300.o
> >  obj-$(CONFIG_APDS9960)		+= apds9960.o
> > diff --git a/drivers/iio/light/adux1020.c b/drivers/iio/light/adux1020.c
> > new file mode 100644
> > index 000000000000..d0b76e5b44f1
> > --- /dev/null
> > +++ b/drivers/iio/light/adux1020.c
> > @@ -0,0 +1,783 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * adux1020.c - Support for Analog Devices ADUX1020 photometric sensor
> 
> Maybe drop the adux1020.c part?
> I think something like this should be sufficient:
> "Analog Devices ADUX1020 photometric sensor"
>

okay.

> > + *
> > + * Copyright (C) 2019 Linaro Ltd.
> > + * Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > + *
> > + * TODO: Triggered buffer support
> 
> Maybe drop the TODO?
> It's not needed for mainline.
>

It is not needed but it can be added. TODO is required to explicitly
mention what are all left in the driver which can be picked up by the
community. So I prefer to keep it.
 
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regmap.h>
> > +
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/iio/events.h>
> > +
> > +#define ADUX1020_REGMAP_NAME		"adux1020_regmap"
> > +#define ADUX1020_DRV_NAME		"adux1020"
> > +
> > +/* System registers */
> > +#define ADUX1020_REG_CHIP_ID		0x08
> > +#define ADUX1020_REG_SLAVE_ADDRESS	0x09
> > +
> > +#define ADUX1020_REG_SW_RESET		0x0f
> > +#define ADUX1020_REG_INT_ENABLE		0x1c
> > +#define ADUX1020_REG_INT_POLARITY	0x1d
> > +#define ADUX1020_REG_PROX_TH_ON1	0x2a
> > +#define ADUX1020_REG_PROX_TH_OFF1	0x2b
> > +#define	ADUX1020_REG_PROX_TYPE		0x2f
> > +#define	ADUX1020_REG_TEST_MODES_3	0x32
> > +#define	ADUX1020_REG_FORCE_MODE		0x33
> > +#define	ADUX1020_REG_FREQUENCY		0x40
> > +#define ADUX1020_REG_LED_CURRENT	0x41
> > +#define	ADUX1020_REG_OP_MODE		0x45
> > +#define	ADUX1020_REG_INT_MASK		0x48
> > +#define	ADUX1020_REG_INT_STATUS		0x49
> > +#define	ADUX1020_REG_DATA_BUFFER	0x60
> > +
> > +/* Chip ID bits */
> > +#define ADUX1020_CHIP_ID_MASK		GENMASK(11, 0)
> > +#define ADUX1020_CHIP_ID		0x03fc
> > +
> > +#define ADUX1020_MODE_OUT_SHIFT		4
> 
> I'm seeing a few _SHIFT macros.
> Maybe use the FIELD_PREP() macro where possible? [1]
> 

okay.

> > +#define ADUX1020_MODE_OUT_PROX_I	1
> > +#define ADUX1020_MODE_OUT_PROX_XY	3
> > +
> > +#define ADUX1020_SW_RESET		BIT(1)
> > +#define ADUX1020_FIFO_FLUSH		BIT(15)
> > +#define ADUX1020_OP_MODE_MASK		GENMASK(3, 0)
> > +#define ADUX1020_DATA_OUT_MODE_MASK	GENMASK(7, 4)
> > +
> > +#define ADUX1020_MODE_INT_MASK		GENMASK(7, 0)
> > +#define ADUX1020_INT_ENABLE		0x2096
> > +#define ADUX1020_INT_DISABLE		0x2090
> > +#define ADUX1020_PROX_INT_ENABLE	0x00f0
> > +#define ADUX1020_PROX_ON1_INT		BIT(0)
> > +#define ADUX1020_PROX_OFF1_INT		BIT(1)
> > +#define ADUX1020_FIFO_INT_ENABLE	0x7f
> > +#define ADUX1020_MODE_INT_DISABLE	0xff
> > +#define ADUX1020_MODE_INT_STATUS_MASK	GENMASK(7, 0)
> > +#define ADUX1020_FIFO_STATUS_MASK	GENMASK(15, 8)
> > +#define ADUX1020_PROX_TYPE		BIT(15)
> > +
> > +#define ADUX1020_INT_PROX_ON1		BIT(0)
> > +#define ADUX1020_INT_PROX_OFF1		BIT(1)
> > +
> > +#define	ADUX1020_FORCE_CLOCK_ON		0x0f4f
> > +#define	ADUX1020_FORCE_CLOCK_RESET	0x0040
> 
> nitpick: indentation seems inconsistent here
> 

will fix it.

> > +#define ADUX1020_ACTIVE_4_STATE		0x0008
> > +
> > +#define ADUX1020_PROX_FREQ_MASK		GENMASK(7, 4)
> > +#define ADUX1020_PROX_FREQ_SHIFT	4
> 
> Same as [1]. Maybe use FIELD_PREP() together with ADUX1020_PROX_FREQ_MASK?
> See other examples, but we sometimes use them as:
> 
> #define ADUX_PROX_FREQ_SET(x)    FIELD_PREP(ADUX1020_PROX_FREQ_MASK, (x))
> 

okay.

> > +
> > +#define ADUX1020_LED_CURRENT_MASK	GENMASK(3, 0)
> > +#define ADUX1020_LED_PIREF_EN		BIT(12)
> > +
> > +/* Operating modes */
> > +enum adux1020_op_modes {
> > +	ADUX1020_MODE_STANDBY,
> > +	ADUX1020_MODE_PROX_I,
> > +	ADUX1020_MODE_PROX_XY,
> > +	ADUX1020_MODE_GEST,
> > +	ADUX1020_MODE_SAMPLE,
> > +	ADUX1020_MODE_FORCE = 0x0e,
> > +	ADUX1020_MODE_IDLE = 0x0f,
> > +};
> > +
> > +struct adux1020_data {
> > +	struct i2c_client *client;
> > +	struct iio_dev *indio_dev;
> > +	struct mutex lock;
> > +	struct regmap *regmap;
> > +};
> > +
> > +struct adux1020_mode_data {
> > +	u8 bytes;
> > +	u8 buf_len;
> > +	u16 int_en;
> > +};
> > +
> > +static const struct adux1020_mode_data adux1020_modes[] = {
> > +	[ADUX1020_MODE_PROX_I] = {
> > +		.bytes = 2,
> > +		.buf_len = 1,
> > +		.int_en = ADUX1020_PROX_INT_ENABLE,
> > +	},
> > +};
> > +
> > +static const struct regmap_config adux1020_regmap_config = {
> > +	.name = ADUX1020_REGMAP_NAME,
> > +	.reg_bits = 8,
> > +	.val_bits = 16,
> > +	.max_register = 0x6F,
> > +	.cache_type = REGCACHE_NONE,
> > +};
> > +
> > +static const int adux1020_def_conf[][2] = {
> 
> Maybe use "struct reg_sequence" here?[2]
> You could then use "regmap_multi_reg_write()"
> 

ack.

> > +	{ 0x000c, 0x000f },
> > +	{ 0x0010, 0x1010 },
> > +	{ 0x0011, 0x004c },
> > +	{ 0x0012, 0x5f0c },
> > +	{ 0x0013, 0xada5 },
> > +	{ 0x0014, 0x0080 },
> > +	{ 0x0015, 0x0000 },
> > +	{ 0x0016, 0x0600 },
> > +	{ 0x0017, 0x0000 },
> > +	{ 0x0018, 0x2693 },
> > +	{ 0x0019, 0x0004 },
> > +	{ 0x001a, 0x4280 },
> > +	{ 0x001b, 0x0060 },
> > +	{ 0x001c, 0x2094 },
> > +	{ 0x001d, 0x0020 },
> > +	{ 0x001e, 0x0001 },
> > +	{ 0x001f, 0x0100 },
> > +	{ 0x0020, 0x0320 },
> > +	{ 0x0021, 0x0A13 },
> > +	{ 0x0022, 0x0320 },
> > +	{ 0x0023, 0x0113 },
> > +	{ 0x0024, 0x0000 },
> > +	{ 0x0025, 0x2412 },
> > +	{ 0x0026, 0x2412 },
> > +	{ 0x0027, 0x0022 },
> > +	{ 0x0028, 0x0000 },
> > +	{ 0x0029, 0x0300 },
> > +	{ 0x002a, 0x0700 },
> > +	{ 0x002b, 0x0600 },
> > +	{ 0x002c, 0x6000 },
> > +	{ 0x002d, 0x4000 },
> > +	{ 0x002e, 0x0000 },
> > +	{ 0x002f, 0x0000 },
> > +	{ 0x0030, 0x0000 },
> > +	{ 0x0031, 0x0000 },
> > +	{ 0x0032, 0x0040 },
> > +	{ 0x0033, 0x0008 },
> > +	{ 0x0034, 0xE400 },
> > +	{ 0x0038, 0x8080 },
> > +	{ 0x0039, 0x8080 },
> > +	{ 0x003a, 0x2000 },
> > +	{ 0x003b, 0x1f00 },
> > +	{ 0x003c, 0x2000 },
> > +	{ 0x003d, 0x2000 },
> > +	{ 0x003e, 0x0000 },
> > +	{ 0x0040, 0x8069 },
> > +	{ 0x0041, 0x1f2f },
> > +	{ 0x0042, 0x4000 },
> > +	{ 0x0043, 0x0000 },
> > +	{ 0x0044, 0x0008 },
> > +	{ 0x0046, 0x0000 },
> > +	{ 0x0048, 0x00ef },
> > +	{ 0x0049, 0x0000 },
> > +	{ 0x0045, 0x0000 },
> > +};
> > +
> > +static const int adux1020_rate[][2] = {
> > +	{ 0, 100000 },
> > +	{ 0, 200000 },
> > +	{ 0, 500000 },
> > +	{ 1, 0 },
> > +	{ 2, 0 },
> > +	{ 5, 0 },
> > +	{ 10, 0 },
> > +	{ 20, 0 },
> > +	{ 50, 0 },
> > +	{ 100, 0 },
> > +	{ 190, 0 },
> > +	{ 450, 0 },
> > +	{ 820, 0 },
> > +	{ 1400, 0 },
> > +};
> > +
> > +static const int adux1020_led_current[][2] = {
> > +	{ 0, 25000 },
> > +	{ 0, 40000 },
> > +	{ 0, 55000 },
> > +	{ 0, 70000 },
> > +	{ 0, 85000 },
> > +	{ 0, 100000 },
> > +	{ 0, 115000 },
> > +	{ 0, 130000 },
> > +	{ 0, 145000 },
> > +	{ 0, 160000 },
> > +	{ 0, 175000 },
> > +	{ 0, 190000 },
> > +	{ 0, 205000 },
> > +	{ 0, 220000 },
> > +	{ 0, 235000 },
> > +	{ 0, 250000 },
> > +};
> > +
> > +static void adux1020_flush_fifo(struct adux1020_data *data)
> > +{
> > +	/* Force Idle mode */
> > +	regmap_write(data->regmap, ADUX1020_REG_FORCE_MODE,
> > +		     ADUX1020_ACTIVE_4_STATE);
> > +	regmap_update_bits(data->regmap, ADUX1020_REG_OP_MODE,
> > +			   ADUX1020_OP_MODE_MASK, ADUX1020_MODE_FORCE);
> > +	regmap_update_bits(data->regmap, ADUX1020_REG_OP_MODE,
> > +			   ADUX1020_OP_MODE_MASK, ADUX1020_MODE_IDLE);
> > +
> > +	/* Flush FIFO */
> > +	regmap_write(data->regmap, ADUX1020_REG_TEST_MODES_3,
> > +		     ADUX1020_FORCE_CLOCK_ON);
> > +	regmap_write(data->regmap, ADUX1020_REG_INT_STATUS,
> > +		     ADUX1020_FIFO_FLUSH);
> > +	regmap_write(data->regmap, ADUX1020_REG_TEST_MODES_3,
> > +		     ADUX1020_FORCE_CLOCK_RESET);
> 
> These bits could use with some minimal error checking.[3]
> Maybe also convert to return int and check errors.
> 

Yes, I will added error check in relevant places.

> > +}
> > +
> > +static int adux1020_read_fifo(struct adux1020_data *data, u16 *buf, u8
> > buf_len)
> > +{
> > +	int i, ret = -EINVAL;
> > +	unsigned int regval;
> > +
> > +	/* Enable 32MHz clock */
> > +	regmap_write(data->regmap, ADUX1020_REG_TEST_MODES_3,
> > +		     ADUX1020_FORCE_CLOCK_ON);
> 
> This looks weird [the FORCE_CLOCK_ON], but it is what the datasheet says.
> Also, related to [3]: some error checking this return would be useful.
> 

ack.

> > +
> > +	for (i = 0; i < buf_len; i++) {
> > +		ret = regmap_read(data->regmap, ADUX1020_REG_DATA_BUFFER,
> > +				      &regval);
> > +		if (ret < 0)
> > +			goto err_out;
> > +
> > +		buf[i] = regval;
> > +	}
> > +
> > +	/* Set 32MHz clock to be controlled by internal state machine */
> > +	regmap_write(data->regmap, ADUX1020_REG_TEST_MODES_3,
> > +		     ADUX1020_FORCE_CLOCK_RESET);
> > +
> > +err_out:
> > +	return ret;
> > +}
> > +
> > +static void adux1020_set_mode(struct adux1020_data *data,
> > +			      enum adux1020_op_modes mode)
> > +{
> > +	/* Switch to standby mode before changing the mode */
> > +	regmap_write(data->regmap, ADUX1020_REG_OP_MODE,
> > ADUX1020_MODE_STANDBY);
> 
> [3] error checking maybe? and maybe int return?
> 

ack.

> > +
> > +	/* Set data out and switch to the desired mode */
> > +	if (mode == ADUX1020_MODE_PROX_I) {
> 
> This could become a switch() statement, so that when/if other modes get
> added, the patch looks cleaner.
> 

okay.

> > +		regmap_update_bits(data->regmap, ADUX1020_REG_OP_MODE,
> > +			ADUX1020_DATA_OUT_MODE_MASK,
> > +			ADUX1020_MODE_OUT_PROX_I <<
> > ADUX1020_MODE_OUT_SHIFT);
> 
> Related to [1]: a FIELD_PREP() macro would be useful.
> 

okay.

> > +		regmap_update_bits(data->regmap, ADUX1020_REG_OP_MODE,
> > +			ADUX1020_OP_MODE_MASK, ADUX1020_MODE_PROX_I);
> > +	}
> > +}
> > +
> > +static int adux1020_measure(struct adux1020_data *data,
> > +			    enum adux1020_op_modes mode,
> > +			    u16 *val)
> > +{
> > +	int ret, tries = 50;
> > +	unsigned int status;
> > +
> > +	mutex_lock(&data->lock);
> 
> The scope of this lock looks like it would need to be extended a bit.
> See [4] on some more notes for this.
> 
> > +
> > +	/* Disable INT pin as polling is going to be used */
> > +	regmap_write(data->regmap, ADUX1020_REG_INT_ENABLE,
> > +		     ADUX1020_INT_DISABLE);
> > +
> > +	/* Enable mode interrupt */
> > +	regmap_update_bits(data->regmap, ADUX1020_REG_INT_MASK,
> > +			   ADUX1020_MODE_INT_MASK,
> > +			   adux1020_modes[mode].int_en);
> 
> [3] maybe some more error checking?
> 

ack.

> > +
> > +	while (tries--) {
> > +		ret = regmap_read(data->regmap, ADUX1020_REG_INT_STATUS,
> > +				  &status);
> > +		if (ret < 0)
> > +			goto fail;
> > +
> > +		status &= ADUX1020_FIFO_STATUS_MASK;
> > +		if (status >= adux1020_modes[mode].bytes)
> > +			break;
> > +		msleep(20);
> > +	}
> > +
> > +	if (tries < 0) {
> > +		ret = -EIO;
> > +		goto fail;
> > +	}
> > +
> 
> Regarding [8], the buffer is passed from adux1020_read_raw(), so maybe just
> move the buffer here, since the information about size is near the buffer
> definition.
> Then do *val = buf[0];
> 

I don't see any benefit from this. As said below, the buffer size is fixed
and is based on the maximum hardware FIFO size. It goes upto 6 bytes
(3-16bit words).

> > +	ret = adux1020_read_fifo(data, val, adux1020_modes[mode].buf_len);
> > +	if (ret < 0)
> > +		goto fail;
> > +
> > +	/* Clear mode interrupt */
> > +	regmap_write(data->regmap, ADUX1020_REG_INT_STATUS,
> > +			   (~adux1020_modes[mode].int_en));
> > +	/* Disable mode interrupts */
> > +	regmap_update_bits(data->regmap, ADUX1020_REG_INT_MASK,
> > +			   ADUX1020_MODE_INT_MASK,
> > ADUX1020_MODE_INT_DISABLE);
> > +
> > +fail:
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int adux1020_read_raw(struct iio_dev *indio_dev,
> > +			     struct iio_chan_spec const *chan,
> > +			     int *val, int *val2, long mask)
> > +{
> > +	struct adux1020_data *data = iio_priv(indio_dev);
> > +	u16 buf[3];
> 
> This buffer looks a bit weird. [8]
> It's 3 elements-wide and passed without any information about size.
> And only the first element is used.
> So, maybe just convert u16 buf[3] -> u16 buf?
> 

The buffer declaration is based on the hardware buffer available. It
is 3 elements wide since the remaining 2 elements will be used by other
modes. The idea here is to reuse the adux1020_measure() API for all 3
modes (which has varying buffer sizes).

> > +	int ret = -EINVAL;
> > +	unsigned int regval;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		switch (chan->type) {
> > +		case IIO_PROXIMITY:
> > +			adux1020_set_mode(data, ADUX1020_MODE_PROX_I);
> > +			ret = adux1020_measure(data, ADUX1020_MODE_PROX_I,
> > buf);
> 
> Regarding [4]: I'm thinking that the lock could be extended to
> adux1020_{read,write}_raw() & adux1020_{read,write}_event_config()
> functions, especialy [or only in places] where functions seem to do
> consecutive R/W ops.
> 

okay.

> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			*val = buf[0];
> > +			ret = IIO_VAL_INT;
> > +			break;
> > +		default:
> > +			break;
> > +		}
> > +		break;
> > +	case IIO_CHAN_INFO_PROCESSED:
> > +		switch (chan->type) {
> > +		case IIO_CURRENT:
> > +			ret = regmap_read(data->regmap,
> > +					  ADUX1020_REG_LED_CURRENT,
> > &regval);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			regval = regval & ADUX1020_LED_CURRENT_MASK;
> > +
> > +			*val = adux1020_led_current[regval][0];
> > +			*val2 = adux1020_led_current[regval][1];
> > +
> > +			ret = IIO_VAL_INT_PLUS_MICRO;
> > +			break;
> > +		default:
> > +			break;
> > +		}
> > +		break;
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		switch (chan->type) {
> > +		case IIO_PROXIMITY:
> > +			ret = regmap_read(data->regmap,
> > ADUX1020_REG_FREQUENCY,
> > +					  &regval);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			regval = (regval & ADUX1020_PROX_FREQ_MASK) >>
> > +				  ADUX1020_PROX_FREQ_SHIFT;
> 
> Related to [1]: a FIELD_PREP() macro would be useful.
> 

ack.

> > +
> > +			*val = adux1020_rate[regval][0];
> > +			*val2 = adux1020_rate[regval][1];
> > +
> > +			ret = IIO_VAL_INT_PLUS_MICRO;
> > +			break;
> > +		default:
> > +			break;
> > +		}
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return ret;
> > +};
> > +
> > +static int adux1020_write_raw(struct iio_dev *indio_dev,
> > +			     struct iio_chan_spec const *chan,
> > +			     int val, int val2, long mask)
> > +{
> > +	struct adux1020_data *data = iio_priv(indio_dev);
> > +	int i, ret = -EINVAL;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > +		if (chan->type == IIO_PROXIMITY) {
> > +			for (i = 0; i < ARRAY_SIZE(adux1020_rate); i++) {
> > +				if ((val == adux1020_rate[i][0]) &&
> > +				     (val2 == adux1020_rate[i][1])) {
> > +					ret = regmap_update_bits(data-
> > >regmap,
> > +						ADUX1020_REG_FREQUENCY,
> > +						ADUX1020_PROX_FREQ_MASK,
> > +						i <<
> > ADUX1020_PROX_FREQ_SHIFT);
> > +				}
> > +			}
> 
> [5] This looks as it has too many level of indentations.
> And could be reworked with either using the "find_closest()" macro, or at
> least split into a helper that finds the index in the "adux1020_rate" array
> based on val & val2.
> 
> Then you get something like:
> ------------------------------------------------------------
> i = adux1020_find_index(adux1020_rate, val, val2);
> if (i < 0)
>     return i;
> 
> return regmap_update_bits(data->regmap,
>                           ADUX1020_REG_FREQUENCY,
>                           ADUX1020_PROX_FREQ_SET(i));
> ------------------------------------------------------------
> 

good idea! will implement.

> > +		}
> > +		break;
> > +	case IIO_CHAN_INFO_PROCESSED:
> > +		if (chan->type == IIO_CURRENT) {
> > +			for (i = 0; i < ARRAY_SIZE(adux1020_led_current);
> > i++) {
> > +				if ((val == adux1020_led_current[i][0]) &&
> > +				     (val2 == adux1020_led_current[i][1]))
> > {
> > +					ret = regmap_update_bits(data-
> > >regmap,
> > +						ADUX1020_REG_LED_CURRENT,
> > +						ADUX1020_LED_CURRENT_MASK,
> > i);
> > +				}
> > +			}
> 
> Same comment as [5]
> 
> > +		}
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int adux1020_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 adux1020_data *data = iio_priv(indio_dev);
> > +
> > +	regmap_write(data->regmap, ADUX1020_REG_INT_ENABLE,
> > +		     ADUX1020_INT_ENABLE);
> > +
> 
> Regarding [4]: it looks like the lock could be useful here.
> Regarding [3]: more error checking could be useful.
> 
> 
> > +	regmap_write(data->regmap, ADUX1020_REG_INT_POLARITY, 0);
> > +
> > +	switch (chan->type) {
> > +	case IIO_PROXIMITY:
> > +		if (dir == IIO_EV_DIR_RISING) {
> > +			regmap_update_bits(data->regmap,
> > ADUX1020_REG_INT_MASK,
> > +					   ADUX1020_PROX_ON1_INT,
> > +					   state ? 0 :
> > ADUX1020_PROX_ON1_INT);
> > +		} else {
> > +			regmap_update_bits(data->regmap,
> > ADUX1020_REG_INT_MASK,
> > +					   ADUX1020_PROX_OFF1_INT,
> > +					   state ? 0 :
> > ADUX1020_PROX_OFF1_INT);
> > +		}
> 
> [6] More about style/preference.
> I like the part in adux1020_read_event_config(), where
> 
> -------------------------------------------------
>   if (dir == IIO_EV_DIR_RISING)
>       mask = ADUX1020_PROX_ON1_INT;
>   else
>       mask = ADUX1020_PROX_OFF1_INT;
> -------------------------------------------------
> 
> Then this could become:
> 
> -------------------------------------------------
> if (state)
>     state = mask;
> else
>     state = 0;
> 
> ret = regmap_update_bits(data->regmap,
>                          ADUX1020_REG_INT_MASK,
>                          mask, state);
> -------------------------------------------------
> 

looks good, will implement.

> 
> > +
> > +		/*
> > +		 * Trigger proximity interrupt when the intensity is above
> > +		 * or below threshold
> > +		 */
> > +		regmap_update_bits(data->regmap, ADUX1020_REG_PROX_TYPE,
> > +				   ADUX1020_PROX_TYPE, ADUX1020_PROX_TYPE);
> > +
> > +		/* Set proximity mode */
> > +		adux1020_set_mode(data, ADUX1020_MODE_PROX_I);
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int adux1020_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 adux1020_data *data = iio_priv(indio_dev);
> > +	int ret, mask;
> > +	unsigned int regval;
> > +
> > +	switch (chan->type) {
> > +	case IIO_PROXIMITY:
> > +		if (dir == IIO_EV_DIR_RISING)
> > +			mask = ADUX1020_PROX_ON1_INT;
> > +		else
> > +			mask = ADUX1020_PROX_OFF1_INT;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = regmap_read(data->regmap, ADUX1020_REG_INT_MASK, &regval);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return !(regval & mask);
> > +}
> > +
> > +static int adux1020_read_thresh(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 adux1020_data *data = iio_priv(indio_dev);
> > +	u8 reg;
> > +	int ret;
> > +	unsigned int regval;
> > +
> > +	switch (chan->type) {
> > +	case IIO_PROXIMITY:
> > +		if (dir == IIO_EV_DIR_RISING)
> > +			reg = ADUX1020_REG_PROX_TH_ON1;
> > +		else
> > +			reg = ADUX1020_REG_PROX_TH_OFF1;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = regmap_read(data->regmap, reg, &regval);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	*val = regval;
> > +
> > +	return IIO_VAL_INT;
> > +}
> > +
> > +static int adux1020_write_thresh(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 adux1020_data *data = iio_priv(indio_dev);
> > +	u8 reg;
> > +
> > +	switch (chan->type) {
> > +	case IIO_PROXIMITY:
> > +		if (dir == IIO_EV_DIR_RISING)
> > +			reg = ADUX1020_REG_PROX_TH_ON1;
> > +		else
> > +			reg = ADUX1020_REG_PROX_TH_OFF1;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Full scale threshold value is 0-65535  */
> > +	if (val < 0 || val > 65535)
> > +		return -EINVAL;
> > +
> > +	return regmap_write(data->regmap, reg, val);
> > +}
> > +
> > +static const struct iio_event_spec adux1020_proximity_event[] = {
> > +	{
> > +		.type = IIO_EV_TYPE_THRESH,
> > +		.dir = IIO_EV_DIR_RISING,
> > +		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> > +			BIT(IIO_EV_INFO_ENABLE),
> > +	},
> > +	{
> > +		.type = IIO_EV_TYPE_THRESH,
> > +		.dir = IIO_EV_DIR_FALLING,
> > +		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> > +			BIT(IIO_EV_INFO_ENABLE),
> > +	},
> > +};
> > +
> > +static const struct iio_chan_spec adux1020_channels[] = {
> > +	{
> > +		.type = IIO_PROXIMITY,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +				      BIT(IIO_CHAN_INFO_SAMP_FREQ),
> > +		.event_spec = adux1020_proximity_event,
> > +		.num_event_specs = ARRAY_SIZE(adux1020_proximity_event),
> > +	},
> > +	{
> > +		.type = IIO_CURRENT,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > +		.extend_name = "led",
> > +	},
> > +};
> > +
> > +static IIO_CONST_ATTR(sampling_frequency_available,
> > +		      "0.1 0.2 0.5 1 2 5 10 20 50 100 190 450 820 1400");
> 
> This could re-use the IIO_CONST_ATTR_SAMP_FREQ_AVAIL() macro.
> 

ack.

> > +
> > +static struct attribute *adux1020_attributes[] = {
> > +	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
> > +	NULL
> > +};
> > +
> > +static const struct attribute_group adux1020_attribute_group = {
> > +	.attrs = adux1020_attributes,
> > +};
> > +
> > +static const struct iio_info adux1020_info = {
> > +	.attrs = &adux1020_attribute_group,
> > +	.read_raw = adux1020_read_raw,
> > +	.write_raw = adux1020_write_raw,
> > +	.read_event_config = adux1020_read_event_config,
> > +	.write_event_config = adux1020_write_event_config,
> > +	.read_event_value = adux1020_read_thresh,
> > +	.write_event_value = adux1020_write_thresh,
> > +};
> > +
> > +static irqreturn_t adux1020_interrupt_handler(int irq, void *private)
> > +{
> > +	struct iio_dev *indio_dev = private;
> > +	struct adux1020_data *data = iio_priv(indio_dev);
> > +	int ret, status;
> > +
> > +	ret = regmap_read(data->regmap, ADUX1020_REG_INT_STATUS, &status);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	status &= ADUX1020_MODE_INT_STATUS_MASK;
> > +
> > +	if (status & ADUX1020_INT_PROX_ON1) {
> > +		iio_push_event(indio_dev,
> > +				IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
> > +					IIO_EV_TYPE_THRESH,
> > +					IIO_EV_DIR_RISING),
> > +				iio_get_time_ns(indio_dev));
> > +	}
> > +
> > +	if (status & ADUX1020_INT_PROX_OFF1) {
> > +		iio_push_event(indio_dev,
> > +				IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
> > +					IIO_EV_TYPE_THRESH,
> > +					IIO_EV_DIR_FALLING),
> > +				iio_get_time_ns(indio_dev));
> > +	}
> > +
> > +	regmap_update_bits(data->regmap, ADUX1020_REG_INT_STATUS,
> > +			   ADUX1020_MODE_INT_MASK, status);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int adux1020_chip_init(struct adux1020_data *data)
> > +{
> > +	struct i2c_client *client = data->client;
> > +	int ret, i;
> > +	unsigned int val;
> > +
> > +	ret = regmap_read(data->regmap, ADUX1020_REG_CHIP_ID, &val);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	val &= ADUX1020_CHIP_ID_MASK;
> > +
> > +	if (val != ADUX1020_CHIP_ID) {
> > +		dev_err(&client->dev, "invalid chip id 0x%04x\n", val);
> > +		return -ENODEV;
> > +	};
> > +
> > +	dev_dbg(&client->dev, "Detected ADUX1020 with chip id: 0x%04x\n",
> > val);
> > +
> > +	/* Perform software reset */
> > +	regmap_update_bits(data->regmap, ADUX1020_REG_SW_RESET,
> > +			   ADUX1020_SW_RESET, ADUX1020_SW_RESET);
> > +
> > +	/* Load default configuration */
> > +	for (i = 0; i < ARRAY_SIZE(adux1020_def_conf); i++)
> > +		regmap_write(data->regmap, adux1020_def_conf[i][0],
> > +			     adux1020_def_conf[i][1]);
> > +
> > +	adux1020_flush_fifo(data);
> > +
> > +	/* Use LED_IREF for proximity mode */
> > +	regmap_update_bits(data->regmap, ADUX1020_REG_LED_CURRENT,
> > +			   ADUX1020_LED_PIREF_EN, 0);
> > +
> > +	/* Mask all interrupts */
> > +	regmap_update_bits(data->regmap, ADUX1020_REG_INT_MASK,
> > +			   ADUX1020_MODE_INT_MASK,
> > ADUX1020_MODE_INT_DISABLE);
> 
> Same as [3]: a bit more error checking here could be useful.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int adux1020_probe(struct i2c_client *client,
> > +			  const struct i2c_device_id *id)
> > +{
> > +	struct adux1020_data *data;
> > +	struct iio_dev *indio_dev;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	indio_dev->dev.parent = &client->dev;
> > +	indio_dev->info = &adux1020_info;
> > +	indio_dev->name = ADUX1020_DRV_NAME;
> > +	indio_dev->channels = adux1020_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(adux1020_channels);
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +	data = iio_priv(indio_dev);
> > +	i2c_set_clientdata(client, indio_dev);
> > +
> > +	data->regmap = devm_regmap_init_i2c(client,
> > &adux1020_regmap_config);
> > +	if (IS_ERR(data->regmap)) {
> > +		dev_err(&client->dev, "regmap initialization failed.\n");
> > +		return PTR_ERR(data->regmap);
> > +	}
> > +
> > +	data->client = client;
> > +	data->indio_dev = indio_dev;
> > +	mutex_init(&data->lock);
> > +
> > +	ret = adux1020_chip_init(data);
> > +	if (ret)
> > +		goto err_out;
> > +
> > +	if (client->irq) {
> > +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> > +				NULL, adux1020_interrupt_handler,
> > +				IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> > +				ADUX1020_DRV_NAME, indio_dev);
> > +		if (ret) {
> > +			dev_err(&client->dev, "irq request error %d\n",
> > -ret);
> > +			goto err_out;
> > +		}
> > +	}
> > +
> > +	ret = iio_device_register(indio_dev);
> 
> [7] Jonathan may correct me here.
> If this doesn't need to do anything more later, this could use
> devm_iio_device_register(), and then the  adux1020_remove() hook could be
> removed.
> 

right. will switch to devm_ API.

Thanks,
Mani

> > +	if (ret) {
> > +		dev_err(&client->dev, "Failed to register IIO device\n");
> > +		goto err_out;
> > +	}
> > +
> > +	return 0;
> > +
> > +err_out:
> > +	return ret;
> > +}
> > +
> > +static int adux1020_remove(struct i2c_client *client)
> > +{
> > +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > +
> > +	iio_device_unregister(indio_dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct i2c_device_id adux1020_id[] = {
> > +	{ "adux1020", 0 },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(i2c, adux1020_id);
> > +
> > +static const struct of_device_id adux1020_of_match[] = {
> > +	{ .compatible = "adi,adux1020" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, adux1020_of_match);
> > +
> > +static struct i2c_driver adux1020_driver = {
> > +	.driver = {
> > +		.name	= ADUX1020_DRV_NAME,
> > +		.of_match_table = adux1020_of_match,
> > +	},
> > +	.probe		= adux1020_probe,
> > +	.remove		= adux1020_remove,
> > +	.id_table	= adux1020_id,
> > +};
> > +module_i2c_driver(adux1020_driver);
> > +
> > +MODULE_AUTHOR("Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>"
> > );
> > +MODULE_DESCRIPTION("ADUX1020 photometric sensor");
> > +MODULE_LICENSE("GPL");

Re: [PATCH 2/2] iio: light: Add support for ADUX1020 sensor

[Ardelean, Alexandru]

On Wed, 2019-10-09 at 15:15 +0530, Manivannan Sadhasivam wrote:
> [External]
> 
> Hi Ardelean,
> 
> Thanks for the quick review!
> 
> On Tue, Oct 08, 2019 at 06:52:50AM +0000, Ardelean, Alexandru wrote:
> > On Mon, 2019-10-07 at 15:40 +0530, Manivannan Sadhasivam wrote:
> > > [External]
> > > 
> > 
> > Hey,
> > 
> > Comments inline.
> > 
> > I thought I sent an initial review, but seems to have gotten lost
> > [maybe in
> > my email client].
> > Oh well. I managed to re-do it anyway.
> > 
> > I tried to group them this time.
> > 
> > The more prominent part is [3]; this driver needs a bit more error
> > checking
> > on regmap_() returns.
> > 
> 
> Yes, agree. I forgot that I'm not working on memory mapped region ;-)
> 
> > Generally some notes:
> > - Is there a need to implement the 32Khz or 32Mhz clock calibration
> > routines on startup? Some drivers need this, some don't/
> 
> Calibration is required to have the precise reading but it is not a
> blocker.
> We can add it later.

Fine from my side.
I should have also mentioned this earlier [that the calibration can be
added later].

> 
> > - From the functional diagram, it looks like maybe the VREF would be
> > needed
> > to be hooked via a regulator framework; but this could be done later
> 
> Right but the reference board schematics is not very clear about VREF and
> neither the sensor datasheet. That's why I intentionally left it. Will
> get
> in touch with the board vendor to figure out what is the recommended VREF
> voltage and submit a patch later.

ack;
well, typically [for the driver] VREF support is just added in the driver
and then the board device-tree just implements what it needs [that is, if
it needs it];

some board designs just have a fixed value, and the driver does not need to
care/know about the VREF regulator;
the idea of adding regulator support in the driver is to allow the driver
to initialize it [if there is a device-tree entry for it]

but this can be done later;

> 
> > - Just curios here: there is gesture mode as well; will that be
> > implemented
> > later? Or will there be other modes implemented?
> 
> Currently only proximity mode is implemented. There are gesture and
> sample
> modes and I left those as a TODO. But I'm not sure whether IIO is
> supporting
> gesture mode properly or not.

I don't have any input on this at the moment [about gesture support & IIO].
I'd have to investigate.
Maybe Jonathan has some thoughts.

> 
> > If I remember anything else I may come back with a reply.
> > 
> 
> Sure.
> 
> > Thanks
> > Alex
> > 
> > > Add initial support for Analog Devices ADUX1020 Photometric sensor.
> > > Only proximity mode has been enabled for now.
> > > 
> > > Signed-off-by: Manivannan Sadhasivam <
> > > manivannan.sadhasivam@linaro.org>
> > > ---
> > >  drivers/iio/light/Kconfig    |  11 +
> > >  drivers/iio/light/Makefile   |   1 +
> > >  drivers/iio/light/adux1020.c | 783
> > > +++++++++++++++++++++++++++++++++++
> > 
> > Does MAINTAINERS need updating as well?
> > 
> 
> I don't prefer to have MAINTAINERS entry for small drivers like this.
> Anyway, get_maintainers will return my mailing address based on the
> commit signing.

ack

> 
> > >  3 files changed, 795 insertions(+)
> > >  create mode 100644 drivers/iio/light/adux1020.c
> > > 
> > > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> > > index 08d7e1ef2186..3f8c8689cd89 100644
> > > --- a/drivers/iio/light/Kconfig
> > > +++ b/drivers/iio/light/Kconfig
> > > @@ -32,6 +32,17 @@ config ADJD_S311
> > >  	  This driver can also be built as a module.  If so, the module
> > >  	  will be called adjd_s311.
> > >  
> > > +config ADUX1020
> > > +	tristate "ADUX1020 photometric sensor"
> > > +	select REGMAP_I2C
> > > +	depends on I2C
> > > +	help
> > > +	 Say Y here if you want to build a driver for the Analog Devices
> > > +	 ADUX1020 photometric sensor.
> > > +
> > > +	 To compile this driver as a module, choose M here: the
> > > +	 module will be called adux1020.
> > > +
> > >  config AL3320A
> > >  	tristate "AL3320A ambient light sensor"
> > >  	depends on I2C
> > > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> > > index 00d1f9b98f39..5d650ce46a40 100644
> > > --- a/drivers/iio/light/Makefile
> > > +++ b/drivers/iio/light/Makefile
> > > @@ -6,6 +6,7 @@
> > >  # When adding new entries keep the list in alphabetical order
> > >  obj-$(CONFIG_ACPI_ALS)		+= acpi-als.o
> > >  obj-$(CONFIG_ADJD_S311)		+= adjd_s311.o
> > > +obj-$(CONFIG_ADUX1020)		+= adux1020.o
> > >  obj-$(CONFIG_AL3320A)		+= al3320a.o
> > >  obj-$(CONFIG_APDS9300)		+= apds9300.o
> > >  obj-$(CONFIG_APDS9960)		+= apds9960.o
> > > diff --git a/drivers/iio/light/adux1020.c
> > > b/drivers/iio/light/adux1020.c
> > > new file mode 100644
> > > index 000000000000..d0b76e5b44f1
> > > --- /dev/null
> > > +++ b/drivers/iio/light/adux1020.c
> > > @@ -0,0 +1,783 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * adux1020.c - Support for Analog Devices ADUX1020 photometric
> > > sensor
> > 
> > Maybe drop the adux1020.c part?
> > I think something like this should be sufficient:
> > "Analog Devices ADUX1020 photometric sensor"
> > 
> 
> okay.
> 
> > > + *
> > > + * Copyright (C) 2019 Linaro Ltd.
> > > + * Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > + *
> > > + * TODO: Triggered buffer support
> > 
> > Maybe drop the TODO?
> > It's not needed for mainline.
> > 
> 
> It is not needed but it can be added. TODO is required to explicitly
> mention what are all left in the driver which can be picked up by the
> community. So I prefer to keep it.

ack; no strong preference

>  
> > > + */
> > > +
> > > +#include <linux/delay.h>
> > > +#include <linux/err.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/init.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/irq.h>
> > > +#include <linux/module.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/regmap.h>
> > > +
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/iio/sysfs.h>
> > > +#include <linux/iio/events.h>
> > > +
> > > +#define ADUX1020_REGMAP_NAME		"adux1020_regmap"
> > > +#define ADUX1020_DRV_NAME		"adux1020"
> > > +
> > > +/* System registers */
> > > +#define ADUX1020_REG_CHIP_ID		0x08
> > > +#define ADUX1020_REG_SLAVE_ADDRESS	0x09
> > > +
> > > +#define ADUX1020_REG_SW_RESET		0x0f
> > > +#define ADUX1020_REG_INT_ENABLE		0x1c
> > > +#define ADUX1020_REG_INT_POLARITY	0x1d
> > > +#define ADUX1020_REG_PROX_TH_ON1	0x2a
> > > +#define ADUX1020_REG_PROX_TH_OFF1	0x2b
> > > +#define	ADUX1020_REG_PROX_TYPE		0x2f
> > > +#define	ADUX1020_REG_TEST_MODES_3	0x32
> > > +#define	ADUX1020_REG_FORCE_MODE		0x33
> > > +#define	ADUX1020_REG_FREQUENCY		0x40
> > > +#define ADUX1020_REG_LED_CURRENT	0x41
> > > +#define	ADUX1020_REG_OP_MODE		0x45
> > > +#define	ADUX1020_REG_INT_MASK		0x48
> > > +#define	ADUX1020_REG_INT_STATUS		0x49
> > > +#define	ADUX1020_REG_DATA_BUFFER	0x60
> > > +
> > > +/* Chip ID bits */
> > > +#define ADUX1020_CHIP_ID_MASK		GENMASK(11, 0)
> > > +#define ADUX1020_CHIP_ID		0x03fc
> > > +
> > > +#define ADUX1020_MODE_OUT_SHIFT		4
> > 
> > I'm seeing a few _SHIFT macros.
> > Maybe use the FIELD_PREP() macro where possible? [1]
> > 
> 
> okay.
> 
> > > +#define ADUX1020_MODE_OUT_PROX_I	1
> > > +#define ADUX1020_MODE_OUT_PROX_XY	3
> > > +
> > > +#define ADUX1020_SW_RESET		BIT(1)
> > > +#define ADUX1020_FIFO_FLUSH		BIT(15)
> > > +#define ADUX1020_OP_MODE_MASK		GENMASK(3, 0)
> > > +#define ADUX1020_DATA_OUT_MODE_MASK	GENMASK(7, 4)
> > > +
> > > +#define ADUX1020_MODE_INT_MASK		GENMASK(7, 0)
> > > +#define ADUX1020_INT_ENABLE		0x2096
> > > +#define ADUX1020_INT_DISABLE		0x2090
> > > +#define ADUX1020_PROX_INT_ENABLE	0x00f0
> > > +#define ADUX1020_PROX_ON1_INT		BIT(0)
> > > +#define ADUX1020_PROX_OFF1_INT		BIT(1)
> > > +#define ADUX1020_FIFO_INT_ENABLE	0x7f
> > > +#define ADUX1020_MODE_INT_DISABLE	0xff
> > > +#define ADUX1020_MODE_INT_STATUS_MASK	GENMASK(7, 0)
> > > +#define ADUX1020_FIFO_STATUS_MASK	GENMASK(15, 8)
> > > +#define ADUX1020_PROX_TYPE		BIT(15)
> > > +
> > > +#define ADUX1020_INT_PROX_ON1		BIT(0)
> > > +#define ADUX1020_INT_PROX_OFF1		BIT(1)
> > > +
> > > +#define	ADUX1020_FORCE_CLOCK_ON		0x0f4f
> > > +#define	ADUX1020_FORCE_CLOCK_RESET	0x0040
> > 
> > nitpick: indentation seems inconsistent here
> > 
> 
> will fix it.
> 
> > > +#define ADUX1020_ACTIVE_4_STATE		0x0008
> > > +
> > > +#define ADUX1020_PROX_FREQ_MASK		GENMASK(7, 4)
> > > +#define ADUX1020_PROX_FREQ_SHIFT	4
> > 
> > Same as [1]. Maybe use FIELD_PREP() together with
> > ADUX1020_PROX_FREQ_MASK?
> > See other examples, but we sometimes use them as:
> > 
> > #define ADUX_PROX_FREQ_SET(x)    FIELD_PREP(ADUX1020_PROX_FREQ_MASK,
> > (x))
> > 
> 
> okay.
> 
> > > +
> > > +#define ADUX1020_LED_CURRENT_MASK	GENMASK(3, 0)
> > > +#define ADUX1020_LED_PIREF_EN		BIT(12)
> > > +
> > > +/* Operating modes */
> > > +enum adux1020_op_modes {
> > > +	ADUX1020_MODE_STANDBY,
> > > +	ADUX1020_MODE_PROX_I,
> > > +	ADUX1020_MODE_PROX_XY,
> > > +	ADUX1020_MODE_GEST,
> > > +	ADUX1020_MODE_SAMPLE,
> > > +	ADUX1020_MODE_FORCE = 0x0e,
> > > +	ADUX1020_MODE_IDLE = 0x0f,
> > > +};
> > > +
> > > +struct adux1020_data {
> > > +	struct i2c_client *client;
> > > +	struct iio_dev *indio_dev;
> > > +	struct mutex lock;
> > > +	struct regmap *regmap;
> > > +};
> > > +
> > > +struct adux1020_mode_data {
> > > +	u8 bytes;
> > > +	u8 buf_len;
> > > +	u16 int_en;
> > > +};
> > > +
> > > +static const struct adux1020_mode_data adux1020_modes[] = {
> > > +	[ADUX1020_MODE_PROX_I] = {
> > > +		.bytes = 2,
> > > +		.buf_len = 1,
> > > +		.int_en = ADUX1020_PROX_INT_ENABLE,
> > > +	},
> > > +};
> > > +
> > > +static const struct regmap_config adux1020_regmap_config = {
> > > +	.name = ADUX1020_REGMAP_NAME,
> > > +	.reg_bits = 8,
> > > +	.val_bits = 16,
> > > +	.max_register = 0x6F,
> > > +	.cache_type = REGCACHE_NONE,
> > > +};
> > > +
> > > +static const int adux1020_def_conf[][2] = {
> > 
> > Maybe use "struct reg_sequence" here?[2]
> > You could then use "regmap_multi_reg_write()"
> > 
> 
> ack.
> 
> > > +	{ 0x000c, 0x000f },
> > > +	{ 0x0010, 0x1010 },
> > > +	{ 0x0011, 0x004c },
> > > +	{ 0x0012, 0x5f0c },
> > > +	{ 0x0013, 0xada5 },
> > > +	{ 0x0014, 0x0080 },
> > > +	{ 0x0015, 0x0000 },
> > > +	{ 0x0016, 0x0600 },
> > > +	{ 0x0017, 0x0000 },
> > > +	{ 0x0018, 0x2693 },
> > > +	{ 0x0019, 0x0004 },
> > > +	{ 0x001a, 0x4280 },
> > > +	{ 0x001b, 0x0060 },
> > > +	{ 0x001c, 0x2094 },
> > > +	{ 0x001d, 0x0020 },
> > > +	{ 0x001e, 0x0001 },
> > > +	{ 0x001f, 0x0100 },
> > > +	{ 0x0020, 0x0320 },
> > > +	{ 0x0021, 0x0A13 },
> > > +	{ 0x0022, 0x0320 },
> > > +	{ 0x0023, 0x0113 },
> > > +	{ 0x0024, 0x0000 },
> > > +	{ 0x0025, 0x2412 },
> > > +	{ 0x0026, 0x2412 },
> > > +	{ 0x0027, 0x0022 },
> > > +	{ 0x0028, 0x0000 },
> > > +	{ 0x0029, 0x0300 },
> > > +	{ 0x002a, 0x0700 },
> > > +	{ 0x002b, 0x0600 },
> > > +	{ 0x002c, 0x6000 },
> > > +	{ 0x002d, 0x4000 },
> > > +	{ 0x002e, 0x0000 },
> > > +	{ 0x002f, 0x0000 },
> > > +	{ 0x0030, 0x0000 },
> > > +	{ 0x0031, 0x0000 },
> > > +	{ 0x0032, 0x0040 },
> > > +	{ 0x0033, 0x0008 },
> > > +	{ 0x0034, 0xE400 },
> > > +	{ 0x0038, 0x8080 },
> > > +	{ 0x0039, 0x8080 },
> > > +	{ 0x003a, 0x2000 },
> > > +	{ 0x003b, 0x1f00 },
> > > +	{ 0x003c, 0x2000 },
> > > +	{ 0x003d, 0x2000 },
> > > +	{ 0x003e, 0x0000 },
> > > +	{ 0x0040, 0x8069 },
> > > +	{ 0x0041, 0x1f2f },
> > > +	{ 0x0042, 0x4000 },
> > > +	{ 0x0043, 0x0000 },
> > > +	{ 0x0044, 0x0008 },
> > > +	{ 0x0046, 0x0000 },
> > > +	{ 0x0048, 0x00ef },
> > > +	{ 0x0049, 0x0000 },
> > > +	{ 0x0045, 0x0000 },
> > > +};
> > > +
> > > +static const int adux1020_rate[][2] = {
> > > +	{ 0, 100000 },
> > > +	{ 0, 200000 },
> > > +	{ 0, 500000 },
> > > +	{ 1, 0 },
> > > +	{ 2, 0 },
> > > +	{ 5, 0 },
> > > +	{ 10, 0 },
> > > +	{ 20, 0 },
> > > +	{ 50, 0 },
> > > +	{ 100, 0 },
> > > +	{ 190, 0 },
> > > +	{ 450, 0 },
> > > +	{ 820, 0 },
> > > +	{ 1400, 0 },
> > > +};
> > > +
> > > +static const int adux1020_led_current[][2] = {
> > > +	{ 0, 25000 },
> > > +	{ 0, 40000 },
> > > +	{ 0, 55000 },
> > > +	{ 0, 70000 },
> > > +	{ 0, 85000 },
> > > +	{ 0, 100000 },
> > > +	{ 0, 115000 },
> > > +	{ 0, 130000 },
> > > +	{ 0, 145000 },
> > > +	{ 0, 160000 },
> > > +	{ 0, 175000 },
> > > +	{ 0, 190000 },
> > > +	{ 0, 205000 },
> > > +	{ 0, 220000 },
> > > +	{ 0, 235000 },
> > > +	{ 0, 250000 },
> > > +};
> > > +
> > > +static void adux1020_flush_fifo(struct adux1020_data *data)
> > > +{
> > > +	/* Force Idle mode */
> > > +	regmap_write(data->regmap, ADUX1020_REG_FORCE_MODE,
> > > +		     ADUX1020_ACTIVE_4_STATE);
> > > +	regmap_update_bits(data->regmap, ADUX1020_REG_OP_MODE,
> > > +			   ADUX1020_OP_MODE_MASK, ADUX1020_MODE_FORCE);
> > > +	regmap_update_bits(data->regmap, ADUX1020_REG_OP_MODE,
> > > +			   ADUX1020_OP_MODE_MASK, ADUX1020_MODE_IDLE);
> > > +
> > > +	/* Flush FIFO */
> > > +	regmap_write(data->regmap, ADUX1020_REG_TEST_MODES_3,
> > > +		     ADUX1020_FORCE_CLOCK_ON);
> > > +	regmap_write(data->regmap, ADUX1020_REG_INT_STATUS,
> > > +		     ADUX1020_FIFO_FLUSH);
> > > +	regmap_write(data->regmap, ADUX1020_REG_TEST_MODES_3,
> > > +		     ADUX1020_FORCE_CLOCK_RESET);
> > 
> > These bits could use with some minimal error checking.[3]
> > Maybe also convert to return int and check errors.
> > 
> 
> Yes, I will added error check in relevant places.
> 
> > > +}
> > > +
> > > +static int adux1020_read_fifo(struct adux1020_data *data, u16 *buf,
> > > u8
> > > buf_len)
> > > +{
> > > +	int i, ret = -EINVAL;
> > > +	unsigned int regval;
> > > +
> > > +	/* Enable 32MHz clock */
> > > +	regmap_write(data->regmap, ADUX1020_REG_TEST_MODES_3,
> > > +		     ADUX1020_FORCE_CLOCK_ON);
> > 
> > This looks weird [the FORCE_CLOCK_ON], but it is what the datasheet
> > says.
> > Also, related to [3]: some error checking this return would be useful.
> > 
> 
> ack.
> 
> > > +
> > > +	for (i = 0; i < buf_len; i++) {
> > > +		ret = regmap_read(data->regmap, ADUX1020_REG_DATA_BUFFER,
> > > +				      &regval);
> > > +		if (ret < 0)
> > > +			goto err_out;
> > > +
> > > +		buf[i] = regval;
> > > +	}
> > > +
> > > +	/* Set 32MHz clock to be controlled by internal state machine */
> > > +	regmap_write(data->regmap, ADUX1020_REG_TEST_MODES_3,
> > > +		     ADUX1020_FORCE_CLOCK_RESET);
> > > +
> > > +err_out:
> > > +	return ret;
> > > +}
> > > +
> > > +static void adux1020_set_mode(struct adux1020_data *data,
> > > +			      enum adux1020_op_modes mode)
> > > +{
> > > +	/* Switch to standby mode before changing the mode */
> > > +	regmap_write(data->regmap, ADUX1020_REG_OP_MODE,
> > > ADUX1020_MODE_STANDBY);
> > 
> > [3] error checking maybe? and maybe int return?
> > 
> 
> ack.
> 
> > > +
> > > +	/* Set data out and switch to the desired mode */
> > > +	if (mode == ADUX1020_MODE_PROX_I) {
> > 
> > This could become a switch() statement, so that when/if other modes get
> > added, the patch looks cleaner.
> > 
> 
> okay.
> 
> > > +		regmap_update_bits(data->regmap, ADUX1020_REG_OP_MODE,
> > > +			ADUX1020_DATA_OUT_MODE_MASK,
> > > +			ADUX1020_MODE_OUT_PROX_I <<
> > > ADUX1020_MODE_OUT_SHIFT);
> > 
> > Related to [1]: a FIELD_PREP() macro would be useful.
> > 
> 
> okay.
> 
> > > +		regmap_update_bits(data->regmap, ADUX1020_REG_OP_MODE,
> > > +			ADUX1020_OP_MODE_MASK, ADUX1020_MODE_PROX_I);
> > > +	}
> > > +}
> > > +
> > > +static int adux1020_measure(struct adux1020_data *data,
> > > +			    enum adux1020_op_modes mode,
> > > +			    u16 *val)
> > > +{
> > > +	int ret, tries = 50;
> > > +	unsigned int status;
> > > +
> > > +	mutex_lock(&data->lock);
> > 
> > The scope of this lock looks like it would need to be extended a bit.
> > See [4] on some more notes for this.
> > 
> > > +
> > > +	/* Disable INT pin as polling is going to be used */
> > > +	regmap_write(data->regmap, ADUX1020_REG_INT_ENABLE,
> > > +		     ADUX1020_INT_DISABLE);
> > > +
> > > +	/* Enable mode interrupt */
> > > +	regmap_update_bits(data->regmap, ADUX1020_REG_INT_MASK,
> > > +			   ADUX1020_MODE_INT_MASK,
> > > +			   adux1020_modes[mode].int_en);
> > 
> > [3] maybe some more error checking?
> > 
> 
> ack.
> 
> > > +
> > > +	while (tries--) {
> > > +		ret = regmap_read(data->regmap, ADUX1020_REG_INT_STATUS,
> > > +				  &status);
> > > +		if (ret < 0)
> > > +			goto fail;
> > > +
> > > +		status &= ADUX1020_FIFO_STATUS_MASK;
> > > +		if (status >= adux1020_modes[mode].bytes)
> > > +			break;
> > > +		msleep(20);
> > > +	}
> > > +
> > > +	if (tries < 0) {
> > > +		ret = -EIO;
> > > +		goto fail;
> > > +	}
> > > +
> > 
> > Regarding [8], the buffer is passed from adux1020_read_raw(), so maybe
> > just
> > move the buffer here, since the information about size is near the
> > buffer
> > definition.
> > Then do *val = buf[0];
> > 
> 
> I don't see any benefit from this. As said below, the buffer size is
> fixed
> and is based on the maximum hardware FIFO size. It goes upto 6 bytes
> (3-16bit words).

I guess in this case [since the buffer size is fixed], I can agree with
that.
This comment stems from a habit developed from also writing API code that
you never know how it will be used [or misused] by users.
Maybe, in this case, it could be ok leave this as-is, as we can follow the
buffer through the calls/uses.

> 
> > > +	ret = adux1020_read_fifo(data, val, adux1020_modes[mode].buf_len);
> > > +	if (ret < 0)
> > > +		goto fail;
> > > +
> > > +	/* Clear mode interrupt */
> > > +	regmap_write(data->regmap, ADUX1020_REG_INT_STATUS,
> > > +			   (~adux1020_modes[mode].int_en));
> > > +	/* Disable mode interrupts */
> > > +	regmap_update_bits(data->regmap, ADUX1020_REG_INT_MASK,
> > > +			   ADUX1020_MODE_INT_MASK,
> > > ADUX1020_MODE_INT_DISABLE);
> > > +
> > > +fail:
> > > +	mutex_unlock(&data->lock);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int adux1020_read_raw(struct iio_dev *indio_dev,
> > > +			     struct iio_chan_spec const *chan,
> > > +			     int *val, int *val2, long mask)
> > > +{
> > > +	struct adux1020_data *data = iio_priv(indio_dev);
> > > +	u16 buf[3];
> > 
> > This buffer looks a bit weird. [8]
> > It's 3 elements-wide and passed without any information about size.
> > And only the first element is used.
> > So, maybe just convert u16 buf[3] -> u16 buf?
> > 
> 
> The buffer declaration is based on the hardware buffer available. It
> is 3 elements wide since the remaining 2 elements will be used by other
> modes. The idea here is to reuse the adux1020_measure() API for all 3
> modes (which has varying buffer sizes).

The only thought I have left about this buffer [and forgot to mention it
earlier], is whether this should be cacheline aligned [or not].
If it has to be, then maybe it shouldn't be stored on the stack and moved
to a malloc-ed buffer [on "struct adux1020_data"].
Cacheline aligned stuff typically deals with potential DMA issues. The DMA
issues [in this case] could be coming from i2c controllers that can do DMA.

Jonathan may have more input here.

> 
> > > +	int ret = -EINVAL;
> > > +	unsigned int regval;
> > > +
> > > +	switch (mask) {
> > > +	case IIO_CHAN_INFO_RAW:
> > > +		switch (chan->type) {
> > > +		case IIO_PROXIMITY:
> > > +			adux1020_set_mode(data, ADUX1020_MODE_PROX_I);
> > > +			ret = adux1020_measure(data, ADUX1020_MODE_PROX_I,
> > > buf);
> > 
> > Regarding [4]: I'm thinking that the lock could be extended to
> > adux1020_{read,write}_raw() & adux1020_{read,write}_event_config()
> > functions, especialy [or only in places] where functions seem to do
> > consecutive R/W ops.
> > 
> 
> okay.
> 
> > > +			if (ret < 0)
> > > +				return ret;
> > > +
> > > +			*val = buf[0];
> > > +			ret = IIO_VAL_INT;
> > > +			break;
> > > +		default:
> > > +			break;
> > > +		}
> > > +		break;
> > > +	case IIO_CHAN_INFO_PROCESSED:
> > > +		switch (chan->type) {
> > > +		case IIO_CURRENT:
> > > +			ret = regmap_read(data->regmap,
> > > +					  ADUX1020_REG_LED_CURRENT,
> > > &regval);
> > > +			if (ret < 0)
> > > +				return ret;
> > > +
> > > +			regval = regval & ADUX1020_LED_CURRENT_MASK;
> > > +
> > > +			*val = adux1020_led_current[regval][0];
> > > +			*val2 = adux1020_led_current[regval][1];
> > > +
> > > +			ret = IIO_VAL_INT_PLUS_MICRO;
> > > +			break;
> > > +		default:
> > > +			break;
> > > +		}
> > > +		break;
> > > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > > +		switch (chan->type) {
> > > +		case IIO_PROXIMITY:
> > > +			ret = regmap_read(data->regmap,
> > > ADUX1020_REG_FREQUENCY,
> > > +					  &regval);
> > > +			if (ret < 0)
> > > +				return ret;
> > > +
> > > +			regval = (regval & ADUX1020_PROX_FREQ_MASK) >>
> > > +				  ADUX1020_PROX_FREQ_SHIFT;
> > 
> > Related to [1]: a FIELD_PREP() macro would be useful.
> > 
> 
> ack.
> 
> > > +
> > > +			*val = adux1020_rate[regval][0];
> > > +			*val2 = adux1020_rate[regval][1];
> > > +
> > > +			ret = IIO_VAL_INT_PLUS_MICRO;
> > > +			break;
> > > +		default:
> > > +			break;
> > > +		}
> > > +		break;
> > > +	default:
> > > +		break;
> > > +	}
> > > +
> > > +	return ret;
> > > +};
> > > +
> > > +static int adux1020_write_raw(struct iio_dev *indio_dev,
> > > +			     struct iio_chan_spec const *chan,
> > > +			     int val, int val2, long mask)
> > > +{
> > > +	struct adux1020_data *data = iio_priv(indio_dev);
> > > +	int i, ret = -EINVAL;
> > > +
> > > +	switch (mask) {
> > > +	case IIO_CHAN_INFO_SAMP_FREQ:
> > > +		if (chan->type == IIO_PROXIMITY) {
> > > +			for (i = 0; i < ARRAY_SIZE(adux1020_rate); i++) {
> > > +				if ((val == adux1020_rate[i][0]) &&
> > > +				     (val2 == adux1020_rate[i][1])) {
> > > +					ret = regmap_update_bits(data-
> > > > regmap,
> > > +						ADUX1020_REG_FREQUENCY,
> > > +						ADUX1020_PROX_FREQ_MASK,
> > > +						i <<
> > > ADUX1020_PROX_FREQ_SHIFT);
> > > +				}
> > > +			}
> > 
> > [5] This looks as it has too many level of indentations.
> > And could be reworked with either using the "find_closest()" macro, or
> > at
> > least split into a helper that finds the index in the "adux1020_rate"
> > array
> > based on val & val2.
> > 
> > Then you get something like:
> > ------------------------------------------------------------
> > i = adux1020_find_index(adux1020_rate, val, val2);
> > if (i < 0)
> >     return i;
> > 
> > return regmap_update_bits(data->regmap,
> >                           ADUX1020_REG_FREQUENCY,
> >                           ADUX1020_PROX_FREQ_SET(i));
> > ------------------------------------------------------------
> > 
> 
> good idea! will implement.
> 
> > > +		}
> > > +		break;
> > > +	case IIO_CHAN_INFO_PROCESSED:
> > > +		if (chan->type == IIO_CURRENT) {
> > > +			for (i = 0; i < ARRAY_SIZE(adux1020_led_current);
> > > i++) {
> > > +				if ((val == adux1020_led_current[i][0]) &&
> > > +				     (val2 == adux1020_led_current[i][1]))
> > > {
> > > +					ret = regmap_update_bits(data-
> > > > regmap,
> > > +						ADUX1020_REG_LED_CURRENT,
> > > +						ADUX1020_LED_CURRENT_MASK,
> > > i);
> > > +				}
> > > +			}
> > 
> > Same comment as [5]
> > 
> > > +		}
> > > +		break;
> > > +	default:
> > > +		break;
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int adux1020_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 adux1020_data *data = iio_priv(indio_dev);
> > > +
> > > +	regmap_write(data->regmap, ADUX1020_REG_INT_ENABLE,
> > > +		     ADUX1020_INT_ENABLE);
> > > +
> > 
> > Regarding [4]: it looks like the lock could be useful here.
> > Regarding [3]: more error checking could be useful.
> > 
> > 
> > > +	regmap_write(data->regmap, ADUX1020_REG_INT_POLARITY, 0);
> > > +
> > > +	switch (chan->type) {
> > > +	case IIO_PROXIMITY:
> > > +		if (dir == IIO_EV_DIR_RISING) {
> > > +			regmap_update_bits(data->regmap,
> > > ADUX1020_REG_INT_MASK,
> > > +					   ADUX1020_PROX_ON1_INT,
> > > +					   state ? 0 :
> > > ADUX1020_PROX_ON1_INT);
> > > +		} else {
> > > +			regmap_update_bits(data->regmap,
> > > ADUX1020_REG_INT_MASK,
> > > +					   ADUX1020_PROX_OFF1_INT,
> > > +					   state ? 0 :
> > > ADUX1020_PROX_OFF1_INT);
> > > +		}
> > 
> > [6] More about style/preference.
> > I like the part in adux1020_read_event_config(), where
> > 
> > -------------------------------------------------
> >   if (dir == IIO_EV_DIR_RISING)
> >       mask = ADUX1020_PROX_ON1_INT;
> >   else
> >       mask = ADUX1020_PROX_OFF1_INT;
> > -------------------------------------------------
> > 
> > Then this could become:
> > 
> > -------------------------------------------------
> > if (state)
> >     state = mask;
> > else
> >     state = 0;
> > 
> > ret = regmap_update_bits(data->regmap,
> >                          ADUX1020_REG_INT_MASK,
> >                          mask, state);
> > -------------------------------------------------
> > 
> 
> looks good, will implement.
> 
> > > +
> > > +		/*
> > > +		 * Trigger proximity interrupt when the intensity is above
> > > +		 * or below threshold
> > > +		 */
> > > +		regmap_update_bits(data->regmap, ADUX1020_REG_PROX_TYPE,
> > > +				   ADUX1020_PROX_TYPE, ADUX1020_PROX_TYPE);
> > > +
> > > +		/* Set proximity mode */
> > > +		adux1020_set_mode(data, ADUX1020_MODE_PROX_I);
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int adux1020_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 adux1020_data *data = iio_priv(indio_dev);
> > > +	int ret, mask;
> > > +	unsigned int regval;
> > > +
> > > +	switch (chan->type) {
> > > +	case IIO_PROXIMITY:
> > > +		if (dir == IIO_EV_DIR_RISING)
> > > +			mask = ADUX1020_PROX_ON1_INT;
> > > +		else
> > > +			mask = ADUX1020_PROX_OFF1_INT;
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	ret = regmap_read(data->regmap, ADUX1020_REG_INT_MASK, &regval);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	return !(regval & mask);
> > > +}
> > > +
> > > +static int adux1020_read_thresh(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 adux1020_data *data = iio_priv(indio_dev);
> > > +	u8 reg;
> > > +	int ret;
> > > +	unsigned int regval;
> > > +
> > > +	switch (chan->type) {
> > > +	case IIO_PROXIMITY:
> > > +		if (dir == IIO_EV_DIR_RISING)
> > > +			reg = ADUX1020_REG_PROX_TH_ON1;
> > > +		else
> > > +			reg = ADUX1020_REG_PROX_TH_OFF1;
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	ret = regmap_read(data->regmap, reg, &regval);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	*val = regval;
> > > +
> > > +	return IIO_VAL_INT;
> > > +}
> > > +
> > > +static int adux1020_write_thresh(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 adux1020_data *data = iio_priv(indio_dev);
> > > +	u8 reg;
> > > +
> > > +	switch (chan->type) {
> > > +	case IIO_PROXIMITY:
> > > +		if (dir == IIO_EV_DIR_RISING)
> > > +			reg = ADUX1020_REG_PROX_TH_ON1;
> > > +		else
> > > +			reg = ADUX1020_REG_PROX_TH_OFF1;
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	/* Full scale threshold value is 0-65535  */
> > > +	if (val < 0 || val > 65535)
> > > +		return -EINVAL;
> > > +
> > > +	return regmap_write(data->regmap, reg, val);
> > > +}
> > > +
> > > +static const struct iio_event_spec adux1020_proximity_event[] = {
> > > +	{
> > > +		.type = IIO_EV_TYPE_THRESH,
> > > +		.dir = IIO_EV_DIR_RISING,
> > > +		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> > > +			BIT(IIO_EV_INFO_ENABLE),
> > > +	},
> > > +	{
> > > +		.type = IIO_EV_TYPE_THRESH,
> > > +		.dir = IIO_EV_DIR_FALLING,
> > > +		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> > > +			BIT(IIO_EV_INFO_ENABLE),
> > > +	},
> > > +};
> > > +
> > > +static const struct iio_chan_spec adux1020_channels[] = {
> > > +	{
> > > +		.type = IIO_PROXIMITY,
> > > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > > +				      BIT(IIO_CHAN_INFO_SAMP_FREQ),
> > > +		.event_spec = adux1020_proximity_event,
> > > +		.num_event_specs = ARRAY_SIZE(adux1020_proximity_event),
> > > +	},
> > > +	{
> > > +		.type = IIO_CURRENT,
> > > +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > > +		.extend_name = "led",
> > > +	},
> > > +};
> > > +
> > > +static IIO_CONST_ATTR(sampling_frequency_available,
> > > +		      "0.1 0.2 0.5 1 2 5 10 20 50 100 190 450 820 1400");
> > 
> > This could re-use the IIO_CONST_ATTR_SAMP_FREQ_AVAIL() macro.
> > 
> 
> ack.
> 
> > > +
> > > +static struct attribute *adux1020_attributes[] = {
> > > +	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
> > > +	NULL
> > > +};
> > > +
> > > +static const struct attribute_group adux1020_attribute_group = {
> > > +	.attrs = adux1020_attributes,
> > > +};
> > > +
> > > +static const struct iio_info adux1020_info = {
> > > +	.attrs = &adux1020_attribute_group,
> > > +	.read_raw = adux1020_read_raw,
> > > +	.write_raw = adux1020_write_raw,
> > > +	.read_event_config = adux1020_read_event_config,
> > > +	.write_event_config = adux1020_write_event_config,
> > > +	.read_event_value = adux1020_read_thresh,
> > > +	.write_event_value = adux1020_write_thresh,
> > > +};
> > > +
> > > +static irqreturn_t adux1020_interrupt_handler(int irq, void
> > > *private)
> > > +{
> > > +	struct iio_dev *indio_dev = private;
> > > +	struct adux1020_data *data = iio_priv(indio_dev);
> > > +	int ret, status;
> > > +
> > > +	ret = regmap_read(data->regmap, ADUX1020_REG_INT_STATUS, &status);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	status &= ADUX1020_MODE_INT_STATUS_MASK;
> > > +
> > > +	if (status & ADUX1020_INT_PROX_ON1) {
> > > +		iio_push_event(indio_dev,
> > > +				IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
> > > +					IIO_EV_TYPE_THRESH,
> > > +					IIO_EV_DIR_RISING),
> > > +				iio_get_time_ns(indio_dev));
> > > +	}
> > > +
> > > +	if (status & ADUX1020_INT_PROX_OFF1) {
> > > +		iio_push_event(indio_dev,
> > > +				IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
> > > +					IIO_EV_TYPE_THRESH,
> > > +					IIO_EV_DIR_FALLING),
> > > +				iio_get_time_ns(indio_dev));
> > > +	}
> > > +
> > > +	regmap_update_bits(data->regmap, ADUX1020_REG_INT_STATUS,
> > > +			   ADUX1020_MODE_INT_MASK, status);
> > > +
> > > +	return IRQ_HANDLED;
> > > +}
> > > +
> > > +static int adux1020_chip_init(struct adux1020_data *data)
> > > +{
> > > +	struct i2c_client *client = data->client;
> > > +	int ret, i;
> > > +	unsigned int val;
> > > +
> > > +	ret = regmap_read(data->regmap, ADUX1020_REG_CHIP_ID, &val);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	val &= ADUX1020_CHIP_ID_MASK;
> > > +
> > > +	if (val != ADUX1020_CHIP_ID) {
> > > +		dev_err(&client->dev, "invalid chip id 0x%04x\n", val);
> > > +		return -ENODEV;
> > > +	};
> > > +
> > > +	dev_dbg(&client->dev, "Detected ADUX1020 with chip id: 0x%04x\n",
> > > val);
> > > +
> > > +	/* Perform software reset */
> > > +	regmap_update_bits(data->regmap, ADUX1020_REG_SW_RESET,
> > > +			   ADUX1020_SW_RESET, ADUX1020_SW_RESET);
> > > +
> > > +	/* Load default configuration */
> > > +	for (i = 0; i < ARRAY_SIZE(adux1020_def_conf); i++)
> > > +		regmap_write(data->regmap, adux1020_def_conf[i][0],
> > > +			     adux1020_def_conf[i][1]);
> > > +
> > > +	adux1020_flush_fifo(data);
> > > +
> > > +	/* Use LED_IREF for proximity mode */
> > > +	regmap_update_bits(data->regmap, ADUX1020_REG_LED_CURRENT,
> > > +			   ADUX1020_LED_PIREF_EN, 0);
> > > +
> > > +	/* Mask all interrupts */
> > > +	regmap_update_bits(data->regmap, ADUX1020_REG_INT_MASK,
> > > +			   ADUX1020_MODE_INT_MASK,
> > > ADUX1020_MODE_INT_DISABLE);
> > 
> > Same as [3]: a bit more error checking here could be useful.
> > 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int adux1020_probe(struct i2c_client *client,
> > > +			  const struct i2c_device_id *id)
> > > +{
> > > +	struct adux1020_data *data;
> > > +	struct iio_dev *indio_dev;
> > > +	int ret;
> > > +
> > > +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > > +	if (!indio_dev)
> > > +		return -ENOMEM;
> > > +
> > > +	indio_dev->dev.parent = &client->dev;
> > > +	indio_dev->info = &adux1020_info;
> > > +	indio_dev->name = ADUX1020_DRV_NAME;
> > > +	indio_dev->channels = adux1020_channels;
> > > +	indio_dev->num_channels = ARRAY_SIZE(adux1020_channels);
> > > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > > +
> > > +	data = iio_priv(indio_dev);
> > > +	i2c_set_clientdata(client, indio_dev);
> > > +
> > > +	data->regmap = devm_regmap_init_i2c(client,
> > > &adux1020_regmap_config);
> > > +	if (IS_ERR(data->regmap)) {
> > > +		dev_err(&client->dev, "regmap initialization failed.\n");
> > > +		return PTR_ERR(data->regmap);
> > > +	}
> > > +
> > > +	data->client = client;
> > > +	data->indio_dev = indio_dev;
> > > +	mutex_init(&data->lock);
> > > +
> > > +	ret = adux1020_chip_init(data);
> > > +	if (ret)
> > > +		goto err_out;
> > > +
> > > +	if (client->irq) {
> > > +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> > > +				NULL, adux1020_interrupt_handler,
> > > +				IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> > > +				ADUX1020_DRV_NAME, indio_dev);
> > > +		if (ret) {
> > > +			dev_err(&client->dev, "irq request error %d\n",
> > > -ret);
> > > +			goto err_out;
> > > +		}
> > > +	}
> > > +
> > > +	ret = iio_device_register(indio_dev);
> > 
> > [7] Jonathan may correct me here.
> > If this doesn't need to do anything more later, this could use
> > devm_iio_device_register(), and then the  adux1020_remove() hook could
> > be
> > removed.
> > 
> 
> right. will switch to devm_ API.
> 
> Thanks,
> Mani
> 
> > > +	if (ret) {
> > > +		dev_err(&client->dev, "Failed to register IIO device\n");
> > > +		goto err_out;
> > > +	}
> > > +
> > > +	return 0;
> > > +
> > > +err_out:
> > > +	return ret;
> > > +}
> > > +
> > > +static int adux1020_remove(struct i2c_client *client)
> > > +{
> > > +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > > +
> > > +	iio_device_unregister(indio_dev);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct i2c_device_id adux1020_id[] = {
> > > +	{ "adux1020", 0 },
> > > +	{}
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, adux1020_id);
> > > +
> > > +static const struct of_device_id adux1020_of_match[] = {
> > > +	{ .compatible = "adi,adux1020" },
> > > +	{ }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, adux1020_of_match);
> > > +
> > > +static struct i2c_driver adux1020_driver = {
> > > +	.driver = {
> > > +		.name	= ADUX1020_DRV_NAME,
> > > +		.of_match_table = adux1020_of_match,
> > > +	},
> > > +	.probe		= adux1020_probe,
> > > +	.remove		= adux1020_remove,
> > > +	.id_table	= adux1020_id,
> > > +};
> > > +module_i2c_driver(adux1020_driver);
> > > +
> > > +MODULE_AUTHOR("Manivannan Sadhasivam <
> > > manivannan.sadhasivam@linaro.org>"
> > > );
> > > +MODULE_DESCRIPTION("ADUX1020 photometric sensor");
> > > +MODULE_LICENSE("GPL");

Re: [PATCH 2/2] iio: light: Add support for ADUX1020 sensor

[Jonathan Cameron]

On Wed, 9 Oct 2019 10:21:27 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Wed, 2019-10-09 at 15:15 +0530, Manivannan Sadhasivam wrote:
> > [External]
> > 
> > Hi Ardelean,

For some reason, my email client decided not to filter this thread
correctly so I didn't realise so much discussion had gone on when
I applied the newer version earlier today.  Oops.  Hopefully
there was nothing major outstanding.  Let me know if there was
as it's not yet in a non rebasing tree...

I've cropped to just where my name got mentioned ;)

..

> >   
> > > - Just curios here: there is gesture mode as well; will that be
> > > implemented
> > > later? Or will there be other modes implemented?  
> > 
> > Currently only proximity mode is implemented. There are gesture and
> > sample
> > modes and I left those as a TODO. But I'm not sure whether IIO is
> > supporting
> > gesture mode properly or not.  
> 
> I don't have any input on this at the moment [about gesture support & IIO].
> I'd have to investigate.
> Maybe Jonathan has some thoughts.

Properly is a hard term for gesture support.  The issue has always
been that every device does it slightly differently.  There are
way too many types of gesture that a device 'might' use.

We do have some drivers (IIRC) doing some gesture sensing, but you may
well find places where things need to expand!

...
> > > > +static int adux1020_read_raw(struct iio_dev *indio_dev,
> > > > +			     struct iio_chan_spec const *chan,
> > > > +			     int *val, int *val2, long mask)
> > > > +{
> > > > +	struct adux1020_data *data = iio_priv(indio_dev);
> > > > +	u16 buf[3];  
> > > 
> > > This buffer looks a bit weird. [8]
> > > It's 3 elements-wide and passed without any information about size.
> > > And only the first element is used.
> > > So, maybe just convert u16 buf[3] -> u16 buf?
> > >   
> > 
> > The buffer declaration is based on the hardware buffer available. It
> > is 3 elements wide since the remaining 2 elements will be used by other
> > modes. The idea here is to reuse the adux1020_measure() API for all 3
> > modes (which has varying buffer sizes).  
> 
> The only thought I have left about this buffer [and forgot to mention it
> earlier], is whether this should be cacheline aligned [or not].
> If it has to be, then maybe it shouldn't be stored on the stack and moved
> to a malloc-ed buffer [on "struct adux1020_data"].
> Cacheline aligned stuff typically deals with potential DMA issues. The DMA
> issues [in this case] could be coming from i2c controllers that can do DMA.
> 
> Jonathan may have more input here.
> 
The i2c subsystem in general doesn't assume that buffers are dma safe
though it would like to ;)

Wolfram did a good presentation on his efforts to sort that out at
ELCE 2018

https://www.youtube.com/watch?v=JDwaMClvV-s