Received-SPF: pass (google.com: domain of linux-kernel+bounces-133885-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Sat, 6 Apr 2024 11:08:17 +0100
From: Jonathan Cameron <jic23@kernel.org>
To: HAEMMERLE Thomas <thomas.haemmerle@leica-geosystems.com>
Cc: "joel@jms.id.au" <joel@jms.id.au>, GEO-CHHER-bsp-development
 <bsp-development.geo@leica-geosystems.com>, Eddie James
 <eajames@linux.ibm.com>, Lars-Peter Clausen <lars@metafoo.de>,
 "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] iio: pressure: dps310: support negative pressure and
 temperature values
Message-ID: <20240406110817.2c2be48d@jic23-huawei>
In-Reply-To: <34c1c26a-4787-4713-8c7d-040732a18092@leica-geosystems.com>
References: <20240327084937.3801125-1-thomas.haemmerle@leica-geosystems.com>
	<20240328133421.1844a83c@jic23-huawei>
	<34c1c26a-4787-4713-8c7d-040732a18092@leica-geosystems.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit


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

More simply avoid _get_pres_precision returning a value that is in the form
that requires us to immediately undo the BIT() logic at the end of that function.
One way to do that is to just call the regmap_read() directly here.

> 
> >> +
> >> +     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.
Just call rc = regmap_read(data->regmap, DPS310_TMP_CFG, val);
here and use 
*val = scale_factors[val & GENMASK(2, 0)];
here which I think ends up with the same value.

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