Change in coreboot[master]: drivers/i2c/at24rf08c: Cache devices associated with this driver
Nico Huber has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35490 ) Change subject: drivers/i2c/at24rf08c: Cache devices associated with this driver ...................................................................... drivers/i2c/at24rf08c: Cache devices associated with this driver To get rid of the fragile dev_find_slot_on_smbus(), we let the chip driver register the individual devices (EEPROMs) instead. They will be cached in an global array. So, the implementation assumes that there is only one instance of this driver but that should always be the case. Change-Id: I11eade2cea924839f4b1e1eeee612931fdfd1299 Signed-off-by: Nico Huber <nico.huber@secunet.com> --- M src/drivers/i2c/at24rf08c/at24rf08c.c M src/drivers/i2c/at24rf08c/lenovo.h M src/drivers/i2c/at24rf08c/lenovo_serials.c 3 files changed, 42 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/35490/1 diff --git a/src/drivers/i2c/at24rf08c/at24rf08c.c b/src/drivers/i2c/at24rf08c/at24rf08c.c index 67760a0..7fbae84 100644 --- a/src/drivers/i2c/at24rf08c/at24rf08c.c +++ b/src/drivers/i2c/at24rf08c/at24rf08c.c @@ -18,6 +18,7 @@ #include <device/smbus.h> #include <smbios.h> #include <console/console.h> +#include "lenovo.h" static void at24rf08c_init(struct device *dev) { @@ -26,9 +27,14 @@ if (!dev->enabled) return; + if (dev->path.type != DEVICE_PATH_I2C) + return; + + lenovo_serials_register_bank(dev); + /* Ensure that EEPROM/RFID chip is not accessible through RFID. Need to do it only on 5c. */ - if (dev->path.type != DEVICE_PATH_I2C || dev->path.i2c.device != 0x5c) + if (dev->path.i2c.device != 0x5c) return; printk (BIOS_DEBUG, "Locking EEPROM RFID\n"); diff --git a/src/drivers/i2c/at24rf08c/lenovo.h b/src/drivers/i2c/at24rf08c/lenovo.h index 6824eb6..45d45cc 100644 --- a/src/drivers/i2c/at24rf08c/lenovo.h +++ b/src/drivers/i2c/at24rf08c/lenovo.h @@ -1 +1,23 @@ +/* + * This file is part of the coreboot project. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef DRIVERS_I2C_AT24RF08C_LENOVO_H +#define DRIVERS_I2C_AT24RF08C_LENOVO_H + +#include <device/device.h> + const char *lenovo_mainboard_partnumber(void); + +void lenovo_serials_register_bank(struct device *); + +#endif /* DRIVERS_I2C_AT24RF08C_LENOVO_H */ diff --git a/src/drivers/i2c/at24rf08c/lenovo_serials.c b/src/drivers/i2c/at24rf08c/lenovo_serials.c index 0a6b343..584714e 100644 --- a/src/drivers/i2c/at24rf08c/lenovo_serials.c +++ b/src/drivers/i2c/at24rf08c/lenovo_serials.c @@ -24,10 +24,20 @@ #define ERROR_STRING "*INVALID*" +static struct device *banks[4]; + +void lenovo_serials_register_bank(struct device *const dev) +{ + if (0x54 > dev->path.i2c.device || dev->path.i2c.device > 0x57) + return; + banks[dev->path.i2c.device & 3] = dev; +} + static struct device *at24rf08c_find_bank(u8 bank) { - struct device *dev; - dev = dev_find_slot_on_smbus(1, 0x54 | bank); + struct device *const dev = banks[bank]; + if (bank > ARRAY_SIZE(banks)) + return NULL; if (!dev) printk(BIOS_WARNING, "EEPROM not found\n"); return dev; @@ -72,7 +82,6 @@ dev = at24rf08c_find_bank(bank); if (dev == NULL) { - printk(BIOS_WARNING, "EEPROM not found\n"); memcpy(result, ERROR_STRING, sizeof (ERROR_STRING)); return; } @@ -134,9 +143,8 @@ memset(result, 0, sizeof (result)); - dev = dev_find_slot_on_smbus(1, 0x56); + dev = at24rf08c_find_bank(2); if (dev == NULL) { - printk(BIOS_WARNING, "EEPROM not found\n"); already_read = 1; memset(uuid, 0, 16); return; -- To view, visit https://review.coreboot.org/c/coreboot/+/35490 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I11eade2cea924839f4b1e1eeee612931fdfd1299 Gerrit-Change-Number: 35490 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h@gmx.de> Gerrit-MessageType: newchange
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35490 ) Change subject: drivers/i2c/at24rf08c: Cache devices associated with this driver ...................................................................... Patch Set 1: (2 comments) https://review.coreboot.org/c/coreboot/+/35490/1/src/drivers/i2c/at24rf08c/l... File src/drivers/i2c/at24rf08c/lenovo.h: https://review.coreboot.org/c/coreboot/+/35490/1/src/drivers/i2c/at24rf08c/l... PS1, Line 21: void lenovo_serials_register_bank(struct device *); function definition argument 'struct device *' should also have an identifier name https://review.coreboot.org/c/coreboot/+/35490/1/src/drivers/i2c/at24rf08c/l... File src/drivers/i2c/at24rf08c/lenovo_serials.c: https://review.coreboot.org/c/coreboot/+/35490/1/src/drivers/i2c/at24rf08c/l... PS1, Line 31: if (0x54 > dev->path.i2c.device || dev->path.i2c.device > 0x57) Comparisons should place the constant on the right side of the test -- To view, visit https://review.coreboot.org/c/coreboot/+/35490 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I11eade2cea924839f4b1e1eeee612931fdfd1299 Gerrit-Change-Number: 35490 Gerrit-PatchSet: 1 Gerrit-Owner: Nico Huber <nico.h@gmx.de> Gerrit-CC: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-Comment-Date: Fri, 20 Sep 2019 15:16:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Hello build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/coreboot/+/35490 to look at the new patch set (#2). Change subject: drivers/i2c/at24rf08c: Cache devices associated with this driver ...................................................................... drivers/i2c/at24rf08c: Cache devices associated with this driver To get rid of the fragile dev_find_slot_on_smbus(), we let the chip driver register the individual devices (EEPROMs) instead. They will be cached in an global array. So, the implementation assumes that there is only one instance of this driver but that should always be the case. Change-Id: I11eade2cea924839f4b1e1eeee612931fdfd1299 Signed-off-by: Nico Huber <nico.huber@secunet.com> --- M src/drivers/i2c/at24rf08c/at24rf08c.c M src/drivers/i2c/at24rf08c/lenovo.h M src/drivers/i2c/at24rf08c/lenovo_serials.c 3 files changed, 42 insertions(+), 6 deletions(-) git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/35490/2 -- To view, visit https://review.coreboot.org/c/coreboot/+/35490 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I11eade2cea924839f4b1e1eeee612931fdfd1299 Gerrit-Change-Number: 35490 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-MessageType: newpatchset
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35490 ) Change subject: drivers/i2c/at24rf08c: Cache devices associated with this driver ...................................................................... Patch Set 2: (1 comment) https://review.coreboot.org/c/coreboot/+/35490/2/src/drivers/i2c/at24rf08c/l... File src/drivers/i2c/at24rf08c/lenovo.h: https://review.coreboot.org/c/coreboot/+/35490/2/src/drivers/i2c/at24rf08c/l... PS2, Line 21: void lenovo_serials_register_bank(struct device *); function definition argument 'struct device *' should also have an identifier name -- To view, visit https://review.coreboot.org/c/coreboot/+/35490 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I11eade2cea924839f4b1e1eeee612931fdfd1299 Gerrit-Change-Number: 35490 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Kyösti Mälkki <kyosti.malkki@gmail.com> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Sun, 06 Oct 2019 16:31:57 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Attention is currently required from: Nico Huber. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35490 ) Change subject: drivers/i2c/at24rf08c: Cache devices associated with this driver ...................................................................... Patch Set 2: Code-Review+1 -- To view, visit https://review.coreboot.org/c/coreboot/+/35490 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I11eade2cea924839f4b1e1eeee612931fdfd1299 Gerrit-Change-Number: 35490 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Attention: Nico Huber <nico.h@gmx.de> Gerrit-Comment-Date: Mon, 15 Mar 2021 10:14:41 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35490 ) Change subject: drivers/i2c/at24rf08c: Cache devices associated with this driver ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: Did I write this? looks good ;) -- To view, visit https://review.coreboot.org/c/coreboot/+/35490 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I11eade2cea924839f4b1e1eeee612931fdfd1299 Gerrit-Change-Number: 35490 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Fri, 19 Mar 2021 12:44:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Attention is currently required from: Nico Huber. Elyes Haouas has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35490 ) Change subject: drivers/i2c/at24rf08c: Cache devices associated with this driver ...................................................................... Patch Set 2: (1 comment) File src/drivers/i2c/at24rf08c/lenovo_serials.c: https://review.coreboot.org/c/coreboot/+/35490/comment/e52a329f_971a4a56 PS2, Line 214: if (s != NULL) : return s; this will never happen, right ? please see https://review.coreboot.org/c/coreboot/+/61969 -- To view, visit https://review.coreboot.org/c/coreboot/+/35490 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I11eade2cea924839f4b1e1eeee612931fdfd1299 Gerrit-Change-Number: 35490 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Elyes Haouas <ehaouas@noos.fr> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-Attention: Nico Huber <nico.h@gmx.de> Gerrit-Comment-Date: Tue, 15 Feb 2022 11:23:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Attention is currently required from: Elyes Haouas. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35490 ) Change subject: drivers/i2c/at24rf08c: Cache devices associated with this driver ...................................................................... Patch Set 2: (1 comment) File src/drivers/i2c/at24rf08c/lenovo_serials.c: https://review.coreboot.org/c/coreboot/+/35490/comment/5535da22_764f8d7d PS2, Line 214: if (s != NULL) : return s;
this will never happen, right ? […] `s` is static, i.e. not a local stack variable. So it can have different values if the function is called multiple times.
-- To view, visit https://review.coreboot.org/c/coreboot/+/35490 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I11eade2cea924839f4b1e1eeee612931fdfd1299 Gerrit-Change-Number: 35490 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Elyes Haouas <ehaouas@noos.fr> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-Attention: Elyes Haouas <ehaouas@noos.fr> Gerrit-Comment-Date: Tue, 15 Feb 2022 12:32:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Elyes Haouas <ehaouas@noos.fr> Gerrit-MessageType: comment
Nico Huber has posted comments on this change by Nico Huber. ( https://review.coreboot.org/c/coreboot/+/35490?usp=email ) Change subject: drivers/i2c/at24rf08c: Cache devices associated with this driver ...................................................................... Patch Set 2: (1 comment) Patchset: PS2:
Hey Nico, is this sill relevant? Looks like we got a new call in the meantime (drivers/i2c/ptn3460) but at least that uses bus 0, so we could still get rid of the indeterministic bus 1 used here.
-- To view, visit https://review.coreboot.org/c/coreboot/+/35490?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: coreboot Gerrit-Branch: main Gerrit-Change-Id: I11eade2cea924839f4b1e1eeee612931fdfd1299 Gerrit-Change-Number: 35490 Gerrit-PatchSet: 2 Gerrit-Owner: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Patrick Rudolph <rudolphpatrick05@gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Paul Menzel <paulepanter@mailbox.org> Gerrit-CC: coreboot org <coreboot.org@gmail.com> Gerrit-Comment-Date: Wed, 11 Sep 2024 22:14:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: coreboot org <coreboot.org@gmail.com>
participants (4)
-
Angel Pons (Code Review) -
build bot (Jenkins) (Code Review)
-
Elyes Haouas (Code Review)
-
Nico Huber (Code Review)