Re: [PATCH] iio: proximity: sx9360: fix sampling frequency write and available range

[Andy Shevchenko]

On Sat, Oct 24, 2026 at 11:31:43PM +0530, Arnav Kapoor wrote:
> GNRL_CTRL1/2 hold the scan period in units of 8192 / FOSC, but
> sx9360_set_samp_freq() computes freq * 8192 / FOSC and mis-scales val2,
> which is in micro units. Any integer frequency then programs a period
> of 0, and fractional parts above ~0.26 Hz overflow the int, writing
> 0x10 to GNRL_CTRL1 outside the SCANPERIOD field.
> 
> Compute FOSC / (freq * 8192), rounded to the closest value, and reject
> frequencies outside the 1..2047 period range.
> 
> The available range doesn't match the limits in the comments next to it
> either: 4 MHz / 8192 is 488.28125 Hz, not 448.28125 Hz,
> 4 MHz / (8192 * 2047) is 0.238535 Hz, and (max - min) / 2047 is
> 0.238419 Hz. Use those values.

...

> static const struct {
>  	int val;
>  	int val2;
>  } sx9360_samp_freq_interval[] = {
> -	{ 0, 281250 },  /* 4MHz / (8192 * 2047) */
> -	{ 0, 281250 },
> -	{ 448, 281250 },  /* 4MHz / 8192 */
> +	{ 0, 238535 },  /* 4MHz / (8192 * 2047) */
> +	{ 0, 238419 },
> +	{ 488, 281250 },  /* 4MHz / 8192 */
>  };

At some point this should be converted to double array of int:s.

...

>  static int sx9360_set_samp_freq(struct sx_common_data *data,
>  				int val, int val2)
>  {
> -	int reg;
> +	unsigned int period;

What's the units for period? Seconds?

> +	u64 freq_uhz;

_uHz

>  	__be16 buf;
>  
> -	reg = val * 8192 / SX9360_FOSC_HZ + val2 * 8192 / (SX9360_FOSC_MHZ);
> -	buf = cpu_to_be16(reg);
> +	if (val < 0 || val2 < 0)
> +		return -EINVAL;
> +
> +	freq_uhz = (u64)val * MICRO + val2;

Read what units.h has for units of Hz.

> +	if (!freq_uhz)
> +		return -EINVAL;
> +
> +	/* The register holds the scan period, in units of 8192 / FOSC. */
> +	period = DIV64_U64_ROUND_CLOSEST((u64)SX9360_FOSC_HZ * MICRO,
> +					 freq_uhz * 8192);

Ditto.

> +	if (!period || period > SX9360_REG_GNRL_SCANPERIOD_MAX)
> +		return -EINVAL;
> +
> +	buf = cpu_to_be16(period);

Missing blank line. Always make guard()() to be standalone in the code.

>  	guard(mutex)(&data->mutex);
>  
>  	return regmap_bulk_write(data->regmap, SX9360_REG_GNRL_CTRL1, &buf,

-- 
With Best Regards,
Andy Shevchenko