All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kent Gibson <warthog618@gmail.com>
To: Chris Packham <Chris.Packham@alliedtelesis.co.nz>
Cc: "andy.shevchenko@gmail.com" <andy.shevchenko@gmail.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	"brgl@bgdev.pl" <brgl@bgdev.pl>,
	"johan@kernel.org" <johan@kernel.org>,
	"maz@kernel.org" <maz@kernel.org>,
	Ben Brown <Ben.Brown@alliedtelesis.co.nz>,
	"linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] gpiolib: Avoid side effects in gpio_is_visible()
Date: Wed, 24 May 2023 13:41:23 +0800	[thread overview]
Message-ID: <ZG2jgwjK+CBmOk3G@sol> (raw)
In-Reply-To: <72990baf-6964-01ad-d891-7090831d0310@alliedtelesis.co.nz>

On Tue, May 23, 2023 at 09:17:26PM +0000, Chris Packham wrote:
> 
> On 24/05/23 04:38, andy.shevchenko@gmail.com 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@alliedtelesis.co.nz> 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()?;

    // 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.

  reply	other threads:[~2023-05-24  5:41 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-05-12  4:28 [PATCH] gpiolib: Avoid side effects in gpio_is_visible() Chris Packham
2023-05-12  7:24 ` Johan Hovold
2023-05-14 21:57   ` Chris Packham
2023-05-15  6:43     ` andy.shevchenko
2023-05-15 21:01       ` Chris Packham
2023-05-12  7:56 ` Linus Walleij
2023-05-14 22:27   ` Chris Packham
2023-05-16 13:57     ` Linus Walleij
2023-05-16 22:19       ` Chris Packham
2023-05-16 22:47         ` Kent Gibson
2023-05-16 23:50           ` Chris Packham
2023-05-17  0:47             ` Kent Gibson
2023-05-17  1:05               ` Kent Gibson
2023-05-17  1:07               ` Chris Packham
2023-05-17  1:21                 ` Kent Gibson
2023-05-17  8:54             ` Andy Shevchenko
2023-05-17  9:10               ` Kent Gibson
2023-05-17 21:30               ` Chris Packham
2023-05-23 16:38                 ` andy.shevchenko
2023-05-23 21:17                   ` Chris Packham
2023-05-24  5:41                     ` Kent Gibson [this message]
2023-05-24 23:53                       ` using libgpiod to replace sysfs ABI (was Re: [PATCH] gpiolib: Avoid side effects in gpio_is_visible()) Chris Packham
2023-05-25  1:19                         ` Kent Gibson
2023-05-25  9:13                         ` Andy Shevchenko
2023-05-25 14:35                           ` Bartosz Golaszewski
2023-05-26 12:46                       ` [PATCH] gpiolib: Avoid side effects in gpio_is_visible() Bartosz Golaszewski
2023-05-28 21:04                         ` Chris Packham
2023-05-29  9:19                 ` Linus Walleij
2023-05-29 15:07                   ` Andrew Lunn
2023-05-29  9:07         ` Linus Walleij
2023-05-29 22:00           ` Chris Packham

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZG2jgwjK+CBmOk3G@sol \
    --to=warthog618@gmail.com \
    --cc=Ben.Brown@alliedtelesis.co.nz \
    --cc=Chris.Packham@alliedtelesis.co.nz \
    --cc=andy.shevchenko@gmail.com \
    --cc=brgl@bgdev.pl \
    --cc=johan@kernel.org \
    --cc=linus.walleij@linaro.org \
    --cc=linux-gpio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maz@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.