| Message ID | 20220825130211.3730461-1-o.rempel@pengutronix.de |
|---|---|
| Headers | show |
| Series | add generic PSE support | expand |
On Thu, Aug 25, 2022 at 03:02:09PM +0200, Oleksij Rempel wrote: > +static struct pse_control * > +fwnode_find_pse_control(struct fwnode_handle *fwnode) > +{ > + struct pse_control *psec; > + struct device_node *np; > + > + if (is_acpi_node(fwnode)) > + return NULL; > + > + np = to_of_node(fwnode); > + if (!np) > + return NULL; Doesn't to_of_node() confirm that the fwnode is a DT node? In other words, isn't the "is_acpi_node()" entirely redundant? > + > + psec = of_pse_control_get(np); > + if (IS_ERR_OR_NULL(psec)) > + return NULL; > + > + return psec; > +} So fwnode_find_pse_control() returns NULL on error. > + psec = fwnode_find_pse_control(child); > + if (IS_ERR(psec)) > + return PTR_ERR(psec); This usage expects it to return an error-pointer. Clearly, there is some disagreement about what fwnode_find_pse_control() returns on error.
On Thu, 25 Aug 2022 15:02:10 +0200 Oleksij Rempel wrote: > +void ethtool_set_ethtool_pse_ops(const struct ethtool_pse_ops *ops) > +{ > + rtnl_lock(); > + ethtool_pse_ops = ops; > + rtnl_unlock(); > +} > +EXPORT_SYMBOL_GPL(ethtool_set_ethtool_pse_ops); Do we really need the loose linking on the PSE ops? It's not a lot of code, and the pcdev->ops should be enough to decouple drivers, it seems. > +static int pse_set_pse_config(struct net_device *dev, > + struct netlink_ext_ack *extack, > + struct nlattr **tb) > +{ > + struct phy_device *phydev = dev->phydev; > + struct pse_control_config config = {}; > + const struct ethtool_pse_ops *ops; > + int ret; > + > + if (!tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL]) > + return 0; If SET has no useful attrs the usual response is -EINVAL. > + ops = ethtool_pse_ops; > + if (!ops || !ops->set_config) > + return -EOPNOTSUPP; > + > + config.admin_cotrol = nla_get_u8(tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL]); > + > + if (!phydev) > + return -EOPNOTSUPP; > + > + // todo resolve phydev dependecy My lack of phydev understanding and laziness are likely the cause, but I haven't found an explanation for this todo. What is it about? > + if (!phydev->psec) > + ret = -EOPNOTSUPP; > + else > + ret = ops->set_config(phydev->psec, extack, &config); > + > + return ret; > +}
On Thu, 25 Aug 2022 15:02:10 +0200 Oleksij Rempel wrote: > +enum ethtool_podl_pse_admin_state { > + ETHTOOL_PODL_PSE_ADMIN_STATE_UNKNOWN = 1, Why define UNKNOWN.. as 1? No real objection here, just in my head somehow UNKNOWN = 0 or just start from 1. > + ETHTOOL_PODL_PSE_ADMIN_STATE_DISABLED, > + ETHTOOL_PODL_PSE_ADMIN_STATE_ENABLED, > + > + /* add new constants above here */ > + ETHTOOL_PODL_PSE_ADMIN_STATE_COUNT Why define count for a value enum like this? For attrs we define it because it's used to size tables, don't think anyone will size tables based on states. There's a bunch of kdoc warnings in the patches as well.
On Thu, Aug 25, 2022 at 11:10:19AM -0700, Jakub Kicinski wrote: > On Thu, 25 Aug 2022 15:02:10 +0200 Oleksij Rempel wrote: > > +enum ethtool_podl_pse_admin_state { > > + ETHTOOL_PODL_PSE_ADMIN_STATE_UNKNOWN = 1, > > Why define UNKNOWN.. as 1? No real objection here, just in my head > somehow UNKNOWN = 0 or just start from 1. I need to keep difference between not supported functionality and supported but unknown. > > + ETHTOOL_PODL_PSE_ADMIN_STATE_DISABLED, > > + ETHTOOL_PODL_PSE_ADMIN_STATE_ENABLED, > > + > > + /* add new constants above here */ > > + ETHTOOL_PODL_PSE_ADMIN_STATE_COUNT > > Why define count for a value enum like this? For attrs we define it > because it's used to size tables, don't think anyone will size tables > based on states. ok, i'll remove it. > There's a bunch of kdoc warnings in the patches as well. ok.
On Thu, Aug 25, 2022 at 11:07:56AM -0700, Jakub Kicinski wrote: > On Thu, 25 Aug 2022 15:02:10 +0200 Oleksij Rempel wrote: > > +void ethtool_set_ethtool_pse_ops(const struct ethtool_pse_ops *ops) > > +{ > > + rtnl_lock(); > > + ethtool_pse_ops = ops; > > + rtnl_unlock(); > > +} > > +EXPORT_SYMBOL_GPL(ethtool_set_ethtool_pse_ops); > > Do we really need the loose linking on the PSE ops? > It's not a lot of code, and the pcdev->ops should be > enough to decouple drivers, it seems. Right now i have no good idea how to properly decouple pse-pd from phydev. @Andrew, should i care about it on this stage or it is currently not a big deal? > > +static int pse_set_pse_config(struct net_device *dev, > > + struct netlink_ext_ack *extack, > > + struct nlattr **tb) > > +{ > > + struct phy_device *phydev = dev->phydev; > > + struct pse_control_config config = {}; > > + const struct ethtool_pse_ops *ops; > > + int ret; > > + > > + if (!tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL]) > > + return 0; > > If SET has no useful attrs the usual response is -EINVAL. ack > > + ops = ethtool_pse_ops; > > + if (!ops || !ops->set_config) > > + return -EOPNOTSUPP; > > + > > + config.admin_cotrol = nla_get_u8(tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL]); > > + > > + if (!phydev) > > + return -EOPNOTSUPP; > > + > > + // todo resolve phydev dependecy > > My lack of phydev understanding and laziness are likely the cause, > but I haven't found an explanation for this todo. What is it about? sorry. old artifact, will be removed. It is part of phydev/phylink related discussion in the last patch version.