On 5/14/20 3:57 AM, Andrew Lunn wrote:
>> 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 <b...@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 ?
[...]
>> + 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.
OK
>> + 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.
This was in the original ks8851.c , so I need to retain it , no ?
