Re: [PATCH RFC 3/4] iio: adc: ad7380: add support for adaq4370-4 and adaq4380-4

Jonathan Cameron
2024-10-20T13:18:42Z

On Tue, 15 Oct 2024 11:09:08 +0200
Julien Stephan <jstephan@baylibre.com> wrote:

> adaq4370-4 (2MSPS) and adaq4380-4 (4MSPS) are quad-channel precision data
> acquisition signal chain μModule solutions compatible with the ad738x
> family, with the following differences:
>
> - configurable gain in front of each 4 adc
> - internal reference is 3V derived from refin-supply (5V)
> - additional supplies
>
> This implies that IIO_CHAN_INFO_SCALE can not be shared by type.
>
> Signed-off-by: Julien Stephan <jstephan@baylibre.com>
Not that much code, so I'll give it a quick review even though I know
you posted it for that one specific dt binding question.

Main thing here is I'd drop the scale from dt bindings and make it writable
via write_raw. You'll also need to compute the appropriate _available
stuff so userspace knows what scales it can use.

Jonathan

> @@ -876,8 +933,15 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
> * * (2 × VREF) / 2^N, for differential chips
> * * VREF / 2^N, for pseudo-differential chips
> * where N is the ADC resolution (i.e realbits)
> + *
> + * The gain is stored as a fraction of 1000 and, as we need to
> + * divide vref_mv by the gain, we invert the gain/1000 fraction.
> */
> - *val = st->vref_mv;
> + if (st->chip_info->has_hardware_gain)
> + *val = mult_frac(st->vref_mv, MILLI,
> + st->gain_milli[chan->scan_index]);
> + else
> + *val = st->vref_mv;
> *val2 = scan_type->realbits - chan->differential;
>
> return IIO_VAL_FRACTIONAL_LOG2;
> @@ -1058,7 +1122,19 @@ static int ad7380_probe(struct spi_device *spi)
> "Failed to enable power supplies\n");
> msleep(T_POWERUP_MS);
>
> - if (st->chip_info->external_ref_only) {
> + if (st->chip_info->adaq_internal_ref_only) {
> + /*
> + * ADAQ chips use fixed internal reference but still
> + * require an external reference supply to power it.
I'd just go with
* require a specific external supply to power it.

Reference kind of implies it is a quality supply. This is 4.5-5.5V
so not so much ;)

> + * "refin" is already enabled with other power supplies
> + * in bulk_get_enable().
> + */
> +
> + st->vref_mv = ADAQ4380_INTERNAL_REF_MV;
> +
> + /* these chips don't have a register bit for this */
> + external_ref_en = false;
> + } else if (st->chip_info->external_ref_only) {
> ret = devm_regulator_get_enable_read_voltage(&spi->dev,
> "refin");
> if (ret < 0)
> @@ -1104,6 +1180,34 @@ static int ad7380_probe(struct spi_device *spi)
> st->vcm_mv[i] = ret / 1000;
> }
>
> + for (i = 0; i < MAX_NUM_CHANNELS; i++)
> + st->gain_milli[i] = AD7380_DEFAULT_GAIN_MILLI;
> +
> + if (st->chip_info->has_hardware_gain) {

Why is this a DT thing rather than exposed to userspace?
Mostly we only control ranges in DT for output devices (where there is a chance
of burning things if we let them be controlled from userspace.).
For ADC amplifiers we tend to just expose them as controllable _SCALE.

> + device_for_each_child_node_scoped(&spi->dev, node) {
> + unsigned int channel, gain;
> +
> + ret = fwnode_property_read_u32(node, "reg", &channel);
> + if (ret)
> + return dev_err_probe(&spi->dev, ret,
> + "Failed to read reg property\n");
> +
> + if (channel >= st->chip_info->num_channels - 1)
> + return dev_err_probe(&spi->dev, -EINVAL,
> + "Invalid channel number %i\n",
> + channel);
> +
> + ret = fwnode_property_read_u32(node, "adi,gain-milli",
> + &gain);
> + if (ret && ret != -EINVAL)
> + return dev_err_probe(&spi->dev, ret,
> + "Failed to read gain for channel %i\n",
> + channel);
> + if (ret != -EINVAL)
> + st->gain_milli[channel] = gain;
> + }
> + }