by Jonathan Cameron

[permalink] [raw]

Subject: Re: [RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

On 17/12/15 23:19, Peter Rosin wrote:
> Hi Linus,
>
>> On Mon, Dec 14, 2015 at 11:38 AM, Peter Rosin <[email protected]> wrote:
>>
>> I think I atleast half-understand what you're trying to do.
>
> Good. It's really not that complicated, but I'm perhaps not describing
> it very clearly...
>
>>> Userspace does the following when doing this w/o the isr patches:
>>>
>>> 1. select signal using the MUX
>>> 2. set the DAC so high that INPUT is never reaching that level.
>>> C (and thus gpio) is now stable
>>> 3. start waiting for interrupts on gpio
>>> 4. adjust the DAC level to the level of interest
>>> 5. abort on interrupt or timeout
>> (...)
>>> With the isr patches, the above transforms into:
>>>
>>> 1. select signal using the MUX
>>> 2. set the DAC so high that INPUT is never reaching that level.
>>> C (and thus gpio) is now stable
>>> 3. read the isr bit to clear it
>>> 4. adjust the DAC level to the level of interest
>>> 5. read the isr bit again after 50ms
>>>
>>> The result is available directly in the isr bit, no interrupts needed.
>>
>> The problem I see here as kernel developer is that there is a fuzzy
>> and implicit separation of concerns between userspace and
>> kernelspace.
>>
>> IMO reading hardware registers is the domain of the kernel, and
>> then the result thereof is presented to userspace. The kernel
>> handles hardware, simply.
>>
>> I think we need to reverse out of this solution and instead ask
>> the question what the kernel should provide your userspace.
>> Maybe parts of what you have in userspace needs to actually
>> go into the kernel?
>>
>> The goal of IIO seems to be to present raw and calibrated
>> (SI unit) data to userspace. So what data is it you want, and
>> why can't you just get that directly from the kernel without
>> tricksing around with reading registers bits in userspace?
>
> This all makes sense. The reason is that I'm not familiar with
> the kernel APIs. I have to wrap my head around how to set up
> work to be performed later, etc etc. All of that is in all
> likelihood pretty straightforward, but I feel that I am
> flundering around every step of the way. End result; I find
> myself trying to do as little as possible inside the kernel.
>
> I.e. I have a pretty clear picture of what needs to be done, but
> the devil is in the details...
>
>> If a kernel module needs to read an interrupt bit directly from the
>> GPIO controller is another thing. That is akin to how polling
>> network drivers start polling registers for incoming packages
>> instead of waiting for them to fire interrupts. Just that these are
>> dedicated IRQ lines, not GPIOs.
>
> Yes, there was also the NACK to adding new gpio sysfs files which
> emphasizes this.
>
>> As struct irq_chip has irq_get_irqchip_state() I think this
>> is actually possible to achieve this by implementing that
>> and calling irq_get_irqchip_state().
>
> I'll have a peek into that, but see below.
>
>>> I have realized that I could work with one-shot-interrupts. Then the
>>> urgency to disable interrupts go away, as only one interrupt would
>>> be served. That was not my immediate solution though, as I have been
>>> using isr type registers in this way several times before.
>>
>> That sounds like the right solution. With ONESHOT a threaded IRQ
>> will have its line masked until you return from the ISR thread. Would this
>> work for your usecase?
>
> The ISR thread would need to be able to disable further interrupts
> before it returned, is that possible without deadlock? If so, it's
> a good fit.
>
>> I suspect this require you to move the logic into the kernel? Into IIO?
> bc
> Yes, and yes IIO seems about right to me too.
Whilst the full approach could well be done in IIO I can also see
some possible demand for a simple latching gpio which is I think all you
were doing with the ISR stuff.

Seems a sensible interface to support in some fashion even aside from this
discussion. Devices like PLCs do this stuff all the time though usually
in conjunction with a capture unit that will stash a copy of some
associated counter...

>
>>> If this is to be done in IIO, I imagine that the sanest thing would
>>> be to integrate the whole DAC-loop and present a finished envelope
>>> value to user space? This envelope detector would have to be pretty
>>> configurable, or it will be next to useless to anybody but me.
>>
>> Makes sense to me, but must be ACKed by Jonathan. But it
>> sounds pretty cool does it not?
>
> Right, but I don't see why it should be a problem? An envelope detector
> surely fits IIO.
Sure - it's within scope I think, but there is probably a fair bit of
new interface needed to control it..
>
>>> I could imaging that this new IIO driver should be able to work
>>> with any DAC type device, which in my case would be the mcp4531
>>> dpot. Which is not a DAC, maybe that could be solved with a new
>>> dac-dpot driver, applicable to cases where a dpot is wired as a
>>> simple voltage divider? The new IIO driver also needs to know how
>>> to get a reading from the comparator. I think the driver should
>>> support having a latch between the comparator and the gpio, so it
>>> need to know how to optionally "reset the comparator". That
>>> would have solved the whole problem, you would never have seen
>>> any of this if I had such a latch on my board. But the isr
>>> register is a latch, so...
>>>
>>> Regardless, I think such a driver still needs support from gpio
>>> and/or pinctrl. Either exposing the isr register or supporting
>>> one-shot-interrupts that disarm themselves before restoring the
>>> processor interrupt flag (maybe that exists?).
>>
>> All irqchips support one-shot interrupts as long as you request a
>> threaded IRQ I think.
>>
>> Your GPIO driver must support IRQs though but AT91 surely does.
>> Maybe you will need to implement irq_get_irqchip_state() on it
>> if you insist on polling the interrupt.
>
> If I get the oneshot irqs to work, that indeed seems like the better
> and more general solution.
>
>>> Otherwise the
>>> core problem remains unsolved. Also, this imaginary IIO driver
>>> probably have to be totally oblivious of the MUX, or the number
>>> of possibilities explode.
>>
>> Usually we try to follow the UNIX philisophy to do one thing
>> and do it good.
>>
>> You haven't said much about how that MUX works, if it's another
>> userspace ThingOfABob or what it is. There is no generic
>> kernel framework for muxes so I see why people would want to
>> drive that using userspace GPIO lines for example.
>>
>> If it is pinmuxing in a standard chip of some kind, pinctrl
>> handles it.
>
> I'm afraid it's currently done from userspace with gpio-sysfs. If
> that's not changed, I need userspace to control *when* the kernel
> performs the envelope detector logic.
Definitely a case of doing this in stages. Obviously you can
do some of the following in a different order!

1) Add a simple general purpose IIO driver to read gpios.
2) Add latching support to that - so rather than getting the current
value allow it to (if possible) support setting up a 'pseudo'
interrupt using the stuff Linus pointed you to above.
You may need an explicit 'start now' signal - not sure.
3) Get your application running from userspace as:
a) Configure your IIO 'latching gpio driver' and probably
read it to force a reset.
b) Set DAC value
c) Wait a bit
d) read from sysfs iio interface (can move to buffered stuff later)
e) Set new Dac value based on result
f) read here to burn any value set in between
g) wait a bit
h) read a gain
etc.

4) Look at tying stuff together in kernel - driving towards your full
setup. Ignoring mux for now this might look like.

A) An overarching driver that is a 'consumer' of both the dac and the
latching gpio drivers.
Latches onto the consumer interfaces of both and drives them in sync,
in first instance probably using the in kernel equivalent of the sysfs
interfaces. (can work out the nice fast version later using buffered
interfaces ;)

B) Associated gpio capture driver as described above
C) Associated DAC driver.

Actually now I think about it, to get this first version up should be
pretty straight forward. The most irritating bit will be working out
how to tie the various drivers together in a generic way. Probably
something for the new configfs interfaces where you would create an instance
of the overarching driver, associate the dac and gpio interfaces then
set some 'instantiate' attribute to true to bring it all up.

Anyhow, sounds fun. I've been mulling over a gpio based DIO driver
for IIO for a while (be it without the latching stuff). The interface
stuff will also be handy for devices with general purpose inputs
alongside their ADC type ones (e.g. ADIS IMUs tend to have a couple
that can be read in the main data burst modes I think).

>
> Thanks for your feedback! I think I have enough info to get
> started. Now I just need to find the time...
Know the feeling but it's always slightly easier when it is fun
and I think this sounds like it should be!

Good luck

Jonathan
>
> Cheers,
> Peter
> N��r��y��b�X��ǧv�^�)޺{.n�+��{��*"��^n�r��z��h��&��G��h�(�階�ݢj"��m��z�ޖ��f��h��~�mml==
>

2015-12-22 08:44:33

by Linus Walleij

[permalink] [raw]

Subject: Re: [RESEND RFC PATCH 0/2] Expose the PIO_ISR register on SAMA5D3

On Fri, Dec 18, 2015 at 12:19 AM, Peter Rosin <[email protected]> wrote:

> This all makes sense. The reason is that I'm not familiar with
> the kernel APIs. I have to wrap my head around how to set up
> work to be performed later, etc etc.

FWIW a random work to be performed later is a delayed work.
It can be queued on a global workqueue or you can create your
own workqueue. This is the latter way, it's even simpler if you
just use NULL as workqueue, it will then end up on the big
system workqueue. The workqueue in turn uses deferrable timers,
and these can also be used directly.

#include <linux/workqueue.h>

struct foo {
struct workqueue_struct *foo_wq;
struct delayed_work delayed_foo_work;
....
};

static void foo_work_cb(struct work_struct *work)
{
struct foo *foo = container_of(work, struct foo, delayed_foo_work.work);

... do the work ...
};

main {
INIT_DEFERRABLE_WORK(&foo->delayed_foo_work, foo_work_cb);
queue_delayed_work(foo->foo_wq, &foo->delayed_foo_work, 6*HZ);
}

6*HZ means wait for 6 seconds as the delay is given in ticks (jiffies)
and the system does HZ ticks per second.

>> > I have realized that I could work with one-shot-interrupts. Then the
>> > urgency to disable interrupts go away, as only one interrupt would
>> > be served. That was not my immediate solution though, as I have been
>> > using isr type registers in this way several times before.
>>
>> That sounds like the right solution. With ONESHOT a threaded IRQ
>> will have its line masked until you return from the ISR thread. Would this
>> work for your usecase?
>
> The ISR thread would need to be able to disable further interrupts
> before it returned, is that possible without deadlock? If so, it's
> a good fit.

Yes that is what the ONESHOT flag means. So like this:

ret = request_threaded_irq(irqnum, NULL, foo_irq_handler,
IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
"foo", foo);

To register a threaded oneshot IRQ that will run on a falling edge,
as a thread, until completed, masking its own interrupt line, but
no others.

The threaded handler can msleep(), mdelay()/udelay() etc since it
is a thread. usleep_range(a, b) is the best as the system can idle
while that happens, and can plan for a good wakeup point.

Yours,
Linus Walleij