On 01/30/2016 08:59 AM, Jonathan Cameron wrote:

On 25/01/16 17:28, Andrew F. Davis wrote:

quoted

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 <redacted>

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

quoted

---
  .../ABI/testing/sysfs-bus-iio-health-afe440x       |  54 ++
  drivers/iio/health/Kconfig                         |  19 +-

[...]

quoted

+
+static const struct afe440x_reg_info afe4404_reg_info[] = {

As noted below, I'd find
[LED1] = AFE440X_REG_INFO(AFE440_LED1VAL...
more readable.

ACK

[...]

quoted

+	int ret;
+
+	iio_device_unregister(indio_dev);
+

Would expect unwinding of the triggered_buffer here.
iio_triggered_buffer_cleanup()

ACK

quoted

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

quoted

+enum reg_type {
+	SIMPLE,
+	RESISTANCE,
+	CAPACITANCE,
+};

ACK

quoted

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