Re: [PATCH v3 4/4] iio: cdc: add Texas Instruments FDC1004 driver

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

On Mon, Oct 26, 2026 at 09:13:25PM -0500, Victor Chernyakin wrote:

...

> > > +		/*
> > > +		 * Now read the two registers making up the measurement.
> > > +		 * The datasheet says to read MSB first, LSB second.
> > > +		 */
> > > +		ret = regmap_bulk_read(map, FDC1004_REG_MEASn_MSB(0),
> > > +				       reg_vals, ARRAY_SIZE(reg_vals));
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		*val = (sign_extend32(reg_vals[0], 15) << 8) | (reg_vals[1] >> 8);
> > 
> > We still have two shift operators, so it might be not a bad idea after all to
> > have a combined variable with wordswap done in case of LITTLE_ENDIAN
> > architecture. Jonathan, what do you think about this? Reading the whole set as
> > 32-bit value and convert it if required
> > 
> > 		// also note in this case we don't need to have _MSB in the name
> > 		ret = regmap_bulk_read(map, FDC1004_REG_MEASn(0), reg_val, sizeof(reg_val));
> 
> I agree we could drop the _MSB if this was the only use
> of FDC1004_REG_MEASn_MSB, but we use both _MSB and _LSB macros in the
> volatile register table definition, and it would be weird to have one
> macro with _LSB but the other without _MSB.

You can have three!

> > 		if (ret)
> > 			return ret;
> > 
> > #ifdef __LITTLE_ENDIAN
> > 		reg_val = swahw32(reg_val);
> > #endif
> > 
> > 		*val = sign_extend32(reg_val >> 8, 23);
> > 
> > 		// I assume the following is also possible in this case
> > 		// *val = (int)reg_val >> 8;
> > 		// but usually we use sign_extend32() in IIO even when simple
> > 		// casting is okay
> >
> > This also reduces stack usage.
> 
> AFAIK a good optimizer should be able to see that reg_val and reg_vals are
> never live at the same time and reuse the same stack slot for both (whether
> GCC or Clang actually do that for this function, I can't be bothered to check).
> 
> If you and Jonathan prefer that version for other reasons though, I can
> switch to it.

I gave an idea, I would wait for Jonathan to judge on it.

...

> > > +static const int fdc1004_calibbias_decomposed[] = {
> > > +	lower_32_bits(fdc1004_calibbias[0]), upper_32_bits(fdc1004_calibbias[0]),
> > > +	lower_32_bits(fdc1004_calibbias[1]), upper_32_bits(fdc1004_calibbias[1]),
> > > +	lower_32_bits(fdc1004_calibbias[2]), upper_32_bits(fdc1004_calibbias[2]),
> > > +};
> > > +
> > > +static const int fdc1004_calibscale_decomposed[] = {
> > > +	lower_32_bits(fdc1004_calibscale[0]), upper_32_bits(fdc1004_calibscale[0]),
> > > +	lower_32_bits(fdc1004_calibscale[1]), upper_32_bits(fdc1004_calibscale[1]),
> > > +	lower_32_bits(fdc1004_calibscale[2]), upper_32_bits(fdc1004_calibscale[2]),
> > > +};
> > 
> > We still haven't decided on this one. I think the 2D array is better for these.
> > And with the above we effectively have a duplication of the tables.
> 
> I don't like the duplication either, but I think the real problem here is
> that read_avail is just a somewhat awkward interface that doesn't allow me
> to return s64 values directly. To me, having this separate decomposed table
> just for read_avail and otherwise working with s64 values is the least bad
> approach.

What about adding .read_avail64() (in a separate change)? Problem solved!

-- 
With Best Regards,
Andy Shevchenko