Karthik Ramasubramanian 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 7:
(14 comments)
https://review.coreboot.org/c/coreboot/+/45911/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45911/4//COMMIT_MSG@8 PS4, Line 8:
Can you mention the kernel driver this is intended to be used with?
Added the kernel driver for both mux and bus chip drivers in the commit message.
https://review.coreboot.org/c/coreboot/+/45911/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/45911/6//COMMIT_MSG@48 PS6, Line 48: Package (0x02) : { : "mux-gpios", : Package (0x04) : { : _SB.PCI0.I2C3.MUX0, : Zero, : Zero, : Zero : } : } : }
Where is this used?
Added the kernel driver with a link to its doc in the commit summary.
https://review.coreboot.org/c/coreboot/+/45911/5/src/drivers/i2c/gpio_mux/ch... File src/drivers/i2c/gpio_mux/chip.h:
https://review.coreboot.org/c/coreboot/+/45911/5/src/drivers/i2c/gpio_mux/ch... PS5, Line 16: char
const
This has been done for the bus chip info here: https://review.coreboot.org/c/coreboot/+/46144/2/src/drivers/i2c/gpiomux/bus...
The mux chip info does not need a name field now.
https://review.coreboot.org/c/coreboot/+/45911/5/src/drivers/i2c/gpio_mux/ch... PS5, Line 17: acpi_hid
I don't think we need to take in acpi_hid as a parameter from the mainboard. […]
Done
https://review.coreboot.org/c/coreboot/+/45911/5/src/drivers/i2c/gpio_mux/ch... PS5, Line 19: /* GPIOs used to select the mux lines */ : uint32_t mux_gpio_count; : struct acpi_gpio mux_gpio[MAX_NUM_MUX_GPIOS];
Should we put these in a structure just to make it clear that they are applicable only to i2c_mux?
Now that the bus and mux chip drivers are split, I have not created a nested structure.
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
Since the bus and mux drivers are split, no need for these helper functions.
https://review.coreboot.org/c/coreboot/+/45911/3/src/drivers/i2c/gpio_mux/gp... PS3, Line 37: acpigen_write_name_integer
acpigen_write_ADR ?
Done here: https://review.coreboot.org/c/coreboot/+/46144/2/src/drivers/i2c/gpiomux/bus...
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
The drivers are split and hence no need for this helper function.
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 stru […]
The drivers are split and hence no need for this helper function.
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.
Done
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.
acpi_device_scope returns NULL if dev is NULL acpi_device_path returns NULL if dev is NULL config_of(dev) is being used to get the chip information. All these things ensure NULL pointer access is not performed.
Should I still restructure the code to satisfy coverity.
https://review.coreboot.org/c/coreboot/+/45911/4/src/drivers/i2c/gpio_mux/gp... File src/drivers/i2c/gpio_mux/gpio_mux.c:
PS4:
Exactly what I was thinking
Done
https://review.coreboot.org/c/coreboot/+/45911/6/src/drivers/i2c/gpiomux/mux... File src/drivers/i2c/gpiomux/mux/mux.c:
https://review.coreboot.org/c/coreboot/+/45911/6/src/drivers/i2c/gpiomux/mux... PS6, Line 55:
space instead of tab.
Done
https://review.coreboot.org/c/coreboot/+/45911/6/src/drivers/i2c/gpiomux/mux... PS6, Line 76: if (!config) : return;
Why is this check required?
My bad. Forgot to remove the dead code. The current check for NULL dev is to satisfy coverity.