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

["Andrew F. Davis" <afd-l0cyMroinI0@public.gmane.org>]

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-l0cyMroinI0@public.gmane.org>
>>>>>>>> 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-l0cyMroinI0@public.gmane.org>
>>>>>>>>> +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.