On 01/30/2016 08:59 AM, Jonathan Cameron wrote:
On 25/01/16 17:28, Andrew F. Davis wrote:
Add driver for the TI AFE4404 heart rate monitor and pulse oximeter.
This device detects reflected LED light fluctuations and presents an ADC
value to the user space for further signal processing.
Datasheet: http://www.ti.com/product/AFE4404/datasheet
Signed-off-by: Andrew F. Davis <afd@xxxxxx>
Hi Andrew,
This changed rather more than I was expecting, but fair enough.
A few bits are missing from remove. Few other little bits inline,
though mostly those are about readability rather than what the code
is doing.
I was a little suprised to see no control over whether the trigger is
enabled or not. I'm trying to get my head around what the result of this will
be. I guess we'll have interrupts pinging away merilly doing nothing until
the buffer is attached then the demux will get set up to actually do
something with the data.
hmm. Might be fine, if I ususual. Normally devices have some means of
masking the interrupt.
I wasn't sure about this ether, but it doesn't seem to break anything.
Jonathan
---
.../ABI/testing/sysfs-bus-iio-health-afe440x | 54 ++
drivers/iio/health/Kconfig | 19 +-
[...]
+
+static const struct afe440x_reg_info afe4404_reg_info[] = {
As noted below, I'd find
[LED1] = AFE440X_REG_INFO(AFE440_LED1VAL...
more readable.
ACK
[...]
+ int ret;
+
+ iio_device_unregister(indio_dev);
+
Would expect unwinding of the triggered_buffer here.
iio_triggered_buffer_cleanup()
ACK
+ iio_trigger_unregister(afe->trig);
This should be protected by a check on afe->irq as for the register in probe.
There is no protection against afe->trig == NULL in iio_trigger_unregister.
That is kind of deliberate as it makes sure the probe / remove in drivers
are balanced unlike here.
Makes sense, will add.
[...]
I'm not overly keen on the hiding fo the indexing. I'd prefer
this was just the bit {...} and you had the fact it was filling in an
indexed element explicit where used.
That works.
[...]
I'd prefix reg_type to avoid possible name clashes in future.
+enum reg_type {
+ SIMPLE,
+ RESISTANCE,
+ CAPACITANCE,
+};
ACK
+
+/* this could be made more general for other IIO drivers if needed --------- */
Yes, but lets do that at as a follow up.
Sure.
Thanks,
Andrew
--
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
