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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Sat, 6 Apr 2024 11:08:17 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: HAEMMERLE Thomas <thomas.haemmerle@...ca-geosystems.com>
Cc: "joel@....id.au" <joel@....id.au>, GEO-CHHER-bsp-development
 <bsp-development.geo@...ca-geosystems.com>, Eddie James
 <eajames@...ux.ibm.com>, Lars-Peter Clausen <lars@...afoo.de>,
 "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] iio: pressure: dps310: support negative pressure and
 temperature values


> >> -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;
> >>   }  
> >   
>