Re: [PATCH 1/7] ath6kl: Add wmi functions to add/delete wow patterns

[Raja Mani <rmani@qca.qualcomm.com>]

On Friday 28 October 2011 05:41 PM, Kalle Valo wrote:
> On 10/25/2011 01:37 PM, rmani@qca.qualcomm.com wrote:
>> From: Raja Mani<rmani@qca.qualcomm.com>
>>
>> Signed-off-by: Raja Mani<rmani@qca.qualcomm.com>
>
> Try to avoid empty commit logs. You could just say that these commands
> will be used by the following patch if nothing else.
>
>> +int ath6kl_wmi_add_wow_pattern_cmd(struct wmi *wmi,
>> +				   struct wmi_add_wow_pattern_cmd *add_wow_cmd,
>> +				   u8 *pattern, u8 *mask)
>> +{
>
> Don't use a struct as the parameter, instead add individual parameters.
> So something like this:
>
> int ath6kl_wmi_add_wow_pattern_cmd(struct wmi *wmi,
> 	u8 id, u8 size, 8 offset,				
> 	u8 *pattern, u8 *mask)
>
> This is easier for the caller and you get to handle the endian in the
> callee which simplifies the code.

Agree.. I'll take care in V2.
>
>> +	size = sizeof(*cmd) +
>> +		((2 * add_wow_cmd->filter_size) * sizeof(u8));
>
> This can fit into line. And sizeof(u8) really doesn't make any sense.
>
> And is this correct? The struct is defined like this:
>
> +struct wmi_add_wow_pattern_cmd {
> +	u8 filter_list_id;
> +	u8 filter_size;
> +	u8 filter_offset;
> +	u8 filter[1];
> +} __packed;
>
> So there's one extra byte for the filter and above you include also that
> byte. But if the sctruct is defined like this the extra byte is not
> included:
>
> +struct wmi_add_wow_pattern_cmd {
> +	u8 filter_list_id;
> +	u8 filter_size;
> +	u8 filter_offset;
> +	u8 filter[0];
> +} __packed;
>

Good catch.. I'll correct it.

>> +int ath6kl_wmi_del_wow_pattern_cmd(struct wmi *wmi,
>> +				   struct wmi_del_wow_pattern_cmd *del_wow_cmd)
>
> Same here as earlier, don't use the struct as a parameter.

Okay..

>
> Kalle