Hello Geert,
----- Original Message -----
> From: "Geert Uytterhoeven" <[email protected]>
> To: "Jeroen De Wachter" <[email protected]>
> Cc: "linux-spi" <[email protected]>, "thomas de schampheleire"
> <[email protected]>
> Sent: Tuesday, December 16, 2014 3:53:14 PM
> Subject: Re: right bit shift error in ad7314.c ?
>
> Hi Jeroen,
>
> On Tue, Dec 16, 2014 at 3:21 PM, Jeroen De Wachter
> <[email protected]> wrote:
> > That driver reads a status from the SPI bus that contains the temperature
> > read by the sensor. There's some other information in that status too, so
> > some bit manipulation is done to get the temperature data:
> >
> > s16 data;
> > int ret;
> > ...
> > data = (ret & AD7314_TEMP_MASK) >> AD7314_TEMP_SHIFT;
> > data = (data << 6) >> 6;
> >
> > Before that last line is executed, the temperature data is in the lowest
> > 10 bits of the data variable. To be able to handle negative temperatures,
> > those 10 bits get shifted to the left (discarding higher bits) and then
> > shifted back to the right, with what is assumed to be an Arithmetic Shift
> > Right, which takes into account the 2's complement content of the lowest
> > 10 bits.
> >
> > However, the implementation of right shift on a signed integer is not
> > defined in the C standard and is implementation-dependent [1].
>
> Have you tried using a signed division instead?
>
> The following seems to work for me:
>
> data = (s16)(data << 6) / 64;
>
> Without the cast, "data << 6" seems to be promoted to an unsigned type.
>
> Alternatively, you can split it in two parts, to force a signed intermediate
> result:
>
> data <<= 6;
> data /= 64;
>
Your code should have the same outcome as mine. I had a look at the generated
assembly though and it looks a lot more complicated:
data = (ret & AD7314_TEMP_MASK) >> AD7314_TEMP_OFFSET;
data = (s16)(data << 6) / 64;
154: e1a03083 lsl r3, r3, #1
return sprintf(buf, "%d\n", 250 * data);
158: e6bf3073 sxth r3, r3
15c: e283203f add r2, r3, #63 ; 0x3f
160: e3530000 cmp r3, #0
164: b1a03002 movlt r3, r2
168: e1a02343 asr r2, r3, #6
vs mine:
data = (ret & AD7314_TEMP_MASK) >> AD7314_TEMP_OFFSET;
148: e1a032a3 lsr r3, r3, #5
if (data & 0x0200) {
14c: e3130c02 tst r3, #512 ; 0x200
data |= 0xfc00;
150: 13833b3f orrne r3, r3, #64512 ; 0xfc00
I only looked at this on my ARM target (with gcc 4.4.3 and gcc 4.8.3 generating
pretty much the same assembly in both cases), so I don't know what the binary
would look like on other architectures...
There's a conditional instruction there in both solutions (orrne vs. movlt), so
that's not a reason to prefer one over the other.
I do think your version is slightly more readable, although both solutions could
do with a clarifying comment.
So: what do we go for? performance or readability? I don't have a strong
preference, but if I *have* to choose, I'd go with the bitwise manipulation...
Regards,
Jeroen
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
