Re: [PATCH 1/2] ath6kl_sdio: Add reset gpio module parameter for CHIP_PWD_L pin

[kbuild test robot]

Hi Steve,

[auto build test WARNING on wireless-drivers/master]
[also build test WARNING on v4.4-rc1 next-20151117]

url:
https://github.com/0day-ci/linux/commits/Steve-deRosier/ath6kl_sdio-add-control-of-CHIP_PWD_L-via-GPIO/20151117-133751
base:   
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers.git 
master
config: blackfin-allmodconfig (attached as .config)
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=blackfin 

All warnings (new ones prefixed by >>):

>> WARNING: drivers/net/wireless/ath/ath6kl/ath6kl_sdio.o(.init.text+0xc2): 
>> Section mismatch in reference from the function _init_module() to the 
>> function .exit.text:_ath6kl_sdio_release_gpio()
   The function __init _init_module() references
   a function __exit _ath6kl_sdio_release_gpio().
   This is often seen when error handling in the init function
   uses functionality in the exit path.
   The fix is often to remove the __exit annotation of
   _ath6kl_sdio_release_gpio() so it may be used outside an exit section.

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: [PATCH 1/2] ath6kl_sdio: Add reset gpio module parameter for CHIP_PWD_L pin

[kbuild test robot]

Hi Steve,

[auto build test ERROR on wireless-drivers/master]
[also build test ERROR on v4.4-rc1 next-20151117]

url:
https://github.com/0day-ci/linux/commits/Steve-deRosier/ath6kl_sdio-add-control-of-CHIP_PWD_L-via-GPIO/20151117-133751
base:   
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers.git 
master
config: microblaze-allmodconfig (attached as .config)
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=microblaze 

All errors (new ones prefixed by >>):

>> ERROR: "isa_io_base" undefined!
>> ERROR: "isa_io_base" undefined!
>> ERROR: "isa_io_base" undefined!
>> ERROR: "isa_io_base" undefined!
>> ERROR: "isa_io_base" undefined!
>> ERROR: "isa_io_base" undefined!
>> ERROR: "isa_io_base" undefined!
>> ERROR: "isa_io_base" undefined!
>> ERROR: "isa_io_base" undefined!
>> ERROR: "isa_io_base" undefined!
>> ERROR: "isa_io_base" undefined!
>> ERROR: "isa_io_base" undefined!
>> ERROR: "isa_io_base" undefined!
>> ERROR: "isa_io_base" undefined!
>> ERROR: "isa_io_base" undefined!
>> ERROR: "isa_io_base" undefined!
>> ERROR: "isa_io_base" undefined!
>> ERROR: "isa_io_base" undefined!
>> ERROR: "isa_io_base" undefined!
>> ERROR: "isa_io_base" undefined!

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: [PATCH 1/2] ath6kl_sdio: Add reset gpio module parameter for CHIP_PWD_L pin

[Steve deRosier]

Hi Julian,

Thanks for looking at this.

In short - I agree with your review and will do most of them. As a
well as a few minor changes suggested by the kbuild test robot. Expect
a new version shortly.

On Mon, Nov 16, 2015 at 10:25 PM, Julian Calaby  wrote:
>>
>>  static void __exit ath6kl_sdio_exit(void)
>>  {
>> sdio_unregister_driver(_sdio_driver);
>> +
>> +#ifdef CONFIG_GPIOLIB
>> +   /* Delay to avoid pulling the plug on the chip when an irq is pending
>> +* and then getting a spurious message:
>> +* "ath6kl: failed to get pending recv messages: -125"
>> +*/
>> +   msleep(300);
>
> Is there some actual synchronisation you can do here against the IRQ?
> 300msec isn't a long time to wait, but it seems wrong here.
>

Considering this is only called on exit, I wasn't too worried about
this, but then again, on reboot as well as a few types of
reconfiguring the interface speed is an important consideration for
our system.

I'm open to suggestions.  I had looked at this and didn't see an
obvious way to try to sync with the IRQ as it goes through several
subsystems.

But looking at it again in a different light...

125 is ECANCELED.  Which is exactly right and appropriate, and
ath6kl_htc_rxmsg_pending_handler() where the message comes from does a
proper cleanup, or so it looks like. I wonder if maybe we can consider
ECANCELED not an error that needs an error message as it might be a
reasonable case?

So instead of synchronization, just consider that particular status an
expected and properly handled exception condition and not print an
error?

Thoughts?

- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ath6kl_sdio: Add reset gpio module parameter for CHIP_PWD_L pin

[Julian Calaby]

Hi Steve,

On Wed, Nov 18, 2015 at 4:42 AM, Steve deRosier  wrote:
> Hi Julian,
>
> Thanks for looking at this.
>
> In short - I agree with your review and will do most of them. As a
> well as a few minor changes suggested by the kbuild test robot. Expect
> a new version shortly.

I'm only really reviewing for style, you might want to wait for Kalle
or someone else to do a technical review.

> On Mon, Nov 16, 2015 at 10:25 PM, Julian Calaby  
> wrote:
>>>
>>>  static void __exit ath6kl_sdio_exit(void)
>>>  {
>>> sdio_unregister_driver(_sdio_driver);
>>> +
>>> +#ifdef CONFIG_GPIOLIB
>>> +   /* Delay to avoid pulling the plug on the chip when an irq is 
>>> pending
>>> +* and then getting a spurious message:
>>> +* "ath6kl: failed to get pending recv messages: -125"
>>> +*/
>>> +   msleep(300);
>>
>> Is there some actual synchronisation you can do here against the IRQ?
>> 300msec isn't a long time to wait, but it seems wrong here.
>>
>
> Considering this is only called on exit, I wasn't too worried about
> this, but then again, on reboot as well as a few types of
> reconfiguring the interface speed is an important consideration for
> our system.
>
> I'm open to suggestions.  I had looked at this and didn't see an
> obvious way to try to sync with the IRQ as it goes through several
> subsystems.

Maybe having this in ath6kl is having it in the wrong layer?

I'm guessing this is on some board that already has a DTS. Do the mmc
pwrseq drivers do what you're after?
https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/mmc-pwrseq-emmc.txt
or 
https://www.kernel.org/doc/Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.txt

> But looking at it again in a different light...
>
> 125 is ECANCELED.  Which is exactly right and appropriate, and
> ath6kl_htc_rxmsg_pending_handler() where the message comes from does a
> proper cleanup, or so it looks like. I wonder if maybe we can consider
> ECANCELED not an error that needs an error message as it might be a
> reasonable case?
>
> So instead of synchronization, just consider that particular status an
> expected and properly handled exception condition and not print an
> error?

Printing it as a debug instead of an error is probably a better option
here if you want to "silence" that error.

Thanks,

-- 
Julian Calaby

Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] ath6kl_sdio: Add reset gpio module parameter for CHIP_PWD_L pin

[Steve deRosier]

Many ath6k chips have a reset pin, usually labeled CHIP_PWD_L. This pin can
be pulled low by the host processor to hold the wifi chip in reset. By
holding the chip in reset, the lowest power consumption available can be
achieved.

This adds a module parameter so the ath6kl_sdio driver can control the
CHIP_PWD_L pin if the implementer so desires. This code is only available
if GPIOLIB is configured.

Signed-off-by: Steve deRosier 
---
 drivers/net/wireless/ath/ath6kl/sdio.c | 80 +-
 1 file changed, 79 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath6kl/sdio.c 
b/drivers/net/wireless/ath/ath6kl/sdio.c
index eab0ab9..7a732f3 100644
--- a/drivers/net/wireless/ath/ath6kl/sdio.c
+++ b/drivers/net/wireless/ath/ath6kl/sdio.c
@@ -67,8 +67,18 @@ struct ath6kl_sdio {
 
/* protects access to wr_asyncq */
spinlock_t wr_async_lock;
+
 };
 
+#ifdef CONFIG_GPIOLIB
+#include 
+#define NO_GPIO0x
+
+static unsigned int reset_pwd_gpio = NO_GPIO;
+module_param(reset_pwd_gpio, uint, 0644);
+MODULE_PARM_DESC(reset_pwd_gpio, "WIFI CHIP_PWD reset pin GPIO");
+#endif
+
 #define CMD53_ARG_READ  0
 #define CMD53_ARG_WRITE 1
 #define CMD53_ARG_BLOCK_BASIS   1
@@ -1414,20 +1424,88 @@ static struct sdio_driver ath6kl_sdio_driver = {
.drv.pm = ATH6KL_SDIO_PM_OPS,
 };
 
+static int __init ath6kl_sdio_init_gpio(void)
+{
+   int ret = 0;
+
+#ifdef CONFIG_GPIOLIB
+   if (!gpio_is_valid(reset_pwd_gpio))
+   return 0;
+
+   /* Request the reset GPIO, and assert it to make sure we get a 100%
+* clean boot in-case we had a floating input or other issue.
+*/
+   ret = gpio_request_one(reset_pwd_gpio,
+  GPIOF_OUT_INIT_LOW | GPIOF_EXPORT_DIR_FIXED,
+  "WIFI_RESET");
+   if (ret) {
+   ath6kl_err("Unable to get WIFI power gpio: %d\n", ret);
+   return ret;
+   }
+
+   ath6kl_dbg(ATH6KL_DBG_SUSPEND, "Setup wifi gpio #%d\n", reset_pwd_gpio);
+   usleep_range(20, 50); /* Pin must be asserted at least 5 usec */
+   gpio_set_value(reset_pwd_gpio, 1); /* De-assert the pin for operation */
+
+   /* Delay to avoid the mmc driver calling the probe on the prior notice
+* of the chip, which we just killed. If this is missing, it results in
+* a spurious warning:
+* "ath6kl_sdio: probe of mmc0:0001:1 failed with error -110"
+*/
+   msleep(150); /* Time chosen experimentally, with padding */
+#endif
+
+   return ret;
+}
+
+static void __exit ath6kl_sdio_release_gpio(void)
+{
+#ifdef CONFIG_GPIOLIB
+   if (gpio_is_valid(reset_pwd_gpio)) {
+   /* Be sure we leave the chip in reset when we unload and also
+* release the GPIO
+*/
+   gpio_set_value(reset_pwd_gpio, 0);
+   gpio_free(reset_pwd_gpio);
+   }
+#endif
+}
+
 static int __init ath6kl_sdio_init(void)
 {
int ret;
 
-   ret = sdio_register_driver(_sdio_driver);
+   ret = ath6kl_sdio_init_gpio();
if (ret)
+   goto err_gpio;
+
+   ret = sdio_register_driver(_sdio_driver);
+   if (ret) {
ath6kl_err("sdio driver registration failed: %d\n", ret);
+   goto err_register;
+   }
+
+   return ret;
+
+err_register:
+   ath6kl_sdio_release_gpio();
 
+err_gpio:
return ret;
 }
 
 static void __exit ath6kl_sdio_exit(void)
 {
sdio_unregister_driver(_sdio_driver);
+
+#ifdef CONFIG_GPIOLIB
+   /* Delay to avoid pulling the plug on the chip when an irq is pending
+* and then getting a spurious message:
+* "ath6kl: failed to get pending recv messages: -125"
+*/
+   msleep(300);
+   ath6kl_sdio_release_gpio();
+#endif
 }
 
 module_init(ath6kl_sdio_init);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ath6kl_sdio: Add reset gpio module parameter for CHIP_PWD_L pin

[Julian Calaby]

Hi Steve,

On Tue, Nov 17, 2015 at 4:32 PM, Steve deRosier  wrote:
> Many ath6k chips have a reset pin, usually labeled CHIP_PWD_L. This pin can
> be pulled low by the host processor to hold the wifi chip in reset. By
> holding the chip in reset, the lowest power consumption available can be
> achieved.
>
> This adds a module parameter so the ath6kl_sdio driver can control the
> CHIP_PWD_L pin if the implementer so desires. This code is only available
> if GPIOLIB is configured.

linux/gpio.h is built so that the exported functions are no-ops when
GPIOLIB isn't defined. Providing you include linux/gpio.h
unconditionally, all of your #ifdefs except the one around the module
parameter can be removed and the code will be optimised away by the
compiler.

> Signed-off-by: Steve deRosier 
> ---
>  drivers/net/wireless/ath/ath6kl/sdio.c | 80 
> +-
>  1 file changed, 79 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath6kl/sdio.c 
> b/drivers/net/wireless/ath/ath6kl/sdio.c
> index eab0ab9..7a732f3 100644
> --- a/drivers/net/wireless/ath/ath6kl/sdio.c
> +++ b/drivers/net/wireless/ath/ath6kl/sdio.c
> @@ -67,8 +67,18 @@ struct ath6kl_sdio {
>
> /* protects access to wr_asyncq */
> spinlock_t wr_async_lock;
> +
>  };
>
> +#ifdef CONFIG_GPIOLIB
> +#include 
> +#define NO_GPIO0x

ARCH_NR_GPIOS could be used here instead of defining NO_GPIO.

> +
> +static unsigned int reset_pwd_gpio = NO_GPIO;
> +module_param(reset_pwd_gpio, uint, 0644);
> +MODULE_PARM_DESC(reset_pwd_gpio, "WIFI CHIP_PWD reset pin GPIO");
> +#endif
> +
>  #define CMD53_ARG_READ  0
>  #define CMD53_ARG_WRITE 1
>  #define CMD53_ARG_BLOCK_BASIS   1
> @@ -1414,20 +1424,88 @@ static struct sdio_driver ath6kl_sdio_driver = {
> .drv.pm = ATH6KL_SDIO_PM_OPS,
>  };
>
> +static int __init ath6kl_sdio_init_gpio(void)
> +{
> +   int ret = 0;
> +
> +#ifdef CONFIG_GPIOLIB
> +   if (!gpio_is_valid(reset_pwd_gpio))
> +   return 0;
> +
> +   /* Request the reset GPIO, and assert it to make sure we get a 100%
> +* clean boot in-case we had a floating input or other issue.
> +*/
> +   ret = gpio_request_one(reset_pwd_gpio,
> +  GPIOF_OUT_INIT_LOW | GPIOF_EXPORT_DIR_FIXED,
> +  "WIFI_RESET");
> +   if (ret) {
> +   ath6kl_err("Unable to get WIFI power gpio: %d\n", ret);
> +   return ret;
> +   }
> +
> +   ath6kl_dbg(ATH6KL_DBG_SUSPEND, "Setup wifi gpio #%d\n", 
> reset_pwd_gpio);
> +   usleep_range(20, 50); /* Pin must be asserted at least 5 usec */
> +   gpio_set_value(reset_pwd_gpio, 1); /* De-assert the pin for operation 
> */
> +
> +   /* Delay to avoid the mmc driver calling the probe on the prior notice
> +* of the chip, which we just killed. If this is missing, it results 
> in
> +* a spurious warning:
> +* "ath6kl_sdio: probe of mmc0:0001:1 failed with error -110"
> +*/
> +   msleep(150); /* Time chosen experimentally, with padding */

Should this be in a #define?

> +#endif
> +
> +   return ret;

ret is always zero here. You should just return 0 here.

> +}
> +
> +static void __exit ath6kl_sdio_release_gpio(void)
> +{
> +#ifdef CONFIG_GPIOLIB
> +   if (gpio_is_valid(reset_pwd_gpio)) {
> +   /* Be sure we leave the chip in reset when we unload and also
> +* release the GPIO
> +*/
> +   gpio_set_value(reset_pwd_gpio, 0);
> +   gpio_free(reset_pwd_gpio);
> +   }
> +#endif
> +}
> +
>  static int __init ath6kl_sdio_init(void)
>  {
> int ret;
>
> -   ret = sdio_register_driver(_sdio_driver);
> +   ret = ath6kl_sdio_init_gpio();
> if (ret)
> +   goto err_gpio;
> +
> +   ret = sdio_register_driver(_sdio_driver);
> +   if (ret) {
> ath6kl_err("sdio driver registration failed: %d\n", ret);
> +   goto err_register;
> +   }
> +
> +   return ret;
> +
> +err_register:
> +   ath6kl_sdio_release_gpio();
>
> +err_gpio:
> return ret;
>  }
>
>  static void __exit ath6kl_sdio_exit(void)
>  {
> sdio_unregister_driver(_sdio_driver);
> +
> +#ifdef CONFIG_GPIOLIB
> +   /* Delay to avoid pulling the plug on the chip when an irq is pending
> +* and then getting a spurious message:
> +* "ath6kl: failed to get pending recv messages: -125"
> +*/
> +   msleep(300);

Is there some actual synchronisation you can do here against the IRQ?
300msec isn't a long time to wait, but it seems wrong here.

> +   ath6kl_sdio_release_gpio();
> +#endif
>  }
>
>  module_init(ath6kl_sdio_init);

Thanks,

-- 
Julian Calaby

Email: julian.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
--
To unsubscribe from this list: send the line 

Re: [PATCH 1/2] ath6kl_sdio: Add reset gpio module parameter for CHIP_PWD_L pin

[kbuild test robot]

Hi Steve,

[auto build test WARNING on wireless-drivers/master]
[also build test WARNING on v4.4-rc1 next-20151117]

url:
https://github.com/0day-ci/linux/commits/Steve-deRosier/ath6kl_sdio-add-control-of-CHIP_PWD_L-via-GPIO/20151117-133751
base:   
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers.git 
master
config: i386-allyesconfig (attached as .config)
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

All warnings (new ones prefixed by >>):

>> WARNING: drivers/net/wireless/built-in.o(.init.text+0x22f3): Section 
>> mismatch in reference from the function ath6kl_sdio_init() to the function 
>> .exit.text:ath6kl_sdio_release_gpio()
   The function __init ath6kl_sdio_init() references
   a function __exit ath6kl_sdio_release_gpio().
   This is often seen when error handling in the init function
   uses functionality in the exit path.
   The fix is often to remove the __exit annotation of
   ath6kl_sdio_release_gpio() so it may be used outside an exit section.

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data