Re: [PATCH] wilc1000: Add reset/enable GPIO support to SPI driver

[<Ajay.Kathat@microchip.com>]

On 07/12/21 15:41, Claudiu Beznea - M18063 wrote:
> On 02.12.2021 05:55, David Mosberger-Tang wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> This patch is based on similar code from the linux4sam/linux-at91 GIT
>> repository.
>>
>> For the SDIO driver, the RESET/ENABLE pins of WILC1000 may be
>> controlled through the SDIO power sequence driver.  This commit adds
>> analogous support for the SPI driver.  Specifically, during bus
>> probing, the chip will be reset-cycled and during unloading, the chip
>> will be placed into reset and disabled (both to reduce power
>> consumption and to ensure the WiFi radio is off).
>>
>> Both RESET and ENABLE GPIOs are optional.  However, if the ENABLE GPIO
>> is specified, then the RESET GPIO also must be specified as otherwise
>> there is no way to ensure proper timing of the ENABLE/RESET sequence.
>>
>> Signed-off-by: David Mosberger-Tang <davidm@egauge.net>
>> ---
>>   .../net/wireless/microchip,wilc1000.yaml      | 11 +++
>>   .../net/wireless/microchip/wilc1000/Makefile  |  2 +-
>>   drivers/net/wireless/microchip/wilc1000/hif.h |  2 +
>>   .../net/wireless/microchip/wilc1000/netdev.h  | 10 +++
>>   .../net/wireless/microchip/wilc1000/power.c   | 73 +++++++++++++++++++
>>   drivers/net/wireless/microchip/wilc1000/spi.c | 15 +++-
>>   .../net/wireless/microchip/wilc1000/wlan.c    |  2 +-
>>   7 files changed, 110 insertions(+), 5 deletions(-)
>>   create mode 100644 drivers/net/wireless/microchip/wilc1000/power.c
>>
>> diff --git a/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
>> index 6c35682377e6..2ce316f5e353 100644
>> --- a/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
>> +++ b/Documentation/devicetree/bindings/net/wireless/microchip,wilc1000.yaml
>> @@ -32,6 +32,15 @@ properties:
>>     clock-names:
>>       const: rtc
>>
>> +  enable-gpios:
>> +    maxItems: 1
>> +    description: A GPIO line connected to the ENABLE line.  If this
>> +      is specified, then reset-gpios also must be specified.
>> +
>> +  reset-gpios:
>> +    maxItems: 1
>> +    description: A GPIO line connected to the RESET line.
>> +
> Bindings should go through a different patch.
>
>>   required:
>>     - compatible
>>     - interrupts
>> @@ -51,6 +60,8 @@ examples:
>>           interrupts = <27 0>;
>>           clocks = <&pck1>;
>>           clock-names = "rtc";
>> +        enable-gpios = <&pioA 5 0>;
>> +        reset-gpios = <&pioA 6 0>;
>>         };
>>       };
>>
>> diff --git a/drivers/net/wireless/microchip/wilc1000/Makefile b/drivers/net/wireless/microchip/wilc1000/Makefile
>> index c3c9e34c1eaa..baf9f021a1d6 100644
>> --- a/drivers/net/wireless/microchip/wilc1000/Makefile
>> +++ b/drivers/net/wireless/microchip/wilc1000/Makefile
>> @@ -2,7 +2,7 @@
>>   obj-$(CONFIG_WILC1000) += wilc1000.o
>>
>>   wilc1000-objs := cfg80211.o netdev.o mon.o \
>> -                       hif.o wlan_cfg.o wlan.o
>> +                       hif.o wlan_cfg.o wlan.o power.o
>>
>>   obj-$(CONFIG_WILC1000_SDIO) += wilc1000-sdio.o
>>   wilc1000-sdio-objs += sdio.o
>> diff --git a/drivers/net/wireless/microchip/wilc1000/hif.h b/drivers/net/wireless/microchip/wilc1000/hif.h
>> index cccd54ed0518..a57095d8088e 100644
>> --- a/drivers/net/wireless/microchip/wilc1000/hif.h
>> +++ b/drivers/net/wireless/microchip/wilc1000/hif.h
>> @@ -213,4 +213,6 @@ void wilc_network_info_received(struct wilc *wilc, u8 *buffer, u32 length);
>>   void wilc_gnrl_async_info_received(struct wilc *wilc, u8 *buffer, u32 length);
>>   void *wilc_parse_join_bss_param(struct cfg80211_bss *bss,
>>                                  struct cfg80211_crypto_settings *crypto);
>> +int wilc_of_parse_power_pins(struct wilc *wilc);
>> +void wilc_wlan_power(struct wilc *wilc, bool on);
>>   #endif
>> diff --git a/drivers/net/wireless/microchip/wilc1000/netdev.h b/drivers/net/wireless/microchip/wilc1000/netdev.h
>> index b9a88b3e322f..b95a247322a6 100644
>> --- a/drivers/net/wireless/microchip/wilc1000/netdev.h
>> +++ b/drivers/net/wireless/microchip/wilc1000/netdev.h
>> @@ -197,6 +197,15 @@ struct wilc_vif {
>>          struct cfg80211_bss *bss;
>>   };
>>
>> +struct wilc_power_gpios {
>> +       int reset;
>> +       int chip_en;
>> +};
>> +
>> +struct wilc_power {
>> +       struct wilc_power_gpios gpios;
>> +};
>> +
>>   struct wilc_tx_queue_status {
>>          u8 buffer[AC_BUFFER_SIZE];
>>          u16 end_index;
>> @@ -265,6 +274,7 @@ struct wilc {
>>          bool suspend_event;
>>
>>          struct workqueue_struct *hif_workqueue;
>> +       struct wilc_power power;
> For SDIO power should be controlled though MMC pwrseq driver. Thus I would
> keep this code for SPI only and move this member to struct wilc_spi.
>
>>          struct wilc_cfg cfg;
>>          void *bus_data;
>>          struct net_device *monitor_dev;
>> diff --git a/drivers/net/wireless/microchip/wilc1000/power.c b/drivers/net/wireless/microchip/wilc1000/power.c
>> new file mode 100644
>> index 000000000000..d26a39b7698d
>> --- /dev/null
>> +++ b/drivers/net/wireless/microchip/wilc1000/power.c
>> @@ -0,0 +1,73 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2012 - 2018 Microchip Technology Inc., and its subsidiaries.
>> + * All rights reserved.
> This is not in the GIT repo. It should have been:
> "Copyright (c) 2021 Microchip Technology Inc. and its subsidiaries"
> 		^
> 		current year
>
>> + */
>> +#include <linux/delay.h>
>> +#include <linux/gpio.h>
>> +#include <linux/of.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/version.h>
>> +
>> +#include "netdev.h"
>> +
>> +/**
>> + * wilc_of_parse_power_pins() - parse power sequence pins
>> + *
>> + * @wilc:      wilc data structure
>> + *
>> + * Returns:     0 on success, negative error number on failures.
>> + */
>> +int wilc_of_parse_power_pins(struct wilc *wilc)
>> +{
>> +       struct device_node *of = wilc->dev->of_node;
>> +       struct wilc_power *power = &wilc->power;
>> +       int ret;
>> +
>> +       power->gpios.reset = of_get_named_gpio_flags(of, "reset-gpios", 0,
>> +                                                    NULL);
>> +       power->gpios.chip_en = of_get_named_gpio_flags(of, "chip_en-gpios", 0,
>> +                                                      NULL);
> Consider using gpio descriptors and devm_gpiod_get().
>
>> +       if (!gpio_is_valid(power->gpios.reset))
>> +               return 0;       /* assume SDIO power sequence driver is used */
> In case of SDIO we should rely on mmc pwrseq all the time. It keeps things
> cleaner. I would keep the code in this file only for SPI.
>
>> +
>> +       if (gpio_is_valid(power->gpios.chip_en)) {
>> +               ret = devm_gpio_request(wilc->dev, power->gpios.chip_en,
>> +                                       "CHIP_EN");
>> +               if (ret)
>> +                       return ret;
>> +       }
>> +       return devm_gpio_request(wilc->dev, power->gpios.reset, "RESET");
>> +}
>> +EXPORT_SYMBOL_GPL(wilc_of_parse_power_pins);
>> +
>> +/**
>> + * wilc_wlan_power() - handle power on/off commands
>> + *
>> + * @wilc:      wilc data structure
>> + * @on:                requested power status
>> + *
>> + * Returns:    none
>> + */
>> +void wilc_wlan_power(struct wilc *wilc, bool on)
>> +{
>> +       if (!gpio_is_valid(wilc->power.gpios.reset)) {
>> +               /* In case SDIO power sequence driver is used to power this
>> +                * device then the powering sequence is handled by the bus
>> +                * via pm_runtime_* functions. */
> I see this function is called anyway only in SPI context, so, don't think
> this handling for SDIO is necessary. Maybe these is something I miss with
> regards to it. Ajay, please correct me if I'm wrong.


Yes, 'wilc_wlan_power' function is called for SPI bus. The comments 
(/**/) specific to SDIO can be removed, though the GPIO's validity check 
is required to verify GPIO entries.


Regards,

Ajay