Re: [PATCH 3/3] iio: heart_monitor: Add TI afe4403 heart monitor

[Jonathan Cameron <jic23@kernel.org>]

On 29/04/15 14:52, Dan Murphy wrote:
> Jonathan
> 
> On 04/26/2015 01:38 PM, Jonathan Cameron wrote:
>> On 22/04/15 17:32, Dan Murphy wrote:
>>> Add the TI afe4403 heart rate monitor.
>>> This device detects reflected LED
>>> wave length fluctuations and presents an ADC
>>> value to the user space to be converted to a
>>> heart rate.
>>>
>>> Data sheet located here:
>>> http://www.ti.com/product/AFE4403/datasheet
>>>
>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> Hi Dan,
>>
>> Good to see this coming back again!
>>
>> Anyhow, various comments inline, but the biggest issue is the ABI usage.
>> Please take a long hard look at the ABI docs (Documentation/ABI/test/sysfs-bus-iio*) and the use that is being made of them here.  This doesn't conform to
>> the ABI in a number of places and there is no documentation to imply that
>> it is creating new ABI.
>>
>> There are 4 input channels here Ambient1, Ambient2, led1 and led2.
>> We also have a two differential channels - though as these are seperated in
>> time it's just (I think) for convenience.
>>
>> These can then feed back to ambient cancellation DACs thus hopefully giving
>> just the nice led signals an allowing measurement of Pleth (which is what
>> we are aiming for?)
>> So two output DAC channels, use extended name to say what they are.
>> out_voltage0_ambientcancelation (perhaps...)  OR... Perhaps it would
>> be preferable to treat this as a caliboffset to the input channels.
>> (I think I prefer this second option).
>>
>> Your other channels allow manipulation of the signal chain - I think these
>> pretty much boil down to gains of one type or another?  If we don't have
>> a suitable ABI element for all of them then propose it, but they don't
>> look like output channels to me.
>>
>> You do have LED controls however which might be representable as output
>> channels, but they need to be more clearly described than below.
>>
>> One big issue here is that this isn't actually pulse measurement device
>> at all. It's measuring the pleth signal (photoplethysmography - which here
>> is just a light intensity measure - I think!) which can via 'magic' be used
>> to derive a pulse.  That's not a problem, but the channel types aren't
>> going to include heart_rate as that's not output from the device.
>>
>>> ---
>>>  drivers/iio/Kconfig                 |   1 +
>>>  drivers/iio/Makefile                |   1 +
>>>  drivers/iio/heart_monitor/Kconfig   |  19 +
>>>  drivers/iio/heart_monitor/Makefile  |   6 +
>>>  drivers/iio/heart_monitor/afe4403.c | 702 ++++++++++++++++++++++++++++++++++++
> 
> I am thinking of changing the directory to health as opposed to heart_monitor.
> 
> Thoughts?
> 
> <snip>
> 
It's easy to move them around later so don't worry too much.  Do you have
other drivers coming up that would make the health naming sensible?

J