Re: [PATCH v2] iio: chemical: scd30: Cleanup initializations in scd30_float_to_fp()

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 25 May 2026 08:49:17 +0200
Joshua Crofts <joshua.crofts1@gmail.com> wrote:

> On Sun, 24 May 2026 at 04:04, Maxwell Doose <m32285159@gmail.com> wrote:
> >
> > Include linux/bitfield.h for FIELD_GET().
> >
> > Create new macros for bit manipulation in combination with manual bit
> > manipulation being replaced with FIELD_GET().
> >
> > The current variable declaration and initializations are barely readable
> > and use comma separations across multiple lines. Refactor the
> > initializations so that mantissa and exp have separate declarations and
> > sign gets initialized later.  
> 
> > Signed-off-by: Maxwell Doose <m32285159@gmail.com>  

Sashiko review of this one is really interesting.  If you fancy doing a bit
of digging it would be good to verify that it is correct on the original
code giving a very wrong answer on 64 bit platforms.

I poked the compiler explorer (https://godbolt.org/) with and without -m32
passed to 64bit gcc. Looks like sashiko is right to me.  However given the way
it is used I think we are in undefined behaviour territory so it might
'work' unless the compiler is feel malicious.

Upshot, verify it for your own understanding and then add a fixes tag
for wherever that issue came from.

https://sashiko.dev/#/patchset/20260524020309.18618-1-m32285159%40gmail.com
> 
> Not really sure, but the title of the commit could be better? Perhaps something
> along the lines of "simplify floating point bit arithmetic". Usually you don't
> need to mention the function name in the title since it can be found
> in the diff.
> 
> > @@ -89,10 +96,15 @@ static int scd30_reset(struct scd30_state *state)
> >  /* simplified float to fixed point conversion with a scaling factor of 0.01 */
> >  static int scd30_float_to_fp(int float32)
> >  {
> > -       int fraction, shift,
> > -           mantissa = float32 & GENMASK(22, 0),
> > -           sign = (float32 & BIT(31)) ? -1 : 1,
> > -           exp = (float32 & ~BIT(31)) >> 23;
> > +       int fraction, shift, sign;
> > +       int mantissa = FIELD_GET(SCD30_FLOAT_MANTISSA_MSK, float32);
> > +       int exp = FIELD_GET(SCD30_FLOAT_EXP_MSK, float32);
> > +
> > +       /* Determine sign of received float based on IEEE 754 standard */  
> 
> IMO this is an unnecessary comment.
> 
> Codewise OK.
> 
> Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>
>