| Message ID | 20240527183805.311501-11-vassilisamir@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: chemical: bme680: Driver cleanup | expand |
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! > > /* > * 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;
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; >
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; /* * 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; *val = bme680_compensate_gas(data, adc_gas_res, gas_range); return IIO_VAL_INT;
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> --- drivers/iio/chemical/bme680_core.c | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-)