Re: [PATCH V5 18/19] net: ks8851: Implement Parallel bus operations

[Andrew Lunn <andrew@lunn.ch>]

> diff --git a/drivers/net/ethernet/micrel/ks8851_par.c b/drivers/net/ethernet/micrel/ks8851_par.c
> new file mode 100644
> index 000000000000..90fffacb1695
> --- /dev/null
> +++ b/drivers/net/ethernet/micrel/ks8851_par.c
> @@ -0,0 +1,348 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* drivers/net/ethernet/micrel/ks8851.c
> + *
> + * Copyright 2009 Simtec Electronics
> + *	http://www.simtec.co.uk/
> + *	Ben Dooks <ben@simtec.co.uk>
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#define DEBUG

I don't think you wanted that left in.

> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/netdevice.h>
> +#include <linux/etherdevice.h>
> +#include <linux/ethtool.h>
> +#include <linux/cache.h>
> +#include <linux/crc32.h>
> +#include <linux/mii.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <linux/platform_device.h>
> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_net.h>

I think some of these includes can be removed. There is no regular, or
gpio code in this file, etc.

> +
> +#include "ks8851.h"
> +
> +static int msg_enable;
> +
> +#define BE3             0x8000      /* Byte Enable 3 */
> +#define BE2             0x4000      /* Byte Enable 2 */
> +#define BE1             0x2000      /* Byte Enable 1 */
> +#define BE0             0x1000      /* Byte Enable 0 */
> +
> +/**
> + * struct ks8851_net_par - KS8851 Parallel driver private data
> + * @ks8851: KS8851 driver common private data
> + * @lock: Lock to ensure that the device is not accessed when busy.
> + * @hw_addr	: start address of data register.
> + * @hw_addr_cmd	: start address of command register.
> + * @cmd_reg_cache	: command register cached.
> + *
> + * The @lock ensures that the chip is protected when certain operations are
> + * in progress. When the read or write packet transfer is in progress, most
> + * of the chip registers are not ccessible until the transfer is finished and

accessible 

> + * We do this to firstly avoid sleeping with the network device locked,
> + * and secondly so we can round up more than one packet to transmit which
> + * means we can try and avoid generating too many transmit done interrupts.
> + */
> +static netdev_tx_t ks8851_start_xmit_par(struct sk_buff *skb,
> +					 struct net_device *dev)
> +{
> +	struct ks8851_net *ks = netdev_priv(dev);
> +	netdev_tx_t ret = NETDEV_TX_OK;
> +	unsigned long flags;
> +	u16 txmir;
> +
> +	netif_dbg(ks, tx_queued, ks->netdev,
> +		  "%s: skb %p, %d@%p\n", __func__, skb, skb->len, skb->data);
> +
> +	ks8851_lock_par(ks, &flags);
> +
> +	txmir = ks8851_rdreg16_par(ks, KS_TXMIR) & 0x1fff;
> +
> +	if (likely(txmir >= skb->len + 12)) {
> +		ks8851_wrreg16_par(ks, KS_RXQCR, ks->rc_rxqcr | RXQCR_SDA);
> +		ks8851_wrfifo_par(ks, skb, false);
> +		ks8851_wrreg16_par(ks, KS_RXQCR, ks->rc_rxqcr);
> +		ks8851_wrreg16_par(ks, KS_TXQCR, TXQCR_METFE);
> +		while (ks8851_rdreg16_par(ks, KS_TXQCR) & TXQCR_METFE)
> +			;

You should have a timeout/retry limit here, just in case.


> +		ks8851_done_tx(ks, skb);
> +	} else {
> +		ret = NETDEV_TX_BUSY;
> +	}
> +
> +	ks8851_unlock_par(ks, &flags);
> +
> +	return ret;
> +}

> +module_param_named(message, msg_enable, int, 0);
> +MODULE_PARM_DESC(message, "Message verbosity level (0=none, 31=all)");

Module parameters are bad. A new driver should not have one, if
possible. Please implement the ethtool .get_msglevel and .set_msglevel
instead.

	Andrew