| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f313c9b4-a70c-1b63-a7dd-9b807cfb8e76@microchip.com>
Date: Tue, 7 Dec 2021 12:57:52 +0000
From: <Ajay.Kathat@...rochip.com>
To: <Claudiu.Beznea@...rochip.com>, <davidm@...uge.net>,
<kvalo@...eaurora.org>
CC: <davem@...emloft.net>, <kuba@...nel.org>, <robh+dt@...nel.org>,
<adham.abozaeid@...rochip.com>, <linux-wireless@...r.kernel.org>,
<netdev@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] wilc1000: Add reset/enable GPIO support to SPI driver
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@...uge.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
Powered by blists - more mailing lists