lkml.org 
[lkml]   [2022]   [Dec]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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!

\
 
 \ /
  Last update: 2022-12-04 17:52    [W:0.079 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site