Messages in this thread | | | Subject | Re: [PATCH v4 2/4] iio: health: Add driver for the TI AFE4404 heart monitor | From | "Andrew F. Davis" <> | Date | Tue, 2 Feb 2016 09:50:35 -0600 |
| |
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@ti.com> > 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
| |