Message ID | 1386283936-26104-6-git-send-email-florian@openwrt.org |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
2013/12/5 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>: >> >> + /* Set phydev->supported based on the "max-speed" property >> + * if present */ > > > Preferred multi-line comment style is this: > > /* > * bla > * bla > */ All other comments in this file are: /* bla * bla */ which is why I used the same style.
On Thu, 2013-12-05 at 15:53 -0800, Florian Fainelli wrote: > 2013/12/5 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>: > >> > >> + /* Set phydev->supported based on the "max-speed" property > >> + * if present */ [] > All other comments in this file are: > > /* bla > * bla > */ > > which is why I used the same style. Kind of. This should be: /* Set phydev->supported based on the "max-speed" property * if present */ Or if you want to use all 80 columns: /* Set phydev->supported based on the "max-speed" property if present */ or maybe /* Set phydev->supported using the "max-speed" property (if present) */ -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Florian Fainelli <f.fainelli@gmail.com> Date: Thu, 5 Dec 2013 15:53:41 -0800 > 2013/12/5 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>: >>> >>> + /* Set phydev->supported based on the "max-speed" property >>> + * if present */ >> >> >> Preferred multi-line comment style is this: >> >> /* >> * bla >> * bla >> */ > > All other comments in this file are: > > /* bla > * bla > */ > > which is why I used the same style. And this is the preferred format for networking, as documented in Documentation/networking/netdev-FAQ.txt -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 12/06/2013 01:52 AM, Florian Fainelli wrote: > From: Florian Fainelli <f.fainelli@gmail.com> > The "max-speed" property is defined per the ePAPR specification to > express the maximum speed a PHY supports. Use that property, if present > to set the phydev->supported features which properly restricts the PHY > within the range of defined speeds. > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > drivers/of/of_mdio.c | 25 ++++++++++++++++++++++++- > 1 file changed, 24 insertions(+), 1 deletion(-) > diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c > index 4923ab2..e1e19e5 100644 > --- a/drivers/of/of_mdio.c > +++ b/drivers/of/of_mdio.c [...] > @@ -58,8 +76,13 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi > return 1; > } > > + /* Set phydev->supported based on the "max-speed" property > + * if present */ Preferred multi-line comment style is this: /* * bla * bla */ WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/05/2013 04:52 PM, Florian Fainelli wrote: > From: Florian Fainelli <f.fainelli@gmail.com> > > The "max-speed" property is defined per the ePAPR specification to > express the maximum speed a PHY supports. Use that property, if present > to set the phydev->supported features which properly restricts the PHY > within the range of defined speeds. > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > drivers/of/of_mdio.c | 25 ++++++++++++++++++++++++- > 1 file changed, 24 insertions(+), 1 deletion(-) > > diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c > index 4923ab2..e1e19e5 100644 > --- a/drivers/of/of_mdio.c > +++ b/drivers/of/of_mdio.c > @@ -22,12 +22,30 @@ > MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>"); > MODULE_LICENSE("GPL"); > > +static void of_set_phy_supported(struct phy_device *phydev, u32 max_speed) > +{ > + phydev->supported |= PHY_DEFAULT_FEATURES; > + > + switch (max_speed) { > + default: > + return; > + > + case SPEED_1000: > + phydev->supported |= PHY_1000BT_FEATURES; This assumes the speed is not already set. Do you need to first mask speeds out? No need to support 10G PHYs? A fall-thru note would be helpful here. > + case SPEED_100: > + phydev->supported |= PHY_100BT_FEATURES; > + case SPEED_10: > + phydev->supported |= PHY_10BT_FEATURES; > + } > +} > + > static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *child, > u32 addr) > { > struct phy_device *phy; > bool is_c45; > int rc, prev_irq; > + u32 max_speed = 0; > > is_c45 = of_device_is_compatible(child, > "ethernet-phy-ieee802.3-c45"); > @@ -58,8 +76,13 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi > return 1; > } > > + /* Set phydev->supported based on the "max-speed" property > + * if present */ > + if (!of_property_read_u32(child, "max-speed", &max_speed)) > + of_set_phy_supported(phy, max_speed); > + > dev_dbg(&mdio->dev, "registered phy %s at address %i\n", > - child->name, addr); > + child->name, addr); > > return 0; > } > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 5 Dec 2013 14:52:14 -0800, Florian Fainelli <florian@openwrt.org> wrote: > From: Florian Fainelli <f.fainelli@gmail.com> > > The "max-speed" property is defined per the ePAPR specification to > express the maximum speed a PHY supports. Use that property, if present > to set the phydev->supported features which properly restricts the PHY > within the range of defined speeds. > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > drivers/of/of_mdio.c | 25 ++++++++++++++++++++++++- > 1 file changed, 24 insertions(+), 1 deletion(-) > > diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c > index 4923ab2..e1e19e5 100644 > --- a/drivers/of/of_mdio.c > +++ b/drivers/of/of_mdio.c > @@ -22,12 +22,30 @@ > MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>"); > MODULE_LICENSE("GPL"); > > +static void of_set_phy_supported(struct phy_device *phydev, u32 max_speed) > +{ > + phydev->supported |= PHY_DEFAULT_FEATURES; > + > + switch (max_speed) { > + default: > + return; No need for a default clause. > + > + case SPEED_1000: > + phydev->supported |= PHY_1000BT_FEATURES; > + case SPEED_100: > + phydev->supported |= PHY_100BT_FEATURES; > + case SPEED_10: > + phydev->supported |= PHY_10BT_FEATURES; > + } > +} > + > static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *child, > u32 addr) > { > struct phy_device *phy; > bool is_c45; > int rc, prev_irq; > + u32 max_speed = 0; > > is_c45 = of_device_is_compatible(child, > "ethernet-phy-ieee802.3-c45"); > @@ -58,8 +76,13 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi > return 1; > } > > + /* Set phydev->supported based on the "max-speed" property > + * if present */ > + if (!of_property_read_u32(child, "max-speed", &max_speed)) > + of_set_phy_supported(phy, max_speed); > + Why the separate function? There is no need since there is only one caller and the block is very short. Just do this: if (!of_property_read_u32(child, "max-speed", &max_speed)) { phydev->supported |= PHY_DEFAULT_FEATURES; switch (max_speed) { case SPEED_1000: phydev->supported |= PHY_1000BT_FEATURES; case SPEED_100: phydev->supported |= PHY_100BT_FEATURES; case SPEED_10: phydev->supported |= PHY_10BT_FEATURES; } } > dev_dbg(&mdio->dev, "registered phy %s at address %i\n", > - child->name, addr); > + child->name, addr); Unrelated whitespace change. I wouldn't even bother with changing this. It adds noise. However, those are style concerns. The content looks fine. Acked-by: Grant Likely <grant.likely@linaro.org> g. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c index 4923ab2..e1e19e5 100644 --- a/drivers/of/of_mdio.c +++ b/drivers/of/of_mdio.c @@ -22,12 +22,30 @@ MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>"); MODULE_LICENSE("GPL"); +static void of_set_phy_supported(struct phy_device *phydev, u32 max_speed) +{ + phydev->supported |= PHY_DEFAULT_FEATURES; + + switch (max_speed) { + default: + return; + + case SPEED_1000: + phydev->supported |= PHY_1000BT_FEATURES; + case SPEED_100: + phydev->supported |= PHY_100BT_FEATURES; + case SPEED_10: + phydev->supported |= PHY_10BT_FEATURES; + } +} + static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *child, u32 addr) { struct phy_device *phy; bool is_c45; int rc, prev_irq; + u32 max_speed = 0; is_c45 = of_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"); @@ -58,8 +76,13 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi return 1; } + /* Set phydev->supported based on the "max-speed" property + * if present */ + if (!of_property_read_u32(child, "max-speed", &max_speed)) + of_set_phy_supported(phy, max_speed); + dev_dbg(&mdio->dev, "registered phy %s at address %i\n", - child->name, addr); + child->name, addr); return 0; }