Re: [PATCH v9 3/3] HID: cp2112: Fwnode Support

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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@linux.intel.com> 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