On Sun, 29 Jan 2023 02:33:07 +0100 Angel Iglesias <ang.iglesiasg@xxxxxxxxx> 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@xxxxxxxxx> > 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; > +}
