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:   Sun, 20 Sep 2020 18:48:26 +0800
From:   Kent Gibson <warthog618@gmail.com>
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Bartosz Golaszewski <bgolaszewski@baylibre.com>,
        Linus Walleij <linus.walleij@linaro.org>
Subject: Re: [PATCH v8 04/20] gpio: uapi: define uAPI v2
Message-ID: <20200920104826.GA793608@sol>
References: <20200909102640.1657622-1-warthog618@gmail.com>
 <20200909102640.1657622-5-warthog618@gmail.com>
 <CAHp75VdF1be-W3iV2JfWYZzEhrj13+5A+1Y4J8XgpBrkvg8cZQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHp75VdF1be-W3iV2JfWYZzEhrj13+5A+1Y4J8XgpBrkvg8cZQ@mail.gmail.com>
Precedence: bulk

On Tue, Sep 15, 2020 at 01:06:36PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 9, 2020 at 1:29 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> 
> Thanks again for doing this and sorry for quite a delay from my side.
> Below my comments regarding uAPI v2 (mostly nit-picks as I agree with
> it in general).
> 

No problems with most of your points - so I'll chop those bits out.
Responses to the remainder below...

> > +/**
> > + * enum gpio_v2_line_attr_id - &struct gpio_v2_line_attribute.id values
> 
> Perhaps per-item description (couple of words per each)?
> 

I'll add kernel doc for all the enum values - see if that works.

> > + * GPIO_V2_LINE_FLAG_OUTPUT etc, OR:ed together.  This is the default for
> 
> I think "OR:ed together" is understandable for programmers but a bit
> special for normal phrases. I would rephrase it somehow, but I'm not a
> native speaker.
> Something like "or any of their combinations".
> 

My bad - I was assuming the primary audience was programmers ;-).

Actually that description is drawn from the v1 uAPI, as is the case for
all of the fields that were carried over.

And "or any of their combinations" is misleading - some combinations are
invalid.

I'll take another look at it, but I'm ok with it as is. (and I' ok with
'ok' over 'OK' or 'okay' as well, btw ;-)

> > + * @attrs: the configuration attributes associated with the requested
> > + * lines.  Any attribute should only be associated with a particular line
> > + * once.  If an attribute is associated with a line multiple times then the
> > + * first occurrence (i.e. lowest index) has precedence.
> 
> This is a bit unexpected. I anticipated last come last served as in a
> shell script argument list.
> 

I don't want to encourage userspace to just add another entry to attrs
to encode a change.
The behaviour was originally as you describe, but was changed in v3 to
encourage userspace to keep the configuration attributes as simplified as
possible, and to make clearer that the fields, particularly the flags bits,
are not overlayed.

> > +       /*
> > +        * Pad to fill implicit padding and provide space for future use.
> > +        */
> 
> > +       __u32 padding[5];
> 
> This is still questionable. First of all why to waste so many bytes (I
> propose 5 -> 1) and how will you do when the structure changes (we
> still need some type of versioning or flags for which one u32 is more
> than enough).
> 

Ack - we disagree on how to manage future changes and the impact of
reserving space.

> > + * @num_lines: number of lines requested in this request, i.e. the number
> > + * of valid fields in the GPIO_V2_LINES_MAX sized arrays, set to 1 to
> > + * request a single line
> 
> I would rather call it "amount" or something which is one word, but
> you may have a reason for the current, so I don't insist.
> Also, I would describe what will be the expected behaviour outside of
> the range (and what is the range?).
> For example, would it mean that we consider all lines if num_lines >=
> _LINES_MAX?
> 

Using num_X to describe the number of active entries in array X is a
common pattern, so I'll stick with that.

And num_lines > _LINES_MAX is interpreted as invalid - since they wont
fit.  The EINVAL the user will get if they try should make that clear.

> > + * @consumer: a functional name for the consumer of this GPIO line as set
> > + * by whatever is using it, will be empty if there is no current user but
> > + * may also be empty if the consumer doesn't set this up
> 
> In both cases "empty" means what? All \0:s, only first \0, white spaces?
> 

Another one inherited from v1 - empty => consumer[0] == '\0'.

Cheers,
Kent.