Messages in this thread | | | From | Amitkumar Karwar <> | Subject | RE: [PATCH v7] Bluetooth: hci_uart: Support firmware download for Marvell | Date | Wed, 20 Apr 2016 14:34:05 +0000 |
| |
Hi Marcel,
Thanks for your review. We will address these comments in updated version.
> > + > > +/* Receive data */ > > +static int mrvl_recv(struct hci_uart *hu, const void *data, int > > +count) { > > + struct mrvl_data *mrvl = hu->priv; > > + > > + if (test_bit(HCI_UART_DNLD_FW, &mrvl->flags)) { > > + mrvl->fwdata->skb = mrvl_process_fw_data(hu, mrvl->fwdata- > >skb, > > + (u8 *)data, count); > > + if (IS_ERR(mrvl->fwdata->skb)) { > > + int err = PTR_ERR(mrvl->fwdata->skb); > > + > > + bt_dev_err(hu->hdev, > > + "Receive firmware data failed (%d)", err); > > + mrvl->fwdata->skb = NULL; > > + return err; > > + } > > + return 0; > > + } > > This part actually worries me a bit. Yes, we can do it this way, but in > general it sounds a bit more like we need a generic approach in > hci_ldisc.c to handle pre-HCI firmware loading and/or setup. > > In the btusb.c driver we added ->setup_on_usb callback. And for it > sounds like we need something similar here. So that hci_ldisc.c can > handle most of the basic TX/RX.
Even if we added "->setup_on_usb" in hci_ldisc.c, we will need protocol specific changes in receive path during firmware download. With this patch, those changes are smoothly handled in hci_mrvl.c file. [hci_uart_tty_receive] -> [proto->recv] -> [mrvl_recv] -> [normal Rx path/FW download Rx handling]
> > + > > +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.
Regards, Amitkumar
| |