Received-SPF: pass (google.com: domain of linux-kernel+bounces-122958-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 28 Mar 2024 13:34:21 +0000
From: Jonathan Cameron <jic23@kernel.org>
To: Thomas Haemmerle <thomas.haemmerle@leica-geosystems.com>
Cc: joel@jms.id.au, bsp-development.geo@leica-geosystems.com, Eddie James
 <eajames@linux.ibm.com>, Lars-Peter Clausen <lars@metafoo.de>,
 linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] iio: pressure: dps310: support negative pressure and
 temperature values
Message-ID: <20240328133421.1844a83c@jic23-huawei>
In-Reply-To: <20240327084937.3801125-1-thomas.haemmerle@leica-geosystems.com>
References: <20240327084937.3801125-1-thomas.haemmerle@leica-geosystems.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Wed, 27 Mar 2024 09:49:36 +0100
Thomas Haemmerle <thomas.haemmerle@leica-geosystems.com> 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@leica-geosystems.com>
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, &reg);
> -	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;
>  }