Re: [PATCH v1] Bluetooth: Add hci_nxp BT submodule with controller Power Save feature for NXP BT chipsets.

[Neeraj sanjay kale]

Hi Paul,

Thank you for your review. I have addressed the comments in V2.
Please check my response in-line below.

Thanks,
Neeraj

> From: Paul Menzel <pmenzel@xxxxxxxxxxxxx>
> Sent: Wednesday, November 16, 2022 7:57 PM
> 
> Dear Neeraj,
> 
> 
> Thank you for your patch.
> 
> 
> Am 16.11.22 um 17:42 schrieb Neeraj Sanjay Kale:
Corrected time on host machine.
> 
> Please remove the dot/period at the end of the summary/subject line.
> 
> > Added proprietary hci_uart driver code to the linux hci uart
> > sub-system as
> 
> Please use present tense: Add
Corrected the summary/subject line, and added few more details to the description.
> 
> “proprietary … code” sounds strange, as Linux is FLOSS. Please clarify.
> 
> > hci_nxp. 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.
> >
> > This Power Save feature will be enabled by default and can be disabled
> > and re-enabled by the following set of commands:
> > hcitool -i hci0 cmd 3F 23 02 00 00    (disable Power Save)
> > hcitool -i hci0 cmd 3F 23 03 00 00    (enable Power Save)
> >
> > The Power Save feature uses UART break signal by default as chip sleep
> > and wake-up source, and can be set with the following set of commands:
> > hcitool -i hci0 cmd 3F 53 03 14 01 FF    (set UART break method)
> > hcitool -i hci0 cmd 3F 53 03 14 02 15    (set chip's GPIO[20] method)
> 
> Please mention the datasheet, and how you tested the driver.
Added explanation of the vendor specific hcitool commands 0x53 and 0x23  to the v2  description.
However, please note that these vendor specific commands are sent by the driver to the chip during initialization, to enable the power save feature. The user need not issue these commands, or be aware of it, if he intends to use the driver along with NXP chip in default mode.
> 
> > Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@xxxxxxx>
> > ---
> >   MAINTAINERS                   |   6 +
> >   drivers/bluetooth/Kconfig     |  10 +
> >   drivers/bluetooth/Makefile    |   1 +
> >   drivers/bluetooth/hci_ldisc.c |   6 +
> >   drivers/bluetooth/hci_nxp.c   | 763
> ++++++++++++++++++++++++++++++++++++++++++
> >   drivers/bluetooth/hci_nxp.h   | 118 +++++++
> >   drivers/bluetooth/hci_uart.h  |   8 +-
> >   7 files changed, 911 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/bluetooth/hci_nxp.c
> >   create mode 100644 drivers/bluetooth/hci_nxp.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS index e55a4d4..6b2c264 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -22591,6 +22591,12 @@ L:   linux-mm@xxxxxxxxx
> >   S:  Maintained
> >   F:  mm/zswap.c
> >
> > +NXP BLUETOOTH WIRELESS DRIVERS
> > +M:   amitkumar.karwar@xxxxxxx
> > +M:   neeraj.sanjaykale@xxxxxxx
> 
> Mentioning a mailing list, and maybe setting up a functional address, would
> be nice (nxp-bluetooth-wireless-linux-driver@xxxxxxx).
I will check if we can have such mailing list.
> 
> > +S:   Maintained
> > +F:   drivers/bluetooth/hci_nxp*
> > +
> >   THE REST
> >   M:  Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> >   L:  linux-kernel@xxxxxxxxxxxxxxx
> 
> […]
> 
> 
> Kind regards,
> 
> Paul