Furquan Shaikh 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 5:
(5 comments)
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
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. This should be a fixed HID value based on what the kernel driver expects.
From the kernel driver implementation, I don't see any ACPI HID since the driver wasn't written for ACPI support. The way we have handled this for some other drivers is using PRP0001 HID and device tree compatible string in _DSD. I am guessing that we would probably go down the same route here.
References: 1. GPIO keys driver in coreboot: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/master:src/th...
2. https://www.kernel.org/doc/html/latest/firmware-guide/acpi/enumeration.html#...
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?
https://review.coreboot.org/c/coreboot/+/45911/4/src/drivers/i2c/gpio_mux/gp... File src/drivers/i2c/gpio_mux/gpio_mux.c:
PS4:
Should this really just two separate chip drivers? It looks like the acpi_fill_ssdt() for each is to […]
I proposed that just to keep the support for the mux and the bus closer in coreboot. Probably we can organize it like drivers/i2c/gpio_mux/mux and drivers/i2c/gpio_mux/bus and have 2 separate chip drivers support them?
https://review.coreboot.org/c/coreboot/+/45911/5/src/drivers/i2c/gpio_mux/gp... File src/drivers/i2c/gpio_mux/gpio_mux.c:
https://review.coreboot.org/c/coreboot/+/45911/5/src/drivers/i2c/gpio_mux/gp... PS5, Line 108: dev->ops = &i2c_gpio_mux_ops; I see that Tim has commented about having 2 separate chip drivers. In case we don't go with that, it would be good to add separate ops here for mux and bus so that you don't have to check the device type in other functions:
static struct device_operations i2c_gpio_mux_ops = { .read_resources = noop_read_resources, .set_resources = noop_set_resources, .scan_bus = scan_static_bus, .acpi_name = mux_acpi_name, .acpi_fill_ssdt = mux_fill_ssdt, };
static struct device_operations i2c_gpio_bus_ops = { .read_resources = noop_read_resources, .set_resources = noop_set_resources, .acpi_name = bus_acpi_name, .acpi_fill_ssdt = bus_fill_ssdt, };
and in here you can set the ops depending upon the device type:
static void i2c_gpio_mux_enable(struct device *dev) { struct drivers_i2c_gpio_mux_config *config = dev->chip_info;
if (!config) return;
if (dev->device_type == I2C_MUX) dev->ops = &i2c_gpio_mux_ops; else if (dev->device_type == I2C_MUX_ADAPTER) dev->ops = &i2c_gpio_bus_ops; }
-
Furquan Shaikh (Code Review)