Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <cover.1666995639.git.tanjubrunostar0@gmail.com>
 <0e6a307052d3a354a850a502e509f46baccdbe1e.1666995639.git.tanjubrunostar0@gmail.com>
 <Y1za5Gzr/uXsPF3N@kroah.com>
In-Reply-To: <Y1za5Gzr/uXsPF3N@kroah.com>
From:   Tanju Brunostar <tanjubrunostar0@gmail.com>
Date:   Sat, 29 Oct 2022 20:20:04 +0100
Message-ID: <CAHJEyKUdL2v4mGLcY-DnJEpzv0CH+hQmd7B=2DWeUak3DJQ6vA@mail.gmail.com>
Subject: Re: [PATCH v9 2/6] staging: vt6655: split long code lines in s_uGetRTSCTSDuration
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org,
        outreachy@lists.linux.dev
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, Oct 29, 2022 at 8:47 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Fri, Oct 28, 2022 at 11:23:23PM +0000, Tanjuate Brunostar wrote:
> > Increase code visibility by splitting long lines of code in the
> > function: s_uGetRTSCTSDuration
> >
> > Signed-off-by: Tanjuate Brunostar <tanjubrunostar0@gmail.com>
> > ---
> >  drivers/staging/vt6655/rxtx.c | 108 ++++++++++++++++++++++++----------
> >  1 file changed, 76 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/staging/vt6655/rxtx.c b/drivers/staging/vt6655/rxtx.c
> > index 7eb7c6eb5cf0..8e56a7ee8035 100644
> > --- a/drivers/staging/vt6655/rxtx.c
> > +++ b/drivers/staging/vt6655/rxtx.c
> > @@ -186,20 +186,29 @@ static __le16 get_rtscts_time(struct vnt_private *priv,
> >
> >       data_time = bb_get_frame_time(priv->preamble_type, pkt_type, frame_length, current_rate);
> >       if (rts_rsvtype == 0) { /* RTSTxRrvTime_bb */
> > -             rts_time = bb_get_frame_time(priv->preamble_type, pkt_type, 20, priv->byTopCCKBasicRate);
> > -             ack_time = bb_get_frame_time(priv->preamble_type, pkt_type, 14, priv->byTopCCKBasicRate);
> > +             rts_time = bb_get_frame_time(priv->preamble_type, pkt_type, 20,
> > +                                          priv->byTopCCKBasicRate);
> > +             ack_time = bb_get_frame_time(priv->preamble_type, pkt_type, 14,
> > +                                          priv->byTopCCKBasicRate);
>
> While I understand the feeling of "let's fix this warning by wrapping
> the line!" type of solution, overall, it's NOT the correct thing to do.
>
> Remember, coding style changes are to be done to make code easier to
> read and understand, not harder.  The original code here is easier to
> read, and you made it harder to understand.
>
> The "best" solution for this will be to fix up the line length by virtue
> of fixing up the incorrect variable names.  Here is the original lines:
>
>                 rts_time = bb_get_frame_time(priv->preamble_type, pkt_type, 20, priv->byTopCCKBasicRate);
>                 ack_time = bb_get_frame_time(priv->preamble_type, pkt_type, 14, priv->byTopCCKBasicRate);
>
> but if you were to fix up just 1 function and one variable name, look at
> what happens and checkpatch is happy with it:
>
>                 rts_time = get_frame_time(priv->preamble_type, pkt_type, 20, priv->top_basic_rate);
>                 ack_time = get_frame_time(priv->preamble_type, pkt_type, 14, priv->top_basic_rate);
>
> Or even better:
>                 type = priv->preamble_type;
>                 rate = priv->top_basic_rate;
>                 rts_time = get_frame_time(type, pkt_type, 20, rate);
>                 ack_time = get_frame_time(type, pkt_type, 14, rate);
>
> Look, no line-wrapping and isn't that easier to understand?  The second
> version here might even produce smaller compiled code overall, making it
> a even better solution.
>
> So step back, revisit this whole series with the goal of overall making
> the code better and easier to review.  Don't create a series just with
> the short-term goal of making checkpatch quiet.
>
> Hope this helps,
>
> greg k-h
It does help. Thank you.