Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Kalle Valo <kvalo@kernel.org>
To:     viktor.barna@celeno.com
Cc:     linux-wireless@vger.kernel.org,
        "David S . Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Aviad Brikman <aviad.brikman@celeno.com>,
        Eliav Farber <eliav.farber@gmail.com>,
        Maksym Kokhan <maksym.kokhan@celeno.com>,
        Oleksandr Savchenko <oleksandr.savchenko@celeno.com>,
        Shay Bar <shay.bar@celeno.com>
Subject: Re: [RFC v2 03/96] cl8k: add Kconfig
References: <20220524113502.1094459-1-viktor.barna@celeno.com>
        <20220524113502.1094459-4-viktor.barna@celeno.com>
Date:   Wed, 13 Jul 2022 10:32:53 +0300
In-Reply-To: <20220524113502.1094459-4-viktor.barna@celeno.com> (viktor
        barna's message of "Tue, 24 May 2022 14:33:29 +0300")
Message-ID: <87v8s1mpqy.fsf@kernel.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

viktor.barna@celeno.com writes:

> From: Viktor Barna <viktor.barna@celeno.com>
>
> (Part of the split. Please, take a look at the cover letter for more
> details).
>
> Signed-off-by: Viktor Barna <viktor.barna@celeno.com>

I haven't looked at rest of the driver yet, will do that in a later
revision. Just looking at build scripts in this round.

> --- /dev/null
> +++ b/drivers/net/wireless/celeno/cl8k/Kconfig
> @@ -0,0 +1,41 @@
> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
> +config CL8K
> +	tristate "Celeno CL8K WLAN support"
> +	depends on m
> +	depends on MAC80211
> +	help
> +	  This option enables support for Celeno CL8K WLAN.
> +	  Select M (recommended), if you have a wireless module.

Why depending on m? Drivers should be able to link to the kernel.

> +config CL8K_VERSION
> +	string "Version"
> +	depends on CL8K
> +	default "8.1.x"
> +	help
> +	  Sets module version, which may be important for FW compatibility
> +	  analysis and syncing upstream codebase with the internal codebase.

Johannes already commented on this.

> +config CL8K_EEPROM_STM24256
> +	bool "EEPROM STM24256 support"
> +	depends on CL8K
> +	default n
> +	help
> +	  Enables EEPROM STM24256 (specific for some of the platforms).

Kconfig should not be used for device configuration, ie. the same kernel
binary should work on all devices. Is there a better way to detect this
runtime? If not, I suggest to drop this in initial submission and submit
after the driver is accepted.

> +config CL8K_DYN_BCAST_RATE
> +	bool "Enable dynamic broadcast rate selection"
> +	depends on CL8K
> +	default n
> +	help
> +	  Enables dynamic broadcast rate selection,
> +	  that allows to tune rate of broadcast frames taking into account
> +	  capabilities of all associated stations.

I don't think features like this should be in Kconfig. Why not always enable this?

> +config CL8K_DYN_MCAST_RATE
> +	bool "Enable dynamic multicast rate selection"
> +	depends on CL8K
> +	default n
> +	help
> +	  Enables dynamic multicast rate selection,
> +	  that allows to tune rate of multicast frames taking into account
> +	  capabilities of all associated stations.

Same with this one.

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

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