On Tue, Apr 11, 2017 at 04:03:07PM +0200, Greg Kroah-Hartman wrote:
> On Mon, Apr 10, 2017 at 01:15:41PM +1000, Tobin C. Harding wrote:
> > Driver uses identifier 'rc' to hold the value for error return
> > code. The rest of the driver predominately uses 'ret' for this
> > purpose. It is easier to follow the code if one name is used for one
> > task.
> >
> > Rename identifier 'rc' to 'ret'.
> >
> > Signed-off-by: Tobin C. Harding <me@xxxxxxxx>
> > ---
> > drivers/staging/ks7010/ks7010_sdio.c | 12 +++++-------
> > drivers/staging/ks7010/ks_wlan_net.c | 11 +++++------
> > 2 files changed, 10 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c
> > index b93c9a4..467c0c4 100644
> > --- a/drivers/staging/ks7010/ks7010_sdio.c
> > +++ b/drivers/staging/ks7010/ks7010_sdio.c
> > @@ -309,19 +309,17 @@ static int write_to_device(struct ks_wlan_private *priv, unsigned char *buffer,
> > static void tx_device_task(struct ks_wlan_private *priv)
> > {
> > struct tx_device_buffer *sp;
> > - int rc = 0;
> > + int ret;
> >
> > DPRINTK(4, "\n");
> > if (cnt_txqbody(priv) > 0 &&
> > atomic_read(&priv->psstatus.status) != PS_SNOOZE) {
> > sp = &priv->tx_dev.tx_dev_buff[priv->tx_dev.qhead];
> > if (priv->dev_state >= DEVICE_STATE_BOOT) {
> > - rc = write_to_device(priv, sp->sendp, sp->size);
> > - if (rc) {
> > - DPRINTK(1, "write_to_device error !!(%d)\n",
> > - rc);
> > - queue_delayed_work(priv->ks_wlan_hw.
> > - ks7010sdio_wq,
> > + ret = write_to_device(priv, sp->sendp, sp->size);
> > + if (ret) {
> > + DPRINTK(1, "write_to_device error !!(%d)\n", ret);
> > + queue_delayed_work(priv->ks_wlan_hw.ks7010sdio_wq,
> > &priv->ks_wlan_hw.rw_wq, 1);
> > return;
> > }
> > diff --git a/drivers/staging/ks7010/ks_wlan_net.c b/drivers/staging/ks7010/ks_wlan_net.c
> > index 5e68699..e86e5e2 100644
> > --- a/drivers/staging/ks7010/ks_wlan_net.c
> > +++ b/drivers/staging/ks7010/ks_wlan_net.c
> > @@ -2902,7 +2902,7 @@ static
> > int ks_wlan_start_xmit(struct sk_buff *skb, struct net_device *dev)
> > {
> > struct ks_wlan_private *priv = netdev_priv(dev);
> > - int rc = 0;
> > + int ret;
> >
> > DPRINTK(3, "in_interrupt()=%ld\n", in_interrupt());
> >
> > @@ -2918,14 +2918,13 @@ int ks_wlan_start_xmit(struct sk_buff *skb, struct net_device *dev)
> > if (netif_running(dev))
> > netif_stop_queue(dev);
> >
> > - rc = hostif_data_request(priv, skb);
> > + ret = hostif_data_request(priv, skb);
> > netif_trans_update(dev);
> >
> > - DPRINTK(4, "rc=%d\n", rc);
> > - if (rc)
> > - rc = 0;
> > + if (ret)
> > + DPRINTK(4, "hostif_data_request error: =%d\n", ret);
> >
> > - return rc;
> > + return 0;
>
> Shouldn't you really return the error here? I know you aren't changing
> the logic, something to consider in the future...
I pondered this one for a while before making the change. I weighed up
returning the error code but the single call site for this function
does not check the return code. I also weighed up changing the
function return type to void. Finally I decided that the dilemma was
to do with the debug print statements, and that there is allot of
these that need removing a bit further down the development path. I
decided to leave it as it was until that time.
thanks for the review,
Tobin
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
