From: Jonathan Cameron <jic23@kernel.org>
To: Vasileios Amoiridis <vassilisamir@gmail.com>
Cc: lars@metafoo.de, andriy.shevchenko@linux.intel.com,
ang.iglesiasg@gmail.com, mazziesaccount@gmail.com,
ak@it-klinger.de, petre.rodan@subdimension.ro,
phil@raspberrypi.com, 579lpy@gmail.com, linus.walleij@linaro.org,
semen.protsenko@linaro.org, linux-iio@vger.kernel.org,
linux-kernel@vger.kernel.org,
Jonathan Cameron <Jonathan.Cameron@huawei.com>
Subject: Re: [PATCH v6 5/9] iio: pressure: bmp280: Refactorize reading functions
Date: Sun, 12 May 2024 13:24:12 +0100 [thread overview]
Message-ID: <20240512132412.35d8fe36@jic23-huawei> (raw)
In-Reply-To: <20240508165207.145554-6-vassilisamir@gmail.com>
On Wed, 8 May 2024 18:52:03 +0200
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> For BMP18x, BMP28x, BME280, BMP38x the reading of the pressure
> value requires an update of the t_fine variable which happens
> through reading the temperature value.
>
> So all the bmpxxx_read_press() functions of the above sensors
> are internally calling the equivalent bmpxxx_read_temp() function
> in order to update the t_fine value. By just looking at the code
> this functionality is a bit hidden and is not easy to understand
> why those channels are not independent.
>
> This commit tries to clear these things a bit by splitting the
> bmpxxx_{read/compensate}_{temp/press/humid}() to the following:
>
> i. bmpxxx_read_{temp/press/humid}_adc(): read the raw value from
> the sensor.
>
> ii. bmpxx_calc_t_fine(): calculate the t_fine variable.
>
> iii. bmpxxx_get_t_fine(): get the t_fine variable.
>
> iv. bmpxxx_compensate_{temp/press/humid}(): compensate the adc
> values and return the calculated value.
>
> v. bmpxxx_read_{temp/press/humid}(): combine calls of the
> aforementioned functions to return the requested value.
>
> Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
A few minor things inline. Given I've picked up the 1st 4 patches,
please base your v7 on top of those.
Thanks,
Jonathan
> ---
> drivers/iio/pressure/bmp280-core.c | 361 ++++++++++++++++++-----------
> drivers/iio/pressure/bmp280.h | 6 -
> 2 files changed, 232 insertions(+), 135 deletions(-)
>
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index dd5c526dacbd..a864f8db8e24 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -288,13 +288,35 @@ static int bme280_read_calib(struct bmp280_data *data)
> *
> * Taken from BME280 datasheet, Section 4.2.3, "Compensation formula".
> */
Seems this comment should probably follow the maths which has moved.
> +static int bme280_read_humid_adc(struct bmp280_data *data, u16 *adc_humidity)
> +{
> + u16 value_humidity;
> + int ret;
> +
> + ret = regmap_bulk_read(data->regmap, BME280_REG_HUMIDITY_MSB,
> + &data->be16, sizeof(data->be16));
> + if (ret) {
> + dev_err(data->dev, "failed to read humidity\n");
> + return ret;
> + }
> +
> + value_humidity = be16_to_cpu(data->be16);
> + if (value_humidity == BMP280_HUMIDITY_SKIPPED) {
> + dev_err(data->dev, "reading humidity skipped\n");
> + return -EIO;
> + }
> + *adc_humidity = value_humidity;
> +
> + return 0;
> +}
...
> @@ -313,8 +335,29 @@ static u32 bme280_compensate_humidity(struct bmp280_data *data,
> *
> * Taken from datasheet, Section 3.11.3, "Compensation formula".
> */
Is this comment still relevant here? Seems it should probably move to follow
that maths.
> -static s32 bmp280_compensate_temp(struct bmp280_data *data,
> - u32 adc_temp)
> +static int bmp280_read_temp_adc(struct bmp280_data *data, u32 *adc_temp)
> +{
>
> static int bmp380_read_press(struct bmp280_data *data, int *val, int *val2)
> {
> - u32 adc_press, comp_press;
> + u32 adc_press, comp_press, t_fine;
> int ret;
>
> - /* Read and compensate for temperature so we get a reading of t_fine */
> - ret = bmp380_read_temp(data, NULL, NULL);
> + ret = bmp380_get_t_fine(data, &t_fine);
> if (ret)
> return ret;
>
> - ret = regmap_bulk_read(data->regmap, BMP380_REG_PRESS_XLSB,
> - data->buf, sizeof(data->buf));
> - if (ret) {
> - dev_err(data->dev, "failed to read pressure\n");
> + ret = bmp380_read_press_adc(data, &adc_press);
> + if (ret)
> return ret;
> - }
>
> - adc_press = get_unaligned_le24(data->buf);
> - if (adc_press == BMP380_PRESS_SKIPPED) {
> - dev_err(data->dev, "reading pressure skipped\n");
> - return -EIO;
> - }
> - comp_press = bmp380_compensate_press(data, adc_press);
> + comp_press = bmp380_compensate_press(data, adc_press, t_fine);
>
> + /* IIO units are in kPa */
Probably worth keeping the reference to what the unit of the
datasheet maths is around.
> *val = comp_press;
> - /* Compensated pressure is in cPa (centipascals) */
> *val2 = 100000;
>
> return IIO_VAL_FRACTIONAL;
> @@ -1825,7 +1916,7 @@ static int bmp180_wait_for_eoc(struct bmp280_data *data, u8 ctrl_meas)
> return 0;
> }
> -static u32 bmp180_compensate_press(struct bmp280_data *data, u32 adc_press)
> +static u32 bmp180_compensate_press(struct bmp280_data *data, u32 adc_press,
> + s32 t_fine)
> {
> struct bmp180_calib *calib = &data->calib.bmp180;
> s32 oss = data->oversampling_press;
> @@ -1965,7 +2068,7 @@ static u32 bmp180_compensate_press(struct bmp280_data *data, u32 adc_press)
> s32 b3, b6;
> u32 b4, b7;
>
> - b6 = data->t_fine - 4000;
> + b6 = t_fine - 4000;
> x1 = (calib->B2 * (b6 * b6 >> 12)) >> 11;
> x2 = calib->AC2 * b6 >> 11;
> x3 = x1 + x2;
> @@ -1974,7 +2077,7 @@ static u32 bmp180_compensate_press(struct bmp280_data *data, u32 adc_press)
> x2 = (calib->B1 * ((b6 * b6) >> 12)) >> 16;
> x3 = (x1 + x2 + 2) >> 2;
> b4 = calib->AC4 * (u32)(x3 + 32768) >> 15;
> - b7 = (adc_press - b3) * (50000 >> oss);
> + b7 = (((u32)adc_press) - b3) * (50000 >> oss);
Casting from u32 to u32?
> if (b7 < 0x80000000)
> p = (b7 * 2) / b4;
> else
> @@ -1990,19 +2093,19 @@ static u32 bmp180_compensate_press(struct bmp280_data *data, u32 adc_press)
> static int bmp180_read_press(struct bmp280_data *data, int *val, int *val2)
> + /* IIO units are in kPa */
I think this is an unrelated improvement as original code doesn't have such a comment.
So shouldn't really be in this patch. If you want to keep it here rather than pushing it
into an additional patch, mention it in the commit message. "additional comments on base
units added for consistency" or something like that.
> *val = comp_press;
> *val2 = 1000;
next prev parent reply other threads:[~2024-05-12 12:24 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-08 16:51 [PATCH v6 0/9] iio: pressure: bmp280: Add triggered buffer support and Vasileios Amoiridis
2024-05-08 16:51 ` [PATCH v6 1/9] iio: pressure: bmp280: Remove dead error checks Vasileios Amoiridis
2024-05-12 12:07 ` Jonathan Cameron
2024-05-08 16:52 ` [PATCH v6 2/9] iio: pressure: bmp280: Remove, add and update error messages Vasileios Amoiridis
2024-05-12 12:09 ` Jonathan Cameron
2024-05-08 16:52 ` [PATCH v6 3/9] iio: pressure: bmp280: Make error checks consistent Vasileios Amoiridis
2024-05-12 12:11 ` Jonathan Cameron
2024-05-08 16:52 ` [PATCH v6 4/9] iio: pressure: bmp280: Use unsigned data types for raw sensor data Vasileios Amoiridis
2024-05-12 12:13 ` Jonathan Cameron
2024-05-08 16:52 ` [PATCH v6 5/9] iio: pressure: bmp280: Refactorize reading functions Vasileios Amoiridis
2024-05-12 12:24 ` Jonathan Cameron [this message]
2024-05-12 18:42 ` Vasileios Amoiridis
2024-05-08 16:52 ` [PATCH v6 6/9] iio: pressure: bmp280: Introduce new cleanup routines Vasileios Amoiridis
2024-05-08 16:52 ` [PATCH v6 7/9] iio: pressure: bmp280: Generalize read_{temp,press,humid}() functions Vasileios Amoiridis
2024-05-08 16:52 ` [PATCH v6 8/9] iio: pressure: bmp280: Add SCALE, RAW values in channels and refactorize them Vasileios Amoiridis
2024-05-12 12:29 ` Jonathan Cameron
2024-05-12 18:48 ` Vasileios Amoiridis
2024-05-08 16:52 ` [PATCH v6 9/9] iio: pressure: bmp280: Add triggered buffer support Vasileios Amoiridis
2024-05-12 12:34 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240512132412.35d8fe36@jic23-huawei \
--to=jic23@kernel.org \
--cc=579lpy@gmail.com \
--cc=Jonathan.Cameron@huawei.com \
--cc=ak@it-klinger.de \
--cc=andriy.shevchenko@linux.intel.com \
--cc=ang.iglesiasg@gmail.com \
--cc=lars@metafoo.de \
--cc=linus.walleij@linaro.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mazziesaccount@gmail.com \
--cc=petre.rodan@subdimension.ro \
--cc=phil@raspberrypi.com \
--cc=semen.protsenko@linaro.org \
--cc=vassilisamir@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).