Re: [PATCH v3 8/8] iio: adc: ti-ads112c14: add measurement channel support

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Fri, Jul 10, 2026 at 05:50:41PM -0500, David Lechner (TI) wrote:
> Add support for parsing devicetree properties for measurement channels
> and doing direct reads on these.
> 
> There are quite a lot of conditions that have to be met for each
> measurement to be made, so quite a bit of state and algorithms are
> required to handle it.
> 
> Channels are created dynamically since the number of possibilities is
> unreasonably large.

...

> +struct ads112c14_measurement {
> +	const char *label;
> +	u32 vref_source;
> +	u8 iunit;
> +	u8 idac1_mag;
> +	u8 idac2_mag;
> +	u8 idac1_mux;
> +	u8 idac2_mux;

I would group this slightly differently:

	u8 idac1_mag;
	u8 idac2_mag;
	u8 idac1_mux;
	u8 idac2_mux;
	u8 iunit;

I haven't seen the code, but names suggest that most likely one would read
*[12] together or close enough, and less probably mixed with 'iunit' reads.
Current layout might lead to interesting code generation complications on
the unaligned-intolerable architectures.

> +	u8 iadc_count;
> +	u8 gain_val;
> +	u8 burnout;
> +	bool global_chop;
> +	bool bipolar;

> +	s64 scale_available[ARRAY_SIZE(ads112c14_pga_gains_x10)];

Also this can be moved upper, but I think it won't save any bytes in this
layout.

> +};

...

>  struct ads112c14_data {
>  	const struct ads112c14_chip_info *chip_info;
>  	struct regmap *regmap;
> +	u32 avdd_uV;
> +	u32 ext_ref_uV;

> +	bool refp_is_avdd;
> +	bool refn_is_gnd;

Here is a definite gain in memory if booleans combined with u8 below

> +	u32 ext_ref_ohms;
> +	struct ads112c14_measurement *measurements;
> +	u32 num_measurements;

Something like

	struct ads112c14_measurement *measurements;
	u32 num_measurements;
	u32 avdd_uV;
	u32 ext_ref_uV;
	u32 ext_ref_ohms;
	bool refp_is_avdd;
	bool refn_is_gnd;

? (Don't forget to run `pahole`.)

>  	u8 sys_mon_chan_short_gain_val;
>  	s64 sys_mon_chan_short_scale_available[ARRAY_SIZE(ads112c14_pga_gains_x10)];
>  };

...

> +	return regmap_update_bits(data->regmap, ADS112C14_REG_REFERENCE_CFG,
> +				  ADS112C14_REFERENCE_CFG_REFP_BUF_EN |
> +				  ADS112C14_REFERENCE_CFG_REFN_BUF_EN |
> +				  ADS112C14_REFERENCE_CFG_REF_VAL |
> +				  ADS112C14_REFERENCE_CFG_REF_SEL,
> +				  FIELD_PREP(ADS112C14_REFERENCE_CFG_REFP_BUF_EN,
> +					     refp_buf_en) |
> +				  FIELD_PREP(ADS112C14_REFERENCE_CFG_REFN_BUF_EN,
> +					     refn_buf_en) |
> +				  FIELD_PREP(ADS112C14_REFERENCE_CFG_REF_VAL,
> +					     ref_val) |
> +				  FIELD_PREP(ADS112C14_REFERENCE_CFG_REF_SEL,
> +					     ref_sel));

Personally I would go over 80 here.

				  FIELD_PREP(ADS112C14_REFERENCE_CFG_REFP_BUF_EN, refp_buf_en) |
				  FIELD_PREP(ADS112C14_REFERENCE_CFG_REFN_BUF_EN, refn_buf_en) |
				  FIELD_PREP(ADS112C14_REFERENCE_CFG_REF_VAL, ref_val) |
				  FIELD_PREP(ADS112C14_REFERENCE_CFG_REF_SEL, ref_sel));

All are less than 100.

> +}

...

> +	/* measurement channels */
> +	if (chan->channel < ADS112C14_SYS_MON_CHANNEL_BASE) {
> +		struct ads112c14_measurement *measurement;
> +
> +		measurement = &data->measurements[chan->scan_index];

> +

I would drop this blank line. It's naturally looking to have them coupled.

> +		if (!measurement->label)
> +			return -EINVAL;
> +
> +		return sysfs_emit(label, "%s\n", measurement->label);
> +	}

...

> +			if (pair[0] <= 100000 && (measurement->iadc_count == 1 || pair[1] <= 100000)) {

100 * (NANO / MICRO)

in the similar way how you done elsewhere in the code.

> +				/*
> +				 * If both values are 100uA or less, then we can
> +				 * use IUNIT = 1uA for better precision.
> +				 */
> +				ret = ads112c14_populate_idac_mag(pair[0],
> +								  &measurement->idac1_mag);
> +				if (ret)
> +					return ret;
> +
> +				if (measurement->iadc_count > 1) {
> +					ret = ads112c14_populate_idac_mag(pair[1],
> +									  &measurement->idac2_mag);
> +					if (ret)
> +						return ret;
> +				}
> +			} else {
> +				/*
> +				 * Otherwise, IUINT is 10uA (flag set) and so
> +				 * IxMAG is 1/10 of the actual current.
> +				 */
> +				measurement->iunit = 1;
> +
> +				ret = ads112c14_populate_idac_mag(pair[0] / 10,
> +								  &measurement->idac1_mag);
> +				if (ret)
> +					return ret;
> +
> +				if (measurement->iadc_count > 1) {
> +					ret = ads112c14_populate_idac_mag(pair[1] / 10,
> +									  &measurement->idac2_mag);
> +					if (ret)
> +						return ret;
> +				}
> +			}
> +		}

-- 
With Best Regards,
Andy Shevchenko