| Message ID | 20201126133152.3211309-18-lee.jones@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [01/17] wil6210: wmi: Correct misnamed function parameter 'ptr_' | expand |
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. > #define RTK_DEFAULT_TX_DESC_NUM 128 > #define RTK_BEQ_TX_DESC_NUM 256 > > @@ -212,6 +214,12 @@ struct rtw_pci { > void __iomem *mmap; > }; > > +const struct dev_pm_ops rtw_pm_ops; > + > +int rtw_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id); > +void rtw_pci_remove(struct pci_dev *pdev); > +void rtw_pci_shutdown(struct pci_dev *pdev); > + > static inline u32 max_num_of_tx_queue(u8 queue) > { > u32 max_num; > diff --git a/drivers/net/wireless/realtek/rtw88/rtw8723de.c > b/drivers/net/wireless/realtek/rtw88/rtw8723de.c > index c81eb4c336425..2dd689441e8dc 100644 > --- a/drivers/net/wireless/realtek/rtw88/rtw8723de.c > +++ b/drivers/net/wireless/realtek/rtw88/rtw8723de.c > @@ -4,6 +4,7 @@ > > #include <linux/module.h> > #include <linux/pci.h> I mean here: #include "main.h" > +#include "pci.h" > #include "rtw8723de.h" > > static const struct pci_device_id rtw_8723de_id_table[] = { > [snip] --- Ping-Ke
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.
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" --- Ping-Ke
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?
> -----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
On Mon, 30 Nov 2020, Pkshih wrote: > > > > -----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. I don't think any subsystem explicitly lists this as a rule. At least not to my knowledge. However you can see the many patches enforcing the rule by doing something like: $ git log -p --all-match --grep=include --grep=alphabetical
Pkshih <pkshih@realtek.com> writes: >> -----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. Yeah, please fix rtw88. Just like Lee said, pci.h should have "#include main.h" and not require other files to include files in correct order. > 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. I have not seen any documentation abou this, but it's a general rule to keep the header files maintainable.
Lee Jones <lee.jones@linaro.org> 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> Patch applied to wireless-drivers-next.git, thanks. 2e86ef413ab3 rtw88: pci: Add prototypes for .probe, .remove and .shutdown
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" + #define RTK_DEFAULT_TX_DESC_NUM 128 #define RTK_BEQ_TX_DESC_NUM 256 @@ -212,6 +214,12 @@ struct rtw_pci { void __iomem *mmap; }; +const struct dev_pm_ops rtw_pm_ops; + +int rtw_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id); +void rtw_pci_remove(struct pci_dev *pdev); +void rtw_pci_shutdown(struct pci_dev *pdev); + static inline u32 max_num_of_tx_queue(u8 queue) { u32 max_num; diff --git a/drivers/net/wireless/realtek/rtw88/rtw8723de.c b/drivers/net/wireless/realtek/rtw88/rtw8723de.c index c81eb4c336425..2dd689441e8dc 100644 --- a/drivers/net/wireless/realtek/rtw88/rtw8723de.c +++ b/drivers/net/wireless/realtek/rtw88/rtw8723de.c @@ -4,6 +4,7 @@ #include <linux/module.h> #include <linux/pci.h> +#include "pci.h" #include "rtw8723de.h" static const struct pci_device_id rtw_8723de_id_table[] = { diff --git a/drivers/net/wireless/realtek/rtw88/rtw8723de.h b/drivers/net/wireless/realtek/rtw88/rtw8723de.h index ba3842360c20a..2b4894846a07f 100644 --- a/drivers/net/wireless/realtek/rtw88/rtw8723de.h +++ b/drivers/net/wireless/realtek/rtw88/rtw8723de.h @@ -5,10 +5,6 @@ #ifndef __RTW_8723DE_H_ #define __RTW_8723DE_H_ -extern const struct dev_pm_ops rtw_pm_ops; extern struct rtw_chip_info rtw8723d_hw_spec; -int rtw_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id); -void rtw_pci_remove(struct pci_dev *pdev); -void rtw_pci_shutdown(struct pci_dev *pdev); #endif diff --git a/drivers/net/wireless/realtek/rtw88/rtw8821ce.c b/drivers/net/wireless/realtek/rtw88/rtw8821ce.c index 616fdcfd62c98..f34de115e4bc4 100644 --- a/drivers/net/wireless/realtek/rtw88/rtw8821ce.c +++ b/drivers/net/wireless/realtek/rtw88/rtw8821ce.c @@ -4,6 +4,7 @@ #include <linux/module.h> #include <linux/pci.h> +#include "pci.h" #include "rtw8821ce.h" static const struct pci_device_id rtw_8821ce_id_table[] = { diff --git a/drivers/net/wireless/realtek/rtw88/rtw8821ce.h b/drivers/net/wireless/realtek/rtw88/rtw8821ce.h index 8d3eb77a876be..54142acca5344 100644 --- a/drivers/net/wireless/realtek/rtw88/rtw8821ce.h +++ b/drivers/net/wireless/realtek/rtw88/rtw8821ce.h @@ -5,10 +5,6 @@ #ifndef __RTW_8821CE_H_ #define __RTW_8821CE_H_ -extern const struct dev_pm_ops rtw_pm_ops; extern struct rtw_chip_info rtw8821c_hw_spec; -int rtw_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id); -void rtw_pci_remove(struct pci_dev *pdev); -void rtw_pci_shutdown(struct pci_dev *pdev); #endif diff --git a/drivers/net/wireless/realtek/rtw88/rtw8822be.c b/drivers/net/wireless/realtek/rtw88/rtw8822be.c index 921916ae15cab..62ee7e62cac02 100644 --- a/drivers/net/wireless/realtek/rtw88/rtw8822be.c +++ b/drivers/net/wireless/realtek/rtw88/rtw8822be.c @@ -4,6 +4,7 @@ #include <linux/module.h> #include <linux/pci.h> +#include "pci.h" #include "rtw8822be.h" static const struct pci_device_id rtw_8822be_id_table[] = { diff --git a/drivers/net/wireless/realtek/rtw88/rtw8822be.h b/drivers/net/wireless/realtek/rtw88/rtw8822be.h index d823ca059f5c9..6668460d664da 100644 --- a/drivers/net/wireless/realtek/rtw88/rtw8822be.h +++ b/drivers/net/wireless/realtek/rtw88/rtw8822be.h @@ -5,10 +5,6 @@ #ifndef __RTW_8822BE_H_ #define __RTW_8822BE_H_ -extern const struct dev_pm_ops rtw_pm_ops; extern struct rtw_chip_info rtw8822b_hw_spec; -int rtw_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id); -void rtw_pci_remove(struct pci_dev *pdev); -void rtw_pci_shutdown(struct pci_dev *pdev); #endif diff --git a/drivers/net/wireless/realtek/rtw88/rtw8822ce.c b/drivers/net/wireless/realtek/rtw88/rtw8822ce.c index 026ac49ce6e3c..3845b1333dc39 100644 --- a/drivers/net/wireless/realtek/rtw88/rtw8822ce.c +++ b/drivers/net/wireless/realtek/rtw88/rtw8822ce.c @@ -4,6 +4,7 @@ #include <linux/module.h> #include <linux/pci.h> +#include "pci.h" #include "rtw8822ce.h" static const struct pci_device_id rtw_8822ce_id_table[] = { diff --git a/drivers/net/wireless/realtek/rtw88/rtw8822ce.h b/drivers/net/wireless/realtek/rtw88/rtw8822ce.h index c2c0e8675d742..fee32d7a45047 100644 --- a/drivers/net/wireless/realtek/rtw88/rtw8822ce.h +++ b/drivers/net/wireless/realtek/rtw88/rtw8822ce.h @@ -5,10 +5,6 @@ #ifndef __RTW_8822CE_H_ #define __RTW_8822CE_H_ -extern const struct dev_pm_ops rtw_pm_ops; extern struct rtw_chip_info rtw8822c_hw_spec; -int rtw_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id); -void rtw_pci_remove(struct pci_dev *pdev); -void rtw_pci_shutdown(struct pci_dev *pdev); #endif
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(-)