Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190602162546.3470-1-benniciemanuel78@gmail.com>
In-Reply-To: <20190602162546.3470-1-benniciemanuel78@gmail.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Wed, 5 Jun 2019 11:41:03 +0300
Message-ID: <CAHp75VfEREXJTr_QpADTsqBr10t16SaJqeM+tbxp6QZgc8Gfjg@mail.gmail.com>
Subject: Re: [PATCH] pci: hotplug: ibmphp: Fix 'line over 80 characters' Warning
To:     benniciemanuel78@gmail.com
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Joe Perches <joe@perches.com>, Lukas Wunner <lukas@wunner.de>,
        Sebastian Ott <sebott@linux.ibm.com>,
        "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
        Arnd Bergmann <arnd@arndb.de>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, Jun 2, 2019 at 7:25 PM Emanuel Bennici
<benniciemanuel78@gmail.com> wrote:
>
> Fix checkpatch.pl 'line over 80 characters' Warning in ibmphp_ebda.c and
> ibmphp_hpc.c

I'm not sure it's needed.
And actually some of the changes are the regressions in order of
readability ratio.

> +                               debug("dev->device = %x, "
> +                                     "dev->subsystem_device = %x\n",

Don't split string literals like this.

> -       if ((pslot == NULL)
> -           || ((pstatus == NULL) && (cmd != READ_ALLSTAT) && (cmd != READ_BUSSTATUS))) {
> +       if ((pslot == NULL) || ((pstatus == NULL) && (cmd != READ_ALLSTAT) &&
> +                               (cmd != READ_BUSSTATUS))) {

No, no, this breaks readability a lot.
In the former it's clearly shown the logic between conditionals, while
in the latter it's a mess.

> +                                       err("%s - Error ctrl_read failed\n",
> +                                               __func__);

Really, no advantage on this line even for 80 characters.

> +                       err("%s - Exit Error:invalid bus, rc[%d]\n",
> +                               __func__, rc);

Same here.

> -               debug("%s - ctlr id[%x] physical[%lx] logical[%lx] i2c[%x]\n", __func__,
> -               ctlr_ptr->ctlr_id, (ulong) (ctlr_ptr->u.wpeg_ctlr.wpegbbar), (ulong) wpg_bbar,
> -               ctlr_ptr->u.wpeg_ctlr.i2c_addr);
> +               debug("%s - ctlr id[%x] physical[%lx] logical[%lx] i2c[%x]\n",
> +                       __func__, ctlr_ptr->ctlr_id,
> +                       (ulong) (ctlr_ptr->u.wpeg_ctlr.wpegbbar),
> +                       (ulong) wpg_bbar, ctlr_ptr->u.wpeg_ctlr.i2c_addr);

Here better to fix explicit casts (perhaps it means wrong specifiers
being used).

> +               if ((poldslot->status & 0x20) &&
> +                   (SLOT_CONNECT(poldslot->status) == HPC_SLOT_CONNECTED)
> +                   && (SLOT_PRESENT(poldslot->status)))

Here you are not consistent in a way where to put logical operation.

> +                               memcpy((void *) &myslot, (void *) pslot,
> +                                      sizeof(struct slot));

Why spaces after ) here?

> +                               debug("%s - call process_changeinstatus for"
> +                                     "slot[%d]\n", __func__, i);

Do not split string literals like this.

-- 
With Best Regards,
Andy Shevchenko