| Message ID | 20190930092710.32739-1-o.rempel@pengutronix.de |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v1] net: phy: at803x: add ar9331 support | expand |
On Mon, Sep 30, 2019 at 11:27:10AM +0200, Oleksij Rempel wrote: > Mostly this hardware can work with generic PHY driver, but this change > is needed to provided interrupt handling support. > Tested with dsa ar9331-switch driver. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > drivers/net/phy/at803x.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c > index 6ad8b1c63c34..d62a77adb8e7 100644 > --- a/drivers/net/phy/at803x.c > +++ b/drivers/net/phy/at803x.c > @@ -53,6 +53,7 @@ > #define AT803X_DEBUG_REG_5 0x05 > #define AT803X_DEBUG_TX_CLK_DLY_EN BIT(8) > > +#define AR9331_PHY_ID 0x004dd041 > #define ATH8030_PHY_ID 0x004dd076 > #define ATH8031_PHY_ID 0x004dd074 > #define ATH8035_PHY_ID 0x004dd072 Hi Oleksij I wonder if we should call this ATH9331_PHY_ID, to keep with the naming convention? Why did you choose AR, not ATH? Thanks Andrew
On Mon, Sep 30, 2019 at 03:20:41PM +0200, Andrew Lunn wrote: > On Mon, Sep 30, 2019 at 11:27:10AM +0200, Oleksij Rempel wrote: > > Mostly this hardware can work with generic PHY driver, but this change > > is needed to provided interrupt handling support. > > Tested with dsa ar9331-switch driver. > > > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > > --- > > drivers/net/phy/at803x.c | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c > > index 6ad8b1c63c34..d62a77adb8e7 100644 > > --- a/drivers/net/phy/at803x.c > > +++ b/drivers/net/phy/at803x.c > > @@ -53,6 +53,7 @@ > > #define AT803X_DEBUG_REG_5 0x05 > > #define AT803X_DEBUG_TX_CLK_DLY_EN BIT(8) > > > > +#define AR9331_PHY_ID 0x004dd041 > > #define ATH8030_PHY_ID 0x004dd076 > > #define ATH8031_PHY_ID 0x004dd074 > > #define ATH8035_PHY_ID 0x004dd072 > > Hi Oleksij > > I wonder if we should call this ATH9331_PHY_ID, to keep with the > naming convention? Why did you choose AR, not ATH? Hi Andrew, i have no strong opinion here. Taking what ever name was on the SoC package, was safe enough for me. If you say, it should be ATH9331, i'll rename it. Should i?
On 30.09.2019 11:27, Oleksij Rempel wrote: > Mostly this hardware can work with generic PHY driver, but this change > is needed to provided interrupt handling support. > Tested with dsa ar9331-switch driver. > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > drivers/net/phy/at803x.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c > index 6ad8b1c63c34..d62a77adb8e7 100644 > --- a/drivers/net/phy/at803x.c > +++ b/drivers/net/phy/at803x.c > @@ -53,6 +53,7 @@ > #define AT803X_DEBUG_REG_5 0x05 > #define AT803X_DEBUG_TX_CLK_DLY_EN BIT(8) > > +#define AR9331_PHY_ID 0x004dd041 > #define ATH8030_PHY_ID 0x004dd076 > #define ATH8031_PHY_ID 0x004dd074 > #define ATH8035_PHY_ID 0x004dd072 > @@ -406,11 +407,24 @@ static struct phy_driver at803x_driver[] = { > .aneg_done = at803x_aneg_done, > .ack_interrupt = &at803x_ack_interrupt, > .config_intr = &at803x_config_intr, > +}, { > + /* ATHEROS AR9331 */ > + .phy_id = AR9331_PHY_ID, > + .name = "Atheros AR9331 built-in PHY", > + .phy_id_mask = AT803X_PHY_ID_MASK, The ID mask of 0xffffffef is quite strange, it ignores the last bit of the model number and requires exact match of the revision number. Unfortunately the original commit doesn't explain why this mask was chosen. It would only make sense if there are functionally identical PHY's with e.g. id 0x004dd051. If in doubt I'd suggest you use macro PHY_ID_MATCH_EXACT. > + .probe = at803x_probe, After 5c5f626bcace ("net: phy: improve handling link_change_notify callback") struct at803x_priv isn't used any longer and the probe callback could be removed. I didn't do that as part of this commit because I could compile-test the change only. > + .config_init = at803x_config_init, > + .suspend = at803x_suspend, > + .resume = at803x_resume, > + /* PHY_BASIC_FEATURES */ > + .ack_interrupt = &at803x_ack_interrupt, > + .config_intr = &at803x_config_intr, > } }; > > module_phy_driver(at803x_driver); > > static struct mdio_device_id __maybe_unused atheros_tbl[] = { > + { AR9331_PHY_ID, AT803X_PHY_ID_MASK }, See comment above regarding the id mask. > { ATH8030_PHY_ID, AT803X_PHY_ID_MASK }, > { ATH8031_PHY_ID, AT803X_PHY_ID_MASK }, > { ATH8035_PHY_ID, AT803X_PHY_ID_MASK }, > Heiner
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c index 6ad8b1c63c34..d62a77adb8e7 100644 --- a/drivers/net/phy/at803x.c +++ b/drivers/net/phy/at803x.c @@ -53,6 +53,7 @@ #define AT803X_DEBUG_REG_5 0x05 #define AT803X_DEBUG_TX_CLK_DLY_EN BIT(8) +#define AR9331_PHY_ID 0x004dd041 #define ATH8030_PHY_ID 0x004dd076 #define ATH8031_PHY_ID 0x004dd074 #define ATH8035_PHY_ID 0x004dd072 @@ -406,11 +407,24 @@ static struct phy_driver at803x_driver[] = { .aneg_done = at803x_aneg_done, .ack_interrupt = &at803x_ack_interrupt, .config_intr = &at803x_config_intr, +}, { + /* ATHEROS AR9331 */ + .phy_id = AR9331_PHY_ID, + .name = "Atheros AR9331 built-in PHY", + .phy_id_mask = AT803X_PHY_ID_MASK, + .probe = at803x_probe, + .config_init = at803x_config_init, + .suspend = at803x_suspend, + .resume = at803x_resume, + /* PHY_BASIC_FEATURES */ + .ack_interrupt = &at803x_ack_interrupt, + .config_intr = &at803x_config_intr, } }; module_phy_driver(at803x_driver); static struct mdio_device_id __maybe_unused atheros_tbl[] = { + { AR9331_PHY_ID, AT803X_PHY_ID_MASK }, { ATH8030_PHY_ID, AT803X_PHY_ID_MASK }, { ATH8031_PHY_ID, AT803X_PHY_ID_MASK }, { ATH8035_PHY_ID, AT803X_PHY_ID_MASK },
Mostly this hardware can work with generic PHY driver, but this change is needed to provided interrupt handling support. Tested with dsa ar9331-switch driver. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- drivers/net/phy/at803x.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)