| Message ID | E1thR9g-003vX6-4s@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next,v3,1/3] net: phylink: provide phylink_mac_implements_lpi() | expand |
On Mon, Feb 10, 2025 at 10:36:44AM +0000, Russell King (Oracle) wrote: > diff --git a/include/linux/phylink.h b/include/linux/phylink.h > index 898b00451bbf..0de78673172d 100644 > --- a/include/linux/phylink.h > +++ b/include/linux/phylink.h > @@ -737,6 +737,18 @@ static inline int phylink_get_link_timer_ns(phy_interface_t interface) > } > } > > +/** > + * phylink_mac_implements_lpi() - determine if MAC implements LPI ops > + * @ops: phylink_mac_ops structure > + * > + * Returns true if the phylink MAC operations structure indicates that the > + * LPI operations have been implemented, false otherwise. This is something that I only noticed for v3 because I wanted to leave a review tag, so I first checked the status in patchwork, but there it says: include/linux/phylink.h:749: warning: No description found for return value of 'phylink_mac_implements_lpi' I am aware of this conversation from November where you raised the point about tooling being able to accept the syntax without the colon as well: https://lore.kernel.org/netdev/87v7wjffo6.fsf@trenco.lwn.net/ but it looks like it didn't go anywhere, with Jon still preferring the strict syntax for now, and no follow-up that I can see. So, the current conventions are not these, and you haven't specifically said anywhere that you are deliberately ignoring them. In the end it's not something for me to decide, but I thought maybe I'm speeding things up a little bit by opening up the discussion now, rather than wait for a maintainer to look at it. > + */ > +static inline bool phylink_mac_implements_lpi(const struct phylink_mac_ops *ops) > +{ > + return ops && ops->mac_disable_tx_lpi && ops->mac_enable_tx_lpi; > +} > + > void phylink_mii_c22_pcs_decode_state(struct phylink_link_state *state, > unsigned int neg_mode, u16 bmsr, u16 lpa); > void phylink_mii_c22_pcs_get_state(struct mdio_device *pcs,
On Mon, Feb 10, 2025 at 03:20:54PM +0200, Vladimir Oltean wrote: > On Mon, Feb 10, 2025 at 10:36:44AM +0000, Russell King (Oracle) wrote: > > diff --git a/include/linux/phylink.h b/include/linux/phylink.h > > index 898b00451bbf..0de78673172d 100644 > > --- a/include/linux/phylink.h > > +++ b/include/linux/phylink.h > > @@ -737,6 +737,18 @@ static inline int phylink_get_link_timer_ns(phy_interface_t interface) > > } > > } > > > > +/** > > + * phylink_mac_implements_lpi() - determine if MAC implements LPI ops > > + * @ops: phylink_mac_ops structure > > + * > > + * Returns true if the phylink MAC operations structure indicates that the > > + * LPI operations have been implemented, false otherwise. > > This is something that I only noticed for v3 because I wanted to leave a > review tag, so I first checked the status in patchwork, but there it says: > > include/linux/phylink.h:749: warning: No description found for return value of 'phylink_mac_implements_lpi' > > I am aware of this conversation from November where you raised the point > about tooling being able to accept the syntax without the colon as well: > https://lore.kernel.org/netdev/87v7wjffo6.fsf@trenco.lwn.net/ > > but it looks like it didn't go anywhere, with Jon still preferring the > strict syntax for now, and no follow-up that I can see. So, the current > conventions are not these, and you haven't specifically said anywhere > that you are deliberately ignoring them. It was explained in this email as part of that thread: https://lore.kernel.org/netdev/ZzjHH-L-ylLe0YhU@shell.armlinux.org.uk/ The reason is that it goes against natural grammar. The only time that "Returns:" would make sense in grammar is when listing with e.g. a bulleted list, where the part before the colon doesn't have to be a complete sentence. This is why it's going to be an uphill battle - grammatically it is wrong, and thus it doesn't flow when thinking about documenting the return value. If we want to go to a bulleted list, then it will be natural to add the colon. I'm not going to explain to this level of detail in every email, and because of the grammatical nature of this, it's going to be very difficult to use a form that goes against proper grammar.
On Mon, Feb 10, 2025 at 02:05:58PM +0000, Russell King (Oracle) wrote: > On Mon, Feb 10, 2025 at 03:20:54PM +0200, Vladimir Oltean wrote: > > On Mon, Feb 10, 2025 at 10:36:44AM +0000, Russell King (Oracle) wrote: > > > diff --git a/include/linux/phylink.h b/include/linux/phylink.h > > > index 898b00451bbf..0de78673172d 100644 > > > --- a/include/linux/phylink.h > > > +++ b/include/linux/phylink.h > > > @@ -737,6 +737,18 @@ static inline int phylink_get_link_timer_ns(phy_interface_t interface) > > > } > > > } > > > > > > +/** > > > + * phylink_mac_implements_lpi() - determine if MAC implements LPI ops > > > + * @ops: phylink_mac_ops structure > > > + * > > > + * Returns true if the phylink MAC operations structure indicates that the > > > + * LPI operations have been implemented, false otherwise. > > > > This is something that I only noticed for v3 because I wanted to leave a > > review tag, so I first checked the status in patchwork, but there it says: > > > > include/linux/phylink.h:749: warning: No description found for return value of 'phylink_mac_implements_lpi' > > > > I am aware of this conversation from November where you raised the point > > about tooling being able to accept the syntax without the colon as well: > > https://lore.kernel.org/netdev/87v7wjffo6.fsf@trenco.lwn.net/ > > > > but it looks like it didn't go anywhere, with Jon still preferring the > > strict syntax for now, and no follow-up that I can see. So, the current > > conventions are not these, and you haven't specifically said anywhere > > that you are deliberately ignoring them. > > It was explained in this email as part of that thread: > > https://lore.kernel.org/netdev/ZzjHH-L-ylLe0YhU@shell.armlinux.org.uk/ > > The reason is that it goes against natural grammar. The only time that > "Returns:" would make sense in grammar is when listing with e.g. a > bulleted list, where the part before the colon doesn't have to be a > complete sentence. > > This is why it's going to be an uphill battle - grammatically it is > wrong, and thus it doesn't flow when thinking about documenting the > return value. > > If we want to go to a bulleted list, then it will be natural to add > the colon. > > I'm not going to explain to this level of detail in every email, and > because of the grammatical nature of this, it's going to be very > difficult to use a form that goes against proper grammar. Also note that it follows the style already present in that file with one exception (which is one of the few cases I remembered to use the new format.)
Hello: This series was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Mon, 10 Feb 2025 10:36:44 +0000 you wrote: > Provide a helper to determine whether the MAC operations structure > implements the LPI operations, which will be used by both phylink and > DSA. > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> > --- > drivers/net/phy/phylink.c | 3 +-- > include/linux/phylink.h | 12 ++++++++++++ > 2 files changed, 13 insertions(+), 2 deletions(-) Here is the summary with links: - [net-next,v3,1/3] net: phylink: provide phylink_mac_implements_lpi() https://git.kernel.org/netdev/net-next/c/2001d21592e5 - [net-next,v3,2/3] net: dsa: allow use of phylink managed EEE support https://git.kernel.org/netdev/net-next/c/b8927bd44f78 - [net-next,v3,3/3] net: dsa: mt7530: convert to phylink managed EEE https://git.kernel.org/netdev/net-next/c/9cf21773f535 You are awesome, thank you!
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 214b62fba991..6fbb5fd5b400 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -1957,8 +1957,7 @@ struct phylink *phylink_create(struct phylink_config *config, return ERR_PTR(-EINVAL); } - pl->mac_supports_eee_ops = mac_ops->mac_disable_tx_lpi && - mac_ops->mac_enable_tx_lpi; + pl->mac_supports_eee_ops = phylink_mac_implements_lpi(mac_ops); pl->mac_supports_eee = pl->mac_supports_eee_ops && pl->config->lpi_capabilities && !phy_interface_empty(pl->config->lpi_interfaces); diff --git a/include/linux/phylink.h b/include/linux/phylink.h index 898b00451bbf..0de78673172d 100644 --- a/include/linux/phylink.h +++ b/include/linux/phylink.h @@ -737,6 +737,18 @@ static inline int phylink_get_link_timer_ns(phy_interface_t interface) } } +/** + * phylink_mac_implements_lpi() - determine if MAC implements LPI ops + * @ops: phylink_mac_ops structure + * + * Returns true if the phylink MAC operations structure indicates that the + * LPI operations have been implemented, false otherwise. + */ +static inline bool phylink_mac_implements_lpi(const struct phylink_mac_ops *ops) +{ + return ops && ops->mac_disable_tx_lpi && ops->mac_enable_tx_lpi; +} + void phylink_mii_c22_pcs_decode_state(struct phylink_link_state *state, unsigned int neg_mode, u16 bmsr, u16 lpa); void phylink_mii_c22_pcs_get_state(struct mdio_device *pcs,
Provide a helper to determine whether the MAC operations structure implements the LPI operations, which will be used by both phylink and DSA. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/phy/phylink.c | 3 +-- include/linux/phylink.h | 12 ++++++++++++ 2 files changed, 13 insertions(+), 2 deletions(-)