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

[Jonathan Cameron <jic23@kernel.org>]

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