linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Johannes Berg <johannes@sipsolutions.net>
To: Dmitry Tarnyagin <dmitry.tarnyagin@stericsson.com>
Cc: linux-wireless@vger.kernel.org
Subject: Re: [PATCH 21/21] cw1200: Kconfig + Makefile for the driver.
Date: Fri, 02 Mar 2012 09:50:13 +0100	[thread overview]
Message-ID: <1330678213.8542.6.camel@jlt3.sipsolutions.net> (raw)
In-Reply-To: <1330652495-25837-22-git-send-email-dmitry.tarnyagin@stericsson.com> (sfid-20120302_024231_185549_5ECA663F)

On Fri, 2012-03-02 at 02:41 +0100, Dmitry Tarnyagin wrote:

> +if CW1200

I don't think that's necessary with the "depends on?"

> +config CW1200_STANDALONE
> +      bool "Build a standalone cw1200 driver"
> +      depends on CW1200
> +      help
> +        Say Y here if you would like to build a standalone cw1200 driver,
> +        not associated with any platform. Likely you wil not be able to use
> +	it for any purposes.
> +        If unsure, say N.

typo "will", and some indentation issue

> +config CW1200_USE_STE_EXTENSIONS
> +      bool "STE extensions"
> +      depends on CW1200
> +      help
> +        Say Y if you want to include experimental code or code with
> +	not resolved dependency.
> +        If unsure, say N.

This seems questionable. Maybe you should get rid of it for now and
explain what is needed and extend the APIs as needed?

> +config CW1200_WAPI_SUPPORT
> +      bool "WAPI support"
> +      depends on CW1200_USE_STE_EXTENSIONS
> +      help
> +        Say Y if your compat-wireless support WAPI.
> +        If unsure, say N.

This is unnecessary. You can always advertise support for SMS4. Also, it
really shouldn't be talking about compat-wireless here since you're
submitting this driver for upstream :-)

> +config CW1200_DISABLE_BEACON_HINTS
> +      bool "Disable 11d beacon hints"
> +      depends on CW1200
> +      help
> +        Say Y if you want to disable 11d beacon hints.
> +        If unsure, say N.

I don't understand this -- beacon hints or not should be your decision
based on how the device works wrt. regulatory, not the users?

> +config CW1200_BH_DEBUG
> +      bool "Enable low-level device communication logs (DEVELOPMENT)"
> +      help
> +        Say Y if you want to enable BH logs.
> +        If unsure, say N.

What does "BH" stand for here? In the kernel it typically stands for
Bottom Half (or softirq) but that can't be meant? In other places in
your code you do mean that though, it seems, so this is a bit confusing.

> +config CW1200_WSM_DEBUG
> +      bool "Enable WSM API debug messages (DEVELOPMENT)"
> +      help
> +        Say Y if you want to enable WSM logs.
> +        If unsure, say N.
> +
> +config CW1200_WSM_DUMPS
> +      bool "Verbose WSM API logging (DEVELOPMENT)"
> +      help
> +        Say Y if you want to enable WSM dumps.
> +        If unsure, say N.

You should think about adding tracing instead.

> +config CW1200_WSM_DUMPS_SHORT
> +      depends on CW1200_WSM_DUMPS
> +      bool "Dump only first x bytes (default 20) (DEVELOPMENT)"
> +      help
> +        Say Y if you want to limit amount of data printed in WSM dumps.
> +        If unsure, say N.

Then you also don't need this since tracing is really fast. On a core i3
I can trace full HT "line" rate. It's also a lot more powerful for
analysis tools. Yes I need to give a talk about this some time ;-)


johannes


  reply	other threads:[~2012-03-02  8:50 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <d53e019b1a0bcd29c2c367fbe5665413f2d33938-submit>
2012-03-02  1:41 ` [PATCH 00/21] cw1200: mac80211-based driver for ST-Ericsson CW1200 device Dmitry Tarnyagin
2012-03-02  1:41   ` [PATCH 01/21] cw1200: cw1200.h, private driver data Dmitry Tarnyagin
2012-03-02  1:41   ` [PATCH 02/21] cw1200: cw1200_plat.h, definition of the driver'ss platform data Dmitry Tarnyagin
2012-03-02  1:41   ` [PATCH 03/21] cw1200: sbus.h, common device interface abstraction Dmitry Tarnyagin
2012-03-02  1:41   ` [PATCH 04/21] cw1200: cw1200_sdio.c, implementation of SDIO wrapper for the driver Dmitry Tarnyagin
2012-03-02  1:41   ` [PATCH 05/21] cw1200: hwio.*, device reg/mem map and low-level i/o primitives Dmitry Tarnyagin
2012-03-02  1:41   ` [PATCH 06/21] cw1200: fwio.*, firmware downloading code for the cw1200 driver Dmitry Tarnyagin
2012-03-02  1:41   ` [PATCH 07/21] cw1200: queue.*, implementation of TX queues of " Dmitry Tarnyagin
2012-03-02  8:33     ` Johannes Berg
2012-03-02 15:32       ` Dmitry Tarnyagin
2012-03-02  1:41   ` [PATCH 08/21] cw1200: wsm.*, implementation of device high-level interface Dmitry Tarnyagin
2012-03-02  8:34     ` Johannes Berg
2012-03-02  8:41       ` Joe Perches
2012-03-02  1:41   ` [PATCH 09/21] cw1200: txrx.*, implementation of datapath Dmitry Tarnyagin
2012-05-07 12:53     ` Bob Copeland
2012-05-08  7:09       ` Dmitry Tarnyagin
2012-05-08 12:54         ` Bob Copeland
2012-03-02  1:41   ` [PATCH 10/21] cw1200: ht.h, small helper header with HT definitions Dmitry Tarnyagin
2012-03-02  1:41   ` [PATCH 11/21] cw1200: bh.*, device serving thread Dmitry Tarnyagin
2012-03-02  1:41   ` [PATCH 12/21] cw1200: sta.*, mac80211 STA callbacks Dmitry Tarnyagin
2012-03-02  1:41   ` [PATCH 13/21] cw1200: ap.*, mac80211 AP callbacks Dmitry Tarnyagin
2012-03-02  1:41   ` [PATCH 14/21] cw1200: scan.*, mac80211 hw_scan callback Dmitry Tarnyagin
2012-03-02  1:41   ` [PATCH 15/21] cw1200: debug.*, implementation of the driver's debugfs Dmitry Tarnyagin
2012-03-02  1:41   ` [PATCH 16/21] cw1200: itp.*, internal device test and calibration code Dmitry Tarnyagin
2012-03-02  1:41   ` [PATCH 17/21] cw1200: pm.*, power management code Dmitry Tarnyagin
2012-03-02  1:41   ` [PATCH 18/21] cw1200: main.c, core initialization code Dmitry Tarnyagin
2012-03-02  1:41   ` [PATCH 19/21] cw1200: TODO list Dmitry Tarnyagin
2012-03-02  8:51     ` Johannes Berg
2012-03-02  1:41   ` [PATCH 20/21] cw1200: Credits Dmitry Tarnyagin
2012-03-02  1:41   ` [PATCH 21/21] cw1200: Kconfig + Makefile for the driver Dmitry Tarnyagin
2012-03-02  8:50     ` Johannes Berg [this message]
2012-03-02 15:45       ` Dmitry Tarnyagin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1330678213.8542.6.camel@jlt3.sipsolutions.net \
    --to=johannes@sipsolutions.net \
    --cc=dmitry.tarnyagin@stericsson.com \
    --cc=linux-wireless@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).