mbox series

[v8,0/3] Add support for NXP bluetooth chipsets

Message ID 20230310181921.1437890-1-neeraj.sanjaykale@nxp.com
Headers show
Series Add support for NXP bluetooth chipsets | expand

Message

Neeraj Sanjay Kale March 10, 2023, 6:19 p.m. UTC 
This patch adds a driver for NXP bluetooth chipsets.

The driver is based on H4 protocol, and uses serdev APIs. It supports host
to chip power save feature, which is signalled by the host by asserting
break over UART TX lines, to put the chip into sleep state.

To support this feature, break_ctl has also been added to serdev-tty along
with a new serdev API serdev_device_break_ctl().

This driver is capable of downloading firmware into the chip over UART.

The document specifying device tree bindings for this driver is also
included in this patch series.

Neeraj Sanjay Kale (3):
  serdev: Add method to assert break signal over tty UART port
  dt-bindings: net: bluetooth: Add NXP bluetooth support
  Bluetooth: NXP: Add protocol support for NXP Bluetooth chipsets

 .../net/bluetooth/nxp,88w8987-bt.yaml         |   46 +
 MAINTAINERS                                   |    7 +
 drivers/bluetooth/Kconfig                     |   11 +
 drivers/bluetooth/Makefile                    |    1 +
 drivers/bluetooth/btnxpuart.c                 | 1293 +++++++++++++++++
 drivers/tty/serdev/core.c                     |   11 +
 drivers/tty/serdev/serdev-ttyport.c           |   12 +
 include/linux/serdev.h                        |    6 +
 8 files changed, 1387 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/bluetooth/nxp,88w8987-bt.yaml
 create mode 100644 drivers/bluetooth/btnxpuart.c

Comments

Neeraj Sanjay Kale March 12, 2023, 7:01 a.m. UTC | #1 
Hi Simon

> 
> On Fri, Mar 10, 2023 at 11:49:19PM +0530, Neeraj Sanjay Kale wrote:
> > Adds serdev_device_break_ctl() and an implementation for ttyport.
> > This function simply calls the break_ctl in tty layer, which can
> > assert a break signal over UART-TX line, if the tty and the underlying
> > platform and UART peripheral supports this operation.
> >
> > Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
> > ---
> > v3: Add details to the commit message. (Greg KH)
> 
> ...
> 
> > diff --git a/include/linux/serdev.h b/include/linux/serdev.h index
> > 66f624fc618c..c065ef1c82f1 100644
> > --- a/include/linux/serdev.h
> > +++ b/include/linux/serdev.h
> 
> ...
> 
> > @@ -255,6 +257,10 @@ static inline int serdev_device_set_tiocm(struct
> > serdev_device *serdev, int set,  {
> >       return -ENOTSUPP;
> >  }
> > +static inline int serdev_device_break_ctl(struct serdev_device
> > +*serdev, int break_state) {
> > +     return -EOPNOTSUPP;
> 
> Is the use of -EOPNOTSUPP intentional here?
> I see -ENOTSUPP is used elsewhere in this file.
I was suggested to use - EOPNOTSUPP instead of - ENOTSUPP by the check patch scripts and by Leon Romanovsky.
https://patchwork.kernel.org/project/bluetooth/patch/20230130180504.2029440-2-neeraj.sanjaykale@nxp.com/

ENOTSUPP is not a standard error code and should be avoided in new patches.
See: https://lore.kernel.org/netdev/20200510182252.GA411829@lunn.ch/

> 
> > +}
> >  static inline int serdev_device_write(struct serdev_device *sdev, const
> unsigned char *buf,
> >                                     size_t count, unsigned long
> > timeout)  {
> > --
> > 2.34.1
> >

Thanks,
Neeraj
Simon Horman March 12, 2023, 7:12 p.m. UTC | #2 
On Sun, Mar 12, 2023 at 07:01:17AM +0000, Neeraj sanjay kale wrote:
> Hi Simon
> 
> > 
> > On Fri, Mar 10, 2023 at 11:49:19PM +0530, Neeraj Sanjay Kale wrote:
> > > Adds serdev_device_break_ctl() and an implementation for ttyport.
> > > This function simply calls the break_ctl in tty layer, which can
> > > assert a break signal over UART-TX line, if the tty and the underlying
> > > platform and UART peripheral supports this operation.
> > >
> > > Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
> > > ---
> > > v3: Add details to the commit message. (Greg KH)
> > 
> > ...
> > 
> > > diff --git a/include/linux/serdev.h b/include/linux/serdev.h index
> > > 66f624fc618c..c065ef1c82f1 100644
> > > --- a/include/linux/serdev.h
> > > +++ b/include/linux/serdev.h
> > 
> > ...
> > 
> > > @@ -255,6 +257,10 @@ static inline int serdev_device_set_tiocm(struct
> > > serdev_device *serdev, int set,  {
> > >       return -ENOTSUPP;
> > >  }
> > > +static inline int serdev_device_break_ctl(struct serdev_device
> > > +*serdev, int break_state) {
> > > +     return -EOPNOTSUPP;
> > 
> > Is the use of -EOPNOTSUPP intentional here?
> > I see -ENOTSUPP is used elsewhere in this file.
> I was suggested to use - EOPNOTSUPP instead of - ENOTSUPP by the check patch scripts and by Leon Romanovsky.
> https://patchwork.kernel.org/project/bluetooth/patch/20230130180504.2029440-2-neeraj.sanjaykale@nxp.com/
> 
> ENOTSUPP is not a standard error code and should be avoided in new patches.
> See: https://lore.kernel.org/netdev/20200510182252.GA411829@lunn.ch/

Thanks.

I agree that EOPNOTSUPP is preferable.
But my question is if we chose to use it in this case,
even if it is inconsistent with similar code in the same file/API.
If so, then I have no objections.
Ilpo Järvinen March 13, 2023, 9:01 a.m. UTC | #3 
On Fri, 10 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@nxp.com>
> ---

> v8: Move bootloader signature handling to a separate function. Add
> select CRC32 to Kconfig file. (Ilpo Järvinen)

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

The select change in not there.
Simon Horman March 13, 2023, 12:06 p.m. UTC | #4 
On Mon, Mar 13, 2023 at 04:19:59AM +0000, Neeraj sanjay kale wrote:
> Hi Simon
> 
> > > >
> > > > On Fri, Mar 10, 2023 at 11:49:19PM +0530, Neeraj Sanjay Kale wrote:
> > > > > Adds serdev_device_break_ctl() and an implementation for ttyport.
> > > > > This function simply calls the break_ctl in tty layer, which can
> > > > > assert a break signal over UART-TX line, if the tty and the
> > > > > underlying platform and UART peripheral supports this operation.
> > > > >
> > > > > Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>
> > > > > ---
> > > > > v3: Add details to the commit message. (Greg KH)
> > > >
> > > > ...
> > > >
> > > > > diff --git a/include/linux/serdev.h b/include/linux/serdev.h index
> > > > > 66f624fc618c..c065ef1c82f1 100644
> > > > > --- a/include/linux/serdev.h
> > > > > +++ b/include/linux/serdev.h
> > > >
> > > > ...
> > > >
> > > > > @@ -255,6 +257,10 @@ static inline int
> > > > > serdev_device_set_tiocm(struct serdev_device *serdev, int set,  {
> > > > >       return -ENOTSUPP;
> > > > >  }
> > > > > +static inline int serdev_device_break_ctl(struct serdev_device
> > > > > +*serdev, int break_state) {
> > > > > +     return -EOPNOTSUPP;
> > > >
> > > > Is the use of -EOPNOTSUPP intentional here?
> > > > I see -ENOTSUPP is used elsewhere in this file.
> > > I was suggested to use - EOPNOTSUPP instead of - ENOTSUPP by the check
> > patch scripts and by Leon Romanovsky.
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> > >
> > hwork.kernel.org%2Fproject%2Fbluetooth%2Fpatch%2F20230130180504.202
> > 944
> > > 0-2-
> > neeraj.sanjaykale%40nxp.com%2F&data=05%7C01%7Cneeraj.sanjaykale%40
> > >
> > nxp.com%7Cf2ae2c9ad3c243df2c1a08db232dc0f1%7C686ea1d3bc2b4c6fa92
> > cd99c5
> > >
> > c301635%7C0%7C0%7C638142451647332825%7CUnknown%7CTWFpbGZsb3
> > d8eyJWIjoiM
> > >
> > C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000
> > %7C%7C
> > > %7C&sdata=6cF0gipe4kkwYI6txo0vs8vnmF8azCO6gxQ%2F6Tdyd%2Fw%3D
> > &reserved=
> > > 0
> > >
> > > ENOTSUPP is not a standard error code and should be avoided in new
> > patches.
> > > See:
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore
> > > .kernel.org%2Fnetdev%2F20200510182252.GA411829%40lunn.ch%2F&data
> > =05%7C
> > >
> > 01%7Cneeraj.sanjaykale%40nxp.com%7Cf2ae2c9ad3c243df2c1a08db232dc0f
> > 1%7C
> > >
> > 686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638142451647332825%
> > 7CUnknow
> > >
> > n%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha
> > WwiLC
> > >
> > JXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=wFgYY6VnZ8BBn6Wme8%2BYj
> > aJRy98qyPnUy
> > > XC8iCFCv5k%3D&reserved=0
> > 
> > Thanks.
> > 
> > I agree that EOPNOTSUPP is preferable.
> > But my question is if we chose to use it in this case, even if it is inconsistent
> > with similar code in the same file/API.
> > If so, then I have no objections.
> 
> No, it was just to satisfy the check patch error and Leon's comment. The driver is happy to check if the serdev returned success or not, and simply print the error code during driver debug.
> Do you think this should be reverted to ENOTSUPP to maintain consistency?

My _opinion_, is that first prize would be converting existing instances
of ENOTSUPP in this file to EOPNOTSUPP. And then use EOPNOTSUPP going
forward. And that second prize would be for your patch to use ENOTSUPP.
Because I think there is a value consistency.

But I do see why you have done things the way you have.
And I don't necessarily think it is wrong.