| Message ID | 20250422-iio-driver-ad4052-v2-3-638af47e9eb3@analog.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Add support for AD4052 device family | expand |
On Tue, 22 Apr 2025 13:34:48 +0200, Jorge Marques wrote: > Add dt-bindings for AD4052 family, devices AD4050/AD4052/AD4056/AD4058, > low-power with monitor capabilities SAR ADCs. > Each variant of the family differs in speed and resolution, resulting > in different scan types and spi word sizes, that are matched by the > compatible with the chip_info. > The device contains one input (cnv) and two outputs (gp0, gp1). > > Signed-off-by: Jorge Marques <jorge.marques@analog.com> > --- > .../devicetree/bindings/iio/adc/adi,ad4052.yaml | 98 ++++++++++++++++++++++ > MAINTAINERS | 6 ++ > 2 files changed, 104 insertions(+) > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
On 4/22/25 6:34 AM, Jorge Marques wrote: > Add dt-bindings for AD4052 family, devices AD4050/AD4052/AD4056/AD4058, > low-power with monitor capabilities SAR ADCs. > Each variant of the family differs in speed and resolution, resulting > in different scan types and spi word sizes, that are matched by the > compatible with the chip_info. > The device contains one input (cnv) and two outputs (gp0, gp1). Don't need line breaks after every period. > > Signed-off-by: Jorge Marques <jorge.marques@analog.com> > --- ... > + interrupts: > + items: > + - description: Signal coming from the GP0 pin (threshold). > + - description: Signal coming from the GP1 pin (data ready). > + > + interrupt-names: > + items: > + - const: gp0 > + - const: gp1 > + > + gpio-controller: true > + > + "#gpio-cells": > + const: 2 > + description: | > + The first cell is the GPn number: 0 to 1. > + The second cell takes standard GPIO flags. > + > + cnv-gpios: > + description: The Convert Input (CNV). If omitted, CNV is tied to SPI CS. > + maxItems: 1 > + Assuming the diagram at [1] is correct, for SPI offload use, we are missing: #trigger-source-cells: const: 2 description: | Output pins used as trigger source. Cell 0 defines which pin: * 0 = GP0 * 1 = GP1 Cell 1 defines the event: * 0 = Data ready * 1 = Min threshold * 2 = Max threshold * 3 = Either threshold * 4 = Device ready * 5 = Device enable * 6 = Chop control Bonus points for adding a header with macros for the arbitrary event values. And we are missing: pwms: maxItems: 1 description: PWM connected to the CNV pin. [1]: https://analogdevicesinc.github.io/hdl/projects/ad4052_ardz/index.html > + spi-max-frequency: > + maximum: 62500000 Datasheet Table 5. SPI Timing—ADC Modes, VIO ≥ 3.0 V says period can be 12 ns. So that would make max frequency 83333333. ... > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + > + spi { > + #address-cells = <1>; > + #size-cells = <0>; > + > + adc@0 { > + compatible = "adi,ad4052"; > + reg = <0>; > + vdd-supply = <&adc_vdd>; > + vio-supply = <&adc_vio>; > + spi-max-frequency = <25000000>; > + > + interrupt-parent = <&gpio>; > + interrupts = <0 0 IRQ_TYPE_EDGE_RISING>, > + <0 1 IRQ_TYPE_EDGE_FALLING>; > + interrupt-names = "gp0", "gp1"; > + cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>; > + }; > + }; Could be nice to have a 2nd example showing SPI offload usage.
Hi David, I didn't went through your's and Jonathan's ad4052.c review yet, but for the trigger-source-cells I need to dig deeper and make considerable changes to the driver, as well as hardware tests. My idea was to have a less customizable driver, but I get that it is more interesting to make it user-definable. On Fri, Apr 25, 2025 at 05:50:58PM -0500, David Lechner wrote: > On 4/22/25 6:34 AM, Jorge Marques wrote: > > Add dt-bindings for AD4052 family, devices AD4050/AD4052/AD4056/AD4058, > > low-power with monitor capabilities SAR ADCs. > > Each variant of the family differs in speed and resolution, resulting > > in different scan types and spi word sizes, that are matched by the > > compatible with the chip_info. > > The device contains one input (cnv) and two outputs (gp0, gp1). > > Don't need line breaks after every period. Ack. > > > > > Signed-off-by: Jorge Marques <jorge.marques@analog.com> > > --- > > ... > > > + interrupts: > > + items: > > + - description: Signal coming from the GP0 pin (threshold). > > + - description: Signal coming from the GP1 pin (data ready). > > + > > + interrupt-names: > > + items: > > + - const: gp0 > > + - const: gp1 > > + > > + gpio-controller: true > > + > > + "#gpio-cells": > > + const: 2 > > + description: | > > + The first cell is the GPn number: 0 to 1. > > + The second cell takes standard GPIO flags. > > + > > + cnv-gpios: > > + description: The Convert Input (CNV). If omitted, CNV is tied to SPI CS. > > + maxItems: 1 > > + > > Assuming the diagram at [1] is correct, for SPI offload use, we are missing: > > #trigger-source-cells: > const: 2 > description: | > Output pins used as trigger source. > > Cell 0 defines which pin: > * 0 = GP0 > * 1 = GP1 > > Cell 1 defines the event: > * 0 = Data ready > * 1 = Min threshold > * 2 = Max threshold > * 3 = Either threshold > * 4 = Device ready > * 5 = Device enable > * 6 = Chop control > > Bonus points for adding a header with macros for the arbitrary event values. In the sense of describing the device and not what the driver does, I believe the proper mapping would be: Cell 1 defines the event: * 0 = Disabled * 1 = Data ready * 2 = Min threshold * 3 = Max threshold * 4 = Either threshold * 5 = CHOP control * 6 = Device enable * 7 = Device ready (only GP1) I will investigate further this. > > And we are missing: > > pwms: > maxItems: 1 > description: PWM connected to the CNV pin. > > [1]: https://analogdevicesinc.github.io/hdl/projects/ad4052_ardz/index.html > > > + spi-max-frequency: > > + maximum: 62500000 > > Datasheet Table 5. SPI Timing—ADC Modes, VIO ≥ 3.0 V says period can be 12 ns. > > So that would make max frequency 83333333. Ack. > > ... > > > +examples: > > + - | > > + #include <dt-bindings/gpio/gpio.h> > > + #include <dt-bindings/interrupt-controller/irq.h> > > + > > + spi { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + adc@0 { > > + compatible = "adi,ad4052"; > > + reg = <0>; > > + vdd-supply = <&adc_vdd>; > > + vio-supply = <&adc_vio>; > > + spi-max-frequency = <25000000>; > > + > > + interrupt-parent = <&gpio>; > > + interrupts = <0 0 IRQ_TYPE_EDGE_RISING>, > > + <0 1 IRQ_TYPE_EDGE_FALLING>; > > + interrupt-names = "gp0", "gp1"; > > + cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>; > > + }; > > + }; > > Could be nice to have a 2nd example showing SPI offload usage. > . Will add. Regards, Jorge
On 4/29/25 8:48 AM, Jorge Marques wrote: > Hi David, > > I didn't went through your's and Jonathan's ad4052.c review yet, > but for the trigger-source-cells I need to dig deeper and make > considerable changes to the driver, as well as hardware tests. > My idea was to have a less customizable driver, but I get that it is > more interesting to make it user-definable. We don't need to make the driver support all possibilities, but the devicetree needs to be as complete as possible since it can't be as easily changed in the future. ... >> >> Assuming the diagram at [1] is correct, for SPI offload use, we are missing: >> >> #trigger-source-cells: >> const: 2 >> description: | >> Output pins used as trigger source. >> >> Cell 0 defines which pin: >> * 0 = GP0 >> * 1 = GP1 >> >> Cell 1 defines the event: >> * 0 = Data ready >> * 1 = Min threshold >> * 2 = Max threshold >> * 3 = Either threshold >> * 4 = Device ready >> * 5 = Device enable >> * 6 = Chop control >> >> Bonus points for adding a header with macros for the arbitrary event values. > > In the sense of describing the device and not what the driver does, I > believe the proper mapping would be: > > Cell 1 defines the event: > * 0 = Disabled > * 1 = Data ready > * 2 = Min threshold > * 3 = Max threshold > * 4 = Either threshold > * 5 = CHOP control > * 6 = Device enable > * 7 = Device ready (only GP1) > > I will investigate further this. > >> 0 = Disabled doesn't make sense to me. One would just not wire up a trigger-source in that case.
On Tue, Apr 29, 2025 at 10:45:20AM -0500, David Lechner wrote: > On 4/29/25 8:48 AM, Jorge Marques wrote: > > Hi David, > > > > I didn't went through your's and Jonathan's ad4052.c review yet, > > but for the trigger-source-cells I need to dig deeper and make > > considerable changes to the driver, as well as hardware tests. > > My idea was to have a less customizable driver, but I get that it is > > more interesting to make it user-definable. > > We don't need to make the driver support all possibilities, but the devicetree > needs to be as complete as possible since it can't be as easily changed in the > future. > Ack. I see that the node goes in the spi controller (the parent). To use the same information in the driver I need to look-up the parent node, then the node. I don't plan to do that in the version of the driver, just an observation. There is something else I want to discuss on the dt-bindings actually. According to the schema, the spi-max-frequency is: > Maximum SPI clocking speed of the device in Hz. The ad4052 has 2 maximum speeds: Configuration mode (lower) and ADC Mode (higher, depends on VIO). The solution I came up, to not require a custom regmap spi bus, is to have spi-max-frequency bound the Configuration mode speed, and have ADC Mode set by VIO regulator voltage, through spi_transfer.speed_hz. At the end of the day, both are bounded by the spi controller maximum speed. My concern is that having ADC mode speed higher than spi-max-frequency may be counter-intuitive, still, it allows to achieve the max data sheet speed considering VIO voltage with the lowest code boilerplate. Let me know if I can proceed this way before submitting V3. > ... > > >> > >> Assuming the diagram at [1] is correct, for SPI offload use, we are missing: > >> > >> #trigger-source-cells: > >> const: 2 > >> description: | > >> Output pins used as trigger source. > >> > >> Cell 0 defines which pin: > >> * 0 = GP0 > >> * 1 = GP1 > >> > >> Cell 1 defines the event: > >> * 0 = Data ready > >> * 1 = Min threshold > >> * 2 = Max threshold > >> * 3 = Either threshold > >> * 4 = Device ready > >> * 5 = Device enable > >> * 6 = Chop control > >> > >> Bonus points for adding a header with macros for the arbitrary event values. > > > > In the sense of describing the device and not what the driver does, I > > believe the proper mapping would be: > > > > Cell 1 defines the event: > > * 0 = Disabled > > * 1 = Data ready > > * 2 = Min threshold > > * 3 = Max threshold > > * 4 = Either threshold > > * 5 = CHOP control > > * 6 = Device enable > > * 7 = Device ready (only GP1) > > > > I will investigate further this. > > > >> > > 0 = Disabled doesn't make sense to me. One would just not wire up a > trigger-source in that case. Ack. Regards, Jorge
On 6/2/25 4:17 AM, Jorge Marques wrote: > On Tue, Apr 29, 2025 at 10:45:20AM -0500, David Lechner wrote: >> On 4/29/25 8:48 AM, Jorge Marques wrote: >>> Hi David, >>> >>> I didn't went through your's and Jonathan's ad4052.c review yet, >>> but for the trigger-source-cells I need to dig deeper and make >>> considerable changes to the driver, as well as hardware tests. >>> My idea was to have a less customizable driver, but I get that it is >>> more interesting to make it user-definable. >> >> We don't need to make the driver support all possibilities, but the devicetree >> needs to be as complete as possible since it can't be as easily changed in the >> future. >> > > Ack. > > I see that the node goes in the spi controller (the parent). To use the > same information in the driver I need to look-up the parent node, then > the node. I don't plan to do that in the version of the driver, just an > observation. > > There is something else I want to discuss on the dt-bindings actually. > According to the schema, the spi-max-frequency is: > > > Maximum SPI clocking speed of the device in Hz. > > The ad4052 has 2 maximum speeds: Configuration mode (lower) and ADC Mode > (higher, depends on VIO). The solution I came up, to not require a > custom regmap spi bus, is to have spi-max-frequency bound the > Configuration mode speed, The purpose of spi-max-frequency in the devicetree is that sometimes the wiring of a complete system makes the effective max frequency lower than what is allowed by the datasheet. So this really needs to be the absolute highest frequency allowed. > and have ADC Mode set by VIO regulator > voltage, through spi_transfer.speed_hz. At the end of the day, both are > bounded by the spi controller maximum speed. If spi_transfer.speed_hz > spi-max-frequency, then the core SPI code uses spi-max-frequency. So I don't think this would actually work. > > My concern is that having ADC mode speed higher than spi-max-frequency > may be counter-intuitive, still, it allows to achieve the max data sheet > speed considering VIO voltage with the lowest code boilerplate. > > Let me know if I can proceed this way before submitting V3.
Hi David, On Mon, Jun 02, 2025 at 10:17:18AM -0500, David Lechner wrote: > On 6/2/25 4:17 AM, Jorge Marques wrote: > > On Tue, Apr 29, 2025 at 10:45:20AM -0500, David Lechner wrote: > >> On 4/29/25 8:48 AM, Jorge Marques wrote: > >>> Hi David, > >>> > >>> I didn't went through your's and Jonathan's ad4052.c review yet, > >>> but for the trigger-source-cells I need to dig deeper and make > >>> considerable changes to the driver, as well as hardware tests. > >>> My idea was to have a less customizable driver, but I get that it is > >>> more interesting to make it user-definable. > >> > >> We don't need to make the driver support all possibilities, but the devicetree > >> needs to be as complete as possible since it can't be as easily changed in the > >> future. > >> > > > > Ack. > > > > I see that the node goes in the spi controller (the parent). To use the > > same information in the driver I need to look-up the parent node, then > > the node. I don't plan to do that in the version of the driver, just an > > observation. > > > > There is something else I want to discuss on the dt-bindings actually. > > According to the schema, the spi-max-frequency is: > > > > > Maximum SPI clocking speed of the device in Hz. > > > > The ad4052 has 2 maximum speeds: Configuration mode (lower) and ADC Mode > > (higher, depends on VIO). The solution I came up, to not require a > > custom regmap spi bus, is to have spi-max-frequency bound the > > Configuration mode speed, > > The purpose of spi-max-frequency in the devicetree is that sometimes > the wiring of a complete system makes the effective max frequency > lower than what is allowed by the datasheet. So this really needs > to be the absolute highest frequency allowed. > > > and have ADC Mode set by VIO regulator > > voltage, through spi_transfer.speed_hz. At the end of the day, both are > > bounded by the spi controller maximum speed. > > If spi_transfer.speed_hz > spi-max-frequency, then the core SPI code > uses spi-max-frequency. So I don't think this would actually work. > Ok, so that's something that may be worth some attention. At spi/spi.c#2472 if (!of_property_read_u32(nc, "spi-max-frequency", &value)) spi->max_speed_hz = value; At spi/spi.c#4090 if (!xfer->speed_hz) xfer->speed_hz = spi->max_speed_hz; So, speed_hz is max-spi-frequency only if xfer->speed_hz is 0 and not bounded by it. Then at spi-axi-spi-engine.c: static int spi_engine_precompile_message(struct spi_message *msg) { clk_div = DIV_ROUND_UP(max_hz, xfer->speed_hz); xfer->effective_speed_hz = max_hz / min(clk_div, 256U); } Where max_hz is set only by the IP spi_clk. If at the driver I set xfer.speed_hz, it won't be bounded by max-spi-frequency. The only that seems to bound as described is the layer for flash memory at spi-mem.c@spi_mem_adjust_op_freq. For the adc driver, I will then consider your behavioral description and create a custom regmap bus to limit set the reg access speed (fixed), and keep adc mode speed set by VIO. And consider spi-max-frequency can further reduce both speeds. (or should instead be handled at the driver like spi-mem.c ?) Thanks for the quick reply! Jorge
On 6/2/25 11:32 AM, Jorge Marques wrote: > Hi David, > > On Mon, Jun 02, 2025 at 10:17:18AM -0500, David Lechner wrote: >> On 6/2/25 4:17 AM, Jorge Marques wrote: >>> On Tue, Apr 29, 2025 at 10:45:20AM -0500, David Lechner wrote: >>>> On 4/29/25 8:48 AM, Jorge Marques wrote: >>>>> Hi David, >>>>> >>>>> I didn't went through your's and Jonathan's ad4052.c review yet, >>>>> but for the trigger-source-cells I need to dig deeper and make >>>>> considerable changes to the driver, as well as hardware tests. >>>>> My idea was to have a less customizable driver, but I get that it is >>>>> more interesting to make it user-definable. >>>> >>>> We don't need to make the driver support all possibilities, but the devicetree >>>> needs to be as complete as possible since it can't be as easily changed in the >>>> future. >>>> >>> >>> Ack. >>> >>> I see that the node goes in the spi controller (the parent). To use the >>> same information in the driver I need to look-up the parent node, then >>> the node. I don't plan to do that in the version of the driver, just an >>> observation. >>> >>> There is something else I want to discuss on the dt-bindings actually. >>> According to the schema, the spi-max-frequency is: >>> >>> > Maximum SPI clocking speed of the device in Hz. >>> >>> The ad4052 has 2 maximum speeds: Configuration mode (lower) and ADC Mode >>> (higher, depends on VIO). The solution I came up, to not require a >>> custom regmap spi bus, is to have spi-max-frequency bound the >>> Configuration mode speed, >> >> The purpose of spi-max-frequency in the devicetree is that sometimes >> the wiring of a complete system makes the effective max frequency >> lower than what is allowed by the datasheet. So this really needs >> to be the absolute highest frequency allowed. >> >>> and have ADC Mode set by VIO regulator >>> voltage, through spi_transfer.speed_hz. At the end of the day, both are >>> bounded by the spi controller maximum speed. >> >> If spi_transfer.speed_hz > spi-max-frequency, then the core SPI code >> uses spi-max-frequency. So I don't think this would actually work. >> > Ok, so that's something that may be worth some attention. > > At spi/spi.c#2472 > if (!of_property_read_u32(nc, "spi-max-frequency", &value)) > spi->max_speed_hz = value; > > At spi/spi.c#4090 > if (!xfer->speed_hz) > xfer->speed_hz = spi->max_speed_hz; > > So, speed_hz is max-spi-frequency only if xfer->speed_hz is 0 and > not bounded by it. Ah, OK, my memory was wrong. It is only bound by the controller max speed, not the device max speed. if (ctlr->max_speed_hz && xfer->speed_hz > ctlr->max_speed_hz) xfer->speed_hz = ctlr->max_speed_hz; It does seem odd that it would allow setting an individual xfer speed higher than than the given device max speed. I suppose we could submit a patch adding that check to the SPI core code and see what Mark has to say. > > Then at spi-axi-spi-engine.c: > > static int spi_engine_precompile_message(struct spi_message *msg) > { > clk_div = DIV_ROUND_UP(max_hz, xfer->speed_hz); > xfer->effective_speed_hz = max_hz / min(clk_div, 256U); > } > > Where max_hz is set only by the IP spi_clk. If at the driver I set > xfer.speed_hz, it won't be bounded by max-spi-frequency. > > The only that seems to bound as described is the layer for flash memory > at spi-mem.c@spi_mem_adjust_op_freq. > > For the adc driver, I will then consider your behavioral description and > create a custom regmap bus to limit set the reg access speed (fixed), > and keep adc mode speed set by VIO. And consider spi-max-frequency can > further reduce both speeds. > (or should instead be handled at the driver like spi-mem.c ?) It would be more work, but if it is common enough, we could generalize this in the core code. For example add a spi-register-max-frequency binding (or even a more general spi-max-freqency-map to map operations to max frequencies). Then we could bake it into the regmap_spi code to handle this property and not have to make a separate bus. FWIW, there are also some SPI TFT displays that use a different frequency for register access compared to framebuffer data that could potentially use this too. Right now, these just have a hard-coded register access frequency of e.g. 10 MHz. > > Thanks for the quick reply! > Jorge
On Mon, Jun 02, 2025 at 12:23:40PM -0500, David Lechner wrote: > On 6/2/25 11:32 AM, Jorge Marques wrote: > > Hi David, > > > > On Mon, Jun 02, 2025 at 10:17:18AM -0500, David Lechner wrote: > >> On 6/2/25 4:17 AM, Jorge Marques wrote: > >>> On Tue, Apr 29, 2025 at 10:45:20AM -0500, David Lechner wrote: > >>>> On 4/29/25 8:48 AM, Jorge Marques wrote: > >>>>> Hi David, > >>>>> > >>>>> I didn't went through your's and Jonathan's ad4052.c review yet, > >>>>> but for the trigger-source-cells I need to dig deeper and make > >>>>> considerable changes to the driver, as well as hardware tests. > >>>>> My idea was to have a less customizable driver, but I get that it is > >>>>> more interesting to make it user-definable. > >>>> > >>>> We don't need to make the driver support all possibilities, but the devicetree > >>>> needs to be as complete as possible since it can't be as easily changed in the > >>>> future. > >>>> > >>> > >>> Ack. > >>> > >>> I see that the node goes in the spi controller (the parent). To use the > >>> same information in the driver I need to look-up the parent node, then > >>> the node. I don't plan to do that in the version of the driver, just an > >>> observation. > >>> > >>> There is something else I want to discuss on the dt-bindings actually. > >>> According to the schema, the spi-max-frequency is: > >>> > >>> > Maximum SPI clocking speed of the device in Hz. > >>> > >>> The ad4052 has 2 maximum speeds: Configuration mode (lower) and ADC Mode > >>> (higher, depends on VIO). The solution I came up, to not require a > >>> custom regmap spi bus, is to have spi-max-frequency bound the > >>> Configuration mode speed, > >> > >> The purpose of spi-max-frequency in the devicetree is that sometimes > >> the wiring of a complete system makes the effective max frequency > >> lower than what is allowed by the datasheet. So this really needs > >> to be the absolute highest frequency allowed. > >> > >>> and have ADC Mode set by VIO regulator > >>> voltage, through spi_transfer.speed_hz. At the end of the day, both are > >>> bounded by the spi controller maximum speed. > >> > >> If spi_transfer.speed_hz > spi-max-frequency, then the core SPI code > >> uses spi-max-frequency. So I don't think this would actually work. > >> > > Ok, so that's something that may be worth some attention. > > > > At spi/spi.c#2472 > > if (!of_property_read_u32(nc, "spi-max-frequency", &value)) > > spi->max_speed_hz = value; > > > > At spi/spi.c#4090 > > if (!xfer->speed_hz) > > xfer->speed_hz = spi->max_speed_hz; > > > > So, speed_hz is max-spi-frequency only if xfer->speed_hz is 0 and > > not bounded by it. > > Ah, OK, my memory was wrong. It is only bound by the controller max > speed, not the device max speed. > > if (ctlr->max_speed_hz && xfer->speed_hz > ctlr->max_speed_hz) > xfer->speed_hz = ctlr->max_speed_hz; > > It does seem odd that it would allow setting an individual xfer > speed higher than than the given device max speed. I suppose we > could submit a patch adding that check to the SPI core code and > see what Mark has to say. > Agreed, the patch itself would be simple: if (!xfer->speed_hz || xfer->speed_hz > spi->max_speed_hz) xfer->speed_hz = spi->max_speed_hz; But I wonder how many drivers rely on this behaviour > > > > Then at spi-axi-spi-engine.c: > > > > static int spi_engine_precompile_message(struct spi_message *msg) > > { > > clk_div = DIV_ROUND_UP(max_hz, xfer->speed_hz); > > xfer->effective_speed_hz = max_hz / min(clk_div, 256U); > > } > > > > Where max_hz is set only by the IP spi_clk. If at the driver I set > > xfer.speed_hz, it won't be bounded by max-spi-frequency. > > > > The only that seems to bound as described is the layer for flash memory > > at spi-mem.c@spi_mem_adjust_op_freq. > > > > For the adc driver, I will then consider your behavioral description and > > create a custom regmap bus to limit set the reg access speed (fixed), > > and keep adc mode speed set by VIO. And consider spi-max-frequency can > > further reduce both speeds. > > (or should instead be handled at the driver like spi-mem.c ?) > > It would be more work, but if it is common enough, we could generalize this > in the core code. For example add a spi-register-max-frequency binding (or > even a more general spi-max-freqency-map to map operations to max frequencies). > Then we could bake it into the regmap_spi code to handle this property > and not have to make a separate bus. > > FWIW, there are also some SPI TFT displays that use a different frequency > for register access compared to framebuffer data that could potentially > use this too. Right now, these just have a hard-coded register access > frequency of e.g. 10 MHz. > I implemented the custom regmap bus for this series. With a `spi-max-frequency-map`, the regmap bus can be removed. I don't want to include this regmap spi patch to this series. As I see it, struct regmap_but first need to be extended to add a max_speed, e.g. @max_speed: Max transfer speed that can be used on the bus. regmap_spi.c would then look for the devicetree node to fill the value and on regmap_write/read fill speed_hz. In this case, it could be called "register-frequency" or "regmap-frequency" If instead it is up to spi.c to read the devicetree node, then a way to differentiate "regular" transfers from "regmap" transfers would be necessary. About submitting v3, should I submit only up-to the base driver, or can I submit also the add offload support and add event support commits? Regards, Jorge
On 6/3/25 2:29 AM, Jorge Marques wrote: > On Mon, Jun 02, 2025 at 12:23:40PM -0500, David Lechner wrote: >> On 6/2/25 11:32 AM, Jorge Marques wrote: >>> Hi David, >>> >>> On Mon, Jun 02, 2025 at 10:17:18AM -0500, David Lechner wrote: >>>> On 6/2/25 4:17 AM, Jorge Marques wrote: >>>>> On Tue, Apr 29, 2025 at 10:45:20AM -0500, David Lechner wrote: >>>>>> On 4/29/25 8:48 AM, Jorge Marques wrote: >>>>>>> Hi David, >>>>>>> >>>>>>> I didn't went through your's and Jonathan's ad4052.c review yet, >>>>>>> but for the trigger-source-cells I need to dig deeper and make >>>>>>> considerable changes to the driver, as well as hardware tests. >>>>>>> My idea was to have a less customizable driver, but I get that it is >>>>>>> more interesting to make it user-definable. >>>>>> >>>>>> We don't need to make the driver support all possibilities, but the devicetree >>>>>> needs to be as complete as possible since it can't be as easily changed in the >>>>>> future. >>>>>> >>>>> >>>>> Ack. >>>>> >>>>> I see that the node goes in the spi controller (the parent). To use the >>>>> same information in the driver I need to look-up the parent node, then >>>>> the node. I don't plan to do that in the version of the driver, just an >>>>> observation. >>>>> >>>>> There is something else I want to discuss on the dt-bindings actually. >>>>> According to the schema, the spi-max-frequency is: >>>>> >>>>> > Maximum SPI clocking speed of the device in Hz. >>>>> >>>>> The ad4052 has 2 maximum speeds: Configuration mode (lower) and ADC Mode >>>>> (higher, depends on VIO). The solution I came up, to not require a >>>>> custom regmap spi bus, is to have spi-max-frequency bound the >>>>> Configuration mode speed, >>>> >>>> The purpose of spi-max-frequency in the devicetree is that sometimes >>>> the wiring of a complete system makes the effective max frequency >>>> lower than what is allowed by the datasheet. So this really needs >>>> to be the absolute highest frequency allowed. >>>> >>>>> and have ADC Mode set by VIO regulator >>>>> voltage, through spi_transfer.speed_hz. At the end of the day, both are >>>>> bounded by the spi controller maximum speed. >>>> >>>> If spi_transfer.speed_hz > spi-max-frequency, then the core SPI code >>>> uses spi-max-frequency. So I don't think this would actually work. >>>> >>> Ok, so that's something that may be worth some attention. >>> >>> At spi/spi.c#2472 >>> if (!of_property_read_u32(nc, "spi-max-frequency", &value)) >>> spi->max_speed_hz = value; >>> >>> At spi/spi.c#4090 >>> if (!xfer->speed_hz) >>> xfer->speed_hz = spi->max_speed_hz; >>> >>> So, speed_hz is max-spi-frequency only if xfer->speed_hz is 0 and >>> not bounded by it. >> >> Ah, OK, my memory was wrong. It is only bound by the controller max >> speed, not the device max speed. >> >> if (ctlr->max_speed_hz && xfer->speed_hz > ctlr->max_speed_hz) >> xfer->speed_hz = ctlr->max_speed_hz; >> >> It does seem odd that it would allow setting an individual xfer >> speed higher than than the given device max speed. I suppose we >> could submit a patch adding that check to the SPI core code and >> see what Mark has to say. >> > > Agreed, the patch itself would be simple: > > if (!xfer->speed_hz || xfer->speed_hz > spi->max_speed_hz) > xfer->speed_hz = spi->max_speed_hz; > > But I wonder how many drivers rely on this behaviour Only one way to find out. Try it. :-) >>> >>> Then at spi-axi-spi-engine.c: >>> >>> static int spi_engine_precompile_message(struct spi_message *msg) >>> { >>> clk_div = DIV_ROUND_UP(max_hz, xfer->speed_hz); >>> xfer->effective_speed_hz = max_hz / min(clk_div, 256U); >>> } >>> >>> Where max_hz is set only by the IP spi_clk. If at the driver I set >>> xfer.speed_hz, it won't be bounded by max-spi-frequency. >>> >>> The only that seems to bound as described is the layer for flash memory >>> at spi-mem.c@spi_mem_adjust_op_freq. >>> >>> For the adc driver, I will then consider your behavioral description and >>> create a custom regmap bus to limit set the reg access speed (fixed), >>> and keep adc mode speed set by VIO. And consider spi-max-frequency can >>> further reduce both speeds. >>> (or should instead be handled at the driver like spi-mem.c ?) >> >> It would be more work, but if it is common enough, we could generalize this >> in the core code. For example add a spi-register-max-frequency binding (or >> even a more general spi-max-freqency-map to map operations to max frequencies). >> Then we could bake it into the regmap_spi code to handle this property >> and not have to make a separate bus. >> >> FWIW, there are also some SPI TFT displays that use a different frequency >> for register access compared to framebuffer data that could potentially >> use this too. Right now, these just have a hard-coded register access >> frequency of e.g. 10 MHz. >> > > I implemented the custom regmap bus for this series. Good plan. > With a `spi-max-frequency-map`, the regmap bus can be removed. > I don't want to include this regmap spi patch to this series. > As I see it, struct regmap_but first need to be extended to add > a max_speed, e.g. > > @max_speed: Max transfer speed that can be used on the bus. > > regmap_spi.c would then look for the devicetree node to fill the value > and on regmap_write/read fill speed_hz. > In this case, it could be called "register-frequency" or > "regmap-frequency" > If instead it is up to spi.c to read the devicetree node, then a way to > differentiate "regular" transfers from "regmap" transfers would be > necessary. > > About submitting v3, should I submit only up-to the base driver, or can > I submit also the add offload support and add event support commits? I wouldn't add anything new at this point. Being able to spread out the review a bit will lead to better reviews.
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml new file mode 100644 index 0000000000000000000000000000000000000000..a0510d485f130ceec15b887e8f8deeb2cf6562c8 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml @@ -0,0 +1,98 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +# Copyright 2025 Analog Devices Inc. +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/adi,ad4052.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Analog Devices AD4052 ADC family device driver + +maintainers: + - Jorge Marques <jorge.marques@analog.com> + +description: | + Analog Devices AD4052 Single Channel Precision SAR ADC family + + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4050-ad4056.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4052-ad4058.pdf + +properties: + compatible: + enum: + - adi,ad4050 + - adi,ad4052 + - adi,ad4056 + - adi,ad4058 + + reg: + maxItems: 1 + + interrupts: + items: + - description: Signal coming from the GP0 pin (threshold). + - description: Signal coming from the GP1 pin (data ready). + + interrupt-names: + items: + - const: gp0 + - const: gp1 + + gpio-controller: true + + "#gpio-cells": + const: 2 + description: | + The first cell is the GPn number: 0 to 1. + The second cell takes standard GPIO flags. + + cnv-gpios: + description: The Convert Input (CNV). If omitted, CNV is tied to SPI CS. + maxItems: 1 + + spi-max-frequency: + maximum: 62500000 + + vdd-supply: + description: Analog power supply. + + vio-supply: + description: Digital interface logic power supply. + + vref-supply: + description: Reference voltage to set the ADC full-scale range. + +required: + - compatible + - reg + - vdd-supply + - vio-supply + +allOf: + - $ref: /schemas/spi/spi-peripheral-props.yaml# + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + + spi { + #address-cells = <1>; + #size-cells = <0>; + + adc@0 { + compatible = "adi,ad4052"; + reg = <0>; + vdd-supply = <&adc_vdd>; + vio-supply = <&adc_vio>; + spi-max-frequency = <25000000>; + + interrupt-parent = <&gpio>; + interrupts = <0 0 IRQ_TYPE_EDGE_RISING>, + <0 1 IRQ_TYPE_EDGE_FALLING>; + interrupt-names = "gp0", "gp1"; + cnv-gpios = <&gpio 2 GPIO_ACTIVE_HIGH>; + }; + }; +... diff --git a/MAINTAINERS b/MAINTAINERS index 01079a189c93697c1db6b0ca4e54212d25589974..81fbe7176475c48eae03ab04115d4ef5b6299fac 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1329,6 +1329,12 @@ F: Documentation/devicetree/bindings/iio/adc/adi,ad4030.yaml F: Documentation/iio/ad4030.rst F: drivers/iio/adc/ad4030.c +ANALOG DEVICES INC AD4052 DRIVER +M: Jorge Marques <jorge.marques@analog.com> +S: Supported +W: https://ez.analog.com/linux-software-drivers +F: Documentation/devicetree/bindings/iio/adc/adi,ad4052.yaml + ANALOG DEVICES INC AD4130 DRIVER M: Cosmin Tanislav <cosmin.tanislav@analog.com> L: linux-iio@vger.kernel.org
Add dt-bindings for AD4052 family, devices AD4050/AD4052/AD4056/AD4058, low-power with monitor capabilities SAR ADCs. Each variant of the family differs in speed and resolution, resulting in different scan types and spi word sizes, that are matched by the compatible with the chip_info. The device contains one input (cnv) and two outputs (gp0, gp1). Signed-off-by: Jorge Marques <jorge.marques@analog.com> --- .../devicetree/bindings/iio/adc/adi,ad4052.yaml | 98 ++++++++++++++++++++++ MAINTAINERS | 6 ++ 2 files changed, 104 insertions(+)