[libgpiod PATCH 0/7] tools: Add by-name support

[libgpiod PATCH 0/7] tools: Add by-name support

[Joel Stanley]

This series introduces a command-line flag "--by-name" that means
"line-specifier arguments should be interpreted as names.

The patches implement the change for gpioset and gpioget tools. Once we
have settled on an implementation for those tools it could be extended
to other tools, such as gpiomon.

The test suite has been updated to cover the new option.

An initial discussion of the idea was presented here:

 http://lore.kernel.org/all/20211201072902.127542-1-joel@jms.id.au

Joel Stanley (7):
  tools: Clean up scandir memory allocations
  tools: Add line name to offset lookup helper
  tools: Add value support to line name lookup
  tools: gpioget: Add by-name support
  tools: gpioset: Add by-name support
  gpio-tools-test: Add gpioset --by-name tests
  gpio-tools-test: Add gpioget --by-name tests

 tools/gpio-tools-test.bats | 184 +++++++++++++++++++++++++++++++++++++
 tools/gpioget.c            |  57 ++++++++----
 tools/gpioset.c            |  68 ++++++++++----
 tools/tools-common.c       |  76 ++++++++++++++-
 tools/tools-common.h       |   4 +
 5 files changed, 352 insertions(+), 37 deletions(-)

-- 
2.34.1

[libgpiod PATCH 1/7] tools: Clean up scandir memory allocations

[Joel Stanley]

The code copied from gpiofind didn't free the memory from scandir.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 tools/tools-common.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/tools-common.c b/tools/tools-common.c
index 0dc3d52668d7..c83e68a2c1e4 100644
--- a/tools/tools-common.c
+++ b/tools/tools-common.c
@@ -190,12 +190,17 @@ struct gpiod_chip *chip_by_line_name(const char *name)
 
 			die_perror("unable to open %s", entries[i]->d_name);
 		}
+		free(entries[i]);
 
 		offset = gpiod_chip_find_line(chip, name);
-		if (offset >= 0)
+		if (offset >= 0) {
+			free(entries);
 			return chip;
+		}
+		gpiod_chip_unref(chip);
 	}
 
+	free(entries);
 	return NULL;
 }
 
-- 
2.34.1

[libgpiod PATCH 2/7] tools: Add line name to offset lookup helper

[Joel Stanley]

line_names_to_offsets to be used by tools that support --by-name.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 tools/tools-common.c | 22 ++++++++++++++++++++++
 tools/tools-common.h |  2 ++
 2 files changed, 24 insertions(+)

diff --git a/tools/tools-common.c b/tools/tools-common.c
index c83e68a2c1e4..958933ed6d51 100644
--- a/tools/tools-common.c
+++ b/tools/tools-common.c
@@ -204,3 +204,25 @@ struct gpiod_chip *chip_by_line_name(const char *name)
 	return NULL;
 }
 
+int line_names_to_offsets(struct gpiod_chip *chip, char **lines,
+			  unsigned int *offsets, int num_lines)
+{
+	int i;
+
+	for (i = 0; i < num_lines; i++) {
+		const char *line_name = lines[i];
+		int offset;
+
+		offset = gpiod_chip_find_line(chip, line_name);
+
+		if (offset < 0) {
+			die("chip '%s' does not contain line '%s'",
+					gpiod_chip_get_name(chip),
+					line_name);
+		}
+
+		offsets[i] = offset;
+	}
+
+	return 0;
+}
diff --git a/tools/tools-common.h b/tools/tools-common.h
index 5ed37dc05885..7affea436a60 100644
--- a/tools/tools-common.h
+++ b/tools/tools-common.h
@@ -32,5 +32,7 @@ int chip_dir_filter(const struct dirent *entry);
 struct gpiod_chip *chip_open_by_name(const char *name);
 struct gpiod_chip *chip_open_lookup(const char *device);
 struct gpiod_chip *chip_by_line_name(const char *name);
+int line_names_to_offsets(struct gpiod_chip *chip, char **lines,
+			  unsigned int *offsets, int num_lines);
 
 #endif /* __GPIOD_TOOLS_COMMON_H__ */
-- 
2.34.1

[libgpiod PATCH 3/7] tools: Add value support to line name lookup

[Joel Stanley]

Add support for pasring the values as well as the name in
line_names_to_offsets.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 tools/tools-common.c | 51 ++++++++++++++++++++++++++++++++++++++++++--
 tools/tools-common.h |  4 +++-
 2 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/tools/tools-common.c b/tools/tools-common.c
index 958933ed6d51..586577566790 100644
--- a/tools/tools-common.c
+++ b/tools/tools-common.c
@@ -204,15 +204,57 @@ struct gpiod_chip *chip_by_line_name(const char *name)
 	return NULL;
 }
 
+char *split_line(const char *line_pair)
+{
+	char *name_end;
+	size_t name_len;
+	char *line_name;
+
+	name_end = strchr(line_pair, '=');
+	if (!name_end)
+		die("invalid name/value '%s'", line_pair);
+
+	name_len = name_end - line_pair;
+
+	if (name_len > 32)
+		die("line name exceeds maximum length");
+
+	line_name = calloc(1, name_len + 1);
+	strncpy(line_name, line_pair, name_len);
+
+	return line_name;
+}
+
 int line_names_to_offsets(struct gpiod_chip *chip, char **lines,
-			  unsigned int *offsets, int num_lines)
+			  unsigned int *offsets,
+			  int *values,
+			  int num_lines)
 {
 	int i;
 
 	for (i = 0; i < num_lines; i++) {
-		const char *line_name = lines[i];
+		char *line_name;
+		int value;
 		int offset;
 
+		if (values) {
+			const char *line_pair = lines[i];
+			char *name_end;
+			int rv;
+
+			line_name = split_line(line_pair);
+			name_end = strchr(line_pair, '=');
+
+			rv = sscanf(name_end, "=%d", &value);
+			if (rv != 1)
+				die("invalid offset<->value mapping: %s", line_pair);
+
+			if (value != 0 && value != 1)
+				die("value must be 0 or 1: %s", line_pair);
+		} else {
+			line_name = lines[i];
+		}
+
 		offset = gpiod_chip_find_line(chip, line_name);
 
 		if (offset < 0) {
@@ -222,6 +264,11 @@ int line_names_to_offsets(struct gpiod_chip *chip, char **lines,
 		}
 
 		offsets[i] = offset;
+
+		if (values) {
+			values[i] = value;
+			free(line_name);
+		}
 	}
 
 	return 0;
diff --git a/tools/tools-common.h b/tools/tools-common.h
index 7affea436a60..723999011733 100644
--- a/tools/tools-common.h
+++ b/tools/tools-common.h
@@ -33,6 +33,8 @@ struct gpiod_chip *chip_open_by_name(const char *name);
 struct gpiod_chip *chip_open_lookup(const char *device);
 struct gpiod_chip *chip_by_line_name(const char *name);
 int line_names_to_offsets(struct gpiod_chip *chip, char **lines,
-			  unsigned int *offsets, int num_lines);
+			  unsigned int *offsets, int *values,
+			  int num_lines);
+char *split_line(const char *line_pair);
 
 #endif /* __GPIOD_TOOLS_COMMON_H__ */
-- 
2.34.1

[libgpiod PATCH 4/7] tools: gpioget: Add by-name support

[Joel Stanley]

Allow users to get the values of gpios by passing the gpio name. The
gpipchip is not specified, instead it is discovered using the same
method as gpiofind.

 $ gpioget --by-name switch-state
 1

 $ gpioget --by-name led-fault led-identify led-attention
 1 0 1

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 tools/gpioget.c | 57 +++++++++++++++++++++++++++++++++++--------------
 1 file changed, 41 insertions(+), 16 deletions(-)

diff --git a/tools/gpioget.c b/tools/gpioget.c
index 51cecb6a18a9..9d2c82b0d64b 100644
--- a/tools/gpioget.c
+++ b/tools/gpioget.c
@@ -15,15 +15,18 @@ static const struct option longopts[] = {
 	{ "active-low",	no_argument,		NULL,	'l' },
 	{ "dir-as-is",	no_argument,		NULL,	'n' },
 	{ "bias",	required_argument,	NULL,	'B' },
+	{ "by-name",	no_argument,		NULL,	'N' },
 	{ GETOPT_NULL_LONGOPT },
 };
 
-static const char *const shortopts = "+hvlnB:";
+static const char *const shortopts = "+hvlnB:N";
 
 static void print_help(void)
 {
 	printf("Usage: %s [OPTIONS] <chip name/number> <offset 1> <offset 2> ...\n",
 	       get_progname());
+	printf("       %s [OPTIONS] -L <line name1> <line name2> ...\n",
+	       get_progname());
 	printf("\n");
 	printf("Read line value(s) from a GPIO chip\n");
 	printf("\n");
@@ -34,6 +37,7 @@ static void print_help(void)
 	printf("  -n, --dir-as-is:\tdon't force-reconfigure line direction\n");
 	printf("  -B, --bias=[as-is|disable|pull-down|pull-up] (defaults to 'as-is'):\n");
 	printf("		set the line bias\n");
+	printf("  -N, --by-name:\tget line by name. All lines must be from the same gpiochip\n");
 	printf("\n");
 	print_bias_help();
 }
@@ -46,7 +50,8 @@ int main(int argc, char **argv)
 	unsigned int *offsets, i, num_lines;
 	struct gpiod_line_bulk *lines;
 	struct gpiod_chip *chip;
-	char *device, *end;
+	bool by_name = false;
+	char *end;
 
 	for (;;) {
 		optc = getopt_long(argc, argv, shortopts, longopts, &opti);
@@ -69,6 +74,9 @@ int main(int argc, char **argv)
 		case 'B':
 			flags |= bias_flags(optarg);
 			break;
+		case 'N':
+			by_name = true;
+			break;
 		case '?':
 			die("try %s --help", get_progname());
 		default:
@@ -79,30 +87,47 @@ int main(int argc, char **argv)
 	argc -= optind;
 	argv += optind;
 
-	if (argc < 1)
-		die("gpiochip must be specified");
+	if (by_name) {
+		if (argc < 1)
+			die("at least one line name must be specified");
+
+		/* line0 line1 ... lineN */
+		num_lines = argc;
 
-	if (argc < 2)
-		die("at least one GPIO line offset must be specified");
+		chip = chip_by_line_name(argv[0]);
+		if (!chip)
+			die("unable to find gpiochip");
+	} else {
+		/* gpiochip offset0 offset1 ... offsetN */
+		if (argc < 1)
+			die("gpiochip must be specified");
 
-	device = argv[0];
-	num_lines = argc - 1;
+		if (argc < 2)
+			die("at least one GPIO line offset must be specified");
+
+		chip = chip_open_lookup(argv[0]);
+		if (!chip)
+			die_perror("unable to open %s", argv[0]);
+
+		argv++;
+		num_lines = argc - 1;
+	}
 
 	values = malloc(sizeof(*values) * num_lines);
 	offsets = malloc(sizeof(*offsets) * num_lines);
 	if (!values || !offsets)
 		die("out of memory");
 
-	for (i = 0; i < num_lines; i++) {
-		offsets[i] = strtoul(argv[i + 1], &end, 10);
-		if (*end != '\0' || offsets[i] > INT_MAX)
-			die("invalid GPIO offset: %s", argv[i + 1]);
+	if (by_name) {
+		line_names_to_offsets(chip, argv, offsets, NULL, num_lines);
+	} else {
+		for (i = 0; i < num_lines; i++) {
+			offsets[i] = strtoul(argv[i], &end, 10);
+			if (*end != '\0' || offsets[i] > INT_MAX)
+				die("invalid GPIO offset: %s", argv[i]);
+		}
 	}
 
-	chip = chip_open_lookup(device);
-	if (!chip)
-		die_perror("unable to open %s", device);
-
 	lines = gpiod_chip_get_lines(chip, offsets, num_lines);
 	if (!lines)
 		die_perror("unable to retrieve GPIO lines from chip");
-- 
2.34.1

[libgpiod PATCH 5/7] tools: gpioset: Add by-name support

[Joel Stanley]

Allow users to set the values of gpios by passing the gpio name. The
gpipchip is not specified, instead it is discovered using the same
method as gpiofind.

 $ gpioset  --mode=wait --by-name cfam-reset=1

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 tools/gpioset.c | 68 ++++++++++++++++++++++++++++++++++---------------
 1 file changed, 48 insertions(+), 20 deletions(-)

diff --git a/tools/gpioset.c b/tools/gpioset.c
index 7882b53bab41..fe3e1c246c89 100644
--- a/tools/gpioset.c
+++ b/tools/gpioset.c
@@ -23,15 +23,18 @@ static const struct option longopts[] = {
 	{ "sec",		required_argument,	NULL,	's' },
 	{ "usec",		required_argument,	NULL,	'u' },
 	{ "background",		no_argument,		NULL,	'b' },
+	{ "by-name",		no_argument,		NULL,	'N' },
 	{ GETOPT_NULL_LONGOPT },
 };
 
-static const char *const shortopts = "+hvlB:D:m:s:u:b";
+static const char *const shortopts = "+hvlB:D:m:s:u:bN";
 
 static void print_help(void)
 {
 	printf("Usage: %s [OPTIONS] <chip name/number> <offset1>=<value1> <offset2>=<value2> ...\n",
 	       get_progname());
+	printf("       %s [OPTIONS] -L <line name1> <line name2> ...\n",
+	       get_progname());
 	printf("\n");
 	printf("Set GPIO line values of a GPIO chip and maintain the state until the process exits\n");
 	printf("\n");
@@ -48,6 +51,7 @@ static void print_help(void)
 	printf("  -s, --sec=SEC:\tspecify the number of seconds to wait (only valid for --mode=time)\n");
 	printf("  -u, --usec=USEC:\tspecify the number of microseconds to wait (only valid for --mode=time)\n");
 	printf("  -b, --background:\tafter setting values: detach from the controlling terminal\n");
+	printf("  -N, --by-name:\tset line by name. All lines must be from the same gpiochip\n");
 	printf("\n");
 	print_bias_help();
 	printf("\n");
@@ -195,7 +199,8 @@ int main(int argc, char **argv)
 	struct gpiod_line_bulk *lines;
 	struct callback_data cbdata;
 	struct gpiod_chip *chip;
-	char *device, *end;
+	bool by_name = false;
+	char *end;
 
 	memset(&cbdata, 0, sizeof(cbdata));
 
@@ -239,6 +244,9 @@ int main(int argc, char **argv)
 		case 'b':
 			cbdata.daemonize = true;
 			break;
+		case 'N':
+			by_name = true;
+			break;
 		case '?':
 			die("try %s --help", get_progname());
 		default:
@@ -257,37 +265,57 @@ int main(int argc, char **argv)
 	    cbdata.daemonize)
 		die("can't daemonize in this mode");
 
-	if (argc < 1)
-		die("gpiochip must be specified");
+	if (by_name) {
+		char *line_name;
+
+		if (argc < 1)
+			die("at least one line name must be specified");
+
+		line_name = split_line(argv[0]);
+		chip = chip_by_line_name(line_name);
+		if (!chip)
+			die("unable to find '%s' on a gpiochip", line_name);
+
+		free(line_name);
 
-	if (argc < 2)
-		die("at least one GPIO line offset to value mapping must be specified");
+		num_lines = argc;
+	} else {
+		if (argc < 1)
+			die("gpiochip must be specified");
 
-	device = argv[0];
+		if (argc < 2)
+			die("at least one GPIO line offset to value mapping must be specified");
 
-	num_lines = argc - 1;
+
+		chip = chip_open_lookup(argv[0]);
+		if (!chip)
+			die_perror("unable to open %s", argv[0]);
+
+		num_lines = argc - 1;
+		argv++;
+	}
 
 	offsets = malloc(sizeof(*offsets) * num_lines);
 	values = malloc(sizeof(*values) * num_lines);
 	if (!values || !offsets)
 		die("out of memory");
 
-	for (i = 0; i < num_lines; i++) {
-		rv = sscanf(argv[i + 1], "%u=%d", &offsets[i], &values[i]);
-		if (rv != 2)
-			die("invalid offset<->value mapping: %s", argv[i + 1]);
+	if (by_name) {
+		line_names_to_offsets(chip, argv, offsets, values, num_lines);
+	} else {
+		for (i = 0; i < num_lines; i++) {
+			rv = sscanf(argv[i], "%u=%d", &offsets[i], &values[i]);
+			if (rv != 2)
+				die("invalid offset<->value mapping: %s", argv[i + 1]);
 
-		if (values[i] != 0 && values[i] != 1)
-			die("value must be 0 or 1: %s", argv[i + 1]);
+			if (values[i] != 0 && values[i] != 1)
+				die("value must be 0 or 1: %s", argv[i]);
 
-		if (offsets[i] > INT_MAX)
-			die("invalid offset: %s", argv[i + 1]);
+			if (offsets[i] > INT_MAX)
+				die("invalid offset: %s", argv[i]);
+		}
 	}
 
-	chip = chip_open_lookup(device);
-	if (!chip)
-		die_perror("unable to open %s", device);
-
 	lines = gpiod_chip_get_lines(chip, offsets, num_lines);
 	if (!lines)
 		die_perror("unable to retrieve GPIO lines from chip");
-- 
2.34.1

[libgpiod PATCH 6/7] gpio-tools-test: Add gpioset --by-name tests

[Joel Stanley]

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 tools/gpio-tools-test.bats | 111 +++++++++++++++++++++++++++++++++++++
 1 file changed, 111 insertions(+)

diff --git a/tools/gpio-tools-test.bats b/tools/gpio-tools-test.bats
index a5b97e1f98ee..a90c695fbc0f 100755
--- a/tools/gpio-tools-test.bats
+++ b/tools/gpio-tools-test.bats
@@ -670,6 +670,117 @@ teardown() {
 	output_regex_match ".*unable to request lines.*"
 }
 
+@test "gpioset: invalid value (by name)" {
+	gpio_mockup_probe named-lines 8 8 8
+
+	run_tool gpioset --by-name gpio-mockup-A-0=foobar
+
+	test "$status" -eq "1"
+	output_regex_match ".*invalid offset<->value mapping: gpio-mockup-A-0=foobar.*"
+}
+
+@test "gpioset: invalid line syntax by name, missing =" {
+	gpio_mockup_probe named-lines 8 8 8
+
+	run_tool gpioset --by-name gpio-mockup-A-0foobar
+
+	test "$status" -eq "1"
+	output_regex_match ".*invalid name/value 'gpio-mockup-A-0foobar'*"
+}
+
+@test "gpioset: invalid line syntax by name, leading =" {
+	gpio_mockup_probe named-lines 8 8 8
+
+	run_tool gpioset --by-name =gpio-mockup-A-0foobar
+
+	test "$status" -eq "1"
+	output_regex_match ".*unable to find '' on a gpiochip*"
+}
+
+@test "gpioset: missing value (by name)" {
+	gpio_mockup_probe named-lines 8 8 8
+
+	run_tool gpioset --by-name gpio-mockup-A-0=
+
+	test "$status" -eq "1"
+	output_regex_match ".*invalid offset<->value mapping: gpio-mockup-A-0=*"
+}
+
+@test "gpioset: invalid line name (from different gpiochip) " {
+	gpio_mockup_probe named-lines 8 8 8
+
+	run_tool gpioset --by-name \
+				gpio-mockup-A-0=1 \
+				gpio-mockup-A-2=0 \
+				gpio-mockup-B-3=1
+
+	test "$status" -eq "1"
+	output_regex_match ".*does not contain line 'gpio-mockup-B-3'.*"
+}
+
+@test "gpiogst: invalid line name (non existant line on a chip) " {
+	gpio_mockup_probe named-lines 8 8 8
+
+	run_tool gpioset --by-name \
+				gpio-mockup-A-0=1 \
+				missing=0
+
+	test "$status" -eq "1"
+	output_regex_match ".*does not contain line 'missing'.*"
+}
+
+@test "gpioset: invalid line name (non existant line, no chip) " {
+	gpio_mockup_probe named-lines 8 8 8
+
+	run_tool gpioset --by-name missing=1
+
+	test "$status" -eq "1"
+	output_regex_match ".*unable to find 'missing' on a gpiochip*"
+}
+
+@test "gpioset: set lines by name and wait for SIGTERM" {
+	gpio_mockup_probe named-lines 2 2 8
+
+	coproc_run_tool gpioset --mode=signal --by-name \
+						gpio-mockup-C-3=1 \
+						gpio-mockup-C-6=1 \
+						gpio-mockup-C-7=0
+
+	gpio_mockup_check_value 2 3 1
+	gpio_mockup_check_value 2 6 1
+	gpio_mockup_check_value 2 7 0
+
+	coproc_tool_kill
+	coproc_tool_wait
+
+	test "$status" -eq "0"
+}
+
+@test "gpioset: set lines by name using short option and wait for SIGTERM" {
+	gpio_mockup_probe named-lines 2 2 8
+
+	coproc_run_tool gpioset --mode=signal -N \
+						gpio-mockup-C-3=1 \
+						gpio-mockup-C-6=1 \
+						gpio-mockup-C-7=0
+
+	gpio_mockup_check_value 2 3 1
+	gpio_mockup_check_value 2 6 1
+	gpio_mockup_check_value 2 7 0
+
+	coproc_tool_kill
+	coproc_tool_wait
+
+	test "$status" -eq "0"
+}
+
+@test "gpioset: no arguments (by name)" {
+	run_tool gpioset --by-name
+
+	test "$status" -eq "1"
+	output_regex_match ".*at least one line name must be specified"
+}
+
 #
 # gpiomon test cases
 #
-- 
2.34.1

[libgpiod PATCH 7/7] gpio-tools-test: Add gpioget --by-name tests

[Joel Stanley]

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 tools/gpio-tools-test.bats | 73 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/tools/gpio-tools-test.bats b/tools/gpio-tools-test.bats
index a90c695fbc0f..068ade357a35 100755
--- a/tools/gpio-tools-test.bats
+++ b/tools/gpio-tools-test.bats
@@ -391,6 +391,79 @@ teardown() {
 	output_regex_match ".*invalid bias.*"
 }
 
+@test "gpioget: invalid line name (from different gpiochip) " {
+	gpio_mockup_probe named-lines 8 8 8
+
+	run_tool gpioget --by-name \
+				gpio-mockup-A-0 \
+				gpio-mockup-A-2 \
+				gpio-mockup-B-3
+
+	test "$status" -eq "1"
+	output_regex_match ".*does not contain line 'gpio-mockup-B-3'.*"
+}
+
+@test "gpioget: invalid line name (non existant line on a chip) " {
+	gpio_mockup_probe named-lines 8 8 8
+
+	run_tool gpioget --by-name \
+				gpio-mockup-A-0 \
+				missing
+
+	test "$status" -eq "1"
+	output_regex_match ".*does not contain line 'missing'.*"
+}
+
+@test "gpioget: invalid line name (non existant line, no chip) " {
+	gpio_mockup_probe named-lines 8 8 8
+
+	run_tool gpioget --by-name missing
+
+	test "$status" -eq "1"
+	output_regex_match ".*unable to find gpiochip.*"
+}
+
+@test "gpioget: read some lines by name" {
+	gpio_mockup_probe named-lines 8 8 8
+
+	gpio_mockup_set_pull 1 1 1
+	gpio_mockup_set_pull 1 4 1
+	gpio_mockup_set_pull 1 6 1
+
+	run_tool gpioget --by-name \
+				gpio-mockup-B-0 \
+				gpio-mockup-B-1 \
+				gpio-mockup-B-4 \
+				gpio-mockup-B-6
+
+	test "$status" -eq "0"
+	test "$output" = "0 1 1 1"
+}
+
+@test "gpioget: no arguments (by name)" {
+	run_tool gpioget --by-name
+
+	test "$status" -eq "1"
+	output_regex_match ".*at least one line name must be specified"
+}
+
+@test "gpioget: read some lines by name using short option" {
+	gpio_mockup_probe named-lines 8 8 8
+
+	gpio_mockup_set_pull 1 1 1
+	gpio_mockup_set_pull 1 4 1
+	gpio_mockup_set_pull 1 6 1
+
+	run_tool gpioget -N \
+				gpio-mockup-B-0 \
+				gpio-mockup-B-1 \
+				gpio-mockup-B-4 \
+				gpio-mockup-B-6
+
+	test "$status" -eq "0"
+	test "$output" = "0 1 1 1"
+}
+
 #
 # gpioset test cases
 #
-- 
2.34.1

Re: [libgpiod PATCH 3/7] tools: Add value support to line name lookup

[Zev Weiss]

On Wed, Feb 02, 2022 at 08:21:30PM PST, Joel Stanley wrote:
>Add support for pasring the values as well as the name in

Nit: typo ("pasring")

>line_names_to_offsets.
>
>Signed-off-by: Joel Stanley <joel@jms.id.au>
>---
> tools/tools-common.c | 51 ++++++++++++++++++++++++++++++++++++++++++--
> tools/tools-common.h |  4 +++-
> 2 files changed, 52 insertions(+), 3 deletions(-)
>
>diff --git a/tools/tools-common.c b/tools/tools-common.c
>index 958933ed6d51..586577566790 100644
>--- a/tools/tools-common.c
>+++ b/tools/tools-common.c
>@@ -204,15 +204,57 @@ struct gpiod_chip *chip_by_line_name(const char *name)
> 	return NULL;
> }
>
>+char *split_line(const char *line_pair)
>+{
>+	char *name_end;
>+	size_t name_len;
>+	char *line_name;
>+
>+	name_end = strchr(line_pair, '=');
>+	if (!name_end)
>+		die("invalid name/value '%s'", line_pair);
>+
>+	name_len = name_end - line_pair;
>+
>+	if (name_len > 32)
>+		die("line name exceeds maximum length");

For mult-line invocations it might be nice to give some feedback on
which line name we errored out on here; perhaps

  die("%s: line name exceeds maximum length", line_pair);

or move the check after the strncpy() below and use line_name if we want
a slightly tidier message without the trailing "=<value>"?

>+
>+	line_name = calloc(1, name_len + 1);
>+	strncpy(line_name, line_pair, name_len);
>+
>+	return line_name;
>+}
>+
> int line_names_to_offsets(struct gpiod_chip *chip, char **lines,
>-			  unsigned int *offsets, int num_lines)
>+			  unsigned int *offsets,
>+			  int *values,
>+			  int num_lines)
> {
> 	int i;
>
> 	for (i = 0; i < num_lines; i++) {
>-		const char *line_name = lines[i];
>+		char *line_name;
>+		int value;
> 		int offset;
>
>+		if (values) {
>+			const char *line_pair = lines[i];
>+			char *name_end;
>+			int rv;
>+
>+			line_name = split_line(line_pair);
>+			name_end = strchr(line_pair, '=');
>+
>+			rv = sscanf(name_end, "=%d", &value);
>+			if (rv != 1)
>+				die("invalid offset<->value mapping: %s", line_pair);
>+
>+			if (value != 0 && value != 1)
>+				die("value must be 0 or 1: %s", line_pair);
>+		} else {
>+			line_name = lines[i];
>+		}
>+
> 		offset = gpiod_chip_find_line(chip, line_name);
>
> 		if (offset < 0) {
>@@ -222,6 +264,11 @@ int line_names_to_offsets(struct gpiod_chip *chip, char **lines,
> 		}
>
> 		offsets[i] = offset;
>+
>+		if (values) {
>+			values[i] = value;
>+			free(line_name);
>+		}
> 	}
>
> 	return 0;
>diff --git a/tools/tools-common.h b/tools/tools-common.h
>index 7affea436a60..723999011733 100644
>--- a/tools/tools-common.h
>+++ b/tools/tools-common.h
>@@ -33,6 +33,8 @@ struct gpiod_chip *chip_open_by_name(const char *name);
> struct gpiod_chip *chip_open_lookup(const char *device);
> struct gpiod_chip *chip_by_line_name(const char *name);
> int line_names_to_offsets(struct gpiod_chip *chip, char **lines,
>-			  unsigned int *offsets, int num_lines);
>+			  unsigned int *offsets, int *values,
>+			  int num_lines);
>+char *split_line(const char *line_pair);
>
> #endif /* __GPIOD_TOOLS_COMMON_H__ */
>-- 
>2.34.1
>

Re: [libgpiod PATCH 4/7] tools: gpioget: Add by-name support

[Zev Weiss]

On Wed, Feb 02, 2022 at 08:21:31PM PST, Joel Stanley wrote:
>Allow users to get the values of gpios by passing the gpio name. The
>gpipchip is not specified, instead it is discovered using the same
>method as gpiofind.
>
> $ gpioget --by-name switch-state
> 1
>
> $ gpioget --by-name led-fault led-identify led-attention
> 1 0 1
>
>Signed-off-by: Joel Stanley <joel@jms.id.au>
>---
> tools/gpioget.c | 57 +++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 41 insertions(+), 16 deletions(-)
>
>diff --git a/tools/gpioget.c b/tools/gpioget.c
>index 51cecb6a18a9..9d2c82b0d64b 100644
>--- a/tools/gpioget.c
>+++ b/tools/gpioget.c
>@@ -15,15 +15,18 @@ static const struct option longopts[] = {
> 	{ "active-low",	no_argument,		NULL,	'l' },
> 	{ "dir-as-is",	no_argument,		NULL,	'n' },
> 	{ "bias",	required_argument,	NULL,	'B' },
>+	{ "by-name",	no_argument,		NULL,	'N' },
> 	{ GETOPT_NULL_LONGOPT },
> };
>
>-static const char *const shortopts = "+hvlnB:";
>+static const char *const shortopts = "+hvlnB:N";
>
> static void print_help(void)
> {
> 	printf("Usage: %s [OPTIONS] <chip name/number> <offset 1> <offset 2> ...\n",
> 	       get_progname());
>+	printf("       %s [OPTIONS] -L <line name1> <line name2> ...\n",

I'm guessing this -L was supposed to be -N?

>+	       get_progname());
> 	printf("\n");
> 	printf("Read line value(s) from a GPIO chip\n");
> 	printf("\n");
>@@ -34,6 +37,7 @@ static void print_help(void)
> 	printf("  -n, --dir-as-is:\tdon't force-reconfigure line direction\n");
> 	printf("  -B, --bias=[as-is|disable|pull-down|pull-up] (defaults to 'as-is'):\n");
> 	printf("		set the line bias\n");
>+	printf("  -N, --by-name:\tget line by name. All lines must be from the same gpiochip\n");
> 	printf("\n");
> 	print_bias_help();
> }
>@@ -46,7 +50,8 @@ int main(int argc, char **argv)
> 	unsigned int *offsets, i, num_lines;
> 	struct gpiod_line_bulk *lines;
> 	struct gpiod_chip *chip;
>-	char *device, *end;
>+	bool by_name = false;
>+	char *end;
>
> 	for (;;) {
> 		optc = getopt_long(argc, argv, shortopts, longopts, &opti);
>@@ -69,6 +74,9 @@ int main(int argc, char **argv)
> 		case 'B':
> 			flags |= bias_flags(optarg);
> 			break;
>+		case 'N':
>+			by_name = true;
>+			break;
> 		case '?':
> 			die("try %s --help", get_progname());
> 		default:
>@@ -79,30 +87,47 @@ int main(int argc, char **argv)
> 	argc -= optind;
> 	argv += optind;
>
>-	if (argc < 1)
>-		die("gpiochip must be specified");
>+	if (by_name) {
>+		if (argc < 1)
>+			die("at least one line name must be specified");
>+
>+		/* line0 line1 ... lineN */
>+		num_lines = argc;
>
>-	if (argc < 2)
>-		die("at least one GPIO line offset must be specified");
>+		chip = chip_by_line_name(argv[0]);
>+		if (!chip)
>+			die("unable to find gpiochip");

The next patch has a slightly more informative corresponding error
message for gpioset; might be nice to make this match that.

>+	} else {
>+		/* gpiochip offset0 offset1 ... offsetN */
>+		if (argc < 1)
>+			die("gpiochip must be specified");
>
>-	device = argv[0];
>-	num_lines = argc - 1;
>+		if (argc < 2)
>+			die("at least one GPIO line offset must be specified");
>+
>+		chip = chip_open_lookup(argv[0]);
>+		if (!chip)
>+			die_perror("unable to open %s", argv[0]);
>+
>+		argv++;
>+		num_lines = argc - 1;
>+	}
>
> 	values = malloc(sizeof(*values) * num_lines);
> 	offsets = malloc(sizeof(*offsets) * num_lines);
> 	if (!values || !offsets)
> 		die("out of memory");
>
>-	for (i = 0; i < num_lines; i++) {
>-		offsets[i] = strtoul(argv[i + 1], &end, 10);
>-		if (*end != '\0' || offsets[i] > INT_MAX)
>-			die("invalid GPIO offset: %s", argv[i + 1]);
>+	if (by_name) {
>+		line_names_to_offsets(chip, argv, offsets, NULL, num_lines);
>+	} else {
>+		for (i = 0; i < num_lines; i++) {
>+			offsets[i] = strtoul(argv[i], &end, 10);
>+			if (*end != '\0' || offsets[i] > INT_MAX)
>+				die("invalid GPIO offset: %s", argv[i]);
>+		}
> 	}
>
>-	chip = chip_open_lookup(device);
>-	if (!chip)
>-		die_perror("unable to open %s", device);
>-
> 	lines = gpiod_chip_get_lines(chip, offsets, num_lines);
> 	if (!lines)
> 		die_perror("unable to retrieve GPIO lines from chip");
>-- 
>2.34.1
>

Re: [libgpiod PATCH 5/7] tools: gpioset: Add by-name support

[Zev Weiss]

On Wed, Feb 02, 2022 at 08:21:32PM PST, Joel Stanley wrote:
>Allow users to set the values of gpios by passing the gpio name. The
>gpipchip is not specified, instead it is discovered using the same
>method as gpiofind.
>
> $ gpioset  --mode=wait --by-name cfam-reset=1
>
>Signed-off-by: Joel Stanley <joel@jms.id.au>
>---
> tools/gpioset.c | 68 ++++++++++++++++++++++++++++++++++---------------
> 1 file changed, 48 insertions(+), 20 deletions(-)
>
>diff --git a/tools/gpioset.c b/tools/gpioset.c
>index 7882b53bab41..fe3e1c246c89 100644
>--- a/tools/gpioset.c
>+++ b/tools/gpioset.c
>@@ -23,15 +23,18 @@ static const struct option longopts[] = {
> 	{ "sec",		required_argument,	NULL,	's' },
> 	{ "usec",		required_argument,	NULL,	'u' },
> 	{ "background",		no_argument,		NULL,	'b' },
>+	{ "by-name",		no_argument,		NULL,	'N' },
> 	{ GETOPT_NULL_LONGOPT },
> };
>
>-static const char *const shortopts = "+hvlB:D:m:s:u:b";
>+static const char *const shortopts = "+hvlB:D:m:s:u:bN";
>
> static void print_help(void)
> {
> 	printf("Usage: %s [OPTIONS] <chip name/number> <offset1>=<value1> <offset2>=<value2> ...\n",
> 	       get_progname());
>+	printf("       %s [OPTIONS] -L <line name1> <line name2> ...\n",

s/-L/-N/ here again, I think...

>+	       get_progname());
> 	printf("\n");
> 	printf("Set GPIO line values of a GPIO chip and maintain the state until the process exits\n");
> 	printf("\n");
>@@ -48,6 +51,7 @@ static void print_help(void)
> 	printf("  -s, --sec=SEC:\tspecify the number of seconds to wait (only valid for --mode=time)\n");
> 	printf("  -u, --usec=USEC:\tspecify the number of microseconds to wait (only valid for --mode=time)\n");
> 	printf("  -b, --background:\tafter setting values: detach from the controlling terminal\n");
>+	printf("  -N, --by-name:\tset line by name. All lines must be from the same gpiochip\n");
> 	printf("\n");
> 	print_bias_help();
> 	printf("\n");
>@@ -195,7 +199,8 @@ int main(int argc, char **argv)
> 	struct gpiod_line_bulk *lines;
> 	struct callback_data cbdata;
> 	struct gpiod_chip *chip;
>-	char *device, *end;
>+	bool by_name = false;
>+	char *end;
>
> 	memset(&cbdata, 0, sizeof(cbdata));
>
>@@ -239,6 +244,9 @@ int main(int argc, char **argv)
> 		case 'b':
> 			cbdata.daemonize = true;
> 			break;
>+		case 'N':
>+			by_name = true;
>+			break;
> 		case '?':
> 			die("try %s --help", get_progname());
> 		default:
>@@ -257,37 +265,57 @@ int main(int argc, char **argv)
> 	    cbdata.daemonize)
> 		die("can't daemonize in this mode");
>
>-	if (argc < 1)
>-		die("gpiochip must be specified");
>+	if (by_name) {
>+		char *line_name;
>+
>+		if (argc < 1)
>+			die("at least one line name must be specified");
>+
>+		line_name = split_line(argv[0]);
>+		chip = chip_by_line_name(line_name);
>+		if (!chip)
>+			die("unable to find '%s' on a gpiochip", line_name);
>+
>+		free(line_name);
>
>-	if (argc < 2)
>-		die("at least one GPIO line offset to value mapping must be specified");
>+		num_lines = argc;
>+	} else {
>+		if (argc < 1)
>+			die("gpiochip must be specified");
>
>-	device = argv[0];
>+		if (argc < 2)
>+			die("at least one GPIO line offset to value mapping must be specified");
>
>-	num_lines = argc - 1;
>+
>+		chip = chip_open_lookup(argv[0]);
>+		if (!chip)
>+			die_perror("unable to open %s", argv[0]);
>+
>+		num_lines = argc - 1;
>+		argv++;
>+	}
>
> 	offsets = malloc(sizeof(*offsets) * num_lines);
> 	values = malloc(sizeof(*values) * num_lines);
> 	if (!values || !offsets)
> 		die("out of memory");
>
>-	for (i = 0; i < num_lines; i++) {
>-		rv = sscanf(argv[i + 1], "%u=%d", &offsets[i], &values[i]);
>-		if (rv != 2)
>-			die("invalid offset<->value mapping: %s", argv[i + 1]);
>+	if (by_name) {
>+		line_names_to_offsets(chip, argv, offsets, values, num_lines);
>+	} else {
>+		for (i = 0; i < num_lines; i++) {
>+			rv = sscanf(argv[i], "%u=%d", &offsets[i], &values[i]);
>+			if (rv != 2)
>+				die("invalid offset<->value mapping: %s", argv[i + 1]);
>
>-		if (values[i] != 0 && values[i] != 1)
>-			die("value must be 0 or 1: %s", argv[i + 1]);
>+			if (values[i] != 0 && values[i] != 1)
>+				die("value must be 0 or 1: %s", argv[i]);
>
>-		if (offsets[i] > INT_MAX)
>-			die("invalid offset: %s", argv[i + 1]);
>+			if (offsets[i] > INT_MAX)
>+				die("invalid offset: %s", argv[i]);
>+		}
> 	}
>
>-	chip = chip_open_lookup(device);
>-	if (!chip)
>-		die_perror("unable to open %s", device);
>-
> 	lines = gpiod_chip_get_lines(chip, offsets, num_lines);
> 	if (!lines)
> 		die_perror("unable to retrieve GPIO lines from chip");
>-- 
>2.34.1
>

Re: [libgpiod PATCH 1/7] tools: Clean up scandir memory allocations

[Bartosz Golaszewski]

On Thu, Feb 3, 2022 at 5:22 AM Joel Stanley <joel@jms.id.au> wrote:
>
> The code copied from gpiofind didn't free the memory from scandir.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  tools/tools-common.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/tools/tools-common.c b/tools/tools-common.c
> index 0dc3d52668d7..c83e68a2c1e4 100644
> --- a/tools/tools-common.c
> +++ b/tools/tools-common.c
> @@ -190,12 +190,17 @@ struct gpiod_chip *chip_by_line_name(const char *name)
>
>                         die_perror("unable to open %s", entries[i]->d_name);
>                 }
> +               free(entries[i]);
>
>                 offset = gpiod_chip_find_line(chip, name);
> -               if (offset >= 0)
> +               if (offset >= 0) {
> +                       free(entries);
>                         return chip;
> +               }
> +               gpiod_chip_unref(chip);
>         }
>
> +       free(entries);
>         return NULL;
>  }
>
> --
> 2.34.1
>

Good catch! It doesn't come up with gpiodetect which is what I tested
with valgrind but does show up for gpiofind and gpioinfo. This patch
however doesn't apply on the current master. Also: I don't really want
new features for the v1.6.x series and we're heavily reworking the
interface. Current version is in the next/libgpiod-2.0 branch but I
should be posting a new version with reworked test suite later this
week. How about working on this feature for the new API right away and
make it part of libgpiod starting with v2.0?

Bart