| Message ID | 20190805165453.3989-4-alexandru.ardelean@analog.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phy: adin: add support for Analog Devices PHYs | expand |
On Mon, Aug 05, 2019 at 07:54:40PM +0300, Alexandru Ardelean wrote: > This change adds support for enabling PHY interrupts that can be used by > the PHY framework to get signal for link/speed/auto-negotiation changes. > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > --- > drivers/net/phy/adin.c | 44 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c > index c100a0dd95cd..b75c723bda79 100644 > --- a/drivers/net/phy/adin.c > +++ b/drivers/net/phy/adin.c > @@ -14,6 +14,22 @@ > #define PHY_ID_ADIN1200 0x0283bc20 > #define PHY_ID_ADIN1300 0x0283bc30 > > +#define ADIN1300_INT_MASK_REG 0x0018 > +#define ADIN1300_INT_MDIO_SYNC_EN BIT(9) > +#define ADIN1300_INT_ANEG_STAT_CHNG_EN BIT(8) > +#define ADIN1300_INT_ANEG_PAGE_RX_EN BIT(6) > +#define ADIN1300_INT_IDLE_ERR_CNT_EN BIT(5) > +#define ADIN1300_INT_MAC_FIFO_OU_EN BIT(4) > +#define ADIN1300_INT_RX_STAT_CHNG_EN BIT(3) > +#define ADIN1300_INT_LINK_STAT_CHNG_EN BIT(2) > +#define ADIN1300_INT_SPEED_CHNG_EN BIT(1) > +#define ADIN1300_INT_HW_IRQ_EN BIT(0) > +#define ADIN1300_INT_MASK_EN \ > + (ADIN1300_INT_ANEG_STAT_CHNG_EN | ADIN1300_INT_ANEG_PAGE_RX_EN | \ > + ADIN1300_INT_LINK_STAT_CHNG_EN | ADIN1300_INT_SPEED_CHNG_EN | \ > + ADIN1300_INT_HW_IRQ_EN) > +#define ADIN1300_INT_STATUS_REG 0x0019 > + > static int adin_config_init(struct phy_device *phydev) > { > int rc; > @@ -25,15 +41,40 @@ static int adin_config_init(struct phy_device *phydev) > return 0; > } > > +static int adin_phy_ack_intr(struct phy_device *phydev) > +{ > + int ret; > + > + /* Clear pending interrupts. */ > + ret = phy_read(phydev, ADIN1300_INT_STATUS_REG); > + if (ret < 0) > + return ret; > + > + return 0; Please go through the whole driver and throw out all the needless if (ret < 0) return ret; return 0; Thanks Andrew
On 05.08.2019 18:54, Alexandru Ardelean wrote: > This change adds support for enabling PHY interrupts that can be used by > the PHY framework to get signal for link/speed/auto-negotiation changes. > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > --- > drivers/net/phy/adin.c | 44 ++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c > index c100a0dd95cd..b75c723bda79 100644 > --- a/drivers/net/phy/adin.c > +++ b/drivers/net/phy/adin.c > @@ -14,6 +14,22 @@ > #define PHY_ID_ADIN1200 0x0283bc20 > #define PHY_ID_ADIN1300 0x0283bc30 > > +#define ADIN1300_INT_MASK_REG 0x0018 > +#define ADIN1300_INT_MDIO_SYNC_EN BIT(9) > +#define ADIN1300_INT_ANEG_STAT_CHNG_EN BIT(8) > +#define ADIN1300_INT_ANEG_PAGE_RX_EN BIT(6) > +#define ADIN1300_INT_IDLE_ERR_CNT_EN BIT(5) > +#define ADIN1300_INT_MAC_FIFO_OU_EN BIT(4) > +#define ADIN1300_INT_RX_STAT_CHNG_EN BIT(3) > +#define ADIN1300_INT_LINK_STAT_CHNG_EN BIT(2) > +#define ADIN1300_INT_SPEED_CHNG_EN BIT(1) > +#define ADIN1300_INT_HW_IRQ_EN BIT(0) > +#define ADIN1300_INT_MASK_EN \ > + (ADIN1300_INT_ANEG_STAT_CHNG_EN | ADIN1300_INT_ANEG_PAGE_RX_EN | \ > + ADIN1300_INT_LINK_STAT_CHNG_EN | ADIN1300_INT_SPEED_CHNG_EN | \ > + ADIN1300_INT_HW_IRQ_EN) > +#define ADIN1300_INT_STATUS_REG 0x0019 > + > static int adin_config_init(struct phy_device *phydev) > { > int rc; > @@ -25,15 +41,40 @@ static int adin_config_init(struct phy_device *phydev) > return 0; > } > > +static int adin_phy_ack_intr(struct phy_device *phydev) > +{ > + int ret; > + > + /* Clear pending interrupts. */ > + ret = phy_read(phydev, ADIN1300_INT_STATUS_REG); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +static int adin_phy_config_intr(struct phy_device *phydev) > +{ > + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) > + return phy_set_bits(phydev, ADIN1300_INT_MASK_REG, > + ADIN1300_INT_MASK_EN); > + > + return phy_clear_bits(phydev, ADIN1300_INT_MASK_REG, > + ADIN1300_INT_MASK_EN); > +} > + > static struct phy_driver adin_driver[] = { > { > .phy_id = PHY_ID_ADIN1200, > .name = "ADIN1200", > .phy_id_mask = 0xfffffff0, > .features = PHY_BASIC_FEATURES, > + .flags = PHY_HAS_INTERRUPT, This flag doesn't exist any longer. This indicates that you develop against an older kernel version. Please develop against net-next. Check up-to-date drivers like the one for Realtek PHY's for hints. > .config_init = adin_config_init, > .config_aneg = genphy_config_aneg, > .read_status = genphy_read_status, > + .ack_interrupt = adin_phy_ack_intr, > + .config_intr = adin_phy_config_intr, > .resume = genphy_resume, > .suspend = genphy_suspend, > }, > @@ -42,9 +83,12 @@ static struct phy_driver adin_driver[] = { > .name = "ADIN1300", > .phy_id_mask = 0xfffffff0, > .features = PHY_GBIT_FEATURES, > + .flags = PHY_HAS_INTERRUPT, > .config_init = adin_config_init, > .config_aneg = genphy_config_aneg, > .read_status = genphy_read_status, > + .ack_interrupt = adin_phy_ack_intr, > + .config_intr = adin_phy_config_intr, > .resume = genphy_resume, > .suspend = genphy_suspend, > }, >
On Mon, 2019-08-05 at 16:21 +0200, Andrew Lunn wrote: > [External] > > On Mon, Aug 05, 2019 at 07:54:40PM +0300, Alexandru Ardelean wrote: > > This change adds support for enabling PHY interrupts that can be used by > > the PHY framework to get signal for link/speed/auto-negotiation changes. > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > --- > > drivers/net/phy/adin.c | 44 ++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 44 insertions(+) > > > > diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c > > index c100a0dd95cd..b75c723bda79 100644 > > --- a/drivers/net/phy/adin.c > > +++ b/drivers/net/phy/adin.c > > @@ -14,6 +14,22 @@ > > #define PHY_ID_ADIN1200 0x0283bc20 > > #define PHY_ID_ADIN1300 0x0283bc30 > > > > +#define ADIN1300_INT_MASK_REG 0x0018 > > +#define ADIN1300_INT_MDIO_SYNC_EN BIT(9) > > +#define ADIN1300_INT_ANEG_STAT_CHNG_EN BIT(8) > > +#define ADIN1300_INT_ANEG_PAGE_RX_EN BIT(6) > > +#define ADIN1300_INT_IDLE_ERR_CNT_EN BIT(5) > > +#define ADIN1300_INT_MAC_FIFO_OU_EN BIT(4) > > +#define ADIN1300_INT_RX_STAT_CHNG_EN BIT(3) > > +#define ADIN1300_INT_LINK_STAT_CHNG_EN BIT(2) > > +#define ADIN1300_INT_SPEED_CHNG_EN BIT(1) > > +#define ADIN1300_INT_HW_IRQ_EN BIT(0) > > +#define ADIN1300_INT_MASK_EN \ > > + (ADIN1300_INT_ANEG_STAT_CHNG_EN | ADIN1300_INT_ANEG_PAGE_RX_EN | \ > > + ADIN1300_INT_LINK_STAT_CHNG_EN | ADIN1300_INT_SPEED_CHNG_EN | \ > > + ADIN1300_INT_HW_IRQ_EN) > > +#define ADIN1300_INT_STATUS_REG 0x0019 > > + > > static int adin_config_init(struct phy_device *phydev) > > { > > int rc; > > @@ -25,15 +41,40 @@ static int adin_config_init(struct phy_device *phydev) > > return 0; > > } > > > > +static int adin_phy_ack_intr(struct phy_device *phydev) > > +{ > > + int ret; > > + > > + /* Clear pending interrupts. */ > > + ret = phy_read(phydev, ADIN1300_INT_STATUS_REG); > > + if (ret < 0) > > + return ret; > > + > > + return 0; > > Please go through the whole driver and throw out all the needless ack; i'll re-visit; > > if (ret < 0) > return ret; > > return 0; > > Thanks > Andrew
On Mon, 2019-08-05 at 23:02 +0200, Heiner Kallweit wrote: > [External] > > On 05.08.2019 18:54, Alexandru Ardelean wrote: > > This change adds support for enabling PHY interrupts that can be used by > > the PHY framework to get signal for link/speed/auto-negotiation changes. > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > --- > > drivers/net/phy/adin.c | 44 ++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 44 insertions(+) > > > > diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c > > index c100a0dd95cd..b75c723bda79 100644 > > --- a/drivers/net/phy/adin.c > > +++ b/drivers/net/phy/adin.c > > @@ -14,6 +14,22 @@ > > #define PHY_ID_ADIN1200 0x0283bc20 > > #define PHY_ID_ADIN1300 0x0283bc30 > > > > +#define ADIN1300_INT_MASK_REG 0x0018 > > +#define ADIN1300_INT_MDIO_SYNC_EN BIT(9) > > +#define ADIN1300_INT_ANEG_STAT_CHNG_EN BIT(8) > > +#define ADIN1300_INT_ANEG_PAGE_RX_EN BIT(6) > > +#define ADIN1300_INT_IDLE_ERR_CNT_EN BIT(5) > > +#define ADIN1300_INT_MAC_FIFO_OU_EN BIT(4) > > +#define ADIN1300_INT_RX_STAT_CHNG_EN BIT(3) > > +#define ADIN1300_INT_LINK_STAT_CHNG_EN BIT(2) > > +#define ADIN1300_INT_SPEED_CHNG_EN BIT(1) > > +#define ADIN1300_INT_HW_IRQ_EN BIT(0) > > +#define ADIN1300_INT_MASK_EN \ > > + (ADIN1300_INT_ANEG_STAT_CHNG_EN | ADIN1300_INT_ANEG_PAGE_RX_EN | \ > > + ADIN1300_INT_LINK_STAT_CHNG_EN | ADIN1300_INT_SPEED_CHNG_EN | \ > > + ADIN1300_INT_HW_IRQ_EN) > > +#define ADIN1300_INT_STATUS_REG 0x0019 > > + > > static int adin_config_init(struct phy_device *phydev) > > { > > int rc; > > @@ -25,15 +41,40 @@ static int adin_config_init(struct phy_device *phydev) > > return 0; > > } > > > > +static int adin_phy_ack_intr(struct phy_device *phydev) > > +{ > > + int ret; > > + > > + /* Clear pending interrupts. */ > > + ret = phy_read(phydev, ADIN1300_INT_STATUS_REG); > > + if (ret < 0) > > + return ret; > > + > > + return 0; > > +} > > + > > +static int adin_phy_config_intr(struct phy_device *phydev) > > +{ > > + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) > > + return phy_set_bits(phydev, ADIN1300_INT_MASK_REG, > > + ADIN1300_INT_MASK_EN); > > + > > + return phy_clear_bits(phydev, ADIN1300_INT_MASK_REG, > > + ADIN1300_INT_MASK_EN); > > +} > > + > > static struct phy_driver adin_driver[] = { > > { > > .phy_id = PHY_ID_ADIN1200, > > .name = "ADIN1200", > > .phy_id_mask = 0xfffffff0, > > .features = PHY_BASIC_FEATURES, > > + .flags = PHY_HAS_INTERRUPT, > > This flag doesn't exist any longer. This indicates that you > develop against an older kernel version. Please develop > against net-next. Check up-to-date drivers like the one > for Realtek PHY's for hints. ack; > > > .config_init = adin_config_init, > > .config_aneg = genphy_config_aneg, > > .read_status = genphy_read_status, > > + .ack_interrupt = adin_phy_ack_intr, > > + .config_intr = adin_phy_config_intr, > > .resume = genphy_resume, > > .suspend = genphy_suspend, > > }, > > @@ -42,9 +83,12 @@ static struct phy_driver adin_driver[] = { > > .name = "ADIN1300", > > .phy_id_mask = 0xfffffff0, > > .features = PHY_GBIT_FEATURES, > > + .flags = PHY_HAS_INTERRUPT, > > .config_init = adin_config_init, > > .config_aneg = genphy_config_aneg, > > .read_status = genphy_read_status, > > + .ack_interrupt = adin_phy_ack_intr, > > + .config_intr = adin_phy_config_intr, > > .resume = genphy_resume, > > .suspend = genphy_suspend, > > }, > >
diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c index c100a0dd95cd..b75c723bda79 100644 --- a/drivers/net/phy/adin.c +++ b/drivers/net/phy/adin.c @@ -14,6 +14,22 @@ #define PHY_ID_ADIN1200 0x0283bc20 #define PHY_ID_ADIN1300 0x0283bc30 +#define ADIN1300_INT_MASK_REG 0x0018 +#define ADIN1300_INT_MDIO_SYNC_EN BIT(9) +#define ADIN1300_INT_ANEG_STAT_CHNG_EN BIT(8) +#define ADIN1300_INT_ANEG_PAGE_RX_EN BIT(6) +#define ADIN1300_INT_IDLE_ERR_CNT_EN BIT(5) +#define ADIN1300_INT_MAC_FIFO_OU_EN BIT(4) +#define ADIN1300_INT_RX_STAT_CHNG_EN BIT(3) +#define ADIN1300_INT_LINK_STAT_CHNG_EN BIT(2) +#define ADIN1300_INT_SPEED_CHNG_EN BIT(1) +#define ADIN1300_INT_HW_IRQ_EN BIT(0) +#define ADIN1300_INT_MASK_EN \ + (ADIN1300_INT_ANEG_STAT_CHNG_EN | ADIN1300_INT_ANEG_PAGE_RX_EN | \ + ADIN1300_INT_LINK_STAT_CHNG_EN | ADIN1300_INT_SPEED_CHNG_EN | \ + ADIN1300_INT_HW_IRQ_EN) +#define ADIN1300_INT_STATUS_REG 0x0019 + static int adin_config_init(struct phy_device *phydev) { int rc; @@ -25,15 +41,40 @@ static int adin_config_init(struct phy_device *phydev) return 0; } +static int adin_phy_ack_intr(struct phy_device *phydev) +{ + int ret; + + /* Clear pending interrupts. */ + ret = phy_read(phydev, ADIN1300_INT_STATUS_REG); + if (ret < 0) + return ret; + + return 0; +} + +static int adin_phy_config_intr(struct phy_device *phydev) +{ + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) + return phy_set_bits(phydev, ADIN1300_INT_MASK_REG, + ADIN1300_INT_MASK_EN); + + return phy_clear_bits(phydev, ADIN1300_INT_MASK_REG, + ADIN1300_INT_MASK_EN); +} + static struct phy_driver adin_driver[] = { { .phy_id = PHY_ID_ADIN1200, .name = "ADIN1200", .phy_id_mask = 0xfffffff0, .features = PHY_BASIC_FEATURES, + .flags = PHY_HAS_INTERRUPT, .config_init = adin_config_init, .config_aneg = genphy_config_aneg, .read_status = genphy_read_status, + .ack_interrupt = adin_phy_ack_intr, + .config_intr = adin_phy_config_intr, .resume = genphy_resume, .suspend = genphy_suspend, }, @@ -42,9 +83,12 @@ static struct phy_driver adin_driver[] = { .name = "ADIN1300", .phy_id_mask = 0xfffffff0, .features = PHY_GBIT_FEATURES, + .flags = PHY_HAS_INTERRUPT, .config_init = adin_config_init, .config_aneg = genphy_config_aneg, .read_status = genphy_read_status, + .ack_interrupt = adin_phy_ack_intr, + .config_intr = adin_phy_config_intr, .resume = genphy_resume, .suspend = genphy_suspend, },
This change adds support for enabling PHY interrupts that can be used by the PHY framework to get signal for link/speed/auto-negotiation changes. Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> --- drivers/net/phy/adin.c | 44 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)