| Message ID | 20240408231727.396452-2-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | gpiolib: Fix gpio_lookup_flags mess and add Return sections | expand |
Hi Andy,
kernel test robot noticed the following build warnings:
[auto build test WARNING on brgl/gpio/for-next]
[also build test WARNING on wireless-next/main wireless/main linus/master v6.9-rc3 next-20240408]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/gpiolib-Fix-a-mess-with-the-GPIO_-flags/20240409-071911
base: https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
patch link: https://lore.kernel.org/r/20240408231727.396452-2-andriy.shevchenko%40linux.intel.com
patch subject: [PATCH v2 1/2] gpiolib: Fix a mess with the GPIO_* flags
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20240409/202404091205.rnu1DOaq-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240409/202404091205.rnu1DOaq-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202404091205.rnu1DOaq-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from arch/x86/include/asm/geode.h:12,
from arch/x86/platform/geode/alix.c:28:
>> include/linux/cs5535.h:149: warning: "GPIO_PULL_UP" redefined
149 | #define GPIO_PULL_UP 0x18
|
In file included from include/linux/gpio/machine.h:5,
from arch/x86/platform/geode/alix.c:25:
include/dt-bindings/gpio/gpio.h:37: note: this is the location of the previous definition
37 | #define GPIO_PULL_UP 16
|
>> include/linux/cs5535.h:150: warning: "GPIO_PULL_DOWN" redefined
150 | #define GPIO_PULL_DOWN 0x1C
|
include/dt-bindings/gpio/gpio.h:40: note: this is the location of the previous definition
40 | #define GPIO_PULL_DOWN 32
|
vim +/GPIO_PULL_UP +149 include/linux/cs5535.h
f060f27007b393 Andres Salomon 2009-12-14 141
5f0a96b044d8ed Andres Salomon 2009-12-14 142 /* GPIOs */
5f0a96b044d8ed Andres Salomon 2009-12-14 143 #define GPIO_OUTPUT_VAL 0x00
5f0a96b044d8ed Andres Salomon 2009-12-14 144 #define GPIO_OUTPUT_ENABLE 0x04
5f0a96b044d8ed Andres Salomon 2009-12-14 145 #define GPIO_OUTPUT_OPEN_DRAIN 0x08
5f0a96b044d8ed Andres Salomon 2009-12-14 146 #define GPIO_OUTPUT_INVERT 0x0C
5f0a96b044d8ed Andres Salomon 2009-12-14 147 #define GPIO_OUTPUT_AUX1 0x10
5f0a96b044d8ed Andres Salomon 2009-12-14 148 #define GPIO_OUTPUT_AUX2 0x14
5f0a96b044d8ed Andres Salomon 2009-12-14 @149 #define GPIO_PULL_UP 0x18
5f0a96b044d8ed Andres Salomon 2009-12-14 @150 #define GPIO_PULL_DOWN 0x1C
5f0a96b044d8ed Andres Salomon 2009-12-14 151 #define GPIO_INPUT_ENABLE 0x20
5f0a96b044d8ed Andres Salomon 2009-12-14 152 #define GPIO_INPUT_INVERT 0x24
5f0a96b044d8ed Andres Salomon 2009-12-14 153 #define GPIO_INPUT_FILTER 0x28
5f0a96b044d8ed Andres Salomon 2009-12-14 154 #define GPIO_INPUT_EVENT_COUNT 0x2C
5f0a96b044d8ed Andres Salomon 2009-12-14 155 #define GPIO_READ_BACK 0x30
5f0a96b044d8ed Andres Salomon 2009-12-14 156 #define GPIO_INPUT_AUX1 0x34
5f0a96b044d8ed Andres Salomon 2009-12-14 157 #define GPIO_EVENTS_ENABLE 0x38
5f0a96b044d8ed Andres Salomon 2009-12-14 158 #define GPIO_LOCK_ENABLE 0x3C
5f0a96b044d8ed Andres Salomon 2009-12-14 159 #define GPIO_POSITIVE_EDGE_EN 0x40
5f0a96b044d8ed Andres Salomon 2009-12-14 160 #define GPIO_NEGATIVE_EDGE_EN 0x44
5f0a96b044d8ed Andres Salomon 2009-12-14 161 #define GPIO_POSITIVE_EDGE_STS 0x48
5f0a96b044d8ed Andres Salomon 2009-12-14 162 #define GPIO_NEGATIVE_EDGE_STS 0x4C
5f0a96b044d8ed Andres Salomon 2009-12-14 163
Hi Andy, kernel test robot noticed the following build warnings: [auto build test WARNING on brgl/gpio/for-next] [also build test WARNING on wireless-next/main wireless/main linus/master v6.9-rc3 next-20240409] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/gpiolib-Fix-a-mess-with-the-GPIO_-flags/20240409-071911 base: https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next patch link: https://lore.kernel.org/r/20240408231727.396452-2-andriy.shevchenko%40linux.intel.com patch subject: [PATCH v2 1/2] gpiolib: Fix a mess with the GPIO_* flags config: i386-buildonly-randconfig-002-20240409 (https://download.01.org/0day-ci/archive/20240409/202404091557.rf8NTu9B-lkp@intel.com/config) compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240409/202404091557.rf8NTu9B-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202404091557.rf8NTu9B-lkp@intel.com/ All warnings (new ones prefixed by >>): In file included from arch/x86/platform/geode/net5501.c:25: In file included from arch/x86/include/asm/geode.h:12: >> include/linux/cs5535.h:149:9: warning: 'GPIO_PULL_UP' macro redefined [-Wmacro-redefined] 149 | #define GPIO_PULL_UP 0x18 | ^ include/dt-bindings/gpio/gpio.h:37:9: note: previous definition is here 37 | #define GPIO_PULL_UP 16 | ^ In file included from arch/x86/platform/geode/net5501.c:25: In file included from arch/x86/include/asm/geode.h:12: >> include/linux/cs5535.h:150:9: warning: 'GPIO_PULL_DOWN' macro redefined [-Wmacro-redefined] 150 | #define GPIO_PULL_DOWN 0x1C | ^ include/dt-bindings/gpio/gpio.h:40:9: note: previous definition is here 40 | #define GPIO_PULL_DOWN 32 | ^ 2 warnings generated. vim +/GPIO_PULL_UP +149 include/linux/cs5535.h f060f27007b393 Andres Salomon 2009-12-14 141 5f0a96b044d8ed Andres Salomon 2009-12-14 142 /* GPIOs */ 5f0a96b044d8ed Andres Salomon 2009-12-14 143 #define GPIO_OUTPUT_VAL 0x00 5f0a96b044d8ed Andres Salomon 2009-12-14 144 #define GPIO_OUTPUT_ENABLE 0x04 5f0a96b044d8ed Andres Salomon 2009-12-14 145 #define GPIO_OUTPUT_OPEN_DRAIN 0x08 5f0a96b044d8ed Andres Salomon 2009-12-14 146 #define GPIO_OUTPUT_INVERT 0x0C 5f0a96b044d8ed Andres Salomon 2009-12-14 147 #define GPIO_OUTPUT_AUX1 0x10 5f0a96b044d8ed Andres Salomon 2009-12-14 148 #define GPIO_OUTPUT_AUX2 0x14 5f0a96b044d8ed Andres Salomon 2009-12-14 @149 #define GPIO_PULL_UP 0x18 5f0a96b044d8ed Andres Salomon 2009-12-14 @150 #define GPIO_PULL_DOWN 0x1C 5f0a96b044d8ed Andres Salomon 2009-12-14 151 #define GPIO_INPUT_ENABLE 0x20 5f0a96b044d8ed Andres Salomon 2009-12-14 152 #define GPIO_INPUT_INVERT 0x24 5f0a96b044d8ed Andres Salomon 2009-12-14 153 #define GPIO_INPUT_FILTER 0x28 5f0a96b044d8ed Andres Salomon 2009-12-14 154 #define GPIO_INPUT_EVENT_COUNT 0x2C 5f0a96b044d8ed Andres Salomon 2009-12-14 155 #define GPIO_READ_BACK 0x30 5f0a96b044d8ed Andres Salomon 2009-12-14 156 #define GPIO_INPUT_AUX1 0x34 5f0a96b044d8ed Andres Salomon 2009-12-14 157 #define GPIO_EVENTS_ENABLE 0x38 5f0a96b044d8ed Andres Salomon 2009-12-14 158 #define GPIO_LOCK_ENABLE 0x3C 5f0a96b044d8ed Andres Salomon 2009-12-14 159 #define GPIO_POSITIVE_EDGE_EN 0x40 5f0a96b044d8ed Andres Salomon 2009-12-14 160 #define GPIO_NEGATIVE_EDGE_EN 0x44 5f0a96b044d8ed Andres Salomon 2009-12-14 161 #define GPIO_POSITIVE_EDGE_STS 0x48 5f0a96b044d8ed Andres Salomon 2009-12-14 162 #define GPIO_NEGATIVE_EDGE_STS 0x4C 5f0a96b044d8ed Andres Salomon 2009-12-14 163
On Tue, Apr 9, 2024 at 1:17 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > The GPIO_* flag definitions are *almost* duplicated in two files > (with unmatches OPEN_SOURCE / OPEN_DRAIN). Moreover, some code relies > on one set of definitions while the rest is on the other. Clean up > this mess by providing only one source of the definitions to all. > > Fixes: b424808115cb ("brcm80211: brcmsmac: Move LEDs to GPIO descriptors") > Fixes: 5923ea6c2ce6 ("gpio: pass lookup and descriptor flags to request_own") > Fixes: fed7026adc7c ("gpiolib: Make use of enum gpio_lookup_flags consistent") > Fixes: 4c0facddb7d8 ("gpio: core: Decouple open drain/source flag with active low/high") > Fixes: 69d301fdd196 ("gpio: add DT bindings for existing consumer flags") > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/gpio/gpiolib-of.c | 5 ++--- > drivers/gpio/gpiolib.c | 8 +++----- > .../broadcom/brcm80211/brcmsmac/led.c | 2 +- > include/linux/gpio/driver.h | 3 +-- > include/linux/gpio/machine.h | 20 +++++-------------- > 5 files changed, 12 insertions(+), 26 deletions(-) > I don't think ./dt-bindings/gpio/gpio.h is the right source of these defines for everyone - including non-OF systems. I would prefer the ones in include/linux/gpio/machine.h be the upstream source but then headers in include/dt-bindings/ cannot include them so my second-best suggestion is to rename the ones in include/linux/gpio/machine.h and treewide too. In general values from ./dt-bindings/gpio/gpio.h should only be used in DTS sources and gpiolib-of code. Bart
On Tue, Apr 09, 2024 at 11:42:37AM +0200, Bartosz Golaszewski wrote: > On Tue, Apr 9, 2024 at 1:17 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > The GPIO_* flag definitions are *almost* duplicated in two files > > (with unmatches OPEN_SOURCE / OPEN_DRAIN). Moreover, some code relies > > on one set of definitions while the rest is on the other. Clean up > > this mess by providing only one source of the definitions to all. > > > > Fixes: b424808115cb ("brcm80211: brcmsmac: Move LEDs to GPIO descriptors") > > Fixes: 5923ea6c2ce6 ("gpio: pass lookup and descriptor flags to request_own") > > Fixes: fed7026adc7c ("gpiolib: Make use of enum gpio_lookup_flags consistent") > > Fixes: 4c0facddb7d8 ("gpio: core: Decouple open drain/source flag with active low/high") > > Fixes: 69d301fdd196 ("gpio: add DT bindings for existing consumer flags") > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > drivers/gpio/gpiolib-of.c | 5 ++--- > > drivers/gpio/gpiolib.c | 8 +++----- > > .../broadcom/brcm80211/brcmsmac/led.c | 2 +- > > include/linux/gpio/driver.h | 3 +-- > > include/linux/gpio/machine.h | 20 +++++-------------- > > 5 files changed, 12 insertions(+), 26 deletions(-) > > I don't think ./dt-bindings/gpio/gpio.h is the right source of these > defines for everyone - including non-OF systems. I would prefer the > ones in include/linux/gpio/machine.h be the upstream source but then > headers in include/dt-bindings/ cannot include them so my second-best > suggestion is to rename the ones in include/linux/gpio/machine.h and > treewide too. In general values from ./dt-bindings/gpio/gpio.h should > only be used in DTS sources and gpiolib-of code. Then, please fix that your way. It's quite annoying issue.
On Tue, Apr 9, 2024 at 2:51 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Apr 09, 2024 at 11:42:37AM +0200, Bartosz Golaszewski wrote: > > On Tue, Apr 9, 2024 at 1:17 AM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > The GPIO_* flag definitions are *almost* duplicated in two files > > > (with unmatches OPEN_SOURCE / OPEN_DRAIN). Moreover, some code relies > > > on one set of definitions while the rest is on the other. Clean up > > > this mess by providing only one source of the definitions to all. > > > > > > Fixes: b424808115cb ("brcm80211: brcmsmac: Move LEDs to GPIO descriptors") > > > Fixes: 5923ea6c2ce6 ("gpio: pass lookup and descriptor flags to request_own") > > > Fixes: fed7026adc7c ("gpiolib: Make use of enum gpio_lookup_flags consistent") > > > Fixes: 4c0facddb7d8 ("gpio: core: Decouple open drain/source flag with active low/high") > > > Fixes: 69d301fdd196 ("gpio: add DT bindings for existing consumer flags") > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > --- > > > drivers/gpio/gpiolib-of.c | 5 ++--- > > > drivers/gpio/gpiolib.c | 8 +++----- > > > .../broadcom/brcm80211/brcmsmac/led.c | 2 +- > > > include/linux/gpio/driver.h | 3 +-- > > > include/linux/gpio/machine.h | 20 +++++-------------- > > > 5 files changed, 12 insertions(+), 26 deletions(-) > > > > I don't think ./dt-bindings/gpio/gpio.h is the right source of these > > defines for everyone - including non-OF systems. I would prefer the > > ones in include/linux/gpio/machine.h be the upstream source but then > > headers in include/dt-bindings/ cannot include them so my second-best > > suggestion is to rename the ones in include/linux/gpio/machine.h and > > treewide too. In general values from ./dt-bindings/gpio/gpio.h should > > only be used in DTS sources and gpiolib-of code. > > Then, please fix that your way. It's quite annoying issue. > This is not difficult in itself but it's a tree-wide change so we will probably have to send it to Torvalds at the end of the merge window in a separate pull-request. I don't really have time now, I'll be travelling for 5 weeks in a row. I'll see closer to the merge window. Or next release cycle. Bart
On Tue, Apr 09, 2024 at 02:55:20PM +0200, Bartosz Golaszewski wrote: > On Tue, Apr 9, 2024 at 2:51 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Apr 09, 2024 at 11:42:37AM +0200, Bartosz Golaszewski wrote: > > > On Tue, Apr 9, 2024 at 1:17 AM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > The GPIO_* flag definitions are *almost* duplicated in two files > > > > (with unmatches OPEN_SOURCE / OPEN_DRAIN). Moreover, some code relies > > > > on one set of definitions while the rest is on the other. Clean up > > > > this mess by providing only one source of the definitions to all. > > > > > > > > Fixes: b424808115cb ("brcm80211: brcmsmac: Move LEDs to GPIO descriptors") > > > > Fixes: 5923ea6c2ce6 ("gpio: pass lookup and descriptor flags to request_own") > > > > Fixes: fed7026adc7c ("gpiolib: Make use of enum gpio_lookup_flags consistent") > > > > Fixes: 4c0facddb7d8 ("gpio: core: Decouple open drain/source flag with active low/high") > > > > Fixes: 69d301fdd196 ("gpio: add DT bindings for existing consumer flags") > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > --- > > > > drivers/gpio/gpiolib-of.c | 5 ++--- > > > > drivers/gpio/gpiolib.c | 8 +++----- > > > > .../broadcom/brcm80211/brcmsmac/led.c | 2 +- > > > > include/linux/gpio/driver.h | 3 +-- > > > > include/linux/gpio/machine.h | 20 +++++-------------- > > > > 5 files changed, 12 insertions(+), 26 deletions(-) > > > > > > I don't think ./dt-bindings/gpio/gpio.h is the right source of these > > > defines for everyone - including non-OF systems. I would prefer the > > > ones in include/linux/gpio/machine.h be the upstream source but then > > > headers in include/dt-bindings/ cannot include them so my second-best > > > suggestion is to rename the ones in include/linux/gpio/machine.h and > > > treewide too. In general values from ./dt-bindings/gpio/gpio.h should > > > only be used in DTS sources and gpiolib-of code. > > > > Then, please fix that your way. It's quite annoying issue. > > This is not difficult in itself I'm not sure, what about enum gpio_lookup_flags? Shall we resurrect it? I see that you have better vision anyway. Consider my patch as a problem report (and as bonus you have already list of Fixes tags :-). > but it's a tree-wide change so we will > probably have to send it to Torvalds at the end of the merge window in > a separate pull-request. WFM! > I don't really have time now, I'll be travelling for 5 weeks in a row. > I'll see closer to the merge window. Or next release cycle. But can you prioritize this? It's a carefully planted minefield with already a bug and confusion here.
On Tue, Apr 9, 2024 at 1:17 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > The GPIO_* flag definitions are *almost* duplicated in two files > (with unmatches OPEN_SOURCE / OPEN_DRAIN). Moreover, some code relies > on one set of definitions while the rest is on the other. Clean up > this mess by providing only one source of the definitions to all. > > Fixes: b424808115cb ("brcm80211: brcmsmac: Move LEDs to GPIO descriptors") > Fixes: 5923ea6c2ce6 ("gpio: pass lookup and descriptor flags to request_own") > Fixes: fed7026adc7c ("gpiolib: Make use of enum gpio_lookup_flags consistent") > Fixes: 4c0facddb7d8 ("gpio: core: Decouple open drain/source flag with active low/high") > Fixes: 69d301fdd196 ("gpio: add DT bindings for existing consumer flags") > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> The way the line lookup flags ("lflags") were conceived was through support for non-DT systems using descriptor tables, and that is how enum gpio_lookup_flags came to be. When OF support was added it was bolted on on the side, in essence assuming that the DT/OF ABI was completely separate (and they/we sure like to think about it that way...) and thus needed translation from OF flags to kernel-internal enum gpio_lookup_flags. The way *I* thought about this when writing it was certainly that the DT bindings was a separate thing (<dt-bindings/*.h> didn't even exist at the time I think) and that translation from OF to kernel-internal lflags would happen in *one* place. The main reasoning still holds: the OF define is an ABI, so it can *never* be changed, but the enum gpio_lookup_flags is subject to Documentation/process/stable-api-nonsense.rst and that means that if we want to swap around the order of the definitions we can. But admittedly this is a bit over-belief in process and separation of concerns and practical matters may be something else... Yours, Linus Walleij
On Fri, Apr 12, 2024 at 10:20:24AM +0200, Linus Walleij wrote: > On Tue, Apr 9, 2024 at 1:17 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > The GPIO_* flag definitions are *almost* duplicated in two files > > (with unmatches OPEN_SOURCE / OPEN_DRAIN). Moreover, some code relies > > on one set of definitions while the rest is on the other. Clean up > > this mess by providing only one source of the definitions to all. > > > > Fixes: b424808115cb ("brcm80211: brcmsmac: Move LEDs to GPIO descriptors") > > Fixes: 5923ea6c2ce6 ("gpio: pass lookup and descriptor flags to request_own") > > Fixes: fed7026adc7c ("gpiolib: Make use of enum gpio_lookup_flags consistent") > > Fixes: 4c0facddb7d8 ("gpio: core: Decouple open drain/source flag with active low/high") > > Fixes: 69d301fdd196 ("gpio: add DT bindings for existing consumer flags") > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > The way the line lookup flags ("lflags") were conceived was through > support for non-DT systems using descriptor tables, and that is how > enum gpio_lookup_flags came to be. > > When OF support was added it was bolted on on the side, in essence > assuming that the DT/OF ABI was completely separate (and they/we > sure like to think about it that way...) and thus needed translation from > OF flags to kernel-internal enum gpio_lookup_flags. > > The way *I* thought about this when writing it was certainly that the > DT bindings was a separate thing (<dt-bindings/*.h> didn't even exist > at the time I think) and that translation from OF to kernel-internal > lflags would happen in *one* place. > > The main reasoning still holds: the OF define is an ABI, so it can > *never* be changed, but the enum gpio_lookup_flags is subject to > Documentation/process/stable-api-nonsense.rst and that means > that if we want to swap around the order of the definitions we can. > > But admittedly this is a bit over-belief in process and separation of > concerns and practical matters may be something else... Got it. But we have a name clash and the mess added to the users. I can redo this to separate these entities. Note, that there is code in the kernel that *does* use #include <dt-bindings/*.h> for Linux internals. $ git grep -lw '^#include <dt-bindings/.*\.h>' -- drivers/ | xargs dirname | cut -f 1,2 -d '/' | sort -u drivers/bus drivers/clk drivers/clocksource drivers/cpufreq drivers/dma drivers/firmware drivers/gpio drivers/gpu drivers/hwtracing drivers/i2c drivers/iio drivers/input drivers/interconnect drivers/iommu drivers/irqchip drivers/leds drivers/mailbox drivers/media drivers/memory drivers/mfd drivers/net drivers/phy drivers/pinctrl drivers/platform drivers/pmdomain drivers/power drivers/pwm drivers/regulator drivers/remoteproc drivers/reset drivers/rtc drivers/soc drivers/spmi drivers/thermal drivers/tty drivers/video drivers/watchdog P.S> One of the patch this tries to fix is yours IIRC :-)
On Fri, Apr 12, 2024 at 5:25 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Apr 12, 2024 at 10:20:24AM +0200, Linus Walleij wrote: > > On Tue, Apr 9, 2024 at 1:17 AM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > The GPIO_* flag definitions are *almost* duplicated in two files > > > (with unmatches OPEN_SOURCE / OPEN_DRAIN). Moreover, some code relies > > > on one set of definitions while the rest is on the other. Clean up > > > this mess by providing only one source of the definitions to all. > > > > > > Fixes: b424808115cb ("brcm80211: brcmsmac: Move LEDs to GPIO descriptors") > > > Fixes: 5923ea6c2ce6 ("gpio: pass lookup and descriptor flags to request_own") > > > Fixes: fed7026adc7c ("gpiolib: Make use of enum gpio_lookup_flags consistent") > > > Fixes: 4c0facddb7d8 ("gpio: core: Decouple open drain/source flag with active low/high") > > > Fixes: 69d301fdd196 ("gpio: add DT bindings for existing consumer flags") > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > The way the line lookup flags ("lflags") were conceived was through > > support for non-DT systems using descriptor tables, and that is how > > enum gpio_lookup_flags came to be. > > > > When OF support was added it was bolted on on the side, in essence > > assuming that the DT/OF ABI was completely separate (and they/we > > sure like to think about it that way...) and thus needed translation from > > OF flags to kernel-internal enum gpio_lookup_flags. > > > > The way *I* thought about this when writing it was certainly that the > > DT bindings was a separate thing (<dt-bindings/*.h> didn't even exist > > at the time I think) and that translation from OF to kernel-internal > > lflags would happen in *one* place. > > > > The main reasoning still holds: the OF define is an ABI, so it can > > *never* be changed, but the enum gpio_lookup_flags is subject to > > Documentation/process/stable-api-nonsense.rst and that means > > that if we want to swap around the order of the definitions we can. > > > > But admittedly this is a bit over-belief in process and separation of > > concerns and practical matters may be something else... > > Got it. But we have a name clash and the mess added to the users. > I can redo this to separate these entities. > > Note, that there is code in the kernel that *does* use > #include <dt-bindings/*.h> > for Linux internals. > Well, then they are wrong. We should convert them to using linux/gpio/machine.h first. Or even put these defines elsewhere depending on what these drivers are using it for in general. Maybe machine.h is not the right place. Then once that's figured out, we can start renaming the constants. IIUC include/dt-bindings/ headers should only be used by DT sources and code that parses the OF properties. But it seems to me that we need to inspect these users, we cannot just automatically convert them at once, this is asking for trouble IMO. Bart
On Fri, Apr 12, 2024 at 9:44 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > IIUC include/dt-bindings/ headers should only be used by DT sources > and code that parses the OF properties. That's what I have come to understand as well. I wonder if there is something that can be done to enforce it? Ideally the code that parses OF properties should have to opt in to get access to the <dt-bindings/*> namespace. Yours, Linus Walleij
On Tue, Apr 16, 2024 at 02:22:09PM +0200, Linus Walleij wrote: > On Fri, Apr 12, 2024 at 9:44 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > IIUC include/dt-bindings/ headers should only be used by DT sources > > and code that parses the OF properties. > > That's what I have come to understand as well. > > I wonder if there is something that can be done to enforce it? > > Ideally the code that parses OF properties should have to > opt in to get access to the <dt-bindings/*> namespace. Whatever you, guys, come up with as a solution, can it be fixed sooner than later? I mean, I would appreciate if somebody got it done for v6.9-rcX/v6.10-rc1 so we don't need to look into this again.
On Tue, Apr 16, 2024 at 4:05 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Apr 16, 2024 at 02:22:09PM +0200, Linus Walleij wrote: > > On Fri, Apr 12, 2024 at 9:44 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > > IIUC include/dt-bindings/ headers should only be used by DT sources > > > and code that parses the OF properties. > > > > That's what I have come to understand as well. > > > > I wonder if there is something that can be done to enforce it? > > > > Ideally the code that parses OF properties should have to > > opt in to get access to the <dt-bindings/*> namespace. > > Whatever you, guys, come up with as a solution, can it be fixed sooner than later? > I mean, I would appreciate if somebody got it done for v6.9-rcX/v6.10-rc1 so we don't > need to look into this again. > I'm not sure you got what I was saying. I don't think this can be fixed quickly. This is just another bunch of technical debt that will have to be addressed carefully on a case-by-case basis and run through autobuilders in all possible configurations. This type of include-related issues is always brittle and will lead to build failures if we don't consider our moves. Bart
On Tue, Apr 16, 2024 at 11:07:58PM +0200, Bartosz Golaszewski wrote: > On Tue, Apr 16, 2024 at 4:05 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Tue, Apr 16, 2024 at 02:22:09PM +0200, Linus Walleij wrote: > > > On Fri, Apr 12, 2024 at 9:44 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > > > > IIUC include/dt-bindings/ headers should only be used by DT sources > > > > and code that parses the OF properties. > > > > > > That's what I have come to understand as well. > > > > > > I wonder if there is something that can be done to enforce it? > > > > > > Ideally the code that parses OF properties should have to > > > opt in to get access to the <dt-bindings/*> namespace. > > > > Whatever you, guys, come up with as a solution, can it be fixed sooner than later? > > I mean, I would appreciate if somebody got it done for v6.9-rcX/v6.10-rc1 so we don't > > need to look into this again. > > I'm not sure you got what I was saying. I don't think this can be > fixed quickly. This is just another bunch of technical debt that will > have to be addressed carefully on a case-by-case basis and run through > autobuilders in all possible configurations. > > This type of include-related issues is always brittle and will lead to > build failures if we don't consider our moves. I proposed a quick fix which was rejected. I think this is still doable in a few steps: - align constant values in DT and enum - drop usage of DT in the kernel code (That's what you want IIUC, however I disagree with this from technical perspective as DT constants can be used in the code as long as they are mapped 1:1 to what code does. That's current state of affairs. OTOH semantically this may be an issue.) - restore enum usage treewide (?) Again, the problem now is only in open source / open drain configurations and there are only a few users of these flags _in kernel_. I do not see why it can not be done in one or two evenings time range. P.S> Most of the time I spent when prepared the proposed fix is digging the history and trying to understand how comes that we have desynchronisation of the values over the time. The output of that is the list of Fixes tags.
On Wed, Apr 17, 2024 at 10:45 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Apr 16, 2024 at 11:07:58PM +0200, Bartosz Golaszewski wrote: > > On Tue, Apr 16, 2024 at 4:05 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On Tue, Apr 16, 2024 at 02:22:09PM +0200, Linus Walleij wrote: > > > > On Fri, Apr 12, 2024 at 9:44 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > > > > > > IIUC include/dt-bindings/ headers should only be used by DT sources > > > > > and code that parses the OF properties. > > > > > > > > That's what I have come to understand as well. > > > > > > > > I wonder if there is something that can be done to enforce it? > > > > > > > > Ideally the code that parses OF properties should have to > > > > opt in to get access to the <dt-bindings/*> namespace. > > > > > > Whatever you, guys, come up with as a solution, can it be fixed sooner than later? > > > I mean, I would appreciate if somebody got it done for v6.9-rcX/v6.10-rc1 so we don't > > > need to look into this again. > > > > I'm not sure you got what I was saying. I don't think this can be > > fixed quickly. This is just another bunch of technical debt that will > > have to be addressed carefully on a case-by-case basis and run through > > autobuilders in all possible configurations. > > > > This type of include-related issues is always brittle and will lead to > > build failures if we don't consider our moves. > > I proposed a quick fix which was rejected. I think this is still doable in a > few steps: > You having proposed a quick fix is not a reason for me to either apply it immediately OR equally promptly provide a proper solution myself. > - align constant values in DT and enum > - drop usage of DT in the kernel code (That's what you want IIUC, however > I disagree with this from technical perspective as DT constants can be used > in the code as long as they are mapped 1:1 to what code does. That's current > state of affairs. OTOH semantically this may be an issue.) It's against the convention of only using dt-bindings headers as I described before. I disagree with your proposition and it seems to me Linus backed me up on this. > - restore enum usage treewide (?) > > Again, the problem now is only in open source / open drain configurations > and there are only a few users of these flags _in kernel_. I do not see > why it can not be done in one or two evenings time range. > So you know what needs doing. I'm at a conference now, I'll be off for a week in April and I also have another conference scheduled for May. If you believe this needs addressing urgently, then I suggest you do it right. Otherwise, I'll get to it when I have the time. Unfortunately my TODO list runneth over. :( But I have to say, I suspect it won't be as easy as you present it because we have so many build configs that may fail. > P.S> > Most of the time I spent when prepared the proposed fix is digging the history > and trying to understand how comes that we have desynchronisation of the values > over the time. The output of that is the list of Fixes tags. > Thank you, this history is useful and should make its way into git history when we fix this issue. Bart
On Wed, Apr 17, 2024 at 08:39:52PM +0200, Bartosz Golaszewski wrote: > On Wed, Apr 17, 2024 at 10:45 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Apr 16, 2024 at 11:07:58PM +0200, Bartosz Golaszewski wrote: ... > > Again, the problem now is only in open source / open drain configurations > > and there are only a few users of these flags _in kernel_. I do not see > > why it can not be done in one or two evenings time range. > > So you know what needs doing. I'm at a conference now, I'll be off for > a week in April and I also have another conference scheduled for May. > If you believe this needs addressing urgently, then I suggest you do > it right. Otherwise, I'll get to it when I have the time. > Unfortunately my TODO list runneth over. :( I have started already as you may have noticed. > But I have to say, I suspect it won't be as easy as you present it > because we have so many build configs that may fail. Let's see (with a hope)...
On Wed, Apr 17, 2024 at 8:40 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> Unfortunately my TODO list runneth over. :(
When in situations like this, patch the objective into
drivers/gpio/TODO so others can pick it up, that's why
I created the file, and it has actually helped a bit!
IMO you don't even need to send edits to this file for
review, it's just a work document. Just edit and commit
it in your tree.
Yours,
Linus Walleij
On Fri, Apr 19, 2024 at 03:29:13PM +0200, Linus Walleij wrote: > On Wed, Apr 17, 2024 at 8:40 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > Unfortunately my TODO list runneth over. :( > > When in situations like this, patch the objective into > drivers/gpio/TODO so others can pick it up, that's why > I created the file, and it has actually helped a bit! > > IMO you don't even need to send edits to this file for > review, it's just a work document. Just edit and commit > it in your tree. Btw, good point and thanks for the reminder, I believe I also can use it, but in my case I probably need to send a formal patch :-)
diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c index cb0cefaec37e..2f251b08173c 100644 --- a/drivers/gpio/gpiolib-of.c +++ b/drivers/gpio/gpiolib-of.c @@ -434,7 +434,7 @@ int of_get_named_gpio(const struct device_node *np, const char *propname, } EXPORT_SYMBOL_GPL(of_get_named_gpio); -/* Converts gpio_lookup_flags into bitmask of GPIO_* values */ +/* Converts of_gpio_flags into bitmask of GPIO_* values */ static unsigned long of_convert_gpio_flags(enum of_gpio_flags flags) { unsigned long lflags = GPIO_LOOKUP_FLAGS_DEFAULT; @@ -708,8 +708,7 @@ struct gpio_desc *of_find_gpio(struct device_node *np, const char *con_id, * @chip: GPIO chip whose hog is parsed * @idx: Index of the GPIO to parse * @name: GPIO line name - * @lflags: bitmask of gpio_lookup_flags GPIO_* values - returned from - * of_find_gpio() or of_parse_own_gpio() + * @lflags: bitmask of GPIO_* values - returned from *_find_gpio() * @dflags: gpiod_flags - optional GPIO initialization flags * * Returns GPIO descriptor to use with Linux GPIO API, or one of the errno diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 69542c2a5b70..cb66506bebde 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -2432,7 +2432,7 @@ static inline const char *function_name_or_default(const char *con_id) struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc, unsigned int hwnum, const char *label, - enum gpio_lookup_flags lflags, + unsigned long lflags, enum gpiod_flags dflags) { struct gpio_desc *desc = gpiochip_get_desc(gc, hwnum); @@ -4348,8 +4348,7 @@ EXPORT_SYMBOL_GPL(gpiod_get_optional); * gpiod_configure_flags - helper function to configure a given GPIO * @desc: gpio whose value will be assigned * @con_id: function within the GPIO consumer - * @lflags: bitmask of gpio_lookup_flags GPIO_* values - returned from - * of_find_gpio() or of_get_gpio_hog() + * @lflags: bitmask of GPIO_* values - returned from *_find_gpio() * @dflags: gpiod_flags - optional GPIO initialization flags * * Return 0 on success, -ENOENT if no GPIO has been assigned to the @@ -4475,8 +4474,7 @@ EXPORT_SYMBOL_GPL(gpiod_get_index_optional); * gpiod_hog - Hog the specified GPIO desc given the provided flags * @desc: gpio whose value will be assigned * @name: gpio line name - * @lflags: bitmask of gpio_lookup_flags GPIO_* values - returned from - * of_find_gpio() or of_get_gpio_hog() + * @lflags: bitmask of GPIO_* values - returned from *_find_gpio() * @dflags: gpiod_flags - optional GPIO initialization flags */ int gpiod_hog(struct gpio_desc *desc, const char *name, diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c index 9540a05247c2..be07d9ba8283 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c @@ -60,8 +60,8 @@ int brcms_led_register(struct brcms_info *wl) &sprom->gpio1, &sprom->gpio2, &sprom->gpio3 }; + unsigned long lflags = GPIO_ACTIVE_HIGH; int hwnum = -1; - enum gpio_lookup_flags lflags = GPIO_ACTIVE_HIGH; /* find radio enabled LED */ for (i = 0; i < BRCMS_LED_NO; i++) { diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h index f8617eaf08ba..0c506c7485bd 100644 --- a/include/linux/gpio/driver.h +++ b/include/linux/gpio/driver.h @@ -31,7 +31,6 @@ struct gpio_chip; struct gpio_desc; struct gpio_device; -enum gpio_lookup_flags; enum gpiod_flags; union gpio_irq_fwspec { @@ -789,7 +788,7 @@ gpiochip_remove_pin_ranges(struct gpio_chip *gc) struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc, unsigned int hwnum, const char *label, - enum gpio_lookup_flags lflags, + unsigned long lflags, enum gpiod_flags dflags); void gpiochip_free_own_desc(struct gpio_desc *desc); diff --git a/include/linux/gpio/machine.h b/include/linux/gpio/machine.h index 44e5f162973e..8221ee91c6f2 100644 --- a/include/linux/gpio/machine.h +++ b/include/linux/gpio/machine.h @@ -2,21 +2,11 @@ #ifndef __LINUX_GPIO_MACHINE_H #define __LINUX_GPIO_MACHINE_H +#include <dt-bindings/gpio/gpio.h> /* for GPIO_* flags */ #include <linux/types.h> -enum gpio_lookup_flags { - GPIO_ACTIVE_HIGH = (0 << 0), - GPIO_ACTIVE_LOW = (1 << 0), - GPIO_OPEN_DRAIN = (1 << 1), - GPIO_OPEN_SOURCE = (1 << 2), - GPIO_PERSISTENT = (0 << 3), - GPIO_TRANSITORY = (1 << 3), - GPIO_PULL_UP = (1 << 4), - GPIO_PULL_DOWN = (1 << 5), - GPIO_PULL_DISABLE = (1 << 6), - - GPIO_LOOKUP_FLAGS_DEFAULT = GPIO_ACTIVE_HIGH | GPIO_PERSISTENT, -}; +/* Additional GPIO_* flags for internal use */ +#define GPIO_LOOKUP_FLAGS_DEFAULT (GPIO_ACTIVE_HIGH | GPIO_PERSISTENT) /** * struct gpiod_lookup - lookup table @@ -27,7 +17,7 @@ enum gpio_lookup_flags { * U16_MAX to indicate that @key is a GPIO line name * @con_id: name of the GPIO from the device's point of view * @idx: index of the GPIO in case several GPIOs share the same name - * @flags: bitmask of gpio_lookup_flags GPIO_* values + * @flags: bitmask of GPIO_* values * * gpiod_lookup is a lookup table for associating GPIOs to specific devices and * functions using platform data. @@ -51,7 +41,7 @@ struct gpiod_lookup_table { * @chip_label: name of the chip the GPIO belongs to * @chip_hwnum: hardware number (i.e. relative to the chip) of the GPIO * @line_name: consumer name for the hogged line - * @lflags: bitmask of gpio_lookup_flags GPIO_* values + * @lflags: bitmask of GPIO_* values * @dflags: GPIO flags used to specify the direction and value */ struct gpiod_hog {
The GPIO_* flag definitions are *almost* duplicated in two files (with unmatches OPEN_SOURCE / OPEN_DRAIN). Moreover, some code relies on one set of definitions while the rest is on the other. Clean up this mess by providing only one source of the definitions to all. Fixes: b424808115cb ("brcm80211: brcmsmac: Move LEDs to GPIO descriptors") Fixes: 5923ea6c2ce6 ("gpio: pass lookup and descriptor flags to request_own") Fixes: fed7026adc7c ("gpiolib: Make use of enum gpio_lookup_flags consistent") Fixes: 4c0facddb7d8 ("gpio: core: Decouple open drain/source flag with active low/high") Fixes: 69d301fdd196 ("gpio: add DT bindings for existing consumer flags") Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/gpio/gpiolib-of.c | 5 ++--- drivers/gpio/gpiolib.c | 8 +++----- .../broadcom/brcm80211/brcmsmac/led.c | 2 +- include/linux/gpio/driver.h | 3 +-- include/linux/gpio/machine.h | 20 +++++-------------- 5 files changed, 12 insertions(+), 26 deletions(-)