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

[Maxwell Doose]

On Mon, May 11, 2026 at 7:00 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Fri,  8 May 2026 17:55:00 -0500
> Maxwell Doose <m32285159@xxxxxxxxx> wrote:
>
> > 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@xxxxxxxxx>
>
> This is good but we can take this opportunity to make it more
> idiomatic - so more 'standard' for new kernel code.
>

Yea that all sounds good. TBH I'd like to get some of the kernel dev
beginners to check out this driver since in some of my earlier emails
I said it was messy (+ I'd like to get it cleaned up) and there are
probably a metric ton of low hanging fruit.

>
> > ---
> >  ps:
> >  Hi Jonathan, I noticed a potential divide-by-zero bug on line 241 in
> >  scd30_read_raw(), where the value of tmp is dictated by hardware.
> >  If the scd30_command_read() call on line 236 assigns 0 to tmp, then
> >  when we run:
> >  *val2 = 1000000000 / tmp;
> >  we'll get a divide-by-zero. Will send a patch for this later.
> Good to know but not sure why you'd put that comment in this patch.
> Also as a fix, it should come first - ah so maybe you were indicating it
> might cause a merge issue?
>

Well now that you told me that the div-by-zero in the write path was
already fixed my v2 would cause a conflict, shouldn't be a problem if
I revert it and add some of the other fixes into it.

>
> >
> >  best regards,
> >  max
> >
> >  drivers/iio/chemical/scd30_core.c | 13 +++++++++----
> >  1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iio/chemical/scd30_core.c b/drivers/iio/chemical/scd30_core.c
> > index a665fcb78806..be8c055be184 100644
> > --- a/drivers/iio/chemical/scd30_core.c
> > +++ b/drivers/iio/chemical/scd30_core.c
> > @@ -89,10 +89,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 = float32 & GENMASK(22, 0);
>         int mantissa = FIELD_GET(GENMASK(22, 0), float32);
>
> though maybe worth some field defines as well.
> //up by defines at top of file.
> #define SCD30_FLOAT_MANTISSA_MSK GENMASK(22, 0)
>
>         int mantissa = FIELD_GET(SCD30_FLOAT_MANTISSA_MSK, float32);
>
>
> > +     int exp = (float32 & ~BIT(31)) >> 23;
> Again with a field define this can become something like:
>
>
> //up by defines at top of file - and check carefully I got this right!
> #define SCD30_FLOAT_EXPONENT_MSK GENMASK(30, 23)
>
>         int exp = FIELD_GET(SCD30_FLOAT_EXPONENT_MSK, float32);
>
> > +
> > +     /* Determine sign of received float based on IEEE 754 standard */
> > +     if (float32 & BIT(31))
>         if (FIELD_GET(SCD30_FLOAT_SIGN, float32))
>
> > +             sign = -1;
> > +     else
> > +             sign = 1;
> >
> >       /* special case 0 */
> >       if (!exp && !mantissa)
> > --
> > 2.54.0
>
>

I'll add those macros for the v2. Also will probably add a comment
saying something along the lines of:

/* IEEE 754 floating point arithmetic macros */

best regards,
max