[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