Received-SPF: pass (google.com: domain of linux-kernel+bounces-198391-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: Vasileios Amoiridis <vassilisamir@gmail.com>
Date: Sun, 2 Jun 2024 21:25:59 +0200
To: Jonathan Cameron <jic23@kernel.org>
Cc: Vasileios Amoiridis <vassilisamir@gmail.com>, lars@metafoo.de,
	himanshujha199640@gmail.com, linux-iio@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 10/17] iio: chemical: bme680: Remove duplicate
 register read
Message-ID: <20240602192559.GC387181@vamoiridPC>
References: <20240527183805.311501-1-vassilisamir@gmail.com>
 <20240527183805.311501-11-vassilisamir@gmail.com>
 <20240602135036.27b0ee9a@jic23-huawei>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240602135036.27b0ee9a@jic23-huawei>

On Sun, Jun 02, 2024 at 01:50:36PM +0100, Jonathan Cameron wrote:
> On Mon, 27 May 2024 20:37:58 +0200
> Vasileios Amoiridis <vassilisamir@gmail.com> wrote:
> 
> > The LSB of the gas register was read first to check if the following
> > check was correct and then the MSB+LSB were read together. Simplify
> > this by reading together the MSB+LSB immediately.
> > 
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> A few trivial things. I wrote a nice comment on dma safe buffers then
> noticed this does a custom regmap using spi_write_then_read() so despite
> not looking like it from this code, DMA safe buffers are used.
> Just thought I'd mention it for any other reviewers!
> 
> > ---
> >  drivers/iio/chemical/bme680_core.c | 21 ++++++++-------------
> >  1 file changed, 8 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/iio/chemical/bme680_core.c b/drivers/iio/chemical/bme680_core.c
> > index 96423861c79a..681f271f9b06 100644
> > --- a/drivers/iio/chemical/bme680_core.c
> > +++ b/drivers/iio/chemical/bme680_core.c
> > @@ -748,7 +748,7 @@ static int bme680_read_gas(struct bme680_data *data,
> >  	int ret;
> >  	__be16 tmp = 0;
> >  	unsigned int check;
> > -	u16 adc_gas_res;
> > +	u16 adc_gas_res, gas_regs_val;
> >  	u8 gas_range;
> >  
> >  	/* Set heater settings */
> > @@ -771,11 +771,14 @@ static int bme680_read_gas(struct bme680_data *data,
> >  		return -EBUSY;
> >  	}
> >  
> > -	ret = regmap_read(data->regmap, BME680_REG_GAS_R_LSB, &check);
> > +	ret = regmap_bulk_read(data->regmap, BME680_REG_GAS_MSB,
> > +			       &tmp, sizeof(tmp)); 
> >  	if (ret < 0) {
> > -		dev_err(dev, "failed to read gas_r_lsb register\n");
> > +		dev_err(dev, "failed to read gas resistance\n");
> >  		return ret;
> >  	}
> > +	gas_regs_val = be16_to_cpu(tmp);
> > +	adc_gas_res = gas_regs_val >> BME680_ADC_GAS_RES_SHIFT;
> I'd rather see this as a FIELD_GET() but given this was what was originally
> here I guess I can cope with keeping it a little longer!
> 

Well, it's not a big deal, I could put it in a FIELD_GET().

> >  
> >  	/*
> >  	 * occurs if either the gas heating duration was insuffient
> > @@ -783,20 +786,12 @@ static int bme680_read_gas(struct bme680_data *data,
> >  	 * heater temperature was too high for the heater sink to
> >  	 * reach.
> >  	 */
> > -	if ((check & BME680_GAS_STAB_BIT) == 0) {
> > +	if ((gas_regs_val & BME680_GAS_STAB_BIT) == 0) {
> >  		dev_err(dev, "heater failed to reach the target temperature\n");
> >  		return -EINVAL;
> >  	}
> >  
> > -	ret = regmap_bulk_read(data->regmap, BME680_REG_GAS_MSB,
> > -			       &tmp, sizeof(tmp));
> > -	if (ret < 0) {
> > -		dev_err(dev, "failed to read gas resistance\n");
> > -		return ret;
> > -	}
> > -
> > -	gas_range = check & BME680_GAS_RANGE_MASK;
> > -	adc_gas_res = be16_to_cpu(tmp) >> BME680_ADC_GAS_RES_SHIFT;
> > +	gas_range = gas_regs_val & BME680_GAS_RANGE_MASK;
> 
> Whilst you are here, may this a FIELD_GET() so we don't have to go
> check that it includes the LSB.
> 

I could do it here as well.

Cheers,
Vasilis
> >  
> >  	*val = bme680_compensate_gas(data, adc_gas_res, gas_range);
> >  	return IIO_VAL_INT;
>