* [PATCH v3 1/5] Broadcom Bluetooth UART Device Tree bindings
2015-06-17 15:42 [PATCH v3 0/5] Broadcom Bluetooth UART device driver Ilya Faenson
@ 2015-06-17 15:42 ` Ilya Faenson
2015-06-17 16:55 ` Marcel Holtmann
2015-06-17 15:42 ` [PATCH v3 2/5] H4 line discipline enhancements Ilya Faenson
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Ilya Faenson @ 2015-06-17 15:42 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth, Arend van Spriel, 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..c2cf93a
--- /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.
+
+ - pcm-syncmode : PCM sync mode. 0-slave, 1-master.
+ Default: 0.
+
+
+Example:
+
+ bcm4354_bt_uart: bcm4354-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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v3 1/5] Broadcom Bluetooth UART Device Tree bindings
2015-06-17 15:42 ` [PATCH v3 1/5] Broadcom Bluetooth UART Device Tree bindings Ilya Faenson
@ 2015-06-17 16:55 ` Marcel Holtmann
0 siblings, 0 replies; 12+ messages in thread
From: Marcel Holtmann @ 2015-06-17 16:55 UTC (permalink / raw)
To: Ilya Faenson; +Cc: linux-bluetooth, Arend van Spriel
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..c2cf93a
> --- /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.
since I am not the DT expert, can we get a few Acks on this patch so that I can take it through the bluetooth-next tree.
Regards
Marcel
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 2/5] H4 line discipline enhancements
2015-06-17 15:42 [PATCH v3 0/5] Broadcom Bluetooth UART device driver Ilya Faenson
2015-06-17 15:42 ` [PATCH v3 1/5] Broadcom Bluetooth UART Device Tree bindings Ilya Faenson
@ 2015-06-17 15:42 ` Ilya Faenson
2015-06-17 16:55 ` Marcel Holtmann
2015-06-17 15:42 ` [PATCH v3 3/5] Broadcom Bluetooth UART Platform Driver Ilya Faenson
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Ilya Faenson @ 2015-06-17 15:42 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth, Arend van Spriel, Ilya Faenson
Added the ability to flow control the UART, improved the UART baud
rate setting, transferred the speeds into line discipline from the
protocol and introduced the tty init function.
Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
---
drivers/bluetooth/hci_ldisc.c | 93 +++++++++++++++++++++++++++++++++++++++----
drivers/bluetooth/hci_uart.h | 9 ++++-
2 files changed, 93 insertions(+), 9 deletions(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index ac87346..7159cd0 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -266,6 +266,85 @@ 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_speeds(struct hci_uart *hu, unsigned int init_speed,
+ unsigned int oper_speed)
+{
+ hu->init_speed = init_speed;
+ hu->oper_speed = oper_speed;
+}
+
+void hci_uart_init_tty(struct hci_uart *hu)
+{
+ struct tty_struct *tty = hu->tty;
+ struct ktermios ktermios;
+
+ /* Bring the UART into a known 8 bits no parity hw fc state */
+ ktermios = tty->termios;
+ 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);
+ ktermios.c_cflag |= CS8;
+ ktermios.c_cflag |= CRTSCTS;
+
+ /* tty_set_termios() return not checked as it is always 0 */
+ tty_set_termios(tty, &ktermios);
+}
+
void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed)
{
struct tty_struct *tty = hu->tty;
@@ -273,13 +352,13 @@ void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed)
ktermios = tty->termios;
ktermios.c_cflag &= ~CBAUD;
- ktermios.c_cflag |= BOTHER;
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", hu->hdev->name,
+ tty->termios.c_ispeed, tty->termios.c_ospeed);
}
static int hci_uart_setup(struct hci_dev *hdev)
@@ -289,13 +368,13 @@ static int hci_uart_setup(struct hci_dev *hdev)
struct sk_buff *skb;
int err;
- if (hu->proto->init_speed)
- hci_uart_set_baudrate(hu, hu->proto->init_speed);
+ if (hu->init_speed)
+ hci_uart_set_baudrate(hu, hu->init_speed);
- if (hu->proto->set_baudrate && hu->proto->oper_speed) {
- err = hu->proto->set_baudrate(hu, hu->proto->oper_speed);
+ if (hu->proto->set_baudrate && hu->oper_speed) {
+ err = hu->proto->set_baudrate(hu, hu->oper_speed);
if (!err)
- hci_uart_set_baudrate(hu, hu->proto->oper_speed);
+ hci_uart_set_baudrate(hu, hu->oper_speed);
}
if (hu->proto->setup)
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index e9f970c..4781636 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -58,8 +58,6 @@ struct hci_uart;
struct hci_uart_proto {
unsigned int id;
const char *name;
- unsigned int init_speed;
- unsigned int oper_speed;
int (*open)(struct hci_uart *hu);
int (*close)(struct hci_uart *hu);
int (*flush)(struct hci_uart *hu);
@@ -85,6 +83,9 @@ struct hci_uart {
struct sk_buff *tx_skb;
unsigned long tx_state;
spinlock_t rx_lock;
+
+ unsigned int init_speed;
+ unsigned int oper_speed;
};
/* HCI_UART proto flag bits */
@@ -99,7 +100,11 @@ int hci_uart_register_proto(const struct hci_uart_proto *p);
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_init_tty(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);
+void hci_uart_set_speeds(struct hci_uart *hu, unsigned int init_speed,
+ unsigned int oper_speed);
#ifdef CONFIG_BT_HCIUART_H4
int h4_init(void);
--
1.9.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v3 2/5] H4 line discipline enhancements
2015-06-17 15:42 ` [PATCH v3 2/5] H4 line discipline enhancements Ilya Faenson
@ 2015-06-17 16:55 ` Marcel Holtmann
2015-06-17 19:48 ` Ilya Faenson
0 siblings, 1 reply; 12+ messages in thread
From: Marcel Holtmann @ 2015-06-17 16:55 UTC (permalink / raw)
To: Ilya Faenson; +Cc: BlueZ development, Arend van Spriel
Hi Ilya,
> Added the ability to flow control the UART, improved the UART baud
> rate setting, transferred the speeds into line discipline from the
> protocol and introduced the tty init function.
>
> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
> ---
> drivers/bluetooth/hci_ldisc.c | 93 +++++++++++++++++++++++++++++++++++++++----
> drivers/bluetooth/hci_uart.h | 9 ++++-
> 2 files changed, 93 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index ac87346..7159cd0 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -266,6 +266,85 @@ 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_speeds(struct hci_uart *hu, unsigned int init_speed,
> + unsigned int oper_speed)
> +{
> + hu->init_speed = init_speed;
> + hu->oper_speed = oper_speed;
> +}
> +
> +void hci_uart_init_tty(struct hci_uart *hu)
> +{
> + struct tty_struct *tty = hu->tty;
> + struct ktermios ktermios;
> +
> + /* Bring the UART into a known 8 bits no parity hw fc state */
> + ktermios = tty->termios;
> + 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);
> + ktermios.c_cflag |= CS8;
> + ktermios.c_cflag |= CRTSCTS;
> +
> + /* tty_set_termios() return not checked as it is always 0 */
> + tty_set_termios(tty, &ktermios);
> +}
> +
> void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed)
> {
> struct tty_struct *tty = hu->tty;
> @@ -273,13 +352,13 @@ void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed)
>
> ktermios = tty->termios;
> ktermios.c_cflag &= ~CBAUD;
> - ktermios.c_cflag |= BOTHER;
> 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", hu->hdev->name,
> + tty->termios.c_ispeed, tty->termios.c_ospeed);
> }
>
> static int hci_uart_setup(struct hci_dev *hdev)
> @@ -289,13 +368,13 @@ static int hci_uart_setup(struct hci_dev *hdev)
> struct sk_buff *skb;
> int err;
>
> - if (hu->proto->init_speed)
> - hci_uart_set_baudrate(hu, hu->proto->init_speed);
> + if (hu->init_speed)
> + hci_uart_set_baudrate(hu, hu->init_speed);
>
> - if (hu->proto->set_baudrate && hu->proto->oper_speed) {
> - err = hu->proto->set_baudrate(hu, hu->proto->oper_speed);
> + if (hu->proto->set_baudrate && hu->oper_speed) {
> + err = hu->proto->set_baudrate(hu, hu->oper_speed);
> if (!err)
> - hci_uart_set_baudrate(hu, hu->proto->oper_speed);
> + hci_uart_set_baudrate(hu, hu->oper_speed);
> }
>
> if (hu->proto->setup)
> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> index e9f970c..4781636 100644
> --- a/drivers/bluetooth/hci_uart.h
> +++ b/drivers/bluetooth/hci_uart.h
> @@ -58,8 +58,6 @@ struct hci_uart;
> struct hci_uart_proto {
> unsigned int id;
> const char *name;
> - unsigned int init_speed;
> - unsigned int oper_speed;
I would leave these in. I mean they are really useful for simple drivers that just want to set one these. What I would do is also just copy the values from here into hu->init_speed and hu->oper_speed as their default values. Then vendor drivers can overwrite them later on or they stay zero.
In addition, removing the fields here will break git bisect and that is not what we can do. Meaning if we really wanted to remove the values completely. You needed to leave them in here and then send a follow up patch that uses the new hci_uart_set_speeds function and remove the fields here and its usage in hci_bcm.c.
> int (*open)(struct hci_uart *hu);
> int (*close)(struct hci_uart *hu);
> int (*flush)(struct hci_uart *hu);
> @@ -85,6 +83,9 @@ struct hci_uart {
> struct sk_buff *tx_skb;
> unsigned long tx_state;
> spinlock_t rx_lock;
> +
> + unsigned int init_speed;
> + unsigned int oper_speed;
> };
>
> /* HCI_UART proto flag bits */
> @@ -99,7 +100,11 @@ int hci_uart_register_proto(const struct hci_uart_proto *p);
> 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_init_tty(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);
> +void hci_uart_set_speeds(struct hci_uart *hu, unsigned int init_speed,
> + unsigned int oper_speed);
The rest looks good to me.
Regards
Marcel
^ permalink raw reply [flat|nested] 12+ messages in thread* RE: [PATCH v3 2/5] H4 line discipline enhancements
2015-06-17 16:55 ` Marcel Holtmann
@ 2015-06-17 19:48 ` Ilya Faenson
0 siblings, 0 replies; 12+ messages in thread
From: Ilya Faenson @ 2015-06-17 19:48 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: BlueZ development, Arend Van Spriel
Hi Marcel,
-----Original Message-----
From: linux-bluetooth-owner@vger.kernel.org [mailto:linux-bluetooth-owner@vger.kernel.org] On Behalf Of Marcel Holtmann
Sent: Wednesday, June 17, 2015 12:55 PM
To: Ilya Faenson
Cc: BlueZ development; Arend Van Spriel
Subject: Re: [PATCH v3 2/5] H4 line discipline enhancements
Hi Ilya,
> Added the ability to flow control the UART, improved the UART baud
> rate setting, transferred the speeds into line discipline from the
> protocol and introduced the tty init function.
>
> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
> ---
> drivers/bluetooth/hci_ldisc.c | 93 +++++++++++++++++++++++++++++++++++++++----
> drivers/bluetooth/hci_uart.h | 9 ++++-
> 2 files changed, 93 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index ac87346..7159cd0 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -266,6 +266,85 @@ 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_speeds(struct hci_uart *hu, unsigned int init_speed,
> + unsigned int oper_speed)
> +{
> + hu->init_speed = init_speed;
> + hu->oper_speed = oper_speed;
> +}
> +
> +void hci_uart_init_tty(struct hci_uart *hu)
> +{
> + struct tty_struct *tty = hu->tty;
> + struct ktermios ktermios;
> +
> + /* Bring the UART into a known 8 bits no parity hw fc state */
> + ktermios = tty->termios;
> + 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);
> + ktermios.c_cflag |= CS8;
> + ktermios.c_cflag |= CRTSCTS;
> +
> + /* tty_set_termios() return not checked as it is always 0 */
> + tty_set_termios(tty, &ktermios);
> +}
> +
> void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed)
> {
> struct tty_struct *tty = hu->tty;
> @@ -273,13 +352,13 @@ void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed)
>
> ktermios = tty->termios;
> ktermios.c_cflag &= ~CBAUD;
> - ktermios.c_cflag |= BOTHER;
> 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", hu->hdev->name,
> + tty->termios.c_ispeed, tty->termios.c_ospeed);
> }
>
> static int hci_uart_setup(struct hci_dev *hdev)
> @@ -289,13 +368,13 @@ static int hci_uart_setup(struct hci_dev *hdev)
> struct sk_buff *skb;
> int err;
>
> - if (hu->proto->init_speed)
> - hci_uart_set_baudrate(hu, hu->proto->init_speed);
> + if (hu->init_speed)
> + hci_uart_set_baudrate(hu, hu->init_speed);
>
> - if (hu->proto->set_baudrate && hu->proto->oper_speed) {
> - err = hu->proto->set_baudrate(hu, hu->proto->oper_speed);
> + if (hu->proto->set_baudrate && hu->oper_speed) {
> + err = hu->proto->set_baudrate(hu, hu->oper_speed);
> if (!err)
> - hci_uart_set_baudrate(hu, hu->proto->oper_speed);
> + hci_uart_set_baudrate(hu, hu->oper_speed);
> }
>
> if (hu->proto->setup)
> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
> index e9f970c..4781636 100644
> --- a/drivers/bluetooth/hci_uart.h
> +++ b/drivers/bluetooth/hci_uart.h
> @@ -58,8 +58,6 @@ struct hci_uart;
> struct hci_uart_proto {
> unsigned int id;
> const char *name;
> - unsigned int init_speed;
> - unsigned int oper_speed;
I would leave these in. I mean they are really useful for simple drivers that just want to set one these. What I would do is also just copy the values from here into hu->init_speed and hu->oper_speed as their default values. Then vendor drivers can overwrite them later on or they stay zero.
In addition, removing the fields here will break git bisect and that is not what we can do. Meaning if we really wanted to remove the values completely. You needed to leave them in here and then send a follow up patch that uses the new hci_uart_set_speeds function and remove the fields here and its usage in hci_bcm.c.
IF: Okay, I will restore the speed fields in the protocol.
> int (*open)(struct hci_uart *hu);
> int (*close)(struct hci_uart *hu);
> int (*flush)(struct hci_uart *hu);
> @@ -85,6 +83,9 @@ struct hci_uart {
> struct sk_buff *tx_skb;
> unsigned long tx_state;
> spinlock_t rx_lock;
> +
> + unsigned int init_speed;
> + unsigned int oper_speed;
> };
>
> /* HCI_UART proto flag bits */
> @@ -99,7 +100,11 @@ int hci_uart_register_proto(const struct hci_uart_proto *p);
> 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_init_tty(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);
> +void hci_uart_set_speeds(struct hci_uart *hu, unsigned int init_speed,
> + unsigned int oper_speed);
The rest looks good to me.
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 3/5] Broadcom Bluetooth UART Platform Driver
2015-06-17 15:42 [PATCH v3 0/5] Broadcom Bluetooth UART device driver Ilya Faenson
2015-06-17 15:42 ` [PATCH v3 1/5] Broadcom Bluetooth UART Device Tree bindings Ilya Faenson
2015-06-17 15:42 ` [PATCH v3 2/5] H4 line discipline enhancements Ilya Faenson
@ 2015-06-17 15:42 ` Ilya Faenson
2015-06-17 17:14 ` Marcel Holtmann
2015-06-17 15:42 ` [PATCH v3 4/5] Support the BCM4354 Bluetooth UART device Ilya Faenson
2015-06-17 15:42 ` [PATCH v3 5/5] BlueZ Broadcom UART Protocol Ilya Faenson
4 siblings, 1 reply; 12+ messages in thread
From: Ilya Faenson @ 2015-06-17 15:42 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth, Arend van Spriel, Ilya Faenson
Introduce the device tree enumerated Broadcom Bluetooth UART driver.
Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
---
drivers/bluetooth/Makefile | 1 +
drivers/bluetooth/btbcm_uart.c | 580 +++++++++++++++++++++++++++++++++++++++++
drivers/bluetooth/btbcm_uart.h | 74 ++++++
3 files changed, 655 insertions(+)
create mode 100644 drivers/bluetooth/btbcm_uart.c
create mode 100644 drivers/bluetooth/btbcm_uart.h
diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
index f40e194..7947abb 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_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..8069997
--- /dev/null
+++ b/drivers/bluetooth/btbcm_uart.c
@@ -0,0 +1,580 @@
+/*
+ * 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_wakeup_task(struct work_struct *work)
+{
+ struct bcm_device *dev = container_of(work, struct bcm_device,
+ wakeup_work);
+ int gpio_value;
+
+ if (!dev)
+ return;
+
+ /* Make sure the device is resumed */
+ gpio_value = !dev->dev_wake_active_low;
+ if (dev->dev_wake_gpio) {
+ gpiod_set_value(dev->dev_wake_gpio, gpio_value);
+ BT_DBG("wakeup_task - resume %d written, delaying 15 ms",
+ gpio_value);
+ mdelay(15);
+ }
+
+ /* Let the protocol know it's time to wake up */
+ if (dev->protocol_callbacks.p_wakeup)
+ dev->protocol_callbacks.p_wakeup(
+ dev->protocol_callbacks.context);
+}
+
+/* Interrupt routine for the wake from the device */
+static irqreturn_t bcm_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("isr with bt_wake of %d (active_low %d), req bh",
+ 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_uart_probe(struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+ const char *tty_name;
+ int ret;
+ struct bcm_device *dev;
+
+ dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
+ if (!dev) {
+ BT_DBG("probe - failing due to no memory");
+ return -ENOMEM;
+ }
+
+ dev->pdev = pdev;
+
+ /* Get dev wake GPIO */
+ dev->dev_wake_gpio = gpiod_get(&pdev->dev, "bt-wake");
+ BT_DBG("probe %p - gpiod_get for bt-wake returned %p", dev,
+ dev->dev_wake_gpio);
+ if (IS_ERR(dev->dev_wake_gpio)) {
+ ret = PTR_ERR(dev->dev_wake_gpio);
+ if (ret != -ENOENT)
+ dev_err(&pdev->dev, "probe - dev_wake GPIO: %d\n", ret);
+ dev->dev_wake_gpio = NULL;
+ } else {
+ int gpio_value;
+
+ dev->dev_wake_active_low = gpiod_is_active_low
+ (dev->dev_wake_gpio);
+ BT_DBG("probe - dev_wake a-low is %d (cans %d)",
+ dev->dev_wake_active_low,
+ gpiod_cansleep(dev->dev_wake_gpio));
+
+ /* configure dev_wake as output with init resumed state */
+ gpio_value = !dev->dev_wake_active_low;
+ ret = gpiod_direction_output(dev->dev_wake_gpio, gpio_value);
+ if (ret < 0) {
+ dev_err(&pdev->dev,
+ "probe - s dev_wake GPIO: %d\n", ret);
+ gpiod_put(dev->dev_wake_gpio);
+ dev->dev_wake_gpio = NULL;
+ goto end;
+ } else {
+ BT_DBG("probe - dev_wake set to %d", gpio_value);
+ }
+ }
+
+ /* Get power on/off GPIO */
+ dev->reg_on_gpio = gpiod_get(&pdev->dev, "bt-reg-on");
+ BT_DBG("probe - gpiod_get for bt-reg-on returned %p", dev->reg_on_gpio);
+ if (IS_ERR(dev->reg_on_gpio)) {
+ ret = PTR_ERR(dev->reg_on_gpio);
+ if (ret != -ENOENT)
+ dev_err(&pdev->dev, "probe - reg_on GPIO: %d\n", ret);
+ dev->reg_on_gpio = NULL;
+ } else {
+ int poweron_flag;
+
+ dev->reg_on_active_low = gpiod_is_active_low(dev->reg_on_gpio);
+ BT_DBG("probe - reg_on a-low is %d (cansleep %d)",
+ dev->reg_on_active_low,
+ gpiod_cansleep(dev->reg_on_gpio));
+
+ /* configure reg_on as output with init on state */
+ poweron_flag = !dev->reg_on_active_low;
+ ret = gpiod_direction_output(dev->reg_on_gpio, poweron_flag);
+ if (ret < 0) {
+ dev_err(&pdev->dev,
+ "probe - set reg_on GPIO: %d\n", ret);
+ gpiod_put(dev->reg_on_gpio);
+ dev->reg_on_gpio = NULL;
+ } else
+ BT_DBG("probe - reg_on initially set to %d",
+ poweron_flag);
+ }
+
+ platform_set_drvdata(pdev, dev);
+ /* Must be done before interrupt is requested */
+ INIT_WORK(&dev->wakeup_work, bcm_wakeup_task);
+
+ /* Get bt host wake GPIO */
+ dev->bt_wake_gpio = gpiod_get(&pdev->dev, "bt-host-wake");
+ BT_DBG("probe - gpiod_get for bt-host-wake returned %p",
+ dev->bt_wake_gpio);
+ if (IS_ERR(dev->bt_wake_gpio)) {
+ ret = PTR_ERR(dev->bt_wake_gpio);
+ if (ret != -ENOENT)
+ dev_err(&pdev->dev, "probe - bt_wake GPIO: %d\n", ret);
+ dev->bt_wake_gpio = NULL;
+ } else {
+ /* configure bt_wake as input */
+ ret = gpiod_direction_input(dev->bt_wake_gpio);
+ if (ret < 0) {
+ dev_err(&pdev->dev,
+ "probe - set bt_wake GPIO: %d\n", ret);
+ gpiod_put(dev->bt_wake_gpio);
+ dev->bt_wake_gpio = NULL;
+ } else {
+ dev->bt_wake_active_low = gpiod_is_active_low
+ (dev->bt_wake_gpio);
+ BT_DBG("probe -bt_wake a-low is %d(cansleep %d)",
+ dev->bt_wake_active_low,
+ gpiod_cansleep(dev->bt_wake_gpio));
+ dev->bt_wake_irq = gpiod_to_irq
+ (dev->bt_wake_gpio);
+ if (dev->bt_wake_irq < 0)
+ dev_err(&pdev->dev,
+ "probe - HOST_WAKE IRQ: %d\n", ret);
+ else {
+ unsigned long intflags = IRQF_TRIGGER_RISING;
+
+ if (dev->bt_wake_active_low)
+ intflags = IRQF_TRIGGER_FALLING;
+
+ ret = request_irq(dev->bt_wake_irq,
+ bcm_uart_isr, intflags,
+ "bt_host_wake", dev);
+ if (ret < 0)
+ dev_err(&pdev->dev,
+ "probe - failed IRQ %d: %d",
+ dev->bt_wake_irq, ret);
+ else
+ BT_DBG("probe - IRQ %d",
+ dev->bt_wake_irq);
+ }
+ }
+ }
+
+ dev->configure_sleep = of_property_read_bool(np, "configure-sleep");
+ dev->manual_fc = 0;
+ of_property_read_u32(np, "manual-fc", &dev->manual_fc);
+ dev->oper_speed = 3000000;
+ of_property_read_u32(np, "oper-speed", &dev->oper_speed);
+ BT_DBG("oper-speed read as %d", dev->oper_speed);
+ dev->configure_audio = of_property_read_bool(np, "configure-audio");
+ BT_DBG("conf-sleep %d, manual-fc %d, oper-speed %d, conf-audio %d",
+ dev->configure_sleep, dev->manual_fc, dev->oper_speed,
+ dev->configure_audio);
+
+ if (dev->configure_audio) {
+ /* Defaults for audio */
+ dev->pcm_clockmode = 0;
+ dev->pcm_fillmethod = 2;
+ dev->pcm_fillnum = 0;
+ dev->pcm_fillvalue = 3;
+ dev->pcm_incallbitclock = 0;
+ dev->pcm_lsbfirst = 0;
+ dev->pcm_rightjustify = 0;
+ dev->pcm_routing = 0;
+ dev->pcm_shortframesync = 0;
+ dev->pcm_syncmode = 0;
+
+ of_property_read_u32(np, "pcm-clockmode", &dev->pcm_clockmode);
+ of_property_read_u32(np, "pcm-fillmethod",
+ &dev->pcm_fillmethod);
+ of_property_read_u32(np, "pcm-fillnum", &dev->pcm_fillnum);
+ of_property_read_u32(np, "pcm-fillvalue", &dev->pcm_fillvalue);
+ of_property_read_u32(np, "pcm-incallbitclock",
+ &dev->pcm_incallbitclock);
+ BT_DBG("pcm-clockmode %d, method %d, num %d, value %d, bit %d",
+ dev->pcm_clockmode, dev->pcm_fillmethod,
+ dev->pcm_fillnum, dev->pcm_fillvalue,
+ dev->pcm_incallbitclock);
+
+ of_property_read_u32(np, "pcm-lsbfirst", &dev->pcm_lsbfirst);
+ of_property_read_u32(np, "pcm-rightjustify",
+ &dev->pcm_rightjustify);
+ of_property_read_u32(np, "pcm-routing", &dev->pcm_routing);
+ of_property_read_u32(np, "pcm-shortframesync",
+ &dev->pcm_shortframesync);
+ of_property_read_u32(np, "pcm-syncmode", &dev->pcm_syncmode);
+ BT_DBG("pcm-lsb %d, right %d, routing %d, short %d, sync %d",
+ dev->pcm_lsbfirst, dev->pcm_rightjustify,
+ dev->pcm_routing, dev->pcm_shortframesync,
+ dev->pcm_syncmode);
+ }
+
+ if (!of_property_read_string(np, "tty", &tty_name)) {
+ strcpy(dev->tty_name, tty_name);
+ BT_DBG("tty name read as %s", dev->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(&dev->list, &device_list);
+ spin_unlock(&device_list_lock);
+
+end:
+ if (ret) {
+ if (dev->reg_on_gpio) {
+ gpiod_put(dev->reg_on_gpio);
+ dev->reg_on_gpio = NULL;
+ }
+ if (dev->bt_wake_gpio) {
+ gpiod_put(dev->bt_wake_gpio);
+ dev->bt_wake_gpio = NULL;
+ }
+ if (dev->dev_wake_gpio) {
+ gpiod_put(dev->dev_wake_gpio);
+ dev->dev_wake_gpio = NULL;
+ }
+ }
+
+ BT_DBG("probe with the result %d", ret);
+ return ret;
+}
+
+/* Device instance removal */
+static int bcm_uart_remove(struct platform_device *pdev)
+{
+ struct bcm_device *dev = platform_get_drvdata(pdev);
+
+ if (!dev) {
+ BT_DBG("remove - logic error, no probe?!");
+ return 0;
+ }
+
+ spin_lock(&device_list_lock);
+ list_del(&dev->list);
+ spin_unlock(&device_list_lock);
+
+ BT_DBG("remove %p - freeing interrupt %d", dev, dev->bt_wake_irq);
+ free_irq(dev->bt_wake_irq, dev);
+
+ if (dev->reg_on_gpio) {
+ BT_DBG("remove - releasing reg_on_gpio");
+ gpiod_put(dev->reg_on_gpio);
+ dev->reg_on_gpio = NULL;
+ }
+
+ if (dev->dev_wake_gpio) {
+ BT_DBG("remove - releasing dev_wake_gpio");
+ gpiod_put(dev->dev_wake_gpio);
+ dev->dev_wake_gpio = NULL;
+ }
+
+ if (dev->bt_wake_gpio) {
+ BT_DBG("remove - releasing bt_wake_gpio");
+ gpiod_put(dev->bt_wake_gpio);
+ dev->bt_wake_gpio = NULL;
+ }
+
+ BT_DBG("remove - %p done", dev);
+ return 0;
+}
+
+/* Platform resume callback */
+static int bcm_uart_resume(struct device *pdev)
+{
+ struct bcm_device *dev = platform_get_drvdata(
+ to_platform_device(pdev));
+ int gpio_value;
+
+ if (!dev) {
+ BT_DBG("resume - logic error, no device?!");
+ return 0;
+ }
+
+ BT_DBG("resume %p", dev);
+
+ gpio_value = !dev->dev_wake_active_low;
+ if (dev->dev_wake_gpio) {
+ gpiod_set_value(dev->dev_wake_gpio, gpio_value);
+ BT_DBG("resume - %d written, delaying 15 ms", gpio_value);
+ mdelay(15);
+ }
+
+ /* Let the protocol know the platform is resuming */
+ if (dev->protocol_callbacks.p_resume)
+ dev->protocol_callbacks.p_resume(
+ dev->protocol_callbacks.context);
+
+ return 0;
+}
+
+/* Platform suspend callback */
+static int bcm_uart_suspend(struct device *pdev)
+{
+ struct bcm_device *dev = platform_get_drvdata(
+ to_platform_device(pdev));
+ int gpio_value;
+
+ if (!dev) {
+ BT_DBG("suspend - logic error, no device?!");
+ return 0;
+ }
+
+ BT_DBG("suspend - %p", dev);
+
+ /* Let the protocol know the platform is suspending */
+ if (dev->protocol_callbacks.p_suspend)
+ dev->protocol_callbacks.p_suspend(
+ dev->protocol_callbacks.context);
+
+ /* Suspend the device */
+ if (dev->dev_wake_gpio) {
+ gpio_value = !!dev->dev_wake_active_low;
+ gpiod_set_value(dev->dev_wake_gpio, gpio_value);
+ BT_DBG("suspend - %d written, delaying 15 ms", gpio_value);
+ mdelay(15);
+ }
+
+ return 0;
+}
+
+/* Entry point for calls from the protocol */
+void *btbcm_uart_set_callbacks(struct btbcm_uart_callbacks *pc)
+{
+ struct bcm_device *dev = NULL;
+ bool is_found = false;
+ struct list_head *ptr;
+
+ BT_DBG("set_callbacks for %s(%p)", pc->name, pc->context);
+
+ spin_lock(&device_list_lock);
+ list_for_each(ptr, &device_list) {
+ dev = list_entry(ptr, struct bcm_device, list);
+ if (!strcmp(dev->tty_name, pc->name)) {
+ is_found = true;
+ break;
+ }
+ }
+ spin_unlock(&device_list_lock);
+
+ if (!is_found) {
+ BT_DBG("set_callbacks - no device!");
+ return NULL;
+ }
+
+ dev->protocol_callbacks = *pc;
+ return dev;
+}
+EXPORT_SYMBOL_GPL(btbcm_uart_set_callbacks);
+
+void btbcm_uart_reset_callbacks(void *device_context)
+{
+ struct bcm_device *dev = device_context;
+
+ memset(&dev->protocol_callbacks, 0, sizeof(dev->protocol_callbacks));
+}
+EXPORT_SYMBOL_GPL(btbcm_uart_reset_callbacks);
+
+void btbcm_uart_poweron(void *device_context)
+{
+ struct bcm_device *dev = device_context;
+ int poweron_flag;
+
+ if (dev->reg_on_gpio) {
+ poweron_flag = !dev->reg_on_active_low;
+ gpiod_set_value(dev->reg_on_gpio, poweron_flag);
+ BT_DBG("poweron %d, delay 15 ms", poweron_flag);
+ }
+}
+EXPORT_SYMBOL_GPL(btbcm_uart_poweron);
+
+void btbcm_uart_poweroff(void *device_context)
+{
+ struct bcm_device *dev = device_context;
+ int poweron_flag;
+
+ if (dev->reg_on_gpio) {
+ poweron_flag = dev->reg_on_active_low;
+ gpiod_set_value(dev->reg_on_gpio, poweron_flag);
+ BT_DBG("poweroff %d, delay 15 ms", poweron_flag);
+ }
+}
+EXPORT_SYMBOL_GPL(btbcm_uart_poweroff);
+
+void btbcm_uart_suspend(void *device_context)
+{
+ struct bcm_device *dev = device_context;
+ int gpio_value;
+
+ if (dev->dev_wake_gpio) {
+ gpio_value = !!dev->dev_wake_active_low;
+ gpiod_set_value(dev->dev_wake_gpio, gpio_value);
+ BT_DBG("suspend %d, delay 15ms", gpio_value);
+ mdelay(15);
+ }
+}
+EXPORT_SYMBOL_GPL(btbcm_uart_suspend);
+
+void btbcm_uart_resume(void *device_context)
+{
+ struct bcm_device *dev = device_context;
+ int gpio_value;
+
+ if (dev->dev_wake_gpio) {
+ gpio_value = !dev->dev_wake_active_low;
+ gpiod_set_value(dev->dev_wake_gpio, gpio_value);
+ BT_DBG("resume %d, delay 15ms", gpio_value);
+ mdelay(15);
+ }
+}
+EXPORT_SYMBOL_GPL(btbcm_uart_resume);
+
+void btbcm_uart_get_params(void *device_context, struct btbcm_uart_params *pp)
+{
+ struct bcm_device *dev = device_context;
+
+ memset(pp, 0, sizeof(struct btbcm_uart_params));
+ pp->configure_sleep = dev->configure_sleep;
+ pp->manual_fc = dev->manual_fc;
+ pp->dev_wake_active_low = dev->dev_wake_active_low;
+ pp->bt_wake_active_low = dev->bt_wake_active_low;
+ pp->idle_timeout_in_secs = idle_timeout;
+ pp->oper_speed = dev->oper_speed;
+ pp->configure_audio = dev->configure_audio;
+ pp->pcm_clockmode = dev->pcm_clockmode;
+ pp->pcm_fillmethod = dev->pcm_fillmethod;
+ pp->pcm_fillnum = dev->pcm_fillnum;
+ pp->pcm_fillvalue = dev->pcm_fillvalue;
+ pp->pcm_incallbitclock = dev->pcm_incallbitclock;
+ pp->pcm_lsbfirst = dev->pcm_lsbfirst;
+ pp->pcm_rightjustify = dev->pcm_rightjustify;
+ pp->pcm_routing = dev->pcm_routing;
+ pp->pcm_shortframesync = dev->pcm_shortframesync;
+ pp->pcm_syncmode = dev->pcm_syncmode;
+}
+EXPORT_SYMBOL_GPL(btbcm_uart_get_params);
+
+/* Platform susp and resume callbacks */
+static SIMPLE_DEV_PM_OPS(bcm_uart_pm_ops, bcm_uart_suspend, bcm_uart_resume);
+
+/* Driver match table */
+static const struct of_device_id bcm_uart_table[] = {
+ { .compatible = "brcm,brcm-bt-uart" },
+ {}
+};
+
+/* Driver configuration */
+static struct platform_driver bcm_uart_driver = {
+ .probe = bcm_uart_probe,
+ .remove = bcm_uart_remove,
+ .driver = {
+ .name = "btbcm_uart",
+ .of_match_table = of_match_ptr(bcm_uart_table),
+ .owner = THIS_MODULE,
+ .pm = &bcm_uart_pm_ops,
+ },
+};
+
+module_platform_driver(bcm_uart_driver);
+
+MODULE_AUTHOR("Ilya Faenson <ifaenson@broadcom.com>");
+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..96dae85
--- /dev/null
+++ b/drivers/bluetooth/btbcm_uart.h
@@ -0,0 +1,74 @@
+/*
+ * 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.
+ *
+ */
+
+/* Callbacks from the driver into the protocol */
+struct btbcm_uart_callbacks {
+ void *context; /* protocol instance context */
+ char name[64]; /* protocol tty device, for example, ttyS0 */
+
+ /* client callbacks */
+ void (*p_suspend)(void *context);
+ void (*p_resume)(void *context);
+ void (*p_wakeup)(void *context);
+};
+
+/* Driver parameters retrieved from the DT or ACPI */
+struct btbcm_uart_params {
+ 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 */
+extern void *btbcm_uart_set_callbacks(struct btbcm_uart_callbacks *pc);
+
+/* Reset protocol callbacks */
+extern void btbcm_uart_reset_callbacks(void *device_context);
+
+/* Retrieve BT device parameters */
+extern void btbcm_uart_get_params(void *device_context,
+ struct btbcm_uart_params *pp);
+
+/* Resume the BT device via GPIO */
+extern void btbcm_uart_resume(void *device_context);
+
+/* Suspend the BT device via GPIO */
+extern void btbcm_uart_suspend(void *device_context);
+
+/* Power the BT device off via GPIO */
+extern void btbcm_uart_poweroff(void *device_context);
+
+/* Power the BT device on via GPIO */
+extern void btbcm_uart_poweron(void *device_context);
+
--
1.9.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v3 3/5] Broadcom Bluetooth UART Platform Driver
2015-06-17 15:42 ` [PATCH v3 3/5] Broadcom Bluetooth UART Platform Driver Ilya Faenson
@ 2015-06-17 17:14 ` Marcel Holtmann
2015-06-17 20:01 ` Ilya Faenson
0 siblings, 1 reply; 12+ messages in thread
From: Marcel Holtmann @ 2015-06-17 17:14 UTC (permalink / raw)
To: Ilya Faenson; +Cc: linux-bluetooth, Arend van Spriel
Hi Ilya,
> Introduce the device tree enumerated Broadcom Bluetooth UART driver.
don't be shy and give a bit of more detailed explanation on what the driver is doing ;)
>
> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
> ---
> drivers/bluetooth/Makefile | 1 +
> drivers/bluetooth/btbcm_uart.c | 580 +++++++++++++++++++++++++++++++++++++++++
> drivers/bluetooth/btbcm_uart.h | 74 ++++++
> 3 files changed, 655 insertions(+)
> create mode 100644 drivers/bluetooth/btbcm_uart.c
> create mode 100644 drivers/bluetooth/btbcm_uart.h
>
> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
> index f40e194..7947abb 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_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..8069997
> --- /dev/null
> +++ b/drivers/bluetooth/btbcm_uart.c
> @@ -0,0 +1,580 @@
> +/*
> + * 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 assumed we agreed on that we add the module parameter as a separate patch. It was still not clear to me if this is just for debugging purposes or not. It sounded more like for fine tuning and once it is fine tuned it stays that way.
> +
> +/* 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;
Don't you want to just call it callbacks here. I think that it will make overall code less awkward and will cause less line breaks.
> + 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_wakeup_task(struct work_struct *work)
> +{
> + struct bcm_device *dev = container_of(work, struct bcm_device,
> + wakeup_work);
> + int gpio_value;
> +
> + if (!dev)
> + return;
Can this be actually NULL?
> +
> + /* Make sure the device is resumed */
> + gpio_value = !dev->dev_wake_active_low;
> + if (dev->dev_wake_gpio) {
> + gpiod_set_value(dev->dev_wake_gpio, gpio_value);
> + BT_DBG("wakeup_task - resume %d written, delaying 15 ms",
> + gpio_value);
> + mdelay(15);
> + }
> +
> + /* Let the protocol know it's time to wake up */
> + if (dev->protocol_callbacks.p_wakeup)
> + dev->protocol_callbacks.p_wakeup(
> + dev->protocol_callbacks.context);
> +}
> +
> +/* Interrupt routine for the wake from the device */
> +static irqreturn_t bcm_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("isr with bt_wake of %d (active_low %d), req bh",
> + 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_uart_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + const char *tty_name;
> + int ret;
> + struct bcm_device *dev;
> +
> + dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> + if (!dev) {
> + BT_DBG("probe - failing due to no memory");
Honestly, do not even bother here. This malloc will almost never fail. There is a funny LWN.net article on how the kernel will almost always give you the memory.
> + return -ENOMEM;
> + }
> +
> + dev->pdev = pdev;
> +
> + /* Get dev wake GPIO */
> + dev->dev_wake_gpio = gpiod_get(&pdev->dev, "bt-wake");
> + BT_DBG("probe %p - gpiod_get for bt-wake returned %p", dev,
> + dev->dev_wake_gpio);
> + if (IS_ERR(dev->dev_wake_gpio)) {
> + ret = PTR_ERR(dev->dev_wake_gpio);
> + if (ret != -ENOENT)
> + dev_err(&pdev->dev, "probe - dev_wake GPIO: %d\n", ret);
> + dev->dev_wake_gpio = NULL;
> + } else {
> + int gpio_value;
> +
> + dev->dev_wake_active_low = gpiod_is_active_low
> + (dev->dev_wake_gpio);
> + BT_DBG("probe - dev_wake a-low is %d (cans %d)",
> + dev->dev_wake_active_low,
> + gpiod_cansleep(dev->dev_wake_gpio));
> +
> + /* configure dev_wake as output with init resumed state */
> + gpio_value = !dev->dev_wake_active_low;
> + ret = gpiod_direction_output(dev->dev_wake_gpio, gpio_value);
> + if (ret < 0) {
> + dev_err(&pdev->dev,
> + "probe - s dev_wake GPIO: %d\n", ret);
> + gpiod_put(dev->dev_wake_gpio);
> + dev->dev_wake_gpio = NULL;
> + goto end;
> + } else {
> + BT_DBG("probe - dev_wake set to %d", gpio_value);
> + }
> + }
> +
> + /* Get power on/off GPIO */
> + dev->reg_on_gpio = gpiod_get(&pdev->dev, "bt-reg-on");
> + BT_DBG("probe - gpiod_get for bt-reg-on returned %p", dev->reg_on_gpio);
> + if (IS_ERR(dev->reg_on_gpio)) {
> + ret = PTR_ERR(dev->reg_on_gpio);
> + if (ret != -ENOENT)
> + dev_err(&pdev->dev, "probe - reg_on GPIO: %d\n", ret);
> + dev->reg_on_gpio = NULL;
> + } else {
> + int poweron_flag;
> +
> + dev->reg_on_active_low = gpiod_is_active_low(dev->reg_on_gpio);
> + BT_DBG("probe - reg_on a-low is %d (cansleep %d)",
> + dev->reg_on_active_low,
> + gpiod_cansleep(dev->reg_on_gpio));
> +
> + /* configure reg_on as output with init on state */
> + poweron_flag = !dev->reg_on_active_low;
> + ret = gpiod_direction_output(dev->reg_on_gpio, poweron_flag);
> + if (ret < 0) {
> + dev_err(&pdev->dev,
> + "probe - set reg_on GPIO: %d\n", ret);
> + gpiod_put(dev->reg_on_gpio);
> + dev->reg_on_gpio = NULL;
> + } else
> + BT_DBG("probe - reg_on initially set to %d",
> + poweron_flag);
> + }
> +
> + platform_set_drvdata(pdev, dev);
> + /* Must be done before interrupt is requested */
> + INIT_WORK(&dev->wakeup_work, bcm_wakeup_task);
> +
> + /* Get bt host wake GPIO */
> + dev->bt_wake_gpio = gpiod_get(&pdev->dev, "bt-host-wake");
> + BT_DBG("probe - gpiod_get for bt-host-wake returned %p",
> + dev->bt_wake_gpio);
> + if (IS_ERR(dev->bt_wake_gpio)) {
> + ret = PTR_ERR(dev->bt_wake_gpio);
> + if (ret != -ENOENT)
> + dev_err(&pdev->dev, "probe - bt_wake GPIO: %d\n", ret);
> + dev->bt_wake_gpio = NULL;
> + } else {
> + /* configure bt_wake as input */
> + ret = gpiod_direction_input(dev->bt_wake_gpio);
> + if (ret < 0) {
> + dev_err(&pdev->dev,
> + "probe - set bt_wake GPIO: %d\n", ret);
> + gpiod_put(dev->bt_wake_gpio);
> + dev->bt_wake_gpio = NULL;
> + } else {
> + dev->bt_wake_active_low = gpiod_is_active_low
> + (dev->bt_wake_gpio);
> + BT_DBG("probe -bt_wake a-low is %d(cansleep %d)",
> + dev->bt_wake_active_low,
> + gpiod_cansleep(dev->bt_wake_gpio));
> + dev->bt_wake_irq = gpiod_to_irq
> + (dev->bt_wake_gpio);
> + if (dev->bt_wake_irq < 0)
> + dev_err(&pdev->dev,
> + "probe - HOST_WAKE IRQ: %d\n", ret);
> + else {
> + unsigned long intflags = IRQF_TRIGGER_RISING;
> +
> + if (dev->bt_wake_active_low)
> + intflags = IRQF_TRIGGER_FALLING;
> +
> + ret = request_irq(dev->bt_wake_irq,
> + bcm_uart_isr, intflags,
> + "bt_host_wake", dev);
> + if (ret < 0)
> + dev_err(&pdev->dev,
> + "probe - failed IRQ %d: %d",
> + dev->bt_wake_irq, ret);
> + else
> + BT_DBG("probe - IRQ %d",
> + dev->bt_wake_irq);
> + }
> + }
> + }
> +
> + dev->configure_sleep = of_property_read_bool(np, "configure-sleep");
> + dev->manual_fc = 0;
> + of_property_read_u32(np, "manual-fc", &dev->manual_fc);
> + dev->oper_speed = 3000000;
> + of_property_read_u32(np, "oper-speed", &dev->oper_speed);
> + BT_DBG("oper-speed read as %d", dev->oper_speed);
> + dev->configure_audio = of_property_read_bool(np, "configure-audio");
> + BT_DBG("conf-sleep %d, manual-fc %d, oper-speed %d, conf-audio %d",
> + dev->configure_sleep, dev->manual_fc, dev->oper_speed,
> + dev->configure_audio);
> +
> + if (dev->configure_audio) {
> + /* Defaults for audio */
> + dev->pcm_clockmode = 0;
> + dev->pcm_fillmethod = 2;
> + dev->pcm_fillnum = 0;
> + dev->pcm_fillvalue = 3;
> + dev->pcm_incallbitclock = 0;
> + dev->pcm_lsbfirst = 0;
> + dev->pcm_rightjustify = 0;
> + dev->pcm_routing = 0;
> + dev->pcm_shortframesync = 0;
> + dev->pcm_syncmode = 0;
> +
> + of_property_read_u32(np, "pcm-clockmode", &dev->pcm_clockmode);
> + of_property_read_u32(np, "pcm-fillmethod",
> + &dev->pcm_fillmethod);
> + of_property_read_u32(np, "pcm-fillnum", &dev->pcm_fillnum);
> + of_property_read_u32(np, "pcm-fillvalue", &dev->pcm_fillvalue);
> + of_property_read_u32(np, "pcm-incallbitclock",
> + &dev->pcm_incallbitclock);
> + BT_DBG("pcm-clockmode %d, method %d, num %d, value %d, bit %d",
> + dev->pcm_clockmode, dev->pcm_fillmethod,
> + dev->pcm_fillnum, dev->pcm_fillvalue,
> + dev->pcm_incallbitclock);
> +
> + of_property_read_u32(np, "pcm-lsbfirst", &dev->pcm_lsbfirst);
> + of_property_read_u32(np, "pcm-rightjustify",
> + &dev->pcm_rightjustify);
> + of_property_read_u32(np, "pcm-routing", &dev->pcm_routing);
> + of_property_read_u32(np, "pcm-shortframesync",
> + &dev->pcm_shortframesync);
> + of_property_read_u32(np, "pcm-syncmode", &dev->pcm_syncmode);
> + BT_DBG("pcm-lsb %d, right %d, routing %d, short %d, sync %d",
> + dev->pcm_lsbfirst, dev->pcm_rightjustify,
> + dev->pcm_routing, dev->pcm_shortframesync,
> + dev->pcm_syncmode);
> + }
> +
> + if (!of_property_read_string(np, "tty", &tty_name)) {
> + strcpy(dev->tty_name, tty_name);
> + BT_DBG("tty name read as %s", dev->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(&dev->list, &device_list);
> + spin_unlock(&device_list_lock);
> +
> +end:
> + if (ret) {
> + if (dev->reg_on_gpio) {
> + gpiod_put(dev->reg_on_gpio);
> + dev->reg_on_gpio = NULL;
> + }
> + if (dev->bt_wake_gpio) {
> + gpiod_put(dev->bt_wake_gpio);
> + dev->bt_wake_gpio = NULL;
> + }
> + if (dev->dev_wake_gpio) {
> + gpiod_put(dev->dev_wake_gpio);
> + dev->dev_wake_gpio = NULL;
> + }
> + }
> +
> + BT_DBG("probe with the result %d", ret);
> + return ret;
> +}
I have the feeling we could remove some of the BT_DBG and make the code a bit more readable.
> +
> +/* Device instance removal */
> +static int bcm_uart_remove(struct platform_device *pdev)
> +{
> + struct bcm_device *dev = platform_get_drvdata(pdev);
> +
> + if (!dev) {
> + BT_DBG("remove - logic error, no probe?!");
> + return 0;
> + }
Same question as above, can this actually ever be NULL. Normally remove is not called when probe returns with an error.
> +
> + spin_lock(&device_list_lock);
> + list_del(&dev->list);
> + spin_unlock(&device_list_lock);
> +
> + BT_DBG("remove %p - freeing interrupt %d", dev, dev->bt_wake_irq);
> + free_irq(dev->bt_wake_irq, dev);
> +
> + if (dev->reg_on_gpio) {
> + BT_DBG("remove - releasing reg_on_gpio");
> + gpiod_put(dev->reg_on_gpio);
> + dev->reg_on_gpio = NULL;
> + }
> +
> + if (dev->dev_wake_gpio) {
> + BT_DBG("remove - releasing dev_wake_gpio");
> + gpiod_put(dev->dev_wake_gpio);
> + dev->dev_wake_gpio = NULL;
> + }
> +
> + if (dev->bt_wake_gpio) {
> + BT_DBG("remove - releasing bt_wake_gpio");
> + gpiod_put(dev->bt_wake_gpio);
> + dev->bt_wake_gpio = NULL;
> + }
> +
> + BT_DBG("remove - %p done", dev);
> + return 0;
> +}
> +
> +/* Platform resume callback */
> +static int bcm_uart_resume(struct device *pdev)
> +{
> + struct bcm_device *dev = platform_get_drvdata(
> + to_platform_device(pdev));
> + int gpio_value;
> +
> + if (!dev) {
> + BT_DBG("resume - logic error, no device?!");
> + return 0;
> + }
Same here. I think these are all not needed.
> +
> + BT_DBG("resume %p", dev);
> +
> + gpio_value = !dev->dev_wake_active_low;
> + if (dev->dev_wake_gpio) {
> + gpiod_set_value(dev->dev_wake_gpio, gpio_value);
> + BT_DBG("resume - %d written, delaying 15 ms", gpio_value);
> + mdelay(15);
> + }
> +
> + /* Let the protocol know the platform is resuming */
> + if (dev->protocol_callbacks.p_resume)
> + dev->protocol_callbacks.p_resume(
> + dev->protocol_callbacks.context);
> +
> + return 0;
> +}
> +
> +/* Platform suspend callback */
> +static int bcm_uart_suspend(struct device *pdev)
> +{
> + struct bcm_device *dev = platform_get_drvdata(
> + to_platform_device(pdev));
> + int gpio_value;
> +
> + if (!dev) {
> + BT_DBG("suspend - logic error, no device?!");
> + return 0;
> + }
> +
> + BT_DBG("suspend - %p", dev);
> +
> + /* Let the protocol know the platform is suspending */
> + if (dev->protocol_callbacks.p_suspend)
> + dev->protocol_callbacks.p_suspend(
> + dev->protocol_callbacks.context);
> +
> + /* Suspend the device */
> + if (dev->dev_wake_gpio) {
> + gpio_value = !!dev->dev_wake_active_low;
> + gpiod_set_value(dev->dev_wake_gpio, gpio_value);
> + BT_DBG("suspend - %d written, delaying 15 ms", gpio_value);
> + mdelay(15);
> + }
> +
> + return 0;
> +}
> +
> +/* Entry point for calls from the protocol */
> +void *btbcm_uart_set_callbacks(struct btbcm_uart_callbacks *pc)
> +{
> + struct bcm_device *dev = NULL;
> + bool is_found = false;
> + struct list_head *ptr;
> +
> + BT_DBG("set_callbacks for %s(%p)", pc->name, pc->context);
> +
> + spin_lock(&device_list_lock);
> + list_for_each(ptr, &device_list) {
> + dev = list_entry(ptr, struct bcm_device, list);
> + if (!strcmp(dev->tty_name, pc->name)) {
> + is_found = true;
> + break;
> + }
> + }
> + spin_unlock(&device_list_lock);
> +
> + if (!is_found) {
> + BT_DBG("set_callbacks - no device!");
> + return NULL;
> + }
> +
> + dev->protocol_callbacks = *pc;
> + return dev;
> +}
> +EXPORT_SYMBOL_GPL(btbcm_uart_set_callbacks);
> +
> +void btbcm_uart_reset_callbacks(void *device_context)
> +{
> + struct bcm_device *dev = device_context;
> +
> + memset(&dev->protocol_callbacks, 0, sizeof(dev->protocol_callbacks));
> +}
> +EXPORT_SYMBOL_GPL(btbcm_uart_reset_callbacks);
> +
> +void btbcm_uart_poweron(void *device_context)
> +{
> + struct bcm_device *dev = device_context;
> + int poweron_flag;
> +
> + if (dev->reg_on_gpio) {
> + poweron_flag = !dev->reg_on_active_low;
> + gpiod_set_value(dev->reg_on_gpio, poweron_flag);
> + BT_DBG("poweron %d, delay 15 ms", poweron_flag);
> + }
> +}
> +EXPORT_SYMBOL_GPL(btbcm_uart_poweron);
> +
> +void btbcm_uart_poweroff(void *device_context)
> +{
> + struct bcm_device *dev = device_context;
> + int poweron_flag;
> +
> + if (dev->reg_on_gpio) {
> + poweron_flag = dev->reg_on_active_low;
> + gpiod_set_value(dev->reg_on_gpio, poweron_flag);
> + BT_DBG("poweroff %d, delay 15 ms", poweron_flag);
> + }
> +}
> +EXPORT_SYMBOL_GPL(btbcm_uart_poweroff);
> +
> +void btbcm_uart_suspend(void *device_context)
> +{
> + struct bcm_device *dev = device_context;
> + int gpio_value;
> +
> + if (dev->dev_wake_gpio) {
> + gpio_value = !!dev->dev_wake_active_low;
> + gpiod_set_value(dev->dev_wake_gpio, gpio_value);
> + BT_DBG("suspend %d, delay 15ms", gpio_value);
> + mdelay(15);
> + }
> +}
> +EXPORT_SYMBOL_GPL(btbcm_uart_suspend);
> +
> +void btbcm_uart_resume(void *device_context)
> +{
> + struct bcm_device *dev = device_context;
> + int gpio_value;
> +
> + if (dev->dev_wake_gpio) {
> + gpio_value = !dev->dev_wake_active_low;
> + gpiod_set_value(dev->dev_wake_gpio, gpio_value);
> + BT_DBG("resume %d, delay 15ms", gpio_value);
> + mdelay(15);
> + }
> +}
> +EXPORT_SYMBOL_GPL(btbcm_uart_resume);
> +
> +void btbcm_uart_get_params(void *device_context, struct btbcm_uart_params *pp)
> +{
> + struct bcm_device *dev = device_context;
> +
> + memset(pp, 0, sizeof(struct btbcm_uart_params));
> + pp->configure_sleep = dev->configure_sleep;
> + pp->manual_fc = dev->manual_fc;
> + pp->dev_wake_active_low = dev->dev_wake_active_low;
> + pp->bt_wake_active_low = dev->bt_wake_active_low;
> + pp->idle_timeout_in_secs = idle_timeout;
> + pp->oper_speed = dev->oper_speed;
> + pp->configure_audio = dev->configure_audio;
> + pp->pcm_clockmode = dev->pcm_clockmode;
> + pp->pcm_fillmethod = dev->pcm_fillmethod;
> + pp->pcm_fillnum = dev->pcm_fillnum;
> + pp->pcm_fillvalue = dev->pcm_fillvalue;
> + pp->pcm_incallbitclock = dev->pcm_incallbitclock;
> + pp->pcm_lsbfirst = dev->pcm_lsbfirst;
> + pp->pcm_rightjustify = dev->pcm_rightjustify;
> + pp->pcm_routing = dev->pcm_routing;
> + pp->pcm_shortframesync = dev->pcm_shortframesync;
> + pp->pcm_syncmode = dev->pcm_syncmode;
> +}
> +EXPORT_SYMBOL_GPL(btbcm_uart_get_params);
> +
> +/* Platform susp and resume callbacks */
> +static SIMPLE_DEV_PM_OPS(bcm_uart_pm_ops, bcm_uart_suspend, bcm_uart_resume);
> +
> +/* Driver match table */
> +static const struct of_device_id bcm_uart_table[] = {
> + { .compatible = "brcm,brcm-bt-uart" },
> + {}
> +};
> +
> +/* Driver configuration */
> +static struct platform_driver bcm_uart_driver = {
> + .probe = bcm_uart_probe,
> + .remove = bcm_uart_remove,
> + .driver = {
> + .name = "btbcm_uart",
> + .of_match_table = of_match_ptr(bcm_uart_table),
> + .owner = THIS_MODULE,
> + .pm = &bcm_uart_pm_ops,
> + },
> +};
> +
> +module_platform_driver(bcm_uart_driver);
> +
> +MODULE_AUTHOR("Ilya Faenson <ifaenson@broadcom.com>");
> +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..96dae85
> --- /dev/null
> +++ b/drivers/bluetooth/btbcm_uart.h
> @@ -0,0 +1,74 @@
> +/*
> + * 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.
> + *
> + */
> +
> +/* Callbacks from the driver into the protocol */
> +struct btbcm_uart_callbacks {
> + void *context; /* protocol instance context */
> + char name[64]; /* protocol tty device, for example, ttyS0 */
> +
> + /* client callbacks */
> + void (*p_suspend)(void *context);
> + void (*p_resume)(void *context);
> + void (*p_wakeup)(void *context);
These p_ for pointer need to go. We are not doing that in the kernel.
And do we need to have the void *context? We can not be specific?
> +};
> +
> +/* Driver parameters retrieved from the DT or ACPI */
> +struct btbcm_uart_params {
> + 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 */
> +extern void *btbcm_uart_set_callbacks(struct btbcm_uart_callbacks *pc);
I think we stopped doing extern for exported function a while ago. So this seems unneeded.
> +
> +/* Reset protocol callbacks */
> +extern void btbcm_uart_reset_callbacks(void *device_context);
> +
> +/* Retrieve BT device parameters */
> +extern void btbcm_uart_get_params(void *device_context,
> + struct btbcm_uart_params *pp);
> +
> +/* Resume the BT device via GPIO */
> +extern void btbcm_uart_resume(void *device_context);
> +
> +/* Suspend the BT device via GPIO */
> +extern void btbcm_uart_suspend(void *device_context);
> +
> +/* Power the BT device off via GPIO */
> +extern void btbcm_uart_poweroff(void *device_context);
> +
> +/* Power the BT device on via GPIO */
> +extern void btbcm_uart_poweron(void *device_context);
> +
Regards
Marcel
^ permalink raw reply [flat|nested] 12+ messages in thread* RE: [PATCH v3 3/5] Broadcom Bluetooth UART Platform Driver
2015-06-17 17:14 ` Marcel Holtmann
@ 2015-06-17 20:01 ` Ilya Faenson
0 siblings, 0 replies; 12+ messages in thread
From: Ilya Faenson @ 2015-06-17 20:01 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth@vger.kernel.org, Arend Van Spriel
Hi Marcel,
-----Original Message-----
From: Marcel Holtmann [mailto:marcel@holtmann.org]
Sent: Wednesday, June 17, 2015 1:15 PM
To: Ilya Faenson
Cc: linux-bluetooth@vger.kernel.org; Arend Van Spriel
Subject: Re: [PATCH v3 3/5] Broadcom Bluetooth UART Platform Driver
Hi Ilya,
> Introduce the device tree enumerated Broadcom Bluetooth UART driver.
don't be shy and give a bit of more detailed explanation on what the driver is doing ;)
IF: Will do. I feel though you've been looking at numerous iterations of that code so much you should be intimately familiar with it by now. :-)
>
> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
> ---
> drivers/bluetooth/Makefile | 1 +
> drivers/bluetooth/btbcm_uart.c | 580 +++++++++++++++++++++++++++++++++++++++++
> drivers/bluetooth/btbcm_uart.h | 74 ++++++
> 3 files changed, 655 insertions(+)
> create mode 100644 drivers/bluetooth/btbcm_uart.c
> create mode 100644 drivers/bluetooth/btbcm_uart.h
>
> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
> index f40e194..7947abb 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_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..8069997
> --- /dev/null
> +++ b/drivers/bluetooth/btbcm_uart.c
> @@ -0,0 +1,580 @@
> +/*
> + * 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 assumed we agreed on that we add the module parameter as a separate patch. It was still not clear to me if this is just for debugging purposes or not. It sounded more like for fine tuning and once it is fine tuned it stays that way.
IF: I will move this parameter into the device tree node where all other parameters are. It will change very infrequently.
> +
> +/* 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;
Don't you want to just call it callbacks here. I think that it will make overall code less awkward and will cause less line breaks.
IF: Will rename.
> + 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_wakeup_task(struct work_struct *work)
> +{
> + struct bcm_device *dev = container_of(work, struct bcm_device,
> + wakeup_work);
> + int gpio_value;
> +
> + if (!dev)
> + return;
Can this be actually NULL?
IF: I was concerned here about a race condition when the device is being removed and the interrupt is triggering. The free_irq is synchronous though so that should not happen. Will remove.
> +
> + /* Make sure the device is resumed */
> + gpio_value = !dev->dev_wake_active_low;
> + if (dev->dev_wake_gpio) {
> + gpiod_set_value(dev->dev_wake_gpio, gpio_value);
> + BT_DBG("wakeup_task - resume %d written, delaying 15 ms",
> + gpio_value);
> + mdelay(15);
> + }
> +
> + /* Let the protocol know it's time to wake up */
> + if (dev->protocol_callbacks.p_wakeup)
> + dev->protocol_callbacks.p_wakeup(
> + dev->protocol_callbacks.context);
> +}
> +
> +/* Interrupt routine for the wake from the device */
> +static irqreturn_t bcm_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("isr with bt_wake of %d (active_low %d), req bh",
> + 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_uart_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + const char *tty_name;
> + int ret;
> + struct bcm_device *dev;
> +
> + dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> + if (!dev) {
> + BT_DBG("probe - failing due to no memory");
Honestly, do not even bother here. This malloc will almost never fail. There is a funny LWN.net article on how the kernel will almost always give you the memory.
IF: Okay, will remove this trace.
> + return -ENOMEM;
> + }
> +
> + dev->pdev = pdev;
> +
> + /* Get dev wake GPIO */
> + dev->dev_wake_gpio = gpiod_get(&pdev->dev, "bt-wake");
> + BT_DBG("probe %p - gpiod_get for bt-wake returned %p", dev,
> + dev->dev_wake_gpio);
> + if (IS_ERR(dev->dev_wake_gpio)) {
> + ret = PTR_ERR(dev->dev_wake_gpio);
> + if (ret != -ENOENT)
> + dev_err(&pdev->dev, "probe - dev_wake GPIO: %d\n", ret);
> + dev->dev_wake_gpio = NULL;
> + } else {
> + int gpio_value;
> +
> + dev->dev_wake_active_low = gpiod_is_active_low
> + (dev->dev_wake_gpio);
> + BT_DBG("probe - dev_wake a-low is %d (cans %d)",
> + dev->dev_wake_active_low,
> + gpiod_cansleep(dev->dev_wake_gpio));
> +
> + /* configure dev_wake as output with init resumed state */
> + gpio_value = !dev->dev_wake_active_low;
> + ret = gpiod_direction_output(dev->dev_wake_gpio, gpio_value);
> + if (ret < 0) {
> + dev_err(&pdev->dev,
> + "probe - s dev_wake GPIO: %d\n", ret);
> + gpiod_put(dev->dev_wake_gpio);
> + dev->dev_wake_gpio = NULL;
> + goto end;
> + } else {
> + BT_DBG("probe - dev_wake set to %d", gpio_value);
> + }
> + }
> +
> + /* Get power on/off GPIO */
> + dev->reg_on_gpio = gpiod_get(&pdev->dev, "bt-reg-on");
> + BT_DBG("probe - gpiod_get for bt-reg-on returned %p", dev->reg_on_gpio);
> + if (IS_ERR(dev->reg_on_gpio)) {
> + ret = PTR_ERR(dev->reg_on_gpio);
> + if (ret != -ENOENT)
> + dev_err(&pdev->dev, "probe - reg_on GPIO: %d\n", ret);
> + dev->reg_on_gpio = NULL;
> + } else {
> + int poweron_flag;
> +
> + dev->reg_on_active_low = gpiod_is_active_low(dev->reg_on_gpio);
> + BT_DBG("probe - reg_on a-low is %d (cansleep %d)",
> + dev->reg_on_active_low,
> + gpiod_cansleep(dev->reg_on_gpio));
> +
> + /* configure reg_on as output with init on state */
> + poweron_flag = !dev->reg_on_active_low;
> + ret = gpiod_direction_output(dev->reg_on_gpio, poweron_flag);
> + if (ret < 0) {
> + dev_err(&pdev->dev,
> + "probe - set reg_on GPIO: %d\n", ret);
> + gpiod_put(dev->reg_on_gpio);
> + dev->reg_on_gpio = NULL;
> + } else
> + BT_DBG("probe - reg_on initially set to %d",
> + poweron_flag);
> + }
> +
> + platform_set_drvdata(pdev, dev);
> + /* Must be done before interrupt is requested */
> + INIT_WORK(&dev->wakeup_work, bcm_wakeup_task);
> +
> + /* Get bt host wake GPIO */
> + dev->bt_wake_gpio = gpiod_get(&pdev->dev, "bt-host-wake");
> + BT_DBG("probe - gpiod_get for bt-host-wake returned %p",
> + dev->bt_wake_gpio);
> + if (IS_ERR(dev->bt_wake_gpio)) {
> + ret = PTR_ERR(dev->bt_wake_gpio);
> + if (ret != -ENOENT)
> + dev_err(&pdev->dev, "probe - bt_wake GPIO: %d\n", ret);
> + dev->bt_wake_gpio = NULL;
> + } else {
> + /* configure bt_wake as input */
> + ret = gpiod_direction_input(dev->bt_wake_gpio);
> + if (ret < 0) {
> + dev_err(&pdev->dev,
> + "probe - set bt_wake GPIO: %d\n", ret);
> + gpiod_put(dev->bt_wake_gpio);
> + dev->bt_wake_gpio = NULL;
> + } else {
> + dev->bt_wake_active_low = gpiod_is_active_low
> + (dev->bt_wake_gpio);
> + BT_DBG("probe -bt_wake a-low is %d(cansleep %d)",
> + dev->bt_wake_active_low,
> + gpiod_cansleep(dev->bt_wake_gpio));
> + dev->bt_wake_irq = gpiod_to_irq
> + (dev->bt_wake_gpio);
> + if (dev->bt_wake_irq < 0)
> + dev_err(&pdev->dev,
> + "probe - HOST_WAKE IRQ: %d\n", ret);
> + else {
> + unsigned long intflags = IRQF_TRIGGER_RISING;
> +
> + if (dev->bt_wake_active_low)
> + intflags = IRQF_TRIGGER_FALLING;
> +
> + ret = request_irq(dev->bt_wake_irq,
> + bcm_uart_isr, intflags,
> + "bt_host_wake", dev);
> + if (ret < 0)
> + dev_err(&pdev->dev,
> + "probe - failed IRQ %d: %d",
> + dev->bt_wake_irq, ret);
> + else
> + BT_DBG("probe - IRQ %d",
> + dev->bt_wake_irq);
> + }
> + }
> + }
> +
> + dev->configure_sleep = of_property_read_bool(np, "configure-sleep");
> + dev->manual_fc = 0;
> + of_property_read_u32(np, "manual-fc", &dev->manual_fc);
> + dev->oper_speed = 3000000;
> + of_property_read_u32(np, "oper-speed", &dev->oper_speed);
> + BT_DBG("oper-speed read as %d", dev->oper_speed);
> + dev->configure_audio = of_property_read_bool(np, "configure-audio");
> + BT_DBG("conf-sleep %d, manual-fc %d, oper-speed %d, conf-audio %d",
> + dev->configure_sleep, dev->manual_fc, dev->oper_speed,
> + dev->configure_audio);
> +
> + if (dev->configure_audio) {
> + /* Defaults for audio */
> + dev->pcm_clockmode = 0;
> + dev->pcm_fillmethod = 2;
> + dev->pcm_fillnum = 0;
> + dev->pcm_fillvalue = 3;
> + dev->pcm_incallbitclock = 0;
> + dev->pcm_lsbfirst = 0;
> + dev->pcm_rightjustify = 0;
> + dev->pcm_routing = 0;
> + dev->pcm_shortframesync = 0;
> + dev->pcm_syncmode = 0;
> +
> + of_property_read_u32(np, "pcm-clockmode", &dev->pcm_clockmode);
> + of_property_read_u32(np, "pcm-fillmethod",
> + &dev->pcm_fillmethod);
> + of_property_read_u32(np, "pcm-fillnum", &dev->pcm_fillnum);
> + of_property_read_u32(np, "pcm-fillvalue", &dev->pcm_fillvalue);
> + of_property_read_u32(np, "pcm-incallbitclock",
> + &dev->pcm_incallbitclock);
> + BT_DBG("pcm-clockmode %d, method %d, num %d, value %d, bit %d",
> + dev->pcm_clockmode, dev->pcm_fillmethod,
> + dev->pcm_fillnum, dev->pcm_fillvalue,
> + dev->pcm_incallbitclock);
> +
> + of_property_read_u32(np, "pcm-lsbfirst", &dev->pcm_lsbfirst);
> + of_property_read_u32(np, "pcm-rightjustify",
> + &dev->pcm_rightjustify);
> + of_property_read_u32(np, "pcm-routing", &dev->pcm_routing);
> + of_property_read_u32(np, "pcm-shortframesync",
> + &dev->pcm_shortframesync);
> + of_property_read_u32(np, "pcm-syncmode", &dev->pcm_syncmode);
> + BT_DBG("pcm-lsb %d, right %d, routing %d, short %d, sync %d",
> + dev->pcm_lsbfirst, dev->pcm_rightjustify,
> + dev->pcm_routing, dev->pcm_shortframesync,
> + dev->pcm_syncmode);
> + }
> +
> + if (!of_property_read_string(np, "tty", &tty_name)) {
> + strcpy(dev->tty_name, tty_name);
> + BT_DBG("tty name read as %s", dev->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(&dev->list, &device_list);
> + spin_unlock(&device_list_lock);
> +
> +end:
> + if (ret) {
> + if (dev->reg_on_gpio) {
> + gpiod_put(dev->reg_on_gpio);
> + dev->reg_on_gpio = NULL;
> + }
> + if (dev->bt_wake_gpio) {
> + gpiod_put(dev->bt_wake_gpio);
> + dev->bt_wake_gpio = NULL;
> + }
> + if (dev->dev_wake_gpio) {
> + gpiod_put(dev->dev_wake_gpio);
> + dev->dev_wake_gpio = NULL;
> + }
> + }
> +
> + BT_DBG("probe with the result %d", ret);
> + return ret;
> +}
I have the feeling we could remove some of the BT_DBG and make the code a bit more readable.
IF: Will remove a few more trace calls.
> +
> +/* Device instance removal */
> +static int bcm_uart_remove(struct platform_device *pdev)
> +{
> + struct bcm_device *dev = platform_get_drvdata(pdev);
> +
> + if (!dev) {
> + BT_DBG("remove - logic error, no probe?!");
> + return 0;
> + }
Same question as above, can this actually ever be NULL. Normally remove is not called when probe returns with an error.
IF: It may happen upon a logic error. :-) Will remove.
> +
> + spin_lock(&device_list_lock);
> + list_del(&dev->list);
> + spin_unlock(&device_list_lock);
> +
> + BT_DBG("remove %p - freeing interrupt %d", dev, dev->bt_wake_irq);
> + free_irq(dev->bt_wake_irq, dev);
> +
> + if (dev->reg_on_gpio) {
> + BT_DBG("remove - releasing reg_on_gpio");
> + gpiod_put(dev->reg_on_gpio);
> + dev->reg_on_gpio = NULL;
> + }
> +
> + if (dev->dev_wake_gpio) {
> + BT_DBG("remove - releasing dev_wake_gpio");
> + gpiod_put(dev->dev_wake_gpio);
> + dev->dev_wake_gpio = NULL;
> + }
> +
> + if (dev->bt_wake_gpio) {
> + BT_DBG("remove - releasing bt_wake_gpio");
> + gpiod_put(dev->bt_wake_gpio);
> + dev->bt_wake_gpio = NULL;
> + }
> +
> + BT_DBG("remove - %p done", dev);
> + return 0;
> +}
> +
> +/* Platform resume callback */
> +static int bcm_uart_resume(struct device *pdev)
> +{
> + struct bcm_device *dev = platform_get_drvdata(
> + to_platform_device(pdev));
> + int gpio_value;
> +
> + if (!dev) {
> + BT_DBG("resume - logic error, no device?!");
> + return 0;
> + }
Same here. I think these are all not needed.
IF: Will remove.
> +
> + BT_DBG("resume %p", dev);
> +
> + gpio_value = !dev->dev_wake_active_low;
> + if (dev->dev_wake_gpio) {
> + gpiod_set_value(dev->dev_wake_gpio, gpio_value);
> + BT_DBG("resume - %d written, delaying 15 ms", gpio_value);
> + mdelay(15);
> + }
> +
> + /* Let the protocol know the platform is resuming */
> + if (dev->protocol_callbacks.p_resume)
> + dev->protocol_callbacks.p_resume(
> + dev->protocol_callbacks.context);
> +
> + return 0;
> +}
> +
> +/* Platform suspend callback */
> +static int bcm_uart_suspend(struct device *pdev)
> +{
> + struct bcm_device *dev = platform_get_drvdata(
> + to_platform_device(pdev));
> + int gpio_value;
> +
> + if (!dev) {
> + BT_DBG("suspend - logic error, no device?!");
> + return 0;
> + }
> +
> + BT_DBG("suspend - %p", dev);
> +
> + /* Let the protocol know the platform is suspending */
> + if (dev->protocol_callbacks.p_suspend)
> + dev->protocol_callbacks.p_suspend(
> + dev->protocol_callbacks.context);
> +
> + /* Suspend the device */
> + if (dev->dev_wake_gpio) {
> + gpio_value = !!dev->dev_wake_active_low;
> + gpiod_set_value(dev->dev_wake_gpio, gpio_value);
> + BT_DBG("suspend - %d written, delaying 15 ms", gpio_value);
> + mdelay(15);
> + }
> +
> + return 0;
> +}
> +
> +/* Entry point for calls from the protocol */
> +void *btbcm_uart_set_callbacks(struct btbcm_uart_callbacks *pc)
> +{
> + struct bcm_device *dev = NULL;
> + bool is_found = false;
> + struct list_head *ptr;
> +
> + BT_DBG("set_callbacks for %s(%p)", pc->name, pc->context);
> +
> + spin_lock(&device_list_lock);
> + list_for_each(ptr, &device_list) {
> + dev = list_entry(ptr, struct bcm_device, list);
> + if (!strcmp(dev->tty_name, pc->name)) {
> + is_found = true;
> + break;
> + }
> + }
> + spin_unlock(&device_list_lock);
> +
> + if (!is_found) {
> + BT_DBG("set_callbacks - no device!");
> + return NULL;
> + }
> +
> + dev->protocol_callbacks = *pc;
> + return dev;
> +}
> +EXPORT_SYMBOL_GPL(btbcm_uart_set_callbacks);
> +
> +void btbcm_uart_reset_callbacks(void *device_context)
> +{
> + struct bcm_device *dev = device_context;
> +
> + memset(&dev->protocol_callbacks, 0, sizeof(dev->protocol_callbacks));
> +}
> +EXPORT_SYMBOL_GPL(btbcm_uart_reset_callbacks);
> +
> +void btbcm_uart_poweron(void *device_context)
> +{
> + struct bcm_device *dev = device_context;
> + int poweron_flag;
> +
> + if (dev->reg_on_gpio) {
> + poweron_flag = !dev->reg_on_active_low;
> + gpiod_set_value(dev->reg_on_gpio, poweron_flag);
> + BT_DBG("poweron %d, delay 15 ms", poweron_flag);
> + }
> +}
> +EXPORT_SYMBOL_GPL(btbcm_uart_poweron);
> +
> +void btbcm_uart_poweroff(void *device_context)
> +{
> + struct bcm_device *dev = device_context;
> + int poweron_flag;
> +
> + if (dev->reg_on_gpio) {
> + poweron_flag = dev->reg_on_active_low;
> + gpiod_set_value(dev->reg_on_gpio, poweron_flag);
> + BT_DBG("poweroff %d, delay 15 ms", poweron_flag);
> + }
> +}
> +EXPORT_SYMBOL_GPL(btbcm_uart_poweroff);
> +
> +void btbcm_uart_suspend(void *device_context)
> +{
> + struct bcm_device *dev = device_context;
> + int gpio_value;
> +
> + if (dev->dev_wake_gpio) {
> + gpio_value = !!dev->dev_wake_active_low;
> + gpiod_set_value(dev->dev_wake_gpio, gpio_value);
> + BT_DBG("suspend %d, delay 15ms", gpio_value);
> + mdelay(15);
> + }
> +}
> +EXPORT_SYMBOL_GPL(btbcm_uart_suspend);
> +
> +void btbcm_uart_resume(void *device_context)
> +{
> + struct bcm_device *dev = device_context;
> + int gpio_value;
> +
> + if (dev->dev_wake_gpio) {
> + gpio_value = !dev->dev_wake_active_low;
> + gpiod_set_value(dev->dev_wake_gpio, gpio_value);
> + BT_DBG("resume %d, delay 15ms", gpio_value);
> + mdelay(15);
> + }
> +}
> +EXPORT_SYMBOL_GPL(btbcm_uart_resume);
> +
> +void btbcm_uart_get_params(void *device_context, struct btbcm_uart_params *pp)
> +{
> + struct bcm_device *dev = device_context;
> +
> + memset(pp, 0, sizeof(struct btbcm_uart_params));
> + pp->configure_sleep = dev->configure_sleep;
> + pp->manual_fc = dev->manual_fc;
> + pp->dev_wake_active_low = dev->dev_wake_active_low;
> + pp->bt_wake_active_low = dev->bt_wake_active_low;
> + pp->idle_timeout_in_secs = idle_timeout;
> + pp->oper_speed = dev->oper_speed;
> + pp->configure_audio = dev->configure_audio;
> + pp->pcm_clockmode = dev->pcm_clockmode;
> + pp->pcm_fillmethod = dev->pcm_fillmethod;
> + pp->pcm_fillnum = dev->pcm_fillnum;
> + pp->pcm_fillvalue = dev->pcm_fillvalue;
> + pp->pcm_incallbitclock = dev->pcm_incallbitclock;
> + pp->pcm_lsbfirst = dev->pcm_lsbfirst;
> + pp->pcm_rightjustify = dev->pcm_rightjustify;
> + pp->pcm_routing = dev->pcm_routing;
> + pp->pcm_shortframesync = dev->pcm_shortframesync;
> + pp->pcm_syncmode = dev->pcm_syncmode;
> +}
> +EXPORT_SYMBOL_GPL(btbcm_uart_get_params);
> +
> +/* Platform susp and resume callbacks */
> +static SIMPLE_DEV_PM_OPS(bcm_uart_pm_ops, bcm_uart_suspend, bcm_uart_resume);
> +
> +/* Driver match table */
> +static const struct of_device_id bcm_uart_table[] = {
> + { .compatible = "brcm,brcm-bt-uart" },
> + {}
> +};
> +
> +/* Driver configuration */
> +static struct platform_driver bcm_uart_driver = {
> + .probe = bcm_uart_probe,
> + .remove = bcm_uart_remove,
> + .driver = {
> + .name = "btbcm_uart",
> + .of_match_table = of_match_ptr(bcm_uart_table),
> + .owner = THIS_MODULE,
> + .pm = &bcm_uart_pm_ops,
> + },
> +};
> +
> +module_platform_driver(bcm_uart_driver);
> +
> +MODULE_AUTHOR("Ilya Faenson <ifaenson@broadcom.com>");
> +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..96dae85
> --- /dev/null
> +++ b/drivers/bluetooth/btbcm_uart.h
> @@ -0,0 +1,74 @@
> +/*
> + * 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.
> + *
> + */
> +
> +/* Callbacks from the driver into the protocol */
> +struct btbcm_uart_callbacks {
> + void *context; /* protocol instance context */
> + char name[64]; /* protocol tty device, for example, ttyS0 */
> +
> + /* client callbacks */
> + void (*p_suspend)(void *context);
> + void (*p_resume)(void *context);
> + void (*p_wakeup)(void *context);
These p_ for pointer need to go. We are not doing that in the kernel.
IF: Will remove.
And do we need to have the void *context? We can not be specific?
IF: We cannot be specific here because the context is the bcm_device internal to the btbcm_uart and not published to the protocol.
> +};
> +
> +/* Driver parameters retrieved from the DT or ACPI */
> +struct btbcm_uart_params {
> + 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 */
> +extern void *btbcm_uart_set_callbacks(struct btbcm_uart_callbacks *pc);
I think we stopped doing extern for exported function a while ago. So this seems unneeded.
IF: Will remove.
> +
> +/* Reset protocol callbacks */
> +extern void btbcm_uart_reset_callbacks(void *device_context);
> +
> +/* Retrieve BT device parameters */
> +extern void btbcm_uart_get_params(void *device_context,
> + struct btbcm_uart_params *pp);
> +
> +/* Resume the BT device via GPIO */
> +extern void btbcm_uart_resume(void *device_context);
> +
> +/* Suspend the BT device via GPIO */
> +extern void btbcm_uart_suspend(void *device_context);
> +
> +/* Power the BT device off via GPIO */
> +extern void btbcm_uart_poweroff(void *device_context);
> +
> +/* Power the BT device on via GPIO */
> +extern void btbcm_uart_poweron(void *device_context);
> +
Regards
Marcel
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 4/5] Support the BCM4354 Bluetooth UART device
2015-06-17 15:42 [PATCH v3 0/5] Broadcom Bluetooth UART device driver Ilya Faenson
` (2 preceding siblings ...)
2015-06-17 15:42 ` [PATCH v3 3/5] Broadcom Bluetooth UART Platform Driver Ilya Faenson
@ 2015-06-17 15:42 ` Ilya Faenson
2015-06-17 16:59 ` Marcel Holtmann
2015-06-17 15:42 ` [PATCH v3 5/5] BlueZ Broadcom UART Protocol Ilya Faenson
4 siblings, 1 reply; 12+ messages in thread
From: Ilya Faenson @ 2015-06-17 15:42 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth, Arend van Spriel, Ilya Faenson
Support the BCM4354 chip and introduce vendor specific command
parameter definitions.
Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
---
drivers/bluetooth/btbcm.c | 2 ++
drivers/bluetooth/btbcm.h | 31 +++++++++++++++++++++++++++++++
2 files changed, 33 insertions(+)
diff --git a/drivers/bluetooth/btbcm.c b/drivers/bluetooth/btbcm.c
index 8e2f6b6..1e1a432 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 */
{ }
};
@@ -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) {
diff --git a/drivers/bluetooth/btbcm.h b/drivers/bluetooth/btbcm.h
index 02f5f96..d9e6b41 100644
--- a/drivers/bluetooth/btbcm.h
+++ b/drivers/bluetooth/btbcm.h
@@ -33,6 +33,37 @@ struct bcm_write_uart_clock_setting {
__u8 type;
} __packed;
+struct bcm_set_sleep_mode {
+ __u8 sleep_mode;
+ __u8 idle_host;
+ __u8 idle_dev;
+ __u8 bt_wake_active;
+ __u8 host_wake_active;
+ __u8 allow_host_sleep;
+ __u8 combine_modes;
+ __u8 tristate_control;
+ __u8 usb_auto_sleep;
+ __u8 usb_resume_timeout;
+ __u8 pulsed_host_wake;
+ __u8 break_to_host;
+} __packed;
+
+struct bcm_set_pcm_int_params {
+ __u8 routing;
+ __u8 rate;
+ __u8 frame_sync;
+ __u8 sync_mode;
+ __u8 clock_mode;
+} __packed;
+
+struct bcm_set_pcm_format_params {
+ __u8 lsb_first;
+ __u8 fill_value;
+ __u8 fill_method;
+ __u8 fill_num;
+ __u8 right_justify;
+} __packed;
+
#if IS_ENABLED(CONFIG_BT_BCM)
int btbcm_check_bdaddr(struct hci_dev *hdev);
--
1.9.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v3 4/5] Support the BCM4354 Bluetooth UART device
2015-06-17 15:42 ` [PATCH v3 4/5] Support the BCM4354 Bluetooth UART device Ilya Faenson
@ 2015-06-17 16:59 ` Marcel Holtmann
0 siblings, 0 replies; 12+ messages in thread
From: Marcel Holtmann @ 2015-06-17 16:59 UTC (permalink / raw)
To: Ilya Faenson; +Cc: linux-bluetooth, Arend van Spriel
Hi Ilya,
> Support the BCM4354 chip and introduce vendor specific command
> parameter definitions.
>
> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
> ---
> drivers/bluetooth/btbcm.c | 2 ++
> drivers/bluetooth/btbcm.h | 31 +++++++++++++++++++++++++++++++
> 2 files changed, 33 insertions(+)
patch has been applied to bluetooth-next tree.
I decide to apply this one since it is self-contained and causes no harm to be applied ahead of time. However I had fix up the subject line. You want to prefix this with Bluetooth: btbcm: or similar in the future. If in doubt look at what the last patches touching some of the files have used as prefix.
Regards
Marcel
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3 5/5] BlueZ Broadcom UART Protocol
2015-06-17 15:42 [PATCH v3 0/5] Broadcom Bluetooth UART device driver Ilya Faenson
` (3 preceding siblings ...)
2015-06-17 15:42 ` [PATCH v3 4/5] Support the BCM4354 Bluetooth UART device Ilya Faenson
@ 2015-06-17 15:42 ` Ilya Faenson
4 siblings, 0 replies; 12+ messages in thread
From: Ilya Faenson @ 2015-06-17 15:42 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth, Arend van Spriel, 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 | 340 ++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 332 insertions(+), 8 deletions(-)
diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index aa3c9ac..fe18651 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,161 @@
#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_params params; /* device parameters */
+ void *device_context; /* ACPI/DT device context */
};
+/* Suspend/resume synchronization mutex */
+static DEFINE_MUTEX(power_lock);
+
+/* Useful timer restart helper */
+static void restart_idle_timer(struct hci_uart *hu)
+{
+ struct bcm_data *h4 = hu->priv;
+
+ mod_timer(&h4->timer, jiffies +
+ msecs_to_jiffies(h4->params.idle_timeout_in_secs * 1000));
+}
+
+/* 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("suspend: is_suspended %d", h4->is_suspended);
+
+ if (!h4->params.configure_sleep)
+ return;
+
+ if (!h4->is_suspended) {
+ if (h4->params.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("resume: is_suspended %d", h4->is_suspended);
+
+ if (!h4->params.configure_sleep)
+ return;
+
+ /* When this callback executes, the device has woken up already */
+ if (h4->is_suspended) {
+ h4->is_suspended = false;
+
+ if (h4->params.manual_fc)
+ hci_uart_set_flow_control(hu, false);
+ }
+
+ /* If we're resumed, the idle timer must be running */
+ restart_idle_timer(hu);
+}
+
+/*
+ * 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("wakeup: is_suspended %d", h4->is_suspended);
+
+ if (!h4->params.configure_sleep)
+ return;
+
+ if (h4->is_suspended) {
+ if (h4->params.manual_fc)
+ hci_uart_set_flow_control(hu, false);
+
+ h4->is_suspended = false;
+ }
+
+ /* If we're resumed, the idle timer must be running */
+ restart_idle_timer(hu);
+}
+
+/*
+ * 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;
+
+ if (!h4->params.configure_sleep)
+ return;
+
+ /* Suspend/resume operations are serialized */
+ mutex_lock(&power_lock);
+
+ /* Nothing to do if resumed already */
+ if (!h4->is_suspended) {
+ mutex_unlock(&power_lock);
+
+ /* Just reset the timer */
+ restart_idle_timer(hu);
+ return;
+ }
+
+ /* Wakeup the device */
+ btbcm_uart_resume(h4->device_context);
+
+ /* Unflow control the port if configured */
+ resume_notification(hu);
+
+ mutex_unlock(&power_lock);
+}
+
+/*
+ * 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;
+
+ BT_DBG("idle_timeout: hu %p", hu);
+
+ /* Suspend/resume operations are serialized */
+ mutex_lock(&power_lock);
+
+ if (!h4->is_suspended) {
+ /* Flow control the port if configured */
+ suspend_notification(hu);
+
+ /* Suspend the device */
+ btbcm_uart_suspend(h4->device_context);
+ }
+
+ mutex_unlock(&power_lock);
+}
+
static int bcm_set_baudrate(struct hci_uart *hu, unsigned int speed)
{
struct hci_dev *hdev = hu->hdev;
@@ -89,6 +240,8 @@ 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;
+ struct tty_struct *tty = hu->tty;
BT_DBG("hu %p", hu);
@@ -99,6 +252,48 @@ static int bcm_open(struct hci_uart *hu)
skb_queue_head_init(&bcm->txq);
hu->priv = bcm;
+ bcm->hu = hu;
+ bcm->is_suspended = false;
+ memset(&bcm->params, 0, sizeof(bcm->params));
+
+ /* Reset UART to a default state */
+ hci_uart_init_tty(hu);
+
+ /* Configure callbacks on the driver */
+ callbacks.context = hu;
+ strcpy(callbacks.name, tty->name);
+ callbacks.p_suspend = suspend_notification;
+ callbacks.p_resume = resume_notification;
+ callbacks.p_wakeup = wakeup_notification;
+ bcm->device_context = btbcm_uart_set_callbacks(&callbacks);
+ if (!bcm->device_context) {
+ /* That is likely an indication of no bcm driver present */
+ BT_DBG("Callback set failure (no driver present)");
+
+ /* Using speed defaults */
+ hci_uart_set_speeds(hu, 115200, 3000000);
+ return 0;
+ }
+
+ /* Retrieve device parameters */
+ btbcm_uart_get_params(bcm->device_context, &bcm->params);
+ BT_DBG("Context %p conf_sleep %d dev_actlow %d bt_actlow %d idle_t %d",
+ bcm->device_context, bcm->params.configure_sleep,
+ bcm->params.dev_wake_active_low, bcm->params.bt_wake_active_low,
+ bcm->params.idle_timeout_in_secs);
+ hci_uart_set_speeds(hu, 115200, bcm->params.oper_speed);
+
+ /* Cycle power to make sure the device is in the known state */
+ btbcm_uart_poweroff(bcm->device_context);
+ btbcm_uart_poweron(bcm->device_context);
+
+ /* Start the idle timer */
+ if (bcm->params.configure_sleep) {
+ setup_timer(&bcm->timer, bcm_idle_timeout, (unsigned long)hu);
+ if (bcm->params.configure_sleep)
+ restart_idle_timer(hu);
+ }
+
return 0;
}
@@ -108,6 +303,31 @@ static int bcm_close(struct hci_uart *hu)
BT_DBG("hu %p", hu);
+ /* If we're being closed, we must suspend */
+ if (bcm->params.configure_sleep) {
+ mutex_lock(&power_lock);
+
+ if (!bcm->is_suspended) {
+ /* Flow control the port */
+ suspend_notification(hu);
+
+ /* Suspend the device */
+ btbcm_uart_suspend(bcm->device_context);
+ }
+
+ mutex_unlock(&power_lock);
+
+ del_timer_sync(&bcm->timer);
+ }
+
+ /* Power off the device if possible */
+ if (bcm->device_context) {
+ btbcm_uart_poweroff(bcm->device_context);
+
+ /* de-configure callbacks on the driver */
+ btbcm_uart_reset_callbacks(bcm->device_context);
+ }
+
skb_queue_purge(&bcm->txq);
kfree_skb(bcm->rx_skb);
kfree(bcm);
@@ -129,9 +349,12 @@ 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;
+ struct sk_buff *skb;
+ struct bcm_data *h4 = hu->priv;
+ unsigned char time_slot_number = 0;
BT_DBG("hu %p", hu);
@@ -153,13 +376,110 @@ static int bcm_setup(struct hci_uart *hu)
goto finalize;
}
- if (hu->proto->init_speed)
- hci_uart_set_baudrate(hu, hu->proto->init_speed);
+ if (hu->init_speed)
+ hci_uart_set_baudrate(hu, hu->init_speed);
- if (hu->proto->oper_speed) {
- err = bcm_set_baudrate(hu, hu->proto->oper_speed);
+ if (hu->oper_speed) {
+ err = bcm_set_baudrate(hu, hu->oper_speed);
if (!err)
- hci_uart_set_baudrate(hu, hu->proto->oper_speed);
+ hci_uart_set_baudrate(hu, hu->oper_speed);
+ }
+
+ /* Configure SCO PCM parameters */
+ if (h4->params.configure_audio) {
+ struct bcm_set_pcm_int_params pcm_int_params;
+ struct bcm_set_pcm_format_params pcm_format_params;
+
+ /* 0=PCM routing, 1=SCO over HCI */
+ pcm_int_params.routing = h4->params.pcm_routing;
+ /* 0=128Kbps,1=256Kbps,2=512Kbps,3=1024Kbps,4=2048Kbps */
+ pcm_int_params.rate = h4->params.pcm_incallbitclock;
+ /* short frame sync 0=short, 1=long */
+ pcm_int_params.frame_sync = h4->params.pcm_shortframesync;
+ /* sync mode 0=slave, 1=master */
+ pcm_int_params.sync_mode = h4->params.pcm_syncmode;
+ /* clock mode 0=slave, 1=master */
+ pcm_int_params.clock_mode = h4->params.pcm_clockmode;
+
+ skb = __hci_cmd_sync(hu->hdev, 0xfc1c, sizeof(pcm_int_params),
+ &pcm_int_params, 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");
+
+ /* LSB_First 1=TRUE, 0=FALSE */
+ pcm_format_params.lsb_first = h4->params.pcm_lsbfirst;
+ /* Fill_Value (use 0-7 for fill bits value) */
+ pcm_format_params.fill_value = h4->params.pcm_fillvalue;
+ /* Fill_Method (2=sign extended) */
+ pcm_format_params.fill_method = h4->params.pcm_fillmethod;
+ /* Fill_Num # of fill bits 0-3) */
+ pcm_format_params.fill_num = h4->params.pcm_fillnum;
+ /* Right_Justify 1=TRUE, 0=FALSE */
+ pcm_format_params.right_justify = h4->params.pcm_rightjustify;
+
+ skb = __hci_cmd_sync(hu->hdev, 0xfc1e,
+ sizeof(pcm_format_params),
+ &pcm_format_params, 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->params.configure_sleep) {
+ struct bcm_set_sleep_mode sleep_params;
+
+ sleep_params.sleep_mode = 1; /* UART */
+ sleep_params.idle_host = 2; /* idle threshold HOST, in 300ms */
+ sleep_params.idle_dev = 2; /* idle threshold device, in 300ms */
+ /* BT_WAKE active mode - 1=active high, 0 = active low */
+ sleep_params.bt_wake_active = (unsigned char)
+ !h4->params.bt_wake_active_low;
+ /* HOST_WAKE active mode - 1=active high, 0 = active low */
+ sleep_params.host_wake_active = (unsigned char)
+ !h4->params.dev_wake_active_low;
+ /* Allow host sleep in SCO flag */
+ sleep_params.allow_host_sleep = 1;
+ /* Combine sleep and LPM flag */
+ sleep_params.combine_modes = 1;
+ /** Allow tristate control of UART tx flag */
+ sleep_params.tristate_control = 0;
+ /* Irrelevant USB flags */
+ sleep_params.usb_auto_sleep = 0;
+ sleep_params.usb_resume_timeout = 0;
+ sleep_params.pulsed_host_wake = 0;
+ sleep_params.break_to_host = 0;
+
+ skb = __hci_cmd_sync(hu->hdev, 0xfc27, sizeof(sleep_params),
+ &sleep_params, 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:
@@ -183,6 +503,9 @@ static int bcm_recv(struct hci_uart *hu, const void *data, int count)
if (!test_bit(HCI_UART_REGISTERED, &hu->flags))
return -EUNATCH;
+ /* 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)) {
@@ -201,6 +524,9 @@ static int bcm_enqueue(struct hci_uart *hu, struct sk_buff *skb)
BT_DBG("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);
skb_queue_tail(&bcm->txq, skb);
@@ -218,8 +544,6 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
static const struct hci_uart_proto bcm_proto = {
.id = HCI_UART_BCM,
.name = "BCM",
- .init_speed = 115200,
- .oper_speed = 4000000,
.open = bcm_open,
.close = bcm_close,
.flush = bcm_flush,
--
1.9.1
^ permalink raw reply related [flat|nested] 12+ messages in thread