Date:   Mon, 13 Feb 2023 16:15:56 +0100
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
Cc:     davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, marcel@holtmann.org,
        johan.hedberg@gmail.com, luiz.dentz@gmail.com,
        jirislaby@kernel.org, alok.a.tiwari@oracle.com, hdanton@sina.com,
        ilpo.jarvinen@linux.intel.com, leon@kernel.org,
        netdev@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-bluetooth@vger.kernel.org,
        linux-serial@vger.kernel.org, amitkumar.karwar@nxp.com,
        rohit.fule@nxp.com, sherry.sun@nxp.com
Subject: Re: [PATCH v3 3/3] Bluetooth: NXP: Add protocol support for NXP
 Bluetooth chipsets
Message-ID: <Y+pULHm2Qys738Zg@kroah.com>
References: <20230213145432.1192911-1-neeraj.sanjaykale@nxp.com>
 <20230213145432.1192911-4-neeraj.sanjaykale@nxp.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230213145432.1192911-4-neeraj.sanjaykale@nxp.com>
Precedence: bulk

On Mon, Feb 13, 2023 at 08:24:32PM +0530, Neeraj Sanjay Kale wrote:
> This adds a driver based on serdev driver for the NXP BT serial
> protocol based on running H:4, which can enable the built-in
> Bluetooth device inside a generic NXP BT chip.
> 
> This driver has Power Save feature that will put the chip into
> sleep state whenever there is no activity for 2000ms, and will
> be woken up when any activity is to be initiated over UART.
> 
> This driver enables the power save feature by default by sending
> the vendor specific commands to the chip during setup.
> 
> During setup, the driver checks if a FW is already running on the
> chip based on the CTS line, and downloads device specific FW file
> into the chip over UART.
> 
> The driver contains certain device specific default parameters
> related to FW filename, baudrate and timeouts which can be
> overwritten by an optional user space config file. These parameters
> may vary from one module vendor to another.
> 
> Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
> ---
> v2: Removed conf file support and added static data for each chip
> based on compatibility devices mentioned in DT bindings. Handled
> potential memory leaks and null pointer dereference issues,
> simplified FW download feature, handled byte-order and few cosmetic
> changes. (Ilpo J?rvinen, Alok Tiwari, Hillf Danton)
> v3: Added conf file support necessary to support different vendor
> modules, moved .h file contents to .c, cosmetic changes. (Luiz
> Augusto von Dentz, Rob Herring, Leon Romanovsky)
> ---
>  drivers/bluetooth/Kconfig     |   11 +
>  drivers/bluetooth/Makefile    |    1 +
>  drivers/bluetooth/btnxpuart.c | 1370 +++++++++++++++++++++++++++++++++
>  3 files changed, 1382 insertions(+)
>  create mode 100644 drivers/bluetooth/btnxpuart.c
> 
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 5a1a7bec3c42..773b40d34b7b 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -465,4 +465,15 @@ config BT_VIRTIO
>  	  Say Y here to compile support for HCI over Virtio into the
>  	  kernel or say M to compile as a module.
>  
> +config BT_NXPUART
> +	tristate "NXP protocol support"
> +	depends on SERIAL_DEV_BUS
> +	help
> +	  NXP is serial driver required for NXP Bluetooth
> +	  devices with UART interface.
> +
> +	  Say Y here to compile support for NXP Bluetooth UART device into
> +	  the kernel, or say M here to compile as a module.

What is the module name?

> +#define MAX_TAG_STR_LEN				20
> +#define BT_FW_CONF_FILE				"nxp/bt_mod_para.conf"

You can not load "configuration files" as firmware, as that is not what
firmware is for.

Firmware is to be sent straight to the device, the kernel is not
supposed to do any parsing like you are doing:

> +#define USER_CONFIG_TAG				"user_config"
> +#define FW_NAME_TAG					"fw_name"
> +#define OPER_SPEED_TAG				"oper_speed"
> +#define FW_DL_PRI_BAUDRATE_TAG		"fw_dl_pri_speed"
> +#define FW_DL_SEC_BAUDRATE_TAG		"fw_dl_sec_speed"
> +#define FW_INIT_BAUDRATE			"fw_init_speed"
> +#define PS_INTERVAL_MS				"ps_interval_ms"

With these values.

Please use the normal kernel interfaces for configuring your device.
that is NOT the firmware interface, sorry.

thanks,

greg k-h