From: Vasileios Amoiridis <vassilisamir@gmail.com>
To: Jonathan Cameron <jic23@kernel.org>
Cc: Vasileios Amoiridis <vassilisamir@gmail.com>,
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,
u.kleine-koenig@pengutronix.de, biju.das.jz@bp.renesas.com,
linus.walleij@linaro.org, semen.protsenko@linaro.org,
linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 1/6] iio: pressure: bmp280: Various driver cleanups
Date: Mon, 15 Apr 2024 01:41:14 +0200 [thread overview]
Message-ID: <20240414234114.GA33095@vamoiridPC> (raw)
In-Reply-To: <20240413175257.6cadbb83@jic23-huawei>
On Sat, Apr 13, 2024 at 05:52:57PM +0100, Jonathan Cameron wrote:
> On Sun, 7 Apr 2024 19:29:15 +0200
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
>
> > Various driver cleanups including:
> >
> Not sure how we got to a v4 with a patch title various.
>
> If you have to list multiple changes, it should normally
> be multiple patches.
>
> White space can all be grouped, but the others should be separate.
> Please break it up for v5.
>
> I'll take a look at the actual changes even though I won't merge
> a 'various' patch like this.
>
> I may well miss things because there is simply too much in here
> and some of the diffs are subtle as it can be hard to spot
> if it's a name change or a functional change.
>
> > a) change names of functions that are used by all sensors from
> > bmp280_read_raw_* to bmp_read_raw_* to make it more clear that
> > these functions are general and not tied to the BMP280 sensor.
> Don't. Convention is to naming such function after the first supported
> part. We've tried generic naming in the past and often becomes even
> less clear. Already you have bmp_x functions applying to bme devices.
> Sooner or later you will have them applying to an xyz280 where none
> of the letter matter.
> >
>
>
> > b) modify some defines that are used only by the BME280 sensor
> > to use the naming convention BME280_* and not BMP280_*.
>
> This is fine, but also move them so they aren't in blocks labeled
> BMP280 specific registers.
>
> >
> > c) add various error messages that were not implemented.
> Also fine in a patch on their own.
> >
While reflecting a bit on that, I would like to ask the following:
In case of regmap_{bulk/}_{read/write/update}() functions, they always
return 0 on success and negative errno in error cases. This driver is
a bit incosistent with many cases checking for returned errors with
the following 2 forms:
a) if (ret) {
...
}
b) if (ret < 0) {
...
}
Which one is prefered in general?
Cheers,
Vasilis
> > d) sort headers.
> Separate patch and this is probably fine.
>
> >
> > e) fix various indentation errors which were found by checkpatch.pl.
>
> White space fixes always belong in a patch that does nothing else.
>
> >
> > g) Add identifier names in function definitions which were warned
> > by checkpatch.pl.
> This is fine, but again not in a patch making other changes.
>
> I want to be reading a patch whilst just looking at one type of thing.
> It is much quicker to review 6 single purpose patches than 1 patch
> combining all 6.
>
>
> Jonathan
> >
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > ---
> > drivers/iio/pressure/bmp280-core.c | 244 ++++++++++++++-------------
> > drivers/iio/pressure/bmp280-i2c.c | 2 +-
> > drivers/iio/pressure/bmp280-regmap.c | 8 +-
> > drivers/iio/pressure/bmp280-spi.c | 8 +-
> > drivers/iio/pressure/bmp280.h | 50 +++---
> > 5 files changed, 159 insertions(+), 153 deletions(-)
> >
> > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > index 09f53d987c7d..1c51139cbfcf 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -52,7 +52,6 @@
> > */
> > enum { AC1, AC2, AC3, AC4, AC5, AC6, B1, B2, MB, MC, MD };
> >
> > -
> > enum bmp380_odr {
> > BMP380_ODR_200HZ,
> > BMP380_ODR_100HZ,
> > @@ -71,7 +70,7 @@ enum bmp380_odr {
> > BMP380_ODR_0_01HZ,
> > BMP380_ODR_0_006HZ,
> > BMP380_ODR_0_003HZ,
> > - BMP380_ODR_0_0015HZ,
> > + BMP380_ODR_0_0015HZ
>
> Why? We remove the comma when the last element is clearly a terminator, not
> because it happens to be the last element. This isn't NULL, or _COUNT or similar
> which must always come at the end.
>
>
> > };
> >
> > enum bmp580_odr {
> > @@ -106,7 +105,7 @@ enum bmp580_odr {
> > BMP580_ODR_1HZ,
> > BMP580_ODR_0_5HZ,
> > BMP580_ODR_0_25HZ,
> > - BMP580_ODR_0_125HZ,
> > + BMP580_ODR_0_125HZ
>
> As above, I can't see a reason to change this.
>
> > };
> >
> > /*
> > @@ -131,7 +130,7 @@ enum {
> > BMP380_P8 = 16,
> > BMP380_P9 = 17,
> > BMP380_P10 = 19,
> > - BMP380_P11 = 20,
> > + BMP380_P11 = 20
> and again.
>
> > };
> >
> > static const struct iio_chan_spec bmp280_channels[] = {
> > @@ -181,11 +180,10 @@ static int bmp280_read_calib(struct bmp280_data *data)
> > struct bmp280_calib *calib = &data->calib.bmp280;
> > int ret;
> >
> > -
> > /* Read temperature and pressure calibration values. */
> > ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_TEMP_START,
> > data->bmp280_cal_buf, sizeof(data->bmp280_cal_buf));
> > - if (ret < 0) {
> > + if (ret) {
> > dev_err(data->dev,
> > "failed to read temperature and pressure calibration parameters\n");
> > return ret;
> > @@ -222,7 +220,7 @@ static int bme280_read_calib(struct bmp280_data *data)
> >
> > /* Load shared calibration params with bmp280 first */
> > ret = bmp280_read_calib(data);
> > - if (ret < 0) {
> > + if (ret) {
> > dev_err(dev, "failed to read common bmp280 calibration parameters\n");
> > return ret;
> > }
> > @@ -235,47 +233,47 @@ static int bme280_read_calib(struct bmp280_data *data)
> > * Humidity data is only available on BME280.
> > */
> >
> > - ret = regmap_read(data->regmap, BMP280_REG_COMP_H1, &tmp);
> > - if (ret < 0) {
> > + ret = regmap_read(data->regmap, BME280_REG_COMP_H1, &tmp);
> > + if (ret) {
> > dev_err(dev, "failed to read H1 comp value\n");
> > return ret;
> > }
> > calib->H1 = tmp;
> >
> > - ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_H2,
> > + ret = regmap_bulk_read(data->regmap, BME280_REG_COMP_H2,
> > &data->le16, sizeof(data->le16));
> > - if (ret < 0) {
> > + if (ret) {
> > dev_err(dev, "failed to read H2 comp value\n");
> > return ret;
> > }
> > calib->H2 = sign_extend32(le16_to_cpu(data->le16), 15);
> >
> > - ret = regmap_read(data->regmap, BMP280_REG_COMP_H3, &tmp);
> > - if (ret < 0) {
> > + ret = regmap_read(data->regmap, BME280_REG_COMP_H3, &tmp);
> > + if (ret) {
> > dev_err(dev, "failed to read H3 comp value\n");
> > return ret;
> > }
> > calib->H3 = tmp;
> >
> > - ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_H4,
> > + ret = regmap_bulk_read(data->regmap, BME280_REG_COMP_H4,
> > &data->be16, sizeof(data->be16));
> > - if (ret < 0) {
> > + if (ret) {
> > dev_err(dev, "failed to read H4 comp value\n");
> > return ret;
> > }
> > calib->H4 = sign_extend32(((be16_to_cpu(data->be16) >> 4) & 0xff0) |
> > (be16_to_cpu(data->be16) & 0xf), 11);
> >
> > - ret = regmap_bulk_read(data->regmap, BMP280_REG_COMP_H5,
> > + ret = regmap_bulk_read(data->regmap, BME280_REG_COMP_H5,
> > &data->le16, sizeof(data->le16));
> > - if (ret < 0) {
> > + if (ret) {
> > dev_err(dev, "failed to read H5 comp value\n");
> > return ret;
> > }
> > - calib->H5 = sign_extend32(FIELD_GET(BMP280_COMP_H5_MASK, le16_to_cpu(data->le16)), 11);
> > + calib->H5 = sign_extend32(FIELD_GET(BME280_COMP_H5_MASK, le16_to_cpu(data->le16)), 11);
> >
> > - ret = regmap_read(data->regmap, BMP280_REG_COMP_H6, &tmp);
> > - if (ret < 0) {
> > + ret = regmap_read(data->regmap, BME280_REG_COMP_H6, &tmp);
> > + if (ret) {
> > dev_err(dev, "failed to read H6 comp value\n");
> > return ret;
> > }
> > @@ -283,14 +281,14 @@ static int bme280_read_calib(struct bmp280_data *data)
> >
> > return 0;
> > }
> > +
> > /*
> > * Returns humidity in percent, resolution is 0.01 percent. Output value of
> > * "47445" represents 47445/1024 = 46.333 %RH.
> > *
> > * Taken from BME280 datasheet, Section 4.2.3, "Compensation formula".
> > */
> > -static u32 bmp280_compensate_humidity(struct bmp280_data *data,
> > - s32 adc_humidity)
> > +static u32 bme280_compensate_humidity(struct bmp280_data *data, s32 adc_humidity)
> > {
> > struct bmp280_calib *calib = &data->calib.bmp280;
> > s32 var;
> > @@ -305,7 +303,7 @@ static u32 bmp280_compensate_humidity(struct bmp280_data *data,
> > var = clamp_val(var, 0, 419430400);
> >
> > return var >> 12;
> > -};
> > +}
> >
> > /*
> > * Returns temperature in DegC, resolution is 0.01 DegC. Output value of
> > @@ -314,8 +312,7 @@ static u32 bmp280_compensate_humidity(struct bmp280_data *data,
> > *
> > * Taken from datasheet, Section 3.11.3, "Compensation formula".
> > */
> > -static s32 bmp280_compensate_temp(struct bmp280_data *data,
> > - s32 adc_temp)
> > +static s32 bmp280_compensate_temp(struct bmp280_data *data, s32 adc_temp)
> > {
> > struct bmp280_calib *calib = &data->calib.bmp280;
> > s32 var1, var2;
> > @@ -337,8 +334,7 @@ static s32 bmp280_compensate_temp(struct bmp280_data *data,
> > *
> > * Taken from datasheet, Section 3.11.3, "Compensation formula".
> > */
> > -static u32 bmp280_compensate_press(struct bmp280_data *data,
> > - s32 adc_press)
> > +static u32 bmp280_compensate_press(struct bmp280_data *data, s32 adc_press)
> > {
> > struct bmp280_calib *calib = &data->calib.bmp280;
> > s64 var1, var2, p;
> > @@ -363,15 +359,14 @@ static u32 bmp280_compensate_press(struct bmp280_data *data,
> > return (u32)p;
> > }
> >
> > -static int bmp280_read_temp(struct bmp280_data *data,
> > - int *val, int *val2)
> > +static int bmp280_read_temp(struct bmp280_data *data, int *val, int *val2)
> > {
> > s32 adc_temp, comp_temp;
> > int ret;
> >
> > ret = regmap_bulk_read(data->regmap, BMP280_REG_TEMP_MSB,
> > data->buf, sizeof(data->buf));
> > - if (ret < 0) {
> > + if (ret) {
> > dev_err(data->dev, "failed to read temperature\n");
> > return ret;
> > }
> > @@ -396,8 +391,7 @@ static int bmp280_read_temp(struct bmp280_data *data,
> > return 0;
> > }
> >
> > -static int bmp280_read_press(struct bmp280_data *data,
> > - int *val, int *val2)
> > +static int bmp280_read_press(struct bmp280_data *data, int *val, int *val2)
> > {
> > u32 comp_press;
> > s32 adc_press;
> > @@ -405,12 +399,12 @@ static int bmp280_read_press(struct bmp280_data *data,
> >
> > /* Read and compensate temperature so we get a reading of t_fine. */
> > ret = bmp280_read_temp(data, NULL, NULL);
> > - if (ret < 0)
> > + if (ret)
> > return ret;
> >
> > ret = regmap_bulk_read(data->regmap, BMP280_REG_PRESS_MSB,
> > data->buf, sizeof(data->buf));
> > - if (ret < 0) {
> > + if (ret) {
> > dev_err(data->dev, "failed to read pressure\n");
> > return ret;
> > }
> > @@ -429,7 +423,7 @@ static int bmp280_read_press(struct bmp280_data *data,
> > return IIO_VAL_FRACTIONAL;
> > }
> >
> > -static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
> > +static int bme280_read_humid(struct bmp280_data *data, int *val, int *val2)
> > {
> > u32 comp_humidity;
> > s32 adc_humidity;
> > @@ -437,12 +431,12 @@ static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
> >
> > /* Read and compensate temperature so we get a reading of t_fine. */
> > ret = bmp280_read_temp(data, NULL, NULL);
> > - if (ret < 0)
> > + if (ret)
> > return ret;
> >
> > - ret = regmap_bulk_read(data->regmap, BMP280_REG_HUMIDITY_MSB,
> > + ret = regmap_bulk_read(data->regmap, BME280_REG_HUMIDITY_MSB,
> > &data->be16, sizeof(data->be16));
> > - if (ret < 0) {
> > + if (ret) {
> > dev_err(data->dev, "failed to read humidity\n");
> > return ret;
> > }
> > @@ -453,16 +447,16 @@ static int bmp280_read_humid(struct bmp280_data *data, int *val, int *val2)
> > dev_err(data->dev, "reading humidity skipped\n");
> > return -EIO;
> > }
> > - comp_humidity = bmp280_compensate_humidity(data, adc_humidity);
> > + comp_humidity = bme280_compensate_humidity(data, adc_humidity);
> >
> > *val = comp_humidity * 1000 / 1024;
> >
> > return IIO_VAL_INT;
> > }
> >
> > -static int bmp280_read_raw(struct iio_dev *indio_dev,
> > - struct iio_chan_spec const *chan,
> > - int *val, int *val2, long mask)
> > +static int bmp_read_raw(struct iio_dev *indio_dev,
>
> No to this sort of change. bmp280_ is the prefix for the driver - it doesn't
> mean that it applies only to that part. As such it is the prefix
> we should use throughout the driver unless a function is specific
> to a different part. bmp is too generic and may cause namespace issues
> like a clash with something in a header at somepoint in future.
>
> > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> > index 5812a344ed8e..ea8eb5691428 100644
> > --- a/drivers/iio/pressure/bmp280.h
> > +++ b/drivers/iio/pressure/bmp280.h
> > @@ -1,10 +1,10 @@
> > /* SPDX-License-Identifier: GPL-2.0 */
> > #include <linux/bitops.h>
> > #include <linux/device.h>
> > -#include <linux/iio/iio.h>
> > #include <linux/regmap.h>
> > #include <linux/regulator/consumer.h>
> >
> > +#include <linux/iio/iio.h>
> >
> > /* BMP580 specific registers */
> > #define BMP580_REG_CMD 0x7E
> > @@ -192,8 +192,8 @@
> > #define BMP380_PRESS_SKIPPED 0x800000
> >
> > /* BMP280 specific registers */
> > -#define BMP280_REG_HUMIDITY_LSB 0xFE
> > -#define BMP280_REG_HUMIDITY_MSB 0xFD
> > +#define BME280_REG_HUMIDITY_LSB 0xFE
> > +#define BME280_REG_HUMIDITY_MSB 0xFD
> They are in a block called BMP280 specific registers why
> are they prefixed with BME280?
>
> If they don't apply to the BMP280 add a new block with
> a comment to say BME280 specific registers.
>
>
> > #define BMP280_REG_TEMP_XLSB 0xFC
> > #define BMP280_REG_TEMP_LSB 0xFB
> > #define BMP280_REG_TEMP_MSB 0xFA
> > @@ -207,15 +207,15 @@
> > #define BMP280_REG_CONFIG 0xF5
> > #define BMP280_REG_CTRL_MEAS 0xF4
> > #define BMP280_REG_STATUS 0xF3
> > -#define BMP280_REG_CTRL_HUMIDITY 0xF2
> > +#define BME280_REG_CTRL_HUMIDITY 0xF2
>
> Jonathan
>
next prev parent reply other threads:[~2024-04-14 23:41 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-07 17:29 [PATCH v4 0/6] Driver cleanup and series to add triggered buffer Vasileios Amoiridis
2024-04-07 17:29 ` [PATCH v4 1/6] iio: pressure: bmp280: Various driver cleanups Vasileios Amoiridis
2024-04-13 16:52 ` Jonathan Cameron
2024-04-14 16:19 ` Vasileios Amoiridis
2024-04-14 18:13 ` Jonathan Cameron
2024-04-14 23:41 ` Vasileios Amoiridis [this message]
2024-04-07 17:29 ` [PATCH v4 2/6] iio: pressure: bmp280: Refactorize reading functions Vasileios Amoiridis
2024-04-13 16:56 ` Jonathan Cameron
2024-04-07 17:29 ` [PATCH v4 3/6] iio: pressure: bmp280: Introduce new cleanup routines Vasileios Amoiridis
2024-04-13 16:58 ` Jonathan Cameron
2024-04-07 17:29 ` [PATCH v4 4/6] iio: pressure: bmp280: Generalize read_{temp,press,humid}() functions Vasileios Amoiridis
2024-04-07 17:29 ` [PATCH v4 5/6] iio: pressure: bmp280: Add SCALE, RAW values in channels and refactorize them Vasileios Amoiridis
2024-04-07 17:29 ` [PATCH v4 6/6] iio: pressure: bmp280: Add triggered buffer support Vasileios Amoiridis
2024-04-13 17:06 ` Jonathan Cameron
2024-04-07 21:51 ` [PATCH v4 0/6] Driver cleanup and series to add triggered buffer Angel Iglesias
2024-04-08 15:01 ` Andy Shevchenko
2024-04-08 16:58 ` Vasileios Amoiridis
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=20240414234114.GA33095@vamoiridPC \
--to=vassilisamir@gmail.com \
--cc=579lpy@gmail.com \
--cc=ak@it-klinger.de \
--cc=andriy.shevchenko@linux.intel.com \
--cc=ang.iglesiasg@gmail.com \
--cc=biju.das.jz@bp.renesas.com \
--cc=jic23@kernel.org \
--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=u.kleine-koenig@pengutronix.de \
/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).