On Mon, Mar 20, 2023 at 02:58:07PM +0200, Andy Shevchenko wrote:
> On Sun, Mar 19, 2023 at 03:48:02PM -0500, Danny Kaehn wrote:
...
> > + 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...
> }
> }
Btw, don't you use "reg" property for the child nodes? It would be better from
de facto used patterns (we have a couple of mode drivers that have a common
code to read "reg" or _ADR() and that code can be split into a helper and used
here).
--
With Best Regards,
Andy Shevchenko
