bluetooth: Add hci_h4p driver
PATCH 1/1 bluetooth: Add hci_h4p driver
Add HCI driver for H4 with Nokia extensions. This device is used on Nokia N900 cell phone.
Older version of this driver lived in staging, before being reverted in a4102f90e87cfaa3fdbed6fdf469b23f0eeb4bfd .
Thanks-to: Sebastian Reichel <[email protected]>
Thanks-to: Joe Perches <[email protected]>
---
Please apply,
Pavel
Kconfig | 10 Makefile | 4 nokia_core.c | 1149 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ nokia_fw.c | 99 +++++ nokia_h4p.h | 214 ++++++++++ nokia_uart.c | 171 ++++++++ 7 files changed, 1667 insertions(+)
drivers/bluetooth/Kconfig +10 -0
@@ -243,4 +243,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 (btwilink). +config BT_NOKIA_H4P + tristate "Nokia H4+ Extensions (H4P) driver" + help + Bluetooth HCI driver with H4 extensions. This driver provides + support for H4+ Bluetooth chip with vendor-specific H4 extensions. + It works on Broadcom based chip found in Nokia N900. + + Say Y here to compile support for h4 extended devices into the kernel + or say M to compile it as module (nokia_h4p). + endmenu
drivers/bluetooth/Makefile +4 -0
@@ -31,4 +31,8 @@ hci_uart-$(CONFIG_BT_HCIUART_ATH3K) += hci_ath.o hci_uart-$(CONFIG_BT_HCIUART_3WIRE) += hci_h5.o hci_uart-objs := $(hci_uart-y) +obj-$(CONFIG_BT_NOKIA_H4P) += nokia_h4p.o + +nokia_h4p-y := nokia_core.o nokia_fw.o nokia_uart.o + ccflags-y += -D__CHECK_ENDIAN__
drivers/bluetooth/nokia_core.c new file +1149 -0
1,149 lines not on this page · raw message
drivers/bluetooth/nokia_fw.c new file +99 -0
99 lines not on this page · raw message
drivers/bluetooth/nokia_h4p.h new file +214 -0
214 lines not on this page · raw message
drivers/bluetooth/nokia_uart.c new file +171 -0
171 lines not on this page · raw message
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi!
Quoted 5 lines
Add HCI driver for H4 with Nokia extensions. This device is used on Nokia N900 cell phone.
Older version of this driver lived in staging, before being reverted in
a4102f90e87cfaa3fdbed6fdf469b23f0eeb4bfd.
Any news here? I'd like the driver to hit 3.20...
Pavel
Quoted 7 lines
Kconfig | 10 Makefile | 4 nokia_core.c | 1149 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ nokia_fw.c | 99 +++++ nokia_h4p.h | 214 ++++++++++ nokia_uart.c | 171 ++++++++ 7 files changed, 1667 insertions(+)
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Marcel Holtmann
maintainerHi Pavel,
Quoted 16 lines
Add HCI driver for H4 with Nokia extensions. This device is used on Nokia N900 cell phone.
Older version of this driver lived in staging, before being reverted in
a4102f90e87cfaa3fdbed6fdf469b23f0eeb4bfd.Signed-off-by Pavel Machek <[email protected]>Thanks-to: Sebastian Reichel <[email protected]>
Thanks-to: Joe Perches <[email protected]>---
Please apply,
Pavel
Quoted 7 lines
Kconfig | 10 Makefile | 4 nokia_core.c | 1149 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ nokia_fw.c | 99 +++++ nokia_h4p.h | 214 ++++++++++ nokia_uart.c | 171 ++++++++ 7 files changed, 1667 insertions(+)
so when I run this through checkpatch --strict, then I get tons of warning that we have DOS style ^M line breaks. There are also trailing whitespace that need fixing. I can use cleanpatch to do this, but so can you.
Even after doing that there are still obvious plain coding style violation in the patch. For example:
ERROR: space prohibited before that ',' (ctx:WxW) #610: FILE: drivers/bluetooth/nokia_core.c:517:
+ __h4p_set_auto_ctsrts(info, 0 , UART_EFR_RTS);
^
CHECK: Alignment should match open parenthesis
#662: FILE: drivers/bluetooth/nokia_core.c:569:
+ h4p_outb(info, UART_OMAP_SCR,
+ h4p_inb(info, UART_OMAP_SCR) |
CHECK: Blank lines aren't necessary before a close brace '}' #692: FILE: drivers/bluetooth/nokia_core.c:599:
+
+}
These are only few. They are more and all these need fixing before I even consider it.
Also this worries me:
WARNING: DT compatible string "brcm,uart,bcm2048" appears un-documented -- check ./Documentation/devicetree/bindings/ #1222: FILE: drivers/bluetooth/nokia_core.c:1129:
+ { .compatible = "brcm,uart,bcm2048" },
Regards
Marcel
Hi!
Quoted 16 lines
Quoted 16 lines
Add HCI driver for H4 with Nokia extensions. This device is used on Nokia N900 cell phone.
Older version of this driver lived in staging, before being reverted in
a4102f90e87cfaa3fdbed6fdf469b23f0eeb4bfd.Signed-off-by Pavel Machek <[email protected]>Thanks-to: Sebastian Reichel <[email protected]>
Thanks-to: Joe Perches <[email protected]>---
Please apply,
Pavel
Quoted 7 lines
Quoted 7 lines
Kconfig | 10 Makefile | 4 nokia_core.c | 1149 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ nokia_fw.c | 99 +++++ nokia_h4p.h | 214 ++++++++++ nokia_uart.c | 171 ++++++++ 7 files changed, 1667 insertions(+)
Speaking about formatting, could you properly format your emails, that is inserting newline after ~78 columns, to make them easier to reply to?
Quoted 2 lines
so when I run this through checkpatch --strict, then I get tons of warning that we have DOS style ^M line breaks. There are also trailing whitespace that need fixing. I can use cleanpatch to do this, but so can you.
Strange, where do you see DOS style line breaks? Checkpatch here does not warn about that, and they really should not be there.
drivers/bluetooth/nokia_core.c line 517
Quoted 5 lines
Even after doing that there are still obvious plain coding style violation in the patch. For example:
ERROR: space prohibited before that ',' (ctx:WxW)
#610: FILE:drivers/bluetooth/nokia_core.c:517:drivers/bluetooth/nokia_core.c new file +1
-0+__h4p_set_auto_ctsrts (info,0 ,UART_EFR_RTS );
Yeah, I should have catched that one.
Quoted 10 lines
CHECK: Alignment should match open parenthesis
#662: FILE:drivers/bluetooth/nokia_core.c:569:(quoted diff) +2
-0+ h4p_outb(info, UART_OMAP_SCR, + h4p_inb(info, UART_OMAP_SCR) |CHECK: Blank lines aren't necessary before a close brace '}' #692: FILE:
drivers/bluetooth/nokia_core.c:599:(quoted diff) +2
-0+ +}
Quoted 2 lines
These are only few. They are more and all these need fixing before I even consider it.
Yeah, so first patch was too good for staging, and I "would be allowed to clean it up in tree", and now you run checkpatch --strict, complaining about very serious stuff such as "blank lines before }".
Yes, checkpatch produces a lot of junk, like warnings about mdelay(). I'm not sure how you'd want #662 above, formatted.
pavel at amd:/data/l/linux-n900$ scripts/checkpatch.pl --strict --file drivers/bluetooth/*.c | wc -l 3194
pavel at amd:/data/l/linux-n900$
...so I really can't know which checkpatch complains you consider serious and which are ok... And yes, I guess I should trim down those FSF notices.
drivers/bluetooth/nokia_core.c line 1129
Quoted 5 lines
Also this worries me:
WARNING: DT compatible string "brcm,uart,bcm2048" appears un-documented -- check
./Documentation/devicetree/bindings/#1222: FILE:drivers/bluetooth/nokia_core.c:1129:drivers/bluetooth/nokia_core.c new file +1
-0+ { .compatible = "brcm,uart,bcm2048" },
Yes, that wories me, too. It is one of reasons I wanted this to be merged to staging. Arguing about right bindings will take some time.
I can fix the checkpatch stuff that makes sense. That does not include uglyfying code just for checkpatch. Can you then take the patch, as you promised, and let me argue the bindings, and the other stuff that needs to be fixed?
If not, can we agree that the driver in staging should be reverted, as Greg promised would be "easy", and I can clean it up there?
Thanks,
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Marcel Holtmann
maintainerHi Pavel,
Quoted 24 lines
Quoted 16 lines
Quoted 16 lines
Add HCI driver for H4 with Nokia extensions. This device is used on Nokia N900 cell phone.
Older version of this driver lived in staging, before being reverted in
a4102f90e87cfaa3fdbed6fdf469b23f0eeb4bfd.Signed-off-by Pavel Machek <[email protected]>Thanks-to: Sebastian Reichel <[email protected]>
Thanks-to: Joe Perches <[email protected]>---
Please apply,
PavelQuoted 7 lines
Quoted 7 lines
Kconfig | 10 Makefile | 4 nokia_core.c | 1149 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ nokia_fw.c | 99 +++++ nokia_h4p.h | 214 ++++++++++ nokia_uart.c | 171 ++++++++ 7 files changed, 1667 insertions(+)
Quoted 3 lines
Speaking about formatting, could you properly format your emails, that is inserting newline after ~78 columns, to make them easier to reply to?
or you get an email client that can handle that part.
Quoted 5 lines
Quoted 2 lines
so when I run this through checkpatch --strict, then I get tons of warning that we have DOS style ^M line breaks. There are also trailing whitespace that need fixing. I can use cleanpatch to do this, but so can you.
Strange, where do you see DOS style line breaks? Checkpatch here does not warn about that, and they really should not be there.
If that would be the only pieces, then I would have fixed it already. That is not the big deal. The rest of checkpatch is what I am not going to fix for you.
Quoted 18 lines
drivers/bluetooth/nokia_core.c line 517
Quoted 5 lines
Even after doing that there are still obvious plain coding style violation in the patch. For example:
ERROR: space prohibited before that ',' (ctx:WxW)
#610: FILE:drivers/bluetooth/nokia_core.c:517:drivers/bluetooth/nokia_core.c new file +1
-0+__h4p_set_auto_ctsrts (info,0 ,UART_EFR_RTS );Yeah, I should have catched that one.
Quoted 10 lines
CHECK: Alignment should match open parenthesis
#662: FILE:drivers/bluetooth/nokia_core.c:569:(quoted diff) +2
-0+ h4p_outb(info, UART_OMAP_SCR, + h4p_inb(info, UART_OMAP_SCR) |CHECK: Blank lines aren't necessary before a close brace '}' #692: FILE:
drivers/bluetooth/nokia_core.c:599:(quoted diff) +2
-0+ +}
Quoted 6 lines
Quoted 2 lines
These are only few. They are more and all these need fixing before I even consider it.
Yeah, so first patch was too good for staging, and I "would be allowed to clean it up in tree", and now you run checkpatch --strict, complaining about very serious stuff such as "blank lines before }".
The network subsystem requires the --strict option.
Please stop complaining about staging. The patch went in, it was ignored for month and multiple kernel release and it got removed. Deal with it.
Quoted 11 lines
Yes, checkpatch produces a lot of junk, like warnings about
mdelay(). I'm not sure how you'd want #662 above, formatted.pavel at amd:/data/l/linux-n900$
scripts/checkpatch.pl--strict --filedrivers/bluetooth/*.c| wc -l 3194
pavel at amd:/data/l/linux-n900$...so I really can't know which checkpatch complains you consider serious and which are ok... And yes, I guess I should trim down those FSF notices.
The FSF notices are the ones I do not care about right now. Even the over 80 characters lines can be ignored if it just makes sense to go over.
The indentations ones need to be fixed.
Quoted 9 lines
drivers/bluetooth/nokia_core.c line 1129
Quoted 6 lines
Also this worries me:
WARNING: DT compatible string "brcm,uart,bcm2048" appears un-documented -- check
./Documentation/devicetree/bindings/#1222: FILE:drivers/bluetooth/nokia_core.c:1129:drivers/bluetooth/nokia_core.c new file +1
-0+ { .compatible = "brcm,uart,bcm2048" },Yes, that wories me, too. It is one of reasons I wanted this to be merged to staging. Arguing about right bindings will take some time.
Then that needs to be figured out. It is not that I have mentioned DT for the first time. I said that right from the beginning.
Quoted 7 lines
I can fix the checkpatch stuff that makes sense. That does not include uglyfying code just for checkpatch. Can you then take the patch, as you promised, and let me argue the bindings, and the other stuff that needs to be fixed?
If not, can we agree that the driver in staging should be reverted, as Greg promised would be "easy", and I can clean it up there?
I am refusing to allow this into staging. Get this into shape for drivers/bluetooth/ or keep the driver external.
Regards
Marcel
Hi!
Quoted 5 lines
Quoted 3 lines
Speaking about formatting, could you properly format your emails, that is inserting newline after ~78 columns, to make them easier to reply to?
or you get an email client that can handle that part.
My email client is configured ok. Your is not. You are speaking on public mailing list, so you should follow the rules.
Quoted 5 lines
Quoted 2 lines
Strange, where do you see DOS style line breaks? Checkpatch here does not warn about that, and they really should not be there.
If that would be the only pieces, then I would have fixed it already. That is not the big deal. The rest of checkpatch is what I am not going to fix for you.
I can fix the checkpatch, but I'd like to understand how ^Ms got there, because otherwise it will happen again.
Quoted 8 lines
Quoted 3 lines
Yeah, so first patch was too good for staging, and I "would be allowed to clean it up in tree", and now you run checkpatch --strict, complaining about very serious stuff such as "blank lines before }".
The network subsystem requires the --strict option.
Please stop complaining about staging. The patch went in, it was ignored for month and multiple kernel release and it got removed. Deal with it.
Yes. It took me one 7 months to set up development environment. You decided you don't like staging, so you made sure it does not work for me -- by removing the driver. Thus, I had not only cleanups to do, but also resulting bitrot. And now, because you don't like staging, are making sure that I'll have to deal with bitrot and device tree at the same time.
Quoted 1 line
The indentations ones need to be fixed.
Ok, done.
Quoted 9 lines
Quoted 8 lines
drivers/bluetooth/nokia_core.c line 1129
Quoted 5 lines
Also this worries me:
WARNING: DT compatible string "brcm,uart,bcm2048" appears un-documented -- check
./Documentation/devicetree/bindings/#1222: FILE:drivers/bluetooth/nokia_core.c:1129:drivers/bluetooth/nokia_core.c new file +1
-0+ { .compatible = "brcm,uart,bcm2048" },Yes, that wories me, too. It is one of reasons I wanted this to be merged to staging. Arguing about right bindings will take some time.
Quoted 4 lines
Then that needs to be figured out. It is not that I have mentioned DT for the first time. I said that right from the beginning.
From the beginning of the second try. I'll try to figure it out with
the dt people.
Quoted 10 lines
Quoted 7 lines
I can fix the checkpatch stuff that makes sense. That does not include uglyfying code just for checkpatch. Can you then take the patch, as you promised, and let me argue the bindings, and the other stuff that needs to be fixed?
If not, can we agree that the driver in staging should be reverted, as Greg promised would be "easy", and I can clean it up there?
I am refusing to allow this into staging. Get this into shape for
drivers/bluetooth/or keep the driver external.
After all the work that gone into the driver, you could at least state the reason. I guess I have demonstrated by now that someone cares about the driver.
For the record, reason I want it in the staging is that I don't want driver to bitrot while dt people do their bikeshed discussion.
Regards,
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Quoted 11 lines · On Wednesday 21 January 2015 12:01:31 Pavel Machek wrote:
Quoted 7 lines
Quoted 3 lines
Strange, where do you see DOS style line breaks? Checkpatch here does not warn about that, and they really should not be there.
If that would be the only pieces, then I would have fixed it already. That is not the big deal. The rest of checkpatch is what I am not going to fix for you.
I can fix the checkpatch, but I'd like to understand how ^Ms got there, because otherwise it will happen again.
IIRC lines in email according to RFC2822 specification must be delimited by CRLF. Maybe this is reason?
Signature · 9 lines
Pali Roh?r pali.rohar at gmail.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 198 bytes Desc: This is a digitally signed message part. URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150121/d67fb4f3/attachment-0001.sig>
Hi Pavel,
Quoted 34 lines · Pavel Machek
drivers/bluetooth/nokia_fw.c new file +34
-0+while (1 ) { +int cmd, len; + + fw_pos += cmd_len; + +if (fw_pos >= fw_entry->size) +break ; + +if (fw_pos +2 > fw_entry->size) { +dev_err (info->dev,"Corrupted firmware image\n" ); + err = -EMSGSIZE ; +break ; + } + + cmd_len = fw_entry->data[fw_pos++]; + cmd_len += fw_entry->data[fw_pos++] <<8 ; +if (cmd_len ==0 ) +break ; + +if (fw_pos + cmd_len > fw_entry->size) { +dev_err (info->dev,"Corrupted firmware image\n" ); + err = -EMSGSIZE ; +break ; + } + +/* Skip first two packets */ +if (++num <=2 ) +continue ; + +/* Note that this is timing-critical. If sending packets takes too +* long, initialization will fail. +*/ + cmd = fw_entry->data[fw_pos+1 ]; + cmd += fw_entry->data[fw_pos+2 ] <<8 ;
drivers/bluetooth/nokia_fw.c line 93
Quoted 10 lines
drivers/bluetooth/nokia_fw.c new file +10
-0+ len = fw_entry->data[fw_pos+3 ]; + + skb =__hci_cmd_sync (info->hdev, cmd, len, fw_entry->data+fw_pos+4 ,500 ); +if (IS_ERR (skb)) { +dev_err (info->dev,"...sending cmd %x len %d failed %ld\n" , + cmd, len,PTR_ERR (skb)); + err = -EIO ; +break ; + } + }
Looks like the code is leaking skb when __hci_cmd_sync() succeeds.
Johan
Quoted 41 lines · Johan Hedberg
Hi Pavel,
Quoted 38 lines · Pavel Machek
drivers/bluetooth/nokia_fw.c new file +38
-0+while (1 ) { +int cmd, len; + + fw_pos += cmd_len; + +if (fw_pos >= fw_entry->size) +break ; + +if (fw_pos +2 > fw_entry->size) { +dev_err (info->dev,"Corrupted firmware image\n" ); + err = -EMSGSIZE ; +break ; + } + + cmd_len = fw_entry->data[fw_pos++]; + cmd_len += fw_entry->data[fw_pos++] <<8 ; +if (cmd_len ==0 ) +break ; + +if (fw_pos + cmd_len > fw_entry->size) { +dev_err (info->dev,"Corrupted firmware image\n" ); + err = -EMSGSIZE ; +break ; + } + +/* Skip first two packets */ +if (++num <=2 ) +continue ; + +/* Note that this is timing-critical. If sending packets takes too +* long, initialization will fail. +*/ + cmd = fw_entry->data[fw_pos+1 ]; + cmd += fw_entry->data[fw_pos+2 ] <<8 ; + len = fw_entry->data[fw_pos+3 ]; + + skb =__hci_cmd_sync (info->hdev, cmd, len, fw_entry->data+fw_pos+4 ,500 ); +if (IS_ERR (skb)) {
Quoted 8 lines
drivers/bluetooth/nokia_fw.c line 93
Quoted 6 lines
drivers/bluetooth/nokia_fw.c new file +6
-0+dev_err (info->dev,"...sending cmd %x len %d failed %ld\n" , + cmd, len,PTR_ERR (skb)); + err = -EIO ; +break ; + } + }Looks like the code is leaking skb when
__hci_cmd_sync()succeeds.
Fixed, thanks!
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html