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 <andriy.shevche...@linux.intel.com>
Intel Finland Oy
