Hi Jonathan, Am 27.08.2010 14:13, schrieb Jonathan Cameron:
The points I'd like to clean up are: - We currently have plenty of (optional) information per channel: * the raw value * an offset and scale to apply in user software * calibration offset and gain applied in the device * enable and bit size (not implemented yet) in the ring buffer Should we put all that stuff into a channel directory? |- /sys/bus/iio/device0/accel_x/ |- raw_value |- offset |- scale |- calib_offset |- calib_scaleMaybe... The disadvantages are: * Breaks the abi compatibility we have been trying to maintain with hwmon. * Lots more attribute groups (actually this is an advantage in some places where we have a driver covering devices with different numbers of channels) * Cannot share attributes of the above types - e.g. often the offset and scale apply to all channels. The abi allows this to be specified by a single attribute. It also allows say all acceleration offsets to be given by simply accel_offset.
Actually we still could have these shared files or a shared directory:
|- /sys/bus/iio/device0/accel/
|- offset
|- scale
|- /sys/bus/iio/device0/accel_x/
|- raw_value
|- /sys/bus/iio/device0/accel_y/
|- raw_value
Not that nice, I have to agree. Another idea would be to use some other
separator, like ':'. Then it's also clear for the human user, what's the
name and what's the postfix:
|- /sys/bus/iio/device0/accel_x:raw
|- /sys/bus/iio/device0/accel_x:offset
|- /sys/bus/iio/device0/accel_x:scale
Advantages: * It looks nicer to a human reader. (though anyone can deploy "ls *accel_x*" to get much the same. * Possibly some minor simplifications in library code. (fairly trivial I would expect?)
Yes, that's not too complicated now. We just have to define the postfixes somewhere. Right now, adis16400 uses _offset for the internal calibration offset and _scale for user space scaling.
Note that a similar level of complexity exists for the event interfaces.
> ..zip..I'm not so familiar with the event stuff, but I don't think we need to make it consistent, if there are different arguments for the one or the other solution. Yet we should use the same seperator, so if we switch to ':' in the channels, we should also switch in the event interface.
The enable and bit size are maybe buffer specific and could go into the buffer directory.We could do this now I guess (couldn't before the max1363 was switched to the current abi) As things currently stand these are in iio:device[n]/scan_elements. I guess we could move them into iio:device[n]/buffer0 It is a bit of a pain in code as the buffer directory is completely managed by the buffer implementation rather than the driver. Still I guess this could be passed to the buffer initialization code. Things are a bit tricky as there are other attributes caused by the buffer it self. It cleans up the abi at the cost of further messing up the separation between buffer implementation and device driver and some complexity in driver. It would be easier to move the scan_elements directory into the buffer directory? Perhaps that is the better option? What do others think on this?
I have no problem with moving the whole directory.
- The trigger is also an iio device so I'd prefer to have: |- /sys/bus/iio/iio0 (can be empty, if it's a pure trigger) |- /sys/bus/iio/iio0:trigger0I did think about this one (note it was marked as to be considered in the last abi discussion). The only reason to have the iio0 directory is if it tells us something. Currently the iio:device0 is the top level device. Is your intent that iio0 will have children of iio0:device0 and iio0:trigger0 etc? Thus it won't be empty under any circumstances. It will always contain at least iio0:device0 or iio0:trigger0
I just thought of renaming device<n> to iio<n> but we can also move the whole device directory. In 'lsiio' I have a structure like:
Device 000: <name>
<channel_group> (if available)
<channel>
:
<channel_group> (if available)
<channel>
:
buffer<n> (if available)
bytes per datum: <bpd>, length: 64
event: /dev/device0:buffer0:event0
access: /dev/device0:buffer0:access0
trigger<m>: <trigger_name> (if available)
So for a single trigger it would be:
# lsiio -v
Device 000: gpio_trigger
trigger0: irqtrig30
trigger1: irqtrig45
trigger2: irqtrig51
or in sysfs:
|- /sys/bus/iio/iio0/
|- name
|- iio0:trigger0
|- name
|- iio0:trigger1
|- name
|- iio0:trigger2
|- name
- Consitent names in ABI and code: * bps: 'bits per sample' or 'bytes per scan'?Yup, that's why bpd was introduced in the code.* bpd: 'bytes per datum' but named bps in user spaceHmm. there does seem to be some confusion still around this. Given we also have bpse (bits per scan element) the confusion gets even worse. Perhaps the right option is to loose the acronyms entirely and have it long hand both in core code (drivers don't really matter for this) and the userspace abi?
OK, I'll prepare a patch.
- Endianess of the buffers: * Device specific? Then we need a sysfs file to publish this info. * CPU native byte order?It is currently cpu native for all software ring buffers. Agreed this may change and hardware buffers may do either. So you are quite right, we need an attribute for this. So I guess we support 3 options, cpu native, big endian, little endian. So what shall we call it? byte_order - [big little native] Mostly read only, though sooner or later I expect we will get some device that allows this to be controlled.
Do we really need native? I think the driver can figure out what the native order is, and just give that one (even at compile time). Another possibility is to let the driver always convert to native. At least that's what most userspace software want's to have. I recently sent a patch for sca3000 to the linux-iio list.
* Allow packed buffers for samples with fractional sizes?Probably not using the current ring buffer. As you say a big issue is how to describe a packed storage particularly if not all of it is packed. (so say 2 x 11 bit readings in 3 bytes - for say a 4 channel device, this is a better bet than packing it tightly into 44 bits.) So far we haven't had a hardware ring device giving us packed data, but I'm sure one will turn up and force this element sometime in the future. I think we leave this until a user comes along then pin it down then. I agree it is definitely something we need to consider, just not now! Still if you want to lay out some guidance for discussion feel free.
OK, no packed buffers for now, but we should implement variable sample sizes for standard types. Indeed we already have this for the timestamp, which is always 64 bit.
To be compatible with future extensions we could have:
|- /sys/bus/iio/ii0/buffer0/scan_elements/
|- accel_x:en (0 or 1)
|- accel_x:type (i.e. s14/16, see *)
|- accel_x:index
* s14/16 means signed 14 bits, stored in 16 bits, right aligned. If it's
left aligned we can just modify the scale attribute and give the 16 bit
interpretation in <channel>:raw.
I don't like the index prefix any more, even I had proposed this once. This is because for devices with more than 10 channels (adis16400) we have to get a leading 0 in the name to maintain alphabetical sorting, which is nearly impossible with the current macros.
Cheers, -- Dipl.-Inf. Manuel Stahl Fraunhofer-Institut für Integrierte Schaltungen IIS - Leistungsoptimierte Systeme - Nordostpark 93 Telefon +49 (0)911/58061-6419 90411 Nürnberg Fax +49 (0)911/58061-6398 http://www.iis.fraunhofer.de manuel.stahl@xxxxxxxxxxxxxxxxx
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
