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, ®);
>> - 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;
>> }
>
