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

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

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.

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.

	Andrew