linux-kernel - Re: [PATCH v2 04/13] iio: chemical: bme680: refactorize set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZyFvg5JyA2IJp4v0@vamoirid-laptop>
Date: Wed, 30 Oct 2024 00:28:03 +0100
From: Vasileios Amoiridis <vassilisamir@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: lars@...afoo.de, robh@...nel.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, andriy.shevchenko@...ux.intel.com,
	anshulusr@...il.com, gustavograzs@...il.com,
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 04/13] iio: chemical: bme680: refactorize set_mode()
 mode

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@...il.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@...il.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;
> >  
>