Re: SV: [PATCH RFC] gpio: new driver for a gpio simulator

[Bartosz Golaszewski <brgl@xxxxxxxx>]

pon., 15 paź 2018 o 22:03 Uwe Kleine-König
<u.kleine-koenig@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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/