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@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!
>
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;
>
