Messages in this thread |  | | Date | Sun, 17 Apr 2022 13:16:53 +0300 | Subject | Re: [PATCH v1 1/3] dt-bindings: iio: adc: add AD4130 | From | Cosmin Tanislav <> |
| |
On 4/16/22 18:00, Jonathan Cameron wrote: > On Wed, 13 Apr 2022 12:40:09 +0300 > Cosmin Tanislav <demonsingur@gmail.com> wrote: > >> AD4130-8 is an ultra-low power, high precision, >> measurement solution for low bandwidth battery >> operated applications. >> >> The fully integrated AFE (Analog Front-End) >> includes a multiplexer for up to 16 single-ended >> or 8 differential inputs, PGA (Programmable Gain >> Amplifier), 24-bit Sigma-Delta ADC, on-chip >> reference and oscillator, selectable filter >> options, smart sequencer, sensor biasing and >> excitation options, diagnostics, and a FIFO >> buffer. > > Hi Cosmin, > > Please wrap at around 70-75 chars for patch descriptions. That > leaves a bit of room for indenting due to automated tooling > / email threads before we hit 80 chars. Definitely don't need > 30 chars of room for it! >
Yeah, I'll do it.
> It seems you hit a lot of things that Rob's bot had found that > you would have seen on doing a build test. Please make sure > you do one in future to save time! >
Yeah, sorry. I already fixed them for V2.
> Please use a cover letter for a series like this. It provides several > advantages: > > 1) Somewhere to add a brief introduction to the whole series. > 2) Place for people to give tags for whole series that the b4 tool > I use to pick up patches can then automatically find them from. > 3) Gives series a nice unified name in patchwork ;) > https://patchwork.kernel.org/project/linux-iio/list/?series=631830 >
Sure thing. I'll have it there for V2 anyway.
>> >> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com> >> --- >> .../bindings/iio/adc/adi,ad4130.yaml | 255 ++++++++++++++++++ >> 1 file changed, 255 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml >> >> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml >> new file mode 100644 >> index 000000000000..e9dce54e9802 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml >> @@ -0,0 +1,255 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +# Copyright 2022 Analog Devices Inc. >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/iio/adc/adi,ad4130.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Analog Devices AD4130 ADC device driver >> + >> +maintainers: >> + - Cosmin Tanislav <cosmin.tanislav@analog.com> >> + >> +description: | >> + Bindings for the Analog Devices AD4130 ADC. Datasheet can be found here: >> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD4130-8.pdf >> + >> +properties: >> + compatible: >> + enum: >> + - adi,ad4130-8-16-lfcsp >> + - adi,ad4130-8-16-wlcsp >> + - adi,ad4130-8-24-lfcsp >> + - adi,ad4130-8-24-wlcsp > > What are the variants? They look to possibly be package differences? > + resolution differences. > They definitely need some description here. > It may make more sense to have one compatible and then some extra > booleans to say what it supported. >
Packaging + available interrupt pins + resolution. Is having extra booleans to describe what is supported really the best approach? It's not really about how the hardware is configured anymore, is it? They're different chips.
> Long shot, but do the different packages have different model IDs? > The datasheet says > Model ID: 24-bit WLCSP Model ID. These bits are set by default for each model and > are read only. If there is one for each of these models then it would be > better to have a single compatible and do the detection of variant in > the driver. > > I'm not immediately spotting the resolution information in the data sheet. > It is marked Preliminary but if there are details missing, please mention > in cover letter so we don't go looking for information that doesn't exist. >
I don't have enough information about the other models to know what Model IDs they will have. That's why I took this approach.
>> + >> + reg: >> + maxItems: 1 >> + >> + clocks: >> + maxItems: 1 >> + description: phandle to the master clock (mclk) >> + >> + clock-names: >> + items: >> + - const: mclk >> + >> + interrupts: >> + minItems: 1 >> + maxItems: 1 >> + >> + interrupt-names: >> + minItems: 1 >> + maxItems: 1 >> + description: >> + Default if not supplied is dout-int. >> + items: >> + enum: >> + - dout-int >> + - clk >> + - p1 > > This is unusual. It is probably helpful to add more description to > explain that the data ready/ fifo interrupt can be routed to any of these > pins.
Is it unusual? ADIS16480 follows a similar approach.
description: | Specify which interrupt pin should be configured as Data Ready / FIFO interrupt. Default if not supplied is dout-int.
How does this sound?
> >> + - dout >> + >> + '#address-cells': >> + const: 1 >> + >> + '#size-cells': >> + const: 0 >> + >> + refin1-supply: >> + description: refin1 supply. Can be used as reference for conversion. >> + >> + refin2-supply: >> + description: refin2 supply. Can be used as reference for conversion. >> + >> + avdd-supply: >> + description: AVDD voltage supply. Can be used as reference for conversion. > > Whilst these are optional in theory, you should call out that they must be > provided if any of the channels use them as a reference. >
I thought that "Can be used as reference for conversion." + it being an option in adi,reference-select property would make it obvious, no?
>> + >> + iovdd-supply: >> + description: IOVDD voltage supply. Used for the chip interface. >> + >> + spi-max-frequency: >> + maximum: 5000000 >> + >> + adi,mclk-sel: >> + description: | >> + Select the clock. >> + 0: Internal 76.8kHz clock. >> + 1: Internal 76.8kHz clock, output to the CLK pin. >> + 2: External 76.8kHz clock. >> + 3. External 153.6kHz clock. > > For the external clocks, can we use the fact that one is supplied > as enough to tell us we should be using them? Then query the > frequency directly from that clock? >
Aren't we supposed to set the frequency of that clock ourselves, in the driver?
> If no clock provided then clearly internal. All that is > necessary after that is a boolean to control if the CLK output > is enabled or not (and ideally constrain that to only be possible > if in internal clock mode). >
Well...
So, mclk present => external, not present => internal. adi,int-clk-out-enable to specify if the internal clock should be exposed? adi,ext-clk-freq to specify the desired clock speed of the external clk?
>> + $ref: /schemas/types.yaml#/definitions/uint32 >> + enum: [0, 1, 2, 3] >> + default: 0 >> + >> + adi,int-ref-en: > > Mentioned below... > >> + description: | >> + Specify if internal reference should be enabled. >> + type: boolean >> + default: true >> + >> + adi,bipolar: >> + description: Specify if the device should be used in bipolar mode. >> + type: boolean >> + default: false >> + >> + adi,vbias-pins: >> + description: Analog inputs to apply a voltage bias of (AVDD − AVSS) / 2 to. >> + items: >> + minimum: 0 >> + maximum: 15 > > If I read things correctly VBIAS_CONTROL is a bitmap across the 16 input lines. > So why use a single value rather than either a list of pins, or a bitmap? >
Umm. Isn't this a list of pins? That's why everything is plural here. I guess I should add `maxItems: 16`? I already added `$ref: /schemas/types.yaml#/definitions/uint32-array`.
>> + >> +required: >> + - compatible >> + - reg >> + - interrupts >> + >> +patternProperties: >> + "^channel@([0-9]|1[0-5])$": >> + type: object >> + $ref: adc.yaml >> + >> + properties: >> + reg: >> + description: | >> + The channel number. >> + items: >> + minimum: 0 >> + maximum: 15 >> + >> + diff-channels: >> + description: | >> + Besides the analog inputs available, internal inputs can be used. >> + 16: Internal temperature sensor. >> + 17: AVss >> + 18: Internal reference. >> + 19: DGND. >> + 20: (AVDD − AVSS)/6+ >> + 21: (AVDD − AVSS)/6- >> + 22: (IOVDD − DGND)/6+ >> + 23: (IOVDD − DGND)/6- >> + 24: (ALDO − AVSS)/6+ >> + 25: (ALDO − AVSS)/6- >> + 26: (DLDO − DGND)/6+ >> + 27: (DLDO − DGND)/6- >> + 28: V_MV_P >> + 29: V_MV_M >> + $ref: adc.yaml >> + items: >> + minimum: 0 >> + maximum: 29 > > Interesting. So we have a part that has a 16 channel sequencer, but > can you have more channels as long as you don't want them all at once? > For example, I doubt anyone wants to permanently configure a device to monitor > the various supplies, but they will want to occasionally. > > As such, perhaps we need to treat this device more flexibly? > There are obviously contraints on what channels + references make sense > but maybe we should allow more than 16 to be specified? >
Ehhhhh. Look at the driver. It's already pushing 2k+ lines with the 8 setups for 16 channels situation + all the extra options the chip provides. If we also make it so that channels are rewritten at runtime, it will turn into a mess. Or at least I don't see a clean way of adding that. Besides, then I'd have to do all these extra allocations depending on the number of channels in the device tree... It gets complicated. If a customer expresses his interest in this, I guess I'll have to add it. Also, presumably the extra inputs are marketed as diagnostics.
>> + >> + adi,reference-select: >> + description: | >> + Select the reference source to use when converting on the >> + specific channel. Valid values are: >> + 0: REFIN1(+)/REFIN1(−). >> + 1: REFIN2(+)/REFIN2(−). >> + 2: REFOUT/AVSS (Internal reference) >> + 3: AVDD/AVSS >> + If not specified, internal reference is used. > > That's not a good idea if it might be turned off... > Perhaps a better approach would be to drop the int_ref_en and > simply walk the channels to find out if any of them use it. > If they don't turn it off, otherwise leave it on. >
Yeah, I guess I could do that.
>> + $ref: /schemas/types.yaml#/definitions/uint32 >> + enum: [0, 1, 2, 3] >> + default: 2 >> + >> + adi,excitation-pin-0: >> + description: | >> + Analog input to apply excitation current to while the channel >> + is active. >> + minimum: 0 >> + maximum: 15 >> + default: 0 >> + >> + adi,excitation-pin-1: >> + description: | >> + Analog input to apply excitation current to while this channel >> + is active. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + minimum: 0 >> + maximum: 15 >> + default: 0 >> + >> + adi,excitation-current-0-nanoamps: >> + description: | >> + Excitation current in nanoamps to be applied to pin specified in >> + adi,excitation-pin-0 while this channel is active. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + enum: [0, 100, 10000, 20000, 50000, 100000, 150000, 200000] >> + default: 0 >> + >> + adi,excitation-current-1-nanoamps: >> + description: | >> + Excitation current in nanoamps to be applied to pin specified in >> + adi,excitation-pin-1 while this channel is active. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + enum: [0, 100, 10000, 20000, 50000, 100000, 150000, 200000] >> + default: 0 >> + >> + adi,burnout-current-nanoamps: >> + description: | >> + Burnout current in nanoamps to be applied for this channel. >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + enum: [0, 500, 2000, 4000] >> + default: 0 >> + >> + adi,buffered-positive: >> + description: Enable buffered mode for positive input. >> + type: boolean >> + >> + adi,buffered-negative: >> + description: Enable buffered mode for negative input. >> + type: boolean >> + >> + required: >> + - reg >> + - diff-channels >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + spi { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + adc@0 { >> + compatible = "adi,ad4130-8-24-wlcsp"; >> + reg = <0>; >> + >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + spi-max-frequency = <5000000>; >> + interrupts = <27 IRQ_TYPE_EDGE_FALLING>; >> + interrupt-parent = <&gpio>; >> + >> + channel@0 { >> + reg = <0>; >> + /* AIN8, AIN9 */ >> + diff-channels = <8 9>; >> + }; >> + >> + channel@1 { >> + reg = <1>; >> + /* AIN10, AIN11 */ >> + diff-channels = <10 11>; >> + }; >> + >> + channel@2 { >> + reg = <2>; >> + /* Temperature Sensor, DGND */ >> + diff-channels = <16 19>; >> + }; >> + >> + channel@3 { >> + reg = <3>; >> + /* Internal reference, DGND */ >> + diff-channels = <18 19>; >> + }; >> + >> + channel@4 { >> + reg = <4>; >> + /* DGND, DGND */ >> + diff-channels = <19 19>; >> + }; >> + }; >> + }; >
|  |