[RFC 0/7] Nokia N9xx bluetooth driver

[RFC 0/7] Nokia N9xx bluetooth driver

[Sebastian Reichel]

Hi,

This series (based von 4.8-rc1) adds support for bluetooth on the Nokia
N9xx devices. It has been tested on the Nokia N950, where it works
correctly. On Nokia N900 it currently fails during negotiation
(probably related to slightly incorrect serial settings/timings).
The N900's bcm2048 correctly answeres to alive check even before
negotiation (on N950 it does not work before negotiation), but replies
with an Hardware error event to the negotiation packet.

Apart from N900 support there are still two "features" missing in the
driver:

1. To save energy the bluetooth module can be put into sleep mode via a
   GPIO. This gpio should be enabled before sending data via UART and
   disabled once the transmission is done. I currently just keep the
   GPIO always enabled.
2. It would be nice to have a bluetooth device exposed by the kernel
   automatically without having to setup the tty disector first for
   proper configurationless out of the box support. I could not find
   a nice way to do this from the kernel, though.

On N950 the driver works with omap-serial and omap8250-serial drivers.

You can also find this series in the following branch:
https://git.kernel.org/cgit/linux/kernel/git/sre/linux-n900.git/log/?h=nokia-bluetooth

-- Sebastian

NeilBrown (1):
  tty: add support for "tty slave" devices

Sebastian Reichel (6):
  tty: serial: omap: add UPF_BOOT_AUTOCONF flag for DT init
  dt: bindings: Add nokia-bluetooth
  Bluetooth: hci_uart: Add support for word alignment
  Bluetooth: hci_nokia: Introduce new driver
  ARM: dts: OMAP3-N900: Add bluetooth
  ARM: dts: OMAP3-N950: Add bluetooth

 .../devicetree/bindings/net/nokia-bluetooth.txt    |  43 ++
 Documentation/devicetree/bindings/serial/8250.txt  |   4 +
 arch/arm/boot/dts/omap3-n900.dts                   |  22 +-
 arch/arm/boot/dts/omap3-n950-n9.dtsi               |  34 +
 drivers/bluetooth/Kconfig                          |  10 +
 drivers/bluetooth/Makefile                         |   1 +
 drivers/bluetooth/hci_h4.c                         |  10 +
 drivers/bluetooth/hci_ldisc.c                      |   6 +
 drivers/bluetooth/hci_nokia.c                      | 734 +++++++++++++++++++++
 drivers/bluetooth/hci_nokia.h                      | 140 ++++
 drivers/bluetooth/hci_uart.h                       |   9 +-
 drivers/tty/serial/omap-serial.c                   |   3 +
 drivers/tty/tty_io.c                               |   6 +
 13 files changed, 1020 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/nokia-bluetooth.txt
 create mode 100644 drivers/bluetooth/hci_nokia.c
 create mode 100644 drivers/bluetooth/hci_nokia.h

-- 
2.8.1

[RFC 1/7] tty: serial: omap: add UPF_BOOT_AUTOCONF flag for DT init

[Sebastian Reichel]

---
 drivers/tty/serial/omap-serial.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index a2a529994ba5..7c2c77789c2c 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1542,6 +1542,9 @@ static struct omap_uart_port_info *of_get_uart_port_info(struct device *dev)
 
 	of_property_read_u32(dev->of_node, "clock-frequency",
 					 &omap_up_info->uartclk);
+
+	omap_up_info->flags = UPF_BOOT_AUTOCONF;
+
 	return omap_up_info;
 }
 
-- 
2.8.1

Re: [RFC 1/7] tty: serial: omap: add UPF_BOOT_AUTOCONF flag for DT init

[Pavel Machek]

It would be nice to have a line about why it is needed, unfortunately
include/linux/serial_core.h is not exactly helpful.

Plus you'll need to sign off the patch.

Acked-by: Pavel Machek <pavel@ucw.cz>

Thanks,
								Pavel
								
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC 1/7] tty: serial: omap: add UPF_BOOT_AUTOCONF flag for DT init

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 345 bytes --]

Hi,

On Sun, Aug 14, 2016 at 10:49:56AM +0200, Pavel Machek wrote:
> It would be nice to have a line about why it is needed,
> unfortunately include/linux/serial_core.h is not exactly helpful.
>
> Plus you'll need to sign off the patch.

I should have had another look at the long patch
descriptions before sending the patchset ;)

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 1/7] tty: serial: omap: add UPF_BOOT_AUTOCONF flag for DT init

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 345 bytes --]

Hi,

On Sun, Aug 14, 2016 at 10:49:56AM +0200, Pavel Machek wrote:
> It would be nice to have a line about why it is needed,
> unfortunately include/linux/serial_core.h is not exactly helpful.
>
> Plus you'll need to sign off the patch.

I should have had another look at the long patch
descriptions before sending the patchset ;)

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[RFC 2/7] tty: add support for "tty slave" devices

[Sebastian Reichel]

From: NeilBrown <neilb@suse.de>

A "tty slave" is a device connected via UART.  It may need a driver to,
for example, power the device on when the tty is opened, and power it
off when the tty is released.

Signed-off-by: NeilBrown <neilb@suse.de>
Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 Documentation/devicetree/bindings/serial/8250.txt | 4 ++++
 drivers/tty/tty_io.c                              | 6 ++++++
 2 files changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/serial/8250.txt b/Documentation/devicetree/bindings/serial/8250.txt
index f5561ac7e17e..ecb74730f71b 100644
--- a/Documentation/devicetree/bindings/serial/8250.txt
+++ b/Documentation/devicetree/bindings/serial/8250.txt
@@ -46,6 +46,10 @@ Optional properties:
   line respectively. It will use specified GPIO instead of the peripheral
   function pin for the UART feature. If unsure, don't specify this property.
 
+Optional child node:
+- a device connected to the uart can be specified as child node with
+  compatible value.
+
 Note:
 * fsl,ns16550:
   ------------
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 734a635e7363..39ff5dcdfd50 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -95,6 +95,7 @@
 #include <linux/seq_file.h>
 #include <linux/serial.h>
 #include <linux/ratelimit.h>
+#include <linux/of_platform.h>
 
 #include <linux/uaccess.h>
 
@@ -3317,6 +3318,11 @@ struct device *tty_register_device_attr(struct tty_driver *driver,
 	retval = device_register(dev);
 	if (retval)
 		goto error;
+	if (device && device->of_node)
+		/* Children are platform devices and will be
+		 * runtime_pm managed by this tty.
+		 */
+		of_platform_populate(device->of_node, NULL, NULL, dev);
 
 	return dev;
 
-- 
2.8.1

Re: [RFC 2/7] tty: add support for "tty slave" devices

[Greg Kroah-Hartman]

On Sat, Aug 13, 2016 at 05:14:33AM +0200, Sebastian Reichel wrote:
> From: NeilBrown <neilb@suse.de>
> 
> A "tty slave" is a device connected via UART.  It may need a driver to,
> for example, power the device on when the tty is opened, and power it
> off when the tty is released.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> ---
>  Documentation/devicetree/bindings/serial/8250.txt | 4 ++++
>  drivers/tty/tty_io.c                              | 6 ++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/8250.txt b/Documentation/devicetree/bindings/serial/8250.txt
> index f5561ac7e17e..ecb74730f71b 100644
> --- a/Documentation/devicetree/bindings/serial/8250.txt
> +++ b/Documentation/devicetree/bindings/serial/8250.txt
> @@ -46,6 +46,10 @@ Optional properties:
>    line respectively. It will use specified GPIO instead of the peripheral
>    function pin for the UART feature. If unsure, don't specify this property.
>  
> +Optional child node:
> +- a device connected to the uart can be specified as child node with
> +  compatible value.
> +
>  Note:
>  * fsl,ns16550:
>    ------------
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index 734a635e7363..39ff5dcdfd50 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -95,6 +95,7 @@
>  #include <linux/seq_file.h>
>  #include <linux/serial.h>
>  #include <linux/ratelimit.h>
> +#include <linux/of_platform.h>
>  
>  #include <linux/uaccess.h>
>  
> @@ -3317,6 +3318,11 @@ struct device *tty_register_device_attr(struct tty_driver *driver,
>  	retval = device_register(dev);
>  	if (retval)
>  		goto error;
> +	if (device && device->of_node)
> +		/* Children are platform devices and will be
> +		 * runtime_pm managed by this tty.
> +		 */
> +		of_platform_populate(device->of_node, NULL, NULL, dev);

Why are these platform devices?  And why only OF?

thanks,

greg k-h

Re: [RFC 2/7] tty: add support for "tty slave" devices

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2559 bytes --]

Hi,

On Sat, Aug 13, 2016 at 12:03:45PM +0200, Greg Kroah-Hartman wrote:
> On Sat, Aug 13, 2016 at 05:14:33AM +0200, Sebastian Reichel wrote:
> > From: NeilBrown <neilb@suse.de>
> > 
> > A "tty slave" is a device connected via UART.  It may need a driver to,
> > for example, power the device on when the tty is opened, and power it
> > off when the tty is released.
> > 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > Signed-off-by: Sebastian Reichel <sre@kernel.org>
> > ---
> >  Documentation/devicetree/bindings/serial/8250.txt | 4 ++++
> >  drivers/tty/tty_io.c                              | 6 ++++++
> >  2 files changed, 10 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/serial/8250.txt b/Documentation/devicetree/bindings/serial/8250.txt
> > index f5561ac7e17e..ecb74730f71b 100644
> > --- a/Documentation/devicetree/bindings/serial/8250.txt
> > +++ b/Documentation/devicetree/bindings/serial/8250.txt
> > @@ -46,6 +46,10 @@ Optional properties:
> >    line respectively. It will use specified GPIO instead of the peripheral
> >    function pin for the UART feature. If unsure, don't specify this property.
> >  
> > +Optional child node:
> > +- a device connected to the uart can be specified as child node with
> > +  compatible value.
> > +
> >  Note:
> >  * fsl,ns16550:
> >    ------------
> > diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> > index 734a635e7363..39ff5dcdfd50 100644
> > --- a/drivers/tty/tty_io.c
> > +++ b/drivers/tty/tty_io.c
> > @@ -95,6 +95,7 @@
> >  #include <linux/seq_file.h>
> >  #include <linux/serial.h>
> >  #include <linux/ratelimit.h>
> > +#include <linux/of_platform.h>
> >  
> >  #include <linux/uaccess.h>
> >  
> > @@ -3317,6 +3318,11 @@ struct device *tty_register_device_attr(struct tty_driver *driver,
> >  	retval = device_register(dev);
> >  	if (retval)
> >  		goto error;
> > +	if (device && device->of_node)
> > +		/* Children are platform devices and will be
> > +		 * runtime_pm managed by this tty.
> > +		 */
> > +		of_platform_populate(device->of_node, NULL, NULL, dev);
>
> Why are these platform devices?
> And why only OF?

I just took this patch over from Neil to get bluetooth working
on N900/N950. Both of them are DT only (well N900 still has
boardcode, but that will be removed shortly), so it was enough
for me.

I guess you have something in mind, that's similar to e.g. i2c
and spi with a serial_client device and support to instanciate
it from DT, ACPI and boardcode?

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 2/7] tty: add support for "tty slave" devices

[Greg Kroah-Hartman]

On Sat, Aug 13, 2016 at 10:31:24PM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Sat, Aug 13, 2016 at 12:03:45PM +0200, Greg Kroah-Hartman wrote:
> > On Sat, Aug 13, 2016 at 05:14:33AM +0200, Sebastian Reichel wrote:
> > > From: NeilBrown <neilb@suse.de>
> > > 
> > > A "tty slave" is a device connected via UART.  It may need a driver to,
> > > for example, power the device on when the tty is opened, and power it
> > > off when the tty is released.
> > > 
> > > Signed-off-by: NeilBrown <neilb@suse.de>
> > > Signed-off-by: Sebastian Reichel <sre@kernel.org>
> > > ---
> > >  Documentation/devicetree/bindings/serial/8250.txt | 4 ++++
> > >  drivers/tty/tty_io.c                              | 6 ++++++
> > >  2 files changed, 10 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/serial/8250.txt b/Documentation/devicetree/bindings/serial/8250.txt
> > > index f5561ac7e17e..ecb74730f71b 100644
> > > --- a/Documentation/devicetree/bindings/serial/8250.txt
> > > +++ b/Documentation/devicetree/bindings/serial/8250.txt
> > > @@ -46,6 +46,10 @@ Optional properties:
> > >    line respectively. It will use specified GPIO instead of the peripheral
> > >    function pin for the UART feature. If unsure, don't specify this property.
> > >  
> > > +Optional child node:
> > > +- a device connected to the uart can be specified as child node with
> > > +  compatible value.
> > > +
> > >  Note:
> > >  * fsl,ns16550:
> > >    ------------
> > > diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> > > index 734a635e7363..39ff5dcdfd50 100644
> > > --- a/drivers/tty/tty_io.c
> > > +++ b/drivers/tty/tty_io.c
> > > @@ -95,6 +95,7 @@
> > >  #include <linux/seq_file.h>
> > >  #include <linux/serial.h>
> > >  #include <linux/ratelimit.h>
> > > +#include <linux/of_platform.h>
> > >  
> > >  #include <linux/uaccess.h>
> > >  
> > > @@ -3317,6 +3318,11 @@ struct device *tty_register_device_attr(struct tty_driver *driver,
> > >  	retval = device_register(dev);
> > >  	if (retval)
> > >  		goto error;
> > > +	if (device && device->of_node)
> > > +		/* Children are platform devices and will be
> > > +		 * runtime_pm managed by this tty.
> > > +		 */
> > > +		of_platform_populate(device->of_node, NULL, NULL, dev);
> >
> > Why are these platform devices?
> > And why only OF?
> 
> I just took this patch over from Neil to get bluetooth working
> on N900/N950. Both of them are DT only (well N900 still has
> boardcode, but that will be removed shortly), so it was enough
> for me.
> 
> I guess you have something in mind, that's similar to e.g. i2c
> and spi with a serial_client device and support to instanciate
> it from DT, ACPI and boardcode?

I'm not going to accept a OF-only patch to the tty layer like this,
especially one that abuses platform drivers.

See my response to Pavel for what this "should" look like.

thanks,

greg k-h

Re: [RFC 2/7] tty: add support for "tty slave" devices

[Pavel Machek]

On Sat 2016-08-13 12:03:45, Greg Kroah-Hartman wrote:
> On Sat, Aug 13, 2016 at 05:14:33AM +0200, Sebastian Reichel wrote:
> > From: NeilBrown <neilb@suse.de>
> > 
> > A "tty slave" is a device connected via UART.  It may need a driver to,
> > for example, power the device on when the tty is opened, and power it
> > off when the tty is released.
> > 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > Signed-off-by: Sebastian Reichel <sre@kernel.org>

Acked-by: Pavel Machek <pavel@ucw.cz>

> > @@ -3317,6 +3318,11 @@ struct device *tty_register_device_attr(struct tty_driver *driver,
> >  	retval = device_register(dev);
> >  	if (retval)
> >  		goto error;
> > +	if (device && device->of_node)
> > +		/* Children are platform devices and will be
> > +		 * runtime_pm managed by this tty.
> > +		 */
> > +		of_platform_populate(device->of_node, NULL, NULL, dev);
> 
> Why are these platform devices?  And why only OF?

OF based systems are the only ones that have this problem, so that's
the only place where we can test this solution.

Given that these devices are connected over the UART, it seems right
to categorize them as platform devices... You can't connect PCI, SATA
or USB device over UART port.


									
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC 2/7] tty: add support for "tty slave" devices

[Greg Kroah-Hartman]

On Sun, Aug 14, 2016 at 10:48:22AM +0200, Pavel Machek wrote:
> On Sat 2016-08-13 12:03:45, Greg Kroah-Hartman wrote:
> > On Sat, Aug 13, 2016 at 05:14:33AM +0200, Sebastian Reichel wrote:
> > > From: NeilBrown <neilb@suse.de>
> > > 
> > > A "tty slave" is a device connected via UART.  It may need a driver to,
> > > for example, power the device on when the tty is opened, and power it
> > > off when the tty is released.
> > > 
> > > Signed-off-by: NeilBrown <neilb@suse.de>
> > > Signed-off-by: Sebastian Reichel <sre@kernel.org>
> 
> Acked-by: Pavel Machek <pavel@ucw.cz>
> 
> > > @@ -3317,6 +3318,11 @@ struct device *tty_register_device_attr(struct tty_driver *driver,
> > >  	retval = device_register(dev);
> > >  	if (retval)
> > >  		goto error;
> > > +	if (device && device->of_node)
> > > +		/* Children are platform devices and will be
> > > +		 * runtime_pm managed by this tty.
> > > +		 */
> > > +		of_platform_populate(device->of_node, NULL, NULL, dev);
> > 
> > Why are these platform devices?  And why only OF?
> 
> OF based systems are the only ones that have this problem, so that's
> the only place where we can test this solution.
> 
> Given that these devices are connected over the UART, it seems right
> to categorize them as platform devices... You can't connect PCI, SATA
> or USB device over UART port.

No, that's a total abuse of the platform bus, please don't.

I've said before that a "serial" bus should be created and you can hang
devices off of it.  For some reason that message keeps getting
ignored...

thanks,

greg k-h

Re: [RFC 2/7] tty: add support for "tty slave" devices

[Greg Kroah-Hartman]

On Sun, Aug 14, 2016 at 10:48:22AM +0200, Pavel Machek wrote:
> On Sat 2016-08-13 12:03:45, Greg Kroah-Hartman wrote:
> > On Sat, Aug 13, 2016 at 05:14:33AM +0200, Sebastian Reichel wrote:
> > > From: NeilBrown <neilb-l3A5Bk7waGM@public.gmane.org>
> > > 
> > > A "tty slave" is a device connected via UART.  It may need a driver to,
> > > for example, power the device on when the tty is opened, and power it
> > > off when the tty is released.
> > > 
> > > Signed-off-by: NeilBrown <neilb-l3A5Bk7waGM@public.gmane.org>
> > > Signed-off-by: Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> 
> Acked-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
> 
> > > @@ -3317,6 +3318,11 @@ struct device *tty_register_device_attr(struct tty_driver *driver,
> > >  	retval = device_register(dev);
> > >  	if (retval)
> > >  		goto error;
> > > +	if (device && device->of_node)
> > > +		/* Children are platform devices and will be
> > > +		 * runtime_pm managed by this tty.
> > > +		 */
> > > +		of_platform_populate(device->of_node, NULL, NULL, dev);
> > 
> > Why are these platform devices?  And why only OF?
> 
> OF based systems are the only ones that have this problem, so that's
> the only place where we can test this solution.
> 
> Given that these devices are connected over the UART, it seems right
> to categorize them as platform devices... You can't connect PCI, SATA
> or USB device over UART port.

No, that's a total abuse of the platform bus, please don't.

I've said before that a "serial" bus should be created and you can hang
devices off of it.  For some reason that message keeps getting
ignored...

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC 3/7] dt: bindings: Add nokia-bluetooth

[Sebastian Reichel]

---
 .../devicetree/bindings/net/nokia-bluetooth.txt    | 43 ++++++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/nokia-bluetooth.txt

diff --git a/Documentation/devicetree/bindings/net/nokia-bluetooth.txt b/Documentation/devicetree/bindings/net/nokia-bluetooth.txt
new file mode 100644
index 000000000000..a0fceabb4cce
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/nokia-bluetooth.txt
@@ -0,0 +1,43 @@
+Nokia bluetooth UART devices
+------
+
+Some vendors have custom versions of their chips, that can be found in Nokia
+devices. These chips are controlled differently, than the non-Nokia version,
+so a different binding is required. All chips listed here implement the Nokia
+H4+ protocol.
+
+Required properties:
+
+  - compatible: should be one of the following:
+	* "nokia,brcm,bcm2048"
+	* "nokia,ti,wl1271-bluetooth"
+  - reset-gpios:		Should specify the gpio for bluetooth reset
+  - host-wakeup-gpios:		Should specify the gpio for host wakeup
+  - bluetooth-wakeup-gpios:	Should specify the gpio for bluetooth wakeup
+  - clock-names:		Should be "sysclk"
+  - clocks:			Should contain a clock phandle for system clock
+
+Example:
+
+/ {
+	/* controlled (enabled/disabled) directly by wl1271 */
+	vctcxo: vctcxo {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <38400000>;
+	};
+};
+
+&uart2 {
+	bluetooth {
+		compatible = "nokia,ti,wl1271-bluetooth";
+
+		reset-gpios = <&gpio1 26 GPIO_ACTIVE_HIGH>; /* 26 */
+		host-wakeup-gpios = <&gpio4 5 GPIO_ACTIVE_HIGH>; /* 101 */
+		bluetooth-wakeup-gpios = <&gpio2 5 GPIO_ACTIVE_HIGH>; /* 37 */
+
+		clocks = <&vctcxo>;
+		clock-names = "sysclk";
+	};
+
+};
-- 
2.8.1

Re: [RFC 3/7] dt: bindings: Add nokia-bluetooth

[Rob Herring]

On Sat, Aug 13, 2016 at 05:14:34AM +0200, Sebastian Reichel wrote:
> ---
>  .../devicetree/bindings/net/nokia-bluetooth.txt    | 43 ++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/nokia-bluetooth.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/nokia-bluetooth.txt b/Documentation/devicetree/bindings/net/nokia-bluetooth.txt
> new file mode 100644
> index 000000000000..a0fceabb4cce
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/nokia-bluetooth.txt
> @@ -0,0 +1,43 @@
> +Nokia bluetooth UART devices
> +------
> +
> +Some vendors have custom versions of their chips, that can be found in Nokia
> +devices. These chips are controlled differently, than the non-Nokia version,
> +so a different binding is required. All chips listed here implement the Nokia
> +H4+ protocol.
> +
> +Required properties:
> +
> +  - compatible: should be one of the following:
> +	* "nokia,brcm,bcm2048"
> +	* "nokia,ti,wl1271-bluetooth"

Perhaps these should be 2 separate strings. Something like 
'"nokia,n900-bt", "brcm,bcm2048"'. However, if they are in no way 
compatible with the default version from the vendors, then just a single 
string is fine, but it doesn't need to be aligned to the vendor 
compatible string. So just "nokia,n900-bcm2048" or similar is fine.

> +  - reset-gpios:		Should specify the gpio for bluetooth reset
> +  - host-wakeup-gpios:		Should specify the gpio for host wakeup

Should be interrupt instead?

> +  - bluetooth-wakeup-gpios:	Should specify the gpio for bluetooth wakeup

State direction and active state for gpios.

> +  - clock-names:		Should be "sysclk"
> +  - clocks:			Should contain a clock phandle for system clock
> +
> +Example:
> +
> +/ {
> +	/* controlled (enabled/disabled) directly by wl1271 */
> +	vctcxo: vctcxo {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <38400000>;
> +	};
> +};
> +
> +&uart2 {

I want to see a common serial device binding doc before accepting any 
device bindings. It's not going to say much initially other than devices 
are child nodes of uarts. Perhaps something on baudrate settings.

> +	bluetooth {
> +		compatible = "nokia,ti,wl1271-bluetooth";
> +
> +		reset-gpios = <&gpio1 26 GPIO_ACTIVE_HIGH>; /* 26 */
> +		host-wakeup-gpios = <&gpio4 5 GPIO_ACTIVE_HIGH>; /* 101 */
> +		bluetooth-wakeup-gpios = <&gpio2 5 GPIO_ACTIVE_HIGH>; /* 37 */
> +
> +		clocks = <&vctcxo>;
> +		clock-names = "sysclk";
> +	};
> +
> +};
> -- 
> 2.8.1
> 

Re: [RFC 3/7] dt: bindings: Add nokia-bluetooth

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3406 bytes --]

Hi Rob,

On Tue, Aug 16, 2016 at 08:51:55AM -0500, Rob Herring wrote:
> On Sat, Aug 13, 2016 at 05:14:34AM +0200, Sebastian Reichel wrote:
> > ---
> >  .../devicetree/bindings/net/nokia-bluetooth.txt    | 43 ++++++++++++++++++++++
> >  1 file changed, 43 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/net/nokia-bluetooth.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/net/nokia-bluetooth.txt b/Documentation/devicetree/bindings/net/nokia-bluetooth.txt
> > new file mode 100644
> > index 000000000000..a0fceabb4cce
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/nokia-bluetooth.txt
> > @@ -0,0 +1,43 @@
> > +Nokia bluetooth UART devices
> > +------
> > +
> > +Some vendors have custom versions of their chips, that can be found in Nokia
> > +devices. These chips are controlled differently, than the non-Nokia version,
> > +so a different binding is required. All chips listed here implement the Nokia
> > +H4+ protocol.
> > +
> > +Required properties:
> > +
> > +  - compatible: should be one of the following:
> > +	* "nokia,brcm,bcm2048"
> > +	* "nokia,ti,wl1271-bluetooth"
> 
> Perhaps these should be 2 separate strings. Something like 
> '"nokia,n900-bt", "brcm,bcm2048"'. However, if they are in no way 
> compatible with the default version from the vendors, then just a single 
> string is fine, but it doesn't need to be aligned to the vendor 
> compatible string. So just "nokia,n900-bcm2048" or similar is fine.

The default bcm2048 variant uses different initialization process
and does not use word alignment as far as I know. I think having
"brcm,bcm2048" in the compatible string is wrong.

I guess "brcm,bcm2048-nokia" would also be an option, since the
chip has been built buy broadcom, but it has a custom Nokia
interface.

> > +  - reset-gpios:		Should specify the gpio for bluetooth reset
> > +  - host-wakeup-gpios:		Should specify the gpio for host wakeup
> 
> Should be interrupt instead?

Yes this is mostly an interrupt, but I need to read the current
line state.

> > +  - bluetooth-wakeup-gpios:	Should specify the gpio for bluetooth wakeup
> 
> State direction and active state for gpios.

ok.

> > +  - clock-names:		Should be "sysclk"
> > +  - clocks:			Should contain a clock phandle for system clock
> > +
> > +Example:
> > +
> > +/ {
> > +	/* controlled (enabled/disabled) directly by wl1271 */
> > +	vctcxo: vctcxo {
> > +		compatible = "fixed-clock";
> > +		#clock-cells = <0>;
> > +		clock-frequency = <38400000>;
> > +	};
> > +};
> > +
> > +&uart2 {
> 
> I want to see a common serial device binding doc before accepting any 
> device bindings. It's not going to say much initially other than devices 
> are child nodes of uarts. Perhaps something on baudrate settings.

Neil added a short sentence about this in "[RFC 2/7] tty: add
support for "tty slave" devices". I just took the unmodified patch
from Neil (*), so it's not in its own patch.

> > +	bluetooth {
> > +		compatible = "nokia,ti,wl1271-bluetooth";
> > +
> > +		reset-gpios = <&gpio1 26 GPIO_ACTIVE_HIGH>; /* 26 */
> > +		host-wakeup-gpios = <&gpio4 5 GPIO_ACTIVE_HIGH>; /* 101 */
> > +		bluetooth-wakeup-gpios = <&gpio2 5 GPIO_ACTIVE_HIGH>; /* 37 */
> > +
> > +		clocks = <&vctcxo>;
> > +		clock-names = "sysclk";
> > +	};
> > +
> > +};

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 3/7] dt: bindings: Add nokia-bluetooth

[Rob Herring]

On Tue, Aug 16, 2016 at 6:28 PM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi Rob,
>
> On Tue, Aug 16, 2016 at 08:51:55AM -0500, Rob Herring wrote:
>> On Sat, Aug 13, 2016 at 05:14:34AM +0200, Sebastian Reichel wrote:
>> > ---
>> >  .../devicetree/bindings/net/nokia-bluetooth.txt    | 43 ++++++++++++++++++++++
>> >  1 file changed, 43 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/net/nokia-bluetooth.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/net/nokia-bluetooth.txt b/Documentation/devicetree/bindings/net/nokia-bluetooth.txt
>> > new file mode 100644
>> > index 000000000000..a0fceabb4cce
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/net/nokia-bluetooth.txt
>> > @@ -0,0 +1,43 @@
>> > +Nokia bluetooth UART devices
>> > +------
>> > +
>> > +Some vendors have custom versions of their chips, that can be found in Nokia
>> > +devices. These chips are controlled differently, than the non-Nokia version,
>> > +so a different binding is required. All chips listed here implement the Nokia
>> > +H4+ protocol.
>> > +
>> > +Required properties:
>> > +
>> > +  - compatible: should be one of the following:
>> > +   * "nokia,brcm,bcm2048"
>> > +   * "nokia,ti,wl1271-bluetooth"
>>
>> Perhaps these should be 2 separate strings. Something like
>> '"nokia,n900-bt", "brcm,bcm2048"'. However, if they are in no way
>> compatible with the default version from the vendors, then just a single
>> string is fine, but it doesn't need to be aligned to the vendor
>> compatible string. So just "nokia,n900-bcm2048" or similar is fine.
>
> The default bcm2048 variant uses different initialization process
> and does not use word alignment as far as I know. I think having
> "brcm,bcm2048" in the compatible string is wrong.

Okay.

> I guess "brcm,bcm2048-nokia" would also be an option, since the
> chip has been built buy broadcom, but it has a custom Nokia
> interface.

That would be okay. Though, in theory Nokia could have different
products with bcm2048 all with different versions.

I was going with defining it as a board level compatible string. Even
if chips are the same, s/w can need to know board level differences so
using the board vendor and name are common.

>> > +  - reset-gpios:           Should specify the gpio for bluetooth reset
>> > +  - host-wakeup-gpios:             Should specify the gpio for host wakeup
>>
>> Should be interrupt instead?
>
> Yes this is mostly an interrupt, but I need to read the current
> line state.

When? If the interrupt is level triggered, then you can get the line
state based on whether you get an interrupt or not. If this needs to
be a wakeup source (see the wakeup source binding), then it needs to
be an interrupt.

Reading the line state is a common problem. It would be nice if the
irq API provided a function to read the line state though that is not
always possible.

>
>> > +  - bluetooth-wakeup-gpios:        Should specify the gpio for bluetooth wakeup
>>
>> State direction and active state for gpios.
>
> ok.
>
>> > +  - clock-names:           Should be "sysclk"
>> > +  - clocks:                        Should contain a clock phandle for system clock
>> > +
>> > +Example:
>> > +
>> > +/ {
>> > +   /* controlled (enabled/disabled) directly by wl1271 */
>> > +   vctcxo: vctcxo {
>> > +           compatible = "fixed-clock";
>> > +           #clock-cells = <0>;
>> > +           clock-frequency = <38400000>;
>> > +   };
>> > +};
>> > +
>> > +&uart2 {
>>
>> I want to see a common serial device binding doc before accepting any
>> device bindings. It's not going to say much initially other than devices
>> are child nodes of uarts. Perhaps something on baudrate settings.
>
> Neil added a short sentence about this in "[RFC 2/7] tty: add
> support for "tty slave" devices". I just took the unmodified patch
> from Neil (*), so it's not in its own patch.

But that is in the 8250 binding. It needs to be a common binding.

Rob

Re: [RFC 3/7] dt: bindings: Add nokia-bluetooth

[Rob Herring]

On Tue, Aug 16, 2016 at 6:28 PM, Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> Hi Rob,
>
> On Tue, Aug 16, 2016 at 08:51:55AM -0500, Rob Herring wrote:
>> On Sat, Aug 13, 2016 at 05:14:34AM +0200, Sebastian Reichel wrote:
>> > ---
>> >  .../devicetree/bindings/net/nokia-bluetooth.txt    | 43 ++++++++++++++++++++++
>> >  1 file changed, 43 insertions(+)
>> >  create mode 100644 Documentation/devicetree/bindings/net/nokia-bluetooth.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/net/nokia-bluetooth.txt b/Documentation/devicetree/bindings/net/nokia-bluetooth.txt
>> > new file mode 100644
>> > index 000000000000..a0fceabb4cce
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/net/nokia-bluetooth.txt
>> > @@ -0,0 +1,43 @@
>> > +Nokia bluetooth UART devices
>> > +------
>> > +
>> > +Some vendors have custom versions of their chips, that can be found in Nokia
>> > +devices. These chips are controlled differently, than the non-Nokia version,
>> > +so a different binding is required. All chips listed here implement the Nokia
>> > +H4+ protocol.
>> > +
>> > +Required properties:
>> > +
>> > +  - compatible: should be one of the following:
>> > +   * "nokia,brcm,bcm2048"
>> > +   * "nokia,ti,wl1271-bluetooth"
>>
>> Perhaps these should be 2 separate strings. Something like
>> '"nokia,n900-bt", "brcm,bcm2048"'. However, if they are in no way
>> compatible with the default version from the vendors, then just a single
>> string is fine, but it doesn't need to be aligned to the vendor
>> compatible string. So just "nokia,n900-bcm2048" or similar is fine.
>
> The default bcm2048 variant uses different initialization process
> and does not use word alignment as far as I know. I think having
> "brcm,bcm2048" in the compatible string is wrong.

Okay.

> I guess "brcm,bcm2048-nokia" would also be an option, since the
> chip has been built buy broadcom, but it has a custom Nokia
> interface.

That would be okay. Though, in theory Nokia could have different
products with bcm2048 all with different versions.

I was going with defining it as a board level compatible string. Even
if chips are the same, s/w can need to know board level differences so
using the board vendor and name are common.

>> > +  - reset-gpios:           Should specify the gpio for bluetooth reset
>> > +  - host-wakeup-gpios:             Should specify the gpio for host wakeup
>>
>> Should be interrupt instead?
>
> Yes this is mostly an interrupt, but I need to read the current
> line state.

When? If the interrupt is level triggered, then you can get the line
state based on whether you get an interrupt or not. If this needs to
be a wakeup source (see the wakeup source binding), then it needs to
be an interrupt.

Reading the line state is a common problem. It would be nice if the
irq API provided a function to read the line state though that is not
always possible.

>
>> > +  - bluetooth-wakeup-gpios:        Should specify the gpio for bluetooth wakeup
>>
>> State direction and active state for gpios.
>
> ok.
>
>> > +  - clock-names:           Should be "sysclk"
>> > +  - clocks:                        Should contain a clock phandle for system clock
>> > +
>> > +Example:
>> > +
>> > +/ {
>> > +   /* controlled (enabled/disabled) directly by wl1271 */
>> > +   vctcxo: vctcxo {
>> > +           compatible = "fixed-clock";
>> > +           #clock-cells = <0>;
>> > +           clock-frequency = <38400000>;
>> > +   };
>> > +};
>> > +
>> > +&uart2 {
>>
>> I want to see a common serial device binding doc before accepting any
>> device bindings. It's not going to say much initially other than devices
>> are child nodes of uarts. Perhaps something on baudrate settings.
>
> Neil added a short sentence about this in "[RFC 2/7] tty: add
> support for "tty slave" devices". I just took the unmodified patch
> from Neil (*), so it's not in its own patch.

But that is in the 8250 binding. It needs to be a common binding.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 3/7] dt: bindings: Add nokia-bluetooth

[Pavel Machek]

Hi!

> >> > +  - reset-gpios:           Should specify the gpio for bluetooth reset
> >> > +  - host-wakeup-gpios:             Should specify the gpio for host wakeup
> >>
> >> Should be interrupt instead?
> >
> > Yes this is mostly an interrupt, but I need to read the current
> > line state.
> 
> When? If the interrupt is level triggered, then you can get the line
> state based on whether you get an interrupt or not. If this needs to
> be a wakeup source (see the wakeup source binding), then it needs to
> be an interrupt.

What is wrong with GPIO? It is simply a piece of wire. Yes, we'll
configure it to generate an interrupt, or maybe not, but that's
software detail, the device tree just should not care....
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC 3/7] dt: bindings: Add nokia-bluetooth

[Pavel Machek]

Hi!

> >> > +  - reset-gpios:           Should specify the gpio for bluetooth reset
> >> > +  - host-wakeup-gpios:             Should specify the gpio for host wakeup
> >>
> >> Should be interrupt instead?
> >
> > Yes this is mostly an interrupt, but I need to read the current
> > line state.
> 
> When? If the interrupt is level triggered, then you can get the line
> state based on whether you get an interrupt or not. If this needs to
> be a wakeup source (see the wakeup source binding), then it needs to
> be an interrupt.

What is wrong with GPIO? It is simply a piece of wire. Yes, we'll
configure it to generate an interrupt, or maybe not, but that's
software detail, the device tree just should not care....
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC 4/7] Bluetooth: hci_uart: Add support for word alignment

[Sebastian Reichel]

This will be used by Nokia's H4+ protocol, which
adds padding to packets to reach word alignment.
---
 drivers/bluetooth/hci_h4.c   | 10 ++++++++++
 drivers/bluetooth/hci_uart.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/drivers/bluetooth/hci_h4.c b/drivers/bluetooth/hci_h4.c
index 635597b6e168..a934e4eb692b 100644
--- a/drivers/bluetooth/hci_h4.c
+++ b/drivers/bluetooth/hci_h4.c
@@ -253,11 +253,21 @@ struct sk_buff *h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb,
 			}
 
 			if (!dlen) {
+				if ((&pkts[i])->wordaligned && !(skb->len % 2)) {
+					buffer++;
+					count--;
+				}
+
 				/* No more data, complete frame */
 				(&pkts[i])->recv(hdev, skb);
 				skb = NULL;
 			}
 		} else {
+			if ((&pkts[i])->wordaligned && !(skb->len % 2)) {
+				buffer++;
+				count--;
+			}
+
 			/* Complete frame */
 			(&pkts[i])->recv(hdev, skb);
 			skb = NULL;
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index 839bad1d8152..a7d67aec3632 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -121,6 +121,7 @@ struct h4_recv_pkt {
 	u8  loff;	/* Data length offset in header */
 	u8  lsize;	/* Data length field size */
 	u16 maxlen;	/* Max overall packet length */
+	bool wordaligned;	/* packets are word aligned */
 	int (*recv)(struct hci_dev *hdev, struct sk_buff *skb);
 };
 
-- 
2.8.1

Re: [RFC 4/7] Bluetooth: hci_uart: Add support for word alignment

[Pavel Machek]

On Sat 2016-08-13 05:14:35, Sebastian Reichel wrote:
> This will be used by Nokia's H4+ protocol, which
> adds padding to packets to reach word alignment.

Add a sign-off.

Acked-by: Pavel Machek <pavel@ucw.cz>

Thanks,
							Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC 4/7] Bluetooth: hci_uart: Add support for word alignment

[Pavel Machek]

On Sat 2016-08-13 05:14:35, Sebastian Reichel wrote:
> This will be used by Nokia's H4+ protocol, which
> adds padding to packets to reach word alignment.

Add a sign-off.

Acked-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>

Thanks,
							Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC 4/7] Bluetooth: hci_uart: Add support for word alignment

[Marcel Holtmann]

Hi Sebatian,

> This will be used by Nokia's H4+ protocol, which
> adds padding to packets to reach word alignment.
> ---
> drivers/bluetooth/hci_h4.c   | 10 ++++++++++
> drivers/bluetooth/hci_uart.h |  1 +
> 2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/bluetooth/hci_h4.c b/drivers/bluetooth/hci_h4.c
> index 635597b6e168..a934e4eb692b 100644
> --- a/drivers/bluetooth/hci_h4.c
> +++ b/drivers/bluetooth/hci_h4.c
> @@ -253,11 +253,21 @@ struct sk_buff *h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb,
> 			}
> 
> 			if (!dlen) {
> +				if ((&pkts[i])->wordaligned && !(skb->len % 2)) {
> +					buffer++;
> +					count--;
> +				}
> +
> 				/* No more data, complete frame */
> 				(&pkts[i])->recv(hdev, skb);
> 				skb = NULL;
> 			}
> 		} else {
> +			if ((&pkts[i])->wordaligned && !(skb->len % 2)) {
> +				buffer++;
> +				count--;
> +			}
> +
> 			/* Complete frame */
> 			(&pkts[i])->recv(hdev, skb);
> 			skb = NULL;
> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> index 839bad1d8152..a7d67aec3632 100644
> --- a/drivers/bluetooth/hci_uart.h
> +++ b/drivers/bluetooth/hci_uart.h
> @@ -121,6 +121,7 @@ struct h4_recv_pkt {
> 	u8  loff;	/* Data length offset in header */
> 	u8  lsize;	/* Data length field size */
> 	u16 maxlen;	/* Max overall packet length */
> +	bool wordaligned;	/* packets are word aligned */

I wonder if not a u8 align would be a way better choice here. We set it to 1 for all existing packet types. And the Nokia driver can use 2 here.

> 	int (*recv)(struct hci_dev *hdev, struct sk_buff *skb);

Regards

Marcel

Re: [RFC 4/7] Bluetooth: hci_uart: Add support for word alignment

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1940 bytes --]

Hi Marcel,

On Tue, Aug 16, 2016 at 09:05:19AM +0200, Marcel Holtmann wrote:
> Hi Sebatian,
> 
> > This will be used by Nokia's H4+ protocol, which
> > adds padding to packets to reach word alignment.
> > ---
> > drivers/bluetooth/hci_h4.c   | 10 ++++++++++
> > drivers/bluetooth/hci_uart.h |  1 +
> > 2 files changed, 11 insertions(+)
> > 
> > diff --git a/drivers/bluetooth/hci_h4.c b/drivers/bluetooth/hci_h4.c
> > index 635597b6e168..a934e4eb692b 100644
> > --- a/drivers/bluetooth/hci_h4.c
> > +++ b/drivers/bluetooth/hci_h4.c
> > @@ -253,11 +253,21 @@ struct sk_buff *h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb,
> > 			}
> > 
> > 			if (!dlen) {
> > +				if ((&pkts[i])->wordaligned && !(skb->len % 2)) {
> > +					buffer++;
> > +					count--;
> > +				}
> > +
> > 				/* No more data, complete frame */
> > 				(&pkts[i])->recv(hdev, skb);
> > 				skb = NULL;
> > 			}
> > 		} else {
> > +			if ((&pkts[i])->wordaligned && !(skb->len % 2)) {
> > +				buffer++;
> > +				count--;
> > +			}
> > +
> > 			/* Complete frame */
> > 			(&pkts[i])->recv(hdev, skb);
> > 			skb = NULL;
> > diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> > index 839bad1d8152..a7d67aec3632 100644
> > --- a/drivers/bluetooth/hci_uart.h
> > +++ b/drivers/bluetooth/hci_uart.h
> > @@ -121,6 +121,7 @@ struct h4_recv_pkt {
> > 	u8  loff;	/* Data length offset in header */
> > 	u8  lsize;	/* Data length field size */
> > 	u16 maxlen;	/* Max overall packet length */
> > +	bool wordaligned;	/* packets are word aligned */
> 
> I wonder if not a u8 align would be a way better choice here. We
> set it to 1 for all existing packet types. And the Nokia driver
> can use 2 here.

Sounds less hacky than my approach. Also it made me notice, that my
code is not safe, since the buffer size is not checked. I will use
u8 align and fix the buffer size check.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 4/7] Bluetooth: hci_uart: Add support for word alignment

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1940 bytes --]

Hi Marcel,

On Tue, Aug 16, 2016 at 09:05:19AM +0200, Marcel Holtmann wrote:
> Hi Sebatian,
> 
> > This will be used by Nokia's H4+ protocol, which
> > adds padding to packets to reach word alignment.
> > ---
> > drivers/bluetooth/hci_h4.c   | 10 ++++++++++
> > drivers/bluetooth/hci_uart.h |  1 +
> > 2 files changed, 11 insertions(+)
> > 
> > diff --git a/drivers/bluetooth/hci_h4.c b/drivers/bluetooth/hci_h4.c
> > index 635597b6e168..a934e4eb692b 100644
> > --- a/drivers/bluetooth/hci_h4.c
> > +++ b/drivers/bluetooth/hci_h4.c
> > @@ -253,11 +253,21 @@ struct sk_buff *h4_recv_buf(struct hci_dev *hdev, struct sk_buff *skb,
> > 			}
> > 
> > 			if (!dlen) {
> > +				if ((&pkts[i])->wordaligned && !(skb->len % 2)) {
> > +					buffer++;
> > +					count--;
> > +				}
> > +
> > 				/* No more data, complete frame */
> > 				(&pkts[i])->recv(hdev, skb);
> > 				skb = NULL;
> > 			}
> > 		} else {
> > +			if ((&pkts[i])->wordaligned && !(skb->len % 2)) {
> > +				buffer++;
> > +				count--;
> > +			}
> > +
> > 			/* Complete frame */
> > 			(&pkts[i])->recv(hdev, skb);
> > 			skb = NULL;
> > diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> > index 839bad1d8152..a7d67aec3632 100644
> > --- a/drivers/bluetooth/hci_uart.h
> > +++ b/drivers/bluetooth/hci_uart.h
> > @@ -121,6 +121,7 @@ struct h4_recv_pkt {
> > 	u8  loff;	/* Data length offset in header */
> > 	u8  lsize;	/* Data length field size */
> > 	u16 maxlen;	/* Max overall packet length */
> > +	bool wordaligned;	/* packets are word aligned */
> 
> I wonder if not a u8 align would be a way better choice here. We
> set it to 1 for all existing packet types. And the Nokia driver
> can use 2 here.

Sounds less hacky than my approach. Also it made me notice, that my
code is not safe, since the buffer size is not checked. I will use
u8 align and fix the buffer size check.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

[Sebastian Reichel]

This driver adds support for Nokia H4+ procotol used
for example by Nokia's internet tablets (N770 - N950).
---
 drivers/bluetooth/Kconfig     |  10 +
 drivers/bluetooth/Makefile    |   1 +
 drivers/bluetooth/hci_ldisc.c |   6 +
 drivers/bluetooth/hci_nokia.c | 734 ++++++++++++++++++++++++++++++++++++++++++
 drivers/bluetooth/hci_nokia.h | 140 ++++++++
 drivers/bluetooth/hci_uart.h  |   8 +-
 6 files changed, 898 insertions(+), 1 deletion(-)
 create mode 100644 drivers/bluetooth/hci_nokia.c
 create mode 100644 drivers/bluetooth/hci_nokia.h

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index cf50fd2e96df..c32d9d5ad1d2 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -86,6 +86,16 @@ config BT_HCIUART_H4
 
 	  Say Y here to compile support for HCI UART (H4) protocol.
 
+config BT_HCIUART_NOKIA
+	bool "UART Nokia H4+ protocol support"
+	depends on BT_HCIUART
+	help
+	  Nokia H4+ is serial protocol for communication between Bluetooth
+	  device and host. This protocol is required for Bluetooth devices
+	  with UART interface in Nokia devices.
+
+	  Say Y here to compile support for Nokia's H4+ protocol.
+
 config BT_HCIUART_BCSP
 	bool "BCSP protocol support"
 	depends on BT_HCIUART
diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
index 9c18939fc5c9..f7951646ee14 100644
--- a/drivers/bluetooth/Makefile
+++ b/drivers/bluetooth/Makefile
@@ -37,6 +37,7 @@ hci_uart-$(CONFIG_BT_HCIUART_INTEL)	+= hci_intel.o
 hci_uart-$(CONFIG_BT_HCIUART_BCM)	+= hci_bcm.o
 hci_uart-$(CONFIG_BT_HCIUART_QCA)	+= hci_qca.o
 hci_uart-$(CONFIG_BT_HCIUART_AG6XX)	+= hci_ag6xx.o
+hci_uart-$(CONFIG_BT_HCIUART_NOKIA)	+= hci_nokia.o
 hci_uart-objs				:= $(hci_uart-y)
 
 ccflags-y += -D__CHECK_ENDIAN__
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index dda97398c59a..83d0de94bf35 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -810,6 +810,9 @@ static int __init hci_uart_init(void)
 #ifdef CONFIG_BT_HCIUART_AG6XX
 	ag6xx_init();
 #endif
+#ifdef CONFIG_BT_HCIUART_NOKIA
+	nokia_init();
+#endif
 
 	return 0;
 }
@@ -845,6 +848,9 @@ static void __exit hci_uart_exit(void)
 #ifdef CONFIG_BT_HCIUART_AG6XX
 	ag6xx_deinit();
 #endif
+#ifdef CONFIG_BT_HCIUART_NOKIA
+	nokia_deinit();
+#endif
 
 	/* Release tty registration of line discipline */
 	err = tty_unregister_ldisc(N_HCI);
diff --git a/drivers/bluetooth/hci_nokia.c b/drivers/bluetooth/hci_nokia.c
new file mode 100644
index 000000000000..efd4dd320838
--- /dev/null
+++ b/drivers/bluetooth/hci_nokia.c
@@ -0,0 +1,734 @@
+/*
+ *
+ *  Bluetooth HCI UART H4 driver with Nokia Extensions
+ *
+ *  Copyright (C) 2015 Marcel Holtmann <marcel@holtmann.org>
+ *  Copyright (C) 2016 Sebastian Reichel <sre@kernel.org>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+
+#include <linux/clk.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/fcntl.h>
+#include <linux/interrupt.h>
+#include <linux/ptrace.h>
+#include <linux/poll.h>
+#include <linux/pm_runtime.h>
+#include <linux/firmware.h>
+#include <linux/slab.h>
+#include <linux/tty.h>
+#include <linux/errno.h>
+#include <linux/string.h>
+#include <linux/signal.h>
+#include <linux/ioctl.h>
+#include <linux/skbuff.h>
+#include <linux/delay.h>
+#include <linux/platform_device.h>
+
+#include <linux/gpio/consumer.h>
+
+#include <linux/unaligned/le_struct.h>
+#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci_core.h>
+
+#include "hci_uart.h"
+#include "hci_nokia.h"
+
+struct nokia_uart_dev {
+	struct device *dev;
+	struct tty_port *port;
+	struct gpio_desc *reset;
+	struct gpio_desc *wakeup_host;
+	struct gpio_desc *wakeup_bt;
+	unsigned long sysclk_speed;
+};
+
+struct nokia_bt_dev {
+	struct hci_uart *hu;
+	struct nokia_uart_dev *btdata;
+	int wake_irq;
+	bool wake_state;
+	struct sk_buff *rx_skb;
+	struct sk_buff_head txq;
+	bdaddr_t bdaddr;
+
+	int init_error;
+	struct completion init_completion;
+
+	uint8_t man_id;
+	uint8_t ver_id;
+};
+
+static char *nokia_get_fw_name(struct nokia_bt_dev *btdev)
+{
+	switch (btdev->man_id) {
+	case NOKIA_ID_CSR:
+		return FIRMWARE_CSR;
+	case NOKIA_ID_BCM2048:
+		return FIRMWARE_BCM2048;
+	case NOKIA_ID_TI1271:
+		return FIRMWARE_TI1271;
+	default:
+		return NULL;
+	}
+}
+
+static int hci_uart_wait_for_cts(struct hci_uart *hu, bool state,
+				 int timeout_ms)
+{
+	unsigned long timeout;
+	int signal;
+
+	timeout = jiffies + msecs_to_jiffies(timeout_ms);
+	for (;;) {
+		signal = hu->tty->ops->tiocmget(hu->tty) & TIOCM_CTS;
+		if (!!signal == !!state) {
+			dev_dbg(hu->tty->dev, "wait for cts... received!\n");
+			return 0;
+		}
+		if (time_after(jiffies, timeout)) {
+			dev_dbg(hu->tty->dev, "wait for cts... timeout!\n");
+			return -ETIMEDOUT;
+		}
+		usleep_range(1000, 2000);
+	}
+}
+
+static int btdev_match(struct device *child, void *data)
+{
+	if (!strcmp(child->driver->name, "nokia-bluetooth"))
+		return 1;
+	else
+		return 0;
+}
+
+static irqreturn_t wakeup_handler(int irq, void *data)
+{
+	struct nokia_bt_dev *btdev = data;
+	struct device *serialdev = btdev->hu->tty->dev;
+	int wake_state = gpiod_get_value(btdev->btdata->wakeup_host);
+
+	dev_dbg(serialdev, "wakeup received: %d -> %d\n",
+		btdev->wake_state, wake_state);
+
+	if (btdev->wake_state == wake_state)
+		return IRQ_HANDLED;
+
+	if (wake_state)
+		pm_runtime_get_sync(serialdev);
+	else if (!wake_state)
+		pm_runtime_put(serialdev);
+
+	btdev->wake_state = wake_state;
+
+	return IRQ_HANDLED;
+}
+
+static int nokia_reset(struct hci_uart *hu)
+{
+	struct nokia_bt_dev *btdev = hu->priv;
+	int err;
+
+	/* reset routine */
+	gpiod_set_value_cansleep(btdev->btdata->reset, 0);
+	gpiod_set_value_cansleep(btdev->btdata->wakeup_bt, 1);
+
+	msleep(50);
+
+	/* safety check */
+	err = gpiod_get_value_cansleep(btdev->btdata->wakeup_host);
+	if (err == 1) {
+		dev_err(hu->tty->dev, "reset: host wakeup not low!\n");
+		return -EPROTO;
+	}
+
+	/* flush queues */
+	tty_ldisc_flush(hu->tty);
+	tty_driver_flush_buffer(hu->tty);
+
+	/* init uart */
+	hci_uart_init_tty(hu);
+	hci_uart_set_flow_control(hu, true);
+	hci_uart_set_baudrate(hu, INIT_SPEED);
+
+	gpiod_set_value_cansleep(btdev->btdata->reset, 1);
+	gpiod_set_value_cansleep(btdev->btdata->wakeup_bt, 0);
+
+	msleep(100);
+
+	err = gpiod_get_value_cansleep(btdev->btdata->wakeup_host);
+	if (err == 0) {
+		dev_err(hu->tty->dev, "reset: host wakeup not high!\n");
+		return -EPROTO;
+	}
+
+	/* wait for cts */
+	err = hci_uart_wait_for_cts(hu, true, 100);
+	if (err < 0) {
+		dev_err(hu->tty->dev, "CTS not received: %d\n", err);
+		return err;
+	}
+
+	gpiod_set_value_cansleep(btdev->btdata->wakeup_bt, 1);
+	hci_uart_set_flow_control(hu, false);
+
+	return 0;
+}
+
+static int nokia_send_alive_packet(struct hci_uart *hu)
+{
+	struct nokia_bt_dev *btdev = hu->priv;
+	struct hci_nokia_alive_hdr *hdr;
+	struct hci_nokia_alive_pkt *pkt;
+	struct sk_buff *skb;
+	int len;
+
+	dev_dbg(hu->tty->dev, "Sending alive packet...\n");
+
+	init_completion(&btdev->init_completion);
+
+	len = H4_TYPE_SIZE + sizeof(*hdr) + sizeof(*pkt);
+	skb = bt_skb_alloc(len, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;
+
+	hci_skb_pkt_type(skb) = HCI_NOKIA_ALIVE_PKT;
+	memset(skb->data, 0x00, len);
+
+	hdr = (struct hci_nokia_alive_hdr *)skb_put(skb, sizeof(*hdr));
+	hdr->dlen = sizeof(*pkt);
+	pkt = (struct hci_nokia_alive_pkt *)skb_put(skb, sizeof(*pkt));
+	pkt->mid = NOKIA_ALIVE_REQ;
+
+	hu->hdev->send(hu->hdev, skb);
+
+	if (!wait_for_completion_interruptible_timeout(&btdev->init_completion,
+		msecs_to_jiffies(1000))) {
+		return -ETIMEDOUT;
+	}
+
+	if (btdev->init_error < 0)
+		return btdev->init_error;
+
+	return 0;
+}
+
+static int nokia_send_negotiation(struct hci_uart *hu)
+{
+	struct nokia_bt_dev *btdev = hu->priv;
+	struct hci_nokia_neg_cmd *neg_cmd;
+	struct hci_nokia_neg_hdr *neg_hdr;
+	struct sk_buff *skb;
+	int len, err;
+	u16 baud = DIV_ROUND_CLOSEST(BT_BAUDRATE_DIVIDER, MAX_BAUD_RATE);
+	int sysclk = btdev->btdata->sysclk_speed / 1000;
+
+	dev_dbg(hu->tty->dev, "Sending negotiation...\n");
+
+	len = H4_TYPE_SIZE + sizeof(*neg_hdr) + sizeof(*neg_cmd);
+	skb = bt_skb_alloc(len, GFP_KERNEL);
+	if (!skb)
+		return -ENOMEM;
+
+	hci_skb_pkt_type(skb) = HCI_NOKIA_NEG_PKT;
+
+	neg_hdr = (struct hci_nokia_neg_hdr *)skb_put(skb, sizeof(*neg_hdr));
+	neg_hdr->dlen = sizeof(*neg_cmd);
+
+	neg_cmd = (struct hci_nokia_neg_cmd *)skb_put(skb, sizeof(*neg_cmd));
+	neg_cmd->ack = NOKIA_NEG_REQ;
+	neg_cmd->baud = cpu_to_le16(baud);
+	neg_cmd->unused1 = 0x0000;
+	neg_cmd->proto = NOKIA_PROTO_BYTE;
+	neg_cmd->sys_clk = cpu_to_le16(sysclk);
+	neg_cmd->unused2 = 0x0000;
+
+	btdev->init_error = 0;
+	init_completion(&btdev->init_completion);
+
+	hu->hdev->send(hu->hdev, skb);
+
+	if (!wait_for_completion_interruptible_timeout(&btdev->init_completion,
+		msecs_to_jiffies(10000))) {
+		return -ETIMEDOUT;
+	}
+
+	if (btdev->init_error < 0)
+		return btdev->init_error;
+
+	/* Change to operational settings */
+	hci_uart_set_flow_control(hu, true); // disable flow control
+
+	/* setup negotiated max. baudrate */
+	hci_uart_set_baudrate(hu, MAX_BAUD_RATE);
+
+	err = hci_uart_wait_for_cts(hu, true, 100);
+	if (err < 0)
+		return err;
+
+	hci_uart_set_flow_control(hu, false); // re-enable flow control
+
+	dev_dbg(hu->tty->dev, "Negotiation successful...\n");
+
+	return 0;
+}
+
+static int nokia_setup_fw(struct hci_uart *hu)
+{
+	struct nokia_bt_dev *btdev = hu->priv;
+	const struct firmware *fw;
+	const u8 *fw_ptr;
+	size_t fw_size;
+	int err;
+
+	BT_DBG("hu %p", hu);
+
+	err = request_firmware(&fw, nokia_get_fw_name(btdev), hu->tty->dev);
+	if (err < 0) {
+		BT_ERR("%s: Failed to load Nokia firmware file (%d)",
+		       hu->hdev->name, err);
+		return err;
+	}
+
+	fw_ptr = fw->data;
+	fw_size = fw->size;
+
+	while (fw_size >= 4) {
+		u16 pkt_size = get_unaligned_le16(fw_ptr);
+		u8 pkt_type = fw_ptr[2];
+		const struct hci_command_hdr *cmd;
+		u16 opcode;
+		struct sk_buff *skb;
+
+		switch (pkt_type) {
+		case HCI_COMMAND_PKT:
+			cmd = (struct hci_command_hdr *)(fw_ptr + 3);
+			opcode = le16_to_cpu(cmd->opcode);
+
+			skb = __hci_cmd_sync(hu->hdev, opcode, cmd->plen,
+					     fw_ptr + 3 + HCI_COMMAND_HDR_SIZE,
+					     HCI_INIT_TIMEOUT);
+			if (IS_ERR(skb)) {
+				err = PTR_ERR(skb);
+				BT_ERR("%s: Firmware command %04x failed (%d)",
+				       hu->hdev->name, opcode, err);
+				goto done;
+			}
+			kfree_skb(skb);
+			break;
+		case HCI_NOKIA_RADIO_PKT:
+		case HCI_NOKIA_NEG_PKT:
+		case HCI_NOKIA_ALIVE_PKT:
+			break;
+		}
+
+		fw_ptr += pkt_size + 2;
+		fw_size -= pkt_size + 2;
+	}
+
+done:
+	release_firmware(fw);
+	return err;
+}
+
+static int nokia_setup(struct hci_uart *hu)
+{
+	int err;
+
+	pm_runtime_get_sync(hu->tty->dev);
+
+	dev_dbg(hu->tty->dev, "Nokia H4+ protocol setup...\n");
+
+	/* 0. reset connection */
+	err = nokia_reset(hu);
+	if (err < 0) {
+		dev_err(hu->tty->dev, "Reset failed: %d\n", err);
+		goto out;
+	}
+
+	/* 1. negotiate speed etc */
+	err = nokia_send_negotiation(hu);
+	if (err < 0) {
+		dev_err(hu->tty->dev, "Negotiation failed: %d\n", err);
+		goto out;
+	}
+
+	/* 2. verify correct setup using alive packet */
+	err = nokia_send_alive_packet(hu);
+	if (err < 0) {
+		dev_err(hu->tty->dev, "Alive check failed: %d\n", err);
+		goto out;
+	}
+
+	/* 3. send firmware */
+	err = nokia_setup_fw(hu);
+	if (err < 0) {
+		dev_err(hu->tty->dev, "Could not setup FW: %d\n", err);
+		goto out;
+	}
+
+	hci_uart_set_flow_control(hu, true);
+	hci_uart_set_baudrate(hu, BC4_MAX_BAUD_RATE);
+	hci_uart_set_flow_control(hu, false);
+
+	dev_dbg(hu->tty->dev, "Nokia H4+ protocol setup done!\n");
+
+	/*
+	 * TODO:
+	 * disable wakeup_bt at this point and automatically enable it when
+	 * data is about to be written until all data has been written (+ some
+	 * delay).
+	 *
+	 * Since this is not yet support by the uart/tty kernel framework we
+	 * will always keep enabled the wakeup_bt gpio for now, so that the
+	 * bluetooth chip will never transit into idle modes.
+	 */
+
+out:
+	pm_runtime_put(hu->tty->dev);
+
+	return err;
+}
+
+static int nokia_open(struct hci_uart *hu)
+{
+	struct device *serialdev = hu->tty->dev;
+	struct nokia_bt_dev *btdev;
+	struct device *uartbtdev;
+	int err;
+
+	btdev = kzalloc(sizeof(*btdev), GFP_KERNEL);
+	if (!btdev)
+		return -ENOMEM;
+
+	btdev->hu = hu;
+
+	skb_queue_head_init(&btdev->txq);
+
+	uartbtdev = device_find_child(serialdev, NULL, btdev_match);
+	if (!uartbtdev) {
+		dev_err(serialdev, "bluetooth device node not found!\n");
+		return -ENODEV;
+	}
+
+	btdev->btdata = dev_get_drvdata(uartbtdev);
+	if (!btdev->btdata)
+		return -EINVAL;
+
+	hu->priv = btdev;
+
+	/* register handler for host wakeup gpio */
+	btdev->wake_irq = gpiod_to_irq(btdev->btdata->wakeup_host);
+	err = request_threaded_irq(btdev->wake_irq, NULL, wakeup_handler,
+		IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+		"wakeup", btdev);
+	if (err) {
+		gpiod_set_value(btdev->btdata->reset, 0);
+		gpiod_set_value(btdev->btdata->wakeup_bt, 0);
+		return err;
+	}
+
+	dev_dbg(serialdev, "Nokia H4+ protocol initialized with %s!\n",
+		dev_name(uartbtdev));
+
+	pm_runtime_enable(hu->tty->dev);
+
+	return 0;
+}
+
+static int nokia_flush(struct hci_uart *hu)
+{
+	struct nokia_bt_dev *btdev = hu->priv;
+
+	BT_DBG("hu %p", hu);
+
+	skb_queue_purge(&btdev->txq);
+
+	return 0;
+}
+
+static int nokia_close(struct hci_uart *hu)
+{
+	struct nokia_bt_dev *btdev = hu->priv;
+
+	hu->priv = NULL;
+
+	BT_DBG("hu %p", hu);
+
+	skb_queue_purge(&btdev->txq);
+
+	kfree_skb(btdev->rx_skb);
+
+	free_irq(btdev->wake_irq, btdev);
+
+	/* disable module */
+	gpiod_set_value(btdev->btdata->reset, 0);
+	gpiod_set_value(btdev->btdata->wakeup_bt, 0);
+
+	hu->priv = NULL;
+	kfree(btdev);
+
+	pm_runtime_disable(hu->tty->dev);
+
+	return 0;
+}
+
+/* Enqueue frame for transmittion (padding, crc, etc) */
+static int nokia_enqueue(struct hci_uart *hu, struct sk_buff *skb)
+{
+	struct nokia_bt_dev *btdev = hu->priv;
+	int err;
+
+	BT_DBG("hu %p skb %p", hu, skb);
+
+	/* Prepend skb with frame type */
+	memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
+
+	/* Packets must be word aligned */
+	if (skb->len % 2) {
+		err = skb_pad(skb, 1);
+		if (err)
+			return err;
+		*skb_put(skb, 1) = 0x00;
+	}
+
+	skb_queue_tail(&btdev->txq, skb);
+
+	return 0;
+}
+
+static int nokia_recv_negotiation_packet(struct hci_dev *hdev,
+					 struct sk_buff *skb)
+{
+	struct hci_uart *hu = hci_get_drvdata(hdev);
+	struct nokia_bt_dev *btdev = hu->priv;
+	struct hci_nokia_neg_hdr *hdr;
+	struct hci_nokia_neg_evt *evt;
+	int ret = 0;
+
+	hdr = (struct hci_nokia_neg_hdr *)skb->data;
+	if (hdr->dlen != sizeof(*evt)) {
+		btdev->init_error = -EIO;
+		ret = -EIO;
+		goto finish_neg;
+	}
+
+	evt = (struct hci_nokia_neg_evt *)skb_pull(skb, sizeof(*hdr));
+
+	if (evt->ack != NOKIA_NEG_ACK) {
+		dev_err(hu->tty->dev, "Could not negotiate hci_nokia settings\n");
+		btdev->init_error = -EINVAL;
+	}
+
+	btdev->man_id = evt->man_id;
+	btdev->ver_id = evt->ver_id;
+
+	dev_dbg(hu->tty->dev, "NOKIA negotiation:\n");
+	dev_dbg(hu->tty->dev, "\tbaudrate = %u\n", evt->baud);
+	dev_dbg(hu->tty->dev, "\tsystem clock = %u\n", evt->sys_clk);
+	dev_dbg(hu->tty->dev, "\tmanufacturer id = %u\n", evt->man_id);
+	dev_dbg(hu->tty->dev, "\tversion id = %u\n", evt->ver_id);
+
+finish_neg:
+	complete(&btdev->init_completion);
+	kfree_skb(skb);
+	return ret;
+}
+
+static int nokia_recv_alive_packet(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	struct hci_uart *hu = hci_get_drvdata(hdev);
+	struct nokia_bt_dev *btdev = hu->priv;
+	struct hci_nokia_alive_hdr *hdr;
+	struct hci_nokia_alive_pkt *pkt;
+	int ret = 0;
+
+	hdr = (struct hci_nokia_alive_hdr *)skb->data;
+	if (hdr->dlen != sizeof(*pkt)) {
+		dev_err(hu->tty->dev, "Corrupted alive message\n");
+		btdev->init_error = -EIO;
+		ret = -EIO;
+		goto finish_alive;
+	}
+
+	pkt = (struct hci_nokia_alive_pkt *)skb_pull(skb, sizeof(*hdr));
+
+	if (pkt->mid != NOKIA_ALIVE_RESP) {
+		dev_err(hu->tty->dev, "Invalid alive response: 0x%02x!\n",
+			pkt->mid);
+		btdev->init_error = -EINVAL;
+		goto finish_alive;
+	}
+
+	dev_dbg(hu->tty->dev, "Received alive packet!\n");
+
+finish_alive:
+	complete(&btdev->init_completion);
+	kfree_skb(skb);
+	return ret;
+}
+
+static int nokia_recv_radio(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	/* Packets received on the dedicated radio channel are
+	 * HCI events and so feed them back into the core.
+	 */
+	bt_cb(skb)->pkt_type = HCI_EVENT_PKT;
+	return hci_recv_frame(hdev, skb);
+}
+
+/* Recv data */
+static const struct h4_recv_pkt nokia_recv_pkts[] = {
+	{ NOKIA_RECV_ACL,	.recv = hci_recv_frame },
+	{ NOKIA_RECV_SCO,	.recv = hci_recv_frame },
+	{ NOKIA_RECV_EVENT,	.recv = hci_recv_frame },
+	{ NOKIA_RECV_ALIVE,	.recv = nokia_recv_alive_packet },
+	{ NOKIA_RECV_NEG,	.recv = nokia_recv_negotiation_packet },
+	{ NOKIA_RECV_RADIO,	.recv = nokia_recv_radio },
+};
+
+static int nokia_recv(struct hci_uart *hu, const void *data, int count)
+{
+	struct nokia_bt_dev *btdev = hu->priv;
+	int err;
+
+	if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
+		return -EUNATCH;
+
+	btdev->rx_skb = h4_recv_buf(hu->hdev, btdev->rx_skb, data, count,
+				  nokia_recv_pkts, ARRAY_SIZE(nokia_recv_pkts));
+	if (IS_ERR(btdev->rx_skb)) {
+		err = PTR_ERR(btdev->rx_skb);
+		BT_ERR("%s: Frame reassembly failed (%d)", hu->hdev->name, err);
+		btdev->rx_skb = NULL;
+		return err;
+	}
+
+	return count;
+}
+
+static struct sk_buff *nokia_dequeue(struct hci_uart *hu)
+{
+	struct nokia_bt_dev *btdev = hu->priv;
+
+	return skb_dequeue(&btdev->txq);
+}
+
+static const struct hci_uart_proto nokia_proto = {
+	.id		= HCI_UART_NOKIA,
+	.name		= "Nokia",
+	.open		= nokia_open,
+	.close		= nokia_close,
+	.recv		= nokia_recv,
+	.enqueue	= nokia_enqueue,
+	.dequeue	= nokia_dequeue,
+	.flush		= nokia_flush,
+	.setup		= nokia_setup,
+};
+
+static int nokia_bluetooth_probe(struct platform_device *pdev)
+{
+	struct nokia_uart_dev *btdata;
+	struct device *bcmdev = &pdev->dev;
+	struct clk *sysclk;
+	int err = 0;
+
+	if(!bcmdev->parent) {
+		dev_err(bcmdev, "parent device missing!\n");
+		return -ENODEV;
+	}
+
+	btdata = devm_kmalloc(bcmdev, sizeof(*btdata), GFP_KERNEL);
+	if(!btdata)
+		return -ENOMEM;
+
+	btdata->dev = bcmdev;
+	dev_set_drvdata(bcmdev, btdata);
+
+	btdata->port = dev_get_drvdata(bcmdev->parent);
+	if(!btdata->port) {
+		dev_err(bcmdev, "port data missing in parent device!\n");
+		return -ENODEV;
+	}
+
+	btdata->reset = devm_gpiod_get(bcmdev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(btdata->reset)) {
+		err = PTR_ERR(btdata->reset);
+		dev_err(bcmdev, "could not get reset gpio: %d\n", err);
+		return err;
+	}
+
+	btdata->wakeup_host = devm_gpiod_get(bcmdev, "host-wakeup", GPIOD_IN);
+	if (IS_ERR(btdata->wakeup_host)) {
+		err = PTR_ERR(btdata->wakeup_host);
+		dev_err(bcmdev, "could not get host wakeup gpio: %d\n", err);
+		return err;
+	}
+
+
+	btdata->wakeup_bt = devm_gpiod_get(bcmdev, "bluetooth-wakeup",
+					    GPIOD_OUT_LOW);
+	if (IS_ERR(btdata->wakeup_bt)) {
+		err = PTR_ERR(btdata->wakeup_bt);
+		dev_err(bcmdev, "could not get BT wakeup gpio: %d\n", err);
+		return err;
+	}
+
+	sysclk = devm_clk_get(bcmdev, "sysclk");
+	if (IS_ERR(sysclk)) {
+		err = PTR_ERR(sysclk);
+		dev_err(bcmdev, "could not get sysclk: %d\n", err);
+		return err;
+	}
+
+	clk_prepare_enable(sysclk);
+	btdata->sysclk_speed = clk_get_rate(sysclk);
+	clk_disable_unprepare(sysclk);
+
+	dev_dbg(bcmdev, "parent uart: %s\n", dev_name(bcmdev->parent));
+	dev_dbg(bcmdev, "sysclk speed: %ld kHz\n", btdata->sysclk_speed / 1000);
+
+	/* TODO: open tty and setup line disector from kernel-side */
+
+	return err;
+}
+
+static const struct of_device_id nokia_bluetooth_of_match[] = {
+	{ .compatible = "nokia,brcm,bcm2048", },
+	{ .compatible = "nokia,ti,wl1271-bluetooth", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, nokia_bluetooth_of_match);
+
+static struct platform_driver platform_nokia_driver = {
+	.driver = {
+		.name = "nokia-bluetooth",
+		.of_match_table = nokia_bluetooth_of_match,
+	},
+	.probe = nokia_bluetooth_probe,
+};
+
+int __init nokia_init(void)
+{
+	platform_driver_register(&platform_nokia_driver);
+	return hci_uart_register_proto(&nokia_proto);
+}
+
+int __exit nokia_deinit(void)
+{
+	platform_driver_unregister(&platform_nokia_driver);
+	return hci_uart_unregister_proto(&nokia_proto);
+}
diff --git a/drivers/bluetooth/hci_nokia.h b/drivers/bluetooth/hci_nokia.h
new file mode 100644
index 000000000000..8c4d307840e5
--- /dev/null
+++ b/drivers/bluetooth/hci_nokia.h
@@ -0,0 +1,140 @@
+/*
+ *  Copyright (C) 2016 Sebastian Reichel <sre@kernel.org>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ */
+
+#ifndef __HCI_NOKIA_H
+#define __HCI_NOKIA_H
+
+#define NOKIA_ID_CSR		0x02
+#define NOKIA_ID_BCM2048	0x04
+#define NOKIA_ID_TI1271		0x31
+
+#define FIRMWARE_CSR		"nokia/bc4fw.bin"
+#define FIRMWARE_BCM2048	"nokia/bcmfw.bin"
+#define FIRMWARE_TI1271		"nokia/ti1273.bin"
+
+#define NOKIA_BCM_BDADDR	0xfc01
+
+#define HCI_NOKIA_NEG_PKT	0x06
+#define HCI_NOKIA_ALIVE_PKT	0x07
+#define HCI_NOKIA_RADIO_PKT	0x08
+
+#define HCI_NOKIA_NEG_HDR_SIZE		1
+#define HCI_NOKIA_MAX_NEG_SIZE		255
+#define HCI_NOKIA_ALIVE_HDR_SIZE	1
+#define HCI_NOKIA_MAX_ALIVE_SIZE	255
+#define HCI_NOKIA_RADIO_HDR_SIZE	2
+#define HCI_NOKIA_MAX_RADIO_SIZE	255
+
+#define NOKIA_PROTO_PKT		0x44
+#define NOKIA_PROTO_BYTE	0x4c
+
+#define NOKIA_NEG_REQ		0x00
+#define NOKIA_NEG_ACK		0x20
+#define NOKIA_NEG_NAK		0x40
+
+#define H4_TYPE_SIZE		1
+
+#define NOKIA_RECV_ACL \
+	H4_RECV_ACL, \
+	.wordaligned = true
+
+#define NOKIA_RECV_SCO \
+	H4_RECV_SCO, \
+	.wordaligned = true
+
+#define NOKIA_RECV_EVENT \
+	H4_RECV_EVENT, \
+	.wordaligned = true
+
+#define NOKIA_RECV_ALIVE \
+	.type = HCI_NOKIA_ALIVE_PKT, \
+	.hlen = HCI_NOKIA_ALIVE_HDR_SIZE, \
+	.loff = 0, \
+	.lsize = 1, \
+	.maxlen = HCI_NOKIA_MAX_ALIVE_SIZE, \
+	.wordaligned = true
+
+#define NOKIA_RECV_NEG \
+	.type = HCI_NOKIA_NEG_PKT, \
+	.hlen = HCI_NOKIA_NEG_HDR_SIZE, \
+	.loff = 0, \
+	.lsize = 1, \
+	.maxlen = HCI_NOKIA_MAX_NEG_SIZE, \
+	.wordaligned = true
+
+#define NOKIA_RECV_RADIO \
+	.type = HCI_NOKIA_RADIO_PKT, \
+	.hlen = HCI_NOKIA_RADIO_HDR_SIZE, \
+	.loff = 1, \
+	.lsize = 1, \
+	.maxlen = HCI_NOKIA_MAX_RADIO_SIZE, \
+	.wordaligned = true
+
+struct hci_nokia_neg_hdr {
+	__u8	dlen;
+} __packed;
+
+struct hci_nokia_neg_cmd {
+	__u8	ack;
+	__u16	baud;
+	__u16	unused1;
+	__u8	proto;
+	__u16	sys_clk;
+	__u16	unused2;
+} __packed;
+
+static inline struct hci_nokia_neg_hdr *hci_nokia_neg_hdr(const struct sk_buff *skb)
+{
+	return (struct hci_nokia_neg_hdr *) skb->data;
+}
+
+#define NOKIA_ALIVE_REQ   0x55
+#define NOKIA_ALIVE_RESP  0xcc
+
+struct hci_nokia_alive_hdr {
+	__u8	dlen;
+} __packed;
+
+struct hci_nokia_alive_pkt {
+	__u8	mid;
+	__u8	unused;
+} __packed;
+
+static inline struct hci_nokia_alive_hdr *hci_nokia_alive_hdr(const struct sk_buff *skb)
+{
+	return (struct hci_nokia_alive_hdr *) skb->data;
+}
+
+struct hci_nokia_neg_evt {
+	__u8	ack;
+	__u16	baud;
+	__u16	unused1;
+	__u8	proto;
+	__u16	sys_clk;
+	__u16	unused2;
+	__u8	man_id;
+	__u8	ver_id;
+} __packed;
+
+#define BT_BAUDRATE_DIVIDER     384000000
+#define BC4_MAX_BAUD_RATE       3692300
+#define MAX_BAUD_RATE           921600
+#define INIT_SPEED              120000
+
+struct hci_nokia_radio_hdr {
+	__u8	evt;
+	__u8	dlen;
+} __packed;
+
+#endif
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index a7d67aec3632..314b243df996 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -35,7 +35,7 @@
 #define HCIUARTGETFLAGS		_IOR('U', 204, int)
 
 /* UART protocols */
-#define HCI_UART_MAX_PROTO	10
+#define HCI_UART_MAX_PROTO	11
 
 #define HCI_UART_H4	0
 #define HCI_UART_BCSP	1
@@ -47,6 +47,7 @@
 #define HCI_UART_BCM	7
 #define HCI_UART_QCA	8
 #define HCI_UART_AG6XX	9
+#define HCI_UART_NOKIA	10
 
 #define HCI_UART_RAW_DEVICE	0
 #define HCI_UART_RESET_ON_INIT	1
@@ -190,3 +191,8 @@ int qca_deinit(void);
 int ag6xx_init(void);
 int ag6xx_deinit(void);
 #endif
+
+#ifdef CONFIG_BT_HCIUART_NOKIA
+int nokia_init(void);
+int nokia_deinit(void);
+#endif
-- 
2.8.1

Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

[Paul Gortmaker]

On Fri, Aug 12, 2016 at 11:14 PM, Sebastian Reichel <sre@kernel.org> wrote:
> This driver adds support for Nokia H4+ procotol used
> for example by Nokia's internet tablets (N770 - N950).
> ---
>  drivers/bluetooth/Kconfig     |  10 +
>  drivers/bluetooth/Makefile    |   1 +
>  drivers/bluetooth/hci_ldisc.c |   6 +
>  drivers/bluetooth/hci_nokia.c | 734 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/bluetooth/hci_nokia.h | 140 ++++++++
>  drivers/bluetooth/hci_uart.h  |   8 +-
>  6 files changed, 898 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/bluetooth/hci_nokia.c
>  create mode 100644 drivers/bluetooth/hci_nokia.h
>
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index cf50fd2e96df..c32d9d5ad1d2 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -86,6 +86,16 @@ config BT_HCIUART_H4
>
>           Say Y here to compile support for HCI UART (H4) protocol.
>
> +config BT_HCIUART_NOKIA
> +       bool "UART Nokia H4+ protocol support"

If the option is a bool, then you don't need module.h or any MODULE
macros, nor do you need any __exit or unregister related code.

Alternatively, if there is a use case for it to be modular, then maybe you
want to change the above to a tristate.

Paul.
--

> +       depends on BT_HCIUART
> +       help
> +         Nokia H4+ is serial protocol for communication between Bluetooth
> +         device and host. This protocol is required for Bluetooth devices
> +         with UART interface in Nokia devices.
> +
> +         Say Y here to compile support for Nokia's H4+ protocol.
> +
>  config BT_HCIUART_BCSP
>         bool "BCSP protocol support"
>         depends on BT_HCIUART


[...]

> +       { .compatible = "nokia,ti,wl1271-bluetooth", },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, nokia_bluetooth_of_match);
> +
> +static struct platform_driver platform_nokia_driver = {
> +       .driver = {
> +               .name = "nokia-bluetooth",
> +               .of_match_table = nokia_bluetooth_of_match,
> +       },
> +       .probe = nokia_bluetooth_probe,
> +};
> +
> +int __init nokia_init(void)
> +{
> +       platform_driver_register(&platform_nokia_driver);
> +       return hci_uart_register_proto(&nokia_proto);
> +}
> +
> +int __exit nokia_deinit(void)
> +{
> +       platform_driver_unregister(&platform_nokia_driver);
> +       return hci_uart_unregister_proto(&nokia_proto);
> +}

Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

[Paul Gortmaker]

On Fri, Aug 12, 2016 at 11:14 PM, Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> This driver adds support for Nokia H4+ procotol used
> for example by Nokia's internet tablets (N770 - N950).
> ---
>  drivers/bluetooth/Kconfig     |  10 +
>  drivers/bluetooth/Makefile    |   1 +
>  drivers/bluetooth/hci_ldisc.c |   6 +
>  drivers/bluetooth/hci_nokia.c | 734 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/bluetooth/hci_nokia.h | 140 ++++++++
>  drivers/bluetooth/hci_uart.h  |   8 +-
>  6 files changed, 898 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/bluetooth/hci_nokia.c
>  create mode 100644 drivers/bluetooth/hci_nokia.h
>
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index cf50fd2e96df..c32d9d5ad1d2 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -86,6 +86,16 @@ config BT_HCIUART_H4
>
>           Say Y here to compile support for HCI UART (H4) protocol.
>
> +config BT_HCIUART_NOKIA
> +       bool "UART Nokia H4+ protocol support"

If the option is a bool, then you don't need module.h or any MODULE
macros, nor do you need any __exit or unregister related code.

Alternatively, if there is a use case for it to be modular, then maybe you
want to change the above to a tristate.

Paul.
--

> +       depends on BT_HCIUART
> +       help
> +         Nokia H4+ is serial protocol for communication between Bluetooth
> +         device and host. This protocol is required for Bluetooth devices
> +         with UART interface in Nokia devices.
> +
> +         Say Y here to compile support for Nokia's H4+ protocol.
> +
>  config BT_HCIUART_BCSP
>         bool "BCSP protocol support"
>         depends on BT_HCIUART


[...]

> +       { .compatible = "nokia,ti,wl1271-bluetooth", },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, nokia_bluetooth_of_match);
> +
> +static struct platform_driver platform_nokia_driver = {
> +       .driver = {
> +               .name = "nokia-bluetooth",
> +               .of_match_table = nokia_bluetooth_of_match,
> +       },
> +       .probe = nokia_bluetooth_probe,
> +};
> +
> +int __init nokia_init(void)
> +{
> +       platform_driver_register(&platform_nokia_driver);
> +       return hci_uart_register_proto(&nokia_proto);
> +}
> +
> +int __exit nokia_deinit(void)
> +{
> +       platform_driver_unregister(&platform_nokia_driver);
> +       return hci_uart_unregister_proto(&nokia_proto);
> +}
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 779 bytes --]

Hi Paul,

On Sun, Aug 14, 2016 at 07:54:28PM -0400, Paul Gortmaker wrote:
> On Fri, Aug 12, 2016 at 11:14 PM, Sebastian Reichel <sre@kernel.org> wrote:
> > This driver adds support for Nokia H4+ procotol used
> > for example by Nokia's internet tablets (N770 - N950).
> >
> > [...]
> >
> > +config BT_HCIUART_NOKIA
> > +       bool "UART Nokia H4+ protocol support"
> 
> If the option is a bool, then you don't need module.h or any MODULE
> macros, nor do you need any __exit or unregister related code.
> 
> Alternatively, if there is a use case for it to be modular, then maybe you
> want to change the above to a tristate.

Actually it is bool and modular: It becomes part of the module
configured by the tristate CONFIG_BT_HCIUART option.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 808 bytes --]

Hi Paul,

On Sun, Aug 14, 2016 at 07:54:28PM -0400, Paul Gortmaker wrote:
> On Fri, Aug 12, 2016 at 11:14 PM, Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > This driver adds support for Nokia H4+ procotol used
> > for example by Nokia's internet tablets (N770 - N950).
> >
> > [...]
> >
> > +config BT_HCIUART_NOKIA
> > +       bool "UART Nokia H4+ protocol support"
> 
> If the option is a bool, then you don't need module.h or any MODULE
> macros, nor do you need any __exit or unregister related code.
> 
> Alternatively, if there is a use case for it to be modular, then maybe you
> want to change the above to a tristate.

Actually it is bool and modular: It becomes part of the module
configured by the tristate CONFIG_BT_HCIUART option.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

[Marcel Holtmann]

Hi Sebastian,

> This driver adds support for Nokia H4+ procotol used
> for example by Nokia's internet tablets (N770 - N950).
> ---
> drivers/bluetooth/Kconfig     |  10 +
> drivers/bluetooth/Makefile    |   1 +
> drivers/bluetooth/hci_ldisc.c |   6 +
> drivers/bluetooth/hci_nokia.c | 734 ++++++++++++++++++++++++++++++++++++++++++
> drivers/bluetooth/hci_nokia.h | 140 ++++++++
> drivers/bluetooth/hci_uart.h  |   8 +-
> 6 files changed, 898 insertions(+), 1 deletion(-)
> create mode 100644 drivers/bluetooth/hci_nokia.c
> create mode 100644 drivers/bluetooth/hci_nokia.h
> 
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index cf50fd2e96df..c32d9d5ad1d2 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -86,6 +86,16 @@ config BT_HCIUART_H4
> 
> 	  Say Y here to compile support for HCI UART (H4) protocol.
> 
> +config BT_HCIUART_NOKIA
> +	bool "UART Nokia H4+ protocol support"
> +	depends on BT_HCIUART
> +	help
> +	  Nokia H4+ is serial protocol for communication between Bluetooth
> +	  device and host. This protocol is required for Bluetooth devices
> +	  with UART interface in Nokia devices.
> +
> +	  Say Y here to compile support for Nokia's H4+ protocol.
> +
> config BT_HCIUART_BCSP
> 	bool "BCSP protocol support"
> 	depends on BT_HCIUART
> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
> index 9c18939fc5c9..f7951646ee14 100644
> --- a/drivers/bluetooth/Makefile
> +++ b/drivers/bluetooth/Makefile
> @@ -37,6 +37,7 @@ hci_uart-$(CONFIG_BT_HCIUART_INTEL)	+= hci_intel.o
> hci_uart-$(CONFIG_BT_HCIUART_BCM)	+= hci_bcm.o
> hci_uart-$(CONFIG_BT_HCIUART_QCA)	+= hci_qca.o
> hci_uart-$(CONFIG_BT_HCIUART_AG6XX)	+= hci_ag6xx.o
> +hci_uart-$(CONFIG_BT_HCIUART_NOKIA)	+= hci_nokia.o
> hci_uart-objs				:= $(hci_uart-y)
> 
> ccflags-y += -D__CHECK_ENDIAN__
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index dda97398c59a..83d0de94bf35 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -810,6 +810,9 @@ static int __init hci_uart_init(void)
> #ifdef CONFIG_BT_HCIUART_AG6XX
> 	ag6xx_init();
> #endif
> +#ifdef CONFIG_BT_HCIUART_NOKIA
> +	nokia_init();
> +#endif
> 
> 	return 0;
> }
> @@ -845,6 +848,9 @@ static void __exit hci_uart_exit(void)
> #ifdef CONFIG_BT_HCIUART_AG6XX
> 	ag6xx_deinit();
> #endif
> +#ifdef CONFIG_BT_HCIUART_NOKIA
> +	nokia_deinit();
> +#endif
> 
> 	/* Release tty registration of line discipline */
> 	err = tty_unregister_ldisc(N_HCI);
> diff --git a/drivers/bluetooth/hci_nokia.c b/drivers/bluetooth/hci_nokia.c
> new file mode 100644
> index 000000000000..efd4dd320838
> --- /dev/null
> +++ b/drivers/bluetooth/hci_nokia.c
> @@ -0,0 +1,734 @@
> +/*
> + *
> + *  Bluetooth HCI UART H4 driver with Nokia Extensions
> + *
> + *  Copyright (C) 2015 Marcel Holtmann <marcel@holtmann.org>
> + *  Copyright (C) 2016 Sebastian Reichel <sre@kernel.org>
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + */
> +
> +#include <linux/module.h>
> +
> +#include <linux/clk.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/types.h>
> +#include <linux/fcntl.h>
> +#include <linux/interrupt.h>
> +#include <linux/ptrace.h>
> +#include <linux/poll.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/firmware.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <linux/errno.h>
> +#include <linux/string.h>
> +#include <linux/signal.h>
> +#include <linux/ioctl.h>
> +#include <linux/skbuff.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +
> +#include <linux/gpio/consumer.h>
> +
> +#include <linux/unaligned/le_struct.h>

are you sure all these includes are needed?

> +#include <net/bluetooth/bluetooth.h>
> +#include <net/bluetooth/hci_core.h>
> +
> +#include "hci_uart.h"
> +#include "hci_nokia.h"
> +
> +struct nokia_uart_dev {
> +	struct device *dev;
> +	struct tty_port *port;
> +	struct gpio_desc *reset;
> +	struct gpio_desc *wakeup_host;
> +	struct gpio_desc *wakeup_bt;
> +	unsigned long sysclk_speed;
> +};
> +
> +struct nokia_bt_dev {
> +	struct hci_uart *hu;
> +	struct nokia_uart_dev *btdata;
> +	int wake_irq;
> +	bool wake_state;
> +	struct sk_buff *rx_skb;
> +	struct sk_buff_head txq;
> +	bdaddr_t bdaddr;
> +
> +	int init_error;
> +	struct completion init_completion;
> +
> +	uint8_t man_id;
> +	uint8_t ver_id;
> +};
> +
> +static char *nokia_get_fw_name(struct nokia_bt_dev *btdev)
> +{
> +	switch (btdev->man_id) {
> +	case NOKIA_ID_CSR:
> +		return FIRMWARE_CSR;
> +	case NOKIA_ID_BCM2048:
> +		return FIRMWARE_BCM2048;
> +	case NOKIA_ID_TI1271:
> +		return FIRMWARE_TI1271;
> +	default:
> +		return NULL;
> +	}
> +}
> +
> +static int hci_uart_wait_for_cts(struct hci_uart *hu, bool state,
> +				 int timeout_ms)
> +{
> +	unsigned long timeout;
> +	int signal;
> +
> +	timeout = jiffies + msecs_to_jiffies(timeout_ms);
> +	for (;;) {
> +		signal = hu->tty->ops->tiocmget(hu->tty) & TIOCM_CTS;
> +		if (!!signal == !!state) {
> +			dev_dbg(hu->tty->dev, "wait for cts... received!\n");
> +			return 0;
> +		}
> +		if (time_after(jiffies, timeout)) {
> +			dev_dbg(hu->tty->dev, "wait for cts... timeout!\n");
> +			return -ETIMEDOUT;
> +		}
> +		usleep_range(1000, 2000);
> +	}
> +}

This is a super odd function. No return value since we essentially have an endless loop.

> +
> +static int btdev_match(struct device *child, void *data)
> +{
> +	if (!strcmp(child->driver->name, "nokia-bluetooth"))
> +		return 1;
> +	else
> +		return 0;
> +}

Anything wrong with just return !strcmp?

> +
> +static irqreturn_t wakeup_handler(int irq, void *data)
> +{
> +	struct nokia_bt_dev *btdev = data;
> +	struct device *serialdev = btdev->hu->tty->dev;
> +	int wake_state = gpiod_get_value(btdev->btdata->wakeup_host);
> +
> +	dev_dbg(serialdev, "wakeup received: %d -> %d\n",
> +		btdev->wake_state, wake_state);
> +
> +	if (btdev->wake_state == wake_state)
> +		return IRQ_HANDLED;
> +
> +	if (wake_state)
> +		pm_runtime_get_sync(serialdev);
> +	else if (!wake_state)
> +		pm_runtime_put(serialdev);
> +
> +	btdev->wake_state = wake_state;
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int nokia_reset(struct hci_uart *hu)
> +{
> +	struct nokia_bt_dev *btdev = hu->priv;
> +	int err;
> +
> +	/* reset routine */
> +	gpiod_set_value_cansleep(btdev->btdata->reset, 0);
> +	gpiod_set_value_cansleep(btdev->btdata->wakeup_bt, 1);
> +
> +	msleep(50);
> +
> +	/* safety check */
> +	err = gpiod_get_value_cansleep(btdev->btdata->wakeup_host);
> +	if (err == 1) {
> +		dev_err(hu->tty->dev, "reset: host wakeup not low!\n");
> +		return -EPROTO;
> +	}
> +
> +	/* flush queues */
> +	tty_ldisc_flush(hu->tty);
> +	tty_driver_flush_buffer(hu->tty);
> +
> +	/* init uart */
> +	hci_uart_init_tty(hu);
> +	hci_uart_set_flow_control(hu, true);
> +	hci_uart_set_baudrate(hu, INIT_SPEED);
> +
> +	gpiod_set_value_cansleep(btdev->btdata->reset, 1);
> +	gpiod_set_value_cansleep(btdev->btdata->wakeup_bt, 0);
> +
> +	msleep(100);
> +
> +	err = gpiod_get_value_cansleep(btdev->btdata->wakeup_host);
> +	if (err == 0) {
> +		dev_err(hu->tty->dev, "reset: host wakeup not high!\n");
> +		return -EPROTO;
> +	}
> +
> +	/* wait for cts */
> +	err = hci_uart_wait_for_cts(hu, true, 100);
> +	if (err < 0) {
> +		dev_err(hu->tty->dev, "CTS not received: %d\n", err);
> +		return err;
> +	}
> +
> +	gpiod_set_value_cansleep(btdev->btdata->wakeup_bt, 1);
> +	hci_uart_set_flow_control(hu, false);
> +
> +	return 0;
> +}
> +
> +static int nokia_send_alive_packet(struct hci_uart *hu)
> +{
> +	struct nokia_bt_dev *btdev = hu->priv;
> +	struct hci_nokia_alive_hdr *hdr;
> +	struct hci_nokia_alive_pkt *pkt;
> +	struct sk_buff *skb;
> +	int len;
> +
> +	dev_dbg(hu->tty->dev, "Sending alive packet...\n");
> +
> +	init_completion(&btdev->init_completion);
> +
> +	len = H4_TYPE_SIZE + sizeof(*hdr) + sizeof(*pkt);
> +	skb = bt_skb_alloc(len, GFP_KERNEL);
> +	if (!skb)
> +		return -ENOMEM;
> +
> +	hci_skb_pkt_type(skb) = HCI_NOKIA_ALIVE_PKT;
> +	memset(skb->data, 0x00, len);
> +
> +	hdr = (struct hci_nokia_alive_hdr *)skb_put(skb, sizeof(*hdr));
> +	hdr->dlen = sizeof(*pkt);
> +	pkt = (struct hci_nokia_alive_pkt *)skb_put(skb, sizeof(*pkt));
> +	pkt->mid = NOKIA_ALIVE_REQ;
> +
> +	hu->hdev->send(hu->hdev, skb);

I am not sure we want these to go through the Bluetooth core packet sending. They are not standard HCI packet and should stay within the driver. If you send them through the core they will cause problems with the monitor interface.

> +
> +	if (!wait_for_completion_interruptible_timeout(&btdev->init_completion,
> +		msecs_to_jiffies(1000))) {
> +		return -ETIMEDOUT;
> +	}
> +
> +	if (btdev->init_error < 0)
> +		return btdev->init_error;
> +
> +	return 0;
> +}
> +
> +static int nokia_send_negotiation(struct hci_uart *hu)
> +{
> +	struct nokia_bt_dev *btdev = hu->priv;
> +	struct hci_nokia_neg_cmd *neg_cmd;
> +	struct hci_nokia_neg_hdr *neg_hdr;
> +	struct sk_buff *skb;
> +	int len, err;
> +	u16 baud = DIV_ROUND_CLOSEST(BT_BAUDRATE_DIVIDER, MAX_BAUD_RATE);
> +	int sysclk = btdev->btdata->sysclk_speed / 1000;
> +
> +	dev_dbg(hu->tty->dev, "Sending negotiation...\n");
> +
> +	len = H4_TYPE_SIZE + sizeof(*neg_hdr) + sizeof(*neg_cmd);
> +	skb = bt_skb_alloc(len, GFP_KERNEL);
> +	if (!skb)
> +		return -ENOMEM;
> +
> +	hci_skb_pkt_type(skb) = HCI_NOKIA_NEG_PKT;
> +
> +	neg_hdr = (struct hci_nokia_neg_hdr *)skb_put(skb, sizeof(*neg_hdr));
> +	neg_hdr->dlen = sizeof(*neg_cmd);
> +
> +	neg_cmd = (struct hci_nokia_neg_cmd *)skb_put(skb, sizeof(*neg_cmd));
> +	neg_cmd->ack = NOKIA_NEG_REQ;
> +	neg_cmd->baud = cpu_to_le16(baud);
> +	neg_cmd->unused1 = 0x0000;
> +	neg_cmd->proto = NOKIA_PROTO_BYTE;
> +	neg_cmd->sys_clk = cpu_to_le16(sysclk);
> +	neg_cmd->unused2 = 0x0000;
> +
> +	btdev->init_error = 0;
> +	init_completion(&btdev->init_completion);
> +
> +	hu->hdev->send(hu->hdev, skb);
> +
> +	if (!wait_for_completion_interruptible_timeout(&btdev->init_completion,
> +		msecs_to_jiffies(10000))) {
> +		return -ETIMEDOUT;
> +	}
> +
> +	if (btdev->init_error < 0)
> +		return btdev->init_error;
> +
> +	/* Change to operational settings */
> +	hci_uart_set_flow_control(hu, true); // disable flow control

Please use a proper comment that explains also disabling flow control.
> +
> +	/* setup negotiated max. baudrate */
> +	hci_uart_set_baudrate(hu, MAX_BAUD_RATE);
> +
> +	err = hci_uart_wait_for_cts(hu, true, 100);
> +	if (err < 0)
> +		return err;
> +
> +	hci_uart_set_flow_control(hu, false); // re-enable flow control
> +
> +	dev_dbg(hu->tty->dev, "Negotiation successful...\n");
> +
> +	return 0;
> +}
> +
> +static int nokia_setup_fw(struct hci_uart *hu)
> +{
> +	struct nokia_bt_dev *btdev = hu->priv;
> +	const struct firmware *fw;
> +	const u8 *fw_ptr;
> +	size_t fw_size;
> +	int err;
> +
> +	BT_DBG("hu %p", hu);
> +
> +	err = request_firmware(&fw, nokia_get_fw_name(btdev), hu->tty->dev);

So does this nokia_get_fw_name really needs to be a separate function? Or can this just be done right here in this function? I prefer it to be done where it is actually used. Unless you use that name in many places.

> +	if (err < 0) {
> +		BT_ERR("%s: Failed to load Nokia firmware file (%d)",
> +		       hu->hdev->name, err);
> +		return err;
> +	}
> +
> +	fw_ptr = fw->data;
> +	fw_size = fw->size;
> +
> +	while (fw_size >= 4) {
> +		u16 pkt_size = get_unaligned_le16(fw_ptr);
> +		u8 pkt_type = fw_ptr[2];
> +		const struct hci_command_hdr *cmd;
> +		u16 opcode;
> +		struct sk_buff *skb;
> +
> +		switch (pkt_type) {
> +		case HCI_COMMAND_PKT:
> +			cmd = (struct hci_command_hdr *)(fw_ptr + 3);
> +			opcode = le16_to_cpu(cmd->opcode);
> +
> +			skb = __hci_cmd_sync(hu->hdev, opcode, cmd->plen,
> +					     fw_ptr + 3 + HCI_COMMAND_HDR_SIZE,
> +					     HCI_INIT_TIMEOUT);
> +			if (IS_ERR(skb)) {
> +				err = PTR_ERR(skb);
> +				BT_ERR("%s: Firmware command %04x failed (%d)",
> +				       hu->hdev->name, opcode, err);
> +				goto done;
> +			}
> +			kfree_skb(skb);
> +			break;
> +		case HCI_NOKIA_RADIO_PKT:

Are you sure you can ignore the RADIO_PKT commands. They are used to set up the FM radio parts of the chip. They are standard HCI commands (in the case of Broadcom at least). At minimum it should be added a comment here that you are ignoring them on purpose.

> +		case HCI_NOKIA_NEG_PKT:
> +		case HCI_NOKIA_ALIVE_PKT:

And here I would also a comment on why are we ignore these commands and driving this all by ourselves.

> +			break;
> +		}
> +
> +		fw_ptr += pkt_size + 2;
> +		fw_size -= pkt_size + 2;
> +	}
> +
> +done:
> +	release_firmware(fw);
> +	return err;
> +}
> +
> +static int nokia_setup(struct hci_uart *hu)
> +{
> +	int err;
> +
> +	pm_runtime_get_sync(hu->tty->dev);
> +
> +	dev_dbg(hu->tty->dev, "Nokia H4+ protocol setup...\n");
> +
> +	/* 0. reset connection */
> +	err = nokia_reset(hu);
> +	if (err < 0) {
> +		dev_err(hu->tty->dev, "Reset failed: %d\n", err);
> +		goto out;
> +	}
> +
> +	/* 1. negotiate speed etc */
> +	err = nokia_send_negotiation(hu);
> +	if (err < 0) {
> +		dev_err(hu->tty->dev, "Negotiation failed: %d\n", err);
> +		goto out;
> +	}
> +
> +	/* 2. verify correct setup using alive packet */
> +	err = nokia_send_alive_packet(hu);
> +	if (err < 0) {
> +		dev_err(hu->tty->dev, "Alive check failed: %d\n", err);
> +		goto out;
> +	}
> +
> +	/* 3. send firmware */
> +	err = nokia_setup_fw(hu);
> +	if (err < 0) {
> +		dev_err(hu->tty->dev, "Could not setup FW: %d\n", err);
> +		goto out;
> +	}
> +
> +	hci_uart_set_flow_control(hu, true);
> +	hci_uart_set_baudrate(hu, BC4_MAX_BAUD_RATE);

I think this variable needs a better name if it is common for all vendors.

> +	hci_uart_set_flow_control(hu, false);
> +
> +	dev_dbg(hu->tty->dev, "Nokia H4+ protocol setup done!\n");
> +
> +	/*
> +	 * TODO:
> +	 * disable wakeup_bt at this point and automatically enable it when
> +	 * data is about to be written until all data has been written (+ some
> +	 * delay).
> +	 *
> +	 * Since this is not yet support by the uart/tty kernel framework we
> +	 * will always keep enabled the wakeup_bt gpio for now, so that the
> +	 * bluetooth chip will never transit into idle modes.
> +	 */
> +
> +out:
> +	pm_runtime_put(hu->tty->dev);
> +
> +	return err;
> +}
> +
> +static int nokia_open(struct hci_uart *hu)
> +{
> +	struct device *serialdev = hu->tty->dev;
> +	struct nokia_bt_dev *btdev;
> +	struct device *uartbtdev;
> +	int err;
> +
> +	btdev = kzalloc(sizeof(*btdev), GFP_KERNEL);
> +	if (!btdev)
> +		return -ENOMEM;
> +
> +	btdev->hu = hu;
> +
> +	skb_queue_head_init(&btdev->txq);
> +
> +	uartbtdev = device_find_child(serialdev, NULL, btdev_match);
> +	if (!uartbtdev) {
> +		dev_err(serialdev, "bluetooth device node not found!\n");
> +		return -ENODEV;
> +	}
> +
> +	btdev->btdata = dev_get_drvdata(uartbtdev);
> +	if (!btdev->btdata)
> +		return -EINVAL;
> +
> +	hu->priv = btdev;
> +
> +	/* register handler for host wakeup gpio */
> +	btdev->wake_irq = gpiod_to_irq(btdev->btdata->wakeup_host);
> +	err = request_threaded_irq(btdev->wake_irq, NULL, wakeup_handler,
> +		IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +		"wakeup", btdev);
> +	if (err) {
> +		gpiod_set_value(btdev->btdata->reset, 0);
> +		gpiod_set_value(btdev->btdata->wakeup_bt, 0);
> +		return err;
> +	}
> +
> +	dev_dbg(serialdev, "Nokia H4+ protocol initialized with %s!\n",
> +		dev_name(uartbtdev));
> +
> +	pm_runtime_enable(hu->tty->dev);
> +
> +	return 0;
> +}
> +
> +static int nokia_flush(struct hci_uart *hu)
> +{
> +	struct nokia_bt_dev *btdev = hu->priv;
> +
> +	BT_DBG("hu %p", hu);
> +
> +	skb_queue_purge(&btdev->txq);
> +
> +	return 0;
> +}
> +
> +static int nokia_close(struct hci_uart *hu)
> +{
> +	struct nokia_bt_dev *btdev = hu->priv;
> +
> +	hu->priv = NULL;
> +
> +	BT_DBG("hu %p", hu);
> +
> +	skb_queue_purge(&btdev->txq);
> +
> +	kfree_skb(btdev->rx_skb);
> +
> +	free_irq(btdev->wake_irq, btdev);
> +
> +	/* disable module */
> +	gpiod_set_value(btdev->btdata->reset, 0);
> +	gpiod_set_value(btdev->btdata->wakeup_bt, 0);
> +
> +	hu->priv = NULL;
> +	kfree(btdev);
> +
> +	pm_runtime_disable(hu->tty->dev);
> +
> +	return 0;
> +}
> +
> +/* Enqueue frame for transmittion (padding, crc, etc) */
> +static int nokia_enqueue(struct hci_uart *hu, struct sk_buff *skb)
> +{
> +	struct nokia_bt_dev *btdev = hu->priv;
> +	int err;
> +
> +	BT_DBG("hu %p skb %p", hu, skb);
> +
> +	/* Prepend skb with frame type */
> +	memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
> +
> +	/* Packets must be word aligned */
> +	if (skb->len % 2) {
> +		err = skb_pad(skb, 1);
> +		if (err)
> +			return err;
> +		*skb_put(skb, 1) = 0x00;
> +	}
> +
> +	skb_queue_tail(&btdev->txq, skb);
> +
> +	return 0;
> +}
> +
> +static int nokia_recv_negotiation_packet(struct hci_dev *hdev,
> +					 struct sk_buff *skb)
> +{
> +	struct hci_uart *hu = hci_get_drvdata(hdev);
> +	struct nokia_bt_dev *btdev = hu->priv;
> +	struct hci_nokia_neg_hdr *hdr;
> +	struct hci_nokia_neg_evt *evt;
> +	int ret = 0;
> +
> +	hdr = (struct hci_nokia_neg_hdr *)skb->data;
> +	if (hdr->dlen != sizeof(*evt)) {
> +		btdev->init_error = -EIO;
> +		ret = -EIO;
> +		goto finish_neg;
> +	}
> +
> +	evt = (struct hci_nokia_neg_evt *)skb_pull(skb, sizeof(*hdr));
> +
> +	if (evt->ack != NOKIA_NEG_ACK) {
> +		dev_err(hu->tty->dev, "Could not negotiate hci_nokia settings\n");
> +		btdev->init_error = -EINVAL;
> +	}
> +
> +	btdev->man_id = evt->man_id;
> +	btdev->ver_id = evt->ver_id;
> +
> +	dev_dbg(hu->tty->dev, "NOKIA negotiation:\n");
> +	dev_dbg(hu->tty->dev, "\tbaudrate = %u\n", evt->baud);
> +	dev_dbg(hu->tty->dev, "\tsystem clock = %u\n", evt->sys_clk);
> +	dev_dbg(hu->tty->dev, "\tmanufacturer id = %u\n", evt->man_id);
> +	dev_dbg(hu->tty->dev, "\tversion id = %u\n", evt->ver_id);
> +
> +finish_neg:
> +	complete(&btdev->init_completion);
> +	kfree_skb(skb);
> +	return ret;
> +}
> +
> +static int nokia_recv_alive_packet(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> +	struct hci_uart *hu = hci_get_drvdata(hdev);
> +	struct nokia_bt_dev *btdev = hu->priv;
> +	struct hci_nokia_alive_hdr *hdr;
> +	struct hci_nokia_alive_pkt *pkt;
> +	int ret = 0;
> +
> +	hdr = (struct hci_nokia_alive_hdr *)skb->data;
> +	if (hdr->dlen != sizeof(*pkt)) {
> +		dev_err(hu->tty->dev, "Corrupted alive message\n");
> +		btdev->init_error = -EIO;
> +		ret = -EIO;
> +		goto finish_alive;
> +	}
> +
> +	pkt = (struct hci_nokia_alive_pkt *)skb_pull(skb, sizeof(*hdr));
> +
> +	if (pkt->mid != NOKIA_ALIVE_RESP) {
> +		dev_err(hu->tty->dev, "Invalid alive response: 0x%02x!\n",
> +			pkt->mid);
> +		btdev->init_error = -EINVAL;
> +		goto finish_alive;
> +	}
> +
> +	dev_dbg(hu->tty->dev, "Received alive packet!\n");
> +
> +finish_alive:
> +	complete(&btdev->init_completion);
> +	kfree_skb(skb);
> +	return ret;
> +}
> +
> +static int nokia_recv_radio(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> +	/* Packets received on the dedicated radio channel are
> +	 * HCI events and so feed them back into the core.
> +	 */
> +	bt_cb(skb)->pkt_type = HCI_EVENT_PKT;

I think using hci_skb_pkt_type(skb) is correct here as well.

> +	return hci_recv_frame(hdev, skb);
> +}
> +
> +/* Recv data */
> +static const struct h4_recv_pkt nokia_recv_pkts[] = {
> +	{ NOKIA_RECV_ACL,	.recv = hci_recv_frame },
> +	{ NOKIA_RECV_SCO,	.recv = hci_recv_frame },
> +	{ NOKIA_RECV_EVENT,	.recv = hci_recv_frame },
> +	{ NOKIA_RECV_ALIVE,	.recv = nokia_recv_alive_packet },
> +	{ NOKIA_RECV_NEG,	.recv = nokia_recv_negotiation_packet },
> +	{ NOKIA_RECV_RADIO,	.recv = nokia_recv_radio },
> +};
> +
> +static int nokia_recv(struct hci_uart *hu, const void *data, int count)
> +{
> +	struct nokia_bt_dev *btdev = hu->priv;
> +	int err;
> +
> +	if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
> +		return -EUNATCH;
> +
> +	btdev->rx_skb = h4_recv_buf(hu->hdev, btdev->rx_skb, data, count,
> +				  nokia_recv_pkts, ARRAY_SIZE(nokia_recv_pkts));
> +	if (IS_ERR(btdev->rx_skb)) {
> +		err = PTR_ERR(btdev->rx_skb);
> +		BT_ERR("%s: Frame reassembly failed (%d)", hu->hdev->name, err);
> +		btdev->rx_skb = NULL;
> +		return err;
> +	}
> +
> +	return count;
> +}
> +
> +static struct sk_buff *nokia_dequeue(struct hci_uart *hu)
> +{
> +	struct nokia_bt_dev *btdev = hu->priv;
> +
> +	return skb_dequeue(&btdev->txq);
> +}
> +
> +static const struct hci_uart_proto nokia_proto = {
> +	.id		= HCI_UART_NOKIA,
> +	.name		= "Nokia",
> +	.open		= nokia_open,
> +	.close		= nokia_close,
> +	.recv		= nokia_recv,
> +	.enqueue	= nokia_enqueue,
> +	.dequeue	= nokia_dequeue,
> +	.flush		= nokia_flush,
> +	.setup		= nokia_setup,
> +};
> +
> +static int nokia_bluetooth_probe(struct platform_device *pdev)
> +{
> +	struct nokia_uart_dev *btdata;
> +	struct device *bcmdev = &pdev->dev;
> +	struct clk *sysclk;
> +	int err = 0;
> +
> +	if(!bcmdev->parent) {
> +		dev_err(bcmdev, "parent device missing!\n");
> +		return -ENODEV;
> +	}
> +
> +	btdata = devm_kmalloc(bcmdev, sizeof(*btdata), GFP_KERNEL);
> +	if(!btdata)
> +		return -ENOMEM;
> +
> +	btdata->dev = bcmdev;
> +	dev_set_drvdata(bcmdev, btdata);
> +
> +	btdata->port = dev_get_drvdata(bcmdev->parent);
> +	if(!btdata->port) {
> +		dev_err(bcmdev, "port data missing in parent device!\n");
> +		return -ENODEV;
> +	}
> +
> +	btdata->reset = devm_gpiod_get(bcmdev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(btdata->reset)) {
> +		err = PTR_ERR(btdata->reset);
> +		dev_err(bcmdev, "could not get reset gpio: %d\n", err);
> +		return err;
> +	}
> +
> +	btdata->wakeup_host = devm_gpiod_get(bcmdev, "host-wakeup", GPIOD_IN);
> +	if (IS_ERR(btdata->wakeup_host)) {
> +		err = PTR_ERR(btdata->wakeup_host);
> +		dev_err(bcmdev, "could not get host wakeup gpio: %d\n", err);
> +		return err;
> +	}
> +
> +
> +	btdata->wakeup_bt = devm_gpiod_get(bcmdev, "bluetooth-wakeup",
> +					    GPIOD_OUT_LOW);
> +	if (IS_ERR(btdata->wakeup_bt)) {
> +		err = PTR_ERR(btdata->wakeup_bt);
> +		dev_err(bcmdev, "could not get BT wakeup gpio: %d\n", err);
> +		return err;
> +	}
> +
> +	sysclk = devm_clk_get(bcmdev, "sysclk");
> +	if (IS_ERR(sysclk)) {
> +		err = PTR_ERR(sysclk);
> +		dev_err(bcmdev, "could not get sysclk: %d\n", err);
> +		return err;
> +	}
> +
> +	clk_prepare_enable(sysclk);
> +	btdata->sysclk_speed = clk_get_rate(sysclk);
> +	clk_disable_unprepare(sysclk);
> +
> +	dev_dbg(bcmdev, "parent uart: %s\n", dev_name(bcmdev->parent));
> +	dev_dbg(bcmdev, "sysclk speed: %ld kHz\n", btdata->sysclk_speed / 1000);
> +
> +	/* TODO: open tty and setup line disector from kernel-side */
> +
> +	return err;
> +}
> +
> +static const struct of_device_id nokia_bluetooth_of_match[] = {
> +	{ .compatible = "nokia,brcm,bcm2048", },
> +	{ .compatible = "nokia,ti,wl1271-bluetooth", },

Where is the CSR BC4 one here? I prefer if we only have support for the ones that are actually supported and detected. We can easily extend things later.

> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, nokia_bluetooth_of_match);
> +
> +static struct platform_driver platform_nokia_driver = {
> +	.driver = {
> +		.name = "nokia-bluetooth",
> +		.of_match_table = nokia_bluetooth_of_match,
> +	},
> +	.probe = nokia_bluetooth_probe,
> +};
> +
> +int __init nokia_init(void)
> +{
> +	platform_driver_register(&platform_nokia_driver);
> +	return hci_uart_register_proto(&nokia_proto);
> +}
> +
> +int __exit nokia_deinit(void)
> +{
> +	platform_driver_unregister(&platform_nokia_driver);
> +	return hci_uart_unregister_proto(&nokia_proto);
> +}
> diff --git a/drivers/bluetooth/hci_nokia.h b/drivers/bluetooth/hci_nokia.h
> new file mode 100644
> index 000000000000..8c4d307840e5
> --- /dev/null
> +++ b/drivers/bluetooth/hci_nokia.h
> @@ -0,0 +1,140 @@
> +/*
> + *  Copyright (C) 2016 Sebastian Reichel <sre@kernel.org>
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or
> + *  (at your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + */
> +
> +#ifndef __HCI_NOKIA_H
> +#define __HCI_NOKIA_H

Lets not do a separate header here. Just move this all into hci_nokia.c. There is really zero benefit in the header.

> +
> +#define NOKIA_ID_CSR		0x02
> +#define NOKIA_ID_BCM2048	0x04
> +#define NOKIA_ID_TI1271		0x31
> +
> +#define FIRMWARE_CSR		"nokia/bc4fw.bin"

If the CSR ones are not yet supported, then leave them out for now. We can add this later.

> +#define FIRMWARE_BCM2048	"nokia/bcmfw.bin"
> +#define FIRMWARE_TI1271		"nokia/ti1273.bin"
> +
> +#define NOKIA_BCM_BDADDR	0xfc01

We have btbcm.[ch] for this.

> +
> +#define HCI_NOKIA_NEG_PKT	0x06
> +#define HCI_NOKIA_ALIVE_PKT	0x07
> +#define HCI_NOKIA_RADIO_PKT	0x08
> +
> +#define HCI_NOKIA_NEG_HDR_SIZE		1
> +#define HCI_NOKIA_MAX_NEG_SIZE		255
> +#define HCI_NOKIA_ALIVE_HDR_SIZE	1
> +#define HCI_NOKIA_MAX_ALIVE_SIZE	255
> +#define HCI_NOKIA_RADIO_HDR_SIZE	2
> +#define HCI_NOKIA_MAX_RADIO_SIZE	255
> +
> +#define NOKIA_PROTO_PKT		0x44
> +#define NOKIA_PROTO_BYTE	0x4c
> +
> +#define NOKIA_NEG_REQ		0x00
> +#define NOKIA_NEG_ACK		0x20
> +#define NOKIA_NEG_NAK		0x40
> +
> +#define H4_TYPE_SIZE		1

I am not sure this define adds any overall value to the code.

> +
> +#define NOKIA_RECV_ACL \
> +	H4_RECV_ACL, \
> +	.wordaligned = true
> +
> +#define NOKIA_RECV_SCO \
> +	H4_RECV_SCO, \
> +	.wordaligned = true
> +
> +#define NOKIA_RECV_EVENT \
> +	H4_RECV_EVENT, \
> +	.wordaligned = true
> +
> +#define NOKIA_RECV_ALIVE \
> +	.type = HCI_NOKIA_ALIVE_PKT, \
> +	.hlen = HCI_NOKIA_ALIVE_HDR_SIZE, \
> +	.loff = 0, \
> +	.lsize = 1, \
> +	.maxlen = HCI_NOKIA_MAX_ALIVE_SIZE, \
> +	.wordaligned = true
> +
> +#define NOKIA_RECV_NEG \
> +	.type = HCI_NOKIA_NEG_PKT, \
> +	.hlen = HCI_NOKIA_NEG_HDR_SIZE, \
> +	.loff = 0, \
> +	.lsize = 1, \
> +	.maxlen = HCI_NOKIA_MAX_NEG_SIZE, \
> +	.wordaligned = true
> +
> +#define NOKIA_RECV_RADIO \
> +	.type = HCI_NOKIA_RADIO_PKT, \
> +	.hlen = HCI_NOKIA_RADIO_HDR_SIZE, \
> +	.loff = 1, \
> +	.lsize = 1, \
> +	.maxlen = HCI_NOKIA_MAX_RADIO_SIZE, \
> +	.wordaligned = true

For this ones I would have use the HCI event ones. My original patch had this:

+#define NOK_RECV_NEG \
+	.type = NOK_NEG_PKT, \
+	.hlen = NOK_NEG_HDR_SIZE, \
+	.loff = 0, \
+	.lsize = 1, \
+	.maxlen = HCI_MAX_EVENT_SIZE
+
+#define NOK_RECV_ALIVE \
+	.type = NOK_ALIVE_PKT, \
+	.hlen = NOK_ALIVE_HDR_SIZE, \
+	.loff = 0, \
+	.lsize = 1, \
+	.maxlen = HCI_MAX_EVENT_SIZE
+
+#define NOK_RECV_RADIO \
+	.type = NOK_RADIO_PKT, \
+	.hlen = HCI_EVENT_HDR_SIZE, \
+	.loff = 1, \
+	.lsize = 1, \
+	.maxlen = HCI_MAX_EVENT_SIZE
+
+static const struct h4_recv_pkt nok_recv_pkts[] = {
+	{ H4_RECV_ACL,    .recv = hci_recv_frame },
+	{ H4_RECV_SCO,    .recv = hci_recv_frame },
+	{ H4_RECV_EVENT,  .recv = hci_recv_frame },
+	{ NOK_RECV_NEG,   .recv = nok_recv_neg   },
+	{ NOK_RECV_ALIVE, .recv = nok_recv_alive },
+	{ NOK_RECV_RADIO, .recv = nok_recv_radio },

With just these simple defines at the top:

+#define NOK_NEG_PKT	0x06
+#define NOK_ALIVE_PKT	0x07
+#define NOK_RADIO_PKT	0x08
+
+#define NOK_NEG_HDR_SIZE	1
+#define NOK_ALIVE_HDR_SIZE	1

And I would prefer if we keep it like that.

> +
> +struct hci_nokia_neg_hdr {
> +	__u8	dlen;
> +} __packed;
> +
> +struct hci_nokia_neg_cmd {
> +	__u8	ack;
> +	__u16	baud;
> +	__u16	unused1;
> +	__u8	proto;
> +	__u16	sys_clk;
> +	__u16	unused2;
> +} __packed;
> +
> +static inline struct hci_nokia_neg_hdr *hci_nokia_neg_hdr(const struct sk_buff *skb)
> +{
> +	return (struct hci_nokia_neg_hdr *) skb->data;
> +}

What good is this inline? A define would be way better, if really needed.

> +
> +#define NOKIA_ALIVE_REQ   0x55
> +#define NOKIA_ALIVE_RESP  0xcc
> +
> +struct hci_nokia_alive_hdr {
> +	__u8	dlen;
> +} __packed;
> +
> +struct hci_nokia_alive_pkt {
> +	__u8	mid;
> +	__u8	unused;
> +} __packed;
> +
> +static inline struct hci_nokia_alive_hdr *hci_nokia_alive_hdr(const struct sk_buff *skb)
> +{
> +	return (struct hci_nokia_alive_hdr *) skb->data;
> +}
> +
> +struct hci_nokia_neg_evt {
> +	__u8	ack;
> +	__u16	baud;
> +	__u16	unused1;
> +	__u8	proto;
> +	__u16	sys_clk;
> +	__u16	unused2;
> +	__u8	man_id;
> +	__u8	ver_id;
> +} __packed;
> +
> +#define BT_BAUDRATE_DIVIDER     384000000
> +#define BC4_MAX_BAUD_RATE       3692300
> +#define MAX_BAUD_RATE           921600
> +#define INIT_SPEED              120000
> +
> +struct hci_nokia_radio_hdr {
> +	__u8	evt;
> +	__u8	dlen;
> +} __packed;
> +
> +#endif
> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> index a7d67aec3632..314b243df996 100644
> --- a/drivers/bluetooth/hci_uart.h
> +++ b/drivers/bluetooth/hci_uart.h
> @@ -35,7 +35,7 @@
> #define HCIUARTGETFLAGS		_IOR('U', 204, int)
> 
> /* UART protocols */
> -#define HCI_UART_MAX_PROTO	10
> +#define HCI_UART_MAX_PROTO	11
> 
> #define HCI_UART_H4	0
> #define HCI_UART_BCSP	1
> @@ -47,6 +47,7 @@
> #define HCI_UART_BCM	7
> #define HCI_UART_QCA	8
> #define HCI_UART_AG6XX	9
> +#define HCI_UART_NOKIA	10
> 
> #define HCI_UART_RAW_DEVICE	0
> #define HCI_UART_RESET_ON_INIT	1
> @@ -190,3 +191,8 @@ int qca_deinit(void);
> int ag6xx_init(void);
> int ag6xx_deinit(void);
> #endif
> +
> +#ifdef CONFIG_BT_HCIUART_NOKIA
> +int nokia_init(void);
> +int nokia_deinit(void);
> +#endif

Regards

Marcel

Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

[Pali Rohár]

On Tuesday 16 August 2016 09:02:14 Marcel Holtmann wrote:
> > +static int nokia_setup_fw(struct hci_uart *hu)
> > +{
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +	const struct firmware *fw;
> > +	const u8 *fw_ptr;
> > +	size_t fw_size;
> > +	int err;
> > +
> > +	BT_DBG("hu %p", hu);
> > +
> > +	err = request_firmware(&fw, nokia_get_fw_name(btdev), hu->tty->dev);
> 
> So does this nokia_get_fw_name really needs to be a separate function? Or can this just be done right here in this function? I prefer it to be done where it is actually used. Unless you use that name in many places.
> 
> > +	if (err < 0) {
> > +		BT_ERR("%s: Failed to load Nokia firmware file (%d)",
> > +		       hu->hdev->name, err);
> > +		return err;
> > +	}
> > +
> > +	fw_ptr = fw->data;
> > +	fw_size = fw->size;
> > +
> > +	while (fw_size >= 4) {
> > +		u16 pkt_size = get_unaligned_le16(fw_ptr);
> > +		u8 pkt_type = fw_ptr[2];
> > +		const struct hci_command_hdr *cmd;
> > +		u16 opcode;
> > +		struct sk_buff *skb;
> > +
> > +		switch (pkt_type) {
> > +		case HCI_COMMAND_PKT:
> > +			cmd = (struct hci_command_hdr *)(fw_ptr + 3);
> > +			opcode = le16_to_cpu(cmd->opcode);
> > +
> > +			skb = __hci_cmd_sync(hu->hdev, opcode, cmd->plen,
> > +					     fw_ptr + 3 + HCI_COMMAND_HDR_SIZE,
> > +					     HCI_INIT_TIMEOUT);
> > +			if (IS_ERR(skb)) {
> > +				err = PTR_ERR(skb);
> > +				BT_ERR("%s: Firmware command %04x failed (%d)",
> > +				       hu->hdev->name, opcode, err);
> > +				goto done;
> > +			}
> > +			kfree_skb(skb);
> > +			break;
> > +		case HCI_NOKIA_RADIO_PKT:
> 
> Are you sure you can ignore the RADIO_PKT commands. They are used to set up the FM radio parts of the chip. They are standard HCI commands (in the case of Broadcom at least). At minimum it should be added a comment here that you are ignoring them on purpose.
> 
> > +		case HCI_NOKIA_NEG_PKT:
> > +		case HCI_NOKIA_ALIVE_PKT:
> 
> And here I would also a comment on why are we ignore these commands and driving this all by ourselves.
> 

Good question... In Pavel's version of bluetooth driver, which is
working on Nokia N900, is sent whole firmware at one __hci_cmd_sync
step. It does not skip any packets, plus he added this comment:

/* Note that this is timing-critical. If sending packets takes
 * too long, initialization will fail.
 */

So really, can we skip those packets? And is not this reason why
this bluetooth driver does not work on Nokia N900?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2233 bytes --]

Hi,

On Tue, Aug 16, 2016 at 09:52:17AM +0200, Pali Rohár wrote:
> > > +		case HCI_NOKIA_RADIO_PKT:
> > 
> > Are you sure you can ignore the RADIO_PKT commands. They are
> > used to set up the FM radio parts of the chip. They are standard
> > HCI commands (in the case of Broadcom at least). At minimum it
> > should be added a comment here that you are ignoring them on
> > purpose.
> > 
> > > +		case HCI_NOKIA_NEG_PKT:
> > > +		case HCI_NOKIA_ALIVE_PKT:
> > 
> > And here I would also a comment on why are we ignore these
> > commands and driving this all by ourselves.
> > 
> 
> Good question... In Pavel's version of bluetooth driver, which is
> working on Nokia N900, is sent whole firmware at one __hci_cmd_sync
> step. It does not skip any packets, plus he added this comment:
> 
> /* Note that this is timing-critical. If sending packets takes
>  * too long, initialization will fail.
>  */
> 
> So really, can we skip those packets? And is not this reason why
> this bluetooth driver does not work on Nokia N900?

Let's have a look - here is Pavel's version:
https://lwn.net/Articles/627201/

In pseudocode:

while(true) {
    cmd = get_cmd_from_firmware();
    if (!cmd) break;
    __hci_cmd_sync(cmd);
}

This is not "whole firmware at one __hci_cmd_sync step", is it?
And obviously that wouldn't work. Next let's have a look at
"It does not skip any packets":

/* Skip first two packets */
if (++num <= 2)
    continue;

Which are HCI_NOKIA_NEG_PKT and HCI_NOKIA_ALIVE_PKT. Those are
open-coded. By using the packets from the firmware we could drop the
negotiation/alive functions from the driver and remove quite a few
lines of code. I think it should only be done after finding the N900
bug, though. I found it quite useful, that first communication does not
happen through the firmware file.

Surely the radio packet is not ignored, but that part is not used on
N950 and N900 fails at first packet, so no way to test the radio
packet handling. I probably should add a /* TODO: check how to
handle radio packets */ for the radio packet entry. Or implement it
the way Marcel suggested and hope that it just works once the other
bug is found.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 25933 bytes --]

Hi Marcel,

On Tue, Aug 16, 2016 at 09:02:14AM +0200, Marcel Holtmann wrote:
> [...]
> > +#include <linux/module.h>
> > +
> > +#include <linux/clk.h>
> > +#include <linux/kernel.h>
> > +#include <linux/init.h>
> > +#include <linux/types.h>
> > +#include <linux/fcntl.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/ptrace.h>
> > +#include <linux/poll.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/firmware.h>
> > +#include <linux/slab.h>
> > +#include <linux/tty.h>
> > +#include <linux/errno.h>
> > +#include <linux/string.h>
> > +#include <linux/signal.h>
> > +#include <linux/ioctl.h>
> > +#include <linux/skbuff.h>
> > +#include <linux/delay.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <linux/gpio/consumer.h>
> > +
> > +#include <linux/unaligned/le_struct.h>
> 
> are you sure all these includes are needed?

No. A few of them are from previous version of
the driver. I will clean this up in the next
version.

> [...]
> > +static int hci_uart_wait_for_cts(struct hci_uart *hu, bool state,
> > +				 int timeout_ms)
> > +{
> > +	unsigned long timeout;
> > +	int signal;
> > +
> > +	timeout = jiffies + msecs_to_jiffies(timeout_ms);
> > +	for (;;) {
> > +		signal = hu->tty->ops->tiocmget(hu->tty) & TIOCM_CTS;
> > +		if (!!signal == !!state) {
> > +			dev_dbg(hu->tty->dev, "wait for cts... received!\n");
> > +			return 0;
> > +		}
> > +		if (time_after(jiffies, timeout)) {
> > +			dev_dbg(hu->tty->dev, "wait for cts... timeout!\n");
> > +			return -ETIMEDOUT;
> > +		}
> > +		usleep_range(1000, 2000);
> > +	}
> > +}
> 
> This is a super odd function. No return value since we essentially
> have an endless loop.

I will rewrite it, so that the loop condition checks for timeout.

> > +
> > +static int btdev_match(struct device *child, void *data)
> > +{
> > +	if (!strcmp(child->driver->name, "nokia-bluetooth"))
> > +		return 1;
> > +	else
> > +		return 0;
> > +}
> 
> Anything wrong with just return !strcmp?

No. I had initially debug prints in the cases.

> > +static int nokia_send_alive_packet(struct hci_uart *hu)
> > +{
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +	struct hci_nokia_alive_hdr *hdr;
> > +	struct hci_nokia_alive_pkt *pkt;
> > +	struct sk_buff *skb;
> > +	int len;
> > +
> > +	dev_dbg(hu->tty->dev, "Sending alive packet...\n");
> > +
> > +	init_completion(&btdev->init_completion);
> > +
> > +	len = H4_TYPE_SIZE + sizeof(*hdr) + sizeof(*pkt);
> > +	skb = bt_skb_alloc(len, GFP_KERNEL);
> > +	if (!skb)
> > +		return -ENOMEM;
> > +
> > +	hci_skb_pkt_type(skb) = HCI_NOKIA_ALIVE_PKT;
> > +	memset(skb->data, 0x00, len);
> > +
> > +	hdr = (struct hci_nokia_alive_hdr *)skb_put(skb, sizeof(*hdr));
> > +	hdr->dlen = sizeof(*pkt);
> > +	pkt = (struct hci_nokia_alive_pkt *)skb_put(skb, sizeof(*pkt));
> > +	pkt->mid = NOKIA_ALIVE_REQ;
> > +
> > +	hu->hdev->send(hu->hdev, skb);
> 
> I am not sure we want these to go through the Bluetooth core
> packet sending. They are not standard HCI packet and should stay
> within the driver. If you send them through the core they will
> cause problems with the monitor interface.

ok. I will directly call nokia_enqueue().

> > +
> > +	if (!wait_for_completion_interruptible_timeout(&btdev->init_completion,
> > +		msecs_to_jiffies(1000))) {
> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	if (btdev->init_error < 0)
> > +		return btdev->init_error;
> > +
> > +	return 0;
> > +}
> > +
> > +static int nokia_send_negotiation(struct hci_uart *hu)
> > +{
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +	struct hci_nokia_neg_cmd *neg_cmd;
> > +	struct hci_nokia_neg_hdr *neg_hdr;
> > +	struct sk_buff *skb;
> > +	int len, err;
> > +	u16 baud = DIV_ROUND_CLOSEST(BT_BAUDRATE_DIVIDER, MAX_BAUD_RATE);
> > +	int sysclk = btdev->btdata->sysclk_speed / 1000;
> > +
> > +	dev_dbg(hu->tty->dev, "Sending negotiation...\n");
> > +
> > +	len = H4_TYPE_SIZE + sizeof(*neg_hdr) + sizeof(*neg_cmd);
> > +	skb = bt_skb_alloc(len, GFP_KERNEL);
> > +	if (!skb)
> > +		return -ENOMEM;
> > +
> > +	hci_skb_pkt_type(skb) = HCI_NOKIA_NEG_PKT;
> > +
> > +	neg_hdr = (struct hci_nokia_neg_hdr *)skb_put(skb, sizeof(*neg_hdr));
> > +	neg_hdr->dlen = sizeof(*neg_cmd);
> > +
> > +	neg_cmd = (struct hci_nokia_neg_cmd *)skb_put(skb, sizeof(*neg_cmd));
> > +	neg_cmd->ack = NOKIA_NEG_REQ;
> > +	neg_cmd->baud = cpu_to_le16(baud);
> > +	neg_cmd->unused1 = 0x0000;
> > +	neg_cmd->proto = NOKIA_PROTO_BYTE;
> > +	neg_cmd->sys_clk = cpu_to_le16(sysclk);
> > +	neg_cmd->unused2 = 0x0000;
> > +
> > +	btdev->init_error = 0;
> > +	init_completion(&btdev->init_completion);
> > +
> > +	hu->hdev->send(hu->hdev, skb);
> > +
> > +	if (!wait_for_completion_interruptible_timeout(&btdev->init_completion,
> > +		msecs_to_jiffies(10000))) {
> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	if (btdev->init_error < 0)
> > +		return btdev->init_error;
> > +
> > +	/* Change to operational settings */
> > +	hci_uart_set_flow_control(hu, true); // disable flow control
> 
> Please use a proper comment that explains also
> disabling flow control.

ok.

> > +
> > +	/* setup negotiated max. baudrate */
> > +	hci_uart_set_baudrate(hu, MAX_BAUD_RATE);
> > +
> > +	err = hci_uart_wait_for_cts(hu, true, 100);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	hci_uart_set_flow_control(hu, false); // re-enable flow control
> > +
> > +	dev_dbg(hu->tty->dev, "Negotiation successful...\n");
> > +
> > +	return 0;
> > +}
> > +
> > +static int nokia_setup_fw(struct hci_uart *hu)
> > +{
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +	const struct firmware *fw;
> > +	const u8 *fw_ptr;
> > +	size_t fw_size;
> > +	int err;
> > +
> > +	BT_DBG("hu %p", hu);
> > +
> > +	err = request_firmware(&fw, nokia_get_fw_name(btdev), hu->tty->dev);
> 
> So does this nokia_get_fw_name really needs to be a separate
> function? Or can this just be done right here in this function? I
> prefer it to be done where it is actually used. Unless you use
> that name in many places.

I inlined it and dropped CSR support.

> > +	if (err < 0) {
> > +		BT_ERR("%s: Failed to load Nokia firmware file (%d)",
> > +		       hu->hdev->name, err);
> > +		return err;
> > +	}
> > +
> > +	fw_ptr = fw->data;
> > +	fw_size = fw->size;
> > +
> > +	while (fw_size >= 4) {
> > +		u16 pkt_size = get_unaligned_le16(fw_ptr);
> > +		u8 pkt_type = fw_ptr[2];
> > +		const struct hci_command_hdr *cmd;
> > +		u16 opcode;
> > +		struct sk_buff *skb;
> > +
> > +		switch (pkt_type) {
> > +		case HCI_COMMAND_PKT:
> > +			cmd = (struct hci_command_hdr *)(fw_ptr + 3);
> > +			opcode = le16_to_cpu(cmd->opcode);
> > +
> > +			skb = __hci_cmd_sync(hu->hdev, opcode, cmd->plen,
> > +					     fw_ptr + 3 + HCI_COMMAND_HDR_SIZE,
> > +					     HCI_INIT_TIMEOUT);
> > +			if (IS_ERR(skb)) {
> > +				err = PTR_ERR(skb);
> > +				BT_ERR("%s: Firmware command %04x failed (%d)",
> > +				       hu->hdev->name, opcode, err);
> > +				goto done;
> > +			}
> > +			kfree_skb(skb);
> > +			break;
> > +		case HCI_NOKIA_RADIO_PKT:
> 
> Are you sure you can ignore the RADIO_PKT commands. They are used
> to set up the FM radio parts of the chip. They are standard HCI
> commands (in the case of Broadcom at least). At minimum it should
> be added a comment here that you are ignoring them on purpose.

I got the driver working on N950. I think it does not make use of
the radio packets at all. On N900 they may be needed, though. I do
not reach far enough in the firmware loading process to know for
sure.

If I remember correctly your template driver does bundle it together
with HCI_COMMAND_PKT, but that does not work, since HCI_NOKIA_RADIO_PKT
opcode size is u8 instead of u16. I ignored it for now, since I
could not properly test it.

> > +		case HCI_NOKIA_NEG_PKT:
> > +		case HCI_NOKIA_ALIVE_PKT:
> 
> And here I would also a comment on why are we ignore these
> commands and driving this all by ourselves.

I think we could use the packets from the firmware instead
of doing it manually (On N900 they are bit identical to the
manually generated one - On N950 I have not yet checked), but
until N900 works having it coded explicitly helps debugging.

> > +			break;
> > +		}
> > +
> > +		fw_ptr += pkt_size + 2;
> > +		fw_size -= pkt_size + 2;
> > +	}
> > +
> > +done:
> > +	release_firmware(fw);
> > +	return err;
> > +}
> > +
> > +static int nokia_setup(struct hci_uart *hu)
> > +{
> > +	int err;
> > +
> > +	pm_runtime_get_sync(hu->tty->dev);
> > +
> > +	dev_dbg(hu->tty->dev, "Nokia H4+ protocol setup...\n");
> > +
> > +	/* 0. reset connection */
> > +	err = nokia_reset(hu);
> > +	if (err < 0) {
> > +		dev_err(hu->tty->dev, "Reset failed: %d\n", err);
> > +		goto out;
> > +	}
> > +
> > +	/* 1. negotiate speed etc */
> > +	err = nokia_send_negotiation(hu);
> > +	if (err < 0) {
> > +		dev_err(hu->tty->dev, "Negotiation failed: %d\n", err);
> > +		goto out;
> > +	}
> > +
> > +	/* 2. verify correct setup using alive packet */
> > +	err = nokia_send_alive_packet(hu);
> > +	if (err < 0) {
> > +		dev_err(hu->tty->dev, "Alive check failed: %d\n", err);
> > +		goto out;
> > +	}
> > +
> > +	/* 3. send firmware */
> > +	err = nokia_setup_fw(hu);
> > +	if (err < 0) {
> > +		dev_err(hu->tty->dev, "Could not setup FW: %d\n", err);
> > +		goto out;
> > +	}
> > +
> > +	hci_uart_set_flow_control(hu, true);
> > +	hci_uart_set_baudrate(hu, BC4_MAX_BAUD_RATE);
> 
> I think this variable needs a better name if
> it is common for all vendors.

It is common. I will rename it to MAX_BAUD_RATE and
old MAX_BAUD_RATE to SETUP_BAUD_RATE.

> > +	hci_uart_set_flow_control(hu, false);
> > +
> > +	dev_dbg(hu->tty->dev, "Nokia H4+ protocol setup done!\n");
> > +
> > +	/*
> > +	 * TODO:
> > +	 * disable wakeup_bt at this point and automatically enable it when
> > +	 * data is about to be written until all data has been written (+ some
> > +	 * delay).
> > +	 *
> > +	 * Since this is not yet support by the uart/tty kernel framework we
> > +	 * will always keep enabled the wakeup_bt gpio for now, so that the
> > +	 * bluetooth chip will never transit into idle modes.
> > +	 */
> > +
> > +out:
> > +	pm_runtime_put(hu->tty->dev);
> > +
> > +	return err;
> > +}
> > +
> > +static int nokia_open(struct hci_uart *hu)
> > +{
> > +	struct device *serialdev = hu->tty->dev;
> > +	struct nokia_bt_dev *btdev;
> > +	struct device *uartbtdev;
> > +	int err;
> > +
> > +	btdev = kzalloc(sizeof(*btdev), GFP_KERNEL);
> > +	if (!btdev)
> > +		return -ENOMEM;
> > +
> > +	btdev->hu = hu;
> > +
> > +	skb_queue_head_init(&btdev->txq);
> > +
> > +	uartbtdev = device_find_child(serialdev, NULL, btdev_match);
> > +	if (!uartbtdev) {
> > +		dev_err(serialdev, "bluetooth device node not found!\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	btdev->btdata = dev_get_drvdata(uartbtdev);
> > +	if (!btdev->btdata)
> > +		return -EINVAL;
> > +
> > +	hu->priv = btdev;
> > +
> > +	/* register handler for host wakeup gpio */
> > +	btdev->wake_irq = gpiod_to_irq(btdev->btdata->wakeup_host);
> > +	err = request_threaded_irq(btdev->wake_irq, NULL, wakeup_handler,
> > +		IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > +		"wakeup", btdev);
> > +	if (err) {
> > +		gpiod_set_value(btdev->btdata->reset, 0);
> > +		gpiod_set_value(btdev->btdata->wakeup_bt, 0);
> > +		return err;
> > +	}
> > +
> > +	dev_dbg(serialdev, "Nokia H4+ protocol initialized with %s!\n",
> > +		dev_name(uartbtdev));
> > +
> > +	pm_runtime_enable(hu->tty->dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static int nokia_flush(struct hci_uart *hu)
> > +{
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +
> > +	BT_DBG("hu %p", hu);
> > +
> > +	skb_queue_purge(&btdev->txq);
> > +
> > +	return 0;
> > +}
> > +
> > +static int nokia_close(struct hci_uart *hu)
> > +{
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +
> > +	hu->priv = NULL;
> > +
> > +	BT_DBG("hu %p", hu);
> > +
> > +	skb_queue_purge(&btdev->txq);
> > +
> > +	kfree_skb(btdev->rx_skb);
> > +
> > +	free_irq(btdev->wake_irq, btdev);
> > +
> > +	/* disable module */
> > +	gpiod_set_value(btdev->btdata->reset, 0);
> > +	gpiod_set_value(btdev->btdata->wakeup_bt, 0);
> > +
> > +	hu->priv = NULL;
> > +	kfree(btdev);
> > +
> > +	pm_runtime_disable(hu->tty->dev);
> > +
> > +	return 0;
> > +}
> > +
> > +/* Enqueue frame for transmittion (padding, crc, etc) */
> > +static int nokia_enqueue(struct hci_uart *hu, struct sk_buff *skb)
> > +{
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +	int err;
> > +
> > +	BT_DBG("hu %p skb %p", hu, skb);
> > +
> > +	/* Prepend skb with frame type */
> > +	memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
> > +
> > +	/* Packets must be word aligned */
> > +	if (skb->len % 2) {
> > +		err = skb_pad(skb, 1);
> > +		if (err)
> > +			return err;
> > +		*skb_put(skb, 1) = 0x00;
> > +	}
> > +
> > +	skb_queue_tail(&btdev->txq, skb);
> > +
> > +	return 0;
> > +}
> > +
> > +static int nokia_recv_negotiation_packet(struct hci_dev *hdev,
> > +					 struct sk_buff *skb)
> > +{
> > +	struct hci_uart *hu = hci_get_drvdata(hdev);
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +	struct hci_nokia_neg_hdr *hdr;
> > +	struct hci_nokia_neg_evt *evt;
> > +	int ret = 0;
> > +
> > +	hdr = (struct hci_nokia_neg_hdr *)skb->data;
> > +	if (hdr->dlen != sizeof(*evt)) {
> > +		btdev->init_error = -EIO;
> > +		ret = -EIO;
> > +		goto finish_neg;
> > +	}
> > +
> > +	evt = (struct hci_nokia_neg_evt *)skb_pull(skb, sizeof(*hdr));
> > +
> > +	if (evt->ack != NOKIA_NEG_ACK) {
> > +		dev_err(hu->tty->dev, "Could not negotiate hci_nokia settings\n");
> > +		btdev->init_error = -EINVAL;
> > +	}
> > +
> > +	btdev->man_id = evt->man_id;
> > +	btdev->ver_id = evt->ver_id;
> > +
> > +	dev_dbg(hu->tty->dev, "NOKIA negotiation:\n");
> > +	dev_dbg(hu->tty->dev, "\tbaudrate = %u\n", evt->baud);
> > +	dev_dbg(hu->tty->dev, "\tsystem clock = %u\n", evt->sys_clk);
> > +	dev_dbg(hu->tty->dev, "\tmanufacturer id = %u\n", evt->man_id);
> > +	dev_dbg(hu->tty->dev, "\tversion id = %u\n", evt->ver_id);
> > +
> > +finish_neg:
> > +	complete(&btdev->init_completion);
> > +	kfree_skb(skb);
> > +	return ret;
> > +}
> > +
> > +static int nokia_recv_alive_packet(struct hci_dev *hdev, struct sk_buff *skb)
> > +{
> > +	struct hci_uart *hu = hci_get_drvdata(hdev);
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +	struct hci_nokia_alive_hdr *hdr;
> > +	struct hci_nokia_alive_pkt *pkt;
> > +	int ret = 0;
> > +
> > +	hdr = (struct hci_nokia_alive_hdr *)skb->data;
> > +	if (hdr->dlen != sizeof(*pkt)) {
> > +		dev_err(hu->tty->dev, "Corrupted alive message\n");
> > +		btdev->init_error = -EIO;
> > +		ret = -EIO;
> > +		goto finish_alive;
> > +	}
> > +
> > +	pkt = (struct hci_nokia_alive_pkt *)skb_pull(skb, sizeof(*hdr));
> > +
> > +	if (pkt->mid != NOKIA_ALIVE_RESP) {
> > +		dev_err(hu->tty->dev, "Invalid alive response: 0x%02x!\n",
> > +			pkt->mid);
> > +		btdev->init_error = -EINVAL;
> > +		goto finish_alive;
> > +	}
> > +
> > +	dev_dbg(hu->tty->dev, "Received alive packet!\n");
> > +
> > +finish_alive:
> > +	complete(&btdev->init_completion);
> > +	kfree_skb(skb);
> > +	return ret;
> > +}
> > +
> > +static int nokia_recv_radio(struct hci_dev *hdev, struct sk_buff *skb)
> > +{
> > +	/* Packets received on the dedicated radio channel are
> > +	 * HCI events and so feed them back into the core.
> > +	 */
> > +	bt_cb(skb)->pkt_type = HCI_EVENT_PKT;
> 
> I think using hci_skb_pkt_type(skb) is correct here as well.

ok.

> > +	return hci_recv_frame(hdev, skb);
> > +}
> > +
> > +/* Recv data */
> > +static const struct h4_recv_pkt nokia_recv_pkts[] = {
> > +	{ NOKIA_RECV_ACL,	.recv = hci_recv_frame },
> > +	{ NOKIA_RECV_SCO,	.recv = hci_recv_frame },
> > +	{ NOKIA_RECV_EVENT,	.recv = hci_recv_frame },
> > +	{ NOKIA_RECV_ALIVE,	.recv = nokia_recv_alive_packet },
> > +	{ NOKIA_RECV_NEG,	.recv = nokia_recv_negotiation_packet },
> > +	{ NOKIA_RECV_RADIO,	.recv = nokia_recv_radio },
> > +};
> > +
> > +static int nokia_recv(struct hci_uart *hu, const void *data, int count)
> > +{
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +	int err;
> > +
> > +	if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
> > +		return -EUNATCH;
> > +
> > +	btdev->rx_skb = h4_recv_buf(hu->hdev, btdev->rx_skb, data, count,
> > +				  nokia_recv_pkts, ARRAY_SIZE(nokia_recv_pkts));
> > +	if (IS_ERR(btdev->rx_skb)) {
> > +		err = PTR_ERR(btdev->rx_skb);
> > +		BT_ERR("%s: Frame reassembly failed (%d)", hu->hdev->name, err);
> > +		btdev->rx_skb = NULL;
> > +		return err;
> > +	}
> > +
> > +	return count;
> > +}
> > +
> > +static struct sk_buff *nokia_dequeue(struct hci_uart *hu)
> > +{
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +
> > +	return skb_dequeue(&btdev->txq);
> > +}
> > +
> > +static const struct hci_uart_proto nokia_proto = {
> > +	.id		= HCI_UART_NOKIA,
> > +	.name		= "Nokia",
> > +	.open		= nokia_open,
> > +	.close		= nokia_close,
> > +	.recv		= nokia_recv,
> > +	.enqueue	= nokia_enqueue,
> > +	.dequeue	= nokia_dequeue,
> > +	.flush		= nokia_flush,
> > +	.setup		= nokia_setup,
> > +};
> > +
> > +static int nokia_bluetooth_probe(struct platform_device *pdev)
> > +{
> > +	struct nokia_uart_dev *btdata;
> > +	struct device *bcmdev = &pdev->dev;
> > +	struct clk *sysclk;
> > +	int err = 0;
> > +
> > +	if(!bcmdev->parent) {
> > +		dev_err(bcmdev, "parent device missing!\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	btdata = devm_kmalloc(bcmdev, sizeof(*btdata), GFP_KERNEL);
> > +	if(!btdata)
> > +		return -ENOMEM;
> > +
> > +	btdata->dev = bcmdev;
> > +	dev_set_drvdata(bcmdev, btdata);
> > +
> > +	btdata->port = dev_get_drvdata(bcmdev->parent);
> > +	if(!btdata->port) {
> > +		dev_err(bcmdev, "port data missing in parent device!\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	btdata->reset = devm_gpiod_get(bcmdev, "reset", GPIOD_OUT_LOW);
> > +	if (IS_ERR(btdata->reset)) {
> > +		err = PTR_ERR(btdata->reset);
> > +		dev_err(bcmdev, "could not get reset gpio: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +	btdata->wakeup_host = devm_gpiod_get(bcmdev, "host-wakeup", GPIOD_IN);
> > +	if (IS_ERR(btdata->wakeup_host)) {
> > +		err = PTR_ERR(btdata->wakeup_host);
> > +		dev_err(bcmdev, "could not get host wakeup gpio: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +
> > +	btdata->wakeup_bt = devm_gpiod_get(bcmdev, "bluetooth-wakeup",
> > +					    GPIOD_OUT_LOW);
> > +	if (IS_ERR(btdata->wakeup_bt)) {
> > +		err = PTR_ERR(btdata->wakeup_bt);
> > +		dev_err(bcmdev, "could not get BT wakeup gpio: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +	sysclk = devm_clk_get(bcmdev, "sysclk");
> > +	if (IS_ERR(sysclk)) {
> > +		err = PTR_ERR(sysclk);
> > +		dev_err(bcmdev, "could not get sysclk: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +	clk_prepare_enable(sysclk);
> > +	btdata->sysclk_speed = clk_get_rate(sysclk);
> > +	clk_disable_unprepare(sysclk);
> > +
> > +	dev_dbg(bcmdev, "parent uart: %s\n", dev_name(bcmdev->parent));
> > +	dev_dbg(bcmdev, "sysclk speed: %ld kHz\n", btdata->sysclk_speed / 1000);
> > +
> > +	/* TODO: open tty and setup line disector from kernel-side */
> > +
> > +	return err;
> > +}
> > +
> > +static const struct of_device_id nokia_bluetooth_of_match[] = {
> > +	{ .compatible = "nokia,brcm,bcm2048", },
> > +	{ .compatible = "nokia,ti,wl1271-bluetooth", },
> 
> Where is the CSR BC4 one here? I prefer if we only have support
> for the ones that are actually supported and detected. We can
> easily extend things later.

I will drop CSR stuff. I don't have a device to test it.

> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, nokia_bluetooth_of_match);
> > +
> > +static struct platform_driver platform_nokia_driver = {
> > +	.driver = {
> > +		.name = "nokia-bluetooth",
> > +		.of_match_table = nokia_bluetooth_of_match,
> > +	},
> > +	.probe = nokia_bluetooth_probe,
> > +};
> > +
> > +int __init nokia_init(void)
> > +{
> > +	platform_driver_register(&platform_nokia_driver);
> > +	return hci_uart_register_proto(&nokia_proto);
> > +}
> > +
> > +int __exit nokia_deinit(void)
> > +{
> > +	platform_driver_unregister(&platform_nokia_driver);
> > +	return hci_uart_unregister_proto(&nokia_proto);
> > +}
> > diff --git a/drivers/bluetooth/hci_nokia.h b/drivers/bluetooth/hci_nokia.h
> > new file mode 100644
> > index 000000000000..8c4d307840e5
> > --- /dev/null
> > +++ b/drivers/bluetooth/hci_nokia.h
> > @@ -0,0 +1,140 @@
> > +/*
> > + *  Copyright (C) 2016 Sebastian Reichel <sre@kernel.org>
> > + *
> > + *  This program is free software; you can redistribute it and/or modify
> > + *  it under the terms of the GNU General Public License as published by
> > + *  the Free Software Foundation; either version 2 of the License, or
> > + *  (at your option) any later version.
> > + *
> > + *  This program is distributed in the hope that it will be useful,
> > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + *  GNU General Public License for more details.
> > + */
> > +
> > +#ifndef __HCI_NOKIA_H
> > +#define __HCI_NOKIA_H
> 
> Lets not do a separate header here. Just move this all into
> hci_nokia.c. There is really zero benefit in the header.

ok.

> > +
> > +#define NOKIA_ID_CSR		0x02
> > +#define NOKIA_ID_BCM2048	0x04
> > +#define NOKIA_ID_TI1271		0x31
> > +
> > +#define FIRMWARE_CSR		"nokia/bc4fw.bin"
> 
> If the CSR ones are not yet supported, then leave them out for
> now. We can add this later.
> 
> > +#define FIRMWARE_BCM2048	"nokia/bcmfw.bin"
> > +#define FIRMWARE_TI1271		"nokia/ti1273.bin"
> > +
> > +#define NOKIA_BCM_BDADDR	0xfc01
> 
> We have btbcm.[ch] for this.

ah this is a leftover. Currently the driver does not set
set_bdaddr() callback, since it differs between ti and bcm backend.
It looks like btbcm_set_bdaddr() can be used for the broadcom based
chips, though.

> > +#define HCI_NOKIA_NEG_PKT	0x06
> > +#define HCI_NOKIA_ALIVE_PKT	0x07
> > +#define HCI_NOKIA_RADIO_PKT	0x08
> > +
> > +#define HCI_NOKIA_NEG_HDR_SIZE		1
> > +#define HCI_NOKIA_MAX_NEG_SIZE		255
> > +#define HCI_NOKIA_ALIVE_HDR_SIZE	1
> > +#define HCI_NOKIA_MAX_ALIVE_SIZE	255
> > +#define HCI_NOKIA_RADIO_HDR_SIZE	2
> > +#define HCI_NOKIA_MAX_RADIO_SIZE	255
> > +
> > +#define NOKIA_PROTO_PKT		0x44
> > +#define NOKIA_PROTO_BYTE	0x4c
> > +
> > +#define NOKIA_NEG_REQ		0x00
> > +#define NOKIA_NEG_ACK		0x20
> > +#define NOKIA_NEG_NAK		0x40
> > +
> > +#define H4_TYPE_SIZE		1
> 
> I am not sure this define adds any overall value to the code.
> 
> > +
> > +#define NOKIA_RECV_ACL \
> > +	H4_RECV_ACL, \
> > +	.wordaligned = true
> > +
> > +#define NOKIA_RECV_SCO \
> > +	H4_RECV_SCO, \
> > +	.wordaligned = true
> > +
> > +#define NOKIA_RECV_EVENT \
> > +	H4_RECV_EVENT, \
> > +	.wordaligned = true
> > +
> > +#define NOKIA_RECV_ALIVE \
> > +	.type = HCI_NOKIA_ALIVE_PKT, \
> > +	.hlen = HCI_NOKIA_ALIVE_HDR_SIZE, \
> > +	.loff = 0, \
> > +	.lsize = 1, \
> > +	.maxlen = HCI_NOKIA_MAX_ALIVE_SIZE, \
> > +	.wordaligned = true
> > +
> > +#define NOKIA_RECV_NEG \
> > +	.type = HCI_NOKIA_NEG_PKT, \
> > +	.hlen = HCI_NOKIA_NEG_HDR_SIZE, \
> > +	.loff = 0, \
> > +	.lsize = 1, \
> > +	.maxlen = HCI_NOKIA_MAX_NEG_SIZE, \
> > +	.wordaligned = true
> > +
> > +#define NOKIA_RECV_RADIO \
> > +	.type = HCI_NOKIA_RADIO_PKT, \
> > +	.hlen = HCI_NOKIA_RADIO_HDR_SIZE, \
> > +	.loff = 1, \
> > +	.lsize = 1, \
> > +	.maxlen = HCI_NOKIA_MAX_RADIO_SIZE, \
> > +	.wordaligned = true
> 
> For this ones I would have use the HCI event ones.
> My original patch had this:
> 
> +#define NOK_RECV_NEG \
> +	.type = NOK_NEG_PKT, \
> +	.hlen = NOK_NEG_HDR_SIZE, \
> +	.loff = 0, \
> +	.lsize = 1, \
> +	.maxlen = HCI_MAX_EVENT_SIZE
> +
> +#define NOK_RECV_ALIVE \
> +	.type = NOK_ALIVE_PKT, \
> +	.hlen = NOK_ALIVE_HDR_SIZE, \
> +	.loff = 0, \
> +	.lsize = 1, \
> +	.maxlen = HCI_MAX_EVENT_SIZE
> +
> +#define NOK_RECV_RADIO \
> +	.type = NOK_RADIO_PKT, \
> +	.hlen = HCI_EVENT_HDR_SIZE, \
> +	.loff = 1, \
> +	.lsize = 1, \
> +	.maxlen = HCI_MAX_EVENT_SIZE
> +
> +static const struct h4_recv_pkt nok_recv_pkts[] = {
> +	{ H4_RECV_ACL,    .recv = hci_recv_frame },
> +	{ H4_RECV_SCO,    .recv = hci_recv_frame },
> +	{ H4_RECV_EVENT,  .recv = hci_recv_frame },
> +	{ NOK_RECV_NEG,   .recv = nok_recv_neg   },
> +	{ NOK_RECV_ALIVE, .recv = nok_recv_alive },
> +	{ NOK_RECV_RADIO, .recv = nok_recv_radio },
> 
> With just these simple defines at the top:
> 
> +#define NOK_NEG_PKT	0x06
> +#define NOK_ALIVE_PKT	0x07
> +#define NOK_RADIO_PKT	0x08
> +
> +#define NOK_NEG_HDR_SIZE	1
> +#define NOK_ALIVE_HDR_SIZE	1
> 
> And I would prefer if we keep it like that.

ok. I used explicit defines, since it looks like
a copy/paste error otherwise.

> > +
> > +struct hci_nokia_neg_hdr {
> > +	__u8	dlen;
> > +} __packed;
> > +
> > +struct hci_nokia_neg_cmd {
> > +	__u8	ack;
> > +	__u16	baud;
> > +	__u16	unused1;
> > +	__u8	proto;
> > +	__u16	sys_clk;
> > +	__u16	unused2;
> > +} __packed;
> > +
> > +static inline struct hci_nokia_neg_hdr *hci_nokia_neg_hdr(const struct sk_buff *skb)
> > +{
> > +	return (struct hci_nokia_neg_hdr *) skb->data;
> > +}
> 
> What good is this inline? A define would be way better, if really
> needed.

I will drop hci_nokia_neg_hdr() and hci_nokia_alive_hdr() inlines

> [...]

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 25962 bytes --]

Hi Marcel,

On Tue, Aug 16, 2016 at 09:02:14AM +0200, Marcel Holtmann wrote:
> [...]
> > +#include <linux/module.h>
> > +
> > +#include <linux/clk.h>
> > +#include <linux/kernel.h>
> > +#include <linux/init.h>
> > +#include <linux/types.h>
> > +#include <linux/fcntl.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/ptrace.h>
> > +#include <linux/poll.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/firmware.h>
> > +#include <linux/slab.h>
> > +#include <linux/tty.h>
> > +#include <linux/errno.h>
> > +#include <linux/string.h>
> > +#include <linux/signal.h>
> > +#include <linux/ioctl.h>
> > +#include <linux/skbuff.h>
> > +#include <linux/delay.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <linux/gpio/consumer.h>
> > +
> > +#include <linux/unaligned/le_struct.h>
> 
> are you sure all these includes are needed?

No. A few of them are from previous version of
the driver. I will clean this up in the next
version.

> [...]
> > +static int hci_uart_wait_for_cts(struct hci_uart *hu, bool state,
> > +				 int timeout_ms)
> > +{
> > +	unsigned long timeout;
> > +	int signal;
> > +
> > +	timeout = jiffies + msecs_to_jiffies(timeout_ms);
> > +	for (;;) {
> > +		signal = hu->tty->ops->tiocmget(hu->tty) & TIOCM_CTS;
> > +		if (!!signal == !!state) {
> > +			dev_dbg(hu->tty->dev, "wait for cts... received!\n");
> > +			return 0;
> > +		}
> > +		if (time_after(jiffies, timeout)) {
> > +			dev_dbg(hu->tty->dev, "wait for cts... timeout!\n");
> > +			return -ETIMEDOUT;
> > +		}
> > +		usleep_range(1000, 2000);
> > +	}
> > +}
> 
> This is a super odd function. No return value since we essentially
> have an endless loop.

I will rewrite it, so that the loop condition checks for timeout.

> > +
> > +static int btdev_match(struct device *child, void *data)
> > +{
> > +	if (!strcmp(child->driver->name, "nokia-bluetooth"))
> > +		return 1;
> > +	else
> > +		return 0;
> > +}
> 
> Anything wrong with just return !strcmp?

No. I had initially debug prints in the cases.

> > +static int nokia_send_alive_packet(struct hci_uart *hu)
> > +{
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +	struct hci_nokia_alive_hdr *hdr;
> > +	struct hci_nokia_alive_pkt *pkt;
> > +	struct sk_buff *skb;
> > +	int len;
> > +
> > +	dev_dbg(hu->tty->dev, "Sending alive packet...\n");
> > +
> > +	init_completion(&btdev->init_completion);
> > +
> > +	len = H4_TYPE_SIZE + sizeof(*hdr) + sizeof(*pkt);
> > +	skb = bt_skb_alloc(len, GFP_KERNEL);
> > +	if (!skb)
> > +		return -ENOMEM;
> > +
> > +	hci_skb_pkt_type(skb) = HCI_NOKIA_ALIVE_PKT;
> > +	memset(skb->data, 0x00, len);
> > +
> > +	hdr = (struct hci_nokia_alive_hdr *)skb_put(skb, sizeof(*hdr));
> > +	hdr->dlen = sizeof(*pkt);
> > +	pkt = (struct hci_nokia_alive_pkt *)skb_put(skb, sizeof(*pkt));
> > +	pkt->mid = NOKIA_ALIVE_REQ;
> > +
> > +	hu->hdev->send(hu->hdev, skb);
> 
> I am not sure we want these to go through the Bluetooth core
> packet sending. They are not standard HCI packet and should stay
> within the driver. If you send them through the core they will
> cause problems with the monitor interface.

ok. I will directly call nokia_enqueue().

> > +
> > +	if (!wait_for_completion_interruptible_timeout(&btdev->init_completion,
> > +		msecs_to_jiffies(1000))) {
> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	if (btdev->init_error < 0)
> > +		return btdev->init_error;
> > +
> > +	return 0;
> > +}
> > +
> > +static int nokia_send_negotiation(struct hci_uart *hu)
> > +{
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +	struct hci_nokia_neg_cmd *neg_cmd;
> > +	struct hci_nokia_neg_hdr *neg_hdr;
> > +	struct sk_buff *skb;
> > +	int len, err;
> > +	u16 baud = DIV_ROUND_CLOSEST(BT_BAUDRATE_DIVIDER, MAX_BAUD_RATE);
> > +	int sysclk = btdev->btdata->sysclk_speed / 1000;
> > +
> > +	dev_dbg(hu->tty->dev, "Sending negotiation...\n");
> > +
> > +	len = H4_TYPE_SIZE + sizeof(*neg_hdr) + sizeof(*neg_cmd);
> > +	skb = bt_skb_alloc(len, GFP_KERNEL);
> > +	if (!skb)
> > +		return -ENOMEM;
> > +
> > +	hci_skb_pkt_type(skb) = HCI_NOKIA_NEG_PKT;
> > +
> > +	neg_hdr = (struct hci_nokia_neg_hdr *)skb_put(skb, sizeof(*neg_hdr));
> > +	neg_hdr->dlen = sizeof(*neg_cmd);
> > +
> > +	neg_cmd = (struct hci_nokia_neg_cmd *)skb_put(skb, sizeof(*neg_cmd));
> > +	neg_cmd->ack = NOKIA_NEG_REQ;
> > +	neg_cmd->baud = cpu_to_le16(baud);
> > +	neg_cmd->unused1 = 0x0000;
> > +	neg_cmd->proto = NOKIA_PROTO_BYTE;
> > +	neg_cmd->sys_clk = cpu_to_le16(sysclk);
> > +	neg_cmd->unused2 = 0x0000;
> > +
> > +	btdev->init_error = 0;
> > +	init_completion(&btdev->init_completion);
> > +
> > +	hu->hdev->send(hu->hdev, skb);
> > +
> > +	if (!wait_for_completion_interruptible_timeout(&btdev->init_completion,
> > +		msecs_to_jiffies(10000))) {
> > +		return -ETIMEDOUT;
> > +	}
> > +
> > +	if (btdev->init_error < 0)
> > +		return btdev->init_error;
> > +
> > +	/* Change to operational settings */
> > +	hci_uart_set_flow_control(hu, true); // disable flow control
> 
> Please use a proper comment that explains also
> disabling flow control.

ok.

> > +
> > +	/* setup negotiated max. baudrate */
> > +	hci_uart_set_baudrate(hu, MAX_BAUD_RATE);
> > +
> > +	err = hci_uart_wait_for_cts(hu, true, 100);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	hci_uart_set_flow_control(hu, false); // re-enable flow control
> > +
> > +	dev_dbg(hu->tty->dev, "Negotiation successful...\n");
> > +
> > +	return 0;
> > +}
> > +
> > +static int nokia_setup_fw(struct hci_uart *hu)
> > +{
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +	const struct firmware *fw;
> > +	const u8 *fw_ptr;
> > +	size_t fw_size;
> > +	int err;
> > +
> > +	BT_DBG("hu %p", hu);
> > +
> > +	err = request_firmware(&fw, nokia_get_fw_name(btdev), hu->tty->dev);
> 
> So does this nokia_get_fw_name really needs to be a separate
> function? Or can this just be done right here in this function? I
> prefer it to be done where it is actually used. Unless you use
> that name in many places.

I inlined it and dropped CSR support.

> > +	if (err < 0) {
> > +		BT_ERR("%s: Failed to load Nokia firmware file (%d)",
> > +		       hu->hdev->name, err);
> > +		return err;
> > +	}
> > +
> > +	fw_ptr = fw->data;
> > +	fw_size = fw->size;
> > +
> > +	while (fw_size >= 4) {
> > +		u16 pkt_size = get_unaligned_le16(fw_ptr);
> > +		u8 pkt_type = fw_ptr[2];
> > +		const struct hci_command_hdr *cmd;
> > +		u16 opcode;
> > +		struct sk_buff *skb;
> > +
> > +		switch (pkt_type) {
> > +		case HCI_COMMAND_PKT:
> > +			cmd = (struct hci_command_hdr *)(fw_ptr + 3);
> > +			opcode = le16_to_cpu(cmd->opcode);
> > +
> > +			skb = __hci_cmd_sync(hu->hdev, opcode, cmd->plen,
> > +					     fw_ptr + 3 + HCI_COMMAND_HDR_SIZE,
> > +					     HCI_INIT_TIMEOUT);
> > +			if (IS_ERR(skb)) {
> > +				err = PTR_ERR(skb);
> > +				BT_ERR("%s: Firmware command %04x failed (%d)",
> > +				       hu->hdev->name, opcode, err);
> > +				goto done;
> > +			}
> > +			kfree_skb(skb);
> > +			break;
> > +		case HCI_NOKIA_RADIO_PKT:
> 
> Are you sure you can ignore the RADIO_PKT commands. They are used
> to set up the FM radio parts of the chip. They are standard HCI
> commands (in the case of Broadcom at least). At minimum it should
> be added a comment here that you are ignoring them on purpose.

I got the driver working on N950. I think it does not make use of
the radio packets at all. On N900 they may be needed, though. I do
not reach far enough in the firmware loading process to know for
sure.

If I remember correctly your template driver does bundle it together
with HCI_COMMAND_PKT, but that does not work, since HCI_NOKIA_RADIO_PKT
opcode size is u8 instead of u16. I ignored it for now, since I
could not properly test it.

> > +		case HCI_NOKIA_NEG_PKT:
> > +		case HCI_NOKIA_ALIVE_PKT:
> 
> And here I would also a comment on why are we ignore these
> commands and driving this all by ourselves.

I think we could use the packets from the firmware instead
of doing it manually (On N900 they are bit identical to the
manually generated one - On N950 I have not yet checked), but
until N900 works having it coded explicitly helps debugging.

> > +			break;
> > +		}
> > +
> > +		fw_ptr += pkt_size + 2;
> > +		fw_size -= pkt_size + 2;
> > +	}
> > +
> > +done:
> > +	release_firmware(fw);
> > +	return err;
> > +}
> > +
> > +static int nokia_setup(struct hci_uart *hu)
> > +{
> > +	int err;
> > +
> > +	pm_runtime_get_sync(hu->tty->dev);
> > +
> > +	dev_dbg(hu->tty->dev, "Nokia H4+ protocol setup...\n");
> > +
> > +	/* 0. reset connection */
> > +	err = nokia_reset(hu);
> > +	if (err < 0) {
> > +		dev_err(hu->tty->dev, "Reset failed: %d\n", err);
> > +		goto out;
> > +	}
> > +
> > +	/* 1. negotiate speed etc */
> > +	err = nokia_send_negotiation(hu);
> > +	if (err < 0) {
> > +		dev_err(hu->tty->dev, "Negotiation failed: %d\n", err);
> > +		goto out;
> > +	}
> > +
> > +	/* 2. verify correct setup using alive packet */
> > +	err = nokia_send_alive_packet(hu);
> > +	if (err < 0) {
> > +		dev_err(hu->tty->dev, "Alive check failed: %d\n", err);
> > +		goto out;
> > +	}
> > +
> > +	/* 3. send firmware */
> > +	err = nokia_setup_fw(hu);
> > +	if (err < 0) {
> > +		dev_err(hu->tty->dev, "Could not setup FW: %d\n", err);
> > +		goto out;
> > +	}
> > +
> > +	hci_uart_set_flow_control(hu, true);
> > +	hci_uart_set_baudrate(hu, BC4_MAX_BAUD_RATE);
> 
> I think this variable needs a better name if
> it is common for all vendors.

It is common. I will rename it to MAX_BAUD_RATE and
old MAX_BAUD_RATE to SETUP_BAUD_RATE.

> > +	hci_uart_set_flow_control(hu, false);
> > +
> > +	dev_dbg(hu->tty->dev, "Nokia H4+ protocol setup done!\n");
> > +
> > +	/*
> > +	 * TODO:
> > +	 * disable wakeup_bt at this point and automatically enable it when
> > +	 * data is about to be written until all data has been written (+ some
> > +	 * delay).
> > +	 *
> > +	 * Since this is not yet support by the uart/tty kernel framework we
> > +	 * will always keep enabled the wakeup_bt gpio for now, so that the
> > +	 * bluetooth chip will never transit into idle modes.
> > +	 */
> > +
> > +out:
> > +	pm_runtime_put(hu->tty->dev);
> > +
> > +	return err;
> > +}
> > +
> > +static int nokia_open(struct hci_uart *hu)
> > +{
> > +	struct device *serialdev = hu->tty->dev;
> > +	struct nokia_bt_dev *btdev;
> > +	struct device *uartbtdev;
> > +	int err;
> > +
> > +	btdev = kzalloc(sizeof(*btdev), GFP_KERNEL);
> > +	if (!btdev)
> > +		return -ENOMEM;
> > +
> > +	btdev->hu = hu;
> > +
> > +	skb_queue_head_init(&btdev->txq);
> > +
> > +	uartbtdev = device_find_child(serialdev, NULL, btdev_match);
> > +	if (!uartbtdev) {
> > +		dev_err(serialdev, "bluetooth device node not found!\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	btdev->btdata = dev_get_drvdata(uartbtdev);
> > +	if (!btdev->btdata)
> > +		return -EINVAL;
> > +
> > +	hu->priv = btdev;
> > +
> > +	/* register handler for host wakeup gpio */
> > +	btdev->wake_irq = gpiod_to_irq(btdev->btdata->wakeup_host);
> > +	err = request_threaded_irq(btdev->wake_irq, NULL, wakeup_handler,
> > +		IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > +		"wakeup", btdev);
> > +	if (err) {
> > +		gpiod_set_value(btdev->btdata->reset, 0);
> > +		gpiod_set_value(btdev->btdata->wakeup_bt, 0);
> > +		return err;
> > +	}
> > +
> > +	dev_dbg(serialdev, "Nokia H4+ protocol initialized with %s!\n",
> > +		dev_name(uartbtdev));
> > +
> > +	pm_runtime_enable(hu->tty->dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static int nokia_flush(struct hci_uart *hu)
> > +{
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +
> > +	BT_DBG("hu %p", hu);
> > +
> > +	skb_queue_purge(&btdev->txq);
> > +
> > +	return 0;
> > +}
> > +
> > +static int nokia_close(struct hci_uart *hu)
> > +{
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +
> > +	hu->priv = NULL;
> > +
> > +	BT_DBG("hu %p", hu);
> > +
> > +	skb_queue_purge(&btdev->txq);
> > +
> > +	kfree_skb(btdev->rx_skb);
> > +
> > +	free_irq(btdev->wake_irq, btdev);
> > +
> > +	/* disable module */
> > +	gpiod_set_value(btdev->btdata->reset, 0);
> > +	gpiod_set_value(btdev->btdata->wakeup_bt, 0);
> > +
> > +	hu->priv = NULL;
> > +	kfree(btdev);
> > +
> > +	pm_runtime_disable(hu->tty->dev);
> > +
> > +	return 0;
> > +}
> > +
> > +/* Enqueue frame for transmittion (padding, crc, etc) */
> > +static int nokia_enqueue(struct hci_uart *hu, struct sk_buff *skb)
> > +{
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +	int err;
> > +
> > +	BT_DBG("hu %p skb %p", hu, skb);
> > +
> > +	/* Prepend skb with frame type */
> > +	memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
> > +
> > +	/* Packets must be word aligned */
> > +	if (skb->len % 2) {
> > +		err = skb_pad(skb, 1);
> > +		if (err)
> > +			return err;
> > +		*skb_put(skb, 1) = 0x00;
> > +	}
> > +
> > +	skb_queue_tail(&btdev->txq, skb);
> > +
> > +	return 0;
> > +}
> > +
> > +static int nokia_recv_negotiation_packet(struct hci_dev *hdev,
> > +					 struct sk_buff *skb)
> > +{
> > +	struct hci_uart *hu = hci_get_drvdata(hdev);
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +	struct hci_nokia_neg_hdr *hdr;
> > +	struct hci_nokia_neg_evt *evt;
> > +	int ret = 0;
> > +
> > +	hdr = (struct hci_nokia_neg_hdr *)skb->data;
> > +	if (hdr->dlen != sizeof(*evt)) {
> > +		btdev->init_error = -EIO;
> > +		ret = -EIO;
> > +		goto finish_neg;
> > +	}
> > +
> > +	evt = (struct hci_nokia_neg_evt *)skb_pull(skb, sizeof(*hdr));
> > +
> > +	if (evt->ack != NOKIA_NEG_ACK) {
> > +		dev_err(hu->tty->dev, "Could not negotiate hci_nokia settings\n");
> > +		btdev->init_error = -EINVAL;
> > +	}
> > +
> > +	btdev->man_id = evt->man_id;
> > +	btdev->ver_id = evt->ver_id;
> > +
> > +	dev_dbg(hu->tty->dev, "NOKIA negotiation:\n");
> > +	dev_dbg(hu->tty->dev, "\tbaudrate = %u\n", evt->baud);
> > +	dev_dbg(hu->tty->dev, "\tsystem clock = %u\n", evt->sys_clk);
> > +	dev_dbg(hu->tty->dev, "\tmanufacturer id = %u\n", evt->man_id);
> > +	dev_dbg(hu->tty->dev, "\tversion id = %u\n", evt->ver_id);
> > +
> > +finish_neg:
> > +	complete(&btdev->init_completion);
> > +	kfree_skb(skb);
> > +	return ret;
> > +}
> > +
> > +static int nokia_recv_alive_packet(struct hci_dev *hdev, struct sk_buff *skb)
> > +{
> > +	struct hci_uart *hu = hci_get_drvdata(hdev);
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +	struct hci_nokia_alive_hdr *hdr;
> > +	struct hci_nokia_alive_pkt *pkt;
> > +	int ret = 0;
> > +
> > +	hdr = (struct hci_nokia_alive_hdr *)skb->data;
> > +	if (hdr->dlen != sizeof(*pkt)) {
> > +		dev_err(hu->tty->dev, "Corrupted alive message\n");
> > +		btdev->init_error = -EIO;
> > +		ret = -EIO;
> > +		goto finish_alive;
> > +	}
> > +
> > +	pkt = (struct hci_nokia_alive_pkt *)skb_pull(skb, sizeof(*hdr));
> > +
> > +	if (pkt->mid != NOKIA_ALIVE_RESP) {
> > +		dev_err(hu->tty->dev, "Invalid alive response: 0x%02x!\n",
> > +			pkt->mid);
> > +		btdev->init_error = -EINVAL;
> > +		goto finish_alive;
> > +	}
> > +
> > +	dev_dbg(hu->tty->dev, "Received alive packet!\n");
> > +
> > +finish_alive:
> > +	complete(&btdev->init_completion);
> > +	kfree_skb(skb);
> > +	return ret;
> > +}
> > +
> > +static int nokia_recv_radio(struct hci_dev *hdev, struct sk_buff *skb)
> > +{
> > +	/* Packets received on the dedicated radio channel are
> > +	 * HCI events and so feed them back into the core.
> > +	 */
> > +	bt_cb(skb)->pkt_type = HCI_EVENT_PKT;
> 
> I think using hci_skb_pkt_type(skb) is correct here as well.

ok.

> > +	return hci_recv_frame(hdev, skb);
> > +}
> > +
> > +/* Recv data */
> > +static const struct h4_recv_pkt nokia_recv_pkts[] = {
> > +	{ NOKIA_RECV_ACL,	.recv = hci_recv_frame },
> > +	{ NOKIA_RECV_SCO,	.recv = hci_recv_frame },
> > +	{ NOKIA_RECV_EVENT,	.recv = hci_recv_frame },
> > +	{ NOKIA_RECV_ALIVE,	.recv = nokia_recv_alive_packet },
> > +	{ NOKIA_RECV_NEG,	.recv = nokia_recv_negotiation_packet },
> > +	{ NOKIA_RECV_RADIO,	.recv = nokia_recv_radio },
> > +};
> > +
> > +static int nokia_recv(struct hci_uart *hu, const void *data, int count)
> > +{
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +	int err;
> > +
> > +	if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
> > +		return -EUNATCH;
> > +
> > +	btdev->rx_skb = h4_recv_buf(hu->hdev, btdev->rx_skb, data, count,
> > +				  nokia_recv_pkts, ARRAY_SIZE(nokia_recv_pkts));
> > +	if (IS_ERR(btdev->rx_skb)) {
> > +		err = PTR_ERR(btdev->rx_skb);
> > +		BT_ERR("%s: Frame reassembly failed (%d)", hu->hdev->name, err);
> > +		btdev->rx_skb = NULL;
> > +		return err;
> > +	}
> > +
> > +	return count;
> > +}
> > +
> > +static struct sk_buff *nokia_dequeue(struct hci_uart *hu)
> > +{
> > +	struct nokia_bt_dev *btdev = hu->priv;
> > +
> > +	return skb_dequeue(&btdev->txq);
> > +}
> > +
> > +static const struct hci_uart_proto nokia_proto = {
> > +	.id		= HCI_UART_NOKIA,
> > +	.name		= "Nokia",
> > +	.open		= nokia_open,
> > +	.close		= nokia_close,
> > +	.recv		= nokia_recv,
> > +	.enqueue	= nokia_enqueue,
> > +	.dequeue	= nokia_dequeue,
> > +	.flush		= nokia_flush,
> > +	.setup		= nokia_setup,
> > +};
> > +
> > +static int nokia_bluetooth_probe(struct platform_device *pdev)
> > +{
> > +	struct nokia_uart_dev *btdata;
> > +	struct device *bcmdev = &pdev->dev;
> > +	struct clk *sysclk;
> > +	int err = 0;
> > +
> > +	if(!bcmdev->parent) {
> > +		dev_err(bcmdev, "parent device missing!\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	btdata = devm_kmalloc(bcmdev, sizeof(*btdata), GFP_KERNEL);
> > +	if(!btdata)
> > +		return -ENOMEM;
> > +
> > +	btdata->dev = bcmdev;
> > +	dev_set_drvdata(bcmdev, btdata);
> > +
> > +	btdata->port = dev_get_drvdata(bcmdev->parent);
> > +	if(!btdata->port) {
> > +		dev_err(bcmdev, "port data missing in parent device!\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	btdata->reset = devm_gpiod_get(bcmdev, "reset", GPIOD_OUT_LOW);
> > +	if (IS_ERR(btdata->reset)) {
> > +		err = PTR_ERR(btdata->reset);
> > +		dev_err(bcmdev, "could not get reset gpio: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +	btdata->wakeup_host = devm_gpiod_get(bcmdev, "host-wakeup", GPIOD_IN);
> > +	if (IS_ERR(btdata->wakeup_host)) {
> > +		err = PTR_ERR(btdata->wakeup_host);
> > +		dev_err(bcmdev, "could not get host wakeup gpio: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +
> > +	btdata->wakeup_bt = devm_gpiod_get(bcmdev, "bluetooth-wakeup",
> > +					    GPIOD_OUT_LOW);
> > +	if (IS_ERR(btdata->wakeup_bt)) {
> > +		err = PTR_ERR(btdata->wakeup_bt);
> > +		dev_err(bcmdev, "could not get BT wakeup gpio: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +	sysclk = devm_clk_get(bcmdev, "sysclk");
> > +	if (IS_ERR(sysclk)) {
> > +		err = PTR_ERR(sysclk);
> > +		dev_err(bcmdev, "could not get sysclk: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +	clk_prepare_enable(sysclk);
> > +	btdata->sysclk_speed = clk_get_rate(sysclk);
> > +	clk_disable_unprepare(sysclk);
> > +
> > +	dev_dbg(bcmdev, "parent uart: %s\n", dev_name(bcmdev->parent));
> > +	dev_dbg(bcmdev, "sysclk speed: %ld kHz\n", btdata->sysclk_speed / 1000);
> > +
> > +	/* TODO: open tty and setup line disector from kernel-side */
> > +
> > +	return err;
> > +}
> > +
> > +static const struct of_device_id nokia_bluetooth_of_match[] = {
> > +	{ .compatible = "nokia,brcm,bcm2048", },
> > +	{ .compatible = "nokia,ti,wl1271-bluetooth", },
> 
> Where is the CSR BC4 one here? I prefer if we only have support
> for the ones that are actually supported and detected. We can
> easily extend things later.

I will drop CSR stuff. I don't have a device to test it.

> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, nokia_bluetooth_of_match);
> > +
> > +static struct platform_driver platform_nokia_driver = {
> > +	.driver = {
> > +		.name = "nokia-bluetooth",
> > +		.of_match_table = nokia_bluetooth_of_match,
> > +	},
> > +	.probe = nokia_bluetooth_probe,
> > +};
> > +
> > +int __init nokia_init(void)
> > +{
> > +	platform_driver_register(&platform_nokia_driver);
> > +	return hci_uart_register_proto(&nokia_proto);
> > +}
> > +
> > +int __exit nokia_deinit(void)
> > +{
> > +	platform_driver_unregister(&platform_nokia_driver);
> > +	return hci_uart_unregister_proto(&nokia_proto);
> > +}
> > diff --git a/drivers/bluetooth/hci_nokia.h b/drivers/bluetooth/hci_nokia.h
> > new file mode 100644
> > index 000000000000..8c4d307840e5
> > --- /dev/null
> > +++ b/drivers/bluetooth/hci_nokia.h
> > @@ -0,0 +1,140 @@
> > +/*
> > + *  Copyright (C) 2016 Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > + *
> > + *  This program is free software; you can redistribute it and/or modify
> > + *  it under the terms of the GNU General Public License as published by
> > + *  the Free Software Foundation; either version 2 of the License, or
> > + *  (at your option) any later version.
> > + *
> > + *  This program is distributed in the hope that it will be useful,
> > + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + *  GNU General Public License for more details.
> > + */
> > +
> > +#ifndef __HCI_NOKIA_H
> > +#define __HCI_NOKIA_H
> 
> Lets not do a separate header here. Just move this all into
> hci_nokia.c. There is really zero benefit in the header.

ok.

> > +
> > +#define NOKIA_ID_CSR		0x02
> > +#define NOKIA_ID_BCM2048	0x04
> > +#define NOKIA_ID_TI1271		0x31
> > +
> > +#define FIRMWARE_CSR		"nokia/bc4fw.bin"
> 
> If the CSR ones are not yet supported, then leave them out for
> now. We can add this later.
> 
> > +#define FIRMWARE_BCM2048	"nokia/bcmfw.bin"
> > +#define FIRMWARE_TI1271		"nokia/ti1273.bin"
> > +
> > +#define NOKIA_BCM_BDADDR	0xfc01
> 
> We have btbcm.[ch] for this.

ah this is a leftover. Currently the driver does not set
set_bdaddr() callback, since it differs between ti and bcm backend.
It looks like btbcm_set_bdaddr() can be used for the broadcom based
chips, though.

> > +#define HCI_NOKIA_NEG_PKT	0x06
> > +#define HCI_NOKIA_ALIVE_PKT	0x07
> > +#define HCI_NOKIA_RADIO_PKT	0x08
> > +
> > +#define HCI_NOKIA_NEG_HDR_SIZE		1
> > +#define HCI_NOKIA_MAX_NEG_SIZE		255
> > +#define HCI_NOKIA_ALIVE_HDR_SIZE	1
> > +#define HCI_NOKIA_MAX_ALIVE_SIZE	255
> > +#define HCI_NOKIA_RADIO_HDR_SIZE	2
> > +#define HCI_NOKIA_MAX_RADIO_SIZE	255
> > +
> > +#define NOKIA_PROTO_PKT		0x44
> > +#define NOKIA_PROTO_BYTE	0x4c
> > +
> > +#define NOKIA_NEG_REQ		0x00
> > +#define NOKIA_NEG_ACK		0x20
> > +#define NOKIA_NEG_NAK		0x40
> > +
> > +#define H4_TYPE_SIZE		1
> 
> I am not sure this define adds any overall value to the code.
> 
> > +
> > +#define NOKIA_RECV_ACL \
> > +	H4_RECV_ACL, \
> > +	.wordaligned = true
> > +
> > +#define NOKIA_RECV_SCO \
> > +	H4_RECV_SCO, \
> > +	.wordaligned = true
> > +
> > +#define NOKIA_RECV_EVENT \
> > +	H4_RECV_EVENT, \
> > +	.wordaligned = true
> > +
> > +#define NOKIA_RECV_ALIVE \
> > +	.type = HCI_NOKIA_ALIVE_PKT, \
> > +	.hlen = HCI_NOKIA_ALIVE_HDR_SIZE, \
> > +	.loff = 0, \
> > +	.lsize = 1, \
> > +	.maxlen = HCI_NOKIA_MAX_ALIVE_SIZE, \
> > +	.wordaligned = true
> > +
> > +#define NOKIA_RECV_NEG \
> > +	.type = HCI_NOKIA_NEG_PKT, \
> > +	.hlen = HCI_NOKIA_NEG_HDR_SIZE, \
> > +	.loff = 0, \
> > +	.lsize = 1, \
> > +	.maxlen = HCI_NOKIA_MAX_NEG_SIZE, \
> > +	.wordaligned = true
> > +
> > +#define NOKIA_RECV_RADIO \
> > +	.type = HCI_NOKIA_RADIO_PKT, \
> > +	.hlen = HCI_NOKIA_RADIO_HDR_SIZE, \
> > +	.loff = 1, \
> > +	.lsize = 1, \
> > +	.maxlen = HCI_NOKIA_MAX_RADIO_SIZE, \
> > +	.wordaligned = true
> 
> For this ones I would have use the HCI event ones.
> My original patch had this:
> 
> +#define NOK_RECV_NEG \
> +	.type = NOK_NEG_PKT, \
> +	.hlen = NOK_NEG_HDR_SIZE, \
> +	.loff = 0, \
> +	.lsize = 1, \
> +	.maxlen = HCI_MAX_EVENT_SIZE
> +
> +#define NOK_RECV_ALIVE \
> +	.type = NOK_ALIVE_PKT, \
> +	.hlen = NOK_ALIVE_HDR_SIZE, \
> +	.loff = 0, \
> +	.lsize = 1, \
> +	.maxlen = HCI_MAX_EVENT_SIZE
> +
> +#define NOK_RECV_RADIO \
> +	.type = NOK_RADIO_PKT, \
> +	.hlen = HCI_EVENT_HDR_SIZE, \
> +	.loff = 1, \
> +	.lsize = 1, \
> +	.maxlen = HCI_MAX_EVENT_SIZE
> +
> +static const struct h4_recv_pkt nok_recv_pkts[] = {
> +	{ H4_RECV_ACL,    .recv = hci_recv_frame },
> +	{ H4_RECV_SCO,    .recv = hci_recv_frame },
> +	{ H4_RECV_EVENT,  .recv = hci_recv_frame },
> +	{ NOK_RECV_NEG,   .recv = nok_recv_neg   },
> +	{ NOK_RECV_ALIVE, .recv = nok_recv_alive },
> +	{ NOK_RECV_RADIO, .recv = nok_recv_radio },
> 
> With just these simple defines at the top:
> 
> +#define NOK_NEG_PKT	0x06
> +#define NOK_ALIVE_PKT	0x07
> +#define NOK_RADIO_PKT	0x08
> +
> +#define NOK_NEG_HDR_SIZE	1
> +#define NOK_ALIVE_HDR_SIZE	1
> 
> And I would prefer if we keep it like that.

ok. I used explicit defines, since it looks like
a copy/paste error otherwise.

> > +
> > +struct hci_nokia_neg_hdr {
> > +	__u8	dlen;
> > +} __packed;
> > +
> > +struct hci_nokia_neg_cmd {
> > +	__u8	ack;
> > +	__u16	baud;
> > +	__u16	unused1;
> > +	__u8	proto;
> > +	__u16	sys_clk;
> > +	__u16	unused2;
> > +} __packed;
> > +
> > +static inline struct hci_nokia_neg_hdr *hci_nokia_neg_hdr(const struct sk_buff *skb)
> > +{
> > +	return (struct hci_nokia_neg_hdr *) skb->data;
> > +}
> 
> What good is this inline? A define would be way better, if really
> needed.

I will drop hci_nokia_neg_hdr() and hci_nokia_alive_hdr() inlines

> [...]

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

[Marcel Holtmann]

Hi Sebastian,

>>> +	if (err < 0) {
>>> +		BT_ERR("%s: Failed to load Nokia firmware file (%d)",
>>> +		       hu->hdev->name, err);
>>> +		return err;
>>> +	}
>>> +
>>> +	fw_ptr = fw->data;
>>> +	fw_size = fw->size;
>>> +
>>> +	while (fw_size >= 4) {
>>> +		u16 pkt_size = get_unaligned_le16(fw_ptr);
>>> +		u8 pkt_type = fw_ptr[2];
>>> +		const struct hci_command_hdr *cmd;
>>> +		u16 opcode;
>>> +		struct sk_buff *skb;
>>> +
>>> +		switch (pkt_type) {
>>> +		case HCI_COMMAND_PKT:
>>> +			cmd = (struct hci_command_hdr *)(fw_ptr + 3);
>>> +			opcode = le16_to_cpu(cmd->opcode);
>>> +
>>> +			skb = __hci_cmd_sync(hu->hdev, opcode, cmd->plen,
>>> +					     fw_ptr + 3 + HCI_COMMAND_HDR_SIZE,
>>> +					     HCI_INIT_TIMEOUT);
>>> +			if (IS_ERR(skb)) {
>>> +				err = PTR_ERR(skb);
>>> +				BT_ERR("%s: Firmware command %04x failed (%d)",
>>> +				       hu->hdev->name, opcode, err);
>>> +				goto done;
>>> +			}
>>> +			kfree_skb(skb);
>>> +			break;
>>> +		case HCI_NOKIA_RADIO_PKT:
>> 
>> Are you sure you can ignore the RADIO_PKT commands. They are used
>> to set up the FM radio parts of the chip. They are standard HCI
>> commands (in the case of Broadcom at least). At minimum it should
>> be added a comment here that you are ignoring them on purpose.
> 
> I got the driver working on N950. I think it does not make use of
> the radio packets at all. On N900 they may be needed, though. I do
> not reach far enough in the firmware loading process to know for
> sure.
> 
> If I remember correctly your template driver does bundle it together
> with HCI_COMMAND_PKT, but that does not work, since HCI_NOKIA_RADIO_PKT
> opcode size is u8 instead of u16. I ignored it for now, since I
> could not properly test it.

that sounds heavily like a bug somehow. I remember having decoded the Broadcom firmware and there it really has to go via standard HCI command. And that is the default Broadcom FM radio command.

My assumption was that it was an initial misunderstanding by the driver itself. Can someone send me all the Nokia firmware files and I have a look at them.

> 
>>> +		case HCI_NOKIA_NEG_PKT:
>>> +		case HCI_NOKIA_ALIVE_PKT:
>> 
>> And here I would also a comment on why are we ignore these
>> commands and driving this all by ourselves.
> 
> I think we could use the packets from the firmware instead
> of doing it manually (On N900 they are bit identical to the
> manually generated one - On N950 I have not yet checked), but
> until N900 works having it coded explicitly helps debugging.

We can also always manually encode the firmware to do it correctly. At the end of the day, the firmware should go into linux-firmware tree anyway.

>>> +
>>> +#define NOKIA_ID_CSR		0x02
>>> +#define NOKIA_ID_BCM2048	0x04
>>> +#define NOKIA_ID_TI1271		0x31
>>> +
>>> +#define FIRMWARE_CSR		"nokia/bc4fw.bin"
>> 
>> If the CSR ones are not yet supported, then leave them out for
>> now. We can add this later.
>> 
>>> +#define FIRMWARE_BCM2048	"nokia/bcmfw.bin"
>>> +#define FIRMWARE_TI1271		"nokia/ti1273.bin"
>>> +
>>> +#define NOKIA_BCM_BDADDR	0xfc01
>> 
>> We have btbcm.[ch] for this.
> 
> ah this is a leftover. Currently the driver does not set
> set_bdaddr() callback, since it differs between ti and bcm backend.
> It looks like btbcm_set_bdaddr() can be used for the broadcom based
> chips, though.

Yes. For the Broadcom chip, just select the btbcm_set_bdaddr and set the module dependency correctly. We already do that for hci_bcm.c anyway. For the TI one, extra the opcode for from the original driver and just add a TI function inside the driver. I think adding a btti.c module is overkill at the moment. We can extract that later. Even btusb.c carries some set_bdaddr function in the main driver where splitting them out made no sense at the moment.

> 
>>> +#define HCI_NOKIA_NEG_PKT	0x06
>>> +#define HCI_NOKIA_ALIVE_PKT	0x07
>>> +#define HCI_NOKIA_RADIO_PKT	0x08
>>> +
>>> +#define HCI_NOKIA_NEG_HDR_SIZE		1
>>> +#define HCI_NOKIA_MAX_NEG_SIZE		255
>>> +#define HCI_NOKIA_ALIVE_HDR_SIZE	1
>>> +#define HCI_NOKIA_MAX_ALIVE_SIZE	255
>>> +#define HCI_NOKIA_RADIO_HDR_SIZE	2
>>> +#define HCI_NOKIA_MAX_RADIO_SIZE	255
>>> +
>>> +#define NOKIA_PROTO_PKT		0x44
>>> +#define NOKIA_PROTO_BYTE	0x4c
>>> +
>>> +#define NOKIA_NEG_REQ		0x00
>>> +#define NOKIA_NEG_ACK		0x20
>>> +#define NOKIA_NEG_NAK		0x40
>>> +
>>> +#define H4_TYPE_SIZE		1
>> 
>> I am not sure this define adds any overall value to the code.
>> 
>>> +
>>> +#define NOKIA_RECV_ACL \
>>> +	H4_RECV_ACL, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_SCO \
>>> +	H4_RECV_SCO, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_EVENT \
>>> +	H4_RECV_EVENT, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_ALIVE \
>>> +	.type = HCI_NOKIA_ALIVE_PKT, \
>>> +	.hlen = HCI_NOKIA_ALIVE_HDR_SIZE, \
>>> +	.loff = 0, \
>>> +	.lsize = 1, \
>>> +	.maxlen = HCI_NOKIA_MAX_ALIVE_SIZE, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_NEG \
>>> +	.type = HCI_NOKIA_NEG_PKT, \
>>> +	.hlen = HCI_NOKIA_NEG_HDR_SIZE, \
>>> +	.loff = 0, \
>>> +	.lsize = 1, \
>>> +	.maxlen = HCI_NOKIA_MAX_NEG_SIZE, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_RADIO \
>>> +	.type = HCI_NOKIA_RADIO_PKT, \
>>> +	.hlen = HCI_NOKIA_RADIO_HDR_SIZE, \
>>> +	.loff = 1, \
>>> +	.lsize = 1, \
>>> +	.maxlen = HCI_NOKIA_MAX_RADIO_SIZE, \
>>> +	.wordaligned = true
>> 
>> For this ones I would have use the HCI event ones.
>> My original patch had this:
>> 
>> +#define NOK_RECV_NEG \
>> +	.type = NOK_NEG_PKT, \
>> +	.hlen = NOK_NEG_HDR_SIZE, \
>> +	.loff = 0, \
>> +	.lsize = 1, \
>> +	.maxlen = HCI_MAX_EVENT_SIZE
>> +
>> +#define NOK_RECV_ALIVE \
>> +	.type = NOK_ALIVE_PKT, \
>> +	.hlen = NOK_ALIVE_HDR_SIZE, \
>> +	.loff = 0, \
>> +	.lsize = 1, \
>> +	.maxlen = HCI_MAX_EVENT_SIZE
>> +
>> +#define NOK_RECV_RADIO \
>> +	.type = NOK_RADIO_PKT, \
>> +	.hlen = HCI_EVENT_HDR_SIZE, \
>> +	.loff = 1, \
>> +	.lsize = 1, \
>> +	.maxlen = HCI_MAX_EVENT_SIZE
>> +
>> +static const struct h4_recv_pkt nok_recv_pkts[] = {
>> +	{ H4_RECV_ACL,    .recv = hci_recv_frame },
>> +	{ H4_RECV_SCO,    .recv = hci_recv_frame },
>> +	{ H4_RECV_EVENT,  .recv = hci_recv_frame },
>> +	{ NOK_RECV_NEG,   .recv = nok_recv_neg   },
>> +	{ NOK_RECV_ALIVE, .recv = nok_recv_alive },
>> +	{ NOK_RECV_RADIO, .recv = nok_recv_radio },
>> 
>> With just these simple defines at the top:
>> 
>> +#define NOK_NEG_PKT	0x06
>> +#define NOK_ALIVE_PKT	0x07
>> +#define NOK_RADIO_PKT	0x08
>> +
>> +#define NOK_NEG_HDR_SIZE	1
>> +#define NOK_ALIVE_HDR_SIZE	1
>> 
>> And I would prefer if we keep it like that.
> 
> ok. I used explicit defines, since it looks like
> a copy/paste error otherwise.

With the .align setting you also need to introduce NOK_RECV_ACL etc. with complete definition anyway. Just make sure to use the HCI_* defines where possible.

An alternative is to add the align parameter to h4_recv_buf. Which might be the better idea anyway since I doubt the alignment only applies to a single packet type. It should apply to all of them since otherwise it makes no sense.

Regards

Marcel

Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

[Marcel Holtmann]

Hi Sebastian,

>>> +	if (err < 0) {
>>> +		BT_ERR("%s: Failed to load Nokia firmware file (%d)",
>>> +		       hu->hdev->name, err);
>>> +		return err;
>>> +	}
>>> +
>>> +	fw_ptr = fw->data;
>>> +	fw_size = fw->size;
>>> +
>>> +	while (fw_size >= 4) {
>>> +		u16 pkt_size = get_unaligned_le16(fw_ptr);
>>> +		u8 pkt_type = fw_ptr[2];
>>> +		const struct hci_command_hdr *cmd;
>>> +		u16 opcode;
>>> +		struct sk_buff *skb;
>>> +
>>> +		switch (pkt_type) {
>>> +		case HCI_COMMAND_PKT:
>>> +			cmd = (struct hci_command_hdr *)(fw_ptr + 3);
>>> +			opcode = le16_to_cpu(cmd->opcode);
>>> +
>>> +			skb = __hci_cmd_sync(hu->hdev, opcode, cmd->plen,
>>> +					     fw_ptr + 3 + HCI_COMMAND_HDR_SIZE,
>>> +					     HCI_INIT_TIMEOUT);
>>> +			if (IS_ERR(skb)) {
>>> +				err = PTR_ERR(skb);
>>> +				BT_ERR("%s: Firmware command %04x failed (%d)",
>>> +				       hu->hdev->name, opcode, err);
>>> +				goto done;
>>> +			}
>>> +			kfree_skb(skb);
>>> +			break;
>>> +		case HCI_NOKIA_RADIO_PKT:
>> 
>> Are you sure you can ignore the RADIO_PKT commands. They are used
>> to set up the FM radio parts of the chip. They are standard HCI
>> commands (in the case of Broadcom at least). At minimum it should
>> be added a comment here that you are ignoring them on purpose.
> 
> I got the driver working on N950. I think it does not make use of
> the radio packets at all. On N900 they may be needed, though. I do
> not reach far enough in the firmware loading process to know for
> sure.
> 
> If I remember correctly your template driver does bundle it together
> with HCI_COMMAND_PKT, but that does not work, since HCI_NOKIA_RADIO_PKT
> opcode size is u8 instead of u16. I ignored it for now, since I
> could not properly test it.

that sounds heavily like a bug somehow. I remember having decoded the Broadcom firmware and there it really has to go via standard HCI command. And that is the default Broadcom FM radio command.

My assumption was that it was an initial misunderstanding by the driver itself. Can someone send me all the Nokia firmware files and I have a look at them.

> 
>>> +		case HCI_NOKIA_NEG_PKT:
>>> +		case HCI_NOKIA_ALIVE_PKT:
>> 
>> And here I would also a comment on why are we ignore these
>> commands and driving this all by ourselves.
> 
> I think we could use the packets from the firmware instead
> of doing it manually (On N900 they are bit identical to the
> manually generated one - On N950 I have not yet checked), but
> until N900 works having it coded explicitly helps debugging.

We can also always manually encode the firmware to do it correctly. At the end of the day, the firmware should go into linux-firmware tree anyway.

>>> +
>>> +#define NOKIA_ID_CSR		0x02
>>> +#define NOKIA_ID_BCM2048	0x04
>>> +#define NOKIA_ID_TI1271		0x31
>>> +
>>> +#define FIRMWARE_CSR		"nokia/bc4fw.bin"
>> 
>> If the CSR ones are not yet supported, then leave them out for
>> now. We can add this later.
>> 
>>> +#define FIRMWARE_BCM2048	"nokia/bcmfw.bin"
>>> +#define FIRMWARE_TI1271		"nokia/ti1273.bin"
>>> +
>>> +#define NOKIA_BCM_BDADDR	0xfc01
>> 
>> We have btbcm.[ch] for this.
> 
> ah this is a leftover. Currently the driver does not set
> set_bdaddr() callback, since it differs between ti and bcm backend.
> It looks like btbcm_set_bdaddr() can be used for the broadcom based
> chips, though.

Yes. For the Broadcom chip, just select the btbcm_set_bdaddr and set the module dependency correctly. We already do that for hci_bcm.c anyway. For the TI one, extra the opcode for from the original driver and just add a TI function inside the driver. I think adding a btti.c module is overkill at the moment. We can extract that later. Even btusb.c carries some set_bdaddr function in the main driver where splitting them out made no sense at the moment.

> 
>>> +#define HCI_NOKIA_NEG_PKT	0x06
>>> +#define HCI_NOKIA_ALIVE_PKT	0x07
>>> +#define HCI_NOKIA_RADIO_PKT	0x08
>>> +
>>> +#define HCI_NOKIA_NEG_HDR_SIZE		1
>>> +#define HCI_NOKIA_MAX_NEG_SIZE		255
>>> +#define HCI_NOKIA_ALIVE_HDR_SIZE	1
>>> +#define HCI_NOKIA_MAX_ALIVE_SIZE	255
>>> +#define HCI_NOKIA_RADIO_HDR_SIZE	2
>>> +#define HCI_NOKIA_MAX_RADIO_SIZE	255
>>> +
>>> +#define NOKIA_PROTO_PKT		0x44
>>> +#define NOKIA_PROTO_BYTE	0x4c
>>> +
>>> +#define NOKIA_NEG_REQ		0x00
>>> +#define NOKIA_NEG_ACK		0x20
>>> +#define NOKIA_NEG_NAK		0x40
>>> +
>>> +#define H4_TYPE_SIZE		1
>> 
>> I am not sure this define adds any overall value to the code.
>> 
>>> +
>>> +#define NOKIA_RECV_ACL \
>>> +	H4_RECV_ACL, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_SCO \
>>> +	H4_RECV_SCO, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_EVENT \
>>> +	H4_RECV_EVENT, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_ALIVE \
>>> +	.type = HCI_NOKIA_ALIVE_PKT, \
>>> +	.hlen = HCI_NOKIA_ALIVE_HDR_SIZE, \
>>> +	.loff = 0, \
>>> +	.lsize = 1, \
>>> +	.maxlen = HCI_NOKIA_MAX_ALIVE_SIZE, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_NEG \
>>> +	.type = HCI_NOKIA_NEG_PKT, \
>>> +	.hlen = HCI_NOKIA_NEG_HDR_SIZE, \
>>> +	.loff = 0, \
>>> +	.lsize = 1, \
>>> +	.maxlen = HCI_NOKIA_MAX_NEG_SIZE, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_RADIO \
>>> +	.type = HCI_NOKIA_RADIO_PKT, \
>>> +	.hlen = HCI_NOKIA_RADIO_HDR_SIZE, \
>>> +	.loff = 1, \
>>> +	.lsize = 1, \
>>> +	.maxlen = HCI_NOKIA_MAX_RADIO_SIZE, \
>>> +	.wordaligned = true
>> 
>> For this ones I would have use the HCI event ones.
>> My original patch had this:
>> 
>> +#define NOK_RECV_NEG \
>> +	.type = NOK_NEG_PKT, \
>> +	.hlen = NOK_NEG_HDR_SIZE, \
>> +	.loff = 0, \
>> +	.lsize = 1, \
>> +	.maxlen = HCI_MAX_EVENT_SIZE
>> +
>> +#define NOK_RECV_ALIVE \
>> +	.type = NOK_ALIVE_PKT, \
>> +	.hlen = NOK_ALIVE_HDR_SIZE, \
>> +	.loff = 0, \
>> +	.lsize = 1, \
>> +	.maxlen = HCI_MAX_EVENT_SIZE
>> +
>> +#define NOK_RECV_RADIO \
>> +	.type = NOK_RADIO_PKT, \
>> +	.hlen = HCI_EVENT_HDR_SIZE, \
>> +	.loff = 1, \
>> +	.lsize = 1, \
>> +	.maxlen = HCI_MAX_EVENT_SIZE
>> +
>> +static const struct h4_recv_pkt nok_recv_pkts[] = {
>> +	{ H4_RECV_ACL,    .recv = hci_recv_frame },
>> +	{ H4_RECV_SCO,    .recv = hci_recv_frame },
>> +	{ H4_RECV_EVENT,  .recv = hci_recv_frame },
>> +	{ NOK_RECV_NEG,   .recv = nok_recv_neg   },
>> +	{ NOK_RECV_ALIVE, .recv = nok_recv_alive },
>> +	{ NOK_RECV_RADIO, .recv = nok_recv_radio },
>> 
>> With just these simple defines at the top:
>> 
>> +#define NOK_NEG_PKT	0x06
>> +#define NOK_ALIVE_PKT	0x07
>> +#define NOK_RADIO_PKT	0x08
>> +
>> +#define NOK_NEG_HDR_SIZE	1
>> +#define NOK_ALIVE_HDR_SIZE	1
>> 
>> And I would prefer if we keep it like that.
> 
> ok. I used explicit defines, since it looks like
> a copy/paste error otherwise.

With the .align setting you also need to introduce NOK_RECV_ACL etc. with complete definition anyway. Just make sure to use the HCI_* defines where possible.

An alternative is to add the align parameter to h4_recv_buf. Which might be the better idea anyway since I doubt the alignment only applies to a single packet type. It should apply to all of them since otherwise it makes no sense.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

[Pavel Machek]

Hi!

> > I think we could use the packets from the firmware instead
> > of doing it manually (On N900 they are bit identical to the
> > manually generated one - On N950 I have not yet checked), but
> > until N900 works having it coded explicitly helps debugging.
> 
> We can also always manually encode the firmware to do it correctly. At the end of the day, the firmware should go into linux-firmware tree anyway.
> 

Editing firmware may raise some "interesting" legal questions... and
its redistribution might be tricky, too. Microsoft is not likely to be
too helpful :-(.

Best,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

[Marcel Holtmann]

Hi Sebastian,

>>> +	if (err < 0) {
>>> +		BT_ERR("%s: Failed to load Nokia firmware file (%d)",
>>> +		       hu->hdev->name, err);
>>> +		return err;
>>> +	}
>>> +
>>> +	fw_ptr = fw->data;
>>> +	fw_size = fw->size;
>>> +
>>> +	while (fw_size >= 4) {
>>> +		u16 pkt_size = get_unaligned_le16(fw_ptr);
>>> +		u8 pkt_type = fw_ptr[2];
>>> +		const struct hci_command_hdr *cmd;
>>> +		u16 opcode;
>>> +		struct sk_buff *skb;
>>> +
>>> +		switch (pkt_type) {
>>> +		case HCI_COMMAND_PKT:
>>> +			cmd = (struct hci_command_hdr *)(fw_ptr + 3);
>>> +			opcode = le16_to_cpu(cmd->opcode);
>>> +
>>> +			skb = __hci_cmd_sync(hu->hdev, opcode, cmd->plen,
>>> +					     fw_ptr + 3 + HCI_COMMAND_HDR_SIZE,
>>> +					     HCI_INIT_TIMEOUT);
>>> +			if (IS_ERR(skb)) {
>>> +				err = PTR_ERR(skb);
>>> +				BT_ERR("%s: Firmware command %04x failed (%d)",
>>> +				       hu->hdev->name, opcode, err);
>>> +				goto done;
>>> +			}
>>> +			kfree_skb(skb);
>>> +			break;
>>> +		case HCI_NOKIA_RADIO_PKT:
>> 
>> Are you sure you can ignore the RADIO_PKT commands. They are used
>> to set up the FM radio parts of the chip. They are standard HCI
>> commands (in the case of Broadcom at least). At minimum it should
>> be added a comment here that you are ignoring them on purpose.
> 
> I got the driver working on N950. I think it does not make use of
> the radio packets at all. On N900 they may be needed, though. I do
> not reach far enough in the firmware loading process to know for
> sure.
> 
> If I remember correctly your template driver does bundle it together
> with HCI_COMMAND_PKT, but that does not work, since HCI_NOKIA_RADIO_PKT
> opcode size is u8 instead of u16. I ignored it for now, since I
> could not properly test it.

that sounds heavily like a bug somehow. I remember having decoded the Broadcom firmware and there it really has to go via standard HCI command. And that is the default Broadcom FM radio command.

My assumption was that it was an initial misunderstanding by the driver itself. Can someone send me all the Nokia firmware files and I have a look at them.

> 
>>> +		case HCI_NOKIA_NEG_PKT:
>>> +		case HCI_NOKIA_ALIVE_PKT:
>> 
>> And here I would also a comment on why are we ignore these
>> commands and driving this all by ourselves.
> 
> I think we could use the packets from the firmware instead
> of doing it manually (On N900 they are bit identical to the
> manually generated one - On N950 I have not yet checked), but
> until N900 works having it coded explicitly helps debugging.

We can also always manually encode the firmware to do it correctly. At the end of the day, the firmware should go into linux-firmware tree anyway.

>>> +
>>> +#define NOKIA_ID_CSR		0x02
>>> +#define NOKIA_ID_BCM2048	0x04
>>> +#define NOKIA_ID_TI1271		0x31
>>> +
>>> +#define FIRMWARE_CSR		"nokia/bc4fw.bin"
>> 
>> If the CSR ones are not yet supported, then leave them out for
>> now. We can add this later.
>> 
>>> +#define FIRMWARE_BCM2048	"nokia/bcmfw.bin"
>>> +#define FIRMWARE_TI1271		"nokia/ti1273.bin"
>>> +
>>> +#define NOKIA_BCM_BDADDR	0xfc01
>> 
>> We have btbcm.[ch] for this.
> 
> ah this is a leftover. Currently the driver does not set
> set_bdaddr() callback, since it differs between ti and bcm backend.
> It looks like btbcm_set_bdaddr() can be used for the broadcom based
> chips, though.

Yes. For the Broadcom chip, just select the btbcm_set_bdaddr and set the module dependency correctly. We already do that for hci_bcm.c anyway. For the TI one, extra the opcode for from the original driver and just add a TI function inside the driver. I think adding a btti.c module is overkill at the moment. We can extract that later. Even btusb.c carries some set_bdaddr function in the main driver where splitting them out made no sense at the moment.

> 
>>> +#define HCI_NOKIA_NEG_PKT	0x06
>>> +#define HCI_NOKIA_ALIVE_PKT	0x07
>>> +#define HCI_NOKIA_RADIO_PKT	0x08
>>> +
>>> +#define HCI_NOKIA_NEG_HDR_SIZE		1
>>> +#define HCI_NOKIA_MAX_NEG_SIZE		255
>>> +#define HCI_NOKIA_ALIVE_HDR_SIZE	1
>>> +#define HCI_NOKIA_MAX_ALIVE_SIZE	255
>>> +#define HCI_NOKIA_RADIO_HDR_SIZE	2
>>> +#define HCI_NOKIA_MAX_RADIO_SIZE	255
>>> +
>>> +#define NOKIA_PROTO_PKT		0x44
>>> +#define NOKIA_PROTO_BYTE	0x4c
>>> +
>>> +#define NOKIA_NEG_REQ		0x00
>>> +#define NOKIA_NEG_ACK		0x20
>>> +#define NOKIA_NEG_NAK		0x40
>>> +
>>> +#define H4_TYPE_SIZE		1
>> 
>> I am not sure this define adds any overall value to the code.
>> 
>>> +
>>> +#define NOKIA_RECV_ACL \
>>> +	H4_RECV_ACL, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_SCO \
>>> +	H4_RECV_SCO, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_EVENT \
>>> +	H4_RECV_EVENT, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_ALIVE \
>>> +	.type = HCI_NOKIA_ALIVE_PKT, \
>>> +	.hlen = HCI_NOKIA_ALIVE_HDR_SIZE, \
>>> +	.loff = 0, \
>>> +	.lsize = 1, \
>>> +	.maxlen = HCI_NOKIA_MAX_ALIVE_SIZE, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_NEG \
>>> +	.type = HCI_NOKIA_NEG_PKT, \
>>> +	.hlen = HCI_NOKIA_NEG_HDR_SIZE, \
>>> +	.loff = 0, \
>>> +	.lsize = 1, \
>>> +	.maxlen = HCI_NOKIA_MAX_NEG_SIZE, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_RADIO \
>>> +	.type = HCI_NOKIA_RADIO_PKT, \
>>> +	.hlen = HCI_NOKIA_RADIO_HDR_SIZE, \
>>> +	.loff = 1, \
>>> +	.lsize = 1, \
>>> +	.maxlen = HCI_NOKIA_MAX_RADIO_SIZE, \
>>> +	.wordaligned = true
>> 
>> For this ones I would have use the HCI event ones.
>> My original patch had this:
>> 
>> +#define NOK_RECV_NEG \
>> +	.type = NOK_NEG_PKT, \
>> +	.hlen = NOK_NEG_HDR_SIZE, \
>> +	.loff = 0, \
>> +	.lsize = 1, \
>> +	.maxlen = HCI_MAX_EVENT_SIZE
>> +
>> +#define NOK_RECV_ALIVE \
>> +	.type = NOK_ALIVE_PKT, \
>> +	.hlen = NOK_ALIVE_HDR_SIZE, \
>> +	.loff = 0, \
>> +	.lsize = 1, \
>> +	.maxlen = HCI_MAX_EVENT_SIZE
>> +
>> +#define NOK_RECV_RADIO \
>> +	.type = NOK_RADIO_PKT, \
>> +	.hlen = HCI_EVENT_HDR_SIZE, \
>> +	.loff = 1, \
>> +	.lsize = 1, \
>> +	.maxlen = HCI_MAX_EVENT_SIZE
>> +
>> +static const struct h4_recv_pkt nok_recv_pkts[] = {
>> +	{ H4_RECV_ACL,    .recv = hci_recv_frame },
>> +	{ H4_RECV_SCO,    .recv = hci_recv_frame },
>> +	{ H4_RECV_EVENT,  .recv = hci_recv_frame },
>> +	{ NOK_RECV_NEG,   .recv = nok_recv_neg   },
>> +	{ NOK_RECV_ALIVE, .recv = nok_recv_alive },
>> +	{ NOK_RECV_RADIO, .recv = nok_recv_radio },
>> 
>> With just these simple defines at the top:
>> 
>> +#define NOK_NEG_PKT	0x06
>> +#define NOK_ALIVE_PKT	0x07
>> +#define NOK_RADIO_PKT	0x08
>> +
>> +#define NOK_NEG_HDR_SIZE	1
>> +#define NOK_ALIVE_HDR_SIZE	1
>> 
>> And I would prefer if we keep it like that.
> 
> ok. I used explicit defines, since it looks like
> a copy/paste error otherwise.

With the .align setting you also need to introduce NOK_RECV_ACL etc. with complete definition anyway. Just make sure to use the HCI_* defines where possible.

An alternative is to add the align parameter to h4_recv_buf. Which might be the better idea anyway since I doubt the alignment only applies to a single packet type. It should apply to all of them since otherwise it makes no sense.

Regards

Marcel

Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

[Marcel Holtmann]

Hi Sebastian,

>>> +	if (err < 0) {
>>> +		BT_ERR("%s: Failed to load Nokia firmware file (%d)",
>>> +		       hu->hdev->name, err);
>>> +		return err;
>>> +	}
>>> +
>>> +	fw_ptr = fw->data;
>>> +	fw_size = fw->size;
>>> +
>>> +	while (fw_size >= 4) {
>>> +		u16 pkt_size = get_unaligned_le16(fw_ptr);
>>> +		u8 pkt_type = fw_ptr[2];
>>> +		const struct hci_command_hdr *cmd;
>>> +		u16 opcode;
>>> +		struct sk_buff *skb;
>>> +
>>> +		switch (pkt_type) {
>>> +		case HCI_COMMAND_PKT:
>>> +			cmd = (struct hci_command_hdr *)(fw_ptr + 3);
>>> +			opcode = le16_to_cpu(cmd->opcode);
>>> +
>>> +			skb = __hci_cmd_sync(hu->hdev, opcode, cmd->plen,
>>> +					     fw_ptr + 3 + HCI_COMMAND_HDR_SIZE,
>>> +					     HCI_INIT_TIMEOUT);
>>> +			if (IS_ERR(skb)) {
>>> +				err = PTR_ERR(skb);
>>> +				BT_ERR("%s: Firmware command %04x failed (%d)",
>>> +				       hu->hdev->name, opcode, err);
>>> +				goto done;
>>> +			}
>>> +			kfree_skb(skb);
>>> +			break;
>>> +		case HCI_NOKIA_RADIO_PKT:
>> 
>> Are you sure you can ignore the RADIO_PKT commands. They are used
>> to set up the FM radio parts of the chip. They are standard HCI
>> commands (in the case of Broadcom at least). At minimum it should
>> be added a comment here that you are ignoring them on purpose.
> 
> I got the driver working on N950. I think it does not make use of
> the radio packets at all. On N900 they may be needed, though. I do
> not reach far enough in the firmware loading process to know for
> sure.
> 
> If I remember correctly your template driver does bundle it together
> with HCI_COMMAND_PKT, but that does not work, since HCI_NOKIA_RADIO_PKT
> opcode size is u8 instead of u16. I ignored it for now, since I
> could not properly test it.

that sounds heavily like a bug somehow. I remember having decoded the Broadcom firmware and there it really has to go via standard HCI command. And that is the default Broadcom FM radio command.

My assumption was that it was an initial misunderstanding by the driver itself. Can someone send me all the Nokia firmware files and I have a look at them.

> 
>>> +		case HCI_NOKIA_NEG_PKT:
>>> +		case HCI_NOKIA_ALIVE_PKT:
>> 
>> And here I would also a comment on why are we ignore these
>> commands and driving this all by ourselves.
> 
> I think we could use the packets from the firmware instead
> of doing it manually (On N900 they are bit identical to the
> manually generated one - On N950 I have not yet checked), but
> until N900 works having it coded explicitly helps debugging.

We can also always manually encode the firmware to do it correctly. At the end of the day, the firmware should go into linux-firmware tree anyway.

>>> +
>>> +#define NOKIA_ID_CSR		0x02
>>> +#define NOKIA_ID_BCM2048	0x04
>>> +#define NOKIA_ID_TI1271		0x31
>>> +
>>> +#define FIRMWARE_CSR		"nokia/bc4fw.bin"
>> 
>> If the CSR ones are not yet supported, then leave them out for
>> now. We can add this later.
>> 
>>> +#define FIRMWARE_BCM2048	"nokia/bcmfw.bin"
>>> +#define FIRMWARE_TI1271		"nokia/ti1273.bin"
>>> +
>>> +#define NOKIA_BCM_BDADDR	0xfc01
>> 
>> We have btbcm.[ch] for this.
> 
> ah this is a leftover. Currently the driver does not set
> set_bdaddr() callback, since it differs between ti and bcm backend.
> It looks like btbcm_set_bdaddr() can be used for the broadcom based
> chips, though.

Yes. For the Broadcom chip, just select the btbcm_set_bdaddr and set the module dependency correctly. We already do that for hci_bcm.c anyway. For the TI one, extra the opcode for from the original driver and just add a TI function inside the driver. I think adding a btti.c module is overkill at the moment. We can extract that later. Even btusb.c carries some set_bdaddr function in the main driver where splitting them out made no sense at the moment.

> 
>>> +#define HCI_NOKIA_NEG_PKT	0x06
>>> +#define HCI_NOKIA_ALIVE_PKT	0x07
>>> +#define HCI_NOKIA_RADIO_PKT	0x08
>>> +
>>> +#define HCI_NOKIA_NEG_HDR_SIZE		1
>>> +#define HCI_NOKIA_MAX_NEG_SIZE		255
>>> +#define HCI_NOKIA_ALIVE_HDR_SIZE	1
>>> +#define HCI_NOKIA_MAX_ALIVE_SIZE	255
>>> +#define HCI_NOKIA_RADIO_HDR_SIZE	2
>>> +#define HCI_NOKIA_MAX_RADIO_SIZE	255
>>> +
>>> +#define NOKIA_PROTO_PKT		0x44
>>> +#define NOKIA_PROTO_BYTE	0x4c
>>> +
>>> +#define NOKIA_NEG_REQ		0x00
>>> +#define NOKIA_NEG_ACK		0x20
>>> +#define NOKIA_NEG_NAK		0x40
>>> +
>>> +#define H4_TYPE_SIZE		1
>> 
>> I am not sure this define adds any overall value to the code.
>> 
>>> +
>>> +#define NOKIA_RECV_ACL \
>>> +	H4_RECV_ACL, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_SCO \
>>> +	H4_RECV_SCO, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_EVENT \
>>> +	H4_RECV_EVENT, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_ALIVE \
>>> +	.type = HCI_NOKIA_ALIVE_PKT, \
>>> +	.hlen = HCI_NOKIA_ALIVE_HDR_SIZE, \
>>> +	.loff = 0, \
>>> +	.lsize = 1, \
>>> +	.maxlen = HCI_NOKIA_MAX_ALIVE_SIZE, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_NEG \
>>> +	.type = HCI_NOKIA_NEG_PKT, \
>>> +	.hlen = HCI_NOKIA_NEG_HDR_SIZE, \
>>> +	.loff = 0, \
>>> +	.lsize = 1, \
>>> +	.maxlen = HCI_NOKIA_MAX_NEG_SIZE, \
>>> +	.wordaligned = true
>>> +
>>> +#define NOKIA_RECV_RADIO \
>>> +	.type = HCI_NOKIA_RADIO_PKT, \
>>> +	.hlen = HCI_NOKIA_RADIO_HDR_SIZE, \
>>> +	.loff = 1, \
>>> +	.lsize = 1, \
>>> +	.maxlen = HCI_NOKIA_MAX_RADIO_SIZE, \
>>> +	.wordaligned = true
>> 
>> For this ones I would have use the HCI event ones.
>> My original patch had this:
>> 
>> +#define NOK_RECV_NEG \
>> +	.type = NOK_NEG_PKT, \
>> +	.hlen = NOK_NEG_HDR_SIZE, \
>> +	.loff = 0, \
>> +	.lsize = 1, \
>> +	.maxlen = HCI_MAX_EVENT_SIZE
>> +
>> +#define NOK_RECV_ALIVE \
>> +	.type = NOK_ALIVE_PKT, \
>> +	.hlen = NOK_ALIVE_HDR_SIZE, \
>> +	.loff = 0, \
>> +	.lsize = 1, \
>> +	.maxlen = HCI_MAX_EVENT_SIZE
>> +
>> +#define NOK_RECV_RADIO \
>> +	.type = NOK_RADIO_PKT, \
>> +	.hlen = HCI_EVENT_HDR_SIZE, \
>> +	.loff = 1, \
>> +	.lsize = 1, \
>> +	.maxlen = HCI_MAX_EVENT_SIZE
>> +
>> +static const struct h4_recv_pkt nok_recv_pkts[] = {
>> +	{ H4_RECV_ACL,    .recv = hci_recv_frame },
>> +	{ H4_RECV_SCO,    .recv = hci_recv_frame },
>> +	{ H4_RECV_EVENT,  .recv = hci_recv_frame },
>> +	{ NOK_RECV_NEG,   .recv = nok_recv_neg   },
>> +	{ NOK_RECV_ALIVE, .recv = nok_recv_alive },
>> +	{ NOK_RECV_RADIO, .recv = nok_recv_radio },
>> 
>> With just these simple defines at the top:
>> 
>> +#define NOK_NEG_PKT	0x06
>> +#define NOK_ALIVE_PKT	0x07
>> +#define NOK_RADIO_PKT	0x08
>> +
>> +#define NOK_NEG_HDR_SIZE	1
>> +#define NOK_ALIVE_HDR_SIZE	1
>> 
>> And I would prefer if we keep it like that.
> 
> ok. I used explicit defines, since it looks like
> a copy/paste error otherwise.

With the .align setting you also need to introduce NOK_RECV_ACL etc. with complete definition anyway. Just make sure to use the HCI_* defines where possible.

An alternative is to add the align parameter to h4_recv_buf. Which might be the better idea anyway since I doubt the alignment only applies to a single packet type. It should apply to all of them since otherwise it makes no sense.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

[Marcel Holtmann]

Hi Sebastien,

> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> index a7d67aec3632..314b243df996 100644
> --- a/drivers/bluetooth/hci_uart.h
> +++ b/drivers/bluetooth/hci_uart.h
> @@ -35,7 +35,7 @@
> #define HCIUARTGETFLAGS		_IOR('U', 204, int)
> 
> /* UART protocols */
> -#define HCI_UART_MAX_PROTO	10
> +#define HCI_UART_MAX_PROTO	11
> 
> #define HCI_UART_H4	0
> #define HCI_UART_BCSP	1
> @@ -47,6 +47,7 @@
> #define HCI_UART_BCM	7
> #define HCI_UART_QCA	8
> #define HCI_UART_AG6XX	9
> +#define HCI_UART_NOKIA	10

since it seems the driver is getting closer to be ready, lets merge this extra protocol identifier as a separate patch. Then we can adapt btattach as well for it.

Regards

Marcel

Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

[Marcel Holtmann]

Hi Sebastien,

> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> index a7d67aec3632..314b243df996 100644
> --- a/drivers/bluetooth/hci_uart.h
> +++ b/drivers/bluetooth/hci_uart.h
> @@ -35,7 +35,7 @@
> #define HCIUARTGETFLAGS		_IOR('U', 204, int)
> 
> /* UART protocols */
> -#define HCI_UART_MAX_PROTO	10
> +#define HCI_UART_MAX_PROTO	11
> 
> #define HCI_UART_H4	0
> #define HCI_UART_BCSP	1
> @@ -47,6 +47,7 @@
> #define HCI_UART_BCM	7
> #define HCI_UART_QCA	8
> #define HCI_UART_AG6XX	9
> +#define HCI_UART_NOKIA	10

since it seems the driver is getting closer to be ready, lets merge this extra protocol identifier as a separate patch. Then we can adapt btattach as well for it.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 5/7] Bluetooth: hci_nokia: Introduce new driver

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1352 bytes --]

Hi Marcel,

On Tue, Aug 16, 2016 at 10:10:07AM +0200, Marcel Holtmann wrote:
> > diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> > index a7d67aec3632..314b243df996 100644
> > --- a/drivers/bluetooth/hci_uart.h
> > +++ b/drivers/bluetooth/hci_uart.h
> > @@ -35,7 +35,7 @@
> > #define HCIUARTGETFLAGS		_IOR('U', 204, int)
> > 
> > /* UART protocols */
> > -#define HCI_UART_MAX_PROTO	10
> > +#define HCI_UART_MAX_PROTO	11
> > 
> > #define HCI_UART_H4	0
> > #define HCI_UART_BCSP	1
> > @@ -47,6 +47,7 @@
> > #define HCI_UART_BCM	7
> > #define HCI_UART_QCA	8
> > #define HCI_UART_AG6XX	9
> > +#define HCI_UART_NOKIA	10
> 
> since it seems the driver is getting closer to be ready,
> lets merge this extra protocol identifier as a separate patch.

depends on the definition of "the driver". It requires
the gpio information from DT, so it depends on the new
serial bus. I still have to implement that and it has
the potential to trigger a bit of discussion :)

Anyways I will split this out into its own patch later. 

> Then we can adapt btattach as well for it.

It's enough to set ldisc=N_HCI and proto=HCI_UART_NOKIA.
Everything else is done by the protocol driver. I still
think it should be done from the kernel and will try to
come up with something in the serial bus.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[RFC 6/7] ARM: dts: OMAP3-N900: Add bluetooth

[Sebastian Reichel]

Add bcm2048 node and its system clock to the N900 device tree file.
Apart from that a reference to the new clock has been added to
wl1251 (which uses it, too).

Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 arch/arm/boot/dts/omap3-n900.dts | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/omap3-n900.dts b/arch/arm/boot/dts/omap3-n900.dts
index 1d7b1d0d471a..1dba4a64d8fd 100644
--- a/arch/arm/boot/dts/omap3-n900.dts
+++ b/arch/arm/boot/dts/omap3-n900.dts
@@ -155,6 +155,13 @@
 		compatible = "nokia,n900-ir";
 		pwms = <&pwm9 0 26316 0>; /* 38000 Hz */
 	};
+
+	/* controlled (enabled/disabled) directly by bcm2048 and wl1251 */
+	vctcxo: vctcxo {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <38400000>;
+	};
 };
 
 &omap3_pmx_core {
@@ -162,8 +169,10 @@
 
 	uart2_pins: pinmux_uart2_pins {
 		pinctrl-single,pins = <
-			OMAP3_CORE1_IOPAD(0x217a, PIN_INPUT | MUX_MODE0)		/* uart2_rx */
+			OMAP3_CORE1_IOPAD(0x2174, PIN_INPUT | MUX_MODE0)		/* uart2_cts */
+			OMAP3_CORE1_IOPAD(0x2176, PIN_OUTPUT | MUX_MODE0)		/* uart2_rts */
 			OMAP3_CORE1_IOPAD(0x2178, PIN_OUTPUT | MUX_MODE0)		/* uart2_tx */
+			OMAP3_CORE1_IOPAD(0x217a, PIN_INPUT | MUX_MODE0)		/* uart2_rx */
 		>;
 	};
 
@@ -917,6 +926,8 @@
 
 		interrupt-parent = <&gpio2>;
 		interrupts = <10 IRQ_TYPE_NONE>; /* gpio line 42 */
+
+		clocks = <&vctcxo>;
 	};
 };
 
@@ -937,6 +948,15 @@
 	interrupts-extended = <&intc 73 &omap3_pmx_core OMAP3_UART2_RX>;
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart2_pins>;
+
+	bcm2048: bluetooth {
+		compatible = "nokia,brcm,bcm2048";
+		reset-gpios = <&gpio3 27 GPIO_ACTIVE_HIGH>; /* 91 */
+		host-wakeup-gpios = <&gpio4 5 GPIO_ACTIVE_HIGH>; /* 101 */
+		bluetooth-wakeup-gpios = <&gpio2 5 GPIO_ACTIVE_HIGH>; /* 37 */
+		clocks = <&vctcxo>;
+		clock-names = "sysclk";
+	};
 };
 
 &uart3 {
-- 
2.8.1

Re: [RFC 6/7] ARM: dts: OMAP3-N900: Add bluetooth

[Pavel Machek]

On Sat 2016-08-13 05:14:37, Sebastian Reichel wrote:
> Add bcm2048 node and its system clock to the N900 device tree file.
> Apart from that a reference to the new clock has been added to
> wl1251 (which uses it, too).
> 
> Signed-off-by: Sebastian Reichel <sre@kernel.org>

As the driver is not currently working at N900, I agree that this
patch is useful, but it should probably not go upstream.

Thanks,
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[RFC 7/7] ARM: dts: OMAP3-N950: Add bluetooth

[Sebastian Reichel]

---
 arch/arm/boot/dts/omap3-n950-n9.dtsi | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/arch/arm/boot/dts/omap3-n950-n9.dtsi b/arch/arm/boot/dts/omap3-n950-n9.dtsi
index 927b17fc4ed8..7c0c53e2f679 100644
--- a/arch/arm/boot/dts/omap3-n950-n9.dtsi
+++ b/arch/arm/boot/dts/omap3-n950-n9.dtsi
@@ -58,6 +58,13 @@
 			pinctrl-0 = <&debug_leds>;
 		};
 	};
+
+	/* controlled (enabled/disabled) directly by wl1271 */
+	vctcxo: vctcxo {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <38400000>;
+	};
 };
 
 &omap3_pmx_core {
@@ -125,6 +132,16 @@
 			OMAP3_CORE1_IOPAD(0x210a, PIN_OUTPUT | MUX_MODE4)            /* gpio_93 (cmt_apeslpx) */
 		>;
 	};
+
+	uart2_pins: pinmux_uart2_pins {
+		pinctrl-single,pins = <
+			OMAP3_CORE1_IOPAD(0x2174, PIN_INPUT | MUX_MODE0)		/* uart2_cts */
+			OMAP3_CORE1_IOPAD(0x2176, PIN_OUTPUT | MUX_MODE0)		/* uart2_rts */
+			OMAP3_CORE1_IOPAD(0x2178, PIN_OUTPUT | MUX_MODE0)		/* uart2_tx */
+			OMAP3_CORE1_IOPAD(0x217a, PIN_INPUT | MUX_MODE0)		/* uart2_rx */
+		>;
+	};
+
 };
 
 &omap3_pmx_core2 {
@@ -435,3 +452,20 @@
 &ssi_port2 {
 	status = "disabled";
 };
+
+&uart2 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart2_pins>;
+
+	bluetooth {
+		compatible = "nokia,ti,wl1271-bluetooth";
+
+		reset-gpios = <&gpio1 26 GPIO_ACTIVE_HIGH>; /* 26 */
+		host-wakeup-gpios = <&gpio4 5 GPIO_ACTIVE_HIGH>; /* 101 */
+		bluetooth-wakeup-gpios = <&gpio2 5 GPIO_ACTIVE_HIGH>; /* 37 */
+
+		clocks = <&vctcxo>;
+		clock-names = "sysclk";
+	};
+
+};
-- 
2.8.1

[RFC 7/7] ARM: dts: OMAP3-N950: Add bluetooth

[Sebastian Reichel]

---
 arch/arm/boot/dts/omap3-n950-n9.dtsi | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/arch/arm/boot/dts/omap3-n950-n9.dtsi b/arch/arm/boot/dts/omap3-n950-n9.dtsi
index 927b17fc4ed8..7c0c53e2f679 100644
--- a/arch/arm/boot/dts/omap3-n950-n9.dtsi
+++ b/arch/arm/boot/dts/omap3-n950-n9.dtsi
@@ -58,6 +58,13 @@
 			pinctrl-0 = <&debug_leds>;
 		};
 	};
+
+	/* controlled (enabled/disabled) directly by wl1271 */
+	vctcxo: vctcxo {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <38400000>;
+	};
 };
 
 &omap3_pmx_core {
@@ -125,6 +132,16 @@
 			OMAP3_CORE1_IOPAD(0x210a, PIN_OUTPUT | MUX_MODE4)            /* gpio_93 (cmt_apeslpx) */
 		>;
 	};
+
+	uart2_pins: pinmux_uart2_pins {
+		pinctrl-single,pins = <
+			OMAP3_CORE1_IOPAD(0x2174, PIN_INPUT | MUX_MODE0)		/* uart2_cts */
+			OMAP3_CORE1_IOPAD(0x2176, PIN_OUTPUT | MUX_MODE0)		/* uart2_rts */
+			OMAP3_CORE1_IOPAD(0x2178, PIN_OUTPUT | MUX_MODE0)		/* uart2_tx */
+			OMAP3_CORE1_IOPAD(0x217a, PIN_INPUT | MUX_MODE0)		/* uart2_rx */
+		>;
+	};
+
 };
 
 &omap3_pmx_core2 {
@@ -435,3 +452,20 @@
 &ssi_port2 {
 	status = "disabled";
 };
+
+&uart2 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart2_pins>;
+
+	bluetooth {
+		compatible = "nokia,ti,wl1271-bluetooth";
+
+		reset-gpios = <&gpio1 26 GPIO_ACTIVE_HIGH>; /* 26 */
+		host-wakeup-gpios = <&gpio4 5 GPIO_ACTIVE_HIGH>; /* 101 */
+		bluetooth-wakeup-gpios = <&gpio2 5 GPIO_ACTIVE_HIGH>; /* 37 */
+
+		clocks = <&vctcxo>;
+		clock-names = "sysclk";
+	};
+
+};
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 7/7] ARM: dts: OMAP3-N950: Add bluetooth

[Pavel Machek]

You'll need to sign off the patch.

Acked-by: Pavel Machek <pavel@ucw.cz>

							Pavel

On Sat 2016-08-13 05:14:38, Sebastian Reichel wrote:
> ---
>  arch/arm/boot/dts/omap3-n950-n9.dtsi | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/omap3-n950-n9.dtsi b/arch/arm/boot/dts/omap3-n950-n9.dtsi
> index 927b17fc4ed8..7c0c53e2f679 100644
> --- a/arch/arm/boot/dts/omap3-n950-n9.dtsi
> +++ b/arch/arm/boot/dts/omap3-n950-n9.dtsi
> @@ -58,6 +58,13 @@
>  			pinctrl-0 = <&debug_leds>;
>  		};
>  	};
> +
> +	/* controlled (enabled/disabled) directly by wl1271 */
> +	vctcxo: vctcxo {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <38400000>;
> +	};
>  };
>  
>  &omap3_pmx_core {
> @@ -125,6 +132,16 @@
>  			OMAP3_CORE1_IOPAD(0x210a, PIN_OUTPUT | MUX_MODE4)            /* gpio_93 (cmt_apeslpx) */
>  		>;
>  	};
> +
> +	uart2_pins: pinmux_uart2_pins {
> +		pinctrl-single,pins = <
> +			OMAP3_CORE1_IOPAD(0x2174, PIN_INPUT | MUX_MODE0)		/* uart2_cts */
> +			OMAP3_CORE1_IOPAD(0x2176, PIN_OUTPUT | MUX_MODE0)		/* uart2_rts */
> +			OMAP3_CORE1_IOPAD(0x2178, PIN_OUTPUT | MUX_MODE0)		/* uart2_tx */
> +			OMAP3_CORE1_IOPAD(0x217a, PIN_INPUT | MUX_MODE0)		/* uart2_rx */
> +		>;
> +	};
> +
>  };
>  
>  &omap3_pmx_core2 {
> @@ -435,3 +452,20 @@
>  &ssi_port2 {
>  	status = "disabled";
>  };
> +
> +&uart2 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart2_pins>;
> +
> +	bluetooth {
> +		compatible = "nokia,ti,wl1271-bluetooth";
> +
> +		reset-gpios = <&gpio1 26 GPIO_ACTIVE_HIGH>; /* 26 */
> +		host-wakeup-gpios = <&gpio4 5 GPIO_ACTIVE_HIGH>; /* 101 */
> +		bluetooth-wakeup-gpios = <&gpio2 5 GPIO_ACTIVE_HIGH>; /* 37 */
> +
> +		clocks = <&vctcxo>;
> +		clock-names = "sysclk";
> +	};
> +
> +};

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC 0/7] Nokia N9xx bluetooth driver

[Marcel Holtmann]

Hi Sebastian,

> This series (based von 4.8-rc1) adds support for bluetooth on the Nokia
> N9xx devices. It has been tested on the Nokia N950, where it works
> correctly. On Nokia N900 it currently fails during negotiation
> (probably related to slightly incorrect serial settings/timings).
> The N900's bcm2048 correctly answeres to alive check even before
> negotiation (on N950 it does not work before negotiation), but replies
> with an Hardware error event to the negotiation packet.
> 
> Apart from N900 support there are still two "features" missing in the
> driver:
> 
> 1. To save energy the bluetooth module can be put into sleep mode via a
>   GPIO. This gpio should be enabled before sending data via UART and
>   disabled once the transmission is done. I currently just keep the
>   GPIO always enabled.
> 2. It would be nice to have a bluetooth device exposed by the kernel
>   automatically without having to setup the tty disector first for
>   proper configurationless out of the box support. I could not find
>   a nice way to do this from the kernel, though.

currently using the HCI line discipline is the only way to attach a Bluetooth device to a serial line / UART.

However I have been advocating for a serial bus or UART bus for a long time now. It is needed especially for Bluetooth devices which have no business in being exposed as TTYs in the first place.

This is true for the ACPI and DT world actually. Some UARTs should not be exposed as TTY and just be stuck on a bus that can be enumerated and matched by a driver that knows how to handle it.

This goes along with the weird fetish to expose certain Bluetooth GPIOs as RFKILL switches. They are not that either since they just power the UART. They have nothing to do with a radio RFKILL switch. We fixed the Intel and Broadcom ones to be mapped into the driver. Having a proper bus would make this one also a lot easier.

Regards

Marcel

Re: [RFC 0/7] Nokia N9xx bluetooth driver

[Marcel Holtmann]

Hi Sebastian,

> This series (based von 4.8-rc1) adds support for bluetooth on the Nokia
> N9xx devices. It has been tested on the Nokia N950, where it works
> correctly. On Nokia N900 it currently fails during negotiation
> (probably related to slightly incorrect serial settings/timings).
> The N900's bcm2048 correctly answeres to alive check even before
> negotiation (on N950 it does not work before negotiation), but replies
> with an Hardware error event to the negotiation packet.
> 
> Apart from N900 support there are still two "features" missing in the
> driver:
> 
> 1. To save energy the bluetooth module can be put into sleep mode via a
>   GPIO. This gpio should be enabled before sending data via UART and
>   disabled once the transmission is done. I currently just keep the
>   GPIO always enabled.
> 2. It would be nice to have a bluetooth device exposed by the kernel
>   automatically without having to setup the tty disector first for
>   proper configurationless out of the box support. I could not find
>   a nice way to do this from the kernel, though.

currently using the HCI line discipline is the only way to attach a Bluetooth device to a serial line / UART.

However I have been advocating for a serial bus or UART bus for a long time now. It is needed especially for Bluetooth devices which have no business in being exposed as TTYs in the first place.

This is true for the ACPI and DT world actually. Some UARTs should not be exposed as TTY and just be stuck on a bus that can be enumerated and matched by a driver that knows how to handle it.

This goes along with the weird fetish to expose certain Bluetooth GPIOs as RFKILL switches. They are not that either since they just power the UART. They have nothing to do with a radio RFKILL switch. We fixed the Intel and Broadcom ones to be mapped into the driver. Having a proper bus would make this one also a lot easier.

Regards

Marcel

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 0/7] Nokia N9xx bluetooth driver

[Rob Herring]

On Tue, Aug 16, 2016 at 2:10 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Sebastian,
>
>> This series (based von 4.8-rc1) adds support for bluetooth on the Nokia
>> N9xx devices. It has been tested on the Nokia N950, where it works
>> correctly. On Nokia N900 it currently fails during negotiation
>> (probably related to slightly incorrect serial settings/timings).
>> The N900's bcm2048 correctly answeres to alive check even before
>> negotiation (on N950 it does not work before negotiation), but replies
>> with an Hardware error event to the negotiation packet.
>>
>> Apart from N900 support there are still two "features" missing in the
>> driver:
>>
>> 1. To save energy the bluetooth module can be put into sleep mode via a
>>   GPIO. This gpio should be enabled before sending data via UART and
>>   disabled once the transmission is done. I currently just keep the
>>   GPIO always enabled.
>> 2. It would be nice to have a bluetooth device exposed by the kernel
>>   automatically without having to setup the tty disector first for
>>   proper configurationless out of the box support. I could not find
>>   a nice way to do this from the kernel, though.
>
> currently using the HCI line discipline is the only way to attach a Bluetooth device to a serial line / UART.
>
> However I have been advocating for a serial bus or UART bus for a long time now. It is needed especially for Bluetooth devices which have no business in being exposed as TTYs in the first place.
>
> This is true for the ACPI and DT world actually. Some UARTs should not be exposed as TTY and just be stuck on a bus that can be enumerated and matched by a driver that knows how to handle it.

To add to this, my initial thought has been to split off uart_port ops
for use by the uart bus/subsystem so we can reuse all the uart
drivers. Then driver ports can be registered either with the tty layer
or the uart bus (or perhaps registered with both and claimed by one
later). This is somewhat already done with the serio subsys (see
drivers/tty/serial/sunsu.c). I thought extending serio might be an
option, but it's data handling is pretty limited.

The transmit side seems pretty straightforward. The receive side is a
bit more complex as the buffering is all in the tty layer. It doesn't
seem so straightforward to share the tty buffer handling code. Perhaps
at least initially, a more simple buffering scheme can be used given
the packet oriented nature of the device protocols.

Rob

Re: [RFC 0/7] Nokia N9xx bluetooth driver

[Rob Herring]

On Tue, Aug 16, 2016 at 2:10 AM, Marcel Holtmann <marcel-kz+m5ild9QBg9hUCZPvPmw@public.gmane.org> wrote:
> Hi Sebastian,
>
>> This series (based von 4.8-rc1) adds support for bluetooth on the Nokia
>> N9xx devices. It has been tested on the Nokia N950, where it works
>> correctly. On Nokia N900 it currently fails during negotiation
>> (probably related to slightly incorrect serial settings/timings).
>> The N900's bcm2048 correctly answeres to alive check even before
>> negotiation (on N950 it does not work before negotiation), but replies
>> with an Hardware error event to the negotiation packet.
>>
>> Apart from N900 support there are still two "features" missing in the
>> driver:
>>
>> 1. To save energy the bluetooth module can be put into sleep mode via a
>>   GPIO. This gpio should be enabled before sending data via UART and
>>   disabled once the transmission is done. I currently just keep the
>>   GPIO always enabled.
>> 2. It would be nice to have a bluetooth device exposed by the kernel
>>   automatically without having to setup the tty disector first for
>>   proper configurationless out of the box support. I could not find
>>   a nice way to do this from the kernel, though.
>
> currently using the HCI line discipline is the only way to attach a Bluetooth device to a serial line / UART.
>
> However I have been advocating for a serial bus or UART bus for a long time now. It is needed especially for Bluetooth devices which have no business in being exposed as TTYs in the first place.
>
> This is true for the ACPI and DT world actually. Some UARTs should not be exposed as TTY and just be stuck on a bus that can be enumerated and matched by a driver that knows how to handle it.

To add to this, my initial thought has been to split off uart_port ops
for use by the uart bus/subsystem so we can reuse all the uart
drivers. Then driver ports can be registered either with the tty layer
or the uart bus (or perhaps registered with both and claimed by one
later). This is somewhat already done with the serio subsys (see
drivers/tty/serial/sunsu.c). I thought extending serio might be an
option, but it's data handling is pretty limited.

The transmit side seems pretty straightforward. The receive side is a
bit more complex as the buffering is all in the tty layer. It doesn't
seem so straightforward to share the tty buffer handling code. Perhaps
at least initially, a more simple buffering scheme can be used given
the packet oriented nature of the device protocols.

Rob