Re: [PATCH v7 3/3] Bluetooth: NXP: Add protocol support for NXP Bluetooth chipsets

[Ilpo Järvinen]

On Mon, 6 Mar 2023, 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 an 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
> by waiting for the bootloader signature, and downloads device specific FW
> file into the chip over UART if bootloader signature is received..
> 
> Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@xxxxxxx>
> ---
> 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)
> v4: Removed conf file support, optimized driver data, add logic to select
> FW name based on chip signature (Greg KH, Ilpo Järvinen, Sherry Sun)
> v5: Replaced bt_dev_info() with bt_dev_dbg(), handled user-space cmd
> parsing in nxp_enqueue() in a better way. (Greg KH, Luiz Augusto von Dentz)
> v6: Add support for fw-init-baudrate parameter from device tree,
> modified logic to detect FW download is needed or FW is running. (Greg
> KH, Sherry Sun)
> v7: Renamed variables, improved FW download functions, include ps_data
> into btnxpuart_dev. (Ilpo Järvinen)
> ---
>  MAINTAINERS                   |    1 +
>  drivers/bluetooth/Kconfig     |   11 +
>  drivers/bluetooth/Makefile    |    1 +
>  drivers/bluetooth/btnxpuart.c | 1309 +++++++++++++++++++++++++++++++++
>  4 files changed, 1322 insertions(+)
>  create mode 100644 drivers/bluetooth/btnxpuart.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 030ec6fe89df..fdb9b0788c89 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22840,6 +22840,7 @@ M:	Amitkumar Karwar <amitkumar.karwar@xxxxxxx>
>  M:	Neeraj Kale <neeraj.sanjaykale@xxxxxxx>
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml
> +F:	drivers/bluetooth/btnxpuart.c
>  
>  THE REST
>  M:	Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 5a1a7bec3c42..359a4833e31f 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

select CRC32 since you're using it now.

> +	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 (btnxpuart).
> +
> +
>  endmenu


> +static void ps_control(struct hci_dev *hdev, u8 ps_state)
> +{
> +	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +	struct ps_data *psdata = &nxpdev->psdata;
> +	int status;
> +
> +	if (psdata->ps_state == ps_state ||
> +	    !test_bit(BTNXPUART_SERDEV_OPEN, &nxpdev->tx_state))
> +		return;
> +
> +	switch (psdata->cur_h2c_wakeupmode) {
> +	case WAKEUP_METHOD_DTR:
> +		if (ps_state == PS_STATE_AWAKE)
> +			status = serdev_device_set_tiocm(nxpdev->serdev, TIOCM_DTR, 0);
> +		else
> +			status = serdev_device_set_tiocm(nxpdev->serdev, 0, TIOCM_DTR);
> +		break;
> +	case WAKEUP_METHOD_BREAK:
> +	default:
> +		if (ps_state == PS_STATE_AWAKE)
> +			status = serdev_device_break_ctl(nxpdev->serdev, 0);
> +		else
> +			status = serdev_device_break_ctl(nxpdev->serdev, -1);
> +		bt_dev_dbg(hdev, "Set UART break: %s, status=%d",
> +			   str_on_off(ps_state == PS_STATE_SLEEP), status);

Add the #include for str_on_off too.


> +/* for legacy chipsets with V1 bootloader */
> +static int nxp_recv_fw_req_v1(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> +	struct btnxpuart_dev *nxpdev = hci_get_drvdata(hdev);
> +	struct btnxpuart_data *nxp_data = nxpdev->nxp_data;
> +	struct v1_data_req *req;
> +	u32 requested_len;
> +
> +	if (test_bit(BTNXPUART_CHECK_BOOT_SIGNATURE, &nxpdev->tx_state)) {
> +		clear_bit(BTNXPUART_CHECK_BOOT_SIGNATURE, &nxpdev->tx_state);
> +		wake_up_interruptible(&nxpdev->check_boot_sign_wait_q);
> +		goto ret;
> +	}
> +
> +	if (!is_fw_downloading(nxpdev))
> +		goto ret;

That BTNXPUART_CHECK_BOOT_SIGNATURE check above is also the same in 3 
callsites of is_fw_downloading() so too should be moved into a common 
helper (there was 4th call into is_fw_downloading() so make another 
help for these 3 users).


-- 
 i.