Re: [PATCH v12 05/11] iio: core: add decimal value formatting into 64-bit value

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Sun, May 10, 2026 at 01:42:23PM +0100, Rodrigo Alencar via B4 Relay wrote:

> Create new format types for iio values (IIO_VAL_DECIMAL64_*), which
> defines the representation of fixed decimal point values into a single
> 64-bit number. This new format increases the range of represented values,
> allowing for integer parts greater than 2^32, as bits are not "wasted"
> in the fractional part, which can be seen in IIO_VAL_INT_PLUS_MICRO and
> IIO_VAL_INT_PLUS_NANO. Helpers are created to compose and decompose 64-bit
> decimals into integer values used in IIO formatting interfaces, which
> creates consistency and avoid error-prone manual assignments when using
> wordpart macros. When doing the parsing, kstrtodec64() is used with the
> scale defined by the specific decimal format type.

...

> +	case IIO_VAL_DECIMAL64_MILLI:
> +	case IIO_VAL_DECIMAL64_MICRO:
> +	case IIO_VAL_DECIMAL64_NANO:
> +	case IIO_VAL_DECIMAL64_PICO:
> +	{
> +		s64 frac;
> +		unsigned int scale = type - IIO_VAL_DECIMAL64_BASE;

Can we stick with reversed xmas tree order?

> +		tmp2 = div64_s64_rem(iio_val_s64_from_array(vals),
> +				     int_pow(10, scale), &frac);
> +		if (tmp2 == 0 && frac < 0)
> +			return sysfs_emit_at(buf, offset, "-0.%0*lld", scale,
> +					     abs(frac));
> +		else
> +			return sysfs_emit_at(buf, offset, "%lld.%0*lld", tmp2,
> +					     scale, abs(frac));
> +	}

What about

		/* Print a leading '-' for negative fractions */
		if (tmp2 == 0 && frac < 0)
			offset += sysfs_emit_at(buf, offset, "-");

		return sysfs_emit_at(buf, offset, "%lld.%0*lld", tmp2, scale, abs(frac));

Also note this won't work with the frac that are == S64_MIN. It's UB (undefined
behaviour), see the comment at abs() implementation. Maybe a time to add abs()
corner case tests...

...

>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> -	int ret, fract_mult = 100000;
> +	int type, ret, fract_mult = 100000, dec_scale = 0;

I wouldn't mix ret here and put it...

>  	int integer, fract = 0;
>  	long long integer64;
>  	bool is_char = false;

...as standalone here

	int ret;


...

> +#include <linux/wordpart.h>

+ blank line.

>  #include <uapi/linux/iio/types.h>

...

>  #define IIO_VAL_FRACTIONAL_LOG2 11
>  #define IIO_VAL_CHAR 12
>  
> +#define IIO_VAL_DECIMAL64_BASE		100

Okay, but I would rather see something smaller like 32 or 64.

...

> +static inline s64 iio_val_s64_compose(int val0, int val1)

Hmm... s64 composed form two int:s...

> +{
> +	return (s64)(((u64)val1 << 32) | (u32)val0);
> +}
> +
> +static inline s64 iio_val_s64_from_array(const int *vals)

When I see 'array' in the name, I think of real array and some index. Here is
no index available. Perhaps

static inline s64 iio_val_s64_from_s32s(const s32 *vals)

> +{
> +	return iio_val_s64_compose(vals[0], vals[1]);
> +}
> +
> +static inline void iio_val_s64_decompose(s64 dec64, int *val0, int *val1)
> +{
> +	*val0 = lower_32_bits(dec64);
> +	*val1 = upper_32_bits(dec64);
> +}
> +
> +static inline void iio_val_s64_array_populate(s64 dec64, int *vals)

_to_array() or _to_s32s()

> +{
> +	iio_val_s64_decompose(dec64, &vals[0], &vals[1]);
> +}

-- 
With Best Regards,
Andy Shevchenko