linux-kernel - Re: [PATCH v2 3/5] dt-bindings: iio: adc: Add adi,ad4052

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <hvexchm2ozsto5s2o6n5j2z3odrkbcamgmg67umd4aehwzmgie@dvtx6anioasq>
Date: Tue, 29 Apr 2025 15:48:43 +0200
From: Jorge Marques <gastmaier@...il.com>
To: David Lechner <dlechner@...libre.com>
Cc: Jorge Marques <jorge.marques@...log.com>, 
	Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>, 
	Michael Hennerich <Michael.Hennerich@...log.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Jonathan Corbet <corbet@....net>, Nuno Sá <nuno.sa@...log.com>, 
	Andy Shevchenko <andy@...nel.org>, Uwe Kleine-König <ukleinek@...nel.org>, 
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-doc@...r.kernel.org, linux-pwm@...r.kernel.org
Subject: Re: [PATCH v2 3/5] dt-bindings: iio: adc: Add adi,ad4052

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@...log.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