Re: [PATCH v2 04/13] iio: chemical: bme680: refactorize set_mode() mode

[Vasileios Amoiridis <vassilisamir@gmail.com>]

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;
> >  
>