On 23-2-2017 22:41, Rafał Miłecki wrote: > Hey Arend, > > On 8 February 2017 at 10:54, Arend Van Spriel > <arend.vanspriel@xxxxxxxxxxxx> wrote: >> On 2-2-2017 22:33, Rafał Miłecki wrote: >>> From: Rafał Miłecki <rafal@xxxxxxxxxx> >>> >>> This will allow getting struct device reference from the passed >>> brcmf_pub for the needs of dev_err. More detailed messages are really >>> important for home routers which frequently have 2 (or even 3) wireless >>> cards supported by brcmfmac. >>> >>> Note that all calls are yet to be updated as for now brcmf_err macro >>> always passes NULL. This will be handled in following patch to make this >>> change easier to review. >> >> I prefer brcmf_err() to have struct device reference directly instead of >> using brcmf_pub. That would remove the need for patches 5 till 7 as bus >> specific code already has struct device. So I have acked the first three >> patches, but would like to revise 8 and 9. > > I wanted to have some better perspective so I analyzed few upstream > Linux wireless drivers. It seems to be a pretty common pattern to have > one struct describing hardware that gets passed across the whole > driver. This way you can access any important info from any place of > the driver & some will probably say it also simplifies a driver. > > For example following drivers use following struct in almost every > part of their code (across multiple files): > ath9k: struct ath_hw > ath10k: struct ath10k > iwlmvm: struct iwl_mvm > mt7601u: struct mt7601u_dev > mwifiex: struct mwifiex_private > rt2x00: struct rt2x00_dev > > The most common / generic struct I found in brcmfmac was struct > brcmf_pub. That's why I tried to use it for the commonly used > brcmf_err. > > What's the reason for hiding this struct from few random parts of the > code? Is there any real gain from this? Hi Rafał, Time flies when you are ... well, me apparently. I realized I did not get back on this and looked at it myself. In brcmfmac we have layering with a common layer and bus specific layer. The interface between those layers is in bus.h, ie. struct brcmf_bus which holds reference to struct device. So I would think using struct brcmf_bus in brcmf_err() would be best fit. I also looked at doing some type introspection using __builtin_types_compatible_p() which is built-in function available in both GCC and Clang. Might be worth exploring. Regards, Arend
