Re: [PATCH] iio: pressure: dps310: support negative pressure and temperature values

[HAEMMERLE Thomas]

Hi Jonathan!

Thanks for the review!

On 28.03.24 14:34, Jonathan Cameron wrote:
> 
> 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.

Will add it.

> 
> 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.

ACK - I will split the patch.

> 
> 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, &reg);
>> -     if (rc)
>> +     if (rc < 0)
>>                return rc;
> 
> Why this change?  It seems unrelated to the issue you are fixing.

The return values in this driver are not checked consistently, and this 
aligns with the other call(s) of `regmap_read`. But I agree - it's not 
related to the issue.

> 
>>
>>        /*
>> @@ -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.

ACK.

>>
>> -     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?

Yes - for consistency.

>> +
>> +     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.

ACK.

>>        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.

ACK.

>>        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.
> 

ACK.

>> +
>> +     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.

I'm not sure if I get you correct, as this function is not directly 
called in `read_pressure`:
You suggest dropping this function at all, call 
`dps310_get_pres_precision` directly in `dps310_calculate_pressure` and 
move the lookup of the compensation scale factor there?

>> +
>> +     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.

Based on my interpretation above:
For `dps310_get_temp_k` it would require to move the lookup of the 
compensation scale factor to `dps310_calculate_pressure` and 
`dps310_calculate_temp`.
Maybe this would simplify the code, but it would make it harder to read.


Thomas

>> +
>> +     return 0;
>>   }
>