Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32031 )
Change subject: chromeos: update old boards to use lb_add_gpios notation ......................................................................
Patch Set 2:
(9 comments)
https://review.coreboot.org/#/c/32031/2/src/mainboard/google/parrot/chromeos... File src/mainboard/google/parrot/chromeos.c:
https://review.coreboot.org/#/c/32031/2/src/mainboard/google/parrot/chromeos... PS2, Line 39: if (!gpio_base) I don't see gpio_base used here.
https://review.coreboot.org/#/c/32031/2/src/mainboard/intel/baskingridge/chr... File src/mainboard/intel/baskingridge/chromeos.c:
https://review.coreboot.org/#/c/32031/2/src/mainboard/intel/baskingridge/chr... PS2, Line 32: u16 gpio_base = pci_read_config32(dev, GPIOBASE) & 0xfffe; I think we should have global declarations for things like pm_base() and gpio_base()... Not something you would fix here though.
https://review.coreboot.org/#/c/32031/2/src/mainboard/intel/baskingridge/chr... PS2, Line 42: {0, ACTIVE_LOW, (gp_lvl >> 22) & 1, "write protect"}, Complete get_write_protect_state() implementation below instead?
https://review.coreboot.org/#/c/32031/2/src/mainboard/intel/baskingridge/chr... PS2, Line 45: {69, ACTIVE_HIGH, (gp_lvl3 >> (69-64)) & 1, "recovery"}, Use get_recovery_mode_switch() ?
https://review.coreboot.org/#/c/32031/2/src/mainboard/intel/emeraldlake2/chr... File src/mainboard/intel/emeraldlake2/chromeos.c:
https://review.coreboot.org/#/c/32031/2/src/mainboard/intel/emeraldlake2/chr... PS2, Line 38: u32 gp_lvl2 = inl(gpio_base + 0x38); GP_LVL==0x0c, GP_LVL3==0x38
But I think this function can be written without gpio_base(), see below.
https://review.coreboot.org/#/c/32031/2/src/mainboard/intel/emeraldlake2/chr... PS2, Line 43: {48, ACTIVE_LOW, (gp_lvl2 >> (48-32)) & 1, "write protect"}, Implement get_write_protect_state() ?
https://review.coreboot.org/#/c/32031/2/src/mainboard/intel/emeraldlake2/chr... PS2, Line 46: {22, ACTIVE_LOW, (gp_lvl >> 22) & 1, "recovery"}, Use get_recovery_mode_switch() ?
https://review.coreboot.org/#/c/32031/2/src/mainboard/intel/emeraldlake2/chr... PS2, Line 69: CROS_GPIO_DEV_AH(57, CROS_GPIO_DEVICE_NAME), Looks like phys dev switch?
https://review.coreboot.org/#/c/32031/2/src/mainboard/samsung/lumpy/chromeos... File src/mainboard/samsung/lumpy/chromeos.c:
https://review.coreboot.org/#/c/32031/2/src/mainboard/samsung/lumpy/chromeos... PS2, Line 101: pci_write_config32(dev, SATA_SP, flags); Just curious, why do we stash the GPIO states here? Looked like some other boards do not.