Re: bluetooth: Add hci_h4p driver
Hi Pavel, >>> I have trouble understanding... h4p_hci_open() seems to be called as >>> soon as I insmod the driver. Who does that? Is it some kind of udev >>> magic? >> >> As soon as you do hci_register_dev, it will bring up the device and run the >> basic initialization. That is needed to get the address, version information >> and features. Otherwise the mgmt interface can not work. We need basic >> information about the device. >> >> So what the kernel will do internally when you find a device is bring it up, >> get the basics and then bring it back down (in case nobody uses it). See >> hci_power_on work. >> > > Aha, slightly unexpected, but it matches observations. Good. we are at Bluetooth 4.2 specification now. Thinks got a lot more complicated these days. > >>> To use __hci_cmd_sync() >>> > + > + SET_HCIDEV_DEV(hdev, info->dev); > + > + if (hci_register_dev(hdev) >= 0) > + return 0; > + > + dev_err(info->dev, "hci_register failed %s.\n", hdev->name); > + hci_free_dev(info->hdev); > + return -ENODEV; > +} And normally this is folded into the probe handling and not in a separate function. >>> >>> The probe function is too long, already... >> >> Have you compared it to other functions. Normally probe() functions in >> general are all pretty long since they have to do tons of stuff. Having all >> in one function means you can read through it at once. >> > > Ok. > > +#include "hci_h4p.h" Why is this a separate file? And if so, why not all UART specific details are in this file. Including bunch of the defines you have in the header. >>> >>> Well, you wanted less global functions, so I moved some as inlines to >>> headers. I guess I can merge nokia_core and nokia_uart if really wanted. >> >> Would this become easier when some of the OMAP specific stuff is moved out >> of this driver? If that is possible. >> > > Yes, I can investigate further cleanups. But original plan was to > merge it and then clean up the rest. Could we do that, please? You need to fix all these small details. There are tons of whitespace damages, indentation issues, trailing whitespaces and what not. Even if they were in the original driver to begin this, they can not be in a driver submitted these days. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bluetooth: Add hci_h4p driver
Hi! > > I have trouble understanding... h4p_hci_open() seems to be called as > > soon as I insmod the driver. Who does that? Is it some kind of udev > > magic? > > As soon as you do hci_register_dev, it will bring up the device and run the > basic initialization. That is needed to get the address, version information > and features. Otherwise the mgmt interface can not work. We need basic > information about the device. > > So what the kernel will do internally when you find a device is bring it up, > get the basics and then bring it back down (in case nobody uses it). See > hci_power_on work. > Aha, slightly unexpected, but it matches observations. Good. > > To use __hci_cmd_sync() > > > >>> + > >>> + SET_HCIDEV_DEV(hdev, info->dev); > >>> + > >>> + if (hci_register_dev(hdev) >= 0) > >>> + return 0; > >>> + > >>> + dev_err(info->dev, "hci_register failed %s.\n", hdev->name); > >>> + hci_free_dev(info->hdev); > >>> + return -ENODEV; > >>> +} > >> > >> And normally this is folded into the probe handling and not in a > >> separate function. > > > > The probe function is too long, already... > > Have you compared it to other functions. Normally probe() functions in > general are all pretty long since they have to do tons of stuff. Having all > in one function means you can read through it at once. > Ok. > >>> +#include "hci_h4p.h" > >> > >> Why is this a separate file? And if so, why not all UART specific details > >> are in this file. Including bunch of the defines you have in the header. > >> > > > > Well, you wanted less global functions, so I moved some as inlines to > > headers. I guess I can merge nokia_core and nokia_uart if really wanted. > > Would this become easier when some of the OMAP specific stuff is moved out of > this driver? If that is possible. > Yes, I can investigate further cleanups. But original plan was to merge it and then clean up the rest. Could we do that, please? Thanks, Pavel -- (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-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bluetooth: Add hci_h4p driver
Hi Pavel,
>> And you want to set the QUIRK_INVALID_BADDR. At least you want to do that in
>> the final submission.
>>
>
> Ok, I found out that I can do it and result works, provided that I do
> hciconfig hci0 up/down first.
that should not be the case. Actually hciconfig uses old ioctl. A full
Bluetooth system running bluetoothd (from BlueZ 5) will never use any old
ioctl. Only an old system (BlueZ 4) will use ioctl.
> I have trouble understanding... h4p_hci_open() seems to be called as
> soon as I insmod the driver. Who does that? Is it some kind of udev
> magic?
As soon as you do hci_register_dev, it will bring up the device and run the
basic initialization. That is needed to get the address, version information
and features. Otherwise the mgmt interface can not work. We need basic
information about the device.
So what the kernel will do internally when you find a device is bring it up,
get the basics and then bring it back down (in case nobody uses it). See
hci_power_on work.
> ...
>>> +#include "hci_h4p.h"
>>> +
>>> +#define BT_DBG(a...) do {} while(0)
>>
>> Why is this needed? BT_DBG is hooked up with dynamic debug.
> ...
>
> I did all the changes as requested. Thanks. I did't see harm in
> include guards, but I removed them; it is not important.
There is no harm in including the guards. We just don't do it for internal
files. And thus if anyone tries to be sneaky and build circular inclusion they
will fall flat on their faces. That is the only reason.
>
>>> + if (info->rx_count == 0) {
>>> + /* H4+ devices should always send word aligned packets */
>>> + if (!(info->rx_skb->len % 2))
>>> + info->garbage_bytes++;
>>> + h4p_recv_frame(info, info->rx_skb);
>>
>> The h4p_recv_frame should maybe done inline here since it only handles 2
>> exception packets. Also to make it easy, just leave the function if
>> (info->rx_count).
>>
>> This packet processing feels like a bit of spaghetti code. I think that is
>> better handled in a proper function that goes through it and not with many
>> tiny sub functions.
>>
>
> Well, CodingStyle says something about functions that should be kept
> short... And when manually inlined, the inner function would have to
> be made less readable, as it uses return to shortcut processing.
It also should not make me have to follow 3 functions to figure out what it is
actually doing.
>
>
> To use __hci_cmd_sync()
>
>>> +
>>> + SET_HCIDEV_DEV(hdev, info->dev);
>>> +
>>> + if (hci_register_dev(hdev) >= 0)
>>> + return 0;
>>> +
>>> + dev_err(info->dev, "hci_register failed %s.\n", hdev->name);
>>> + hci_free_dev(info->hdev);
>>> + return -ENODEV;
>>> +}
>>
>> And normally this is folded into the probe handling and not in a
>> separate function.
>
> The probe function is too long, already...
Have you compared it to other functions. Normally probe() functions in general
are all pretty long since they have to do tons of stuff. Having all in one
function means you can read through it at once.
>
>>> +#include
>>> +#include
>>> +
>>> +#include
>>> +
>>> +#include "hci_h4p.h"
>>
>> Why is this a separate file? And if so, why not all UART specific details
>> are in this file. Including bunch of the defines you have in the header.
>>
>
> Well, you wanted less global functions, so I moved some as inlines to
> headers. I guess I can merge nokia_core and nokia_uart if really wanted.
Would this become easier when some of the OMAP specific stuff is moved out of
this driver? If that is possible.
Regards
Marcel
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bluetooth: Add hci_h4p driver
Hi!
> And you want to set the QUIRK_INVALID_BADDR. At least you want to do that in
> the final submission.
>
Ok, I found out that I can do it and result works, provided that I do
hciconfig hci0 up/down first.
I have trouble understanding... h4p_hci_open() seems to be called as
soon as I insmod the driver. Who does that? Is it some kind of udev
magic?
...
> > +#include "hci_h4p.h"
> > +
> > +#define BT_DBG(a...) do {} while(0)
>
> Why is this needed? BT_DBG is hooked up with dynamic debug.
...
I did all the changes as requested. Thanks. I did't see harm in
include guards, but I removed them; it is not important.
> > + if (info->rx_count == 0) {
> > + /* H4+ devices should always send word aligned packets */
> > + if (!(info->rx_skb->len % 2))
> > + info->garbage_bytes++;
> > + h4p_recv_frame(info, info->rx_skb);
>
> The h4p_recv_frame should maybe done inline here since it only handles 2
> exception packets. Also to make it easy, just leave the function if
> (info->rx_count).
>
> This packet processing feels like a bit of spaghetti code. I think that is
> better handled in a proper function that goes through it and not with many
> tiny sub functions.
>
Well, CodingStyle says something about functions that should be kept
short... And when manually inlined, the inner function would have to
be made less readable, as it uses return to shortcut processing.
To use __hci_cmd_sync()
> > +
> > + SET_HCIDEV_DEV(hdev, info->dev);
> > +
> > + if (hci_register_dev(hdev) >= 0)
> > + return 0;
> > +
> > + dev_err(info->dev, "hci_register failed %s.\n", hdev->name);
> > + hci_free_dev(info->hdev);
> > + return -ENODEV;
> > +}
>
> And normally this is folded into the probe handling and not in a
> separate function.
The probe function is too long, already...
> > +#include
> > +#include
> > +
> > +#include
> > +
> > +#include "hci_h4p.h"
>
> Why is this a separate file? And if so, why not all UART specific details are
> in this file. Including bunch of the defines you have in the header.
>
Well, you wanted less global functions, so I moved some as inlines to
headers. I guess I can merge nokia_core and nokia_uart if really wanted.
I did rest of requested changes.
Thanks,
Pavel
--
(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-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bluetooth: Add hci_h4p driver
Hi!
On Thu 2014-12-18 20:31:53, Pavel Machek wrote:
> Hi!
>
> > > + h4p_simple_send_frame(info, skb);
> > > +
> > > + if (!wait_for_completion_interruptible_timeout(&info->init_completion,
> > > +msecs_to_jiffies(1000)))
> > > {
> > > + printk("h4p: negotiation did not return\n");
> >
> > Memory leak in the error case
>
> And memory leak in the normal case, too, no? Fixed.
Actually, no, h4p_simple_send_frame passes skb to network stack, and
it should free it as neccessary.
Pavel
--
(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-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bluetooth: Add hci_h4p driver
Hi!
> > + h4p_simple_send_frame(info, skb);
> > +
> > + if (!wait_for_completion_interruptible_timeout(&info->init_completion,
> > + msecs_to_jiffies(1000)))
> > {
> > + printk("h4p: negotiation did not return\n");
>
> Memory leak in the error case
And memory leak in the normal case, too, no? Fixed.
> > + case H4_ACL_PKT:
> > + acl_hdr = (struct hci_acl_hdr *)skb->data;
> > + retval = le16_to_cpu(acl_hdr->dlen);
>
> Could you explain, why only this needs endianness converted?
This one is 16 bit, the others I checked are 8 bit.
> > +static void h4p_rx_tasklet(unsigned long data)
> > +{
> > + u8 byte;
> > + struct h4p_info *info = (struct h4p_info *)data;
> > +
> > + BT_DBG("tasklet woke up");
> > + BT_DBG("rx_tasklet woke up");
>
> Isn't this a bit redundant?
Fixed.
> > + struct sk_buff *skb;
> > + struct h4p_info *info = (struct h4p_info *)data;
> > +
> > + BT_DBG("tasklet woke up");
> > + BT_DBG("tx_tasklet woke up");
>
> Doubled?
Fixed.
Thanks,
Pavel
--
(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-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bluetooth: Add hci_h4p driver
Hi Pavel,
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 4547dc2..0fc7d3a 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -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 "HCI driver with H4 Nokia extensions"
> + depends on ARCH_OMAP
this dependency needs to go away. The actual loading and detection of the
driver should purely be based on DT. All OMAP specific details should not
really be part of the Bluetooth driver.
> + help
> + Bluetooth HCI driver with H4 extensions. This driver provides
> + support for H4+ Bluetooth chip with vendor-specific H4 extensions.
> +
> + Say Y here to compile support for h4 extended devices into the kernel
> + or say M to compile it as module (btnokia_h4p).
> +
> endmenu
> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
> index 9fe8a87..ec7074fe 100644
> --- a/drivers/bluetooth/Makefile
> +++ b/drivers/bluetooth/Makefile
> @@ -31,4 +31,7 @@ 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) += hci_h4p.o
> +hci_h4p-objs := nokia_core.o nokia_fw.o nokia_uart.o
> +
We are moving away from calling drivers hci_ since that is a bit too generic.
Just call the the driver object nokia_h4p.o for now.
> ccflags-y += -D__CHECK_ENDIAN__
> new file mode 100644
> index 000..6445a99
> --- /dev/null
> +++ b/drivers/bluetooth/hci_h4p.h
this should be nokia_h4p.h then.
> @@ -0,0 +1,234 @@
> +/*
> + * This file is part of Nokia H4P bluetooth driver
> + *
> + * Copyright (C) 2005-2008 Nokia Corporation.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + *
> + */
> +
> +#ifndef __DRIVERS_BLUETOOTH_H4P_H
> +#define __DRIVERS_BLUETOOTH_H4P_H
Skip these circular inclusion protection checks. The header file is local and
you know what you are doing.
> +
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#define UART_SYSC_OMAP_RESET 0x03
> +#define UART_SYSS_RESETDONE 0x01
> +#define UART_OMAP_SCR_EMPTY_THR 0x08
> +#define UART_OMAP_SCR_WAKEUP 0x10
> +#define UART_OMAP_SSR_WAKEUP 0x02
> +#define UART_OMAP_SSR_TXFULL 0x01
> +
> +#define UART_OMAP_SYSC_IDLEMODE 0x03
> +#define UART_OMAP_SYSC_IDLEMASK (3 << UART_OMAP_SYSC_IDLEMODE)
> +
> +#define UART_OMAP_SYSC_FORCE_IDLE(0 << UART_OMAP_SYSC_IDLEMODE)
> +#define UART_OMAP_SYSC_NO_IDLE (1 << UART_OMAP_SYSC_IDLEMODE)
> +#define UART_OMAP_SYSC_SMART_IDLE(2 << UART_OMAP_SYSC_IDLEMODE)
> +
> +#define H4P_TRANSFER_MODE1
> +#define H4P_SCHED_TRANSFER_MODE 2
> +#define H4P_ACTIVE_MODE 3
> +
> +struct h4p_info {
> + struct timer_list lazy_release;
> + struct hci_dev *hdev;
> + spinlock_t lock;
> +
> + void __iomem *uart_base;
> + unsigned long uart_phys_base;
> + int irq;
> + struct device *dev;
> + u8 chip_type;
> + u8 bt_wakeup_gpio;
> + u8 host_wakeup_gpio;
> + u8 reset_gpio;
> + u8 reset_gpio_shared;
> + u8 bt_sysclk;
> + u8 man_id;
> + u8 ver_id;
> +
> + struct sk_buff_head fw_queue;
> + struct sk_buff *alive_cmd_skb;
> + struct completion init_completion;
> + struct completion fw_completion;
> + struct completion test_completion;
> + int fw_error;
> + int init_error;
> +
> + struct sk_buff_head txq;
> +
> + struct sk_buff *rx_skb;
> + long rx_count;
> + unsigned long rx_state;
> + unsigned long garbage_bytes;
> +
> + bdaddr_t bd_addr;
Remove this one. Let the core handle any BD_ADDR related details.
> + struct sk_buff_head *fw_q;
> +
> + int pm_enabled;
> + int tx_enabled;
> + int autorts;
> + int rx_enabled;
> + unsigned long pm_flags;
> +
> + int tx_clocks_en;
> + int rx_clocks_en;
> + spinlock_t clocks_lock;
> + struct clk *uart_iclk;
> + struct clk *uart_fclk;
> + atomic_t clk_users;
> + u16 dll;
> + u16 dlh;
> + u16 ier;
> + u16 mdr1;
> + u16 efr;
> +
> + int initing;
> +};
> +
Re: bluetooth: Add hci_h4p driver
Hi,
a few remarks about possible issues.
Regards
Oliver
> +static int h4p_send_negotiation(struct h4p_info *info)
> +{
> + struct h4p_neg_cmd *neg_cmd;
> + struct h4p_neg_hdr *neg_hdr;
> + struct sk_buff *skb;
> + int err, len;
> + u16 sysclk = 38400;
> +
> + printk("Sending negotiation..");
> + len = sizeof(*neg_cmd) + sizeof(*neg_hdr) + H4_TYPE_SIZE;
> +
> + skb = bt_skb_alloc(len, GFP_KERNEL);
> + if (!skb)
> + return -ENOMEM;
> +
> + memset(skb->data, 0x00, len);
> + *skb_put(skb, 1) = H4_NEG_PKT;
> + neg_hdr = (struct h4p_neg_hdr *)skb_put(skb, sizeof(*neg_hdr));
> + neg_cmd = (struct h4p_neg_cmd *)skb_put(skb, sizeof(*neg_cmd));
> +
> + neg_hdr->dlen = sizeof(*neg_cmd);
> + neg_cmd->ack = H4P_NEG_REQ;
> + neg_cmd->baud = cpu_to_le16(BT_BAUDRATE_DIVIDER/MAX_BAUD_RATE);
> + neg_cmd->proto = H4P_PROTO_BYTE;
> + neg_cmd->sys_clk = cpu_to_le16(sysclk);
> +
> + h4p_change_speed(info, INIT_SPEED);
> +
> + h4p_set_rts(info, 1);
> + info->init_error = 0;
> + init_completion(&info->init_completion);
> +
> + h4p_simple_send_frame(info, skb);
> +
> + if (!wait_for_completion_interruptible_timeout(&info->init_completion,
> +msecs_to_jiffies(1000)))
> {
> + printk("h4p: negotiation did not return\n");
Memory leak in the error case
> + return -ETIMEDOUT;
> + }
> +
> + if (info->init_error < 0)
> + return info->init_error;
> +
> + /* Change to operational settings */
> + h4p_set_auto_ctsrts(info, 0, UART_EFR_RTS);
> + h4p_set_rts(info, 0);
> + h4p_change_speed(info, MAX_BAUD_RATE);
> +
> + err = h4p_wait_for_cts(info, 1, 100);
> + if (err < 0)
> + return err;
> +
> + h4p_set_auto_ctsrts(info, 1, UART_EFR_RTS);
> + init_completion(&info->init_completion);
> + err = h4p_send_alive_packet(info);
> +
> + if (err < 0)
> + return err;
> +
> + if (!wait_for_completion_interruptible_timeout(&info->init_completion,
> + msecs_to_jiffies(1000)))
> + return -ETIMEDOUT;
> +
> + if (info->init_error < 0)
> + return info->init_error;
> +
> + printk("Negotiation successful\n");
> + return 0;
> +}
> +static unsigned int h4p_get_data_len(struct h4p_info *info,
> + struct sk_buff *skb)
> +{
> + long retval = -1;
> + struct hci_acl_hdr *acl_hdr;
> + struct hci_sco_hdr *sco_hdr;
> + struct hci_event_hdr *evt_hdr;
> + struct h4p_neg_hdr *neg_hdr;
> + struct h4p_alive_hdr *alive_hdr;
> + struct h4p_radio_hdr *radio_hdr;
> +
> + switch (bt_cb(skb)->pkt_type) {
> + case H4_EVT_PKT:
> + evt_hdr = (struct hci_event_hdr *)skb->data;
> + retval = evt_hdr->plen;
> + break;
> + case H4_ACL_PKT:
> + acl_hdr = (struct hci_acl_hdr *)skb->data;
> + retval = le16_to_cpu(acl_hdr->dlen);
Could you explain, why only this needs endianness converted?
> + break;
> + case H4_SCO_PKT:
> + sco_hdr = (struct hci_sco_hdr *)skb->data;
> + retval = sco_hdr->dlen;
> + break;
> + case H4_RADIO_PKT:
> + radio_hdr = (struct h4p_radio_hdr *)skb->data;
> + retval = radio_hdr->dlen;
> + break;
> + case H4_NEG_PKT:
> + neg_hdr = (struct h4p_neg_hdr *)skb->data;
> + retval = neg_hdr->dlen;
> + break;
> + case H4_ALIVE_PKT:
> + alive_hdr = (struct h4p_alive_hdr *)skb->data;
> + retval = alive_hdr->dlen;
> + break;
> + }
> +
> + return retval;
> +}
> +static void h4p_rx_tasklet(unsigned long data)
> +{
> + u8 byte;
> + struct h4p_info *info = (struct h4p_info *)data;
> +
> + BT_DBG("tasklet woke up");
> + BT_DBG("rx_tasklet woke up");
Isn't this a bit redundant?
> +
> + while (h4p_inb(info, UART_LSR) & UART_LSR_DR) {
> + byte = h4p_inb(info, UART_RX);
> + BT_DBG("[in: %02x]", byte);
> + if (info->garbage_bytes) {
> + info->garbage_bytes--;
> + continue;
> + }
> + if (info->rx_skb == NULL) {
> + info->rx_skb = bt_skb_alloc(HCI_MAX_FRAME_SIZE,
> + GFP_ATOMIC | GFP_DMA);
> + if (!info->rx_skb) {
> + dev_err(info->dev,
> + "No memory for new packet\n");
> + goto finish_rx;
> + }
> + info->rx_state = WAIT_FOR_PKT_TYPE;
> + info->rx_skb->dev = (void *)info->hdev;
> + }
> + info->hdev->stat.byte_rx++;
> + h4p_hand
Re: bluetooth: Add hci_h4p driver
Hi! > >>> Hacks surrounding bluetooth address were removed; this results in > >>> working driver with address that is probably not unique. > >> > >> Just set HCI_QUIRK_INVALID_BDADDR and let someone deal with that in > >> userspace. You can use the btmgmt public-addr command for testing. > >> > > > > Ok, it took me a while to figure out that --enable-experimental is > > needed. > > > > Then help was playing tricks with me: > > > > For more information on the usage of each command use: > >btmgmt --help > > root@n900:/my/bluez-5.26/tools# ./btmgmt public-addr --help > > Set Public Address for hci0 failed with status 0x0b (Rejected) > > root@n900:/my/bluez-5.26/tools# ./btmgmt public-addr > > Usage: btmgmt public-addr > > that is indeed confusing. Something went wrong with that tool. Just keep in > mind that tool is just for internal testing by the developers. Someone would > still need to write a proper integration with listening for the appropriate > events and dig out the actual address from the memory location. > You have patch that helps that issues in the email :-). > > root@n900:/my/bluez-5.26/tools# ./btmgmt public-addr 01:02:03:04:05:06 > > Set Public Address for hci0 failed with status 0x0b (Rejected) > > > > Hmm. Since setting public address only works when interface is down, > > and we lose all the settings during up, this does not work at all, > > right? > > It does keep it. The address is stored in hdev->public_addr which is actually > different from hdev->bdaddr which is the current address in use. > > However I am not sure that hdev->set_bdaddr is executed again. Same > as hdev->setup is not executed twice. The controller loosing all > states is something we have not yet encountered. At least not > while the hci_dev stays around. It looks like that's the exact issue. It seems to me like set_bdaddr is actually called while the device is "down". > I wonder if the drastic power off might be better hooked into a > platform RFKILL switch. And with that you would just unregister > hci_dev and re-register it when the RFKILL switch is unblocked. Ok, so kernel currently wants bluetooth out of reset at > The other option is to introduce a quirk that allows running hdev->setup and > hdev->set_bdaddr all the time when you bring up your device. > > Another alternate idea for getting something upstream for testing is to > ignore the whole firmware loading in the kernel and bring up the controller > with HCI_QUIRK_EXTERNAL_CONFIG. Doing that would allow you to do bette > testing and evolve the driver in small tests. > Actually, firmware loading is _not_ the biggest problem. That is pretty self-contained now. I guess I should just configure it at probe time, put it into reset during rmmod, keep it out of reset otherwise, and see how it works. Thanks, Pavel -- (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-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bluetooth: Add hci_h4p driver
Hi! > > Firmware load was converted to hci_cmd_sync(). Unfortunately, the > > firmware is needed after every open/close, so the setup mechanism does > > not quite fit. (But code is now way cleaner). > > What is the reason for that? Does it mean that the device will always loose > all its settings when powering it down. Do we know why that is that way and > can we maybe change it? > Well, on hci_close(), original code asserts reset GPIO. I guess that clears everything... I got it working with that removed, and it means I can set the address the normal way. What are implications for power consumption... I'm not sure at this point. Lets worry about that later. Pavel -- (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-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bluetooth: Add hci_h4p driver
Hi Pavel, >>> Hacks surrounding bluetooth address were removed; this results in >>> working driver with address that is probably not unique. >> >> Just set HCI_QUIRK_INVALID_BDADDR and let someone deal with that in >> userspace. You can use the btmgmt public-addr command for testing. >> > > Ok, it took me a while to figure out that --enable-experimental is > needed. > > Then help was playing tricks with me: > > For more information on the usage of each command use: >btmgmt --help > root@n900:/my/bluez-5.26/tools# ./btmgmt public-addr --help > Set Public Address for hci0 failed with status 0x0b (Rejected) > root@n900:/my/bluez-5.26/tools# ./btmgmt public-addr > Usage: btmgmt public-addr that is indeed confusing. Something went wrong with that tool. Just keep in mind that tool is just for internal testing by the developers. Someone would still need to write a proper integration with listening for the appropriate events and dig out the actual address from the memory location. > root@n900:/my/bluez-5.26/tools# ./btmgmt public-addr 01:02:03:04:05:06 > Set Public Address for hci0 failed with status 0x0b (Rejected) > > Hmm. Since setting public address only works when interface is down, > and we lose all the settings during up, this does not work at all, > right? It does keep it. The address is stored in hdev->public_addr which is actually different from hdev->bdaddr which is the current address in use. However I am not sure that hdev->set_bdaddr is executed again. Same as hdev->setup is not executed twice. The controller loosing all states is something we have not yet encountered. At least not while the hci_dev stays around. I wonder if the drastic power off might be better hooked into a platform RFKILL switch. And with that you would just unregister hci_dev and re-register it when the RFKILL switch is unblocked. The other option is to introduce a quirk that allows running hdev->setup and hdev->set_bdaddr all the time when you bring up your device. Another alternate idea for getting something upstream for testing is to ignore the whole firmware loading in the kernel and bring up the controller with HCI_QUIRK_EXTERNAL_CONFIG. Doing that would allow you to do bette testing and evolve the driver in small tests. So what this quirk does is that the hci_dev comes up unconfigured. So mgmt interface will only show it as unconfigured controller. However you can open it with HCI User Channel and run all HCI commands manually. That is what src/shared/hci.[ch] is good at. Once you are done, you just flip the switch via btmgmt ext-config and then the kernel goes and treats it as fully configured. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bluetooth: Add hci_h4p driver
Hi!
> > Hacks surrounding bluetooth address were removed; this results in
> > working driver with address that is probably not unique.
>
> Just set HCI_QUIRK_INVALID_BDADDR and let someone deal with that in
> userspace. You can use the btmgmt public-addr command for testing.
>
Ok, it took me a while to figure out that --enable-experimental is
needed.
Then help was playing tricks with me:
For more information on the usage of each command use:
btmgmt --help
root@n900:/my/bluez-5.26/tools# ./btmgmt public-addr --help
Set Public Address for hci0 failed with status 0x0b (Rejected)
root@n900:/my/bluez-5.26/tools# ./btmgmt public-addr
Usage: btmgmt public-addr
root@n900:/my/bluez-5.26/tools# ./btmgmt public-addr 01:02:03:04:05:06
Set Public Address for hci0 failed with status 0x0b (Rejected)
Hmm. Since setting public address only works when interface is down,
and we lose all the settings during up, this does not work at all,
right?
Anyway, you might want to apply this to lessen the confusion.
diff -ur bluez-5.26.ofic/tools/btmgmt.c bluez-5.26/tools/btmgmt.c
--- bluez-5.26.ofic/tools/btmgmt.c 2014-12-14 12:32:19.742595000 +0100
+++ bluez-5.26/tools/btmgmt.c 2014-12-14 20:06:40.432497000 +0100
@@ -2603,7 +2603,7 @@
static void static_addr_usage(void)
{
- printf("Usage: btmgmt static-addr \n");
+ printf("Usage: btmgmt static-addr ??:??:??:??:??:??\n");
}
static void cmd_static_addr(struct mgmt *mgmt, uint16_t index,
@@ -2660,7 +2660,8 @@
struct mgmt_cp_set_public_address cp;
if (argc < 2) {
- printf("Usage: btmgmt public-addr \n");
+ printf("Usage: btmgmt public-addr ??:??:??:??:??:??\n"
+ "Note: interface must be down for this to work\n");
exit(EXIT_FAILURE);
}
@@ -2934,7 +2935,7 @@
static void add_device_usage(void)
{
- printf("Usage: btmgmt add-device [-a action] [-t type] \n");
+ printf("Usage: btmgmt add-device [-a action] [-t type]
??:??:??:??:??:??\n");
}
static struct option add_device_options[] = {
@@ -3007,7 +3008,7 @@
static void del_device_usage(void)
{
- printf("Usage: btmgmt del-device [-t type] \n");
+ printf("Usage: btmgmt del-device [-t type] ??:??:??:??:??:??\n");
}
static struct option del_device_options[] = {
@@ -3153,7 +3154,7 @@
printf("\n"
"For more information on the usage of each command use:\n"
- "\tbtmgmt --help\n" );
+ "\tbtmgmt \n" );
}
static struct option main_options[] = {
Only in bluez-5.26/tools: btmgmt.c~
--
(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-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bluetooth: Add hci_h4p driver
On Sun 2014-12-14 00:30:13, Marcel Holtmann wrote: > Hi Pavel, > > > Add hci_h4p bluetooth driver to staging tree. This device is used > > for example on Nokia N900 cell phone. > > > > Signed-off-by: Pavel Machek > > Thanks-to: Sebastian Reichel > > Thanks-to: Joe Perches > > > > --- > > > > > > I'd prefer to resurect the driver in staging/ in order not to lose > > history, but Marcel wanted to treat it as new submission, so I'm doing > > that. > > that history is in linux.git now for all times. No need to repeat it. I > rather not play around with that again. Lets get a minimal driver merged so > we can give people something to improve. > > > > > Firmware load was converted to hci_cmd_sync(). Unfortunately, the > > firmware is needed after every open/close, so the setup mechanism does > > not quite fit. (But code is now way cleaner). > > What is the reason for that? Does it mean that the device will always loose > all its settings when powering it down. Do we know why that is that way and > can we maybe change it? > > If there is no way around this, we can introduce a quirk that will always run > hdev->setup. However if the device keeps forgetting all settings all the > time, that means it will also keep forgetting its address. So that power on > procedure will be wasting time. We would need to check if we can make it so > that it only has unconfigured state once and then keeps remembering the > address even if we have to re-program it every time. > > > Device tree bindings work for me, but they are not yet official and I > > expect some more fun there. > > > > Hacks surrounding bluetooth address were removed; this results in > > working driver with address that is probably not unique. > > Just set HCI_QUIRK_INVALID_BDADDR and let someone deal with that in >userspace. You can use the btmgmt public-addr command for testing. Hmm. So I installed bluez-5.36, then libudev-dev, then libical-dev, libreadline-dev. I'm glad I have debian and not maemo, and nfsroot and not limited flash. I had to turn off systemd support. It still compiles.. Pavel -- (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-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bluetooth: Add hci_h4p driver
On Sun 2014-12-14 00:30:13, Marcel Holtmann wrote: > Hi Pavel, > > > Add hci_h4p bluetooth driver to staging tree. This device is used > > for example on Nokia N900 cell phone. > > > > Signed-off-by: Pavel Machek > > Thanks-to: Sebastian Reichel > > Thanks-to: Joe Perches > > > > --- > > > > > > I'd prefer to resurect the driver in staging/ in order not to lose > > history, but Marcel wanted to treat it as new submission, so I'm doing > > that. > > that history is in linux.git now for all times. No need to repeat it. I > rather not play around with that again. Lets get a minimal driver merged so > we can give people something to improve. > We will merge minimum version. Still I'd like complete history for credits and bisect and easy ability to diff original and minimum version. > > Firmware load was converted to hci_cmd_sync(). Unfortunately, the > > firmware is needed after every open/close, so the setup mechanism does > > not quite fit. (But code is now way cleaner). > > What is the reason for that? Does it mean that the device will always loose > all its settings when powering it down. Do we know why that is that way and > can we maybe change it? > > If there is no way around this, we can introduce a quirk that will always run > hdev->setup. However if the device keeps forgetting all settings all the > time, that means it will also keep forgetting its address. So that power on > procedure will be wasting time. We would need to check if we can make it so > that it only has unconfigured state once and then keeps remembering the > address even if we have to re-program it every time. > > > Device tree bindings work for me, but they are not yet official and I > > expect some more fun there. > > > > Hacks surrounding bluetooth address were removed; this results in > > working driver with address that is probably not unique. > > Just set HCI_QUIRK_INVALID_BDADDR and let someone deal with that in > userspace. You can use the btmgmt public-addr command for testing. What command would that be? Do I need some new versions of tools? root@n900:/my/modules# man hciconfig (don't see anything relevant). root@n900:/my/modules# btmgmt -bash: btmgmt: command not found root@n900:/my/modules# apt-cache search btmgmt root@n900:/my/modules# > On the kernel side you need to add hdev->set_bdaddr support. For the Broadcom > based devices you can copy it from the btusb.c driver. > I tried doing that from setup, but either command failed or it killed hte kernel. > > HCI_QUIRK_RESET_ON_CLOSE will need to be investigated some more. > > Why was that needed again? If the device loses power anyway, then that seems > wasteful. Also the core changed to make sure it resets all settings on power > down correctly. > Ok. > > My notes say that Marcel wanted different filenames, but I'd need > > advice exactly what filenames. I guess platform data supprort should > > be removed altogether, rather than renamed. > > Yes, the platform support should go away. This should be purely based on DT. > > Can you also start using git format-patch so that patches get a diffstat. I can do diffstat manually for you, git format-patch does not fit my workflow. Thanks, Pavel -- (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-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bluetooth: Add hci_h4p driver
On Sun 2014-12-14 01:07:42, Marcel Holtmann wrote: > Hi Pavel, > > >>> My notes say that Marcel wanted different filenames, but I'd need > >>> advice exactly what filenames. I guess platform data supprort should > >>> be removed altogether, rather than renamed. > >> > >> Yes, the platform support should go away. This should be purely based on > >> DT. > >> > >> Can you also start using git format-patch so that patches get a diffstat. > > > > So the rest of filenames look ok? > > I have not looked at the patch or the filenames at all. Ok, so here's diffstat. Can you take a look? Thanks, Pavel drivers/bluetooth/Kconfig | 10 drivers/bluetooth/Makefile |3 drivers/bluetooth/hci_h4p.h| 234 + drivers/bluetooth/nokia_core.c | 1262 + drivers/bluetooth/nokia_fw-bcm.c | 27 drivers/bluetooth/nokia_fw.c | 105 ++ drivers/bluetooth/nokia_uart.c | 179 include/linux/platform_data/bt-nokia-h4p.h |4 8 files changed, 1822 insertions(+), 2 deletions(-) -- (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-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bluetooth: Add hci_h4p driver
Hi Pavel, >>> My notes say that Marcel wanted different filenames, but I'd need >>> advice exactly what filenames. I guess platform data supprort should >>> be removed altogether, rather than renamed. >> >> Yes, the platform support should go away. This should be purely based on DT. >> >> Can you also start using git format-patch so that patches get a diffstat. > > So the rest of filenames look ok? I have not looked at the patch or the filenames at all. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bluetooth: Add hci_h4p driver
Hi! > > My notes say that Marcel wanted different filenames, but I'd need > > advice exactly what filenames. I guess platform data supprort should > > be removed altogether, rather than renamed. > > Yes, the platform support should go away. This should be purely based on DT. > > Can you also start using git format-patch so that patches get a diffstat. So the rest of filenames look ok? Pavel -- (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-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: bluetooth: Add hci_h4p driver
Hi Pavel, > Add hci_h4p bluetooth driver to staging tree. This device is used > for example on Nokia N900 cell phone. > > Signed-off-by: Pavel Machek > Thanks-to: Sebastian Reichel > Thanks-to: Joe Perches > > --- > > > I'd prefer to resurect the driver in staging/ in order not to lose > history, but Marcel wanted to treat it as new submission, so I'm doing > that. that history is in linux.git now for all times. No need to repeat it. I rather not play around with that again. Lets get a minimal driver merged so we can give people something to improve. > > Firmware load was converted to hci_cmd_sync(). Unfortunately, the > firmware is needed after every open/close, so the setup mechanism does > not quite fit. (But code is now way cleaner). What is the reason for that? Does it mean that the device will always loose all its settings when powering it down. Do we know why that is that way and can we maybe change it? If there is no way around this, we can introduce a quirk that will always run hdev->setup. However if the device keeps forgetting all settings all the time, that means it will also keep forgetting its address. So that power on procedure will be wasting time. We would need to check if we can make it so that it only has unconfigured state once and then keeps remembering the address even if we have to re-program it every time. > Device tree bindings work for me, but they are not yet official and I > expect some more fun there. > > Hacks surrounding bluetooth address were removed; this results in > working driver with address that is probably not unique. Just set HCI_QUIRK_INVALID_BDADDR and let someone deal with that in userspace. You can use the btmgmt public-addr command for testing. In the end someone needs to write a small tool that users mgmt interface and listens for unconfigured controllers, reads the address from whatever storage it is, uses the Set Public Address command and that is it. On the kernel side you need to add hdev->set_bdaddr support. For the Broadcom based devices you can copy it from the btusb.c driver. > HCI_QUIRK_RESET_ON_CLOSE will need to be investigated some more. Why was that needed again? If the device loses power anyway, then that seems wasteful. Also the core changed to make sure it resets all settings on power down correctly. > My notes say that Marcel wanted different filenames, but I'd need > advice exactly what filenames. I guess platform data supprort should > be removed altogether, rather than renamed. Yes, the platform support should go away. This should be purely based on DT. Can you also start using git format-patch so that patches get a diffstat. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
