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@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.
>
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
