| Message ID | 5daa7f3e467b218410238ef0fb97f01779f8f49f.1536916714.git-series.quentin.schulz@bootlin.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | add support for VSC8584 and VSC8574 Microsemi quad-port PHYs | expand |
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.
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
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;
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(-)