| Message ID | 20241101083910.3362945-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v1,1/1] nfc: mrvl: Don't use "proxy" headers | expand |
On 01/11/2024 09:39, Andy Shevchenko wrote: > Update header inclusions to follow IWYU (Include What You Use) > principle. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/nfc/nfcmrvl/uart.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/nfc/nfcmrvl/uart.c b/drivers/nfc/nfcmrvl/uart.c > index 956ae92f7573..2037cd6d4f4f 100644 > --- a/drivers/nfc/nfcmrvl/uart.c > +++ b/drivers/nfc/nfcmrvl/uart.c > @@ -5,11 +5,16 @@ > * Copyright (C) 2015, Marvell International Ltd. > */ > > -#include <linux/module.h> > #include <linux/delay.h> > -#include <linux/of_gpio.h> > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/printk.h> Do we really include printk? It's almost everywhere and pulled by kernel.h. I assume you checked rest of the nfcmrvl files for similar cleanups, so anyway: Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On Fri, 1 Nov 2024 10:39:10 +0200 Andy Shevchenko wrote: > Subject: [PATCH net-next v1 1/1] nfc: mrvl: Don't use "proxy" headers What is a "proxy" header? Guessing by the two patches you posted - are you trying to get rid of of_gpio.h? > Update header inclusions to follow IWYU (Include What You Use) > principle. I'm definitely on board with cleaning this up, but would prefer to make sure we can validate new patches against introducing regressions. Otherwise the stream of patches will be never ending. What tooling do you use? Is it easy to integrate into a CI system?
On Sat, Nov 02, 2024 at 09:00:04AM +0100, Krzysztof Kozlowski wrote: > On 01/11/2024 09:39, Andy Shevchenko wrote: > > Update header inclusions to follow IWYU (Include What You Use) > > principle. ... > > -#include <linux/module.h> > > #include <linux/delay.h> > > -#include <linux/of_gpio.h> > > +#include <linux/device.h> > > +#include <linux/err.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/printk.h> > > Do we really include printk? Yes, we use it here. > It's almost everywhere and pulled by kernel.h. kernel.h shouldn't be considered at all, it's a mess which is on undergoing cleaning up process. > I assume you checked rest of the nfcmrvl files for similar cleanups, so > anyway: > > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Thank you!
On Sun, Nov 03, 2024 at 08:17:40AM -0800, Jakub Kicinski wrote: > On Fri, 1 Nov 2024 10:39:10 +0200 Andy Shevchenko wrote: > > Subject: [PATCH net-next v1 1/1] nfc: mrvl: Don't use "proxy" headers > > What is a "proxy" header? The "proxy" header is any header that the code rely on to include other(s) *not related* to that header headers. Hopefully, despite too many words "header" you got the idea. > Guessing by the two patches you posted - are you trying to get rid of > of_gpio.h? In this particular case: 1) I want to get rid of deprecated of_gpio.h; 2) but at the same time it *is* a "proxy" header in this case. > > Update header inclusions to follow IWYU (Include What You Use) > > principle. > > I'm definitely on board with cleaning this up, but would prefer > to make sure we can validate new patches against introducing > regressions. 0-day LKP quite likely will notice any issues with this (it's quite good at it), and so far it reported A-OK. > Otherwise the stream of patches will be never ending. I know, and this is pity. And there was an attempt to make clang based tool for that, but no move forward as far as I can see. So, you are welcome to help developing such a tool. > What tooling do you use? My brains and my expertise in Linux Kernel project for the dependency hell we have in the headers. > Is it easy to integrate into a CI system? I don't think so. Can we have this being applied meanwhile, please? It's a showstopper for getting rid of of_gpio.h rather sooner than later.
On 04/11/2024 09:07, Andy Shevchenko wrote: > On Sat, Nov 02, 2024 at 09:00:04AM +0100, Krzysztof Kozlowski wrote: >> On 01/11/2024 09:39, Andy Shevchenko wrote: >>> Update header inclusions to follow IWYU (Include What You Use) >>> principle. > > ... > >>> -#include <linux/module.h> >>> #include <linux/delay.h> >>> -#include <linux/of_gpio.h> >>> +#include <linux/device.h> >>> +#include <linux/err.h> >>> +#include <linux/module.h> >>> +#include <linux/of.h> >>> +#include <linux/printk.h> >> >> Do we really include printk? > > Yes, we use it here. Yes, I know, I meant if this is actual practice and coding style, regardless of IWYU. Best regards, Krzysztof
On Mon, 4 Nov 2024 10:15:02 +0200 Andy Shevchenko wrote: > So, you are welcome to help developing such a tool. FTR I find saying such things to maintains very, very rude. > Can we have this being applied meanwhile, please? It's a showstopper for > getting rid of of_gpio.h rather sooner than later. Doing this cleanup as part of deleting a deprecated header seems legit. But you need to say that in the commit message.
On Mon, Nov 04, 2024 at 06:29:41PM -0800, Jakub Kicinski wrote: > On Mon, 4 Nov 2024 10:15:02 +0200 Andy Shevchenko wrote: > > So, you are welcome to help developing such a tool. > > FTR I find saying such things to maintains very, very rude. Sorry, it wasn't meant to be that. Let me elaborate. The tool is really what we need, but seems nobody is ready to invest time into its appearance. It would great and appreciated to have a consolidated work towards that. > > Can we have this being applied meanwhile, please? It's a showstopper for > > getting rid of of_gpio.h rather sooner than later. > > Doing this cleanup as part of deleting a deprecated header seems legit. > But you need to say that in the commit message. Thanks for review, I will address then in next version.
diff --git a/drivers/nfc/nfcmrvl/uart.c b/drivers/nfc/nfcmrvl/uart.c index 956ae92f7573..2037cd6d4f4f 100644 --- a/drivers/nfc/nfcmrvl/uart.c +++ b/drivers/nfc/nfcmrvl/uart.c @@ -5,11 +5,16 @@ * Copyright (C) 2015, Marvell International Ltd. */ -#include <linux/module.h> #include <linux/delay.h> -#include <linux/of_gpio.h> +#include <linux/device.h> +#include <linux/err.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/printk.h> + #include <net/nfc/nci.h> #include <net/nfc/nci_core.h> + #include "nfcmrvl.h" static unsigned int hci_muxed;
Update header inclusions to follow IWYU (Include What You Use) principle. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/nfc/nfcmrvl/uart.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)