Blackfin Serial Driver: Enable IR function when user application (irattach /dev/ttyBFx -s) call TIOCSETD ioctl with line discipline N_IRDA

O> Blackfin Serial Driver: Enable IR function when user application (irattach /dev/ttyBFx -s) call TIOCSETD ioctl with line discipline N_IRDA

Hi, Alan,

Thanks for your suggestions!

Have the tty patches that you mentioned been checked in? Would you please give a link about "the tty patches as they move the tty driver operations out of the tty driver ..."?

I made and tested this patch on 2.6.24.4 kernel, it works, although is ugly.
Blackfin serial driver register as a UART driver, which will call tty_set_operations(...,&uart_ops), while uart_ops hasn't set_ldisc function. Do you mean I should declare a tty_ops and call tty_set_operations(..., &tty_ops) in bfin_5xx.c to register my set_ldisc?

BRs,
Graf
-----Original Message-----
From: Alan Cox [mailto:[email protected]]
Sent: Fri 4/25/2008 4:50 AM
To: Linux Kernel Mailing List
Cc: [email protected]
Subject: Re: Blackfin Serial Driver: Enable IR function when user application (irattach /dev/ttyBFx -s) call TIOCSETD ioctl with line discipline N_IRDA

O> Blackfin Serial Driver: Enable IR function when user application (irattach /dev/ttyBFx -s) call TIOCSETD ioctl with line discipline N_IRDA

Then we need to add a set_ldisc to uart_ops. I'll do that in todays hacking.

Alan

Oh, I got the compile failure now:
--
CC drivers/serial/bfin_5xx.o
drivers/serial/bfin_5xx.c: In function 'bfin_serial_init':
drivers/serial/bfin_5xx.c:1281: error: 'struct tty_driver' has no member named 'set_ldisc'
make[2]: *** [drivers/serial/bfin_5xx.o] Error 1 make[1]: *** [drivers/serial] Error 2
make: *** [drivers] Error 2
--

Obviously, it is no way to change code like this for passing compiling:

-             bfin_serial_reg.tty_driver->set_ldisc = bfin_set_ldisc;
+             bfin_serial_reg.tty_driver->ops->set_ldisc = bfin_set_ldisc;
--
Because the ops uart_ops is const and does not include the set_ldisc.

IMO, adding set_ldisc to uart_ops is useless for Graf's IrDA request,
because this function is specific for Blackfin, not shared by other
UART drivers. Maybe the only way is to create a new tty_operations
named bfin_uart_ops based on uart_ops like below:
--
static const struct tty_operations bfin_uart_ops = {
        .open           = uart_open,
        .close          = uart_close,
        .write          = uart_write,
        .put_char       = uart_put_char,
        .flush_chars    = uart_flush_chars,
        .write_room     = uart_write_room,
        .chars_in_buffer= uart_chars_in_buffer,
        .flush_buffer   = uart_flush_buffer,
        .ioctl          = uart_ioctl,
        .throttle       = uart_throttle,
        .unthrottle     = uart_unthrottle,
        .send_xchar     = uart_send_xchar,
        .set_termios    = uart_set_termios,
        .stop           = uart_stop,
        .start          = uart_start,
        .hangup         = uart_hangup,
        .break_ctl      = uart_break_ctl,
        .wait_until_sent= uart_wait_until_sent,
#ifdef CONFIG_PROC_FS
        .read_proc      = uart_read_proc,
#endif
        .tiocmget       = uart_tiocmget,
        .tiocmset       = uart_tiocmset,
#ifdef CONFIG_CONSOLE_POLL
        .poll_init      = uart_poll_init,
        .poll_get_char  = uart_poll_get_char,
        .poll_put_char  = uart_poll_put_char,
#endif
        .set_ldisc         = bfin_set_ldisc,
};
--

But it looks like funny as copying a whole structure for just one
simple function member assignment.
Alan, do you have any other suggestion for this issue? maybe we can
remove 'const' from struct tty_operations, then we can simply set the
set_ldisc for bfin_set_ldisc on the fly.

Thanks a lot
-Bryan Wu

I've added the uart set_ldisc operator to my tree. I thought Andrew had it as well, but if not I'll push it again Tuesday (Monday is a holiday here)

Alan
--
"Alan, I'm getting a bit worried about you."
-- Linus Torvalds

Hi, Alan,

Have the set_ldisc operator been put into main tree?

As current development cycle will end in few weeks, while rc4 kernel still can't be compiled successfully because of this issue.

BRs,
Graf

It's been in for ages. The Uart level one seems to have gotten stuck. Let me poke that again
--

The tty layer provides a callback that is used when the line discipline is changed. Some hardware uses this to configure hardware specific features such as IrDA mode on serial ports. Unfortunately the serial layer does not provide this feature or pass it down to drivers.

Blackfin used to hack around this by rewriting the tty ops, but those are now properly shared and const so the hack fails. Instead provide the proper operations.

Linus: This change plus a follow up from the Blackfin guys is needed to avoid blackfin losing features in this release.

diff -u --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.26-rc2/drivers/serial/serial_core.c linux-2.6.26-rc2/drivers/serial/serial_core.c
@@ -1165,6 +1166,15 @@
 	return ret;
 }
 
+static void uart_set_ldisc(struct tty_struct *tty, int ldisc)
+{
+	struct uart_state *state = tty->driver_data;
+	struct uart_port *port = state->port;
+
+	if (port->ops->set_ldisc)
+		port->ops->set_ldisc(port);
+}
+
 static void uart_set_termios(struct tty_struct *tty,
 						struct ktermios *old_termios)
 {

@@ -2285,6 +2298,7 @@
 	.unthrottle	= uart_unthrottle,
 	.send_xchar	= uart_send_xchar,
 	.set_termios	= uart_set_termios,
+	.set_ldisc	= uart_set_ldisc,
 	.stop		= uart_stop,
 	.start		= uart_start,
 	.hangup		= uart_hangup,

diff -u --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.26-rc2/include/linux/serial_core.h linux-2.6.26-rc2/include/linux/serial_core.h
@@ -192,6 +192,7 @@
 	void		(*shutdown)(struct uart_port *);
 	void		(*set_termios)(struct uart_port *, struct ktermios *new,
 				       struct ktermios *old);
+	void		(*set_ldisc)(struct uart_port *);
 	void		(*pm)(struct uart_port *, unsigned int state,
 			      unsigned int oldstate);
 	int		(*set_wake)(struct uart_port *, unsigned int state);

Alan, I applied this as "obviously safe", since nobody sets the uart-level set_ldisc thing yet... BUT!

I now get an annoying compiler warning, and the compiler is definitely right. You have:

static void uart_set_ldisc(struct tty_struct *tty, int ldisc) ..
.set_ldisc = uart_set_ldisc,

Oops. Totally wrong function type. Because the tty-level one is

	void (*set_ldisc)(struct tty_struct *tty);

in my tree, and as a result I suspect you sent me the wrong version of a patch (perhaps based on -mm or -next).

I assume I should just remove the (unused) "int ldisc" argument, but would like to get confirmation first.

Linus

--

 drivers/serial/serial_core.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index 951a75e..c9b64e7 100644
@@ -1165,7 +1165,7 @@ out:
 	return ret;
 }
 
-static void uart_set_ldisc(struct tty_struct *tty, int ldisc)
+static void uart_set_ldisc(struct tty_struct *tty)
 {
 	struct uart_state *state = tty->driver_data;
 	struct uart_port *port = state->port;