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

On 5/14/20 3:15 PM, Andrew Lunn wrote:

On Thu, May 14, 2020 at 04:26:30AM +0200, Marek Vasut wrote:

quoted

On 5/14/20 3:57 AM, Andrew Lunn wrote:

quoted

quoted

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.

This actually was in the original ks8851.c since forever, so I wonder.
Maybe a separate patch would be better ?

Yes, please add another patch.

OK

quoted

quoted

quoted

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

quoted

+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.

This was in the original ks8851.c , so I need to retain it , no ?

Ah. Err.

This patch looks like a new driver. It has probe, remove
module_platform_driver(), etc. So as a new driver, it should not have
module parameters.

But then your next patch removes the mll driver. Your intention is
that this driver replaces the mll driver. So for backwards
compatibility, yes you do need the module parameter.

All right

btw is jiffies-based timeout OK? Like this:

diff --git a/drivers/net/ethernet/micrel/ks8851_par.c

b/drivers/net/ethernet/micrel/ks8851_par.c
index 5163d10971f4..1d9e7a33ffcf 100644

--- a/drivers/net/ethernet/micrel/ks8851_par.c
+++ b/drivers/net/ethernet/micrel/ks8851_par.c

@@ -238,6 +238,7 @@ static netdev_tx_t ks8851_start_xmit_par(struct

sk_buff *skb,
                                         struct net_device *dev)
 {
        struct ks8851_net *ks = netdev_priv(dev);
+       unsigned long txpoll_start_time;
        netdev_tx_t ret = NETDEV_TX_OK;
        unsigned long flags;
        u16 txmir;

@@ -254,8 +255,20 @@ static netdev_tx_t ks8851_start_xmit_par(struct

sk_buff *skb,
                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)
-                       ;
+
+               txpoll_start_time = jiffies;
+               while (true) {
+                       if (!(ks8851_rdreg16_par(ks, KS_TXQCR) &
TXQCR_METFE))
+                               break;
+
+                       if (time_after(jiffies, txpoll_start_time + HZ)) {
+                               netdev_warn(dev, "%s: did not complete.\n",
+                                           __func__);
+                               ret = NETDEV_TX_BUSY;
+                               break;
+                       }
+               }
+
                ks8851_done_tx(ks, skb);
        } else {
                ret = NETDEV_TX_BUSY;