[RFC] gpio: new driver for a gpio simulator

Message ID 20181008101356.5672-1-uwe@kleine-koenig.org
State RFC
Headers show
Series
  • [RFC] gpio: new driver for a gpio simulator
Related show

Commit Message

Uwe Kleine-König Oct. 8, 2018, 10:13 a.m.
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

Comments

Uwe Kleine-König Oct. 8, 2018, 1:08 p.m. | #1
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
Linus Walleij Oct. 9, 2018, 12:51 p.m. | #2
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
Uwe Kleine-König Oct. 9, 2018, 7:11 p.m. | #3
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
Linus Walleij Oct. 10, 2018, 11:47 a.m. | #4
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
Uwe Kleine-König Oct. 11, 2018, 8:16 a.m. | #5
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
Vincent Whitchurch Oct. 11, 2018, 9:49 a.m. | #6
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.
Einar Vading Oct. 12, 2018, 8:02 a.m. | #7
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
Uwe Kleine-König Oct. 12, 2018, 9:06 a.m. | #8
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
Einar Vading Oct. 12, 2018, 9:27 a.m. | #9
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
Uwe Kleine-König Oct. 12, 2018, 9:47 a.m. | #10
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
Bartosz Golaszewski Oct. 15, 2018, 9:54 a.m. | #11
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
Uwe Kleine-König Oct. 15, 2018, 8:03 p.m. | #12
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
Bartosz Golaszewski Oct. 18, 2018, 3:03 p.m. | #13
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/
Uwe Kleine-König Oct. 18, 2018, 7:16 p.m. | #14
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

Patch

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");