On Mon, Mar 20, 2023 at 8:00 AM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> 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).
>
Named nodes _seem_ to be preferred in DT for when there isn't a
logical / natural
numbering to the child nodes. A.e. for USB, reg is used to specify
which port, for I2C,
which address on the bus, but for two parallel and independent
functions on the same
device, it seems named nodes would make more sense in DT. Many examples exist
in mainline where named nodes are used in DT in this way. One example
is network cards
which provide an mdio bus bind through the child "mdio". One example
of a specifically a
child i2c controller being bound to "i2c" can be found in
pine64,pinephone-keyboard.yaml.
But it's certainly possible this isn't the desired direction moving
forward in DT -- my opinion
should definitely be taken with a grain of salt. Maybe this is
something I should follow up on
with DT folks on that DT vs. ACPI thread made earlier.
One thing I did notice when looking at the mfd subsystem is that most
DT drivers actually
match on the compatible string of the child nodes, a.e.
"silabs,cp2112", "silabs,cp2112-gpio".
"silabs,cp2112-i2c". We could implement that here, but I think that
would make more sense if
we were to actually split the cp2112 into mfd & platform drivers, and
additionally split the DT
binding by function.
Thanks,
Danny Kaehn
