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

I suppose, but I would prefer to avoid duplicating the macro, and
I think having _MSB here helps to confirm what the comment is saying,
that we should be reading MSB first.

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

Since we're waiting for an answer, I've posted v4 without this change
so that the other parts of the patch can move forward, but I can
always add it to v5, etc.