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

Path	csiph.com!eternal-september.org!feeder.eternal-september.org!border1.nntp.ams1.giganews.com!nntp.giganews.com!news.panservice.it!diesel.cu.mi.it!bofh.it!news.nic.it!robomod
From	Amitkumar Karwar <akarwar@marvell.com>
Newsgroups	linux.kernel
Subject	RE: [PATCH v3] Bluetooth: hci_uart: Support firmware download for Marvell
Date	Wed, 02 Mar 2016 07:50:02 +0100
Message-ID	<r88LE-3PD-5@gated-at.bofh.it> (permalink)
References	<r7W7L-3DI-15@gated-at.bofh.it> <r80uK-6zV-23@gated-at.bofh.it>
Thread-Topic	[PATCH v3] Bluetooth: hci_uart: Support firmware download for Marvell
Thread-Index	AQHRc92M20b2YnI+JUmlkJ5jXBDqSp9FqZwAgAANMWA=
Accept-Language	en-US
Content-Language	en-US
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	robomod@news.nic.it
List-ID	<linux-kernel.vger.kernel.org>
X-Mailing-List	linux-kernel@vger.kernel.org
Approved	robomod@news.nic.it
Lines	116
Organization	linux.* mail to news gateway
X-Original-Cc	"linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>, "Cathy Luo" <cluo@marvell.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Nishant Sarmukadam <nishants@marvell.com>, Ganapathi Bhat <gbhat@marvell.com>
X-Original-Date	Wed, 2 Mar 2016 06:47:50 +0000
X-Original-Message-ID	<f04d2664b8c54469a64495cc44445b79@SC-EXCH04.marvell.com>
X-Original-References	<1456852109-14298-1-git-send-email-akarwar@marvell.com> <20160301215924.6b884508@lxorguk.ukuu.org.uk>
X-Original-Sender	linux-kernel-owner@vger.kernel.org
Xref	csiph.com linux.kernel:1347811

Show key headers only | View raw

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

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

[PATCH v3] Bluetooth: hci_uart: Support firmware download for Marvell Amitkumar Karwar <akarwar@marvell.com> - 2016-03-01 18:20 +0100
  [PATCH BlueZ v3] tools/btattach: add marvell support Amitkumar Karwar <akarwar@marvell.com> - 2016-03-01 18:20 +0100
  Re: [PATCH v3] Bluetooth: hci_uart: Support firmware download for  Marvell One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> - 2016-03-01 23:00 +0100
    RE: [PATCH v3] Bluetooth: hci_uart: Support firmware download for  Marvell Amitkumar Karwar <akarwar@marvell.com> - 2016-03-02 07:50 +0100

csiph-web