* [PATCH 1/3] dt-bindings: iio: adc: add support for AD5940
@ 2019-11-08 13:09 Song Qiang
2019-11-08 13:09 ` [PATCH 2/3] iio: adc: add driver " Song Qiang
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Song Qiang @ 2019-11-08 13:09 UTC (permalink / raw)
To: lars, Michael.Hennerich, stefan.popa, jic23, knaack.h, pmeerw,
lgirdwood, broonie, robh+dt, mark.rutland
Cc: linux-iio, devicetree, linux-kernel, Song Qiang
Add yaml devicetree description file and a header file for
helping configure positive and negtive input of AD5940.
Signed-off-by: Song Qiang <songqiang1304521@gmail.com>
---
.../bindings/iio/adc/adi,ad5940.yaml | 240 ++++++++++++++++++
include/dt-bindings/iio/adc/adi,ad5940.h | 52 ++++
2 files changed, 292 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml
create mode 100644 include/dt-bindings/iio/adc/adi,ad5940.h
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml
new file mode 100644
index 000000000000..f7f034fdd8ec
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml
@@ -0,0 +1,240 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright 2019 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/bindings/iio/adc/adi,ad5940.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD5940 Device Tree Bindings
+
+maintainers:
+ - Song Qiang <songqiang1304521@gmail.com>
+
+description: |
+ Analog Devices AD5940 High Precision, Impedance, and Electrochemical Front End.
+ https://www.analog.com/media/en/technical-documentation/data-sheets/AD5940.pdf
+
+properties:
+ compatible:
+ enum:
+ - adi,ad5940
+
+ reg:
+ maxItems: 1
+
+ vref-supply:
+ description:
+ The regulator to be used to supply the reference voltage.
+ maxItems: 1
+
+ adi,interrupt-io:
+ description:
+ Output GPIO index of interrupt controller of AD5940.
+ maxItems: 1
+ allOf:
+ - $ref: /schemas/types.yaml#/definitions/uint32
+ - enum: [0, 3, 6, 7]
+
+ '#address-cells':
+ const: 1
+
+ '#size-cells':
+ const: 0
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - adi,interrupt-io
+
+patternProperties:
+ # 'channel@0-255'
+ "^channel@([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])$":
+ type: object
+ description: |
+ Represents the external channels which are connected to the ADC.
+ See Documentation/devicetree/bindings/iio/adc/adc.txt.
+ properties:
+ reg:
+ description:
+ Index of this channel, must be starting from 0.
+ maxItems: 1
+
+ diff-channels:
+ description:
+ Positive input and negtive input of the ADC buffer of this channel.
+ Input candidates are defined in include/dt-bindings/iio/adc/adi,ad5940.h.
+ minItems: 2
+ maxItems: 2
+ items:
+ - description: Positive input channel
+ - enum:
+ - AD5940_ADC_INPUTP_EXCITATION
+ - AD5940_ADC_INPUTP_FLOATING
+ - AD5940_ADC_INPUTP_HSTIA
+ - AD5940_ADC_INPUTP_LPTIA_LP
+ - AD5940_ADC_INPUTP_AIN0
+ - AD5940_ADC_INPUTP_AIN1
+ - AD5940_ADC_INPUTP_AIN2
+ - AD5940_ADC_INPUTP_AIN3
+ - AD5940_ADC_INPUTP_AVDD_2
+ - AD5940_ADC_INPUTP_DVDD_2
+ - AD5940_ADC_INPUTP_AVDD_REG_2
+ - AD5940_ADC_INPUTP_TEMP
+ - AD5940_ADC_INPUTP_VBIAS_CAP
+ - AD5940_ADC_INPUTP_DE0
+ - AD5940_ADC_INPUTP_SE0
+ - AD5940_ADC_INPUTP_VREF_2V5_2
+ - AD5940_ADC_INPUTP_VREF_1V82
+ - AD5940_ADC_INPUTP_P_TEMP_N
+ - AD5940_ADC_INPUTP_AIN4
+ - AD5940_ADC_INPUTP_AIN6
+ - AD5940_ADC_INPUTP_VZERO
+ - AD5940_ADC_INPUTP_VBIAS0
+ - AD5940_ADC_INPUTP_VCE0
+ - AD5940_ADC_INPUTP_VRE0
+ - AD5940_ADC_INPUTP_VCE0_2
+ - AD5940_ADC_INPUTP_LPTIA
+ - AD5940_ADC_INPUTP_AGND_REF
+
+ - description: Negtive input channel
+ - enum:
+ # Negtive input candidates
+ - AD5940_ADC_INPUTN_FLOATING
+ - AD5940_ADC_INPUTN_HSTIA
+ - AD5940_ADC_INPUTN_LPTIA
+ - AD5940_ADC_INPUTN_AIN0
+ - AD5940_ADC_INPUTN_AIN1
+ - AD5940_ADC_INPUTN_AIN2
+ - AD5940_ADC_INPUTN_AIN3
+ - AD5940_ADC_INPUTN_VBIAS_CA8
+ - AD5940_ADC_INPUTN_TEMP_N
+ - AD5940_ADC_INPUTN_AIN4
+ - AD5940_ADC_INPUTN_AIN6
+ - AD5940_ADC_INPUTN_VZERO
+ - AD5940_ADC_INPUTN_VBIAS0
+ - AD5940_ADC_INPUTN_EXCITATION
+
+ channel-name:
+ description:
+ Any string format name you would like to assign to this channel.
+ maxItems: 1
+
+ required:
+ - reg
+ - diff-channels
+ - channel-name
+
+examples:
+ - |
+ ad5940: ad5940@0 {
+ compatible = "adi,ad5940";
+ reg = <0>;
+ spi-max-frequency = <16000000>;
+ vref-supply = <&adc_vref>;
+ interrupt-parent = <&gpio>;
+ interrupts = <24 2>;
+
+ adi,interrupt-io = <0>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ channel@0 {
+ reg = <0>;
+ diff-channels = <AD5940_ADC_INPUTP_VCE0
+ AD5940_ADC_INPUTN_VBIAS0>;
+ channel-name = "Vce-Vbias";
+ };
+
+ channel@1 {
+ reg = <1>;
+ diff-channels = <AD5940_ADC_INPUTP_VRE0
+ AD5940_ADC_INPUTN_VBIAS0>;
+ channel-name = "Vre-Vbias";
+ };
+
+ channel@2 {
+ reg = <2>;
+ diff-channels = <AD5940_ADC_INPUTP_SE0
+ AD5940_ADC_INPUTN_VBIAS0>;
+ channel-name = "Vse-Vbias";
+ };
+
+ channel@3 {
+ reg = <3>;
+ diff-channels = <AD5940_ADC_INPUTP_DE0
+ AD5940_ADC_INPUTN_VBIAS0>;
+ channel-name = "Vde-Vbias";
+ };
+
+ channel@4 {
+ reg = <4>;
+ diff-channels = <AD5940_ADC_INPUTP_AIN0
+ AD5940_ADC_INPUTN_VBIAS0>;
+ channel-name = "ain0-Vbias";
+ };
+
+ channel@5 {
+ reg = <5>;
+ diff-channels = <AD5940_ADC_INPUTP_AIN1
+ AD5940_ADC_INPUTN_VBIAS0>;
+ channel-name = "ain1-Vbias";
+ };
+
+ channel@6 {
+ reg = <6>;
+ diff-channels = <AD5940_ADC_INPUTP_AIN2
+ AD5940_ADC_INPUTN_VBIAS0>;
+ channel-name = "ain2-Vbias";
+ };
+
+ channel@7 {
+ reg = <7>;
+ diff-channels = <AD5940_ADC_INPUTP_AIN3
+ AD5940_ADC_INPUTN_VBIAS0>;
+ channel-name = "ain3-Vbias";
+ };
+
+ channel@8 {
+ reg = <8>;
+ diff-channels = <AD5940_ADC_INPUTP_AIN4
+ AD5940_ADC_INPUTN_VBIAS0>;
+ channel-name = "ain4-Vbias";
+ };
+
+ channel@9 {
+ reg = <9>;
+ diff-channels = <AD5940_ADC_INPUTP_AIN6
+ AD5940_ADC_INPUTN_VBIAS0>;
+ channel-name = "ain6-Vbias";
+ };
+
+ channel@10 {
+ reg = <10>;
+ diff-channels = <AD5940_ADC_INPUTP_LPTIA_LP
+ AD5940_ADC_INPUTN_LPTIA>;
+ channel-name = "Low power TIA DC";
+ };
+
+ channel@11 {
+ reg = <11>;
+ diff-channels = <AD5940_ADC_INPUTP_LPTIA
+ AD5940_ADC_INPUTN_LPTIA>;
+ channel-name = "Low power TIA AC";
+ };
+
+ channel@12 {
+ reg = <12>;
+ diff-channels = <AD5940_ADC_INPUTP_HSTIA
+ AD5940_ADC_INPUTN_HSTIA>;
+ channel-name = "High Speed TIA";
+ };
+
+ channel@13 {
+ reg = <13>;
+ diff-channels = <AD5940_ADC_INPUTP_TEMP
+ AD5940_ADC_INPUTN_VBIAS0>;
+ channel-name = "Temperature";
+ };
+ };
diff --git a/include/dt-bindings/iio/adc/adi,ad5940.h b/include/dt-bindings/iio/adc/adi,ad5940.h
new file mode 100644
index 000000000000..c17826f2f654
--- /dev/null
+++ b/include/dt-bindings/iio/adc/adi,ad5940.h
@@ -0,0 +1,52 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * This header provides constants for configuring the AD5940 AFE
+ */
+
+#ifndef _DT_BINDINGS_IIO_ADC_AD5940_H
+#define _DT_BINDINGS_IIO_ADC_AD5940_H
+
+#define AD5940_ADC_INPUTN_FLOATING 0
+#define AD5940_ADC_INPUTN_HSTIA 1
+#define AD5940_ADC_INPUTN_LPTIA 2
+#define AD5940_ADC_INPUTN_AIN0 4
+#define AD5940_ADC_INPUTN_AIN1 5
+#define AD5940_ADC_INPUTN_AIN2 6
+#define AD5940_ADC_INPUTN_AIN3 7
+#define AD5940_ADC_INPUTN_VBIAS_CA8 10
+#define AD5940_ADC_INPUTN_TEMP_N 11
+#define AD5940_ADC_INPUTN_AIN4 12
+#define AD5940_ADC_INPUTN_AIN6 14
+#define AD5940_ADC_INPUTN_VZERO 16
+#define AD5940_ADC_INPUTN_VBIAS0 17
+#define AD5940_ADC_INPUTN_EXCITATION 20
+
+#define AD5940_ADC_INPUTP_FLOATING 0
+#define AD5940_ADC_INPUTP_HSTIA 1
+#define AD5940_ADC_INPUTP_LPTIA_LP 2
+#define AD5940_ADC_INPUTP_AIN0 4
+#define AD5940_ADC_INPUTP_AIN1 5
+#define AD5940_ADC_INPUTP_AIN2 6
+#define AD5940_ADC_INPUTP_AIN3 7
+#define AD5940_ADC_INPUTP_AVDD_2 8
+#define AD5940_ADC_INPUTP_DVDD_2 9
+#define AD5940_ADC_INPUTP_AVDD_REG_2 10
+#define AD5940_ADC_INPUTP_TEMP 11
+#define AD5940_ADC_INPUTP_VBIAS_CAP 12
+#define AD5940_ADC_INPUTP_DE0 13
+#define AD5940_ADC_INPUTP_SE0 14
+#define AD5940_ADC_INPUTP_VREF_2V5_2 16
+#define AD5940_ADC_INPUTP_VREF_1V82 18
+#define AD5940_ADC_INPUTP_P_TEMP_N 19
+#define AD5940_ADC_INPUTP_AIN4 20
+#define AD5940_ADC_INPUTP_AIN6 22
+#define AD5940_ADC_INPUTP_VZERO 23
+#define AD5940_ADC_INPUTP_VBIAS0 24
+#define AD5940_ADC_INPUTP_VCE0 25
+#define AD5940_ADC_INPUTP_VRE0 26
+#define AD5940_ADC_INPUTP_VCE0_2 31
+#define AD5940_ADC_INPUTP_LPTIA 33
+#define AD5940_ADC_INPUTP_AGND_REF 35
+#define AD5940_ADC_INPUTP_EXCITATION 36
+
+#endif /* _DT_BINDINGS_IIO_ADC_AD5940 */
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/3] iio: adc: add driver support for AD5940
2019-11-08 13:09 [PATCH 1/3] dt-bindings: iio: adc: add support for AD5940 Song Qiang
@ 2019-11-08 13:09 ` Song Qiang
2019-11-10 16:55 ` Jonathan Cameron
2019-11-08 13:09 ` [PATCH 3/3] MAINTAINERS: add maintainer " Song Qiang
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Song Qiang @ 2019-11-08 13:09 UTC (permalink / raw)
To: lars, Michael.Hennerich, stefan.popa, jic23, knaack.h, pmeerw,
lgirdwood, broonie, robh+dt, mark.rutland
Cc: linux-iio, devicetree, linux-kernel, Song Qiang
The AD5940 is a high precision, low power analog front end (AFE)
designed for portable applications that require high precision,
electrochemical-based measurement techniques, such as amper-
ometric, voltammetric, or impedance measurements.
Datasheet:
Link: https://www.analog.com/media/en/technical-documentation/data-sheets/AD5940.pdf
Signed-off-by: Song Qiang <songqiang1304521@gmail.com>
---
.../ABI/testing/sysfs-bus-iio-adc-ad5940 | 7 +
drivers/iio/adc/Kconfig | 10 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ad5940.c | 679 ++++++++++++++++++
4 files changed, 697 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad5940
create mode 100644 drivers/iio/adc/ad5940.c
diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad5940 b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad5940
new file mode 100644
index 000000000000..839644ffbd9b
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad5940
@@ -0,0 +1,7 @@
+What: /sys/bus/iio/devices/iio:deviceX/events/in_voltageY-voltageZ_name
+Date: June 2019
+KernelVersion: 4.14.5
+Contact: songqiang1304521@gmail.com
+Description:
+ Channel name assigned in devicetree, used to tell userspace
+ users which channel this is.
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index f0af3a42f53c..345f934d3444 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -11,6 +11,16 @@ config AD_SIGMA_DELTA
select IIO_BUFFER
select IIO_TRIGGERED_BUFFER
+config AD5940
+ tristate "Analog Devices AD5940 ADC Driver"
+ depends on SPI
+ help
+ Say yes here to build support for the ADC part of Analog Devices
+ analog front end AD5940.
+
+ To compile this driver as a module, choose M here: the
+ module will be called ad5940.
+
config AD7124
tristate "Analog Devices AD7124 and similar sigma-delta ADCs driver"
depends on SPI_MASTER
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index ef9cc485fb67..ac52d1b6f3fb 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -5,6 +5,7 @@
# When adding new entries keep the list in alphabetical order
obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
+obj-$(CONFIG_AD5940) += ad5940.o
obj-$(CONFIG_AD7124) += ad7124.o
obj-$(CONFIG_AD7266) += ad7266.o
obj-$(CONFIG_AD7291) += ad7291.o
diff --git a/drivers/iio/adc/ad5940.c b/drivers/iio/adc/ad5940.c
new file mode 100644
index 000000000000..57f737edca36
--- /dev/null
+++ b/drivers/iio/adc/ad5940.c
@@ -0,0 +1,679 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AD5940 SPI ADC driver
+ *
+ * Copyright (C) 2019 Song Qiang <songqiang1304521@gmail.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bsearch.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of_irq.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#include <linux/iio/iio.h>
+
+#define AD5940_CHANNEL_AINP_MSK GENMASK(5, 0)
+#define AD5940_CHANNEL_AINP(x) FIELD_PREP(AD5940_CHANNEL_AINP_MSK, x)
+#define AD5940_CHANNEL_AINN_MSK GENMASK(12, 8)
+#define AD5940_CHANNEL_AINN(x) FIELD_PREP(AD5940_CHANNEL_AINN_MSK, x)
+
+#define AD5940_CHANNEL_NAME 0
+
+#define AD5940_SPICMD_SETADDR 0x20
+#define AD5940_SPICMD_READREG 0x6d
+#define AD5940_SPICMD_WRITEREG 0x2d
+#define AD5940_SPICMD_READFIFO 0x5f
+
+#define AD5940_REG_AFECON 0x00002000
+#define AD5940_AFECON_ADCCONV_MSK BIT(8)
+#define AD5940_AFECON_ADCCONV_EN FIELD_PREP(AD5940_AFECON_ADCCONV_MSK, 1)
+#define AD5940_AFECON_ADCCONV_DIS FIELD_PREP(AD5940_AFECON_ADCCONV_MSK, 0)
+#define AD5940_AFECON_ADCEN_MSK BIT(7)
+#define AD5940_AFECON_ADCEN FIELD_PREP(AD5940_AFECON_ADCEN_MSK, 1)
+
+#define AD5940_REG_ADCDAT 0x00002074
+#define AD5940_REG_DFTREAL 0x00002078
+#define AD5940_REG_DFTIMAG 0x0000207C
+#define AD5940_REG_SINC2DAT 0x00002080
+#define AD5940_REG_TEMPSENSDAT 0x00002084
+#define AD5940_REG_STATSMEAN 0x000021C8
+#define AD5940_REG_STATSVAR 0x000021C0
+
+#define AD5940_REG_INTCPOL 0x00003000 /* Interrupt Polarity. */
+#define AD5940_INTCPOL_MSK BIT(0)
+#define AD5940_INTCPOL_POS FIELD_PREP(AD5940_INTCPOL_MSK, 1)
+#define AD5940_INTCPOL_NEG FIELD_PREP(AD5940_INTCPOL_MSK, 0)
+#define AD5940_REG_INTCLR 0x00003004
+
+#define AD5940_REG_PMBW 0x000022F0
+#define AD5940_PMBW_SYSHS_MSK BIT(0)
+#define AD5940_PMBW_HP FIELD_PREP(AD5940_PMBW_SYSHS_MSK, 1)
+#define AD5940_PMBW_LP FIELD_PREP(AD5940_PMBW_SYSHS_MSK, 0)
+#define AD5940_PMBW_SYSBW_MSK GENMASK(3, 2)
+#define AD5940_PMBW_BWNA FIELD_PREP(AD5940_PMBW_SYSBW_MSK, 0)
+#define AD5940_PMBW_BW50 FIELD_PREP(AD5940_PMBW_SYSBW_MSK, 1)
+#define AD5940_PMBW_BW100 FIELD_PREP(AD5940_PMBW_SYSBW_MSK, 2)
+#define AD5940_PMBW_BW250 FIELD_PREP(AD5940_PMBW_SYSBW_MSK, 3)
+
+#define AD5940_REG_ADCCON 0x000021A8
+#define AD5940_ADCCON_PGA_MSK GENMASK(18, 16)
+#define AD5940_ADCCON_PGA_1 FIELD_PREP(AD5940_ADCCON_PGA_MSK, 0)
+#define AD5940_ADCCON_PGA_1P5 FIELD_PREP(AD5940_ADCCON_PGA_MSK, 1)
+#define AD5940_ADCCON_PGA_2 FIELD_PREP(AD5940_ADCCON_PGA_MSK, 2)
+#define AD5940_ADCCON_PGA_4 FIELD_PREP(AD5940_ADCCON_PGA_MSK, 3)
+#define AD5940_ADCCON_PGA_9 FIELD_PREP(AD5940_ADCCON_PGA_MSK, 4)
+#define AD5940_ADCCON_MUX_MSK (GENMASK(12, 8) | GENMASK(5, 0))
+
+#define AD5940_REG_INTCLR 0x00003004
+#define AD5940_REG_INTCSEL0 0x00003008
+#define AD5940_INTC_BRK BIT(31)
+#define AD5940_INTC_OUTLIER BIT(29)
+#define AD5940_INTC_FIFO_UNDERFLOW BIT(27)
+#define AD5940_INTC_FIFO_OVERFLOW BIT(26)
+#define AD5940_INTC_FIFO_THRESHOLD BIT(25)
+#define AD5940_INTC_FIFO_EMPTY BIT(24)
+#define AD5940_INTC_FIFO_FULL BIT(23)
+#define AD5940_INTC_SEQ_TO_ERR BIT(17)
+#define AD5940_INTC_SEQ_TO_FINISH BIT(16)
+#define AD5940_INTC_SEQ_END BIT(15)
+#define AD5940_INTC_BL_DONE BIT(13)
+#define AD5940_INTC_IRQ_3 BIT(12)
+#define AD5940_INTC_IRQ_2 BIT(11)
+#define AD5940_INTC_IRQ_1 BIT(10)
+#define AD5940_INTC_IRQ_0 BIT(9)
+#define AD5940_INTC_VAR BIT(8)
+#define AD5940_INTC_MEAN BIT(7)
+#define AD5940_INTC_ADC_DELTA_FAIL BIT(6)
+#define AD5940_INTC_ADC_MAX_FAIL BIT(5)
+#define AD5940_INTC_ADC_MIN_FAIL BIT(4)
+#define AD5940_INTC_TEMP BIT(3)
+#define AD5940_INTC_SINC2 BIT(2)
+#define AD5940_INTC_DFT BIT(1)
+#define AD5940_INTC_ADC BIT(0)
+
+#define AD5940_REG_GP0CON 0x00000000
+#define AD5940_GP0CON_3_MSK GENMASK(7, 6)
+#define AD5940_GP0CON_3_INT FIELD_PREP(AD5940_GP0CON_3_MSK, 3)
+#define AD5940_GP0CON_6_MSK GENMASK(13, 12)
+#define AD5940_GP0CON_6_INT FIELD_PREP(AD5940_GP0CON_6_MSK, 3)
+
+#define AD5940_REG_GP0OEN 0x00000004
+
+#define AD5940_AFECON_CHIPID 0x00000404
+#define AD5940_CHIPID 0x5502
+
+struct ad5940_channel_config {
+ u32 ain;
+ const char *channel_name;
+};
+
+struct ad5940_state {
+ struct spi_device *spi;
+ /* This mutex is for protecting the SPI command sequences. */
+ struct mutex lock;
+
+ struct completion complete;
+ u32 conversion_time;
+
+ u8 n_input;
+ u8 p_input;
+
+ struct regulator *vref;
+ int vref_mv;
+
+ int num_channels;
+ struct ad5940_channel_config *channel_config;
+ union {
+ __be32 d32;
+ u8 d8[5];
+ } data ____cacheline_aligned;
+};
+
+static int ad5940_set_addr(struct ad5940_state *st, u16 addr)
+{
+ st->data.d8[0] = AD5940_SPICMD_SETADDR;
+ st->data.d8[1] = (addr >> 8) & 0xff;
+ st->data.d8[2] = addr & 0xff;
+
+ return spi_write(st->spi, st->data.d8, 3);
+}
+
+static int ad5940_read_reg(struct ad5940_state *st, u16 addr, u32 *data)
+{
+ int rx_len;
+ u8 shift;
+ int ret;
+
+ ret = ad5940_set_addr(st, addr);
+ if (ret < 0)
+ return ret;
+
+ if ((addr >= 0x1000) && (addr <= 0x3014))
+ rx_len = 4;
+ else
+ rx_len = 2;
+
+ st->data.d8[0] = AD5940_SPICMD_READREG;
+ st->data.d8[1] = 0;
+ shift = 32 - (8 * rx_len);
+
+ ret = spi_write_then_read(st->spi, st->data.d8, 2, &st->data.d32,
+ rx_len);
+ if (ret < 0)
+ return ret;
+
+ *data = be32_to_cpu(st->data.d32) >> shift;
+
+ return 0;
+}
+
+static int ad5940_write_reg(struct ad5940_state *st, u16 addr, u32 data)
+{
+ int tx_len;
+ int ret;
+
+ ret = ad5940_set_addr(st, addr);
+ if (ret < 0)
+ return ret;
+
+ st->data.d8[0] = AD5940_SPICMD_WRITEREG;
+ if ((addr >= 0x1000) && (addr <= 0x3014)) {
+ st->data.d8[1] = (data >> 24) & 0xff;
+ st->data.d8[2] = (data >> 16) & 0xff;
+ st->data.d8[3] = (data >> 8) & 0xff;
+ st->data.d8[4] = data & 0xff;
+ tx_len = 5;
+ } else {
+ st->data.d8[1] = (data >> 8) & 0xff;
+ st->data.d8[2] = data & 0xff;
+ tx_len = 3;
+ }
+
+ return spi_write(st->spi, st->data.d8, tx_len);
+}
+
+static int ad5940_write_reg_mask(struct ad5940_state *st, u16 addr,
+ u32 mask, u32 data)
+{
+ u32 temp;
+ int ret;
+
+ ret = ad5940_read_reg(st, addr, &temp);
+ if (ret < 0)
+ return ret;
+
+ temp &= ~mask;
+ temp |= data;
+
+ return ad5940_write_reg(st, addr, temp);
+}
+
+static ssize_t ad5940_read_info(struct iio_dev *indio_dev,
+ uintptr_t private,
+ const struct iio_chan_spec *chan,
+ char *buf)
+{
+ struct ad5940_state *st = iio_priv(indio_dev);
+
+ switch ((u32)private) {
+ case AD5940_CHANNEL_NAME:
+ return sprintf(buf, "%s\n",
+ st->channel_config[chan->address].channel_name);
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct iio_chan_spec_ext_info ad4590_ext_info[] = {
+ {
+ .name = "name",
+ .read = ad5940_read_info,
+ .private = AD5940_CHANNEL_NAME,
+ .shared = IIO_SEPARATE,
+ },
+ { },
+};
+
+static const struct iio_chan_spec ad5940_channel_template = {
+ .type = IIO_VOLTAGE,
+ .differential = 1,
+ .indexed = 1,
+ .ext_info = ad4590_ext_info,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+};
+
+static int ad5940_clear_ready(struct ad5940_state *st)
+{
+ int ret;
+
+ ret = ad5940_write_reg_mask(st, AD5940_REG_AFECON,
+ AD5940_AFECON_ADCCONV_MSK,
+ AD5940_AFECON_ADCCONV_DIS);
+ if (ret < 0)
+ return ret;
+
+ return ad5940_write_reg(st, AD5940_REG_INTCLR, AD5940_INTC_ADC);
+}
+
+static irqreturn_t ad5940_irq_handler(int irq, void *private)
+{
+ struct ad5940_state *st = private;
+ int ret;
+
+ ret = ad5940_clear_ready(st);
+ if (ret < 0)
+ return ret;
+
+ complete(&st->complete);
+
+ return IRQ_HANDLED;
+}
+
+static int ad5940_scan_direct(struct ad5940_state *st, u32 mux, int *val)
+{
+ int ret;
+ u32 result;
+
+ mutex_lock(&st->lock);
+ ret = ad5940_write_reg_mask(st, AD5940_REG_ADCCON,
+ AD5940_ADCCON_MUX_MSK, mux);
+ if (ret < 0)
+ goto unlock_return;
+
+ reinit_completion(&st->complete);
+ ret = ad5940_write_reg_mask(st, AD5940_REG_AFECON,
+ AD5940_AFECON_ADCCONV_MSK,
+ AD5940_AFECON_ADCCONV_EN);
+ if (ret < 0)
+ goto unlock_return;
+
+ ret = wait_for_completion_timeout(&st->complete,
+ msecs_to_jiffies(1000));
+ if (!ret) {
+ ad5940_clear_ready(st);
+ ret = -ETIMEDOUT;
+ goto unlock_return;
+ }
+
+ ret = ad5940_read_reg(st, AD5940_REG_ADCDAT, &result);
+ mutex_unlock(&st->lock);
+ if (ret < 0)
+ return ret;
+ *val = result & 0xffff;
+
+ return 0;
+
+unlock_return:
+ mutex_unlock(&st->lock);
+ return ret;
+}
+
+static int ad5940_read_raw(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan, int *val, int *val2, long info)
+{
+ struct ad5940_state *st = iio_priv(indio_dev);
+ int ret;
+
+ switch (info) {
+ case IIO_CHAN_INFO_RAW:
+ ret = ad5940_scan_direct(st,
+ st->channel_config[chan->address].ain,
+ val);
+ if (ret < 0)
+ return ret;
+ else
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_SCALE:
+ *val = st->vref_mv;
+ *val2 = 16;
+ return IIO_VAL_FRACTIONAL_LOG2;
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct iio_info ad5940_info = {
+ .read_raw = &ad5940_read_raw,
+};
+
+int cmp_u8(const void *a, const void *b)
+{
+ return (*(u8 *)a - *(u8 *)b);
+}
+
+static int ad5940_check_channel_indexes(struct device *dev, u32 *ain)
+{
+ const u8 channel_p[] = {
+ 0, 1, 2, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 16, 18, 19,
+ 20, 22, 23, 24, 25, 26, 31, 33, 35, 36
+ };
+ const u8 channel_n[] = {
+ 0, 1, 2, 4, 5, 6, 7, 10, 11, 12, 14, 16, 17, 20
+ };
+ u8 *index;
+
+ index = (u8 *) bsearch(&ain[0], channel_p, ARRAY_SIZE(channel_p),
+ sizeof(u8), cmp_u8);
+ if (!index) {
+ dev_err(dev, "Positive input index not found.\n");
+ return -EINVAL;
+ }
+
+ index = (u8 *) bsearch(&ain[1], channel_n, ARRAY_SIZE(channel_n),
+ sizeof(u8), cmp_u8);
+ if (!index) {
+ dev_err(dev, "negtive input index not found.\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int ad5940_of_parse_channel_config(struct iio_dev *indio_dev,
+ struct device_node *np)
+{
+ struct ad5940_state *st = iio_priv(indio_dev);
+ struct iio_chan_spec *chan;
+ struct device_node *child;
+ u32 channel, ain[2];
+ int ret;
+
+ st->num_channels = of_get_available_child_count(np);
+ if (!st->num_channels) {
+ dev_err(indio_dev->dev.parent, "no channel children\n");
+ return -ENODEV;
+ }
+
+ chan = devm_kcalloc(indio_dev->dev.parent, st->num_channels,
+ sizeof(*chan), GFP_KERNEL);
+ if (!chan)
+ return -ENOMEM;
+
+ st->channel_config = devm_kcalloc(indio_dev->dev.parent,
+ st->num_channels,
+ sizeof(*st->channel_config),
+ GFP_KERNEL);
+ if (!st->channel_config)
+ return -ENOMEM;
+
+ indio_dev->channels = chan;
+ indio_dev->num_channels = st->num_channels;
+
+ for_each_available_child_of_node(np, child) {
+ ret = of_property_read_u32(child, "reg", &channel);
+ if (ret)
+ goto err;
+
+ ret = of_property_read_u32_array(child, "diff-channels",
+ ain, 2);
+ if (ret)
+ goto err;
+
+ ret = of_property_read_string(child, "channel-name",
+ &st->channel_config[channel].channel_name);
+ if (ret)
+ st->channel_config[channel].channel_name = "none-name";
+
+ ret = ad5940_check_channel_indexes(indio_dev->dev.parent, ain);
+ if (ret) {
+ dev_err(indio_dev->dev.parent,
+ "some input channel index does not exist: %d, %d, %d",
+ channel, ain[0], ain[1]);
+ goto err;
+ }
+
+ st->channel_config[channel].ain = AD5940_CHANNEL_AINP(ain[0]) |
+ AD5940_CHANNEL_AINN(ain[1]);
+
+ *chan = ad5940_channel_template;
+ chan->address = channel;
+ chan->scan_index = channel;
+ chan->channel = ain[0];
+ chan->channel2 = ain[1];
+
+ chan++;
+ }
+
+ return 0;
+err:
+ of_node_put(child);
+
+ return ret;
+}
+
+static int ad5940_config_polarity(struct ad5940_state *st, u32 polarity)
+{
+ u32 val;
+
+ if (polarity == IRQF_TRIGGER_RISING)
+ val = AD5940_INTCPOL_POS;
+ else
+ val = AD5940_INTCPOL_NEG;
+
+ return ad5940_write_reg_mask(st, AD5940_REG_INTCPOL,
+ AD5940_INTCPOL_MSK, val);
+}
+
+static int ad5940_config_int_io(struct ad5940_state *st, u8 int_io)
+{
+ int ret = 0;
+
+ if (int_io == 3)
+ ret = ad5940_write_reg_mask(st, AD5940_REG_GP0CON,
+ AD5940_GP0CON_3_MSK,
+ AD5940_GP0CON_3_INT);
+ else if (int_io == 6)
+ ret = ad5940_write_reg_mask(st, AD5940_REG_GP0CON,
+ AD5940_GP0CON_6_MSK,
+ AD5940_GP0CON_6_INT);
+ if (ret < 0)
+ return ret;
+
+ return ad5940_write_reg(st, AD5940_REG_GP0OEN, BIT(int_io));
+}
+
+static const u32 ad5940_powerup_setting[][2] = {
+ { 0x0908, 0x02c9 },
+ { 0x0c08, 0x206c },
+ { 0x21f0, 0x0010 },
+ { 0x0410, 0x02c9 },
+ { 0x0a28, 0x0009 },
+ { 0x238c, 0x0104 },
+ { 0x0a04, 0x4859 },
+ { 0x0a04, 0xf27b },
+ { 0x0a00, 0x8009 },
+ { 0x22f0, 0x0000 },
+ { 0x2230, 0xde87a5af },
+ { 0x2250, 0x103f },
+ { 0x22b0, 0x203c },
+ { 0x2230, 0xde87a5a0 },
+};
+
+static int ad5940_setup(struct ad5940_state *st, u8 int_io)
+{
+ u32 chip_id;
+ int ret;
+ u8 i;
+
+ for (i = 0; i < ARRAY_SIZE(ad5940_powerup_setting); i++) {
+ ret = ad5940_write_reg(st, ad5940_powerup_setting[i][0],
+ ad5940_powerup_setting[i][1]);
+ if (ret < 0)
+ return ret;
+ }
+
+ ret = ad5940_read_reg(st, AD5940_AFECON_CHIPID, &chip_id);
+ if (ret < 0)
+ return ret;
+ if (chip_id != AD5940_CHIPID) {
+ dev_err(&st->spi->dev, "Wrong chip ID with 0x%x.", chip_id);
+ return -ENXIO;
+ }
+ dev_info(&st->spi->dev, "Found ad5940");
+
+ ret = ad5940_write_reg(st, AD5940_REG_PMBW,
+ AD5940_PMBW_LP | AD5940_PMBW_BW250);
+ if (ret < 0)
+ return ret;
+
+ ret = ad5940_config_int_io(st, int_io);
+ if (ret < 0)
+ return ret;
+
+ ret = ad5940_clear_ready(st);
+ if (ret < 0)
+ return ret;
+
+ ret = ad5940_write_reg(st, AD5940_REG_INTCSEL0, AD5940_INTC_ADC);
+ if (ret < 0)
+ return ret;
+
+ ret = ad5940_write_reg_mask(st, AD5940_REG_ADCCON,
+ AD5940_ADCCON_PGA_MSK,
+ AD5940_ADCCON_PGA_1);
+ if (ret < 0)
+ return ret;
+
+ return ad5940_write_reg_mask(st, AD5940_REG_AFECON,
+ AD5940_AFECON_ADCEN_MSK,
+ AD5940_AFECON_ADCEN);
+}
+
+static void ad5940_regulator_disable(void *data)
+{
+ struct ad5940_state *st = data;
+
+ regulator_disable(st->vref);
+}
+
+static inline int ad5940_check_int_io(u32 io, struct device *dev)
+{
+ static const u8 int_io[3] = {0, 3, 6};
+ u8 *p;
+
+ p = bsearch(&io, int_io, 3, sizeof(u8), cmp_u8);
+ if (!p) {
+ dev_err(dev, "interrupt output pin not valid,");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int ad5940_probe(struct spi_device *spi)
+{
+ struct iio_dev *indio_dev;
+ struct ad5940_state *st;
+ u32 trig_type;
+ int vref_uv = 0;
+ u32 int_io;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ st = iio_priv(indio_dev);
+
+ ret = device_property_read_u32(&spi->dev, "adi,interrupt-io", &int_io);
+ if (ret) {
+ dev_err(&spi->dev,
+ "reading dt property 'adi,interrupt-io' failed.");
+ return -EINVAL;
+ }
+
+ st->spi = spi;
+
+ trig_type = irq_get_trigger_type(spi->irq);
+ if (trig_type != IRQF_TRIGGER_RISING &&
+ trig_type != IRQF_TRIGGER_FALLING) {
+ dev_err(&spi->dev, "trigger type must be rising or falling.");
+ return -EINVAL;
+ }
+ ret = ad5940_config_polarity(st, trig_type);
+ if (ret < 0) {
+ dev_err(&spi->dev, "config polarity failed.");
+ return ret;
+ }
+
+ ret = ad5940_check_int_io(int_io, &spi->dev);
+ if (ret < 0)
+ return ret;
+
+ st->vref = devm_regulator_get_optional(&spi->dev, "vref");
+ if (!IS_ERR(st->vref)) {
+ ret = regulator_enable(st->vref);
+ if (ret) {
+ dev_err(&spi->dev, "Failed to enbale specified vref supply.\n");
+ return ret;
+ }
+
+ ret = devm_add_action_or_reset(&spi->dev,
+ ad5940_regulator_disable, st);
+ if (ret) {
+ regulator_disable(st->vref);
+ return ret;
+ }
+
+ ret = regulator_get_voltage(st->vref);
+ if (ret < 0)
+ return ret;
+
+ vref_uv = ret;
+ }
+
+ if (vref_uv)
+ st->vref_mv = vref_uv / 1000;
+ else
+ st->vref_mv = 1820;
+
+ indio_dev->dev.parent = &spi->dev;
+ indio_dev->dev.of_node = spi->dev.of_node;
+ indio_dev->name = spi->dev.of_node->name;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->info = &ad5940_info;
+
+ ret = ad5940_of_parse_channel_config(indio_dev, spi->dev.of_node);
+ if (ret < 0)
+ return ret;
+
+ init_completion(&st->complete);
+
+ ret = ad5940_setup(st, int_io);
+ if (ret)
+ return ret;
+
+ ret = devm_request_threaded_irq(&spi->dev, spi->irq, NULL,
+ ad5940_irq_handler,
+ trig_type | IRQF_ONESHOT,
+ dev_name(&spi->dev), st);
+ if (ret < 0)
+ return ret;
+
+ return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct of_device_id ad5940_dt_match[] = {
+ { .compatible = "adi,ad5940" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, ad5940_spi_ids);
+
+static struct spi_driver ad5940_driver = {
+ .driver = {
+ .name = "ad5940",
+ .of_match_table = ad5940_dt_match,
+ },
+ .probe = ad5940_probe,
+};
+module_spi_driver(ad5940_driver);
+
+MODULE_AUTHOR("Song Qiang <songqiang1304521@gmail.com>");
+MODULE_DESCRIPTION("Analog Device AD5940 ADC driver");
+MODULE_LICENSE("GPL");
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/3] MAINTAINERS: add maintainer for AD5940
2019-11-08 13:09 [PATCH 1/3] dt-bindings: iio: adc: add support for AD5940 Song Qiang
2019-11-08 13:09 ` [PATCH 2/3] iio: adc: add driver " Song Qiang
@ 2019-11-08 13:09 ` Song Qiang
2019-11-10 16:26 ` [PATCH 1/3] dt-bindings: iio: adc: add support " Jonathan Cameron
2019-11-14 1:28 ` Rob Herring
3 siblings, 0 replies; 13+ messages in thread
From: Song Qiang @ 2019-11-08 13:09 UTC (permalink / raw)
To: lars, Michael.Hennerich, stefan.popa, jic23, knaack.h, pmeerw,
lgirdwood, broonie, robh+dt, mark.rutland
Cc: linux-iio, devicetree, linux-kernel, Song Qiang
Add myself as the maintainer of the driver of AD5940 and it's
docs.
Signed-off-by: Song Qiang <songqiang1304521@gmail.com>
---
MAINTAINERS | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 2a427d1e9f01..c3d29dcc50ad 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -431,6 +431,16 @@ W: http://ez.analog.com/community/linux-device-drivers
S: Supported
F: drivers/regulator/ad5398.c
+AD5940 ANALOG FRONT END DRIVER
+M: Song Qiang <songqiang1304521@gmail.com>
+W: https://wiki.analog.com/resources/eval/user-guides/ad5940
+W: http://ez.analog.com/community/linux-device-drivers
+S: Supported
+F: drivers/iio/adc/ad5940.c
+F: Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml
+F: arch/arm/boot/dts/overlays/rpi-ad5940-overlay.dts
+F: Documentation/ABI/testing/sysfs-bus-iio-adc-ad594
+
AD714X CAPACITANCE TOUCH SENSOR DRIVER (AD7142/3/7/8/7A)
M: Michael Hennerich <michael.hennerich@analog.com>
W: http://wiki.analog.com/AD7142
--
2.17.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] dt-bindings: iio: adc: add support for AD5940
2019-11-08 13:09 [PATCH 1/3] dt-bindings: iio: adc: add support for AD5940 Song Qiang
2019-11-08 13:09 ` [PATCH 2/3] iio: adc: add driver " Song Qiang
2019-11-08 13:09 ` [PATCH 3/3] MAINTAINERS: add maintainer " Song Qiang
@ 2019-11-10 16:26 ` Jonathan Cameron
2019-11-14 13:32 ` Song Qiang
2019-11-14 1:28 ` Rob Herring
3 siblings, 1 reply; 13+ messages in thread
From: Jonathan Cameron @ 2019-11-10 16:26 UTC (permalink / raw)
To: Song Qiang
Cc: lars, Michael.Hennerich, stefan.popa, knaack.h, pmeerw,
lgirdwood, broonie, robh+dt, mark.rutland, linux-iio, devicetree,
linux-kernel
On Fri, 8 Nov 2019 21:09:44 +0800
Song Qiang <songqiang1304521@gmail.com> wrote:
> Add yaml devicetree description file and a header file for
> helping configure positive and negtive input of AD5940.
>
> Signed-off-by: Song Qiang <songqiang1304521@gmail.com>
Ouch. This is a very complex device, so I'm guessing this is the tip
of the iceberg when it comes to the eventual binding.
For reference of others this has a similarly complex DAC and
TIA + some excitation voltage generators (DDS).
Anyhow, a few comments inline but I'll definitely be looking for
a dt maintainer input on this one.
Thanks,
Jonathan
> ---
> .../bindings/iio/adc/adi,ad5940.yaml | 240 ++++++++++++++++++
> include/dt-bindings/iio/adc/adi,ad5940.h | 52 ++++
> 2 files changed, 292 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml
> create mode 100644 include/dt-bindings/iio/adc/adi,ad5940.h
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml
> new file mode 100644
> index 000000000000..f7f034fdd8ec
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml
> @@ -0,0 +1,240 @@
> +# SPDX-License-Identifier: GPL-2.0
For new bindings, preference is to include a dual license
(GPL-2.0-only OR BSD-2-Clause)
If Analog is fine doing this that would be great.
> +# Copyright 2019 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/bindings/iio/adc/adi,ad5940.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD5940 Device Tree Bindings
> +
> +maintainers:
> + - Song Qiang <songqiang1304521@gmail.com>
> +
> +description: |
> + Analog Devices AD5940 High Precision, Impedance, and Electrochemical Front End.
> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD5940.pdf
> +
> +properties:
> + compatible:
> + enum:
> + - adi,ad5940
> +
> + reg:
> + maxItems: 1
> +
> + vref-supply:
> + description:
> + The regulator to be used to supply the reference voltage.
> + maxItems: 1
It's worth taking a look at similar patch reviews to pick up on things
that are common issues. Rob has pointed out a few times recently that
vref-supply can only ever have one item, so no need for maxItems.
> +
> + adi,interrupt-io:
> + description:
> + Output GPIO index of interrupt controller of AD5940.
> + maxItems: 1
I'm fairly sure an enum can only have one entry so don't think this is
needed.
> + allOf:
> + - $ref: /schemas/types.yaml#/definitions/uint32
> + - enum: [0, 3, 6, 7]
> +
> + '#address-cells':
> + const: 1
> +
> + '#size-cells':
> + const: 0
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - adi,interrupt-io
> +
> +patternProperties:
> + # 'channel@0-255'
Really? That is a lot of channels. Superficially it looks like a much
smaller number of possibilities from the datasheet.
> + "^channel@([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])$":
> + type: object
> + description: |
> + Represents the external channels which are connected to the ADC.
> + See Documentation/devicetree/bindings/iio/adc/adc.txt.
> + properties:
> + reg:
> + description:
> + Index of this channel, must be starting from 0.
> + maxItems: 1
> +
> + diff-channels:
> + description:
> + Positive input and negtive input of the ADC buffer of this channel.
> + Input candidates are defined in include/dt-bindings/iio/adc/adi,ad5940.h.
> + minItems: 2
> + maxItems: 2
> + items:
> + - description: Positive input channel
> + - enum:
> + - AD5940_ADC_INPUTP_EXCITATION
> + - AD5940_ADC_INPUTP_FLOATING
> + - AD5940_ADC_INPUTP_HSTIA
> + - AD5940_ADC_INPUTP_LPTIA_LP
> + - AD5940_ADC_INPUTP_AIN0
> + - AD5940_ADC_INPUTP_AIN1
> + - AD5940_ADC_INPUTP_AIN2
> + - AD5940_ADC_INPUTP_AIN3
> + - AD5940_ADC_INPUTP_AVDD_2
> + - AD5940_ADC_INPUTP_DVDD_2
> + - AD5940_ADC_INPUTP_AVDD_REG_2
> + - AD5940_ADC_INPUTP_TEMP
> + - AD5940_ADC_INPUTP_VBIAS_CAP
> + - AD5940_ADC_INPUTP_DE0
> + - AD5940_ADC_INPUTP_SE0
> + - AD5940_ADC_INPUTP_VREF_2V5_2
> + - AD5940_ADC_INPUTP_VREF_1V82
> + - AD5940_ADC_INPUTP_P_TEMP_N
> + - AD5940_ADC_INPUTP_AIN4
> + - AD5940_ADC_INPUTP_AIN6
> + - AD5940_ADC_INPUTP_VZERO
> + - AD5940_ADC_INPUTP_VBIAS0
> + - AD5940_ADC_INPUTP_VCE0
> + - AD5940_ADC_INPUTP_VRE0
> + - AD5940_ADC_INPUTP_VCE0_2
> + - AD5940_ADC_INPUTP_LPTIA
> + - AD5940_ADC_INPUTP_AGND_REF
> +
> + - description: Negtive input channel
> + - enum:
> + # Negtive input candidates
> + - AD5940_ADC_INPUTN_FLOATING
> + - AD5940_ADC_INPUTN_HSTIA
> + - AD5940_ADC_INPUTN_LPTIA
> + - AD5940_ADC_INPUTN_AIN0
> + - AD5940_ADC_INPUTN_AIN1
> + - AD5940_ADC_INPUTN_AIN2
> + - AD5940_ADC_INPUTN_AIN3
> + - AD5940_ADC_INPUTN_VBIAS_CA8
> + - AD5940_ADC_INPUTN_TEMP_N
> + - AD5940_ADC_INPUTN_AIN4
> + - AD5940_ADC_INPUTN_AIN6
> + - AD5940_ADC_INPUTN_VZERO
> + - AD5940_ADC_INPUTN_VBIAS0
> + - AD5940_ADC_INPUTN_EXCITATION
> +
> + channel-name:
> + description:
> + Any string format name you would like to assign to this channel.
> + maxItems: 1
> +
> + required:
> + - reg
> + - diff-channels
> + - channel-name
> +
> +examples:
> + - |
> + ad5940: ad5940@0 {
> + compatible = "adi,ad5940";
> + reg = <0>;
> + spi-max-frequency = <16000000>;
> + vref-supply = <&adc_vref>;
> + interrupt-parent = <&gpio>;
> + interrupts = <24 2>;
> +
> + adi,interrupt-io = <0>;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + channel@0 {
> + reg = <0>;
> + diff-channels = <AD5940_ADC_INPUTP_VCE0
> + AD5940_ADC_INPUTN_VBIAS0>;
> + channel-name = "Vce-Vbias";
> + };
> +
> + channel@1 {
> + reg = <1>;
> + diff-channels = <AD5940_ADC_INPUTP_VRE0
> + AD5940_ADC_INPUTN_VBIAS0>;
> + channel-name = "Vre-Vbias";
> + };
> +
> + channel@2 {
> + reg = <2>;
> + diff-channels = <AD5940_ADC_INPUTP_SE0
> + AD5940_ADC_INPUTN_VBIAS0>;
> + channel-name = "Vse-Vbias";
> + };
> +
> + channel@3 {
> + reg = <3>;
> + diff-channels = <AD5940_ADC_INPUTP_DE0
> + AD5940_ADC_INPUTN_VBIAS0>;
> + channel-name = "Vde-Vbias";
> + };
> +
> + channel@4 {
> + reg = <4>;
> + diff-channels = <AD5940_ADC_INPUTP_AIN0
> + AD5940_ADC_INPUTN_VBIAS0>;
> + channel-name = "ain0-Vbias";
> + };
> +
> + channel@5 {
> + reg = <5>;
> + diff-channels = <AD5940_ADC_INPUTP_AIN1
> + AD5940_ADC_INPUTN_VBIAS0>;
> + channel-name = "ain1-Vbias";
> + };
> +
> + channel@6 {
> + reg = <6>;
> + diff-channels = <AD5940_ADC_INPUTP_AIN2
> + AD5940_ADC_INPUTN_VBIAS0>;
> + channel-name = "ain2-Vbias";
> + };
> +
> + channel@7 {
> + reg = <7>;
> + diff-channels = <AD5940_ADC_INPUTP_AIN3
> + AD5940_ADC_INPUTN_VBIAS0>;
> + channel-name = "ain3-Vbias";
> + };
> +
> + channel@8 {
> + reg = <8>;
> + diff-channels = <AD5940_ADC_INPUTP_AIN4
> + AD5940_ADC_INPUTN_VBIAS0>;
> + channel-name = "ain4-Vbias";
> + };
> +
> + channel@9 {
> + reg = <9>;
> + diff-channels = <AD5940_ADC_INPUTP_AIN6
> + AD5940_ADC_INPUTN_VBIAS0>;
> + channel-name = "ain6-Vbias";
> + };
> +
> + channel@10 {
> + reg = <10>;
> + diff-channels = <AD5940_ADC_INPUTP_LPTIA_LP
> + AD5940_ADC_INPUTN_LPTIA>;
> + channel-name = "Low power TIA DC";
> + };
> +
> + channel@11 {
> + reg = <11>;
> + diff-channels = <AD5940_ADC_INPUTP_LPTIA
> + AD5940_ADC_INPUTN_LPTIA>;
> + channel-name = "Low power TIA AC";
> + };
> +
> + channel@12 {
> + reg = <12>;
> + diff-channels = <AD5940_ADC_INPUTP_HSTIA
> + AD5940_ADC_INPUTN_HSTIA>;
> + channel-name = "High Speed TIA";
> + };
> +
> + channel@13 {
> + reg = <13>;
> + diff-channels = <AD5940_ADC_INPUTP_TEMP
> + AD5940_ADC_INPUTN_VBIAS0>;
> + channel-name = "Temperature";
> + };
> + };
> diff --git a/include/dt-bindings/iio/adc/adi,ad5940.h b/include/dt-bindings/iio/adc/adi,ad5940.h
> new file mode 100644
> index 000000000000..c17826f2f654
> --- /dev/null
> +++ b/include/dt-bindings/iio/adc/adi,ad5940.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * This header provides constants for configuring the AD5940 AFE
> + */
> +
> +#ifndef _DT_BINDINGS_IIO_ADC_AD5940_H
> +#define _DT_BINDINGS_IIO_ADC_AD5940_H
> +
> +#define AD5940_ADC_INPUTN_FLOATING 0
> +#define AD5940_ADC_INPUTN_HSTIA 1
> +#define AD5940_ADC_INPUTN_LPTIA 2
> +#define AD5940_ADC_INPUTN_AIN0 4
> +#define AD5940_ADC_INPUTN_AIN1 5
> +#define AD5940_ADC_INPUTN_AIN2 6
> +#define AD5940_ADC_INPUTN_AIN3 7
> +#define AD5940_ADC_INPUTN_VBIAS_CA8 10
> +#define AD5940_ADC_INPUTN_TEMP_N 11
> +#define AD5940_ADC_INPUTN_AIN4 12
> +#define AD5940_ADC_INPUTN_AIN6 14
> +#define AD5940_ADC_INPUTN_VZERO 16
> +#define AD5940_ADC_INPUTN_VBIAS0 17
> +#define AD5940_ADC_INPUTN_EXCITATION 20
> +
> +#define AD5940_ADC_INPUTP_FLOATING 0
> +#define AD5940_ADC_INPUTP_HSTIA 1
> +#define AD5940_ADC_INPUTP_LPTIA_LP 2
> +#define AD5940_ADC_INPUTP_AIN0 4
> +#define AD5940_ADC_INPUTP_AIN1 5
> +#define AD5940_ADC_INPUTP_AIN2 6
> +#define AD5940_ADC_INPUTP_AIN3 7
> +#define AD5940_ADC_INPUTP_AVDD_2 8
> +#define AD5940_ADC_INPUTP_DVDD_2 9
> +#define AD5940_ADC_INPUTP_AVDD_REG_2 10
> +#define AD5940_ADC_INPUTP_TEMP 11
> +#define AD5940_ADC_INPUTP_VBIAS_CAP 12
> +#define AD5940_ADC_INPUTP_DE0 13
> +#define AD5940_ADC_INPUTP_SE0 14
> +#define AD5940_ADC_INPUTP_VREF_2V5_2 16
> +#define AD5940_ADC_INPUTP_VREF_1V82 18
> +#define AD5940_ADC_INPUTP_P_TEMP_N 19
> +#define AD5940_ADC_INPUTP_AIN4 20
> +#define AD5940_ADC_INPUTP_AIN6 22
> +#define AD5940_ADC_INPUTP_VZERO 23
> +#define AD5940_ADC_INPUTP_VBIAS0 24
> +#define AD5940_ADC_INPUTP_VCE0 25
> +#define AD5940_ADC_INPUTP_VRE0 26
> +#define AD5940_ADC_INPUTP_VCE0_2 31
> +#define AD5940_ADC_INPUTP_LPTIA 33
> +#define AD5940_ADC_INPUTP_AGND_REF 35
> +#define AD5940_ADC_INPUTP_EXCITATION 36
> +
> +#endif /* _DT_BINDINGS_IIO_ADC_AD5940 */
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] iio: adc: add driver support for AD5940
2019-11-08 13:09 ` [PATCH 2/3] iio: adc: add driver " Song Qiang
@ 2019-11-10 16:55 ` Jonathan Cameron
2019-11-14 14:05 ` Song Qiang
0 siblings, 1 reply; 13+ messages in thread
From: Jonathan Cameron @ 2019-11-10 16:55 UTC (permalink / raw)
To: Song Qiang
Cc: lars, Michael.Hennerich, stefan.popa, knaack.h, pmeerw,
lgirdwood, broonie, robh+dt, mark.rutland, linux-iio, devicetree,
linux-kernel
On Fri, 8 Nov 2019 21:09:45 +0800
Song Qiang <songqiang1304521@gmail.com> wrote:
> The AD5940 is a high precision, low power analog front end (AFE)
> designed for portable applications that require high precision,
> electrochemical-based measurement techniques, such as amper-
> ometric, voltammetric, or impedance measurements.
>
> Datasheet:
> Link: https://www.analog.com/media/en/technical-documentation/data-sheets/AD5940.pdf
>
> Signed-off-by: Song Qiang <songqiang1304521@gmail.com>
Nice little driver on the whole. I'm guessing this is very much the 'first of
many' patches to support this complex part. Makes sense and keeps it manageable
from a review point of view.
A few comments inline.
Thanks,
Jonathan
> ---
> .../ABI/testing/sysfs-bus-iio-adc-ad5940 | 7 +
> drivers/iio/adc/Kconfig | 10 +
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ad5940.c | 679 ++++++++++++++++++
> 4 files changed, 697 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad5940
> create mode 100644 drivers/iio/adc/ad5940.c
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad5940 b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad5940
> new file mode 100644
> index 000000000000..839644ffbd9b
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad5940
> @@ -0,0 +1,7 @@
> +What: /sys/bus/iio/devices/iio:deviceX/events/in_voltageY-voltageZ_name
> +Date: June 2019
> +KernelVersion: 4.14.5
> +Contact: songqiang1304521@gmail.com
> +Description:
> + Channel name assigned in devicetree, used to tell userspace
> + users which channel this is.
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index f0af3a42f53c..345f934d3444 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -11,6 +11,16 @@ config AD_SIGMA_DELTA
> select IIO_BUFFER
> select IIO_TRIGGERED_BUFFER
>
> +config AD5940
> + tristate "Analog Devices AD5940 ADC Driver"
> + depends on SPI
> + help
> + Say yes here to build support for the ADC part of Analog Devices
> + analog front end AD5940.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called ad5940.
> +
> config AD7124
> tristate "Analog Devices AD7124 and similar sigma-delta ADCs driver"
> depends on SPI_MASTER
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index ef9cc485fb67..ac52d1b6f3fb 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -5,6 +5,7 @@
>
> # When adding new entries keep the list in alphabetical order
> obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
> +obj-$(CONFIG_AD5940) += ad5940.o
> obj-$(CONFIG_AD7124) += ad7124.o
> obj-$(CONFIG_AD7266) += ad7266.o
> obj-$(CONFIG_AD7291) += ad7291.o
> diff --git a/drivers/iio/adc/ad5940.c b/drivers/iio/adc/ad5940.c
> new file mode 100644
> index 000000000000..57f737edca36
> --- /dev/null
> +++ b/drivers/iio/adc/ad5940.c
> @@ -0,0 +1,679 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * AD5940 SPI ADC driver
> + *
> + * Copyright (C) 2019 Song Qiang <songqiang1304521@gmail.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bsearch.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of_irq.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#include <linux/iio/iio.h>
> +
> +#define AD5940_CHANNEL_AINP_MSK GENMASK(5, 0)
> +#define AD5940_CHANNEL_AINP(x) FIELD_PREP(AD5940_CHANNEL_AINP_MSK, x)
> +#define AD5940_CHANNEL_AINN_MSK GENMASK(12, 8)
> +#define AD5940_CHANNEL_AINN(x) FIELD_PREP(AD5940_CHANNEL_AINN_MSK, x)
> +
> +#define AD5940_CHANNEL_NAME 0
> +
> +#define AD5940_SPICMD_SETADDR 0x20
> +#define AD5940_SPICMD_READREG 0x6d
> +#define AD5940_SPICMD_WRITEREG 0x2d
> +#define AD5940_SPICMD_READFIFO 0x5f
> +
> +#define AD5940_REG_AFECON 0x00002000
> +#define AD5940_AFECON_ADCCONV_MSK BIT(8)
> +#define AD5940_AFECON_ADCCONV_EN FIELD_PREP(AD5940_AFECON_ADCCONV_MSK, 1)
> +#define AD5940_AFECON_ADCCONV_DIS FIELD_PREP(AD5940_AFECON_ADCCONV_MSK, 0)
> +#define AD5940_AFECON_ADCEN_MSK BIT(7)
> +#define AD5940_AFECON_ADCEN FIELD_PREP(AD5940_AFECON_ADCEN_MSK, 1)
> +
> +#define AD5940_REG_ADCDAT 0x00002074
> +#define AD5940_REG_DFTREAL 0x00002078
> +#define AD5940_REG_DFTIMAG 0x0000207C
> +#define AD5940_REG_SINC2DAT 0x00002080
> +#define AD5940_REG_TEMPSENSDAT 0x00002084
> +#define AD5940_REG_STATSMEAN 0x000021C8
> +#define AD5940_REG_STATSVAR 0x000021C0
> +
> +#define AD5940_REG_INTCPOL 0x00003000 /* Interrupt Polarity. */
> +#define AD5940_INTCPOL_MSK BIT(0)
> +#define AD5940_INTCPOL_POS FIELD_PREP(AD5940_INTCPOL_MSK, 1)
> +#define AD5940_INTCPOL_NEG FIELD_PREP(AD5940_INTCPOL_MSK, 0)
> +#define AD5940_REG_INTCLR 0x00003004
> +
> +#define AD5940_REG_PMBW 0x000022F0
> +#define AD5940_PMBW_SYSHS_MSK BIT(0)
> +#define AD5940_PMBW_HP FIELD_PREP(AD5940_PMBW_SYSHS_MSK, 1)
> +#define AD5940_PMBW_LP FIELD_PREP(AD5940_PMBW_SYSHS_MSK, 0)
> +#define AD5940_PMBW_SYSBW_MSK GENMASK(3, 2)
> +#define AD5940_PMBW_BWNA FIELD_PREP(AD5940_PMBW_SYSBW_MSK, 0)
> +#define AD5940_PMBW_BW50 FIELD_PREP(AD5940_PMBW_SYSBW_MSK, 1)
> +#define AD5940_PMBW_BW100 FIELD_PREP(AD5940_PMBW_SYSBW_MSK, 2)
> +#define AD5940_PMBW_BW250 FIELD_PREP(AD5940_PMBW_SYSBW_MSK, 3)
> +
> +#define AD5940_REG_ADCCON 0x000021A8
> +#define AD5940_ADCCON_PGA_MSK GENMASK(18, 16)
> +#define AD5940_ADCCON_PGA_1 FIELD_PREP(AD5940_ADCCON_PGA_MSK, 0)
> +#define AD5940_ADCCON_PGA_1P5 FIELD_PREP(AD5940_ADCCON_PGA_MSK, 1)
> +#define AD5940_ADCCON_PGA_2 FIELD_PREP(AD5940_ADCCON_PGA_MSK, 2)
> +#define AD5940_ADCCON_PGA_4 FIELD_PREP(AD5940_ADCCON_PGA_MSK, 3)
> +#define AD5940_ADCCON_PGA_9 FIELD_PREP(AD5940_ADCCON_PGA_MSK, 4)
> +#define AD5940_ADCCON_MUX_MSK (GENMASK(12, 8) | GENMASK(5, 0))
> +
> +#define AD5940_REG_INTCLR 0x00003004
> +#define AD5940_REG_INTCSEL0 0x00003008
> +#define AD5940_INTC_BRK BIT(31)
> +#define AD5940_INTC_OUTLIER BIT(29)
> +#define AD5940_INTC_FIFO_UNDERFLOW BIT(27)
> +#define AD5940_INTC_FIFO_OVERFLOW BIT(26)
> +#define AD5940_INTC_FIFO_THRESHOLD BIT(25)
> +#define AD5940_INTC_FIFO_EMPTY BIT(24)
> +#define AD5940_INTC_FIFO_FULL BIT(23)
> +#define AD5940_INTC_SEQ_TO_ERR BIT(17)
> +#define AD5940_INTC_SEQ_TO_FINISH BIT(16)
> +#define AD5940_INTC_SEQ_END BIT(15)
> +#define AD5940_INTC_BL_DONE BIT(13)
> +#define AD5940_INTC_IRQ_3 BIT(12)
> +#define AD5940_INTC_IRQ_2 BIT(11)
> +#define AD5940_INTC_IRQ_1 BIT(10)
> +#define AD5940_INTC_IRQ_0 BIT(9)
> +#define AD5940_INTC_VAR BIT(8)
> +#define AD5940_INTC_MEAN BIT(7)
> +#define AD5940_INTC_ADC_DELTA_FAIL BIT(6)
> +#define AD5940_INTC_ADC_MAX_FAIL BIT(5)
> +#define AD5940_INTC_ADC_MIN_FAIL BIT(4)
> +#define AD5940_INTC_TEMP BIT(3)
> +#define AD5940_INTC_SINC2 BIT(2)
> +#define AD5940_INTC_DFT BIT(1)
> +#define AD5940_INTC_ADC BIT(0)
> +
> +#define AD5940_REG_GP0CON 0x00000000
> +#define AD5940_GP0CON_3_MSK GENMASK(7, 6)
> +#define AD5940_GP0CON_3_INT FIELD_PREP(AD5940_GP0CON_3_MSK, 3)
> +#define AD5940_GP0CON_6_MSK GENMASK(13, 12)
> +#define AD5940_GP0CON_6_INT FIELD_PREP(AD5940_GP0CON_6_MSK, 3)
> +
> +#define AD5940_REG_GP0OEN 0x00000004
> +
> +#define AD5940_AFECON_CHIPID 0x00000404
> +#define AD5940_CHIPID 0x5502
> +
> +struct ad5940_channel_config {
> + u32 ain;
> + const char *channel_name;
> +};
> +
> +struct ad5940_state {
> + struct spi_device *spi;
> + /* This mutex is for protecting the SPI command sequences. */
> + struct mutex lock;
> +
> + struct completion complete;
> + u32 conversion_time;
> +
> + u8 n_input;
> + u8 p_input;
> +
> + struct regulator *vref;
> + int vref_mv;
> +
> + int num_channels;
> + struct ad5940_channel_config *channel_config;
> + union {
> + __be32 d32;
> + u8 d8[5];
> + } data ____cacheline_aligned;
> +};
> +
> +static int ad5940_set_addr(struct ad5940_state *st, u16 addr)
> +{
> + st->data.d8[0] = AD5940_SPICMD_SETADDR;
> + st->data.d8[1] = (addr >> 8) & 0xff;
> + st->data.d8[2] = addr & 0xff;
> +
> + return spi_write(st->spi, st->data.d8, 3);
> +}
> +
> +static int ad5940_read_reg(struct ad5940_state *st, u16 addr, u32 *data)
> +{
> + int rx_len;
> + u8 shift;
> + int ret;
> +
> + ret = ad5940_set_addr(st, addr);
> + if (ret < 0)
> + return ret;
> +
> + if ((addr >= 0x1000) && (addr <= 0x3014))
> + rx_len = 4;
> + else
> + rx_len = 2;
> +
> + st->data.d8[0] = AD5940_SPICMD_READREG;
> + st->data.d8[1] = 0;
> + shift = 32 - (8 * rx_len);
> +
> + ret = spi_write_then_read(st->spi, st->data.d8, 2, &st->data.d32,
> + rx_len);
> + if (ret < 0)
> + return ret;
> +
> + *data = be32_to_cpu(st->data.d32) >> shift;
> +
> + return 0;
> +}
> +
> +static int ad5940_write_reg(struct ad5940_state *st, u16 addr, u32 data)
> +{
> + int tx_len;
> + int ret;
> +
> + ret = ad5940_set_addr(st, addr);
> + if (ret < 0)
> + return ret;
> +
> + st->data.d8[0] = AD5940_SPICMD_WRITEREG;
> + if ((addr >= 0x1000) && (addr <= 0x3014)) {
> + st->data.d8[1] = (data >> 24) & 0xff;
> + st->data.d8[2] = (data >> 16) & 0xff;
> + st->data.d8[3] = (data >> 8) & 0xff;
> + st->data.d8[4] = data & 0xff;
> + tx_len = 5;
> + } else {
> + st->data.d8[1] = (data >> 8) & 0xff;
> + st->data.d8[2] = data & 0xff;
> + tx_len = 3;
> + }
> +
> + return spi_write(st->spi, st->data.d8, tx_len);
> +}
> +
> +static int ad5940_write_reg_mask(struct ad5940_state *st, u16 addr,
> + u32 mask, u32 data)
> +{
> + u32 temp;
> + int ret;
> +
> + ret = ad5940_read_reg(st, addr, &temp);
> + if (ret < 0)
> + return ret;
> +
> + temp &= ~mask;
> + temp |= data;
> +
> + return ad5940_write_reg(st, addr, temp);
> +}
> +
> +static ssize_t ad5940_read_info(struct iio_dev *indio_dev,
> + uintptr_t private,
> + const struct iio_chan_spec *chan,
> + char *buf)
> +{
> + struct ad5940_state *st = iio_priv(indio_dev);
> +
> + switch ((u32)private) {
> + case AD5940_CHANNEL_NAME:
What is the logic here in this magic define?
> + return sprintf(buf, "%s\n",
> + st->channel_config[chan->address].channel_name);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct iio_chan_spec_ext_info ad4590_ext_info[] = {
> + {
> + .name = "name",
This is defining new ABI. I'm not necessarily against doing so but
it needs a separate documentation patch so that it's easy to discuss.
Documentation/ABI/testing/sysfs-bus-iio
We might want to think of a similar naming to the label we recently
added for the device itself.
> + .read = ad5940_read_info,
> + .private = AD5940_CHANNEL_NAME,
> + .shared = IIO_SEPARATE,
> + },
> + { },
> +};
> +
> +static const struct iio_chan_spec ad5940_channel_template = {
> + .type = IIO_VOLTAGE,
> + .differential = 1,
> + .indexed = 1,
> + .ext_info = ad4590_ext_info,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> +};
> +
> +static int ad5940_clear_ready(struct ad5940_state *st)
> +{
> + int ret;
> +
> + ret = ad5940_write_reg_mask(st, AD5940_REG_AFECON,
> + AD5940_AFECON_ADCCONV_MSK,
> + AD5940_AFECON_ADCCONV_DIS);
> + if (ret < 0)
> + return ret;
> +
> + return ad5940_write_reg(st, AD5940_REG_INTCLR, AD5940_INTC_ADC);
> +}
> +
> +static irqreturn_t ad5940_irq_handler(int irq, void *private)
> +{
> + struct ad5940_state *st = private;
> + int ret;
> +
> + ret = ad5940_clear_ready(st);
> + if (ret < 0)
> + return ret;
> +
> + complete(&st->complete);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int ad5940_scan_direct(struct ad5940_state *st, u32 mux, int *val)
> +{
> + int ret;
> + u32 result;
> +
> + mutex_lock(&st->lock);
> + ret = ad5940_write_reg_mask(st, AD5940_REG_ADCCON,
> + AD5940_ADCCON_MUX_MSK, mux);
> + if (ret < 0)
> + goto unlock_return;
> +
> + reinit_completion(&st->complete);
> + ret = ad5940_write_reg_mask(st, AD5940_REG_AFECON,
> + AD5940_AFECON_ADCCONV_MSK,
> + AD5940_AFECON_ADCCONV_EN);
> + if (ret < 0)
> + goto unlock_return;
> +
> + ret = wait_for_completion_timeout(&st->complete,
> + msecs_to_jiffies(1000));
> + if (!ret) {
> + ad5940_clear_ready(st);
> + ret = -ETIMEDOUT;
> + goto unlock_return;
> + }
> +
> + ret = ad5940_read_reg(st, AD5940_REG_ADCDAT, &result);
> + mutex_unlock(&st->lock);
> + if (ret < 0)
> + return ret;
As you already have the unlock_return below on this occasion I would
move the if (ret) inside the lock so all similar errors take a simpler
exit path. If this was the only case and you didn't have the handling
I would agree with how you have done it.
> + *val = result & 0xffff;
> +
> + return 0;
> +
> +unlock_return:
> + mutex_unlock(&st->lock);
> + return ret;
> +}
> +
> +static int ad5940_read_raw(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan, int *val, int *val2, long info)
> +{
> + struct ad5940_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + switch (info) {
> + case IIO_CHAN_INFO_RAW:
> + ret = ad5940_scan_direct(st,
> + st->channel_config[chan->address].ain,
> + val);
> + if (ret < 0)
> + return ret;
> + else
> + return IIO_VAL_INT;
return IIO_VAL_INT;
Cleaner to only have the error path indented.
if / else kind of implies some 'equality' of the two
options.
> + case IIO_CHAN_INFO_SCALE:
> + *val = st->vref_mv;
> + *val2 = 16;
> + return IIO_VAL_FRACTIONAL_LOG2;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct iio_info ad5940_info = {
> + .read_raw = &ad5940_read_raw,
> +};
> +
> +int cmp_u8(const void *a, const void *b)
> +{
> + return (*(u8 *)a - *(u8 *)b);
> +}
> +
> +static int ad5940_check_channel_indexes(struct device *dev, u32 *ain)
> +{
> + const u8 channel_p[] = {
> + 0, 1, 2, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 16, 18, 19,
> + 20, 22, 23, 24, 25, 26, 31, 33, 35, 36
> + };
> + const u8 channel_n[] = {
> + 0, 1, 2, 4, 5, 6, 7, 10, 11, 12, 14, 16, 17, 20
> + };
> + u8 *index;
> +
> + index = (u8 *) bsearch(&ain[0], channel_p, ARRAY_SIZE(channel_p),
> + sizeof(u8), cmp_u8);
> + if (!index) {
> + dev_err(dev, "Positive input index not found.\n");
> + return -EINVAL;
> + }
> +
> + index = (u8 *) bsearch(&ain[1], channel_n, ARRAY_SIZE(channel_n),
> + sizeof(u8), cmp_u8);
> + if (!index) {
> + dev_err(dev, "negtive input index not found.\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int ad5940_of_parse_channel_config(struct iio_dev *indio_dev,
> + struct device_node *np)
> +{
> + struct ad5940_state *st = iio_priv(indio_dev);
> + struct iio_chan_spec *chan;
> + struct device_node *child;
> + u32 channel, ain[2];
> + int ret;
> +
> + st->num_channels = of_get_available_child_count(np);
> + if (!st->num_channels) {
> + dev_err(indio_dev->dev.parent, "no channel children\n");
> + return -ENODEV;
> + }
> +
> + chan = devm_kcalloc(indio_dev->dev.parent, st->num_channels,
> + sizeof(*chan), GFP_KERNEL);
> + if (!chan)
> + return -ENOMEM;
> +
> + st->channel_config = devm_kcalloc(indio_dev->dev.parent,
> + st->num_channels,
> + sizeof(*st->channel_config),
> + GFP_KERNEL);
> + if (!st->channel_config)
> + return -ENOMEM;
> +
> + indio_dev->channels = chan;
> + indio_dev->num_channels = st->num_channels;
> +
> + for_each_available_child_of_node(np, child) {
> + ret = of_property_read_u32(child, "reg", &channel);
> + if (ret)
> + goto err;
> +
> + ret = of_property_read_u32_array(child, "diff-channels",
> + ain, 2);
> + if (ret)
> + goto err;
> +
> + ret = of_property_read_string(child, "channel-name",
> + &st->channel_config[channel].channel_name);
> + if (ret)
> + st->channel_config[channel].channel_name = "none-name";
You have this as required I think in the dt properties. If that is the case then
enforce it and refuse to load the driver if not supplied. Otherwise change
the dt docs to make it optional (which is probably better)
> +
> + ret = ad5940_check_channel_indexes(indio_dev->dev.parent, ain);
> + if (ret) {
> + dev_err(indio_dev->dev.parent,
> + "some input channel index does not exist: %d, %d, %d",
> + channel, ain[0], ain[1]);
> + goto err;
> + }
> +
> + st->channel_config[channel].ain = AD5940_CHANNEL_AINP(ain[0]) |
> + AD5940_CHANNEL_AINN(ain[1]);
> +
> + *chan = ad5940_channel_template;
> + chan->address = channel;
> + chan->scan_index = channel;
> + chan->channel = ain[0];
> + chan->channel2 = ain[1];
> +
> + chan++;
> + }
> +
> + return 0;
> +err:
> + of_node_put(child);
> +
> + return ret;
> +}
> +
> +static int ad5940_config_polarity(struct ad5940_state *st, u32 polarity)
> +{
> + u32 val;
> +
> + if (polarity == IRQF_TRIGGER_RISING)
> + val = AD5940_INTCPOL_POS;
> + else
> + val = AD5940_INTCPOL_NEG;
> +
> + return ad5940_write_reg_mask(st, AD5940_REG_INTCPOL,
> + AD5940_INTCPOL_MSK, val);
> +}
> +
> +static int ad5940_config_int_io(struct ad5940_state *st, u8 int_io)
> +{
> + int ret = 0;
> +
> + if (int_io == 3)
Switch statement preferred for matches like this.
> + ret = ad5940_write_reg_mask(st, AD5940_REG_GP0CON,
> + AD5940_GP0CON_3_MSK,
> + AD5940_GP0CON_3_INT);
> + else if (int_io == 6)
> + ret = ad5940_write_reg_mask(st, AD5940_REG_GP0CON,
> + AD5940_GP0CON_6_MSK,
> + AD5940_GP0CON_6_INT);
> + if (ret < 0)
> + return ret;
> +
> + return ad5940_write_reg(st, AD5940_REG_GP0OEN, BIT(int_io));
> +}
> +
> +static const u32 ad5940_powerup_setting[][2] = {
Hmm. This is not good practice when we have docs for the values.
If we can provide a breakdown into what is being set that would be
great. I can't find docs immediately for some of these however...
> + { 0x0908, 0x02c9 },
> + { 0x0c08, 0x206c },
> + { 0x21f0, 0x0010 },
This one is is simply saying only 1 repeat conversion for example.
Add some defines and you can make that clear.
> + { 0x0410, 0x02c9 },
> + { 0x0a28, 0x0009 },
> + { 0x238c, 0x0104 },
> + { 0x0a04, 0x4859 },
> + { 0x0a04, 0xf27b },
> + { 0x0a00, 0x8009 },
> + { 0x22f0, 0x0000 },
> + { 0x2230, 0xde87a5af },
> + { 0x2250, 0x103f },
> + { 0x22b0, 0x203c },
> + { 0x2230, 0xde87a5a0 },
> +};
> +
> +static int ad5940_setup(struct ad5940_state *st, u8 int_io)
> +{
> + u32 chip_id;
> + int ret;
> + u8 i;
> +
> + for (i = 0; i < ARRAY_SIZE(ad5940_powerup_setting); i++) {
> + ret = ad5940_write_reg(st, ad5940_powerup_setting[i][0],
> + ad5940_powerup_setting[i][1]);
> + if (ret < 0)
> + return ret;
> + }
> +
> + ret = ad5940_read_reg(st, AD5940_AFECON_CHIPID, &chip_id);
> + if (ret < 0)
> + return ret;
> + if (chip_id != AD5940_CHIPID) {
> + dev_err(&st->spi->dev, "Wrong chip ID with 0x%x.", chip_id);
> + return -ENXIO;
> + }
> + dev_info(&st->spi->dev, "Found ad5940");
There are lots of ways of telling this, so this is just noise in the log.
If there was other useful information conveyed (serial number etc) then it
would be fine. For a simple 'found', don't bother. So drop this print.
> +
> + ret = ad5940_write_reg(st, AD5940_REG_PMBW,
> + AD5940_PMBW_LP | AD5940_PMBW_BW250);
> + if (ret < 0)
> + return ret;
> +
> + ret = ad5940_config_int_io(st, int_io);
> + if (ret < 0)
> + return ret;
> +
> + ret = ad5940_clear_ready(st);
> + if (ret < 0)
> + return ret;
> +
> + ret = ad5940_write_reg(st, AD5940_REG_INTCSEL0, AD5940_INTC_ADC);
> + if (ret < 0)
> + return ret;
> +
> + ret = ad5940_write_reg_mask(st, AD5940_REG_ADCCON,
> + AD5940_ADCCON_PGA_MSK,
> + AD5940_ADCCON_PGA_1);
> + if (ret < 0)
> + return ret;
> +
> + return ad5940_write_reg_mask(st, AD5940_REG_AFECON,
> + AD5940_AFECON_ADCEN_MSK,
> + AD5940_AFECON_ADCEN);
> +}
> +
> +static void ad5940_regulator_disable(void *data)
> +{
> + struct ad5940_state *st = data;
> +
> + regulator_disable(st->vref);
> +}
> +
> +static inline int ad5940_check_int_io(u32 io, struct device *dev)
> +{
> + static const u8 int_io[3] = {0, 3, 6};
> + u8 *p;
> +
> + p = bsearch(&io, int_io, 3, sizeof(u8), cmp_u8);
I'd just use a switch statement here as there are only 3 options.
> + if (!p) {
> + dev_err(dev, "interrupt output pin not valid,");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int ad5940_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
> + struct ad5940_state *st;
> + u32 trig_type;
> + int vref_uv = 0;
> + u32 int_io;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> +
> + ret = device_property_read_u32(&spi->dev, "adi,interrupt-io", &int_io);
For other devices we have done the same thing via interrupt names. See
the imu/adis16480 for example. That lets us use generic bindings.
> + if (ret) {
> + dev_err(&spi->dev,
> + "reading dt property 'adi,interrupt-io' failed.");
> + return -EINVAL;
> + }
> +
> + st->spi = spi;
> +
> + trig_type = irq_get_trigger_type(spi->irq);
> + if (trig_type != IRQF_TRIGGER_RISING &&
> + trig_type != IRQF_TRIGGER_FALLING) {
> + dev_err(&spi->dev, "trigger type must be rising or falling.");
> + return -EINVAL;
> + }
Slight preference for a blank line here.
> + ret = ad5940_config_polarity(st, trig_type);
> + if (ret < 0) {
> + dev_err(&spi->dev, "config polarity failed.");
> + return ret;
> + }
> +
> + ret = ad5940_check_int_io(int_io, &spi->dev);
> + if (ret < 0)
> + return ret;
> +
> + st->vref = devm_regulator_get_optional(&spi->dev, "vref");
> + if (!IS_ERR(st->vref)) {
> + ret = regulator_enable(st->vref);
> + if (ret) {
> + dev_err(&spi->dev, "Failed to enbale specified vref supply.\n");
> + return ret;
> + }
> +
> + ret = devm_add_action_or_reset(&spi->dev,
> + ad5940_regulator_disable, st);
> + if (ret) {
> + regulator_disable(st->vref);
> + return ret;
> + }
> +
> + ret = regulator_get_voltage(st->vref);
> + if (ret < 0)
> + return ret;
We could move this read to be on demand, but I doubt anyone will share a voltage
reference between this device and any other, so unlikely this value will change.
> +
> + vref_uv = ret;
> + }
> +
> + if (vref_uv)
> + st->vref_mv = vref_uv / 1000;
> + else
> + st->vref_mv = 1820;
Hmm. So the one way this code is different from using the above
if statement is if there is a vref regulator, but it's voltage == 0.
That seems unlikely to occur in a real system. Perhaps we can just move
the regulator value based version into the block above and initialize
the st->vref_mv to 1820 either in an else block to that or from the start.
> +
> + indio_dev->dev.parent = &spi->dev;
> + indio_dev->dev.of_node = spi->dev.of_node;
> + indio_dev->name = spi->dev.of_node->name;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->info = &ad5940_info;
> +
> + ret = ad5940_of_parse_channel_config(indio_dev, spi->dev.of_node);
> + if (ret < 0)
> + return ret;
> +
> + init_completion(&st->complete);
> +
> + ret = ad5940_setup(st, int_io);
> + if (ret)
> + return ret;
> +
> + ret = devm_request_threaded_irq(&spi->dev, spi->irq, NULL,
> + ad5940_irq_handler,
> + trig_type | IRQF_ONESHOT,
> + dev_name(&spi->dev), st);
> + if (ret < 0)
> + return ret;
> +
> + return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +
> +static const struct of_device_id ad5940_dt_match[] = {
> + { .compatible = "adi,ad5940" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, ad5940_spi_ids);
> +
> +static struct spi_driver ad5940_driver = {
> + .driver = {
> + .name = "ad5940",
> + .of_match_table = ad5940_dt_match,
> + },
> + .probe = ad5940_probe,
> +};
> +module_spi_driver(ad5940_driver);
> +
> +MODULE_AUTHOR("Song Qiang <songqiang1304521@gmail.com>");
> +MODULE_DESCRIPTION("Analog Device AD5940 ADC driver");
> +MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] dt-bindings: iio: adc: add support for AD5940
2019-11-08 13:09 [PATCH 1/3] dt-bindings: iio: adc: add support for AD5940 Song Qiang
` (2 preceding siblings ...)
2019-11-10 16:26 ` [PATCH 1/3] dt-bindings: iio: adc: add support " Jonathan Cameron
@ 2019-11-14 1:28 ` Rob Herring
2019-11-14 17:06 ` Song Qiang
3 siblings, 1 reply; 13+ messages in thread
From: Rob Herring @ 2019-11-14 1:28 UTC (permalink / raw)
To: Song Qiang
Cc: Lars-Peter Clausen, Michael Hennerich, Stefan Popa,
Jonathan Cameron, Hartmut Knaack, Peter Meerwald, Liam Girdwood,
Mark Brown, Mark Rutland, open list:IIO SUBSYSTEM AND DRIVERS,
devicetree, linux-kernel
On Fri, Nov 8, 2019 at 7:09 AM Song Qiang <songqiang1304521@gmail.com> wrote:
>
> Add yaml devicetree description file and a header file for
> helping configure positive and negtive input of AD5940.
>
> Signed-off-by: Song Qiang <songqiang1304521@gmail.com>
> ---
> .../bindings/iio/adc/adi,ad5940.yaml | 240 ++++++++++++++++++
> include/dt-bindings/iio/adc/adi,ad5940.h | 52 ++++
> 2 files changed, 292 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml
> create mode 100644 include/dt-bindings/iio/adc/adi,ad5940.h
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml
> new file mode 100644
> index 000000000000..f7f034fdd8ec
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml
> @@ -0,0 +1,240 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright 2019 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/bindings/iio/adc/adi,ad5940.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD5940 Device Tree Bindings
> +
> +maintainers:
> + - Song Qiang <songqiang1304521@gmail.com>
> +
> +description: |
> + Analog Devices AD5940 High Precision, Impedance, and Electrochemical Front End.
> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD5940.pdf
> +
> +properties:
> + compatible:
> + enum:
> + - adi,ad5940
> +
> + reg:
> + maxItems: 1
> +
> + vref-supply:
> + description:
> + The regulator to be used to supply the reference voltage.
> + maxItems: 1
> +
> + adi,interrupt-io:
> + description:
> + Output GPIO index of interrupt controller of AD5940.
> + maxItems: 1
> + allOf:
> + - $ref: /schemas/types.yaml#/definitions/uint32
> + - enum: [0, 3, 6, 7]
> +
> + '#address-cells':
> + const: 1
> +
> + '#size-cells':
> + const: 0
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - adi,interrupt-io
> +
> +patternProperties:
> + # 'channel@0-255'
> + "^channel@([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])$":
> + type: object
> + description: |
> + Represents the external channels which are connected to the ADC.
> + See Documentation/devicetree/bindings/iio/adc/adc.txt.
> + properties:
> + reg:
> + description:
> + Index of this channel, must be starting from 0.
> + maxItems: 1
Drop maxItems and do:
items:
- minimum: 0
maximum: 255
> +
> + diff-channels:
> + description:
> + Positive input and negtive input of the ADC buffer of this channel.
> + Input candidates are defined in include/dt-bindings/iio/adc/adi,ad5940.h.
> + minItems: 2
> + maxItems: 2
> + items:
> + - description: Positive input channel
> + - enum:
> + - AD5940_ADC_INPUTP_EXCITATION
You can't have defines in the schema. minimum/maximum might work better here.
> + - AD5940_ADC_INPUTP_FLOATING
> + - AD5940_ADC_INPUTP_HSTIA
> + - AD5940_ADC_INPUTP_LPTIA_LP
> + - AD5940_ADC_INPUTP_AIN0
> + - AD5940_ADC_INPUTP_AIN1
> + - AD5940_ADC_INPUTP_AIN2
> + - AD5940_ADC_INPUTP_AIN3
> + - AD5940_ADC_INPUTP_AVDD_2
> + - AD5940_ADC_INPUTP_DVDD_2
> + - AD5940_ADC_INPUTP_AVDD_REG_2
> + - AD5940_ADC_INPUTP_TEMP
> + - AD5940_ADC_INPUTP_VBIAS_CAP
> + - AD5940_ADC_INPUTP_DE0
> + - AD5940_ADC_INPUTP_SE0
> + - AD5940_ADC_INPUTP_VREF_2V5_2
> + - AD5940_ADC_INPUTP_VREF_1V82
> + - AD5940_ADC_INPUTP_P_TEMP_N
> + - AD5940_ADC_INPUTP_AIN4
> + - AD5940_ADC_INPUTP_AIN6
> + - AD5940_ADC_INPUTP_VZERO
> + - AD5940_ADC_INPUTP_VBIAS0
> + - AD5940_ADC_INPUTP_VCE0
> + - AD5940_ADC_INPUTP_VRE0
> + - AD5940_ADC_INPUTP_VCE0_2
> + - AD5940_ADC_INPUTP_LPTIA
> + - AD5940_ADC_INPUTP_AGND_REF
> +
> + - description: Negtive input channel
> + - enum:
> + # Negtive input candidates
> + - AD5940_ADC_INPUTN_FLOATING
> + - AD5940_ADC_INPUTN_HSTIA
> + - AD5940_ADC_INPUTN_LPTIA
> + - AD5940_ADC_INPUTN_AIN0
> + - AD5940_ADC_INPUTN_AIN1
> + - AD5940_ADC_INPUTN_AIN2
> + - AD5940_ADC_INPUTN_AIN3
> + - AD5940_ADC_INPUTN_VBIAS_CA8
> + - AD5940_ADC_INPUTN_TEMP_N
> + - AD5940_ADC_INPUTN_AIN4
> + - AD5940_ADC_INPUTN_AIN6
> + - AD5940_ADC_INPUTN_VZERO
> + - AD5940_ADC_INPUTN_VBIAS0
> + - AD5940_ADC_INPUTN_EXCITATION
You've defined that diff-channels is 4 items. I don't think that's
what you want. Each enum and description should be under a single '-'.
> +
> + channel-name:
Perhaps standard property 'label' should be used here. Seems like a
common thing.
> + description:
> + Any string format name you would like to assign to this channel.
> + maxItems: 1
> +
> + required:
> + - reg
> + - diff-channels
> + - channel-name
> +
> +examples:
> + - |
> + ad5940: ad5940@0 {
> + compatible = "adi,ad5940";
> + reg = <0>;
> + spi-max-frequency = <16000000>;
> + vref-supply = <&adc_vref>;
> + interrupt-parent = <&gpio>;
> + interrupts = <24 2>;
> +
> + adi,interrupt-io = <0>;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + channel@0 {
> + reg = <0>;
> + diff-channels = <AD5940_ADC_INPUTP_VCE0
> + AD5940_ADC_INPUTN_VBIAS0>;
You'll need the header included for this to build. Run 'make
dt_binding_check' on this.
> + channel-name = "Vce-Vbias";
> + };
> +
> + channel@1 {
> + reg = <1>;
> + diff-channels = <AD5940_ADC_INPUTP_VRE0
> + AD5940_ADC_INPUTN_VBIAS0>;
> + channel-name = "Vre-Vbias";
> + };
> +
> + channel@2 {
> + reg = <2>;
> + diff-channels = <AD5940_ADC_INPUTP_SE0
> + AD5940_ADC_INPUTN_VBIAS0>;
> + channel-name = "Vse-Vbias";
> + };
> +
> + channel@3 {
> + reg = <3>;
> + diff-channels = <AD5940_ADC_INPUTP_DE0
> + AD5940_ADC_INPUTN_VBIAS0>;
> + channel-name = "Vde-Vbias";
> + };
> +
> + channel@4 {
> + reg = <4>;
> + diff-channels = <AD5940_ADC_INPUTP_AIN0
> + AD5940_ADC_INPUTN_VBIAS0>;
> + channel-name = "ain0-Vbias";
> + };
> +
> + channel@5 {
> + reg = <5>;
> + diff-channels = <AD5940_ADC_INPUTP_AIN1
> + AD5940_ADC_INPUTN_VBIAS0>;
> + channel-name = "ain1-Vbias";
> + };
> +
> + channel@6 {
> + reg = <6>;
> + diff-channels = <AD5940_ADC_INPUTP_AIN2
> + AD5940_ADC_INPUTN_VBIAS0>;
> + channel-name = "ain2-Vbias";
> + };
> +
> + channel@7 {
> + reg = <7>;
> + diff-channels = <AD5940_ADC_INPUTP_AIN3
> + AD5940_ADC_INPUTN_VBIAS0>;
> + channel-name = "ain3-Vbias";
> + };
> +
> + channel@8 {
> + reg = <8>;
> + diff-channels = <AD5940_ADC_INPUTP_AIN4
> + AD5940_ADC_INPUTN_VBIAS0>;
> + channel-name = "ain4-Vbias";
> + };
> +
> + channel@9 {
> + reg = <9>;
> + diff-channels = <AD5940_ADC_INPUTP_AIN6
> + AD5940_ADC_INPUTN_VBIAS0>;
> + channel-name = "ain6-Vbias";
> + };
> +
> + channel@10 {
> + reg = <10>;
> + diff-channels = <AD5940_ADC_INPUTP_LPTIA_LP
> + AD5940_ADC_INPUTN_LPTIA>;
> + channel-name = "Low power TIA DC";
> + };
> +
> + channel@11 {
> + reg = <11>;
> + diff-channels = <AD5940_ADC_INPUTP_LPTIA
> + AD5940_ADC_INPUTN_LPTIA>;
> + channel-name = "Low power TIA AC";
> + };
> +
> + channel@12 {
> + reg = <12>;
> + diff-channels = <AD5940_ADC_INPUTP_HSTIA
> + AD5940_ADC_INPUTN_HSTIA>;
> + channel-name = "High Speed TIA";
> + };
> +
> + channel@13 {
> + reg = <13>;
> + diff-channels = <AD5940_ADC_INPUTP_TEMP
> + AD5940_ADC_INPUTN_VBIAS0>;
> + channel-name = "Temperature";
> + };
> + };
> diff --git a/include/dt-bindings/iio/adc/adi,ad5940.h b/include/dt-bindings/iio/adc/adi,ad5940.h
> new file mode 100644
> index 000000000000..c17826f2f654
> --- /dev/null
> +++ b/include/dt-bindings/iio/adc/adi,ad5940.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * This header provides constants for configuring the AD5940 AFE
> + */
> +
> +#ifndef _DT_BINDINGS_IIO_ADC_AD5940_H
> +#define _DT_BINDINGS_IIO_ADC_AD5940_H
> +
> +#define AD5940_ADC_INPUTN_FLOATING 0
> +#define AD5940_ADC_INPUTN_HSTIA 1
> +#define AD5940_ADC_INPUTN_LPTIA 2
> +#define AD5940_ADC_INPUTN_AIN0 4
> +#define AD5940_ADC_INPUTN_AIN1 5
> +#define AD5940_ADC_INPUTN_AIN2 6
> +#define AD5940_ADC_INPUTN_AIN3 7
> +#define AD5940_ADC_INPUTN_VBIAS_CA8 10
> +#define AD5940_ADC_INPUTN_TEMP_N 11
> +#define AD5940_ADC_INPUTN_AIN4 12
> +#define AD5940_ADC_INPUTN_AIN6 14
> +#define AD5940_ADC_INPUTN_VZERO 16
> +#define AD5940_ADC_INPUTN_VBIAS0 17
> +#define AD5940_ADC_INPUTN_EXCITATION 20
> +
> +#define AD5940_ADC_INPUTP_FLOATING 0
> +#define AD5940_ADC_INPUTP_HSTIA 1
> +#define AD5940_ADC_INPUTP_LPTIA_LP 2
> +#define AD5940_ADC_INPUTP_AIN0 4
> +#define AD5940_ADC_INPUTP_AIN1 5
> +#define AD5940_ADC_INPUTP_AIN2 6
> +#define AD5940_ADC_INPUTP_AIN3 7
> +#define AD5940_ADC_INPUTP_AVDD_2 8
> +#define AD5940_ADC_INPUTP_DVDD_2 9
> +#define AD5940_ADC_INPUTP_AVDD_REG_2 10
> +#define AD5940_ADC_INPUTP_TEMP 11
> +#define AD5940_ADC_INPUTP_VBIAS_CAP 12
> +#define AD5940_ADC_INPUTP_DE0 13
> +#define AD5940_ADC_INPUTP_SE0 14
> +#define AD5940_ADC_INPUTP_VREF_2V5_2 16
> +#define AD5940_ADC_INPUTP_VREF_1V82 18
> +#define AD5940_ADC_INPUTP_P_TEMP_N 19
> +#define AD5940_ADC_INPUTP_AIN4 20
> +#define AD5940_ADC_INPUTP_AIN6 22
> +#define AD5940_ADC_INPUTP_VZERO 23
> +#define AD5940_ADC_INPUTP_VBIAS0 24
> +#define AD5940_ADC_INPUTP_VCE0 25
> +#define AD5940_ADC_INPUTP_VRE0 26
> +#define AD5940_ADC_INPUTP_VCE0_2 31
> +#define AD5940_ADC_INPUTP_LPTIA 33
> +#define AD5940_ADC_INPUTP_AGND_REF 35
> +#define AD5940_ADC_INPUTP_EXCITATION 36
> +
> +#endif /* _DT_BINDINGS_IIO_ADC_AD5940 */
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] dt-bindings: iio: adc: add support for AD5940
2019-11-10 16:26 ` [PATCH 1/3] dt-bindings: iio: adc: add support " Jonathan Cameron
@ 2019-11-14 13:32 ` Song Qiang
2019-11-14 14:01 ` Ardelean, Alexandru
2019-11-16 14:15 ` Jonathan Cameron
0 siblings, 2 replies; 13+ messages in thread
From: Song Qiang @ 2019-11-14 13:32 UTC (permalink / raw)
To: Jonathan Cameron
Cc: lars, Michael.Hennerich, stefan.popa, knaack.h, pmeerw,
lgirdwood, broonie, robh+dt, mark.rutland, linux-iio, devicetree,
linux-kernel
On 11/11/19 12:26 AM, Jonathan Cameron wrote:
> On Fri, 8 Nov 2019 21:09:44 +0800
> Song Qiang <songqiang1304521@gmail.com> wrote:
>
>> Add yaml devicetree description file and a header file for
>> helping configure positive and negtive input of AD5940.
>>
>> Signed-off-by: Song Qiang <songqiang1304521@gmail.com>
> Ouch. This is a very complex device, so I'm guessing this is the tip
> of the iceberg when it comes to the eventual binding.
> For reference of others this has a similarly complex DAC and
> TIA + some excitation voltage generators (DDS).
>
> Anyhow, a few comments inline but I'll definitely be looking for
> a dt maintainer input on this one.
>
> Thanks,
>
> Jonathan
>
Thanks, this is the first part support of the driver.
>> ---
>> .../bindings/iio/adc/adi,ad5940.yaml | 240 ++++++++++++++++++
>> include/dt-bindings/iio/adc/adi,ad5940.h | 52 ++++
>> 2 files changed, 292 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml
>> create mode 100644 include/dt-bindings/iio/adc/adi,ad5940.h
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml
>> new file mode 100644
>> index 000000000000..f7f034fdd8ec
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml
>> @@ -0,0 +1,240 @@
>> +# SPDX-License-Identifier: GPL-2.0
> For new bindings, preference is to include a dual license
> (GPL-2.0-only OR BSD-2-Clause)
>
> If Analog is fine doing this that would be great.
>
I'll consult my mentor about this.
>> +# Copyright 2019 Analog Devices Inc.
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/bindings/iio/adc/adi,ad5940.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Analog Devices AD5940 Device Tree Bindings
>> +
>> +maintainers:
>> + - Song Qiang <songqiang1304521@gmail.com>
>> +
>> +description: |
>> + Analog Devices AD5940 High Precision, Impedance, and Electrochemical Front End.
>> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD5940.pdf
>> +
>> +properties:
>> + compatible:
>> + enum:
>> + - adi,ad5940
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + vref-supply:
>> + description:
>> + The regulator to be used to supply the reference voltage.
>> + maxItems: 1
>
> It's worth taking a look at similar patch reviews to pick up on things
> that are common issues. Rob has pointed out a few times recently that
> vref-supply can only ever have one item, so no need for maxItems.
>
That's right, thanks.
>> +
>> + adi,interrupt-io:
>> + description:
>> + Output GPIO index of interrupt controller of AD5940.
>> + maxItems: 1
>
> I'm fairly sure an enum can only have one entry so don't think this is
> needed.
>
>> + allOf:
>> + - $ref: /schemas/types.yaml#/definitions/uint32
>> + - enum: [0, 3, 6, 7]
>> +
>> + '#address-cells':
>> + const: 1
>> +
>> + '#size-cells':
>> + const: 0
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - interrupts
>> + - adi,interrupt-io
>> +
>> +patternProperties:
>> + # 'channel@0-255'
>
> Really? That is a lot of channels. Superficially it looks like a much
> smaller number of possibilities from the datasheet.
>
This device has some positive inputs and some negative inputs. A channel
here is a combination of one positive input and one negative input.
These channels I listed in examples are only suggested combinations in
the datasheet, while other combinations are all possible. So I was
thinking to not limit the total count of channels here. I failed to find
examples of doing this kind of stuff in the tree.
yours,
Song Qiang
>> + "^channel@([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])$":
>> + type: object
>> + description: |
>> + Represents the external channels which are connected to the ADC.
>> + See Documentation/devicetree/bindings/iio/adc/adc.txt.
>> + properties:
>> + reg:
>> + description:
>> + Index of this channel, must be starting from 0.
>> + maxItems: 1
>> +
>> + diff-channels:
>> + description:
>> + Positive input and negtive input of the ADC buffer of this channel.
>> + Input candidates are defined in include/dt-bindings/iio/adc/adi,ad5940.h.
>> + minItems: 2
>> + maxItems: 2
>> + items:
>> + - description: Positive input channel
>> + - enum:
>> + - AD5940_ADC_INPUTP_EXCITATION
>> + - AD5940_ADC_INPUTP_FLOATING
>> + - AD5940_ADC_INPUTP_HSTIA
>> + - AD5940_ADC_INPUTP_LPTIA_LP
>> + - AD5940_ADC_INPUTP_AIN0
>> + - AD5940_ADC_INPUTP_AIN1
>> + - AD5940_ADC_INPUTP_AIN2
>> + - AD5940_ADC_INPUTP_AIN3
>> + - AD5940_ADC_INPUTP_AVDD_2
>> + - AD5940_ADC_INPUTP_DVDD_2
>> + - AD5940_ADC_INPUTP_AVDD_REG_2
>> + - AD5940_ADC_INPUTP_TEMP
>> + - AD5940_ADC_INPUTP_VBIAS_CAP
>> + - AD5940_ADC_INPUTP_DE0
>> + - AD5940_ADC_INPUTP_SE0
>> + - AD5940_ADC_INPUTP_VREF_2V5_2
>> + - AD5940_ADC_INPUTP_VREF_1V82
>> + - AD5940_ADC_INPUTP_P_TEMP_N
>> + - AD5940_ADC_INPUTP_AIN4
>> + - AD5940_ADC_INPUTP_AIN6
>> + - AD5940_ADC_INPUTP_VZERO
>> + - AD5940_ADC_INPUTP_VBIAS0
>> + - AD5940_ADC_INPUTP_VCE0
>> + - AD5940_ADC_INPUTP_VRE0
>> + - AD5940_ADC_INPUTP_VCE0_2
>> + - AD5940_ADC_INPUTP_LPTIA
>> + - AD5940_ADC_INPUTP_AGND_REF
>> +
>> + - description: Negtive input channel
>> + - enum:
>> + # Negtive input candidates
>> + - AD5940_ADC_INPUTN_FLOATING
>> + - AD5940_ADC_INPUTN_HSTIA
>> + - AD5940_ADC_INPUTN_LPTIA
>> + - AD5940_ADC_INPUTN_AIN0
>> + - AD5940_ADC_INPUTN_AIN1
>> + - AD5940_ADC_INPUTN_AIN2
>> + - AD5940_ADC_INPUTN_AIN3
>> + - AD5940_ADC_INPUTN_VBIAS_CA8
>> + - AD5940_ADC_INPUTN_TEMP_N
>> + - AD5940_ADC_INPUTN_AIN4
>> + - AD5940_ADC_INPUTN_AIN6
>> + - AD5940_ADC_INPUTN_VZERO
>> + - AD5940_ADC_INPUTN_VBIAS0
>> + - AD5940_ADC_INPUTN_EXCITATION
>> +
>> + channel-name:
>> + description:
>> + Any string format name you would like to assign to this channel.
>> + maxItems: 1
>> +
>> + required:
>> + - reg
>> + - diff-channels
>> + - channel-name
>> +
>> +examples:
>> + - |
>> + ad5940: ad5940@0 {
>> + compatible = "adi,ad5940";
>> + reg = <0>;
>> + spi-max-frequency = <16000000>;
>> + vref-supply = <&adc_vref>;
>> + interrupt-parent = <&gpio>;
>> + interrupts = <24 2>;
>> +
>> + adi,interrupt-io = <0>;
>> +
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + channel@0 {
>> + reg = <0>;
>> + diff-channels = <AD5940_ADC_INPUTP_VCE0
>> + AD5940_ADC_INPUTN_VBIAS0>;
>> + channel-name = "Vce-Vbias";
>> + };
>> +
>> + channel@1 {
>> + reg = <1>;
>> + diff-channels = <AD5940_ADC_INPUTP_VRE0
>> + AD5940_ADC_INPUTN_VBIAS0>;
>> + channel-name = "Vre-Vbias";
>> + };
>> +
>> + channel@2 {
>> + reg = <2>;
>> + diff-channels = <AD5940_ADC_INPUTP_SE0
>> + AD5940_ADC_INPUTN_VBIAS0>;
>> + channel-name = "Vse-Vbias";
>> + };
>> +
>> + channel@3 {
>> + reg = <3>;
>> + diff-channels = <AD5940_ADC_INPUTP_DE0
>> + AD5940_ADC_INPUTN_VBIAS0>;
>> + channel-name = "Vde-Vbias";
>> + };
>> +
>> + channel@4 {
>> + reg = <4>;
>> + diff-channels = <AD5940_ADC_INPUTP_AIN0
>> + AD5940_ADC_INPUTN_VBIAS0>;
>> + channel-name = "ain0-Vbias";
>> + };
>> +
>> + channel@5 {
>> + reg = <5>;
>> + diff-channels = <AD5940_ADC_INPUTP_AIN1
>> + AD5940_ADC_INPUTN_VBIAS0>;
>> + channel-name = "ain1-Vbias";
>> + };
>> +
>> + channel@6 {
>> + reg = <6>;
>> + diff-channels = <AD5940_ADC_INPUTP_AIN2
>> + AD5940_ADC_INPUTN_VBIAS0>;
>> + channel-name = "ain2-Vbias";
>> + };
>> +
>> + channel@7 {
>> + reg = <7>;
>> + diff-channels = <AD5940_ADC_INPUTP_AIN3
>> + AD5940_ADC_INPUTN_VBIAS0>;
>> + channel-name = "ain3-Vbias";
>> + };
>> +
>> + channel@8 {
>> + reg = <8>;
>> + diff-channels = <AD5940_ADC_INPUTP_AIN4
>> + AD5940_ADC_INPUTN_VBIAS0>;
>> + channel-name = "ain4-Vbias";
>> + };
>> +
>> + channel@9 {
>> + reg = <9>;
>> + diff-channels = <AD5940_ADC_INPUTP_AIN6
>> + AD5940_ADC_INPUTN_VBIAS0>;
>> + channel-name = "ain6-Vbias";
>> + };
>> +
>> + channel@10 {
>> + reg = <10>;
>> + diff-channels = <AD5940_ADC_INPUTP_LPTIA_LP
>> + AD5940_ADC_INPUTN_LPTIA>;
>> + channel-name = "Low power TIA DC";
>> + };
>> +
>> + channel@11 {
>> + reg = <11>;
>> + diff-channels = <AD5940_ADC_INPUTP_LPTIA
>> + AD5940_ADC_INPUTN_LPTIA>;
>> + channel-name = "Low power TIA AC";
>> + };
>> +
>> + channel@12 {
>> + reg = <12>;
>> + diff-channels = <AD5940_ADC_INPUTP_HSTIA
>> + AD5940_ADC_INPUTN_HSTIA>;
>> + channel-name = "High Speed TIA";
>> + };
>> +
>> + channel@13 {
>> + reg = <13>;
>> + diff-channels = <AD5940_ADC_INPUTP_TEMP
>> + AD5940_ADC_INPUTN_VBIAS0>;
>> + channel-name = "Temperature";
>> + };
>> + };
>> diff --git a/include/dt-bindings/iio/adc/adi,ad5940.h b/include/dt-bindings/iio/adc/adi,ad5940.h
>> new file mode 100644
>> index 000000000000..c17826f2f654
>> --- /dev/null
>> +++ b/include/dt-bindings/iio/adc/adi,ad5940.h
>> @@ -0,0 +1,52 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * This header provides constants for configuring the AD5940 AFE
>> + */
>> +
>> +#ifndef _DT_BINDINGS_IIO_ADC_AD5940_H
>> +#define _DT_BINDINGS_IIO_ADC_AD5940_H
>> +
>> +#define AD5940_ADC_INPUTN_FLOATING 0
>> +#define AD5940_ADC_INPUTN_HSTIA 1
>> +#define AD5940_ADC_INPUTN_LPTIA 2
>> +#define AD5940_ADC_INPUTN_AIN0 4
>> +#define AD5940_ADC_INPUTN_AIN1 5
>> +#define AD5940_ADC_INPUTN_AIN2 6
>> +#define AD5940_ADC_INPUTN_AIN3 7
>> +#define AD5940_ADC_INPUTN_VBIAS_CA8 10
>> +#define AD5940_ADC_INPUTN_TEMP_N 11
>> +#define AD5940_ADC_INPUTN_AIN4 12
>> +#define AD5940_ADC_INPUTN_AIN6 14
>> +#define AD5940_ADC_INPUTN_VZERO 16
>> +#define AD5940_ADC_INPUTN_VBIAS0 17
>> +#define AD5940_ADC_INPUTN_EXCITATION 20
>> +
>> +#define AD5940_ADC_INPUTP_FLOATING 0
>> +#define AD5940_ADC_INPUTP_HSTIA 1
>> +#define AD5940_ADC_INPUTP_LPTIA_LP 2
>> +#define AD5940_ADC_INPUTP_AIN0 4
>> +#define AD5940_ADC_INPUTP_AIN1 5
>> +#define AD5940_ADC_INPUTP_AIN2 6
>> +#define AD5940_ADC_INPUTP_AIN3 7
>> +#define AD5940_ADC_INPUTP_AVDD_2 8
>> +#define AD5940_ADC_INPUTP_DVDD_2 9
>> +#define AD5940_ADC_INPUTP_AVDD_REG_2 10
>> +#define AD5940_ADC_INPUTP_TEMP 11
>> +#define AD5940_ADC_INPUTP_VBIAS_CAP 12
>> +#define AD5940_ADC_INPUTP_DE0 13
>> +#define AD5940_ADC_INPUTP_SE0 14
>> +#define AD5940_ADC_INPUTP_VREF_2V5_2 16
>> +#define AD5940_ADC_INPUTP_VREF_1V82 18
>> +#define AD5940_ADC_INPUTP_P_TEMP_N 19
>> +#define AD5940_ADC_INPUTP_AIN4 20
>> +#define AD5940_ADC_INPUTP_AIN6 22
>> +#define AD5940_ADC_INPUTP_VZERO 23
>> +#define AD5940_ADC_INPUTP_VBIAS0 24
>> +#define AD5940_ADC_INPUTP_VCE0 25
>> +#define AD5940_ADC_INPUTP_VRE0 26
>> +#define AD5940_ADC_INPUTP_VCE0_2 31
>> +#define AD5940_ADC_INPUTP_LPTIA 33
>> +#define AD5940_ADC_INPUTP_AGND_REF 35
>> +#define AD5940_ADC_INPUTP_EXCITATION 36
>> +
>> +#endif /* _DT_BINDINGS_IIO_ADC_AD5940 */
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] dt-bindings: iio: adc: add support for AD5940
2019-11-14 13:32 ` Song Qiang
@ 2019-11-14 14:01 ` Ardelean, Alexandru
2019-11-16 14:15 ` Jonathan Cameron
1 sibling, 0 replies; 13+ messages in thread
From: Ardelean, Alexandru @ 2019-11-14 14:01 UTC (permalink / raw)
To: jic23, songqiang1304521
Cc: Popa, Stefan Serban, broonie, mark.rutland, devicetree, lars,
knaack.h, Hennerich, Michael, pmeerw, lgirdwood, robh+dt,
linux-kernel, linux-iio
On Thu, 2019-11-14 at 21:32 +0800, Song Qiang wrote:
> [External]
>
>
>
> On 11/11/19 12:26 AM, Jonathan Cameron wrote:
> > On Fri, 8 Nov 2019 21:09:44 +0800
> > Song Qiang <songqiang1304521@gmail.com> wrote:
> >
> > > Add yaml devicetree description file and a header file for
> > > helping configure positive and negtive input of AD5940.
> > >
> > > Signed-off-by: Song Qiang <songqiang1304521@gmail.com>
> > Ouch. This is a very complex device, so I'm guessing this is the tip
> > of the iceberg when it comes to the eventual binding.
> > For reference of others this has a similarly complex DAC and
> > TIA + some excitation voltage generators (DDS).
> >
> > Anyhow, a few comments inline but I'll definitely be looking for
> > a dt maintainer input on this one.
> >
> > Thanks,
> >
> > Jonathan
> >
>
> Thanks, this is the first part support of the driver.
>
> > > ---
> > > .../bindings/iio/adc/adi,ad5940.yaml | 240
> > > ++++++++++++++++++
> > > include/dt-bindings/iio/adc/adi,ad5940.h | 52 ++++
> > > 2 files changed, 292 insertions(+)
> > > create mode 100644
> > > Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml
> > > create mode 100644 include/dt-bindings/iio/adc/adi,ad5940.h
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml
> > > b/Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml
> > > new file mode 100644
> > > index 000000000000..f7f034fdd8ec
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml
> > > @@ -0,0 +1,240 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > For new bindings, preference is to include a dual license
> > (GPL-2.0-only OR BSD-2-Clause)
> >
> > If Analog is fine doing this that would be great.
For the license:
Acked-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> >
>
> I'll consult my mentor about this.
>
> > > +# Copyright 2019 Analog Devices Inc.
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/bindings/iio/adc/adi,ad5940.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Analog Devices AD5940 Device Tree Bindings
> > > +
> > > +maintainers:
> > > + - Song Qiang <songqiang1304521@gmail.com>
> > > +
> > > +description: |
> > > + Analog Devices AD5940 High Precision, Impedance, and
> > > Electrochemical Front End.
> > > +
> > > https://www.analog.com/media/en/technical-documentation/data-sheets/AD5940.pdf
> > > +
> > > +properties:
> > > + compatible:
> > > + enum:
> > > + - adi,ad5940
> > > +
> > > + reg:
> > > + maxItems: 1
> > > +
> > > + vref-supply:
> > > + description:
> > > + The regulator to be used to supply the reference voltage.
> > > + maxItems: 1
> >
> > It's worth taking a look at similar patch reviews to pick up on things
> > that are common issues. Rob has pointed out a few times recently that
> > vref-supply can only ever have one item, so no need for maxItems.
> >
>
> That's right, thanks.
>
> > > +
> > > + adi,interrupt-io:
> > > + description:
> > > + Output GPIO index of interrupt controller of AD5940.
> > > + maxItems: 1
> >
> > I'm fairly sure an enum can only have one entry so don't think this is
> > needed.
> >
> > > + allOf:
> > > + - $ref: /schemas/types.yaml#/definitions/uint32
> > > + - enum: [0, 3, 6, 7]
> > > +
> > > + '#address-cells':
> > > + const: 1
> > > +
> > > + '#size-cells':
> > > + const: 0
> > > +
> > > +required:
> > > + - compatible
> > > + - reg
> > > + - interrupts
> > > + - adi,interrupt-io
> > > +
> > > +patternProperties:
> > > + # 'channel@0-255'
> >
> > Really? That is a lot of channels. Superficially it looks like a much
> > smaller number of possibilities from the datasheet.
> >
>
> This device has some positive inputs and some negative inputs. A channel
> here is a combination of one positive input and one negative input.
> These channels I listed in examples are only suggested combinations in
> the datasheet, while other combinations are all possible. So I was
> thinking to not limit the total count of channels here. I failed to find
> examples of doing this kind of stuff in the tree.
>
> yours,
> Song Qiang
>
> > > + "^channel@([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])$":
> > > + type: object
> > > + description: |
> > > + Represents the external channels which are connected to the
> > > ADC.
> > > + See Documentation/devicetree/bindings/iio/adc/adc.txt.
> > > + properties:
> > > + reg:
> > > + description:
> > > + Index of this channel, must be starting from 0.
> > > + maxItems: 1
> > > +
> > > + diff-channels:
> > > + description:
> > > + Positive input and negtive input of the ADC buffer of this
> > > channel.
> > > + Input candidates are defined in include/dt-
> > > bindings/iio/adc/adi,ad5940.h.
> > > + minItems: 2
> > > + maxItems: 2
> > > + items:
> > > + - description: Positive input channel
> > > + - enum:
> > > + - AD5940_ADC_INPUTP_EXCITATION
> > > + - AD5940_ADC_INPUTP_FLOATING
> > > + - AD5940_ADC_INPUTP_HSTIA
> > > + - AD5940_ADC_INPUTP_LPTIA_LP
> > > + - AD5940_ADC_INPUTP_AIN0
> > > + - AD5940_ADC_INPUTP_AIN1
> > > + - AD5940_ADC_INPUTP_AIN2
> > > + - AD5940_ADC_INPUTP_AIN3
> > > + - AD5940_ADC_INPUTP_AVDD_2
> > > + - AD5940_ADC_INPUTP_DVDD_2
> > > + - AD5940_ADC_INPUTP_AVDD_REG_2
> > > + - AD5940_ADC_INPUTP_TEMP
> > > + - AD5940_ADC_INPUTP_VBIAS_CAP
> > > + - AD5940_ADC_INPUTP_DE0
> > > + - AD5940_ADC_INPUTP_SE0
> > > + - AD5940_ADC_INPUTP_VREF_2V5_2
> > > + - AD5940_ADC_INPUTP_VREF_1V82
> > > + - AD5940_ADC_INPUTP_P_TEMP_N
> > > + - AD5940_ADC_INPUTP_AIN4
> > > + - AD5940_ADC_INPUTP_AIN6
> > > + - AD5940_ADC_INPUTP_VZERO
> > > + - AD5940_ADC_INPUTP_VBIAS0
> > > + - AD5940_ADC_INPUTP_VCE0
> > > + - AD5940_ADC_INPUTP_VRE0
> > > + - AD5940_ADC_INPUTP_VCE0_2
> > > + - AD5940_ADC_INPUTP_LPTIA
> > > + - AD5940_ADC_INPUTP_AGND_REF
> > > +
> > > + - description: Negtive input channel
> > > + - enum:
> > > + # Negtive input candidates
> > > + - AD5940_ADC_INPUTN_FLOATING
> > > + - AD5940_ADC_INPUTN_HSTIA
> > > + - AD5940_ADC_INPUTN_LPTIA
> > > + - AD5940_ADC_INPUTN_AIN0
> > > + - AD5940_ADC_INPUTN_AIN1
> > > + - AD5940_ADC_INPUTN_AIN2
> > > + - AD5940_ADC_INPUTN_AIN3
> > > + - AD5940_ADC_INPUTN_VBIAS_CA8
> > > + - AD5940_ADC_INPUTN_TEMP_N
> > > + - AD5940_ADC_INPUTN_AIN4
> > > + - AD5940_ADC_INPUTN_AIN6
> > > + - AD5940_ADC_INPUTN_VZERO
> > > + - AD5940_ADC_INPUTN_VBIAS0
> > > + - AD5940_ADC_INPUTN_EXCITATION
> > > +
> > > + channel-name:
> > > + description:
> > > + Any string format name you would like to assign to this
> > > channel.
> > > + maxItems: 1
> > > +
> > > + required:
> > > + - reg
> > > + - diff-channels
> > > + - channel-name
> > > +
> > > +examples:
> > > + - |
> > > + ad5940: ad5940@0 {
> > > + compatible = "adi,ad5940";
> > > + reg = <0>;
> > > + spi-max-frequency = <16000000>;
> > > + vref-supply = <&adc_vref>;
> > > + interrupt-parent = <&gpio>;
> > > + interrupts = <24 2>;
> > > +
> > > + adi,interrupt-io = <0>;
> > > +
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + channel@0 {
> > > + reg = <0>;
> > > + diff-channels = <AD5940_ADC_INPUTP_VCE0
> > > + AD5940_ADC_INPUTN_VBIAS0>;
> > > + channel-name = "Vce-Vbias";
> > > + };
> > > +
> > > + channel@1 {
> > > + reg = <1>;
> > > + diff-channels = <AD5940_ADC_INPUTP_VRE0
> > > + AD5940_ADC_INPUTN_VBIAS0>;
> > > + channel-name = "Vre-Vbias";
> > > + };
> > > +
> > > + channel@2 {
> > > + reg = <2>;
> > > + diff-channels = <AD5940_ADC_INPUTP_SE0
> > > + AD5940_ADC_INPUTN_VBIAS0>;
> > > + channel-name = "Vse-Vbias";
> > > + };
> > > +
> > > + channel@3 {
> > > + reg = <3>;
> > > + diff-channels = <AD5940_ADC_INPUTP_DE0
> > > + AD5940_ADC_INPUTN_VBIAS0>;
> > > + channel-name = "Vde-Vbias";
> > > + };
> > > +
> > > + channel@4 {
> > > + reg = <4>;
> > > + diff-channels = <AD5940_ADC_INPUTP_AIN0
> > > + AD5940_ADC_INPUTN_VBIAS0>;
> > > + channel-name = "ain0-Vbias";
> > > + };
> > > +
> > > + channel@5 {
> > > + reg = <5>;
> > > + diff-channels = <AD5940_ADC_INPUTP_AIN1
> > > + AD5940_ADC_INPUTN_VBIAS0>;
> > > + channel-name = "ain1-Vbias";
> > > + };
> > > +
> > > + channel@6 {
> > > + reg = <6>;
> > > + diff-channels = <AD5940_ADC_INPUTP_AIN2
> > > + AD5940_ADC_INPUTN_VBIAS0>;
> > > + channel-name = "ain2-Vbias";
> > > + };
> > > +
> > > + channel@7 {
> > > + reg = <7>;
> > > + diff-channels = <AD5940_ADC_INPUTP_AIN3
> > > + AD5940_ADC_INPUTN_VBIAS0>;
> > > + channel-name = "ain3-Vbias";
> > > + };
> > > +
> > > + channel@8 {
> > > + reg = <8>;
> > > + diff-channels = <AD5940_ADC_INPUTP_AIN4
> > > + AD5940_ADC_INPUTN_VBIAS0>;
> > > + channel-name = "ain4-Vbias";
> > > + };
> > > +
> > > + channel@9 {
> > > + reg = <9>;
> > > + diff-channels = <AD5940_ADC_INPUTP_AIN6
> > > + AD5940_ADC_INPUTN_VBIAS0>;
> > > + channel-name = "ain6-Vbias";
> > > + };
> > > +
> > > + channel@10 {
> > > + reg = <10>;
> > > + diff-channels = <AD5940_ADC_INPUTP_LPTIA_LP
> > > + AD5940_ADC_INPUTN_LPTIA>;
> > > + channel-name = "Low power TIA DC";
> > > + };
> > > +
> > > + channel@11 {
> > > + reg = <11>;
> > > + diff-channels = <AD5940_ADC_INPUTP_LPTIA
> > > + AD5940_ADC_INPUTN_LPTIA>;
> > > + channel-name = "Low power TIA AC";
> > > + };
> > > +
> > > + channel@12 {
> > > + reg = <12>;
> > > + diff-channels = <AD5940_ADC_INPUTP_HSTIA
> > > + AD5940_ADC_INPUTN_HSTIA>;
> > > + channel-name = "High Speed TIA";
> > > + };
> > > +
> > > + channel@13 {
> > > + reg = <13>;
> > > + diff-channels = <AD5940_ADC_INPUTP_TEMP
> > > + AD5940_ADC_INPUTN_VBIAS0>;
> > > + channel-name = "Temperature";
> > > + };
> > > + };
> > > diff --git a/include/dt-bindings/iio/adc/adi,ad5940.h b/include/dt-
> > > bindings/iio/adc/adi,ad5940.h
> > > new file mode 100644
> > > index 000000000000..c17826f2f654
> > > --- /dev/null
> > > +++ b/include/dt-bindings/iio/adc/adi,ad5940.h
> > > @@ -0,0 +1,52 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * This header provides constants for configuring the AD5940 AFE
> > > + */
> > > +
> > > +#ifndef _DT_BINDINGS_IIO_ADC_AD5940_H
> > > +#define _DT_BINDINGS_IIO_ADC_AD5940_H
> > > +
> > > +#define AD5940_ADC_INPUTN_FLOATING 0
> > > +#define AD5940_ADC_INPUTN_HSTIA 1
> > > +#define AD5940_ADC_INPUTN_LPTIA 2
> > > +#define AD5940_ADC_INPUTN_AIN0 4
> > > +#define AD5940_ADC_INPUTN_AIN1 5
> > > +#define AD5940_ADC_INPUTN_AIN2 6
> > > +#define AD5940_ADC_INPUTN_AIN3 7
> > > +#define AD5940_ADC_INPUTN_VBIAS_CA8 10
> > > +#define AD5940_ADC_INPUTN_TEMP_N 11
> > > +#define AD5940_ADC_INPUTN_AIN4 12
> > > +#define AD5940_ADC_INPUTN_AIN6 14
> > > +#define AD5940_ADC_INPUTN_VZERO 16
> > > +#define AD5940_ADC_INPUTN_VBIAS0 17
> > > +#define AD5940_ADC_INPUTN_EXCITATION 20
> > > +
> > > +#define AD5940_ADC_INPUTP_FLOATING 0
> > > +#define AD5940_ADC_INPUTP_HSTIA 1
> > > +#define AD5940_ADC_INPUTP_LPTIA_LP 2
> > > +#define AD5940_ADC_INPUTP_AIN0 4
> > > +#define AD5940_ADC_INPUTP_AIN1 5
> > > +#define AD5940_ADC_INPUTP_AIN2 6
> > > +#define AD5940_ADC_INPUTP_AIN3 7
> > > +#define AD5940_ADC_INPUTP_AVDD_2 8
> > > +#define AD5940_ADC_INPUTP_DVDD_2 9
> > > +#define AD5940_ADC_INPUTP_AVDD_REG_2 10
> > > +#define AD5940_ADC_INPUTP_TEMP 11
> > > +#define AD5940_ADC_INPUTP_VBIAS_CAP 12
> > > +#define AD5940_ADC_INPUTP_DE0 13
> > > +#define AD5940_ADC_INPUTP_SE0 14
> > > +#define AD5940_ADC_INPUTP_VREF_2V5_2 16
> > > +#define AD5940_ADC_INPUTP_VREF_1V82 18
> > > +#define AD5940_ADC_INPUTP_P_TEMP_N 19
> > > +#define AD5940_ADC_INPUTP_AIN4 20
> > > +#define AD5940_ADC_INPUTP_AIN6 22
> > > +#define AD5940_ADC_INPUTP_VZERO 23
> > > +#define AD5940_ADC_INPUTP_VBIAS0 24
> > > +#define AD5940_ADC_INPUTP_VCE0 25
> > > +#define AD5940_ADC_INPUTP_VRE0 26
> > > +#define AD5940_ADC_INPUTP_VCE0_2 31
> > > +#define AD5940_ADC_INPUTP_LPTIA 33
> > > +#define AD5940_ADC_INPUTP_AGND_REF 35
> > > +#define AD5940_ADC_INPUTP_EXCITATION 36
> > > +
> > > +#endif /* _DT_BINDINGS_IIO_ADC_AD5940 */
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] iio: adc: add driver support for AD5940
2019-11-10 16:55 ` Jonathan Cameron
@ 2019-11-14 14:05 ` Song Qiang
2019-11-16 14:08 ` Jonathan Cameron
0 siblings, 1 reply; 13+ messages in thread
From: Song Qiang @ 2019-11-14 14:05 UTC (permalink / raw)
To: Jonathan Cameron
Cc: lars, Michael.Hennerich, stefan.popa, knaack.h, pmeerw,
lgirdwood, broonie, robh+dt, mark.rutland, linux-iio, devicetree,
linux-kernel
On 11/11/19 12:55 AM, Jonathan Cameron wrote:
> On Fri, 8 Nov 2019 21:09:45 +0800
> Song Qiang <songqiang1304521@gmail.com> wrote:
>
>> The AD5940 is a high precision, low power analog front end (AFE)
>> designed for portable applications that require high precision,
>> electrochemical-based measurement techniques, such as amper-
>> ometric, voltammetric, or impedance measurements.
>>
>> Datasheet:
>> Link: https://www.analog.com/media/en/technical-documentation/data-sheets/AD5940.pdf
>>
>> Signed-off-by: Song Qiang <songqiang1304521@gmail.com>
>
> Nice little driver on the whole. I'm guessing this is very much the 'first of
> many' patches to support this complex part. Makes sense and keeps it manageable
> from a review point of view.
>
> A few comments inline.
>
> Thanks,
>
> Jonathan
>
>> ---
>> .../ABI/testing/sysfs-bus-iio-adc-ad5940 | 7 +
>> drivers/iio/adc/Kconfig | 10 +
>> drivers/iio/adc/Makefile | 1 +
>> drivers/iio/adc/ad5940.c | 679 ++++++++++++++++++
>> 4 files changed, 697 insertions(+)
>> create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad5940
>> create mode 100644 drivers/iio/adc/ad5940.c
>>
...
>> +static int ad5940_write_reg_mask(struct ad5940_state *st, u16 addr,
>> + u32 mask, u32 data)
>> +{
>> + u32 temp;
>> + int ret;
>> +
>> + ret = ad5940_read_reg(st, addr, &temp);
>> + if (ret < 0)
>> + return ret;
>> +
>> + temp &= ~mask;
>> + temp |= data;
>> +
>> + return ad5940_write_reg(st, addr, temp);
>> +}
>> +
>> +static ssize_t ad5940_read_info(struct iio_dev *indio_dev,
>> + uintptr_t private,
>> + const struct iio_chan_spec *chan,
>> + char *buf)
>> +{
>> + struct ad5940_state *st = iio_priv(indio_dev);
>> +
>> + switch ((u32)private) {
>> + case AD5940_CHANNEL_NAME:
>
> What is the logic here in this magic define?
>
Do you mean this is not necessary? In this driver, 'ad5940_read_info' is
only used in name reading, but I was thinking maybe there will be some
other stuff to be reading from this in the future.
>> + return sprintf(buf, "%s\n",
>> + st->channel_config[chan->address].channel_name);
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +static const struct iio_chan_spec_ext_info ad4590_ext_info[] = {
>> + {
>> + .name = "name",
>
> This is defining new ABI. I'm not necessarily against doing so but
> it needs a separate documentation patch so that it's easy to discuss.
> Documentation/ABI/testing/sysfs-bus-iio
>
> We might want to think of a similar naming to the label we recently
> added for the device itself.
>
I'll look into this and see if I can use 'label' instead.
>> + .read = ad5940_read_info,
>> + .private = AD5940_CHANNEL_NAME,
>> + .shared = IIO_SEPARATE,
>> + },
>> + { },
>> +};
>> +
>> +static const struct iio_chan_spec ad5940_channel_template = {
>> + .type = IIO_VOLTAGE,
>> + .differential = 1,
>> + .indexed = 1,
>> + .ext_info = ad4590_ext_info,
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
>> +};
>> +
>> +static int ad5940_clear_ready(struct ad5940_state *st)
>> +{
>> + int ret;
>> +
>> + ret = ad5940_write_reg_mask(st, AD5940_REG_AFECON,
>> + AD5940_AFECON_ADCCONV_MSK,
>> + AD5940_AFECON_ADCCONV_DIS);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return ad5940_write_reg(st, AD5940_REG_INTCLR, AD5940_INTC_ADC);
>> +}
>> +
>> +static irqreturn_t ad5940_irq_handler(int irq, void *private)
>> +{
>> + struct ad5940_state *st = private;
>> + int ret;
>> +
>> + ret = ad5940_clear_ready(st);
>> + if (ret < 0)
>> + return ret;
>> +
>> + complete(&st->complete);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int ad5940_scan_direct(struct ad5940_state *st, u32 mux, int *val)
>> +{
>> + int ret;
>> + u32 result;
>> +
>> + mutex_lock(&st->lock);
>> + ret = ad5940_write_reg_mask(st, AD5940_REG_ADCCON,
>> + AD5940_ADCCON_MUX_MSK, mux);
>> + if (ret < 0)
>> + goto unlock_return;
>> +
>> + reinit_completion(&st->complete);
>> + ret = ad5940_write_reg_mask(st, AD5940_REG_AFECON,
>> + AD5940_AFECON_ADCCONV_MSK,
>> + AD5940_AFECON_ADCCONV_EN);
>> + if (ret < 0)
>> + goto unlock_return;
>> +
>> + ret = wait_for_completion_timeout(&st->complete,
>> + msecs_to_jiffies(1000));
>> + if (!ret) {
>> + ad5940_clear_ready(st);
>> + ret = -ETIMEDOUT;
>> + goto unlock_return;
>> + }
>> +
>> + ret = ad5940_read_reg(st, AD5940_REG_ADCDAT, &result);
>> + mutex_unlock(&st->lock);
>> + if (ret < 0)
>> + return ret;
>
> As you already have the unlock_return below on this occasion I would
> move the if (ret) inside the lock so all similar errors take a simpler
> exit path. If this was the only case and you didn't have the handling
> I would agree with how you have done it.
>
>> + *val = result & 0xffff;
>> +
>> + return 0;
>> +
>> +unlock_return:
>> + mutex_unlock(&st->lock);
>> + return ret;
>> +}
>> +
>> +static int ad5940_read_raw(struct iio_dev *indio_dev,
>> + const struct iio_chan_spec *chan, int *val, int *val2, long info)
>> +{
>> + struct ad5940_state *st = iio_priv(indio_dev);
>> + int ret;
>> +
>> + switch (info) {
>> + case IIO_CHAN_INFO_RAW:
>> + ret = ad5940_scan_direct(st,
>> + st->channel_config[chan->address].ain,
>> + val);
>> + if (ret < 0)
>> + return ret;
>> + else
>> + return IIO_VAL_INT;
> return IIO_VAL_INT;
>
> Cleaner to only have the error path indented.
> if / else kind of implies some 'equality' of the two
> options.
>
>> + case IIO_CHAN_INFO_SCALE:
>> + *val = st->vref_mv;
>> + *val2 = 16;
>> + return IIO_VAL_FRACTIONAL_LOG2;
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +static const struct iio_info ad5940_info = {
>> + .read_raw = &ad5940_read_raw,
>> +};
>> +
>> +int cmp_u8(const void *a, const void *b)
>> +{
>> + return (*(u8 *)a - *(u8 *)b);
>> +}
>> +
>> +static int ad5940_check_channel_indexes(struct device *dev, u32 *ain)
>> +{
>> + const u8 channel_p[] = {
>> + 0, 1, 2, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 16, 18, 19,
>> + 20, 22, 23, 24, 25, 26, 31, 33, 35, 36
>> + };
>> + const u8 channel_n[] = {
>> + 0, 1, 2, 4, 5, 6, 7, 10, 11, 12, 14, 16, 17, 20
>> + };
>> + u8 *index;
>> +
>> + index = (u8 *) bsearch(&ain[0], channel_p, ARRAY_SIZE(channel_p),
>> + sizeof(u8), cmp_u8);
>> + if (!index) {
>> + dev_err(dev, "Positive input index not found.\n");
>> + return -EINVAL;
>> + }
>> +
>> + index = (u8 *) bsearch(&ain[1], channel_n, ARRAY_SIZE(channel_n),
>> + sizeof(u8), cmp_u8);
>> + if (!index) {
>> + dev_err(dev, "negtive input index not found.\n");
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int ad5940_of_parse_channel_config(struct iio_dev *indio_dev,
>> + struct device_node *np)
>> +{
>> + struct ad5940_state *st = iio_priv(indio_dev);
>> + struct iio_chan_spec *chan;
>> + struct device_node *child;
>> + u32 channel, ain[2];
>> + int ret;
>> +
>> + st->num_channels = of_get_available_child_count(np);
>> + if (!st->num_channels) {
>> + dev_err(indio_dev->dev.parent, "no channel children\n");
>> + return -ENODEV;
>> + }
>> +
>> + chan = devm_kcalloc(indio_dev->dev.parent, st->num_channels,
>> + sizeof(*chan), GFP_KERNEL);
>> + if (!chan)
>> + return -ENOMEM;
>> +
>> + st->channel_config = devm_kcalloc(indio_dev->dev.parent,
>> + st->num_channels,
>> + sizeof(*st->channel_config),
>> + GFP_KERNEL);
>> + if (!st->channel_config)
>> + return -ENOMEM;
>> +
>> + indio_dev->channels = chan;
>> + indio_dev->num_channels = st->num_channels;
>> +
>> + for_each_available_child_of_node(np, child) {
>> + ret = of_property_read_u32(child, "reg", &channel);
>> + if (ret)
>> + goto err;
>> +
>> + ret = of_property_read_u32_array(child, "diff-channels",
>> + ain, 2);
>> + if (ret)
>> + goto err;
>> +
>> + ret = of_property_read_string(child, "channel-name",
>> + &st->channel_config[channel].channel_name);
>> + if (ret)
>> + st->channel_config[channel].channel_name = "none-name";
>
> You have this as required I think in the dt properties. If that is the case then
> enforce it and refuse to load the driver if not supplied. Otherwise change
> the dt docs to make it optional (which is probably better)
>
I prefer to have name required because a channel here is a combination
of some input and some output. Without name, we will have to look into
dt to see which input and which output is used in this channel.
>> +
>> + ret = ad5940_check_channel_indexes(indio_dev->dev.parent, ain);
>> + if (ret) {
>> + dev_err(indio_dev->dev.parent,
>> + "some input channel index does not exist: %d, %d, %d",
>> + channel, ain[0], ain[1]);
>> + goto err;
>> + }
>> +
>> + st->channel_config[channel].ain = AD5940_CHANNEL_AINP(ain[0]) |
>> + AD5940_CHANNEL_AINN(ain[1]);
>> +
>> + *chan = ad5940_channel_template;
>> + chan->address = channel;
>> + chan->scan_index = channel;
>> + chan->channel = ain[0];
>> + chan->channel2 = ain[1];
>> +
>> + chan++;
>> + }
>> +
>> + return 0;
>> +err:
>> + of_node_put(child);
>> +
>> + return ret;
>> +}
>> +
>> +static int ad5940_config_polarity(struct ad5940_state *st, u32 polarity)
>> +{
>> + u32 val;
>> +
>> + if (polarity == IRQF_TRIGGER_RISING)
>> + val = AD5940_INTCPOL_POS;
>> + else
>> + val = AD5940_INTCPOL_NEG;
>> +
>> + return ad5940_write_reg_mask(st, AD5940_REG_INTCPOL,
>> + AD5940_INTCPOL_MSK, val);
>> +}
>> +
>> +static int ad5940_config_int_io(struct ad5940_state *st, u8 int_io)
>> +{
>> + int ret = 0;
>> +
>> + if (int_io == 3)
>
> Switch statement preferred for matches like this.
>
>> + ret = ad5940_write_reg_mask(st, AD5940_REG_GP0CON,
>> + AD5940_GP0CON_3_MSK,
>> + AD5940_GP0CON_3_INT);
>> + else if (int_io == 6)
>> + ret = ad5940_write_reg_mask(st, AD5940_REG_GP0CON,
>> + AD5940_GP0CON_6_MSK,
>> + AD5940_GP0CON_6_INT);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return ad5940_write_reg(st, AD5940_REG_GP0OEN, BIT(int_io));
>> +}
>> +
>> +static const u32 ad5940_powerup_setting[][2] = {
>
> Hmm. This is not good practice when we have docs for the values.
> If we can provide a breakdown into what is being set that would be
> great. I can't find docs immediately for some of these however...
>
We have docs for some of them, but not for all. I got some of the init
register values from Analog's example code, which didn't explain much.
I'll add comment to these values as much as I can.
yours,
Song Qiang
>> + { 0x0908, 0x02c9 },
>> + { 0x0c08, 0x206c },
>> + { 0x21f0, 0x0010 },
>
> This one is is simply saying only 1 repeat conversion for example.
> Add some defines and you can make that clear.
>
>> + { 0x0410, 0x02c9 },
>> + { 0x0a28, 0x0009 },
>> + { 0x238c, 0x0104 },
>> + { 0x0a04, 0x4859 },
>> + { 0x0a04, 0xf27b },
>> + { 0x0a00, 0x8009 },
>> + { 0x22f0, 0x0000 },
>> + { 0x2230, 0xde87a5af },
>> + { 0x2250, 0x103f },
>> + { 0x22b0, 0x203c },
>> + { 0x2230, 0xde87a5a0 },
>> +};
>> +
...
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] dt-bindings: iio: adc: add support for AD5940
2019-11-14 1:28 ` Rob Herring
@ 2019-11-14 17:06 ` Song Qiang
2019-11-16 14:17 ` Jonathan Cameron
0 siblings, 1 reply; 13+ messages in thread
From: Song Qiang @ 2019-11-14 17:06 UTC (permalink / raw)
To: Rob Herring
Cc: Lars-Peter Clausen, Michael Hennerich, Stefan Popa,
Jonathan Cameron, Hartmut Knaack, Peter Meerwald, Liam Girdwood,
Mark Brown, Mark Rutland, open list:IIO SUBSYSTEM AND DRIVERS,
devicetree, linux-kernel
Thanks, I'll see how to use 'lable' to pass name of channels, other
problems will be fixed in the next patch.
yours,
Song Qiang
On 11/14/19 9:28 AM, Rob Herring wrote:
> On Fri, Nov 8, 2019 at 7:09 AM Song Qiang <songqiang1304521@gmail.com> wrote:
>>
>> Add yaml devicetree description file and a header file for
>> helping configure positive and negtive input of AD5940.
>>
>> Signed-off-by: Song Qiang <songqiang1304521@gmail.com>
>> ---
>> .../bindings/iio/adc/adi,ad5940.yaml | 240 ++++++++++++++++++
>> include/dt-bindings/iio/adc/adi,ad5940.h | 52 ++++
>> 2 files changed, 292 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml
>> create mode 100644 include/dt-bindings/iio/adc/adi,ad5940.h
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml
>> new file mode 100644
>> index 000000000000..f7f034fdd8ec
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml
>> @@ -0,0 +1,240 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright 2019 Analog Devices Inc.
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/bindings/iio/adc/adi,ad5940.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Analog Devices AD5940 Device Tree Bindings
>> +
>> +maintainers:
>> + - Song Qiang <songqiang1304521@gmail.com>
>> +
>> +description: |
>> + Analog Devices AD5940 High Precision, Impedance, and Electrochemical Front End.
>> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD5940.pdf
>> +
>> +properties:
>> + compatible:
>> + enum:
>> + - adi,ad5940
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + vref-supply:
>> + description:
>> + The regulator to be used to supply the reference voltage.
>> + maxItems: 1
>> +
>> + adi,interrupt-io:
>> + description:
>> + Output GPIO index of interrupt controller of AD5940.
>> + maxItems: 1
>> + allOf:
>> + - $ref: /schemas/types.yaml#/definitions/uint32
>> + - enum: [0, 3, 6, 7]
>> +
>> + '#address-cells':
>> + const: 1
>> +
>> + '#size-cells':
>> + const: 0
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - interrupts
>> + - adi,interrupt-io
>> +
>> +patternProperties:
>> + # 'channel@0-255'
>> + "^channel@([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])$":
>> + type: object
>> + description: |
>> + Represents the external channels which are connected to the ADC.
>> + See Documentation/devicetree/bindings/iio/adc/adc.txt.
>> + properties:
>> + reg:
>> + description:
>> + Index of this channel, must be starting from 0.
>> + maxItems: 1
>
> Drop maxItems and do:
>
> items:
> - minimum: 0
> maximum: 255
>
>> +
>> + diff-channels:
>> + description:
>> + Positive input and negtive input of the ADC buffer of this channel.
>> + Input candidates are defined in include/dt-bindings/iio/adc/adi,ad5940.h.
>> + minItems: 2
>> + maxItems: 2
>> + items:
>> + - description: Positive input channel
>> + - enum:
>> + - AD5940_ADC_INPUTP_EXCITATION
>
> You can't have defines in the schema. minimum/maximum might work better here.
>
>> + - AD5940_ADC_INPUTP_FLOATING
>> + - AD5940_ADC_INPUTP_HSTIA
>> + - AD5940_ADC_INPUTP_LPTIA_LP
>> + - AD5940_ADC_INPUTP_AIN0
>> + - AD5940_ADC_INPUTP_AIN1
>> + - AD5940_ADC_INPUTP_AIN2
>> + - AD5940_ADC_INPUTP_AIN3
>> + - AD5940_ADC_INPUTP_AVDD_2
>> + - AD5940_ADC_INPUTP_DVDD_2
>> + - AD5940_ADC_INPUTP_AVDD_REG_2
>> + - AD5940_ADC_INPUTP_TEMP
>> + - AD5940_ADC_INPUTP_VBIAS_CAP
>> + - AD5940_ADC_INPUTP_DE0
>> + - AD5940_ADC_INPUTP_SE0
>> + - AD5940_ADC_INPUTP_VREF_2V5_2
>> + - AD5940_ADC_INPUTP_VREF_1V82
>> + - AD5940_ADC_INPUTP_P_TEMP_N
>> + - AD5940_ADC_INPUTP_AIN4
>> + - AD5940_ADC_INPUTP_AIN6
>> + - AD5940_ADC_INPUTP_VZERO
>> + - AD5940_ADC_INPUTP_VBIAS0
>> + - AD5940_ADC_INPUTP_VCE0
>> + - AD5940_ADC_INPUTP_VRE0
>> + - AD5940_ADC_INPUTP_VCE0_2
>> + - AD5940_ADC_INPUTP_LPTIA
>> + - AD5940_ADC_INPUTP_AGND_REF
>> +
>> + - description: Negtive input channel
>> + - enum:
>> + # Negtive input candidates
>> + - AD5940_ADC_INPUTN_FLOATING
>> + - AD5940_ADC_INPUTN_HSTIA
>> + - AD5940_ADC_INPUTN_LPTIA
>> + - AD5940_ADC_INPUTN_AIN0
>> + - AD5940_ADC_INPUTN_AIN1
>> + - AD5940_ADC_INPUTN_AIN2
>> + - AD5940_ADC_INPUTN_AIN3
>> + - AD5940_ADC_INPUTN_VBIAS_CA8
>> + - AD5940_ADC_INPUTN_TEMP_N
>> + - AD5940_ADC_INPUTN_AIN4
>> + - AD5940_ADC_INPUTN_AIN6
>> + - AD5940_ADC_INPUTN_VZERO
>> + - AD5940_ADC_INPUTN_VBIAS0
>> + - AD5940_ADC_INPUTN_EXCITATION
>
> You've defined that diff-channels is 4 items. I don't think that's
> what you want. Each enum and description should be under a single '-'.
>
>> +
>> + channel-name:
>
> Perhaps standard property 'label' should be used here. Seems like a
> common thing.
>
>> + description:
>> + Any string format name you would like to assign to this channel.
>> + maxItems: 1
>> +
>> + required:
>> + - reg
>> + - diff-channels
>> + - channel-name
>> +
>> +examples:
>> + - |
>> + ad5940: ad5940@0 {
>> + compatible = "adi,ad5940";
>> + reg = <0>;
>> + spi-max-frequency = <16000000>;
>> + vref-supply = <&adc_vref>;
>> + interrupt-parent = <&gpio>;
>> + interrupts = <24 2>;
>> +
>> + adi,interrupt-io = <0>;
>> +
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + channel@0 {
>> + reg = <0>;
>> + diff-channels = <AD5940_ADC_INPUTP_VCE0
>> + AD5940_ADC_INPUTN_VBIAS0>;
>
> You'll need the header included for this to build. Run 'make
> dt_binding_check' on this.
>
>> + channel-name = "Vce-Vbias";
>> + };
>> +
>> + channel@1 {
>> + reg = <1>;
>> + diff-channels = <AD5940_ADC_INPUTP_VRE0
>> + AD5940_ADC_INPUTN_VBIAS0>;
>> + channel-name = "Vre-Vbias";
>> + };
>> +
>> + channel@2 {
>> + reg = <2>;
>> + diff-channels = <AD5940_ADC_INPUTP_SE0
>> + AD5940_ADC_INPUTN_VBIAS0>;
>> + channel-name = "Vse-Vbias";
>> + };
>> +
>> + channel@3 {
>> + reg = <3>;
>> + diff-channels = <AD5940_ADC_INPUTP_DE0
>> + AD5940_ADC_INPUTN_VBIAS0>;
>> + channel-name = "Vde-Vbias";
>> + };
>> +
>> + channel@4 {
>> + reg = <4>;
>> + diff-channels = <AD5940_ADC_INPUTP_AIN0
>> + AD5940_ADC_INPUTN_VBIAS0>;
>> + channel-name = "ain0-Vbias";
>> + };
>> +
>> + channel@5 {
>> + reg = <5>;
>> + diff-channels = <AD5940_ADC_INPUTP_AIN1
>> + AD5940_ADC_INPUTN_VBIAS0>;
>> + channel-name = "ain1-Vbias";
>> + };
>> +
>> + channel@6 {
>> + reg = <6>;
>> + diff-channels = <AD5940_ADC_INPUTP_AIN2
>> + AD5940_ADC_INPUTN_VBIAS0>;
>> + channel-name = "ain2-Vbias";
>> + };
>> +
>> + channel@7 {
>> + reg = <7>;
>> + diff-channels = <AD5940_ADC_INPUTP_AIN3
>> + AD5940_ADC_INPUTN_VBIAS0>;
>> + channel-name = "ain3-Vbias";
>> + };
>> +
>> + channel@8 {
>> + reg = <8>;
>> + diff-channels = <AD5940_ADC_INPUTP_AIN4
>> + AD5940_ADC_INPUTN_VBIAS0>;
>> + channel-name = "ain4-Vbias";
>> + };
>> +
>> + channel@9 {
>> + reg = <9>;
>> + diff-channels = <AD5940_ADC_INPUTP_AIN6
>> + AD5940_ADC_INPUTN_VBIAS0>;
>> + channel-name = "ain6-Vbias";
>> + };
>> +
>> + channel@10 {
>> + reg = <10>;
>> + diff-channels = <AD5940_ADC_INPUTP_LPTIA_LP
>> + AD5940_ADC_INPUTN_LPTIA>;
>> + channel-name = "Low power TIA DC";
>> + };
>> +
>> + channel@11 {
>> + reg = <11>;
>> + diff-channels = <AD5940_ADC_INPUTP_LPTIA
>> + AD5940_ADC_INPUTN_LPTIA>;
>> + channel-name = "Low power TIA AC";
>> + };
>> +
>> + channel@12 {
>> + reg = <12>;
>> + diff-channels = <AD5940_ADC_INPUTP_HSTIA
>> + AD5940_ADC_INPUTN_HSTIA>;
>> + channel-name = "High Speed TIA";
>> + };
>> +
>> + channel@13 {
>> + reg = <13>;
>> + diff-channels = <AD5940_ADC_INPUTP_TEMP
>> + AD5940_ADC_INPUTN_VBIAS0>;
>> + channel-name = "Temperature";
>> + };
>> + };
>> diff --git a/include/dt-bindings/iio/adc/adi,ad5940.h b/include/dt-bindings/iio/adc/adi,ad5940.h
>> new file mode 100644
>> index 000000000000..c17826f2f654
>> --- /dev/null
>> +++ b/include/dt-bindings/iio/adc/adi,ad5940.h
>> @@ -0,0 +1,52 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * This header provides constants for configuring the AD5940 AFE
>> + */
>> +
>> +#ifndef _DT_BINDINGS_IIO_ADC_AD5940_H
>> +#define _DT_BINDINGS_IIO_ADC_AD5940_H
>> +
>> +#define AD5940_ADC_INPUTN_FLOATING 0
>> +#define AD5940_ADC_INPUTN_HSTIA 1
>> +#define AD5940_ADC_INPUTN_LPTIA 2
>> +#define AD5940_ADC_INPUTN_AIN0 4
>> +#define AD5940_ADC_INPUTN_AIN1 5
>> +#define AD5940_ADC_INPUTN_AIN2 6
>> +#define AD5940_ADC_INPUTN_AIN3 7
>> +#define AD5940_ADC_INPUTN_VBIAS_CA8 10
>> +#define AD5940_ADC_INPUTN_TEMP_N 11
>> +#define AD5940_ADC_INPUTN_AIN4 12
>> +#define AD5940_ADC_INPUTN_AIN6 14
>> +#define AD5940_ADC_INPUTN_VZERO 16
>> +#define AD5940_ADC_INPUTN_VBIAS0 17
>> +#define AD5940_ADC_INPUTN_EXCITATION 20
>> +
>> +#define AD5940_ADC_INPUTP_FLOATING 0
>> +#define AD5940_ADC_INPUTP_HSTIA 1
>> +#define AD5940_ADC_INPUTP_LPTIA_LP 2
>> +#define AD5940_ADC_INPUTP_AIN0 4
>> +#define AD5940_ADC_INPUTP_AIN1 5
>> +#define AD5940_ADC_INPUTP_AIN2 6
>> +#define AD5940_ADC_INPUTP_AIN3 7
>> +#define AD5940_ADC_INPUTP_AVDD_2 8
>> +#define AD5940_ADC_INPUTP_DVDD_2 9
>> +#define AD5940_ADC_INPUTP_AVDD_REG_2 10
>> +#define AD5940_ADC_INPUTP_TEMP 11
>> +#define AD5940_ADC_INPUTP_VBIAS_CAP 12
>> +#define AD5940_ADC_INPUTP_DE0 13
>> +#define AD5940_ADC_INPUTP_SE0 14
>> +#define AD5940_ADC_INPUTP_VREF_2V5_2 16
>> +#define AD5940_ADC_INPUTP_VREF_1V82 18
>> +#define AD5940_ADC_INPUTP_P_TEMP_N 19
>> +#define AD5940_ADC_INPUTP_AIN4 20
>> +#define AD5940_ADC_INPUTP_AIN6 22
>> +#define AD5940_ADC_INPUTP_VZERO 23
>> +#define AD5940_ADC_INPUTP_VBIAS0 24
>> +#define AD5940_ADC_INPUTP_VCE0 25
>> +#define AD5940_ADC_INPUTP_VRE0 26
>> +#define AD5940_ADC_INPUTP_VCE0_2 31
>> +#define AD5940_ADC_INPUTP_LPTIA 33
>> +#define AD5940_ADC_INPUTP_AGND_REF 35
>> +#define AD5940_ADC_INPUTP_EXCITATION 36
>> +
>> +#endif /* _DT_BINDINGS_IIO_ADC_AD5940 */
>> --
>> 2.17.1
>>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] iio: adc: add driver support for AD5940
2019-11-14 14:05 ` Song Qiang
@ 2019-11-16 14:08 ` Jonathan Cameron
0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2019-11-16 14:08 UTC (permalink / raw)
To: Song Qiang
Cc: lars, Michael.Hennerich, stefan.popa, knaack.h, pmeerw,
lgirdwood, broonie, robh+dt, mark.rutland, linux-iio, devicetree,
linux-kernel
On Thu, 14 Nov 2019 22:05:54 +0800
Song Qiang <songqiang1304521@gmail.com> wrote:
> >> +static ssize_t ad5940_read_info(struct iio_dev *indio_dev,
> >> + uintptr_t private,
> >> + const struct iio_chan_spec *chan,
> >> + char *buf)
> >> +{
> >> + struct ad5940_state *st = iio_priv(indio_dev);
> >> +
> >> + switch ((u32)private) {
> >> + case AD5940_CHANNEL_NAME:
> >
> > What is the logic here in this magic define?
> >
>
> Do you mean this is not necessary? In this driver, 'ad5940_read_info' is
> only used in name reading, but I was thinking maybe there will be some
> other stuff to be reading from this in the future.
That might happen, but you may also extend this by providing more
callbacks. Certainly don't put code in place to support things
that might be added. Keep it simple.
>
> >> + return sprintf(buf, "%s\n",
> >> + st->channel_config[chan->address].channel_name);
> >> + default:
> >> + return -EINVAL;
> >> + }
> >> +}
> >> +
...
> >> +static int ad5940_of_parse_channel_config(struct iio_dev *indio_dev,
> >> + struct device_node *np)
> >> +{
> >> + struct ad5940_state *st = iio_priv(indio_dev);
> >> + struct iio_chan_spec *chan;
> >> + struct device_node *child;
> >> + u32 channel, ain[2];
> >> + int ret;
> >> +
> >> + st->num_channels = of_get_available_child_count(np);
> >> + if (!st->num_channels) {
> >> + dev_err(indio_dev->dev.parent, "no channel children\n");
> >> + return -ENODEV;
> >> + }
> >> +
> >> + chan = devm_kcalloc(indio_dev->dev.parent, st->num_channels,
> >> + sizeof(*chan), GFP_KERNEL);
> >> + if (!chan)
> >> + return -ENOMEM;
> >> +
> >> + st->channel_config = devm_kcalloc(indio_dev->dev.parent,
> >> + st->num_channels,
> >> + sizeof(*st->channel_config),
> >> + GFP_KERNEL);
> >> + if (!st->channel_config)
> >> + return -ENOMEM;
> >> +
> >> + indio_dev->channels = chan;
> >> + indio_dev->num_channels = st->num_channels;
> >> +
> >> + for_each_available_child_of_node(np, child) {
> >> + ret = of_property_read_u32(child, "reg", &channel);
> >> + if (ret)
> >> + goto err;
> >> +
> >> + ret = of_property_read_u32_array(child, "diff-channels",
> >> + ain, 2);
> >> + if (ret)
> >> + goto err;
> >> +
> >> + ret = of_property_read_string(child, "channel-name",
> >> + &st->channel_config[channel].channel_name);
> >> + if (ret)
> >> + st->channel_config[channel].channel_name = "none-name";
> >
> > You have this as required I think in the dt properties. If that is the case then
> > enforce it and refuse to load the driver if not supplied. Otherwise change
> > the dt docs to make it optional (which is probably better)
> >
>
> I prefer to have name required because a channel here is a combination
> of some input and some output. Without name, we will have to look into
> dt to see which input and which output is used in this channel.
Then don't make it optional.
>
> >> +
> >> + ret = ad5940_check_channel_indexes(indio_dev->dev.parent, ain);
> >> + if (ret) {
> >> + dev_err(indio_dev->dev.parent,
> >> + "some input channel index does not exist: %d, %d, %d",
> >> + channel, ain[0], ain[1]);
> >> + goto err;
> >> + }
> >> +
> >> + st->channel_config[channel].ain = AD5940_CHANNEL_AINP(ain[0]) |
> >> + AD5940_CHANNEL_AINN(ain[1]);
> >> +
> >> + *chan = ad5940_channel_template;
> >> + chan->address = channel;
> >> + chan->scan_index = channel;
> >> + chan->channel = ain[0];
> >> + chan->channel2 = ain[1];
> >> +
> >> + chan++;
> >> + }
> >> +
> >> + return 0;
> >> +err:
> >> + of_node_put(child);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static int ad5940_config_polarity(struct ad5940_state *st, u32 polarity)
> >> +{
> >> + u32 val;
> >> +
> >> + if (polarity == IRQF_TRIGGER_RISING)
> >> + val = AD5940_INTCPOL_POS;
> >> + else
> >> + val = AD5940_INTCPOL_NEG;
> >> +
> >> + return ad5940_write_reg_mask(st, AD5940_REG_INTCPOL,
> >> + AD5940_INTCPOL_MSK, val);
> >> +}
> >> +
> >> +static int ad5940_config_int_io(struct ad5940_state *st, u8 int_io)
> >> +{
> >> + int ret = 0;
> >> +
> >> + if (int_io == 3)
> >
> > Switch statement preferred for matches like this.
> >
> >> + ret = ad5940_write_reg_mask(st, AD5940_REG_GP0CON,
> >> + AD5940_GP0CON_3_MSK,
> >> + AD5940_GP0CON_3_INT);
> >> + else if (int_io == 6)
> >> + ret = ad5940_write_reg_mask(st, AD5940_REG_GP0CON,
> >> + AD5940_GP0CON_6_MSK,
> >> + AD5940_GP0CON_6_INT);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + return ad5940_write_reg(st, AD5940_REG_GP0OEN, BIT(int_io));
> >> +}
> >> +
> >> +static const u32 ad5940_powerup_setting[][2] = {
> >
> > Hmm. This is not good practice when we have docs for the values.
> > If we can provide a breakdown into what is being set that would be
> > great. I can't find docs immediately for some of these however...
> >
>
> We have docs for some of them, but not for all. I got some of the init
> register values from Analog's example code, which didn't explain much.
> I'll add comment to these values as much as I can.
Great.
Jonathan
>
> yours,
> Song Qiang
>
> >> + { 0x0908, 0x02c9 },
> >> + { 0x0c08, 0x206c },
> >> + { 0x21f0, 0x0010 },
> >
> > This one is is simply saying only 1 repeat conversion for example.
> > Add some defines and you can make that clear.
> >
> >> + { 0x0410, 0x02c9 },
> >> + { 0x0a28, 0x0009 },
> >> + { 0x238c, 0x0104 },
> >> + { 0x0a04, 0x4859 },
> >> + { 0x0a04, 0xf27b },
> >> + { 0x0a00, 0x8009 },
> >> + { 0x22f0, 0x0000 },
> >> + { 0x2230, 0xde87a5af },
> >> + { 0x2250, 0x103f },
> >> + { 0x22b0, 0x203c },
> >> + { 0x2230, 0xde87a5a0 },
> >> +};
> >> +
>
> ...
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] dt-bindings: iio: adc: add support for AD5940
2019-11-14 13:32 ` Song Qiang
2019-11-14 14:01 ` Ardelean, Alexandru
@ 2019-11-16 14:15 ` Jonathan Cameron
1 sibling, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2019-11-16 14:15 UTC (permalink / raw)
To: Song Qiang
Cc: lars, Michael.Hennerich, stefan.popa, knaack.h, pmeerw,
lgirdwood, broonie, robh+dt, mark.rutland, linux-iio, devicetree,
linux-kernel
On Thu, 14 Nov 2019 21:32:06 +0800
Song Qiang <songqiang1304521@gmail.com> wrote:
> On 11/11/19 12:26 AM, Jonathan Cameron wrote:
> > On Fri, 8 Nov 2019 21:09:44 +0800
> > Song Qiang <songqiang1304521@gmail.com> wrote:
> >
> >> Add yaml devicetree description file and a header file for
> >> helping configure positive and negtive input of AD5940.
> >>
> >> Signed-off-by: Song Qiang <songqiang1304521@gmail.com>
> > Ouch. This is a very complex device, so I'm guessing this is the tip
> > of the iceberg when it comes to the eventual binding.
> > For reference of others this has a similarly complex DAC and
> > TIA + some excitation voltage generators (DDS).
> >
> > Anyhow, a few comments inline but I'll definitely be looking for
> > a dt maintainer input on this one.
> >
> > Thanks,
> >
> > Jonathan
> >
>
> Thanks, this is the first part support of the driver.
>
> >> ---
> >> .../bindings/iio/adc/adi,ad5940.yaml | 240 ++++++++++++++++++
> >> include/dt-bindings/iio/adc/adi,ad5940.h | 52 ++++
> >> 2 files changed, 292 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml
> >> create mode 100644 include/dt-bindings/iio/adc/adi,ad5940.h
> >>
> >> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml
> >> new file mode 100644
> >> index 000000000000..f7f034fdd8ec
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml
> >> @@ -0,0 +1,240 @@
> >> +# SPDX-License-Identifier: GPL-2.0
> > For new bindings, preference is to include a dual license
> > (GPL-2.0-only OR BSD-2-Clause)
> >
> > If Analog is fine doing this that would be great.
> >
>
> I'll consult my mentor about this.
>
> >> +# Copyright 2019 Analog Devices Inc.
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/bindings/iio/adc/adi,ad5940.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Analog Devices AD5940 Device Tree Bindings
> >> +
> >> +maintainers:
> >> + - Song Qiang <songqiang1304521@gmail.com>
> >> +
> >> +description: |
> >> + Analog Devices AD5940 High Precision, Impedance, and Electrochemical Front End.
> >> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD5940.pdf
> >> +
> >> +properties:
> >> + compatible:
> >> + enum:
> >> + - adi,ad5940
> >> +
> >> + reg:
> >> + maxItems: 1
> >> +
> >> + vref-supply:
> >> + description:
> >> + The regulator to be used to supply the reference voltage.
> >> + maxItems: 1
> >
> > It's worth taking a look at similar patch reviews to pick up on things
> > that are common issues. Rob has pointed out a few times recently that
> > vref-supply can only ever have one item, so no need for maxItems.
> >
>
> That's right, thanks.
>
> >> +
> >> + adi,interrupt-io:
> >> + description:
> >> + Output GPIO index of interrupt controller of AD5940.
> >> + maxItems: 1
> >
> > I'm fairly sure an enum can only have one entry so don't think this is
> > needed.
> >
> >> + allOf:
> >> + - $ref: /schemas/types.yaml#/definitions/uint32
> >> + - enum: [0, 3, 6, 7]
> >> +
> >> + '#address-cells':
> >> + const: 1
> >> +
> >> + '#size-cells':
> >> + const: 0
> >> +
> >> +required:
> >> + - compatible
> >> + - reg
> >> + - interrupts
> >> + - adi,interrupt-io
> >> +
> >> +patternProperties:
> >> + # 'channel@0-255'
> >
> > Really? That is a lot of channels. Superficially it looks like a much
> > smaller number of possibilities from the datasheet.
> >
>
> This device has some positive inputs and some negative inputs. A channel
> here is a combination of one positive input and one negative input.
> These channels I listed in examples are only suggested combinations in
> the datasheet, while other combinations are all possible. So I was
> thinking to not limit the total count of channels here. I failed to find
> examples of doing this kind of stuff in the tree.
As we are putting the controls for these in DT, we are making the claim
that they are inherently part of the 'hardware'. The fully flexibility
probably doesn't make sense even if the hardware supports it.
I'd be tempted to just not specify a limit at all, but rely on the type
of 'reg' to provide an ultimate limit. That gets rid of a lot
of the complexity in the binding.
Reality is that for a device like this you aren't going to have more
channels than postive inputs because the vast majority of the time
there is little reason to measure a given input against multiple
references.
Thanks,
Jonathan
>
> yours,
> Song Qiang
>
> >> + "^channel@([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])$":
> >> + type: object
> >> + description: |
> >> + Represents the external channels which are connected to the ADC.
> >> + See Documentation/devicetree/bindings/iio/adc/adc.txt.
> >> + properties:
> >> + reg:
> >> + description:
> >> + Index of this channel, must be starting from 0.
> >> + maxItems: 1
> >> +
> >> + diff-channels:
> >> + description:
> >> + Positive input and negtive input of the ADC buffer of this channel.
> >> + Input candidates are defined in include/dt-bindings/iio/adc/adi,ad5940.h.
> >> + minItems: 2
> >> + maxItems: 2
> >> + items:
> >> + - description: Positive input channel
> >> + - enum:
> >> + - AD5940_ADC_INPUTP_EXCITATION
> >> + - AD5940_ADC_INPUTP_FLOATING
> >> + - AD5940_ADC_INPUTP_HSTIA
> >> + - AD5940_ADC_INPUTP_LPTIA_LP
> >> + - AD5940_ADC_INPUTP_AIN0
> >> + - AD5940_ADC_INPUTP_AIN1
> >> + - AD5940_ADC_INPUTP_AIN2
> >> + - AD5940_ADC_INPUTP_AIN3
> >> + - AD5940_ADC_INPUTP_AVDD_2
> >> + - AD5940_ADC_INPUTP_DVDD_2
> >> + - AD5940_ADC_INPUTP_AVDD_REG_2
> >> + - AD5940_ADC_INPUTP_TEMP
> >> + - AD5940_ADC_INPUTP_VBIAS_CAP
> >> + - AD5940_ADC_INPUTP_DE0
> >> + - AD5940_ADC_INPUTP_SE0
> >> + - AD5940_ADC_INPUTP_VREF_2V5_2
> >> + - AD5940_ADC_INPUTP_VREF_1V82
> >> + - AD5940_ADC_INPUTP_P_TEMP_N
> >> + - AD5940_ADC_INPUTP_AIN4
> >> + - AD5940_ADC_INPUTP_AIN6
> >> + - AD5940_ADC_INPUTP_VZERO
> >> + - AD5940_ADC_INPUTP_VBIAS0
> >> + - AD5940_ADC_INPUTP_VCE0
> >> + - AD5940_ADC_INPUTP_VRE0
> >> + - AD5940_ADC_INPUTP_VCE0_2
> >> + - AD5940_ADC_INPUTP_LPTIA
> >> + - AD5940_ADC_INPUTP_AGND_REF
> >> +
> >> + - description: Negtive input channel
> >> + - enum:
> >> + # Negtive input candidates
> >> + - AD5940_ADC_INPUTN_FLOATING
> >> + - AD5940_ADC_INPUTN_HSTIA
> >> + - AD5940_ADC_INPUTN_LPTIA
> >> + - AD5940_ADC_INPUTN_AIN0
> >> + - AD5940_ADC_INPUTN_AIN1
> >> + - AD5940_ADC_INPUTN_AIN2
> >> + - AD5940_ADC_INPUTN_AIN3
> >> + - AD5940_ADC_INPUTN_VBIAS_CA8
> >> + - AD5940_ADC_INPUTN_TEMP_N
> >> + - AD5940_ADC_INPUTN_AIN4
> >> + - AD5940_ADC_INPUTN_AIN6
> >> + - AD5940_ADC_INPUTN_VZERO
> >> + - AD5940_ADC_INPUTN_VBIAS0
> >> + - AD5940_ADC_INPUTN_EXCITATION
> >> +
> >> + channel-name:
> >> + description:
> >> + Any string format name you would like to assign to this channel.
> >> + maxItems: 1
> >> +
> >> + required:
> >> + - reg
> >> + - diff-channels
> >> + - channel-name
> >> +
> >> +examples:
> >> + - |
> >> + ad5940: ad5940@0 {
> >> + compatible = "adi,ad5940";
> >> + reg = <0>;
> >> + spi-max-frequency = <16000000>;
> >> + vref-supply = <&adc_vref>;
> >> + interrupt-parent = <&gpio>;
> >> + interrupts = <24 2>;
> >> +
> >> + adi,interrupt-io = <0>;
> >> +
> >> + #address-cells = <1>;
> >> + #size-cells = <0>;
> >> +
> >> + channel@0 {
> >> + reg = <0>;
> >> + diff-channels = <AD5940_ADC_INPUTP_VCE0
> >> + AD5940_ADC_INPUTN_VBIAS0>;
> >> + channel-name = "Vce-Vbias";
> >> + };
> >> +
> >> + channel@1 {
> >> + reg = <1>;
> >> + diff-channels = <AD5940_ADC_INPUTP_VRE0
> >> + AD5940_ADC_INPUTN_VBIAS0>;
> >> + channel-name = "Vre-Vbias";
> >> + };
> >> +
> >> + channel@2 {
> >> + reg = <2>;
> >> + diff-channels = <AD5940_ADC_INPUTP_SE0
> >> + AD5940_ADC_INPUTN_VBIAS0>;
> >> + channel-name = "Vse-Vbias";
> >> + };
> >> +
> >> + channel@3 {
> >> + reg = <3>;
> >> + diff-channels = <AD5940_ADC_INPUTP_DE0
> >> + AD5940_ADC_INPUTN_VBIAS0>;
> >> + channel-name = "Vde-Vbias";
> >> + };
> >> +
> >> + channel@4 {
> >> + reg = <4>;
> >> + diff-channels = <AD5940_ADC_INPUTP_AIN0
> >> + AD5940_ADC_INPUTN_VBIAS0>;
> >> + channel-name = "ain0-Vbias";
> >> + };
> >> +
> >> + channel@5 {
> >> + reg = <5>;
> >> + diff-channels = <AD5940_ADC_INPUTP_AIN1
> >> + AD5940_ADC_INPUTN_VBIAS0>;
> >> + channel-name = "ain1-Vbias";
> >> + };
> >> +
> >> + channel@6 {
> >> + reg = <6>;
> >> + diff-channels = <AD5940_ADC_INPUTP_AIN2
> >> + AD5940_ADC_INPUTN_VBIAS0>;
> >> + channel-name = "ain2-Vbias";
> >> + };
> >> +
> >> + channel@7 {
> >> + reg = <7>;
> >> + diff-channels = <AD5940_ADC_INPUTP_AIN3
> >> + AD5940_ADC_INPUTN_VBIAS0>;
> >> + channel-name = "ain3-Vbias";
> >> + };
> >> +
> >> + channel@8 {
> >> + reg = <8>;
> >> + diff-channels = <AD5940_ADC_INPUTP_AIN4
> >> + AD5940_ADC_INPUTN_VBIAS0>;
> >> + channel-name = "ain4-Vbias";
> >> + };
> >> +
> >> + channel@9 {
> >> + reg = <9>;
> >> + diff-channels = <AD5940_ADC_INPUTP_AIN6
> >> + AD5940_ADC_INPUTN_VBIAS0>;
> >> + channel-name = "ain6-Vbias";
> >> + };
> >> +
> >> + channel@10 {
> >> + reg = <10>;
> >> + diff-channels = <AD5940_ADC_INPUTP_LPTIA_LP
> >> + AD5940_ADC_INPUTN_LPTIA>;
> >> + channel-name = "Low power TIA DC";
> >> + };
> >> +
> >> + channel@11 {
> >> + reg = <11>;
> >> + diff-channels = <AD5940_ADC_INPUTP_LPTIA
> >> + AD5940_ADC_INPUTN_LPTIA>;
> >> + channel-name = "Low power TIA AC";
> >> + };
> >> +
> >> + channel@12 {
> >> + reg = <12>;
> >> + diff-channels = <AD5940_ADC_INPUTP_HSTIA
> >> + AD5940_ADC_INPUTN_HSTIA>;
> >> + channel-name = "High Speed TIA";
> >> + };
> >> +
> >> + channel@13 {
> >> + reg = <13>;
> >> + diff-channels = <AD5940_ADC_INPUTP_TEMP
> >> + AD5940_ADC_INPUTN_VBIAS0>;
> >> + channel-name = "Temperature";
> >> + };
> >> + };
> >> diff --git a/include/dt-bindings/iio/adc/adi,ad5940.h b/include/dt-bindings/iio/adc/adi,ad5940.h
> >> new file mode 100644
> >> index 000000000000..c17826f2f654
> >> --- /dev/null
> >> +++ b/include/dt-bindings/iio/adc/adi,ad5940.h
> >> @@ -0,0 +1,52 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/*
> >> + * This header provides constants for configuring the AD5940 AFE
> >> + */
> >> +
> >> +#ifndef _DT_BINDINGS_IIO_ADC_AD5940_H
> >> +#define _DT_BINDINGS_IIO_ADC_AD5940_H
> >> +
> >> +#define AD5940_ADC_INPUTN_FLOATING 0
> >> +#define AD5940_ADC_INPUTN_HSTIA 1
> >> +#define AD5940_ADC_INPUTN_LPTIA 2
> >> +#define AD5940_ADC_INPUTN_AIN0 4
> >> +#define AD5940_ADC_INPUTN_AIN1 5
> >> +#define AD5940_ADC_INPUTN_AIN2 6
> >> +#define AD5940_ADC_INPUTN_AIN3 7
> >> +#define AD5940_ADC_INPUTN_VBIAS_CA8 10
> >> +#define AD5940_ADC_INPUTN_TEMP_N 11
> >> +#define AD5940_ADC_INPUTN_AIN4 12
> >> +#define AD5940_ADC_INPUTN_AIN6 14
> >> +#define AD5940_ADC_INPUTN_VZERO 16
> >> +#define AD5940_ADC_INPUTN_VBIAS0 17
> >> +#define AD5940_ADC_INPUTN_EXCITATION 20
> >> +
> >> +#define AD5940_ADC_INPUTP_FLOATING 0
> >> +#define AD5940_ADC_INPUTP_HSTIA 1
> >> +#define AD5940_ADC_INPUTP_LPTIA_LP 2
> >> +#define AD5940_ADC_INPUTP_AIN0 4
> >> +#define AD5940_ADC_INPUTP_AIN1 5
> >> +#define AD5940_ADC_INPUTP_AIN2 6
> >> +#define AD5940_ADC_INPUTP_AIN3 7
> >> +#define AD5940_ADC_INPUTP_AVDD_2 8
> >> +#define AD5940_ADC_INPUTP_DVDD_2 9
> >> +#define AD5940_ADC_INPUTP_AVDD_REG_2 10
> >> +#define AD5940_ADC_INPUTP_TEMP 11
> >> +#define AD5940_ADC_INPUTP_VBIAS_CAP 12
> >> +#define AD5940_ADC_INPUTP_DE0 13
> >> +#define AD5940_ADC_INPUTP_SE0 14
> >> +#define AD5940_ADC_INPUTP_VREF_2V5_2 16
> >> +#define AD5940_ADC_INPUTP_VREF_1V82 18
> >> +#define AD5940_ADC_INPUTP_P_TEMP_N 19
> >> +#define AD5940_ADC_INPUTP_AIN4 20
> >> +#define AD5940_ADC_INPUTP_AIN6 22
> >> +#define AD5940_ADC_INPUTP_VZERO 23
> >> +#define AD5940_ADC_INPUTP_VBIAS0 24
> >> +#define AD5940_ADC_INPUTP_VCE0 25
> >> +#define AD5940_ADC_INPUTP_VRE0 26
> >> +#define AD5940_ADC_INPUTP_VCE0_2 31
> >> +#define AD5940_ADC_INPUTP_LPTIA 33
> >> +#define AD5940_ADC_INPUTP_AGND_REF 35
> >> +#define AD5940_ADC_INPUTP_EXCITATION 36
> >> +
> >> +#endif /* _DT_BINDINGS_IIO_ADC_AD5940 */
> >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] dt-bindings: iio: adc: add support for AD5940
2019-11-14 17:06 ` Song Qiang
@ 2019-11-16 14:17 ` Jonathan Cameron
0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2019-11-16 14:17 UTC (permalink / raw)
To: Song Qiang
Cc: Rob Herring, Lars-Peter Clausen, Michael Hennerich, Stefan Popa,
Hartmut Knaack, Peter Meerwald, Liam Girdwood, Mark Brown,
Mark Rutland, open list:IIO SUBSYSTEM AND DRIVERS, devicetree,
linux-kernel
On Fri, 15 Nov 2019 01:06:32 +0800
Song Qiang <songqiang1304521@gmail.com> wrote:
> Thanks, I'll see how to use 'lable' to pass name of channels, other
> problems will be fixed in the next patch.
For a channel, it will be a new thing. We can introduce it in a
similar fashion to you did for channel-name but ultimately I think we
want to make it a standard part of the IIO core.
However, for either to happen the key thing is a documentation patch
describing it so that we can get the discussion going!
Thanks,
Jonathan
>
> yours,
> Song Qiang
>
> On 11/14/19 9:28 AM, Rob Herring wrote:
> > On Fri, Nov 8, 2019 at 7:09 AM Song Qiang <songqiang1304521@gmail.com> wrote:
> >>
> >> Add yaml devicetree description file and a header file for
> >> helping configure positive and negtive input of AD5940.
> >>
> >> Signed-off-by: Song Qiang <songqiang1304521@gmail.com>
> >> ---
> >> .../bindings/iio/adc/adi,ad5940.yaml | 240 ++++++++++++++++++
> >> include/dt-bindings/iio/adc/adi,ad5940.h | 52 ++++
> >> 2 files changed, 292 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml
> >> create mode 100644 include/dt-bindings/iio/adc/adi,ad5940.h
> >>
> >> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml
> >> new file mode 100644
> >> index 000000000000..f7f034fdd8ec
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad5940.yaml
> >> @@ -0,0 +1,240 @@
> >> +# SPDX-License-Identifier: GPL-2.0
> >> +# Copyright 2019 Analog Devices Inc.
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/bindings/iio/adc/adi,ad5940.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Analog Devices AD5940 Device Tree Bindings
> >> +
> >> +maintainers:
> >> + - Song Qiang <songqiang1304521@gmail.com>
> >> +
> >> +description: |
> >> + Analog Devices AD5940 High Precision, Impedance, and Electrochemical Front End.
> >> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD5940.pdf
> >> +
> >> +properties:
> >> + compatible:
> >> + enum:
> >> + - adi,ad5940
> >> +
> >> + reg:
> >> + maxItems: 1
> >> +
> >> + vref-supply:
> >> + description:
> >> + The regulator to be used to supply the reference voltage.
> >> + maxItems: 1
> >> +
> >> + adi,interrupt-io:
> >> + description:
> >> + Output GPIO index of interrupt controller of AD5940.
> >> + maxItems: 1
> >> + allOf:
> >> + - $ref: /schemas/types.yaml#/definitions/uint32
> >> + - enum: [0, 3, 6, 7]
> >> +
> >> + '#address-cells':
> >> + const: 1
> >> +
> >> + '#size-cells':
> >> + const: 0
> >> +
> >> +required:
> >> + - compatible
> >> + - reg
> >> + - interrupts
> >> + - adi,interrupt-io
> >> +
> >> +patternProperties:
> >> + # 'channel@0-255'
> >> + "^channel@([0-9]|[1-9][0-9]|1[0-9][0-9]|2[0-4][0-9]|25[0-5])$":
> >> + type: object
> >> + description: |
> >> + Represents the external channels which are connected to the ADC.
> >> + See Documentation/devicetree/bindings/iio/adc/adc.txt.
> >> + properties:
> >> + reg:
> >> + description:
> >> + Index of this channel, must be starting from 0.
> >> + maxItems: 1
> >
> > Drop maxItems and do:
> >
> > items:
> > - minimum: 0
> > maximum: 255
> >
> >> +
> >> + diff-channels:
> >> + description:
> >> + Positive input and negtive input of the ADC buffer of this channel.
> >> + Input candidates are defined in include/dt-bindings/iio/adc/adi,ad5940.h.
> >> + minItems: 2
> >> + maxItems: 2
> >> + items:
> >> + - description: Positive input channel
> >> + - enum:
> >> + - AD5940_ADC_INPUTP_EXCITATION
> >
> > You can't have defines in the schema. minimum/maximum might work better here.
> >
> >> + - AD5940_ADC_INPUTP_FLOATING
> >> + - AD5940_ADC_INPUTP_HSTIA
> >> + - AD5940_ADC_INPUTP_LPTIA_LP
> >> + - AD5940_ADC_INPUTP_AIN0
> >> + - AD5940_ADC_INPUTP_AIN1
> >> + - AD5940_ADC_INPUTP_AIN2
> >> + - AD5940_ADC_INPUTP_AIN3
> >> + - AD5940_ADC_INPUTP_AVDD_2
> >> + - AD5940_ADC_INPUTP_DVDD_2
> >> + - AD5940_ADC_INPUTP_AVDD_REG_2
> >> + - AD5940_ADC_INPUTP_TEMP
> >> + - AD5940_ADC_INPUTP_VBIAS_CAP
> >> + - AD5940_ADC_INPUTP_DE0
> >> + - AD5940_ADC_INPUTP_SE0
> >> + - AD5940_ADC_INPUTP_VREF_2V5_2
> >> + - AD5940_ADC_INPUTP_VREF_1V82
> >> + - AD5940_ADC_INPUTP_P_TEMP_N
> >> + - AD5940_ADC_INPUTP_AIN4
> >> + - AD5940_ADC_INPUTP_AIN6
> >> + - AD5940_ADC_INPUTP_VZERO
> >> + - AD5940_ADC_INPUTP_VBIAS0
> >> + - AD5940_ADC_INPUTP_VCE0
> >> + - AD5940_ADC_INPUTP_VRE0
> >> + - AD5940_ADC_INPUTP_VCE0_2
> >> + - AD5940_ADC_INPUTP_LPTIA
> >> + - AD5940_ADC_INPUTP_AGND_REF
> >> +
> >> + - description: Negtive input channel
> >> + - enum:
> >> + # Negtive input candidates
> >> + - AD5940_ADC_INPUTN_FLOATING
> >> + - AD5940_ADC_INPUTN_HSTIA
> >> + - AD5940_ADC_INPUTN_LPTIA
> >> + - AD5940_ADC_INPUTN_AIN0
> >> + - AD5940_ADC_INPUTN_AIN1
> >> + - AD5940_ADC_INPUTN_AIN2
> >> + - AD5940_ADC_INPUTN_AIN3
> >> + - AD5940_ADC_INPUTN_VBIAS_CA8
> >> + - AD5940_ADC_INPUTN_TEMP_N
> >> + - AD5940_ADC_INPUTN_AIN4
> >> + - AD5940_ADC_INPUTN_AIN6
> >> + - AD5940_ADC_INPUTN_VZERO
> >> + - AD5940_ADC_INPUTN_VBIAS0
> >> + - AD5940_ADC_INPUTN_EXCITATION
> >
> > You've defined that diff-channels is 4 items. I don't think that's
> > what you want. Each enum and description should be under a single '-'.
> >
> >> +
> >> + channel-name:
> >
> > Perhaps standard property 'label' should be used here. Seems like a
> > common thing.
> >
> >> + description:
> >> + Any string format name you would like to assign to this channel.
> >> + maxItems: 1
> >> +
> >> + required:
> >> + - reg
> >> + - diff-channels
> >> + - channel-name
> >> +
> >> +examples:
> >> + - |
> >> + ad5940: ad5940@0 {
> >> + compatible = "adi,ad5940";
> >> + reg = <0>;
> >> + spi-max-frequency = <16000000>;
> >> + vref-supply = <&adc_vref>;
> >> + interrupt-parent = <&gpio>;
> >> + interrupts = <24 2>;
> >> +
> >> + adi,interrupt-io = <0>;
> >> +
> >> + #address-cells = <1>;
> >> + #size-cells = <0>;
> >> +
> >> + channel@0 {
> >> + reg = <0>;
> >> + diff-channels = <AD5940_ADC_INPUTP_VCE0
> >> + AD5940_ADC_INPUTN_VBIAS0>;
> >
> > You'll need the header included for this to build. Run 'make
> > dt_binding_check' on this.
> >
> >> + channel-name = "Vce-Vbias";
> >> + };
> >> +
> >> + channel@1 {
> >> + reg = <1>;
> >> + diff-channels = <AD5940_ADC_INPUTP_VRE0
> >> + AD5940_ADC_INPUTN_VBIAS0>;
> >> + channel-name = "Vre-Vbias";
> >> + };
> >> +
> >> + channel@2 {
> >> + reg = <2>;
> >> + diff-channels = <AD5940_ADC_INPUTP_SE0
> >> + AD5940_ADC_INPUTN_VBIAS0>;
> >> + channel-name = "Vse-Vbias";
> >> + };
> >> +
> >> + channel@3 {
> >> + reg = <3>;
> >> + diff-channels = <AD5940_ADC_INPUTP_DE0
> >> + AD5940_ADC_INPUTN_VBIAS0>;
> >> + channel-name = "Vde-Vbias";
> >> + };
> >> +
> >> + channel@4 {
> >> + reg = <4>;
> >> + diff-channels = <AD5940_ADC_INPUTP_AIN0
> >> + AD5940_ADC_INPUTN_VBIAS0>;
> >> + channel-name = "ain0-Vbias";
> >> + };
> >> +
> >> + channel@5 {
> >> + reg = <5>;
> >> + diff-channels = <AD5940_ADC_INPUTP_AIN1
> >> + AD5940_ADC_INPUTN_VBIAS0>;
> >> + channel-name = "ain1-Vbias";
> >> + };
> >> +
> >> + channel@6 {
> >> + reg = <6>;
> >> + diff-channels = <AD5940_ADC_INPUTP_AIN2
> >> + AD5940_ADC_INPUTN_VBIAS0>;
> >> + channel-name = "ain2-Vbias";
> >> + };
> >> +
> >> + channel@7 {
> >> + reg = <7>;
> >> + diff-channels = <AD5940_ADC_INPUTP_AIN3
> >> + AD5940_ADC_INPUTN_VBIAS0>;
> >> + channel-name = "ain3-Vbias";
> >> + };
> >> +
> >> + channel@8 {
> >> + reg = <8>;
> >> + diff-channels = <AD5940_ADC_INPUTP_AIN4
> >> + AD5940_ADC_INPUTN_VBIAS0>;
> >> + channel-name = "ain4-Vbias";
> >> + };
> >> +
> >> + channel@9 {
> >> + reg = <9>;
> >> + diff-channels = <AD5940_ADC_INPUTP_AIN6
> >> + AD5940_ADC_INPUTN_VBIAS0>;
> >> + channel-name = "ain6-Vbias";
> >> + };
> >> +
> >> + channel@10 {
> >> + reg = <10>;
> >> + diff-channels = <AD5940_ADC_INPUTP_LPTIA_LP
> >> + AD5940_ADC_INPUTN_LPTIA>;
> >> + channel-name = "Low power TIA DC";
> >> + };
> >> +
> >> + channel@11 {
> >> + reg = <11>;
> >> + diff-channels = <AD5940_ADC_INPUTP_LPTIA
> >> + AD5940_ADC_INPUTN_LPTIA>;
> >> + channel-name = "Low power TIA AC";
> >> + };
> >> +
> >> + channel@12 {
> >> + reg = <12>;
> >> + diff-channels = <AD5940_ADC_INPUTP_HSTIA
> >> + AD5940_ADC_INPUTN_HSTIA>;
> >> + channel-name = "High Speed TIA";
> >> + };
> >> +
> >> + channel@13 {
> >> + reg = <13>;
> >> + diff-channels = <AD5940_ADC_INPUTP_TEMP
> >> + AD5940_ADC_INPUTN_VBIAS0>;
> >> + channel-name = "Temperature";
> >> + };
> >> + };
> >> diff --git a/include/dt-bindings/iio/adc/adi,ad5940.h b/include/dt-bindings/iio/adc/adi,ad5940.h
> >> new file mode 100644
> >> index 000000000000..c17826f2f654
> >> --- /dev/null
> >> +++ b/include/dt-bindings/iio/adc/adi,ad5940.h
> >> @@ -0,0 +1,52 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/*
> >> + * This header provides constants for configuring the AD5940 AFE
> >> + */
> >> +
> >> +#ifndef _DT_BINDINGS_IIO_ADC_AD5940_H
> >> +#define _DT_BINDINGS_IIO_ADC_AD5940_H
> >> +
> >> +#define AD5940_ADC_INPUTN_FLOATING 0
> >> +#define AD5940_ADC_INPUTN_HSTIA 1
> >> +#define AD5940_ADC_INPUTN_LPTIA 2
> >> +#define AD5940_ADC_INPUTN_AIN0 4
> >> +#define AD5940_ADC_INPUTN_AIN1 5
> >> +#define AD5940_ADC_INPUTN_AIN2 6
> >> +#define AD5940_ADC_INPUTN_AIN3 7
> >> +#define AD5940_ADC_INPUTN_VBIAS_CA8 10
> >> +#define AD5940_ADC_INPUTN_TEMP_N 11
> >> +#define AD5940_ADC_INPUTN_AIN4 12
> >> +#define AD5940_ADC_INPUTN_AIN6 14
> >> +#define AD5940_ADC_INPUTN_VZERO 16
> >> +#define AD5940_ADC_INPUTN_VBIAS0 17
> >> +#define AD5940_ADC_INPUTN_EXCITATION 20
> >> +
> >> +#define AD5940_ADC_INPUTP_FLOATING 0
> >> +#define AD5940_ADC_INPUTP_HSTIA 1
> >> +#define AD5940_ADC_INPUTP_LPTIA_LP 2
> >> +#define AD5940_ADC_INPUTP_AIN0 4
> >> +#define AD5940_ADC_INPUTP_AIN1 5
> >> +#define AD5940_ADC_INPUTP_AIN2 6
> >> +#define AD5940_ADC_INPUTP_AIN3 7
> >> +#define AD5940_ADC_INPUTP_AVDD_2 8
> >> +#define AD5940_ADC_INPUTP_DVDD_2 9
> >> +#define AD5940_ADC_INPUTP_AVDD_REG_2 10
> >> +#define AD5940_ADC_INPUTP_TEMP 11
> >> +#define AD5940_ADC_INPUTP_VBIAS_CAP 12
> >> +#define AD5940_ADC_INPUTP_DE0 13
> >> +#define AD5940_ADC_INPUTP_SE0 14
> >> +#define AD5940_ADC_INPUTP_VREF_2V5_2 16
> >> +#define AD5940_ADC_INPUTP_VREF_1V82 18
> >> +#define AD5940_ADC_INPUTP_P_TEMP_N 19
> >> +#define AD5940_ADC_INPUTP_AIN4 20
> >> +#define AD5940_ADC_INPUTP_AIN6 22
> >> +#define AD5940_ADC_INPUTP_VZERO 23
> >> +#define AD5940_ADC_INPUTP_VBIAS0 24
> >> +#define AD5940_ADC_INPUTP_VCE0 25
> >> +#define AD5940_ADC_INPUTP_VRE0 26
> >> +#define AD5940_ADC_INPUTP_VCE0_2 31
> >> +#define AD5940_ADC_INPUTP_LPTIA 33
> >> +#define AD5940_ADC_INPUTP_AGND_REF 35
> >> +#define AD5940_ADC_INPUTP_EXCITATION 36
> >> +
> >> +#endif /* _DT_BINDINGS_IIO_ADC_AD5940 */
> >> --
> >> 2.17.1
> >>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-11-16 14:17 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-08 13:09 [PATCH 1/3] dt-bindings: iio: adc: add support for AD5940 Song Qiang
2019-11-08 13:09 ` [PATCH 2/3] iio: adc: add driver " Song Qiang
2019-11-10 16:55 ` Jonathan Cameron
2019-11-14 14:05 ` Song Qiang
2019-11-16 14:08 ` Jonathan Cameron
2019-11-08 13:09 ` [PATCH 3/3] MAINTAINERS: add maintainer " Song Qiang
2019-11-10 16:26 ` [PATCH 1/3] dt-bindings: iio: adc: add support " Jonathan Cameron
2019-11-14 13:32 ` Song Qiang
2019-11-14 14:01 ` Ardelean, Alexandru
2019-11-16 14:15 ` Jonathan Cameron
2019-11-14 1:28 ` Rob Herring
2019-11-14 17:06 ` Song Qiang
2019-11-16 14:17 ` 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).