| Message ID | 20241021195316.58911-5-vassilisamir@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | : chemical: bme680: 2nd set of cleanup | expand |
On Mon, 21 Oct 2024 21:53:07 +0200 Vasileios Amoiridis <vassilisamir@gmail.com> 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@gmail.com> Improves readability so fair enough. Applied.
On Mon, 21 Oct 2024 21:53:07 +0200 Vasileios Amoiridis <vassilisamir@gmail.com> 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@gmail.com> 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. > +}; > + > 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; >
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@gmail.com> 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@gmail.com> > 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; > > >
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, +}; + 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); + 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;
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@gmail.com> --- drivers/iio/chemical/bme680_core.c | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-)