[net-next,3/7] net: phy: mscc: split config_init in two functions for VSC8584

Message ID 5daa7f3e467b218410238ef0fb97f01779f8f49f.1536916714.git-series.quentin.schulz@bootlin.com
State Changes Requested
Delegated to: David Miller
Headers show
Series
  • add support for VSC8584 and VSC8574 Microsemi quad-port PHYs
Related show

Commit Message

Quentin Schulz Sept. 14, 2018, 9:44 a.m.
Part of the config init is common between the VSC8584 and the VSC8574,
so to prepare the upcoming support for VSC8574, separate config_init
PHY-specific code to config_pre_init function which is set in the probe
function of the PHY and used in config_init.

Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
---
 drivers/net/phy/mscc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Florian Fainelli Sept. 14, 2018, 5:57 p.m. | #1
On 09/14/2018 02:44 AM, Quentin Schulz wrote:
> Part of the config init is common between the VSC8584 and the VSC8574,
> so to prepare the upcoming support for VSC8574, separate config_init
> PHY-specific code to config_pre_init function which is set in the probe
> function of the PHY and used in config_init.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
> ---
>  drivers/net/phy/mscc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
> index b450489..69cc3cf 100644
> --- a/drivers/net/phy/mscc.c
> +++ b/drivers/net/phy/mscc.c
> @@ -355,6 +355,7 @@ struct vsc8531_private {
>  	u64 *stats;
>  	int nstats;
>  	bool pkg_init;
> +	int (*config_pre_init)(struct mii_bus *bus, int phy);

Is not this overkill given that you have a reference to the phy_device,
you could check for the for phy_id to know which exact type you have and
call the appropriate pre_init function?

unsigned int phy might be more appropriate.
Quentin Schulz Oct. 1, 2018, 9:07 a.m. | #2
Hi Florian,

On Fri, Sep 14, 2018 at 10:57:08AM -0700, Florian Fainelli wrote:
> On 09/14/2018 02:44 AM, Quentin Schulz wrote:
> > Part of the config init is common between the VSC8584 and the VSC8574,
> > so to prepare the upcoming support for VSC8574, separate config_init
> > PHY-specific code to config_pre_init function which is set in the probe
> > function of the PHY and used in config_init.
> > 
> > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
> > ---
> >  drivers/net/phy/mscc.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
> > index b450489..69cc3cf 100644
> > --- a/drivers/net/phy/mscc.c
> > +++ b/drivers/net/phy/mscc.c
> > @@ -355,6 +355,7 @@ struct vsc8531_private {
> >  	u64 *stats;
> >  	int nstats;
> >  	bool pkg_init;
> > +	int (*config_pre_init)(struct mii_bus *bus, int phy);
> 
> Is not this overkill given that you have a reference to the phy_device,
> you could check for the for phy_id to know which exact type you have and
> call the appropriate pre_init function?
> 

Agreed. It just seemed "cleaner" to me to set config_pre_init in
separate probe functions which are pretty straightforward and short.
Thus not complexifying an already-not-so-straightforward config_init
function.

Anyway, I'll use the phy_id as suggested so that we don't overload the
vsc8531_private structure (since it's also only called once, in
config_init).

Thanks,
Quentin

Patch

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index b450489..69cc3cf 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -355,6 +355,7 @@  struct vsc8531_private {
 	u64 *stats;
 	int nstats;
 	bool pkg_init;
+	int (*config_pre_init)(struct mii_bus *bus, int phy);
 };
 
 #ifdef CONFIG_OF_MDIO
@@ -1298,7 +1299,7 @@  static int vsc8584_config_init(struct phy_device *phydev)
 	 */
 	if (!vsc8584_is_pkg_init(phydev, base_addr,
 				 val & PHY_ADDR_REVERSED ? 1 : 0)) {
-		ret = vsc8584_config_pre_init(phydev->mdio.bus, base_addr);
+		ret = vsc8531->config_pre_init(phydev->mdio.bus, base_addr);
 		if (ret)
 			goto err;
 	}
@@ -1486,6 +1487,7 @@  static int vsc8584_probe(struct phy_device *phydev)
 
 	phydev->priv = vsc8531;
 
+	vsc8531->config_pre_init = vsc8584_config_pre_init;
 	vsc8531->nleds = 4;
 	vsc8531->supp_led_modes = VSC8584_SUPP_LED_MODES;
 	vsc8531->hw_stats = vsc8584_hw_stats;