* [PATCH v2 0/2] add AD8460 DAC driver
@ 2024-07-30 3:05 Mariel Tinaco
2024-07-30 3:05 ` [PATCH v2 1/2] dt-bindings: iio: dac: add docs for ad8460 Mariel Tinaco
2024-07-30 3:05 ` [PATCH v2 2/2] iio: dac: support the ad8460 Waveform DAC Mariel Tinaco
0 siblings, 2 replies; 13+ messages in thread
From: Mariel Tinaco @ 2024-07-30 3:05 UTC (permalink / raw)
To: linux-iio, devicetree, linux-kernel, Jonathan Cameron,
Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
Michael Hennerich, Conor Dooley, Marcelo Schmitt, Dimitri Fedrau,
David Lechner, Nuno Sá
Add support to AD8460 Waveform Generator DAC
changes in v2
ad8460:
* Mapped the fault monitoring settings for overcurrent, overvoltage
and overtemperature limits to IIO Event threshold controls.
* Added optional raw temperature attribute that gets data from an
IIO provider if it is present. e.g. an ADC channel that reads data
from TMP pin
* Added setter/getter for raw current
* Used devm_iio_dmaengine_buffer_setup_ext to setup DMA engine buffer
(No IIO Backend)
* Used byte-swapping and bulk-transfer for HVDAC data words
* Refactored regulator section to make us of
devm_regulator_get_enable_read_voltage
* Fixed error handling for rset_ohms property
* Reverted IIO_ALTVOLTAGE channel type to IIO_VOLTAGE. Setting it aside
for when IIO backend would be implemented
* Added attributes for toggle_en, symbol and 16 raw values following
the generalized sysfs ABI for DAC devices.
toggle_en: (0) to enable Arbitrary Waveform Generator (AWG) mode,
generate DAC output from parallel interface
(1) to enable Arbitrary Pattern Generator (APG) mode,
generate DAC output from HVDAC data words
symbol: for APG mode, declare the number of raw HVDAC data words
from 0 to cycle through in the DAC output, a.k.a Pattern Depth
rawN: HVDAC Data words available, from 0 to 15
Bindings:
* Matched property name of REFIO_1P2V regulator to its pin name.
* Added GPIO bindings for sdn-reset, reset, and sdn-io although only
reset is supported by the driver.
* Added Regulator bindings for hvcc, hvee, vcc-5v, vref-5v,
dvdd-3p3v and avdd-3p3v
* Added DMA-channel bindings.
* Hard-coded limits for voltage, current and temperature
Mariel Tinaco (2):
dt-bindings: iio: dac: add docs for ad8460
iio: dac: support the ad8460 Waveform DAC
.../bindings/iio/dac/adi,ad8460.yaml | 154 +++
MAINTAINERS | 8 +
drivers/iio/dac/Kconfig | 13 +
drivers/iio/dac/Makefile | 1 +
drivers/iio/dac/ad8460.c | 976 ++++++++++++++++++
5 files changed, 1152 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
create mode 100644 drivers/iio/dac/ad8460.c
base-commit: 9900e7a54764998ba3a22f06ec629f7b5fe0b422
--
2.34.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/2] dt-bindings: iio: dac: add docs for ad8460
2024-07-30 3:05 [PATCH v2 0/2] add AD8460 DAC driver Mariel Tinaco
@ 2024-07-30 3:05 ` Mariel Tinaco
2024-07-30 6:15 ` Krzysztof Kozlowski
2024-08-03 10:05 ` Jonathan Cameron
2024-07-30 3:05 ` [PATCH v2 2/2] iio: dac: support the ad8460 Waveform DAC Mariel Tinaco
1 sibling, 2 replies; 13+ messages in thread
From: Mariel Tinaco @ 2024-07-30 3:05 UTC (permalink / raw)
To: linux-iio, devicetree, linux-kernel, Jonathan Cameron,
Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
Michael Hennerich, Conor Dooley, Marcelo Schmitt, Dimitri Fedrau,
David Lechner, Nuno Sá
This adds the bindings documentation for the 14-bit
High Voltage, High Current, Waveform Generator
Digital-to-Analog converter.
Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com>
---
.../bindings/iio/dac/adi,ad8460.yaml | 154 ++++++++++++++++++
MAINTAINERS | 7 +
2 files changed, 161 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
new file mode 100644
index 000000000..6a7031d0d
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
@@ -0,0 +1,154 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2024 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/dac/adi,ad8460.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD8460 DAC
+
+maintainers:
+ - Mariel Tinaco <mariel.tinaco@analog.com>
+
+description: |
+ Analog Devices AD8460 110 V High Voltage, 1 A High Current,
+ Arbitrary Waveform Generator with Integrated 14-Bit High Speed DAC
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad8460.pdf
+
+properties:
+ compatible:
+ enum:
+ - adi,ad8460
+
+ reg:
+ maxItems: 1
+
+ dmas:
+ maxItems: 1
+
+ dma-names:
+ items:
+ - const: tx
+
+ spi-max-frequency:
+ maximum: 20000000
+
+ hvcc-supply:
+ description: Positive high voltage power supply line
+
+ hvee-supply:
+ description: Negative high voltage power supply line
+
+ vcc-5v-supply:
+ description: Low voltage power supply
+
+ vref-5v-supply:
+ description: Reference voltage for analog low voltage and protection threshold DACs.
+
+ dvdd-3p3v-supply:
+ description: Digital supply bypass
+
+ avdd-3p3v-supply:
+ description: Analog supply bypass
+
+ refio-1p2v-supply:
+ description: Drive voltage in the range of 1.2V maximum to as low as
+ low as 0.12V through the REF_IO pin to adjust full scale output span
+
+ clocks:
+ description: The clock for the DAC. This is the sync clock
+
+ adi,rset-ohms:
+ description: Specify value of external resistor connected to FS_ADJ pin
+ to establish internal HVDAC's reference current I_REF
+ default: 2000
+ minimum: 2000
+ maximum: 20000
+
+ adi,range-microvolt:
+ description: |
+ Voltage output range specified as <minimum, maximum>
+ oneOf:
+ - items:
+ - const: -40000000
+ - const: 40000000
+
+ adi,range-microamp:
+ description: |
+ Current output range specified as <minimum, maximum>
+ oneOf:
+ - items:
+ - const: 0
+ - const: 50000
+ - items:
+ - const: -50000
+ - const: 50000
+
+ adi,temp-max-millicelsius:
+ description: Overtemperature threshold
+ default: 50000
+ minimum: 20000
+ maximum: 150000
+
+ sdn-reset-gpios:
+ description: GPIO spec for the SHUTDOWN RESET pin. As the line is active high,
+ it should be marked GPIO_ACTIVE_HIGH.
+ maxItems: 1
+
+ reset-gpios:
+ description: GPIO spec for the RESET pin. As the line is active low, it
+ should be marked GPIO_ACTIVE_LOW.
+ maxItems: 1
+
+ sdn-io-gpios:
+ description: GPIO spec for the SHUTDOWN INPUT/OUTPUT pin. As the line is
+ active high, it should be marked GPIO_ACTIVE_HIGH.
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - clocks
+
+allOf:
+ - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ dac@0 {
+ compatible = "adi,ad8460";
+ reg = <0>;
+ spi-max-frequency = <8000000>;
+ adi,rset-ohms = <2000>;
+ adi,range-microvolt = <(-40000000) 40000000>;
+ adi,range-microamp = <0 50000>;
+ adi,temp-max-millicelsius = <50000>;
+
+ dmas = <&tx_dma 0>;
+ dma-names = "tx";
+
+ sdn-reset-gpios = <&gpio 86 GPIO_ACTIVE_HIGH>;
+ reset-gpios = <&gpio 91 GPIO_ACTIVE_LOW>;
+ sdn-io-gpios = <&gpio 88 GPIO_ACTIVE_HIGH>;
+
+ clocks = <&sync_ext_clk>;
+
+ hvcc-supply = <&hvcc>;
+ hvee-supply = <&hvee>;
+ vcc-5v-supply = <&vcc_5>;
+ vref-5v-supply = <&vref_5>;
+ dvdd-3p3v-supply = <&dvdd_3_3>;
+ avdd-3p3v-supply = <&avdd_3_3>;
+ refio-1p2v-supply = <&refio_1_2>;
+ };
+ };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 758c202ec..dae93df2a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1234,6 +1234,13 @@ W: https://ez.analog.com/linux-software-drivers
F: Documentation/devicetree/bindings/iio/adc/adi,ad7780.yaml
F: drivers/iio/adc/ad7780.c
+ANALOG DEVICES INC AD8460 DRIVER
+M: Mariel Tinaco <Mariel.Tinaco@analog.com>
+L: linux-iio@vger.kernel.org
+S: Supported
+W: https://ez.analog.com/linux-software-drivers
+F: Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
+
ANALOG DEVICES INC AD9739a DRIVER
M: Nuno Sa <nuno.sa@analog.com>
M: Dragos Bogdan <dragos.bogdan@analog.com>
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/2] iio: dac: support the ad8460 Waveform DAC
2024-07-30 3:05 [PATCH v2 0/2] add AD8460 DAC driver Mariel Tinaco
2024-07-30 3:05 ` [PATCH v2 1/2] dt-bindings: iio: dac: add docs for ad8460 Mariel Tinaco
@ 2024-07-30 3:05 ` Mariel Tinaco
2024-08-02 6:21 ` kernel test robot
` (3 more replies)
1 sibling, 4 replies; 13+ messages in thread
From: Mariel Tinaco @ 2024-07-30 3:05 UTC (permalink / raw)
To: linux-iio, devicetree, linux-kernel, Jonathan Cameron,
Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski,
Michael Hennerich, Conor Dooley, Marcelo Schmitt, Dimitri Fedrau,
David Lechner, Nuno Sá
The AD8460 is a “bits in, power out” high voltage, high-power,
high-speed driver optimized for large output current (up to ±1 A)
and high slew rate (up to ±1800 V/μs) at high voltage (up to ±40 V)
into capacitive loads.
A digital engine implements user-configurable features: modes for
digital input, programmable supply current, and fault monitoring
and programmable protection settings for output current,
output voltage, and junction temperature. The AD8460 operates on
high voltage dual supplies up to ±55 V and a single low voltage
supply of 5 V.
Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com>
---
MAINTAINERS | 1 +
drivers/iio/dac/Kconfig | 13 +
drivers/iio/dac/Makefile | 1 +
drivers/iio/dac/ad8460.c | 976 +++++++++++++++++++++++++++++++++++++++
4 files changed, 991 insertions(+)
create mode 100644 drivers/iio/dac/ad8460.c
diff --git a/MAINTAINERS b/MAINTAINERS
index dae93df2a..6134cac65 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1240,6 +1240,7 @@ L: linux-iio@vger.kernel.org
S: Supported
W: https://ez.analog.com/linux-software-drivers
F: Documentation/devicetree/bindings/iio/dac/adi,ad8460.yaml
+F: drivers/iio/dac/ad8460.c
ANALOG DEVICES INC AD9739a DRIVER
M: Nuno Sa <nuno.sa@analog.com>
diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index 3c2bf620f..8da5cfe4f 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -300,6 +300,19 @@ config AD7303
To compile this driver as module choose M here: the module will be called
ad7303.
+config AD8460
+ tristate "Analog Devices AD8460 DAC driver"
+ depends on SPI
+ select REGMAP_SPI
+ select IIO_BUFFER
+ select IIO_BUFFER_DMAENGINE
+ help
+ Say yes here to build support for Analog Devices AD8460 Digital to
+ Analog Converters (DAC).
+
+ To compile this driver as a module choose M here: the module will be called
+ ad8460.
+
config AD8801
tristate "Analog Devices AD8801/AD8803 DAC driver"
depends on SPI_MASTER
diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
index 8432a81a1..0fa2849e1 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_AD5686_SPI) += ad5686-spi.o
obj-$(CONFIG_AD5696_I2C) += ad5696-i2c.o
obj-$(CONFIG_AD7293) += ad7293.o
obj-$(CONFIG_AD7303) += ad7303.o
+obj-$(CONFIG_AD8460) += ad8460.o
obj-$(CONFIG_AD8801) += ad8801.o
obj-$(CONFIG_AD9739A) += ad9739a.o
obj-$(CONFIG_ADI_AXI_DAC) += adi-axi-dac.o
diff --git a/drivers/iio/dac/ad8460.c b/drivers/iio/dac/ad8460.c
new file mode 100644
index 000000000..a94fa4526
--- /dev/null
+++ b/drivers/iio/dac/ad8460.c
@@ -0,0 +1,976 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * AD8460 Waveform generator DAC Driver
+ *
+ * Copyright (C) 2024 Analog Devices, Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/cleanup.h>
+#include <linux/clk.h>
+#include <linux/debugfs.h>
+#include <linux/delay.h>
+#include <linux/dmaengine.h>
+#include <linux/gpio/consumer.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/buffer-dma.h>
+#include <linux/iio/buffer-dmaengine.h>
+#include <linux/iio/consumer.h>
+#include <linux/iio/events.h>
+#include <linux/iio/iio.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#define AD8460_CTRL_REG(x) (x)
+#define AD8460_HVDAC_DATA_WORD_LOW(x) (0x60 + (2 * (x)))
+#define AD8460_HVDAC_DATA_WORD_HIGH(x) (0x61 + (2 * (x)))
+
+#define AD8460_HV_RESET_MSK BIT(7)
+#define AD8460_HV_SLEEP_MSK BIT(4)
+#define AD8460_WAVE_GEN_MODE_MSK BIT(0)
+
+#define AD8460_HVDAC_SLEEP_MSK BIT(3)
+
+#define AD8460_FAULT_ARM_MSK BIT(7)
+#define AD8460_FAULT_LIMIT_MSK GENMASK(6, 0)
+
+#define AD8460_APG_MODE_ENABLE_MSK BIT(5)
+#define AD8460_PATTERN_DEPTH_MSK GENMASK(3, 0)
+
+#define AD8460_SET_QUIESCENT_CURRENT_MSK GENMASK(7, 0)
+
+#define AD8460_SHUTDOWN_FLAG_MSK BIT(7)
+
+#define AD8460_DATA_BYTE_LOW_MSK GENMASK(7, 0)
+#define AD8460_DATA_BYTE_HIGH_MSK GENMASK(5, 0)
+
+#define AD8460_DATA_BYTE_WORD_LENGTH 2
+#define AD8460_NUM_DATA_WORDS 16
+#define AD8460_NOMINAL_VOLTAGE_SPAN 80
+#define AD8460_MIN_RSET_OHMS 2000
+#define AD8460_MAX_RSET_OHMS 20000
+#define AD8460_MIN_VREFIO_UV 120000
+#define AD8460_MAX_VREFIO_UV 1200000
+#define AD8460_CURRENT_LIMIT_CONV(x) ((x) / 15625)
+#define AD8460_VOLTAGE_LIMIT_CONV(x) ((x) / 1953000)
+#define AD8460_TEMP_LIMIT_CONV(x) (((x) + 266640) / 6510)
+
+enum ad8460_fault_type {
+ AD8460_OVERCURRENT_SRC,
+ AD8460_OVERCURRENT_SNK,
+ AD8460_OVERVOLTAGE_POS,
+ AD8460_OVERVOLTAGE_NEG,
+ AD8460_OVERTEMPERATURE,
+};
+
+struct ad8460_state {
+ struct spi_device *spi;
+ struct regmap *regmap;
+ struct iio_channel *tmp_adc_channel;
+ struct clk *sync_clk;
+ /* lock to protect against multiple access to the device and shared data */
+ struct mutex lock;
+ int refio_1p2v_mv;
+ u32 rset_ohms;
+ /*
+ * DMA (thus cache coherency maintenance) requires the
+ * transfer buffers to live in their own cache lines.
+ */
+ u8 spi_tx_buf[2] __aligned(IIO_DMA_MINALIGN);
+};
+
+static int ad8460_hv_reset(struct ad8460_state *state)
+{
+ int ret;
+
+ ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x00),
+ AD8460_HV_RESET_MSK,
+ FIELD_PREP(AD8460_HV_RESET_MSK, 1));
+ if (ret)
+ return ret;
+
+ fsleep(20);
+
+ return regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x00),
+ AD8460_HV_RESET_MSK,
+ FIELD_PREP(AD8460_HV_RESET_MSK, 0));
+}
+
+static int ad8460_reset(const struct ad8460_state *state)
+{
+ struct device *dev = &state->spi->dev;
+ struct gpio_desc *reset;
+
+ reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
+ if (IS_ERR(reset))
+ return dev_err_probe(dev, PTR_ERR(reset),
+ "Failed to get reset gpio");
+ if (reset) {
+ /* minimum duration of 10ns */
+ ndelay(10);
+ gpiod_set_value_cansleep(reset, 1);
+ return 0;
+ }
+
+ /* bring all registers to their default state */
+ return regmap_write(state->regmap, AD8460_CTRL_REG(0x03), 1);
+}
+
+static int ad8460_enable_apg_mode(struct ad8460_state *state, int val)
+{
+ int ret;
+
+ ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x02),
+ AD8460_APG_MODE_ENABLE_MSK,
+ FIELD_PREP(AD8460_APG_MODE_ENABLE_MSK, val));
+ if (ret)
+ return ret;
+
+ return regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x00),
+ AD8460_WAVE_GEN_MODE_MSK,
+ FIELD_PREP(AD8460_WAVE_GEN_MODE_MSK, val));
+}
+
+static int ad8460_read_shutdown_flag(struct ad8460_state *state, u64 *flag)
+{
+ int ret, val;
+
+ ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x0E), &val);
+ if (ret)
+ return ret;
+
+ *flag = FIELD_GET(AD8460_SHUTDOWN_FLAG_MSK, val);
+ return 0;
+}
+
+static ssize_t ad8460_dac_input_read(struct iio_dev *indio_dev,
+ uintptr_t private,
+ const struct iio_chan_spec *chan,
+ char *buf)
+{
+ struct ad8460_state *state = iio_priv(indio_dev);
+ unsigned int reg;
+ int ret;
+
+ ret = ad8460_get_hvdac_word(state, private, ®);
+ if (ret)
+ return ret;
+
+ return sysfs_emit(buf, "%ld\n", reg);
+}
+
+static ssize_t ad8460_dac_input_write(struct iio_dev *indio_dev,
+ uintptr_t private,
+ const struct iio_chan_spec *chan,
+ const char *buf,
+ size_t len)
+{
+ struct ad8460_state *state = iio_priv(indio_dev);
+ unsigned int reg;
+ int ret;
+
+ ret = kstrtou32(buf, ®);
+ if (ret)
+ return ret;
+
+ guard(mutex)(&state->lock);
+
+ return ad8460_set_hvdac_word(state, private, reg);
+}
+
+static ssize_t ad8460_read_symbol(struct iio_dev *indio_dev,
+ uintptr_t private,
+ const struct iio_chan_spec *chan,
+ char *buf)
+{
+ struct ad8460_state *state = iio_priv(indio_dev);
+ unsigned int reg;
+ int ret;
+
+ ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x02), ®);
+ if (ret)
+ return ret;
+
+ return sysfs_emit(buf, "%ld\n", FIELD_GET(AD8460_PATTERN_DEPTH_MSK, reg));
+}
+
+static ssize_t ad8460_write_symbol(struct iio_dev *indio_dev,
+ uintptr_t private,
+ const struct iio_chan_spec *chan,
+ const char *buf,
+ size_t len)
+{
+ struct ad8460_state *state = iio_priv(indio_dev);
+ bool sym;
+ int ret;
+
+ ret = kstrtou16(buf, &sym);
+ if (ret)
+ return ret;
+
+ guard(mutex)(&state->lock);
+
+ return regmap_update_bits(state->regmap,
+ AD8460_CTRL_REG(0x02),
+ AD8460_PATTERN_DEPTH_MSK,
+ FIELD_PREP(AD8460_PATTERN_DEPTH_MSK, sym));
+}
+
+static ssize_t ad8460_read_toggle_en(struct iio_dev *indio_dev,
+ uintptr_t private,
+ const struct iio_chan_spec *chan,
+ char *buf)
+{
+ struct ad8460_state *state = iio_priv(indio_dev);
+ unsigned int reg;
+ int ret;
+
+ ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x02), ®);
+ if (ret)
+ return ret;
+
+ return sysfs_emit(buf, "%ld\n", FIELD_GET(AD8460_APG_MODE_ENABLE_MSK, reg));
+}
+
+static ssize_t ad8460_write_toggle_en(struct iio_dev *indio_dev,
+ uintptr_t private,
+ const struct iio_chan_spec *chan,
+ const char *buf,
+ size_t len)
+{
+ struct ad8460_state *state = iio_priv(indio_dev);
+ bool toggle_en;
+ int ret;
+
+ ret = kstrtou16(buf, &toggle_en);
+ if (ret)
+ return ret;
+
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
+ return ad8460_enable_apg_mode(state, toggle_en);
+ unreachable();
+}
+
+static ssize_t ad8460_read_powerdown(struct iio_dev *indio_dev,
+ uintptr_t private,
+ const struct iio_chan_spec *chan,
+ char *buf)
+{
+ struct ad8460_state *state = iio_priv(indio_dev);
+ unsigned int reg;
+ int ret;
+
+ ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x01), ®);
+ if (ret)
+ return ret;
+
+ return sysfs_emit(buf, "%ld\n", FIELD_GET(AD8460_HVDAC_SLEEP_MSK, reg));
+}
+
+static ssize_t ad8460_write_powerdown(struct iio_dev *indio_dev,
+ uintptr_t private,
+ const struct iio_chan_spec *chan,
+ const char *buf,
+ size_t len)
+{
+ struct ad8460_state *state = iio_priv(indio_dev);
+ bool pwr_down;
+ u64 sdn_flag;
+ int ret;
+
+ ret = kstrtobool(buf, &pwr_down);
+ if (ret)
+ return ret;
+
+ guard(mutex)(&state->lock);
+
+ ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01),
+ AD8460_HVDAC_SLEEP_MSK,
+ FIELD_PREP(AD8460_HVDAC_SLEEP_MSK, pwr_down));
+ if (ret)
+ return ret;
+
+ if (!pwr_down) {
+ ret = ad8460_read_shutdown_flag(state, &sdn_flag);
+ if (ret)
+ return ret;
+
+ if (sdn_flag) {
+ ret = ad8460_hv_reset(state);
+ if (ret)
+ return ret;
+ }
+ }
+
+ ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x00),
+ AD8460_HV_SLEEP_MSK,
+ FIELD_PREP(AD8460_HV_SLEEP_MSK, !pwr_down));
+ if (ret)
+ return ret;
+
+ return len;
+}
+
+static const char * const ad8460_powerdown_modes[] = {
+ "three_state",
+};
+
+static int ad8460_get_powerdown_mode(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan)
+{
+ return 0;
+}
+
+static int ad8460_set_powerdown_mode(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ unsigned int type)
+{
+ return 0;
+}
+
+static int ad8460_get_hvdac_word(struct ad8460_state *state,
+ int index,
+ int *val)
+{
+ int ret;
+
+ ret = regmap_bulk_read(state->regmap, AD8460_HVDAC_DATA_WORD_LOW(index),
+ &state->spi_tx_buf, AD8460_DATA_BYTE_WORD_LENGTH);
+ if (ret)
+ return ret;
+
+ *val = get_unaligned_le16(state->spi_tx_buf);
+
+ return ret;
+}
+
+static int ad8460_set_hvdac_word(struct ad8460_state *state,
+ int index,
+ int val)
+{
+ put_unaligned_le16(val & 0x3FFF, &state->spi_tx_buf);
+
+ return regmap_bulk_write(state->regmap, AD8460_HVDAC_DATA_WORD_LOW(index),
+ state->spi_tx_buf, AD8460_DATA_BYTE_WORD_LENGTH);
+}
+
+static int ad8460_set_sample(struct ad8460_state *state, int val)
+{
+ int ret;
+
+ ret = ad8460_enable_apg_mode(state, 1);
+ if (ret)
+ return ret;
+
+ guard(mutex)(&state->lock);
+ ret = ad8460_set_hvdac_word(state, 0, val);
+ if (ret)
+ return ret;
+
+ return regmap_update_bits(state->regmap,
+ AD8460_CTRL_REG(0x02),
+ AD8460_PATTERN_DEPTH_MSK,
+ FIELD_PREP(AD8460_PATTERN_DEPTH_MSK, 0));
+}
+
+static int ad8460_set_fault_threshold(struct ad8460_state *state,
+ enum ad8460_fault_type fault,
+ unsigned int threshold)
+{
+ return regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x08 + fault),
+ AD8460_FAULT_LIMIT_MSK,
+ FIELD_PREP(AD8460_FAULT_LIMIT_MSK, threshold));
+}
+
+static int ad8460_get_fault_threshold(struct ad8460_state *state,
+ enum ad8460_fault_type fault,
+ unsigned int *threshold)
+{
+ unsigned int val;
+ int ret;
+
+ ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x08 + fault), &val);
+ if (ret)
+ return ret;
+
+ *threshold = FIELD_GET(AD8460_FAULT_LIMIT_MSK, val);
+
+ return ret;
+}
+
+static int ad8460_set_fault_threshold_en(struct ad8460_state *state,
+ enum ad8460_fault_type fault,
+ bool en)
+{
+ return regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x08 + fault),
+ AD8460_FAULT_ARM_MSK,
+ FIELD_PREP(AD8460_FAULT_ARM_MSK, en));
+}
+
+static int ad8460_get_fault_threshold_en(struct ad8460_state *state,
+ enum ad8460_fault_type fault,
+ bool *en)
+{
+ unsigned int val;
+ int ret;
+
+ ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x08 + fault), &val);
+ if (ret)
+ return ret;
+
+ *en = FIELD_GET(AD8460_FAULT_ARM_MSK, val);
+
+ return 0;
+}
+
+static int ad8460_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val,
+ int val2,
+ long mask)
+{
+ struct ad8460_state *state = iio_priv(indio_dev);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ switch (chan->type) {
+ case IIO_VOLTAGE:
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
+ return ad8460_set_sample(state, val);
+ unreachable();
+ case IIO_CURRENT:
+ return regmap_write(state->regmap, AD8460_CTRL_REG(0x04),
+ FIELD_PREP(AD8460_SET_QUIESCENT_CURRENT_MSK, val));
+ default:
+ return -EINVAL;
+ }
+ default:
+ return -EINVAL;
+ }
+}
+
+static int ad8460_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val,
+ int *val2,
+ long mask)
+{
+ struct ad8460_state *state = iio_priv(indio_dev);
+ int data, ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ switch (chan->type) {
+ case IIO_VOLTAGE:
+ scoped_guard(mutex, &state->lock) {
+ ret = ad8460_get_hvdac_word(state, 0, &data);
+ if (ret)
+ return ret;
+ }
+ *val = data;
+ return IIO_VAL_INT;
+ case IIO_CURRENT:
+ ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x04), &data);
+ if (ret)
+ return ret;
+ *val = data;
+ return IIO_VAL_INT;
+ case IIO_TEMP:
+ ret = iio_read_channel_raw(state->tmp_adc_channel, &data);
+ if (ret)
+ return ret;
+ *val = data;
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ *val = clk_get_rate(state->sync_clk);
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_SCALE:
+ /* vCONV = vNOMINAL_SPAN * (DAC_CODE / 2**14) - 40V
+ * vMAX = vNOMINAL_SPAN * (2**14 / 2**14) - 40V
+ * vMIN = vNOMINAL_SPAN * (0 / 2**14) - 40V
+ * vADJ = vCONV * (2000 / rSET) * (vREF / 1.2)
+ * vSPAN = vADJ_MAX - vADJ_MIN
+ * See datasheet page 49, section FULL-SCALE REDUCTION
+ */
+ *val = AD8460_NOMINAL_VOLTAGE_SPAN * 2000 * state->refio_1p2v_mv;
+ *val2 = state->rset_ohms * 1200;
+ return IIO_VAL_FRACTIONAL;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int ad8460_select_fault_type(int chan_type, enum iio_event_direction dir)
+{
+ switch (chan_type) {
+ case IIO_VOLTAGE:
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ return AD8460_OVERVOLTAGE_POS;
+ case IIO_EV_DIR_FALLING:
+ return AD8460_OVERVOLTAGE_NEG;
+ default:
+ return -EINVAL;
+ }
+ break;
+ case IIO_CURRENT:
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ return AD8460_OVERCURRENT_SRC;
+ case IIO_EV_DIR_FALLING:
+ return AD8460_OVERCURRENT_SNK;
+ default:
+ return -EINVAL;
+ }
+ break;
+ case IIO_TEMP:
+ switch (dir) {
+ case IIO_EV_DIR_RISING:
+ return AD8460_OVERTEMPERATURE;
+ default:
+ return -EINVAL;
+ }
+ default:
+ return -EINVAL;
+ }
+}
+
+static int ad8460_write_event_value(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info, int val, int val2)
+{
+ struct ad8460_state *state = iio_priv(indio_dev);
+ unsigned int fault;
+
+ if (type != IIO_EV_TYPE_THRESH)
+ return -EINVAL;
+
+ if (info != IIO_EV_INFO_VALUE)
+ return -EINVAL;
+
+ fault = ad8460_select_fault_type(chan->type, dir);
+ if (fault < 0)
+ return fault;
+
+ return ad8460_set_fault_threshold(state, fault, val);
+}
+
+static int ad8460_read_event_value(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info, int *val, int *val2)
+{
+ struct ad8460_state *state = iio_priv(indio_dev);
+ unsigned int fault;
+
+ if (type != IIO_EV_TYPE_THRESH)
+ return -EINVAL;
+
+ if (info != IIO_EV_INFO_VALUE)
+ return -EINVAL;
+
+ fault = ad8460_select_fault_type(chan->type, dir);
+ if (fault < 0)
+ return fault;
+
+ return ad8460_get_fault_threshold(state, fault, val);
+}
+
+static int ad8460_write_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir, int val)
+{
+ struct ad8460_state *state = iio_priv(indio_dev);
+ unsigned int fault;
+
+ if (type != IIO_EV_TYPE_THRESH)
+ return -EINVAL;
+
+ fault = ad8460_select_fault_type(chan->type, dir);
+ if (fault < 0)
+ return fault;
+
+ return ad8460_set_fault_threshold_en(state, fault, val);
+}
+
+static int ad8460_read_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir)
+{
+ struct ad8460_state *state = iio_priv(indio_dev);
+ unsigned int fault;
+ bool en;
+ int ret;
+
+ if (type != IIO_EV_TYPE_THRESH)
+ return -EINVAL;
+
+ fault = ad8460_select_fault_type(chan->type, dir);
+ if (fault < 0)
+ return fault;
+
+ ret = ad8460_get_fault_threshold_en(state, fault, &en);
+ return ret ?: en;
+}
+
+static int ad8460_reg_access(struct iio_dev *indio_dev,
+ unsigned int reg, unsigned int writeval,
+ unsigned int *readval)
+{
+ struct ad8460_state *state = iio_priv(indio_dev);
+
+ if (readval)
+ return regmap_read(state->regmap, reg, readval);
+
+ return regmap_write(state->regmap, reg, writeval);
+}
+
+static int ad8460_buffer_preenable(struct iio_dev *indio_dev)
+{
+ struct ad8460_state *state = iio_priv(indio_dev);
+
+ return ad8460_enable_apg_mode(state, 0);
+}
+
+static int ad8460_buffer_postdisable(struct iio_dev *indio_dev)
+{
+ struct ad8460_state *state = iio_priv(indio_dev);
+
+ return ad8460_enable_apg_mode(state, 1);
+}
+
+static const struct iio_buffer_setup_ops ad8460_buffer_setup_ops = {
+ .preenable = &ad8460_buffer_preenable,
+ .postdisable = &ad8460_buffer_postdisable,
+};
+
+static const struct iio_info ad8460_info = {
+ .read_raw = &ad8460_read_raw,
+ .write_raw = &ad8460_write_raw,
+ .write_event_value = &ad8460_write_event_value,
+ .read_event_value = &ad8460_read_event_value,
+ .write_event_config = &ad8460_write_event_config,
+ .read_event_config = &ad8460_read_event_config,
+ .debugfs_reg_access = &ad8460_reg_access,
+};
+
+static const struct iio_enum ad8460_powerdown_mode_enum = {
+ .items = ad8460_powerdown_modes,
+ .num_items = ARRAY_SIZE(ad8460_powerdown_modes),
+ .get = ad8460_get_powerdown_mode,
+ .set = ad8460_set_powerdown_mode,
+};
+
+#define AD8460_CHAN_EXT_INFO(_name, _what, _shared, _read, _write) { \
+ .name = _name, \
+ .read = (_read), \
+ .write = (_write), \
+ .private = (_what), \
+ .shared = (_shared), \
+}
+
+static struct iio_chan_spec_ext_info ad8460_ext_info[] = {
+ AD8460_CHAN_EXT_INFO("raw0", 0, IIO_SEPARATE,
+ ad8460_dac_input_read, ad8460_dac_input_write),
+ AD8460_CHAN_EXT_INFO("raw1", 1, IIO_SEPARATE,
+ ad8460_dac_input_read, ad8460_dac_input_write),
+ AD8460_CHAN_EXT_INFO("raw2", 2, IIO_SEPARATE,
+ ad8460_dac_input_read, ad8460_dac_input_write),
+ AD8460_CHAN_EXT_INFO("raw3", 3, IIO_SEPARATE,
+ ad8460_dac_input_read, ad8460_dac_input_write),
+ AD8460_CHAN_EXT_INFO("raw4", 4, IIO_SEPARATE,
+ ad8460_dac_input_read, ad8460_dac_input_write),
+ AD8460_CHAN_EXT_INFO("raw5", 5, IIO_SEPARATE,
+ ad8460_dac_input_read, ad8460_dac_input_write),
+ AD8460_CHAN_EXT_INFO("raw6", 6, IIO_SEPARATE,
+ ad8460_dac_input_read, ad8460_dac_input_write),
+ AD8460_CHAN_EXT_INFO("raw7", 7, IIO_SEPARATE,
+ ad8460_dac_input_read, ad8460_dac_input_write),
+ AD8460_CHAN_EXT_INFO("raw8", 8, IIO_SEPARATE,
+ ad8460_dac_input_read, ad8460_dac_input_write),
+ AD8460_CHAN_EXT_INFO("raw9", 9, IIO_SEPARATE,
+ ad8460_dac_input_read, ad8460_dac_input_write),
+ AD8460_CHAN_EXT_INFO("raw10", 10, IIO_SEPARATE,
+ ad8460_dac_input_read, ad8460_dac_input_write),
+ AD8460_CHAN_EXT_INFO("raw11", 11, IIO_SEPARATE,
+ ad8460_dac_input_read, ad8460_dac_input_write),
+ AD8460_CHAN_EXT_INFO("raw12", 12, IIO_SEPARATE,
+ ad8460_dac_input_read, ad8460_dac_input_write),
+ AD8460_CHAN_EXT_INFO("raw13", 13, IIO_SEPARATE,
+ ad8460_dac_input_read, ad8460_dac_input_write),
+ AD8460_CHAN_EXT_INFO("raw14", 14, IIO_SEPARATE,
+ ad8460_dac_input_read, ad8460_dac_input_write),
+ AD8460_CHAN_EXT_INFO("raw15", 15, IIO_SEPARATE,
+ ad8460_dac_input_read, ad8460_dac_input_write),
+ AD8460_CHAN_EXT_INFO("toggle_en", 0, IIO_SEPARATE,
+ ad8460_read_toggle_en,
+ ad8460_write_toggle_en),
+ AD8460_CHAN_EXT_INFO("symbol", 0, IIO_SEPARATE,
+ ad8460_read_symbol,
+ ad8460_write_symbol),
+ AD8460_CHAN_EXT_INFO("powerdown", 0, IIO_SEPARATE,
+ ad8460_read_powerdown, ad8460_write_powerdown),
+ IIO_ENUM("powerdown_mode", IIO_SEPARATE, &ad8460_powerdown_mode_enum),
+ IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE,
+ &ad8460_powerdown_mode_enum),
+ {}
+};
+
+static const struct iio_event_spec ad8460_events[] = {
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_RISING,
+ .mask_separate = BIT(IIO_EV_INFO_VALUE) |
+ BIT(IIO_EV_INFO_ENABLE),
+ },
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_FALLING,
+ .mask_separate = BIT(IIO_EV_INFO_VALUE) |
+ BIT(IIO_EV_INFO_ENABLE),
+ },
+};
+
+#define AD8460_VOLTAGE_CHAN(_chan) { \
+ .type = IIO_VOLTAGE, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
+ BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE), \
+ .output = 1, \
+ .indexed = 1, \
+ .channel = (_chan), \
+ .scan_index = 0, \
+ .scan_type = { \
+ .sign = 'u', \
+ .realbits = 14, \
+ .storagebits = 16, \
+ .endianness = IIO_LE, \
+ }, \
+ .ext_info = ad8460_ext_info, \
+ .event_spec = ad8460_events, \
+ .num_event_specs = ARRAY_SIZE(ad8460_events), \
+}
+
+#define AD8460_CURRENT_CHAN(_chan) { \
+ .type = IIO_CURRENT, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .output = 0, \
+ .indexed = 1, \
+ .channel = (_chan), \
+ .scan_index = 1, \
+ .scan_type = { \
+ .sign = 'u', \
+ .realbits = 8, \
+ .storagebits = 8, \
+ .endianness = IIO_LE, \
+ }, \
+ .event_spec = ad8460_events, \
+ .num_event_specs = ARRAY_SIZE(ad8460_events), \
+}
+
+#define AD8460_TEMP_CHAN(_chan) { \
+ .type = IIO_TEMP, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .output = 1, \
+ .indexed = 1, \
+ .channel = (_chan), \
+ .scan_index = 2, \
+ .scan_type = { \
+ .sign = 'u', \
+ .endianness = IIO_LE, \
+ }, \
+ .event_spec = ad8460_events, \
+ .num_event_specs = 1, \
+}
+
+static const struct iio_chan_spec ad8460_channels[] = {
+ AD8460_VOLTAGE_CHAN(0),
+ AD8460_CURRENT_CHAN(0),
+};
+
+static const struct iio_chan_spec ad8460_channels_with_tmp_adc[] = {
+ AD8460_VOLTAGE_CHAN(0),
+ AD8460_CURRENT_CHAN(0),
+ AD8460_TEMP_CHAN(0),
+};
+
+static const struct regmap_config ad8460_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .max_register = 0x7F,
+};
+
+static int ad8460_probe(struct spi_device *spi)
+{
+ struct ad8460_state *state;
+ struct iio_dev *indio_dev;
+ struct regulator *refio_1p2v;
+ u32 tmp[2], temp;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ state = iio_priv(indio_dev);
+ mutex_init(&state->lock);
+
+ indio_dev->name = "ad8460";
+ indio_dev->info = &ad8460_info;
+
+ state->spi = spi;
+
+ state->regmap = devm_regmap_init_spi(spi, &ad8460_regmap_config);
+ if (IS_ERR(state->regmap))
+ return dev_err_probe(&spi->dev, PTR_ERR(state->regmap),
+ "Failed to initialize regmap");
+
+ state->sync_clk = devm_clk_get_enabled(&spi->dev, NULL);
+ if (IS_ERR(state->sync_clk))
+ return dev_err_probe(&spi->dev, PTR_ERR(state->sync_clk),
+ "Failed to get sync clk\n");
+
+ state->tmp_adc_channel = devm_iio_channel_get(&spi->dev, "ad8460-tmp");
+ if (IS_ERR_OR_NULL(state->tmp_adc_channel)) {
+ indio_dev->channels = ad8460_channels;
+ indio_dev->num_channels = ARRAY_SIZE(ad8460_channels);
+ } else {
+ indio_dev->channels = ad8460_channels_with_tmp_adc;
+ indio_dev->num_channels = ARRAY_SIZE(ad8460_channels_with_tmp_adc);
+ dev_info(&spi->dev, "Found ADC channel to read TMP pin\n");
+ }
+
+ ret = devm_regulator_get_enable_read_voltage(&spi->dev, "refio_1p2v");
+ if (ret == -ENODEV) {
+ state->refio_1p2v_mv = 1200;
+ } else if (ret < 0) {
+ return dev_err_probe(&spi->dev, PTR_ERR(vrefio),
+ "Failed to get reference voltage\n");
+ } else {
+ state->refio_1p2v_mv = ret / 1000;
+ }
+
+ if (!in_range(ret, AD8460_MIN_VREFIO_UV, AD8460_MAX_VREFIO_UV))
+ return dev_err_probe(&spi->dev, -EINVAL,
+ "Invalid ref voltage range(%u mV) [%u mV, %u mV]\n",
+ ret, AD8460_MIN_VREFIO_UV / 1000,
+ AD8460_MAX_VREFIO_UV / 1000);
+
+ ret = device_property_read_u32(&spi->dev, "adi,rset-ohms", &state->rset_ohms);
+ if (ret) {
+ state->rset_ohms = 2000;
+ } else {
+ if (!in_range(state->rset_ohms, AD8460_MIN_RSET_OHMS,
+ AD8460_MAX_RSET_OHMS))
+ return dev_err_probe(&spi->dev, -EINVAL,
+ "Invalid resistor set range(%u) [%u, %u]\n",
+ state->rset_ohms,
+ AD8460_MIN_RSET_OHMS,
+ AD8460_MAX_RSET_OHMS);
+ }
+
+ /* Arm the device by default */
+ ret = device_property_read_u32_array(&spi->dev, "adi,range-microamp",
+ tmp, ARRAY_SIZE(tmp));
+ if (!ret) {
+ ret = regmap_write(state->regmap, AD8460_CTRL_REG(0x08),
+ (1 << 7) | AD8460_CURRENT_LIMIT_CONV(tmp[1]));
+ if (ret)
+ return dev_err_probe(&spi->dev, -EINVAL,
+ "overcurrent src not valid: %d uA",
+ tmp[1]);
+
+ ret = regmap_write(state->regmap, AD8460_CTRL_REG(0x09),
+ (1 << 7) | AD8460_CURRENT_LIMIT_CONV(abs(tmp[0])));
+ if (ret)
+ return dev_err_probe(&spi->dev, -EINVAL,
+ "overcurrent snk not valid: %d uA",
+ tmp[0]);
+ }
+
+ ret = device_property_read_u32_array(&spi->dev, "adi,range-microvolt",
+ tmp, ARRAY_SIZE(tmp));
+ if (!ret) {
+ ret = regmap_write(state->regmap, AD8460_CTRL_REG(0x0A),
+ (1 << 7) | AD8460_VOLTAGE_LIMIT_CONV(tmp[1]));
+ if (ret)
+ return dev_err_probe(&spi->dev, -EINVAL,
+ "positive overvoltage not valid: %d uV",
+ tmp[1]);
+
+ ret = regmap_write(state->regmap, AD8460_CTRL_REG(0x0B),
+ (1 << 7) | AD8460_VOLTAGE_LIMIT_CONV(abs(tmp[0])));
+ if (ret)
+ return dev_err_probe(&spi->dev, -EINVAL,
+ "negative overvoltage not valid: %d uV",
+ tmp[0]);
+ }
+
+ ret = device_property_read_u32(&spi->dev, "adi,temp-max-millicelsius", &temp);
+ if (!ret) {
+ ret = regmap_write(state->regmap, AD8460_CTRL_REG(0x0C),
+ (1 << 7) | AD8460_TEMP_LIMIT_CONV(abs(temp)));
+ if (ret)
+ return dev_err_probe(&spi->dev, -EINVAL,
+ "overtemperature not valid: %d",
+ temp);
+ }
+
+ ret = ad8460_reset(state);
+ if (ret)
+ return ret;
+
+ /* Enables DAC by default */
+ ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01),
+ AD8460_HVDAC_SLEEP_MSK,
+ FIELD_PREP(AD8460_HVDAC_SLEEP_MSK, 0));
+ if (ret)
+ return ret;
+
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->setup_ops = &ad8460_buffer_setup_ops;
+
+ ret = devm_iio_dmaengine_buffer_setup_ext(&spi->dev, indio_dev, "tx",
+ IIO_BUFFER_DIRECTION_OUT);
+ if (ret)
+ return dev_err_probe(&spi->dev, ret,
+ "Failed to get DMA buffer\n");
+
+ ret = devm_iio_device_register(&spi->dev, indio_dev);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static const struct of_device_id ad8460_of_match[] = {
+ { .compatible = "adi, ad8460" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, ad8460_of_match);
+
+static struct spi_driver ad8460_driver = {
+ .driver = {
+ .name = "ad8460",
+ .of_match_table = ad8460_of_match,
+ },
+ .probe = ad8460_probe,
+};
+module_spi_driver(ad8460_driver);
+
+MODULE_AUTHOR("Mariel Tinaco <mariel.tinaco@analog.com");
+MODULE_DESCRIPTION("AD8460 DAC driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_DMAENGINE_BUFFER);
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: iio: dac: add docs for ad8460
2024-07-30 3:05 ` [PATCH v2 1/2] dt-bindings: iio: dac: add docs for ad8460 Mariel Tinaco
@ 2024-07-30 6:15 ` Krzysztof Kozlowski
2024-08-17 11:18 ` Tinaco, Mariel
2024-08-03 10:05 ` Jonathan Cameron
1 sibling, 1 reply; 13+ messages in thread
From: Krzysztof Kozlowski @ 2024-07-30 6:15 UTC (permalink / raw)
To: Mariel Tinaco, linux-iio, devicetree, linux-kernel,
Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Michael Hennerich, Conor Dooley,
Marcelo Schmitt, Dimitri Fedrau, David Lechner, Nuno Sá
On 30/07/2024 05:05, Mariel Tinaco wrote:
> This adds the bindings documentation for the 14-bit
> High Voltage, High Current, Waveform Generator
> Digital-to-Analog converter.
>
> Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com>
> +
> + refio-1p2v-supply:
> + description: Drive voltage in the range of 1.2V maximum to as low as
> + low as 0.12V through the REF_IO pin to adjust full scale output span
> +
> + clocks:
maxItems: 1
and drop description (or use items: - description, but then do not
repeat redundant parts)
> + description: The clock for the DAC. This is the sync clock
> +
> + adi,rset-ohms:
> + description: Specify value of external resistor connected to FS_ADJ pin
> + to establish internal HVDAC's reference current I_REF
> + default: 2000
> + minimum: 2000
> + maximum: 20000
> +
> + adi,range-microvolt:
> + description: |
> + Voltage output range specified as <minimum, maximum>
> + oneOf:
Not an oneOf.
> + - items:
> + - const: -40000000
> + - const: 40000000
Why do you need this property if this cannot be anything else? Drop.
> +
> + adi,range-microamp:
> + description: |
Do not need '|' unless you need to preserve formatting.
> + Current output range specified as <minimum, maximum>
> + oneOf:
> + - items:
> + - const: 0
> + - const: 50000
> + - items:
> + - const: -50000
> + - const: 50000
> +
> + adi,temp-max-millicelsius:
> + description: Overtemperature threshold
> + default: 50000
> + minimum: 20000
> + maximum: 150000
> +
> + sdn-reset-gpios:
reset-gpios or shutdown-gpios or anything from gpio-consumer-common
which is not deprecated.
> + description: GPIO spec for the SHUTDOWN RESET pin. As the line is active high,
Do not repeat the obvious or redundant parts. There is no point in
saying that "GPIO spec is a GPIO spec for ...". It cannot be anything
else than GPIO spec. Instead say something useful. It's confusing to
have two reset pins, so explain what is the purpose of this pin.
> + it should be marked GPIO_ACTIVE_HIGH.
Drop last part "it should be marked", because it is clearly incorrect.
Different board designs can have it differently.
> + maxItems: 1
> +
> + reset-gpios:
> + description: GPIO spec for the RESET pin. As the line is active low, it
> + should be marked GPIO_ACTIVE_LOW.
Again, marking it always as active low is not correct. It is enough to
say that line is active low.
> + maxItems: 1
> +
> + sdn-io-gpios:
> + description: GPIO spec for the SHUTDOWN INPUT/OUTPUT pin. As the line is
> + active high, it should be marked GPIO_ACTIVE_HIGH.
What's the purpose?
> + maxItems: 1
> +
> +required:
> + - compatible
> + - reg
> + - clocks
> +
> +allOf:
> + - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> +
> + spi {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + dac@0 {
> + compatible = "adi,ad8460";
> + reg = <0>;
> + spi-max-frequency = <8000000>;
> + adi,rset-ohms = <2000>;
> + adi,range-microvolt = <(-40000000) 40000000>;
> + adi,range-microamp = <0 50000>;
> + adi,temp-max-millicelsius = <50000>;
Custom properties go to the end. See DTS coding style.
> +
> + dmas = <&tx_dma 0>;
> + dma-names = "tx";
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] iio: dac: support the ad8460 Waveform DAC
2024-07-30 3:05 ` [PATCH v2 2/2] iio: dac: support the ad8460 Waveform DAC Mariel Tinaco
@ 2024-08-02 6:21 ` kernel test robot
2024-08-02 8:04 ` kernel test robot
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2024-08-02 6:21 UTC (permalink / raw)
To: Mariel Tinaco, linux-iio, devicetree, linux-kernel,
Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Michael Hennerich, Conor Dooley,
Marcelo Schmitt, Dimitri Fedrau, David Lechner, Nuno Sá
Cc: oe-kbuild-all
Hi Mariel,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 9900e7a54764998ba3a22f06ec629f7b5fe0b422]
url: https://github.com/intel-lab-lkp/linux/commits/Mariel-Tinaco/dt-bindings-iio-dac-add-docs-for-ad8460/20240730-112724
base: 9900e7a54764998ba3a22f06ec629f7b5fe0b422
patch link: https://lore.kernel.org/r/20240730030509.57834-3-Mariel.Tinaco%40analog.com
patch subject: [PATCH v2 2/2] iio: dac: support the ad8460 Waveform DAC
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20240802/202408021321.uic81edY-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240802/202408021321.uic81edY-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408021321.uic81edY-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/iio/dac/ad8460.c: In function 'ad8460_dac_input_read':
drivers/iio/dac/ad8460.c:159:15: error: implicit declaration of function 'ad8460_get_hvdac_word' [-Werror=implicit-function-declaration]
159 | ret = ad8460_get_hvdac_word(state, private, ®);
| ^~~~~~~~~~~~~~~~~~~~~
>> drivers/iio/dac/ad8460.c:163:35: warning: format '%ld' expects argument of type 'long int', but argument 3 has type 'unsigned int' [-Wformat=]
163 | return sysfs_emit(buf, "%ld\n", reg);
| ~~^ ~~~
| | |
| | unsigned int
| long int
| %d
drivers/iio/dac/ad8460.c: In function 'ad8460_dac_input_write':
>> drivers/iio/dac/ad8460.c:176:30: warning: passing argument 2 of 'kstrtou32' makes integer from pointer without a cast [-Wint-conversion]
176 | ret = kstrtou32(buf, ®);
| ^~~~
| |
| unsigned int *
In file included from include/linux/kernel.h:25,
from include/linux/clk.h:13,
from drivers/iio/dac/ad8460.c:10:
include/linux/kstrtox.h:84:70: note: expected 'unsigned int' but argument is of type 'unsigned int *'
84 | static inline int __must_check kstrtou32(const char *s, unsigned int base, u32 *res)
| ~~~~~~~~~~~~~^~~~
drivers/iio/dac/ad8460.c:176:15: error: too few arguments to function 'kstrtou32'
176 | ret = kstrtou32(buf, ®);
| ^~~~~~~~~
include/linux/kstrtox.h:84:32: note: declared here
84 | static inline int __must_check kstrtou32(const char *s, unsigned int base, u32 *res)
| ^~~~~~~~~
drivers/iio/dac/ad8460.c:182:16: error: implicit declaration of function 'ad8460_set_hvdac_word' [-Werror=implicit-function-declaration]
182 | return ad8460_set_hvdac_word(state, private, reg);
| ^~~~~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad8460.c: In function 'ad8460_write_symbol':
>> drivers/iio/dac/ad8460.c:211:30: warning: passing argument 2 of 'kstrtou16' makes integer from pointer without a cast [-Wint-conversion]
211 | ret = kstrtou16(buf, &sym);
| ^~~~
| |
| bool * {aka _Bool *}
include/linux/kstrtox.h:94:56: note: expected 'unsigned int' but argument is of type 'bool *' {aka '_Bool *'}
94 | int __must_check kstrtou16(const char *s, unsigned int base, u16 *res);
| ~~~~~~~~~~~~~^~~~
drivers/iio/dac/ad8460.c:211:15: error: too few arguments to function 'kstrtou16'
211 | ret = kstrtou16(buf, &sym);
| ^~~~~~~~~
include/linux/kstrtox.h:94:18: note: declared here
94 | int __must_check kstrtou16(const char *s, unsigned int base, u16 *res);
| ^~~~~~~~~
drivers/iio/dac/ad8460.c: In function 'ad8460_write_toggle_en':
drivers/iio/dac/ad8460.c:249:30: warning: passing argument 2 of 'kstrtou16' makes integer from pointer without a cast [-Wint-conversion]
249 | ret = kstrtou16(buf, &toggle_en);
| ^~~~~~~~~~
| |
| bool * {aka _Bool *}
include/linux/kstrtox.h:94:56: note: expected 'unsigned int' but argument is of type 'bool *' {aka '_Bool *'}
94 | int __must_check kstrtou16(const char *s, unsigned int base, u16 *res);
| ~~~~~~~~~~~~~^~~~
drivers/iio/dac/ad8460.c:249:15: error: too few arguments to function 'kstrtou16'
249 | ret = kstrtou16(buf, &toggle_en);
| ^~~~~~~~~
include/linux/kstrtox.h:94:18: note: declared here
94 | int __must_check kstrtou16(const char *s, unsigned int base, u16 *res);
| ^~~~~~~~~
drivers/iio/dac/ad8460.c: At top level:
drivers/iio/dac/ad8460.c:335:12: error: static declaration of 'ad8460_get_hvdac_word' follows non-static declaration
335 | static int ad8460_get_hvdac_word(struct ad8460_state *state,
| ^~~~~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad8460.c:159:15: note: previous implicit declaration of 'ad8460_get_hvdac_word' with type 'int()'
159 | ret = ad8460_get_hvdac_word(state, private, ®);
| ^~~~~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad8460.c: In function 'ad8460_get_hvdac_word':
drivers/iio/dac/ad8460.c:346:16: error: implicit declaration of function 'get_unaligned_le16' [-Werror=implicit-function-declaration]
346 | *val = get_unaligned_le16(state->spi_tx_buf);
| ^~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad8460.c: At top level:
drivers/iio/dac/ad8460.c:351:12: error: static declaration of 'ad8460_set_hvdac_word' follows non-static declaration
351 | static int ad8460_set_hvdac_word(struct ad8460_state *state,
| ^~~~~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad8460.c:182:16: note: previous implicit declaration of 'ad8460_set_hvdac_word' with type 'int()'
182 | return ad8460_set_hvdac_word(state, private, reg);
| ^~~~~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad8460.c: In function 'ad8460_set_hvdac_word':
drivers/iio/dac/ad8460.c:355:9: error: implicit declaration of function 'put_unaligned_le16' [-Werror=implicit-function-declaration]
355 | put_unaligned_le16(val & 0x3FFF, &state->spi_tx_buf);
| ^~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad8460.c: In function 'ad8460_probe':
drivers/iio/dac/ad8460.c:855:15: error: implicit declaration of function 'devm_regulator_get_enable_read_voltage'; did you mean 'devm_regulator_get_enable_optional'? [-Werror=implicit-function-declaration]
855 | ret = devm_regulator_get_enable_read_voltage(&spi->dev, "refio_1p2v");
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| devm_regulator_get_enable_optional
drivers/iio/dac/ad8460.c:859:57: error: 'vrefio' undeclared (first use in this function)
859 | return dev_err_probe(&spi->dev, PTR_ERR(vrefio),
| ^~~~~~
drivers/iio/dac/ad8460.c:859:57: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/iio/dac/ad8460.c:819:27: warning: unused variable 'refio_1p2v' [-Wunused-variable]
819 | struct regulator *refio_1p2v;
| ^~~~~~~~~~
cc1: some warnings being treated as errors
vim +163 drivers/iio/dac/ad8460.c
149
150 static ssize_t ad8460_dac_input_read(struct iio_dev *indio_dev,
151 uintptr_t private,
152 const struct iio_chan_spec *chan,
153 char *buf)
154 {
155 struct ad8460_state *state = iio_priv(indio_dev);
156 unsigned int reg;
157 int ret;
158
159 ret = ad8460_get_hvdac_word(state, private, ®);
160 if (ret)
161 return ret;
162
> 163 return sysfs_emit(buf, "%ld\n", reg);
164 }
165
166 static ssize_t ad8460_dac_input_write(struct iio_dev *indio_dev,
167 uintptr_t private,
168 const struct iio_chan_spec *chan,
169 const char *buf,
170 size_t len)
171 {
172 struct ad8460_state *state = iio_priv(indio_dev);
173 unsigned int reg;
174 int ret;
175
> 176 ret = kstrtou32(buf, ®);
177 if (ret)
178 return ret;
179
180 guard(mutex)(&state->lock);
181
182 return ad8460_set_hvdac_word(state, private, reg);
183 }
184
185 static ssize_t ad8460_read_symbol(struct iio_dev *indio_dev,
186 uintptr_t private,
187 const struct iio_chan_spec *chan,
188 char *buf)
189 {
190 struct ad8460_state *state = iio_priv(indio_dev);
191 unsigned int reg;
192 int ret;
193
194 ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x02), ®);
195 if (ret)
196 return ret;
197
198 return sysfs_emit(buf, "%ld\n", FIELD_GET(AD8460_PATTERN_DEPTH_MSK, reg));
199 }
200
201 static ssize_t ad8460_write_symbol(struct iio_dev *indio_dev,
202 uintptr_t private,
203 const struct iio_chan_spec *chan,
204 const char *buf,
205 size_t len)
206 {
207 struct ad8460_state *state = iio_priv(indio_dev);
208 bool sym;
209 int ret;
210
> 211 ret = kstrtou16(buf, &sym);
212 if (ret)
213 return ret;
214
215 guard(mutex)(&state->lock);
216
217 return regmap_update_bits(state->regmap,
218 AD8460_CTRL_REG(0x02),
219 AD8460_PATTERN_DEPTH_MSK,
220 FIELD_PREP(AD8460_PATTERN_DEPTH_MSK, sym));
221 }
222
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] iio: dac: support the ad8460 Waveform DAC
2024-07-30 3:05 ` [PATCH v2 2/2] iio: dac: support the ad8460 Waveform DAC Mariel Tinaco
2024-08-02 6:21 ` kernel test robot
@ 2024-08-02 8:04 ` kernel test robot
2024-08-02 9:46 ` kernel test robot
2024-08-03 10:29 ` Jonathan Cameron
3 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2024-08-02 8:04 UTC (permalink / raw)
To: Mariel Tinaco, linux-iio, devicetree, linux-kernel,
Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Michael Hennerich, Conor Dooley,
Marcelo Schmitt, Dimitri Fedrau, David Lechner, Nuno Sá
Cc: llvm, oe-kbuild-all
Hi Mariel,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 9900e7a54764998ba3a22f06ec629f7b5fe0b422]
url: https://github.com/intel-lab-lkp/linux/commits/Mariel-Tinaco/dt-bindings-iio-dac-add-docs-for-ad8460/20240730-112724
base: 9900e7a54764998ba3a22f06ec629f7b5fe0b422
patch link: https://lore.kernel.org/r/20240730030509.57834-3-Mariel.Tinaco%40analog.com
patch subject: [PATCH v2 2/2] iio: dac: support the ad8460 Waveform DAC
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20240802/202408021509.ug75TMoS-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240802/202408021509.ug75TMoS-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408021509.ug75TMoS-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/iio/dac/ad8460.c:159:8: error: call to undeclared function 'ad8460_get_hvdac_word'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
159 | ret = ad8460_get_hvdac_word(state, private, ®);
| ^
>> drivers/iio/dac/ad8460.c:163:34: warning: format specifies type 'long' but the argument has type 'unsigned int' [-Wformat]
163 | return sysfs_emit(buf, "%ld\n", reg);
| ~~~ ^~~
| %u
drivers/iio/dac/ad8460.c:176:27: error: too few arguments to function call, expected 3, have 2
176 | ret = kstrtou32(buf, ®);
| ~~~~~~~~~ ^
include/linux/kstrtox.h:84:32: note: 'kstrtou32' declared here
84 | static inline int __must_check kstrtou32(const char *s, unsigned int base, u32 *res)
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad8460.c:182:9: error: call to undeclared function 'ad8460_set_hvdac_word'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
182 | return ad8460_set_hvdac_word(state, private, reg);
| ^
drivers/iio/dac/ad8460.c:211:27: error: too few arguments to function call, expected 3, have 2
211 | ret = kstrtou16(buf, &sym);
| ~~~~~~~~~ ^
include/linux/kstrtox.h:94:18: note: 'kstrtou16' declared here
94 | int __must_check kstrtou16(const char *s, unsigned int base, u16 *res);
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad8460.c:249:33: error: too few arguments to function call, expected 3, have 2
249 | ret = kstrtou16(buf, &toggle_en);
| ~~~~~~~~~ ^
include/linux/kstrtox.h:94:18: note: 'kstrtou16' declared here
94 | int __must_check kstrtou16(const char *s, unsigned int base, u16 *res);
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad8460.c:335:12: error: static declaration of 'ad8460_get_hvdac_word' follows non-static declaration
335 | static int ad8460_get_hvdac_word(struct ad8460_state *state,
| ^
drivers/iio/dac/ad8460.c:159:8: note: previous implicit declaration is here
159 | ret = ad8460_get_hvdac_word(state, private, ®);
| ^
drivers/iio/dac/ad8460.c:346:9: error: call to undeclared function 'get_unaligned_le16'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
346 | *val = get_unaligned_le16(state->spi_tx_buf);
| ^
drivers/iio/dac/ad8460.c:351:12: error: static declaration of 'ad8460_set_hvdac_word' follows non-static declaration
351 | static int ad8460_set_hvdac_word(struct ad8460_state *state,
| ^
drivers/iio/dac/ad8460.c:182:9: note: previous implicit declaration is here
182 | return ad8460_set_hvdac_word(state, private, reg);
| ^
drivers/iio/dac/ad8460.c:355:2: error: call to undeclared function 'put_unaligned_le16'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
355 | put_unaligned_le16(val & 0x3FFF, &state->spi_tx_buf);
| ^
drivers/iio/dac/ad8460.c:855:8: error: call to undeclared function 'devm_regulator_get_enable_read_voltage'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
855 | ret = devm_regulator_get_enable_read_voltage(&spi->dev, "refio_1p2v");
| ^
drivers/iio/dac/ad8460.c:855:8: note: did you mean 'devm_regulator_get_enable_optional'?
include/linux/regulator/consumer.h:166:5: note: 'devm_regulator_get_enable_optional' declared here
166 | int devm_regulator_get_enable_optional(struct device *dev, const char *id);
| ^
drivers/iio/dac/ad8460.c:859:43: error: use of undeclared identifier 'vrefio'
859 | return dev_err_probe(&spi->dev, PTR_ERR(vrefio),
| ^
1 warning and 11 errors generated.
vim +163 drivers/iio/dac/ad8460.c
149
150 static ssize_t ad8460_dac_input_read(struct iio_dev *indio_dev,
151 uintptr_t private,
152 const struct iio_chan_spec *chan,
153 char *buf)
154 {
155 struct ad8460_state *state = iio_priv(indio_dev);
156 unsigned int reg;
157 int ret;
158
159 ret = ad8460_get_hvdac_word(state, private, ®);
160 if (ret)
161 return ret;
162
> 163 return sysfs_emit(buf, "%ld\n", reg);
164 }
165
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] iio: dac: support the ad8460 Waveform DAC
2024-07-30 3:05 ` [PATCH v2 2/2] iio: dac: support the ad8460 Waveform DAC Mariel Tinaco
2024-08-02 6:21 ` kernel test robot
2024-08-02 8:04 ` kernel test robot
@ 2024-08-02 9:46 ` kernel test robot
2024-08-03 10:29 ` Jonathan Cameron
3 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2024-08-02 9:46 UTC (permalink / raw)
To: Mariel Tinaco, linux-iio, devicetree, linux-kernel,
Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Michael Hennerich, Conor Dooley,
Marcelo Schmitt, Dimitri Fedrau, David Lechner, Nuno Sá
Cc: oe-kbuild-all
Hi Mariel,
kernel test robot noticed the following build errors:
[auto build test ERROR on 9900e7a54764998ba3a22f06ec629f7b5fe0b422]
url: https://github.com/intel-lab-lkp/linux/commits/Mariel-Tinaco/dt-bindings-iio-dac-add-docs-for-ad8460/20240730-112724
base: 9900e7a54764998ba3a22f06ec629f7b5fe0b422
patch link: https://lore.kernel.org/r/20240730030509.57834-3-Mariel.Tinaco%40analog.com
patch subject: [PATCH v2 2/2] iio: dac: support the ad8460 Waveform DAC
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20240802/202408021737.KMIdEjmt-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240802/202408021737.KMIdEjmt-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408021737.KMIdEjmt-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/iio/dac/ad8460.c: In function 'ad8460_dac_input_read':
>> drivers/iio/dac/ad8460.c:159:15: error: implicit declaration of function 'ad8460_get_hvdac_word' [-Werror=implicit-function-declaration]
159 | ret = ad8460_get_hvdac_word(state, private, ®);
| ^~~~~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad8460.c:163:35: warning: format '%ld' expects argument of type 'long int', but argument 3 has type 'unsigned int' [-Wformat=]
163 | return sysfs_emit(buf, "%ld\n", reg);
| ~~^ ~~~
| | |
| | unsigned int
| long int
| %d
drivers/iio/dac/ad8460.c: In function 'ad8460_dac_input_write':
drivers/iio/dac/ad8460.c:176:30: warning: passing argument 2 of 'kstrtou32' makes integer from pointer without a cast [-Wint-conversion]
176 | ret = kstrtou32(buf, ®);
| ^~~~
| |
| unsigned int *
In file included from include/linux/kernel.h:25,
from include/linux/clk.h:13,
from drivers/iio/dac/ad8460.c:10:
include/linux/kstrtox.h:84:70: note: expected 'unsigned int' but argument is of type 'unsigned int *'
84 | static inline int __must_check kstrtou32(const char *s, unsigned int base, u32 *res)
| ~~~~~~~~~~~~~^~~~
>> drivers/iio/dac/ad8460.c:176:15: error: too few arguments to function 'kstrtou32'
176 | ret = kstrtou32(buf, ®);
| ^~~~~~~~~
include/linux/kstrtox.h:84:32: note: declared here
84 | static inline int __must_check kstrtou32(const char *s, unsigned int base, u32 *res)
| ^~~~~~~~~
>> drivers/iio/dac/ad8460.c:182:16: error: implicit declaration of function 'ad8460_set_hvdac_word' [-Werror=implicit-function-declaration]
182 | return ad8460_set_hvdac_word(state, private, reg);
| ^~~~~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad8460.c: In function 'ad8460_write_symbol':
drivers/iio/dac/ad8460.c:211:30: warning: passing argument 2 of 'kstrtou16' makes integer from pointer without a cast [-Wint-conversion]
211 | ret = kstrtou16(buf, &sym);
| ^~~~
| |
| bool * {aka _Bool *}
include/linux/kstrtox.h:94:56: note: expected 'unsigned int' but argument is of type 'bool *' {aka '_Bool *'}
94 | int __must_check kstrtou16(const char *s, unsigned int base, u16 *res);
| ~~~~~~~~~~~~~^~~~
>> drivers/iio/dac/ad8460.c:211:15: error: too few arguments to function 'kstrtou16'
211 | ret = kstrtou16(buf, &sym);
| ^~~~~~~~~
include/linux/kstrtox.h:94:18: note: declared here
94 | int __must_check kstrtou16(const char *s, unsigned int base, u16 *res);
| ^~~~~~~~~
drivers/iio/dac/ad8460.c: In function 'ad8460_write_toggle_en':
drivers/iio/dac/ad8460.c:249:30: warning: passing argument 2 of 'kstrtou16' makes integer from pointer without a cast [-Wint-conversion]
249 | ret = kstrtou16(buf, &toggle_en);
| ^~~~~~~~~~
| |
| bool * {aka _Bool *}
include/linux/kstrtox.h:94:56: note: expected 'unsigned int' but argument is of type 'bool *' {aka '_Bool *'}
94 | int __must_check kstrtou16(const char *s, unsigned int base, u16 *res);
| ~~~~~~~~~~~~~^~~~
drivers/iio/dac/ad8460.c:249:15: error: too few arguments to function 'kstrtou16'
249 | ret = kstrtou16(buf, &toggle_en);
| ^~~~~~~~~
include/linux/kstrtox.h:94:18: note: declared here
94 | int __must_check kstrtou16(const char *s, unsigned int base, u16 *res);
| ^~~~~~~~~
drivers/iio/dac/ad8460.c: At top level:
>> drivers/iio/dac/ad8460.c:335:12: error: static declaration of 'ad8460_get_hvdac_word' follows non-static declaration
335 | static int ad8460_get_hvdac_word(struct ad8460_state *state,
| ^~~~~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad8460.c:159:15: note: previous implicit declaration of 'ad8460_get_hvdac_word' with type 'int()'
159 | ret = ad8460_get_hvdac_word(state, private, ®);
| ^~~~~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad8460.c: In function 'ad8460_get_hvdac_word':
>> drivers/iio/dac/ad8460.c:346:16: error: implicit declaration of function 'get_unaligned_le16' [-Werror=implicit-function-declaration]
346 | *val = get_unaligned_le16(state->spi_tx_buf);
| ^~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad8460.c: At top level:
>> drivers/iio/dac/ad8460.c:351:12: error: static declaration of 'ad8460_set_hvdac_word' follows non-static declaration
351 | static int ad8460_set_hvdac_word(struct ad8460_state *state,
| ^~~~~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad8460.c:182:16: note: previous implicit declaration of 'ad8460_set_hvdac_word' with type 'int()'
182 | return ad8460_set_hvdac_word(state, private, reg);
| ^~~~~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad8460.c: In function 'ad8460_set_hvdac_word':
>> drivers/iio/dac/ad8460.c:355:9: error: implicit declaration of function 'put_unaligned_le16' [-Werror=implicit-function-declaration]
355 | put_unaligned_le16(val & 0x3FFF, &state->spi_tx_buf);
| ^~~~~~~~~~~~~~~~~~
drivers/iio/dac/ad8460.c: In function 'ad8460_probe':
>> drivers/iio/dac/ad8460.c:855:15: error: implicit declaration of function 'devm_regulator_get_enable_read_voltage'; did you mean 'devm_regulator_get_enable_optional'? [-Werror=implicit-function-declaration]
855 | ret = devm_regulator_get_enable_read_voltage(&spi->dev, "refio_1p2v");
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| devm_regulator_get_enable_optional
>> drivers/iio/dac/ad8460.c:859:57: error: 'vrefio' undeclared (first use in this function)
859 | return dev_err_probe(&spi->dev, PTR_ERR(vrefio),
| ^~~~~~
drivers/iio/dac/ad8460.c:859:57: note: each undeclared identifier is reported only once for each function it appears in
drivers/iio/dac/ad8460.c:819:27: warning: unused variable 'refio_1p2v' [-Wunused-variable]
819 | struct regulator *refio_1p2v;
| ^~~~~~~~~~
cc1: some warnings being treated as errors
vim +/ad8460_get_hvdac_word +159 drivers/iio/dac/ad8460.c
149
150 static ssize_t ad8460_dac_input_read(struct iio_dev *indio_dev,
151 uintptr_t private,
152 const struct iio_chan_spec *chan,
153 char *buf)
154 {
155 struct ad8460_state *state = iio_priv(indio_dev);
156 unsigned int reg;
157 int ret;
158
> 159 ret = ad8460_get_hvdac_word(state, private, ®);
160 if (ret)
161 return ret;
162
163 return sysfs_emit(buf, "%ld\n", reg);
164 }
165
166 static ssize_t ad8460_dac_input_write(struct iio_dev *indio_dev,
167 uintptr_t private,
168 const struct iio_chan_spec *chan,
169 const char *buf,
170 size_t len)
171 {
172 struct ad8460_state *state = iio_priv(indio_dev);
173 unsigned int reg;
174 int ret;
175
> 176 ret = kstrtou32(buf, ®);
177 if (ret)
178 return ret;
179
180 guard(mutex)(&state->lock);
181
> 182 return ad8460_set_hvdac_word(state, private, reg);
183 }
184
185 static ssize_t ad8460_read_symbol(struct iio_dev *indio_dev,
186 uintptr_t private,
187 const struct iio_chan_spec *chan,
188 char *buf)
189 {
190 struct ad8460_state *state = iio_priv(indio_dev);
191 unsigned int reg;
192 int ret;
193
194 ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x02), ®);
195 if (ret)
196 return ret;
197
198 return sysfs_emit(buf, "%ld\n", FIELD_GET(AD8460_PATTERN_DEPTH_MSK, reg));
199 }
200
201 static ssize_t ad8460_write_symbol(struct iio_dev *indio_dev,
202 uintptr_t private,
203 const struct iio_chan_spec *chan,
204 const char *buf,
205 size_t len)
206 {
207 struct ad8460_state *state = iio_priv(indio_dev);
208 bool sym;
209 int ret;
210
> 211 ret = kstrtou16(buf, &sym);
212 if (ret)
213 return ret;
214
215 guard(mutex)(&state->lock);
216
217 return regmap_update_bits(state->regmap,
218 AD8460_CTRL_REG(0x02),
219 AD8460_PATTERN_DEPTH_MSK,
220 FIELD_PREP(AD8460_PATTERN_DEPTH_MSK, sym));
221 }
222
223 static ssize_t ad8460_read_toggle_en(struct iio_dev *indio_dev,
224 uintptr_t private,
225 const struct iio_chan_spec *chan,
226 char *buf)
227 {
228 struct ad8460_state *state = iio_priv(indio_dev);
229 unsigned int reg;
230 int ret;
231
232 ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x02), ®);
233 if (ret)
234 return ret;
235
236 return sysfs_emit(buf, "%ld\n", FIELD_GET(AD8460_APG_MODE_ENABLE_MSK, reg));
237 }
238
239 static ssize_t ad8460_write_toggle_en(struct iio_dev *indio_dev,
240 uintptr_t private,
241 const struct iio_chan_spec *chan,
242 const char *buf,
243 size_t len)
244 {
245 struct ad8460_state *state = iio_priv(indio_dev);
246 bool toggle_en;
247 int ret;
248
249 ret = kstrtou16(buf, &toggle_en);
250 if (ret)
251 return ret;
252
253 iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
254 return ad8460_enable_apg_mode(state, toggle_en);
255 unreachable();
256 }
257
258 static ssize_t ad8460_read_powerdown(struct iio_dev *indio_dev,
259 uintptr_t private,
260 const struct iio_chan_spec *chan,
261 char *buf)
262 {
263 struct ad8460_state *state = iio_priv(indio_dev);
264 unsigned int reg;
265 int ret;
266
267 ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x01), ®);
268 if (ret)
269 return ret;
270
271 return sysfs_emit(buf, "%ld\n", FIELD_GET(AD8460_HVDAC_SLEEP_MSK, reg));
272 }
273
274 static ssize_t ad8460_write_powerdown(struct iio_dev *indio_dev,
275 uintptr_t private,
276 const struct iio_chan_spec *chan,
277 const char *buf,
278 size_t len)
279 {
280 struct ad8460_state *state = iio_priv(indio_dev);
281 bool pwr_down;
282 u64 sdn_flag;
283 int ret;
284
285 ret = kstrtobool(buf, &pwr_down);
286 if (ret)
287 return ret;
288
289 guard(mutex)(&state->lock);
290
291 ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01),
292 AD8460_HVDAC_SLEEP_MSK,
293 FIELD_PREP(AD8460_HVDAC_SLEEP_MSK, pwr_down));
294 if (ret)
295 return ret;
296
297 if (!pwr_down) {
298 ret = ad8460_read_shutdown_flag(state, &sdn_flag);
299 if (ret)
300 return ret;
301
302 if (sdn_flag) {
303 ret = ad8460_hv_reset(state);
304 if (ret)
305 return ret;
306 }
307 }
308
309 ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x00),
310 AD8460_HV_SLEEP_MSK,
311 FIELD_PREP(AD8460_HV_SLEEP_MSK, !pwr_down));
312 if (ret)
313 return ret;
314
315 return len;
316 }
317
318 static const char * const ad8460_powerdown_modes[] = {
319 "three_state",
320 };
321
322 static int ad8460_get_powerdown_mode(struct iio_dev *indio_dev,
323 const struct iio_chan_spec *chan)
324 {
325 return 0;
326 }
327
328 static int ad8460_set_powerdown_mode(struct iio_dev *indio_dev,
329 const struct iio_chan_spec *chan,
330 unsigned int type)
331 {
332 return 0;
333 }
334
> 335 static int ad8460_get_hvdac_word(struct ad8460_state *state,
336 int index,
337 int *val)
338 {
339 int ret;
340
341 ret = regmap_bulk_read(state->regmap, AD8460_HVDAC_DATA_WORD_LOW(index),
342 &state->spi_tx_buf, AD8460_DATA_BYTE_WORD_LENGTH);
343 if (ret)
344 return ret;
345
> 346 *val = get_unaligned_le16(state->spi_tx_buf);
347
348 return ret;
349 }
350
> 351 static int ad8460_set_hvdac_word(struct ad8460_state *state,
352 int index,
353 int val)
354 {
> 355 put_unaligned_le16(val & 0x3FFF, &state->spi_tx_buf);
356
357 return regmap_bulk_write(state->regmap, AD8460_HVDAC_DATA_WORD_LOW(index),
358 state->spi_tx_buf, AD8460_DATA_BYTE_WORD_LENGTH);
359 }
360
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: iio: dac: add docs for ad8460
2024-07-30 3:05 ` [PATCH v2 1/2] dt-bindings: iio: dac: add docs for ad8460 Mariel Tinaco
2024-07-30 6:15 ` Krzysztof Kozlowski
@ 2024-08-03 10:05 ` Jonathan Cameron
2024-08-17 11:19 ` Tinaco, Mariel
1 sibling, 1 reply; 13+ messages in thread
From: Jonathan Cameron @ 2024-08-03 10:05 UTC (permalink / raw)
To: Mariel Tinaco
Cc: linux-iio, devicetree, linux-kernel, Lars-Peter Clausen,
Rob Herring, Krzysztof Kozlowski, Michael Hennerich, Conor Dooley,
Marcelo Schmitt, Dimitri Fedrau, David Lechner, Nuno Sá
On Tue, 30 Jul 2024 11:05:08 +0800
Mariel Tinaco <Mariel.Tinaco@analog.com> wrote:
> This adds the bindings documentation for the 14-bit
> High Voltage, High Current, Waveform Generator
> Digital-to-Analog converter.
>
A few additions to Krzysztof's much more detailed review.
Wrap patch descriptions to 75 chars. not sub 55.
> Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com>
> +
> + adi,rset-ohms:
Please rename this as rset sounds like reset to me. Not sure what a
good name is however!
> + description: Specify value of external resistor connected to FS_ADJ pin
> + to establish internal HVDAC's reference current I_REF
> + default: 2000
> + minimum: 2000
> + maximum: 20000
> +
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] iio: dac: support the ad8460 Waveform DAC
2024-07-30 3:05 ` [PATCH v2 2/2] iio: dac: support the ad8460 Waveform DAC Mariel Tinaco
` (2 preceding siblings ...)
2024-08-02 9:46 ` kernel test robot
@ 2024-08-03 10:29 ` Jonathan Cameron
2024-08-17 11:59 ` Tinaco, Mariel
3 siblings, 1 reply; 13+ messages in thread
From: Jonathan Cameron @ 2024-08-03 10:29 UTC (permalink / raw)
To: Mariel Tinaco
Cc: linux-iio, devicetree, linux-kernel, Lars-Peter Clausen,
Rob Herring, Krzysztof Kozlowski, Michael Hennerich, Conor Dooley,
Marcelo Schmitt, Dimitri Fedrau, David Lechner, Nuno Sá
On Tue, 30 Jul 2024 11:05:09 +0800
Mariel Tinaco <Mariel.Tinaco@analog.com> wrote:
> The AD8460 is a “bits in, power out” high voltage, high-power,
> high-speed driver optimized for large output current (up to ±1 A)
> and high slew rate (up to ±1800 V/μs) at high voltage (up to ±40 V)
> into capacitive loads.
>
> A digital engine implements user-configurable features: modes for
> digital input, programmable supply current, and fault monitoring
> and programmable protection settings for output current,
> output voltage, and junction temperature. The AD8460 operates on
> high voltage dual supplies up to ±55 V and a single low voltage
> supply of 5 V.
>
> Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com>
Hi Mariel,
The test bot detected problems don't give me a high degree of confidence
in this driver in general. Please be much more careful and thorough in
build tests for v3.
Various minor comments inline
Jonathan
> diff --git a/drivers/iio/dac/ad8460.c b/drivers/iio/dac/ad8460.c
> new file mode 100644
> index 000000000..a94fa4526
> --- /dev/null
> +++ b/drivers/iio/dac/ad8460.c
> @@ -0,0 +1,976 @@
> +static int ad8460_set_hvdac_word(struct ad8460_state *state,
> + int index,
> + int val)
> +{
> + put_unaligned_le16(val & 0x3FFF, &state->spi_tx_buf);
GENMASK for that constant.
> +
> + return regmap_bulk_write(state->regmap, AD8460_HVDAC_DATA_WORD_LOW(index),
> + state->spi_tx_buf, AD8460_DATA_BYTE_WORD_LENGTH);
> +}
> +
> +static int ad8460_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val,
> + int val2,
> + long mask)
Aim for wrapping to below 80 chars, not much shorter.
Feel free to group parameters though if you want to.
> +{
> + struct ad8460_state *state = iio_priv(indio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + switch (chan->type) {
> + case IIO_VOLTAGE:
> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
> + return ad8460_set_sample(state, val);
> + unreachable();
> + case IIO_CURRENT:
> + return regmap_write(state->regmap, AD8460_CTRL_REG(0x04),
> + FIELD_PREP(AD8460_SET_QUIESCENT_CURRENT_MSK, val));
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int ad8460_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val,
> + int *val2,
> + long mask)
> +{
> + struct ad8460_state *state = iio_priv(indio_dev);
> + int data, ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + switch (chan->type) {
> + case IIO_VOLTAGE:
> + scoped_guard(mutex, &state->lock) {
> + ret = ad8460_get_hvdac_word(state, 0, &data);
> + if (ret)
> + return ret;
> + }
> + *val = data;
> + return IIO_VAL_INT;
> + case IIO_CURRENT:
> + ret = regmap_read(state->regmap, AD8460_CTRL_REG(0x04), &data);
> + if (ret)
> + return ret;
> + *val = data;
> + return IIO_VAL_INT;
> + case IIO_TEMP:
> + ret = iio_read_channel_raw(state->tmp_adc_channel, &data);
> + if (ret)
> + return ret;
> + *val = data;
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *val = clk_get_rate(state->sync_clk);
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + /* vCONV = vNOMINAL_SPAN * (DAC_CODE / 2**14) - 40V
/*
* vCONV ...
for IIO multiline comment syntax.
> + * vMAX = vNOMINAL_SPAN * (2**14 / 2**14) - 40V
> + * vMIN = vNOMINAL_SPAN * (0 / 2**14) - 40V
> + * vADJ = vCONV * (2000 / rSET) * (vREF / 1.2)
> + * vSPAN = vADJ_MAX - vADJ_MIN
> + * See datasheet page 49, section FULL-SCALE REDUCTION
> + */
> + *val = AD8460_NOMINAL_VOLTAGE_SPAN * 2000 * state->refio_1p2v_mv;
> + *val2 = state->rset_ohms * 1200;
> + return IIO_VAL_FRACTIONAL;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int ad8460_select_fault_type(int chan_type, enum iio_event_direction dir)
> +{
> + switch (chan_type) {
> + case IIO_VOLTAGE:
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + return AD8460_OVERVOLTAGE_POS;
> + case IIO_EV_DIR_FALLING:
> + return AD8460_OVERVOLTAGE_NEG;
> + default:
> + return -EINVAL;
> + }
> + break;
as below.
> + case IIO_CURRENT:
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + return AD8460_OVERCURRENT_SRC;
> + case IIO_EV_DIR_FALLING:
> + return AD8460_OVERCURRENT_SNK;
> + default:
> + return -EINVAL;
> + }
> + break;
Can't get here. So drop the break.
> + case IIO_TEMP:
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + return AD8460_OVERTEMPERATURE;
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +static int ad8460_read_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir)
> +{
> + struct ad8460_state *state = iio_priv(indio_dev);
> + unsigned int fault;
> + bool en;
> + int ret;
> +
> + if (type != IIO_EV_TYPE_THRESH)
> + return -EINVAL;
> +
> + fault = ad8460_select_fault_type(chan->type, dir);
> + if (fault < 0)
> + return fault;
> +
> + ret = ad8460_get_fault_threshold_en(state, fault, &en);
> + return ret ?: en;
Keep to simpler to read
if (ret)
return ret;
return en;
> +}
> +
> +static struct iio_chan_spec_ext_info ad8460_ext_info[] = {
> + AD8460_CHAN_EXT_INFO("raw0", 0, IIO_SEPARATE,
> + ad8460_dac_input_read, ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw1", 1, IIO_SEPARATE,
> + ad8460_dac_input_read, ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw2", 2, IIO_SEPARATE,
> + ad8460_dac_input_read, ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw3", 3, IIO_SEPARATE,
> + ad8460_dac_input_read, ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw4", 4, IIO_SEPARATE,
> + ad8460_dac_input_read, ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw5", 5, IIO_SEPARATE,
> + ad8460_dac_input_read, ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw6", 6, IIO_SEPARATE,
> + ad8460_dac_input_read, ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw7", 7, IIO_SEPARATE,
> + ad8460_dac_input_read, ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw8", 8, IIO_SEPARATE,
> + ad8460_dac_input_read, ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw9", 9, IIO_SEPARATE,
> + ad8460_dac_input_read, ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw10", 10, IIO_SEPARATE,
> + ad8460_dac_input_read, ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw11", 11, IIO_SEPARATE,
> + ad8460_dac_input_read, ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw12", 12, IIO_SEPARATE,
> + ad8460_dac_input_read, ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw13", 13, IIO_SEPARATE,
> + ad8460_dac_input_read, ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw14", 14, IIO_SEPARATE,
> + ad8460_dac_input_read, ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("raw15", 15, IIO_SEPARATE,
> + ad8460_dac_input_read, ad8460_dac_input_write),
> + AD8460_CHAN_EXT_INFO("toggle_en", 0, IIO_SEPARATE,
> + ad8460_read_toggle_en,
> + ad8460_write_toggle_en),
> + AD8460_CHAN_EXT_INFO("symbol", 0, IIO_SEPARATE,
> + ad8460_read_symbol,
> + ad8460_write_symbol),
Wrap closer to 80 chars.
> + AD8460_CHAN_EXT_INFO("powerdown", 0, IIO_SEPARATE,
> + ad8460_read_powerdown, ad8460_write_powerdown),
> + IIO_ENUM("powerdown_mode", IIO_SEPARATE, &ad8460_powerdown_mode_enum),
> + IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE,
> + &ad8460_powerdown_mode_enum),
> + {}
> +};
> +
> +static const struct iio_event_spec ad8460_events[] = {
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_RISING,
> + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> + BIT(IIO_EV_INFO_ENABLE),
> + },
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_FALLING,
> + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> + BIT(IIO_EV_INFO_ENABLE),
> + },
> +};
> +
> +#define AD8460_VOLTAGE_CHAN(_chan) { \
> + .type = IIO_VOLTAGE, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> + BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE), \
> + .output = 1, \
> + .indexed = 1, \
> + .channel = (_chan), \
> + .scan_index = 0, \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 14, \
> + .storagebits = 16, \
> + .endianness = IIO_LE, \
> + }, \
> + .ext_info = ad8460_ext_info, \
> + .event_spec = ad8460_events, \
> + .num_event_specs = ARRAY_SIZE(ad8460_events), \
> +}
> +
> +#define AD8460_CURRENT_CHAN(_chan) { \
> + .type = IIO_CURRENT, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .output = 0, \
I'm guessing this is also not used with the buffer.
set scan_index = -1 if so.
> + .indexed = 1, \
> + .channel = (_chan), \
> + .scan_index = 1, \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 8, \
> + .storagebits = 8, \
> + .endianness = IIO_LE, \
Generally don't provide these if you aren't using them for buffered
capture. It's just unnecessary noise in the driver.
> + }, \
> + .event_spec = ad8460_events, \
> + .num_event_specs = ARRAY_SIZE(ad8460_events), \
> +}
> +
> +#define AD8460_TEMP_CHAN(_chan) { \
> + .type = IIO_TEMP, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .output = 1, \
> + .indexed = 1, \
> + .channel = (_chan), \
> + .scan_index = 2, \
> + .scan_type = { \
No sizing?
I suspect you don't want to present this for the buffered interface.
If so, set .scan_index = -1
> + .sign = 'u', \
> + .endianness = IIO_LE, \
> + }, \
> + .event_spec = ad8460_events, \
> + .num_event_specs = 1, \
> +}
> +
> +static const struct iio_chan_spec ad8460_channels[] = {
> + AD8460_VOLTAGE_CHAN(0),
> + AD8460_CURRENT_CHAN(0),
> +};
> +
> +static const struct iio_chan_spec ad8460_channels_with_tmp_adc[] = {
> + AD8460_VOLTAGE_CHAN(0),
> + AD8460_CURRENT_CHAN(0),
> + AD8460_TEMP_CHAN(0),
> +};
Unless you plan to very soon add support for devices with more channels,
drop the parameter and hardcode 0 in the definitions.
Chances are that if you do get a more complex device with more channels
you will need more than just a channel number anyway so this code
will change either way. Hence better to keep it simple for now.
> +static int ad8460_probe(struct spi_device *spi)
> +{
> + struct ad8460_state *state;
> + struct iio_dev *indio_dev;
> + struct regulator *refio_1p2v;
> + u32 tmp[2], temp;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + state = iio_priv(indio_dev);
> + mutex_init(&state->lock);
> +
> + indio_dev->name = "ad8460";
> + indio_dev->info = &ad8460_info;
> +
> + state->spi = spi;
> +
> + state->regmap = devm_regmap_init_spi(spi, &ad8460_regmap_config);
> + if (IS_ERR(state->regmap))
> + return dev_err_probe(&spi->dev, PTR_ERR(state->regmap),
> + "Failed to initialize regmap");
> +
> + state->sync_clk = devm_clk_get_enabled(&spi->dev, NULL);
Lots of use of spi->dev, I'd suggest
struct device *dev = &spi->dev;
then replace all these instances with that local variable.
> + if (IS_ERR(state->sync_clk))
> + return dev_err_probe(&spi->dev, PTR_ERR(state->sync_clk),
> + "Failed to get sync clk\n");
> +
> + state->tmp_adc_channel = devm_iio_channel_get(&spi->dev, "ad8460-tmp");
> + if (IS_ERR_OR_NULL(state->tmp_adc_channel)) {
> + indio_dev->channels = ad8460_channels;
> + indio_dev->num_channels = ARRAY_SIZE(ad8460_channels);
> + } else {
> + indio_dev->channels = ad8460_channels_with_tmp_adc;
> + indio_dev->num_channels = ARRAY_SIZE(ad8460_channels_with_tmp_adc);
> + dev_info(&spi->dev, "Found ADC channel to read TMP pin\n");
That will be apparent anyway once driver is registered, so don't fill the log
with messages like this. dev_dbg() perhaps or drop it entirely.
> + }
> +
> + ret = devm_regulator_get_enable_read_voltage(&spi->dev, "refio_1p2v");
> + if (ret == -ENODEV) {
> + state->refio_1p2v_mv = 1200;
> + } else if (ret < 0) {
> + return dev_err_probe(&spi->dev, PTR_ERR(vrefio),
> + "Failed to get reference voltage\n");
> + } else {
> + state->refio_1p2v_mv = ret / 1000;
> + }
> +
> + if (!in_range(ret, AD8460_MIN_VREFIO_UV, AD8460_MAX_VREFIO_UV))
> + return dev_err_probe(&spi->dev, -EINVAL,
> + "Invalid ref voltage range(%u mV) [%u mV, %u mV]\n",
> + ret, AD8460_MIN_VREFIO_UV / 1000,
Why print ret rather than state->refio_1p2v_mv?
Having a dev_err_probe() with ret as a later parameter is rather confusing
if nothing else.
> + AD8460_MAX_VREFIO_UV / 1000);
> +
> + ret = device_property_read_u32(&spi->dev, "adi,rset-ohms", &state->rset_ohms);
> + if (ret) {
> + state->rset_ohms = 2000;
> + } else {
> + if (!in_range(state->rset_ohms, AD8460_MIN_RSET_OHMS,
> + AD8460_MAX_RSET_OHMS))
Might as well combine as
else if (!...
Then can also drop some brackets as single statements in each leg.
> + return dev_err_probe(&spi->dev, -EINVAL,
> + "Invalid resistor set range(%u) [%u, %u]\n",
> + state->rset_ohms,
> + AD8460_MIN_RSET_OHMS,
> + AD8460_MAX_RSET_OHMS);
> + }
> +
> + /* Arm the device by default */
> + ret = device_property_read_u32_array(&spi->dev, "adi,range-microamp",
> + tmp, ARRAY_SIZE(tmp));
> + if (!ret) {
> + ret = regmap_write(state->regmap, AD8460_CTRL_REG(0x08),
> + (1 << 7) | AD8460_CURRENT_LIMIT_CONV(tmp[1]));
What is the (1 << 7)? Feels like a magic number that should have a define.
> + if (ret)
> + return dev_err_probe(&spi->dev, -EINVAL,
ret not -EINVAL;
> + "overcurrent src not valid: %d uA",
> + tmp[1]);
> +
> + ret = regmap_write(state->regmap, AD8460_CTRL_REG(0x09),
> + (1 << 7) | AD8460_CURRENT_LIMIT_CONV(abs(tmp[0])));
> + if (ret)
> + return dev_err_probe(&spi->dev, -EINVAL,
same here and everywhere else where you are eating a potentially more meaningful error
value.
> + "overcurrent snk not valid: %d uA",
> + tmp[0]);
> + }
> +
> + ret = device_property_read_u32_array(&spi->dev, "adi,range-microvolt",
> + tmp, ARRAY_SIZE(tmp));
> + if (!ret) {
This is currently documented in the binding as only taking one set of values.
Seems a lot more flexible here.
> + ret = regmap_write(state->regmap, AD8460_CTRL_REG(0x0A),
> + (1 << 7) | AD8460_VOLTAGE_LIMIT_CONV(tmp[1]));
> + if (ret)
> + return dev_err_probe(&spi->dev, -EINVAL,
> + "positive overvoltage not valid: %d uV",
> + tmp[1]);
> +
> + ret = regmap_write(state->regmap, AD8460_CTRL_REG(0x0B),
> + (1 << 7) | AD8460_VOLTAGE_LIMIT_CONV(abs(tmp[0])));
> + if (ret)
> + return dev_err_probe(&spi->dev, -EINVAL,
> + "negative overvoltage not valid: %d uV",
> + tmp[0]);
> + }
> +
> + ret = device_property_read_u32(&spi->dev, "adi,temp-max-millicelsius", &temp);
I'd handled as a default of whatever is already in that
register. Just write temp = default; before this call.
That way you just don't check ret for the property query. If it's there the
value in temp will be updated, if not we'll have the default.
Consider similar for the other cases.
It will make it easier to tell from the driver what happens if a property is not
provided. Also document defaults in the dt binding.
> + if (!ret) {
> + ret = regmap_write(state->regmap, AD8460_CTRL_REG(0x0C),
> + (1 << 7) | AD8460_TEMP_LIMIT_CONV(abs(temp)));
> + if (ret)
> + return dev_err_probe(&spi->dev, -EINVAL,
> + "overtemperature not valid: %d",
> + temp);
> + }
> +
> + ret = ad8460_reset(state);
> + if (ret)
> + return ret;
> +
> + /* Enables DAC by default */
> + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01),
> + AD8460_HVDAC_SLEEP_MSK,
> + FIELD_PREP(AD8460_HVDAC_SLEEP_MSK, 0));
> + if (ret)
> + return ret;
> +
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->setup_ops = &ad8460_buffer_setup_ops;
> +
> + ret = devm_iio_dmaengine_buffer_setup_ext(&spi->dev, indio_dev, "tx",
> + IIO_BUFFER_DIRECTION_OUT);
> + if (ret)
> + return dev_err_probe(&spi->dev, ret,
> + "Failed to get DMA buffer\n");
> +
> + ret = devm_iio_device_register(&spi->dev, indio_dev);
return devm_iio_device_register()
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v2 1/2] dt-bindings: iio: dac: add docs for ad8460
2024-07-30 6:15 ` Krzysztof Kozlowski
@ 2024-08-17 11:18 ` Tinaco, Mariel
2024-08-18 7:04 ` Krzysztof Kozlowski
0 siblings, 1 reply; 13+ messages in thread
From: Tinaco, Mariel @ 2024-08-17 11:18 UTC (permalink / raw)
To: Krzysztof Kozlowski, linux-iio@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Hennerich, Michael, Conor Dooley,
Marcelo Schmitt, Dimitri Fedrau, David Lechner, Nuno Sá
> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Tuesday, July 30, 2024 2:15 PM
> To: Tinaco, Mariel <Mariel.Tinaco@analog.com>; linux-iio@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Jonathan Cameron
> <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>; Rob Herring
> <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Hennerich,
> Michael <Michael.Hennerich@analog.com>; Conor Dooley
> <conor+dt@kernel.org>; Marcelo Schmitt <marcelo.schmitt1@gmail.com>;
> Dimitri Fedrau <dima.fedrau@gmail.com>; David Lechner
> <dlechner@baylibre.com>; Nuno Sá <noname.nuno@gmail.com>
> Subject: Re: [PATCH v2 1/2] dt-bindings: iio: dac: add docs for ad8460
>
> [External]
>
> On 30/07/2024 05:05, Mariel Tinaco wrote:
> > This adds the bindings documentation for the 14-bit
> > High Voltage, High Current, Waveform Generator
> > Digital-to-Analog converter.
> >
> > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com>
>
> > +
> > + refio-1p2v-supply:
> > + description: Drive voltage in the range of 1.2V maximum to as low as
> > + low as 0.12V through the REF_IO pin to adjust full scale output span
> > +
> > + clocks:
>
> maxItems: 1
> and drop description (or use items: - description, but then do not
> repeat redundant parts)
>
Simplified the description for this item
refio-1p2v-supply:
description: Reference voltage to adjust full scale output span
maxItems: 1
Should I put, "maxItems: 1" in the other vrefs as well?
> > + description: The clock for the DAC. This is the sync clock
> > +
> > + adi,rset-ohms:
> > + description: Specify value of external resistor connected to FS_ADJ pin
> > + to establish internal HVDAC's reference current I_REF
> > + default: 2000
> > + minimum: 2000
> > + maximum: 20000
> > +
> > + adi,range-microvolt:
> > + description: |
> > + Voltage output range specified as <minimum, maximum>
> > + oneOf:
>
> Not an oneOf.
>
You're right. I should have put all the possible values. I populated it with common
Values found in the datasheet
adi,range-microvolt:
description: Voltage output range specified as <minimum, maximum>
oneOf:
- items:
- enum: [0, -10000000, -20000000, -30000000, -40000000, -55000000]
- enum: [10000000, 20000000, 30000000, 40000000, 55000000]
> > + - items:
> > + - const: -40000000
> > + - const: 40000000
>
> Why do you need this property if this cannot be anything else? Drop.
>
> > +
> > + adi,range-microamp:
> > + description: |
>
> Do not need '|' unless you need to preserve formatting.
>
> > + Current output range specified as <minimum, maximum>
> > + oneOf:
> > + - items:
> > + - const: 0
> > + - const: 50000
> > + - items:
> > + - const: -50000
> > + - const: 50000
> > +
> > + adi,temp-max-millicelsius:
> > + description: Overtemperature threshold
> > + default: 50000
> > + minimum: 20000
> > + maximum: 150000
> > +
> > + sdn-reset-gpios:
>
> reset-gpios or shutdown-gpios or anything from gpio-consumer-common
> which is not deprecated.
>
Lifted from gpio-consumer-common yaml. Mapped to corresponding pins
wakeup-gpios:
description: Corresponds to SDN_RESET pin. To exit shutdown
or sleep mode, pulse SDN_RESET HIGH, then leave LOW.
maxItems: 1
reset-gpios:
description: Manual Power On Reset (POR). Pull this GPIO pin
LOW and then HIGH to reset all digital registers to default
maxItems: 1
shutdown-gpios:
description: Corresponds to SDN_IO pin. Shutdown may be
initiated by the user, by pulsing SDN_IO high. To exit shutdown,
pulse SDN_IO low, then float.
maxItems: 1
Replaced description based on the datasheet
> > + description: GPIO spec for the SHUTDOWN RESET pin. As the line is active
> high,
>
> Do not repeat the obvious or redundant parts. There is no point in
> saying that "GPIO spec is a GPIO spec for ...". It cannot be anything
> else than GPIO spec. Instead say something useful. It's confusing to
> have two reset pins, so explain what is the purpose of this pin.
>
> > + it should be marked GPIO_ACTIVE_HIGH.
>
> Drop last part "it should be marked", because it is clearly incorrect.
> Different board designs can have it differently.
>
>
> > + maxItems: 1
> > +
> > + reset-gpios:
> > + description: GPIO spec for the RESET pin. As the line is active low, it
> > + should be marked GPIO_ACTIVE_LOW.
>
> Again, marking it always as active low is not correct. It is enough to
> say that line is active low.
>
> > + maxItems: 1
> > +
> > + sdn-io-gpios:
> > + description: GPIO spec for the SHUTDOWN INPUT/OUTPUT pin. As the
> line is
> > + active high, it should be marked GPIO_ACTIVE_HIGH.
>
> What's the purpose?
>
> > + maxItems: 1
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - clocks
> > +
> > +allOf:
> > + - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/gpio/gpio.h>
> > +
> > + spi {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + dac@0 {
> > + compatible = "adi,ad8460";
> > + reg = <0>;
> > + spi-max-frequency = <8000000>;
> > + adi,rset-ohms = <2000>;
> > + adi,range-microvolt = <(-40000000) 40000000>;
> > + adi,range-microamp = <0 50000>;
> > + adi,temp-max-millicelsius = <50000>;
>
> Custom properties go to the end. See DTS coding style.
>
Moved custom attributes in the end
examples:
- |
#include <dt-bindings/gpio/gpio.h>
spi {
#address-cells = <1>;
#size-cells = <0>;
dac@0 {
compatible = "adi,ad8460";
reg = <0>;
spi-max-frequency = <8000000>;
dmas = <&tx_dma 0>;
dma-names = "tx";
wakeup-gpios = <&gpio 86 GPIO_ACTIVE_HIGH>;
reset-gpios = <&gpio 91 GPIO_ACTIVE_LOW>;
shutdown-gpios = <&gpio 88 GPIO_ACTIVE_HIGH>;
clocks = <&sync_ext_clk>;
hvcc-supply = <&hvcc>;
hvee-supply = <&hvee>;
vcc-5v-supply = <&vcc_5>;
vref-5v-supply = <&vref_5>;
dvdd-3p3v-supply = <&dvdd_3_3>;
avdd-3p3v-supply = <&avdd_3_3>;
refio-1p2v-supply = <&refio_1_2>;
adi,external-resistor-ohms = <2000>;
adi,range-microvolt = <(-40000000) 40000000>;
adi,range-microamp = <0 50000>;
adi,max-millicelsius = <50000>;
};
};
> > +
> > + dmas = <&tx_dma 0>;
> > + dma-names = "tx";
>
>
> Best regards,
> Krzysztof
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v2 1/2] dt-bindings: iio: dac: add docs for ad8460
2024-08-03 10:05 ` Jonathan Cameron
@ 2024-08-17 11:19 ` Tinaco, Mariel
0 siblings, 0 replies; 13+ messages in thread
From: Tinaco, Mariel @ 2024-08-17 11:19 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Hennerich, Michael, Conor Dooley,
Marcelo Schmitt, Dimitri Fedrau, David Lechner, Nuno Sá
> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, August 3, 2024 6:06 PM
> To: Tinaco, Mariel <Mariel.Tinaco@analog.com>
> Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>; Rob Herring
> <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Hennerich,
> Michael <Michael.Hennerich@analog.com>; Conor Dooley
> <conor+dt@kernel.org>; Marcelo Schmitt <marcelo.schmitt1@gmail.com>;
> Dimitri Fedrau <dima.fedrau@gmail.com>; David Lechner
> <dlechner@baylibre.com>; Nuno Sá <noname.nuno@gmail.com>
> Subject: Re: [PATCH v2 1/2] dt-bindings: iio: dac: add docs for ad8460
>
> [External]
>
> On Tue, 30 Jul 2024 11:05:08 +0800
> Mariel Tinaco <Mariel.Tinaco@analog.com> wrote:
>
> > This adds the bindings documentation for the 14-bit High Voltage, High
> > Current, Waveform Generator Digital-to-Analog converter.
> >
> A few additions to Krzysztof's much more detailed review.
>
> Wrap patch descriptions to 75 chars. not sub 55.
>
> > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com>
> > +
> > + adi,rset-ohms:
>
> Please rename this as rset sounds like reset to me. Not sure what a good name
> is however!
>
>
> > + description: Specify value of external resistor connected to FS_ADJ pin
> > + to establish internal HVDAC's reference current I_REF
> > + default: 2000
> > + minimum: 2000
> > + maximum: 20000
> > +
Replaced the name
adi,external-resistor-ohms:
description: Specify value of external resistor connected to FS_ADJ pin
to establish internal HVDAC's reference current I_REF
default: 2000
minimum: 2000
maximum: 20000
Regards,
Mariel
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v2 2/2] iio: dac: support the ad8460 Waveform DAC
2024-08-03 10:29 ` Jonathan Cameron
@ 2024-08-17 11:59 ` Tinaco, Mariel
0 siblings, 0 replies; 13+ messages in thread
From: Tinaco, Mariel @ 2024-08-17 11:59 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Hennerich, Michael, Conor Dooley,
Marcelo Schmitt, Dimitri Fedrau, David Lechner, Nuno Sá
> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: Saturday, August 3, 2024 6:30 PM
> To: Tinaco, Mariel <Mariel.Tinaco@analog.com>
> Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; Lars-Peter Clausen <lars@metafoo.de>; Rob Herring
> <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Hennerich,
> Michael <Michael.Hennerich@analog.com>; Conor Dooley
> <conor+dt@kernel.org>; Marcelo Schmitt <marcelo.schmitt1@gmail.com>;
> Dimitri Fedrau <dima.fedrau@gmail.com>; David Lechner
> <dlechner@baylibre.com>; Nuno Sá <noname.nuno@gmail.com>
> Subject: Re: [PATCH v2 2/2] iio: dac: support the ad8460 Waveform DAC
>
> [External]
>
> On Tue, 30 Jul 2024 11:05:09 +0800
> Mariel Tinaco <Mariel.Tinaco@analog.com> wrote:
>
> > The AD8460 is a “bits in, power out” high voltage, high-power,
> > high-speed driver optimized for large output current (up to ±1 A)
> > and high slew rate (up to ±1800 V/μs) at high voltage (up to ±40 V)
> > into capacitive loads.
> >
> > A digital engine implements user-configurable features: modes for
> > digital input, programmable supply current, and fault monitoring
> > and programmable protection settings for output current,
> > output voltage, and junction temperature. The AD8460 operates on
> > high voltage dual supplies up to ±55 V and a single low voltage
> > supply of 5 V.
> >
> > Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com>
> Hi Mariel,
>
> The test bot detected problems don't give me a high degree of confidence
> in this driver in general. Please be much more careful and thorough in
> build tests for v3.
>
This is a lapse on my end. I'll be more careful the next time. Apologies
> Various minor comments inline
>
> Jonathan
>
> > diff --git a/drivers/iio/dac/ad8460.c b/drivers/iio/dac/ad8460.c
> > new file mode 100644
> > index 000000000..a94fa4526
> > --- /dev/null
> > +++ b/drivers/iio/dac/ad8460.c
> > @@ -0,0 +1,976 @@
>
> > +static int ad8460_set_hvdac_word(struct ad8460_state *state,
> > + int index,
> > + int val)
> > +{
> > + put_unaligned_le16(val & 0x3FFF, &state->spi_tx_buf);
>
> GENMASK for that constant.
>
Created GENMASK for mask of full data byte
#define AD8460_DATA_BYTE_FULL_MSK GENMASK(13, 0)
static int ad8460_set_hvdac_word(struct ad8460_state *state, int index, int val)
{
put_unaligned_le16(FIELD_PREP(AD8460_DATA_BYTE_FULL_MSK, val),
&state->spi_tx_buf);
> > +
> > + return regmap_bulk_write(state->regmap,
> AD8460_HVDAC_DATA_WORD_LOW(index),
> > + state->spi_tx_buf,
> AD8460_DATA_BYTE_WORD_LENGTH);
> > +}
>
>
> > +
> > +static int ad8460_write_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int val,
> > + int val2,
> > + long mask)
>
> Aim for wrapping to below 80 chars, not much shorter.
> Feel free to group parameters though if you want to.
>
Fitted most lines to 80 chars
> > +{
> > + struct ad8460_state *state = iio_priv(indio_dev);
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + switch (chan->type) {
> > + case IIO_VOLTAGE:
> > + iio_device_claim_direct_scoped(return -EBUSY,
> indio_dev)
> > + return ad8460_set_sample(state, val);
> > + unreachable();
> > + case IIO_CURRENT:
> > + return regmap_write(state->regmap,
> AD8460_CTRL_REG(0x04),
> > +
> FIELD_PREP(AD8460_SET_QUIESCENT_CURRENT_MSK, val));
> > + default:
> > + return -EINVAL;
> > + }
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int ad8460_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val,
> > + int *val2,
> > + long mask)
> > +{
> > + struct ad8460_state *state = iio_priv(indio_dev);
> > + int data, ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + switch (chan->type) {
> > + case IIO_VOLTAGE:
> > + scoped_guard(mutex, &state->lock) {
> > + ret = ad8460_get_hvdac_word(state, 0,
> &data);
> > + if (ret)
> > + return ret;
> > + }
> > + *val = data;
> > + return IIO_VAL_INT;
> > + case IIO_CURRENT:
> > + ret = regmap_read(state->regmap,
> AD8460_CTRL_REG(0x04), &data);
> > + if (ret)
> > + return ret;
> > + *val = data;
> > + return IIO_VAL_INT;
> > + case IIO_TEMP:
> > + ret = iio_read_channel_raw(state->tmp_adc_channel,
> &data);
> > + if (ret)
> > + return ret;
> > + *val = data;
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > + case IIO_CHAN_INFO_SAMP_FREQ:
> > + *val = clk_get_rate(state->sync_clk);
> > + return IIO_VAL_INT;
> > + case IIO_CHAN_INFO_SCALE:
> > + /* vCONV = vNOMINAL_SPAN * (DAC_CODE / 2**14) - 40V
> /*
> * vCONV ...
>
> for IIO multiline comment syntax.
>
Followed syntax
case IIO_CHAN_INFO_SCALE:
/*
* vCONV = vNOMINAL_SPAN * (DAC_CODE / 2**14) - 40V
* vMAX = vNOMINAL_SPAN * (2**14 / 2**14) - 40V
* vMIN = vNOMINAL_SPAN * (0 / 2**14) - 40V
* vADJ = vCONV * (2000 / rSET) * (vREF / 1.2)
* vSPAN = vADJ_MAX - vADJ_MIN
* See datasheet page 49, section FULL-SCALE REDUCTION
*/
> > + * vMAX = vNOMINAL_SPAN * (2**14 / 2**14) - 40V
> > + * vMIN = vNOMINAL_SPAN * (0 / 2**14) - 40V
> > + * vADJ = vCONV * (2000 / rSET) * (vREF / 1.2)
> > + * vSPAN = vADJ_MAX - vADJ_MIN
> > + * See datasheet page 49, section FULL-SCALE REDUCTION
> > + */
> > + *val = AD8460_NOMINAL_VOLTAGE_SPAN * 2000 * state-
> >refio_1p2v_mv;
> > + *val2 = state->rset_ohms * 1200;
> > + return IIO_VAL_FRACTIONAL;
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static int ad8460_select_fault_type(int chan_type, enum
> iio_event_direction dir)
> > +{
> > + switch (chan_type) {
> > + case IIO_VOLTAGE:
> > + switch (dir) {
> > + case IIO_EV_DIR_RISING:
> > + return AD8460_OVERVOLTAGE_POS;
> > + case IIO_EV_DIR_FALLING:
> > + return AD8460_OVERVOLTAGE_NEG;
> > + default:
> > + return -EINVAL;
> > + }
> > + break;
> as below.
>
> > + case IIO_CURRENT:
> > + switch (dir) {
> > + case IIO_EV_DIR_RISING:
> > + return AD8460_OVERCURRENT_SRC;
> > + case IIO_EV_DIR_FALLING:
> > + return AD8460_OVERCURRENT_SNK;
> > + default:
> > + return -EINVAL;
> > + }
> > + break;
> Can't get here. So drop the break.
> > + case IIO_TEMP:
> > + switch (dir) {
> > + case IIO_EV_DIR_RISING:
> > + return AD8460_OVERTEMPERATURE;
> > + default:
> > + return -EINVAL;
> > + }
> > + default:
> > + return -EINVAL;
> > + }
> > +}
>
> > +static int ad8460_read_event_config(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir)
> > +{
> > + struct ad8460_state *state = iio_priv(indio_dev);
> > + unsigned int fault;
> > + bool en;
> > + int ret;
> > +
> > + if (type != IIO_EV_TYPE_THRESH)
> > + return -EINVAL;
> > +
> > + fault = ad8460_select_fault_type(chan->type, dir);
> > + if (fault < 0)
> > + return fault;
> > +
> > + ret = ad8460_get_fault_threshold_en(state, fault, &en);
> > + return ret ?: en;
>
> Keep to simpler to read
> if (ret)
> return ret;
>
> return en;
>
> > +}
>
> > +
> > +static struct iio_chan_spec_ext_info ad8460_ext_info[] = {
> > + AD8460_CHAN_EXT_INFO("raw0", 0, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw1", 1, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw2", 2, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw3", 3, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw4", 4, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw5", 5, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw6", 6, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw7", 7, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw8", 8, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw9", 9, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw10", 10, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw11", 11, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw12", 12, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw13", 13, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw14", 14, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw15", 15, IIO_SEPARATE,
> > + ad8460_dac_input_read, ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("toggle_en", 0, IIO_SEPARATE,
> > + ad8460_read_toggle_en,
> > + ad8460_write_toggle_en),
> > + AD8460_CHAN_EXT_INFO("symbol", 0, IIO_SEPARATE,
> > + ad8460_read_symbol,
> > + ad8460_write_symbol),
>
> Wrap closer to 80 chars.
>
> > + AD8460_CHAN_EXT_INFO("powerdown", 0, IIO_SEPARATE,
> > + ad8460_read_powerdown,
> ad8460_write_powerdown),
> > + IIO_ENUM("powerdown_mode", IIO_SEPARATE,
> &ad8460_powerdown_mode_enum),
> > + IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE,
> > + &ad8460_powerdown_mode_enum),
> > + {}
> > +};
> > +
> > +static const struct iio_event_spec ad8460_events[] = {
> > + {
> > + .type = IIO_EV_TYPE_THRESH,
> > + .dir = IIO_EV_DIR_RISING,
> > + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> > + BIT(IIO_EV_INFO_ENABLE),
> > + },
> > + {
> > + .type = IIO_EV_TYPE_THRESH,
> > + .dir = IIO_EV_DIR_FALLING,
> > + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
> > + BIT(IIO_EV_INFO_ENABLE),
> > + },
> > +};
> > +
> > +#define AD8460_VOLTAGE_CHAN(_chan) { \
> > + .type = IIO_VOLTAGE, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> > + BIT(IIO_CHAN_INFO_RAW) | \
> > + BIT(IIO_CHAN_INFO_SCALE), \
> > + .output = 1, \
> > + .indexed = 1, \
> > + .channel = (_chan), \
> > + .scan_index = 0, \
> > + .scan_type = { \
> > + .sign = 'u', \
> > + .realbits = 14, \
> > + .storagebits = 16, \
> > + .endianness = IIO_LE, \
> > + }, \
> > + .ext_info = ad8460_ext_info, \
> > + .event_spec = ad8460_events, \
> > + .num_event_specs = ARRAY_SIZE(ad8460_events), \
> > +}
> > +
> > +#define AD8460_CURRENT_CHAN(_chan) { \
> > + .type = IIO_CURRENT, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> > + .output = 0, \
>
> I'm guessing this is also not used with the buffer.
> set scan_index = -1 if so.
>
> > + .indexed = 1, \
> > + .channel = (_chan), \
> > + .scan_index = 1, \
> > + .scan_type = { \
> > + .sign = 'u', \
> > + .realbits = 8, \
> > + .storagebits = 8, \
> > + .endianness = IIO_LE, \
> Generally don't provide these if you aren't using them for buffered
> capture. It's just unnecessary noise in the driver.
>
> > + }, \
> > + .event_spec = ad8460_events, \
> > + .num_event_specs = ARRAY_SIZE(ad8460_events), \
> > +}
> > +
> > +#define AD8460_TEMP_CHAN(_chan) { \
> > + .type = IIO_TEMP, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> > + .output = 1, \
> > + .indexed = 1, \
> > + .channel = (_chan), \
> > + .scan_index = 2, \
> > + .scan_type = { \
> No sizing?
> I suspect you don't want to present this for the buffered interface.
> If so, set .scan_index = -1
>
Refactored the channel macros like thise
#define AD8460_VOLTAGE_CHAN { \
.type = IIO_VOLTAGE, \
.info_mask_separate = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
BIT(IIO_CHAN_INFO_RAW) | \
BIT(IIO_CHAN_INFO_SCALE), \
.output = 1, \
.indexed = 1, \
.channel = 0, \
.scan_index = 0, \
.scan_type = { \
.sign = 'u', \
.realbits = 14, \
.storagebits = 16, \
.endianness = IIO_LE, \
}, \
.ext_info = ad8460_ext_info, \
.event_spec = ad8460_events, \
.num_event_specs = ARRAY_SIZE(ad8460_events), \
}
#define AD8460_CURRENT_CHAN { \
.type = IIO_CURRENT, \
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
.output = 0, \
.indexed = 1, \
.channel = 0, \
.scan_index = -1, \
.event_spec = ad8460_events, \
.num_event_specs = ARRAY_SIZE(ad8460_events), \
}
#define AD8460_TEMP_CHAN { \
.type = IIO_TEMP, \
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
.output = 1, \
.indexed = 1, \
.channel = 0, \
.scan_index = -1, \
.event_spec = ad8460_events, \
.num_event_specs = 1, \
}
> > + .sign = 'u', \
> > + .endianness = IIO_LE, \
> > + }, \
> > + .event_spec = ad8460_events, \
> > + .num_event_specs = 1, \
> > +}
> > +
> > +static const struct iio_chan_spec ad8460_channels[] = {
> > + AD8460_VOLTAGE_CHAN(0),
> > + AD8460_CURRENT_CHAN(0),
> > +};
> > +
> > +static const struct iio_chan_spec ad8460_channels_with_tmp_adc[] = {
> > + AD8460_VOLTAGE_CHAN(0),
> > + AD8460_CURRENT_CHAN(0),
> > + AD8460_TEMP_CHAN(0),
> > +};
> Unless you plan to very soon add support for devices with more channels,
> drop the parameter and hardcode 0 in the definitions.
>
> Chances are that if you do get a more complex device with more channels
> you will need more than just a channel number anyway so this code
> will change either way. Hence better to keep it simple for now.
>
Dropped the channel parameter
static const struct iio_chan_spec ad8460_channels[] = {
AD8460_VOLTAGE_CHAN,
AD8460_CURRENT_CHAN,
};
static const struct iio_chan_spec ad8460_channels_with_tmp_adc[] = {
AD8460_VOLTAGE_CHAN,
AD8460_CURRENT_CHAN,
AD8460_TEMP_CHAN,
};
>
> > +static int ad8460_probe(struct spi_device *spi)
> > +{
> > + struct ad8460_state *state;
> > + struct iio_dev *indio_dev;
> > + struct regulator *refio_1p2v;
> > + u32 tmp[2], temp;
> > + int ret;
> > +
> > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + state = iio_priv(indio_dev);
> > + mutex_init(&state->lock);
> > +
> > + indio_dev->name = "ad8460";
> > + indio_dev->info = &ad8460_info;
> > +
> > + state->spi = spi;
> > +
> > + state->regmap = devm_regmap_init_spi(spi,
> &ad8460_regmap_config);
> > + if (IS_ERR(state->regmap))
> > + return dev_err_probe(&spi->dev, PTR_ERR(state->regmap),
> > + "Failed to initialize regmap");
> > +
> > + state->sync_clk = devm_clk_get_enabled(&spi->dev, NULL);
>
> Lots of use of spi->dev, I'd suggest
> struct device *dev = &spi->dev;
> then replace all these instances with that local variable.
>
Applied this structure
struct device *dev;
state->spi = spi;
dev = &spi->dev;
state->regmap = devm_regmap_init_spi(spi, &ad8460_regmap_config);
if (IS_ERR(state->regmap))
return dev_err_probe(dev, PTR_ERR(state->regmap),
"Failed to initialize regmap");
> > + if (IS_ERR(state->sync_clk))
> > + return dev_err_probe(&spi->dev, PTR_ERR(state->sync_clk),
> > + "Failed to get sync clk\n");
> > +
> > + state->tmp_adc_channel = devm_iio_channel_get(&spi->dev, "ad8460-
> tmp");
> > + if (IS_ERR_OR_NULL(state->tmp_adc_channel)) {
> > + indio_dev->channels = ad8460_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(ad8460_channels);
> > + } else {
> > + indio_dev->channels = ad8460_channels_with_tmp_adc;
> > + indio_dev->num_channels =
> ARRAY_SIZE(ad8460_channels_with_tmp_adc);
> > + dev_info(&spi->dev, "Found ADC channel to read TMP pin\n");
>
> That will be apparent anyway once driver is registered, so don't fill the log
> with messages like this. dev_dbg() perhaps or drop it entirely.
>
Dropped the dev_info print
> > + }
> > +
> > + ret = devm_regulator_get_enable_read_voltage(&spi->dev,
> "refio_1p2v");
> > + if (ret == -ENODEV) {
> > + state->refio_1p2v_mv = 1200;
> > + } else if (ret < 0) {
> > + return dev_err_probe(&spi->dev, PTR_ERR(vrefio),
> > + "Failed to get reference voltage\n");
> > + } else {
> > + state->refio_1p2v_mv = ret / 1000;
> > + }
> > +
> > + if (!in_range(ret, AD8460_MIN_VREFIO_UV,
> AD8460_MAX_VREFIO_UV))
> > + return dev_err_probe(&spi->dev, -EINVAL,
> > + "Invalid ref voltage range(%u mV) [%u mV,
> %u mV]\n",
> > + ret, AD8460_MIN_VREFIO_UV / 1000,
>
> Why print ret rather than state->refio_1p2v_mv?
> Having a dev_err_probe() with ret as a later parameter is rather confusing
> if nothing else.
>
Applied changes
ret = devm_regulator_get_enable_read_voltage(dev, "refio_1p2v");
if (ret == -ENODEV)
state->refio_1p2v_mv = 1200;
else if (ret >= 0)
state->refio_1p2v_mv = ret / 1000;
else
return dev_err_probe(dev, ret, "Failed to get reference voltage\n");
if (!in_range(state->refio_1p2v_mv, AD8460_MIN_VREFIO_UV,
AD8460_MAX_VREFIO_UV))
return dev_err_probe(dev, -EINVAL,
"Invalid ref voltage range(%u mV) [%u mV, %u mV]\n",
state->refio_1p2v_mv,
AD8460_MIN_VREFIO_UV / 1000,
AD8460_MAX_VREFIO_UV / 1000);
>
> > + AD8460_MAX_VREFIO_UV / 1000);
> > +
> > + ret = device_property_read_u32(&spi->dev, "adi,rset-ohms", &state-
> >rset_ohms);
> > + if (ret) {
> > + state->rset_ohms = 2000;
> > + } else {
> > + if (!in_range(state->rset_ohms, AD8460_MIN_RSET_OHMS,
> > + AD8460_MAX_RSET_OHMS))
>
> Might as well combine as
>
> else if (!...
> Then can also drop some brackets as single statements in each leg.
>
Applied changes.
ret = device_property_read_u32(dev, "adi,external-resistor-ohms",
&state->ext_resistor_ohms);
if (ret)
state->ext_resistor_ohms = 2000;
else if (!in_range(state->ext_resistor_ohms, AD8460_MIN_EXT_RESISTOR_OHMS,
AD8460_MAX_EXT_RESISTOR_OHMS))
return dev_err_probe(dev, -EINVAL,
"Invalid resistor set range(%u) [%u, %u]\n",
state->ext_resistor_ohms,
AD8460_MIN_EXT_RESISTOR_OHMS,
AD8460_MAX_EXT_RESISTOR_OHMS);
>
> > + return dev_err_probe(&spi->dev, -EINVAL,
> > + "Invalid resistor set range(%u) [%u,
> %u]\n",
> > + state->rset_ohms,
> > + AD8460_MIN_RSET_OHMS,
> > + AD8460_MAX_RSET_OHMS);
> > + }
> > +
> > + /* Arm the device by default */
> > + ret = device_property_read_u32_array(&spi->dev, "adi,range-
> microamp",
> > + tmp, ARRAY_SIZE(tmp));
> > + if (!ret) {
> > + ret = regmap_write(state->regmap, AD8460_CTRL_REG(0x08),
> > + (1 << 7) |
> AD8460_CURRENT_LIMIT_CONV(tmp[1]));
>
> What is the (1 << 7)? Feels like a magic number that should have a define.
>
Used FIELD_PREP for this section. Used existing BITFIELD for (1 << 7)
if (!ret) {
if (in_range(tmp[1], 0, AD8460_ABS_MAX_OVERCURRENT_UA))
regmap_write(state->regmap, AD8460_CTRL_REG(0x08),
FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) |
AD8460_CURRENT_LIMIT_CONV(tmp[1]));
> > + if (ret)
> > + return dev_err_probe(&spi->dev, -EINVAL,
> ret not -EINVAL;
>
> > + "overcurrent src not valid: %d uA",
> > + tmp[1]);
> > +
> > + ret = regmap_write(state->regmap, AD8460_CTRL_REG(0x09),
> > + (1 << 7) |
> AD8460_CURRENT_LIMIT_CONV(abs(tmp[0])));
> > + if (ret)
> > + return dev_err_probe(&spi->dev, -EINVAL,
>
> same here and everywhere else where you are eating a potentially more
> meaningful error
> value.
>
> > + "overcurrent snk not valid: %d uA",
> > + tmp[0]);
> > + }
> > +
> > + ret = device_property_read_u32_array(&spi->dev, "adi,range-
> microvolt",
> > + tmp, ARRAY_SIZE(tmp));
> > + if (!ret) {
>
> This is currently documented in the binding as only taking one set of values.
> Seems a lot more flexible here.
>
Changed bindings so that this value would be flexible
> > + ret = regmap_write(state->regmap, AD8460_CTRL_REG(0x0A),
> > + (1 << 7) |
> AD8460_VOLTAGE_LIMIT_CONV(tmp[1]));
> > + if (ret)
> > + return dev_err_probe(&spi->dev, -EINVAL,
> > + "positive overvoltage not valid: %d
> uV",
> > + tmp[1]);
> > +
> > + ret = regmap_write(state->regmap, AD8460_CTRL_REG(0x0B),
> > + (1 << 7) |
> AD8460_VOLTAGE_LIMIT_CONV(abs(tmp[0])));
> > + if (ret)
> > + return dev_err_probe(&spi->dev, -EINVAL,
> > + "negative overvoltage not valid: %d
> uV",
> > + tmp[0]);
> > + }
> > +
> > + ret = device_property_read_u32(&spi->dev, "adi,temp-max-
> millicelsius", &temp);
> I'd handled as a default of whatever is already in that
> register. Just write temp = default; before this call.
>
> That way you just don't check ret for the property query. If it's there the
> value in temp will be updated, if not we'll have the default.
>
> Consider similar for the other cases.
> It will make it easier to tell from the driver what happens if a property is not
> provided. Also document defaults in the dt binding.
>
Regarding the default, I just made sure that the register would only be updated
If the custom attribute is defined in the device tree, and if the value is within
Range, as per the datasheet. If it is not met, the register will not be touched
/* Arm the device by default */
ret = device_property_read_u32_array(dev, "adi,range-microamp",
tmp, ARRAY_SIZE(tmp));
if (!ret) {
if (in_range(tmp[1], 0, AD8460_ABS_MAX_OVERCURRENT_UA))
regmap_write(state->regmap, AD8460_CTRL_REG(0x08),
FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) |
AD8460_CURRENT_LIMIT_CONV(tmp[1]));
if (in_range(tmp[0], -AD8460_ABS_MAX_OVERCURRENT_UA, 0))
regmap_write(state->regmap, AD8460_CTRL_REG(0x09),
FIELD_PREP(AD8460_FAULT_ARM_MSK, 1) |
AD8460_CURRENT_LIMIT_CONV(abs(tmp[0])));
}
Should there be a processing for the return of regmap_write? I didn't handle it here
> > + if (!ret) {
> > + ret = regmap_write(state->regmap, AD8460_CTRL_REG(0x0C),
> > + (1 << 7) |
> AD8460_TEMP_LIMIT_CONV(abs(temp)));
> > + if (ret)
> > + return dev_err_probe(&spi->dev, -EINVAL,
> > + "overtemperature not valid: %d",
> > + temp);
> > + }
> > +
> > + ret = ad8460_reset(state);
> > + if (ret)
> > + return ret;
> > +
> > + /* Enables DAC by default */
> > + ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01),
> > + AD8460_HVDAC_SLEEP_MSK,
> > + FIELD_PREP(AD8460_HVDAC_SLEEP_MSK,
> 0));
> > + if (ret)
> > + return ret;
> > +
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->setup_ops = &ad8460_buffer_setup_ops;
> > +
> > + ret = devm_iio_dmaengine_buffer_setup_ext(&spi->dev, indio_dev,
> "tx",
> > +
> IIO_BUFFER_DIRECTION_OUT);
> > + if (ret)
> > + return dev_err_probe(&spi->dev, ret,
> > + "Failed to get DMA buffer\n");
> > +
> > + ret = devm_iio_device_register(&spi->dev, indio_dev);
>
> return devm_iio_device_register()
>
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
Best regards,
Mariel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: iio: dac: add docs for ad8460
2024-08-17 11:18 ` Tinaco, Mariel
@ 2024-08-18 7:04 ` Krzysztof Kozlowski
0 siblings, 0 replies; 13+ messages in thread
From: Krzysztof Kozlowski @ 2024-08-18 7:04 UTC (permalink / raw)
To: Tinaco, Mariel, linux-iio@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Hennerich, Michael, Conor Dooley,
Marcelo Schmitt, Dimitri Fedrau, David Lechner, Nuno Sá
On 17/08/2024 13:18, Tinaco, Mariel wrote:
>>> + clocks:
>>
>> maxItems: 1
>> and drop description (or use items: - description, but then do not
>> repeat redundant parts)
>>
>
> Simplified the description for this item
>
> refio-1p2v-supply:
> description: Reference voltage to adjust full scale output span
> maxItems: 1
>
> Should I put, "maxItems: 1" in the other vrefs as well?
No, why do you change supply? My comment was under clocks. Comments are
always under specific part of code which is being discussed.
>
>>> + description: The clock for the DAC. This is the sync clock
>>> +
>>> + adi,rset-ohms:
>>> + description: Specify value of external resistor connected to FS_ADJ pin
>>> + to establish internal HVDAC's reference current I_REF
>>> + default: 2000
>>> + minimum: 2000
>>> + maximum: 20000
>>> +
>>> + adi,range-microvolt:
>>> + description: |
>>> + Voltage output range specified as <minimum, maximum>
>>> + oneOf:
>>
>> Not an oneOf.
>>
>
> You're right. I should have put all the possible values. I populated it with common
> Values found in the datasheet
>
> adi,range-microvolt:
> description: Voltage output range specified as <minimum, maximum>
> oneOf:
> - items:
> - enum: [0, -10000000, -20000000, -30000000, -40000000, -55000000]
> - enum: [10000000, 20000000, 30000000, 40000000, 55000000]
>
>>> + - items:
>>> + - const: -40000000
>>> + - const: 40000000
>>
>> Why do you need this property if this cannot be anything else? Drop.
>>
>>> +
>>> + adi,range-microamp:
>>> + description: |
>>
>> Do not need '|' unless you need to preserve formatting.
>>
>>> + Current output range specified as <minimum, maximum>
>>> + oneOf:
>>> + - items:
>>> + - const: 0
>>> + - const: 50000
>>> + - items:
>>> + - const: -50000
>>> + - const: 50000
>>> +
>>> + adi,temp-max-millicelsius:
>>> + description: Overtemperature threshold
>>> + default: 50000
>>> + minimum: 20000
>>> + maximum: 150000
>>> +
>>> + sdn-reset-gpios:
>>
>> reset-gpios or shutdown-gpios or anything from gpio-consumer-common
>> which is not deprecated.
>>
>
> Lifted from gpio-consumer-common yaml. Mapped to corresponding pins
>
> wakeup-gpios:
> description: Corresponds to SDN_RESET pin. To exit shutdown
> or sleep mode, pulse SDN_RESET HIGH, then leave LOW.
That's not a wakeup-gpio. I think my comment was imprecise. You have
something like three reset/shutdown GPIOs, so pick from
gpio-consumer-common the reset and shutdown. Remaining one could stay as
in your original code.
> maxItems: 1
>
> reset-gpios:
> description: Manual Power On Reset (POR). Pull this GPIO pin
> LOW and then HIGH to reset all digital registers to default
> maxItems: 1
>
> shutdown-gpios:
> description: Corresponds to SDN_IO pin. Shutdown may be
> initiated by the user, by pulsing SDN_IO high. To exit shutdown,
> pulse SDN_IO low, then float.
> maxItems: 1
>
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2024-08-18 7:05 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-30 3:05 [PATCH v2 0/2] add AD8460 DAC driver Mariel Tinaco
2024-07-30 3:05 ` [PATCH v2 1/2] dt-bindings: iio: dac: add docs for ad8460 Mariel Tinaco
2024-07-30 6:15 ` Krzysztof Kozlowski
2024-08-17 11:18 ` Tinaco, Mariel
2024-08-18 7:04 ` Krzysztof Kozlowski
2024-08-03 10:05 ` Jonathan Cameron
2024-08-17 11:19 ` Tinaco, Mariel
2024-07-30 3:05 ` [PATCH v2 2/2] iio: dac: support the ad8460 Waveform DAC Mariel Tinaco
2024-08-02 6:21 ` kernel test robot
2024-08-02 8:04 ` kernel test robot
2024-08-02 9:46 ` kernel test robot
2024-08-03 10:29 ` Jonathan Cameron
2024-08-17 11:59 ` Tinaco, Mariel
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).