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

[Marek Vasut]

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 <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 ?
> 
> 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;