Messages in this thread Patch in this message | | | From | Amitkumar Karwar <> | Subject | RE: [PATCH v7] Bluetooth: hci_uart: Support firmware download for Marvell | Date | Tue, 26 Apr 2016 10:39:23 +0000 |
| |
Hi Marcel,
> From: Marcel Holtmann [mailto:marcel@holtmann.org] > Sent: Friday, April 22, 2016 6:19 PM > To: Amitkumar Karwar > Cc: Linux Bluetooth; LKML; Ganapathi Bhat; Cathy Luo > Subject: Re: [PATCH v7] Bluetooth: hci_uart: Support firmware download > for Marvell > > Hi Amitkumar, > > >>>> + > >>>> +static int mrvl_setup(struct hci_uart *hu) { > >>>> + struct mrvl_data *mrvl = hu->priv; > >>>> + > >>>> + mrvl_init_fw_data(hu); > >>>> + set_bit(HCI_UART_DNLD_FW, &mrvl->flags); > >>>> + > >>>> + return hci_uart_dnld_fw(hu); > >>>> +} > >>> > >>> So this is clearly the wrong spot. When ->setup is called it is > >>> expected that HCI is ready. You are misusing it here. > >>> > >> > >> Sure. We will move this to mrvl_open() where HCI is not yet > initialized. > > > > We tried moving firmware download to mrvl_open(), but it's not > feasible. "hu->proto" is not yet initialized at that time. So when the > data/ack is received during firmware download, we can't have Marvell > specific handling. Also, I can see other vendor's (broadcomm, Intel) > have done firmware download in setup handler. > > firmware download in ->setup() is fine as long as it uses HCI commands. > If it does not use HCI commands, then we need to come up with something > new. > > The problem here is that for all intense and purposes once ->setup() is > called, then assumption is that you are in an HCI capable transport and > it is ready. >
I understand your concern. We are now planning to add new handler called "->prepare" where we can do firmware download. We are working on the change. Meanwhile let us know if you have any suggestion.
---------hci_ldisc.c--------- @@ -641,6 +641,10 @@ static int hci_uart_set_proto(struct hci_uart *hu, int id) hu->proto = p; set_bit(HCI_UART_PROTO_READY, &hu->flags);
+ err = p->prepare(hu); + if (err) + return err; + err = hci_uart_register_dev(hu); if (err) { --------------------------- Regards, Amitkumar
| |