[PATCH v3 0/4] iio: pressure: dps310: support negative temperature values

[PATCH v3 0/4] iio: pressure: dps310: support negative temperature values

[Thomas Haemmerle]

This patch set fixes the reading of negative temperatures (returned in
millidegree celsius). As this requires a change of the error handling
other functions are aligned with this.
In addition a small code simplification for reading the scale factors
for temperature and pressure is included.

---
Changes in v2:
 - include fixes tag
 - Split up patch
 - introduce variables for intermediate results in functions
 - simplify scale factor reading

Changes in v3:
 - fix locking issues reported in https://lore.kernel.org/all/cbdafb33-fd3b-47ad-a678-83fa92475278@moroto.mountain/

Thomas Haemmerle (4):
  iio: pressure: dps310: support negative temperature values
  iio: pressure: dps310: introduce consistent error handling
  iio: pressure: dps310: consistently check return value of
    `regmap_read`
  iio: pressure: dps310: simplify scale factor reading

 drivers/iio/pressure/dps310.c | 138 +++++++++++++++++++---------------
 1 file changed, 77 insertions(+), 61 deletions(-)


base-commit: 0bbac3facb5d6cc0171c45c9873a2dc96bea9680
--
2.34.1

[PATCH v3 1/4] iio: pressure: dps310: support negative temperature values

[Thomas Haemmerle]

The current implementation interprets negative values returned from
`dps310_calculate_temp` as error codes.
This has a side effect that when negative temperature values are
calculated, they are interpreted as error.

Fix this by using the return value only for error handling and passing a
pointer for the value.

Fixes: ba6ec48e76bc ("iio: Add driver for Infineon DPS310")
Signed-off-by: Thomas Haemmerle <thomas.haemmerle@leica-geosystems.com>
---
 drivers/iio/pressure/dps310.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
index 1ff091b2f764..d0a516d56da4 100644
--- a/drivers/iio/pressure/dps310.c
+++ b/drivers/iio/pressure/dps310.c
@@ -730,7 +730,7 @@ static int dps310_read_pressure(struct dps310_data *data, int *val, int *val2,
 	}
 }
 
-static int dps310_calculate_temp(struct dps310_data *data)
+static int dps310_calculate_temp(struct dps310_data *data, int *val)
 {
 	s64 c0;
 	s64 t;
@@ -746,7 +746,9 @@ static int dps310_calculate_temp(struct dps310_data *data)
 	t = c0 + ((s64)data->temp_raw * (s64)data->c1);
 
 	/* Convert to milliCelsius and scale the temperature */
-	return (int)div_s64(t * 1000LL, kt);
+	*val = (int)div_s64(t * 1000LL, kt);
+
+	return 0;
 }
 
 static int dps310_read_temp(struct dps310_data *data, int *val, int *val2,
@@ -768,11 +770,10 @@ static int dps310_read_temp(struct dps310_data *data, int *val, int *val2,
 		if (rc)
 			return rc;
 
-		rc = dps310_calculate_temp(data);
-		if (rc < 0)
+		rc = dps310_calculate_temp(data, val);
+		if (rc)
 			return rc;
 
-		*val = rc;
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
-- 
2.34.1

[PATCH v3 2/4] iio: pressure: dps310: introduce consistent error handling

[Thomas Haemmerle]

Align error handling with `dps310_calculate_temp`, where it's not
possible to differentiate between errors and valid calculations by
checking if the returned value is negative.

Signed-off-by: Thomas Haemmerle <thomas.haemmerle@leica-geosystems.com>
---
 drivers/iio/pressure/dps310.c | 129 +++++++++++++++++++---------------
 1 file changed, 72 insertions(+), 57 deletions(-)

diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
index d0a516d56da4..3fc436f36fa7 100644
--- a/drivers/iio/pressure/dps310.c
+++ b/drivers/iio/pressure/dps310.c
@@ -256,24 +256,24 @@ static int dps310_startup(struct dps310_data *data)
 	return dps310_temp_workaround(data);
 }
 
-static int dps310_get_pres_precision(struct dps310_data *data)
+static int dps310_get_pres_precision(struct dps310_data *data, int *val)
 {
-	int rc;
-	int val;
+	int reg_val, rc;
 
-	rc = regmap_read(data->regmap, DPS310_PRS_CFG, &val);
+	rc = regmap_read(data->regmap, DPS310_PRS_CFG, &reg_val);
 	if (rc < 0)
 		return rc;
 
-	return BIT(val & GENMASK(2, 0));
+	*val = BIT(reg_val & GENMASK(2, 0));
+
+	return 0;
 }
 
-static int dps310_get_temp_precision(struct dps310_data *data)
+static int dps310_get_temp_precision(struct dps310_data *data, int *val)
 {
-	int rc;
-	int val;
+	int reg_val, rc;
 
-	rc = regmap_read(data->regmap, DPS310_TMP_CFG, &val);
+	rc = regmap_read(data->regmap, DPS310_TMP_CFG, &reg_val);
 	if (rc < 0)
 		return rc;
 
@@ -281,7 +281,9 @@ static int dps310_get_temp_precision(struct dps310_data *data)
 	 * Scale factor is bottom 4 bits of the register, but 1111 is
 	 * reserved so just grab bottom three
 	 */
-	return BIT(val & GENMASK(2, 0));
+	*val = BIT(reg_val & GENMASK(2, 0));
+
+	return 0;
 }
 
 /* Called with lock held */
@@ -350,48 +352,56 @@ static int dps310_set_temp_samp_freq(struct dps310_data *data, int freq)
 				  DPS310_TMP_RATE_BITS, val);
 }
 
-static int dps310_get_pres_samp_freq(struct dps310_data *data)
+static int dps310_get_pres_samp_freq(struct dps310_data *data, int *val)
 {
-	int rc;
-	int val;
+	int reg_val, rc;
 
-	rc = regmap_read(data->regmap, DPS310_PRS_CFG, &val);
+	rc = regmap_read(data->regmap, DPS310_PRS_CFG, &reg_val);
 	if (rc < 0)
 		return rc;
 
-	return BIT((val & DPS310_PRS_RATE_BITS) >> 4);
+	*val = BIT((reg_val & DPS310_PRS_RATE_BITS) >> 4);
+
+	return 0;
 }
 
-static int dps310_get_temp_samp_freq(struct dps310_data *data)
+static int dps310_get_temp_samp_freq(struct dps310_data *data, int *val)
 {
-	int rc;
-	int val;
+	int reg_val, rc;
 
-	rc = regmap_read(data->regmap, DPS310_TMP_CFG, &val);
+	rc = regmap_read(data->regmap, DPS310_TMP_CFG, &reg_val);
 	if (rc < 0)
 		return rc;
 
-	return BIT((val & DPS310_TMP_RATE_BITS) >> 4);
+	*val = BIT((reg_val & DPS310_TMP_RATE_BITS) >> 4);
+
+	return 0;
 }
 
-static int dps310_get_pres_k(struct dps310_data *data)
+static int dps310_get_pres_k(struct dps310_data *data, int *val)
 {
-	int rc = dps310_get_pres_precision(data);
+	int reg_val, rc;
 
-	if (rc < 0)
+	rc = dps310_get_pres_precision(data, &reg_val);
+	if (rc)
 		return rc;
 
-	return scale_factors[ilog2(rc)];
+	*val = scale_factors[ilog2(reg_val)];
+
+	return 0;
 }
 
-static int dps310_get_temp_k(struct dps310_data *data)
+static int dps310_get_temp_k(struct dps310_data *data, int *val)
 {
-	int rc = dps310_get_temp_precision(data);
+	int reg_val, rc;
 
-	if (rc < 0)
+	rc = dps310_get_temp_precision(data, &reg_val);
+	if (rc)
 		return rc;
 
-	return scale_factors[ilog2(rc)];
+	*val = scale_factors[ilog2(reg_val)];
+
+	return 0;
 }
 
 static int dps310_reset_wait(struct dps310_data *data)
@@ -464,7 +474,10 @@ static int dps310_read_pres_raw(struct dps310_data *data)
 	if (mutex_lock_interruptible(&data->lock))
 		return -EINTR;
 
-	rate = dps310_get_pres_samp_freq(data);
+	rc = dps310_get_pres_samp_freq(data, &rate);
+	if (rc)
+		goto done;
+
 	timeout = DPS310_POLL_TIMEOUT_US(rate);
 
 	/* Poll for sensor readiness; base the timeout upon the sample rate. */
@@ -510,7 +523,10 @@ static int dps310_read_temp_raw(struct dps310_data *data)
 	if (mutex_lock_interruptible(&data->lock))
 		return -EINTR;
 
-	rate = dps310_get_temp_samp_freq(data);
+	rc = dps310_get_temp_samp_freq(data, &rate);
+	if (rc)
+		goto done;
+
 	timeout = DPS310_POLL_TIMEOUT_US(rate);
 
 	/* Poll for sensor readiness; base the timeout upon the sample rate. */
@@ -612,13 +628,13 @@ static int dps310_write_raw(struct iio_dev *iio,
 	return rc;
 }
 
-static int dps310_calculate_pressure(struct dps310_data *data)
+static int dps310_calculate_pressure(struct dps310_data *data, int *val)
 {
 	int i;
 	int rc;
 	int t_ready;
-	int kpi = dps310_get_pres_k(data);
-	int kti = dps310_get_temp_k(data);
+	int kpi;
+	int kti;
 	s64 rem = 0ULL;
 	s64 pressure = 0ULL;
 	s64 p;
@@ -629,11 +645,13 @@ static int dps310_calculate_pressure(struct dps310_data *data)
 	s64 kp;
 	s64 kt;
 
-	if (kpi < 0)
-		return kpi;
+	rc = dps310_get_pres_k(data, &kpi);
+	if (rc)
+		return rc;
 
-	if (kti < 0)
-		return kti;
+	rc = dps310_get_temp_k(data, &kti);
+	if (rc)
+		return rc;
 
 	kp = (s64)kpi;
 	kt = (s64)kti;
@@ -687,7 +705,9 @@ static int dps310_calculate_pressure(struct dps310_data *data)
 	if (pressure < 0LL)
 		return -ERANGE;
 
-	return (int)min_t(s64, pressure, INT_MAX);
+	*val = (int)min_t(s64, pressure, INT_MAX);
+
+	return 0;
 }
 
 static int dps310_read_pressure(struct dps310_data *data, int *val, int *val2,
@@ -697,11 +717,10 @@ static int dps310_read_pressure(struct dps310_data *data, int *val, int *val2,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		rc = dps310_get_pres_samp_freq(data);
-		if (rc < 0)
+		rc = dps310_get_pres_samp_freq(data, val);
+		if (rc)
 			return rc;
 
-		*val = rc;
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_PROCESSED:
@@ -709,20 +728,17 @@ static int dps310_read_pressure(struct dps310_data *data, int *val, int *val2,
 		if (rc)
 			return rc;
 
-		rc = dps310_calculate_pressure(data);
-		if (rc < 0)
+		rc = dps310_calculate_pressure(data, val);
+		if (rc)
 			return rc;
 
-		*val = rc;
 		*val2 = 1000; /* Convert Pa to KPa per IIO ABI */
 		return IIO_VAL_FRACTIONAL;
 
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
-		rc = dps310_get_pres_precision(data);
-		if (rc < 0)
+		rc = dps310_get_pres_precision(data, val);
+		if (rc)
 			return rc;
-
-		*val = rc;
 		return IIO_VAL_INT;
 
 	default:
@@ -734,10 +750,11 @@ static int dps310_calculate_temp(struct dps310_data *data, int *val)
 {
 	s64 c0;
 	s64 t;
-	int kt = dps310_get_temp_k(data);
+	int kt, rc;
 
-	if (kt < 0)
-		return kt;
+	rc = dps310_get_temp_k(data, &kt);
+	if (rc)
+		return rc;
 
 	/* Obtain inverse-scaled offset */
 	c0 = div_s64((s64)kt * (s64)data->c0, 2);
@@ -758,11 +775,10 @@ static int dps310_read_temp(struct dps310_data *data, int *val, int *val2,
 
 	switch (mask) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		rc = dps310_get_temp_samp_freq(data);
-		if (rc < 0)
+		rc = dps310_get_temp_samp_freq(data, val);
+		if (rc)
 			return rc;
 
-		*val = rc;
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_PROCESSED:
@@ -777,11 +793,10 @@ static int dps310_read_temp(struct dps310_data *data, int *val, int *val2,
 		return IIO_VAL_INT;
 
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
-		rc = dps310_get_temp_precision(data);
-		if (rc < 0)
+		rc = dps310_get_temp_precision(data, val);
+		if (rc)
 			return rc;
 
-		*val = rc;
 		return IIO_VAL_INT;
 
 	default:
-- 
2.34.1

[PATCH v3 3/4] iio: pressure: dps310: consistently check return value of `regmap_read`

[Thomas Haemmerle]

Align the check of return values `regmap_read` so that it's consistent
across this driver code.

Signed-off-by: Thomas Haemmerle <thomas.haemmerle@leica-geosystems.com>
---
 drivers/iio/pressure/dps310.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
index 3fc436f36fa7..c30623d96f0e 100644
--- a/drivers/iio/pressure/dps310.c
+++ b/drivers/iio/pressure/dps310.c
@@ -171,7 +171,7 @@ static int dps310_temp_workaround(struct dps310_data *data)
 	int reg;
 
 	rc = regmap_read(data->regmap, 0x32, &reg);
-	if (rc)
+	if (rc < 0)
 		return rc;
 
 	/*
-- 
2.34.1

[PATCH v3 4/4] iio: pressure: dps310: simplify scale factor reading

[Thomas Haemmerle]

Both functions `dps310_get_pres_precision` and
`dps310_get_temp_precision` provide the oversampling rate by calling the
`BIT()` macro. However, to look up the corresponding scale factor, we
need the register value itself. Currently, this is achieved by undoing
the calculation of the oversampling rate with `ilog2()`.

Simplify the two functions for getting the scale factor and directly
use the register content for the lookup.

Signed-off-by: Thomas Haemmerle <thomas.haemmerle@leica-geosystems.com>
---
 drivers/iio/pressure/dps310.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
index c30623d96f0e..7d882e15e556 100644
--- a/drivers/iio/pressure/dps310.c
+++ b/drivers/iio/pressure/dps310.c
@@ -382,11 +382,11 @@ static int dps310_get_pres_k(struct dps310_data *data, int *val)
 {
 	int reg_val, rc;
 
-	rc = dps310_get_pres_precision(data, &reg_val);
-	if (rc)
+	rc = regmap_read(data->regmap, DPS310_PRS_CFG, &reg_val);
+	if (rc < 0)
 		return rc;
 
-	*val = scale_factors[ilog2(reg_val)];
+	*val = scale_factors[reg_val & GENMASK(2, 0)];
 
 	return 0;
 }
@@ -395,11 +395,11 @@ static int dps310_get_temp_k(struct dps310_data *data, int *val)
 {
 	int reg_val, rc;
 
-	rc = dps310_get_temp_precision(data, &reg_val);
-	if (rc)
+	rc = regmap_read(data->regmap, DPS310_TMP_CFG, &reg_val);
+	if (rc < 0)
 		return rc;
 
-	*val = scale_factors[ilog2(reg_val)];
+	*val = scale_factors[reg_val & GENMASK(2, 0)];
 
 	return 0;
 }
-- 
2.34.1

Re: [PATCH v3 0/4] iio: pressure: dps310: support negative temperature values

[Jonathan Cameron]

On Mon, 15 Apr 2024 12:50:26 +0200
Thomas Haemmerle <thomas.haemmerle@leica-geosystems.com> wrote:

> This patch set fixes the reading of negative temperatures (returned in
> millidegree celsius). As this requires a change of the error handling
> other functions are aligned with this.
> In addition a small code simplification for reading the scale factors
> for temperature and pressure is included.
Series applied to the togreg branch of iio.git.
Note I'll first push it out as testing to let the autobuilders see
if they can find any problems before I make a mess of linux-next.

Thanks,

Jonathan

> 
> ---
> Changes in v2:
>  - include fixes tag
>  - Split up patch
>  - introduce variables for intermediate results in functions
>  - simplify scale factor reading
> 
> Changes in v3:
>  - fix locking issues reported in https://lore.kernel.org/all/cbdafb33-fd3b-47ad-a678-83fa92475278@moroto.mountain/
> 
> Thomas Haemmerle (4):
>   iio: pressure: dps310: support negative temperature values
>   iio: pressure: dps310: introduce consistent error handling
>   iio: pressure: dps310: consistently check return value of
>     `regmap_read`
>   iio: pressure: dps310: simplify scale factor reading
> 
>  drivers/iio/pressure/dps310.c | 138 +++++++++++++++++++---------------
>  1 file changed, 77 insertions(+), 61 deletions(-)
> 
> 
> base-commit: 0bbac3facb5d6cc0171c45c9873a2dc96bea9680
> --
> 2.34.1