May 2021 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: linux_mtd: move global state into programmer data field
by Anastasia Klimchuk (Code Review) 09 May '21

09 May '21

Attention is currently required from: Nikolai Artemiev. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/53947 ) Change subject: linux_mtd: move global state into programmer data field ...................................................................... Patch Set 3: Code-Review+1 (4 comments) Patchset: PS3: Thank you for doing this work, makes me happy :) File linux_mtd.c: https://review.coreboot.org/c/flashrom/+/53947/comment/11db57db_f5399676 PS3, Line 365: mtd_data->dev_fp = NULL; Is this line needed? Data is freed below anyway and won't be reused. If init is called again it creates new data. My understanding that was needed before when dev_fp was global var and it could be reused again - but now it's not global anymore. https://review.coreboot.org/c/flashrom/+/53947/comment/1449c9c7_493348a1 PS3, Line 421: programmer_linux_mtd.data = data; Just to double-check, data has 6 members and none of them is initialised in init function, is this WAI? https://review.coreboot.org/c/flashrom/+/53947/comment/e5ea1d50_a0f03d35 PS3, Line 431: free(param); Looking at the code, there is no reason to keep param until the very end, it can be freed just before data calloc, right? And then all `goto linux_mtd_init_exit` can become `return 1`s? This needs to be a separate patch of course. Thank you! -- To view, visit https://review.coreboot.org/c/flashrom/+/53947 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I5ce6900e4892ed5687cfddb245dfe5461a3e2e84 Gerrit-Change-Number: 53947 Gerrit-PatchSet: 3 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Sun, 09 May 2021 23:08:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashchips.c: add support for W25Q32JW..IM
by Nikolai Artemiev (Code Review) 09 May '21

09 May '21

Attention is currently required from: Edward O'Callaghan, Angel Pons. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/53946 ) Change subject: flashchips.c: add support for W25Q32JW..IM ...................................................................... Patch Set 4: (4 comments) File flashchips.c: https://review.coreboot.org/c/flashrom/+/53946/comment/c9203535_23293a8c PS1, Line 17583: ..IM > For consistency with W25Q64JW, I'd drop the trailing `.. […] This only works with `..IM` chips; there are other variants such as `W25Q32JW..IQ` that have a different device ID. We should probably change `W25Q64JW` to `W25Q64JW..IM` though. https://review.coreboot.org/c/flashrom/+/53946/comment/d59fb65f_9c64e946 PS1, Line 17589: FEATURE_WRSR_WREN > Datasheet says that OTP is supported Done https://review.coreboot.org/c/flashrom/+/53946/comment/828be866_172c078e PS1, Line 17611: }, > Add: […] Done https://review.coreboot.org/c/flashrom/+/53946/comment/b0d5edb3_5f47a4f4 PS1, Line 17612: spi_disable_blockprotect > spi_disable_blockprotect_bp2_srwd seems more accurate. Not perfect, but better. Done -- To view, visit https://review.coreboot.org/c/flashrom/+/53946 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7425e12658dd69c4ec8d3309dd591d09a935bb4d Gerrit-Change-Number: 53946 Gerrit-PatchSet: 4 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Sun, 09 May 2021 23:06:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashchips.c: add support for W25Q32JW..IM
by Nikolai Artemiev (Code Review) 09 May '21

09 May '21

Attention is currently required from: Edward O'Callaghan, Nikolai Artemiev. Hello build bot (Jenkins), Edward O'Callaghan, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/53946 to look at the new patch set (#4). Change subject: flashchips.c: add support for W25Q32JW..IM ...................................................................... flashchips.c: add support for W25Q32JW..IM The chip was added to cros flashrom in `commit 1fc77dd1ee27a5d6e58a82c6ed6ed390a15372d7`. Quoting from the commit message: > We have varied the correct chip name is reported as well as > write and read 16MBytes of random data and verified the checksum's match. > Further, --wp-list appears to report the correct ranges. > > BUG=b:130199963 > BRANCH=none > TEST=Ran flashrom with a Dediprog SF100, RW random data and checksum matched. Change-Id: I7425e12658dd69c4ec8d3309dd591d09a935bb4d Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M flashchips.c M flashchips.h 2 files changed, 39 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/46/53946/4 -- To view, visit https://review.coreboot.org/c/flashrom/+/53946 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7425e12658dd69c4ec8d3309dd591d09a935bb4d Gerrit-Change-Number: 53946 Gerrit-PatchSet: 4 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: flashchips.c: add support for W25Q32JW..IM
by Nikolai Artemiev (Code Review) 09 May '21

09 May '21

Attention is currently required from: Edward O'Callaghan, Nikolai Artemiev. Hello build bot (Jenkins), Edward O'Callaghan, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/53946 to look at the new patch set (#3). Change subject: flashchips.c: add support for W25Q32JW..IM ...................................................................... flashchips.c: add support for W25Q32JW..IM The chip was added to cros flashrom in `commit 1fc77dd1ee27a5d6e58a82c6ed6ed390a15372d7`. Quoting from the commit message: > We have varied the correct chip name is reported as well as > write and read 16MBytes of random data and verified the checksum's match. > Further, --wp-list appears to report the correct ranges. > > BUG=b:130199963 > BRANCH=none > TEST=Ran flashrom with a Dediprog SF100, RW random data and checksum matched. Change-Id: I7425e12658dd69c4ec8d3309dd591d09a935bb4d Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M flashchips.c M flashchips.h 2 files changed, 39 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/46/53946/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/53946 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7425e12658dd69c4ec8d3309dd591d09a935bb4d Gerrit-Change-Number: 53946 Gerrit-PatchSet: 3 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: Makefile: Only enable I2C programmers on Linux
by Angel Pons (Code Review) 09 May '21

09 May '21

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/53999 ) Change subject: Makefile: Only enable I2C programmers on Linux ...................................................................... Makefile: Only enable I2C programmers on Linux The I2C support code they use is Linux-specific. Change-Id: I04d25ff8f8c3097428ac8695669b1757c38f49e9 Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M Makefile 1 file changed, 10 insertions(+), 30 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/99/53999/1 diff --git a/Makefile b/Makefile index dd19e14..b8b40ca 100644 --- a/Makefile +++ b/Makefile @@ -212,16 +212,6 @@ else override CONFIG_STLINKV3_SPI = no endif -ifeq ($(CONFIG_LSPCON_I2C_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_LSPCON_I2C_SPI=yes -else -override CONFIG_LSPCON_I2C_SPI = no -endif -ifeq ($(CONFIG_REALTEK_MST_I2C_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_REALTEK_MST_I2C_SPI=yes -else -override CONFIG_REALTEK_MST_I2C_SPI = no -endif # libjaylink is also not available for DOS ifeq ($(CONFIG_JLINK_SPI), yes) UNSUPPORTED_FEATURES += CONFIG_JLINK_SPI=yes @@ -338,16 +328,6 @@ else override CONFIG_SATAMV = no endif -ifeq ($(CONFIG_LSPCON_I2C_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_LSPCON_I2C_SPI=yes -else -override CONFIG_LSPCON_I2C_SPI = no -endif -ifeq ($(CONFIG_REALTEK_MST_I2C_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_REALTEK_MST_I2C_SPI=yes -else -override CONFIG_REALTEK_MST_I2C_SPI = no -endif endif ifneq ($(TARGET_OS), MinGW) @@ -433,16 +413,6 @@ else override CONFIG_STLINKV3_SPI = no endif -ifeq ($(CONFIG_LSPCON_I2C_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_LSPCON_I2C_SPI=yes -else -override CONFIG_LSPCON_I2C_SPI = no -endif -ifeq ($(CONFIG_REALTEK_MST_I2C_SPI), yes) -UNSUPPORTED_FEATURES += CONFIG_REALTEK_MST_I2C_SPI=yes -else -override CONFIG_REALTEK_MST_I2C_SPI = no -endif ifeq ($(CONFIG_CH341A_SPI), yes) UNSUPPORTED_FEATURES += CONFIG_CH341A_SPI=yes else @@ -467,6 +437,16 @@ UNSUPPORTED_FEATURES += CONFIG_MSTARDDC_SPI=yes else override CONFIG_MSTARDDC_SPI = no +ifeq ($(CONFIG_LSPCON_I2C_SPI), yes) +UNSUPPORTED_FEATURES += CONFIG_LSPCON_I2C_SPI=yes +else +override CONFIG_LSPCON_I2C_SPI = no +endif +ifeq ($(CONFIG_REALTEK_MST_I2C_SPI), yes) +UNSUPPORTED_FEATURES += CONFIG_REALTEK_MST_I2C_SPI=yes +else +override CONFIG_REALTEK_MST_I2C_SPI = no +endif endif endif endif -- To view, visit https://review.coreboot.org/c/flashrom/+/53999 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I04d25ff8f8c3097428ac8695669b1757c38f49e9 Gerrit-Change-Number: 53999 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: realtek_mst_i2c_spi.c: Rename `PAGE_SIZE` macro
by Angel Pons (Code Review) 09 May '21

09 May '21

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/53998 ) Change subject: realtek_mst_i2c_spi.c: Rename `PAGE_SIZE` macro ...................................................................... realtek_mst_i2c_spi.c: Rename `PAGE_SIZE` macro This fixes building with musl libc on alpine:amd64-v3.9. Change-Id: I043e3d8c2d2498e94b5e7577a7378c8c3e0e6c81 Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> --- M realtek_mst_i2c_spi.c 1 file changed, 9 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/98/53998/1 diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c index 64579e1..1b8ef72 100644 --- a/realtek_mst_i2c_spi.c +++ b/realtek_mst_i2c_spi.c @@ -28,7 +28,7 @@ #define MCU_I2C_SLAVE_ADDR 0x94 #define REGISTER_ADDRESS (0x94 >> 1) -#define PAGE_SIZE 128 +#define REALTEK_PAGE_SIZE 128 #define MAX_SPI_WAIT_RETRIES 1000 #define MCU_MODE 0x6F @@ -310,8 +310,8 @@ * Using static buffer with maximum possible size, * extra byte is needed for prefixing the data port register at index 0. */ - uint8_t wbuf[PAGE_SIZE + 1] = { MCU_DATA_PORT }; - if (len > PAGE_SIZE) + uint8_t wbuf[REALTEK_PAGE_SIZE + 1] = { MCU_DATA_PORT }; + if (len > REALTEK_PAGE_SIZE) return SPI_GENERIC_ERROR; memcpy(&wbuf[1], buf, len); @@ -352,9 +352,9 @@ uint8_t dummy; realtek_mst_i2c_spi_read_register(fd, MCU_DATA_PORT, &dummy); - for (i = 0; i < len; i += PAGE_SIZE) { + for (i = 0; i < len; i += REALTEK_PAGE_SIZE) { ret |= realtek_mst_i2c_spi_read_data(fd, REGISTER_ADDRESS, - buf + i, min(len - i, PAGE_SIZE)); + buf + i, min(len - i, REALTEK_PAGE_SIZE)); if (ret) return ret; } @@ -376,11 +376,11 @@ return SPI_GENERIC_ERROR; ret |= realtek_mst_i2c_spi_write_register(fd, 0x6D, 0x02); /* write opcode */ - ret |= realtek_mst_i2c_spi_write_register(fd, 0x71, (PAGE_SIZE - 1)); /* fit len=256 */ + ret |= realtek_mst_i2c_spi_write_register(fd, 0x71, (REALTEK_PAGE_SIZE - 1)); /* fit len=256 */ - for (i = 0; i < len; i += PAGE_SIZE) { - uint16_t page_len = min(len - i, PAGE_SIZE); - if (len - i < PAGE_SIZE) + for (i = 0; i < len; i += REALTEK_PAGE_SIZE) { + uint16_t page_len = min(len - i, REALTEK_PAGE_SIZE); + if (len - i < REALTEK_PAGE_SIZE) ret |= realtek_mst_i2c_spi_write_register(fd, 0x71, page_len-1); ret |= realtek_mst_i2c_spi_map_page(fd, start + i); if (ret) -- To view, visit https://review.coreboot.org/c/flashrom/+/53998 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I043e3d8c2d2498e94b5e7577a7378c8c3e0e6c81 Gerrit-Change-Number: 53998 Gerrit-PatchSet: 1 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: linux_mtd: drop 'mtd_' prefix from programmer data fields
by Angel Pons (Code Review) 09 May '21

09 May '21

Attention is currently required from: Nikolai Artemiev. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/53953 ) Change subject: linux_mtd: drop 'mtd_' prefix from programmer data fields ...................................................................... Patch Set 1: Code-Review+2 (1 comment) File linux_mtd.c: https://review.coreboot.org/c/flashrom/+/53953/comment/23e8e4b7_f08edfbd PS1, Line 126: device_name Strictly speaking, this isn't a programmer data field. I don't really care, though. -- To view, visit https://review.coreboot.org/c/flashrom/+/53953 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2503c98e9111d1fecd911473f65eeea7031cfdc3 Gerrit-Change-Number: 53953 Gerrit-PatchSet: 1 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Sun, 09 May 2021 07:09:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

Change in flashrom[master]: realtek_mst_i2c_spi: Use `i2c_open_from_programmer_params`
by Edward O'Callaghan (Code Review) 08 May '21

08 May '21

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/52831 ) Change subject: realtek_mst_i2c_spi: Use `i2c_open_from_programmer_params` ...................................................................... realtek_mst_i2c_spi: Use `i2c_open_from_programmer_params` This allows using `buspath` to specify which I2C device to use. Change-Id: Ibdf07a9fde0ddfcda1c0bfa35a3e7cde5c22cedb Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/52831 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Reviewed-by: Peter Marheine <pmarheine(a)chromium.org> --- M realtek_mst_i2c_spi.c 1 file changed, 5 insertions(+), 36 deletions(-) Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved Peter Marheine: Looks good to me, but someone else must approve diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c index 27bcb4a..64579e1 100644 --- a/realtek_mst_i2c_spi.c +++ b/realtek_mst_i2c_spi.c @@ -442,38 +442,11 @@ return ret; } -static int get_params(int *i2c_bus, int *reset, int *enter_isp) +static int get_params(int *reset, int *enter_isp) { - char *bus_str = NULL, *reset_str = NULL, *isp_str = NULL; + char *reset_str = NULL, *isp_str = NULL; int ret = SPI_GENERIC_ERROR; - bus_str = extract_programmer_param("bus"); - if (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__); - goto _get_params_failed; - } - - if (bus < 0 || bus > 255) { - msg_perr("%s: Value for 'bus' is out of range(0-255).\n", __func__); - goto _get_params_failed; - } - - if (strlen(bus_suffix) > 0) { - msg_perr("%s: Garbage following 'bus' value.\n", __func__); - goto _get_params_failed; - } - - msg_pinfo("Using i2c bus %i.\n", bus); - *i2c_bus = bus; - ret = 0; - } else { - msg_perr("%s: Bus number not specified.\n", __func__); - } - reset_str = extract_programmer_param("reset-mcu"); if (reset_str) { if (reset_str[0] == '1') { @@ -504,22 +477,18 @@ } free(isp_str); -_get_params_failed: - if (bus_str) - free(bus_str); - return ret; } int realtek_mst_i2c_spi_init(void) { int ret = 0; - int i2c_bus = 0, reset = 0, enter_isp = 0; + int reset = 0, enter_isp = 0; - if (get_params(&i2c_bus, &reset, &enter_isp)) + if (get_params(&reset, &enter_isp)) return SPI_GENERIC_ERROR; - int fd = i2c_open(i2c_bus, REGISTER_ADDRESS, 0); + int fd = i2c_open_from_programmer_params(REGISTER_ADDRESS, 0); if (fd < 0) return fd; -- To view, visit https://review.coreboot.org/c/flashrom/+/52831 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibdf07a9fde0ddfcda1c0bfa35a3e7cde5c22cedb Gerrit-Change-Number: 52831 Gerrit-PatchSet: 2 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: lspcon_i2c_spi: Extract I2C bus parameter handling
by Edward O'Callaghan (Code Review) 08 May '21

08 May '21

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 <th3fanbus(a)gmail.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/52830 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> --- 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; -- To view, visit https://review.coreboot.org/c/flashrom/+/52830 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I006b311c88feea37fe4b217f769b21ca1505def9 Gerrit-Change-Number: 52830 Gerrit-PatchSet: 3 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

Change in flashrom[master]: lspcon_i2c_spi: Release I2C fd on failed init
by Edward O'Callaghan (Code Review) 08 May '21

08 May '21

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/52829 ) Change subject: lspcon_i2c_spi: Release I2C fd on failed init ...................................................................... lspcon_i2c_spi: Release I2C fd on failed init Change-Id: Idec3cd349ab8d6e2ebb0fafae70c5d69bb2c8880 Signed-off-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/52829 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-by: Peter Marheine <pmarheine(a)chromium.org> --- M lspcon_i2c_spi.c 1 file changed, 2 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved Peter Marheine: Looks good to me, but someone else must approve Anastasia Klimchuk: Looks good to me, but someone else must approve diff --git a/lspcon_i2c_spi.c b/lspcon_i2c_spi.c index b79e7f7..e2c4a3c 100644 --- a/lspcon_i2c_spi.c +++ b/lspcon_i2c_spi.c @@ -502,12 +502,14 @@ int ret = lspcon_i2c_spi_reset_mpu_stop(fd); if (ret) { msg_perr("%s: call to reset_mpu_stop failed.\n", __func__); + i2c_close(fd); return ret; } struct lspcon_i2c_spi_data *data = calloc(1, sizeof(struct lspcon_i2c_spi_data)); if (!data) { msg_perr("Unable to allocate space for extra SPI master data.\n"); + i2c_close(fd); return SPI_GENERIC_ERROR; } -- To view, visit https://review.coreboot.org/c/flashrom/+/52829 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Idec3cd349ab8d6e2ebb0fafae70c5d69bb2c8880 Gerrit-Change-Number: 52829 Gerrit-PatchSet: 2 Gerrit-Owner: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit May 2021