Re: [PATCH v3 6/9] iio: adc: ti-ads1262: support excitation currents

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] IDAC excitation currents will not work because the ADC's internal voltage reference is never enabled.
- [Low] Build failure due to incompatible pointer types when passing 'ads1262_idac_mags_nA' to 'ads1262_find_one()'.
--

--- Patch [6]: [PATCH v3 6/9] iio: adc: ti-ads1262: support excitation currents ---
commit 62f1b6149a86f48a45e748f65baba909e175b7ee
Author: Kurt Borja <[email protected]>

iio: adc: ti-ads1262: support excitation currents

This commit adds support for two IDAC excitation current sources in the
ti-ads1262 driver. It allows routing IDAC1 and IDAC2 to specific pins and
configuring their magnitudes via device tree properties.

> diff --git a/drivers/iio/adc/ti-ads1262.c b/drivers/iio/adc/ti-ads1262.c
> --- a/drivers/iio/adc/ti-ads1262.c
> +++ b/drivers/iio/adc/ti-ads1262.c
[ ... ]
> @@ -456,6 +482,22 @@ static int ads1262_channel_enable(struct ads1262 *st,
>  	if (ret)
>  		return ret;
>  
> +	val = FIELD_PREP(ADS1262_IDACMUX_MUX1_MASK, chan->idac_mux[0]) |
> +	      FIELD_PREP(ADS1262_IDACMUX_MUX2_MASK, chan->idac_mux[1]);
> +	ret = regmap_update_bits(st->regmap, ADS1262_IDACMUX_REG,
> +				 ADS1262_IDACMUX_MUX1_MASK |
> +				 ADS1262_IDACMUX_MUX2_MASK, val);
> +	if (ret)
> +		return ret;
> +
> +	val = FIELD_PREP(ADS1262_IDACMAG_MAG1_MASK, chan->idac_mag[0]) |
> +	      FIELD_PREP(ADS1262_IDACMAG_MAG2_MASK, chan->idac_mag[1]);
> +	ret = regmap_update_bits(st->regmap, ADS1262_IDACMAG_REG,
> +				 ADS1262_IDACMAG_MAG1_MASK |
> +				 ADS1262_IDACMAG_MAG2_MASK, val);

[Severity: High]
Will the IDAC excitation currents function without the internal voltage
reference being enabled?

The driver configures the IDAC multiplexer and magnitude here in
ads1262_channel_enable(), but the ADC hardware physically requires the
internal reference (INTREF bit in the POWER register) to be active for the
current sources to output current. It appears this bit is never set in the
driver initialization or channel enable paths, leaving the internal
reference disabled by default.

[ ... ]
> @@ -1150,7 +1192,58 @@ static int ads1262_parse_channel_node(struct ads1262 *st,
[ ... ]
> +		ret = ads1262_find_one(ads1262_idac_mags_nA,
> +				       ARRAY_SIZE(ads1262_idac_mags_nA), mags[0]);

[Severity: Low]
Could this result in a build failure?

The ads1262_idac_mags_nA array is declared earlier in the file as a
const u32 array, but the ads1262_find_one() function explicitly expects a
const int pointer argument.

Passing an unsigned pointer where a signed pointer is expected triggers a
-Wincompatible-pointer-types warning. In modern kernel builds, this warning
is promoted to a fatal error and breaks the build.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6