Re: [PATCH v5] Bluetooth: Add hci_nxp to hci_uart module to support NXP BT chipsets

[Neeraj sanjay kale]

Hi Marcel,

> From: Marcel Holtmann <marcel@xxxxxxxxxxxx>
> Sent: Monday, December 19, 2022 5:12 PM
> To: Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>
> Cc: Neeraj sanjay kale <neeraj.sanjaykale@xxxxxxx>; Johan Hedberg
> <johan.hedberg@xxxxxxxxx>; Paul Menzel <pmenzel@xxxxxxxxxxxxx>;
> Amitkumar Karwar <amitkumar.karwar@xxxxxxx>; Rohit Fule
> <rohit.fule@xxxxxxx>; Sherry Sun <sherry.sun@xxxxxxx>; LKML <linux-
> kernel@xxxxxxxxxxxxxxx>; linux-bluetooth@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v5] Bluetooth: Add hci_nxp to hci_uart module to
> support NXP BT chipsets
> 
> Hi Luiz,
> 
> >>> Add hci_nxp to the hci_uart module which adds support for the NXP BT
> >>> chips. This driver has Power Save feature that will put the NXP
> >>> bluetooth chip into sleep state, whenever there is no activity for
> >>> certain duration of time (2000ms), and will be woken up when any
> >>> activity is to be initiated.
> >>>
> >>> The Power Save feature can be configured with the following set of
> >>> commands (optional):
> >>> hcitool -i hci0 cmd 3F 23 02 00 00    (enable Power Save)
> >>> hcitool -i hci0 cmd 3F 23 03 00 00    (disable Power Save)
> >>> where,
> >>> OGF = 0x3F (vendor specific command) OCF = 0x23 (command to set
> >>> Power Save state) arg[0] = 0x02 (disable Power Save) arg[0] = 0x03
> >>> (enable Power Save) arg[1,2,...] = XX (don't care)
> >>>
> >>> The sleep/wake-up source can be configured with the following set of
> >>> commands (optional):
> >>> hcitool -i hci0 cmd 3F 53 03 14 01 FF    (set UART break method)
> >>> hcitool -i hci0 cmd 3F 53 03 14 00 FF    (set UART DSR method)
> >>> where,
> >>> OGF = 0x3F (vendor specific command) OCF = 0x53 (command to set
> >>> sleep and wake-up source) arg[0] = 0x00 (Chip to host method NONE)
> >>> arg[0] = 0x01 (Chip to host method UART DTR) arg[0] = 0x02 (Chip to
> >>> host method UART BREAK) arg[0] = 0x03 (Chip to host method GPIO)
> >>> arg[1] = 0x14 (Chip to host GPIO[20] if arg[0] is 0x03, else 0xFF)
> >>> arg[2] = 0x00 (Host to chip method UART DSR) arg[2] = 0x01 (Host to
> >>> chip method UART BREAK) arg[3] = 0xXX (Reserved for future use)
> >>>
> >>> By default, the hci_nxp sets power save enable, chip to host wake-up
> >>> source as GPIO and host to chip sleep and wake-up source as UART
> >>> break during driver initialization, by sending the respective
> >>> commands to the chip.
> >>>
> >>> Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@xxxxxxx>
> >>> ---
> >>> v2: Changed the subject/summary lines and added more details in the
> >>> description. (Paul Menzel)
> >>> v3: Made internal functions static, optimized the code, added few
> >>> comments. (Sherry Sun)
> >>> v4: Reworked entire code to send vendor commands cmd23 and cmd53
> by
> >>> using __hci_cmd_sync. (Luiz Augusto von Dentz)
> >>> v5: Used hci_command_hdr and combined OGF+OCF into a single
> opcode.
> >>> (Luiz Augusto von Dentz)
> >>> ---
> >>> MAINTAINERS                   |   6 +
> >>> drivers/bluetooth/Kconfig     |  10 +
> >>> drivers/bluetooth/Makefile    |   1 +
> >>> drivers/bluetooth/hci_ldisc.c |   6 +
> >>> drivers/bluetooth/hci_nxp.c   | 592
> ++++++++++++++++++++++++++++++++++
> >>> drivers/bluetooth/hci_nxp.h   |  94 ++++++
> >>> drivers/bluetooth/hci_uart.h  |   8 +-
> >>> 7 files changed, 716 insertions(+), 1 deletion(-) create mode 100644
> >>> drivers/bluetooth/hci_nxp.c create mode 100644
> >>> drivers/bluetooth/hci_nxp.h
> >>
> >> so this is a clear NAK. Add this as serdev driver and not hook
> >> further into the mess that is the HCI line discipline.
> >
> > I wonder if we should make it more clear somehow, perhaps include a
> > text on the likes of BT_HCIUART that is deprecated and new drivers
> > shall use BT_HCIUART_SERDEV instead.
> 
> not even that. They need to be separate drivers. A long time ago I posted the
> skeleton for btuart.ko and bt3wire.ko and that is where this has to go.
> 
> Regards
> 
> Marcel

I have just sent out a new patch for btnxp.ko which is based on serdev with subject "Add support for NXP bluetooth chipsets".
Please have a look at it and let me know if this is how you want the driver to be implemented.
Let me know if you have any comments or suggestions and please discard this patch series.

Thanks,
Neeraj