[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20200514015753.GL527401@lunn.ch>
Date: Thu, 14 May 2020 03:57:53 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Marek Vasut <marex@...x.de>
Cc: netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>,
Lukas Wunner <lukas@...ner.de>, Petr Stetiar <ynezz@...e.cz>,
YueHaibing <yuehaibing@...wei.com>
Subject: Re: [PATCH V5 18/19] net: ks8851: Implement Parallel bus operations
> 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@...tec.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
Powered by blists - more mailing lists