Hi,
Thanks for review. I agree with all of the comments and will fix these
in next patch version. Below is just a comment on hwfifo_* sysfs access.
On 06.11.2016 13:35, Jonathan Cameron wrote:
On 03/11/16 11:25, Marcin Niestroj wrote:
This patch was developed primarily based on bmc150_accel hardware fifo
implementation.
IRQ handler was added, which for now is responsible only for handling
watermark interrupts. The BMI160 chip has two interrupt outputs. By
default INT is considered to be connected. If INT2 is used instead, the
interrupt-names device-tree property can be used to specify that.
Signed-off-by: Marcin Niestroj <m.niestroj@xxxxxxxxxxxxxxxx>
I agree with Peter that there should have been a few precursor patches
to this one doing various cleanups and reworking bits that you have
in here. Would have made it easier to review (always a good thing :)
In general the resulting code looks good to me. A few little
additional comments inline from me. Mostly about small code ordering things
and function rename suggestions that would make the code more 'obviously'
correct.
Thanks,
Jonathan
---
drivers/iio/imu/bmi160/bmi160.h | 3 +-
drivers/iio/imu/bmi160/bmi160_core.c | 633 +++++++++++++++++++++++++++++++++--
drivers/iio/imu/bmi160/bmi160_i2c.c | 7 +-
drivers/iio/imu/bmi160/bmi160_spi.c | 3 +-
4 files changed, 618 insertions(+), 28 deletions(-)
<snip>
+
+static IIO_CONST_ATTR(hwfifo_watermark_min, "1");
+static IIO_CONST_ATTR(hwfifo_watermark_max,
+ __stringify(BMI160_FIFO_LENGTH));
+static IIO_DEVICE_ATTR(hwfifo_enabled, S_IRUGO,
+ bmi160_get_fifo_state, NULL, 0);
+static IIO_DEVICE_ATTR(hwfifo_watermark, S_IRUGO,
+ bmi160_get_fifo_watermark, NULL, 0);
+
+static const struct attribute *bmi160_fifo_attributes[] = {
+ &iio_const_attr_hwfifo_watermark_min.dev_attr.attr,
+ &iio_const_attr_hwfifo_watermark_max.dev_attr.attr,
+ &iio_dev_attr_hwfifo_watermark.dev_attr.attr,
+ &iio_dev_attr_hwfifo_enabled.dev_attr.attr,
There are enough of these drivers now that sometimes soon we should
revisit the question of pulling these into the core. Can certainly
concieve of downstream consumer devices (in particular the iio_input
bridge when that finally resurfaces - my fault) wanting to be able to
manipulate or at least have visibilty of these.
One more thing to consider is setting hwfifo_watermark to other value
than "userspace" watermark. It would be nice to set hwfifo_watermark to
a *safe* value to be able to get all data from hardware to kfifo. By
safe I mean that the chance of hardware fifo overflow will be small. On
the other hand there might be no reason to have such small watermark for
userspace application, so we can save scheduler cycles.
<snip>
--
Marcin Niestroj
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
