On Fri, Dec 1, 2017 at 4:24 PM, Marcel Holtmann <marcel@xxxxxxxxxxxx> wrote:
> Hi Amitkumar,
>
>> Redpine bluetooth driver is a thin driver which depends on
>> 'rsi_91x' driver for transmitting and receiving packets
>> to/from device. It creates hci interface when attach() is
>> called from 'rsi_91x' module.
>>
>> Signed-off-by: Prameela Rani Garnepudi <prameela.j04cs@xxxxxxxxx>
>> Signed-off-by: Siva Rebbagondla <siva.rebbagondla@xxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Amitkumar Karwar <amit.karwar@xxxxxxxxxxxxxxxxxx>
>> ---
>> v3: Made BT_RSI module by default off(Marcel)
>> Removed redundant exported function rsi_get_hci_ops()(Marcel)
>> v2: Addressed review comments from Marcel
>> Removed unnecessary 'depends on BT && BT_RFOMM' line in Kconfig
>> Removed redundant BT_INFO messages
>> h_adapter initialization and declaration in a single line.
>> Removed unnecessary error checks for HCI_RUNNING and fsm_state
>> Allocated new skb with skb_realloc_headroom() API if headroom is not sufficient
>> Used get_unaligned_le16 helpers
>> Moved a structure and union from header file to btrsi.c file
>> ---
>> drivers/bluetooth/Kconfig | 11 +++
>> drivers/bluetooth/Makefile | 2 +
>> drivers/bluetooth/btrsi.c | 211 ++++++++++++++++++++++++++++++++++++++++++++
>> drivers/bluetooth/rsi_hci.h | 40 +++++++++
>> include/linux/rsi_header.h | 2 +
>> 5 files changed, 266 insertions(+)
>> create mode 100644 drivers/bluetooth/btrsi.c
>> create mode 100644 drivers/bluetooth/rsi_hci.h
>>
>> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
>> index 60e1c7d..33d7514 100644
>> --- a/drivers/bluetooth/Kconfig
>> +++ b/drivers/bluetooth/Kconfig
>> @@ -378,4 +378,15 @@ config BT_QCOMSMD
>> Say Y here to compile support for HCI over Qualcomm SMD into the
>> kernel or say M to compile as a module.
>>
>> +config BT_RSI
>> + tristate "Redpine HCI support"
>> + default n
>> + help
>> + Redpine BT driver.
>> + This driver handles BT traffic from upper layers and pass
>> + to the RSI_91x coex module for further scheduling to device
>> +
>> + Say Y here to compile support for HCI over Redpine into the
>> + kernel or say M to compile as a module.
>> +
>> endmenu
>> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
>> index 4e4e44d..712af83a 100644
>> --- a/drivers/bluetooth/Makefile
>> +++ b/drivers/bluetooth/Makefile
>> @@ -28,6 +28,8 @@ obj-$(CONFIG_BT_QCA) += btqca.o
>>
>> obj-$(CONFIG_BT_HCIUART_NOKIA) += hci_nokia.o
>>
>> +obj-$(CONFIG_BT_RSI) += btrsi.o
>> +
>> btmrvl-y := btmrvl_main.o
>> btmrvl-$(CONFIG_DEBUG_FS) += btmrvl_debugfs.o
>>
>> diff --git a/drivers/bluetooth/btrsi.c b/drivers/bluetooth/btrsi.c
>> new file mode 100644
>> index 0000000..71f6f4e
>> --- /dev/null
>> +++ b/drivers/bluetooth/btrsi.c
>> @@ -0,0 +1,211 @@
>> +/**
>> + * Copyright (c) 2017 Redpine Signals Inc.
>> + *
>> + * Permission to use, copy, modify, and/or distribute this software for any
>> + * purpose with or without fee is hereby granted, provided that the above
>> + * copyright notice and this permission notice appear in all copies.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
>> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
>> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
>> +e* ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
>> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
>> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
>> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
>> + */
>> +#include <linux/version.h>
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include "rsi_hci.h"
>> +
>> +struct rsi_hci_adapter {
>> + void *priv;
>> + struct rsi_proto_ops *proto_ops;
>> + struct hci_dev *hdev;
>> +};
>> +
>> +const struct rsi_mod_ops rsi_bt_ops = {
>> + .attach = rsi_hci_attach,
>> + .detach = rsi_hci_detach,
>> + .recv_pkt = rsi_hci_recv_pkt,
>> +};
>> +EXPORT_SYMBOL(rsi_bt_ops);
>> +
>> +static int rsi_hci_open(struct hci_dev *hdev)
>> +{
>> + return 0;
>> +}
>> +
>> +static int rsi_hci_close(struct hci_dev *hdev)
>> +{
>> + return 0;
>> +}
>> +
>> +static int rsi_hci_flush(struct hci_dev *hdev)
>> +{
>> + return 0;
>> +}
>> +
>> +static int rsi_hci_send_pkt(struct hci_dev *hdev, struct sk_buff *skb)
>> +{
>> + struct rsi_hci_adapter *h_adapter = hci_get_drvdata(hdev);
>> + struct sk_buff *new_skb = NULL;
>> +
>> + switch (bt_cb(skb)->pkt_type) {
>> + case HCI_COMMAND_PKT:
>> + hdev->stat.cmd_tx++;
>> + break;
>> +
>> + case HCI_ACLDATA_PKT:
>> + hdev->stat.acl_tx++;
>> + break;
>> +
>> + case HCI_SCODATA_PKT:
>> + hdev->stat.sco_tx++;
>> + break;
>> + }
>> +
>> + if (skb_headroom(skb) < RSI_HEADROOM_FOR_BT_HAL) {
>> + /* Insufficient skb headroom - allocate a new skb */
>> + new_skb = skb_realloc_headroom(skb, RSI_HEADROOM_FOR_BT_HAL);
>> + if (unlikely(!new_skb))
>> + return -ENOMEM;
>> + bt_cb(new_skb)->pkt_type = bt_cb(skb)->pkt_type;
>> + kfree_skb(skb);
>> + skb = new_skb;
>> + }
>> +
>> + return h_adapter->proto_ops->coex_send_pkt(h_adapter->priv, skb,
>> + RSI_BT_Q);
>> +}
>> +
>> +int rsi_hci_recv_pkt(void *priv, u8 *pkt)
>> +{
>> + struct rsi_hci_adapter *h_adapter = (struct rsi_hci_adapter *)priv;
>> + struct hci_dev *hdev = h_adapter->hdev;
>> + struct sk_buff *skb;
>> +
>> + int pkt_len = get_unaligned_le16(pkt) & 0x0fff;
>> + u8 queue_no = (get_unaligned_le16(pkt) & 0x7000) >> 12;
>> +
>> + if (queue_no == RSI_BT_MGMT_Q) {
>> + u8 msg_type = pkt[14] & 0xFF;
>> +
>> + switch (msg_type) {
>> + case RSI_RESULT_CONFIRM:
>> + bt_dev_info(hdev, "BT Result Confirm");
>> + return 0;
>> + case RSI_BT_BER:
>> + bt_dev_info(hdev, "BT Ber");
>> + return 0;
>> + case RSI_BT_CW:
>> + bt_dev_info(hdev, "BT CW");
>> + return 0;
>> + default:
>> + break;
>> + }
>> + }
>> +
>> + skb = dev_alloc_skb(pkt_len);
>> + if (!skb)
>> + return -ENOMEM;
>> +
>> + memcpy(skb->data, pkt + RSI_FRAME_DESC_SIZE, pkt_len);
>> + skb_put(skb, pkt_len);
>> + h_adapter->hdev->stat.byte_rx += skb->len;
>> +
>> + skb->dev = (void *)hdev;
>> + bt_cb(skb)->pkt_type = pkt[14];
>> +
>> + return hci_recv_frame(hdev, skb);
>> +}
>> +
>> +int rsi_hci_attach(void *priv, struct rsi_proto_ops *ops)
>> +{
>> + struct rsi_hci_adapter *h_adapter = NULL;
>> + struct hci_dev *hdev;
>> + int status = 0;
>> +
>> + h_adapter = kzalloc(sizeof(*h_adapter), GFP_KERNEL);
>> + if (!h_adapter)
>> + return -ENOMEM;
>> +
>> + h_adapter->priv = priv;
>> + ops->set_bt_context(priv, h_adapter);
>> + h_adapter->proto_ops = ops;
>> +
>> + hdev = hci_alloc_dev();
>> + if (!hdev) {
>> + BT_ERR("Failed to alloc HCI device\n");
>> + goto err;
>> + }
>> +
>> + h_adapter->hdev = hdev;
>> +
>> + if (ops->get_host_intf(priv) == RSI_HOST_INTF_SDIO)
>> + hdev->bus = HCI_SDIO;
>> + else
>> + hdev->bus = HCI_USB;
>> +
>> + hci_set_drvdata(hdev, h_adapter);
>> + hdev->dev_type = HCI_PRIMARY;
>> + hdev->open = rsi_hci_open;
>> + hdev->close = rsi_hci_close;
>> + hdev->flush = rsi_hci_flush;
>> + hdev->send = rsi_hci_send_pkt;
>> +
>> + status = hci_register_dev(hdev);
>> + if (status < 0) {
>> + BT_ERR("%s: HCI registration failed with errcode %d\n",
>> + __func__, status);
>> + goto err;
>> + }
>> +
>> + return 0;
>> +err:
>> + if (hdev) {
>> + hci_unregister_dev(hdev);
>> + hci_free_dev(hdev);
>> + h_adapter->hdev = NULL;
>> + }
>> + kfree(h_adapter);
>> +
>> + return -EINVAL;
>> +}
>> +
>> +void rsi_hci_detach(void *priv)
>> +{
>> + struct rsi_hci_adapter *h_adapter = (struct rsi_hci_adapter *)priv;
>> + struct hci_dev *hdev;
>> +
>> + if (!h_adapter)
>> + return;
>> +
>> + hdev = h_adapter->hdev;
>> + if (hdev) {
>> + hci_unregister_dev(hdev);
>> + hci_free_dev(hdev);
>> + h_adapter->hdev = NULL;
>> + }
>> +
>> + kfree(h_adapter);
>> +}
>> +
>> +static int rsi_91x_bt_module_init(void)
>> +{
>> + BT_INFO("%s: BT Module init called\n", __func__);
>> +
>> + return 0;
>> +}
>> +
>> +static void rsi_91x_bt_module_exit(void)
>> +{
>> + BT_INFO("%s: BT Module exit called\n", __func__);
>> +}
>> +
>> +module_init(rsi_91x_bt_module_init);
>> +module_exit(rsi_91x_bt_module_exit);
>> +MODULE_AUTHOR("Redpine Signals Inc");
>> +MODULE_DESCRIPTION("RSI BT driver");
>> +MODULE_SUPPORTED_DEVICE("RSI-BT");
>> +MODULE_LICENSE("Dual BSD/GPL");
>> diff --git a/drivers/bluetooth/rsi_hci.h b/drivers/bluetooth/rsi_hci.h
>> new file mode 100644
>> index 0000000..6fc7757
>> --- /dev/null
>> +++ b/drivers/bluetooth/rsi_hci.h
>> @@ -0,0 +1,40 @@
>> +/**
>> + * Copyright (c) 2017 Redpine Signals Inc.
>> + *
>> + * Permission to use, copy, modify, and/or distribute this software for any
>> + * purpose with or without fee is hereby granted, provided that the above
>> + * copyright notice and this permission notice appear in all copies.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
>> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
>> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
>> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
>> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
>> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
>> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
>> + */
>> +
>> +#ifndef __RSI_HCI_H__
>> +#define __RSI_HCI_H__
>> +
>> +#include <net/bluetooth/bluetooth.h>
>> +#include <net/bluetooth/hci_core.h>
>> +#include <linux/unaligned/le_byteshift.h>
>> +#include <linux/rsi_header.h>
>> +#include <net/genetlink.h>
>> +
>> +/* RX frame types */
>> +#define RSI_RESULT_CONFIRM 0x80
>> +#define RSI_BT_PER 0x10
>> +#define RSI_BT_BER 0x11
>> +#define RSI_BT_CW 0x12
>> +
>> +#define RSI_HEADROOM_FOR_BT_HAL 16
>
> I still like to see changes to the BT core that allows the driver to specify how headroom it needs. This can be follow up patches, but we should do that since reallocating the SKBs all the time is also pointless.
Ok. I will prepare follow up patch for this.
>
>> +
>> +#define RSI_FRAME_DESC_SIZE 16
>> +
>> +int rsi_hci_attach(void *priv, struct rsi_proto_ops *ops);
>> +void rsi_hci_detach(void *priv);
>> +int rsi_hci_recv_pkt(void *data, u8 *pkt);
>> +
>> +#endif
>
> I have no idea why this header file is needed at all. Fold this into btrsi.c and make the rsi_hci_* functions static.
I will take care of this in v4.
>
>> diff --git a/include/linux/rsi_header.h b/include/linux/rsi_header.h
>> index 737ab4e..07d9574 100644
>> --- a/include/linux/rsi_header.h
>> +++ b/include/linux/rsi_header.h
>> @@ -51,4 +51,6 @@ struct rsi_mod_ops {
>> void (*detach)(void *priv);
>> int (*recv_pkt)(void *priv, u8 *msg);
>> };
>> +
>> +extern const struct rsi_mod_ops rsi_bt_ops;
>
> If you expose rsi_hci_attach via rsi_bt_ops, then there is no need for making rsi_hci_attach also public. I mentioned this in the first review as well. I do not get it. Either you expose rsi_bt_ops from this module or you expose the rsi_hci_* function, but both is totally pointless.
>
Got it. I will get rid of header file and make these functions static
Regards,
Amitkumar
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
