-
Notifications
You must be signed in to change notification settings - Fork 7.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
driver: wifi: siwx91x: Add Access Point (AP) mode support to siwx91x wifi driver #85906
base: main
Are you sure you want to change the base?
driver: wifi: siwx91x: Add Access Point (AP) mode support to siwx91x wifi driver #85906
Conversation
5add5e0
to
d02f00a
Compare
drivers/wifi/siwx91x/siwx91x_wifi.c
Outdated
return SL_WIFI_BANDWIDTH_80MHz; | ||
default: | ||
LOG_ERR("Invalid bandwidth"); | ||
return -EAGAIN; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably -EINVAL
is better suited.
I am not sure LOG_ERR()
is useful (in fact, I don't know what policy we should have for the the logs).
drivers/wifi/siwx91x/siwx91x_wifi.h
Outdated
@@ -14,6 +14,8 @@ | |||
#include "sl_si91x_types.h" | |||
#include "sl_si91x_protocol_types.h" | |||
|
|||
#define SIWX91X_INTERFACE_MASK (0x03) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, SIWX91X_INTERFACE_MASK
is only used in siwx91x_wifi.c
. I am not sure it make sense to expose it in the header file.
d02f00a
to
d610343
Compare
drivers/wifi/siwx91x/siwx91x_wifi.c
Outdated
status = sl_wifi_get_mac_address(SL_WIFI_CLIENT_INTERFACE, &sidev->macaddr); | ||
interface = sl_wifi_get_default_interface(); | ||
status = sl_wifi_get_mac_address(FIELD_GET(SIWX91X_INTERFACE_MASK, interface), | ||
&sidev->macaddr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what case the original code didn't work?
drivers/wifi/siwx91x/siwx91x_wifi.c
Outdated
} | ||
} | ||
|
||
ret = sl_wifi_start_ap(SL_WIFI_AP_2_4GHZ_INTERFACE, &configuration); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the reader, it difficult to understand SL_WIFI_AP_2_4GHZ_INTERFACE == SL_WIFI_AP_INTERFACE | SL_WIFI_2_4GHZ_INTERFACE
. I suggest either:
- Replace references to
SL_WIFI_AP_INTERFACE
byFIELD_GET(SIWX91X_INTERFACE_MASK, SL_WIFI_AP_2_4GHZ_INTERFACE)
- Replace reference to
SL_WIFI_AP_2_4GHZ_INTERFACE
bySL_WIFI_AP_INTERFACE | SL_WIFI_2_4GHZ_INTERFACE
drivers/wifi/siwx91x/siwx91x_wifi.c
Outdated
} | ||
|
||
sl_si91x_set_join_configuration(SL_WIFI_AP_INTERFACE, | ||
SL_SI91X_JOIN_FEAT_MFP_CAPABLE_REQUIRED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think MFP is orthogonal to the authentication scheme. Probably this configuration should done outside of this switch case.
BTW, WPA2-PSK w/o MFP seems legit. Is it a limitation of Wiseconnect?
I also assume configuration.security = SL_WIFI_WPA3
automatically enables MFP? If ye, we could check params->mfp == WIFI_MFP_REQUIRED
.
drivers/wifi/siwx91x/siwx91x_wifi.c
Outdated
strncpy(status->ssid, wlan_info.ssid, WIFI_SSID_MAX_LEN); | ||
status->ssid_len = strlen(status->ssid); | ||
memcpy(status->bssid, wlan_info.mac_address, WIFI_MAC_ADDR_LEN); | ||
status->mfp = WIFI_MFP_REQUIRED; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can you be sure?
drivers/wifi/siwx91x/siwx91x_wifi.c
Outdated
memcpy(status->bssid, wlan_info.mac_address, WIFI_MAC_ADDR_LEN); | ||
status->mfp = WIFI_MFP_REQUIRED; | ||
|
||
if (FIELD_GET(SL_WIFI_2_4GHZ_INTERFACE, interface)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SL_WIFI_2_4GHZ_INTERFACE
is not a mask. I would either write:
FIELD_GET(SIWX91X_BAND_MASK, interface) == 1
interface & SL_WIFI_2_4GHZ_INTERFACE
drivers/wifi/siwx91x/siwx91x_wifi.c
Outdated
status->band = WIFI_FREQ_BAND_2_4_GHZ; | ||
} | ||
|
||
if (FIELD_GET(SL_WIFI_CLIENT_INTERFACE, interface)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SL_WIFI_CLIENT_INTERFACE
is not a mask. I suggest FIELD_GET(SIWX91X_INTERFACE_MASK, interface) == SL_WIFI_CLIENT_INTERFACE
.
drivers/wifi/siwx91x/siwx91x_wifi.c
Outdated
|
||
sl_wifi_get_listen_interval(SL_WIFI_CLIENT_INTERFACE, &listen_interval); | ||
status->beacon_interval = listen_interval.listen_interval; | ||
} else if (FIELD_GET(SL_WIFI_AP_INTERFACE, interface)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this condition be true? oper_mode
is always SL_SI91X_CLIENT_MODE
. So, I think interface
never set SL_WIFI_AP_INTERFACE
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have added the support to change oper_mode at runtime #85919
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe #85919 should be a part of this current PR.
drivers/wifi/siwx91x/siwx91x_wifi.c
Outdated
sl_wifi_get_listen_interval(SL_WIFI_CLIENT_INTERFACE, &listen_interval); | ||
status->beacon_interval = listen_interval.listen_interval; | ||
} else if (FIELD_GET(SL_WIFI_AP_INTERFACE, interface)) { | ||
sl_wifi_ap_configuration_t conf = { }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ap_cfg
or sl_ap_cfg
or siwx91x_ap_cfg
.
d610343
to
768343b
Compare
768343b
to
d5c66f1
Compare
acff2d2
to
d5d3c31
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 3 first commits depend on drivers: wifi: siwx91x: Add run time opermode
, so you have place it first.
drivers/wifi/siwx91x/siwx91x_wifi.c
Outdated
SL_SI91X_TCP_IP_FEAT_BYPASS), | ||
.custom_feature_bit_map = SL_SI91X_CUSTOM_FEAT_EXTENSION_VALID, | ||
.feature_bit_map = SL_SI91X_FEAT_SECURITY_OPEN, | ||
.coex_mode = SL_SI91X_WLAN_ONLY_MODE, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you deal with BLE?
I feel this function is in fact shared with Bluetooth and event with Flash driver. Probably it should be located in soc/silabs/silabs_siwx91x/siwg917/nwp_init.c
.
d5d3c31
to
e55ccfa
Compare
cdcfe3b
to
6d05903
Compare
|
||
#include "sl_wifi.h" | ||
|
||
static inline int get_wifi_device_configuration(sl_wifi_device_configuration_t *net_dev_cfg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this function is defined in the header file?
BTW, prefix the function name with the right prefix (siwg917_nwp_
I think).
drivers/wifi/siwx91x/siwx91x_wifi.c
Outdated
NET_BUF_POOL_FIXED_DEFINE(siwx91x_tx_pool, 1, _NET_ETH_MAX_FRAME_SIZE, 0, NULL); | ||
|
||
static inline int siwx91x_get_mode(uint8_t z_mode, sl_wifi_interface_t iface, uint16_t *sl_opermode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why inline
?
drivers/wifi/siwx91x/siwx91x_wifi.c
Outdated
{ | ||
sl_status_t status; | ||
|
||
status = sl_wifi_deinit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the impact of this on the other services: bluetooth and flash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If Bluetooth is enabled, it will also be deinitialized along with Wi-Fi. I did not find any logic in sl_si91x_host_power_cycle
affecting the flash, so I believe there is no impact on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume any Flash access will also abort because of the reset of a DMA somewhere.
I believe the user do not expect the bluetooth will be impacted. I assume it is very difficult (impossible?) remove this limitation?
At least, we have at least to add a FIXME
in the code and a LOG_WARN()
.
I think we could detect Bluetooth or Flash is in use (typically with an atomic counter, but maybe WiseConnect already provide an API for that). Then we could return an error if Bluetooth/Flash is in use.
Anyway, I believe it make more sense to relocate this function in the "shared" area of the 917 (nwp_init.c
). I believe it will be easier if it take the new oper_mode
in parameter rather than the whole sl_wifi_device_configuration_t
.
drivers/wifi/siwx91x/siwx91x_wifi.c
Outdated
case WIFI_STA_MODE: | ||
if (FIELD_GET(SIWX91X_INTERFACE_MASK, iface) == SL_WIFI_CLIENT_INTERFACE) { | ||
return -EALREADY; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this function is bit misleading. Reader does not expect get_mode()
does such test. Probably you want one function that return SL_SI91X_CLIENT_MODE
/ SL_SI91X_ACCESS_POINT_MODE
from z_mode and another that return SL_SI91X_CLIENT_MODE
/ SL_SI91X_ACCESS_POINT_MODE
from the sl_wifi_interface_t
. Then you can check if nwp_reboot()
is required or not.
6d05903
to
7e5681d
Compare
#include "sl_wifi_callback_framework.h" | ||
#ifdef CONFIG_BT_SILABS_SIWX91X | ||
#include "rsi_ble_common_config.h" | ||
#endif | ||
|
||
static int siwg917_nwp_init(void) | ||
int siwg917_nwp_get_configuration(sl_wifi_device_configuration_t *net_dev_cfg) | ||
{ | ||
sl_wifi_device_configuration_t network_config = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you find more consistent names, rather than net_dev_cfg
and network_config
(and cfg
).
drivers/wifi/siwx91x/siwx91x_wifi.c
Outdated
{ | ||
sl_status_t status; | ||
|
||
status = sl_wifi_deinit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume any Flash access will also abort because of the reset of a DMA somewhere.
I believe the user do not expect the bluetooth will be impacted. I assume it is very difficult (impossible?) remove this limitation?
At least, we have at least to add a FIXME
in the code and a LOG_WARN()
.
I think we could detect Bluetooth or Flash is in use (typically with an atomic counter, but maybe WiseConnect already provide an API for that). Then we could return an error if Bluetooth/Flash is in use.
Anyway, I believe it make more sense to relocate this function in the "shared" area of the 917 (nwp_init.c
). I believe it will be easier if it take the new oper_mode
in parameter rather than the whole sl_wifi_device_configuration_t
.
drivers/wifi/siwx91x/siwx91x_wifi.c
Outdated
} | ||
|
||
if (siwx91x_get_mode(mode->mode) < 0) { | ||
return -EINVAL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If siwx91x_get_mode() < 0
, it means that we don't have to reboot, so we should return 0, no?
BTW, I believe the following code would be more explicit for the reader:
int current_mode = siwx91x_get_iface_mode(interface);
int requested_mode = siwx91x_convert_mode(mode->mode);
if (current_mode == requested_mode) {
return 0;
}
siwx91x_nwp_reboot(requested_mode);
b56cfee
to
edfb378
Compare
.ble_feature_bit_map = 0, | ||
.bt_feature_bit_map = 0, | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With these two instances of sl_wifi_device_configuration_t
, it is difficult to compare the two instance and to understand which options are required for AP. I would rather provide something like siwg91x_get_nwp_config(int wifi_oper_mode)
.
BTW, this code ignores BLE when AP is enabled. Is it expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this behavior is expected. When the device operates in AP mode, Bluetooth coexistence is not supported.
drivers/wifi/siwx91x/siwx91x_wifi.c
Outdated
return 0; | ||
} | ||
|
||
static int siwx91x_nwp_reboot(uint8_t req_mode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function impact the whole NWP. I believe it should be located in nwp.c
(probably you want to pass oper_mode
as argument).
drivers/wifi/siwx91x/siwx91x_wifi.c
Outdated
} | ||
|
||
sidev->state = WIFI_STATE_INACTIVE; | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The opposite of mode->oper == WIFI_MGMT_SET
is not clear (after checking the code, it means mode->oper == WIFI_MGMT_GET
). Can you write this condition in way it is easier to understand?
drivers/wifi/siwx91x/siwx91x_wifi.c
Outdated
switch (sl_iface) { | ||
case SL_WIFI_CLIENT_2_4GHZ_INTERFACE: | ||
return WIFI_STA_MODE; | ||
case SL_WIFI_AP_2_4GHZ_INTERFACE: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please avoid symbols SL_WIFI_(AP|CLIENT)_2_4GHZ_INTERFACE
. They are misleading. Here, the relevant data is probably: FIELD_GET(SIWX91X_INTERFACE_MASK, sl_iface)
drivers/wifi/siwx91x/siwx91x_wifi.c
Outdated
interface = sl_wifi_get_default_interface(); | ||
switch (req_mode) { | ||
case WIFI_STA_MODE: | ||
if (interface == (SL_WIFI_CLIENT_INTERFACE | SL_WIFI_2_4GHZ_INTERFACE)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What 2.4GHz is doing i the picture? You probably want to say: FIELD_GET(SIWX91X_INTERFACE_MASK, interface) == SL_WIFI_CLIENT_INTERFACE
.
drivers/wifi/siwx91x/siwx91x_wifi.c
Outdated
NET_BUF_POOL_FIXED_DEFINE(siwx91x_tx_pool, 1, _NET_ETH_MAX_FRAME_SIZE, 0, NULL); | ||
|
||
static inline int siwx91x_convert_mode(sl_wifi_interface_t sl_iface) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use consistent names in the code: sl_iface
vs interface
.
drivers/wifi/siwx91x/siwx91x_wifi.c
Outdated
if (mode->oper == WIFI_MGMT_SET) { | ||
ret = siwx91x_nwp_reboot(mode->mode); | ||
if (ret) { | ||
return -ETIMEDOUT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ret
can also be -EINVAL
. Does it make sense to return -ETIMEDOUT
in this case? Just return ret
.
drivers/wifi/siwx91x/siwx91x_wifi.c
Outdated
return 0; | ||
} | ||
|
||
static int siwx91x_nwp_reboot(uint8_t req_mode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of this function is misleading. It does not always reboot the NWP.
12b3355
to
171eaa2
Compare
@@ -53,7 +79,7 @@ static int siwg917_nwp_init(void) | |||
|
|||
#ifdef CONFIG_WIFI_SILABS_SIWX91X | |||
cfg->feature_bit_map |= SL_SI91X_FEAT_SECURITY_OPEN | SL_SI91X_FEAT_WPS_DISABLE, | |||
cfg->ext_custom_feature_bit_map |= SL_SI91X_EXT_FEAT_IEEE_80211W; | |||
cfg->ext_custom_feature_bit_map |= SL_SI91X_EXT_FEAT_IEEE_80211W; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation
@@ -31,12 +55,14 @@ static int siwg917_nwp_init(void) | |||
.config_feature_bit_map = SL_SI91X_ENABLE_ENHANCED_MAX_PSP, | |||
.custom_feature_bit_map = SL_SI91X_CUSTOM_FEAT_EXTENSION_VALID, | |||
.ext_custom_feature_bit_map = | |||
MEMORY_CONFIG | | |||
SL_SI91X_EXT_FEAT_XTAL_CLK | | |||
MEMORY_CONFIG | SL_SI91X_EXT_FEAT_XTAL_CLK | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change was not required.
.feature_bit_map = SL_SI91X_FEAT_SECURITY_OPEN, | ||
.oper_mode = SL_SI91X_ACCESS_POINT_MODE, | ||
.coex_mode = SL_SI91X_WLAN_ONLY_MODE, | ||
.tcp_ip_feature_bit_map = (SL_SI91X_TCP_IP_FEAT_DHCPV4_SERVER | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think DHCP server is provided by Zephyr.
It is currently not easy to spot the differences between default_nwp_config
and default_ap_configuration
. Can you use the same ordering than default_nwp_config
? Or better, use a common struct and fill the differences using code (as we do for coex_mode
and some other fields)?
* | ||
* @return 0 on success, -EINVAL if an invalid mode is provided. | ||
*/ | ||
int siwg91x_get_nwp_config(int wifi_oper_mode, sl_wifi_device_configuration_t *get_config); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to export this function?
break; | ||
default: | ||
return -EINVAL; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have the full picture, but maybe siwx91x_nwp_mode_switch()
could take a WIFI_AP_MODE
/ WIFI_STA_MODE
as parameter. So, we could keep the oper_mode
stuff internal to nwp.c
and we won't have to expose it to the wifi driver.
7c9fb33
to
2ec1384
Compare
|
||
sl_si91x_boot_configuration_t *cfg = &default_config.boot_config; | ||
|
||
#ifndef CONFIG_WIFI_SILABS_SIWX91X_AP_MODE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand right, the idea is disable BLE only if oper_mode == SL_SI91X_ACCESS_POINT_MODE
. If don't think we want to disable it as soon as the user enable support for AP.
wifi_oper_mode = SL_SI91X_ACCESS_POINT_MODE; | ||
#else | ||
wifi_oper_mode = SL_SI91X_CLIENT_MODE; | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
User has enabled support for AP. We don't have any hint if he wants AP or client. Since AP has several limitations, I suggest to use SL_SI91X_CLIENT_MODE
.
return 0; | ||
} else if (wifi_oper_mode == SL_SI91X_ACCESS_POINT_MODE) { | ||
cfg->oper_mode = SL_SI91X_ACCESS_POINT_MODE; | ||
cfg->coex_mode = SL_SI91X_WLAN_ONLY_MODE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about CONFIG_WIFI_SILABS_SIWX91X_NET_STACK_OFFLOAD
?
|
||
__ASSERT(get_config, "get_config cannot be NULL"); | ||
|
||
sl_si91x_boot_configuration_t *cfg = &default_config.boot_config; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try to be consistent in naming: cfg
vs default_config
vs get_config
.
2ec1384
to
b5c5046
Compare
07a7a4c
to
2c5ebc9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code has now a good shape. This is probably the last iteration.
/* FIXME: Calling sl_wifi_deinit() impacts Bluetooth if coexistence is enabled */ | ||
if (IS_ENABLED(CONFIG_BT_SILABS_SIWX91X) && oper_mode == WIFI_AP_MODE) { | ||
LOG_WRN("Bluetooth is not supported in AP mode"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it make more sense to make this test in siwg91x_get_nwp_config()
/ if (wifi_oper_mode == SL_SI91X_ACCESS_POINT_MODE)
.
drivers/wifi/siwx91x/siwx91x_wifi.c
Outdated
ret = siwx91x_nwp_mode_switch(mode->mode); | ||
if (ret) { | ||
return ret; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once siwx91x_nwp_mode_switch()
takes a Zephyr mode as argument, the idea was to simplify everything:
static int siwx91x_mode(const struct device *dev, struct wifi_mode_info *mode)
{
sl_wifi_interface_t interface = sl_wifi_get_default_interface();
struct siwx91x_dev *sidev = dev->data;
int cur_mode;
int ret = 0;
__ASSERT(mode, "mode cannot be NULL");
cur_mode = siwx91x_sl_to_z_mode(FIELD_GET(SIWX91X_INTERFACE_MASK, interface));
if (cur_mode < 0) {
return -EIO;
}
if (mode->oper == WIFI_MGMT_GET) {
mode->mode = cur_mode;
} else if (mode->oper == WIFI_MGMT_SET) {
if (cur_mode != mode->mode) {
ret = siwx91x_nwp_mode_switch(mode->mode);
if (ret < 0) {
return ret;
}
}
sidev->state = WIFI_STATE_INACTIVE;
}
return 0;
}
Then, siwx91x_nwp_mode_switch()
does not have to check the current status of the interface.
61862ae
to
71a08a7
Compare
@jhedberg Could you please review it? |
This commit enables the device to change its operating mode at runtime. Signed-off-by: Arunmani Alagarsamy <[email protected]>
71a08a7
to
730bbc5
Compare
drivers/wifi/siwx91x/siwx91x_wifi.c
Outdated
if (siwx91x_security(params->security) < 0) { | ||
return -EINVAL; | ||
} | ||
|
||
siwx91x_ap_cfg.security = siwx91x_security(params->security); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another minor thing (sorry, I should have pointed this out in the last review): it seems a bit inefficient to be calling siwx91x_security()
twice on the exact same input parameter. Why not just store the return value in a local int sec
variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the comment, we followed this approach to maintain consistency with the style used in the silabs DMA driver.
https://github.com/zephyrproject-rtos/zephyr/blob/658f9357b87ab3e02351a57183743ec70f92cdd7/drivers/dma/dma_silabs_siwx91x.c#L143
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consistency is important, but that doesn't mean that you should not question existing code. The code you pointed to looks also inefficient. Any comment from @smalae? (the author)
This commit introduces Access Point (AP) mode support It enables the following AP-related commands: - ap enable: Enable the AP mode. - ap disable: Disable the AP mode. Signed-off-by: Arunmani Alagarsamy <[email protected]>
This commit adds support the following wifi commands: - wifi ap disconnect: Disconnect a specific station from the AP. - wifi ap stations: List connected devices. Signed-off-by: Arunmani Alagarsamy <[email protected]>
This commit adds support for Wi-Fi status and statistics functionalities: - wifi status : Displays the current mode status. - wifi statistics : Provides details on the transmitted and received packet counts. Signed-off-by: Arunmani Alagarsamy <[email protected]>
730bbc5
to
26ebdd8
Compare
Basic functionalities are included; further enhancements will be added in future PR.