* [PATCH v5 0/3] add support for ad777x family
@ 2024-09-12 12:15 Ramona Alexandra Nechita
2024-09-12 12:15 ` [PATCH v5 1/3] dt-bindings: iio: adc: add a7779 doc Ramona Alexandra Nechita
` (2 more replies)
0 siblings, 3 replies; 24+ messages in thread
From: Ramona Alexandra Nechita @ 2024-09-12 12:15 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Cosmin Tanislav,
Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Nuno Sa, David Lechner, Andy Shevchenko, Marcelo Schmitt,
Olivier Moysan, Dumitru Ceclan, Matteo Martelli,
João Paulo Gonçalves, Alisa-Dariana Roman,
Ivan Mikhaylov, AngeloGioacchino Del Regno, Marius Cristea,
Ramona Nechita, linux-iio, linux-kernel, devicetree
v4:
* https://lore.kernel.org/all/20240724155517.12470-1-ramona.nechita@analog.com/
v5:
* Patch1:
- removed yaml unnecessary nodes
- removed vref_supply from yaml altogether, as it is not used by the driver
- documented interrupt line in yaml file
- updated commit message with short description
* Patch2:
- version downgrade in sysfs-bus-iio file due to combinig the two drivers'
attributes together (one was available in 6.1 and the other in 6.2 and it
seemed right to leave it as 6.1)
* Patch3:
- removed comments with old code
- fix commit messages
- removed power_mode from driver private structure
- used spi_write for ad7779_spi_write function
- fixed various indents and line wraps, typos etc
- used goto in trig_handler to get rid of duplicate code
- switched to iio_for_each_active_channel
- removed unnecessary trig setting (done by devm already)
- removed __maybe_unused from driver suspend/resume functions
Ramona Alexandra Nechita (3):
dt-bindings: iio: adc: add a7779 doc
Documentation: ABI: added filter mode doc in sysfs-bus-iio
drivers: iio: adc: add support for ad777x family
Documentation/ABI/testing/sysfs-bus-iio | 22 +
.../ABI/testing/sysfs-bus-iio-adc-ad4130 | 46 -
.../bindings/iio/adc/adi,ad7779.yaml | 84 ++
drivers/iio/adc/Kconfig | 11 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ad7779.c | 917 ++++++++++++++++++
6 files changed, 1035 insertions(+), 46 deletions(-)
delete mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad4130
create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7779.yaml
create mode 100644 drivers/iio/adc/ad7779.c
--
2.43.0
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH v5 1/3] dt-bindings: iio: adc: add a7779 doc 2024-09-12 12:15 [PATCH v5 0/3] add support for ad777x family Ramona Alexandra Nechita @ 2024-09-12 12:15 ` Ramona Alexandra Nechita 2024-09-12 13:15 ` Rob Herring (Arm) 2024-09-14 16:38 ` Jonathan Cameron 2024-09-12 12:15 ` [PATCH v5 2/3] Documentation: ABI: added filter mode doc in sysfs-bus-iio Ramona Alexandra Nechita 2024-09-12 12:15 ` [PATCH v5 3/3] drivers: iio: adc: add support for ad777x family Ramona Alexandra Nechita 2 siblings, 2 replies; 24+ messages in thread From: Ramona Alexandra Nechita @ 2024-09-12 12:15 UTC (permalink / raw) To: Jonathan Cameron, Lars-Peter Clausen, Cosmin Tanislav, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Nuno Sa, Andy Shevchenko, David Lechner, Marcelo Schmitt, Olivier Moysan, Dumitru Ceclan, Matteo Martelli, João Paulo Gonçalves, Alisa-Dariana Roman, Marius Cristea, AngeloGioacchino Del Regno, Ramona Nechita, linux-iio, linux-kernel, devicetree Add dt bindings for AD7779 8-channel, simultaneous sampling ADC family with eight full Σ-Δ ADCs on chip and ultra-low input current to allow direct sensor connection. Signed-off-by: Ramona Alexandra Nechita <ramona.nechita@analog.com> --- .../bindings/iio/adc/adi,ad7779.yaml | 84 +++++++++++++++++++ 1 file changed, 84 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7779.yaml diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7779.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7779.yaml new file mode 100644 index 000000000000..0ed5ec5dd8fe --- /dev/null +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7779.yaml @@ -0,0 +1,84 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/adi,ad7779.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Analog Devices AD777X family 8-Channel, 24-Bit, Simultaneous Sampling ADCs + +maintainers: + - Ramona Nechita <ramona.nechita@analog.com> + +description: | + The AD777X family consist of 8-channel, simultaneous sampling analog-to- + digital converter (ADC). Eight full Σ-Δ ADCs are on-chip. The + AD7771 provides an ultralow input current to allow direct sensor + connection. Each input channel has a programmable gain stage + allowing gains of 1, 2, 4, and 8 to map lower amplitude sensor + outputs into the full-scale ADC input range, maximizing the + dynamic range of the signal chain. + + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7770.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7771.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7779.pdf + +$ref: /schemas/spi/spi-peripheral-props.yaml# + +properties: + compatible: + enum: + - adi,ad7770 + - adi,ad7771 + - adi,ad7779 + + reg: + maxItems: 1 + + '#address-cells': + const: 1 + + '#size-cells': + const: 0 + + clocks: + maxItems: 1 + + interrupts: + maxItems: 1 + description: + Interrupt line for DRDY signal which indicates the end of conversion + independently of the interface selected to read back the Σ-∆ conversion. + + start-gpios: + description: + Pin that controls start synchronization pulse. + maxItems: 1 + + reset-gpios: + maxItems: 1 + +required: + - compatible + - reg + - clocks + - interrupts + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + spi { + #address-cells = <1>; + #size-cells = <0>; + + adc@0 { + compatible = "adi,ad7779"; + reg = <0>; + vref-supply = <&vref>; + start-gpios = <&gpio0 87 GPIO_ACTIVE_LOW>; + reset-gpios = <&gpio0 93 GPIO_ACTIVE_LOW>; + clocks = <&adc_clk>; + }; + }; +... -- 2.43.0 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v5 1/3] dt-bindings: iio: adc: add a7779 doc 2024-09-12 12:15 ` [PATCH v5 1/3] dt-bindings: iio: adc: add a7779 doc Ramona Alexandra Nechita @ 2024-09-12 13:15 ` Rob Herring (Arm) 2024-09-14 16:38 ` Jonathan Cameron 1 sibling, 0 replies; 24+ messages in thread From: Rob Herring (Arm) @ 2024-09-12 13:15 UTC (permalink / raw) To: Ramona Alexandra Nechita Cc: Cosmin Tanislav, Conor Dooley, devicetree, linux-kernel, Marcelo Schmitt, David Lechner, Krzysztof Kozlowski, Matteo Martelli, Michael Hennerich, Alisa-Dariana Roman, Andy Shevchenko, Olivier Moysan, Lars-Peter Clausen, Dumitru Ceclan, João Paulo Gonçalves, Marius Cristea, linux-iio, Nuno Sa, Jonathan Cameron, AngeloGioacchino Del Regno On Thu, 12 Sep 2024 15:15:45 +0300, Ramona Alexandra Nechita wrote: > Add dt bindings for AD7779 8-channel, simultaneous sampling ADC > family with eight full Σ-Δ ADCs on chip and ultra-low input > current to allow direct sensor connection. > > Signed-off-by: Ramona Alexandra Nechita <ramona.nechita@analog.com> > --- > .../bindings/iio/adc/adi,ad7779.yaml | 84 +++++++++++++++++++ > 1 file changed, 84 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7779.yaml > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/adi,ad7779.example.dtb: adc@0: Unevaluated properties are not allowed ('vref-supply' was unexpected) from schema $id: http://devicetree.org/schemas/iio/adc/adi,ad7779.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/iio/adc/adi,ad7779.example.dtb: adc@0: 'oneOf' conditional failed, one must be fixed: 'interrupts' is a required property 'interrupts-extended' is a required property from schema $id: http://devicetree.org/schemas/iio/adc/adi,ad7779.yaml# doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240912121609.13438-2-ramona.nechita@analog.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 1/3] dt-bindings: iio: adc: add a7779 doc 2024-09-12 12:15 ` [PATCH v5 1/3] dt-bindings: iio: adc: add a7779 doc Ramona Alexandra Nechita 2024-09-12 13:15 ` Rob Herring (Arm) @ 2024-09-14 16:38 ` Jonathan Cameron 1 sibling, 0 replies; 24+ messages in thread From: Jonathan Cameron @ 2024-09-14 16:38 UTC (permalink / raw) To: Ramona Alexandra Nechita Cc: Lars-Peter Clausen, Cosmin Tanislav, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Nuno Sa, Andy Shevchenko, David Lechner, Marcelo Schmitt, Olivier Moysan, Dumitru Ceclan, Matteo Martelli, João Paulo Gonçalves, Alisa-Dariana Roman, Marius Cristea, AngeloGioacchino Del Regno, linux-iio, linux-kernel, devicetree On Thu, 12 Sep 2024 15:15:45 +0300 Ramona Alexandra Nechita <ramona.nechita@analog.com> wrote: > Add dt bindings for AD7779 8-channel, simultaneous sampling ADC > family with eight full Σ-Δ ADCs on chip and ultra-low input > current to allow direct sensor connection. Typo in device name in the title Also no need to say 'doc' when in dt-bindings. dt-bindings: iio: adc: add adi,ad7779 > > Signed-off-by: Ramona Alexandra Nechita <ramona.nechita@analog.com> > --- > .../bindings/iio/adc/adi,ad7779.yaml | 84 +++++++++++++++++++ > 1 file changed, 84 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7779.yaml > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7779.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7779.yaml > new file mode 100644 > index 000000000000..0ed5ec5dd8fe > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7779.yaml > @@ -0,0 +1,84 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/adc/adi,ad7779.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Analog Devices AD777X family 8-Channel, 24-Bit, Simultaneous Sampling ADCs > + > +maintainers: > + - Ramona Nechita <ramona.nechita@analog.com> > + > +description: | > + The AD777X family consist of 8-channel, simultaneous sampling analog-to- > + digital converter (ADC). Eight full Σ-Δ ADCs are on-chip. The > + AD7771 provides an ultralow input current to allow direct sensor > + connection. Each input channel has a programmable gain stage > + allowing gains of 1, 2, 4, and 8 to map lower amplitude sensor > + outputs into the full-scale ADC input range, maximizing the > + dynamic range of the signal chain. > + > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7770.pdf > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7771.pdf > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad7779.pdf > + > +$ref: /schemas/spi/spi-peripheral-props.yaml# > + > +properties: > + compatible: > + enum: > + - adi,ad7770 > + - adi,ad7771 > + - adi,ad7779 No power in general? All power supplies should be listed and if they need to have voltage on them for the device to function they should be in the required list. I left feedback on this and alert gpios in review of v4. Please address that as well for v6. > + > + reg: > + maxItems: 1 > + > + '#address-cells': > + const: 1 > + > + '#size-cells': > + const: 0 > + > + clocks: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + description: > + Interrupt line for DRDY signal which indicates the end of conversion > + independently of the interface selected to read back the Σ-∆ conversion. > + > + start-gpios: > + description: > + Pin that controls start synchronization pulse. > + maxItems: 1 > + > + reset-gpios: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + - clocks > + - interrupts > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + spi { > + #address-cells = <1>; > + #size-cells = <0>; > + > + adc@0 { > + compatible = "adi,ad7779"; > + reg = <0>; > + vref-supply = <&vref>; > + start-gpios = <&gpio0 87 GPIO_ACTIVE_LOW>; > + reset-gpios = <&gpio0 93 GPIO_ACTIVE_LOW>; > + clocks = <&adc_clk>; > + }; > + }; > +... ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 2/3] Documentation: ABI: added filter mode doc in sysfs-bus-iio 2024-09-12 12:15 [PATCH v5 0/3] add support for ad777x family Ramona Alexandra Nechita 2024-09-12 12:15 ` [PATCH v5 1/3] dt-bindings: iio: adc: add a7779 doc Ramona Alexandra Nechita @ 2024-09-12 12:15 ` Ramona Alexandra Nechita 2024-09-12 14:47 ` Andy Shevchenko 2024-09-13 4:16 ` kernel test robot 2024-09-12 12:15 ` [PATCH v5 3/3] drivers: iio: adc: add support for ad777x family Ramona Alexandra Nechita 2 siblings, 2 replies; 24+ messages in thread From: Ramona Alexandra Nechita @ 2024-09-12 12:15 UTC (permalink / raw) To: Jonathan Cameron, Lars-Peter Clausen, Cosmin Tanislav, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Nuno Sa, Andy Shevchenko, David Lechner, Marcelo Schmitt, Olivier Moysan, Dumitru Ceclan, Matteo Martelli, João Paulo Gonçalves, Alisa-Dariana Roman, Mike Looijmans, Ramona Nechita, linux-iio, linux-kernel, devicetree The filter mode / filter type property is used for ad4130 and ad7779 drivers, therefore the ABI doc file for ad4130 was removed, merging both of them in the sysfs-bus-iio. Signed-off-by: Ramona Alexandra Nechita <ramona.nechita@analog.com> --- Documentation/ABI/testing/sysfs-bus-iio | 22 +++++++++ .../ABI/testing/sysfs-bus-iio-adc-ad4130 | 46 ------------------- 2 files changed, 22 insertions(+), 46 deletions(-) delete mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad4130 diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio index 345d58535dc9..aac41e69aa43 100644 --- a/Documentation/ABI/testing/sysfs-bus-iio +++ b/Documentation/ABI/testing/sysfs-bus-iio @@ -2265,6 +2265,28 @@ Description: An example format is 16-bytes, 2-digits-per-byte, HEX-string representing the sensor unique ID number. +What: /sys/bus/iio/devices/iio:deviceX/filter_type_available +What: /sys/bus/iio/devices/iio:deviceX/in_voltage-voltage_filter_mode_available +KernelVersion: 6.1 +Contact: linux-iio@vger.kernel.org +Description: + Reading returns a list with the possible filter modes. Options + for the attribute: + * "sinc3" - The digital sinc3 filter. Moderate 1st conversion time. + Good noise performance. + * "sinc4" - Sinc 4. Excellent noise performance. Long + 1st conversion time. + * "sinc5" - The digital sinc5 filter. Excellent noise performance + * "sinc4+sinc1" - Sinc4 + averaging by 8. Low 1st conversion + time. + * "sinc3+rej60" - Sinc3 + 60Hz rejection. + * "sinc3+sinc1" - Sinc3 + averaging by 8. Low 1st conversion + time. + * "sinc3+pf1" - Sinc3 + device specific Post Filter 1. + * "sinc3+pf2" - Sinc3 + device specific Post Filter 2. + * "sinc3+pf3" - Sinc3 + device specific Post Filter 3. + * "sinc3+pf4" - Sinc3 + device specific Post Filter 4. + What: /sys/.../events/in_proximity_thresh_either_runningperiod KernelVersion: 6.6 Contact: linux-iio@vger.kernel.org diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad4130 b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad4130 deleted file mode 100644 index f24ed6687e90..000000000000 --- a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad4130 +++ /dev/null @@ -1,46 +0,0 @@ -What: /sys/bus/iio/devices/iio:deviceX/in_voltage-voltage_filter_mode_available -KernelVersion: 6.2 -Contact: linux-iio@vger.kernel.org -Description: - Reading returns a list with the possible filter modes. - - * "sinc4" - Sinc 4. Excellent noise performance. Long - 1st conversion time. No natural 50/60Hz rejection. - - * "sinc4+sinc1" - Sinc4 + averaging by 8. Low 1st conversion - time. - - * "sinc3" - Sinc3. Moderate 1st conversion time. - Good noise performance. - - * "sinc3+rej60" - Sinc3 + 60Hz rejection. At a sampling - frequency of 50Hz, achieves simultaneous 50Hz and 60Hz - rejection. - - * "sinc3+sinc1" - Sinc3 + averaging by 8. Low 1st conversion - time. Best used with a sampling frequency of at least - 216.19Hz. - - * "sinc3+pf1" - Sinc3 + Post Filter 1. 53dB rejection @ - 50Hz, 58dB rejection @ 60Hz. - - * "sinc3+pf2" - Sinc3 + Post Filter 2. 70dB rejection @ - 50Hz, 70dB rejection @ 60Hz. - - * "sinc3+pf3" - Sinc3 + Post Filter 3. 99dB rejection @ - 50Hz, 103dB rejection @ 60Hz. - - * "sinc3+pf4" - Sinc3 + Post Filter 4. 103dB rejection @ - 50Hz, 109dB rejection @ 60Hz. - -What: /sys/bus/iio/devices/iio:deviceX/in_voltageY-voltageZ_filter_mode -KernelVersion: 6.2 -Contact: linux-iio@vger.kernel.org -Description: - Set the filter mode of the differential channel. When the filter - mode changes, the in_voltageY-voltageZ_sampling_frequency and - in_voltageY-voltageZ_sampling_frequency_available attributes - might also change to accommodate the new filter mode. - If the current sampling frequency is out of range for the new - filter mode, the sampling frequency will be changed to the - closest valid one. -- 2.43.0 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/3] Documentation: ABI: added filter mode doc in sysfs-bus-iio 2024-09-12 12:15 ` [PATCH v5 2/3] Documentation: ABI: added filter mode doc in sysfs-bus-iio Ramona Alexandra Nechita @ 2024-09-12 14:47 ` Andy Shevchenko 2024-09-13 14:06 ` Nechita, Ramona 2024-09-13 4:16 ` kernel test robot 1 sibling, 1 reply; 24+ messages in thread From: Andy Shevchenko @ 2024-09-12 14:47 UTC (permalink / raw) To: Ramona Alexandra Nechita Cc: Jonathan Cameron, Lars-Peter Clausen, Cosmin Tanislav, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Nuno Sa, Andy Shevchenko, David Lechner, Marcelo Schmitt, Olivier Moysan, Dumitru Ceclan, Matteo Martelli, João Paulo Gonçalves, Alisa-Dariana Roman, Mike Looijmans, linux-iio, linux-kernel, devicetree On Thu, Sep 12, 2024 at 3:17 PM Ramona Alexandra Nechita <ramona.nechita@analog.com> wrote: > > The filter mode / filter type property is used for ad4130 > and ad7779 drivers, therefore the ABI doc file for ad4130 > was removed, merging both of them in the sysfs-bus-iio. ... > +What: /sys/bus/iio/devices/iio:deviceX/filter_type_available > +What: /sys/bus/iio/devices/iio:deviceX/in_voltage-voltage_filter_mode_available > +KernelVersion: 6.1 I believe I have already commented on this. The commit message keeps silent about version changes. Why? > +Contact: linux-iio@vger.kernel.org > +Description: > + Reading returns a list with the possible filter modes. Options > + for the attribute: > + * "sinc3" - The digital sinc3 filter. Moderate 1st conversion time. > + Good noise performance. > + * "sinc4" - Sinc 4. Excellent noise performance. Long > + 1st conversion time. > + * "sinc5" - The digital sinc5 filter. Excellent noise performance > + * "sinc4+sinc1" - Sinc4 + averaging by 8. Low 1st conversion > + time. > + * "sinc3+rej60" - Sinc3 + 60Hz rejection. > + * "sinc3+sinc1" - Sinc3 + averaging by 8. Low 1st conversion > + time. > + * "sinc3+pf1" - Sinc3 + device specific Post Filter 1. > + * "sinc3+pf2" - Sinc3 + device specific Post Filter 2. > + * "sinc3+pf3" - Sinc3 + device specific Post Filter 3. > + * "sinc3+pf4" - Sinc3 + device specific Post Filter 4. Also, the original file was more verbose for the complex cases, like "sinc3+pfX", why has this been changed? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH v5 2/3] Documentation: ABI: added filter mode doc in sysfs-bus-iio 2024-09-12 14:47 ` Andy Shevchenko @ 2024-09-13 14:06 ` Nechita, Ramona 2024-09-13 17:36 ` Andy Shevchenko 0 siblings, 1 reply; 24+ messages in thread From: Nechita, Ramona @ 2024-09-13 14:06 UTC (permalink / raw) To: Andy Shevchenko Cc: Jonathan Cameron, Lars-Peter Clausen, Tanislav, Cosmin, Hennerich, Michael, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Sa, Nuno, Andy Shevchenko, David Lechner, Schmitt, Marcelo, Olivier Moysan, Dumitru Ceclan, Matteo Martelli, João Paulo Gonçalves, Alisa-Dariana Roman, Mike Looijmans, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org >> >> The filter mode / filter type property is used for ad4130 and ad7779 >> drivers, therefore the ABI doc file for ad4130 was removed, merging >> both of them in the sysfs-bus-iio. > >... > >> +What: /sys/bus/iio/devices/iio:deviceX/filter_type_available >> +What: /sys/bus/iio/devices/iio:deviceX/in_voltage-voltage_filter_mode_available >> +KernelVersion: 6.1 > >I believe I have already commented on this. The commit message keeps silent about version changes. Why? I mentioned it in the cover-letter, since the attributes of two devices were merged, and one of them was available in 6.1 ad the other in 6.2, it felt appropriate to leave it as 6.1. I was wondering if this is ok or if it should be kept as 6.2. Should this be mentioned in the commit message as well? > >> +Contact: linux-iio@vger.kernel.org >> +Description: >> + Reading returns a list with the possible filter modes. Options >> + for the attribute: >> + * "sinc3" - The digital sinc3 filter. Moderate 1st conversion time. >> + Good noise performance. >> + * "sinc4" - Sinc 4. Excellent noise performance. Long >> + 1st conversion time. >> + * "sinc5" - The digital sinc5 filter. Excellent noise performance >> + * "sinc4+sinc1" - Sinc4 + averaging by 8. Low 1st conversion >> + time. >> + * "sinc3+rej60" - Sinc3 + 60Hz rejection. >> + * "sinc3+sinc1" - Sinc3 + averaging by 8. Low 1st conversion >> + time. >> + * "sinc3+pf1" - Sinc3 + device specific Post Filter 1. >> + * "sinc3+pf2" - Sinc3 + device specific Post Filter 2. >> + * "sinc3+pf3" - Sinc3 + device specific Post Filter 3. >> + * "sinc3+pf4" - Sinc3 + device specific Post Filter 4. > >Also, the original file was more verbose for the complex cases, like "sinc3+pfX", why has this been changed? Since this is a more generic file I was advised to leave out specific details, should I include them just as they were in the original file? -- Best Regards, Ramona ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/3] Documentation: ABI: added filter mode doc in sysfs-bus-iio 2024-09-13 14:06 ` Nechita, Ramona @ 2024-09-13 17:36 ` Andy Shevchenko 2024-09-14 16:42 ` Jonathan Cameron 0 siblings, 1 reply; 24+ messages in thread From: Andy Shevchenko @ 2024-09-13 17:36 UTC (permalink / raw) To: Nechita, Ramona Cc: Jonathan Cameron, Lars-Peter Clausen, Tanislav, Cosmin, Hennerich, Michael, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Sa, Nuno, David Lechner, Schmitt, Marcelo, Olivier Moysan, Dumitru Ceclan, Matteo Martelli, João Paulo Gonçalves, Alisa-Dariana Roman, Mike Looijmans, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org On Fri, Sep 13, 2024 at 02:06:48PM +0000, Nechita, Ramona wrote: > >> > >> The filter mode / filter type property is used for ad4130 and ad7779 > >> drivers, therefore the ABI doc file for ad4130 was removed, merging > >> both of them in the sysfs-bus-iio. ... > >> +What: /sys/bus/iio/devices/iio:deviceX/filter_type_available > >> +What: /sys/bus/iio/devices/iio:deviceX/in_voltage-voltage_filter_mode_available > >> +KernelVersion: 6.1 > > > >I believe I have already commented on this. The commit message keeps silent about version changes. Why? > > I mentioned it in the cover-letter, since the attributes of two devices were > merged, and one of them was available in 6.1 ad the other in 6.2, it felt > appropriate to leave it as 6.1. > I was wondering if this is ok or if it should be kept as 6.2. Should this be > mentioned in the commit message as well? Please, mention in the commit message. > >> +Contact: linux-iio@vger.kernel.org > >> +Description: > >> + Reading returns a list with the possible filter modes. Options > >> + for the attribute: > >> + * "sinc3" - The digital sinc3 filter. Moderate 1st conversion time. > >> + Good noise performance. > >> + * "sinc4" - Sinc 4. Excellent noise performance. Long > >> + 1st conversion time. > >> + * "sinc5" - The digital sinc5 filter. Excellent noise performance > >> + * "sinc4+sinc1" - Sinc4 + averaging by 8. Low 1st conversion > >> + time. > >> + * "sinc3+rej60" - Sinc3 + 60Hz rejection. > >> + * "sinc3+sinc1" - Sinc3 + averaging by 8. Low 1st conversion > >> + time. > >> + * "sinc3+pf1" - Sinc3 + device specific Post Filter 1. > >> + * "sinc3+pf2" - Sinc3 + device specific Post Filter 2. > >> + * "sinc3+pf3" - Sinc3 + device specific Post Filter 3. > >> + * "sinc3+pf4" - Sinc3 + device specific Post Filter 4. > > > >Also, the original file was more verbose for the complex cases, like > >"sinc3+pfX", why has this been changed? > > Since this is a more generic file I was advised to leave out specific > details, should I include them just as they were in the original file? I would leave the examples for the mentioned chip in the parentheses. But it's up to Jonathan, I have no such device anyway, so personally I'm not affected :-) -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/3] Documentation: ABI: added filter mode doc in sysfs-bus-iio 2024-09-13 17:36 ` Andy Shevchenko @ 2024-09-14 16:42 ` Jonathan Cameron 0 siblings, 0 replies; 24+ messages in thread From: Jonathan Cameron @ 2024-09-14 16:42 UTC (permalink / raw) To: Andy Shevchenko Cc: Nechita, Ramona, Lars-Peter Clausen, Tanislav, Cosmin, Hennerich, Michael, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Sa, Nuno, David Lechner, Schmitt, Marcelo, Olivier Moysan, Dumitru Ceclan, Matteo Martelli, João Paulo Gonçalves, Alisa-Dariana Roman, Mike Looijmans, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org On Fri, 13 Sep 2024 20:36:32 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Sep 13, 2024 at 02:06:48PM +0000, Nechita, Ramona wrote: > > >> > > >> The filter mode / filter type property is used for ad4130 and ad7779 > > >> drivers, therefore the ABI doc file for ad4130 was removed, merging > > >> both of them in the sysfs-bus-iio. > > ... > > > >> +What: /sys/bus/iio/devices/iio:deviceX/filter_type_available > > >> +What: /sys/bus/iio/devices/iio:deviceX/in_voltage-voltage_filter_mode_available > > >> +KernelVersion: 6.1 > > > > > >I believe I have already commented on this. The commit message keeps silent about version changes. Why? > > > > I mentioned it in the cover-letter, since the attributes of two devices were > > merged, and one of them was available in 6.1 ad the other in 6.2, it felt > > appropriate to leave it as 6.1. > > I was wondering if this is ok or if it should be kept as 6.2. Should this be > > mentioned in the commit message as well? > > Please, mention in the commit message. > > > >> +Contact: linux-iio@vger.kernel.org > > >> +Description: > > >> + Reading returns a list with the possible filter modes. Options > > >> + for the attribute: > > >> + * "sinc3" - The digital sinc3 filter. Moderate 1st conversion time. > > >> + Good noise performance. > > >> + * "sinc4" - Sinc 4. Excellent noise performance. Long > > >> + 1st conversion time. > > >> + * "sinc5" - The digital sinc5 filter. Excellent noise performance > > >> + * "sinc4+sinc1" - Sinc4 + averaging by 8. Low 1st conversion > > >> + time. > > >> + * "sinc3+rej60" - Sinc3 + 60Hz rejection. > > >> + * "sinc3+sinc1" - Sinc3 + averaging by 8. Low 1st conversion > > >> + time. > > >> + * "sinc3+pf1" - Sinc3 + device specific Post Filter 1. > > >> + * "sinc3+pf2" - Sinc3 + device specific Post Filter 2. > > >> + * "sinc3+pf3" - Sinc3 + device specific Post Filter 3. > > >> + * "sinc3+pf4" - Sinc3 + device specific Post Filter 4. > > > > > >Also, the original file was more verbose for the complex cases, like > > >"sinc3+pfX", why has this been changed? > > > > Since this is a more generic file I was advised to leave out specific > > details, should I include them just as they were in the original file? > > I would leave the examples for the mentioned chip in the parentheses. But it's > up to Jonathan, I have no such device anyway, so personally I'm not affected > :-) It gets tricky as we gain more and more devices. Nice to have that data somewhere, but maybe a device specific document is more appropriate than keeping it in here? (i.e. not in the ABI docs, but in general IIO per device documentation similar to the docs added recently for various other ADI parts). Jonathan > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/3] Documentation: ABI: added filter mode doc in sysfs-bus-iio 2024-09-12 12:15 ` [PATCH v5 2/3] Documentation: ABI: added filter mode doc in sysfs-bus-iio Ramona Alexandra Nechita 2024-09-12 14:47 ` Andy Shevchenko @ 2024-09-13 4:16 ` kernel test robot 2024-09-14 16:43 ` Jonathan Cameron 1 sibling, 1 reply; 24+ messages in thread From: kernel test robot @ 2024-09-13 4:16 UTC (permalink / raw) To: Ramona Alexandra Nechita, Jonathan Cameron, Lars-Peter Clausen, Cosmin Tanislav, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Nuno Sa, Andy Shevchenko, David Lechner, Marcelo Schmitt, Olivier Moysan, Dumitru Ceclan, Matteo Martelli, João Paulo Gonçalves, Alisa-Dariana Roman, Mike Looijmans, linux-iio, linux-kernel, devicetree Cc: oe-kbuild-all Hi Ramona, kernel test robot noticed the following build warnings: [auto build test WARNING on jic23-iio/togreg] [also build test WARNING on robh/for-next linus/master v6.11-rc7 next-20240912] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Ramona-Alexandra-Nechita/dt-bindings-iio-adc-add-a7779-doc/20240912-201936 base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg patch link: https://lore.kernel.org/r/20240912121609.13438-3-ramona.nechita%40analog.com patch subject: [PATCH v5 2/3] Documentation: ABI: added filter mode doc in sysfs-bus-iio reproduce: (https://download.01.org/0day-ci/archive/20240913/202409131243.olYA3Qdt-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202409131243.olYA3Qdt-lkp@intel.com/ All warnings (new ones prefixed by >>): Warning: Documentation/devicetree/bindings/power/wakeup-source.txt references a file that doesn't exist: Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt Warning: Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml references a file that doesn't exist: Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml Warning: Documentation/hwmon/g762.rst references a file that doesn't exist: Documentation/devicetree/bindings/hwmon/g762.txt >> Warning: MAINTAINERS references a file that doesn't exist: Documentation/ABI/testing/sysfs-bus-iio-adc-ad4130 Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/reserved-memory/qcom Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/display/exynos/ Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/nvmem/u-boot,env.yaml Using alabaster theme -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 2/3] Documentation: ABI: added filter mode doc in sysfs-bus-iio 2024-09-13 4:16 ` kernel test robot @ 2024-09-14 16:43 ` Jonathan Cameron 0 siblings, 0 replies; 24+ messages in thread From: Jonathan Cameron @ 2024-09-14 16:43 UTC (permalink / raw) To: kernel test robot Cc: Ramona Alexandra Nechita, Lars-Peter Clausen, Cosmin Tanislav, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Nuno Sa, Andy Shevchenko, David Lechner, Marcelo Schmitt, Olivier Moysan, Dumitru Ceclan, Matteo Martelli, João Paulo Gonçalves, Alisa-Dariana Roman, Mike Looijmans, linux-iio, linux-kernel, devicetree, oe-kbuild-all On Fri, 13 Sep 2024 12:16:34 +0800 kernel test robot <lkp@intel.com> wrote: > Hi Ramona, > > kernel test robot noticed the following build warnings: > > [auto build test WARNING on jic23-iio/togreg] > [also build test WARNING on robh/for-next linus/master v6.11-rc7 next-20240912] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: https://github.com/intel-lab-lkp/linux/commits/Ramona-Alexandra-Nechita/dt-bindings-iio-adc-add-a7779-doc/20240912-201936 > base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg > patch link: https://lore.kernel.org/r/20240912121609.13438-3-ramona.nechita%40analog.com > patch subject: [PATCH v5 2/3] Documentation: ABI: added filter mode doc in sysfs-bus-iio > reproduce: (https://download.01.org/0day-ci/archive/20240913/202409131243.olYA3Qdt-lkp@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: https://lore.kernel.org/oe-kbuild-all/202409131243.olYA3Qdt-lkp@intel.com/ > > All warnings (new ones prefixed by >>): > > Warning: Documentation/devicetree/bindings/power/wakeup-source.txt references a file that doesn't exist: Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt > Warning: Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml references a file that doesn't exist: Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml > Warning: Documentation/hwmon/g762.rst references a file that doesn't exist: Documentation/devicetree/bindings/hwmon/g762.txt > >> Warning: MAINTAINERS references a file that doesn't exist: Documentation/ABI/testing/sysfs-bus-iio-adc-ad4130 Ah. Ramona, make sure to delete this reference as well in this patch. Thanks, Jonathan > Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/reserved-memory/qcom > Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/display/exynos/ > Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/misc/fsl,qoriq-mc.txt > Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/nvmem/u-boot,env.yaml > Using alabaster theme > ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 3/3] drivers: iio: adc: add support for ad777x family 2024-09-12 12:15 [PATCH v5 0/3] add support for ad777x family Ramona Alexandra Nechita 2024-09-12 12:15 ` [PATCH v5 1/3] dt-bindings: iio: adc: add a7779 doc Ramona Alexandra Nechita 2024-09-12 12:15 ` [PATCH v5 2/3] Documentation: ABI: added filter mode doc in sysfs-bus-iio Ramona Alexandra Nechita @ 2024-09-12 12:15 ` Ramona Alexandra Nechita 2024-09-12 15:15 ` Andy Shevchenko ` (2 more replies) 2 siblings, 3 replies; 24+ messages in thread From: Ramona Alexandra Nechita @ 2024-09-12 12:15 UTC (permalink / raw) To: Jonathan Cameron, Lars-Peter Clausen, Cosmin Tanislav, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Nuno Sa, Andy Shevchenko, David Lechner, Marcelo Schmitt, Olivier Moysan, Dumitru Ceclan, Matteo Martelli, AngeloGioacchino Del Regno, Alisa-Dariana Roman, Ivan Mikhaylov, Mike Looijmans, Ramona Nechita, linux-iio, linux-kernel, devicetree Add support for AD7770, AD7771, AD7779 ADCs. The device is capable of sending out data both on DOUT lines interface,as on the SDO line. The driver currently implements only the SDO data streaming mode. SPI communication is used alternatively for accessing registers and streaming data. Register access are protected by crc8. Signed-off-by: Ramona Alexandra Nechita <ramona.nechita@analog.com> --- drivers/iio/adc/Kconfig | 11 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/ad7779.c | 917 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 929 insertions(+) create mode 100644 drivers/iio/adc/ad7779.c diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index 88e8ce2e78b3..fe6d56225fa1 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -267,6 +267,17 @@ config AD7768_1 To compile this driver as a module, choose M here: the module will be called ad7768-1. +config AD7779 + tristate "Analog Devices AD7779 ADC driver" + depends on SPI + select IIO_BUFFER + help + Say yes here to build support for Analog Devices AD777X family + (AD7770, AD7771, AD7779) analog to digital converter (ADC). + + To compile this driver as a module, choose M here: the module will be + called ad7779. + config AD7780 tristate "Analog Devices AD7780 and similar ADCs driver" depends on SPI diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index 8b80664c6d6b..3d28ea490e01 100644 --- a/drivers/iio/adc/Makefile +++ b/drivers/iio/adc/Makefile @@ -27,6 +27,7 @@ obj-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o obj-$(CONFIG_AD7606) += ad7606.o obj-$(CONFIG_AD7766) += ad7766.o obj-$(CONFIG_AD7768_1) += ad7768-1.o +obj-$(CONFIG_AD7779) += ad7779.o obj-$(CONFIG_AD7780) += ad7780.o obj-$(CONFIG_AD7791) += ad7791.o obj-$(CONFIG_AD7793) += ad7793.o diff --git a/drivers/iio/adc/ad7779.c b/drivers/iio/adc/ad7779.c new file mode 100644 index 000000000000..05ae72257c9e --- /dev/null +++ b/drivers/iio/adc/ad7779.c @@ -0,0 +1,917 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * AD7770, AD7771, AD7779 ADC + * + * Copyright 2023-2024 Analog Devices Inc. + */ + +#include <linux/bitfield.h> +#include <linux/bitmap.h> +#include <linux/clk.h> +#include <linux/crc8.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/gpio/consumer.h> +#include <linux/interrupt.h> +#include <linux/irq.h> +#include <linux/module.h> +#include <linux/mod_devicetable.h> +#include <linux/regulator/consumer.h> +#include <linux/spi/spi.h> +#include <linux/string.h> +#include <linux/units.h> + +#include <asm/unaligned.h> + +#include <linux/iio/iio.h> +#include <linux/iio/buffer.h> +#include <linux/iio/sysfs.h> +#include <linux/iio/trigger.h> +#include <linux/iio/triggered_buffer.h> +#include <linux/iio/trigger_consumer.h> + +#define AD7779_SPI_READ_CMD BIT(7) + +#define AD7779_DISABLE_SD BIT(7) + +#define AD7779_REG_CH_DISABLE 0x08 +#define AD7779_REG_CH_SYNC_OFFSET(ch) (0x09 + (ch)) +#define AD7779_REG_CH_CONFIG(ch) (0x00 + (ch)) +#define AD7779_REG_GENERAL_USER_CONFIG_1 0x11 +#define AD7779_REG_GENERAL_USER_CONFIG_2 0x12 +#define AD7779_REG_GENERAL_USER_CONFIG_3 0x13 +#define AD7779_REG_DOUT_FORMAT 0x14 +#define AD7779_REG_ADC_MUX_CONFIG 0x15 +#define AD7779_REG_GPIO_CONFIG 0x17 +#define AD7779_REG_BUFFER_CONFIG_1 0x19 +#define AD7779_REG_GLOBAL_MUX_CONFIG 0x16 +#define AD7779_REG_BUFFER_CONFIG_2 0x1A +#define AD7779_REG_GPIO_DATA 0x18 +#define AD7779_REG_CH_OFFSET_UPPER_BYTE(ch) (0x1C + (ch) * 6) +#define AD7779_REG_CH_OFFSET_LOWER_BYTE(ch) (0x1E + (ch) * 6) +#define AD7779_REG_CH_GAIN_UPPER_BYTE(ch) (0x1F + (ch) * 6) +#define AD7779_REG_CH_OFFSET_MID_BYTE(ch) (0x1D + (ch) * 6) +#define AD7779_REG_CH_GAIN_MID_BYTE(ch) (0x20 + (ch) * 6) +#define AD7779_REG_CH_ERR_REG(ch) (0x4C + (ch)) +#define AD7779_REG_CH0_1_SAT_ERR 0x54 +#define AD7779_REG_CH_GAIN_LOWER_BYTE(ch) (0x21 + (ch) * 6) +#define AD7779_REG_CH2_3_SAT_ERR 0x55 +#define AD7779_REG_CH4_5_SAT_ERR 0x56 +#define AD7779_REG_CH6_7_SAT_ERR 0x57 +#define AD7779_REG_CHX_ERR_REG_EN 0x58 +#define AD7779_REG_GEN_ERR_REG_1 0x59 +#define AD7779_REG_GEN_ERR_REG_1_EN 0x5A +#define AD7779_REG_GEN_ERR_REG_2 0x5B +#define AD7779_REG_GEN_ERR_REG_2_EN 0x5C +#define AD7779_REG_STATUS_REG_1 0x5D +#define AD7779_REG_STATUS_REG_2 0x5E +#define AD7779_REG_STATUS_REG_3 0x5F +#define AD7779_REG_SRC_N_MSB 0x60 +#define AD7779_REG_SRC_N_LSB 0x61 +#define AD7779_REG_SRC_IF_MSB 0x62 +#define AD7779_REG_SRC_IF_LSB 0x63 +#define AD7779_REG_SRC_UPDATE 0x64 + +#define AD7779_FILTER_MSK BIT(6) +#define AD7779_MOD_POWERMODE_MSK BIT(6) +#define AD7779_MOD_PDB_REFOUT_MSK BIT(4) +#define AD7779_MOD_SPI_EN_MSK BIT(4) +#define AD7779_USRMOD_INIT_MSK GENMASK(6,4) + +/* AD7779_REG_DOUT_FORMAT */ +#define AD7779_DOUT_FORMAT_MSK GENMASK(7, 6) +#define AD7779_DOUT_HEADER_FORMAT BIT(5) +#define AD7779_DCLK_CLK_DIV_MSK GENMASK(3, 1) + +#define AD7779_REFMUX_CTRL_MSK GENMASK(7, 6) +#define AD7779_SPI_CRC_EN_MSK BIT(0) + +#define AD7779_MAXCLK_LOWPOWER 4096000 +#define AD7779_NUM_CHANNELS 8 +#define AD7779_RESET_BUF_SIZE 8 +#define AD7779_CHAN_DATA_SIZE 4 + +#define AD7779_LOWPOWER_DIV 512 +#define AD7779_HIGHPOWER_DIV 2048 + +#define AD7779_SINC3_MAXFREQ (16 * HZ_PER_KHZ) +#define AD7779_SINC5_MAXFREQ (128 * HZ_PER_KHZ) + +#define AD7779_DEFAULT_SAMPLING_FREQ (8 * HZ_PER_KHZ) +#define AD7779_DEFAULT_SAMPLING_2LINE (4 * HZ_PER_KHZ) +#define AD7779_DEFAULT_SAMPLING_1LINE (2 * HZ_PER_KHZ) + +#define AD7779_SPIMODE_MAX_SAMP_FREQ (16 * HZ_PER_KHZ) + +#define GAIN_REL 0x555555 +#define AD7779_FREQ_MSB_MSK GENMASK(15, 8) +#define AD7779_FREQ_LSB_MSK GENMASK(7, 0) +#define AD7779_UPPER GENMASK(23, 16) +#define AD7779_MID GENMASK(15, 8) +#define AD7779_LOWER GENMASK(7, 0) + +#define AD7779_REG_MSK GENMASK(6, 0) + +#define AD7779_CRC8_POLY 0x07 +DECLARE_CRC8_TABLE(ad7779_crc8_table); + +enum ad7779_filter { + AD7779_SINC3, + AD7779_SINC5, +}; + +enum ad7779_variant { + ad7770, + ad7771, + ad7779, +}; + +enum ad7779_power_mode { + AD7779_LOW_POWER, + AD7779_HIGH_POWER, +}; + +struct ad7779_chip_info { + const char *name; + struct iio_chan_spec const *channels; +}; + +struct ad7779_state { + struct spi_device *spi; + const struct ad7779_chip_info *chip_info; + struct clk *mclk; + struct iio_trigger *trig; + struct completion completion; + unsigned int sampling_freq; + enum ad7779_filter filter_enabled; + /* + * DMA (thus cache coherency maintenance) requires the + * transfer buffers to live in their own cache lines. + */ + u8 reg_rx_buf[3] __aligned(IIO_DMA_MINALIGN); + u8 reg_tx_buf[3]; + u32 spidata_rx[8]; + u32 spidata_tx[8]; + u8 reset_buf[8]; +}; + +static const char * const ad7779_filter_type[] = { + [AD7779_SINC3] = "sinc3", + [AD7779_SINC5] = "sinc5", +}; + +static int ad7779_spi_read(struct ad7779_state *st, u8 reg, u8 *rbuf) +{ + int ret; + int length = 3; + u8 crc_buf[2]; + u8 exp_crc = 0; + struct spi_transfer reg_read_tr[] = { + { + .tx_buf = st->reg_tx_buf, + .rx_buf = st->reg_rx_buf, + }, + }; + + if (reg == AD7779_REG_GEN_ERR_REG_1_EN) + length = 2; + reg_read_tr[0].len = length; + + st->reg_tx_buf[0] = AD7779_SPI_READ_CMD | FIELD_GET(AD7779_REG_MSK, reg); + st->reg_tx_buf[1] = 0; + st->reg_tx_buf[2] = crc8(ad7779_crc8_table, st->reg_tx_buf, 2, 0); + + ret = spi_sync_transfer(st->spi, reg_read_tr, ARRAY_SIZE(reg_read_tr)); + if (ret) + return ret; + + crc_buf[0] = AD7779_SPI_READ_CMD | FIELD_GET(AD7779_REG_MSK, reg); + crc_buf[1] = st->reg_rx_buf[1]; + exp_crc = crc8(ad7779_crc8_table, crc_buf, 2, 0); + if (reg != AD7779_REG_GEN_ERR_REG_1_EN && exp_crc != st->reg_rx_buf[2]) { + dev_err(&st->spi->dev, "Bad CRC %x, expected %x", + st->reg_rx_buf[2], exp_crc); + return -EINVAL; + } + *rbuf = st->reg_rx_buf[1]; + + return 0; +} + +static int ad7779_spi_write(struct ad7779_state *st, u8 reg, u8 val) +{ + int length = 3; + + if (reg == AD7779_REG_GEN_ERR_REG_1_EN) + length = 2; + + st->reg_tx_buf[0] = FIELD_GET(AD7779_REG_MSK, reg); + st->reg_tx_buf[1] = val; + st->reg_tx_buf[2] = crc8(ad7779_crc8_table, st->reg_tx_buf, 2, 0); + + return spi_write(st->spi, st->reg_tx_buf, length); +} + +static int ad7779_spi_write_mask(struct ad7779_state *st, u8 reg, u8 mask, + u8 val) +{ + int ret; + u8 regval, data; + + ret = ad7779_spi_read(st, reg, &data); + if (ret) + return ret; + + regval = data; + regval &= ~mask; + regval |= val; + + if (regval == data) + return 0; + + return ad7779_spi_write(st, reg, regval); +} + +static int ad7779_reg_access(struct iio_dev *indio_dev, + unsigned int reg, + unsigned int writeval, + unsigned int *readval) +{ + struct ad7779_state *st = iio_priv(indio_dev); + + if (readval) + return ad7779_spi_read(st, reg, (u8 *)readval); + + return ad7779_spi_write(st, reg, writeval); +} + +static int ad7779_set_sampling_frequency(struct ad7779_state *st, + unsigned int sampling_freq) +{ + int ret; + unsigned int dec; + unsigned int div; + unsigned int decimal; + int temp; + unsigned int kfreq; + + if (st->filter_enabled == AD7779_SINC3 && + sampling_freq > AD7779_SINC3_MAXFREQ) + return -EINVAL; + + if (st->filter_enabled == AD7779_SINC5 && + sampling_freq > AD7779_SINC5_MAXFREQ) + return -EINVAL; + + if (sampling_freq > AD7779_SPIMODE_MAX_SAMP_FREQ) + return -EINVAL; + + div = AD7779_HIGHPOWER_DIV; + + kfreq = sampling_freq / KILO; + dec = div / kfreq; + + ret = ad7779_spi_write(st, AD7779_REG_SRC_N_MSB, + FIELD_GET(AD7779_FREQ_MSB_MSK, dec)); + if (ret) + return ret; + ret = ad7779_spi_write(st, AD7779_REG_SRC_N_LSB, + FIELD_GET(AD7779_FREQ_LSB_MSK, dec)); + if (ret) + return ret; + + if (div % kfreq) { + temp = (div * KILO) / kfreq; + decimal = ((temp - dec * KILO) << 16) / KILO; + ret = ad7779_spi_write(st, AD7779_REG_SRC_N_MSB, + FIELD_GET(AD7779_FREQ_MSB_MSK, decimal)); + if (ret) + return ret; + ret = ad7779_spi_write(st, AD7779_REG_SRC_N_LSB, + FIELD_GET(AD7779_FREQ_LSB_MSK, decimal)); + if (ret) + return ret; + } else { + ret = ad7779_spi_write(st, AD7779_REG_SRC_N_MSB, + FIELD_GET(AD7779_FREQ_MSB_MSK, 0x0)); + if (ret) + return ret; + ret = ad7779_spi_write(st, AD7779_REG_SRC_N_LSB, + FIELD_GET(AD7779_FREQ_LSB_MSK, 0x0)); + if (ret) + return ret; + } + ret = ad7779_spi_write(st, AD7779_REG_SRC_UPDATE, 0x1); + if (ret) + return ret; + + /* SRC update settling time */ + fsleep(15); + ret = ad7779_spi_write(st, AD7779_REG_SRC_UPDATE, 0x0); + if (ret) + return ret; + + /* SRC update settling time */ + fsleep(15); + + st->sampling_freq = sampling_freq; + + return 0; +} + +static int ad7779_get_filter(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan) +{ + struct ad7779_state *st = iio_priv(indio_dev); + u8 temp; + int ret; + + ret = ad7779_spi_read(st, AD7779_REG_GENERAL_USER_CONFIG_2, &temp); + if (ret) + return ret; + + return FIELD_GET(AD7779_FILTER_MSK, temp); +} + +static int ad7779_set_filter(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + unsigned int mode) +{ + struct ad7779_state *st = iio_priv(indio_dev); + int ret; + + ret = ad7779_spi_write_mask(st, + AD7779_REG_GENERAL_USER_CONFIG_2, + AD7779_FILTER_MSK, + FIELD_PREP(AD7779_FILTER_MSK, mode)); + if (ret < 0) + return ret; + + ret = ad7779_set_sampling_frequency(st, st->sampling_freq); + if (ret < 0) + return ret; + + st->filter_enabled = mode; + + return 0; +} + +static int ad7779_get_calibscale(struct ad7779_state *st, int channel) +{ + int ret; + u8 calibscale[3]; + + ret = ad7779_spi_read(st, AD7779_REG_CH_GAIN_LOWER_BYTE(channel), &calibscale[0]); + if (ret) + return ret; + + ret = ad7779_spi_read(st, AD7779_REG_CH_GAIN_MID_BYTE(channel), &calibscale[1]); + if (ret) + return ret; + + ret = ad7779_spi_read(st, AD7779_REG_CH_GAIN_UPPER_BYTE(channel), &calibscale[2]); + if (ret) + return ret; + + return get_unaligned_be24(calibscale); +} + +static int ad7779_set_calibscale(struct ad7779_state *st, int channel, int val) +{ + int ret; + unsigned int gain; + unsigned long long tmp; + u8 gain_bytes[3]; + + tmp = val * 5592405LL; + gain = DIV_ROUND_CLOSEST_ULL(tmp, MEGA); + put_unaligned_be24(gain, gain_bytes); + ret = ad7779_spi_write(st, + AD7779_REG_CH_GAIN_UPPER_BYTE(channel), + gain_bytes[0]); + if (ret) + return ret; + + ret = ad7779_spi_write(st, + AD7779_REG_CH_GAIN_MID_BYTE(channel), + gain_bytes[1]); + if (ret) + return ret; + + return ad7779_spi_write(st, + AD7779_REG_CH_GAIN_LOWER_BYTE(channel), + gain_bytes[2]); +} + +static int ad7779_get_calibbias(struct ad7779_state *st, int channel) +{ + int ret; + u8 calibbias[3]; + + ret = ad7779_spi_read(st, AD7779_REG_CH_OFFSET_LOWER_BYTE(channel), + &calibbias[0]); + if (ret) + return ret; + + ret = ad7779_spi_read(st, AD7779_REG_CH_OFFSET_MID_BYTE(channel), + &calibbias[1]); + if (ret) + return ret; + + ret = ad7779_spi_read(st, + AD7779_REG_CH_OFFSET_UPPER_BYTE(channel), + &calibbias[2]); + if (ret) + return ret; + + return get_unaligned_be24(calibbias); +} + +static int ad7779_set_calibbias(struct ad7779_state *st, int channel, int val) +{ + int ret; + u8 calibbias[3]; + + put_unaligned_be24(val, calibbias); + ret = ad7779_spi_write(st, + AD7779_REG_CH_OFFSET_UPPER_BYTE(channel), + calibbias[0]); + if (ret) + return ret; + + ret = ad7779_spi_write(st, + AD7779_REG_CH_OFFSET_MID_BYTE(channel), + calibbias[1]); + if (ret) + return ret; + + return ad7779_spi_write(st, + AD7779_REG_CH_OFFSET_LOWER_BYTE(channel), + calibbias[2]); +} + +static int ad7779_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int *val, + int *val2, long mask) +{ + struct ad7779_state *st = iio_priv(indio_dev); + + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { + switch (mask) { + case IIO_CHAN_INFO_CALIBSCALE: + *val = ad7779_get_calibscale(st, chan->channel); + if (*val < 0) + return -EINVAL; + *val2 = GAIN_REL; + return IIO_VAL_FRACTIONAL; + case IIO_CHAN_INFO_CALIBBIAS: + *val = ad7779_get_calibbias(st, chan->channel); + if (*val < 0) + return -EINVAL; + return IIO_VAL_INT; + case IIO_CHAN_INFO_SAMP_FREQ: + *val = st->sampling_freq; + if (*val < 0) + return -EINVAL; + return IIO_VAL_INT; + } + return -EINVAL; + } + unreachable(); +} + +static int ad7779_write_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int val, int val2, + long mask) +{ + struct ad7779_state *st = iio_priv(indio_dev); + + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { + switch (mask) { + case IIO_CHAN_INFO_CALIBSCALE: + return ad7779_set_calibscale(st, chan->channel, val2); + case IIO_CHAN_INFO_CALIBBIAS: + return ad7779_set_calibbias(st, chan->channel, val); + case IIO_CHAN_INFO_SAMP_FREQ: + return ad7779_set_sampling_frequency(st, val); + default: + return -EINVAL; + } + } + unreachable(); +} + +static int ad7779_buffer_preenable(struct iio_dev *indio_dev) +{ + int ret; + struct ad7779_state *st = iio_priv(indio_dev); + + ret = ad7779_spi_write_mask(st, + AD7779_REG_GENERAL_USER_CONFIG_3, + AD7779_MOD_SPI_EN_MSK, + FIELD_PREP(AD7779_MOD_SPI_EN_MSK, 1)); + if (ret) + return ret; + + /* + * DRDY output cannot be disabled at device level therefore we mask + * the irq at host end. + */ + enable_irq(st->spi->irq); + + return 0; +} + +static int ad7779_buffer_postdisable(struct iio_dev *indio_dev) +{ + struct ad7779_state *st = iio_priv(indio_dev); + + disable_irq(st->spi->irq); + + return ad7779_spi_write(st, AD7779_REG_GENERAL_USER_CONFIG_3, + AD7779_DISABLE_SD); +} + +static irqreturn_t ad7779_trigger_handler(int irq, void *p) +{ + struct iio_poll_func *pf = p; + struct iio_dev *indio_dev = pf->indio_dev; + struct ad7779_state *st = iio_priv(indio_dev); + int ret; + int bit; + int k = 0; + /* + * Each channel shifts out HEADER + 24 bits of data therefore 8 * u32 + * for the data and 64 bits for the timestamp + */ + u32 tmp[10]; + + struct spi_transfer sd_readback_tr[] = { + { + .rx_buf = st->spidata_rx, + .tx_buf = st->spidata_tx, + .len = AD7779_NUM_CHANNELS * AD7779_CHAN_DATA_SIZE, + } + }; + + if (!iio_buffer_enabled(indio_dev)) + goto exit_handler; + + st->spidata_tx[0] = AD7779_SPI_READ_CMD; + ret = spi_sync_transfer(st->spi, sd_readback_tr, + ARRAY_SIZE(sd_readback_tr)); + if (ret) { + dev_err(&st->spi->dev, + "spi transfer error in irq handler"); + goto exit_handler; + } + + for_each_set_bit(bit, indio_dev->active_scan_mask, AD7779_NUM_CHANNELS - 1) + tmp[k++] = st->spidata_rx[bit]; + + iio_push_to_buffers_with_timestamp(indio_dev, &tmp[0], pf->timestamp); + +exit_handler: + iio_trigger_notify_done(indio_dev->trig); + return IRQ_HANDLED; +} + +static int ad7779_reset(struct iio_dev *indio_dev, struct gpio_desc *reset_gpio) +{ + struct ad7779_state *st = iio_priv(indio_dev); + int ret; + struct spi_transfer reg_read_tr[] = { + { + .tx_buf = st->reset_buf, + .len = 8, + }, + }; + + memset(st->reset_buf, 0xff, sizeof(st->reset_buf)); + + if (reset_gpio) { + /* Delay for reset to occur is 225 microseconds*/ + gpiod_set_value(reset_gpio, 1); + fsleep(230); + return 0; + } + + ret = spi_sync_transfer(st->spi, reg_read_tr, + ARRAY_SIZE(reg_read_tr)); + if (ret) + return ret; + + /* Delay for reset to occur is 225 microseconds*/ + fsleep(230); + + return 0; +} + +static const struct iio_info ad7779_info = { + .read_raw = ad7779_read_raw, + .write_raw = ad7779_write_raw, + .debugfs_reg_access = &ad7779_reg_access, +}; + +static const struct iio_enum ad7779_filter_enum = { + .items = ad7779_filter_type, + .num_items = ARRAY_SIZE(ad7779_filter_type), + .get = ad7779_get_filter, + .set = ad7779_set_filter, +}; + +static const struct iio_chan_spec_ext_info ad7779_ext_filter[] = { + IIO_ENUM("filter_type", IIO_SHARED_BY_ALL, &ad7779_filter_enum), + IIO_ENUM_AVAILABLE("filter_type", IIO_SHARED_BY_ALL, + &ad7779_filter_enum), + { } +}; + +#define AD777x_CHAN_S(index, _ext_info) \ + { \ + .type = IIO_VOLTAGE, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) | \ + BIT(IIO_CHAN_INFO_CALIBBIAS), \ + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \ + .address = (index), \ + .indexed = 1, \ + .channel = (index), \ + .scan_index = (index), \ + .ext_info = (_ext_info), \ + .scan_type = { \ + .sign = 's', \ + .realbits = 24, \ + .storagebits = 32, \ + .endianness = IIO_BE, \ + }, \ + } + +#define AD777x_CHAN_NO_FILTER_S(index) \ + AD777x_CHAN_S(index, NULL) + +#define AD777x_CHAN_FILTER_S(index) \ + AD777x_CHAN_S(index, ad7779_ext_filter) +static const struct iio_chan_spec ad7779_channels[] = { + AD777x_CHAN_NO_FILTER_S(0), + AD777x_CHAN_NO_FILTER_S(1), + AD777x_CHAN_NO_FILTER_S(2), + AD777x_CHAN_NO_FILTER_S(3), + AD777x_CHAN_NO_FILTER_S(4), + AD777x_CHAN_NO_FILTER_S(5), + AD777x_CHAN_NO_FILTER_S(6), + AD777x_CHAN_NO_FILTER_S(7), + IIO_CHAN_SOFT_TIMESTAMP(8), +}; + +static const struct iio_chan_spec ad7779_channels_filter[] = { + AD777x_CHAN_FILTER_S(0), + AD777x_CHAN_FILTER_S(1), + AD777x_CHAN_FILTER_S(2), + AD777x_CHAN_FILTER_S(3), + AD777x_CHAN_FILTER_S(4), + AD777x_CHAN_FILTER_S(5), + AD777x_CHAN_FILTER_S(6), + AD777x_CHAN_FILTER_S(7), + IIO_CHAN_SOFT_TIMESTAMP(8), +}; + +static const struct iio_buffer_setup_ops ad7779_buffer_setup_ops = { + .preenable = ad7779_buffer_preenable, + .postdisable = ad7779_buffer_postdisable, +}; + +static const struct iio_trigger_ops ad7779_trigger_ops = { + .validate_device = iio_trigger_validate_own_device, +}; + +static int ad7779_powerup(struct ad7779_state *st, struct gpio_desc *start_gpio) +{ + int ret; + + ret = ad7779_spi_write_mask(st, AD7779_REG_GEN_ERR_REG_1_EN, + AD7779_SPI_CRC_EN_MSK, + FIELD_PREP(AD7779_SPI_CRC_EN_MSK, 1)); + if (ret) + return ret; + + ret = ad7779_spi_write_mask(st, AD7779_REG_GENERAL_USER_CONFIG_1, + AD7779_USRMOD_INIT_MSK, + FIELD_PREP(AD7779_USRMOD_INIT_MSK, 5)); + if (ret) + return ret; + + ret = ad7779_spi_write_mask(st, AD7779_REG_DOUT_FORMAT, + AD7779_DCLK_CLK_DIV_MSK, + FIELD_PREP(AD7779_DCLK_CLK_DIV_MSK, 1)); + if (ret) + return ret; + + ret = ad7779_spi_write_mask(st, AD7779_REG_ADC_MUX_CONFIG, + AD7779_REFMUX_CTRL_MSK, + FIELD_PREP(AD7779_REFMUX_CTRL_MSK, 1)); + if (ret) + return ret; + + ret = ad7779_set_sampling_frequency(st, AD7779_DEFAULT_SAMPLING_FREQ); + if (ret) + return ret; + + gpiod_set_value(start_gpio, 0); + /* Start setup time */ + fsleep(15); + gpiod_set_value(start_gpio, 1); + fsleep(15); + gpiod_set_value(start_gpio, 0); + fsleep(15); + + return 0; +} + +static int ad7779_probe(struct spi_device *spi) +{ + struct iio_dev *indio_dev; + struct ad7779_state *st; + struct gpio_desc *reset_gpio, *start_gpio; + int ret; + + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st)); + if (!indio_dev) + return -ENOMEM; + + st = iio_priv(indio_dev); + + st->mclk = devm_clk_get_enabled(&spi->dev, "mclk"); + if (IS_ERR(st->mclk)) + return PTR_ERR(st->mclk); + + if (!spi->irq) + return dev_err_probe(&spi->dev, ret, + "DRDY irq not present\n"); + + reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW); + if (IS_ERR(reset_gpio)) + return PTR_ERR(reset_gpio); + + start_gpio = devm_gpiod_get(&spi->dev, "start", GPIOD_OUT_HIGH); + if (IS_ERR(start_gpio)) + return PTR_ERR(start_gpio); + + crc8_populate_msb(ad7779_crc8_table, AD7779_CRC8_POLY); + st->spi = spi; + + st->chip_info = spi_get_device_match_data(spi); + if (!st->chip_info) + return -ENODEV; + + ret = ad7779_reset(indio_dev, reset_gpio); + if (ret) + return ret; + + ad7779_powerup(st, start_gpio); + if (ret) + return ret; + + indio_dev->name = st->chip_info->name; + indio_dev->info = &ad7779_info; + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->channels = st->chip_info->channels; + indio_dev->num_channels = ARRAY_SIZE(ad7779_channels); + + st->trig = devm_iio_trigger_alloc(&spi->dev, "%s-dev%d", + indio_dev->name, iio_device_id(indio_dev)); + if (!st->trig) + return -ENOMEM; + + st->trig->ops = &ad7779_trigger_ops; + + iio_trigger_set_drvdata(st->trig, st); + + ret = devm_request_irq(&spi->dev, spi->irq, + iio_trigger_generic_data_rdy_poll, + IRQF_ONESHOT | IRQF_NO_AUTOEN, + indio_dev->name, st->trig); + if (ret) + return dev_err_probe(&spi->dev, ret, "request irq %d failed\n", + st->spi->irq); + + ret = devm_iio_trigger_register(&spi->dev, st->trig); + if (ret) + return ret; + + indio_dev->trig = iio_trigger_get(st->trig); + + init_completion(&st->completion); + + ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev, + &iio_pollfunc_store_time, + &ad7779_trigger_handler, + &ad7779_buffer_setup_ops); + if (ret) + return ret; + + ret = ad7779_spi_write_mask(st, AD7779_REG_DOUT_FORMAT, + AD7779_DCLK_CLK_DIV_MSK, + FIELD_PREP(AD7779_DCLK_CLK_DIV_MSK, 7)); + if (ret) + return ret; + + return devm_iio_device_register(&spi->dev, indio_dev); +} + +static int ad7779_suspend(struct device *dev) +{ + struct iio_dev *indio_dev = dev_get_drvdata(dev); + struct ad7779_state *st = iio_priv(indio_dev); + int ret; + + ret = ad7779_spi_write_mask(st, AD7779_REG_GENERAL_USER_CONFIG_1, + AD7779_MOD_POWERMODE_MSK, + FIELD_PREP(AD7779_MOD_POWERMODE_MSK, + AD7779_LOW_POWER)); + if (ret) + return ret; + + return 0; +} + +static int ad7779_resume(struct device *dev) +{ + struct iio_dev *indio_dev = dev_get_drvdata(dev); + struct ad7779_state *st = iio_priv(indio_dev); + int ret; + + ret = ad7779_spi_write_mask(st, AD7779_REG_GENERAL_USER_CONFIG_1, + AD7779_MOD_POWERMODE_MSK, + FIELD_PREP(AD7779_MOD_POWERMODE_MSK, + AD7779_HIGH_POWER)); + if (ret) + return ret; + + return 0; +} + +static DEFINE_SIMPLE_DEV_PM_OPS(ad7779_pm_ops, ad7779_suspend, ad7779_resume); + +static const struct ad7779_chip_info ad7770_chip_info = { + .name = "ad7770", + .channels = ad7779_channels, +}; + +static const struct ad7779_chip_info ad7771_chip_info = { + .name = "ad7771", + .channels = ad7779_channels_filter, +}; + +static const struct ad7779_chip_info ad7779_chip_info = { + .name = "ad7779", + .channels = ad7779_channels, +}; + +static const struct spi_device_id ad7779_id[] = { + { + .name = "ad7770", + .driver_data = (kernel_ulong_t)&ad7770_chip_info + }, + { + .name = "ad7771", + .driver_data = (kernel_ulong_t)&ad7771_chip_info + }, + { + .name = "ad7779", + .driver_data = (kernel_ulong_t)&ad7779_chip_info + }, + { } +}; +MODULE_DEVICE_TABLE(spi, ad7779_id); + +static const struct of_device_id ad7779_of_table[] = { + { + .compatible = "adi,ad7770", + .data = &ad7770_chip_info, + }, + { + .compatible = "adi,ad7771", + .data = &ad7771_chip_info, + }, + { + .compatible = "adi,ad7779", + .data = &ad7779_chip_info, + }, + { } +}; +MODULE_DEVICE_TABLE(of, ad7779_of_table); + +static struct spi_driver ad7779_driver = { + .driver = { + .name = "ad7779", + .pm = pm_sleep_ptr(&ad7779_pm_ops), + .of_match_table = ad7779_of_table, + }, + .probe = ad7779_probe, + .id_table = ad7779_id, +}; +module_spi_driver(ad7779_driver); + +MODULE_AUTHOR("Ramona Alexandra Nechita <ramona.nechita@analog.com>"); +MODULE_DESCRIPTION("Analog Devices AD7779 ADC"); +MODULE_LICENSE("GPL"); -- 2.43.0 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH v5 3/3] drivers: iio: adc: add support for ad777x family 2024-09-12 12:15 ` [PATCH v5 3/3] drivers: iio: adc: add support for ad777x family Ramona Alexandra Nechita @ 2024-09-12 15:15 ` Andy Shevchenko 2024-09-14 16:46 ` Jonathan Cameron 2024-09-14 1:44 ` kernel test robot 2024-09-14 17:06 ` Jonathan Cameron 2 siblings, 1 reply; 24+ messages in thread From: Andy Shevchenko @ 2024-09-12 15:15 UTC (permalink / raw) To: Ramona Alexandra Nechita Cc: Jonathan Cameron, Lars-Peter Clausen, Cosmin Tanislav, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Nuno Sa, Andy Shevchenko, David Lechner, Marcelo Schmitt, Olivier Moysan, Dumitru Ceclan, Matteo Martelli, AngeloGioacchino Del Regno, Alisa-Dariana Roman, Ivan Mikhaylov, Mike Looijmans, linux-iio, linux-kernel, devicetree On Thu, Sep 12, 2024 at 3:17 PM Ramona Alexandra Nechita <ramona.nechita@analog.com> wrote: > > Add support for AD7770, AD7771, AD7779 ADCs. The device is capable of "..., and AD7779..." > sending out data both on DOUT lines interface,as on the SDO line. > The driver currently implements only the SDO data streaming mode. SPI > communication is used alternatively for accessing registers and streaming > data. Register access are protected by crc8. accesses ... > +/* > + * AD7770, AD7771, AD7779 ADC "..., and AD7779..." > + * > + * Copyright 2023-2024 Analog Devices Inc. > + */ ... > +#define AD7779_MAXCLK_LOWPOWER 4096000 Units? _HZ? _uV? ... > +#define GAIN_REL 0x555555 Is it something like making value for 12 channels? Can you elaborate a bit (perhaps in the comment)? ... > +struct ad7779_state { > + struct spi_device *spi; > + const struct ad7779_chip_info *chip_info; > + struct clk *mclk; > + struct iio_trigger *trig; > + struct completion completion; > + unsigned int sampling_freq; > + enum ad7779_filter filter_enabled; > + /* > + * DMA (thus cache coherency maintenance) requires the > + * transfer buffers to live in their own cache lines. > + */ > + u8 reg_rx_buf[3] __aligned(IIO_DMA_MINALIGN); > + u8 reg_tx_buf[3]; > + u32 spidata_rx[8]; > + u32 spidata_tx[8]; > + u8 reset_buf[8]; > +}; Have you run `pahole` to check if this can be optimised in size? ... > +static int ad7779_spi_read(struct ad7779_state *st, u8 reg, u8 *rbuf) > +{ > + int ret; > + int length = 3; > + u8 crc_buf[2]; > + u8 exp_crc = 0; > + struct spi_transfer reg_read_tr[] = { > + { > + .tx_buf = st->reg_tx_buf, > + .rx_buf = st->reg_rx_buf, > + }, > + }; > + > + if (reg == AD7779_REG_GEN_ERR_REG_1_EN) > + length = 2; Does it mean the crc byte will be ignored? If so, why do we even bother to spend resources on calculating it in that case? Same Q for other similar cases. > + reg_read_tr[0].len = length; > + > + st->reg_tx_buf[0] = AD7779_SPI_READ_CMD | FIELD_GET(AD7779_REG_MSK, reg); > + st->reg_tx_buf[1] = 0; > + st->reg_tx_buf[2] = crc8(ad7779_crc8_table, st->reg_tx_buf, 2, 0); > + > + ret = spi_sync_transfer(st->spi, reg_read_tr, ARRAY_SIZE(reg_read_tr)); > + if (ret) > + return ret; > + > + crc_buf[0] = AD7779_SPI_READ_CMD | FIELD_GET(AD7779_REG_MSK, reg); > + crc_buf[1] = st->reg_rx_buf[1]; > + exp_crc = crc8(ad7779_crc8_table, crc_buf, 2, 0); > + if (reg != AD7779_REG_GEN_ERR_REG_1_EN && exp_crc != st->reg_rx_buf[2]) { > + dev_err(&st->spi->dev, "Bad CRC %x, expected %x", > + st->reg_rx_buf[2], exp_crc); > + return -EINVAL; > + } > + *rbuf = st->reg_rx_buf[1]; > + > + return 0; > +} ... > + regval = data; > + regval &= ~mask; > + regval |= val; Traditional pattern is to have this as regval = (data & ~mask) | (val & mask); ... > +static int ad7779_set_sampling_frequency(struct ad7779_state *st, > + unsigned int sampling_freq) > +{ > + int ret; > + unsigned int dec; > + unsigned int div; > + unsigned int decimal; > + int temp; > + unsigned int kfreq; freq_khz will be better name > + if (st->filter_enabled == AD7779_SINC3 && > + sampling_freq > AD7779_SINC3_MAXFREQ) > + return -EINVAL; > + > + if (st->filter_enabled == AD7779_SINC5 && > + sampling_freq > AD7779_SINC5_MAXFREQ) > + return -EINVAL; > + > + if (sampling_freq > AD7779_SPIMODE_MAX_SAMP_FREQ) > + return -EINVAL; > + > + div = AD7779_HIGHPOWER_DIV; > + > + kfreq = sampling_freq / KILO; > + dec = div / kfreq; > + > + ret = ad7779_spi_write(st, AD7779_REG_SRC_N_MSB, > + FIELD_GET(AD7779_FREQ_MSB_MSK, dec)); > + if (ret) > + return ret; > + ret = ad7779_spi_write(st, AD7779_REG_SRC_N_LSB, > + FIELD_GET(AD7779_FREQ_LSB_MSK, dec)); > + if (ret) > + return ret; > + if (div % kfreq) { It's better to combine with / above, because some ISAs may have a single assembly instruction to do both at the same time. I'm not sure compiler (os some versions of it) may see this. dec = div / freq_khz; frac = div % freq_khz; ... if (frac) { ... } > + temp = (div * KILO) / kfreq; > + decimal = ((temp - dec * KILO) << 16) / KILO; I would add a small comment to show the formula behind this. > + ret = ad7779_spi_write(st, AD7779_REG_SRC_N_MSB, > + FIELD_GET(AD7779_FREQ_MSB_MSK, decimal)); > + if (ret) > + return ret; > + ret = ad7779_spi_write(st, AD7779_REG_SRC_N_LSB, > + FIELD_GET(AD7779_FREQ_LSB_MSK, decimal)); > + if (ret) > + return ret; > + } else { > + ret = ad7779_spi_write(st, AD7779_REG_SRC_N_MSB, > + FIELD_GET(AD7779_FREQ_MSB_MSK, 0x0)); > + if (ret) > + return ret; > + ret = ad7779_spi_write(st, AD7779_REG_SRC_N_LSB, > + FIELD_GET(AD7779_FREQ_LSB_MSK, 0x0)); > + if (ret) > + return ret; > + } > + ret = ad7779_spi_write(st, AD7779_REG_SRC_UPDATE, 0x1); > + if (ret) > + return ret; > + > + /* SRC update settling time */ > + fsleep(15); + Blank line, otherwise it's unclear how fsleep() is applied (or to which sections). > + ret = ad7779_spi_write(st, AD7779_REG_SRC_UPDATE, 0x0); > + if (ret) > + return ret; > + > + /* SRC update settling time */ > + fsleep(15); Alternatively make a helper to avoid potential desynchronisation in the code pieces statuc int ad7779_update_src(..., val) { ... /* ... */ fsleep(...); return 0; } > + st->sampling_freq = sampling_freq; > + > + return 0; > +} ... > +static int ad7779_set_calibscale(struct ad7779_state *st, int channel, int val) > +{ > + int ret; > + unsigned int gain; > + unsigned long long tmp; > + u8 gain_bytes[3]; > + > + tmp = val * 5592405LL; > + gain = DIV_ROUND_CLOSEST_ULL(tmp, MEGA); Formula to be explained? Magic number to be described (in the same comment), please? > + put_unaligned_be24(gain, gain_bytes); > + ret = ad7779_spi_write(st, > + AD7779_REG_CH_GAIN_UPPER_BYTE(channel), > + gain_bytes[0]); > + if (ret) > + return ret; > + > + ret = ad7779_spi_write(st, > + AD7779_REG_CH_GAIN_MID_BYTE(channel), > + gain_bytes[1]); > + if (ret) > + return ret; > + > + return ad7779_spi_write(st, > + AD7779_REG_CH_GAIN_LOWER_BYTE(channel), > + gain_bytes[2]); > +} ... > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { > + switch (mask) { > + case IIO_CHAN_INFO_CALIBSCALE: > + *val = ad7779_get_calibscale(st, chan->channel); > + if (*val < 0) > + return -EINVAL; > + *val2 = GAIN_REL; > + return IIO_VAL_FRACTIONAL; > + case IIO_CHAN_INFO_CALIBBIAS: > + *val = ad7779_get_calibbias(st, chan->channel); > + if (*val < 0) > + return -EINVAL; > + return IIO_VAL_INT; > + case IIO_CHAN_INFO_SAMP_FREQ: > + *val = st->sampling_freq; > + if (*val < 0) > + return -EINVAL; > + return IIO_VAL_INT; > + } > + return -EINVAL; > + } > + unreachable(); Hmm... Is it necessary? Same Q for other similar cases. I.o.w. what will be if we don't add this line? > +} ... > +static irqreturn_t ad7779_trigger_handler(int irq, void *p) > +{ > + struct iio_poll_func *pf = p; > + struct iio_dev *indio_dev = pf->indio_dev; > + struct ad7779_state *st = iio_priv(indio_dev); > + int ret; > + int bit; > + int k = 0; > + /* > + * Each channel shifts out HEADER + 24 bits of data therefore 8 * u32 > + * for the data and 64 bits for the timestamp > + */ May you do the respective structure and use aligned_s64 for the timestamp? > + u32 tmp[10]; > + > + struct spi_transfer sd_readback_tr[] = { > + { > + .rx_buf = st->spidata_rx, > + .tx_buf = st->spidata_tx, > + .len = AD7779_NUM_CHANNELS * AD7779_CHAN_DATA_SIZE, > + } > + }; > + > + if (!iio_buffer_enabled(indio_dev)) > + goto exit_handler; > + > + st->spidata_tx[0] = AD7779_SPI_READ_CMD; > + ret = spi_sync_transfer(st->spi, sd_readback_tr, > + ARRAY_SIZE(sd_readback_tr)); > + if (ret) { > + dev_err(&st->spi->dev, > + "spi transfer error in irq handler"); > + goto exit_handler; > + } > + > + for_each_set_bit(bit, indio_dev->active_scan_mask, AD7779_NUM_CHANNELS - 1) > + tmp[k++] = st->spidata_rx[bit]; > + > + iio_push_to_buffers_with_timestamp(indio_dev, &tmp[0], pf->timestamp); > + > +exit_handler: > + iio_trigger_notify_done(indio_dev->trig); > + return IRQ_HANDLED; > +} ... > + st->mclk = devm_clk_get_enabled(&spi->dev, "mclk"); > + if (IS_ERR(st->mclk)) > + return PTR_ERR(st->mclk); > + if (!spi->irq) > + return dev_err_probe(&spi->dev, ret, > + "DRDY irq not present\n"); This can be done much earlier and avoid unneeded resource allocations. ... > +static int ad7779_suspend(struct device *dev) > +{ > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > + struct ad7779_state *st = iio_priv(indio_dev); > + int ret; > + > + ret = ad7779_spi_write_mask(st, AD7779_REG_GENERAL_USER_CONFIG_1, > + AD7779_MOD_POWERMODE_MSK, > + FIELD_PREP(AD7779_MOD_POWERMODE_MSK, > + AD7779_LOW_POWER)); > + if (ret) > + return ret; > + > + return 0; return ad7779_spi_write_mask(...); > +} > + > +static int ad7779_resume(struct device *dev) > +{ > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > + struct ad7779_state *st = iio_priv(indio_dev); > + int ret; > + > + ret = ad7779_spi_write_mask(st, AD7779_REG_GENERAL_USER_CONFIG_1, > + AD7779_MOD_POWERMODE_MSK, > + FIELD_PREP(AD7779_MOD_POWERMODE_MSK, > + AD7779_HIGH_POWER)); > + if (ret) > + return ret; > + > + return 0; Ditto. > +} ... > +static const struct spi_device_id ad7779_id[] = { > + { > + .name = "ad7770", > + .driver_data = (kernel_ulong_t)&ad7770_chip_info Leave trailing comma. > + }, > + { > + .name = "ad7771", > + .driver_data = (kernel_ulong_t)&ad7771_chip_info Ditto. > + }, > + { > + .name = "ad7779", > + .driver_data = (kernel_ulong_t)&ad7779_chip_info Ditto. > + }, > + { } > +}; -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 3/3] drivers: iio: adc: add support for ad777x family 2024-09-12 15:15 ` Andy Shevchenko @ 2024-09-14 16:46 ` Jonathan Cameron 0 siblings, 0 replies; 24+ messages in thread From: Jonathan Cameron @ 2024-09-14 16:46 UTC (permalink / raw) To: Andy Shevchenko Cc: Ramona Alexandra Nechita, Lars-Peter Clausen, Cosmin Tanislav, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Nuno Sa, Andy Shevchenko, David Lechner, Marcelo Schmitt, Olivier Moysan, Dumitru Ceclan, Matteo Martelli, AngeloGioacchino Del Regno, Alisa-Dariana Roman, Ivan Mikhaylov, Mike Looijmans, linux-iio, linux-kernel, devicetree > ... > > > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { > > + switch (mask) { > > + case IIO_CHAN_INFO_CALIBSCALE: > > + *val = ad7779_get_calibscale(st, chan->channel); > > + if (*val < 0) > > + return -EINVAL; > > + *val2 = GAIN_REL; > > + return IIO_VAL_FRACTIONAL; > > + case IIO_CHAN_INFO_CALIBBIAS: > > + *val = ad7779_get_calibbias(st, chan->channel); > > + if (*val < 0) > > + return -EINVAL; > > + return IIO_VAL_INT; > > + case IIO_CHAN_INFO_SAMP_FREQ: > > + *val = st->sampling_freq; > > + if (*val < 0) > > + return -EINVAL; > > + return IIO_VAL_INT; > > + } > > + return -EINVAL; > > + } > > > + unreachable(); > > Hmm... Is it necessary? Same Q for other similar cases. I.o.w. what > will be if we don't add this line? The compiler can't tell that the contents of iio_device_claim_direct_scoped() always runs. Hence normal result is it complains that nothing was returned. Why the compiler can't figure out? Who knows... Jonathan ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 3/3] drivers: iio: adc: add support for ad777x family 2024-09-12 12:15 ` [PATCH v5 3/3] drivers: iio: adc: add support for ad777x family Ramona Alexandra Nechita 2024-09-12 15:15 ` Andy Shevchenko @ 2024-09-14 1:44 ` kernel test robot 2024-09-14 16:48 ` Jonathan Cameron 2024-09-14 17:06 ` Jonathan Cameron 2 siblings, 1 reply; 24+ messages in thread From: kernel test robot @ 2024-09-14 1:44 UTC (permalink / raw) To: Ramona Alexandra Nechita, Jonathan Cameron, Lars-Peter Clausen, Cosmin Tanislav, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Nuno Sa, Andy Shevchenko, David Lechner, Marcelo Schmitt, Olivier Moysan, Dumitru Ceclan, Matteo Martelli, AngeloGioacchino Del Regno, Alisa-Dariana Roman, Ivan Mikhaylov, Mike Looijmans, linux-iio, linux-kernel, devicetree Cc: llvm, oe-kbuild-all Hi Ramona, kernel test robot noticed the following build warnings: [auto build test WARNING on jic23-iio/togreg] [also build test WARNING on robh/for-next linus/master v6.11-rc7 next-20240913] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Ramona-Alexandra-Nechita/dt-bindings-iio-adc-add-a7779-doc/20240912-201936 base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg patch link: https://lore.kernel.org/r/20240912121609.13438-4-ramona.nechita%40analog.com patch subject: [PATCH v5 3/3] drivers: iio: adc: add support for ad777x family config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240914/202409140955.bz1rH7Q6-lkp@intel.com/config) compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project bf684034844c660b778f0eba103582f582b710c9) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240914/202409140955.bz1rH7Q6-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202409140955.bz1rH7Q6-lkp@intel.com/ All warnings (new ones prefixed by >>): In file included from drivers/iio/adc/ad7779.c:15: In file included from include/linux/interrupt.h:11: In file included from include/linux/hardirq.h:11: In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:14: In file included from arch/hexagon/include/asm/io.h:328: include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 548 | val = __raw_readb(PCI_IOBASE + addr); | ~~~~~~~~~~ ^ include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 561 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr)); | ~~~~~~~~~~ ^ include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu' 37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x)) | ^ In file included from drivers/iio/adc/ad7779.c:15: In file included from include/linux/interrupt.h:11: In file included from include/linux/hardirq.h:11: In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:14: In file included from arch/hexagon/include/asm/io.h:328: include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 574 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); | ~~~~~~~~~~ ^ include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu' 35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x)) | ^ In file included from drivers/iio/adc/ad7779.c:15: In file included from include/linux/interrupt.h:11: In file included from include/linux/hardirq.h:11: In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:17: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:14: In file included from arch/hexagon/include/asm/io.h:328: include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 585 | __raw_writeb(value, PCI_IOBASE + addr); | ~~~~~~~~~~ ^ include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 595 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr); | ~~~~~~~~~~ ^ include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] 605 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr); | ~~~~~~~~~~ ^ In file included from drivers/iio/adc/ad7779.c:19: In file included from include/linux/regulator/consumer.h:35: In file included from include/linux/suspend.h:5: In file included from include/linux/swap.h:9: In file included from include/linux/memcontrol.h:21: In file included from include/linux/mm.h:2232: include/linux/vmstat.h:517:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion] 517 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_" | ~~~~~~~~~~~ ^ ~~~ >> drivers/iio/adc/ad7779.c:748:35: warning: variable 'ret' is uninitialized when used here [-Wuninitialized] 748 | return dev_err_probe(&spi->dev, ret, | ^~~ drivers/iio/adc/ad7779.c:735:9: note: initialize the variable 'ret' to silence this warning 735 | int ret; | ^ | = 0 8 warnings generated. vim +/ret +748 drivers/iio/adc/ad7779.c 729 730 static int ad7779_probe(struct spi_device *spi) 731 { 732 struct iio_dev *indio_dev; 733 struct ad7779_state *st; 734 struct gpio_desc *reset_gpio, *start_gpio; 735 int ret; 736 737 indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st)); 738 if (!indio_dev) 739 return -ENOMEM; 740 741 st = iio_priv(indio_dev); 742 743 st->mclk = devm_clk_get_enabled(&spi->dev, "mclk"); 744 if (IS_ERR(st->mclk)) 745 return PTR_ERR(st->mclk); 746 747 if (!spi->irq) > 748 return dev_err_probe(&spi->dev, ret, 749 "DRDY irq not present\n"); 750 751 reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW); 752 if (IS_ERR(reset_gpio)) 753 return PTR_ERR(reset_gpio); 754 755 start_gpio = devm_gpiod_get(&spi->dev, "start", GPIOD_OUT_HIGH); 756 if (IS_ERR(start_gpio)) 757 return PTR_ERR(start_gpio); 758 759 crc8_populate_msb(ad7779_crc8_table, AD7779_CRC8_POLY); 760 st->spi = spi; 761 762 st->chip_info = spi_get_device_match_data(spi); 763 if (!st->chip_info) 764 return -ENODEV; 765 766 ret = ad7779_reset(indio_dev, reset_gpio); 767 if (ret) 768 return ret; 769 770 ad7779_powerup(st, start_gpio); 771 if (ret) 772 return ret; 773 774 indio_dev->name = st->chip_info->name; 775 indio_dev->info = &ad7779_info; 776 indio_dev->modes = INDIO_DIRECT_MODE; 777 indio_dev->channels = st->chip_info->channels; 778 indio_dev->num_channels = ARRAY_SIZE(ad7779_channels); 779 780 st->trig = devm_iio_trigger_alloc(&spi->dev, "%s-dev%d", 781 indio_dev->name, iio_device_id(indio_dev)); 782 if (!st->trig) 783 return -ENOMEM; 784 785 st->trig->ops = &ad7779_trigger_ops; 786 787 iio_trigger_set_drvdata(st->trig, st); 788 789 ret = devm_request_irq(&spi->dev, spi->irq, 790 iio_trigger_generic_data_rdy_poll, 791 IRQF_ONESHOT | IRQF_NO_AUTOEN, 792 indio_dev->name, st->trig); 793 if (ret) 794 return dev_err_probe(&spi->dev, ret, "request irq %d failed\n", 795 st->spi->irq); 796 797 ret = devm_iio_trigger_register(&spi->dev, st->trig); 798 if (ret) 799 return ret; 800 801 indio_dev->trig = iio_trigger_get(st->trig); 802 803 init_completion(&st->completion); 804 805 ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev, 806 &iio_pollfunc_store_time, 807 &ad7779_trigger_handler, 808 &ad7779_buffer_setup_ops); 809 if (ret) 810 return ret; 811 812 ret = ad7779_spi_write_mask(st, AD7779_REG_DOUT_FORMAT, 813 AD7779_DCLK_CLK_DIV_MSK, 814 FIELD_PREP(AD7779_DCLK_CLK_DIV_MSK, 7)); 815 if (ret) 816 return ret; 817 818 return devm_iio_device_register(&spi->dev, indio_dev); 819 } 820 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 3/3] drivers: iio: adc: add support for ad777x family 2024-09-14 1:44 ` kernel test robot @ 2024-09-14 16:48 ` Jonathan Cameron 0 siblings, 0 replies; 24+ messages in thread From: Jonathan Cameron @ 2024-09-14 16:48 UTC (permalink / raw) To: kernel test robot Cc: Ramona Alexandra Nechita, Lars-Peter Clausen, Cosmin Tanislav, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Nuno Sa, Andy Shevchenko, David Lechner, Marcelo Schmitt, Olivier Moysan, Dumitru Ceclan, Matteo Martelli, AngeloGioacchino Del Regno, Alisa-Dariana Roman, Ivan Mikhaylov, Mike Looijmans, linux-iio, linux-kernel, devicetree, llvm, oe-kbuild-all skip "nr_" > | ~~~~~~~~~~~ ^ ~~~ > >> drivers/iio/adc/ad7779.c:748:35: warning: variable 'ret' is uninitialized when used here [-Wuninitialized] > 748 | return dev_err_probe(&spi->dev, ret, > | ^~~ > drivers/iio/adc/ad7779.c:735:9: note: initialize the variable 'ret' to silence this warning > 735 | int ret; > | ^ > | = 0 Definitely don't do that! just replace ret with -EINVAL Jonathan > 8 warnings generated. > > > vim +/ret +748 drivers/iio/adc/ad7779.c > > 729 > 730 static int ad7779_probe(struct spi_device *spi) > 731 { > 732 struct iio_dev *indio_dev; > 733 struct ad7779_state *st; > 734 struct gpio_desc *reset_gpio, *start_gpio; > 735 int ret; > 736 > 737 indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st)); > 738 if (!indio_dev) > 739 return -ENOMEM; > 740 > 741 st = iio_priv(indio_dev); > 742 > 743 st->mclk = devm_clk_get_enabled(&spi->dev, "mclk"); > 744 if (IS_ERR(st->mclk)) > 745 return PTR_ERR(st->mclk); > 746 > 747 if (!spi->irq) > > 748 return dev_err_probe(&spi->dev, ret, > 749 "DRDY irq not present\n"); > 750 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 3/3] drivers: iio: adc: add support for ad777x family 2024-09-12 12:15 ` [PATCH v5 3/3] drivers: iio: adc: add support for ad777x family Ramona Alexandra Nechita 2024-09-12 15:15 ` Andy Shevchenko 2024-09-14 1:44 ` kernel test robot @ 2024-09-14 17:06 ` Jonathan Cameron 2024-09-20 13:24 ` Nechita, Ramona 2024-09-26 13:41 ` Nechita, Ramona 2 siblings, 2 replies; 24+ messages in thread From: Jonathan Cameron @ 2024-09-14 17:06 UTC (permalink / raw) To: Ramona Alexandra Nechita Cc: Lars-Peter Clausen, Cosmin Tanislav, Michael Hennerich, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Nuno Sa, Andy Shevchenko, David Lechner, Marcelo Schmitt, Olivier Moysan, Dumitru Ceclan, Matteo Martelli, AngeloGioacchino Del Regno, Alisa-Dariana Roman, Ivan Mikhaylov, Mike Looijmans, linux-iio, linux-kernel, devicetree On Thu, 12 Sep 2024 15:15:47 +0300 Ramona Alexandra Nechita <ramona.nechita@analog.com> wrote: > Add support for AD7770, AD7771, AD7779 ADCs. The device is capable of > sending out data both on DOUT lines interface,as on the SDO line. > The driver currently implements only the SDO data streaming mode. SPI > communication is used alternatively for accessing registers and streaming > data. Register access are protected by crc8. > > Signed-off-by: Ramona Alexandra Nechita <ramona.nechita@analog.com> Hi Ramona, A few additional comments inline, Jonathan > diff --git a/drivers/iio/adc/ad7779.c b/drivers/iio/adc/ad7779.c > new file mode 100644 > index 000000000000..05ae72257c9e > --- /dev/null > +++ b/drivers/iio/adc/ad7779.c > @@ -0,0 +1,917 @@ > +static int ad7779_set_calibbias(struct ad7779_state *st, int channel, int val) > +{ > + int ret; > + u8 calibbias[3]; > + > + put_unaligned_be24(val, calibbias); > + ret = ad7779_spi_write(st, > + AD7779_REG_CH_OFFSET_UPPER_BYTE(channel), > + calibbias[0]); > + if (ret) > + return ret; > + > + ret = ad7779_spi_write(st, > + AD7779_REG_CH_OFFSET_MID_BYTE(channel), > + calibbias[1]); > + if (ret) > + return ret; > + > + return ad7779_spi_write(st, > + AD7779_REG_CH_OFFSET_LOWER_BYTE(channel), Wrap closer to 80 chars. return ad7779_spi_write(st, AD7779_REG_CH_OFFSET_LOWER_BYTE(channel), calibbias[2]); etc as it'll shorted the code a little bit for no significant loss of readability. Do the same for all other such cases. > +} > + > +static irqreturn_t ad7779_trigger_handler(int irq, void *p) > +{ > + struct iio_poll_func *pf = p; > + struct iio_dev *indio_dev = pf->indio_dev; > + struct ad7779_state *st = iio_priv(indio_dev); > + int ret; > + int bit; > + int k = 0; > + /* > + * Each channel shifts out HEADER + 24 bits of data therefore 8 * u32 > + * for the data and 64 bits for the timestamp > + */ > + u32 tmp[10]; > + > + struct spi_transfer sd_readback_tr[] = { > + { > + .rx_buf = st->spidata_rx, > + .tx_buf = st->spidata_tx, > + .len = AD7779_NUM_CHANNELS * AD7779_CHAN_DATA_SIZE, > + } > + }; > + > + if (!iio_buffer_enabled(indio_dev)) > + goto exit_handler; If buffers aren't enabled, the push to buffers won't do anything. So this race shouldn't matter. If it does, what happens? I'm curious because I'd expect any races that cause trouble in this case to be pretty universal across drivers. > + > + st->spidata_tx[0] = AD7779_SPI_READ_CMD; > + ret = spi_sync_transfer(st->spi, sd_readback_tr, > + ARRAY_SIZE(sd_readback_tr)); > + if (ret) { > + dev_err(&st->spi->dev, > + "spi transfer error in irq handler"); > + goto exit_handler; > + } > + > + for_each_set_bit(bit, indio_dev->active_scan_mask, AD7779_NUM_CHANNELS - 1) > + tmp[k++] = st->spidata_rx[bit]; If you can't use the core demux handling + available_scan_masks, please add a comment here to say why. That would allow you to do the SPI transfer directly into the buffer you then push below. The IIO core will figure out how to pull data out of that if the user wants a subset of channels. > + > + iio_push_to_buffers_with_timestamp(indio_dev, &tmp[0], pf->timestamp); > + > +exit_handler: > + iio_trigger_notify_done(indio_dev->trig); > + return IRQ_HANDLED; > +} > + > +static int ad7779_reset(struct iio_dev *indio_dev, struct gpio_desc *reset_gpio) > +{ > + struct ad7779_state *st = iio_priv(indio_dev); > + int ret; > + struct spi_transfer reg_read_tr[] = { > + { > + .tx_buf = st->reset_buf, > + .len = 8, > + }, > + }; > + > + memset(st->reset_buf, 0xff, sizeof(st->reset_buf)); > + > + if (reset_gpio) { > + /* Delay for reset to occur is 225 microseconds*/ > + gpiod_set_value(reset_gpio, 1); > + fsleep(230); > + return 0; + } if (reset_gpio) { /* Delay for reset to occur is 225 microseconds*/ gpiod_set_value(reset_gpio, 1); } else { struct spi_transfer reg_read_tr[] = { { .tx_buf = st->reset_buf, .len = 8, }, }; int ret; memset(st->reset_buf, 0xff, sizeof(st->reset_buf)); ret = spi_sync_transfer(st->spi, reg_read_tr, ARRAY_SIZE(reg_read_tr)); if (ret) return ret; } fsleep(230); return 0; or something along those lines. Shares the sleep so showing the wait is the same in both types of reset and doesn't do a memset that I think is otherwise irrelevant if there is a gpio. > + > + ret = spi_sync_transfer(st->spi, reg_read_tr, > + ARRAY_SIZE(reg_read_tr)); > + if (ret) > + return ret; > + > + /* Delay for reset to occur is 225 microseconds*/ > + fsleep(230); > + > + return 0; > +} > +static int ad7779_probe(struct spi_device *spi) > +{ > + struct iio_dev *indio_dev; > + struct ad7779_state *st; > + struct gpio_desc *reset_gpio, *start_gpio; > + int ret; > + > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st)); > + if (!indio_dev) > + return -ENOMEM; > + > + st = iio_priv(indio_dev); > + > + st->mclk = devm_clk_get_enabled(&spi->dev, "mclk"); > + if (IS_ERR(st->mclk)) > + return PTR_ERR(st->mclk); > + > + if (!spi->irq) > + return dev_err_probe(&spi->dev, ret, > + "DRDY irq not present\n"); > + > + reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW); > + if (IS_ERR(reset_gpio)) > + return PTR_ERR(reset_gpio); > + > + start_gpio = devm_gpiod_get(&spi->dev, "start", GPIOD_OUT_HIGH); > + if (IS_ERR(start_gpio)) > + return PTR_ERR(start_gpio); > + > + crc8_populate_msb(ad7779_crc8_table, AD7779_CRC8_POLY); > + st->spi = spi; > + > + st->chip_info = spi_get_device_match_data(spi); > + if (!st->chip_info) > + return -ENODEV; > + > + ret = ad7779_reset(indio_dev, reset_gpio); > + if (ret) > + return ret; > + > + ad7779_powerup(st, start_gpio); > + if (ret) > + return ret; What powers the device down again if we hit an error? Probably need a devm_add_action_or_reset() or if it self powers down may a comment on that. > + > + indio_dev->name = st->chip_info->name; > + indio_dev->info = &ad7779_info; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->channels = st->chip_info->channels; > + indio_dev->num_channels = ARRAY_SIZE(ad7779_channels); > + > + st->trig = devm_iio_trigger_alloc(&spi->dev, "%s-dev%d", > + indio_dev->name, iio_device_id(indio_dev)); > + if (!st->trig) > + return -ENOMEM; > + > + st->trig->ops = &ad7779_trigger_ops; > + > + iio_trigger_set_drvdata(st->trig, st); > + > + ret = devm_request_irq(&spi->dev, spi->irq, > + iio_trigger_generic_data_rdy_poll, > + IRQF_ONESHOT | IRQF_NO_AUTOEN, > + indio_dev->name, st->trig); > + if (ret) > + return dev_err_probe(&spi->dev, ret, "request irq %d failed\n", > + st->spi->irq); > + > + ret = devm_iio_trigger_register(&spi->dev, st->trig); > + if (ret) > + return ret; > + > + indio_dev->trig = iio_trigger_get(st->trig); > + > + init_completion(&st->completion); > + > + ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev, > + &iio_pollfunc_store_time, > + &ad7779_trigger_handler, > + &ad7779_buffer_setup_ops); > + if (ret) > + return ret; > + > + ret = ad7779_spi_write_mask(st, AD7779_REG_DOUT_FORMAT, > + AD7779_DCLK_CLK_DIV_MSK, > + FIELD_PREP(AD7779_DCLK_CLK_DIV_MSK, 7)); > + if (ret) > + return ret; > + > + return devm_iio_device_register(&spi->dev, indio_dev); > +} > + > +static int ad7779_suspend(struct device *dev) > +{ > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > + struct ad7779_state *st = iio_priv(indio_dev); > + int ret; > + > + ret = ad7779_spi_write_mask(st, AD7779_REG_GENERAL_USER_CONFIG_1, > + AD7779_MOD_POWERMODE_MSK, > + FIELD_PREP(AD7779_MOD_POWERMODE_MSK, > + AD7779_LOW_POWER)); > + if (ret) > + return ret; As below. Given if !ret, ret == 0 so same as just returning it unconditionally. > + > + return 0; > +} > + > +static int ad7779_resume(struct device *dev) > +{ > + struct iio_dev *indio_dev = dev_get_drvdata(dev); > + struct ad7779_state *st = iio_priv(indio_dev); > + int ret; > + > + ret = ad7779_spi_write_mask(st, AD7779_REG_GENERAL_USER_CONFIG_1, > + AD7779_MOD_POWERMODE_MSK, > + FIELD_PREP(AD7779_MOD_POWERMODE_MSK, > + AD7779_HIGH_POWER)); > + if (ret) > + return ret; return ad7779_spi_write_mask... > + > + return 0; > +} ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH v5 3/3] drivers: iio: adc: add support for ad777x family 2024-09-14 17:06 ` Jonathan Cameron @ 2024-09-20 13:24 ` Nechita, Ramona 2024-09-23 12:51 ` David Lechner 2024-09-26 13:41 ` Nechita, Ramona 1 sibling, 1 reply; 24+ messages in thread From: Nechita, Ramona @ 2024-09-20 13:24 UTC (permalink / raw) To: Jonathan Cameron Cc: Lars-Peter Clausen, Tanislav, Cosmin, Hennerich, Michael, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Sa, Nuno, Andy Shevchenko, David Lechner, Schmitt, Marcelo, Olivier Moysan, Dumitru Ceclan, Matteo Martelli, AngeloGioacchino Del Regno, Alisa-Dariana Roman, Ivan Mikhaylov, Mike Looijmans, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org Hello all, Just a minor question ... > >> + >> +static irqreturn_t ad7779_trigger_handler(int irq, void *p) { >> + struct iio_poll_func *pf = p; >> + struct iio_dev *indio_dev = pf->indio_dev; >> + struct ad7779_state *st = iio_priv(indio_dev); >> + int ret; >> + int bit; >> + int k = 0; >> + /* >> + * Each channel shifts out HEADER + 24 bits of data therefore 8 * u32 >> + * for the data and 64 bits for the timestamp >> + */ >> + u32 tmp[10]; >> + >> + struct spi_transfer sd_readback_tr[] = { >> + { >> + .rx_buf = st->spidata_rx, >> + .tx_buf = st->spidata_tx, >> + .len = AD7779_NUM_CHANNELS * AD7779_CHAN_DATA_SIZE, >> + } >> + }; >> + >> + if (!iio_buffer_enabled(indio_dev)) >> + goto exit_handler; > >If buffers aren't enabled, the push to buffers won't do anything. So this race shouldn't matter. If it does, what happens? >I'm curious because I'd expect any races that cause trouble in this case to be pretty universal across drivers. I added that condition rather because the DRDY pulse will keep on being generated even when the buffers are not active, and it would be better to exit the function sooner. I tested it and it does not break to remove the condition, I just thought it made more sense like this. Should I delete it? >.... Best regards, Ramona Nechita ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 3/3] drivers: iio: adc: add support for ad777x family 2024-09-20 13:24 ` Nechita, Ramona @ 2024-09-23 12:51 ` David Lechner 2024-09-28 14:29 ` Jonathan Cameron 0 siblings, 1 reply; 24+ messages in thread From: David Lechner @ 2024-09-23 12:51 UTC (permalink / raw) To: Nechita, Ramona Cc: Jonathan Cameron, Lars-Peter Clausen, Tanislav, Cosmin, Hennerich, Michael, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Sa, Nuno, Andy Shevchenko, Schmitt, Marcelo, Olivier Moysan, Dumitru Ceclan, Matteo Martelli, AngeloGioacchino Del Regno, Alisa-Dariana Roman, Ivan Mikhaylov, Mike Looijmans, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org On Fri, Sep 20, 2024 at 3:24 PM Nechita, Ramona <Ramona.Nechita@analog.com> wrote: > > Hello all, > > Just a minor question > ... > > > >> + > >> +static irqreturn_t ad7779_trigger_handler(int irq, void *p) { > >> + struct iio_poll_func *pf = p; > >> + struct iio_dev *indio_dev = pf->indio_dev; > >> + struct ad7779_state *st = iio_priv(indio_dev); > >> + int ret; > >> + int bit; > >> + int k = 0; > >> + /* > >> + * Each channel shifts out HEADER + 24 bits of data therefore 8 * u32 > >> + * for the data and 64 bits for the timestamp > >> + */ > >> + u32 tmp[10]; > >> + > >> + struct spi_transfer sd_readback_tr[] = { > >> + { > >> + .rx_buf = st->spidata_rx, > >> + .tx_buf = st->spidata_tx, > >> + .len = AD7779_NUM_CHANNELS * AD7779_CHAN_DATA_SIZE, > >> + } > >> + }; > >> + > >> + if (!iio_buffer_enabled(indio_dev)) > >> + goto exit_handler; > > > >If buffers aren't enabled, the push to buffers won't do anything. So this race shouldn't matter. If it does, what happens? > >I'm curious because I'd expect any races that cause trouble in this case to be pretty universal across drivers. > > I added that condition rather because the DRDY pulse will keep on being generated even when the buffers are not active, > and it would be better to exit the function sooner. I tested it and it does not break to remove the condition, I just > thought it made more sense like this. Should I delete it? > > >.... > > Best regards, > Ramona Nechita > > Perhaps a better way to handle this would be to move disable_irq(st->spi->irq); to the buffer predisable callback instead of doing it in the buffer postdisable callback. Then we will be sure to not get any more DRDY interrupts after the buffer is disabled. (And to keep things balanced, moved the corresponding irq_enable() to the buffer postenable callback.) Since ad7779_trigger_handler is the IIO trigger interrupt handler and not the DRDY interrupt handler though, it is already not possible for this interrupt handler to be called while the IIO buffer is enabled. So it should be safe to remove the if (!iio_buffere_enabled(indio_dev)) even without the other changes I suggested. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 3/3] drivers: iio: adc: add support for ad777x family 2024-09-23 12:51 ` David Lechner @ 2024-09-28 14:29 ` Jonathan Cameron 0 siblings, 0 replies; 24+ messages in thread From: Jonathan Cameron @ 2024-09-28 14:29 UTC (permalink / raw) To: David Lechner Cc: Nechita, Ramona, Lars-Peter Clausen, Tanislav, Cosmin, Hennerich, Michael, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Sa, Nuno, Andy Shevchenko, Schmitt, Marcelo, Olivier Moysan, Dumitru Ceclan, Matteo Martelli, AngeloGioacchino Del Regno, Alisa-Dariana Roman, Ivan Mikhaylov, Mike Looijmans, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org On Mon, 23 Sep 2024 14:51:28 +0200 David Lechner <dlechner@baylibre.com> wrote: > On Fri, Sep 20, 2024 at 3:24 PM Nechita, Ramona > <Ramona.Nechita@analog.com> wrote: > > > > Hello all, > > > > Just a minor question > > ... > > > > > >> + > > >> +static irqreturn_t ad7779_trigger_handler(int irq, void *p) { > > >> + struct iio_poll_func *pf = p; > > >> + struct iio_dev *indio_dev = pf->indio_dev; > > >> + struct ad7779_state *st = iio_priv(indio_dev); > > >> + int ret; > > >> + int bit; > > >> + int k = 0; > > >> + /* > > >> + * Each channel shifts out HEADER + 24 bits of data therefore 8 * u32 > > >> + * for the data and 64 bits for the timestamp > > >> + */ > > >> + u32 tmp[10]; > > >> + > > >> + struct spi_transfer sd_readback_tr[] = { > > >> + { > > >> + .rx_buf = st->spidata_rx, > > >> + .tx_buf = st->spidata_tx, > > >> + .len = AD7779_NUM_CHANNELS * AD7779_CHAN_DATA_SIZE, > > >> + } > > >> + }; > > >> + > > >> + if (!iio_buffer_enabled(indio_dev)) > > >> + goto exit_handler; > > > > > >If buffers aren't enabled, the push to buffers won't do anything. So this race shouldn't matter. If it does, what happens? > > >I'm curious because I'd expect any races that cause trouble in this case to be pretty universal across drivers. > > > > I added that condition rather because the DRDY pulse will keep on being generated even when the buffers are not active, > > and it would be better to exit the function sooner. I tested it and it does not break to remove the condition, I just > > thought it made more sense like this. Should I delete it? > > > > >.... > > > > Best regards, > > Ramona Nechita > > > > > > Perhaps a better way to handle this would be to move > > disable_irq(st->spi->irq); > > to the buffer predisable callback instead of doing it in the buffer > postdisable callback. Then we will be sure to not get any more DRDY > interrupts after the buffer is disabled. > > (And to keep things balanced, moved the corresponding irq_enable() to > the buffer postenable callback.) That makes logical sense but in reality in the vast majority of cases it makes no practical difference whether things are in the pre or post callbacks as the fundamental races with tear down are there in both cases but shouldn't matter as they correspond to slightly earlier or later disabling of the buffer. So this is a nice to have for readabilty and understanding rather than a required change I think. > > Since ad7779_trigger_handler is the IIO trigger interrupt handler and > not the DRDY interrupt handler though, it is already not possible for > this interrupt handler to be called while the IIO buffer is enabled. > So it should be safe to remove the if > (!iio_buffere_enabled(indio_dev)) even without the other changes I > suggested. Exactly. Jonathan ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH v5 3/3] drivers: iio: adc: add support for ad777x family 2024-09-14 17:06 ` Jonathan Cameron 2024-09-20 13:24 ` Nechita, Ramona @ 2024-09-26 13:41 ` Nechita, Ramona 2024-09-28 14:31 ` Jonathan Cameron 1 sibling, 1 reply; 24+ messages in thread From: Nechita, Ramona @ 2024-09-26 13:41 UTC (permalink / raw) To: Jonathan Cameron Cc: Lars-Peter Clausen, Tanislav, Cosmin, Hennerich, Michael, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Sa, Nuno, Andy Shevchenko, David Lechner, Schmitt, Marcelo, Olivier Moysan, Dumitru Ceclan, Matteo Martelli, AngeloGioacchino Del Regno, Alisa-Dariana Roman, Ivan Mikhaylov, Mike Looijmans, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org Hello all, There is another thing that I forgot to mention inline with my last email. > Add support for AD7770, AD7771, AD7779 ADCs. The device is capable of > sending out data both on DOUT lines interface,as on the SDO line. > The driver currently implements only the SDO data streaming mode. SPI > communication is used alternatively for accessing registers and > streaming data. Register access are protected by crc8. > > Signed-off-by: Ramona Alexandra Nechita <ramona.nechita@analog.com> Hi Ramona, .... >> +static int ad7779_probe(struct spi_device *spi) { >> + struct iio_dev *indio_dev; >> + struct ad7779_state *st; >> + struct gpio_desc *reset_gpio, *start_gpio; >> + int ret; >> + >> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st)); >> + if (!indio_dev) >> + return -ENOMEM; >> + >> + st = iio_priv(indio_dev); >> + >> + st->mclk = devm_clk_get_enabled(&spi->dev, "mclk"); >> + if (IS_ERR(st->mclk)) >> + return PTR_ERR(st->mclk); >> + >> + if (!spi->irq) >> + return dev_err_probe(&spi->dev, ret, >> + "DRDY irq not present\n"); >> + >> + reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_LOW); >> + if (IS_ERR(reset_gpio)) >> + return PTR_ERR(reset_gpio); >> + >> + start_gpio = devm_gpiod_get(&spi->dev, "start", GPIOD_OUT_HIGH); >> + if (IS_ERR(start_gpio)) >> + return PTR_ERR(start_gpio); >> + >> + crc8_populate_msb(ad7779_crc8_table, AD7779_CRC8_POLY); >> + st->spi = spi; >> + >> + st->chip_info = spi_get_device_match_data(spi); >> + if (!st->chip_info) >> + return -ENODEV; >> + >> + ret = ad7779_reset(indio_dev, reset_gpio); >> + if (ret) >> + return ret; >> + >> + ad7779_powerup(st, start_gpio); >> + if (ret) >> + return ret; >What powers the device down again if we hit an error? > >Probably need a devm_add_action_or_reset() or if it self powers down may a comment on that. In the powerup function there are only some register writes and the start gpio is only a synchronization pulse (perhaps the name powerup is not very appropriate), would an action or reset be necessary in this case? Since the regulators are not used in the driver, should there be a function disabling them anyway? Best Regards, Ramona ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 3/3] drivers: iio: adc: add support for ad777x family 2024-09-26 13:41 ` Nechita, Ramona @ 2024-09-28 14:31 ` Jonathan Cameron 2024-10-10 14:35 ` Nechita, Ramona 0 siblings, 1 reply; 24+ messages in thread From: Jonathan Cameron @ 2024-09-28 14:31 UTC (permalink / raw) To: Nechita, Ramona Cc: Lars-Peter Clausen, Tanislav, Cosmin, Hennerich, Michael, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Sa, Nuno, Andy Shevchenko, David Lechner, Schmitt, Marcelo, Olivier Moysan, Dumitru Ceclan, Matteo Martelli, AngeloGioacchino Del Regno, Alisa-Dariana Roman, Ivan Mikhaylov, Mike Looijmans, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org > >> + > >> + ret = ad7779_reset(indio_dev, reset_gpio); > >> + if (ret) > >> + return ret; > >> + > >> + ad7779_powerup(st, start_gpio); > >> + if (ret) > >> + return ret; > >What powers the device down again if we hit an error? > > > >Probably need a devm_add_action_or_reset() or if it self powers down may a comment on that. > > In the powerup function there are only some register writes and the start gpio is only a synchronization pulse (perhaps the name powerup is not very appropriate), > would an action or reset be necessary in this case? Since the regulators are not used in the driver, should there be a function disabling them anyway? > If there is nothing useful to do indeed not but when I see a power up, I rather expect a power down. Is there anything that can do that or is it a case of it will go to sleep anyway for some other reason? Jonathan > > Best Regards, > Ramona > ^ permalink raw reply [flat|nested] 24+ messages in thread
* RE: [PATCH v5 3/3] drivers: iio: adc: add support for ad777x family 2024-09-28 14:31 ` Jonathan Cameron @ 2024-10-10 14:35 ` Nechita, Ramona 2024-10-10 16:40 ` Jonathan Cameron 0 siblings, 1 reply; 24+ messages in thread From: Nechita, Ramona @ 2024-10-10 14:35 UTC (permalink / raw) To: Jonathan Cameron Cc: Lars-Peter Clausen, Tanislav, Cosmin, Hennerich, Michael, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Sa, Nuno, Andy Shevchenko, David Lechner, Schmitt, Marcelo, Olivier Moysan, Dumitru Ceclan, Matteo Martelli, AngeloGioacchino Del Regno, Alisa-Dariana Roman, Ivan Mikhaylov, Mike Looijmans, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org Hello Jonathan, >> >> + >> >> + ret = ad7779_reset(indio_dev, reset_gpio); >> >> + if (ret) >> >> + return ret; >> >> + >> >> + ad7779_powerup(st, start_gpio); >> >> + if (ret) >> >> + return ret; >> >What powers the device down again if we hit an error? >> > >> >Probably need a devm_add_action_or_reset() or if it self powers down may a comment on that. >> >> In the powerup function there are only some register writes and the >> start gpio is only a synchronization pulse (perhaps the name powerup is not very appropriate), would an action or reset be necessary in this case? Since the regulators are not used in the driver, should there be a function disabling them anyway? >> >If there is nothing useful to do indeed not but when I see a power up, I rather expect a power down. Is there anything that can do that or is it a case of it will go to sleep anyway for some other reason? I don't think there would be anything to do in a powerdown function specifically, but I could rename the powerup function to "_config" or something similar, to make it more intuitive. > Best Regards, Ramona ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 3/3] drivers: iio: adc: add support for ad777x family 2024-10-10 14:35 ` Nechita, Ramona @ 2024-10-10 16:40 ` Jonathan Cameron 0 siblings, 0 replies; 24+ messages in thread From: Jonathan Cameron @ 2024-10-10 16:40 UTC (permalink / raw) To: Nechita, Ramona Cc: Jonathan Cameron, Lars-Peter Clausen, Tanislav, Cosmin, Hennerich, Michael, Rob Herring, Krzysztof Kozlowski, Conor Dooley, Sa, Nuno, Andy Shevchenko, David Lechner, Schmitt, Marcelo, Olivier Moysan, Dumitru Ceclan, Matteo Martelli, AngeloGioacchino Del Regno, Alisa-Dariana Roman, Ivan Mikhaylov, Mike Looijmans, linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org On Thu, 10 Oct 2024 14:35:52 +0000 "Nechita, Ramona" <Ramona.Nechita@analog.com> wrote: > Hello Jonathan, > > >> >> + > >> >> + ret = ad7779_reset(indio_dev, reset_gpio); > >> >> + if (ret) > >> >> + return ret; > >> >> + > >> >> + ad7779_powerup(st, start_gpio); > >> >> + if (ret) > >> >> + return ret; > >> >What powers the device down again if we hit an error? > >> > > >> >Probably need a devm_add_action_or_reset() or if it self powers down may a comment on that. > >> > >> In the powerup function there are only some register writes and the > >> start gpio is only a synchronization pulse (perhaps the name powerup is not very appropriate), would an action or reset be necessary in this case? Since the regulators are not used in the driver, should there be a function disabling them anyway? > >> > >If there is nothing useful to do indeed not but when I see a power up, I rather expect a power down. Is there anything that can do that or is it a case of it will go to sleep anyway for some other reason? > > I don't think there would be anything to do in a powerdown function specifically, but I could rename the powerup function to "_config" or something similar, to make it more intuitive. ok. That will at least make me less suspicious when I read this after forgetting all about it ;) thanks, Jonathan > > > > Best Regards, > Ramona > > ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2024-10-10 16:40 UTC | newest] Thread overview: 24+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-09-12 12:15 [PATCH v5 0/3] add support for ad777x family Ramona Alexandra Nechita 2024-09-12 12:15 ` [PATCH v5 1/3] dt-bindings: iio: adc: add a7779 doc Ramona Alexandra Nechita 2024-09-12 13:15 ` Rob Herring (Arm) 2024-09-14 16:38 ` Jonathan Cameron 2024-09-12 12:15 ` [PATCH v5 2/3] Documentation: ABI: added filter mode doc in sysfs-bus-iio Ramona Alexandra Nechita 2024-09-12 14:47 ` Andy Shevchenko 2024-09-13 14:06 ` Nechita, Ramona 2024-09-13 17:36 ` Andy Shevchenko 2024-09-14 16:42 ` Jonathan Cameron 2024-09-13 4:16 ` kernel test robot 2024-09-14 16:43 ` Jonathan Cameron 2024-09-12 12:15 ` [PATCH v5 3/3] drivers: iio: adc: add support for ad777x family Ramona Alexandra Nechita 2024-09-12 15:15 ` Andy Shevchenko 2024-09-14 16:46 ` Jonathan Cameron 2024-09-14 1:44 ` kernel test robot 2024-09-14 16:48 ` Jonathan Cameron 2024-09-14 17:06 ` Jonathan Cameron 2024-09-20 13:24 ` Nechita, Ramona 2024-09-23 12:51 ` David Lechner 2024-09-28 14:29 ` Jonathan Cameron 2024-09-26 13:41 ` Nechita, Ramona 2024-09-28 14:31 ` Jonathan Cameron 2024-10-10 14:35 ` Nechita, Ramona 2024-10-10 16:40 ` 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).