Re: [RFC v2 03/96] cl8k: add Kconfig

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
Date: Wed, 13 Jul 2022 10:32:53 +0300	[thread overview]
Message-ID: <87v8s1mpqy.fsf@kernel.org> (raw)
In-Reply-To: <20220524113502.1094459-4-viktor.barna@celeno.com> (viktor barna's message of "Tue, 24 May 2022 14:33:29 +0300")

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