Re: [PATCH v7] Bluetooth: hci_uart: Support firmware download for Marvell

[Marcel Holtmann]

Hi Amitkumar,

> 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]

we might need to find a better callback name, but yes, we want to make this generic in hci_ldisc.c. I do not have a perfect solution or quick answer, but I think this needs a bit more generic framework.

Regards

Marcel