On Sun, Oct 27, 2024 at 09:59:39AM +0000, Jonathan Cameron wrote:
> On Mon, 21 Oct 2024 21:53:07 +0200
> Vasileios Amoiridis <vassilisamir@xxxxxxxxx> wrote:
>
> > Refactorize the set_mode() function to use an external enum that
> > describes the possible modes of the BME680 device instead of using
> > true/false variables for selecting SLEEPING/FORCED mode.
> >
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@xxxxxxxxx>
> I changed my mind on this one...
>
> > ---
> > drivers/iio/chemical/bme680_core.c | 30 +++++++++++++-----------------
> > 1 file changed, 13 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
> > index d228f90b4dc6..9002519d2c33 100644
> > --- a/drivers/iio/chemical/bme680_core.c
> > +++ b/drivers/iio/chemical/bme680_core.c
> > @@ -95,6 +95,11 @@ struct bme680_calib {
> > s8 range_sw_err;
> > };
> >
> > +enum bme680_op_mode {
> > + BME680_SLEEP,
> > + BME680_FORCED,
> Use this enum to replace the existing BME680_MODE_SLEEP etc definitions
> rather than adding another one.
> Also assign explicit values as you are going to write this into a register
> so they matter.
>
>
Hi Jonathan,
Thank you very much once again for the review! I totally understand what
you mean and I will fix it for next version.
Cheers,
Vasilis
> > +};
> > +
> > struct bme680_data {
> > struct regmap *regmap;
> > struct bme680_calib bme680;
> > @@ -502,23 +507,16 @@ static u8 bme680_calc_heater_dur(u16 dur)
> > return durval;
> > }
> >
> > -static int bme680_set_mode(struct bme680_data *data, bool mode)
> > +static int bme680_set_mode(struct bme680_data *data, enum bme680_op_mode mode)
> > {
> > struct device *dev = regmap_get_device(data->regmap);
> > int ret;
> >
> > - if (mode) {
> > - ret = regmap_write_bits(data->regmap, BME680_REG_CTRL_MEAS,
> > - BME680_MODE_MASK, BME680_MODE_FORCED);
> > - if (ret < 0)
> > - dev_err(dev, "failed to set forced mode\n");
> > -
> > - } else {
> > - ret = regmap_write_bits(data->regmap, BME680_REG_CTRL_MEAS,
> > - BME680_MODE_MASK, BME680_MODE_SLEEP);
> > - if (ret < 0)
> > - dev_err(dev, "failed to set sleep mode\n");
> > -
> > + ret = regmap_write_bits(data->regmap, BME680_REG_CTRL_MEAS,
> > + BME680_MODE_MASK, mode);
> This is the problematic code. No obvious reason the enum should match the original
> values. It does, but that should be made true by only having an enum, not definitions
> and an enum.
>
> > + if (ret < 0) {
> > + dev_err(dev, "failed to set ctrl_meas register\n");
> > + return ret;
> > }
> >
> > return ret;
> > @@ -615,8 +613,7 @@ static int bme680_gas_config(struct bme680_data *data)
> > int ret;
> > u8 heatr_res, heatr_dur;
> >
> > - /* Go to sleep */
> > - ret = bme680_set_mode(data, false);
> > + ret = bme680_set_mode(data, BME680_SLEEP);
> > if (ret < 0)
> > return ret;
> >
> > @@ -756,8 +753,7 @@ static int bme680_read_raw(struct iio_dev *indio_dev,
> >
> > guard(mutex)(&data->lock);
> >
> > - /* set forced mode to trigger measurement */
> > - ret = bme680_set_mode(data, true);
> > + ret = bme680_set_mode(data, BME680_FORCED);
> > if (ret < 0)
> > return ret;
> >
>
