Hi Alan, > From: One Thousand Gnomes [mailto:gnomes@xxxxxxxxxxxxxxxxxxx] > Sent: Wednesday, March 02, 2016 3:29 AM > To: Amitkumar Karwar > Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Cathy Luo; linux- > kernel@xxxxxxxxxxxxxxx; 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 -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
