* [PATCH 2/2] rtl8180: don't use weird trick to access "far" registers [not found] <20140328093038.GD7045@mwanda> @ 2014-03-28 17:14 ` Andrea Merello 2014-04-02 19:24 ` Dave Kilroy 0 siblings, 1 reply; 3+ messages in thread From: Andrea Merello @ 2014-03-28 17:14 UTC (permalink / raw) To: dan.carpenter, linville; +Cc: johannes, linux-wireless, andrea.merello In rtl8180/rtl8185/rtl8187se the register space is represented using packed structure type. Register are thus accessed using a pointer of this type. All registers are packed toghether, and only small gaps are present. However Rtl8187se has also some "sparse" registers, very far from the "main register block". It could be possible to access them by simply declare huge reserved blocks inside the register struct (and this causes NO memory waste). However, for various reasons, access to those "far" registers is done with special dedicated macros, without declaring them in the register struct. This is done in an intricate manner, that makes code less readable and caused static analisys tool to produce warnings. This patch keeps the "macro" mechanism, but it changes its implementation in a simplier and more straightforward way. Signed-off-by: Andrea Merello <andrea.merello@gmail.com> --- drivers/net/wireless/rtl818x/rtl818x.h | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/drivers/net/wireless/rtl818x/rtl818x.h b/drivers/net/wireless/rtl818x/rtl818x.h index 99dbc12..45ea4e1 100644 --- a/drivers/net/wireless/rtl818x/rtl818x.h +++ b/drivers/net/wireless/rtl818x/rtl818x.h @@ -17,13 +17,7 @@ struct rtl818x_csr { - union { - u8 MAC[6]; - u8 offset1[6]; /* upper page indexing helpers */ - __le16 offset2[1]; - __le32 offset4[1]; - } __packed; - + u8 MAC[6]; u8 reserved_0[2]; union { @@ -340,9 +334,9 @@ struct rtl818x_csr { * I don't like to introduce a ton of "reserved".. * They are for RTL8187SE */ -#define REG_ADDR1(addr) ((u8 __iomem *)(&priv->map->offset1[(addr)])) -#define REG_ADDR2(addr) ((__le16 __iomem *)(&priv->map->offset2[((addr) >> 1)])) -#define REG_ADDR4(addr) ((__le32 __iomem *)(&priv->map->offset4[((addr) >> 2)])) +#define REG_ADDR1(addr) ((u8 __iomem *)priv->map + addr) +#define REG_ADDR2(addr) ((__le16 __iomem *)priv->map + (addr >> 1)) +#define REG_ADDR4(addr) ((__le32 __iomem *)priv->map + (addr >> 2)) #define FEMR_SE REG_ADDR2(0x1D4) #define ARFR REG_ADDR2(0x1E0) -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 2/2] rtl8180: don't use weird trick to access "far" registers 2014-03-28 17:14 ` [PATCH 2/2] rtl8180: don't use weird trick to access "far" registers Andrea Merello @ 2014-04-02 19:24 ` Dave Kilroy 2014-04-04 16:24 ` [PATCH 3/4] rtl8180: add parentheses to REG_ADDR macros Andrea Merello 0 siblings, 1 reply; 3+ messages in thread From: Dave Kilroy @ 2014-04-02 19:24 UTC (permalink / raw) To: Andrea Merello, dan.carpenter, linville; +Cc: johannes, linux-wireless On 28/03/2014 17:14, Andrea Merello wrote: > @@ -340,9 +334,9 @@ struct rtl818x_csr { > * I don't like to introduce a ton of "reserved".. > * They are for RTL8187SE > */ > -#define REG_ADDR1(addr) ((u8 __iomem *)(&priv->map->offset1[(addr)])) > -#define REG_ADDR2(addr) ((__le16 __iomem *)(&priv->map->offset2[((addr) >> 1)])) > -#define REG_ADDR4(addr) ((__le32 __iomem *)(&priv->map->offset4[((addr) >> 2)])) > +#define REG_ADDR1(addr) ((u8 __iomem *)priv->map + addr) > +#define REG_ADDR2(addr) ((__le16 __iomem *)priv->map + (addr >> 1)) > +#define REG_ADDR4(addr) ((__le32 __iomem *)priv->map + (addr >> 2)) > > #define FEMR_SE REG_ADDR2(0x1D4) > #define ARFR REG_ADDR2(0x1E0) I suggest parenthesizing the use of addr in your macros (as the original code does), to avoid any issues with operator precedence wrt >>. If the removal was intentional and you've verified there's no issues, you should probably mention it in the commit message. Dave. ^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH 3/4] rtl8180: add parentheses to REG_ADDR macros 2014-04-02 19:24 ` Dave Kilroy @ 2014-04-04 16:24 ` Andrea Merello 0 siblings, 0 replies; 3+ messages in thread From: Andrea Merello @ 2014-04-04 16:24 UTC (permalink / raw) To: linville, kilroyd Cc: linux-wireless, Larry.Finger, bernhard, dan.carpenter, andrea.merello Parentheses are missing around the macro argument, causing the macro possibly not to work passing certain expressions as arguments. This should not cause any issues with current code, however it's worth to add them, as a good practice, and to eventually avoid future bugs. Suggested-by: Dave Kilroy <kilroyd@googlemail.com> Signed-off-by: Andrea Merello <andrea.merello@gmail.com> --- drivers/net/wireless/rtl818x/rtl818x.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/net/wireless/rtl818x/rtl818x.h b/drivers/net/wireless/rtl818x/rtl818x.h index 45ea4e1..7abef95 100644 --- a/drivers/net/wireless/rtl818x/rtl818x.h +++ b/drivers/net/wireless/rtl818x/rtl818x.h @@ -334,9 +334,9 @@ struct rtl818x_csr { * I don't like to introduce a ton of "reserved".. * They are for RTL8187SE */ -#define REG_ADDR1(addr) ((u8 __iomem *)priv->map + addr) -#define REG_ADDR2(addr) ((__le16 __iomem *)priv->map + (addr >> 1)) -#define REG_ADDR4(addr) ((__le32 __iomem *)priv->map + (addr >> 2)) +#define REG_ADDR1(addr) ((u8 __iomem *)priv->map + (addr)) +#define REG_ADDR2(addr) ((__le16 __iomem *)priv->map + ((addr) >> 1)) +#define REG_ADDR4(addr) ((__le32 __iomem *)priv->map + ((addr) >> 2)) #define FEMR_SE REG_ADDR2(0x1D4) #define ARFR REG_ADDR2(0x1E0) -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-04-04 16:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20140328093038.GD7045@mwanda>
2014-03-28 17:14 ` [PATCH 2/2] rtl8180: don't use weird trick to access "far" registers Andrea Merello
2014-04-02 19:24 ` Dave Kilroy
2014-04-04 16:24 ` [PATCH 3/4] rtl8180: add parentheses to REG_ADDR macros Andrea Merello
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).