| Message ID | 20221010201453.77401-3-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | pinctrl: Clean up and add missed headers | expand |
On Mon, Oct 10, 2022 at 11:14:18PM +0300, Andy Shevchenko wrote: > Do not imply that some of the generic headers may be always included. > Instead, include explicitly what we are direct user of. > > While at it, sort headers alphabetically. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/gpio/gpiolib-cdev.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c > index f8041d4898d1..60a60e2d60c5 100644 > --- a/drivers/gpio/gpiolib-cdev.c > +++ b/drivers/gpio/gpiolib-cdev.c > @@ -10,8 +10,9 @@ > #include <linux/device.h> > #include <linux/err.h> > #include <linux/file.h> > -#include <linux/gpio.h> > #include <linux/gpio/driver.h> > +#include <linux/gpio.h> > +#include <linux/hte.h> Ok with the hte re-order. But moving the gpio subsystem header after the gpio/driver is not alphabetical ('.' precedes '/') and it read better and made more sense to me the way it was. > #include <linux/interrupt.h> > #include <linux/irqreturn.h> > #include <linux/kernel.h> > @@ -20,11 +21,12 @@ > #include <linux/mutex.h> > #include <linux/pinctrl/consumer.h> > #include <linux/poll.h> > +#include <linux/seq_file.h> I wasn't aware that we use anything from seq_file. What am I missing? Cheers, Kent. > #include <linux/spinlock.h> > #include <linux/timekeeping.h> > #include <linux/uaccess.h> > #include <linux/workqueue.h> > -#include <linux/hte.h> > + > #include <uapi/linux/gpio.h> > > #include "gpiolib.h" > -- > 2.35.1 >
On Tue, Oct 11, 2022 at 3:02 AM Kent Gibson <warthog618@gmail.com> wrote: > On Mon, Oct 10, 2022 at 11:14:18PM +0300, Andy Shevchenko wrote: ... > > -#include <linux/gpio.h> > > #include <linux/gpio/driver.h> > > +#include <linux/gpio.h> > > +#include <linux/hte.h> > > Ok with the hte re-order. > > But moving the gpio subsystem header after the gpio/driver is not > alphabetical ('.' precedes '/') and it read better and made more sense > to me the way it was. I see, I guess this is vim sort vs shell sort. Strange, they should follow the locale settings... ... > > +#include <linux/seq_file.h> > > I wasn't aware that we use anything from seq_file. > What am I missing? I will recheck, because in v6.0 I don't see anything, but LKP was not okay with something IIRC. -- With Best Regards, Andy Shevchenko
On Tue, Oct 11, 2022 at 11:05:42AM +0300, Andy Shevchenko wrote: > On Tue, Oct 11, 2022 at 3:02 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Mon, Oct 10, 2022 at 11:14:18PM +0300, Andy Shevchenko wrote: ... > > > -#include <linux/gpio.h> > > > #include <linux/gpio/driver.h> > > > +#include <linux/gpio.h> > > > +#include <linux/hte.h> > > > > Ok with the hte re-order. > > > > But moving the gpio subsystem header after the gpio/driver is not > > alphabetical ('.' precedes '/') and it read better and made more sense > > to me the way it was. > > I see, I guess this is vim sort vs shell sort. Strange, they should > follow the locale settings... I have checked, the shell and vim sort gave the same result as in this patch.
On Tue, Oct 11, 2022 at 04:48:17PM +0300, Andy Shevchenko wrote: > On Tue, Oct 11, 2022 at 11:05:42AM +0300, Andy Shevchenko wrote: > > On Tue, Oct 11, 2022 at 3:02 AM Kent Gibson <warthog618@gmail.com> wrote: > > > On Mon, Oct 10, 2022 at 11:14:18PM +0300, Andy Shevchenko wrote: > > ... > > > > > -#include <linux/gpio.h> > > > > #include <linux/gpio/driver.h> > > > > +#include <linux/gpio.h> > > > > +#include <linux/hte.h> > > > > > > Ok with the hte re-order. > > > > > > But moving the gpio subsystem header after the gpio/driver is not > > > alphabetical ('.' precedes '/') and it read better and made more sense > > > to me the way it was. > > > > I see, I guess this is vim sort vs shell sort. Strange, they should > > follow the locale settings... > > I have checked, the shell and vim sort gave the same result as in this patch. > The original order (sans hte.h) was done by VSCode Sort Lines Ascending, and that still returns the same result. That matches what I would expect to see given the content of the text. And for me vim also gives the original order. Just to confirm - is '.' 0x2e and '/' 0x2f in your universe? Cheers, Kent.
On Tue, Oct 11, 2022 at 10:13:02PM +0800, Kent Gibson wrote: > On Tue, Oct 11, 2022 at 04:48:17PM +0300, Andy Shevchenko wrote: > > On Tue, Oct 11, 2022 at 11:05:42AM +0300, Andy Shevchenko wrote: > > > On Tue, Oct 11, 2022 at 3:02 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Mon, Oct 10, 2022 at 11:14:18PM +0300, Andy Shevchenko wrote: ... > > > > > -#include <linux/gpio.h> > > > > > #include <linux/gpio/driver.h> > > > > > +#include <linux/gpio.h> > > > > > +#include <linux/hte.h> > > > > > > > > Ok with the hte re-order. > > > > > > > > But moving the gpio subsystem header after the gpio/driver is not > > > > alphabetical ('.' precedes '/') and it read better and made more sense > > > > to me the way it was. > > > > > > I see, I guess this is vim sort vs shell sort. Strange, they should > > > follow the locale settings... > > > > I have checked, the shell and vim sort gave the same result as in this patch. > > > > The original order (sans hte.h) was done by VSCode Sort Lines Ascending, > and that still returns the same result. That matches what I would > expect to see given the content of the text. > > And for me vim also gives the original order. > > Just to confirm - is '.' 0x2e and '/' 0x2f in your universe? $ LC_COLLATE=C sort test1.txt #include <linux/gpio.h> #include <linux/gpio/driver.h> $ LC_COLLATE= sort test1.txt #include <linux/gpio/driver.h> #include <linux/gpio.h> I guess this explains the difference. Currently I have en_US.UTF-8.
Hi Andy, On Tue, Oct 11, 2022 at 4:31 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Tue, Oct 11, 2022 at 10:13:02PM +0800, Kent Gibson wrote: > > On Tue, Oct 11, 2022 at 04:48:17PM +0300, Andy Shevchenko wrote: > > > On Tue, Oct 11, 2022 at 11:05:42AM +0300, Andy Shevchenko wrote: > > > > On Tue, Oct 11, 2022 at 3:02 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > On Mon, Oct 10, 2022 at 11:14:18PM +0300, Andy Shevchenko wrote: > > ... > > > > > > > -#include <linux/gpio.h> > > > > > > #include <linux/gpio/driver.h> > > > > > > +#include <linux/gpio.h> > > > > > > +#include <linux/hte.h> > > > > > > > > > > Ok with the hte re-order. > > > > > > > > > > But moving the gpio subsystem header after the gpio/driver is not > > > > > alphabetical ('.' precedes '/') and it read better and made more sense > > > > > to me the way it was. > > > > > > > > I see, I guess this is vim sort vs shell sort. Strange, they should > > > > follow the locale settings... > > > > > > I have checked, the shell and vim sort gave the same result as in this patch. > > > > > > > The original order (sans hte.h) was done by VSCode Sort Lines Ascending, > > and that still returns the same result. That matches what I would > > expect to see given the content of the text. > > > > And for me vim also gives the original order. > > > > Just to confirm - is '.' 0x2e and '/' 0x2f in your universe? > > $ LC_COLLATE=C sort test1.txt > #include <linux/gpio.h> > #include <linux/gpio/driver.h> > > $ LC_COLLATE= sort test1.txt > #include <linux/gpio/driver.h> > #include <linux/gpio.h> > > I guess this explains the difference. Currently I have en_US.UTF-8. Throwing my can of paint into the mix... I think it is more logical to first include the general <linux/gpio.h>, followed by whatever <linux/gpio-foo.h> and <linux/gpio/bar.h>, irrespective of (language-specific or phonebook) sort order. Yeah, it sucks that this requires some manual work after running sort... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Tue, Oct 11, 2022 at 04:39:46PM +0200, Geert Uytterhoeven wrote: > On Tue, Oct 11, 2022 at 4:31 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Oct 11, 2022 at 10:13:02PM +0800, Kent Gibson wrote: > > > On Tue, Oct 11, 2022 at 04:48:17PM +0300, Andy Shevchenko wrote: > > > > On Tue, Oct 11, 2022 at 11:05:42AM +0300, Andy Shevchenko wrote: > > > > > On Tue, Oct 11, 2022 at 3:02 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > On Mon, Oct 10, 2022 at 11:14:18PM +0300, Andy Shevchenko wrote: ... > > > > > > > -#include <linux/gpio.h> > > > > > > > #include <linux/gpio/driver.h> > > > > > > > +#include <linux/gpio.h> > > > > > > > +#include <linux/hte.h> > > > > > > > > > > > > Ok with the hte re-order. > > > > > > > > > > > > But moving the gpio subsystem header after the gpio/driver is not > > > > > > alphabetical ('.' precedes '/') and it read better and made more sense > > > > > > to me the way it was. > > > > > > > > > > I see, I guess this is vim sort vs shell sort. Strange, they should > > > > > follow the locale settings... > > > > > > > > I have checked, the shell and vim sort gave the same result as in this patch. > > > > > > > > > > The original order (sans hte.h) was done by VSCode Sort Lines Ascending, > > > and that still returns the same result. That matches what I would > > > expect to see given the content of the text. > > > > > > And for me vim also gives the original order. > > > > > > Just to confirm - is '.' 0x2e and '/' 0x2f in your universe? > > > > $ LC_COLLATE=C sort test1.txt > > #include <linux/gpio.h> > > #include <linux/gpio/driver.h> > > > > $ LC_COLLATE= sort test1.txt > > #include <linux/gpio/driver.h> > > #include <linux/gpio.h> > > > > I guess this explains the difference. Currently I have en_US.UTF-8. > > Throwing my can of paint into the mix... > > I think it is more logical to first include the general <linux/gpio.h>, > followed by whatever <linux/gpio-foo.h> and <linux/gpio/bar.h>, > irrespective of (language-specific or phonebook) sort order. > > Yeah, it sucks that this requires some manual work after running sort... It seems that kind of issue is in this patch only.
On Tue, Oct 11, 2022 at 06:19:13PM +0300, Andy Shevchenko wrote:
> On Tue, Oct 11, 2022 at 04:39:46PM +0200, Geert Uytterhoeven wrote:
...
After all this patch is not needed. However, during checking of the necessity
of this patch I realized that seq_file is used in a few GPIO drivers without
any actual users, so I will prepare clean up series for that as well.
On Tue, Oct 11, 2022 at 08:01:27AM +0800, Kent Gibson wrote: > On Mon, Oct 10, 2022 at 11:14:18PM +0300, Andy Shevchenko wrote: ... > > -#include <linux/gpio.h> > > #include <linux/gpio/driver.h> > > +#include <linux/gpio.h> > But moving the gpio subsystem header after the gpio/driver is not > alphabetical ('.' precedes '/') and it read better and made more sense > to me the way it was. Okay, I will move it back. ... > > +#include <linux/seq_file.h> > > I wasn't aware that we use anything from seq_file. > What am I missing? Eventually I can answer to your question: the commit 0ae3109a8391 ("gpiolib: cdev: add fdinfo output for line request file descriptors") is what you are missing. That said, we need this patch.
On Wed, Oct 12, 2022 at 04:30:05PM +0300, Andy Shevchenko wrote: > On Tue, Oct 11, 2022 at 08:01:27AM +0800, Kent Gibson wrote: > > On Mon, Oct 10, 2022 at 11:14:18PM +0300, Andy Shevchenko wrote: > > ... > > > > -#include <linux/gpio.h> > > > #include <linux/gpio/driver.h> > > > +#include <linux/gpio.h> > > > But moving the gpio subsystem header after the gpio/driver is not > > alphabetical ('.' precedes '/') and it read better and made more sense > > to me the way it was. > > Okay, I will move it back. > > ... > > > > +#include <linux/seq_file.h> > > > > I wasn't aware that we use anything from seq_file. > > What am I missing? > > > Eventually I can answer to your question: the commit 0ae3109a8391 > ("gpiolib: cdev: add fdinfo output for line request file descriptors") > is what you are missing. > > That said, we need this patch. > Ah, yes - totally forgot that one is in flight. That makes sense then. With the gpio headers retaining their original order: Rewiewed-by: Kent Gibson <warthog618@gmail.com> > -- > With Best Regards, > Andy Shevchenko > >
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index f8041d4898d1..60a60e2d60c5 100644 --- a/drivers/gpio/gpiolib-cdev.c +++ b/drivers/gpio/gpiolib-cdev.c @@ -10,8 +10,9 @@ #include <linux/device.h> #include <linux/err.h> #include <linux/file.h> -#include <linux/gpio.h> #include <linux/gpio/driver.h> +#include <linux/gpio.h> +#include <linux/hte.h> #include <linux/interrupt.h> #include <linux/irqreturn.h> #include <linux/kernel.h> @@ -20,11 +21,12 @@ #include <linux/mutex.h> #include <linux/pinctrl/consumer.h> #include <linux/poll.h> +#include <linux/seq_file.h> #include <linux/spinlock.h> #include <linux/timekeeping.h> #include <linux/uaccess.h> #include <linux/workqueue.h> -#include <linux/hte.h> + #include <uapi/linux/gpio.h> #include "gpiolib.h"
Do not imply that some of the generic headers may be always included. Instead, include explicitly what we are direct user of. While at it, sort headers alphabetically. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/gpio/gpiolib-cdev.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)