Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200814030257.135463-1-warthog618@gmail.com> <20200814030257.135463-8-warthog618@gmail.com>
 <CAMpxmJXdGUnnomfWNRmpi979jLPMj17JuA=0K2Nq-oVS_-oQ3A@mail.gmail.com>
 <20200815065309.GA13905@sol> <CAMpxmJVMJYJ71z59Mu-mCTQPNqV9N9sXJXNBjYazGo0tmXthfg@mail.gmail.com>
 <20200815132132.GA26885@sol>
In-Reply-To: <20200815132132.GA26885@sol>
From:   Bartosz Golaszewski <bgolaszewski@baylibre.com>
Date:   Sun, 16 Aug 2020 14:13:30 +0200
Message-ID: <CAMpxmJVz=ZYR+PEei4C+xjChkRsCsp7K=L5=GgMt2K4Vv3vBZQ@mail.gmail.com>
Subject: Re: [PATCH v4 07/20] gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL
 and GPIO_V2_LINE_GET_VALUES_IOCTL
To:     Kent Gibson <warthog618@gmail.com>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        linux-gpio <linux-gpio@vger.kernel.org>,
        Linus Walleij <linus.walleij@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, Aug 15, 2020 at 3:21 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> 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".
>

But struct file is always associated with a single file descriptor,
it's not the case for struct line. I would be fine with this name if
it was an object representing a single line like in libgpiod's
gpiod_line.

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

struct line sounds as if it represented a single line, struct
line_data is more ambiguous and can be understood both ways IMO.

Bart