Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752551AbcCBGsB (ORCPT ); Wed, 2 Mar 2016 01:48:01 -0500 Received: from mx0a-0016f401.pphosted.com ([67.231.148.174]:42600 "EHLO mx0a-0016f401.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750865AbcCBGsA convert rfc822-to-8bit (ORCPT ); Wed, 2 Mar 2016 01:48:00 -0500 From: Amitkumar Karwar To: One Thousand Gnomes CC: "linux-bluetooth@vger.kernel.org" , "Cathy Luo" , "linux-kernel@vger.kernel.org" , Nishant Sarmukadam , Ganapathi Bhat Subject: RE: [PATCH v3] Bluetooth: hci_uart: Support firmware download for Marvell Thread-Topic: [PATCH v3] Bluetooth: hci_uart: Support firmware download for Marvell Thread-Index: AQHRc92M20b2YnI+JUmlkJ5jXBDqSp9FqZwAgAANMWA= Date: Wed, 2 Mar 2016 06:47:50 +0000 Message-ID: References: <1456852109-14298-1-git-send-email-akarwar@marvell.com> <20160301215924.6b884508@lxorguk.ukuu.org.uk> In-Reply-To: <20160301215924.6b884508@lxorguk.ukuu.org.uk> 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-03-02_02:,, 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-1601100000 definitions=main-1603020120 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2796 Lines: 116 Hi Alan, > From: One Thousand Gnomes [mailto:gnomes@lxorguk.ukuu.org.uk] > Sent: Wednesday, March 02, 2016 3:29 AM > To: Amitkumar Karwar > Cc: linux-bluetooth@vger.kernel.org; Cathy Luo; linux- > kernel@vger.kernel.org; Nishant Sarmukadam; Ganapathi Bhat > Subject: Re: [PATCH v3] Bluetooth: hci_uart: Support firmware download > for Marvell > > O > > +/* Get standard baud rate, given the speed */ static unsigned int > > +get_baud_rate(unsigned int speed) { > > + switch (speed) { > > + case 9600: > > + return B9600; > > + case 19200: > > + return B19200; > > + case 38400: > > + return B38400; > > + case 57600: > > + return B57600; > > + case 115200: > > + return B115200; > > + case 230400: > > + return B230400; > > + case 460800: > > + return B460800; > > + case 921600: > > + return B921600; > > + case 2000000: > > + return B2000000; > > + case 3000000: > > + return B3000000; > > + default: > > + return -1; > > + } > > +} > > > NAK. Please use the existing kernel helpers for this > > > + > > +/* Set terminal properties */ > > +static int mrvl_set_term_baud(struct tty_struct *tty, unsigned int > speed, > > + unsigned char flow_ctl) > > +{ > > + struct ktermios old_termios = tty->termios; > > + int baud; > > + > > + tty->termios.c_cflag &= ~CBAUD; > > + baud = get_baud_rate(speed); > > + > > + if (baud == -1) { > > + BT_ERR("Baud rate not supported"); > > + return -1; > > + } > > + > > + tty->termios.c_cflag |= baud; > > + > > This isn't the correct way to do any of this, just do > > tty_termios_encode_baud_rate(&tty->termios, speed, speed) > > > > + if (flow_ctl) > > + tty->termios.c_cflag |= CRTSCTS; > > + else > > + tty->termios.c_cflag &= ~CRTSCTS; > > + > > + tty->ops->set_termios(tty, &old_termios); > > Call the provided kernel helpers that get the locking right. > > tty_set_termios(tty, &new_termios); > > You should also do your error checking here and see what baud rate was > actually provided by the hardware (tty_get_baud_rate(tty)) by checking > the value actually selected by the tty. > > > + /* restore uart settings */ > > + new_termios = tty->termios; > > + tty->termios.c_cflag = old_termios.c_cflag; > > + tty->ops->set_termios(tty, &new_termios); > > + clear_bit(HCI_UART_DNLD_FW, &hu->flags); > > Again use the proper helpers > > > + > > +set_baud: > > + ret = mrvl_set_baud(hu); > > + if (ret) > > + goto fail; > > + > > + mdelay(MRVL_DNLD_DELAY); > > Why not msleep() ? > > > + > > + return ret; > > +fail: > > + /* restore uart settings */ > > + new_termios = tty->termios; > > + tty->termios.c_cflag = old_termios.c_cflag; > > + tty->ops->set_termios(tty, &new_termios); > > + clear_bit(HCI_UART_DNLD_FW, &hu->flags); > > + > Thanks for review. We will work on these comments. Regards, Amitkumar