| Message ID | 20231212104451.22522-1-mitrutzceclan@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v8,1/2] dt-bindings: adc: add AD7173 | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Tue, Dec 12, 2023 at 11:45 AM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote: > > The AD7173 family offer a complete integrated Sigma-Delta ADC solution > which can be used in high precision, low noise single channel applications > or higher speed multiplexed applications. The Sigma-Delta ADC is intended > primarily for measurement of signals close to DC but also delivers > outstanding performance with input bandwidths out to ~10kHz. As stated in [1], we should try to make complete bindings. I think more could be done here to make this more complete. Most notably, the gpio-controller binding is missing. Also maybe something is needed to describe how the SYNC/ERROR pin is wired up since it can be an input or an output with different functions? [1]: https://www.kernel.org/doc/html/latest/devicetree/bindings/writing-bindings.html > > Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com> > --- > V7->V8 > - include missing fix from V6 Including the cumulative changelog for all revisions would be helpful to reviewers who haven't been following closely. > > .../bindings/iio/adc/adi,ad7173.yaml | 170 ++++++++++++++++++ > 1 file changed, 170 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml > new file mode 100644 > index 000000000000..25a5404ee353 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml > @@ -0,0 +1,170 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +# Copyright 2023 Analog Devices Inc. > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/adc/adi,ad7173.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Analog Devices AD7173 ADC > + > +maintainers: > + - Ceclan Dumitru <dumitru.ceclan@analog.com> > + > +description: | > + Bindings for the Analog Devices AD717X ADC's. Datasheets for supported chips: > + https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf > + https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf > + https://www.analog.com/media/en/technical-documentation/data-sheets/AD7175-2.pdf > + https://www.analog.com/media/en/technical-documentation/data-sheets/AD7176-2.pdf > + > +properties: > + compatible: > + enum: > + - adi,ad7172-2 > + - adi,ad7173-8 > + - adi,ad7175-2 > + - adi,ad7176-2 > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 Shouldn't this be 2? The datasheet says there is a "Data Output Ready" signal on the DOUT/RDY pin and an "Error Output" on the SYNC/ERROR pin. Although I could see how RDY could be considered part of the SPI bus. In any case, a description explaining what the interrupt is would be useful. > + > + '#address-cells': > + const: 1 > + > + '#size-cells': > + const: 0 > + > + spi-max-frequency: > + maximum: 20000000 > + > + refin-supply: > + description: external reference supply, can be used as reference for conversion. > + > + refin2-supply: > + description: external reference supply, can be used as reference for conversion. > + > + avdd-supply: > + description: avdd supply, can be used as reference for conversion. What about other supplies? AVDD1, AVDD2, IOVDD. > + > +patternProperties: > + "^channel@[0-9a-f]$": > + type: object > + $ref: adc.yaml > + unevaluatedProperties: false > + > + properties: > + reg: > + minimum: 0 > + maximum: 15 > + > + diff-channels: > + items: > + minimum: 0 > + maximum: 31 Do we need to add overrides to limit the maximums for each compatible string? > + > + adi,reference-select: > + description: | > + Select the reference source to use when converting on > + the specific channel. Valid values are: > + refin : REFIN(+)/REFIN(−). > + refin2 : REFIN2(+)/REFIN2(−) > + refout-avss: REFOUT/AVSS (Internal reference) > + avdd : AVDD > + > + External reference refin2 only available on ad7173-8. > + If not specified, internal reference used. > + enum: > + - refin > + - refin2 > + - refout-avss > + - avdd > + default: refout-avss Missing string type? > + > + required: > + - reg > + - diff-channels Individual analog inputs can be used as single-ended or in pairs as differential, right? If so, diff-channels should not be required to allow for single-ended use. And we would need to add something like a single-ended-channel property to adc.yaml to allow mapping analog input pins to channels similar to how diff-channels works, I think (I don't see anything like that there already)? So maybe something like: oneOf: - required: single-ended-channel - required: diff-channels > + > +required: > + - compatible > + - reg > + - interrupts > + > +allOf: > + - $ref: /schemas/spi/spi-peripheral-props.yaml# > + > + - if: > + properties: > + compatible: > + not: > + contains: > + const: adi,ad7173-8 > + then: > + properties: > + refin2-supply: false > + patternProperties: > + "^channel@[0-9a-f]$": > + properties: > + adi,reference-select: > + enum: > + - refin > + - refout-avss > + - avdd > + > +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,ad7173-8"; > + reg = <0>; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + interrupts = <25 IRQ_TYPE_EDGE_FALLING>; > + interrupt-parent = <&gpio>; > + spi-max-frequency = <5000000>; > + > + refin-supply = <&dummy_regulator>; > + > + channel@0 { > + reg = <0>; > + bipolar; > + diff-channels = <0 1>; > + adi,reference-select = "refin"; > + }; > + > + channel@1 { > + reg = <1>; > + diff-channels = <2 3>; > + }; > + > + channel@2 { > + reg = <2>; > + bipolar; > + diff-channels = <4 5>; > + }; > + > + channel@3 { > + reg = <3>; > + bipolar; > + diff-channels = <6 7>; > + }; > + > + channel@4 { > + reg = <4>; > + diff-channels = <8 9>; > + adi,reference-select = "avdd"; > + }; > + }; > + }; > -- > 2.42.0 > >
On Tue, Dec 12, 2023 at 12:44:36PM +0200, Dumitru Ceclan wrote: > The AD7173 family offer a complete integrated Sigma-Delta ADC solution > which can be used in high precision, low noise single channel > applications or higher speed multiplexed applications. The Sigma-Delta > ADC is intended primarily for measurement of signals close to DC but also > delivers outstanding performance with input bandwidths out to ~10kHz. I do not see any major problem in the code, Reviewed-by: Andy Shevchenko <andy@kernel.org> Some nit-picks below, but it's fine if it get addressed later on. Up to you and Jonathan. ... > +static const unsigned int ad7173_sinc5_data_rates[] = { > + 6211000, 6211000, 6211000, 6211000, 6211000, 6211000, 5181000, 4444000, > + 3115000, 2597000, 1007000, 503800, 381000, 200300, 100500, 59520, > + 49680, 20010, 16333, 10000, 5000, 2500, 1250, > +}; > + > +static const unsigned int ad7175_sinc5_data_rates[] = { > + 50000000, 41667000, 31250000, 27778000, 20833000, 17857000, 12500000, I would add a comment with offsets, like ... /* 0-6 */ But better to make it power of two, like each 4 on one line or 8. > + 10000000, 5000000, 2500000, 1000000, 500000, 397500, 200000, > + 100000, 59920, 49960, 20000, 16666, 10000, 5000, > +}; Not that I insist, just consider readability of these tables. ... > + if (chan->type == IIO_TEMP) { > + temp = ((u32)AD7173_VOLTAGE_INT_REF_uV) * MILLI; Hmm... Is the casting mandatory here? > + temp /= AD7173_TEMP_SENSIIVITY_uV_per_C; > + *val = temp; > + *val2 = chan->scan_type.realbits; > + } else { > + *val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel); > + *val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar); > + } ... > + if (chan->type == IIO_TEMP) > + *val = -874379; //-milli_kelvin_to_millicelsius(0)/scale Hmm... Besides C99 comment format, can we actually use the mentioned API? In such a case the comment won't be needed and the value semantics is better to get. > + else > + *val = -BIT(chan->scan_type.realbits - 1); ... > +static int ad7173_debug_reg_access(struct iio_dev *indio_dev, unsigned int reg, > + unsigned int writeval, unsigned int *readval) > +{ > + struct ad7173_state *st = iio_priv(indio_dev); > + u8 reg_size; > + > + if (reg == 0) 0 does not have its definition, does it? > + reg_size = 1; > + else if (reg == AD7173_REG_CRC || reg == AD7173_REG_DATA || > + reg >= AD7173_REG_OFFSET(0)) > + reg_size = 3; > + else > + reg_size = 2; > + > + if (readval) > + return ad_sd_read_reg(&st->sd, reg, reg_size, readval); > + > + return ad_sd_write_reg(&st->sd, reg, reg_size, writeval); > +} ... > + channels_st_priv_arr = devm_kcalloc(dev, num_channels, > + sizeof(*channels_st_priv_arr), > + GFP_KERNEL); > + if (!channels_st_priv_arr) > + return -ENOMEM; The variable name can be made shorter and hence the above will take less LoCs.
On 12/13/23 16:24, Andy Shevchenko wrote: > On Tue, Dec 12, 2023 at 12:44:36PM +0200, Dumitru Ceclan wrote: > ... > >> + if (chan->type == IIO_TEMP) { >> + temp = ((u32)AD7173_VOLTAGE_INT_REF_uV) * MILLI; > > Hmm... Is the casting mandatory here? > Yep, not really needed as MILLI is already declared as unsigned and it will promote the _INT_REF as well. On signed32 it would have overflowed. Are there any cases where this would not be alright?
On Tue, 12 Dec 2023 12:44:36 +0200 Dumitru Ceclan <mitrutzceclan@gmail.com> wrote: > The AD7173 family offer a complete integrated Sigma-Delta ADC solution > which can be used in high precision, low noise single channel > applications or higher speed multiplexed applications. The Sigma-Delta > ADC is intended primarily for measurement of signals close to DC but also > delivers outstanding performance with input bandwidths out to ~10kHz. > > Reviewed-by: Michael Walle <michael@walle.cc> # for gpio-regmap > Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com> Hi Given it seems like you'll be doing a v9, one quick comment from me below. Jonathan > diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c > new file mode 100644 > index 000000000000..96918b24a10a > --- /dev/null > +++ b/drivers/iio/adc/ad7173.c > @@ -0,0 +1,964 @@ ... > +static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev) > +{ ... > + > + if (st->info->has_temp) { > + chan_arr[chan_index] = ad7173_temp_iio_channel_template; > + chan_st_priv = &channels_st_priv_arr[chan_index]; > + chan_st_priv->ain = > + AD7173_CH_ADDRESS(chan_arr[chan_index].channel, chan_arr[chan_index].channel2); > + chan_st_priv->cfg.bipolar = false; > + chan_st_priv->cfg.input_buf = true; > + chan_st_priv->cfg.ref_sel = AD7173_SETUP_REF_SEL_INT_REF; > + st->adc_mode |= AD7173_ADC_MODE_REF_EN; > + > + chan_index++; > + } > + > + device_for_each_child_node(dev, child) { > + chan = &chan_arr[chan_index]; > + chan_st_priv = &channels_st_priv_arr[chan_index]; > + ret = fwnode_property_read_u32_array(child, "diff-channels", > + ain, ARRAY_SIZE(ain)); > + if (ret) { > + fwnode_handle_put(child); > + return ret; > + } > + > + if (ain[0] >= st->info->num_inputs || > + ain[1] >= st->info->num_inputs) { > + fwnode_handle_put(child); > + return dev_err_probe(dev, -EINVAL, > + "Input pin number out of range for pair (%d %d).\n", > + ain[0], ain[1]); > + } > + > + ret = fwnode_property_match_property_string(child, > + "adi,reference-select", > + ad7173_ref_sel_str, > + ARRAY_SIZE(ad7173_ref_sel_str)); > + > + if (ret < 0) > + ref_sel = AD7173_SETUP_REF_SEL_INT_REF; > + else > + ref_sel = ret; Simpler pattern for properties with a default is not to check the error code. ref_sel = AD7173_SETUP_REF_SEL_INT_REF; fwnode_property_match_property_String(child, ... so only if it succeeds is the value overridden.
On 12/12/23 17:09, David Lechner wrote: > On Tue, Dec 12, 2023 at 11:45 AM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote: >> >> The AD7173 family offer a complete integrated Sigma-Delta ADC solution >> which can be used in high precision, low noise single channel applications >> or higher speed multiplexed applications. The Sigma-Delta ADC is intended >> primarily for measurement of signals close to DC but also delivers >> outstanding performance with input bandwidths out to ~10kHz. > > As stated in [1], we should try to make complete bindings. I think > more could be done here to make this more complete. Most notably, the > gpio-controller binding is missing. Also maybe something is needed to > describe how the SYNC/ERROR pin is wired up since it can be an input > or an output with different functions? > GPIO-controller: '#gpio-cells': const: 2 gpio-controller: true Like this, in properties? Sync can only be an output, Error is configurable. Are there any examples for how something like this is described? ... >> + interrupts: >> + maxItems: 1 > > Shouldn't this be 2? The datasheet says there is a "Data Output Ready" > signal on the DOUT/RDY pin and an "Error Output" on the SYNC/ERROR > pin. Although I could see how RDY could be considered part of the SPI > bus. In any case, a description explaining what the interrupt is would > be useful. > I do not see how there could be 2 interrupts. DOUT/RDY is used as an interrupt when waiting for a conversion to finalize. Sync and Error are sepparate pins, Sync(if enabled) works only as an input that resets the modulator and the digital filter. Error can be configured as input, output or ERROR output (OR between all internal error sources). Would this be alright interrupts: description: Conversion completion interrupt. Pin is shared with SPI DOUT. maxItems: 1 ... >> + >> +patternProperties: >> + "^channel@[0-9a-f]$": >> + type: object >> + $ref: adc.yaml >> + unevaluatedProperties: false >> + >> + properties: >> + reg: >> + minimum: 0 >> + maximum: 15 >> + >> + diff-channels: >> + items: >> + minimum: 0 >> + maximum: 31 > > Do we need to add overrides to limit the maximums for each compatible string? > Just to be sure, in the allOf section? If yes, is there any other more elegant method to obtain this behavior? ... >> + >> + required: >> + - reg >> + - diff-channels > > Individual analog inputs can be used as single-ended or in pairs as > differential, right? If so, diff-channels should not be required to > allow for single-ended use. > > And we would need to add something like a single-ended-channel > property to adc.yaml to allow mapping analog input pins to channels > similar to how diff-channels works, I think (I don't see anything like > that there already)? > > So maybe something like: > > oneOf: > - required: > single-ended-channel > - required: > diff-channels > All channels must specify 2 analog input sources, there is no input source wired by default to AVSS. In my opinion, there is no need to specify channels as single-ended because that would require a property that specifies the input that is wired to AVSS.
On 12/14/23 14:30, Jonathan Cameron wrote: > On Tue, 12 Dec 2023 12:44:36 +0200 > Dumitru Ceclan <mitrutzceclan@gmail.com> wrote: > >> The AD7173 family offer a complete integrated Sigma-Delta ADC solution >> which can be used in high precision, low noise single channel >> applications or higher speed multiplexed applications. The Sigma-Delta >> ADC is intended primarily for measurement of signals close to DC but also >> delivers outstanding performance with input bandwidths out to ~10kHz. >> >> Reviewed-by: Michael Walle <michael@walle.cc> # for gpio-regmap >> Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com> > Hi > > Given it seems like you'll be doing a v9, one quick comment from me below. > > Jonathan > >> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c >> new file mode 100644 >> index 000000000000..96918b24a10a >> --- /dev/null >> +++ b/drivers/iio/adc/ad7173.c >> @@ -0,0 +1,964 @@ > ... > >> +static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev) >> +{ > > ... > >> + >> + if (st->info->has_temp) { >> + chan_arr[chan_index] = ad7173_temp_iio_channel_template; >> + chan_st_priv = &channels_st_priv_arr[chan_index]; >> + chan_st_priv->ain = >> + AD7173_CH_ADDRESS(chan_arr[chan_index].channel, chan_arr[chan_index].channel2); >> + chan_st_priv->cfg.bipolar = false; >> + chan_st_priv->cfg.input_buf = true; >> + chan_st_priv->cfg.ref_sel = AD7173_SETUP_REF_SEL_INT_REF; >> + st->adc_mode |= AD7173_ADC_MODE_REF_EN; >> + >> + chan_index++; >> + } >> + >> + device_for_each_child_node(dev, child) { >> + chan = &chan_arr[chan_index]; >> + chan_st_priv = &channels_st_priv_arr[chan_index]; >> + ret = fwnode_property_read_u32_array(child, "diff-channels", >> + ain, ARRAY_SIZE(ain)); >> + if (ret) { >> + fwnode_handle_put(child); >> + return ret; >> + } >> + >> + if (ain[0] >= st->info->num_inputs || >> + ain[1] >= st->info->num_inputs) { >> + fwnode_handle_put(child); >> + return dev_err_probe(dev, -EINVAL, >> + "Input pin number out of range for pair (%d %d).\n", >> + ain[0], ain[1]); >> + } >> + >> + ret = fwnode_property_match_property_string(child, >> + "adi,reference-select", >> + ad7173_ref_sel_str, >> + ARRAY_SIZE(ad7173_ref_sel_str)); >> + >> + if (ret < 0) >> + ref_sel = AD7173_SETUP_REF_SEL_INT_REF; >> + else >> + ref_sel = ret; > Simpler pattern for properties with a default is not to check the error code. > > ref_sel = AD7173_SETUP_REF_SEL_INT_REF; > > fwnode_property_match_property_String(child, ... > > so only if it succeeds is the value overridden. Where exactly would the value be overridden, the function does not have an argument passed for the found index. The function is written to return either the found index or a negative error. The proposed pattern would just ignore the returned index and would always leave ref_sel to default. Am I missing something? I can see in the thread where it was introduced that you proposed: "Looking at the usecases I wonder if it would be better to pass in an unsigned int *ret which is only updated on a match?" But on the iio togreg branch that was suggested I could the function on, it does not have that parameter.
On Thu, Dec 14, 2023 at 02:57:35PM +0200, Ceclan Dumitru wrote: > On 12/14/23 14:30, Jonathan Cameron wrote: > > On Tue, 12 Dec 2023 12:44:36 +0200 > > Dumitru Ceclan <mitrutzceclan@gmail.com> wrote: ... > >> + ret = fwnode_property_match_property_string(child, > >> + "adi,reference-select", > >> + ad7173_ref_sel_str, > >> + ARRAY_SIZE(ad7173_ref_sel_str)); > >> + Redundant blank line. > >> + if (ret < 0) > >> + ref_sel = AD7173_SETUP_REF_SEL_INT_REF; > >> + else > >> + ref_sel = ret; > > Simpler pattern for properties with a default is not to check the error code. > > > > ref_sel = AD7173_SETUP_REF_SEL_INT_REF; > > > > fwnode_property_match_property_String(child, ... > > > > so only if it succeeds is the value overridden. > > Where exactly would the value be overridden, the function does not have an > argument passed for the found index. The function is written to return either > the found index or a negative error. > > The proposed pattern would just ignore the returned index and would always > leave ref_sel to default. Am I missing something? > > I can see in the thread where it was introduced that you proposed: > "Looking at the usecases I wonder if it would be better to pass in > an unsigned int *ret which is only updated on a match?" > > But on the iio togreg branch that was suggested I could the function on, it > does not have that parameter. Yeah, with the current API we can have one check (no 'else' branch): ref_sel = AD7173_SETUP_REF_SEL_INT_REF; ret = ... if (ret >= 0) ref_sel = ret; But your approach is good to me. ... It's always possible to change prototype, and now of course is the best time as all the users are provided in the single tree. That said, patches are welcome if this is what we want. (My proposal was to return index in case of no error, but at the same time leave it in the returned code, so it will be aligned with other match functions of fwnode. But this in either way will complicate the implementation. And I don't find critical to have if-else in each caller as some of them may do something different on the error case, when option is mandatory. In such cases we usually don't provide output if we know that an error condition occurs.
On Thu, Dec 14, 2023 at 1:43 PM Ceclan Dumitru <mitrutzceclan@gmail.com> wrote: > > > > On 12/12/23 17:09, David Lechner wrote: > > On Tue, Dec 12, 2023 at 11:45 AM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote: > >> > >> The AD7173 family offer a complete integrated Sigma-Delta ADC solution > >> which can be used in high precision, low noise single channel applications > >> or higher speed multiplexed applications. The Sigma-Delta ADC is intended > >> primarily for measurement of signals close to DC but also delivers > >> outstanding performance with input bandwidths out to ~10kHz. > > > > As stated in [1], we should try to make complete bindings. I think > > more could be done here to make this more complete. Most notably, the > > gpio-controller binding is missing. Also maybe something is needed to > > describe how the SYNC/ERROR pin is wired up since it can be an input > > or an output with different functions? > > > > GPIO-controller: > '#gpio-cells': > > const: 2 > > > gpio-controller: true > Like this, in properties? Yes (with not so many blank lines). > > Sync can only be an output, Error is configurable. Are there any > examples for how something like this is described? > Configurable pins sounds like a pinmux. Sounds a bit overkill to specify everything for a pin-controller for one pin if no one is ever going to use it. But I will leave it to the DT maintainers to say how complete the bindings should be. > ... > > >> + interrupts: > >> + maxItems: 1 > > > > Shouldn't this be 2? The datasheet says there is a "Data Output Ready" > > signal on the DOUT/RDY pin and an "Error Output" on the SYNC/ERROR > > pin. Although I could see how RDY could be considered part of the SPI > > bus. In any case, a description explaining what the interrupt is would > > be useful. > > > > I do not see how there could be 2 interrupts. DOUT/RDY is used as an > interrupt when waiting for a conversion to finalize. > > Sync and Error are sepparate pins, Sync(if enabled) works only as an > input that resets the modulator and the digital filter. I only looked at the AD7172-2 datasheet and pin 15 is labeled SYNC/ERROR. Maybe they are separate pins on other chips? > > Error can be configured as input, output or ERROR output (OR between all > internal error sources). > > Would this be alright > interrupts: > > description: Conversion completion interrupt. > Pin is shared with SPI DOUT. > maxItems: 1 Since ERROR is an output, I would expect it to be an interrupt. The RDY output, on the other hand, would be wired to a SPI controller with the SPI_READY feature (I use the Linux flag name here because I'm not aware of a corresponding devicetree flag). So I don't think the RDY signal would be an interrupt. > > ... > > >> + > >> +patternProperties: > >> + "^channel@[0-9a-f]$": > >> + type: object > >> + $ref: adc.yaml > >> + unevaluatedProperties: false > >> + > >> + properties: > >> + reg: > >> + minimum: 0 > >> + maximum: 15 > >> + > >> + diff-channels: > >> + items: > >> + minimum: 0 > >> + maximum: 31 > > > > Do we need to add overrides to limit the maximums for each compatible string? > > > > Just to be sure, in the allOf section? > If yes, is there any other more elegant method to obtain this behavior? I'm not sure. I would like to know if there is a more elegant way as well. ;-) > > ... > > >> + > >> + required: > >> + - reg > >> + - diff-channels > > > > Individual analog inputs can be used as single-ended or in pairs as > > differential, right? If so, diff-channels should not be required to > > allow for single-ended use. > > > > And we would need to add something like a single-ended-channel > > property to adc.yaml to allow mapping analog input pins to channels > > similar to how diff-channels works, I think (I don't see anything like > > that there already)? > > > > So maybe something like: > > > > oneOf: > > - required: > > single-ended-channel > > - required: > > diff-channels > > > All channels must specify 2 analog input sources, there is no input > source wired by default to AVSS. > > In my opinion, there is no need to specify channels as single-ended > because that would require a property that specifies the input that is > wired to AVSS. Makes sense to me.
On 12/14/23 18:12, David Lechner wrote: > On Thu, Dec 14, 2023 at 1:43 PM Ceclan Dumitru <mitrutzceclan@gmail.com> wrote: >> On 12/12/23 17:09, David Lechner wrote: >>> On Tue, Dec 12, 2023 at 11:45 AM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote: >> ... >> >>>> + interrupts: >>>> + maxItems: 1 >>> >>> Shouldn't this be 2? The datasheet says there is a "Data Output Ready" >>> signal on the DOUT/RDY pin and an "Error Output" on the SYNC/ERROR >>> pin. Although I could see how RDY could be considered part of the SPI >>> bus. In any case, a description explaining what the interrupt is would >>> be useful. >>> >> >> I do not see how there could be 2 interrupts. DOUT/RDY is used as an >> interrupt when waiting for a conversion to finalize. >> >> Sync and Error are sepparate pins, Sync(if enabled) works only as an >> input that resets the modulator and the digital filter. > > I only looked at the AD7172-2 datasheet and pin 15 is labeled > SYNC/ERROR. Maybe they are separate pins on other chips? Yep, sorry, missed that. All other supported models have them separate. > >> >> Error can be configured as input, output or ERROR output (OR between all >> internal error sources). >> >> Would this be alright >> interrupts: >> >> description: Conversion completion interrupt. >> Pin is shared with SPI DOUT. >> maxItems: 1 > > Since ERROR is an output, I would expect it to be an interrupt. The > RDY output, on the other hand, would be wired to a SPI controller with > the SPI_READY feature (I use the Linux flag name here because I'm not > aware of a corresponding devicetree flag). So I don't think the RDY > signal would be an interrupt. > Error does not have the purpose to be an interrupt. The only interrupt used from this chip is the one from the DOUT/~RDY pin. Sure, it is wired to the SPI controller, but when you can't also receive interrupts on that very same CPU pin an issue arises. So that pin is also wired to another GPIO with interrupt support. This is the same way that ad4130.yaml is written for example (with the exception that ad4130 supports configuring where the interrupt is routed). In regards to SPI_READY _BITUL(7) /* slave pulls low to pause */: the ad_sigma_delta framework (if it can be called that) is written to expect a pin interrupt, not to use SPI_READY controller feature.
On Thu, 14 Dec 2023 16:47:29 +0200 Andy Shevchenko <andy@kernel.org> wrote: > On Thu, Dec 14, 2023 at 02:57:35PM +0200, Ceclan Dumitru wrote: > > On 12/14/23 14:30, Jonathan Cameron wrote: > > > On Tue, 12 Dec 2023 12:44:36 +0200 > > > Dumitru Ceclan <mitrutzceclan@gmail.com> wrote: > > ... > > > >> + ret = fwnode_property_match_property_string(child, > > >> + "adi,reference-select", > > >> + ad7173_ref_sel_str, > > >> + ARRAY_SIZE(ad7173_ref_sel_str)); > > > >> + > > Redundant blank line. > > > >> + if (ret < 0) > > >> + ref_sel = AD7173_SETUP_REF_SEL_INT_REF; > > >> + else > > >> + ref_sel = ret; > > > Simpler pattern for properties with a default is not to check the error code. > > > > > > ref_sel = AD7173_SETUP_REF_SEL_INT_REF; > > > > > > fwnode_property_match_property_String(child, ... > > > > > > so only if it succeeds is the value overridden. > > > > Where exactly would the value be overridden, the function does not have an > > argument passed for the found index. The function is written to return either > > the found index or a negative error. > > > > The proposed pattern would just ignore the returned index and would always > > leave ref_sel to default. Am I missing something? > > > > I can see in the thread where it was introduced that you proposed: > > "Looking at the usecases I wonder if it would be better to pass in > > an unsigned int *ret which is only updated on a match?" > > > > But on the iio togreg branch that was suggested I could the function on, it > > does not have that parameter. > > Yeah, with the current API we can have one check (no 'else' branch): > > ref_sel = AD7173_SETUP_REF_SEL_INT_REF; > ret = ... > if (ret >= 0) > ref_sel = ret; > Yeah. I was clearly lacking in coffee or just being an idiot that day! > But your approach is good to me. > > ... > > It's always possible to change prototype, and now of course is the best time > as all the users are provided in the single tree. That said, patches are > welcome if this is what we want. (My proposal was to return index in case of > no error, but at the same time leave it in the returned code, so it will be > aligned with other match functions of fwnode. > > But this in either way will complicate the implementation. And I don't find > critical to have if-else in each caller as some of them may do something > different on the error case, when option is mandatory. In such cases we > usually don't provide output if we know that an error condition occurs. I'm fine with it being as it is. Was just having a slow brain day. J >
On Thu, 14 Dec 2023 19:03:28 +0200 Ceclan Dumitru <mitrutzceclan@gmail.com> wrote: > On 12/14/23 18:12, David Lechner wrote: > > On Thu, Dec 14, 2023 at 1:43 PM Ceclan Dumitru <mitrutzceclan@gmail.com> wrote: > >> On 12/12/23 17:09, David Lechner wrote: > >>> On Tue, Dec 12, 2023 at 11:45 AM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote: > > >> ... > >> > >>>> + interrupts: > >>>> + maxItems: 1 > >>> > >>> Shouldn't this be 2? The datasheet says there is a "Data Output Ready" > >>> signal on the DOUT/RDY pin and an "Error Output" on the SYNC/ERROR > >>> pin. Although I could see how RDY could be considered part of the SPI > >>> bus. In any case, a description explaining what the interrupt is would > >>> be useful. > >>> > >> > >> I do not see how there could be 2 interrupts. DOUT/RDY is used as an > >> interrupt when waiting for a conversion to finalize. > >> > >> Sync and Error are sepparate pins, Sync(if enabled) works only as an > >> input that resets the modulator and the digital filter. > > > > I only looked at the AD7172-2 datasheet and pin 15 is labeled > > SYNC/ERROR. Maybe they are separate pins on other chips? > > Yep, sorry, missed that. All other supported models have them separate. > > > >> > >> Error can be configured as input, output or ERROR output (OR between all > >> internal error sources). > >> > >> Would this be alright > >> interrupts: > >> > >> description: Conversion completion interrupt. > >> Pin is shared with SPI DOUT. > >> maxItems: 1 > > > > Since ERROR is an output, I would expect it to be an interrupt. The > > RDY output, on the other hand, would be wired to a SPI controller with > > the SPI_READY feature (I use the Linux flag name here because I'm not > > aware of a corresponding devicetree flag). So I don't think the RDY > > signal would be an interrupt. > > > > Error does not have the purpose to be an interrupt. The only interrupt > used from this chip is the one from the DOUT/~RDY pin. Sure, it is wired > to the SPI controller, but when you can't also receive interrupts on > that very same CPU pin an issue arises. So that pin is also wired to > another GPIO with interrupt support. You've lost me. It's a pin that has a state change when an error condition occurs. Why not an interrupt? Doesn't matter that the driver doesn't use this functionality. dt-bindings should be as comprehensive as possible. Given it's a multipurpose pin you'd also want to support it as a gpio to be complete alongside the other GPIOs. > > This is the same way that ad4130.yaml is written for example (with the > exception that ad4130 supports configuring where the interrupt is routed). > > In regards to SPI_READY _BITUL(7) /* slave pulls low to pause */: the > ad_sigma_delta framework (if it can be called that) is written to expect > a pin interrupt, not to use SPI_READY controller feature. SPI_READY is supported by only a couple of controllers. I'm not even that sure exactly how it is defined and whether that lines up with this usecase. From some old asci art. https://lore.kernel.org/all/1456747459-8559-1-git-send-email-linux@rempel-privat.de/ Flow control: Ready Sequence Master CS |-----1\_______________________| Slave FC |--------2\____________________| DATA |-----------3\_________________| So you set master and then wait for a flow control pin (the ready signal) before you can actually talk to the device. Here we are indicating data is ready to be be read out. So I don't 'think' SPI_READY applies. Jonathan >
On Sun, Dec 17, 2023 at 7:50 AM Jonathan Cameron <jic23@kernel.org> wrote: > > On Thu, 14 Dec 2023 19:03:28 +0200 > Ceclan Dumitru <mitrutzceclan@gmail.com> wrote: > > > On 12/14/23 18:12, David Lechner wrote: > > > On Thu, Dec 14, 2023 at 1:43 PM Ceclan Dumitru <mitrutzceclan@gmail.com> wrote: > > >> On 12/12/23 17:09, David Lechner wrote: > > >>> On Tue, Dec 12, 2023 at 11:45 AM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote: > > > > >> ... > > >> > > >>>> + interrupts: > > >>>> + maxItems: 1 > > >>> > > >>> Shouldn't this be 2? The datasheet says there is a "Data Output Ready" > > >>> signal on the DOUT/RDY pin and an "Error Output" on the SYNC/ERROR > > >>> pin. Although I could see how RDY could be considered part of the SPI > > >>> bus. In any case, a description explaining what the interrupt is would > > >>> be useful. > > >>> > > >> > > >> I do not see how there could be 2 interrupts. DOUT/RDY is used as an > > >> interrupt when waiting for a conversion to finalize. > > >> > > >> Sync and Error are sepparate pins, Sync(if enabled) works only as an > > >> input that resets the modulator and the digital filter. > > > > > > I only looked at the AD7172-2 datasheet and pin 15 is labeled > > > SYNC/ERROR. Maybe they are separate pins on other chips? > > > > Yep, sorry, missed that. All other supported models have them separate. > > > > > > > >> > > >> Error can be configured as input, output or ERROR output (OR between all > > >> internal error sources). > > >> > > >> Would this be alright > > >> interrupts: > > >> > > >> description: Conversion completion interrupt. > > >> Pin is shared with SPI DOUT. > > >> maxItems: 1 > > > > > > Since ERROR is an output, I would expect it to be an interrupt. The > > > RDY output, on the other hand, would be wired to a SPI controller with > > > the SPI_READY feature (I use the Linux flag name here because I'm not > > > aware of a corresponding devicetree flag). So I don't think the RDY > > > signal would be an interrupt. > > > > > > > Error does not have the purpose to be an interrupt. The only interrupt > > used from this chip is the one from the DOUT/~RDY pin. Sure, it is wired > > to the SPI controller, but when you can't also receive interrupts on > > that very same CPU pin an issue arises. So that pin is also wired to > > another GPIO with interrupt support. > > You've lost me. It's a pin that has a state change when an error condition > occurs. Why not an interrupt? Doesn't matter that the driver doesn't > use this functionality. dt-bindings should be as comprehensive as possible. > Given it's a multipurpose pin you'd also want to support it as a gpio to be > complete alongside the other GPIOs. > > > > > This is the same way that ad4130.yaml is written for example (with the > > exception that ad4130 supports configuring where the interrupt is routed). > > > > In regards to SPI_READY _BITUL(7) /* slave pulls low to pause */: the > > ad_sigma_delta framework (if it can be called that) is written to expect > > a pin interrupt, not to use SPI_READY controller feature. > > SPI_READY is supported by only a couple of controllers. I'm not even that > sure exactly how it is defined and whether that lines up with this usecase. > From some old asci art. https://lore.kernel.org/all/1456747459-8559-1-git-send-email-linux@rempel-privat.de/ > > Flow control: Ready Sequence > Master CS |-----1\_______________________| > Slave FC |--------2\____________________| > DATA |-----------3\_________________| > > So you set master and then wait for a flow control pin (the ready signal) before > you can actually talk to the device. > > Here we are indicating data is ready to be be read out. > > So I don't 'think' SPI_READY applies. > > Jonathan > I'm not arguing that SPI_READY applies in this particular case, but FWIW it does seem to me like... 1) SPI_READY could be implemented in any SPI driver using a GPIO interrupt (similar to how we already have GPIO CS) 2) In cases where the SPI controller does have actual hardware support for SPI_READY and the ADC chip A) uses CS to trigger a conversion and B) has a "busy" signal that goes low when the conversion is complete, then the SPI_READY feature could be used to make reading sample data more efficient by avoiding any CPU intervention between CS assertion and starting the data xfer due to waiting for the conversion to complete either by waiting for an interrupt or sleeping for a fixed amount of time.
On Sun, 17 Dec 2023 19:00:32 -0600 David Lechner <dlechner@baylibre.com> wrote: > On Sun, Dec 17, 2023 at 7:50 AM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Thu, 14 Dec 2023 19:03:28 +0200 > > Ceclan Dumitru <mitrutzceclan@gmail.com> wrote: > > > > > On 12/14/23 18:12, David Lechner wrote: > > > > On Thu, Dec 14, 2023 at 1:43 PM Ceclan Dumitru <mitrutzceclan@gmail.com> wrote: > > > >> On 12/12/23 17:09, David Lechner wrote: > > > >>> On Tue, Dec 12, 2023 at 11:45 AM Dumitru Ceclan <mitrutzceclan@gmail.com> wrote: > > > > > > >> ... > > > >> > > > >>>> + interrupts: > > > >>>> + maxItems: 1 > > > >>> > > > >>> Shouldn't this be 2? The datasheet says there is a "Data Output Ready" > > > >>> signal on the DOUT/RDY pin and an "Error Output" on the SYNC/ERROR > > > >>> pin. Although I could see how RDY could be considered part of the SPI > > > >>> bus. In any case, a description explaining what the interrupt is would > > > >>> be useful. > > > >>> > > > >> > > > >> I do not see how there could be 2 interrupts. DOUT/RDY is used as an > > > >> interrupt when waiting for a conversion to finalize. > > > >> > > > >> Sync and Error are sepparate pins, Sync(if enabled) works only as an > > > >> input that resets the modulator and the digital filter. > > > > > > > > I only looked at the AD7172-2 datasheet and pin 15 is labeled > > > > SYNC/ERROR. Maybe they are separate pins on other chips? > > > > > > Yep, sorry, missed that. All other supported models have them separate. > > > > > > > > > > > >> > > > >> Error can be configured as input, output or ERROR output (OR between all > > > >> internal error sources). > > > >> > > > >> Would this be alright > > > >> interrupts: > > > >> > > > >> description: Conversion completion interrupt. > > > >> Pin is shared with SPI DOUT. > > > >> maxItems: 1 > > > > > > > > Since ERROR is an output, I would expect it to be an interrupt. The > > > > RDY output, on the other hand, would be wired to a SPI controller with > > > > the SPI_READY feature (I use the Linux flag name here because I'm not > > > > aware of a corresponding devicetree flag). So I don't think the RDY > > > > signal would be an interrupt. > > > > > > > > > > Error does not have the purpose to be an interrupt. The only interrupt > > > used from this chip is the one from the DOUT/~RDY pin. Sure, it is wired > > > to the SPI controller, but when you can't also receive interrupts on > > > that very same CPU pin an issue arises. So that pin is also wired to > > > another GPIO with interrupt support. > > > > You've lost me. It's a pin that has a state change when an error condition > > occurs. Why not an interrupt? Doesn't matter that the driver doesn't > > use this functionality. dt-bindings should be as comprehensive as possible. > > Given it's a multipurpose pin you'd also want to support it as a gpio to be > > complete alongside the other GPIOs. > > > > > > > > This is the same way that ad4130.yaml is written for example (with the > > > exception that ad4130 supports configuring where the interrupt is routed). > > > > > > In regards to SPI_READY _BITUL(7) /* slave pulls low to pause */: the > > > ad_sigma_delta framework (if it can be called that) is written to expect > > > a pin interrupt, not to use SPI_READY controller feature. > > > > SPI_READY is supported by only a couple of controllers. I'm not even that > > sure exactly how it is defined and whether that lines up with this usecase. > > From some old asci art. https://lore.kernel.org/all/1456747459-8559-1-git-send-email-linux@rempel-privat.de/ > > > > Flow control: Ready Sequence > > Master CS |-----1\_______________________| > > Slave FC |--------2\____________________| > > DATA |-----------3\_________________| > > > > So you set master and then wait for a flow control pin (the ready signal) before > > you can actually talk to the device. > > > > Here we are indicating data is ready to be be read out. > > > > So I don't 'think' SPI_READY applies. > > > > Jonathan > > > > I'm not arguing that SPI_READY applies in this particular case, but > FWIW it does seem to me like... > > 1) SPI_READY could be implemented in any SPI driver using a GPIO > interrupt (similar to how we already have GPIO CS) > 2) In cases where the SPI controller does have actual hardware support > for SPI_READY and the ADC chip A) uses CS to trigger a conversion and > B) has a "busy" signal that goes low when the conversion is complete, > then the SPI_READY feature could be used to make reading sample data > more efficient by avoiding any CPU intervention between CS assertion > and starting the data xfer due to waiting for the conversion to > complete either by waiting for an interrupt or sleeping for a fixed > amount of time. Agreed that SPI_READY is possible if an SPI controller uses GPIOs for CS and that signal. If not a GPIO for CS then the SPI_READY should also be hardware managed. I could potentially be adapted to this sort of case if conditions like the CS being active before the ready is set is taking into account. This is a bit like SPI in general - far too many things that could be built and no particular standards for them. Jonathan
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml new file mode 100644 index 000000000000..25a5404ee353 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml @@ -0,0 +1,170 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +# Copyright 2023 Analog Devices Inc. +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/adi,ad7173.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Analog Devices AD7173 ADC + +maintainers: + - Ceclan Dumitru <dumitru.ceclan@analog.com> + +description: | + Bindings for the Analog Devices AD717X ADC's. Datasheets for supported chips: + https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/AD7175-2.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/AD7176-2.pdf + +properties: + compatible: + enum: + - adi,ad7172-2 + - adi,ad7173-8 + - adi,ad7175-2 + - adi,ad7176-2 + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + '#address-cells': + const: 1 + + '#size-cells': + const: 0 + + spi-max-frequency: + maximum: 20000000 + + refin-supply: + description: external reference supply, can be used as reference for conversion. + + refin2-supply: + description: external reference supply, can be used as reference for conversion. + + avdd-supply: + description: avdd supply, can be used as reference for conversion. + +patternProperties: + "^channel@[0-9a-f]$": + type: object + $ref: adc.yaml + unevaluatedProperties: false + + properties: + reg: + minimum: 0 + maximum: 15 + + diff-channels: + items: + minimum: 0 + maximum: 31 + + adi,reference-select: + description: | + Select the reference source to use when converting on + the specific channel. Valid values are: + refin : REFIN(+)/REFIN(−). + refin2 : REFIN2(+)/REFIN2(−) + refout-avss: REFOUT/AVSS (Internal reference) + avdd : AVDD + + External reference refin2 only available on ad7173-8. + If not specified, internal reference used. + enum: + - refin + - refin2 + - refout-avss + - avdd + default: refout-avss + + required: + - reg + - diff-channels + +required: + - compatible + - reg + - interrupts + +allOf: + - $ref: /schemas/spi/spi-peripheral-props.yaml# + + - if: + properties: + compatible: + not: + contains: + const: adi,ad7173-8 + then: + properties: + refin2-supply: false + patternProperties: + "^channel@[0-9a-f]$": + properties: + adi,reference-select: + enum: + - refin + - refout-avss + - avdd + +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,ad7173-8"; + reg = <0>; + + #address-cells = <1>; + #size-cells = <0>; + + interrupts = <25 IRQ_TYPE_EDGE_FALLING>; + interrupt-parent = <&gpio>; + spi-max-frequency = <5000000>; + + refin-supply = <&dummy_regulator>; + + channel@0 { + reg = <0>; + bipolar; + diff-channels = <0 1>; + adi,reference-select = "refin"; + }; + + channel@1 { + reg = <1>; + diff-channels = <2 3>; + }; + + channel@2 { + reg = <2>; + bipolar; + diff-channels = <4 5>; + }; + + channel@3 { + reg = <3>; + bipolar; + diff-channels = <6 7>; + }; + + channel@4 { + reg = <4>; + diff-channels = <8 9>; + adi,reference-select = "avdd"; + }; + }; + };
The AD7173 family offer a complete integrated Sigma-Delta ADC solution which can be used in high precision, low noise single channel applications or higher speed multiplexed applications. The Sigma-Delta ADC is intended primarily for measurement of signals close to DC but also delivers outstanding performance with input bandwidths out to ~10kHz. Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com> --- V7->V8 - include missing fix from V6 .../bindings/iio/adc/adi,ad7173.yaml | 170 ++++++++++++++++++ 1 file changed, 170 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml