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: <20200831032006.1019978-1-warthog618@gmail.com> <20200831032006.1019978-5-warthog618@gmail.com>
In-Reply-To: <20200831032006.1019978-5-warthog618@gmail.com>
From:   Bartosz Golaszewski <bgolaszewski@baylibre.com>
Date:   Fri, 4 Sep 2020 08:23:23 +0200
Message-ID: <CAMpxmJVmMfND78VqF0cv=WF1PceGvRvWoQpdO6-8XVt84W9Aiw@mail.gmail.com>
Subject: Re: [PATCH v6 04/20] gpio: uapi: define uAPI v2
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 Mon, Aug 31, 2020 at 5:21 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> Add a new version of the uAPI to address existing 32/64-bit alignment
> issues, add support for debounce and event sequence numbers, allow
> requested lines with different configurations, and provide some future
> proofing by adding padding reserved for future use.
>
> The alignment issue relates to the gpioevent_data, which packs to different
> sizes on 32-bit and 64-bit platforms. That creates problems for 32-bit apps
> running on 64-bit kernels.  uAPI v2 addresses that particular issue, and
> the problem more generally, by adding pad fields that explicitly pad
> structs out to 64-bit boundaries, so they will pack to the same size now,
> and even if some of the reserved padding is used for __u64 fields in the
> future.
>
> The new structs have been analysed with pahole to ensure that they
> are sized as expected and contain no implicit padding.
>
> The lack of future proofing in v1 makes it impossible to, for example,
> add the debounce feature that is included in v2.
> The future proofing is addressed by providing configurable attributes in
> line config and reserved padding in all structs for future features.
> Specifically, the line request, config, info, info_changed and event
> structs receive updated versions and new ioctls.
>
> As the majority of the structs and ioctls were being replaced, it is
> opportune to rework some of the other aspects of the uAPI:
>
> v1 has three different flags fields, each with their own separate
> bit definitions.  In v2 that is collapsed to one - gpio_v2_line_flag.
>
> The handle and event requests are merged into a single request, the line
> request, as the two requests were mostly the same other than the edge
> detection provided by event requests.  As a byproduct, the v2 uAPI allows
> for multiple lines producing edge events on the same line handle.
> This is a new capability as v1 only supports a single line in an event
> request.
>
> As a consequence, there are now only two types of file handle to be
> concerned with, the chip and the line, and it is clearer which ioctls
> apply to which type of handle.
>
> There is also some minor renaming of fields for consistency compared to
> their v1 counterparts, e.g. offset rather than lineoffset or line_offset,
> and consumer rather than consumer_label.
>
> Additionally, v1 GPIOHANDLES_MAX becomes GPIO_V2_LINES_MAX in v2 for
> clarity, and the gpiohandle_data __u8 array becomes a bitmap in
> gpio_v2_line_values.
>
> The v2 uAPI is mostly a reorganisation and extension of v1, so userspace
> code, particularly libgpiod, should readily port to it.
>
> Signed-off-by: Kent Gibson <warthog618@gmail.com>
> ---

[snip]

> +
> +/**
> + * enum gpio_v2_line_flag - &struct gpio_v2_line_attribute.flags values
> + */
> +enum gpio_v2_line_flag {
> +       GPIO_V2_LINE_FLAG_USED                  = 1ULL << 0, /* line is not available for request */
> +       GPIO_V2_LINE_FLAG_ACTIVE_LOW            = 1ULL << 1,
> +       GPIO_V2_LINE_FLAG_INPUT                 = 1ULL << 2,
> +       GPIO_V2_LINE_FLAG_OUTPUT                = 1ULL << 3,
> +       GPIO_V2_LINE_FLAG_EDGE_RISING           = 1ULL << 4,
> +       GPIO_V2_LINE_FLAG_EDGE_FALLING          = 1ULL << 5,
> +       GPIO_V2_LINE_FLAG_OPEN_DRAIN            = 1ULL << 6,
> +       GPIO_V2_LINE_FLAG_OPEN_SOURCE           = 1ULL << 7,
> +       GPIO_V2_LINE_FLAG_BIAS_PULL_UP          = 1ULL << 8,
> +       GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN        = 1ULL << 9,
> +       GPIO_V2_LINE_FLAG_BIAS_DISABLED         = 1ULL << 10,
> +};
> +
>

One more small thing I noticed: the uapi exports _BITULL() macro to
user-space for bit definitions. I think it's worth using it here as
it's more readable than (1ULL << X) IMO.

Bart

[snip]