I've started looking at the current code base in staging,
this is basically a log of what I'm finding there that
may get improved:
* Your bus_type is not really a bus in the sense we normally
use it, because it does not have any driver/device matching.
You only register devices from the same files that drive
the hardware. This is normally done using a class, not
a bus.
* Since you specify the sysfs attributes globally in the
documentation, it would be nice if drivers did not have
to implement them as attribute show function, but as
a function that simply returns a value that is then
printed by common code. You can do this using a structure
of function pointers that each driver fills with the
relevant ones, like file_operations, replacing the
attribute groups.
* iio_allocate_device() could get a size argument and
allocate the dev_data together with the iio_dev in
a single allocation, like netdev_alloc does. In addition,
you can pass a structure with all constant data, such
as THIS_MODULE, and the operations and/or attribute groups,
instead of having to set them individually for each dev.
* It's not clear to me why you need one additional device
and one chardev for each interrupt line of a device, my
feeling is that this could be simplified a lot if you find
a way to represent an iio_dev with a single chardev to
user space. This may have been an attempt to avoid ioctl()
calls, but I think in this case an ioctl interface would
be cleaner than a complex hierarchy of devices.
Another alternative would be for the device driver to
register multiple iio_devs in case it needs multiple
interfaces.
* Neither of your two file operations supports poll(), only
read. Having poll() support is essential if you want to
wait for multiple devices in user space. Maybe it's
possible to convert the code to use eventfd instead of
rolling your own event interface?
* The name rip_lots doesn't really mean anything to me
as a non-native speaker. Maybe think of a better word for
this. Also, there is only one driver providing such a function,
so this is probably a good candidate for deferring to a later
stage, along with all the the entire ring file code that seems
rather pointless otherwise.
* The exported symbols should probably become EXPORT_SYMBOL_GPL
* iio-trig-sysfs should not be a platform driver
That's probably enough for today, to start some discussion
about the core. Overall, I'm pleasantly surprised by the
quality of the code, it's much better than I was expecting
after having looked at drivers from hardware vendors a lot
recently.
Arnd
Hi Arnd,
> I've started looking at the current code base in staging,
> this is basically a log of what I'm finding there that
> may get improved:
Thanks for taking a look!
>
> * Your bus_type is not really a bus in the sense we normally
> use it, because it does not have any driver/device matching.
> You only register devices from the same files that drive
> the hardware. This is normally done using a class, not
> a bus.
That's what I originally thought and indeed how it was
initially done. The responses from Greg KH and Kay Sievers
to one of our ABI discussions convinced me otherwise.
http://lkml.org/lkml/2010/1/20/233 + the previous email in
that thread.
>
> * Since you specify the sysfs attributes globally in the
> documentation, it would be nice if drivers did not have
> to implement them as attribute show function, but as
> a function that simply returns a value that is then
> printed by common code. You can do this using a structure
> of function pointers that each driver fills with the
> relevant ones, like file_operations, replacing the
> attribute groups.
This may indeed work. However it would be a fair bit
more fiddly than that a simple function pointer structure
as the sets of attributes supplied by different devices
vary considerably.
So to confirm I understand you correctly we either have something like:
/* access ops */
int (*get_accel_x)(struct iio_dev *devinfo)
char *(*get_accel_offset)(struct iio_dev *devinfo); /* note these can't return a value as
they may well be floating point */
char *(*get_accel_gain)(struct iio_dev *devinfo)
int (*get_accel_y)(struct iio_dev *devinfo)
Sticky cases that will make this fiddly.
* Unknown maximum number of some types of attributes.
- Could use a
int (*get_in)(struct iio_dev dev_info, int channel);
int num_in;
* Whatever you pick also immediately becomes restrictive as the main
class has to be updated to handle any new elements.
* Note we'll have function points for all the scan element related stuff
as well. Basically the set that would be needed is huge.
At the end of the day I'm unconvinced this makes sense from the point
of view of simplifying the driver code. The gain is that it enforces the abi
and may allow in kernel uses.
Ultimately we lifted this approach from hwmon where it works well
under similar circumstances.
Alternative is a small set such as:
/* access ops */
int (*get_channel_value)(struct iio_dev *info, enum CHANNEL);
char (*get_channel_offset)(struct iio_dev *info, enum CHANNEL);
and a list of which channels exist. Then the driver side becomes a massive
switch statement rather than the attribute tables. Again, I'm not convinced
that is actually cleaner or easier to understand. Also this approach pretty
much means you end up with large numbers of equal attributes as you can't
trivially group them. e.g we have accel_scale to cover case where it's the
same for all accelerometer axes.
>
> * iio_allocate_device() could get a size argument and
> allocate the dev_data together with the iio_dev in
> a single allocation, like netdev_alloc does. In addition,
> you can pass a structure with all constant data, such
> as THIS_MODULE, and the operations and/or attribute groups,
> instead of having to set them individually for each dev.
Good idea.
>
> * It's not clear to me why you need one additional device
> and one chardev for each interrupt line of a device, my
> feeling is that this could be simplified a lot if you find
> a way to represent an iio_dev with a single chardev to
> user space. This may have been an attempt to avoid ioctl()
> calls, but I think in this case an ioctl interface would
> be cleaner than a complex hierarchy of devices.
> Another alternative would be for the device driver to
> register multiple iio_devs in case it needs multiple
> interfaces.
In a sense this isn't quite as bad as it seems. The only cases we
have so far (and it's more or less all that's out there) are those where
the device has one interrupt line for 'events' - e.g threshold
interrupts of one type or another, and one for one for data ready
signals. The data ready signals are handled as triggers and hence
don't have a chardev (directly).
The other uses of chrdevs are for accessing buffers. Each buffer may have
two of them. The first is a dirty read chrdev. The second is
for out of band flow information to userspace. Basically you
control what events will come out of the flow control chrdev.
Programs block on that and read from the data flow chrdev
according to what comes down the line. The messy events stuff
is to handle event escalation. Dealing with getting a 50% full
event and then a 75% full event with no inherent knowledge of
if you read anything in between is a pain. We could specify
only one level event exists, which would simplify the code
somewhat. Perhaps wise for an initial merge though I'm loath
to not support functionality that is fairly common in hardware
buffers.
Some buffer implementations don't provide flow control (kfifo_buf)
so for them you just have a read chrdev (actually this isn't true
right now - I need to fix this). This chrdev is separate from
the 'event' chrdevs purely because for speed reasons. You don't
want to have what is coming down this line change except due
to software interaction (and to do that you normally have to stop
the buffer fill).
>
> * Neither of your two file operations supports poll(), only
> read. Having poll() support is essential if you want to
> wait for multiple devices in user space. Maybe it's
> possible to convert the code to use eventfd instead of
> rolling your own event interface?
I'll look into this one. Curiously I don't think it has
ever been suggested before. (maybe I'm just forgetful)
We have had the suggestion of using inputs event interface and
I am still considering that option. The issue
there is that it is really far too heavyweight. So far we have
no use cases for multiple readers and we never want to aggregate
across devices. Also input views events as carrying data. All
actual readings in our case are going through the faster buffer
route.
I agree entirely though that our event interface
needs a rethink.
>
> * The name rip_lots doesn't really mean anything to me
> as a non-native speaker. Maybe think of a better word for
> this.
Will do.
> Also, there is only one driver providing such a function,
> so this is probably a good candidate for deferring to a later
> stage, along with all the the entire ring file code that seems
> rather pointless otherwise.
Lots of drivers do it. They are simply using one of the software
provided buffers. ring_sw or kfifo_buf (and yes they do have
useless names as well so we'll clean that up for which ever
ones we keep!)
>
> * The exported symbols should probably become EXPORT_SYMBOL_GPL
Fair enough. That's a nice easy change to make ;)
>
> * iio-trig-sysfs should not be a platform driver
This one got discussed before merging it and I agree. We let it
go then because there was a clear use case, a working driver and
no one had the time to do a better job. Michael eventually talked Greg
round to letting it in.
http://marc.info/?t=129665398600002&r=1&w=2
>
> That's probably enough for today, to start some discussion
> about the core. Overall, I'm pleasantly surprised by the
> quality of the code, it's much better than I was expecting
> after having looked at drivers from hardware vendors a lot
> recently.
*laughs*
To be honest though most academic code is probably even worse.
Other than the Analog dump of drivers (which I'm still
not sure was a good idea) we have been fairly strictly reviewing
changes for a while now.
Thanks again for taking a look. This sort of general review
from someone not intimately tied up in the code is extremely useful!
Jonathan
On Wednesday 16 March 2011, Jonathan Cameron wrote:
> Hi Arnd,
> > I've started looking at the current code base in staging,
> > this is basically a log of what I'm finding there that
> > may get improved:
> Thanks for taking a look!
> >
> > * Your bus_type is not really a bus in the sense we normally
> > use it, because it does not have any driver/device matching.
> > You only register devices from the same files that drive
> > the hardware. This is normally done using a class, not
> > a bus.
> That's what I originally thought and indeed how it was
> initially done. The responses from Greg KH and Kay Sievers
> to one of our ABI discussions convinced me otherwise.
>
> http://lkml.org/lkml/2010/1/20/233 + the previous email in
> that thread.
(adding Kay to Cc here)
I see. However, I'd still disagree with Kay here. Given that
the common use of existing subsystems is to have "bus" for
things that are physically connected, and "class" for things
that are logical abstractions, I think we should continue
that way for new subsystems, even if there is little difference
technically.
There is little point discussing how things should have been
done in the past when we're stuck with them. The best we
can do now is make the code as consistent as possible.
> > * Since you specify the sysfs attributes globally in the
> > documentation, it would be nice if drivers did not have
> > to implement them as attribute show function, but as
> > a function that simply returns a value that is then
> > printed by common code. You can do this using a structure
> > of function pointers that each driver fills with the
> > relevant ones, like file_operations, replacing the
> > attribute groups.
> This may indeed work. However it would be a fair bit
> more fiddly than that a simple function pointer structure
> as the sets of attributes supplied by different devices
> vary considerably.
It's certainly more work for the subsystem, but the intention
is to simplify the individual drivers, to make things scale
better as many more drivers get added.
> So to confirm I understand you correctly we either have something like:
> /* access ops */
>
> int (*get_accel_x)(struct iio_dev *devinfo)
> char *(*get_accel_offset)(struct iio_dev *devinfo); /* note these can't return a value as
> they may well be floating point */
> char *(*get_accel_gain)(struct iio_dev *devinfo)
> int (*get_accel_y)(struct iio_dev *devinfo)
>
> Sticky cases that will make this fiddly.
You can't really return a char* because of the memory allocation,
but aside from that, the idea is right, yes.
Note that this forces all drivers to use the same unit
for data they return in the attributes, but I consider
this a good thing.
For attributes that really want to return a floating point
number, I think we should either define a fixed-point
form with a constant exponent, or some other representation
that is allowed in the kernel.
> * Unknown maximum number of some types of attributes.
> - Could use a
> int (*get_in)(struct iio_dev dev_info, int channel);
> int num_in;
Good point. So a driver may have multiple attributes
of the same type, which need to get enumerated, but should
all behave the same way, right?
Your suggestion seems good, but I'd still recommend making
it so that the core chooses the attribute names, not the
device driver. One possible way to lay this out is something
like:
/* global defines */
enum iio_channel_type {
IIO_CHAN_TEMPERATURE,
IIO_CHAN_ADC,
IIO_CHAN_FOO,
...
};
static const char *channel_names[] = {
[IIO_CHAN_TEMPERATURE] = "temperature%d",
[IIO_CHAN_ADC] = "adc%d",
[IIO_CHAN_FOO] = "foo%d",
...
};
typedef unsigned long long iio_result_t;
struct iio_channel_defs {
iio_result_t (*get)(struct iio_dev *dev_info, int channel);
enum iio_channel_type channels[0];
};
/* example driver */
enum {
FOO_CHAN_TEMP0,
FOO_CHAN_TEMP1,
FOO_CHAN_ADC,
};
struct iio_channel_defs foo_driver_channels = {
.get = &foo_get_sensor,
.channels = {
/* this device has two temperature and one adc input */
[FOO_CHAN_TEMP0] = IIO_CHAN_TEMPERATURE,
[FOO_CHAN_TEMP1] = IIO_CHAN_TEMPERATURE,
[FOO_CHAN_ADC] = IIO_CHAN_ADC,
},
};
iio_result_t foo_getsensor(struct iio_dev *dev_info, int channel)
{
switch (channel) {
case FOO_CHAN_TEMP0:
return read_temp0(dev_info);
case FOO_CHAN_TEMP1:
return read_temp1(dev_info);
case FOO_CHAN_ADC:
return read_adc(dev_info);
}
return -EINVAL;
}
> * Whatever you pick also immediately becomes restrictive as the main
> class has to be updated to handle any new elements.
I believe that's a good thing, we want to limit the number
of different interfaces that people come up with. If two
drivers are similar enough, they should probably use the
same one.
It also makes it easier to verify that all attributes are
documented properly, because the documentation only has
to be matched with a single file, not with every single
driver.
It also forces out-of-tree modules to use the same interfaces
that other drivers are using. If someone has hardware for
something new, it will only have an interface to talk to
after it has been discussed and documented in mainline.
> * Note we'll have function points for all the scan element related stuff
> as well. Basically the set that would be needed is huge.
>
> At the end of the day I'm unconvinced this makes sense from the point
> of view of simplifying the driver code. The gain is that it enforces the abi
> and may allow in kernel uses.
> Ultimately we lifted this approach from hwmon where it works well
> under similar circumstances.
>
> Alternative is a small set such as:
>
> /* access ops */
> int (*get_channel_value)(struct iio_dev *info, enum CHANNEL);
> char (*get_channel_offset)(struct iio_dev *info, enum CHANNEL);
>
> and a list of which channels exist. Then the driver side becomes a massive
> switch statement rather than the attribute tables. Again, I'm not convinced
> that is actually cleaner or easier to understand. Also this approach pretty
> much means you end up with large numbers of equal attributes as you can't
> trivially group them. e.g we have accel_scale to cover case where it's the
> same for all accelerometer axes.
The whole point would be to avoid duplication, so maybe something
a little smarter is needed, but the current code duplicates the
definition of attributes to every driver, and I'm sure we can
find a solution that requires less code.
In many cases, you should be able to replace a long switch statement
by a table driven approach inside of the driver.
> > * It's not clear to me why you need one additional device
> > and one chardev for each interrupt line of a device, my
> > feeling is that this could be simplified a lot if you find
> > a way to represent an iio_dev with a single chardev to
> > user space. This may have been an attempt to avoid ioctl()
> > calls, but I think in this case an ioctl interface would
> > be cleaner than a complex hierarchy of devices.
> > Another alternative would be for the device driver to
> > register multiple iio_devs in case it needs multiple
> > interfaces.
> In a sense this isn't quite as bad as it seems. The only cases we
> have so far (and it's more or less all that's out there) are those where
> the device has one interrupt line for 'events' - e.g threshold
> interrupts of one type or another, and one for one for data ready
> signals. The data ready signals are handled as triggers and hence
> don't have a chardev (directly).
If you only have the two, you might be able to handle them both
in the poll function, where the regular case means POLLIN
or POLLRDNORM, while the threshold interrupt gets turned
into POLLPRI or POLLRDBAND.
I'm not sure if that covers all cases you are interested in,
but it sounds like a useful simplification. It also makes
it possible for user programs to only wait for one type of
event.
> The other uses of chrdevs are for accessing buffers. Each buffer may have
> two of them. The first is a dirty read chrdev. The second is
> for out of band flow information to userspace. Basically you
> control what events will come out of the flow control chrdev.
> Programs block on that and read from the data flow chrdev
> according to what comes down the line. The messy events stuff
> is to handle event escalation. Dealing with getting a 50% full
> event and then a 75% full event with no inherent knowledge of
> if you read anything in between is a pain. We could specify
> only one level event exists, which would simplify the code
> somewhat. Perhaps wise for an initial merge though I'm loath
> to not support functionality that is fairly common in hardware
> buffers.
>
> Some buffer implementations don't provide flow control (kfifo_buf)
> so for them you just have a read chrdev (actually this isn't true
> right now - I need to fix this). This chrdev is separate from
> the 'event' chrdevs purely because for speed reasons. You don't
> want to have what is coming down this line change except due
> to software interaction (and to do that you normally have to stop
> the buffer fill).
Doesn't epoll handle the event escalation with its edge triggered
mode? Basically you can get notified every time that there is
more to be read.
> > * Neither of your two file operations supports poll(), only
> > read. Having poll() support is essential if you want to
> > wait for multiple devices in user space. Maybe it's
> > possible to convert the code to use eventfd instead of
> > rolling your own event interface?
> I'll look into this one. Curiously I don't think it has
> ever been suggested before. (maybe I'm just forgetful)
>
> We have had the suggestion of using inputs event interface and
> I am still considering that option. The issue
> there is that it is really far too heavyweight. So far we have
> no use cases for multiple readers and we never want to aggregate
> across devices. Also input views events as carrying data. All
> actual readings in our case are going through the faster buffer
> route.
I'm slightly confused, mainly because I have no idea how the
buffer and event interfaces are actually being used. I would
have expected an interface as simple as:
* One iio device per sensor that can provide data, possibly
many per hardware device
* One chardev for each iio device
* Use epoll to wait for data and/or out-of-band messages
* Use chrdev read to get events from the buffer
* Use sysfs to read from sensors that are not event based
What you have is obviously more complex than this, and you
probably have good reasons that I still need to understand.
> > Also, there is only one driver providing such a function,
> > so this is probably a good candidate for deferring to a later
> > stage, along with all the the entire ring file code that seems
> > rather pointless otherwise.
> Lots of drivers do it. They are simply using one of the software
> provided buffers. ring_sw or kfifo_buf (and yes they do have
> useless names as well so we'll clean that up for which ever
> ones we keep!)
Ok, I see. I did not realize that iio_rip_sw_rb was getting assigned
to ->rip_lots.
> > * iio-trig-sysfs should not be a platform driver
> This one got discussed before merging it and I agree. We let it
> go then because there was a clear use case, a working driver and
> no one had the time to do a better job. Michael eventually talked Greg
> round to letting it in.
>
> http://marc.info/?t=129665398600002&r=1&w=2
Ok. I still think that it is a really bad idea because the driver
is essentially a software feature that could be use on any machine,
but using a platform device ties it directly to a specific machine
that registers the device. It would make much more sense to
remove all the driver registration from there and just create the
iio device at module load time, or to have some interface that
can be used to dynamically create these triggers if the module
is loaded.
Arnd
On 03/16/11 13:33, Arnd Bergmann wrote:
> On Wednesday 16 March 2011, Jonathan Cameron wrote:
>> Hi Arnd,
>>> I've started looking at the current code base in staging,
>>> this is basically a log of what I'm finding there that
>>> may get improved:
>> Thanks for taking a look!
>>>
>>> * Your bus_type is not really a bus in the sense we normally
>>> use it, because it does not have any driver/device matching.
>>> You only register devices from the same files that drive
>>> the hardware. This is normally done using a class, not
>>> a bus.
>> That's what I originally thought and indeed how it was
>> initially done. The responses from Greg KH and Kay Sievers
>> to one of our ABI discussions convinced me otherwise.
>>
>> http://lkml.org/lkml/2010/1/20/233 + the previous email in
>> that thread.
>
> (adding Kay to Cc here)
>
> I see. However, I'd still disagree with Kay here. Given that
> the common use of existing subsystems is to have "bus" for
> things that are physically connected, and "class" for things
> that are logical abstractions, I think we should continue
> that way for new subsystems, even if there is little difference
> technically.
>
> There is little point discussing how things should have been
> done in the past when we're stuck with them. The best we
> can do now is make the code as consistent as possible.
Leaving this for Kay.
>
>>> * Since you specify the sysfs attributes globally in the
>>> documentation, it would be nice if drivers did not have
>>> to implement them as attribute show function, but as
>>> a function that simply returns a value that is then
>>> printed by common code. You can do this using a structure
>>> of function pointers that each driver fills with the
>>> relevant ones, like file_operations, replacing the
>>> attribute groups.
>> This may indeed work. However it would be a fair bit
>> more fiddly than that a simple function pointer structure
>> as the sets of attributes supplied by different devices
>> vary considerably.
I've cc'd Jean and Guenter as they may be able to offer so
insight from what they see in hwmon (sorry for dropping you
in half way through a discussion!)
>
> It's certainly more work for the subsystem, but the intention
> is to simplify the individual drivers, to make things scale
> better as many more drivers get added.
I'm yet to be convinced this simplifies things. We basically move
from one form of big table in the drivers to another and add complexity
to the core. I'd be fine with that complexity if it actually simplified
the drivers, but I'm not sure it does. Perhaps the only way to really
answer that is to actually try doing it with a few drivers and see
where we end up.
>
>> So to confirm I understand you correctly we either have something like:
>> /* access ops */
>>
>> int (*get_accel_x)(struct iio_dev *devinfo)
>> char *(*get_accel_offset)(struct iio_dev *devinfo); /* note these can't return a value as
>> they may well be floating point */
>> char *(*get_accel_gain)(struct iio_dev *devinfo)
>> int (*get_accel_y)(struct iio_dev *devinfo)
>>
>> Sticky cases that will make this fiddly.
>
> You can't really return a char* because of the memory allocation,
> but aside from that, the idea is right, yes.
sure. It would need a bit of management code around that or
some suitable encapsulation.
>
> Note that this forces all drivers to use the same unit
> for data they return in the attributes, but I consider
> this a good thing.
True, though the other approach to enforcing this is what hwmon
does. Thorough review against the spec for all new drivers.
>
> For attributes that really want to return a floating point
> number, I think we should either define a fixed-point
> form with a constant exponent, or some other representation
> that is allowed in the kernel.
>
>> * Unknown maximum number of some types of attributes.
>> - Could use a
>> int (*get_in)(struct iio_dev dev_info, int channel);
>> int num_in;
>
> Good point. So a driver may have multiple attributes
> of the same type, which need to get enumerated, but should
> all behave the same way, right?
>
> Your suggestion seems good, but I'd still recommend making
> it so that the core chooses the attribute names, not the
> device driver. One possible way to lay this out is something
> like:
>
> /* global defines */
> enum iio_channel_type {
> IIO_CHAN_TEMPERATURE,
> IIO_CHAN_ADC,
> IIO_CHAN_FOO,
> ...
> };
>
> static const char *channel_names[] = {
> [IIO_CHAN_TEMPERATURE] = "temperature%d",
> [IIO_CHAN_ADC] = "adc%d",
> [IIO_CHAN_FOO] = "foo%d",
> ...
> };
>
> typedef unsigned long long iio_result_t;
>
> struct iio_channel_defs {
> iio_result_t (*get)(struct iio_dev *dev_info, int channel);
>
> enum iio_channel_type channels[0];
> };
>
> /* example driver */
> enum {
> FOO_CHAN_TEMP0,
> FOO_CHAN_TEMP1,
> FOO_CHAN_ADC,
> };
>
> struct iio_channel_defs foo_driver_channels = {
> .get = &foo_get_sensor,
>
> .channels = {
> /* this device has two temperature and one adc input */
> [FOO_CHAN_TEMP0] = IIO_CHAN_TEMPERATURE,
> [FOO_CHAN_TEMP1] = IIO_CHAN_TEMPERATURE,
> [FOO_CHAN_ADC] = IIO_CHAN_ADC,
> },
> };
>
> iio_result_t foo_getsensor(struct iio_dev *dev_info, int channel)
> {
> switch (channel) {
> case FOO_CHAN_TEMP0:
> return read_temp0(dev_info);
> case FOO_CHAN_TEMP1:
> return read_temp1(dev_info);
> case FOO_CHAN_ADC:
> return read_adc(dev_info);
> }
> return -EINVAL;
> }
>
>> * Whatever you pick also immediately becomes restrictive as the main
>> class has to be updated to handle any new elements.
>
> I believe that's a good thing, we want to limit the number
> of different interfaces that people come up with. If two
> drivers are similar enough, they should probably use the
> same one.
Limiting them is fine, but there are still an awful lot of
them - so we still end up with big and ugly tables. Again
currently enforcement of interface is done by review.
>
> It also makes it easier to verify that all attributes are
> documented properly, because the documentation only has
> to be matched with a single file, not with every single
> driver.
That would be a gain. However, the real thing you need
to check with use of an attribute is not it's naming
(which is a trivial match against the docs), but that
the units of whatever is returned are correct. Thus you
end up looking closely at the docs alongside every driver
anyway and trawling through datasheets.
>
> It also forces out-of-tree modules to use the same interfaces
> that other drivers are using. If someone has hardware for
> something new, it will only have an interface to talk to
> after it has been discussed and documented in mainline.
The question this brings up is how to do we then handle the one
of a kind interface elements? However general our spec
there are always things we don't really want in the core
as they will only ever be used by one specific driver.
Right now we operate a policy which says all elements must
be documented, but those that are not 'general' shouldn't go
in the main doc files. Things can move to general the moment
we find a second device using them. (sometimes we are wrong
in thinking something is so obscure no one else will ever
want it). Note these obscure elements don't get supported by
any remotely standard userspace code. Take the TOAS 258x driver
from Jobn Brenner for example. That has one nasty calibration
attribute. Try as we might we really haven't managed to
blugeon that one into a 'standard' form (it's the coefficients
of a magic transfer function which takes into account the exact
glass window over the sensor). Normally we'd argue this should
be platform data, but Jon has real users where devices that are
otherwise identical have different glass - hence it must be
runtime controlled.
The moment we put a route in for registering additional attributes
the out-of-tree argument is weakened.
>
>> * Note we'll have function points for all the scan element related stuff
>> as well. Basically the set that would be needed is huge.
>>
>> At the end of the day I'm unconvinced this makes sense from the point
>> of view of simplifying the driver code. The gain is that it enforces the abi
>> and may allow in kernel uses.
>> Ultimately we lifted this approach from hwmon where it works well
>> under similar circumstances.
>>
>> Alternative is a small set such as:
>>
>> /* access ops */
>> int (*get_channel_value)(struct iio_dev *info, enum CHANNEL);
>> char (*get_channel_offset)(struct iio_dev *info, enum CHANNEL);
>>
>> and a list of which channels exist. Then the driver side becomes a massive
>> switch statement rather than the attribute tables. Again, I'm not convinced
>> that is actually cleaner or easier to understand. Also this approach pretty
>> much means you end up with large numbers of equal attributes as you can't
>> trivially group them. e.g we have accel_scale to cover case where it's the
>> same for all accelerometer axes.
>
> The whole point would be to avoid duplication, so maybe something
> a little smarter is needed, but the current code duplicates the
> definition of attributes to every driver, and I'm sure we can
> find a solution that requires less code.
A little less perhaps, but it's still going to be a big table, just of
subtly different things.
>
> In many cases, you should be able to replace a long switch statement
> by a table driven approach inside of the driver.
>
I think the only way to answer this is to do a quick implementation of
this and see how well it works.
I'll have a crack at this sometime soon with a representative set of
drivers and see how it goes.
>>> * It's not clear to me why you need one additional device
>>> and one chardev for each interrupt line of a device, my
>>> feeling is that this could be simplified a lot if you find
>>> a way to represent an iio_dev with a single chardev to
>>> user space. This may have been an attempt to avoid ioctl()
>>> calls, but I think in this case an ioctl interface would
>>> be cleaner than a complex hierarchy of devices.
>>> Another alternative would be for the device driver to
>>> register multiple iio_devs in case it needs multiple
>>> interfaces.
>> In a sense this isn't quite as bad as it seems. The only cases we
>> have so far (and it's more or less all that's out there) are those where
>> the device has one interrupt line for 'events' - e.g threshold
>> interrupts of one type or another, and one for one for data ready
>> signals. The data ready signals are handled as triggers and hence
>> don't have a chardev (directly).
>
> If you only have the two, you might be able to handle them both
> in the poll function, where the regular case means POLLIN
> or POLLRDNORM, while the threshold interrupt gets turned
> into POLLPRI or POLLRDBAND.
>
> I'm not sure if that covers all cases you are interested in,
> but it sounds like a useful simplification. It also makes
> it possible for user programs to only wait for one type of
> event.
I will need to have a play and get back to you on this.
I don't suppose there is anything similar to this in kernel
I could take a look at? Its not an area I'm even remotely
familiar with. Right now I don't see exactly how this helps us
with the 'out of band' messages not colliding with the main data
flow.
>
>> The other uses of chrdevs are for accessing buffers. Each buffer may have
>> two of them. The first is a dirty read chrdev. The second is
>> for out of band flow information to userspace. Basically you
>> control what events will come out of the flow control chrdev.
>> Programs block on that and read from the data flow chrdev
>> according to what comes down the line. The messy events stuff
>> is to handle event escalation. Dealing with getting a 50% full
>> event and then a 75% full event with no inherent knowledge of
>> if you read anything in between is a pain. We could specify
>> only one level event exists, which would simplify the code
>> somewhat. Perhaps wise for an initial merge though I'm loath
>> to not support functionality that is fairly common in hardware
>> buffers.
>>
>> Some buffer implementations don't provide flow control (kfifo_buf)
>> so for them you just have a read chrdev (actually this isn't true
>> right now - I need to fix this). This chrdev is separate from
>> the 'event' chrdevs purely because for speed reasons. You don't
>> want to have what is coming down this line change except due
>> to software interaction (and to do that you normally have to stop
>> the buffer fill).
>
> Doesn't epoll handle the event escalation with its edge triggered
> mode? Basically you can get notified every time that there is
> more to be read.
We also need to know how much more this is to be read. So at the
moment, if a 50% full turns up and isn't picked up by userspace
before a 75% full event, the 50% is escalated to become a 75%
event which userspace then receives. To be honest I'm tempted,
for now at least to drop this functionality. It has few users
and makes the events system considerably more complex. We can
think about how / whether to introduce it later.
>
>>> * Neither of your two file operations supports poll(), only
>>> read. Having poll() support is essential if you want to
>>> wait for multiple devices in user space. Maybe it's
>>> possible to convert the code to use eventfd instead of
>>> rolling your own event interface?
>> I'll look into this one. Curiously I don't think it has
>> ever been suggested before. (maybe I'm just forgetful)
>>
>> We have had the suggestion of using inputs event interface and
>> I am still considering that option. The issue
>> there is that it is really far too heavyweight. So far we have
>> no use cases for multiple readers and we never want to aggregate
>> across devices. Also input views events as carrying data. All
>> actual readings in our case are going through the faster buffer
>> route.
>
> I'm slightly confused, mainly because I have no idea how the
> buffer and event interfaces are actually being used. I would
> have expected an interface as simple as:
>
> * One iio device per sensor that can provide data, possibly
> many per hardware device
One per device.
> * One chardev for each iio device
currently 1-3. (event line, buffer access, buffer event)
> * Use epoll to wait for data and/or out-of-band messages
> * Use chrdev read to get events from the buffer
and data?
> * Use sysfs to read from sensors that are not event based
>
> What you have is obviously more complex than this, and you
> probably have good reasons that I still need to understand.
>
>>> Also, there is only one driver providing such a function,
>>> so this is probably a good candidate for deferring to a later
>>> stage, along with all the the entire ring file code that seems
>>> rather pointless otherwise.
>> Lots of drivers do it. They are simply using one of the software
>> provided buffers. ring_sw or kfifo_buf (and yes they do have
>> useless names as well so we'll clean that up for which ever
>> ones we keep!)
>
> Ok, I see. I did not realize that iio_rip_sw_rb was getting assigned
> to ->rip_lots.
renamed to read_first_n which is a somewhat clearer I hope.
>
>>> * iio-trig-sysfs should not be a platform driver
>> This one got discussed before merging it and I agree. We let it
>> go then because there was a clear use case, a working driver and
>> no one had the time to do a better job. Michael eventually talked Greg
>> round to letting it in.
>>
>> http://marc.info/?t=129665398600002&r=1&w=2
>
> Ok. I still think that it is a really bad idea because the driver
> is essentially a software feature that could be use on any machine,
> but using a platform device ties it directly to a specific machine
> that registers the device. It would make much more sense to
> remove all the driver registration from there and just create the
> iio device at module load time, or to have some interface that
> can be used to dynamically create these triggers if the module
> is loaded.
Yup, the dynamic route to creation is what I favour. As you say
this needs fixing up before going out of staging. We definitely
need these for a bridge to input to implement polled input devices.
I'll put a todo in place for that element in staging so we don't
forget.
>
> Arnd
>
On Wed, Mar 16, 2011 at 10:50:22AM -0400, Jonathan Cameron wrote:
> On 03/16/11 13:33, Arnd Bergmann wrote:
> > On Wednesday 16 March 2011, Jonathan Cameron wrote:
> >> Hi Arnd,
> >>> I've started looking at the current code base in staging,
> >>> this is basically a log of what I'm finding there that
> >>> may get improved:
> >> Thanks for taking a look!
> >>>
> >>> * Your bus_type is not really a bus in the sense we normally
> >>> use it, because it does not have any driver/device matching.
> >>> You only register devices from the same files that drive
> >>> the hardware. This is normally done using a class, not
> >>> a bus.
> >> That's what I originally thought and indeed how it was
> >> initially done. The responses from Greg KH and Kay Sievers
> >> to one of our ABI discussions convinced me otherwise.
> >>
> >> http://lkml.org/lkml/2010/1/20/233 + the previous email in
> >> that thread.
> >
> > (adding Kay to Cc here)
> >
> > I see. However, I'd still disagree with Kay here. Given that
> > the common use of existing subsystems is to have "bus" for
> > things that are physically connected, and "class" for things
> > that are logical abstractions, I think we should continue
> > that way for new subsystems, even if there is little difference
> > technically.
> >
> > There is little point discussing how things should have been
> > done in the past when we're stuck with them. The best we
> > can do now is make the code as consistent as possible.
> Leaving this for Kay.
> >
> >>> * Since you specify the sysfs attributes globally in the
> >>> documentation, it would be nice if drivers did not have
> >>> to implement them as attribute show function, but as
> >>> a function that simply returns a value that is then
> >>> printed by common code. You can do this using a structure
> >>> of function pointers that each driver fills with the
> >>> relevant ones, like file_operations, replacing the
> >>> attribute groups.
> >> This may indeed work. However it would be a fair bit
> >> more fiddly than that a simple function pointer structure
> >> as the sets of attributes supplied by different devices
> >> vary considerably.
> I've cc'd Jean and Guenter as they may be able to offer so
> insight from what they see in hwmon (sorry for dropping you
> in half way through a discussion!)
My thinking is to have only two function pointers (read/write) and index
the actual attribute with a parameter. This would keep the API (much) smaller.
Guenter
> >
> > It's certainly more work for the subsystem, but the intention
> > is to simplify the individual drivers, to make things scale
> > better as many more drivers get added.
> I'm yet to be convinced this simplifies things. We basically move
> from one form of big table in the drivers to another and add complexity
> to the core. I'd be fine with that complexity if it actually simplified
> the drivers, but I'm not sure it does. Perhaps the only way to really
> answer that is to actually try doing it with a few drivers and see
> where we end up.
> >
> >> So to confirm I understand you correctly we either have something like:
> >> /* access ops */
> >>
> >> int (*get_accel_x)(struct iio_dev *devinfo)
> >> char *(*get_accel_offset)(struct iio_dev *devinfo); /* note these can't return a value as
> >> they may well be floating point */
> >> char *(*get_accel_gain)(struct iio_dev *devinfo)
> >> int (*get_accel_y)(struct iio_dev *devinfo)
> >>
> >> Sticky cases that will make this fiddly.
> >
> > You can't really return a char* because of the memory allocation,
> > but aside from that, the idea is right, yes.
> sure. It would need a bit of management code around that or
> some suitable encapsulation.
> >
> > Note that this forces all drivers to use the same unit
> > for data they return in the attributes, but I consider
> > this a good thing.
> True, though the other approach to enforcing this is what hwmon
> does. Thorough review against the spec for all new drivers.
> >
> > For attributes that really want to return a floating point
> > number, I think we should either define a fixed-point
> > form with a constant exponent, or some other representation
> > that is allowed in the kernel.
> >
> >> * Unknown maximum number of some types of attributes.
> >> - Could use a
> >> int (*get_in)(struct iio_dev dev_info, int channel);
> >> int num_in;
> >
> > Good point. So a driver may have multiple attributes
> > of the same type, which need to get enumerated, but should
> > all behave the same way, right?
> >
> > Your suggestion seems good, but I'd still recommend making
> > it so that the core chooses the attribute names, not the
> > device driver. One possible way to lay this out is something
> > like:
> >
> > /* global defines */
> > enum iio_channel_type {
> > IIO_CHAN_TEMPERATURE,
> > IIO_CHAN_ADC,
> > IIO_CHAN_FOO,
> > ...
> > };
> >
> > static const char *channel_names[] = {
> > [IIO_CHAN_TEMPERATURE] = "temperature%d",
> > [IIO_CHAN_ADC] = "adc%d",
> > [IIO_CHAN_FOO] = "foo%d",
> > ...
> > };
> >
> > typedef unsigned long long iio_result_t;
> >
> > struct iio_channel_defs {
> > iio_result_t (*get)(struct iio_dev *dev_info, int channel);
> >
> > enum iio_channel_type channels[0];
> > };
> >
> > /* example driver */
> > enum {
> > FOO_CHAN_TEMP0,
> > FOO_CHAN_TEMP1,
> > FOO_CHAN_ADC,
> > };
> >
> > struct iio_channel_defs foo_driver_channels = {
> > .get = &foo_get_sensor,
> >
> > .channels = {
> > /* this device has two temperature and one adc input */
> > [FOO_CHAN_TEMP0] = IIO_CHAN_TEMPERATURE,
> > [FOO_CHAN_TEMP1] = IIO_CHAN_TEMPERATURE,
> > [FOO_CHAN_ADC] = IIO_CHAN_ADC,
> > },
> > };
> >
> > iio_result_t foo_getsensor(struct iio_dev *dev_info, int channel)
> > {
> > switch (channel) {
> > case FOO_CHAN_TEMP0:
> > return read_temp0(dev_info);
> > case FOO_CHAN_TEMP1:
> > return read_temp1(dev_info);
> > case FOO_CHAN_ADC:
> > return read_adc(dev_info);
> > }
> > return -EINVAL;
> > }
> >
> >> * Whatever you pick also immediately becomes restrictive as the main
> >> class has to be updated to handle any new elements.
> >
> > I believe that's a good thing, we want to limit the number
> > of different interfaces that people come up with. If two
> > drivers are similar enough, they should probably use the
> > same one.
> Limiting them is fine, but there are still an awful lot of
> them - so we still end up with big and ugly tables. Again
> currently enforcement of interface is done by review.
> >
> > It also makes it easier to verify that all attributes are
> > documented properly, because the documentation only has
> > to be matched with a single file, not with every single
> > driver.
> That would be a gain. However, the real thing you need
> to check with use of an attribute is not it's naming
> (which is a trivial match against the docs), but that
> the units of whatever is returned are correct. Thus you
> end up looking closely at the docs alongside every driver
> anyway and trawling through datasheets.
> >
> > It also forces out-of-tree modules to use the same interfaces
> > that other drivers are using. If someone has hardware for
> > something new, it will only have an interface to talk to
> > after it has been discussed and documented in mainline.
>
> The question this brings up is how to do we then handle the one
> of a kind interface elements? However general our spec
> there are always things we don't really want in the core
> as they will only ever be used by one specific driver.
>
> Right now we operate a policy which says all elements must
> be documented, but those that are not 'general' shouldn't go
> in the main doc files. Things can move to general the moment
> we find a second device using them. (sometimes we are wrong
> in thinking something is so obscure no one else will ever
> want it). Note these obscure elements don't get supported by
> any remotely standard userspace code. Take the TOAS 258x driver
> from Jobn Brenner for example. That has one nasty calibration
> attribute. Try as we might we really haven't managed to
> blugeon that one into a 'standard' form (it's the coefficients
> of a magic transfer function which takes into account the exact
> glass window over the sensor). Normally we'd argue this should
> be platform data, but Jon has real users where devices that are
> otherwise identical have different glass - hence it must be
> runtime controlled.
>
> The moment we put a route in for registering additional attributes
> the out-of-tree argument is weakened.
> >
> >> * Note we'll have function points for all the scan element related stuff
> >> as well. Basically the set that would be needed is huge.
> >>
> >> At the end of the day I'm unconvinced this makes sense from the point
> >> of view of simplifying the driver code. The gain is that it enforces the abi
> >> and may allow in kernel uses.
> >> Ultimately we lifted this approach from hwmon where it works well
> >> under similar circumstances.
> >>
> >> Alternative is a small set such as:
> >>
> >> /* access ops */
> >> int (*get_channel_value)(struct iio_dev *info, enum CHANNEL);
> >> char (*get_channel_offset)(struct iio_dev *info, enum CHANNEL);
> >>
> >> and a list of which channels exist. Then the driver side becomes a massive
> >> switch statement rather than the attribute tables. Again, I'm not convinced
> >> that is actually cleaner or easier to understand. Also this approach pretty
> >> much means you end up with large numbers of equal attributes as you can't
> >> trivially group them. e.g we have accel_scale to cover case where it's the
> >> same for all accelerometer axes.
> >
> > The whole point would be to avoid duplication, so maybe something
> > a little smarter is needed, but the current code duplicates the
> > definition of attributes to every driver, and I'm sure we can
> > find a solution that requires less code.
> A little less perhaps, but it's still going to be a big table, just of
> subtly different things.
> >
> > In many cases, you should be able to replace a long switch statement
> > by a table driven approach inside of the driver.
> >
> I think the only way to answer this is to do a quick implementation of
> this and see how well it works.
>
> I'll have a crack at this sometime soon with a representative set of
> drivers and see how it goes.
>
> >>> * It's not clear to me why you need one additional device
> >>> and one chardev for each interrupt line of a device, my
> >>> feeling is that this could be simplified a lot if you find
> >>> a way to represent an iio_dev with a single chardev to
> >>> user space. This may have been an attempt to avoid ioctl()
> >>> calls, but I think in this case an ioctl interface would
> >>> be cleaner than a complex hierarchy of devices.
> >>> Another alternative would be for the device driver to
> >>> register multiple iio_devs in case it needs multiple
> >>> interfaces.
> >> In a sense this isn't quite as bad as it seems. The only cases we
> >> have so far (and it's more or less all that's out there) are those where
> >> the device has one interrupt line for 'events' - e.g threshold
> >> interrupts of one type or another, and one for one for data ready
> >> signals. The data ready signals are handled as triggers and hence
> >> don't have a chardev (directly).
> >
> > If you only have the two, you might be able to handle them both
> > in the poll function, where the regular case means POLLIN
> > or POLLRDNORM, while the threshold interrupt gets turned
> > into POLLPRI or POLLRDBAND.
> >
> > I'm not sure if that covers all cases you are interested in,
> > but it sounds like a useful simplification. It also makes
> > it possible for user programs to only wait for one type of
> > event.
> I will need to have a play and get back to you on this.
>
> I don't suppose there is anything similar to this in kernel
> I could take a look at? Its not an area I'm even remotely
> familiar with. Right now I don't see exactly how this helps us
> with the 'out of band' messages not colliding with the main data
> flow.
> >
> >> The other uses of chrdevs are for accessing buffers. Each buffer may have
> >> two of them. The first is a dirty read chrdev. The second is
> >> for out of band flow information to userspace. Basically you
> >> control what events will come out of the flow control chrdev.
> >> Programs block on that and read from the data flow chrdev
> >> according to what comes down the line. The messy events stuff
> >> is to handle event escalation. Dealing with getting a 50% full
> >> event and then a 75% full event with no inherent knowledge of
> >> if you read anything in between is a pain. We could specify
> >> only one level event exists, which would simplify the code
> >> somewhat. Perhaps wise for an initial merge though I'm loath
> >> to not support functionality that is fairly common in hardware
> >> buffers.
> >>
> >> Some buffer implementations don't provide flow control (kfifo_buf)
> >> so for them you just have a read chrdev (actually this isn't true
> >> right now - I need to fix this). This chrdev is separate from
> >> the 'event' chrdevs purely because for speed reasons. You don't
> >> want to have what is coming down this line change except due
> >> to software interaction (and to do that you normally have to stop
> >> the buffer fill).
> >
> > Doesn't epoll handle the event escalation with its edge triggered
> > mode? Basically you can get notified every time that there is
> > more to be read.
> We also need to know how much more this is to be read. So at the
> moment, if a 50% full turns up and isn't picked up by userspace
> before a 75% full event, the 50% is escalated to become a 75%
> event which userspace then receives. To be honest I'm tempted,
> for now at least to drop this functionality. It has few users
> and makes the events system considerably more complex. We can
> think about how / whether to introduce it later.
> >
> >>> * Neither of your two file operations supports poll(), only
> >>> read. Having poll() support is essential if you want to
> >>> wait for multiple devices in user space. Maybe it's
> >>> possible to convert the code to use eventfd instead of
> >>> rolling your own event interface?
> >> I'll look into this one. Curiously I don't think it has
> >> ever been suggested before. (maybe I'm just forgetful)
> >>
> >> We have had the suggestion of using inputs event interface and
> >> I am still considering that option. The issue
> >> there is that it is really far too heavyweight. So far we have
> >> no use cases for multiple readers and we never want to aggregate
> >> across devices. Also input views events as carrying data. All
> >> actual readings in our case are going through the faster buffer
> >> route.
> >
> > I'm slightly confused, mainly because I have no idea how the
> > buffer and event interfaces are actually being used. I would
> > have expected an interface as simple as:
> >
> > * One iio device per sensor that can provide data, possibly
> > many per hardware device
> One per device.
> > * One chardev for each iio device
> currently 1-3. (event line, buffer access, buffer event)
> > * Use epoll to wait for data and/or out-of-band messages
> > * Use chrdev read to get events from the buffer
> and data?
> > * Use sysfs to read from sensors that are not event based
> >
> > What you have is obviously more complex than this, and you
> > probably have good reasons that I still need to understand.
> >
> >>> Also, there is only one driver providing such a function,
> >>> so this is probably a good candidate for deferring to a later
> >>> stage, along with all the the entire ring file code that seems
> >>> rather pointless otherwise.
> >> Lots of drivers do it. They are simply using one of the software
> >> provided buffers. ring_sw or kfifo_buf (and yes they do have
> >> useless names as well so we'll clean that up for which ever
> >> ones we keep!)
> >
> > Ok, I see. I did not realize that iio_rip_sw_rb was getting assigned
> > to ->rip_lots.
> renamed to read_first_n which is a somewhat clearer I hope.
> >
> >>> * iio-trig-sysfs should not be a platform driver
> >> This one got discussed before merging it and I agree. We let it
> >> go then because there was a clear use case, a working driver and
> >> no one had the time to do a better job. Michael eventually talked Greg
> >> round to letting it in.
> >>
> >> http://marc.info/?t=129665398600002&r=1&w=2
> >
> > Ok. I still think that it is a really bad idea because the driver
> > is essentially a software feature that could be use on any machine,
> > but using a platform device ties it directly to a specific machine
> > that registers the device. It would make much more sense to
> > remove all the driver registration from there and just create the
> > iio device at module load time, or to have some interface that
> > can be used to dynamically create these triggers if the module
> > is loaded.
> Yup, the dynamic route to creation is what I favour. As you say
> this needs fixing up before going out of staging. We definitely
> need these for a bridge to input to implement polled input devices.
>
> I'll put a todo in place for that element in staging so we don't
> forget.
> >
> > Arnd
> >
>
On Wednesday 16 March 2011, Jonathan Cameron wrote:
> On 03/16/11 13:33, Arnd Bergmann wrote:
> > On Wednesday 16 March 2011, Jonathan Cameron wrote:
> >
> > It's certainly more work for the subsystem, but the intention
> > is to simplify the individual drivers, to make things scale
> > better as many more drivers get added.
>
> I'm yet to be convinced this simplifies things. We basically move
> from one form of big table in the drivers to another and add complexity
> to the core. I'd be fine with that complexity if it actually simplified
> the drivers, but I'm not sure it does. Perhaps the only way to really
> answer that is to actually try doing it with a few drivers and see
> where we end up.
Yes. I'm pretty sure it's possible to do better than the current
interface, but don't have a way to prove it.
> >> * Whatever you pick also immediately becomes restrictive as the main
> >> class has to be updated to handle any new elements.
> >
> > I believe that's a good thing, we want to limit the number
> > of different interfaces that people come up with. If two
> > drivers are similar enough, they should probably use the
> > same one.
>
> Limiting them is fine, but there are still an awful lot of
> them - so we still end up with big and ugly tables.
So we should find a way to make it a nice and small table
instead ;-)
> Again currently enforcement of interface is done by review.
I believe enforcing the rules using the compiler is always
better than by review, if all other factors are the same.
> > It also makes it easier to verify that all attributes are
> > documented properly, because the documentation only has
> > to be matched with a single file, not with every single
> > driver.
> That would be a gain. However, the real thing you need
> to check with use of an attribute is not it's naming
> (which is a trivial match against the docs), but that
> the units of whatever is returned are correct. Thus you
> end up looking closely at the docs alongside every driver
> anyway and trawling through datasheets.
Are you currently printing the unit as part of the
attribute output? If that gets handled by the core,
any driver that tries to use something else would
simply be considered a bug.
> > It also forces out-of-tree modules to use the same interfaces
> > that other drivers are using. If someone has hardware for
> > something new, it will only have an interface to talk to
> > after it has been discussed and documented in mainline.
>
> The question this brings up is how to do we then handle the one
> of a kind interface elements? However general our spec
> there are always things we don't really want in the core
> as they will only ever be used by one specific driver.
>
> Right now we operate a policy which says all elements must
> be documented, but those that are not 'general' shouldn't go
> in the main doc files. Things can move to general the moment
> we find a second device using them. (sometimes we are wrong
> in thinking something is so obscure no one else will ever
> want it). Note these obscure elements don't get supported by
> any remotely standard userspace code. Take the TOAS 258x driver
> from Jobn Brenner for example. That has one nasty calibration
> attribute. Try as we might we really haven't managed to
> blugeon that one into a 'standard' form (it's the coefficients
> of a magic transfer function which takes into account the exact
> glass window over the sensor). Normally we'd argue this should
> be platform data, but Jon has real users where devices that are
> otherwise identical have different glass - hence it must be
> runtime controlled.
>
> The moment we put a route in for registering additional attributes
> the out-of-tree argument is weakened.
Would it help to have multiple types of interfaces (I suppose
you already do), each with their own table?
This way you could enforce that all drivers of one type use
exactly the same specification, but also make it possible
to have new types, even those that have only one driver by
design.
In the case of the TOAS 258x driver (I can't find that in the
tree), I fear that would still have to become a standard attribute
of some sort.
> >
> > The whole point would be to avoid duplication, so maybe something
> > a little smarter is needed, but the current code duplicates the
> > definition of attributes to every driver, and I'm sure we can
> > find a solution that requires less code.
> A little less perhaps, but it's still going to be a big table, just of
> subtly different things.
> >
> > In many cases, you should be able to replace a long switch statement
> > by a table driven approach inside of the driver.
> >
> I think the only way to answer this is to do a quick implementation of
> this and see how well it works.
>
> I'll have a crack at this sometime soon with a representative set of
> drivers and see how it goes.
Ok, cool! Just send out the drivers as you get there, and maybe someone
has ideas to simplify them further if you're not happy with the
result.
> >
> > If you only have the two, you might be able to handle them both
> > in the poll function, where the regular case means POLLIN
> > or POLLRDNORM, while the threshold interrupt gets turned
> > into POLLPRI or POLLRDBAND.
> >
> > I'm not sure if that covers all cases you are interested in,
> > but it sounds like a useful simplification. It also makes
> > it possible for user programs to only wait for one type of
> > event.
> I will need to have a play and get back to you on this.
>
> I don't suppose there is anything similar to this in kernel
> I could take a look at? Its not an area I'm even remotely
> familiar with. Right now I don't see exactly how this helps us
> with the 'out of band' messages not colliding with the main data
> flow.
One way to deal with out of band data (if it's more than just
a flag) is to have an ioctl function that returns structured
data, while the regular read function returns the buffered
input stream.
> > Doesn't epoll handle the event escalation with its edge triggered
> > mode? Basically you can get notified every time that there is
> > more to be read.
> We also need to know how much more this is to be read. So at the
> moment, if a 50% full turns up and isn't picked up by userspace
> before a 75% full event, the 50% is escalated to become a 75%
> event which userspace then receives. To be honest I'm tempted,
> for now at least to drop this functionality. It has few users
> and makes the events system considerably more complex. We can
> think about how / whether to introduce it later.
Yes, good point. I also don't get the use case of this. Why would
user space not just always read all data that is available?
> Yup, the dynamic route to creation is what I favour. As you say
> this needs fixing up before going out of staging. We definitely
> need these for a bridge to input to implement polled input devices.
>
> I'll put a todo in place for that element in staging so we don't
> forget.
ok
Arnd
On 03/16/11 15:15, Arnd Bergmann wrote:
> On Wednesday 16 March 2011, Jonathan Cameron wrote:
>> On 03/16/11 13:33, Arnd Bergmann wrote:
>>> On Wednesday 16 March 2011, Jonathan Cameron wrote:
>>>
>>> It's certainly more work for the subsystem, but the intention
>>> is to simplify the individual drivers, to make things scale
>>> better as many more drivers get added.
>>
>> I'm yet to be convinced this simplifies things. We basically move
>> from one form of big table in the drivers to another and add complexity
>> to the core. I'd be fine with that complexity if it actually simplified
>> the drivers, but I'm not sure it does. Perhaps the only way to really
>> answer that is to actually try doing it with a few drivers and see
>> where we end up.
>
> Yes. I'm pretty sure it's possible to do better than the current
> interface, but don't have a way to prove it.
>
>>>> * Whatever you pick also immediately becomes restrictive as the main
>>>> class has to be updated to handle any new elements.
>>>
>>> I believe that's a good thing, we want to limit the number
>>> of different interfaces that people come up with. If two
>>> drivers are similar enough, they should probably use the
>>> same one.
>>
>> Limiting them is fine, but there are still an awful lot of
>> them - so we still end up with big and ugly tables.
>
> So we should find a way to make it a nice and small table
> instead ;-)
>
>> Again currently enforcement of interface is done by review.
>
> I believe enforcing the rules using the compiler is always
> better than by review, if all other factors are the same.
>
>>> It also makes it easier to verify that all attributes are
>>> documented properly, because the documentation only has
>>> to be matched with a single file, not with every single
>>> driver.
>> That would be a gain. However, the real thing you need
>> to check with use of an attribute is not it's naming
>> (which is a trivial match against the docs), but that
>> the units of whatever is returned are correct. Thus you
>> end up looking closely at the docs alongside every driver
>> anyway and trawling through datasheets.
>
> Are you currently printing the unit as part of the
> attribute output? If that gets handled by the core,
> any driver that tries to use something else would
> simply be considered a bug.
Nope. It's done set by the Docs. This we need to keep for compatibility
(where relevant) with hwmon. It's a bug now if it doesn't meet
the spec but you'd be amazed how often these are wrong. Sometimes
it is a nightmare figuring out from the datasheet what they actually
are!
>
>>> It also forces out-of-tree modules to use the same interfaces
>>> that other drivers are using. If someone has hardware for
>>> something new, it will only have an interface to talk to
>>> after it has been discussed and documented in mainline.
>>
>> The question this brings up is how to do we then handle the one
>> of a kind interface elements? However general our spec
>> there are always things we don't really want in the core
>> as they will only ever be used by one specific driver.
>>
>> Right now we operate a policy which says all elements must
>> be documented, but those that are not 'general' shouldn't go
>> in the main doc files. Things can move to general the moment
>> we find a second device using them. (sometimes we are wrong
>> in thinking something is so obscure no one else will ever
>> want it). Note these obscure elements don't get supported by
>> any remotely standard userspace code. Take the TOAS 258x driver
>> from Jobn Brenner for example. That has one nasty calibration
>> attribute. Try as we might we really haven't managed to
>> blugeon that one into a 'standard' form (it's the coefficients
>> of a magic transfer function which takes into account the exact
>> glass window over the sensor). Normally we'd argue this should
>> be platform data, but Jon has real users where devices that are
>> otherwise identical have different glass - hence it must be
>> runtime controlled.
>>
>> The moment we put a route in for registering additional attributes
>> the out-of-tree argument is weakened.
>
> Would it help to have multiple types of interfaces (I suppose
> you already do), each with their own table?
>
> This way you could enforce that all drivers of one type use
> exactly the same specification, but also make it possible
> to have new types, even those that have only one driver by
> design.
Sadly there are no clean definitions of types. You'd be amazed what
downright weird sets end up in a given device.
Might be some way of breaking it down into a set of tables though
in a vaguely coherent way. Each device would then have one or more
of these for which it would define one or more of the functions.
>
> In the case of the TOAS 258x driver (I can't find that in the
> tree), I fear that would still have to become a standard attribute
> of some sort.
Under review at the mo.
http://www.spinics.net/lists/linux-iio/msg01295.html
(I think that's the latest version - john forgot to give them numbers).
>
>>>
>>> The whole point would be to avoid duplication, so maybe something
>>> a little smarter is needed, but the current code duplicates the
>>> definition of attributes to every driver, and I'm sure we can
>>> find a solution that requires less code.
>> A little less perhaps, but it's still going to be a big table, just of
>> subtly different things.
>>>
>>> In many cases, you should be able to replace a long switch statement
>>> by a table driven approach inside of the driver.
>>>
>> I think the only way to answer this is to do a quick implementation of
>> this and see how well it works.
>>
>> I'll have a crack at this sometime soon with a representative set of
>> drivers and see how it goes.
>
> Ok, cool! Just send out the drivers as you get there, and maybe someone
> has ideas to simplify them further if you're not happy with the
> result.
>
>>>
>>> If you only have the two, you might be able to handle them both
>>> in the poll function, where the regular case means POLLIN
>>> or POLLRDNORM, while the threshold interrupt gets turned
>>> into POLLPRI or POLLRDBAND.
>>>
>>> I'm not sure if that covers all cases you are interested in,
>>> but it sounds like a useful simplification. It also makes
>>> it possible for user programs to only wait for one type of
>>> event.
>> I will need to have a play and get back to you on this.
>>
>> I don't suppose there is anything similar to this in kernel
>> I could take a look at? Its not an area I'm even remotely
>> familiar with. Right now I don't see exactly how this helps us
>> with the 'out of band' messages not colliding with the main data
>> flow.
>
> One way to deal with out of band data (if it's more than just
> a flag)
It is effectively a flag, but there are lots of them.
> is to have an ioctl function that returns structured
> data, while the regular read function returns the buffered
> input stream.
That would work, but it is it really worth it to kill off the chrdev?
Note we still need most of the underlying structure just to keep the
various attributes well categorized.
>
>>> Doesn't epoll handle the event escalation with its edge triggered
>>> mode? Basically you can get notified every time that there is
>>> more to be read.
>> We also need to know how much more this is to be read. So at the
>> moment, if a 50% full turns up and isn't picked up by userspace
>> before a 75% full event, the 50% is escalated to become a 75%
>> event which userspace then receives. To be honest I'm tempted,
>> for now at least to drop this functionality. It has few users
>> and makes the events system considerably more complex. We can
>> think about how / whether to introduce it later.
>
> Yes, good point. I also don't get the use case of this. Why would
> user space not just always read all data that is available?
Good point. We could just move the handling of these events into the
actual driver (this is only ever relevant for hardware buffers). Thus
it could keep track of what it knows is there. Note these devices don't
always provide any other way of knowing and yes you can read more data
from them than is actually there. Whether you can tell this has happened
is very much device dependent.
>
>> Yup, the dynamic route to creation is what I favour. As you say
>> this needs fixing up before going out of staging. We definitely
>> need these for a bridge to input to implement polled input devices.
>>
>> I'll put a todo in place for that element in staging so we don't
>> forget.
>
Note I have quite a few changes currently queued up so might be a little while
before I get to trying out the more complex stuff you have suggested.
Jonathan
On 03/16/11 11:57, Jonathan Cameron wrote:
> Hi Arnd,
>> I've started looking at the current code base in staging,
>> this is basically a log of what I'm finding there that
>> may get improved:
> Thanks for taking a look!
>>
>> * Your bus_type is not really a bus in the sense we normally
>> use it, because it does not have any driver/device matching.
>> You only register devices from the same files that drive
>> the hardware. This is normally done using a class, not
>> a bus.
> That's what I originally thought and indeed how it was
> initially done. The responses from Greg KH and Kay Sievers
> to one of our ABI discussions convinced me otherwise.
>
> http://lkml.org/lkml/2010/1/20/233 + the previous email in
> that thread.
>
>>
>> * Since you specify the sysfs attributes globally in the
>> documentation, it would be nice if drivers did not have
>> to implement them as attribute show function, but as
>> a function that simply returns a value that is then
>> printed by common code. You can do this using a structure
>> of function pointers that each driver fills with the
>> relevant ones, like file_operations, replacing the
>> attribute groups.
> This may indeed work. However it would be a fair bit
> more fiddly than that a simple function pointer structure
> as the sets of attributes supplied by different devices
> vary considerably.
>
> So to confirm I understand you correctly we either have something like:
> /* access ops */
>
> int (*get_accel_x)(struct iio_dev *devinfo)
> char *(*get_accel_offset)(struct iio_dev *devinfo); /* note these can't return a value as
> they may well be floating point */
> char *(*get_accel_gain)(struct iio_dev *devinfo)
> int (*get_accel_y)(struct iio_dev *devinfo)
>
> Sticky cases that will make this fiddly.
>
> * Unknown maximum number of some types of attributes.
> - Could use a
> int (*get_in)(struct iio_dev dev_info, int channel);
> int num_in;
> * Whatever you pick also immediately becomes restrictive as the main
> class has to be updated to handle any new elements.
> * Note we'll have function points for all the scan element related stuff
> as well. Basically the set that would be needed is huge.
>
> At the end of the day I'm unconvinced this makes sense from the point
> of view of simplifying the driver code. The gain is that it enforces the abi
> and may allow in kernel uses.
> Ultimately we lifted this approach from hwmon where it works well
> under similar circumstances.
>
> Alternative is a small set such as:
>
> /* access ops */
> int (*get_channel_value)(struct iio_dev *info, enum CHANNEL);
> char (*get_channel_offset)(struct iio_dev *info, enum CHANNEL);
>
> and a list of which channels exist. Then the driver side becomes a massive
> switch statement rather than the attribute tables. Again, I'm not convinced
> that is actually cleaner or easier to understand. Also this approach pretty
> much means you end up with large numbers of equal attributes as you can't
> trivially group them. e.g we have accel_scale to cover case where it's the
> same for all accelerometer axes.
>>
>> * iio_allocate_device() could get a size argument and
>> allocate the dev_data together with the iio_dev in
>> a single allocation, like netdev_alloc does. In addition,
>> you can pass a structure with all constant data, such
>> as THIS_MODULE, and the operations and/or attribute groups,
>> instead of having to set them individually for each dev.
> Good idea.
There is an issue here I'd forgotten. Some of our structures have a
elements which are cacheline aligned. For first version of this I'll
set our alignment requirements as L1_CACHE_BYTES. Would be nice to be
able to relax that down the line though as it means we use at least two
cachelines even if we don't care about that alignment requirement.
On Wednesday 16 March 2011 17:54:55 Jonathan Cameron wrote:
> >> * iio_allocate_device() could get a size argument and
> >> allocate the dev_data together with the iio_dev in
> >> a single allocation, like netdev_alloc does. In addition,
> >> you can pass a structure with all constant data, such
> >> as THIS_MODULE, and the operations and/or attribute groups,
> >> instead of having to set them individually for each dev.
> > Good idea.
>
> There is an issue here I'd forgotten. Some of our structures have a
> elements which are cacheline aligned. For first version of this I'll
> set our alignment requirements as L1_CACHE_BYTES. Would be nice to be
> able to relax that down the line though as it means we use at least two
> cachelines even if we don't care about that alignment requirement.
I think the data used by a device structure is not really an issue,
especially extending it by just another cache line.
Arnd
On Wednesday 16 March 2011, Jonathan Cameron wrote:
> On 03/16/11 15:15, Arnd Bergmann wrote:
> > Are you currently printing the unit as part of the
> > attribute output? If that gets handled by the core,
> > any driver that tries to use something else would
> > simply be considered a bug.
> Nope. It's done set by the Docs. This we need to keep for compatibility
> (where relevant) with hwmon. It's a bug now if it doesn't meet
> the spec but you'd be amazed how often these are wrong. Sometimes
> it is a nightmare figuring out from the datasheet what they actually
> are!
Ok.
> >>
> >> The moment we put a route in for registering additional attributes
> >> the out-of-tree argument is weakened.
> >
> > Would it help to have multiple types of interfaces (I suppose
> > you already do), each with their own table?
> >
> > This way you could enforce that all drivers of one type use
> > exactly the same specification, but also make it possible
> > to have new types, even those that have only one driver by
> > design.
>
> Sadly there are no clean definitions of types. You'd be amazed what
> downright weird sets end up in a given device.
> Might be some way of breaking it down into a set of tables though
> in a vaguely coherent way. Each device would then have one or more
> of these for which it would define one or more of the functions.
Right, or you could mandate that each device has exactly one of these,
but that you could have multiple iio devices in a single hardware
device.
> > is to have an ioctl function that returns structured
> > data, while the regular read function returns the buffered
> > input stream.
>
> That would work, but it is it really worth it to kill off the chrdev?
I don't know.
> Note we still need most of the underlying structure just to keep the
> various attributes well categorized.
The hardest part of interface design is always to find the simplest
possible way that can cover all the corner cases. I'm trying to
come up with possible simplifications, but I don't know all the
requirements that get in the way.
> >>> Doesn't epoll handle the event escalation with its edge triggered
> >>> mode? Basically you can get notified every time that there is
> >>> more to be read.
> >> We also need to know how much more this is to be read. So at the
> >> moment, if a 50% full turns up and isn't picked up by userspace
> >> before a 75% full event, the 50% is escalated to become a 75%
> >> event which userspace then receives. To be honest I'm tempted,
> >> for now at least to drop this functionality. It has few users
> >> and makes the events system considerably more complex. We can
> >> think about how / whether to introduce it later.
> >
> > Yes, good point. I also don't get the use case of this. Why would
> > user space not just always read all data that is available?
> Good point. We could just move the handling of these events into the
> actual driver (this is only ever relevant for hardware buffers). Thus
> it could keep track of what it knows is there. Note these devices don't
> always provide any other way of knowing and yes you can read more data
> from them than is actually there. Whether you can tell this has happened
> is very much device dependent.
Can you give an example? I don't understand what you mean with "can read
mode data [...] than is actually there".
> >> Yup, the dynamic route to creation is what I favour. As you say
> >> this needs fixing up before going out of staging. We definitely
> >> need these for a bridge to input to implement polled input devices.
> >>
> >> I'll put a todo in place for that element in staging so we don't
> >> forget.
> >
> Note I have quite a few changes currently queued up so might be a little while
> before I get to trying out the more complex stuff you have suggested.
Ok, no problem. Just take your time.
Arnd
I just went back to the older email and noticed that I missed some of your
important replies.
On Wednesday 16 March 2011, Jonathan Cameron wrote:
> > I'm slightly confused, mainly because I have no idea how the
> > buffer and event interfaces are actually being used. I would
> > have expected an interface as simple as:
> >
> > * One iio device per sensor that can provide data, possibly
> > many per hardware device
>
> One per device.
I'm not sure what that means. One iio device per hardware device?
What about hardware devices that have multiple unrelated streams
of buffered input data?
> > * One chardev for each iio device
>
> currently 1-3. (event line, buffer access, buffer event)
It would be really nice to unify this, as I said. What
are the reasons why you think it cannot or should not be
done?
> > * Use epoll to wait for data and/or out-of-band messages
> > * Use chrdev read to get events from the buffer
>
> and data?
I mean get the data associated with the event. The event
itself as you said does not have any data, so we would not
need to read it, just to use poll()/epoll() in order to
wait for it.
> > * Use sysfs to read from sensors that are not event based
> >
> > What you have is obviously more complex than this, and you
> > probably have good reasons that I still need to understand.
Arnd
On 03/17/11 13:47, Arnd Bergmann wrote:
> I just went back to the older email and noticed that I missed some of your
> important replies.
>
> On Wednesday 16 March 2011, Jonathan Cameron wrote:
>>> I'm slightly confused, mainly because I have no idea how the
>>> buffer and event interfaces are actually being used. I would
>>> have expected an interface as simple as:
>>>
>>> * One iio device per sensor that can provide data, possibly
>>> many per hardware device
>>
>> One per device.
>
> I'm not sure what that means. One iio device per hardware device?
Oops. That was particularly unclear!
>
> What about hardware devices that have multiple unrelated streams
> of buffered input data?
Certainly plausible, but so far the only ones I've seen that actually
do this are really just two bits of silicon in the same plastic
package. They tend to use different i2c addresses or spi chip
selects anyway so as far as the kernel is concerned are completely
separate. You are correct that any device which truly has different
streams of data would indeed need more than one device.
>
>>> * One chardev for each iio device
>>
>> currently 1-3. (event line, buffer access, buffer event)
>
> It would be really nice to unify this, as I said. What
> are the reasons why you think it cannot or should not be
> done?
Simplicity perhaps, but I'll definitely give your suggestions
a go and see where we end up.
>
>>> * Use epoll to wait for data and/or out-of-band messages
>>> * Use chrdev read to get events from the buffer
>>
>> and data?
>
> I mean get the data associated with the event. The event
> itself as you said does not have any data, so we would not
> need to read it, just to use poll()/epoll() in order to
> wait for it.
Sure. But devices can do a heck of a lot of different events.
(certainly 10's or maybe more). I'm not immediately clear
on how to handle this via poll etc. This is probably just
because I've never tried though!
>
>>> * Use sysfs to read from sensors that are not event based
>>>
>>> What you have is obviously more complex than this, and you
>>> probably have good reasons that I still need to understand.
>
> Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Thursday 17 March 2011, Jonathan Cameron wrote:
> On 03/17/11 13:47, Arnd Bergmann wrote:
> > What about hardware devices that have multiple unrelated streams
> > of buffered input data?
>
> Certainly plausible, but so far the only ones I've seen that actually
> do this are really just two bits of silicon in the same plastic
> package. They tend to use different i2c addresses or spi chip
> selects anyway so as far as the kernel is concerned are completely
> separate. You are correct that any device which truly has different
> streams of data would indeed need more than one device.
Ok.
> >>> * One chardev for each iio device
> >>
> >> currently 1-3. (event line, buffer access, buffer event)
> >
> > It would be really nice to unify this, as I said. What
> > are the reasons why you think it cannot or should not be
> > done?
>
> Simplicity perhaps, but I'll definitely give your suggestions
> a go and see where we end up.
Since I haven't fully understood the distinction between the
three chardevs, it may of course turn out a bad idea, but I
think it would simplify the core code if you could assume
that every iio device has exactly one chardev interface,
so you could give them the same unique number and manage
the life time together.
> >>> * Use epoll to wait for data and/or out-of-band messages
> >>> * Use chrdev read to get events from the buffer
> >>
> >> and data?
> >
> > I mean get the data associated with the event. The event
> > itself as you said does not have any data, so we would not
> > need to read it, just to use poll()/epoll() in order to
> > wait for it.
>
> Sure. But devices can do a heck of a lot of different events.
> (certainly 10's or maybe more). I'm not immediately clear
> on how to handle this via poll etc. This is probably just
> because I've never tried though!
(e)poll can generally distinguish between very few types of
activity: data for reading available, space for writing available,
out-of-band events (to be read with e.g. ioctl) and errors.
If you want to wait for multiple equal types of events for
one hardware device, it would be logical to have multiple
character devices for them, so a user could open and wait
for some of them independent of the others.
Intuitively, I would also expect these to be separate iio
devices for the same hardware (each with one chardev), but
there may be good reasons why that is not possible.
Arnd
On 03/17/11 15:03, Arnd Bergmann wrote:
> On Thursday 17 March 2011, Jonathan Cameron wrote:
>> On 03/17/11 13:47, Arnd Bergmann wrote:
>
>>> What about hardware devices that have multiple unrelated streams
>>> of buffered input data?
>>
>> Certainly plausible, but so far the only ones I've seen that actually
>> do this are really just two bits of silicon in the same plastic
>> package. They tend to use different i2c addresses or spi chip
>> selects anyway so as far as the kernel is concerned are completely
>> separate. You are correct that any device which truly has different
>> streams of data would indeed need more than one device.
>
> Ok.
>
>>>>> * One chardev for each iio device
>>>>
>>>> currently 1-3. (event line, buffer access, buffer event)
>>>
>>> It would be really nice to unify this, as I said. What
>>> are the reasons why you think it cannot or should not be
>>> done?
>>
>> Simplicity perhaps, but I'll definitely give your suggestions
>> a go and see where we end up.
>
> Since I haven't fully understood the distinction between the
> three chardevs, it may of course turn out a bad idea, but I
> think it would simplify the core code if you could assume
> that every iio device has exactly one chardev interface,
> so you could give them the same unique number and manage
> the life time together.
It simplifies that corner, but I'm a little worried that it
will add a lot of interlinks between the currently fairly
disconnected elements that go through a character device.
If we can keep those links to a minimum (which I think
we can, but haven't tried yet!) it will be a sensible move.
>
>>>>> * Use epoll to wait for data and/or out-of-band messages
>>>>> * Use chrdev read to get events from the buffer
>>>>
>>>> and data?
>>>
>>> I mean get the data associated with the event. The event
>>> itself as you said does not have any data, so we would not
>>> need to read it, just to use poll()/epoll() in order to
>>> wait for it.
>>
>> Sure. But devices can do a heck of a lot of different events.
>> (certainly 10's or maybe more). I'm not immediately clear
>> on how to handle this via poll etc. This is probably just
>> because I've never tried though!
>
> (e)poll can generally distinguish between very few types of
> activity: data for reading available, space for writing available,
> out-of-band events (to be read with e.g. ioctl) and errors.
>
> If you want to wait for multiple equal types of events for
> one hardware device, it would be logical to have multiple
> character devices for them, so a user could open and wait
> for some of them independent of the others.
>
> Intuitively, I would also expect these to be separate iio
> devices for the same hardware (each with one chardev), but
> there may be good reasons why that is not possible.
For reasons above, there can only be one iio device per
physical hardware. We could define some other intermediate
representation similar to the bus structure we currently have,
but then I'm not sure where we gain.
As we only care about single reader cases here,
the reader can simply configure which events it is interested
in to be the only ones produced. A good chunk of the sysfs
interface is concerned with doing this.
The ioctl approach you suggest can then be used to query what
actually occurred.
On 03/17/11 16:46, Jonathan Cameron wrote:
> On 03/17/11 15:03, Arnd Bergmann wrote:
>> On Thursday 17 March 2011, Jonathan Cameron wrote:
>>> On 03/17/11 13:47, Arnd Bergmann wrote:
>>
>>>> What about hardware devices that have multiple unrelated streams
>>>> of buffered input data?
>>>
>>> Certainly plausible, but so far the only ones I've seen that actually
>>> do this are really just two bits of silicon in the same plastic
>>> package. They tend to use different i2c addresses or spi chip
>>> selects anyway so as far as the kernel is concerned are completely
>>> separate. You are correct that any device which truly has different
>>> streams of data would indeed need more than one device.
>>
>> Ok.
>>
>>>>>> * One chardev for each iio device
>>>>>
>>>>> currently 1-3. (event line, buffer access, buffer event)
>>>>
>>>> It would be really nice to unify this, as I said. What
>>>> are the reasons why you think it cannot or should not be
>>>> done?
>>>
>>> Simplicity perhaps, but I'll definitely give your suggestions
>>> a go and see where we end up.
>>
>> Since I haven't fully understood the distinction between the
>> three chardevs, it may of course turn out a bad idea, but I
>> think it would simplify the core code if you could assume
>> that every iio device has exactly one chardev interface,
>> so you could give them the same unique number and manage
>> the life time together.
> It simplifies that corner, but I'm a little worried that it
> will add a lot of interlinks between the currently fairly
> disconnected elements that go through a character device.
>
> If we can keep those links to a minimum (which I think
> we can, but haven't tried yet!) it will be a sensible move.
>
>>
>>>>>> * Use epoll to wait for data and/or out-of-band messages
>>>>>> * Use chrdev read to get events from the buffer
>>>>>
>>>>> and data?
>>>>
>>>> I mean get the data associated with the event. The event
>>>> itself as you said does not have any data, so we would not
>>>> need to read it, just to use poll()/epoll() in order to
>>>> wait for it.
>>>
>>> Sure. But devices can do a heck of a lot of different events.
>>> (certainly 10's or maybe more). I'm not immediately clear
>>> on how to handle this via poll etc. This is probably just
>>> because I've never tried though!
>>
>> (e)poll can generally distinguish between very few types of
>> activity: data for reading available, space for writing available,
>> out-of-band events (to be read with e.g. ioctl) and errors.
>>
>> If you want to wait for multiple equal types of events for
>> one hardware device, it would be logical to have multiple
>> character devices for them, so a user could open and wait
>> for some of them independent of the others.
>>
>> Intuitively, I would also expect these to be separate iio
>> devices for the same hardware (each with one chardev), but
>> there may be good reasons why that is not possible.
> For reasons above,
Actually in the other branch of the thread. sorry!
> there can only be one iio device per
> physical hardware. We could define some other intermediate
> representation similar to the bus structure we currently have,
> but then I'm not sure where we gain.
>
> As we only care about single reader cases here,
> the reader can simply configure which events it is interested
> in to be the only ones produced. A good chunk of the sysfs
> interface is concerned with doing this.
>
> The ioctl approach you suggest can then be used to query what
> actually occurred.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
...
>>>> The moment we put a route in for registering additional attributes
>>>> the out-of-tree argument is weakened.
>>>
>>> Would it help to have multiple types of interfaces (I suppose
>>> you already do), each with their own table?
>>>
>>> This way you could enforce that all drivers of one type use
>>> exactly the same specification, but also make it possible
>>> to have new types, even those that have only one driver by
>>> design.
>>
>> Sadly there are no clean definitions of types. You'd be amazed what
>> downright weird sets end up in a given device.
>> Might be some way of breaking it down into a set of tables though
>> in a vaguely coherent way. Each device would then have one or more
>> of these for which it would define one or more of the functions.
>
> Right, or you could mandate that each device has exactly one of these,
> but that you could have multiple iio devices in a single hardware
> device.
That might lead to some very ugly largely false divides. I guess trying
it is the only way to find out. Some things will be really fiddly if
we make this divide, particularly buffering. The hardware tends to
spit out 'scans' of a set of channels taken at more or less the same time.
If we split into multiple iio devices we'll need to split this data into
streams for each one. The interactions between the available channels
can also be extremely complex, so we need to make it absolutely clear
which physical device they are on. Note that you can only tell what
'scan' elements are configured by trying to set up what you want, then
reading back to see what you got. In some cases you even end up with
more than you asked for.
Probably the only way to do work out how manageable this is will
be to list what would be in this table, with vaguely sane restrictions.
Some awkward corner cases include differential channels where we
at least in theory have a combinatorial explosion. (inX-inY_raw)
We also have to double to cover _raw and _input cases though these
could easily be a pair of tables or use some metadata for each channel.
Taking just the stuff in docs for the raw channel readings (and restricting
based on what I have to hand). Input devices only.
in0, in1, in2, in3, in4, in5, in6, in7, in8, in9, in10, in11,
in0-in1, in0-in1, in2-in3, in3-in2, in5-in4, in4-in5, in7-in6,
in8-in9, in9-in8, in10-in11, in11-in10
accel_x, accel_y, accel_z, accel_x_peak, accel_y_peak, accel_z_peak, accel_xyz_squared_peak,
temp, temp_x, temp_y, temp_z, gyro_x, gyro_y, gyro_z,
magn_x, magn_y, magn_z, incli_x, inci_y, incli_z,
illuminance0, proximity, intensity_infrared.
So a starting set of 46. Also note that the really large tables will
be handling the event controls. The only consistent way we have come
up with for that so far is to have the option of a separate value and
enable attribute for every channel and every type of event. This would
definitely require more complex metadata to do. I'd suggest a bitmap
specifying which are available.
The compound versions (accel_xyz_squared_peak etc) are relatively unusual.
We haven't covered energy meters or resolvers here as they don't have
documented interfaces yet. It'll be about another 10 for them.
So all in all a flat table structure is going to be huge. Given the
core won't operate on these things terribly often (basically setup
and tear down) rather than a table of things that may or may
not be set, we could have an association table which says what
is present.
It would be interesting to work out what the minumum structure
required to generate everything associated with a given channel
actually looks like...
struct CHAN {
enum CHAN_TYPE type;
int index; (x = 0, y = 1 etc).
long sharedmask; //which of the functions are shared
//so can we do single accel_bias
//rather than accel_x_bias etc..
long eventmask; //what events exist;
bool scannable;
etc.
}
Then set of callbacks to be registered:
read_val(type, index);
read_calibbais(type index);
etc.
This would keep the table reasonably short at the cost of a fair
number of function pointers. Using the masks, the core can figure
out what attributes should exist and wire them up.
>
>>> is to have an ioctl function that returns structured
>>> data, while the regular read function returns the buffered
>>> input stream.
>>
>> That would work, but it is it really worth it to kill off the chrdev?
>
> I don't know.
>
>> Note we still need most of the underlying structure just to keep the
>> various attributes well categorized.
>
> The hardest part of interface design is always to find the simplest
> possible way that can cover all the corner cases. I'm trying to
> come up with possible simplifications, but I don't know all the
> requirements that get in the way.
Sure. Most of these suggestions are going to be a case of suck it
and see. We could analyse ever case we have in there now, but
a new one we haven't considered could come along tomorrow. That's
partly why the subsystem has kept up a high rate of changes whilst
it's been in staging.
>
>>>>> Doesn't epoll handle the event escalation with its edge triggered
>>>>> mode? Basically you can get notified every time that there is
>>>>> more to be read.
>>>> We also need to know how much more this is to be read. So at the
>>>> moment, if a 50% full turns up and isn't picked up by userspace
>>>> before a 75% full event, the 50% is escalated to become a 75%
>>>> event which userspace then receives. To be honest I'm tempted,
>>>> for now at least to drop this functionality. It has few users
>>>> and makes the events system considerably more complex. We can
>>>> think about how / whether to introduce it later.
>>>
>>> Yes, good point. I also don't get the use case of this. Why would
>>> user space not just always read all data that is available?
>> Good point. We could just move the handling of these events into the
>> actual driver (this is only ever relevant for hardware buffers). Thus
>> it could keep track of what it knows is there. Note these devices don't
>> always provide any other way of knowing and yes you can read more data
>> from them than is actually there. Whether you can tell this has happened
>> is very much device dependent.
>
> Can you give an example? I don't understand what you mean with "can read
> mode data [...] than is actually there".
I might have the details of this wrong as I haven't checked everything
against the datasheets.
Lets take the sca3000 VTI accelerometers.
They have 64 scan (read of x, y, z set) long hardware ring buffers.
There are two watershed events for how full the buffer is, 50% and 75%.
These result in a pin being raised if enabled. This is routed through
a gpio on the host as an interrupt. These thresholds are edge triggered
so will not retrigger unless the contents falls below the level then
rises past it again.
Currently we pass that event straight up to userspace. Userspace can
then know there is at least that much data in the buffer and hence
request that much data back.
If user space attempts to read less data from the buffer than is there
it is all fine and assuming user space always reads at least 50% then
we will get a new event. If only one of the 50% full or 75% full
events is enabled, then we don't need to read which one occurred. Thus
we cut out one transaction on the bus (given the bus is slow this
really can matter). The delays around a transaction can be a significant
part of the time taken to do a transaction (so this isn't swamped by
the time taken to read 32*3*2 bytes).
There is a separate register for the number of samples in the buffer. Right
now we don't use it. On other parts this may not be present (as it
needn't ever be used. It was this case I was referring two.
I'd forgotten it existed till I checked just now.
So options to clean up this case and avoid event escalation.
1) Allow only one watershead interrupt to be configured on the device
at a time. (issue is if we ever hit a device where we can't turn them off).
2) Burn a transaction reading the buffer count then make sure we don't read
more than is there. The events then just become a use of poll - perhaps
different flags for 50% full or 75% full if they are both enabled.
3) Handle the two watershead events in the driver, basically holding state.
Things may get fiddly if an event shows up during a read.
For simplicity of review I'm tempted to go with 1 and make the a
requirement of all drivers unless someone comes up with a very
good reason why we need this functionality.
>
>>>> Yup, the dynamic route to creation is what I favour. As you say
>>>> this needs fixing up before going out of staging. We definitely
>>>> need these for a bridge to input to implement polled input devices.
>>>>
>>>> I'll put a todo in place for that element in staging so we don't
>>>> forget.
>>>
>> Note I have quite a few changes currently queued up so might be a little while
>> before I get to trying out the more complex stuff you have suggested.
>
> Ok, no problem. Just take your time.
Thanks again for your advice. You've certainly made me think about some
areas I haven't considered for quite a while!
Jonathan
On Thursday 17 March 2011 17:47:52 Jonathan Cameron wrote:
> > Right, or you could mandate that each device has exactly one of these,
> > but that you could have multiple iio devices in a single hardware
> > device.
>
> That might lead to some very ugly largely false divides. I guess trying
> it is the only way to find out. Some things will be really fiddly if
> we make this divide, particularly buffering. The hardware tends to
> spit out 'scans' of a set of channels taken at more or less the same time.
> If we split into multiple iio devices we'll need to split this data into
> streams for each one. The interactions between the available channels
> can also be extremely complex, so we need to make it absolutely clear
> which physical device they are on. Note that you can only tell what
> 'scan' elements are configured by trying to set up what you want, then
> reading back to see what you got. In some cases you even end up with
> more than you asked for.
Ok, that makes sense. As you say, it's still possible to split
the hardware streams into multiple streams on character devices,
but it's less clear that this would simplify anything.
However, when you have devices that have separate hardware buffers
for their channels, I think there is a much stronger incentive
to model them as separate character devices.
> Probably the only way to do work out how manageable this is will
> be to list what would be in this table, with vaguely sane restrictions.
> Some awkward corner cases include differential channels where we
> at least in theory have a combinatorial explosion. (inX-inY_raw)
> We also have to double to cover _raw and _input cases though these
> could easily be a pair of tables or use some metadata for each channel.
>
> Taking just the stuff in docs for the raw channel readings (and restricting
> based on what I have to hand). Input devices only.
>
> in0, in1, in2, in3, in4, in5, in6, in7, in8, in9, in10, in11,
> in0-in1, in0-in1, in2-in3, in3-in2, in5-in4, in4-in5, in7-in6,
> in8-in9, in9-in8, in10-in11, in11-in10
> accel_x, accel_y, accel_z, accel_x_peak, accel_y_peak, accel_z_peak, accel_xyz_squared_peak,
> temp, temp_x, temp_y, temp_z, gyro_x, gyro_y, gyro_z,
> magn_x, magn_y, magn_z, incli_x, inci_y, incli_z,
> illuminance0, proximity, intensity_infrared.
>
> So a starting set of 46. Also note that the really large tables will
> be handling the event controls. The only consistent way we have come
> up with for that so far is to have the option of a separate value and
> enable attribute for every channel and every type of event. This would
> definitely require more complex metadata to do. I'd suggest a bitmap
> specifying which are available.
I don't completely understand the notation. Regarding the various
{in0, in1, in2, ...} inputs, is there a fundamental difference between
them? In the code example I gave, a driver would simply list
a set of inputs of the same type (IIO_CHAN_IN) and let the core
enumerate them. What does "in0-in1" mean?
Also, the only table that would have to list all of them would be
in the core, to list the names, and possibly a function point for
pretty-printing the data, but many of these function pointers would
point to the same few functions.
> The compound versions (accel_xyz_squared_peak etc) are relatively unusual.
>
> We haven't covered energy meters or resolvers here as they don't have
> documented interfaces yet. It'll be about another 10 for them.
Fair enough.
> So all in all a flat table structure is going to be huge. Given the
> core won't operate on these things terribly often (basically setup
> and tear down) rather than a table of things that may or may
> not be set, we could have an association table which says what
> is present.
Well, a driver only needs to list what it has, and that could
be written in a rather compact way.
> It would be interesting to work out what the minumum structure
> required to generate everything associated with a given channel
> actually looks like...
>
> struct CHAN {
> enum CHAN_TYPE type;
> int index; (x = 0, y = 1 etc).
Do you have drivers that have sparse indices? The core could simply
enumerate them when it encounters channels of the same type for
one device.
> long sharedmask; //which of the functions are shared
> //so can we do single accel_bias
> //rather than accel_x_bias etc..
> long eventmask; //what events exist;
> bool scannable;
> etc.
> }
Ok, I didn't think of other fields, but these seem to be needed,
even if I don't understand them.
> Then set of callbacks to be registered:
>
> read_val(type, index);
> read_calibbais(type index);
>
> etc.
>
> This would keep the table reasonably short at the cost of a fair
> number of function pointers. Using the masks, the core can figure
> out what attributes should exist and wire them up.
I don't think you need many function pointers. Having a function
pointer in struct chan is may be a good idea, but if you have
ten inputs that are all alike, they can all point to the same
function, right?
> >> Good point. We could just move the handling of these events into the
> >> actual driver (this is only ever relevant for hardware buffers). Thus
> >> it could keep track of what it knows is there. Note these devices don't
> >> always provide any other way of knowing and yes you can read more data
> >> from them than is actually there. Whether you can tell this has happened
> >> is very much device dependent.
> >
> > Can you give an example? I don't understand what you mean with "can read
> > mode data [...] than is actually there".
>
> I might have the details of this wrong as I haven't checked everything
> against the datasheets.
> Lets take the sca3000 VTI accelerometers.
>
> They have 64 scan (read of x, y, z set) long hardware ring buffers.
> There are two watershed events for how full the buffer is, 50% and 75%.
> These result in a pin being raised if enabled. This is routed through
> a gpio on the host as an interrupt. These thresholds are edge triggered
> so will not retrigger unless the contents falls below the level then
> rises past it again.
Ok, I see. So the hardware provides no way at all to find out how
many events you are allowed to read, other than being allowed to
read half the events if you got the 50% interrupt, right?
I wonder what the hardware people were thinking, there should at least
be a way to find out whether the buffer is completely empty!
> Currently we pass that event straight up to userspace. Userspace can
> then know there is at least that much data in the buffer and hence
> request that much data back.
>
> If user space attempts to read less data from the buffer than is there
> it is all fine and assuming user space always reads at least 50% then
> we will get a new event. If only one of the 50% full or 75% full
> events is enabled, then we don't need to read which one occurred. Thus
> we cut out one transaction on the bus (given the bus is slow this
> really can matter). The delays around a transaction can be a significant
> part of the time taken to do a transaction (so this isn't swamped by
> the time taken to read 32*3*2 bytes).
To be honest, this sounds like a horrible user interface. The kernel
should always try to shield the user from details of the hardware buffer
and provide an interface that can be used like a pipe: If there is
any data to read, let the user get it, otherwise block the reading
process or return -EAGAIN (in case of O_NONBLOCK) and wait for the
user to call poll() for notification.
The kernel probably can spare much more RAM for input data than
the device, so the natural implementation in the kernel would
register a handler for the interrupt that reads all available data
into a kernel fifo buffer, which gets emptied on the next read
from user space.
> There is a separate register for the number of samples in the buffer. Right
> now we don't use it. On other parts this may not be present (as it
> needn't ever be used. It was this case I was referring two.
> I'd forgotten it existed till I checked just now.
Ok. I truely hope that most hardware has something like this, but
we can probably work around it as explained above if not.
> So options to clean up this case and avoid event escalation.
>
> 1) Allow only one watershead interrupt to be configured on the device
> at a time. (issue is if we ever hit a device where we can't turn them off).
>
> 2) Burn a transaction reading the buffer count then make sure we don't read
> more than is there. The events then just become a use of poll - perhaps
> different flags for 50% full or 75% full if they are both enabled.
>
> 3) Handle the two watershead events in the driver, basically holding state.
> Things may get fiddly if an event shows up during a read.
>
> For simplicity of review I'm tempted to go with 1 and make the a
> requirement of all drivers unless someone comes up with a very
> good reason why we need this functionality.
I would argue for a combination of 1 & 2. Configuring which of the
two interrupts you want would be determined by the real-time and/or
power management requirements, but should not be visible to the
application reading the data, only when setting up the device.
Arnd
On Thursday 17 March 2011 17:46:48 Jonathan Cameron wrote:
> >
> > Since I haven't fully understood the distinction between the
> > three chardevs, it may of course turn out a bad idea, but I
> > think it would simplify the core code if you could assume
> > that every iio device has exactly one chardev interface,
> > so you could give them the same unique number and manage
> > the life time together.
>
> It simplifies that corner, but I'm a little worried that it
> will add a lot of interlinks between the currently fairly
> disconnected elements that go through a character device.
>
> If we can keep those links to a minimum (which I think
> we can, but haven't tried yet!) it will be a sensible move.
Let's first work out how a single event buffer should work
using read and poll, as discussed in the other thread.
Once we have sorted that out, this may become a lot clearer.
Arnd
On 03/17/11 17:51, Arnd Bergmann wrote:
> On Thursday 17 March 2011 17:47:52 Jonathan Cameron wrote:
>
>>> Right, or you could mandate that each device has exactly one of these,
>>> but that you could have multiple iio devices in a single hardware
>>> device.
>>
>> That might lead to some very ugly largely false divides. I guess trying
>> it is the only way to find out. Some things will be really fiddly if
>> we make this divide, particularly buffering. The hardware tends to
>> spit out 'scans' of a set of channels taken at more or less the same time.
>> If we split into multiple iio devices we'll need to split this data into
>> streams for each one. The interactions between the available channels
>> can also be extremely complex, so we need to make it absolutely clear
>> which physical device they are on. Note that you can only tell what
>> 'scan' elements are configured by trying to set up what you want, then
>> reading back to see what you got. In some cases you even end up with
>> more than you asked for.
>
> Ok, that makes sense. As you say, it's still possible to split
> the hardware streams into multiple streams on character devices,
> but it's less clear that this would simplify anything.
>
> However, when you have devices that have separate hardware buffers
> for their channels, I think there is a much stronger incentive
> to model them as separate character devices.
Agreed. I reserve the right to make rude noises at any hardware
manufacturers who have complex interactions between channels
spread across multiple hardware buffers.
>
>> Probably the only way to do work out how manageable this is will
>> be to list what would be in this table, with vaguely sane restrictions.
>> Some awkward corner cases include differential channels where we
>> at least in theory have a combinatorial explosion. (inX-inY_raw)
>> We also have to double to cover _raw and _input cases though these
>> could easily be a pair of tables or use some metadata for each channel.
>>
>> Taking just the stuff in docs for the raw channel readings (and restricting
>> based on what I have to hand). Input devices only.
>>
>> in0, in1, in2, in3, in4, in5, in6, in7, in8, in9, in10, in11,
>> in0-in1, in0-in1, in2-in3, in3-in2, in5-in4, in4-in5, in7-in6,
>> in8-in9, in9-in8, in10-in11, in11-in10
>> accel_x, accel_y, accel_z, accel_x_peak, accel_y_peak, accel_z_peak, accel_xyz_squared_peak,
>> temp, temp_x, temp_y, temp_z, gyro_x, gyro_y, gyro_z,
>> magn_x, magn_y, magn_z, incli_x, inci_y, incli_z,
>> illuminance0, proximity, intensity_infrared.
>>
>> So a starting set of 46. Also note that the really large tables will
>> be handling the event controls. The only consistent way we have come
>> up with for that so far is to have the option of a separate value and
>> enable attribute for every channel and every type of event. This would
>> definitely require more complex metadata to do. I'd suggest a bitmap
>> specifying which are available.
>
> I don't completely understand the notation. Regarding the various
> {in0, in1, in2, ...} inputs, is there a fundamental difference between
> them? In the code example I gave, a driver would simply list
> a set of inputs of the same type (IIO_CHAN_IN) and let the core
> enumerate them. What does "in0-in1" mean?
in0-in1 is a differential adc channel. Literally outputs value on
physical pin 1 subtracted from physical pin 2.
>
> Also, the only table that would have to list all of them would be
> in the core, to list the names, and possibly a function point for
> pretty-printing the data, but many of these function pointers would
> point to the same few functions.
True.
>
>> The compound versions (accel_xyz_squared_peak etc) are relatively unusual.
>>
>> We haven't covered energy meters or resolvers here as they don't have
>> documented interfaces yet. It'll be about another 10 for them.
>
> Fair enough.
>
>> So all in all a flat table structure is going to be huge. Given the
>> core won't operate on these things terribly often (basically setup
>> and tear down) rather than a table of things that may or may
>> not be set, we could have an association table which says what
>> is present.
>
> Well, a driver only needs to list what it has, and that could
> be written in a rather compact way.
>
>> It would be interesting to work out what the minumum structure
>> required to generate everything associated with a given channel
>> actually looks like...
>>
>> struct CHAN {
>> enum CHAN_TYPE type;
>> int index; (x = 0, y = 1 etc).
>
> Do you have drivers that have sparse indices? The core could simply
> enumerate them when it encounters channels of the same type for
> one device.
Sadly yes we do. Some IMUs have 3D accelerometer and 2D gyros.
>
>> long sharedmask; //which of the functions are shared
>> //so can we do single accel_bias
>> //rather than accel_x_bias etc..
>> long eventmask; //what events exist;
>> bool scannable;
>> etc.
>> }
>
> Ok, I didn't think of other fields, but these seem to be needed,
> even if I don't understand them.
They basically provide information for the creation of the rest of abi apart form direct
reading attributes. I merely want to avoid having lots of parallel tables.
>
>> Then set of callbacks to be registered:
>>
>> read_val(type, index);
>> read_calibbais(type index);
>>
>> etc.
>>
>> This would keep the table reasonably short at the cost of a fair
>> number of function pointers. Using the masks, the core can figure
>> out what attributes should exist and wire them up.
>
> I don't think you need many function pointers. Having a function
> pointer in struct chan is may be a good idea, but if you have
> ten inputs that are all alike, they can all point to the same
> function, right?
Agreed. I had them in there originally but decided it was getting rather
clunky. In a sense this will look a little like taking the current
huge attribute tables and breaking them up into bits associated with
each channel. We may want a certain amount of 'private_data' space
in the channel array as well to allow for things like addresses. Not
sure on that yet though.
>
>>>> Good point. We could just move the handling of these events into the
>>>> actual driver (this is only ever relevant for hardware buffers). Thus
>>>> it could keep track of what it knows is there. Note these devices don't
>>>> always provide any other way of knowing and yes you can read more data
>>>> from them than is actually there. Whether you can tell this has happened
>>>> is very much device dependent.
>>>
>>> Can you give an example? I don't understand what you mean with "can read
>>> mode data [...] than is actually there".
>>
>> I might have the details of this wrong as I haven't checked everything
>> against the datasheets.
>> Lets take the sca3000 VTI accelerometers.
>>
>> They have 64 scan (read of x, y, z set) long hardware ring buffers.
>> There are two watershed events for how full the buffer is, 50% and 75%.
>> These result in a pin being raised if enabled. This is routed through
>> a gpio on the host as an interrupt. These thresholds are edge triggered
>> so will not retrigger unless the contents falls below the level then
>> rises past it again.
>
> Ok, I see. So the hardware provides no way at all to find out how
> many events you are allowed to read, other than being allowed to
> read half the events if you got the 50% interrupt, right?
Yes, that's the nasty case.
'scans' in the terminology we are using (comes from that used in ADC
datasheets - have to be careful not to confuse the out of band stuff
which we are calling 'events' from the in band data flow).
> I wonder what the hardware people were thinking, there should at least
> be a way to find out whether the buffer is completely empty!
It actually makes sense on entirely interrupt driver embedded hardware.
In those case there are no pesky user controls allowing them to try
and read at the wrong time.
VTI seem to have decided their hardware buffers were a dead end and the
newer entries in that field tend to be more sensible (analog has
quite a few parts with hardware buffers, though some are somewhat
esoteric to put it lightly - their vibration sensors are going
to require some 'interesting' drivers to put it lightly).
>
>> Currently we pass that event straight up to userspace. Userspace can
>> then know there is at least that much data in the buffer and hence
>> request that much data back.
>>
>> If user space attempts to read less data from the buffer than is there
>> it is all fine and assuming user space always reads at least 50% then
>> we will get a new event. If only one of the 50% full or 75% full
>> events is enabled, then we don't need to read which one occurred. Thus
>> we cut out one transaction on the bus (given the bus is slow this
>> really can matter). The delays around a transaction can be a significant
>> part of the time taken to do a transaction (so this isn't swamped by
>> the time taken to read 32*3*2 bytes).
>
> To be honest, this sounds like a horrible user interface. The kernel
> should always try to shield the user from details of the hardware buffer
> and provide an interface that can be used like a pipe: If there is
> any data to read, let the user get it, otherwise block the reading
> process or return -EAGAIN (in case of O_NONBLOCK) and wait for the
> user to call poll() for notification.
>
> The kernel probably can spare much more RAM for input data than
> the device, so the natural implementation in the kernel would
> register a handler for the interrupt that reads all available data
> into a kernel fifo buffer, which gets emptied on the next read
> from user space.
>
>> There is a separate register for the number of samples in the buffer. Right
>> now we don't use it. On other parts this may not be present (as it
>> needn't ever be used. It was this case I was referring two.
>> I'd forgotten it existed till I checked just now.
>
> Ok. I truely hope that most hardware has something like this, but
> we can probably work around it as explained above if not.
Yes. Though do beware. spi and i2c buses for some of these things
can be 'very' slow and often congested on the actual boards. Hence
we sometimes spend a lot of effort to avoid transactions.
>
>> So options to clean up this case and avoid event escalation.
>>
>> 1) Allow only one watershead interrupt to be configured on the device
>> at a time. (issue is if we ever hit a device where we can't turn them off).
>>
>> 2) Burn a transaction reading the buffer count then make sure we don't read
>> more than is there. The events then just become a use of poll - perhaps
>> different flags for 50% full or 75% full if they are both enabled.
>>
>> 3) Handle the two watershead events in the driver, basically holding state.
>> Things may get fiddly if an event shows up during a read.
>>
>> For simplicity of review I'm tempted to go with 1 and make the a
>> requirement of all drivers unless someone comes up with a very
>> good reason why we need this functionality.
>
> I would argue for a combination of 1 & 2. Configuring which of the
> two interrupts you want would be determined by the real-time and/or
> power management requirements, but should not be visible to the
> application reading the data, only when setting up the device.
I'd prefer to allow some direct control. There are use cases where
for filtering purposes you are only interested in a particular
length block of data. Still, that control may be the exception
rather than rule. Lets just turn on the 50% by default then
vast majority of users won't ever touch it!
>
> Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Thursday 17 March 2011, Jonathan Cameron wrote:
> On 03/17/11 17:51, Arnd Bergmann wrote:
> > I don't completely understand the notation. Regarding the various
> > {in0, in1, in2, ...} inputs, is there a fundamental difference between
> > them? In the code example I gave, a driver would simply list
> > a set of inputs of the same type (IIO_CHAN_IN) and let the core
> > enumerate them. What does "in0-in1" mean?
>
> in0-in1 is a differential adc channel. Literally outputs value on
> physical pin 1 subtracted from physical pin 2.
Ok, I see. So these would be fairly hard to enumerate, right?
Would it be possible to have one attribute with named "diff%d"
and another attribute associated with it that describes which
channels are compared?
> >> It would be interesting to work out what the minumum structure
> >> required to generate everything associated with a given channel
> >> actually looks like...
> >>
> >> struct CHAN {
> >> enum CHAN_TYPE type;
> >> int index; (x = 0, y = 1 etc).
> >
> > Do you have drivers that have sparse indices? The core could simply
> > enumerate them when it encounters channels of the same type for
> > one device.
>
> Sadly yes we do. Some IMUs have 3D accelerometer and 2D gyros.
Ok, I see. So you might have {x0,y0} for one sensor but {x1,y1,z1}
for the other one, right?
> > I don't think you need many function pointers. Having a function
> > pointer in struct chan is may be a good idea, but if you have
> > ten inputs that are all alike, they can all point to the same
> > function, right?
> Agreed. I had them in there originally but decided it was getting rather
> clunky. In a sense this will look a little like taking the current
> huge attribute tables and breaking them up into bits associated with
> each channel. We may want a certain amount of 'private_data' space
> in the channel array as well to allow for things like addresses. Not
> sure on that yet though.
Makes sense. So you either need a private-data pointer for each
element and point that to another static data structure, or you
need two arrays of different structures but using the same indices.
I think both ways would work, but it would be nice to come up with
a cleaner solution.
Maybe it could be an anonymous union of an unsigned long and a
pointer, so you can initialize either of the two members, depending
on how complex the driver needs it.
> > Ok. I truely hope that most hardware has something like this, but
> > we can probably work around it as explained above if not.
>
> Yes. Though do beware. spi and i2c buses for some of these things
> can be 'very' slow and often congested on the actual boards. Hence
> we sometimes spend a lot of effort to avoid transactions.
Do the transactions require spinning on the CPU, or do they
always work in the background when they are slow?
> >> For simplicity of review I'm tempted to go with 1 and make the a
> >> requirement of all drivers unless someone comes up with a very
> >> good reason why we need this functionality.
> >
> > I would argue for a combination of 1 & 2. Configuring which of the
> > two interrupts you want would be determined by the real-time and/or
> > power management requirements, but should not be visible to the
> > application reading the data, only when setting up the device.
> I'd prefer to allow some direct control. There are use cases where
> for filtering purposes you are only interested in a particular
> length block of data. Still, that control may be the exception
> rather than rule. Lets just turn on the 50% by default then
> vast majority of users won't ever touch it!
Ok.
Arnd
On 03/18/11 12:47, Arnd Bergmann wrote:
> On Thursday 17 March 2011, Jonathan Cameron wrote:
>> On 03/17/11 17:51, Arnd Bergmann wrote:
>
>>> I don't completely understand the notation. Regarding the various
>>> {in0, in1, in2, ...} inputs, is there a fundamental difference between
>>> them? In the code example I gave, a driver would simply list
>>> a set of inputs of the same type (IIO_CHAN_IN) and let the core
>>> enumerate them. What does "in0-in1" mean?
>>
>> in0-in1 is a differential adc channel. Literally outputs value on
>> physical pin 1 subtracted from physical pin 2.
>
> Ok, I see. So these would be fairly hard to enumerate, right?
> Would it be possible to have one attribute with named "diff%d"
> and another attribute associated with it that describes which
> channels are compared?
Could do, but what would it gain us? If the information is available
to the core, then it can use it give us the explicit naming? As shown
below we have to handle holes in the enumeration anyway so this isn't
to much of a problem.
>
>>>> It would be interesting to work out what the minumum structure
>>>> required to generate everything associated with a given channel
>>>> actually looks like...
>>>>
>>>> struct CHAN {
>>>> enum CHAN_TYPE type;
>>>> int index; (x = 0, y = 1 etc).
>>>
>>> Do you have drivers that have sparse indices? The core could simply
>>> enumerate them when it encounters channels of the same type for
>>> one device.
>>
>> Sadly yes we do. Some IMUs have 3D accelerometer and 2D gyros.
>
> Ok, I see. So you might have {x0,y0} for one sensor but {x1,y1,z1}
> for the other one, right?
Yup. Or even my favourite of two accelerometers on some axis and not
others (to give high accuracy for low acceleration and low accuracy
over a much larger range).
>
>>> I don't think you need many function pointers. Having a function
>>> pointer in struct chan is may be a good idea, but if you have
>>> ten inputs that are all alike, they can all point to the same
>>> function, right?
>> Agreed. I had them in there originally but decided it was getting rather
>> clunky. In a sense this will look a little like taking the current
>> huge attribute tables and breaking them up into bits associated with
>> each channel. We may want a certain amount of 'private_data' space
>> in the channel array as well to allow for things like addresses. Not
>> sure on that yet though.
>
> Makes sense. So you either need a private-data pointer for each
> element and point that to another static data structure, or you
> need two arrays of different structures but using the same indices.
>
> I think both ways would work, but it would be nice to come up with
> a cleaner solution.
Agreed.
>
> Maybe it could be an anonymous union of an unsigned long and a
> pointer, so you can initialize either of the two members, depending
> on how complex the driver needs it.
That would be a neat option
>
>>> Ok. I truely hope that most hardware has something like this, but
>>> we can probably work around it as explained above if not.
>>
>> Yes. Though do beware. spi and i2c buses for some of these things
>> can be 'very' slow and often congested on the actual boards. Hence
>> we sometimes spend a lot of effort to avoid transactions.
>
> Do the transactions require spinning on the CPU, or do they
> always work in the background when they are slow?
If you have a proper controller it's often DMA based. So the issue
is more congestion on the bus limiting possible sampling rates than
cpu load. To a certain extent we can just ignore this issue as long
as we are open to changes to a driver to minimise bus usage if someone
has a use case that actually requires it for a given device.
>
>>>> For simplicity of review I'm tempted to go with 1 and make the a
>>>> requirement of all drivers unless someone comes up with a very
>>>> good reason why we need this functionality.
>>>
>>> I would argue for a combination of 1 & 2. Configuring which of the
>>> two interrupts you want would be determined by the real-time and/or
>>> power management requirements, but should not be visible to the
>>> application reading the data, only when setting up the device.
>> I'd prefer to allow some direct control. There are use cases where
>> for filtering purposes you are only interested in a particular
>> length block of data. Still, that control may be the exception
>> rather than rule. Lets just turn on the 50% by default then
>> vast majority of users won't ever touch it!
>
> Ok.
>
> Arnd
>
On Friday 18 March 2011, Jonathan Cameron wrote:
> On 03/18/11 12:47, Arnd Bergmann wrote:
> > On Thursday 17 March 2011, Jonathan Cameron wrote:
> >> On 03/17/11 17:51, Arnd Bergmann wrote:
> >>> I don't completely understand the notation. Regarding the various
> >>> {in0, in1, in2, ...} inputs, is there a fundamental difference between
> >>> them? In the code example I gave, a driver would simply list
> >>> a set of inputs of the same type (IIO_CHAN_IN) and let the core
> >>> enumerate them. What does "in0-in1" mean?
> >>
> >> in0-in1 is a differential adc channel. Literally outputs value on
> >> physical pin 1 subtracted from physical pin 2.
> >
> > Ok, I see. So these would be fairly hard to enumerate, right?
> > Would it be possible to have one attribute with named "diff%d"
> > and another attribute associated with it that describes which
> > channels are compared?
>
> Could do, but what would it gain us? If the information is available
> to the core, then it can use it give us the explicit naming? As shown
> below we have to handle holes in the enumeration anyway so this isn't
> to much of a problem.
Maybe I was seeing problems that don't exist here. Wouldn't
you need to numeric identifiers to describe a differential
channel?
I guess if it's always in${i}-in${i+1}, it's still not too hard.
> >>> Ok. I truely hope that most hardware has something like this, but
> >>> we can probably work around it as explained above if not.
> >>
> >> Yes. Though do beware. spi and i2c buses for some of these things
> >> can be 'very' slow and often congested on the actual boards. Hence
> >> we sometimes spend a lot of effort to avoid transactions.
> >
> > Do the transactions require spinning on the CPU, or do they
> > always work in the background when they are slow?
> If you have a proper controller it's often DMA based. So the issue
> is more congestion on the bus limiting possible sampling rates than
> cpu load. To a certain extent we can just ignore this issue as long
> as we are open to changes to a driver to minimise bus usage if someone
> has a use case that actually requires it for a given device.
Ok.
Arnd
On 03/18/11 16:18, Arnd Bergmann wrote:
> On Friday 18 March 2011, Jonathan Cameron wrote:
>> On 03/18/11 12:47, Arnd Bergmann wrote:
>>> On Thursday 17 March 2011, Jonathan Cameron wrote:
>>>> On 03/17/11 17:51, Arnd Bergmann wrote:
>>>>> I don't completely understand the notation. Regarding the various
>>>>> {in0, in1, in2, ...} inputs, is there a fundamental difference between
>>>>> them? In the code example I gave, a driver would simply list
>>>>> a set of inputs of the same type (IIO_CHAN_IN) and let the core
>>>>> enumerate them. What does "in0-in1" mean?
>>>>
>>>> in0-in1 is a differential adc channel. Literally outputs value on
>>>> physical pin 1 subtracted from physical pin 2.
>>>
>>> Ok, I see. So these would be fairly hard to enumerate, right?
>>> Would it be possible to have one attribute with named "diff%d"
>>> and another attribute associated with it that describes which
>>> channels are compared?
>>
>> Could do, but what would it gain us? If the information is available
>> to the core, then it can use it give us the explicit naming? As shown
>> below we have to handle holes in the enumeration anyway so this isn't
>> to much of a problem.
>
> Maybe I was seeing problems that don't exist here. Wouldn't
> you need to numeric identifiers to describe a differential
> channel?
Agreed, but from the point of view of what is exposed in naming it doesn't
matter. The core and the driver need that number. Userspace doesn't.
Also note that we'll need to pass over an explicit index for the buffering
order anyway so what is one more. The buffering one we can't avoid as
userspace has to know what it is and they are sometimes dependent on
really esoteric rules on the hardware. Right now we assume that
there aren't sets with different contents where the ordering
of any given pair changes. I think that is probably valid.
Hence what we have won't allow
a b c and b a d (in some sense they have to have a monotonic index).
> I guess if it's always in${i}-in${i+1}, it's still not too hard.
I think they have been so far, but doubt this is universal.
How about having a diff type and just having a pair of indices in the
channel structure? Actually may need a third for x^2+y^2+z^2 devices.
(iirc there are parts that do x^2+y^2 despite also having a z channel)
...
On Friday 18 March 2011, Jonathan Cameron wrote:
> > I guess if it's always in${i}-in${i+1}, it's still not too hard.
> I think they have been so far, but doubt this is universal.
> How about having a diff type and just having a pair of indices in the
> channel structure? Actually may need a third for x^2+y^2+z^2 devices.
> (iirc there are parts that do x^2+y^2 despite also having a z channel)
> ...
If two identifiers are common, that would probably be fine.
If you have a x^2+y^2+z^2 device, it might be easier to call that
a different type with a fixed name, as long as there is a small
number of combinations.
Arnd
On 03/18/11 16:57, Arnd Bergmann wrote:
> On Friday 18 March 2011, Jonathan Cameron wrote:
>>> I guess if it's always in${i}-in${i+1}, it's still not too hard.
>> I think they have been so far, but doubt this is universal.
>> How about having a diff type and just having a pair of indices in the
>> channel structure? Actually may need a third for x^2+y^2+z^2 devices.
>> (iirc there are parts that do x^2+y^2 despite also having a z channel)
>> ...
>
> If two identifiers are common, that would probably be fine.
>
> If you have a x^2+y^2+z^2 device, it might be easier to call that
> a different type with a fixed name, as long as there is a small
> number of combinations.
True. A balance to be struck there as and when they occur.
Thanks,
Jonathan