On Mon, 17 Jun 2024 13:38:41 +0200 Kamil Horák - 2N wrote:
> Implement single-pair BroadR-Reach modes on bcm5481x PHY by Broadcom.
> Create set of functions alternative to IEEE 802.3 to handle configuration
> of these modes on compatible Broadcom PHYs.
Some nit picks below, but please don't repost until next week.
Sorry for the delay but it's vacation season, I think some of
the key folks are currently AFK :(
> + * bcm_config_lre_advert - sanitize and advertise Long-Distance Signaling
> + * auto-negotiation parameters
> + * @phydev: target phy_device struct
> + * @return: 0 if the PHY's advertisement hasn't changed, < 0 on error,
> + * > 0 if it has changed
* Return: 0 if the PHY
no @ and after the description
> + *
> + * Description: Writes MII_BCM54XX_LREANAA with the appropriate values,
Please don't prefix the description with the word "description"..
> + * after sanitizing the values to make sure we only advertise
> + * what is supported.
> + */
> +int bcm_config_lre_advert(struct phy_device *phydev)
> +{
> + int err;
> + u32 adv;
> +
> + /* Only allow advertising what this PHY supports */
> + linkmode_and(phydev->advertising, phydev->advertising,
> + phydev->supported);
> +
> + adv = bcm_linkmode_adv_to_lre_adv_t(phydev->advertising);
> +
> + /* Setup BroadR-Reach mode advertisement */
> + err = phy_modify_changed(phydev, MII_BCM54XX_LREANAA,
> + LRE_ADVERTISE_ALL | LREANAA_PAUSE |
> + LREANAA_PAUSE_ASYM, adv);
> +
> + if (err < 0)
> + return err;
> +
> + return err;
You can return phy_modify_changed(... directly, no need for err
> +}
> +EXPORT_SYMBOL_GPL(bcm_config_lre_advert);
> +
> MODULE_DESCRIPTION("Broadcom PHY Library");
> MODULE_LICENSE("GPL v2");
> MODULE_AUTHOR("Broadcom Corporation");
> diff --git a/drivers/net/phy/bcm-phy-lib.h b/drivers/net/phy/bcm-phy-lib.h
> index b52189e45a84..fecdd66ad736 100644
> --- a/drivers/net/phy/bcm-phy-lib.h
> +++ b/drivers/net/phy/bcm-phy-lib.h
> @@ -121,4 +121,8 @@ irqreturn_t bcm_phy_wol_isr(int irq, void *dev_id);
> int bcm_phy_led_brightness_set(struct phy_device *phydev,
> u8 index, enum led_brightness value);
>
> +int bcm_setup_master_slave(struct phy_device *phydev);
> +int bcm_config_lre_aneg(struct phy_device *phydev, bool changed);
> +int bcm_config_lre_advert(struct phy_device *phydev);
> +
> #endif /* _LINUX_BCM_PHY_LIB_H */
> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
> index 370e4ed45098..5e590c8f82c4 100644
> --- a/drivers/net/phy/broadcom.c
> +++ b/drivers/net/phy/broadcom.c
> @@ -5,6 +5,9 @@
> * Broadcom BCM5411, BCM5421 and BCM5461 Gigabit Ethernet
> * transceivers.
> *
> + * Broadcom BCM54810, BCM54811 BroadR-Reach transceivers.
> + *
> + *
double new line
> * Copyright (c) 2006 Maciej W. Rozycki
> *
> * Inspired by code written by Amy Fong.
> @@ -553,18 +556,97 @@ static int bcm54810_write_mmd(struct phy_device *phydev, int devnum, u16 regnum,
> return -EOPNOTSUPP;
> }
>
> -static int bcm54811_config_init(struct phy_device *phydev)
> +static int bcm5481x_get_brrmode(struct phy_device *phydev, u8 *data)
> {
> - int err, reg;
> + int reg;
>
> - /* Disable BroadR-Reach function. */
> reg = bcm_phy_read_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL);
> - reg &= ~BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN;
> - err = bcm_phy_write_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL,
> - reg);
> - if (err < 0)
> +
> + *data = (reg & BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN) ? 1 : 0;
> +
> + return 0;
> +}
> +
> +static int bcm54811_read_abilities(struct phy_device *phydev)
> +{
> + static const int modes_array[] = { ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
> + ETHTOOL_LINK_MODE_10baseT1BRR_Full_BIT,
> + ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> + ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
> + ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
> + ETHTOOL_LINK_MODE_100baseT_Full_BIT,
> + ETHTOOL_LINK_MODE_100baseT_Half_BIT,
> + ETHTOOL_LINK_MODE_10baseT_Full_BIT,
> + ETHTOOL_LINK_MODE_10baseT_Half_BIT };
This is more normal formatting for the kernel:
static const int modes_array[] = {
ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
ETHTOOL_LINK_MODE_10baseT1BRR_Full_BIT,
please try to avoid going over 80 characters
> +static int bcm54811_config_init(struct phy_device *phydev)
> +{
> + int err, reg;
> + bool brr = false;
> + struct device_node *np = phydev->mdio.dev.of_node;
order variable declaration lines longest to shortest,
AKA reverse xmas tree
