[PATCH v1 0/5] gpio: First attempt to clean up headers

[PATCH v1 0/5] gpio: First attempt to clean up headers

[Andy Shevchenko]

Header inclusions in the _headers_ of GPIO library is semi-random or
outdated. Here is an attempt to fix the mess.

This is based on latest Linux Next with Pierluigi's patch which I
consider a good quick fix to the issue that can't be easily solved.

Patches 2-4 from me are pretty much straightforward, and are not
expected to fail (so may be applied as soon as test is done).
However the last one is to detect any other hellness of the mess.

Andy Shevchenko (4):
  gpio: Drop unused forward declaration from driver.h
  gpio: Deduplicate forward declarations in consumer.h
  gpio: Group forward declarations in consumer.h
  gpio: Clean up headers

Pierluigi Passaro (1):
  gpiolib: fix linker errors when GPIOLIB is disabled

 include/asm-generic/gpio.h    |  8 -----
 include/linux/gpio.h          |  9 ++----
 include/linux/gpio/consumer.h | 24 +++++++--------
 include/linux/gpio/driver.h   | 56 +++++++++++++++++++++++++++--------
 4 files changed, 59 insertions(+), 38 deletions(-)


base-commit: 9fbee811e479aca2f3523787cae1f46553141b40
-- 
2.39.0

[PATCH v1 1/5] gpiolib: fix linker errors when GPIOLIB is disabled

[Andy Shevchenko]

From: Pierluigi Passaro <pierluigi.p@variscite.com>

Both the functions gpiochip_request_own_desc and
gpiochip_free_own_desc are exported from
    drivers/gpio/gpiolib.c
but this file is compiled only when CONFIG_GPIOLIB is enabled.
Move the prototypes under "#ifdef CONFIG_GPIOLIB" and provide
reasonable definitions and includes in the "#else" branch.

Fixes: 9091373ab7ea ("gpio: remove less important #ifdef around declarations")
Signed-off-by: Pierluigi Passaro <pierluigi.p@variscite.com>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/gpio/driver.h | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 26a808fb8a25..67990908a040 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -751,6 +751,8 @@ gpiochip_remove_pin_ranges(struct gpio_chip *gc)
 
 #endif /* CONFIG_PINCTRL */
 
+#ifdef CONFIG_GPIOLIB
+
 struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc,
 					    unsigned int hwnum,
 					    const char *label,
@@ -758,8 +760,6 @@ struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc,
 					    enum gpiod_flags dflags);
 void gpiochip_free_own_desc(struct gpio_desc *desc);
 
-#ifdef CONFIG_GPIOLIB
-
 /* lock/unlock as IRQ */
 int gpiochip_lock_as_irq(struct gpio_chip *gc, unsigned int offset);
 void gpiochip_unlock_as_irq(struct gpio_chip *gc, unsigned int offset);
@@ -769,6 +769,25 @@ struct gpio_chip *gpiod_to_chip(const struct gpio_desc *desc);
 
 #else /* CONFIG_GPIOLIB */
 
+#include <linux/gpio/machine.h>
+#include <linux/gpio/consumer.h>
+
+static inline struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc,
+					    unsigned int hwnum,
+					    const char *label,
+					    enum gpio_lookup_flags lflags,
+					    enum gpiod_flags dflags)
+{
+	/* GPIO can never have been requested */
+	WARN_ON(1);
+	return ERR_PTR(-ENODEV);
+}
+
+static inline void gpiochip_free_own_desc(struct gpio_desc *desc)
+{
+	WARN_ON(1);
+}
+
 static inline struct gpio_chip *gpiod_to_chip(const struct gpio_desc *desc)
 {
 	/* GPIO can never have been requested */
-- 
2.39.0

[PATCH v1 2/5] gpio: Drop unused forward declaration from driver.h

[Andy Shevchenko]

There is no struct device_node pointers anywhere in the header,
drop unused forward declaration.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/gpio/driver.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 67990908a040..caf2376dd98b 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -16,7 +16,6 @@
 
 struct gpio_desc;
 struct of_phandle_args;
-struct device_node;
 struct seq_file;
 struct gpio_device;
 struct module;
-- 
2.39.0

[PATCH v1 3/5] gpio: Deduplicate forward declarations in consumer.h

[Andy Shevchenko]

The struct fwnode_handle pointer is used in both branches of ifdeffery,
no need to have a copy of the same in each of them, just make it global.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/gpio/consumer.h | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 59cb20cfac3d..a7eb8aa1e54c 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -8,6 +8,7 @@
 #include <linux/err.h>
 
 struct device;
+struct fwnode_handle;
 struct gpio_desc;
 struct gpio_array;
 
@@ -171,9 +172,6 @@ int gpiod_set_consumer_name(struct gpio_desc *desc, const char *name);
 struct gpio_desc *gpio_to_desc(unsigned gpio);
 int desc_to_gpio(const struct gpio_desc *desc);
 
-/* Child properties interface */
-struct fwnode_handle;
-
 struct gpio_desc *fwnode_gpiod_get_index(struct fwnode_handle *fwnode,
 					 const char *con_id, int index,
 					 enum gpiod_flags flags,
@@ -546,9 +544,6 @@ static inline int desc_to_gpio(const struct gpio_desc *desc)
 	return -EINVAL;
 }
 
-/* Child properties interface */
-struct fwnode_handle;
-
 static inline
 struct gpio_desc *fwnode_gpiod_get_index(struct fwnode_handle *fwnode,
 					 const char *con_id, int index,
-- 
2.39.0

[PATCH v1 4/5] gpio: Group forward declarations in consumer.h

[Andy Shevchenko]

For better maintenance group the forward declarations together.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/gpio/consumer.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index a7eb8aa1e54c..5432e5d5fbfb 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -7,6 +7,7 @@
 #include <linux/compiler_types.h>
 #include <linux/err.h>
 
+struct acpi_device;
 struct device;
 struct fwnode_handle;
 struct gpio_desc;
@@ -602,8 +603,6 @@ struct acpi_gpio_mapping {
 	unsigned int quirks;
 };
 
-struct acpi_device;
-
 #if IS_ENABLED(CONFIG_GPIOLIB) && IS_ENABLED(CONFIG_ACPI)
 
 int acpi_dev_add_driver_gpios(struct acpi_device *adev,
-- 
2.39.0

[PATCH v1 5/5] gpio: Clean up headers

[Andy Shevchenko]

There is a few things done:
- include only the headers we are direct user of
- when pointer is in use, provide a forward declaration
- add missing headers
- group generic headers and subsystem headers
- sort each group alphabetically

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/asm-generic/gpio.h    |  8 --------
 include/linux/gpio.h          |  9 +++------
 include/linux/gpio/consumer.h | 14 ++++++++++----
 include/linux/gpio/driver.h   | 34 ++++++++++++++++++++++++----------
 4 files changed, 37 insertions(+), 28 deletions(-)

diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index 22cb8c9efc1d..5d4d3529324c 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -3,11 +3,9 @@
 #define _ASM_GENERIC_GPIO_H
 
 #include <linux/types.h>
-#include <linux/errno.h>
 
 #ifdef CONFIG_GPIOLIB
 
-#include <linux/compiler.h>
 #include <linux/gpio/driver.h>
 #include <linux/gpio/consumer.h>
 
@@ -24,12 +22,7 @@
  */
 #define GPIO_DYNAMIC_BASE	512
 
-struct device;
 struct gpio;
-struct seq_file;
-struct module;
-struct device_node;
-struct gpio_desc;
 
 /* Always use the library code for GPIO management calls,
  * or when sleeping may be involved.
@@ -60,7 +53,6 @@ static inline void gpio_set_value_cansleep(unsigned gpio, int value)
 	return gpiod_set_raw_value_cansleep(gpio_to_desc(gpio), value);
 }
 
-
 /* A platform's <asm/gpio.h> code may want to inline the I/O calls when
  * the GPIO is constant and refers to some always-present controller,
  * giving direct access to chip registers and tight bitbanging loops.
diff --git a/include/linux/gpio.h b/include/linux/gpio.h
index 85beb236c925..cc28c8d5e93c 100644
--- a/include/linux/gpio.h
+++ b/include/linux/gpio.h
@@ -12,7 +12,7 @@
 #ifndef __LINUX_GPIO_H
 #define __LINUX_GPIO_H
 
-#include <linux/errno.h>
+struct device;
 
 /* see Documentation/driver-api/gpio/legacy.rst */
 
@@ -85,20 +85,17 @@ static inline int gpio_to_irq(unsigned int gpio)
 
 /* CONFIG_GPIOLIB: bindings for managed devices that want to request gpios */
 
-struct device;
-
 int devm_gpio_request(struct device *dev, unsigned gpio, const char *label);
 int devm_gpio_request_one(struct device *dev, unsigned gpio,
 			  unsigned long flags, const char *label);
 
 #else /* ! CONFIG_GPIOLIB */
 
-#include <linux/bug.h>
 #include <linux/kernel.h>
 #include <linux/types.h>
 
-struct device;
-struct gpio_chip;
+#include <asm/bug.h>
+#include <asm/errno.h>
 
 static inline bool gpio_is_valid(int number)
 {
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 5432e5d5fbfb..1c4385a00f88 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -3,15 +3,14 @@
 #define __LINUX_GPIO_CONSUMER_H
 
 #include <linux/bits.h>
-#include <linux/bug.h>
-#include <linux/compiler_types.h>
-#include <linux/err.h>
+#include <linux/types.h>
 
 struct acpi_device;
 struct device;
 struct fwnode_handle;
-struct gpio_desc;
+
 struct gpio_array;
+struct gpio_desc;
 
 /**
  * struct gpio_descs - Struct containing an array of descriptors that can be
@@ -185,8 +184,11 @@ struct gpio_desc *devm_fwnode_gpiod_get_index(struct device *dev,
 
 #else /* CONFIG_GPIOLIB */
 
+#include <linux/err.h>
 #include <linux/kernel.h>
 
+#include <asm/bug.h>
+
 static inline int gpiod_count(struct device *dev, const char *con_id)
 {
 	return 0;
@@ -616,6 +618,8 @@ struct gpio_desc *acpi_get_and_request_gpiod(char *path, unsigned int pin, char
 
 #else  /* CONFIG_GPIOLIB && CONFIG_ACPI */
 
+#include <linux/err.h>
+
 static inline int acpi_dev_add_driver_gpios(struct acpi_device *adev,
 			      const struct acpi_gpio_mapping *gpios)
 {
@@ -647,6 +651,8 @@ void gpiod_unexport(struct gpio_desc *desc);
 
 #else  /* CONFIG_GPIOLIB && CONFIG_GPIO_SYSFS */
 
+#include <asm/errno.h>
+
 static inline int gpiod_export(struct gpio_desc *desc,
 			       bool direction_may_change)
 {
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index caf2376dd98b..208c7cfeadb2 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -2,26 +2,29 @@
 #ifndef __LINUX_GPIO_DRIVER_H
 #define __LINUX_GPIO_DRIVER_H
 
-#include <linux/device.h>
-#include <linux/irq.h>
-#include <linux/irqchip/chained_irq.h>
+#include <linux/bits.h>
 #include <linux/irqdomain.h>
+#include <linux/irqhandler.h>
 #include <linux/lockdep.h>
-#include <linux/pinctrl/pinconf-generic.h>
 #include <linux/pinctrl/pinctrl.h>
 #include <linux/property.h>
+#include <linux/spinlock_types.h>
 #include <linux/types.h>
 
+#ifdef CONFIG_GENERIC_MSI_IRQ
 #include <asm/msi.h>
+#endif
 
-struct gpio_desc;
+struct device;
+struct irq_chip;
+struct irq_data;
+struct module;
 struct of_phandle_args;
+struct pinctrl_dev;
 struct seq_file;
-struct gpio_device;
-struct module;
-enum gpiod_flags;
-enum gpio_lookup_flags;
 
+struct gpio_desc;
+struct gpio_device;
 struct gpio_chip;
 
 union gpio_irq_fwspec {
@@ -691,6 +694,10 @@ bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gc,
 int gpiochip_irqchip_add_domain(struct gpio_chip *gc,
 				struct irq_domain *domain);
 #else
+
+#include <asm/bug.h>
+#include <asm/errno.h>
+
 static inline int gpiochip_irqchip_add_domain(struct gpio_chip *gc,
 					      struct irq_domain *domain)
 {
@@ -752,6 +759,9 @@ gpiochip_remove_pin_ranges(struct gpio_chip *gc)
 
 #ifdef CONFIG_GPIOLIB
 
+enum gpiod_flags;
+enum gpio_lookup_flags;
+
 struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc,
 					    unsigned int hwnum,
 					    const char *label,
@@ -768,8 +778,12 @@ struct gpio_chip *gpiod_to_chip(const struct gpio_desc *desc);
 
 #else /* CONFIG_GPIOLIB */
 
-#include <linux/gpio/machine.h>
+#include <linux/err.h>
+
+#include <asm/bug.h>
+
 #include <linux/gpio/consumer.h>
+#include <linux/gpio/machine.h>
 
 static inline struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc,
 					    unsigned int hwnum,
-- 
2.39.0

Re: [PATCH v1 1/5] gpiolib: fix linker errors when GPIOLIB is disabled

[Christophe Leroy]

Le 25/01/2023 à 21:10, Andy Shevchenko a écrit :
> From: Pierluigi Passaro <pierluigi.p@variscite.com>
> 
> Both the functions gpiochip_request_own_desc and
> gpiochip_free_own_desc are exported from
>      drivers/gpio/gpiolib.c
> but this file is compiled only when CONFIG_GPIOLIB is enabled.
> Move the prototypes under "#ifdef CONFIG_GPIOLIB" and provide
> reasonable definitions and includes in the "#else" branch.

Can you give more details on when and why link fails ?

You are adding a WARN(), I understand it mean the function should never 
ever be called. Shouldn't it be dropped completely by the compiler ? In 
that case, no call to gpiochip_request_own_desc() should be emitted and 
so link should be ok.

If link fails, it means we still have unexpected calls to 
gpiochip_request_own_desc() or gpiochip_free_own_desc(), and we should 
fix the root cause instead of hiding it with a WARN().

Christophe


> 
> Fixes: 9091373ab7ea ("gpio: remove less important #ifdef around declarations")
> Signed-off-by: Pierluigi Passaro <pierluigi.p@variscite.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   include/linux/gpio/driver.h | 23 +++++++++++++++++++++--
>   1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index 26a808fb8a25..67990908a040 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -751,6 +751,8 @@ gpiochip_remove_pin_ranges(struct gpio_chip *gc)
>   
>   #endif /* CONFIG_PINCTRL */
>   
> +#ifdef CONFIG_GPIOLIB
> +
>   struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc,
>   					    unsigned int hwnum,
>   					    const char *label,
> @@ -758,8 +760,6 @@ struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc,
>   					    enum gpiod_flags dflags);
>   void gpiochip_free_own_desc(struct gpio_desc *desc);
>   
> -#ifdef CONFIG_GPIOLIB
> -
>   /* lock/unlock as IRQ */
>   int gpiochip_lock_as_irq(struct gpio_chip *gc, unsigned int offset);
>   void gpiochip_unlock_as_irq(struct gpio_chip *gc, unsigned int offset);
> @@ -769,6 +769,25 @@ struct gpio_chip *gpiod_to_chip(const struct gpio_desc *desc);
>   
>   #else /* CONFIG_GPIOLIB */
>   
> +#include <linux/gpio/machine.h>
> +#include <linux/gpio/consumer.h>
> +
> +static inline struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc,
> +					    unsigned int hwnum,
> +					    const char *label,
> +					    enum gpio_lookup_flags lflags,
> +					    enum gpiod_flags dflags)
> +{
> +	/* GPIO can never have been requested */
> +	WARN_ON(1);
> +	return ERR_PTR(-ENODEV);
> +}
> +
> +static inline void gpiochip_free_own_desc(struct gpio_desc *desc)
> +{
> +	WARN_ON(1);
> +}
> +
>   static inline struct gpio_chip *gpiod_to_chip(const struct gpio_desc *desc)
>   {
>   	/* GPIO can never have been requested */

Re: [PATCH v1 1/5] gpiolib: fix linker errors when GPIOLIB is disabled

[Arnd Bergmann]

On Thu, Jan 26, 2023, at 09:14, Christophe Leroy wrote:
> Le 25/01/2023 à 21:10, Andy Shevchenko a écrit :
>> From: Pierluigi Passaro <pierluigi.p@variscite.com>
>> 
>> Both the functions gpiochip_request_own_desc and
>> gpiochip_free_own_desc are exported from
>>      drivers/gpio/gpiolib.c
>> but this file is compiled only when CONFIG_GPIOLIB is enabled.
>> Move the prototypes under "#ifdef CONFIG_GPIOLIB" and provide
>> reasonable definitions and includes in the "#else" branch.
>
> Can you give more details on when and why link fails ?
>
> You are adding a WARN(), I understand it mean the function should never 
> ever be called. Shouldn't it be dropped completely by the compiler ? In 
> that case, no call to gpiochip_request_own_desc() should be emitted and 
> so link should be ok.
>
> If link fails, it means we still have unexpected calls to 
> gpiochip_request_own_desc() or gpiochip_free_own_desc(), and we should 
> fix the root cause instead of hiding it with a WARN().

There are only a handful of files calling these functions:

$ git grep -l gpiochip_request_own_desc
Documentation/driver-api/gpio/driver.rst
arch/arm/mach-omap1/ams-delta-fiq.c
arch/arm/mach-omap1/board-ams-delta.c
drivers/gpio/gpio-mvebu.c
drivers/gpio/gpiolib-acpi.c
drivers/gpio/gpiolib.c
drivers/hid/hid-cp2112.c
drivers/memory/omap-gpmc.c
drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c
drivers/power/supply/collie_battery.c
drivers/spi/spi-bcm2835.c
include/linux/gpio/driver.h

All of these should already prevent the link failure through
a Kconfig 'depends on GPIOLIB' for the driver, or 'select GPIOLIB'
for the platform code. I checked all of the above and they seem fine.
If anything else calls the function, I'd add the same dependency
there.

      Arnd

Re: [PATCH v1 5/5] gpio: Clean up headers

[Arnd Bergmann]

On Wed, Jan 25, 2023, at 21:10, Andy Shevchenko wrote:
> There is a few things done:
> - include only the headers we are direct user of
> - when pointer is in use, provide a forward declaration
> - add missing headers
> - group generic headers and subsystem headers
> - sort each group alphabetically
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  include/asm-generic/gpio.h    |  8 --------
>  include/linux/gpio.h          |  9 +++------
>  include/linux/gpio/consumer.h | 14 ++++++++++----
>  include/linux/gpio/driver.h   | 34 ++++++++++++++++++++++++----------
>  4 files changed, 37 insertions(+), 28 deletions(-)

This change looks fine, but it conflicts with a slightly
broader cleanup that I meant to have already submitted,
folding include/asm-generic/gpio.h into linux/gpio.h and
removing the driver-side interface from that.

Let me try to dig out my series again, we should be able to
either use my version, or merge parts of this patch into it.

     Arnd

Re: [PATCH v1 5/5] gpio: Clean up headers

[Andy Shevchenko]

On Thu, Jan 26, 2023 at 09:42:32AM +0100, Arnd Bergmann wrote:
> On Wed, Jan 25, 2023, at 21:10, Andy Shevchenko wrote:
> > There is a few things done:
> > - include only the headers we are direct user of
> > - when pointer is in use, provide a forward declaration
> > - add missing headers
> > - group generic headers and subsystem headers
> > - sort each group alphabetically
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  include/asm-generic/gpio.h    |  8 --------
> >  include/linux/gpio.h          |  9 +++------
> >  include/linux/gpio/consumer.h | 14 ++++++++++----
> >  include/linux/gpio/driver.h   | 34 ++++++++++++++++++++++++----------
> >  4 files changed, 37 insertions(+), 28 deletions(-)
> 
> This change looks fine, but it conflicts with a slightly
> broader cleanup that I meant to have already submitted,
> folding include/asm-generic/gpio.h into linux/gpio.h and
> removing the driver-side interface from that.
> 
> Let me try to dig out my series again, we should be able to
> either use my version, or merge parts of this patch into it.

Can you share your patches, so I will rebase mine on top and see what's left?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/5] gpiolib: fix linker errors when GPIOLIB is disabled

[Andy Shevchenko]

On Thu, Jan 26, 2023 at 09:40:18AM +0100, Arnd Bergmann wrote:
> On Thu, Jan 26, 2023, at 09:14, Christophe Leroy wrote:
> > Le 25/01/2023 à 21:10, Andy Shevchenko a écrit :
> >> From: Pierluigi Passaro <pierluigi.p@variscite.com>
> >> 
> >> Both the functions gpiochip_request_own_desc and
> >> gpiochip_free_own_desc are exported from
> >>      drivers/gpio/gpiolib.c
> >> but this file is compiled only when CONFIG_GPIOLIB is enabled.
> >> Move the prototypes under "#ifdef CONFIG_GPIOLIB" and provide
> >> reasonable definitions and includes in the "#else" branch.
> >
> > Can you give more details on when and why link fails ?
> >
> > You are adding a WARN(), I understand it mean the function should never 
> > ever be called. Shouldn't it be dropped completely by the compiler ? In 
> > that case, no call to gpiochip_request_own_desc() should be emitted and 
> > so link should be ok.
> >
> > If link fails, it means we still have unexpected calls to 
> > gpiochip_request_own_desc() or gpiochip_free_own_desc(), and we should 
> > fix the root cause instead of hiding it with a WARN().
> 
> There are only a handful of files calling these functions:
> 
> $ git grep -l gpiochip_request_own_desc
> Documentation/driver-api/gpio/driver.rst
> arch/arm/mach-omap1/ams-delta-fiq.c
> arch/arm/mach-omap1/board-ams-delta.c
> drivers/gpio/gpio-mvebu.c
> drivers/gpio/gpiolib-acpi.c
> drivers/gpio/gpiolib.c
> drivers/hid/hid-cp2112.c
> drivers/memory/omap-gpmc.c
> drivers/net/wireless/broadcom/brcm80211/brcmsmac/led.c
> drivers/power/supply/collie_battery.c
> drivers/spi/spi-bcm2835.c
> include/linux/gpio/driver.h
> 
> All of these should already prevent the link failure through
> a Kconfig 'depends on GPIOLIB' for the driver, or 'select GPIOLIB'
> for the platform code. I checked all of the above and they seem fine.
> If anything else calls the function, I'd add the same dependency
> there.

So, you think it's worth to send a few separate fixes as adding that
dependency? But doesn't it feel like a papering over the issue with
that APIs used in some of the drivers in the first place?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/5] gpiolib: fix linker errors when GPIOLIB is disabled

[Andy Shevchenko]

On Thu, Jan 26, 2023 at 08:14:49AM +0000, Christophe Leroy wrote:
> Le 25/01/2023 à 21:10, Andy Shevchenko a écrit :
> > From: Pierluigi Passaro <pierluigi.p@variscite.com>
> > 
> > Both the functions gpiochip_request_own_desc and
> > gpiochip_free_own_desc are exported from
> >      drivers/gpio/gpiolib.c
> > but this file is compiled only when CONFIG_GPIOLIB is enabled.
> > Move the prototypes under "#ifdef CONFIG_GPIOLIB" and provide
> > reasonable definitions and includes in the "#else" branch.
> 
> Can you give more details on when and why link fails ?
> 
> You are adding a WARN(), I understand it mean the function should never 
> ever be called. Shouldn't it be dropped completely by the compiler ? In 
> that case, no call to gpiochip_request_own_desc() should be emitted and 
> so link should be ok.
> 
> If link fails, it means we still have unexpected calls to 
> gpiochip_request_own_desc() or gpiochip_free_own_desc(), and we should 
> fix the root cause instead of hiding it with a WARN().

I agree, but what do you suggest exactly? I think the calls to that functions
shouldn't be in the some drivers as it's layering violation (they are not a
GPIO chips to begin with). Simply adding a dependency not better than this one.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 5/5] gpio: Clean up headers

[Linus Walleij]

On Wed, Jan 25, 2023 at 9:10 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> There is a few things done:
> - include only the headers we are direct user of
> - when pointer is in use, provide a forward declaration
> - add missing headers
> - group generic headers and subsystem headers
> - sort each group alphabetically
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Just to add to the confusion I was also pursuing a series of cleanups directed
at just removing <linux/gpio/driver.h> from <linux/gpio.h>:
https://lore.kernel.org/linux-gpio/CACRpkdb6yMqTkrJOg+K46RZ1478-gbxh6=tw4bzWmd--5nj_Bw@mail.gmail.com/

Right now I don't know what to do :D

Linus

Re: [PATCH v1 1/5] gpiolib: fix linker errors when GPIOLIB is disabled

[Arnd Bergmann]

On Thu, Jan 26, 2023, at 11:17, Andy Shevchenko wrote:
> On Thu, Jan 26, 2023 at 09:40:18AM +0100, Arnd Bergmann wrote:
>> On Thu, Jan 26, 2023, at 09:14, Christophe Leroy wrote:
>> 
>> All of these should already prevent the link failure through
>> a Kconfig 'depends on GPIOLIB' for the driver, or 'select GPIOLIB'
>> for the platform code. I checked all of the above and they seem fine.
>> If anything else calls the function, I'd add the same dependency
>> there.
>
> So, you think it's worth to send a few separate fixes as adding that
> dependency? But doesn't it feel like a papering over the issue with
> that APIs used in some of the drivers in the first place?

If there are drivers that use the interfaces but shouldn't then
fixing those drivers is clearly better than adding a dependency,
but we can decide that when someone sends a patch.

Adding a stub helper that can never be used legitimately
but turns a build time error into a run time warning seems
counterproductive to me, as the CI systems are no longer
able to report these automatically.

    Arnd

Re: [PATCH v1 1/5] gpiolib: fix linker errors when GPIOLIB is disabled

[Andy Shevchenko]

On Thu, Jan 26, 2023 at 11:27:51AM +0100, Arnd Bergmann wrote:
> On Thu, Jan 26, 2023, at 11:17, Andy Shevchenko wrote:
> > On Thu, Jan 26, 2023 at 09:40:18AM +0100, Arnd Bergmann wrote:
> >> On Thu, Jan 26, 2023, at 09:14, Christophe Leroy wrote:
> >> 
> >> All of these should already prevent the link failure through
> >> a Kconfig 'depends on GPIOLIB' for the driver, or 'select GPIOLIB'
> >> for the platform code. I checked all of the above and they seem fine.
> >> If anything else calls the function, I'd add the same dependency
> >> there.
> >
> > So, you think it's worth to send a few separate fixes as adding that
> > dependency? But doesn't it feel like a papering over the issue with
> > that APIs used in some of the drivers in the first place?
> 
> If there are drivers that use the interfaces but shouldn't then
> fixing those drivers is clearly better than adding a dependency,
> but we can decide that when someone sends a patch.
> 
> Adding a stub helper that can never be used legitimately
> but turns a build time error into a run time warning seems
> counterproductive to me, as the CI systems are no longer
> able to report these automatically.

What about adding ifdeffery in their code instead with a FIXME comment? So
we will know that it's ugly and needs to be solved better sooner than later.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 5/5] gpio: Clean up headers

[Andy Shevchenko]

On Thu, Jan 26, 2023 at 11:22:00AM +0100, Linus Walleij wrote:
> On Wed, Jan 25, 2023 at 9:10 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> 
> > There is a few things done:
> > - include only the headers we are direct user of
> > - when pointer is in use, provide a forward declaration
> > - add missing headers
> > - group generic headers and subsystem headers
> > - sort each group alphabetically
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Just to add to the confusion I was also pursuing a series of cleanups directed
> at just removing <linux/gpio/driver.h> from <linux/gpio.h>:
> https://lore.kernel.org/linux-gpio/CACRpkdb6yMqTkrJOg+K46RZ1478-gbxh6=tw4bzWmd--5nj_Bw@mail.gmail.com/
> 
> Right now I don't know what to do :D

I saw your series and find them very good!
Can we have them applied?

Arnd's one probably also needs to be considered sooner.

This one can be postponed.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 5/5] gpio: Clean up headers

[Linus Walleij]

On Thu, Jan 26, 2023 at 1:16 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> > Just to add to the confusion I was also pursuing a series of cleanups directed
> > at just removing <linux/gpio/driver.h> from <linux/gpio.h>:
> > https://lore.kernel.org/linux-gpio/CACRpkdb6yMqTkrJOg+K46RZ1478-gbxh6=tw4bzWmd--5nj_Bw@mail.gmail.com/
> >
> > Right now I don't know what to do :D
>
> I saw your series and find them very good!
> Can we have them applied?

I see that they will cause conflicts (patching files deleted in the
big board removal)
so I don't want to push them for the moment. I will send them out again though.

Yours,
Linus Walleij

Re: [PATCH v1 1/5] gpiolib: fix linker errors when GPIOLIB is disabled

[Christophe Leroy]

Le 26/01/2023 à 11:19, Andy Shevchenko a écrit :
> On Thu, Jan 26, 2023 at 08:14:49AM +0000, Christophe Leroy wrote:
>> Le 25/01/2023 à 21:10, Andy Shevchenko a écrit :
>>> From: Pierluigi Passaro <pierluigi.p@variscite.com>
>>>
>>> Both the functions gpiochip_request_own_desc and
>>> gpiochip_free_own_desc are exported from
>>>       drivers/gpio/gpiolib.c
>>> but this file is compiled only when CONFIG_GPIOLIB is enabled.
>>> Move the prototypes under "#ifdef CONFIG_GPIOLIB" and provide
>>> reasonable definitions and includes in the "#else" branch.
>>
>> Can you give more details on when and why link fails ?
>>
>> You are adding a WARN(), I understand it mean the function should never
>> ever be called. Shouldn't it be dropped completely by the compiler ? In
>> that case, no call to gpiochip_request_own_desc() should be emitted and
>> so link should be ok.
>>
>> If link fails, it means we still have unexpected calls to
>> gpiochip_request_own_desc() or gpiochip_free_own_desc(), and we should
>> fix the root cause instead of hiding it with a WARN().
> 
> I agree, but what do you suggest exactly? I think the calls to that functions
> shouldn't be in the some drivers as it's layering violation (they are not a
> GPIO chips to begin with). Simply adding a dependency not better than this one.
> 

My suggestion is to go step by step. First step is to explicitely list 
drivers that call those functions without selecting GPIOLIB.

Once we have this list we can see one by one how we solve it.

And if we want to catch the problem before the final link, then I think 
we may use BUILD_BUG() but not WARN or WARN_ON.

Christophe

Re: [PATCH v1 1/5] gpiolib: fix linker errors when GPIOLIB is disabled

[Arnd Bergmann]

On Thu, Jan 26, 2023, at 13:44, Christophe Leroy wrote:
> Le 26/01/2023 à 11:19, Andy Shevchenko a écrit :
>> On Thu, Jan 26, 2023 at 08:14:49AM +0000, Christophe Leroy wrote:
>>> Le 25/01/2023 à 21:10, Andy Shevchenko a écrit :
>>>> From: Pierluigi Passaro <pierluigi.p@variscite.com>
>>>>
>>>> Both the functions gpiochip_request_own_desc and
>>>> gpiochip_free_own_desc are exported from
>>>>       drivers/gpio/gpiolib.c
>>>> but this file is compiled only when CONFIG_GPIOLIB is enabled.
>>>> Move the prototypes under "#ifdef CONFIG_GPIOLIB" and provide
>>>> reasonable definitions and includes in the "#else" branch.
>>>
>>> Can you give more details on when and why link fails ?
>>>
>>> You are adding a WARN(), I understand it mean the function should never
>>> ever be called. Shouldn't it be dropped completely by the compiler ? In
>>> that case, no call to gpiochip_request_own_desc() should be emitted and
>>> so link should be ok.
>>>
>>> If link fails, it means we still have unexpected calls to
>>> gpiochip_request_own_desc() or gpiochip_free_own_desc(), and we should
>>> fix the root cause instead of hiding it with a WARN().
>> 
>> I agree, but what do you suggest exactly? I think the calls to that functions
>> shouldn't be in the some drivers as it's layering violation (they are not a
>> GPIO chips to begin with). Simply adding a dependency not better than this one.
>> 
>
> My suggestion is to go step by step. First step is to explicitely list 
> drivers that call those functions without selecting GPIOLIB.

I tried that and sent the list of the drivers that call these functions,
but as I wrote, all of them already require GPIOLIB to be set.

This means either I made a mistake in my search, or the problem
has already been fixed. Either way, I think Andy should provide
the exact build failure he observed so we know what caller caused
the issue.

     Arnd

Re: [PATCH v1 5/5] gpio: Clean up headers

[Arnd Bergmann]

On Thu, Jan 26, 2023, at 11:16, Andy Shevchenko wrote:
> On Thu, Jan 26, 2023 at 09:42:32AM +0100, Arnd Bergmann wrote:
>> On Wed, Jan 25, 2023, at 21:10, Andy Shevchenko wrote:
>> > There is a few things done:
>> > - include only the headers we are direct user of
>> > - when pointer is in use, provide a forward declaration
>> > - add missing headers
>> > - group generic headers and subsystem headers
>> > - sort each group alphabetically
>> >
>> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> > ---
>> >  include/asm-generic/gpio.h    |  8 --------
>> >  include/linux/gpio.h          |  9 +++------
>> >  include/linux/gpio/consumer.h | 14 ++++++++++----
>> >  include/linux/gpio/driver.h   | 34 ++++++++++++++++++++++++----------
>> >  4 files changed, 37 insertions(+), 28 deletions(-)
>> 
>> This change looks fine, but it conflicts with a slightly
>> broader cleanup that I meant to have already submitted,
>> folding include/asm-generic/gpio.h into linux/gpio.h and
>> removing the driver-side interface from that.
>> 
>> Let me try to dig out my series again, we should be able to
>> either use my version, or merge parts of this patch into it.
>
> Can you share your patches, so I will rebase mine on top and see what's left?
>

See the top patches of my randconfig branch at

https://git.kernel.org/pub/scm/linux/kernel/git/arnd/playground.git/log/?h=randconfig-6.3-next

There are eight cleanup patches for gpiolib, plus another seven
patches for individual drivers. I just rebased the tree on top of
linux-next, fixed previous build regression, and will send the
gpiolib patches later on if there are no new problems with it.

   Arnd

Re: [PATCH v1 1/5] gpiolib: fix linker errors when GPIOLIB is disabled

[Andy Shevchenko]

On Thu, Jan 26, 2023 at 01:56:26PM +0100, Arnd Bergmann wrote:
> On Thu, Jan 26, 2023, at 13:44, Christophe Leroy wrote:
> > Le 26/01/2023 à 11:19, Andy Shevchenko a écrit :
> >> On Thu, Jan 26, 2023 at 08:14:49AM +0000, Christophe Leroy wrote:
> >>> Le 25/01/2023 à 21:10, Andy Shevchenko a écrit :
> >>>> From: Pierluigi Passaro <pierluigi.p@variscite.com>
> >>>>
> >>>> Both the functions gpiochip_request_own_desc and
> >>>> gpiochip_free_own_desc are exported from
> >>>>       drivers/gpio/gpiolib.c
> >>>> but this file is compiled only when CONFIG_GPIOLIB is enabled.
> >>>> Move the prototypes under "#ifdef CONFIG_GPIOLIB" and provide
> >>>> reasonable definitions and includes in the "#else" branch.
> >>>
> >>> Can you give more details on when and why link fails ?
> >>>
> >>> You are adding a WARN(), I understand it mean the function should never
> >>> ever be called. Shouldn't it be dropped completely by the compiler ? In
> >>> that case, no call to gpiochip_request_own_desc() should be emitted and
> >>> so link should be ok.
> >>>
> >>> If link fails, it means we still have unexpected calls to
> >>> gpiochip_request_own_desc() or gpiochip_free_own_desc(), and we should
> >>> fix the root cause instead of hiding it with a WARN().
> >> 
> >> I agree, but what do you suggest exactly? I think the calls to that functions
> >> shouldn't be in the some drivers as it's layering violation (they are not a
> >> GPIO chips to begin with). Simply adding a dependency not better than this one.
> >> 
> >
> > My suggestion is to go step by step. First step is to explicitely list 
> > drivers that call those functions without selecting GPIOLIB.
> 
> I tried that and sent the list of the drivers that call these functions,
> but as I wrote, all of them already require GPIOLIB to be set.
> 
> This means either I made a mistake in my search, or the problem
> has already been fixed. Either way, I think Andy should provide
> the exact build failure he observed so we know what caller caused
> the issue.

I believe it's not me, who first reported it. So, Pierluigi, can you point
out to the LKP message that reported the issue?

P.S> LKP sometimes finds a really twisted configurations to probe on.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/5] gpiolib: fix linker errors when GPIOLIB is disabled

[Pierluigi Passaro]

On Thu, Jan 26, 2023 at 02:29PM +0100, Arnd Bergmann wrote:
> On Thu, Jan 26, 2023 at 01:56:26PM +0100, Arnd Bergmann wrote:
> > On Thu, Jan 26, 2023, at 13:44, Christophe Leroy wrote:
> > > Le 26/01/2023 à 11:19, Andy Shevchenko a écrit :
> > >> On Thu, Jan 26, 2023 at 08:14:49AM +0000, Christophe Leroy wrote:
> > >>> Le 25/01/2023 à 21:10, Andy Shevchenko a écrit :
> > >>>> From: Pierluigi Passaro <pierluigi.p@variscite.com>
> > >>>>
> > >>>> Both the functions gpiochip_request_own_desc and
> > >>>> gpiochip_free_own_desc are exported from
> > >>>>       drivers/gpio/gpiolib.c
> > >>>> but this file is compiled only when CONFIG_GPIOLIB is enabled.
> > >>>> Move the prototypes under "#ifdef CONFIG_GPIOLIB" and provide
> > >>>> reasonable definitions and includes in the "#else" branch.
> > >>>
> > >>> Can you give more details on when and why link fails ?
> > >>>
> > >>> You are adding a WARN(), I understand it mean the function should never
> > >>> ever be called. Shouldn't it be dropped completely by the compiler ? In
> > >>> that case, no call to gpiochip_request_own_desc() should be emitted and
> > >>> so link should be ok.
> > >>>
> > >>> If link fails, it means we still have unexpected calls to
> > >>> gpiochip_request_own_desc() or gpiochip_free_own_desc(), and we should
> > >>> fix the root cause instead of hiding it with a WARN().
> > >> 
> > >> I agree, but what do you suggest exactly? I think the calls to that functions
> > >> shouldn't be in the some drivers as it's layering violation (they are not a
> > >> GPIO chips to begin with). Simply adding a dependency not better than this one.
> > >> 
> > >
> > > My suggestion is to go step by step. First step is to explicitely list 
> > > drivers that call those functions without selecting GPIOLIB.
> > 
> > I tried that and sent the list of the drivers that call these functions,
> > but as I wrote, all of them already require GPIOLIB to be set.
> > 
> > This means either I made a mistake in my search, or the problem
> > has already been fixed. Either way, I think Andy should provide
> > the exact build failure he observed so we know what caller caused
> > the issue.
> 
> I believe it's not me, who first reported it. So, Pierluigi, can you point
> out to the LKP message that reported the issue?
> 
> P.S> LKP sometimes finds a really twisted configurations to probe on.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
I've received the following messages:
- https://lore.kernel.org/all/202301240409.tZdm0o0a-lkp@intel.com/
- https://lore.kernel.org/all/202301240439.wYz6uU0k-lkp@intel.com/
- https://lore.kernel.org/all/20230124075600.649bd7bb@canb.auug.org.au/
Please let me know if you need further details.
Regards
Pier

Re: [PATCH v1 1/5] gpiolib: fix linker errors when GPIOLIB is disabled

[Arnd Bergmann]

On Thu, Jan 26, 2023, at 14:41, Pierluigi Passaro wrote:
> On Thu, Jan 26, 2023 at 02:29PM +0100, Arnd Bergmann wrote:
>> > >>>
>> > >>> If link fails, it means we still have unexpected calls to
>> > >>> gpiochip_request_own_desc() or gpiochip_free_own_desc(), and we should
>> > >>> fix the root cause instead of hiding it with a WARN().

>> > This means either I made a mistake in my search, or the problem
>> > has already been fixed. Either way, I think Andy should provide
>> > the exact build failure he observed so we know what caller caused
>> > the issue.
>> 
>> I believe it's not me, who first reported it. So, Pierluigi, can you point
>> out to the LKP message that reported the issue?
>> 
>> P.S> LKP sometimes finds a really twisted configurations to probe on.
>> 
>> 
> I've received the following messages:
> - https://lore.kernel.org/all/202301240409.tZdm0o0a-lkp@intel.com/
> - https://lore.kernel.org/all/202301240439.wYz6uU0k-lkp@intel.com/
> - https://lore.kernel.org/all/20230124075600.649bd7bb@canb.auug.org.au/
> Please let me know if you need further details.

I think these three are all regressions that are caused by the patch
in this thread, rather than the original problem that it was trying
to fix.

The one we're looking for is a randconfig bug that showed up
as a link failure when referencing gpiochip_request_own_desc
or gpiochip_free_own_desc.

      Arnd