Re: [PATCH v2 2/2] iio: pressure: ms5637: Add variant specific temperature compensation

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

On Tue, Jun 09, 2026 at 10:04:58PM -0400, Louis Adamian wrote:
> Add correct temperature compensation for ms5637-30BA, MS5803-01BA,02BA,
> 05BA, 14BA, 30BA, MS5837-30BA. The temperature compensation formula is
> shared across these sensors but with different constants. Add
> ms_tp_comp_consts to capture these per-device differences. Add pressure
> variant specific pressure scale variable.

Is there SPI driver? If so, why only i2c is affected?

...

> +/* apply second order temperature compensation */
> +static void ms_tp_compensate(const struct ms_tp_comp_consts *c,
> +			     s32 temp, s32 dt, s64 *t2, s64 *off2, s64 *sens2)
> +{
> +	if (temp < 2000) {
> +		s64 tmp = (s64)temp - 2000;

Why casting?

> +
> +		*t2 = (c->low_t2_multiplier * ((s64)dt * (s64)dt)) >>
> +		      c->low_t2_shift;
> +		*off2 = (c->low_off2_multiplier * tmp * tmp) >>
> +			c->low_off2_shift;
> +		*sens2 = (c->low_sens2_multiplier * tmp * tmp) >>
> +			 c->low_sens2_shift;


> +		if (temp < -1500) {
> +			s64 tmp_vlow = (s64)temp + 1500;

Missing blank line. But Q here is why tmp can't be used here?

> +			*off2 += c->vlow_off2_multiplier * tmp_vlow * tmp_vlow;
> +			*sens2 +=
> +				c->vlow_sens2_multiplier * tmp_vlow * tmp_vlow;

Make it one line.

> +		}
> +	} else {
> +		*sens2 = 0;
> +		if (c->has_vhigh_temp && temp > 4500)
> +			*sens2 -= (((s64)temp - 4500) * ((s64)temp - 4500)) >> 3;

Here...

> +		*t2 = (c->high_t2_multiplier * ((s64)dt * (s64)dt)) >>
> +		      c->high_t2_shift;
> +		*off2 = (c->high_off2_multiplier *
> +			 ((s64)temp - 2000) * ((s64)temp - 2000)) >>

...and here you may also use tmp, just make it global to the function.

> +			c->high_off2_shift;
> +	}
> +}

Overall this all needs a good comment or even comments to explain all
calculations with the references to the respective sections / tables / pages
in the datasheet.

...

> int ms_sensors_read_temp_and_pressure(struct ms_tp_dev *dev_data,

>  	s32 dt, temp;
>  	s64 off, sens, t2, off2, sens2;
>  	u16 *prom = dev_data->prom, delay;
> +	const struct ms_tp_comp_consts *c = dev_data->comp_consts;

Try to keep more or less reversed xmas tree order.

...

>   * struct ms_tp_dev - Temperature/Pressure sensor device structure
>   * @client:	i2c client

>   * @prom:	array of PROM coefficients used for conversion. Added element
>   *              for CRC computation
>   * @res_index:	index to selected sensor resolution
> + * @comp_consts: temperature compensation constants
>   */
>  struct ms_tp_dev {
>  	struct i2c_client *client;

>  	const struct ms_tp_hw_data *hw;
>  	u16 prom[MS_SENSORS_TP_PROM_WORDS_NB];
>  	u8 res_index;
> +	const struct ms_tp_comp_consts *comp_consts;

Please, check with `pahole` if this is the best layout.

>  };

>  struct ms_tp_data {
>  	const char *name;
>  	const struct ms_tp_hw_data *hw;
> +	const struct ms_tp_comp_consts *comp_consts;
>  };

Can this be simply embedded into ms_tp_dev (and copied there if required)?

...

>  		case IIO_PRESSURE:	/* in kPa */
> -			*val = pressure / 1000;
> -			*val2 = (pressure % 1000) * 1000;
> +			*val = pressure / dev_data->comp_consts->press_scale;
> +			*val2 = (pressure %
> +				 (s64)dev_data->comp_consts->press_scale) *
> +				(1000000 / dev_data->comp_consts->press_scale);

MICRO (might need units.h)?

>  			return IIO_VAL_INT_PLUS_MICRO;

...

Have you considered to prepare the infrastructure in one patch and add
the actual compensation data tables in another?

-- 
With Best Regards,
Andy Shevchenko