Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45911 )
Change subject: drivers/i2c: Add chip driver for GPIO based I2C multiplexer ......................................................................
Patch Set 4:
(6 comments)
https://review.coreboot.org/c/coreboot/+/45911/3/src/drivers/i2c/gpio_mux/gp... File src/drivers/i2c/gpio_mux/gpio_mux.c:
https://review.coreboot.org/c/coreboot/+/45911/3/src/drivers/i2c/gpio_mux/gp... PS3, Line 34: i2c_gpio_mux_adapter_fill_ssdt suggestion: i2c_gpio_mux_adapter_fill
https://review.coreboot.org/c/coreboot/+/45911/3/src/drivers/i2c/gpio_mux/gp... PS3, Line 37: acpigen_write_name_integer acpigen_write_ADR ?
https://review.coreboot.org/c/coreboot/+/45911/3/src/drivers/i2c/gpio_mux/gp... PS3, Line 40: i2c_gpio_mux_fill_ssdt suggestion: i2c_gpio_mux_write_gpios
https://review.coreboot.org/c/coreboot/+/45911/3/src/drivers/i2c/gpio_mux/gp... PS3, Line 40: static void i2c_gpio_mux_fill_ssdt(struct drivers_i2c_gpio_mux_config *config) : { : /* Resources */ : acpigen_write_name("_CRS"); : acpigen_write_resourcetemplate_header(); : acpi_device_write_gpio(&config->mux_gpio); : acpigen_write_resourcetemplate_footer(); All this function uses from `config` is the mux_gpio field, why not pass that in instead (const struct acpi_gpio *) ?
https://review.coreboot.org/c/coreboot/+/45911/3/src/drivers/i2c/gpio_mux/gp... PS3, Line 50: i2c_gpio_mux_fill_ssdt_generator note: we removed the 'generator' names a while ago.
https://review.coreboot.org/c/coreboot/+/45911/3/src/drivers/i2c/gpio_mux/gp... PS3, Line 51: { : const char *scope = acpi_device_scope(dev); : const char *path = acpi_device_path(dev); : struct drivers_i2c_gpio_mux_config *config = dev->chip_info; : : if (!dev->enabled || !scope) : return; : Could you restructure this so that dev is checked for NULL first? That will make coverity happy.