Re: [RFC] Staging:IIO: New ABI

From: Jonathan Cameron
Date: Sun Jan 24 2010 - 06:26:01 EST


Hi Greg,
>>> On Wed, Jan 20, 2010 at 03:13:40PM +0000, Jonathan Cameron wrote:
>>>> What: /sys/class/iio/device[n]
>>>> Description:
>>>> Hardware chip or device accessed by on communication port.
>>>> Corresponds to a grouping of sensor channels.
>>>>
>>>> What: /sys/class/iio/trigger[n]
>>>> Description:
>>>> An event driven driver of data capture to an in kernel buffer.
>>>> May be provided a device driver that also has an IIO device
>>>> based on hardware generated events (e.g. data ready) or
>>>> provided by other hardware (e.g. periodic timer or gpio)
>>>> Contains trigger type specific elements. These do not
>>>> generalize well.
>>>>
>>>>
>>>> What: /sys/class/iio/ring_buffer[m]
>>>> Description:
>>>> Link to /sys/class/iio/device[n]/ring_buffer[m]. Ring buffer
>>>> numbering may not match that of device as some devices do not
>>>> have ring buffers.
>>>
>>> Why is this link needed? Why can't you just look in the device
>>> directory for a ring buffer? And wouldn't the ring buffer be 1..n for
>>> every device? They shouldn't be "unique" for all iio devices, that
>>> would be wierd.
>> I'm a little unclear what you mean here (so my apologies if I'm misinterpreting!)
>> A given IIO device will indeed have a either none or typically 1 ring buffers.
>> They are not currently shared across devices. Aggregation if desired is done
>> in userspace.
>
> Ok, that's fine, but the name of those buffers do not have to be unique
> to all ring buffers in the system.
>
> Hm, oh yeah, they do, ok, nevermind, stupid me :)
>
>>>> What: /sys/class/iio/device[n]/name
>>>> Description:
>>>> Description of the physical chip / device. Typically a part
>>>> number.
>>>>
>>>> What: /sys/class/iio/device[n]/volt_[name][m]_raw
>>>> Description:
>>>> Raw (unscaled no bias removal etc) voltage measurement from
>>>> channel m. name is used in special cases where this does
>>>> not correspond to externally available input (e.g. supply
>>>> voltage monitoring).
>>>
>>> So what would 'name' be?
>> This is for a few device corner cases such as volt_supply_raw for the the
>> adis16350 below. It can't really be used as a general purpose ADC
>> so the idea was to separate it out. The vast majority of voltage channels
>> would simply be numbered. There is an argument that, it might make sense
>> to support this single parameter via hwmon, but then there are high end
>> gyros where we would be grabbing the voltage, temperature and actual reading
>> on each trigger so as to be able to compensate for the huge difference
>> these can make to the resulting data.
>
> But again, what would 'name' be?
Probably an option from a small list. (supply being the only element right now!)
Actually, whilst the list is small we might as well put it straight in the abi
description line.

>
>>>> What: /sys/class/iio/device[n]/volt_[name][m]_scale
>>>> Description:
>>>> If known for a device, scale to be applied to volt[m]_raw post
>>>> addition of volt[m]_offset in order to obtain the measured voltage
>>>> in volts. If shared across all voltage channels the m is not present.
>>>
>>> For all of these voltage measurements, why use something different from
>>> what the kernel already supports for the existing hardware monitoring
>>> devices? There is already a well-defined api for these things.
>> Agreed. We did consider using in0 etc as per hwmon but decided that we were
>> breaking sufficiently from the approach there (where the readings are always
>> provided in microvolts iirc) that we could change the name to something that
>> was a little more specific. I'm not overly tied to volt but the semantics
>> are different enough here (with offsets and gain passed to userspace) that
>> we may actually create confusion by mirroring that api.
>
> I really think you should merge with that api somehow.
>
> I understand that you don't want to do conversions in the kernel, so
> perhaps there could be some way to show this like you have described,
> that the hwmon interface could also use.
>
> That way userspace tools will work for all types of devices, and you
> don't have a custom interface just for these device. Unification is a
> good thing :)
A good point. I'll have a think about how to do this then post an update
including the hwmon list to see if we can work out what needs adding to their
current interface.
>
>>>> What: /sys/class/iio/device[n]/accel_[x|y|z][m]_raw
>>>> Description:
>>>> Acceleration in direction x, y or z (may be arbitrarily assigned
>>>> but should match other such assignments on device)
>>>> channel m (not present if only one accelerometer channel at
>>>> this orientation). Has all of the equivalent parameters as per volt[m].
>>>> Units after application of scale and offset are m/s^2.
>>>
>>> Shouldn't this just use the existing input accelerometer interface so as
>>> to keep userspace sane?
>> Again, it comes down to whether we process the data or not. IIO is all about
>> the ability to handle things fast. These sysfs interfaces are actually meant
>> to mirror the chrdev ring buffer accesses which are the primary access point
>> for higher end devices. A lot of the use cases are either interested in logging
>> for later use, or algorithms that will run in the device units (sometime in integer
>> arithmetic). Hence all conversion to sane units is left to userspace. The
>> drivers merely provide sufficient information to do this conversion if it
>> is desired.
>
> See above. I suggest working with the hwmon developers to add the "raw"
> type interface to their api making these able to be used there as well.
>
>>>> What: /sys/class/iio/device[n]/event_line[m]
>>>> Description:
>>>> Configuration of which hardware generated events are passed up to
>>>> userspace. Some of these are a bit complex to generalize so this
>>>> section is a work in progress.
>>>>
>>>> What: /sys/class/iio/device[n]/event_line[m]/dev
>>>> Description:
>>>> major:minor character device numbers.
>>>
>>> No, don't bury devices down a level in sysfs, that's not ok. Actually,
>>> I think it would take you a lot of work to get this to properly work on
>>> the implementation side :)
>> This bit of the documentation is indeed wrong (oops). These are members of the IIO
>> class and so this is just a link (much like the event elements in input) created
>> by making them in class iio with the device as a a parent. Again, application
>> wise we aren't normally interested in aggregation so there is one (or more) of these
>> per device.
>
> Ok.
>
>>>
>>> If event_lines need device nodes, then they should be real devices.
>> (oops) The are ;)
>
> Heh, that will not work, as Kay described :)
>
>>> Actually, all of this looks like it needs to be a bus, not a class, if
>>> you are having this many different types of things hanging off of them.
>>> Have you thought about that instead? It would make your code a lot
>>> easier in the end.
>> That is definitely an option. The reason we didn't is more to do with following
>> the most similar current case (input) than any particular preference.
>
> As Kay said, this should be a bus, and you should have devices attach to
> them, be it virtual or not. Or, you just tie into the hwmon interface
> which makes it much easier for you overall.
We'll definitely move over to a bus. I'd fallen for the naming and never
really considered it as an option! As for tying into hwmon, I'll take another
look as I can see it may be simpler with the bus structure separating the different
'devices'.
>
>>>> Again taking accel_x0 as example and serious liberties with ABI spec.
>>>>
>>>> What: /sys/.../event_line[m]/accel_x0_thresh[_high|_low]
>>>> Description:
>>>> Event generated when accel_x0 passes a threshold in correction direction
>>>> (or stays beyond one). If direction isn't specified, either triggers it.
>>>> Note driver will assume last p events requested are enabled where p is
>>>> however many it supports. So if you want to be sure you have
>>>> set what you think you have, check the contents of these. Drivers
>>>> may have to buffer any parameters so that they are consistent when a
>>>> given event type is enabled a future point (and not those for whatever
>>>> alarm was previously enabled).
>>>>
>>>> What: /sys/.../event_line[m]/accel_x0_roc[_high|_low]
>>>> Description:
>>>> Same as above but based on the first differential of the value.
>>>>
>>>>
>>>> What: /sys/.../event_line[m]/accel_x0[_thresh|_roc][_high|_low]_period
>>>> Description:
>>>> A period of time (microsecs) for which the condition must be broken
>>>> before an interrupt is triggered. Applies to all alarms if type is not
>>>> specified.
>>>>
>>>> What: /sys/.../event_line[m]/accel_x0[_thresh|_roc][_high|_low]_value
>>>> Description:
>>>> The actual value of the threshold either in raw device units
>>>> obtained by reverse application of scale and offfset to the
>>>> acceleration in m/s^2.
>>>
>>> Again, look at our existing apis, I think we already cover these types
>>> of things, don't create new ones please.
>> I am not aware of these. Could you direct me to the current api? Also note that these
>> aren't the actual alarms, merely a means of enabling the relevant event on the related
>> event character device.
>
> Hm, I thought we had an accelerator interface somewhere...
(cleared up by Dmitry further down the thread)
>
>>>> What: /sys/.../event_line[m]/free_fall
>>>> Description:
>>>> Common hardware event in accelerometers. Takes no parameters.
>>>
>>> I know we have this one already, please use it.
>>
>> Again, does it make sense to match api when the result is very different? The event
>> infrastructure in IIO is much slimmer than that in input (this was one of the original
>> reasons for not simply adding these things to input in the first place). This particular
>> example is a bit unusual in that the current api (hwmon/lis3lv02d.c for example) handles
>> this via a dedicated freefall character device. This doesn't really generalize to the more
>> complex events handled here. To be honest this one doesn't really make sense in IIO's
>> intended applications so it might be best to drop it entirely!
>
> Yeah, a dedicated char device doesn't seem to make much sense either,
> but any unification you could find here would be nice. Even if it means
> the existing driver converts over to your new api :)
Fair point.

>
>>> Again, don't bury devices. Or if you are, use a bus, not a class,
>>> that's the wrong classification.
>> Cool, I'll look into making the change. What we really have here
>> is a single conceptual device using a pair or character interfaces. Is a bus
>> the right way to handle that?
>>
>> How about the following under a bus (this is for a single physical chip).
>>
>> device0
>> event0 (for physical events)
>> ringaccess0 (actual device from which data is read)
>> ringevent0 (associated event interface for the ring - ideally not aggregated with the event0)
>>
>> and sometimes:
>> trigger0
>>
>> Is that best way to go. Currently ringacces0 and ring event0 are
>> grouped under ring_buffer0 but that was simply for the reason they are
>> always matched.
>
> Yes, that would work, and make sense.
Excellent,

Thanks for taking a look at this!

Jonathan
--
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/