Messages in this thread | | | Date | Wed, 8 May 2024 08:39:52 +0100 | From | Simon Horman <> | Subject | Re: [PATCH v3 3/3] net: phy: bcm-phy-lib: Implement BroadR-Reach link modes |
| |
On Mon, May 06, 2024 at 04:40:15PM +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. > > Signed-off-by: Kamil Horák - 2N <kamilh@axis.com>
Hi Kamil,
Some minor feedback from my side.
..
> diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
..
> +/** > + * bcm_linkmode_adv_to_mii_adv_t > + * @advertising: the linkmode advertisement settings > + * > + * A small helper function that translates linkmode advertisement > + * settings to phy autonegotiation advertisements for the > + * MII_BCM54XX_LREANAA register.
Please consider including a Return: section in the Kernel doc.
Flagged by ./scripts/kernel-doc -Wall -none
> + */ > +static inline u32 bcm_linkmode_adv_to_mii_adv_t(unsigned long *advertising)
..
> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
..
> +static int bcm_read_master_slave(struct phy_device *phydev) > +{ > + int cfg, state; > + int val; > + > + /* In BroadR-Reach mode we are always capable of master-slave > + * and there is no preferred master or slave configuration > + */ > + phydev->master_slave_get = MASTER_SLAVE_CFG_UNKNOWN; > + phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN; > + > + val = phy_read(phydev, MII_BCM54XX_LRECR); > + if (val < 0) > + return val; > + > + if ((val & LRECR_LDSEN) == 0) { > + if (val & LRECR_MASTER) > + cfg = MASTER_SLAVE_CFG_MASTER_FORCE; > + else > + cfg = MASTER_SLAVE_CFG_SLAVE_FORCE; > + } > + > + val = phy_read(phydev, MII_BCM54XX_LRELDSE); > + if (val < 0) > + return val; > + > + if (val & LDSE_MASTER) > + state = MASTER_SLAVE_STATE_MASTER; > + else > + state = MASTER_SLAVE_STATE_SLAVE; > + > + phydev->master_slave_get = cfg;
Perhaps it is not possible, but it appears that if the ((val & LRECR_LDSEN) == 0) condition above is not met then cfg is used uninitialised here.
Flagged by Smatch.
> + phydev->master_slave_state = state; > + > + return 0; > +}
..
| |