Re: [PATCH v1 12/13] iio: chemical: bme680: Add triggered buffer support

[Andy Shevchenko]

On Thu, Oct 10, 2024 at 11:00:29PM +0200, vamoirid wrote:
> From: Vasileios Amoiridis <vassilisamir@xxxxxxxxx>
> 
> Add triggered buffer and soft timestamp support. The available scan mask
> enables all the channels of the sensor in order to follow the operation of
> the sensor. The sensor basically starts to capture from all channels
> as long as it enters into FORCED mode.

...

>  	struct regulator_bulk_data supplies[BME680_NUM_SUPPLIES];
>  	int ambient_temp;
>  
> +	u8 buffer[ALIGN(sizeof(s32) * BME680_NUM_CHANNELS, sizeof(s64))
> +		  + sizeof(s64)] __aligned(sizeof(s64));

Can it be represented as a structure?
We also have aligned_s64 for the timestamp.

>  	union {
> -		u8 buf[3];
> +		u8 buf[15];
>  		unsigned int check;
>  		__be16 be16;
>  		u8 bme680_cal_buf_1[BME680_CALIB_RANGE_1_LEN];

...

> +static irqreturn_t bme680_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct bme680_data *data = iio_priv(indio_dev);
> +	u32 adc_temp, adc_press, adc_humid, comp_press, comp_humid;

> +	s32 *chans = (s32 *)data->buffer;

With the structure in place this becomes much more readable.

> +	u16 adc_gas_res, gas_regs_val;
> +	s32 t_fine, comp_gas_res;
> +	s16 comp_temp;
> +	u8 gas_range;
> +	int ret;
> +
> +	guard(mutex)(&data->lock);
> +
> +	ret = bme680_set_mode(data, BME680_FORCED);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = bme680_wait_for_eoc(data);
> +	if (ret)
> +		goto out;
> +
> +	/* Burst read data regs */
> +	ret = regmap_bulk_read(data->regmap, BME680_REG_MEAS_STAT_0,
> +			       data->buf, sizeof(data->buf));
> +	if (ret) {
> +		dev_err(data->dev, "failed to burst read sensor data\n");
> +		goto out;
> +	}
> +	if (data->buf[0] & BME680_GAS_MEAS_BIT) {
> +		dev_err(data->dev, "gas measurement incomplete\n");
> +		goto out;
> +	}
> +
> +	/* Temperature calculations */
> +	adc_temp = FIELD_GET(BME680_MEAS_TRIM_MASK, get_unaligned_be24(&data->buf[5]));
> +	if (adc_temp == BME680_MEAS_SKIPPED) {
> +		dev_err(data->dev, "reading temperature skipped\n");
> +		goto out;
> +	}
> +	comp_temp = bme680_compensate_temp(data, adc_temp);
> +	t_fine = bme680_calc_t_fine(data, adc_temp);
> +
> +	/* Pressure calculations */
> +	adc_press = FIELD_GET(BME680_MEAS_TRIM_MASK, get_unaligned_be24(&data->buf[2]));
> +	if (adc_press == BME680_MEAS_SKIPPED) {
> +		dev_err(data->dev, "reading pressure skipped\n");
> +		goto out;
> +	}
> +	comp_press = bme680_compensate_press(data, adc_press, t_fine);

> +	pr_info("comp_press: %d\n", comp_press);

No debugging in the production code. Or if you really need that, it should
use dev_dbg().

> +	/* Humidity calculations */
> +	adc_humid = get_unaligned_be16(&data->buf[8]);
> +	if (adc_humid == BME680_MEAS_SKIPPED) {
> +		dev_err(data->dev, "reading humidity skipped\n");
> +		goto out;
> +	}
> +	comp_humid = bme680_compensate_humid(data, adc_humid, t_fine);

> +	pr_info("comp_humid: %d\n", comp_humid);

Ditto.

> +
> +	/* Gas calculations */
> +	gas_regs_val = get_unaligned_be16(&data->buf[13]);
> +	adc_gas_res = FIELD_GET(BME680_ADC_GAS_RES, gas_regs_val);
> +	if ((gas_regs_val & BME680_GAS_STAB_BIT) == 0) {
> +		dev_err(data->dev, "heater failed to reach the target temperature\n");
> +		goto out;
> +	}
> +	gas_range = FIELD_GET(BME680_GAS_RANGE_MASK, gas_regs_val);
> +	comp_gas_res = bme680_compensate_gas(data, adc_gas_res, gas_range);
> +	pr_info("comp_gas_res: %d\n", comp_gas_res);
> +
> +	chans[0] = comp_temp;
> +	chans[1] = comp_press;
> +	chans[2] = comp_humid;
> +	chans[3] = comp_gas_res;
> +
> +	/* Push to buffer */
> +	iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer,
> +					   iio_get_time_ns(indio_dev));
> +out:
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}

-- 
With Best Regards,
Andy Shevchenko