| Message ID | 20220715044948.434149-1-cosmin.tanislav@analog.com |
|---|---|
| Headers | show |
| Series | AD4130 | expand |
On 7/18/22 16:14, Linus Walleij wrote: > Hi Cosmin, > > thanks for your patch! > > On Fri, Jul 15, 2022 at 6:50 AM 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. >> >> Signed-off-by: Cosmin Tanislav <cosmin.tanislav@analog.com> >> Reviewed-by: Rob Herring <robh@kernel.org> > (...) > > This caught my eye: > >> + adi,int-clk-out: >> + description: Specify if the internal clock should be exposed on the CLK pin. >> + type: boolean > > Okay, but would it not make more sense to just imply this if the clock > on the CLK > pin has any consumers? Like update this setting in hardware when the consumer > does clk_prepare() or so on that externally routed clock? > You're right, this is indeed fit for being implemented using the clock framework. >> + adi,ext-clk-freq-hz: >> + description: Specify the frequency of the external clock. >> + enum: [76800, 153600] >> + default: 76800 > > This looks like cheating, i.e just outputting a clock on that pin > and ignoring to model the consumer. You got this wrong. The chip has 4 operating modes regarding clocking. Internal 76.8kHz clock (clkout can be used as an interrupt pin). Internal 76.8kHz clock, available externally on the clkout pin (clkout becomes an output). External 76.8kHz clock (clkout is an input). External 153.6kHz clock, internally divided by two (clkout is an input). This property is used to choose between what frequency to set the external clock up with. Indeed, if the external clock is not present, then exposing the 76.8kHz clock using the clock framework would be fine. Maybe you have a better suggestion about what to do with this? How do I tell the chip what frequency the external clock is, but also tell the clock what frequency to use? It's a bit of a conundrum for me. > > Shouldn't this rather be a clkout subnode with 2 #clock-cells > and the fequency set in a cell in a consumer phandle? > Like how I did in > commit 7335631fcd5eecfa84555bd57433e6446d06ad21 > "dt-bindings: clock: u8500: Add clkout clock bindings" > > Usually it is the consumer that requests a specific clock and then the > producer will respond. > > Certainly whatever is consuming this clock needs to be in the device tree > as well, and then this is the right pattern. > > (In Linux you will then use the clk framework to manage the clock and callbacks > but that is irrelevant for the DT bindings.) > >> + adi,bipolar: >> + description: Specify if the device should be used in bipolar mode. >> + type: boolean > > Can you explain what this means? I don't understand what it would > mean for an analog device / AFE to be in bipolar mode. > Range becomes [-VRef, VRef], as opposed to [0, VRef], resolution is halved. > Other than that it looks very nice! > > Yours, > Linus Walleij
On Thu, Sep 8, 2022 at 9:03 AM Cosmin Tanislav <demonsingur@gmail.com> wrote: > >> + adi,ext-clk-freq-hz: > >> + description: Specify the frequency of the external clock. > >> + enum: [76800, 153600] > >> + default: 76800 > > > > This looks like cheating, i.e just outputting a clock on that pin > > and ignoring to model the consumer. > > You got this wrong. > > The chip has 4 operating modes regarding clocking. > > Internal 76.8kHz clock (clkout can be used as an interrupt pin). > Internal 76.8kHz clock, available externally on the clkout pin (clkout > becomes an output). > External 76.8kHz clock (clkout is an input). > External 153.6kHz clock, internally divided by two (clkout is an input). > > This property is used to choose between what frequency to set the > external clock up with. Indeed, if the external clock is not present, > then exposing the 76.8kHz clock using the clock framework would be fine. > > Maybe you have a better suggestion about what to do with this? > How do I tell the chip what frequency the external clock is, but also > tell the clock what frequency to use? It's a bit of a conundrum for me. I would imagine not specify in the device tree what frequency to use at all. Instead use software for that. The clock provide for the clkout should provide static const struct clk_ops extclk_ops = { .recalc_rate = extclk_recalc_rate, .round_rate = extclk_round_rate, .set_rate = extclk_set_rate, }; This way the consumer driver can issue clk_round_rate(), clk_set_rate() etc to make the clk driver determine which parent to use for the consumer. Likewise .enable or .prepare should then mux out the desired clock. > >> + adi,bipolar: > >> + description: Specify if the device should be used in bipolar mode. > >> + type: boolean > > > > Can you explain what this means? I don't understand what it would > > mean for an analog device / AFE to be in bipolar mode. > > > > Range becomes [-VRef, VRef], as opposed to [0, VRef], resolution is > halved. OK that makes sense, put that in the binding description. Yours, Linus Walleij
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. V1 -> V2 * add kernel version to ABI file * merge ABI patch into driver patch * make copyright header similar to other drivers * rearrange includes * use units.h defines where possible and add unit sufix to SOFT_RESET_SLEEP define * remove ending comma to last members of enums / lists * remove unused FILTER_MAX define * use BIT macro for PIN_FN_* * rearrange SETUP_SIZE definition * group bools in ad4130_state and ad4130_chan_info * put scale_tbls definition on one line * remove newline before reg size == 0 check * put mask used as value in a variable * remove useless ret = 0 assignment * make buffer attrs oneline * use for_each_set_bit in update_scan_mode * use if else for internal reference voltage error checking * inline reference voltage check * check number of vbias pins * remove .has_int_pin = false * remove avail_len for IIO_AVAIL_RANGE * remove useless enabled_channels check in unlink_slot * remove unused AD4130_RESET_CLK_COUNT define * only call fwnode_handle_put for child in case of error * default adi,reference-select to REFIN1 * default adi,int-ref-en to false * of_irq_get_byname -> fwnode_irq_get_byname * P1 -> P2 as interrupt pin options * add missing comma in db3_freq_avail init * cast values to u64 to make math using units.h work * add datasheet reference to IRQ polarity * add comment about disabling channels in predisable * add part number prefix find_table_index * return voltage from get_ref_voltage * add datasheet reference for internal reference voltage selection * add comment explaining AIN and GPIO pin sharing * parse channel setup before parsing excitation pins * only validate excitation pin if value is not off * use FIELD_PREP for bipolar and int_ref_en * put devm_regmap_init call on one line * introduce a slot_info struct to contain setup_info for each slot * enable internal reference automatically if needed * decide mclk sel based on adi,ext-clk-freq and adi,int-clk-out * dt-bindings: use internal reference explicitly * dt-bindings: set type for adi,excitation-pin-0 * dt-bindings: set $ref for adi,vbias-pins * dt-bindings: remove minItems from interrupts property * dt-bindings: remove adi,int-ref-en default value * dt-bindings: remove adi,bipolar default value * dt-bindings: inline adi,int-ref-en description * dt-bindings: default adi,reference-select to REFIN1 * dt-bindings: clean up description for diff-channels and adi,reference-select * dt-bindings: add more text to interrupt-names description * dt-bindings: turn interrupt-names into a single string * dt-bindings: add maxItems to adi,vbias-pins V2 -> V3 * dt-bindings: add interrupt controller include to example * dt-bindings: remove $ref in diff-channels V3 -> V4: * handle watermark value as number of datum * DOUT_OR_INT -> INT * AD4130_8_NAME -> AD4130_NAME * return early in case of failure when parsing fw channel * use IIO_DMA_MINALIGN for aligning buffer * add comments for fs_to_freq and freq_to_fs * remove support for other variants because of unavailability of model ids for future chip variants * remove support for db3 frequency because of inaccuracy when calculating * remove ternary where possible * refactor defines * dt-bindings: add unevaluatedProperties: true to channel node V4 -> V5: * simplify get_ref_voltage function and move print statement to first user * inline statements not going over the 80 cols limit * simplify scale table filling * determine table length inside find table index macro * current_na -> tmp inside ad4130_parse_fw_setup * define full register set * put range register size definitions on one line * nanoamps -> nanoamp * adi,ext-clk-freq -> adi,ext-clk-freq-hz * return directly in ad4130_validate_vbias_pins * place comment regarding irq_trigger at assignment * inversed -> inverted inside irq_trigger comment * do not initialize int_clk_out * return directly in ad4130_validate_diff_channels * add () after reference to update_scan_mode in comment * use BIT() for channel offset * comment nitpicks on slot finding * return -EINVAL out of reg read for invalid sizes * place regmap at start of ad4130_state * place bools at the end of ad4130_setup_info * remove commas after terminators * dt-bindings: only allow one element in reg * dt-bindings: inline reg description * dt-bindings: remove $ref from adi,ext-clk-freq-hz V5 -> V6: * bump KernelVersion * use IIO_DEVICE_ATTR_RO * nitpick inside mutex comment * use valid_mask for validating gpios * improve DMA comment V6 -> V7: * remove $ref from -nanoamp properties (to be added to dtschema) * use hexadecimal numbers for channel unit-addresses V7 -> V8: * pick up Reviewed-By tags * bump KernelVersion Cosmin Tanislav (2): dt-bindings: iio: adc: add AD4130 iio: adc: ad4130: add AD4130 driver .../ABI/testing/sysfs-bus-iio-adc-ad4130 | 36 + .../bindings/iio/adc/adi,ad4130.yaml | 256 +++ MAINTAINERS | 8 + drivers/iio/adc/Kconfig | 13 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/ad4130.c | 2014 +++++++++++++++++ 6 files changed, 2328 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad4130 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml create mode 100644 drivers/iio/adc/ad4130.c