On Wed, 27 Mar 2024 09:49:36 +0100
Thomas Haemmerle <thomas.haemmerle@xxxxxxxxxxxxxxxxxxxx> wrote:
> The current implementation interprets negative values returned from
> function invocation as error codes, even those that report actual data.
> This has a side effect that when temperature values are calculated -
> they also converted by error code, which leads to false interpretation
> of results.
>
> Fix this by using the return values only for error handling and passing
> a pointer for the values.
>
> Signed-off-by: Thomas Haemmerle <thomas.haemmerle@xxxxxxxxxxxxxxxxxxxx>
Hi Thomas,
This needs a fixes tag so we know where to backport it to.
A few other comments inline. Note that one aim in a fix is to keep things
minimal to make it easy to backport. If you want to the follow the fix
with a cleanup patch that makes the driver more consistent that is great,
just don't combine that with the bug fix.
Jonathan
> ---
> drivers/iio/pressure/dps310.c | 122 +++++++++++++++++++---------------
> 1 file changed, 69 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/iio/pressure/dps310.c b/drivers/iio/pressure/dps310.c
> index 1ff091b2f764..373d1c063b05 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, ®);
> - if (rc)
> + if (rc < 0)
> return rc;
Why this change? It seems unrelated to the issue you are fixing.
>
> /*
> @@ -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;
>
> - rc = regmap_read(data->regmap, DPS310_PRS_CFG, &val);
> + rc = regmap_read(data->regmap, DPS310_PRS_CFG, val);
> if (rc < 0)
> return rc;
I'd prefer a local variable here for the intermediate result.
>
> - return BIT(val & GENMASK(2, 0));
> + *val = BIT(*val & GENMASK(2, 0));
For these precision values, it's positive anyway, so why
change it to report this way? Consistency only or am I missing something else?
> +
> + 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;
>
> - rc = regmap_read(data->regmap, DPS310_TMP_CFG, &val);
> + rc = regmap_read(data->regmap, DPS310_TMP_CFG, val);
As above, local variable for intermediate result would be clearer.
> 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(*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;
>
> - rc = regmap_read(data->regmap, DPS310_PRS_CFG, &val);
> + rc = regmap_read(data->regmap, DPS310_PRS_CFG, val);
Same again.
> if (rc < 0)
> return rc;
>
> - return BIT((val & DPS310_PRS_RATE_BITS) >> 4);
> + *val = BIT((*val & DPS310_PRS_RATE_BITS) >> 4);
Whilst here nice to use BIT(FIELD_GET(regval, DPS310_PRS_RATE_BITS));
> +
> + 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;
>
> - rc = regmap_read(data->regmap, DPS310_TMP_CFG, &val);
> + rc = regmap_read(data->regmap, DPS310_TMP_CFG, val);
> if (rc < 0)
> return rc;
>
> - return BIT((val & DPS310_TMP_RATE_BITS) >> 4);
> + *val = BIT((*val & DPS310_TMP_RATE_BITS) >> 4);
As above.
> +
> + 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 rc;
>
> - if (rc < 0)
> + rc = dps310_get_pres_precision(data, val);
> + if (rc)
> return rc;
>
> - return scale_factors[ilog2(rc)];
> + *val = scale_factors[ilog2(*val)];
This only just went to the effort of 2^val, so why not skip that step and
pull the BIT() section out to read_pressure() where we do want that form.
You will need an extra local variable at that call site I think, but
in general it is a useful additional simplification of the code.
> +
> + 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 rc;
>
> - if (rc < 0)
> + rc = dps310_get_temp_precision(data, val);
> + if (rc)
> return rc;
>
> - return scale_factors[ilog2(rc)];
> + *val = scale_factors[ilog2(*val)];
As above.
> +
> + return 0;
> }
