Re: [PATCH net-next v2 8/9] onsemi: s2500: Add driver support for TS2500 MAC-PHY

[Andrew Lunn]

> +static const char s2500_stat_strings[][ETH_GSTRING_LEN] = {
> +	"tx-bytes-ok",
> +	"tx-frames",
> +	"tx-broadcast-frames",
> +	"tx-multicast-frames",
> +	"tx-64-frames",
> +	"tx-65-127-frames",
> +	"tx-128-255-frames",
> +	"tx-256-511-frames",
> +	"tx-512-1023-frames",
> +	"tx-1024-1518-frames",
> +	"tx-underrun-errors",
> +	"tx-single-collision",
> +	"tx-multiple-collision",
> +	"tx-excessive-collision",
> +	"tx-deferred-frames",
> +	"tx-carrier-sense-errors",
> +	"rx-bytes-ok",
> +	"rx-frames",
> +	"rx-broadcast-frames",
> +	"rx-multicast-frames",
> +	"rx-64-frames",
> +	"rx-65-127-frames",
> +	"rx-128-255-frames",
> +	"rx-256-511-frames",
> +	"rx-512-1023-frames",
> +	"rx-1024-1518-frames",
> +	"rx-runt",
> +	"rx-too-long-frames",
> +	"rx-crc-errors",
> +	"rx-symbol-errors",
> +	"rx-alignment-errors",
> +	"rx-busy-drop-frames",
> +	"rx-mismatch-drop-frames",
> +	"ts_frames",

Looks like many of these should be returned via .get_rmon_stats

> +static void s2500_get_drvinfo(struct net_device *ndev,
> +			      struct ethtool_drvinfo *info)
> +{
> +	strscpy(info->driver, DRV_NAME, sizeof(info->driver));
> +	strscpy(info->bus_info, dev_name(&ndev->dev), sizeof(info->bus_info));
> +	strscpy(info->version, DRV_VERSION, sizeof(info->version));

version is pointless because it never changes, but the kernel is
always changing. If you don't fill it, the core will return the kernel
versions.

> +static int s2500_ethtool_set_link_ksettings(struct net_device *ndev,
> +					    const struct ethtool_link_ksettings *cmd)
> +{
> +	phy_ethtool_ksettings_set(ndev->phydev, cmd);
> +	return 0;
> +}

phy_ethtool_set_link_ksettings()

> +
> +static int s2500_ethtool_get_link_ksettings(struct net_device *ndev,
> +					    struct ethtool_link_ksettings *cmd)
> +{
> +	phy_ethtool_ksettings_get(ndev->phydev, cmd);
> +	return 0;
> +}

phy_ethtool_get_link_ksettings()

> +++ b/drivers/net/ethernet/onsemi/s2500/s2500_main.c
> @@ -0,0 +1,698 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright 2026 Semiconductor Components Industries, LLC ("onsemi").
> + * onsemi's S2500 10BASE-T1S MAC-PHY driver
> + */
> +
> +#include "s2500_hw_def.h"
> +
> +#include <linux/etherdevice.h>
> +#include <linux/if_ether.h>
> +#include <linux/irqchip.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/phy.h>
> +
> +/* S2500 functions & definitions */
> +
> +#define S2500_STATUS0_MASK	(S2500_SPI_ST0_CDPE_BIT | \
> +				S2500_SPI_ST0_TXFCSE_BIT | \
> +				S2500_SPI_ST0_TTSCAC_BIT | \
> +				S2500_SPI_ST0_TTSCAB_BIT | \
> +				S2500_SPI_ST0_TTSCAA_BIT | \
> +				S2500_SPI_ST0_RESETC_BIT | \
> +				S2500_SPI_ST0_HDRE_BIT | \
> +				S2500_SPI_ST0_LOFE_BIT | \
> +				S2500_SPI_ST0_RXBOE_BIT | \
> +				S2500_SPI_ST0_TXBUE_BIT | \
> +				S2500_SPI_ST0_TXBOE_BIT | \
> +				S2500_SPI_ST0_TXPE_BIT)
> +
> +/* Converts a MACPHY ID to a device name */
> +static inline const char *s2500_id_to_name(u32 id)
> +{
> +	if (id == S2500_MODEL_ID)
> +		return "S2500";
> +	return "unknown";
> +}

No inline functions in .c file please.

> +static int s2500_open(struct net_device *ndev)
> +{
> +	struct s2500_info *priv = netdev_priv(ndev);
> +	int ret = 0;
> +	u32 val;
> +
> +	dev_info(&priv->spi->dev, "%s", "s2500_open");

dev_dbg() or not at all. Please don't spam the kernel log.

> +static int s2500_hwtstamp_get(struct net_device *ndev,
> +			      struct kernel_hwtstamp_config *cfg)
> +{
> +	struct s2500_info *priv = netdev_priv(ndev);
> +
> +	if (!priv->ptp_clock) {
> +		cfg->tx_type = 0;
> +		cfg->rx_filter = 0;
> +	} else {
> +		oa_tc6_hwtstamp_get(priv->tc6, cfg);
> +	}
> +	return 0;

What is actually specific to the s2500 here? Your aim should be to put
everything you can into the core.

> +	/* Configure PTP if the model supports it */
> +	if (priv->capabilities & S2500_CAP_PTP)
> +		s2500_ptp_register(priv);

That should be in the core, based on FTSC.

	Andrew