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

[David Lechner]

On 7/11/26 8:00 AM, Andy Shevchenko wrote:
> 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.

iunit is scaling factor for *_mag, so logically gets grouped with
those. (These are all datasheet register field names.)

> 
>> +	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`.)

We've had the discussion before [1]. I don't think it is worth the effort for
structs that only get used once and doesn't have performance implications.

[1]: https://lore.kernel.org/linux-iio/20240318124041.0000032d@xxxxxxxxxx/

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