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

[Jonathan Cameron <jic23@kernel.org>]

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;
> +		}
> +	}