On Wed, Sep 13, 2017 at 08:47:39AM +0200, Frans Klaver wrote:
> On Tue, Sep 12, 2017 at 2:40 AM, Liam Ryan <liamryandev@xxxxxxxxx> wrote:
> > Fix checkpath-reported unbalanced braces in the following areas
> >
> > 221: FILE: drivers/staging/rtl8712/hal_init.c:221:
> > 392: FILE: drivers/staging/rtl8712/os_intfs.c:392:
> > 363: FILE: drivers/staging/rtl8712/rtl8712_cmd.c:363:
> > 889: FILE: drivers/staging/rtl8712/rtl8712_recv.c:889:
> > 902: FILE: drivers/staging/rtl8712/rtl871x_cmd.c:902:
> > 84: FILE: drivers/staging/rtl8712/rtl871x_ioctl_set.c:84:
> > 580: FILE: drivers/staging/rtl8712/rtl871x_mlme.c:580:
> > 593: FILE: drivers/staging/rtl8712/usb_intf.c:593:
> >
> > Signed-off-by: Liam Ryan <liamryandev@xxxxxxxxx>
> > ---
> > This is my first patch and I have several doubts about style choices
> >
> > At line 216 of hal_init.c should opening brace follow comment instead?
> >
> > At line 577 of rtl871x_mlme.c should I bring logic to one line instead of
> > opening the brace on the continued line?
> >
> > At line 353 of rtl8712_cmd.c the if/else is technically only 1 line each.
> > Should the braces still have been added per checkpath for readability?
> >
> > drivers/staging/rtl8712/hal_init.c | 4 ++--
> > drivers/staging/rtl8712/os_intfs.c | 4 ++--
> > drivers/staging/rtl8712/rtl8712_cmd.c | 4 ++--
> > drivers/staging/rtl8712/rtl8712_recv.c | 4 ++--
> > drivers/staging/rtl8712/rtl871x_cmd.c | 3 ++-
> > drivers/staging/rtl8712/rtl871x_ioctl_set.c | 4 ++--
> > drivers/staging/rtl8712/rtl871x_mlme.c | 4 ++--
> > drivers/staging/rtl8712/usb_intf.c | 3 ++-
> > 8 files changed, 16 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8712/hal_init.c b/drivers/staging/rtl8712/hal_init.c
> > index c83d7eb..de832b0b5 100644
> > --- a/drivers/staging/rtl8712/hal_init.c
> > +++ b/drivers/staging/rtl8712/hal_init.c
> > @@ -216,9 +216,9 @@ static u8 rtl8712_dl_fw(struct _adapter *padapter)
> > emem_sz = fwhdr.img_SRAM_size;
> > do {
> > memset(ptx_desc, 0, TXDESC_SIZE);
> > - if (emem_sz > MAX_DUMP_FWSZ) /* max=48k */
> > + if (emem_sz > MAX_DUMP_FWSZ) { /* max=48k */
>
> I would not have the comment there in the first place. It doesn't add
> any new information and just messes up the style.
I'm happy to remove the comment, the patch was accepted so I'm not sure
of procedure, should it be a new patch or a revision to this patch?
In general I don't have the knowledge to decide which comments are worth keeping in the source
code, is there a rule of thumb for brace placement near inline comments such as this? There's
another example in rtl8712_cmd.c below for reference.
>
> > dump_emem_sz = MAX_DUMP_FWSZ;
> > - else {
> > + } else {
> > dump_emem_sz = emem_sz;
> > ptx_desc->txdw0 |= cpu_to_le32(BIT(28));
> > }
> > diff --git a/drivers/staging/rtl8712/os_intfs.c b/drivers/staging/rtl8712/os_intfs.c
> > index e698f6e..990a343 100644
> > --- a/drivers/staging/rtl8712/os_intfs.c
> > +++ b/drivers/staging/rtl8712/os_intfs.c
> > @@ -385,11 +385,11 @@ static int netdev_open(struct net_device *pnetdev)
> > padapter->bup = true;
> > if (rtl871x_hal_init(padapter) != _SUCCESS)
> > goto netdev_open_error;
> > - if (!r8712_initmac)
> > + if (!r8712_initmac) {
> > /* Use the mac address stored in the Efuse */
> > memcpy(pnetdev->dev_addr,
> > padapter->eeprompriv.mac_addr, ETH_ALEN);
> > - else {
> > + } else {
> > /* We have to inform f/w to use user-supplied MAC
> > * address.
> > */
> > diff --git a/drivers/staging/rtl8712/rtl8712_cmd.c b/drivers/staging/rtl8712/rtl8712_cmd.c
> > index 0104ace..3c88994 100644
> > --- a/drivers/staging/rtl8712/rtl8712_cmd.c
> > +++ b/drivers/staging/rtl8712/rtl8712_cmd.c
> > @@ -356,11 +356,11 @@ int r8712_cmd_thread(void *context)
> > if ((wr_sz % 64) == 0)
> > blnPending = 1;
> > }
> > - if (blnPending) /* 32 bytes for TX Desc - 8 offset */
> > + if (blnPending) { /* 32 bytes for TX Desc - 8 offset */
> > pdesc->txdw0 |= cpu_to_le32(((TXDESC_SIZE +
> > OFFSET_SZ + 8) << OFFSET_SHT) &
> > 0x00ff0000);
> > - else {
> > + } else {
> > pdesc->txdw0 |= cpu_to_le32(((TXDESC_SIZE +
> > OFFSET_SZ) <<
> > OFFSET_SHT) &
>
> I would add the braces. I think once you have multiple lines, even
> though it's just one statement, braces are in order, so as to avoid
> any confusion.
>
Thanks, this is the other example of inline comments with braces I
referred to above
>
> > diff --git a/drivers/staging/rtl8712/rtl8712_recv.c b/drivers/staging/rtl8712/rtl8712_recv.c
> > index ea3eb94..0978727 100644
> > --- a/drivers/staging/rtl8712/rtl8712_recv.c
> > +++ b/drivers/staging/rtl8712/rtl8712_recv.c
> > @@ -883,10 +883,10 @@ static void query_rx_phy_status(struct _adapter *padapter,
> > * from 0~100. It is assigned to the BSS List in
> > * GetValueFromBeaconOrProbeRsp().
> > */
> > - if (bcck_rate)
> > + if (bcck_rate) {
> > prframe->u.hdr.attrib.signal_strength =
> > (u8)r8712_signal_scale_mapping(pwdb_all);
> > - else {
> > + } else {
> > if (rf_rx_num != 0)
> > prframe->u.hdr.attrib.signal_strength =
> > (u8)(r8712_signal_scale_mapping(total_rssi /=
> > diff --git a/drivers/staging/rtl8712/rtl871x_cmd.c b/drivers/staging/rtl8712/rtl871x_cmd.c
> > index 04638f1..a424f447 100644
> > --- a/drivers/staging/rtl8712/rtl871x_cmd.c
> > +++ b/drivers/staging/rtl8712/rtl871x_cmd.c
> > @@ -899,9 +899,10 @@ void r8712_createbss_cmd_callback(struct _adapter *padapter,
> > if (!pwlan)
> > goto createbss_cmd_fail;
> > pwlan->last_scanned = jiffies;
> > - } else
> > + } else {
> > list_add_tail(&(pwlan->list),
> > &pmlmepriv->scanned_queue.queue);
> > + }
> > pnetwork->Length = r8712_get_wlan_bssid_ex_sz(pnetwork);
> > memcpy(&(pwlan->network), pnetwork, pnetwork->Length);
> > pwlan->fixed = true;
> > diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_set.c b/drivers/staging/rtl8712/rtl871x_ioctl_set.c
> > index 01a1504..8a5ced4 100644
> > --- a/drivers/staging/rtl8712/rtl871x_ioctl_set.c
> > +++ b/drivers/staging/rtl8712/rtl871x_ioctl_set.c
> > @@ -78,10 +78,10 @@ static u8 do_join(struct _adapter *padapter)
> > int ret;
> >
> > ret = r8712_select_and_join_from_scan(pmlmepriv);
> > - if (ret == _SUCCESS)
> > + if (ret == _SUCCESS) {
> > mod_timer(&pmlmepriv->assoc_timer,
> > jiffies + msecs_to_jiffies(MAX_JOIN_TIMEOUT));
> > - else {
> > + } else {
> > if (check_fwstate(pmlmepriv, WIFI_ADHOC_STATE)) {
> > /* submit r8712_createbss_cmd to change to an
> > * ADHOC_MASTER pmlmepriv->lock has been
> > diff --git a/drivers/staging/rtl8712/rtl871x_mlme.c b/drivers/staging/rtl8712/rtl871x_mlme.c
> > index bf1ac22..111c809 100644
> > --- a/drivers/staging/rtl8712/rtl871x_mlme.c
> > +++ b/drivers/staging/rtl8712/rtl871x_mlme.c
> > @@ -574,10 +574,10 @@ void r8712_surveydone_event_callback(struct _adapter *adapter, u8 *pbuf)
> > set_fwstate(pmlmepriv, _FW_UNDER_LINKING);
> >
> > if (r8712_select_and_join_from_scan(pmlmepriv)
> > - == _SUCCESS)
> > + == _SUCCESS) {
>
> I think this merits another patch to get rid of the rather generic
> SUCCESS, and translate it into a normal kernel return value, so that
> we can just say !r8712_select_and_join_from_scan(). Although I think
> the function name is also a possible target for a rename. I'd leave it
> like it is in this patch.
>
>
> > mod_timer(&pmlmepriv->assoc_timer, jiffies +
> > msecs_to_jiffies(MAX_JOIN_TIMEOUT));
> > - else {
> > + } else {
> > struct wlan_bssid_ex *pdev_network =
> > &(adapter->registrypriv.dev_network);
> > u8 *pibss =
> > diff --git a/drivers/staging/rtl8712/usb_intf.c b/drivers/staging/rtl8712/usb_intf.c
> > index b3e266b..85eaddd 100644
> > --- a/drivers/staging/rtl8712/usb_intf.c
> > +++ b/drivers/staging/rtl8712/usb_intf.c
> > @@ -590,9 +590,10 @@ static int r871xu_drv_init(struct usb_interface *pusb_intf,
> > mac[0] &= 0xFE;
> > dev_info(&udev->dev,
> > "r8712u: MAC Address from user = %pM\n", mac);
> > - } else
> > + } else {
> > dev_info(&udev->dev,
> > "r8712u: MAC Address from efuse = %pM\n", mac);
> > + }
> > ether_addr_copy(pnetdev->dev_addr, mac);
> > }
> > /* step 6. Load the firmware asynchronously */
> > --
> > 2.7.4
> >
> > _______________________________________________
> > devel mailing list
> > devel@xxxxxxxxxxxxxxxxxxxxxx
> > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
