Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932956AbcDTOeO (ORCPT ); Wed, 20 Apr 2016 10:34:14 -0400 Received: from mx0a-0016f401.pphosted.com ([67.231.148.174]:24309 "EHLO mx0a-0016f401.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754829AbcDTOeK convert rfc822-to-8bit (ORCPT ); Wed, 20 Apr 2016 10:34:10 -0400 From: Amitkumar Karwar To: Marcel Holtmann CC: Linux Bluetooth , LKML , Ganapathi Bhat Subject: RE: [PATCH v7] Bluetooth: hci_uart: Support firmware download for Marvell Thread-Topic: [PATCH v7] Bluetooth: hci_uart: Support firmware download for Marvell Thread-Index: AQHRjxYDCrmEZxJGlEWIcvhh9vcEqJ+TY7sA//+eY0A= Date: Wed, 20 Apr 2016 14:34:05 +0000 Message-ID: References: <1459845266-2771-1-git-send-email-akarwar@marvell.com> <2A69CC2B-1FC2-4148-8564-38335A87CCD7@holtmann.org> In-Reply-To: <2A69CC2B-1FC2-4148-8564-38335A87CCD7@holtmann.org> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.93.176.43] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-04-20_09:,, signatures=0 X-Proofpoint-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1603290000 definitions=main-1604200233 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1808 Lines: 56 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