From: Oscar Carter <oscar.carter@gmx.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Oscar Carter <oscar.carter@gmx.com>,
Forest Bond <forest@alittletooquiet.net>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
devel@driverdev.osuosl.org,
Malcolm Priestley <tvboxspy@gmail.com>,
linux-kernel@vger.kernel.org,
Gabriela Bittencourt <gabrielabittencourt00@gmail.com>,
Colin Ian King <colin.king@canonical.com>
Subject: Re: [PATCH] staging: vt6656: Use BIT() macro in vnt_mac_reg_bits_* functions
Date: Thu, 26 Mar 2020 18:10:43 +0100 [thread overview]
Message-ID: <20200326171043.GB3629@ubuntu> (raw)
In-Reply-To: <20200323073214.GJ4650@kadam>
On Mon, Mar 23, 2020 at 10:32:14AM +0300, Dan Carpenter wrote:
> On Fri, Mar 20, 2020 at 07:13:26PM +0100, Oscar Carter wrote:
> > +#include <linux/bits.h>
> > #include "mac.h"
> > #include "baseband.h"
> > #include "rf.h"
> > @@ -468,7 +469,7 @@ int vnt_vt3184_init(struct vnt_private *priv)
> > if (ret)
> > goto end;
> >
> > - ret = vnt_mac_reg_bits_on(priv, MAC_REG_PAPEDELAY, 0x01);
> > + ret = vnt_mac_reg_bits_on(priv, MAC_REG_PAPEDELAY, BIT(0));
>
> Everyone knows 0x01 is bit(0) already. This isn't more clear. It
> should be a define instead of a magic number.
>
I agree. I create a new define for this case.
> > @@ -63,7 +64,8 @@ void vnt_set_channel(struct vnt_private *priv, u32 connection_channel)
> > vnt_mac_reg_bits_on(priv, MAC_REG_MACCR, MACCR_CLRNAV);
> >
> > /* Set Channel[7] = 0 to tell H/W channel is changing now. */
> > - vnt_mac_reg_bits_off(priv, MAC_REG_CHANNEL, 0xb0);
> > + vnt_mac_reg_bits_off(priv, MAC_REG_CHANNEL,
> > + (BIT(7) | BIT(5) | BIT(4)));
>
> This one especially is just a lot longer now but still not clear.
>
Like the previous one, i create a define. In this case to avoid the magic
number or the OR operation between BIT macros.
> regards,
> dan carpenter
>
I will make these changes and i will send and incremental patch with the
"Fixes:" tag due to the this patch has already been added to the staging-next
branch of the greg staging tree.
thanks,
oscar carter
WARNING: multiple messages have this Message-ID (diff)
From: Oscar Carter <oscar.carter@gmx.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: devel@driverdev.osuosl.org, Oscar Carter <oscar.carter@gmx.com>,
Malcolm Priestley <tvboxspy@gmail.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
linux-kernel@vger.kernel.org,
Forest Bond <forest@alittletooquiet.net>,
Gabriela Bittencourt <gabrielabittencourt00@gmail.com>,
Colin Ian King <colin.king@canonical.com>
Subject: Re: [PATCH] staging: vt6656: Use BIT() macro in vnt_mac_reg_bits_* functions
Date: Thu, 26 Mar 2020 18:10:43 +0100 [thread overview]
Message-ID: <20200326171043.GB3629@ubuntu> (raw)
In-Reply-To: <20200323073214.GJ4650@kadam>
On Mon, Mar 23, 2020 at 10:32:14AM +0300, Dan Carpenter wrote:
> On Fri, Mar 20, 2020 at 07:13:26PM +0100, Oscar Carter wrote:
> > +#include <linux/bits.h>
> > #include "mac.h"
> > #include "baseband.h"
> > #include "rf.h"
> > @@ -468,7 +469,7 @@ int vnt_vt3184_init(struct vnt_private *priv)
> > if (ret)
> > goto end;
> >
> > - ret = vnt_mac_reg_bits_on(priv, MAC_REG_PAPEDELAY, 0x01);
> > + ret = vnt_mac_reg_bits_on(priv, MAC_REG_PAPEDELAY, BIT(0));
>
> Everyone knows 0x01 is bit(0) already. This isn't more clear. It
> should be a define instead of a magic number.
>
I agree. I create a new define for this case.
> > @@ -63,7 +64,8 @@ void vnt_set_channel(struct vnt_private *priv, u32 connection_channel)
> > vnt_mac_reg_bits_on(priv, MAC_REG_MACCR, MACCR_CLRNAV);
> >
> > /* Set Channel[7] = 0 to tell H/W channel is changing now. */
> > - vnt_mac_reg_bits_off(priv, MAC_REG_CHANNEL, 0xb0);
> > + vnt_mac_reg_bits_off(priv, MAC_REG_CHANNEL,
> > + (BIT(7) | BIT(5) | BIT(4)));
>
> This one especially is just a lot longer now but still not clear.
>
Like the previous one, i create a define. In this case to avoid the magic
number or the OR operation between BIT macros.
> regards,
> dan carpenter
>
I will make these changes and i will send and incremental patch with the
"Fixes:" tag due to the this patch has already been added to the staging-next
branch of the greg staging tree.
thanks,
oscar carter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
next prev parent reply other threads:[~2020-03-26 17:10 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-20 18:13 [PATCH] staging: vt6656: Use BIT() macro in vnt_mac_reg_bits_* functions Oscar Carter
2020-03-20 18:13 ` Oscar Carter
2020-03-23 7:32 ` Dan Carpenter
2020-03-23 7:32 ` Dan Carpenter
2020-03-26 17:10 ` Oscar Carter [this message]
2020-03-26 17:10 ` Oscar Carter
2020-03-31 10:41 ` Dan Carpenter
2020-03-31 10:41 ` Dan Carpenter
2020-04-01 15:31 ` Oscar Carter
2020-04-01 15:31 ` Oscar Carter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200326171043.GB3629@ubuntu \
--to=oscar.carter@gmx.com \
--cc=colin.king@canonical.com \
--cc=dan.carpenter@oracle.com \
--cc=devel@driverdev.osuosl.org \
--cc=forest@alittletooquiet.net \
--cc=gabrielabittencourt00@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=tvboxspy@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.