From:   Kalle Valo <kvalo@kernel.org>
To:     Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Cc:     linux-wireless@vger.kernel.org, wcn36xx@lists.infradead.org,
        linux-arm-msm@vger.kernel.org, loic.poulain@linaro.org,
        benl@squareup.com
Subject: Re: [PATCH v3 3/3] wcn36xx: Implement downstream compliant beacon filtering
References: <20211214134630.2214840-1-bryan.odonoghue@linaro.org>
        <20211214134630.2214840-4-bryan.odonoghue@linaro.org>
Date:   Thu, 16 Dec 2021 12:31:36 +0200
In-Reply-To: <20211214134630.2214840-4-bryan.odonoghue@linaro.org> (Bryan
        O'Donoghue's message of "Tue, 14 Dec 2021 13:46:30 +0000")
Message-ID: <87ilvodenr.fsf@codeaurora.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes:

> Downstream facilitates the direct programming of beacon filter tables via
> SMD commands.

I don't think the term downstream is clear for everyone, I guess with
that you mean the prima driver? I would prefer to use "prima driver" in
commit logs, I can change it and no need to resend because of this.

> --- a/drivers/net/wireless/ath/wcn36xx/hal.h
> +++ b/drivers/net/wireless/ath/wcn36xx/hal.h
> @@ -3468,6 +3468,23 @@ struct beacon_filter_ie {
>  	u8 ref;
>  } __packed;
>  
> +/* Downstream values for the bitmask field */
> +#define WCN36XX_FILTER_CAPABILITY_MASK		0x73cf
> +#define WCN36XX_FILTER_IE_DS_CHANNEL_MASK	0x00
> +#define WCN36XX_FILTER_IE_ERP_FILTER_MASK	0xF8
> +#define WCN36XX_FILTER_IE_EDCA_FILTER_MASK	0xF0
> +#define WCN36XX_FILTER_IE_QOS_FILTER_MASK	0xF0
> +#define WCN36XX_FILTER_IE_CHANNEL_SWITCH_MASK	0x00
> +#define WCN36XX_FILTER_IE_HT_BYTE0_FILTER_MASK	0x00
> +#define WCN36XX_FILTER_IE_HT_BYTE1_FILTER_MASK	0xF8
> +#define WCN36XX_FILTER_IE_HT_BYTE2_FILTER_MASK	0xEB
> +#define WCN36XX_FILTER_IE_HT_BYTE5_FILTER_MASK	0xFD
> +#define WCN36XX_FILTER_IE_PWR_CONSTRAINT_MASK	0x00
> +#define WCN36XX_FILTER_IE_OPMODE_NOTIF_MASK	0x00
> +#define WCN36XX_FILTER_IE_VHTOP_CHWIDTH_MASK	0xFC
> +#define WCN36XX_FILTER_IE_RSN_MASK		0x00
> +#define WCN36XX_FILTER_IE_VENDOR_MASK		0x00

TBH I don't see much value in the comment, there's only one set of
values, so I would like to remove it.

> --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> @@ -3193,6 +3193,92 @@ int wcn36xx_smd_host_resume(struct wcn36xx *wcn)
>  	return ret;
>  }
>  
> +#define BEACON_FILTER(eid, presence, offs, val, mask, ref_val) \
> +	{					\
> +		.element_id = eid,		\
> +		.check_ie_presence = presence,	\
> +		.offset = offs,			\
> +		.value = val,			\
> +		.bitmask = mask,		\
> +		.ref = ref_val,			\
> +	}
> +
> +/* CORE/MAC/src/pe/lim/limSendMessages.c::beaconFilterTable[] */
> +static struct beacon_filter_ie bcn_filter_ies[] = {

I guess the comment refers to the prima driver, we shouldn't have that
in upstream. I can remove it.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches