On Fri, 2017-01-06 at 10:26 -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=y
Do you need one?
> +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))
> + goto out;
> +
> + serdev_controller_receive_buf(ctrl, cp, count);
> +
> +out:
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);
This doesn't prevent to be called this function in parallel. Is it okay?
> +
> + if (test_bit(SERPORT_ACTIVE, &serport->flags))
> + serdev_controller_write_wakeup(ctrl);
> +}
> +
> +static int ttyport_write_buf(struct serdev_controller *ctrl, const
> unsigned char *data, size_t len)
> +{
> + struct serport *serport =
> serdev_controller_get_drvdata(ctrl);
> + struct tty_struct *tty = serport->tty;
> +
> + set_bit(TTY_DO_WRITE_WAKEUP, &tty->flags);
> + return serport->tty->ops->write(serport->tty, data, len);
Just tty->ops->...(); ?
> +}
> +int serdev_tty_port_register(struct tty_port *port, struct device
> *parent,
> + struct tty_driver *drv, int idx)
> +{
> + struct serdev_controller *ctrl;
> + struct serport *serport;
> + int ret;
> +
> + if (!port || !drv || !parent || !parent->of_node)
And if it's ACPI? Perhaps last is redundant.
> + return -ENODEV;
> +
> + ctrl = serdev_controller_alloc(parent, sizeof(struct
> serport));
> + if (!ctrl)
> + return -ENOMEM;
> + serport = serdev_controller_get_drvdata(ctrl);
> +
> + mutex_init(&serport->lock);
> + serport->port = port;
> + serport->tty_idx = idx;
> + serport->tty_drv = drv;
> +
> + ctrl->ops = &ctrl_ops;
> +
> + ret = serdev_controller_add(ctrl);
> + if (ret)
> + goto err;
> +
> + printk(KERN_INFO "serdev: Serial port %s\n", drv->name);
Hmm... It's not a debug message, why not use pr_info()?
> + return 0;
> +
> +err:
err_controller_put: ?
> + serdev_controller_put(ctrl);
> + return ret;
> +}
> +
> +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;
Same question, whose responsibility to do this?
+
> +#ifdef CONFIG_SERIAL_DEV_CTRL_TTYPORT
> +int serdev_tty_port_register(struct tty_port *port, struct device
> *parent,
> + struct tty_driver *drv, int idx);
> +void serdev_tty_port_unregister(struct tty_port *port);
> +#else
> +static inline int serdev_tty_port_register(struct tty_port *port,
> + struct device *parent,
> + struct tty_driver *drv,
> int idx)
> +{
> + return -ENODEV;
> +}
> +static inline void serdev_tty_port_unregister(struct tty_port *port)
> {}
> +#endif
Perhaps comment to see from which if this one.
> +
> #endif
--
Andy Shevchenko <[email protected]>
Intel Finland Oy
