Re: [PATCH] gpiolib: Avoid side effects in gpio_is_visible()

[Bartosz Golaszewski]

On Wed, May 24, 2023 at 7:41 AM Kent Gibson <warthog618@xxxxxxxxx> wrote:
>
> On Tue, May 23, 2023 at 09:17:26PM +0000, Chris Packham wrote:
> >
> > On 24/05/23 04:38, andy.shevchenko@xxxxxxxxx wrote:
> > > Wed, May 17, 2023 at 09:30:51PM +0000, Chris Packham kirjoitti:
> > >> On 17/05/23 20:54, Andy Shevchenko wrote:
> > >>> On Wed, May 17, 2023 at 2:50 AM Chris Packham
> > >>> <Chris.Packham@xxxxxxxxxxxxxxxxxxx> wrote:
> > >>>> On 17/05/23 10:47, Kent Gibson wrote:
> > > ...
> > >
> > >> Again the problem boils down to the fact that we have a userspace switch
> > >> driver (which uses a vendor supplied non-free SDK). So despite the
> > >> kernel having quite good support for SFPs I can't use it without a
> > >> netdev to attach it to.
> > > That user space driver is using what from the kernel? GPIO sysfs?
> >
> > Yes GPIO sysfs and exported links with known names, which allows things
> > to be done per-port but also wildcarded from shell scripts if necessary.
> > I think the key point here is that it doesn't care about the GPIO chips
> > just the individual GPIO lines. Anything involving libgpiod currently
> > has to start caring about GPIO chips (or I'm misreading the docs).
> >
>
> As previously mentioned, the libgpiod tools now support identification of
> lines by name.
> As long as your line names are unique at system scope you should be
> good.  Otherwise you have no option but to identify by (chip,offset).
>
> Wrt the library itself, I was thinking about relocating the line name
> resolution logic from the tools into the library itself, so it would be
> more generally accessible, but haven't gotten there yet.
>
> I'm also of the opinion that libgpiod is too low level for common
> tasks.  That is necessary to access all the features of the uAPI, but
> for basic tasks it would be nice to have a higher level abstraction to
> reduce the barrier to entry.
>
> e.g. in Rust I can do this:
>
>     let led0 = gpiocdev::find_named_line("LED0").unwrap();
>     let req = Request::builder()
>         .with_found_line(&led0)
>         .as_output(Value::Active)
>         .request()?;
>

I would argue that existing high-level bindings for mainline libgpiod
(C++ and Python) allow similar functionality in a comparable number of
LOC. On the other hand - core C library should remain relatively
simple and limited in features.

Chris: are you forced to use C or could you use C++ for line lookup
and management?

I'm also in the process of designing the DBus API and the base for it
will be GLib/GObject bindings for the core C lib. Maybe this is the
place where we could place more advanced features in C as GLib already
makes C coding so much easier.

Bart

>     // change value later
>     req.set_value(led0.offset, Value::Inactive)
>
> which is the equivalent of the sysfs
>
> echo 1 > /some/sysfs/line
> ...
> echo 0 > /some/sysfs/line
>
> That is bad enough. It pains me to see how complex the equivalent is using
> the libgpiod v2 API (or v1), and that is not putting any shade on Bart or
> anyone else who worked on it - there are a lot of constraints on how it
> is designed.  It just doesn't feel complete yet, particularly from a
> casual user's perspective.
>
> One of the things I would like to see added to libgpiod is a set of working
> examples of simple use cases.  Formerly the tools took double duty to
> fill that role, but they've now grown too complex.
> Those examples would highlight where we could provide simplified
> higher level APIs.
> Then rinse and repeat until the simple use cases are simple.
>
> > >>>> I'm sure both of these applications could be re-written around libgpiod
> > >>>> but that would incur a significant amount of regression testing on
> > >>>> existing platforms. And I still consider dealing with GPIO chips an
> > >>>> extra headache that the applications don't need (particularly with the
> > >>>> sheer number of them the SFP case).
> > >>> It seems to me that having no in-kernel driver for your stuff is the
> > >>> main point of all headache here. But I might be mistaken.
> > >> It certainly doesn't help, but I do think that is all orthogonal to the
> > >> fact that gpio_is_visible() changes things rather than just determining
> > >> if an attribute should be exported or not.
> > > Sorry for being unhelpful here. But without understanding the issue we can't
> > > propose better solutions.
> > No problem, this is probably the most engagement I've had out of a Linux
> > patch submission. Hopefully it's not too annoying for those on the Cc list.
>
> Well, now you mention it....
>
> Along the same lines as Andy, it is always useful to get feedback about
> problems people are facing, and how the available solutions aren't
> working for them.
> If we don't know the problem exists then we can't fix it - except by
> accident.
>
> Cheers,
> Kent.