DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 71CBE2075C
Message-ID: <1539009224.3687.68.camel@sipsolutions.net>
Subject: Re: [PATCH 05/19] wilc: add wilc_wlan_if.h
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Ajay Singh <ajay.kathat@microchip.com>,
        linux-wireless@vger.kernel.org
Cc:     kvalo@codeaurora.org, gregkh@linuxfoundation.org,
        ganesh.krishna@microchip.com, aditya.shankar@microchip.com,
        venkateswara.kaja@microchip.com, claudiu.beznea@microchip.com,
        adham.abozaeid@microchip.com
Date:   Mon, 08 Oct 2018 16:33:44 +0200
In-Reply-To: <1537957525-11467-6-git-send-email-ajay.kathat@microchip.com> (sfid-20180926_122555_951472_AF2668E3)
References: <1537957525-11467-1-git-send-email-ajay.kathat@microchip.com>
         <1537957525-11467-6-git-send-email-ajay.kathat@microchip.com>
         (sfid-20180926_122555_951472_AF2668E3)
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk


> +#define WILC_TX_ERR_NO_BUF	(-2)

Hmm? what's wrong with just e.g. -ENOBUFS? If it doesn't go to userspace
it doesn't matter, and if it does you can't use this anyway? This would
be -ENOENT which is a bad idea.

> +
> +/********************************************
> + *
> + *      Wlan Configuration ID
> + *
> + ********************************************/
> +#define WILC_MULTICAST_TABLE_SIZE	8
> +#define MAX_SSID_LEN            33

Err, it's 32?

> +#define MAX_RATES_SUPPORTED     12
> +
> +enum bss_types {
> +	INFRASTRUCTURE		= 0,
> +	INDEPENDENT,
> +	AP,
> +};
> +
> +enum {
> +	B_ONLY_MODE		= 0,    /* 1, 2 M, otherwise 5, 11 M */
> +	G_ONLY_MODE,			/* 6,12,24 otherwise 9,18,36,48,54 */
> +	G_MIXED_11B_1_MODE,		/* 1,2,5.5,11 otherwise all on */
> +	G_MIXED_11B_2_MODE,		/* 1,2,5,11,6,12,24 otherwise all on */
> +};
> +
> +enum {
> +	G_SHORT_PREAMBLE	= 0,	/* Short Preamble */
> +	G_LONG_PREAMBLE		= 1,	/* Long Preamble */
> +	G_AUTO_PREAMBLE		= 2,	/* Auto Preamble Selection */
> +};

here we have a lot of those "constants should have some sort of prefix"
things ... it's not even clear if they're spec or not:

> +enum authtype {
> +	OPEN_SYSTEM		= 1,
> +	SHARED_KEY		= 2,
> +	ANY			= 3,
> +	IEEE8021		= 5
> +};

These look like they're spec but aren't ... not a good idea.

johannes