Re: [PATCH net-next v3 01/14] motorcomm:yt6801: Implement mdio register

[Andrew Lunn <andrew@lunn.ch>]

On Fri, Feb 28, 2025 at 06:01:04PM +0800, Frank Sae wrote:
> Implement the mdio bus read, write and register function.

> +++ b/drivers/net/ethernet/motorcomm/yt6801/yt6801.h
> @@ -0,0 +1,379 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/* Copyright (c) 2022 - 2024 Motorcomm Electronic Technology Co.,Ltd. */
> +
> +#ifndef YT6801_H
> +#define YT6801_H
> +
> +#include <linux/dma-mapping.h>
> +#include <linux/timecounter.h>
> +#include <linux/pm_wakeup.h>
> +#include <linux/workqueue.h>
> +#include <linux/crc32poly.h>
> +#include <linux/if_vlan.h>
> +#include <linux/bitrev.h>
> +#include <linux/bitops.h>
> +#include <linux/mdio.h>
> +#include <linux/phy.h>
> +
> +#ifdef CONFIG_PCI_MSI
> +#include <linux/pci.h>
> +#endif
> +
> +#include "yt6801_type.h"
> +
> +#define FXGMAC_DRV_NAME		"yt6801"
> +#define FXGMAC_DRV_DESC		"Motorcomm Gigabit Ethernet Driver"
> +
> +#define FXGMAC_RX_BUF_ALIGN	64
> +#define FXGMAC_TX_MAX_BUF_SIZE	(0x3fff & ~(FXGMAC_RX_BUF_ALIGN - 1))
> +#define FXGMAC_RX_MIN_BUF_SIZE	(ETH_FRAME_LEN + ETH_FCS_LEN + VLAN_HLEN)
> +
> +/* Descriptors required for maximum contiguous TSO/GSO packet */
> +#define FXGMAC_TX_MAX_SPLIT	((GSO_MAX_SIZE / FXGMAC_TX_MAX_BUF_SIZE) + 1)
> +
> +/* Maximum possible descriptors needed for a SKB */
> +#define FXGMAC_TX_MAX_DESC_NR	(MAX_SKB_FRAGS + FXGMAC_TX_MAX_SPLIT + 2)
> +
> +#define FXGMAC_DMA_STOP_TIMEOUT		5
> +#define FXGMAC_JUMBO_PACKET_MTU		9014
> +#define FXGMAC_MAX_DMA_RX_CHANNELS	4
> +#define FXGMAC_MAX_DMA_TX_CHANNELS	1
> +#define FXGMAC_MAX_DMA_CHANNELS                                           \
> +	(FXGMAC_MAX_DMA_RX_CHANNELS + FXGMAC_MAX_DMA_TX_CHANNELS)
> +
> +struct fxgmac_ring_buf {
> +	struct sk_buff *skb;
> +	dma_addr_t skb_dma;
> +	unsigned int skb_len;
> +};

There is clearly a lot of stuff here which has nothing to do with
MDIO. Please only add things which are truly to do with MDIO, in a
patch which claims to add support for MDIO read/write/register.

> +++ b/drivers/net/ethernet/motorcomm/yt6801/yt6801_net.c
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/* Copyright (c) 2022 - 2024 Motorcomm Electronic Technology Co.,Ltd. */
> +
> +#include <linux/inetdevice.h>
> +#include <linux/netdevice.h>
> +#include <linux/interrupt.h>
> +#include <net/addrconf.h>
> +#include <linux/inet.h>
> +#include <linux/tcp.h>
> +
> +#include "yt6801.h"
> +
> +#define PHY_WR_CONFIG(reg_offset)	(0x8000205 + ((reg_offset) * 0x10000))
> +static int fxgmac_phy_write_reg(struct fxgmac_pdata *priv, u32 reg_id, u32 data)
> +{
> +	u32 val;
> +	int ret;
> +
> +	FXGMAC_MAC_IO_WR(priv, MAC_MDIO_DATA, data);
> +	FXGMAC_MAC_IO_WR(priv, MAC_MDIO_ADDRESS, PHY_WR_CONFIG(reg_id));

The upper case suggest FXGMAC_MAC_IO_WR() is a macro. Why not use a
function? You then get type checking. There is nothing hot path here,
so it is better to have the compiler doing all the checks it can.

> +	ret = read_poll_timeout_atomic(FXGMAC_MAC_IO_RD, val,
> +				       !FXGMAC_GET_BITS(val, MAC_MDIO_ADDR, BUSY),
> +				       10, 250, false, priv, MAC_MDIO_ADDRESS);
> +	if (ret == -ETIMEDOUT) {
> +		yt_err(priv, "%s err, id:%x ctrl:0x%08x, data:0x%08x\n",
> +		       __func__, reg_id, PHY_WR_CONFIG(reg_id), data);

It would be much more usual to use netdev_err(priv->ndev, ....), or
dev_err(priv->dev, ....). Wrapping these functions is not really
liked.

The "err" should also be unnecessary since you call netdev_err().

> +		return ret;
> +	}
> +
> +	return ret;

You can consolidate the two return statements.

> +static int fxgmac_mdio_register(struct fxgmac_pdata *priv)
> +{
> +	struct pci_dev *pdev = to_pci_dev(priv->dev);
> +	struct phy_device *phydev;
> +	struct mii_bus *new_bus;
> +	int ret;
> +
> +	new_bus = devm_mdiobus_alloc(&pdev->dev);
> +	if (!new_bus)
> +		return -ENOMEM;
> +
> +	new_bus->name = "yt6801";
> +	new_bus->priv = priv;
> +	new_bus->parent = &pdev->dev;
> +	new_bus->read = fxgmac_mdio_read_reg;
> +	new_bus->write = fxgmac_mdio_write_reg;
> +	snprintf(new_bus->id, MII_BUS_ID_SIZE, "yt6801-%x-%x",
> +		 pci_domain_nr(pdev->bus), pci_dev_id(pdev));
> +
> +	ret = devm_mdiobus_register(&pdev->dev, new_bus);
> +	if (ret < 0)
> +		return ret;
> +
> +	phydev = mdiobus_get_phy(new_bus, 0);
> +	if (!phydev)
> +		return -ENODEV;
> +
> +	priv->phydev = phydev;
> +	return 0;
> +}

> +/* Bit getting and setting macros
> + *  The get macro will extract the current bit field value from within
> + *  the variable
> + *
> + *  The set macro will clear the current bit field value within the
> + *  variable and then set the bit field of the variable to the
> + *  specified value
> + */
> +#define GET_BITS(_var, _index, _width) \
> +	(((_var) >> (_index)) & ((0x1U << (_width)) - 1))
> +
> +#define SET_BITS(_var, _index, _width, _val)                                  \
> +	do {                                                                  \
> +		(_var) &= ~(((0x1U << (_width)) - 1) << (_index));            \
> +		(_var) |= (((_val) & ((0x1U << (_width)) - 1)) << (_index));  \
> +	} while (0)
> +
> +#define GET_BITS_LE(_var, _index, _width) \
> +	((le32_to_cpu((_var)) >> (_index)) & ((0x1U << (_width)) - 1))
> +
> +#define SET_BITS_LE(_var, _index, _width, _val)                               \
> +	do {                                                                  \
> +		(_var) &=                                                     \
> +			cpu_to_le32(~(((0x1U << (_width)) - 1) << (_index))); \
> +		(_var) |= cpu_to_le32(                                        \
> +			(((_val) & ((0x1U << (_width)) - 1)) << (_index)));   \
> +	} while (0)

Please don't reinvent include/linux/bitfield.h.  We want all drivers
to look the same, use the same macros for bit manipulation, since it
makes maintenance easier. We know what the macros in bitfield.h do
because everybody uses them. When you do something different, you
loose out on that knowledge.

We also want endianness to be obvious, and use the correct markup, so
that tools like sparse can find missing conversions.

	Andrew