All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH/RFC v4 0/3] gp2ap020a00f ambient light/proximity sensor
@ 2013-08-16 13:11 Jacek Anaszewski
  2013-08-18 11:19 ` Jonathan Cameron
  0 siblings, 1 reply; 7+ messages in thread
From: Jacek Anaszewski @ 2013-08-16 13:11 UTC (permalink / raw)
  To: linux-iio; +Cc: devicetree, jic23, kyungmin.park, s.nawrocki, Jacek Anaszewski

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:

  - 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 interrupts

  - 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).
  - 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?

Besides the above I've detected that write_event_config callback
is called even if the state to be set is already set. This poses
an issue for me, as if it is done by design then I have to handle
it in my driver.

Thanks,
Jacek Anaszewski

Jacek Anaszewski (1):
  iio: gp2ap020a00f: Add a driver for the device

 .../devicetree/bindings/iio/light/gp2ap020a00f.txt |   20 +
 drivers/iio/light/Kconfig                          |   12 +
 drivers/iio/light/Makefile                         |    1 +
 drivers/iio/light/gp2ap020a00f.c                   | 1473 ++++++++++++++++++++
 4 files changed, 1506 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/light/gp2ap020a00f.txt
 create mode 100644 drivers/iio/light/gp2ap020a00f.c

-- 
1.7.5.4


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH/RFC v4 0/3] gp2ap020a00f ambient light/proximity sensor
  2013-08-16 13:11 [PATCH/RFC v4 0/3] gp2ap020a00f ambient light/proximity sensor Jacek Anaszewski
@ 2013-08-18 11:19 ` Jonathan Cameron
  2013-08-19 14:50   ` Jacek Anaszewski
  0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Cameron @ 2013-08-18 11:19 UTC (permalink / raw)
  To: Jacek Anaszewski; +Cc: linux-iio, devicetree, kyungmin.park, s.nawrocki

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 interrupts
Drat, 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.

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.

>   - 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.  Clearly
this may result in needing more space in the buffer storage, but thats
still cleaner than having an explicit way to detecting that the scale
of the device has automatically changed.  We have only so far seen this
auto scaling done in light sensors and right now I think yours may be
the only one that does buffered output rather than just being polling on
sysfs (in which the conversions to lux are typically done including
any such scaling).
> 
> Besides the above I've detected that write_event_config callback
> is called even if the state to be set is already set. This poses
> an issue for me, as if it is done by design then I have to handle
> it in my driver.

The core 'could' call read_event_config to get whether the state is
already set, but there is no advantage in doing that over simply
doing the check in the driver itself that I can see.  Hence I'd
expect that logic to be in the driver. Feel free to propose otherwise
and we can see if there is a strong feeling out there about one way
or the other.

> 
> Thanks,
> Jacek Anaszewski
> 
> Jacek Anaszewski (1):
>   iio: gp2ap020a00f: Add a driver for the device
> 
>  .../devicetree/bindings/iio/light/gp2ap020a00f.txt |   20 +
>  drivers/iio/light/Kconfig                          |   12 +
>  drivers/iio/light/Makefile                         |    1 +
>  drivers/iio/light/gp2ap020a00f.c                   | 1473 ++++++++++++++++++++
>  4 files changed, 1506 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/iio/light/gp2ap020a00f.txt
>  create mode 100644 drivers/iio/light/gp2ap020a00f.c
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH/RFC v4 0/3] gp2ap020a00f ambient light/proximity sensor
  2013-08-18 11:19 ` Jonathan Cameron
@ 2013-08-19 14:50   ` Jacek Anaszewski
  2013-08-19 19:08     ` Jonathan Cameron
  0 siblings, 1 reply; 7+ messages in thread
From: Jacek Anaszewski @ 2013-08-19 14:50 UTC (permalink / raw)
  To: Jonathan Cameron; +Cc: Jacek Anaszewski, linux-iio, Kyungmin Park, s.nawrocki

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 interrupts
> Drat, 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH/RFC v4 0/3] gp2ap020a00f ambient light/proximity sensor
  2013-08-19 14:50   ` Jacek Anaszewski
@ 2013-08-19 19:08     ` Jonathan Cameron
  2013-08-20 14:52       ` Jacek Anaszewski
  0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Cameron @ 2013-08-19 19:08 UTC (permalink / raw)
  To: Jacek Anaszewski; +Cc: linux-iio, Kyungmin Park, s.nawrocki

On 08/19/13 15:50, Jacek Anaszewski wrote:
> 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 interrupts
>> Drat, 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.
Yes, that version only calls the bottom half of the interrupt handler which is clearly
not good if timestamping using the standard top half.  That's what I meant
by 'don't allow a top half'.  Sorry should have been clearer on that as the top
half bottom half naming has kind of faded away with threaded interrupts...
> 
>> 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.
That provides the triggered_buffer utility functions. When those are used
it needs to be there.

Unfortunately this isn't something I can chase down without the hardware.
It works fine in the iio_dummy driver.  Could you perhaps just build that
and check that works fine with a sysfs-trigger?  Would act to indicate
if there is something causing the issue in this driver that we haven't spotted
or something nasty is going on in the core.

> 
>>>    - 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?
Yes.
> If so, then how to inform the user what scale has been applied to
> the values in the buffer?
No need to do so given all user cares about is that the value can be
predictably converted to real world units.
> And how to provide in-driver-scaled
> output value to sysfs?
Apply it in there as well.  _raw doesn't have to be 'entirely' raw
if it is particularly handy to have it otherwise.

> 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?
Already have one : calibscale. Whilst this was originally intended
for scalings applied in device, as far as userspace is concerned this
might as well be.  We've used it for this purpose a few times before.

Jonathan
> 
> Thanks,
> Jacek
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH/RFC v4 0/3] gp2ap020a00f ambient light/proximity sensor
  2013-08-19 19:08     ` Jonathan Cameron
@ 2013-08-20 14:52       ` Jacek Anaszewski
  2013-08-20 16:09         ` Jonathan Cameron
  2013-08-20 17:18         ` Jonathan Cameron
  0 siblings, 2 replies; 7+ messages in thread
From: Jacek Anaszewski @ 2013-08-20 14:52 UTC (permalink / raw)
  To: Jonathan Cameron; +Cc: Jacek Anaszewski, linux-iio, Kyungmin Park, s.nawrocki

On 08/19/2013 09:08 PM, Jonathan Cameron wrote:

>>>>     - 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.
> That provides the triggered_buffer utility functions. When those are used
> it needs to be there.
>
> Unfortunately this isn't something I can chase down without the hardware.
> It works fine in the iio_dummy driver.  Could you perhaps just build that
> and check that works fine with a sysfs-trigger?  Would act to indicate
> if there is something causing the issue in this driver that we haven't spotted
> or something nasty is going on in the core.

I've managed to discover what is going on. The bad symptom is in the
information returned by lsmod. Before launching generic_buffer
application the "Used by" column for the driver module is 0.
After the application finishes it is 0x7fffffff.
I figured out that the problem is in the function
iio_trigger_write_current (industrialio-trigger.c:371).

If I comment lines:

if (oldtrig && indio_dev->trig != oldtrig)
	iio_trigger_put(oldtrig);

the issue ceases to appear. It seems that module_put is called too many
times.

Thanks,
Jacek

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH/RFC v4 0/3] gp2ap020a00f ambient light/proximity sensor
  2013-08-20 14:52       ` Jacek Anaszewski
@ 2013-08-20 16:09         ` Jonathan Cameron
  2013-08-20 17:18         ` Jonathan Cameron
  1 sibling, 0 replies; 7+ messages in thread
From: Jonathan Cameron @ 2013-08-20 16:09 UTC (permalink / raw)
  To: Jacek Anaszewski; +Cc: linux-iio, Kyungmin Park, s.nawrocki



Jacek Anaszewski <j.anaszewski@samsung.com> wrote:
>On 08/19/2013 09:08 PM, Jonathan Cameron wrote:
>
>>>>>     - 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.
>> That provides the triggered_buffer utility functions. When those are
>used
>> it needs to be there.
>>
>> Unfortunately this isn't something I can chase down without the
>hardware.
>> It works fine in the iio_dummy driver.  Could you perhaps just build
>that
>> and check that works fine with a sysfs-trigger?  Would act to
>indicate
>> if there is something causing the issue in this driver that we
>haven't spotted
>> or something nasty is going on in the core.
>
>I've managed to discover what is going on. The bad symptom is in the
>information returned by lsmod. Before launching generic_buffer
>application the "Used by" column for the driver module is 0.
>After the application finishes it is 0x7fffffff.
>I figured out that the problem is in the function
>iio_trigger_write_current (industrialio-trigger.c:371).
>
>If I comment lines:
>
>if (oldtrig && indio_dev->trig != oldtrig)
>	iio_trigger_put(oldtrig);
>
>the issue ceases to appear. It seems that module_put is called too many
>times.
Thanks. Will look into this... 
>
>Thanks,
>Jacek
>
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH/RFC v4 0/3] gp2ap020a00f ambient light/proximity sensor
  2013-08-20 14:52       ` Jacek Anaszewski
  2013-08-20 16:09         ` Jonathan Cameron
@ 2013-08-20 17:18         ` Jonathan Cameron
  1 sibling, 0 replies; 7+ messages in thread
From: Jonathan Cameron @ 2013-08-20 17:18 UTC (permalink / raw)
  To: Jacek Anaszewski; +Cc: linux-iio, Kyungmin Park, s.nawrocki



Jacek Anaszewski <j.anaszewski@samsung.com> wrote:
>On 08/19/2013 09:08 PM, Jonathan Cameron wrote:
>
>>>>>     - 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.
>> That provides the triggered_buffer utility functions. When those are
>used
>> it needs to be there.
>>
>> Unfortunately this isn't something I can chase down without the
>hardware.
>> It works fine in the iio_dummy driver.  Could you perhaps just build
>that
>> and check that works fine with a sysfs-trigger?  Would act to
>indicate
>> if there is something causing the issue in this driver that we
>haven't spotted
>> or something nasty is going on in the core.
>
>I've managed to discover what is going on. The bad symptom is in the
>information returned by lsmod. Before launching generic_buffer
>application the "Used by" column for the driver module is 0.
>After the application finishes it is 0x7fffffff.
>I figured out that the problem is in the function
>iio_trigger_write_current (industrialio-trigger.c:371).
>
>If I comment lines:
>
>if (oldtrig && indio_dev->trig != oldtrig)
>	iio_trigger_put(oldtrig);
>
>the issue ceases to appear. It seems that module_put is called too many
>times.

Ah.
I am clearly blind.

This is caused by setting a default trigger in the driver. None of my test drivers do that and generally it is considered a bad idea.

If there is a strong argument for doing that then we need a utility function that also does an additional iio_trigger_get.  I would rather we just dropped the setting of indio_dev->trig and left that to user space. As with having default sets of Channels enabled which we killed off during transition out of staging, it is much cleaner to make no assumptions about what the user wants.
 
Please drop that line from the driver and repost with that little bit of suggested extra text in the binding.


Sorry I should have picked up on this review before now and from your earlier comment is would guess there are a few other cases out there to be fixed up.

Thanks for chasing this down!

Jonathan

>
>Thanks,
>Jacek
>
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2013-08-20 17:18 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-16 13:11 [PATCH/RFC v4 0/3] gp2ap020a00f ambient light/proximity sensor Jacek Anaszewski
2013-08-18 11:19 ` Jonathan Cameron
2013-08-19 14:50   ` Jacek Anaszewski
2013-08-19 19:08     ` Jonathan Cameron
2013-08-20 14:52       ` Jacek Anaszewski
2013-08-20 16:09         ` Jonathan Cameron
2013-08-20 17:18         ` Jonathan Cameron

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.