On Sat, Aug 15, 2020 at 3:21 PM Kent Gibson [off-list ref] wrote:

On Sat, Aug 15, 2020 at 09:21:22AM +0200, Bartosz Golaszewski wrote:

quoted

On Sat, Aug 15, 2020 at 8:53 AM Kent Gibson [off-list ref] wrote:

quoted

On Fri, Aug 14, 2020 at 09:31:29PM +0200, Bartosz Golaszewski wrote:

quoted

On Fri, Aug 14, 2020 at 5:04 AM Kent Gibson [off-list ref] wrote:

quoted

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]

quoted

quoted

+
+/**
+ * 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.

quoted

I'll listen to other's suggestions/voices but personally I think that
_ctx, _data etc. suffixes actually make sense.

quoted

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.

quoted

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