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

[Victor Chernyakin]

> > +		/*
> > +		 * 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.

> 		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.

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