Joel Kitching 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 5: Code-Review+1
(10 comments)
https://review.coreboot.org/#/c/32031/2/src/mainboard/google/butterfly/chrom... File src/mainboard/google/butterfly/chromeos.c:
https://review.coreboot.org/#/c/32031/2/src/mainboard/google/butterfly/chrom... PS2, Line 39: /* Write Protect: GPIO active Low */
nit: not sure if we still need these comments everywhere because with the shorter notation it's all […]
I think I'd like to preserve them here for posterity's sake - we can clean them up / remove them in another CL if we wish.
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.
Done
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()... […]
What other kinds of places are they used? But yeah, probably can work on that separately.
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?
Done
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() ?
Done
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 […]
Done
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() ?
Done
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() ?
Done
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?
Will remove this in a subsequent CL.
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);
No idea... […]
Ack