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

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

[Maxwell Doose]

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>
---
v2:
 - Added new floating point macro constants per Jonathan's suggestion.
 - Included linux/bitfield.h to use FIELD_GET() since Jonathan also
   recommended its use.

 drivers/iio/chemical/scd30_core.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/chemical/scd30_core.c b/drivers/iio/chemical/scd30_core.c
index 8dbba9a7c426..db5cc295aeab 100644
--- a/drivers/iio/chemical/scd30_core.c
+++ b/drivers/iio/chemical/scd30_core.c
@@ -4,6 +4,8 @@
  *
  * Copyright (c) 2020 Tomasz Duszynski <tomasz.duszynski@octakon.com>
  */
+
+#include <linux/bitfield.h>
 #include <linux/bits.h>
 #include <linux/cleanup.h>
 #include <linux/completion.h>
@@ -43,6 +45,11 @@
 #define SCD30_TEMP_OFFSET_MAX 655360
 #define SCD30_EXTRA_TIMEOUT_PER_S 250
 
+/* Floating point arithmetic macros */
+#define SCD30_FLOAT_MANTISSA_MSK GENMASK(22, 0)
+#define SCD30_FLOAT_EXP_MSK GENMASK(30, 23)
+#define SCD30_FLOAT_SIGN_MSK BIT(31)
+
 enum {
 	SCD30_CONC,
 	SCD30_TEMP,
@@ -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 */
+	if (float32 & SCD30_FLOAT_SIGN_MSK)
+		sign = -1;
+	else
+		sign = 1;
 
 	/* special case 0 */
 	if (!exp && !mantissa)
-- 
2.54.0

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

[Joshua Crofts]

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>

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>

-- 
Kind regards

CJD

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

[Jonathan Cameron]

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

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

[Maxwell Doose]

On Tue, May 26, 2026 at 9:38 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> 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
>

Woof, this'll be quite a doozy to look into. I'm no good in terms of
understanding compilers (I ought to do a deeper dive into stuff like
that at some point), but I guess compiler explorer is sufficient for
now (note that I'm also no good at understanding assembly). I think
(and correct me if I'm wrong) that under the hood, BIT(n) does (1UL <<
n) and since an unsigned long is 64 bits on 64-bit architectures,
float32 gets sign extended to 0xFFFFFFFF7FFFFFFF which is a problem.

Regardless, will send a v3 with fixes, reported-by, and closes tags (I
guess sashiko technically reported it) as well as an explanation for
the fixes tag.

best regards,
max