Skip to contentSkip to conversation

iio: sx932x: Add driver for Semtech sx932x proximity sensor

PATCH In review Enrico Granata 2 participants 2 messages Nov 1, 2018 – Nov 3, 2018

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 iio: sx932x: Add driver for Semtech sx932x proximity sensor

From: Enrico Granata <[email protected]>

This driver allows interfacing with the Semtech sx9320, sx9321 and sx9324 proximity sensors via the IIO subsystem.

---
 .../bindings/iio/proximity/sx932x.txt         |   54 +
 drivers/iio/proximity/Kconfig                 |   36 +-
 drivers/iio/proximity/Makefile                |    1 +
 drivers/iio/proximity/sx932x.c                | 1462 +++++++++++++++++
 4 files changed, 1542 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/proximity/sx932x.txt
 create mode 100644 drivers/iio/proximity/sx932x.c
Documentation/devicetree/bindings/iio/proximity/sx932x.txt new file +54 -0
new file mode 100644
index 0000000000000..e1f1fefdbca08
@@ -0,0 +1,54 @@
+Semtech's SX9320/SX9321/SX9324 capacitive proximity button device driver
+
+Required properties:
+	- compatible: must be "semtech,sx9320 or sx9321 or sx9324"
+	- reg: i2c address where to find the device
+	- interrupt-parent : should be the phandle for the interrupt controller
+	- interrupts : the sole interrupt generated by the device
+	- semtech,register-prefix: the string prefixing all register names in the
+
+	Refer to interrupt-controller/interrupts.txt for generic
+	interrupt client node bindings.
+
+Optional properties:
+	- semtech,adv_ctrl10: Initial value for register AdvCtrl10.
+	- semtech,afe_ctrl4: Initial value for register AfeCtrl4.
+	- semtech,afe_ctrl7: Initial value for register AfeCtrl7.
+	- semtech,afe_ctrl9: Initial value for register AfeCtrl9.
+	- semtech,prox_ctrl0: Initial value for register ProxCtrl0.
+	- semtech,prox_ctrl1: Initial value for register ProxCtrl1.
+	- semtech,prox_ctrl2: Initial value for register ProxCtrl2.
+	- semtech,prox_ctrl4: Initial value for register ProxCtrl4.
+	- semtech,prox_ctrl5: Initial value for register ProxCtrl5.
+	- semtech,prox_ctrl6: Initial value for register ProxCtrl6.
+	- semtech,prox_ctrl7: Initial value for register Prox_Trl7.
+	- semtech,adv_ctrl16: Initial value for register AdvCtrl16.
+	- semtech,adv_ctrl17: Initial value for register AdvCtrl17.
+	- semtech,adv_ctrl18: Initial value for register AdvCtrl18.
+	- semtech,gnrl_ctrl1: Initial value for register GnrlCtrl1.
+
+	Refer to the data sheet for register definitions.
+
+Example:
+
+sx9310@28 {
+	compatible = "semtech,sx9320";
+	reg = <0x28>;
+	interrupt-parent = <&gpio2>;
+	interrupts = <16 IRQ_TYPE_LEVEL_LOW>;
+};
+
+Driver supports ACPI bindings. We can store registers configuration obtained
+from per-model SAR sensor calibration. Refer to the data sheet for meaning.
+
+ACPI name is STH9320 or STH9321 or STH9324.
+
+Example:
+    chip drivers/i2c/generic
+            register "hid" = ""STH9321""
+            register "name" = ""SEMTECH SX9321""
+            register "desc" = ""SAR Proximity Sensor""
+            register "irq" = "ACPI_IRQ_LEVEL_LOW(GPP_A18_IRQ)"
+            register "device_present_gpio" = "GPP_B20"
+            device i2c 28 on end
+    end
drivers/iio/proximity/Kconfig +25 -11
index b99367a89f810..3b979b8198d7c 100644
@@ -66,31 +66,45 @@ config SRF04
 	  To compile this driver as a module, choose M here: the
 	  module will be called srf04.
 
-config SX9500
-	tristate "SX9500 Semtech proximity sensor"
+config SRF08
+	tristate "Devantech SRF02/SRF08/SRF10 ultrasonic ranger sensor"
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	depends on I2C
+	help
+	  Say Y here to build a driver for Devantech SRF02/SRF08/SRF10
+	  ultrasonic ranger sensors with i2c interface.
+	  This driver can be used to measure the distance of objects.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called srf08.
+
+config SX932X
+	tristate "sx932x Semtech proximity sensor"
 	select IIO_BUFFER
 	select IIO_TRIGGERED_BUFFER
 	select REGMAP_I2C
 	depends on I2C
 	help
-	  Say Y here to build a driver for Semtech's SX9500 capacitive
-	  proximity/button sensor.
+	  Say Y here to build a driver for Semtech's sx932x capacitive
+	  proximity/button sensor. This driver supports Semtech's
+	  sx9320, sx9321 and sx9324 sensors.
 
 	  To compile this driver as a module, choose M here: the
-	  module will be called sx9500.
+	  module will be called sx932x.
 
-config SRF08
-	tristate "Devantech SRF02/SRF08/SRF10 ultrasonic ranger sensor"
+config SX9500
+	tristate "SX9500 Semtech proximity sensor"
 	select IIO_BUFFER
 	select IIO_TRIGGERED_BUFFER
+	select REGMAP_I2C
 	depends on I2C
 	help
-	  Say Y here to build a driver for Devantech SRF02/SRF08/SRF10
-	  ultrasonic ranger sensors with i2c interface.
-	  This driver can be used to measure the distance of objects.
+	  Say Y here to build a driver for Semtech's SX9500 capacitive
+	  proximity/button sensor.
 
 	  To compile this driver as a module, choose M here: the
-	  module will be called srf08.
+	  module will be called sx9500.
 
 config VL53L0X_I2C
 	tristate "STMicroelectronics VL53L0X ToF ranger sensor (I2C)"
drivers/iio/proximity/Makefile +1 -0
index 6d031f903c4c2..65ac237d63dd6 100644
@@ -10,6 +10,7 @@ obj-$(CONFIG_LIDAR_LITE_V2)	+= pulsedlight-lidar-lite-v2.o
 obj-$(CONFIG_RFD77402)		+= rfd77402.o
 obj-$(CONFIG_SRF04)		+= srf04.o
 obj-$(CONFIG_SRF08)		+= srf08.o
+obj-$(CONFIG_SX932X)		+= sx932x.o
 obj-$(CONFIG_SX9500)		+= sx9500.o
 obj-$(CONFIG_VL53L0X_I2C)	+= vl53l0x-i2c.o
 
drivers/iio/proximity/sx932x.c new file +1462 -0

1,462 lines not on this page · raw message

Signature · 1 line
2.19.1.930.g4563a0d9d0-goog
maintainer
#1 lore raw
Quoted 6 lines · On Thu, 1 Nov 2018 14:43:13 -0700 Enrico Granata <[email protected]> wrote:

From: Enrico Granata <[email protected]>

This driver allows interfacing with the Semtech sx9320, sx9321 and sx9324 proximity sensors via the IIO subsystem.

Various comments inline. Please make sure to proof read your patches before sending. This one picked up some changes you definitely didn't mean to make int the build files!

There is a mass of non standard ABI in here. It all needs documenting in Documentation/ABI/testing/sysfs-bus-iio-* as appropriate.

Is there a datasheet available somewhere? It would make life easier.

I think this is a device with it's own clock / sequencer and that we need to enable channels before their events work? That would explain some of the complexity in here.. I'm not yet convinced the reference counting logic makes sense so want more information on the constraints. You may have it right, but without the datasheet (link below doesn't work) it's hard to know what we 'need' to happen.

There is quite a lot of non standard ABI that probably just wants to be removed.

Quoted 7 lines
---
 .../bindings/iio/proximity/sx932x.txt         |   54 +
 drivers/iio/proximity/Kconfig                 |   36 +-
 drivers/iio/proximity/Makefile                |    1 +
 drivers/iio/proximity/sx932x.c                | 1462 +++++++++++++++++
 4 files changed, 1542 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/proximity/sx932x.txt

I know we have a number of wild card named drivers in tree (with weird historical or compatibility reasons for most of them) but generally we don't use wild cards in naming. It goes wrong far too often when the company things it'll be smart to use other names in the range we thought was ours!
So pick a part and name it after that. sx9320.txt etc.

Also as you have a devicetree binding in here you should cc the relevant maintainers and list. +cc Rob, Mark and List.

Documentation/devicetree/bindings/iio/proximity/sx932x.txt line 4

Quoted 12 lines
 create mode 100644 drivers/iio/proximity/sx932x.c
Documentation/devicetree/bindings/iio/proximity/sx932x.txt new file +4 -0
new file mode 100644
index 0000000000000..e1f1fefdbca08
@@ -0,0 +1,54 @@
+Semtech's SX9320/SX9321/SX9324 capacitive proximity button device driver
+
+Required properties:
+	- compatible: must be "semtech,sx9320 or sx9321 or sx9324"

One compatible per line and fully stated with prefix for each one.

Documentation/devicetree/bindings/iio/proximity/sx932x.txt line 8

Quoted 4 lines
Documentation/devicetree/bindings/iio/proximity/sx932x.txt new file +4 -0
+	- reg: i2c address where to find the device
+	- interrupt-parent : should be the phandle for the interrupt controller
+	- interrupts : the sole interrupt generated by the device
+	- semtech,register-prefix: the string prefixing all register names in the

I'm lost on what this one is.

Quoted 10 lines
Documentation/devicetree/bindings/iio/proximity/sx932x.txt new file +10 -0
+
+	Refer to interrupt-controller/interrupts.txt for generic
+	interrupt client node bindings.
+
+Optional properties:
+	- semtech,adv_ctrl10: Initial value for register AdvCtrl10.
+	- semtech,afe_ctrl4: Initial value for register AfeCtrl4.
+	- semtech,afe_ctrl7: Initial value for register AfeCtrl7.
+	- semtech,afe_ctrl9: Initial value for register AfeCtrl9.
+	- semtech,prox_ctrl0: Initial value for register ProxCtrl0.

Documentation/devicetree/bindings/iio/proximity/sx932x.txt line 28

Quoted 10 lines
Documentation/devicetree/bindings/iio/proximity/sx932x.txt new file +10 -0
+	- semtech,prox_ctrl1: Initial value for register ProxCtrl1.
+	- semtech,prox_ctrl2: Initial value for register ProxCtrl2.
+	- semtech,prox_ctrl4: Initial value for register ProxCtrl4.
+	- semtech,prox_ctrl5: Initial value for register ProxCtrl5.
+	- semtech,prox_ctrl6: Initial value for register ProxCtrl6.
+	- semtech,prox_ctrl7: Initial value for register Prox_Trl7.
+	- semtech,adv_ctrl16: Initial value for register AdvCtrl16.
+	- semtech,adv_ctrl17: Initial value for register AdvCtrl17.
+	- semtech,adv_ctrl18: Initial value for register AdvCtrl18.
+	- semtech,gnrl_ctrl1: Initial value for register GnrlCtrl1.

Bindings should never include raw register addresses. If there are some things in those registers that need controlling please break them out as human readable elements so we know what is being set.

As these are optional I suspect you can just drop the lot.

Documentation/devicetree/bindings/iio/proximity/sx932x.txt line 34

Quoted 6 lines
Documentation/devicetree/bindings/iio/proximity/sx932x.txt new file +6 -0
+
+	Refer to the data sheet for register definitions.
+
+Example:
+
+sx9310@28 {

Hmm. Not sure we have a generic name for a proximity sensor but proximity@28 {
would seems sensible.

Documentation/devicetree/bindings/iio/proximity/sx932x.txt line 41

Quoted 7 lines
Documentation/devicetree/bindings/iio/proximity/sx932x.txt new file +7 -0
+	compatible = "semtech,sx9320";
+	reg = <0x28>;
+	interrupt-parent = <&gpio2>;
+	interrupts = <16 IRQ_TYPE_LEVEL_LOW>;
+};
+
+Driver supports ACPI bindings. We can store registers configuration obtained

While interesting, doesn't belong in this binding file.

Quoted 25 lines
Documentation/devicetree/bindings/iio/proximity/sx932x.txt new file +13 -0
+from per-model SAR sensor calibration. Refer to the data sheet for meaning.
+
+ACPI name is STH9320 or STH9321 or STH9324.
+
+Example:
+    chip drivers/i2c/generic
+            register "hid" = ""STH9321""
+            register "name" = ""SEMTECH SX9321""
+            register "desc" = ""SAR Proximity Sensor""
+            register "irq" = "ACPI_IRQ_LEVEL_LOW(GPP_A18_IRQ)"
+            register "device_present_gpio" = "GPP_B20"
+            device i2c 28 on end
+    end
drivers/iio/proximity/Kconfig +2 -2
index b99367a89f810..3b979b8198d7c 100644
@@ -66,31 +66,45 @@ config SRF04
 	  To compile this driver as a module, choose M here: the
 	  module will be called srf04.
 
-config SX9500
-	tristate "SX9500 Semtech proximity sensor"
+config SRF08
+	tristate "Devantech SRF02/SRF08/SRF10 ultrasonic ranger sensor"

drivers/iio/proximity/Kconfig config SRF04 · line 80

Quoted 10 lines
drivers/iio/proximity/Kconfig +10 -0
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	depends on I2C
+	help
+	  Say Y here to build a driver for Devantech SRF02/SRF08/SRF10
+	  ultrasonic ranger sensors with i2c interface.
+	  This driver can be used to measure the distance of objects.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called srf08.

??? Stray blocks here!

Please make sure you aren't killing off other drivers!

Quoted 57 lines
drivers/iio/proximity/Kconfig +13 -9
+
+config SX932X
+	tristate "sx932x Semtech proximity sensor"
 	select IIO_BUFFER
 	select IIO_TRIGGERED_BUFFER
 	select REGMAP_I2C
 	depends on I2C
 	help
-	  Say Y here to build a driver for Semtech's SX9500 capacitive
-	  proximity/button sensor.
+	  Say Y here to build a driver for Semtech's sx932x capacitive
+	  proximity/button sensor. This driver supports Semtech's
+	  sx9320, sx9321 and sx9324 sensors.
 
 	  To compile this driver as a module, choose M here: the
-	  module will be called sx9500.
+	  module will be called sx932x.
 
-config SRF08
-	tristate "Devantech SRF02/SRF08/SRF10 ultrasonic ranger sensor"
+config SX9500
+	tristate "SX9500 Semtech proximity sensor"
 	select IIO_BUFFER
 	select IIO_TRIGGERED_BUFFER
+	select REGMAP_I2C
 	depends on I2C
 	help
-	  Say Y here to build a driver for Devantech SRF02/SRF08/SRF10
-	  ultrasonic ranger sensors with i2c interface.
-	  This driver can be used to measure the distance of objects.
+	  Say Y here to build a driver for Semtech's SX9500 capacitive
+	  proximity/button sensor.
 
 	  To compile this driver as a module, choose M here: the
-	  module will be called srf08.
+	  module will be called sx9500.
 
 config VL53L0X_I2C
 	tristate "STMicroelectronics VL53L0X ToF ranger sensor (I2C)"
drivers/iio/proximity/Makefile +1 -0
index 6d031f903c4c2..65ac237d63dd6 100644
@@ -10,6 +10,7 @@ obj-$(CONFIG_LIDAR_LITE_V2)	+= pulsedlight-lidar-lite-v2.o
 obj-$(CONFIG_RFD77402)		+= rfd77402.o
 obj-$(CONFIG_SRF04)		+= srf04.o
 obj-$(CONFIG_SRF08)		+= srf08.o
+obj-$(CONFIG_SX932X)		+= sx932x.o
 obj-$(CONFIG_SX9500)		+= sx9500.o
 obj-$(CONFIG_VL53L0X_I2C)	+= vl53l0x-i2c.o
 
drivers/iio/proximity/sx932x.c new file +0 -0
new file mode 100644
index 0000000000000..069d3678f6bc9

drivers/iio/proximity/sx932x.c line 7

Quoted 7 lines
drivers/iio/proximity/sx932x.c new file +7 -0
@@ -0,0 +1,1462 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2018 Google LLC.
+ *
+ * Driver for Semtech's SX9320 capacitive proximity/button solution.
+ * Datasheet available at
+ * <http://www.semtech.com/images/datasheet/sx9320.pdf>.

Sadly not it seems...

drivers/iio/proximity/sx932x.c line 10

Quoted 3 lines
drivers/iio/proximity/sx932x.c new file +3 -0
+ * Based on SX9500 driver and Semtech driver using the input framework
+ * <https://my.syncplicity.com/share/teouwsim8niiaud/
+ *          linux-driver-SX9320_NoSmartHSensing>.

I'm not 100% sure what this device actually is. It might be better off as an input driver... Depends on just how much info it gives and how that maps to input.

drivers/iio/proximity/sx932x.c line 14

Quoted 4 lines
drivers/iio/proximity/sx932x.c new file +4 -0
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published by
+ * the Free Software Foundation.

If using an SPDX header it is generally considered that there is no need to repeat the license statement.

drivers/iio/proximity/sx932x.c line 26

Quoted 12 lines
drivers/iio/proximity/sx932x.c new file +12 -0
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/irq.h>
+#include <linux/acpi.h>
+#include <linux/gpio/consumer.h>
+#include <linux/regmap.h>
+#include <linux/pm.h>
+#include <linux/delay.h>

If no reason to do otherwise, it's preferred to have headers in alphabetical order. Absolutely fine to have the separate block for iio headers though.

drivers/iio/proximity/sx932x.c line 36

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+
+#include <linux/iio/iio.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/events.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
+
+#define SX932X_DRIVER_NAME		"sx932x"

After removing the wrong usage of this I've marked you should only have the one place where it is used. Just put it directly there. The define just makes it harder to know where this is used.

drivers/iio/proximity/sx932x.c line 39

Quoted 3 lines
drivers/iio/proximity/sx932x.c new file +3 -0
+#define SX9320_ACPI_NAME		"STH9320"
+#define SX9321_ACPI_NAME		"STH9321"
+#define SX9324_ACPI_NAME		"STH9324"

I'd expect to only see these in the binding, not up here as defines.

drivers/iio/proximity/sx932x.c line 40

Quoted 1 line
drivers/iio/proximity/sx932x.c new file +1 -0
+#define SX932X_DEV_NB			2

I'm relieved this isn't used! Please remove.

drivers/iio/proximity/sx932x.c line 42

Quoted 2 lines
drivers/iio/proximity/sx932x.c new file +2 -0
+
+#define SX932X_IRQ_NAME			"sx932x_event"

Put it inline.

drivers/iio/proximity/sx932x.c line 44

Quoted 2 lines
drivers/iio/proximity/sx932x.c new file +2 -0
+
+#define SX932X_GPIO_INT			"interrupt"

This is worrying. We shouldn't care that it is a gpio inside the driver. Ah, we don't as this is unused. Please delete.

Quoted 78 lines
drivers/iio/proximity/sx932x.c new file +78 -0
+
+/* Register definitions. */
+#define SX932X_REG_IRQ_SRC		0x00
+#define SX932X_REG_STAT0		0x01
+#define SX932X_REG_STAT1		0x02
+#define SX932X_REG_STAT2		0x03
+#define SX932X_REG_STAT3		0x04
+#define SX932X_REG_IRQ_MSK		0x05
+#define SX932X_REG_IRQ_CFG0		0x06
+#define SX932X_REG_IRQ_CFG2		0x07
+
+#define SX932X_REG_GNRL_CTRL0		0x10
+#define SX932X_REG_GNRL_CTRL1		0x11
+
+#define SX932X_REG_I2C_ADDR		0x14
+#define SX932X_REG_CLK_SPRD		0x15
+
+#define SX932X_REG_AFE_CTRL0		0x20
+#define SX932X_REG_AFE_CTRL1		0x21
+#define SX932X_REG_AFE_CTRL2		0x22
+#define SX932X_REG_AFE_CTRL3		0x23
+#define SX932X_REG_AFE_CTRL4		0x24
+#define SX932X_REG_AFE_CTRL5		0x25
+#define SX932X_REG_AFE_CTRL6		0x26
+#define SX932X_REG_AFE_CTRL7		0x27
+#define SX932X_REG_AFE_PH0		0x28
+#define SX932X_REG_AFE_PH1		0x29
+#define SX932X_REG_AFE_PH2		0x2a
+#define SX932X_REG_AFE_PH3		0x2b
+#define SX932X_REG_AFE_CTRL8		0x2c
+#define SX932X_REG_AFE_CTRL9		0x2d
+
+#define SX932X_REG_PROX_CTRL0		0x30
+#define SX932X_REG_PROX_CTRL1		0x31
+#define SX932X_REG_PROX_CTRL2		0x32
+#define SX932X_REG_PROX_CTRL3		0x33
+#define SX932X_REG_PROX_CTRL4		0x34
+#define SX932X_REG_PROX_CTRL5		0x35
+#define SX932X_REG_PROX_CTRL6		0x36
+#define SX932X_REG_PROX_CTRL7		0x37
+
+#define SX932X_REG_ADV_CTRL0		0x40
+#define SX932X_REG_ADV_CTRL1		0x41
+#define SX932X_REG_ADV_CTRL2		0x42
+#define SX932X_REG_ADV_CTRL3		0x43
+#define SX932X_REG_ADV_CTRL4		0x44
+#define SX932X_REG_ADV_CTRL5		0x45
+#define SX932X_REG_ADV_CTRL6		0x46
+#define SX932X_REG_ADV_CTRL7		0x47
+#define SX932X_REG_ADV_CTRL8		0x48
+#define SX932X_REG_ADV_CTRL9		0x49
+#define SX932X_REG_ADV_CTRL10		0x4a
+#define SX932X_REG_ADV_CTRL11		0x4b
+#define SX932X_REG_ADV_CTRL12		0x4c
+#define SX932X_REG_ADV_CTRL13		0x4d
+#define SX932X_REG_ADV_CTRL14		0x4e
+#define SX932X_REG_ADV_CTRL15		0x4f
+#define SX932X_REG_ADV_CTRL16		0x50
+#define SX932X_REG_ADV_CTRL17		0x51
+#define SX932X_REG_ADV_CTRL18		0x52
+#define SX932X_REG_ADV_CTRL19		0x53
+#define SX932X_REG_ADV_CTRL20		0x54
+
+#define SX932X_REG_PHASE_SEL		0x60
+
+#define SX932X_REG_USE_MSB		0x61
+#define SX932X_REG_USE_LSB		0x62
+
+#define SX932X_REG_AVG_MSB		0x63
+#define SX932X_REG_AVG_LSB		0x64
+
+#define SX932X_REG_DIFF_MSB		0x65
+#define SX932X_REG_DIFF_LSB		0x66
+
+#define SX932X_REG_OFFSET_MSB		0x67
+#define SX932X_REG_OFFSET_LSB		0x68
+
+#define SX932X_REG_SAR_MSB		0x69

drivers/iio/proximity/sx932x.c line 132

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+#define SX932X_REG_SAR_LSB		0x6a
+
+#define SX932X_REG_RESET		0x9f
+/* Write this to REG_RESET to do a soft reset. */
+#define SX932X_SOFT_RESET		0xde
+
+#define SX932X_REG_WHOAMI		0xfa
+#define SX9320_WHOAMI			0x20
+#define SX9321_WHOAMI			0x21
+#define SX9324_WHOAMI 0x23

Very odd indenting. Please fix.

drivers/iio/proximity/sx932x.c line 135

Quoted 3 lines
drivers/iio/proximity/sx932x.c new file +3 -0
+#define SX932X_REG_REVISION		0xfe
+
+

One blank line is almost always enough for readability. Adding more just reduces what is on my screen at one time and makes the code harder to review.

drivers/iio/proximity/sx932x.c line 136

Quoted 1 line
drivers/iio/proximity/sx932x.c new file +1 -0
+/* Sensor Readback */

This comment seems unlikely to add much.

drivers/iio/proximity/sx932x.c line 146

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+
+/*
+ * These serve for identifying IRQ source in the IRQ_SRC register, and
+ * also for masking the IRQs in the IRQ_MSK register.
+ */
+#define SX932X_RESET_IRQ		BIT(7)
+#define SX932X_CLOSE_IRQ		BIT(6)
+#define SX932X_FAR_IRQ			BIT(5)
+#define SX932X_COMPDONE_IRQ		BIT(4)
+#define SX932X_CONVDONE_IRQ		BIT(3)

I would prefer it to be obvious which register these are used for from the naming. There are various ways of doing this, but a nice naming convention is something like.

#define SX9320_IRQ_MASK_REG xxx
#define SX9320_IRQ_MASK_RESET BIT(7)
#define SX9320_IRQ_MASK_CLOSE

etc. Note the indenting to make it clear which is the register name and which the field name. Also note no wild cards in the prefix. Just pick a device and name everything common after that one. If there are differences then make those clear by using the relevant prefix.

Quoted 8 lines
drivers/iio/proximity/sx932x.c new file +8 -0
+
+#define SX932X_SCAN_PERIOD_MASK		GENMASK(7, 4)
+#define SX932X_SCAN_PERIOD_SHIFT	4
+
+#define SX932X_COMPSTAT_MASK		GENMASK(3, 0)
+
+/* 4 channels, as defined in STAT0: PH0, PH1, PH2 and PH3. */
+#define SX932X_NUM_CHANNELS		4

drivers/iio/proximity/sx932x.c line 164

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+#define SX932X_CHAN_MASK		GENMASK(1, 0)
+
+struct sx932x_reg_config {
+	const char *property;
+	u8 reg_addr;
+	u8 def_value;
+};
+
+struct sx932x_data {
+	struct mutex mutex;

Locks should always have clear comments explaining exactly what they are protecting.

Quoted 28 lines
drivers/iio/proximity/sx932x.c new file +28 -0
+	struct i2c_client *client;
+	const struct sx932x_reg_config *reg_config;
+	struct iio_trigger *trig;
+	struct regmap *regmap;
+	/*
+	 * Last reading of the proximity status for each channel.
+	 * We only send an event to user space when this changes.
+	 */
+	bool prox_stat[SX932X_NUM_CHANNELS];
+	bool event_enabled[SX932X_NUM_CHANNELS];
+	bool trigger_enabled;
+	u16 *buffer;
+	/* Remember which phases are enabled during a suspend. */
+	unsigned int suspend_reg_gnrl_ctrl1;
+	struct completion completion;
+	int data_rdy_users, close_far_users;
+	int channel_users[SX932X_NUM_CHANNELS];
+};
+
+static const struct iio_event_spec sx932x_events[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_EITHER,
+		.mask_separate = BIT(IIO_EV_INFO_ENABLE),
+	},
+};
+
+#define SX932X_RAW_CHANNEL(idx, name, addr)			\

drivers/iio/proximity/sx932x.c line 202

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+	{							\
+		.type = IIO_PROXIMITY,				\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+		.indexed = 1,					\
+		.channel = idx,					\
+		.address = addr,				\
+		.event_spec = sx932x_events,			\
+		.num_event_specs = ARRAY_SIZE(sx932x_events),	\
+		.extend_name = name,				\

The extend name field is almost always a bad idea as it breaks generic userspace code.

drivers/iio/proximity/sx932x.c line 208

Quoted 6 lines
drivers/iio/proximity/sx932x.c new file +6 -0
+		.scan_index = idx,				\
+		.scan_type = {					\
+			.sign = 's',				\
+			.realbits = 16,				\
+			.storagebits = 16,			\
+			.shift = 0,				\

Shift default will be 0 anyway so no need to specify.

Quoted 12 lines
drivers/iio/proximity/sx932x.c new file +12 -0
+		},						\
+	}
+
+#define SX932X_PROCESSED_CHANNEL(idx, name, addr)		\
+	{							\
+		.type = IIO_PROXIMITY,				\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),	\
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+		.indexed = 1,					\
+		.channel = idx,					\
+		.address = addr,				\
+		.event_spec = sx932x_events,			\

drivers/iio/proximity/sx932x.c line 230

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+		.num_event_specs = ARRAY_SIZE(sx932x_events),	\
+		.extend_name = name,				\
+		.scan_index = idx,				\
+		.scan_type = {					\
+			.sign = 's',				\
+			.realbits = 16,				\
+			.storagebits = 16,			\
+			.shift = 0,				\
+		},						\
+	}

This doesn't seem to be used...

drivers/iio/proximity/sx932x.c line 233

Quoted 3 lines
drivers/iio/proximity/sx932x.c new file +3 -0
+
+static const struct iio_chan_spec sx932x_channels[] = {
+	SX932X_RAW_CHANNEL(0, "UC0", SX932X_REG_USE_MSB),

These channels are somewhat 'unusual'. What do they actually give you?

Quoted 63 lines
drivers/iio/proximity/sx932x.c new file +63 -0
+	SX932X_RAW_CHANNEL(1, "UC1", SX932X_REG_USE_MSB),
+	SX932X_RAW_CHANNEL(2, "UC2", SX932X_REG_USE_MSB),
+	SX932X_RAW_CHANNEL(3, "UC3", SX932X_REG_USE_MSB),
+
+	SX932X_RAW_CHANNEL(4, "AC0", SX932X_REG_AVG_MSB),
+	SX932X_RAW_CHANNEL(5, "AC1", SX932X_REG_AVG_MSB),
+	SX932X_RAW_CHANNEL(6, "AC2", SX932X_REG_AVG_MSB),
+	SX932X_RAW_CHANNEL(7, "AC3", SX932X_REG_AVG_MSB),
+
+	SX932X_RAW_CHANNEL(8,  "DC0", SX932X_REG_DIFF_MSB),
+	SX932X_RAW_CHANNEL(9,  "DC1", SX932X_REG_DIFF_MSB),
+	SX932X_RAW_CHANNEL(10, "DC2", SX932X_REG_DIFF_MSB),
+	SX932X_RAW_CHANNEL(11, "DC3", SX932X_REG_DIFF_MSB),
+
+	SX932X_RAW_CHANNEL(12, "CO0", SX932X_REG_OFFSET_MSB),
+	SX932X_RAW_CHANNEL(13, "CO1", SX932X_REG_OFFSET_MSB),
+	SX932X_RAW_CHANNEL(14, "CO2", SX932X_REG_OFFSET_MSB),
+	SX932X_RAW_CHANNEL(15, "CO3", SX932X_REG_OFFSET_MSB),
+
+	IIO_CHAN_SOFT_TIMESTAMP(16),
+};
+
+static const struct {
+	int val;
+	int val2;
+} sx932x_samp_freq_table[] = {
+	{500, 0}, /* 0000: Min (no idle time) */
+	{66, 666666}, /* 0001: 15 ms */
+	{33, 333333}, /* 0010: 30 ms (Typ.) */
+	{22, 222222}, /* 0011: 45 ms */
+	{16, 666666}, /* 0100: 60 ms */
+	{11, 111111}, /* 0101: 90 ms */
+	{8, 333333}, /* 0110: 120 ms */
+	{5, 0}, /* 0111: 200 ms */
+	{2, 500000}, /* 1000: 400 ms */
+	{1, 666666}, /* 1001: 600 ms */
+	{1, 250000}, /* 1010: 800 ms */
+	{1, 0}, /* 1011: 1 s */
+	{0, 500000}, /* 1100: 2 s */
+	{8, 333333}, /* 1101: 3 s */
+	{0, 250000}, /* 1110: 4 s */
+	{0, 200000}, /* 1111: 5 s */
+};
+static const unsigned int sx932x_scan_period_table[] = {
+	2, 15, 30, 45, 60, 90, 120, 200, 400, 800, 1000, 2000, 3000, 4000, 5000,
+};
+
+static IIO_CONST_ATTR_SAMP_FREQ_AVAIL(
+	"500.0 66.666666 33.333333 22.222222 16.666666 "
+	"11.111111 8.333333 5.0 2.500000 1.666666 1.250000 "
+	"1.0 0.500000 8.333333 0.250000 0.200000");
+
+
+static const struct regmap_range sx932x_writable_reg_ranges[] = {
+	regmap_reg_range(SX932X_REG_IRQ_MSK, SX932X_REG_IRQ_CFG2),
+	regmap_reg_range(SX932X_REG_GNRL_CTRL0, SX932X_REG_GNRL_CTRL1),
+	/* Leave i2c and clock spreading as unavailable */
+	regmap_reg_range(SX932X_REG_AFE_CTRL0, SX932X_REG_AFE_CTRL9),
+	regmap_reg_range(SX932X_REG_PROX_CTRL0, SX932X_REG_PROX_CTRL7),
+	regmap_reg_range(SX932X_REG_ADV_CTRL0, SX932X_REG_ADV_CTRL20),
+	regmap_reg_range(SX932X_REG_PHASE_SEL, SX932X_REG_PHASE_SEL),
+	regmap_reg_range(SX932X_REG_OFFSET_MSB, SX932X_REG_OFFSET_LSB),
+	regmap_reg_range(SX932X_REG_RESET, SX932X_REG_RESET),

drivers/iio/proximity/sx932x.c line 306

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+};
+
+static const struct regmap_access_table sx932x_writeable_regs = {
+	.yes_ranges = sx932x_writable_reg_ranges,
+	.n_yes_ranges = ARRAY_SIZE(sx932x_writable_reg_ranges),
+};
+
+/*
+ * All allocated registers are readable, so we just list unallocated
+ * ones.

Whilst it might be shorter, I would prefer a positive list to the negative one based on 1 more than a register... Easier to audit if the datasheet is available.

Quoted 39 lines
drivers/iio/proximity/sx932x.c new file +39 -0
+ */
+static const struct regmap_range sx932x_non_readable_reg_ranges[] = {
+	regmap_reg_range(SX932X_REG_IRQ_CFG2 + 1, SX932X_REG_GNRL_CTRL0 - 1),
+	regmap_reg_range(SX932X_REG_GNRL_CTRL1 + 1, SX932X_REG_AFE_CTRL0 - 1),
+	regmap_reg_range(SX932X_REG_AFE_CTRL9 + 1, SX932X_REG_PROX_CTRL0 - 1),
+	regmap_reg_range(SX932X_REG_PROX_CTRL7 + 1, SX932X_REG_ADV_CTRL0 - 1),
+	regmap_reg_range(SX932X_REG_ADV_CTRL20 + 1, SX932X_REG_PHASE_SEL - 1),
+	regmap_reg_range(SX932X_REG_SAR_LSB + 1, SX932X_REG_RESET - 1),
+	regmap_reg_range(SX932X_REG_RESET + 1, SX932X_REG_WHOAMI - 1),
+	regmap_reg_range(SX932X_REG_WHOAMI + 1, SX932X_REG_REVISION - 1),
+};
+
+static const struct regmap_access_table sx932x_readable_regs = {
+	.no_ranges = sx932x_non_readable_reg_ranges,
+	.n_no_ranges = ARRAY_SIZE(sx932x_non_readable_reg_ranges),
+};
+
+static const struct regmap_range sx932x_volatile_reg_ranges[] = {
+	regmap_reg_range(SX932X_REG_IRQ_SRC, SX932X_REG_STAT3),
+	regmap_reg_range(SX932X_REG_USE_MSB, SX932X_REG_DIFF_LSB),
+	regmap_reg_range(SX932X_REG_SAR_MSB, SX932X_REG_SAR_LSB),
+	regmap_reg_range(SX932X_REG_WHOAMI, SX932X_REG_WHOAMI),
+	regmap_reg_range(SX932X_REG_REVISION, SX932X_REG_REVISION),
+};
+
+static const struct regmap_access_table sx932x_volatile_regs = {
+	.yes_ranges = sx932x_volatile_reg_ranges,
+	.n_yes_ranges = ARRAY_SIZE(sx932x_volatile_reg_ranges),
+};
+
+static const struct regmap_config sx932x_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.max_register = SX932X_REG_REVISION,
+	.cache_type = REGCACHE_RBTREE,
+
+	.wr_table = &sx932x_writeable_regs,
+	.rd_table = &sx932x_readable_regs,

drivers/iio/proximity/sx932x.c line 355

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+	.volatile_table = &sx932x_volatile_regs,
+};
+
+static int sx932x_inc_users(struct sx932x_data *data, int *counter,
+			    unsigned int reg, unsigned int bitmask)
+{
+	++(*counter);
+	if (*counter != 1)
+		/* Bit is already active, nothing to do. */
+		return 0;

Hmm. I want a full description of what this reference counting is actually doing. It is possible to work out, but I'm not sure what the hardware constraints actually are.

There are 3 things that can be going on that might result in a channel needing to be enabled

1. Buffered capture of that channel
2. Events on that channel
3. An individual read of that channel.

1 and 3 are sort of prevented from happening at the same time (I'm not sure this intended or not).

Anyhow, looks like 3 simple masks to me rather than reference counting. In paths were it 'might' make sense to disable a given channel then the OR of these can be checked to see if someone else is using it.

It might be that what you have simpler, but I'm not convinced yet.

Quoted 41 lines
drivers/iio/proximity/sx932x.c new file +41 -0
+
+	return regmap_update_bits(data->regmap, reg, bitmask, bitmask);
+}
+
+static int sx932x_dec_users(struct sx932x_data *data, int *counter,
+			    unsigned int reg, unsigned int bitmask)
+{
+	--(*counter);
+	if (*counter != 0)
+		/* There are more users, do not deactivate. */
+		return 0;
+
+	return regmap_update_bits(data->regmap, reg, bitmask, 0);
+}
+
+static int sx932x_inc_chan_users(struct sx932x_data *data, int chan)
+{
+	return sx932x_inc_users(data, &data->channel_users[chan],
+				SX932X_REG_PROX_CTRL0, BIT(chan));
+}
+
+static int sx932x_dec_chan_users(struct sx932x_data *data, int chan)
+{
+	return sx932x_dec_users(data, &data->channel_users[chan],
+				SX932X_REG_PROX_CTRL0, BIT(chan));
+}
+
+static int sx932x_inc_data_rdy_users(struct sx932x_data *data)
+{
+	return sx932x_inc_users(data, &data->data_rdy_users,
+				SX932X_REG_IRQ_MSK, SX932X_CONVDONE_IRQ);
+}
+
+static int sx932x_dec_data_rdy_users(struct sx932x_data *data)
+{
+	return sx932x_dec_users(data, &data->data_rdy_users,
+				SX932X_REG_IRQ_MSK, SX932X_CONVDONE_IRQ);
+}
+
+static int sx932x_inc_close_far_users(struct sx932x_data *data)
+{

drivers/iio/proximity/sx932x.c line 406

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+	return sx932x_inc_users(data, &data->close_far_users,
+				SX932X_REG_IRQ_MSK,
+				SX932X_CLOSE_IRQ | SX932X_FAR_IRQ);
+}
+
+static int sx932x_dec_close_far_users(struct sx932x_data *data)
+{
+	return sx932x_dec_users(data, &data->close_far_users,
+				SX932X_REG_IRQ_MSK,
+				SX932X_CLOSE_IRQ | SX932X_FAR_IRQ);

Given we have separate irqs for close and far why not support separately enabled interrupts for the two?

Quoted 9 lines
drivers/iio/proximity/sx932x.c new file +9 -0
+}
+
+static int sx932x_read_prox_data(struct sx932x_data *data,
+				 const struct iio_chan_spec *chan,
+				 int *val)
+{
+	int ret;
+	__be16 regval;
+	const int regid = chan->channel & 3;

drivers/iio/proximity/sx932x.c line 425

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+
+	ret = regmap_write(data->regmap, SX932X_REG_PHASE_SEL, regid);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_bulk_read(data->regmap, chan->address, &regval, 2);
+	if (ret < 0)
+		return ret;
+
+	*val = sign_extend32(be16_to_cpu(regval), 15);

This should only be in the read_raw case. Where buffers are in use we should just pass it up be16 and let userspace deal with it. Also should not be sign extended for that case - or indeed converted to a 32 bit (to be truncated when put into the buffer).

Quoted 4 lines
drivers/iio/proximity/sx932x.c new file +4 -0
+
+	return 0;
+}
+

drivers/iio/proximity/sx932x.c line 439

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+static int sx932x_read_stat_data(struct sx932x_data *data,
+		const struct iio_chan_spec
+		*chan,
+		int *val)
+{
+	int ret;
+
+	ret = regmap_read(data->regmap, chan->address, val);
+	if (ret < 0)
+		return ret;

regmap read always I think returns 0 or < 0 so just return regmap_read(...)

Quoted 17 lines
drivers/iio/proximity/sx932x.c new file +17 -0
+
+	return 0;
+}
+
+/*
+ * If we have no interrupt support, we have to wait for a scan period
+ * after enabling a channel to get a result.
+ */
+static int sx932x_wait_for_sample(struct sx932x_data *data)
+{
+	int ret;
+	unsigned int val;
+
+	ret = regmap_read(data->regmap, SX932X_REG_PROX_CTRL0, &val);
+	if (ret < 0)
+		return ret;
+

drivers/iio/proximity/sx932x.c line 466

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+	val = (val & SX932X_SCAN_PERIOD_MASK) >> SX932X_SCAN_PERIOD_SHIFT;
+
+	msleep(sx932x_scan_period_table[val]);
+
+	return 0;
+}
+
+static int sx932x_read_stat(struct sx932x_data *data,
+				 const struct iio_chan_spec *chan,
+				 int *val)

I don't think this is ever actually called...

Quoted 85 lines
drivers/iio/proximity/sx932x.c new file +85 -0
+{
+	int ret;
+
+	mutex_lock(&data->mutex);
+
+	ret = sx932x_inc_chan_users(data, chan->channel);
+	if (ret < 0)
+		goto out;
+
+	ret = sx932x_inc_data_rdy_users(data);
+	if (ret < 0)
+		goto out_dec_chan;
+
+	mutex_unlock(&data->mutex);
+
+	if (data->client->irq > 0)
+		ret = wait_for_completion_interruptible(&data->completion);
+	else
+		ret = sx932x_wait_for_sample(data);
+
+	mutex_lock(&data->mutex);
+
+	if (ret < 0)
+		goto out_dec_data_rdy;
+
+	ret = sx932x_read_stat_data(data, chan, val);
+	if (ret < 0)
+		goto out_dec_data_rdy;
+
+	ret = sx932x_dec_data_rdy_users(data);
+	if (ret < 0)
+		goto out_dec_chan;
+
+	ret = sx932x_dec_chan_users(data, chan->channel);
+	if (ret < 0)
+		goto out;
+
+	ret = IIO_VAL_INT;
+
+	goto out;
+
+out_dec_data_rdy:
+	sx932x_dec_data_rdy_users(data);
+out_dec_chan:
+	sx932x_dec_chan_users(data, chan->channel);
+out:
+	mutex_unlock(&data->mutex);
+	reinit_completion(&data->completion);
+
+	return ret;
+}
+
+static int sx932x_read_proximity(struct sx932x_data *data,
+				 const struct iio_chan_spec *chan,
+				 int *val)
+{
+	int ret;
+
+	mutex_lock(&data->mutex);
+
+	ret = sx932x_inc_chan_users(data, chan->channel);
+	if (ret < 0)
+		goto out;
+
+	ret = sx932x_inc_data_rdy_users(data);
+	if (ret < 0)
+		goto out_dec_chan;
+
+	mutex_unlock(&data->mutex);
+
+	if (data->client->irq > 0)
+		ret = wait_for_completion_interruptible(&data->completion);
+	else
+		ret = sx932x_wait_for_sample(data);
+
+	mutex_lock(&data->mutex);
+
+	if (ret < 0)
+		goto out_dec_data_rdy;
+
+	ret = sx932x_read_prox_data(data, chan, val);
+	if (ret < 0)
+		goto out_dec_data_rdy;
+
+	ret = sx932x_dec_data_rdy_users(data);

drivers/iio/proximity/sx932x.c line 561

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+	if (ret < 0)
+		goto out_dec_chan;
+
+	ret = sx932x_dec_chan_users(data, chan->channel);
+	if (ret < 0)
+		goto out;
+
+	ret = IIO_VAL_INT;
+
+	goto out;

Separate the error path so you don't need to jump cleanup. That makes for messy and hard to read code to just save a few lines.

Quoted 35 lines
drivers/iio/proximity/sx932x.c new file +35 -0
+
+out_dec_data_rdy:
+	sx932x_dec_data_rdy_users(data);
+out_dec_chan:
+	sx932x_dec_chan_users(data, chan->channel);
+out:
+	mutex_unlock(&data->mutex);
+	reinit_completion(&data->completion);
+
+	return ret;
+}
+
+static int sx932x_read_samp_freq(struct sx932x_data *data,
+				 int *val, int *val2)
+{
+	int ret;
+	unsigned int regval;
+
+	mutex_lock(&data->mutex);
+	ret = regmap_read(data->regmap, SX932X_REG_PROX_CTRL0, &regval);
+	mutex_unlock(&data->mutex);
+
+	if (ret < 0)
+		return ret;
+
+	regval = (regval & SX932X_SCAN_PERIOD_MASK) >> SX932X_SCAN_PERIOD_SHIFT;
+	*val = sx932x_samp_freq_table[regval].val;
+	*val2 = sx932x_samp_freq_table[regval].val2;
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int sx932x_read_raw(struct iio_dev *indio_dev,
+			   const struct iio_chan_spec *chan,
+			   int *val, int *val2, long mask)

drivers/iio/proximity/sx932x.c line 606

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+{
+	struct sx932x_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (chan->type) {
+	case IIO_PROXIMITY:
+		switch (mask) {
+		case IIO_CHAN_INFO_RAW:
+			mutex_lock(&indio_dev->mlock);
+

Use the iio_device_claim_direct_mode helper. Whilst you have this correct I think, the helper save us checking that the locking right :)

drivers/iio/proximity/sx932x.c line 614

Quoted 8 lines
drivers/iio/proximity/sx932x.c new file +8 -0
+			if (iio_buffer_enabled(indio_dev)) {
+				mutex_unlock(&indio_dev->mlock);
+				return -EBUSY;
+			}
+			ret = sx932x_read_proximity(data, chan, val);
+			mutex_unlock(&indio_dev->mlock);
+			return ret;
+		case IIO_CHAN_INFO_PROCESSED:

This bothers me. A given channel should only ever be either raw (if you can provide scale and offset to allow userspace to get to the processed value) or processed. Never both. I don't think you ever actually use this path though so just clean it up and all the functions that are only called from here..

Quoted 59 lines
drivers/iio/proximity/sx932x.c new file +59 -0
+			mutex_lock(&indio_dev->mlock);
+
+			if (iio_buffer_enabled(indio_dev)) {
+				mutex_unlock(&indio_dev->mlock);
+				return -EBUSY;
+			}
+			ret = sx932x_read_stat(data, chan, val);
+			mutex_unlock(&indio_dev->mlock);
+			return ret;
+		case IIO_CHAN_INFO_SAMP_FREQ:
+			return sx932x_read_samp_freq(data, val, val2);
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static int sx932x_set_samp_freq(struct sx932x_data *data,
+				int val, int val2)
+{
+	int i, ret;
+
+	for (i = 0; i < ARRAY_SIZE(sx932x_samp_freq_table); i++)
+		if (val == sx932x_samp_freq_table[i].val &&
+		    val2 == sx932x_samp_freq_table[i].val2)
+			break;
+
+	if (i == ARRAY_SIZE(sx932x_samp_freq_table))
+		return -EINVAL;
+
+	mutex_lock(&data->mutex);
+
+	ret = regmap_update_bits(data->regmap, SX932X_REG_PROX_CTRL0,
+				 SX932X_SCAN_PERIOD_MASK,
+				 i << SX932X_SCAN_PERIOD_SHIFT);
+
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int sx932x_write_raw(struct iio_dev *indio_dev,
+			    const struct iio_chan_spec *chan,
+			    int val, int val2, long mask)
+{
+	struct sx932x_data *data = iio_priv(indio_dev);
+
+	switch (chan->type) {
+	case IIO_PROXIMITY:
+		switch (mask) {
+		case IIO_CHAN_INFO_SAMP_FREQ:
+			return sx932x_set_samp_freq(data, val, val2);
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;

drivers/iio/proximity/sx932x.c line 683

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+	}
+}
+
+static irqreturn_t sx932x_irq_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct sx932x_data *data = iio_priv(indio_dev);
+
+	if (data->trigger_enabled)
+		iio_trigger_poll(data->trig);

Is it possible to have an interrupt that doesn't include the dataready signal? I'm guessing no...

Quoted 21 lines
drivers/iio/proximity/sx932x.c new file +21 -0
+
+	/*
+	 * Even if no event is enabled, we need to wake the thread to
+	 * clear the interrupt state by reading SX932X_REG_IRQ_SRC.  It
+	 * is not possible to do that here because regmap_read takes a
+	 * mutex.
+	 */
+	return IRQ_WAKE_THREAD;
+}
+
+static void sx932x_push_events(struct iio_dev *indio_dev)
+{
+	int ret;
+	unsigned int val, chan;
+	struct sx932x_data *data = iio_priv(indio_dev);
+
+	ret = regmap_read(data->regmap, SX932X_REG_STAT0, &val);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "i2c transfer error in irq\n");
+		return;
+	}

drivers/iio/proximity/sx932x.c line 714

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+
+	for (chan = 0; chan < SX932X_NUM_CHANNELS; chan++) {
+		int dir;
+		u64 ev;
+		bool new_prox = val & BIT(chan);
+
+		if (!data->event_enabled[chan])
+			continue;
+		if (new_prox == data->prox_stat[chan])
+			/* No change on this channel. */

So we are looking at a level state register and edge interrupt? Perhaps add a comment to make this clear.

drivers/iio/proximity/sx932x.c line 720

Quoted 6 lines
drivers/iio/proximity/sx932x.c new file +6 -0
+			continue;
+
+		dir = new_prox ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
+		ev = IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, chan,
+					  IIO_EV_TYPE_THRESH, dir);
+		iio_push_event(indio_dev, ev, iio_get_time_ns(indio_dev));

Hmm. Should this be a single timestamp for all events? In theory we could have more turn up here that weren't true when the interrupt fires so I guess this is better.

Quoted 31 lines
drivers/iio/proximity/sx932x.c new file +31 -0
+		data->prox_stat[chan] = new_prox;
+	}
+}
+
+static irqreturn_t sx932x_irq_thread_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct sx932x_data *data = iio_priv(indio_dev);
+	int ret;
+	unsigned int val;
+
+	mutex_lock(&data->mutex);
+
+	ret = regmap_read(data->regmap, SX932X_REG_IRQ_SRC, &val);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "i2c transfer error in irq\n");
+		goto out;
+	}
+
+	if (val & (SX932X_CLOSE_IRQ | SX932X_FAR_IRQ))
+		sx932x_push_events(indio_dev);
+
+	if (val & SX932X_CONVDONE_IRQ)
+		complete(&data->completion);
+
+out:
+	mutex_unlock(&data->mutex);
+
+	return IRQ_HANDLED;
+}
+

drivers/iio/proximity/sx932x.c line 761

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+static int sx932x_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 sx932x_data *data = iio_priv(indio_dev);
+
+	if (chan->type != IIO_PROXIMITY || type != IIO_EV_TYPE_THRESH ||
+	    dir != IIO_EV_DIR_EITHER)
+		return -EINVAL;

Given that you don't register this for other options, that check cannot ever fail as far as I can see. I would drop it.

Quoted 11 lines
drivers/iio/proximity/sx932x.c new file +11 -0
+
+	return data->event_enabled[chan->channel & SX932X_CHAN_MASK];
+}
+
+static int sx932x_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 sx932x_data *data = iio_priv(indio_dev);

drivers/iio/proximity/sx932x.c line 782

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+	int ret;
+
+	if (chan->type != IIO_PROXIMITY || type != IIO_EV_TYPE_THRESH ||
+	    dir != IIO_EV_DIR_EITHER)
+		return -EINVAL;
+
+	mutex_lock(&data->mutex);
+
+	if (state == 1) {
+		ret = sx932x_inc_chan_users(data, chan->channel);

Ah. I'm starting to understand what is going on here. You need to enable the channels if you are either reading them, or you have their events enabled. That doesn't need full reference counting. Just have a pair of masks and track the | of the two of them.

Quoted 25 lines
drivers/iio/proximity/sx932x.c new file +25 -0
+		if (ret < 0)
+			goto out_unlock;
+		ret = sx932x_inc_close_far_users(data);
+		if (ret < 0)
+			goto out_undo_chan;
+	} else {
+		ret = sx932x_dec_chan_users(data, chan->channel);
+		if (ret < 0)
+			goto out_unlock;
+		ret = sx932x_dec_close_far_users(data);
+		if (ret < 0)
+			goto out_undo_chan;
+	}
+
+	data->event_enabled[chan->channel & SX932X_CHAN_MASK] = state;
+	goto out_unlock;
+
+out_undo_chan:
+	if (state == 1)
+		sx932x_dec_chan_users(data, chan->channel);
+	else
+		sx932x_inc_chan_users(data, chan->channel);
+out_unlock:
+	mutex_unlock(&data->mutex);
+	return ret;

drivers/iio/proximity/sx932x.c line 817

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+}
+
+static int sx932x_update_scan_mode(struct iio_dev *indio_dev,
+				   const unsigned long *scan_mask)
+{
+	struct sx932x_data *data = iio_priv(indio_dev);
+
+	mutex_lock(&data->mutex);
+	kfree(data->buffer);
+	data->buffer = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);

What's the maximum? My suspicion is that it's not that large so you should just put it in the data structure directly, sized to take the maximum and not worry about this complexity.

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+	mutex_unlock(&data->mutex);
+
+	if (data->buffer == NULL)
+		return -ENOMEM;
+
+	return 0;
+}
+
+static ssize_t sx932x_uid_show(struct device *dev,
+			       struct device_attribute *attr,

drivers/iio/proximity/sx932x.c line 837

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+			       char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+#ifdef CONFIG_ACPI
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+
+	if (adev != NULL)
+		return scnprintf(buf, PAGE_SIZE, "%s\n", acpi_device_uid(adev));
+#endif
+	return scnprintf(buf, PAGE_SIZE, "%d\n", indio_dev->id);

No to this one. There are lots of ways of finding out an ACPI uid, we don't need it exposed as ABI in IIO.

Quoted 106 lines
drivers/iio/proximity/sx932x.c new file +106 -0
+}
+
+static IIO_DEVICE_ATTR(uid, 0444, sx932x_uid_show, NULL, 0);
+
+struct sx932x_ctrl_attribute {
+	struct device_attribute dev_attr;
+	u8 ctrl;
+};
+
+static inline struct sx932x_ctrl_attribute *sx932x_to_ctrl_attr(
+		struct device_attribute *attr)
+{
+	return container_of(attr, struct sx932x_ctrl_attribute, dev_attr);
+}
+
+static ssize_t sx932x_show_ctrl_attr(struct device *dev,
+				     struct device_attribute *dev_attr,
+				     char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct sx932x_ctrl_attribute *attr = sx932x_to_ctrl_attr(dev_attr);
+	struct sx932x_data *data = iio_priv(indio_dev);
+	unsigned int val;
+	int ret;
+
+	mutex_lock(&data->mutex);
+	ret = regmap_read(data->regmap, attr->ctrl, &val);
+	mutex_unlock(&data->mutex);
+	if (ret)
+		return ret;
+
+	return scnprintf(buf, PAGE_SIZE, "%02x\n", val);
+}
+
+static ssize_t sx932x_store_ctrl_attr(struct device *dev,
+				      struct device_attribute *dev_attr,
+				      const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct sx932x_ctrl_attribute *attr = sx932x_to_ctrl_attr(dev_attr);
+	struct sx932x_data *data = iio_priv(indio_dev);
+	u8 val;
+	int ret;
+
+	if (kstrtou8(buf, 0, &val))
+		return -EINVAL;
+
+	mutex_lock(&data->mutex);
+	ret = regmap_write(data->regmap, attr->ctrl, val);
+	mutex_unlock(&data->mutex);
+	if (ret)
+		return ret;
+
+	return len;
+}
+
+#define SX932X_CTRL_ATTR_FILL(_name, _mode, _show, _store, _ctrl, _offset) \
+	{ .dev_attr	= __ATTR(_name, _mode, _show, _store),		\
+	  .ctrl		= _ctrl + _offset }
+
+#define SX932X_CTRL_ATTR(_name, _mode, _show, _store, _ctrl, _offset)	\
+	struct sx932x_ctrl_attribute sx932x_ctrl_attr_##_name =		\
+		SX932X_CTRL_ATTR_FILL( \
+			_name, _mode, _show, _store, _ctrl, _offset)
+
+#define SX932X_ADV_CTRL_RW(_ctrl)					\
+	SX932X_CTRL_ATTR(adv_ctrl##_ctrl##_raw,	0644,			\
+			 sx932x_show_ctrl_attr, sx932x_store_ctrl_attr,	\
+			 _ctrl, SX932X_REG_ADV_CTRL0)
+
+static SX932X_ADV_CTRL_RW(0);
+static SX932X_ADV_CTRL_RW(1);
+static SX932X_ADV_CTRL_RW(2);
+static SX932X_ADV_CTRL_RW(3);
+static SX932X_ADV_CTRL_RW(4);
+static SX932X_ADV_CTRL_RW(5);
+static SX932X_ADV_CTRL_RW(6);
+static SX932X_ADV_CTRL_RW(7);
+static SX932X_ADV_CTRL_RW(8);
+static SX932X_ADV_CTRL_RW(9);
+static SX932X_ADV_CTRL_RW(10);
+static SX932X_ADV_CTRL_RW(11);
+static SX932X_ADV_CTRL_RW(12);
+static SX932X_ADV_CTRL_RW(13);
+static SX932X_ADV_CTRL_RW(14);
+static SX932X_ADV_CTRL_RW(15);
+static SX932X_ADV_CTRL_RW(16);
+static SX932X_ADV_CTRL_RW(17);
+static SX932X_ADV_CTRL_RW(18);
+static SX932X_ADV_CTRL_RW(19);
+static SX932X_ADV_CTRL_RW(20);
+
+static struct attribute *sx932x_attributes[] = {
+	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
+	&iio_dev_attr_uid.dev_attr.attr,
+	&sx932x_ctrl_attr_adv_ctrl0_raw.dev_attr.attr,
+	&sx932x_ctrl_attr_adv_ctrl1_raw.dev_attr.attr,
+	&sx932x_ctrl_attr_adv_ctrl2_raw.dev_attr.attr,
+	&sx932x_ctrl_attr_adv_ctrl3_raw.dev_attr.attr,
+	&sx932x_ctrl_attr_adv_ctrl4_raw.dev_attr.attr,
+	&sx932x_ctrl_attr_adv_ctrl5_raw.dev_attr.attr,
+	&sx932x_ctrl_attr_adv_ctrl6_raw.dev_attr.attr,
+	&sx932x_ctrl_attr_adv_ctrl7_raw.dev_attr.attr,
+	&sx932x_ctrl_attr_adv_ctrl8_raw.dev_attr.attr,
+	&sx932x_ctrl_attr_adv_ctrl9_raw.dev_attr.attr,
+	&sx932x_ctrl_attr_adv_ctrl10_raw.dev_attr.attr,

drivers/iio/proximity/sx932x.c line 953

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+	&sx932x_ctrl_attr_adv_ctrl11_raw.dev_attr.attr,
+	&sx932x_ctrl_attr_adv_ctrl12_raw.dev_attr.attr,
+	&sx932x_ctrl_attr_adv_ctrl13_raw.dev_attr.attr,
+	&sx932x_ctrl_attr_adv_ctrl14_raw.dev_attr.attr,
+	&sx932x_ctrl_attr_adv_ctrl15_raw.dev_attr.attr,
+	&sx932x_ctrl_attr_adv_ctrl16_raw.dev_attr.attr,
+	&sx932x_ctrl_attr_adv_ctrl17_raw.dev_attr.attr,
+	&sx932x_ctrl_attr_adv_ctrl18_raw.dev_attr.attr,
+	&sx932x_ctrl_attr_adv_ctrl19_raw.dev_attr.attr,
+	&sx932x_ctrl_attr_adv_ctrl20_raw.dev_attr.attr,

I had a feeling that would be somewhere. Do not have raw registers exposed in sysfs. If you really want them for debug reasons, read only access in debugfs is reasonable - possibly even write access though probably not...

They cannot form part of a remotely useable or generic userspace ABI.

Quoted 17 lines
drivers/iio/proximity/sx932x.c new file +17 -0
+	NULL,
+};
+
+static const struct attribute_group sx932x_attribute_group = {
+	.attrs = sx932x_attributes,
+};
+
+static const struct iio_info sx932x_info = {
+	.attrs = &sx932x_attribute_group,
+	.read_raw = &sx932x_read_raw,
+	.write_raw = &sx932x_write_raw,
+	.read_event_config = &sx932x_read_event_config,
+	.write_event_config = &sx932x_write_event_config,
+	.update_scan_mode = &sx932x_update_scan_mode,
+};
+
+static int sx932x_set_trigger_state(struct iio_trigger *trig,

drivers/iio/proximity/sx932x.c line 980

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+				    bool state)
+{
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+	struct sx932x_data *data = iio_priv(indio_dev);
+	int ret;
+
+	mutex_lock(&data->mutex);
+
+	if (state)
+		ret = sx932x_inc_data_rdy_users(data);

It feels like you are doing this twice - once in the trigger setup and once in the buffer enable. That doesn't seem likely to be correct.

Quoted 20 lines
drivers/iio/proximity/sx932x.c new file +20 -0
+	else
+		ret = sx932x_dec_data_rdy_users(data);
+	if (ret < 0)
+		goto out;
+
+	data->trigger_enabled = state;
+
+out:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static const struct iio_trigger_ops sx932x_trigger_ops = {
+	.set_trigger_state = sx932x_set_trigger_state,
+};
+
+static irqreturn_t sx932x_trigger_handler(int irq, void *private)
+{
+	struct iio_poll_func *pf = private;

drivers/iio/proximity/sx932x.c line 1010

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct sx932x_data *data = iio_priv(indio_dev);
+	int val, bit, ret, i = 0;
+
+	mutex_lock(&data->mutex);
+
+	for_each_set_bit(bit, indio_dev->active_scan_mask,
+			 indio_dev->masklength) {
+		ret = sx932x_read_prox_data(data, &indio_dev->channels[bit],
+					    &val);

so there is an endian conversion in this call that we would normally leave to userspace if possible..

Quoted 15 lines
drivers/iio/proximity/sx932x.c new file +15 -0
+		if (ret < 0)
+			goto out;
+
+		data->buffer[i++] = val;
+	}
+
+	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
+					   iio_get_time_ns(indio_dev));
+out:
+	mutex_unlock(&data->mutex);
+
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}

drivers/iio/proximity/sx932x.c line 1035

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+
+static int sx932x_buffer_preenable(struct iio_dev *indio_dev)
+{
+	struct sx932x_data *data = iio_priv(indio_dev);
+	int ret = 0, i;
+
+	mutex_lock(&data->mutex);
+
+	for (i = 0; i < SX932X_NUM_CHANNELS; i++)
+		if (test_bit(i, indio_dev->active_scan_mask)) {

for_each_set_bit

drivers/iio/proximity/sx932x.c line 1044

Quoted 9 lines
drivers/iio/proximity/sx932x.c new file +9 -0
+			ret = sx932x_inc_chan_users(data, i);
+			if (ret)
+				break;
+		}
+
+	if (ret)
+		for (i = i - 1; i >= 0; i--)
+			if (test_bit(i, indio_dev->active_scan_mask))
+				sx932x_dec_chan_users(data, i);

Pull this error block out of the main flow via a goto.

Quoted 7 lines
drivers/iio/proximity/sx932x.c new file +7 -0
+
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int sx932x_buffer_predisable(struct iio_dev *indio_dev)

drivers/iio/proximity/sx932x.c line 1061

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+{
+	struct sx932x_data *data = iio_priv(indio_dev);
+	int ret = 0, i;
+
+	iio_triggered_buffer_predisable(indio_dev);
+
+	mutex_lock(&data->mutex);
+
+	for (i = 0; i < SX932X_NUM_CHANNELS; i++)
+		if (test_bit(i, indio_dev->active_scan_mask)) {

for_each_set_bit

drivers/iio/proximity/sx932x.c line 1070

Quoted 9 lines
drivers/iio/proximity/sx932x.c new file +9 -0
+			ret = sx932x_dec_chan_users(data, i);
+			if (ret)
+				break;
+		}
+
+	if (ret)
+		for (i = i - 1; i >= 0; i--)
+			if (test_bit(i, indio_dev->active_scan_mask))
+				sx932x_inc_chan_users(data, i);

I'm finding it hard to understand why this complexity is needed. IIO handles muxing multiple readers explicitly with all the necessary reference counting. What case are we missing that this covers? (Ah, I think I found it above. Events need the channels enabled..)

Quoted 5 lines
drivers/iio/proximity/sx932x.c new file +5 -0
+
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}

drivers/iio/proximity/sx932x.c line 1085

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+
+static const struct iio_buffer_setup_ops sx932x_buffer_setup_ops = {
+	.preenable = sx932x_buffer_preenable,
+	.postenable = iio_triggered_buffer_postenable,
+	.predisable = sx932x_buffer_predisable,
+};
+#define SX932X_REG_CONFIG(_property_prefix, _property_name, _reg_addr,         \
+			  _def_value)                                          \
+	{                                                                      \
+		.property = _property_prefix ",reg_" _property_name,           \

This property prefix stuff will hopefully go away once we've dropped the DT binding. As I've said the ACPI binding is hideous if there are actual instances of what you describe. I would prefer to loose the abstraction here and have that all named explicitly where we use it.

drivers/iio/proximity/sx932x.c line 1094

Quoted 9 lines
drivers/iio/proximity/sx932x.c new file +9 -0
+		.reg_addr = SX932X_REG_##_reg_addr, .def_value = _def_value,   \
+	}
+
+#define SX932X_DEFAULT_REG_CONFIGS(_property_prefix)                           \
+	{                                                                      \
+		{                                                              \
+			.property = NULL,                                      \
+			.reg_addr = SX932X_REG_ADV_CTRL11,                     \
+			.def_value = (5 << 4) | 2,                             \

Do we know what these magic values are? Please use defines to make the clear. It would be much nicer to see the device configured element by element in code rather than a register dump like this.

drivers/iio/proximity/sx932x.c line 1095

Quoted 1 line
drivers/iio/proximity/sx932x.c new file +1 -0
+		},                                                             \

Fix the odd indenting.

Quoted 33 lines
drivers/iio/proximity/sx932x.c new file +33 -0
+			{                                                      \
+				.property = NULL,                              \
+				.reg_addr = SX932X_REG_ADV_CTRL12,             \
+				.def_value = (11 << 4) | 5,                    \
+			},                                                     \
+			SX932X_REG_CONFIG(_property_prefix, "adv_ctrl10",      \
+					  ADV_CTRL10, (5 << 4) | 12),          \
+			SX932X_REG_CONFIG(_property_prefix, "afe_ctrl4",       \
+					  AFE_CTRL4, 7),                       \
+			SX932X_REG_CONFIG(_property_prefix, "afe_ctrl7",       \
+					  AFE_CTRL7, 7),                       \
+			SX932X_REG_CONFIG(_property_prefix, "afe_ctrl9",       \
+					  AFE_CTRL9, 15),                      \
+			SX932X_REG_CONFIG(_property_prefix, "prox_ctrl0",      \
+					  PROX_CTRL0, (2 << 3) | 2),           \
+			SX932X_REG_CONFIG(_property_prefix, "prox_ctrl1",      \
+					  PROX_CTRL1, (2 << 3) | 2),           \
+			SX932X_REG_CONFIG(_property_prefix, "prox_ctrl2",      \
+					  PROX_CTRL2, 0x80 | 16),              \
+			SX932X_REG_CONFIG(_property_prefix, "prox_ctrl4",      \
+					  PROX_CTRL4, (1 << 3) | 4),           \
+			SX932X_REG_CONFIG(_property_prefix, "prox_ctrl5",      \
+					  PROX_CTRL5, (1 << 4) | 2),           \
+			SX932X_REG_CONFIG(_property_prefix, "prox_ctrl6",      \
+					  PROX_CTRL6, 60),                     \
+			SX932X_REG_CONFIG(_property_prefix, "prox_ctrl7",      \
+					  PROX_CTRL7, 88),                     \
+			SX932X_REG_CONFIG(_property_prefix, "adv_ctrl16",      \
+					  ADV_CTRL16, (3 << 4) | (2 << 2)),    \
+			SX932X_REG_CONFIG(_property_prefix, "adv_ctrl17",      \
+					  ADV_CTRL17, (5 << 4) | 6),           \
+			SX932X_REG_CONFIG(_property_prefix, "adv_ctrl18",      \
+					  ADV_CTRL18, (3 << 4) | 3),           \

drivers/iio/proximity/sx932x.c line 1138

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+			SX932X_REG_CONFIG(_property_prefix, "gnrl_ctrl1",      \
+					  GNRL_CTRL1, 0x2f),                   \
+			{                                                      \
+				.property = NULL,                              \
+				.reg_addr = 0,                                 \
+				.def_value = 0,                                \
+			},                                                     \
+	}
+
+#ifdef CONFIG_ACPI

ACPI and DT will typically be configured on for example a defconfig arm64 build.

Quoted 11 lines
drivers/iio/proximity/sx932x.c new file +11 -0
+static const struct sx932x_reg_config sx9320_reg_config[] =
+	SX932X_DEFAULT_REG_CONFIGS(SX9320_ACPI_NAME);
+static const struct sx932x_reg_config sx9321_reg_config[] =
+	SX932X_DEFAULT_REG_CONFIGS(SX9321_ACPI_NAME);
+static const struct sx932x_reg_config sx9324_reg_config[] =
+	SX932X_DEFAULT_REG_CONFIGS(SX9324_ACPI_NAME);
+#else
+static const struct sx932x_reg_config sx9320_reg_config[] =
+	SX932X_DEFAULT_REG_CONFIGS("semtech,");
+static const struct sx932x_reg_config sx9321_reg_config[] =
+	SX932X_DEFAULT_REG_CONFIGS("semtech,");

drivers/iio/proximity/sx932x.c line 1159

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+static const struct sx932x_reg_config sx9324_reg_config[] =
+	SX932X_DEFAULT_REG_CONFIGS("semtech,");
+#endif
+static const struct sx932x_reg_config *sx932x_reg_configs[] = {
+	sx9320_reg_config, sx9321_reg_config, sx9324_reg_config
+};
+static int sx932x_read_register_property(struct device *dev,
+					 const struct sx932x_reg_config *cfg,
+					 u8 *value)
+{

This looks like a lot of 'magic' register fun. Are the lots of random ACPI DSDT files out there with different values? Can we figure out a good common default?

This is a truely horrible mess. Note that I'm definitely not keen on propagating this into DT, but if we have cope with ACPI on existing systems I guess we can...

drivers/iio/proximity/sx932x.c line 1162

Quoted 3 lines
drivers/iio/proximity/sx932x.c new file +3 -0
+#ifdef CONFIG_ACPI
+	const union acpi_object *acpi_value = NULL;
+#endif

It's perfectly possible to have both ACPI and OF built, so you need to handle that case nicely.

drivers/iio/proximity/sx932x.c line 1165

Quoted 3 lines
drivers/iio/proximity/sx932x.c new file +3 -0
+	int ret;
+
+	if ((dev == NULL) || (cfg->property == NULL)) {

if (!dev || !cfg->property) {

Quoted 36 lines
drivers/iio/proximity/sx932x.c new file +36 -0
+		*value = cfg->def_value;
+		return 0;
+	}
+
+#ifdef CONFIG_ACPI
+	ret = acpi_dev_get_property(ACPI_COMPANION(dev), cfg->property,
+				    ACPI_TYPE_INTEGER, &acpi_value);
+#else
+	ret = of_property_read_u8(dev->of_node, cfg->property, value);
+#endif
+	if (ret)
+		*value = cfg->def_value;
+#ifdef CONFIG_ACPI
+	*value = acpi_value ? (u8)acpi_value->integer.value : cfg->def_value;
+#endif
+	return 0;
+}
+
+static int sx932x_load_config(struct device *dev, struct sx932x_data *data)
+{
+	u8 val;
+	int i, ret;
+	const struct sx932x_reg_config *cfg;
+
+	for (i = 0; data->reg_config[i].reg_addr != 0; i++) {
+		cfg = &data->reg_config[i];
+		ret = sx932x_read_register_property(dev, cfg, &val);
+		if (ret < 0)
+			return ret;
+		ret = regmap_write(data->regmap, cfg->reg_addr, val);
+		if (ret < 0)
+			return ret;
+	}
+	return 0;
+}
+

drivers/iio/proximity/sx932x.c line 1211

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+/* Activate all channels and perform an initial compensation. */
+static int sx932x_init_compensation(struct iio_dev *indio_dev)
+{
+	struct sx932x_data *data = iio_priv(indio_dev);
+	int i, ret;
+	bool success = false;
+	unsigned int val;
+
+	for (i = 100; i >= 0; i--) {
+		usleep_range(10000, 20000);

msleep(10);

drivers/iio/proximity/sx932x.c line 1216

Quoted 5 lines
drivers/iio/proximity/sx932x.c new file +5 -0
+		ret = regmap_read(data->regmap, SX932X_REG_STAT2, &val);
+		if (ret < 0)
+			goto out;
+		if (!(val & SX932X_COMPSTAT_MASK)) {
+			success = true;

return 0; instead.

drivers/iio/proximity/sx932x.c line 1223

Quoted 7 lines
drivers/iio/proximity/sx932x.c new file +7 -0
+			break;
+		}
+	}
+
+	if (success) {
+		dev_info(&data->client->dev,
+			 "initial compensation success");

No need to report the good path. Which also menas you can just return directly where you currently set success.

drivers/iio/proximity/sx932x.c line 1227

Quoted 4 lines
drivers/iio/proximity/sx932x.c new file +4 -0
+	} else {
+		dev_err(&data->client->dev,
+			"initial compensation timed out: 0x%02x", val);
+		ret = -ETIMEDOUT;

return -ETIMEDOUT;

Quoted 6 lines
drivers/iio/proximity/sx932x.c new file +6 -0
+	}
+
+out:
+	return ret;
+}
+

drivers/iio/proximity/sx932x.c line 1243

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+static int sx932x_init_device(struct iio_dev *indio_dev)
+{
+	struct device *dev = &indio_dev->dev;
+	struct sx932x_data *data = iio_priv(indio_dev);
+	int ret;
+	unsigned int val;
+
+	ret = regmap_read(data->regmap, SX932X_REG_WHOAMI, &val);
+	if (ret < 0 || (val != SX9320_WHOAMI && val != SX9321_WHOAMI &&
+			val != SX9324_WHOAMI)) {

If ret is less than 0, return that error it may be more meaningful. Then check the value separately.

Quoted 5 lines
drivers/iio/proximity/sx932x.c new file +5 -0
+		dev_err(&data->client->dev,
+			"Unable to identify the chip: %d - 0x%02x", ret, val);
+		return -ENODEV;
+	}
+

drivers/iio/proximity/sx932x.c line 1258

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+	ret = regmap_write(data->regmap, SX932X_REG_IRQ_MSK, 0);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_write(data->regmap, SX932X_REG_RESET,
+			   SX932X_SOFT_RESET);
+	if (ret < 0)
+		return ret;
+
+	// wait for the reset to be completed

/* */

drivers/iio/proximity/sx932x.c line 1259

Quoted 1 line
drivers/iio/proximity/sx932x.c new file +1 -0
+	usleep_range(10000, 20000);

msleep given the large values would be fine I think.

drivers/iio/proximity/sx932x.c line 1267

Quoted 8 lines
drivers/iio/proximity/sx932x.c new file +8 -0
+
+	ret = regmap_write(data->regmap, SX932X_REG_RESET, 0);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_read(data->regmap, SX932X_REG_IRQ_SRC, &val);
+	if (ret < 0)
+		return ret;

If this is a read to clear register, please add a comment saying so. Right now it just looks odd to read a value and do nothing with it.

Quoted 29 lines
drivers/iio/proximity/sx932x.c new file +29 -0
+
+	ret = sx932x_load_config(dev, data);
+	if (ret < 0)
+		return ret;
+
+	return sx932x_init_compensation(indio_dev);
+}
+
+static int sx932x_probe(struct i2c_client *client,
+			const struct i2c_device_id *i2c_id)
+{
+	int ret;
+	struct iio_dev *indio_dev;
+	const struct acpi_device_id *acpi_id;
+	struct sx932x_data *data;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (indio_dev == NULL)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	data->client = client;
+	if (i2c_id) {
+		data->reg_config = sx932x_reg_configs[i2c_id->driver_data];
+	} else if (ACPI_HANDLE(&client->dev)) {
+		acpi_id = acpi_match_device(
+				client->dev.driver->acpi_match_table,
+				&client->dev);
+		if (!acpi_id) {

drivers/iio/proximity/sx932x.c line 1306

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+			dev_err(&client->dev, "No driver data\n");
+			return -EINVAL;
+		}
+		data->reg_config = sx932x_reg_configs[acpi_id->driver_data];
+	} else {
+		return -ENODEV;
+	}
+	mutex_init(&data->mutex);
+	init_completion(&data->completion);
+	data->trigger_enabled = false;

That will actually be the default and is what people would expect (data is kzalloc'd).

drivers/iio/proximity/sx932x.c line 1310

Quoted 4 lines
drivers/iio/proximity/sx932x.c new file +4 -0
+
+	data->regmap = devm_regmap_init_i2c(client, &sx932x_regmap_config);
+	if (IS_ERR(data->regmap))
+		return PTR_ERR(data->regmap);

blank line here would help readability slightly.

drivers/iio/proximity/sx932x.c line 1313

Quoted 3 lines
drivers/iio/proximity/sx932x.c new file +3 -0
+	ACPI_COMPANION_SET(&indio_dev->dev, ACPI_COMPANION(&client->dev));
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->name = SX932X_DRIVER_NAME;

Please use the actual part name, not the driver name.

Quoted 5 lines
drivers/iio/proximity/sx932x.c new file +5 -0
+	indio_dev->channels = sx932x_channels;
+	indio_dev->num_channels = ARRAY_SIZE(sx932x_channels);
+	indio_dev->info = &sx932x_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	i2c_set_clientdata(client, indio_dev);

drivers/iio/proximity/sx932x.c line 1328

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+
+	ret = sx932x_init_device(indio_dev);
+	if (ret < 0) {
+		dev_err(&client->dev, "tried to init sx932x - error %d\n",
+			ret);
+		return ret;
+	}
+
+	if (client->irq <= 0)
+		dev_warn(&client->dev, "no valid irq found\n");

If it's not been provided, that was probably deliberate and the driver works fine like that (if not this should be an error) so drop this print.

Quoted 8 lines
drivers/iio/proximity/sx932x.c new file +8 -0
+	else {
+		ret = devm_request_threaded_irq(&client->dev, client->irq,
+				sx932x_irq_handler, sx932x_irq_thread_handler,
+				IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+				SX932X_IRQ_NAME, indio_dev);
+		if (ret < 0)
+			return ret;
+

drivers/iio/proximity/sx932x.c line 1346

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+		data->trig = devm_iio_trigger_alloc(&client->dev,
+				"%s-dev%d", indio_dev->name, indio_dev->id);
+		if (!data->trig)
+			return -ENOMEM;
+
+		data->trig->dev.parent = &client->dev;
+		data->trig->ops = &sx932x_trigger_ops;
+		iio_trigger_set_drvdata(data->trig, indio_dev);
+
+		ret = iio_trigger_register(data->trig);

devm_iio_trigger_register

Quoted 21 lines
drivers/iio/proximity/sx932x.c new file +21 -0
+		if (ret)
+			return ret;
+	}
+
+	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev,
+					      NULL, sx932x_trigger_handler,
+					      &sx932x_buffer_setup_ops);
+	if (ret < 0)
+		goto out_trigger_unregister;
+
+	ret = devm_iio_device_register(&client->dev, indio_dev);
+	if (ret < 0)
+		goto out_trigger_unregister;
+
+	return 0;
+
+out_trigger_unregister:
+	if (client->irq > 0)
+		iio_trigger_unregister(data->trig);
+
+	return ret;

drivers/iio/proximity/sx932x.c line 1377

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+}
+
+static int sx932x_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct sx932x_data *data = iio_priv(indio_dev);
+
+	if (client->irq > 0)
+		iio_trigger_unregister(data->trig);
+	kfree(data->buffer);

This feels like something that could have been done with managed interfaces. This is also racy. You can't manually unwind a few random items by hand and use the devm_ interfaces for the rest as the unwind order is not obviously correct (should be reverse of the set up order).

Quoted 7 lines
drivers/iio/proximity/sx932x.c new file +7 -0
+	return 0;
+}
+
+static int __maybe_unused sx932x_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct sx932x_data *data = iio_priv(indio_dev);

drivers/iio/proximity/sx932x.c line 1394

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+	int ret;
+
+	mutex_lock(&data->mutex);
+	ret = regmap_read(data->regmap, SX932X_REG_GNRL_CTRL1,
+			  &data->suspend_reg_gnrl_ctrl1);
+
+	if (ret < 0)
+		goto out;
+
+	// disable all phases, send the device to sleep

/* style comments */

Quoted 24 lines
drivers/iio/proximity/sx932x.c new file +24 -0
+	ret = regmap_write(data->regmap, SX932X_REG_GNRL_CTRL1, 0);
+
+out:
+	mutex_unlock(&data->mutex);
+	return ret;
+}
+
+static int __maybe_unused sx932x_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct sx932x_data *data = iio_priv(indio_dev);
+	int ret;
+
+	mutex_lock(&data->mutex);
+	ret = regmap_write(data->regmap, SX932X_REG_GNRL_CTRL1,
+			   data->suspend_reg_gnrl_ctrl1);
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static const struct dev_pm_ops sx932x_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(sx932x_suspend, sx932x_resume)
+};

drivers/iio/proximity/sx932x.c line 1428

Quoted 10 lines
drivers/iio/proximity/sx932x.c new file +10 -0
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id sx932x_acpi_match[] = {
+	{ SX9320_ACPI_NAME, 0 },
+	{ SX9321_ACPI_NAME, 1 },
+	{ SX9324_ACPI_NAME, 2 },
+	{},
+};
+MODULE_DEVICE_TABLE(acpi, sx932x_acpi_match);
+#endif
+#ifdef CONFIG_OF

Don't protect of bindings like this. They can be used via the obscure ACPI route that uses the DT bindings. Just let them always be built.

Quoted 34 lines
drivers/iio/proximity/sx932x.c new file +34 -0
+static const struct of_device_id sx932x_of_match[] = {
+	{ .compatible = "semtech,sx9320" },
+	{ .compatible = "semtech,sx9321" },
+	{ .compatible = "semtech,sx9324" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, sx932x_of_match);
+#endif
+static const struct i2c_device_id sx932x_id[] = {
+	{ "sx9320", 0 },
+	{ "sx9321", 1 },
+	{ "sx9324", 2 },
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, sx932x_id);
+
+static struct i2c_driver sx932x_driver = {
+	.driver = {
+		.name	= SX932X_DRIVER_NAME,
+		.acpi_match_table = ACPI_PTR(sx932x_acpi_match),
+		.of_match_table = of_match_ptr(sx932x_of_match),
+		.pm = &sx932x_pm_ops,
+	},
+	.probe		= sx932x_probe,
+	.remove		= sx932x_remove,
+	.id_table	= sx932x_id,
+};
+module_i2c_driver(sx932x_driver);
+
+MODULE_AUTHOR("Enrico Granata <[email protected]>");
+MODULE_AUTHOR("Gwendal Grignou <[email protected]>");
+MODULE_AUTHOR("Pat Plunkett <[email protected]");
+MODULE_DESCRIPTION("Driver for Semtech SX9320/SX9321/SX9324 proximity sensor");
+MODULE_LICENSE("GPL v2");