[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230205145331.1d67b5e6@jic23-huawei>
Date: Sun, 5 Feb 2023 14:53:31 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Angel Iglesias <ang.iglesiasg@...il.com>
Cc: linux-iio@...r.kernel.org, Lars-Peter Clausen <lars@...afoo.de>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Andreas Klinger <ak@...klinger.de>, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 5/7] iio: pressure: bmp280: Add support for new
sensor BMP580
On Sun, 29 Jan 2023 02:33:07 +0100
Angel Iglesias <ang.iglesiasg@...il.com> wrote:
> Adds compatibility with the new sensor generation, the BMP580.
>
> The measurement and initialization codepaths are adapted from
> the device datasheet and the repository from manufacturer at
> https://github.com/boschsensortec/BMP5-Sensor-API.
>
> Signed-off-by: Angel Iglesias <ang.iglesiasg@...il.com>
>
Hi Angel,
As you are doing one more version anyway, a few really minor comments inline.
Thanks,
Jonathan
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index 22addaaa5393..c65fb4025ad9 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> /*
> * These enums are used for indexing into the array of compensation
> * parameters for BMP280.
> @@ -1216,6 +1252,303 @@ const struct bmp280_chip_info bmp380_chip_info = {
> };
> EXPORT_SYMBOL_NS(bmp380_chip_info, IIO_BMP280);
>
> +/*
> + * BMP5xx soft reset procedure
Wild cards are often a bad idea, even in comments. Tend to end up covering
some device that works differently. With that in mind, not sure this comment
adds anything over the function name.
> + */
> +static int bmp580_soft_reset(struct bmp280_data *data)
> +{
> + unsigned int reg;
> + int ret;
> +
> + /* Write reset word to CMD register */
Not that informative as comments go.
> + ret = regmap_write(data->regmap, BMP580_REG_CMD, BMP580_CMD_SOFT_RESET);
> + if (ret) {
> + dev_err(data->dev, "failed to send reset command to device\n");
> + return ret;
> + }
> + /* Wait 2ms for reset completion */
nor is this one - drop them both.
> + usleep_range(2000, 2500);
> +
> + /* Dummy read of chip_id */
Now this one is good as not obvious why read is here so keep it!
> + ret = regmap_read(data->regmap, BMP580_REG_CHIP_ID, ®);
> + if (ret) {
> + dev_err(data->dev, "failed to reestablish comms after reset\n");
> + return ret;
> + }
> +
> + /* Check if POR bit is set on interrupt reg */
Not sure the comment adds anything not obviously from code. I'd be inclined
to drop it.
> + ret = regmap_read(data->regmap, BMP580_REG_INT_STATUS, ®);
> + if (ret) {
> + dev_err(data->dev, "error reading interrupt status register\n");
> + return ret;
> + }
> + if (!(reg & BMP580_INT_STATUS_POR_MASK)) {
> + dev_err(data->dev, "error resetting sensor\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * Contrary to previous sensors families, compensation algorithm is builtin.
> + * We are only required to read the register raw data and adapt the ranges
> + * for what is expected on IIO ABI.
> + */
> +
> +static int bmp580_read_temp(struct bmp280_data *data, int *val)
> +{
> + s32 raw_temp;
> + int ret;
> +
> + ret = regmap_bulk_read(data->regmap, BMP580_REG_TEMP_XLSB, data->buf,
> + sizeof(data->buf));
> + if (ret) {
> + dev_err(data->dev, "failed to read temperature\n");
> + return ret;
> + }
> +
> + raw_temp = get_unaligned_le24(data->buf);
> + if (raw_temp == BMP580_TEMP_SKIPPED) {
> + dev_err(data->dev, "reading temperature skipped\n");
> + return -EIO;
> + }
> +
> + /*
> + * Temperature is returned in Celsius degrees in fractional
> + * form down 2^16. We reescale by x1000 to return milli Celsius
> + * to respect IIO ABI.
> + */
> + *val = (raw_temp * 1000) >> 16;
Why not use IIO_VAL_FRACTION_LOG2 and keep the precision?
> + return IIO_VAL_INT;
> +}
Powered by blists - more mailing lists