On 7/12/2019 1:12 PM, Johannes Berg wrote:
> External E-Mail
>
>
>> +struct wilc_set_multicast {
>> + u32 enabled;
>> + u32 cnt;
>> + u8 *mc_list;
>> +};
>> +
>> +struct wilc_del_all_sta {
>> + u8 assoc_sta;
>> + u8 mac[WILC_MAX_NUM_STA][ETH_ALEN];
>> +};
>> +
>> +struct wilc_op_mode {
>> + __le32 mode;
>> +};
>> +
>> +struct wilc_reg_frame {
>> + bool reg;
>> + u8 reg_id;
>> + __le16 frame_type;
>> +} __packed;
>
> 'bool' is a pretty bad idea, there's no storage guarantee for it. Use u8
> instead, especially in a firmware struct.
>
> But overall, if I remember correctly, this is a massive improvement,
> last time I looked I think you basically had something like
>
> char msg[10];
> int i = 0;
> msg[i++] = reg;
> msg[i++] = reg_id;
> msg[i++] = frame_type >> 8;
> msg[i++] = (u8)frame_type;
>
> so obviously this is *much* better.
>
> I still think you'd benefit from putting the firmware API structs into a
> separate include file so you can differentiate them, but YMMV.
>
>> +int wilc_scan(struct wilc_vif *vif, u8 scan_source, u8 scan_type,
>> + u8 *ch_freq_list, u8 ch_list_len,
>> + void (*scan_result_fn)(enum scan_event,
>> + struct wilc_rcvd_net_info *, void *),
>> + void *user_arg, struct cfg80211_scan_request *request)
>> +{
>> + int result = 0;
>> + struct wid wid_list[5];
>
>> + wid_list[index].id = WID_INFO_ELEMENT_PROBE;
>> + wid_list[index].type = WID_BIN_DATA;
>> + wid_list[index].val = (s8 *)request->ie;
>> + wid_list[index].size = request->ie_len;
>> + index++;
>> +
>> + wid_list[index].id = WID_SCAN_TYPE;
>> + wid_list[index].type = WID_CHAR;
>> + wid_list[index].size = sizeof(char);
>> + wid_list[index].val = (s8 *)&scan_type;
>> + index++;
>
>
> I still find this whole wid_list stuff to be a bit confusing, especially
> since it looks like a *firmware* thing but then you have the *host
> pointer* inside the value ...
>
> There must be a translation layer somewhere, but I can't help but wonder
> if that's really worth the complexity, vs. just building the right thing
> directly here (with some helpers perhaps).
>
The translation to firmware buffer happens in wilc_wlan_cfg_set() and
wilc_wlan_cfg_commit() adds a single *wilc_cfg_cmd_hdr* header before
sending to firmware.
There are two ways to send the wid's from host to firmware.
1/ Single Wid -> In this case, single wid is sent by adding
*wilc_cfg_cmd_hdr* in single command buffer to firmware.
i.e <wilc_cfg_cmd_hdr><wid1>
2/ Mutliple Wid's -> In this case, multiple wid's are clubbled together
and sent in single command buffer.
e.g. <wilc_cfg_cmd_hdr><wid1><wid2><wid3>
As the firmware is design to receive configuration under different
WID's, so it is required from the host side to club these parameters
whenever data is related.
Currently, wilc_send_config_pkt() is provided as helper API to construct
buffer based on passed *wids* and *counts*. This will avoid adding
similar logic in multiple places.
Regards,
Ajay
