From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
To: Kent Gibson <warthog618@gmail.com>
Cc: linux-gpio <linux-gpio@vger.kernel.org>,
Linus Walleij <linus.walleij@linaro.org>
Subject: Re: [PATCH] gpio: add new SET_CONFIG ioctl() to gpio chardev
Date: Thu, 31 Oct 2019 08:11:00 +0100 [thread overview]
Message-ID: <CAMpxmJWb=6QQqMWNsDTJKMEr=cd8h1pq+zewc16cFfosFRwmDQ@mail.gmail.com> (raw)
In-Reply-To: <20191028073912.25743-1-warthog618@gmail.com>
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
next prev parent reply other threads:[~2019-10-31 7:11 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-28 7:39 [PATCH] gpio: add new SET_CONFIG ioctl() to gpio chardev Kent Gibson
2019-10-31 7:11 ` Bartosz Golaszewski [this message]
2019-10-31 7:34 ` Kent Gibson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMpxmJWb=6QQqMWNsDTJKMEr=cd8h1pq+zewc16cFfosFRwmDQ@mail.gmail.com' \
--to=bgolaszewski@baylibre.com \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=warthog618@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).