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

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

[Maxwell Doose]

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

 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 exp = (float32 & ~BIT(31)) >> 23;
+
+	/* Determine sign of received float based on IEEE 754 standard */
+	if (float32 & BIT(31))
+		sign = -1;
+	else
+		sign = 1;

 	/* special case 0 */
 	if (!exp && !mantissa)
--
2.54.0

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

[Sanjay Chitroda]

On 9 May 2026 4:25:00 am IST, Maxwell Doose <m32285159@gmail.com> 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@gmail.com>
>---
> 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.
>
> 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 exp = (float32 & ~BIT(31)) >> 23;
>+
>+	/* Determine sign of received float based on IEEE 754 standard */

>+	if (float32 & BIT(31))
>+		sign = -1;
>+	else
>+		sign = 1;
Hi,

Thank you for the refactor change.

The previous version was more compact and readable. Splitting the variable declarations and expanding the sign assignment into an if/else block does not improve clarity significantly.

You can keep it simpler like:

sign = (float32 & BIT(31)) ? -1 : 1;

The IEEE 754 representation is already implicit from the bit manipulation, so additional comment is probably unnecessary.

Thanks, Sanjay
>
> 	/* special case 0 */
> 	if (!exp && !mantissa)
>--
>2.54.0
>

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

[Maxwell Doose]

On Fri, May 8, 2026 at 9:56 PM Sanjay Chitroda
<sanjayembeddedse@gmail.com> wrote:
>
> Thank you for the refactor change.
>
> The previous version was more compact and readable. Splitting the variable declarations and expanding the sign assignment into an if/else block does not improve clarity significantly.
>

Are you sure? I find it to be far more readable than the comma mess
that there was before, plus not a huge fan of the ternary operator in
such expressions. One of the goals I have as the new maintainer [1] of
the scd30 driver is to get rid of "clever" code and replace it with
more explicit and readable code. It's going to get optimized by the
compiler anyways. As a wise man once said,

“Programs must be written for people to read, and only incidentally
for machines to execute.”

Plus if we need to drop a printk() or whatever in there to see the
variable assignment we can just add {} and add the printk(), no
messing with the ternary required.

>
> You can keep it simpler like:
>
> sign = (float32 & BIT(31)) ? -1 : 1;
>

See above.

>
> The IEEE 754 representation is already implicit from the bit manipulation, so additional comment is probably unnecessary.
>

I'll see if Jonathan will want to remove that during merge or if I
need to send a v2 (likely not).

best regards,
max

[1] ("MAINTAINERS: Add myself as SCD30 maintainer"):
https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=togreg

> Thanks, Sanjay
> >
> >       /* special case 0 */
> >       if (!exp && !mantissa)
> >--
> >2.54.0
> >

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

[Andy Shevchenko]

On Fri, May 08, 2026 at 05:55:00PM -0500, Maxwell Doose 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.

The change LGTM,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

...

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

Have you checked the Clang and GCC compiled code in that cases?
Do they put anything interesting in the binary? (I.o.w. can they prove
the division-by-zero at compile-time?)

See the (somehow) related case in the 7e5c0f97c66a ("iio: adc: nxp-sar-adc:
Avoid division by zero"), which is in Linux Next only (for now).

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sat, May 09, 2026 at 07:27:38AM -0500, Maxwell Doose wrote:
> On Fri, May 8, 2026 at 9:56 PM Sanjay Chitroda
> <sanjayembeddedse@gmail.com> wrote:
> >
> > Thank you for the refactor change.
> >
> > The previous version was more compact and readable. Splitting the variable
> > declarations and expanding the sign assignment into an if/else block does
> > not improve clarity significantly.
> 
> Are you sure? I find it to be far more readable than the comma mess
> that there was before, plus not a huge fan of the ternary operator in
> such expressions. One of the goals I have as the new maintainer [1] of
> the scd30 driver is to get rid of "clever" code and replace it with
> more explicit and readable code. It's going to get optimized by the
> compiler anyways. As a wise man once said,
> 
> “Programs must be written for people to read, and only incidentally
> for machines to execute.”
> 
> Plus if we need to drop a printk() or whatever in there to see the
> variable assignment we can just add {} and add the printk(), no
> messing with the ternary required.

Sanjay, the comma operator is discouraged in the kernel. For this part I'm
fully on the Maxwell's side. As for if-else versus ternary it depends on the
maintainer, but in general if-else is considered better. (For instance, I'm
judging on case-by-case basis, and in some cases ternary makes a lot of sense.)
So, in this case I have no strong opinion about sign assignment.

> > You can keep it simpler like:
> >
> > sign = (float32 & BIT(31)) ? -1 : 1;
> 
> See above.
> 
> > The IEEE 754 representation is already implicit from the bit manipulation,
> > so additional comment is probably unnecessary.

Since we don't do floats in the kernel (all arithmetics is integer-based)
some might find the comment useful. But here as well, no strong opinion.

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On Fri,  8 May 2026 17:55:00 -0500
Maxwell Doose <m32285159@gmail.com> 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@gmail.com>

This is good but we can take this opportunity to make it more
idiomatic - so more 'standard' for new kernel code.

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

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@kernel.org> wrote:
>
> On Fri,  8 May 2026 17:55:00 -0500
> Maxwell Doose <m32285159@gmail.com> 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@gmail.com>
>
> 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