linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jakub Kicinski <jakub.kicinski@netronome.com>
To: Kalle Valo <kvalo@codeaurora.org>,
	Linux Wireless List <linux-wireless@vger.kernel.org>
Cc: Jakub Kicinski <jakub.kicinski@netronome.com>
Subject: [PATCHv7 0/4] register-field manipulation macros
Date: Fri, 19 Aug 2016 17:44:05 +0100	[thread overview]
Message-ID: <1471625049-12643-1-git-send-email-jakub.kicinski@netronome.com> (raw)

Hi!

Looks like we're good to go :)

I've thrown in two mt7601u cleanups, macros are as approved
by Linus in the RFC.  I'm posting this already to give build
bot a head start while we wait for any additional feedback
on LKML.

-- v6 blurb:

This set moves to a global header file macros which I find
very useful and worth popularising.  The basic problem is
that since C bitfields are not very dependable accessing
subfields of registers becomes slightly inconvenient.
It is nice to have the necessary mask and shift operations
wrapped in a macro and have that macro compute shift
at compilation time using FFS.

My implementation follows what Felix Fietkau has done in
mt76.  Hannes Frederic Sowa suggested more use of standard
Linux/GCC functions.  Since the RFC I've also added a 
compile-time check to validate that the value passed to
setters fits in the mask.

I attempted the use of static inlines instead of macros
but it makes GCC < 6.0 barf at the BUILD_BUG_ON()s.
I also noticed that forcing arguments to be u32 for inlines
makes the compiler use 32bit arithmetic where it could
get away with 64bit before (on 64bit machines, obviously).
That's a potential performance concern but probably not
a very practical one today.  Apart from looking "cleaner"
static inlines would have the advantage that we could #undef
the auxiliary macros at the end of the header.

Build bot caught a build failure with -Os set.  AFAICT gcc
did not handle temporary variable I put in the macro
expression too well.  I work around that by defining
__BUILD_BUG_ON_NOT_POWER_OF_2 and using it instead of
BUILD_BUG_ON(!tmp || is_power_of_2(tmp)).

I'm planning to use those macros in another driver soon,
Felix may also use them when upstreaming mt76 if he chooses
to do so.

IMHO if accepted it would be best to push these through
Kalle's wireless drivers' tree, since the only existing
user is in drivers/net/wireless/.

v7:
 - drop the explicit type marking (u32/u64) - depend on the type
   of the mask instead;
 - only allow compilation time constant masks;
 - barf at "type of register too small to ever match mask" on get;
 - rename PUT -> PREP.
v6:
 - do a full rename in patch 2;
 - CC many people.
v5:
 - repost.
v4:
 - add documentation in the header.
v3:
 - don't use variables in statement expressions;
 - use __BUILD_BUG_ON_NOT_POWER_OF_2.
v2:
 - change Felix's email address.

Jakub Kicinski (4):
  add basic register-field manipulation macros
  mt7601u: remove redefinition of GENMASK
  mt7601u: remove unnecessary include
  mt7601u: use linux/bitfield.h

 drivers/net/wireless/mediatek/mt7601u/dma.c     |  2 +-
 drivers/net/wireless/mediatek/mt7601u/dma.h     | 10 ++-
 drivers/net/wireless/mediatek/mt7601u/eeprom.c  | 12 ++--
 drivers/net/wireless/mediatek/mt7601u/init.c    | 10 +--
 drivers/net/wireless/mediatek/mt7601u/mac.c     | 38 +++++-----
 drivers/net/wireless/mediatek/mt7601u/main.c    |  1 -
 drivers/net/wireless/mediatek/mt7601u/mcu.c     | 20 +++---
 drivers/net/wireless/mediatek/mt7601u/mt7601u.h |  4 +-
 drivers/net/wireless/mediatek/mt7601u/phy.c     | 44 ++++++------
 drivers/net/wireless/mediatek/mt7601u/regs.h    |  4 --
 drivers/net/wireless/mediatek/mt7601u/tx.c      | 19 ++---
 drivers/net/wireless/mediatek/mt7601u/util.h    | 77 --------------------
 include/linux/bitfield.h                        | 93 +++++++++++++++++++++++++
 include/linux/bug.h                             |  3 +
 14 files changed, 177 insertions(+), 160 deletions(-)
 delete mode 100644 drivers/net/wireless/mediatek/mt7601u/util.h
 create mode 100644 include/linux/bitfield.h

-- 
1.9.1

             reply	other threads:[~2016-08-19 16:44 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-08-19 16:44 Jakub Kicinski [this message]
2016-08-19 16:44 ` [PATCHv7 1/4] add basic register-field manipulation macros Jakub Kicinski
2016-08-19 16:44 ` [PATCHv7 2/4] mt7601u: remove redefinition of GENMASK Jakub Kicinski
2016-08-19 16:44 ` [PATCHv7 3/4] mt7601u: remove unnecessary include Jakub Kicinski
2016-08-19 16:44 ` [PATCHv7 4/4] mt7601u: use linux/bitfield.h Jakub Kicinski
2016-08-19 19:02   ` Arend Van Spriel
2016-08-22 10:52     ` Jakub Kicinski
2016-08-22 19:19       ` Arend Van Spriel
2016-08-22 20:59         ` Jakub Kicinski
2016-08-31 11:46 ` [PATCHv8 0/4] register-field manipulation macros Jakub Kicinski
2016-08-31 11:46   ` [PATCHv8 1/4] add basic " Jakub Kicinski
2016-09-09  9:11     ` [PATCHv8,1/4] " Kalle Valo
2016-08-31 11:46   ` [PATCHv8 2/4] mt7601u: remove redefinition of GENMASK Jakub Kicinski
2016-08-31 11:46   ` [PATCHv8 3/4] mt7601u: remove unnecessary include Jakub Kicinski
2016-08-31 11:46   ` [PATCHv8 4/4] mt7601u: use linux/bitfield.h Jakub Kicinski
2016-09-03 11:05   ` [PATCHv8 0/4] register-field manipulation macros Kalle Valo

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=1471625049-12643-1-git-send-email-jakub.kicinski@netronome.com \
    --to=jakub.kicinski@netronome.com \
    --cc=kvalo@codeaurora.org \
    --cc=linux-wireless@vger.kernel.org \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).