On Sun, Mar 19, 2023 at 03:48:02PM -0500, Danny Kaehn wrote:
> Support describing the CP2112's I2C and GPIO interfaces in firmware.
>
> I2C and GPIO child nodes can either be children with names "i2c" and
> "gpio", or, for ACPI, device nodes with _ADR Zero and One,
> respectively.
>
> Additionally, support configuring the I2C bus speed from the
> clock-frequency device property.
Thank you for the update, my comments below.
...
> + struct i2c_timings timings;
> + struct fwnode_handle *child;
Longer line first easier to read.
> + u32 addr;
> + const char *name;
Ditto.
...
> + device_for_each_child_node(&hdev->dev, child) {
> + name = fwnode_get_name(child);
> + ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> +
> + if ((name && strcmp("i2c", name) == 0) || (!ret && addr == 0))
> + device_set_node(&dev->adap.dev, child);
> + else if ((name && strcmp("gpio", name)) == 0 ||
> + (!ret && addr == 1))
> + dev->gc.fwnode = child;
> + }
Please, make addresses defined explicitly. You may also do it with node naming
schema:
#define CP2112_I2C_ADR 0
#define CP2112_GPIO_ADR 1
static const char * const cp2112_cell_names[] = {
[CP2112_I2C_ADR] = "i2c",
[CP2112_GPIO_ADR] = "gpio",
};
device_for_each_child_node(&hdev->dev, child) {
name = fwnode_get_name(child);
if (name) {
ret = match_string(cp2112_cell_names, ARRAY_SIZE(cp2112_cell_names), name);
if (ret >= 0)
addr = ret;
} else
ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
if (ret < 0)
...error handling if needed...
switch (addr) {
case CP2112_I2C_ADR:
device_set_node(&dev->adap.dev, child);
break;
case CP2112_GPIO_ADR:
dev->gc.fwnode = child;
break;
default:
...error handling...
}
}
--
With Best Regards,
Andy Shevchenko
