RE: [PATCH v3] Bluetooth: hci_uart: Support firmware download for Marvell

From: Amitkumar Karwar
Date: Wed Mar 02 2016 - 01:48:05 EST


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