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;
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
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
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
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
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 ;)
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
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.
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.
-
Angel Pons (Code Review) -
build bot (Jenkins) (Code Review)
-
Elyes Haouas (Code Review)
-
Nico Huber (Code Review)