Messages in this thread |  | | Date | Sun, 4 Dec 2022 16:52:04 +0000 | From | "Russell King (Oracle)" <> | Subject | Re: [PATCH net-next 3/4] drivers/net/phy: Add driver for the onsemi NCN26000 10BASE-T1S PHY |
| |
Hi,
On Sun, Dec 04, 2022 at 03:31:33AM +0100, Piergiorgio Beruto wrote: > diff --git a/drivers/net/phy/ncn26000.c b/drivers/net/phy/ncn26000.c > new file mode 100644 > index 000000000000..65a34edc5b20 > --- /dev/null > +++ b/drivers/net/phy/ncn26000.c > @@ -0,0 +1,193 @@ > +// SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) > +/* > + * Driver for Analog Devices Industrial Ethernet T1L PHYs > + * > + * Copyright 2020 Analog Devices Inc. > + */ > +#include <linux/kernel.h> > +#include <linux/bitfield.h> > +#include <linux/delay.h> > +#include <linux/errno.h> > +#include <linux/init.h> > +#include <linux/module.h> > +#include <linux/mii.h> > +#include <linux/phy.h> > +#include <linux/property.h> > + > +#define PHY_ID_NCN26000 0x180FF5A1 > + > +#define NCN26000_REG_IRQ_CTL ((u16)16) > +#define NCN26000_REG_IRQ_STATUS ((u16)17) > + > +#define NCN26000_IRQ_LINKST_BIT ((u16)1) > +#define NCN26000_IRQ_PLCAST_BIT ((u16)(1 << 1)) > +#define NCN26000_IRQ_LJABBER_BIT ((u16)(1 << 2)) > +#define NCN26000_IRQ_RJABBER_BIT ((u16)(1 << 3)) > +#define NCN26000_IRQ_RJABBER_BIT ((u16)(1 << 3)) > +#define NCN26000_IRQ_PLCAREC_BIT ((u16)(1 << 4)) > +#define NCN26000_IRQ_PHYSCOL_BIT ((u16)(1 << 5))
There isn't much point in having the casts to u16 here. Also, BIT() is useful for identifying single bits.
> +static int ncn26000_enable(struct phy_device *phydev) > +{
This is actually the config_aneg() implementation, it should be named as such.
> + phydev->mdix_ctrl = ETH_TP_MDI_AUTO; > + phydev->mdix = ETH_TP_MDI; > + phydev->pause = 0; > + phydev->asym_pause = 0; > + phydev->speed = SPEED_10; > + phydev->duplex = DUPLEX_HALF;
Is this initialisation actually necessary?
> + > + // bring up the link (link_ctrl is mapped to BMCR_ANENABLE) > + // clear also ISOLATE mode and Collision Test > + return phy_write(phydev, MII_BMCR, BMCR_ANENABLE);
You always use AN even when ethtool turns off AN? If AN is mandatory, it seems there should be some way that phylib can force that to be the case.
> +} > + > +static int ncn26000_soft_reset(struct phy_device *phydev) > +{ > + int ret; > + > + ret = phy_set_bits(phydev, MII_BMCR, BMCR_RESET); > + > + if (ret != 0) > + return ret; > + > + return phy_read_poll_timeout(phydev, > + MII_BMCR, > + ret, > + !(ret & BMCR_RESET), > + 500, > + 20000, > + true);
Isn't this just genphy_reset() ?
> +} > + > +static int ncn26000_get_features(struct phy_device *phydev) > +{ > + linkmode_zero(phydev->supported); > + linkmode_set_bit(ETHTOOL_LINK_MODE_MII_BIT, phydev->supported); > + > + linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT1S_P2MP_Half_BIT, > + phydev->supported); > + > + linkmode_copy(phydev->advertising, phydev->supported); > + return 0; > +} > + > +static irqreturn_t ncn26000_handle_interrupt(struct phy_device *phydev) > +{ > + const struct ncn26000_priv *const priv = phydev->priv; > + u16 events; > + int ret; > + > + // read and aknowledge the IRQ status register > + ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS); > + > + if (unlikely(ret < 0)) > + return IRQ_NONE; > + > + events = (u16)ret & priv->enabled_irqs; > + if (events == 0) > + return IRQ_NONE; > + > + if (events & NCN26000_IRQ_LINKST_BIT) { > + ret = phy_read(phydev, MII_BMSR); > + > + if (unlikely(ret < 0)) { > + phydev_err(phydev, > + "error reading the status register (%d)\n", > + ret); > + > + return IRQ_NONE; > + } > + > + phydev->link = ((u16)ret & BMSR_ANEGCOMPLETE) ? 1 : 0;
1. aneg_complete shouldn't be used to set phydev->link. 2. phydev->link should be updated in the read_status() function, which the state machine will call. Setting it here without taking the lock introduces races.
> + } > + > + // handle more IRQs here > + > + phy_trigger_machine(phydev); > + return IRQ_HANDLED; > +} > + > +static int ncn26000_config_intr(struct phy_device *phydev) > +{ > + int ret; > + struct ncn26000_priv *priv = phydev->priv; > + > + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) { > + // acknowledge IRQs > + ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS); > + if (ret < 0) > + return ret; > + > + // get link status notifications > + priv->enabled_irqs = NCN26000_IRQ_LINKST_BIT; > + } else { > + // disable all IRQs > + priv->enabled_irqs = 0; > + } > + > + ret = phy_write(phydev, NCN26000_REG_IRQ_CTL, priv->enabled_irqs); > + if (ret != 0) > + return ret; > + > + return 0; > +} > + > +static int ncn26000_probe(struct phy_device *phydev) > +{ > + struct device *dev = &phydev->mdio.dev; > + struct ncn26000_priv *priv; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + phydev->priv = priv; > + > + return 0; > +} > + > +static void ncn26000_remove(struct phy_device *phydev) > +{ > + struct device *dev = &phydev->mdio.dev; > + struct ncn26000_priv *priv = phydev->priv; > + > + // free the private structure pointer > + devm_kfree(dev, priv);
No need to call devm_kfree() - the point of devm_*() is that resources are automatically released.
Thanks.
-- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
|  |