lkml.org 
[lkml]   [2013]   [Dec]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v2] Bluetooth: Add hci_h4p driver
Hi!

> > @@ -242,4 +242,14 @@ config BT_WILINK
> >
> > Say Y here to compile support for Texas Instrument's WiLink7 driver
> > into the kernel or say M to compile it as module.
> > +
> > +config BT_HCIH4P
> > + tristate "HCI driver with H4 Nokia extensions"
> > + depends on BT && ARCH_OMAP
>
> Since then we moved away from doing hci_* prefix of drivers since that is misleading. See btusb.ko, btmrvl_sdio.ko etc.
>
> So this might be better named BT_NOK_H4P or BT_NOKIA_H4P and the module named btnok_h4p.ko or btnokia_h4p.ko.
>

Well, I can rename config option, but renaming the module would break
existing userland, no?

> Can we also make this just depend on some device tree information
> and not on a specific architecture. I know that this driver is
> pretty much OMAP specific, but if we want this upstream, we should
> at least try to make it more generic.

Nokia N900 is certainly moving towards device tree, but we are not
ready, yet...


> > @@ -0,0 +1,1357 @@
> > +/*
> > + * This file is part of hci_h4p bluetooth driver
> > + *
> > + * Copyright (C) 2005-2008 Nokia Corporation.
> > + *
> > + * Contact: Ville Tervo <ville.tervo@nokia.com>
>
> I think you can just remove the contact names since I think nobody of the original authors is still working at Nokia and I bet this emails addresses just do not work anymore.
>

I sent him an email with question. I guess we should move Ville to
CREDITS?

> > +#include <linux/bluetooth/hci_h4p.h>
> > +
> > +#include "hci_h4p.h”
> > +
>
> Please do not introduce public includes for a driver. This should be all confined to the driver itself or if it platform data, it should go into the place for platform data.
>

(Could you insert newlines after 80 or so characters?)

Where would you like platform_data definition to go? That indeed is
for platform data, and quick grep shows drivers normally do public
header files for that.

> > +
> > + if (!wait_for_completion_interruptible_timeout(&info->init_completion,
> > + msecs_to_jiffies(1000)))
>
> Please follow the net subsystem coding style.

Could you elaborate? I see nothing too obviously wrong here.

> > +{
> > + if (unlikely(!test_bit(HCI_RUNNING, &info->hdev->flags))) {
> > + if (bt_cb(skb)->pkt_type == H4_NEG_PKT) {
> > + hci_h4p_negotiation_packet(info, skb);
> > + info->rx_state = WAIT_FOR_PKT_TYPE;
> > + return;
> > + }
>
> Use "else if” here or a switch statement.

Ok.

> > +/* hci callback functions */
> > +static int hci_h4p_hci_flush(struct hci_dev *hdev)
> > +{
> > + struct hci_h4p_info *info;
> > + info = hci_get_drvdata(hdev);
>
> This can be directly assigned at variable declaration.

Ok.

> > +static ssize_t hci_h4p_show_bdaddr(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct hci_h4p_info *info = dev_get_drvdata(dev);
> > +
> > + return sprintf(buf, "%.2x:%.2x:%.2x:%.2x:%.2x:%.2x\n",
> > + info->bd_addr[0], info->bd_addr[1], info->bd_addr[2],
> > + info->bd_addr[3], info->bd_addr[4], info->bd_addr[5]);
>
> We have printf modifier to print BD_ADDRs.

%pMR. Ok.

> > + if (hci_register_dev(hdev) < 0) {
> > + dev_err(info->dev, "hci_register failed %s.\n", hdev->name);
> > + hci_h4p_sysfs_remove_files(info->dev);
> > + return -ENODEV;
> > + }
> > +
>
> Who is freeing hdev here in case of an error?

Ok.

> > + if (!info->reset_gpio_shared)
> > + gpio_free(info->reset_gpio);
> > + gpio_free(info->bt_wakeup_gpio);
> > + gpio_free(info->host_wakeup_gpio);
> > +
> > +cleanup_setup:
> > +
> > + kfree(info);
> > + return err;
> > +
>
> Avoid these extra empty lines at function end.

Ok.

> > +
> > + if (not_valid) {
> > + dev_info(info->dev, "Valid bluetooth address not found, setting some random\n");
> > + /* When address is not valid, use some random but Nokia MAC */
> > + memcpy(info->bd_addr, nokia_oui, 3);
> > + get_random_bytes(info->bd_addr + 3, 3);
> > + }
>
>
> This behavior is extremely dangerous. I would rather have the device init or powering on the device fail instead of making up a number that might clash with a real Nokia device.
>

Perhaps people can donate bt addresses from their no-longer-functional
bluetooth devices and we can select from such pool here? ;-).

Is there some experimental range we can allocate from?

> > +int hci_h4p_reset_uart(struct hci_h4p_info *info)
> > +{
> > + int count = 0;
> > +
> > + /* Reset the UART */
> > + hci_h4p_outb(info, UART_OMAP_SYSC, UART_SYSC_OMAP_RESET);
> > + while (!(hci_h4p_inb(info, UART_OMAP_SYSS) & UART_SYSS_RESETDONE)) {
> > + if (count++ > 100) {
> > + dev_err(info->dev, "hci_h4p: UART reset timeout\n");
> > + return -ENODEV;
> > + }
> > + udelay(1);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +
>
> No double empty lines please.

Ok.

Pali, here's first round of cleanups...

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

Pavel

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index 95155c3..9d46f23 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -243,7 +243,7 @@ config BT_WILINK
Say Y here to compile support for Texas Instrument's WiLink7 driver
into the kernel or say M to compile it as module.

-config BT_HCIH4P
+config BT_NOKIA_H4P
tristate "HCI driver with H4 Nokia extensions"
depends on BT && ARCH_OMAP
help
diff --git a/drivers/bluetooth/hci_h4p/Makefile b/drivers/bluetooth/hci_h4p/Makefile
index f20bd9a..6f2ef85 100644
--- a/drivers/bluetooth/hci_h4p/Makefile
+++ b/drivers/bluetooth/hci_h4p/Makefile
@@ -2,6 +2,6 @@
# Makefile for the Linux Bluetooth HCI device drivers.
#

-obj-$(CONFIG_BT_HCIH4P) += hci_h4p.o
+obj-$(CONFIG_BT_NOKIA_H4P) += hci_h4p.o

hci_h4p-objs := core.o fw.o uart.o fw-csr.o fw-bcm.o fw-ti1273.o
diff --git a/drivers/bluetooth/hci_h4p/core.c b/drivers/bluetooth/hci_h4p/core.c
index 2f1f8d4..a91bd7b 100644
--- a/drivers/bluetooth/hci_h4p/core.c
+++ b/drivers/bluetooth/hci_h4p/core.c
@@ -436,12 +436,12 @@ static inline void hci_h4p_recv_frame(struct hci_h4p_info *info,
struct sk_buff *skb)
{
if (unlikely(!test_bit(HCI_RUNNING, &info->hdev->flags))) {
- if (bt_cb(skb)->pkt_type == H4_NEG_PKT) {
+ switch (bt_cb(skb)->pkt_type) {
+ case H4_NEG_PKT:
hci_h4p_negotiation_packet(info, skb);
info->rx_state = WAIT_FOR_PKT_TYPE;
return;
- }
- if (bt_cb(skb)->pkt_type == H4_ALIVE_PKT) {
+ case H4_ALIVE_PKT:
hci_h4p_alive_packet(info, skb);
info->rx_state = WAIT_FOR_PKT_TYPE;
return;
@@ -843,9 +843,7 @@ static int hci_h4p_reset(struct hci_h4p_info *info)
/* hci callback functions */
static int hci_h4p_hci_flush(struct hci_dev *hdev)
{
- struct hci_h4p_info *info;
- info = hci_get_drvdata(hdev);
-
+ struct hci_h4p_info *info = hci_get_drvdata(hdev);
skb_queue_purge(&info->txq);

return 0;
@@ -853,7 +851,8 @@ static int hci_h4p_hci_flush(struct hci_dev *hdev)

static int hci_h4p_bt_wakeup_test(struct hci_h4p_info *info)
{
- /* Test Sequence:
+ /*
+ * Test Sequence:
* Host de-asserts the BT_WAKE_UP line.
* Host polls the UART_CTS line, waiting for it to be de-asserted.
* Host asserts the BT_WAKE_UP line.
@@ -1101,9 +1100,7 @@ static ssize_t hci_h4p_show_bdaddr(struct device *dev,
{
struct hci_h4p_info *info = dev_get_drvdata(dev);

- return sprintf(buf, "%.2x:%.2x:%.2x:%.2x:%.2x:%.2x\n",
- info->bd_addr[0], info->bd_addr[1], info->bd_addr[2],
- info->bd_addr[3], info->bd_addr[4], info->bd_addr[5]);
+ return sprintf(buf, "%pMR\n", info->bd_addr);
}

static DEVICE_ATTR(bdaddr, S_IRUGO | S_IWUSR, hci_h4p_show_bdaddr,
@@ -1146,16 +1143,17 @@ static int hci_h4p_register_hdev(struct hci_h4p_info *info)

if (hci_h4p_sysfs_create_files(info->dev) < 0) {
dev_err(info->dev, "failed to create sysfs files\n");
- return -ENODEV;
+ goto free;
}

- if (hci_register_dev(hdev) < 0) {
- dev_err(info->dev, "hci_register failed %s.\n", hdev->name);
- hci_h4p_sysfs_remove_files(info->dev);
- return -ENODEV;
- }
+ if (hci_register_dev(hdev) >= 0)
+ return 0;

- return 0;
+ dev_err(info->dev, "hci_register failed %s.\n", hdev->name);
+ hci_h4p_sysfs_remove_files(info->dev);
+free:
+ hci_free_dev(info->hdev);
+ return -ENODEV;
}

static int hci_h4p_probe(struct platform_device *pdev)
@@ -1296,12 +1294,9 @@ cleanup:
gpio_free(info->reset_gpio);
gpio_free(info->bt_wakeup_gpio);
gpio_free(info->host_wakeup_gpio);
-
cleanup_setup:
-
kfree(info);
return err;
-
}

static int hci_h4p_remove(struct platform_device *pdev)
diff --git a/drivers/bluetooth/hci_h4p/uart.c b/drivers/bluetooth/hci_h4p/uart.c
index 7973c6c..8e0a93c 100644
--- a/drivers/bluetooth/hci_h4p/uart.c
+++ b/drivers/bluetooth/hci_h4p/uart.c
@@ -129,7 +129,7 @@ int hci_h4p_reset_uart(struct hci_h4p_info *info)
{
int count = 0;

- /* Reset the UART */
+ /* Reset the UART */
hci_h4p_outb(info, UART_OMAP_SYSC, UART_SYSC_OMAP_RESET);
while (!(hci_h4p_inb(info, UART_OMAP_SYSS) & UART_SYSS_RESETDONE)) {
if (count++ > 100) {
@@ -142,7 +142,6 @@ int hci_h4p_reset_uart(struct hci_h4p_info *info)
return 0;
}

-
void hci_h4p_store_regs(struct hci_h4p_info *info)
{
u16 lcr = 0;

--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2013-12-30 14:01    [W:0.240 / U:4.488 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site