This patch series extends the SPI bitbang, gpio, and spi-engine controllers to
support configurable MOSI line idle state.
It then introduces the ad4000 driver which makes use of the MOSI idle
configuration to properly support AD4000 series of ADCs.
Change log v2 -> v3:
- [new patch] Extended SPI modes to allow MOSI idle high configuration.
- [new patch] Implemented configurable MOSI idle state for spi-bitbang controller.
- [new patch] Implemented configurable MOSI idle state for spi-gpio controller.
- [new patch] Implemented configurable MOSI idle state for spi-axi-spi-engine controller.
- Dropped spi-cpha property (these devices communicate in SPI mode 0).
- Added dt-binding check to constrain adi,gain-milli property to ADAQ devices only.
- Device config doesn't enable TURBO anymore to save power.
- Fixed device buffer declaration.
- Simplified ADC input gain handling by keeping gain value from DT.
- Reworked ADC sample read function in "3-wire mode" making it latency free.
- Added ADC sample read function for "4-wire mode".
- Read adi,spi-mode dt prop and use "3-wire" or "4-wire" mode accordingly.
- Many other minor improvements such as better code execution flows and comments.
- Removed Mircea Caprioru from authors. This driver is now completely different
from what Mircea left in ADI Linux.
I'll leave additional ADC features (e.g. single-ended to differential configuration)
for future patches if no one minds it.
Thanks,
Marcelo
Marcelo Schmitt (6):
spi: Add SPI mode bit for MOSI idle state configuration
spi: bitbang: Implement support for MOSI idle state configuration
spi: spi-gpio: Add support for MOSI idle state configuration
spi: spi-axi-spi-engine: Add support for MOSI idle configuration
dt-bindings: iio: adc: Add AD4000
iio: adc: Add support for AD4000
.../bindings/iio/adc/adi,ad4000.yaml | 207 +++++
MAINTAINERS | 8 +
drivers/iio/adc/Kconfig | 12 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ad4000.c | 735 ++++++++++++++++++
drivers/spi/spi-axi-spi-engine.c | 8 +-
drivers/spi/spi-bitbang.c | 24 +
drivers/spi/spi-gpio.c | 12 +-
drivers/spi/spi.c | 6 +
include/linux/spi/spi_bitbang.h | 1 +
include/uapi/linux/spi/spi.h | 3 +-
11 files changed, 1014 insertions(+), 3 deletions(-)
create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
create mode 100644 drivers/iio/adc/ad4000.c
--
2.43.0
The behavior of an SPI controller data output line (SDO or MOSI or COPI
(Controller Output Peripheral Input) for disambiguation) is not specified
when the controller is not clocking out data on SCLK edges. However, there
exist SPI peripherals that require specific COPI line state when data is
not being clocked out of the controller.
Add SPI mode bit to allow pheripherals to request explicit COPI idle
behavior when needed.
Signed-off-by: Marcelo Schmitt <[email protected]>
---
drivers/spi/spi.c | 6 ++++++
include/uapi/linux/spi/spi.h | 3 ++-
2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 289feccca376..6072b6e93bef 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -3921,6 +3921,12 @@ int spi_setup(struct spi_device *spi)
(SPI_TX_DUAL | SPI_TX_QUAD | SPI_TX_OCTAL |
SPI_RX_DUAL | SPI_RX_QUAD | SPI_RX_OCTAL)))
return -EINVAL;
+ /* Check against conflicting MOSI idle configuration */
+ if ((spi->mode & SPI_MOSI_IDLE_LOW) && (spi->mode & SPI_MOSI_IDLE_HIGH)) {
+ dev_warn(&spi->dev,
+ "setup: erratic MOSI idle configuration. Set to idle low\n");
+ spi->mode &= ~SPI_MOSI_IDLE_HIGH;
+ }
/*
* Help drivers fail *cleanly* when they need options
* that aren't supported with their current controller.
diff --git a/include/uapi/linux/spi/spi.h b/include/uapi/linux/spi/spi.h
index ca56e477d161..ba9adba25927 100644
--- a/include/uapi/linux/spi/spi.h
+++ b/include/uapi/linux/spi/spi.h
@@ -29,6 +29,7 @@
#define SPI_3WIRE_HIZ _BITUL(15) /* high impedance turnaround */
#define SPI_RX_CPHA_FLIP _BITUL(16) /* flip CPHA on Rx only xfer */
#define SPI_MOSI_IDLE_LOW _BITUL(17) /* leave mosi line low when idle */
+#define SPI_MOSI_IDLE_HIGH _BITUL(18) /* leave mosi line high when idle */
/*
* All the bits defined above should be covered by SPI_MODE_USER_MASK.
@@ -38,6 +39,6 @@
* These bits must not overlap. A static assert check should make sure of that.
* If adding extra bits, make sure to increase the bit index below as well.
*/
-#define SPI_MODE_USER_MASK (_BITUL(18) - 1)
+#define SPI_MODE_USER_MASK (_BITUL(19) - 1)
#endif /* _UAPI_SPI_H */
--
2.43.0
Some SPI peripherals may require strict MOSI line state when the controller
is not clocking out data.
Implement support for MOSI idle state configuration (low or high) by
setting the data output line level on controller setup and after transfers.
Bitbang operations now call controller specific set_mosi_idle() call back
to set MOSI to its idle state.
The MOSI line is kept at its idle state if no tx buffer is provided.
Signed-off-by: Marcelo Schmitt <[email protected]>
---
drivers/spi/spi-bitbang.c | 24 ++++++++++++++++++++++++
include/linux/spi/spi_bitbang.h | 1 +
2 files changed, 25 insertions(+)
diff --git a/drivers/spi/spi-bitbang.c b/drivers/spi/spi-bitbang.c
index ca5cc67555c5..3dee085d3570 100644
--- a/drivers/spi/spi-bitbang.c
+++ b/drivers/spi/spi-bitbang.c
@@ -63,21 +63,28 @@ static unsigned bitbang_txrx_8(
unsigned flags
)
{
+ struct spi_bitbang *bitbang;
unsigned bits = t->bits_per_word;
unsigned count = t->len;
const u8 *tx = t->tx_buf;
u8 *rx = t->rx_buf;
+ bitbang = spi_controller_get_devdata(spi->controller);
while (likely(count > 0)) {
u8 word = 0;
if (tx)
word = *tx++;
+ else
+ word = (spi->mode & SPI_MOSI_IDLE_HIGH) ? 0xFF : 0;
word = txrx_word(spi, ns, word, bits, flags);
if (rx)
*rx++ = word;
count -= 1;
}
+ if (bitbang->set_mosi_idle)
+ bitbang->set_mosi_idle(spi);
+
return t->len - count;
}
@@ -92,21 +99,28 @@ static unsigned bitbang_txrx_16(
unsigned flags
)
{
+ struct spi_bitbang *bitbang;
unsigned bits = t->bits_per_word;
unsigned count = t->len;
const u16 *tx = t->tx_buf;
u16 *rx = t->rx_buf;
+ bitbang = spi_controller_get_devdata(spi->controller);
while (likely(count > 1)) {
u16 word = 0;
if (tx)
word = *tx++;
+ else
+ word = (spi->mode & SPI_MOSI_IDLE_HIGH) ? 0xFFFF : 0;
word = txrx_word(spi, ns, word, bits, flags);
if (rx)
*rx++ = word;
count -= 2;
}
+ if (bitbang->set_mosi_idle)
+ bitbang->set_mosi_idle(spi);
+
return t->len - count;
}
@@ -121,21 +135,28 @@ static unsigned bitbang_txrx_32(
unsigned flags
)
{
+ struct spi_bitbang *bitbang;
unsigned bits = t->bits_per_word;
unsigned count = t->len;
const u32 *tx = t->tx_buf;
u32 *rx = t->rx_buf;
+ bitbang = spi_controller_get_devdata(spi->controller);
while (likely(count > 3)) {
u32 word = 0;
if (tx)
word = *tx++;
+ else
+ word = (spi->mode & SPI_MOSI_IDLE_HIGH) ? 0xFFFFFFFF : 0;
word = txrx_word(spi, ns, word, bits, flags);
if (rx)
*rx++ = word;
count -= 4;
}
+ if (bitbang->set_mosi_idle)
+ bitbang->set_mosi_idle(spi);
+
return t->len - count;
}
@@ -211,6 +232,9 @@ int spi_bitbang_setup(struct spi_device *spi)
goto err_free;
}
+ if (bitbang->set_mosi_idle)
+ bitbang->set_mosi_idle(spi);
+
dev_dbg(&spi->dev, "%s, %u nsec/bit\n", __func__, 2 * cs->nsecs);
return 0;
diff --git a/include/linux/spi/spi_bitbang.h b/include/linux/spi/spi_bitbang.h
index b930eca2ef7b..1a54b593c691 100644
--- a/include/linux/spi/spi_bitbang.h
+++ b/include/linux/spi/spi_bitbang.h
@@ -22,6 +22,7 @@ struct spi_bitbang {
#define BITBANG_CS_ACTIVE 1 /* normally nCS, active low */
#define BITBANG_CS_INACTIVE 0
+ void (*set_mosi_idle)(struct spi_device *spi);
/* txrx_bufs() may handle dma mapping for transfers that don't
* already have one (transfer.{tx,rx}_dma is zero), or use PIO
*/
--
2.43.0
Add device tree documentation for AD4000 series of ADC devices.
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
Suggested-by: David Lechner <[email protected]>
Signed-off-by: Marcelo Schmitt <[email protected]>
---
Even though didn't pick all suggestions to the dt-bindings, I did pick most them
so kept David's Suggested-by tag.
.../bindings/iio/adc/adi,ad4000.yaml | 207 ++++++++++++++++++
MAINTAINERS | 7 +
2 files changed, 214 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
new file mode 100644
index 000000000000..7470d386906b
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
@@ -0,0 +1,207 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad4000.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD4000 and similar Analog to Digital Converters
+
+maintainers:
+ - Marcelo Schmitt <[email protected]>
+
+description: |
+ Analog Devices AD4000 family of Analog to Digital Converters with SPI support.
+ Specifications can be found at:
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
+ https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
+
+$ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+ compatible:
+ enum:
+ - adi,ad4000
+ - adi,ad4001
+ - adi,ad4002
+ - adi,ad4003
+ - adi,ad4004
+ - adi,ad4005
+ - adi,ad4006
+ - adi,ad4007
+ - adi,ad4008
+ - adi,ad4010
+ - adi,ad4011
+ - adi,ad4020
+ - adi,ad4021
+ - adi,ad4022
+ - adi,adaq4001
+ - adi,adaq4003
+
+ reg:
+ maxItems: 1
+
+ spi-max-frequency:
+ maximum: 102040816 # for VIO > 2.7 V, 81300813 for VIO > 1.7 V
+
+ adi,spi-mode:
+ $ref: /schemas/types.yaml#/definitions/string
+ enum: [ single, chain ]
+ description: |
+ This property indicates the SPI wiring configuration.
+
+ When this property is omitted, it is assumed that the device is using what
+ the datasheet calls "4-wire mode". This is the conventional SPI mode used
+ when there are multiple devices on the same bus. In this mode, the CNV
+ line is used to initiate the conversion and the SDI line is connected to
+ CS on the SPI controller.
+
+ When this property is present, it indicates that the device is using one
+ of the following alternative wiring configurations:
+
+ * single: The datasheet calls this "3-wire mode". (NOTE: The datasheet's
+ definition of 3-wire mode is NOT at all related to the standard
+ spi-3wire property!) This mode is often used when the ADC is the only
+ device on the bus. In this mode, SDI is connected to MOSI or to VIO, and
+ the CNV line can be connected to the CS line of the SPI controller or to
+ a GPIO, in which case the CS line of the controller is unused.
+ * chain: The datasheet calls this "chain mode". This mode is used to save
+ on wiring when multiple ADCs are used. In this mode, the SDI line of
+ one chip is tied to the SDO of the next chip in the chain and the SDI of
+ the last chip in the chain is tied to GND. Only the first chip in the
+ chain is connected to the SPI bus. The CNV line of all chips are tied
+ together. The CS line of the SPI controller can be used as the CNV line
+ only if it is active high.
+
+ '#daisy-chained-devices': true
+
+ vdd-supply:
+ description: A 1.8V supply that powers the chip (VDD).
+
+ vio-supply:
+ description:
+ A 1.8V to 5.5V supply for the digital inputs and outputs (VIO).
+
+ ref-supply:
+ description:
+ A 2.5 to 5V supply for the external reference voltage (REF).
+
+ cnv-gpios:
+ description:
+ The Convert Input (CNV). This input has multiple functions. It initiates
+ the conversions and selects the SPI mode of the device (chain or CS). In
+ 'single' mode, this property is omitted if the CNV pin is connected to the
+ CS line of the SPI controller.
+ maxItems: 1
+
+ adi,high-z-input:
+ type: boolean
+ description:
+ High-Z mode allows the amplifier and RC filter in front of the ADC to be
+ chosen based on the signal bandwidth of interest, rather than the settling
+ requirements of the switched capacitor SAR ADC inputs.
+
+ adi,gain-milli:
+ description: |
+ The hardware gain applied to the ADC input (in milli units).
+ The gain provided by the ADC input scaler is defined by the hardware
+ connections between chip pins OUT+, R1K-, R1K1-, R1K+, R1K1+, and OUT-.
+ If not present, default to 1000 (no actual gain applied).
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [454, 909, 1000, 1900]
+ default: 1000
+
+ interrupts:
+ description:
+ The SDO pin can also function as a busy indicator. This node should be
+ connected to an interrupt that is triggered when the SDO line goes low
+ while the SDI line is high and the CNV line is low ('single' mode) or the
+ SDI line is low and the CNV line is high ('multi' mode); or when the SDO
+ line goes high while the SDI and CNV lines are high (chain mode),
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - vdd-supply
+ - vio-supply
+ - ref-supply
+
+allOf:
+ # in '4-wire' mode, cnv-gpios is required, for other modes it is optional
+ - if:
+ not:
+ required:
+ - adi,spi-mode
+ then:
+ required:
+ - cnv-gpios
+ # chain mode has lower SCLK max rate
+ - if:
+ required:
+ - adi,spi-mode
+ properties:
+ adi,spi-mode:
+ const: chain
+ then:
+ properties:
+ spi-max-frequency:
+ maximum: 50000000 # for VIO > 2.7 V, 40000000 for VIO > 1.7 V
+ required:
+ - '#daisy-chained-devices'
+ else:
+ properties:
+ '#daisy-chained-devices': false
+ # Gain property only applies to ADAQ devices
+ - if:
+ properties:
+ compatible:
+ not:
+ contains:
+ enum:
+ - adi,adaq4001
+ - adi,adaq4003
+ then:
+ properties:
+ adi,gain-milli: false
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ /* Example for a AD devices */
+ adc@0 {
+ compatible = "adi,ad4020";
+ reg = <0>;
+ spi-max-frequency = <71000000>;
+ vdd-supply = <&supply_1_8V>;
+ vio-supply = <&supply_1_8V>;
+ ref-supply = <&supply_5V>;
+ cnv-gpios = <&gpio0 88 GPIO_ACTIVE_HIGH>;
+ };
+ };
+ - |
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ /* Example for a ADAQ devices */
+ adc@0 {
+ compatible = "adi,adaq4003";
+ reg = <0>;
+ adi,spi-mode = "single";
+ spi-max-frequency = <80000000>;
+ vdd-supply = <&supply_1_8V>;
+ vio-supply = <&supply_1_8V>;
+ ref-supply = <&supply_5V>;
+ adi,high-z-input;
+ adi,gain-milli = <454>;
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index bff979a507ba..1f052b9cd912 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1200,6 +1200,13 @@ W: https://ez.analog.com/linux-software-drivers
F: Documentation/devicetree/bindings/iio/dac/adi,ad3552r.yaml
F: drivers/iio/dac/ad3552r.c
+ANALOG DEVICES INC AD4000 DRIVER
+M: Marcelo Schmitt <[email protected]>
+L: [email protected]
+S: Supported
+W: https://ez.analog.com/linux-software-drivers
+F: Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
+
ANALOG DEVICES INC AD4130 DRIVER
M: Cosmin Tanislav <[email protected]>
L: [email protected]
--
2.43.0
Implement MOSI idle low and MOSI idle high to better support peripherals
that request specific MOSI behavior.
Signed-off-by: Marcelo Schmitt <[email protected]>
---
drivers/spi/spi-gpio.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-gpio.c b/drivers/spi/spi-gpio.c
index 909cce109bba..d3b8c99f0cb4 100644
--- a/drivers/spi/spi-gpio.c
+++ b/drivers/spi/spi-gpio.c
@@ -236,6 +236,14 @@ static void spi_gpio_chipselect(struct spi_device *spi, int is_active)
}
}
+static void spi_gpio_set_mosi_idle(struct spi_device *spi)
+{
+ struct spi_gpio *spi_gpio = spi_to_spi_gpio(spi);
+
+ gpiod_set_value_cansleep(spi_gpio->mosi,
+ !!(spi->mode & SPI_MOSI_IDLE_HIGH));
+}
+
static int spi_gpio_setup(struct spi_device *spi)
{
struct gpio_desc *cs;
@@ -411,7 +419,8 @@ static int spi_gpio_probe(struct platform_device *pdev)
host->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
host->mode_bits = SPI_3WIRE | SPI_3WIRE_HIZ | SPI_CPHA | SPI_CPOL |
- SPI_CS_HIGH | SPI_LSB_FIRST;
+ SPI_CS_HIGH | SPI_LSB_FIRST | SPI_MOSI_IDLE_LOW |
+ SPI_MOSI_IDLE_HIGH;
if (!spi_gpio->mosi) {
/* HW configuration without MOSI pin
*
@@ -436,6 +445,7 @@ static int spi_gpio_probe(struct platform_device *pdev)
host->flags |= SPI_CONTROLLER_GPIO_SS;
bb->chipselect = spi_gpio_chipselect;
bb->set_line_direction = spi_gpio_set_direction;
+ bb->set_mosi_idle = spi_gpio_set_mosi_idle;
if (host->flags & SPI_CONTROLLER_NO_TX) {
bb->txrx_word[SPI_MODE_0] = spi_gpio_spec_txrx_word_mode0;
--
2.43.0
Implement MOSI idle low and MOSI idle high to better support peripherals
that request specific MOSI behavior.
Signed-off-by: Marcelo Schmitt <[email protected]>
---
drivers/spi/spi-axi-spi-engine.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c
index 0aa31d745734..549f03069d0e 100644
--- a/drivers/spi/spi-axi-spi-engine.c
+++ b/drivers/spi/spi-axi-spi-engine.c
@@ -41,6 +41,7 @@
#define SPI_ENGINE_CONFIG_CPHA BIT(0)
#define SPI_ENGINE_CONFIG_CPOL BIT(1)
#define SPI_ENGINE_CONFIG_3WIRE BIT(2)
+#define SPI_ENGINE_CONFIG_SDO_IDLE BIT(3)
#define SPI_ENGINE_INST_TRANSFER 0x0
#define SPI_ENGINE_INST_ASSERT 0x1
@@ -132,6 +133,10 @@ static unsigned int spi_engine_get_config(struct spi_device *spi)
config |= SPI_ENGINE_CONFIG_CPHA;
if (spi->mode & SPI_3WIRE)
config |= SPI_ENGINE_CONFIG_3WIRE;
+ if (spi->mode & SPI_MOSI_IDLE_HIGH)
+ config |= SPI_ENGINE_CONFIG_SDO_IDLE;
+ if (spi->mode & SPI_MOSI_IDLE_LOW)
+ config &= ~SPI_ENGINE_CONFIG_SDO_IDLE;
return config;
}
@@ -645,7 +650,8 @@ static int spi_engine_probe(struct platform_device *pdev)
return ret;
host->dev.of_node = pdev->dev.of_node;
- host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE;
+ host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE | SPI_MOSI_IDLE_LOW
+ | SPI_MOSI_IDLE_HIGH;
host->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
host->max_speed_hz = clk_get_rate(spi_engine->ref_clk) / 2;
host->transfer_one_message = spi_engine_transfer_one_message;
--
2.43.0
On Tue, 2024-06-04 at 19:41 -0300, Marcelo Schmitt wrote:
> The behavior of an SPI controller data output line (SDO or MOSI or COPI
> (Controller Output Peripheral Input) for disambiguation) is not specified
> when the controller is not clocking out data on SCLK edges. However, there
> exist SPI peripherals that require specific COPI line state when data is
> not being clocked out of the controller.
> Add SPI mode bit to allow pheripherals to request explicit COPI idle
> behavior when needed.
>
> Signed-off-by: Marcelo Schmitt <[email protected]>
> ---
> drivers/spi/spi.c | 6 ++++++
> include/uapi/linux/spi/spi.h | 3 ++-
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 289feccca376..6072b6e93bef 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -3921,6 +3921,12 @@ int spi_setup(struct spi_device *spi)
> (SPI_TX_DUAL | SPI_TX_QUAD | SPI_TX_OCTAL |
> SPI_RX_DUAL | SPI_RX_QUAD | SPI_RX_OCTAL)))
> return -EINVAL;
> + /* Check against conflicting MOSI idle configuration */
> + if ((spi->mode & SPI_MOSI_IDLE_LOW) && (spi->mode &
> SPI_MOSI_IDLE_HIGH)) {
> + dev_warn(&spi->dev,
> + "setup: erratic MOSI idle configuration. Set to idle
> low\n");
> + spi->mode &= ~SPI_MOSI_IDLE_HIGH;
> + }
Should we assume such a thing? IOW, should this be treated as a warning or a
real error? I would assume this should be a configuration error and return -
EINVAL but...
- Nuno Sá
On Tue, 2024-06-04 at 19:42 -0300, Marcelo Schmitt wrote:
> Implement MOSI idle low and MOSI idle high to better support peripherals
> that request specific MOSI behavior.
>
> Signed-off-by: Marcelo Schmitt <[email protected]>
> ---
Acked-by: Nuno Sa <[email protected]>
> drivers/spi/spi-gpio.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-gpio.c b/drivers/spi/spi-gpio.c
> index 909cce109bba..d3b8c99f0cb4 100644
> --- a/drivers/spi/spi-gpio.c
> +++ b/drivers/spi/spi-gpio.c
> @@ -236,6 +236,14 @@ static void spi_gpio_chipselect(struct spi_device *spi,
> int is_active)
> }
> }
>
> +static void spi_gpio_set_mosi_idle(struct spi_device *spi)
> +{
> + struct spi_gpio *spi_gpio = spi_to_spi_gpio(spi);
> +
> + gpiod_set_value_cansleep(spi_gpio->mosi,
> + !!(spi->mode & SPI_MOSI_IDLE_HIGH));
> +}
> +
> static int spi_gpio_setup(struct spi_device *spi)
> {
> struct gpio_desc *cs;
> @@ -411,7 +419,8 @@ static int spi_gpio_probe(struct platform_device *pdev)
>
> host->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
> host->mode_bits = SPI_3WIRE | SPI_3WIRE_HIZ | SPI_CPHA | SPI_CPOL |
> - SPI_CS_HIGH | SPI_LSB_FIRST;
> + SPI_CS_HIGH | SPI_LSB_FIRST | SPI_MOSI_IDLE_LOW |
> + SPI_MOSI_IDLE_HIGH;
> if (!spi_gpio->mosi) {
> /* HW configuration without MOSI pin
> *
> @@ -436,6 +445,7 @@ static int spi_gpio_probe(struct platform_device *pdev)
> host->flags |= SPI_CONTROLLER_GPIO_SS;
> bb->chipselect = spi_gpio_chipselect;
> bb->set_line_direction = spi_gpio_set_direction;
> + bb->set_mosi_idle = spi_gpio_set_mosi_idle;
>
> if (host->flags & SPI_CONTROLLER_NO_TX) {
> bb->txrx_word[SPI_MODE_0] = spi_gpio_spec_txrx_word_mode0;
On Tue, 2024-06-04 at 19:42 -0300, Marcelo Schmitt wrote:
> Some SPI peripherals may require strict MOSI line state when the controller
> is not clocking out data.
> Implement support for MOSI idle state configuration (low or high) by
> setting the data output line level on controller setup and after transfers.
> Bitbang operations now call controller specific set_mosi_idle() call back
> to set MOSI to its idle state.
> The MOSI line is kept at its idle state if no tx buffer is provided.
>
> Signed-off-by: Marcelo Schmitt <[email protected]>
> ---
Some minor nit below in case a re-spin is needed. Anyways,
Acked-by: Nuno Sa <[email protected]>
> drivers/spi/spi-bitbang.c | 24 ++++++++++++++++++++++++
> include/linux/spi/spi_bitbang.h | 1 +
> 2 files changed, 25 insertions(+)
>
> diff --git a/drivers/spi/spi-bitbang.c b/drivers/spi/spi-bitbang.c
> index ca5cc67555c5..3dee085d3570 100644
> --- a/drivers/spi/spi-bitbang.c
> +++ b/drivers/spi/spi-bitbang.c
> @@ -63,21 +63,28 @@ static unsigned bitbang_txrx_8(
> unsigned flags
> )
> {
> + struct spi_bitbang *bitbang;
> unsigned bits = t->bits_per_word;
> unsigned count = t->len;
> const u8 *tx = t->tx_buf;
> u8 *rx = t->rx_buf;
>
> + bitbang = spi_controller_get_devdata(spi->controller);
> while (likely(count > 0)) {
> u8 word = 0;
>
> if (tx)
> word = *tx++;
> + else
> + word = (spi->mode & SPI_MOSI_IDLE_HIGH) ? 0xFF : 0;
no need for ()
> word = txrx_word(spi, ns, word, bits, flags);
> if (rx)
> *rx++ = word;
> count -= 1;
> }
> + if (bitbang->set_mosi_idle)
> + bitbang->set_mosi_idle(spi);
> +
> return t->len - count;
> }
>
> @@ -92,21 +99,28 @@ static unsigned bitbang_txrx_16(
> unsigned flags
> )
> {
> + struct spi_bitbang *bitbang;
> unsigned bits = t->bits_per_word;
> unsigned count = t->len;
> const u16 *tx = t->tx_buf;
> u16 *rx = t->rx_buf;
>
> + bitbang = spi_controller_get_devdata(spi->controller);
> while (likely(count > 1)) {
> u16 word = 0;
>
> if (tx)
> word = *tx++;
> + else
> + word = (spi->mode & SPI_MOSI_IDLE_HIGH) ? 0xFFFF : 0;
ditto (and for the txrx8 function)
- Nuno Sá
Hi Marcelo,
On Tue, 2024-06-04 at 19:40 -0300, Marcelo Schmitt wrote:
> This patch series extends the SPI bitbang, gpio, and spi-engine controllers to
> support configurable MOSI line idle state.
> It then introduces the ad4000 driver which makes use of the MOSI idle
> configuration to properly support AD4000 series of ADCs.
Not sure what happened but I'm not seeing the patch for ad4000...
- Nuno Sá
On Tue, 2024-06-04 at 19:43 -0300, Marcelo Schmitt wrote:
> Implement MOSI idle low and MOSI idle high to better support peripherals
> that request specific MOSI behavior.
>
> Signed-off-by: Marcelo Schmitt <[email protected]>
> ---
Reviewed-by: Nuno Sa <[email protected]>
Add support for AD4000 series of low noise, low power, high speed,
successive aproximation register (SAR) ADCs.
Signed-off-by: Marcelo Schmitt <[email protected]>
---
MAINTAINERS | 1 +
drivers/iio/adc/Kconfig | 12 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ad4000.c | 735 +++++++++++++++++++++++++++++++++++++++
4 files changed, 749 insertions(+)
create mode 100644 drivers/iio/adc/ad4000.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 1f052b9cd912..c732cf13f511 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1206,6 +1206,7 @@ L: [email protected]
S: Supported
W: https://ez.analog.com/linux-software-drivers
F: Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
+F: drivers/iio/adc/ad4000.c
ANALOG DEVICES INC AD4130 DRIVER
M: Cosmin Tanislav <[email protected]>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 5030319249c5..dcc49d9711a4 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -21,6 +21,18 @@ config AD_SIGMA_DELTA
select IIO_BUFFER
select IIO_TRIGGERED_BUFFER
+config AD4000
+ tristate "Analog Devices AD4000 ADC Driver"
+ depends on SPI
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
+ help
+ Say yes here to build support for Analog Devices AD4000 high speed
+ SPI analog to digital converters (ADC).
+
+ To compile this driver as a module, choose M here: the module will be
+ called ad4000.
+
config AD4130
tristate "Analog Device AD4130 ADC Driver"
depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 37ac689a0209..c32bd0ef6128 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -6,6 +6,7 @@
# When adding new entries keep the list in alphabetical order
obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
+obj-$(CONFIG_AD4000) += ad4000.o
obj-$(CONFIG_AD4130) += ad4130.o
obj-$(CONFIG_AD7091R) += ad7091r-base.o
obj-$(CONFIG_AD7091R5) += ad7091r5.o
diff --git a/drivers/iio/adc/ad4000.c b/drivers/iio/adc/ad4000.c
new file mode 100644
index 000000000000..55dc36fbd549
--- /dev/null
+++ b/drivers/iio/adc/ad4000.c
@@ -0,0 +1,735 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * AD4000 SPI ADC driver
+ *
+ * Copyright 2024 Analog Devices Inc.
+ */
+#include <asm/unaligned.h>
+#include <linux/bits.h>
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/math.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <linux/sysfs.h>
+#include <linux/units.h>
+#include <linux/util_macros.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
+
+#define AD4000_READ_COMMAND 0x54
+#define AD4000_WRITE_COMMAND 0x14
+
+#define AD4000_CONFIG_REG_MSK 0xFF
+
+/* AD4000 Configuration Register programmable bits */
+#define AD4000_CFG_STATUS BIT(4) /* Status bits output */
+#define AD4000_CFG_SPAN_COMP BIT(3) /* Input span compression */
+#define AD4000_CFG_HIGHZ BIT(2) /* High impedance mode */
+#define AD4000_CFG_TURBO BIT(1) /* Turbo mode */
+
+#define AD4000_TQUIET1_NS 190
+#define AD4000_TQUIET2_NS 60
+#define AD4000_TCONV_NS 320
+
+#define AD4000_18BIT_MSK GENMASK(31, 14)
+#define AD4000_20BIT_MSK GENMASK(31, 12)
+
+#define AD4000_DIFF_CHANNEL(_sign, _real_bits) \
+ { \
+ .type = IIO_VOLTAGE, \
+ .indexed = 1, \
+ .differential = 1, \
+ .channel = 0, \
+ .channel2 = 1, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE), \
+ .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),\
+ .scan_type = { \
+ .sign = _sign, \
+ .realbits = _real_bits, \
+ .storagebits = _real_bits > 16 ? 32 : 16, \
+ .shift = _real_bits > 16 ? 32 - _real_bits : 0, \
+ .endianness = IIO_BE, \
+ }, \
+ } \
+
+#define AD4000_PSEUDO_DIFF_CHANNEL(_sign, _real_bits) \
+ { \
+ .type = IIO_VOLTAGE, \
+ .indexed = 1, \
+ .channel = 0, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE) | \
+ BIT(IIO_CHAN_INFO_OFFSET), \
+ .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),\
+ .scan_type = { \
+ .sign = _sign, \
+ .realbits = _real_bits, \
+ .storagebits = _real_bits > 16 ? 32 : 16, \
+ .shift = _real_bits > 16 ? 32 - _real_bits : 0, \
+ .endianness = IIO_BE, \
+ }, \
+ } \
+
+enum ad4000_ids {
+ ID_AD4000,
+ ID_AD4001,
+ ID_AD4002,
+ ID_AD4003,
+ ID_AD4004,
+ ID_AD4005,
+ ID_AD4006,
+ ID_AD4007,
+ ID_AD4008,
+ ID_AD4010,
+ ID_AD4011,
+ ID_AD4020,
+ ID_AD4021,
+ ID_AD4022,
+ ID_ADAQ4001,
+ ID_ADAQ4003,
+};
+
+enum ad4000_spi_mode {
+ /* datasheet calls this "4-wire mode" (controller CS goes to ADC SDI!) */
+ AD4000_SPI_MODE_DEFAULT,
+ /* datasheet calls this "3-wire mode" (not related to SPI_3WIRE!) */
+ AD4000_SPI_MODE_SINGLE,
+};
+
+/* maps adi,spi-mode property value to enum */
+static const char * const ad4000_spi_modes[] = {
+ [AD4000_SPI_MODE_DEFAULT] = "",
+ [AD4000_SPI_MODE_SINGLE] = "single",
+};
+
+struct ad4000_chip_info {
+ const char *dev_name;
+ struct iio_chan_spec chan_spec;
+};
+
+static const struct ad4000_chip_info ad4000_chips[] = {
+ [ID_AD4000] = {
+ .dev_name = "ad4000",
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
+ },
+ [ID_AD4001] = {
+ .dev_name = "ad4001",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 16),
+ },
+ [ID_AD4002] = {
+ .dev_name = "ad4002",
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
+ },
+ [ID_AD4003] = {
+ .dev_name = "ad4003",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
+ },
+ [ID_AD4004] = {
+ .dev_name = "ad4004",
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
+ },
+ [ID_AD4005] = {
+ .dev_name = "ad4005",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 16),
+ },
+ [ID_AD4006] = {
+ .dev_name = "ad4006",
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
+ },
+ [ID_AD4007] = {
+ .dev_name = "ad4007",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
+ },
+ [ID_AD4008] = {
+ .dev_name = "ad4008",
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
+ },
+ [ID_AD4010] = {
+ .dev_name = "ad4010",
+ .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
+ },
+ [ID_AD4011] = {
+ .dev_name = "ad4011",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
+ },
+ [ID_AD4020] = {
+ .dev_name = "ad4020",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 20),
+ },
+ [ID_AD4021] = {
+ .dev_name = "ad4021",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 20),
+ },
+ [ID_AD4022] = {
+ .dev_name = "ad4022",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 20),
+ },
+ [ID_ADAQ4001] = {
+ .dev_name = "adaq4001",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 16),
+ },
+ [ID_ADAQ4003] = {
+ .dev_name = "adaq4003",
+ .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
+ },
+};
+
+struct ad4000_state {
+ struct spi_device *spi;
+ struct gpio_desc *cnv_gpio;
+ struct spi_transfer xfers[2];
+ struct spi_message msg;
+ int vref;
+ enum ad4000_spi_mode spi_mode;
+ bool span_comp;
+ bool turbo_mode;
+ int gain_milli;
+ int scale_tbl[2][2];
+
+ /*
+ * DMA (thus cache coherency maintenance) requires the
+ * transfer buffers to live in their own cache lines.
+ */
+ struct {
+ union {
+ __be16 sample_buf16;
+ __be32 sample_buf32;
+ } data;
+ s64 timestamp __aligned(8);
+ } scan __aligned(IIO_DMA_MINALIGN);
+ __be16 tx_buf;
+ __be16 rx_buf;
+};
+
+static void ad4000_fill_scale_tbl(struct ad4000_state *st, int scale_bits,
+ const struct ad4000_chip_info *chip)
+{
+ int diff = chip->chan_spec.differential;
+ int val, val2, tmp0, tmp1;
+ u64 tmp2;
+
+ val2 = scale_bits;
+ val = st->vref / 1000;
+ /*
+ * The gain is stored as a fraction of 1000 and, as we need to
+ * divide vref by gain, we invert the gain/1000 fraction.
+ * Also multiply by an extra MILLI to avoid losing precision.
+ */
+ val = mult_frac(val, MILLI * MILLI, st->gain_milli);
+ /* Would multiply by NANO here but we multiplied by extra MILLI */
+ tmp2 = shift_right((u64)val * MICRO, val2);
+ tmp0 = (int)div_s64_rem(tmp2, NANO, &tmp1);
+ /* Store scale for when span compression is disabled */
+ st->scale_tbl[0][0] = tmp0; /* Integer part */
+ st->scale_tbl[0][1] = abs(tmp1); /* Fractional part */
+ /* Store scale for when span compression is enabled */
+ st->scale_tbl[1][0] = tmp0;
+ /* The integer part is always zero so don't bother to divide it. */
+ if (diff)
+ st->scale_tbl[1][1] = DIV_ROUND_CLOSEST(abs(tmp1) * 4, 5);
+ else
+ st->scale_tbl[1][1] = DIV_ROUND_CLOSEST(abs(tmp1) * 9, 10);
+}
+
+static int ad4000_write_reg(struct ad4000_state *st, uint8_t val)
+{
+ st->tx_buf = cpu_to_be16(AD4000_WRITE_COMMAND << BITS_PER_BYTE | val);
+ return spi_write(st->spi, &st->tx_buf, 2);
+}
+
+static int ad4000_read_reg(struct ad4000_state *st, unsigned int *val)
+{
+ struct spi_transfer t[] = {
+ {
+ .tx_buf = &st->tx_buf,
+ .rx_buf = &st->rx_buf,
+ .len = 2,
+ },
+ };
+ int ret;
+
+ st->tx_buf = cpu_to_be16(AD4000_READ_COMMAND << BITS_PER_BYTE);
+ ret = spi_sync_transfer(st->spi, t, ARRAY_SIZE(t));
+ if (ret < 0)
+ return ret;
+
+ *val = be16_to_cpu(st->rx_buf);
+
+ return ret;
+}
+
+static void ad4000_unoptimize_msg(void *msg)
+{
+ spi_unoptimize_message(msg);
+}
+
+/*
+ * This executes a data sample transfer for when the device connections are
+ * in "3-wire" mode, selected by setting the adi,spi-mode device tree property
+ * to "single". In this connection mode, the ADC SDI pin is connected to MOSI or
+ * to VIO and ADC CNV pin is connected either to a SPI controller CS or to a GPIO.
+ * AD4000 series of devices initiate conversions on the rising edge of CNV pin.
+ *
+ * If the CNV pin is connected to an SPI controller CS line (which is by default
+ * active low), the ADC readings would have a latency (delay) of one read.
+ * Moreover, since we also do ADC sampling for filling the buffer on triggered
+ * buffer mode, the timestamps of buffer readings would be disarranged.
+ * To prevent the read latency and reduce the time discrepancy between the
+ * sample read request and the time of actual sampling by the ADC, do a
+ * preparatory transfer to pulse the CS/CNV line.
+ */
+static int ad4000_prepare_3wire_mode_message(struct ad4000_state *st,
+ const struct iio_chan_spec *chan)
+{
+ unsigned int cnv_pulse_time = st->turbo_mode ? AD4000_TQUIET1_NS
+ : AD4000_TCONV_NS;
+ struct spi_transfer *xfers = st->xfers;
+ int ret;
+
+ xfers[0].cs_change = 1;
+ xfers[0].cs_change_delay.value = cnv_pulse_time;
+ xfers[0].cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
+
+ xfers[1].rx_buf = &st->scan.data;
+ xfers[1].len = BITS_TO_BYTES(chan->scan_type.storagebits);
+ xfers[1].delay.value = AD4000_TQUIET2_NS;
+ xfers[1].delay.unit = SPI_DELAY_UNIT_NSECS;
+
+ spi_message_init_with_transfers(&st->msg, st->xfers, 2);
+
+ ret = spi_optimize_message(st->spi, &st->msg);
+ if (ret)
+ return ret;
+
+ return devm_add_action_or_reset(&st->spi->dev, ad4000_unoptimize_msg,
+ &st->msg);
+}
+
+/*
+ * This executes a data sample transfer for when the device connections are
+ * in "4-wire" mode, selected when the adi,spi-mode device tree
+ * property is absent or empty. In this connection mode, the controller CS pin
+ * is connected to ADC SDI pin and a GPIO is connected to ADC CNV pin.
+ * The GPIO connected to ADC CNV pin is set outside of the SPI transfer.
+ */
+static int ad4000_prepare_4wire_mode_message(struct ad4000_state *st,
+ const struct iio_chan_spec *chan)
+{
+ unsigned int cnv_to_sdi_time = st->turbo_mode ? AD4000_TQUIET1_NS
+ : AD4000_TCONV_NS;
+ struct spi_transfer *xfers = st->xfers;
+ int ret;
+
+ /*
+ * Dummy transfer to cause enough delay between CNV going high and SDI
+ * going low.
+ */
+ xfers[0].cs_off = 1;
+ xfers[0].delay.value = cnv_to_sdi_time;
+ xfers[0].delay.unit = SPI_DELAY_UNIT_NSECS;
+
+ xfers[1].rx_buf = &st->scan.data;
+ xfers[1].len = BITS_TO_BYTES(chan->scan_type.storagebits);
+
+ spi_message_init_with_transfers(&st->msg, st->xfers, 2);
+
+ ret = spi_optimize_message(st->spi, &st->msg);
+ if (ret)
+ return ret;
+
+ return devm_add_action_or_reset(&st->spi->dev, ad4000_unoptimize_msg,
+ &st->msg);
+}
+
+static int ad4000_convert_and_acquire(struct ad4000_state *st)
+{
+ int ret;
+
+ /*
+ * In 4-wire mode, the CNV line is held high for the entire conversion
+ * and acquisition process. In other modes st->cnv_gpio is NULL and is
+ * ignored (CS is wired to CNV in those cases).
+ */
+ gpiod_set_value_cansleep(st->cnv_gpio, 1);
+ ret = spi_sync(st->spi, &st->msg);
+ gpiod_set_value_cansleep(st->cnv_gpio, 0);
+
+ return ret;
+}
+
+static int ad4000_single_conversion(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan, int *val)
+{
+ struct ad4000_state *st = iio_priv(indio_dev);
+ u32 sample;
+ int ret;
+
+ ret = ad4000_convert_and_acquire(st);
+
+ if (chan->scan_type.storagebits > 16)
+ sample = be32_to_cpu(st->scan.data.sample_buf32);
+ else
+ sample = be16_to_cpu(st->scan.data.sample_buf16);
+
+ switch (chan->scan_type.realbits) {
+ case 16:
+ break;
+ case 18:
+ sample = FIELD_GET(AD4000_18BIT_MSK, sample);
+ break;
+ case 20:
+ sample = FIELD_GET(AD4000_20BIT_MSK, sample);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (chan->scan_type.sign == 's')
+ *val = sign_extend32(sample, chan->scan_type.realbits - 1);
+
+ return IIO_VAL_INT;
+}
+
+static int ad4000_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int *val,
+ int *val2, long info)
+{
+ struct ad4000_state *st = iio_priv(indio_dev);
+
+ switch (info) {
+ case IIO_CHAN_INFO_RAW:
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
+ return ad4000_single_conversion(indio_dev, chan, val);
+ unreachable();
+ case IIO_CHAN_INFO_SCALE:
+ *val = st->scale_tbl[st->span_comp][0];
+ *val2 = st->scale_tbl[st->span_comp][1];
+ return IIO_VAL_INT_PLUS_NANO;
+ case IIO_CHAN_INFO_OFFSET:
+ *val = 0;
+ if (st->span_comp)
+ *val = mult_frac(st->vref / 1000, 1, 10);
+
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int ad4000_read_avail(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int **vals, int *type, int *length,
+ long info)
+{
+ struct ad4000_state *st = iio_priv(indio_dev);
+
+ switch (info) {
+ case IIO_CHAN_INFO_SCALE:
+ *vals = (int *)st->scale_tbl;
+ *length = 2 * 2;
+ *type = IIO_VAL_INT_PLUS_NANO;
+ return IIO_AVAIL_LIST;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int ad4000_write_raw_get_fmt(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, long mask)
+{
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ return IIO_VAL_INT_PLUS_NANO;
+ default:
+ return IIO_VAL_INT_PLUS_MICRO;
+ }
+}
+
+static int ad4000_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int val, int val2,
+ long mask)
+{
+ struct ad4000_state *st = iio_priv(indio_dev);
+ unsigned int reg_val;
+ bool span_comp_en;
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
+ ret = ad4000_read_reg(st, ®_val);
+ if (ret < 0)
+ return ret;
+
+ span_comp_en = (val2 == st->scale_tbl[1][1]);
+ reg_val &= ~AD4000_CFG_SPAN_COMP;
+ reg_val |= FIELD_PREP(AD4000_CFG_SPAN_COMP, span_comp_en);
+
+ ret = ad4000_write_reg(st, reg_val);
+ if (ret < 0)
+ return ret;
+
+ st->span_comp = span_comp_en;
+ return 0;
+ }
+ unreachable();
+ default:
+ return -EINVAL;
+ }
+}
+
+static irqreturn_t ad4000_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ struct ad4000_state *st = iio_priv(indio_dev);
+ int ret;
+
+ ret = ad4000_convert_and_acquire(st);
+ if (ret < 0)
+ goto err_out;
+
+ iio_push_to_buffers_with_timestamp(indio_dev, &st->scan,
+ iio_get_time_ns(indio_dev));
+
+err_out:
+ iio_trigger_notify_done(indio_dev->trig);
+ return IRQ_HANDLED;
+}
+
+static int ad4000_reg_access(struct iio_dev *indio_dev, unsigned int reg,
+ unsigned int writeval, unsigned int *readval)
+{
+ struct ad4000_state *st = iio_priv(indio_dev);
+ int ret;
+
+ if (readval)
+ ret = ad4000_read_reg(st, readval);
+ else
+ ret = ad4000_write_reg(st, writeval);
+
+ return ret;
+}
+
+static const struct iio_info ad4000_info = {
+ .read_raw = &ad4000_read_raw,
+ .read_avail = &ad4000_read_avail,
+ .write_raw = &ad4000_write_raw,
+ .write_raw_get_fmt = &ad4000_write_raw_get_fmt,
+ .debugfs_reg_access = &ad4000_reg_access,
+
+};
+
+static int ad4000_config(struct ad4000_state *st)
+{
+ unsigned int reg_val;
+
+ if (device_property_present(&st->spi->dev, "adi,high-z-input"))
+ reg_val |= FIELD_PREP(AD4000_CFG_HIGHZ, 1);
+
+ /*
+ * The ADC SDI pin might be connected to controller CS line in which
+ * case the write might fail. This, however, does not prevent the device
+ * from functioning even though in a configuration other than the
+ * requested one.
+ */
+ return ad4000_write_reg(st, reg_val);
+}
+
+static void ad4000_regulator_disable(void *reg)
+{
+ regulator_disable(reg);
+}
+
+static int ad4000_probe(struct spi_device *spi)
+{
+ const struct ad4000_chip_info *chip;
+ struct regulator *vref_reg;
+ struct iio_dev *indio_dev;
+ struct ad4000_state *st;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ chip = spi_get_device_match_data(spi);
+ if (!chip)
+ return -EINVAL;
+
+ st = iio_priv(indio_dev);
+ st->spi = spi;
+
+ ret = devm_regulator_get_enable(&spi->dev, "vdd");
+ if (ret)
+ return dev_err_probe(&spi->dev, ret, "Failed to enable VDD supply\n");
+
+ ret = devm_regulator_get_enable(&spi->dev, "vio");
+ if (ret)
+ return dev_err_probe(&spi->dev, ret, "Failed to enable VIO supply\n");
+
+ vref_reg = devm_regulator_get(&spi->dev, "ref");
+ if (IS_ERR(vref_reg))
+ return dev_err_probe(&spi->dev, PTR_ERR(vref_reg),
+ "Failed to get vref regulator\n");
+
+ ret = regulator_enable(vref_reg);
+ if (ret < 0)
+ return dev_err_probe(&spi->dev, ret,
+ "Failed to enable voltage regulator\n");
+
+ ret = devm_add_action_or_reset(&spi->dev, ad4000_regulator_disable, vref_reg);
+ if (ret)
+ return dev_err_probe(&spi->dev, ret,
+ "Failed to add regulator disable action\n");
+
+ st->vref = regulator_get_voltage(vref_reg);
+ if (st->vref < 0)
+ return dev_err_probe(&spi->dev, st->vref, "Failed to get vref\n");
+
+ st->cnv_gpio = devm_gpiod_get_optional(&spi->dev, "cnv", GPIOD_OUT_HIGH);
+ if (IS_ERR(st->cnv_gpio))
+ return dev_err_probe(&spi->dev, PTR_ERR(st->cnv_gpio),
+ "Failed to get CNV GPIO");
+
+ ret = device_property_match_property_string(&spi->dev, "adi,spi-mode",
+ ad4000_spi_modes,
+ ARRAY_SIZE(ad4000_spi_modes));
+ /* Default to 4-wire mode if adi,spi-mode property is not present */
+ if (ret == -EINVAL)
+ st->spi_mode = AD4000_SPI_MODE_DEFAULT;
+ else if (ret < 0)
+ return dev_err_probe(&spi->dev, ret,
+ "getting adi,spi-mode property failed\n");
+ else
+ st->spi_mode = ret;
+
+ switch (st->spi_mode) {
+ case AD4000_SPI_MODE_DEFAULT:
+ ret = ad4000_prepare_4wire_mode_message(st, &chip->chan_spec);
+ if (ret)
+ return ret;
+
+ break;
+ case AD4000_SPI_MODE_SINGLE:
+ ret = ad4000_prepare_3wire_mode_message(st, &chip->chan_spec);
+ if (ret)
+ return ret;
+
+ /*
+ * In "3-wire mode", the ADC SDI line must be kept high when
+ * data is not being clocked out of the controller.
+ * Request the SPI controller to make MOSI idle high.
+ */
+ spi->mode = SPI_MODE_0 | SPI_MOSI_IDLE_HIGH;
+ if (spi_setup(spi))
+ dev_warn(&st->spi->dev, "SPI controller setup failed\n");
+
+ break;
+ }
+
+ ret = ad4000_config(st);
+ if (ret < 0)
+ dev_dbg(&st->spi->dev, "Failed to config device\n");
+
+ indio_dev->name = chip->dev_name;
+ indio_dev->info = &ad4000_info;
+ indio_dev->channels = &chip->chan_spec;
+ indio_dev->num_channels = 1;
+
+ /* Hardware gain only applies to ADAQ devices */
+ st->gain_milli = 1000;
+ if (device_property_present(&spi->dev, "adi,gain-milli")) {
+
+ ret = device_property_read_u32(&spi->dev, "adi,gain-milli",
+ &st->gain_milli);
+ if (ret)
+ return dev_err_probe(&spi->dev, ret,
+ "Failed to read gain property\n");
+ }
+
+ /*
+ * ADCs that output two's complement code have one less bit to express
+ * voltage magnitude.
+ */
+ if (chip->chan_spec.scan_type.sign == 's')
+ ad4000_fill_scale_tbl(st, chip->chan_spec.scan_type.realbits - 1,
+ chip);
+ else
+ ad4000_fill_scale_tbl(st, chip->chan_spec.scan_type.realbits,
+ chip);
+
+ ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
+ &iio_pollfunc_store_time,
+ &ad4000_trigger_handler, NULL);
+ if (ret)
+ return ret;
+
+ return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct spi_device_id ad4000_id[] = {
+ { "ad4000", (kernel_ulong_t)&ad4000_chips[ID_AD4000] },
+ { "ad4001", (kernel_ulong_t)&ad4000_chips[ID_AD4001] },
+ { "ad4002", (kernel_ulong_t)&ad4000_chips[ID_AD4002] },
+ { "ad4003", (kernel_ulong_t)&ad4000_chips[ID_AD4003] },
+ { "ad4004", (kernel_ulong_t)&ad4000_chips[ID_AD4004] },
+ { "ad4005", (kernel_ulong_t)&ad4000_chips[ID_AD4005] },
+ { "ad4006", (kernel_ulong_t)&ad4000_chips[ID_AD4006] },
+ { "ad4007", (kernel_ulong_t)&ad4000_chips[ID_AD4007] },
+ { "ad4008", (kernel_ulong_t)&ad4000_chips[ID_AD4008] },
+ { "ad4010", (kernel_ulong_t)&ad4000_chips[ID_AD4010] },
+ { "ad4011", (kernel_ulong_t)&ad4000_chips[ID_AD4011] },
+ { "ad4020", (kernel_ulong_t)&ad4000_chips[ID_AD4020] },
+ { "ad4021", (kernel_ulong_t)&ad4000_chips[ID_AD4021] },
+ { "ad4022", (kernel_ulong_t)&ad4000_chips[ID_AD4022] },
+ { "adaq4001", (kernel_ulong_t)&ad4000_chips[ID_ADAQ4001] },
+ { "adaq4003", (kernel_ulong_t)&ad4000_chips[ID_ADAQ4003] },
+ { }
+};
+MODULE_DEVICE_TABLE(spi, ad4000_id);
+
+static const struct of_device_id ad4000_of_match[] = {
+ { .compatible = "adi,ad4000", .data = &ad4000_chips[ID_AD4000] },
+ { .compatible = "adi,ad4001", .data = &ad4000_chips[ID_AD4001] },
+ { .compatible = "adi,ad4002", .data = &ad4000_chips[ID_AD4002] },
+ { .compatible = "adi,ad4003", .data = &ad4000_chips[ID_AD4003] },
+ { .compatible = "adi,ad4004", .data = &ad4000_chips[ID_AD4004] },
+ { .compatible = "adi,ad4005", .data = &ad4000_chips[ID_AD4005] },
+ { .compatible = "adi,ad4006", .data = &ad4000_chips[ID_AD4006] },
+ { .compatible = "adi,ad4007", .data = &ad4000_chips[ID_AD4007] },
+ { .compatible = "adi,ad4008", .data = &ad4000_chips[ID_AD4008] },
+ { .compatible = "adi,ad4010", .data = &ad4000_chips[ID_AD4010] },
+ { .compatible = "adi,ad4011", .data = &ad4000_chips[ID_AD4011] },
+ { .compatible = "adi,ad4020", .data = &ad4000_chips[ID_AD4020] },
+ { .compatible = "adi,ad4021", .data = &ad4000_chips[ID_AD4021] },
+ { .compatible = "adi,ad4022", .data = &ad4000_chips[ID_AD4022] },
+ { .compatible = "adi,adaq4001", .data = &ad4000_chips[ID_ADAQ4001] },
+ { .compatible = "adi,adaq4003", .data = &ad4000_chips[ID_ADAQ4003] },
+ { }
+};
+MODULE_DEVICE_TABLE(of, ad4000_of_match);
+
+static struct spi_driver ad4000_driver = {
+ .driver = {
+ .name = "ad4000",
+ .of_match_table = ad4000_of_match,
+ },
+ .probe = ad4000_probe,
+ .id_table = ad4000_id,
+};
+module_spi_driver(ad4000_driver);
+
+MODULE_AUTHOR("Marcelo Schmitt <[email protected]>");
+MODULE_DESCRIPTION("Analog Devices AD4000 ADC driver");
+MODULE_LICENSE("GPL");
--
2.43.0
On 06/05, Nuno S? wrote:
> Hi Marcelo,
>
> On Tue, 2024-06-04 at 19:40 -0300, Marcelo Schmitt wrote:
> > This patch series extends the SPI bitbang, gpio, and spi-engine controllers to
> > support configurable MOSI line idle state.
> > It then introduces the ad4000 driver which makes use of the MOSI idle
> > configuration to properly support AD4000 series of ADCs.
>
> Not sure what happened but I'm not seeing the patch for ad4000...
I thought patches were numbered up to 5 only and forgot to send the last one.
Sent that one now.
Thanks,
Marcelo
>
> - Nuno S?
>
>
On Wed, Jun 05, 2024 at 11:14:33AM +0200, Nuno S? wrote:
> On Tue, 2024-06-04 at 19:41 -0300, Marcelo Schmitt wrote:
> > + /* Check against conflicting MOSI idle configuration */
> > + if ((spi->mode & SPI_MOSI_IDLE_LOW) && (spi->mode &
> > SPI_MOSI_IDLE_HIGH)) {
> > + dev_warn(&spi->dev,
> > + "setup: erratic MOSI idle configuration. Set to idle
> > low\n");
> > + spi->mode &= ~SPI_MOSI_IDLE_HIGH;
> > + }
> Should we assume such a thing? IOW, should this be treated as a warning or a
> real error? I would assume this should be a configuration error and return -
> EINVAL but...
Right, and the error message isn't very clear.
On Tue, Jun 04, 2024 at 07:41:47PM -0300, Marcelo Schmitt wrote:
> The behavior of an SPI controller data output line (SDO or MOSI or COPI
> (Controller Output Peripheral Input) for disambiguation) is not specified
> when the controller is not clocking out data on SCLK edges. However, there
> exist SPI peripherals that require specific COPI line state when data is
> not being clocked out of the controller.
This is an optimisation for accelerating devices that need a specific
value, really if these devices need a value they should send it.
> #define SPI_MOSI_IDLE_LOW _BITUL(17) /* leave mosi line low when idle */
> +#define SPI_MOSI_IDLE_HIGH _BITUL(18) /* leave mosi line high when idle */
Realistically we'll have a large set of drivers that are expecting the
line to be held low so I'm not sure we need that option. I would also
expect to have an implementation of these options in the core which
supplies buffers with the relevant data for use with controllers that
don't have the feature (similar to how _MUST_TX and _MUST_RX are done).
Even without that we'd need feature detection so that drivers that try
to use this aren't just buggy when used with a controller that doesn't
implement it, but once you're detecting you may as well just make things
work.
On Wed, 2024-06-05 at 08:14 -0300, Marcelo Schmitt wrote:
> Add support for AD4000 series of low noise, low power, high speed,
> successive aproximation register (SAR) ADCs.
>
> Signed-off-by: Marcelo Schmitt <[email protected]>
> ---
...
> +
> +static const struct ad4000_chip_info ad4000_chips[] = {
> + [ID_AD4000] = {
> + .dev_name = "ad4000",
> + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
> + },
> + [ID_AD4001] = {
> + .dev_name = "ad4001",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 16),
> + },
> + [ID_AD4002] = {
> + .dev_name = "ad4002",
> + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
> + },
> + [ID_AD4003] = {
> + .dev_name = "ad4003",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
> + },
> + [ID_AD4004] = {
> + .dev_name = "ad4004",
> + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
> + },
> + [ID_AD4005] = {
> + .dev_name = "ad4005",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 16),
> + },
> + [ID_AD4006] = {
> + .dev_name = "ad4006",
> + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
> + },
> + [ID_AD4007] = {
> + .dev_name = "ad4007",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
> + },
> + [ID_AD4008] = {
> + .dev_name = "ad4008",
> + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 16),
> + },
> + [ID_AD4010] = {
> + .dev_name = "ad4010",
> + .chan_spec = AD4000_PSEUDO_DIFF_CHANNEL('u', 18),
> + },
> + [ID_AD4011] = {
> + .dev_name = "ad4011",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
> + },
> + [ID_AD4020] = {
> + .dev_name = "ad4020",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 20),
> + },
> + [ID_AD4021] = {
> + .dev_name = "ad4021",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 20),
> + },
> + [ID_AD4022] = {
> + .dev_name = "ad4022",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 20),
> + },
> + [ID_ADAQ4001] = {
> + .dev_name = "adaq4001",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 16),
> + },
> + [ID_ADAQ4003] = {
> + .dev_name = "adaq4003",
> + .chan_spec = AD4000_DIFF_CHANNEL('s', 18),
> + },
> +};
> +
Please have the above as a different variable per device rather than the array.
Likely no need for the enum then...
> +struct ad4000_state {
> + struct spi_device *spi;
> + struct gpio_desc *cnv_gpio;
> + struct spi_transfer xfers[2];
> + struct spi_message msg;
> + int vref;
> + enum ad4000_spi_mode spi_mode;
> + bool span_comp;
> + bool turbo_mode;
> + int gain_milli;
> + int scale_tbl[2][2];
> +
> + /*
> + * DMA (thus cache coherency maintenance) requires the
> + * transfer buffers to live in their own cache lines.
> + */
> + struct {
> + union {
> + __be16 sample_buf16;
> + __be32 sample_buf32;
> + } data;
> + s64 timestamp __aligned(8);
> + } scan __aligned(IIO_DMA_MINALIGN);
> + __be16 tx_buf;
> + __be16 rx_buf;
> +};
> +
> +static void ad4000_fill_scale_tbl(struct ad4000_state *st, int scale_bits,
> + const struct ad4000_chip_info *chip)
> +{
> + int diff = chip->chan_spec.differential;
> + int val, val2, tmp0, tmp1;
> + u64 tmp2;
> +
> + val2 = scale_bits;
> + val = st->vref / 1000;
> + /*
> + * The gain is stored as a fraction of 1000 and, as we need to
> + * divide vref by gain, we invert the gain/1000 fraction.
> + * Also multiply by an extra MILLI to avoid losing precision.
> + */
> + val = mult_frac(val, MILLI * MILLI, st->gain_milli);
Why not MICRO :)?
> + /* Would multiply by NANO here but we multiplied by extra MILLI */
> + tmp2 = shift_right((u64)val * MICRO, val2);
> + tmp0 = (int)div_s64_rem(tmp2, NANO, &tmp1);
no cast needed...
> + /* Store scale for when span compression is disabled */
> + st->scale_tbl[0][0] = tmp0; /* Integer part */
> + st->scale_tbl[0][1] = abs(tmp1); /* Fractional part */
> + /* Store scale for when span compression is enabled */
> + st->scale_tbl[1][0] = tmp0;
> + /* The integer part is always zero so don't bother to divide it. */
> + if (diff)
> + st->scale_tbl[1][1] = DIV_ROUND_CLOSEST(abs(tmp1) * 4, 5);
> + else
> + st->scale_tbl[1][1] = DIV_ROUND_CLOSEST(abs(tmp1) * 9, 10);
> +}
> +
> +static int ad4000_write_reg(struct ad4000_state *st, uint8_t val)
> +{
> + st->tx_buf = cpu_to_be16(AD4000_WRITE_COMMAND << BITS_PER_BYTE |
> val);
> + return spi_write(st->spi, &st->tx_buf, 2);
sizeof(tx_buf)?
> +}
> +
> +static int ad4000_read_reg(struct ad4000_state *st, unsigned int *val)
> +{
> + struct spi_transfer t[] = {
> + {
> + .tx_buf = &st->tx_buf,
> + .rx_buf = &st->rx_buf,
> + .len = 2,
> + },
> + };
> + int ret;
> +
> + st->tx_buf = cpu_to_be16(AD4000_READ_COMMAND << BITS_PER_BYTE);
> + ret = spi_sync_transfer(st->spi, t, ARRAY_SIZE(t));
> + if (ret < 0)
> + return ret;
> +
> + *val = be16_to_cpu(st->rx_buf);
> +
> + return ret;
> +}
> +
> +static void ad4000_unoptimize_msg(void *msg)
> +{
> + spi_unoptimize_message(msg);
> +}
> +
> +/*
> + * This executes a data sample transfer for when the device connections are
> + * in "3-wire" mode, selected by setting the adi,spi-mode device tree
> property
> + * to "single". In this connection mode, the ADC SDI pin is connected to MOSI
> or
> + * to VIO and ADC CNV pin is connected either to a SPI controller CS or to a
> GPIO.
> + * AD4000 series of devices initiate conversions on the rising edge of CNV
> pin.
> + *
> + * If the CNV pin is connected to an SPI controller CS line (which is by
> default
> + * active low), the ADC readings would have a latency (delay) of one read.
> + * Moreover, since we also do ADC sampling for filling the buffer on
> triggered
> + * buffer mode, the timestamps of buffer readings would be disarranged.
> + * To prevent the read latency and reduce the time discrepancy between the
> + * sample read request and the time of actual sampling by the ADC, do a
> + * preparatory transfer to pulse the CS/CNV line.
> + */
> +static int ad4000_prepare_3wire_mode_message(struct ad4000_state *st,
> + const struct iio_chan_spec
> *chan)
> +{
> + unsigned int cnv_pulse_time = st->turbo_mode ? AD4000_TQUIET1_NS
> + : AD4000_TCONV_NS;
> + struct spi_transfer *xfers = st->xfers;
> + int ret;
> +
> + xfers[0].cs_change = 1;
> + xfers[0].cs_change_delay.value = cnv_pulse_time;
> + xfers[0].cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
> +
> + xfers[1].rx_buf = &st->scan.data;
> + xfers[1].len = BITS_TO_BYTES(chan->scan_type.storagebits);
> + xfers[1].delay.value = AD4000_TQUIET2_NS;
> + xfers[1].delay.unit = SPI_DELAY_UNIT_NSECS;
> +
> + spi_message_init_with_transfers(&st->msg, st->xfers, 2);
> +
> + ret = spi_optimize_message(st->spi, &st->msg);
> + if (ret)
> + return ret;
> +
> + return devm_add_action_or_reset(&st->spi->dev, ad4000_unoptimize_msg,
> + &st->msg);
> +}
> +
> +/*
> + * This executes a data sample transfer for when the device connections are
> + * in "4-wire" mode, selected when the adi,spi-mode device tree
> + * property is absent or empty. In this connection mode, the controller CS
> pin
> + * is connected to ADC SDI pin and a GPIO is connected to ADC CNV pin.
> + * The GPIO connected to ADC CNV pin is set outside of the SPI transfer.
> + */
> +static int ad4000_prepare_4wire_mode_message(struct ad4000_state *st,
> + const struct iio_chan_spec
> *chan)
> +{
> + unsigned int cnv_to_sdi_time = st->turbo_mode ? AD4000_TQUIET1_NS
> + : AD4000_TCONV_NS;
> + struct spi_transfer *xfers = st->xfers;
> + int ret;
> +
> + /*
> + * Dummy transfer to cause enough delay between CNV going high and
> SDI
> + * going low.
> + */
> + xfers[0].cs_off = 1;
> + xfers[0].delay.value = cnv_to_sdi_time;
> + xfers[0].delay.unit = SPI_DELAY_UNIT_NSECS;
> +
> + xfers[1].rx_buf = &st->scan.data;
> + xfers[1].len = BITS_TO_BYTES(chan->scan_type.storagebits);
> +
> + spi_message_init_with_transfers(&st->msg, st->xfers, 2);
> +
> + ret = spi_optimize_message(st->spi, &st->msg);
> + if (ret)
> + return ret;
> +
> + return devm_add_action_or_reset(&st->spi->dev, ad4000_unoptimize_msg,
> + &st->msg);
> +}
> +
> +static int ad4000_convert_and_acquire(struct ad4000_state *st)
> +{
> + int ret;
> +
> + /*
> + * In 4-wire mode, the CNV line is held high for the entire
> conversion
> + * and acquisition process. In other modes st->cnv_gpio is NULL and
> is
> + * ignored (CS is wired to CNV in those cases).
> + */
> + gpiod_set_value_cansleep(st->cnv_gpio, 1);
Not sure it's a good practise to assume internal details as you're going for
GPIO. I would prefer to have an explicit check for st->cnv_gpio being NULL or
not.
> + ret = spi_sync(st->spi, &st->msg);
> + gpiod_set_value_cansleep(st->cnv_gpio, 0);
> +
> + return ret;
> +}
> +
> +static int ad4000_single_conversion(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan, int
> *val)
> +{
> + struct ad4000_state *st = iio_priv(indio_dev);
> + u32 sample;
> + int ret;
> +
> + ret = ad4000_convert_and_acquire(st);
>
no error check
> + if (chan->scan_type.storagebits > 16)
> + sample = be32_to_cpu(st->scan.data.sample_buf32);
> + else
> + sample = be16_to_cpu(st->scan.data.sample_buf16);
> +
> + switch (chan->scan_type.realbits) {
> + case 16:
> + break;
> + case 18:
> + sample = FIELD_GET(AD4000_18BIT_MSK, sample);
> + break;
> + case 20:
> + sample = FIELD_GET(AD4000_20BIT_MSK, sample);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + if (chan->scan_type.sign == 's')
> + *val = sign_extend32(sample, chan->scan_type.realbits - 1);
> +
> + return IIO_VAL_INT;
> +}
> +
> +static int ad4000_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long info)
> +{
> + struct ad4000_state *st = iio_priv(indio_dev);
> +
> + switch (info) {
> + case IIO_CHAN_INFO_RAW:
> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
> + return ad4000_single_conversion(indio_dev, chan,
> val);
> + unreachable();
> + case IIO_CHAN_INFO_SCALE:
> + *val = st->scale_tbl[st->span_comp][0];
> + *val2 = st->scale_tbl[st->span_comp][1];
> + return IIO_VAL_INT_PLUS_NANO;
> + case IIO_CHAN_INFO_OFFSET:
> + *val = 0;
> + if (st->span_comp)
> + *val = mult_frac(st->vref / 1000, 1, 10);
> +
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int ad4000_read_avail(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + const int **vals, int *type, int *length,
> + long info)
> +{
> + struct ad4000_state *st = iio_priv(indio_dev);
> +
> + switch (info) {
> + case IIO_CHAN_INFO_SCALE:
> + *vals = (int *)st->scale_tbl;
> + *length = 2 * 2;
> + *type = IIO_VAL_INT_PLUS_NANO;
> + return IIO_AVAIL_LIST;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int ad4000_write_raw_get_fmt(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, long
> mask)
> +{
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + return IIO_VAL_INT_PLUS_NANO;
> + default:
> + return IIO_VAL_INT_PLUS_MICRO;
> + }
> +}
> +
> +static int ad4000_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val, int
> val2,
> + long mask)
> +{
> + struct ad4000_state *st = iio_priv(indio_dev);
> + unsigned int reg_val;
> + bool span_comp_en;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
> + ret = ad4000_read_reg(st, ®_val);
> + if (ret < 0)
> + return ret;
> +
> + span_comp_en = (val2 == st->scale_tbl[1][1]);
no () needed
> + reg_val &= ~AD4000_CFG_SPAN_COMP;
> + reg_val |= FIELD_PREP(AD4000_CFG_SPAN_COMP,
> span_comp_en);
> +
> + ret = ad4000_write_reg(st, reg_val);
> + if (ret < 0)
> + return ret;
> +
> + st->span_comp = span_comp_en;
> + return 0;
> + }
> + unreachable();
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static irqreturn_t ad4000_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct ad4000_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + ret = ad4000_convert_and_acquire(st);
> + if (ret < 0)
> + goto err_out;
> +
> + iio_push_to_buffers_with_timestamp(indio_dev, &st->scan,
> + iio_get_time_ns(indio_dev));
> +
> +err_out:
> + iio_trigger_notify_done(indio_dev->trig);
> + return IRQ_HANDLED;
> +}
> +
> +static int ad4000_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> + unsigned int writeval, unsigned int *readval)
> +{
> + struct ad4000_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + if (readval)
> + ret = ad4000_read_reg(st, readval);
> + else
> + ret = ad4000_write_reg(st, writeval);
if (readval)
return ad4000_read_reg();
return ad4000_write_reg();
> +
> + return ret;
> +}
> +
> +static const struct iio_info ad4000_info = {
> + .read_raw = &ad4000_read_raw,
> + .read_avail = &ad4000_read_avail,
> + .write_raw = &ad4000_write_raw,
> + .write_raw_get_fmt = &ad4000_write_raw_get_fmt,
> + .debugfs_reg_access = &ad4000_reg_access,
> +
> +};
> +
> +static int ad4000_config(struct ad4000_state *st)
> +{
> + unsigned int reg_val;
> +
> + if (device_property_present(&st->spi->dev, "adi,high-z-input"))
> + reg_val |= FIELD_PREP(AD4000_CFG_HIGHZ, 1);
> +
> + /*
> + * The ADC SDI pin might be connected to controller CS line in which
> + * case the write might fail. This, however, does not prevent the
> device
> + * from functioning even though in a configuration other than the
> + * requested one.
> + */
This raises the question if there's any way to describe that through DT (if not
doing it already)? So that, if SDI is connected to CS we don't even call this?
Other question that comes to mind is that in case SDI is connected to CS, will
all writes fail? Because if that's the case we other writes (like scale) that
won't work and we should take care of that...
> + return ad4000_write_reg(st, reg_val);
> +}
> +
> +static void ad4000_regulator_disable(void *reg)
> +{
> + regulator_disable(reg);
> +}
> +
> +static int ad4000_probe(struct spi_device *spi)
> +{
> + const struct ad4000_chip_info *chip;
> + struct regulator *vref_reg;
> + struct iio_dev *indio_dev;
> + struct ad4000_state *st;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + chip = spi_get_device_match_data(spi);
> + if (!chip)
> + return -EINVAL;
> +
> + st = iio_priv(indio_dev);
> + st->spi = spi;
> +
> + ret = devm_regulator_get_enable(&spi->dev, "vdd");
> + if (ret)
> + return dev_err_probe(&spi->dev, ret, "Failed to enable VDD
> supply\n");
> +
> + ret = devm_regulator_get_enable(&spi->dev, "vio");
> + if (ret)
> + return dev_err_probe(&spi->dev, ret, "Failed to enable VIO
> supply\n");
> +
> + vref_reg = devm_regulator_get(&spi->dev, "ref");
> + if (IS_ERR(vref_reg))
> + return dev_err_probe(&spi->dev, PTR_ERR(vref_reg),
> + "Failed to get vref regulator\n");
Should this not be an optional one? If not, why not devm_regulator_get_enable()?
Also consider the new devm_regulator_get_enable_read_voltage() - you need to
handle -ENODEV in case this is optional.
> +
> + ret = regulator_enable(vref_reg);
> + if (ret < 0)
> + return dev_err_probe(&spi->dev, ret,
> + "Failed to enable voltage regulator\n");
> +
> + ret = devm_add_action_or_reset(&spi->dev, ad4000_regulator_disable,
> vref_reg);
> + if (ret)
> + return dev_err_probe(&spi->dev, ret,
> + "Failed to add regulator disable
> action\n");
> +
> + st->vref = regulator_get_voltage(vref_reg);
> + if (st->vref < 0)
> + return dev_err_probe(&spi->dev, st->vref, "Failed to get
> vref\n");
> +
I think in all places you're using this you st->vref / 1000, right? Do it here
just once...
> + st->cnv_gpio = devm_gpiod_get_optional(&spi->dev, "cnv",
> GPIOD_OUT_HIGH);
> + if (IS_ERR(st->cnv_gpio))
> + return dev_err_probe(&spi->dev, PTR_ERR(st->cnv_gpio),
> + "Failed to get CNV GPIO");
> +
> + ret = device_property_match_property_string(&spi->dev, "adi,spi-
> mode",
> + ad4000_spi_modes,
> +
> ARRAY_SIZE(ad4000_spi_modes));
> + /* Default to 4-wire mode if adi,spi-mode property is not present */
> + if (ret == -EINVAL)
> + st->spi_mode = AD4000_SPI_MODE_DEFAULT;
> + else if (ret < 0)
> + return dev_err_probe(&spi->dev, ret,
> + "getting adi,spi-mode property
> failed\n");
> + else
> + st->spi_mode = ret;
> +
> + switch (st->spi_mode) {
> + case AD4000_SPI_MODE_DEFAULT:
> + ret = ad4000_prepare_4wire_mode_message(st, &chip-
> >chan_spec);
> + if (ret)
> + return ret;
> +
> + break;
> + case AD4000_SPI_MODE_SINGLE:
> + ret = ad4000_prepare_3wire_mode_message(st, &chip-
> >chan_spec);
> + if (ret)
> + return ret;
> +
> + /*
> + * In "3-wire mode", the ADC SDI line must be kept high when
> + * data is not being clocked out of the controller.
> + * Request the SPI controller to make MOSI idle high.
> + */
> + spi->mode = SPI_MODE_0 | SPI_MOSI_IDLE_HIGH;
> + if (spi_setup(spi))
> + dev_warn(&st->spi->dev, "SPI controller setup
> failed\n");
Does not look like a warn to me... Also, spi_setup() should already print an
error message so this will duplicate that.
> +
> + break;
> + }
> +
> + ret = ad4000_config(st);
> + if (ret < 0)
> + dev_dbg(&st->spi->dev, "Failed to config device\n");
Also questionable but see the my comment on ad4000_config(). In any case this
should be visible to the user so I would at least use dev_warn().
> +
> + indio_dev->name = chip->dev_name;
> + indio_dev->info = &ad4000_info;
> + indio_dev->channels = &chip->chan_spec;
> + indio_dev->num_channels = 1;
> +
> + /* Hardware gain only applies to ADAQ devices */
> + st->gain_milli = 1000;
> + if (device_property_present(&spi->dev, "adi,gain-milli")) {
> +
> + ret = device_property_read_u32(&spi->dev, "adi,gain-milli",
> + &st->gain_milli);
Can we have full unsigned int range for the gain? I guess not :)
- Nuno Sá
On 6/5/24 7:24 AM, Mark Brown wrote:
> On Tue, Jun 04, 2024 at 07:41:47PM -0300, Marcelo Schmitt wrote:
>
>> The behavior of an SPI controller data output line (SDO or MOSI or COPI
>> (Controller Output Peripheral Input) for disambiguation) is not specified
>> when the controller is not clocking out data on SCLK edges. However, there
>> exist SPI peripherals that require specific COPI line state when data is
>> not being clocked out of the controller.
I think this description is missing a key detail that the tx data line
needs to be high just before and also during the CS assertion at the start
of each message.
And it would be helpful to have this more detailed description in the
source code somewhere and not just in the commit message.
>
> This is an optimisation for accelerating devices that need a specific
> value, really if these devices need a value they should send it.
>
>> #define SPI_MOSI_IDLE_LOW _BITUL(17) /* leave mosi line low when idle */
>> +#define SPI_MOSI_IDLE_HIGH _BITUL(18) /* leave mosi line high when idle */
>
> Realistically we'll have a large set of drivers that are expecting the
> line to be held low so I'm not sure we need that option. I would also
> expect to have an implementation of these options in the core which
> supplies buffers with the relevant data for use with controllers that
> don't have the feature (similar to how _MUST_TX and _MUST_RX are done).
> Even without that we'd need feature detection so that drivers that try
> to use this aren't just buggy when used with a controller that doesn't
> implement it, but once you're detecting you may as well just make things
> work.
I could see something like this working for controllers that leave the
tx data line in the state of the last bit of a write transfer. I.e. do a
write transfer of 0xff (using the smallest number of bits per word
supported by the controller) with CS not asserted, then assert CS, then
do the rest of actual the transfers requested by the peripheral.
But it doesn't seem like it would work for controllers that always
return the tx data line to a low state after a write since this would
mean that the data line would still be low during the CS assertion
which is what we need to prevent.
On 6/4/24 5:43 PM, Marcelo Schmitt wrote:
> Implement MOSI idle low and MOSI idle high to better support peripherals
> that request specific MOSI behavior.
>
> Signed-off-by: Marcelo Schmitt <[email protected]>
> ---
> drivers/spi/spi-axi-spi-engine.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c
> index 0aa31d745734..549f03069d0e 100644
> --- a/drivers/spi/spi-axi-spi-engine.c
> +++ b/drivers/spi/spi-axi-spi-engine.c
> @@ -41,6 +41,7 @@
> #define SPI_ENGINE_CONFIG_CPHA BIT(0)
> #define SPI_ENGINE_CONFIG_CPOL BIT(1)
> #define SPI_ENGINE_CONFIG_3WIRE BIT(2)
> +#define SPI_ENGINE_CONFIG_SDO_IDLE BIT(3)
>
> #define SPI_ENGINE_INST_TRANSFER 0x0
> #define SPI_ENGINE_INST_ASSERT 0x1
> @@ -132,6 +133,10 @@ static unsigned int spi_engine_get_config(struct spi_device *spi)
> config |= SPI_ENGINE_CONFIG_CPHA;
> if (spi->mode & SPI_3WIRE)
> config |= SPI_ENGINE_CONFIG_3WIRE;
> + if (spi->mode & SPI_MOSI_IDLE_HIGH)
> + config |= SPI_ENGINE_CONFIG_SDO_IDLE;
> + if (spi->mode & SPI_MOSI_IDLE_LOW)
> + config &= ~SPI_ENGINE_CONFIG_SDO_IDLE;
>
> return config;
> }
> @@ -645,7 +650,8 @@ static int spi_engine_probe(struct platform_device *pdev)
> return ret;
>
> host->dev.of_node = pdev->dev.of_node;
> - host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE;
> + host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE | SPI_MOSI_IDLE_LOW
> + | SPI_MOSI_IDLE_HIGH;
> host->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
> host->max_speed_hz = clk_get_rate(spi_engine->ref_clk) / 2;
> host->transfer_one_message = spi_engine_transfer_one_message;
I think we need a version check instead of setting the flags unconditionally
here since older versions of the AXI SPI Engine won't support this feature.
On Wed, Jun 05, 2024 at 11:37:48AM -0500, David Lechner wrote:
> On 6/5/24 7:24 AM, Mark Brown wrote:
> > On Tue, Jun 04, 2024 at 07:41:47PM -0300, Marcelo Schmitt wrote:
> >> The behavior of an SPI controller data output line (SDO or MOSI or COPI
> >> (Controller Output Peripheral Input) for disambiguation) is not specified
> >> when the controller is not clocking out data on SCLK edges. However, there
> >> exist SPI peripherals that require specific COPI line state when data is
> >> not being clocked out of the controller.
> I think this description is missing a key detail that the tx data line
> needs to be high just before and also during the CS assertion at the start
> of each message.
> And it would be helpful to have this more detailed description in the
> source code somewhere and not just in the commit message.
Yes, there's no way anyone could infer this from any aspect of the
series. I think the properties also need a clearer name since someone
might want the accelerator functionality at some point.
> > Even without that we'd need feature detection so that drivers that try
> > to use this aren't just buggy when used with a controller that doesn't
> > implement it, but once you're detecting you may as well just make things
> > work.
> I could see something like this working for controllers that leave the
> tx data line in the state of the last bit of a write transfer. I.e. do a
> write transfer of 0xff (using the smallest number of bits per word
> supported by the controller) with CS not asserted, then assert CS, then
> do the rest of actual the transfers requested by the peripheral.
> But it doesn't seem like it would work for controllers that always
> return the tx data line to a low state after a write since this would
> mean that the data line would still be low during the CS assertion
> which is what we need to prevent.
With the additional requirement it's not emulatable, but we'd still need
the checks in the core.
On Tue, Jun 04, 2024 at 07:43:53PM -0300, Marcelo Schmitt wrote:
> Add device tree documentation for AD4000 series of ADC devices.
>
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
>
> Suggested-by: David Lechner <[email protected]>
A suggested-by on a binding? That's unusual...
> Signed-off-by: Marcelo Schmitt <[email protected]>
> ---
> Even though didn't pick all suggestions to the dt-bindings, I did pick most them
> so kept David's Suggested-by tag.
>
> .../bindings/iio/adc/adi,ad4000.yaml | 207 ++++++++++++++++++
> MAINTAINERS | 7 +
> 2 files changed, 214 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> new file mode 100644
> index 000000000000..7470d386906b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> @@ -0,0 +1,207 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad4000.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD4000 and similar Analog to Digital Converters
> +
> +maintainers:
> + - Marcelo Schmitt <[email protected]>
> +
> +description: |
> + Analog Devices AD4000 family of Analog to Digital Converters with SPI support.
> + Specifications can be found at:
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
> + https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
> +
> +$ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> + compatible:
> + enum:
> + - adi,ad4000
> + - adi,ad4001
> + - adi,ad4002
> + - adi,ad4003
> + - adi,ad4004
> + - adi,ad4005
> + - adi,ad4006
> + - adi,ad4007
> + - adi,ad4008
> + - adi,ad4010
> + - adi,ad4011
> + - adi,ad4020
> + - adi,ad4021
> + - adi,ad4022
> + - adi,adaq4001
> + - adi,adaq4003
Are all these actually incompatible? I'd like a note in the commit
message as to why that's the case. A quick look at the driver showed
that the differences in the driver between the ad402{0,1,2} are limited
to the "dev_name". Same went for some other devices, like the
ad40{02,06,10}.
Thanks,
Conor.
Hi Marcelo,
kernel test robot noticed the following build warnings:
[auto build test WARNING on broonie-spi/for-next]
[also build test WARNING on jic23-iio/togreg linus/master v6.10-rc2 next-20240605]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Marcelo-Schmitt/spi-Add-SPI-mode-bit-for-MOSI-idle-state-configuration/20240605-231912
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
patch link: https://lore.kernel.org/r/e340f48324b0ea3afb1c715cb2fba184c27112a1.1717539384.git.marcelo.schmitt%40analog.com
patch subject: [PATCH v3 6/6] iio: adc: Add support for AD4000
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20240606/[email protected]/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240606/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
drivers/iio/adc/ad4000.c: In function 'ad4000_single_conversion':
>> drivers/iio/adc/ad4000.c:375:13: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
375 | int ret;
| ^~~
vim +/ret +375 drivers/iio/adc/ad4000.c
369
370 static int ad4000_single_conversion(struct iio_dev *indio_dev,
371 const struct iio_chan_spec *chan, int *val)
372 {
373 struct ad4000_state *st = iio_priv(indio_dev);
374 u32 sample;
> 375 int ret;
376
377 ret = ad4000_convert_and_acquire(st);
378
379 if (chan->scan_type.storagebits > 16)
380 sample = be32_to_cpu(st->scan.data.sample_buf32);
381 else
382 sample = be16_to_cpu(st->scan.data.sample_buf16);
383
384 switch (chan->scan_type.realbits) {
385 case 16:
386 break;
387 case 18:
388 sample = FIELD_GET(AD4000_18BIT_MSK, sample);
389 break;
390 case 20:
391 sample = FIELD_GET(AD4000_20BIT_MSK, sample);
392 break;
393 default:
394 return -EINVAL;
395 }
396
397 if (chan->scan_type.sign == 's')
398 *val = sign_extend32(sample, chan->scan_type.realbits - 1);
399
400 return IIO_VAL_INT;
401 }
402
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Marcelo,
kernel test robot noticed the following build warnings:
[auto build test WARNING on broonie-spi/for-next]
[also build test WARNING on jic23-iio/togreg linus/master v6.10-rc2 next-20240605]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Marcelo-Schmitt/spi-Add-SPI-mode-bit-for-MOSI-idle-state-configuration/20240605-231912
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
patch link: https://lore.kernel.org/r/e340f48324b0ea3afb1c715cb2fba184c27112a1.1717539384.git.marcelo.schmitt%40analog.com
patch subject: [PATCH v3 6/6] iio: adc: Add support for AD4000
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20240606/[email protected]/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project d7d2d4f53fc79b4b58e8d8d08151b577c3699d4a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240606/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
In file included from drivers/iio/adc/ad4000.c:17:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
548 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
561 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from drivers/iio/adc/ad4000.c:17:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
574 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from drivers/iio/adc/ad4000.c:17:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/hexagon/include/asm/io.h:328:
include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
585 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
595 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
605 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
In file included from drivers/iio/adc/ad4000.c:17:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:21:
In file included from include/linux/mm.h:2253:
include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
514 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
drivers/iio/adc/ad4000.c:375:6: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
375 | int ret;
| ^
>> drivers/iio/adc/ad4000.c:538:3: warning: variable 'reg_val' is uninitialized when used here [-Wuninitialized]
538 | reg_val |= FIELD_PREP(AD4000_CFG_HIGHZ, 1);
| ^~~~~~~
drivers/iio/adc/ad4000.c:535:22: note: initialize the variable 'reg_val' to silence this warning
535 | unsigned int reg_val;
| ^
| = 0
9 warnings generated.
vim +/reg_val +538 drivers/iio/adc/ad4000.c
369
370 static int ad4000_single_conversion(struct iio_dev *indio_dev,
371 const struct iio_chan_spec *chan, int *val)
372 {
373 struct ad4000_state *st = iio_priv(indio_dev);
374 u32 sample;
> 375 int ret;
376
377 ret = ad4000_convert_and_acquire(st);
378
379 if (chan->scan_type.storagebits > 16)
380 sample = be32_to_cpu(st->scan.data.sample_buf32);
381 else
382 sample = be16_to_cpu(st->scan.data.sample_buf16);
383
384 switch (chan->scan_type.realbits) {
385 case 16:
386 break;
387 case 18:
388 sample = FIELD_GET(AD4000_18BIT_MSK, sample);
389 break;
390 case 20:
391 sample = FIELD_GET(AD4000_20BIT_MSK, sample);
392 break;
393 default:
394 return -EINVAL;
395 }
396
397 if (chan->scan_type.sign == 's')
398 *val = sign_extend32(sample, chan->scan_type.realbits - 1);
399
400 return IIO_VAL_INT;
401 }
402
403 static int ad4000_read_raw(struct iio_dev *indio_dev,
404 struct iio_chan_spec const *chan, int *val,
405 int *val2, long info)
406 {
407 struct ad4000_state *st = iio_priv(indio_dev);
408
409 switch (info) {
410 case IIO_CHAN_INFO_RAW:
411 iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
412 return ad4000_single_conversion(indio_dev, chan, val);
413 unreachable();
414 case IIO_CHAN_INFO_SCALE:
415 *val = st->scale_tbl[st->span_comp][0];
416 *val2 = st->scale_tbl[st->span_comp][1];
417 return IIO_VAL_INT_PLUS_NANO;
418 case IIO_CHAN_INFO_OFFSET:
419 *val = 0;
420 if (st->span_comp)
421 *val = mult_frac(st->vref / 1000, 1, 10);
422
423 return IIO_VAL_INT;
424 default:
425 return -EINVAL;
426 }
427 }
428
429 static int ad4000_read_avail(struct iio_dev *indio_dev,
430 struct iio_chan_spec const *chan,
431 const int **vals, int *type, int *length,
432 long info)
433 {
434 struct ad4000_state *st = iio_priv(indio_dev);
435
436 switch (info) {
437 case IIO_CHAN_INFO_SCALE:
438 *vals = (int *)st->scale_tbl;
439 *length = 2 * 2;
440 *type = IIO_VAL_INT_PLUS_NANO;
441 return IIO_AVAIL_LIST;
442 default:
443 return -EINVAL;
444 }
445 }
446
447 static int ad4000_write_raw_get_fmt(struct iio_dev *indio_dev,
448 struct iio_chan_spec const *chan, long mask)
449 {
450 switch (mask) {
451 case IIO_CHAN_INFO_SCALE:
452 return IIO_VAL_INT_PLUS_NANO;
453 default:
454 return IIO_VAL_INT_PLUS_MICRO;
455 }
456 }
457
458 static int ad4000_write_raw(struct iio_dev *indio_dev,
459 struct iio_chan_spec const *chan, int val, int val2,
460 long mask)
461 {
462 struct ad4000_state *st = iio_priv(indio_dev);
463 unsigned int reg_val;
464 bool span_comp_en;
465 int ret;
466
467 switch (mask) {
468 case IIO_CHAN_INFO_SCALE:
469 iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
470 ret = ad4000_read_reg(st, ®_val);
471 if (ret < 0)
472 return ret;
473
474 span_comp_en = (val2 == st->scale_tbl[1][1]);
475 reg_val &= ~AD4000_CFG_SPAN_COMP;
476 reg_val |= FIELD_PREP(AD4000_CFG_SPAN_COMP, span_comp_en);
477
478 ret = ad4000_write_reg(st, reg_val);
479 if (ret < 0)
480 return ret;
481
482 st->span_comp = span_comp_en;
483 return 0;
484 }
485 unreachable();
486 default:
487 return -EINVAL;
488 }
489 }
490
491 static irqreturn_t ad4000_trigger_handler(int irq, void *p)
492 {
493 struct iio_poll_func *pf = p;
494 struct iio_dev *indio_dev = pf->indio_dev;
495 struct ad4000_state *st = iio_priv(indio_dev);
496 int ret;
497
498 ret = ad4000_convert_and_acquire(st);
499 if (ret < 0)
500 goto err_out;
501
502 iio_push_to_buffers_with_timestamp(indio_dev, &st->scan,
503 iio_get_time_ns(indio_dev));
504
505 err_out:
506 iio_trigger_notify_done(indio_dev->trig);
507 return IRQ_HANDLED;
508 }
509
510 static int ad4000_reg_access(struct iio_dev *indio_dev, unsigned int reg,
511 unsigned int writeval, unsigned int *readval)
512 {
513 struct ad4000_state *st = iio_priv(indio_dev);
514 int ret;
515
516 if (readval)
517 ret = ad4000_read_reg(st, readval);
518 else
519 ret = ad4000_write_reg(st, writeval);
520
521 return ret;
522 }
523
524 static const struct iio_info ad4000_info = {
525 .read_raw = &ad4000_read_raw,
526 .read_avail = &ad4000_read_avail,
527 .write_raw = &ad4000_write_raw,
528 .write_raw_get_fmt = &ad4000_write_raw_get_fmt,
529 .debugfs_reg_access = &ad4000_reg_access,
530
531 };
532
533 static int ad4000_config(struct ad4000_state *st)
534 {
535 unsigned int reg_val;
536
537 if (device_property_present(&st->spi->dev, "adi,high-z-input"))
> 538 reg_val |= FIELD_PREP(AD4000_CFG_HIGHZ, 1);
539
540 /*
541 * The ADC SDI pin might be connected to controller CS line in which
542 * case the write might fail. This, however, does not prevent the device
543 * from functioning even though in a configuration other than the
544 * requested one.
545 */
546 return ad4000_write_reg(st, reg_val);
547 }
548
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Wed, 2024-06-05 at 12:03 -0500, David Lechner wrote:
> On 6/4/24 5:43 PM, Marcelo Schmitt wrote:
> > Implement MOSI idle low and MOSI idle high to better support peripherals
> > that request specific MOSI behavior.
> >
> > Signed-off-by: Marcelo Schmitt <[email protected]>
> > ---
> > drivers/spi/spi-axi-spi-engine.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-
> > engine.c
> > index 0aa31d745734..549f03069d0e 100644
> > --- a/drivers/spi/spi-axi-spi-engine.c
> > +++ b/drivers/spi/spi-axi-spi-engine.c
> > @@ -41,6 +41,7 @@
> > #define SPI_ENGINE_CONFIG_CPHA BIT(0)
> > #define SPI_ENGINE_CONFIG_CPOL BIT(1)
> > #define SPI_ENGINE_CONFIG_3WIRE BIT(2)
> > +#define SPI_ENGINE_CONFIG_SDO_IDLE BIT(3)
> >
> > #define SPI_ENGINE_INST_TRANSFER 0x0
> > #define SPI_ENGINE_INST_ASSERT 0x1
> > @@ -132,6 +133,10 @@ static unsigned int spi_engine_get_config(struct
> > spi_device *spi)
> > config |= SPI_ENGINE_CONFIG_CPHA;
> > if (spi->mode & SPI_3WIRE)
> > config |= SPI_ENGINE_CONFIG_3WIRE;
> > + if (spi->mode & SPI_MOSI_IDLE_HIGH)
> > + config |= SPI_ENGINE_CONFIG_SDO_IDLE;
> > + if (spi->mode & SPI_MOSI_IDLE_LOW)
> > + config &= ~SPI_ENGINE_CONFIG_SDO_IDLE;
> >
> > return config;
> > }
> > @@ -645,7 +650,8 @@ static int spi_engine_probe(struct platform_device
> > *pdev)
> > return ret;
> >
> > host->dev.of_node = pdev->dev.of_node;
> > - host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE;
> > + host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE |
> > SPI_MOSI_IDLE_LOW
> > + | SPI_MOSI_IDLE_HIGH;
> > host->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
> > host->max_speed_hz = clk_get_rate(spi_engine->ref_clk) / 2;
> > host->transfer_one_message = spi_engine_transfer_one_message;
>
> I think we need a version check instead of setting the flags unconditionally
> here since older versions of the AXI SPI Engine won't support this feature.
Oh, was not aware of that... Then, we definitely need to do that. Marcelo, only
add my r-b tag with the version change in place.
- Nuno Sá
On 6/6/24 1:51 AM, Nuno Sá wrote:
> On Wed, 2024-06-05 at 12:03 -0500, David Lechner wrote:
>> On 6/4/24 5:43 PM, Marcelo Schmitt wrote:
>>> Implement MOSI idle low and MOSI idle high to better support peripherals
>>> that request specific MOSI behavior.
>>>
>>> Signed-off-by: Marcelo Schmitt <[email protected]>
>>> ---
>>> drivers/spi/spi-axi-spi-engine.c | 8 +++++++-
>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-
>>> engine.c
>>> index 0aa31d745734..549f03069d0e 100644
>>> --- a/drivers/spi/spi-axi-spi-engine.c
>>> +++ b/drivers/spi/spi-axi-spi-engine.c
>>> @@ -41,6 +41,7 @@
>>> #define SPI_ENGINE_CONFIG_CPHA BIT(0)
>>> #define SPI_ENGINE_CONFIG_CPOL BIT(1)
>>> #define SPI_ENGINE_CONFIG_3WIRE BIT(2)
>>> +#define SPI_ENGINE_CONFIG_SDO_IDLE BIT(3)
>>>
>>> #define SPI_ENGINE_INST_TRANSFER 0x0
>>> #define SPI_ENGINE_INST_ASSERT 0x1
>>> @@ -132,6 +133,10 @@ static unsigned int spi_engine_get_config(struct
>>> spi_device *spi)
>>> config |= SPI_ENGINE_CONFIG_CPHA;
>>> if (spi->mode & SPI_3WIRE)
>>> config |= SPI_ENGINE_CONFIG_3WIRE;
>>> + if (spi->mode & SPI_MOSI_IDLE_HIGH)
>>> + config |= SPI_ENGINE_CONFIG_SDO_IDLE;
>>> + if (spi->mode & SPI_MOSI_IDLE_LOW)
>>> + config &= ~SPI_ENGINE_CONFIG_SDO_IDLE;
>>>
>>> return config;
>>> }
>>> @@ -645,7 +650,8 @@ static int spi_engine_probe(struct platform_device
>>> *pdev)
>>> return ret;
>>>
>>> host->dev.of_node = pdev->dev.of_node;
>>> - host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE;
>>> + host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE |
>>> SPI_MOSI_IDLE_LOW
>>> + | SPI_MOSI_IDLE_HIGH;
>>> host->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
>>> host->max_speed_hz = clk_get_rate(spi_engine->ref_clk) / 2;
>>> host->transfer_one_message = spi_engine_transfer_one_message;
>>
>> I think we need a version check instead of setting the flags unconditionally
>> here since older versions of the AXI SPI Engine won't support this feature.
>
> Oh, was not aware of that... Then, we definitely need to do that. Marcelo, only
> add my r-b tag with the version change in place.
>
> - Nuno Sá
Actually, looking at [1], it looks like this could be a compile-time
flag when the HDL is built. If it stays that way, then we would need
a way to read that flag from a register instead of using the version.
[1]: https://github.com/analogdevicesinc/hdl/pull/1320#issuecomment-2145744521
Hi,
On 06/05, Mark Brown wrote:
> On Tue, Jun 04, 2024 at 07:41:47PM -0300, Marcelo Schmitt wrote:
>
> > The behavior of an SPI controller data output line (SDO or MOSI or COPI
> > (Controller Output Peripheral Input) for disambiguation) is not specified
> > when the controller is not clocking out data on SCLK edges. However, there
> > exist SPI peripherals that require specific COPI line state when data is
> > not being clocked out of the controller.
>
> This is an optimisation for accelerating devices that need a specific
> value, really if these devices need a value they should send it.
I see it more like an extension of SPI controller functionality.
Though I guess it might also be used for optimization if tx is known to be
always 0s or always 1s for a device.
>
> > #define SPI_MOSI_IDLE_LOW _BITUL(17) /* leave mosi line low when idle */
> > +#define SPI_MOSI_IDLE_HIGH _BITUL(18) /* leave mosi line high when idle */
>
> Realistically we'll have a large set of drivers that are expecting the
> line to be held low so I'm not sure we need that option. I would also
Yes, I also think most SPI devices, if ever requiring anything, would
expect the MOSI line to be low. But this patchset is about the exception to that. :)
> expect to have an implementation of these options in the core which
> supplies buffers with the relevant data for use with controllers that
> don't have the feature (similar to how _MUST_TX and _MUST_RX are done).
> Even without that we'd need feature detection so that drivers that try
> to use this aren't just buggy when used with a controller that doesn't
> implement it, but once you're detecting you may as well just make things
> work.
As far as I searched, the definitions for SPI protocol usually don't specify any
behavior for the MOSI line when the controller is not clocking out data.
So, I think SPI controllers that are not capable of implementing any type
of MOSI idle configuration are anyway compliant to what is usual SPI.
For those that can implement such feature, I thought peripherals could request
it by setting SPI mode bits.
If the controller can provide MOSI idle state configuration, then the controller
sets itself to act according to what peripheral asked.
If MOSI idle configuration is not supported, then we just move on and let
peripheral driver adapt to what is supported?
Guess we can't blame an SPI controller for it not supporting something that is
not specified in usual SPI protocols.
But yeah, it's not that evident what this patch set is all about and why this is
wanted so I made a wiki page to explain the reasoning for this set.
https://wiki.analog.com/software/linux/docs/spi/spi_copi_idle?rev=1717699755
Hopefully the figures with timing diagrams and transfer captures there will
provide quicker understanding of this rather than I try to explain it with
only text.
If you still think we need feature detection for MOSI idle capability just let
me know, I'll implement what be needed.
Thanks,
Marcelo
On 06/05, Mark Brown wrote:
> On Wed, Jun 05, 2024 at 11:14:33AM +0200, Nuno S? wrote:
> > On Tue, 2024-06-04 at 19:41 -0300, Marcelo Schmitt wrote:
>
> > > + /* Check against conflicting MOSI idle configuration */
> > > + if ((spi->mode & SPI_MOSI_IDLE_LOW) && (spi->mode &
> > > SPI_MOSI_IDLE_HIGH)) {
> > > + dev_warn(&spi->dev,
> > > + "setup: erratic MOSI idle configuration. Set to idle
> > > low\n");
> > > + spi->mode &= ~SPI_MOSI_IDLE_HIGH;
> > > + }
>
> > Should we assume such a thing? IOW, should this be treated as a warning or a
> > real error? I would assume this should be a configuration error and return -
> > EINVAL but...
>
> Right, and the error message isn't very clear.
Yeah, the message is not all that clear. I'll think of something better.
I'm biased towards having this as a warning because I don't see this as a
feature of usual SPI protocol but not really sure about either ...
On 06/06, David Lechner wrote:
> On 6/6/24 1:51 AM, Nuno S? wrote:
> > On Wed, 2024-06-05 at 12:03 -0500, David Lechner wrote:
> >> On 6/4/24 5:43 PM, Marcelo Schmitt wrote:
> >>> Implement MOSI idle low and MOSI idle high to better support peripherals
> >>> that request specific MOSI behavior.
> >>>
> >>> Signed-off-by: Marcelo Schmitt <[email protected]>
> >>> ---
> >>> ?drivers/spi/spi-axi-spi-engine.c | 8 +++++++-
> >>> ?1 file changed, 7 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-
> >>> engine.c
> >>> index 0aa31d745734..549f03069d0e 100644
> >>> --- a/drivers/spi/spi-axi-spi-engine.c
> >>> +++ b/drivers/spi/spi-axi-spi-engine.c
> >>> @@ -41,6 +41,7 @@
> >>> ?#define SPI_ENGINE_CONFIG_CPHA BIT(0)
> >>> ?#define SPI_ENGINE_CONFIG_CPOL BIT(1)
> >>> ?#define SPI_ENGINE_CONFIG_3WIRE BIT(2)
> >>> +#define SPI_ENGINE_CONFIG_SDO_IDLE BIT(3)
> >>> ?
> >>> ?#define SPI_ENGINE_INST_TRANSFER 0x0
> >>> ?#define SPI_ENGINE_INST_ASSERT 0x1
> >>> @@ -132,6 +133,10 @@ static unsigned int spi_engine_get_config(struct
> >>> spi_device *spi)
> >>> ? config |= SPI_ENGINE_CONFIG_CPHA;
> >>> ? if (spi->mode & SPI_3WIRE)
> >>> ? config |= SPI_ENGINE_CONFIG_3WIRE;
> >>> + if (spi->mode & SPI_MOSI_IDLE_HIGH)
> >>> + config |= SPI_ENGINE_CONFIG_SDO_IDLE;
> >>> + if (spi->mode & SPI_MOSI_IDLE_LOW)
> >>> + config &= ~SPI_ENGINE_CONFIG_SDO_IDLE;
> >>> ?
> >>> ? return config;
> >>> ?}
> >>> @@ -645,7 +650,8 @@ static int spi_engine_probe(struct platform_device
> >>> *pdev)
> >>> ? return ret;
> >>> ?
> >>> ? host->dev.of_node = pdev->dev.of_node;
> >>> - host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE;
> >>> + host->mode_bits = SPI_CPOL | SPI_CPHA | SPI_3WIRE |
> >>> SPI_MOSI_IDLE_LOW
> >>> + ? | SPI_MOSI_IDLE_HIGH;
> >>> ? host->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
> >>> ? host->max_speed_hz = clk_get_rate(spi_engine->ref_clk) / 2;
> >>> ? host->transfer_one_message = spi_engine_transfer_one_message;
> >>
> >> I think we need a version check instead of setting the flags unconditionally
> >> here since older versions of the AXI SPI Engine won't support this feature.
> >
> > Oh, was not aware of that... Then, we definitely need to do that. Marcelo, only
> > add my r-b tag with the version change in place.
> >
> > - Nuno S?
Nuno,
I think there will be more disscussion about this series.
Maybe better I not add the tag at all so you may check to agree with the next
patch version.
>
> Actually, looking at [1], it looks like this could be a compile-time
> flag when the HDL is built. If it stays that way, then we would need
> a way to read that flag from a register instead of using the version.
>
>
> [1]: https://github.com/analogdevicesinc/hdl/pull/1320#issuecomment-2145744521
When is a driver version check needed?
Yes, older versions of SPI-Engine won't support this, but the patch set should
cause no regression. Even if loading the current ad4000 driver with
older SPI-Engine HDL and driver, the ADC driver would get a warn (or error?)
and do what's possible without MOSI idle feature (probably only be able to do
reg access) or fail probing.
We decided to have the MOSI idle state feature for SPI-Engine configured by
writing to a dedicated bit [1] in the SPI Configuration Register [2].
Does this looks good?
[1]: https://github.com/analogdevicesinc/hdl/pull/1320/commits/941937eedae6701d253b4930d8f279c21ef3f807#diff-dc9213744b55493ca9430cd02cd62212436c2379ca121d1a2681356e6a37e22dR257
[2]: https://analogdevicesinc.github.io/hdl/library/spi_engine/instruction-format.html#spi-configuration-register
Thanks,
Marcelo
On 06/05, Mark Brown wrote:
> On Wed, Jun 05, 2024 at 11:37:48AM -0500, David Lechner wrote:
> > On 6/5/24 7:24 AM, Mark Brown wrote:
> > > On Tue, Jun 04, 2024 at 07:41:47PM -0300, Marcelo Schmitt wrote:
>
> > >> The behavior of an SPI controller data output line (SDO or MOSI or COPI
> > >> (Controller Output Peripheral Input) for disambiguation) is not specified
> > >> when the controller is not clocking out data on SCLK edges. However, there
> > >> exist SPI peripherals that require specific COPI line state when data is
> > >> not being clocked out of the controller.
>
> > I think this description is missing a key detail that the tx data line
> > needs to be high just before and also during the CS assertion at the start
> > of each message.
>
> > And it would be helpful to have this more detailed description in the
> > source code somewhere and not just in the commit message.
>
> Yes, there's no way anyone could infer this from any aspect of the
> series. I think the properties also need a clearer name since someone
> might want the accelerator functionality at some point.
So, if I understand correctly, it would be desirable to also have flags and
descriptions for the MOSI idle configuration capabilities in include/linux/spi/spi.h.
Does the following definitions look good?
#define SPI_CONTROLLER_MOSI_IDLE_LOW BIT(8)
#define SPI_CONTROLLER_MOSI_IDLE_HIGH BIT(9)
Maybe also document the MOSI idle configuration feature in spi-summary.rst?
>
> > > Even without that we'd need feature detection so that drivers that try
> > > to use this aren't just buggy when used with a controller that doesn't
> > > implement it, but once you're detecting you may as well just make things
> > > work.
>
> > I could see something like this working for controllers that leave the
> > tx data line in the state of the last bit of a write transfer. I.e. do a
> > write transfer of 0xff (using the smallest number of bits per word
> > supported by the controller) with CS not asserted, then assert CS, then
> > do the rest of actual the transfers requested by the peripheral.
>
> > But it doesn't seem like it would work for controllers that always
> > return the tx data line to a low state after a write since this would
> > mean that the data line would still be low during the CS assertion
> > which is what we need to prevent.
>
> With the additional requirement it's not emulatable, but we'd still need
> the checks in the core.
On Thu, 2024-06-06 at 18:31 -0300, Marcelo Schmitt wrote:
...
>
>
>
> When is a driver version check needed?
> Yes, older versions of SPI-Engine won't support this, but the patch set should
> cause no regression. Even if loading the current ad4000 driver with
> older SPI-Engine HDL and driver, the ADC driver would get a warn (or error?)
> and do what's possible without MOSI idle feature (probably only be able to do
> reg access) or fail probing.
>
Maybe I'm missing something but with the patchset we unconditionally set
SPI_MOSI_IDLE_HIGH. So if we load an hdl which does not support it things will
apparently be ok but it won't actually work, right? If I'm right we should have
a bit in a RO config_register telling us that the feature is being supported or
not. That way we only set the mode bit if we do support it...
- Nuno Sá
On Thu, Jun 06, 2024 at 07:08:30PM -0300, Marcelo Schmitt wrote:
> So, if I understand correctly, it would be desirable to also have flags and
> descriptions for the MOSI idle configuration capabilities in include/linux/spi/spi.h.
> Does the following definitions look good?
> #define SPI_CONTROLLER_MOSI_IDLE_LOW BIT(8)
> #define SPI_CONTROLLER_MOSI_IDLE_HIGH BIT(9)
Yes.
> Maybe also document the MOSI idle configuration feature in spi-summary.rst?
Yes.
On Thu, Jun 06, 2024 at 05:10:04PM -0300, Marcelo Schmitt wrote:
> On 06/05, Mark Brown wrote:
> > > Should we assume such a thing? IOW, should this be treated as a warning or a
> > > real error? I would assume this should be a configuration error and return -
> > > EINVAL but...
> > Right, and the error message isn't very clear.
> Yeah, the message is not all that clear. I'll think of something better.
> I'm biased towards having this as a warning because I don't see this as a
> feature of usual SPI protocol but not really sure about either ...
The less usual it is the more likely it is that it won't be supported
and we should actually check that to try to avoid data corruption.
On Thu, Jun 06, 2024 at 04:57:52PM -0300, Marcelo Schmitt wrote:
> As far as I searched, the definitions for SPI protocol usually don't specify any
> behavior for the MOSI line when the controller is not clocking out data.
> So, I think SPI controllers that are not capable of implementing any type
> of MOSI idle configuration are anyway compliant to what is usual SPI.
> For those that can implement such feature, I thought peripherals could request
> it by setting SPI mode bits.
The issue here is the one that Richard highlighted with it not being
clear exactly what the intended behaviour is.
> But yeah, it's not that evident what this patch set is all about and why this is
> wanted so I made a wiki page to explain the reasoning for this set.
> https://wiki.analog.com/software/linux/docs/spi/spi_copi_idle?rev=1717699755
> Hopefully the figures with timing diagrams and transfer captures there will
> provide quicker understanding of this rather than I try to explain it with
> only text.
It needs to be apparent to someone looking at the kernel what the code
is intended to do.
> If you still think we need feature detection for MOSI idle capability just let
> me know, I'll implement what be needed.
If the devices actually require this mode then we can't just randomly
ignore them when they request it.
On 06/05, Conor Dooley wrote:
> On Tue, Jun 04, 2024 at 07:43:53PM -0300, Marcelo Schmitt wrote:
> > Add device tree documentation for AD4000 series of ADC devices.
> >
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
> > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
> >
> > Suggested-by: David Lechner <[email protected]>
>
> A suggested-by on a binding? That's unusual...
>
> > Signed-off-by: Marcelo Schmitt <[email protected]>
> > ---
> > Even though didn't pick all suggestions to the dt-bindings, I did pick most them
> > so kept David's Suggested-by tag.
> >
> > .../bindings/iio/adc/adi,ad4000.yaml | 207 ++++++++++++++++++
> > MAINTAINERS | 7 +
> > 2 files changed, 214 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> > new file mode 100644
> > index 000000000000..7470d386906b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4000.yaml
> > @@ -0,0 +1,207 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/adc/adi,ad4000.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices AD4000 and similar Analog to Digital Converters
> > +
> > +maintainers:
> > + - Marcelo Schmitt <[email protected]>
> > +
> > +description: |
> > + Analog Devices AD4000 family of Analog to Digital Converters with SPI support.
> > + Specifications can be found at:
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4000-4004-4008.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4001-4005.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4002-4006-4010.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4003-4007-4011.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4020-4021-4022.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4001.pdf
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/adaq4003.pdf
> > +
> > +$ref: /schemas/spi/spi-peripheral-props.yaml#
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - adi,ad4000
> > + - adi,ad4001
> > + - adi,ad4002
> > + - adi,ad4003
> > + - adi,ad4004
> > + - adi,ad4005
> > + - adi,ad4006
> > + - adi,ad4007
> > + - adi,ad4008
> > + - adi,ad4010
> > + - adi,ad4011
> > + - adi,ad4020
> > + - adi,ad4021
> > + - adi,ad4022
> > + - adi,adaq4001
> > + - adi,adaq4003
>
> Are all these actually incompatible? I'd like a note in the commit
> message as to why that's the case. A quick look at the driver showed
> that the differences in the driver between the ad402{0,1,2} are limited
> to the "dev_name". Same went for some other devices, like the
> ad40{02,06,10}.
Yes, that's correct. Some chips only vary by name and max sample rate which
boils down to only having a different dev_name in the driver.
Can those have grouped compatible strings?
dt_binding_check fails if curly brackets are used.
properties:
compatible:
enum:
- adi,ad402{0,1,2}
The groups of similar chips are:
AD4020/AD4021/AD4022
AD4003/AD4007/AD4011
AD4002/AD4006/AD4010
AD4001/AD4005
AD4000/AD4004/AD4008
Thanks,
Marcelo
>
> Thanks,
> Conor.
On Fri, Jun 07, 2024 at 11:35:44AM -0300, Marcelo Schmitt wrote:
> On 06/05, Conor Dooley wrote:
> > On Tue, Jun 04, 2024 at 07:43:53PM -0300, Marcelo Schmitt wrote:
> > > Add device tree documentation for AD4000 series of ADC devices.
> > > +properties:
> > > + compatible:
> > > + enum:
> > > + - adi,ad4000
> > > + - adi,ad4001
> > > + - adi,ad4002
> > > + - adi,ad4003
> > > + - adi,ad4004
> > > + - adi,ad4005
> > > + - adi,ad4006
> > > + - adi,ad4007
> > > + - adi,ad4008
> > > + - adi,ad4010
> > > + - adi,ad4011
> > > + - adi,ad4020
> > > + - adi,ad4021
> > > + - adi,ad4022
> > > + - adi,adaq4001
> > > + - adi,adaq4003
> >
> > Are all these actually incompatible? I'd like a note in the commit
> > message as to why that's the case. A quick look at the driver showed
> > that the differences in the driver between the ad402{0,1,2} are limited
> > to the "dev_name". Same went for some other devices, like the
> > ad40{02,06,10}.
>
> Yes, that's correct. Some chips only vary by name and max sample rate which
> boils down to only having a different dev_name in the driver.
> Can those have grouped compatible strings?
> dt_binding_check fails if curly brackets are used.
> properties:
> compatible:
> enum:
> - adi,ad402{0,1,2}
compatible:
oneOf:
- const: adi,ad4020
- items:
- enum:
- adi,ad4021
- adi,ad4022
- const: adi,ad4020
>
> The groups of similar chips are:
> AD4020/AD4021/AD4022
> AD4003/AD4007/AD4011
> AD4002/AD4006/AD4010
> AD4001/AD4005
> AD4000/AD4004/AD4008
On 06/07, Nuno S? wrote:
> On Thu, 2024-06-06 at 18:31 -0300, Marcelo Schmitt wrote:
>
> ...
>
> >
> >
> >
> > When is a driver version check needed?
> > Yes, older versions of SPI-Engine won't support this, but the patch set should
> > cause no regression. Even if loading the current ad4000 driver with
> > older SPI-Engine HDL and driver, the ADC driver would get a warn (or error?)
> > and do what's possible without MOSI idle feature (probably only be able to do
> > reg access) or fail probing.
> >
>
> Maybe I'm missing something but with the patchset we unconditionally set
> SPI_MOSI_IDLE_HIGH. So if we load an hdl which does not support it things will
> apparently be ok but it won't actually work, right? If I'm right we should have
Yes, that's correct.
> a bit in a RO config_register telling us that the feature is being supported or
> not. That way we only set the mode bit if we do support it...
Ok, understood. Will do it for v4.
Thanks,
Marcelo
>
> - Nuno S?
>
>
On 06/07, Mark Brown wrote:
> On Thu, Jun 06, 2024 at 04:57:52PM -0300, Marcelo Schmitt wrote:
>
> > As far as I searched, the definitions for SPI protocol usually don't specify any
> > behavior for the MOSI line when the controller is not clocking out data.
> > So, I think SPI controllers that are not capable of implementing any type
> > of MOSI idle configuration are anyway compliant to what is usual SPI.
> > For those that can implement such feature, I thought peripherals could request
> > it by setting SPI mode bits.
>
> The issue here is the one that Richard highlighted with it not being
> clear exactly what the intended behaviour is.
>
> > But yeah, it's not that evident what this patch set is all about and why this is
> > wanted so I made a wiki page to explain the reasoning for this set.
> > https://wiki.analog.com/software/linux/docs/spi/spi_copi_idle?rev=1717699755
> > Hopefully the figures with timing diagrams and transfer captures there will
> > provide quicker understanding of this rather than I try to explain it with
> > only text.
>
> It needs to be apparent to someone looking at the kernel what the code
> is intended to do.
Ack
>
> > If you still think we need feature detection for MOSI idle capability just let
> > me know, I'll implement what be needed.
>
> If the devices actually require this mode then we can't just randomly
> ignore them when they request it.
Ok. Yes, when connected in that datasheet "3-wire" mode the MOSI idle high
feature is pretty much required otherwise users won't be able to sample the ADC.
Will document the behavior for the MOSI idle feature and make spi_setup() fail
with better message if the controller can't support a device requesting it.
Thanks,
Marcelo
On Fri, 7 Jun 2024 11:40:23 -0300
Marcelo Schmitt <[email protected]> wrote:
> On 06/07, Nuno Sá wrote:
> > On Thu, 2024-06-06 at 18:31 -0300, Marcelo Schmitt wrote:
> >
> > ...
> >
> > >
> > >
> > >
> > > When is a driver version check needed?
> > > Yes, older versions of SPI-Engine won't support this, but the patch set should
> > > cause no regression. Even if loading the current ad4000 driver with
> > > older SPI-Engine HDL and driver, the ADC driver would get a warn (or error?)
> > > and do what's possible without MOSI idle feature (probably only be able to do
> > > reg access) or fail probing.
> > >
> >
> > Maybe I'm missing something but with the patchset we unconditionally set
> > SPI_MOSI_IDLE_HIGH. So if we load an hdl which does not support it things will
> > apparently be ok but it won't actually work, right? If I'm right we should have
> Yes, that's correct.
>
> > a bit in a RO config_register telling us that the feature is being supported or
> > not. That way we only set the mode bit if we do support it...
>
> Ok, understood. Will do it for v4.
If you don't have such a mode bit, you will need to add a property to the
dt-binding. Or a suitable compatible.
Nasty, so fingers crossed you do have a capability flag to check!
Jonathan
>
> Thanks,
> Marcelo
>
> >
> > - Nuno Sá
> >
> >
> > +
> > +static int ad4000_convert_and_acquire(struct ad4000_state *st)
> > +{
> > + int ret;
> > +
> > + /*
> > + * In 4-wire mode, the CNV line is held high for the entire
> > conversion
> > + * and acquisition process. In other modes st->cnv_gpio is NULL and
> > is
> > + * ignored (CS is wired to CNV in those cases).
> > + */
> > + gpiod_set_value_cansleep(st->cnv_gpio, 1);
>
> Not sure it's a good practise to assume internal details as you're going for
> GPIO. I would prefer to have an explicit check for st->cnv_gpio being NULL or
> not.
Hmm. I had it in my head that this was documented behaviour, but
I can't find such in the docs, so agreed checking it makes sense.
I would be very surprised if this ever changed as it's one of the
things that makes optional gpios easy to work with but who knows!
+CC Linus and Bartosz for feedback on this one.
>
> > + ret = spi_sync(st->spi, &st->msg);
> > + gpiod_set_value_cansleep(st->cnv_gpio, 0);
> > +
> > + return ret;
> > +}
> > +
> > +static int ad4000_config(struct ad4000_state *st)
> > +{
> > + unsigned int reg_val;
> > +
> > + if (device_property_present(&st->spi->dev, "adi,high-z-input"))
> > + reg_val |= FIELD_PREP(AD4000_CFG_HIGHZ, 1);
> > +
> > + /*
> > + * The ADC SDI pin might be connected to controller CS line in which
> > + * case the write might fail. This, however, does not prevent the
> > device
> > + * from functioning even though in a configuration other than the
> > + * requested one.
> > + */
>
> This raises the question if there's any way to describe that through DT (if not
> doing it already)? So that, if SDI is connected to CS we don't even call this?
> Other question that comes to mind is that in case SDI is connected to CS, will
> all writes fail? Because if that's the case we other writes (like scale) that
> won't work and we should take care of that...
Definitely needs describing and all configuration sysfs etc needs to be read only
if we can't control it.
>
> > + return ad4000_write_reg(st, reg_val);
> > +}
> > +
Jonathan
Sun, Jun 09, 2024 at 10:23:54AM +0100, Jonathan Cameron kirjoitti:
...
> > > + /*
> > > + * In 4-wire mode, the CNV line is held high for the entire
> > > conversion
> > > + * and acquisition process. In other modes st->cnv_gpio is NULL and
> > > is
> > > + * ignored (CS is wired to CNV in those cases).
> > > + */
> > > + gpiod_set_value_cansleep(st->cnv_gpio, 1);
> >
> > Not sure it's a good practise to assume internal details as you're going for
> > GPIO. I would prefer to have an explicit check for st->cnv_gpio being NULL or
> > not.
>
> Hmm. I had it in my head that this was documented behaviour, but
> I can't find such in the docs, so agreed checking it makes sense.
>
> I would be very surprised if this ever changed as it's one of the
> things that makes optional gpios easy to work with but who knows!
Not Linus and not Bart, but we have tons of drivers that call GPIO APIs
unconditionally as long as they want optional GPIO.
What I see here is the comment that should be rewritten to say something like
"if GPIO is defined blablabla, otherwise blablabla."
I.o.w. do not mention that implementation detail (being NULL, i.e. optional).
--
With Best Regards,
Andy Shevchenko