From: Marcelo Schmitt <marcelo.schmitt1@gmail.com>
To: Jonathan Cameron <jic23@kernel.org>
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>,
David Lechner <dlechner@baylibre.com>,
Marcelo Schmitt <marcelo.schmitt@analog.com>,
lars@metafoo.de, Michael.Hennerich@analog.com,
robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
conor+dt@kernel.org, linux-iio@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] dt-bindings: iio: adc: Add AD4000
Date: Sun, 21 Apr 2024 19:38:38 -0300 [thread overview]
Message-ID: <ZiWVboPqrzyMJB9n@debian-BULLSEYE-live-builder-AMD64> (raw)
In-Reply-To: <20240420151727.767b2861@jic23-huawei>
On 04/20, Jonathan Cameron wrote:
> On Tue, 16 Apr 2024 18:46:11 -0300
> Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:
>
> > So, I have been trying to make this work, though I haven't been successful so
> > far, and I don't really think using pinctrl is a good solution for this either.
> >
> > Comments inline.
> >
> > On 04/14, Jonathan Cameron wrote:
> > > On Sat, 13 Apr 2024 12:33:54 -0500
> > > David Lechner <dlechner@baylibre.com> wrote:
> > >
> > > > On 4/13/24 11:14 AM, Jonathan Cameron wrote:
> > > > > On Tue, 9 Apr 2024 12:30:09 -0300
> > > > > Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:
> > > > >
> > > > >> On 04/08, David Lechner wrote:
> > > > >>> On Mon, Apr 8, 2024 at 9:32 AM Marcelo Schmitt
> > > > >>> <marcelo.schmitt@analog.com> wrote:
> > > > >>>>
> > > >
> > > > ...
> > > >
> >
> > ...
> >
...
> >
> > The pinctrl configuration for this ADC would not be meant to change once after
> > boot as it looks to be the usual use case for pinctrl (including mediatek-bluetooth.txt).
> >
> > Also, no suitable mux for the "3-wire" mode in
> > Documentation/devicetree/bindings/pinctrl/xlnx,pinctrl-zynq.yaml
> > to do it like Documentation/devicetree/bindings/net/mediatek-bluetooth.txt.
> > The zynq pinctrl driver (drivers/pinctrl/pinctrl-zynq.c) would have to be
> > updated to add the new mux function in
> > static const struct zynq_pinmux_function zynq_pmux_functions[] = {
> > DEFINE_ZYNQ_PINMUX_FUNCTION(ethernet0, 1),
> > ...
> > DEFINE_ZYNQ_PINMUX_FUNCTION(axi_spi_single, 1),
> > DEFINE_ZYNQ_PINMUX_FUNCTION(axi_spi_multi, 2),
> > though this is not really a thing that's on zynq, but one that is related to
> > these ADCs so I'm not sure it should go there.
>
>
> I'd argue we are after a specific SPI controller setup for this.
> A controller driver would need modifying to make it work.
Ack, makes sense to me.
>
> >
> > > > For example, if we wanted to use 3-wire mode for reading
> > > > data, we would set the pinctrl to "default" to write the
> > > > register to configure the chip during driver probe. Then
> > > > to read data, we would change the pinctrl to "single" before
> > > > doing the SPI xfer to ensure that the ADC SDI pin is pulled
> > > > high independent of what the SDO line of the SPI controller
> > > > is currently doing.
> >
> > No, the pin configuration for this ADCs would be expected to change unrestricted
> > amount of times. The pin configuration would have to change every time a sample
> > read is made after a register access transfers and vice-versa. If we decide
> > to support span compression, every change to _scale would lead to pinctrl
> > configuration change.
> >
> > At pin level, we would want to rise SPI controller SDO line to VIO but then
> > the new SDO pin config would conflict with SPI pin group config.
> >
> > I included pinctrl properties in my test dts to make use of pinctrl framework.
> > Yet, that doesn't really alternate SPI line configuration we are using because
> > the SPI function that is in the PS (FPGA's Processing System) is not what we are
> > interfacing when using spi-engine. Copy of my test dts at end of email.
> >
> > Currently, the SPI controller we are using to test with these exotic ADCs
> > is the spi-engine which is implemented in the FPGA as an IP block which
> > owns control of the bus lines (SPI, SDO, CS, ...). To alternate the
> > configuration of SPI lines (pull SDO (ADC SDI) up to VIO, connect controller CS
> > to ADC SDI, etc.) I think it should be done in the HDL project. I don't think
> > it's a good idea to hijack spi-engine lines through pinctrl.
>
> Such functionality would need to be pushed to the spi controller driver
> which could know if there was any need to do anything like this, or if there
> was simply a register to set.
>
Ack.
> >
> > >
> > > Ah ok. This is implying that we are switching to a controllable
> > > mode to pull that pin high (or I guess one where it is always
> > > high). I'm not sure if that's more common than an SPI controller
> > > where you can set the 'don't' care state to high or low.
> > > I assume we can't get away with just setting the output buffer
> > > to all 1s because it won't hold that state between transfers?
> >
> > I tried sending the tx buffer filled with 1s when testing this with a RPi4 but
> > it brought the controller SDO (ADC SDI) line low between each 8 bits of transfer
> > (attaching an image (yellow line is SCLK, green lines is controller SDO)).
>
> Pity - thought that was overly optimistic.
>
> > Anyway, can we have any guaranties with respect to controller SDO line behaviour
> > when its not clocking out data?
>
>
> >
> > >
> > > Feels like that could be rolled into the SPI subsystem with
> > > any necessary transitions handled there (maybe)
> >
> > To me, this sounds more reasonable than pinctrl.
> > Yeah, if we can set a don't' care state for the SDO line then that should be
> > enough for these ADCs.
> > Otherwise, can we really do anything if the controller can't keep SDO high?
>
> There is one similar (ish) entry already.
> https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/spi/spi.h#L29
> #define SPI_3WIRE_HIZ _BITUL(15) /* high impedance turnaround */
> in that it is controlling state in what I think would normally be a don't care state.
>
> I think we could have an
> SPI_SDO_DONT_CARE_HIGH (naming to be improved upon ;)
> that a driver could advertise support for and the spi device could request.
>
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/panel/panel-tpo-tpg110.c#L429
>
> Implement that in the spi-gpio driver as a PoC probably and in your SPI copntoller
> driver.
>
> Ultimately if the controller really isn't capable (including dances through pin
> mode changes if necessary) then the ADC won't work in this wiring with that host
> controller.
>
> I'd propose something along these lines and see whether Mark + any other active
> SPI folk think it is reasonable or not?
Sounds promising. Will try implement something like that.
>
> >
> > >
> > > Just to check, this isn't just a case of a missing pull up
> > > resistor to drag that line correctly when it isn't actively
> > > driven (combined with all 1s in the write buffer) etc?
> >
> > When using spi-engine, the controller SDO is connected to ADC SDI, controller
> > CS to ADC CNV and, for reg access, it works as conventional SPI.
> > spi-engine leaves the SDO line the state it was after last transfer so it we
> > can make it idle high during sample read. No hardware pull-up needed.
>
> Fair enough. No multi master support I guess (that is a bit obscure for
> SPI). A little ugly that it's dependent on the last access - so you would need
> to do a dummy access if the normal last bit was wrong level?
Seems I've been lucky with it but yes, we would need a dummy transfer to put
controller SDO line in the desired state. I'm thinking it should not be
difficult to make spi-engine SDO line always idle high (or idle in a
pre-configured state). I'll talk with the guys in the HDL team and what can be
done about it.
Thanks,
Marcelo
>
> Jonathan
>
> >
> > Marcelo
> >
> > >
> > > Jonathan
> > >
> > >
> >
> > The device tree source file I was using for testing with the ADC with the
> > changes for using pinctrl. Didn't really work.
> >
> > // SPDX-License-Identifier: GPL-2.0
> > /*
> > * Analog Devices ADAQ4003
> > * https://wiki.analog.com/resources/tools-software/linux-drivers/iio-adc/ad400x
> > * https://wiki.analog.com/resources/eval/user-guides/ad400x
> > *
> > * hdl_project: <pulsar_adc_pmdz/zed>
> > * board_revision: <>
> > *
> > * Copyright (C) 2016-2023 Analog Devices Inc.
> > */
> > /dts-v1/;
> >
> > #include "zynq-zed.dtsi"
> > #include "zynq-zed-adv7511.dtsi"
> > #include <dt-bindings/pinctrl/pinctrl-zynq.h>
> >
> > / {
> > adc_vref: regulator-vref {
> > compatible = "regulator-fixed";
> > regulator-name = "EVAL 5V Vref";
> > regulator-min-microvolt = <5000000>;
> > regulator-max-microvolt = <5000000>;
> > regulator-always-on;
> > };
> >
> > adc_vdd: regulator-vdd {
> > compatible = "regulator-fixed";
> > regulator-name = "Eval VDD supply";
> > regulator-min-microvolt = <1800000>;
> > regulator-max-microvolt = <1800000>;
> > regulator-always-on;
> > };
> >
> > adc_vio: regulator-vio {
> > compatible = "regulator-fixed";
> > regulator-name = "Eval VIO supply";
> > regulator-min-microvolt = <3300000>;
> > regulator-max-microvolt = <3300000>;
> > regulator-always-on;
> > };
> > };
> >
> > &pinctrl0 {
> > /* Restore conventional SPI pin configuration */
> > pinctrl_spi_default: default_config {
> > mux {
> > /* Are these the ones used by spi-engine? */
> > groups = "spi0_0_grp";
> > function = "spi0";
> > };
> > conf {
> > groups = "spi0_0_grp";
> > power-source = <IO_STANDARD_LVCMOS33>;
> > };
> > conf-spi-sdo {
> > pins = "MIO17"; /* SPI0 SDO? */
> > bias-disable;
> > };
> > };
> >
> > /* Pull-up SPI SDO (ADC SDI) to VIO */
> > pinctrl_spi_single: single_config {
> > conf-spi-sdo {
> > pins = "MIO17"; /* Conflicts with SPI0 pin group */
> > bias-pull-up;
> > };
> > };
> > };
> >
> > &fpga_axi {
> > rx_dma: rx-dmac@44a30000 {
> > compatible = "adi,axi-dmac-1.00.a";
> > reg = <0x44a30000 0x1000>;
> > #dma-cells = <1>;
> > interrupts = <0 57 IRQ_TYPE_LEVEL_HIGH>;
> > clocks = <&clkc 17>;
> >
> > adi,channels {
> > #size-cells = <0>;
> > #address-cells = <1>;
> >
> > dma-channel@0 {
> > reg = <0>;
> > adi,source-bus-width = <32>;
> > adi,source-bus-type = <1>;
> > adi,destination-bus-width = <64>;
> > adi,destination-bus-type = <0>;
> > };
> > };
> > };
> >
> > axi_pwm_gen: axi-pwm-gen@ {
> > compatible = "adi,axi-pwmgen";
> > reg = <0x44b00000 0x1000>;
> > label = "cnv";
> > #pwm-cells = <2>;
> > clocks = <&spi_clk>;
> > };
> >
> > spi_clk: axi-clkgen@44a70000 {
> > compatible = "adi,axi-clkgen-2.00.a";
> > reg = <0x44a70000 0x10000>;
> > #clock-cells = <0>;
> > clocks = <&clkc 15>, <&clkc 15>;
> > clock-names = "s_axi_aclk", "clkin1";
> > clock-output-names = "spi_clk";
> > };
> >
> > axi_spi_engine_0: spi@44a00000 {
> > compatible = "adi,axi-spi-engine-1.00.a";
> > reg = <0x44a00000 0x1000>;
> > interrupt-parent = <&intc>;
> > interrupts = <0 56 IRQ_TYPE_LEVEL_HIGH>;
> > clocks = <&clkc 15 &spi_clk>;
> > clock-names = "s_axi_aclk", "spi_clk";
> > num-cs = <1>;
> >
> > #address-cells = <0x1>;
> > #size-cells = <0x0>;
> >
> > adaq4003: adc@0 {
> > compatible = "adi,adaq4003";
> > reg = <0>;
> > spi-max-frequency = <102000000>;
> > spi-cpha;
> > pinctrl-names = "default", "single";
> > pinctrl-0 = <&pinctrl_spi_default>;
> > pinctrl-1 = <&pinctrl_spi_single>;
> > vdd-supply = <&adc_vdd>;
> > vio-supply = <&adc_vio>;
> > ref-supply = <&adc_vref>;
> > adi,high-z-input;
> > adi,gain-milli = <454>;
> > };
> > };
> > };
>
next prev parent reply other threads:[~2024-04-21 22:37 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-08 14:30 [PATCH v2 0/2] Add support for AD4000 series Marcelo Schmitt
2024-04-08 14:31 ` [PATCH v2 1/2] dt-bindings: iio: adc: Add AD4000 Marcelo Schmitt
2024-04-09 2:57 ` David Lechner
2024-04-09 15:30 ` Marcelo Schmitt
2024-04-13 16:14 ` Jonathan Cameron
2024-04-13 17:33 ` David Lechner
2024-04-14 18:09 ` Jonathan Cameron
2024-04-16 21:46 ` Marcelo Schmitt
2024-04-20 14:17 ` Jonathan Cameron
2024-04-21 22:38 ` Marcelo Schmitt [this message]
2024-04-08 14:31 ` [PATCH v2 2/2] iio: adc: Add support for AD4000 Marcelo Schmitt
2024-04-09 3:05 ` David Lechner
2024-04-09 16:09 ` Marcelo Schmitt
2024-04-09 16:44 ` David Lechner
2024-04-09 18:12 ` Marcelo Schmitt
2024-04-13 16:34 ` Jonathan Cameron
2024-04-13 16:19 ` Jonathan Cameron
2024-04-13 16:39 ` Jonathan Cameron
2024-04-09 2:54 ` [PATCH v2 0/2] Add support for AD4000 series David Lechner
2024-04-09 14:59 ` Marcelo Schmitt
2024-04-09 16:31 ` David Lechner
2024-04-13 16:07 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZiWVboPqrzyMJB9n@debian-BULLSEYE-live-builder-AMD64 \
--to=marcelo.schmitt1@gmail.com \
--cc=Jonathan.Cameron@huawei.com \
--cc=Michael.Hennerich@analog.com \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=dlechner@baylibre.com \
--cc=jic23@kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=marcelo.schmitt@analog.com \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.