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

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

[Alan Cox]

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

>  	if (ret == 0) {
> +		bfin_serial_reg.tty_driver->set_ldisc = bfin_set_ldisc;
>  		ret = platform_driver_register(&bfin_serial_driver);

This will not work when combined with the tty patches as they move the
tty driver operations out of the tty driver and also make them shared and
const. You need to declare two sets of tty_ops and register the correct
one for your device.

Alan

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

[Yang, Graf]

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:alan@lxorguk.ukuu.org.uk]
Sent: Fri 4/25/2008 4:50 AM
To: Linux Kernel Mailing List
Cc: graf.yang@analog.com
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

>  	if (ret == 0) {
> +		bfin_serial_reg.tty_driver->set_ldisc = bfin_set_ldisc;
>  		ret = platform_driver_register(&bfin_serial_driver);

This will not work when combined with the tty patches as they move the
tty driver operations out of the tty driver and also make them shared and
const. You need to declare two sets of tty_ops and register the correct
one for your device.

Alan

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

[Alan Cox]

> 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?

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

Alan

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

[Bryan Wu]

On Fri, Apr 25, 2008 at 5:55 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> > 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?
>
>  Then we need to add a set_ldisc to uart_ops. I'll do that in todays
>  hacking.
>

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

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

[Alan Cox]

> 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:

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

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

[gyang]

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


在 2008-05-04日的 16:56 +0100，Alan Cox写道：
> > 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:
> 
> 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

[PATCH] serial_core: uart_set_ldisc (Was Re: Blackfin Serial Driver: Enable IR function when user application (irattach /dev/ttyBFx -s) call TIOCSETD ioctl with line discipline N_IRDA)

[Alan Cox]

On Tue, 03 Jun 2008 10:18:46 +0800
gyang <graf.yang@analog.com> wrote:

> Hi, Alan,
> 
> Have the set_ldisc operator been put into main tree?

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.

Signed-off-by: Alan Cox <alan@redhat.com>

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
--- linux.vanilla-2.6.26-rc2/drivers/serial/serial_core.c	2008-05-12 12:09:06.000000000 +0100
+++ linux-2.6.26-rc2/drivers/serial/serial_core.c	2008-06-03 15:09:52.000000000 +0100
@@ -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
--- linux.vanilla-2.6.26-rc2/include/linux/serial_core.h	2008-05-12 12:09:08.000000000 +0100
+++ linux-2.6.26-rc2/include/linux/serial_core.h	2008-06-03 15:08:28.000000000 +0100
@@ -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);

Re: [PATCH] serial_core: uart_set_ldisc (Was Re: Blackfin Serial Driver: Enable IR function when user application (irattach /dev/ttyBFx -s) call TIOCSETD ioctl with line discipline N_IRDA)

[Linus Torvalds]

On Tue, 3 Jun 2008, Alan Cox wrote:
> 
> Linus: This change plus a follow up from the Blackfin guys is needed to
> avoid blackfin losing features in this release.

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(-)

diff --git a/drivers/serial/serial_core.c b/drivers/serial/serial_core.c
index 951a75e..c9b64e7 100644
--- a/drivers/serial/serial_core.c
+++ b/drivers/serial/serial_core.c
@@ -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;