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

[Chris Packham]

Hi Bart,

On 27/05/23 00:46, Bartosz Golaszewski wrote:
> 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?

We're talking embedded devices so the majority of stuff is written in C. 
C++ is available but we'd need to interface from an existing application 
written in C. Python/Rust are probably out for the time being (Rust 
probably will happen eventually).

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

That'd work too. We already use GLib quite a lot.

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