Received-SPF: pass (google.com: domain of linux-kernel+bounces-203523-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Thu, 6 Jun 2024 04:10:40 +0200
From: Andrew Lunn <andrew@lunn.ch>
To: Kamil =?iso-8859-1?Q?Hor=E1k?= - 2N <kamilh@axis.com>
Cc: florian.fainelli@broadcom.com, bcm-kernel-feedback-list@broadcom.com,
	hkallweit1@gmail.com, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5 3/3] net: phy: bcm-phy-lib: Implement BroadR-Reach
 link modes
Message-ID: <7a313ff6-574b-425d-be06-23bb588402c8@lunn.ch>
References: <20240605095646.3924454-1-kamilh@axis.com>
 <20240605095646.3924454-4-kamilh@axis.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240605095646.3924454-4-kamilh@axis.com>

On Wed, Jun 05, 2024 at 11:56:46AM +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.
> 
> Change-Id: I592d261bc0d60aaa78fc1717a315b0b1c1449c81
> Signed-off-by: Kamil Hor?k - 2N <kamilh@axis.com>
> ---
>  drivers/net/phy/bcm-phy-lib.c | 123 ++++++++++++
>  drivers/net/phy/bcm-phy-lib.h |   4 +
>  drivers/net/phy/broadcom.c    | 368 ++++++++++++++++++++++++++++++++--
>  drivers/net/phy/phy-core.c    |   2 +-
>  include/linux/brcmphy.h       |   9 +
>  5 files changed, 488 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
> index 876f28fd8256..cc1b5e5a958c 100644
> --- a/drivers/net/phy/bcm-phy-lib.c
> +++ b/drivers/net/phy/bcm-phy-lib.c
> @@ -794,6 +794,47 @@ static int _bcm_phy_cable_test_get_status(struct phy_device *phydev,
>  	return ret;
>  }
>  
> +static int bcm_setup_forced(struct phy_device *phydev)
> +{
> +	u16 ctl = 0;
> +
> +	phydev->pause = 0;
> +	phydev->asym_pause = 0;
> +
> +	if (phydev->speed == SPEED_100)
> +		ctl |= LRECR_SPEED100;
> +
> +	if (phydev->duplex != DUPLEX_FULL)
> +		return -EOPNOTSUPP;
> +
> +	return phy_modify(phydev, MII_BCM54XX_LRECR, LRECR_SPEED100, ctl);

We should consider naming here. I assume this will not work for IEEE
forced mode? So maybe this should have _lre_ or _brr_ in the name to
make it clear what it actually does.

> +}


> +
> +/**
> + * bcm_linkmode_adv_to_mii_adv_t
> + * @advertising: the linkmode advertisement settings
> + * @return: LDS Auto-Negotiation Advertised Ability register value
> + *
> + * A small helper function that translates linkmode advertisement
> + * settings to phy autonegotiation advertisements for the
> + * MII_BCM54XX_LREANAA register of Broadcom PHYs capable of LDS
> + */
> +static u32 bcm_linkmode_adv_to_mii_adv_t(unsigned long *advertising)
> +{
> +	u32 result = 0;

Make here, and maybe lre_advertising?

Please go through all the functions and think about naming.

    Andrew

---
pw-bot: cr