[PATCH net v2] net: smc91x: Fix build without gpiolib

[PATCH net v2] net: smc91x: Fix build without gpiolib

[Tobias Klauser]

If GPIOLIB=n the following build errors occur:

drivers/net/ethernet/smsc/smc91x.c: In function 'try_toggle_control_gpio':
drivers/net/ethernet/smsc/smc91x.c:2204:2: error: implicit declaration of function 'devm_gpiod_get_index' [-Werror=implicit-function-declaration]
drivers/net/ethernet/smsc/smc91x.c:2204:7: warning: assignment makes pointer from integer without a cast [enabled by default]
drivers/net/ethernet/smsc/smc91x.c:2213:2: error: implicit declaration of function 'gpiod_direction_output' [-Werror=implicit-function-declaration]
drivers/net/ethernet/smsc/smc91x.c:2216:3: error: implicit declaration of function 'devm_gpiod_put' [-Werror=implicit-function-declaration]
drivers/net/ethernet/smsc/smc91x.c:2222:2: error: implicit declaration of function 'gpiod_set_value_cansleep' [-Werror=implicit-function-declaration]

Since the toggling of the GPIOs is an optional feature, define
try_toggle_control_gpio only if GPIOLIB is enabled.

Fixes: 7d2911c4381 ("net: smc91x: Fix gpios for device tree based booting")
Cc: Tony Lindgren <tony@atomide.com>
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
 drivers/net/ethernet/smsc/smc91x.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/smsc/smc91x.c b/drivers/net/ethernet/smsc/smc91x.c
index 6cc3cf6..050bcb6 100644
--- a/drivers/net/ethernet/smsc/smc91x.c
+++ b/drivers/net/ethernet/smsc/smc91x.c
@@ -2193,6 +2193,7 @@ MODULE_DEVICE_TABLE(of, smc91x_match);
 /**
  * of_try_set_control_gpio - configure a gpio if it exists
  */
+#ifdef CONFIG_GPIOLIB
 static int try_toggle_control_gpio(struct device *dev,
 				   struct gpio_desc **desc,
 				   const char *name, int index,
@@ -2224,7 +2225,16 @@ static int try_toggle_control_gpio(struct device *dev,
 
 	return 0;
 }
-#endif
+#else
+static int try_toggle_control_gpio(struct device *dev,
+				   struct gpio_desc **desc,
+				   const char *name, int index,
+				   int value, unsigned int nsdelay)
+{
+	return 0;
+}
+#endif /* CONFIG_GPIOLIB */
+#endif /* CONFIG_OF */
 
 /*
  * smc_init(void)
-- 
2.2.0

Re: [PATCH net v2] net: smc91x: Fix build without gpiolib

[Tony Lindgren]

* Tobias Klauser <tklauser@distanz.ch> [141212 08:09]:
> If GPIOLIB=n the following build errors occur:
> 
> drivers/net/ethernet/smsc/smc91x.c: In function 'try_toggle_control_gpio':
> drivers/net/ethernet/smsc/smc91x.c:2204:2: error: implicit declaration of function 'devm_gpiod_get_index' [-Werror=implicit-function-declaration]
> drivers/net/ethernet/smsc/smc91x.c:2204:7: warning: assignment makes pointer from integer without a cast [enabled by default]
> drivers/net/ethernet/smsc/smc91x.c:2213:2: error: implicit declaration of function 'gpiod_direction_output' [-Werror=implicit-function-declaration]
> drivers/net/ethernet/smsc/smc91x.c:2216:3: error: implicit declaration of function 'devm_gpiod_put' [-Werror=implicit-function-declaration]
> drivers/net/ethernet/smsc/smc91x.c:2222:2: error: implicit declaration of function 'gpiod_set_value_cansleep' [-Werror=implicit-function-declaration]
> 
> Since the toggling of the GPIOs is an optional feature, define
> try_toggle_control_gpio only if GPIOLIB is enabled.

Oops sorry about that, I guess my systems have it all selected so
randconfig builds did not expose it.

Would it make sense to add the missing gpio stub into
include/linux/gpio/consumer.h instead?

Regards,

Tony
 
> Fixes: 7d2911c4381 ("net: smc91x: Fix gpios for device tree based booting")
> Cc: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
> ---
>  drivers/net/ethernet/smsc/smc91x.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/smsc/smc91x.c b/drivers/net/ethernet/smsc/smc91x.c
> index 6cc3cf6..050bcb6 100644
> --- a/drivers/net/ethernet/smsc/smc91x.c
> +++ b/drivers/net/ethernet/smsc/smc91x.c
> @@ -2193,6 +2193,7 @@ MODULE_DEVICE_TABLE(of, smc91x_match);
>  /**
>   * of_try_set_control_gpio - configure a gpio if it exists
>   */
> +#ifdef CONFIG_GPIOLIB
>  static int try_toggle_control_gpio(struct device *dev,
>  				   struct gpio_desc **desc,
>  				   const char *name, int index,
> @@ -2224,7 +2225,16 @@ static int try_toggle_control_gpio(struct device *dev,
>  
>  	return 0;
>  }
> -#endif
> +#else
> +static int try_toggle_control_gpio(struct device *dev,
> +				   struct gpio_desc **desc,
> +				   const char *name, int index,
> +				   int value, unsigned int nsdelay)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_GPIOLIB */
> +#endif /* CONFIG_OF */
>  
>  /*
>   * smc_init(void)
> -- 
> 2.2.0
> 
> 

Re: [PATCH net v2] net: smc91x: Fix build without gpiolib

[David Miller]

From: Tony Lindgren <tony@atomide.com>
Date: Fri, 12 Dec 2014 08:11:00 -0800

> * Tobias Klauser <tklauser@distanz.ch> [141212 08:09]:
>> If GPIOLIB=n the following build errors occur:
>> 
>> drivers/net/ethernet/smsc/smc91x.c: In function 'try_toggle_control_gpio':
>> drivers/net/ethernet/smsc/smc91x.c:2204:2: error: implicit declaration of function 'devm_gpiod_get_index' [-Werror=implicit-function-declaration]
>> drivers/net/ethernet/smsc/smc91x.c:2204:7: warning: assignment makes pointer from integer without a cast [enabled by default]
>> drivers/net/ethernet/smsc/smc91x.c:2213:2: error: implicit declaration of function 'gpiod_direction_output' [-Werror=implicit-function-declaration]
>> drivers/net/ethernet/smsc/smc91x.c:2216:3: error: implicit declaration of function 'devm_gpiod_put' [-Werror=implicit-function-declaration]
>> drivers/net/ethernet/smsc/smc91x.c:2222:2: error: implicit declaration of function 'gpiod_set_value_cansleep' [-Werror=implicit-function-declaration]
>> 
>> Since the toggling of the GPIOs is an optional feature, define
>> try_toggle_control_gpio only if GPIOLIB is enabled.
> 
> Oops sorry about that, I guess my systems have it all selected so
> randconfig builds did not expose it.
> 
> Would it make sense to add the missing gpio stub into
> include/linux/gpio/consumer.h instead?

I really don't like these kinds of things at all.

If GPIO is required to fully program the hardware properly,
the smc91x driver should have it as a dependency.

Re: [PATCH net v2] net: smc91x: Fix build without gpiolib

[Tobias Klauser]

On 2014-12-12 at 17:21:19 +0100, David Miller <davem@davemloft.net> wrote:
> From: Tony Lindgren <tony@atomide.com>
> Date: Fri, 12 Dec 2014 08:11:00 -0800
> 
> > * Tobias Klauser <tklauser@distanz.ch> [141212 08:09]:
> >> If GPIOLIB=n the following build errors occur:
> >> 
> >> drivers/net/ethernet/smsc/smc91x.c: In function 'try_toggle_control_gpio':
> >> drivers/net/ethernet/smsc/smc91x.c:2204:2: error: implicit declaration of function 'devm_gpiod_get_index' [-Werror=implicit-function-declaration]
> >> drivers/net/ethernet/smsc/smc91x.c:2204:7: warning: assignment makes pointer from integer without a cast [enabled by default]
> >> drivers/net/ethernet/smsc/smc91x.c:2213:2: error: implicit declaration of function 'gpiod_direction_output' [-Werror=implicit-function-declaration]
> >> drivers/net/ethernet/smsc/smc91x.c:2216:3: error: implicit declaration of function 'devm_gpiod_put' [-Werror=implicit-function-declaration]
> >> drivers/net/ethernet/smsc/smc91x.c:2222:2: error: implicit declaration of function 'gpiod_set_value_cansleep' [-Werror=implicit-function-declaration]
> >> 
> >> Since the toggling of the GPIOs is an optional feature, define
> >> try_toggle_control_gpio only if GPIOLIB is enabled.
> > 
> > Oops sorry about that, I guess my systems have it all selected so
> > randconfig builds did not expose it.
> > 
> > Would it make sense to add the missing gpio stub into
> > include/linux/gpio/consumer.h instead?
> 
> I really don't like these kinds of things at all.
> 
> If GPIO is required to fully program the hardware properly,
> the smc91x driver should have it as a dependency.

GPIO is not needed in all configurations of the chip, they are optional.
That's why I was going for the ugly #ifdef solution.

It seems that there are stubs for the functions in question in
linux/gpio/consumer.h already as Tony suggested. Thus, adding an
#include <linux/gpio/consumer.h> to the driver should be enough. I'll
send an updated patch.

Re: [PATCH net v2] net: smc91x: Fix build without gpiolib

[David Miller]

From: Tobias Klauser <tklauser@distanz.ch>
Date: Fri, 12 Dec 2014 17:27:46 +0100

> GPIO is not needed in all configurations of the chip, they are optional.
> That's why I was going for the ugly #ifdef solution.
> 
> It seems that there are stubs for the functions in question in
> linux/gpio/consumer.h already as Tony suggested. Thus, adding an
> #include <linux/gpio/consumer.h> to the driver should be enough. I'll
> send an updated patch.

So for the "configurations" that need it, how does the user figure out
that GPIO is needed?

In my opinion, if the code blocks enabling the configurations that
need this are enabled, so should GPIO be depended upon.

I think, at a minimum, when CONFIG_OF is enabled smsc91x should
require GPIO.

Re: [PATCH net v2] net: smc91x: Fix build without gpiolib

[Tony Lindgren]

* David Miller <davem@davemloft.net> [141212 08:32]:
> From: Tobias Klauser <tklauser@distanz.ch>
> Date: Fri, 12 Dec 2014 17:27:46 +0100
> 
> > GPIO is not needed in all configurations of the chip, they are optional.
> > That's why I was going for the ugly #ifdef solution.
> > 
> > It seems that there are stubs for the functions in question in
> > linux/gpio/consumer.h already as Tony suggested. Thus, adding an
> > #include <linux/gpio/consumer.h> to the driver should be enough. I'll
> > send an updated patch.
> 
> So for the "configurations" that need it, how does the user figure out
> that GPIO is needed?
> 
> In my opinion, if the code blocks enabling the configurations that
> need this are enabled, so should GPIO be depended upon.
> 
> I think, at a minimum, when CONFIG_OF is enabled smsc91x should
> require GPIO.

Well in many cases these control GPIOs, or some part of them, are
hardwired with pulls. That's why they are optional.

Regards,

Tony

Re: [PATCH net v2] net: smc91x: Fix build without gpiolib

[Tobias Klauser]

On 2014-12-12 at 17:30:20 +0100, David Miller <davem@davemloft.net> wrote:
> From: Tobias Klauser <tklauser@distanz.ch>
> Date: Fri, 12 Dec 2014 17:27:46 +0100
> 
> > GPIO is not needed in all configurations of the chip, they are optional.
> > That's why I was going for the ugly #ifdef solution.
> > 
> > It seems that there are stubs for the functions in question in
> > linux/gpio/consumer.h already as Tony suggested. Thus, adding an
> > #include <linux/gpio/consumer.h> to the driver should be enough. I'll
> > send an updated patch.
> 
> So for the "configurations" that need it, how does the user figure out
> that GPIO is needed?

Probably ionly by consulting the manual of the board usingthe chip.
Which presumably very few users will do.

> In my opinion, if the code blocks enabling the configurations that
> need this are enabled, so should GPIO be depended upon.
> 
> I think, at a minimum, when CONFIG_OF is enabled smsc91x should
> require GPIO.

Agreed. This is the better solution, causing less surprises for the
user. Should this be a "select GPIOLIB if OF" then?

Re: [PATCH net v2] net: smc91x: Fix build without gpiolib

[David Miller]

From: Tobias Klauser <tklauser@distanz.ch>
Date: Fri, 12 Dec 2014 17:45:29 +0100

> On 2014-12-12 at 17:30:20 +0100, David Miller <davem@davemloft.net> wrote:
>> In my opinion, if the code blocks enabling the configurations that
>> need this are enabled, so should GPIO be depended upon.
>> 
>> I think, at a minimum, when CONFIG_OF is enabled smsc91x should
>> require GPIO.
> 
> Agreed. This is the better solution, causing less surprises for the
> user. Should this be a "select GPIOLIB if OF" then?

If GPIO is a child node in the Kconfig hierarchy (ie. has no
dependencies of it's own), then yes.  Otherwise, a depends
will need to be used, because select does not recursively
trigger a select'd nodes dependencies.

Re: [PATCH net v2] net: smc91x: Fix build without gpiolib

[Tobias Klauser]

On 2014-12-12 at 17:58:38 +0100, David Miller <davem@davemloft.net> wrote:
> From: Tobias Klauser <tklauser@distanz.ch>
> Date: Fri, 12 Dec 2014 17:45:29 +0100
> 
> > On 2014-12-12 at 17:30:20 +0100, David Miller <davem@davemloft.net> wrote:
> >> In my opinion, if the code blocks enabling the configurations that
> >> need this are enabled, so should GPIO be depended upon.
> >> 
> >> I think, at a minimum, when CONFIG_OF is enabled smsc91x should
> >> require GPIO.
> > 
> > Agreed. This is the better solution, causing less surprises for the
> > user. Should this be a "select GPIOLIB if OF" then?
> 
> If GPIO is a child node in the Kconfig hierarchy (ie. has no
> dependencies of it's own), then yes.  Otherwise, a depends
> will need to be used, because select does not recursively
> trigger a select'd nodes dependencies.

Thank you for the explanation. Since GPIOLIB depends on
ARCH_WANT_OPTIONAL_GPIOLIB || ARCH_REQUIRE_GPIOLIB, I think it's
appropriate to let SMC91X depend on (!OF || GPIOLIB). I'll send an
updated patch.