Messages in this thread | | | From | Bartosz Golaszewski <> | Date | Sat, 15 Aug 2020 09:21:22 +0200 | Subject | Re: [PATCH v4 07/20] gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL and GPIO_V2_LINE_GET_VALUES_IOCTL |
| |
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.
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.
Bart
> No problems with your other comments. > > Cheers, > Kent. >
| |