On 08/18/2013 01:19 PM, Jonathan Cameron wrote:
On 08/16/13 14:11, Jacek Anaszewski wrote:This driver supports: - reading channels in 'one shot' mode through read_raw callback, - four events - rising and falling ambient light events and rising and falling proximity roc events. - triggers for all the three channels (triggers can't be enabled simultaneosly with proximity detection event) This is a follow-up of the previous patch and it includes following improvements (Jonathan - thanks for the review) - switched over to using devm_iio_device_alloc - switched over to using devm_request_threaded_irq - switched over to using newly implemented managed allocator for iio_trigger - simplified error handling path in the probe function - switched over to using standard endian conversion functions - removed redundant debugfs interface - removed local reg_mask variables from gp2ap020a00f_set_operation_mode Jonathan, I'd like to also make sure that you've seen my emails in the threads related to the first and the second version of this RFC. I asked there some vital questions related to this driver but they are left unanswered. I will repeat them here:oops. I'm awful at responding to tricky questions / or reading cover letters. Sorry about that.- I am getting warning while calling iio_trigger_poll, and I am not sure if it is acceptable: WARNING: at kernel/irq/handle.c:146handle_irq_event_percpu+0x2a4/0x2b4() irq 8 handler iio_pollfunc_store_time+0x0/0x38 enabled interruptsDrat, I'd missed this entirely. The issue here is that your trigger has to sleep (and hence occurs in a bottom half) in order to work out what it is receiving (event or dataready). Triggers are actually interrupt chips thus the top half is expected to be called in interrupt context but you've already left it in this case. Hence there are two options... Either don't allow for a top half and call iio_trigger_poll_chained instead (which will only call the bottom halfs) or do it in a similar fashion to that done in the sysfs trigger. (drivers/iio/triggers/iio-trig-sysfs) This uses an irq_work structure to jump back into interrupt context and call the iio_trigger_poll successfully.
I've applied the second option. The first one also works but there is a problem with timestamp - its value for each capture is the same. It gets changed only after the driver module is reloaded. It seems to contain garbage as it can assume also extremely high negative values.
We probably need to check for other drivers suffering this issue. Mostly hardware uses separate physical pins for events vs data ready so this isn't that common. I know some ST devices do this. This always made the lis3l02dq driver a pain to deal with (though now the that the irq_work stuff exists that should be easy to tidy up).- I am still encountering "module in use" message when I am trying to execute rmmod on a driver module after generic_buffer application has been launched at least once. This is not specific only to my implementation but also for lps331ap driver (the only one of the remaining IIO drivers supporting triggers I am able to test currently).Umm.. I'm unsure, but it 'might' be something to do with the interrupt issues that are firing the above warning (though I doubt it as the lps331ap isn't suffering from that bug - as it currently stands in tree). Check that all the sysfs entries are as one would expect (no trigger attached or buffered enabled etc). Might be a bug in generic_buffer but I haven't personally seen it do this.
Fixing the warning didn't fix this problem. I've checked sysfs entries - the buffer is not enabled, no trigger is attached. I don't know if this is correct, but when I build build my driver as a module I get also the module industrialio-triggered-buffer.ko built, which has to be loaded prior to the driver module.
- The scale value for the illuminance_clear channel changes dynamically, depending on the lux mode set. I think that currently IIO isn't prepared for such a situation?Indeed not. The overhead to indicate this in buffered mode will completely defeat the object of having that so lightweight. My suggestion would be to apply the scale to the data before pushing it to the driver.
Do you mean before pushing it to the buffer, in the trigger handler? If so, then how to inform the user what scale has been applied to the values in the buffer? And how to provide in-driver-scaled output value to sysfs? Currently there are only *_raw and _*scale attributes available. Maybe it would be convenient to come up with a new attrribute, dedicated to the in-driver-scaled values? Thanks, Jacek -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
