Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46961 )
Change subject: drivers/usb/acpi: Add support for privacy_gpio ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/46961/2/src/drivers/usb/acpi/chip.h File src/drivers/usb/acpi/chip.h:
https://review.coreboot.org/c/coreboot/+/46961/2/src/drivers/usb/acpi/chip.h... PS2, Line 48: privacy If we decide to go with this name, I think we should have a comment block explaining what this is used for and what kind of devices would use it.
https://review.coreboot.org/c/coreboot/+/46961/2/src/drivers/usb/acpi/usb_ac... File src/drivers/usb/acpi/usb_acpi.c:
https://review.coreboot.org/c/coreboot/+/46961/2/src/drivers/usb/acpi/usb_ac... PS2, Line 13: /* : * Return false if reset GPIO is not provided. : */ Comment needs update. Or we can probably drop it since it isn't really very helpful.
https://review.coreboot.org/c/coreboot/+/46961/2/src/drivers/usb/acpi/usb_ac... PS2, Line 60: I think it would be safer to set reset_index and privacy_index here instead of expecting the same order of _CRS and _DSD writes for the GPIOs.
reset_index = acpi_device_write_gpio(&config->reset_gpio, &index); privacy_index = acpi_device_write_gpio(&config->privacy_gpio, &index);
acpi_device_write_gpio() can return -1 if gpio.pin_count is 0. Else the index used for the GPIO and increment current index.
https://review.coreboot.org/c/coreboot/+/46961/2/src/drivers/usb/acpi/usb_ac... PS2, Line 70: privacy Do we have the liberty to choose the gpio name string that the driver uses? I would prefer using "irq-gpio" if possible because it aligns well with how the other drivers are written and the GPIOs they expose.