[PATCH v2 0/5] Broadcom Bluetooth UART device driver

[PATCH v2 0/5] Broadcom Bluetooth UART device driver

[Ilya Faenson]

v2 - Release upon the acceptance of Fred's updates,
	updated as per the latest comments from Marcel.
v1 - Original release against the Fred Danis' updates.

Ilya Faenson (5):
  Broadcom Bluetooth UART Device Tree bindings
  H4 line discipline enhancements
  Broadcom Bluetooth UART Platform Driver
  Support the BCM4354 Bluetooth UART device
  BlueZ Broadcom UART Protocol

 .../devicetree/bindings/net/bluetooth/btbcm.txt    |  82 +++
 drivers/bluetooth/Kconfig                          |   9 +
 drivers/bluetooth/Makefile                         |   1 +
 drivers/bluetooth/btbcm.c                          |   2 +
 drivers/bluetooth/btbcm_uart.c                     | 673 +++++++++++++++++++++
 drivers/bluetooth/btbcm_uart.h                     |  89 +++
 drivers/bluetooth/hci_bcm.c                        | 400 +++++++++++-
 drivers/bluetooth/hci_ldisc.c                      |  66 +-
 drivers/bluetooth/hci_uart.h                       |   1 +
 9 files changed, 1305 insertions(+), 18 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/bluetooth/btbcm.txt
 create mode 100644 drivers/bluetooth/btbcm_uart.c
 create mode 100644 drivers/bluetooth/btbcm_uart.h

-- 
1.9.1

[PATCH v2 1/5] Broadcom Bluetooth UART Device Tree bindings

[Ilya Faenson]

Device Tree bindings to configure the Broadcom Bluetooth UART device.

Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
---
 .../devicetree/bindings/net/bluetooth/btbcm.txt    | 82 ++++++++++++++++++++++
 1 file changed, 82 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/bluetooth/btbcm.txt

diff --git a/Documentation/devicetree/bindings/net/bluetooth/btbcm.txt b/Documentation/devicetree/bindings/net/bluetooth/btbcm.txt
new file mode 100644
index 0000000..2679819
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/bluetooth/btbcm.txt
@@ -0,0 +1,82 @@
+btbcm
+------
+
+Required properties:
+
+	- compatible : must be "brcm,brcm-bt-uart".
+	- tty : tty device connected to this Bluetooth device.
+
+Optional properties:
+
+  - bt-host-wake-gpios : bt-host-wake input GPIO to be used as an interrupt.
+
+  - bt-wake-gpios : bt-wake output GPIO to be used to suspend / resume device.
+
+  - bt-reg-on-gpios : reg-on output GPIO to be used to power device on/off.
+
+  - oper-speed : Bluetooth device operational baud rate.
+    Default: 3000000.
+
+  - manual-fc : flow control UART in suspend / resume scenarios.
+    Default: 0.
+
+  - configure-sleep : configure suspend / resume flag.
+    Default: false.
+
+  - configure-audio : configure platform PCM SCO flag.
+    Default: false.
+
+  - pcm-clockmode : PCM clock mode. 0-slave, 1-master.
+    Default: 0.
+
+  - pcm-fillmethod : PCM fill method. 0 to 3.
+    Default: 2.
+
+  - pcm-fillnum : PCM number of fill bits. 0 to 3.
+    Default: 0.
+
+  - pcm-fillvalue : PCM fill value. 0 to 7.
+    Default: 3.
+
+  - pcm-incallbitclock : PCM interface rate. 0-128Kbps, 1-256Kbps, 2-512Kbps,
+    3-1024Kbps, 4-2048Kbps.
+    Default: 0.
+
+  - pcm-lsbfirst : PCM LSB first. 0 or 1.
+    Default: 0.
+
+  - pcm-rightjustify : PCM Justify. 0-left, 1-right.
+    Default: 0.
+
+  - pcm-routing : PCM routing. 0-PCM, 1-SCO over HCI.
+    Default: 0.
+
+  - pcm-shortframesync : PCM sync. 0-short, 1-long.
+    Default: 0.
+
+  - pcmsyncmode : PCM sync mode. 0-slave, 1-master.
+    Default: 0.
+
+
+Example:
+
+	brcm4354_bt_uart: brcm4354-bt-uart {
+		compatible = "brcm,brcm-bt-uart";
+		bt-wake-gpios = <&gpio4 30 GPIO_ACTIVE_HIGH>;
+		bt-host-wake-gpios = <&gpio4 31 GPIO_ACTIVE_HIGH>;
+		tty = "ttyS0";
+		oper-speed = <3000000>;
+		configure-sleep;
+		configure-audio;
+		pcm-clockmode = <0>;
+		pcm-fillmethod = <2>;
+		pcm-fillnum = <0>;
+		pcm-fillvalue = <3>;
+		pcm-incallbitclock = <0>;
+		pcm-lsbfirst = <0>;
+		pcm-rightjustify = <0>;
+		pcm-routing = <0>;
+		pcm-shortframesync = <0>;
+		pcm-syncmode = <0>;
+	};
+
-- 
1.9.1

Re: [PATCH v2 1/5] Broadcom Bluetooth UART Device Tree bindings

[Marcel Holtmann]

Hi Ilya,

> Device Tree bindings to configure the Broadcom Bluetooth UART device.
> 
> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
> ---
> .../devicetree/bindings/net/bluetooth/btbcm.txt    | 82 ++++++++++++++++++++++
> 1 file changed, 82 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/bluetooth/btbcm.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/bluetooth/btbcm.txt b/Documentation/devicetree/bindings/net/bluetooth/btbcm.txt
> new file mode 100644
> index 0000000..2679819
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/bluetooth/btbcm.txt
> @@ -0,0 +1,82 @@
> +btbcm
> +------
> +
> +Required properties:
> +
> +	- compatible : must be "brcm,brcm-bt-uart".
> +	- tty : tty device connected to this Bluetooth device.
> +
> +Optional properties:
> +
> +  - bt-host-wake-gpios : bt-host-wake input GPIO to be used as an interrupt.
> +
> +  - bt-wake-gpios : bt-wake output GPIO to be used to suspend / resume device.
> +
> +  - bt-reg-on-gpios : reg-on output GPIO to be used to power device on/off.
> +
> +  - oper-speed : Bluetooth device operational baud rate.
> +    Default: 3000000.
> +
> +  - manual-fc : flow control UART in suspend / resume scenarios.
> +    Default: 0.
> +
> +  - configure-sleep : configure suspend / resume flag.
> +    Default: false.
> +
> +  - configure-audio : configure platform PCM SCO flag.
> +    Default: false.
> +
> +  - pcm-clockmode : PCM clock mode. 0-slave, 1-master.
> +    Default: 0.
> +
> +  - pcm-fillmethod : PCM fill method. 0 to 3.
> +    Default: 2.
> +
> +  - pcm-fillnum : PCM number of fill bits. 0 to 3.
> +    Default: 0.
> +
> +  - pcm-fillvalue : PCM fill value. 0 to 7.
> +    Default: 3.
> +
> +  - pcm-incallbitclock : PCM interface rate. 0-128Kbps, 1-256Kbps, 2-512Kbps,
> +    3-1024Kbps, 4-2048Kbps.
> +    Default: 0.
> +
> +  - pcm-lsbfirst : PCM LSB first. 0 or 1.
> +    Default: 0.
> +
> +  - pcm-rightjustify : PCM Justify. 0-left, 1-right.
> +    Default: 0.
> +
> +  - pcm-routing : PCM routing. 0-PCM, 1-SCO over HCI.
> +    Default: 0.
> +
> +  - pcm-shortframesync : PCM sync. 0-short, 1-long.
> +    Default: 0.
> +
> +  - pcmsyncmode : PCM sync mode. 0-slave, 1-master.
> +    Default: 0.

I think you meant pcm-syncmode here.

> +
> +
> +Example:
> +
> +	brcm4354_bt_uart: brcm4354-bt-uart {

Since in general the chips and firmware name refer to BCM4354, it might be a really good idea to use bcm4354 here and not confuse people further. I am just trying to avoid confusion here.

Regards

Marcel

RE: [PATCH v2 1/5] Broadcom Bluetooth UART Device Tree bindings

[Ilya Faenson]

Hi Marcel,

-----Original Message-----
From: Marcel Holtmann [mailto:marcel@holtmann.org] 
Sent: Thursday, June 11, 2015 5:40 AM
To: Ilya Faenson
Cc: linux-bluetooth@vger.kernel.org; Arend Van Spriel
Subject: Re: [PATCH v2 1/5] Broadcom Bluetooth UART Device Tree bindings

Hi Ilya,

> Device Tree bindings to configure the Broadcom Bluetooth UART device.
> 
> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
> ---
> .../devicetree/bindings/net/bluetooth/btbcm.txt    | 82 ++++++++++++++++++++++
> 1 file changed, 82 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/bluetooth/btbcm.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/bluetooth/btbcm.txt b/Documentation/devicetree/bindings/net/bluetooth/btbcm.txt
> new file mode 100644
> index 0000000..2679819
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/bluetooth/btbcm.txt
> @@ -0,0 +1,82 @@
> +btbcm
> +------
> +
> +Required properties:
> +
> +	- compatible : must be "brcm,brcm-bt-uart".
> +	- tty : tty device connected to this Bluetooth device.
> +
> +Optional properties:
> +
> +  - bt-host-wake-gpios : bt-host-wake input GPIO to be used as an interrupt.
> +
> +  - bt-wake-gpios : bt-wake output GPIO to be used to suspend / resume device.
> +
> +  - bt-reg-on-gpios : reg-on output GPIO to be used to power device on/off.
> +
> +  - oper-speed : Bluetooth device operational baud rate.
> +    Default: 3000000.
> +
> +  - manual-fc : flow control UART in suspend / resume scenarios.
> +    Default: 0.
> +
> +  - configure-sleep : configure suspend / resume flag.
> +    Default: false.
> +
> +  - configure-audio : configure platform PCM SCO flag.
> +    Default: false.
> +
> +  - pcm-clockmode : PCM clock mode. 0-slave, 1-master.
> +    Default: 0.
> +
> +  - pcm-fillmethod : PCM fill method. 0 to 3.
> +    Default: 2.
> +
> +  - pcm-fillnum : PCM number of fill bits. 0 to 3.
> +    Default: 0.
> +
> +  - pcm-fillvalue : PCM fill value. 0 to 7.
> +    Default: 3.
> +
> +  - pcm-incallbitclock : PCM interface rate. 0-128Kbps, 1-256Kbps, 2-512Kbps,
> +    3-1024Kbps, 4-2048Kbps.
> +    Default: 0.
> +
> +  - pcm-lsbfirst : PCM LSB first. 0 or 1.
> +    Default: 0.
> +
> +  - pcm-rightjustify : PCM Justify. 0-left, 1-right.
> +    Default: 0.
> +
> +  - pcm-routing : PCM routing. 0-PCM, 1-SCO over HCI.
> +    Default: 0.
> +
> +  - pcm-shortframesync : PCM sync. 0-short, 1-long.
> +    Default: 0.
> +
> +  - pcmsyncmode : PCM sync mode. 0-slave, 1-master.
> +    Default: 0.

I think you meant pcm-syncmode here.

IF: That's right, will change.
> +
> +
> +Example:
> +
> +	brcm4354_bt_uart: brcm4354-bt-uart {

Since in general the chips and firmware name refer to BCM4354, it might be a really good idea to use bcm4354 here and not confuse people further. I am just trying to avoid confusion here.

IF: You're right, the chip name is BCM4354 but we must use the "brcm" prefix in the device tree bindings. Alright, I will change these to start from the "bcm" but will keep the "brcm" in the "compatible" strings. That would also be somewhat confusing but possibly not as confusing as now.

Regards

Marcel

Re: [PATCH v2 1/5] Broadcom Bluetooth UART Device Tree bindings

[Marcel Holtmann]

Hi Ilya,

>> Device Tree bindings to configure the Broadcom Bluetooth UART device.
>> 
>> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
>> ---
>> .../devicetree/bindings/net/bluetooth/btbcm.txt    | 82 ++++++++++++++++++++++
>> 1 file changed, 82 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/net/bluetooth/btbcm.txt
>> 
>> diff --git a/Documentation/devicetree/bindings/net/bluetooth/btbcm.txt b/Documentation/devicetree/bindings/net/bluetooth/btbcm.txt
>> new file mode 100644
>> index 0000000..2679819
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/bluetooth/btbcm.txt
>> @@ -0,0 +1,82 @@
>> +btbcm
>> +------
>> +
>> +Required properties:
>> +
>> +	- compatible : must be "brcm,brcm-bt-uart".
>> +	- tty : tty device connected to this Bluetooth device.
>> +
>> +Optional properties:
>> +
>> +  - bt-host-wake-gpios : bt-host-wake input GPIO to be used as an interrupt.
>> +
>> +  - bt-wake-gpios : bt-wake output GPIO to be used to suspend / resume device.
>> +
>> +  - bt-reg-on-gpios : reg-on output GPIO to be used to power device on/off.
>> +
>> +  - oper-speed : Bluetooth device operational baud rate.
>> +    Default: 3000000.
>> +
>> +  - manual-fc : flow control UART in suspend / resume scenarios.
>> +    Default: 0.
>> +
>> +  - configure-sleep : configure suspend / resume flag.
>> +    Default: false.
>> +
>> +  - configure-audio : configure platform PCM SCO flag.
>> +    Default: false.
>> +
>> +  - pcm-clockmode : PCM clock mode. 0-slave, 1-master.
>> +    Default: 0.
>> +
>> +  - pcm-fillmethod : PCM fill method. 0 to 3.
>> +    Default: 2.
>> +
>> +  - pcm-fillnum : PCM number of fill bits. 0 to 3.
>> +    Default: 0.
>> +
>> +  - pcm-fillvalue : PCM fill value. 0 to 7.
>> +    Default: 3.
>> +
>> +  - pcm-incallbitclock : PCM interface rate. 0-128Kbps, 1-256Kbps, 2-512Kbps,
>> +    3-1024Kbps, 4-2048Kbps.
>> +    Default: 0.
>> +
>> +  - pcm-lsbfirst : PCM LSB first. 0 or 1.
>> +    Default: 0.
>> +
>> +  - pcm-rightjustify : PCM Justify. 0-left, 1-right.
>> +    Default: 0.
>> +
>> +  - pcm-routing : PCM routing. 0-PCM, 1-SCO over HCI.
>> +    Default: 0.
>> +
>> +  - pcm-shortframesync : PCM sync. 0-short, 1-long.
>> +    Default: 0.
>> +
>> +  - pcmsyncmode : PCM sync mode. 0-slave, 1-master.
>> +    Default: 0.
> 
> I think you meant pcm-syncmode here.
> 
> IF: That's right, will change.
>> +
>> +
>> +Example:
>> +
>> +	brcm4354_bt_uart: brcm4354-bt-uart {
> 
> Since in general the chips and firmware name refer to BCM4354, it might be a really good idea to use bcm4354 here and not confuse people further. I am just trying to avoid confusion here.
> 
> IF: You're right, the chip name is BCM4354 but we must use the "brcm" prefix in the device tree bindings. Alright, I will change these to start from the "bcm" but will keep the "brcm" in the "compatible" strings. That would also be somewhat confusing but possibly not as confusing as now.

I think Arend had a good proposal there. If I remember correctly then when it refers to Broadcom as generic DT identifier, then it is brcm. If it is device specific use the actual name bcm1234. SO things like brcm,bcm1234 made sense to at least me.

Regards

Marcel

RE: [PATCH v2 1/5] Broadcom Bluetooth UART Device Tree bindings

[Ilya Faenson]

Hi Marcel,

-----Original Message-----
From: Marcel Holtmann [mailto:marcel@holtmann.org]=20
Sent: Friday, June 12, 2015 11:39 AM
To: Ilya Faenson
Cc: linux-bluetooth@vger.kernel.org; Arend Van Spriel
Subject: Re: [PATCH v2 1/5] Broadcom Bluetooth UART Device Tree bindings

Hi Ilya,

>> Device Tree bindings to configure the Broadcom Bluetooth UART device.
>>=20
>> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
>> ---
>> .../devicetree/bindings/net/bluetooth/btbcm.txt    | 82 ++++++++++++++++=
++++++
>> 1 file changed, 82 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/net/bluetooth/btbcm=
.txt
>>=20
>> diff --git a/Documentation/devicetree/bindings/net/bluetooth/btbcm.txt b=
/Documentation/devicetree/bindings/net/bluetooth/btbcm.txt
>> new file mode 100644
>> index 0000000..2679819
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/bluetooth/btbcm.txt
>> @@ -0,0 +1,82 @@
>> +btbcm
>> +------
>> +
>> +Required properties:
>> +
>> +	- compatible : must be "brcm,brcm-bt-uart".
>> +	- tty : tty device connected to this Bluetooth device.
>> +
>> +Optional properties:
>> +
>> +  - bt-host-wake-gpios : bt-host-wake input GPIO to be used as an inter=
rupt.
>> +
>> +  - bt-wake-gpios : bt-wake output GPIO to be used to suspend / resume =
device.
>> +
>> +  - bt-reg-on-gpios : reg-on output GPIO to be used to power device on/=
off.
>> +
>> +  - oper-speed : Bluetooth device operational baud rate.
>> +    Default: 3000000.
>> +
>> +  - manual-fc : flow control UART in suspend / resume scenarios.
>> +    Default: 0.
>> +
>> +  - configure-sleep : configure suspend / resume flag.
>> +    Default: false.
>> +
>> +  - configure-audio : configure platform PCM SCO flag.
>> +    Default: false.
>> +
>> +  - pcm-clockmode : PCM clock mode. 0-slave, 1-master.
>> +    Default: 0.
>> +
>> +  - pcm-fillmethod : PCM fill method. 0 to 3.
>> +    Default: 2.
>> +
>> +  - pcm-fillnum : PCM number of fill bits. 0 to 3.
>> +    Default: 0.
>> +
>> +  - pcm-fillvalue : PCM fill value. 0 to 7.
>> +    Default: 3.
>> +
>> +  - pcm-incallbitclock : PCM interface rate. 0-128Kbps, 1-256Kbps, 2-51=
2Kbps,
>> +    3-1024Kbps, 4-2048Kbps.
>> +    Default: 0.
>> +
>> +  - pcm-lsbfirst : PCM LSB first. 0 or 1.
>> +    Default: 0.
>> +
>> +  - pcm-rightjustify : PCM Justify. 0-left, 1-right.
>> +    Default: 0.
>> +
>> +  - pcm-routing : PCM routing. 0-PCM, 1-SCO over HCI.
>> +    Default: 0.
>> +
>> +  - pcm-shortframesync : PCM sync. 0-short, 1-long.
>> +    Default: 0.
>> +
>> +  - pcmsyncmode : PCM sync mode. 0-slave, 1-master.
>> +    Default: 0.
>=20
> I think you meant pcm-syncmode here.
>=20
> IF: That's right, will change.
>> +
>> +
>> +Example:
>> +
>> +	brcm4354_bt_uart: brcm4354-bt-uart {
>=20
> Since in general the chips and firmware name refer to BCM4354, it might b=
e a really good idea to use bcm4354 here and not confuse people further. I =
am just trying to avoid confusion here.
>=20
> IF: You're right, the chip name is BCM4354 but we must use the "brcm" pre=
fix in the device tree bindings. Alright, I will change these to start from=
 the "bcm" but will keep the "brcm" in the "compatible" strings. That would=
 also be somewhat confusing but possibly not as confusing as now.

I think Arend had a good proposal there. If I remember correctly then when =
it refers to Broadcom as generic DT identifier, then it is brcm. If it is d=
evice specific use the actual name bcm1234. SO things like brcm,bcm1234 mad=
e sense to at least me.

IF: Agreed.

Regards

Marcel

[PATCH v2 2/5] H4 line discipline enhancements

[Ilya Faenson]

Added the ability to flow control the UART and improved the
UART baud rate setting.

Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
---
 drivers/bluetooth/hci_ldisc.c | 66 +++++++++++++++++++++++++++++++++++++++++--
 drivers/bluetooth/hci_uart.h  |  1 +
 2 files changed, 65 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index ac87346..f86e973 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -266,20 +266,82 @@ static int hci_uart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
 	return 0;
 }
 
+/* Flow control or un-flow control the device */
+void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
+{
+	struct tty_struct *tty = hu->tty;
+	struct ktermios ktermios;
+	int status;
+	unsigned int set = 0;
+	unsigned int clear = 0;
+
+	if (enable) {
+		/* Disable hardware flow control */
+		ktermios = tty->termios;
+		ktermios.c_cflag &= ~CRTSCTS;
+		status = tty_set_termios(tty, &ktermios);
+		BT_DBG("Disabling hardware flow control: %s", status ?
+		       "failed" : "success");
+
+		/* Clear RTS to prevent the device from sending */
+		/* Most UARTs need OUT2 to enable interrupts */
+		status = tty->driver->ops->tiocmget(tty);
+		BT_DBG("Current tiocm 0x%x", status);
+
+		set &= ~(TIOCM_OUT2 | TIOCM_RTS);
+		clear = ~set;
+		set &= TIOCM_DTR | TIOCM_RTS | TIOCM_OUT1 |
+		       TIOCM_OUT2 | TIOCM_LOOP;
+		clear &= TIOCM_DTR | TIOCM_RTS | TIOCM_OUT1 |
+			 TIOCM_OUT2 | TIOCM_LOOP;
+		status = tty->driver->ops->tiocmset(tty, set, clear);
+		BT_DBG("Clearing RTS: %s", status ? "failed" : "success");
+	} else {
+		/* Set RTS to allow the device to send again */
+		status = tty->driver->ops->tiocmget(tty);
+		BT_DBG("Current tiocm 0x%x", status);
+
+		set |= (TIOCM_OUT2 | TIOCM_RTS);
+		clear = ~set;
+		set &= TIOCM_DTR | TIOCM_RTS | TIOCM_OUT1 |
+		       TIOCM_OUT2 | TIOCM_LOOP;
+		clear &= TIOCM_DTR | TIOCM_RTS | TIOCM_OUT1 |
+			 TIOCM_OUT2 | TIOCM_LOOP;
+		status = tty->driver->ops->tiocmset(tty, set, clear);
+		BT_DBG("Setting RTS: %s", status ? "failed" : "success");
+
+		/* Re-enable hardware flow control */
+		ktermios = tty->termios;
+		ktermios.c_cflag |= CRTSCTS;
+		status = tty_set_termios(tty, &ktermios);
+		BT_DBG("Enabling hardware flow control: %s", status ?
+		       "failed" : "success");
+	}
+}
+
 void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed)
 {
 	struct tty_struct *tty = hu->tty;
 	struct ktermios ktermios;
 
+	/* Bring the UART into a known state with a given baud rate */
 	ktermios = tty->termios;
 	ktermios.c_cflag &= ~CBAUD;
-	ktermios.c_cflag |= BOTHER;
+	ktermios.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP
+			    | INLCR | IGNCR | ICRNL | IXON);
+	ktermios.c_oflag &= ~OPOST;
+	ktermios.c_lflag &= ~(ECHO | ECHONL | ICANON | ISIG | IEXTEN);
+	ktermios.c_cflag &= ~(CSIZE | PARENB | CBAUD);
+	ktermios.c_cflag |= CS8;
+	ktermios.c_cflag |= CRTSCTS;
 	tty_termios_encode_baud_rate(&ktermios, speed, speed);
 
 	/* tty_set_termios() return not checked as it is always 0 */
 	tty_set_termios(tty, &ktermios);
 
-	BT_DBG("%s: New tty speed: %d", hu->hdev->name, tty->termios.c_ispeed);
+	BT_DBG("%s: New tty speeds: %d/%d, cflag: 0x%x", hu->hdev->name,
+	       tty->termios.c_ispeed, tty->termios.c_ospeed,
+	       tty->termios.c_cflag);
 }
 
 static int hci_uart_setup(struct hci_dev *hdev)
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index e9f970c..8ef1881 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -100,6 +100,7 @@ int hci_uart_unregister_proto(const struct hci_uart_proto *p);
 int hci_uart_tx_wakeup(struct hci_uart *hu);
 int hci_uart_init_ready(struct hci_uart *hu);
 void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed);
+void hci_uart_set_flow_control(struct hci_uart *hu, bool enable);
 
 #ifdef CONFIG_BT_HCIUART_H4
 int h4_init(void);
-- 
1.9.1

Re: [PATCH v2 2/5] H4 line discipline enhancements

[Marcel Holtmann]

Hi Ilya,

> Added the ability to flow control the UART and improved the
> UART baud rate setting.
> 
> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
> ---
> drivers/bluetooth/hci_ldisc.c | 66 +++++++++++++++++++++++++++++++++++++++++--
> drivers/bluetooth/hci_uart.h  |  1 +
> 2 files changed, 65 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index ac87346..f86e973 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -266,20 +266,82 @@ static int hci_uart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
> 	return 0;
> }
> 
> +/* Flow control or un-flow control the device */
> +void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
> +{
> +	struct tty_struct *tty = hu->tty;
> +	struct ktermios ktermios;
> +	int status;
> +	unsigned int set = 0;
> +	unsigned int clear = 0;
> +
> +	if (enable) {
> +		/* Disable hardware flow control */
> +		ktermios = tty->termios;
> +		ktermios.c_cflag &= ~CRTSCTS;
> +		status = tty_set_termios(tty, &ktermios);
> +		BT_DBG("Disabling hardware flow control: %s", status ?
> +		       "failed" : "success");
> +
> +		/* Clear RTS to prevent the device from sending */
> +		/* Most UARTs need OUT2 to enable interrupts */
> +		status = tty->driver->ops->tiocmget(tty);
> +		BT_DBG("Current tiocm 0x%x", status);
> +
> +		set &= ~(TIOCM_OUT2 | TIOCM_RTS);
> +		clear = ~set;
> +		set &= TIOCM_DTR | TIOCM_RTS | TIOCM_OUT1 |
> +		       TIOCM_OUT2 | TIOCM_LOOP;
> +		clear &= TIOCM_DTR | TIOCM_RTS | TIOCM_OUT1 |
> +			 TIOCM_OUT2 | TIOCM_LOOP;
> +		status = tty->driver->ops->tiocmset(tty, set, clear);
> +		BT_DBG("Clearing RTS: %s", status ? "failed" : "success");
> +	} else {
> +		/* Set RTS to allow the device to send again */
> +		status = tty->driver->ops->tiocmget(tty);
> +		BT_DBG("Current tiocm 0x%x", status);
> +
> +		set |= (TIOCM_OUT2 | TIOCM_RTS);
> +		clear = ~set;
> +		set &= TIOCM_DTR | TIOCM_RTS | TIOCM_OUT1 |
> +		       TIOCM_OUT2 | TIOCM_LOOP;
> +		clear &= TIOCM_DTR | TIOCM_RTS | TIOCM_OUT1 |
> +			 TIOCM_OUT2 | TIOCM_LOOP;
> +		status = tty->driver->ops->tiocmset(tty, set, clear);
> +		BT_DBG("Setting RTS: %s", status ? "failed" : "success");
> +
> +		/* Re-enable hardware flow control */
> +		ktermios = tty->termios;
> +		ktermios.c_cflag |= CRTSCTS;
> +		status = tty_set_termios(tty, &ktermios);
> +		BT_DBG("Enabling hardware flow control: %s", status ?
> +		       "failed" : "success");
> +	}
> +}
> +
> void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed)
> {
> 	struct tty_struct *tty = hu->tty;
> 	struct ktermios ktermios;
> 
> +	/* Bring the UART into a known state with a given baud rate */
> 	ktermios = tty->termios;
> 	ktermios.c_cflag &= ~CBAUD;
> -	ktermios.c_cflag |= BOTHER;
> +	ktermios.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP
> +			    | INLCR | IGNCR | ICRNL | IXON);

can we put the | at the end of the previous line and keep IGNBRK and INLCR aligned.

Honestly I have no idea what the absolute 100% correct coding style rules are here, but at least lets try to be consistent. So referring to the code above, the | should be at the end.

> +	ktermios.c_oflag &= ~OPOST;
> +	ktermios.c_lflag &= ~(ECHO | ECHONL | ICANON | ISIG | IEXTEN);
> +	ktermios.c_cflag &= ~(CSIZE | PARENB | CBAUD);
> +	ktermios.c_cflag |= CS8;
> +	ktermios.c_cflag |= CRTSCTS;
> 	tty_termios_encode_baud_rate(&ktermios, speed, speed);
> 
> 	/* tty_set_termios() return not checked as it is always 0 */
> 	tty_set_termios(tty, &ktermios);
> 
> -	BT_DBG("%s: New tty speed: %d", hu->hdev->name, tty->termios.c_ispeed);
> +	BT_DBG("%s: New tty speeds: %d/%d, cflag: 0x%x", hu->hdev->name,
> +	       tty->termios.c_ispeed, tty->termios.c_ospeed,
> +	       tty->termios.c_cflag);
> }
> 
> static int hci_uart_setup(struct hci_dev *hdev)
> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> index e9f970c..8ef1881 100644
> --- a/drivers/bluetooth/hci_uart.h
> +++ b/drivers/bluetooth/hci_uart.h
> @@ -100,6 +100,7 @@ int hci_uart_unregister_proto(const struct hci_uart_proto *p);
> int hci_uart_tx_wakeup(struct hci_uart *hu);
> int hci_uart_init_ready(struct hci_uart *hu);
> void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed);
> +void hci_uart_set_flow_control(struct hci_uart *hu, bool enable);

The rest looks good.

Regards

Marcel

RE: [PATCH v2 2/5] H4 line discipline enhancements

[Ilya Faenson]

Hi Marcel,

-----Original Message-----
From: Marcel Holtmann [mailto:marcel@holtmann.org]=20
Sent: Thursday, June 11, 2015 6:05 AM
To: Ilya Faenson
Cc: linux-bluetooth@vger.kernel.org; Arend Van Spriel
Subject: Re: [PATCH v2 2/5] H4 line discipline enhancements

Hi Ilya,

> Added the ability to flow control the UART and improved the
> UART baud rate setting.
>=20
> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
> ---
> drivers/bluetooth/hci_ldisc.c | 66 ++++++++++++++++++++++++++++++++++++++=
+++--
> drivers/bluetooth/hci_uart.h  |  1 +
> 2 files changed, 65 insertions(+), 2 deletions(-)
>=20
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.=
c
> index ac87346..f86e973 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -266,20 +266,82 @@ static int hci_uart_send_frame(struct hci_dev *hdev=
, struct sk_buff *skb)
> 	return 0;
> }
>=20
> +/* Flow control or un-flow control the device */
> +void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
> +{
> +	struct tty_struct *tty =3D hu->tty;
> +	struct ktermios ktermios;
> +	int status;
> +	unsigned int set =3D 0;
> +	unsigned int clear =3D 0;
> +
> +	if (enable) {
> +		/* Disable hardware flow control */
> +		ktermios =3D tty->termios;
> +		ktermios.c_cflag &=3D ~CRTSCTS;
> +		status =3D tty_set_termios(tty, &ktermios);
> +		BT_DBG("Disabling hardware flow control: %s", status ?
> +		       "failed" : "success");
> +
> +		/* Clear RTS to prevent the device from sending */
> +		/* Most UARTs need OUT2 to enable interrupts */
> +		status =3D tty->driver->ops->tiocmget(tty);
> +		BT_DBG("Current tiocm 0x%x", status);
> +
> +		set &=3D ~(TIOCM_OUT2 | TIOCM_RTS);
> +		clear =3D ~set;
> +		set &=3D TIOCM_DTR | TIOCM_RTS | TIOCM_OUT1 |
> +		       TIOCM_OUT2 | TIOCM_LOOP;
> +		clear &=3D TIOCM_DTR | TIOCM_RTS | TIOCM_OUT1 |
> +			 TIOCM_OUT2 | TIOCM_LOOP;
> +		status =3D tty->driver->ops->tiocmset(tty, set, clear);
> +		BT_DBG("Clearing RTS: %s", status ? "failed" : "success");
> +	} else {
> +		/* Set RTS to allow the device to send again */
> +		status =3D tty->driver->ops->tiocmget(tty);
> +		BT_DBG("Current tiocm 0x%x", status);
> +
> +		set |=3D (TIOCM_OUT2 | TIOCM_RTS);
> +		clear =3D ~set;
> +		set &=3D TIOCM_DTR | TIOCM_RTS | TIOCM_OUT1 |
> +		       TIOCM_OUT2 | TIOCM_LOOP;
> +		clear &=3D TIOCM_DTR | TIOCM_RTS | TIOCM_OUT1 |
> +			 TIOCM_OUT2 | TIOCM_LOOP;
> +		status =3D tty->driver->ops->tiocmset(tty, set, clear);
> +		BT_DBG("Setting RTS: %s", status ? "failed" : "success");
> +
> +		/* Re-enable hardware flow control */
> +		ktermios =3D tty->termios;
> +		ktermios.c_cflag |=3D CRTSCTS;
> +		status =3D tty_set_termios(tty, &ktermios);
> +		BT_DBG("Enabling hardware flow control: %s", status ?
> +		       "failed" : "success");
> +	}
> +}
> +
> void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed)
> {
> 	struct tty_struct *tty =3D hu->tty;
> 	struct ktermios ktermios;
>=20
> +	/* Bring the UART into a known state with a given baud rate */
> 	ktermios =3D tty->termios;
> 	ktermios.c_cflag &=3D ~CBAUD;
> -	ktermios.c_cflag |=3D BOTHER;
> +	ktermios.c_iflag &=3D ~(IGNBRK | BRKINT | PARMRK | ISTRIP
> +			    | INLCR | IGNCR | ICRNL | IXON);

can we put the | at the end of the previous line and keep IGNBRK and INLCR =
aligned.

Honestly I have no idea what the absolute 100% correct coding style rules a=
re here, but at least lets try to be consistent. So referring to the code a=
bove, the | should be at the end.

IF: Okay, will do.

> +	ktermios.c_oflag &=3D ~OPOST;
> +	ktermios.c_lflag &=3D ~(ECHO | ECHONL | ICANON | ISIG | IEXTEN);
> +	ktermios.c_cflag &=3D ~(CSIZE | PARENB | CBAUD);
> +	ktermios.c_cflag |=3D CS8;
> +	ktermios.c_cflag |=3D CRTSCTS;
> 	tty_termios_encode_baud_rate(&ktermios, speed, speed);
>=20
> 	/* tty_set_termios() return not checked as it is always 0 */
> 	tty_set_termios(tty, &ktermios);
>=20
> -	BT_DBG("%s: New tty speed: %d", hu->hdev->name, tty->termios.c_ispeed);
> +	BT_DBG("%s: New tty speeds: %d/%d, cflag: 0x%x", hu->hdev->name,
> +	       tty->termios.c_ispeed, tty->termios.c_ospeed,
> +	       tty->termios.c_cflag);
> }
>=20
> static int hci_uart_setup(struct hci_dev *hdev)
> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> index e9f970c..8ef1881 100644
> --- a/drivers/bluetooth/hci_uart.h
> +++ b/drivers/bluetooth/hci_uart.h
> @@ -100,6 +100,7 @@ int hci_uart_unregister_proto(const struct hci_uart_p=
roto *p);
> int hci_uart_tx_wakeup(struct hci_uart *hu);
> int hci_uart_init_ready(struct hci_uart *hu);
> void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed);
> +void hci_uart_set_flow_control(struct hci_uart *hu, bool enable);

The rest looks good.

Regards

Marcel

[PATCH v2 3/5] Broadcom Bluetooth UART Platform Driver

[Ilya Faenson]

Introduce the device tree enumerated Broadcom Bluetooth UART driver.

Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
---
 drivers/bluetooth/Kconfig      |   9 +
 drivers/bluetooth/Makefile     |   1 +
 drivers/bluetooth/btbcm_uart.c | 673 +++++++++++++++++++++++++++++++++++++++++
 drivers/bluetooth/btbcm_uart.h |  89 ++++++
 4 files changed, 772 insertions(+)
 create mode 100644 drivers/bluetooth/btbcm_uart.c
 create mode 100644 drivers/bluetooth/btbcm_uart.h

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index 2e77707..5eee3ed 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -143,6 +143,7 @@ config BT_HCIUART_BCM
 	bool "Broadcom protocol support"
 	depends on BT_HCIUART
 	select BT_HCIUART_H4
+	select BT_UART_BCM
 	select BT_BCM
 	help
 	  The Broadcom protocol support enables Bluetooth HCI over serial
@@ -150,6 +151,14 @@ config BT_HCIUART_BCM
 
 	  Say Y here to compile support for Broadcom protocol.
 
+config BT_UART_BCM
+	tristate "Broadcom BT UART driver"
+	depends on BT_HCIUART_H4 && TTY
+	help
+	  This driver supports the HCI_UART_BT protocol.
+
+	  It manages Bluetooth UART device properties and GPIOs.
+
 config BT_HCIBCM203X
 	tristate "HCI BCM203x USB driver"
 	depends on USB
diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
index f40e194..e908a88 100644
--- a/drivers/bluetooth/Makefile
+++ b/drivers/bluetooth/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_BT_MRVL)		+= btmrvl.o
 obj-$(CONFIG_BT_MRVL_SDIO)	+= btmrvl_sdio.o
 obj-$(CONFIG_BT_WILINK)		+= btwilink.o
 obj-$(CONFIG_BT_BCM)		+= btbcm.o
+obj-$(CONFIG_BT_UART_BCM)	+= btbcm_uart.o
 obj-$(CONFIG_BT_RTL)		+= btrtl.o
 
 btmrvl-y			:= btmrvl_main.o
diff --git a/drivers/bluetooth/btbcm_uart.c b/drivers/bluetooth/btbcm_uart.c
new file mode 100644
index 0000000..ccd02a9
--- /dev/null
+++ b/drivers/bluetooth/btbcm_uart.c
@@ -0,0 +1,673 @@
+/*
+ *  Bluetooth BCM UART Driver
+ *
+ *  Copyright (c) 2015 Broadcom Corporation
+ *
+ *  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/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/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/list.h>
+
+#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci_core.h>
+
+#include <linux/gpio/consumer.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/of_platform.h>
+
+#include "btbcm_uart.h"
+
+static int idle_timeout = 5;
+module_param(idle_timeout, int, 0);
+MODULE_PARM_DESC(idle_timeout, "Bluetooth idle timeout in seconds");
+
+/* Device context */
+struct bcm_device {
+	struct list_head list;
+
+	struct platform_device *pdev;
+	struct gpio_desc *bt_wake_gpio;
+	struct gpio_desc *dev_wake_gpio;
+	struct gpio_desc *reg_on_gpio;
+	int bt_wake_irq;
+	int dev_wake_active_low;
+	int reg_on_active_low;
+	int bt_wake_active_low;
+	bool configure_sleep;
+	u32 manual_fc;
+	u32 oper_speed;
+	bool configure_audio;
+	u32 pcm_clockmode;
+	u32 pcm_fillmethod;
+	u32 pcm_fillnum;
+	u32 pcm_fillvalue;
+	u32 pcm_incallbitclock;
+	u32 pcm_lsbfirst;
+	u32 pcm_rightjustify;
+	u32 pcm_routing;
+	u32 pcm_shortframesync;
+	u32 pcm_syncmode;
+
+	char tty_name[64];
+
+	struct btbcm_uart_callbacks protocol_callbacks;
+	struct work_struct wakeup_work;
+};
+
+/* List of BCM BT UART devices */
+static DEFINE_SPINLOCK(device_list_lock);
+static LIST_HEAD(device_list);
+
+/*
+ * Calling the BCM protocol at lower execution priority
+ */
+static void bcm_bt_wakeup_task(struct work_struct *ws)
+{
+	int gpio_value;
+	struct bcm_device *p_bcm_device =
+		container_of(ws, struct bcm_device, wakeup_work);
+
+	if (!p_bcm_device) {
+		BT_DBG("%s - failing, no device", __func__);
+		return;
+	}
+
+	/* Make sure the device is resumed */
+	gpio_value = !p_bcm_device->dev_wake_active_low;
+	if (p_bcm_device->dev_wake_gpio) {
+		gpiod_set_value(p_bcm_device->dev_wake_gpio, gpio_value);
+		BT_DBG("%s - resume %d written, delaying 15 ms",
+		       __func__, gpio_value);
+		mdelay(15);
+	}
+
+	/* Let the protocol know it's time to wake up */
+	if (p_bcm_device->protocol_callbacks.p_wakeup)
+		p_bcm_device->protocol_callbacks.p_wakeup(
+			p_bcm_device->protocol_callbacks.context);
+}
+
+/*
+ * Interrupt routine for the wake from the device
+ */
+static irqreturn_t bcm_bt_uart_isr(int irq, void *context)
+{
+	unsigned int bt_wake;
+	struct bcm_device *p = (struct bcm_device *)context;
+
+	bt_wake = gpiod_get_value(p->bt_wake_gpio);
+	BT_DBG("%s with bt_wake of %d (active_low %d), req bh",
+	       __func__, bt_wake, p->bt_wake_active_low);
+
+	/* Defer the actual processing to the platform work queue */
+	schedule_work(&p->wakeup_work);
+	return IRQ_HANDLED;
+}
+
+/*
+ * Device instance startup
+ */
+static int bcm_bt_uart_probe(struct platform_device *pdev)
+{
+	int ret = 0;
+	struct device_node *np = pdev->dev.of_node;
+	const char *tty_name;
+	struct bcm_device *p_bcm_device = NULL;
+
+	p_bcm_device = devm_kzalloc(&pdev->dev, sizeof(*p_bcm_device),
+		GFP_KERNEL);
+	if (!p_bcm_device) {
+		BT_DBG("%s - failing due to no memory", __func__);
+		return -ENOMEM;
+	}
+	p_bcm_device->pdev = pdev;
+	BT_DBG("%s %p context", __func__, p_bcm_device);
+
+	/* Get dev wake GPIO */
+	p_bcm_device->dev_wake_gpio = gpiod_get(&pdev->dev, "bt-wake");
+	BT_DBG("%s - gpiod_get for bt-wake returned %p",
+	       __func__, p_bcm_device->dev_wake_gpio);
+	if (IS_ERR(p_bcm_device->dev_wake_gpio)) {
+		ret = PTR_ERR(p_bcm_device->dev_wake_gpio);
+		if (ret != -ENOENT) {
+			dev_err(&pdev->dev,
+				"%s - dev_wake GPIO: %d\n", __func__, ret);
+		}
+		p_bcm_device->dev_wake_gpio = NULL;
+	} else {
+		int gpio_value;
+
+		p_bcm_device->dev_wake_active_low = gpiod_is_active_low
+			(p_bcm_device->dev_wake_gpio);
+		BT_DBG("%s - dev_wake a-low is %d (cans %d)",
+		       __func__, p_bcm_device->dev_wake_active_low,
+		       gpiod_cansleep(p_bcm_device->dev_wake_gpio));
+
+		/* configure dev_wake as output with init resumed state */
+		gpio_value = !p_bcm_device->dev_wake_active_low;
+		ret = gpiod_direction_output(p_bcm_device->dev_wake_gpio,
+					     gpio_value);
+		if (ret < 0) {
+			dev_err(&pdev->dev,
+				"%s s dev_wake GPIO: %d\n", __func__, ret);
+			gpiod_put(p_bcm_device->dev_wake_gpio);
+			p_bcm_device->dev_wake_gpio = NULL;
+			goto end;
+		} else {
+			BT_DBG("%s - dev_wake set to %d", __func__,
+			       gpio_value);
+		}
+	}
+
+	/* Get power on/off GPIO */
+	p_bcm_device->reg_on_gpio = gpiod_get(&pdev->dev, "bt-reg-on");
+	BT_DBG("%s - gpiod_get for bt-reg-on returned %p", __func__,
+	       p_bcm_device->reg_on_gpio);
+	if (IS_ERR(p_bcm_device->reg_on_gpio)) {
+		ret = PTR_ERR(p_bcm_device->reg_on_gpio);
+		if (ret != -ENOENT) {
+			dev_err(&pdev->dev,
+				"%s - reg_on GPIO: %d\n", __func__, ret);
+		}
+		p_bcm_device->reg_on_gpio = NULL;
+	} else {
+		int poweron_flag;
+
+		p_bcm_device->reg_on_active_low = gpiod_is_active_low
+			(p_bcm_device->reg_on_gpio);
+		BT_DBG("%s - reg_on a-low is %d (cans %d)",
+		       __func__, p_bcm_device->reg_on_active_low,
+		       gpiod_cansleep(p_bcm_device->reg_on_gpio));
+
+		/* configure reg_on as output with init on state */
+		poweron_flag = !p_bcm_device->reg_on_active_low;
+		ret = gpiod_direction_output(p_bcm_device->reg_on_gpio,
+					     poweron_flag);
+		if (ret < 0) {
+			dev_err(&pdev->dev,
+				"%s set reg_on GPIO: %d\n", __func__, ret);
+			gpiod_put(p_bcm_device->reg_on_gpio);
+			p_bcm_device->reg_on_gpio = NULL;
+		} else {
+			BT_DBG("%s - reg_on initially set to %d", __func__,
+			       poweron_flag);
+		}
+	}
+
+	platform_set_drvdata(pdev, p_bcm_device);
+	/* Must be done before interrupt is requested */
+	INIT_WORK(&p_bcm_device->wakeup_work, bcm_bt_wakeup_task);
+
+	/* Get bt host wake GPIO */
+	p_bcm_device->bt_wake_gpio = gpiod_get(&pdev->dev, "bt-host-wake");
+	BT_DBG("%s - gpiod_get for bt-host-wake returned %p", __func__,
+	       p_bcm_device->bt_wake_gpio);
+	if (IS_ERR(p_bcm_device->bt_wake_gpio)) {
+		ret = PTR_ERR(p_bcm_device->bt_wake_gpio);
+		if (ret != -ENOENT) {
+			dev_err(&pdev->dev,
+				"%s - bt_wake GPIO: %d\n", __func__, ret);
+		}
+		p_bcm_device->bt_wake_gpio = NULL;
+	} else {
+		/* configure bt_wake as input */
+		ret = gpiod_direction_input(p_bcm_device->bt_wake_gpio);
+		if (ret < 0) {
+			dev_err(&pdev->dev,
+				"%s set bt_wake GPIO: %d\n", __func__, ret);
+			gpiod_put(p_bcm_device->bt_wake_gpio);
+			p_bcm_device->bt_wake_gpio = NULL;
+		} else {
+			p_bcm_device->bt_wake_active_low = gpiod_is_active_low
+				(p_bcm_device->bt_wake_gpio);
+			BT_DBG("%s -bt_wake a-low is %d(cans%d)",
+			       __func__, p_bcm_device->bt_wake_active_low,
+			       gpiod_cansleep(p_bcm_device->bt_wake_gpio));
+			p_bcm_device->bt_wake_irq = gpiod_to_irq
+				(p_bcm_device->bt_wake_gpio);
+			if (p_bcm_device->bt_wake_irq < 0) {
+				dev_err(&pdev->dev,
+				"%s - HOST_WAKE IRQ: %d\n", __func__, ret);
+			} else {
+				unsigned long intflags = IRQF_TRIGGER_RISING;
+
+				if (p_bcm_device->bt_wake_active_low)
+					intflags = IRQF_TRIGGER_FALLING;
+
+				ret = request_irq(p_bcm_device->bt_wake_irq,
+						  bcm_bt_uart_isr,
+						  intflags, "bt_host_wake",
+						  p_bcm_device);
+				if (ret < 0) {
+					dev_err(&pdev->dev,
+						"%s - failed IRQ %d: %d",
+						__func__,
+						p_bcm_device->bt_wake_irq, ret);
+				} else {
+					BT_DBG("%s - IRQ %d", __func__,
+					       p_bcm_device->bt_wake_irq);
+				}
+			}
+		}
+	}
+
+	p_bcm_device->configure_sleep = of_property_read_bool(
+		np, "configure-sleep");
+	BT_DBG("configure-sleep read as %d", p_bcm_device->configure_sleep);
+	p_bcm_device->manual_fc = 0;
+	if (!of_property_read_u32(np, "manual-fc",
+				  &p_bcm_device->manual_fc)) {
+		BT_DBG("manual-fc read as %d",
+			p_bcm_device->manual_fc);
+	}
+	p_bcm_device->oper_speed = 3000000;
+	if (!of_property_read_u32(
+		np, "oper-speed",
+		&p_bcm_device->oper_speed)) {
+		BT_DBG("oper-speed read as %d",
+			p_bcm_device->oper_speed);
+	}
+	p_bcm_device->configure_audio = of_property_read_bool(
+		np, "configure-audio");
+	BT_DBG("configure-audio read as %d", p_bcm_device->configure_audio);
+	if (p_bcm_device->configure_audio) {
+		/* Defaults for audio */
+		p_bcm_device->pcm_clockmode = 0;
+		p_bcm_device->pcm_fillmethod = 2;
+		p_bcm_device->pcm_fillnum = 0;
+		p_bcm_device->pcm_fillvalue = 3;
+		p_bcm_device->pcm_incallbitclock = 0;
+		p_bcm_device->pcm_lsbfirst = 0;
+		p_bcm_device->pcm_rightjustify = 0;
+		p_bcm_device->pcm_routing = 0;
+		p_bcm_device->pcm_shortframesync = 0;
+		p_bcm_device->pcm_syncmode = 0;
+
+		if (!of_property_read_u32(np, "pcm-clockmode",
+					  &p_bcm_device->pcm_clockmode))
+			BT_DBG("pcm-clockmode read as %d",
+				p_bcm_device->pcm_clockmode);
+		if (!of_property_read_u32(np, "pcm-fillmethod",
+					  &p_bcm_device->pcm_fillmethod))
+			BT_DBG("pcm-fillmethod readas %d",
+				p_bcm_device->pcm_fillmethod);
+		if (!of_property_read_u32(np, "pcm-fillnum",
+					  &p_bcm_device->pcm_fillnum))
+			BT_DBG("pcm-fillnum read as %d",
+				p_bcm_device->pcm_fillnum);
+		if (!of_property_read_u32(np, "pcm-fillvalue",
+					  &p_bcm_device->pcm_fillvalue))
+			BT_DBG("pcm-fillvalue read as %d",
+				p_bcm_device->pcm_fillvalue);
+		if (!of_property_read_u32(np, "pcm-incallbitclock",
+					  &p_bcm_device->pcm_incallbitclock))
+			BT_DBG("pcm-incallbitclock read as %d",
+				p_bcm_device->pcm_incallbitclock);
+		if (!of_property_read_u32(np, "pcm-lsbfirst",
+					  &p_bcm_device->pcm_lsbfirst))
+			BT_DBG("pcm-lsbfirst read as %d",
+				p_bcm_device->pcm_lsbfirst);
+		if (!of_property_read_u32(np, "pcm-rightjustify",
+					  &p_bcm_device->pcm_rightjustify))
+			BT_DBG("pcm-rightjustify read as %d",
+				p_bcm_device->pcm_rightjustify);
+		if (!of_property_read_u32(np, "pcm-routing",
+					  &p_bcm_device->pcm_routing))
+			BT_DBG("pcm-routing read as %d",
+				p_bcm_device->pcm_routing);
+		if (!of_property_read_u32(np, "pcm-shortframesync",
+					  &p_bcm_device->pcm_shortframesync))
+			BT_DBG("pcm-shortframesync read as %d",
+				p_bcm_device->pcm_shortframesync);
+		if (!of_property_read_u32(np, "pcm-syncmode",
+					  &p_bcm_device->pcm_syncmode))
+			BT_DBG("pcm-syncmode read as %d",
+				p_bcm_device->pcm_syncmode);
+	}
+
+	if (!of_property_read_string(np, "tty", &tty_name)) {
+		strcpy(p_bcm_device->tty_name, tty_name);
+		BT_DBG("tty name read as %s", p_bcm_device->tty_name);
+	}
+
+	BT_DBG("idle_timeout set as %d", idle_timeout);
+
+	ret = 0;  /* If we made it here, we're fine */
+
+	/* Place this instance on the device list */
+	spin_lock(&device_list_lock);
+	list_add_tail(&p_bcm_device->list, &device_list);
+	spin_unlock(&device_list_lock);
+
+end:
+	if (ret) {
+		if (p_bcm_device->reg_on_gpio) {
+			gpiod_put(p_bcm_device->reg_on_gpio);
+			p_bcm_device->reg_on_gpio = NULL;
+		}
+		if (p_bcm_device->bt_wake_gpio) {
+			gpiod_put(p_bcm_device->bt_wake_gpio);
+			p_bcm_device->bt_wake_gpio = NULL;
+		}
+		if (p_bcm_device->dev_wake_gpio) {
+			gpiod_put(p_bcm_device->dev_wake_gpio);
+			p_bcm_device->dev_wake_gpio = NULL;
+		}
+	}
+
+	BT_DBG("%s with the result %d", __func__, ret);
+	return ret;
+}
+
+/*
+ * Device instance removal
+ */
+static int bcm_bt_uart_remove(struct platform_device *pdev)
+{
+	struct bcm_device *p_bcm_device = platform_get_drvdata(pdev);
+
+	if (p_bcm_device == NULL) {
+		BT_DBG("%s - logic error, no probe?!", __func__);
+		return 0;
+	}
+
+	BT_DBG("%s %p context", __func__, p_bcm_device);
+
+	spin_lock(&device_list_lock);
+	list_del(&p_bcm_device->list);
+	spin_unlock(&device_list_lock);
+
+	BT_DBG("%s - freeing interrupt %d", __func__,
+		p_bcm_device->bt_wake_irq);
+	free_irq(p_bcm_device->bt_wake_irq, p_bcm_device);
+
+	if (p_bcm_device->reg_on_gpio) {
+		BT_DBG("%s - releasing reg_on_gpio", __func__);
+		gpiod_put(p_bcm_device->reg_on_gpio);
+		p_bcm_device->reg_on_gpio = NULL;
+	}
+
+	if (p_bcm_device->dev_wake_gpio) {
+		BT_DBG("%s - releasing dev_wake_gpio", __func__);
+		gpiod_put(p_bcm_device->dev_wake_gpio);
+		p_bcm_device->dev_wake_gpio = NULL;
+	}
+
+	if (p_bcm_device->bt_wake_gpio) {
+		BT_DBG("%s - releasing bt_wake_gpio", __func__);
+		gpiod_put(p_bcm_device->bt_wake_gpio);
+		p_bcm_device->bt_wake_gpio = NULL;
+	}
+
+	BT_DBG("%s %p done", __func__, p_bcm_device);
+	return 0;
+}
+
+/*
+ * Platform resume callback
+ */
+static int bcm_bt_uart_resume(struct device *pdev)
+{
+	int gpio_value;
+	struct bcm_device *p_bcm_device = platform_get_drvdata(
+		to_platform_device(pdev));
+
+	if (p_bcm_device == NULL) {
+		BT_DBG("%s - logic error, no device?!", __func__);
+		return 0;
+	}
+
+	BT_DBG("%s %p", __func__, p_bcm_device);
+
+	gpio_value = !p_bcm_device->dev_wake_active_low;
+	if (p_bcm_device->dev_wake_gpio) {
+		gpiod_set_value(p_bcm_device->dev_wake_gpio, gpio_value);
+		BT_DBG("%s - %d written, delaying 15 ms", __func__,
+		       gpio_value);
+		mdelay(15);
+	}
+
+	/* Let the protocol know the platform is resuming */
+	if (p_bcm_device->protocol_callbacks.p_resume)
+		p_bcm_device->protocol_callbacks.p_resume(
+			p_bcm_device->protocol_callbacks.context);
+
+	return 0;
+}
+
+/*
+ * Platform suspend callback
+ */
+static int bcm_bt_uart_suspend(struct device *pdev)
+{
+	int gpio_value;
+	struct bcm_device *p_bcm_device = platform_get_drvdata(
+		to_platform_device(pdev));
+
+	if (p_bcm_device == NULL) {
+		BT_DBG("%s - logic error, no device?!", __func__);
+		return 0;
+	}
+
+	BT_DBG("%s %p", __func__, p_bcm_device);
+
+	/* Let the protocol know the platform is suspending */
+	if (p_bcm_device->protocol_callbacks.p_suspend)
+		p_bcm_device->protocol_callbacks.p_suspend(
+			p_bcm_device->protocol_callbacks.context);
+
+	/* Suspend the device */
+	if (p_bcm_device->dev_wake_gpio) {
+		gpio_value = !!p_bcm_device->dev_wake_active_low;
+		gpiod_set_value(p_bcm_device->dev_wake_gpio, gpio_value);
+		BT_DBG("%s - %d written, delaying 15 ms", __func__,
+			gpio_value);
+		mdelay(15);
+	}
+
+	return 0;
+}
+
+/*
+ * Entry point for calls from the protocol
+ */
+int btbcm_uart_control(int action, void *device_context,
+	void *p_data, unsigned long *p_size)
+{
+	struct btbcm_uart_callbacks *pc;
+	struct btbcm_uart_parameters *pp = p_data; /* for pars action only */
+	int ret = 0;
+	int gpio_value, poweron_flag;
+	struct bcm_device *p_bcm_device = device_context;
+	struct list_head *ptr;
+	bool is_found = false;
+
+	/* Special processing for the callback configuration */
+	if (action == BTBCM_UART_ACTION_CONFIGURE_CALLBACKS) {
+		pc = p_data;
+
+		BT_DBG("%s - configure callbacks", __func__);
+		if (p_data == NULL || *p_size != sizeof(struct
+		    btbcm_uart_callbacks) || (pc->interface_version !=
+		    BTBCM_UART_INTERFACE_VERSION)) {
+			BT_DBG("%s - callbacks mismatch!", __func__);
+			return -E2BIG;
+		}
+
+		BT_DBG("%s - configure callbacks for %s(%p)", __func__,
+		       pc->name, pc->context);
+		if (p_bcm_device == NULL) {
+			spin_lock(&device_list_lock);
+			list_for_each(ptr, &device_list) {
+				p_bcm_device = list_entry(ptr, struct
+							  bcm_device, list);
+				if (!strcmp(p_bcm_device->tty_name, pc->name)) {
+					is_found = true;
+					break;
+				}
+			}
+
+			spin_unlock(&device_list_lock);
+			if (!is_found) {
+				BT_DBG("%s - no device!", __func__);
+				return -ENOENT;
+			}
+		}
+
+		p_bcm_device->protocol_callbacks = *pc;
+		memcpy(p_data, &p_bcm_device, sizeof(p_bcm_device));
+		*p_size = sizeof(p_bcm_device);
+		return ret;
+	}
+
+	/* All other requests must have the right context */
+	if (p_bcm_device == NULL) {
+		BT_DBG("%s - failing, no device", __func__);
+		return -ENOENT;
+	}
+
+	switch (action) {
+	case BTBCM_UART_ACTION_POWER_ON:
+		BT_DBG("%s %p - power on", __func__, device_context);
+		if (p_bcm_device->reg_on_gpio) {
+			poweron_flag = !p_bcm_device->reg_on_active_low;
+			gpiod_set_value(p_bcm_device->reg_on_gpio,
+					poweron_flag);
+			BT_DBG("%s - pwron %d, delay 15 ms", __func__,
+			       poweron_flag);
+			mdelay(15);
+		}
+		break;
+
+	case BTBCM_UART_ACTION_POWER_OFF:
+		BT_DBG("%s %p - power off", __func__, device_context);
+		if (p_bcm_device->reg_on_gpio) {
+			poweron_flag = p_bcm_device->reg_on_active_low;
+			gpiod_set_value(p_bcm_device->reg_on_gpio,
+					poweron_flag);
+			BT_DBG("%s - pwroff %d, delay 15 ms", __func__,
+			       poweron_flag);
+			mdelay(15);
+		}
+		break;
+
+	case BTBCM_UART_ACTION_RESUME:
+		BT_DBG("%s %p - resume", __func__, device_context);
+		if (p_bcm_device->dev_wake_gpio) {
+			gpio_value = !p_bcm_device->dev_wake_active_low;
+			gpiod_set_value(p_bcm_device->dev_wake_gpio,
+					gpio_value);
+			BT_DBG("%s - resume %d, delay 15 ms", __func__,
+			       gpio_value);
+			mdelay(15);
+		}
+		break;
+
+	case BTBCM_UART_ACTION_SUSPEND:
+		BT_DBG("%s %p - suspend", __func__, device_context);
+		if (p_bcm_device->dev_wake_gpio) {
+			gpio_value = !!p_bcm_device->dev_wake_active_low;
+			gpiod_set_value(p_bcm_device->dev_wake_gpio,
+					gpio_value);
+			BT_DBG("btbcm_uart_control - suspend %d, delay 15ms",
+			       gpio_value);
+			mdelay(15);
+		}
+		break;
+
+	case BTBCM_UART_ACTION_GET_PARAMETERS:
+		BT_DBG("%s %p - get pars", __func__, device_context);
+		if ((p_data == NULL) ||
+			(*p_size < sizeof(struct btbcm_uart_parameters))) {
+			BT_DBG("%s - failing, wrong par size", __func__);
+			return -E2BIG;
+		}
+
+		memset(pp, 0, sizeof(struct btbcm_uart_parameters));
+		pp->interface_version = BTBCM_UART_INTERFACE_VERSION;
+		pp->configure_sleep = p_bcm_device->configure_sleep;
+		pp->manual_fc = p_bcm_device->manual_fc;
+		pp->dev_wake_active_low = p_bcm_device->dev_wake_active_low;
+		pp->bt_wake_active_low = p_bcm_device->bt_wake_active_low;
+		pp->idle_timeout_in_secs = idle_timeout;
+		pp->oper_speed = p_bcm_device->oper_speed;
+		pp->configure_audio = p_bcm_device->configure_audio;
+		pp->pcm_clockmode = p_bcm_device->pcm_clockmode;
+		pp->pcm_fillmethod = p_bcm_device->pcm_fillmethod;
+		pp->pcm_fillnum = p_bcm_device->pcm_fillnum;
+		pp->pcm_fillvalue = p_bcm_device->pcm_fillvalue;
+		pp->pcm_incallbitclock = p_bcm_device->pcm_incallbitclock;
+		pp->pcm_lsbfirst = p_bcm_device->pcm_lsbfirst;
+		pp->pcm_rightjustify = p_bcm_device->pcm_rightjustify;
+		pp->pcm_routing = p_bcm_device->pcm_routing;
+		pp->pcm_shortframesync = p_bcm_device->pcm_shortframesync;
+		pp->pcm_syncmode = p_bcm_device->pcm_syncmode;
+		*p_size = sizeof(struct btbcm_uart_parameters);
+		break;
+
+	default:
+		BT_DBG("%s %p unknown act %d", __func__,
+		       device_context, action);
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(btbcm_uart_control);
+
+/* Platform susp and resume callbacks */
+static SIMPLE_DEV_PM_OPS(bcm_bt_uart_pm_ops,
+	bcm_bt_uart_suspend, bcm_bt_uart_resume);
+
+/* Driver match table */
+static const struct of_device_id bcm_bt_uart_table[] = {
+	{ .compatible = "brcm,brcm-bt-uart" },
+	{}
+};
+
+/* Driver configuration */
+static struct platform_driver bcm_bt_uart_driver = {
+	.probe = bcm_bt_uart_probe,
+	.remove = bcm_bt_uart_remove,
+	.driver = {
+		.name = "brcm_bt_uart",
+		.of_match_table = of_match_ptr(bcm_bt_uart_table),
+		.owner = THIS_MODULE,
+		.pm = &bcm_bt_uart_pm_ops,
+	},
+};
+
+module_platform_driver(bcm_bt_uart_driver);
+
+MODULE_AUTHOR("Ilya Faenson");
+MODULE_DESCRIPTION("Broadcom Bluetooth UART Driver");
+MODULE_LICENSE("GPL");
+
diff --git a/drivers/bluetooth/btbcm_uart.h b/drivers/bluetooth/btbcm_uart.h
new file mode 100644
index 0000000..fbc7285
--- /dev/null
+++ b/drivers/bluetooth/btbcm_uart.h
@@ -0,0 +1,89 @@
+/*
+ *  Bluetooth BCM UART Driver Header
+ *
+ *  Copyright (c) 2015 Broadcom Corporation
+ *
+ *  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 BTBCM_UART_H
+#define BTBCM_UART_H
+
+/* Change the version if you change anything in this header */
+#define BTBCM_UART_INTERFACE_VERSION 1
+/* Callbacks from the driver into the protocol */
+typedef void (*p_suspend_callback)(void *context);
+typedef void (*p_resume_callback)(void *context);
+typedef void (*p_wakeup_callback)(void *context);
+struct btbcm_uart_callbacks {
+	int interface_version; /* interface # compiled against */
+	void *context;         /* protocol instance context */
+	char name[64];         /* protocol tty device, for example, ttyS0 */
+
+	/* client callbacks */
+	p_suspend_callback p_suspend;
+	p_resume_callback p_resume;
+	p_wakeup_callback p_wakeup;
+};
+
+/* Driver parameters retrieved from the DT or ACPI */
+struct btbcm_uart_parameters {
+	int interface_version; /* interface # compiled against */
+
+	/* Parameters themselves */
+	bool configure_sleep;
+	int manual_fc;
+	int dev_wake_active_low;
+	int bt_wake_active_low;
+	int idle_timeout_in_secs;
+	int oper_speed;
+	bool configure_audio;
+	int pcm_clockmode;
+	int pcm_fillmethod;
+	int pcm_fillnum;
+	int pcm_fillvalue;
+	int pcm_incallbitclock;
+	int pcm_lsbfirst;
+	int pcm_rightjustify;
+	int pcm_routing;
+	int pcm_shortframesync;
+	int pcm_syncmode;
+};
+
+/*
+ * Actions on the BTBCM_UART driver
+ */
+
+/* Configure protocol callbacks */
+#define BTBCM_UART_ACTION_CONFIGURE_CALLBACKS 0
+
+/* Retrieve BT device parameters */
+#define BTBCM_UART_ACTION_GET_PARAMETERS      1
+
+/* Resume the BT device via GPIO */
+#define BTBCM_UART_ACTION_RESUME              2
+
+/* Suspend the BT device via GPIO */
+#define BTBCM_UART_ACTION_SUSPEND             3
+
+/* Power the BT device off via GPIO */
+#define BTBCM_UART_ACTION_POWER_OFF           4
+
+/* Power the BT device on via GPIO */
+#define BTBCM_UART_ACTION_POWER_ON            5
+
+/* Execute an action on the BT device */
+extern int btbcm_uart_control(int action, void *device_context,
+			      void *p_data, unsigned long *p_size);
+
+#endif
+
-- 
1.9.1

Re: [PATCH v2 3/5] Broadcom Bluetooth UART Platform Driver

[Marcel Holtmann]

Hi Ilya,

> Introduce the device tree enumerated Broadcom Bluetooth UART driver.
> 
> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
> ---
> drivers/bluetooth/Kconfig      |   9 +
> drivers/bluetooth/Makefile     |   1 +
> drivers/bluetooth/btbcm_uart.c | 673 +++++++++++++++++++++++++++++++++++++++++
> drivers/bluetooth/btbcm_uart.h |  89 ++++++
> 4 files changed, 772 insertions(+)
> create mode 100644 drivers/bluetooth/btbcm_uart.c
> create mode 100644 drivers/bluetooth/btbcm_uart.h
> 
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 2e77707..5eee3ed 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -143,6 +143,7 @@ config BT_HCIUART_BCM
> 	bool "Broadcom protocol support"
> 	depends on BT_HCIUART
> 	select BT_HCIUART_H4
> +	select BT_UART_BCM
> 	select BT_BCM
> 	help
> 	  The Broadcom protocol support enables Bluetooth HCI over serial
> @@ -150,6 +151,14 @@ config BT_HCIUART_BCM
> 
> 	  Say Y here to compile support for Broadcom protocol.
> 
> +config BT_UART_BCM

if we follow our new naming that we have pushed since a few years now for new modules, then this should be BT_BCM_UART actually.

> +	tristate "Broadcom BT UART driver"
> +	depends on BT_HCIUART_H4 && TTY

So I am thinking that we do not even make this an user visible option. This should be just automatically selected from BT_HCIUART_BCM and that is it.

Meaning this is will be enough in the end:

	config BT_BCM_UART
		tristate

And I would just put it directly under the BT_BCM entry we already have.

> +	help
> +	  This driver supports the HCI_UART_BT protocol.
> +
> +	  It manages Bluetooth UART device properties and GPIOs.
> +

This is something we have to figure out from a design point of view. Do we want to keep this as a separate module or not. My initial thinking here is that the platform driver should be just registered from bcm_init in hci_bcm.c.

I mean wouldn't it be a lot simpler if we can match up the BT HCI UART bcm_proto driver to the platform driver? I have no objection to make this a separate module, but I want to make sure that we looked at these two options.

If we ever get to UART slave drivers, then this would be essentially the UART slave driver, correct?

> config BT_HCIBCM203X
> 	tristate "HCI BCM203x USB driver"
> 	depends on USB
> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
> index f40e194..e908a88 100644
> --- a/drivers/bluetooth/Makefile
> +++ b/drivers/bluetooth/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_BT_MRVL)		+= btmrvl.o
> obj-$(CONFIG_BT_MRVL_SDIO)	+= btmrvl_sdio.o
> obj-$(CONFIG_BT_WILINK)		+= btwilink.o
> obj-$(CONFIG_BT_BCM)		+= btbcm.o
> +obj-$(CONFIG_BT_UART_BCM)	+= btbcm_uart.o
> obj-$(CONFIG_BT_RTL)		+= btrtl.o
> 
> btmrvl-y			:= btmrvl_main.o
> diff --git a/drivers/bluetooth/btbcm_uart.c b/drivers/bluetooth/btbcm_uart.c
> new file mode 100644
> index 0000000..ccd02a9
> --- /dev/null
> +++ b/drivers/bluetooth/btbcm_uart.c
> @@ -0,0 +1,673 @@
> +/*
> + *  Bluetooth BCM UART Driver
> + *
> + *  Copyright (c) 2015 Broadcom Corporation
> + *
> + *  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/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/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/list.h>
> +
> +#include <net/bluetooth/bluetooth.h>
> +#include <net/bluetooth/hci_core.h>
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_platform.h>
> +
> +#include "btbcm_uart.h"
> +
> +static int idle_timeout = 5;
> +module_param(idle_timeout, int, 0);
> +MODULE_PARM_DESC(idle_timeout, "Bluetooth idle timeout in seconds");

I wonder if this isn't a debugfs option? I have told Marvell that some of their additions might be better served as debugfs option. So for example hooked up under /sys/kernel/debug/bluetooth/hci0/bcm_xx for example.

Not sure if this is easy to do or even if this is something static, then it should be a DT entry.

> +
> +/* Device context */
> +struct bcm_device {
> +	struct list_head list;
> +
> +	struct platform_device *pdev;
> +	struct gpio_desc *bt_wake_gpio;
> +	struct gpio_desc *dev_wake_gpio;
> +	struct gpio_desc *reg_on_gpio;
> +	int bt_wake_irq;
> +	int dev_wake_active_low;
> +	int reg_on_active_low;
> +	int bt_wake_active_low;
> +	bool configure_sleep;
> +	u32 manual_fc;
> +	u32 oper_speed;
> +	bool configure_audio;
> +	u32 pcm_clockmode;
> +	u32 pcm_fillmethod;
> +	u32 pcm_fillnum;
> +	u32 pcm_fillvalue;
> +	u32 pcm_incallbitclock;
> +	u32 pcm_lsbfirst;
> +	u32 pcm_rightjustify;
> +	u32 pcm_routing;
> +	u32 pcm_shortframesync;
> +	u32 pcm_syncmode;
> +
> +	char tty_name[64];
> +
> +	struct btbcm_uart_callbacks protocol_callbacks;
> +	struct work_struct wakeup_work;
> +};
> +
> +/* List of BCM BT UART devices */
> +static DEFINE_SPINLOCK(device_list_lock);
> +static LIST_HEAD(device_list);
> +
> +/*
> + * Calling the BCM protocol at lower execution priority
> + */

Please follow network subsystem coding style rules for comments.

> +static void bcm_bt_wakeup_task(struct work_struct *ws)

Instead of *ws, lets use *work here to be a bit more consistent on how the Bluetooth subsystem labels them.

> +{
> +	int gpio_value;
> +	struct bcm_device *p_bcm_device =
> +		container_of(ws, struct bcm_device, wakeup_work);

As a more generic rule, have the struct with the assignment from the "user data" as the first one.

Also p_bcm_device is something we generally do not do. The p_ for as in pointer is not really our style. So I work short it to just *dev here.

> +
> +	if (!p_bcm_device) {
> +		BT_DBG("%s - failing, no device", __func__);

Lets avoid __func__ and rely on dynamic debug feature.

> +		return;
> +	}
> +
> +	/* Make sure the device is resumed */
> +	gpio_value = !p_bcm_device->dev_wake_active_low;
> +	if (p_bcm_device->dev_wake_gpio) {


The variable declaration of gpio_value and it assignment can go here.

> +		gpiod_set_value(p_bcm_device->dev_wake_gpio, gpio_value);
> +		BT_DBG("%s - resume %d written, delaying 15 ms",
> +		       __func__, gpio_value);
> +		mdelay(15);
> +	}
> +
> +	/* Let the protocol know it's time to wake up */
> +	if (p_bcm_device->protocol_callbacks.p_wakeup)
> +		p_bcm_device->protocol_callbacks.p_wakeup(
> +			p_bcm_device->protocol_callbacks.context);
> +}
> +
> +/*
> + * Interrupt routine for the wake from the device
> + */
> +static irqreturn_t bcm_bt_uart_isr(int irq, void *context)
> +{
> +	unsigned int bt_wake;
> +	struct bcm_device *p = (struct bcm_device *)context;
> +
> +	bt_wake = gpiod_get_value(p->bt_wake_gpio);
> +	BT_DBG("%s with bt_wake of %d (active_low %d), req bh",
> +	       __func__, bt_wake, p->bt_wake_active_low);
> +
> +	/* Defer the actual processing to the platform work queue */
> +	schedule_work(&p->wakeup_work);
> +	return IRQ_HANDLED;
> +}
> +
> +/*
> + * Device instance startup
> + */
> +static int bcm_bt_uart_probe(struct platform_device *pdev)
> +{
> +	int ret = 0;

Please only assign variables to zero only if that is needed and there is no other way.

> +	struct device_node *np = pdev->dev.of_node;
> +	const char *tty_name;
> +	struct bcm_device *p_bcm_device = NULL;

No point to assign to NULL if you assign anyway. Please remove these.

> +
> +	p_bcm_device = devm_kzalloc(&pdev->dev, sizeof(*p_bcm_device),
> +		GFP_KERNEL);

Please use proper alignment when calls break into multiple lines.

> +	if (!p_bcm_device) {
> +		BT_DBG("%s - failing due to no memory", __func__);
> +		return -ENOMEM;
> +	}

There should be an empty line here. Indicated a logical break if you leave the function is a good idea.

> +	p_bcm_device->pdev = pdev;
> +	BT_DBG("%s %p context", __func__, p_bcm_device);
> +
> +	/* Get dev wake GPIO */
> +	p_bcm_device->dev_wake_gpio = gpiod_get(&pdev->dev, "bt-wake");
> +	BT_DBG("%s - gpiod_get for bt-wake returned %p",
> +	       __func__, p_bcm_device->dev_wake_gpio);

I get the feeling that we have a little bit too much BT_DBG. Especially if you print errors later on anyway. Keep in mind that a lot of distribution will have dynamic debug enabled. So going crazy with BT_DBG is not a good idea.

> +	if (IS_ERR(p_bcm_device->dev_wake_gpio)) {
> +		ret = PTR_ERR(p_bcm_device->dev_wake_gpio);
> +		if (ret != -ENOENT) {
> +			dev_err(&pdev->dev,
> +				"%s - dev_wake GPIO: %d\n", __func__, ret);
> +		}

Single calls do not require { }.

> +		p_bcm_device->dev_wake_gpio = NULL;
> +	} else {
> +		int gpio_value;
> +
> +		p_bcm_device->dev_wake_active_low = gpiod_is_active_low
> +			(p_bcm_device->dev_wake_gpio);
> +		BT_DBG("%s - dev_wake a-low is %d (cans %d)",
> +		       __func__, p_bcm_device->dev_wake_active_low,
> +		       gpiod_cansleep(p_bcm_device->dev_wake_gpio));
> +
> +		/* configure dev_wake as output with init resumed state */
> +		gpio_value = !p_bcm_device->dev_wake_active_low;
> +		ret = gpiod_direction_output(p_bcm_device->dev_wake_gpio,
> +					     gpio_value);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev,
> +				"%s s dev_wake GPIO: %d\n", __func__, ret);
> +			gpiod_put(p_bcm_device->dev_wake_gpio);
> +			p_bcm_device->dev_wake_gpio = NULL;
> +			goto end;
> +		} else {
> +			BT_DBG("%s - dev_wake set to %d", __func__,
> +			       gpio_value);
> +		}
> +	}
> +
> +	/* Get power on/off GPIO */
> +	p_bcm_device->reg_on_gpio = gpiod_get(&pdev->dev, "bt-reg-on");
> +	BT_DBG("%s - gpiod_get for bt-reg-on returned %p", __func__,
> +	       p_bcm_device->reg_on_gpio);
> +	if (IS_ERR(p_bcm_device->reg_on_gpio)) {
> +		ret = PTR_ERR(p_bcm_device->reg_on_gpio);
> +		if (ret != -ENOENT) {
> +			dev_err(&pdev->dev,
> +				"%s - reg_on GPIO: %d\n", __func__, ret);
> +		}
> +		p_bcm_device->reg_on_gpio = NULL;
> +	} else {
> +		int poweron_flag;
> +
> +		p_bcm_device->reg_on_active_low = gpiod_is_active_low
> +			(p_bcm_device->reg_on_gpio);
> +		BT_DBG("%s - reg_on a-low is %d (cans %d)",
> +		       __func__, p_bcm_device->reg_on_active_low,
> +		       gpiod_cansleep(p_bcm_device->reg_on_gpio));
> +
> +		/* configure reg_on as output with init on state */
> +		poweron_flag = !p_bcm_device->reg_on_active_low;
> +		ret = gpiod_direction_output(p_bcm_device->reg_on_gpio,
> +					     poweron_flag);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev,
> +				"%s set reg_on GPIO: %d\n", __func__, ret);
> +			gpiod_put(p_bcm_device->reg_on_gpio);
> +			p_bcm_device->reg_on_gpio = NULL;
> +		} else {
> +			BT_DBG("%s - reg_on initially set to %d", __func__,
> +			       poweron_flag);
> +		}
> +	}
> +
> +	platform_set_drvdata(pdev, p_bcm_device);
> +	/* Must be done before interrupt is requested */
> +	INIT_WORK(&p_bcm_device->wakeup_work, bcm_bt_wakeup_task);
> +
> +	/* Get bt host wake GPIO */
> +	p_bcm_device->bt_wake_gpio = gpiod_get(&pdev->dev, "bt-host-wake");
> +	BT_DBG("%s - gpiod_get for bt-host-wake returned %p", __func__,
> +	       p_bcm_device->bt_wake_gpio);
> +	if (IS_ERR(p_bcm_device->bt_wake_gpio)) {
> +		ret = PTR_ERR(p_bcm_device->bt_wake_gpio);
> +		if (ret != -ENOENT) {
> +			dev_err(&pdev->dev,
> +				"%s - bt_wake GPIO: %d\n", __func__, ret);
> +		}
> +		p_bcm_device->bt_wake_gpio = NULL;
> +	} else {
> +		/* configure bt_wake as input */
> +		ret = gpiod_direction_input(p_bcm_device->bt_wake_gpio);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev,
> +				"%s set bt_wake GPIO: %d\n", __func__, ret);
> +			gpiod_put(p_bcm_device->bt_wake_gpio);
> +			p_bcm_device->bt_wake_gpio = NULL;
> +		} else {
> +			p_bcm_device->bt_wake_active_low = gpiod_is_active_low
> +				(p_bcm_device->bt_wake_gpio);
> +			BT_DBG("%s -bt_wake a-low is %d(cans%d)",
> +			       __func__, p_bcm_device->bt_wake_active_low,
> +			       gpiod_cansleep(p_bcm_device->bt_wake_gpio));
> +			p_bcm_device->bt_wake_irq = gpiod_to_irq
> +				(p_bcm_device->bt_wake_gpio);
> +			if (p_bcm_device->bt_wake_irq < 0) {
> +				dev_err(&pdev->dev,
> +				"%s - HOST_WAKE IRQ: %d\n", __func__, ret);

Please use proper alignment. If you run out of space, then consider restructuring this whole code. These many nested if statements are really bad since they make the code hard to understand.

One advice would be to actually leave the function or goto to a later step in case of errors. In this case that saves you one indentation.

Then } else if { is totally valid as well and I think that might save you a second one. Please work on making this readable so that my brain hurts less when trying to understand it ;)

> +			} else {
> +				unsigned long intflags = IRQF_TRIGGER_RISING;
> +
> +				if (p_bcm_device->bt_wake_active_low)
> +					intflags = IRQF_TRIGGER_FALLING;
> +
> +				ret = request_irq(p_bcm_device->bt_wake_irq,
> +						  bcm_bt_uart_isr,
> +						  intflags, "bt_host_wake",
> +						  p_bcm_device);
> +				if (ret < 0) {
> +					dev_err(&pdev->dev,
> +						"%s - failed IRQ %d: %d",
> +						__func__,
> +						p_bcm_device->bt_wake_irq, ret);
> +				} else {
> +					BT_DBG("%s - IRQ %d", __func__,
> +					       p_bcm_device->bt_wake_irq);
> +				}
> +			}
> +		}
> +	}
> +
> +	p_bcm_device->configure_sleep = of_property_read_bool(
> +		np, "configure-sleep");

I do not know what the right indentation is here, but it is not this one. Might want to check how other parts of the network subsystem deal with this.

> +	BT_DBG("configure-sleep read as %d", p_bcm_device->configure_sleep);
> +	p_bcm_device->manual_fc = 0;
> +	if (!of_property_read_u32(np, "manual-fc",
> +				  &p_bcm_device->manual_fc)) {
> +		BT_DBG("manual-fc read as %d",
> +			p_bcm_device->manual_fc);
> +	}
> +	p_bcm_device->oper_speed = 3000000;
> +	if (!of_property_read_u32(
> +		np, "oper-speed",
> +		&p_bcm_device->oper_speed)) {
> +		BT_DBG("oper-speed read as %d",
> +			p_bcm_device->oper_speed);

Here the code and test have the same indentation. That is not acceptable at all.

> +	}
> +	p_bcm_device->configure_audio = of_property_read_bool(
> +		np, "configure-audio");
> +	BT_DBG("configure-audio read as %d", p_bcm_device->configure_audio);
> +	if (p_bcm_device->configure_audio) {
> +		/* Defaults for audio */
> +		p_bcm_device->pcm_clockmode = 0;
> +		p_bcm_device->pcm_fillmethod = 2;
> +		p_bcm_device->pcm_fillnum = 0;
> +		p_bcm_device->pcm_fillvalue = 3;
> +		p_bcm_device->pcm_incallbitclock = 0;
> +		p_bcm_device->pcm_lsbfirst = 0;
> +		p_bcm_device->pcm_rightjustify = 0;
> +		p_bcm_device->pcm_routing = 0;
> +		p_bcm_device->pcm_shortframesync = 0;
> +		p_bcm_device->pcm_syncmode = 0;
> +
> +		if (!of_property_read_u32(np, "pcm-clockmode",
> +					  &p_bcm_device->pcm_clockmode))
> +			BT_DBG("pcm-clockmode read as %d",
> +				p_bcm_device->pcm_clockmode);
> +		if (!of_property_read_u32(np, "pcm-fillmethod",
> +					  &p_bcm_device->pcm_fillmethod))
> +			BT_DBG("pcm-fillmethod readas %d",
> +				p_bcm_device->pcm_fillmethod);
> +		if (!of_property_read_u32(np, "pcm-fillnum",
> +					  &p_bcm_device->pcm_fillnum))
> +			BT_DBG("pcm-fillnum read as %d",
> +				p_bcm_device->pcm_fillnum);
> +		if (!of_property_read_u32(np, "pcm-fillvalue",
> +					  &p_bcm_device->pcm_fillvalue))
> +			BT_DBG("pcm-fillvalue read as %d",
> +				p_bcm_device->pcm_fillvalue);
> +		if (!of_property_read_u32(np, "pcm-incallbitclock",
> +					  &p_bcm_device->pcm_incallbitclock))
> +			BT_DBG("pcm-incallbitclock read as %d",
> +				p_bcm_device->pcm_incallbitclock);
> +		if (!of_property_read_u32(np, "pcm-lsbfirst",
> +					  &p_bcm_device->pcm_lsbfirst))
> +			BT_DBG("pcm-lsbfirst read as %d",
> +				p_bcm_device->pcm_lsbfirst);
> +		if (!of_property_read_u32(np, "pcm-rightjustify",
> +					  &p_bcm_device->pcm_rightjustify))
> +			BT_DBG("pcm-rightjustify read as %d",
> +				p_bcm_device->pcm_rightjustify);
> +		if (!of_property_read_u32(np, "pcm-routing",
> +					  &p_bcm_device->pcm_routing))
> +			BT_DBG("pcm-routing read as %d",
> +				p_bcm_device->pcm_routing);
> +		if (!of_property_read_u32(np, "pcm-shortframesync",
> +					  &p_bcm_device->pcm_shortframesync))
> +			BT_DBG("pcm-shortframesync read as %d",
> +				p_bcm_device->pcm_shortframesync);
> +		if (!of_property_read_u32(np, "pcm-syncmode",
> +					  &p_bcm_device->pcm_syncmode))
> +			BT_DBG("pcm-syncmode read as %d",
> +				p_bcm_device->pcm_syncmode);

I wonder if the BT_DBG are all needed here. I mean one debug statement on what we are going to use is better. If someone ends up trying to debug this, then they can go and look at their DT.

> +	}
> +
> +	if (!of_property_read_string(np, "tty", &tty_name)) {
> +		strcpy(p_bcm_device->tty_name, tty_name);
> +		BT_DBG("tty name read as %s", p_bcm_device->tty_name);
> +	}
> +
> +	BT_DBG("idle_timeout set as %d", idle_timeout);
> +
> +	ret = 0;  /* If we made it here, we're fine */
> +
> +	/* Place this instance on the device list */
> +	spin_lock(&device_list_lock);
> +	list_add_tail(&p_bcm_device->list, &device_list);
> +	spin_unlock(&device_list_lock);
> +
> +end:
> +	if (ret) {
> +		if (p_bcm_device->reg_on_gpio) {
> +			gpiod_put(p_bcm_device->reg_on_gpio);
> +			p_bcm_device->reg_on_gpio = NULL;
> +		}
> +		if (p_bcm_device->bt_wake_gpio) {
> +			gpiod_put(p_bcm_device->bt_wake_gpio);
> +			p_bcm_device->bt_wake_gpio = NULL;
> +		}
> +		if (p_bcm_device->dev_wake_gpio) {
> +			gpiod_put(p_bcm_device->dev_wake_gpio);
> +			p_bcm_device->dev_wake_gpio = NULL;
> +		}
> +	}
> +
> +	BT_DBG("%s with the result %d", __func__, ret);
> +	return ret;
> +}
> +
> +/*
> + * Device instance removal
> + */
> +static int bcm_bt_uart_remove(struct platform_device *pdev)
> +{
> +	struct bcm_device *p_bcm_device = platform_get_drvdata(pdev);
> +
> +	if (p_bcm_device == NULL) {

General preference is !pointer here.

> +		BT_DBG("%s - logic error, no probe?!", __func__);
> +		return 0;
> +	}
> +
> +	BT_DBG("%s %p context", __func__, p_bcm_device);
> +
> +	spin_lock(&device_list_lock);
> +	list_del(&p_bcm_device->list);
> +	spin_unlock(&device_list_lock);
> +
> +	BT_DBG("%s - freeing interrupt %d", __func__,
> +		p_bcm_device->bt_wake_irq);
> +	free_irq(p_bcm_device->bt_wake_irq, p_bcm_device);
> +
> +	if (p_bcm_device->reg_on_gpio) {
> +		BT_DBG("%s - releasing reg_on_gpio", __func__);
> +		gpiod_put(p_bcm_device->reg_on_gpio);
> +		p_bcm_device->reg_on_gpio = NULL;
> +	}
> +
> +	if (p_bcm_device->dev_wake_gpio) {
> +		BT_DBG("%s - releasing dev_wake_gpio", __func__);
> +		gpiod_put(p_bcm_device->dev_wake_gpio);
> +		p_bcm_device->dev_wake_gpio = NULL;
> +	}
> +
> +	if (p_bcm_device->bt_wake_gpio) {
> +		BT_DBG("%s - releasing bt_wake_gpio", __func__);
> +		gpiod_put(p_bcm_device->bt_wake_gpio);
> +		p_bcm_device->bt_wake_gpio = NULL;
> +	}
> +
> +	BT_DBG("%s %p done", __func__, p_bcm_device);
> +	return 0;
> +}
> +
> +/*
> + * Platform resume callback
> + */
> +static int bcm_bt_uart_resume(struct device *pdev)
> +{
> +	int gpio_value;
> +	struct bcm_device *p_bcm_device = platform_get_drvdata(
> +		to_platform_device(pdev));
> +
> +	if (p_bcm_device == NULL) {
> +		BT_DBG("%s - logic error, no device?!", __func__);
> +		return 0;
> +	}
> +
> +	BT_DBG("%s %p", __func__, p_bcm_device);
> +
> +	gpio_value = !p_bcm_device->dev_wake_active_low;
> +	if (p_bcm_device->dev_wake_gpio) {
> +		gpiod_set_value(p_bcm_device->dev_wake_gpio, gpio_value);
> +		BT_DBG("%s - %d written, delaying 15 ms", __func__,
> +		       gpio_value);
> +		mdelay(15);
> +	}
> +
> +	/* Let the protocol know the platform is resuming */
> +	if (p_bcm_device->protocol_callbacks.p_resume)
> +		p_bcm_device->protocol_callbacks.p_resume(
> +			p_bcm_device->protocol_callbacks.context);
> +
> +	return 0;
> +}
> +
> +/*
> + * Platform suspend callback
> + */
> +static int bcm_bt_uart_suspend(struct device *pdev)
> +{
> +	int gpio_value;
> +	struct bcm_device *p_bcm_device = platform_get_drvdata(
> +		to_platform_device(pdev));
> +
> +	if (p_bcm_device == NULL) {
> +		BT_DBG("%s - logic error, no device?!", __func__);
> +		return 0;
> +	}
> +
> +	BT_DBG("%s %p", __func__, p_bcm_device);
> +
> +	/* Let the protocol know the platform is suspending */
> +	if (p_bcm_device->protocol_callbacks.p_suspend)
> +		p_bcm_device->protocol_callbacks.p_suspend(
> +			p_bcm_device->protocol_callbacks.context);
> +
> +	/* Suspend the device */
> +	if (p_bcm_device->dev_wake_gpio) {
> +		gpio_value = !!p_bcm_device->dev_wake_active_low;
> +		gpiod_set_value(p_bcm_device->dev_wake_gpio, gpio_value);
> +		BT_DBG("%s - %d written, delaying 15 ms", __func__,
> +			gpio_value);
> +		mdelay(15);
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Entry point for calls from the protocol
> + */
> +int btbcm_uart_control(int action, void *device_context,
> +	void *p_data, unsigned long *p_size)
> +{
> +	struct btbcm_uart_callbacks *pc;
> +	struct btbcm_uart_parameters *pp = p_data; /* for pars action only */
> +	int ret = 0;
> +	int gpio_value, poweron_flag;
> +	struct bcm_device *p_bcm_device = device_context;
> +	struct list_head *ptr;
> +	bool is_found = false;
> +
> +	/* Special processing for the callback configuration */
> +	if (action == BTBCM_UART_ACTION_CONFIGURE_CALLBACKS) {
> +		pc = p_data;
> +
> +		BT_DBG("%s - configure callbacks", __func__);
> +		if (p_data == NULL || *p_size != sizeof(struct
> +		    btbcm_uart_callbacks) || (pc->interface_version !=
> +		    BTBCM_UART_INTERFACE_VERSION)) {
> +			BT_DBG("%s - callbacks mismatch!", __func__);
> +			return -E2BIG;
> +		}
> +
> +		BT_DBG("%s - configure callbacks for %s(%p)", __func__,
> +		       pc->name, pc->context);
> +		if (p_bcm_device == NULL) {
> +			spin_lock(&device_list_lock);
> +			list_for_each(ptr, &device_list) {
> +				p_bcm_device = list_entry(ptr, struct
> +							  bcm_device, list);
> +				if (!strcmp(p_bcm_device->tty_name, pc->name)) {
> +					is_found = true;
> +					break;
> +				}
> +			}
> +
> +			spin_unlock(&device_list_lock);
> +			if (!is_found) {
> +				BT_DBG("%s - no device!", __func__);
> +				return -ENOENT;
> +			}
> +		}
> +
> +		p_bcm_device->protocol_callbacks = *pc;
> +		memcpy(p_data, &p_bcm_device, sizeof(p_bcm_device));
> +		*p_size = sizeof(p_bcm_device);
> +		return ret;
> +	}
> +
> +	/* All other requests must have the right context */
> +	if (p_bcm_device == NULL) {
> +		BT_DBG("%s - failing, no device", __func__);
> +		return -ENOENT;
> +	}
> +
> +	switch (action) {
> +	case BTBCM_UART_ACTION_POWER_ON:
> +		BT_DBG("%s %p - power on", __func__, device_context);
> +		if (p_bcm_device->reg_on_gpio) {
> +			poweron_flag = !p_bcm_device->reg_on_active_low;
> +			gpiod_set_value(p_bcm_device->reg_on_gpio,
> +					poweron_flag);
> +			BT_DBG("%s - pwron %d, delay 15 ms", __func__,
> +			       poweron_flag);
> +			mdelay(15);
> +		}
> +		break;
> +
> +	case BTBCM_UART_ACTION_POWER_OFF:
> +		BT_DBG("%s %p - power off", __func__, device_context);
> +		if (p_bcm_device->reg_on_gpio) {
> +			poweron_flag = p_bcm_device->reg_on_active_low;
> +			gpiod_set_value(p_bcm_device->reg_on_gpio,
> +					poweron_flag);
> +			BT_DBG("%s - pwroff %d, delay 15 ms", __func__,
> +			       poweron_flag);
> +			mdelay(15);
> +		}
> +		break;
> +
> +	case BTBCM_UART_ACTION_RESUME:
> +		BT_DBG("%s %p - resume", __func__, device_context);
> +		if (p_bcm_device->dev_wake_gpio) {
> +			gpio_value = !p_bcm_device->dev_wake_active_low;
> +			gpiod_set_value(p_bcm_device->dev_wake_gpio,
> +					gpio_value);
> +			BT_DBG("%s - resume %d, delay 15 ms", __func__,
> +			       gpio_value);
> +			mdelay(15);
> +		}
> +		break;
> +
> +	case BTBCM_UART_ACTION_SUSPEND:
> +		BT_DBG("%s %p - suspend", __func__, device_context);
> +		if (p_bcm_device->dev_wake_gpio) {
> +			gpio_value = !!p_bcm_device->dev_wake_active_low;
> +			gpiod_set_value(p_bcm_device->dev_wake_gpio,
> +					gpio_value);
> +			BT_DBG("btbcm_uart_control - suspend %d, delay 15ms",
> +			       gpio_value);
> +			mdelay(15);
> +		}
> +		break;
> +
> +	case BTBCM_UART_ACTION_GET_PARAMETERS:
> +		BT_DBG("%s %p - get pars", __func__, device_context);
> +		if ((p_data == NULL) ||
> +			(*p_size < sizeof(struct btbcm_uart_parameters))) {
> +			BT_DBG("%s - failing, wrong par size", __func__);
> +			return -E2BIG;
> +		}
> +
> +		memset(pp, 0, sizeof(struct btbcm_uart_parameters));
> +		pp->interface_version = BTBCM_UART_INTERFACE_VERSION;
> +		pp->configure_sleep = p_bcm_device->configure_sleep;
> +		pp->manual_fc = p_bcm_device->manual_fc;
> +		pp->dev_wake_active_low = p_bcm_device->dev_wake_active_low;
> +		pp->bt_wake_active_low = p_bcm_device->bt_wake_active_low;
> +		pp->idle_timeout_in_secs = idle_timeout;
> +		pp->oper_speed = p_bcm_device->oper_speed;
> +		pp->configure_audio = p_bcm_device->configure_audio;
> +		pp->pcm_clockmode = p_bcm_device->pcm_clockmode;
> +		pp->pcm_fillmethod = p_bcm_device->pcm_fillmethod;
> +		pp->pcm_fillnum = p_bcm_device->pcm_fillnum;
> +		pp->pcm_fillvalue = p_bcm_device->pcm_fillvalue;
> +		pp->pcm_incallbitclock = p_bcm_device->pcm_incallbitclock;
> +		pp->pcm_lsbfirst = p_bcm_device->pcm_lsbfirst;
> +		pp->pcm_rightjustify = p_bcm_device->pcm_rightjustify;
> +		pp->pcm_routing = p_bcm_device->pcm_routing;
> +		pp->pcm_shortframesync = p_bcm_device->pcm_shortframesync;
> +		pp->pcm_syncmode = p_bcm_device->pcm_syncmode;
> +		*p_size = sizeof(struct btbcm_uart_parameters);
> +		break;
> +
> +	default:
> +		BT_DBG("%s %p unknown act %d", __func__,
> +		       device_context, action);
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(btbcm_uart_control);

I am not okay with having one bug exported symbol that does everything. This is the kernel and we can export each function individually. No need to have this massive single point of entry. So please split these out.

> +
> +/* Platform susp and resume callbacks */
> +static SIMPLE_DEV_PM_OPS(bcm_bt_uart_pm_ops,

I think bcm_uart_pm_ops is enough.

> +	bcm_bt_uart_suspend, bcm_bt_uart_resume);

Alignment is wrong here.

> +
> +/* Driver match table */
> +static const struct of_device_id bcm_bt_uart_table[] = {

Remove the bt here. I think bcm_uart_table is enough.

> +	{ .compatible = "brcm,brcm-bt-uart" },
> +	{}
> +};
> +
> +/* Driver configuration */
> +static struct platform_driver bcm_bt_uart_driver = {

Just call it bcm_uart_driver.

> +	.probe = bcm_bt_uart_probe,
> +	.remove = bcm_bt_uart_remove,
> +	.driver = {
> +		.name = "brcm_bt_uart",

Matching up with our naming, this should be "btbcm_uart" or "bt_bcm_uart".

> +		.of_match_table = of_match_ptr(bcm_bt_uart_table),
> +		.owner = THIS_MODULE,
> +		.pm = &bcm_bt_uart_pm_ops,
> +	},
> +};

In general it is .name<tab>=<space>value,

> +
> +module_platform_driver(bcm_bt_uart_driver);
> +
> +MODULE_AUTHOR("Ilya Faenson");

Preference is to include the email address in the author information.

> +MODULE_DESCRIPTION("Broadcom Bluetooth UART Driver");
> +MODULE_LICENSE("GPL");
> +
> diff --git a/drivers/bluetooth/btbcm_uart.h b/drivers/bluetooth/btbcm_uart.h
> new file mode 100644
> index 0000000..fbc7285
> --- /dev/null
> +++ b/drivers/bluetooth/btbcm_uart.h
> @@ -0,0 +1,89 @@
> +/*
> + *  Bluetooth BCM UART Driver Header
> + *
> + *  Copyright (c) 2015 Broadcom Corporation
> + *
> + *  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 BTBCM_UART_H
> +#define BTBCM_UART_H

Remove these since we do not do these includes guards for local includes in drivers/bluetooth/.

> +
> +/* Change the version if you change anything in this header */
> +#define BTBCM_UART_INTERFACE_VERSION 1

No idea why we need this. There is no external consumer of this. Please get rid of this.

> +/* Callbacks from the driver into the protocol */
> +typedef void (*p_suspend_callback)(void *context);
> +typedef void (*p_resume_callback)(void *context);
> +typedef void (*p_wakeup_callback)(void *context);

I do not see the need for these typedefs.

> +struct btbcm_uart_callbacks {
> +	int interface_version; /* interface # compiled against */

Get rid of this.

> +	void *context;         /* protocol instance context */
> +	char name[64];         /* protocol tty device, for example, ttyS0 */
> +
> +	/* client callbacks */
> +	p_suspend_callback p_suspend;
> +	p_resume_callback p_resume;
> +	p_wakeup_callback p_wakeup;

Just spell these out here.

> +};
> +
> +/* Driver parameters retrieved from the DT or ACPI */
> +struct btbcm_uart_parameters {
> +	int interface_version; /* interface # compiled against */

Get rid of this as well.

> +
> +	/* Parameters themselves */
> +	bool configure_sleep;
> +	int manual_fc;
> +	int dev_wake_active_low;
> +	int bt_wake_active_low;
> +	int idle_timeout_in_secs;
> +	int oper_speed;
> +	bool configure_audio;
> +	int pcm_clockmode;
> +	int pcm_fillmethod;
> +	int pcm_fillnum;
> +	int pcm_fillvalue;
> +	int pcm_incallbitclock;
> +	int pcm_lsbfirst;
> +	int pcm_rightjustify;
> +	int pcm_routing;
> +	int pcm_shortframesync;
> +	int pcm_syncmode;
> +};
> +
> +/*
> + * Actions on the BTBCM_UART driver
> + */
> +
> +/* Configure protocol callbacks */
> +#define BTBCM_UART_ACTION_CONFIGURE_CALLBACKS 0
> +
> +/* Retrieve BT device parameters */
> +#define BTBCM_UART_ACTION_GET_PARAMETERS      1
> +
> +/* Resume the BT device via GPIO */
> +#define BTBCM_UART_ACTION_RESUME              2
> +
> +/* Suspend the BT device via GPIO */
> +#define BTBCM_UART_ACTION_SUSPEND             3
> +
> +/* Power the BT device off via GPIO */
> +#define BTBCM_UART_ACTION_POWER_OFF           4
> +
> +/* Power the BT device on via GPIO */
> +#define BTBCM_UART_ACTION_POWER_ON            5
> +
> +/* Execute an action on the BT device */
> +extern int btbcm_uart_control(int action, void *device_context,
> +			      void *p_data, unsigned long *p_size);
> +
> +#endif

Regards

Marcel

RE: [PATCH v2 3/5] Broadcom Bluetooth UART Platform Driver

[Ilya Faenson]

Hi Marcel,

Appreciate extensive comments on the very large patch.

-----Original Message-----
From: Marcel Holtmann [mailto:marcel@holtmann.org]=20
Sent: Friday, June 12, 2015 4:46 AM
To: Ilya Faenson
Cc: linux-bluetooth@vger.kernel.org; Arend Van Spriel
Subject: Re: [PATCH v2 3/5] Broadcom Bluetooth UART Platform Driver

Hi Ilya,

> Introduce the device tree enumerated Broadcom Bluetooth UART driver.
>=20
> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
> ---
> drivers/bluetooth/Kconfig      |   9 +
> drivers/bluetooth/Makefile     |   1 +
> drivers/bluetooth/btbcm_uart.c | 673 ++++++++++++++++++++++++++++++++++++=
+++++
> drivers/bluetooth/btbcm_uart.h |  89 ++++++
> 4 files changed, 772 insertions(+)
> create mode 100644 drivers/bluetooth/btbcm_uart.c
> create mode 100644 drivers/bluetooth/btbcm_uart.h
>=20
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 2e77707..5eee3ed 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -143,6 +143,7 @@ config BT_HCIUART_BCM
> 	bool "Broadcom protocol support"
> 	depends on BT_HCIUART
> 	select BT_HCIUART_H4
> +	select BT_UART_BCM
> 	select BT_BCM
> 	help
> 	  The Broadcom protocol support enables Bluetooth HCI over serial
> @@ -150,6 +151,14 @@ config BT_HCIUART_BCM
>=20
> 	  Say Y here to compile support for Broadcom protocol.
>=20
> +config BT_UART_BCM

if we follow our new naming that we have pushed since a few years now for n=
ew modules, then this should be BT_BCM_UART actually.

IF: Okay, will change.

> +	tristate "Broadcom BT UART driver"
> +	depends on BT_HCIUART_H4 && TTY

So I am thinking that we do not even make this an user visible option. This=
 should be just automatically selected from BT_HCIUART_BCM and that is it.

Meaning this is will be enough in the end:

	config BT_BCM_UART
		tristate

And I would just put it directly under the BT_BCM entry we already have.

IF: As per today's Loic's comment, the platform driver should be optional. =
I agree. I think it should be configured separately as well.

> +	help
> +	  This driver supports the HCI_UART_BT protocol.
> +
> +	  It manages Bluetooth UART device properties and GPIOs.
> +

This is something we have to figure out from a design point of view. Do we =
want to keep this as a separate module or not. My initial thinking here is =
that the platform driver should be just registered from bcm_init in hci_bcm=
.c.

I mean wouldn't it be a lot simpler if we can match up the BT HCI UART bcm_=
proto driver to the platform driver? I have no objection to make this a sep=
arate module, but I want to make sure that we looked at these two options.

If we ever get to UART slave drivers, then this would be essentially the UA=
RT slave driver, correct?

IF: The platform and ACPI (in the future) are logically separate drivers. I=
 would not want to see them both within a BlueZ protocol. Sharing a module =
will not make mapping a protocol instance into a driver instance any easier=
. They would still need to exchange the identifying info. They are logicall=
y two completely different entities in my opinion. UART slave driver, to th=
e best of my limited current understanding, will just introduce one more la=
yer into the already fairly complicated picture. The platform and ACPI driv=
ers may still be needed to manage GPIOs and wakeup interrupts. Also, BlueZ =
protocol resides above the tty line discipline while UART slave is below so=
 having them in a single module would be very confusing in my opinion.

> config BT_HCIBCM203X
> 	tristate "HCI BCM203x USB driver"
> 	depends on USB
> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
> index f40e194..e908a88 100644
> --- a/drivers/bluetooth/Makefile
> +++ b/drivers/bluetooth/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_BT_MRVL)		+=3D btmrvl.o
> obj-$(CONFIG_BT_MRVL_SDIO)	+=3D btmrvl_sdio.o
> obj-$(CONFIG_BT_WILINK)		+=3D btwilink.o
> obj-$(CONFIG_BT_BCM)		+=3D btbcm.o
> +obj-$(CONFIG_BT_UART_BCM)	+=3D btbcm_uart.o
> obj-$(CONFIG_BT_RTL)		+=3D btrtl.o
>=20
> btmrvl-y			:=3D btmrvl_main.o
> diff --git a/drivers/bluetooth/btbcm_uart.c b/drivers/bluetooth/btbcm_uar=
t.c
> new file mode 100644
> index 0000000..ccd02a9
> --- /dev/null
> +++ b/drivers/bluetooth/btbcm_uart.c
> @@ -0,0 +1,673 @@
> +/*
> + *  Bluetooth BCM UART Driver
> + *
> + *  Copyright (c) 2015 Broadcom Corporation
> + *
> + *  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/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/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/list.h>
> +
> +#include <net/bluetooth/bluetooth.h>
> +#include <net/bluetooth/hci_core.h>
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_platform.h>
> +
> +#include "btbcm_uart.h"
> +
> +static int idle_timeout =3D 5;
> +module_param(idle_timeout, int, 0);
> +MODULE_PARM_DESC(idle_timeout, "Bluetooth idle timeout in seconds");

I wonder if this isn't a debugfs option? I have told Marvell that some of t=
heir additions might be better served as debugfs option. So for example hoo=
ked up under /sys/kernel/debug/bluetooth/hci0/bcm_xx for example.

Not sure if this is easy to do or even if this is something static, then it=
 should be a DT entry.

IF: I've actually had it as the DT parameter initially but the application =
engineer for a customer wanted it easily changeable at runtime. I think it =
would be an overkill to roll out the debugfs support for this single parame=
ter. Also, what about the platforms (like phones) where debugfs is not avai=
lable at all?

> +
> +/* Device context */
> +struct bcm_device {
> +	struct list_head list;
> +
> +	struct platform_device *pdev;
> +	struct gpio_desc *bt_wake_gpio;
> +	struct gpio_desc *dev_wake_gpio;
> +	struct gpio_desc *reg_on_gpio;
> +	int bt_wake_irq;
> +	int dev_wake_active_low;
> +	int reg_on_active_low;
> +	int bt_wake_active_low;
> +	bool configure_sleep;
> +	u32 manual_fc;
> +	u32 oper_speed;
> +	bool configure_audio;
> +	u32 pcm_clockmode;
> +	u32 pcm_fillmethod;
> +	u32 pcm_fillnum;
> +	u32 pcm_fillvalue;
> +	u32 pcm_incallbitclock;
> +	u32 pcm_lsbfirst;
> +	u32 pcm_rightjustify;
> +	u32 pcm_routing;
> +	u32 pcm_shortframesync;
> +	u32 pcm_syncmode;
> +
> +	char tty_name[64];
> +
> +	struct btbcm_uart_callbacks protocol_callbacks;
> +	struct work_struct wakeup_work;
> +};
> +
> +/* List of BCM BT UART devices */
> +static DEFINE_SPINLOCK(device_list_lock);
> +static LIST_HEAD(device_list);
> +
> +/*
> + * Calling the BCM protocol at lower execution priority
> + */

Please follow network subsystem coding style rules for comments.

IF: Okay.

> +static void bcm_bt_wakeup_task(struct work_struct *ws)

Instead of *ws, lets use *work here to be a bit more consistent on how the =
Bluetooth subsystem labels them.

IF: Okay.

> +{
> +	int gpio_value;
> +	struct bcm_device *p_bcm_device =3D
> +		container_of(ws, struct bcm_device, wakeup_work);

As a more generic rule, have the struct with the assignment from the "user =
data" as the first one.

IF: Okay.

Also p_bcm_device is something we generally do not do. The p_ for as in poi=
nter is not really our style. So I work short it to just *dev here.

IF: Okay.

> +
> +	if (!p_bcm_device) {
> +		BT_DBG("%s - failing, no device", __func__);

Lets avoid __func__ and rely on dynamic debug feature.

IF: Okay.

> +		return;
> +	}
> +
> +	/* Make sure the device is resumed */
> +	gpio_value =3D !p_bcm_device->dev_wake_active_low;
> +	if (p_bcm_device->dev_wake_gpio) {


The variable declaration of gpio_value and it assignment can go here.

IF: Okay.

> +		gpiod_set_value(p_bcm_device->dev_wake_gpio, gpio_value);
> +		BT_DBG("%s - resume %d written, delaying 15 ms",
> +		       __func__, gpio_value);
> +		mdelay(15);
> +	}
> +
> +	/* Let the protocol know it's time to wake up */
> +	if (p_bcm_device->protocol_callbacks.p_wakeup)
> +		p_bcm_device->protocol_callbacks.p_wakeup(
> +			p_bcm_device->protocol_callbacks.context);
> +}
> +
> +/*
> + * Interrupt routine for the wake from the device
> + */
> +static irqreturn_t bcm_bt_uart_isr(int irq, void *context)
> +{
> +	unsigned int bt_wake;
> +	struct bcm_device *p =3D (struct bcm_device *)context;
> +
> +	bt_wake =3D gpiod_get_value(p->bt_wake_gpio);
> +	BT_DBG("%s with bt_wake of %d (active_low %d), req bh",
> +	       __func__, bt_wake, p->bt_wake_active_low);
> +
> +	/* Defer the actual processing to the platform work queue */
> +	schedule_work(&p->wakeup_work);
> +	return IRQ_HANDLED;
> +}
> +
> +/*
> + * Device instance startup
> + */
> +static int bcm_bt_uart_probe(struct platform_device *pdev)
> +{
> +	int ret =3D 0;

Please only assign variables to zero only if that is needed and there is no=
 other way.

IF: Okay.

> +	struct device_node *np =3D pdev->dev.of_node;
> +	const char *tty_name;
> +	struct bcm_device *p_bcm_device =3D NULL;

No point to assign to NULL if you assign anyway. Please remove these.

IF: Okay.

> +
> +	p_bcm_device =3D devm_kzalloc(&pdev->dev, sizeof(*p_bcm_device),
> +		GFP_KERNEL);

Please use proper alignment when calls break into multiple lines.

IF: Okay.

> +	if (!p_bcm_device) {
> +		BT_DBG("%s - failing due to no memory", __func__);
> +		return -ENOMEM;
> +	}

There should be an empty line here. Indicated a logical break if you leave =
the function is a good idea.

IF: Okay.

> +	p_bcm_device->pdev =3D pdev;
> +	BT_DBG("%s %p context", __func__, p_bcm_device);
> +
> +	/* Get dev wake GPIO */
> +	p_bcm_device->dev_wake_gpio =3D gpiod_get(&pdev->dev, "bt-wake");
> +	BT_DBG("%s - gpiod_get for bt-wake returned %p",
> +	       __func__, p_bcm_device->dev_wake_gpio);

I get the feeling that we have a little bit too much BT_DBG. Especially if =
you print errors later on anyway. Keep in mind that a lot of distribution w=
ill have dynamic debug enabled. So going crazy with BT_DBG is not a good id=
ea.

IF: Will try reducing debug spew some.

> +	if (IS_ERR(p_bcm_device->dev_wake_gpio)) {
> +		ret =3D PTR_ERR(p_bcm_device->dev_wake_gpio);
> +		if (ret !=3D -ENOENT) {
> +			dev_err(&pdev->dev,
> +				"%s - dev_wake GPIO: %d\n", __func__, ret);
> +		}

Single calls do not require { }.

IF: Okay, will change.

> +		p_bcm_device->dev_wake_gpio =3D NULL;
> +	} else {
> +		int gpio_value;
> +
> +		p_bcm_device->dev_wake_active_low =3D gpiod_is_active_low
> +			(p_bcm_device->dev_wake_gpio);
> +		BT_DBG("%s - dev_wake a-low is %d (cans %d)",
> +		       __func__, p_bcm_device->dev_wake_active_low,
> +		       gpiod_cansleep(p_bcm_device->dev_wake_gpio));
> +
> +		/* configure dev_wake as output with init resumed state */
> +		gpio_value =3D !p_bcm_device->dev_wake_active_low;
> +		ret =3D gpiod_direction_output(p_bcm_device->dev_wake_gpio,
> +					     gpio_value);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev,
> +				"%s s dev_wake GPIO: %d\n", __func__, ret);
> +			gpiod_put(p_bcm_device->dev_wake_gpio);
> +			p_bcm_device->dev_wake_gpio =3D NULL;
> +			goto end;
> +		} else {
> +			BT_DBG("%s - dev_wake set to %d", __func__,
> +			       gpio_value);
> +		}
> +	}
> +
> +	/* Get power on/off GPIO */
> +	p_bcm_device->reg_on_gpio =3D gpiod_get(&pdev->dev, "bt-reg-on");
> +	BT_DBG("%s - gpiod_get for bt-reg-on returned %p", __func__,
> +	       p_bcm_device->reg_on_gpio);
> +	if (IS_ERR(p_bcm_device->reg_on_gpio)) {
> +		ret =3D PTR_ERR(p_bcm_device->reg_on_gpio);
> +		if (ret !=3D -ENOENT) {
> +			dev_err(&pdev->dev,
> +				"%s - reg_on GPIO: %d\n", __func__, ret);
> +		}
> +		p_bcm_device->reg_on_gpio =3D NULL;
> +	} else {
> +		int poweron_flag;
> +
> +		p_bcm_device->reg_on_active_low =3D gpiod_is_active_low
> +			(p_bcm_device->reg_on_gpio);
> +		BT_DBG("%s - reg_on a-low is %d (cans %d)",
> +		       __func__, p_bcm_device->reg_on_active_low,
> +		       gpiod_cansleep(p_bcm_device->reg_on_gpio));
> +
> +		/* configure reg_on as output with init on state */
> +		poweron_flag =3D !p_bcm_device->reg_on_active_low;
> +		ret =3D gpiod_direction_output(p_bcm_device->reg_on_gpio,
> +					     poweron_flag);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev,
> +				"%s set reg_on GPIO: %d\n", __func__, ret);
> +			gpiod_put(p_bcm_device->reg_on_gpio);
> +			p_bcm_device->reg_on_gpio =3D NULL;
> +		} else {
> +			BT_DBG("%s - reg_on initially set to %d", __func__,
> +			       poweron_flag);
> +		}
> +	}
> +
> +	platform_set_drvdata(pdev, p_bcm_device);
> +	/* Must be done before interrupt is requested */
> +	INIT_WORK(&p_bcm_device->wakeup_work, bcm_bt_wakeup_task);
> +
> +	/* Get bt host wake GPIO */
> +	p_bcm_device->bt_wake_gpio =3D gpiod_get(&pdev->dev, "bt-host-wake");
> +	BT_DBG("%s - gpiod_get for bt-host-wake returned %p", __func__,
> +	       p_bcm_device->bt_wake_gpio);
> +	if (IS_ERR(p_bcm_device->bt_wake_gpio)) {
> +		ret =3D PTR_ERR(p_bcm_device->bt_wake_gpio);
> +		if (ret !=3D -ENOENT) {
> +			dev_err(&pdev->dev,
> +				"%s - bt_wake GPIO: %d\n", __func__, ret);
> +		}
> +		p_bcm_device->bt_wake_gpio =3D NULL;
> +	} else {
> +		/* configure bt_wake as input */
> +		ret =3D gpiod_direction_input(p_bcm_device->bt_wake_gpio);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev,
> +				"%s set bt_wake GPIO: %d\n", __func__, ret);
> +			gpiod_put(p_bcm_device->bt_wake_gpio);
> +			p_bcm_device->bt_wake_gpio =3D NULL;
> +		} else {
> +			p_bcm_device->bt_wake_active_low =3D gpiod_is_active_low
> +				(p_bcm_device->bt_wake_gpio);
> +			BT_DBG("%s -bt_wake a-low is %d(cans%d)",
> +			       __func__, p_bcm_device->bt_wake_active_low,
> +			       gpiod_cansleep(p_bcm_device->bt_wake_gpio));
> +			p_bcm_device->bt_wake_irq =3D gpiod_to_irq
> +				(p_bcm_device->bt_wake_gpio);
> +			if (p_bcm_device->bt_wake_irq < 0) {
> +				dev_err(&pdev->dev,
> +				"%s - HOST_WAKE IRQ: %d\n", __func__, ret);

Please use proper alignment. If you run out of space, then consider restruc=
turing this whole code. These many nested if statements are really bad sinc=
e they make the code hard to understand.

One advice would be to actually leave the function or goto to a later step =
in case of errors. In this case that saves you one indentation.

Then } else if { is totally valid as well and I think that might save you a=
 second one. Please work on making this readable so that my brain hurts les=
s when trying to understand it ;)

IF: Will try.

> +			} else {
> +				unsigned long intflags =3D IRQF_TRIGGER_RISING;
> +
> +				if (p_bcm_device->bt_wake_active_low)
> +					intflags =3D IRQF_TRIGGER_FALLING;
> +
> +				ret =3D request_irq(p_bcm_device->bt_wake_irq,
> +						  bcm_bt_uart_isr,
> +						  intflags, "bt_host_wake",
> +						  p_bcm_device);
> +				if (ret < 0) {
> +					dev_err(&pdev->dev,
> +						"%s - failed IRQ %d: %d",
> +						__func__,
> +						p_bcm_device->bt_wake_irq, ret);
> +				} else {
> +					BT_DBG("%s - IRQ %d", __func__,
> +					       p_bcm_device->bt_wake_irq);
> +				}
> +			}
> +		}
> +	}
> +
> +	p_bcm_device->configure_sleep =3D of_property_read_bool(
> +		np, "configure-sleep");

I do not know what the right indentation is here, but it is not this one. M=
ight want to check how other parts of the network subsystem deal with this.

IF: Will change.

> +	BT_DBG("configure-sleep read as %d", p_bcm_device->configure_sleep);
> +	p_bcm_device->manual_fc =3D 0;
> +	if (!of_property_read_u32(np, "manual-fc",
> +				  &p_bcm_device->manual_fc)) {
> +		BT_DBG("manual-fc read as %d",
> +			p_bcm_device->manual_fc);
> +	}
> +	p_bcm_device->oper_speed =3D 3000000;
> +	if (!of_property_read_u32(
> +		np, "oper-speed",
> +		&p_bcm_device->oper_speed)) {
> +		BT_DBG("oper-speed read as %d",
> +			p_bcm_device->oper_speed);

Here the code and test have the same indentation. That is not acceptable at=
 all.

IF: Will change.

> +	}
> +	p_bcm_device->configure_audio =3D of_property_read_bool(
> +		np, "configure-audio");
> +	BT_DBG("configure-audio read as %d", p_bcm_device->configure_audio);
> +	if (p_bcm_device->configure_audio) {
> +		/* Defaults for audio */
> +		p_bcm_device->pcm_clockmode =3D 0;
> +		p_bcm_device->pcm_fillmethod =3D 2;
> +		p_bcm_device->pcm_fillnum =3D 0;
> +		p_bcm_device->pcm_fillvalue =3D 3;
> +		p_bcm_device->pcm_incallbitclock =3D 0;
> +		p_bcm_device->pcm_lsbfirst =3D 0;
> +		p_bcm_device->pcm_rightjustify =3D 0;
> +		p_bcm_device->pcm_routing =3D 0;
> +		p_bcm_device->pcm_shortframesync =3D 0;
> +		p_bcm_device->pcm_syncmode =3D 0;
> +
> +		if (!of_property_read_u32(np, "pcm-clockmode",
> +					  &p_bcm_device->pcm_clockmode))
> +			BT_DBG("pcm-clockmode read as %d",
> +				p_bcm_device->pcm_clockmode);
> +		if (!of_property_read_u32(np, "pcm-fillmethod",
> +					  &p_bcm_device->pcm_fillmethod))
> +			BT_DBG("pcm-fillmethod readas %d",
> +				p_bcm_device->pcm_fillmethod);
> +		if (!of_property_read_u32(np, "pcm-fillnum",
> +					  &p_bcm_device->pcm_fillnum))
> +			BT_DBG("pcm-fillnum read as %d",
> +				p_bcm_device->pcm_fillnum);
> +		if (!of_property_read_u32(np, "pcm-fillvalue",
> +					  &p_bcm_device->pcm_fillvalue))
> +			BT_DBG("pcm-fillvalue read as %d",
> +				p_bcm_device->pcm_fillvalue);
> +		if (!of_property_read_u32(np, "pcm-incallbitclock",
> +					  &p_bcm_device->pcm_incallbitclock))
> +			BT_DBG("pcm-incallbitclock read as %d",
> +				p_bcm_device->pcm_incallbitclock);
> +		if (!of_property_read_u32(np, "pcm-lsbfirst",
> +					  &p_bcm_device->pcm_lsbfirst))
> +			BT_DBG("pcm-lsbfirst read as %d",
> +				p_bcm_device->pcm_lsbfirst);
> +		if (!of_property_read_u32(np, "pcm-rightjustify",
> +					  &p_bcm_device->pcm_rightjustify))
> +			BT_DBG("pcm-rightjustify read as %d",
> +				p_bcm_device->pcm_rightjustify);
> +		if (!of_property_read_u32(np, "pcm-routing",
> +					  &p_bcm_device->pcm_routing))
> +			BT_DBG("pcm-routing read as %d",
> +				p_bcm_device->pcm_routing);
> +		if (!of_property_read_u32(np, "pcm-shortframesync",
> +					  &p_bcm_device->pcm_shortframesync))
> +			BT_DBG("pcm-shortframesync read as %d",
> +				p_bcm_device->pcm_shortframesync);
> +		if (!of_property_read_u32(np, "pcm-syncmode",
> +					  &p_bcm_device->pcm_syncmode))
> +			BT_DBG("pcm-syncmode read as %d",
> +				p_bcm_device->pcm_syncmode);

I wonder if the BT_DBG are all needed here. I mean one debug statement on w=
hat we are going to use is better. If someone ends up trying to debug this,=
 then they can go and look at their DT.

IF: will try tracing a few parameters via the one BT_DBG.

> +	}
> +
> +	if (!of_property_read_string(np, "tty", &tty_name)) {
> +		strcpy(p_bcm_device->tty_name, tty_name);
> +		BT_DBG("tty name read as %s", p_bcm_device->tty_name);
> +	}
> +
> +	BT_DBG("idle_timeout set as %d", idle_timeout);
> +
> +	ret =3D 0;  /* If we made it here, we're fine */
> +
> +	/* Place this instance on the device list */
> +	spin_lock(&device_list_lock);
> +	list_add_tail(&p_bcm_device->list, &device_list);
> +	spin_unlock(&device_list_lock);
> +
> +end:
> +	if (ret) {
> +		if (p_bcm_device->reg_on_gpio) {
> +			gpiod_put(p_bcm_device->reg_on_gpio);
> +			p_bcm_device->reg_on_gpio =3D NULL;
> +		}
> +		if (p_bcm_device->bt_wake_gpio) {
> +			gpiod_put(p_bcm_device->bt_wake_gpio);
> +			p_bcm_device->bt_wake_gpio =3D NULL;
> +		}
> +		if (p_bcm_device->dev_wake_gpio) {
> +			gpiod_put(p_bcm_device->dev_wake_gpio);
> +			p_bcm_device->dev_wake_gpio =3D NULL;
> +		}
> +	}
> +
> +	BT_DBG("%s with the result %d", __func__, ret);
> +	return ret;
> +}
> +
> +/*
> + * Device instance removal
> + */
> +static int bcm_bt_uart_remove(struct platform_device *pdev)
> +{
> +	struct bcm_device *p_bcm_device =3D platform_get_drvdata(pdev);
> +
> +	if (p_bcm_device =3D=3D NULL) {

General preference is !pointer here.

IF: Will change.

> +		BT_DBG("%s - logic error, no probe?!", __func__);
> +		return 0;
> +	}
> +
> +	BT_DBG("%s %p context", __func__, p_bcm_device);
> +
> +	spin_lock(&device_list_lock);
> +	list_del(&p_bcm_device->list);
> +	spin_unlock(&device_list_lock);
> +
> +	BT_DBG("%s - freeing interrupt %d", __func__,
> +		p_bcm_device->bt_wake_irq);
> +	free_irq(p_bcm_device->bt_wake_irq, p_bcm_device);
> +
> +	if (p_bcm_device->reg_on_gpio) {
> +		BT_DBG("%s - releasing reg_on_gpio", __func__);
> +		gpiod_put(p_bcm_device->reg_on_gpio);
> +		p_bcm_device->reg_on_gpio =3D NULL;
> +	}
> +
> +	if (p_bcm_device->dev_wake_gpio) {
> +		BT_DBG("%s - releasing dev_wake_gpio", __func__);
> +		gpiod_put(p_bcm_device->dev_wake_gpio);
> +		p_bcm_device->dev_wake_gpio =3D NULL;
> +	}
> +
> +	if (p_bcm_device->bt_wake_gpio) {
> +		BT_DBG("%s - releasing bt_wake_gpio", __func__);
> +		gpiod_put(p_bcm_device->bt_wake_gpio);
> +		p_bcm_device->bt_wake_gpio =3D NULL;
> +	}
> +
> +	BT_DBG("%s %p done", __func__, p_bcm_device);
> +	return 0;
> +}
> +
> +/*
> + * Platform resume callback
> + */
> +static int bcm_bt_uart_resume(struct device *pdev)
> +{
> +	int gpio_value;
> +	struct bcm_device *p_bcm_device =3D platform_get_drvdata(
> +		to_platform_device(pdev));
> +
> +	if (p_bcm_device =3D=3D NULL) {
> +		BT_DBG("%s - logic error, no device?!", __func__);
> +		return 0;
> +	}
> +
> +	BT_DBG("%s %p", __func__, p_bcm_device);
> +
> +	gpio_value =3D !p_bcm_device->dev_wake_active_low;
> +	if (p_bcm_device->dev_wake_gpio) {
> +		gpiod_set_value(p_bcm_device->dev_wake_gpio, gpio_value);
> +		BT_DBG("%s - %d written, delaying 15 ms", __func__,
> +		       gpio_value);
> +		mdelay(15);
> +	}
> +
> +	/* Let the protocol know the platform is resuming */
> +	if (p_bcm_device->protocol_callbacks.p_resume)
> +		p_bcm_device->protocol_callbacks.p_resume(
> +			p_bcm_device->protocol_callbacks.context);
> +
> +	return 0;
> +}
> +
> +/*
> + * Platform suspend callback
> + */
> +static int bcm_bt_uart_suspend(struct device *pdev)
> +{
> +	int gpio_value;
> +	struct bcm_device *p_bcm_device =3D platform_get_drvdata(
> +		to_platform_device(pdev));
> +
> +	if (p_bcm_device =3D=3D NULL) {
> +		BT_DBG("%s - logic error, no device?!", __func__);
> +		return 0;
> +	}
> +
> +	BT_DBG("%s %p", __func__, p_bcm_device);
> +
> +	/* Let the protocol know the platform is suspending */
> +	if (p_bcm_device->protocol_callbacks.p_suspend)
> +		p_bcm_device->protocol_callbacks.p_suspend(
> +			p_bcm_device->protocol_callbacks.context);
> +
> +	/* Suspend the device */
> +	if (p_bcm_device->dev_wake_gpio) {
> +		gpio_value =3D !!p_bcm_device->dev_wake_active_low;
> +		gpiod_set_value(p_bcm_device->dev_wake_gpio, gpio_value);
> +		BT_DBG("%s - %d written, delaying 15 ms", __func__,
> +			gpio_value);
> +		mdelay(15);
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * Entry point for calls from the protocol
> + */
> +int btbcm_uart_control(int action, void *device_context,
> +	void *p_data, unsigned long *p_size)
> +{
> +	struct btbcm_uart_callbacks *pc;
> +	struct btbcm_uart_parameters *pp =3D p_data; /* for pars action only */
> +	int ret =3D 0;
> +	int gpio_value, poweron_flag;
> +	struct bcm_device *p_bcm_device =3D device_context;
> +	struct list_head *ptr;
> +	bool is_found =3D false;
> +
> +	/* Special processing for the callback configuration */
> +	if (action =3D=3D BTBCM_UART_ACTION_CONFIGURE_CALLBACKS) {
> +		pc =3D p_data;
> +
> +		BT_DBG("%s - configure callbacks", __func__);
> +		if (p_data =3D=3D NULL || *p_size !=3D sizeof(struct
> +		    btbcm_uart_callbacks) || (pc->interface_version !=3D
> +		    BTBCM_UART_INTERFACE_VERSION)) {
> +			BT_DBG("%s - callbacks mismatch!", __func__);
> +			return -E2BIG;
> +		}
> +
> +		BT_DBG("%s - configure callbacks for %s(%p)", __func__,
> +		       pc->name, pc->context);
> +		if (p_bcm_device =3D=3D NULL) {
> +			spin_lock(&device_list_lock);
> +			list_for_each(ptr, &device_list) {
> +				p_bcm_device =3D list_entry(ptr, struct
> +							  bcm_device, list);
> +				if (!strcmp(p_bcm_device->tty_name, pc->name)) {
> +					is_found =3D true;
> +					break;
> +				}
> +			}
> +
> +			spin_unlock(&device_list_lock);
> +			if (!is_found) {
> +				BT_DBG("%s - no device!", __func__);
> +				return -ENOENT;
> +			}
> +		}
> +
> +		p_bcm_device->protocol_callbacks =3D *pc;
> +		memcpy(p_data, &p_bcm_device, sizeof(p_bcm_device));
> +		*p_size =3D sizeof(p_bcm_device);
> +		return ret;
> +	}
> +
> +	/* All other requests must have the right context */
> +	if (p_bcm_device =3D=3D NULL) {
> +		BT_DBG("%s - failing, no device", __func__);
> +		return -ENOENT;
> +	}
> +
> +	switch (action) {
> +	case BTBCM_UART_ACTION_POWER_ON:
> +		BT_DBG("%s %p - power on", __func__, device_context);
> +		if (p_bcm_device->reg_on_gpio) {
> +			poweron_flag =3D !p_bcm_device->reg_on_active_low;
> +			gpiod_set_value(p_bcm_device->reg_on_gpio,
> +					poweron_flag);
> +			BT_DBG("%s - pwron %d, delay 15 ms", __func__,
> +			       poweron_flag);
> +			mdelay(15);
> +		}
> +		break;
> +
> +	case BTBCM_UART_ACTION_POWER_OFF:
> +		BT_DBG("%s %p - power off", __func__, device_context);
> +		if (p_bcm_device->reg_on_gpio) {
> +			poweron_flag =3D p_bcm_device->reg_on_active_low;
> +			gpiod_set_value(p_bcm_device->reg_on_gpio,
> +					poweron_flag);
> +			BT_DBG("%s - pwroff %d, delay 15 ms", __func__,
> +			       poweron_flag);
> +			mdelay(15);
> +		}
> +		break;
> +
> +	case BTBCM_UART_ACTION_RESUME:
> +		BT_DBG("%s %p - resume", __func__, device_context);
> +		if (p_bcm_device->dev_wake_gpio) {
> +			gpio_value =3D !p_bcm_device->dev_wake_active_low;
> +			gpiod_set_value(p_bcm_device->dev_wake_gpio,
> +					gpio_value);
> +			BT_DBG("%s - resume %d, delay 15 ms", __func__,
> +			       gpio_value);
> +			mdelay(15);
> +		}
> +		break;
> +
> +	case BTBCM_UART_ACTION_SUSPEND:
> +		BT_DBG("%s %p - suspend", __func__, device_context);
> +		if (p_bcm_device->dev_wake_gpio) {
> +			gpio_value =3D !!p_bcm_device->dev_wake_active_low;
> +			gpiod_set_value(p_bcm_device->dev_wake_gpio,
> +					gpio_value);
> +			BT_DBG("btbcm_uart_control - suspend %d, delay 15ms",
> +			       gpio_value);
> +			mdelay(15);
> +		}
> +		break;
> +
> +	case BTBCM_UART_ACTION_GET_PARAMETERS:
> +		BT_DBG("%s %p - get pars", __func__, device_context);
> +		if ((p_data =3D=3D NULL) ||
> +			(*p_size < sizeof(struct btbcm_uart_parameters))) {
> +			BT_DBG("%s - failing, wrong par size", __func__);
> +			return -E2BIG;
> +		}
> +
> +		memset(pp, 0, sizeof(struct btbcm_uart_parameters));
> +		pp->interface_version =3D BTBCM_UART_INTERFACE_VERSION;
> +		pp->configure_sleep =3D p_bcm_device->configure_sleep;
> +		pp->manual_fc =3D p_bcm_device->manual_fc;
> +		pp->dev_wake_active_low =3D p_bcm_device->dev_wake_active_low;
> +		pp->bt_wake_active_low =3D p_bcm_device->bt_wake_active_low;
> +		pp->idle_timeout_in_secs =3D idle_timeout;
> +		pp->oper_speed =3D p_bcm_device->oper_speed;
> +		pp->configure_audio =3D p_bcm_device->configure_audio;
> +		pp->pcm_clockmode =3D p_bcm_device->pcm_clockmode;
> +		pp->pcm_fillmethod =3D p_bcm_device->pcm_fillmethod;
> +		pp->pcm_fillnum =3D p_bcm_device->pcm_fillnum;
> +		pp->pcm_fillvalue =3D p_bcm_device->pcm_fillvalue;
> +		pp->pcm_incallbitclock =3D p_bcm_device->pcm_incallbitclock;
> +		pp->pcm_lsbfirst =3D p_bcm_device->pcm_lsbfirst;
> +		pp->pcm_rightjustify =3D p_bcm_device->pcm_rightjustify;
> +		pp->pcm_routing =3D p_bcm_device->pcm_routing;
> +		pp->pcm_shortframesync =3D p_bcm_device->pcm_shortframesync;
> +		pp->pcm_syncmode =3D p_bcm_device->pcm_syncmode;
> +		*p_size =3D sizeof(struct btbcm_uart_parameters);
> +		break;
> +
> +	default:
> +		BT_DBG("%s %p unknown act %d", __func__,
> +		       device_context, action);
> +		ret =3D -EINVAL;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(btbcm_uart_control);

I am not okay with having one bug exported symbol that does everything. Thi=
s is the kernel and we can export each function individually. No need to ha=
ve this massive single point of entry. So please split these out.

IF: Okay, will change.

> +
> +/* Platform susp and resume callbacks */
> +static SIMPLE_DEV_PM_OPS(bcm_bt_uart_pm_ops,

I think bcm_uart_pm_ops is enough.

IF: Will change.

> +	bcm_bt_uart_suspend, bcm_bt_uart_resume);

Alignment is wrong here.

IF: Will change.

> +
> +/* Driver match table */
> +static const struct of_device_id bcm_bt_uart_table[] =3D {

Remove the bt here. I think bcm_uart_table is enough.

IF: will do.

> +	{ .compatible =3D "brcm,brcm-bt-uart" },
> +	{}
> +};
> +
> +/* Driver configuration */
> +static struct platform_driver bcm_bt_uart_driver =3D {

Just call it bcm_uart_driver.

IF: Okay.

> +	.probe =3D bcm_bt_uart_probe,
> +	.remove =3D bcm_bt_uart_remove,
> +	.driver =3D {
> +		.name =3D "brcm_bt_uart",

Matching up with our naming, this should be "btbcm_uart" or "bt_bcm_uart".

IF: This should match with the "compatible" string that we must keep with t=
he "brcm" prefix.

> +		.of_match_table =3D of_match_ptr(bcm_bt_uart_table),
> +		.owner =3D THIS_MODULE,
> +		.pm =3D &bcm_bt_uart_pm_ops,
> +	},
> +};

In general it is .name<tab>=3D<space>value,

IF: Okay, will change.

> +
> +module_platform_driver(bcm_bt_uart_driver);
> +
> +MODULE_AUTHOR("Ilya Faenson");

Preference is to include the email address in the author information.

IF: Will do.

> +MODULE_DESCRIPTION("Broadcom Bluetooth UART Driver");
> +MODULE_LICENSE("GPL");
> +
> diff --git a/drivers/bluetooth/btbcm_uart.h b/drivers/bluetooth/btbcm_uar=
t.h
> new file mode 100644
> index 0000000..fbc7285
> --- /dev/null
> +++ b/drivers/bluetooth/btbcm_uart.h
> @@ -0,0 +1,89 @@
> +/*
> + *  Bluetooth BCM UART Driver Header
> + *
> + *  Copyright (c) 2015 Broadcom Corporation
> + *
> + *  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 BTBCM_UART_H
> +#define BTBCM_UART_H

Remove these since we do not do these includes guards for local includes in=
 drivers/bluetooth/.

IF: Okay.

> +
> +/* Change the version if you change anything in this header */
> +#define BTBCM_UART_INTERFACE_VERSION 1

No idea why we need this. There is no external consumer of this. Please get=
 rid of this.

IF: Okay.

> +/* Callbacks from the driver into the protocol */
> +typedef void (*p_suspend_callback)(void *context);
> +typedef void (*p_resume_callback)(void *context);
> +typedef void (*p_wakeup_callback)(void *context);

I do not see the need for these typedefs.

IF: Okay.

> +struct btbcm_uart_callbacks {
> +	int interface_version; /* interface # compiled against */

Get rid of this.

IF: Okay.

> +	void *context;         /* protocol instance context */
> +	char name[64];         /* protocol tty device, for example, ttyS0 */
> +
> +	/* client callbacks */
> +	p_suspend_callback p_suspend;
> +	p_resume_callback p_resume;
> +	p_wakeup_callback p_wakeup;

Just spell these out here.

IF: Okay.

> +};
> +
> +/* Driver parameters retrieved from the DT or ACPI */
> +struct btbcm_uart_parameters {
> +	int interface_version; /* interface # compiled against */

Get rid of this as well.

IF: Okay.

> +
> +	/* Parameters themselves */
> +	bool configure_sleep;
> +	int manual_fc;
> +	int dev_wake_active_low;
> +	int bt_wake_active_low;
> +	int idle_timeout_in_secs;
> +	int oper_speed;
> +	bool configure_audio;
> +	int pcm_clockmode;
> +	int pcm_fillmethod;
> +	int pcm_fillnum;
> +	int pcm_fillvalue;
> +	int pcm_incallbitclock;
> +	int pcm_lsbfirst;
> +	int pcm_rightjustify;
> +	int pcm_routing;
> +	int pcm_shortframesync;
> +	int pcm_syncmode;
> +};
> +
> +/*
> + * Actions on the BTBCM_UART driver
> + */
> +
> +/* Configure protocol callbacks */
> +#define BTBCM_UART_ACTION_CONFIGURE_CALLBACKS 0
> +
> +/* Retrieve BT device parameters */
> +#define BTBCM_UART_ACTION_GET_PARAMETERS      1
> +
> +/* Resume the BT device via GPIO */
> +#define BTBCM_UART_ACTION_RESUME              2
> +
> +/* Suspend the BT device via GPIO */
> +#define BTBCM_UART_ACTION_SUSPEND             3
> +
> +/* Power the BT device off via GPIO */
> +#define BTBCM_UART_ACTION_POWER_OFF           4
> +
> +/* Power the BT device on via GPIO */
> +#define BTBCM_UART_ACTION_POWER_ON            5
> +
> +/* Execute an action on the BT device */
> +extern int btbcm_uart_control(int action, void *device_context,
> +			      void *p_data, unsigned long *p_size);
> +
> +#endif

Regards

Marcel

Re: [PATCH v2 3/5] Broadcom Bluetooth UART Platform Driver

[Marcel Holtmann]

Hi Ilya,

>> Introduce the device tree enumerated Broadcom Bluetooth UART driver.
>> 
>> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
>> ---
>> drivers/bluetooth/Kconfig      |   9 +
>> drivers/bluetooth/Makefile     |   1 +
>> drivers/bluetooth/btbcm_uart.c | 673 +++++++++++++++++++++++++++++++++++++++++
>> drivers/bluetooth/btbcm_uart.h |  89 ++++++
>> 4 files changed, 772 insertions(+)
>> create mode 100644 drivers/bluetooth/btbcm_uart.c
>> create mode 100644 drivers/bluetooth/btbcm_uart.h
>> 
>> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
>> index 2e77707..5eee3ed 100644
>> --- a/drivers/bluetooth/Kconfig
>> +++ b/drivers/bluetooth/Kconfig
>> @@ -143,6 +143,7 @@ config BT_HCIUART_BCM
>> 	bool "Broadcom protocol support"
>> 	depends on BT_HCIUART
>> 	select BT_HCIUART_H4
>> +	select BT_UART_BCM
>> 	select BT_BCM
>> 	help
>> 	  The Broadcom protocol support enables Bluetooth HCI over serial
>> @@ -150,6 +151,14 @@ config BT_HCIUART_BCM
>> 
>> 	  Say Y here to compile support for Broadcom protocol.
>> 
>> +config BT_UART_BCM
> 
> if we follow our new naming that we have pushed since a few years now for new modules, then this should be BT_BCM_UART actually.
> 
> IF: Okay, will change.
> 
>> +	tristate "Broadcom BT UART driver"
>> +	depends on BT_HCIUART_H4 && TTY
> 
> So I am thinking that we do not even make this an user visible option. This should be just automatically selected from BT_HCIUART_BCM and that is it.
> 
> Meaning this is will be enough in the end:
> 
> 	config BT_BCM_UART
> 		tristate
> 
> And I would just put it directly under the BT_BCM entry we already have.
> 
> IF: As per today's Loic's comment, the platform driver should be optional. I agree. I think it should be configured separately as well.

I am fine with that in the long run. However to not over-engineer it from the start, lets do it simple with not exposing the menu item for selection. We can always change that later and give the option. I prefer that we get things merged and then go on refining them. Turning this back into a user selectable option is pretty easy.

If we go this way, then for me I do not have to think about it too much at the moment. Even the side where the Broadcom device is exposed via ACPI will need something and that something is still not there. And I am also expecting that UART slaves will come around eventually and we have to start thinking about some details as well.

>> +	help
>> +	  This driver supports the HCI_UART_BT protocol.
>> +
>> +	  It manages Bluetooth UART device properties and GPIOs.
>> +
> 
> This is something we have to figure out from a design point of view. Do we want to keep this as a separate module or not. My initial thinking here is that the platform driver should be just registered from bcm_init in hci_bcm.c.
> 
> I mean wouldn't it be a lot simpler if we can match up the BT HCI UART bcm_proto driver to the platform driver? I have no objection to make this a separate module, but I want to make sure that we looked at these two options.
> 
> If we ever get to UART slave drivers, then this would be essentially the UART slave driver, correct?
> 
> IF: The platform and ACPI (in the future) are logically separate drivers. I would not want to see them both within a BlueZ protocol. Sharing a module will not make mapping a protocol instance into a driver instance any easier. They would still need to exchange the identifying info. They are logically two completely different entities in my opinion. UART slave driver, to the best of my limited current understanding, will just introduce one more layer into the already fairly complicated picture. The platform and ACPI drivers may still be needed to manage GPIOs and wakeup interrupts. Also, BlueZ protocol resides above the tty line discipline while UART slave is below so having them in a single module would be very confusing in my opinion.

We could do it as a compile time option where either one or both are compiled into the driver. That is how hci_uart driver actually does it today. So there are many options in this area. Lets keep them separated for now. I have no problem with that. We are not set in stone here anyway. This can easily change later.

Regards

Marcel

RE: [PATCH v2 3/5] Broadcom Bluetooth UART Platform Driver

[Ilya Faenson]

Hi Marcel,

-----Original Message-----
From: linux-bluetooth-owner@vger.kernel.org [mailto:linux-bluetooth-owner@v=
ger.kernel.org] On Behalf Of Marcel Holtmann
Sent: Saturday, June 13, 2015 4:04 AM
To: Ilya Faenson
Cc: linux-bluetooth@vger.kernel.org; Arend Van Spriel
Subject: Re: [PATCH v2 3/5] Broadcom Bluetooth UART Platform Driver

Hi Ilya,

>> Introduce the device tree enumerated Broadcom Bluetooth UART driver.
>>=20
>> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
>> ---
>> drivers/bluetooth/Kconfig      |   9 +
>> drivers/bluetooth/Makefile     |   1 +
>> drivers/bluetooth/btbcm_uart.c | 673 +++++++++++++++++++++++++++++++++++=
++++++
>> drivers/bluetooth/btbcm_uart.h |  89 ++++++
>> 4 files changed, 772 insertions(+)
>> create mode 100644 drivers/bluetooth/btbcm_uart.c
>> create mode 100644 drivers/bluetooth/btbcm_uart.h
>>=20
>> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
>> index 2e77707..5eee3ed 100644
>> --- a/drivers/bluetooth/Kconfig
>> +++ b/drivers/bluetooth/Kconfig
>> @@ -143,6 +143,7 @@ config BT_HCIUART_BCM
>> 	bool "Broadcom protocol support"
>> 	depends on BT_HCIUART
>> 	select BT_HCIUART_H4
>> +	select BT_UART_BCM
>> 	select BT_BCM
>> 	help
>> 	  The Broadcom protocol support enables Bluetooth HCI over serial
>> @@ -150,6 +151,14 @@ config BT_HCIUART_BCM
>>=20
>> 	  Say Y here to compile support for Broadcom protocol.
>>=20
>> +config BT_UART_BCM
>=20
> if we follow our new naming that we have pushed since a few years now for=
 new modules, then this should be BT_BCM_UART actually.
>=20
> IF: Okay, will change.
>=20
>> +	tristate "Broadcom BT UART driver"
>> +	depends on BT_HCIUART_H4 && TTY
>=20
> So I am thinking that we do not even make this an user visible option. Th=
is should be just automatically selected from BT_HCIUART_BCM and that is it=
.
>=20
> Meaning this is will be enough in the end:
>=20
> 	config BT_BCM_UART
> 		tristate
>=20
> And I would just put it directly under the BT_BCM entry we already have.
>=20
> IF: As per today's Loic's comment, the platform driver should be optional=
. I agree. I think it should be configured separately as well.

I am fine with that in the long run. However to not over-engineer it from t=
he start, lets do it simple with not exposing the menu item for selection. =
We can always change that later and give the option. I prefer that we get t=
hings merged and then go on refining them. Turning this back into a user se=
lectable option is pretty easy.

If we go this way, then for me I do not have to think about it too much at =
the moment. Even the side where the Broadcom device is exposed via ACPI wil=
l need something and that something is still not there. And I am also expec=
ting that UART slaves will come around eventually and we have to start thin=
king about some details as well.

IF: Okay, I will remove the separate platform device configuration.

>> +	help
>> +	  This driver supports the HCI_UART_BT protocol.
>> +
>> +	  It manages Bluetooth UART device properties and GPIOs.
>> +
>=20
> This is something we have to figure out from a design point of view. Do w=
e want to keep this as a separate module or not. My initial thinking here i=
s that the platform driver should be just registered from bcm_init in hci_b=
cm.c.
>=20
> I mean wouldn't it be a lot simpler if we can match up the BT HCI UART bc=
m_proto driver to the platform driver? I have no objection to make this a s=
eparate module, but I want to make sure that we looked at these two options=
.
>=20
> If we ever get to UART slave drivers, then this would be essentially the =
UART slave driver, correct?
>=20
> IF: The platform and ACPI (in the future) are logically separate drivers.=
 I would not want to see them both within a BlueZ protocol. Sharing a modul=
e will not make mapping a protocol instance into a driver instance any easi=
er. They would still need to exchange the identifying info. They are logica=
lly two completely different entities in my opinion. UART slave driver, to =
the best of my limited current understanding, will just introduce one more =
layer into the already fairly complicated picture. The platform and ACPI dr=
ivers may still be needed to manage GPIOs and wakeup interrupts. Also, Blue=
Z protocol resides above the tty line discipline while UART slave is below =
so having them in a single module would be very confusing in my opinion.

We could do it as a compile time option where either one or both are compil=
ed into the driver. That is how hci_uart driver actually does it today. So =
there are many options in this area. Lets keep them separated for now. I ha=
ve no problem with that. We are not set in stone here anyway. This can easi=
ly change later.

IF: Understood, thanks.

Regards

Marcel

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

[PATCH v2 4/5] Support the BCM4354 Bluetooth UART device

[Ilya Faenson]

The ability to run over the BCM4354 while we are awaiting
the Bluetooth UART firmware map file introduction.

Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
---
 drivers/bluetooth/btbcm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
index 8e2f6b6..1849db2 100644
--- a/drivers/bluetooth/btbcm.c
+++ b/drivers/bluetooth/btbcm.c
@@ -243,6 +243,7 @@ static const struct {
 } bcm_uart_subver_table[] = {
 	{ 0x410e, "BCM43341B0"	},	/* 002.001.014 */
 	{ 0x4406, "BCM4324B3"	},	/* 002.004.006 */
+	{ 0x610c, "BCM4354_003.001.012.0306.0659"}, /* 003.001.012 */
 	{ }
 };
 
@@ -279,6 +280,7 @@ int btbcm_initialize(struct hci_dev *hdev, char *fw_name, size_t len)
 
 	switch ((rev & 0xf000) >> 12) {
 	case 0:
+	case 1:
 	case 3:
 		for (i = 0; bcm_uart_subver_table[i].name; i++) {
 			if (subver == bcm_uart_subver_table[i].subver) {
-- 
1.9.1

Re: [PATCH v2 4/5] Support the BCM4354 Bluetooth UART device

[Marcel Holtmann]

Hi Ilya,

> The ability to run over the BCM4354 while we are awaiting
> the Bluetooth UART firmware map file introduction.
> 
> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
> ---
> drivers/bluetooth/btbcm.c | 2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
> index 8e2f6b6..1849db2 100644
> --- a/drivers/bluetooth/btbcm.c
> +++ b/drivers/bluetooth/btbcm.c
> @@ -243,6 +243,7 @@ static const struct {
> } bcm_uart_subver_table[] = {
> 	{ 0x410e, "BCM43341B0"	},	/* 002.001.014 */
> 	{ 0x4406, "BCM4324B3"	},	/* 002.004.006 */
> +	{ 0x610c, "BCM4354_003.001.012.0306.0659"}, /* 003.001.012 */

Please shorten this to BCM4354 here. I know what you are trying to do, but we fix that different way with a manifest file.

Regards

Marcel

RE: [PATCH v2 4/5] Support the BCM4354 Bluetooth UART device

[Ilya Faenson]

Hi Marcel,

-----Original Message-----
From: Marcel Holtmann [mailto:marcel@holtmann.org]=20
Sent: Friday, June 12, 2015 4:47 AM
To: Ilya Faenson
Cc: linux-bluetooth@vger.kernel.org; Arend Van Spriel
Subject: Re: [PATCH v2 4/5] Support the BCM4354 Bluetooth UART device

Hi Ilya,

> The ability to run over the BCM4354 while we are awaiting
> the Bluetooth UART firmware map file introduction.
>=20
> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
> ---
> drivers/bluetooth/btbcm.c | 2 ++
> 1 file changed, 2 insertions(+)
>=20
> diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
> index 8e2f6b6..1849db2 100644
> --- a/drivers/bluetooth/btbcm.c
> +++ b/drivers/bluetooth/btbcm.c
> @@ -243,6 +243,7 @@ static const struct {
> } bcm_uart_subver_table[] =3D {
> 	{ 0x410e, "BCM43341B0"	},	/* 002.001.014 */
> 	{ 0x4406, "BCM4324B3"	},	/* 002.004.006 */
> +	{ 0x610c, "BCM4354_003.001.012.0306.0659"}, /* 003.001.012 */

Please shorten this to BCM4354 here. I know what you are trying to do, but =
we fix that different way with a manifest file.

IF: Okay, will do.

Regards

Marcel

[PATCH v2 5/5] BlueZ Broadcom UART Protocol

[Ilya Faenson]

Merge Broadcom protocol with the existing implementation, providing
for UART setup, firmware download and power management.

Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
---
 drivers/bluetooth/hci_bcm.c | 400 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 384 insertions(+), 16 deletions(-)

diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index e4d66b6..5ff35b7 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -3,6 +3,7 @@
  *  Bluetooth HCI UART driver for Broadcom devices
  *
  *  Copyright (C) 2015  Intel Corporation
+ *  Copyright (C) 2015  Broadcom Corporation
  *
  *
  *  This program is free software; you can redistribute it and/or modify
@@ -24,6 +25,7 @@
 #include <linux/kernel.h>
 #include <linux/errno.h>
 #include <linux/skbuff.h>
+#include <linux/tty.h>
 #include <linux/firmware.h>
 
 #include <net/bluetooth/bluetooth.h>
@@ -31,12 +33,170 @@
 
 #include "btbcm.h"
 #include "hci_uart.h"
+#include "btbcm_uart.h"
 
 struct bcm_data {
 	struct sk_buff *rx_skb;
 	struct sk_buff_head txq;
+	struct hci_uart *hu;
+
+	bool is_suspended; /* suspend/resume flag */
+
+	struct timer_list timer; /* idle timer */
+
+	struct btbcm_uart_parameters pars; /* device parameters */
+	void *device_context; /* ACPI/DT device context */
 };
 
+/* Suspend/resume synchronization mutex */
+static DEFINE_MUTEX(plock);
+
+/* Forward reference */
+static struct hci_uart_proto bcm_proto;
+
+/*
+ * Callbacks from the BCMBT_UART device
+ */
+
+/*
+ * The platform is suspending. Stop UART activity
+ */
+static void suspend_notification(void *context)
+{
+	struct hci_uart *hu = (struct hci_uart *)context;
+	struct bcm_data *h4 = hu->priv;
+
+	BT_DBG("%s: is_suspended %d", __func__, h4->is_suspended);
+
+	if (!h4->pars.configure_sleep)
+		return;
+
+	if (!h4->is_suspended) {
+		if (h4->pars.manual_fc)
+			hci_uart_set_flow_control(hu, true);
+
+		/* Once this callback returns, driver suspends BT via GPIO */
+		h4->is_suspended = true;
+	}
+}
+
+/*
+ * The platform is resuming. Resume UART activity.
+ */
+static void resume_notification(void *context)
+{
+	struct hci_uart *hu = (struct hci_uart *)context;
+	struct bcm_data *h4 = hu->priv;
+
+	BT_DBG("%s: is_suspended %d", __func__, h4->is_suspended);
+
+	if (!h4->pars.configure_sleep)
+		return;
+
+	/* When this callback executes, the device has woken up already */
+	if (h4->is_suspended) {
+		h4->is_suspended = false;
+
+		if (h4->pars.manual_fc)
+			hci_uart_set_flow_control(hu, false);
+	}
+
+	/* If we're resumed, the idle timer must be running */
+	mod_timer(&h4->timer, jiffies +
+	    msecs_to_jiffies(h4->pars.idle_timeout_in_secs * 1000));
+}
+
+/*
+ * The BT device is resuming. Resume UART activity if suspended
+ */
+static void wakeup_notification(void *context)
+{
+	struct hci_uart *hu = (struct hci_uart *)context;
+	struct bcm_data *h4 = hu->priv;
+
+	BT_DBG("%s: is_suspended %d", __func__, h4->is_suspended);
+
+	if (!h4->pars.configure_sleep)
+		return;
+
+	if (h4->is_suspended) {
+		if (h4->pars.manual_fc)
+			hci_uart_set_flow_control(hu, false);
+
+		h4->is_suspended = false;
+	}
+
+	/* If we're resumed, the idle timer must be running */
+	mod_timer(&h4->timer, jiffies + msecs_to_jiffies(
+			   h4->pars.idle_timeout_in_secs * 1000));
+}
+
+/*
+ * Make sure we're awake
+ * (called when the resumed state is required)
+ */
+static void bcm_ensure_wakeup(struct hci_uart *hu)
+{
+	struct bcm_data *h4 = hu->priv;
+	int status;
+
+	if (!h4->pars.configure_sleep)
+		return;
+
+	/* Suspend/resume operations are serialized */
+	mutex_lock(&plock);
+
+	/* Nothing to do if resumed already */
+	if (!h4->is_suspended) {
+		mutex_unlock(&plock);
+
+		/* Just reset the timer */
+		status = mod_timer(&h4->timer, jiffies + msecs_to_jiffies(
+				   h4->pars.idle_timeout_in_secs * 1000));
+		return;
+	}
+
+	/* Wakeup the device */
+	status = btbcm_uart_control(BTBCM_UART_ACTION_RESUME,
+				    h4->device_context, NULL, NULL);
+	if (status)
+		BT_DBG("%s: failed to resume driver %d", __func__, status);
+
+	/* Unflow control the port if configured */
+	resume_notification(hu);
+
+	mutex_unlock(&plock);
+}
+
+/*
+ * Idle timer callback
+ */
+static void bcm_idle_timeout(unsigned long arg)
+{
+	struct hci_uart *hu = (struct hci_uart *)arg;
+	struct bcm_data *h4 = hu->priv;
+	int status;
+
+	BT_DBG("%s: hu %p", __func__, hu);
+
+	/* Suspend/resume operations are serialized */
+	mutex_lock(&plock);
+
+	if (!h4->is_suspended) {
+		/* Flow control the port if configured */
+		suspend_notification(hu);
+
+		/* Suspend the device */
+		status = btbcm_uart_control(BTBCM_UART_ACTION_SUSPEND,
+					    h4->device_context, NULL, NULL);
+		if (status)
+			BT_DBG("%s: failed to suspend device %d", __func__,
+			       status);
+	}
+
+	mutex_unlock(&plock);
+}
+
 static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)
 {
 	struct hci_dev *hdev = hu->hdev;
@@ -88,31 +248,138 @@ static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)
 
 static int bcm_open(struct hci_uart *hu)
 {
-	struct bcm_data *bcm;
+	struct btbcm_uart_callbacks callbacks;
+	unsigned long callbacks_size = sizeof(callbacks);
+	int status;
+	struct bcm_data *h4;
+	struct tty_struct *tty = hu->tty;
 
-	BT_DBG("hu %p", hu);
+	BT_DBG("bcm_open hu %p", hu);
 
-	bcm = kzalloc(sizeof(*bcm), GFP_KERNEL);
-	if (!bcm)
+	h4 = kzalloc(sizeof(*h4), GFP_KERNEL);
+	if (!h4)
 		return -ENOMEM;
 
-	skb_queue_head_init(&bcm->txq);
+	skb_queue_head_init(&h4->txq);
+	hu->priv = h4;
+	h4->hu = hu;
+	h4->is_suspended = false;
+
+	/* Configure callbacks on the driver */
+	callbacks.interface_version = BTBCM_UART_INTERFACE_VERSION;
+	callbacks.context = hu;
+	strcpy(callbacks.name, tty->name);
+	callbacks.p_suspend = suspend_notification;
+	callbacks.p_resume = resume_notification;
+	callbacks.p_wakeup = wakeup_notification;
+	status = btbcm_uart_control(BTBCM_UART_ACTION_CONFIGURE_CALLBACKS,
+				    NULL, &callbacks, &callbacks_size);
+	if (status) {
+		BT_DBG("bcm_open failed to set driver callbacks %d", status);
+		return status;
+	}
+	if (callbacks_size != sizeof(void *)) {
+		BT_DBG("bcm_open got back %d bytes from callbacks?!",
+		       (int)callbacks_size);
+		return -EMSGSIZE;
+	}
+	memcpy(&h4->device_context, &callbacks, sizeof(void *));
+	BT_DBG("bcm_open callbacks context %p", h4->device_context);
+
+	/* Retrieve device parameters */
+	callbacks_size = sizeof(h4->pars);
+	status = btbcm_uart_control(BTBCM_UART_ACTION_GET_PARAMETERS,
+				    h4->device_context, &h4->pars,
+				    &callbacks_size);
+	if (status) {
+		BT_DBG("bcm_open failed to get dev parameters %d", status);
+		return status;
+	}
+	BT_DBG("Pars ver %d csleep %d dalow %d balow %d idle %d",
+	       h4->pars.interface_version, h4->pars.configure_sleep,
+	       h4->pars.dev_wake_active_low, h4->pars.bt_wake_active_low,
+	       h4->pars.idle_timeout_in_secs);
+	bcm_proto.oper_speed = h4->pars.oper_speed;
+
+	/* Cycle power to make sure the device is in the known state */
+	status = btbcm_uart_control(BTBCM_UART_ACTION_POWER_OFF,
+				    h4->device_context, NULL, NULL);
+	if (status) {
+		BT_DBG("bcm_open failed to power off %d", status);
+	} else {
+		status = btbcm_uart_control(BTBCM_UART_ACTION_POWER_ON,
+					    h4->device_context, NULL, NULL);
+		if (status)
+			BT_DBG("bcm_open failed to power on %d", status);
+	}
+
+	/* Start the idle timer */
+	if (h4->pars.configure_sleep) {
+		setup_timer(&h4->timer, bcm_idle_timeout, (unsigned long)hu);
+		if (h4->pars.configure_sleep)
+			mod_timer(&h4->timer, jiffies + msecs_to_jiffies(
+				  h4->pars.idle_timeout_in_secs * 1000));
+	}
 
-	hu->priv = bcm;
 	return 0;
 }
 
 static int bcm_close(struct hci_uart *hu)
 {
-	struct bcm_data *bcm = hu->priv;
+	struct btbcm_uart_callbacks callbacks;
+	unsigned long callbacks_size = sizeof(callbacks);
+	struct bcm_data *h4 = hu->priv;
+	int status;
 
-	BT_DBG("hu %p", hu);
+	hu->priv = NULL;
 
-	skb_queue_purge(&bcm->txq);
-	kfree_skb(bcm->rx_skb);
-	kfree(bcm);
+	BT_DBG("bcm_close hu %p", hu);
+
+	/* If we're being closed, we must suspend */
+	if (h4->pars.configure_sleep) {
+		mutex_lock(&plock);
+
+		if (!h4->is_suspended) {
+			/* Flow control the port */
+			suspend_notification(hu);
+
+			/* Suspend the device */
+			status = btbcm_uart_control(BTBCM_UART_ACTION_SUSPEND,
+						    h4->device_context, NULL,
+						    NULL);
+			if (status) {
+				BT_DBG("bcm_close suspend driver fail %d",
+				       status);
+			}
+		}
+
+	mutex_unlock(&plock);
+
+	del_timer_sync(&h4->timer);
+	}
+
+	/* Power off the device if possible */
+	status = btbcm_uart_control(BTBCM_UART_ACTION_POWER_OFF,
+				    h4->device_context, NULL, NULL);
+	if (status)
+		BT_DBG("bcm_close failed to power off %d", status);
+
+	/* de-configure callbacks on the driver */
+	callbacks.interface_version = BTBCM_UART_INTERFACE_VERSION;
+	callbacks.context = hu;
+	callbacks.p_suspend = NULL;
+	callbacks.p_resume = NULL;
+	callbacks.p_wakeup = NULL;
+	status = btbcm_uart_control(BTBCM_UART_ACTION_CONFIGURE_CALLBACKS,
+				    h4->device_context, &callbacks,
+				    &callbacks_size);
+	if (status)
+		BT_DBG("bcm_close failed to reset drv callbacks %d", status);
+	skb_queue_purge(&h4->txq);
 
 	hu->priv = NULL;
+	kfree(h4);
+
 	return 0;
 }
 
@@ -129,11 +396,42 @@ static int bcm_flush(struct hci_uart *hu)
 
 static int bcm_setup(struct hci_uart *hu)
 {
-	char fw_name[64];
+	char fw_name[128];
 	const struct firmware *fw;
 	int err;
-
-	BT_DBG("hu %p", hu);
+	struct sk_buff *skb;
+	struct bcm_data *h4 = hu->priv;
+	unsigned char sleep_pars[] = {
+	0x01,       /* sleep mode 1=UART */
+	0x02,       /* idle threshold HOST (value * 300ms) */
+	0x02,       /* idle threshold HC   (value * 300ms) */
+	0x01,       /* BT_WAKE active mode - 1=active high, 0 = active low */
+	0x00,       /* HOST_WAKE active mode - 1=active high, 0 = active low */
+	0x01,       /* Allow host sleep during SCO - FALSE */
+	0x01,       /* combine sleep mode and LPM - 1 == TRUE */
+	0x00,       /* enable tristate control of UART TX line - FALSE */
+	0x00,       /* USB auto-sleep on USB SUSPEND */
+	0x00,       /* USB USB RESUME timeout (seconds) */
+	0x00,       /* Pulsed Host Wake */
+	0x00        /* Enable Break To Host */
+	};
+	unsigned char pcm_int_pars[] = {
+	0x00,       /* 0=PCM routing, 1=SCO over HCI */
+	0x02,       /* 0=128Kbps,1=256Kbps,2=512Kbps,3=1024Kbps,4=2048Kbps */
+	0x00,       /* short frame sync  0=short, 1=long */
+	0x00,       /* sync mode         0=slave, 1=master */
+	0x00        /* clock mode        0=slave, 1=master */
+	};
+	unsigned char pcm_format_pars[] = {
+	0x00,       /* LSB_First 1=TRUE, 0=FALSE */
+	0x00,       /* Fill_Value (use 0-7 for fill bits value) */
+	0x02,       /* Fill_Method   (2=sign extended) */
+	0x03,       /* Fill_Num      # of fill bits 0-3) */
+	0x01        /* Right_Justify 1=TRUE, 0=FALSE */
+	};
+	unsigned char time_slot_number = 0;
+
+	BT_DBG("bcm_setup hu %p", hu);
 
 	hu->hdev->set_bdaddr = btbcm_set_bdaddr;
 
@@ -162,6 +460,67 @@ static int bcm_setup(struct hci_uart *hu)
 			hci_uart_set_baudrate(hu, hu->proto->oper_speed);
 	}
 
+	/* Configure SCO PCM parameters */
+	if (h4->pars.configure_audio) {
+		pcm_int_pars[0] = h4->pars.pcm_routing;
+		pcm_int_pars[1] = h4->pars.pcm_incallbitclock;
+		pcm_int_pars[2] = h4->pars.pcm_shortframesync;
+		pcm_int_pars[3] = h4->pars.pcm_syncmode;
+		pcm_int_pars[4] = h4->pars.pcm_clockmode;
+		skb = __hci_cmd_sync(hu->hdev, 0xfc1c, sizeof(pcm_int_pars),
+				     pcm_int_pars, HCI_INIT_TIMEOUT);
+		if (IS_ERR(skb)) {
+			err = PTR_ERR(skb);
+			BT_ERR("bcm_setup pcm_ INT VSC failed (%d)", err);
+			goto finalize;
+		}
+		kfree_skb(skb);
+		BT_DBG("bcm_setup pcm_ INT Parameters VSC succeeded");
+
+		pcm_format_pars[0] = h4->pars.pcm_lsbfirst;
+		pcm_format_pars[1] = h4->pars.pcm_fillvalue;
+		pcm_format_pars[2] = h4->pars.pcm_fillmethod;
+		pcm_format_pars[3] = h4->pars.pcm_fillnum;
+		pcm_format_pars[4] = h4->pars.pcm_rightjustify;
+		skb = __hci_cmd_sync(hu->hdev, 0xfc1e, sizeof(pcm_format_pars),
+				     pcm_format_pars, HCI_INIT_TIMEOUT);
+		if (IS_ERR(skb)) {
+			err = PTR_ERR(skb);
+			BT_ERR("bcm_setup pcm_ Format VSC failed (%d)",
+			       err);
+			goto finalize;
+		}
+		kfree_skb(skb);
+		BT_DBG("bcm_setup pcm_ Format VSC succeeded");
+
+		skb = __hci_cmd_sync(hu->hdev, 0xfc22, sizeof(time_slot_number),
+				     &time_slot_number, HCI_INIT_TIMEOUT);
+		if (IS_ERR(skb)) {
+			err = PTR_ERR(skb);
+			BT_ERR("bcm_setup SCO Time Slot VSC failed (%d)",
+			       err);
+			goto finalize;
+		}
+		kfree_skb(skb);
+		BT_DBG("bcm_setup SCO Time Slot VSC succeeded");
+	}
+
+	/* Configure device's suspend/resume operation */
+	if (h4->pars.configure_sleep) {
+		/* Override the default */
+		sleep_pars[3] = (unsigned char)!h4->pars.bt_wake_active_low;
+		sleep_pars[4] = (unsigned char)!h4->pars.dev_wake_active_low;
+		skb = __hci_cmd_sync(hu->hdev, 0xfc27, sizeof(sleep_pars),
+				     sleep_pars, HCI_INIT_TIMEOUT);
+		if (IS_ERR(skb)) {
+			err = PTR_ERR(skb);
+			BT_ERR("bcm_setup Sleep VSC failed (%d)", err);
+			goto finalize;
+		}
+		kfree_skb(skb);
+		BT_DBG("bcm_setup Set Sleep Parameters VSC succeeded");
+	}
+
 finalize:
 	release_firmware(fw);
 
@@ -183,6 +542,11 @@ static int bcm_recv(struct hci_uart *hu, const void *data, int count)
 	if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
 		return -EUNATCH;
 
+	BT_DBG("bcm_recv %d bytes", count);
+
+	/* Make sure we're resumed */
+	bcm_ensure_wakeup(hu);
+
 	bcm->rx_skb = h4_recv_buf(hu->hdev, bcm->rx_skb, data, count,
 				  bcm_recv_pkts, ARRAY_SIZE(bcm_recv_pkts));
 	if (IS_ERR(bcm->rx_skb)) {
@@ -198,7 +562,10 @@ static int bcm_enqueue(struct hci_uart *hu, struct sk_buff *skb)
 {
 	struct bcm_data *bcm = hu->priv;
 
-	BT_DBG("hu %p skb %p", hu, skb);
+	BT_DBG("bcm_enqueue hu %p skb %p", hu, skb);
+
+	/* Make sure we're resumed */
+	bcm_ensure_wakeup(hu);
 
 	/* Prepend skb with frame type */
 	memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
@@ -214,7 +581,8 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
 	return skb_dequeue(&bcm->txq);
 }
 
-static const struct hci_uart_proto bcm_proto = {
+/* This structure may not be const as speed may change at runtime */
+static struct hci_uart_proto bcm_proto = {
 	.id		= HCI_UART_BCM,
 	.name		= "BCM",
 	.init_speed	= 115200,
-- 
1.9.1

Re: [PATCH v2 5/5] BlueZ Broadcom UART Protocol

[Marcel Holtmann]

Hi Ilya,

> Merge Broadcom protocol with the existing implementation, providing
> for UART setup, firmware download and power management.
> 
> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
> ---
> drivers/bluetooth/hci_bcm.c | 400 ++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 384 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index e4d66b6..5ff35b7 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -3,6 +3,7 @@
>  *  Bluetooth HCI UART driver for Broadcom devices
>  *
>  *  Copyright (C) 2015  Intel Corporation
> + *  Copyright (C) 2015  Broadcom Corporation
>  *
>  *
>  *  This program is free software; you can redistribute it and/or modify
> @@ -24,6 +25,7 @@
> #include <linux/kernel.h>
> #include <linux/errno.h>
> #include <linux/skbuff.h>
> +#include <linux/tty.h>
> #include <linux/firmware.h>
> 
> #include <net/bluetooth/bluetooth.h>
> @@ -31,12 +33,170 @@
> 
> #include "btbcm.h"
> #include "hci_uart.h"
> +#include "btbcm_uart.h"
> 
> struct bcm_data {
> 	struct sk_buff *rx_skb;
> 	struct sk_buff_head txq;
> +	struct hci_uart *hu;
> +
> +	bool is_suspended; /* suspend/resume flag */
> +
> +	struct timer_list timer; /* idle timer */

I prefer not to use timers if we can use delayed work instead.

> +
> +	struct btbcm_uart_parameters pars; /* device parameters */

btbcm_uart_params and params please. No pars and not point in spelling out parameters in a struct name.

> +	void *device_context; /* ACPI/DT device context */
> };
> 
> +/* Suspend/resume synchronization mutex */
> +static DEFINE_MUTEX(plock);

Please use a more descriptive name than plock. That is too generic for a global variable.

> +
> +/* Forward reference */
> +static struct hci_uart_proto bcm_proto;

Why do we need this? I am generally not in favor of having forward reference until really really needed.

> +
> +/*
> + * Callbacks from the BCMBT_UART device
> + */
> +
> +/*
> + * The platform is suspending. Stop UART activity
> + */
> +static void suspend_notification(void *context)
> +{
> +	struct hci_uart *hu = (struct hci_uart *)context;

context is void * and so no need to cast.

> +	struct bcm_data *h4 = hu->priv;
> +
> +	BT_DBG("%s: is_suspended %d", __func__, h4->is_suspended);
> +
> +	if (!h4->pars.configure_sleep)
> +		return;
> +
> +	if (!h4->is_suspended) {
> +		if (h4->pars.manual_fc)
> +			hci_uart_set_flow_control(hu, true);
> +
> +		/* Once this callback returns, driver suspends BT via GPIO */
> +		h4->is_suspended = true;
> +	}
> +}
> +
> +/*
> + * The platform is resuming. Resume UART activity.
> + */
> +static void resume_notification(void *context)
> +{
> +	struct hci_uart *hu = (struct hci_uart *)context;
> +	struct bcm_data *h4 = hu->priv;
> +
> +	BT_DBG("%s: is_suspended %d", __func__, h4->is_suspended);
> +
> +	if (!h4->pars.configure_sleep)
> +		return;
> +
> +	/* When this callback executes, the device has woken up already */
> +	if (h4->is_suspended) {
> +		h4->is_suspended = false;
> +
> +		if (h4->pars.manual_fc)
> +			hci_uart_set_flow_control(hu, false);
> +	}
> +
> +	/* If we're resumed, the idle timer must be running */
> +	mod_timer(&h4->timer, jiffies +
> +	    msecs_to_jiffies(h4->pars.idle_timeout_in_secs * 1000));

Indention is violated here.

> +}
> +
> +/*
> + * The BT device is resuming. Resume UART activity if suspended
> + */
> +static void wakeup_notification(void *context)
> +{
> +	struct hci_uart *hu = (struct hci_uart *)context;
> +	struct bcm_data *h4 = hu->priv;
> +
> +	BT_DBG("%s: is_suspended %d", __func__, h4->is_suspended);
> +
> +	if (!h4->pars.configure_sleep)
> +		return;
> +
> +	if (h4->is_suspended) {
> +		if (h4->pars.manual_fc)
> +			hci_uart_set_flow_control(hu, false);
> +
> +		h4->is_suspended = false;
> +	}
> +
> +	/* If we're resumed, the idle timer must be running */
> +	mod_timer(&h4->timer, jiffies + msecs_to_jiffies(
> +			   h4->pars.idle_timeout_in_secs * 1000));

Same here actually and worse it is different from the other one.

Splitting the rearming into its own helper might help here.

> +}
> +
> +/*
> + * Make sure we're awake
> + * (called when the resumed state is required)
> + */
> +static void bcm_ensure_wakeup(struct hci_uart *hu)
> +{
> +	struct bcm_data *h4 = hu->priv;
> +	int status;
> +
> +	if (!h4->pars.configure_sleep)
> +		return;
> +
> +	/* Suspend/resume operations are serialized */
> +	mutex_lock(&plock);
> +
> +	/* Nothing to do if resumed already */
> +	if (!h4->is_suspended) {
> +		mutex_unlock(&plock);
> +
> +		/* Just reset the timer */
> +		status = mod_timer(&h4->timer, jiffies + msecs_to_jiffies(
> +				   h4->pars.idle_timeout_in_secs * 1000));

This indentation is still not correct and now we have a 3rd version ;)

> +		return;
> +	}
> +
> +	/* Wakeup the device */
> +	status = btbcm_uart_control(BTBCM_UART_ACTION_RESUME,
> +				    h4->device_context, NULL, NULL);
> +	if (status)
> +		BT_DBG("%s: failed to resume driver %d", __func__, status);

Conditional BT_DBG on the status is pretty useless in my mind. Either BT_DBG the status or don't at all.

> +
> +	/* Unflow control the port if configured */
> +	resume_notification(hu);
> +
> +	mutex_unlock(&plock);
> +}
> +
> +/*
> + * Idle timer callback
> + */
> +static void bcm_idle_timeout(unsigned long arg)
> +{
> +	struct hci_uart *hu = (struct hci_uart *)arg;
> +	struct bcm_data *h4 = hu->priv;
> +	int status;
> +
> +	BT_DBG("%s: hu %p", __func__, hu);
> +
> +	/* Suspend/resume operations are serialized */
> +	mutex_lock(&plock);
> +
> +	if (!h4->is_suspended) {
> +		/* Flow control the port if configured */
> +		suspend_notification(hu);
> +
> +		/* Suspend the device */
> +		status = btbcm_uart_control(BTBCM_UART_ACTION_SUSPEND,
> +					    h4->device_context, NULL, NULL);
> +		if (status)
> +			BT_DBG("%s: failed to suspend device %d", __func__,
> +			       status);
> +	}
> +
> +	mutex_unlock(&plock);
> +}
> +
> static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)
> {
> 	struct hci_dev *hdev = hu->hdev;
> @@ -88,31 +248,138 @@ static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)
> 
> static int bcm_open(struct hci_uart *hu)
> {
> -	struct bcm_data *bcm;
> +	struct btbcm_uart_callbacks callbacks;
> +	unsigned long callbacks_size = sizeof(callbacks);

Not needed since it is all inside the kernel. And I think once this is split into individual exported functions, this will become a lot clearer.

> +	int status;
> +	struct bcm_data *h4;
> +	struct tty_struct *tty = hu->tty;
> 
> -	BT_DBG("hu %p", hu);
> +	BT_DBG("bcm_open hu %p", hu);
> 
> -	bcm = kzalloc(sizeof(*bcm), GFP_KERNEL);
> -	if (!bcm)
> +	h4 = kzalloc(sizeof(*h4), GFP_KERNEL);
> +	if (!h4)
> 		return -ENOMEM;

If we are doing renaming of variables, then I am expecting a cleanup patch first that does only that. If this is needed at all or just some left-over merge issue.

> 
> -	skb_queue_head_init(&bcm->txq);
> +	skb_queue_head_init(&h4->txq);
> +	hu->priv = h4;
> +	h4->hu = hu;
> +	h4->is_suspended = false;
> +
> +	/* Configure callbacks on the driver */
> +	callbacks.interface_version = BTBCM_UART_INTERFACE_VERSION;
> +	callbacks.context = hu;
> +	strcpy(callbacks.name, tty->name);
> +	callbacks.p_suspend = suspend_notification;
> +	callbacks.p_resume = resume_notification;
> +	callbacks.p_wakeup = wakeup_notification;
> +	status = btbcm_uart_control(BTBCM_UART_ACTION_CONFIGURE_CALLBACKS,
> +				    NULL, &callbacks, &callbacks_size);
> +	if (status) {
> +		BT_DBG("bcm_open failed to set driver callbacks %d", status);
> +		return status;
> +	}
> +	if (callbacks_size != sizeof(void *)) {
> +		BT_DBG("bcm_open got back %d bytes from callbacks?!",
> +		       (int)callbacks_size);
> +		return -EMSGSIZE;
> +	}
> +	memcpy(&h4->device_context, &callbacks, sizeof(void *));
> +	BT_DBG("bcm_open callbacks context %p", h4->device_context);
> +
> +	/* Retrieve device parameters */
> +	callbacks_size = sizeof(h4->pars);
> +	status = btbcm_uart_control(BTBCM_UART_ACTION_GET_PARAMETERS,
> +				    h4->device_context, &h4->pars,
> +				    &callbacks_size);
> +	if (status) {
> +		BT_DBG("bcm_open failed to get dev parameters %d", status);
> +		return status;
> +	}
> +	BT_DBG("Pars ver %d csleep %d dalow %d balow %d idle %d",
> +	       h4->pars.interface_version, h4->pars.configure_sleep,
> +	       h4->pars.dev_wake_active_low, h4->pars.bt_wake_active_low,
> +	       h4->pars.idle_timeout_in_secs);
> +	bcm_proto.oper_speed = h4->pars.oper_speed;
> +
> +	/* Cycle power to make sure the device is in the known state */
> +	status = btbcm_uart_control(BTBCM_UART_ACTION_POWER_OFF,
> +				    h4->device_context, NULL, NULL);
> +	if (status) {
> +		BT_DBG("bcm_open failed to power off %d", status);
> +	} else {
> +		status = btbcm_uart_control(BTBCM_UART_ACTION_POWER_ON,
> +					    h4->device_context, NULL, NULL);
> +		if (status)
> +			BT_DBG("bcm_open failed to power on %d", status);
> +	}
> +
> +	/* Start the idle timer */
> +	if (h4->pars.configure_sleep) {
> +		setup_timer(&h4->timer, bcm_idle_timeout, (unsigned long)hu);
> +		if (h4->pars.configure_sleep)
> +			mod_timer(&h4->timer, jiffies + msecs_to_jiffies(
> +				  h4->pars.idle_timeout_in_secs * 1000));
> +	}
> 
> -	hu->priv = bcm;
> 	return 0;
> }
> 
> static int bcm_close(struct hci_uart *hu)
> {
> -	struct bcm_data *bcm = hu->priv;
> +	struct btbcm_uart_callbacks callbacks;
> +	unsigned long callbacks_size = sizeof(callbacks);
> +	struct bcm_data *h4 = hu->priv;
> +	int status;
> 
> -	BT_DBG("hu %p", hu);
> +	hu->priv = NULL;
> 
> -	skb_queue_purge(&bcm->txq);
> -	kfree_skb(bcm->rx_skb);
> -	kfree(bcm);
> +	BT_DBG("bcm_close hu %p", hu);
> +
> +	/* If we're being closed, we must suspend */
> +	if (h4->pars.configure_sleep) {
> +		mutex_lock(&plock);
> +
> +		if (!h4->is_suspended) {
> +			/* Flow control the port */
> +			suspend_notification(hu);
> +
> +			/* Suspend the device */
> +			status = btbcm_uart_control(BTBCM_UART_ACTION_SUSPEND,
> +						    h4->device_context, NULL,
> +						    NULL);
> +			if (status) {
> +				BT_DBG("bcm_close suspend driver fail %d",
> +				       status);
> +			}
> +		}
> +
> +	mutex_unlock(&plock);
> +
> +	del_timer_sync(&h4->timer);
> +	}
> +
> +	/* Power off the device if possible */
> +	status = btbcm_uart_control(BTBCM_UART_ACTION_POWER_OFF,
> +				    h4->device_context, NULL, NULL);
> +	if (status)
> +		BT_DBG("bcm_close failed to power off %d", status);
> +
> +	/* de-configure callbacks on the driver */
> +	callbacks.interface_version = BTBCM_UART_INTERFACE_VERSION;
> +	callbacks.context = hu;
> +	callbacks.p_suspend = NULL;
> +	callbacks.p_resume = NULL;
> +	callbacks.p_wakeup = NULL;
> +	status = btbcm_uart_control(BTBCM_UART_ACTION_CONFIGURE_CALLBACKS,
> +				    h4->device_context, &callbacks,
> +				    &callbacks_size);
> +	if (status)
> +		BT_DBG("bcm_close failed to reset drv callbacks %d", status);
> +	skb_queue_purge(&h4->txq);
> 
> 	hu->priv = NULL;
> +	kfree(h4);
> +
> 	return 0;
> }
> 
> @@ -129,11 +396,42 @@ static int bcm_flush(struct hci_uart *hu)
> 
> static int bcm_setup(struct hci_uart *hu)
> {
> -	char fw_name[64];
> +	char fw_name[128];
> 	const struct firmware *fw;
> 	int err;
> -
> -	BT_DBG("hu %p", hu);
> +	struct sk_buff *skb;
> +	struct bcm_data *h4 = hu->priv;
> +	unsigned char sleep_pars[] = {
> +	0x01,       /* sleep mode 1=UART */
> +	0x02,       /* idle threshold HOST (value * 300ms) */
> +	0x02,       /* idle threshold HC   (value * 300ms) */
> +	0x01,       /* BT_WAKE active mode - 1=active high, 0 = active low */
> +	0x00,       /* HOST_WAKE active mode - 1=active high, 0 = active low */
> +	0x01,       /* Allow host sleep during SCO - FALSE */
> +	0x01,       /* combine sleep mode and LPM - 1 == TRUE */
> +	0x00,       /* enable tristate control of UART TX line - FALSE */
> +	0x00,       /* USB auto-sleep on USB SUSPEND */
> +	0x00,       /* USB USB RESUME timeout (seconds) */
> +	0x00,       /* Pulsed Host Wake */
> +	0x00        /* Enable Break To Host */
> +	};

Unless these are all single byte values, this is not endian safe. And it is horrible to read actually. Lets add the proper structs to btbcm.h like Fred did for the other commands.

And in addition it might be good to add these structs in a separate patch.

> +	unsigned char pcm_int_pars[] = {
> +	0x00,       /* 0=PCM routing, 1=SCO over HCI */
> +	0x02,       /* 0=128Kbps,1=256Kbps,2=512Kbps,3=1024Kbps,4=2048Kbps */
> +	0x00,       /* short frame sync  0=short, 1=long */
> +	0x00,       /* sync mode         0=slave, 1=master */
> +	0x00        /* clock mode        0=slave, 1=master */
> +	};
> +	unsigned char pcm_format_pars[] = {
> +	0x00,       /* LSB_First 1=TRUE, 0=FALSE */
> +	0x00,       /* Fill_Value (use 0-7 for fill bits value) */
> +	0x02,       /* Fill_Method   (2=sign extended) */
> +	0x03,       /* Fill_Num      # of fill bits 0-3) */
> +	0x01        /* Right_Justify 1=TRUE, 0=FALSE */
> +	};
> +	unsigned char time_slot_number = 0;
> +
> +	BT_DBG("bcm_setup hu %p", hu);
> 
> 	hu->hdev->set_bdaddr = btbcm_set_bdaddr;
> 
> @@ -162,6 +460,67 @@ static int bcm_setup(struct hci_uart *hu)
> 			hci_uart_set_baudrate(hu, hu->proto->oper_speed);
> 	}
> 
> +	/* Configure SCO PCM parameters */
> +	if (h4->pars.configure_audio) {
> +		pcm_int_pars[0] = h4->pars.pcm_routing;
> +		pcm_int_pars[1] = h4->pars.pcm_incallbitclock;
> +		pcm_int_pars[2] = h4->pars.pcm_shortframesync;
> +		pcm_int_pars[3] = h4->pars.pcm_syncmode;
> +		pcm_int_pars[4] = h4->pars.pcm_clockmode;

I prefer if vendor drivers add comments above their vendor commands explaining what they do and way. See the Intel driver on how far we went to help others understand what is going here.

> +		skb = __hci_cmd_sync(hu->hdev, 0xfc1c, sizeof(pcm_int_pars),
> +				     pcm_int_pars, HCI_INIT_TIMEOUT);
> +		if (IS_ERR(skb)) {
> +			err = PTR_ERR(skb);
> +			BT_ERR("bcm_setup pcm_ INT VSC failed (%d)", err);
> +			goto finalize;
> +		}
> +		kfree_skb(skb);
> +		BT_DBG("bcm_setup pcm_ INT Parameters VSC succeeded");
> +
> +		pcm_format_pars[0] = h4->pars.pcm_lsbfirst;
> +		pcm_format_pars[1] = h4->pars.pcm_fillvalue;
> +		pcm_format_pars[2] = h4->pars.pcm_fillmethod;
> +		pcm_format_pars[3] = h4->pars.pcm_fillnum;
> +		pcm_format_pars[4] = h4->pars.pcm_rightjustify;
> +		skb = __hci_cmd_sync(hu->hdev, 0xfc1e, sizeof(pcm_format_pars),
> +				     pcm_format_pars, HCI_INIT_TIMEOUT);
> +		if (IS_ERR(skb)) {
> +			err = PTR_ERR(skb);
> +			BT_ERR("bcm_setup pcm_ Format VSC failed (%d)",
> +			       err);
> +			goto finalize;
> +		}
> +		kfree_skb(skb);
> +		BT_DBG("bcm_setup pcm_ Format VSC succeeded");
> +
> +		skb = __hci_cmd_sync(hu->hdev, 0xfc22, sizeof(time_slot_number),
> +				     &time_slot_number, HCI_INIT_TIMEOUT);
> +		if (IS_ERR(skb)) {
> +			err = PTR_ERR(skb);
> +			BT_ERR("bcm_setup SCO Time Slot VSC failed (%d)",
> +			       err);
> +			goto finalize;
> +		}
> +		kfree_skb(skb);
> +		BT_DBG("bcm_setup SCO Time Slot VSC succeeded");
> +	}
> +
> +	/* Configure device's suspend/resume operation */
> +	if (h4->pars.configure_sleep) {
> +		/* Override the default */
> +		sleep_pars[3] = (unsigned char)!h4->pars.bt_wake_active_low;
> +		sleep_pars[4] = (unsigned char)!h4->pars.dev_wake_active_low;
> +		skb = __hci_cmd_sync(hu->hdev, 0xfc27, sizeof(sleep_pars),
> +				     sleep_pars, HCI_INIT_TIMEOUT);
> +		if (IS_ERR(skb)) {
> +			err = PTR_ERR(skb);
> +			BT_ERR("bcm_setup Sleep VSC failed (%d)", err);
> +			goto finalize;
> +		}
> +		kfree_skb(skb);
> +		BT_DBG("bcm_setup Set Sleep Parameters VSC succeeded");
> +	}
> +
> finalize:
> 	release_firmware(fw);
> 
> @@ -183,6 +542,11 @@ static int bcm_recv(struct hci_uart *hu, const void *data, int count)
> 	if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
> 		return -EUNATCH;
> 
> +	BT_DBG("bcm_recv %d bytes", count);

I would prefer not to have this BT_DBG here. This function will be called a lot.

> +
> +	/* Make sure we're resumed */
> +	bcm_ensure_wakeup(hu);
> +
> 	bcm->rx_skb = h4_recv_buf(hu->hdev, bcm->rx_skb, data, count,
> 				  bcm_recv_pkts, ARRAY_SIZE(bcm_recv_pkts));
> 	if (IS_ERR(bcm->rx_skb)) {
> @@ -198,7 +562,10 @@ static int bcm_enqueue(struct hci_uart *hu, struct sk_buff *skb)
> {
> 	struct bcm_data *bcm = hu->priv;
> 
> -	BT_DBG("hu %p skb %p", hu, skb);
> +	BT_DBG("bcm_enqueue hu %p skb %p", hu, skb);

This is again that the function name is available with dynamic debug. So please do not change these or send separate patches with a commit message explaining why this change makes sense.

> +
> +	/* Make sure we're resumed */
> +	bcm_ensure_wakeup(hu);
> 
> 	/* Prepend skb with frame type */
> 	memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
> @@ -214,7 +581,8 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
> 	return skb_dequeue(&bcm->txq);
> }
> 
> -static const struct hci_uart_proto bcm_proto = {
> +/* This structure may not be const as speed may change at runtime */

No. This struct is const. These are the value that a protocol driver sets.

If values are dynamic and change based on DT, then left them zero here and provide a method for overwriting them internally in hci_uart. Please do not hack this in. There are simple UART drivers that we want to enable in a really simple way.

> +static struct hci_uart_proto bcm_proto = {
> 	.id		= HCI_UART_BCM,
> 	.name		= "BCM",
> 	.init_speed	= 115200,

Regards

Marcel

RE: [PATCH v2 5/5] BlueZ Broadcom UART Protocol

[Ilya Faenson]

Hi Marcel,

-----Original Message-----
From: Marcel Holtmann [mailto:marcel@holtmann.org]=20
Sent: Friday, June 12, 2015 5:31 AM
To: Ilya Faenson
Cc: linux-bluetooth@vger.kernel.org; Arend Van Spriel
Subject: Re: [PATCH v2 5/5] BlueZ Broadcom UART Protocol

Hi Ilya,

> Merge Broadcom protocol with the existing implementation, providing
> for UART setup, firmware download and power management.
>=20
> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
> ---
> drivers/bluetooth/hci_bcm.c | 400 +++++++++++++++++++++++++++++++++++++++=
+++--
> 1 file changed, 384 insertions(+), 16 deletions(-)
>=20
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index e4d66b6..5ff35b7 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -3,6 +3,7 @@
>  *  Bluetooth HCI UART driver for Broadcom devices
>  *
>  *  Copyright (C) 2015  Intel Corporation
> + *  Copyright (C) 2015  Broadcom Corporation
>  *
>  *
>  *  This program is free software; you can redistribute it and/or modify
> @@ -24,6 +25,7 @@
> #include <linux/kernel.h>
> #include <linux/errno.h>
> #include <linux/skbuff.h>
> +#include <linux/tty.h>
> #include <linux/firmware.h>
>=20
> #include <net/bluetooth/bluetooth.h>
> @@ -31,12 +33,170 @@
>=20
> #include "btbcm.h"
> #include "hci_uart.h"
> +#include "btbcm_uart.h"
>=20
> struct bcm_data {
> 	struct sk_buff *rx_skb;
> 	struct sk_buff_head txq;
> +	struct hci_uart *hu;
> +
> +	bool is_suspended; /* suspend/resume flag */
> +
> +	struct timer_list timer; /* idle timer */

I prefer not to use timers if we can use delayed work instead.

IF: Could you clarify how you want me to use the delayed work for the frequ=
ently refreshed 5 second timer? It would essentially be an extra thread run=
ning most of the time with us having to synchronize it with the device stop=
s etc. Clear invite for race conditions in my opinion.

> +
> +	struct btbcm_uart_parameters pars; /* device parameters */

btbcm_uart_params and params please. No pars and not point in spelling out =
parameters in a struct name.

IF: Okay.

> +	void *device_context; /* ACPI/DT device context */
> };
>=20
> +/* Suspend/resume synchronization mutex */
> +static DEFINE_MUTEX(plock);

Please use a more descriptive name than plock. That is too generic for a gl=
obal variable.

IF: Okay.

> +
> +/* Forward reference */
> +static struct hci_uart_proto bcm_proto;

Why do we need this? I am generally not in favor of having forward referenc=
e until really really needed.

IF: That is needed to set the oper-speed in this structure once the device =
parameters are available. Intel patch has hard-coded it to a single value f=
or all Broadcom devices which is not acceptable.

> +
> +/*
> + * Callbacks from the BCMBT_UART device
> + */
> +
> +/*
> + * The platform is suspending. Stop UART activity
> + */
> +static void suspend_notification(void *context)
> +{
> +	struct hci_uart *hu =3D (struct hci_uart *)context;

context is void * and so no need to cast.

IF: Okay.

> +	struct bcm_data *h4 =3D hu->priv;
> +
> +	BT_DBG("%s: is_suspended %d", __func__, h4->is_suspended);
> +
> +	if (!h4->pars.configure_sleep)
> +		return;
> +
> +	if (!h4->is_suspended) {
> +		if (h4->pars.manual_fc)
> +			hci_uart_set_flow_control(hu, true);
> +
> +		/* Once this callback returns, driver suspends BT via GPIO */
> +		h4->is_suspended =3D true;
> +	}
> +}
> +
> +/*
> + * The platform is resuming. Resume UART activity.
> + */
> +static void resume_notification(void *context)
> +{
> +	struct hci_uart *hu =3D (struct hci_uart *)context;
> +	struct bcm_data *h4 =3D hu->priv;
> +
> +	BT_DBG("%s: is_suspended %d", __func__, h4->is_suspended);
> +
> +	if (!h4->pars.configure_sleep)
> +		return;
> +
> +	/* When this callback executes, the device has woken up already */
> +	if (h4->is_suspended) {
> +		h4->is_suspended =3D false;
> +
> +		if (h4->pars.manual_fc)
> +			hci_uart_set_flow_control(hu, false);
> +	}
> +
> +	/* If we're resumed, the idle timer must be running */
> +	mod_timer(&h4->timer, jiffies +
> +	    msecs_to_jiffies(h4->pars.idle_timeout_in_secs * 1000));

Indention is violated here.

IF: Okay, will change.

> +}
> +
> +/*
> + * The BT device is resuming. Resume UART activity if suspended
> + */
> +static void wakeup_notification(void *context)
> +{
> +	struct hci_uart *hu =3D (struct hci_uart *)context;
> +	struct bcm_data *h4 =3D hu->priv;
> +
> +	BT_DBG("%s: is_suspended %d", __func__, h4->is_suspended);
> +
> +	if (!h4->pars.configure_sleep)
> +		return;
> +
> +	if (h4->is_suspended) {
> +		if (h4->pars.manual_fc)
> +			hci_uart_set_flow_control(hu, false);
> +
> +		h4->is_suspended =3D false;
> +	}
> +
> +	/* If we're resumed, the idle timer must be running */
> +	mod_timer(&h4->timer, jiffies + msecs_to_jiffies(
> +			   h4->pars.idle_timeout_in_secs * 1000));

Same here actually and worse it is different from the other one.

Splitting the rearming into its own helper might help here.

IF: Okay, will do.

> +}
> +
> +/*
> + * Make sure we're awake
> + * (called when the resumed state is required)
> + */
> +static void bcm_ensure_wakeup(struct hci_uart *hu)
> +{
> +	struct bcm_data *h4 =3D hu->priv;
> +	int status;
> +
> +	if (!h4->pars.configure_sleep)
> +		return;
> +
> +	/* Suspend/resume operations are serialized */
> +	mutex_lock(&plock);
> +
> +	/* Nothing to do if resumed already */
> +	if (!h4->is_suspended) {
> +		mutex_unlock(&plock);
> +
> +		/* Just reset the timer */
> +		status =3D mod_timer(&h4->timer, jiffies + msecs_to_jiffies(
> +				   h4->pars.idle_timeout_in_secs * 1000));

This indentation is still not correct and now we have a 3rd version ;)

IF: Will change.

> +		return;
> +	}
> +
> +	/* Wakeup the device */
> +	status =3D btbcm_uart_control(BTBCM_UART_ACTION_RESUME,
> +				    h4->device_context, NULL, NULL);
> +	if (status)
> +		BT_DBG("%s: failed to resume driver %d", __func__, status);

Conditional BT_DBG on the status is pretty useless in my mind. Either BT_DB=
G the status or don't at all.

IF: No trace upon success sounds reasonable to me but I'll change it as per=
 your preferences.

> +
> +	/* Unflow control the port if configured */
> +	resume_notification(hu);
> +
> +	mutex_unlock(&plock);
> +}
> +
> +/*
> + * Idle timer callback
> + */
> +static void bcm_idle_timeout(unsigned long arg)
> +{
> +	struct hci_uart *hu =3D (struct hci_uart *)arg;
> +	struct bcm_data *h4 =3D hu->priv;
> +	int status;
> +
> +	BT_DBG("%s: hu %p", __func__, hu);
> +
> +	/* Suspend/resume operations are serialized */
> +	mutex_lock(&plock);
> +
> +	if (!h4->is_suspended) {
> +		/* Flow control the port if configured */
> +		suspend_notification(hu);
> +
> +		/* Suspend the device */
> +		status =3D btbcm_uart_control(BTBCM_UART_ACTION_SUSPEND,
> +					    h4->device_context, NULL, NULL);
> +		if (status)
> +			BT_DBG("%s: failed to suspend device %d", __func__,
> +			       status);
> +	}
> +
> +	mutex_unlock(&plock);
> +}
> +
> static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)
> {
> 	struct hci_dev *hdev =3D hu->hdev;
> @@ -88,31 +248,138 @@ static int bcm_set_baudrate(struct hci_uart *hu, un=
signed int speed)
>=20
> static int bcm_open(struct hci_uart *hu)
> {
> -	struct bcm_data *bcm;
> +	struct btbcm_uart_callbacks callbacks;
> +	unsigned long callbacks_size =3D sizeof(callbacks);

Not needed since it is all inside the kernel. And I think once this is spli=
t into individual exported functions, this will become a lot clearer.

IF: Okay, will remove.

> +	int status;
> +	struct bcm_data *h4;
> +	struct tty_struct *tty =3D hu->tty;
>=20
> -	BT_DBG("hu %p", hu);
> +	BT_DBG("bcm_open hu %p", hu);
>=20
> -	bcm =3D kzalloc(sizeof(*bcm), GFP_KERNEL);
> -	if (!bcm)
> +	h4 =3D kzalloc(sizeof(*h4), GFP_KERNEL);
> +	if (!h4)
> 		return -ENOMEM;

If we are doing renaming of variables, then I am expecting a cleanup patch =
first that does only that. If this is needed at all or just some left-over =
merge issue.

IF: This is indeed a leftover merge issue. Will restore the original.

>=20
> -	skb_queue_head_init(&bcm->txq);
> +	skb_queue_head_init(&h4->txq);
> +	hu->priv =3D h4;
> +	h4->hu =3D hu;
> +	h4->is_suspended =3D false;
> +
> +	/* Configure callbacks on the driver */
> +	callbacks.interface_version =3D BTBCM_UART_INTERFACE_VERSION;
> +	callbacks.context =3D hu;
> +	strcpy(callbacks.name, tty->name);
> +	callbacks.p_suspend =3D suspend_notification;
> +	callbacks.p_resume =3D resume_notification;
> +	callbacks.p_wakeup =3D wakeup_notification;
> +	status =3D btbcm_uart_control(BTBCM_UART_ACTION_CONFIGURE_CALLBACKS,
> +				    NULL, &callbacks, &callbacks_size);
> +	if (status) {
> +		BT_DBG("bcm_open failed to set driver callbacks %d", status);
> +		return status;
> +	}
> +	if (callbacks_size !=3D sizeof(void *)) {
> +		BT_DBG("bcm_open got back %d bytes from callbacks?!",
> +		       (int)callbacks_size);
> +		return -EMSGSIZE;
> +	}
> +	memcpy(&h4->device_context, &callbacks, sizeof(void *));
> +	BT_DBG("bcm_open callbacks context %p", h4->device_context);
> +
> +	/* Retrieve device parameters */
> +	callbacks_size =3D sizeof(h4->pars);
> +	status =3D btbcm_uart_control(BTBCM_UART_ACTION_GET_PARAMETERS,
> +				    h4->device_context, &h4->pars,
> +				    &callbacks_size);
> +	if (status) {
> +		BT_DBG("bcm_open failed to get dev parameters %d", status);
> +		return status;
> +	}
> +	BT_DBG("Pars ver %d csleep %d dalow %d balow %d idle %d",
> +	       h4->pars.interface_version, h4->pars.configure_sleep,
> +	       h4->pars.dev_wake_active_low, h4->pars.bt_wake_active_low,
> +	       h4->pars.idle_timeout_in_secs);
> +	bcm_proto.oper_speed =3D h4->pars.oper_speed;
> +
> +	/* Cycle power to make sure the device is in the known state */
> +	status =3D btbcm_uart_control(BTBCM_UART_ACTION_POWER_OFF,
> +				    h4->device_context, NULL, NULL);
> +	if (status) {
> +		BT_DBG("bcm_open failed to power off %d", status);
> +	} else {
> +		status =3D btbcm_uart_control(BTBCM_UART_ACTION_POWER_ON,
> +					    h4->device_context, NULL, NULL);
> +		if (status)
> +			BT_DBG("bcm_open failed to power on %d", status);
> +	}
> +
> +	/* Start the idle timer */
> +	if (h4->pars.configure_sleep) {
> +		setup_timer(&h4->timer, bcm_idle_timeout, (unsigned long)hu);
> +		if (h4->pars.configure_sleep)
> +			mod_timer(&h4->timer, jiffies + msecs_to_jiffies(
> +				  h4->pars.idle_timeout_in_secs * 1000));
> +	}
>=20
> -	hu->priv =3D bcm;
> 	return 0;
> }
>=20
> static int bcm_close(struct hci_uart *hu)
> {
> -	struct bcm_data *bcm =3D hu->priv;
> +	struct btbcm_uart_callbacks callbacks;
> +	unsigned long callbacks_size =3D sizeof(callbacks);
> +	struct bcm_data *h4 =3D hu->priv;
> +	int status;
>=20
> -	BT_DBG("hu %p", hu);
> +	hu->priv =3D NULL;
>=20
> -	skb_queue_purge(&bcm->txq);
> -	kfree_skb(bcm->rx_skb);
> -	kfree(bcm);
> +	BT_DBG("bcm_close hu %p", hu);
> +
> +	/* If we're being closed, we must suspend */
> +	if (h4->pars.configure_sleep) {
> +		mutex_lock(&plock);
> +
> +		if (!h4->is_suspended) {
> +			/* Flow control the port */
> +			suspend_notification(hu);
> +
> +			/* Suspend the device */
> +			status =3D btbcm_uart_control(BTBCM_UART_ACTION_SUSPEND,
> +						    h4->device_context, NULL,
> +						    NULL);
> +			if (status) {
> +				BT_DBG("bcm_close suspend driver fail %d",
> +				       status);
> +			}
> +		}
> +
> +	mutex_unlock(&plock);
> +
> +	del_timer_sync(&h4->timer);
> +	}
> +
> +	/* Power off the device if possible */
> +	status =3D btbcm_uart_control(BTBCM_UART_ACTION_POWER_OFF,
> +				    h4->device_context, NULL, NULL);
> +	if (status)
> +		BT_DBG("bcm_close failed to power off %d", status);
> +
> +	/* de-configure callbacks on the driver */
> +	callbacks.interface_version =3D BTBCM_UART_INTERFACE_VERSION;
> +	callbacks.context =3D hu;
> +	callbacks.p_suspend =3D NULL;
> +	callbacks.p_resume =3D NULL;
> +	callbacks.p_wakeup =3D NULL;
> +	status =3D btbcm_uart_control(BTBCM_UART_ACTION_CONFIGURE_CALLBACKS,
> +				    h4->device_context, &callbacks,
> +				    &callbacks_size);
> +	if (status)
> +		BT_DBG("bcm_close failed to reset drv callbacks %d", status);
> +	skb_queue_purge(&h4->txq);
>=20
> 	hu->priv =3D NULL;
> +	kfree(h4);
> +
> 	return 0;
> }
>=20
> @@ -129,11 +396,42 @@ static int bcm_flush(struct hci_uart *hu)
>=20
> static int bcm_setup(struct hci_uart *hu)
> {
> -	char fw_name[64];
> +	char fw_name[128];
> 	const struct firmware *fw;
> 	int err;
> -
> -	BT_DBG("hu %p", hu);
> +	struct sk_buff *skb;
> +	struct bcm_data *h4 =3D hu->priv;
> +	unsigned char sleep_pars[] =3D {
> +	0x01,       /* sleep mode 1=3DUART */
> +	0x02,       /* idle threshold HOST (value * 300ms) */
> +	0x02,       /* idle threshold HC   (value * 300ms) */
> +	0x01,       /* BT_WAKE active mode - 1=3Dactive high, 0 =3D active low =
*/
> +	0x00,       /* HOST_WAKE active mode - 1=3Dactive high, 0 =3D active lo=
w */
> +	0x01,       /* Allow host sleep during SCO - FALSE */
> +	0x01,       /* combine sleep mode and LPM - 1 =3D=3D TRUE */
> +	0x00,       /* enable tristate control of UART TX line - FALSE */
> +	0x00,       /* USB auto-sleep on USB SUSPEND */
> +	0x00,       /* USB USB RESUME timeout (seconds) */
> +	0x00,       /* Pulsed Host Wake */
> +	0x00        /* Enable Break To Host */
> +	};

Unless these are all single byte values, this is not endian safe. And it is=
 horrible to read actually. Lets add the proper structs to btbcm.h like Fre=
d did for the other commands.

And in addition it might be good to add these structs in a separate patch.

IF: These are all single byte values so it is safe for any architecture. Ok=
ay, I'll add separate structures since you wish so. I don't understand the =
need for a separate patch though as the sleep configuration is a primary ob=
jective of this driver.

> +	unsigned char pcm_int_pars[] =3D {
> +	0x00,       /* 0=3DPCM routing, 1=3DSCO over HCI */
> +	0x02,       /* 0=3D128Kbps,1=3D256Kbps,2=3D512Kbps,3=3D1024Kbps,4=3D204=
8Kbps */
> +	0x00,       /* short frame sync  0=3Dshort, 1=3Dlong */
> +	0x00,       /* sync mode         0=3Dslave, 1=3Dmaster */
> +	0x00        /* clock mode        0=3Dslave, 1=3Dmaster */
> +	};
> +	unsigned char pcm_format_pars[] =3D {
> +	0x00,       /* LSB_First 1=3DTRUE, 0=3DFALSE */
> +	0x00,       /* Fill_Value (use 0-7 for fill bits value) */
> +	0x02,       /* Fill_Method   (2=3Dsign extended) */
> +	0x03,       /* Fill_Num      # of fill bits 0-3) */
> +	0x01        /* Right_Justify 1=3DTRUE, 0=3DFALSE */
> +	};
> +	unsigned char time_slot_number =3D 0;
> +
> +	BT_DBG("bcm_setup hu %p", hu);
>=20
> 	hu->hdev->set_bdaddr =3D btbcm_set_bdaddr;
>=20
> @@ -162,6 +460,67 @@ static int bcm_setup(struct hci_uart *hu)
> 			hci_uart_set_baudrate(hu, hu->proto->oper_speed);
> 	}
>=20
> +	/* Configure SCO PCM parameters */
> +	if (h4->pars.configure_audio) {
> +		pcm_int_pars[0] =3D h4->pars.pcm_routing;
> +		pcm_int_pars[1] =3D h4->pars.pcm_incallbitclock;
> +		pcm_int_pars[2] =3D h4->pars.pcm_shortframesync;
> +		pcm_int_pars[3] =3D h4->pars.pcm_syncmode;
> +		pcm_int_pars[4] =3D h4->pars.pcm_clockmode;

I prefer if vendor drivers add comments above their vendor commands explain=
ing what they do and way. See the Intel driver on how far we went to help o=
thers understand what is going here.

IF: I will add comments here.

> +		skb =3D __hci_cmd_sync(hu->hdev, 0xfc1c, sizeof(pcm_int_pars),
> +				     pcm_int_pars, HCI_INIT_TIMEOUT);
> +		if (IS_ERR(skb)) {
> +			err =3D PTR_ERR(skb);
> +			BT_ERR("bcm_setup pcm_ INT VSC failed (%d)", err);
> +			goto finalize;
> +		}
> +		kfree_skb(skb);
> +		BT_DBG("bcm_setup pcm_ INT Parameters VSC succeeded");
> +
> +		pcm_format_pars[0] =3D h4->pars.pcm_lsbfirst;
> +		pcm_format_pars[1] =3D h4->pars.pcm_fillvalue;
> +		pcm_format_pars[2] =3D h4->pars.pcm_fillmethod;
> +		pcm_format_pars[3] =3D h4->pars.pcm_fillnum;
> +		pcm_format_pars[4] =3D h4->pars.pcm_rightjustify;
> +		skb =3D __hci_cmd_sync(hu->hdev, 0xfc1e, sizeof(pcm_format_pars),
> +				     pcm_format_pars, HCI_INIT_TIMEOUT);
> +		if (IS_ERR(skb)) {
> +			err =3D PTR_ERR(skb);
> +			BT_ERR("bcm_setup pcm_ Format VSC failed (%d)",
> +			       err);
> +			goto finalize;
> +		}
> +		kfree_skb(skb);
> +		BT_DBG("bcm_setup pcm_ Format VSC succeeded");
> +
> +		skb =3D __hci_cmd_sync(hu->hdev, 0xfc22, sizeof(time_slot_number),
> +				     &time_slot_number, HCI_INIT_TIMEOUT);
> +		if (IS_ERR(skb)) {
> +			err =3D PTR_ERR(skb);
> +			BT_ERR("bcm_setup SCO Time Slot VSC failed (%d)",
> +			       err);
> +			goto finalize;
> +		}
> +		kfree_skb(skb);
> +		BT_DBG("bcm_setup SCO Time Slot VSC succeeded");
> +	}
> +
> +	/* Configure device's suspend/resume operation */
> +	if (h4->pars.configure_sleep) {
> +		/* Override the default */
> +		sleep_pars[3] =3D (unsigned char)!h4->pars.bt_wake_active_low;
> +		sleep_pars[4] =3D (unsigned char)!h4->pars.dev_wake_active_low;
> +		skb =3D __hci_cmd_sync(hu->hdev, 0xfc27, sizeof(sleep_pars),
> +				     sleep_pars, HCI_INIT_TIMEOUT);
> +		if (IS_ERR(skb)) {
> +			err =3D PTR_ERR(skb);
> +			BT_ERR("bcm_setup Sleep VSC failed (%d)", err);
> +			goto finalize;
> +		}
> +		kfree_skb(skb);
> +		BT_DBG("bcm_setup Set Sleep Parameters VSC succeeded");
> +	}
> +
> finalize:
> 	release_firmware(fw);
>=20
> @@ -183,6 +542,11 @@ static int bcm_recv(struct hci_uart *hu, const void =
*data, int count)
> 	if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
> 		return -EUNATCH;
>=20
> +	BT_DBG("bcm_recv %d bytes", count);

I would prefer not to have this BT_DBG here. This function will be called a=
 lot.

IF: This has been very useful to me to see whether anything has been receiv=
ed or not at the driver setup time. How would you want me to learn that?

> +
> +	/* Make sure we're resumed */
> +	bcm_ensure_wakeup(hu);
> +
> 	bcm->rx_skb =3D h4_recv_buf(hu->hdev, bcm->rx_skb, data, count,
> 				  bcm_recv_pkts, ARRAY_SIZE(bcm_recv_pkts));
> 	if (IS_ERR(bcm->rx_skb)) {
> @@ -198,7 +562,10 @@ static int bcm_enqueue(struct hci_uart *hu, struct s=
k_buff *skb)
> {
> 	struct bcm_data *bcm =3D hu->priv;
>=20
> -	BT_DBG("hu %p skb %p", hu, skb);
> +	BT_DBG("bcm_enqueue hu %p skb %p", hu, skb);

This is again that the function name is available with dynamic debug. So pl=
ease do not change these or send separate patches with a commit message exp=
laining why this change makes sense.

IF: Okay, will remove although dynamic debug is not available on all platfo=
rms.

> +
> +	/* Make sure we're resumed */
> +	bcm_ensure_wakeup(hu);
>=20
> 	/* Prepend skb with frame type */
> 	memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
> @@ -214,7 +581,8 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *h=
u)
> 	return skb_dequeue(&bcm->txq);
> }
>=20
> -static const struct hci_uart_proto bcm_proto =3D {
> +/* This structure may not be const as speed may change at runtime */

No. This struct is const. These are the value that a protocol driver sets.

If values are dynamic and change based on DT, then left them zero here and =
provide a method for overwriting them internally in hci_uart. Please do not=
 hack this in. There are simple UART drivers that we want to enable in a re=
ally simple way.

IF: As I said above, I've inherited the need to change the speed(s) from th=
e recent Intel patches. Will try fixing that.

> +static struct hci_uart_proto bcm_proto =3D {
> 	.id		=3D HCI_UART_BCM,
> 	.name		=3D "BCM",
> 	.init_speed	=3D 115200,

Regards

Marcel

Re: [PATCH v2 5/5] BlueZ Broadcom UART Protocol

[Marcel Holtmann]

Hi Ilya,

>> Merge Broadcom protocol with the existing implementation, providing
>> for UART setup, firmware download and power management.
>> 
>> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
>> ---
>> drivers/bluetooth/hci_bcm.c | 400 ++++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 384 insertions(+), 16 deletions(-)
>> 
>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>> index e4d66b6..5ff35b7 100644
>> --- a/drivers/bluetooth/hci_bcm.c
>> +++ b/drivers/bluetooth/hci_bcm.c
>> @@ -3,6 +3,7 @@
>> *  Bluetooth HCI UART driver for Broadcom devices
>> *
>> *  Copyright (C) 2015  Intel Corporation
>> + *  Copyright (C) 2015  Broadcom Corporation
>> *
>> *
>> *  This program is free software; you can redistribute it and/or modify
>> @@ -24,6 +25,7 @@
>> #include <linux/kernel.h>
>> #include <linux/errno.h>
>> #include <linux/skbuff.h>
>> +#include <linux/tty.h>
>> #include <linux/firmware.h>
>> 
>> #include <net/bluetooth/bluetooth.h>
>> @@ -31,12 +33,170 @@
>> 
>> #include "btbcm.h"
>> #include "hci_uart.h"
>> +#include "btbcm_uart.h"
>> 
>> struct bcm_data {
>> 	struct sk_buff *rx_skb;
>> 	struct sk_buff_head txq;
>> +	struct hci_uart *hu;
>> +
>> +	bool is_suspended; /* suspend/resume flag */
>> +
>> +	struct timer_list timer; /* idle timer */
> 
> I prefer not to use timers if we can use delayed work instead.
> 
> IF: Could you clarify how you want me to use the delayed work for the frequently refreshed 5 second timer? It would essentially be an extra thread running most of the time with us having to synchronize it with the device stops etc. Clear invite for race conditions in my opinion.

refreshed means re-armed? I wonder how delayed work is different then? Maybe I need to understand a bit more than first.

> 
>> +
>> +	struct btbcm_uart_parameters pars; /* device parameters */
> 
> btbcm_uart_params and params please. No pars and not point in spelling out parameters in a struct name.
> 
> IF: Okay.
> 
>> +	void *device_context; /* ACPI/DT device context */
>> };
>> 
>> +/* Suspend/resume synchronization mutex */
>> +static DEFINE_MUTEX(plock);
> 
> Please use a more descriptive name than plock. That is too generic for a global variable.
> 
> IF: Okay.
> 
>> +
>> +/* Forward reference */
>> +static struct hci_uart_proto bcm_proto;
> 
> Why do we need this? I am generally not in favor of having forward reference until really really needed.
> 
> IF: That is needed to set the oper-speed in this structure once the device parameters are available. Intel patch has hard-coded it to a single value for all Broadcom devices which is not acceptable.

So for Broadcom devices, I think in the static table, no oper-speed should be set. That means it has to come by other means. Either ACPI or DT. But as I mentioned in the other part, that table has to stay const.

> 
>> +
>> +/*
>> + * Callbacks from the BCMBT_UART device
>> + */
>> +
>> +/*
>> + * The platform is suspending. Stop UART activity
>> + */
>> +static void suspend_notification(void *context)
>> +{
>> +	struct hci_uart *hu = (struct hci_uart *)context;
> 
> context is void * and so no need to cast.
> 
> IF: Okay.
> 
>> +	struct bcm_data *h4 = hu->priv;
>> +
>> +	BT_DBG("%s: is_suspended %d", __func__, h4->is_suspended);
>> +
>> +	if (!h4->pars.configure_sleep)
>> +		return;
>> +
>> +	if (!h4->is_suspended) {
>> +		if (h4->pars.manual_fc)
>> +			hci_uart_set_flow_control(hu, true);
>> +
>> +		/* Once this callback returns, driver suspends BT via GPIO */
>> +		h4->is_suspended = true;
>> +	}
>> +}
>> +
>> +/*
>> + * The platform is resuming. Resume UART activity.
>> + */
>> +static void resume_notification(void *context)
>> +{
>> +	struct hci_uart *hu = (struct hci_uart *)context;
>> +	struct bcm_data *h4 = hu->priv;
>> +
>> +	BT_DBG("%s: is_suspended %d", __func__, h4->is_suspended);
>> +
>> +	if (!h4->pars.configure_sleep)
>> +		return;
>> +
>> +	/* When this callback executes, the device has woken up already */
>> +	if (h4->is_suspended) {
>> +		h4->is_suspended = false;
>> +
>> +		if (h4->pars.manual_fc)
>> +			hci_uart_set_flow_control(hu, false);
>> +	}
>> +
>> +	/* If we're resumed, the idle timer must be running */
>> +	mod_timer(&h4->timer, jiffies +
>> +	    msecs_to_jiffies(h4->pars.idle_timeout_in_secs * 1000));
> 
> Indention is violated here.
> 
> IF: Okay, will change.
> 
>> +}
>> +
>> +/*
>> + * The BT device is resuming. Resume UART activity if suspended
>> + */
>> +static void wakeup_notification(void *context)
>> +{
>> +	struct hci_uart *hu = (struct hci_uart *)context;
>> +	struct bcm_data *h4 = hu->priv;
>> +
>> +	BT_DBG("%s: is_suspended %d", __func__, h4->is_suspended);
>> +
>> +	if (!h4->pars.configure_sleep)
>> +		return;
>> +
>> +	if (h4->is_suspended) {
>> +		if (h4->pars.manual_fc)
>> +			hci_uart_set_flow_control(hu, false);
>> +
>> +		h4->is_suspended = false;
>> +	}
>> +
>> +	/* If we're resumed, the idle timer must be running */
>> +	mod_timer(&h4->timer, jiffies + msecs_to_jiffies(
>> +			   h4->pars.idle_timeout_in_secs * 1000));
> 
> Same here actually and worse it is different from the other one.
> 
> Splitting the rearming into its own helper might help here.
> 
> IF: Okay, will do.
> 
>> +}
>> +
>> +/*
>> + * Make sure we're awake
>> + * (called when the resumed state is required)
>> + */
>> +static void bcm_ensure_wakeup(struct hci_uart *hu)
>> +{
>> +	struct bcm_data *h4 = hu->priv;
>> +	int status;
>> +
>> +	if (!h4->pars.configure_sleep)
>> +		return;
>> +
>> +	/* Suspend/resume operations are serialized */
>> +	mutex_lock(&plock);
>> +
>> +	/* Nothing to do if resumed already */
>> +	if (!h4->is_suspended) {
>> +		mutex_unlock(&plock);
>> +
>> +		/* Just reset the timer */
>> +		status = mod_timer(&h4->timer, jiffies + msecs_to_jiffies(
>> +				   h4->pars.idle_timeout_in_secs * 1000));
> 
> This indentation is still not correct and now we have a 3rd version ;)
> 
> IF: Will change.
> 
>> +		return;
>> +	}
>> +
>> +	/* Wakeup the device */
>> +	status = btbcm_uart_control(BTBCM_UART_ACTION_RESUME,
>> +				    h4->device_context, NULL, NULL);
>> +	if (status)
>> +		BT_DBG("%s: failed to resume driver %d", __func__, status);
> 
> Conditional BT_DBG on the status is pretty useless in my mind. Either BT_DBG the status or don't at all.
> 
> IF: No trace upon success sounds reasonable to me but I'll change it as per your preferences.

They are debug entries and in all the other code we have used them mainly unconditional. I am even fine if you just use BT_ERR here since something really went wrong. Just a conditional debug seems odd.

> 
>> +
>> +	/* Unflow control the port if configured */
>> +	resume_notification(hu);
>> +
>> +	mutex_unlock(&plock);
>> +}
>> +
>> +/*
>> + * Idle timer callback
>> + */
>> +static void bcm_idle_timeout(unsigned long arg)
>> +{
>> +	struct hci_uart *hu = (struct hci_uart *)arg;
>> +	struct bcm_data *h4 = hu->priv;
>> +	int status;
>> +
>> +	BT_DBG("%s: hu %p", __func__, hu);
>> +
>> +	/* Suspend/resume operations are serialized */
>> +	mutex_lock(&plock);
>> +
>> +	if (!h4->is_suspended) {
>> +		/* Flow control the port if configured */
>> +		suspend_notification(hu);
>> +
>> +		/* Suspend the device */
>> +		status = btbcm_uart_control(BTBCM_UART_ACTION_SUSPEND,
>> +					    h4->device_context, NULL, NULL);
>> +		if (status)
>> +			BT_DBG("%s: failed to suspend device %d", __func__,
>> +			       status);
>> +	}
>> +
>> +	mutex_unlock(&plock);
>> +}
>> +
>> static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)
>> {
>> 	struct hci_dev *hdev = hu->hdev;
>> @@ -88,31 +248,138 @@ static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)
>> 
>> static int bcm_open(struct hci_uart *hu)
>> {
>> -	struct bcm_data *bcm;
>> +	struct btbcm_uart_callbacks callbacks;
>> +	unsigned long callbacks_size = sizeof(callbacks);
> 
> Not needed since it is all inside the kernel. And I think once this is split into individual exported functions, this will become a lot clearer.
> 
> IF: Okay, will remove.
> 
>> +	int status;
>> +	struct bcm_data *h4;
>> +	struct tty_struct *tty = hu->tty;
>> 
>> -	BT_DBG("hu %p", hu);
>> +	BT_DBG("bcm_open hu %p", hu);
>> 
>> -	bcm = kzalloc(sizeof(*bcm), GFP_KERNEL);
>> -	if (!bcm)
>> +	h4 = kzalloc(sizeof(*h4), GFP_KERNEL);
>> +	if (!h4)
>> 		return -ENOMEM;
> 
> If we are doing renaming of variables, then I am expecting a cleanup patch first that does only that. If this is needed at all or just some left-over merge issue.
> 
> IF: This is indeed a leftover merge issue. Will restore the original.
> 
>> 
>> -	skb_queue_head_init(&bcm->txq);
>> +	skb_queue_head_init(&h4->txq);
>> +	hu->priv = h4;
>> +	h4->hu = hu;
>> +	h4->is_suspended = false;
>> +
>> +	/* Configure callbacks on the driver */
>> +	callbacks.interface_version = BTBCM_UART_INTERFACE_VERSION;
>> +	callbacks.context = hu;
>> +	strcpy(callbacks.name, tty->name);
>> +	callbacks.p_suspend = suspend_notification;
>> +	callbacks.p_resume = resume_notification;
>> +	callbacks.p_wakeup = wakeup_notification;
>> +	status = btbcm_uart_control(BTBCM_UART_ACTION_CONFIGURE_CALLBACKS,
>> +				    NULL, &callbacks, &callbacks_size);
>> +	if (status) {
>> +		BT_DBG("bcm_open failed to set driver callbacks %d", status);
>> +		return status;
>> +	}
>> +	if (callbacks_size != sizeof(void *)) {
>> +		BT_DBG("bcm_open got back %d bytes from callbacks?!",
>> +		       (int)callbacks_size);
>> +		return -EMSGSIZE;
>> +	}
>> +	memcpy(&h4->device_context, &callbacks, sizeof(void *));
>> +	BT_DBG("bcm_open callbacks context %p", h4->device_context);
>> +
>> +	/* Retrieve device parameters */
>> +	callbacks_size = sizeof(h4->pars);
>> +	status = btbcm_uart_control(BTBCM_UART_ACTION_GET_PARAMETERS,
>> +				    h4->device_context, &h4->pars,
>> +				    &callbacks_size);
>> +	if (status) {
>> +		BT_DBG("bcm_open failed to get dev parameters %d", status);
>> +		return status;
>> +	}
>> +	BT_DBG("Pars ver %d csleep %d dalow %d balow %d idle %d",
>> +	       h4->pars.interface_version, h4->pars.configure_sleep,
>> +	       h4->pars.dev_wake_active_low, h4->pars.bt_wake_active_low,
>> +	       h4->pars.idle_timeout_in_secs);
>> +	bcm_proto.oper_speed = h4->pars.oper_speed;
>> +
>> +	/* Cycle power to make sure the device is in the known state */
>> +	status = btbcm_uart_control(BTBCM_UART_ACTION_POWER_OFF,
>> +				    h4->device_context, NULL, NULL);
>> +	if (status) {
>> +		BT_DBG("bcm_open failed to power off %d", status);
>> +	} else {
>> +		status = btbcm_uart_control(BTBCM_UART_ACTION_POWER_ON,
>> +					    h4->device_context, NULL, NULL);
>> +		if (status)
>> +			BT_DBG("bcm_open failed to power on %d", status);
>> +	}
>> +
>> +	/* Start the idle timer */
>> +	if (h4->pars.configure_sleep) {
>> +		setup_timer(&h4->timer, bcm_idle_timeout, (unsigned long)hu);
>> +		if (h4->pars.configure_sleep)
>> +			mod_timer(&h4->timer, jiffies + msecs_to_jiffies(
>> +				  h4->pars.idle_timeout_in_secs * 1000));
>> +	}
>> 
>> -	hu->priv = bcm;
>> 	return 0;
>> }
>> 
>> static int bcm_close(struct hci_uart *hu)
>> {
>> -	struct bcm_data *bcm = hu->priv;
>> +	struct btbcm_uart_callbacks callbacks;
>> +	unsigned long callbacks_size = sizeof(callbacks);
>> +	struct bcm_data *h4 = hu->priv;
>> +	int status;
>> 
>> -	BT_DBG("hu %p", hu);
>> +	hu->priv = NULL;
>> 
>> -	skb_queue_purge(&bcm->txq);
>> -	kfree_skb(bcm->rx_skb);
>> -	kfree(bcm);
>> +	BT_DBG("bcm_close hu %p", hu);
>> +
>> +	/* If we're being closed, we must suspend */
>> +	if (h4->pars.configure_sleep) {
>> +		mutex_lock(&plock);
>> +
>> +		if (!h4->is_suspended) {
>> +			/* Flow control the port */
>> +			suspend_notification(hu);
>> +
>> +			/* Suspend the device */
>> +			status = btbcm_uart_control(BTBCM_UART_ACTION_SUSPEND,
>> +						    h4->device_context, NULL,
>> +						    NULL);
>> +			if (status) {
>> +				BT_DBG("bcm_close suspend driver fail %d",
>> +				       status);
>> +			}
>> +		}
>> +
>> +	mutex_unlock(&plock);
>> +
>> +	del_timer_sync(&h4->timer);
>> +	}
>> +
>> +	/* Power off the device if possible */
>> +	status = btbcm_uart_control(BTBCM_UART_ACTION_POWER_OFF,
>> +				    h4->device_context, NULL, NULL);
>> +	if (status)
>> +		BT_DBG("bcm_close failed to power off %d", status);
>> +
>> +	/* de-configure callbacks on the driver */
>> +	callbacks.interface_version = BTBCM_UART_INTERFACE_VERSION;
>> +	callbacks.context = hu;
>> +	callbacks.p_suspend = NULL;
>> +	callbacks.p_resume = NULL;
>> +	callbacks.p_wakeup = NULL;
>> +	status = btbcm_uart_control(BTBCM_UART_ACTION_CONFIGURE_CALLBACKS,
>> +				    h4->device_context, &callbacks,
>> +				    &callbacks_size);
>> +	if (status)
>> +		BT_DBG("bcm_close failed to reset drv callbacks %d", status);
>> +	skb_queue_purge(&h4->txq);
>> 
>> 	hu->priv = NULL;
>> +	kfree(h4);
>> +
>> 	return 0;
>> }
>> 
>> @@ -129,11 +396,42 @@ static int bcm_flush(struct hci_uart *hu)
>> 
>> static int bcm_setup(struct hci_uart *hu)
>> {
>> -	char fw_name[64];
>> +	char fw_name[128];
>> 	const struct firmware *fw;
>> 	int err;
>> -
>> -	BT_DBG("hu %p", hu);
>> +	struct sk_buff *skb;
>> +	struct bcm_data *h4 = hu->priv;
>> +	unsigned char sleep_pars[] = {
>> +	0x01,       /* sleep mode 1=UART */
>> +	0x02,       /* idle threshold HOST (value * 300ms) */
>> +	0x02,       /* idle threshold HC   (value * 300ms) */
>> +	0x01,       /* BT_WAKE active mode - 1=active high, 0 = active low */
>> +	0x00,       /* HOST_WAKE active mode - 1=active high, 0 = active low */
>> +	0x01,       /* Allow host sleep during SCO - FALSE */
>> +	0x01,       /* combine sleep mode and LPM - 1 == TRUE */
>> +	0x00,       /* enable tristate control of UART TX line - FALSE */
>> +	0x00,       /* USB auto-sleep on USB SUSPEND */
>> +	0x00,       /* USB USB RESUME timeout (seconds) */
>> +	0x00,       /* Pulsed Host Wake */
>> +	0x00        /* Enable Break To Host */
>> +	};
> 
> Unless these are all single byte values, this is not endian safe. And it is horrible to read actually. Lets add the proper structs to btbcm.h like Fred did for the other commands.
> 
> And in addition it might be good to add these structs in a separate patch.
> 
> IF: These are all single byte values so it is safe for any architecture. Okay, I'll add separate structures since you wish so. I don't understand the need for a separate patch though as the sleep configuration is a primary objective of this driver.

Separate patch makes my life easier to review. Keep in mind that a separate patch with a clean commit explain what things are is also easy to apply. Either I wait until I can apply all patches or I cherry-pick ones that are already fine on its own. Less re-basing for you if we can get things merged quickly.

> 
>> +	unsigned char pcm_int_pars[] = {
>> +	0x00,       /* 0=PCM routing, 1=SCO over HCI */
>> +	0x02,       /* 0=128Kbps,1=256Kbps,2=512Kbps,3=1024Kbps,4=2048Kbps */
>> +	0x00,       /* short frame sync  0=short, 1=long */
>> +	0x00,       /* sync mode         0=slave, 1=master */
>> +	0x00        /* clock mode        0=slave, 1=master */
>> +	};
>> +	unsigned char pcm_format_pars[] = {
>> +	0x00,       /* LSB_First 1=TRUE, 0=FALSE */
>> +	0x00,       /* Fill_Value (use 0-7 for fill bits value) */
>> +	0x02,       /* Fill_Method   (2=sign extended) */
>> +	0x03,       /* Fill_Num      # of fill bits 0-3) */
>> +	0x01        /* Right_Justify 1=TRUE, 0=FALSE */
>> +	};
>> +	unsigned char time_slot_number = 0;
>> +
>> +	BT_DBG("bcm_setup hu %p", hu);
>> 
>> 	hu->hdev->set_bdaddr = btbcm_set_bdaddr;
>> 
>> @@ -162,6 +460,67 @@ static int bcm_setup(struct hci_uart *hu)
>> 			hci_uart_set_baudrate(hu, hu->proto->oper_speed);
>> 	}
>> 
>> +	/* Configure SCO PCM parameters */
>> +	if (h4->pars.configure_audio) {
>> +		pcm_int_pars[0] = h4->pars.pcm_routing;
>> +		pcm_int_pars[1] = h4->pars.pcm_incallbitclock;
>> +		pcm_int_pars[2] = h4->pars.pcm_shortframesync;
>> +		pcm_int_pars[3] = h4->pars.pcm_syncmode;
>> +		pcm_int_pars[4] = h4->pars.pcm_clockmode;
> 
> I prefer if vendor drivers add comments above their vendor commands explaining what they do and way. See the Intel driver on how far we went to help others understand what is going here.
> 
> IF: I will add comments here.
> 
>> +		skb = __hci_cmd_sync(hu->hdev, 0xfc1c, sizeof(pcm_int_pars),
>> +				     pcm_int_pars, HCI_INIT_TIMEOUT);
>> +		if (IS_ERR(skb)) {
>> +			err = PTR_ERR(skb);
>> +			BT_ERR("bcm_setup pcm_ INT VSC failed (%d)", err);
>> +			goto finalize;
>> +		}
>> +		kfree_skb(skb);
>> +		BT_DBG("bcm_setup pcm_ INT Parameters VSC succeeded");
>> +
>> +		pcm_format_pars[0] = h4->pars.pcm_lsbfirst;
>> +		pcm_format_pars[1] = h4->pars.pcm_fillvalue;
>> +		pcm_format_pars[2] = h4->pars.pcm_fillmethod;
>> +		pcm_format_pars[3] = h4->pars.pcm_fillnum;
>> +		pcm_format_pars[4] = h4->pars.pcm_rightjustify;
>> +		skb = __hci_cmd_sync(hu->hdev, 0xfc1e, sizeof(pcm_format_pars),
>> +				     pcm_format_pars, HCI_INIT_TIMEOUT);
>> +		if (IS_ERR(skb)) {
>> +			err = PTR_ERR(skb);
>> +			BT_ERR("bcm_setup pcm_ Format VSC failed (%d)",
>> +			       err);
>> +			goto finalize;
>> +		}
>> +		kfree_skb(skb);
>> +		BT_DBG("bcm_setup pcm_ Format VSC succeeded");
>> +
>> +		skb = __hci_cmd_sync(hu->hdev, 0xfc22, sizeof(time_slot_number),
>> +				     &time_slot_number, HCI_INIT_TIMEOUT);
>> +		if (IS_ERR(skb)) {
>> +			err = PTR_ERR(skb);
>> +			BT_ERR("bcm_setup SCO Time Slot VSC failed (%d)",
>> +			       err);
>> +			goto finalize;
>> +		}
>> +		kfree_skb(skb);
>> +		BT_DBG("bcm_setup SCO Time Slot VSC succeeded");
>> +	}
>> +
>> +	/* Configure device's suspend/resume operation */
>> +	if (h4->pars.configure_sleep) {
>> +		/* Override the default */
>> +		sleep_pars[3] = (unsigned char)!h4->pars.bt_wake_active_low;
>> +		sleep_pars[4] = (unsigned char)!h4->pars.dev_wake_active_low;
>> +		skb = __hci_cmd_sync(hu->hdev, 0xfc27, sizeof(sleep_pars),
>> +				     sleep_pars, HCI_INIT_TIMEOUT);
>> +		if (IS_ERR(skb)) {
>> +			err = PTR_ERR(skb);
>> +			BT_ERR("bcm_setup Sleep VSC failed (%d)", err);
>> +			goto finalize;
>> +		}
>> +		kfree_skb(skb);
>> +		BT_DBG("bcm_setup Set Sleep Parameters VSC succeeded");
>> +	}
>> +
>> finalize:
>> 	release_firmware(fw);
>> 
>> @@ -183,6 +542,11 @@ static int bcm_recv(struct hci_uart *hu, const void *data, int count)
>> 	if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
>> 		return -EUNATCH;
>> 
>> +	BT_DBG("bcm_recv %d bytes", count);
> 
> I would prefer not to have this BT_DBG here. This function will be called a lot.
> 
> IF: This has been very useful to me to see whether anything has been received or not at the driver setup time. How would you want me to learn that?

Because it will kept being call during every single operation. Including A2DP traffic. I just think that for a final patch that goes upstream this should not be there. We did not do it for the other drivers either. Or did we?

> 
>> +
>> +	/* Make sure we're resumed */
>> +	bcm_ensure_wakeup(hu);
>> +
>> 	bcm->rx_skb = h4_recv_buf(hu->hdev, bcm->rx_skb, data, count,
>> 				  bcm_recv_pkts, ARRAY_SIZE(bcm_recv_pkts));
>> 	if (IS_ERR(bcm->rx_skb)) {
>> @@ -198,7 +562,10 @@ static int bcm_enqueue(struct hci_uart *hu, struct sk_buff *skb)
>> {
>> 	struct bcm_data *bcm = hu->priv;
>> 
>> -	BT_DBG("hu %p skb %p", hu, skb);
>> +	BT_DBG("bcm_enqueue hu %p skb %p", hu, skb);
> 
> This is again that the function name is available with dynamic debug. So please do not change these or send separate patches with a commit message explaining why this change makes sense.
> 
> IF: Okay, will remove although dynamic debug is not available on all platforms.
> 
>> +
>> +	/* Make sure we're resumed */
>> +	bcm_ensure_wakeup(hu);
>> 
>> 	/* Prepend skb with frame type */
>> 	memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
>> @@ -214,7 +581,8 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
>> 	return skb_dequeue(&bcm->txq);
>> }
>> 
>> -static const struct hci_uart_proto bcm_proto = {
>> +/* This structure may not be const as speed may change at runtime */
> 
> No. This struct is const. These are the value that a protocol driver sets.
> 
> If values are dynamic and change based on DT, then left them zero here and provide a method for overwriting them internally in hci_uart. Please do not hack this in. There are simple UART drivers that we want to enable in a really simple way.
> 
> IF: As I said above, I've inherited the need to change the speed(s) from the recent Intel patches. Will try fixing that.

I get where you are coming from. And for Broadcom the table will not contain an oper-speed. That will come from ACPI or DT. So you action should be to remove it here and introduce a variable in bcm_data that has it and that can be used instead. I have not figured out all the details, but that is the general idea. I leave the details up to you.

Regards

Marcel

RE: [PATCH v2 5/5] BlueZ Broadcom UART Protocol

[Ilya Faenson]

Hi Marcel,

-----Original Message-----
From: Marcel Holtmann [mailto:marcel@holtmann.org]=20
Sent: Saturday, June 13, 2015 3:54 AM
To: Ilya Faenson
Cc: linux-bluetooth@vger.kernel.org; Arend Van Spriel
Subject: Re: [PATCH v2 5/5] BlueZ Broadcom UART Protocol

Hi Ilya,

>> Merge Broadcom protocol with the existing implementation, providing
>> for UART setup, firmware download and power management.
>>=20
>> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
>> ---
>> drivers/bluetooth/hci_bcm.c | 400 ++++++++++++++++++++++++++++++++++++++=
++++--
>> 1 file changed, 384 insertions(+), 16 deletions(-)
>>=20
>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>> index e4d66b6..5ff35b7 100644
>> --- a/drivers/bluetooth/hci_bcm.c
>> +++ b/drivers/bluetooth/hci_bcm.c
>> @@ -3,6 +3,7 @@
>> *  Bluetooth HCI UART driver for Broadcom devices
>> *
>> *  Copyright (C) 2015  Intel Corporation
>> + *  Copyright (C) 2015  Broadcom Corporation
>> *
>> *
>> *  This program is free software; you can redistribute it and/or modify
>> @@ -24,6 +25,7 @@
>> #include <linux/kernel.h>
>> #include <linux/errno.h>
>> #include <linux/skbuff.h>
>> +#include <linux/tty.h>
>> #include <linux/firmware.h>
>>=20
>> #include <net/bluetooth/bluetooth.h>
>> @@ -31,12 +33,170 @@
>>=20
>> #include "btbcm.h"
>> #include "hci_uart.h"
>> +#include "btbcm_uart.h"
>>=20
>> struct bcm_data {
>> 	struct sk_buff *rx_skb;
>> 	struct sk_buff_head txq;
>> +	struct hci_uart *hu;
>> +
>> +	bool is_suspended; /* suspend/resume flag */
>> +
>> +	struct timer_list timer; /* idle timer */
>=20
> I prefer not to use timers if we can use delayed work instead.
>=20
> IF: Could you clarify how you want me to use the delayed work for the fre=
quently refreshed 5 second timer? It would essentially be an extra thread r=
unning most of the time with us having to synchronize it with the device st=
ops etc. Clear invite for race conditions in my opinion.

refreshed means re-armed? I wonder how delayed work is different then? Mayb=
e I need to understand a bit more than first.

IF: Every time a new packet comes from either direction, the idle timer nee=
ds to be re-armed to fire in 5 seconds regardless of the current state of t=
his timer. Hope that clarifies the logic.

>=20
>> +
>> +	struct btbcm_uart_parameters pars; /* device parameters */
>=20
> btbcm_uart_params and params please. No pars and not point in spelling ou=
t parameters in a struct name.
>=20
> IF: Okay.
>=20
>> +	void *device_context; /* ACPI/DT device context */
>> };
>>=20
>> +/* Suspend/resume synchronization mutex */
>> +static DEFINE_MUTEX(plock);
>=20
> Please use a more descriptive name than plock. That is too generic for a =
global variable.
>=20
> IF: Okay.
>=20
>> +
>> +/* Forward reference */
>> +static struct hci_uart_proto bcm_proto;
>=20
> Why do we need this? I am generally not in favor of having forward refere=
nce until really really needed.
>=20
> IF: That is needed to set the oper-speed in this structure once the devic=
e parameters are available. Intel patch has hard-coded it to a single value=
 for all Broadcom devices which is not acceptable.

So for Broadcom devices, I think in the static table, no oper-speed should =
be set. That means it has to come by other means. Either ACPI or DT. But as=
 I mentioned in the other part, that table has to stay const.

IF: The oper_speed comes from DT in my current code already. The BlueZ line=
 discipline currently acts on the oper_speed for all manufacturers. Do you =
want an exception for Broadcom there? If not, the oper-speed changes should=
 apply to everybody.

>=20
>> +
>> +/*
>> + * Callbacks from the BCMBT_UART device
>> + */
>> +
>> +/*
>> + * The platform is suspending. Stop UART activity
>> + */
>> +static void suspend_notification(void *context)
>> +{
>> +	struct hci_uart *hu =3D (struct hci_uart *)context;
>=20
> context is void * and so no need to cast.
>=20
> IF: Okay.
>=20
>> +	struct bcm_data *h4 =3D hu->priv;
>> +
>> +	BT_DBG("%s: is_suspended %d", __func__, h4->is_suspended);
>> +
>> +	if (!h4->pars.configure_sleep)
>> +		return;
>> +
>> +	if (!h4->is_suspended) {
>> +		if (h4->pars.manual_fc)
>> +			hci_uart_set_flow_control(hu, true);
>> +
>> +		/* Once this callback returns, driver suspends BT via GPIO */
>> +		h4->is_suspended =3D true;
>> +	}
>> +}
>> +
>> +/*
>> + * The platform is resuming. Resume UART activity.
>> + */
>> +static void resume_notification(void *context)
>> +{
>> +	struct hci_uart *hu =3D (struct hci_uart *)context;
>> +	struct bcm_data *h4 =3D hu->priv;
>> +
>> +	BT_DBG("%s: is_suspended %d", __func__, h4->is_suspended);
>> +
>> +	if (!h4->pars.configure_sleep)
>> +		return;
>> +
>> +	/* When this callback executes, the device has woken up already */
>> +	if (h4->is_suspended) {
>> +		h4->is_suspended =3D false;
>> +
>> +		if (h4->pars.manual_fc)
>> +			hci_uart_set_flow_control(hu, false);
>> +	}
>> +
>> +	/* If we're resumed, the idle timer must be running */
>> +	mod_timer(&h4->timer, jiffies +
>> +	    msecs_to_jiffies(h4->pars.idle_timeout_in_secs * 1000));
>=20
> Indention is violated here.
>=20
> IF: Okay, will change.
>=20
>> +}
>> +
>> +/*
>> + * The BT device is resuming. Resume UART activity if suspended
>> + */
>> +static void wakeup_notification(void *context)
>> +{
>> +	struct hci_uart *hu =3D (struct hci_uart *)context;
>> +	struct bcm_data *h4 =3D hu->priv;
>> +
>> +	BT_DBG("%s: is_suspended %d", __func__, h4->is_suspended);
>> +
>> +	if (!h4->pars.configure_sleep)
>> +		return;
>> +
>> +	if (h4->is_suspended) {
>> +		if (h4->pars.manual_fc)
>> +			hci_uart_set_flow_control(hu, false);
>> +
>> +		h4->is_suspended =3D false;
>> +	}
>> +
>> +	/* If we're resumed, the idle timer must be running */
>> +	mod_timer(&h4->timer, jiffies + msecs_to_jiffies(
>> +			   h4->pars.idle_timeout_in_secs * 1000));
>=20
> Same here actually and worse it is different from the other one.
>=20
> Splitting the rearming into its own helper might help here.
>=20
> IF: Okay, will do.
>=20
>> +}
>> +
>> +/*
>> + * Make sure we're awake
>> + * (called when the resumed state is required)
>> + */
>> +static void bcm_ensure_wakeup(struct hci_uart *hu)
>> +{
>> +	struct bcm_data *h4 =3D hu->priv;
>> +	int status;
>> +
>> +	if (!h4->pars.configure_sleep)
>> +		return;
>> +
>> +	/* Suspend/resume operations are serialized */
>> +	mutex_lock(&plock);
>> +
>> +	/* Nothing to do if resumed already */
>> +	if (!h4->is_suspended) {
>> +		mutex_unlock(&plock);
>> +
>> +		/* Just reset the timer */
>> +		status =3D mod_timer(&h4->timer, jiffies + msecs_to_jiffies(
>> +				   h4->pars.idle_timeout_in_secs * 1000));
>=20
> This indentation is still not correct and now we have a 3rd version ;)
>=20
> IF: Will change.
>=20
>> +		return;
>> +	}
>> +
>> +	/* Wakeup the device */
>> +	status =3D btbcm_uart_control(BTBCM_UART_ACTION_RESUME,
>> +				    h4->device_context, NULL, NULL);
>> +	if (status)
>> +		BT_DBG("%s: failed to resume driver %d", __func__, status);
>=20
> Conditional BT_DBG on the status is pretty useless in my mind. Either BT_=
DBG the status or don't at all.
>=20
> IF: No trace upon success sounds reasonable to me but I'll change it as p=
er your preferences.

They are debug entries and in all the other code we have used them mainly u=
nconditional. I am even fine if you just use BT_ERR here since something re=
ally went wrong. Just a conditional debug seems odd.

IF: Understood, thanks for the clarifications.

>=20
>> +
>> +	/* Unflow control the port if configured */
>> +	resume_notification(hu);
>> +
>> +	mutex_unlock(&plock);
>> +}
>> +
>> +/*
>> + * Idle timer callback
>> + */
>> +static void bcm_idle_timeout(unsigned long arg)
>> +{
>> +	struct hci_uart *hu =3D (struct hci_uart *)arg;
>> +	struct bcm_data *h4 =3D hu->priv;
>> +	int status;
>> +
>> +	BT_DBG("%s: hu %p", __func__, hu);
>> +
>> +	/* Suspend/resume operations are serialized */
>> +	mutex_lock(&plock);
>> +
>> +	if (!h4->is_suspended) {
>> +		/* Flow control the port if configured */
>> +		suspend_notification(hu);
>> +
>> +		/* Suspend the device */
>> +		status =3D btbcm_uart_control(BTBCM_UART_ACTION_SUSPEND,
>> +					    h4->device_context, NULL, NULL);
>> +		if (status)
>> +			BT_DBG("%s: failed to suspend device %d", __func__,
>> +			       status);
>> +	}
>> +
>> +	mutex_unlock(&plock);
>> +}
>> +
>> static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)
>> {
>> 	struct hci_dev *hdev =3D hu->hdev;
>> @@ -88,31 +248,138 @@ static int bcm_set_baudrate(struct hci_uart *hu, u=
nsigned int speed)
>>=20
>> static int bcm_open(struct hci_uart *hu)
>> {
>> -	struct bcm_data *bcm;
>> +	struct btbcm_uart_callbacks callbacks;
>> +	unsigned long callbacks_size =3D sizeof(callbacks);
>=20
> Not needed since it is all inside the kernel. And I think once this is sp=
lit into individual exported functions, this will become a lot clearer.
>=20
> IF: Okay, will remove.
>=20
>> +	int status;
>> +	struct bcm_data *h4;
>> +	struct tty_struct *tty =3D hu->tty;
>>=20
>> -	BT_DBG("hu %p", hu);
>> +	BT_DBG("bcm_open hu %p", hu);
>>=20
>> -	bcm =3D kzalloc(sizeof(*bcm), GFP_KERNEL);
>> -	if (!bcm)
>> +	h4 =3D kzalloc(sizeof(*h4), GFP_KERNEL);
>> +	if (!h4)
>> 		return -ENOMEM;
>=20
> If we are doing renaming of variables, then I am expecting a cleanup patc=
h first that does only that. If this is needed at all or just some left-ove=
r merge issue.
>=20
> IF: This is indeed a leftover merge issue. Will restore the original.
>=20
>>=20
>> -	skb_queue_head_init(&bcm->txq);
>> +	skb_queue_head_init(&h4->txq);
>> +	hu->priv =3D h4;
>> +	h4->hu =3D hu;
>> +	h4->is_suspended =3D false;
>> +
>> +	/* Configure callbacks on the driver */
>> +	callbacks.interface_version =3D BTBCM_UART_INTERFACE_VERSION;
>> +	callbacks.context =3D hu;
>> +	strcpy(callbacks.name, tty->name);
>> +	callbacks.p_suspend =3D suspend_notification;
>> +	callbacks.p_resume =3D resume_notification;
>> +	callbacks.p_wakeup =3D wakeup_notification;
>> +	status =3D btbcm_uart_control(BTBCM_UART_ACTION_CONFIGURE_CALLBACKS,
>> +				    NULL, &callbacks, &callbacks_size);
>> +	if (status) {
>> +		BT_DBG("bcm_open failed to set driver callbacks %d", status);
>> +		return status;
>> +	}
>> +	if (callbacks_size !=3D sizeof(void *)) {
>> +		BT_DBG("bcm_open got back %d bytes from callbacks?!",
>> +		       (int)callbacks_size);
>> +		return -EMSGSIZE;
>> +	}
>> +	memcpy(&h4->device_context, &callbacks, sizeof(void *));
>> +	BT_DBG("bcm_open callbacks context %p", h4->device_context);
>> +
>> +	/* Retrieve device parameters */
>> +	callbacks_size =3D sizeof(h4->pars);
>> +	status =3D btbcm_uart_control(BTBCM_UART_ACTION_GET_PARAMETERS,
>> +				    h4->device_context, &h4->pars,
>> +				    &callbacks_size);
>> +	if (status) {
>> +		BT_DBG("bcm_open failed to get dev parameters %d", status);
>> +		return status;
>> +	}
>> +	BT_DBG("Pars ver %d csleep %d dalow %d balow %d idle %d",
>> +	       h4->pars.interface_version, h4->pars.configure_sleep,
>> +	       h4->pars.dev_wake_active_low, h4->pars.bt_wake_active_low,
>> +	       h4->pars.idle_timeout_in_secs);
>> +	bcm_proto.oper_speed =3D h4->pars.oper_speed;
>> +
>> +	/* Cycle power to make sure the device is in the known state */
>> +	status =3D btbcm_uart_control(BTBCM_UART_ACTION_POWER_OFF,
>> +				    h4->device_context, NULL, NULL);
>> +	if (status) {
>> +		BT_DBG("bcm_open failed to power off %d", status);
>> +	} else {
>> +		status =3D btbcm_uart_control(BTBCM_UART_ACTION_POWER_ON,
>> +					    h4->device_context, NULL, NULL);
>> +		if (status)
>> +			BT_DBG("bcm_open failed to power on %d", status);
>> +	}
>> +
>> +	/* Start the idle timer */
>> +	if (h4->pars.configure_sleep) {
>> +		setup_timer(&h4->timer, bcm_idle_timeout, (unsigned long)hu);
>> +		if (h4->pars.configure_sleep)
>> +			mod_timer(&h4->timer, jiffies + msecs_to_jiffies(
>> +				  h4->pars.idle_timeout_in_secs * 1000));
>> +	}
>>=20
>> -	hu->priv =3D bcm;
>> 	return 0;
>> }
>>=20
>> static int bcm_close(struct hci_uart *hu)
>> {
>> -	struct bcm_data *bcm =3D hu->priv;
>> +	struct btbcm_uart_callbacks callbacks;
>> +	unsigned long callbacks_size =3D sizeof(callbacks);
>> +	struct bcm_data *h4 =3D hu->priv;
>> +	int status;
>>=20
>> -	BT_DBG("hu %p", hu);
>> +	hu->priv =3D NULL;
>>=20
>> -	skb_queue_purge(&bcm->txq);
>> -	kfree_skb(bcm->rx_skb);
>> -	kfree(bcm);
>> +	BT_DBG("bcm_close hu %p", hu);
>> +
>> +	/* If we're being closed, we must suspend */
>> +	if (h4->pars.configure_sleep) {
>> +		mutex_lock(&plock);
>> +
>> +		if (!h4->is_suspended) {
>> +			/* Flow control the port */
>> +			suspend_notification(hu);
>> +
>> +			/* Suspend the device */
>> +			status =3D btbcm_uart_control(BTBCM_UART_ACTION_SUSPEND,
>> +						    h4->device_context, NULL,
>> +						    NULL);
>> +			if (status) {
>> +				BT_DBG("bcm_close suspend driver fail %d",
>> +				       status);
>> +			}
>> +		}
>> +
>> +	mutex_unlock(&plock);
>> +
>> +	del_timer_sync(&h4->timer);
>> +	}
>> +
>> +	/* Power off the device if possible */
>> +	status =3D btbcm_uart_control(BTBCM_UART_ACTION_POWER_OFF,
>> +				    h4->device_context, NULL, NULL);
>> +	if (status)
>> +		BT_DBG("bcm_close failed to power off %d", status);
>> +
>> +	/* de-configure callbacks on the driver */
>> +	callbacks.interface_version =3D BTBCM_UART_INTERFACE_VERSION;
>> +	callbacks.context =3D hu;
>> +	callbacks.p_suspend =3D NULL;
>> +	callbacks.p_resume =3D NULL;
>> +	callbacks.p_wakeup =3D NULL;
>> +	status =3D btbcm_uart_control(BTBCM_UART_ACTION_CONFIGURE_CALLBACKS,
>> +				    h4->device_context, &callbacks,
>> +				    &callbacks_size);
>> +	if (status)
>> +		BT_DBG("bcm_close failed to reset drv callbacks %d", status);
>> +	skb_queue_purge(&h4->txq);
>>=20
>> 	hu->priv =3D NULL;
>> +	kfree(h4);
>> +
>> 	return 0;
>> }
>>=20
>> @@ -129,11 +396,42 @@ static int bcm_flush(struct hci_uart *hu)
>>=20
>> static int bcm_setup(struct hci_uart *hu)
>> {
>> -	char fw_name[64];
>> +	char fw_name[128];
>> 	const struct firmware *fw;
>> 	int err;
>> -
>> -	BT_DBG("hu %p", hu);
>> +	struct sk_buff *skb;
>> +	struct bcm_data *h4 =3D hu->priv;
>> +	unsigned char sleep_pars[] =3D {
>> +	0x01,       /* sleep mode 1=3DUART */
>> +	0x02,       /* idle threshold HOST (value * 300ms) */
>> +	0x02,       /* idle threshold HC   (value * 300ms) */
>> +	0x01,       /* BT_WAKE active mode - 1=3Dactive high, 0 =3D active low=
 */
>> +	0x00,       /* HOST_WAKE active mode - 1=3Dactive high, 0 =3D active l=
ow */
>> +	0x01,       /* Allow host sleep during SCO - FALSE */
>> +	0x01,       /* combine sleep mode and LPM - 1 =3D=3D TRUE */
>> +	0x00,       /* enable tristate control of UART TX line - FALSE */
>> +	0x00,       /* USB auto-sleep on USB SUSPEND */
>> +	0x00,       /* USB USB RESUME timeout (seconds) */
>> +	0x00,       /* Pulsed Host Wake */
>> +	0x00        /* Enable Break To Host */
>> +	};
>=20
> Unless these are all single byte values, this is not endian safe. And it =
is horrible to read actually. Lets add the proper structs to btbcm.h like F=
red did for the other commands.
>=20
> And in addition it might be good to add these structs in a separate patch=
.
>=20
> IF: These are all single byte values so it is safe for any architecture. =
Okay, I'll add separate structures since you wish so. I don't understand th=
e need for a separate patch though as the sleep configuration is a primary =
objective of this driver.

Separate patch makes my life easier to review. Keep in mind that a separate=
 patch with a clean commit explain what things are is also easy to apply. E=
ither I wait until I can apply all patches or I cherry-pick ones that are a=
lready fine on its own. Less re-basing for you if we can get things merged =
quickly.

IF: Understood, will create a separate patch.

>=20
>> +	unsigned char pcm_int_pars[] =3D {
>> +	0x00,       /* 0=3DPCM routing, 1=3DSCO over HCI */
>> +	0x02,       /* 0=3D128Kbps,1=3D256Kbps,2=3D512Kbps,3=3D1024Kbps,4=3D20=
48Kbps */
>> +	0x00,       /* short frame sync  0=3Dshort, 1=3Dlong */
>> +	0x00,       /* sync mode         0=3Dslave, 1=3Dmaster */
>> +	0x00        /* clock mode        0=3Dslave, 1=3Dmaster */
>> +	};
>> +	unsigned char pcm_format_pars[] =3D {
>> +	0x00,       /* LSB_First 1=3DTRUE, 0=3DFALSE */
>> +	0x00,       /* Fill_Value (use 0-7 for fill bits value) */
>> +	0x02,       /* Fill_Method   (2=3Dsign extended) */
>> +	0x03,       /* Fill_Num      # of fill bits 0-3) */
>> +	0x01        /* Right_Justify 1=3DTRUE, 0=3DFALSE */
>> +	};
>> +	unsigned char time_slot_number =3D 0;
>> +
>> +	BT_DBG("bcm_setup hu %p", hu);
>>=20
>> 	hu->hdev->set_bdaddr =3D btbcm_set_bdaddr;
>>=20
>> @@ -162,6 +460,67 @@ static int bcm_setup(struct hci_uart *hu)
>> 			hci_uart_set_baudrate(hu, hu->proto->oper_speed);
>> 	}
>>=20
>> +	/* Configure SCO PCM parameters */
>> +	if (h4->pars.configure_audio) {
>> +		pcm_int_pars[0] =3D h4->pars.pcm_routing;
>> +		pcm_int_pars[1] =3D h4->pars.pcm_incallbitclock;
>> +		pcm_int_pars[2] =3D h4->pars.pcm_shortframesync;
>> +		pcm_int_pars[3] =3D h4->pars.pcm_syncmode;
>> +		pcm_int_pars[4] =3D h4->pars.pcm_clockmode;
>=20
> I prefer if vendor drivers add comments above their vendor commands expla=
ining what they do and way. See the Intel driver on how far we went to help=
 others understand what is going here.
>=20
> IF: I will add comments here.
>=20
>> +		skb =3D __hci_cmd_sync(hu->hdev, 0xfc1c, sizeof(pcm_int_pars),
>> +				     pcm_int_pars, HCI_INIT_TIMEOUT);
>> +		if (IS_ERR(skb)) {
>> +			err =3D PTR_ERR(skb);
>> +			BT_ERR("bcm_setup pcm_ INT VSC failed (%d)", err);
>> +			goto finalize;
>> +		}
>> +		kfree_skb(skb);
>> +		BT_DBG("bcm_setup pcm_ INT Parameters VSC succeeded");
>> +
>> +		pcm_format_pars[0] =3D h4->pars.pcm_lsbfirst;
>> +		pcm_format_pars[1] =3D h4->pars.pcm_fillvalue;
>> +		pcm_format_pars[2] =3D h4->pars.pcm_fillmethod;
>> +		pcm_format_pars[3] =3D h4->pars.pcm_fillnum;
>> +		pcm_format_pars[4] =3D h4->pars.pcm_rightjustify;
>> +		skb =3D __hci_cmd_sync(hu->hdev, 0xfc1e, sizeof(pcm_format_pars),
>> +				     pcm_format_pars, HCI_INIT_TIMEOUT);
>> +		if (IS_ERR(skb)) {
>> +			err =3D PTR_ERR(skb);
>> +			BT_ERR("bcm_setup pcm_ Format VSC failed (%d)",
>> +			       err);
>> +			goto finalize;
>> +		}
>> +		kfree_skb(skb);
>> +		BT_DBG("bcm_setup pcm_ Format VSC succeeded");
>> +
>> +		skb =3D __hci_cmd_sync(hu->hdev, 0xfc22, sizeof(time_slot_number),
>> +				     &time_slot_number, HCI_INIT_TIMEOUT);
>> +		if (IS_ERR(skb)) {
>> +			err =3D PTR_ERR(skb);
>> +			BT_ERR("bcm_setup SCO Time Slot VSC failed (%d)",
>> +			       err);
>> +			goto finalize;
>> +		}
>> +		kfree_skb(skb);
>> +		BT_DBG("bcm_setup SCO Time Slot VSC succeeded");
>> +	}
>> +
>> +	/* Configure device's suspend/resume operation */
>> +	if (h4->pars.configure_sleep) {
>> +		/* Override the default */
>> +		sleep_pars[3] =3D (unsigned char)!h4->pars.bt_wake_active_low;
>> +		sleep_pars[4] =3D (unsigned char)!h4->pars.dev_wake_active_low;
>> +		skb =3D __hci_cmd_sync(hu->hdev, 0xfc27, sizeof(sleep_pars),
>> +				     sleep_pars, HCI_INIT_TIMEOUT);
>> +		if (IS_ERR(skb)) {
>> +			err =3D PTR_ERR(skb);
>> +			BT_ERR("bcm_setup Sleep VSC failed (%d)", err);
>> +			goto finalize;
>> +		}
>> +		kfree_skb(skb);
>> +		BT_DBG("bcm_setup Set Sleep Parameters VSC succeeded");
>> +	}
>> +
>> finalize:
>> 	release_firmware(fw);
>>=20
>> @@ -183,6 +542,11 @@ static int bcm_recv(struct hci_uart *hu, const void=
 *data, int count)
>> 	if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
>> 		return -EUNATCH;
>>=20
>> +	BT_DBG("bcm_recv %d bytes", count);
>=20
> I would prefer not to have this BT_DBG here. This function will be called=
 a lot.
>=20
> IF: This has been very useful to me to see whether anything has been rece=
ived or not at the driver setup time. How would you want me to learn that?

Because it will kept being call during every single operation. Including A2=
DP traffic. I just think that for a final patch that goes upstream this sho=
uld not be there. We did not do it for the other drivers either. Or did we?

IF: That's right: drivers for other manufacturers do not have that. I will =
remove it.

>=20
>> +
>> +	/* Make sure we're resumed */
>> +	bcm_ensure_wakeup(hu);
>> +
>> 	bcm->rx_skb =3D h4_recv_buf(hu->hdev, bcm->rx_skb, data, count,
>> 				  bcm_recv_pkts, ARRAY_SIZE(bcm_recv_pkts));
>> 	if (IS_ERR(bcm->rx_skb)) {
>> @@ -198,7 +562,10 @@ static int bcm_enqueue(struct hci_uart *hu, struct =
sk_buff *skb)
>> {
>> 	struct bcm_data *bcm =3D hu->priv;
>>=20
>> -	BT_DBG("hu %p skb %p", hu, skb);
>> +	BT_DBG("bcm_enqueue hu %p skb %p", hu, skb);
>=20
> This is again that the function name is available with dynamic debug. So =
please do not change these or send separate patches with a commit message e=
xplaining why this change makes sense.
>=20
> IF: Okay, will remove although dynamic debug is not available on all plat=
forms.
>=20
>> +
>> +	/* Make sure we're resumed */
>> +	bcm_ensure_wakeup(hu);
>>=20
>> 	/* Prepend skb with frame type */
>> 	memcpy(skb_push(skb, 1), &bt_cb(skb)->pkt_type, 1);
>> @@ -214,7 +581,8 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *=
hu)
>> 	return skb_dequeue(&bcm->txq);
>> }
>>=20
>> -static const struct hci_uart_proto bcm_proto =3D {
>> +/* This structure may not be const as speed may change at runtime */
>=20
> No. This struct is const. These are the value that a protocol driver sets=
.
>=20
> If values are dynamic and change based on DT, then left them zero here an=
d provide a method for overwriting them internally in hci_uart. Please do n=
ot hack this in. There are simple UART drivers that we want to enable in a =
really simple way.
>=20
> IF: As I said above, I've inherited the need to change the speed(s) from =
the recent Intel patches. Will try fixing that.

I get where you are coming from. And for Broadcom the table will not contai=
n an oper-speed. That will come from ACPI or DT. So you action should be to=
 remove it here and introduce a variable in bcm_data that has it and that c=
an be used instead. I have not figured out all the details, but that is the=
 general idea. I leave the details up to you.

IF: As I said above, this is slightly more complicated as the oper_speed is=
 used by the BlueZ line discipline for all manufacturers. And any manufactu=
rer with the oper-speed changeable among the devices will be affected. Sinc=
e the oper-speed used is largely defined by the UART rather than BT device =
capabilities, I'm afraid it should be configurable and not reside in a cons=
t structure for all.

Regards

Marcel

Re: [PATCH v2 5/5] BlueZ Broadcom UART Protocol

[Frederic Danis]

Hello Marcel and Ilya,

On 13/06/2015 16:10, Ilya Faenson wrote:
> Hi Marcel,
>
> -----Original Message-----
> From: Marcel Holtmann [mailto:marcel@holtmann.org]
> Sent: Saturday, June 13, 2015 3:54 AM
> To: Ilya Faenson
> Cc: linux-bluetooth@vger.kernel.org; Arend Van Spriel
> Subject: Re: [PATCH v2 5/5] BlueZ Broadcom UART Protocol
>
> Hi Ilya,
>
> >> Merge Broadcom protocol with the existing implementation, providing
> >> for UART setup, firmware download and power management.
> >>
> >> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
> >> ---
> >> drivers/bluetooth/hci_bcm.c | 400 
> ++++++++++++++++++++++++++++++++++++++++++--
> >> 1 file changed, 384 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> >> index e4d66b6..5ff35b7 100644
> >> --- a/drivers/bluetooth/hci_bcm.c
> >> +++ b/drivers/bluetooth/hci_bcm.c
> >> @@ -3,6 +3,7 @@
> >> *  Bluetooth HCI UART driver for Broadcom devices
> >> *
> >> *  Copyright (C) 2015  Intel Corporation
> >> + *  Copyright (C) 2015  Broadcom Corporation
> >> *
> >> *
> >> *  This program is free software; you can redistribute it and/or modify
> >> @@ -24,6 +25,7 @@
> >> #include <linux/kernel.h>
> >> #include <linux/errno.h>
> >> #include <linux/skbuff.h>
> >> +#include <linux/tty.h>
> >> #include <linux/firmware.h>
> >>
> >> #include <net/bluetooth/bluetooth.h>
> >> @@ -31,12 +33,170 @@
> >>
> >> #include "btbcm.h"
> >> #include "hci_uart.h"
> >> +#include "btbcm_uart.h"
> >>
> >> struct bcm_data {
> >>     struct sk_buff *rx_skb;
> >>     struct sk_buff_head txq;
> >> +    struct hci_uart *hu;
> >> +
> >> +    bool is_suspended; /* suspend/resume flag */
> >> +
> >> +    struct timer_list timer; /* idle timer */
> >
> > I prefer not to use timers if we can use delayed work instead.
> >
> > IF: Could you clarify how you want me to use the delayed work for 
> the frequently refreshed 5 second timer? It would essentially be an 
> extra thread running most of the time with us having to synchronize it 
> with the device stops etc. Clear invite for race conditions in my opinion.
>
> refreshed means re-armed? I wonder how delayed work is different then? 
> Maybe I need to understand a bit more than first.
>
> IF: Every time a new packet comes from either direction, the idle 
> timer needs to be re-armed to fire in 5 seconds regardless of the 
> current state of this timer. Hope that clarifies the logic.
>
> >
> >> +
> >> +    struct btbcm_uart_parameters pars; /* device parameters */
> >
> > btbcm_uart_params and params please. No pars and not point in 
> spelling out parameters in a struct name.
> >
> > IF: Okay.
> >
> >> +    void *device_context; /* ACPI/DT device context */
> >> };
> >>
> >> +/* Suspend/resume synchronization mutex */
> >> +static DEFINE_MUTEX(plock);
> >
> > Please use a more descriptive name than plock. That is too generic 
> for a global variable.
> >
> > IF: Okay.
> >
> >> +
> >> +/* Forward reference */
> >> +static struct hci_uart_proto bcm_proto;
> >
> > Why do we need this? I am generally not in favor of having forward 
> reference until really really needed.
> >
> > IF: That is needed to set the oper-speed in this structure once the 
> device parameters are available. Intel patch has hard-coded it to a 
> single value for all Broadcom devices which is not acceptable.
>
> So for Broadcom devices, I think in the static table, no oper-speed 
> should be set. That means it has to come by other means. Either ACPI 
> or DT. But as I mentioned in the other part, that table has to stay const.
>
> IF: The oper_speed comes from DT in my current code already. The BlueZ 
> line discipline currently acts on the oper_speed for all 
> manufacturers. Do you want an exception for Broadcom there? If not, 
> the oper-speed changes should apply to everybody.

<snip>
> >> -static const struct hci_uart_proto bcm_proto = {
> >> +/* This structure may not be const as speed may change at runtime */
> >
> > No. This struct is const. These are the value that a protocol driver 
> sets.
> >
> > If values are dynamic and change based on DT, then left them zero 
> here and provide a method for overwriting them internally in hci_uart. 
> Please do not hack this in. There are simple UART drivers that we want 
> to enable in a really simple way.
> >
> > IF: As I said above, I've inherited the need to change the speed(s) 
> from the recent Intel patches. Will try fixing that.
>
> I get where you are coming from. And for Broadcom the table will not 
> contain an oper-speed. That will come from ACPI or DT. So you action 
> should be to remove it here and introduce a variable in bcm_data that 
> has it and that can be used instead. I have not figured out all the 
> details, but that is the general idea. I leave the details up to you.
>
> IF: As I said above, this is slightly more complicated as the 
> oper_speed is used by the BlueZ line discipline for all manufacturers. 
> And any manufacturer with the oper-speed changeable among the devices 
> will be affected. Since the oper-speed used is largely defined by the 
> UART rather than BT device capabilities, I'm afraid it should be 
> configurable and not reside in a const structure for all.

FYI, the oper_speed is not available in ACPI table of T100.
Depending on device, even the init_speed may not be available.

So, I agree with Ilya these values should be configurable.

Regards

Fred

-- 
Frederic Danis Open Source Technology Center
frederic.danis@intel.com Intel Corporation

Re: [PATCH v2 5/5] BlueZ Broadcom UART Protocol

[Marcel Holtmann]

Hi Fred,

>> >> Merge Broadcom protocol with the existing implementation, providing
>> >> for UART setup, firmware download and power management.
>> >>
>> >> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
>> >> ---
>> >> drivers/bluetooth/hci_bcm.c | 400 ++++++++++++++++++++++++++++++++++++++++++--
>> >> 1 file changed, 384 insertions(+), 16 deletions(-)
>> >>
>> >> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>> >> index e4d66b6..5ff35b7 100644
>> >> --- a/drivers/bluetooth/hci_bcm.c
>> >> +++ b/drivers/bluetooth/hci_bcm.c
>> >> @@ -3,6 +3,7 @@
>> >> *  Bluetooth HCI UART driver for Broadcom devices
>> >> *
>> >> *  Copyright (C) 2015  Intel Corporation
>> >> + *  Copyright (C) 2015  Broadcom Corporation
>> >> *
>> >> *
>> >> *  This program is free software; you can redistribute it and/or modify
>> >> @@ -24,6 +25,7 @@
>> >> #include <linux/kernel.h>
>> >> #include <linux/errno.h>
>> >> #include <linux/skbuff.h>
>> >> +#include <linux/tty.h>
>> >> #include <linux/firmware.h>
>> >>
>> >> #include <net/bluetooth/bluetooth.h>
>> >> @@ -31,12 +33,170 @@
>> >>
>> >> #include "btbcm.h"
>> >> #include "hci_uart.h"
>> >> +#include "btbcm_uart.h"
>> >>
>> >> struct bcm_data {
>> >>     struct sk_buff *rx_skb;
>> >>     struct sk_buff_head txq;
>> >> +    struct hci_uart *hu;
>> >> +
>> >> +    bool is_suspended; /* suspend/resume flag */
>> >> +
>> >> +    struct timer_list timer; /* idle timer */
>> >
>> > I prefer not to use timers if we can use delayed work instead.
>> >
>> > IF: Could you clarify how you want me to use the delayed work for the frequently refreshed 5 second timer? It would essentially be an extra thread running most of the time with us having to synchronize it with the device stops etc. Clear invite for race conditions in my opinion.
>> 
>> refreshed means re-armed? I wonder how delayed work is different then? Maybe I need to understand a bit more than first.
>> 
>> IF: Every time a new packet comes from either direction, the idle timer needs to be re-armed to fire in 5 seconds regardless of the current state of this timer. Hope that clarifies the logic.
>> 
>> >
>> >> +
>> >> +    struct btbcm_uart_parameters pars; /* device parameters */
>> >
>> > btbcm_uart_params and params please. No pars and not point in spelling out parameters in a struct name.
>> >
>> > IF: Okay.
>> >
>> >> +    void *device_context; /* ACPI/DT device context */
>> >> };
>> >>
>> >> +/* Suspend/resume synchronization mutex */
>> >> +static DEFINE_MUTEX(plock);
>> >
>> > Please use a more descriptive name than plock. That is too generic for a global variable.
>> >
>> > IF: Okay.
>> >
>> >> +
>> >> +/* Forward reference */
>> >> +static struct hci_uart_proto bcm_proto;
>> >
>> > Why do we need this? I am generally not in favor of having forward reference until really really needed.
>> >
>> > IF: That is needed to set the oper-speed in this structure once the device parameters are available. Intel patch has hard-coded it to a single value for all Broadcom devices which is not acceptable.
>> 
>> So for Broadcom devices, I think in the static table, no oper-speed should be set. That means it has to come by other means. Either ACPI or DT. But as I mentioned in the other part, that table has to stay const.
>> 
>> IF: The oper_speed comes from DT in my current code already. The BlueZ line discipline currently acts on the oper_speed for all manufacturers. Do you want an exception for Broadcom there? If not, the oper-speed changes should apply to everybody.
> 
> <snip>
>> >> -static const struct hci_uart_proto bcm_proto = {
>> >> +/* This structure may not be const as speed may change at runtime */
>> >
>> > No. This struct is const. These are the value that a protocol driver sets.
>> >
>> > If values are dynamic and change based on DT, then left them zero here and provide a method for overwriting them internally in hci_uart. Please do not hack this in. There are simple UART drivers that we want to enable in a really simple way.
>> >
>> > IF: As I said above, I've inherited the need to change the speed(s) from the recent Intel patches. Will try fixing that.
>> 
>> I get where you are coming from. And for Broadcom the table will not contain an oper-speed. That will come from ACPI or DT. So you action should be to remove it here and introduce a variable in bcm_data that has it and that can be used instead. I have not figured out all the details, but that is the general idea. I leave the details up to you.
>> 
>> IF: As I said above, this is slightly more complicated as the oper_speed is used by the BlueZ line discipline for all manufacturers. And any manufacturer with the oper-speed changeable among the devices will be affected. Since the oper-speed used is largely defined by the UART rather than BT device capabilities, I'm afraid it should be configurable and not reside in a const structure for all.
> 
> FYI, the oper_speed is not available in ACPI table of T100.
> Depending on device, even the init_speed may not be available.
> 
> So, I agree with Ilya these values should be configurable.

that is fine, but not in the driver callback table. That is for simple devices that always have the same default and operational speed. If it is configured outside, then these two need to be left out.

Regards

Marcel

Re: [PATCH v2 0/5] Broadcom Bluetooth UART device driver

[Loic Poulain]

Hi Ilya,

On 10/06/2015 22:05, Ilya Faenson wrote:
> v2 - Release upon the acceptance of Fred's updates,
> 	updated as per the latest comments from Marcel.
> v1 - Original release against the Fred Danis' updates.
>
> Ilya Faenson (5):
>    Broadcom Bluetooth UART Device Tree bindings
>    H4 line discipline enhancements
>    Broadcom Bluetooth UART Platform Driver
>    Support the BCM4354 Bluetooth UART device
>    BlueZ Broadcom UART Protocol

I think that the bcm line discipline proto should work even if there is
no bcm platform device (due to missing DT/ACPI entry).
If there is no matching plat device -> just don't use this platform 
driver to manage pm.
This is for hardware backward compatibility. Moreover we should be able 
to use an
external bcm device over serial/FTDI (dev board/simulator...) which is 
not a plat device.

Regards,
Loic

-- 
Intel Open Source Technology Center
http://oss.intel.com/

RE: [PATCH v2 0/5] Broadcom Bluetooth UART device driver

[Ilya Faenson]

Hi Loic,

-----Original Message-----
From: Loic Poulain [mailto:loic.poulain@intel.com]=20
Sent: Friday, June 12, 2015 4:28 AM
To: Ilya Faenson; Marcel Holtmann
Cc: linux-bluetooth@vger.kernel.org; Arend Van Spriel
Subject: Re: [PATCH v2 0/5] Broadcom Bluetooth UART device driver

Hi Ilya,

On 10/06/2015 22:05, Ilya Faenson wrote:
> v2 - Release upon the acceptance of Fred's updates,
> 	updated as per the latest comments from Marcel.
> v1 - Original release against the Fred Danis' updates.
>
> Ilya Faenson (5):
>    Broadcom Bluetooth UART Device Tree bindings
>    H4 line discipline enhancements
>    Broadcom Bluetooth UART Platform Driver
>    Support the BCM4354 Bluetooth UART device
>    BlueZ Broadcom UART Protocol

I think that the bcm line discipline proto should work even if there is
no bcm platform device (due to missing DT/ACPI entry).
If there is no matching plat device -> just don't use this platform=20
driver to manage pm.
This is for hardware backward compatibility. Moreover we should be able=20
to use an
external bcm device over serial/FTDI (dev board/simulator...) which is=20
not a plat device.

IF: Good point. It is certainly possible to configure the platform device t=
o do exactly nothing but it would be easier for the users not to have it at=
 all this case. Will change the code accordingly.

Regards,
Loic

--=20
Intel Open Source Technology Center
http://oss.intel.com/