Re: [PATCH v4 net-next 4/5] drivers/net/phy: add helpers to get/set PLCA configuration

Piergiorgio Beruto
2022-12-06T18:08:00Z

On Tue, Dec 06, 2022 at 02:59:39PM +0100, Andrew Lunn wrote:
> > +/* MDIO Manageable Devices (MMDs). */
> > +#define MDIO_MMD_OATC14 MDIO_MMD_VEND2
>
> As i said in a comment somewhere, i would prefer you use
> MDIO_MMD_VEND2, not MDIO_MMD_OATC14. We want the gentle reminder that
> these registers can contain anything the vendor wants, including, but
> not limited to, PLCA.
Ok, I'll fix that.

> > +/* Open Alliance TC14 PLCA CTRL0 register */
> > +#define MDIO_OATC14_PLCA_EN 0x8000 /* PLCA enable */
> > +#define MDIO_OATC14_PLCA_RST 0x4000 /* PLCA reset */
>
> These are bits, so use the BIT macro. When this was part of mii.h,
> that file used this hex format so it made sense to follow that
> format. Now you are in a few file, you should use the macro.
Sure, I just moved the code in the new file without thinking of changing
this. Will do.

> > +/* Open Alliance TC14 PLCA CTRL1 register */
> > +#define MDIO_OATC14_PLCA_NCNT 0xff00 /* PLCA node count */
> > +#define MDIO_OATC14_PLCA_ID 0x00ff /* PLCA local node ID */
> > +
> > +/* Open Alliance TC14 PLCA STATUS register */
> > +#define MDIO_OATC14_PLCA_PST 0x8000 /* PLCA status indication */
> > +
> > +/* Open Alliance TC14 PLCA TOTMR register */
> > +#define MDIO_OATC14_PLCA_TOT 0x00ff
> > +
> > +/* Open Alliance TC14 PLCA BURST register */
> > +#define MDIO_OATC14_PLCA_MAXBC 0xff00
> > +#define MDIO_OATC14_PLCA_BTMR 0x00ff
> > +
> > +#endif /* __MDIO_OPEN_ALLIANCE__ */
> > diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> > index a87a4b3ffce4..dace5d3b29ad 100644
> > --- a/drivers/net/phy/phy-c45.c
> > +++ b/drivers/net/phy/phy-c45.c
> > @@ -8,6 +8,8 @@
> > #include <linux/mii.h>
> > #include <linux/phy.h>
> >
> > +#include "mdio-open-alliance.h"
> > +
> > /**
> > * genphy_c45_baset1_able - checks if the PMA has BASE-T1 extended abilities
> > * @phydev: target phy_device struct
> > @@ -931,6 +933,184 @@ int genphy_c45_fast_retrain(struct phy_device *phydev, bool enable)
> > }
> > EXPORT_SYMBOL_GPL(genphy_c45_fast_retrain);
> >
> > +/**
> > + * genphy_c45_plca_get_cfg - get PLCA configuration from standard registers
> > + * @phydev: target phy_device struct
> > + * @plca_cfg: output structure to store the PLCA configuration
> > + *
> > + * Description: if the PHY complies to the Open Alliance TC14 10BASE-T1S PLCA
> > + * Management Registers specifications, this function can be used to retrieve
> > + * the current PLCA configuration from the standard registers in MMD 31.
> > + */
> > +int genphy_c45_plca_get_cfg(struct phy_device *phydev,
> > + struct phy_plca_cfg *plca_cfg)
> > +{
> > + int ret;
> > +
> > + ret = phy_read_mmd(phydev, MDIO_MMD_OATC14, MDIO_OATC14_PLCA_IDVER);
> > + if (ret < 0)
> > + return ret;
> > +
> > + plca_cfg->version = ret;
>
> It would be good to verify this value, and return -ENODEV if it is not
> valid.
Got it, thanks.

Piergiorgio