Re: [PATCH 2/5] Intel based H4 line discipline enhancements

[Arend van Spriel <arend@broadcom.com>]

On 06/06/15 17:33, Ilya Faenson wrote:
> Hi Marcel,
>
> Appreciate you applying Fred's patches. I will now put together and post similar patches against the bluetooth-next proper.
>
> Sorry for many minor changes. I just happen to run the checkpatch script automatically prior to publishing the patches (Arend taught me good Linux practices :-)) and that results in those spaces getting deleted et al.

Actually, checkpatch by default only does the check and just tell you 
what is wrong. Normally, it would only warn about the thing your patch 
modified. Unless you ran checkpatch on source file.

Regards,
Arend

> Thanks,
>   -Ilya
>
> -----Original Message-----
> From: Marcel Holtmann [mailto:marcel@holtmann.org]
> Sent: Saturday, June 06, 2015 2:37 AM
> To: Ilya Faenson
> Cc: linux-bluetooth@vger.kernel.org
> Subject: Re: [PATCH 2/5] Intel based H4 line discipline enhancements
>
> Hi Ilya,
>
>> This is largely enhancements implemented by Frederic Danis of Intel.
>> I've also added the ability to flow control the UART and improved the
>> UART baud rate setting some.
>
> I applied parts of Fred’s patches and so please rebase on top of them. Makes it a lot easier for me to review changes instead of figuring out what comes from you and what comes from Fred.
>
>>
>> Signed-off-by: Ilya Faenson<ifaenson@broadcom.com>
>> ---
>> drivers/bluetooth/hci_ldisc.c | 110 ++++++++++++++++++++++++++++++++++++++++--
>> drivers/bluetooth/hci_uart.h  |   6 +++
>> 2 files changed, 113 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
>> index 5c9a73f..58dcb24 100644
>> --- a/drivers/bluetooth/hci_ldisc.c
>> +++ b/drivers/bluetooth/hci_ldisc.c
>> @@ -1,4 +1,4 @@
>> -/*
>> +/*_
>
> Funky change ;)
>
>>   *
>>   *  Bluetooth HCI UART driver
>>   *
>> @@ -256,7 +256,8 @@ static int hci_uart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
>> 	if (!test_bit(HCI_RUNNING,&hdev->flags))
>> 		return -EBUSY;
>>
>> -	BT_DBG("%s: type %d len %d", hdev->name, bt_cb(skb)->pkt_type, skb->len);
>> +	BT_DBG("%s: type %d len %d", hdev->name, bt_cb(skb)->pkt_type,
>> +	       skb->len);
>
> In case you spot coding style errors, please just send them as tiny cleanup patches. I can easily apply them out of order and we get of this. Makes review a lot easier.
>
>>
>> 	hu->proto->enqueue(hu, skb);
>>
>> @@ -265,11 +266,114 @@ static int hci_uart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
>> 	return 0;
>> }
>>
>> +void hci_uart_flow_control_device(struct hci_uart *hu)
>> +{
>> +	struct tty_struct *tty = hu->tty;
>> +	struct ktermios ktermios;
>> +	int status;
>> +	unsigned int set = 0;
>> +	unsigned int clear = 0;
>> +
>> +	/* Disable hardware flow control */
>> +	ktermios = tty->termios;
>> +	ktermios.c_cflag&= ~CRTSCTS;
>> +	status = tty_set_termios(tty,&ktermios);
>> +	if (status)
>> +		BT_DBG("%s dis fc failure %d", __func__, status);
>> +	else
>> +		BT_DBG("%s hw fc disabled", __func__);
>
> Lets do it like this:
>
> 	BT_DBG(“Disabling hardware flow control: %s”, status ? “failed” : “success”);
>
>> +
>> +	/* Clear RTS to prevent the device from sending */
>> +	/* (most PCs need OUT2 to enable interrupts)    */
>
> Please use the network subsystem comment style.
>
>> +	status = tty->driver->ops->tiocmget(tty);
>> +	BT_DBG("%s cur tiocm 0x%x", __func__, status);
>
> 	Spell current out here and get rid of __func__
>
> I would also add an extra empty line here to make it a bit more readable.
>
>> +	set&= ~(TIOCM_OUT2 | TIOCM_RTS);
>> +	clear = ~set;
>> +	set&= TIOCM_DTR | TIOCM_RTS | TIOCM_OUT1 |
>> +		TIOCM_OUT2 | TIOCM_LOOP;
>> +	clear&= TIOCM_DTR | TIOCM_RTS | TIOCM_OUT1 |
>> +		TIOCM_OUT2 | TIOCM_LOOP;
>
> You need to align the second lines correctly.
>
>> +	status = tty->driver->ops->tiocmset(tty, set, clear);
>> +	if (status)
>> +		BT_DBG("%s clr RTS fail %d", __func__, status);
>> +	else
>> +		BT_DBG("%s RTS cleared", __func__);
>
> Similar change as above.
>
>> +	status = tty->driver->ops->tiocmget(tty);
>
> Why is this needed?
>
>> +}
>> +
>> +void hci_uart_unflow_control_device(struct hci_uart *hu)
>> +{
>> +	struct tty_struct *tty = hu->tty;
>> +	struct ktermios ktermios;
>> +	int status;
>> +	unsigned int set = 0;
>> +	unsigned int clear = 0;
>> +
>> +	status = tty->driver->ops->tiocmget(tty);
>> +	BT_DBG("%s cur tiocm 0x%x", __func__, status);
>> +	set |= (TIOCM_OUT2 | TIOCM_RTS);
>> +	clear = ~set;
>> +	set&= TIOCM_DTR | TIOCM_RTS | TIOCM_OUT1 |
>> +		TIOCM_OUT2 | TIOCM_LOOP;
>> +	clear&= TIOCM_DTR | TIOCM_RTS | TIOCM_OUT1 |
>> +		TIOCM_OUT2 | TIOCM_LOOP;
>> +	status = tty->driver->ops->tiocmset(tty, set, clear);
>> +	if (status)
>> +		BT_DBG("%s set RTS fail %d", __func__, status);
>> +	else
>> +		BT_DBG("%s RTS set", __func__);
>> +
>> +	/* Re-enable hardware flow control */
>> +	ktermios = tty->termios;
>> +	ktermios.c_cflag |= CRTSCTS;
>> +	status = tty_set_termios(tty,&ktermios);
>> +	if (status)
>> +		BT_DBG("%s enable fc fail %d", __func__, status);
>> +	else
>> +		BT_DBG("%s hw fc re-enabled", __func__);
>> +}
>
> Pretty much same modifications as for the other function.
>
>> +
>> +void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed)
>> +{
>> +	struct tty_struct *tty = hu->tty;
>> +	struct ktermios ktermios;
>> +
>> +	/* Bring the UART into a known state with a given baud rate */
>> +	ktermios = tty->termios;
>> +	ktermios.c_cflag&= ~CBAUD;
>> +	ktermios.c_iflag&= ~(IGNBRK | BRKINT | PARMRK | ISTRIP
>> +		| INLCR | IGNCR | ICRNL | IXON);
>
> Fix the alignment here.
>
>> +	ktermios.c_oflag&= ~OPOST;
>> +	ktermios.c_lflag&= ~(ECHO | ECHONL | ICANON | ISIG | IEXTEN);
>> +	ktermios.c_cflag&= ~(CSIZE | PARENB | CBAUD);
>> +	ktermios.c_cflag |= CS8;
>> +	ktermios.c_cflag |= CRTSCTS;
>> +	/* ktermios.c_cflag |= BOTHER; */
>
> Don’t like uncommented code. Better to just remove it.
>
>> +	tty_termios_encode_baud_rate(&ktermios, speed, speed);
>> +
>> +	/* tty_set_termios() return not checked as it is always 0 */
>> +	tty_set_termios(tty,&ktermios);
>> +
>> +	BT_DBG("%s: New tty speeds: %d/%d, cflag: 0x%x", hu->hdev->name,
>> +	       tty->termios.c_ispeed, tty->termios.c_ospeed,
>> +	       tty->termios.c_cflag);
>> +}
>> +
>> static int hci_uart_setup(struct hci_dev *hdev)
>> {
>> 	struct hci_uart *hu = hci_get_drvdata(hdev);
>> 	struct hci_rp_read_local_version *ver;
>> 	struct sk_buff *skb;
>> +	int err;
>> +
>> +	if (hu->proto->init_speed)
>> +		hci_uart_set_baudrate(hu, hu->proto->init_speed);
>> +
>> +	if (hu->proto->set_baudrate&&  hu->proto->oper_speed) {
>> +		err = hu->proto->set_baudrate(hu, hu->proto->oper_speed);
>> +		if (!err)
>> +			hci_uart_set_baudrate(hu, hu->proto->oper_speed);
>> +	}
>>
>> 	if (hu->proto->setup)
>> 		return hu->proto->setup(hu);
>> @@ -647,7 +751,7 @@ static int __init hci_uart_init(void)
>>
>> 	/* Register the tty discipline */
>>
>> -	memset(&hci_uart_ldisc, 0, sizeof (hci_uart_ldisc));
>> +	memset(&hci_uart_ldisc, 0, sizeof(hci_uart_ldisc));
>
> If this is wrong in existing code, please send a cleanup patch for it. Easy to apply.
>
>> 	hci_uart_ldisc.magic		= TTY_LDISC_MAGIC;
>> 	hci_uart_ldisc.name		= "n_hci";
>> 	hci_uart_ldisc.open		= hci_uart_tty_open;
>> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
>> index 72120a5..2271cc0 100644
>> --- a/drivers/bluetooth/hci_uart.h
>> +++ b/drivers/bluetooth/hci_uart.h
>> @@ -58,10 +58,13 @@ struct hci_uart;
>> struct hci_uart_proto {
>> 	unsigned int id;
>> 	const char *name;
>> +	unsigned int init_speed;
>> +	unsigned int oper_speed;
>> 	int (*open)(struct hci_uart *hu);
>> 	int (*close)(struct hci_uart *hu);
>> 	int (*flush)(struct hci_uart *hu);
>> 	int (*setup)(struct hci_uart *hu);
>> +	int (*set_baudrate)(struct hci_uart *hu, unsigned int speed);
>> 	int (*recv)(struct hci_uart *hu, const void *data, int len);
>> 	int (*enqueue)(struct hci_uart *hu, struct sk_buff *skb);
>> 	struct sk_buff *(*dequeue)(struct hci_uart *hu);
>> @@ -96,6 +99,9 @@ int hci_uart_register_proto(const struct hci_uart_proto *p);
>> int hci_uart_unregister_proto(const struct hci_uart_proto *p);
>> int hci_uart_tx_wakeup(struct hci_uart *hu);
>> int hci_uart_init_ready(struct hci_uart *hu);
>> +void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed);
>> +void hci_uart_flow_control_device(struct hci_uart *hu);
>> +void hci_uart_unflow_control_device(struct hci_uart *hu);
>
> I actually dislike this kind of naming. What about simple stuff like:
>
> 	hci_uart_enable_device()
> 	hci_uart_disable_device()
>
> Or something with a boolean for the state:
>
> 	hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
>
> Regards
>
> Marcel
>