Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Jerome Pouiller <Jerome.Pouiller@silabs.com>
To:     Suraj Upadhyay <usuraj35@gmail.com>
CC:     "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "devel@driverdev.osuosl.org" <devel@driverdev.osuosl.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] staging: wfx: cleanup long lines in data_tx.c
Thread-Topic: [PATCH] staging: wfx: cleanup long lines in data_tx.c
Thread-Index: AQHWGMCFqMdP/B3JUUC6zI1I3nAVZaiGcdsA
Date:   Thu, 23 Apr 2020 09:29:28 +0000
Message-ID: <2253201.upfRyW1aZW@pc-42>
References: <20200422153900.GA6184@blackclown>
In-Reply-To: <20200422153900.GA6184@blackclown>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: silabs.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="iso-8859-1"
Content-ID: <BC1F2FA59F0CC24EA218433A0B01B8D3@namprd11.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 178ce78d-335b-4b44-2892-08d7e768d27d
X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Apr 2020 09:29:28.6968
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 54dbd822-5231-4b20-944d-6f4abcd541fb
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Djy0P7/WPCuUEtpPfBqkR0y3XEybQx2vbA5eTsdDAiVMbHDxOI7QImyDtD0h8vxr5A3gsycY5wEw4c5iXWTHSA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR11MB1390
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hello Suraj,

Thank you for your contribution.

On Wednesday 22 April 2020 17:39:00 CEST Suraj Upadhyay wrote:
>=20
> Break lines with length over 80 characters to conform
> to the linux coding style. Issue found by checkpatch.
>=20
> Signed-off-by: Suraj Upadhyay <usuraj35@gmail.com>
> ---
>  drivers/staging/wfx/data_tx.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
>=20
> diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.=
c
> index c30e4f5b6e2d..85ad94de20e5 100644
> --- a/drivers/staging/wfx/data_tx.c
> +++ b/drivers/staging/wfx/data_tx.c
> @@ -165,7 +165,8 @@ static int wfx_tx_policy_upload(struct wfx_vif *wvif)
>                 spin_lock_bh(&wvif->tx_policy_cache.lock);
>                 for (i =3D 0; i < HIF_TX_RETRY_POLICY_MAX; ++i)
>                         if (!policies[i].uploaded &&
> -                           memzcmp(policies[i].rates, sizeof(policies[i]=
.rates)))
> +                           memzcmp(policies[i].rates,
> +                                   sizeof(policies[i].rates)))

hmm... since memzcmp() is called from a if condition, I am not sure that
breaking the line improves the readability. May be you should introduce a
temporary variable instead.

Before that line, I also see a line that exceed 80 columns in
wfx_get_hw_rate() could also address it?


>                                 break;
>                 if (i < HIF_TX_RETRY_POLICY_MAX) {
>                         policies[i].uploaded =3D true;
> @@ -290,7 +291,8 @@ static void wfx_tx_fixup_rates(struct ieee80211_tx_ra=
te *rates)
>                 if (rates[i].idx =3D=3D -1) {
>                         rates[i].idx =3D 0;
>                         rates[i].count =3D 8; // =3D=3D hw->max_rate_trie=
s
> -                       rates[i].flags =3D rates[i - 1].flags & IEEE80211=
_TX_RC_MCS;
> +                       rates[i].flags =3D rates[i - 1].flags &
> +                                        IEEE80211_TX_RC_MCS;

Ack.

>                         break;
>                 }
>         }
> @@ -318,7 +320,9 @@ static u8 wfx_tx_get_rate_id(struct wfx_vif *wvif,
>         return rate_id;
>  }
>=20
> -static struct hif_ht_tx_parameters wfx_tx_get_tx_parms(struct wfx_dev *w=
dev, struct ieee80211_tx_info *tx_info)
> +static struct
> +hif_ht_tx_parameters wfx_tx_get_tx_parms(struct wfx_dev *wdev,
> +                                        struct ieee80211_tx_info *tx_inf=
o)

Please, dont't break between 'struct' and name of the struct. In add, in
the rest of the file, there is no line break between the returned type the
name of the function. Let's try to keep it uniform.

>  {
>         struct ieee80211_tx_rate *rate =3D &tx_info->driver_rates[0];
>         struct hif_ht_tx_parameters ret =3D { };
> @@ -381,7 +385,8 @@ static int wfx_tx_inner(struct wfx_vif *wvif, struct =
ieee80211_sta *sta,
>         hif_msg->id =3D HIF_REQ_ID_TX;
>         hif_msg->interface =3D wvif->id;
>         if (skb->len > wvif->wdev->hw_caps.size_inp_ch_buf) {
> -               dev_warn(wvif->wdev->dev, "requested frame size (%d) is l=
arger than maximum supported (%d)\n",
> +               dev_warn(wvif->wdev->dev,
> +                        "requested frame size (%d) is larger than maximu=
m supported (%d)\n",
>                          skb->len, wvif->wdev->hw_caps.size_inp_ch_buf);
>                 skb_pull(skb, wmsg_len);
>                 return -EIO;
> @@ -394,7 +399,8 @@ static int wfx_tx_inner(struct wfx_vif *wvif, struct =
ieee80211_sta *sta,
>         // data for debug.
>         req->packet_id =3D queue_id << 28 |
>                          IEEE80211_SEQ_TO_SN(le16_to_cpu(hdr->seq_ctrl)) =
<< 16 |
> -                        (atomic_add_return(1, &wvif->wdev->packet_id) & =
0xFFFF);
> +                        (atomic_add_return(1, &wvif->wdev->packet_id) &
> +                         0xFFFF);

Since the line is already split on '|', I am not sure that adding a break
here improve the readability. Maybe:

         req->packet_id =3D atomic_add_return(1, &wvif->wdev->packet_id) & =
0xFFFF;
         req->packet_id |=3D IEEE80211_SEQ_TO_SN(le16_to_cpu(hdr->seq_ctrl)=
) << 16;
         req->packet_id |=3D queue_id << 28;

or maybe if you introduce "struct wfx_dev *wdev =3D wvif->wdev;", you would
simplify the things.

>         req->data_flags.fc_offset =3D offset;
>         if (tx_info->flags & IEEE80211_TX_CTL_SEND_AFTER_DTIM)
>                 req->data_flags.after_dtim =3D 1;
> @@ -517,7 +523,8 @@ void wfx_tx_confirm_cb(struct wfx_vif *wvif, const st=
ruct hif_cnf_tx *arg)
>                 if (tx_count < rate->count &&
>                     arg->status =3D=3D HIF_STATUS_RETRY_EXCEEDED &&
>                     arg->ack_failures)
> -                       dev_dbg(wvif->wdev->dev, "all retries were not co=
nsumed: %d !=3D %d\n",
> +                       dev_dbg(wvif->wdev->dev,
> +                               "all retries were not consumed: %d !=3D %=
d\n",

Is this reported by checkpatch? The strings can exceed 80 columns.

>                                 rate->count, tx_count);
>                 if (tx_count <=3D rate->count && tx_count &&
>                     arg->txed_rate !=3D wfx_get_hw_rate(wvif->wdev, rate)=
)
> @@ -557,7 +564,8 @@ void wfx_tx_confirm_cb(struct wfx_vif *wvif, const st=
ruct hif_cnf_tx *arg)
>                 else
>                         tx_info->flags |=3D IEEE80211_TX_STAT_ACK;
>         } else if (arg->status =3D=3D HIF_REQUEUE) {
> -               WARN(!arg->tx_result_flags.requeue, "incoherent status an=
d result_flags");
> +               WARN(!arg->tx_result_flags.requeue,
> +                    "incoherent status and result_flags");

Ditto

>                 if (tx_info->flags & IEEE80211_TX_CTL_SEND_AFTER_DTIM) {
>                         wvif->after_dtim_tx_allowed =3D false; // DTIM pe=
riod elapsed
>                         schedule_work(&wvif->update_tim_work);
> @@ -595,7 +603,8 @@ void wfx_flush(struct ieee80211_hw *hw, struct ieee80=
211_vif *vif,
>                 if (wait_event_timeout(wdev->tx_dequeue,
>                                        wfx_tx_queue_empty(wdev, queue, vi=
f_id),
>                                        msecs_to_jiffies(1000)) <=3D 0)
> -                       dev_warn(wdev->dev, "frames queued while flushing=
 tx queues?");
> +                       dev_warn(wdev->dev,
> +                                "frames queued while flushing tx queues?=
");

Ditto

>         }
>         wfx_tx_flush(wdev);
>         if (wdev->chip_frozen)
> --
> 2.17.1
>=20
>=20


--=20
J=E9r=F4me Pouiller