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 3:21 PM Kent Gibson wrote:
>
> On Sat, Aug 15, 2020 at 09:21:22AM +0200, Bartosz Golaszewski wrote:
> > On Sat, Aug 15, 2020 at 8:53 AM Kent Gibson 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
> > > > 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
> > > > > ---
> > > >
> > > > 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.
> >
>
> 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.
> > 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.
> >
>
> 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
Re: [PATCH v4 07/20] gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL and GPIO_V2_LINE_GET_VALUES_IOCTL
On Fri, Aug 14, 2020 at 09:31:29PM +0200, Bartosz Golaszewski wrote:
> On Fri, Aug 14, 2020 at 5:04 AM Kent Gibson 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
> > ---
>
> 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.
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 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 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
> > > ---
> >
> > 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.
>
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 09:21:22AM +0200, Bartosz Golaszewski wrote:
> On Sat, Aug 15, 2020 at 8:53 AM Kent Gibson 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 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
> > > > ---
> > >
> > > 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.
>
My counter example to both points is "struct file *file".
> 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.
>
And struct line_data isn't?
Cheers,
Kent.
Re: [PATCH v4 07/20] gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL and GPIO_V2_LINE_GET_VALUES_IOCTL
On Fri, Aug 14, 2020 at 5:04 AM Kent Gibson 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
> ---
Hi Kent,
not many comments here, just a couple minor details below.
>
> The struct line implementation is based on the v1 struct linehandle
> implementation.
>
> The line_ioctl() is a simple wrapper around line_get_values() here, but
> will be extended with other ioctls in subsequent patches.
>
> Changes for v4:
> - fix handling of mask in line_get_values
>
> drivers/gpio/gpiolib-cdev.c | 413
> 1 file changed, 413 insertions(+)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index 8b012879fe3f..8671e04ff989 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -1,7 +1,9 @@
> // SPDX-License-Identifier: GPL-2.0
>
> #include
> +#include
> #include
> +#include
> #include
> #include
> #include
> @@ -34,6 +36,7 @@
> * GPIO line handle management
> */
>
> +#ifdef CONFIG_GPIO_CDEV_V1
> /**
> * struct linehandle_state - contains the state of a userspace handle
> * @gdev: the GPIO device the handle pertains to
> @@ -376,6 +379,390 @@ static int linehandle_create(struct gpio_device *gdev,
> void __user *ip)
> linehandle_free(lh);
> return ret;
> }
> +#endif /* CONFIG_GPIO_CDEV_V1 */
> +
> +/**
> + * 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.
> + struct gpio_device *gdev;
> + const char *label;
> + u32 num_descs;
> + struct gpio_desc *descs[];
> +};
> +
> +#define GPIO_V2_LINE_BIAS_FLAGS \
> + (GPIO_V2_LINE_FLAG_BIAS_PULL_UP | \
> +GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN | \
> +GPIO_V2_LINE_FLAG_BIAS_DISABLED)
> +
> +#define GPIO_V2_LINE_DIRECTION_FLAGS \
> + (GPIO_V2_LINE_FLAG_INPUT | \
> +GPIO_V2_LINE_FLAG_OUTPUT)
> +
> +#define GPIO_V2_LINE_DRIVE_FLAGS \
> + (GPIO_V2_LINE_FLAG_OPEN_DRAIN | \
> +GPIO_V2_LINE_FLAG_OPEN_SOURCE)
> +
> +#define GPIO_V2_LINE_VALID_FLAGS \
> + (GPIO_V2_LINE_FLAG_ACTIVE_LOW | \
> +GPIO_V2_LINE_DIRECTION_FLAGS | \
> +GPIO_V2_LINE_DRIVE_FLAGS | \
> +GPIO_V2_LINE_BIAS_FLAGS)
> +
> +static u64 gpio_v2_line_config_flags(struct gpio_v2_line_config *lc,
> +int line_idx)
> +{
> + int i;
> + u64 mask = BIT_ULL(line_idx);
> +
> + for (i = 0; i < lc->num_attrs; i++) {
> + if ((lc->attrs[i].attr.id == GPIO_V2_LINE_ATTR_ID_FLAGS) &&
> + (lc->attrs[i].mask & mask))
> + return lc->attrs[i].attr.flags;
> + }
> + return lc->flags;
> +}
> +
> +static int gpio_v2_line_config_output_value(struct gpio_v2_line_config *lc,
> + int line_idx)
> +{
> + int i;
> + u64 mask = BIT_ULL(line_idx);
> +
> + for (i = 0; i < lc->num_attrs; i++) {
> + if ((lc->attrs[i].attr.id ==
> GPIO_V2_LINE_ATTR_ID_OUTPUT_VALUES) &&
> + (lc->attrs[i].mask & mask))
> + return !!(lc->attrs[i].attr.values & mask);
> + }
> + return 0;
> +}
> +
> +static int gpio_v2_line_flags_validate(u64 flags)
> +{
> + /* Return an error if an unknown flag is set */
> + if (flags & ~GPIO_V2_LINE_VALID_FLAGS)
> + return -EINVAL;
> +
> + /*
> +* Do not allow both INPUT & OUTPUT flags to be set as they are
> +* contradictory.
> +*/
> + if ((flags & GPIO_V2_LINE_FLAG_INPUT) &&
> + (flags & GPIO_V2_LINE_FLAG_OUTPUT))
> + return -EINVAL;
> +
> + /*
> +* Do not allow OPEN_SOURCE & OPEN_DRAIN flags in a single request. If
> +* the hardware actually supports enabling both at the same time the
> +* electrical result would be disastrous.
> +*/
> + if ((flags & GPIO_V2_LINE_FLAG_OPEN_DRAIN) &&
> + (flags & GPIO_V2_LINE_FLAG_OPEN_SOURCE))
> + return -EINVAL;
> +
> + /* Drive requires explicit output direction. */
> + if ((flags & GPIO_V2_LINE_DRIVE_FLAGS) &&
> + !(flags & GPIO_V2_LINE_FLAG_OUTPUT))
> + return -EINVAL;
> +
> + /* Bias requies explicit direction. */
s/requies/requires/
> + if ((flags & GPIO_V2_LINE_BIAS_FLAGS) &&
> + !(flags &
[PATCH v4 07/20] gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL and GPIO_V2_LINE_GET_VALUES_IOCTL
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
---
The struct line implementation is based on the v1 struct linehandle
implementation.
The line_ioctl() is a simple wrapper around line_get_values() here, but
will be extended with other ioctls in subsequent patches.
Changes for v4:
- fix handling of mask in line_get_values
drivers/gpio/gpiolib-cdev.c | 413
1 file changed, 413 insertions(+)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index 8b012879fe3f..8671e04ff989 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1,7 +1,9 @@
// SPDX-License-Identifier: GPL-2.0
#include
+#include
#include
+#include
#include
#include
#include
@@ -34,6 +36,7 @@
* GPIO line handle management
*/
+#ifdef CONFIG_GPIO_CDEV_V1
/**
* struct linehandle_state - contains the state of a userspace handle
* @gdev: the GPIO device the handle pertains to
@@ -376,6 +379,390 @@ static int linehandle_create(struct gpio_device *gdev,
void __user *ip)
linehandle_free(lh);
return ret;
}
+#endif /* CONFIG_GPIO_CDEV_V1 */
+
+/**
+ * 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 {
+ struct gpio_device *gdev;
+ const char *label;
+ u32 num_descs;
+ struct gpio_desc *descs[];
+};
+
+#define GPIO_V2_LINE_BIAS_FLAGS \
+ (GPIO_V2_LINE_FLAG_BIAS_PULL_UP | \
+GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN | \
+GPIO_V2_LINE_FLAG_BIAS_DISABLED)
+
+#define GPIO_V2_LINE_DIRECTION_FLAGS \
+ (GPIO_V2_LINE_FLAG_INPUT | \
+GPIO_V2_LINE_FLAG_OUTPUT)
+
+#define GPIO_V2_LINE_DRIVE_FLAGS \
+ (GPIO_V2_LINE_FLAG_OPEN_DRAIN | \
+GPIO_V2_LINE_FLAG_OPEN_SOURCE)
+
+#define GPIO_V2_LINE_VALID_FLAGS \
+ (GPIO_V2_LINE_FLAG_ACTIVE_LOW | \
+GPIO_V2_LINE_DIRECTION_FLAGS | \
+GPIO_V2_LINE_DRIVE_FLAGS | \
+GPIO_V2_LINE_BIAS_FLAGS)
+
+static u64 gpio_v2_line_config_flags(struct gpio_v2_line_config *lc,
+int line_idx)
+{
+ int i;
+ u64 mask = BIT_ULL(line_idx);
+
+ for (i = 0; i < lc->num_attrs; i++) {
+ if ((lc->attrs[i].attr.id == GPIO_V2_LINE_ATTR_ID_FLAGS) &&
+ (lc->attrs[i].mask & mask))
+ return lc->attrs[i].attr.flags;
+ }
+ return lc->flags;
+}
+
+static int gpio_v2_line_config_output_value(struct gpio_v2_line_config *lc,
+ int line_idx)
+{
+ int i;
+ u64 mask = BIT_ULL(line_idx);
+
+ for (i = 0; i < lc->num_attrs; i++) {
+ if ((lc->attrs[i].attr.id ==
GPIO_V2_LINE_ATTR_ID_OUTPUT_VALUES) &&
+ (lc->attrs[i].mask & mask))
+ return !!(lc->attrs[i].attr.values & mask);
+ }
+ return 0;
+}
+
+static int gpio_v2_line_flags_validate(u64 flags)
+{
+ /* Return an error if an unknown flag is set */
+ if (flags & ~GPIO_V2_LINE_VALID_FLAGS)
+ return -EINVAL;
+
+ /*
+* Do not allow both INPUT & OUTPUT flags to be set as they are
+* contradictory.
+*/
+ if ((flags & GPIO_V2_LINE_FLAG_INPUT) &&
+ (flags & GPIO_V2_LINE_FLAG_OUTPUT))
+ return -EINVAL;
+
+ /*
+* Do not allow OPEN_SOURCE & OPEN_DRAIN flags in a single request. If
+* the hardware actually supports enabling both at the same time the
+* electrical result would be disastrous.
+*/
+ if ((flags & GPIO_V2_LINE_FLAG_OPEN_DRAIN) &&
+ (flags & GPIO_V2_LINE_FLAG_OPEN_SOURCE))
+ return -EINVAL;
+
+ /* Drive requires explicit output direction. */
+ if ((flags & GPIO_V2_LINE_DRIVE_FLAGS) &&
+ !(flags & GPIO_V2_LINE_FLAG_OUTPUT))
+ return -EINVAL;
+
+ /* Bias requies explicit direction. */
+ if ((flags & GPIO_V2_LINE_BIAS_FLAGS) &&
+ !(flags & GPIO_V2_LINE_DIRECTION_FLAGS))
+ return -EINVAL;
+
+ /* Only one bias flag can be set. */
+ if (((flags & GPIO_V2_LINE_FLAG_BIAS_DISABLED) &&
+(flags & (GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN |
+ GPIO_V2_LINE_FLAG_BIAS_PULL_UP))) ||
+ ((flags & GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN) &&
+(flags & GPIO_V2_LINE_FLAG_BIAS_PULL_UP)))
+ return -EINVAL;
+
+ return 0;
+}
+
+static int gpio_v2_line_config_validate(struct gpio_v2_line_config *lc,
+ int num_lines)
+{
+ int i, ret;
+ u64
