| Message ID | 20231123152331.5751-1-user@HYB-hhAwRlzzMZb |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v6,1/2] dt-bindings: adc: add AD7173 | expand |
On Thu, 23 Nov 2023 17:23:22 +0200 mitrutzceclan <mitrutzceclan@gmail.com> wrote: > From: Dumitru Ceclan <mitrutzceclan@gmail.com> > > 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. A few minor things from me. thanks, Jonathan > new file mode 100644 > index 000000000000..ca307d406cee > --- /dev/null > +++ b/drivers/iio/adc/ad7173.c > @@ -0,0 +1,962 @@ ... > + > +static int ad7173_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long info) > +{ > + struct ad7173_state *st = iio_priv(indio_dev); > + struct ad7173_channel *ch = &st->channels[chan->address]; > + unsigned int reg; > + int ret; > + > + switch (info) { > + case IIO_CHAN_INFO_RAW: > + ret = ad_sigma_delta_single_conversion(indio_dev, chan, val); > + if (ret < 0) > + return ret; > + > + /* disable channel after single conversion */ > + ret = ad_sd_write_reg(&st->sd, AD7173_REG_CH(chan->address), 2, 0); > + if (ret < 0) > + return ret; > + > + return IIO_VAL_INT; > + case IIO_CHAN_INFO_SCALE: > + if (chan->type == IIO_TEMP) { > + *val = 250000000; > + *val2 = 800273203; /* (2^24 * 477) / 10 */ > + return IIO_VAL_FRACTIONAL; > + } else { > + *val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel); > + *val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar); > + return IIO_VAL_FRACTIONAL_LOG2; > + } > + case IIO_CHAN_INFO_OFFSET: > + if (chan->type == IIO_TEMP) { > + *val = -874379; > + } else { > + if (ch->cfg.bipolar) > + *val = -BIT(chan->scan_type.realbits - 1); > + else > + *val = 0; Ideally if the channel isn't bipolar, don't expose the offset at all. Default assumption if there is no such file is 0 offset. If that's fiddly to do then fine to keep the file though. > + } > + return IIO_VAL_INT; > + case IIO_CHAN_INFO_SAMP_FREQ: > + reg = st->channels[chan->address].cfg.odr; > + > + *val = st->info->sinc5_data_rates[reg] / MILLI; > + *val2 = (st->info->sinc5_data_rates[reg] % MILLI) * (MICRO/MILLI); > + > + return IIO_VAL_INT_PLUS_MICRO; > + } > + return -EINVAL; ... > +static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev) > +{ > + struct ad7173_channel *channels_st_priv_arr, *chan_st_priv; > + struct ad7173_state *st = iio_priv(indio_dev); > + struct device *dev = indio_dev->dev.parent; > + struct iio_chan_spec *chan_arr, *chan; > + struct fwnode_handle *child; > + unsigned int ain[2], chan_index = 0; > + unsigned int num_channels; > + const char *ref_label; > + u32 ref_sel; > + int i, ret; > + > + st->regulators[0].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF]; > + st->regulators[1].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF2]; > + st->regulators[2].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_AVDD1_AVSS]; > + > + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(st->regulators), > + st->regulators); If only some of them are supplied, the driver works fine as long as no channel is using them? If so should probably get the optionally then check for availability of the ones you want. I'm sure someone will just wire up ref. Incidentally, ref2 isn't there for all chips I think, so we should not get it on the ones where it can't exist. > + if (ret) > + return dev_err_probe(dev, ret, "Failed to get regulators\n"); > + > + ret = regulator_bulk_enable(ARRAY_SIZE(st->regulators), st->regulators); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to enable regulators\n"); > + > + ret = devm_add_action_or_reset(dev, ad7173_disable_regulators, st); > + if (ret) > + return dev_err_probe(dev, ret, > + "Failed to add regulators disable action\n"); > + > + num_channels = device_get_child_node_count(dev); > + > + if (st->info->has_temp) > + num_channels++; > + > + if (num_channels == 0) > + return 0; Isn't it all a bit pointless if this happens? So maybe an error makes more sense? > + st->num_channels = num_channels; > + > + chan_arr = devm_kcalloc(dev, sizeof(*chan_arr), num_channels, GFP_KERNEL); > + if (!chan_arr) > + return -ENOMEM; > + > + channels_st_priv_arr = devm_kcalloc(dev, num_channels, > + sizeof(*channels_st_priv_arr), > + GFP_KERNEL); > + if (!channels_st_priv_arr) > + return -ENOMEM; > + > + indio_dev->channels = chan_arr; > + indio_dev->num_channels = num_channels; > + st->channels = channels_st_priv_arr; > + > + 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).", ain[0], ain[1]); > + } > + > + ref_sel = AD7173_SETUP_REF_SEL_INT_REF; > + ret = fwnode_property_read_string(child, "adi,reference-select", &ref_label); > + if (!ret) { > + for (i = 0; i < ARRAY_SIZE(ad7173_ref_sel_str); i++) > + if (strcmp(ref_label, ad7173_ref_sel_str[i]) == 0) { > + ref_sel = i; > + break; > + } > + > + if (i == ARRAY_SIZE(ad7173_ref_sel_str)) Missing fwnode_handle_put(child). Same for other error paths in th device_for_each_child_node() loop. I wonder if that can get cleaned up with the cleanup.h magic. Anyhow, that's a question for another day. > + return dev_err_probe(dev, -EINVAL, "Invalid channel reference name %s", ref_label); > + } else if (ret != -EINVAL) { Up to you, but you could be lazier and set ref_label = ref_sel_str[AD7173_SETUP_REF_SEL_INT_REF] before trying to read it then don't bother handling the fwnode property read error at all That's common practice when there is a default. It would unnecessarily run the string match when the property wasn't there, but that's not too bad for the code simplification it would give I think > + return dev_err_probe(dev, ret, "Invalid channel reference value"); > + } > + > + ret = ad7173_get_ref_voltage_milli(st, (u8)ref_sel); > + if (ret < 0) > + return dev_err_probe(dev, ret, > + "Cannot use reference %u", ref_sel); > + if (ref_sel == AD7173_SETUP_REF_SEL_EXT_REF2 && > + st->info->id != AD7173_ID) > + return dev_err_probe(dev, -EINVAL, "External reference 2 is only available on ad7173-8"); Why all the way down here? I'd do this a few lines earlier so we haven't already used the bonkers value. > + if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF) > + st->adc_mode |= AD7173_ADC_MODE_REF_EN; > + chan_st_priv->cfg.ref_sel = ref_sel; > + > + *chan = ad7173_channel_template; > + chan->address = chan_index; > + chan->scan_index = chan_index; > + chan->channel = ain[0]; > + chan->channel2 = ain[1]; > + chan->differential = true; > + > + chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]); > + chan_st_priv->chan_reg = chan_index; > + chan_st_priv->cfg.input_buf = true; > + chan_st_priv->cfg.odr = 0; > + > + chan_st_priv->cfg.bipolar = fwnode_property_read_bool(child, "bipolar"); > + if (chan_st_priv->cfg.bipolar) > + chan->info_mask_separate |= BIT(IIO_CHAN_INFO_OFFSET); > + > + chan_index++; > + } > + > + return 0; > +} > + > +static int ad7173_probe(struct spi_device *spi) > +{ > + struct ad7173_state *st; > + struct iio_dev *indio_dev; > + struct device *dev = &spi->dev; > + int ret; > + > + indio_dev = devm_iio_device_alloc(dev, sizeof(*st)); > + if (!indio_dev) > + return -ENOMEM; > + > + st = iio_priv(indio_dev); > + st->info = device_get_match_data(dev); > + if (!st->info) > + return -ENODEV; > + > + ida_init(&st->cfg_slots_status); > + ret = devm_add_action_or_reset(dev, ad7173_ida_destroy, st); > + if (ret) > + return ret; > + > + indio_dev->name = st->info->name; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->info = &ad7173_info; > + > + spi->mode = SPI_MODE_3; > + > + ad7173_sigma_delta_info.num_slots = st->info->num_configs; > + ret = ad_sd_init(&st->sd, indio_dev, spi, &ad7173_sigma_delta_info); > + if (ret) > + return ret; > + > + spi_set_drvdata(spi, indio_dev); Used? I'm not immediately seeing where. If not, don't set it. Thanks, Jonathan
On 11/23/23 20:02, Conor Dooley wrote: > On Thu, Nov 23, 2023 at 05:23:21PM +0200, mitrutzceclan wrote: >> +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 > > I assume you tested that this restriction works as intended? > Yes, for both reference and reference-select
On 11/25/23 19:21, Jonathan Cameron wrote: > On Thu, 23 Nov 2023 17:23:22 +0200 > mitrutzceclan <mitrutzceclan@gmail.com> wrote: > >> From: Dumitru Ceclan <mitrutzceclan@gmail.com> ... >> + st->regulators[0].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF]; >> + st->regulators[1].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF2]; >> + st->regulators[2].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_AVDD1_AVSS]; >> + >> + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(st->regulators), >> + st->regulators); > > If only some of them are supplied, the driver works fine as long as no channel is using them? > If so should probably get the optionally then check for availability of the ones you > want. I'm sure someone will just wire up ref. Incidentally, ref2 isn't there for all chips > I think, so we should not get it on the ones where it can't exist. > This sets a dummy regulator in place if no proper supply is found. Then the call regulator_get_voltage() on the dummy will fail. About getting ref2, sure, I'll set the string only with the right ID. ... >> + ret = ad7173_get_ref_voltage_milli(st, (u8)ref_sel); >> + if (ret < 0) >> + return dev_err_probe(dev, ret, >> + "Cannot use reference %u", ref_sel); Here the probe would not continue if a channel selects a supply that is not available in the DT.
On Mon, 4 Dec 2023 18:49:21 +0200 Ceclan Dumitru <mitrutzceclan@gmail.com> wrote: > On 11/25/23 19:21, Jonathan Cameron wrote: > > On Thu, 23 Nov 2023 17:23:22 +0200 > > mitrutzceclan <mitrutzceclan@gmail.com> wrote: > > > >> From: Dumitru Ceclan <mitrutzceclan@gmail.com> > > ... > >> + st->regulators[0].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF]; > >> + st->regulators[1].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF2]; > >> + st->regulators[2].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_AVDD1_AVSS]; > >> + > >> + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(st->regulators), > >> + st->regulators); > > > > If only some of them are supplied, the driver works fine as long as no channel is using them? > > If so should probably get the optionally then check for availability of the ones you > > want. I'm sure someone will just wire up ref. Incidentally, ref2 isn't there for all chips > > I think, so we should not get it on the ones where it can't exist. > > > > > This sets a dummy regulator in place if no proper supply is found. Then > the call regulator_get_voltage() on the dummy will fail. About getting > ref2, sure, I'll set the string only with the right ID. > > ... > >> + ret = ad7173_get_ref_voltage_milli(st, (u8)ref_sel); > >> + if (ret < 0) > >> + return dev_err_probe(dev, ret, > >> + "Cannot use reference %u", ref_sel); > > Here the probe would not continue if a channel selects a supply that is > not available in the DT. Ok. It's a little ugly though - so maybe a comment at the bulk_get to say it will fail cleanly a bit later.
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..82e870122553 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml @@ -0,0 +1,178 @@ +# 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: | + Analog Devices AD717X ADC's. + + The AD717x family offer a complete integrated Sigma-Delta ADC solution which + can be used in high precision, low noise single channel applications (Life + Science measurements) or higher speed multiplexed applications (Factory Automation + PLC Input modules). 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. + + 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 + + bipolar: + type: boolean + + 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>; + + channel@0 { + reg = <0>; + bipolar; + diff-channels = <0 1>; + }; + + 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>; + }; + }; + };