Re: [PATCH v1 05/13] iio: chemical: bme680: refactorize set_mode() mode

[Jonathan Cameron]

On Thu, 10 Oct 2024 23:00:22 +0200
vamoirid <vassilisamir@xxxxxxxxx> wrote:

> From: Vasileios Amoiridis <vassilisamir@xxxxxxxxx>
> 
> 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>
> ---
>  drivers/iio/chemical/bme680_core.c | 36 ++++++++++++++++--------------
>  1 file changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
> index 9e843e463502..dedb7edaf43d 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;
> @@ -501,25 +506,24 @@ 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");
> -
> +	switch (mode) {
> +	case BME680_SLEEP:
> +	case BME680_FORCED:
You are passing in an enum that currently has no other values.
The compiler should complain if it isn't one of these (and it can tell)

So unless I'm missing you adding another enum value later, this switch
should be unnecessary.


> +		break;
> +	default:
> +		return -EINVAL;
>  	}
>  
> +	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;
>  }
>  
> @@ -612,8 +616,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;
>  
> @@ -750,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;
>