* [PATCH] BlueZ line discipline baud rate setting update
@ 2015-06-15 16:50 Ilya Faenson
2015-06-15 16:50 ` Ilya Faenson
0 siblings, 1 reply; 13+ messages in thread
From: Ilya Faenson @ 2015-06-15 16:50 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth, Ilya Faenson
At Marcel's request, I'm providing a version of the hci_uart_set_baudrate
function that works well for me.
Ilya Faenson (1):
BlueZ line discipline baud rate setting update
drivers/bluetooth/hci_ldisc.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
--
1.9.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] BlueZ line discipline baud rate setting update
2015-06-15 16:50 [PATCH] BlueZ line discipline baud rate setting update Ilya Faenson
@ 2015-06-15 16:50 ` Ilya Faenson
2015-06-15 17:35 ` Marcel Holtmann
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Ilya Faenson @ 2015-06-15 16:50 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth, Ilya Faenson
Bring the tty into a known 8 bits, 1 start bit, 1 stop bit,
hardware flow control state with a given baud rate.
Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
---
drivers/bluetooth/hci_ldisc.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index ac87346..606cc5a 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -271,9 +271,16 @@ 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_cflag |= BOTHER;
+ ktermios.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP | INLCR |
+ IGNCR | ICRNL | IXON);
+ 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;
tty_termios_encode_baud_rate(&ktermios, speed, speed);
/* tty_set_termios() return not checked as it is always 0 */
--
1.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] BlueZ line discipline baud rate setting update
2015-06-15 16:50 ` Ilya Faenson
@ 2015-06-15 17:35 ` Marcel Holtmann
2015-06-16 8:43 ` Frederic Danis
2015-06-15 21:40 ` Marcel Holtmann
2015-06-17 13:16 ` Marcel Holtmann
2 siblings, 1 reply; 13+ messages in thread
From: Marcel Holtmann @ 2015-06-15 17:35 UTC (permalink / raw)
To: Ilya Faenson; +Cc: linux-bluetooth
Hi Ilya,
> Bring the tty into a known 8 bits, 1 start bit, 1 stop bit,
> hardware flow control state with a given baud rate.
>
> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
> ---
> drivers/bluetooth/hci_ldisc.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index ac87346..606cc5a 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -271,9 +271,16 @@ 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_cflag |= BOTHER;
> + ktermios.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP | INLCR |
> + IGNCR | ICRNL | IXON);
I think this one needs to align like this:
&= ~(IGNBRK | .. |
IGNCR | ..);
However I can fix that one easily inline. So no worries.
Fred, can you test this patch so I can add a Tested-by line.
Regards
Marcel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] BlueZ line discipline baud rate setting update
2015-06-15 16:50 ` Ilya Faenson
2015-06-15 17:35 ` Marcel Holtmann
@ 2015-06-15 21:40 ` Marcel Holtmann
2015-06-15 21:45 ` Ilya Faenson
2015-06-17 13:16 ` Marcel Holtmann
2 siblings, 1 reply; 13+ messages in thread
From: Marcel Holtmann @ 2015-06-15 21:40 UTC (permalink / raw)
To: Ilya Faenson; +Cc: linux-bluetooth
Hi Ilya,
> Bring the tty into a known 8 bits, 1 start bit, 1 stop bit,
> hardware flow control state with a given baud rate.
>
> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
> ---
> drivers/bluetooth/hci_ldisc.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index ac87346..606cc5a 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -271,9 +271,16 @@ 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_cflag |= BOTHER;
> + ktermios.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP | INLCR |
> + IGNCR | ICRNL | IXON);
> + ktermios.c_oflag &= ~OPOST;
> + ktermios.c_lflag &= ~(ECHO | ECHONL | ICANON | ISIG | IEXTEN);
> + ktermios.c_cflag &= ~(CSIZE | PARENB | CBAUD);
I just realize that we are clearing CBAUD twice here. I think the one above BOTHER should also be removed.
> + ktermios.c_cflag |= CS8;
> + ktermios.c_cflag |= CRTSCTS;
> tty_termios_encode_baud_rate(&ktermios, speed, speed);
Regards
Marcel
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] BlueZ line discipline baud rate setting update
2015-06-15 21:40 ` Marcel Holtmann
@ 2015-06-15 21:45 ` Ilya Faenson
0 siblings, 0 replies; 13+ messages in thread
From: Ilya Faenson @ 2015-06-15 21:45 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth@vger.kernel.org
Hi Marcel,
-----Original Message-----
From: Marcel Holtmann [mailto:marcel@holtmann.org]=20
Sent: Monday, June 15, 2015 5:41 PM
To: Ilya Faenson
Cc: linux-bluetooth@vger.kernel.org
Subject: Re: [PATCH] BlueZ line discipline baud rate setting update
Hi Ilya,
> Bring the tty into a known 8 bits, 1 start bit, 1 stop bit,
> hardware flow control state with a given baud rate.
>=20
> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
> ---
> drivers/bluetooth/hci_ldisc.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>=20
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.=
c
> index ac87346..606cc5a 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -271,9 +271,16 @@ void hci_uart_set_baudrate(struct hci_uart *hu, unsi=
gned int speed)
> struct tty_struct *tty =3D hu->tty;
> struct ktermios ktermios;
>=20
> + /* Bring the UART into a known state with a given baud rate */
> ktermios =3D tty->termios;
> ktermios.c_cflag &=3D ~CBAUD;
> - ktermios.c_cflag |=3D BOTHER;
> + ktermios.c_iflag &=3D ~(IGNBRK | BRKINT | PARMRK | ISTRIP | INLCR |
> + IGNCR | ICRNL | IXON);
> + ktermios.c_oflag &=3D ~OPOST;
> + ktermios.c_lflag &=3D ~(ECHO | ECHONL | ICANON | ISIG | IEXTEN);
> + ktermios.c_cflag &=3D ~(CSIZE | PARENB | CBAUD);
I just realize that we are clearing CBAUD twice here. I think the one above=
BOTHER should also be removed.
IF: Agreed, good catch.
> + ktermios.c_cflag |=3D CS8;
> + ktermios.c_cflag |=3D CRTSCTS;
> tty_termios_encode_baud_rate(&ktermios, speed, speed);
Regards
Marcel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] BlueZ line discipline baud rate setting update
2015-06-15 17:35 ` Marcel Holtmann
@ 2015-06-16 8:43 ` Frederic Danis
2015-06-16 12:19 ` Ilya Faenson
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Frederic Danis @ 2015-06-16 8:43 UTC (permalink / raw)
To: Marcel Holtmann, Ilya Faenson; +Cc: linux-bluetooth
Hello Marcel and Ilya,
On 15/06/2015 19:35, Marcel Holtmann wrote:
> Hi Ilya,
>
>> Bring the tty into a known 8 bits, 1 start bit, 1 stop bit,
>> hardware flow control state with a given baud rate.
>>
>> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
>> ---
>> drivers/bluetooth/hci_ldisc.c | 9 ++++++++-
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
>> index ac87346..606cc5a 100644
>> --- a/drivers/bluetooth/hci_ldisc.c
>> +++ b/drivers/bluetooth/hci_ldisc.c
>> @@ -271,9 +271,16 @@ 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_cflag |= BOTHER;
>> + ktermios.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP | INLCR |
>> + IGNCR | ICRNL | IXON);
>
> I think this one needs to align like this:
>
> &= ~(IGNBRK | .. |
> IGNCR | ..);
>
> However I can fix that one easily inline. So no worries.
>
> Fred, can you test this patch so I can add a Tested-by line.
OK, I will test it.
Btw, I thought that this function should only change the UART speed.
For other UART parameters, I thought they should be set by btattach,
ACPI or DT part.
Regards
Fred
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] BlueZ line discipline baud rate setting update
2015-06-16 8:43 ` Frederic Danis
@ 2015-06-16 12:19 ` Ilya Faenson
2015-06-16 13:34 ` Frederic Danis
2015-06-16 14:36 ` Marcel Holtmann
2 siblings, 0 replies; 13+ messages in thread
From: Ilya Faenson @ 2015-06-16 12:19 UTC (permalink / raw)
To: Frederic Danis, Marcel Holtmann; +Cc: linux-bluetooth@vger.kernel.org
Hi Fred,
________________________________________
From: Frederic Danis [frederic.danis@linux.intel.com]
Sent: Tuesday, June 16, 2015 4:43 AM
To: Marcel Holtmann; Ilya Faenson
Cc: linux-bluetooth@vger.kernel.org
Subject: Re: [PATCH] BlueZ line discipline baud rate setting update
Hello Marcel and Ilya,
On 15/06/2015 19:35, Marcel Holtmann wrote:
> Hi Ilya,
>
>> Bring the tty into a known 8 bits, 1 start bit, 1 stop bit,
>> hardware flow control state with a given baud rate.
>>
>> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
>> ---
>> drivers/bluetooth/hci_ldisc.c | 9 ++++++++-
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
>> index ac87346..606cc5a 100644
>> --- a/drivers/bluetooth/hci_ldisc.c
>> +++ b/drivers/bluetooth/hci_ldisc.c
>> @@ -271,9 +271,16 @@ 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_cflag |= BOTHER;
>> + ktermios.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP | INLCR |
>> + IGNCR | ICRNL | IXON);
>
> I think this one needs to align like this:
>
> &= ~(IGNBRK | .. |
> IGNCR | ..);
>
> However I can fix that one easily inline. So no worries.
>
> Fred, can you test this patch so I can add a Tested-by line.
OK, I will test it.
Btw, I thought that this function should only change the UART speed.
For other UART parameters, I thought they should be set by btattach,
ACPI or DT part.
IF: UART parameters should be fully configured in kernel space in my opinion. I don't set any UART parameters from user space in my testing. As far as the DT/ACPI are concerned, none of the current ACPI DSL BT nodes configure UART parameters yet. I've introduced the oper-speed parameter for DT and that's the only UART parameter there for now. If we decide to add more, we'll have to change the line discipline configuration accordingly. Not sure it is needed though as all the BT UART-connected devices I've ever worked with in the last 10 years or so (including test devices connected via the FTDI-based USB boards) always worked fine with 8 bits, 1 start bit, 1 stop bit, no parity, hardware flow control.
Regards
Fred
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] BlueZ line discipline baud rate setting update
2015-06-16 8:43 ` Frederic Danis
2015-06-16 12:19 ` Ilya Faenson
@ 2015-06-16 13:34 ` Frederic Danis
2015-06-16 14:36 ` Marcel Holtmann
2 siblings, 0 replies; 13+ messages in thread
From: Frederic Danis @ 2015-06-16 13:34 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Ilya Faenson, linux-bluetooth
Hello Marcel,
On 16/06/2015 10:43, Frederic Danis wrote:
> Hello Marcel and Ilya,
>
> On 15/06/2015 19:35, Marcel Holtmann wrote:
>> Hi Ilya,
>>
>>> Bring the tty into a known 8 bits, 1 start bit, 1 stop bit,
>>> hardware flow control state with a given baud rate.
>>>
>>> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
>>> ---
>>> drivers/bluetooth/hci_ldisc.c | 9 ++++++++-
>>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/bluetooth/hci_ldisc.c
>>> b/drivers/bluetooth/hci_ldisc.c
>>> index ac87346..606cc5a 100644
>>> --- a/drivers/bluetooth/hci_ldisc.c
>>> +++ b/drivers/bluetooth/hci_ldisc.c
>>> @@ -271,9 +271,16 @@ 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_cflag |= BOTHER;
>>> + ktermios.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP | INLCR |
>>> + IGNCR | ICRNL | IXON);
>>
>> I think this one needs to align like this:
>>
>> &= ~(IGNBRK | .. |
>> IGNCR | ..);
>>
>> However I can fix that one easily inline. So no worries.
>>
>> Fred, can you test this patch so I can add a Tested-by line.
>
> OK, I will test it.
Device setup works correctly.
Regards
Fred
--
Frederic Danis Open Source Technology Center
frederic.danis@intel.com Intel Corporation
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] BlueZ line discipline baud rate setting update
2015-06-16 8:43 ` Frederic Danis
2015-06-16 12:19 ` Ilya Faenson
2015-06-16 13:34 ` Frederic Danis
@ 2015-06-16 14:36 ` Marcel Holtmann
2015-06-16 16:15 ` Frederic Danis
2 siblings, 1 reply; 13+ messages in thread
From: Marcel Holtmann @ 2015-06-16 14:36 UTC (permalink / raw)
To: Frederic Danis; +Cc: Ilya Faenson, linux-bluetooth
Hi Fred,
>>> Bring the tty into a known 8 bits, 1 start bit, 1 stop bit,
>>> hardware flow control state with a given baud rate.
>>>
>>> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
>>> ---
>>> drivers/bluetooth/hci_ldisc.c | 9 ++++++++-
>>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
>>> index ac87346..606cc5a 100644
>>> --- a/drivers/bluetooth/hci_ldisc.c
>>> +++ b/drivers/bluetooth/hci_ldisc.c
>>> @@ -271,9 +271,16 @@ 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_cflag |= BOTHER;
>>> + ktermios.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP | INLCR |
>>> + IGNCR | ICRNL | IXON);
>>
>> I think this one needs to align like this:
>>
>> &= ~(IGNBRK | .. |
>> IGNCR | ..);
>>
>> However I can fix that one easily inline. So no worries.
>>
>> Fred, can you test this patch so I can add a Tested-by line.
>
> OK, I will test it.
>
> Btw, I thought that this function should only change the UART speed.
> For other UART parameters, I thought they should be set by btattach, ACPI or DT part.
do you think we want this split out into one hci_uart_reset_termios or something and one that actually changes the baud rate. That seems fair to me as well so then the individual vendor code can pick and choose what they want to do.
Regards
Marcel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] BlueZ line discipline baud rate setting update
2015-06-16 14:36 ` Marcel Holtmann
@ 2015-06-16 16:15 ` Frederic Danis
2015-06-16 16:28 ` Marcel Holtmann
0 siblings, 1 reply; 13+ messages in thread
From: Frederic Danis @ 2015-06-16 16:15 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Ilya Faenson, linux-bluetooth
Hello Marcel,
On 16/06/2015 16:36, Marcel Holtmann wrote:
> Hi Fred,
>
> >>> Bring the tty into a known 8 bits, 1 start bit, 1 stop bit,
> >>> hardware flow control state with a given baud rate.
> >>>
> >>> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
> >>> ---
> >>> drivers/bluetooth/hci_ldisc.c | 9 ++++++++-
> >>> 1 file changed, 8 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/bluetooth/hci_ldisc.c
> b/drivers/bluetooth/hci_ldisc.c
> >>> index ac87346..606cc5a 100644
> >>> --- a/drivers/bluetooth/hci_ldisc.c
> >>> +++ b/drivers/bluetooth/hci_ldisc.c
> >>> @@ -271,9 +271,16 @@ 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_cflag |= BOTHER;
> >>> + ktermios.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP | INLCR |
> >>> + IGNCR | ICRNL | IXON);
> >>
> >> I think this one needs to align like this:
> >>
> >> &= ~(IGNBRK | .. |
> >> IGNCR | ..);
> >>
> >> However I can fix that one easily inline. So no worries.
> >>
> >> Fred, can you test this patch so I can add a Tested-by line.
> >
> > OK, I will test it.
> >
> > Btw, I thought that this function should only change the UART speed.
> > For other UART parameters, I thought they should be set by btattach,
> ACPI or DT part.
>
> do you think we want this split out into one hci_uart_reset_termios or
> something and one that actually changes the baud rate. That seems fair
> to me as well so then the individual vendor code can pick and choose
> what they want to do.
Yes.
Imo, the question is to know who will be responsible to open the serial
port and configure it:
- from user space using btattach or bluetoothd (as you wrote in a
previous mail), may be based on uevent giving those info,
- or from kernel space, which will imply to have all info from ACPI or DT
Regards
Fred
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] BlueZ line discipline baud rate setting update
2015-06-16 16:15 ` Frederic Danis
@ 2015-06-16 16:28 ` Marcel Holtmann
0 siblings, 0 replies; 13+ messages in thread
From: Marcel Holtmann @ 2015-06-16 16:28 UTC (permalink / raw)
To: Frederic Danis; +Cc: Ilya Faenson, linux-bluetooth
Hi Fred,
>> >>> Bring the tty into a known 8 bits, 1 start bit, 1 stop bit,
>> >>> hardware flow control state with a given baud rate.
>> >>>
>> >>> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
>> >>> ---
>> >>> drivers/bluetooth/hci_ldisc.c | 9 ++++++++-
>> >>> 1 file changed, 8 insertions(+), 1 deletion(-)
>> >>>
>> >>> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
>> >>> index ac87346..606cc5a 100644
>> >>> --- a/drivers/bluetooth/hci_ldisc.c
>> >>> +++ b/drivers/bluetooth/hci_ldisc.c
>> >>> @@ -271,9 +271,16 @@ 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_cflag |= BOTHER;
>> >>> + ktermios.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP | INLCR |
>> >>> + IGNCR | ICRNL | IXON);
>> >>
>> >> I think this one needs to align like this:
>> >>
>> >> &= ~(IGNBRK | .. |
>> >> IGNCR | ..);
>> >>
>> >> However I can fix that one easily inline. So no worries.
>> >>
>> >> Fred, can you test this patch so I can add a Tested-by line.
>> >
>> > OK, I will test it.
>> >
>> > Btw, I thought that this function should only change the UART speed.
>> > For other UART parameters, I thought they should be set by btattach, ACPI or DT part.
>>
>> do you think we want this split out into one hci_uart_reset_termios or something and one that actually changes the baud rate. That seems fair to me as well so then the individual vendor code can pick and choose what they want to do.
>
> Yes.
> Imo, the question is to know who will be responsible to open the serial port and configure it:
> - from user space using btattach or bluetoothd (as you wrote in a previous mail), may be based on uevent giving those info,
> - or from kernel space, which will imply to have all info from ACPI or DT
I think we need both. Some vendor drivers might rely on that userspace is opening the port correctly. Otherwise need to toggle things and actually take control.
Regards
Marcel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] BlueZ line discipline baud rate setting update
2015-06-15 16:50 ` Ilya Faenson
2015-06-15 17:35 ` Marcel Holtmann
2015-06-15 21:40 ` Marcel Holtmann
@ 2015-06-17 13:16 ` Marcel Holtmann
2015-06-17 13:26 ` Ilya Faenson
2 siblings, 1 reply; 13+ messages in thread
From: Marcel Holtmann @ 2015-06-17 13:16 UTC (permalink / raw)
To: Ilya Faenson; +Cc: linux-bluetooth
Hi Ilya,
> Bring the tty into a known 8 bits, 1 start bit, 1 stop bit,
> hardware flow control state with a given baud rate.
>
> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
> ---
> drivers/bluetooth/hci_ldisc.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index ac87346..606cc5a 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -271,9 +271,16 @@ 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_cflag |= BOTHER;
> + ktermios.c_iflag &= ~(IGNBRK | BRKINT | PARMRK | ISTRIP | INLCR |
> + IGNCR | ICRNL | IXON);
> + 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;
> tty_termios_encode_baud_rate(&ktermios, speed, speed);
with the questions raised by the others, I think for now we just want to change the baud rate and leave the rest to a more generic function. I am fine if the kernel side wants to enforce the settings on the UART. That seems fully reasonable. However we need to give drivers a chance to let it come from userspace as well and honor whatever they configured there.
So we need to clear CBAUD and then encode the baud rate. Do we need to bother with BOTHER or is that just not needed at all. I really like to get this detail fixed as soon as possible since we are close to the merge window.
Regards
Marcel
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] BlueZ line discipline baud rate setting update
2015-06-17 13:16 ` Marcel Holtmann
@ 2015-06-17 13:26 ` Ilya Faenson
0 siblings, 0 replies; 13+ messages in thread
From: Ilya Faenson @ 2015-06-17 13:26 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth@vger.kernel.org
Hi Marcel,
________________________________________
From: Marcel Holtmann [marcel@holtmann.org]
Sent: Wednesday, June 17, 2015 9:16 AM
To: Ilya Faenson
Cc: linux-bluetooth@vger.kernel.org
Subject: Re: [PATCH] BlueZ line discipline baud rate setting update
Hi Ilya,
> Bring the tty into a known 8 bits, 1 start bit, 1 stop bit,
> hardware flow control state with a given baud rate.
>
> Signed-off-by: Ilya Faenson <ifaenson@broadcom.com>
> ---
> drivers/bluetooth/hci_ldisc.c | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.=
c
> index ac87346..606cc5a 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -271,9 +271,16 @@ void hci_uart_set_baudrate(struct hci_uart *hu, unsi=
gned int speed)
> struct tty_struct *tty =3D hu->tty;
> struct ktermios ktermios;
>
> + /* Bring the UART into a known state with a given baud rate */
> ktermios =3D tty->termios;
> ktermios.c_cflag &=3D ~CBAUD;
> - ktermios.c_cflag |=3D BOTHER;
> + ktermios.c_iflag &=3D ~(IGNBRK | BRKINT | PARMRK | ISTRIP | INLCR |
> + IGNCR | ICRNL | IXON);
> + ktermios.c_oflag &=3D ~OPOST;
> + ktermios.c_lflag &=3D ~(ECHO | ECHONL | ICANON | ISIG | IEXTEN);
> + ktermios.c_cflag &=3D ~(CSIZE | PARENB | CBAUD);
> + ktermios.c_cflag |=3D CS8;
> + ktermios.c_cflag |=3D CRTSCTS;
> tty_termios_encode_baud_rate(&ktermios, speed, speed);
with the questions raised by the others, I think for now we just want to ch=
ange the baud rate and leave the rest to a more generic function. I am fine=
if the kernel side wants to enforce the settings on the UART. That seems f=
ully reasonable. However we need to give drivers a chance to let it come fr=
om userspace as well and honor whatever they configured there.
So we need to clear CBAUD and then encode the baud rate. Do we need to both=
er with BOTHER or is that just not needed at all. I really like to get this=
detail fixed as soon as possible since we are close to the merge window.
IF: The BOTHER is not needed. Okay: if this function deals with the baud ra=
te only, I will then introduce a separate function to bring the UART into a=
reasonable default state for Bluetooth as part of the next Broadcom patch =
set. I'll try publishing them today if my other duties do not interfere.
Regards
Marcel=
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2015-06-17 13:26 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-15 16:50 [PATCH] BlueZ line discipline baud rate setting update Ilya Faenson
2015-06-15 16:50 ` Ilya Faenson
2015-06-15 17:35 ` Marcel Holtmann
2015-06-16 8:43 ` Frederic Danis
2015-06-16 12:19 ` Ilya Faenson
2015-06-16 13:34 ` Frederic Danis
2015-06-16 14:36 ` Marcel Holtmann
2015-06-16 16:15 ` Frederic Danis
2015-06-16 16:28 ` Marcel Holtmann
2015-06-15 21:40 ` Marcel Holtmann
2015-06-15 21:45 ` Ilya Faenson
2015-06-17 13:16 ` Marcel Holtmann
2015-06-17 13:26 ` Ilya Faenson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.