[libgpiod v2][PATCH 0/3] api tweaks

[libgpiod v2][PATCH 0/3] api tweaks

[Kent Gibson]

This series builds on my recent doc tweaks series, picking up a few
things I missed there.

The first patch is another constistency renaming. This is the last
one of these - this time I checked all the function names in gpiod.h
to ensure all the functions follow the pattern.

The second is splitting chip_info out from chip.  This is probably
something you looked at before and decided against, but it bugs
me that the core library is doing more than it should here.

The final one is a variable renaming for clarity.  You use a "buf"
suffix to indicate the uAPI version of structs.  I find that confusing
as to me buf implies an array of structs.  I used a "u" for uAPI
prefix instead, though on reflection "k" for kernel might be more
visually distinct.  There are other examples of this throughout the
core - happy to do a respin including those - this one just fixes
line-info.c to bring it into line with the corresponding chip-info.c
introduced in the previous patch.

As with all my v2 patches, happy for you to apply them, or not,
in whatever way you find appropriate.

Cheers,
Kent.

Kent Gibson (3):
  core: rename gpiod_chip_info_event_wait and gpiod_chip_info_event_read
  core: split chip_info out of chip
  line-info: rename infobuf to uinfo

 include/gpiod.h            | 73 +++++++++++++++++++++++--------
 lib/Makefile.am            |  1 +
 lib/chip-info.c            | 72 ++++++++++++++++++++++++++++++
 lib/chip.c                 | 89 ++++++++++++++++----------------------
 lib/internal.h             |  2 +
 lib/line-info.c            | 38 ++++++++--------
 tests/Makefile.am          |  1 +
 tests/gpiod-test-helpers.h |  3 ++
 tests/tests-chip-info.c    | 54 +++++++++++++++++++++++
 tests/tests-chip.c         | 32 --------------
 tests/tests-info-event.c   | 22 +++++-----
 tools/gpiodetect.c         | 13 ++++--
 tools/gpiofind.c           |  8 +++-
 tools/gpioinfo.c           | 16 ++++---
 14 files changed, 283 insertions(+), 141 deletions(-)
 create mode 100644 lib/chip-info.c
 create mode 100644 tests/tests-chip-info.c


base-commit: 6e15b78d6e9c956c295c755aed793ffd963b1c53
prerequisite-patch-id: 4c18b0ccbdc27f0478078c520fc831c6f8e99cae
prerequisite-patch-id: 9813ae923bac1692f9558db3c09081c3d088ce78
prerequisite-patch-id: ebd388a7c411a6b730f23a6995852e62c07fb369
prerequisite-patch-id: bf291d6ac4300ed0ef6bf09eb187530098a7a338
prerequisite-patch-id: 3af98e60123f5e4b1199a2e0fb06eb6e1cfda18f
prerequisite-patch-id: 26c5fa12c404c09e78b9dfdf7fab5e3e7afcce4e
-- 
2.35.1

[libgpiod v2][PATCH 1/3] core: rename gpiod_chip_info_event_wait and gpiod_chip_info_event_read

[Kent Gibson]

Another couple of functions that don't follow the
gpiod_<object>_<action>_<object> pattern.
So rename gpiod_chip_info_event_wait to gpiod_chip_wait_info_event
and gpiod_chip_info_event_read to gpiod_chip_read_info_event.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 include/gpiod.h          |  4 ++--
 lib/chip.c               |  4 ++--
 tests/tests-info-event.c | 22 +++++++++++-----------
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/include/gpiod.h b/include/gpiod.h
index 2365630..956ee12 100644
--- a/include/gpiod.h
+++ b/include/gpiod.h
@@ -151,7 +151,7 @@ int gpiod_chip_get_fd(struct gpiod_chip *chip);
  * @return 0 if wait timed out, -1 if an error occurred, 1 if an event is
  *	   pending.
  */
-int gpiod_chip_info_event_wait(struct gpiod_chip *chip, uint64_t timeout_ns);
+int gpiod_chip_wait_info_event(struct gpiod_chip *chip, uint64_t timeout_ns);
 
 /**
  * @brief Read a single line status change event from the chip.
@@ -160,7 +160,7 @@ int gpiod_chip_info_event_wait(struct gpiod_chip *chip, uint64_t timeout_ns);
  *	   freed by the caller using ::gpiod_info_event_free.
  * @note If no events are pending, this function will block.
  */
-struct gpiod_info_event *gpiod_chip_info_event_read(struct gpiod_chip *chip);
+struct gpiod_info_event *gpiod_chip_read_info_event(struct gpiod_chip *chip);
 
 /**
  * @brief Map a line's name to its offset within the chip.
diff --git a/lib/chip.c b/lib/chip.c
index b881be1..50d8312 100644
--- a/lib/chip.c
+++ b/lib/chip.c
@@ -164,14 +164,14 @@ GPIOD_API int gpiod_chip_get_fd(struct gpiod_chip *chip)
 	return chip->fd;
 }
 
-GPIOD_API int gpiod_chip_info_event_wait(struct gpiod_chip *chip,
+GPIOD_API int gpiod_chip_wait_info_event(struct gpiod_chip *chip,
 					 uint64_t timeout_ns)
 {
 	return gpiod_poll_fd(chip->fd, timeout_ns);
 }
 
 GPIOD_API struct gpiod_info_event *
-gpiod_chip_info_event_read(struct gpiod_chip *chip)
+gpiod_chip_read_info_event(struct gpiod_chip *chip)
 {
 	return gpiod_info_event_read_fd(chip->fd);
 }
diff --git a/tests/tests-info-event.c b/tests/tests-info-event.c
index 32ae690..0640d66 100644
--- a/tests/tests-info-event.c
+++ b/tests/tests-info-event.c
@@ -50,7 +50,7 @@ GPIOD_TEST_CASE(event_timeout)
 	g_assert_nonnull(info);
 	gpiod_test_return_if_failed();
 
-	ret = gpiod_chip_info_event_wait(chip, 100000000);
+	ret = gpiod_chip_wait_info_event(chip, 100000000);
 	g_assert_cmpint(ret, ==, 0);
 }
 
@@ -130,11 +130,11 @@ GPIOD_TEST_CASE(request_reconfigure_release_events)
 			      request_reconfigure_release_line, &ctx);
 	g_thread_ref(thread);
 
-	ret = gpiod_chip_info_event_wait(chip, 1000000000);
+	ret = gpiod_chip_wait_info_event(chip, 1000000000);
 	g_assert_cmpint(ret, >, 0);
 	gpiod_test_join_thread_and_return_if_failed(thread);
 
-	request_event = gpiod_chip_info_event_read(chip);
+	request_event = gpiod_chip_read_info_event(chip);
 	g_assert_nonnull(request_event);
 	gpiod_test_join_thread_and_return_if_failed(thread);
 
@@ -148,11 +148,11 @@ GPIOD_TEST_CASE(request_reconfigure_release_events)
 	g_assert_cmpint(gpiod_line_info_get_direction(request_info), ==,
 			GPIOD_LINE_DIRECTION_INPUT);
 
-	ret = gpiod_chip_info_event_wait(chip, 1000000000);
+	ret = gpiod_chip_wait_info_event(chip, 1000000000);
 	g_assert_cmpint(ret, >, 0);
 	gpiod_test_join_thread_and_return_if_failed(thread);
 
-	reconfigure_event = gpiod_chip_info_event_read(chip);
+	reconfigure_event = gpiod_chip_read_info_event(chip);
 	g_assert_nonnull(reconfigure_event);
 	gpiod_test_join_thread_and_return_if_failed(thread);
 
@@ -166,11 +166,11 @@ GPIOD_TEST_CASE(request_reconfigure_release_events)
 	g_assert_cmpint(gpiod_line_info_get_direction(reconfigure_info), ==,
 			GPIOD_LINE_DIRECTION_OUTPUT);
 
-	ret = gpiod_chip_info_event_wait(chip, 1000000000);
+	ret = gpiod_chip_wait_info_event(chip, 1000000000);
 	g_assert_cmpint(ret, >, 0);
 	gpiod_test_join_thread_and_return_if_failed(thread);
 
-	release_event = gpiod_chip_info_event_read(chip);
+	release_event = gpiod_chip_read_info_event(chip);
 	g_assert_nonnull(release_event);
 	gpiod_test_join_thread_and_return_if_failed(thread);
 
@@ -243,7 +243,7 @@ GPIOD_TEST_CASE(chip_fd_can_be_polled)
 	g_assert_cmpint(ret, >, 0);
 	gpiod_test_join_thread_and_return_if_failed(thread);
 
-	event = gpiod_chip_info_event_read(chip);
+	event = gpiod_chip_read_info_event(chip);
 	g_assert_nonnull(event);
 	gpiod_test_join_thread_and_return_if_failed(thread);
 
@@ -283,11 +283,11 @@ GPIOD_TEST_CASE(unwatch_and_check_that_no_events_are_generated)
 
 	request = gpiod_test_request_lines_or_fail(chip, req_cfg, line_cfg);
 
-	ret = gpiod_chip_info_event_wait(chip, 100000000);
+	ret = gpiod_chip_wait_info_event(chip, 100000000);
 	g_assert_cmpint(ret, >, 0);
 	gpiod_test_return_if_failed();
 
-	event = gpiod_chip_info_event_read(chip);
+	event = gpiod_chip_read_info_event(chip);
 	g_assert_nonnull(event);
 	gpiod_test_return_if_failed();
 
@@ -298,6 +298,6 @@ GPIOD_TEST_CASE(unwatch_and_check_that_no_events_are_generated)
 	gpiod_line_request_release(request);
 	request = NULL;
 
-	ret = gpiod_chip_info_event_wait(chip, 100000000);
+	ret = gpiod_chip_wait_info_event(chip, 100000000);
 	g_assert_cmpint(ret, ==, 0);
 }
-- 
2.35.1

[libgpiod v2][PATCH 2/3] core: split chip_info out of chip

[Kent Gibson]

The core library should be minimalist, only performing the operations
required, but gpiod_chip_open() always fetches the chip info,
even though the majority of the time the chip will only be used to
request a line, so the chip info is not required.

Split the chip_info from the chip, in the same style as line_info, and
update methods and tools appropriately.

In the rare occasions that the user requires the chip info they can
request it themselves using gpiod_chip_get_info(), as demonstrated
in the tool changes.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 include/gpiod.h            | 69 +++++++++++++++++++++++--------
 lib/Makefile.am            |  1 +
 lib/chip-info.c            | 72 ++++++++++++++++++++++++++++++++
 lib/chip.c                 | 85 ++++++++++++++++----------------------
 lib/internal.h             |  2 +
 tests/Makefile.am          |  1 +
 tests/gpiod-test-helpers.h |  3 ++
 tests/tests-chip-info.c    | 54 ++++++++++++++++++++++++
 tests/tests-chip.c         | 32 --------------
 tools/gpiodetect.c         | 13 ++++--
 tools/gpiofind.c           |  8 +++-
 tools/gpioinfo.c           | 16 ++++---
 12 files changed, 249 insertions(+), 107 deletions(-)
 create mode 100644 lib/chip-info.c
 create mode 100644 tests/tests-chip-info.c

diff --git a/include/gpiod.h b/include/gpiod.h
index 956ee12..e099515 100644
--- a/include/gpiod.h
+++ b/include/gpiod.h
@@ -41,6 +41,7 @@ extern "C" {
  */
 
 struct gpiod_chip;
+struct gpiod_chip_info;
 struct gpiod_line_info;
 struct gpiod_line_config;
 struct gpiod_request_config;
@@ -75,18 +76,12 @@ struct gpiod_chip *gpiod_chip_open(const char *path);
 void gpiod_chip_close(struct gpiod_chip *chip);
 
 /**
- * @brief Get the name of the chip as represented in the kernel.
+ * @brief Get information about the chip.
  * @param chip GPIO chip object.
- * @return Pointer to a human-readable string containing the chip name.
+ * @return New GPIO chip info object or NULL if an error occurred. The returned
+ *         object must be freed by the caller using ::gpiod_chip_info_free.
  */
-const char *gpiod_chip_get_name(struct gpiod_chip *chip);
-
-/**
- * @brief Get the label of the chip as represented in the kernel.
- * @param chip GPIO chip object.
- * @return Pointer to a human-readable string containing the chip label.
- */
-const char *gpiod_chip_get_label(struct gpiod_chip *chip);
+struct gpiod_chip_info *gpiod_chip_get_info(struct gpiod_chip *chip);
 
 /**
  * @brief Get the path used to open the chip.
@@ -95,13 +90,6 @@ const char *gpiod_chip_get_label(struct gpiod_chip *chip);
  */
 const char *gpiod_chip_get_path(struct gpiod_chip *chip);
 
-/**
- * @brief Get the number of lines exposed by the chip.
- * @param chip GPIO chip object.
- * @return Number of GPIO lines.
- */
-size_t gpiod_chip_get_num_lines(struct gpiod_chip *chip);
-
 /**
  * @brief Get a snapshot of information about a line.
  * @param chip GPIO chip object.
@@ -187,6 +175,53 @@ gpiod_chip_request_lines(struct gpiod_chip *chip,
 			 struct gpiod_request_config *req_cfg,
 			 struct gpiod_line_config *line_cfg);
 
+/**
+ * @}
+ *
+ * @defgroup chip_info Chip info
+ * @{
+ *
+ * Functions for retrieving kernel information about chips.
+ *
+ * Line info object contains an immutable snapshot of a chip's status.
+ *
+ * The chip info contains all the publicly available information about a
+ * chip.
+ *
+ * Some accessor methods return pointers.  Those pointers refer to internal
+ * fields.  The lifetimes of those fields are tied to the lifetime of the
+ * containing chip info object.
+ * Such pointers remain valid until ::gpiod_chip_info_free is called on the
+ * containing chip info object. They must not be freed by the caller.
+ */
+
+/**
+ * @brief Free a chip info object and release all associated resources.
+ * @param info GPIO chip info object to free.
+ */
+void gpiod_chip_info_free(struct gpiod_chip_info *info);
+
+/**
+ * @brief Get the name of the chip as represented in the kernel.
+ * @param info GPIO chip info object.
+ * @return Pointer to a human-readable string containing the chip name.
+ */
+const char *gpiod_chip_info_get_name(struct gpiod_chip_info *info);
+
+/**
+ * @brief Get the label of the chip as represented in the kernel.
+ * @param info GPIO chip info object.
+ * @return Pointer to a human-readable string containing the chip label.
+ */
+const char *gpiod_chip_info_get_label(struct gpiod_chip_info *info);
+
+/**
+ * @brief Get the number of lines exposed by the chip.
+ * @param info GPIO chip info object.
+ * @return Number of GPIO lines.
+ */
+size_t gpiod_chip_info_get_num_lines(struct gpiod_chip_info *info);
+
 /**
  * @}
  *
diff --git a/lib/Makefile.am b/lib/Makefile.am
index b6854d9..1bd2b2e 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -3,6 +3,7 @@
 
 lib_LTLIBRARIES = libgpiod.la
 libgpiod_la_SOURCES =	chip.c \
+			chip-info.c \
 			edge-event.c \
 			info-event.c \
 			internal.h \
diff --git a/lib/chip-info.c b/lib/chip-info.c
new file mode 100644
index 0000000..df1f66a
--- /dev/null
+++ b/lib/chip-info.c
@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: LGPL-2.1-or-later
+// SPDX-FileCopyrightText: 2022 Bartosz Golaszewski <brgl@bgdev.pl>
+
+/* Line attribute data structure and functions. */
+
+#include <gpiod.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include "internal.h"
+
+struct gpiod_chip_info {
+	size_t num_lines;
+	char name[32];
+	char label[32];
+};
+
+GPIOD_API void gpiod_chip_info_free(struct gpiod_chip_info *info)
+{
+	if (!info)
+		return;
+
+	free(info);
+}
+
+
+GPIOD_API const char *gpiod_chip_info_get_name(struct gpiod_chip_info *info)
+{
+	return info->name;
+}
+
+GPIOD_API const char *gpiod_chip_info_get_label(struct gpiod_chip_info *info)
+{
+	return info->label;
+}
+
+GPIOD_API size_t gpiod_chip_info_get_num_lines(struct gpiod_chip_info *info)
+{
+	return info->num_lines;
+}
+
+struct gpiod_chip_info *
+gpiod_chip_info_from_kernel(struct gpiochip_info *uinfo)
+{
+	struct gpiod_chip_info *info;
+
+	info = malloc(sizeof(*info));
+	if (!info)
+		return NULL;
+
+	memset(info, 0, sizeof(*info));
+
+	info->num_lines = uinfo->lines;
+
+	/*
+	 * GPIO device must have a name - don't bother checking this field. In
+	 * the worst case (would have to be a weird kernel bug) it'll be empty.
+	 */
+	strncpy(info->name, uinfo->name, sizeof(info->name));
+
+	/*
+	 * The kernel sets the label of a GPIO device to "unknown" if it
+	 * hasn't been defined in DT, board file etc. On the off-chance that
+	 * we got an empty string, do the same.
+	 */
+	if (uinfo->label[0] == '\0')
+		strncpy(info->label, "unknown", sizeof(info->label));
+	else
+		strncpy(info->label, uinfo->label, sizeof(info->label));
+
+	return info;
+}
diff --git a/lib/chip.c b/lib/chip.c
index 50d8312..2dcabb3 100644
--- a/lib/chip.c
+++ b/lib/chip.c
@@ -15,17 +15,13 @@
 
 struct gpiod_chip {
 	int fd;
-	size_t num_lines;
-	char name[32];
-	char label[32];
 	char *path;
 };
 
 GPIOD_API struct gpiod_chip *gpiod_chip_open(const char *path)
 {
-	struct gpiochip_info info;
 	struct gpiod_chip *chip;
-	int ret, fd;
+	int fd;
 
 	if (!gpiod_is_gpiochip_device(path))
 		return NULL;
@@ -39,39 +35,15 @@ GPIOD_API struct gpiod_chip *gpiod_chip_open(const char *path)
 		goto err_close_fd;
 
 	memset(chip, 0, sizeof(*chip));
-	memset(&info, 0, sizeof(info));
 
 	chip->path = strdup(path);
 	if (!chip->path)
 		goto err_free_chip;
 
-	ret = ioctl(fd, GPIO_GET_CHIPINFO_IOCTL, &info);
-	if (ret < 0)
-		goto err_free_path;
-
 	chip->fd = fd;
-	chip->num_lines = info.lines;
-
-	/*
-	 * GPIO device must have a name - don't bother checking this field. In
-	 * the worst case (would have to be a weird kernel bug) it'll be empty.
-	 */
-	strncpy(chip->name, info.name, sizeof(chip->name));
-
-	/*
-	 * The kernel sets the label of a GPIO device to "unknown" if it
-	 * hasn't been defined in DT, board file etc. On the off-chance that
-	 * we got an empty string, do the same.
-	 */
-	if (info.label[0] == '\0')
-		strncpy(chip->label, "unknown", sizeof(chip->label));
-	else
-		strncpy(chip->label, info.label, sizeof(chip->label));
 
 	return chip;
 
-err_free_path:
-	free(chip->path);
 err_free_chip:
 	free(chip);
 err_close_fd:
@@ -90,14 +62,29 @@ GPIOD_API void gpiod_chip_close(struct gpiod_chip *chip)
 	free(chip);
 }
 
-GPIOD_API const char *gpiod_chip_get_name(struct gpiod_chip *chip)
+static int chip_read_chip_info(int fd, struct gpiochip_info *info)
 {
-	return chip->name;
+	int ret;
+
+	memset(info, 0, sizeof(*info));
+
+	ret = ioctl(fd, GPIO_GET_CHIPINFO_IOCTL, info);
+	if (ret)
+		return -1;
+
+	return 0;
 }
 
-GPIOD_API const char *gpiod_chip_get_label(struct gpiod_chip *chip)
+GPIOD_API struct gpiod_chip_info *gpiod_chip_get_info(struct gpiod_chip *chip)
 {
-	return chip->label;
+	struct gpiochip_info info;
+	int ret;
+
+	ret = chip_read_chip_info(chip->fd, &info);
+	if (ret < 0)
+		return NULL;
+
+	return gpiod_chip_info_from_kernel(&info);
 }
 
 GPIOD_API const char *gpiod_chip_get_path(struct gpiod_chip *chip)
@@ -105,23 +92,18 @@ GPIOD_API const char *gpiod_chip_get_path(struct gpiod_chip *chip)
 	return chip->path;
 }
 
-GPIOD_API size_t gpiod_chip_get_num_lines(struct gpiod_chip *chip)
-{
-	return chip->num_lines;
-}
-
 static int chip_read_line_info(int fd, unsigned int offset,
-			       struct gpio_v2_line_info *infobuf, bool watch)
+			       struct gpio_v2_line_info *info, bool watch)
 {
 	int ret, cmd;
 
-	memset(infobuf, 0, sizeof(*infobuf));
-	infobuf->offset = offset;
+	memset(info, 0, sizeof(*info));
+	info->offset = offset;
 
 	cmd = watch ? GPIO_V2_GET_LINEINFO_WATCH_IOCTL :
 		      GPIO_V2_GET_LINEINFO_IOCTL;
 
-	ret = ioctl(fd, cmd, infobuf);
+	ret = ioctl(fd, cmd, info);
 	if (ret)
 		return -1;
 
@@ -131,14 +113,14 @@ static int chip_read_line_info(int fd, unsigned int offset,
 static struct gpiod_line_info *
 chip_get_line_info(struct gpiod_chip *chip, unsigned int offset, bool watch)
 {
-	struct gpio_v2_line_info infobuf;
+	struct gpio_v2_line_info info;
 	int ret;
 
-	ret = chip_read_line_info(chip->fd, offset, &infobuf, watch);
+	ret = chip_read_line_info(chip->fd, offset, &info, watch);
 	if (ret)
 		return NULL;
 
-	return gpiod_line_info_from_kernel(&infobuf);
+	return gpiod_line_info_from_kernel(&info);
 }
 
 GPIOD_API struct gpiod_line_info *
@@ -178,16 +160,21 @@ gpiod_chip_read_info_event(struct gpiod_chip *chip)
 
 GPIOD_API int gpiod_chip_find_line(struct gpiod_chip *chip, const char *name)
 {
-	struct gpio_v2_line_info infobuf;
+	struct gpiochip_info chinfo;
+	struct gpio_v2_line_info linfo;
 	unsigned int offset;
 	int ret;
 
-	for (offset = 0; offset < chip->num_lines; offset++) {
-		ret = chip_read_line_info(chip->fd, offset, &infobuf, false);
+	ret = chip_read_chip_info(chip->fd, &chinfo);
+	if (ret < 0)
+		return -1;
+
+	for (offset = 0; offset < chinfo.lines; offset++) {
+		ret = chip_read_line_info(chip->fd, offset, &linfo, false);
 		if (ret)
 			return -1;
 
-		if (strcmp(name, infobuf.name) == 0)
+		if (strcmp(name, linfo.name) == 0)
 			return offset;
 	}
 
diff --git a/lib/internal.h b/lib/internal.h
index 9af2cda..ffef578 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -18,6 +18,8 @@
 
 #define GPIOD_BIT(nr)	(1UL << (nr))
 
+struct gpiod_chip_info *
+gpiod_chip_info_from_kernel(struct gpiochip_info *infobuf);
 struct gpiod_line_info *
 gpiod_line_info_from_kernel(struct gpio_v2_line_info *infobuf);
 int gpiod_request_config_to_kernel(struct gpiod_request_config *config,
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 89bf868..f37dc03 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -23,6 +23,7 @@ gpiod_test_SOURCES =			\
 		gpiod-test-sim.c	\
 		gpiod-test-sim.h	\
 		tests-chip.c		\
+		tests-chip-info.c	\
 		tests-edge-event.c	\
 		tests-info-event.c	\
 		tests-line-config.c	\
diff --git a/tests/gpiod-test-helpers.h b/tests/gpiod-test-helpers.h
index 4aa4202..acba686 100644
--- a/tests/gpiod-test-helpers.h
+++ b/tests/gpiod-test-helpers.h
@@ -18,6 +18,9 @@
 typedef struct gpiod_chip struct_gpiod_chip;
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(struct_gpiod_chip, gpiod_chip_close);
 
+typedef struct gpiod_chip_info struct_gpiod_chip_info;
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(struct_gpiod_chip_info, gpiod_chip_info_free);
+
 typedef struct gpiod_line_info struct_gpiod_line_info;
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(struct_gpiod_line_info, gpiod_line_info_free);
 
diff --git a/tests/tests-chip-info.c b/tests/tests-chip-info.c
new file mode 100644
index 0000000..bffe823
--- /dev/null
+++ b/tests/tests-chip-info.c
@@ -0,0 +1,54 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// SPDX-FileCopyrightText: 2017-2022 Bartosz Golaszewski <bartekgola@gmail.com>
+
+#include <errno.h>
+#include <glib.h>
+#include <gpiod.h>
+
+#include "gpiod-test.h"
+#include "gpiod-test-helpers.h"
+#include "gpiod-test-sim.h"
+
+#define GPIOD_TEST_GROUP "chip-info"
+
+GPIOD_TEST_CASE(get_chip_info_name)
+{
+	g_autoptr(GPIOSimChip) sim = g_gpiosim_chip_new(NULL);
+	g_autoptr(struct_gpiod_chip) chip = NULL;
+	g_autoptr(struct_gpiod_chip_info) info = NULL;
+
+	chip = gpiod_test_open_chip_or_fail(g_gpiosim_chip_get_dev_path(sim));
+
+	info = gpiod_chip_get_info(chip);
+	g_assert_nonnull(info);
+	g_assert_cmpstr(gpiod_chip_info_get_name(info), ==,
+			g_gpiosim_chip_get_name(sim));
+}
+
+GPIOD_TEST_CASE(get_chip_info_label)
+{
+	g_autoptr(GPIOSimChip) sim = g_gpiosim_chip_new("label", "foobar",
+							NULL);
+	g_autoptr(struct_gpiod_chip) chip = NULL;
+	g_autoptr(struct_gpiod_chip_info) info = NULL;
+
+	chip = gpiod_test_open_chip_or_fail(g_gpiosim_chip_get_dev_path(sim));
+	info = gpiod_chip_get_info(chip);
+	g_assert_nonnull(info);
+
+	g_assert_cmpstr(gpiod_chip_info_get_label(info), ==, "foobar");
+}
+
+GPIOD_TEST_CASE(get_num_lines)
+{
+	g_autoptr(GPIOSimChip) sim = g_gpiosim_chip_new("num-lines", 16, NULL);
+	g_autoptr(struct_gpiod_chip) chip = NULL;
+	g_autoptr(struct_gpiod_chip_info) info = NULL;
+
+	chip = gpiod_test_open_chip_or_fail(g_gpiosim_chip_get_dev_path(sim));
+	info = gpiod_chip_get_info(chip);
+	g_assert_nonnull(info);
+
+	g_assert_cmpuint(gpiod_chip_info_get_num_lines(info), ==, 16);
+}
+
diff --git a/tests/tests-chip.c b/tests/tests-chip.c
index 09906e3..3fad16d 100644
--- a/tests/tests-chip.c
+++ b/tests/tests-chip.c
@@ -47,28 +47,6 @@ GPIOD_TEST_CASE(open_chip_not_a_gpio_device)
 	gpiod_test_expect_errno(ENODEV);
 }
 
-GPIOD_TEST_CASE(get_chip_name)
-{
-	g_autoptr(GPIOSimChip) sim = g_gpiosim_chip_new(NULL);
-	g_autoptr(struct_gpiod_chip) chip = NULL;
-
-	chip = gpiod_test_open_chip_or_fail(g_gpiosim_chip_get_dev_path(sim));
-
-	g_assert_cmpstr(gpiod_chip_get_name(chip), ==,
-			g_gpiosim_chip_get_name(sim));
-}
-
-GPIOD_TEST_CASE(get_chip_label)
-{
-	g_autoptr(GPIOSimChip) sim = g_gpiosim_chip_new("label", "foobar",
-							NULL);
-	g_autoptr(struct_gpiod_chip) chip = NULL;
-
-	chip = gpiod_test_open_chip_or_fail(g_gpiosim_chip_get_dev_path(sim));
-
-	g_assert_cmpstr(gpiod_chip_get_label(chip), ==, "foobar");
-}
-
 GPIOD_TEST_CASE(get_chip_path)
 {
 	g_autoptr(GPIOSimChip) sim = g_gpiosim_chip_new(NULL);
@@ -80,16 +58,6 @@ GPIOD_TEST_CASE(get_chip_path)
 	g_assert_cmpstr(gpiod_chip_get_path(chip), ==, path);
 }
 
-GPIOD_TEST_CASE(get_num_lines)
-{
-	g_autoptr(GPIOSimChip) sim = g_gpiosim_chip_new("num-lines", 16, NULL);
-	g_autoptr(struct_gpiod_chip) chip = NULL;
-
-	chip = gpiod_test_open_chip_or_fail(g_gpiosim_chip_get_dev_path(sim));
-
-	g_assert_cmpuint(gpiod_chip_get_num_lines(chip), ==, 16);
-}
-
 GPIOD_TEST_CASE(get_fd)
 {
 	g_autoptr(GPIOSimChip) sim = g_gpiosim_chip_new(NULL);
diff --git a/tools/gpiodetect.c b/tools/gpiodetect.c
index 6ce3cb8..8f6e8b3 100644
--- a/tools/gpiodetect.c
+++ b/tools/gpiodetect.c
@@ -34,6 +34,7 @@ int main(int argc, char **argv)
 {
 	int optc, opti, num_chips, i;
 	struct gpiod_chip *chip;
+	struct gpiod_chip_info *info;
 	struct dirent **entries;
 
 	for (;;) {
@@ -70,11 +71,17 @@ int main(int argc, char **argv)
 		if (!chip)
 			die_perror("unable to open %s", entries[i]->d_name);
 
+		info = gpiod_chip_get_info(chip);
+		if (!info)
+			die_perror("unable to get info for %s", entries[i]->d_name);
+
+
 		printf("%s [%s] (%zu lines)\n",
-		       gpiod_chip_get_name(chip),
-		       gpiod_chip_get_label(chip),
-		       gpiod_chip_get_num_lines(chip));
+		       gpiod_chip_info_get_name(info),
+		       gpiod_chip_info_get_label(info),
+		       gpiod_chip_info_get_num_lines(info));
 
+		gpiod_chip_info_free(info);
 		gpiod_chip_close(chip);
 		free(entries[i]);
 	}
diff --git a/tools/gpiofind.c b/tools/gpiofind.c
index 910cb8b..36eba86 100644
--- a/tools/gpiofind.c
+++ b/tools/gpiofind.c
@@ -34,6 +34,7 @@ int main(int argc, char **argv)
 {
 	int i, num_chips, optc, opti, offset;
 	struct gpiod_chip *chip;
+	struct gpiod_chip_info *info;
 	struct dirent **entries;
 
 	for (;;) {
@@ -76,8 +77,13 @@ int main(int argc, char **argv)
 
 		offset = gpiod_chip_find_line(chip, argv[0]);
 		if (offset >= 0) {
+			info = gpiod_chip_get_info(chip);
+			if (!info)
+				die_perror("unable to get info for %s", entries[i]->d_name);
+
 			printf("%s %u\n",
-			       gpiod_chip_get_name(chip), offset);
+			       gpiod_chip_info_get_name(info), offset);
+			gpiod_chip_info_free(info);
 			gpiod_chip_close(chip);
 			return EXIT_SUCCESS;
 		}
diff --git a/tools/gpioinfo.c b/tools/gpioinfo.c
index c852b36..fbe2a13 100644
--- a/tools/gpioinfo.c
+++ b/tools/gpioinfo.c
@@ -125,19 +125,24 @@ static PRINTF(3, 4) void prinfo(bool *of,
 static void list_lines(struct gpiod_chip *chip)
 {
 	bool flag_printed, of, active_low;
+	struct gpiod_chip_info *chip_info;
 	struct gpiod_line_info *info;
 	const char *name, *consumer;
-	size_t i, offset;
+	size_t i, offset, num_lines;
 	int direction;
 
+	chip_info = gpiod_chip_get_info(chip);
+	if (!chip_info)
+		die_perror("unable to retrieve the chip info from chip");
+
+	num_lines = gpiod_chip_info_get_num_lines(chip_info);
 	printf("%s - %zu lines:\n",
-	       gpiod_chip_get_name(chip), gpiod_chip_get_num_lines(chip));
+	       gpiod_chip_info_get_name(chip_info), num_lines);
 
-	for (offset = 0; offset < gpiod_chip_get_num_lines(chip); offset++) {
+	for (offset = 0; offset < num_lines; offset++) {
 		info = gpiod_chip_get_line_info(chip, offset);
 		if (!info)
-			die_perror("unable to retrieve the line object from chip");
-
+			die_perror("unable to retrieve the line info from chip");
 		name = gpiod_line_info_get_name(info);
 		consumer = gpiod_line_info_get_consumer(info);
 		direction = gpiod_line_info_get_direction(info);
@@ -184,6 +189,7 @@ static void list_lines(struct gpiod_chip *chip)
 
 		gpiod_line_info_free(info);
 	}
+	gpiod_chip_info_free(chip_info);
 }
 
 int main(int argc, char **argv)
-- 
2.35.1

[libgpiod v2][PATCH 3/3] line-info: rename infobuf to uinfo

[Kent Gibson]

The infobuf variable in gpiod_line_info_from_kernel() refers to the
uAPI version of the info, and the "buf" suffix doesn't really
emphasise that, so rename it to uinfo.

Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
 lib/line-info.c | 38 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/lib/line-info.c b/lib/line-info.c
index fc656f9..168fc86 100644
--- a/lib/line-info.c
+++ b/lib/line-info.c
@@ -106,7 +106,7 @@ gpiod_line_info_get_debounce_period_us(struct gpiod_line_info *info)
 }
 
 struct gpiod_line_info *
-gpiod_line_info_from_kernel(struct gpio_v2_line_info *infobuf)
+gpiod_line_info_from_kernel(struct gpio_v2_line_info *uinfo)
 {
 	struct gpio_v2_line_attribute *attr;
 	struct gpiod_line_info *info;
@@ -118,47 +118,47 @@ gpiod_line_info_from_kernel(struct gpio_v2_line_info *infobuf)
 
 	memset(info, 0, sizeof(*info));
 
-	info->offset = infobuf->offset;
-	strncpy(info->name, infobuf->name, GPIO_MAX_NAME_SIZE);
+	info->offset = uinfo->offset;
+	strncpy(info->name, uinfo->name, GPIO_MAX_NAME_SIZE);
 
-	info->used = !!(infobuf->flags & GPIO_V2_LINE_FLAG_USED);
-	strncpy(info->consumer, infobuf->consumer, GPIO_MAX_NAME_SIZE);
+	info->used = !!(uinfo->flags & GPIO_V2_LINE_FLAG_USED);
+	strncpy(info->consumer, uinfo->consumer, GPIO_MAX_NAME_SIZE);
 
-	if (infobuf->flags & GPIO_V2_LINE_FLAG_OUTPUT)
+	if (uinfo->flags & GPIO_V2_LINE_FLAG_OUTPUT)
 		info->direction = GPIOD_LINE_DIRECTION_OUTPUT;
 	else
 		info->direction = GPIOD_LINE_DIRECTION_INPUT;
 
-	if (infobuf->flags & GPIO_V2_LINE_FLAG_ACTIVE_LOW)
+	if (uinfo->flags & GPIO_V2_LINE_FLAG_ACTIVE_LOW)
 		info->active_low = true;
 
-	if (infobuf->flags & GPIO_V2_LINE_FLAG_BIAS_PULL_UP)
+	if (uinfo->flags & GPIO_V2_LINE_FLAG_BIAS_PULL_UP)
 		info->bias = GPIOD_LINE_BIAS_PULL_UP;
-	else if (infobuf->flags & GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN)
+	else if (uinfo->flags & GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN)
 		info->bias = GPIOD_LINE_BIAS_PULL_DOWN;
-	else if (infobuf->flags & GPIO_V2_LINE_FLAG_BIAS_DISABLED)
+	else if (uinfo->flags & GPIO_V2_LINE_FLAG_BIAS_DISABLED)
 		info->bias = GPIOD_LINE_BIAS_DISABLED;
 	else
 		info->bias = GPIOD_LINE_BIAS_UNKNOWN;
 
-	if (infobuf->flags & GPIO_V2_LINE_FLAG_OPEN_DRAIN)
+	if (uinfo->flags & GPIO_V2_LINE_FLAG_OPEN_DRAIN)
 		info->drive = GPIOD_LINE_DRIVE_OPEN_DRAIN;
-	else if (infobuf->flags & GPIO_V2_LINE_FLAG_OPEN_SOURCE)
+	else if (uinfo->flags & GPIO_V2_LINE_FLAG_OPEN_SOURCE)
 		info->drive = GPIOD_LINE_DRIVE_OPEN_SOURCE;
 	else
 		info->drive = GPIOD_LINE_DRIVE_PUSH_PULL;
 
-	if ((infobuf->flags & GPIO_V2_LINE_FLAG_EDGE_RISING) &&
-	    (infobuf->flags & GPIO_V2_LINE_FLAG_EDGE_FALLING))
+	if ((uinfo->flags & GPIO_V2_LINE_FLAG_EDGE_RISING) &&
+	    (uinfo->flags & GPIO_V2_LINE_FLAG_EDGE_FALLING))
 		info->edge = GPIOD_LINE_EDGE_BOTH;
-	else if (infobuf->flags & GPIO_V2_LINE_FLAG_EDGE_RISING)
+	else if (uinfo->flags & GPIO_V2_LINE_FLAG_EDGE_RISING)
 		info->edge = GPIOD_LINE_EDGE_RISING;
-	else if (infobuf->flags & GPIO_V2_LINE_FLAG_EDGE_FALLING)
+	else if (uinfo->flags & GPIO_V2_LINE_FLAG_EDGE_FALLING)
 		info->edge = GPIOD_LINE_EDGE_FALLING;
 	else
 		info->edge = GPIOD_LINE_EDGE_NONE;
 
-	if (infobuf->flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME)
+	if (uinfo->flags & GPIO_V2_LINE_FLAG_EVENT_CLOCK_REALTIME)
 		info->event_clock = GPIOD_LINE_EVENT_CLOCK_REALTIME;
 	else
 		info->event_clock = GPIOD_LINE_EVENT_CLOCK_MONOTONIC;
@@ -167,8 +167,8 @@ gpiod_line_info_from_kernel(struct gpio_v2_line_info *infobuf)
 	 * We assume that the kernel returns correct configuration and that no
 	 * attributes repeat.
 	 */
-	for (i = 0; i < infobuf->num_attrs; i++) {
-		attr = &infobuf->attrs[i];
+	for (i = 0; i < uinfo->num_attrs; i++) {
+		attr = &uinfo->attrs[i];
 
 		if (attr->id == GPIO_V2_LINE_ATTR_ID_DEBOUNCE) {
 			info->debounced = true;
-- 
2.35.1

Re: [libgpiod v2][PATCH 1/3] core: rename gpiod_chip_info_event_wait and gpiod_chip_info_event_read

[Bartosz Golaszewski]

On Tue, Mar 15, 2022 at 6:32 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> Another couple of functions that don't follow the
> gpiod_<object>_<action>_<object> pattern.
> So rename gpiod_chip_info_event_wait to gpiod_chip_wait_info_event
> and gpiod_chip_info_event_read to gpiod_chip_read_info_event.
>
> Signed-off-by: Kent Gibson <warthog618@gmail.com>

Applied.

Bart

Re: [libgpiod v2][PATCH 2/3] core: split chip_info out of chip

[Bartosz Golaszewski]

On Tue, Mar 15, 2022 at 6:32 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> The core library should be minimalist, only performing the operations
> required, but gpiod_chip_open() always fetches the chip info,
> even though the majority of the time the chip will only be used to
> request a line, so the chip info is not required.
>
> Split the chip_info from the chip, in the same style as line_info, and
> update methods and tools appropriately.
>
> In the rare occasions that the user requires the chip info they can
> request it themselves using gpiod_chip_get_info(), as demonstrated
> in the tool changes.
>
> Signed-off-by: Kent Gibson <warthog618@gmail.com>

Ha! Interesting take. Makes sense. I queued this but tweaked a couple
things (some bad copy-paste and stray newlines).

Bart

Re: [libgpiod v2][PATCH 2/3] core: split chip_info out of chip

[Kent Gibson]

On Tue, Mar 15, 2022 at 12:59:55PM +0100, Bartosz Golaszewski wrote:
> On Tue, Mar 15, 2022 at 6:32 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > The core library should be minimalist, only performing the operations
> > required, but gpiod_chip_open() always fetches the chip info,
> > even though the majority of the time the chip will only be used to
> > request a line, so the chip info is not required.
> >
> > Split the chip_info from the chip, in the same style as line_info, and
> > update methods and tools appropriately.
> >
> > In the rare occasions that the user requires the chip info they can
> > request it themselves using gpiod_chip_get_info(), as demonstrated
> > in the tool changes.
> >
> > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> 
> Ha! Interesting take. Makes sense. I queued this but tweaked a couple
> things (some bad copy-paste and stray newlines).
> 

I've been flip-flopping on this one, but wanted to float it as we're
getting close to finalising the API.
In some ways hiding the chip info is simpler, and gpiod_chip_open()
already does a whole lot in gpiod_is_gpiochip_device(), so an extra
ioctl call isn't going to make much difference...
But it does reduce the gpiod_chip size a fair bit as well.
As I said - flip-flopping.

Cheers,
Kent.

Re: [libgpiod v2][PATCH 3/3] line-info: rename infobuf to uinfo

[Bartosz Golaszewski]

On Tue, Mar 15, 2022 at 6:33 AM Kent Gibson <warthog618@gmail.com> wrote:
>
> The infobuf variable in gpiod_line_info_from_kernel() refers to the
> uAPI version of the info, and the "buf" suffix doesn't really
> emphasise that, so rename it to uinfo.
>
> Signed-off-by: Kent Gibson <warthog618@gmail.com>
> ---

I don't entirely disagree with infobuf not being the best name but
uinfo is even less so. The 'u' prefix doesn't mean anything on its
own. If anything I would think it has something to do with udev,
udisks, upower etc. Or "mili" like useconds. How about info_kernel?
kernel_info? uapi_info?

Bart

Re: [libgpiod v2][PATCH 2/3] core: split chip_info out of chip

[Bartosz Golaszewski]

On Tue, Mar 15, 2022 at 1:12 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Tue, Mar 15, 2022 at 12:59:55PM +0100, Bartosz Golaszewski wrote:
> > On Tue, Mar 15, 2022 at 6:32 AM Kent Gibson <warthog618@gmail.com> wrote:
> > >
> > > The core library should be minimalist, only performing the operations
> > > required, but gpiod_chip_open() always fetches the chip info,
> > > even though the majority of the time the chip will only be used to
> > > request a line, so the chip info is not required.
> > >
> > > Split the chip_info from the chip, in the same style as line_info, and
> > > update methods and tools appropriately.
> > >
> > > In the rare occasions that the user requires the chip info they can
> > > request it themselves using gpiod_chip_get_info(), as demonstrated
> > > in the tool changes.
> > >
> > > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> >
> > Ha! Interesting take. Makes sense. I queued this but tweaked a couple
> > things (some bad copy-paste and stray newlines).
> >
>
> I've been flip-flopping on this one, but wanted to float it as we're
> getting close to finalising the API.
> In some ways hiding the chip info is simpler, and gpiod_chip_open()
> already does a whole lot in gpiod_is_gpiochip_device(), so an extra
> ioctl call isn't going to make much difference...
> But it does reduce the gpiod_chip size a fair bit as well.
> As I said - flip-flopping.
>

No, it's good. I like to idea of limiting the responsiblities of each structure.

Bart

Re: [libgpiod v2][PATCH 3/3] line-info: rename infobuf to uinfo

[Kent Gibson]

On Tue, Mar 15, 2022 at 01:14:33PM +0100, Bartosz Golaszewski wrote:
> On Tue, Mar 15, 2022 at 6:33 AM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > The infobuf variable in gpiod_line_info_from_kernel() refers to the
> > uAPI version of the info, and the "buf" suffix doesn't really
> > emphasise that, so rename it to uinfo.
> >
> > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> > ---
> 
> I don't entirely disagree with infobuf not being the best name but
> uinfo is even less so. The 'u' prefix doesn't mean anything on its
> own. If anything I would think it has something to do with udev,
> udisks, upower etc. Or "mili" like useconds. How about info_kernel?
> kernel_info? uapi_info?
> 

uapi_info works for me.

Cheers,
Kent.