From: Kent Gibson <warthog618@gmail.com>
To: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Cc: LKML <linux-kernel@vger.kernel.org>,
linux-gpio <linux-gpio@vger.kernel.org>,
Linus Walleij <linus.walleij@linaro.org>
Subject: Re: [PATCH v4 07/20] gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL and GPIO_V2_LINE_GET_VALUES_IOCTL
Date: Sat, 15 Aug 2020 21:21:32 +0800 [thread overview]
Message-ID: <20200815132132.GA26885@sol> (raw)
In-Reply-To: <CAMpxmJVMJYJ71z59Mu-mCTQPNqV9N9sXJXNBjYazGo0tmXthfg@mail.gmail.com>
On Sat, Aug 15, 2020 at 09:21:22AM +0200, Bartosz Golaszewski wrote:
> On Sat, Aug 15, 2020 at 8:53 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Fri, Aug 14, 2020 at 09:31:29PM +0200, Bartosz Golaszewski wrote:
> > > On Fri, Aug 14, 2020 at 5:04 AM Kent Gibson <warthog618@gmail.com> wrote:
> > > >
> > > > Add support for requesting lines using the GPIO_V2_GET_LINE_IOCTL, and
> > > > returning their current values using GPIO_V2_LINE_GET_VALUES_IOCTL.
> > > >
> > > > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> > > > ---
> > >
> > > Hi Kent,
> > >
> > > not many comments here, just a couple minor details below.
> > >
> >
> > [snip]
> >
> > > > +
> > > > +/**
> > > > + * struct line - contains the state of a userspace line request
> > > > + * @gdev: the GPIO device the line request pertains to
> > > > + * @label: consumer label used to tag descriptors
> > > > + * @num_descs: the number of descriptors held in the descs array
> > > > + * @descs: the GPIO descriptors held by this line request, with @num_descs
> > > > + * elements.
> > > > + */
> > > > +struct line {
> > >
> > > How about line_request, line_request_data or line_req_ctx? Something
> > > more intuitive than struct line that doesn't even refer to a single
> > > line. Same for relevant functions below.
> > >
> >
> > As I've mentioned previously, I'm not a fan of names that include _data,
> > _ctx, _state, or similar that don't really add anything.
> >
>
> I certainly disagree with you on this. I think it's useful to discern
> the object itself from data associated with it. Let's consider struct
> irq_data and let's imagine it would be called struct irq instead. The
> latter would be misleading - as this struct contains a lot additional
> fields that form the context for the irq but aren't logically part of
> the "irq object". And then you have irq_common_data which is even more
> disconnected from the irq. This also would make using the name "irq"
> for the variables containing the global irq number confusing.
>
> I think the same happens here: we may want to use the name "line" for
> local variables and then having "struct line_data" (or similar) would
> make it easier to read.
>
My counter example to both points is "struct file *file".
> I'll listen to other's suggestions/voices but personally I think that
> _ctx, _data etc. suffixes actually make sense.
>
> > I did consider line_request, but that was too close to the
> > gpio_v2_line_request in gpio.d, not just the struct but also the
> > resulting local variables, particularly in line_create() where they
> > co-exist.
> >
> > Given the ioctl names, GPIO_V2_GET_LINE_IOCTL and
> > GPIO_V2_LINE_GET/SET_xxx, that all create or operate on this struct, and
> > that this is within the scope of gpiolib-cdev, the name 'line' seemed the
> > best fit.
> >
>
> And that's why line_data or line_request_data do make sense IMO.
>
> > And how does it not refer to a single line - what are the descs??
> >
>
> I meant the fact that it can refer to multiple lines while being
> called "struct line". I do find this misleading.
>
And struct line_data isn't?
Cheers,
Kent.
next prev parent reply other threads:[~2020-08-15 21:58 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-14 3:02 [PATCH v4 00/20] gpio: cdev: add uAPI v2 Kent Gibson
2020-08-14 3:02 ` [PATCH v4 01/20] gpiolib: cdev: desc_to_lineinfo should set info offset Kent Gibson
2020-08-14 3:02 ` [PATCH v4 02/20] gpiolib: cdev: replace strncpy with strscpy Kent Gibson
2020-08-14 3:02 ` [PATCH v4 03/20] gpio: uapi: define GPIO_MAX_NAME_SIZE for array sizes Kent Gibson
2020-08-14 3:02 ` [PATCH v4 04/20] gpio: uapi: define uAPI v2 Kent Gibson
2020-08-14 3:02 ` [PATCH v4 05/20] gpiolib: make cdev a build option Kent Gibson
2020-08-14 3:02 ` [PATCH v4 06/20] gpiolib: add build option for CDEV v1 ABI Kent Gibson
2020-08-14 3:02 ` [PATCH v4 07/20] gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL and GPIO_V2_LINE_GET_VALUES_IOCTL Kent Gibson
2020-08-14 19:31 ` Bartosz Golaszewski
2020-08-15 6:53 ` Kent Gibson
2020-08-15 7:21 ` Bartosz Golaszewski
2020-08-15 13:21 ` Kent Gibson [this message]
2020-08-16 12:13 ` Bartosz Golaszewski
2020-08-14 3:02 ` [PATCH v4 08/20] gpiolib: cdev: support GPIO_V2_GET_LINEINFO_IOCTL and GPIO_V2_GET_LINEINFO_WATCH_IOCTL Kent Gibson
2020-08-16 12:03 ` Bartosz Golaszewski
2020-08-14 3:02 ` [PATCH v4 09/20] gpiolib: cdev: support edge detection for uAPI v2 Kent Gibson
2020-08-16 14:32 ` Bartosz Golaszewski
2020-08-18 14:00 ` Kent Gibson
2020-08-14 3:02 ` [PATCH v4 10/20] gpiolib: cdev: support GPIO_V2_LINE_SET_CONFIG_IOCTL Kent Gibson
2020-08-16 14:40 ` Bartosz Golaszewski
2020-08-14 3:02 ` [PATCH v4 11/20] gpiolib: cdev: support GPIO_V2_LINE_SET_VALUES_IOCTL Kent Gibson
2020-08-16 14:57 ` Bartosz Golaszewski
2020-08-14 3:02 ` [PATCH v4 12/20] gpiolib: cdev: support setting debounce Kent Gibson
2020-08-17 18:21 ` Bartosz Golaszewski
2020-08-18 14:37 ` Kent Gibson
2020-08-14 3:02 ` [PATCH v4 13/20] gpio: uapi: document uAPI v1 as deprecated Kent Gibson
2020-08-14 3:02 ` [PATCH v4 14/20] tools: gpio: port lsgpio to v2 uAPI Kent Gibson
2020-08-14 3:02 ` [PATCH v4 15/20] tools: gpio: port gpio-watch " Kent Gibson
2020-08-14 3:02 ` [PATCH v4 16/20] tools: gpio: rename nlines to num_lines Kent Gibson
2020-08-14 3:02 ` [PATCH v4 17/20] tools: gpio: port gpio-hammer to v2 uAPI Kent Gibson
2020-08-14 3:02 ` [PATCH v4 18/20] tools: gpio: port gpio-event-mon " Kent Gibson
2020-08-14 3:02 ` [PATCH v4 19/20] tools: gpio: add multi-line monitoring to gpio-event-mon Kent Gibson
2020-08-14 3:02 ` [PATCH v4 20/20] tools: gpio: add debounce support " Kent Gibson
2020-08-17 18:24 ` [PATCH v4 00/20] gpio: cdev: add uAPI v2 Bartosz Golaszewski
2020-08-17 18:40 ` Andy Shevchenko
2020-08-17 19:25 ` Bartosz Golaszewski
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=20200815132132.GA26885@sol \
--to=warthog618@gmail.com \
--cc=bgolaszewski@baylibre.com \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@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 NNTP newsgroup(s).