On Sun, 2023-02-05 at 14:53 +0000, Jonathan Cameron wrote: > 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. Understood! > > > + 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? Although this sensor has a resolution of 1/2^16, its absolute accuracy is of +/- 0.5ºC so I suppose in the end we aren't really losing precision. But in the future a high accuracy variant of the sensor might pop up (like the bmp384 in the previous gen) so I think is a good idea to keep the precision. Thanks for the heads up! > > + return IIO_VAL_INT; > > +} > > Thanks for your time! Angel
