Re: [PATCH v10 2/5] gpio: sim: new testing module

[Bartosz Golaszewski]

On Fri, Nov 26, 2021 at 3:23 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> On Thu, Nov 25, 2021 at 02:14:20PM +0100, Bartosz Golaszewski wrote:
> > On Wed, Nov 24, 2021 at 12:43 PM Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
> > >
> > > Implement a new, modern GPIO testing module controlled by configfs
> > > attributes instead of module parameters. The goal of this driver is
> > > to provide a replacement for gpio-mockup that will be easily extensible
> > > with new features and doesn't require reloading the module to change
> > > the setup.
> > >
> > > Signed-off-by: Bartosz Golaszewski <brgl@xxxxxxxx>
> > > ---
> > >  Documentation/admin-guide/gpio/gpio-sim.rst |   80 ++
> > >  drivers/gpio/Kconfig                        |    8 +
> > >  drivers/gpio/Makefile                       |    1 +
> > >  drivers/gpio/gpio-sim.c                     | 1370 +++++++++++++++++++
> > >  4 files changed, 1459 insertions(+)
> > >  create mode 100644 Documentation/admin-guide/gpio/gpio-sim.rst
> > >  create mode 100644 drivers/gpio/gpio-sim.c
> > >
> >
> > Hi guys!
> >
> > I'd like to get your opinion on some parts of the interface.
> >
> > Should we allow creating multiple gpiochips per platform device like
> > some drivers do? And if so - should the sysfs groups be created for
> > each gpiochip device kobject and not the parent?
> >
> > Currently we do this:
> >
> > # Create the chip (platform device + single gpiochip):
> > mkdir /sys/kernel/config/gpio-sim/my-chip
> > # Configure it
> > echo 8 > /sys/kernel/config/gpio-sim/my-chip/num_lines
> > # Enable it
> > echo 1 > /sys/kernel/config/gpio-sim/my-chip/live
> >
> > What I mean above would make it look like this:
> >
> > # Create the platform device
> > mkdir /sys/kernel/config/gpio-sim/my-gpio-device
> >
> > # what's inside?
> > ls /sys/kernel/config/gpio-sim/my-gpio-device
> > live
> >
> > # Create GPIO chips
> > mkdir /sys/kernel/config/gpio-sim/my-gpio-device/chip0
> > mkdir /sys/kernel/config/gpio-sim/my-gpio-device/chip1
> >
> > # Configure chips
> > echo 8 > /sys/kernel/config/gpio-sim/my-gpio-device/chip0/num_lines
> > echo 4 > /sys/kernel/config/gpio-sim/my-gpio-device/chip1/num_lines
> > echo foobar > /sys/kernel/config/gpio-sim/my-gpio-device/chip1/label
> >
> > # Enable both chips
> > echo 1 > /sys/kernel/config/gpio-sim/my-gpio-device/live
> >
> > And in sysfs instead of current:
> >
> > echo pull-up > /sys/devices/platform/gpio-sim.0/sim_line0/pull
> >
> > We'd have to do:
> >
> > echo pull-up > /sys/devices/platform/gpio-sim.0/gpiochip1/sim_line0/pull
> >
> > While I don't see any usefulness of that at this time, if we don't do
> > it now, then it'll be hard to extend this module later. What are your
> > thoughts?
> >
>
> I might be missing something, but I don't see the platform abstraction
> adding anything that can't be easily emulated in userspace using multiple
> chips, and it complicates the minimal case as you now have to create a
> platform as well as the chip.

I wouldn't stress about the level of complication of the user-space
interface. Normally users will wrap it in some kind of library anyway.

> So I'd keep it simple and stick with the chip level abstraction.
>
> Cheers,
> Kent.
>

Yes, in user-space nothing would change as it only sees separate
gpiochips whether they're just banks of the same device or actual
devices of their own (except if you start digging into uevents'
contents, then you'll see it).

But this module is also aimed at doing some kernel subsystem testing
(even if triggered from user-space) and providing multiple banks of
GPIOs is a regularly used feature. Adding it allows us to test this
other level of hierarchy + multi-level device properties etc.

Another issue is logical correctness. In this version the line
attributes in sysfs are at the platform device level
(/sys/platform/devices/gpio-sim.X). Logically they really should be at
the gpio device level (/sys/platform/devices/gpio-sim.X/gpiochipY).

So I'm willing to give it a shot.