[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5dbab44d-de45-f8e2-b4e4-4be15408657e@denx.de>
Date: Thu, 14 May 2020 04:26:30 +0200
From: Marek Vasut <marex@...x.de>
To: Andrew Lunn <andrew@...n.ch>
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
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 <ben@...tec.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 ?
Powered by blists - more mailing lists