On Mon, Mar 20, 2023 at 08:40:07AM -0500, Daniel Kaehn wrote:
> 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.
Okay, I'm not an expert in the DT preferable schemas, so I believe DT people
should answer on this.
> 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.
IIRC (but might be very well mistaken) the compatible strings for children
are discouraged.
--
With Best Regards,
Andy Shevchenko
