Message ID | 20181008101356.5672-1-uwe@kleine-koenig.org |
---|---|
State | RFC, archived |
Headers | show |
Series | [RFC] gpio: new driver for a gpio simulator | expand |
Hello, On Mon, Oct 08, 2018 at 12:13:56PM +0200, Uwe Kleine-König wrote: > + struct irq_domain *irqdomain; > + unsigned int irq; > + > + irq_domain = ddata->port[1].gchip.irq.domain; > + irq = irq_find_mapping(irqdomain, offset); > + > + generic_handle_irq(irq); while wrapping long lines I introduced a bug here. s/irq_domain/irqdomain/ to fix it. I'll wait for some feedback before resending. Best regards Uwe
Hi Uwe, thanks for this interesting patch! On Mon, Oct 8, 2018 at 12:14 PM Uwe Kleine-König <uwe@kleine-koenig.org> wrote: > A gpio simulator device provides 2x 32 virtual GPIOs that are pairwise > connected. > > That is, if both GPIOs are configured as input both read a zero. If one > is an output the other reads the first's output value. > > This can for example be used to add a push button device on one side and > "push" it by driving the other side via gpioctl. > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> > --- > Hello, > > this is a patch that I intend to use to test a few patches that are > still on my todo list. > > Do you consider it interesting enough to be suitable for mainline? > > There is one issue (that I'm aware of): If the driver is unbound (via > sysfs) I get warnings that the gpios and/or irqs are still in use. Not > sure how to fix this properly. I would run this by Bartosz who maintains the gpio-mockup.c driver. I have basically two questions: 1. When compared to gpio-mockup.c, what features does this simulator offer that gpio-mockup doesn't already have? 2. Would it be possible to extend gpio-mockup.c to cover your usecases instead of introducing another unreal GPIO device? Vincent recently posted patches to even enable device tree probing of the mockup device, indicating that there is already some industrial use of that driver for prototyping. Yours, Linus Walleij
Hello Linus, On Tue, Oct 09, 2018 at 02:51:37PM +0200, Linus Walleij wrote: > On Mon, Oct 8, 2018 at 12:14 PM Uwe Kleine-König <uwe@kleine-koenig.org> wrote: > > > A gpio simulator device provides 2x 32 virtual GPIOs that are pairwise > > connected. > > > > That is, if both GPIOs are configured as input both read a zero. If one > > is an output the other reads the first's output value. > > > > This can for example be used to add a push button device on one side and > > "push" it by driving the other side via gpioctl. > > > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> > > --- > > Hello, > > > > this is a patch that I intend to use to test a few patches that are > > still on my todo list. > > > > Do you consider it interesting enough to be suitable for mainline? > > > > There is one issue (that I'm aware of): If the driver is unbound (via > > sysfs) I get warnings that the gpios and/or irqs are still in use. Not > > sure how to fix this properly. > > I would run this by Bartosz who maintains the gpio-mockup.c > driver. Oh, I wasn't aware of that driver. > I have basically two questions: > > 1. When compared to gpio-mockup.c, what features does this > simulator offer that gpio-mockup doesn't already have? I like my driver better because the interface to drive (or read) a simulated gpio is more natural (but of course I'm not objective here). I only read through the mockup driver quickly but I wonder if/how it supports different irq sensitivities. I saw there are some tools to work with such a mockup device. For my gpio-simulator the tools used to interact with gpios are directly suitable which is a nice result of the simulator design. A nice property of the symmetry of gpio-simulator is that both ends are available in the kernel. So you could even use it to connect an i2c-gpio instance to a i2c-slave-gpio instance (though this would need to add support for pullups instead of the currently hard-coded pull-down). It already offers device tree probing and I successfully added a rotary encoder device to it. Currently it is not yet supported, but with the API of gpio-simulator it should be trivially possible, to atomically set gpios from userspace which won't work with the debugfs files used by gpio-mockup. An advantage of gpio-mockup over gpio-simulator I noticed by reading is that it already supports atomic setting in the direction to userspace. Also the number of gpios isn't fixed. (But 64 GPIOs should be enough for everybody :-) A bit unrelated: I would probably have noticed the mockup driver if it were not hidden in the section "Memory mapped GPIO drivers". > 2. Would it be possible to extend gpio-mockup.c to cover your > usecases instead of introducing another unreal GPIO device? Sure, I could change the interface from debugfs to two gpio ports that behave like gpio-simulator :-) The motivation to create gpio-simulator was to have a nice test case for the rotary-encoder driver and for that it is crucial to be able to set gpios atomically (in the direction that isn't possible for mockup) to test quick turning. > Vincent recently posted patches to even enable device tree > probing of the mockup device, indicating that there is already > some industrial use of that driver for prototyping. My gpio-simulator has dt support, too, but it's more a private project of me without industrial use (yet). Best regards Uwe
On Tue, Oct 9, 2018 at 9:11 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > Currently it is not yet supported, but with the API of gpio-simulator it > should be trivially possible, to atomically set gpios from userspace > which won't work with the debugfs files used by gpio-mockup. Hm. That seems like a feature we might want in the mockup driver then. > An advantage of gpio-mockup over gpio-simulator I noticed by reading is > that it already supports atomic setting in the direction to userspace. > Also the number of gpios isn't fixed. (But 64 GPIOs should be enough for > everybody :-) > > A bit unrelated: I would probably have noticed the mockup driver if it > were not hidden in the section "Memory mapped GPIO drivers". This should be fixed. > > 2. Would it be possible to extend gpio-mockup.c to cover your > > usecases instead of introducing another unreal GPIO device? > > Sure, I could change the interface from debugfs to two gpio ports that > behave like gpio-simulator :-) Can't we do both... maybe I just don't get it here. Certainly gpio-mockup will give you the B side, there is a gpiochip that appears after all as a result of probing the driver. What you want is to add a second gpiochip that can be used to stimulate it. Maybe that is as simple as a module parameter or Kconfig option to also create a controlling port. I would prefer to have one GPIO-mockup/fake/simulator thing instead of several, so we can focus efforts. It's good for prototyping and testing alike. > The motivation to create gpio-simulator > was to have a nice test case for the rotary-encoder driver and for that > it is crucial to be able to set gpios atomically (in the direction that > isn't possible for mockup) to test quick turning. This is a valid prototyping usecase. As is Vincent's. > > Vincent recently posted patches to even enable device tree > > probing of the mockup device, indicating that there is already > > some industrial use of that driver for prototyping. > > My gpio-simulator has dt support, too, but it's more a private project > of me without industrial use (yet). It's pretty easy for me to see the general utility, so I would ideally like to incorporate these things into one and the same simulator/mockup driver. Yours, Linus Walleij
On Wed, Oct 10, 2018 at 01:47:42PM +0200, Linus Walleij wrote: > On Tue, Oct 9, 2018 at 9:11 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > Currently it is not yet supported, but with the API of gpio-simulator it > > should be trivially possible, to atomically set gpios from userspace > > which won't work with the debugfs files used by gpio-mockup. > > Hm. That seems like a feature we might want in the mockup > driver then. > > > An advantage of gpio-mockup over gpio-simulator I noticed by reading is > > that it already supports atomic setting in the direction to userspace. > > Also the number of gpios isn't fixed. (But 64 GPIOs should be enough for > > everybody :-) > > > > A bit unrelated: I would probably have noticed the mockup driver if it > > were not hidden in the section "Memory mapped GPIO drivers". > > This should be fixed. > > > > 2. Would it be possible to extend gpio-mockup.c to cover your > > > usecases instead of introducing another unreal GPIO device? > > > > Sure, I could change the interface from debugfs to two gpio ports that > > behave like gpio-simulator :-) > > Can't we do both... maybe I just don't get it here. This would complicate things because then there are two controllers of the same resource with all the resulting effects. We'd need to make sure that only one of the two controllers can be active. It's not hard, but I see little use. > Certainly gpio-mockup will give you the B side, there is > a gpiochip that appears after all as a result of probing the > driver. What you want is to add a second gpiochip that can > be used to stimulate it. > > Maybe that is as simple as a module parameter or > Kconfig option to also create a controlling port. > > I would prefer to have one GPIO-mockup/fake/simulator > thing instead of several, so we can focus efforts. > > It's good for prototyping and testing alike. > > > The motivation to create gpio-simulator > > was to have a nice test case for the rotary-encoder driver and for that > > it is crucial to be able to set gpios atomically (in the direction that > > isn't possible for mockup) to test quick turning. > > This is a valid prototyping usecase. As is Vincent's. @Vincent: What is your usecase? I currently cannot imagine a use case that can be done with the mockup driver but not with the simulator. The conceptual difference is just that mockup uses files in debugfs to control the "inner side" while the simulator uses another set of gpios. The advantages of the latter are that you could wire both sides to otherwise unaware drivers (though this is a bit theoretical as there is currently no use case I'm aware of that already exists) and it works with the tools that might already exist to work on real hardware. Also it is dogfooding which is nice. So if you ask me the conceptually right solution is to use the gpio-simulator and throw away the mockup driver. The only downside I see here is that users of mockup have to adapt. I didn't look into the tool for mockup yet, but probably they can be easily adapted to work with the simulator. But maybe I missed the killer feature of the mockup driver so feel free to prove me wrong. Best regards Uwe
On Thu, Oct 11, 2018 at 10:16:46AM +0200, Uwe Kleine-König wrote: > @Vincent: What is your usecase? I currently cannot imagine a use case > that can be done with the mockup driver but not with the simulator. I just needed a fake GPIO chip with a bunch of pins that I could use before I got the hardware with a real GPIO expander. In my case I only needed to test output so IIRC I didn't use the debugfs interface at all but instead used ftrace to check that my userspace triggered the correct operations on the "pins". So, yes, that would probably have worked just as well with this simulator instead of the mockup driver.
Since I was just evaluating the mockup driver to use in a test setup I'll give my 2 cents. The benefit of the simulator is that we get the possibility to, if I understand correctly, monitor an input for changes on the connected output. Sure, we could also use ftrace but I like the idea of having the two connected GPIOs. As a matter of fact I was just thinking about implementing something that gave the same functionality for the mockup driver. What I like from the mockup driver is the possibility to have *many* gpios on the same chip. We currently have a MCU I/O expander that gives us 110+ gpios. So for us 64 GPIOs is really not enough. So neither driver works out of the box for us right now but both looks very promising. Best Regards Einar
Hello Einar, On Fri, Oct 12, 2018 at 08:02:09AM +0000, Einar Vading wrote: > Since I was just evaluating the mockup driver to use in a test setup > I'll give my 2 cents. very welcome, thanks. > The benefit of the simulator is that we get the possibility to, if I > understand correctly, monitor an input for changes on the connected > output. Sure, we could also use ftrace but I like the idea of having > the two connected GPIOs. As a matter of fact I was just thinking about > implementing something that gave the same functionality for the mockup > driver. > > What I like from the mockup driver is the possibility to have *many* > gpios on the same chip. We currently have a MCU I/O expander that > gives us 110+ gpios. So for us 64 GPIOs is really not enough. Currently several states are stored in an u32 where each bit corresponds to a GPIO which simplifies some operations. Probably an array of unsigned long could be used, which would have the benefit that set_bit et al would work. Then we either need a compile time limit, a variadic driver data struct or one additional allocation for each variable that is an u32 currently. Would it help to instanciate more than one gpio-simulator? Best regards Uwe
On Fri, Oct 12, 2018 at 11:06:12AM +0200, Uwe Kleine-König wrote:
> Would it help to instanciate more than one gpio-simulator?
Hmm, I don't think that would pose a problem. With DT it would be easy to name
the GPIOs and get then get them by name. What gpiochip they are on should not
matter... I think.
Best Regards
Einar
On Fri, Oct 12, 2018 at 11:27:18AM +0200, Einar Vading wrote: > On Fri, Oct 12, 2018 at 11:06:12AM +0200, Uwe Kleine-König wrote: > > Would it help to instanciate more than one gpio-simulator? > > Hmm, I don't think that would pose a problem. With DT it would be easy to name > the GPIOs and get then get them by name. What gpiochip they are on should not > matter... I think. The only downside is that you cannot atomically set GPIOs on different chips. I didn't try to use naming, but maybe the gpio framework is good enough that it just works. Best regards Uwe
pt., 12 paź 2018 o 11:47 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> napisał(a): > > On Fri, Oct 12, 2018 at 11:27:18AM +0200, Einar Vading wrote: > > On Fri, Oct 12, 2018 at 11:06:12AM +0200, Uwe Kleine-König wrote: > > > Would it help to instanciate more than one gpio-simulator? > > > > Hmm, I don't think that would pose a problem. With DT it would be easy to name > > the GPIOs and get then get them by name. What gpiochip they are on should not > > matter... I think. > > The only downside is that you cannot atomically set GPIOs on different > chips. I didn't try to use naming, but maybe the gpio framework is good > enough that it just works. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ | Sorry for the late reply, I was travelling last week. If I understand correctly the main difference between gpio-mockup and gpio-simulator is that simulating interrupts on input lines would happen by changing the value of connected output GPIO lines. I started using gpio-mockup for testing of both libgpiod and GPIO user API some time ago. I initially thought about doing the exact same thing with interrupts but figured that if we want to test the user API then we'd better not be actually using it since we won't know where the eventual bugs will actually come from - i.e. when testing reading line events, let's not be using the output API from the other side, but rather something not linked to GPIO in any way - debugfs in this case. That being said: I have nothing against extending gpio-mockup with this feature - I actually like it. I guess having a single module param that would create a second corresponding gpiochip for every standard one would be enough? I could have the numbering starting right after the standard chips and a special label too. However I don't really see a need to have two separate testing drivers for a single subsystem. Is there anything in the way mockup is implemented that stops you from reusing it? Please note that libgpiod extensively uses gpio-mockup for testing so there's now way we're getting rid of it anytime soon. Best regards, Bartosz Golaszewski
On Mon, Oct 15, 2018 at 11:54:22AM +0200, Bartosz Golaszewski wrote: > pt., 12 paź 2018 o 11:47 Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> napisał(a): > > > > On Fri, Oct 12, 2018 at 11:27:18AM +0200, Einar Vading wrote: > > > On Fri, Oct 12, 2018 at 11:06:12AM +0200, Uwe Kleine-König wrote: > > > > Would it help to instanciate more than one gpio-simulator? > > > > > > Hmm, I don't think that would pose a problem. With DT it would be easy to name > > > the GPIOs and get then get them by name. What gpiochip they are on should not > > > matter... I think. > > > > The only downside is that you cannot atomically set GPIOs on different > > chips. I didn't try to use naming, but maybe the gpio framework is good > > enough that it just works. > > If I understand correctly the main difference between gpio-mockup and > gpio-simulator is that simulating interrupts on input lines would > happen by changing the value of connected output GPIO lines. Not only for input lines. What is done with gpio-mockup via debugfs is done for gpio-simulator with the gpio functions on the other side. > I started using gpio-mockup for testing of both libgpiod and GPIO user > API some time ago. I initially thought about doing the exact same > thing with interrupts but figured that if we want to test the user API > then we'd better not be actually using it since we won't know where > the eventual bugs will actually come from - i.e. when testing reading > line events, let's not be using the output API from the other side, > but rather something not linked to GPIO in any way - debugfs in this > case. I wouldn't take this as a reason to not use GPIO stuff for the "other side". On the contrary, I'd see it as an advantage as this way you test setting an output on one side and generating an event on the other side in a single test. And if it's broken and needs debugging there are the normal trace mechanisms. > That being said: I have nothing against extending gpio-mockup with > this feature - I actually like it. I guess having a single module > param that would create a second corresponding gpiochip for every > standard one would be enough? I could have the numbering starting > right after the standard chips and a special label too. However I > don't really see a need to have two separate testing drivers for a > single subsystem. Is there anything in the way mockup is implemented > that stops you from reusing it? An advantage of my simulator over gpio-mockup (as mentioned already earlier) is that both sides are available as GPIOs in the kernel. So I could connect a rotary-encoder input device to an device that mimics a rotary-encoder. > Please note that libgpiod extensively uses gpio-mockup for testing so > there's now way we're getting rid of it anytime soon. >From my POV in order of preference, we'd do the following: a) take gpio-simulator in parallel to gpio-mockup b) take gpio-simulator and drop gpio-mockup c) merge the two I think c) is ugly because it complicates the combined driver which takes away much of the beauty and simplicity I see in at least the gpio-simulator. (I don't know the mockup driver good enough to judge here, but on my few looks it looked more complicated which might be subjective.) Also there are a few issues I see in the mockup driver (which are not implied by the architecture and so are fixable): - I think it's racy because there is no locking (ditto for the irq simulator). - Each write to the event file generates an irq, so there is no way to test level sensitive irqs. - SPDX header claims GPL-2.0+, MODULE_LICENCE claims GPL-v2 only. - The debugfs interface doesn't give complete control over the gpios. Of course there is no *need* to have two virtual gpio devices, but I don't see a big reason not to have two anyhow. Best regards Uwe
pon., 15 paź 2018 o 22:03 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> napisał(a): > > On Mon, Oct 15, 2018 at 11:54:22AM +0200, Bartosz Golaszewski wrote: > > pt., 12 paź 2018 o 11:47 Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> napisał(a): > > > > > > On Fri, Oct 12, 2018 at 11:27:18AM +0200, Einar Vading wrote: > > > > On Fri, Oct 12, 2018 at 11:06:12AM +0200, Uwe Kleine-König wrote: > > > > > Would it help to instanciate more than one gpio-simulator? > > > > > > > > Hmm, I don't think that would pose a problem. With DT it would be easy to name > > > > the GPIOs and get then get them by name. What gpiochip they are on should not > > > > matter... I think. > > > > > > The only downside is that you cannot atomically set GPIOs on different > > > chips. I didn't try to use naming, but maybe the gpio framework is good > > > enough that it just works. > > > > If I understand correctly the main difference between gpio-mockup and > > gpio-simulator is that simulating interrupts on input lines would > > happen by changing the value of connected output GPIO lines. > > Not only for input lines. What is done with gpio-mockup via debugfs is > done for gpio-simulator with the gpio functions on the other side. > > > I started using gpio-mockup for testing of both libgpiod and GPIO user > > API some time ago. I initially thought about doing the exact same > > thing with interrupts but figured that if we want to test the user API > > then we'd better not be actually using it since we won't know where > > the eventual bugs will actually come from - i.e. when testing reading > > line events, let's not be using the output API from the other side, > > but rather something not linked to GPIO in any way - debugfs in this > > case. > > I wouldn't take this as a reason to not use GPIO stuff for the "other > side". On the contrary, I'd see it as an advantage as this way you test > setting an output on one side and generating an event on the other side > in a single test. And if it's broken and needs debugging there are the > normal trace mechanisms. > > > That being said: I have nothing against extending gpio-mockup with > > this feature - I actually like it. I guess having a single module > > param that would create a second corresponding gpiochip for every > > standard one would be enough? I could have the numbering starting > > right after the standard chips and a special label too. However I > > don't really see a need to have two separate testing drivers for a > > single subsystem. Is there anything in the way mockup is implemented > > that stops you from reusing it? > > An advantage of my simulator over gpio-mockup (as mentioned already > earlier) is that both sides are available as GPIOs in the kernel. So I > could connect a rotary-encoder input device to an device that mimics a > rotary-encoder. > Sure! > > Please note that libgpiod extensively uses gpio-mockup for testing so > > there's now way we're getting rid of it anytime soon. > > From my POV in order of preference, we'd do the following: > > a) take gpio-simulator in parallel to gpio-mockup > b) take gpio-simulator and drop gpio-mockup > c) merge the two > > I think c) is ugly because it complicates the combined driver which > takes away much of the beauty and simplicity I see in at least the > gpio-simulator. (I don't know the mockup driver good enough to judge > here, but on my few looks it looked more complicated which might be > subjective.) > Every single developer likes to start from scratch and implement his own ideal solution but that's usually the worst decision[1]. Also: it's harder to read code than to write it. The mockup driver works fine. It's also reasonably clean as far as code goes. I didn't write it from scratch either - it had existed for some time before I took it over and made it into something I could use with libgpiod. Check git blame. > Also there are a few issues I see in the mockup driver (which are not > implied by the architecture and so are fixable): > > - I think it's racy because there is no locking (ditto for the irq > simulator). Yes, it's fixable - I didn't need locking since usually there's only one user at any given time. For irq_sim - I don't think we need any locking here but I may take a second look at the irq subsystem. > - Each write to the event file generates an irq, so there is no way to > test level sensitive irqs. Can't it be fixed by extending the driver with your solution? > - SPDX header claims GPL-2.0+, MODULE_LICENCE claims GPL-v2 only. > - The debugfs interface doesn't give complete control over the gpios. > Good point. The license boilerplate before SPDX also says GPL-2.0 or later. I guess we need to fix MODULE_LICENSE. > Of course there is no *need* to have two virtual gpio devices, but I > don't see a big reason not to have two anyhow. > I do believe that having a single driver will cause less confusion in the future and make it less likely that someone needing another testing future will try to get merged a third separate driver. Linus will have the last word of course but personally I'd like to work towards extending gpio-mockup. Best regards, Bartosz Golaszewski > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ | [1] https://www.joelonsoftware.com/2000/04/06/things-you-should-never-do-part-i/
Hello Bartosz, On Thu, Oct 18, 2018 at 05:03:42PM +0200, Bartosz Golaszewski wrote: > > > Please note that libgpiod extensively uses gpio-mockup for testing so > > > there's now way we're getting rid of it anytime soon. > > > > From my POV in order of preference, we'd do the following: > > > > a) take gpio-simulator in parallel to gpio-mockup > > b) take gpio-simulator and drop gpio-mockup I'd like to add here between b) and c): don't mainline gpio-simulator > > c) merge the two > > > > I think c) is ugly because it complicates the combined driver which > > takes away much of the beauty and simplicity I see in at least the > > gpio-simulator. (I don't know the mockup driver good enough to judge > > here, but on my few looks it looked more complicated which might be > > subjective.) > > Every single developer likes to start from scratch and implement his > own ideal solution but that's usually the worst decision[1]. Also: > it's harder to read code than to write it. The mockup driver works > fine. It's also reasonably clean as far as code goes. I didn't write > it from scratch either - it had existed for some time before I took > it over and made it into something I could use with libgpiod. Check > git blame. I don't agree completely to what you (and Joel) say here. Sometimes it's sensible to adapt stuff, but if it's already clear from the start that in the end the result isn't recognizable or the merged work is more ugly or harder to maintain than the two unmerged ones, I'd say having two is just fine. Maybe it's doable, but it won't be me who does it. > > Also there are a few issues I see in the mockup driver (which are not > > implied by the architecture and so are fixable): > > > > - I think it's racy because there is no locking (ditto for the irq > > simulator). > > Yes, it's fixable - I didn't need locking since usually there's only > one user at any given time. For irq_sim - I don't think we need any > locking here but I may take a second look at the irq subsystem. If two CPUs call irq_sim_fire() with different offsets they fight for assigning sim->work_ctx.irq. One looses and the respective irq doesn't fire and maybe the winning one fires twice. The thing here is: Yes, it works most of the time. But only most. And if your libgpiod isn't tested with quick sequences that might race against each other, I'd call that a gap in your test setup. > > - Each write to the event file generates an irq, so there is no way to > > test level sensitive irqs. > > Can't it be fixed by extending the driver with your solution? Not really. The debugfs interface would need fixing, too. Otherwise you can trigger a GPIO that is supposed to trigger an irq on a falling edge by writing a 1 to its debugfs file. Probably it's easier to add the debugfs interface to the gpio-simulator than to add the gpio-interface to the gpio-mockup driver. Then opening the debugfs file could request the GPIO, writing a value would result in gpio_set_value() and closing in gpio_free(). But then you have a changed behaviour compared to gpio-mockup and there will be three interfaces to the GPIOs (/dev/gpiochip, /sys/class/gpio/ and the debugfs) which is IMHO worse than having gpio-simulator and gpio-mockup side by side. > > Of course there is no *need* to have two virtual gpio devices, but I > > don't see a big reason not to have two anyhow. > > I do believe that having a single driver will cause less confusion in > the future and make it less likely that someone needing another > testing future will try to get merged a third separate driver. Linus > will have the last word of course but personally I'd like to work > towards extending gpio-mockup. I won't argue here. Iff you think gpio-simulator is good to take without merging with gpio-mockup I'm willing to work on the (other) identified weaknesses. Best regards Uwe
On Thu, Oct 18, 2018 at 9:16 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > [Bartosz] > > I do believe that having a single driver will cause less confusion in > > the future and make it less likely that someone needing another > > testing future will try to get merged a third separate driver. Linus > > will have the last word of course but personally I'd like to work > > towards extending gpio-mockup. > > I won't argue here. Iff you think gpio-simulator is good to take without > merging with gpio-mockup I'm willing to work on the (other) identified > weaknesses. We just don't want to have to maintain two synthetic drivers for this. libgpiod has an extensive set of tests: https://github.com/brgl/libgpiod/tree/master/tests If gpio-simulator should replace gpio-mockup, all it needs to do is pass all these tests without changes. That is the beauty of test driven development. However I would have a serious allergic reaction to any "merge this now fix any tests later" or "not my problem to pass these tests" approach. A slot-in replacement doing all that gpio-mockup does and then some would likely be accepted on the condition that gpio-mockup is deleted at the same time, as that would prove it is a bigger and better swiss army knife for GPIO simulation/mockuping. Yours, Linus Walleij
On Tue, Oct 30, 2018 at 01:45:29PM +0100, Linus Walleij wrote: > On Thu, Oct 18, 2018 at 9:16 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > [Bartosz] > > > I do believe that having a single driver will cause less confusion in > > > the future and make it less likely that someone needing another > > > testing future will try to get merged a third separate driver. Linus > > > will have the last word of course but personally I'd like to work > > > towards extending gpio-mockup. > > > > I won't argue here. Iff you think gpio-simulator is good to take without > > merging with gpio-mockup I'm willing to work on the (other) identified > > weaknesses. > > We just don't want to have to maintain two synthetic drivers for this. > > libgpiod has an extensive set of tests: > https://github.com/brgl/libgpiod/tree/master/tests > > If gpio-simulator should replace gpio-mockup, all it needs to do > is pass all these tests without changes. That is the beauty of > test driven development. Just from a quick glance, it won't be able to pass without modifications (which I think should be fine). The place that won't work is event_worker() which hardcodes some things specific to the mockup driver (i.e. the sysfs path). > However I would have a serious allergic reaction to any > "merge this now fix any tests later" or "not my problem to > pass these tests" approach. I agree here. gpio-mockup shouldn't be deleted if that breaks libgpiod. > A slot-in replacement doing all that gpio-mockup does and then some > would likely be accepted on the condition that gpio-mockup is deleted > at the same time, as that would prove it is a bigger and better swiss > army knife for GPIO simulation/mockuping. Not sure I find the time to work on this in the near future. Best regards Uwe
diff --git a/Documentation/devicetree/bindings/gpio/gpio-simulator.txt b/Documentation/devicetree/bindings/gpio/gpio-simulator.txt new file mode 100644 index 000000000000..4540b656f7c6 --- /dev/null +++ b/Documentation/devicetree/bindings/gpio/gpio-simulator.txt @@ -0,0 +1,49 @@ +GPIO simulator +============== + +This is a completely simulated device that connects the 32 GPIOs of one side +pairwise to the 32 GPIOs on the other side such that controlling a GPIO changes +the level for the matching GPIO on the other side. In hardware such a pair +would look as follows: + + ,-------. ,-------. + portA GPIO i ] ---| 100 Ω |----+----| 100 Ω |---[ portB GPIO i + `-------' | `-------' + ,---. + | 1 | + | 0 | + | k | + | Ω | + `---' + | + ⏚ + +Required properties +------------------- + +- compatible: "gpio-simulator" + +The node is expected to have two sub-nodes called "sidea" and "sideb" that +implement a gpio-controller and an interrupt-controller with 2 cells each. + + +Example: + gpiosim { + compatible = "gpio-simulator"; + + gpiosima: sidea { + gpio-controller; + #gpio-cells = <2>; + + interrupt-controller; + #interrupt-cells = <2>; + }; + + gpiosimb: sideb { + gpio-controller; + #gpio-cells = <2>; + + interrupt-controller; + #interrupt-cells = <2>; + }; + }; diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 4f52c3a8ec99..1db13cc4bedb 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1387,4 +1387,16 @@ config GPIO_VIPERBOARD endmenu +config GPIO_SIMULATOR + tristate "GPIO simulator" + help + This provides a driver for a virtual device that exposes two banks + with 32 GPIOs each. + The virtual lines are pairwise connected, so setting the first GPIO + on the first bank as output and toggling its value makes the first GPIO + on the second bank (configured as input) change the level + accordingly. This way you can attach (say) a GPIO button device to + one of the GPIOs and "press" it by using /dev/gpioctl on the + companion GPIO. + endif diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index c256aff66a65..3560c656a984 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -111,6 +111,7 @@ obj-$(CONFIG_GPIO_REG) += gpio-reg.o obj-$(CONFIG_ARCH_SA1100) += gpio-sa1100.o obj-$(CONFIG_GPIO_SCH) += gpio-sch.o obj-$(CONFIG_GPIO_SCH311X) += gpio-sch311x.o +obj-$(CONFIG_GPIO_SIMULATOR) += gpio-simulator.o obj-$(CONFIG_GPIO_SODAVILLE) += gpio-sodaville.o obj-$(CONFIG_GPIO_SPEAR_SPICS) += gpio-spear-spics.o obj-$(CONFIG_GPIO_SPRD) += gpio-sprd.o diff --git a/drivers/gpio/gpio-simulator.c b/drivers/gpio/gpio-simulator.c new file mode 100644 index 000000000000..e8ad5539bd4a --- /dev/null +++ b/drivers/gpio/gpio-simulator.c @@ -0,0 +1,456 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/module.h> +#include <linux/gpio.h> +#include <linux/gpio/driver.h> +#include <linux/kthread.h> +#include <linux/platform_device.h> + +struct gpio_simulator_ddata; + +struct gpio_simulator_port { + struct gpio_simulator_port *other; + struct gpio_simulator_ddata *ddata; + + /* + * Each bit specifies the direction of a gpio. 1=output; 0=input + */ + u32 output; + + /* + * Each bit specifies the output value of a gpio (if the matching bit in + * .output is 1). + */ + u32 output_value; + + /* + * A one in .xxx_enable enables setting the respective bit in + * .xxx_status if the matching event happens. + */ + u32 irq_level_low_enable; + u32 irq_level_high_enable; + u32 irq_edge_falling_enable; + u32 irq_edge_raising_enable; + + /* + * .xxx_status triggers the respective irq if the matching bit in + * .irq_enable is one + */ + u32 irq_level_low_status; + u32 irq_level_high_status; + u32 irq_edge_falling_status; + u32 irq_edge_raising_status; + + u32 irq_enable; + + struct gpio_chip gchip; + struct irq_chip ichip; + + u32 level_cache; +}; + +struct gpio_simulator_ddata { + struct gpio_simulator_port port[2]; + spinlock_t lock; + struct task_struct *irqtrigger_thread; +}; + +static u32 gpio_simulator_get_pending_irq(struct gpio_simulator_port *port) +{ + u32 ret; + + ret = port->irq_enable & (port->irq_level_low_status | + port->irq_level_high_status | + port->irq_edge_falling_status | + port->irq_edge_raising_status); + + return ret; +} + +static void gpio_simulator_update_port(struct gpio_simulator_port *port) +{ + struct gpio_simulator_port *otherport = port->other; + u32 level; + + level = (port->output & port->output_value) | + (otherport->output & otherport->output_value & ~port->output); + + port->irq_level_low_status = ~level & port->irq_level_low_enable; + port->irq_level_high_status = level & port->irq_level_high_enable; + + port->irq_edge_falling_status |= + port->level_cache & ~level & port->irq_edge_falling_enable; + port->irq_edge_raising_status |= + ~port->level_cache & level & port->irq_edge_raising_enable; + + port->level_cache = level; + + if (gpio_simulator_get_pending_irq(port)) + wake_up_process(port->ddata->irqtrigger_thread); +} + +static void gpio_simulator_update(struct gpio_simulator_ddata *ddata) +{ + gpio_simulator_update_port(&ddata->port[0]); + gpio_simulator_update_port(&ddata->port[1]); +} + +static int gpio_simulator_get_direction(struct gpio_chip *chip, + unsigned int offset) +{ + struct gpio_simulator_port *port = + container_of(chip, struct gpio_simulator_port, gchip); + + if (port->output & (1 << offset)) + return GPIOF_DIR_OUT; + else + return GPIOF_DIR_IN; +} + +static int gpio_simulator_direction_input(struct gpio_chip *chip, + unsigned int offset) +{ + struct gpio_simulator_port *port = + container_of(chip, struct gpio_simulator_port, gchip); + unsigned long flags; + + spin_lock_irqsave(&port->ddata->lock, flags); + + port->output &= ~(1 << offset); + + gpio_simulator_update(port->ddata); + + spin_unlock_irqrestore(&port->ddata->lock, flags); + + return 0; +} + +static int gpio_simulator_direction_output(struct gpio_chip *chip, + unsigned int offset, int value) +{ + struct gpio_simulator_port *port = + container_of(chip, struct gpio_simulator_port, gchip); + unsigned long flags; + + spin_lock_irqsave(&port->ddata->lock, flags); + + if (value) + port->output_value |= 1 << offset; + else + port->output_value &= ~(1 << offset); + + port->output |= 1 << offset; + + gpio_simulator_update(port->ddata); + + spin_unlock_irqrestore(&port->ddata->lock, flags); + + return 0; +} + +static int gpio_simulator_get(struct gpio_chip *chip, + unsigned int offset) +{ + struct gpio_simulator_port *port = + container_of(chip, struct gpio_simulator_port, gchip); + int ret; + + ret = port->level_cache & (1 << offset); + + return ret; +} + +static void gpio_simulator_set(struct gpio_chip *chip, + unsigned int offset, int value) +{ + struct gpio_simulator_port *port = + container_of(chip, struct gpio_simulator_port, gchip); + unsigned long flags; + + spin_lock_irqsave(&port->ddata->lock, flags); + + if (value) + port->output_value |= 1 << offset; + else + port->output_value &= ~(1 << offset); + + gpio_simulator_update(port->ddata); + + spin_unlock_irqrestore(&port->ddata->lock, flags); +} + +static void gpio_simulator_irq_ack(struct irq_data *d) +{ + struct irq_chip *ic = irq_data_get_irq_chip(d); + struct gpio_simulator_port *port = + container_of(ic, struct gpio_simulator_port, ichip); + int level; + unsigned long flags; + + spin_lock_irqsave(&port->ddata->lock, flags); + + level = port->level_cache & d->mask; + if (level) + port->irq_level_low_status &= ~d->mask; + else + port->irq_level_high_status &= ~d->mask; + + port->irq_edge_raising_status &= ~d->mask; + port->irq_edge_falling_status &= ~d->mask; + + spin_unlock_irqrestore(&port->ddata->lock, flags); +} + +static void gpio_simulator_irq_mask(struct irq_data *d) +{ + struct irq_chip *ic = irq_data_get_irq_chip(d); + struct gpio_simulator_port *port = + container_of(ic, struct gpio_simulator_port, ichip); + unsigned long flags; + + spin_lock_irqsave(&port->ddata->lock, flags); + + port->irq_enable &= ~d->mask; + + spin_unlock_irqrestore(&port->ddata->lock, flags); +} + +static void gpio_simulator_irq_unmask(struct irq_data *d) +{ + struct irq_chip *ic = irq_data_get_irq_chip(d); + struct gpio_simulator_port *port = + container_of(ic, struct gpio_simulator_port, ichip); + unsigned long flags; + + spin_lock_irqsave(&port->ddata->lock, flags); + + port->irq_enable |= d->mask; + + spin_unlock_irqrestore(&port->ddata->lock, flags); +} + +static int gpio_simulator_irq_set_type(struct irq_data *d, u32 type) +{ + struct irq_chip *ic = irq_data_get_irq_chip(d); + struct gpio_simulator_port *port = + container_of(ic, struct gpio_simulator_port, ichip); + unsigned long flags; + + spin_lock_irqsave(&port->ddata->lock, flags); + + if (d->mask == 0) + d->mask = 1 << d->hwirq; + + if (type & IRQ_TYPE_EDGE_RISING) { + port->irq_edge_raising_enable |= d->mask; + } else { + port->irq_edge_raising_enable &= ~d->mask; + port->irq_edge_raising_status &= ~d->mask; + } + + if (type & IRQ_TYPE_EDGE_FALLING) { + port->irq_edge_falling_enable |= d->mask; + } else { + port->irq_edge_falling_enable &= ~d->mask; + port->irq_edge_falling_status &= ~d->mask; + } + + if (type & IRQ_TYPE_LEVEL_LOW) { + port->irq_level_low_enable |= d->mask; + } else { + port->irq_level_low_enable &= ~d->mask; + port->irq_level_low_status &= ~d->mask; + } + + if (type & IRQ_TYPE_LEVEL_HIGH) { + port->irq_level_high_enable |= d->mask; + } else { + port->irq_level_high_enable &= ~d->mask; + port->irq_level_high_status &= ~d->mask; + } + + gpio_simulator_update(port->ddata); + + spin_unlock_irqrestore(&port->ddata->lock, flags); + + return 0; +} + +static int gpio_simulator_init_port(struct platform_device *pdev, + struct gpio_simulator_ddata *ddata, + unsigned int portno, + struct device_node *of_node) +{ + struct gpio_simulator_port *here = &ddata->port[portno]; + struct gpio_simulator_port *there = &ddata->port[1 - portno]; + struct gpio_chip *gc = &here->gchip; + struct irq_chip *ic = &here->ichip; + int ret; + + here->other = there; + here->ddata = ddata; + + gc->parent = &pdev->dev; + gc->label = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-%u", + dev_name(&pdev->dev), portno); + if (!gc->label) + return -ENOMEM; + + gc->of_node = of_node; + gc->of_gpio_n_cells = 2; + + gc->base = -1; + gc->ngpio = 32; + + gc->get_direction = gpio_simulator_get_direction; + gc->direction_input = gpio_simulator_direction_input; + gc->direction_output = gpio_simulator_direction_output; + gc->get = gpio_simulator_get; + gc->set = gpio_simulator_set; + + ic->name = gc->label; + ic->irq_ack = gpio_simulator_irq_ack; + ic->irq_mask = gpio_simulator_irq_mask; + ic->irq_unmask = gpio_simulator_irq_unmask; + ic->irq_set_type = gpio_simulator_irq_set_type; + + ret = devm_gpiochip_add_data(&pdev->dev, gc, here); + if (ret) + return ret; + + ret = gpiochip_irqchip_add(gc, ic, 0, handle_level_irq, IRQ_TYPE_NONE); + + return ret; +} + +static int gpio_simulator_irqtrigger(void *data) +{ + struct platform_device *pdev = data; + struct gpio_simulator_ddata *ddata = platform_get_drvdata(pdev); + + get_device(&pdev->dev); + + for (;;) { + u32 pending0, pending1; + int offset; + + if (kthread_should_stop()) { + put_device(&pdev->dev); + return 0; + } + + spin_lock_irq(&ddata->lock); + + pending0 = gpio_simulator_get_pending_irq(&ddata->port[0]); + pending1 = gpio_simulator_get_pending_irq(&ddata->port[1]); + + spin_unlock(&ddata->lock); + + for (offset = 0; offset < 32; ++offset) { + if (pending0 & (1 << offset)) { + struct irq_domain *irqdomain; + unsigned int irq; + + irqdomain = ddata->port[0].gchip.irq.domain; + irq = irq_find_mapping(irqdomain, offset); + + generic_handle_irq(irq); + } + + if (pending1 & (1 << offset)) { + struct irq_domain *irqdomain; + unsigned int irq; + + irq_domain = ddata->port[1].gchip.irq.domain; + irq = irq_find_mapping(irqdomain, offset); + + generic_handle_irq(irq); + } + } + + spin_lock(&ddata->lock); + + pending0 = gpio_simulator_get_pending_irq(&ddata->port[0]); + pending1 = gpio_simulator_get_pending_irq(&ddata->port[1]); + + if (!pending0 && !pending1) + set_current_state(TASK_IDLE); + + spin_unlock_irq(&ddata->lock); + + schedule(); + + set_current_state(TASK_RUNNING); + } +} + +static int gpio_simulator_probe(struct platform_device *pdev) +{ + struct gpio_simulator_ddata *ddata; + int ret; + struct device_node *of_nodea, *of_nodeb; + + ddata = devm_kzalloc(&pdev->dev, sizeof(*ddata), GFP_KERNEL); + if (!ddata) + return -ENOMEM; + + platform_set_drvdata(pdev, ddata); + + spin_lock_init(&ddata->lock); + + of_nodea = of_get_child_by_name(pdev->dev.of_node, "sidea"); + ret = gpio_simulator_init_port(pdev, ddata, 0, of_nodea); + if (ret) + return ret; + + of_nodeb = of_get_child_by_name(pdev->dev.of_node, "sideb"); + ret = gpio_simulator_init_port(pdev, ddata, 1, of_nodeb); + if (ret) + return ret; + + ddata->irqtrigger_thread = kthread_run(gpio_simulator_irqtrigger, pdev, + "gpiosim"); + if (IS_ERR(ddata->irqtrigger_thread)) { + dev_err(&pdev->dev, "Failed to create thread\n"); + return PTR_ERR(ddata->irqtrigger_thread); + } + + return 0; +} + +static int gpio_simulator_remove(struct platform_device *pdev) +{ + struct gpio_simulator_ddata *ddata = platform_get_drvdata(pdev); + + kthread_stop(ddata->irqtrigger_thread); + + /* + * XXX: Do I need to remove the irqchip? If yes, then .probe also needs + * an error path. + */ + + return 0; +} + +static const struct of_device_id gpio_simulator_dt_ids[] = { + { + .compatible = "gpio-simulator", + }, { + /* sentinel */ + } +}; +MODULE_DEVICE_TABLE(of, gpio_simulator_dt_ids); + +static struct platform_driver gpio_simulator_driver = { + .driver = { + .name = "gpio-simulator", + .of_match_table = gpio_simulator_dt_ids, + }, + .probe = gpio_simulator_probe, + .remove = gpio_simulator_remove, +}; +module_platform_driver(gpio_simulator_driver); + +MODULE_AUTHOR("Uwe Kleine-Koenig <uwe@kleine-koenig.org>"); +MODULE_DESCRIPTION("gpio simulator driver"); +MODULE_LICENSE("GPL v2");
A gpio simulator device provides 2x 32 virtual GPIOs that are pairwise connected. That is, if both GPIOs are configured as input both read a zero. If one is an output the other reads the first's output value. This can for example be used to add a push button device on one side and "push" it by driving the other side via gpioctl. Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org> --- Hello, this is a patch that I intend to use to test a few patches that are still on my todo list. Do you consider it interesting enough to be suitable for mainline? There is one issue (that I'm aware of): If the driver is unbound (via sysfs) I get warnings that the gpios and/or irqs are still in use. Not sure how to fix this properly. Best regards Uwe .../bindings/gpio/gpio-simulator.txt | 49 ++ drivers/gpio/Kconfig | 12 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-simulator.c | 456 ++++++++++++++++++ 4 files changed, 518 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpio/gpio-simulator.txt create mode 100644 drivers/gpio/gpio-simulator.c