[PATCH v3 00/15] sx9310 iio driver updates

[PATCH v3 00/15] sx9310 iio driver updates

[Daniel Campello]

The first patch resends the DT binding for the driver that was merged in
v5.8-rc1 with a small change to update for proper regulators. The second
through the eleventh patch fixes several issues dropped from v8 to v9
when the initial patch was merged.  The twelveth patch drops
channel_users because it's unused. The thirteenth patch adds support to
enable the svdd and vdd supplies so that this driver can work on a board
where the svdd supply isn't enabled at boot and needs to be turned on
before this driver starts to communicate with the chip. The final patch
updates irq trigger flags.

Changes in v3:
 - Moved irq presence check down to lower methods
 - Changed buffer to struct type to align timestamp memory properly.
 - Added static assert for number of channels.
 - Added irq trigger flags commit to the series.

Changes in v2:
 - Added #io-channel-cells as a required property
 - Reordered error handling on sx9310_resume()
 - Added #include <linux/mod_devicetable.h>
 - Added '\n' to dev_err()
 - Fixed commit message from "iio: sx9310: Align memory"
 - Changed prox_stat to chan_prox_stat bitmap.
 - Fixed dev_err() message
 - Added '\n' to dev_err()

Daniel Campello (12):
  dt-bindings: iio: Add bindings for sx9310 sensor
  iio: sx9310: Update macros declarations
  iio: sx9310: Fix irq handling
  iio: sx9310: Remove acpi and of table macros
  iio: sx9310: Change from .probe to .probe_new
  iio: sx9310: Fixes various memory handling
  iio: sx9310: Use long instead of int for channel bitmaps
  iio: sx9310: Use regmap_read_poll_timeout() for compensation
  iio: sx9310: Update copyright
  iio: sx9310: Simplify error return handling
  iio: sx9310: Use variable to hold &client->dev
  iio: sx9310: Miscellaneous format fixes

Stephen Boyd (3):
  iio: sx9310: Drop channel_users[]
  iio: sx9310: Enable vdd and svdd regulators at probe
  iio: sx9310: Use irq trigger flags from firmware

 .../iio/proximity/semtech,sx9310.yaml         |  65 +++
 drivers/iio/proximity/sx9310.c                | 429 +++++++++---------
 2 files changed, 282 insertions(+), 212 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml

--
2.28.0.163.g6104cc2f0b6-goog

[PATCH v3 01/15] dt-bindings: iio: Add bindings for sx9310 sensor

[Daniel Campello]

Adds device tree bandings for sx9310 sensor.

Signed-off-by: Daniel Campello <campello@chromium.org>
Cc: Hartmut Knaack <knaack.h@gmx.de>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
Cc: Rob Herring <robh+dt@kernel.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
[swboyd@chromium.org: Add both regulators and make them optional]
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---

Changes in v3: None
Changes in v2:
 - Added #io-channel-cells as a required property

 .../iio/proximity/semtech,sx9310.yaml         | 65 +++++++++++++++++++
 1 file changed, 65 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml

diff --git a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
new file mode 100644
index 00000000000000..5739074d3592fe
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
@@ -0,0 +1,65 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/proximity/semtech,sx9310.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Semtech's SX9310 capacitive proximity sensor
+
+maintainers:
+  - Daniel Campello <campello@chromium.org>
+
+description: |
+  Semtech's SX9310/SX9311 capacitive proximity/button solution.
+
+  Specifications about the devices can be found at:
+  https://www.semtech.com/products/smart-sensing/sar-sensors/sx9310
+
+properties:
+  compatible:
+    enum:
+      - semtech,sx9310
+      - semtech,sx9311
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    description:
+      The sole interrupt generated by the device used to announce the
+      preceding reading request has finished and that data is
+      available or that a close/far proximity event has happened.
+    maxItems: 1
+
+  vdd-supply:
+    description: Main power supply
+
+  svdd-supply:
+    description: Host interface power supply
+
+  "#io-channel-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - "#io-channel-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      proximity@28 {
+        compatible = "semtech,sx9310";
+        reg = <0x28>;
+        interrupt-parent = <&pio>;
+        interrupts = <5 IRQ_TYPE_LEVEL_LOW 5>;
+        vdd-supply = <&pp3300_a>;
+        svdd-supply = <&pp1800_prox>;
+        #io-channel-cells = <1>;
+      };
+    };
-- 
2.28.0.163.g6104cc2f0b6-goog

Re: [PATCH v3 01/15] dt-bindings: iio: Add bindings for sx9310 sensor

[Rob Herring]

On Fri, 31 Jul 2020 10:48:38 -0600, Daniel Campello wrote:
> Adds device tree bandings for sx9310 sensor.
> 
> Signed-off-by: Daniel Campello <campello@chromium.org>
> Cc: Hartmut Knaack <knaack.h@gmx.de>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
> Cc: Rob Herring <robh+dt@kernel.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> [swboyd@chromium.org: Add both regulators and make them optional]
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
> 
> Changes in v3: None
> Changes in v2:
>  - Added #io-channel-cells as a required property
> 
>  .../iio/proximity/semtech,sx9310.yaml         | 65 +++++++++++++++++++
>  1 file changed, 65 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 01/15] dt-bindings: iio: Add bindings for sx9310 sensor

[Jonathan Cameron]

On Fri, 31 Jul 2020 10:48:38 -0600
Daniel Campello <campello@chromium.org> wrote:

> Adds device tree bandings for sx9310 sensor.
> 
> Signed-off-by: Daniel Campello <campello@chromium.org>
> Cc: Hartmut Knaack <knaack.h@gmx.de>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
> Cc: Rob Herring <robh+dt@kernel.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> [swboyd@chromium.org: Add both regulators and make them optional]
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
> 
> Changes in v3: None
> Changes in v2:
>  - Added #io-channel-cells as a required property
> 
>  .../iio/proximity/semtech,sx9310.yaml         | 65 +++++++++++++++++++
>  1 file changed, 65 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> new file mode 100644
> index 00000000000000..5739074d3592fe
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> @@ -0,0 +1,65 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/proximity/semtech,sx9310.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Semtech's SX9310 capacitive proximity sensor
> +
> +maintainers:
> +  - Daniel Campello <campello@chromium.org>
> +
> +description: |
> +  Semtech's SX9310/SX9311 capacitive proximity/button solution.
> +
> +  Specifications about the devices can be found at:
> +  https://www.semtech.com/products/smart-sensing/sar-sensors/sx9310
> +
> +properties:
> +  compatible:
> +    enum:
> +      - semtech,sx9310
> +      - semtech,sx9311
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    description:
> +      The sole interrupt generated by the device used to announce the
> +      preceding reading request has finished and that data is
> +      available or that a close/far proximity event has happened.
> +    maxItems: 1
> +
> +  vdd-supply:
> +    description: Main power supply
> +
> +  svdd-supply:
> +    description: Host interface power supply
> +
> +  "#io-channel-cells":
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#io-channel-cells"

Missed this in earlier review (only noticed when I saw whilst santity
checking earlier versions.

Fairly sure we should only need #io-channel-cells if we have
a consumer of a channel somewhere else in DT.  So it's not
required as far as I can see.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +      proximity@28 {
> +        compatible = "semtech,sx9310";
> +        reg = <0x28>;
> +        interrupt-parent = <&pio>;
> +        interrupts = <5 IRQ_TYPE_LEVEL_LOW 5>;
> +        vdd-supply = <&pp3300_a>;
> +        svdd-supply = <&pp1800_prox>;
> +        #io-channel-cells = <1>;
> +      };
> +    };

Re: [PATCH v3 01/15] dt-bindings: iio: Add bindings for sx9310 sensor

[Stephen Boyd]

Quoting Jonathan Cameron (2020-08-01 08:06:39)
> On Fri, 31 Jul 2020 10:48:38 -0600
> Daniel Campello <campello@chromium.org> wrote:
> > diff --git a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> > new file mode 100644
> > index 00000000000000..5739074d3592fe
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> > @@ -0,0 +1,65 @@
[...]
> > +
> > +  "#io-channel-cells":
> > +    const: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - "#io-channel-cells"
> 
> Missed this in earlier review (only noticed when I saw whilst santity
> checking earlier versions.
> 
> Fairly sure we should only need #io-channel-cells if we have
> a consumer of a channel somewhere else in DT.  So it's not
> required as far as I can see.
> 

This is mostly a decision for Rob to make, but I would make it required
because the device is always an io channel provider. It may be that it
isn't providing anything in the DT to something else in the DT but it is
providing this information somewhere so always having to spell that out
is simple and doesn't hurt.

Re: [PATCH v3 01/15] dt-bindings: iio: Add bindings for sx9310 sensor

[Rob Herring]

On Mon, Aug 3, 2020 at 1:00 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Jonathan Cameron (2020-08-01 08:06:39)
> > On Fri, 31 Jul 2020 10:48:38 -0600
> > Daniel Campello <campello@chromium.org> wrote:
> > > diff --git a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> > > new file mode 100644
> > > index 00000000000000..5739074d3592fe
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> > > @@ -0,0 +1,65 @@
> [...]
> > > +
> > > +  "#io-channel-cells":
> > > +    const: 1
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - "#io-channel-cells"
> >
> > Missed this in earlier review (only noticed when I saw whilst santity
> > checking earlier versions.
> >
> > Fairly sure we should only need #io-channel-cells if we have
> > a consumer of a channel somewhere else in DT.  So it's not
> > required as far as I can see.
> >
>
> This is mostly a decision for Rob to make, but I would make it required
> because the device is always an io channel provider. It may be that it
> isn't providing anything in the DT to something else in the DT but it is
> providing this information somewhere so always having to spell that out
> is simple and doesn't hurt.

I agree. If the user is split in a board file or overlay, we don't
want to have to be adding it to the provider at that time.

Rob

Re: [PATCH v3 01/15] dt-bindings: iio: Add bindings for sx9310 sensor

[Jonathan Cameron]

On Mon, 3 Aug 2020 20:01:06 -0600
Rob Herring <robh+dt@kernel.org> wrote:

> On Mon, Aug 3, 2020 at 1:00 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Quoting Jonathan Cameron (2020-08-01 08:06:39)  
> > > On Fri, 31 Jul 2020 10:48:38 -0600
> > > Daniel Campello <campello@chromium.org> wrote:  
> > > > diff --git a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> > > > new file mode 100644
> > > > index 00000000000000..5739074d3592fe
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> > > > @@ -0,0 +1,65 @@  
> > [...]  
> > > > +
> > > > +  "#io-channel-cells":
> > > > +    const: 1
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +  - "#io-channel-cells"  
> > >
> > > Missed this in earlier review (only noticed when I saw whilst santity
> > > checking earlier versions.
> > >
> > > Fairly sure we should only need #io-channel-cells if we have
> > > a consumer of a channel somewhere else in DT.  So it's not
> > > required as far as I can see.
> > >  
> >
> > This is mostly a decision for Rob to make, but I would make it required
> > because the device is always an io channel provider. It may be that it
> > isn't providing anything in the DT to something else in the DT but it is
> > providing this information somewhere so always having to spell that out
> > is simple and doesn't hurt.  
> 
> I agree. If the user is split in a board file or overlay, we don't
> want to have to be adding it to the provider at that time.

That is perhaps a reasonable view point for devices with channels that
are likely to be used by consumer drivers, but in this particular case we
are talking about a proximity sensor.  So far I don't think we
have any consumer drivers for this type of sensor (I might have forgotten
one of course!)

I'm not that fussed though, so will leave it in.

The argument is a lot stronger for ADCs and such like, so we can
start encouraging it for those.

Jonathan


> 
> Rob

Re: [PATCH v3 01/15] dt-bindings: iio: Add bindings for sx9310 sensor

[Rob Herring]

On Thu, Aug 6, 2020 at 12:14 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 3 Aug 2020 20:01:06 -0600
> Rob Herring <robh+dt@kernel.org> wrote:
>
> > On Mon, Aug 3, 2020 at 1:00 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > Quoting Jonathan Cameron (2020-08-01 08:06:39)
> > > > On Fri, 31 Jul 2020 10:48:38 -0600
> > > > Daniel Campello <campello@chromium.org> wrote:
> > > > > diff --git a/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> > > > > new file mode 100644
> > > > > index 00000000000000..5739074d3592fe
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/iio/proximity/semtech,sx9310.yaml
> > > > > @@ -0,0 +1,65 @@
> > > [...]
> > > > > +
> > > > > +  "#io-channel-cells":
> > > > > +    const: 1
> > > > > +
> > > > > +required:
> > > > > +  - compatible
> > > > > +  - reg
> > > > > +  - "#io-channel-cells"
> > > >
> > > > Missed this in earlier review (only noticed when I saw whilst santity
> > > > checking earlier versions.
> > > >
> > > > Fairly sure we should only need #io-channel-cells if we have
> > > > a consumer of a channel somewhere else in DT.  So it's not
> > > > required as far as I can see.
> > > >
> > >
> > > This is mostly a decision for Rob to make, but I would make it required
> > > because the device is always an io channel provider. It may be that it
> > > isn't providing anything in the DT to something else in the DT but it is
> > > providing this information somewhere so always having to spell that out
> > > is simple and doesn't hurt.
> >
> > I agree. If the user is split in a board file or overlay, we don't
> > want to have to be adding it to the provider at that time.
>
> That is perhaps a reasonable view point for devices with channels that
> are likely to be used by consumer drivers, but in this particular case we
> are talking about a proximity sensor.  So far I don't think we
> have any consumer drivers for this type of sensor (I might have forgotten
> one of course!)

Indeed, I didn't consider whether it made sense in the first place. So
should it just not be specified at all in this case? I can't really
picture what the usecase for a consumer node would be.

Rob

Re: [PATCH v3 01/15] dt-bindings: iio: Add bindings for sx9310 sensor

[Stephen Boyd]

Quoting Rob Herring (2020-08-06 15:12:34)
> On Thu, Aug 6, 2020 at 12:14 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Mon, 3 Aug 2020 20:01:06 -0600
> > Rob Herring <robh+dt@kernel.org> wrote:
> >
> > > On Mon, Aug 3, 2020 at 1:00 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > >
> > > > This is mostly a decision for Rob to make, but I would make it required
> > > > because the device is always an io channel provider. It may be that it
> > > > isn't providing anything in the DT to something else in the DT but it is
> > > > providing this information somewhere so always having to spell that out
> > > > is simple and doesn't hurt.
> > >
> > > I agree. If the user is split in a board file or overlay, we don't
> > > want to have to be adding it to the provider at that time.
> >
> > That is perhaps a reasonable view point for devices with channels that
> > are likely to be used by consumer drivers, but in this particular case we
> > are talking about a proximity sensor.  So far I don't think we
> > have any consumer drivers for this type of sensor (I might have forgotten
> > one of course!)
> 
> Indeed, I didn't consider whether it made sense in the first place. So
> should it just not be specified at all in this case? I can't really
> picture what the usecase for a consumer node would be.
> 

I was thinking that a WiFi DT node may directly grab the channel from
this device and use this for SAR power changes. That would avoid going
all the way to userspace to figure out that something is close proximity
and then tell WiFi to reduce power.

[PATCH v3 02/15] iio: sx9310: Update macros declarations

[Daniel Campello]

Follows spec sheet for macro declarations.

Signed-off-by: Daniel Campello <campello@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---

Changes in v3: None
Changes in v2: None

 drivers/iio/proximity/sx9310.c | 143 +++++++++++++++------------------
 1 file changed, 67 insertions(+), 76 deletions(-)

diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index d161f3061e353d..07895d4b935d12 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -33,45 +33,45 @@
 #define SX9310_REG_IRQ_SRC				0x00
 #define SX9310_REG_STAT0				0x01
 #define SX9310_REG_STAT1				0x02
+#define SX9310_REG_STAT1_COMPSTAT_MASK			GENMASK(3, 0)
 #define SX9310_REG_IRQ_MSK				0x03
 #define   SX9310_CONVDONE_IRQ				BIT(3)
 #define   SX9310_FAR_IRQ				BIT(5)
 #define   SX9310_CLOSE_IRQ				BIT(6)
-#define   SX9310_EVENT_IRQ				(SX9310_FAR_IRQ | \
-							 SX9310_CLOSE_IRQ)
 #define SX9310_REG_IRQ_FUNC				0x04
 
 #define SX9310_REG_PROX_CTRL0				0x10
-#define   SX9310_REG_PROX_CTRL0_PROXSTAT2		0x10
-#define   SX9310_REG_PROX_CTRL0_EN_MASK			0x0F
+#define   SX9310_REG_PROX_CTRL0_SENSOREN_MASK		GENMASK(3, 0)
+#define   SX9310_REG_PROX_CTRL0_SCANPERIOD_MASK		GENMASK(7, 4)
+#define   SX9310_REG_PROX_CTRL0_SCANPERIOD_SHIFT	4
+#define   SX9310_REG_PROX_CTRL0_SCANPERIOD_15MS		0x01
 #define SX9310_REG_PROX_CTRL1				0x11
 #define SX9310_REG_PROX_CTRL2				0x12
-#define   SX9310_REG_PROX_CTRL2_COMBMODE_ALL		0x80
-#define   SX9310_REG_PROX_CTRL2_SHIELDEN_DYNAMIC	0x04
+#define   SX9310_REG_PROX_CTRL2_COMBMODE_CS1_CS2	(0x02 << 6)
+#define   SX9310_REG_PROX_CTRL2_SHIELDEN_DYNAMIC	(0x01 << 2)
 #define SX9310_REG_PROX_CTRL3				0x13
-#define   SX9310_REG_PROX_CTRL3_GAIN0_X8		0x0c
+#define   SX9310_REG_PROX_CTRL3_GAIN0_X8		(0x03 << 2)
 #define   SX9310_REG_PROX_CTRL3_GAIN12_X4		0x02
 #define SX9310_REG_PROX_CTRL4				0x14
 #define   SX9310_REG_PROX_CTRL4_RESOLUTION_FINEST	0x07
 #define SX9310_REG_PROX_CTRL5				0x15
-#define   SX9310_REG_PROX_CTRL5_RANGE_SMALL		0xc0
-#define   SX9310_REG_PROX_CTRL5_STARTUPSENS_CS1		0x04
+#define   SX9310_REG_PROX_CTRL5_RANGE_SMALL		(0x03 << 6)
+#define   SX9310_REG_PROX_CTRL5_STARTUPSENS_CS1		(0x01 << 2)
 #define   SX9310_REG_PROX_CTRL5_RAWFILT_1P25		0x02
 #define SX9310_REG_PROX_CTRL6				0x16
-#define   SX9310_REG_PROX_CTRL6_COMP_COMMON		0x20
+#define   SX9310_REG_PROX_CTRL6_AVGTHRESH_DEFAULT	0x20
 #define SX9310_REG_PROX_CTRL7				0x17
-#define   SX9310_REG_PROX_CTRL7_AVGNEGFILT_2		0x08
+#define   SX9310_REG_PROX_CTRL7_AVGNEGFILT_2		(0x01 << 3)
 #define   SX9310_REG_PROX_CTRL7_AVGPOSFILT_512		0x05
 #define SX9310_REG_PROX_CTRL8				0x18
 #define SX9310_REG_PROX_CTRL9				0x19
-#define   SX9310_REG_PROX_CTRL8_9_PTHRESH12_28		0x40
-#define   SX9310_REG_PROX_CTRL8_9_PTHRESH_96		0x88
+#define   SX9310_REG_PROX_CTRL8_9_PTHRESH_28		(0x08 << 3)
+#define   SX9310_REG_PROX_CTRL8_9_PTHRESH_96		(0x11 << 3)
 #define   SX9310_REG_PROX_CTRL8_9_BODYTHRESH_900	0x03
 #define   SX9310_REG_PROX_CTRL8_9_BODYTHRESH_1500	0x05
 #define SX9310_REG_PROX_CTRL10				0x1a
-#define   SX9310_REG_PROX_CTRL10_HYST_6PCT		0x10
-#define   SX9310_REG_PROX_CTRL10_CLOSE_DEBOUNCE_8	0x12
-#define   SX9310_REG_PROX_CTRL10_FAR_DEBOUNCE_8		0x03
+#define   SX9310_REG_PROX_CTRL10_HYST_6PCT		(0x01 << 4)
+#define   SX9310_REG_PROX_CTRL10_FAR_DEBOUNCE_2		0x01
 #define SX9310_REG_PROX_CTRL11				0x1b
 #define SX9310_REG_PROX_CTRL12				0x1c
 #define SX9310_REG_PROX_CTRL13				0x1d
@@ -82,8 +82,8 @@
 #define SX9310_REG_PROX_CTRL18				0x22
 #define SX9310_REG_PROX_CTRL19				0x23
 #define SX9310_REG_SAR_CTRL0				0x2a
-#define   SX9310_REG_SAR_CTRL0_SARDEB_4_SAMPLES		0x40
-#define   SX9310_REG_SAR_CTRL0_SARHYST_8		0x10
+#define   SX9310_REG_SAR_CTRL0_SARDEB_4_SAMPLES		(0x02 << 5)
+#define   SX9310_REG_SAR_CTRL0_SARHYST_8		(0x02 << 3)
 #define SX9310_REG_SAR_CTRL1				0x2b
 /* Each increment of the slope register is 0.0078125. */
 #define   SX9310_REG_SAR_CTRL1_SLOPE(_hnslope)		(_hnslope / 78125)
@@ -107,7 +107,7 @@
 #define SX9310_REG_SAR_MSB				0x39
 #define SX9310_REG_SAR_LSB				0x3a
 
-#define SX9310_REG_I2CADDR				0x40
+#define SX9310_REG_I2C_ADDR				0x40
 #define SX9310_REG_PAUSE				0x41
 #define SX9310_REG_WHOAMI				0x42
 #define   SX9310_WHOAMI_VALUE				0x01
@@ -116,14 +116,9 @@
 #define SX9310_REG_RESET				0x7f
 #define   SX9310_SOFT_RESET				0xde
 
-#define SX9310_SCAN_PERIOD_MASK				GENMASK(7, 4)
-#define SX9310_SCAN_PERIOD_SHIFT			4
-
-#define SX9310_COMPSTAT_MASK				GENMASK(3, 0)
 
 /* 4 hardware channels, as defined in STAT0: COMB, CS2, CS1 and CS0. */
 #define SX9310_NUM_CHANNELS				4
-#define SX9310_CHAN_ENABLED_MASK			GENMASK(3, 0)
 
 struct sx9310_data {
 	/* Serialize access to registers and channel configuration */
@@ -251,7 +246,7 @@ static const struct regmap_range sx9310_readable_reg_ranges[] = {
 	regmap_reg_range(SX9310_REG_PROX_CTRL0, SX9310_REG_PROX_CTRL19),
 	regmap_reg_range(SX9310_REG_SAR_CTRL0, SX9310_REG_SAR_CTRL2),
 	regmap_reg_range(SX9310_REG_SENSOR_SEL, SX9310_REG_SAR_LSB),
-	regmap_reg_range(SX9310_REG_I2CADDR, SX9310_REG_WHOAMI),
+	regmap_reg_range(SX9310_REG_I2C_ADDR, SX9310_REG_WHOAMI),
 	regmap_reg_range(SX9310_REG_RESET, SX9310_REG_RESET),
 };
 
@@ -292,7 +287,7 @@ static int sx9310_update_chan_en(struct sx9310_data *data,
 
 	if ((data->chan_read | data->chan_event) != (chan_read | chan_event)) {
 		ret = regmap_update_bits(data->regmap, SX9310_REG_PROX_CTRL0,
-					 SX9310_CHAN_ENABLED_MASK,
+					 SX9310_REG_PROX_CTRL0_SENSOREN_MASK,
 					 chan_read | chan_event);
 		if (ret)
 			return ret;
@@ -361,7 +356,8 @@ static int sx9310_wait_for_sample(struct sx9310_data *data)
 	if (ret < 0)
 		return ret;
 
-	val = (val & SX9310_SCAN_PERIOD_MASK) >> SX9310_SCAN_PERIOD_SHIFT;
+	val = (val & SX9310_REG_PROX_CTRL0_SCANPERIOD_MASK) >>
+	      SX9310_REG_PROX_CTRL0_SCANPERIOD_SHIFT;
 
 	msleep(sx9310_scan_period_table[val]);
 
@@ -435,7 +431,8 @@ static int sx9310_read_samp_freq(struct sx9310_data *data, int *val, int *val2)
 	if (ret < 0)
 		return ret;
 
-	regval = (regval & SX9310_SCAN_PERIOD_MASK) >> SX9310_SCAN_PERIOD_SHIFT;
+	regval = (regval & SX9310_REG_PROX_CTRL0_SCANPERIOD_MASK) >>
+		 SX9310_REG_PROX_CTRL0_SCANPERIOD_SHIFT;
 	*val = sx9310_samp_freq_table[regval].val;
 	*val2 = sx9310_samp_freq_table[regval].val2;
 
@@ -483,8 +480,8 @@ static int sx9310_set_samp_freq(struct sx9310_data *data, int val, int val2)
 	mutex_lock(&data->mutex);
 
 	ret = regmap_update_bits(data->regmap, SX9310_REG_PROX_CTRL0,
-				 SX9310_SCAN_PERIOD_MASK,
-				 i << SX9310_SCAN_PERIOD_SHIFT);
+				 SX9310_REG_PROX_CTRL0_SCANPERIOD_MASK,
+				 i << SX9310_REG_PROX_CTRL0_SCANPERIOD_SHIFT);
 
 	mutex_unlock(&data->mutex);
 
@@ -572,7 +569,7 @@ static irqreturn_t sx9310_irq_thread_handler(int irq, void *private)
 		goto out;
 	}
 
-	if (val & SX9310_EVENT_IRQ)
+	if (val & (SX9310_FAR_IRQ | SX9310_CLOSE_IRQ))
 		sx9310_push_events(indio_dev);
 
 	if (val & SX9310_CONVDONE_IRQ)
@@ -600,6 +597,7 @@ static int sx9310_write_event_config(struct iio_dev *indio_dev,
 				     enum iio_event_direction dir, int state)
 {
 	struct sx9310_data *data = iio_priv(indio_dev);
+	unsigned int eventirq = SX9310_FAR_IRQ | SX9310_CLOSE_IRQ;
 	int ret;
 
 	/* If the state hasn't changed, there's nothing to do. */
@@ -612,7 +610,7 @@ static int sx9310_write_event_config(struct iio_dev *indio_dev,
 		if (ret < 0)
 			goto out_unlock;
 		if (!(data->chan_event & ~BIT(chan->channel))) {
-			ret = sx9310_enable_irq(data, SX9310_EVENT_IRQ);
+			ret = sx9310_enable_irq(data, eventirq);
 			if (ret < 0)
 				sx9310_put_event_channel(data, chan->channel);
 		}
@@ -621,7 +619,7 @@ static int sx9310_write_event_config(struct iio_dev *indio_dev,
 		if (ret < 0)
 			goto out_unlock;
 		if (!data->chan_event) {
-			ret = sx9310_disable_irq(data, SX9310_EVENT_IRQ);
+			ret = sx9310_disable_irq(data, eventirq);
 			if (ret < 0)
 				sx9310_get_event_channel(data, chan->channel);
 		}
@@ -746,53 +744,46 @@ struct sx9310_reg_default {
 	u8 def;
 };
 
-#define SX_INIT(_reg, _def)			\
-	{					\
-		.reg = SX9310_REG_##_reg,	\
-		.def = _def,			\
-	}
-
 static const struct sx9310_reg_default sx9310_default_regs[] = {
-	SX_INIT(IRQ_MSK, 0x00),
-	SX_INIT(IRQ_FUNC, 0x00),
+	{ SX9310_REG_IRQ_MSK, 0x00 },
+	{ SX9310_REG_IRQ_FUNC, 0x00 },
 	/*
 	 * The lower 4 bits should not be set as it enable sensors measurements.
 	 * Turning the detection on before the configuration values are set to
 	 * good values can cause the device to return erroneous readings.
 	 */
-	SX_INIT(PROX_CTRL0, SX9310_REG_PROX_CTRL0_PROXSTAT2),
-	SX_INIT(PROX_CTRL1, 0x00),
-	SX_INIT(PROX_CTRL2, SX9310_REG_PROX_CTRL2_COMBMODE_ALL |
-			    SX9310_REG_PROX_CTRL2_SHIELDEN_DYNAMIC),
-	SX_INIT(PROX_CTRL3, SX9310_REG_PROX_CTRL3_GAIN0_X8 |
-			    SX9310_REG_PROX_CTRL3_GAIN12_X4),
-	SX_INIT(PROX_CTRL4, SX9310_REG_PROX_CTRL4_RESOLUTION_FINEST),
-	SX_INIT(PROX_CTRL5, SX9310_REG_PROX_CTRL5_RANGE_SMALL |
-			    SX9310_REG_PROX_CTRL5_STARTUPSENS_CS1 |
-			    SX9310_REG_PROX_CTRL5_RAWFILT_1P25),
-	SX_INIT(PROX_CTRL6, SX9310_REG_PROX_CTRL6_COMP_COMMON),
-	SX_INIT(PROX_CTRL7, SX9310_REG_PROX_CTRL7_AVGNEGFILT_2 |
-			    SX9310_REG_PROX_CTRL7_AVGPOSFILT_512),
-	SX_INIT(PROX_CTRL8, SX9310_REG_PROX_CTRL8_9_PTHRESH_96 |
-			    SX9310_REG_PROX_CTRL8_9_BODYTHRESH_1500),
-	SX_INIT(PROX_CTRL9, SX9310_REG_PROX_CTRL8_9_PTHRESH12_28 |
-			    SX9310_REG_PROX_CTRL8_9_BODYTHRESH_900),
-	SX_INIT(PROX_CTRL10, SX9310_REG_PROX_CTRL10_HYST_6PCT |
-			     SX9310_REG_PROX_CTRL10_CLOSE_DEBOUNCE_8 |
-			     SX9310_REG_PROX_CTRL10_FAR_DEBOUNCE_8),
-	SX_INIT(PROX_CTRL11, 0x00),
-	SX_INIT(PROX_CTRL12, 0x00),
-	SX_INIT(PROX_CTRL13, 0x00),
-	SX_INIT(PROX_CTRL14, 0x00),
-	SX_INIT(PROX_CTRL15, 0x00),
-	SX_INIT(PROX_CTRL16, 0x00),
-	SX_INIT(PROX_CTRL17, 0x00),
-	SX_INIT(PROX_CTRL18, 0x00),
-	SX_INIT(PROX_CTRL19, 0x00),
-	SX_INIT(SAR_CTRL0, SX9310_REG_SAR_CTRL0_SARDEB_4_SAMPLES |
-			   SX9310_REG_SAR_CTRL0_SARHYST_8),
-	SX_INIT(SAR_CTRL1, SX9310_REG_SAR_CTRL1_SLOPE(10781250)),
-	SX_INIT(SAR_CTRL2, SX9310_REG_SAR_CTRL2_SAROFFSET_DEFAULT),
+	{ SX9310_REG_PROX_CTRL0, SX9310_REG_PROX_CTRL0_SCANPERIOD_15MS },
+	{ SX9310_REG_PROX_CTRL1, 0x00 },
+	{ SX9310_REG_PROX_CTRL2, SX9310_REG_PROX_CTRL2_COMBMODE_CS1_CS2 |
+				 SX9310_REG_PROX_CTRL2_SHIELDEN_DYNAMIC },
+	{ SX9310_REG_PROX_CTRL3, SX9310_REG_PROX_CTRL3_GAIN0_X8 |
+				 SX9310_REG_PROX_CTRL3_GAIN12_X4 },
+	{ SX9310_REG_PROX_CTRL4, SX9310_REG_PROX_CTRL4_RESOLUTION_FINEST },
+	{ SX9310_REG_PROX_CTRL5, SX9310_REG_PROX_CTRL5_RANGE_SMALL |
+				 SX9310_REG_PROX_CTRL5_STARTUPSENS_CS1 |
+				 SX9310_REG_PROX_CTRL5_RAWFILT_1P25 },
+	{ SX9310_REG_PROX_CTRL6, SX9310_REG_PROX_CTRL6_AVGTHRESH_DEFAULT },
+	{ SX9310_REG_PROX_CTRL7, SX9310_REG_PROX_CTRL7_AVGNEGFILT_2 |
+				 SX9310_REG_PROX_CTRL7_AVGPOSFILT_512 },
+	{ SX9310_REG_PROX_CTRL8, SX9310_REG_PROX_CTRL8_9_PTHRESH_96 |
+				 SX9310_REG_PROX_CTRL8_9_BODYTHRESH_1500 },
+	{ SX9310_REG_PROX_CTRL9, SX9310_REG_PROX_CTRL8_9_PTHRESH_28 |
+				 SX9310_REG_PROX_CTRL8_9_BODYTHRESH_900 },
+	{ SX9310_REG_PROX_CTRL10, SX9310_REG_PROX_CTRL10_HYST_6PCT |
+				  SX9310_REG_PROX_CTRL10_FAR_DEBOUNCE_2 },
+	{ SX9310_REG_PROX_CTRL11, 0x00 },
+	{ SX9310_REG_PROX_CTRL12, 0x00 },
+	{ SX9310_REG_PROX_CTRL13, 0x00 },
+	{ SX9310_REG_PROX_CTRL14, 0x00 },
+	{ SX9310_REG_PROX_CTRL15, 0x00 },
+	{ SX9310_REG_PROX_CTRL16, 0x00 },
+	{ SX9310_REG_PROX_CTRL17, 0x00 },
+	{ SX9310_REG_PROX_CTRL18, 0x00 },
+	{ SX9310_REG_PROX_CTRL19, 0x00 },
+	{ SX9310_REG_SAR_CTRL0, SX9310_REG_SAR_CTRL0_SARDEB_4_SAMPLES |
+				SX9310_REG_SAR_CTRL0_SARHYST_8 },
+	{ SX9310_REG_SAR_CTRL1, SX9310_REG_SAR_CTRL1_SLOPE(10781250) },
+	{ SX9310_REG_SAR_CTRL2, SX9310_REG_SAR_CTRL2_SAROFFSET_DEFAULT },
 };
 
 /* Activate all channels and perform an initial compensation. */
@@ -809,7 +800,7 @@ static int sx9310_init_compensation(struct iio_dev *indio_dev)
 
 	/* run the compensation phase on all channels */
 	ret = regmap_write(data->regmap, SX9310_REG_PROX_CTRL0,
-			   ctrl0 | SX9310_REG_PROX_CTRL0_EN_MASK);
+			   ctrl0 | SX9310_REG_PROX_CTRL0_SENSOREN_MASK);
 	if (ret < 0)
 		return ret;
 
@@ -992,7 +983,7 @@ static int __maybe_unused sx9310_suspend(struct device *dev)
 	if (ret)
 		goto out;
 
-	ctrl0 = data->suspend_ctrl0 & ~SX9310_REG_PROX_CTRL0_EN_MASK;
+	ctrl0 = data->suspend_ctrl0 & ~SX9310_REG_PROX_CTRL0_SENSOREN_MASK;
 	ret = regmap_write(data->regmap, SX9310_REG_PROX_CTRL0, ctrl0);
 	if (ret)
 		goto out;
-- 
2.28.0.163.g6104cc2f0b6-goog

Re: [PATCH v3 02/15] iio: sx9310: Update macros declarations

[Jonathan Cameron]

On Fri, 31 Jul 2020 10:48:39 -0600
Daniel Campello <campello@chromium.org> wrote:

> Follows spec sheet for macro declarations.
> 
> Signed-off-by: Daniel Campello <campello@chromium.org>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>

I'm fairly sure this patch breaks bisection as you've renamed
one macro without changing everywhere it's used.
The driver has to build even half way through the series.

Jonathan


> ---
> 
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/iio/proximity/sx9310.c | 143 +++++++++++++++------------------
>  1 file changed, 67 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
> index d161f3061e353d..07895d4b935d12 100644
> --- a/drivers/iio/proximity/sx9310.c
> +++ b/drivers/iio/proximity/sx9310.c
> @@ -33,45 +33,45 @@
>  #define SX9310_REG_IRQ_SRC				0x00
>  #define SX9310_REG_STAT0				0x01
>  #define SX9310_REG_STAT1				0x02
> +#define SX9310_REG_STAT1_COMPSTAT_MASK			GENMASK(3, 0)

I was curious to see that the patch doesn't change any 'users' of
this macro.  There are a few, so this patch will leave us unable to build
the driver.

>  #define SX9310_REG_IRQ_MSK				0x03
>  #define   SX9310_CONVDONE_IRQ				BIT(3)
>  #define   SX9310_FAR_IRQ				BIT(5)
>  #define   SX9310_CLOSE_IRQ				BIT(6)
> -#define   SX9310_EVENT_IRQ				(SX9310_FAR_IRQ | \
> -							 SX9310_CLOSE_IRQ)
>  #define SX9310_REG_IRQ_FUNC				0x04
>  
>  #define SX9310_REG_PROX_CTRL0				0x10
> -#define   SX9310_REG_PROX_CTRL0_PROXSTAT2		0x10
> -#define   SX9310_REG_PROX_CTRL0_EN_MASK			0x0F
> +#define   SX9310_REG_PROX_CTRL0_SENSOREN_MASK		GENMASK(3, 0)
> +#define   SX9310_REG_PROX_CTRL0_SCANPERIOD_MASK		GENMASK(7, 4)
> +#define   SX9310_REG_PROX_CTRL0_SCANPERIOD_SHIFT	4

Perhaps worth using FIELD_PREP so you don't need to separately define
the shift.

> +#define   SX9310_REG_PROX_CTRL0_SCANPERIOD_15MS		0x01
>  #define SX9310_REG_PROX_CTRL1				0x11
>  #define SX9310_REG_PROX_CTRL2				0x12
> -#define   SX9310_REG_PROX_CTRL2_COMBMODE_ALL		0x80
> -#define   SX9310_REG_PROX_CTRL2_SHIELDEN_DYNAMIC	0x04
> +#define   SX9310_REG_PROX_CTRL2_COMBMODE_CS1_CS2	(0x02 << 6)
> +#define   SX9310_REG_PROX_CTRL2_SHIELDEN_DYNAMIC	(0x01 << 2)
>  #define SX9310_REG_PROX_CTRL3				0x13
> -#define   SX9310_REG_PROX_CTRL3_GAIN0_X8		0x0c
> +#define   SX9310_REG_PROX_CTRL3_GAIN0_X8		(0x03 << 2)
>  #define   SX9310_REG_PROX_CTRL3_GAIN12_X4		0x02
>  #define SX9310_REG_PROX_CTRL4				0x14
>  #define   SX9310_REG_PROX_CTRL4_RESOLUTION_FINEST	0x07
>  #define SX9310_REG_PROX_CTRL5				0x15
> -#define   SX9310_REG_PROX_CTRL5_RANGE_SMALL		0xc0
> -#define   SX9310_REG_PROX_CTRL5_STARTUPSENS_CS1		0x04
> +#define   SX9310_REG_PROX_CTRL5_RANGE_SMALL		(0x03 << 6)
> +#define   SX9310_REG_PROX_CTRL5_STARTUPSENS_CS1		(0x01 << 2)
>  #define   SX9310_REG_PROX_CTRL5_RAWFILT_1P25		0x02
>  #define SX9310_REG_PROX_CTRL6				0x16
> -#define   SX9310_REG_PROX_CTRL6_COMP_COMMON		0x20
> +#define   SX9310_REG_PROX_CTRL6_AVGTHRESH_DEFAULT	0x20
>  #define SX9310_REG_PROX_CTRL7				0x17
> -#define   SX9310_REG_PROX_CTRL7_AVGNEGFILT_2		0x08
> +#define   SX9310_REG_PROX_CTRL7_AVGNEGFILT_2		(0x01 << 3)
>  #define   SX9310_REG_PROX_CTRL7_AVGPOSFILT_512		0x05
>  #define SX9310_REG_PROX_CTRL8				0x18
>  #define SX9310_REG_PROX_CTRL9				0x19
> -#define   SX9310_REG_PROX_CTRL8_9_PTHRESH12_28		0x40
> -#define   SX9310_REG_PROX_CTRL8_9_PTHRESH_96		0x88
> +#define   SX9310_REG_PROX_CTRL8_9_PTHRESH_28		(0x08 << 3)
> +#define   SX9310_REG_PROX_CTRL8_9_PTHRESH_96		(0x11 << 3)
>  #define   SX9310_REG_PROX_CTRL8_9_BODYTHRESH_900	0x03
>  #define   SX9310_REG_PROX_CTRL8_9_BODYTHRESH_1500	0x05
>  #define SX9310_REG_PROX_CTRL10				0x1a
> -#define   SX9310_REG_PROX_CTRL10_HYST_6PCT		0x10
> -#define   SX9310_REG_PROX_CTRL10_CLOSE_DEBOUNCE_8	0x12
> -#define   SX9310_REG_PROX_CTRL10_FAR_DEBOUNCE_8		0x03
> +#define   SX9310_REG_PROX_CTRL10_HYST_6PCT		(0x01 << 4)
> +#define   SX9310_REG_PROX_CTRL10_FAR_DEBOUNCE_2		0x01
>  #define SX9310_REG_PROX_CTRL11				0x1b
>  #define SX9310_REG_PROX_CTRL12				0x1c
>  #define SX9310_REG_PROX_CTRL13				0x1d
> @@ -82,8 +82,8 @@
>  #define SX9310_REG_PROX_CTRL18				0x22
>  #define SX9310_REG_PROX_CTRL19				0x23
>  #define SX9310_REG_SAR_CTRL0				0x2a
> -#define   SX9310_REG_SAR_CTRL0_SARDEB_4_SAMPLES		0x40
> -#define   SX9310_REG_SAR_CTRL0_SARHYST_8		0x10
> +#define   SX9310_REG_SAR_CTRL0_SARDEB_4_SAMPLES		(0x02 << 5)
> +#define   SX9310_REG_SAR_CTRL0_SARHYST_8		(0x02 << 3)
>  #define SX9310_REG_SAR_CTRL1				0x2b
>  /* Each increment of the slope register is 0.0078125. */
>  #define   SX9310_REG_SAR_CTRL1_SLOPE(_hnslope)		(_hnslope / 78125)
> @@ -107,7 +107,7 @@
>  #define SX9310_REG_SAR_MSB				0x39
>  #define SX9310_REG_SAR_LSB				0x3a
>  
> -#define SX9310_REG_I2CADDR				0x40
> +#define SX9310_REG_I2C_ADDR				0x40
>  #define SX9310_REG_PAUSE				0x41
>  #define SX9310_REG_WHOAMI				0x42
>  #define   SX9310_WHOAMI_VALUE				0x01
> @@ -116,14 +116,9 @@
>  #define SX9310_REG_RESET				0x7f
>  #define   SX9310_SOFT_RESET				0xde
>  
> -#define SX9310_SCAN_PERIOD_MASK				GENMASK(7, 4)
> -#define SX9310_SCAN_PERIOD_SHIFT			4
> -
> -#define SX9310_COMPSTAT_MASK				GENMASK(3, 0)
>  
>  /* 4 hardware channels, as defined in STAT0: COMB, CS2, CS1 and CS0. */
>  #define SX9310_NUM_CHANNELS				4
> -#define SX9310_CHAN_ENABLED_MASK			GENMASK(3, 0)
>  
>  struct sx9310_data {
>  	/* Serialize access to registers and channel configuration */
> @@ -251,7 +246,7 @@ static const struct regmap_range sx9310_readable_reg_ranges[] = {
>  	regmap_reg_range(SX9310_REG_PROX_CTRL0, SX9310_REG_PROX_CTRL19),
>  	regmap_reg_range(SX9310_REG_SAR_CTRL0, SX9310_REG_SAR_CTRL2),
>  	regmap_reg_range(SX9310_REG_SENSOR_SEL, SX9310_REG_SAR_LSB),
> -	regmap_reg_range(SX9310_REG_I2CADDR, SX9310_REG_WHOAMI),
> +	regmap_reg_range(SX9310_REG_I2C_ADDR, SX9310_REG_WHOAMI),
>  	regmap_reg_range(SX9310_REG_RESET, SX9310_REG_RESET),
>  };
>  
> @@ -292,7 +287,7 @@ static int sx9310_update_chan_en(struct sx9310_data *data,
>  
>  	if ((data->chan_read | data->chan_event) != (chan_read | chan_event)) {
>  		ret = regmap_update_bits(data->regmap, SX9310_REG_PROX_CTRL0,
> -					 SX9310_CHAN_ENABLED_MASK,
> +					 SX9310_REG_PROX_CTRL0_SENSOREN_MASK,
>  					 chan_read | chan_event);
>  		if (ret)
>  			return ret;
> @@ -361,7 +356,8 @@ static int sx9310_wait_for_sample(struct sx9310_data *data)
>  	if (ret < 0)
>  		return ret;
>  
> -	val = (val & SX9310_SCAN_PERIOD_MASK) >> SX9310_SCAN_PERIOD_SHIFT;
> +	val = (val & SX9310_REG_PROX_CTRL0_SCANPERIOD_MASK) >>
> +	      SX9310_REG_PROX_CTRL0_SCANPERIOD_SHIFT;
>  
>  	msleep(sx9310_scan_period_table[val]);
>  
> @@ -435,7 +431,8 @@ static int sx9310_read_samp_freq(struct sx9310_data *data, int *val, int *val2)
>  	if (ret < 0)
>  		return ret;
>  
> -	regval = (regval & SX9310_SCAN_PERIOD_MASK) >> SX9310_SCAN_PERIOD_SHIFT;
> +	regval = (regval & SX9310_REG_PROX_CTRL0_SCANPERIOD_MASK) >>
> +		 SX9310_REG_PROX_CTRL0_SCANPERIOD_SHIFT;
>  	*val = sx9310_samp_freq_table[regval].val;
>  	*val2 = sx9310_samp_freq_table[regval].val2;
>  
> @@ -483,8 +480,8 @@ static int sx9310_set_samp_freq(struct sx9310_data *data, int val, int val2)
>  	mutex_lock(&data->mutex);
>  
>  	ret = regmap_update_bits(data->regmap, SX9310_REG_PROX_CTRL0,
> -				 SX9310_SCAN_PERIOD_MASK,
> -				 i << SX9310_SCAN_PERIOD_SHIFT);
> +				 SX9310_REG_PROX_CTRL0_SCANPERIOD_MASK,
> +				 i << SX9310_REG_PROX_CTRL0_SCANPERIOD_SHIFT);
>  
>  	mutex_unlock(&data->mutex);
>  
> @@ -572,7 +569,7 @@ static irqreturn_t sx9310_irq_thread_handler(int irq, void *private)
>  		goto out;
>  	}
>  
> -	if (val & SX9310_EVENT_IRQ)
> +	if (val & (SX9310_FAR_IRQ | SX9310_CLOSE_IRQ))
>  		sx9310_push_events(indio_dev);
>  
>  	if (val & SX9310_CONVDONE_IRQ)
> @@ -600,6 +597,7 @@ static int sx9310_write_event_config(struct iio_dev *indio_dev,
>  				     enum iio_event_direction dir, int state)
>  {
>  	struct sx9310_data *data = iio_priv(indio_dev);
> +	unsigned int eventirq = SX9310_FAR_IRQ | SX9310_CLOSE_IRQ;
>  	int ret;
>  
>  	/* If the state hasn't changed, there's nothing to do. */
> @@ -612,7 +610,7 @@ static int sx9310_write_event_config(struct iio_dev *indio_dev,
>  		if (ret < 0)
>  			goto out_unlock;
>  		if (!(data->chan_event & ~BIT(chan->channel))) {
> -			ret = sx9310_enable_irq(data, SX9310_EVENT_IRQ);
> +			ret = sx9310_enable_irq(data, eventirq);
>  			if (ret < 0)
>  				sx9310_put_event_channel(data, chan->channel);
>  		}
> @@ -621,7 +619,7 @@ static int sx9310_write_event_config(struct iio_dev *indio_dev,
>  		if (ret < 0)
>  			goto out_unlock;
>  		if (!data->chan_event) {
> -			ret = sx9310_disable_irq(data, SX9310_EVENT_IRQ);
> +			ret = sx9310_disable_irq(data, eventirq);
>  			if (ret < 0)
>  				sx9310_get_event_channel(data, chan->channel);
>  		}
> @@ -746,53 +744,46 @@ struct sx9310_reg_default {
>  	u8 def;
>  };
>  
> -#define SX_INIT(_reg, _def)			\
> -	{					\
> -		.reg = SX9310_REG_##_reg,	\
> -		.def = _def,			\
> -	}
> -
>  static const struct sx9310_reg_default sx9310_default_regs[] = {
> -	SX_INIT(IRQ_MSK, 0x00),
> -	SX_INIT(IRQ_FUNC, 0x00),
> +	{ SX9310_REG_IRQ_MSK, 0x00 },
> +	{ SX9310_REG_IRQ_FUNC, 0x00 },
>  	/*
>  	 * The lower 4 bits should not be set as it enable sensors measurements.
>  	 * Turning the detection on before the configuration values are set to
>  	 * good values can cause the device to return erroneous readings.
>  	 */
> -	SX_INIT(PROX_CTRL0, SX9310_REG_PROX_CTRL0_PROXSTAT2),
> -	SX_INIT(PROX_CTRL1, 0x00),
> -	SX_INIT(PROX_CTRL2, SX9310_REG_PROX_CTRL2_COMBMODE_ALL |
> -			    SX9310_REG_PROX_CTRL2_SHIELDEN_DYNAMIC),
> -	SX_INIT(PROX_CTRL3, SX9310_REG_PROX_CTRL3_GAIN0_X8 |
> -			    SX9310_REG_PROX_CTRL3_GAIN12_X4),
> -	SX_INIT(PROX_CTRL4, SX9310_REG_PROX_CTRL4_RESOLUTION_FINEST),
> -	SX_INIT(PROX_CTRL5, SX9310_REG_PROX_CTRL5_RANGE_SMALL |
> -			    SX9310_REG_PROX_CTRL5_STARTUPSENS_CS1 |
> -			    SX9310_REG_PROX_CTRL5_RAWFILT_1P25),
> -	SX_INIT(PROX_CTRL6, SX9310_REG_PROX_CTRL6_COMP_COMMON),
> -	SX_INIT(PROX_CTRL7, SX9310_REG_PROX_CTRL7_AVGNEGFILT_2 |
> -			    SX9310_REG_PROX_CTRL7_AVGPOSFILT_512),
> -	SX_INIT(PROX_CTRL8, SX9310_REG_PROX_CTRL8_9_PTHRESH_96 |
> -			    SX9310_REG_PROX_CTRL8_9_BODYTHRESH_1500),
> -	SX_INIT(PROX_CTRL9, SX9310_REG_PROX_CTRL8_9_PTHRESH12_28 |
> -			    SX9310_REG_PROX_CTRL8_9_BODYTHRESH_900),
> -	SX_INIT(PROX_CTRL10, SX9310_REG_PROX_CTRL10_HYST_6PCT |
> -			     SX9310_REG_PROX_CTRL10_CLOSE_DEBOUNCE_8 |
> -			     SX9310_REG_PROX_CTRL10_FAR_DEBOUNCE_8),
> -	SX_INIT(PROX_CTRL11, 0x00),
> -	SX_INIT(PROX_CTRL12, 0x00),
> -	SX_INIT(PROX_CTRL13, 0x00),
> -	SX_INIT(PROX_CTRL14, 0x00),
> -	SX_INIT(PROX_CTRL15, 0x00),
> -	SX_INIT(PROX_CTRL16, 0x00),
> -	SX_INIT(PROX_CTRL17, 0x00),
> -	SX_INIT(PROX_CTRL18, 0x00),
> -	SX_INIT(PROX_CTRL19, 0x00),
> -	SX_INIT(SAR_CTRL0, SX9310_REG_SAR_CTRL0_SARDEB_4_SAMPLES |
> -			   SX9310_REG_SAR_CTRL0_SARHYST_8),
> -	SX_INIT(SAR_CTRL1, SX9310_REG_SAR_CTRL1_SLOPE(10781250)),
> -	SX_INIT(SAR_CTRL2, SX9310_REG_SAR_CTRL2_SAROFFSET_DEFAULT),
> +	{ SX9310_REG_PROX_CTRL0, SX9310_REG_PROX_CTRL0_SCANPERIOD_15MS },
> +	{ SX9310_REG_PROX_CTRL1, 0x00 },
> +	{ SX9310_REG_PROX_CTRL2, SX9310_REG_PROX_CTRL2_COMBMODE_CS1_CS2 |
> +				 SX9310_REG_PROX_CTRL2_SHIELDEN_DYNAMIC },
> +	{ SX9310_REG_PROX_CTRL3, SX9310_REG_PROX_CTRL3_GAIN0_X8 |
> +				 SX9310_REG_PROX_CTRL3_GAIN12_X4 },
> +	{ SX9310_REG_PROX_CTRL4, SX9310_REG_PROX_CTRL4_RESOLUTION_FINEST },
> +	{ SX9310_REG_PROX_CTRL5, SX9310_REG_PROX_CTRL5_RANGE_SMALL |
> +				 SX9310_REG_PROX_CTRL5_STARTUPSENS_CS1 |
> +				 SX9310_REG_PROX_CTRL5_RAWFILT_1P25 },
> +	{ SX9310_REG_PROX_CTRL6, SX9310_REG_PROX_CTRL6_AVGTHRESH_DEFAULT },
> +	{ SX9310_REG_PROX_CTRL7, SX9310_REG_PROX_CTRL7_AVGNEGFILT_2 |
> +				 SX9310_REG_PROX_CTRL7_AVGPOSFILT_512 },
> +	{ SX9310_REG_PROX_CTRL8, SX9310_REG_PROX_CTRL8_9_PTHRESH_96 |
> +				 SX9310_REG_PROX_CTRL8_9_BODYTHRESH_1500 },
> +	{ SX9310_REG_PROX_CTRL9, SX9310_REG_PROX_CTRL8_9_PTHRESH_28 |
> +				 SX9310_REG_PROX_CTRL8_9_BODYTHRESH_900 },
> +	{ SX9310_REG_PROX_CTRL10, SX9310_REG_PROX_CTRL10_HYST_6PCT |
> +				  SX9310_REG_PROX_CTRL10_FAR_DEBOUNCE_2 },
> +	{ SX9310_REG_PROX_CTRL11, 0x00 },
> +	{ SX9310_REG_PROX_CTRL12, 0x00 },
> +	{ SX9310_REG_PROX_CTRL13, 0x00 },
> +	{ SX9310_REG_PROX_CTRL14, 0x00 },
> +	{ SX9310_REG_PROX_CTRL15, 0x00 },
> +	{ SX9310_REG_PROX_CTRL16, 0x00 },
> +	{ SX9310_REG_PROX_CTRL17, 0x00 },
> +	{ SX9310_REG_PROX_CTRL18, 0x00 },
> +	{ SX9310_REG_PROX_CTRL19, 0x00 },
> +	{ SX9310_REG_SAR_CTRL0, SX9310_REG_SAR_CTRL0_SARDEB_4_SAMPLES |
> +				SX9310_REG_SAR_CTRL0_SARHYST_8 },
> +	{ SX9310_REG_SAR_CTRL1, SX9310_REG_SAR_CTRL1_SLOPE(10781250) },
> +	{ SX9310_REG_SAR_CTRL2, SX9310_REG_SAR_CTRL2_SAROFFSET_DEFAULT },
>  };
>  
>  /* Activate all channels and perform an initial compensation. */
> @@ -809,7 +800,7 @@ static int sx9310_init_compensation(struct iio_dev *indio_dev)
>  
>  	/* run the compensation phase on all channels */
>  	ret = regmap_write(data->regmap, SX9310_REG_PROX_CTRL0,
> -			   ctrl0 | SX9310_REG_PROX_CTRL0_EN_MASK);
> +			   ctrl0 | SX9310_REG_PROX_CTRL0_SENSOREN_MASK);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -992,7 +983,7 @@ static int __maybe_unused sx9310_suspend(struct device *dev)
>  	if (ret)
>  		goto out;
>  
> -	ctrl0 = data->suspend_ctrl0 & ~SX9310_REG_PROX_CTRL0_EN_MASK;
> +	ctrl0 = data->suspend_ctrl0 & ~SX9310_REG_PROX_CTRL0_SENSOREN_MASK;
>  	ret = regmap_write(data->regmap, SX9310_REG_PROX_CTRL0, ctrl0);
>  	if (ret)
>  		goto out;

[PATCH v3 03/15] iio: sx9310: Fix irq handling

[Daniel Campello]

Fixes enable/disable irq handling at various points. The driver needs to
only enable/disable irqs if there is an actual irq handler installed.

Signed-off-by: Daniel Campello <campello@chromium.org>
---

Changes in v3:
 - Moved irq presence check down to lower methods

Changes in v2:
 - Reordered error handling on sx9310_resume()

 drivers/iio/proximity/sx9310.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index 07895d4b935d12..c7a27c21c20cd1 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -323,12 +323,18 @@ static int sx9310_put_event_channel(struct sx9310_data *data, int channel)
 
 static int sx9310_enable_irq(struct sx9310_data *data, unsigned int irq)
 {
-	return regmap_update_bits(data->regmap, SX9310_REG_IRQ_MSK, irq, irq);
+	if (data->client->irq)
+		return regmap_update_bits(data->regmap, SX9310_REG_IRQ_MSK, irq,
+					  irq);
+	return 0;
 }
 
 static int sx9310_disable_irq(struct sx9310_data *data, unsigned int irq)
 {
-	return regmap_update_bits(data->regmap, SX9310_REG_IRQ_MSK, irq, 0);
+	if (data->client->irq)
+		return regmap_update_bits(data->regmap, SX9310_REG_IRQ_MSK, irq,
+					  0);
+	return 0;
 }
 
 static int sx9310_read_prox_data(struct sx9310_data *data,
@@ -382,7 +388,7 @@ static int sx9310_read_proximity(struct sx9310_data *data,
 
 	mutex_unlock(&data->mutex);
 
-	if (data->client->irq > 0) {
+	if (data->client->irq) {
 		ret = wait_for_completion_interruptible(&data->completion);
 		reinit_completion(&data->completion);
 	} else {
@@ -1011,10 +1017,11 @@ static int __maybe_unused sx9310_resume(struct device *dev)
 
 out:
 	mutex_unlock(&data->mutex);
+	if (ret)
+		return ret;
 
 	enable_irq(data->client->irq);
-
-	return ret;
+	return 0;
 }
 
 static const struct dev_pm_ops sx9310_pm_ops = {
-- 
2.28.0.163.g6104cc2f0b6-goog

Re: [PATCH v3 03/15] iio: sx9310: Fix irq handling

[Stephen Boyd]

Quoting Daniel Campello (2020-07-31 09:48:40)
> Fixes enable/disable irq handling at various points. The driver needs to
> only enable/disable irqs if there is an actual irq handler installed.
> 
> Signed-off-by: Daniel Campello <campello@chromium.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [PATCH v3 03/15] iio: sx9310: Fix irq handling

[Andy Shevchenko]

On Fri, Jul 31, 2020 at 7:49 PM Daniel Campello <campello@chromium.org> wrote:
>
> Fixes enable/disable irq handling at various points. The driver needs to
> only enable/disable irqs if there is an actual irq handler installed.

...

>  static int sx9310_enable_irq(struct sx9310_data *data, unsigned int irq)
>  {
> -       return regmap_update_bits(data->regmap, SX9310_REG_IRQ_MSK, irq, irq);
> +       if (data->client->irq)
> +               return regmap_update_bits(data->regmap, SX9310_REG_IRQ_MSK, irq,
> +                                         irq);
> +       return 0;

Wouldn't it be better to insert rather
if (!irq)
  return 0;
than disrupting the entire function?

>  }
>
>  static int sx9310_disable_irq(struct sx9310_data *data, unsigned int irq)
>  {
> -       return regmap_update_bits(data->regmap, SX9310_REG_IRQ_MSK, irq, 0);
> +       if (data->client->irq)
> +               return regmap_update_bits(data->regmap, SX9310_REG_IRQ_MSK, irq,
> +                                         0);
> +       return 0;

Ditto.

>  }



-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 04/15] iio: sx9310: Remove acpi and of table macros

[Daniel Campello]

Avoids unused warnings due to acpi/of table macros.

Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Daniel Campello <campello@chromium.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---

Changes in v3: None
Changes in v2:
 - Added #include <linux/mod_devicetable.h>

 drivers/iio/proximity/sx9310.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index c7a27c21c20cd1..66f7050d63ae97 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -15,8 +15,8 @@
 #include <linux/i2c.h>
 #include <linux/irq.h>
 #include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
-#include <linux/of.h>
 #include <linux/pm.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
@@ -1052,8 +1052,8 @@ MODULE_DEVICE_TABLE(i2c, sx9310_id);
 static struct i2c_driver sx9310_driver = {
 	.driver = {
 		.name	= "sx9310",
-		.acpi_match_table = ACPI_PTR(sx9310_acpi_match),
-		.of_match_table = of_match_ptr(sx9310_of_match),
+		.acpi_match_table = sx9310_acpi_match,
+		.of_match_table = sx9310_of_match,
 		.pm = &sx9310_pm_ops,
 	},
 	.probe		= sx9310_probe,
-- 
2.28.0.163.g6104cc2f0b6-goog

[PATCH v3 05/15] iio: sx9310: Change from .probe to .probe_new

[Daniel Campello]

Uses .probe_new in place of .probe. Also uses device_get_match_data()
for whoami matching.

Signed-off-by: Daniel Campello <campello@chromium.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---

Changes in v3: None
Changes in v2:
 - Added '\n' to dev_err()

 drivers/iio/proximity/sx9310.c | 39 ++++++++++++----------------------
 1 file changed, 14 insertions(+), 25 deletions(-)

diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index 66f7050d63ae97..2ed062d01634bc 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -139,7 +139,7 @@ struct sx9310_data {
 	struct completion completion;
 	unsigned int chan_read, chan_event;
 	int channel_users[SX9310_NUM_CHANNELS];
-	int whoami;
+	unsigned int whoami;
 };
 
 static const struct iio_event_spec sx9310_events[] = {
@@ -861,24 +861,15 @@ static int sx9310_init_device(struct iio_dev *indio_dev)
 
 static int sx9310_set_indio_dev_name(struct device *dev,
 				     struct iio_dev *indio_dev,
-				     const struct i2c_device_id *id, int whoami)
+				     unsigned int whoami)
 {
-	const struct acpi_device_id *acpi_id;
-
-	/* id will be NULL when enumerated via ACPI */
-	if (id) {
-		if (id->driver_data != whoami)
-			dev_err(dev, "WHOAMI does not match i2c_device_id: %s",
-				id->name);
-	} else if (ACPI_HANDLE(dev)) {
-		acpi_id = acpi_match_device(dev->driver->acpi_match_table, dev);
-		if (!acpi_id)
-			return -ENODEV;
-		if (acpi_id->driver_data != whoami)
-			dev_err(dev, "WHOAMI does not match acpi_device_id: %s",
-				acpi_id->id);
-	} else
+	unsigned int long ddata;
+
+	ddata = (uintptr_t)device_get_match_data(dev);
+	if (ddata != whoami) {
+		dev_err(dev, "WHOAMI does not match device data: %u\n", whoami);
 		return -ENODEV;
+	}
 
 	switch (whoami) {
 	case SX9310_WHOAMI_VALUE:
@@ -888,15 +879,14 @@ static int sx9310_set_indio_dev_name(struct device *dev,
 		indio_dev->name = "sx9311";
 		break;
 	default:
-		dev_err(dev, "unexpected WHOAMI response: %u", whoami);
+		dev_err(dev, "unexpected WHOAMI response: %u\n", whoami);
 		return -ENODEV;
 	}
 
 	return 0;
 }
 
-static int sx9310_probe(struct i2c_client *client,
-			const struct i2c_device_id *id)
+static int sx9310_probe(struct i2c_client *client)
 {
 	int ret;
 	struct iio_dev *indio_dev;
@@ -922,8 +912,7 @@ static int sx9310_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	ret = sx9310_set_indio_dev_name(&client->dev, indio_dev, id,
-					data->whoami);
+	ret = sx9310_set_indio_dev_name(&client->dev, indio_dev, data->whoami);
 	if (ret < 0)
 		return ret;
 
@@ -1036,8 +1025,8 @@ static const struct acpi_device_id sx9310_acpi_match[] = {
 MODULE_DEVICE_TABLE(acpi, sx9310_acpi_match);
 
 static const struct of_device_id sx9310_of_match[] = {
-	{ .compatible = "semtech,sx9310" },
-	{ .compatible = "semtech,sx9311" },
+	{ .compatible = "semtech,sx9310", (void *)SX9310_WHOAMI_VALUE },
+	{ .compatible = "semtech,sx9311", (void *)SX9311_WHOAMI_VALUE },
 	{},
 };
 MODULE_DEVICE_TABLE(of, sx9310_of_match);
@@ -1056,7 +1045,7 @@ static struct i2c_driver sx9310_driver = {
 		.of_match_table = sx9310_of_match,
 		.pm = &sx9310_pm_ops,
 	},
-	.probe		= sx9310_probe,
+	.probe_new	= sx9310_probe,
 	.id_table	= sx9310_id,
 };
 module_i2c_driver(sx9310_driver);
-- 
2.28.0.163.g6104cc2f0b6-goog

[PATCH v3 06/15] iio: sx9310: Fixes various memory handling

[Daniel Campello]

Makes use __aligned(8) to ensure that the timestamp is correctly aligned
when we call io_push_to_buffers_with_timestamp().
Also makes use of sizeof() for regmap_bulk_read instead of static value.

Signed-off-by: Daniel Campello <campello@chromium.org>
---

Changes in v3:
 - Changed buffer to struct type to align timestamp memory properly.

Changes in v2:
 - Fixed commit message from "iio: sx9310: Align memory"

 drivers/iio/proximity/sx9310.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index 2ed062d01634bc..c46584b4817b4a 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -132,8 +132,11 @@ struct sx9310_data {
 	 */
 	bool prox_stat[SX9310_NUM_CHANNELS];
 	bool trigger_enabled;
-	__be16 buffer[SX9310_NUM_CHANNELS +
-		      4]; /* 64-bit data + 64-bit timestamp */
+	/* Ensure correct alignment of timestamp when present. */
+	struct {
+		__be16 channels[SX9310_NUM_CHANNELS];
+		s64 ts __aligned(8);
+	} buffer;
 	/* Remember enabled channels and sample rate during suspend. */
 	unsigned int suspend_ctrl0;
 	struct completion completion;
@@ -346,7 +349,7 @@ static int sx9310_read_prox_data(struct sx9310_data *data,
 	if (ret < 0)
 		return ret;
 
-	return regmap_bulk_read(data->regmap, chan->address, val, 2);
+	return regmap_bulk_read(data->regmap, chan->address, val, sizeof(*val));
 }
 
 /*
@@ -697,10 +700,10 @@ static irqreturn_t sx9310_trigger_handler(int irq, void *private)
 		if (ret < 0)
 			goto out;
 
-		data->buffer[i++] = val;
+		data->buffer.channels[i++] = val;
 	}
 
-	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
+	iio_push_to_buffers_with_timestamp(indio_dev, data->buffer.channels,
 					   pf->timestamp);
 
 out:
-- 
2.28.0.163.g6104cc2f0b6-goog

Re: [PATCH v3 06/15] iio: sx9310: Fixes various memory handling

[Andy Shevchenko]

On Fri, Jul 31, 2020 at 7:49 PM Daniel Campello <campello@chromium.org> wrote:
>
> Makes use __aligned(8) to ensure that the timestamp is correctly aligned
> when we call io_push_to_buffers_with_timestamp().
> Also makes use of sizeof() for regmap_bulk_read instead of static value.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Daniel Campello <campello@chromium.org>
> ---
>
> Changes in v3:
>  - Changed buffer to struct type to align timestamp memory properly.
>
> Changes in v2:
>  - Fixed commit message from "iio: sx9310: Align memory"
>
>  drivers/iio/proximity/sx9310.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
> index 2ed062d01634bc..c46584b4817b4a 100644
> --- a/drivers/iio/proximity/sx9310.c
> +++ b/drivers/iio/proximity/sx9310.c
> @@ -132,8 +132,11 @@ struct sx9310_data {
>          */
>         bool prox_stat[SX9310_NUM_CHANNELS];
>         bool trigger_enabled;
> -       __be16 buffer[SX9310_NUM_CHANNELS +
> -                     4]; /* 64-bit data + 64-bit timestamp */
> +       /* Ensure correct alignment of timestamp when present. */
> +       struct {
> +               __be16 channels[SX9310_NUM_CHANNELS];
> +               s64 ts __aligned(8);
> +       } buffer;
>         /* Remember enabled channels and sample rate during suspend. */
>         unsigned int suspend_ctrl0;
>         struct completion completion;
> @@ -346,7 +349,7 @@ static int sx9310_read_prox_data(struct sx9310_data *data,
>         if (ret < 0)
>                 return ret;
>
> -       return regmap_bulk_read(data->regmap, chan->address, val, 2);
> +       return regmap_bulk_read(data->regmap, chan->address, val, sizeof(*val));
>  }
>
>  /*
> @@ -697,10 +700,10 @@ static irqreturn_t sx9310_trigger_handler(int irq, void *private)
>                 if (ret < 0)
>                         goto out;
>
> -               data->buffer[i++] = val;
> +               data->buffer.channels[i++] = val;
>         }
>
> -       iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> +       iio_push_to_buffers_with_timestamp(indio_dev, data->buffer.channels,
>                                            pf->timestamp);
>
>  out:
> --
> 2.28.0.163.g6104cc2f0b6-goog
>


--
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 06/15] iio: sx9310: Fixes various memory handling

[Jonathan Cameron]

On Fri, 31 Jul 2020 22:24:47 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Jul 31, 2020 at 7:49 PM Daniel Campello <campello@chromium.org> wrote:
> >
> > Makes use __aligned(8) to ensure that the timestamp is correctly aligned
> > when we call io_push_to_buffers_with_timestamp().
> > Also makes use of sizeof() for regmap_bulk_read instead of static value.  
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> > Signed-off-by: Daniel Campello <campello@chromium.org>
> > ---
> >
> > Changes in v3:
> >  - Changed buffer to struct type to align timestamp memory properly.
> >
> > Changes in v2:
> >  - Fixed commit message from "iio: sx9310: Align memory"
> >
> >  drivers/iio/proximity/sx9310.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
> > index 2ed062d01634bc..c46584b4817b4a 100644
> > --- a/drivers/iio/proximity/sx9310.c
> > +++ b/drivers/iio/proximity/sx9310.c
> > @@ -132,8 +132,11 @@ struct sx9310_data {
> >          */
> >         bool prox_stat[SX9310_NUM_CHANNELS];
> >         bool trigger_enabled;
> > -       __be16 buffer[SX9310_NUM_CHANNELS +
> > -                     4]; /* 64-bit data + 64-bit timestamp */
> > +       /* Ensure correct alignment of timestamp when present. */
> > +       struct {
> > +               __be16 channels[SX9310_NUM_CHANNELS];
> > +               s64 ts __aligned(8);
> > +       } buffer;
> >         /* Remember enabled channels and sample rate during suspend. */
> >         unsigned int suspend_ctrl0;
> >         struct completion completion;
> > @@ -346,7 +349,7 @@ static int sx9310_read_prox_data(struct sx9310_data *data,
> >         if (ret < 0)
> >                 return ret;
> >
> > -       return regmap_bulk_read(data->regmap, chan->address, val, 2);
> > +       return regmap_bulk_read(data->regmap, chan->address, val, sizeof(*val));
> >  }
> >
> >  /*
> > @@ -697,10 +700,10 @@ static irqreturn_t sx9310_trigger_handler(int irq, void *private)
> >                 if (ret < 0)
> >                         goto out;
> >
> > -               data->buffer[i++] = val;
> > +               data->buffer.channels[i++] = val;
> >         }
> >
> > -       iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
> > +       iio_push_to_buffers_with_timestamp(indio_dev, data->buffer.channels,

Whilst it's the same address, it makes more sense to push &data->buffer
Technically buffer.channels isn't large enough to meant the requirements
of iio_push_to_buffers_with_timestamp.

Otherwise, looks good.

Thanks,


Jonathan

> >                                            pf->timestamp);
> >
> >  out:
> > --
> > 2.28.0.163.g6104cc2f0b6-goog
> >  
> 
> 
> --
> With Best Regards,
> Andy Shevchenko

[PATCH v3 07/15] iio: sx9310: Use long instead of int for channel bitmaps

[Daniel Campello]

Uses for_each_set_bit() macro to loop over channel bitmaps.

Signed-off-by: Daniel Campello <campello@chromium.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---

Changes in v3:
 - Added static assert for number of channels.

Changes in v2:
 - Changed prox_stat to chan_prox_stat bitmap.

 drivers/iio/proximity/sx9310.c | 39 ++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index c46584b4817b4a..8f80a4c20ac05e 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -119,6 +119,7 @@
 
 /* 4 hardware channels, as defined in STAT0: COMB, CS2, CS1 and CS0. */
 #define SX9310_NUM_CHANNELS				4
+static_assert(SX9310_NUM_CHANNELS < BITS_PER_LONG);
 
 struct sx9310_data {
 	/* Serialize access to registers and channel configuration */
@@ -130,7 +131,7 @@ struct sx9310_data {
 	 * Last reading of the proximity status for each channel.
 	 * We only send an event to user space when this changes.
 	 */
-	bool prox_stat[SX9310_NUM_CHANNELS];
+	unsigned long chan_prox_stat;
 	bool trigger_enabled;
 	/* Ensure correct alignment of timestamp when present. */
 	struct {
@@ -140,7 +141,8 @@ struct sx9310_data {
 	/* Remember enabled channels and sample rate during suspend. */
 	unsigned int suspend_ctrl0;
 	struct completion completion;
-	unsigned int chan_read, chan_event;
+	unsigned long chan_read;
+	unsigned long chan_event;
 	int channel_users[SX9310_NUM_CHANNELS];
 	unsigned int whoami;
 };
@@ -283,15 +285,16 @@ static const struct regmap_config sx9310_regmap_config = {
 };
 
 static int sx9310_update_chan_en(struct sx9310_data *data,
-				 unsigned int chan_read,
-				 unsigned int chan_event)
+				 unsigned long chan_read,
+				 unsigned long chan_event)
 {
 	int ret;
+	unsigned long channels = chan_read | chan_event;
 
-	if ((data->chan_read | data->chan_event) != (chan_read | chan_event)) {
+	if ((data->chan_read | data->chan_event) != channels) {
 		ret = regmap_update_bits(data->regmap, SX9310_REG_PROX_CTRL0,
 					 SX9310_REG_PROX_CTRL0_SENSOREN_MASK,
-					 chan_read | chan_event);
+					 channels);
 		if (ret)
 			return ret;
 	}
@@ -535,6 +538,7 @@ static void sx9310_push_events(struct iio_dev *indio_dev)
 	unsigned int val, chan;
 	struct sx9310_data *data = iio_priv(indio_dev);
 	s64 timestamp = iio_get_time_ns(indio_dev);
+	unsigned long prox_changed;
 
 	/* Read proximity state on all channels */
 	ret = regmap_read(data->regmap, SX9310_REG_STAT0, &val);
@@ -543,24 +547,23 @@ static void sx9310_push_events(struct iio_dev *indio_dev)
 		return;
 	}
 
-	for (chan = 0; chan < SX9310_NUM_CHANNELS; chan++) {
+	/*
+	 * Only iterate over channels with changes on proximity status that have
+	 * events enabled.
+	 */
+	prox_changed = (data->chan_prox_stat ^ val) & data->chan_event;
+
+	for_each_set_bit(chan, &prox_changed, SX9310_NUM_CHANNELS) {
 		int dir;
 		u64 ev;
-		bool new_prox = val & BIT(chan);
-
-		if (!(data->chan_event & BIT(chan)))
-			continue;
-		if (new_prox == data->prox_stat[chan])
-			/* No change on this channel. */
-			continue;
 
-		dir = new_prox ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
+		dir = val & BIT(chan) ? 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, timestamp);
-		data->prox_stat[chan] = new_prox;
 	}
+	data->chan_prox_stat = val;
 }
 
 static irqreturn_t sx9310_irq_thread_handler(int irq, void *private)
@@ -717,13 +720,13 @@ static irqreturn_t sx9310_trigger_handler(int irq, void *private)
 static int sx9310_buffer_preenable(struct iio_dev *indio_dev)
 {
 	struct sx9310_data *data = iio_priv(indio_dev);
-	unsigned int channels = 0;
+	unsigned long channels = 0;
 	int bit, ret;
 
 	mutex_lock(&data->mutex);
 	for_each_set_bit(bit, indio_dev->active_scan_mask,
 			 indio_dev->masklength)
-		channels |= BIT(indio_dev->channels[bit].channel);
+		__set_bit(indio_dev->channels[bit].channel, &channels);
 
 	ret = sx9310_update_chan_en(data, channels, data->chan_event);
 	mutex_unlock(&data->mutex);
-- 
2.28.0.163.g6104cc2f0b6-goog

Re: [PATCH v3 07/15] iio: sx9310: Use long instead of int for channel bitmaps

[Stephen Boyd]

Quoting Daniel Campello (2020-07-31 09:48:44)
> Uses for_each_set_bit() macro to loop over channel bitmaps.
> 
> Signed-off-by: Daniel Campello <campello@chromium.org>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

[PATCH v3 08/15] iio: sx9310: Use regmap_read_poll_timeout() for compensation

[Daniel Campello]

Simplify compensation stage by using regmap_read_poll_timeout().

Signed-off-by: Daniel Campello <campello@chromium.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---

Changes in v3: None
Changes in v2:
 - Fixed dev_err() message

 drivers/iio/proximity/sx9310.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index 8f80a4c20ac05e..9f6292cc86146d 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -802,7 +802,7 @@ static const struct sx9310_reg_default sx9310_default_regs[] = {
 static int sx9310_init_compensation(struct iio_dev *indio_dev)
 {
 	struct sx9310_data *data = iio_priv(indio_dev);
-	int i, ret;
+	int ret;
 	unsigned int val;
 	unsigned int ctrl0;
 
@@ -816,22 +816,17 @@ static int sx9310_init_compensation(struct iio_dev *indio_dev)
 	if (ret < 0)
 		return ret;
 
-	for (i = 100; i >= 0; i--) {
-		msleep(20);
-		ret = regmap_read(data->regmap, SX9310_REG_STAT1, &val);
-		if (ret < 0)
-			goto out;
-		if (!(val & SX9310_COMPSTAT_MASK))
-			break;
-	}
-
-	if (i < 0) {
-		dev_err(&data->client->dev,
-			"initial compensation timed out: 0x%02x", val);
-		ret = -ETIMEDOUT;
+	ret = regmap_read_poll_timeout(data->regmap, SX9310_REG_STAT1, val,
+				       !(val & SX9310_REG_STAT1_COMPSTAT_MASK),
+				       20000, 2000000);
+	if (ret) {
+		if (ret == -ETIMEDOUT)
+			dev_err(&data->client->dev,
+				"initial compensation timed out: 0x%02x\n",
+				val);
+		return ret;
 	}
 
-out:
 	regmap_write(data->regmap, SX9310_REG_PROX_CTRL0, ctrl0);
 	return ret;
 }
-- 
2.28.0.163.g6104cc2f0b6-goog

Re: [PATCH v3 08/15] iio: sx9310: Use regmap_read_poll_timeout() for compensation

[Stephen Boyd]

Quoting Daniel Campello (2020-07-31 09:48:45)
> Simplify compensation stage by using regmap_read_poll_timeout().
> 
> Signed-off-by: Daniel Campello <campello@chromium.org>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

[PATCH v3 09/15] iio: sx9310: Update copyright

[Daniel Campello]

Fixes wrong copyright year.

Signed-off-by: Daniel Campello <campello@chromium.org>
---

Changes in v3: None
Changes in v2: None

 drivers/iio/proximity/sx9310.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index 9f6292cc86146d..19253bf335437f 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -1,13 +1,13 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * Copyright 2018 Google LLC.
+ * Copyright 2020 Google LLC.
  *
  * Driver for Semtech's SX9310/SX9311 capacitive proximity/button solution.
  * Based on SX9500 driver and Semtech driver using the input framework
  * <https://my.syncplicity.com/share/teouwsim8niiaud/
  *          linux-driver-SX9310_NoSmartHSensing>.
- * Reworked April 2019 by Evan Green <evgreen@chromium.org>
- * and January 2020 by Daniel Campello <campello@chromium.org>
+ * Reworked in April 2019 by Evan Green <evgreen@chromium.org>
+ * and in January 2020 by Daniel Campello <campello@chromium.org>.
  */
 
 #include <linux/acpi.h>
-- 
2.28.0.163.g6104cc2f0b6-goog

[PATCH v3 10/15] iio: sx9310: Simplify error return handling

[Daniel Campello]

Checks for non-zero return values to signal error conditions.

Signed-off-by: Daniel Campello <campello@chromium.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---

Changes in v3: None
Changes in v2: None

 drivers/iio/proximity/sx9310.c | 56 +++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index 19253bf335437f..6c5425c6fe0d03 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -349,7 +349,7 @@ static int sx9310_read_prox_data(struct sx9310_data *data,
 	int ret;
 
 	ret = regmap_write(data->regmap, SX9310_REG_SENSOR_SEL, chan->channel);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	return regmap_bulk_read(data->regmap, chan->address, val, sizeof(*val));
@@ -365,7 +365,7 @@ static int sx9310_wait_for_sample(struct sx9310_data *data)
 	unsigned int val;
 
 	ret = regmap_read(data->regmap, SX9310_REG_PROX_CTRL0, &val);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	val = (val & SX9310_REG_PROX_CTRL0_SCANPERIOD_MASK) >>
@@ -379,17 +379,17 @@ static int sx9310_wait_for_sample(struct sx9310_data *data)
 static int sx9310_read_proximity(struct sx9310_data *data,
 				 const struct iio_chan_spec *chan, int *val)
 {
-	int ret = 0;
+	int ret;
 	__be16 rawval;
 
 	mutex_lock(&data->mutex);
 
 	ret = sx9310_get_read_channel(data, chan->channel);
-	if (ret < 0)
+	if (ret)
 		goto out;
 
 	ret = sx9310_enable_irq(data, SX9310_CONVDONE_IRQ);
-	if (ret < 0)
+	if (ret)
 		goto out_put_channel;
 
 	mutex_unlock(&data->mutex);
@@ -403,22 +403,22 @@ static int sx9310_read_proximity(struct sx9310_data *data,
 
 	mutex_lock(&data->mutex);
 
-	if (ret < 0)
+	if (ret)
 		goto out_disable_irq;
 
 	ret = sx9310_read_prox_data(data, chan, &rawval);
-	if (ret < 0)
+	if (ret)
 		goto out_disable_irq;
 
 	*val = sign_extend32(be16_to_cpu(rawval),
 			     (chan->address == SX9310_REG_DIFF_MSB ? 11 : 15));
 
 	ret = sx9310_disable_irq(data, SX9310_CONVDONE_IRQ);
-	if (ret < 0)
+	if (ret)
 		goto out_put_channel;
 
 	ret = sx9310_put_read_channel(data, chan->channel);
-	if (ret < 0)
+	if (ret)
 		goto out;
 
 	mutex_unlock(&data->mutex);
@@ -440,7 +440,7 @@ static int sx9310_read_samp_freq(struct sx9310_data *data, int *val, int *val2)
 	unsigned int regval;
 	int ret = regmap_read(data->regmap, SX9310_REG_PROX_CTRL0, &regval);
 
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	regval = (regval & SX9310_REG_PROX_CTRL0_SCANPERIOD_MASK) >>
@@ -542,7 +542,7 @@ static void sx9310_push_events(struct iio_dev *indio_dev)
 
 	/* Read proximity state on all channels */
 	ret = regmap_read(data->regmap, SX9310_REG_STAT0, &val);
-	if (ret < 0) {
+	if (ret) {
 		dev_err(&data->client->dev, "i2c transfer error in irq\n");
 		return;
 	}
@@ -576,7 +576,7 @@ static irqreturn_t sx9310_irq_thread_handler(int irq, void *private)
 	mutex_lock(&data->mutex);
 
 	ret = regmap_read(data->regmap, SX9310_REG_IRQ_SRC, &val);
-	if (ret < 0) {
+	if (ret) {
 		dev_err(&data->client->dev, "i2c transfer error in irq\n");
 		goto out;
 	}
@@ -619,20 +619,20 @@ static int sx9310_write_event_config(struct iio_dev *indio_dev,
 	mutex_lock(&data->mutex);
 	if (state) {
 		ret = sx9310_get_event_channel(data, chan->channel);
-		if (ret < 0)
+		if (ret)
 			goto out_unlock;
 		if (!(data->chan_event & ~BIT(chan->channel))) {
 			ret = sx9310_enable_irq(data, eventirq);
-			if (ret < 0)
+			if (ret)
 				sx9310_put_event_channel(data, chan->channel);
 		}
 	} else {
 		ret = sx9310_put_event_channel(data, chan->channel);
-		if (ret < 0)
+		if (ret)
 			goto out_unlock;
 		if (!data->chan_event) {
 			ret = sx9310_disable_irq(data, eventirq);
-			if (ret < 0)
+			if (ret)
 				sx9310_get_event_channel(data, chan->channel);
 		}
 	}
@@ -671,7 +671,7 @@ static int sx9310_set_trigger_state(struct iio_trigger *trig, bool state)
 		ret = sx9310_enable_irq(data, SX9310_CONVDONE_IRQ);
 	else if (!data->chan_read)
 		ret = sx9310_disable_irq(data, SX9310_CONVDONE_IRQ);
-	if (ret < 0)
+	if (ret)
 		goto out;
 
 	data->trigger_enabled = state;
@@ -700,7 +700,7 @@ static irqreturn_t sx9310_trigger_handler(int irq, void *private)
 			 indio_dev->masklength) {
 		ret = sx9310_read_prox_data(data, &indio_dev->channels[bit],
 					    &val);
-		if (ret < 0)
+		if (ret)
 			goto out;
 
 		data->buffer.channels[i++] = val;
@@ -807,13 +807,13 @@ static int sx9310_init_compensation(struct iio_dev *indio_dev)
 	unsigned int ctrl0;
 
 	ret = regmap_read(data->regmap, SX9310_REG_PROX_CTRL0, &ctrl0);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	/* run the compensation phase on all channels */
 	ret = regmap_write(data->regmap, SX9310_REG_PROX_CTRL0,
 			   ctrl0 | SX9310_REG_PROX_CTRL0_SENSOREN_MASK);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	ret = regmap_read_poll_timeout(data->regmap, SX9310_REG_STAT1, val,
@@ -839,21 +839,21 @@ static int sx9310_init_device(struct iio_dev *indio_dev)
 	unsigned int i, val;
 
 	ret = regmap_write(data->regmap, SX9310_REG_RESET, SX9310_SOFT_RESET);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	usleep_range(1000, 2000); /* power-up time is ~1ms. */
 
 	/* Clear reset interrupt state by reading SX9310_REG_IRQ_SRC. */
 	ret = regmap_read(data->regmap, SX9310_REG_IRQ_SRC, &val);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	/* Program some sane defaults. */
 	for (i = 0; i < ARRAY_SIZE(sx9310_default_regs); i++) {
 		initval = &sx9310_default_regs[i];
 		ret = regmap_write(data->regmap, initval->reg, initval->def);
-		if (ret < 0)
+		if (ret)
 			return ret;
 	}
 
@@ -907,14 +907,14 @@ static int sx9310_probe(struct i2c_client *client)
 		return PTR_ERR(data->regmap);
 
 	ret = regmap_read(data->regmap, SX9310_REG_WHOAMI, &data->whoami);
-	if (ret < 0) {
+	if (ret) {
 		dev_err(&client->dev, "error in reading WHOAMI register: %d",
 			ret);
 		return ret;
 	}
 
 	ret = sx9310_set_indio_dev_name(&client->dev, indio_dev, data->whoami);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	ACPI_COMPANION_SET(&indio_dev->dev, ACPI_COMPANION(&client->dev));
@@ -926,7 +926,7 @@ static int sx9310_probe(struct i2c_client *client)
 	i2c_set_clientdata(client, indio_dev);
 
 	ret = sx9310_init_device(indio_dev);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	if (client->irq) {
@@ -935,7 +935,7 @@ static int sx9310_probe(struct i2c_client *client)
 						sx9310_irq_thread_handler,
 						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
 						"sx9310_event", indio_dev);
-		if (ret < 0)
+		if (ret)
 			return ret;
 
 		data->trig =
@@ -957,7 +957,7 @@ static int sx9310_probe(struct i2c_client *client)
 					      iio_pollfunc_store_time,
 					      sx9310_trigger_handler,
 					      &sx9310_buffer_setup_ops);
-	if (ret < 0)
+	if (ret)
 		return ret;
 
 	return devm_iio_device_register(&client->dev, indio_dev);
-- 
2.28.0.163.g6104cc2f0b6-goog

[PATCH v3 11/15] iio: sx9310: Use variable to hold &client->dev

[Daniel Campello]

Improves readability by storing &client->dev in a local variable.

Signed-off-by: Daniel Campello <campello@chromium.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---

Changes in v3: None
Changes in v2:
 - Added '\n' to dev_err()

 drivers/iio/proximity/sx9310.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index 6c5425c6fe0d03..feb5bbff56f447 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -890,11 +890,12 @@ static int sx9310_set_indio_dev_name(struct device *dev,
 static int sx9310_probe(struct i2c_client *client)
 {
 	int ret;
+	struct device *dev = &client->dev;
 	struct iio_dev *indio_dev;
 	struct sx9310_data *data;
 
-	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
-	if (indio_dev == NULL)
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
 		return -ENOMEM;
 
 	data = iio_priv(indio_dev);
@@ -908,17 +909,16 @@ static int sx9310_probe(struct i2c_client *client)
 
 	ret = regmap_read(data->regmap, SX9310_REG_WHOAMI, &data->whoami);
 	if (ret) {
-		dev_err(&client->dev, "error in reading WHOAMI register: %d",
-			ret);
+		dev_err(dev, "error in reading WHOAMI register: %d\n", ret);
 		return ret;
 	}
 
-	ret = sx9310_set_indio_dev_name(&client->dev, indio_dev, data->whoami);
+	ret = sx9310_set_indio_dev_name(dev, indio_dev, data->whoami);
 	if (ret)
 		return ret;
 
-	ACPI_COMPANION_SET(&indio_dev->dev, ACPI_COMPANION(&client->dev));
-	indio_dev->dev.parent = &client->dev;
+	ACPI_COMPANION_SET(&indio_dev->dev, ACPI_COMPANION(dev));
+	indio_dev->dev.parent = dev;
 	indio_dev->channels = sx9310_channels;
 	indio_dev->num_channels = ARRAY_SIZE(sx9310_channels);
 	indio_dev->info = &sx9310_info;
@@ -930,7 +930,7 @@ static int sx9310_probe(struct i2c_client *client)
 		return ret;
 
 	if (client->irq) {
-		ret = devm_request_threaded_irq(&client->dev, client->irq,
+		ret = devm_request_threaded_irq(dev, client->irq,
 						sx9310_irq_handler,
 						sx9310_irq_thread_handler,
 						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
@@ -938,29 +938,29 @@ static int sx9310_probe(struct i2c_client *client)
 		if (ret)
 			return ret;
 
-		data->trig =
-			devm_iio_trigger_alloc(&client->dev, "%s-dev%d",
-					       indio_dev->name, indio_dev->id);
+		data->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
+						    indio_dev->name,
+						    indio_dev->id);
 		if (!data->trig)
 			return -ENOMEM;
 
-		data->trig->dev.parent = &client->dev;
+		data->trig->dev.parent = dev;
 		data->trig->ops = &sx9310_trigger_ops;
 		iio_trigger_set_drvdata(data->trig, indio_dev);
 
-		ret = devm_iio_trigger_register(&client->dev, data->trig);
+		ret = devm_iio_trigger_register(dev, data->trig);
 		if (ret)
 			return ret;
 	}
 
-	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev,
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
 					      iio_pollfunc_store_time,
 					      sx9310_trigger_handler,
 					      &sx9310_buffer_setup_ops);
 	if (ret)
 		return ret;
 
-	return devm_iio_device_register(&client->dev, indio_dev);
+	return devm_iio_device_register(dev, indio_dev);
 }
 
 static int __maybe_unused sx9310_suspend(struct device *dev)
-- 
2.28.0.163.g6104cc2f0b6-goog

[PATCH v3 12/15] iio: sx9310: Miscellaneous format fixes

[Daniel Campello]

Miscellaneous format fixes throughout the whole file.

Signed-off-by: Daniel Campello <campello@chromium.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
---

Changes in v3: None
Changes in v2: None

 drivers/iio/proximity/sx9310.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index feb5bbff56f447..886fcbd52703c5 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -91,28 +91,21 @@
 #define   SX9310_REG_SAR_CTRL2_SAROFFSET_DEFAULT	0x3c
 
 #define SX9310_REG_SENSOR_SEL				0x30
-
 #define SX9310_REG_USE_MSB				0x31
 #define SX9310_REG_USE_LSB				0x32
-
 #define SX9310_REG_AVG_MSB				0x33
 #define SX9310_REG_AVG_LSB				0x34
-
 #define SX9310_REG_DIFF_MSB				0x35
 #define SX9310_REG_DIFF_LSB				0x36
-
 #define SX9310_REG_OFFSET_MSB				0x37
 #define SX9310_REG_OFFSET_LSB				0x38
-
 #define SX9310_REG_SAR_MSB				0x39
 #define SX9310_REG_SAR_LSB				0x3a
-
 #define SX9310_REG_I2C_ADDR				0x40
 #define SX9310_REG_PAUSE				0x41
 #define SX9310_REG_WHOAMI				0x42
 #define   SX9310_WHOAMI_VALUE				0x01
 #define   SX9311_WHOAMI_VALUE				0x02
-
 #define SX9310_REG_RESET				0x7f
 #define   SX9310_SOFT_RESET				0xde
 
@@ -411,7 +404,7 @@ static int sx9310_read_proximity(struct sx9310_data *data,
 		goto out_disable_irq;
 
 	*val = sign_extend32(be16_to_cpu(rawval),
-			     (chan->address == SX9310_REG_DIFF_MSB ? 11 : 15));
+			     chan->address == SX9310_REG_DIFF_MSB ? 11 : 15);
 
 	ret = sx9310_disable_irq(data, SX9310_CONVDONE_IRQ);
 	if (ret)
@@ -438,8 +431,9 @@ static int sx9310_read_proximity(struct sx9310_data *data,
 static int sx9310_read_samp_freq(struct sx9310_data *data, int *val, int *val2)
 {
 	unsigned int regval;
-	int ret = regmap_read(data->regmap, SX9310_REG_PROX_CTRL0, &regval);
+	int ret;
 
+	ret = regmap_read(data->regmap, SX9310_REG_PROX_CTRL0, &regval);
 	if (ret)
 		return ret;
 
@@ -524,10 +518,9 @@ static irqreturn_t sx9310_irq_handler(int irq, void *private)
 		iio_trigger_poll(data->trig);
 
 	/*
-	 * Even if no event is enabled, we need to wake the thread to
-	 * clear the interrupt state by reading SX9310_REG_IRQ_SRC.  It
-	 * is not possible to do that here because regmap_read takes a
-	 * mutex.
+	 * Even if no event is enabled, we need to wake the thread to clear the
+	 * interrupt state by reading SX9310_REG_IRQ_SRC.
+	 * It is not possible to do that here because regmap_read takes a mutex.
 	 */
 	return IRQ_WAKE_THREAD;
 }
@@ -644,7 +637,7 @@ static int sx9310_write_event_config(struct iio_dev *indio_dev,
 
 static struct attribute *sx9310_attributes[] = {
 	&iio_dev_attr_sampling_frequency_available.dev_attr.attr,
-	NULL,
+	NULL
 };
 
 static const struct attribute_group sx9310_attribute_group = {
@@ -975,7 +968,6 @@ static int __maybe_unused sx9310_suspend(struct device *dev)
 	mutex_lock(&data->mutex);
 	ret = regmap_read(data->regmap, SX9310_REG_PROX_CTRL0,
 			  &data->suspend_ctrl0);
-
 	if (ret)
 		goto out;
 
@@ -1021,21 +1013,21 @@ static const struct dev_pm_ops sx9310_pm_ops = {
 static const struct acpi_device_id sx9310_acpi_match[] = {
 	{ "STH9310", SX9310_WHOAMI_VALUE },
 	{ "STH9311", SX9311_WHOAMI_VALUE },
-	{},
+	{}
 };
 MODULE_DEVICE_TABLE(acpi, sx9310_acpi_match);
 
 static const struct of_device_id sx9310_of_match[] = {
 	{ .compatible = "semtech,sx9310", (void *)SX9310_WHOAMI_VALUE },
 	{ .compatible = "semtech,sx9311", (void *)SX9311_WHOAMI_VALUE },
-	{},
+	{}
 };
 MODULE_DEVICE_TABLE(of, sx9310_of_match);
 
 static const struct i2c_device_id sx9310_id[] = {
 	{ "sx9310", SX9310_WHOAMI_VALUE },
 	{ "sx9311", SX9311_WHOAMI_VALUE },
-	{},
+	{}
 };
 MODULE_DEVICE_TABLE(i2c, sx9310_id);
 
-- 
2.28.0.163.g6104cc2f0b6-goog

[PATCH v3 13/15] iio: sx9310: Drop channel_users[]

[Daniel Campello]

From: Stephen Boyd <swboyd@chromium.org>

This struct member isn't used. Drop it.

Fixes: 72ad02b15d63 ("iio: Add SEMTECH SX9310/9311 sensor driver")
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Daniel Campello <campello@chromium.org>
Signed-off-by: Daniel Campello <campello@chromium.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---

Changes in v3: None
Changes in v2: None

 drivers/iio/proximity/sx9310.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index 886fcbd52703c5..aa21f6f1a58e3c 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -136,7 +136,6 @@ struct sx9310_data {
 	struct completion completion;
 	unsigned long chan_read;
 	unsigned long chan_event;
-	int channel_users[SX9310_NUM_CHANNELS];
 	unsigned int whoami;
 };
 
-- 
2.28.0.163.g6104cc2f0b6-goog

[PATCH v3 14/15] iio: sx9310: Enable vdd and svdd regulators at probe

[Daniel Campello]

From: Stephen Boyd <swboyd@chromium.org>

Enable the main power supply (vdd) and digital IO power supply (svdd)
during probe so that the i2c communication and device works properly on
boards that aggressively power gate these supplies.

Signed-off-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Daniel Campello <campello@chromium.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---

Changes in v3: None
Changes in v2: None

 drivers/iio/proximity/sx9310.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index aa21f6f1a58e3c..a20cd6a4dad729 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -19,6 +19,7 @@
 #include <linux/module.h>
 #include <linux/pm.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 
 #include <linux/iio/buffer.h>
@@ -120,6 +121,7 @@ struct sx9310_data {
 	struct i2c_client *client;
 	struct iio_trigger *trig;
 	struct regmap *regmap;
+	struct regulator_bulk_data supplies[2];
 	/*
 	 * Last reading of the proximity status for each channel.
 	 * We only send an event to user space when this changes.
@@ -879,6 +881,13 @@ static int sx9310_set_indio_dev_name(struct device *dev,
 	return 0;
 }
 
+static void sx9310_regulator_disable(void *_data)
+{
+	struct sx9310_data *data = _data;
+
+	regulator_bulk_disable(ARRAY_SIZE(data->supplies), data->supplies);
+}
+
 static int sx9310_probe(struct i2c_client *client)
 {
 	int ret;
@@ -892,6 +901,8 @@ static int sx9310_probe(struct i2c_client *client)
 
 	data = iio_priv(indio_dev);
 	data->client = client;
+	data->supplies[0].supply = "vdd";
+	data->supplies[1].supply = "svdd";
 	mutex_init(&data->mutex);
 	init_completion(&data->completion);
 
@@ -899,6 +910,21 @@ static int sx9310_probe(struct i2c_client *client)
 	if (IS_ERR(data->regmap))
 		return PTR_ERR(data->regmap);
 
+	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->supplies),
+				      data->supplies);
+	if (ret)
+		return ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(data->supplies), data->supplies);
+	if (ret)
+		return ret;
+	/* Must wait for Tpor time after initial power up */
+	usleep_range(1000, 1100);
+
+	ret = devm_add_action_or_reset(dev, sx9310_regulator_disable, data);
+	if (ret)
+		return ret;
+
 	ret = regmap_read(data->regmap, SX9310_REG_WHOAMI, &data->whoami);
 	if (ret) {
 		dev_err(dev, "error in reading WHOAMI register: %d\n", ret);
-- 
2.28.0.163.g6104cc2f0b6-goog

[PATCH v3 15/15] iio: sx9310: Use irq trigger flags from firmware

[Daniel Campello]

From: Stephen Boyd <swboyd@chromium.org>

We shouldn't need to set default irq trigger flags here as the firmware
should have properly indicated the trigger type, i.e. level low, in the
DT or ACPI tables.

Signed-off-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Daniel Campello <campello@chromium.org>
---

Changes in v3:
 - Added irq trigger flags commit to the series.

Changes in v2: None

 drivers/iio/proximity/sx9310.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index a20cd6a4dad729..c41fa7f6558e3f 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -951,7 +951,7 @@ static int sx9310_probe(struct i2c_client *client)
 		ret = devm_request_threaded_irq(dev, client->irq,
 						sx9310_irq_handler,
 						sx9310_irq_thread_handler,
-						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+						IRQF_ONESHOT,
 						"sx9310_event", indio_dev);
 		if (ret)
 			return ret;
-- 
2.28.0.163.g6104cc2f0b6-goog

Re: [PATCH v3 15/15] iio: sx9310: Use irq trigger flags from firmware

[Andy Shevchenko]

On Fri, Jul 31, 2020 at 7:49 PM Daniel Campello <campello@chromium.org> wrote:
>
> From: Stephen Boyd <swboyd@chromium.org>
>
> We shouldn't need to set default irq trigger flags here as the firmware
> should have properly indicated the trigger type, i.e. level low, in the
> DT or ACPI tables.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Daniel Campello <campello@chromium.org>
> ---
>
> Changes in v3:
>  - Added irq trigger flags commit to the series.
>
> Changes in v2: None
>
>  drivers/iio/proximity/sx9310.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
> index a20cd6a4dad729..c41fa7f6558e3f 100644
> --- a/drivers/iio/proximity/sx9310.c
> +++ b/drivers/iio/proximity/sx9310.c
> @@ -951,7 +951,7 @@ static int sx9310_probe(struct i2c_client *client)
>                 ret = devm_request_threaded_irq(dev, client->irq,
>                                                 sx9310_irq_handler,
>                                                 sx9310_irq_thread_handler,
> -                                               IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> +                                               IRQF_ONESHOT,
>                                                 "sx9310_event", indio_dev);
>                 if (ret)
>                         return ret;
> --
> 2.28.0.163.g6104cc2f0b6-goog
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 15/15] iio: sx9310: Use irq trigger flags from firmware

[Jonathan Cameron]

On Fri, 31 Jul 2020 22:46:55 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Jul 31, 2020 at 7:49 PM Daniel Campello <campello@chromium.org> wrote:
> >
> > From: Stephen Boyd <swboyd@chromium.org>
> >
> > We shouldn't need to set default irq trigger flags here as the firmware
> > should have properly indicated the trigger type, i.e. level low, in the
> > DT or ACPI tables.  
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Other than the two patches I replied to the rest look good to me.

Thanks,

Jonathan
 
> 
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > Signed-off-by: Daniel Campello <campello@chromium.org>
> > ---
> >
> > Changes in v3:
> >  - Added irq trigger flags commit to the series.
> >
> > Changes in v2: None
> >
> >  drivers/iio/proximity/sx9310.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
> > index a20cd6a4dad729..c41fa7f6558e3f 100644
> > --- a/drivers/iio/proximity/sx9310.c
> > +++ b/drivers/iio/proximity/sx9310.c
> > @@ -951,7 +951,7 @@ static int sx9310_probe(struct i2c_client *client)
> >                 ret = devm_request_threaded_irq(dev, client->irq,
> >                                                 sx9310_irq_handler,
> >                                                 sx9310_irq_thread_handler,
> > -                                               IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> > +                                               IRQF_ONESHOT,
> >                                                 "sx9310_event", indio_dev);
> >                 if (ret)
> >                         return ret;
> > --
> > 2.28.0.163.g6104cc2f0b6-goog
> >  
> 
>