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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zwl3aBDFiLN9b0TK@vamoirid-laptop>
Date: Fri, 11 Oct 2024 21:07:20 +0200
From: Vasileios Aoiridis <vassilisamir@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc: jic23@...nel.org, lars@...afoo.de, robh@...nel.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, anshulusr@...il.com, gustavograzs@...il.com,
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 12/13] iio: chemical: bme680: Add triggered buffer
 support

On Fri, Oct 11, 2024 at 01:37:56PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 10, 2024 at 11:00:29PM +0200, vamoirid wrote:
> > From: Vasileios Amoiridis <vassilisamir@...il.com>
> > 
> > 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.
>

Hi Andy,

The same approach was used also for the bmp280 driver and since I was
working on the bmp280 as well, I did it here. You think the
representation as a struct would look better? Personally I like the
nature of this one because of the ALIGN() but I have no problem of using
a struct here.

> >  	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().
> 

ACK. I shouldn't have forgotten those here..

> > +	/* 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.
> 

ACK.

> > +
> > +	/* 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
> 
> 

Cheers,
Vasilis