[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <16f60604-f3e9-1391-ff47-37c40ab9c6f7@denx.de>
Date: Thu, 14 May 2020 16:00:18 +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:15 PM, Andrew Lunn wrote:
> On Thu, May 14, 2020 at 04:26:30AM +0200, Marek Vasut wrote:
>> 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 ?
>
> Yes, please add another patch.
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 ?
>
> 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;
Powered by blists - more mailing lists