Hi.
This series enable the PPS reporting for USB serial devices (second
submission).
As opposed to the previous one this patch doesn't change the
usb_serial_handle_dcd_change interface, and only add the minimum of
code in order to enable the PPS over USB serial devices. It also add
some comments in the PPS documentation to warn about the unprecision
of this solution.
Patch 01 : call dcd_change handler in the ftdi driver.
Patch 02 : add the callback to the line discipline dcd_change handler.
Patch 03 : this patch is optional. While reading the code i've found
that wake_up_interruptible was often called after status processing
(ch341, 8250, ...). So i suggest to move this one.
Thank for your comments.
Regards.
Paul.
Paul Chavent (3):
USB : serial : call handle_dcd_change in ftdi driver.
USB : serial : invoke dcd_change ldisc's handler.
USB : serial : pl2303 wake up after dcd status check.
Documentation/pps/pps.txt | 15 +++++++++++++++
drivers/usb/serial/ftdi_sio.c | 8 +++++++-
drivers/usb/serial/generic.c | 9 +++++++++
drivers/usb/serial/pl2303.c | 17 +++++++++--------
4 files changed, 40 insertions(+), 9 deletions(-)
--
1.7.12.1
Seems to be done this way in other drivers (ch341, 8250, ...).
And get tty reference only if dcd_change need to be called.
Signed-off-by: Paul Chavent <[email protected]>
---
drivers/usb/serial/pl2303.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
index e7a84f0..8b81188 100644
--- a/drivers/usb/serial/pl2303.c
+++ b/drivers/usb/serial/pl2303.c
@@ -854,15 +854,16 @@ static void pl2303_update_line_status(struct usb_serial_port *port,
spin_unlock_irqrestore(&priv->lock, flags);
if (priv->line_status & UART_BREAK_ERROR)
usb_serial_handle_break(port);
- wake_up_interruptible(&port->port.delta_msr_wait);
- tty = tty_port_tty_get(&port->port);
- if (!tty)
- return;
- if ((priv->line_status ^ prev_line_status) & UART_DCD)
- usb_serial_handle_dcd_change(port, tty,
- priv->line_status & UART_DCD);
- tty_kref_put(tty);
+ if ((priv->line_status ^ prev_line_status) & UART_DCD) {
+ tty = tty_port_tty_get(&port->port);
+ if (tty)
+ usb_serial_handle_dcd_change(port, tty,
+ priv->line_status & UART_DCD);
+ tty_kref_put(tty);
+ }
+
+ wake_up_interruptible(&port->port.delta_msr_wait);
}
static void pl2303_read_int_callback(struct urb *urb)
--
1.7.12.1
Signed-off-by: Paul Chavent <[email protected]>
---
Documentation/pps/pps.txt | 15 +++++++++++++++
drivers/usb/serial/generic.c | 9 +++++++++
2 files changed, 24 insertions(+)
diff --git a/Documentation/pps/pps.txt b/Documentation/pps/pps.txt
index d35dcdd..67b9a94 100644
--- a/Documentation/pps/pps.txt
+++ b/Documentation/pps/pps.txt
@@ -66,6 +66,21 @@ In LinuxPPS the PPS sources are simply char devices usually mapped
into files /dev/pps0, /dev/pps1, etc..
+PPS with USB to serial devices
+------------------------------
+
+It is possible to grab the PPS from an USB to serial device. However,
+you should take into account the latencies and jitter introduced by
+the USB stack. Users has reported clock instability around +-1ms when
+synchronized with PPS through USB. This isn't suited for time server
+synchronisation.
+
+If your device doesn't report PPS, you can check that the feature is
+supported by its driver. Most of the time, you only need to add a call
+to usb_serial_handle_dcd_change after checking the DCD status (see
+ch341 and pl2303 examples).
+
+
Coding example
--------------
diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
index 1f31e6b..877d6e0 100644
--- a/drivers/usb/serial/generic.c
+++ b/drivers/usb/serial/generic.c
@@ -568,6 +568,15 @@ void usb_serial_handle_dcd_change(struct usb_serial_port *usb_port,
{
struct tty_port *port = &usb_port->port;
+ if (tty) {
+ struct tty_ldisc *ld = tty_ldisc_ref(tty);
+ if (ld) {
+ if (ld->ops->dcd_change)
+ ld->ops->dcd_change(tty, status);
+ tty_ldisc_deref(ld);
+ }
+ }
+
dev_dbg(&usb_port->dev, "%s - status %d\n", __func__, status);
if (status)
--
1.7.12.1
Signed-off-by: Paul Chavent <[email protected]>
---
drivers/usb/serial/ftdi_sio.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index c45f9c0..df66495 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1966,8 +1966,14 @@ static int ftdi_process_packet(struct usb_serial_port *port,
port->icount.dsr++;
if (diff_status & FTDI_RS0_RI)
port->icount.rng++;
- if (diff_status & FTDI_RS0_RLSD)
+ if (diff_status & FTDI_RS0_RLSD) {
port->icount.dcd++;
+ struct tty_struct *tty = tty_port_tty_get(&port->port);
+ if (tty)
+ usb_serial_handle_dcd_change(port, tty,
+ status & FTDI_RS0_RLSD);
+ tty_kref_put(tty);
+ }
wake_up_interruptible(&port->port.delta_msr_wait);
priv->prev_status = status;
--
1.7.12.1
On Fri, Sep 13, 2013 at 05:35:11PM +0200, Paul Chavent wrote:
> Signed-off-by: Paul Chavent <[email protected]>
Why? We need more information in the changelog body to be able to
accept this.
On Fri, Sep 13, 2013 at 05:35:12PM +0200, Paul Chavent wrote:
> Signed-off-by: Paul Chavent <[email protected]>
Again, why? I need a whole lot more information in here to be able to
accept this.
Yes, you provided much of this in the 0/3 message, but that doesn't end
up in the kernel.
Third time's a charm?
thanks,
greg k-h
On Fri, Sep 13, 2013 at 05:35:11PM +0200, Paul Chavent wrote:
> Signed-off-by: Paul Chavent <[email protected]>
> ---
> drivers/usb/serial/ftdi_sio.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index c45f9c0..df66495 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -1966,8 +1966,14 @@ static int ftdi_process_packet(struct usb_serial_port *port,
> port->icount.dsr++;
> if (diff_status & FTDI_RS0_RI)
> port->icount.rng++;
> - if (diff_status & FTDI_RS0_RLSD)
> + if (diff_status & FTDI_RS0_RLSD) {
> port->icount.dcd++;
> + struct tty_struct *tty = tty_port_tty_get(&port->port);
Please move the tty_struct definition to the top of the function (or at
least to the top of the block).
Thanks,
Johan
On Fri, Sep 13, 2013 at 05:35:12PM +0200, Paul Chavent wrote:
> Signed-off-by: Paul Chavent <[email protected]>
> ---
> Documentation/pps/pps.txt | 15 +++++++++++++++
> drivers/usb/serial/generic.c | 9 +++++++++
> 2 files changed, 24 insertions(+)
>
> diff --git a/Documentation/pps/pps.txt b/Documentation/pps/pps.txt
> index d35dcdd..67b9a94 100644
> --- a/Documentation/pps/pps.txt
> +++ b/Documentation/pps/pps.txt
> @@ -66,6 +66,21 @@ In LinuxPPS the PPS sources are simply char devices usually mapped
> into files /dev/pps0, /dev/pps1, etc..
>
>
> +PPS with USB to serial devices
> +------------------------------
> +
> +It is possible to grab the PPS from an USB to serial device. However,
> +you should take into account the latencies and jitter introduced by
> +the USB stack. Users has reported clock instability around +-1ms when
> +synchronized with PPS through USB. This isn't suited for time server
> +synchronisation.
> +
> +If your device doesn't report PPS, you can check that the feature is
> +supported by its driver. Most of the time, you only need to add a call
> +to usb_serial_handle_dcd_change after checking the DCD status (see
> +ch341 and pl2303 examples).
> +
> +
> Coding example
> --------------
>
> diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
> index 1f31e6b..877d6e0 100644
> --- a/drivers/usb/serial/generic.c
> +++ b/drivers/usb/serial/generic.c
> @@ -568,6 +568,15 @@ void usb_serial_handle_dcd_change(struct usb_serial_port *usb_port,
> {
> struct tty_port *port = &usb_port->port;
>
> + if (tty) {
> + struct tty_ldisc *ld = tty_ldisc_ref(tty);
> + if (ld) {
> + if (ld->ops->dcd_change)
> + ld->ops->dcd_change(tty, status);
> + tty_ldisc_deref(ld);
> + }
> + }
> +
> dev_dbg(&usb_port->dev, "%s - status %d\n", __func__, status);
Please add your ldisc handling after the debug statement.
Thanks,
Johan
On Fri, Sep 13, 2013 at 05:35:13PM +0200, Paul Chavent wrote:
> Seems to be done this way in other drivers (ch341, 8250, ...).
> And get tty reference only if dcd_change need to be called.
This is fine. I have a patch here doing the same two changes as part of
a larger clean-up of the pl2303 interrupt handling (which in turn is
part of the MSR-refactoring I mentioned). I could rebase on top of this,
unless you care to wait another week. :)
Thanks,
Johan
> Signed-off-by: Paul Chavent <[email protected]>
> ---
> drivers/usb/serial/pl2303.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
> index e7a84f0..8b81188 100644
> --- a/drivers/usb/serial/pl2303.c
> +++ b/drivers/usb/serial/pl2303.c
> @@ -854,15 +854,16 @@ static void pl2303_update_line_status(struct usb_serial_port *port,
> spin_unlock_irqrestore(&priv->lock, flags);
> if (priv->line_status & UART_BREAK_ERROR)
> usb_serial_handle_break(port);
> - wake_up_interruptible(&port->port.delta_msr_wait);
>
> - tty = tty_port_tty_get(&port->port);
> - if (!tty)
> - return;
> - if ((priv->line_status ^ prev_line_status) & UART_DCD)
> - usb_serial_handle_dcd_change(port, tty,
> - priv->line_status & UART_DCD);
> - tty_kref_put(tty);
> + if ((priv->line_status ^ prev_line_status) & UART_DCD) {
> + tty = tty_port_tty_get(&port->port);
> + if (tty)
> + usb_serial_handle_dcd_change(port, tty,
> + priv->line_status & UART_DCD);
> + tty_kref_put(tty);
> + }
> +
> + wake_up_interruptible(&port->port.delta_msr_wait);
> }
>
> static void pl2303_read_int_callback(struct urb *urb)
> --
> 1.7.12.1
>
Hi.
This patch was just a suggestion. Don't bother with rebasing. I will
wait your changes and remove this patch from the next submission.
Cheers.
Paul.
On 09/13/2013 06:23 PM, Johan Hovold wrote:
> On Fri, Sep 13, 2013 at 05:35:13PM +0200, Paul Chavent wrote:
>> Seems to be done this way in other drivers (ch341, 8250, ...).
>> And get tty reference only if dcd_change need to be called.
>
> This is fine. I have a patch here doing the same two changes as part of
> a larger clean-up of the pl2303 interrupt handling (which in turn is
> part of the MSR-refactoring I mentioned). I could rebase on top of this,
> unless you care to wait another week. :)
>
> Thanks,
> Johan
>
>> Signed-off-by: Paul Chavent <[email protected]>
>> ---
>> drivers/usb/serial/pl2303.c | 17 +++++++++--------
>> 1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/usb/serial/pl2303.c b/drivers/usb/serial/pl2303.c
>> index e7a84f0..8b81188 100644
>> --- a/drivers/usb/serial/pl2303.c
>> +++ b/drivers/usb/serial/pl2303.c
>> @@ -854,15 +854,16 @@ static void pl2303_update_line_status(struct usb_serial_port *port,
>> spin_unlock_irqrestore(&priv->lock, flags);
>> if (priv->line_status & UART_BREAK_ERROR)
>> usb_serial_handle_break(port);
>> - wake_up_interruptible(&port->port.delta_msr_wait);
>>
>> - tty = tty_port_tty_get(&port->port);
>> - if (!tty)
>> - return;
>> - if ((priv->line_status ^ prev_line_status) & UART_DCD)
>> - usb_serial_handle_dcd_change(port, tty,
>> - priv->line_status & UART_DCD);
>> - tty_kref_put(tty);
>> + if ((priv->line_status ^ prev_line_status) & UART_DCD) {
>> + tty = tty_port_tty_get(&port->port);
>> + if (tty)
>> + usb_serial_handle_dcd_change(port, tty,
>> + priv->line_status & UART_DCD);
>> + tty_kref_put(tty);
>> + }
>> +
>> + wake_up_interruptible(&port->port.delta_msr_wait);
>> }
>>
>> static void pl2303_read_int_callback(struct urb *urb)
>> --
>> 1.7.12.1
>>
>
On Fri, Sep 13, 2013 at 05:35:12PM +0200, Paul Chavent wrote:
> Signed-off-by: Paul Chavent <[email protected]>
Acked-by: Rodolfo Giometti <[email protected]>
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
Hello.
On 09/13/2013 07:35 PM, Paul Chavent wrote:
> Signed-off-by: Paul Chavent <[email protected]>
> ---
> drivers/usb/serial/ftdi_sio.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index c45f9c0..df66495 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -1966,8 +1966,14 @@ static int ftdi_process_packet(struct usb_serial_port *port,
> port->icount.dsr++;
> if (diff_status & FTDI_RS0_RI)
> port->icount.rng++;
> - if (diff_status & FTDI_RS0_RLSD)
> + if (diff_status & FTDI_RS0_RLSD) {
> port->icount.dcd++;
> + struct tty_struct *tty = tty_port_tty_get(&port->port);
Don't mix declaration and code. And add an empty line between them please.
> + if (tty)
> + usb_serial_handle_dcd_change(port, tty,
> + status & FTDI_RS0_RLSD);
> + tty_kref_put(tty);
> + }
WBR, Sergei
Hello.
On 09/13/2013 07:35 PM, Paul Chavent wrote:
> Signed-off-by: Paul Chavent <[email protected]>
> ---
> Documentation/pps/pps.txt | 15 +++++++++++++++
> drivers/usb/serial/generic.c | 9 +++++++++
> 2 files changed, 24 insertions(+)
> diff --git a/Documentation/pps/pps.txt b/Documentation/pps/pps.txt
> index d35dcdd..67b9a94 100644
> --- a/Documentation/pps/pps.txt
> +++ b/Documentation/pps/pps.txt
> @@ -66,6 +66,21 @@ In LinuxPPS the PPS sources are simply char devices usually mapped
> into files /dev/pps0, /dev/pps1, etc..
>
>
> +PPS with USB to serial devices
> +------------------------------
> +
> +It is possible to grab the PPS from an USB to serial device. However,
> +you should take into account the latencies and jitter introduced by
> +the USB stack. Users has reported clock instability around +-1ms when
> +synchronized with PPS through USB. This isn't suited for time server
> +synchronisation.
Thunderbird spellchecker underlines this word -- "synchronization" is
probably better.
[...]
> diff --git a/drivers/usb/serial/generic.c b/drivers/usb/serial/generic.c
> index 1f31e6b..877d6e0 100644
> --- a/drivers/usb/serial/generic.c
> +++ b/drivers/usb/serial/generic.c
> @@ -568,6 +568,15 @@ void usb_serial_handle_dcd_change(struct usb_serial_port *usb_port,
> {
> struct tty_port *port = &usb_port->port;
>
> + if (tty) {
> + struct tty_ldisc *ld = tty_ldisc_ref(tty);
Insert an empty line after declaration as above -- keep the style
consistent, please.
> + if (ld) {
> + if (ld->ops->dcd_change)
> + ld->ops->dcd_change(tty, status);
> + tty_ldisc_deref(ld);
> + }
> + }
> +
WBR, Sergei