From: Kent Gibson <warthog618@gmail.com>
To: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: linux-gpio@vger.kernel.org
Subject: Re: [libgpiod][RFC] helper functions for basic use cases
Date: Wed, 15 May 2024 22:14:36 +0800 [thread overview]
Message-ID: <20240515141436.GA349711@rigel> (raw)
In-Reply-To: <CAMRc=MeBGJwyKBTYD1PQkk940t6ECsBxHCprjFUx1KFSKMe7fw@mail.gmail.com>
On Wed, May 15, 2024 at 06:54:15AM -0700, Bartosz Golaszewski wrote:
> On Wed, 15 May 2024 11:18:48 +0200, Kent Gibson <warthog618@gmail.com> said:
> >
> > Sure thing. This is what I have at the moment (the declarations are as
> > per earlier, just renamed. And I just noticed some variables I haven't
> > renamed. I'll add it to the todo list.):
> >
> > diff --git a/lib/line-request.c b/lib/line-request.c
> > index b76b3d7..5af23e0 100644
> > --- a/lib/line-request.c
> > +++ b/lib/line-request.c
> > @@ -305,3 +305,200 @@ gpiod_line_request_read_edge_events(struct gpiod_line_request *request,
> >
> > return gpiod_edge_event_buffer_read_fd(request->fd, buffer, max_events);
> > }
> > +
> > +static struct gpiod_line_request *
> > +ext_request(const char *path, unsigned int offset,
> > + enum gpiod_line_direction direction,
> > + enum gpiod_line_value value)
> > +{
> > + struct gpiod_line_request *request = NULL;
> > + struct gpiod_line_settings *settings;
> > + struct gpiod_line_config *line_cfg;
> > + struct gpiod_chip *chip;
> > + int ret;
> > +
> > + chip = gpiod_chip_open(path);
> > + if (!chip)
> > + return NULL;
> > +
> > + settings = gpiod_line_settings_new();
> > + if (!settings)
> > + goto close_chip;
> > +
> > + gpiod_line_settings_set_direction(settings, direction);
> > + if (direction == GPIOD_LINE_DIRECTION_OUTPUT)
> > + gpiod_line_settings_set_output_value(settings, value);
> > +
> > + line_cfg = gpiod_line_config_new();
> > + if (!line_cfg)
> > + goto free_settings;
> > +
> > + ret = gpiod_line_config_add_line_settings(line_cfg, &offset, 1,
> > + settings);
> > + if (ret)
> > + goto free_line_cfg;
> > +
> > + request = gpiod_chip_request_lines(chip, NULL, line_cfg);
> > +
> > +free_line_cfg:
> > + gpiod_line_config_free(line_cfg);
> > +
> > +free_settings:
> > + gpiod_line_settings_free(settings);
> > +
> > +close_chip:
> > + gpiod_chip_close(chip);
> > +
> > + return request;
> > +}
> > +
> > +GPIOD_API struct gpiod_line_request *
> > +gpiod_ext_request_input(const char *path, unsigned int offset)
> > +{
> > + return ext_request(path, offset, GPIOD_LINE_DIRECTION_INPUT, 0);
> > +}
> > +
> > +GPIOD_API struct gpiod_line_request *
> > +gpiod_ext_request_output(const char *path, unsigned int offset,
> > + enum gpiod_line_value value)
> > +{
> > + return ext_request(path, offset, GPIOD_LINE_DIRECTION_OUTPUT, value);
> > +}
> > +
> > +static struct gpiod_line_settings *
> > +ext_line_settings(struct gpiod_line_request * olr)
> > +{
> > + struct gpiod_line_settings *settings = NULL;
> > + struct gpiod_line_info *line_info;
> > + struct gpiod_chip *chip;
> > + char path[32];
> > +
> > + assert(olr);
> > +
> > + if (olr->num_lines != 1) {
> > + errno = EINVAL;
> > + return NULL;
> > + }
> > +
> > + /*
> > + * This is all decidedly non-optimal, as generally the user has the
> > + * config available from when they made the request, but here we need to
> > + * rebuild it from the line info...
> > + */
>
> Yeah, I hate it...
>
I wasn't expecting any love for it - it is ugly.
But it does the job.
> I would assume hogging memory for config structs is still cheaper than all these
> operations needed to reread the settings for a line. Not to mention the fact
> that if we ever extend the settings, we'll need to remember to update this
> routine too.
>
It shouldn't be used often, so I'm fine with the overhead.
The benefit is it uses a standard line-request.
If the user isn't hapopy with the overhead then they can use the core API.
And the function only has to deal with the config that is settable via
ext. If you have set config via the core API then what the hell are you
doing calling one of these?
> The users of the _ext_ functions most likely wouldn't care whether the context
> is stored in struct gpiod_line_request or struct gpiod_ext_request or otherwise.
>
Yeah, but then they can't use the existing gpiod_line_request_XXX functions
can they? At least not without going through an accessor to pull the
line_request from the ext_request.
> > + memcpy(path, "/dev/", 5);
> > + strncpy(&path[5], olr->chip_name, 26);
> > + chip = gpiod_chip_open(path);
> > + if (!chip)
> > + return NULL;
> > +
> > + // get info
> > + line_info = gpiod_chip_get_line_info(chip, olr->offsets[0]);
> > + gpiod_chip_close(chip);
> > + if (!line_info)
> > + return NULL;
> > +
> > + if (gpiod_line_info_get_direction(line_info) != GPIOD_LINE_DIRECTION_INPUT) {
> > + errno = EINVAL;
> > + goto free_info;
> > + }
> > +
> > + settings = gpiod_line_settings_new();
> > + if (!settings)
> > + goto free_info;
> > +
> > + gpiod_line_settings_set_direction(settings, GPIOD_LINE_DIRECTION_INPUT);
> > + gpiod_line_settings_set_bias(settings,
> > + gpiod_line_info_get_bias(line_info));
> > + gpiod_line_settings_set_edge_detection(settings,
> > + gpiod_line_info_get_edge_detection(line_info));
> > + gpiod_line_settings_set_debounce_period_us(settings,
> > + gpiod_line_info_get_debounce_period_us(line_info));
> > +
> > +free_info:
> > + gpiod_line_info_free(line_info);
> > +
> > + return settings;
> > +}
> > +
> >
> > Oh, I also added this:
> >
> > +/**
> > + * @brief The size required to contain a single edge event.
> > + * @return The size in bytes.
> > + */
> > +size_t gpiod_edge_event_size();
> > +
> >
> > So users can perform the event read themselves without requiring any
> > knowledge of event buffers (at the moment they can't determine the size
> > required to perform the read).
> >
>
> Can you post an example of how this is used?
>
Sure [1].
> > I also intend to provide an updated set of examples that use the ext API.
> > They should go in examples/ext?
> >
>
> I think the code should go into ext/, the gpiod-ext.h header can go right next
> to gpiod.h in include/ and the examples can be in the same examples/ directory,
> just call them something_something_ext.c to indicate they use the simpler API.
>
> Does that sound right?
>
At the moment I've made the code a conditionally compiled block in
line-request.c, so it can directly use the line-request internals.
Pretty sure that can be changed to use the core API, but isn't pimpl within
the library itself a tad extreme?
Cheers,
Kent.
[1] https://github.com/warthog618/libgpiod/blob/ext/examples/ext/async_watch_line_value.c
next prev parent reply other threads:[~2024-05-15 14:14 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-07 2:21 [libgpiod][RFC] helper functions for basic use cases Kent Gibson
2024-05-10 18:37 ` Bartosz Golaszewski
2024-05-11 1:11 ` Kent Gibson
2024-05-13 8:28 ` Bartosz Golaszewski
2024-05-13 10:13 ` Kent Gibson
2024-05-14 13:31 ` Bartosz Golaszewski
2024-05-14 13:38 ` Kent Gibson
2024-05-15 8:26 ` Bartosz Golaszewski
2024-05-15 9:18 ` Kent Gibson
2024-05-15 13:37 ` Kent Gibson
2024-05-15 13:54 ` Bartosz Golaszewski
2024-05-15 14:14 ` Kent Gibson [this message]
2024-05-16 0:19 ` Kent Gibson
2024-05-16 13:55 ` Kent Gibson
2024-05-17 10:53 ` Bartosz Golaszewski
2024-05-17 12:37 ` Kent Gibson
2024-05-22 10:59 ` Bartosz Golaszewski
2024-05-22 12:41 ` Kent Gibson
2024-05-24 14:20 ` Bartosz Golaszewski
2024-05-15 7:06 ` Martin Hundebøll
2024-05-15 9:32 ` Kent Gibson
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=20240515141436.GA349711@rigel \
--to=warthog618@gmail.com \
--cc=brgl@bgdev.pl \
--cc=linux-gpio@vger.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).