* Re: [PATCH v5 10/10] iio: accel: adxl345: add FIFO with watermark events
@ 2024-12-10 4:17 kernel test robot
0 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2024-12-10 4:17 UTC (permalink / raw)
To: oe-kbuild; +Cc: lkp, Dan Carpenter
BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20241205171343.308963-11-l.rubusch@gmail.com>
References: <20241205171343.308963-11-l.rubusch@gmail.com>
TO: Lothar Rubusch <l.rubusch@gmail.com>
TO: lars@metafoo.de
TO: Michael.Hennerich@analog.com
TO: jic23@kernel.org
TO: robh@kernel.org
TO: krzk+dt@kernel.org
TO: conor+dt@kernel.org
CC: devicetree@vger.kernel.org
CC: linux-iio@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: eraretuya@gmail.com
CC: l.rubusch@gmail.com
Hi Lothar,
kernel test robot noticed the following build warnings:
[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on linus/master v6.13-rc2 next-20241209]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Lothar-Rubusch/iio-accel-adxl345-refrase-comment-on-probe/20241206-011802
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/20241205171343.308963-11-l.rubusch%40gmail.com
patch subject: [PATCH v5 10/10] iio: accel: adxl345: add FIFO with watermark events
:::::: branch date: 4 days ago
:::::: commit date: 4 days ago
config: nios2-randconfig-r072-20241210 (https://download.01.org/0day-ci/archive/20241210/202412101132.Kj6R6i3h-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 14.2.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202412101132.Kj6R6i3h-lkp@intel.com/
smatch warnings:
drivers/iio/accel/adxl345_core.c:321 adxl345_fifo_transfer() error: uninitialized symbol 'ret'.
drivers/iio/accel/adxl345_core.c:441 adxl345_event_handler() warn: unsigned 'int_stat' is never less than zero.
vim +/ret +321 drivers/iio/accel/adxl345_core.c
55d2386488598bb Lothar Rubusch 2024-12-05 296
55d2386488598bb Lothar Rubusch 2024-12-05 297 /**
55d2386488598bb Lothar Rubusch 2024-12-05 298 * adxl345_fifo_transfer() - Read samples number of elements.
55d2386488598bb Lothar Rubusch 2024-12-05 299 * @st: The instance of the state object of this sensor.
55d2386488598bb Lothar Rubusch 2024-12-05 300 * @samples: The number of lines in the FIFO referred to as fifo_entry,
55d2386488598bb Lothar Rubusch 2024-12-05 301 * a fifo_entry has 3 elements for X, Y and Z direction of 2 bytes each.
55d2386488598bb Lothar Rubusch 2024-12-05 302 *
55d2386488598bb Lothar Rubusch 2024-12-05 303 * It is recommended that a multiple-byte read of all registers be performed to
55d2386488598bb Lothar Rubusch 2024-12-05 304 * prevent a change in data between reads of sequential registers. That is to
55d2386488598bb Lothar Rubusch 2024-12-05 305 * read out the data registers X0, X1, Y0, Y1, Z0, Z1 at once.
55d2386488598bb Lothar Rubusch 2024-12-05 306 *
55d2386488598bb Lothar Rubusch 2024-12-05 307 * Return: 0 or error value.
55d2386488598bb Lothar Rubusch 2024-12-05 308 */
55d2386488598bb Lothar Rubusch 2024-12-05 309 static int adxl345_fifo_transfer(struct adxl345_state *st, int samples)
55d2386488598bb Lothar Rubusch 2024-12-05 310 {
55d2386488598bb Lothar Rubusch 2024-12-05 311 size_t count;
55d2386488598bb Lothar Rubusch 2024-12-05 312 int i, ret;
55d2386488598bb Lothar Rubusch 2024-12-05 313
55d2386488598bb Lothar Rubusch 2024-12-05 314 count = sizeof(st->fifo_buf[0]) * ADXL345_DIRS;
55d2386488598bb Lothar Rubusch 2024-12-05 315 for (i = 0; i < samples; i++) {
55d2386488598bb Lothar Rubusch 2024-12-05 316 ret = regmap_noinc_read(st->regmap, ADXL345_REG_XYZ_BASE,
55d2386488598bb Lothar Rubusch 2024-12-05 317 st->fifo_buf + (i * count / 2), count);
55d2386488598bb Lothar Rubusch 2024-12-05 318 if (ret < 0)
55d2386488598bb Lothar Rubusch 2024-12-05 319 return ret;
55d2386488598bb Lothar Rubusch 2024-12-05 320 }
55d2386488598bb Lothar Rubusch 2024-12-05 @321 return ret;
55d2386488598bb Lothar Rubusch 2024-12-05 322 }
55d2386488598bb Lothar Rubusch 2024-12-05 323
55d2386488598bb Lothar Rubusch 2024-12-05 324 /**
55d2386488598bb Lothar Rubusch 2024-12-05 325 * adxl345_fifo_reset() - Empty the FIFO in error condition.
55d2386488598bb Lothar Rubusch 2024-12-05 326 * @st: The instance to the state object of the sensor.
55d2386488598bb Lothar Rubusch 2024-12-05 327 *
55d2386488598bb Lothar Rubusch 2024-12-05 328 * Read all elements of the FIFO. Reading the interrupt source register
55d2386488598bb Lothar Rubusch 2024-12-05 329 * resets the sensor.
55d2386488598bb Lothar Rubusch 2024-12-05 330 */
55d2386488598bb Lothar Rubusch 2024-12-05 331 static void adxl345_fifo_reset(struct adxl345_state *st)
55d2386488598bb Lothar Rubusch 2024-12-05 332 {
55d2386488598bb Lothar Rubusch 2024-12-05 333 int regval;
55d2386488598bb Lothar Rubusch 2024-12-05 334 int samples;
55d2386488598bb Lothar Rubusch 2024-12-05 335
55d2386488598bb Lothar Rubusch 2024-12-05 336 adxl345_set_measure_en(st, false);
55d2386488598bb Lothar Rubusch 2024-12-05 337
55d2386488598bb Lothar Rubusch 2024-12-05 338 samples = adxl345_get_samples(st);
55d2386488598bb Lothar Rubusch 2024-12-05 339 if (samples > 0)
55d2386488598bb Lothar Rubusch 2024-12-05 340 adxl345_fifo_transfer(st, samples);
55d2386488598bb Lothar Rubusch 2024-12-05 341
55d2386488598bb Lothar Rubusch 2024-12-05 342 regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, ®val);
55d2386488598bb Lothar Rubusch 2024-12-05 343
55d2386488598bb Lothar Rubusch 2024-12-05 344 adxl345_set_measure_en(st, true);
55d2386488598bb Lothar Rubusch 2024-12-05 345 }
55d2386488598bb Lothar Rubusch 2024-12-05 346
55d2386488598bb Lothar Rubusch 2024-12-05 347 static int adxl345_buffer_postenable(struct iio_dev *indio_dev)
55d2386488598bb Lothar Rubusch 2024-12-05 348 {
55d2386488598bb Lothar Rubusch 2024-12-05 349 struct adxl345_state *st = iio_priv(indio_dev);
55d2386488598bb Lothar Rubusch 2024-12-05 350 int ret;
55d2386488598bb Lothar Rubusch 2024-12-05 351
55d2386488598bb Lothar Rubusch 2024-12-05 352 ret = adxl345_set_interrupts(st);
55d2386488598bb Lothar Rubusch 2024-12-05 353 if (ret < 0)
55d2386488598bb Lothar Rubusch 2024-12-05 354 return ret;
55d2386488598bb Lothar Rubusch 2024-12-05 355
55d2386488598bb Lothar Rubusch 2024-12-05 356 st->fifo_mode = ADXL345_FIFO_STREAM;
55d2386488598bb Lothar Rubusch 2024-12-05 357 return adxl345_set_fifo(st);
55d2386488598bb Lothar Rubusch 2024-12-05 358 }
55d2386488598bb Lothar Rubusch 2024-12-05 359
55d2386488598bb Lothar Rubusch 2024-12-05 360 static int adxl345_buffer_predisable(struct iio_dev *indio_dev)
55d2386488598bb Lothar Rubusch 2024-12-05 361 {
55d2386488598bb Lothar Rubusch 2024-12-05 362 struct adxl345_state *st = iio_priv(indio_dev);
55d2386488598bb Lothar Rubusch 2024-12-05 363 int ret;
55d2386488598bb Lothar Rubusch 2024-12-05 364
55d2386488598bb Lothar Rubusch 2024-12-05 365 st->int_map = 0x00;
55d2386488598bb Lothar Rubusch 2024-12-05 366
55d2386488598bb Lothar Rubusch 2024-12-05 367 ret = adxl345_set_interrupts(st);
55d2386488598bb Lothar Rubusch 2024-12-05 368 if (ret < 0)
55d2386488598bb Lothar Rubusch 2024-12-05 369 return ret;
55d2386488598bb Lothar Rubusch 2024-12-05 370
55d2386488598bb Lothar Rubusch 2024-12-05 371 st->fifo_mode = ADXL345_FIFO_BYPASS;
55d2386488598bb Lothar Rubusch 2024-12-05 372 return adxl345_set_fifo(st);
55d2386488598bb Lothar Rubusch 2024-12-05 373 }
55d2386488598bb Lothar Rubusch 2024-12-05 374
55d2386488598bb Lothar Rubusch 2024-12-05 375 static const struct iio_buffer_setup_ops adxl345_buffer_ops = {
55d2386488598bb Lothar Rubusch 2024-12-05 376 .postenable = adxl345_buffer_postenable,
55d2386488598bb Lothar Rubusch 2024-12-05 377 .predisable = adxl345_buffer_predisable,
55d2386488598bb Lothar Rubusch 2024-12-05 378 };
55d2386488598bb Lothar Rubusch 2024-12-05 379
55d2386488598bb Lothar Rubusch 2024-12-05 380 static int adxl345_get_status(struct adxl345_state *st)
55d2386488598bb Lothar Rubusch 2024-12-05 381 {
55d2386488598bb Lothar Rubusch 2024-12-05 382 int ret;
55d2386488598bb Lothar Rubusch 2024-12-05 383 unsigned int regval;
55d2386488598bb Lothar Rubusch 2024-12-05 384
55d2386488598bb Lothar Rubusch 2024-12-05 385 ret = regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, ®val);
55d2386488598bb Lothar Rubusch 2024-12-05 386 if (ret < 0)
55d2386488598bb Lothar Rubusch 2024-12-05 387 return ret;
55d2386488598bb Lothar Rubusch 2024-12-05 388
55d2386488598bb Lothar Rubusch 2024-12-05 389 return (0xff & regval);
55d2386488598bb Lothar Rubusch 2024-12-05 390 }
55d2386488598bb Lothar Rubusch 2024-12-05 391
55d2386488598bb Lothar Rubusch 2024-12-05 392 static int adxl345_fifo_push(struct iio_dev *indio_dev,
55d2386488598bb Lothar Rubusch 2024-12-05 393 int samples)
55d2386488598bb Lothar Rubusch 2024-12-05 394 {
55d2386488598bb Lothar Rubusch 2024-12-05 395 struct adxl345_state *st = iio_priv(indio_dev);
55d2386488598bb Lothar Rubusch 2024-12-05 396 int i, ret;
55d2386488598bb Lothar Rubusch 2024-12-05 397
55d2386488598bb Lothar Rubusch 2024-12-05 398 if (samples <= 0)
55d2386488598bb Lothar Rubusch 2024-12-05 399 return -EINVAL;
55d2386488598bb Lothar Rubusch 2024-12-05 400
55d2386488598bb Lothar Rubusch 2024-12-05 401 ret = adxl345_fifo_transfer(st, samples);
55d2386488598bb Lothar Rubusch 2024-12-05 402 if (ret)
55d2386488598bb Lothar Rubusch 2024-12-05 403 return ret;
55d2386488598bb Lothar Rubusch 2024-12-05 404
55d2386488598bb Lothar Rubusch 2024-12-05 405 for (i = 0; i < ADXL345_DIRS * samples; i += ADXL345_DIRS) {
55d2386488598bb Lothar Rubusch 2024-12-05 406 /*
55d2386488598bb Lothar Rubusch 2024-12-05 407 * To ensure that the FIFO has completely popped, there must be at least 5
55d2386488598bb Lothar Rubusch 2024-12-05 408 * us between the end of reading the data registers, signified by the
55d2386488598bb Lothar Rubusch 2024-12-05 409 * transition to register 0x38 from 0x37 or the CS pin going high, and the
55d2386488598bb Lothar Rubusch 2024-12-05 410 * start of new reads of the FIFO or reading the FIFO_STATUS register. For
55d2386488598bb Lothar Rubusch 2024-12-05 411 * SPI operation at 1.5 MHz or lower, the register addressing portion of the
55d2386488598bb Lothar Rubusch 2024-12-05 412 * transmission is sufficient delay to ensure the FIFO has completely
55d2386488598bb Lothar Rubusch 2024-12-05 413 * popped. It is necessary for SPI operation greater than 1.5 MHz to
55d2386488598bb Lothar Rubusch 2024-12-05 414 * de-assert the CS pin to ensure a total of 5 us, which is at most 3.4 us
55d2386488598bb Lothar Rubusch 2024-12-05 415 * at 5 MHz operation.
55d2386488598bb Lothar Rubusch 2024-12-05 416 */
55d2386488598bb Lothar Rubusch 2024-12-05 417 if (st->fifo_delay && (samples > 1))
55d2386488598bb Lothar Rubusch 2024-12-05 418 udelay(3);
55d2386488598bb Lothar Rubusch 2024-12-05 419
55d2386488598bb Lothar Rubusch 2024-12-05 420 iio_push_to_buffers(indio_dev, &st->fifo_buf[i]);
55d2386488598bb Lothar Rubusch 2024-12-05 421 }
55d2386488598bb Lothar Rubusch 2024-12-05 422
55d2386488598bb Lothar Rubusch 2024-12-05 423 return 0;
55d2386488598bb Lothar Rubusch 2024-12-05 424 }
55d2386488598bb Lothar Rubusch 2024-12-05 425
55d2386488598bb Lothar Rubusch 2024-12-05 426 /**
55d2386488598bb Lothar Rubusch 2024-12-05 427 * adxl345_event_handler() - Handle events of the ADXL345.
55d2386488598bb Lothar Rubusch 2024-12-05 428 * @irq: The irq being handled.
55d2386488598bb Lothar Rubusch 2024-12-05 429 * @p: The struct iio_device pointer for the device.
55d2386488598bb Lothar Rubusch 2024-12-05 430 *
55d2386488598bb Lothar Rubusch 2024-12-05 431 * Return: The interrupt was handled.
55d2386488598bb Lothar Rubusch 2024-12-05 432 */
55d2386488598bb Lothar Rubusch 2024-12-05 433 static irqreturn_t adxl345_event_handler(int irq, void *p)
55d2386488598bb Lothar Rubusch 2024-12-05 434 {
55d2386488598bb Lothar Rubusch 2024-12-05 435 struct iio_dev *indio_dev = p;
55d2386488598bb Lothar Rubusch 2024-12-05 436 struct adxl345_state *st = iio_priv(indio_dev);
55d2386488598bb Lothar Rubusch 2024-12-05 437 u8 int_stat;
55d2386488598bb Lothar Rubusch 2024-12-05 438 int samples;
55d2386488598bb Lothar Rubusch 2024-12-05 439
55d2386488598bb Lothar Rubusch 2024-12-05 440 int_stat = adxl345_get_status(st);
55d2386488598bb Lothar Rubusch 2024-12-05 @441 if (int_stat < 0)
55d2386488598bb Lothar Rubusch 2024-12-05 442 return IRQ_NONE;
55d2386488598bb Lothar Rubusch 2024-12-05 443
55d2386488598bb Lothar Rubusch 2024-12-05 444 if (int_stat == 0x0)
55d2386488598bb Lothar Rubusch 2024-12-05 445 goto err;
55d2386488598bb Lothar Rubusch 2024-12-05 446
55d2386488598bb Lothar Rubusch 2024-12-05 447 if (int_stat & ADXL345_INT_OVERRUN)
55d2386488598bb Lothar Rubusch 2024-12-05 448 goto err;
55d2386488598bb Lothar Rubusch 2024-12-05 449
55d2386488598bb Lothar Rubusch 2024-12-05 450 if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) {
55d2386488598bb Lothar Rubusch 2024-12-05 451 samples = adxl345_get_samples(st);
55d2386488598bb Lothar Rubusch 2024-12-05 452 if (samples < 0)
55d2386488598bb Lothar Rubusch 2024-12-05 453 goto err;
55d2386488598bb Lothar Rubusch 2024-12-05 454
55d2386488598bb Lothar Rubusch 2024-12-05 455 if (adxl345_fifo_push(indio_dev, samples) < 0)
55d2386488598bb Lothar Rubusch 2024-12-05 456 goto err;
55d2386488598bb Lothar Rubusch 2024-12-05 457
55d2386488598bb Lothar Rubusch 2024-12-05 458 }
55d2386488598bb Lothar Rubusch 2024-12-05 459 return IRQ_HANDLED;
55d2386488598bb Lothar Rubusch 2024-12-05 460
55d2386488598bb Lothar Rubusch 2024-12-05 461 err:
55d2386488598bb Lothar Rubusch 2024-12-05 462 adxl345_fifo_reset(st);
55d2386488598bb Lothar Rubusch 2024-12-05 463
55d2386488598bb Lothar Rubusch 2024-12-05 464 return IRQ_HANDLED;
55d2386488598bb Lothar Rubusch 2024-12-05 465 }
55d2386488598bb Lothar Rubusch 2024-12-05 466
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH v5 00/10] iio: accel: adxl345: add FIFO operating with IRQ triggered watermark events @ 2024-12-05 17:13 Lothar Rubusch 2024-12-05 17:13 ` [PATCH v5 10/10] iio: accel: adxl345: add FIFO with " Lothar Rubusch 0 siblings, 1 reply; 8+ messages in thread From: Lothar Rubusch @ 2024-12-05 17:13 UTC (permalink / raw) To: lars, Michael.Hennerich, jic23, robh, krzk+dt, conor+dt Cc: devicetree, linux-iio, linux-kernel, eraretuya, l.rubusch The adxl345 sensor offers several features. Most of them are based on using the hardware FIFO and reacting on events coming in on an interrupt line. Add access to configure and read out the FIFO, handling of interrupts and configuration and application of the watermark feature on that FIFO. Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> --- Although I tried to implement most of the requested changes, now the code is simplified and clearer, I still encounter some issues. 1) Unsure if my way reading out the FIFO elements with a regmap_noinc_read() is supposed to be like that. TBH, isn't there a better way, say, to configure the channel correctly and this is handled by the iio API internally? As I understood from v2 there might be a way using available data, also in iio_info I see now as buffer attributes an available data field. Where can I find information how to use it or did I get this wrong? 2) Overrun handling: I'm trying to reset the FIFO and registers. Unsure, if this is the correct dealing here. 3) I can see the IRQs coming in, and with a `watch -n 0.1 iio_info` I can see the correct fields changing. I tried the follwoing down below, but the iio_readdev shows me the following result. I don't quite understand if I still have an issue here, or if this is a calibration thing? # iio_info Library version: 0.23 (git tag: v0.23) Compiled with backends: local xml ip usb IIO context created with local backend. Backend version: 0.23 (git tag: v0.23) Backend description string: Linux dut1138 6.6.21-lothar02 #3 SMP PREEMPT Wed Nov 6 21:21:14 UTC 2024 aarch64 IIO context has 2 attributes: local,kernel: 6.6.21-lothar02 uri: local: IIO context has 1 devices: iio:device0: adxl345 (buffer capable) 3 channels found: accel_x: (input, index: 0, format: le:s13/16>>0) 4 channel-specific attributes found: attr 0: calibbias value: 0 attr 1: raw value: -14 <--- CHANGES attr 2: sampling_frequency value: 100.000000000 attr 3: scale value: 0.038300 accel_y: (input, index: 1, format: le:s13/16>>0) 4 channel-specific attributes found: attr 0: calibbias value: 0 attr 1: raw value: 6 <--- CHANGES attr 2: sampling_frequency value: 100.000000000 attr 3: scale value: 0.038300 accel_z: (input, index: 2, format: le:s13/16>>0) 4 channel-specific attributes found: attr 0: calibbias value: 0 attr 1: raw value: 247 <--- CHANGES attr 2: sampling_frequency value: 100.000000000 attr 3: scale value: 0.038300 2 device-specific attributes found: attr 0: sampling_frequency_available value: 0.09765625 0.1953125 0.390625 0.78125 1.5625 3.125 6.25 12.5 25 50 100 200 400 800 1600 3200 attr 1: waiting_for_supplier value: 0 3 buffer-specific attributes found: attr 0: data_available value: 13 attr 1: direction value: in attr 2: watermark value: 15 No trigger on this device Above I marked what keeps changing with "CHANGES", that's what I expect. Then with readdev I obtain the following result. # iio_attr -c adxl345 dev 'adxl345', channel 'accel_x' (input, index: 0, format: le:s13/16>>0), found 4 channel-specific attributes dev 'adxl345', channel 'accel_y' (input, index: 1, format: le:s13/16>>0), found 4 channel-specific attributes dev 'adxl345', channel 'accel_z' (input, index: 2, format: le:s13/16>>0), found 4 channel-specific attributes # echo 1 > ./scan_elements/in_accel_x_en # echo 1 > ./scan_elements/in_accel_y_en # echo 1 > ./scan_elements/in_accel_z_en # echo 32 > ./buffer0/length # echo 15 > ./buffer0/watermark # echo 1 > ./buffer0/enable # iio_readdev -b 16 -s 21 adxl345 > samples.dat # hexdump -d ./samples.dat 0000000 65523 00006 00248 65523 00005 00235 65522 00006 0000010 00248 65522 00006 00248 65521 00005 00247 65522 0000020 00007 00249 65523 00005 00249 65521 00006 00248 0000030 65522 00006 00248 65522 00006 00250 65522 00006 0000040 00249 65522 00005 00248 65523 00005 00248 65521 0000050 00007 00248 65521 00006 00250 65522 00005 00248 0000060 65521 00006 00248 65522 00007 00247 65522 00006 0000070 00248 65522 00006 00248 65521 00004 00250 000007e Am I doing this actually correctly? --- v4 -> v5: - fix dt-binding for enum array of INT1 and INT2 v3 -> v4: - fix dt-binding indention v2 -> v3: - reorganize commits, merge the watermark handling - INT lines are defined by binding - kfifo is prepared by devm_iio_kfifo_buffer_setup() - event handler is registered w/ devm_request_threaded_irq() v1 -> v2: Fix comments according to Documentation/doc-guide/kernel-doc.rst and missing static declaration of function. --- Lothar Rubusch (10): iio: accel: adxl345: refrase comment on probe iio: accel: adxl345: rename variable data to st iio: accel: adxl345: measure right-justified iio: accel: adxl345: add function to switch measuring mode iio: accel: adxl345: complete list of defines dt-bindings: iio: accel: add interrupt-names iio: accel: adxl345: introduce interrupt handling iio: accel: adxl345: initialize FIFO delay value for SPI iio: accel: adxl345: prepare channel for scan_index iio: accel: adxl345: add FIFO with watermark events .../bindings/iio/accel/adi,adxl345.yaml | 7 + drivers/iio/accel/adxl345.h | 90 +++- drivers/iio/accel/adxl345_core.c | 427 ++++++++++++++++-- drivers/iio/accel/adxl345_i2c.c | 2 +- drivers/iio/accel/adxl345_spi.c | 7 +- 5 files changed, 478 insertions(+), 55 deletions(-) -- 2.39.2 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v5 10/10] iio: accel: adxl345: add FIFO with watermark events 2024-12-05 17:13 [PATCH v5 00/10] iio: accel: adxl345: add FIFO operating with IRQ triggered " Lothar Rubusch @ 2024-12-05 17:13 ` Lothar Rubusch 2024-12-08 16:34 ` Jonathan Cameron 2024-12-10 8:47 ` Dan Carpenter 0 siblings, 2 replies; 8+ messages in thread From: Lothar Rubusch @ 2024-12-05 17:13 UTC (permalink / raw) To: lars, Michael.Hennerich, jic23, robh, krzk+dt, conor+dt Cc: devicetree, linux-iio, linux-kernel, eraretuya, l.rubusch Add a basic setup for FIFO with configurable watermark. Add a handler for watermark interrupt events and extend the channel for the scan_index needed for the iio channel. The sensor is configurable to use a FIFO_BYPASSED mode or a FIFO_STREAM mode. For the FIFO_STREAM mode now a watermark can be configured, or disabled by setting 0. Further features require a working FIFO setup. Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> --- drivers/iio/accel/adxl345_core.c | 300 +++++++++++++++++++++++++++++++ 1 file changed, 300 insertions(+) diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c index 3067a70c54e..58ed82d66dc 100644 --- a/drivers/iio/accel/adxl345_core.c +++ b/drivers/iio/accel/adxl345_core.c @@ -15,15 +15,28 @@ #include <linux/iio/iio.h> #include <linux/iio/sysfs.h> +#include <linux/iio/buffer.h> +#include <linux/iio/events.h> +#include <linux/iio/kfifo_buf.h> #include "adxl345.h" +#define ADXL345_FIFO_BYPASS 0 +#define ADXL345_FIFO_FIFO 1 +#define ADXL345_FIFO_STREAM 2 + +#define ADXL345_DIRS 3 + struct adxl345_state { int irq; const struct adxl345_chip_info *info; struct regmap *regmap; + __le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE]; bool fifo_delay; /* delay: delay is needed for SPI */ u8 intio; + u8 int_map; + u8 watermark; + u8 fifo_mode; }; #define ADXL345_CHANNEL(index, reg, axis) { \ @@ -36,6 +49,13 @@ struct adxl345_state { .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \ BIT(IIO_CHAN_INFO_SAMP_FREQ), \ .scan_index = (index), \ + .scan_type = { \ + .sign = 's', \ + .realbits = 13, \ + .storagebits = 16, \ + .shift = 0, \ + .endianness = IIO_LE, \ + }, \ } enum adxl345_chans { @@ -48,6 +68,25 @@ static const struct iio_chan_spec adxl345_channels[] = { ADXL345_CHANNEL(2, chan_z, Z), }; +static int adxl345_set_interrupts(struct adxl345_state *st) +{ + int ret; + unsigned int int_enable = st->int_map; + unsigned int int_map; + + /* Any bits set to 0 in the INT map register send their respective + * interrupts to the INT1 pin, whereas bits set to 1 send their respective + * interrupts to the INT2 pin. The intio shall convert this accordingly. + */ + int_map = 0xFF & (st->intio ? st->int_map : ~st->int_map); + + ret = regmap_write(st->regmap, ADXL345_REG_INT_MAP, int_map); + if (ret) + return ret; + + return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, int_enable); +} + static int adxl345_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int *val, int *val2, long mask) @@ -133,6 +172,31 @@ static int adxl345_write_raw(struct iio_dev *indio_dev, return -EINVAL; } +static int adxl345_set_watermark(struct iio_dev *indio_dev, unsigned int value) +{ + struct adxl345_state *st = iio_priv(indio_dev); + unsigned int fifo_mask = 0x1F; + int ret; + + if (value == 0) { + st->int_map &= ~ADXL345_INT_WATERMARK; + return 0; + } + + if (value > ADXL345_FIFO_SIZE) + value = ADXL345_FIFO_SIZE; + + ret = regmap_update_bits(st->regmap, ADXL345_REG_FIFO_CTL, + fifo_mask, value); + if (ret) + return ret; + + st->watermark = value; + st->int_map |= ADXL345_INT_WATERMARK; + + return 0; +} + static int adxl345_write_raw_get_fmt(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, long mask) @@ -188,11 +252,224 @@ static const struct attribute_group adxl345_attrs_group = { .attrs = adxl345_attrs, }; +static int adxl345_set_fifo(struct adxl345_state *st) +{ + u8 fifo_ctl; + int ret; + + /* FIFO should only be configured while in standby mode */ + ret = adxl345_set_measure_en(st, false); + if (ret < 0) + return ret; + + fifo_ctl = ADXL345_FIFO_CTL_SAMLPES(st->watermark) | + ADXL345_FIFO_CTL_TRIGGER(st->intio) | + ADXL345_FIFO_CTL_MODE(st->fifo_mode); + + ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL, fifo_ctl); + if (ret < 0) + return ret; + + return adxl345_set_measure_en(st, true); +} + +/** + * adxl345_get_samples() - Read number of FIFO entries. + * @st: The initialized state instance of this driver. + * + * The sensor does not support treating any axis individually, or exclude them + * from measuring. + * + * Return: negative error, or value. + */ +static int adxl345_get_samples(struct adxl345_state *st) +{ + unsigned int regval = 0; + int ret; + + ret = regmap_read(st->regmap, ADXL345_REG_FIFO_STATUS, ®val); + if (ret < 0) + return ret; + + return 0x3f & regval; +} + +/** + * adxl345_fifo_transfer() - Read samples number of elements. + * @st: The instance of the state object of this sensor. + * @samples: The number of lines in the FIFO referred to as fifo_entry, + * a fifo_entry has 3 elements for X, Y and Z direction of 2 bytes each. + * + * It is recommended that a multiple-byte read of all registers be performed to + * prevent a change in data between reads of sequential registers. That is to + * read out the data registers X0, X1, Y0, Y1, Z0, Z1 at once. + * + * Return: 0 or error value. + */ +static int adxl345_fifo_transfer(struct adxl345_state *st, int samples) +{ + size_t count; + int i, ret; + + count = sizeof(st->fifo_buf[0]) * ADXL345_DIRS; + for (i = 0; i < samples; i++) { + ret = regmap_noinc_read(st->regmap, ADXL345_REG_XYZ_BASE, + st->fifo_buf + (i * count / 2), count); + if (ret < 0) + return ret; + } + return ret; +} + +/** + * adxl345_fifo_reset() - Empty the FIFO in error condition. + * @st: The instance to the state object of the sensor. + * + * Read all elements of the FIFO. Reading the interrupt source register + * resets the sensor. + */ +static void adxl345_fifo_reset(struct adxl345_state *st) +{ + int regval; + int samples; + + adxl345_set_measure_en(st, false); + + samples = adxl345_get_samples(st); + if (samples > 0) + adxl345_fifo_transfer(st, samples); + + regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, ®val); + + adxl345_set_measure_en(st, true); +} + +static int adxl345_buffer_postenable(struct iio_dev *indio_dev) +{ + struct adxl345_state *st = iio_priv(indio_dev); + int ret; + + ret = adxl345_set_interrupts(st); + if (ret < 0) + return ret; + + st->fifo_mode = ADXL345_FIFO_STREAM; + return adxl345_set_fifo(st); +} + +static int adxl345_buffer_predisable(struct iio_dev *indio_dev) +{ + struct adxl345_state *st = iio_priv(indio_dev); + int ret; + + st->int_map = 0x00; + + ret = adxl345_set_interrupts(st); + if (ret < 0) + return ret; + + st->fifo_mode = ADXL345_FIFO_BYPASS; + return adxl345_set_fifo(st); +} + +static const struct iio_buffer_setup_ops adxl345_buffer_ops = { + .postenable = adxl345_buffer_postenable, + .predisable = adxl345_buffer_predisable, +}; + +static int adxl345_get_status(struct adxl345_state *st) +{ + int ret; + unsigned int regval; + + ret = regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, ®val); + if (ret < 0) + return ret; + + return (0xff & regval); +} + +static int adxl345_fifo_push(struct iio_dev *indio_dev, + int samples) +{ + struct adxl345_state *st = iio_priv(indio_dev); + int i, ret; + + if (samples <= 0) + return -EINVAL; + + ret = adxl345_fifo_transfer(st, samples); + if (ret) + return ret; + + for (i = 0; i < ADXL345_DIRS * samples; i += ADXL345_DIRS) { + /* + * To ensure that the FIFO has completely popped, there must be at least 5 + * us between the end of reading the data registers, signified by the + * transition to register 0x38 from 0x37 or the CS pin going high, and the + * start of new reads of the FIFO or reading the FIFO_STATUS register. For + * SPI operation at 1.5 MHz or lower, the register addressing portion of the + * transmission is sufficient delay to ensure the FIFO has completely + * popped. It is necessary for SPI operation greater than 1.5 MHz to + * de-assert the CS pin to ensure a total of 5 us, which is at most 3.4 us + * at 5 MHz operation. + */ + if (st->fifo_delay && (samples > 1)) + udelay(3); + + iio_push_to_buffers(indio_dev, &st->fifo_buf[i]); + } + + return 0; +} + +/** + * adxl345_event_handler() - Handle events of the ADXL345. + * @irq: The irq being handled. + * @p: The struct iio_device pointer for the device. + * + * Return: The interrupt was handled. + */ +static irqreturn_t adxl345_event_handler(int irq, void *p) +{ + struct iio_dev *indio_dev = p; + struct adxl345_state *st = iio_priv(indio_dev); + u8 int_stat; + int samples; + + int_stat = adxl345_get_status(st); + if (int_stat < 0) + return IRQ_NONE; + + if (int_stat == 0x0) + goto err; + + if (int_stat & ADXL345_INT_OVERRUN) + goto err; + + if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) { + samples = adxl345_get_samples(st); + if (samples < 0) + goto err; + + if (adxl345_fifo_push(indio_dev, samples) < 0) + goto err; + + } + return IRQ_HANDLED; + +err: + adxl345_fifo_reset(st); + + return IRQ_HANDLED; +} + static const struct iio_info adxl345_info = { .attrs = &adxl345_attrs_group, .read_raw = adxl345_read_raw, .write_raw = adxl345_write_raw, .write_raw_get_fmt = adxl345_write_raw_get_fmt, + .hwfifo_set_watermark = adxl345_set_watermark, }; /** @@ -222,6 +499,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE | ADXL345_DATA_FORMAT_FULL_RES | ADXL345_DATA_FORMAT_SELF_TEST); + u8 fifo_ctl; int ret; indio_dev = devm_iio_device_alloc(dev, sizeof(*st)); @@ -293,6 +571,28 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, if (ret < 0) return dev_err_probe(dev, ret, "Failed to add action or reset\n"); + if (st->irq > 0) { + dev_dbg(dev, "initialize for FIFO_STREAM mode\n"); + + ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &adxl345_buffer_ops); + if (ret) + return ret; + + ret = devm_request_threaded_irq(dev, st->irq, NULL, &adxl345_event_handler, + IRQF_SHARED | IRQF_ONESHOT, + indio_dev->name, indio_dev); + if (ret) + return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n"); + + } else { + dev_dbg(dev, "initialize for FIFO_BYPASS mode (fallback)\n"); + + fifo_ctl = ADXL345_FIFO_CTL_MODE(ADXL345_FIFO_BYPASS); + + ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL, fifo_ctl); + if (ret < 0) + return ret; + } return devm_iio_device_register(dev, indio_dev); } EXPORT_SYMBOL_NS_GPL(adxl345_core_probe, IIO_ADXL345); -- 2.39.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v5 10/10] iio: accel: adxl345: add FIFO with watermark events 2024-12-05 17:13 ` [PATCH v5 10/10] iio: accel: adxl345: add FIFO with " Lothar Rubusch @ 2024-12-08 16:34 ` Jonathan Cameron 2024-12-10 21:54 ` Lothar Rubusch 2024-12-10 8:47 ` Dan Carpenter 1 sibling, 1 reply; 8+ messages in thread From: Jonathan Cameron @ 2024-12-08 16:34 UTC (permalink / raw) To: Lothar Rubusch Cc: lars, Michael.Hennerich, robh, krzk+dt, conor+dt, devicetree, linux-iio, linux-kernel, eraretuya On Thu, 5 Dec 2024 17:13:43 +0000 Lothar Rubusch <l.rubusch@gmail.com> wrote: > Add a basic setup for FIFO with configurable watermark. Add a handler > for watermark interrupt events and extend the channel for the > scan_index needed for the iio channel. The sensor is configurable to use > a FIFO_BYPASSED mode or a FIFO_STREAM mode. For the FIFO_STREAM mode now > a watermark can be configured, or disabled by setting 0. Further features > require a working FIFO setup. > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> Various comments inline. Thanks, Jonathan > --- > drivers/iio/accel/adxl345_core.c | 300 +++++++++++++++++++++++++++++++ > 1 file changed, 300 insertions(+) > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c > index 3067a70c54e..58ed82d66dc 100644 > --- a/drivers/iio/accel/adxl345_core.c > +++ b/drivers/iio/accel/adxl345_core.c > @@ -15,15 +15,28 @@ > > #include <linux/iio/iio.h> > #include <linux/iio/sysfs.h> > +#include <linux/iio/buffer.h> > +#include <linux/iio/events.h> I'm not seeing any use of this header yet. Bring it in when you need it. > +#include <linux/iio/kfifo_buf.h> > > #include "adxl345.h" > > +#define ADXL345_FIFO_BYPASS 0 > +#define ADXL345_FIFO_FIFO 1 > +#define ADXL345_FIFO_STREAM 2 > + > +#define ADXL345_DIRS 3 > + > struct adxl345_state { > int irq; > const struct adxl345_chip_info *info; > struct regmap *regmap; > + __le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE]; > bool fifo_delay; /* delay: delay is needed for SPI */ > u8 intio; > + u8 int_map; > + u8 watermark; > + u8 fifo_mode; > }; > > #define ADXL345_CHANNEL(index, reg, axis) { \ > @@ -36,6 +49,13 @@ struct adxl345_state { > .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \ > BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > .scan_index = (index), \ > + .scan_type = { \ > + .sign = 's', \ > + .realbits = 13, \ > + .storagebits = 16, \ > + .shift = 0, \ No need to specify shift of 0. It's the 'obvious' default and C will set it to 0 for you anyway. > + .endianness = IIO_LE, \ > + }, \ > } > > enum adxl345_chans { > @@ -48,6 +68,25 @@ static const struct iio_chan_spec adxl345_channels[] = { > ADXL345_CHANNEL(2, chan_z, Z), > }; > > +static int adxl345_set_interrupts(struct adxl345_state *st) > +{ > + int ret; > + unsigned int int_enable = st->int_map; > + unsigned int int_map; > + > + /* Any bits set to 0 in the INT map register send their respective /* * Any bits.... > + * interrupts to the INT1 pin, whereas bits set to 1 send their respective > + * interrupts to the INT2 pin. The intio shall convert this accordingly. > + */ > + int_map = 0xFF & (st->intio ? st->int_map : ~st->int_map); > + > + ret = regmap_write(st->regmap, ADXL345_REG_INT_MAP, int_map); > + if (ret) > + return ret; > + > + return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, int_enable); > +} > +/** > + * adxl345_get_samples() - Read number of FIFO entries. > + * @st: The initialized state instance of this driver. > + * > + * The sensor does not support treating any axis individually, or exclude them > + * from measuring. > + * > + * Return: negative error, or value. > + */ > +static int adxl345_get_samples(struct adxl345_state *st) > +{ > + unsigned int regval = 0; > + int ret; > + > + ret = regmap_read(st->regmap, ADXL345_REG_FIFO_STATUS, ®val); > + if (ret < 0) > + return ret; > + > + return 0x3f & regval; FIELD_GET() for all stuff like this with appropriate #define for the mask. > +} > + > +/** > + * adxl345_fifo_transfer() - Read samples number of elements. > + * @st: The instance of the state object of this sensor. > + * @samples: The number of lines in the FIFO referred to as fifo_entry, > + * a fifo_entry has 3 elements for X, Y and Z direction of 2 bytes each. > + * > + * It is recommended that a multiple-byte read of all registers be performed to > + * prevent a change in data between reads of sequential registers. That is to > + * read out the data registers X0, X1, Y0, Y1, Z0, Z1 at once. Doesn't match the code which is reading just one register lots of times. > + * > + * Return: 0 or error value. > + */ > +static int adxl345_fifo_transfer(struct adxl345_state *st, int samples) > +{ > + size_t count; > + int i, ret; > + > + count = sizeof(st->fifo_buf[0]) * ADXL345_DIRS; > + for (i = 0; i < samples; i++) { > + ret = regmap_noinc_read(st->regmap, ADXL345_REG_XYZ_BASE, > + st->fifo_buf + (i * count / 2), count); > + if (ret < 0) > + return ret; This is where I'd expect to see the delay mentioned below. > + } > + return ret; > +} > + > +static int adxl345_buffer_predisable(struct iio_dev *indio_dev) > +{ > + struct adxl345_state *st = iio_priv(indio_dev); > + int ret; > + > + st->int_map = 0x00; > + > + ret = adxl345_set_interrupts(st); > + if (ret < 0) > + return ret; > + > + st->fifo_mode = ADXL345_FIFO_BYPASS; > + return adxl345_set_fifo(st); I'd normally expect order in predisable to be reverse of that in postenable. Why isn't it? That is why is the set_fifo here after set_interrupts and not before it. Add a comment. > +} > + > +static const struct iio_buffer_setup_ops adxl345_buffer_ops = { > + .postenable = adxl345_buffer_postenable, > + .predisable = adxl345_buffer_predisable, > +}; > + > +static int adxl345_get_status(struct adxl345_state *st) > +{ > + int ret; > + unsigned int regval; > + > + ret = regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, ®val); > + if (ret < 0) > + return ret; > + > + return (0xff & regval); No brackets needed. > +} > + > +static int adxl345_fifo_push(struct iio_dev *indio_dev, > + int samples) > +{ > + struct adxl345_state *st = iio_priv(indio_dev); > + int i, ret; > + > + if (samples <= 0) > + return -EINVAL; > + > + ret = adxl345_fifo_transfer(st, samples); > + if (ret) > + return ret; > + > + for (i = 0; i < ADXL345_DIRS * samples; i += ADXL345_DIRS) { > + /* > + * To ensure that the FIFO has completely popped, there must be at least 5 > + * us between the end of reading the data registers, signified by the > + * transition to register 0x38 from 0x37 or the CS pin going high, and the > + * start of new reads of the FIFO or reading the FIFO_STATUS register. For > + * SPI operation at 1.5 MHz or lower, the register addressing portion of the > + * transmission is sufficient delay to ensure the FIFO has completely > + * popped. It is necessary for SPI operation greater than 1.5 MHz to > + * de-assert the CS pin to ensure a total of 5 us, which is at most 3.4 us > + * at 5 MHz operation. > + */ > + if (st->fifo_delay && (samples > 1)) > + udelay(3); I'm not following why a delay here helps. At this point you're read masses of data from the fifo without the delays you mention. > + > + iio_push_to_buffers(indio_dev, &st->fifo_buf[i]); > + } > + > + return 0; > +} > + > +/** > + * adxl345_event_handler() - Handle events of the ADXL345. Up to you but... Given it's an IIO driver and that we have a very specific meaning for events, maybe just call this adxl345_irq_handler()? > + * @irq: The irq being handled. > + * @p: The struct iio_device pointer for the device. > + * > + * Return: The interrupt was handled. > + */ > +static irqreturn_t adxl345_event_handler(int irq, void *p) > +{ > + struct iio_dev *indio_dev = p; > + struct adxl345_state *st = iio_priv(indio_dev); > + u8 int_stat; > + int samples; > + > + int_stat = adxl345_get_status(st); > + if (int_stat < 0) > + return IRQ_NONE; > + > + if (int_stat == 0x0) Doesn't this correspond to 'not our interrupt'? If that's the case return IRQ_NONE is the right way to go and not reset the interrupt. You have registered it as maybe shared, and if it is, then this is a common thing to happen as interrupt from another device. > + goto err; > + > + if (int_stat & ADXL345_INT_OVERRUN) > + goto err; > + > + if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) { I think you only ever enable the INT_WATERMARK? If so does it make sense to check for DATA_READY as well? > + samples = adxl345_get_samples(st); > + if (samples < 0) > + goto err; > + > + if (adxl345_fifo_push(indio_dev, samples) < 0) > + goto err; > + > + } > + return IRQ_HANDLED; > + > +err: > + adxl345_fifo_reset(st); > + > + return IRQ_HANDLED; > +} > + > static const struct iio_info adxl345_info = { > .attrs = &adxl345_attrs_group, > .read_raw = adxl345_read_raw, > .write_raw = adxl345_write_raw, > .write_raw_get_fmt = adxl345_write_raw_get_fmt, > + .hwfifo_set_watermark = adxl345_set_watermark, > }; > > /** > @@ -222,6 +499,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, > unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE | > ADXL345_DATA_FORMAT_FULL_RES | > ADXL345_DATA_FORMAT_SELF_TEST); > + u8 fifo_ctl; > int ret; > > indio_dev = devm_iio_device_alloc(dev, sizeof(*st)); > @@ -293,6 +571,28 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, > if (ret < 0) > return dev_err_probe(dev, ret, "Failed to add action or reset\n"); > > + if (st->irq > 0) { > + dev_dbg(dev, "initialize for FIFO_STREAM mode\n"); > + > + ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &adxl345_buffer_ops); > + if (ret) > + return ret; > + > + ret = devm_request_threaded_irq(dev, st->irq, NULL, &adxl345_event_handler, > + IRQF_SHARED | IRQF_ONESHOT, > + indio_dev->name, indio_dev); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n"); > + > + } else { > + dev_dbg(dev, "initialize for FIFO_BYPASS mode (fallback)\n"); > + Given you haven't removed this code from elsewhere, was the driver relying on the defaults after reset before this patch? I don't mind having this branch as a form of documentation even if that's true but maybe add a note to the patch description. > + fifo_ctl = ADXL345_FIFO_CTL_MODE(ADXL345_FIFO_BYPASS); > + > + ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL, fifo_ctl); > + if (ret < 0) > + return ret; > + } > return devm_iio_device_register(dev, indio_dev); > } > EXPORT_SYMBOL_NS_GPL(adxl345_core_probe, IIO_ADXL345); ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v5 10/10] iio: accel: adxl345: add FIFO with watermark events 2024-12-08 16:34 ` Jonathan Cameron @ 2024-12-10 21:54 ` Lothar Rubusch 2024-12-11 19:14 ` Jonathan Cameron 0 siblings, 1 reply; 8+ messages in thread From: Lothar Rubusch @ 2024-12-10 21:54 UTC (permalink / raw) To: Jonathan Cameron Cc: lars, Michael.Hennerich, robh, krzk+dt, conor+dt, devicetree, linux-iio, linux-kernel, eraretuya Hi Jonathan, Find my answers down below. Best, L On Sun, Dec 8, 2024 at 5:34 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Thu, 5 Dec 2024 17:13:43 +0000 > Lothar Rubusch <l.rubusch@gmail.com> wrote: > > > Add a basic setup for FIFO with configurable watermark. Add a handler > > for watermark interrupt events and extend the channel for the > > scan_index needed for the iio channel. The sensor is configurable to use > > a FIFO_BYPASSED mode or a FIFO_STREAM mode. For the FIFO_STREAM mode now > > a watermark can be configured, or disabled by setting 0. Further features > > require a working FIFO setup. > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> > Various comments inline. > > Thanks, > > Jonathan > > > --- > > drivers/iio/accel/adxl345_core.c | 300 +++++++++++++++++++++++++++++++ > > 1 file changed, 300 insertions(+) > > > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c > > index 3067a70c54e..58ed82d66dc 100644 > > --- a/drivers/iio/accel/adxl345_core.c > > +++ b/drivers/iio/accel/adxl345_core.c > > @@ -15,15 +15,28 @@ > > > > #include <linux/iio/iio.h> > > #include <linux/iio/sysfs.h> > > +#include <linux/iio/buffer.h> > > +#include <linux/iio/events.h> > > I'm not seeing any use of this header yet. Bring it in when you need it. > > > +#include <linux/iio/kfifo_buf.h> > > > > #include "adxl345.h" > > > > +#define ADXL345_FIFO_BYPASS 0 > > +#define ADXL345_FIFO_FIFO 1 > > +#define ADXL345_FIFO_STREAM 2 > > + > > +#define ADXL345_DIRS 3 > > + > > struct adxl345_state { > > int irq; > > const struct adxl345_chip_info *info; > > struct regmap *regmap; > > + __le16 fifo_buf[ADXL345_DIRS * ADXL345_FIFO_SIZE]; > > bool fifo_delay; /* delay: delay is needed for SPI */ > > u8 intio; > > + u8 int_map; > > + u8 watermark; > > + u8 fifo_mode; > > }; > > > > #define ADXL345_CHANNEL(index, reg, axis) { \ > > @@ -36,6 +49,13 @@ struct adxl345_state { > > .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \ > > BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > > .scan_index = (index), \ > > + .scan_type = { \ > > + .sign = 's', \ > > + .realbits = 13, \ > > + .storagebits = 16, \ > > + .shift = 0, \ > > No need to specify shift of 0. It's the 'obvious' default and C will set it to 0 > for you anyway. > > > + .endianness = IIO_LE, \ > > + }, \ > > } > > > > enum adxl345_chans { > > @@ -48,6 +68,25 @@ static const struct iio_chan_spec adxl345_channels[] = { > > ADXL345_CHANNEL(2, chan_z, Z), > > }; > > > > +static int adxl345_set_interrupts(struct adxl345_state *st) > > +{ > > + int ret; > > + unsigned int int_enable = st->int_map; > > + unsigned int int_map; > > + > > + /* Any bits set to 0 in the INT map register send their respective > > /* > * Any bits.... > > > > + * interrupts to the INT1 pin, whereas bits set to 1 send their respective > > + * interrupts to the INT2 pin. The intio shall convert this accordingly. > > + */ > > + int_map = 0xFF & (st->intio ? st->int_map : ~st->int_map); > > + > > + ret = regmap_write(st->regmap, ADXL345_REG_INT_MAP, int_map); > > + if (ret) > > + return ret; > > + > > + return regmap_write(st->regmap, ADXL345_REG_INT_ENABLE, int_enable); > > +} > > > > +/** > > + * adxl345_get_samples() - Read number of FIFO entries. > > + * @st: The initialized state instance of this driver. > > + * > > + * The sensor does not support treating any axis individually, or exclude them > > + * from measuring. > > + * > > + * Return: negative error, or value. > > + */ > > +static int adxl345_get_samples(struct adxl345_state *st) > > +{ > > + unsigned int regval = 0; > > + int ret; > > + > > + ret = regmap_read(st->regmap, ADXL345_REG_FIFO_STATUS, ®val); > > + if (ret < 0) > > + return ret; > > + > > + return 0x3f & regval; > FIELD_GET() for all stuff like this with appropriate #define for the mask. > > > +} > > + > > +/** > > + * adxl345_fifo_transfer() - Read samples number of elements. > > + * @st: The instance of the state object of this sensor. > > + * @samples: The number of lines in the FIFO referred to as fifo_entry, > > + * a fifo_entry has 3 elements for X, Y and Z direction of 2 bytes each. > > + * > > + * It is recommended that a multiple-byte read of all registers be performed to > > + * prevent a change in data between reads of sequential registers. That is to > > + * read out the data registers X0, X1, Y0, Y1, Z0, Z1 at once. > > Doesn't match the code which is reading just one register lots of times. This is one of my painpoints, regmap_noinc_read() worked here, for a linewise reading of FIFO elements. Say, I could read X0, X1, Y0,... Z1 in one command. Also, I tried here regmap_bulk_read(). At all, I find this solution is working, but I'm not sure if there is not a total differnt way to do the read out. > > + * > > + * Return: 0 or error value. > > + */ > > +static int adxl345_fifo_transfer(struct adxl345_state *st, int samples) > > +{ > > + size_t count; > > + int i, ret; > > + > > + count = sizeof(st->fifo_buf[0]) * ADXL345_DIRS; > > + for (i = 0; i < samples; i++) { > > + ret = regmap_noinc_read(st->regmap, ADXL345_REG_XYZ_BASE, > > + st->fifo_buf + (i * count / 2), count); > > + if (ret < 0) > > + return ret; > > This is where I'd expect to see the delay mentioned below. I agree with the delay, I'll move it. > > + } > > + return ret; > > +} > > + > > > +static int adxl345_buffer_predisable(struct iio_dev *indio_dev) > > +{ > > + struct adxl345_state *st = iio_priv(indio_dev); > > + int ret; > > + > > + st->int_map = 0x00; > > + > > + ret = adxl345_set_interrupts(st); > > + if (ret < 0) > > + return ret; > > + > > + st->fifo_mode = ADXL345_FIFO_BYPASS; > > + return adxl345_set_fifo(st); > I'd normally expect order in predisable to be reverse of that in postenable. > Why isn't it? That is why is the set_fifo here after set_interrupts and > not before it. Add a comment. > > > +} > > + > > +static const struct iio_buffer_setup_ops adxl345_buffer_ops = { > > + .postenable = adxl345_buffer_postenable, > > + .predisable = adxl345_buffer_predisable, > > +}; > > + > > +static int adxl345_get_status(struct adxl345_state *st) > > +{ > > + int ret; > > + unsigned int regval; > > + > > + ret = regmap_read(st->regmap, ADXL345_REG_INT_SOURCE, ®val); > > + if (ret < 0) > > + return ret; > > + > > + return (0xff & regval); > > No brackets needed. > > > +} > > + > > +static int adxl345_fifo_push(struct iio_dev *indio_dev, > > + int samples) > > +{ > > + struct adxl345_state *st = iio_priv(indio_dev); > > + int i, ret; > > + > > + if (samples <= 0) > > + return -EINVAL; > > + > > + ret = adxl345_fifo_transfer(st, samples); > > + if (ret) > > + return ret; > > + > > + for (i = 0; i < ADXL345_DIRS * samples; i += ADXL345_DIRS) { > > + /* > > + * To ensure that the FIFO has completely popped, there must be at least 5 > > + * us between the end of reading the data registers, signified by the > > + * transition to register 0x38 from 0x37 or the CS pin going high, and the > > + * start of new reads of the FIFO or reading the FIFO_STATUS register. For > > + * SPI operation at 1.5 MHz or lower, the register addressing portion of the > > + * transmission is sufficient delay to ensure the FIFO has completely > > + * popped. It is necessary for SPI operation greater than 1.5 MHz to > > + * de-assert the CS pin to ensure a total of 5 us, which is at most 3.4 us > > + * at 5 MHz operation. > > + */ > > + if (st->fifo_delay && (samples > 1)) > > + udelay(3); > > I'm not following why a delay here helps. At this point you're read masses of > data from the fifo without the delays you mention. > > > + > > + iio_push_to_buffers(indio_dev, &st->fifo_buf[i]); > > + } > > + > > + return 0; > > +} > > + > > +/** > > + * adxl345_event_handler() - Handle events of the ADXL345. > > Up to you but... > Given it's an IIO driver and that we have a very specific meaning > for events, maybe just call this adxl345_irq_handler()? > > > + * @irq: The irq being handled. > > + * @p: The struct iio_device pointer for the device. > > + * > > + * Return: The interrupt was handled. > > + */ > > +static irqreturn_t adxl345_event_handler(int irq, void *p) > > +{ > > + struct iio_dev *indio_dev = p; > > + struct adxl345_state *st = iio_priv(indio_dev); > > + u8 int_stat; > > + int samples; > > + > > + int_stat = adxl345_get_status(st); > > + if (int_stat < 0) > > + return IRQ_NONE; > > + > > + if (int_stat == 0x0) > Doesn't this correspond to 'not our interrupt'? > If that's the case return IRQ_NONE is the right way to go and not reset the > interrupt. You have registered it as maybe shared, and if it is, then this > is a common thing to happen as interrupt from another device. > Here I see actually + int_stat = adxl345_get_status(st); + if (int_stat < 0) + return IRQ_NONE; // a bus error, reading register not possible ...and then... + if (int_stat == 0x0) + // interrupt sources were 0, so IRQ not from our sensor I'm unsure if the first IRQ_NONE here is actually correct. I mean, if the bus is not working, actually any IRQ usage should be considered broken. Is there a way to break out of measuring? > > + goto err; > > + > > + if (int_stat & ADXL345_INT_OVERRUN) > > + goto err; > > + > > + if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) { > > I think you only ever enable the INT_WATERMARK? If so does it > make sense to check for DATA_READY as well? > Watermark comes usually with data ready or overrun. I guess for the FIFO watermark, just evaluating watermark is probably sufficient. For other events, it then might be notification w/ a data ready set. Probably better to introduce data ready when it's actually really needed? > > + samples = adxl345_get_samples(st); > > + if (samples < 0) > > + goto err; > > + > > + if (adxl345_fifo_push(indio_dev, samples) < 0) > > + goto err; > > + > > + } > > + return IRQ_HANDLED; > > + > > +err: > > + adxl345_fifo_reset(st); > > + > > + return IRQ_HANDLED; > > +} > > + > > static const struct iio_info adxl345_info = { > > .attrs = &adxl345_attrs_group, > > .read_raw = adxl345_read_raw, > > .write_raw = adxl345_write_raw, > > .write_raw_get_fmt = adxl345_write_raw_get_fmt, > > + .hwfifo_set_watermark = adxl345_set_watermark, > > }; > > > > /** > > @@ -222,6 +499,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, > > unsigned int data_format_mask = (ADXL345_DATA_FORMAT_RANGE | > > ADXL345_DATA_FORMAT_FULL_RES | > > ADXL345_DATA_FORMAT_SELF_TEST); > > + u8 fifo_ctl; > > int ret; > > > > indio_dev = devm_iio_device_alloc(dev, sizeof(*st)); > > @@ -293,6 +571,28 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, > > if (ret < 0) > > return dev_err_probe(dev, ret, "Failed to add action or reset\n"); > > > > + if (st->irq > 0) { > > + dev_dbg(dev, "initialize for FIFO_STREAM mode\n"); > > + > > + ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &adxl345_buffer_ops); > > + if (ret) > > + return ret; > > + > > + ret = devm_request_threaded_irq(dev, st->irq, NULL, &adxl345_event_handler, > > + IRQF_SHARED | IRQF_ONESHOT, > > + indio_dev->name, indio_dev); > > + if (ret) > > + return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n"); > > + > > + } else { > > + dev_dbg(dev, "initialize for FIFO_BYPASS mode (fallback)\n"); > > + > Given you haven't removed this code from elsewhere, was the driver relying > on the defaults after reset before this patch? > > I don't mind having this branch as a form of documentation even if that's > true but maybe add a note to the patch description. > I'm not sure if I get you correctly. The driver before only implemented "BYPASS" mode. This was the case w/o a defined interrupt-name. My intention now is to keep this behavior as fallback. If no IRQ is around, i.e. interrupt + interrupt-name, the sensor driver will operate the sensor in BYPASS mode. I was interpreting this also as the "default behavior", you mentioned in the dt-binding patch? Is this correct? What do you mean when the driver is relying on the defaults after reset? > > + fifo_ctl = ADXL345_FIFO_CTL_MODE(ADXL345_FIFO_BYPASS); > > + > > + ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL, fifo_ctl); > > + if (ret < 0) > > + return ret; > > + } > > return devm_iio_device_register(dev, indio_dev); > > } > > EXPORT_SYMBOL_NS_GPL(adxl345_core_probe, IIO_ADXL345); > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v5 10/10] iio: accel: adxl345: add FIFO with watermark events 2024-12-10 21:54 ` Lothar Rubusch @ 2024-12-11 19:14 ` Jonathan Cameron 2024-12-11 22:32 ` Lothar Rubusch 0 siblings, 1 reply; 8+ messages in thread From: Jonathan Cameron @ 2024-12-11 19:14 UTC (permalink / raw) To: Lothar Rubusch Cc: lars, Michael.Hennerich, robh, krzk+dt, conor+dt, devicetree, linux-iio, linux-kernel, eraretuya > > > +} > > > + > > > +/** > > > + * adxl345_fifo_transfer() - Read samples number of elements. > > > + * @st: The instance of the state object of this sensor. > > > + * @samples: The number of lines in the FIFO referred to as fifo_entry, > > > + * a fifo_entry has 3 elements for X, Y and Z direction of 2 bytes each. > > > + * > > > + * It is recommended that a multiple-byte read of all registers be performed to > > > + * prevent a change in data between reads of sequential registers. That is to > > > + * read out the data registers X0, X1, Y0, Y1, Z0, Z1 at once. > > > > Doesn't match the code which is reading just one register lots of times. > > This is one of my painpoints, regmap_noinc_read() worked here, for a > linewise reading of FIFO elements. Say, I could read X0, X1, Y0,... Z1 > in one command. Also, I tried here regmap_bulk_read(). At all, I find > this solution is working, but I'm not sure if there is not a total > differnt way to do the read out. A bulk read is defined as indexing through registers. Eg. ADDR, ADDR + 1, ADDR + 2 etc. regmap_noinc_read() just keeps reading the same register, so is typically used for fifos. I opened the datasheet. It seems to say you need to read 3 registers repeatedly rather than one 3 times as often. There isn't a good way to do that sort of sequenced read in one go. So you will need a loop like you have, but it should need a bulk read. Curious it doesn't seem to... Ah. Device auto increments for both SPI and I2C. So in that case the noinc_read and normal bulk read will actually issue the same thing and as these are volatile registers it doesn't matter (it would if you were caching the result as the data would end up cached in different places). It will do the wrong thing though if you have an i2c controller that is less capable and can't do large reads. So you should definitely use bulk_read not noinc. Have you set available_scan_masks? If not you want to do so as per comment I made in the cover letter. > > > + * @irq: The irq being handled. > > > + * @p: The struct iio_device pointer for the device. > > > + * > > > + * Return: The interrupt was handled. > > > + */ > > > +static irqreturn_t adxl345_event_handler(int irq, void *p) > > > +{ > > > + struct iio_dev *indio_dev = p; > > > + struct adxl345_state *st = iio_priv(indio_dev); > > > + u8 int_stat; > > > + int samples; > > > + > > > + int_stat = adxl345_get_status(st); > > > + if (int_stat < 0) > > > + return IRQ_NONE; > > > + > > > + if (int_stat == 0x0) > > Doesn't this correspond to 'not our interrupt'? > > If that's the case return IRQ_NONE is the right way to go and not reset the > > interrupt. You have registered it as maybe shared, and if it is, then this > > is a common thing to happen as interrupt from another device. > > > > Here I see actually > + int_stat = adxl345_get_status(st); > + if (int_stat < 0) > + return IRQ_NONE; // a bus error, reading register not possible > ...and then... > + if (int_stat == 0x0) > + // interrupt sources were 0, so IRQ not from our sensor > > I'm unsure if the first IRQ_NONE here is actually correct. I mean, if > the bus is not working, > actually any IRQ usage should be considered broken. Is there a way to > break out of measuring? > It is a much debated thing on what you should return if you have no idea if it is our interrupt or not. There isn't really a right answer. If you get a lot of IRQ_NONE and no one else claims it eventually the interrupt will be disabled (to break the interrupt storm freezing the machine). > > > + goto err; > > > + > > > + if (int_stat & ADXL345_INT_OVERRUN) > > > + goto err; > > > + > > > + if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) { > > > > I think you only ever enable the INT_WATERMARK? If so does it > > make sense to check for DATA_READY as well? > > > > Watermark comes usually with data ready or overrun. I guess for the > FIFO watermark, just evaluating watermark is probably sufficient. For > other events, it then might be notification w/ a data ready set. > Probably better to introduce data ready when it's actually really > needed? Yes. That dataready is normally used when you are doing capture without the fifo and want to read each sample - kind of same as a watermark depth of 1, but less hardware turned on. As such, may be no need to ever support it. return dev_err_probe(dev, ret, "Failed to add action or reset\n"); > > > > > > + if (st->irq > 0) { > > > + dev_dbg(dev, "initialize for FIFO_STREAM mode\n"); > > > + > > > + ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &adxl345_buffer_ops); > > > + if (ret) > > > + return ret; > > > + > > > + ret = devm_request_threaded_irq(dev, st->irq, NULL, &adxl345_event_handler, > > > + IRQF_SHARED | IRQF_ONESHOT, > > > + indio_dev->name, indio_dev); > > > + if (ret) > > > + return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n"); > > > + > > > + } else { > > > + dev_dbg(dev, "initialize for FIFO_BYPASS mode (fallback)\n"); > > > + > > Given you haven't removed this code from elsewhere, was the driver relying > > on the defaults after reset before this patch? > > > > I don't mind having this branch as a form of documentation even if that's > > true but maybe add a note to the patch description. > > > > I'm not sure if I get you correctly. The driver before only > implemented "BYPASS" mode. This was the case w/o a defined > interrupt-name. My intention now is to keep this behavior as fallback. > If no IRQ is around, i.e. interrupt + interrupt-name, the sensor > driver will operate the sensor in BYPASS mode. > > I was interpreting this also as the "default behavior", you mentioned > in the dt-binding patch? Is this correct? > > What do you mean when the driver is relying on the defaults after reset? The driver worked without irq before now. Which is same as this path. So how was the register written here being configured correctly before this patch? I'm guessing it wasn't and that the value written here is the default on power up. Either that or I'm miss understanding what this branch of the if / else is about. Jonathan > > > > + fifo_ctl = ADXL345_FIFO_CTL_MODE(ADXL345_FIFO_BYPASS); > > > + > > > + ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL, fifo_ctl); > > > + if (ret < 0) > > > + return ret; > > > + } > > > return devm_iio_device_register(dev, indio_dev); > > > } > > > EXPORT_SYMBOL_NS_GPL(adxl345_core_probe, IIO_ADXL345); > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v5 10/10] iio: accel: adxl345: add FIFO with watermark events 2024-12-11 19:14 ` Jonathan Cameron @ 2024-12-11 22:32 ` Lothar Rubusch 0 siblings, 0 replies; 8+ messages in thread From: Lothar Rubusch @ 2024-12-11 22:32 UTC (permalink / raw) To: Jonathan Cameron Cc: lars, Michael.Hennerich, robh, krzk+dt, conor+dt, devicetree, linux-iio, linux-kernel, eraretuya Hello Jonathan, Thank you again. This brought several pieces together for me. See answeres down below. Best, L On Wed, Dec 11, 2024 at 8:14 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > +} > > > > + > > > > +/** > > > > + * adxl345_fifo_transfer() - Read samples number of elements. > > > > + * @st: The instance of the state object of this sensor. > > > > + * @samples: The number of lines in the FIFO referred to as fifo_entry, > > > > + * a fifo_entry has 3 elements for X, Y and Z direction of 2 bytes each. > > > > + * > > > > + * It is recommended that a multiple-byte read of all registers be performed to > > > > + * prevent a change in data between reads of sequential registers. That is to > > > > + * read out the data registers X0, X1, Y0, Y1, Z0, Z1 at once. > > > > > > Doesn't match the code which is reading just one register lots of times. > > > > This is one of my painpoints, regmap_noinc_read() worked here, for a > > linewise reading of FIFO elements. Say, I could read X0, X1, Y0,... Z1 > > in one command. Also, I tried here regmap_bulk_read(). At all, I find > > this solution is working, but I'm not sure if there is not a total > > differnt way to do the read out. > > A bulk read is defined as indexing through registers. Eg. ADDR, ADDR + 1, ADDR + 2 > etc. regmap_noinc_read() just keeps reading the same register, so is typically > used for fifos. > > I opened the datasheet. It seems to say you need to read 3 registers repeatedly > rather than one 3 times as often. There isn't a good way to do that sort of > sequenced read in one go. So you will need a loop like you have, but it > should need a bulk read. Curious it doesn't seem to... > > Ah. Device auto increments for both SPI and I2C. So in that case > the noinc_read and normal bulk read will actually issue the same thing and > as these are volatile registers it doesn't matter (it would if you were > caching the result as the data would end up cached in different places). > > It will do the wrong thing though if you have an i2c controller that > is less capable and can't do large reads. So you should definitely use > bulk_read not noinc. > > Have you set available_scan_masks? If not you want to do so as > per comment I made in the cover letter. > For v6 I'll setup available_scan_masks, as I understand its usage. I'm not sure if I understood what it actually does. I can see it needs the channel indexes available, but I'll need to read a bit more code. Hopefully for now it'll be sufficient. > > > > > + * @irq: The irq being handled. > > > > + * @p: The struct iio_device pointer for the device. > > > > + * > > > > + * Return: The interrupt was handled. > > > > + */ > > > > +static irqreturn_t adxl345_event_handler(int irq, void *p) > > > > +{ > > > > + struct iio_dev *indio_dev = p; > > > > + struct adxl345_state *st = iio_priv(indio_dev); > > > > + u8 int_stat; > > > > + int samples; > > > > + > > > > + int_stat = adxl345_get_status(st); > > > > + if (int_stat < 0) > > > > + return IRQ_NONE; > > > > + > > > > + if (int_stat == 0x0) > > > Doesn't this correspond to 'not our interrupt'? > > > If that's the case return IRQ_NONE is the right way to go and not reset the > > > interrupt. You have registered it as maybe shared, and if it is, then this > > > is a common thing to happen as interrupt from another device. > > > > > > > Here I see actually > > + int_stat = adxl345_get_status(st); > > + if (int_stat < 0) > > + return IRQ_NONE; // a bus error, reading register not possible > > ...and then... > > + if (int_stat == 0x0) > > + // interrupt sources were 0, so IRQ not from our sensor > > > > I'm unsure if the first IRQ_NONE here is actually correct. I mean, if > > the bus is not working, > > actually any IRQ usage should be considered broken. Is there a way to > > break out of measuring? > > Here actually, I merged them, when returning IRQ_NONE in both statements. I mean, if the bus is broken, the sensor driver then is anyway not operational anymore. It cannot process interrupts anymore be it from someone else or own ones. When the interrupt source register was 0, then the interrupt was not ours for sure. As I understand, if such happens, there will be no more IRQs anyway as long as interrupt source reg and fifo are not cleaned up. > It is a much debated thing on what you should return if you have no > idea if it is our interrupt or not. There isn't really a right > answer. If you get a lot of IRQ_NONE and no one else claims it eventually > the interrupt will be disabled (to break the interrupt storm freezing the > machine). > > > > > > + goto err; > > > > + > > > > + if (int_stat & ADXL345_INT_OVERRUN) > > > > + goto err; > > > > + > > > > + if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) { > > > > > > I think you only ever enable the INT_WATERMARK? If so does it > > > make sense to check for DATA_READY as well? > > > > > > > Watermark comes usually with data ready or overrun. I guess for the > > FIFO watermark, just evaluating watermark is probably sufficient. For > > other events, it then might be notification w/ a data ready set. > > Probably better to introduce data ready when it's actually really > > needed? > > Yes. That dataready is normally used when you are doing capture without > the fifo and want to read each sample - kind of same as a watermark depth > of 1, but less hardware turned on. As such, may be no need to ever support it. > > return dev_err_probe(dev, ret, "Failed to add action or reset\n"); > > > > > > > > + if (st->irq > 0) { > > > > + dev_dbg(dev, "initialize for FIFO_STREAM mode\n"); > > > > + > > > > + ret = devm_iio_kfifo_buffer_setup(dev, indio_dev, &adxl345_buffer_ops); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + ret = devm_request_threaded_irq(dev, st->irq, NULL, &adxl345_event_handler, > > > > + IRQF_SHARED | IRQF_ONESHOT, > > > > + indio_dev->name, indio_dev); > > > > + if (ret) > > > > + return dev_err_probe(dev, ret, "Failed to setup triggered buffer\n"); > > > > + > > > > + } else { > > > > + dev_dbg(dev, "initialize for FIFO_BYPASS mode (fallback)\n"); > > > > + > > > Given you haven't removed this code from elsewhere, was the driver relying > > > on the defaults after reset before this patch? > > > > > > I don't mind having this branch as a form of documentation even if that's > > > true but maybe add a note to the patch description. > > > > > > > I'm not sure if I get you correctly. The driver before only > > implemented "BYPASS" mode. This was the case w/o a defined > > interrupt-name. My intention now is to keep this behavior as fallback. > > If no IRQ is around, i.e. interrupt + interrupt-name, the sensor > > driver will operate the sensor in BYPASS mode. > > > > I was interpreting this also as the "default behavior", you mentioned > > in the dt-binding patch? Is this correct? > > > > What do you mean when the driver is relying on the defaults after reset? > > The driver worked without irq before now. Which is same as this path. > So how was the register written here being configured correctly before > this patch? I'm guessing it wasn't and that the value written here > is the default on power up. > > Either that or I'm miss understanding what this branch of the if / else is > about. Well, I'm pretty convinced the sensor before was operated in FIFO_BYPASS mode. TBH I'm not sure now if this is default setting, or explicitely configured. But I remember that also from somewhere in the datasheet. That's why I used this as fallback. > Jonathan > > > > > > > + fifo_ctl = ADXL345_FIFO_CTL_MODE(ADXL345_FIFO_BYPASS); > > > > + > > > > + ret = regmap_write(st->regmap, ADXL345_REG_FIFO_CTL, fifo_ctl); > > > > + if (ret < 0) > > > > + return ret; > > > > + } > > > > return devm_iio_device_register(dev, indio_dev); > > > > } > > > > EXPORT_SYMBOL_NS_GPL(adxl345_core_probe, IIO_ADXL345); > > > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v5 10/10] iio: accel: adxl345: add FIFO with watermark events 2024-12-05 17:13 ` [PATCH v5 10/10] iio: accel: adxl345: add FIFO with " Lothar Rubusch 2024-12-08 16:34 ` Jonathan Cameron @ 2024-12-10 8:47 ` Dan Carpenter 2024-12-11 19:15 ` Jonathan Cameron 1 sibling, 1 reply; 8+ messages in thread From: Dan Carpenter @ 2024-12-10 8:47 UTC (permalink / raw) To: oe-kbuild, Lothar Rubusch, lars, Michael.Hennerich, jic23, robh, krzk+dt, conor+dt Cc: lkp, oe-kbuild-all, devicetree, linux-iio, linux-kernel, eraretuya, l.rubusch Hi Lothar, kernel test robot noticed the following build warnings: https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Lothar-Rubusch/iio-accel-adxl345-refrase-comment-on-probe/20241206-011802 base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg patch link: https://lore.kernel.org/r/20241205171343.308963-11-l.rubusch%40gmail.com patch subject: [PATCH v5 10/10] iio: accel: adxl345: add FIFO with watermark events config: nios2-randconfig-r072-20241210 (https://download.01.org/0day-ci/archive/20241210/202412101132.Kj6R6i3h-lkp@intel.com/config) compiler: nios2-linux-gcc (GCC) 14.2.0 If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org> | Closes: https://lore.kernel.org/r/202412101132.Kj6R6i3h-lkp@intel.com/ smatch warnings: drivers/iio/accel/adxl345_core.c:441 adxl345_event_handler() warn: unsigned 'int_stat' is never less than zero. vim +/ret +321 drivers/iio/accel/adxl345_core.c 55d2386488598bb Lothar Rubusch 2024-12-05 433 static irqreturn_t adxl345_event_handler(int irq, void *p) 55d2386488598bb Lothar Rubusch 2024-12-05 434 { 55d2386488598bb Lothar Rubusch 2024-12-05 435 struct iio_dev *indio_dev = p; 55d2386488598bb Lothar Rubusch 2024-12-05 436 struct adxl345_state *st = iio_priv(indio_dev); 55d2386488598bb Lothar Rubusch 2024-12-05 437 u8 int_stat; ^^^^^^^^^^^ 55d2386488598bb Lothar Rubusch 2024-12-05 438 int samples; 55d2386488598bb Lothar Rubusch 2024-12-05 439 55d2386488598bb Lothar Rubusch 2024-12-05 440 int_stat = adxl345_get_status(st); 55d2386488598bb Lothar Rubusch 2024-12-05 @441 if (int_stat < 0) ^^^^^^^^^^^^ signedness bug 55d2386488598bb Lothar Rubusch 2024-12-05 442 return IRQ_NONE; 55d2386488598bb Lothar Rubusch 2024-12-05 443 55d2386488598bb Lothar Rubusch 2024-12-05 444 if (int_stat == 0x0) 55d2386488598bb Lothar Rubusch 2024-12-05 445 goto err; 55d2386488598bb Lothar Rubusch 2024-12-05 446 55d2386488598bb Lothar Rubusch 2024-12-05 447 if (int_stat & ADXL345_INT_OVERRUN) 55d2386488598bb Lothar Rubusch 2024-12-05 448 goto err; 55d2386488598bb Lothar Rubusch 2024-12-05 449 55d2386488598bb Lothar Rubusch 2024-12-05 450 if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) { 55d2386488598bb Lothar Rubusch 2024-12-05 451 samples = adxl345_get_samples(st); 55d2386488598bb Lothar Rubusch 2024-12-05 452 if (samples < 0) 55d2386488598bb Lothar Rubusch 2024-12-05 453 goto err; 55d2386488598bb Lothar Rubusch 2024-12-05 454 55d2386488598bb Lothar Rubusch 2024-12-05 455 if (adxl345_fifo_push(indio_dev, samples) < 0) 55d2386488598bb Lothar Rubusch 2024-12-05 456 goto err; 55d2386488598bb Lothar Rubusch 2024-12-05 457 55d2386488598bb Lothar Rubusch 2024-12-05 458 } 55d2386488598bb Lothar Rubusch 2024-12-05 459 return IRQ_HANDLED; 55d2386488598bb Lothar Rubusch 2024-12-05 460 55d2386488598bb Lothar Rubusch 2024-12-05 461 err: 55d2386488598bb Lothar Rubusch 2024-12-05 462 adxl345_fifo_reset(st); 55d2386488598bb Lothar Rubusch 2024-12-05 463 55d2386488598bb Lothar Rubusch 2024-12-05 464 return IRQ_HANDLED; 55d2386488598bb Lothar Rubusch 2024-12-05 465 } -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v5 10/10] iio: accel: adxl345: add FIFO with watermark events 2024-12-10 8:47 ` Dan Carpenter @ 2024-12-11 19:15 ` Jonathan Cameron 0 siblings, 0 replies; 8+ messages in thread From: Jonathan Cameron @ 2024-12-11 19:15 UTC (permalink / raw) To: Dan Carpenter Cc: oe-kbuild, Lothar Rubusch, lars, Michael.Hennerich, robh, krzk+dt, conor+dt, lkp, oe-kbuild-all, devicetree, linux-iio, linux-kernel, eraretuya On Tue, 10 Dec 2024 11:47:37 +0300 Dan Carpenter <dan.carpenter@linaro.org> wrote: > Hi Lothar, > > kernel test robot noticed the following build warnings: Ah. This was the report you were referring to I guess. Just fix these in v6 version of this patch. Thanks, Jonathan > > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: https://github.com/intel-lab-lkp/linux/commits/Lothar-Rubusch/iio-accel-adxl345-refrase-comment-on-probe/20241206-011802 > base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg > patch link: https://lore.kernel.org/r/20241205171343.308963-11-l.rubusch%40gmail.com > patch subject: [PATCH v5 10/10] iio: accel: adxl345: add FIFO with watermark events > config: nios2-randconfig-r072-20241210 (https://download.01.org/0day-ci/archive/20241210/202412101132.Kj6R6i3h-lkp@intel.com/config) > compiler: nios2-linux-gcc (GCC) 14.2.0 > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > | Closes: https://lore.kernel.org/r/202412101132.Kj6R6i3h-lkp@intel.com/ > > smatch warnings: > drivers/iio/accel/adxl345_core.c:441 adxl345_event_handler() warn: unsigned 'int_stat' is never less than zero. > > vim +/ret +321 drivers/iio/accel/adxl345_core.c > > 55d2386488598bb Lothar Rubusch 2024-12-05 433 static irqreturn_t adxl345_event_handler(int irq, void *p) > 55d2386488598bb Lothar Rubusch 2024-12-05 434 { > 55d2386488598bb Lothar Rubusch 2024-12-05 435 struct iio_dev *indio_dev = p; > 55d2386488598bb Lothar Rubusch 2024-12-05 436 struct adxl345_state *st = iio_priv(indio_dev); > 55d2386488598bb Lothar Rubusch 2024-12-05 437 u8 int_stat; > ^^^^^^^^^^^ > > 55d2386488598bb Lothar Rubusch 2024-12-05 438 int samples; > 55d2386488598bb Lothar Rubusch 2024-12-05 439 > 55d2386488598bb Lothar Rubusch 2024-12-05 440 int_stat = adxl345_get_status(st); > 55d2386488598bb Lothar Rubusch 2024-12-05 @441 if (int_stat < 0) > ^^^^^^^^^^^^ > signedness bug > > 55d2386488598bb Lothar Rubusch 2024-12-05 442 return IRQ_NONE; > 55d2386488598bb Lothar Rubusch 2024-12-05 443 > 55d2386488598bb Lothar Rubusch 2024-12-05 444 if (int_stat == 0x0) > 55d2386488598bb Lothar Rubusch 2024-12-05 445 goto err; > 55d2386488598bb Lothar Rubusch 2024-12-05 446 > 55d2386488598bb Lothar Rubusch 2024-12-05 447 if (int_stat & ADXL345_INT_OVERRUN) > 55d2386488598bb Lothar Rubusch 2024-12-05 448 goto err; > 55d2386488598bb Lothar Rubusch 2024-12-05 449 > 55d2386488598bb Lothar Rubusch 2024-12-05 450 if (int_stat & (ADXL345_INT_DATA_READY | ADXL345_INT_WATERMARK)) { > 55d2386488598bb Lothar Rubusch 2024-12-05 451 samples = adxl345_get_samples(st); > 55d2386488598bb Lothar Rubusch 2024-12-05 452 if (samples < 0) > 55d2386488598bb Lothar Rubusch 2024-12-05 453 goto err; > 55d2386488598bb Lothar Rubusch 2024-12-05 454 > 55d2386488598bb Lothar Rubusch 2024-12-05 455 if (adxl345_fifo_push(indio_dev, samples) < 0) > 55d2386488598bb Lothar Rubusch 2024-12-05 456 goto err; > 55d2386488598bb Lothar Rubusch 2024-12-05 457 > 55d2386488598bb Lothar Rubusch 2024-12-05 458 } > 55d2386488598bb Lothar Rubusch 2024-12-05 459 return IRQ_HANDLED; > 55d2386488598bb Lothar Rubusch 2024-12-05 460 > 55d2386488598bb Lothar Rubusch 2024-12-05 461 err: > 55d2386488598bb Lothar Rubusch 2024-12-05 462 adxl345_fifo_reset(st); > 55d2386488598bb Lothar Rubusch 2024-12-05 463 > 55d2386488598bb Lothar Rubusch 2024-12-05 464 return IRQ_HANDLED; > 55d2386488598bb Lothar Rubusch 2024-12-05 465 } > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-12-11 22:33 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-12-10 4:17 [PATCH v5 10/10] iio: accel: adxl345: add FIFO with watermark events kernel test robot -- strict thread matches above, loose matches on Subject: below -- 2024-12-05 17:13 [PATCH v5 00/10] iio: accel: adxl345: add FIFO operating with IRQ triggered " Lothar Rubusch 2024-12-05 17:13 ` [PATCH v5 10/10] iio: accel: adxl345: add FIFO with " Lothar Rubusch 2024-12-08 16:34 ` Jonathan Cameron 2024-12-10 21:54 ` Lothar Rubusch 2024-12-11 19:14 ` Jonathan Cameron 2024-12-11 22:32 ` Lothar Rubusch 2024-12-10 8:47 ` Dan Carpenter 2024-12-11 19:15 ` Jonathan Cameron
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.