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;
Date:   Sat, 15 Aug 2020 14:53:09 +0800
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
Message-ID: <20200815065309.GA13905@sol>
References: <20200814030257.135463-1-warthog618@gmail.com>
 <20200814030257.135463-8-warthog618@gmail.com>
 <CAMpxmJXdGUnnomfWNRmpi979jLPMj17JuA=0K2Nq-oVS_-oQ3A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAMpxmJXdGUnnomfWNRmpi979jLPMj17JuA=0K2Nq-oVS_-oQ3A@mail.gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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 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 how does it not refer to a single line - what are the descs??

No problems with your other comments.

Cheers,
Kent.