Re: [PATCH] staging: wfx: cleanup long lines in data_tx.c

[Jerome Pouiller]

Hello Suraj,

Thank you for your contribution.

On Wednesday 22 April 2020 17:39:00 CEST Suraj Upadhyay wrote:
> 
> Break lines with length over 80 characters to conform
> to the linux coding style. Issue found by checkpatch.
> 
> Signed-off-by: Suraj Upadhyay <usuraj35@xxxxxxxxx>
> ---
>  drivers/staging/wfx/data_tx.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> 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 = 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 = true;
> @@ -290,7 +291,8 @@ static void wfx_tx_fixup_rates(struct ieee80211_tx_rate *rates)
>                 if (rates[i].idx == -1) {
>                         rates[i].idx = 0;
>                         rates[i].count = 8; // == hw->max_rate_tries
> -                       rates[i].flags = rates[i - 1].flags & IEEE80211_TX_RC_MCS;
> +                       rates[i].flags = 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;
>  }
> 
> -static struct hif_ht_tx_parameters wfx_tx_get_tx_parms(struct wfx_dev *wdev, 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_info)

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 = &tx_info->driver_rates[0];
>         struct hif_ht_tx_parameters ret = { };
> @@ -381,7 +385,8 @@ static int wfx_tx_inner(struct wfx_vif *wvif, struct ieee80211_sta *sta,
>         hif_msg->id = HIF_REQ_ID_TX;
>         hif_msg->interface = wvif->id;
>         if (skb->len > wvif->wdev->hw_caps.size_inp_ch_buf) {
> -               dev_warn(wvif->wdev->dev, "requested frame size (%d) is larger than maximum supported (%d)\n",
> +               dev_warn(wvif->wdev->dev,
> +                        "requested frame size (%d) is larger than maximum 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 = 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 = atomic_add_return(1, &wvif->wdev->packet_id) & 0xFFFF;
         req->packet_id |= IEEE80211_SEQ_TO_SN(le16_to_cpu(hdr->seq_ctrl)) << 16;
         req->packet_id |= queue_id << 28;

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

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

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

>                                 rate->count, tx_count);
>                 if (tx_count <= rate->count && tx_count &&
>                     arg->txed_rate != wfx_get_hw_rate(wvif->wdev, rate))
> @@ -557,7 +564,8 @@ void wfx_tx_confirm_cb(struct wfx_vif *wvif, const struct hif_cnf_tx *arg)
>                 else
>                         tx_info->flags |= IEEE80211_TX_STAT_ACK;
>         } else if (arg->status == HIF_REQUEUE) {
> -               WARN(!arg->tx_result_flags.requeue, "incoherent status and 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 = false; // DTIM period elapsed
>                         schedule_work(&wvif->update_tim_work);
> @@ -595,7 +603,8 @@ void wfx_flush(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
>                 if (wait_event_timeout(wdev->tx_dequeue,
>                                        wfx_tx_queue_empty(wdev, queue, vif_id),
>                                        msecs_to_jiffies(1000)) <= 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
> 
> 


-- 
Jérôme Pouiller