On Mon, 27 May 2024 20:37:58 +0200
Vasileios Amoiridis <vassilisamir@xxxxxxxxx> 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@xxxxxxxxx>
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!
>
> /*
> * 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.
>
> *val = bme680_compensate_gas(data, adc_gas_res, gas_range);
> return IIO_VAL_INT;
