[PATCHv2] gpio: add simple get/set helpers for GPIO lines

[PATCHv2] gpio: add simple get/set helpers for GPIO lines

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

[-- Attachment #1: Type: text/plain, Size: 8854 bytes --]

The Linux kernel GPIO api operates with chips, lines, handles, and
events.

The chip and line structures represent info about gpio chips, and
gpio lines, respectively. They are used to e.g. lookup a line with a
certain name, and/or line flags.

The handle structure is used to "obtain" a handle to one or more gpio
lines on a chip. Until the file descriptor in this handle is closed, the
gpio lines cannot be used by others. The same file descriptor is used
when setting or getting the line values (one can also set the initial
value when obtaining handles for output lines).

The event structure is used to get a file descriptor that can be used
with select/poll to wait for changes in line levels.

This commit add simple support for setting and getting the value for a
single gpio line. It does so by obtaining a line handle to get/set the
value, and then release the handle immediately again.

Functionality that could be implemented, but is postponed until the need
arises includes:

 * looking up a gpio line by its name
 * setting/getting multiple gpio lines with a single function
 * waiting for events
 * holding on to handles

Some of the above probably require adding structures to represent gpio
lines and events, while handles should be private to the class.
---

Changes since v1:
 * added gpiochip info and corresponding getters
 * changed "line" to "line_num" a few places

Changes since RFC:
 * added gpio.h to ell.h
 * changed copyright to Geanix
 * open gpiochip in l_gpio_chip_new()
 * open gpiochip read-only
 * added input checks
 * clear ioctl structs with memset instead of = {0}
 * reorder error-paths

 Makefile.am |   6 +-
 ell/ell.h   |   1 +
 ell/ell.sym |   7 +++
 ell/gpio.c  | 175 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 ell/gpio.h  |  49 +++++++++++++++
 5 files changed, 236 insertions(+), 2 deletions(-)
 create mode 100644 ell/gpio.c
 create mode 100644 ell/gpio.h

diff --git a/Makefile.am b/Makefile.am
index 8401972..0ecb9a1 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -51,7 +51,8 @@ pkginclude_HEADERS = ell/ell.h \
 			ell/dhcp.h \
 			ell/cert.h \
 			ell/ecc.h \
-			ell/ecdh.h
+			ell/ecdh.h \
+			ell/gpio.h
 
 lib_LTLIBRARIES = ell/libell.la
 
@@ -119,7 +120,8 @@ ell_libell_la_SOURCES = $(linux_headers) \
 			ell/ecc.h \
 			ell/ecc-external.c \
 			ell/ecc.c \
-			ell/ecdh.c
+			ell/ecdh.c \
+			ell/gpio.c
 
 ell_libell_la_LDFLAGS = -no-undefined \
 			-Wl,--version-script=$(top_srcdir)/ell/ell.sym \
diff --git a/ell/ell.h b/ell/ell.h
index aab6417..fb1dd79 100644
--- a/ell/ell.h
+++ b/ell/ell.h
@@ -59,3 +59,4 @@
 #include <ell/cert.h>
 #include <ell/ecc.h>
 #include <ell/ecdh.h>
+#include <ell/gpio.h>
diff --git a/ell/ell.sym b/ell/ell.sym
index 841bc49..793e4c3 100644
--- a/ell/ell.sym
+++ b/ell/ell.sym
@@ -463,6 +463,13 @@ global:
 	/* ecdh */
 	l_ecdh_generate_key_pair;
 	l_ecdh_generate_shared_secret;
+	/* gpio */
+	l_gpio_chip_new;
+	l_gpio_chip_free;
+	l_gpio_chip_get_label;
+	l_gpio_chip_get_num_lines;
+	l_gpio_chip_get_line_value;
+	l_gpio_chip_set_line_value;
 local:
 	*;
 };
diff --git a/ell/gpio.c b/ell/gpio.c
new file mode 100644
index 0000000..83032f9
--- /dev/null
+++ b/ell/gpio.c
@@ -0,0 +1,175 @@
+/*
+ *
+ *  Embedded Linux library
+ *
+ *  Copyright (C) 2018 Geanix. All rights reserved.
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <unistd.h>
+#include <sys/ioctl.h>
+#include <fcntl.h>
+#include <linux/gpio.h>
+
+#include "util.h"
+#include "gpio.h"
+#include "private.h"
+
+struct l_gpio_chip {
+	int fd;
+	char *label;
+	uint32_t num_lines;
+};
+
+LIB_EXPORT struct l_gpio_chip *l_gpio_chip_new(const char *chip_name)
+{
+	struct l_gpio_chip *chip;
+	struct gpiochip_info info;
+	char *path;
+	int ret;
+
+	if (!chip_name)
+		return NULL;
+
+	chip = l_new(struct l_gpio_chip, 1);
+
+	path = l_strdup_printf("/dev/%s", chip_name);
+	chip->fd = open(path, O_RDONLY | O_CLOEXEC);
+	l_free(path);
+
+	if (chip->fd < 0) {
+		l_free(chip);
+		return NULL;
+	}
+
+	memset(&info, 0, sizeof(info));
+
+	ret = ioctl(chip->fd, GPIO_GET_CHIPINFO_IOCTL, &info);
+	if (ret < 0) {
+		l_free(chip);
+		return NULL;
+	}
+
+	if (info.label)
+		chip->label = l_strndup(info.label, sizeof(info.label));
+
+	chip->num_lines = info.lines;
+
+	return chip;
+}
+
+LIB_EXPORT void l_gpio_chip_free(struct l_gpio_chip *chip)
+{
+	if (!chip)
+		return;
+
+	if (chip->fd >= 0)
+		close(chip->fd);
+
+	l_free(chip->label);
+	l_free(chip);
+}
+
+LIB_EXPORT const char *l_gpio_chip_get_label(struct l_gpio_chip *chip)
+{
+	if (!chip)
+		return NULL;
+
+	return chip->label;
+}
+
+LIB_EXPORT uint32_t l_gpio_chip_get_num_lines(struct l_gpio_chip *chip)
+{
+	if (!chip)
+		return 0;
+
+	return chip->num_lines;
+}
+
+LIB_EXPORT bool l_gpio_chip_get_line_value(struct l_gpio_chip *chip,
+						uint32_t line_num, bool *value)
+{
+	struct gpiohandle_request req;
+	struct gpiohandle_data data;
+	int ret;
+
+	if (!chip)
+		return false;
+
+	if (line_num >= chip->num_lines)
+		return false;
+
+	if (chip->fd < 0)
+		return false;
+
+	memset(&req, 0, sizeof(req));
+	req.lineoffsets[0] = line_num;
+	req.lines = 1;
+	req.flags = GPIOHANDLE_REQUEST_INPUT;
+
+	ret = ioctl(chip->fd, GPIO_GET_LINEHANDLE_IOCTL, &req);
+	if (ret < 0 || req.fd <= 0)
+		return false;
+
+	memset(&data, 0, sizeof(data));
+
+	ret = ioctl(req.fd, GPIOHANDLE_GET_LINE_VALUES_IOCTL, &data);
+
+	close(req.fd);
+
+	if (ret < 0)
+		return false;
+
+	if (value)
+		*value = !!data.values[0];
+
+	return true;
+}
+
+LIB_EXPORT bool l_gpio_chip_set_line_value(struct l_gpio_chip *chip,
+						uint32_t line_num, bool value)
+{
+	struct gpiohandle_request req;
+	int ret;
+
+	if (!chip)
+		return false;
+
+	if (line_num >= chip->num_lines)
+		return false;
+
+	if (chip->fd < 0)
+		return false;
+
+	memset(&req, 0, sizeof(req));
+	req.lineoffsets[0] = line_num;
+	req.lines = 1;
+	req.flags = GPIOHANDLE_REQUEST_OUTPUT;
+	req.default_values[0] = value;
+
+	ret = ioctl(chip->fd, GPIO_GET_LINEHANDLE_IOCTL, &req);
+	if (ret < 0 || req.fd <= 0)
+		return false;
+
+	close(req.fd);
+
+	return true;
+}
diff --git a/ell/gpio.h b/ell/gpio.h
new file mode 100644
index 0000000..9f4ae6a
--- /dev/null
+++ b/ell/gpio.h
@@ -0,0 +1,49 @@
+/*
+ *
+ *  Embedded Linux library
+ *
+ *  Copyright (C) 2011-2018  Intel Corporation. All rights reserved.
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifndef __ELL_GPIO_H
+#define __ELL_GPIO_H
+
+#include <stdint.h>
+#include <stdbool.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+struct l_gpio_chip;
+
+struct l_gpio_chip *l_gpio_chip_new(const char *chip_name);
+void l_gpio_chip_free(struct l_gpio_chip *chip);
+
+const char *l_gpio_chip_get_label(struct l_gpio_chip *chip);
+uint32_t l_gpio_chip_get_num_lines(struct l_gpio_chip *chip);
+bool l_gpio_chip_get_line_value(struct l_gpio_chip *chip, uint32_t line_num,
+				bool *value);
+bool l_gpio_chip_set_line_value(struct l_gpio_chip *chip, uint32_t line_num,
+				bool value);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* __ELL_GPIO_H */
-- 
2.20.0

Re: [PATCHv2] gpio: add simple get/set helpers for GPIO lines

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

[-- Attachment #1: Type: text/plain, Size: 10658 bytes --]



On 14/12/2018 14.13, Martin Hundebøll wrote:
> The Linux kernel GPIO api operates with chips, lines, handles, and
> events.
> 
> The chip and line structures represent info about gpio chips, and
> gpio lines, respectively. They are used to e.g. lookup a line with a
> certain name, and/or line flags.
> 
> The handle structure is used to "obtain" a handle to one or more gpio
> lines on a chip. Until the file descriptor in this handle is closed, the
> gpio lines cannot be used by others. The same file descriptor is used
> when setting or getting the line values (one can also set the initial
> value when obtaining handles for output lines).
> 
> The event structure is used to get a file descriptor that can be used
> with select/poll to wait for changes in line levels.
> 
> This commit add simple support for setting and getting the value for a
> single gpio line. It does so by obtaining a line handle to get/set the
> value, and then release the handle immediately again.
> 
> Functionality that could be implemented, but is postponed until the need
> arises includes:
> 
>   * looking up a gpio line by its name
>   * setting/getting multiple gpio lines with a single function
>   * waiting for events
>   * holding on to handles
> 
> Some of the above probably require adding structures to represent gpio
> lines and events, while handles should be private to the class.
> ---
> 
> Changes since v1:
>   * added gpiochip info and corresponding getters
>   * changed "line" to "line_num" a few places
> 
> Changes since RFC:
>   * added gpio.h to ell.h
>   * changed copyright to Geanix
>   * open gpiochip in l_gpio_chip_new()
>   * open gpiochip read-only
>   * added input checks
>   * clear ioctl structs with memset instead of = {0}
>   * reorder error-paths
> 
>   Makefile.am |   6 +-
>   ell/ell.h   |   1 +
>   ell/ell.sym |   7 +++
>   ell/gpio.c  | 175 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   ell/gpio.h  |  49 +++++++++++++++
>   5 files changed, 236 insertions(+), 2 deletions(-)
>   create mode 100644 ell/gpio.c
>   create mode 100644 ell/gpio.h
> 
> diff --git a/Makefile.am b/Makefile.am
> index 8401972..0ecb9a1 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -51,7 +51,8 @@ pkginclude_HEADERS = ell/ell.h \
>   			ell/dhcp.h \
>   			ell/cert.h \
>   			ell/ecc.h \
> -			ell/ecdh.h
> +			ell/ecdh.h \
> +			ell/gpio.h
>   
>   lib_LTLIBRARIES = ell/libell.la
>   
> @@ -119,7 +120,8 @@ ell_libell_la_SOURCES = $(linux_headers) \
>   			ell/ecc.h \
>   			ell/ecc-external.c \
>   			ell/ecc.c \
> -			ell/ecdh.c
> +			ell/ecdh.c \
> +			ell/gpio.c
>   
>   ell_libell_la_LDFLAGS = -no-undefined \
>   			-Wl,--version-script=$(top_srcdir)/ell/ell.sym \
> diff --git a/ell/ell.h b/ell/ell.h
> index aab6417..fb1dd79 100644
> --- a/ell/ell.h
> +++ b/ell/ell.h
> @@ -59,3 +59,4 @@
>   #include <ell/cert.h>
>   #include <ell/ecc.h>
>   #include <ell/ecdh.h>
> +#include <ell/gpio.h>
> diff --git a/ell/ell.sym b/ell/ell.sym
> index 841bc49..793e4c3 100644
> --- a/ell/ell.sym
> +++ b/ell/ell.sym
> @@ -463,6 +463,13 @@ global:
>   	/* ecdh */
>   	l_ecdh_generate_key_pair;
>   	l_ecdh_generate_shared_secret;
> +	/* gpio */
> +	l_gpio_chip_new;
> +	l_gpio_chip_free;
> +	l_gpio_chip_get_label;
> +	l_gpio_chip_get_num_lines;
> +	l_gpio_chip_get_line_value;
> +	l_gpio_chip_set_line_value;
>   local:
>   	*;
>   };
> diff --git a/ell/gpio.c b/ell/gpio.c
> new file mode 100644
> index 0000000..83032f9
> --- /dev/null
> +++ b/ell/gpio.c
> @@ -0,0 +1,175 @@
> +/*
> + *
> + *  Embedded Linux library
> + *
> + *  Copyright (C) 2018 Geanix. All rights reserved.
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2.1 of the License, or (at your option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  Lesser General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Lesser General Public
> + *  License along with this library; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <unistd.h>
> +#include <sys/ioctl.h>
> +#include <fcntl.h>
> +#include <linux/gpio.h>
> +
> +#include "util.h"
> +#include "gpio.h"
> +#include "private.h"
> +
> +struct l_gpio_chip {
> +	int fd;
> +	char *label;
> +	uint32_t num_lines;
> +};
> +
> +LIB_EXPORT struct l_gpio_chip *l_gpio_chip_new(const char *chip_name)
> +{
> +	struct l_gpio_chip *chip;
> +	struct gpiochip_info info;
> +	char *path;
> +	int ret;
> +
> +	if (!chip_name)
> +		return NULL;
> +
> +	chip = l_new(struct l_gpio_chip, 1);
> +
> +	path = l_strdup_printf("/dev/%s", chip_name);
> +	chip->fd = open(path, O_RDONLY | O_CLOEXEC);
> +	l_free(path);
> +
> +	if (chip->fd < 0) {
> +		l_free(chip);
> +		return NULL;
> +	}
> +
> +	memset(&info, 0, sizeof(info));
> +
> +	ret = ioctl(chip->fd, GPIO_GET_CHIPINFO_IOCTL, &info);
> +	if (ret < 0) {
> +		l_free(chip);
> +		return NULL;
> +	}
> +
> +	if (info.label)
> +		chip->label = l_strndup(info.label, sizeof(info.label));
> +
> +	chip->num_lines = info.lines;
> +
> +	return chip;
> +}
> +
> +LIB_EXPORT void l_gpio_chip_free(struct l_gpio_chip *chip)
> +{
> +	if (!chip)
> +		return;
> +
> +	if (chip->fd >= 0)
> +		close(chip->fd);
> +
> +	l_free(chip->label);
> +	l_free(chip);
> +}
> +
> +LIB_EXPORT const char *l_gpio_chip_get_label(struct l_gpio_chip *chip)
> +{
> +	if (!chip)
> +		return NULL;
> +
> +	return chip->label;
> +}
> +
> +LIB_EXPORT uint32_t l_gpio_chip_get_num_lines(struct l_gpio_chip *chip)
> +{
> +	if (!chip)
> +		return 0;
> +
> +	return chip->num_lines;
> +}
> +
> +LIB_EXPORT bool l_gpio_chip_get_line_value(struct l_gpio_chip *chip,
> +						uint32_t line_num, bool *value)
> +{
> +	struct gpiohandle_request req;
> +	struct gpiohandle_data data;
> +	int ret;
> +
> +	if (!chip)
> +		return false;
> +
> +	if (line_num >= chip->num_lines)
> +		return false;
> +
> +	if (chip->fd < 0)
> +		return false;
> +
> +	memset(&req, 0, sizeof(req));
> +	req.lineoffsets[0] = line_num;
> +	req.lines = 1;
> +	req.flags = GPIOHANDLE_REQUEST_INPUT;
> +
> +	ret = ioctl(chip->fd, GPIO_GET_LINEHANDLE_IOCTL, &req);
> +	if (ret < 0 || req.fd <= 0)
> +		return false;
> +
> +	memset(&data, 0, sizeof(data));
> +
> +	ret = ioctl(req.fd, GPIOHANDLE_GET_LINE_VALUES_IOCTL, &data);
> +
> +	close(req.fd);
> +
> +	if (ret < 0)
> +		return false;
> +
> +	if (value)
> +		*value = !!data.values[0];
> +
> +	return true;
> +}
> +
> +LIB_EXPORT bool l_gpio_chip_set_line_value(struct l_gpio_chip *chip,
> +						uint32_t line_num, bool value)
> +{
> +	struct gpiohandle_request req;
> +	int ret;
> +
> +	if (!chip)
> +		return false;
> +
> +	if (line_num >= chip->num_lines)
> +		return false;
> +
> +	if (chip->fd < 0)
> +		return false;
> +
> +	memset(&req, 0, sizeof(req));
> +	req.lineoffsets[0] = line_num;
> +	req.lines = 1;
> +	req.flags = GPIOHANDLE_REQUEST_OUTPUT;
> +	req.default_values[0] = value;
> +
> +	ret = ioctl(chip->fd, GPIO_GET_LINEHANDLE_IOCTL, &req);
> +	if (ret < 0 || req.fd <= 0)
> +		return false;
> +
> +	close(req.fd);
> +
> +	return true;
> +}

I am not sure if the l_gpio_chip_{set,get}_line_value() functions are 
the right approach.

I am working on a (ofono) user for this class, where I need to lookup a 
line, and store it in a private data structure. With the l_gpio_chip + 
line number approach I need to have two gpio-related members in my 
private struct.

If we instead required callers to first create a l_gpio_chip instance, 
and use that to get l_gpio_line instances, then we could store a 
reference to the chip in the line instance, which would make it simpler 
for users to keep a reference to line.

That creates a new set of problems though, where l_gpio_line instances 
can get dangling references to freed l_gpio_chip instances. Maybe a list 
of references to "child" lines in l_gpio_chip could be maintained and 
freed when freeing the l_gpio_chip "parent".

Any thoughts?

// Martin

> diff --git a/ell/gpio.h b/ell/gpio.h
> new file mode 100644
> index 0000000..9f4ae6a
> --- /dev/null
> +++ b/ell/gpio.h
> @@ -0,0 +1,49 @@
> +/*
> + *
> + *  Embedded Linux library
> + *
> + *  Copyright (C) 2011-2018  Intel Corporation. All rights reserved.
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2.1 of the License, or (at your option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  Lesser General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Lesser General Public
> + *  License along with this library; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#ifndef __ELL_GPIO_H
> +#define __ELL_GPIO_H
> +
> +#include <stdint.h>
> +#include <stdbool.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +struct l_gpio_chip;
> +
> +struct l_gpio_chip *l_gpio_chip_new(const char *chip_name);
> +void l_gpio_chip_free(struct l_gpio_chip *chip);
> +
> +const char *l_gpio_chip_get_label(struct l_gpio_chip *chip);
> +uint32_t l_gpio_chip_get_num_lines(struct l_gpio_chip *chip);
> +bool l_gpio_chip_get_line_value(struct l_gpio_chip *chip, uint32_t line_num,
> +				bool *value);
> +bool l_gpio_chip_set_line_value(struct l_gpio_chip *chip, uint32_t line_num,
> +				bool value);
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* __ELL_GPIO_H */
> 

-- 
Kind regards,
Martin Hundebøll
Embedded Linux Consultant

+45 61 65 54 61
martin(a)geanix.com

Geanix IVS
https://geanix.com
DK39600706

Re: [PATCHv2] gpio: add simple get/set helpers for GPIO lines

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1529 bytes --]

Hi Martin,

> I am not sure if the l_gpio_chip_{set,get}_line_value() functions are 
> the right approach.
> 
> I am working on a (ofono) user for this class, where I need to lookup a 
> line, and store it in a private data structure. With the l_gpio_chip + 
> line number approach I need to have two gpio-related members in my 
> private struct.
> 

Never worked with GPIOs, so this may be a silly question, but...

How does one figure out the line number that is needed?  You just have 
to know based on some docs?

> If we instead required callers to first create a l_gpio_chip instance, 
> and use that to get l_gpio_line instances, then we could store a 
> reference to the chip in the line instance, which would make it simpler 
> for users to keep a reference to line.

Possible, but the question is how does the kernel behave...

> 
> That creates a new set of problems though, where l_gpio_line instances 
> can get dangling references to freed l_gpio_chip instances. Maybe a list 
> of references to "child" lines in l_gpio_chip could be maintained and 
> freed when freeing the l_gpio_chip "parent".
> 

You have the main /dev/chip file descriptor and you have a file 
descriptor returned from the GPIO_GET_LINEHANDLE_IOCTL.  What happens to 
the fds returned by GPIO_GET_LINEHANDLE_IOCTL if the main /dev/chip fd 
is closed?

There's no hotplug/unplug with gpio devices right?  So you can count of 
the /dev/chip & fd to be owned exlusively by the ell client?

Regards,
-Denis

Re: [PATCHv2] gpio: add simple get/set helpers for GPIO lines

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

[-- Attachment #1: Type: text/plain, Size: 2245 bytes --]

Hi Denis,

On 14/12/2018 18.18, Denis Kenzior wrote:
>> I am not sure if the l_gpio_chip_{set,get}_line_value() functions are 
>> the right approach.
>>
>> I am working on a (ofono) user for this class, where I need to lookup 
>> a line, and store it in a private data structure. With the l_gpio_chip 
>> + line number approach I need to have two gpio-related members in my 
>> private struct.
>>
> 
> Never worked with GPIOs, so this may be a silly question, but...
> 
> How does one figure out the line number that is needed?  You just have 
> to know based on some docs?

The device tree can configure labels for each line that one can 
read/compare using GPIO_GET_LINEINFO_IOCTL. If that is not the case, one 
would have to either read the docs (often the schematic) or do 
trial'n'error.

>> If we instead required callers to first create a l_gpio_chip instance, 
>> and use that to get l_gpio_line instances, then we could store a 
>> reference to the chip in the line instance, which would make it 
>> simpler for users to keep a reference to line.
> 
> Possible, but the question is how does the kernel behave...
> 
>>
>> That creates a new set of problems though, where l_gpio_line instances 
>> can get dangling references to freed l_gpio_chip instances. Maybe a 
>> list of references to "child" lines in l_gpio_chip could be maintained 
>> and freed when freeing the l_gpio_chip "parent".
>>
> 
> You have the main /dev/chip file descriptor and you have a file 
> descriptor returned from the GPIO_GET_LINEHANDLE_IOCTL.  What happens to 
> the fds returned by GPIO_GET_LINEHANDLE_IOCTL if the main /dev/chip fd 
> is closed?

The virtual fds from GPIO_GET_LINEHANDLE_IOCTL stays valid after closing 
the /dev/gpiochipX fd. (So I should probably close those virtual fds 
after getting/setting the values.)

> There's no hotplug/unplug with gpio devices right?  So you can count of 
> the /dev/chip & fd to be owned exlusively by the ell client?

You can get USB gpio expanders:
http://nanorivertech.com/viperboard.html
So I guess /dev/gpiochipX nodes can come and go.

Multiple processes can use the same gpiochip, but only one process can 
request a line handle.

// Martin

Re: [PATCHv2] gpio: add simple get/set helpers for GPIO lines

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1638 bytes --]

Hi Martin,

> The device tree can configure labels for each line that one can 
> read/compare using GPIO_GET_LINEINFO_IOCTL. If that is not the case, one 
> would have to either read the docs (often the schematic) or do 
> trial'n'error.
> 

Okay, so I'm guessing we might need to expose this in the future somehow.

>>
>> You have the main /dev/chip file descriptor and you have a file 
>> descriptor returned from the GPIO_GET_LINEHANDLE_IOCTL.  What happens 
>> to the fds returned by GPIO_GET_LINEHANDLE_IOCTL if the main /dev/chip 
>> fd is closed?
> 
> The virtual fds from GPIO_GET_LINEHANDLE_IOCTL stays valid after closing 
> the /dev/gpiochipX fd. (So I should probably close those virtual fds 
> after getting/setting the values.)

So if this is the case, do we even need to worry about reference 
tracking between gpio_chip and gpio_line?

Perhaps all we need is a gpio_chip_info internal structure that can be 
reference counted and used by gpio_chip and gpio_line.  And gpio_chip 
and gpio_line objects can be destroyed independently of each other.

> 
>> There's no hotplug/unplug with gpio devices right?  So you can count 
>> of the /dev/chip & fd to be owned exlusively by the ell client?
> 
> You can get USB gpio expanders:
> http://nanorivertech.com/viperboard.html
> So I guess /dev/gpiochipX nodes can come and go.

Probably out of scope for this simple API, just curious: how do we 
detect that? udev?

> 
> Multiple processes can use the same gpiochip, but only one process can 
> request a line handle.

How do you define 'use the same gpiochip'?

Regards,
-Denis

Re: [PATCHv2] gpio: add simple get/set helpers for GPIO lines

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

[-- Attachment #1: Type: text/plain, Size: 2987 bytes --]

Hi Denis,

On 17/12/2018 18.17, Denis Kenzior wrote:
>> The device tree can configure labels for each line that one can 
>> read/compare using GPIO_GET_LINEINFO_IOCTL. If that is not the case, 
>> one would have to either read the docs (often the schematic) or do 
>> trial'n'error.
>>
> 
> Okay, so I'm guessing we might need to expose this in the future somehow.

I have these in my dirty tree now:


struct l_gpio_line *l_gpio_chip_get_line(struct l_gpio_chip *gpio_chip,
                                          uint32_t line_num);

struct l_gpio_line *l_gpio_chip_find_line(struct l_gpio_chip *gpio_chip,
                                           const char *line_label);

struct l_gpio_line *l_gpio_find_line(const char *line_label);

struct l_gpio_chip *l_gpio_line_get_chip(struct l_gpio_line *gpio_line);

const char *l_gpio_line_get_label(struct l_gpio_line *line);


They are WIP, though.

>>> You have the main /dev/chip file descriptor and you have a file 
>>> descriptor returned from the GPIO_GET_LINEHANDLE_IOCTL.  What happens 
>>> to the fds returned by GPIO_GET_LINEHANDLE_IOCTL if the main 
>>> /dev/chip fd is closed?
>>
>> The virtual fds from GPIO_GET_LINEHANDLE_IOCTL stays valid after 
>> closing the /dev/gpiochipX fd. (So I should probably close those 
>> virtual fds after getting/setting the values.)
> 
> So if this is the case, do we even need to worry about reference 
> tracking between gpio_chip and gpio_line?
> 
> Perhaps all we need is a gpio_chip_info internal structure that can be 
> reference counted and used by gpio_chip and gpio_line.  And gpio_chip 
> and gpio_line objects can be destroyed independently of each other.

Is there generic reference counting functionality in ell?

How much of the kernel API do we want to expose? Limiting ourselves to 
setting/getting a single gpio-line at the time is clearly the simplest, 
but I wonder how long it will be before someone requests support for 
handling multiple lines with a single structure (as is possible with the 
gpiohandle_request structure in the kernel API).

>>> There's no hotplug/unplug with gpio devices right?  So you can count 
>>> of the /dev/chip & fd to be owned exlusively by the ell client?
>>
>> You can get USB gpio expanders:
>> http://nanorivertech.com/viperboard.html
>> So I guess /dev/gpiochipX nodes can come and go.
> 
> Probably out of scope for this simple API, just curious: how do we 
> detect that? udev?

It sound like a job for udev, yes.

>>
>> Multiple processes can use the same gpiochip, but only one process can 
>> request a line handle.
> 
> How do you define 'use the same gpiochip'?

Opening e.g. /dev/gpiochip0 can happen in parallel, but requesting a 
handle/virtual fd to gpioline0 on gpiochip0 is serialized.


-- 
Kind regards,
Martin Hundebøll
Embedded Linux Consultant

+45 61 65 54 61
martin(a)geanix.com

Geanix IVS
https://geanix.com
DK39600706

Re: [PATCHv2] gpio: add simple get/set helpers for GPIO lines

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2043 bytes --]

Hi Martin,

>> Okay, so I'm guessing we might need to expose this in the future somehow.
> 
> I have these in my dirty tree now:
> 
> 
> struct l_gpio_line *l_gpio_chip_get_line(struct l_gpio_chip *gpio_chip,
>                                           uint32_t line_num);
> 
> struct l_gpio_line *l_gpio_chip_find_line(struct l_gpio_chip *gpio_chip,
>                                            const char *line_label);

Okay, makes sense...

> 
> struct l_gpio_line *l_gpio_find_line(const char *line_label);

How does that work?

> 
> struct l_gpio_chip *l_gpio_line_get_chip(struct l_gpio_line *gpio_line);

This might be tricky depending on how you setup the tracking 
relationship between line & chip.

> 
> const char *l_gpio_line_get_label(struct l_gpio_line *line);
> 

Makes sense..

> 
> They are WIP, though.

Sure, understood.

>> So if this is the case, do we even need to worry about reference 
>> tracking between gpio_chip and gpio_line?
>>
>> Perhaps all we need is a gpio_chip_info internal structure that can be 
>> reference counted and used by gpio_chip and gpio_line.  And gpio_chip 
>> and gpio_line objects can be destroyed independently of each other.
> 
> Is there generic reference counting functionality in ell?

No, but ell is not thread safe, so you can just simply use a counter. 
Some of our classes use __sync_sub_and_fetch, but that is really 
overkill and pointless.

> 
> How much of the kernel API do we want to expose? Limiting ourselves to 
> setting/getting a single gpio-line at the time is clearly the simplest, 
> but I wonder how long it will be before someone requests support for 
> handling multiple lines with a single structure (as is possible with the 
> gpiohandle_request structure in the kernel API).

I think the immediate goal is to get enough into ell so that you can 
work on your oFono bits.  We can then go on from there.

Regards,
-Denis

Re: [PATCHv2] gpio: add simple get/set helpers for GPIO lines

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

[-- Attachment #1: Type: text/plain, Size: 2271 bytes --]

Hi,

On 17/12/2018 20.19, Denis Kenzior wrote:
>> struct l_gpio_line *l_gpio_find_line(const char *line_label);
> 
> How does that work?

It does a scandir on /dev/ filtering on "gpiochip" and then traverses 
each line on each found gpiochip - pretty much like libgpiod does:

https://git.kernel.org/pub/scm/libs/libgpiod/libgpiod.git/tree/src/lib/iter.c

>> struct l_gpio_chip *l_gpio_line_get_chip(struct l_gpio_line *gpio_line);
> 
> This might be tricky depending on how you setup the tracking 
> relationship between line & chip.

If we put the primary fd in the internal gpio structure, this function 
could create a new struct l_gpio_chip using GPIO_GET_CHIPINFO_IOCTL. But 
I should probably postpone that to later.

>>> So if this is the case, do we even need to worry about reference 
>>> tracking between gpio_chip and gpio_line?
>>>
>>> Perhaps all we need is a gpio_chip_info internal structure that can 
>>> be reference counted and used by gpio_chip and gpio_line.  And 
>>> gpio_chip and gpio_line objects can be destroyed independently of 
>>> each other.
>>
>> Is there generic reference counting functionality in ell?
> 
> No, but ell is not thread safe, so you can just simply use a counter. 
> Some of our classes use __sync_sub_and_fetch, but that is really 
> overkill and pointless.

I started out with this:


static struct l_gpio *gpio_get(struct l_gpio *gpio)
{
         gpio->refcount++;

         return gpio;
}

static void gpio_put(struct l_gpio *gpio)
{
         if (--gpio->refcount == 0)
                 l_free(gpio);
}

>> How much of the kernel API do we want to expose? Limiting ourselves to 
>> setting/getting a single gpio-line at the time is clearly the 
>> simplest, but I wonder how long it will be before someone requests 
>> support for handling multiple lines with a single structure (as is 
>> possible with the gpiohandle_request structure in the kernel API).
> 
> I think the immediate goal is to get enough into ell so that you can 
> work on your oFono bits.  We can then go on from there.

Copy that.

-- 
Kind regards,
Martin Hundebøll
Embedded Linux Consultant

+45 61 65 54 61
martin(a)geanix.com

Geanix IVS
https://geanix.com
DK39600706

Re: [PATCHv2] gpio: add simple get/set helpers for GPIO lines

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 4066 bytes --]

Hi Martin,

>>> The device tree can configure labels for each line that one can read/compare using GPIO_GET_LINEINFO_IOCTL. If that is not the case, one would have to either read the docs (often the schematic) or do trial'n'error.
>>> 
>> Okay, so I'm guessing we might need to expose this in the future somehow.
> 
> I have these in my dirty tree now:
> 
> 
> struct l_gpio_line *l_gpio_chip_get_line(struct l_gpio_chip *gpio_chip,
>                                         uint32_t line_num);
> 
> struct l_gpio_line *l_gpio_chip_find_line(struct l_gpio_chip *gpio_chip,
>                                          const char *line_label);
> 
> struct l_gpio_line *l_gpio_find_line(const char *line_label);
> 
> struct l_gpio_chip *l_gpio_line_get_chip(struct l_gpio_line *gpio_line);
> 
> const char *l_gpio_line_get_label(struct l_gpio_line *line);
> 
> 
> They are WIP, though.

so I am not 100% sure this is useful since it adds a complexity that most like is never going to be used. What I would first check is what happens when the underlying physical device goes away. Does the l_gpio_chip fd gets a HUP signal. Since we might really need to allow for disconnect callback here.

I am fine with the lines being address with the uint32 index number. Creating a struct for it seems kinda useless since you will always need special knowledge to some degree anyway. I rather have l_gpio_chip_find_line that takes a line label and tries to find it. Especially with the background you might need to set more than one GPIO at once, then the line concept blows up.

>>>> You have the main /dev/chip file descriptor and you have a file descriptor returned from the GPIO_GET_LINEHANDLE_IOCTL.  What happens to the fds returned by GPIO_GET_LINEHANDLE_IOCTL if the main /dev/chip fd is closed?
>>> 
>>> The virtual fds from GPIO_GET_LINEHANDLE_IOCTL stays valid after closing the /dev/gpiochipX fd. (So I should probably close those virtual fds after getting/setting the values.)
>> So if this is the case, do we even need to worry about reference tracking between gpio_chip and gpio_line?
>> Perhaps all we need is a gpio_chip_info internal structure that can be reference counted and used by gpio_chip and gpio_line.  And gpio_chip and gpio_line objects can be destroyed independently of each other.
> 
> Is there generic reference counting functionality in ell?
> 
> How much of the kernel API do we want to expose? Limiting ourselves to setting/getting a single gpio-line at the time is clearly the simplest, but I wonder how long it will be before someone requests support for handling multiple lines with a single structure (as is possible with the gpiohandle_request structure in the kernel API).
> 
>>>> There's no hotplug/unplug with gpio devices right?  So you can count of the /dev/chip & fd to be owned exlusively by the ell client?
>>> 
>>> You can get USB gpio expanders:
>>> http://nanorivertech.com/viperboard.html
>>> So I guess /dev/gpiochipX nodes can come and go.
>> Probably out of scope for this simple API, just curious: how do we detect that? udev?
> 
> It sound like a job for udev, yes.

If the HUP is actually signaled on the /dev/gpiochipX fd, then I would ignore udev. And instead add a helper to find a given l_gpio_chip_new_from_name() that then would walk either /dev/ (with scandir like libgpiod does) or by going through sysfs. I would prefer sysfs since it will be mostly part of a bus or class.

> 
>>> 
>>> Multiple processes can use the same gpiochip, but only one process can request a line handle.
>> How do you define 'use the same gpiochip'?
> 
> Opening e.g. /dev/gpiochip0 can happen in parallel, but requesting a handle/virtual fd to gpioline0 on gpiochip0 is serialized.

I would just allow opening /dev/gpiochipX multiple times. I think there is not need for ELL to multiplex here.

For one-shot triggers we could do a l_gpio_set(const char *chipname, const char *linename) that then will try to find the chip and line by name and toggle it.

Regards

Marcel

Re: [PATCHv2] gpio: add simple get/set helpers for GPIO lines

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

[-- Attachment #1: Type: text/plain, Size: 3606 bytes --]

Hi Marcel,

On 18/12/2018 20.03, Marcel Holtmann wrote:
>> I have these in my dirty tree now:
>>
>>
>> struct l_gpio_line *l_gpio_chip_get_line(struct l_gpio_chip *gpio_chip,
>>                                          uint32_t line_num);
>>
>> struct l_gpio_line *l_gpio_chip_find_line(struct l_gpio_chip *gpio_chip,
>>                                           const char *line_label);
>>
>> struct l_gpio_line *l_gpio_find_line(const char *line_label);
>>
>> struct l_gpio_chip *l_gpio_line_get_chip(struct l_gpio_line *gpio_line);
>>
>> const char *l_gpio_line_get_label(struct l_gpio_line *line);
>>
>>
>> They are WIP, though.
> 
> so I am not 100% sure this is useful since it adds a complexity that most like is never going to be used. What I would first check is what happens when the underlying physical device goes away. Does the l_gpio_chip fd gets a HUP signal. Since we might really need to allow for disconnect callback here.

I assume you mean POLLHUP here? If that's the case, then no: I don't get 
a POLLHUP event when removing an open gpiochip device.

> I am fine with the lines being address with the uint32 index number. Creating a struct for it seems kinda useless since you will always need special knowledge to some degree anyway. I rather have l_gpio_chip_find_line that takes a line label and tries to find it. Especially with the background you might need to set more than one GPIO at once, then the line concept blows up.

Yes, the one-struct-per-line approach doesn't really scale to handling 
multiple lines in a single ioctl(). Now that I think about it, a single 
structure (and a convenient way to set it up) might be all we need. 
Something like:

uint32_t lines[] = {0, 4, 8};
char *labels[] = {"3v3", "reset", "led", NULL};
bool values[] = {true, true, false};

struct l_gpio *gpio;

gpio = l_gpio_new("/dev/gpiochip0");
l_gpio_lines_by_num(gpio, lines, L_ARRAY_SIZE(lines), L_GPIO_OUT);
l_gpio_set_values(gpio, values);
l_gpio_free(gpio);

gpio = l_gpio_new("/dev/gpiochip0");
l_gpio_lines_by_label(gpio, labels, L_GPIO_IN);
l_gpio_get_values(gpio, values);
l_gpio_free(gpio);

// And maybe a few convenience helpers:
gpio = l_gpio_new_line_by_num("/dev/gpiochip0", values[1], L_GPIO_IN);
l_gpio_free(gpio);

gpio = l_gpio_new_line_by_label(labels[1], L_GPIO_OUT);
l_gpio_free(gpio);

> If the HUP is actually signaled on the /dev/gpiochipX fd, then I would ignore udev. And instead add a helper to find a given l_gpio_chip_new_from_name() that then would walk either /dev/ (with scandir like libgpiod does) or by going through sysfs. I would prefer sysfs since it will be mostly part of a bus or class.

root(a)rpi3:~# ls /sys/bus/gpio/devices/
gpiochip0  gpiochip1  gpiochip2

I need some guidance on how that should be done...

>> Opening e.g. /dev/gpiochip0 can happen in parallel, but requesting a handle/virtual fd to gpioline0 on gpiochip0 is serialized.
> 
> I would just allow opening /dev/gpiochipX multiple times. I think there is not need for ELL to multiplex here.

I think this is in line with the example usage I proposed above.

> For one-shot triggers we could do a l_gpio_set(const char *chipname, const char *linename) that then will try to find the chip and line by name and toggle it.

Do we want to support both numeric and labeled line identifiers?

l_gpio_{get,set,clear}_by_chip_and_label(const char *chipname, const 
char *label);
l_gpio_{get,set,clear}_by_label(const char *label);
l_gpio_{get,set,clear}_by_num(const char *chipname, uint32_t num);


// Martin

Re: [PATCHv2] gpio: add simple get/set helpers for GPIO lines

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 4300 bytes --]

Hi Martin,

>>> I have these in my dirty tree now:
>>> 
>>> 
>>> struct l_gpio_line *l_gpio_chip_get_line(struct l_gpio_chip *gpio_chip,
>>>                                         uint32_t line_num);
>>> 
>>> struct l_gpio_line *l_gpio_chip_find_line(struct l_gpio_chip *gpio_chip,
>>>                                          const char *line_label);
>>> 
>>> struct l_gpio_line *l_gpio_find_line(const char *line_label);
>>> 
>>> struct l_gpio_chip *l_gpio_line_get_chip(struct l_gpio_line *gpio_line);
>>> 
>>> const char *l_gpio_line_get_label(struct l_gpio_line *line);
>>> 
>>> 
>>> They are WIP, though.
>> so I am not 100% sure this is useful since it adds a complexity that most like is never going to be used. What I would first check is what happens when the underlying physical device goes away. Does the l_gpio_chip fd gets a HUP signal. Since we might really need to allow for disconnect callback here.
> 
> I assume you mean POLLHUP here? If that's the case, then no: I don't get a POLLHUP event when removing an open gpiochip device.

why not? Can we fix that in the kernel. It is silly to have a stale fd around that can signal that is is dead.

>> I am fine with the lines being address with the uint32 index number. Creating a struct for it seems kinda useless since you will always need special knowledge to some degree anyway. I rather have l_gpio_chip_find_line that takes a line label and tries to find it. Especially with the background you might need to set more than one GPIO at once, then the line concept blows up.
> 
> Yes, the one-struct-per-line approach doesn't really scale to handling multiple lines in a single ioctl(). Now that I think about it, a single structure (and a convenient way to set it up) might be all we need. Something like:
> 
> uint32_t lines[] = {0, 4, 8};
> char *labels[] = {"3v3", "reset", "led", NULL};
> bool values[] = {true, true, false};
> 
> struct l_gpio *gpio;
> 
> gpio = l_gpio_new("/dev/gpiochip0");
> l_gpio_lines_by_num(gpio, lines, L_ARRAY_SIZE(lines), L_GPIO_OUT);
> l_gpio_set_values(gpio, values);
> l_gpio_free(gpio);
> 
> gpio = l_gpio_new("/dev/gpiochip0");
> l_gpio_lines_by_label(gpio, labels, L_GPIO_IN);
> l_gpio_get_values(gpio, values);
> l_gpio_free(gpio);
> 
> // And maybe a few convenience helpers:
> gpio = l_gpio_new_line_by_num("/dev/gpiochip0", values[1], L_GPIO_IN);
> l_gpio_free(gpio);
> 
> gpio = l_gpio_new_line_by_label(labels[1], L_GPIO_OUT);
> l_gpio_free(gpio);
> 
>> If the HUP is actually signaled on the /dev/gpiochipX fd, then I would ignore udev. And instead add a helper to find a given l_gpio_chip_new_from_name() that then would walk either /dev/ (with scandir like libgpiod does) or by going through sysfs. I would prefer sysfs since it will be mostly part of a bus or class.
> 
> root(a)rpi3:~# ls /sys/bus/gpio/devices/
> gpiochip0  gpiochip1  gpiochip2
> 
> I need some guidance on how that should be done…

That is just a reading directory operation to get the names and then you need to retrieve their /dev/ name. This can still fail if udev start renaming things left and right, but on a system that is not totally broken, this will actually work. Since you will get the major/minor numbers of the device node from sysfs.

> 
>>> Opening e.g. /dev/gpiochip0 can happen in parallel, but requesting a handle/virtual fd to gpioline0 on gpiochip0 is serialized.
>> I would just allow opening /dev/gpiochipX multiple times. I think there is not need for ELL to multiplex here.
> 
> I think this is in line with the example usage I proposed above.
> 
>> For one-shot triggers we could do a l_gpio_set(const char *chipname, const char *linename) that then will try to find the chip and line by name and toggle it.
> 
> Do we want to support both numeric and labeled line identifiers?
> 
> l_gpio_{get,set,clear}_by_chip_and_label(const char *chipname, const char *label);
> l_gpio_{get,set,clear}_by_label(const char *label);
> l_gpio_{get,set,clear}_by_num(const char *chipname, uint32_t num);

I think that you always have to give the chipname since somehow you need to find GPIO chip device node. Hoping that all lines have unique names is something that doesn’t look like a good idea.

Regards

Marcel

Re: [PATCHv2] gpio: add simple get/set helpers for GPIO lines

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

[-- Attachment #1: Type: text/plain, Size: 4964 bytes --]

Hi Marcel,

On 19/12/2018 15.37, Marcel Holtmann wrote:
>>>> I have these in my dirty tree now:
>>>>
>>>>
>>>> struct l_gpio_line *l_gpio_chip_get_line(struct l_gpio_chip *gpio_chip,
>>>>                                          uint32_t line_num);
>>>>
>>>> struct l_gpio_line *l_gpio_chip_find_line(struct l_gpio_chip *gpio_chip,
>>>>                                           const char *line_label);
>>>>
>>>> struct l_gpio_line *l_gpio_find_line(const char *line_label);
>>>>
>>>> struct l_gpio_chip *l_gpio_line_get_chip(struct l_gpio_line *gpio_line);
>>>>
>>>> const char *l_gpio_line_get_label(struct l_gpio_line *line);
>>>>
>>>>
>>>> They are WIP, though.
>>> so I am not 100% sure this is useful since it adds a complexity that most like is never going to be used. What I would first check is what happens when the underlying physical device goes away. Does the l_gpio_chip fd gets a HUP signal. Since we might really need to allow for disconnect callback here.
>>
>> I assume you mean POLLHUP here? If that's the case, then no: I don't get a POLLHUP event when removing an open gpiochip device.
> 
> why not? Can we fix that in the kernel. It is silly to have a stale fd around that can signal that is is dead.

There is no .poll in the gpio chardev fileops. I can check with Linus 
Walleij if he is open to adding it.

>>> I am fine with the lines being address with the uint32 index number. Creating a struct for it seems kinda useless since you will always need special knowledge to some degree anyway. I rather have l_gpio_chip_find_line that takes a line label and tries to find it. Especially with the background you might need to set more than one GPIO at once, then the line concept blows up.
>>
>> Yes, the one-struct-per-line approach doesn't really scale to handling multiple lines in a single ioctl(). Now that I think about it, a single structure (and a convenient way to set it up) might be all we need. Something like:
>>
>> uint32_t lines[] = {0, 4, 8};
>> char *labels[] = {"3v3", "reset", "led", NULL};
>> bool values[] = {true, true, false};
>>
>> struct l_gpio *gpio;
>>
>> gpio = l_gpio_new("/dev/gpiochip0");
>> l_gpio_lines_by_num(gpio, lines, L_ARRAY_SIZE(lines), L_GPIO_OUT);
>> l_gpio_set_values(gpio, values);
>> l_gpio_free(gpio);
>>
>> gpio = l_gpio_new("/dev/gpiochip0");
>> l_gpio_lines_by_label(gpio, labels, L_GPIO_IN);
>> l_gpio_get_values(gpio, values);
>> l_gpio_free(gpio);
>>
>> // And maybe a few convenience helpers:
>> gpio = l_gpio_new_line_by_num("/dev/gpiochip0", values[1], L_GPIO_IN);
>> l_gpio_free(gpio);
>>
>> gpio = l_gpio_new_line_by_label(labels[1], L_GPIO_OUT);
>> l_gpio_free(gpio);
>>
>>> If the HUP is actually signaled on the /dev/gpiochipX fd, then I would ignore udev. And instead add a helper to find a given l_gpio_chip_new_from_name() that then would walk either /dev/ (with scandir like libgpiod does) or by going through sysfs. I would prefer sysfs since it will be mostly part of a bus or class.
>>
>> root(a)rpi3:~# ls /sys/bus/gpio/devices/
>> gpiochip0  gpiochip1  gpiochip2
>>
>> I need some guidance on how that should be done…
> 
> That is just a reading directory operation to get the names and then you need to retrieve their /dev/ name. This can still fail if udev start renaming things left and right, but on a system that is not totally broken, this will actually work. Since you will get the major/minor numbers of the device node from sysfs.

We have the name and label: name is assigned sequentially (unless 
renamed by udev), and label is taken from e.g. the device tree node-name 
(or label). I guess we should stick to "label" if we want to support 
opening a device by such an identifier.

>>
>>>> Opening e.g. /dev/gpiochip0 can happen in parallel, but requesting a handle/virtual fd to gpioline0 on gpiochip0 is serialized.
>>> I would just allow opening /dev/gpiochipX multiple times. I think there is not need for ELL to multiplex here.
>>
>> I think this is in line with the example usage I proposed above.
>>
>>> For one-shot triggers we could do a l_gpio_set(const char *chipname, const char *linename) that then will try to find the chip and line by name and toggle it.
>>
>> Do we want to support both numeric and labeled line identifiers?
>>
>> l_gpio_{get,set,clear}_by_chip_and_label(const char *chipname, const char *label);
>> l_gpio_{get,set,clear}_by_label(const char *label);
>> l_gpio_{get,set,clear}_by_num(const char *chipname, uint32_t num);
> 
> I think that you always have to give the chipname since somehow you need to find GPIO chip device node. Hoping that all lines have unique names is something that doesn’t look like a good idea.

For the reference: libgpiod simply returns the first match, but I agree: 
lets require either a chip name (e.g. gpiochip0) or a chip label (e.g. 
raspberrypi-exp-gpio) together with line number/label

// Martin

Re: [PATCHv2] gpio: add simple get/set helpers for GPIO lines

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2649 bytes --]

Hi Martin,

>>>> so I am not 100% sure this is useful since it adds a complexity that 
>>>> most like is never going to be used. What I would first check is 
>>>> what happens when the underlying physical device goes away. Does the 
>>>> l_gpio_chip fd gets a HUP signal. Since we might really need to 
>>>> allow for disconnect callback here.
>>>
>>> I assume you mean POLLHUP here? If that's the case, then no: I don't 
>>> get a POLLHUP event when removing an open gpiochip device.
>>
>> why not? Can we fix that in the kernel. It is silly to have a stale fd 
>> around that can signal that is is dead.
> 
> There is no .poll in the gpio chardev fileops. I can check with Linus 
> Walleij if he is open to adding it.

Just a thought, but we can add l_gpio_chip_set_disconnect_watch at a 
later date, if/when this becomes available.  You don't need this for 
oFono work right now, right?

> 
>>>> I am fine with the lines being address with the uint32 index number. 
>>>> Creating a struct for it seems kinda useless since you will always 
>>>> need special knowledge to some degree anyway. I rather have 
>>>> l_gpio_chip_find_line that takes a line label and tries to find it. 
>>>> Especially with the background you might need to set more than one 
>>>> GPIO at once, then the line concept blows up.
>>>
>>> Yes, the one-struct-per-line approach doesn't really scale to 
>>> handling multiple lines in a single ioctl(). Now that I think about 
>>> it, a single structure (and a convenient way to set it up) might be 
>>> all we need. Something like:
>>>
>>> uint32_t lines[] = {0, 4, 8};
>>> char *labels[] = {"3v3", "reset", "led", NULL};
>>> bool values[] = {true, true, false};
>>>
>>> struct l_gpio *gpio;
>>>
>>> gpio = l_gpio_new("/dev/gpiochip0");
>>> l_gpio_lines_by_num(gpio, lines, L_ARRAY_SIZE(lines), L_GPIO_OUT);

So this implies some sort of temporary state being kept by l_gpio.  Not 
sure I like that...

>>> l_gpio_set_values(gpio, values);

Why not use varargs here?  So something like:

l_gpio_set_by_index(gpio, 0, true, 4, true, 8, false, -1);

>>> l_gpio_free(gpio);
>>>
>>> gpio = l_gpio_new("/dev/gpiochip0");
>>> l_gpio_lines_by_label(gpio, labels, L_GPIO_IN);
>>> l_gpio_get_values(gpio, values);

l_gpio_get_values(gpio, "3v3", &v1, "reset", &v2, "led", &v3, NULL);

>>> l_gpio_free(gpio);
>>>
>>> // And maybe a few convenience helpers:
>>> gpio = l_gpio_new_line_by_num("/dev/gpiochip0", values[1], L_GPIO_IN);
>>> l_gpio_free(gpio);
>>>
>>> gpio = l_gpio_new_line_by_label(labels[1], L_GPIO_OUT);
>>> l_gpio_free(gpio);
>>>

Regards,
-Denis

Re: [PATCHv2] gpio: add simple get/set helpers for GPIO lines

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

[-- Attachment #1: Type: text/plain, Size: 3519 bytes --]

Hi Denis,

On 19/12/2018 17.46, Denis Kenzior wrote:
>>>>> so I am not 100% sure this is useful since it adds a complexity 
>>>>> that most like is never going to be used. What I would first check 
>>>>> is what happens when the underlying physical device goes away. Does 
>>>>> the l_gpio_chip fd gets a HUP signal. Since we might really need to 
>>>>> allow for disconnect callback here.
>>>>
>>>> I assume you mean POLLHUP here? If that's the case, then no: I don't 
>>>> get a POLLHUP event when removing an open gpiochip device.
>>>
>>> why not? Can we fix that in the kernel. It is silly to have a stale 
>>> fd around that can signal that is is dead.
>>
>> There is no .poll in the gpio chardev fileops. I can check with Linus 
>> Walleij if he is open to adding it.
> 
> Just a thought, but we can add l_gpio_chip_set_disconnect_watch at a 
> later date, if/when this becomes available.  You don't need this for 
> oFono work right now, right?

No need in my case, no. I'm only using on-soc gpio controllers.

>>>>> I am fine with the lines being address with the uint32 index 
>>>>> number. Creating a struct for it seems kinda useless since you will 
>>>>> always need special knowledge to some degree anyway. I rather have 
>>>>> l_gpio_chip_find_line that takes a line label and tries to find it. 
>>>>> Especially with the background you might need to set more than one 
>>>>> GPIO at once, then the line concept blows up.
>>>>
>>>> Yes, the one-struct-per-line approach doesn't really scale to 
>>>> handling multiple lines in a single ioctl(). Now that I think about 
>>>> it, a single structure (and a convenient way to set it up) might be 
>>>> all we need. Something like:
>>>>
>>>> uint32_t lines[] = {0, 4, 8};
>>>> char *labels[] = {"3v3", "reset", "led", NULL};
>>>> bool values[] = {true, true, false};
>>>>
>>>> struct l_gpio *gpio;
>>>>
>>>> gpio = l_gpio_new("/dev/gpiochip0");
>>>> l_gpio_lines_by_num(gpio, lines, L_ARRAY_SIZE(lines), L_GPIO_OUT);
> 
> So this implies some sort of temporary state being kept by l_gpio.  Not 
> sure I like that...

GPIO lines are more or less meant to be acquired (hogged in 
kernel-nomenclature) by the controlling process. The reservation is tied 
to the virtual file descriptor, which maps to one or more gpio lines on 
a gpio chip. Closing the virtual fd releases the line(s).

In my previous patches I simply acquired the gpio line while 
setting/getting the value, and then released again before returning.

>>>> l_gpio_set_values(gpio, values);
> 
> Why not use varargs here?  So something like:
> 
> l_gpio_set_by_index(gpio, 0, true, 4, true, 8, false, -1);

I would very much like to use varargs, but we should consider the 
stateful approach explained above.

// Martin

>>>> l_gpio_free(gpio);
>>>>
>>>> gpio = l_gpio_new("/dev/gpiochip0");
>>>> l_gpio_lines_by_label(gpio, labels, L_GPIO_IN);
>>>> l_gpio_get_values(gpio, values);
> 
> l_gpio_get_values(gpio, "3v3", &v1, "reset", &v2, "led", &v3, NULL);
> 
>>>> l_gpio_free(gpio);
>>>>
>>>> // And maybe a few convenience helpers:
>>>> gpio = l_gpio_new_line_by_num("/dev/gpiochip0", values[1], L_GPIO_IN);
>>>> l_gpio_free(gpio);
>>>>
>>>> gpio = l_gpio_new_line_by_label(labels[1], L_GPIO_OUT);
>>>> l_gpio_free(gpio);
>>>>
> 
> Regards,
> -Denis

-- 
Kind regards,
Martin Hundebøll
Embedded Linux Consultant

+45 61 65 54 61
martin(a)geanix.com

Geanix IVS
https://geanix.com
DK39600706

Re: [PATCHv2] gpio: add simple get/set helpers for GPIO lines

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2303 bytes --]

Hi Martin,

>>>>>> I am fine with the lines being address with the uint32 index
>>>>>> number. Creating a struct for it seems kinda useless since you 
>>>>>> will always need special knowledge to some degree anyway. I rather 
>>>>>> have l_gpio_chip_find_line that takes a line label and tries to 
>>>>>> find it. Especially with the background you might need to set more 
>>>>>> than one GPIO at once, then the line concept blows up.
>>>>>
>>>>> Yes, the one-struct-per-line approach doesn't really scale to 
>>>>> handling multiple lines in a single ioctl(). Now that I think about 
>>>>> it, a single structure (and a convenient way to set it up) might be 
>>>>> all we need. Something like:
>>>>>
>>>>> uint32_t lines[] = {0, 4, 8};
>>>>> char *labels[] = {"3v3", "reset", "led", NULL};
>>>>> bool values[] = {true, true, false};
>>>>>
>>>>> struct l_gpio *gpio;
>>>>>
>>>>> gpio = l_gpio_new("/dev/gpiochip0");
>>>>> l_gpio_lines_by_num(gpio, lines, L_ARRAY_SIZE(lines), L_GPIO_OUT);
>>
>> So this implies some sort of temporary state being kept by l_gpio.  
>> Not sure I like that...
> 
> GPIO lines are more or less meant to be acquired (hogged in 
> kernel-nomenclature) by the controlling process. The reservation is tied 
> to the virtual file descriptor, which maps to one or more gpio lines on 
> a gpio chip. Closing the virtual fd releases the line(s).
> 
> In my previous patches I simply acquired the gpio line while 
> setting/getting the value, and then released again before returning.

But isn't the intent to do this here as well?  At least on l_gpio level 
it doesn't seem like we should be keeping the virtual fd.  If we want to 
keep that open, then we need a whole new class for that.  One that might 
need to expose l_io or some other way to subscribe to changes on gpio 
lines.  From what I understood this functionality isn't needed yet, so 
it can be added later...

> 
>>>>> l_gpio_set_values(gpio, values);
>>
>> Why not use varargs here?  So something like:
>>
>> l_gpio_set_by_index(gpio, 0, true, 4, true, 8, false, -1);
> 
> I would very much like to use varargs, but we should consider the 
> stateful approach explained above.
> 

That statement seems to be contradictory?  Can you explain?

Regards,
-Denis

Re: [PATCHv2] gpio: add simple get/set helpers for GPIO lines

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

[-- Attachment #1: Type: text/plain, Size: 3543 bytes --]

Hi Denis,

On 19/12/2018 20.43, Denis Kenzior wrote:
>>>>>>> I am fine with the lines being address with the uint32 index
>>>>>>> number. Creating a struct for it seems kinda useless since you 
>>>>>>> will always need special knowledge to some degree anyway. I 
>>>>>>> rather have l_gpio_chip_find_line that takes a line label and 
>>>>>>> tries to find it. Especially with the background you might need 
>>>>>>> to set more than one GPIO at once, then the line concept blows up.
>>>>>>
>>>>>> Yes, the one-struct-per-line approach doesn't really scale to 
>>>>>> handling multiple lines in a single ioctl(). Now that I think 
>>>>>> about it, a single structure (and a convenient way to set it up) 
>>>>>> might be all we need. Something like:
>>>>>>
>>>>>> uint32_t lines[] = {0, 4, 8};
>>>>>> char *labels[] = {"3v3", "reset", "led", NULL};
>>>>>> bool values[] = {true, true, false};
>>>>>>
>>>>>> struct l_gpio *gpio;
>>>>>>
>>>>>> gpio = l_gpio_new("/dev/gpiochip0");
>>>>>> l_gpio_lines_by_num(gpio, lines, L_ARRAY_SIZE(lines), L_GPIO_OUT);
>>>
>>> So this implies some sort of temporary state being kept by l_gpio. 
>>> Not sure I like that...
>>
>> GPIO lines are more or less meant to be acquired (hogged in 
>> kernel-nomenclature) by the controlling process. The reservation is 
>> tied to the virtual file descriptor, which maps to one or more gpio 
>> lines on a gpio chip. Closing the virtual fd releases the line(s).
>>
>> In my previous patches I simply acquired the gpio line while 
>> setting/getting the value, and then released again before returning.
> 
> But isn't the intent to do this here as well?  At least on l_gpio level 
> it doesn't seem like we should be keeping the virtual fd.

It was my initial intent to do a simple "open vfd -> set/get value -> 
close vfd", but as our discussion has evolved, I am more inclined to 
stick to what the kernel API exposes, and simply tie the l_gpio instance 
to a single set of gpio-lines. If the user need different sets of lines, 
he should create multiple l_gpio instances.

> If we want to 
> keep that open, then we need a whole new class for that.  One that might 
> need to expose l_io or some other way to subscribe to changes on gpio 
> lines.  From what I understood this functionality isn't needed yet, so 
> it can be added later...

The kernel API don't signal any changes on either the gpiochip fd or the 
gpiohandle fd. Changes on gpio-lines are exposed through a gpioevent 
virtual fd that is acquired using GPIO_GET_LINEEVENT_IOCTL.

It implements .poll() to signal changes and .read() to supply details 
about those changes, so users can use l_io to monitor it for events.

gpioevents are mutual exclusive to gpiohandles, so one cannot acquire a 
line for gpioevent if it is already acquired as a gpiohandle, and 
vice-versa. That might require two distinct classes to handle?

This event handling is of course not needed right now, but we should 
consider it when designing the get/set interface.

>>
>>>>>> l_gpio_set_values(gpio, values);
>>>
>>> Why not use varargs here?  So something like:
>>>
>>> l_gpio_set_by_index(gpio, 0, true, 4, true, 8, false, -1);
>>
>> I would very much like to use varargs, but we should consider the 
>> stateful approach explained above.
>>
> 
> That statement seems to be contradictory?  Can you explain?

Sorry, I meant to use varargs in either l_gpio_new() or l_gpio_set_lines().


Anyways, thanks for being patient.

// Martin

Re: [PATCHv2] gpio: add simple get/set helpers for GPIO lines

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2260 bytes --]

Hi Martin,

>>>> So this implies some sort of temporary state being kept by l_gpio. 
>>>> Not sure I like that...
>>>
>>> GPIO lines are more or less meant to be acquired (hogged in 
>>> kernel-nomenclature) by the controlling process. The reservation is 
>>> tied to the virtual file descriptor, which maps to one or more gpio 
>>> lines on a gpio chip. Closing the virtual fd releases the line(s).
>>>
>>> In my previous patches I simply acquired the gpio line while 
>>> setting/getting the value, and then released again before returning.
>>
>> But isn't the intent to do this here as well?  At least on l_gpio 
>> level it doesn't seem like we should be keeping the virtual fd.
> 
> It was my initial intent to do a simple "open vfd -> set/get value -> 
> close vfd", but as our discussion has evolved, I am more inclined to 
> stick to what the kernel API exposes, and simply tie the l_gpio instance 
> to a single set of gpio-lines. If the user need different sets of lines, 
> he should create multiple l_gpio instances.

Ah, okay.  I think I get what you're trying to do.  So you want to have 
something like:

struct l_gpio *l_gpio_new(const char *file_path, ...);

Which would open(file_path), and issue GPIO_GET_CHIPINFO_IOCTL and 
GPIO_GET_LINEINFO_IOCTL to find the right lines, then

GPIO_GET_LINEHANDLE_IOCTL with GPIOHANDLE_REQUEST_OUTPUT and 
GPIOHANDLE_REQUEST_INPUT flags set to obtain exclusive rights to the lines.

bool l_gpio_set_values(struct l_gpio *, ...);

would issue a GPIOHANDLE_SET_LINE_VALUES_IOCTL

bool l_gpio_get_values(struct l_gpio *, ...);

would issue a GPIOHANDLE_GET_LINE_VALUES_IOCTL,

Am I right so far?

And use it something like:

struct l_gpio *gpio = l_gpio_new("/foo/bar", "3v3", "reset", "led", NULL);
l_gpio_set_values(gpio, true, true, false);
l_gpio_get_values(gpio, &foo, &bar, &baz);

So the only issue I see here is how does one discover the lines for a 
given chip?  You might still want a l_gpio_chip abstraction to discover 
lines / line numbers, no?

Maybe something like:

chip = l_gpio_chip_new("/foo/bar");
gpio = chip.acquire(chip, "3v3", "reset", "led");
l_gpio_set_values(gpio, ...);
l_gpio_get_values(gpio, ...);

Regards,
-Denis

Re: [PATCHv2] gpio: add simple get/set helpers for GPIO lines

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

[-- Attachment #1: Type: text/plain, Size: 2361 bytes --]

Hi Denis,

On 20/12/2018 18.51, Denis Kenzior wrote:
> Ah, okay.  I think I get what you're trying to do.  So you want to have 
> something like:
> 
> struct l_gpio *l_gpio_new(const char *file_path, ...);
> 
> Which would open(file_path), and issue GPIO_GET_CHIPINFO_IOCTL and 
> GPIO_GET_LINEINFO_IOCTL to find the right lines, then
> 
> GPIO_GET_LINEHANDLE_IOCTL with GPIOHANDLE_REQUEST_OUTPUT and 
> GPIOHANDLE_REQUEST_INPUT flags set to obtain exclusive rights to the lines.
> 
> bool l_gpio_set_values(struct l_gpio *, ...);
> 
> would issue a GPIOHANDLE_SET_LINE_VALUES_IOCTL
> 
> bool l_gpio_get_values(struct l_gpio *, ...);
> 
> would issue a GPIOHANDLE_GET_LINE_VALUES_IOCTL,
> 
> Am I right so far?

Yes, this is pretty much how I have come to imagine it.

> And use it something like:
> 
> struct l_gpio *gpio = l_gpio_new("/foo/bar", "3v3", "reset", "led", NULL);
> l_gpio_set_values(gpio, true, true, false);
> l_gpio_get_values(gpio, &foo, &bar, &baz);
> 
> So the only issue I see here is how does one discover the lines for a 
> given chip?  You might still want a l_gpio_chip abstraction to discover 
> lines / line numbers, no?

How about a "raw" API like:

input_lines = l_gpio_lines("/some/chip", L_GPIO_IN, 1, 2, 6, -1);
output_lines = l_gpio_lines("/some/chip", L_GPIO_OUT, 1, true, 2, false, 
-1);

Output lines need a default value to set when the kernel changes them 
from high-impedance inputs to outputs. I don't particular like how the 
defaults are set in the above. Feel free to propose something better!

Lookup by label is then available via helpers:

chip_path = l_gpio_label_chip("chip-label");
input_lines = l_gpio_label_lines(chip_path, L_GPIO_IN, "3v3", "reset", 
"led", NULL);

chip_path = l_gpio_chip_with_line_labels("3v3", "reset", "led", NULL);
output_lines = l_gpio_label_lines(chip_path, L_GPIO_OUT, "3v3", "reset", 
"led", NULL);

Again, I would like something nicer here.

> Maybe something like:
> 
> chip = l_gpio_chip_new("/foo/bar");
> gpio = chip.acquire(chip, "3v3", "reset", "led");
> l_gpio_set_values(gpio, ...);
> l_gpio_get_values(gpio, ...);

It all boils down to line handles (and probably line events too, at some 
point). The chip and line info are only important when looking up lines, 
and secondary after that.

// Martin

Re: [PATCHv2] gpio: add simple get/set helpers for GPIO lines

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3197 bytes --]

Hi Martin,

On 12/20/2018 01:45 PM, Martin Hundebøll wrote:
> Hi Denis,
> 
> On 20/12/2018 18.51, Denis Kenzior wrote:
>> Ah, okay.  I think I get what you're trying to do.  So you want to 
>> have something like:
>>
>> struct l_gpio *l_gpio_new(const char *file_path, ...);
>>
>> Which would open(file_path), and issue GPIO_GET_CHIPINFO_IOCTL and 
>> GPIO_GET_LINEINFO_IOCTL to find the right lines, then
>>
>> GPIO_GET_LINEHANDLE_IOCTL with GPIOHANDLE_REQUEST_OUTPUT and 
>> GPIOHANDLE_REQUEST_INPUT flags set to obtain exclusive rights to the 
>> lines.
>>
>> bool l_gpio_set_values(struct l_gpio *, ...);
>>
>> would issue a GPIOHANDLE_SET_LINE_VALUES_IOCTL
>>
>> bool l_gpio_get_values(struct l_gpio *, ...);
>>
>> would issue a GPIOHANDLE_GET_LINE_VALUES_IOCTL,
>>
>> Am I right so far?
> 
> Yes, this is pretty much how I have come to imagine it.
> 

Okay.  One more doubt I have is what is the practical purpose of 
obtaining exclusive rights to the GPIO lines for reading only.  That 
seems kind of useless?  Especially if you need to provide default values 
for GPIOHANDLE_REQUEST_OUTPUT requests?

Perhaps reading should be on a different object?

>> And use it something like:
>>
>> struct l_gpio *gpio = l_gpio_new("/foo/bar", "3v3", "reset", "led", 
>> NULL);
>> l_gpio_set_values(gpio, true, true, false);
>> l_gpio_get_values(gpio, &foo, &bar, &baz);
>>
>> So the only issue I see here is how does one discover the lines for a 
>> given chip?  You might still want a l_gpio_chip abstraction to 
>> discover lines / line numbers, no?
> 
> How about a "raw" API like:
> 
> input_lines = l_gpio_lines("/some/chip", L_GPIO_IN, 1, 2, 6, -1);
> output_lines = l_gpio_lines("/some/chip", L_GPIO_OUT, 1, true, 2, false, 
> -1);
> 
> Output lines need a default value to set when the kernel changes them 
> from high-impedance inputs to outputs. I don't particular like how the 
> defaults are set in the above. Feel free to propose something better!

Another issue with varargs is that they presume knowledge at compile 
time.  So the above works if you don't need to query the chip and 
dynamically add lines to / from the request.  Perhaps this is the 99% 
use case anyway.

Can you open the chip for both input & output?

> 
> Lookup by label is then available via helpers:
> 
> chip_path = l_gpio_label_chip("chip-label");
> input_lines = l_gpio_label_lines(chip_path, L_GPIO_IN, "3v3", "reset", 
> "led", NULL);
> 
> chip_path = l_gpio_chip_with_line_labels("3v3", "reset", "led", NULL);
> output_lines = l_gpio_label_lines(chip_path, L_GPIO_OUT, "3v3", "reset", 

I guess you need the defaults here...

> "led", NULL);
>

The only other possibility would be to add ability to add lines 
dynamically. Maybe roughly:

l_gpio_request *req = l_gpio_request_new();
int line = l_gpio_chip_lookup_line(chip, "3v3");
l_gpio_add_line(gpio, line, 0 // default);
gpio = l_gpio_new("/foo/bar", L_GPIO_OUT, req);

 From the above, what is absolutely required for your oFono work?  Just 
the vararg style constructor with L_GPIO_OUT and being able to specify 
by line or by label?

Regards,
-Denis

Re: [PATCHv2] gpio: add simple get/set helpers for GPIO lines

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

[-- Attachment #1: Type: text/plain, Size: 4905 bytes --]

Hi Denis,

I am really sorry for being silent for the past half year - more 
pressing issues arose that needed my attention.

I hope you can find the time to look into this again.

On 20/12/2018 21.40, Denis Kenzior wrote:
> On 12/20/2018 01:45 PM, Martin Hundebøll wrote:
>> On 20/12/2018 18.51, Denis Kenzior wrote:
>>> Ah, okay.  I think I get what you're trying to do.  So you want to 
>>> have something like:
>>>
>>> struct l_gpio *l_gpio_new(const char *file_path, ...);
>>>
>>> Which would open(file_path), and issue GPIO_GET_CHIPINFO_IOCTL and 
>>> GPIO_GET_LINEINFO_IOCTL to find the right lines, then
>>>
>>> GPIO_GET_LINEHANDLE_IOCTL with GPIOHANDLE_REQUEST_OUTPUT and 
>>> GPIOHANDLE_REQUEST_INPUT flags set to obtain exclusive rights to the 
>>> lines.
>>>
>>> bool l_gpio_set_values(struct l_gpio *, ...);
>>>
>>> would issue a GPIOHANDLE_SET_LINE_VALUES_IOCTL
>>>
>>> bool l_gpio_get_values(struct l_gpio *, ...);
>>>
>>> would issue a GPIOHANDLE_GET_LINE_VALUES_IOCTL,
>>>
>>> Am I right so far?
>>
>> Yes, this is pretty much how I have come to imagine it.
>>
> 
> Okay.  One more doubt I have is what is the practical purpose of 
> obtaining exclusive rights to the GPIO lines for reading only.  That 
> seems kind of useless?  Especially if you need to provide default values 
> for GPIOHANDLE_REQUEST_OUTPUT requests?
> 
> Perhaps reading should be on a different object?

While looking at this again, I concluded that having separate types for 
input and output gpios makes sense:

struct l_gpio_out;
struct l_gpio_in;

This also allows doing the two parts in individual steps.

>>> And use it something like:
>>>
>>> struct l_gpio *gpio = l_gpio_new("/foo/bar", "3v3", "reset", "led", 
>>> NULL);
>>> l_gpio_set_values(gpio, true, true, false);
>>> l_gpio_get_values(gpio, &foo, &bar, &baz);
>>>
>>> So the only issue I see here is how does one discover the lines for a 
>>> given chip?  You might still want a l_gpio_chip abstraction to 
>>> discover lines / line numbers, no?
>>
>> How about a "raw" API like:
>>
>> input_lines = l_gpio_lines("/some/chip", L_GPIO_IN, 1, 2, 6, -1);
>> output_lines = l_gpio_lines("/some/chip", L_GPIO_OUT, 1, true, 2, 
>> false, -1);
>>
>> Output lines need a default value to set when the kernel changes them 
>> from high-impedance inputs to outputs. I don't particular like how the 
>> defaults are set in the above. Feel free to propose something better!
> 
> Another issue with varargs is that they presume knowledge at compile 
> time.  So the above works if you don't need to query the chip and 
> dynamically add lines to / from the request.  Perhaps this is the 99% 
> use case anyway.
> 
> Can you open the chip for both input & output?
> 
>>
>> Lookup by label is then available via helpers:
>>
>> chip_path = l_gpio_label_chip("chip-label");
>> input_lines = l_gpio_label_lines(chip_path, L_GPIO_IN, "3v3", "reset", 
>> "led", NULL);
>>
>> chip_path = l_gpio_chip_with_line_labels("3v3", "reset", "led", NULL);
>> output_lines = l_gpio_label_lines(chip_path, L_GPIO_OUT, "3v3", "reset", 
> 
> I guess you need the defaults here...
> 
>> "led", NULL);
>>
> 
> The only other possibility would be to add ability to add lines 
> dynamically. Maybe roughly:
> 
> l_gpio_request *req = l_gpio_request_new();
> int line = l_gpio_chip_lookup_line(chip, "3v3");
> l_gpio_add_line(gpio, line, 0 // default);
> gpio = l_gpio_new("/foo/bar", L_GPIO_OUT, req);
> 
>  From the above, what is absolutely required for your oFono work?  Just 
> the vararg style constructor with L_GPIO_OUT and being able to specify 
> by line or by label?

I have implemented my ofono driver using libgpiod for now. To use ell 
instead it would need to:

1. (optionally) lookup a gpio line by label
2. open the gpio line as output with a default value
3. set the value to true/false

These could be supported by the following:

   struct l_gpio_out;

   struct l_gpio_out *l_gpio_out(const char *chip, uint32_t line_num,
                                         bool default_value);

   struct l_gpio_out *l_gpio_out_label(const char *chip,
                                         const char *label,
                                         bool default_value);

   void l_gpio_out_free(struct l_gpio_out *gpio_out);

   bool l_gpio_out_set(struct l_gpio_out *gpio_out, bool value);

   bool l_gpio_find(char *chip, uint32_t *line_num);

This should allow to add the needed parts for multiple-lines, inputs, 
and gpio-chips at a later point. I.e. adding API for

   struct l_gpio_chip;
   struct l_gpio_in;

and maybe also

   struct l_gpio_out *l_gpio_out_many(const char *chip,
                                         uint32_t line_num1, bool val1,
                                         ..., -1);

// Martin

Re: [PATCHv2] gpio: add simple get/set helpers for GPIO lines

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3305 bytes --]

Hi Martin,

> I have implemented my ofono driver using libgpiod for now. To use ell 
> instead it would need to:
> 
> 1. (optionally) lookup a gpio line by label
> 2. open the gpio line as output with a default value
> 3. set the value to true/false
> 
> These could be supported by the following:
> 
>    struct l_gpio_out;

I'm not super excited about the name.  Maybe l_gpio_writer?

> 
>    struct l_gpio_out *l_gpio_out(const char *chip, uint32_t line_num,
>                                          bool default_value);
> 
>    struct l_gpio_out *l_gpio_out_label(const char *chip,
>                                          const char *label,
>                                          bool default_value);

So do you really want to limit yourself to just a single line per object?

> 
>    void l_gpio_out_free(struct l_gpio_out *gpio_out);
> 
>    bool l_gpio_out_set(struct l_gpio_out *gpio_out, bool value);
> 
>    bool l_gpio_find(char *chip, uint32_t *line_num);

I'm guessing this is missing a const char * variable?

> 
> This should allow to add the needed parts for multiple-lines, inputs, 
> and gpio-chips at a later point. I.e. adding API for
> 
>    struct l_gpio_chip;
>    struct l_gpio_in;
> 
> and maybe also
> 
>    struct l_gpio_out *l_gpio_out_many(const char *chip,
>                                          uint32_t line_num1, bool val1,
>                                          ..., -1);

This still doesn't solve the dynamically discovered lines issue.

I would maybe do something like this.  Perhaps some of this might be 
redundant or not needed...

struct l_gpio_info *l_gpio_discover_lines(const char *device);
const char *l_gpio_info_get_name(struct l_gpio_info *);
const char *l_gpio_info_get_label(struct l_gpio_info *);
uint32_t l_gpio_info_get_num_lines(struct l_gpio_info *);
char **l_gpio_get_lines(struct l_gpio_info *);
uint32_t l_gpio_info_find_offset(struct l_gpio_info *, const char *name);
const char *l_gpio_info_get_name(struct l_gpio_info *, uint32_t offset);
void l_gpio_info_free(struct l_gpio_info *);
int l_gpio_get_values_by_name(const char *device, const char *line, ...);
int l_gpio_get_values_by_offset(const char *device, int offset, ...);

struct l_gpio_writer *l_gpio_new(const char *device, uint32_t n_lines, 
const uint32_t lines[][2]);
l_gpio_free(struct l_gpio *);
bool l_gpio_set_linesv(struct l_gpio *gpio, uint32_t n_lines, const 
uint32_t lines[][2]);
bool l_gpio_set_line(struct l_gpio *gpio, uint32_t line, uint32_t value);
bool l_gpio_set_lines(struct l_gpio *gpio, int line, ...);

So at the bare minimum, you could do:

struct l_gpio_info *info = l_gpio_discover_lines("/foo/bar");
uint32_t line = l_gpio_info_find_offset(info, "gpio_line");
uint32_t lines[1][2];
struct l_gpio *gpio;

lines[0][0] = line;
lines[0][1] = some_default;
l_gpio_info_free(info);

gpio = l_gpio_new("/foo/bar", 1, lines);

// off
l_gpio_set_line(gpio, lines[0][0], 0);

// on
l_gpio_set_line(gpio, lines[0][0], 1);

> 
> // Martin

Regards,
-Denis

Re: [PATCHv2] gpio: add simple get/set helpers for GPIO lines

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

[-- Attachment #1: Type: text/plain, Size: 5831 bytes --]

Hi Denis,

On 29/05/2019 01.46, Denis Kenzior wrote:
>> I have implemented my ofono driver using libgpiod for now. To use ell 
>> instead it would need to:
>>
>> 1. (optionally) lookup a gpio line by label
>> 2. open the gpio line as output with a default value
>> 3. set the value to true/false
>>
>> These could be supported by the following:
>>
>>    struct l_gpio_out;
> 
> I'm not super excited about the name.  Maybe l_gpio_writer?

Sounds good.

>>
>>    struct l_gpio_out *l_gpio_out(const char *chip, uint32_t line_num,
>>                                          bool default_value);
>>
>>    struct l_gpio_out *l_gpio_out_label(const char *chip,
>>                                          const char *label,
>>                                          bool default_value);
> 
> So do you really want to limit yourself to just a single line per object?

I figured a single l_gpio_writer object could easily represent multiple 
lines. But for handling power to my peripheral only a single line is needed.

If/when we implement multi-line handling, the single-line functions can 
be turned into wrappers that set up 1-sized arrays.

>>
>>    void l_gpio_out_free(struct l_gpio_out *gpio_out);
>>
>>    bool l_gpio_out_set(struct l_gpio_out *gpio_out, bool value);
>>
>>    bool l_gpio_find(char *chip, uint32_t *line_num);
> 
> I'm guessing this is missing a const char * variable?

Ahh, yes. So like this:

     bool l_gpio_find(const char *label, char *chip, uint32_t *line_num);

I wanted this to search all available gpio chips and return the 
chip-name in char *chip.

My use-case as a BSP developer is to tell the application developer to 
simply open gpio label "FOO" and then let the BSP care about gpio chips 
and line numbers. The label might exist on gpio chip X on one board, and 
on gpio chip Y on another.

If we really want to support cases where a label exists on multiple 
chips, then we could implement the above as

     char **l_gpio_find_chip(const char *label);

The user can decide what to do if multiple devices are returned...

>>
>> This should allow to add the needed parts for multiple-lines, inputs, 
>> and gpio-chips at a later point. I.e. adding API for
>>
>>    struct l_gpio_chip;
>>    struct l_gpio_in;
>>
>> and maybe also
>>
>>    struct l_gpio_out *l_gpio_out_many(const char *chip,
>>                                          uint32_t line_num1, bool val1,
>>                                          ..., -1);
> 
> This still doesn't solve the dynamically discovered lines issue.

varargs are probably not the right choice.

> I would maybe do something like this.  Perhaps some of this might be 
> redundant or not needed...
> 
> struct l_gpio_info *l_gpio_discover_lines(const char *device);
> const char *l_gpio_info_get_name(struct l_gpio_info *);
> const char *l_gpio_info_get_label(struct l_gpio_info *);
> uint32_t l_gpio_info_get_num_lines(struct l_gpio_info *);
> char **l_gpio_get_lines(struct l_gpio_info *);
> uint32_t l_gpio_info_find_offset(struct l_gpio_info *, const char *name);
> const char *l_gpio_info_get_name(struct l_gpio_info *, uint32_t offset);
> void l_gpio_info_free(struct l_gpio_info *);

I think we should use l_gpio_device instead of l_gpio_info to make it 
clear that the names and labels are for the chip and not the line.

> int l_gpio_get_values_by_name(const char *device, const char *line, ...);
> int l_gpio_get_values_by_offset(const char *device, int offset, ...);
> 
> struct l_gpio_writer *l_gpio_new(const char *device, uint32_t n_lines, 
> const uint32_t lines[][2]);

I like this. I guess the following functions should have _writer in 
their names, and take a struct l_gpio_writer * as first argument?

> l_gpio_free(struct l_gpio *);
> bool l_gpio_set_linesv(struct l_gpio *gpio, uint32_t n_lines, const 
> uint32_t lines[][2]);
> bool l_gpio_set_line(struct l_gpio *gpio, uint32_t line, uint32_t value);
> bool l_gpio_set_lines(struct l_gpio *gpio, int line, ...);

I prefer the l_gpio_set_linesv() approach to this.

> So at the bare minimum, you could do:
> 
> struct l_gpio_info *info = l_gpio_discover_lines("/foo/bar");
> uint32_t line = l_gpio_info_find_offset(info, "gpio_line");
> uint32_t lines[1][2];
> struct l_gpio *gpio;
> 
> lines[0][0] = line;
> lines[0][1] = some_default;
> l_gpio_info_free(info);
> 
> gpio = l_gpio_new("/foo/bar", 1, lines);
> 
> // off
> l_gpio_set_line(gpio, lines[0][0], 0);
> 
> // on
> l_gpio_set_line(gpio, lines[0][0], 1);

How about:

char **devices;
struct l_gpio_device *device;
uint32_t lines[1][2];
struct l_gpio_writer *line;

devices = l_gpio_devices_with_line_label("my_label");
if (!devices)
	return;

if (l_strv_length(devices) != 1) {
	l_error("label not unique");
	return;
}

device = l_gpio_device_new(devices[0]);
if (!device)
	return;

l_strfreev(devices);

if (!l_gpio_device_find_offset(device, "my_label", &lines[0][0]);
	return;

lines[0][1] = 0;

line = l_gpio_writer_new(device, 1, lines);
if (!line)
	return;

l_gpio_device_free(device);

// off
l_gpio_writer_set(line, lines[0][0], 0);

// on
l_gpio_writer_set(line, lines[0][0], 1);

// many
l_gpio_writer_setv(line, 1, lines);

l_gpio_writer_free(line);



We could also consider using a struct l_gpio_line_value to represent 
line numbers and values:

struct l_gpio_line_value {
	uint32_t line_number;
	uint32_t line_value;
};

and take an array of those as argument instead of uint32_t[][2].

// Martin

Re: [PATCHv2] gpio: add simple get/set helpers for GPIO lines

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 6189 bytes --]

Hi Martin,

>> So do you really want to limit yourself to just a single line per object?
> 
> I figured a single l_gpio_writer object could easily represent multiple 
> lines. But for handling power to my peripheral only a single line is 
> needed.
> 
> If/when we implement multi-line handling, the single-line functions can 
> be turned into wrappers that set up 1-sized arrays.
> 

I think you're not going to gain much up front and will make your job 
(or someone else's) much harder if you do that.

>>>
>>>    void l_gpio_out_free(struct l_gpio_out *gpio_out);
>>>
>>>    bool l_gpio_out_set(struct l_gpio_out *gpio_out, bool value);
>>>
>>>    bool l_gpio_find(char *chip, uint32_t *line_num);
>>
>> I'm guessing this is missing a const char * variable?
> 
> Ahh, yes. So like this:
> 
>      bool l_gpio_find(const char *label, char *chip, uint32_t *line_num);
> 
> I wanted this to search all available gpio chips and return the 
> chip-name in char *chip.

So chip is a return value? In that case char **chip.  But I'm still 
confused how you're going to accomplish this?  Are you going to scan 
sysfs for this?

> 
> My use-case as a BSP developer is to tell the application developer to 
> simply open gpio label "FOO" and then let the BSP care about gpio chips 
> and line numbers. The label might exist on gpio chip X on one board, and 
> on gpio chip Y on another.
> 
> If we really want to support cases where a label exists on multiple 
> chips, then we could implement the above as
> 
>      char **l_gpio_find_chip(const char *label);

This sounds more general

> 
> The user can decide what to do if multiple devices are returned...
> 
>>>
>>> This should allow to add the needed parts for multiple-lines, inputs, 
>>> and gpio-chips at a later point. I.e. adding API for
>>>
>>>    struct l_gpio_chip;
>>>    struct l_gpio_in;
>>>
>>> and maybe also
>>>
>>>    struct l_gpio_out *l_gpio_out_many(const char *chip,
>>>                                          uint32_t line_num1, bool val1,
>>>                                          ..., -1);
>>
>> This still doesn't solve the dynamically discovered lines issue.
> 
> varargs are probably not the right choice.

I think ultimately we may want both, but I'm fine holding off 
implementing them until that day comes.

> 
>> I would maybe do something like this.  Perhaps some of this might be 
>> redundant or not needed...
>>
>> struct l_gpio_info *l_gpio_discover_lines(const char *device);
>> const char *l_gpio_info_get_name(struct l_gpio_info *);
>> const char *l_gpio_info_get_label(struct l_gpio_info *);
>> uint32_t l_gpio_info_get_num_lines(struct l_gpio_info *);
>> char **l_gpio_get_lines(struct l_gpio_info *);
>> uint32_t l_gpio_info_find_offset(struct l_gpio_info *, const char *name);
>> const char *l_gpio_info_get_name(struct l_gpio_info *, uint32_t offset);
>> void l_gpio_info_free(struct l_gpio_info *);
> 
> I think we should use l_gpio_device instead of l_gpio_info to make it 
> clear that the names and labels are for the chip and not the line.

That's fine, or use l_gpio_chip or l_gpio_chip_info to keep in line with 
the kernel naming.

> 
>> int l_gpio_get_values_by_name(const char *device, const char *line, ...);
>> int l_gpio_get_values_by_offset(const char *device, int offset, ...);
>>
>> struct l_gpio_writer *l_gpio_new(const char *device, uint32_t n_lines, 
>> const uint32_t lines[][2]);
> 
> I like this. I guess the following functions should have _writer in 
> their names, and take a struct l_gpio_writer * as first argument?

I'm also fine naming them l_gpio.  Not sure what looks better yet 
honestly.  We can fight over that later.

> 
>> l_gpio_free(struct l_gpio *);
>> bool l_gpio_set_linesv(struct l_gpio *gpio, uint32_t n_lines, const 
>> uint32_t lines[][2]);
>> bool l_gpio_set_line(struct l_gpio *gpio, uint32_t line, uint32_t value);
>> bool l_gpio_set_lines(struct l_gpio *gpio, int line, ...);
> 
> I prefer the l_gpio_set_linesv() approach to this.
> 

Sure, lets implement the set_line & set_linesv first.

>> So at the bare minimum, you could do:
>>
>> struct l_gpio_info *info = l_gpio_discover_lines("/foo/bar");
>> uint32_t line = l_gpio_info_find_offset(info, "gpio_line");
>> uint32_t lines[1][2];
>> struct l_gpio *gpio;
>>
>> lines[0][0] = line;
>> lines[0][1] = some_default;
>> l_gpio_info_free(info);
>>
>> gpio = l_gpio_new("/foo/bar", 1, lines);
>>
>> // off
>> l_gpio_set_line(gpio, lines[0][0], 0);
>>
>> // on
>> l_gpio_set_line(gpio, lines[0][0], 1);
> 
> How about:
> 
> char **devices;
> struct l_gpio_device *device;
> uint32_t lines[1][2];
> struct l_gpio_writer *line;
> 
> devices = l_gpio_devices_with_line_label("my_label");
> if (!devices)
>      return;
> 
> if (l_strv_length(devices) != 1) {
>      l_error("label not unique");
>      return;
> }
> 
> device = l_gpio_device_new(devices[0]);
> if (!device)
>      return;
> 
> l_strfreev(devices);
> 
> if (!l_gpio_device_find_offset(device, "my_label", &lines[0][0]);
>      return;
> 
> lines[0][1] = 0;
> 
> line = l_gpio_writer_new(device, 1, lines);
> if (!line)
>      return;
> 
> l_gpio_device_free(device);
> 
> // off
> l_gpio_writer_set(line, lines[0][0], 0);
> 
> // on
> l_gpio_writer_set(line, lines[0][0], 1);
> 
> // many
> l_gpio_writer_setv(line, 1, lines);
> 
> l_gpio_writer_free(line);
> 
>

That looks fine to me.

> 
> We could also consider using a struct l_gpio_line_value to represent 
> line numbers and values:
> 
> struct l_gpio_line_value {
>      uint32_t line_number;
line_offset I guess.
>      uint32_t line_value;
> };
> 
> and take an array of those as argument instead of uint32_t[][2].

Yep.  I thought about that, but generally I dislike introducing 
non-opaque types into the public namespace unless absolutely needed. 
But in this case it doesn't matter a whole lot, so I'm fine either way.

Regards,
-Denis

Re: [PATCHv2] gpio: add simple get/set helpers for GPIO lines

[Martin =?unknown-8bit?q?Hundeb=C3=B8ll?=]

[-- Attachment #1: Type: text/plain, Size: 594 bytes --]

Hi Denis,

On 29/05/2019 18.46, Denis Kenzior wrote:
>>
>> // off
>> l_gpio_writer_set(line, lines[0][0], 0);
>>
>> // on
>> l_gpio_writer_set(line, lines[0][0], 1);
>>
>> // many
>> l_gpio_writer_setv(line, 1, lines);
> 
> That looks fine to me.

While working at implementing this, I realized that the kernel API 
requires setting all lines at once, so I guess we have to settle with:

l_gpio_writer_set(struct l_gpio_writer *, uint32_t, uint32_t[]);

And bail out if the number of values doesn't match the number of lines 
passed when creating the writer.

// Martin

Re: [PATCHv2] gpio: add simple get/set helpers for GPIO lines

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1365 bytes --]

Hi Martin,

On 06/03/2019 03:56 PM, Martin Hundebøll wrote:
> Hi Denis,
> 
> On 29/05/2019 18.46, Denis Kenzior wrote:
>>>
>>> // off
>>> l_gpio_writer_set(line, lines[0][0], 0);
>>>
>>> // on
>>> l_gpio_writer_set(line, lines[0][0], 1);
>>>
>>> // many
>>> l_gpio_writer_setv(line, 1, lines);
>>
>> That looks fine to me.
> 
> While working at implementing this, I realized that the kernel API 
> requires setting all lines at once, so I guess we have to settle with:
> 
> l_gpio_writer_set(struct l_gpio_writer *, uint32_t, uint32_t[]);
> 
> And bail out if the number of values doesn't match the number of lines 
> passed when creating the writer.

I think this is fine for the first iteration.  But one thing occurs to 
me is that since we have exclusive access to the GPIO for writing, we 
still can support the vararg or offset variation by simply storing all 
the values we set into the GPIO and maintain/buffer the values 
persistently.  So if the user provides just a single value out of N, 
then we just update that value and set the whole thing in its entirety.

This can lead to a bit nicer API where you can simply do:

l_gpio_writer_set(writer, "gpio_line_label", value);

With the above, we can safely drop the whole set_one / get_one and use 
the much nicer form above.

> 
> // Martin

Regards,
-Denis