* [PATCH] gpio: add new SET_CONFIG ioctl() to gpio chardev
@ 2019-10-28 7:39 Kent Gibson
2019-10-31 7:11 ` Bartosz Golaszewski
0 siblings, 1 reply; 3+ messages in thread
From: Kent Gibson @ 2019-10-28 7:39 UTC (permalink / raw)
To: linux-gpio, bgolaszewski, linus.walleij; +Cc: Kent Gibson
Add the GPIOHANDLE_SET_CONFIG_IOCTL to the gpio chardev.
The ioctl allows some of the configuration of a requested handle to be
changed without having to release the line.
The primary use case is the changing of direction for bi-directional
lines.
Based on initial work by Bartosz Golaszewski <bgolaszewski@baylibre.com>
Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
The patch is against Bart's gpio/for-next branch[1] with v4 of my bias
patch series "gpio: expose line bias flags to userspace" applied.
This is necessary to support setting of the bias flags introduced
there.
The patch has been successfully tested against gpio-mockup using the
feature/pud_set_config branch of my Go gpiod library[2], as libgpiod has
not yet been updated with the uAPI change.
drivers/gpio/gpiolib.c | 163 ++++++++++++++++++++++++++------------
include/uapi/linux/gpio.h | 18 +++++
2 files changed, 132 insertions(+), 49 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index df27f05f9b8d..62a74e27ad02 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -427,23 +427,74 @@ struct linehandle_state {
GPIOHANDLE_REQUEST_OPEN_DRAIN | \
GPIOHANDLE_REQUEST_OPEN_SOURCE)
+static int linehandle_validate_flags(u32 flags)
+{
+ /* Return an error if an unknown flag is set */
+ if (flags & ~GPIOHANDLE_REQUEST_VALID_FLAGS)
+ return -EINVAL;
+
+ /*
+ * Do not allow both INPUT & OUTPUT flags to be set as they are
+ * contradictory.
+ */
+ if ((flags & GPIOHANDLE_REQUEST_INPUT) &&
+ (flags & GPIOHANDLE_REQUEST_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 & GPIOHANDLE_REQUEST_OPEN_DRAIN) &&
+ (flags & GPIOHANDLE_REQUEST_OPEN_SOURCE))
+ return -EINVAL;
+
+ /* OPEN_DRAIN and OPEN_SOURCE flags only make sense for output mode. */
+ if (!(flags & GPIOHANDLE_REQUEST_OUTPUT) &&
+ ((flags & GPIOHANDLE_REQUEST_OPEN_DRAIN) ||
+ (flags & GPIOHANDLE_REQUEST_OPEN_SOURCE)))
+ return -EINVAL;
+
+ /* Bias flags only allowed for input or output mode. */
+ if (!((flags & GPIOHANDLE_REQUEST_INPUT) ||
+ (flags & GPIOHANDLE_REQUEST_OUTPUT)) &&
+ ((flags & GPIOHANDLE_REQUEST_BIAS_DISABLE) ||
+ (flags & GPIOHANDLE_REQUEST_BIAS_PULL_UP) ||
+ (flags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)))
+ return -EINVAL;
+
+ /* Only one bias flag can be set. */
+ if (((flags & GPIOHANDLE_REQUEST_BIAS_DISABLE) &&
+ (flags & (GPIOHANDLE_REQUEST_BIAS_PULL_DOWN |
+ GPIOHANDLE_REQUEST_BIAS_PULL_UP))) ||
+ ((flags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN) &&
+ (flags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)))
+ return -EINVAL;
+
+ return 0;
+}
+
static long linehandle_ioctl(struct file *filep, unsigned int cmd,
unsigned long arg)
{
struct linehandle_state *lh = filep->private_data;
void __user *ip = (void __user *)arg;
struct gpiohandle_data ghd;
+ struct gpiohandle_config gcnf;
+ struct gpio_desc *desc;
DECLARE_BITMAP(vals, GPIOHANDLES_MAX);
- int i;
+ int i, ret;
+ u32 lflags;
if (cmd == GPIOHANDLE_GET_LINE_VALUES_IOCTL) {
/* NOTE: It's ok to read values of output lines. */
- int ret = gpiod_get_array_value_complex(false,
- true,
- lh->numdescs,
- lh->descs,
- NULL,
- vals);
+ ret = gpiod_get_array_value_complex(false,
+ true,
+ lh->numdescs,
+ lh->descs,
+ NULL,
+ vals);
if (ret)
return ret;
@@ -477,6 +528,59 @@ static long linehandle_ioctl(struct file *filep, unsigned int cmd,
lh->descs,
NULL,
vals);
+ } else if (cmd == GPIOHANDLE_SET_CONFIG_IOCTL) {
+ if (copy_from_user(&gcnf, ip, sizeof(gcnf)))
+ return -EFAULT;
+
+ lflags = gcnf.flags;
+ ret = linehandle_validate_flags(lflags);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < lh->numdescs; i++) {
+ desc = lh->descs[i];
+ if (lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW)
+ set_bit(FLAG_ACTIVE_LOW, &desc->flags);
+ else
+ clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
+ if (lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN)
+ set_bit(FLAG_OPEN_DRAIN, &desc->flags);
+ else
+ clear_bit(FLAG_OPEN_DRAIN, &desc->flags);
+ if (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)
+ set_bit(FLAG_OPEN_SOURCE, &desc->flags);
+ else
+ clear_bit(FLAG_OPEN_SOURCE, &desc->flags);
+ if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)
+ set_bit(FLAG_PULL_UP, &desc->flags);
+ else
+ clear_bit(FLAG_PULL_UP, &desc->flags);
+ if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)
+ set_bit(FLAG_PULL_DOWN, &desc->flags);
+ else
+ clear_bit(FLAG_PULL_DOWN, &desc->flags);
+ if (lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE)
+ set_bit(FLAG_BIAS_DISABLE, &desc->flags);
+ else
+ clear_bit(FLAG_BIAS_DISABLE, &desc->flags);
+
+ /*
+ * Lines have to be requested explicitly for input
+ * or output, else the line will be treated "as is".
+ */
+ if (lflags & GPIOHANDLE_REQUEST_OUTPUT) {
+ int val = !!gcnf.default_values[i];
+
+ ret = gpiod_direction_output(desc, val);
+ if (ret)
+ return ret;
+ } else if (lflags & GPIOHANDLE_REQUEST_INPUT) {
+ ret = gpiod_direction_input(desc);
+ if (ret)
+ return ret;
+ }
+ }
+ return 0;
}
return -EINVAL;
}
@@ -528,48 +632,9 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
lflags = handlereq.flags;
- /* Return an error if an unknown flag is set */
- if (lflags & ~GPIOHANDLE_REQUEST_VALID_FLAGS)
- return -EINVAL;
-
- /*
- * Do not allow both INPUT & OUTPUT flags to be set as they are
- * contradictory.
- */
- if ((lflags & GPIOHANDLE_REQUEST_INPUT) &&
- (lflags & GPIOHANDLE_REQUEST_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 ((lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN) &&
- (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE))
- return -EINVAL;
-
- /* OPEN_DRAIN and OPEN_SOURCE flags only make sense for output mode. */
- if (!(lflags & GPIOHANDLE_REQUEST_OUTPUT) &&
- ((lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN) ||
- (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)))
- return -EINVAL;
-
- /* Bias flags only allowed for input or output mode. */
- if (!((lflags & GPIOHANDLE_REQUEST_INPUT) ||
- (lflags & GPIOHANDLE_REQUEST_OUTPUT)) &&
- ((lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE) ||
- (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP) ||
- (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)))
- return -EINVAL;
-
- /* Only one bias flag can be set. */
- if (((lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE) &&
- (lflags & (GPIOHANDLE_REQUEST_BIAS_PULL_DOWN |
- GPIOHANDLE_REQUEST_BIAS_PULL_UP))) ||
- ((lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN) &&
- (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)))
- return -EINVAL;
+ ret = linehandle_validate_flags(lflags);
+ if (ret)
+ return ret;
lh = kzalloc(sizeof(*lh), GFP_KERNEL);
if (!lh)
diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
index 7cc21c3b0839..799cf823d493 100644
--- a/include/uapi/linux/gpio.h
+++ b/include/uapi/linux/gpio.h
@@ -100,6 +100,24 @@ struct gpiohandle_request {
int fd;
};
+/**
+ * struct gpiohandle_config - Configuration for a GPIO handle request
+ * @flags: updated flags for the requested GPIO lines, such as
+ * GPIOHANDLE_REQUEST_OUTPUT, GPIOHANDLE_REQUEST_ACTIVE_LOW etc, OR:ed
+ * together
+ * @default_values: if the GPIOHANDLE_REQUEST_OUTPUT is set in flags,
+ * this specifies the default output value, should be 0 (low) or
+ * 1 (high), anything else than 0 or 1 will be interpreted as 1 (high)
+ * @padding: reserved for future use and should be zero filled
+ */
+struct gpiohandle_config {
+ __u32 flags;
+ __u8 default_values[GPIOHANDLES_MAX];
+ __u32 padding[4]; /* padding for future use */
+};
+
+#define GPIOHANDLE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x0a, struct gpiohandle_config)
+
/**
* struct gpiohandle_data - Information of values on a GPIO handle
* @values: when getting the state of lines this contains the current
--
2.23.0
[1] git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git
[2] https://github.com/warthog618/gpiod.git
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] gpio: add new SET_CONFIG ioctl() to gpio chardev
2019-10-28 7:39 [PATCH] gpio: add new SET_CONFIG ioctl() to gpio chardev Kent Gibson
@ 2019-10-31 7:11 ` Bartosz Golaszewski
2019-10-31 7:34 ` Kent Gibson
0 siblings, 1 reply; 3+ messages in thread
From: Bartosz Golaszewski @ 2019-10-31 7:11 UTC (permalink / raw)
To: Kent Gibson; +Cc: linux-gpio, Linus Walleij
pon., 28 paź 2019 o 08:39 Kent Gibson <warthog618@gmail.com> napisał(a):
>
> Add the GPIOHANDLE_SET_CONFIG_IOCTL to the gpio chardev.
> The ioctl allows some of the configuration of a requested handle to be
> changed without having to release the line.
> The primary use case is the changing of direction for bi-directional
> lines.
>
> Based on initial work by Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> Signed-off-by: Kent Gibson <warthog618@gmail.com>
> ---
>
> The patch is against Bart's gpio/for-next branch[1] with v4 of my bias
> patch series "gpio: expose line bias flags to userspace" applied.
> This is necessary to support setting of the bias flags introduced
> there.
>
> The patch has been successfully tested against gpio-mockup using the
> feature/pud_set_config branch of my Go gpiod library[2], as libgpiod has
> not yet been updated with the uAPI change.
>
> drivers/gpio/gpiolib.c | 163 ++++++++++++++++++++++++++------------
> include/uapi/linux/gpio.h | 18 +++++
> 2 files changed, 132 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index df27f05f9b8d..62a74e27ad02 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -427,23 +427,74 @@ struct linehandle_state {
> GPIOHANDLE_REQUEST_OPEN_DRAIN | \
> GPIOHANDLE_REQUEST_OPEN_SOURCE)
>
> +static int linehandle_validate_flags(u32 flags)
> +{
> + /* Return an error if an unknown flag is set */
> + if (flags & ~GPIOHANDLE_REQUEST_VALID_FLAGS)
> + return -EINVAL;
> +
> + /*
> + * Do not allow both INPUT & OUTPUT flags to be set as they are
> + * contradictory.
> + */
> + if ((flags & GPIOHANDLE_REQUEST_INPUT) &&
> + (flags & GPIOHANDLE_REQUEST_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 & GPIOHANDLE_REQUEST_OPEN_DRAIN) &&
> + (flags & GPIOHANDLE_REQUEST_OPEN_SOURCE))
> + return -EINVAL;
> +
> + /* OPEN_DRAIN and OPEN_SOURCE flags only make sense for output mode. */
> + if (!(flags & GPIOHANDLE_REQUEST_OUTPUT) &&
> + ((flags & GPIOHANDLE_REQUEST_OPEN_DRAIN) ||
> + (flags & GPIOHANDLE_REQUEST_OPEN_SOURCE)))
> + return -EINVAL;
> +
> + /* Bias flags only allowed for input or output mode. */
> + if (!((flags & GPIOHANDLE_REQUEST_INPUT) ||
> + (flags & GPIOHANDLE_REQUEST_OUTPUT)) &&
> + ((flags & GPIOHANDLE_REQUEST_BIAS_DISABLE) ||
> + (flags & GPIOHANDLE_REQUEST_BIAS_PULL_UP) ||
> + (flags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)))
> + return -EINVAL;
> +
> + /* Only one bias flag can be set. */
> + if (((flags & GPIOHANDLE_REQUEST_BIAS_DISABLE) &&
> + (flags & (GPIOHANDLE_REQUEST_BIAS_PULL_DOWN |
> + GPIOHANDLE_REQUEST_BIAS_PULL_UP))) ||
> + ((flags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN) &&
> + (flags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)))
> + return -EINVAL;
> +
> + return 0;
> +}
> +
This is great but I'd prefer that it be put into a separate patch with
its own description that would come before the other changes.
> static long linehandle_ioctl(struct file *filep, unsigned int cmd,
> unsigned long arg)
> {
> struct linehandle_state *lh = filep->private_data;
> void __user *ip = (void __user *)arg;
> struct gpiohandle_data ghd;
> + struct gpiohandle_config gcnf;
> + struct gpio_desc *desc;
> DECLARE_BITMAP(vals, GPIOHANDLES_MAX);
> - int i;
> + int i, ret;
> + u32 lflags;
>
> if (cmd == GPIOHANDLE_GET_LINE_VALUES_IOCTL) {
> /* NOTE: It's ok to read values of output lines. */
> - int ret = gpiod_get_array_value_complex(false,
> - true,
> - lh->numdescs,
> - lh->descs,
> - NULL,
> - vals);
> + ret = gpiod_get_array_value_complex(false,
> + true,
> + lh->numdescs,
> + lh->descs,
> + NULL,
> + vals);
> if (ret)
> return ret;
>
> @@ -477,6 +528,59 @@ static long linehandle_ioctl(struct file *filep, unsigned int cmd,
> lh->descs,
> NULL,
> vals);
> + } else if (cmd == GPIOHANDLE_SET_CONFIG_IOCTL) {
> + if (copy_from_user(&gcnf, ip, sizeof(gcnf)))
> + return -EFAULT;
> +
> + lflags = gcnf.flags;
> + ret = linehandle_validate_flags(lflags);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < lh->numdescs; i++) {
> + desc = lh->descs[i];
> + if (lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW)
> + set_bit(FLAG_ACTIVE_LOW, &desc->flags);
> + else
> + clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
> + if (lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN)
> + set_bit(FLAG_OPEN_DRAIN, &desc->flags);
> + else
> + clear_bit(FLAG_OPEN_DRAIN, &desc->flags);
> + if (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)
> + set_bit(FLAG_OPEN_SOURCE, &desc->flags);
> + else
> + clear_bit(FLAG_OPEN_SOURCE, &desc->flags);
> + if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)
> + set_bit(FLAG_PULL_UP, &desc->flags);
> + else
> + clear_bit(FLAG_PULL_UP, &desc->flags);
> + if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)
> + set_bit(FLAG_PULL_DOWN, &desc->flags);
> + else
> + clear_bit(FLAG_PULL_DOWN, &desc->flags);
> + if (lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE)
> + set_bit(FLAG_BIAS_DISABLE, &desc->flags);
> + else
> + clear_bit(FLAG_BIAS_DISABLE, &desc->flags);
Could you add some new lines between the if elses? I think it'll be
more readable.
> +
> + /*
> + * Lines have to be requested explicitly for input
> + * or output, else the line will be treated "as is".
> + */
> + if (lflags & GPIOHANDLE_REQUEST_OUTPUT) {
> + int val = !!gcnf.default_values[i];
> +
> + ret = gpiod_direction_output(desc, val);
> + if (ret)
> + return ret;
> + } else if (lflags & GPIOHANDLE_REQUEST_INPUT) {
> + ret = gpiod_direction_input(desc);
> + if (ret)
> + return ret;
> + }
> + }
> + return 0;
> }
> return -EINVAL;
> }
> @@ -528,48 +632,9 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
>
> lflags = handlereq.flags;
>
> - /* Return an error if an unknown flag is set */
> - if (lflags & ~GPIOHANDLE_REQUEST_VALID_FLAGS)
> - return -EINVAL;
> -
> - /*
> - * Do not allow both INPUT & OUTPUT flags to be set as they are
> - * contradictory.
> - */
> - if ((lflags & GPIOHANDLE_REQUEST_INPUT) &&
> - (lflags & GPIOHANDLE_REQUEST_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 ((lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN) &&
> - (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE))
> - return -EINVAL;
> -
> - /* OPEN_DRAIN and OPEN_SOURCE flags only make sense for output mode. */
> - if (!(lflags & GPIOHANDLE_REQUEST_OUTPUT) &&
> - ((lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN) ||
> - (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)))
> - return -EINVAL;
> -
> - /* Bias flags only allowed for input or output mode. */
> - if (!((lflags & GPIOHANDLE_REQUEST_INPUT) ||
> - (lflags & GPIOHANDLE_REQUEST_OUTPUT)) &&
> - ((lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE) ||
> - (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP) ||
> - (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)))
> - return -EINVAL;
> -
> - /* Only one bias flag can be set. */
> - if (((lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE) &&
> - (lflags & (GPIOHANDLE_REQUEST_BIAS_PULL_DOWN |
> - GPIOHANDLE_REQUEST_BIAS_PULL_UP))) ||
> - ((lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN) &&
> - (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)))
> - return -EINVAL;
> + ret = linehandle_validate_flags(lflags);
> + if (ret)
> + return ret;
>
> lh = kzalloc(sizeof(*lh), GFP_KERNEL);
> if (!lh)
> diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
> index 7cc21c3b0839..799cf823d493 100644
> --- a/include/uapi/linux/gpio.h
> +++ b/include/uapi/linux/gpio.h
> @@ -100,6 +100,24 @@ struct gpiohandle_request {
> int fd;
> };
>
> +/**
> + * struct gpiohandle_config - Configuration for a GPIO handle request
> + * @flags: updated flags for the requested GPIO lines, such as
> + * GPIOHANDLE_REQUEST_OUTPUT, GPIOHANDLE_REQUEST_ACTIVE_LOW etc, OR:ed
> + * together
> + * @default_values: if the GPIOHANDLE_REQUEST_OUTPUT is set in flags,
> + * this specifies the default output value, should be 0 (low) or
> + * 1 (high), anything else than 0 or 1 will be interpreted as 1 (high)
> + * @padding: reserved for future use and should be zero filled
> + */
> +struct gpiohandle_config {
> + __u32 flags;
> + __u8 default_values[GPIOHANDLES_MAX];
> + __u32 padding[4]; /* padding for future use */
> +};
> +
> +#define GPIOHANDLE_SET_CONFIG_IOCTL _IOWR(0xB4, 0x0a, struct gpiohandle_config)
> +
> /**
> * struct gpiohandle_data - Information of values on a GPIO handle
> * @values: when getting the state of lines this contains the current
> --
> 2.23.0
>
> [1] git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git
> [2] https://github.com/warthog618/gpiod.git
Bart
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] gpio: add new SET_CONFIG ioctl() to gpio chardev
2019-10-31 7:11 ` Bartosz Golaszewski
@ 2019-10-31 7:34 ` Kent Gibson
0 siblings, 0 replies; 3+ messages in thread
From: Kent Gibson @ 2019-10-31 7:34 UTC (permalink / raw)
To: Bartosz Golaszewski; +Cc: linux-gpio, Linus Walleij
On Thu, Oct 31, 2019 at 08:11:00AM +0100, Bartosz Golaszewski wrote:
> pon., 28 paź 2019 o 08:39 Kent Gibson <warthog618@gmail.com> napisał(a):
> >
> > Add the GPIOHANDLE_SET_CONFIG_IOCTL to the gpio chardev.
> > The ioctl allows some of the configuration of a requested handle to be
> > changed without having to release the line.
> > The primary use case is the changing of direction for bi-directional
> > lines.
> >
> > Based on initial work by Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> > ---
> >
> > The patch is against Bart's gpio/for-next branch[1] with v4 of my bias
> > patch series "gpio: expose line bias flags to userspace" applied.
> > This is necessary to support setting of the bias flags introduced
> > there.
> >
> > The patch has been successfully tested against gpio-mockup using the
> > feature/pud_set_config branch of my Go gpiod library[2], as libgpiod has
> > not yet been updated with the uAPI change.
> >
> > drivers/gpio/gpiolib.c | 163 ++++++++++++++++++++++++++------------
> > include/uapi/linux/gpio.h | 18 +++++
> > 2 files changed, 132 insertions(+), 49 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index df27f05f9b8d..62a74e27ad02 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -427,23 +427,74 @@ struct linehandle_state {
> > GPIOHANDLE_REQUEST_OPEN_DRAIN | \
> > GPIOHANDLE_REQUEST_OPEN_SOURCE)
> >
> > +static int linehandle_validate_flags(u32 flags)
> > +{
> > + /* Return an error if an unknown flag is set */
> > + if (flags & ~GPIOHANDLE_REQUEST_VALID_FLAGS)
> > + return -EINVAL;
> > +
> > + /*
> > + * Do not allow both INPUT & OUTPUT flags to be set as they are
> > + * contradictory.
> > + */
> > + if ((flags & GPIOHANDLE_REQUEST_INPUT) &&
> > + (flags & GPIOHANDLE_REQUEST_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 & GPIOHANDLE_REQUEST_OPEN_DRAIN) &&
> > + (flags & GPIOHANDLE_REQUEST_OPEN_SOURCE))
> > + return -EINVAL;
> > +
> > + /* OPEN_DRAIN and OPEN_SOURCE flags only make sense for output mode. */
> > + if (!(flags & GPIOHANDLE_REQUEST_OUTPUT) &&
> > + ((flags & GPIOHANDLE_REQUEST_OPEN_DRAIN) ||
> > + (flags & GPIOHANDLE_REQUEST_OPEN_SOURCE)))
> > + return -EINVAL;
> > +
> > + /* Bias flags only allowed for input or output mode. */
> > + if (!((flags & GPIOHANDLE_REQUEST_INPUT) ||
> > + (flags & GPIOHANDLE_REQUEST_OUTPUT)) &&
> > + ((flags & GPIOHANDLE_REQUEST_BIAS_DISABLE) ||
> > + (flags & GPIOHANDLE_REQUEST_BIAS_PULL_UP) ||
> > + (flags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)))
> > + return -EINVAL;
> > +
> > + /* Only one bias flag can be set. */
> > + if (((flags & GPIOHANDLE_REQUEST_BIAS_DISABLE) &&
> > + (flags & (GPIOHANDLE_REQUEST_BIAS_PULL_DOWN |
> > + GPIOHANDLE_REQUEST_BIAS_PULL_UP))) ||
> > + ((flags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN) &&
> > + (flags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)))
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
>
> This is great but I'd prefer that it be put into a separate patch with
> its own description that would come before the other changes.
>
Will do.
> > static long linehandle_ioctl(struct file *filep, unsigned int cmd,
> > unsigned long arg)
> > {
> > struct linehandle_state *lh = filep->private_data;
> > void __user *ip = (void __user *)arg;
> > struct gpiohandle_data ghd;
> > + struct gpiohandle_config gcnf;
> > + struct gpio_desc *desc;
> > DECLARE_BITMAP(vals, GPIOHANDLES_MAX);
> > - int i;
> > + int i, ret;
> > + u32 lflags;
> >
> > if (cmd == GPIOHANDLE_GET_LINE_VALUES_IOCTL) {
> > /* NOTE: It's ok to read values of output lines. */
> > - int ret = gpiod_get_array_value_complex(false,
> > - true,
> > - lh->numdescs,
> > - lh->descs,
> > - NULL,
> > - vals);
> > + ret = gpiod_get_array_value_complex(false,
> > + true,
> > + lh->numdescs,
> > + lh->descs,
> > + NULL,
> > + vals);
> > if (ret)
> > return ret;
> >
> > @@ -477,6 +528,59 @@ static long linehandle_ioctl(struct file *filep, unsigned int cmd,
> > lh->descs,
> > NULL,
> > vals);
> > + } else if (cmd == GPIOHANDLE_SET_CONFIG_IOCTL) {
> > + if (copy_from_user(&gcnf, ip, sizeof(gcnf)))
> > + return -EFAULT;
> > +
> > + lflags = gcnf.flags;
> > + ret = linehandle_validate_flags(lflags);
> > + if (ret)
> > + return ret;
> > +
> > + for (i = 0; i < lh->numdescs; i++) {
> > + desc = lh->descs[i];
> > + if (lflags & GPIOHANDLE_REQUEST_ACTIVE_LOW)
> > + set_bit(FLAG_ACTIVE_LOW, &desc->flags);
> > + else
> > + clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
> > + if (lflags & GPIOHANDLE_REQUEST_OPEN_DRAIN)
> > + set_bit(FLAG_OPEN_DRAIN, &desc->flags);
> > + else
> > + clear_bit(FLAG_OPEN_DRAIN, &desc->flags);
> > + if (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)
> > + set_bit(FLAG_OPEN_SOURCE, &desc->flags);
> > + else
> > + clear_bit(FLAG_OPEN_SOURCE, &desc->flags);
> > + if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_UP)
> > + set_bit(FLAG_PULL_UP, &desc->flags);
> > + else
> > + clear_bit(FLAG_PULL_UP, &desc->flags);
> > + if (lflags & GPIOHANDLE_REQUEST_BIAS_PULL_DOWN)
> > + set_bit(FLAG_PULL_DOWN, &desc->flags);
> > + else
> > + clear_bit(FLAG_PULL_DOWN, &desc->flags);
> > + if (lflags & GPIOHANDLE_REQUEST_BIAS_DISABLE)
> > + set_bit(FLAG_BIAS_DISABLE, &desc->flags);
> > + else
> > + clear_bit(FLAG_BIAS_DISABLE, &desc->flags);
>
> Could you add some new lines between the if elses? I think it'll be
> more readable.
>
Agreed. I would prefer to call a function that atomically sets or clears
the flag bit based on the requested state, but there doesn't seem to be one.
Cheers,
Kent.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-10-31 7:34 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-28 7:39 [PATCH] gpio: add new SET_CONFIG ioctl() to gpio chardev Kent Gibson
2019-10-31 7:11 ` Bartosz Golaszewski
2019-10-31 7:34 ` Kent Gibson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.