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

vamoirid posted 13 patches 1 month, 2 weeks ago
[PATCH v1 05/13] iio: chemical: bme680: refactorize set_mode() mode
Posted by vamoirid 1 month, 2 weeks ago
From: Vasileios Amoiridis <vassilisamir@gmail.com>

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 | 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:
+		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;
 
-- 
2.43.0
Re: [PATCH v1 05/13] iio: chemical: bme680: refactorize set_mode() mode
Posted by Jonathan Cameron 1 month, 2 weeks ago
On Thu, 10 Oct 2024 23:00:22 +0200
vamoirid <vassilisamir@gmail.com> wrote:

> From: Vasileios Amoiridis <vassilisamir@gmail.com>
> 
> 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 | 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;
>
Re: [PATCH v1 05/13] iio: chemical: bme680: refactorize set_mode() mode
Posted by Andy Shevchenko 1 month, 2 weeks ago
On Thu, Oct 10, 2024 at 11:00:22PM +0200, vamoirid wrote:
> From: Vasileios Amoiridis <vassilisamir@gmail.com>
> 
> 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.

...

> -	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:
> +		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");

This is an information loss. You shuld probably still have a value of mode to
be printed.

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 05/13] iio: chemical: bme680: refactorize set_mode() mode
Posted by Vasileios Aoiridis 1 month, 2 weeks ago
On Fri, Oct 11, 2024 at 01:02:28PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 10, 2024 at 11:00:22PM +0200, vamoirid wrote:
> > From: Vasileios Amoiridis <vassilisamir@gmail.com>
> > 
> > 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.
> 
> ...
> 
> > -	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:
> > +		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");
> 
> This is an information loss. You shuld probably still have a value of mode to
> be printed.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>

You are right, I missed a return here.

Cheers,
Vasilis