linux-kernel - Re: [PATCH] staging: ks7010: fix styling WARNINGs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAH6sp9OD+TGLNYa3H5h8=rccCT841AaX4vXR8wHChbpyi2azWw@mail.gmail.com>
Date:   Fri, 30 Jun 2017 07:54:09 +0200
From:   Frans Klaver <fransklaver@...il.com>
To:     Mark Rogers <mail@...k-rogers.org>
Cc:     Greg KH <gregkh@...uxfoundation.org>,
        "Tobin C. Harding" <me@...in.cc>,
        sayli karnik <karniksayli1995@...il.com>,
        driverdevel <devel@...verdev.osuosl.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] staging: ks7010: fix styling WARNINGs

On Fri, Jun 30, 2017 at 6:52 AM, Mark Rogers <mail@...k-rogers.org> wrote:
> Trivial style changes. There are still 3 "line over 80 characters"
> checkpatch.pl warnings, but I think they are best left alone as
> breaking the first two warning lines could hurt readability. The third
> warning is a message that should not be broken for the sake of grep.
>
> All but one of the changes fix lines that exceed 80 characters. An
> embedded function name was replaced by __func__ as well.
>
> Signed-off-by: Mark Rogers <mail@...k-rogers.org>
> ---
>  drivers/staging/ks7010/ks7010_sdio.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/ks7010/ks7010_sdio.c b/drivers/staging/ks7010/ks7010_sdio.c
> index c325f48..6c0c6b2 100644
> --- a/drivers/staging/ks7010/ks7010_sdio.c
> +++ b/drivers/staging/ks7010/ks7010_sdio.c
> @@ -548,7 +548,8 @@ static void ks_sdio_interrupt(struct sdio_func *func)
>                         if (atomic_read(&priv->psstatus.status) == PS_SNOOZE) {
>                                 if (cnt_txqbody(priv)) {
>                                         ks_wlan_hw_wakeup_request(priv);
> -                                       queue_delayed_work(priv->wq, &priv->rw_dwork, 1);
> +                                       queue_delayed_work(priv->wq,
> +                                                          &priv->rw_dwork, 1);
>                                         return;
>                                 }
>                         } else {
> @@ -693,15 +694,18 @@ static int ks7010_upload_firmware(struct ks_sdio_card *card)
>                 memcpy(rom_buf, fw_entry->data + n, size);
>
>                 offset = n;
> -               ret = ks7010_sdio_update_index(priv, KS7010_IRAM_ADDRESS + offset);
> +               ret = ks7010_sdio_update_index(priv,
> +                                              KS7010_IRAM_ADDRESS + offset);
>                 if (ret)
>                         goto release_firmware;
>
> -               ret = ks7010_sdio_write(priv, DATA_WINDOW, rom_buf, size);
> +               ret = ks7010_sdio_write(priv,
> +                                       DATA_WINDOW, rom_buf, size);
>                 if (ret)
>                         goto release_firmware;
>
> -               ret = ks7010_sdio_data_compare(priv, DATA_WINDOW, rom_buf, size);
> +               ret = ks7010_sdio_data_compare(priv,
> +                                              DATA_WINDOW, rom_buf, size);
>                 if (ret)
>                         goto release_firmware;
>
> @@ -889,7 +893,7 @@ static int ks7010_sdio_probe(struct sdio_func *func,
>         priv = netdev_priv(netdev);
>
>         card->priv = priv;
> -       SET_NETDEV_DEV(netdev, &card->func->dev);       /* for create sysfs symlinks */
> +       SET_NETDEV_DEV(netdev, &card->func->dev);/* for create sysfs symlinks */

I don't think this is much of an improvement for readability. Should
we move the comment about a bit?

>
>         /* private memory initialize */
>         priv->ks_sdio_card = card;
> @@ -923,7 +927,7 @@ static int ks7010_sdio_probe(struct sdio_func *func,
>         }
>
>         /* interrupt setting */
> -       /* clear Interrupt status write (ARMtoSD_InterruptPending FN1:00_0024) */
> +       /* clear Interrupt status write (ARMtoSD_InterruptPending FN1:00_0024)*/

This is a bit of a pointless change, isn't it? It also makes the comment uglier.

>         sdio_claim_host(func);
>         ret = ks7010_sdio_writeb(priv, INT_PENDING, 0xff);
>         sdio_release_host(func);
> @@ -1006,7 +1010,7 @@ static void ks7010_sdio_remove(struct sdio_func *func)
>         struct ks_sdio_card *card;
>         struct ks_wlan_private *priv;
>
> -       DPRINTK(1, "ks7010_sdio_remove()\n");
> +       DPRINTK(1, "%s()\n", __func__);

You might get a "one thing per patch please" for this. You wouldn't
have had to change this line if you'd strictly stuck to that.

Frans