| Message ID | 20210112134054.342-1-calvin.johnson@oss.nxp.com |
|---|---|
| Headers | show |
| Series | ACPI support for dpaa2 driver | expand |
On Tue, Jan 12, 2021 at 3:43 PM Calvin Johnson <calvin.johnson@oss.nxp.com> wrote: > > Define phylink_fwnode_phy_connect() to connect phy specified by > a fwnode to a phylink instance. ... > + phy_dev = fwnode_phy_find_device(phy_fwnode); > + /* We're done with the phy_node handle */ > + fwnode_handle_put(phy_fwnode); > + if (!phy_dev) > + return -ENODEV; > + > + ret = phy_attach_direct(pl->netdev, phy_dev, flags, > + pl->link_interface); > + if (ret) Hmm... Shouldn't you put phy_dev here? > + return ret;
On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote: > On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson > <calvin.johnson@oss.nxp.com> wrote: > > > > Using fwnode_get_id(), get the reg property value for DT node > > or get the _ADR object value for ACPI node. ... > > +/** > > + * fwnode_get_id - Get the id of a fwnode. > > + * @fwnode: firmware node > > + * @id: id of the fwnode > > + * > > + * This function provides the id of a fwnode which can be either > > + * DT or ACPI node. For ACPI, "reg" property value, if present will > > + * be provided or else _ADR value will be provided. > > + * Returns 0 on success or a negative errno. > > + */ > > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id) > > +{ > > +#ifdef CONFIG_ACPI > > + unsigned long long adr; > > + acpi_status status; > > +#endif > > + int ret; > > + > > + ret = fwnode_property_read_u32(fwnode, "reg", id); > > + if (!(ret && is_acpi_node(fwnode))) > > + return ret; > > + > > +#ifdef CONFIG_ACPI > > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode), > > + METHOD_NAME__ADR, NULL, &adr); > > + if (ACPI_FAILURE(status)) > > + return -EINVAL; > > + *id = (u32)adr; > > +#endif > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(fwnode_get_id); > Please don't do it this way. The whole point of fwnode_operations is > to avoid conditional stuff at the fwnode level. Not fully true. We have non-POD getters that are conditional. Moreover, we have additional layer of Primary / Secondary fwnodes on top of that. The caller of fwnode API is indeed agnostic, but under the hood it differs by the definition (obviously due to natural differences between ACPI and DT and whatever else might come in the future. > Also ACPI and DT > aren't mutually exclusive if I'm not mistaken. That's why we try 'reg' property for both cases first. is_acpi_fwnode() conditional is that what I don't like though. ... > fwnode is lower level that the device-driver framework. Agree. > Making > it aware of busses like mdio, etc doesn't sound right. Disagree. Conceptually resource providers can be quite different and fwnode API *is* LCM for them.
On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > On Tue, Jan 12, 2021 at 7:02 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote: > > > On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson > > > <calvin.johnson@oss.nxp.com> wrote: ... > > > > + ret = fwnode_property_read_u32(fwnode, "reg", id); > > > > + if (!(ret && is_acpi_node(fwnode))) > > > > + return ret; > > > > + > > > > +#ifdef CONFIG_ACPI > > > > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode), > > > > + METHOD_NAME__ADR, NULL, &adr); > > > > + if (ACPI_FAILURE(status)) > > > > + return -EINVAL; > > > > + *id = (u32)adr; > > > > +#endif > > > > + return 0; > > > Also ACPI and DT > > > aren't mutually exclusive if I'm not mistaken. > > > > That's why we try 'reg' property for both cases first. > > > > is_acpi_fwnode() conditional is that what I don't like though. > > I'm not sure what you mean here, care to elaborate? I meant is_acpi_node(fwnode) in the conditional. I think it's redundant and we can simple do something like this: if (ret) { #ifdef ACPI ... #else return ret; #endif } return 0; -- With Best Regards, Andy Shevchenko
On Wed, Jan 20, 2021 at 7:51 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Tue, Jan 12, 2021 at 7:02 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote: > > > > On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson > > > > <calvin.johnson@oss.nxp.com> wrote: > > ... > > > > > > + ret = fwnode_property_read_u32(fwnode, "reg", id); > > > > > + if (!(ret && is_acpi_node(fwnode))) > > > > > + return ret; > > > > > + > > > > > +#ifdef CONFIG_ACPI > > > > > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode), > > > > > + METHOD_NAME__ADR, NULL, &adr); > > > > > + if (ACPI_FAILURE(status)) > > > > > + return -EINVAL; > > > > > + *id = (u32)adr; > > > > > +#endif > > > > > + return 0; > > > > > Also ACPI and DT > > > > aren't mutually exclusive if I'm not mistaken. > > > > > > That's why we try 'reg' property for both cases first. > > > > > > is_acpi_fwnode() conditional is that what I don't like though. > > > > I'm not sure what you mean here, care to elaborate? > > I meant is_acpi_node(fwnode) in the conditional. > > I think it's redundant and we can simple do something like this: > > if (ret) { > #ifdef ACPI > ... > #else > return ret; > #endif > } > return 0; > > -- Right, that should work. And I'd prefer it too.
On Wed, Jan 20, 2021 at 11:15 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Wed, Jan 20, 2021 at 7:51 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > > On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > On Tue, Jan 12, 2021 at 7:02 PM Andy Shevchenko > > > <andy.shevchenko@gmail.com> wrote: > > > > On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote: > > > > > On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson > > > > > <calvin.johnson@oss.nxp.com> wrote: > > > > ... > > > > > > > > + ret = fwnode_property_read_u32(fwnode, "reg", id); > > > > > > + if (!(ret && is_acpi_node(fwnode))) > > > > > > + return ret; > > > > > > + > > > > > > +#ifdef CONFIG_ACPI > > > > > > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode), > > > > > > + METHOD_NAME__ADR, NULL, &adr); > > > > > > + if (ACPI_FAILURE(status)) > > > > > > + return -EINVAL; > > > > > > + *id = (u32)adr; > > > > > > +#endif > > > > > > + return 0; > > > > > > > Also ACPI and DT > > > > > aren't mutually exclusive if I'm not mistaken. > > > > > > > > That's why we try 'reg' property for both cases first. > > > > > > > > is_acpi_fwnode() conditional is that what I don't like though. > > > > > > I'm not sure what you mean here, care to elaborate? > > > > I meant is_acpi_node(fwnode) in the conditional. > > > > I think it's redundant and we can simple do something like this: > > > > if (ret) { > > #ifdef ACPI > > ... > > #else > > return ret; > > #endif > > } > > return 0; > > > > -- > > Right, that should work. And I'd prefer it too. Rafael, I'd rather this new function be an ops instead of a bunch of #ifdef or if (acpi) checks. Thoughts? -Saravana
On Wed, Jan 20, 2021 at 9:01 PM Saravana Kannan <saravanak@google.com> wrote: > > On Wed, Jan 20, 2021 at 11:15 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Wed, Jan 20, 2021 at 7:51 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > > > > On Wed, Jan 20, 2021 at 8:18 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > > On Tue, Jan 12, 2021 at 7:02 PM Andy Shevchenko > > > > <andy.shevchenko@gmail.com> wrote: > > > > > On Tue, Jan 12, 2021 at 09:30:31AM -0800, Saravana Kannan wrote: > > > > > > On Tue, Jan 12, 2021 at 5:42 AM Calvin Johnson > > > > > > <calvin.johnson@oss.nxp.com> wrote: > > > > > > ... > > > > > > > > > > + ret = fwnode_property_read_u32(fwnode, "reg", id); > > > > > > > + if (!(ret && is_acpi_node(fwnode))) > > > > > > > + return ret; > > > > > > > + > > > > > > > +#ifdef CONFIG_ACPI > > > > > > > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode), > > > > > > > + METHOD_NAME__ADR, NULL, &adr); > > > > > > > + if (ACPI_FAILURE(status)) > > > > > > > + return -EINVAL; > > > > > > > + *id = (u32)adr; > > > > > > > +#endif > > > > > > > + return 0; > > > > > > > > > Also ACPI and DT > > > > > > aren't mutually exclusive if I'm not mistaken. > > > > > > > > > > That's why we try 'reg' property for both cases first. > > > > > > > > > > is_acpi_fwnode() conditional is that what I don't like though. > > > > > > > > I'm not sure what you mean here, care to elaborate? > > > > > > I meant is_acpi_node(fwnode) in the conditional. > > > > > > I think it's redundant and we can simple do something like this: > > > > > > if (ret) { > > > #ifdef ACPI > > > ... > > > #else > > > return ret; > > > #endif > > > } > > > return 0; > > > > > > -- > > > > Right, that should work. And I'd prefer it too. > > Rafael, > > I'd rather this new function be an ops instead of a bunch of #ifdef or > if (acpi) checks. Thoughts? Well, it looks more like a helper function than like an op and I'm not even sure how many potential users of it will expect that _ADR should be evaluated in the absence of the "reg" property. It's just that the "reg" property happens to be kind of an _ADR equivalent in this particular binding AFAICS.
On Fri, Jan 22, 2021 at 10:59 PM Saravana Kannan <saravanak@google.com> wrote: > On Fri, Jan 22, 2021 at 8:34 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Wed, Jan 20, 2021 at 9:01 PM Saravana Kannan <saravanak@google.com> wrote: > > > On Wed, Jan 20, 2021 at 11:15 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > > I'd rather this new function be an ops instead of a bunch of #ifdef or > > > if (acpi) checks. Thoughts? > > > > Well, it looks more like a helper function than like an op and I'm not > > even sure how many potential users of it will expect that _ADR should > > be evaluated in the absence of the "reg" property. > > > > It's just that the "reg" property happens to be kind of an _ADR > > equivalent in this particular binding AFAICS. > > I agree it is not clear how useful this helper function is going to be. > > But in general, to me, any time the wrapper/helper functions in > drivers/base/property.c need to do something like this: > > if (ACPI) > ACPI specific code > if (OF) > OF specific code > > I think the code should be pushed to the fwnode ops. That's one of the > main point of fwnode. So that firmware specific stuff is done by > firmware specific code. Also, when adding support for new firmware, > it's pretty clear what support the firmware needs to implement. > Instead of having to go fix up a bunch of code all over the place. Wishful thinking. In the very case of GPIO it's related to framework using headers local to framework. Are you suggesting to open its guts to the entire wild world? I don't think it's a good idea. You see, here we have different layering POD types, which are natural and quite low level that ops suits best for them and quite different resource types like GPIO. And the latter is closer to certain framework rather than to POD handling cases. > So fwnode_ops->get_id() would be the OP ACPI and OF would implement. > And then we can have a wrapper in drivers/base/property.c. -- With Best Regards, Andy Shevchenko
This patch set provides ACPI support to DPAA2 network drivers. It also introduces new fwnode based APIs to support phylink and phy layers Following functions are defined: phylink_fwnode_phy_connect() fwnode_mdiobus_register_phy() fwnode_mdiobus_register() fwnode_get_phy_id() fwnode_phy_find_device() device_phy_find_device() fwnode_get_phy_node() fwnode_mdio_find_device() fwnode_get_id() First one helps in connecting phy to phylink instance. Next three helps in getting phy_id and registering phy to mdiobus Next two help in finding a phy on a mdiobus. Next one helps in getting phy_node from a fwnode. Last one is used to get fwnode ID. Corresponding OF functions are refactored. END Changes in v3: - Add more info on legacy DT properties "phy" and "phy-device" - Redefine fwnode_phy_find_device() to follow of_phy_find_device() - Use traditional comparison pattern - Use GENMASK - Modified to retrieve reg property value for ACPI as well - Resolved compilation issue with CONFIG_ACPI = n - Added more info into documentation - Use acpi_mdiobus_register() - Avoid unnecessary line removal - Remove unused inclusion of acpi.h Changes in v2: - Updated with more description in document - use reverse christmas tree ordering for local variables - Refactor OF functions to use fwnode functions Calvin Johnson (15): Documentation: ACPI: DSD: Document MDIO PHY net: phy: Introduce fwnode_mdio_find_device() net: phy: Introduce phy related fwnode functions of: mdio: Refactor of_phy_find_device() net: phy: Introduce fwnode_get_phy_id() of: mdio: Refactor of_get_phy_id() net: mdiobus: Introduce fwnode_mdiobus_register_phy() of: mdio: Refactor of_mdiobus_register_phy() device property: Introduce fwnode_get_id() net: mdio: Add ACPI support code for mdio net: mdiobus: Introduce fwnode_mdiobus_register() net/fsl: Use fwnode_mdiobus_register() phylink: introduce phylink_fwnode_phy_connect() net: phylink: Refactor phylink_of_phy_connect() net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver Documentation/firmware-guide/acpi/dsd/phy.rst | 129 ++++++++++++++++++ MAINTAINERS | 1 + drivers/base/property.c | 33 +++++ .../net/ethernet/freescale/dpaa2/dpaa2-mac.c | 87 +++++++----- drivers/net/ethernet/freescale/xgmac_mdio.c | 12 +- drivers/net/mdio/Kconfig | 7 + drivers/net/mdio/Makefile | 1 + drivers/net/mdio/acpi_mdio.c | 49 +++++++ drivers/net/mdio/of_mdio.c | 79 +---------- drivers/net/phy/mdio_bus.c | 87 ++++++++++++ drivers/net/phy/phy_device.c | 106 ++++++++++++++ drivers/net/phy/phylink.c | 49 ++++--- include/linux/acpi_mdio.h | 27 ++++ include/linux/mdio.h | 2 + include/linux/of_mdio.h | 6 +- include/linux/phy.h | 32 +++++ include/linux/phylink.h | 3 + include/linux/property.h | 1 + 18 files changed, 579 insertions(+), 132 deletions(-) create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst create mode 100644 drivers/net/mdio/acpi_mdio.c create mode 100644 include/linux/acpi_mdio.h