RE: [PATCH 17/17] realtek: rtw88: pci: Add prototypes for .probe, .remove and .shutdown

[Pkshih <pkshih@realtek.com>]

> -----Original Message-----
> From: Lee Jones [mailto:lee.jones@linaro.org]
> Sent: Friday, November 27, 2020 4:57 PM
> To: Pkshih
> Cc: Tony Chuang; kvalo@codeaurora.org; linux-kernel@vger.kernel.org; linux-wireless@vger.kernel.org;
> davem@davemloft.net; netdev@vger.kernel.org; kuba@kernel.org
> Subject: Re: [PATCH 17/17] realtek: rtw88: pci: Add prototypes for .probe, .remove and .shutdown
> 
> On Fri, 27 Nov 2020, Pkshih wrote:
> 
> > On Fri, 2020-11-27 at 07:38 +0000, Lee Jones wrote:
> > > On Fri, 27 Nov 2020, Pkshih wrote:
> > >
> > > >
> > > > The subject prefix doesn't need 'realtek:'; use 'rtw88:'.
> > > >
> > > > On Thu, 2020-11-26 at 13:31 +0000, Lee Jones wrote:
> > > > > Also strip out other duplicates from driver specific headers.
> > > > >
> > > > > Ensure 'main.h' is explicitly included in 'pci.h' since the latter
> > > > > uses some defines from the former.  It avoids issues like:
> > > > >
> > > > >  from drivers/net/wireless/realtek/rtw88/rtw8822be.c:5:
> > > > >  drivers/net/wireless/realtek/rtw88/pci.h:209:28: error:
> > > > > ‘RTK_MAX_TX_QUEUE_NUM’ undeclared here (not in a function); did you mean
> > > > > ‘RTK_MAX_RX_DESC_NUM’?
> > > > >  209 | DECLARE_BITMAP(tx_queued, RTK_MAX_TX_QUEUE_NUM);
> > > > >  | ^~~~~~~~~~~~~~~~~~~~
> > > > >
> > > > > Fixes the following W=1 kernel build warning(s):
> > > > >
> > > > >  drivers/net/wireless/realtek/rtw88/pci.c:1488:5: warning: no previous
> > > > > prototype for ‘rtw_pci_probe’ [-Wmissing-prototypes]
> > > > >  1488 | int rtw_pci_probe(struct pci_dev *pdev,
> > > > >  | ^~~~~~~~~~~~~
> > > > >  drivers/net/wireless/realtek/rtw88/pci.c:1568:6: warning: no previous
> > > > > prototype for ‘rtw_pci_remove’ [-Wmissing-prototypes]
> > > > >  1568 | void rtw_pci_remove(struct pci_dev *pdev)
> > > > >  | ^~~~~~~~~~~~~~
> > > > >  drivers/net/wireless/realtek/rtw88/pci.c:1590:6: warning: no previous
> > > > > prototype for ‘rtw_pci_shutdown’ [-Wmissing-prototypes]
> > > > >  1590 | void rtw_pci_shutdown(struct pci_dev *pdev)
> > > > >  | ^~~~~~~~~~~~~~~~
> > > > >
> > > > > Cc: Yan-Hsuan Chuang <yhchuang@realtek.com>
> > > > > Cc: Kalle Valo <kvalo@codeaurora.org>
> > > > > Cc: "David S. Miller" <davem@davemloft.net>
> > > > > Cc: Jakub Kicinski <kuba@kernel.org>
> > > > > Cc: linux-wireless@vger.kernel.org
> > > > > Cc: netdev@vger.kernel.org
> > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > ---
> > > > >  drivers/net/wireless/realtek/rtw88/pci.h       | 8 ++++++++
> > > > >  drivers/net/wireless/realtek/rtw88/rtw8723de.c | 1 +
> > > > >  drivers/net/wireless/realtek/rtw88/rtw8723de.h | 4 ----
> > > > >  drivers/net/wireless/realtek/rtw88/rtw8821ce.c | 1 +
> > > > >  drivers/net/wireless/realtek/rtw88/rtw8821ce.h | 4 ----
> > > > >  drivers/net/wireless/realtek/rtw88/rtw8822be.c | 1 +
> > > > >  drivers/net/wireless/realtek/rtw88/rtw8822be.h | 4 ----
> > > > >  drivers/net/wireless/realtek/rtw88/rtw8822ce.c | 1 +
> > > > >  drivers/net/wireless/realtek/rtw88/rtw8822ce.h | 4 ----
> > > > >  9 files changed, 12 insertions(+), 16 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/wireless/realtek/rtw88/pci.h
> > > > > b/drivers/net/wireless/realtek/rtw88/pci.h
> > > > > index ca17aa9cf7dc7..cda56919a5f0f 100644
> > > > > --- a/drivers/net/wireless/realtek/rtw88/pci.h
> > > > > +++ b/drivers/net/wireless/realtek/rtw88/pci.h
> > > > > @@ -5,6 +5,8 @@
> > > > >  #ifndef __RTK_PCI_H_
> > > > >  #define __RTK_PCI_H_
> > > > >
> > > > > +#include "main.h"
> > > > > +
> > > >
> > > > Please #include "main.h" ahead of "pci.h" in each of rtw8xxxxe.c.
> > >
> > > You mean instead of in pci.h?
> > >
> > > Surely that's a hack.
> > >
> >
> > I mean don't include main.h in pci.h, but include both of them in each
> > of rtw8xxxxe.c.
> >
> > +#include "main.h"
> > +#include "pci.h"
> 
> Yes, that's what I thought you meant.  I think that's a hack.
> 
> Source files shouldn't rely on the ordering of include files to
> resolve dependencies.  In fact, a lot of subsystems require includes to
> be in alphabetical order.
> 
> If a source or header file references a resource from a specific
> header file (for instance here pci.h uses defines from main.h) then it
> should explicitly include it.
> 
> Can you tell me the technical reason as to why these drivers are
> handled differently please?
> 

No technical reason, but that's our coding convention that needs some
changes now.
Could you point out where kernel or subsystem describes the rules?
Or, point out the subsystem you mentioned above.
Then, I can study and follow the rules for further development.

Thank you
---
Ping-Ke