[flashrom-gerrit] Change in flashrom[master]: lspcon_i2c_spi: Extract I2C bus parameter handling

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/52830 )

Change subject: lspcon_i2c_spi: Extract I2C bus parameter handling
......................................................................

lspcon_i2c_spi: Extract I2C bus parameter handling

Introduce the `i2c_open_from_programmer_params` function to avoid having
to duplicate parameter parsing code on all I2C programmers. This also
allows having the same programmer parameters on all I2C programmers.

Change-Id: I006b311c88feea37fe4b217f769b21ca1505def9
Signed-off-by: Angel Pons 
Reviewed-on: https://review.coreboot.org/c/flashrom/+/52830
Tested-by: build bot (Jenkins) 
Reviewed-by: Edward O'Callaghan 
---
M i2c_helper.h
M i2c_helper_linux.c
M lspcon_i2c_spi.c
3 files changed, 57 insertions(+), 61 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Edward O'Callaghan: Looks good to me, approved

diff --git a/i2c_helper.h b/i2c_helper.h
index cab2ce6..709a748 100644
--- a/i2c_helper.h
+++ b/i2c_helper.h
@@ -80,6 +80,15 @@
 int i2c_open_path(const char *path, uint16_t addr, int force);

/**
+ * i2c_open_from_programmer_params: open an I2C device from programmer params
+ *
+ * This function is a wrapper for i2c_open and i2c_open_path that obtains the
+ * I2C device to use from programmer parameters. It is meant to be called
+ * from I2C-based programmers to avoid repeating parameter parsing code.
+ */
+int i2c_open_from_programmer_params(uint16_t addr, int force);
+
+/**
  * i2c_close - closes the file descriptor returned by i2c_open
  *
  * @fd:	file descriptor to be closed.
diff --git a/i2c_helper_linux.c b/i2c_helper_linux.c
index 963c399..15f8f2a 100644
--- a/i2c_helper_linux.c
+++ b/i2c_helper_linux.c
@@ -27,6 +27,7 @@

#include "flash.h"
 #include "i2c_helper.h"
+#include "programmer.h"

/* Null characters are placeholders for bus number digits */
 #define I2C_DEV_PREFIX	"/dev/i2c-\0\0\0"
@@ -76,6 +77,52 @@
    return i2c_open_path(dev, addr, force);
 }

+static int get_bus_number(char *bus_str)
+{
+	char *bus_suffix;
+	errno = 0;
+	int bus = (int)strtol(bus_str, &bus_suffix, 10);
+	if (errno != 0 || bus_str == bus_suffix) {
+		msg_perr("%s: Could not convert 'bus'.\n", __func__);
+		return -1;
+	}
+
+	if (strlen(bus_suffix) > 0) {
+		msg_perr("%s: Garbage following 'bus' value.\n", __func__);
+		return -1;
+	}
+
+	msg_pinfo("Using I2C bus %d.\n", bus);
+	return bus;
+}
+
+int i2c_open_from_programmer_params(uint16_t addr, int force)
+{
+	int fd = -1;
+
+	char *bus_str = extract_programmer_param("bus");
+	char *device_path = extract_programmer_param("devpath");
+
+	if (device_path != NULL && bus_str != NULL) {
+		msg_perr("%s: only one of bus and devpath may be specified\n", __func__);
+		goto out;
+	}
+	if (device_path == NULL && bus_str == NULL) {
+		msg_perr("%s: one of bus and devpath must be specified\n", __func__);
+		goto out;
+	}
+
+	if (device_path != NULL)
+		fd = i2c_open_path(device_path, addr, force);
+	else
+		fd = i2c_open(get_bus_number(bus_str), addr, force);
+
+out:
+	free(bus_str);
+	free(device_path);
+	return fd;
+}
+
 int i2c_read(int fd, uint16_t addr, i2c_buffer_t *buf)
 {
    if (buf->len == 0)
diff --git a/lspcon_i2c_spi.c b/lspcon_i2c_spi.c
index e2c4a3c..096fb76 100644
--- a/lspcon_i2c_spi.c
+++ b/lspcon_i2c_spi.c
@@ -433,69 +433,9 @@
    return ret;
 }

-/* TODO: remove this out of the specific SPI master implementation. */
-static int get_bus_number(void)
-{
-	char *bus_str = extract_programmer_param("bus");
-	/* Return INVALID_ADDRESS if bus value was given but invalid, and GENERIC_ERROR
-	 * if no value was provided. */
-        int ret = SPI_INVALID_ADDRESS;
-
-        if (bus_str == NULL)
-        	return SPI_GENERIC_ERROR;
-
-	char *bus_suffix;
-	errno = 0;
-	int bus = (int)strtol(bus_str, &bus_suffix, 10);
-	if (errno != 0 || bus_str == bus_suffix) {
-		msg_perr("%s: Could not convert 'bus'.\n", __func__);
-		goto get_bus_done;
-	}
-
-	if (bus < 0 || bus > 255) {
-		msg_perr("%s: Value for 'bus' is out of range(0-255).\n", __func__);
-		goto get_bus_done;
-	}
-
-	if (strlen(bus_suffix) > 0) {
-		msg_perr("%s: Garbage following 'bus' value.\n", __func__);
-		goto get_bus_done;
-	}
-
-	msg_pinfo("Using i2c bus %i.\n", bus);
-	ret = bus;
-
-get_bus_done:
-	free(bus_str);
-	return ret;
-}
-
 int lspcon_i2c_spi_init(void)
 {
-	int fd = -1;
-	int bus_number = get_bus_number();
-	if (bus_number == SPI_INVALID_ADDRESS) {
-		/* Bus was specified but unusable, bail out immediately */
-		return SPI_GENERIC_ERROR;
-	}
-	char *device_path = extract_programmer_param("devpath");
-
-	if (device_path != NULL && bus_number >= 0) {
-		msg_perr("%s: only one of bus and devpath may be specified\n", __func__);
-		free(device_path);
-		return SPI_GENERIC_ERROR;
-	} else if (device_path == NULL && bus_number < 0) {
-		msg_perr("%s: one of bus and devpath must be specified\n", __func__);
-		return SPI_GENERIC_ERROR;
-	}
-
-	if (device_path != NULL) {
-		fd = i2c_open_path(device_path, REGISTER_ADDRESS, 0);
-		free(device_path);
-	} else {
-		fd = i2c_open(bus_number, REGISTER_ADDRESS, 0);
-	}
-
+	int fd = i2c_open_from_programmer_params(REGISTER_ADDRESS, 0);
    if (fd < 0)
    	return fd;