* [PATCH 0/3] Add ADE9078 Driver
@ 2022-02-17 13:51 chegbeli
2022-02-17 13:51 ` [PATCH 1/3] iio: add meter subdirectory chegbeli
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: chegbeli @ 2022-02-17 13:51 UTC (permalink / raw)
To: jic23, robh+dt, linux-iio, devicetree, linux-kernel; +Cc: chegbeli
The ADE9078 is a highly accurate, fully integrated energy metering
device. It allows the monitoring of three independent phases
simultaneously, by using seven high performances ADCs and a flexible
DSP core for a fixed fundamental signal frequency.
Datasheet:
www.analog.com/media/en/technical-documentation/data-sheets/ADE9078.pdf
NOTE:
Given the unique nature of this type of ADC a new sub-folder has been
created in the driver/iio/ section. This folder would serve as a
location for energy meters.
This series of patches provide an initial implementation of the
driver with access from the userspace to the:
- For each active phase from the device tree:
- Current (read raw, read scale, set hwgain)
- Voltage (read raw, read scale, set hwgain)
- Current RMS (read raw, read scale, set offset)
- Voltage RMS (read raw, read scale, set offset)
- Active Power (read raw, read scale, set hwgain, set offset)
- Reactive Power (read raw, read scale, set offset)
- Apparent Power (read raw, read scale)
- Fundamental Active Power (read raw, read scale, set offset)
- Apparent Power (read raw, read scale)
- Event control for zero-crossing events on Voltage and Current
- Buffer implementation for read out of the internal FIFO
chegbeli (3):
iio: add meter subdirectory
dt-bindings: iio: add ADE9078
iio: meter: add ADE9078 driver
.../bindings/iio/meter/adi,ade9078.yaml | 153 ++
MAINTAINERS | 8 +
drivers/iio/Kconfig | 1 +
drivers/iio/Makefile | 1 +
drivers/iio/meter/Kconfig | 22 +
drivers/iio/meter/Makefile | 7 +
drivers/iio/meter/ade9078.c | 1666 +++++++++++++++++
include/dt-bindings/iio/meter/adi,ade9078.h | 21 +
8 files changed, 1879 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/meter/adi,ade9078.yaml
create mode 100644 drivers/iio/meter/Kconfig
create mode 100644 drivers/iio/meter/Makefile
create mode 100644 drivers/iio/meter/ade9078.c
create mode 100644 include/dt-bindings/iio/meter/adi,ade9078.h
--
2.34.1
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/3] iio: add meter subdirectory 2022-02-17 13:51 [PATCH 0/3] Add ADE9078 Driver chegbeli @ 2022-02-17 13:51 ` chegbeli 2022-02-17 15:33 ` Krzysztof Kozlowski 2022-02-17 18:34 ` Jonathan Cameron 2022-02-17 13:51 ` [PATCH 2/3] dt-bindings: iio: add ADE9078 chegbeli 2022-02-17 13:51 ` [PATCH 3/3] iio: meter: add ADE9078 driver chegbeli 2 siblings, 2 replies; 11+ messages in thread From: chegbeli @ 2022-02-17 13:51 UTC (permalink / raw) To: jic23, robh+dt, linux-iio, devicetree, linux-kernel; +Cc: chegbeli For IIO devices which are designed to measure and process power readings. Signed-off-by: chegbeli <ciprian.hegbeli@analog.com> --- drivers/iio/Kconfig | 1 + drivers/iio/Makefile | 1 + drivers/iio/meter/Kconfig | 9 +++++++++ drivers/iio/meter/Makefile | 5 +++++ 4 files changed, 16 insertions(+) create mode 100644 drivers/iio/meter/Kconfig create mode 100644 drivers/iio/meter/Makefile diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig index b190846c3dc2..892c7f575416 100644 --- a/drivers/iio/Kconfig +++ b/drivers/iio/Kconfig @@ -86,6 +86,7 @@ source "drivers/iio/humidity/Kconfig" source "drivers/iio/imu/Kconfig" source "drivers/iio/light/Kconfig" source "drivers/iio/magnetometer/Kconfig" +source "drivers/iio/meter/Kconfig" source "drivers/iio/multiplexer/Kconfig" source "drivers/iio/orientation/Kconfig" source "drivers/iio/test/Kconfig" diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile index 3be08cdadd7e..c8afc053fccf 100644 --- a/drivers/iio/Makefile +++ b/drivers/iio/Makefile @@ -32,6 +32,7 @@ obj-y += humidity/ obj-y += imu/ obj-y += light/ obj-y += magnetometer/ +obj-y += meter/ obj-y += multiplexer/ obj-y += orientation/ obj-y += position/ diff --git a/drivers/iio/meter/Kconfig b/drivers/iio/meter/Kconfig new file mode 100644 index 000000000000..d1c91eed5283 --- /dev/null +++ b/drivers/iio/meter/Kconfig @@ -0,0 +1,9 @@ +# SPDX-License-Identifier: GPL-2.0 +# +# Energy meter drivers +# +# When adding new entries keep the list in alphabetical order + +menu "Active energy metering" + +endmenu diff --git a/drivers/iio/meter/Makefile b/drivers/iio/meter/Makefile new file mode 100644 index 000000000000..c27cba44fc0b --- /dev/null +++ b/drivers/iio/meter/Makefile @@ -0,0 +1,5 @@ +# SPDX-License-Identifier: GPL-2.0 +# +# Makefile for enrgy metering drivers +# +# When adding new entries keep the list in alphabetical order -- 2.34.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] iio: add meter subdirectory 2022-02-17 13:51 ` [PATCH 1/3] iio: add meter subdirectory chegbeli @ 2022-02-17 15:33 ` Krzysztof Kozlowski 2022-02-17 18:34 ` Jonathan Cameron 1 sibling, 0 replies; 11+ messages in thread From: Krzysztof Kozlowski @ 2022-02-17 15:33 UTC (permalink / raw) To: chegbeli, jic23, robh+dt, linux-iio, devicetree, linux-kernel On 17/02/2022 14:51, chegbeli wrote: > For IIO devices which are designed to measure and process power > readings. > > Signed-off-by: chegbeli <ciprian.hegbeli@analog.com> Here and in other commits - please use full name in "From:" and Signed-off-by. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] iio: add meter subdirectory 2022-02-17 13:51 ` [PATCH 1/3] iio: add meter subdirectory chegbeli 2022-02-17 15:33 ` Krzysztof Kozlowski @ 2022-02-17 18:34 ` Jonathan Cameron 1 sibling, 0 replies; 11+ messages in thread From: Jonathan Cameron @ 2022-02-17 18:34 UTC (permalink / raw) To: chegbeli; +Cc: jic23, robh+dt, linux-iio, devicetree, linux-kernel On Thu, 17 Feb 2022 15:51:38 +0200 chegbeli <ciprian.hegbeli@analog.com> wrote: > For IIO devices which are designed to measure and process power > readings. > > Signed-off-by: chegbeli <ciprian.hegbeli@analog.com> No need for separate patch - just squash this with the driver patch. > --- > drivers/iio/Kconfig | 1 + > drivers/iio/Makefile | 1 + > drivers/iio/meter/Kconfig | 9 +++++++++ > drivers/iio/meter/Makefile | 5 +++++ > 4 files changed, 16 insertions(+) > create mode 100644 drivers/iio/meter/Kconfig > create mode 100644 drivers/iio/meter/Makefile > > diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig > index b190846c3dc2..892c7f575416 100644 > --- a/drivers/iio/Kconfig > +++ b/drivers/iio/Kconfig > @@ -86,6 +86,7 @@ source "drivers/iio/humidity/Kconfig" > source "drivers/iio/imu/Kconfig" > source "drivers/iio/light/Kconfig" > source "drivers/iio/magnetometer/Kconfig" > +source "drivers/iio/meter/Kconfig" > source "drivers/iio/multiplexer/Kconfig" > source "drivers/iio/orientation/Kconfig" > source "drivers/iio/test/Kconfig" > diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile > index 3be08cdadd7e..c8afc053fccf 100644 > --- a/drivers/iio/Makefile > +++ b/drivers/iio/Makefile > @@ -32,6 +32,7 @@ obj-y += humidity/ > obj-y += imu/ > obj-y += light/ > obj-y += magnetometer/ > +obj-y += meter/ > obj-y += multiplexer/ > obj-y += orientation/ > obj-y += position/ > diff --git a/drivers/iio/meter/Kconfig b/drivers/iio/meter/Kconfig > new file mode 100644 > index 000000000000..d1c91eed5283 > --- /dev/null > +++ b/drivers/iio/meter/Kconfig > @@ -0,0 +1,9 @@ > +# SPDX-License-Identifier: GPL-2.0 > +# > +# Energy meter drivers > +# > +# When adding new entries keep the list in alphabetical order > + > +menu "Active energy metering" > + > +endmenu > diff --git a/drivers/iio/meter/Makefile b/drivers/iio/meter/Makefile > new file mode 100644 > index 000000000000..c27cba44fc0b > --- /dev/null > +++ b/drivers/iio/meter/Makefile > @@ -0,0 +1,5 @@ > +# SPDX-License-Identifier: GPL-2.0 > +# > +# Makefile for enrgy metering drivers > +# > +# When adding new entries keep the list in alphabetical order ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3] dt-bindings: iio: add ADE9078 2022-02-17 13:51 [PATCH 0/3] Add ADE9078 Driver chegbeli 2022-02-17 13:51 ` [PATCH 1/3] iio: add meter subdirectory chegbeli @ 2022-02-17 13:51 ` chegbeli 2022-02-17 15:44 ` Krzysztof Kozlowski 2022-02-21 2:36 ` Rob Herring 2022-02-17 13:51 ` [PATCH 3/3] iio: meter: add ADE9078 driver chegbeli 2 siblings, 2 replies; 11+ messages in thread From: chegbeli @ 2022-02-17 13:51 UTC (permalink / raw) To: jic23, robh+dt, linux-iio, devicetree, linux-kernel; +Cc: chegbeli Added device tree bindings for the ADE9078 Signed-off-by: chegbeli <ciprian.hegbeli@analog.com> --- .../bindings/iio/meter/adi,ade9078.yaml | 153 ++++++++++++++++++ include/dt-bindings/iio/meter/adi,ade9078.h | 21 +++ 2 files changed, 174 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/meter/adi,ade9078.yaml create mode 100644 include/dt-bindings/iio/meter/adi,ade9078.h diff --git a/Documentation/devicetree/bindings/iio/meter/adi,ade9078.yaml b/Documentation/devicetree/bindings/iio/meter/adi,ade9078.yaml new file mode 100644 index 000000000000..e27d52e06e32 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/meter/adi,ade9078.yaml @@ -0,0 +1,153 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +# Copyright 2021 Analog Devices Inc. +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/bindings/iio/addac/adi,ade9078.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Analog Devices ADE9078 High Performance, Polyphase Energy Metering driver + +mainterners: + -Ciprian Hegbeli <ciprian.hegbeli@analog.com> + +description: | + The ADE9078 1 is a highly accurate, fully integrated energy + metering device. Interfacing with both current transformer + (CT) and Rogowski coil sensors, the ADE9078 enables users to + develop a 3-phase metrology platform, which achieves high + performance for Class 1 up to Class 0.2 meters. + +properties: + compatible: + enum: + - adi,ade9078 + + reg: + maxItems: 1 + + '#address-cells': + const: 1 + + '#size-cells': + const: 0 + + spi-max-frequency: + maximum: 1000000 + + interrupts: + maxItems: 2 + + reset-gpios: + description: | + Must be the device tree identifier of the RESET pin. As the line is + active low, it should be marked GPIO_ACTIVE_LOW. + maxItems: 1 + + interrupt-names: + description: | + Names to be attributed to the interrupts of the device. Should be "irq0" + or "irq1" + + adi,wf-cap-sel: + description: | + This bit selects whether the waveform buffer is filled with resampled + data or fixed data rate data + 0 - WF_RESAMPLED_DATA + 1 - WF_FIXED_DATA_RATE + maxItems: 1 + minimum: 0 + maximum: 1 + + adi,wf-mode: + description: | + Fixed data rate waveforms filling and trigger based modes. + 0 - WFB_FULL_MODE (Stop when waveform buffer is full) + 1 - WFB_EN_TRIG_MODE (Continuous fill—stop only on enabled trigger events) + 2 - WFB_CENTER_EN_TRIG_MODE (Continuous filling—center capture around enabled trigger events) + 3 - WFB_SVAE_EN_TRIG_MODE (Continuous fill—save event address of enabled trigger events) + maxItems: 1 + minimum: 0 + maximum: 3 + + adi,wf-src: + description: | + Waveform buffer source and DREADY, data ready update rate, selection. + 0 - WFB_SRC_SINC4 (Sinc4 output, at 16 kSPS) + 1 - Reserved + 2 - WFB_SRC_SINC4_IIR_LPF (Sinc4 + IIR LPF output, at 4 kSPS) + 3 - WFB_SRC_DSP (Current and voltage channel waveform samples,processed by the DSP + (xI_PCF, xV_PCF) at 4 kSPS) + maxItems: 1 + minimum: 0 + maximum: 3 + + adi,wf-in-en: + description: | + This setting determines whether the IN waveform samples are read out of + the waveform buffer through SPI. + 0 - WFB_IN_DISABLE + 1 - WFB_IN_EN + maxItems: 1 + minimum: 0 + maximum: 1 + + required: + - compatible + - reg + - reset-gpios + - interrupts + - interrupt-names + - adi,wf-cap-sel + - adi,wf-mode + - adi,wf-src + - adi,wf-in-en + +patternProperties: + "^phase@[0-3]$": + type: object + description: | + Represents the external phases which are externally connected. Each phase + has a current, voltage and power component + + properties: + reg: + description: | + The phase represented by a number + 0 - Phase A + 1 - unused + 2 - Phase B + 3 - unused + 4 - Phase C + maxItems: 1 + minimum: 0 + maximum: 4 + + required: + - reg + +examples: + - | + ade9078@0 { + compatible = "adi,ade9078"; + reg = <0>; + spi-max-frequency = <7000000>; + + #address-cells = <1>; + #size-cells = <0>; + reset-gpios = <&gpio 4 GPIO_ACTIVE_LOW>; + interrupts = <2 IRQ_TYPE_EDGE_FALLING>, <3 IRQ_TYPE_EDGE_FALLING>; + interrupt-names = "irq0", "irq1"; + interrupt-parent = <&gpio>; + + adi,wf-cap-sel = <WF_FIXED_DATA_RATE>; + adi,wf-mode = <WFB_FULL_MODE>; + adi,wf-src = <WFB_SRC_DSP>; + adi,wf-in-en = <WFB_IN_DISABLE>; + + phase@0 { + reg = <0>; + }; + phase@1 { + reg = <4>; + }; + }; diff --git a/include/dt-bindings/iio/meter/adi,ade9078.h b/include/dt-bindings/iio/meter/adi,ade9078.h new file mode 100644 index 000000000000..85004251a36b --- /dev/null +++ b/include/dt-bindings/iio/meter/adi,ade9078.h @@ -0,0 +1,21 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#ifndef _DT_BINDINGS_ADI_ADE9078_H +#define _DT_BINDINGS_ADI_ADE9078_H + +#define WF_RESAMPLED_DATA 0x0 +#define WF_FIXED_DATA_RATE 0x1 + +#define WFB_FULL_MODE 0x0 +#define WFB_EN_TRIG_MODE 0x1 +#define WFB_CENTER_EN_TRIG_MODE 0x2 +#define WFB_SVAE_EN_TRIG_MODE 0x3 + +#define WFB_SRC_SINC4 0x0 +#define WFB_SRC_SINC4_IIR_LPF 0x2 +#define WFB_SRC_DSP 0x3 + +#define WFB_IN_DISABLE 0x0 +#define WFB_IN_EN 0x1 + +#endif /* _DT_BINDINGS_ADI_ADE9078_H */ -- 2.34.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] dt-bindings: iio: add ADE9078 2022-02-17 13:51 ` [PATCH 2/3] dt-bindings: iio: add ADE9078 chegbeli @ 2022-02-17 15:44 ` Krzysztof Kozlowski 2022-02-21 2:36 ` Rob Herring 1 sibling, 0 replies; 11+ messages in thread From: Krzysztof Kozlowski @ 2022-02-17 15:44 UTC (permalink / raw) To: chegbeli, jic23, robh+dt, linux-iio, devicetree, linux-kernel On 17/02/2022 14:51, chegbeli wrote: > Added device tree bindings for the ADE9078 > > Signed-off-by: chegbeli <ciprian.hegbeli@analog.com> > --- > .../bindings/iio/meter/adi,ade9078.yaml | 153 ++++++++++++++++++ > include/dt-bindings/iio/meter/adi,ade9078.h | 21 +++ > 2 files changed, 174 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/meter/adi,ade9078.yaml > create mode 100644 include/dt-bindings/iio/meter/adi,ade9078.h > > diff --git a/Documentation/devicetree/bindings/iio/meter/adi,ade9078.yaml b/Documentation/devicetree/bindings/iio/meter/adi,ade9078.yaml > new file mode 100644 > index 000000000000..e27d52e06e32 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/meter/adi,ade9078.yaml > @@ -0,0 +1,153 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +# Copyright 2021 Analog Devices Inc. > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/bindings/iio/addac/adi,ade9078.yaml# Did you test your schema with dt_binding_check? This should fail. > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Analog Devices ADE9078 High Performance, Polyphase Energy Metering driver > + > +mainterners: > + -Ciprian Hegbeli <ciprian.hegbeli@analog.com> Space after '-'. > + > +description: | > + The ADE9078 1 is a highly accurate, fully integrated energy > + metering device. Interfacing with both current transformer > + (CT) and Rogowski coil sensors, the ADE9078 enables users to > + develop a 3-phase metrology platform, which achieves high > + performance for Class 1 up to Class 0.2 meters. > + > +properties: > + compatible: > + enum: > + - adi,ade9078 > + > + reg: > + maxItems: 1 > + > + '#address-cells': > + const: 1 > + > + '#size-cells': > + const: 0 > + > + spi-max-frequency: > + maximum: 1000000 > + > + interrupts: > + maxItems: 2 > + > + reset-gpios: > + description: | > + Must be the device tree identifier of the RESET pin. As the line is > + active low, it should be marked GPIO_ACTIVE_LOW. > + maxItems: 1 > + > + interrupt-names: > + description: | > + Names to be attributed to the interrupts of the device. Should be "irq0" > + or "irq1" Skip description but instead list items ( - const: irq0 ...) > + > + adi,wf-cap-sel: > + description: | > + This bit selects whether the waveform buffer is filled with resampled > + data or fixed data rate data > + 0 - WF_RESAMPLED_DATA > + 1 - WF_FIXED_DATA_RATE > + maxItems: 1 > + minimum: 0 > + maximum: 1 1. You need a type definition. 2. This looks like bool. 3. maxItems seems wrong here. 4. Do not describe registers and their bits but a feature of the device. This applies to fields below as well. > + > + adi,wf-mode: > + description: | > + Fixed data rate waveforms filling and trigger based modes. > + 0 - WFB_FULL_MODE (Stop when waveform buffer is full) > + 1 - WFB_EN_TRIG_MODE (Continuous fill—stop only on enabled trigger events) > + 2 - WFB_CENTER_EN_TRIG_MODE (Continuous filling—center capture around enabled trigger events) > + 3 - WFB_SVAE_EN_TRIG_MODE (Continuous fill—save event address of enabled trigger events) > + maxItems: 1 > + minimum: 0 > + maximum: 3 Everything above + this looks like an enum, so use enum, instead of min/max. > + > + adi,wf-src: > + description: | > + Waveform buffer source and DREADY, data ready update rate, selection. > + 0 - WFB_SRC_SINC4 (Sinc4 output, at 16 kSPS) > + 1 - Reserved > + 2 - WFB_SRC_SINC4_IIR_LPF (Sinc4 + IIR LPF output, at 4 kSPS) > + 3 - WFB_SRC_DSP (Current and voltage channel waveform samples,processed by the DSP > + (xI_PCF, xV_PCF) at 4 kSPS) > + maxItems: 1 > + minimum: 0 > + maximum: 3 > + > + adi,wf-in-en: > + description: | > + This setting determines whether the IN waveform samples are read out of > + the waveform buffer through SPI. > + 0 - WFB_IN_DISABLE > + 1 - WFB_IN_EN > + maxItems: 1 > + minimum: 0 > + maximum: 1 Also bool. > + > + required: > + - compatible > + - reg > + - reset-gpios > + - interrupts > + - interrupt-names > + - adi,wf-cap-sel > + - adi,wf-mode > + - adi,wf-src > + - adi,wf-in-en > + > +patternProperties: > + "^phase@[0-3]$": > + type: object > + description: | > + Represents the external phases which are externally connected. Each phase > + has a current, voltage and power component > + > + properties: > + reg: > + description: | > + The phase represented by a number > + 0 - Phase A > + 1 - unused > + 2 - Phase B > + 3 - unused > + 4 - Phase C > + maxItems: 1 > + minimum: 0 > + maximum: 4 > + > + required: > + - reg > + > +examples: > + - | > + ade9078@0 { Generic node name. Please pick something appropriate. Maybe "meter"? > + compatible = "adi,ade9078"; You have entirely broken indentation here. Use four spaces for DTS example. > + reg = <0>; > + spi-max-frequency = <7000000>; > + > + #address-cells = <1>; > + #size-cells = <0>; > + reset-gpios = <&gpio 4 GPIO_ACTIVE_LOW>; > + interrupts = <2 IRQ_TYPE_EDGE_FALLING>, <3 IRQ_TYPE_EDGE_FALLING>; You clearly did not test it... this won't work without headers. > + interrupt-names = "irq0", "irq1"; > + interrupt-parent = <&gpio>; Best regards, Krzysztof ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] dt-bindings: iio: add ADE9078 2022-02-17 13:51 ` [PATCH 2/3] dt-bindings: iio: add ADE9078 chegbeli 2022-02-17 15:44 ` Krzysztof Kozlowski @ 2022-02-21 2:36 ` Rob Herring 1 sibling, 0 replies; 11+ messages in thread From: Rob Herring @ 2022-02-21 2:36 UTC (permalink / raw) To: chegbeli; +Cc: devicetree, linux-iio, robh+dt, linux-kernel, jic23 On Thu, 17 Feb 2022 15:51:39 +0200, chegbeli wrote: > Added device tree bindings for the ADE9078 > > Signed-off-by: chegbeli <ciprian.hegbeli@analog.com> > --- > .../bindings/iio/meter/adi,ade9078.yaml | 153 ++++++++++++++++++ > include/dt-bindings/iio/meter/adi,ade9078.h | 21 +++ > 2 files changed, 174 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/meter/adi,ade9078.yaml > create mode 100644 include/dt-bindings/iio/meter/adi,ade9078.h > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: ./Documentation/devicetree/bindings/iio/meter/adi,ade9078.yaml:131:1: [error] syntax error: found character '\t' that cannot start any token (syntax) dtschema/dtc warnings/errors: ./Documentation/devicetree/bindings/iio/meter/adi,ade9078.yaml: while scanning a block scalar in "<unicode string>", line 129, column 5 found a tab character where an indentation space is expected in "<unicode string>", line 131, column 1 make[1]: *** Deleting file 'Documentation/devicetree/bindings/iio/meter/adi,ade9078.example.dts' Traceback (most recent call last): File "/usr/local/bin/dt-extract-example", line 46, in <module> binding = yaml.load(open(args.yamlfile, encoding='utf-8').read()) File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/main.py", line 434, in load return constructor.get_single_data() File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 119, in get_single_data node = self.composer.get_single_node() File "_ruamel_yaml.pyx", line 706, in _ruamel_yaml.CParser.get_single_node File "_ruamel_yaml.pyx", line 724, in _ruamel_yaml.CParser._compose_document File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node File "_ruamel_yaml.pyx", line 889, in _ruamel_yaml.CParser._compose_mapping_node File "_ruamel_yaml.pyx", line 773, in _ruamel_yaml.CParser._compose_node File "_ruamel_yaml.pyx", line 848, in _ruamel_yaml.CParser._compose_sequence_node File "_ruamel_yaml.pyx", line 904, in _ruamel_yaml.CParser._parse_next_event ruamel.yaml.scanner.ScannerError: while scanning a block scalar in "<unicode string>", line 129, column 5 found a tab character where an indentation space is expected in "<unicode string>", line 131, column 1 make[1]: *** [Documentation/devicetree/bindings/Makefile:25: Documentation/devicetree/bindings/iio/meter/adi,ade9078.example.dts] Error 1 make[1]: *** Waiting for unfinished jobs.... /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/meter/adi,ade9078.yaml: ignoring, error parsing file make: *** [Makefile:1398: dt_binding_check] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/1594257 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] iio: meter: add ADE9078 driver 2022-02-17 13:51 [PATCH 0/3] Add ADE9078 Driver chegbeli 2022-02-17 13:51 ` [PATCH 1/3] iio: add meter subdirectory chegbeli 2022-02-17 13:51 ` [PATCH 2/3] dt-bindings: iio: add ADE9078 chegbeli @ 2022-02-17 13:51 ` chegbeli 2022-02-17 18:33 ` Jonathan Cameron 2 siblings, 1 reply; 11+ messages in thread From: chegbeli @ 2022-02-17 13:51 UTC (permalink / raw) To: jic23, robh+dt, linux-iio, devicetree, linux-kernel; +Cc: chegbeli The ADE9078 is a highly accurate, fully integrated energy metering device. It allows the monitoring of three independent phases simultaneously, by using seven high performances ADCs and a flexible DSP core for a fixed fundamental signal frequency of either 50Hz or 60Hz. Datasheet: www.analog.com/media/en/technical-documentation/data-sheets/ADE9078.pdf Signed-off-by: chegbeli <ciprian.hegbeli@analog.com> --- MAINTAINERS | 8 + drivers/iio/meter/Kconfig | 13 + drivers/iio/meter/Makefile | 2 + drivers/iio/meter/ade9078.c | 1666 +++++++++++++++++++++++++++++++++++ 4 files changed, 1689 insertions(+) create mode 100644 drivers/iio/meter/ade9078.c diff --git a/MAINTAINERS b/MAINTAINERS index a2c8699e9e41..72fac9f4f837 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1101,6 +1101,14 @@ L: linux-media@vger.kernel.org S: Maintained F: drivers/media/i2c/ad9389b* +ANALOG DEVICES INC ADE9078 DRIVER +M: Ciprian Hegbeli <ciprian.hegbeli@analog.com> +L: linux-iio@vger.kernel.org +S: Supported +W: https://ez.analog.com/linux-software-drivers +F: Documentation/devicetree/bindings/iio/meter/adi,ade9078.yaml +F: drivers/iio/meter/ade9078.c + ANALOG DEVICES INC ADGS1408 DRIVER M: Mircea Caprioru <mircea.caprioru@analog.com> S: Supported diff --git a/drivers/iio/meter/Kconfig b/drivers/iio/meter/Kconfig index d1c91eed5283..b46e93af1052 100644 --- a/drivers/iio/meter/Kconfig +++ b/drivers/iio/meter/Kconfig @@ -6,4 +6,17 @@ menu "Active energy metering" +config ADE9078 + tristate "Analog Devices ADE9078 Poly Phase Multifunction Energy Metering IC Driver" + depends on SPI + select REGMAP_SPI + select IIO_BUFFER + select IIO_KFIFO_BUF + help + Support driver for the ADE9078Poly Phase Multifunction Energy Meter, + select y to build the driver. + + To compile this driver as a module, choose M here: the module + will be called ade9078 + endmenu diff --git a/drivers/iio/meter/Makefile b/drivers/iio/meter/Makefile index c27cba44fc0b..866b7f7dc703 100644 --- a/drivers/iio/meter/Makefile +++ b/drivers/iio/meter/Makefile @@ -3,3 +3,5 @@ # Makefile for enrgy metering drivers # # When adding new entries keep the list in alphabetical order + +obj-$(CONFIG_ADE9078) += ade9078.o diff --git a/drivers/iio/meter/ade9078.c b/drivers/iio/meter/ade9078.c new file mode 100644 index 000000000000..cfcb093e4c27 --- /dev/null +++ b/drivers/iio/meter/ade9078.c @@ -0,0 +1,1666 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * ADE9078 driver + * + * Copyright 2021 Analog Devices Inc. + */ + +#include <asm/unaligned.h> +#include <linux/bitfield.h> +#include <linux/delay.h> +#include <linux/gpio/consumer.h> +#include <linux/iio/iio.h> +#include <linux/iio/buffer.h> +#include <linux/iio/kfifo_buf.h> +#include <linux/iio/events.h> +#include <linux/iio/sysfs.h> +#include <linux/interrupt.h> +#include <linux/module.h> +#include <linux/of_irq.h> +#include <linux/regmap.h> +#include <linux/spi/spi.h> + +/* Address of ADE90XX registers */ +#define ADE9078_REG_AIGAIN 0x000 +#define ADE9078_REG_AVGAIN 0x00B +#define ADE9078_REG_AIRMSOS 0x00C +#define ADE9078_REG_AVRMSOS 0x00D +#define ADE9078_REG_APGAIN 0x00E +#define ADE9078_REG_AWATTOS 0x00F +#define ADE9078_REG_AVAROS 0x010 +#define ADE9078_REG_AFVAROS 0x012 +#define ADE9078_REG_CONFIG0 0x060 +#define ADE9078_REG_DICOEFF 0x072 +#define ADE9078_REG_AI_PCF 0x20A +#define ADE9078_REG_AV_PCF 0x20B +#define ADE9078_REG_AIRMS 0x20C +#define ADE9078_REG_AVRMS 0x20D +#define ADE9078_REG_AWATT 0x210 +#define ADE9078_REG_AVAR 0x211 +#define ADE9078_REG_AVA 0x212 +#define ADE9078_REG_AFVAR 0x214 +#define ADE9078_REG_APF 0x216 +#define ADE9078_REG_BI_PCF 0x22A +#define ADE9078_REG_BV_PCF 0x22B +#define ADE9078_REG_BIRMS 0x22C +#define ADE9078_REG_BVRMS 0x22D +#define ADE9078_REG_CI_PCF 0x24A +#define ADE9078_REG_CV_PCF 0x24B +#define ADE9078_REG_CIRMS 0x24C +#define ADE9078_REG_CVRMS 0x24D +#define ADE9078_REG_AWATT_ACC 0x2E5 +#define ADE9078_REG_STATUS0 0x402 +#define ADE9078_REG_STATUS1 0x403 +#define ADE9078_REG_MASK0 0x405 +#define ADE9078_REG_MASK1 0x406 +#define ADE9078_REG_EVENT_MASK 0x407 +#define ADE9078_REG_VLEVEL 0x40F +#define ADE9078_REG_RUN 0x480 +#define ADE9078_REG_CONFIG1 0x481 +#define ADE9078_REG_ACCMODE 0x492 +#define ADE9078_REG_CONFIG3 0x493 +#define ADE9078_REG_ZX_LP_SEL 0x49A +#define ADE9078_REG_WFB_CFG 0x4A0 +#define ADE9078_REG_WFB_PG_IRQEN 0x4A1 +#define ADE9078_REG_WFB_TRG_CFG 0x4A2 +#define ADE9078_REG_WFB_TRG_STAT 0x4A3 +#define ADE9078_REG_CONFIG2 0x4AF +#define ADE9078_REG_EP_CFG 0x4B0 +#define ADE9078_REG_EGY_TIME 0x4B2 +#define ADE9078_REG_PGA_GAIN 0x4B9 +#define ADE9078_REG_VERSION 0x4FE +#define ADE9078_REG_WF_BUFF 0x800 + +#define ADE9078_REG_ADDR_MASK GENMASK(15, 4) +#define ADE9078_REG_READ_BIT_MASK BIT(3) +#define ADE9078_RX_DEPTH 6 +#define ADE9078_TX_DEPTH 10 + +#define ADE9078_WF_CAP_EN_MASK BIT(4) +#define ADE9078_WF_CAP_SEL_MASK BIT(5) +#define ADE9078_WF_MODE_MASK GENMASK(7, 6) +#define ADE9078_WF_SRC_MASK GENMASK(9, 8) +#define ADE9078_WF_IN_EN_MASK BIT(12) + +/* + * Configuration registers + * PGA@0x0000. Gain of all channels=1 + */ +#define ADE9078_PGA_GAIN 0x0000 + +/* Default configuration */ +#define ADE9078_CONFIG0 0x00000000 + +/* CF3/ZX pin outputs Zero crossing */ +#define ADE9078_CONFIG1 0x0002 + +/* Default High pass corner frequency of 1.25Hz */ +#define ADE9078_CONFIG2 0x0A00 + +/* Peak and overcurrent detection disabled */ +#define ADE9078_CONFIG3 0x0000 + +/* + * 50Hz operation, 3P4W Wye configuration, signed accumulation + * Clear bit 8 i.e. ACCMODE=0x00xx for 50Hz operation + * ACCMODE=0x0x9x for 3Wire delta when phase B is used as reference + */ +#define ADE9078_ACCMODE 0x0000 + +/*Line period and zero crossing obtained from VA */ +#define ADE9078_ZX_LP_SEL 0x0000 + +/* Disable all interrupts */ +#define ADE9078_MASK0 0x00000000 + +/* Disable all interrupts */ +#define ADE9078_MASK1 0x00000000 + +/* Events disabled */ +#define ADE9078_EVENT_MASK 0x00000000 + +/* + * Assuming Vnom=1/2 of full scale. + * Refer to Technical reference manual for detailed calculations. + */ +#define ADE9078_VLEVEL 0x0022EA28 + +/* Set DICOEFF= 0xFFFFE000 when integrator is enabled */ +#define ADE9078_DICOEFF 0x00000000 + +/* DSP ON */ +#define ADE9078_RUN_ON 0xFFFFFFFF + +/* + * Energy Accumulation Settings + * Enable energy accumulation, accumulate samples at 8ksps + * latch energy accumulation after EGYRDY + * If accumulation is changed to half line cycle mode, change EGY_TIME + */ +#define ADE9078_EP_CFG 0x0011 + +/* Accumulate 4000 samples */ +#define ADE9078_EGY_TIME 0x0FA0 + +/* + * Constant Definitions + * ADE9000 FDSP: 8000sps, ADE9078 FDSP: 4000sps + */ +#define ADE9078_FDSP 4000 +#define ADE9078_WFB_CFG 0x0329 +#define ADE9078_WFB_PAGE_SIZE 128 +#define ADE9078_WFB_BYTES_IN_PAGE 4 +#define ADE9078_WFB_PAGE_ARRAY_SIZE \ + (ADE9078_WFB_PAGE_SIZE * ADE9078_WFB_BYTES_IN_PAGE) +#define ADE9078_WFB_FULL_BUFF_SIZE \ + (ADE9078_WFB_PAGE_ARRAY_SIZE * 16) +#define ADE9078_WFB_FULL_BUFF_NR_SAMPLES \ + (ADE9078_WFB_PAGE_SIZE * 16) + +#define ADE9078_SWRST_BIT BIT(0) + +/* Status and Mask register bits*/ +#define ADE9078_ST0_WFB_TRIG_BIT BIT(16) +#define ADE9078_ST0_PAGE_FULL_BIT BIT(17) + +#define ADE9078_ST1_ZXTOVA_BIT BIT(6) +#define ADE9078_ST1_ZXTOVB_BIT BIT(7) +#define ADE9078_ST1_ZXTOVC_BIT BIT(8) +#define ADE9078_ST1_ZXVA_BIT BIT(9) +#define ADE9078_ST1_ZXVB_BIT BIT(10) +#define ADE9078_ST1_ZXVC_BIT BIT(11) +#define ADE9078_ST1_ZXIA_BIT BIT(13) +#define ADE9078_ST1_ZXIB_BIT BIT(14) +#define ADE9078_ST1_ZXIC_BIT BIT(15) +#define ADE9078_ST1_RSTDONE_BIT BIT(16) +#define ADE9078_ST1_ERROR0_BIT BIT(28) +#define ADE9078_ST1_ERROR1_BIT BIT(29) +#define ADE9078_ST1_ERROR2_BIT BIT(30) +#define ADE9078_ST1_ERROR3_BIT BIT(31) +#define ADE9078_ST_ERROR \ + (ADE9078_ST1_ERROR0 | \ + ADE9078_ST1_ERROR1 | \ + ADE9078_ST1_ERROR2 | \ + ADE9078_ST1_ERROR3) +#define ADE9078_ST1_CROSSING_FIRST 6 +#define ADE9078_ST1_CROSSING_DEPTH 16 + +#define ADE9078_WFB_TRG_ZXIA_BIT BIT(3) +#define ADE9078_WFB_TRG_ZXIB_BIT BIT(4) +#define ADE9078_WFB_TRG_ZXIC_BIT BIT(5) +#define ADE9078_WFB_TRG_ZXVA_BIT BIT(6) +#define ADE9078_WFB_TRG_ZXVB_BIT BIT(7) +#define ADE9078_WFB_TRG_ZXVC_BIT BIT(8) + +/* Stop when waveform buffer is full */ +#define ADE9078_WFB_FULL_MODE 0x0 +/* Continuous fill—stop only on enabled trigger events */ +#define ADE9078_WFB_EN_TRIG_MODE 0x1 +/* Continuous filling—center capture around enabled trigger events */ +#define ADE9078_WFB_C_EN_TRIG_MODE 0x2 +/* Continuous fill—save event address of enabled trigger events */ +#define ADE9078_WFB_SAVE_EN_TRIG_MODE 0x3 + +#define ADE9078_MODE_0_1_PAGE_BIT BIT(15) +#define ADE9078_MODE_2_PAGE_BIT BIT(7) + +/* + * Full scale Codes referred from Datasheet.Respective digital codes are + * produced when ADC inputs are at full scale. Do not Change. + */ +#define ADE9078_RMS_FULL_SCALE_CODES 52866837 +#define ADE9000_WATT_FULL_SCALE_CODES 20694066 +#define ADE9078_PCF_FULL_SCALE_CODES 74770000 + +/* Phase and channel definitions */ +#define ADE9078_PHASE_A_NR 0 +#define ADE9078_PHASE_B_NR 2 +#define ADE9078_PHASE_C_NR 4 +#define ADE9078_PHASE_A_NAME "A" +#define ADE9078_PHASE_B_NAME "B" +#define ADE9078_PHASE_C_NAME "C" + +#define ADE9078_SCAN_POS_IA BIT(0) +#define ADE9078_SCAN_POS_VA BIT(1) +#define ADE9078_SCAN_POS_IB BIT(2) +#define ADE9078_SCAN_POS_VB BIT(3) +#define ADE9078_SCAN_POS_IC BIT(4) +#define ADE9078_SCAN_POS_VC BIT(5) + +#define ADE9078_PHASE_B_POS_BIT BIT(5) +#define ADE9078_PHASE_C_POS_BIT BIT(6) + +#define ADE9078_MAX_PHASE_NR 3 +#define AD9078_CHANNELS_PER_PHASE 9 + +#define ADE9078_ADDR_ADJUST(addr, chan) \ + (((chan) << 4) | (addr)) + +#define ADE9078_CURRENT_CHANNEL(num, name) { \ + .type = IIO_CURRENT, \ + .channel = num, \ + .extend_name = name, \ + .address = ADE9078_ADDR_ADJUST(ADE9078_REG_AI_PCF, num), \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ + BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_HARDWAREGAIN), \ + .event_spec = ade9078_events, \ + .num_event_specs = ARRAY_SIZE(ade9078_events), \ + .scan_index = num, \ + .scan_type = { \ + .sign = 's', \ + .realbits = 32, \ + .storagebits = 32, \ + .shift = 0, \ + .endianness = IIO_BE, \ + }, \ +} + +#define ADE9078_VOLTAGE_CHANNEL(num, name) { \ + .type = IIO_VOLTAGE, \ + .channel = num, \ + .extend_name = name, \ + .address = ADE9078_ADDR_ADJUST(ADE9078_REG_AV_PCF, num), \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ + BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_HARDWAREGAIN), \ + .event_spec = ade9078_events, \ + .num_event_specs = ARRAY_SIZE(ade9078_events), \ + .scan_index = num + 1, \ + .scan_type = { \ + .sign = 's', \ + .realbits = 32, \ + .storagebits = 32, \ + .shift = 0, \ + .endianness = IIO_BE, \ + }, \ +} + +#define ADE9078_CURRENT_RMS_CHANNEL(num, name) { \ + .type = IIO_CURRENT, \ + .channel = num, \ + .address = ADE9078_ADDR_ADJUST(ADE9078_REG_AIRMS, num), \ + .extend_name = name "_rms", \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ + BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_OFFSET), \ + .scan_index = -1 \ +} + +#define ADE9078_VOLTAGE_RMS_CHANNEL(num, name) { \ + .type = IIO_VOLTAGE, \ + .channel = num, \ + .address = ADE9078_ADDR_ADJUST(ADE9078_REG_AVRMS, num), \ + .extend_name = name "_rms", \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ + BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_OFFSET), \ + .scan_index = -1 \ +} + +#define ADE9078_POWER_ACTIV_CHANNEL(num, name) { \ + .type = IIO_POWER, \ + .channel = num, \ + .address = ADE9078_ADDR_ADJUST(ADE9078_REG_AWATT, num), \ + .extend_name = name "_activ", \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ + BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_OFFSET) | \ + BIT(IIO_CHAN_INFO_HARDWAREGAIN), \ + .scan_index = -1 \ +} + +#define ADE9078_POWER_REACTIV_CHANNEL(num, name) { \ + .type = IIO_POWER, \ + .channel = num, \ + .address = ADE9078_ADDR_ADJUST(ADE9078_REG_AVAR, num), \ + .extend_name = name "_reactiv", \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ + BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_OFFSET), \ + .scan_index = -1 \ +} + +#define ADE9078_POWER_APPARENT_CHANNEL(num, name) { \ + .type = IIO_POWER, \ + .channel = num, \ + .address = ADE9078_ADDR_ADJUST(ADE9078_REG_AVA, num), \ + .extend_name = name "_apparent", \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ + BIT(IIO_CHAN_INFO_SCALE), \ + .scan_index = -1 \ +} + +#define ADE9078_POWER_FUND_REACTIV_CHANNEL(num, name) { \ + .type = IIO_POWER, \ + .channel = num, \ + .address = ADE9078_ADDR_ADJUST(ADE9078_REG_AFVAR, num), \ + .extend_name = name "_fund_reactiv", \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ + BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_OFFSET), \ + .scan_index = -1 \ +} + +#define ADE9078_POWER_FACTOR_CHANNEL(num, name) { \ + .type = IIO_POWER, \ + .channel = num, \ + .address = ADE9078_ADDR_ADJUST(ADE9078_REG_APF, num), \ + .extend_name = name "_factor", \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ + BIT(IIO_CHAN_INFO_SCALE), \ + .scan_index = -1 \ +} + +/* + * struct ade9078_state - ade9078 specific data + * @rst_done flag for when reset sequence irq has been received + * @wf_mode wave form buffer mode, read datasheet for more details, + * retrieved from DT + * @wfb_trg wave form buffer triger configuration, read datasheet for more + * details, retrieved from DT + * @spi spi device associated to the ade9078 + * @tx transmit buffer for the spi + * @rx receive buffer for the spi + * @xfer transfer setup used in iio buffer configuration + * @spi_msg message transfer trough spi, used in iio buffer + * configuration + * @regmap register map pointer + * @indio_dev: the IIO device + * @trig iio trigger pointer, is connected to IRQ0 and IRQ1 + * @rx_buff receive buffer for the iio buffer trough spi, will + * contain the samples from the IC wave form buffer + * @tx_buff transmit buffer for the iio buffer trough spi, used + * in iio buffer configuration + */ + +struct ade9078_state { + bool rst_done; + u8 wf_mode; + u32 wfb_trg; + struct spi_device *spi; + u8 *tx; + u8 *rx; + struct spi_transfer xfer[2]; + struct spi_message spi_msg; + struct regmap *regmap; + struct iio_dev *indio_dev; + union{ + u8 byte[ADE9078_WFB_FULL_BUFF_SIZE]; + __be32 word[ADE9078_WFB_FULL_BUFF_NR_SAMPLES]; + } rx_buff ____cacheline_aligned; + u8 tx_buff[2]; +}; + +static const struct iio_event_spec ade9078_events[] = { + { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_EITHER, + .mask_separate = BIT(IIO_EV_INFO_ENABLE) | + BIT(IIO_EV_INFO_VALUE), + }, +}; + +/* IIO channels of the ade9078 for each phase individually */ +static const struct iio_chan_spec ade9078_a_channels[] = { + ADE9078_CURRENT_CHANNEL(ADE9078_PHASE_A_NR, ADE9078_PHASE_A_NAME), + ADE9078_VOLTAGE_CHANNEL(ADE9078_PHASE_A_NR, ADE9078_PHASE_A_NAME), + ADE9078_CURRENT_RMS_CHANNEL(ADE9078_PHASE_A_NR, ADE9078_PHASE_A_NAME), + ADE9078_VOLTAGE_RMS_CHANNEL(ADE9078_PHASE_A_NR, ADE9078_PHASE_A_NAME), + ADE9078_POWER_ACTIV_CHANNEL(ADE9078_PHASE_A_NR, ADE9078_PHASE_A_NAME), + ADE9078_POWER_REACTIV_CHANNEL(ADE9078_PHASE_A_NR, ADE9078_PHASE_A_NAME), + ADE9078_POWER_APPARENT_CHANNEL(ADE9078_PHASE_A_NR, + ADE9078_PHASE_A_NAME), + ADE9078_POWER_FUND_REACTIV_CHANNEL(ADE9078_PHASE_A_NR, + ADE9078_PHASE_A_NAME), + ADE9078_POWER_FACTOR_CHANNEL(ADE9078_PHASE_A_NR, ADE9078_PHASE_A_NAME), +}; + +static const struct iio_chan_spec ade9078_b_channels[] = { + ADE9078_CURRENT_CHANNEL(ADE9078_PHASE_B_NR, ADE9078_PHASE_B_NAME), + ADE9078_VOLTAGE_CHANNEL(ADE9078_PHASE_B_NR, ADE9078_PHASE_B_NAME), + ADE9078_CURRENT_RMS_CHANNEL(ADE9078_PHASE_B_NR, ADE9078_PHASE_B_NAME), + ADE9078_VOLTAGE_RMS_CHANNEL(ADE9078_PHASE_B_NR, ADE9078_PHASE_B_NAME), + ADE9078_POWER_ACTIV_CHANNEL(ADE9078_PHASE_B_NR, ADE9078_PHASE_B_NAME), + ADE9078_POWER_REACTIV_CHANNEL(ADE9078_PHASE_B_NR, ADE9078_PHASE_B_NAME), + ADE9078_POWER_APPARENT_CHANNEL(ADE9078_PHASE_B_NR, + ADE9078_PHASE_B_NAME), + ADE9078_POWER_FUND_REACTIV_CHANNEL(ADE9078_PHASE_B_NR, + ADE9078_PHASE_B_NAME), + ADE9078_POWER_FACTOR_CHANNEL(ADE9078_PHASE_B_NR, ADE9078_PHASE_B_NAME), +}; + +static const struct iio_chan_spec ade9078_c_channels[] = { + ADE9078_CURRENT_CHANNEL(ADE9078_PHASE_C_NR, ADE9078_PHASE_C_NAME), + ADE9078_VOLTAGE_CHANNEL(ADE9078_PHASE_C_NR, ADE9078_PHASE_C_NAME), + ADE9078_CURRENT_RMS_CHANNEL(ADE9078_PHASE_C_NR, ADE9078_PHASE_C_NAME), + ADE9078_VOLTAGE_RMS_CHANNEL(ADE9078_PHASE_C_NR, ADE9078_PHASE_C_NAME), + ADE9078_POWER_ACTIV_CHANNEL(ADE9078_PHASE_C_NR, ADE9078_PHASE_C_NAME), + ADE9078_POWER_REACTIV_CHANNEL(ADE9078_PHASE_C_NR, ADE9078_PHASE_C_NAME), + ADE9078_POWER_APPARENT_CHANNEL(ADE9078_PHASE_C_NR, + ADE9078_PHASE_C_NAME), + ADE9078_POWER_FUND_REACTIV_CHANNEL(ADE9078_PHASE_C_NR, + ADE9078_PHASE_C_NAME), + ADE9078_POWER_FACTOR_CHANNEL(ADE9078_PHASE_C_NR, ADE9078_PHASE_C_NAME), +}; + +static const struct reg_sequence ade9078_reg_sequence[] = { + { ADE9078_REG_PGA_GAIN, ADE9078_PGA_GAIN }, + { ADE9078_REG_CONFIG0, ADE9078_CONFIG0 }, + { ADE9078_REG_CONFIG1, ADE9078_CONFIG1 }, + { ADE9078_REG_CONFIG2, ADE9078_CONFIG2 }, + { ADE9078_REG_CONFIG3, ADE9078_CONFIG3 }, + { ADE9078_REG_ACCMODE, ADE9078_ACCMODE }, + { ADE9078_REG_ZX_LP_SEL, ADE9078_ZX_LP_SEL }, + { ADE9078_REG_MASK0, ADE9078_MASK0 }, + { ADE9078_REG_MASK1, ADE9078_MASK1 }, + { ADE9078_REG_EVENT_MASK, ADE9078_EVENT_MASK }, + { ADE9078_REG_WFB_CFG, ADE9078_WFB_CFG }, + { ADE9078_REG_VLEVEL, ADE9078_VLEVEL }, + { ADE9078_REG_DICOEFF, ADE9078_DICOEFF }, + { ADE9078_REG_EGY_TIME, ADE9078_EGY_TIME }, + { ADE9078_REG_EP_CFG, ADE9078_EP_CFG }, + { ADE9078_REG_RUN, ADE9078_RUN_ON } +}; + +/* + * ade9078_spi_write_reg() - ade9078 write register over SPI + * the data format for communicating with the ade9078 over SPI + * is very specific and can access both 32bit and 16bit registers + * @context: void pointer to the SPI device + * @reg: address of the of desired register + * @val: value to be written to the ade9078 + */ +static int ade9078_spi_write_reg(void *context, + unsigned int reg, + unsigned int val) +{ + struct device *dev = context; + struct spi_device *spi = to_spi_device(dev); + struct ade9078_state *st = spi_get_drvdata(spi); + + u16 addr; + int ret = 0; + struct spi_transfer xfer[] = { + { + .tx_buf = st->tx, + }, + }; + + addr = FIELD_PREP(ADE9078_REG_ADDR_MASK, reg); + + put_unaligned_be16(addr, st->tx); + put_unaligned_be32(val, &st->tx[2]); + + if (reg > ADE9078_REG_RUN && reg < ADE9078_REG_VERSION) { + put_unaligned_be16(val, &st->tx[2]); + xfer[0].len = 4; + } else { + xfer[0].len = 6; + } + + ret = spi_sync_transfer(st->spi, xfer, ARRAY_SIZE(xfer)); + if (ret) { + dev_err(&st->spi->dev, "problem when writing register 0x%x", + reg); + } + + return ret; +} + +/* + * ade9078_spi_write_reg() - ade9078 read register over SPI + * the data format for communicating with the ade9078 over SPI + * is very specific and can access both 32bit and 16bit registers + * @context: void pointer to the SPI device + * @reg: address of the of desired register + * @val: value to be read to the ade9078 + */ +static int ade9078_spi_read_reg(void *context, + unsigned int reg, + unsigned int *val) +{ + struct device *dev = context; + struct spi_device *spi = to_spi_device(dev); + struct ade9078_state *st = spi_get_drvdata(spi); + + u16 addr; + int ret = 0; + struct spi_transfer xfer[] = { + { + .tx_buf = st->tx, + .len = 2, + }, + { + .rx_buf = st->rx, + }, + }; + + addr = FIELD_PREP(ADE9078_REG_ADDR_MASK, reg) | + ADE9078_REG_READ_BIT_MASK; + + put_unaligned_be16(addr, st->tx); + + if (reg > ADE9078_REG_RUN && reg < ADE9078_REG_VERSION) + xfer[1].len = 4; + else + xfer[1].len = 6; + + ret = spi_sync_transfer(st->spi, xfer, ARRAY_SIZE(xfer)); + if (ret) { + dev_err(&st->spi->dev, "problem when reading register 0x%x", + reg); + goto err_ret; + } + + //registers which are 16 bits + if (reg > 0x480 && reg < 0x4FE) + *val = get_unaligned_be16(st->rx); + else + *val = get_unaligned_be32(st->rx); + +err_ret: + return ret; +} + +/* + * ade9078_is_volatile_reg() - list of ade9078 registers which should use + * caching + * @dev: device data + * @reg: address of the of desired register + */ +static bool ade9078_is_volatile_reg(struct device *dev, unsigned int reg) +{ + switch (reg) { + case ADE9078_REG_STATUS0: + case ADE9078_REG_STATUS1: + case ADE9078_REG_MASK0: + case ADE9078_REG_MASK1: + return true; + default: + return false; + } +} + +/* + * ade9078_en_wfb() - enables or disables the WFBuffer in the ADE9078 + * @st: ade9078 device data + * @state: true for enabled; false for disabled + */ +static int ade9078_en_wfb(struct ade9078_state *st, bool state) +{ + return regmap_update_bits(st->regmap, ADE9078_REG_WFB_CFG, BIT(4), + state ? BIT(4) : 0); +} + +/* + * ade9078_iio_push_buffer() - reads out the content of the waveform buffer and + * pushes it to the IIO buffer. + * @st: ade9078 device data + */ +static int ade9078_iio_push_buffer(struct ade9078_state *st) +{ + int ret; + u32 i; + + ret = spi_sync(st->spi, &st->spi_msg); + if (ret) { + dev_err(&st->spi->dev, "SPI fail in trigger handler"); + return ret; + } + + for (i = 0; i < ADE9078_WFB_FULL_BUFF_NR_SAMPLES; i++) + iio_push_to_buffers(st->indio_dev, &st->rx_buff.word[i]); + + return 0; +} + +/* + * ade9078_irq0_thread() - Thread for IRQ0. It reads Status register 0 and + * checks for the IRQ activation. This is configured to acquire samples in to + * the IC buffer and dump it in to the iio_buffer according to Stop When Buffer + * Is Full Mode, Stop Filling on Trigger and Capture Around Trigger from the + * ADE9078 Datasheet + */ +static irqreturn_t ade9078_irq0_thread(int irq, void *data) +{ + struct ade9078_state *st = data; + u32 handled_irq = 0; + u32 interrupts; + u32 status; + int ret; + + ret = regmap_read(st->regmap, ADE9078_REG_STATUS0, &status); + if (ret) { + dev_err(&st->spi->dev, "IRQ0 read status fail"); + goto irq0_done; + } + + ret = regmap_read(st->regmap, ADE9078_REG_MASK0, &interrupts); + if (ret) { + dev_err(&st->spi->dev, "IRQ0 read status fail"); + goto irq0_done; + } + + if ((status & ADE9078_ST0_PAGE_FULL_BIT) && + (interrupts & ADE9078_ST0_PAGE_FULL_BIT)) { + //Stop Filling on Trigger and Center Capture Around Trigger + if (st->wf_mode) { + ret = regmap_write(st->regmap, ADE9078_REG_WFB_TRG_CFG, + st->wfb_trg); + if (ret) { + dev_err(&st->spi->dev, "IRQ0 WFB write fail"); + goto irq0_done; + } + + interrupts |= ADE9078_ST0_WFB_TRIG_BIT; + + } else { + //Stop When Buffer Is Full Mode + ret = ade9078_en_wfb(st, false); + if (ret) { + dev_err(&st->spi->dev, "IRQ0 WFB stop fail"); + goto irq0_done; + } + ret = ade9078_iio_push_buffer(st); + if (ret) { + dev_err(&st->spi->dev, "IRQ0 IIO push fail"); + goto irq0_done; + } + } + + //disable Page full interrupt + interrupts &= ~ADE9078_ST0_PAGE_FULL_BIT; + + ret = regmap_write(st->regmap, ADE9078_REG_MASK0, interrupts); + if (ret) { + dev_err(&st->spi->dev, "IRQ0 MAKS0 write fail"); + goto irq0_done; + } + + handled_irq |= ADE9078_ST0_PAGE_FULL_BIT; + } + + if ((status & ADE9078_ST0_WFB_TRIG_BIT) && + (interrupts & ADE9078_ST0_WFB_TRIG_BIT)) { + //Stop Filling on Trigger and Center Capture Around Trigger + ret = ade9078_en_wfb(st, false); + if (ret) { + dev_err(&st->spi->dev, "IRQ0 WFB fail"); + goto irq0_done; + } + + ret = ade9078_iio_push_buffer(st); + if (ret) { + dev_err(&st->spi->dev, "IRQ0 IIO push fail @ WFB TRIG"); + goto irq0_done; + } + + handled_irq |= ADE9078_ST0_WFB_TRIG_BIT; + } + + ret = regmap_write(st->regmap, ADE9078_REG_STATUS0, handled_irq); + if (ret) + dev_err(&st->spi->dev, "IRQ0 write status fail"); + +irq0_done: + return IRQ_HANDLED; +} + +/* + * ade9078_irq1_thread() - Thread for IRQ1. It reads Status register 1 and + * checks for the IRQ activation. This thread handles the reset condition and + * the zero-crossing conditions for all 3 phases on Voltage and Current + */ +static irqreturn_t ade9078_irq1_thread(int irq, void *data) +{ + struct ade9078_state *st = data; + struct iio_dev *indio_dev = st->indio_dev; + struct iio_chan_spec const *chan = indio_dev->channels; + unsigned int bit = ADE9078_ST1_CROSSING_FIRST; + s64 timestamp = iio_get_time_ns(indio_dev); + u32 interrupts; + u32 result; + u32 status; + u32 tmp; + int ret; + + //reset + if (!st->rst_done) { + ret = regmap_read(st->regmap, ADE9078_REG_STATUS1, &result); + if (ret) + return ret; + + if (result & ADE9078_ST1_RSTDONE_BIT) + st->rst_done = true; + else + dev_err(&st->spi->dev, "Error testing reset done"); + + goto irq1_done; + } + + ret = regmap_read(st->regmap, ADE9078_REG_STATUS1, &status); + if (ret) + goto irq1_done; + + ret = regmap_read(st->regmap, ADE9078_REG_MASK1, &interrupts); + if (ret) { + dev_err(&st->spi->dev, "IRQ1 read status fail"); + goto irq1_done; + } + + //crossings + for_each_set_bit_from(bit, (unsigned long *)&interrupts, + ADE9078_ST1_CROSSING_DEPTH) { + tmp = status & BIT(bit); + + switch (tmp) { + case ADE9078_ST1_ZXVA_BIT: + case ADE9078_ST1_ZXTOVA_BIT: + case ADE9078_ST1_ZXIA_BIT: + iio_push_event(indio_dev, + IIO_UNMOD_EVENT_CODE(chan->type, + ADE9078_PHASE_A_NR, + IIO_EV_TYPE_THRESH, + IIO_EV_DIR_EITHER), + timestamp); + break; + case ADE9078_ST1_ZXVB_BIT: + case ADE9078_ST1_ZXTOVB_BIT: + case ADE9078_ST1_ZXIB_BIT: + iio_push_event(indio_dev, + IIO_UNMOD_EVENT_CODE(chan->type, + ADE9078_PHASE_B_NR, + IIO_EV_TYPE_THRESH, + IIO_EV_DIR_EITHER), + timestamp); + break; + case ADE9078_ST1_ZXVC_BIT: + case ADE9078_ST1_ZXTOVC_BIT: + case ADE9078_ST1_ZXIC_BIT: + iio_push_event(indio_dev, + IIO_UNMOD_EVENT_CODE(chan->type, + ADE9078_PHASE_C_NR, + IIO_EV_TYPE_THRESH, + IIO_EV_DIR_EITHER), + timestamp); + break; + default: + goto irq1_done; + } + } + +irq1_done: + return IRQ_HANDLED; +} + +/* + * ade9078_configure_scan() - sets up the transfer parameters + * as well as the tx and rx buffers + * @indio_dev: the IIO device + */ +static int ade9078_configure_scan(struct iio_dev *indio_dev) +{ + struct ade9078_state *st = iio_priv(indio_dev); + u16 addr; + + addr = FIELD_PREP(ADE9078_REG_ADDR_MASK, ADE9078_REG_WF_BUFF) | + ADE9078_REG_READ_BIT_MASK; + + put_unaligned_be16(addr, st->tx_buff); + + st->xfer[0].tx_buf = &st->tx_buff[0]; + st->xfer[0].len = 2; + + st->xfer[1].rx_buf = &st->rx_buff.byte[0]; + st->xfer[1].len = ADE9078_WFB_FULL_BUFF_SIZE; + + spi_message_init_with_transfers(&st->spi_msg, st->xfer, 2); + return 0; +} + +/* + * ade9078_read_raw() - IIO read function + * @indio_dev: the IIO device + * @chan: channel specs of the ade9078 + * @val: first half of the read value + * @val2: second half of the read value + * @mask: info mask of the channel + */ +static int ade9078_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int *val, + int *val2, + long mask) +{ + struct ade9078_state *st = iio_priv(indio_dev); + int measured; + int ret; + + switch (mask) { + case IIO_CHAN_INFO_RAW: + ret = iio_device_claim_direct_mode(indio_dev); + if (ret) + return ret; + + ret = regmap_read(st->regmap, chan->address, &measured); + + iio_device_release_direct_mode(indio_dev); + *val = measured; + + return ret ?: IIO_VAL_INT; + + case IIO_CHAN_INFO_SCALE: + if (chan->type == IIO_CURRENT || chan->type == IIO_VOLTAGE) { + switch (chan->address) { + case ADE9078_REG_AI_PCF: + case ADE9078_REG_AV_PCF: + case ADE9078_REG_BI_PCF: + case ADE9078_REG_BV_PCF: + case ADE9078_REG_CI_PCF: + case ADE9078_REG_CV_PCF: + *val = 1; + *val2 = ADE9078_PCF_FULL_SCALE_CODES; + return IIO_VAL_FRACTIONAL; + case ADE9078_REG_AIRMS: + case ADE9078_REG_AVRMS: + case ADE9078_REG_BIRMS: + case ADE9078_REG_BVRMS: + case ADE9078_REG_CIRMS: + case ADE9078_REG_CVRMS: + *val = 1; + *val2 = ADE9078_RMS_FULL_SCALE_CODES; + return IIO_VAL_FRACTIONAL; + default: + return -EINVAL; + } + } else if (chan->type == IIO_POWER) { + *val = 1; + *val2 = ADE9000_WATT_FULL_SCALE_CODES; + return IIO_VAL_FRACTIONAL; + } else { + return -EINVAL; + } + break; + default: + return -EINVAL; + } + + return 0; +} + +/* + * ade9078_write_raw() - IIO write function + * @indio_dev: the IIO device + * @chan: channel specs of the ade9078 + * @val: first half of the read value + * @val2: second half of the read value + * @mask: info mask of the channel + */ +static int ade9078_write_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int val, + int val2, + long mask) +{ + struct ade9078_state *st = iio_priv(indio_dev); + u32 addr = 0xFFFFF; + u32 tmp; + int ret; + + switch (mask) { + case IIO_CHAN_INFO_OFFSET: + switch (chan->type) { + case IIO_CURRENT: + addr = ADE9078_ADDR_ADJUST(ADE9078_REG_AIRMSOS, + chan->channel); + break; + case IIO_VOLTAGE: + addr = ADE9078_ADDR_ADJUST(ADE9078_REG_AVRMSOS, + chan->channel); + break; + case IIO_POWER: + tmp = chan->address; + tmp &= ~ADE9078_PHASE_B_POS_BIT; + tmp &= ~ADE9078_PHASE_C_POS_BIT; + + switch (tmp) { + case ADE9078_REG_AWATTOS: + addr = ADE9078_ADDR_ADJUST(ADE9078_REG_AWATTOS, + chan->channel); + break; + case ADE9078_REG_AVAR: + addr = ADE9078_ADDR_ADJUST(ADE9078_REG_AVAROS, + chan->channel); + break; + case ADE9078_REG_AFVAR: + addr = ADE9078_ADDR_ADJUST(ADE9078_REG_AFVAROS, + chan->channel); + break; + default: + return -EINVAL; + } + + break; + default: + return -EINVAL; + } + break; + case IIO_CHAN_INFO_HARDWAREGAIN: + switch (chan->type) { + case IIO_CURRENT: + addr = ADE9078_ADDR_ADJUST(ADE9078_REG_AIGAIN, + chan->channel); + break; + case IIO_VOLTAGE: + addr = ADE9078_ADDR_ADJUST(ADE9078_REG_AVGAIN, + chan->channel); + break; + case IIO_POWER: + addr = ADE9078_ADDR_ADJUST(ADE9078_REG_APGAIN, + chan->channel); + break; + default: + return -EINVAL; + } + break; + default: + return -EINVAL; + } + + ret = regmap_write(st->regmap, addr, val); + return ret; +} + +/* + * ade9078_reg_access() - IIO debug register access + * @indio_dev: the IIO device + * @reg: register to be accessed + * @tx_val: value to be transmitted + * @rx_val: value to be received + */ +static int ade9078_reg_access(struct iio_dev *indio_dev, + unsigned int reg, + unsigned int tx_val, + unsigned int *rx_val) +{ + struct ade9078_state *st = iio_priv(indio_dev); + + if (rx_val) + return regmap_read(st->regmap, reg, rx_val); + + return regmap_write(st->regmap, reg, tx_val); +} + +/* + * ade9078_write_event_config() - IIO event configure to enable zero-crossing + * and zero-crossing timeout on voltage and current for each phases. These + * events will also influence the trigger conditions for the buffer capture. + */ +static int ade9078_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 ade9078_state *st = iio_priv(indio_dev); + u32 interrupts; + u32 number; + int ret; + + number = chan->channel; + + switch (number) { + case ADE9078_PHASE_A_NR: + if (chan->type == IIO_VOLTAGE) { + if (state) { + interrupts |= ADE9078_ST1_ZXVA_BIT; + interrupts |= ADE9078_ST1_ZXTOVA_BIT; + st->wfb_trg |= ADE9078_WFB_TRG_ZXVA_BIT; + } else { + interrupts &= ~ADE9078_ST1_ZXVA_BIT; + interrupts &= ~ADE9078_ST1_ZXTOVA_BIT; + st->wfb_trg &= ~ADE9078_WFB_TRG_ZXVA_BIT; + } + } else if (chan->type == IIO_CURRENT) { + if (state) { + interrupts |= ADE9078_ST1_ZXIA_BIT; + st->wfb_trg |= ADE9078_WFB_TRG_ZXIA_BIT; + } else { + interrupts &= ~ADE9078_ST1_ZXIA_BIT; + st->wfb_trg &= ~ADE9078_WFB_TRG_ZXIA_BIT; + } + } + break; + case ADE9078_PHASE_B_NR: + if (chan->type == IIO_VOLTAGE) { + if (state) { + interrupts |= ADE9078_ST1_ZXVB_BIT; + interrupts |= ADE9078_ST1_ZXTOVB_BIT; + st->wfb_trg |= ADE9078_WFB_TRG_ZXVB_BIT; + } else { + interrupts &= ~ADE9078_ST1_ZXVB_BIT; + interrupts &= ~ADE9078_ST1_ZXTOVB_BIT; + st->wfb_trg &= ~ADE9078_WFB_TRG_ZXVB_BIT; + } + } else if (chan->type == IIO_CURRENT) { + if (state) { + interrupts |= ADE9078_ST1_ZXIB_BIT; + st->wfb_trg |= ADE9078_WFB_TRG_ZXIB_BIT; + } else { + interrupts &= ~ADE9078_ST1_ZXIB_BIT; + st->wfb_trg &= ~ADE9078_WFB_TRG_ZXIB_BIT; + } + } + break; + case ADE9078_PHASE_C_NR: + if (chan->type == IIO_VOLTAGE) { + if (state) { + interrupts |= ADE9078_ST1_ZXVC_BIT; + interrupts |= ADE9078_ST1_ZXTOVC_BIT; + st->wfb_trg |= ADE9078_WFB_TRG_ZXVC_BIT; + } else { + interrupts &= ~ADE9078_ST1_ZXVC_BIT; + interrupts &= ~ADE9078_ST1_ZXTOVC_BIT; + st->wfb_trg &= ~ADE9078_WFB_TRG_ZXVC_BIT; + } + } else if (chan->type == IIO_CURRENT) { + if (state) { + interrupts |= ADE9078_ST1_ZXIC_BIT; + st->wfb_trg |= ADE9078_WFB_TRG_ZXIC_BIT; + } else { + interrupts &= ~ADE9078_ST1_ZXIC_BIT; + st->wfb_trg &= ~ADE9078_WFB_TRG_ZXIC_BIT; + } + } + break; + default: + return -EINVAL; + } + + ret = regmap_write(st->regmap, ADE9078_REG_STATUS1, GENMASK(31, 0)); + if (ret) + return ret; + + return regmap_update_bits(st->regmap, ADE9078_REG_MASK1, interrupts, + interrupts); +} + +/* + * ade9078_read_event_vlaue() - Outputs the result of the zero-crossing for + * voltage and current for each phase. + * Result: + * 0 - if crossing event not set + * 1 - if crossing event occurred + * -1 - if crossing timeout (only for Voltages) + */ +static int ade9078_read_event_vlaue(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, + enum iio_event_info info, + int *val, int *val2) +{ + struct ade9078_state *st = iio_priv(indio_dev); + u32 handeled_irq = 0; + u32 interrupts; + u32 number; + u32 status; + int ret; + + ret = regmap_read(st->regmap, ADE9078_REG_STATUS1, &status); + if (ret) + return ret; + + ret = regmap_read(st->regmap, ADE9078_REG_MASK1, &interrupts); + if (ret) + return ret; + + *val = 0; + number = chan->channel; + switch (number) { + case ADE9078_PHASE_A_NR: + if (chan->type == IIO_VOLTAGE) { + if (status & ADE9078_ST1_ZXVA_BIT) { + *val = 1; + handeled_irq |= ADE9078_ST1_ZXVA_BIT; + } else if (status & ADE9078_ST1_ZXTOVA_BIT) { + *val = -1; + handeled_irq |= ADE9078_ST1_ZXTOVA_BIT; + } + if (!(interrupts & ADE9078_ST1_ZXTOVA_BIT)) + *val = 0; + } else if (chan->type == IIO_CURRENT) { + if (status & ADE9078_ST1_ZXIA_BIT) { + *val = 1; + handeled_irq |= ADE9078_ST1_ZXIA_BIT; + } + } + break; + case ADE9078_PHASE_B_NR: + if (chan->type == IIO_VOLTAGE) { + if (status & ADE9078_ST1_ZXVB_BIT) { + *val = 1; + handeled_irq |= ADE9078_ST1_ZXVB_BIT; + } else if (status & ADE9078_ST1_ZXTOVB_BIT) { + *val = -1; + handeled_irq |= ADE9078_ST1_ZXTOVB_BIT; + } + if (!(interrupts & ADE9078_ST1_ZXTOVB_BIT)) + *val = 0; + } else if (chan->type == IIO_CURRENT) { + if (status & ADE9078_ST1_ZXIB_BIT) { + *val = 1; + handeled_irq |= ADE9078_ST1_ZXIB_BIT; + } + } + break; + case ADE9078_PHASE_C_NR: + if (chan->type == IIO_VOLTAGE) { + if (status & ADE9078_ST1_ZXVC_BIT) { + *val = 1; + handeled_irq |= ADE9078_ST1_ZXVC_BIT; + } else if (status & ADE9078_ST1_ZXTOVC_BIT) { + *val = -1; + handeled_irq |= ADE9078_ST1_ZXTOVC_BIT; + } + if (!(interrupts & ADE9078_ST1_ZXTOVC_BIT)) + *val = 0; + } else if (chan->type == IIO_CURRENT) { + if (status & ADE9078_ST1_ZXIC_BIT) { + *val = 1; + handeled_irq |= ADE9078_ST1_ZXIC_BIT; + } + } + break; + default: + return -EINVAL; + } + + ret = regmap_write(st->regmap, ADE9078_REG_STATUS1, handeled_irq); + if (ret) + return ret; + + return IIO_VAL_INT; +} + +/* + * ade9078_config_wfb() - reads the ade9078 node and configures the wave form + * buffer based on the options set. Additionally is reads the active scan mask + * in order to set the input data of the buffer. There are only a few available + * input configurations permitted by the IC, any unpermitted configuration will + * result in all channels being active. + * @indio_dev: the IIO device + */ +static int ade9078_config_wfb(struct iio_dev *indio_dev) +{ + struct ade9078_state *st = iio_priv(indio_dev); + u32 wfg_cfg_val = 0; + u32 tmp; + int ret; + + bitmap_to_arr32(&wfg_cfg_val, indio_dev->active_scan_mask, + indio_dev->masklength); + + switch (wfg_cfg_val) { + case ADE9078_SCAN_POS_IA | ADE9078_SCAN_POS_VA: + wfg_cfg_val = 0x1; + break; + case ADE9078_SCAN_POS_IB | ADE9078_SCAN_POS_VB: + wfg_cfg_val = 0x2; + break; + case ADE9078_SCAN_POS_IC | ADE9078_SCAN_POS_VC: + wfg_cfg_val = 0x3; + break; + case ADE9078_SCAN_POS_IA: + wfg_cfg_val = 0x8; + break; + case ADE9078_SCAN_POS_VA: + wfg_cfg_val = 0x9; + break; + case ADE9078_SCAN_POS_IB: + wfg_cfg_val = 0xA; + break; + case ADE9078_SCAN_POS_VB: + wfg_cfg_val = 0xB; + break; + case ADE9078_SCAN_POS_IC: + wfg_cfg_val = 0xC; + break; + case ADE9078_SCAN_POS_VC: + wfg_cfg_val = 0xD; + break; + default: + wfg_cfg_val = 0x0; + break; + } + + ret = of_property_read_u32((&st->spi->dev)->of_node, "adi,wf-cap-sel", + &tmp); + if (ret) { + dev_err(&st->spi->dev, "Failed to get wf-cap-sel: %d\n", ret); + return ret; + } + wfg_cfg_val |= FIELD_PREP(ADE9078_WF_CAP_SEL_MASK, tmp); + + ret = of_property_read_u32((&st->spi->dev)->of_node, "adi,wf-mode", + &tmp); + if (ret) { + dev_err(&st->spi->dev, "Failed to get wf-mode: %d\n", ret); + return ret; + } + wfg_cfg_val |= FIELD_PREP(ADE9078_WF_MODE_MASK, tmp); + st->wf_mode = tmp; + + ret = of_property_read_u32((&st->spi->dev)->of_node, "adi,wf-src", + &tmp); + if (ret) { + dev_err(&st->spi->dev, + "Failed to get wf-src: %d\n", + ret); + return ret; + } + wfg_cfg_val |= FIELD_PREP(ADE9078_WF_SRC_MASK, tmp); + + ret = of_property_read_u32((&st->spi->dev)->of_node, "adi,wf-in-en", + &tmp); + if (ret) { + dev_err(&st->spi->dev, "Failed to get wf-in-en: %d\n", ret); + return ret; + } + wfg_cfg_val |= FIELD_PREP(ADE9078_WF_IN_EN_MASK, tmp); + + return regmap_write(st->regmap, ADE9078_REG_WFB_CFG, wfg_cfg_val); +} + +/* + * ade9078_wfb_interrupt_setup() - Configures the wave form buffer interrupt + * according to modes + * @st: ade9078 device data + * @mode: modes according to datasheet; values [0-2] + * + * This sets the interrupt register and other registers related to the + * interrupts according to mode [0-2] from the datasheet + */ +static int ade9078_wfb_interrupt_setup(struct ade9078_state *st, u8 mode) +{ + int ret; + + ret = regmap_write(st->regmap, ADE9078_REG_WFB_TRG_CFG, 0x0); + if (ret) + return ret; + + if (mode == ADE9078_WFB_FULL_MODE || mode == ADE9078_WFB_EN_TRIG_MODE) { + ret = regmap_write(st->regmap, ADE9078_REG_WFB_PG_IRQEN, + ADE9078_MODE_0_1_PAGE_BIT); + if (ret) + return ret; + } else if (mode == ADE9078_WFB_C_EN_TRIG_MODE) { + ret = regmap_write(st->regmap, ADE9078_REG_WFB_PG_IRQEN, + ADE9078_MODE_2_PAGE_BIT); + if (ret) + return ret; + } + + ret = regmap_write(st->regmap, ADE9078_REG_STATUS0, GENMASK(31, 0)); + if (ret) + return ret; + + return regmap_update_bits(st->regmap, ADE9078_REG_MASK0, + ADE9078_ST0_PAGE_FULL_BIT, + ADE9078_ST0_PAGE_FULL_BIT); + if (ret) + return ret; + + return 0; +} + +/* + * ade9078_buffer_preenable() - configures the waveform buffer, sets the + * interrupts and enables the buffer + * @indio_dev: the IIO device + */ +static int ade9078_buffer_preenable(struct iio_dev *indio_dev) +{ + struct ade9078_state *st = iio_priv(indio_dev); + int ret; + + ret = ade9078_config_wfb(indio_dev); + if (ret) + return ret; + + ret = ade9078_wfb_interrupt_setup(st, st->wf_mode); + if (ret) + return ret; + + ret = ade9078_en_wfb(st, true); + if (ret) { + dev_err(&st->spi->dev, "Post-enable wfb enable fail"); + return ret; + } + + return 0; +} + +/* + * ade9078_buffer_postdisable() - after the iio is disable + * this will disable the ade9078 internal buffer for acquisition + * @indio_dev: the IIO device + */ +static int ade9078_buffer_postdisable(struct iio_dev *indio_dev) +{ + struct ade9078_state *st = iio_priv(indio_dev); + u32 interrupts = 0; + int ret; + + ret = ade9078_en_wfb(st, false); + if (ret) { + dev_err(&st->spi->dev, "Post-disable wfb disable fail"); + return ret; + } + + ret = regmap_write(st->regmap, ADE9078_REG_WFB_TRG_CFG, 0x0); + if (ret) + return ret; + + interrupts |= ADE9078_ST0_WFB_TRIG_BIT; + interrupts |= ADE9078_ST0_PAGE_FULL_BIT; + + return regmap_update_bits(st->regmap, ADE9078_REG_MASK0, interrupts, 0); + if (ret) { + dev_err(&st->spi->dev, "Post-disable update maks0 fail"); + return ret; + } + + ret = regmap_write(st->regmap, ADE9078_REG_STATUS0, GENMASK(31, 0)); + if (ret) + return ret; + + return 0; +} + +/* + * ade9078_setup_iio_channels() - parses the phase nodes of the device-tree and + * creates the iio channels based on the active phases in the DT. + * @st: ade9078 device data + */ +static int ade9078_setup_iio_channels(struct ade9078_state *st) +{ + struct fwnode_handle *phase_node = NULL; + struct device *dev = &st->spi->dev; + struct iio_chan_spec *chan; + u32 phase_nr; + int ret; + + chan = devm_kcalloc(dev, + ADE9078_MAX_PHASE_NR * + ARRAY_SIZE(ade9078_a_channels), + sizeof(*ade9078_a_channels), GFP_KERNEL); + if (!chan) { + dev_err(dev, "Unable to allocate ADE9078 channels"); + return -ENOMEM; + } + st->indio_dev->num_channels = 0; + st->indio_dev->channels = chan; + + fwnode_for_each_available_child_node(dev_fwnode(dev), phase_node) { + ret = fwnode_property_read_u32(phase_node, "reg", &phase_nr); + if (ret) { + dev_err(dev, "Could not read channel reg : %d\n", ret); + return ret; + } + + switch (phase_nr) { + case ADE9078_PHASE_A_NR: + memcpy(chan, ade9078_a_channels, + sizeof(ade9078_a_channels)); + break; + case ADE9078_PHASE_B_NR: + memcpy(chan, ade9078_b_channels, + sizeof(ade9078_b_channels)); + break; + case ADE9078_PHASE_C_NR: + memcpy(chan, ade9078_c_channels, + sizeof(ade9078_c_channels)); + break; + default: + return -EINVAL; + } + + chan += AD9078_CHANNELS_PER_PHASE; + st->indio_dev->num_channels += AD9078_CHANNELS_PER_PHASE; + } + + return 0; +} + +/* + * ade9078_reset() - Reset sequence for the ADE9078, the hardware reset is + * optional in the DT. When no hardware reset has been declared a software + * reset is executed + * @st: ade9078 device data + */ +static int ade9078_reset(struct ade9078_state *st) +{ + struct gpio_desc *gpio_reset; + int ret; + + st->rst_done = false; + + gpio_reset = devm_gpiod_get_optional(&st->spi->dev, "reset", + GPIOD_OUT_HIGH); + if (IS_ERR(gpio_reset)) + return PTR_ERR(gpio_reset); + + if (gpio_reset) { + gpiod_set_value_cansleep(gpio_reset, 1); + usleep_range(1, 100); + gpiod_set_value_cansleep(gpio_reset, 0); + msleep_interruptible(50); + } else { + ret = regmap_update_bits(st->regmap, ADE9078_REG_CONFIG1, + ADE9078_SWRST_BIT, ADE9078_SWRST_BIT); + if (ret) + return ret; + usleep_range(80, 100); + } + + if (!st->rst_done) + return -EPERM; + + return 0; +} + +/* + * ade9078_setup() - initial register setup of the ade9078 + * @st: ade9078 device data + */ +static int ade9078_setup(struct ade9078_state *st) +{ + int ret = 0; + + ret = regmap_multi_reg_write(st->regmap, ade9078_reg_sequence, + ARRAY_SIZE(ade9078_reg_sequence)); + if (ret) + return ret; + + msleep_interruptible(2); + + ret = regmap_write(st->regmap, ADE9078_REG_STATUS0, GENMASK(31, 0)); + if (ret) + return ret; + + ret = regmap_write(st->regmap, ADE9078_REG_STATUS1, GENMASK(31, 0)); + if (ret) + return ret; + + return ret; +} + +static const struct iio_buffer_setup_ops ade9078_buffer_ops = { + .preenable = &ade9078_buffer_preenable, + .postdisable = &ade9078_buffer_postdisable, +}; + +static const struct iio_info ade9078_info = { + .read_raw = &ade9078_read_raw, + .write_raw = &ade9078_write_raw, + .debugfs_reg_access = &ade9078_reg_access, + .write_event_config = &ade9078_write_event_config, + .read_event_value = &ade9078_read_event_vlaue, +}; + +/* + * Regmap configuration + * The register access of the ade9078 requires a 16 bit address + * with the read flag on bit 3. This is not supported by default + * regmap functionality, thus reg_read and reg_write have been + * replaced with custom functions + */ +static const struct regmap_config ade9078_regmap_config = { + .reg_bits = 16, + .val_bits = 32, + .zero_flag_mask = true, + .cache_type = REGCACHE_RBTREE, + .reg_read = ade9078_spi_read_reg, + .reg_write = ade9078_spi_write_reg, + .volatile_reg = ade9078_is_volatile_reg, +}; + +static int ade9078_probe(struct spi_device *spi) +{ + struct iio_dev *indio_dev; + struct ade9078_state *st; + unsigned long irqflags; + struct regmap *regmap; + int irq; + int ret; + + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st)); + if (!indio_dev) { + dev_err(&spi->dev, "Unable to allocate ADE9078 IIO"); + return -ENOMEM; + } + st = iio_priv(indio_dev); + if (!st) { + dev_err(&spi->dev, + "Unable to allocate ADE9078 device structure"); + return -ENOMEM; + } + + st->rx = devm_kcalloc(&spi->dev, ADE9078_RX_DEPTH, sizeof(*st->rx), + GFP_KERNEL); + if (!st->rx) + return -ENOMEM; + + st->tx = devm_kcalloc(&spi->dev, ADE9078_TX_DEPTH, sizeof(*st->tx), + GFP_KERNEL); + if (!st->tx) + return -ENOMEM; + + regmap = devm_regmap_init(&spi->dev, NULL, spi, &ade9078_regmap_config); + if (IS_ERR(regmap)) { + dev_err(&spi->dev, "Unable to allocate ADE9078 regmap"); + return PTR_ERR(regmap); + } + spi_set_drvdata(spi, st); + + irq = of_irq_get_byname((&spi->dev)->of_node, "irq0"); + if (irq < 0) { + dev_err(&spi->dev, "Unable to find irq0"); + return -EINVAL; + } + irqflags = irq_get_trigger_type(irq); + ret = devm_request_threaded_irq(&spi->dev, irq, NULL, + ade9078_irq0_thread, + irqflags | IRQF_ONESHOT, + KBUILD_MODNAME, st); + if (ret) { + dev_err(&spi->dev, "Failed to request threaded irq: %d\n", ret); + return ret; + } + + irq = of_irq_get_byname((&spi->dev)->of_node, "irq1"); + if (irq < 0) { + dev_err(&spi->dev, "Unable to find irq1"); + return -EINVAL; + } + irqflags = irq_get_trigger_type(irq); + ret = devm_request_threaded_irq(&spi->dev, irq, NULL, + ade9078_irq1_thread, + irqflags | IRQF_ONESHOT, + KBUILD_MODNAME, st); + if (ret) { + dev_err(&spi->dev, "Failed to request threaded irq: %d\n", ret); + return ret; + } + + st->spi = spi; + + indio_dev->dev.parent = &st->spi->dev; + indio_dev->info = &ade9078_info; + indio_dev->modes = INDIO_DIRECT_MODE; + + st->regmap = regmap; + st->indio_dev = indio_dev; + ade9078_setup_iio_channels(st); + if (ret) { + dev_err(&spi->dev, "Failed to set up IIO channels"); + return ret; + } + + ret = ade9078_configure_scan(indio_dev); + if (ret) + return ret; + + ret = devm_iio_kfifo_buffer_setup(&spi->dev, indio_dev, + INDIO_BUFFER_SOFTWARE, + &ade9078_buffer_ops); + if (ret) + return ret; + + ret = devm_iio_device_register(&spi->dev, indio_dev); + if (ret) { + dev_err(&spi->dev, "Unable to register IIO device"); + return ret; + } + + ret = ade9078_reset(st); + if (ret) { + dev_err(&spi->dev, "ADE9078 reset failed"); + return ret; + } + + ret = ade9078_setup(st); + if (ret) { + dev_err(&spi->dev, "Unable to setup ADE9078"); + return ret; + } + + return ret; +}; + +static const struct spi_device_id ade9078_id[] = { + {"ade9078", 0}, + {} +}; +MODULE_DEVICE_TABLE(spi, ade9078_id); + +static const struct of_device_id ade9078_of_match[] = { + { .compatible = "adi,ade9078" }, + {} +}; +MODULE_DEVICE_TABLE(of, ade9078_of_match); + +static struct spi_driver ade9078_driver = { + .driver = { + .name = "ade9078", + }, + .probe = ade9078_probe, + .id_table = ade9078_id, +}; +module_spi_driver(ade9078_driver); + +MODULE_AUTHOR("Ciprian Hegbeli <ciprian.hegbeli@analog.com>"); +MODULE_DESCRIPTION("Analog Devices ADE9078 High Performance,Polyphase Energy Metering IC Driver"); +MODULE_LICENSE("GPL v2"); -- 2.34.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] iio: meter: add ADE9078 driver 2022-02-17 13:51 ` [PATCH 3/3] iio: meter: add ADE9078 driver chegbeli @ 2022-02-17 18:33 ` Jonathan Cameron 2022-04-07 12:56 ` Hegbeli, Ciprian 0 siblings, 1 reply; 11+ messages in thread From: Jonathan Cameron @ 2022-02-17 18:33 UTC (permalink / raw) To: chegbeli; +Cc: jic23, robh+dt, linux-iio, devicetree, linux-kernel On Thu, 17 Feb 2022 15:51:40 +0200 chegbeli <ciprian.hegbeli@analog.com> wrote: > The ADE9078 is a highly accurate, fully integrated energy metering > device. It allows the monitoring of three independent phases > simultaneously, by using seven high performances ADCs and a flexible > DSP core for a fixed fundamental signal frequency of either 50Hz or > 60Hz. > > Datasheet: > www.analog.com/media/en/technical-documentation/data-sheets/ADE9078.pdf > > Signed-off-by: chegbeli <ciprian.hegbeli@analog.com> Hi Ciprian I took a very quick end of day look so probably missed some stuff I'll pick up on in v2. Biggest issue is new ABI without docs. Various other comments inline. Plus as already observed, please use full name in from and SoB as they form part of the Developer Certificate of Origin. Thanks, Jonathan > --- > MAINTAINERS | 8 + > drivers/iio/meter/Kconfig | 13 + > drivers/iio/meter/Makefile | 2 + > drivers/iio/meter/ade9078.c | 1666 +++++++++++++++++++++++++++++++++++ > 4 files changed, 1689 insertions(+) > create mode 100644 drivers/iio/meter/ade9078.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index a2c8699e9e41..72fac9f4f837 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1101,6 +1101,14 @@ L: linux-media@vger.kernel.org > S: Maintained > F: drivers/media/i2c/ad9389b* > > +ANALOG DEVICES INC ADE9078 DRIVER > +M: Ciprian Hegbeli <ciprian.hegbeli@analog.com> > +L: linux-iio@vger.kernel.org > +S: Supported > +W: https://ez.analog.com/linux-software-drivers > +F: Documentation/devicetree/bindings/iio/meter/adi,ade9078.yaml > +F: drivers/iio/meter/ade9078.c > + > ANALOG DEVICES INC ADGS1408 DRIVER > M: Mircea Caprioru <mircea.caprioru@analog.com> > S: Supported > diff --git a/drivers/iio/meter/Kconfig b/drivers/iio/meter/Kconfig > index d1c91eed5283..b46e93af1052 100644 > --- a/drivers/iio/meter/Kconfig > +++ b/drivers/iio/meter/Kconfig > @@ -6,4 +6,17 @@ > > menu "Active energy metering" > > +config ADE9078 > + tristate "Analog Devices ADE9078 Poly Phase Multifunction Energy Metering IC Driver" > + depends on SPI > + select REGMAP_SPI > + select IIO_BUFFER > + select IIO_KFIFO_BUF > + help > + Support driver for the ADE9078Poly Phase Multifunction Energy Meter, > + select y to build the driver. > + > + To compile this driver as a module, choose M here: the module > + will be called ade9078 > + > endmenu > diff --git a/drivers/iio/meter/Makefile b/drivers/iio/meter/Makefile > index c27cba44fc0b..866b7f7dc703 100644 > --- a/drivers/iio/meter/Makefile > +++ b/drivers/iio/meter/Makefile > @@ -3,3 +3,5 @@ > # Makefile for enrgy metering drivers > # > # When adding new entries keep the list in alphabetical order > + > +obj-$(CONFIG_ADE9078) += ade9078.o > diff --git a/drivers/iio/meter/ade9078.c b/drivers/iio/meter/ade9078.c > new file mode 100644 > index 000000000000..cfcb093e4c27 > --- /dev/null > +++ b/drivers/iio/meter/ade9078.c > @@ -0,0 +1,1666 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * ADE9078 driver > + * > + * Copyright 2021 Analog Devices Inc. > + */ > + > +#include <asm/unaligned.h> Preference to put asm includes after the linux ones. > +#include <linux/bitfield.h> > +#include <linux/delay.h> > +#include <linux/gpio/consumer.h> > +#include <linux/iio/iio.h> > +#include <linux/iio/buffer.h> > +#include <linux/iio/kfifo_buf.h> > +#include <linux/iio/events.h> > +#include <linux/iio/sysfs.h> > +#include <linux/interrupt.h> > +#include <linux/module.h> > +#include <linux/of_irq.h> > +#include <linux/regmap.h> > +#include <linux/spi/spi.h> > + > +/* Address of ADE90XX registers */ > +#define ADE9078_REG_AIGAIN 0x000 > +#define ADE9078_REG_AVGAIN 0x00B > +#define ADE9078_REG_AIRMSOS 0x00C > +#define ADE9078_REG_AVRMSOS 0x00D > +#define ADE9078_REG_APGAIN 0x00E > +#define ADE9078_REG_AWATTOS 0x00F > +#define ADE9078_REG_AVAROS 0x010 > +#define ADE9078_REG_AFVAROS 0x012 > +#define ADE9078_REG_CONFIG0 0x060 > +#define ADE9078_REG_DICOEFF 0x072 > +#define ADE9078_REG_AI_PCF 0x20A > +#define ADE9078_REG_AV_PCF 0x20B > +#define ADE9078_REG_AIRMS 0x20C > +#define ADE9078_REG_AVRMS 0x20D > +#define ADE9078_REG_AWATT 0x210 > +#define ADE9078_REG_AVAR 0x211 > +#define ADE9078_REG_AVA 0x212 > +#define ADE9078_REG_AFVAR 0x214 > +#define ADE9078_REG_APF 0x216 > +#define ADE9078_REG_BI_PCF 0x22A > +#define ADE9078_REG_BV_PCF 0x22B > +#define ADE9078_REG_BIRMS 0x22C > +#define ADE9078_REG_BVRMS 0x22D > +#define ADE9078_REG_CI_PCF 0x24A > +#define ADE9078_REG_CV_PCF 0x24B > +#define ADE9078_REG_CIRMS 0x24C > +#define ADE9078_REG_CVRMS 0x24D > +#define ADE9078_REG_AWATT_ACC 0x2E5 > +#define ADE9078_REG_STATUS0 0x402 > +#define ADE9078_REG_STATUS1 0x403 > +#define ADE9078_REG_MASK0 0x405 > +#define ADE9078_REG_MASK1 0x406 > +#define ADE9078_REG_EVENT_MASK 0x407 > +#define ADE9078_REG_VLEVEL 0x40F > +#define ADE9078_REG_RUN 0x480 > +#define ADE9078_REG_CONFIG1 0x481 > +#define ADE9078_REG_ACCMODE 0x492 > +#define ADE9078_REG_CONFIG3 0x493 > +#define ADE9078_REG_ZX_LP_SEL 0x49A > +#define ADE9078_REG_WFB_CFG 0x4A0 > +#define ADE9078_REG_WFB_PG_IRQEN 0x4A1 > +#define ADE9078_REG_WFB_TRG_CFG 0x4A2 > +#define ADE9078_REG_WFB_TRG_STAT 0x4A3 > +#define ADE9078_REG_CONFIG2 0x4AF > +#define ADE9078_REG_EP_CFG 0x4B0 > +#define ADE9078_REG_EGY_TIME 0x4B2 > +#define ADE9078_REG_PGA_GAIN 0x4B9 > +#define ADE9078_REG_VERSION 0x4FE > +#define ADE9078_REG_WF_BUFF 0x800 > + > +#define ADE9078_REG_ADDR_MASK GENMASK(15, 4) > +#define ADE9078_REG_READ_BIT_MASK BIT(3) > +#define ADE9078_RX_DEPTH 6 > +#define ADE9078_TX_DEPTH 10 > + > +#define ADE9078_WF_CAP_EN_MASK BIT(4) > +#define ADE9078_WF_CAP_SEL_MASK BIT(5) > +#define ADE9078_WF_MODE_MASK GENMASK(7, 6) > +#define ADE9078_WF_SRC_MASK GENMASK(9, 8) > +#define ADE9078_WF_IN_EN_MASK BIT(12) > + > +/* > + * Configuration registers > + * PGA@0x0000. Gain of all channels=1 > + */ > +#define ADE9078_PGA_GAIN 0x0000 > + > +/* Default configuration */ > +#define ADE9078_CONFIG0 0x00000000 > + > +/* CF3/ZX pin outputs Zero crossing */ > +#define ADE9078_CONFIG1 0x0002 This looks like it could be better expressed in terms of defines for the fields contained in this register. Same for many of the ones that follow. > + > +/* Default High pass corner frequency of 1.25Hz */ > +#define ADE9078_CONFIG2 0x0A00 > + > +/* Peak and overcurrent detection disabled */ > +#define ADE9078_CONFIG3 0x0000 > + > +/* > + * 50Hz operation, 3P4W Wye configuration, signed accumulation > + * Clear bit 8 i.e. ACCMODE=0x00xx for 50Hz operation > + * ACCMODE=0x0x9x for 3Wire delta when phase B is used as reference > + */ > +#define ADE9078_ACCMODE 0x0000 > + > +/*Line period and zero crossing obtained from VA */ > +#define ADE9078_ZX_LP_SEL 0x0000 > + > +/* Disable all interrupts */ > +#define ADE9078_MASK0 0x00000000 > + > +/* Disable all interrupts */ > +#define ADE9078_MASK1 0x00000000 > + > +/* Events disabled */ > +#define ADE9078_EVENT_MASK 0x00000000 > + > +/* > + * Assuming Vnom=1/2 of full scale. > + * Refer to Technical reference manual for detailed calculations. > + */ > +#define ADE9078_VLEVEL 0x0022EA28 > + > +/* Set DICOEFF= 0xFFFFE000 when integrator is enabled */ > +#define ADE9078_DICOEFF 0x00000000 > + > +/* DSP ON */ > +#define ADE9078_RUN_ON 0xFFFFFFFF > + > +/* > + * Energy Accumulation Settings > + * Enable energy accumulation, accumulate samples at 8ksps > + * latch energy accumulation after EGYRDY > + * If accumulation is changed to half line cycle mode, change EGY_TIME > + */ > +#define ADE9078_EP_CFG 0x0011 > + > +/* Accumulate 4000 samples */ > +#define ADE9078_EGY_TIME 0x0FA0 > + > +/* > + * Constant Definitions > + * ADE9000 FDSP: 8000sps, ADE9078 FDSP: 4000sps > + */ > +#define ADE9078_FDSP 4000 > +#define ADE9078_WFB_CFG 0x0329 > +#define ADE9078_WFB_PAGE_SIZE 128 > +#define ADE9078_WFB_BYTES_IN_PAGE 4 > +#define ADE9078_WFB_PAGE_ARRAY_SIZE \ > + (ADE9078_WFB_PAGE_SIZE * ADE9078_WFB_BYTES_IN_PAGE) > +#define ADE9078_WFB_FULL_BUFF_SIZE \ > + (ADE9078_WFB_PAGE_ARRAY_SIZE * 16) > +#define ADE9078_WFB_FULL_BUFF_NR_SAMPLES \ > + (ADE9078_WFB_PAGE_SIZE * 16) > + > +#define ADE9078_SWRST_BIT BIT(0) > + > +/* Status and Mask register bits*/ > +#define ADE9078_ST0_WFB_TRIG_BIT BIT(16) > +#define ADE9078_ST0_PAGE_FULL_BIT BIT(17) > + > +#define ADE9078_ST1_ZXTOVA_BIT BIT(6) > +#define ADE9078_ST1_ZXTOVB_BIT BIT(7) > +#define ADE9078_ST1_ZXTOVC_BIT BIT(8) > +#define ADE9078_ST1_ZXVA_BIT BIT(9) > +#define ADE9078_ST1_ZXVB_BIT BIT(10) > +#define ADE9078_ST1_ZXVC_BIT BIT(11) > +#define ADE9078_ST1_ZXIA_BIT BIT(13) > +#define ADE9078_ST1_ZXIB_BIT BIT(14) > +#define ADE9078_ST1_ZXIC_BIT BIT(15) > +#define ADE9078_ST1_RSTDONE_BIT BIT(16) > +#define ADE9078_ST1_ERROR0_BIT BIT(28) > +#define ADE9078_ST1_ERROR1_BIT BIT(29) > +#define ADE9078_ST1_ERROR2_BIT BIT(30) > +#define ADE9078_ST1_ERROR3_BIT BIT(31) > +#define ADE9078_ST_ERROR \ > + (ADE9078_ST1_ERROR0 | \ > + ADE9078_ST1_ERROR1 | \ > + ADE9078_ST1_ERROR2 | \ > + ADE9078_ST1_ERROR3) > +#define ADE9078_ST1_CROSSING_FIRST 6 > +#define ADE9078_ST1_CROSSING_DEPTH 16 > + > +#define ADE9078_WFB_TRG_ZXIA_BIT BIT(3) > +#define ADE9078_WFB_TRG_ZXIB_BIT BIT(4) > +#define ADE9078_WFB_TRG_ZXIC_BIT BIT(5) > +#define ADE9078_WFB_TRG_ZXVA_BIT BIT(6) > +#define ADE9078_WFB_TRG_ZXVB_BIT BIT(7) > +#define ADE9078_WFB_TRG_ZXVC_BIT BIT(8) > + > +/* Stop when waveform buffer is full */ > +#define ADE9078_WFB_FULL_MODE 0x0 > +/* Continuous fill—stop only on enabled trigger events */ > +#define ADE9078_WFB_EN_TRIG_MODE 0x1 > +/* Continuous filling—center capture around enabled trigger events */ > +#define ADE9078_WFB_C_EN_TRIG_MODE 0x2 > +/* Continuous fill—save event address of enabled trigger events */ > +#define ADE9078_WFB_SAVE_EN_TRIG_MODE 0x3 > + > +#define ADE9078_MODE_0_1_PAGE_BIT BIT(15) > +#define ADE9078_MODE_2_PAGE_BIT BIT(7) > + > +/* > + * Full scale Codes referred from Datasheet.Respective digital codes are > + * produced when ADC inputs are at full scale. Do not Change. > + */ > +#define ADE9078_RMS_FULL_SCALE_CODES 52866837 > +#define ADE9000_WATT_FULL_SCALE_CODES 20694066 > +#define ADE9078_PCF_FULL_SCALE_CODES 74770000 > + > +/* Phase and channel definitions */ > +#define ADE9078_PHASE_A_NR 0 > +#define ADE9078_PHASE_B_NR 2 > +#define ADE9078_PHASE_C_NR 4 > +#define ADE9078_PHASE_A_NAME "A" > +#define ADE9078_PHASE_B_NAME "B" > +#define ADE9078_PHASE_C_NAME "C" These will be passed into a new macro as below (once each) so probably will be better just used inline there. > + > +#define ADE9078_SCAN_POS_IA BIT(0) > +#define ADE9078_SCAN_POS_VA BIT(1) > +#define ADE9078_SCAN_POS_IB BIT(2) > +#define ADE9078_SCAN_POS_VB BIT(3) > +#define ADE9078_SCAN_POS_IC BIT(4) > +#define ADE9078_SCAN_POS_VC BIT(5) > + > +#define ADE9078_PHASE_B_POS_BIT BIT(5) > +#define ADE9078_PHASE_C_POS_BIT BIT(6) > + > +#define ADE9078_MAX_PHASE_NR 3 > +#define AD9078_CHANNELS_PER_PHASE 9 > + > +#define ADE9078_ADDR_ADJUST(addr, chan) \ > + (((chan) << 4) | (addr)) > + > +#define ADE9078_CURRENT_CHANNEL(num, name) { \ > + .type = IIO_CURRENT, \ > + .channel = num, \ > + .extend_name = name, \ > + .address = ADE9078_ADDR_ADJUST(ADE9078_REG_AI_PCF, num), \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_SCALE) | \ > + BIT(IIO_CHAN_INFO_HARDWAREGAIN), \ > + .event_spec = ade9078_events, \ > + .num_event_specs = ARRAY_SIZE(ade9078_events), \ > + .scan_index = num, \ > + .scan_type = { \ > + .sign = 's', \ > + .realbits = 32, \ > + .storagebits = 32, \ > + .shift = 0, \ > + .endianness = IIO_BE, \ > + }, \ > +} > + > +#define ADE9078_VOLTAGE_CHANNEL(num, name) { \ > + .type = IIO_VOLTAGE, \ > + .channel = num, \ > + .extend_name = name, \ > + .address = ADE9078_ADDR_ADJUST(ADE9078_REG_AV_PCF, num), \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_SCALE) | \ > + BIT(IIO_CHAN_INFO_HARDWAREGAIN), \ > + .event_spec = ade9078_events, \ > + .num_event_specs = ARRAY_SIZE(ade9078_events), \ > + .scan_index = num + 1, \ > + .scan_type = { \ > + .sign = 's', \ > + .realbits = 32, \ > + .storagebits = 32, \ > + .shift = 0, \ > + .endianness = IIO_BE, \ > + }, \ > +} > + > +#define ADE9078_CURRENT_RMS_CHANNEL(num, name) { \ > + .type = IIO_CURRENT, \ > + .channel = num, \ > + .address = ADE9078_ADDR_ADJUST(ADE9078_REG_AIRMS, num), \ > + .extend_name = name "_rms", \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_SCALE) | \ > + BIT(IIO_CHAN_INFO_OFFSET), \ > + .scan_index = -1 \ > +} > + > +#define ADE9078_VOLTAGE_RMS_CHANNEL(num, name) { \ > + .type = IIO_VOLTAGE, \ > + .channel = num, \ > + .address = ADE9078_ADDR_ADJUST(ADE9078_REG_AVRMS, num), \ > + .extend_name = name "_rms", \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_SCALE) | \ > + BIT(IIO_CHAN_INFO_OFFSET), \ > + .scan_index = -1 \ > +} > + > +#define ADE9078_POWER_ACTIV_CHANNEL(num, name) { \ > + .type = IIO_POWER, \ > + .channel = num, \ > + .address = ADE9078_ADDR_ADJUST(ADE9078_REG_AWATT, num), \ > + .extend_name = name "_activ", \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_SCALE) | \ > + BIT(IIO_CHAN_INFO_OFFSET) | \ > + BIT(IIO_CHAN_INFO_HARDWAREGAIN), \ > + .scan_index = -1 \ > +} > + > +#define ADE9078_POWER_REACTIV_CHANNEL(num, name) { \ > + .type = IIO_POWER, \ > + .channel = num, \ > + .address = ADE9078_ADDR_ADJUST(ADE9078_REG_AVAR, num), \ > + .extend_name = name "_reactiv", \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_SCALE) | \ > + BIT(IIO_CHAN_INFO_OFFSET), \ > + .scan_index = -1 \ > +} > + > +#define ADE9078_POWER_APPARENT_CHANNEL(num, name) { \ > + .type = IIO_POWER, \ > + .channel = num, \ > + .address = ADE9078_ADDR_ADJUST(ADE9078_REG_AVA, num), \ > + .extend_name = name "_apparent", \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_SCALE), \ > + .scan_index = -1 \ > +} > + > +#define ADE9078_POWER_FUND_REACTIV_CHANNEL(num, name) { \ > + .type = IIO_POWER, \ > + .channel = num, \ > + .address = ADE9078_ADDR_ADJUST(ADE9078_REG_AFVAR, num), \ > + .extend_name = name "_fund_reactiv", \ extend_name defines new ABI. Needs documentation in Documentation/ABI/testing/sysfs-bus-iio-* and a very strong reason why it makes sense to do it this way rather than via modifiers or similar. > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_SCALE) | \ > + BIT(IIO_CHAN_INFO_OFFSET), \ > + .scan_index = -1 \ > +} > + > +#define ADE9078_POWER_FACTOR_CHANNEL(num, name) { \ > + .type = IIO_POWER, \ > + .channel = num, \ > + .address = ADE9078_ADDR_ADJUST(ADE9078_REG_APF, num), \ > + .extend_name = name "_factor", \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > + BIT(IIO_CHAN_INFO_SCALE), \ > + .scan_index = -1 \ > +} reorder the code so these defs are near the use of them. > + > +/* > + * struct ade9078_state - ade9078 specific data > + * @rst_done flag for when reset sequence irq has been received > + * @wf_mode wave form buffer mode, read datasheet for more details, > + * retrieved from DT > + * @wfb_trg wave form buffer triger configuration, read datasheet for more > + * details, retrieved from DT > + * @spi spi device associated to the ade9078 > + * @tx transmit buffer for the spi > + * @rx receive buffer for the spi > + * @xfer transfer setup used in iio buffer configuration > + * @spi_msg message transfer trough spi, used in iio buffer > + * configuration > + * @regmap register map pointer > + * @indio_dev: the IIO device > + * @trig iio trigger pointer, is connected to IRQ0 and IRQ1 > + * @rx_buff receive buffer for the iio buffer trough spi, will > + * contain the samples from the IC wave form buffer > + * @tx_buff transmit buffer for the iio buffer trough spi, used > + * in iio buffer configuration > + */ > + > +struct ade9078_state { > + bool rst_done; > + u8 wf_mode; > + u32 wfb_trg; > + struct spi_device *spi; > + u8 *tx; > + u8 *rx; > + struct spi_transfer xfer[2]; > + struct spi_message spi_msg; > + struct regmap *regmap; > + struct iio_dev *indio_dev; This is always a bad sign. If you need to go from the state back to the iio_dev then you should have passed the iio_dev in the first place - in this particular case the iio_dev should be your private data for the irq handlers, not the ade9078 state structure. > + union{ > + u8 byte[ADE9078_WFB_FULL_BUFF_SIZE]; > + __be32 word[ADE9078_WFB_FULL_BUFF_NR_SAMPLES]; > + } rx_buff ____cacheline_aligned; > + u8 tx_buff[2]; > +}; > + > +static const struct iio_event_spec ade9078_events[] = { > + { > + .type = IIO_EV_TYPE_THRESH, > + .dir = IIO_EV_DIR_EITHER, > + .mask_separate = BIT(IIO_EV_INFO_ENABLE) | > + BIT(IIO_EV_INFO_VALUE), > + }, > +}; > + > +/* IIO channels of the ade9078 for each phase individually */ > +static const struct iio_chan_spec ade9078_a_channels[] = { > + ADE9078_CURRENT_CHANNEL(ADE9078_PHASE_A_NR, ADE9078_PHASE_A_NAME), > + ADE9078_VOLTAGE_CHANNEL(ADE9078_PHASE_A_NR, ADE9078_PHASE_A_NAME), > + ADE9078_CURRENT_RMS_CHANNEL(ADE9078_PHASE_A_NR, ADE9078_PHASE_A_NAME), > + ADE9078_VOLTAGE_RMS_CHANNEL(ADE9078_PHASE_A_NR, ADE9078_PHASE_A_NAME), > + ADE9078_POWER_ACTIV_CHANNEL(ADE9078_PHASE_A_NR, ADE9078_PHASE_A_NAME), > + ADE9078_POWER_REACTIV_CHANNEL(ADE9078_PHASE_A_NR, ADE9078_PHASE_A_NAME), > + ADE9078_POWER_APPARENT_CHANNEL(ADE9078_PHASE_A_NR, > + ADE9078_PHASE_A_NAME), > + ADE9078_POWER_FUND_REACTIV_CHANNEL(ADE9078_PHASE_A_NR, > + ADE9078_PHASE_A_NAME), > + ADE9078_POWER_FACTOR_CHANNEL(ADE9078_PHASE_A_NR, ADE9078_PHASE_A_NAME), These definitely look like a case for a macro defining all the channels for a phase. > +}; > + > +static const struct iio_chan_spec ade9078_b_channels[] = { > + ADE9078_CURRENT_CHANNEL(ADE9078_PHASE_B_NR, ADE9078_PHASE_B_NAME), > + ADE9078_VOLTAGE_CHANNEL(ADE9078_PHASE_B_NR, ADE9078_PHASE_B_NAME), > + ADE9078_CURRENT_RMS_CHANNEL(ADE9078_PHASE_B_NR, ADE9078_PHASE_B_NAME), > + ADE9078_VOLTAGE_RMS_CHANNEL(ADE9078_PHASE_B_NR, ADE9078_PHASE_B_NAME), > + ADE9078_POWER_ACTIV_CHANNEL(ADE9078_PHASE_B_NR, ADE9078_PHASE_B_NAME), > + ADE9078_POWER_REACTIV_CHANNEL(ADE9078_PHASE_B_NR, ADE9078_PHASE_B_NAME), > + ADE9078_POWER_APPARENT_CHANNEL(ADE9078_PHASE_B_NR, > + ADE9078_PHASE_B_NAME), > + ADE9078_POWER_FUND_REACTIV_CHANNEL(ADE9078_PHASE_B_NR, > + ADE9078_PHASE_B_NAME), > + ADE9078_POWER_FACTOR_CHANNEL(ADE9078_PHASE_B_NR, ADE9078_PHASE_B_NAME), > +}; > + > +static const struct iio_chan_spec ade9078_c_channels[] = { > + ADE9078_CURRENT_CHANNEL(ADE9078_PHASE_C_NR, ADE9078_PHASE_C_NAME), > + ADE9078_VOLTAGE_CHANNEL(ADE9078_PHASE_C_NR, ADE9078_PHASE_C_NAME), > + ADE9078_CURRENT_RMS_CHANNEL(ADE9078_PHASE_C_NR, ADE9078_PHASE_C_NAME), > + ADE9078_VOLTAGE_RMS_CHANNEL(ADE9078_PHASE_C_NR, ADE9078_PHASE_C_NAME), > + ADE9078_POWER_ACTIV_CHANNEL(ADE9078_PHASE_C_NR, ADE9078_PHASE_C_NAME), > + ADE9078_POWER_REACTIV_CHANNEL(ADE9078_PHASE_C_NR, ADE9078_PHASE_C_NAME), > + ADE9078_POWER_APPARENT_CHANNEL(ADE9078_PHASE_C_NR, > + ADE9078_PHASE_C_NAME), > + ADE9078_POWER_FUND_REACTIV_CHANNEL(ADE9078_PHASE_C_NR, > + ADE9078_PHASE_C_NAME), > + ADE9078_POWER_FACTOR_CHANNEL(ADE9078_PHASE_C_NR, ADE9078_PHASE_C_NAME), > +}; > + > +static const struct reg_sequence ade9078_reg_sequence[] = { > + { ADE9078_REG_PGA_GAIN, ADE9078_PGA_GAIN }, > + { ADE9078_REG_CONFIG0, ADE9078_CONFIG0 }, > + { ADE9078_REG_CONFIG1, ADE9078_CONFIG1 }, > + { ADE9078_REG_CONFIG2, ADE9078_CONFIG2 }, > + { ADE9078_REG_CONFIG3, ADE9078_CONFIG3 }, > + { ADE9078_REG_ACCMODE, ADE9078_ACCMODE }, > + { ADE9078_REG_ZX_LP_SEL, ADE9078_ZX_LP_SEL }, > + { ADE9078_REG_MASK0, ADE9078_MASK0 }, > + { ADE9078_REG_MASK1, ADE9078_MASK1 }, > + { ADE9078_REG_EVENT_MASK, ADE9078_EVENT_MASK }, > + { ADE9078_REG_WFB_CFG, ADE9078_WFB_CFG }, > + { ADE9078_REG_VLEVEL, ADE9078_VLEVEL }, > + { ADE9078_REG_DICOEFF, ADE9078_DICOEFF }, > + { ADE9078_REG_EGY_TIME, ADE9078_EGY_TIME }, > + { ADE9078_REG_EP_CFG, ADE9078_EP_CFG }, > + { ADE9078_REG_RUN, ADE9078_RUN_ON } > +}; > + > +/* As mentioned below, I don't immediately understand why this can't be done with appropriate standard regmap. Perhaps you could give more details of what is missing. I'd like to see that added to regmap if possible. > + * ade9078_spi_write_reg() - ade9078 write register over SPI > + * the data format for communicating with the ade9078 over SPI > + * is very specific and can access both 32bit and 16bit registers > + * @context: void pointer to the SPI device > + * @reg: address of the of desired register > + * @val: value to be written to the ade9078 > + */ > +static int ade9078_spi_write_reg(void *context, > + unsigned int reg, > + unsigned int val) > +{ > + struct device *dev = context; > + struct spi_device *spi = to_spi_device(dev); > + struct ade9078_state *st = spi_get_drvdata(spi); > + > + u16 addr; > + int ret = 0; > + struct spi_transfer xfer[] = { > + { > + .tx_buf = st->tx, > + }, > + }; > + > + addr = FIELD_PREP(ADE9078_REG_ADDR_MASK, reg); > + > + put_unaligned_be16(addr, st->tx); > + put_unaligned_be32(val, &st->tx[2]); > + > + if (reg > ADE9078_REG_RUN && reg < ADE9078_REG_VERSION) { > + put_unaligned_be16(val, &st->tx[2]); > + xfer[0].len = 4; > + } else { > + xfer[0].len = 6; > + } > + > + ret = spi_sync_transfer(st->spi, xfer, ARRAY_SIZE(xfer)); > + if (ret) { > + dev_err(&st->spi->dev, "problem when writing register 0x%x", > + reg); > + } > + > + return ret; > +} > + > +/* > + * ade9078_spi_write_reg() - ade9078 read register over SPI > + * the data format for communicating with the ade9078 over SPI > + * is very specific and can access both 32bit and 16bit registers > + * @context: void pointer to the SPI device > + * @reg: address of the of desired register > + * @val: value to be read to the ade9078 > + */ > +static int ade9078_spi_read_reg(void *context, > + unsigned int reg, > + unsigned int *val) > +{ > + struct device *dev = context; > + struct spi_device *spi = to_spi_device(dev); > + struct ade9078_state *st = spi_get_drvdata(spi); > + > + u16 addr; > + int ret = 0; > + struct spi_transfer xfer[] = { > + { > + .tx_buf = st->tx, > + .len = 2, > + }, > + { > + .rx_buf = st->rx, > + }, > + }; > + > + addr = FIELD_PREP(ADE9078_REG_ADDR_MASK, reg) | > + ADE9078_REG_READ_BIT_MASK; > + > + put_unaligned_be16(addr, st->tx); > + > + if (reg > ADE9078_REG_RUN && reg < ADE9078_REG_VERSION) > + xfer[1].len = 4; > + else > + xfer[1].len = 6; This doesn't look like a fixed length register which is expected for regmap... Also that len should just be the rx register which you treat below as 16 bit and 32 bit (so 2 and 4, not 4 and 6). Can the larger registers be treated as bulk reads of pairs of smaller ones? > + > + ret = spi_sync_transfer(st->spi, xfer, ARRAY_SIZE(xfer)); > + if (ret) { > + dev_err(&st->spi->dev, "problem when reading register 0x%x", > + reg); > + goto err_ret; > + } > + > + //registers which are 16 bits > + if (reg > 0x480 && reg < 0x4FE) > + *val = get_unaligned_be16(st->rx); > + else > + *val = get_unaligned_be32(st->rx); > + > +err_ret: > + return ret; > +} > + > +/* > + * ade9078_is_volatile_reg() - list of ade9078 registers which should use > + * caching > + * @dev: device data > + * @reg: address of the of desired register > + */ > +static bool ade9078_is_volatile_reg(struct device *dev, unsigned int reg) > +{ > + switch (reg) { > + case ADE9078_REG_STATUS0: > + case ADE9078_REG_STATUS1: > + case ADE9078_REG_MASK0: > + case ADE9078_REG_MASK1: > + return true; > + default: > + return false; > + } > +} > + > +/* > + * ade9078_en_wfb() - enables or disables the WFBuffer in the ADE9078 > + * @st: ade9078 device data > + * @state: true for enabled; false for disabled > + */ > +static int ade9078_en_wfb(struct ade9078_state *st, bool state) > +{ > + return regmap_update_bits(st->regmap, ADE9078_REG_WFB_CFG, BIT(4), > + state ? BIT(4) : 0); > +} > + > +/* > + * ade9078_iio_push_buffer() - reads out the content of the waveform buffer and > + * pushes it to the IIO buffer. > + * @st: ade9078 device data > + */ > +static int ade9078_iio_push_buffer(struct ade9078_state *st) > +{ > + int ret; > + u32 i; > + > + ret = spi_sync(st->spi, &st->spi_msg); > + if (ret) { > + dev_err(&st->spi->dev, "SPI fail in trigger handler"); > + return ret; > + } > + > + for (i = 0; i < ADE9078_WFB_FULL_BUFF_NR_SAMPLES; i++) > + iio_push_to_buffers(st->indio_dev, &st->rx_buff.word[i]); > + > + return 0; > +} > + > +/* > + * ade9078_irq0_thread() - Thread for IRQ0. It reads Status register 0 and > + * checks for the IRQ activation. This is configured to acquire samples in to > + * the IC buffer and dump it in to the iio_buffer according to Stop When Buffer > + * Is Full Mode, Stop Filling on Trigger and Capture Around Trigger from the > + * ADE9078 Datasheet > + */ > +static irqreturn_t ade9078_irq0_thread(int irq, void *data) > +{ > + struct ade9078_state *st = data; > + u32 handled_irq = 0; > + u32 interrupts; > + u32 status; > + int ret; > + > + ret = regmap_read(st->regmap, ADE9078_REG_STATUS0, &status); > + if (ret) { > + dev_err(&st->spi->dev, "IRQ0 read status fail"); > + goto irq0_done; > + } > + > + ret = regmap_read(st->regmap, ADE9078_REG_MASK0, &interrupts); > + if (ret) { > + dev_err(&st->spi->dev, "IRQ0 read status fail"); > + goto irq0_done; > + } > + > + if ((status & ADE9078_ST0_PAGE_FULL_BIT) && > + (interrupts & ADE9078_ST0_PAGE_FULL_BIT)) { > + //Stop Filling on Trigger and Center Capture Around Trigger > + if (st->wf_mode) { > + ret = regmap_write(st->regmap, ADE9078_REG_WFB_TRG_CFG, > + st->wfb_trg); > + if (ret) { > + dev_err(&st->spi->dev, "IRQ0 WFB write fail"); > + goto irq0_done; > + } > + > + interrupts |= ADE9078_ST0_WFB_TRIG_BIT; > + > + } else { > + //Stop When Buffer Is Full Mode > + ret = ade9078_en_wfb(st, false); > + if (ret) { > + dev_err(&st->spi->dev, "IRQ0 WFB stop fail"); > + goto irq0_done; > + } > + ret = ade9078_iio_push_buffer(st); > + if (ret) { > + dev_err(&st->spi->dev, "IRQ0 IIO push fail"); > + goto irq0_done; > + } > + } > + > + //disable Page full interrupt > + interrupts &= ~ADE9078_ST0_PAGE_FULL_BIT; > + > + ret = regmap_write(st->regmap, ADE9078_REG_MASK0, interrupts); > + if (ret) { > + dev_err(&st->spi->dev, "IRQ0 MAKS0 write fail"); > + goto irq0_done; > + } > + > + handled_irq |= ADE9078_ST0_PAGE_FULL_BIT; > + } > + > + if ((status & ADE9078_ST0_WFB_TRIG_BIT) && > + (interrupts & ADE9078_ST0_WFB_TRIG_BIT)) { > + //Stop Filling on Trigger and Center Capture Around Trigger > + ret = ade9078_en_wfb(st, false); > + if (ret) { > + dev_err(&st->spi->dev, "IRQ0 WFB fail"); > + goto irq0_done; > + } > + > + ret = ade9078_iio_push_buffer(st); > + if (ret) { > + dev_err(&st->spi->dev, "IRQ0 IIO push fail @ WFB TRIG"); > + goto irq0_done; > + } > + > + handled_irq |= ADE9078_ST0_WFB_TRIG_BIT; > + } > + > + ret = regmap_write(st->regmap, ADE9078_REG_STATUS0, handled_irq); > + if (ret) > + dev_err(&st->spi->dev, "IRQ0 write status fail"); > + > +irq0_done: As below. return wherever you have a goto. This common exit just makes the code less readable. > + return IRQ_HANDLED; > +} > + > +/* > + * ade9078_irq1_thread() - Thread for IRQ1. It reads Status register 1 and > + * checks for the IRQ activation. This thread handles the reset condition and > + * the zero-crossing conditions for all 3 phases on Voltage and Current > + */ > +static irqreturn_t ade9078_irq1_thread(int irq, void *data) > +{ > + struct ade9078_state *st = data; > + struct iio_dev *indio_dev = st->indio_dev; > + struct iio_chan_spec const *chan = indio_dev->channels; > + unsigned int bit = ADE9078_ST1_CROSSING_FIRST; > + s64 timestamp = iio_get_time_ns(indio_dev); > + u32 interrupts; > + u32 result; > + u32 status; > + u32 tmp; > + int ret; > + > + //reset /* reset */ etc // isn't used for comments in IIO (or most of the rest of the kernel) > + if (!st->rst_done) { > + ret = regmap_read(st->regmap, ADE9078_REG_STATUS1, &result); > + if (ret) > + return ret; > + > + if (result & ADE9078_ST1_RSTDONE_BIT) > + st->rst_done = true; > + else > + dev_err(&st->spi->dev, "Error testing reset done"); > + > + goto irq1_done; > + } > + > + ret = regmap_read(st->regmap, ADE9078_REG_STATUS1, &status); > + if (ret) > + goto irq1_done; > + > + ret = regmap_read(st->regmap, ADE9078_REG_MASK1, &interrupts); > + if (ret) { > + dev_err(&st->spi->dev, "IRQ1 read status fail"); > + goto irq1_done; return IRQ_HANDLED; in all these paths. No point in going to a common exit that doesn't do anything. > + } > + > + //crossings > + for_each_set_bit_from(bit, (unsigned long *)&interrupts, > + ADE9078_ST1_CROSSING_DEPTH) { > + tmp = status & BIT(bit); > + > + switch (tmp) { > + case ADE9078_ST1_ZXVA_BIT: > + case ADE9078_ST1_ZXTOVA_BIT: > + case ADE9078_ST1_ZXIA_BIT: > + iio_push_event(indio_dev, > + IIO_UNMOD_EVENT_CODE(chan->type, > + ADE9078_PHASE_A_NR, > + IIO_EV_TYPE_THRESH, > + IIO_EV_DIR_EITHER), > + timestamp); > + break; > + case ADE9078_ST1_ZXVB_BIT: > + case ADE9078_ST1_ZXTOVB_BIT: > + case ADE9078_ST1_ZXIB_BIT: > + iio_push_event(indio_dev, > + IIO_UNMOD_EVENT_CODE(chan->type, > + ADE9078_PHASE_B_NR, > + IIO_EV_TYPE_THRESH, > + IIO_EV_DIR_EITHER), > + timestamp); > + break; > + case ADE9078_ST1_ZXVC_BIT: > + case ADE9078_ST1_ZXTOVC_BIT: > + case ADE9078_ST1_ZXIC_BIT: > + iio_push_event(indio_dev, > + IIO_UNMOD_EVENT_CODE(chan->type, > + ADE9078_PHASE_C_NR, > + IIO_EV_TYPE_THRESH, > + IIO_EV_DIR_EITHER), > + timestamp); > + break; > + default: > + goto irq1_done; > + } > + } > + > +irq1_done: > + return IRQ_HANDLED; > +} > + > +/* > + * ade9078_configure_scan() - sets up the transfer parameters > + * as well as the tx and rx buffers > + * @indio_dev: the IIO device > + */ > +static int ade9078_configure_scan(struct iio_dev *indio_dev) > +{ > + struct ade9078_state *st = iio_priv(indio_dev); > + u16 addr; > + > + addr = FIELD_PREP(ADE9078_REG_ADDR_MASK, ADE9078_REG_WF_BUFF) | > + ADE9078_REG_READ_BIT_MASK; > + > + put_unaligned_be16(addr, st->tx_buff); > + > + st->xfer[0].tx_buf = &st->tx_buff[0]; > + st->xfer[0].len = 2; > + > + st->xfer[1].rx_buf = &st->rx_buff.byte[0]; > + st->xfer[1].len = ADE9078_WFB_FULL_BUFF_SIZE; > + > + spi_message_init_with_transfers(&st->spi_msg, st->xfer, 2); > + return 0; > +} > + > +/* > + * ade9078_read_raw() - IIO read function > + * @indio_dev: the IIO device > + * @chan: channel specs of the ade9078 > + * @val: first half of the read value > + * @val2: second half of the read value > + * @mask: info mask of the channel > + */ > +static int ade9078_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, > + int *val2, > + long mask) > +{ > + struct ade9078_state *st = iio_priv(indio_dev); > + int measured; > + int ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + ret = iio_device_claim_direct_mode(indio_dev); > + if (ret) > + return ret; > + > + ret = regmap_read(st->regmap, chan->address, &measured); > + > + iio_device_release_direct_mode(indio_dev); > + *val = measured; Convention is to only set values on no error. so if (ret) return ret; *val = measured; return IIO_VAL_INT; > + > + return ret ?: IIO_VAL_INT; > + > + case IIO_CHAN_INFO_SCALE: > + if (chan->type == IIO_CURRENT || chan->type == IIO_VOLTAGE) { > + switch (chan->address) { > + case ADE9078_REG_AI_PCF: > + case ADE9078_REG_AV_PCF: > + case ADE9078_REG_BI_PCF: > + case ADE9078_REG_BV_PCF: > + case ADE9078_REG_CI_PCF: > + case ADE9078_REG_CV_PCF: > + *val = 1; > + *val2 = ADE9078_PCF_FULL_SCALE_CODES; > + return IIO_VAL_FRACTIONAL; > + case ADE9078_REG_AIRMS: > + case ADE9078_REG_AVRMS: > + case ADE9078_REG_BIRMS: > + case ADE9078_REG_BVRMS: > + case ADE9078_REG_CIRMS: > + case ADE9078_REG_CVRMS: > + *val = 1; > + *val2 = ADE9078_RMS_FULL_SCALE_CODES; > + return IIO_VAL_FRACTIONAL; > + default: > + return -EINVAL; > + } > + } else if (chan->type == IIO_POWER) { > + *val = 1; > + *val2 = ADE9000_WATT_FULL_SCALE_CODES; > + return IIO_VAL_FRACTIONAL; > + } else { > + return -EINVAL; > + } > + break; > + default: > + return -EINVAL; > + } > + > + return 0; I don't think you can reach this. > +} > + > +/* > + * ade9078_write_raw() - IIO write function > + * @indio_dev: the IIO device > + * @chan: channel specs of the ade9078 > + * @val: first half of the read value > + * @val2: second half of the read value > + * @mask: info mask of the channel > + */ > +static int ade9078_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int val, > + int val2, > + long mask) > +{ > + struct ade9078_state *st = iio_priv(indio_dev); > + u32 addr = 0xFFFFF; If that value is used, something went wrong. I don't think it is, so don't assign it. > + u32 tmp; > + int ret; > + > + switch (mask) { > + case IIO_CHAN_INFO_OFFSET: > + switch (chan->type) { > + case IIO_CURRENT: > + addr = ADE9078_ADDR_ADJUST(ADE9078_REG_AIRMSOS, > + chan->channel); > + break; > + case IIO_VOLTAGE: > + addr = ADE9078_ADDR_ADJUST(ADE9078_REG_AVRMSOS, > + chan->channel); > + break; > + case IIO_POWER: > + tmp = chan->address; > + tmp &= ~ADE9078_PHASE_B_POS_BIT; > + tmp &= ~ADE9078_PHASE_C_POS_BIT; > + > + switch (tmp) { > + case ADE9078_REG_AWATTOS: > + addr = ADE9078_ADDR_ADJUST(ADE9078_REG_AWATTOS, > + chan->channel); > + break; > + case ADE9078_REG_AVAR: > + addr = ADE9078_ADDR_ADJUST(ADE9078_REG_AVAROS, > + chan->channel); > + break; > + case ADE9078_REG_AFVAR: > + addr = ADE9078_ADDR_ADJUST(ADE9078_REG_AFVAROS, > + chan->channel); > + break; > + default: > + return -EINVAL; > + } > + > + break; > + default: > + return -EINVAL; > + } > + break; > + case IIO_CHAN_INFO_HARDWAREGAIN: > + switch (chan->type) { > + case IIO_CURRENT: > + addr = ADE9078_ADDR_ADJUST(ADE9078_REG_AIGAIN, > + chan->channel); > + break; > + case IIO_VOLTAGE: > + addr = ADE9078_ADDR_ADJUST(ADE9078_REG_AVGAIN, > + chan->channel); > + break; > + case IIO_POWER: > + addr = ADE9078_ADDR_ADJUST(ADE9078_REG_APGAIN, > + chan->channel); > + break; > + default: > + return -EINVAL; > + } > + break; > + default: > + return -EINVAL; > + } > + > + ret = regmap_write(st->regmap, addr, val); > + return ret; return regmap_write() > +} > + > +/* > + * ade9078_reg_access() - IIO debug register access > + * @indio_dev: the IIO device > + * @reg: register to be accessed > + * @tx_val: value to be transmitted > + * @rx_val: value to be received > + */ > +static int ade9078_reg_access(struct iio_dev *indio_dev, > + unsigned int reg, > + unsigned int tx_val, > + unsigned int *rx_val) > +{ > + struct ade9078_state *st = iio_priv(indio_dev); > + > + if (rx_val) > + return regmap_read(st->regmap, reg, rx_val); > + > + return regmap_write(st->regmap, reg, tx_val); > +} > + > +/* > + * ade9078_write_event_config() - IIO event configure to enable zero-crossing All these function description comments should be valid kernel-doc. > + * and zero-crossing timeout on voltage and current for each phases. These > + * events will also influence the trigger conditions for the buffer capture. > + */ > +static int ade9078_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 ade9078_state *st = iio_priv(indio_dev); > + u32 interrupts; > + u32 number; > + int ret; > + > + number = chan->channel; > + > + switch (number) { > + case ADE9078_PHASE_A_NR: I would use a lookup on the phase into an array of structures. The structure would then have fields for which bit to set etc for a voltage channel and for a current channel. That way this all becomes one bit of code and some const data rather that 3 sets of near identical code. If you can make this sort of thing data rather than code that is almost always the best choice. > + if (chan->type == IIO_VOLTAGE) { > + if (state) { > + interrupts |= ADE9078_ST1_ZXVA_BIT; > + interrupts |= ADE9078_ST1_ZXTOVA_BIT; > + st->wfb_trg |= ADE9078_WFB_TRG_ZXVA_BIT; > + } else { > + interrupts &= ~ADE9078_ST1_ZXVA_BIT; > + interrupts &= ~ADE9078_ST1_ZXTOVA_BIT; > + st->wfb_trg &= ~ADE9078_WFB_TRG_ZXVA_BIT; > + } > + } else if (chan->type == IIO_CURRENT) { > + if (state) { > + interrupts |= ADE9078_ST1_ZXIA_BIT; > + st->wfb_trg |= ADE9078_WFB_TRG_ZXIA_BIT; > + } else { > + interrupts &= ~ADE9078_ST1_ZXIA_BIT; > + st->wfb_trg &= ~ADE9078_WFB_TRG_ZXIA_BIT; > + } > + } > + break; > + case ADE9078_PHASE_B_NR: > + if (chan->type == IIO_VOLTAGE) { > + if (state) { > + interrupts |= ADE9078_ST1_ZXVB_BIT; > + interrupts |= ADE9078_ST1_ZXTOVB_BIT; > + st->wfb_trg |= ADE9078_WFB_TRG_ZXVB_BIT; > + } else { > + interrupts &= ~ADE9078_ST1_ZXVB_BIT; > + interrupts &= ~ADE9078_ST1_ZXTOVB_BIT; > + st->wfb_trg &= ~ADE9078_WFB_TRG_ZXVB_BIT; > + } > + } else if (chan->type == IIO_CURRENT) { > + if (state) { > + interrupts |= ADE9078_ST1_ZXIB_BIT; > + st->wfb_trg |= ADE9078_WFB_TRG_ZXIB_BIT; > + } else { > + interrupts &= ~ADE9078_ST1_ZXIB_BIT; > + st->wfb_trg &= ~ADE9078_WFB_TRG_ZXIB_BIT; > + } > + } > + break; > + case ADE9078_PHASE_C_NR: > + if (chan->type == IIO_VOLTAGE) { > + if (state) { > + interrupts |= ADE9078_ST1_ZXVC_BIT; > + interrupts |= ADE9078_ST1_ZXTOVC_BIT; > + st->wfb_trg |= ADE9078_WFB_TRG_ZXVC_BIT; > + } else { > + interrupts &= ~ADE9078_ST1_ZXVC_BIT; > + interrupts &= ~ADE9078_ST1_ZXTOVC_BIT; > + st->wfb_trg &= ~ADE9078_WFB_TRG_ZXVC_BIT; > + } > + } else if (chan->type == IIO_CURRENT) { > + if (state) { > + interrupts |= ADE9078_ST1_ZXIC_BIT; > + st->wfb_trg |= ADE9078_WFB_TRG_ZXIC_BIT; > + } else { > + interrupts &= ~ADE9078_ST1_ZXIC_BIT; > + st->wfb_trg &= ~ADE9078_WFB_TRG_ZXIC_BIT; > + } > + } > + break; > + default: > + return -EINVAL; > + } > + > + ret = regmap_write(st->regmap, ADE9078_REG_STATUS1, GENMASK(31, 0)); > + if (ret) > + return ret; > + > + return regmap_update_bits(st->regmap, ADE9078_REG_MASK1, interrupts, > + interrupts); > +} > + > +/* > + * ade9078_read_event_vlaue() - Outputs the result of the zero-crossing for > + * voltage and current for each phase. > + * Result: > + * 0 - if crossing event not set > + * 1 - if crossing event occurred > + * -1 - if crossing timeout (only for Voltages) > + */ > +static int ade9078_read_event_vlaue(struct iio_dev *indio_dev, value? If so, this should be reading the thresholds, not anything about current status of events. > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir, > + enum iio_event_info info, > + int *val, int *val2) > +{ > + struct ade9078_state *st = iio_priv(indio_dev); > + u32 handeled_irq = 0; handled > + u32 interrupts; > + u32 number; > + u32 status; > + int ret; > + > + ret = regmap_read(st->regmap, ADE9078_REG_STATUS1, &status); > + if (ret) > + return ret; > + > + ret = regmap_read(st->regmap, ADE9078_REG_MASK1, &interrupts); > + if (ret) > + return ret; > + > + *val = 0; > + number = chan->channel; > + switch (number) { > + case ADE9078_PHASE_A_NR: > + if (chan->type == IIO_VOLTAGE) { > + if (status & ADE9078_ST1_ZXVA_BIT) { > + *val = 1; > + handeled_irq |= ADE9078_ST1_ZXVA_BIT; > + } else if (status & ADE9078_ST1_ZXTOVA_BIT) { > + *val = -1; > + handeled_irq |= ADE9078_ST1_ZXTOVA_BIT; > + } > + if (!(interrupts & ADE9078_ST1_ZXTOVA_BIT)) > + *val = 0; > + } else if (chan->type == IIO_CURRENT) { > + if (status & ADE9078_ST1_ZXIA_BIT) { > + *val = 1; > + handeled_irq |= ADE9078_ST1_ZXIA_BIT; > + } > + } > + break; > + case ADE9078_PHASE_B_NR: > + if (chan->type == IIO_VOLTAGE) { > + if (status & ADE9078_ST1_ZXVB_BIT) { > + *val = 1; > + handeled_irq |= ADE9078_ST1_ZXVB_BIT; > + } else if (status & ADE9078_ST1_ZXTOVB_BIT) { > + *val = -1; > + handeled_irq |= ADE9078_ST1_ZXTOVB_BIT; > + } > + if (!(interrupts & ADE9078_ST1_ZXTOVB_BIT)) > + *val = 0; > + } else if (chan->type == IIO_CURRENT) { > + if (status & ADE9078_ST1_ZXIB_BIT) { > + *val = 1; > + handeled_irq |= ADE9078_ST1_ZXIB_BIT; > + } > + } > + break; > + case ADE9078_PHASE_C_NR: > + if (chan->type == IIO_VOLTAGE) { > + if (status & ADE9078_ST1_ZXVC_BIT) { > + *val = 1; > + handeled_irq |= ADE9078_ST1_ZXVC_BIT; > + } else if (status & ADE9078_ST1_ZXTOVC_BIT) { > + *val = -1; > + handeled_irq |= ADE9078_ST1_ZXTOVC_BIT; > + } > + if (!(interrupts & ADE9078_ST1_ZXTOVC_BIT)) > + *val = 0; > + } else if (chan->type == IIO_CURRENT) { > + if (status & ADE9078_ST1_ZXIC_BIT) { > + *val = 1; > + handeled_irq |= ADE9078_ST1_ZXIC_BIT; > + } > + } > + break; > + default: > + return -EINVAL; > + } > + > + ret = regmap_write(st->regmap, ADE9078_REG_STATUS1, handeled_irq); Are there other bits, or is this always going to write all bits that might be set? If it is all such bits then > + if (ret) > + return ret; > + > + return IIO_VAL_INT; > +} > + > +/* > + * ade9078_config_wfb() - reads the ade9078 node and configures the wave form > + * buffer based on the options set. Additionally is reads the active scan mask > + * in order to set the input data of the buffer. There are only a few available > + * input configurations permitted by the IC, any unpermitted configuration will > + * result in all channels being active. > + * @indio_dev: the IIO device Make this valid kernel-doc. Then check for any warnings by running the kernel-doc build scripts. > + */ > +static int ade9078_config_wfb(struct iio_dev *indio_dev) > +{ > + struct ade9078_state *st = iio_priv(indio_dev); > + u32 wfg_cfg_val = 0; > + u32 tmp; > + int ret; > + > + bitmap_to_arr32(&wfg_cfg_val, indio_dev->active_scan_mask, > + indio_dev->masklength); > + > + switch (wfg_cfg_val) { Use another variable so that this becomes more readable. > + case ADE9078_SCAN_POS_IA | ADE9078_SCAN_POS_VA: > + wfg_cfg_val = 0x1; > + break; > + case ADE9078_SCAN_POS_IB | ADE9078_SCAN_POS_VB: > + wfg_cfg_val = 0x2; > + break; > + case ADE9078_SCAN_POS_IC | ADE9078_SCAN_POS_VC: > + wfg_cfg_val = 0x3; > + break; > + case ADE9078_SCAN_POS_IA: > + wfg_cfg_val = 0x8; > + break; > + case ADE9078_SCAN_POS_VA: > + wfg_cfg_val = 0x9; > + break; > + case ADE9078_SCAN_POS_IB: > + wfg_cfg_val = 0xA; > + break; > + case ADE9078_SCAN_POS_VB: > + wfg_cfg_val = 0xB; > + break; > + case ADE9078_SCAN_POS_IC: > + wfg_cfg_val = 0xC; > + break; > + case ADE9078_SCAN_POS_VC: > + wfg_cfg_val = 0xD; > + break; > + default: > + wfg_cfg_val = 0x0; > + break; > + } > + > + ret = of_property_read_u32((&st->spi->dev)->of_node, "adi,wf-cap-sel", > + &tmp); Generic device properties form include/linux/properties.h only for new IIO drivers unless there is a very strong reason why it won't work. > + if (ret) { > + dev_err(&st->spi->dev, "Failed to get wf-cap-sel: %d\n", ret); > + return ret; > + } > + wfg_cfg_val |= FIELD_PREP(ADE9078_WF_CAP_SEL_MASK, tmp); > + > + ret = of_property_read_u32((&st->spi->dev)->of_node, "adi,wf-mode", > + &tmp); > + if (ret) { > + dev_err(&st->spi->dev, "Failed to get wf-mode: %d\n", ret); > + return ret; > + } > + wfg_cfg_val |= FIELD_PREP(ADE9078_WF_MODE_MASK, tmp); > + st->wf_mode = tmp; > + > + ret = of_property_read_u32((&st->spi->dev)->of_node, "adi,wf-src", > + &tmp); > + if (ret) { > + dev_err(&st->spi->dev, > + "Failed to get wf-src: %d\n", > + ret); > + return ret; > + } > + wfg_cfg_val |= FIELD_PREP(ADE9078_WF_SRC_MASK, tmp); > + > + ret = of_property_read_u32((&st->spi->dev)->of_node, "adi,wf-in-en", > + &tmp); > + if (ret) { > + dev_err(&st->spi->dev, "Failed to get wf-in-en: %d\n", ret); > + return ret; > + } > + wfg_cfg_val |= FIELD_PREP(ADE9078_WF_IN_EN_MASK, tmp); > + > + return regmap_write(st->regmap, ADE9078_REG_WFB_CFG, wfg_cfg_val); > +} > + > +/* > + * ade9078_wfb_interrupt_setup() - Configures the wave form buffer interrupt > + * according to modes > + * @st: ade9078 device data > + * @mode: modes according to datasheet; values [0-2] > + * > + * This sets the interrupt register and other registers related to the > + * interrupts according to mode [0-2] from the datasheet > + */ > +static int ade9078_wfb_interrupt_setup(struct ade9078_state *st, u8 mode) > +{ > + int ret; > + > + ret = regmap_write(st->regmap, ADE9078_REG_WFB_TRG_CFG, 0x0); > + if (ret) > + return ret; > + > + if (mode == ADE9078_WFB_FULL_MODE || mode == ADE9078_WFB_EN_TRIG_MODE) { A switch statement would be cleaner here. > + ret = regmap_write(st->regmap, ADE9078_REG_WFB_PG_IRQEN, > + ADE9078_MODE_0_1_PAGE_BIT); > + if (ret) > + return ret; > + } else if (mode == ADE9078_WFB_C_EN_TRIG_MODE) { > + ret = regmap_write(st->regmap, ADE9078_REG_WFB_PG_IRQEN, > + ADE9078_MODE_2_PAGE_BIT); > + if (ret) > + return ret; > + } > + > + ret = regmap_write(st->regmap, ADE9078_REG_STATUS0, GENMASK(31, 0)); > + if (ret) > + return ret; > + > + return regmap_update_bits(st->regmap, ADE9078_REG_MASK0, > + ADE9078_ST0_PAGE_FULL_BIT, > + ADE9078_ST0_PAGE_FULL_BIT); > + if (ret) Unreachable code... > + return ret; > + > + return 0; > +} > + > +/* > + * ade9078_buffer_preenable() - configures the waveform buffer, sets the > + * interrupts and enables the buffer > + * @indio_dev: the IIO device > + */ > +static int ade9078_buffer_preenable(struct iio_dev *indio_dev) > +{ > + struct ade9078_state *st = iio_priv(indio_dev); > + int ret; > + > + ret = ade9078_config_wfb(indio_dev); > + if (ret) > + return ret; > + > + ret = ade9078_wfb_interrupt_setup(st, st->wf_mode); > + if (ret) > + return ret; > + > + ret = ade9078_en_wfb(st, true); > + if (ret) { > + dev_err(&st->spi->dev, "Post-enable wfb enable fail"); > + return ret; > + } > + > + return 0; > +} > + > +/* > + * ade9078_buffer_postdisable() - after the iio is disable > + * this will disable the ade9078 internal buffer for acquisition > + * @indio_dev: the IIO device > + */ > +static int ade9078_buffer_postdisable(struct iio_dev *indio_dev) > +{ > + struct ade9078_state *st = iio_priv(indio_dev); > + u32 interrupts = 0; > + int ret; > + > + ret = ade9078_en_wfb(st, false); > + if (ret) { > + dev_err(&st->spi->dev, "Post-disable wfb disable fail"); > + return ret; > + } > + > + ret = regmap_write(st->regmap, ADE9078_REG_WFB_TRG_CFG, 0x0); > + if (ret) > + return ret; > + > + interrupts |= ADE9078_ST0_WFB_TRIG_BIT; > + interrupts |= ADE9078_ST0_PAGE_FULL_BIT; > + > + return regmap_update_bits(st->regmap, ADE9078_REG_MASK0, interrupts, 0); > + if (ret) { > + dev_err(&st->spi->dev, "Post-disable update maks0 fail"); > + return ret; > + } > + > + ret = regmap_write(st->regmap, ADE9078_REG_STATUS0, GENMASK(31, 0)); > + if (ret) > + return ret; > + > + return 0; return regmap_write()... > +} > + > +/* > + * ade9078_setup_iio_channels() - parses the phase nodes of the device-tree and > + * creates the iio channels based on the active phases in the DT. > + * @st: ade9078 device data > + */ > +static int ade9078_setup_iio_channels(struct ade9078_state *st) > +{ > + struct fwnode_handle *phase_node = NULL; > + struct device *dev = &st->spi->dev; > + struct iio_chan_spec *chan; > + u32 phase_nr; > + int ret; > + > + chan = devm_kcalloc(dev, > + ADE9078_MAX_PHASE_NR * > + ARRAY_SIZE(ade9078_a_channels), > + sizeof(*ade9078_a_channels), GFP_KERNEL); > + if (!chan) { > + dev_err(dev, "Unable to allocate ADE9078 channels"); > + return -ENOMEM; > + } > + st->indio_dev->num_channels = 0; > + st->indio_dev->channels = chan; > + > + fwnode_for_each_available_child_node(dev_fwnode(dev), phase_node) { > + ret = fwnode_property_read_u32(phase_node, "reg", &phase_nr); > + if (ret) { > + dev_err(dev, "Could not read channel reg : %d\n", ret); > + return ret; > + } > + > + switch (phase_nr) { > + case ADE9078_PHASE_A_NR: > + memcpy(chan, ade9078_a_channels, > + sizeof(ade9078_a_channels)); > + break; > + case ADE9078_PHASE_B_NR: > + memcpy(chan, ade9078_b_channels, > + sizeof(ade9078_b_channels)); > + break; > + case ADE9078_PHASE_C_NR: > + memcpy(chan, ade9078_c_channels, > + sizeof(ade9078_c_channels)); > + break; > + default: > + return -EINVAL; > + } > + > + chan += AD9078_CHANNELS_PER_PHASE; > + st->indio_dev->num_channels += AD9078_CHANNELS_PER_PHASE; > + } > + > + return 0; > +} > + > +/* > + * ade9078_reset() - Reset sequence for the ADE9078, the hardware reset is > + * optional in the DT. When no hardware reset has been declared a software > + * reset is executed > + * @st: ade9078 device data > + */ > +static int ade9078_reset(struct ade9078_state *st) > +{ > + struct gpio_desc *gpio_reset; > + int ret; > + > + st->rst_done = false; > + > + gpio_reset = devm_gpiod_get_optional(&st->spi->dev, "reset", > + GPIOD_OUT_HIGH); > + if (IS_ERR(gpio_reset)) > + return PTR_ERR(gpio_reset); > + > + if (gpio_reset) { > + gpiod_set_value_cansleep(gpio_reset, 1); > + usleep_range(1, 100); > + gpiod_set_value_cansleep(gpio_reset, 0); > + msleep_interruptible(50); > + } else { > + ret = regmap_update_bits(st->regmap, ADE9078_REG_CONFIG1, > + ADE9078_SWRST_BIT, ADE9078_SWRST_BIT); > + if (ret) > + return ret; > + usleep_range(80, 100); > + } > + > + if (!st->rst_done) Why EPERM? Seems more like an IO error > + return -EPERM; > + > + return 0; > +} > + > +/* > + * ade9078_setup() - initial register setup of the ade9078 > + * @st: ade9078 device data > + */ > +static int ade9078_setup(struct ade9078_state *st) > +{ > + int ret = 0; The value here is never used. Make sure you run a bunch of build tests including setting W=1 and ensuring it is clean + run at very least sparse. Some of those tests would warn about this. > + > + ret = regmap_multi_reg_write(st->regmap, ade9078_reg_sequence, > + ARRAY_SIZE(ade9078_reg_sequence)); > + if (ret) > + return ret; > + > + msleep_interruptible(2); > + > + ret = regmap_write(st->regmap, ADE9078_REG_STATUS0, GENMASK(31, 0)); > + if (ret) > + return ret; > + > + ret = regmap_write(st->regmap, ADE9078_REG_STATUS1, GENMASK(31, 0)); return regmap_write() > + if (ret) > + return ret; > + > + return ret; > +} > + > +static const struct iio_buffer_setup_ops ade9078_buffer_ops = { > + .preenable = &ade9078_buffer_preenable, > + .postdisable = &ade9078_buffer_postdisable, > +}; > + > +static const struct iio_info ade9078_info = { > + .read_raw = &ade9078_read_raw, > + .write_raw = &ade9078_write_raw, > + .debugfs_reg_access = &ade9078_reg_access, > + .write_event_config = &ade9078_write_event_config, > + .read_event_value = &ade9078_read_event_vlaue, > +}; > + > +/* > + * Regmap configuration > + * The register access of the ade9078 requires a 16 bit address > + * with the read flag on bit 3. This is not supported by default > + * regmap functionality, thus reg_read and reg_write have been > + * replaced with custom functions How big would the changes needed to support this in the regmap core be? Superficially I can't immediately see why it won't work. regmap appears to support setting flags in any of the address bytes as it uses regmap_set_work_buf_flag_mask() internally and that will set bits in any of the reg_bits/8 bytes. > + */ > +static const struct regmap_config ade9078_regmap_config = { > + .reg_bits = 16, > + .val_bits = 32, > + .zero_flag_mask = true, > + .cache_type = REGCACHE_RBTREE, > + .reg_read = ade9078_spi_read_reg, > + .reg_write = ade9078_spi_write_reg, > + .volatile_reg = ade9078_is_volatile_reg, > +}; > + > +static int ade9078_probe(struct spi_device *spi) > +{ > + struct iio_dev *indio_dev; > + struct ade9078_state *st; > + unsigned long irqflags; > + struct regmap *regmap; > + int irq; > + int ret; > + > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st)); > + if (!indio_dev) { > + dev_err(&spi->dev, "Unable to allocate ADE9078 IIO"); > + return -ENOMEM; > + } > + st = iio_priv(indio_dev); > + if (!st) { > + dev_err(&spi->dev, > + "Unable to allocate ADE9078 device structure"); This can't fail. So drop this check. > + return -ENOMEM; > + } > + > + st->rx = devm_kcalloc(&spi->dev, ADE9078_RX_DEPTH, sizeof(*st->rx), > + GFP_KERNEL); Avoid having multiple small allocations like this by making them part of the iio_priv() structure. > + if (!st->rx) > + return -ENOMEM; > + > + st->tx = devm_kcalloc(&spi->dev, ADE9078_TX_DEPTH, sizeof(*st->tx), > + GFP_KERNEL); > + if (!st->tx) > + return -ENOMEM; > + > + regmap = devm_regmap_init(&spi->dev, NULL, spi, &ade9078_regmap_config); Probably better to pass the iio_device structure and have a pointer to spi in there. > + if (IS_ERR(regmap)) { > + dev_err(&spi->dev, "Unable to allocate ADE9078 regmap"); > + return PTR_ERR(regmap); > + } > + spi_set_drvdata(spi, st); This shouldn't be needed after you've changed the passed context to provide the iio_dev structure and you can then call iio_priv() on that. > + > + irq = of_irq_get_byname((&spi->dev)->of_node, "irq0"); > + if (irq < 0) { > + dev_err(&spi->dev, "Unable to find irq0"); > + return -EINVAL; > + } > + irqflags = irq_get_trigger_type(irq); > + ret = devm_request_threaded_irq(&spi->dev, irq, NULL, > + ade9078_irq0_thread, > + irqflags | IRQF_ONESHOT, > + KBUILD_MODNAME, st); > + if (ret) { > + dev_err(&spi->dev, "Failed to request threaded irq: %d\n", ret); > + return ret; > + } > + > + irq = of_irq_get_byname((&spi->dev)->of_node, "irq1"); A generic firmware properties version of irq_get_by_name is queued in the i2c tree. Please use that here and mention it in the patch description / cover letter. I'll probably have pulled it into IIO shortly anyway. https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git/commit/?h=i2c/alert-for-acpi&id=ca0acb511c21738b32386ce0f85c284b351d919e > + if (irq < 0) { > + dev_err(&spi->dev, "Unable to find irq1"); > + return -EINVAL; > + } blank line after error checks like this make things more readable (slightly) > + irqflags = irq_get_trigger_type(irq); > + ret = devm_request_threaded_irq(&spi->dev, irq, NULL, > + ade9078_irq1_thread, > + irqflags | IRQF_ONESHOT, You shouldn't need to read and then feed through the flags. Unless something unusual is going on this will have the same affect as just passing IRQF_ONESHOT. > + KBUILD_MODNAME, st); > + if (ret) { > + dev_err(&spi->dev, "Failed to request threaded irq: %d\n", ret); > + return ret; > + } > + > + st->spi = spi; > + > + indio_dev->dev.parent = &st->spi->dev; > + indio_dev->info = &ade9078_info; > + indio_dev->modes = INDIO_DIRECT_MODE; > + > + st->regmap = regmap; > + st->indio_dev = indio_dev; > + ade9078_setup_iio_channels(st); > + if (ret) { ret = ade9078_setup_iio_channels(st); > + dev_err(&spi->dev, "Failed to set up IIO channels"); > + return ret; > + } > + > + ret = ade9078_configure_scan(indio_dev); > + if (ret) > + return ret; > + > + ret = devm_iio_kfifo_buffer_setup(&spi->dev, indio_dev, > + INDIO_BUFFER_SOFTWARE, > + &ade9078_buffer_ops); > + if (ret) > + return ret; > + > + ret = devm_iio_device_register(&spi->dev, indio_dev); Anything happening in probe after this call is usually a bad sign because this is the point at which userspace interfaces are exposed - hence it normally comes last to avoid any potential race condtions. > + if (ret) { > + dev_err(&spi->dev, "Unable to register IIO device"); > + return ret; > + } > + > + ret = ade9078_reset(st); > + if (ret) { > + dev_err(&spi->dev, "ADE9078 reset failed"); > + return ret; > + } > + > + ret = ade9078_setup(st); > + if (ret) { > + dev_err(&spi->dev, "Unable to setup ADE9078"); > + return ret; > + } > + > + return ret; > +}; > + > +static const struct spi_device_id ade9078_id[] = { > + {"ade9078", 0}, > + {} > +}; > +MODULE_DEVICE_TABLE(spi, ade9078_id); > + > +static const struct of_device_id ade9078_of_match[] = { > + { .compatible = "adi,ade9078" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, ade9078_of_match); > + > +static struct spi_driver ade9078_driver = { > + .driver = { > + .name = "ade9078", > + }, > + .probe = ade9078_probe, > + .id_table = ade9078_id, > +}; > +module_spi_driver(ade9078_driver); > + > +MODULE_AUTHOR("Ciprian Hegbeli <ciprian.hegbeli@analog.com>"); > +MODULE_DESCRIPTION("Analog Devices ADE9078 High Performance,Polyphase Energy Metering IC Driver"); Shorter might be good. Could definitely drop "High Performance" without changing the useful content in a driver description :) > +MODULE_LICENSE("GPL v2"); ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 3/3] iio: meter: add ADE9078 driver 2022-02-17 18:33 ` Jonathan Cameron @ 2022-04-07 12:56 ` Hegbeli, Ciprian 2022-04-10 15:42 ` Jonathan Cameron 0 siblings, 1 reply; 11+ messages in thread From: Hegbeli, Ciprian @ 2022-04-07 12:56 UTC (permalink / raw) To: Jonathan Cameron; +Cc: jic23, linux-iio, linux-kernel > Hi Ciprian > > I took a very quick end of day look so probably missed some stuff I'll > pick up on in v2. Biggest issue is new ABI without docs. Various > other comments inline. Plus as already observed, please use full name > in from and SoB as they form part of the Developer Certificate of Origin. > > Thanks, > > Jonathan Hi Jonathan, Thank you for the review. I wanted to answer the regmap related questions and maybe ask for some advice regarding them. There are some details in the messages below; the short versions is that it's doable but I will have to give up on some of the regmap functionality and mask some of the basic regmap functions in driver specific functions. These changes might make the code less readable. > > +static const struct reg_sequence ade9078_reg_sequence[] = { > > + { ADE9078_REG_PGA_GAIN, ADE9078_PGA_GAIN }, > > + { ADE9078_REG_CONFIG0, ADE9078_CONFIG0 }, > > + { ADE9078_REG_CONFIG1, ADE9078_CONFIG1 }, > > + { ADE9078_REG_CONFIG2, ADE9078_CONFIG2 }, > > + { ADE9078_REG_CONFIG3, ADE9078_CONFIG3 }, > > + { ADE9078_REG_ACCMODE, ADE9078_ACCMODE }, > > + { ADE9078_REG_ZX_LP_SEL, ADE9078_ZX_LP_SEL }, > > + { ADE9078_REG_MASK0, ADE9078_MASK0 }, > > + { ADE9078_REG_MASK1, ADE9078_MASK1 }, > > + { ADE9078_REG_EVENT_MASK, ADE9078_EVENT_MASK }, > > + { ADE9078_REG_WFB_CFG, ADE9078_WFB_CFG }, > > + { ADE9078_REG_VLEVEL, ADE9078_VLEVEL }, > > + { ADE9078_REG_DICOEFF, ADE9078_DICOEFF }, > > + { ADE9078_REG_EGY_TIME, ADE9078_EGY_TIME }, > > + { ADE9078_REG_EP_CFG, ADE9078_EP_CFG }, > > + { ADE9078_REG_RUN, ADE9078_RUN_ON } > > +}; CONFIG0, MASK0, MASK1, EVENT_MASK, VLEVEL, DICOEFF are 32bit registers. > > +/* > > As mentioned below, I don't immediately understand why this can't > be done with appropriate standard regmap. Perhaps you could give > more details of what is missing. I'd like to see that added to > regmap if possible. > I started implementing the driver with a standard SPI and using bulk read/write for the 32 bit registers. This however renders some of the standard features of the regmap unusable (as far as I know). For example I can't use regmap_update_bits for 32 bit registers, which happens a few times, which I have marked in the code below. I could implement a substitute function in my driver for this, but that would complicate the code (in my opinion). > > + * ade9078_spi_write_reg() - ade9078 write register over SPI > > + * the data format for communicating with the ade9078 over SPI > > + * is very specific and can access both 32bit and 16bit registers > > + * @context: void pointer to the SPI device > > + * @reg: address of the of desired register > > + * @val: value to be written to the ade9078 > > + */ > > +static int ade9078_spi_write_reg(void *context, > > + unsigned int reg, > > + unsigned int val) > > +{ > > + struct device *dev = context; > > + struct spi_device *spi = to_spi_device(dev); > > + struct ade9078_state *st = spi_get_drvdata(spi); > > > + > > + u16 addr; > > + int ret = 0; > > + struct spi_transfer xfer[] = { > > + { > > + .tx_buf = st->tx, > > + }, > > + }; > > + > > + addr = FIELD_PREP(ADE9078_REG_ADDR_MASK, reg); > > + > > + put_unaligned_be16(addr, st->tx); > > + put_unaligned_be32(val, &st->tx[2]); > > + > > + if (reg > ADE9078_REG_RUN && reg < ADE9078_REG_VERSION) { > > + put_unaligned_be16(val, &st->tx[2]); > > + xfer[0].len = 4; > > + } else { > > + xfer[0].len = 6; > > + } > > + > > + ret = spi_sync_transfer(st->spi, xfer, ARRAY_SIZE(xfer)); > > + if (ret) { > > + dev_err(&st->spi->dev, "problem when writing register > 0x%x", > > + reg); > > + } > > + > > + return ret; > > +} > > + > > +/* > > + * ade9078_spi_write_reg() - ade9078 read register over SPI > > + * the data format for communicating with the ade9078 over SPI > > + * is very specific and can access both 32bit and 16bit registers > > + * @context: void pointer to the SPI device > > + * @reg: address of the of desired register > > + * @val: value to be read to the ade9078 > > + */ > > +static int ade9078_spi_read_reg(void *context, > > + unsigned int reg, > > + unsigned int *val) > > +{ > > + struct device *dev = context; > > + struct spi_device *spi = to_spi_device(dev); > > + struct ade9078_state *st = spi_get_drvdata(spi); > > + > > + u16 addr; > > + int ret = 0; > > + struct spi_transfer xfer[] = { > > + { > > + .tx_buf = st->tx, > > + .len = 2, > > + }, > > + { > > + .rx_buf = st->rx, > > + }, > > + }; > > + > > + addr = FIELD_PREP(ADE9078_REG_ADDR_MASK, reg) | > > + ADE9078_REG_READ_BIT_MASK; > > + > > + put_unaligned_be16(addr, st->tx); > > + > > + if (reg > ADE9078_REG_RUN && reg < ADE9078_REG_VERSION) > > + xfer[1].len = 4; > > + else > > + xfer[1].len = 6; > > This doesn't look like a fixed length register which is expected > for regmap... Also that len should just be the rx register which > you treat below as 16 bit and 32 bit (so 2 and 4, not 4 and 6). > > Can the larger registers be treated as bulk reads of pairs of smaller ones? > It could, but I would still need an if or a type of "decoder" to determine if it is a 16 bit or 32 bit register, which means I still need a form of ade9078_read_reg which selects between a regmap_read function or regmap_bulk_read. This goes for write as well. > > + > > + ret = spi_sync_transfer(st->spi, xfer, ARRAY_SIZE(xfer)); > > + if (ret) { > > + dev_err(&st->spi->dev, "problem when reading register > 0x%x", > > + reg); > > + goto err_ret; > > + } > > + > > + //registers which are 16 bits > > + if (reg > 0x480 && reg < 0x4FE) > > + *val = get_unaligned_be16(st->rx); > > + else > > + *val = get_unaligned_be32(st->rx); > > + > > +err_ret: > > + return ret; > > +} > > + > > +/* > > + * ade9078_is_volatile_reg() - list of ade9078 registers which should use > > + * caching > > + * @dev: device data > > + * @reg: address of the of desired register > > + */ > > +static bool ade9078_is_volatile_reg(struct device *dev, unsigned int reg) > > +{ > > + switch (reg) { > > + case ADE9078_REG_STATUS0: > > + case ADE9078_REG_STATUS1: > > + case ADE9078_REG_MASK0: > > + case ADE9078_REG_MASK1: > > + return true; > > + default: > > + return false; > > + } > > +} > > + I left this part in for context. > > +/* > > + * ade9078_write_event_config() - IIO event configure to enable zero- > crossing > > All these function description comments should be valid kernel-doc. > > > + * and zero-crossing timeout on voltage and current for each phases. > These > > + * events will also influence the trigger conditions for the buffer capture. > > + */ > > +static int ade9078_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 ade9078_state *st = iio_priv(indio_dev); > > + u32 interrupts; > > + u32 number; > > + int ret; > > + > > + number = chan->channel; > > + > > + switch (number) { > > + case ADE9078_PHASE_A_NR: > > I would use a lookup on the phase into an array of structures. > The structure would then have fields for which bit to set etc > for a voltage channel and for a current channel. > > That way this all becomes one bit of code and some const data > rather that 3 sets of near identical code. If you can make > this sort of thing data rather than code that is almost always > the best choice. > > > + if (chan->type == IIO_VOLTAGE) { > > + if (state) { > > + interrupts |= ADE9078_ST1_ZXVA_BIT; > > + interrupts |= ADE9078_ST1_ZXTOVA_BIT; > > + st->wfb_trg |= > ADE9078_WFB_TRG_ZXVA_BIT; > > + } else { > > + interrupts &= ~ADE9078_ST1_ZXVA_BIT; > > + interrupts &= ~ADE9078_ST1_ZXTOVA_BIT; > > + st->wfb_trg &= > ~ADE9078_WFB_TRG_ZXVA_BIT; > > + } > > + } else if (chan->type == IIO_CURRENT) { > > + if (state) { > > + interrupts |= ADE9078_ST1_ZXIA_BIT; > > + st->wfb_trg |= > ADE9078_WFB_TRG_ZXIA_BIT; > > + } else { > > + interrupts &= ~ADE9078_ST1_ZXIA_BIT; > > + st->wfb_trg &= > ~ADE9078_WFB_TRG_ZXIA_BIT; > > + } > > + } > > + break; > > + case ADE9078_PHASE_B_NR: > > + if (chan->type == IIO_VOLTAGE) { > > + if (state) { > > + interrupts |= ADE9078_ST1_ZXVB_BIT; > > + interrupts |= ADE9078_ST1_ZXTOVB_BIT; > > + st->wfb_trg |= > ADE9078_WFB_TRG_ZXVB_BIT; > > + } else { > > + interrupts &= ~ADE9078_ST1_ZXVB_BIT; > > + interrupts &= ~ADE9078_ST1_ZXTOVB_BIT; > > + st->wfb_trg &= > ~ADE9078_WFB_TRG_ZXVB_BIT; > > + } > > + } else if (chan->type == IIO_CURRENT) { > > + if (state) { > > + interrupts |= ADE9078_ST1_ZXIB_BIT; > > + st->wfb_trg |= > ADE9078_WFB_TRG_ZXIB_BIT; > > + } else { > > + interrupts &= ~ADE9078_ST1_ZXIB_BIT; > > + st->wfb_trg &= > ~ADE9078_WFB_TRG_ZXIB_BIT; > > + } > > + } > > + break; > > + case ADE9078_PHASE_C_NR: > > + if (chan->type == IIO_VOLTAGE) { > > + if (state) { > > + interrupts |= ADE9078_ST1_ZXVC_BIT; > > + interrupts |= ADE9078_ST1_ZXTOVC_BIT; > > + st->wfb_trg |= > ADE9078_WFB_TRG_ZXVC_BIT; > > + } else { > > + interrupts &= ~ADE9078_ST1_ZXVC_BIT; > > + interrupts &= ~ADE9078_ST1_ZXTOVC_BIT; > > + st->wfb_trg &= > ~ADE9078_WFB_TRG_ZXVC_BIT; > > + } > > + } else if (chan->type == IIO_CURRENT) { > > + if (state) { > > + interrupts |= ADE9078_ST1_ZXIC_BIT; > > + st->wfb_trg |= > ADE9078_WFB_TRG_ZXIC_BIT; > > + } else { > > + interrupts &= ~ADE9078_ST1_ZXIC_BIT; > > + st->wfb_trg &= > ~ADE9078_WFB_TRG_ZXIC_BIT; > > + } > > + } > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + ret = regmap_write(st->regmap, ADE9078_REG_STATUS1, > GENMASK(31, 0)); > > + if (ret) > > + return ret; > > + > > + return regmap_update_bits(st->regmap, ADE9078_REG_MASK1, > interrupts, > > + interrupts); > > +} MASK1 is a 32 bit registers. > > +/* > > + * ade9078_wfb_interrupt_setup() - Configures the wave form buffer > interrupt > > + * according to modes > > + * @st: ade9078 device data > > + * @mode: modes according to datasheet; values [0-2] > > + * > > + * This sets the interrupt register and other registers related to the > > + * interrupts according to mode [0-2] from the datasheet > > + */ > > +static int ade9078_wfb_interrupt_setup(struct ade9078_state *st, u8 > mode) > > +{ > > + int ret; > > + > > + ret = regmap_write(st->regmap, ADE9078_REG_WFB_TRG_CFG, 0x0); > > + if (ret) > > + return ret; > > + > > + if (mode == ADE9078_WFB_FULL_MODE || mode == > ADE9078_WFB_EN_TRIG_MODE) { > > A switch statement would be cleaner here. > > > + ret = regmap_write(st->regmap, > ADE9078_REG_WFB_PG_IRQEN, > > + ADE9078_MODE_0_1_PAGE_BIT); > > + if (ret) > > + return ret; > > + } else if (mode == ADE9078_WFB_C_EN_TRIG_MODE) { > > + ret = regmap_write(st->regmap, > ADE9078_REG_WFB_PG_IRQEN, > > + ADE9078_MODE_2_PAGE_BIT); > > + if (ret) > > + return ret; > > + } > > + > > + ret = regmap_write(st->regmap, ADE9078_REG_STATUS0, > GENMASK(31, 0)); > > + if (ret) > > + return ret; > > + > > + return regmap_update_bits(st->regmap, ADE9078_REG_MASK0, > > + ADE9078_ST0_PAGE_FULL_BIT, > > + ADE9078_ST0_PAGE_FULL_BIT); > > + if (ret) > > Unreachable code... > > > + return ret; > > + > > + return 0; > > +} MASK0 is a 32 bit registers. > > +/* > > + * ade9078_buffer_postdisable() - after the iio is disable > > + * this will disable the ade9078 internal buffer for acquisition > > + * @indio_dev: the IIO device > > + */ > > +static int ade9078_buffer_postdisable(struct iio_dev *indio_dev) > > +{ > > + struct ade9078_state *st = iio_priv(indio_dev); > > + u32 interrupts = 0; > > + int ret; > > + > > + ret = ade9078_en_wfb(st, false); > > + if (ret) { > > + dev_err(&st->spi->dev, "Post-disable wfb disable fail"); > > + return ret; > > + } > > + > > + ret = regmap_write(st->regmap, ADE9078_REG_WFB_TRG_CFG, 0x0); > > + if (ret) > > + return ret; > > + > > + interrupts |= ADE9078_ST0_WFB_TRIG_BIT; > > + interrupts |= ADE9078_ST0_PAGE_FULL_BIT; > > + > > + return regmap_update_bits(st->regmap, ADE9078_REG_MASK0, > interrupts, 0); > > + if (ret) { > > + dev_err(&st->spi->dev, "Post-disable update maks0 fail"); > > + return ret; > > + } > > + > > + ret = regmap_write(st->regmap, ADE9078_REG_STATUS0, > GENMASK(31, 0)); > > + if (ret) > > + return ret; > > + > > + return 0; > > return regmap_write()... > > > +} MASK0 is updated again. > > +/* > > + * ade9078_setup() - initial register setup of the ade9078 > > + * @st: ade9078 device data > > + */ > > +static int ade9078_setup(struct ade9078_state *st) > > +{ > > + int ret = 0; > The value here is never used. > > Make sure you run a bunch of build tests including setting W=1 and ensuring > it > is clean + run at very least sparse. Some of those tests would warn about > this. > > > + > > + ret = regmap_multi_reg_write(st->regmap, ade9078_reg_sequence, > > + ARRAY_SIZE(ade9078_reg_sequence)); > > + if (ret) > > + return ret; > > + regmap_multi_reg_write is another function I don't think I could use at its full extent if I switch to standard regmap with bulk transfers, because I configure both 16 bit and 32 bit registres. The 32 bit registers configured in this set are listed above, near the begging of the e-mail. > > + msleep_interruptible(2); > > + > > + ret = regmap_write(st->regmap, ADE9078_REG_STATUS0, > GENMASK(31, 0)); > > + if (ret) > > + return ret; > > + > > + ret = regmap_write(st->regmap, ADE9078_REG_STATUS1, > GENMASK(31, 0)); > > return regmap_write() > > > + if (ret) > > + return ret; > > + > > + return ret; > > +} > > + I left this part in for context. > > +/* > > + * Regmap configuration > > + * The register access of the ade9078 requires a 16 bit address > > + * with the read flag on bit 3. This is not supported by default > > + * regmap functionality, thus reg_read and reg_write have been > > + * replaced with custom functions > > How big would the changes needed to support this in the regmap > core be? Superficially I can't immediately see why it won't work. > regmap appears to support setting flags in any of the address bytes > as it uses regmap_set_work_buf_flag_mask() internally and > that will set bits in any of the reg_bits/8 bytes. > > > + */ > > +static const struct regmap_config ade9078_regmap_config = { > > + .reg_bits = 16, > > + .val_bits = 32, > > + .zero_flag_mask = true, > > + .cache_type = REGCACHE_RBTREE, > > + .reg_read = ade9078_spi_read_reg, > > + .reg_write = ade9078_spi_write_reg, > > + .volatile_reg = ade9078_is_volatile_reg, > > +}; It dose work but as mentioned above it comes with an additional set of driver specific functions which will ultimately mask the underlying regmap functions in the best case scenario. All in all, I could implement the changes for V2 but given the fact that this would mean replacing some regmap functions, I decide to ask for your opinion and some guidance first. - Ciprian ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] iio: meter: add ADE9078 driver 2022-04-07 12:56 ` Hegbeli, Ciprian @ 2022-04-10 15:42 ` Jonathan Cameron 0 siblings, 0 replies; 11+ messages in thread From: Jonathan Cameron @ 2022-04-10 15:42 UTC (permalink / raw) To: Hegbeli, Ciprian; +Cc: Jonathan Cameron, linux-iio, linux-kernel On Thu, 7 Apr 2022 12:56:47 +0000 "Hegbeli, Ciprian" <Ciprian.Hegbeli@analog.com> wrote: > > Hi Ciprian > > > > I took a very quick end of day look so probably missed some stuff I'll > > pick up on in v2. Biggest issue is new ABI without docs. Various > > other comments inline. Plus as already observed, please use full name > > in from and SoB as they form part of the Developer Certificate of Origin. > > > > Thanks, > > > > Jonathan > > Hi Jonathan, > > Thank you for the review. I wanted to answer the regmap related > questions and maybe ask for some advice regarding them. There > are some details in the messages below; the short versions is that > it's doable but I will have to give up on some of the regmap > functionality and mask some of the basic regmap functions in driver > specific functions. These changes might make the code less > readable. > > > > +static const struct reg_sequence ade9078_reg_sequence[] = { > > > + { ADE9078_REG_PGA_GAIN, ADE9078_PGA_GAIN }, > > > + { ADE9078_REG_CONFIG0, ADE9078_CONFIG0 }, > > > + { ADE9078_REG_CONFIG1, ADE9078_CONFIG1 }, > > > + { ADE9078_REG_CONFIG2, ADE9078_CONFIG2 }, > > > + { ADE9078_REG_CONFIG3, ADE9078_CONFIG3 }, > > > + { ADE9078_REG_ACCMODE, ADE9078_ACCMODE }, > > > + { ADE9078_REG_ZX_LP_SEL, ADE9078_ZX_LP_SEL }, > > > + { ADE9078_REG_MASK0, ADE9078_MASK0 }, > > > + { ADE9078_REG_MASK1, ADE9078_MASK1 }, > > > + { ADE9078_REG_EVENT_MASK, ADE9078_EVENT_MASK }, > > > + { ADE9078_REG_WFB_CFG, ADE9078_WFB_CFG }, > > > + { ADE9078_REG_VLEVEL, ADE9078_VLEVEL }, > > > + { ADE9078_REG_DICOEFF, ADE9078_DICOEFF }, > > > + { ADE9078_REG_EGY_TIME, ADE9078_EGY_TIME }, > > > + { ADE9078_REG_EP_CFG, ADE9078_EP_CFG }, > > > + { ADE9078_REG_RUN, ADE9078_RUN_ON } > > > +}; > > CONFIG0, MASK0, MASK1, EVENT_MASK, VLEVEL, DICOEFF are > 32bit registers. Ah. I'd missed that. Indeed changes the whole discussion. > > > > +/* > > > > As mentioned below, I don't immediately understand why this can't > > be done with appropriate standard regmap. Perhaps you could give > > more details of what is missing. I'd like to see that added to > > regmap if possible. > > > > I started implementing the driver with a standard SPI and using > bulk read/write for the 32 bit registers. This however renders some > of the standard features of the regmap unusable (as far as I know). > For example I can't use regmap_update_bits for 32 bit registers, > which happens a few times, which I have marked in the code below. > I could implement a substitute function in my driver for this, but that > would complicate the code (in my opinion). Agreed - regmap doesn't fit well in my opinion when there are a mixture of register sizes. This may just be a case where you indeed should just not use regmap at all. It's challenging to make it work and sometimes it's not sensible to bother. > > > > + * ade9078_spi_write_reg() - ade9078 write register over SPI > > > + * the data format for communicating with the ade9078 over SPI > > > + * is very specific and can access both 32bit and 16bit registers > > > + * @context: void pointer to the SPI device > > > + * @reg: address of the of desired register > > > + * @val: value to be written to the ade9078 > > > + */ > > > +static int ade9078_spi_write_reg(void *context, > > > + unsigned int reg, > > > + unsigned int val) > > > +{ > > > + struct device *dev = context; > > > + struct spi_device *spi = to_spi_device(dev); > > > + struct ade9078_state *st = spi_get_drvdata(spi); > > > > > + > > > + u16 addr; > > > + int ret = 0; > > > + struct spi_transfer xfer[] = { > > > + { > > > + .tx_buf = st->tx, > > > + }, > > > + }; > > > + > > > + addr = FIELD_PREP(ADE9078_REG_ADDR_MASK, reg); > > > + > > > + put_unaligned_be16(addr, st->tx); > > > + put_unaligned_be32(val, &st->tx[2]); > > > + > > > + if (reg > ADE9078_REG_RUN && reg < ADE9078_REG_VERSION) { > > > + put_unaligned_be16(val, &st->tx[2]); > > > + xfer[0].len = 4; > > > + } else { > > > + xfer[0].len = 6; > > > + } > > > + > > > + ret = spi_sync_transfer(st->spi, xfer, ARRAY_SIZE(xfer)); > > > + if (ret) { > > > + dev_err(&st->spi->dev, "problem when writing register > > 0x%x", > > > + reg); > > > + } > > > + > > > + return ret; > > > +} > > > + > > > +/* > > > + * ade9078_spi_write_reg() - ade9078 read register over SPI > > > + * the data format for communicating with the ade9078 over SPI > > > + * is very specific and can access both 32bit and 16bit registers > > > + * @context: void pointer to the SPI device > > > + * @reg: address of the of desired register > > > + * @val: value to be read to the ade9078 > > > + */ > > > +static int ade9078_spi_read_reg(void *context, > > > + unsigned int reg, > > > + unsigned int *val) > > > +{ > > > + struct device *dev = context; > > > + struct spi_device *spi = to_spi_device(dev); > > > + struct ade9078_state *st = spi_get_drvdata(spi); > > > + > > > + u16 addr; > > > + int ret = 0; > > > + struct spi_transfer xfer[] = { > > > + { > > > + .tx_buf = st->tx, > > > + .len = 2, > > > + }, > > > + { > > > + .rx_buf = st->rx, > > > + }, > > > + }; > > > + > > > + addr = FIELD_PREP(ADE9078_REG_ADDR_MASK, reg) | > > > + ADE9078_REG_READ_BIT_MASK; > > > + > > > + put_unaligned_be16(addr, st->tx); > > > + > > > + if (reg > ADE9078_REG_RUN && reg < ADE9078_REG_VERSION) > > > + xfer[1].len = 4; > > > + else > > > + xfer[1].len = 6; > > > > This doesn't look like a fixed length register which is expected > > for regmap... Also that len should just be the rx register which > > you treat below as 16 bit and 32 bit (so 2 and 4, not 4 and 6). > > > > Can the larger registers be treated as bulk reads of pairs of smaller ones? > > > > It could, but I would still need an if or a type of "decoder" to determine > if it is a 16 bit or 32 bit register, which means I still need a form of > ade9078_read_reg which selects between a regmap_read function > or regmap_bulk_read. This goes for write as well. My assumption (wrong :) was that this was the normal 'large register' case where we were actually looking at 2 registers that had to always be read as a pair to make up the large register. That isn't true here. > > > > + > > > + ret = spi_sync_transfer(st->spi, xfer, ARRAY_SIZE(xfer)); > > > + if (ret) { > > > + dev_err(&st->spi->dev, "problem when reading register > > 0x%x", > > > + reg); > > > + goto err_ret; > > > + } > > > + > > > + //registers which are 16 bits > > > + if (reg > 0x480 && reg < 0x4FE) > > > + *val = get_unaligned_be16(st->rx); > > > + else > > > + *val = get_unaligned_be32(st->rx); > > > + > > > +err_ret: > > > + return ret; > > > +} > > > + > > > +/* > > > + * ade9078_is_volatile_reg() - list of ade9078 registers which should use > > > + * caching > > > + * @dev: device data > > > + * @reg: address of the of desired register > > > + */ > > > +static bool ade9078_is_volatile_reg(struct device *dev, unsigned int reg) > > > +{ > > > + switch (reg) { > > > + case ADE9078_REG_STATUS0: > > > + case ADE9078_REG_STATUS1: > > > + case ADE9078_REG_MASK0: > > > + case ADE9078_REG_MASK1: > > > + return true; > > > + default: > > > + return false; > > > + } > > > +} > > > + > > I left this part in for context. > > > > +/* > > > + * ade9078_write_event_config() - IIO event configure to enable zero- > > crossing > > > > All these function description comments should be valid kernel-doc. > > > > > + * and zero-crossing timeout on voltage and current for each phases. > > These > > > + * events will also influence the trigger conditions for the buffer capture. > > > + */ > > > +static int ade9078_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 ade9078_state *st = iio_priv(indio_dev); > > > + u32 interrupts; > > > + u32 number; > > > + int ret; > > > + > > > + number = chan->channel; > > > + > > > + switch (number) { > > > + case ADE9078_PHASE_A_NR: > > > > I would use a lookup on the phase into an array of structures. > > The structure would then have fields for which bit to set etc > > for a voltage channel and for a current channel. > > > > That way this all becomes one bit of code and some const data > > rather that 3 sets of near identical code. If you can make > > this sort of thing data rather than code that is almost always > > the best choice. > > > > > + if (chan->type == IIO_VOLTAGE) { > > > + if (state) { > > > + interrupts |= ADE9078_ST1_ZXVA_BIT; > > > + interrupts |= ADE9078_ST1_ZXTOVA_BIT; > > > + st->wfb_trg |= > > ADE9078_WFB_TRG_ZXVA_BIT; > > > + } else { > > > + interrupts &= ~ADE9078_ST1_ZXVA_BIT; > > > + interrupts &= ~ADE9078_ST1_ZXTOVA_BIT; > > > + st->wfb_trg &= > > ~ADE9078_WFB_TRG_ZXVA_BIT; > > > + } > > > + } else if (chan->type == IIO_CURRENT) { > > > + if (state) { > > > + interrupts |= ADE9078_ST1_ZXIA_BIT; > > > + st->wfb_trg |= > > ADE9078_WFB_TRG_ZXIA_BIT; > > > + } else { > > > + interrupts &= ~ADE9078_ST1_ZXIA_BIT; > > > + st->wfb_trg &= > > ~ADE9078_WFB_TRG_ZXIA_BIT; > > > + } > > > + } > > > + break; > > > + case ADE9078_PHASE_B_NR: > > > + if (chan->type == IIO_VOLTAGE) { > > > + if (state) { > > > + interrupts |= ADE9078_ST1_ZXVB_BIT; > > > + interrupts |= ADE9078_ST1_ZXTOVB_BIT; > > > + st->wfb_trg |= > > ADE9078_WFB_TRG_ZXVB_BIT; > > > + } else { > > > + interrupts &= ~ADE9078_ST1_ZXVB_BIT; > > > + interrupts &= ~ADE9078_ST1_ZXTOVB_BIT; > > > + st->wfb_trg &= > > ~ADE9078_WFB_TRG_ZXVB_BIT; > > > + } > > > + } else if (chan->type == IIO_CURRENT) { > > > + if (state) { > > > + interrupts |= ADE9078_ST1_ZXIB_BIT; > > > + st->wfb_trg |= > > ADE9078_WFB_TRG_ZXIB_BIT; > > > + } else { > > > + interrupts &= ~ADE9078_ST1_ZXIB_BIT; > > > + st->wfb_trg &= > > ~ADE9078_WFB_TRG_ZXIB_BIT; > > > + } > > > + } > > > + break; > > > + case ADE9078_PHASE_C_NR: > > > + if (chan->type == IIO_VOLTAGE) { > > > + if (state) { > > > + interrupts |= ADE9078_ST1_ZXVC_BIT; > > > + interrupts |= ADE9078_ST1_ZXTOVC_BIT; > > > + st->wfb_trg |= > > ADE9078_WFB_TRG_ZXVC_BIT; > > > + } else { > > > + interrupts &= ~ADE9078_ST1_ZXVC_BIT; > > > + interrupts &= ~ADE9078_ST1_ZXTOVC_BIT; > > > + st->wfb_trg &= > > ~ADE9078_WFB_TRG_ZXVC_BIT; > > > + } > > > + } else if (chan->type == IIO_CURRENT) { > > > + if (state) { > > > + interrupts |= ADE9078_ST1_ZXIC_BIT; > > > + st->wfb_trg |= > > ADE9078_WFB_TRG_ZXIC_BIT; > > > + } else { > > > + interrupts &= ~ADE9078_ST1_ZXIC_BIT; > > > + st->wfb_trg &= > > ~ADE9078_WFB_TRG_ZXIC_BIT; > > > + } > > > + } > > > + break; > > > + default: > > > + return -EINVAL; > > > + } > > > + > > > + ret = regmap_write(st->regmap, ADE9078_REG_STATUS1, > > GENMASK(31, 0)); > > > + if (ret) > > > + return ret; > > > + > > > + return regmap_update_bits(st->regmap, ADE9078_REG_MASK1, > > interrupts, > > > + interrupts); > > > +} > > MASK1 is a 32 bit registers. > > > > +/* > > > + * ade9078_wfb_interrupt_setup() - Configures the wave form buffer > > interrupt > > > + * according to modes > > > + * @st: ade9078 device data > > > + * @mode: modes according to datasheet; values [0-2] > > > + * > > > + * This sets the interrupt register and other registers related to the > > > + * interrupts according to mode [0-2] from the datasheet > > > + */ > > > +static int ade9078_wfb_interrupt_setup(struct ade9078_state *st, u8 > > mode) > > > +{ > > > + int ret; > > > + > > > + ret = regmap_write(st->regmap, ADE9078_REG_WFB_TRG_CFG, 0x0); > > > + if (ret) > > > + return ret; > > > + > > > + if (mode == ADE9078_WFB_FULL_MODE || mode == > > ADE9078_WFB_EN_TRIG_MODE) { > > > > A switch statement would be cleaner here. > > > > > + ret = regmap_write(st->regmap, > > ADE9078_REG_WFB_PG_IRQEN, > > > + ADE9078_MODE_0_1_PAGE_BIT); > > > + if (ret) > > > + return ret; > > > + } else if (mode == ADE9078_WFB_C_EN_TRIG_MODE) { > > > + ret = regmap_write(st->regmap, > > ADE9078_REG_WFB_PG_IRQEN, > > > + ADE9078_MODE_2_PAGE_BIT); > > > + if (ret) > > > + return ret; > > > + } > > > + > > > + ret = regmap_write(st->regmap, ADE9078_REG_STATUS0, > > GENMASK(31, 0)); > > > + if (ret) > > > + return ret; > > > + > > > + return regmap_update_bits(st->regmap, ADE9078_REG_MASK0, > > > + ADE9078_ST0_PAGE_FULL_BIT, > > > + ADE9078_ST0_PAGE_FULL_BIT); > > > + if (ret) > > > > Unreachable code... > > > > > + return ret; > > > + > > > + return 0; > > > +} > > MASK0 is a 32 bit registers. > > > > +/* > > > + * ade9078_buffer_postdisable() - after the iio is disable > > > + * this will disable the ade9078 internal buffer for acquisition > > > + * @indio_dev: the IIO device > > > + */ > > > +static int ade9078_buffer_postdisable(struct iio_dev *indio_dev) > > > +{ > > > + struct ade9078_state *st = iio_priv(indio_dev); > > > + u32 interrupts = 0; > > > + int ret; > > > + > > > + ret = ade9078_en_wfb(st, false); > > > + if (ret) { > > > + dev_err(&st->spi->dev, "Post-disable wfb disable fail"); > > > + return ret; > > > + } > > > + > > > + ret = regmap_write(st->regmap, ADE9078_REG_WFB_TRG_CFG, 0x0); > > > + if (ret) > > > + return ret; > > > + > > > + interrupts |= ADE9078_ST0_WFB_TRIG_BIT; > > > + interrupts |= ADE9078_ST0_PAGE_FULL_BIT; > > > + > > > + return regmap_update_bits(st->regmap, ADE9078_REG_MASK0, > > interrupts, 0); > > > + if (ret) { > > > + dev_err(&st->spi->dev, "Post-disable update maks0 fail"); > > > + return ret; > > > + } > > > + > > > + ret = regmap_write(st->regmap, ADE9078_REG_STATUS0, > > GENMASK(31, 0)); > > > + if (ret) > > > + return ret; > > > + > > > + return 0; > > > > return regmap_write()... > > > > > +} > > MASK0 is updated again. > > > > +/* > > > + * ade9078_setup() - initial register setup of the ade9078 > > > + * @st: ade9078 device data > > > + */ > > > +static int ade9078_setup(struct ade9078_state *st) > > > +{ > > > + int ret = 0; > > The value here is never used. > > > > Make sure you run a bunch of build tests including setting W=1 and ensuring > > it > > is clean + run at very least sparse. Some of those tests would warn about > > this. > > > > > + > > > + ret = regmap_multi_reg_write(st->regmap, ade9078_reg_sequence, > > > + ARRAY_SIZE(ade9078_reg_sequence)); > > > + if (ret) > > > + return ret; > > > + > > regmap_multi_reg_write is another function I don't think I could use at its > full extent if I switch to standard regmap with bulk transfers, because I > configure both 16 bit and 32 bit registres. The 32 bit registers configured > in this set are listed above, near the begging of the e-mail. > > > > + msleep_interruptible(2); > > > + > > > + ret = regmap_write(st->regmap, ADE9078_REG_STATUS0, > > GENMASK(31, 0)); > > > + if (ret) > > > + return ret; > > > + > > > + ret = regmap_write(st->regmap, ADE9078_REG_STATUS1, > > GENMASK(31, 0)); > > > > return regmap_write() > > > > > + if (ret) > > > + return ret; > > > + > > > + return ret; > > > +} > > > + > > I left this part in for context. > > > > +/* > > > + * Regmap configuration > > > + * The register access of the ade9078 requires a 16 bit address > > > + * with the read flag on bit 3. This is not supported by default > > > + * regmap functionality, thus reg_read and reg_write have been > > > + * replaced with custom functions > > > > How big would the changes needed to support this in the regmap > > core be? Superficially I can't immediately see why it won't work. > > regmap appears to support setting flags in any of the address bytes > > as it uses regmap_set_work_buf_flag_mask() internally and > > that will set bits in any of the reg_bits/8 bytes. > > > > > + */ > > > +static const struct regmap_config ade9078_regmap_config = { > > > + .reg_bits = 16, > > > + .val_bits = 32, > > > + .zero_flag_mask = true, > > > + .cache_type = REGCACHE_RBTREE, > > > + .reg_read = ade9078_spi_read_reg, > > > + .reg_write = ade9078_spi_write_reg, > > > + .volatile_reg = ade9078_is_volatile_reg, > > > +}; > > It dose work but as mentioned above it comes with an additional set > of driver specific functions which will ultimately mask the underlying > regmap functions in the best case scenario. > > All in all, I could implement the changes for V2 but given the fact that > this would mean replacing some regmap functions, I decide to ask for your > opinion and some guidance first. I'd just drop regmap and do things the old fashions (and more flexible) way. Whilst the device is register based - mixed sized registers are a horrible hardware quirk so it doesn't map (afaik) to regmap. Jonathan > > - Ciprian ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-04-10 15:34 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-02-17 13:51 [PATCH 0/3] Add ADE9078 Driver chegbeli 2022-02-17 13:51 ` [PATCH 1/3] iio: add meter subdirectory chegbeli 2022-02-17 15:33 ` Krzysztof Kozlowski 2022-02-17 18:34 ` Jonathan Cameron 2022-02-17 13:51 ` [PATCH 2/3] dt-bindings: iio: add ADE9078 chegbeli 2022-02-17 15:44 ` Krzysztof Kozlowski 2022-02-21 2:36 ` Rob Herring 2022-02-17 13:51 ` [PATCH 3/3] iio: meter: add ADE9078 driver chegbeli 2022-02-17 18:33 ` Jonathan Cameron 2022-04-07 12:56 ` Hegbeli, Ciprian 2022-04-10 15:42 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).