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

[Jonathan Cameron <jic23@kernel.org>]

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

To esoteric for me.  We'd have to add comments on why it worked to limit
the future blast radius.  Now, what could we do that didn't need that...
The problem is that the regmap_bulk_read is feeding us two native endian
16 bit values.  Well, you could 'fix' that...

		u8 be_temp[4];

		ret = regmap_bulk_read(map, FDC1004_REG_MEAS_n_MSB(0),
				       reg_vals, ARRAY_SIZE(reg_vals));
		put_unaligned_be16(reg_vals[0], &be_temp[0]);
		put_unaligned_be16(reg_vals[1], &be_temp[2]);
		*val = sign_extgend32(get_unaligned_be32(be_temp) >> 8, 23);

Or something along those lines...  I'd rather something explicit like that
than Andy's suggestion, but I'm not convinced on this either.

> 
> > +		return IIO_VAL_INT;
> > +	}

Jonathan