Return-Path: Message-ID: <1484743377.2133.204.camel@linux.intel.com> Subject: Re: [PATCH v2 8/9] serdev: add a tty port controller driver From: Andy Shevchenko To: Rob Herring , Greg Kroah-Hartman , Marcel Holtmann , Jiri Slaby , Sebastian Reichel , Arnd Bergmann , "Dr . H . Nikolaus Schaller" , Peter Hurley , Alan Cox Cc: Loic Poulain , Pavel Machek , NeilBrown , Linus Walleij , linux-bluetooth@vger.kernel.org, linux-serial@vger.kernel.org, linux-kernel@vger.kernel.org Date: Wed, 18 Jan 2017 14:42:57 +0200 In-Reply-To: <20170116225436.17505-9-robh@kernel.org> References: <20170116225436.17505-1-robh@kernel.org> <20170116225436.17505-9-robh@kernel.org> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 List-ID: On Mon, 2017-01-16 at 16:54 -0600, Rob Herring wrote: > Add a serdev controller driver for tty ports. > > The controller is registered with serdev when tty ports are registered > with the TTY core. As the TTY core is built-in only, this has the side > effect of making serdev built-in as well. > > > +if SERIAL_DEV_BUS > + > +config SERIAL_DEV_CTRL_TTYPORT > + bool "Serial device TTY port controller" > + depends on TTY > + depends on SERIAL_DEV_BUS != m Since you have this line the if SERIAL_DEV_BUS is redundant for it. So, leave either one or another (as an example you can look at DMADEVICES). > + > +#define SERPORT_BUSY 1 > +#define SERPORT_ACTIVE 2 > +#define SERPORT_DEAD 3 > + > +struct serport { > + struct tty_port *port; > + struct tty_struct *tty; > + struct tty_driver *tty_drv; > + int tty_idx; Do you need tty_ prefix for them? > + struct mutex lock; > + unsigned long flags; > +}; > + > +/* > + * Callback functions from the tty port. > + */ > + > +static int ttyport_receive_buf(struct tty_port *port, const unsigned > char *cp, > + const unsigned char *fp, size_t > count) > +{ > + struct serdev_controller *ctrl = port->client_data; > + struct serport *serport = > serdev_controller_get_drvdata(ctrl); > + > + mutex_lock(&serport->lock); > + > + if (!test_bit(SERPORT_ACTIVE, &serport->flags)) So, if you are going to use serport->flags always under lock, you don't need to use atomic bit operations. Either __test_bit() and Co Or flags & BIT(x) > + goto out_unlock; > + > + serdev_controller_receive_buf(ctrl, cp, count); > + > +out_unlock: > + mutex_unlock(&serport->lock); > + return count; > +} > + > +static void ttyport_write_wakeup(struct tty_port *port) > +{ > + struct serdev_controller *ctrl = port->client_data; > + struct serport *serport = > serdev_controller_get_drvdata(ctrl); > + > + clear_bit(TTY_DO_WRITE_WAKEUP, &port->tty->flags); > + > + if (test_bit(SERPORT_ACTIVE, &serport->flags)) Hmm... > + serdev_controller_write_wakeup(ctrl); > +} > > + return tty_write_room(tty); > +} > + > + One extra line. > +static int ttyport_open(struct serdev_controller *ctrl) > +{ > + struct serport *serport = > serdev_controller_get_drvdata(ctrl); > + struct tty_struct *tty; > + struct ktermios ktermios; > + > + tty = tty_init_dev(serport->tty_drv, serport->tty_idx); > + serport->tty = tty; > + > + serport->port->client_ops = &client_ops; > + serport->port->client_data = ctrl; > + > > + tty->receive_room = 65536; Magic? > + > + if (tty->ops->open) > + tty->ops->open(serport->tty, NULL); > + else > + tty_port_open(serport->port, tty, NULL); > + > + /* Bring the UART into a known 8 bits no parity hw fc state > */ > + ktermios = tty->termios; > + 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); > + ktermios.c_cflag |= CS8; > + ktermios.c_cflag |= CRTSCTS; > + tty_set_termios(tty, &ktermios); > + > + set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags); > + > > + mutex_lock(&serport->lock); > + set_bit(SERPORT_ACTIVE, &serport->flags); > + mutex_unlock(&serport->lock); So, some clarification would be good to have to understand why you need mutex _and_ atomic operation together. What does mutex protect? > + > + tty_unlock(serport->tty); > + return 0; > +} > +void serdev_tty_port_unregister(struct tty_port *port) > +{ > + struct serdev_controller *ctrl = port->client_data; > + struct serport *serport = > serdev_controller_get_drvdata(ctrl); > + > + if (!serport) > + return; What this check prevents from? > + > + serdev_controller_remove(ctrl); > + port->client_ops = NULL; > + port->client_data = NULL; > + serdev_controller_put(ctrl); > +} -- Andy Shevchenko Intel Finland Oy