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@xxxxxxxxxx> > > --- > > ... > > > + 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
