Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 6 Dec 2022 19:07:56 +0100
From:   Piergiorgio Beruto <piergiorgio.beruto@gmail.com>
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org, Oleksij Rempel <o.rempel@pengutronix.de>
Subject: Re: [PATCH v4 net-next 4/5] drivers/net/phy: add helpers to get/set
 PLCA configuration
Message-ID: <Y4+E/H5ZXqdd4med@gvm01>
References: <cover.1670329232.git.piergiorgio.beruto@gmail.com>
 <4c6bb420c2169edb31abd5c4d5fe04090ed329e4.1670329232.git.piergiorgio.beruto@gmail.com>
 <Y49Ky7aCUZxE5Fwg@lunn.ch>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y49Ky7aCUZxE5Fwg@lunn.ch>
Precedence: bulk

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