Re: [PATCH 2/2] iio: health: Add driver for the TI AFE4404 heart monitor

From: Andrew F. Davis
Date: Mon Nov 23 2015 - 15:54:04 EST



On 11/17/2015 11:07 AM, Andrew F. Davis wrote:
On 11/15/2015 06:07 AM, Jonathan Cameron wrote:
On 10/11/15 19:19, Andrew F. Davis wrote:
On 11/05/2015 01:09 PM, Jonathan Cameron wrote:
Lars, Hartmut, Peter,

This is becoming a really involved ABI discussion so I'd like some
input on this if any of you have the time.

I'm going to be busy now until at least the weekend...

On 04/11/15 21:17, Andrew F. Davis wrote:
On 11/04/2015 01:40 PM, Jonathan Cameron wrote:
On 02/11/15 20:37, Andrew F. Davis wrote:
On 11/01/2015 02:52 PM, Jonathan Cameron wrote:
On 31/10/15 16:31, 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.

Data sheet located here:
http://www.ti.com/product/AFE4404/datasheet

Signed-off-by: Andrew F. Davis <afd@xxxxxx>
Hi Andrew,

Good to see this resurface. It's a fascinating little device.

Anyhow, most of the interesting bit in here is unsuprisingly concerned
with the interface. I know we went round a few loops of this before but
it still feels like we haven't worked out to handle it well. I would like
as much input as we can get on this as inevitablly it will have
repercussions outside this driver.

Your approach of hammering out descriptive sysfs attributes is a good
starting point but we need to work towards a formal description that
can be generalised. Whilst there are not many similar devices out there
to this one, I suspect there are a few and more may well show up in
future.


Yeah, I'm working on brining up drivers for them now :)
cool

The escence of my rather roundabout response inline is that I'm suggesting
adding a new channel type to represent light transmission, taking the analogous
case of proximity devices in which we are looking at light reflection.
I've also taken the justification we use for illuminance vs intensity readings
for two sensor ALS sensors as a precident for having compound channels of a different
type to the 'raw' data that feeds them. Hence I propose something along
the lines of:

in_intensityX_raw (raw channel value with the led on)
in_intensityX_ambient_raw (raw channel value with the led off)


I'm not sure, I know it may be too late for a lot of drivers but putting the 'X'
against the 'intensity' works for devices like ADCs/DACs with a simple list
of numeric channels, but for any other device with named channels this will
become very inconsistent, especially when adding modified channels and
differential channels.
Sadly its ABI now so we can't realistically change it. We can extend, we can't
replace (we we can over the course of a lot of years but that's a nightmare).


For example:

in_intensity5_name_ambient-2_mean_raw
The oddity here is that in your case the device in interacting with a stimulus
output. Normally the index reflects an actual sensor. We are kind of bludgeoning
it in. The only equivalently nasty case I know of is touch screens where different
resistances are being connected - from a generic point of view those are a nightmare
as well (as every implementation does it differently).

Yeah, this part really doesn't fit the mold for this subsystem, or
any really, hwmon, input, were also considered, but the plan is still
to attempt to shoehorn it in to this one, so hopefully you can bear with me
on all these oddities :)
Much as it irritates my sense of neat and tidy I guess that if we do end up with
an ABI for this that we don't like later it isn't the end of the world as I doubt
we'll be inundated with hundreds of these sensors.

However, lets keep the naming within reason to how we would naturally extend
the ABI.

Having thought more on these differential channels, do we really need to have
them explicitly as differential channels at all? Perhaps we are better off with

in_intensity0_led1_raw
in_intensity1_ambient_raw
in_intensity2_transmitted_led1_raw

in_intensity3_led2_raw
in_intensity4_ambient_raw
in_intensity5_transmitted_led2_raw

in_intenisty6_led3_raw
in_intenisty7_ambient_raw
in_intensity8_transmitted_led3_raw

in_intensity9_transmitted_led1_meanfiltered_raw
(and it does want to be explicitly meanfiltered and not mean
which would imply the mean over all time)

in_intensity10_transmitted_led2_meanfiltered_raw

It's simple, descriptive and almost fits in the current ABI - you could
even blugeon it in easily enough except for the mean filtered case.
In many ways this is your naming proposal after all.


One issue might be that we really only have 4 real channels that become
different things depending on how you setup the device. Matching the
names of the registers is the only way we can label these, as the user
might change their use.

in_intensity_[RegName]_raw

I really can't see any way around it, the channels are just too adjustable.
Lots of channels to cover the use case the hardware supports. This happens
all the time on SoC ADCs as they can be wired to pretty much anything
internally or externally.

This will really be true for the driver, the part looks to
have about 13 different measurement inputs it can take, all user-select
multiplexed into 1 register/channel. :/

That's fine, support them all independently then use available_scan_masks
to control which sets are possible. You end up with a lot of 'channels'
but a coherent interface. Sounds like 52 channels in that devices case
which isn't too bad - of which you can only have 4 at a time (or looking
at the sheet, only 1 at a time perhaps? - note for fiddly cases we have
the validate_scan_mask callback to do this in code - see validate_scanmask_onehot
for example).


I see, I'll look into this.

After looking over the max30100 driver, I've realized I really don't need to
be exposing these channels to sysfs at all as they are only useful measured
together in a triggered/timed buffer. Should clean things up a bit.

This is a common enough case on ADCs (particularly soc ones) where you have
sampling slots that can effectively be allocated to hundreds of channels,
but the ability to only pick a few at a time.

Looking at that part you might want to add some configuration (device tree
or similar) to restrict what channels are actually plausible given you either
have a weighcell or a body composition thingy attached to a given physical
input!


I think all inputs will be hooked up in a real use case, I'm not sure
though.

[...]




The led version of ambient strikes me as odd to start with given I think the LED
is turned off during that measurement? This is merely to do with when they
occur in the sequence?

What we are really dealing with here is a single photodiode and an led sequencer.
Perhaps we need a modifier that simply means the source is an led driven at the same instance?
(this is the same as for proximity sensors, but there the signal is explicitly proximity).


Yeah, the device is basically one photodiode and one ADC feeding to one of four storage
registers. The sequencer controls which LEDs are on, what buffer to fill, and
when the ADC is sampling from which buffer to which register. This is all user definable
so you can sample one LED twice, or not even sample the ambient light at all if you
want.

This I why I would like to keep the input names locked to the data sheet, they are named
based on the name of the sequencer control that fills them. Abstracting this away would
add endless confusion.

Maybe, we should be treating these as a different type entirely? They are measuring light
levels, but in common with proximity sensors the 'interesting' bit is what is effecting
those levels. Perhaps a new type would make sense.
How about:

in_intensitytransmittedX_raw
in_intensityX_raw

This makes a mess of the differential channels however, as suddenly they are taking the
difference of two signals of different types. Ah well there goes a good plan. I'd neglected
that the transmitted version is the combination of the ambient and the transmitted.

This is irritatingly hard to map onto anything generic.


Exactly, there is no reason to enforce generic names for devices like these.
If there is going to be more than one of them and a common userspace library
then we need to have at least a consistent ABI.

Sure, so then I would just avoid the issue by not adding another type for this,
mostly one off, case.
I'm wondering ultimately how one off it is... What over devices use light transmitted...
Hmm. scanners etc I guess, can't think of other cases with a single led and light sensor
off the top of my head.. Ahah, optical swipe card readers (I'm sure I saw one somewhere
once ;)


Radar, X-ray, if you include all reflected electromagnetic waves as light...
Fair point (my day job is x-ray so I ought to have thought of that - we use pin diodes
on some machines to indirectly estimate very small masses).

Still this got more complex in my mind when I saw the other device vaguely similar to this
one that I have a driver to review for... Matt Rantostay's
[RFC v1] iio: light: add MAX30100 oximeter driver support

I think that type is using reflected light rather than transmitted for a similar job. What
fun.

Actually if you wouldn't mind taking a look at Matt's driver as someone rather more
knowledgeable about these kinds of drivers than me that would be great!


ACK, I'll comment on that thread if I find something interesting. Looks like the health
folder will be growing :)

[...]


Yes, the framework grows over time, and yes it needs to be extended. This is only
natural as new devices turn up that do new things.

Be careful to note that your strings naming the things would be just as much part of
the ABI as any new modifier or channel type.


Not necessarily, if the names match a regualar pattern or are provided to userspace in
a standard way, it wouldn't be any different that any other ABI that has different files
available or returns different values depending on what devices are available.

I agree, so where is the advantage? All you end up with is a massive look up table
of namings. We have that now, just the other way around and deliberately more restrictive
to try and keep life sane fo the userspace libraries.


This will help us to lose the lookup table we need now, the available sysfs names and
their uses can all be read out dynamically from a single common interface.
It's just moving the look up table around. From a review point of view I much
prefer the restrictions we effectively apply now by having it done this way as
it means I can be 99% sure that most drivers are within the ABI (sure we could write
tools to check this doing it the other way)

I can see it being nice from a review point of view, no real objections to the current
way, just an idea.


[...]

+
+What: /sys/bus/iio/devices/iio:deviceX/out_current_offdac_ledY_raw
+ /sys/bus/iio/devices/iio:deviceX/out_current_offdac_ledY_ambient_raw
+Date: October 2015
+KernelVersion:
+Contact: Andrew F. Davis <afd@xxxxxx>
+Description:
+ Get and set the offset cancellation DAC setting for these
+ stages.
+ Values range from 0 -> 15
Are these in mA?

Not sure I like the naming here. You could either treat them as explicit output
channels, or (and I'd be tempted to favour this) as calibration offsets for the
in_intensitytransmitted_ channel described above (or maybe the straight intensity
channels - I'm now confused on what is what here!).


Can you imagine how the user will feel if we try to hide all the details with
these names? The data sheet calls them 'offdac_led1' so why hide that.
Because the next datasheet that comes along for a different part might call
them something subtly different then we end up with needing custom userspace
code for each part. If we do that then there is no point in having the devices
in IIO in the first place. The reason all this ABI needs to be considered from
a generic point of view is that we are setting precedence. Naming should not
be defined by what it happened to be called on the particular instance of
the datasheet against which the first driver was defined (and yes we have
had instances of the names changing entirely on datasheets).

The point is to come up with ABI that is generic. That is probably the most
important part of IIO (and the bit we spend most time discussing / arguing about).

This is a calibration offset applied to the incoming signal - arguably by calling
offdac_led1 you are obscuring the useful information to the user which is 'what
is this for?'.


If anything they would be offsets for the in_intensity_ledX_raw channels, but
then I'm not sure how you would handle types, the offset is set with current,
the measured value is in intensity.
The advantage of caliboffset is it's unscaled and the relationship to the output
is deliberately never defined as it's rarely linear - so 'what' it is doesn't
actually matter.

We have these on IMUs for example - they often correspond to something magic
in the analog front end that is not even in the datasheet - though if you are
lucky there is an application note explaining the magic test needed to derive
a value (sometimes read from another register under some particular condition).
Usually they are just burnt in values that no one normally touches.


Hmm, looking back at the data sheet these gains are not for the individual channels,
they change the whole front end gain, so they probably won't work as channel
claiboffsets.
That's what info_mask_shared_by_all is for.



Hmmm, I may have been misinterpreting it's use, I'll look into this.


After looking into that, I still have no idea how this needs to be organized.
What do you think should be a channel, what should be a channel modifier, what
should get what.

I'll resend the series with my current best guess, maybe we can get something
set in stone next time.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/