Hi Manuel, > > I updated my drivers and the iio_utils to the current state in linux > staging Excellent. > and I must say I'm still not very satisfied with the > userspace ABI. Maybe we can clean this up until more drivers get into > staging and would have to be restructured accordingly. Definitely some rough edges still that need cleaning up. > > 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_scale Maybe... 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. 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?) Note that a similar level of complexity exists for the event interfaces. There we have multiple types of event corresponding to individual channels (each with typically 2 or 3 attributes) and also events corresponding to sets of channels, sometimes fun things like the sum squared value of a set of channels. This is easily the worst bit of the abi at the mo. (not to say we shouldn't fix the more stable bits!) I can't really see an equivalent of the above for that directory. If we do this one and not the events then we will have an inconsistent approach to the interfaces. To give an idea, for the adis16350 driver I have been working with recently (not quite finished) - we have: (and this interface is definitely up for debate) Note this device is fairly straight forward and doesn't have any alarms across multiple channels (and hence would falling into directories - perhaps not the best illustration of my point!) in_supply_thresh_rising_en in_supply_thresh_rising_value in_supply_thresh_falling_en in_supply_thresh_falling_value in_supply_roc_rising_en in_supply_roc_rising_value in_supply_roc_rising_period in_supply_roc_falling_en in_supply_roc_falling_value in_supply_roc_falling_period gyro_x_thresh_rising_en gyro_x_thresh_rising_value gyro_x_thresh_falling_en gyro_x_thresh_falling_value gyro_x_roc_rising_en gyro_x_roc_rising_value gyro_x_roc_rising_period gyro_x_roc_falling_en gyro_x_roc_falling_value gyro_x_roc_falling_period etc for gyro_y, gyro_z, accel_x, accel_y, accel_z, temp_x, temp_y, temp_z and in0 (the code supporting this lot is 'interesting' due to the pair of available alarms, I've restricted things marginally by not allowing both alarms to be the same parameter with different thresholds...) Yes this interface is huge, but it's the only option we have yet had suggested that is truly generalizable. The other approach (similar to the old 'scan' approach for channels) doesn't scale given some devices have the same number (or more) of alarms as channels. Anyhow, back to the original question. I think the gains do not out weight breaking from our attempts to match hwmon where ever possible. > 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? > > - 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:trigger0 I 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 > > - 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 space Hmm. 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? > > - 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. > * Network byte order? > * 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. Jonathan -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
