On Thu, Mar 23, 2023 at 09:10:47AM +0200, Tony Lindgren wrote:
> -obj-$(CONFIG_SERIAL_CORE) += serial_core.o
> +obj-$(CONFIG_SERIAL_CORE) += serial_base.o serial_core.o serial_ctrl.o serial_port.o
Why is this 3 new modules and not just all go into serial_base? What's
going to auto-load the other modules you created here? Feels like this
should all end up in the same .ko as they all depend on each other,
right?
>
> obj-$(CONFIG_SERIAL_EARLYCON) += earlycon.o
> obj-$(CONFIG_SERIAL_EARLYCON_SEMIHOST) += earlycon-semihost.o
> diff --git a/drivers/tty/serial/serial_base.c b/drivers/tty/serial/serial_base.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/tty/serial/serial_base.c
> @@ -0,0 +1,142 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/*
> + * Serial core base layer for controllers
> + *
> + * The serial core bus manages the serial core controller instances.
> + */
> +
> +#include <linux/container_of.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/serial_core.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +#include "serial_base.h"
> +
> +struct serial_base_device {
> + struct device dev;
> + struct uart_port *port;
> +};
> +
> +#define to_serial_base_device(d) container_of((d), struct serial_base_device, dev)
> +
> +struct uart_port *serial_base_get_port(struct device *dev)
> +{
> + struct serial_base_device *sbd;
> +
> + if (!dev)
> + return NULL;
> +
> + sbd = to_serial_base_device(dev);
> +
> + /* Check in case serial_core_add_one_port() happened to fail */
> + if (!sbd->port->state) {
This is odd, how can it fail and then this function be called after that
failure?
> + dev_warn(dev, "uninitialized serial port?\n");
> + return NULL;
> + }
> +
> + return sbd->port;
> +}
> +EXPORT_SYMBOL_NS(serial_base_get_port, SERIAL_BASE);
> +
> +static int serial_base_match(struct device *dev, struct device_driver *drv)
> +{
> + int len = strlen(drv->name);
> +
> + return !strncmp(dev_name(dev), drv->name, len);
> +}
> +
> +static struct bus_type serial_base_bus_type = {
> + .name = "serial-base",
> + .match = serial_base_match,
> +};
> +
> +int serial_base_driver_register(struct device_driver *driver)
> +{
> + driver->bus = &serial_base_bus_type;
> +
> + return driver_register(driver);
> +}
> +EXPORT_SYMBOL_NS(serial_base_driver_register, SERIAL_BASE);
> +
> +void serial_base_driver_unregister(struct device_driver *driver)
> +{
> + driver_unregister(driver);
> +}
> +EXPORT_SYMBOL_NS(serial_base_driver_unregister, SERIAL_BASE);
> +
> +static void serial_base_release(struct device *dev)
> +{
> + struct serial_base_device *sbd = to_serial_base_device(dev);
> +
> + kfree(sbd);
> +}
> +
> +struct device *serial_base_device_add(struct uart_port *port, const char *name,
> + struct device *parent_dev)
> +{
> + struct serial_base_device *sbd;
> + int err, id;
> +
> + sbd = kzalloc(sizeof(*sbd), GFP_KERNEL);
> + if (!sbd)
> + return NULL;
> +
> + device_initialize(&sbd->dev);
> + sbd->dev.parent = parent_dev;
> + sbd->dev.bus = &serial_base_bus_type;
> + sbd->dev.release = &serial_base_release;
> +
> + if (str_has_prefix(name, "ctrl")) {
That's a magic string, shouldn't it be documented somewhere?
> + id = port->ctrl_id;
> + } else {
> + id = port->line;
> + sbd->port = port;
> + }
> +
> + err = dev_set_name(&sbd->dev, "%s.%s.%d", name, dev_name(port->dev), id);
> + if (err)
> + goto err_free_dev;
> +
> + err = device_add(&sbd->dev);
> + if (err)
> + goto err_put_device;
> +
> + return &sbd->dev;
> +
> +err_put_device:
> + put_device(&sbd->dev);
> +
> +err_free_dev:
> + kfree(sbd);
> +
> + return NULL;
> +}
> +EXPORT_SYMBOL_NS(serial_base_device_add, SERIAL_BASE);
> +
> +void serial_base_device_remove(struct device *dev)
> +{
> + if (!dev)
> + return;
> +
> + device_del(dev);
> +}
> +EXPORT_SYMBOL_NS(serial_base_device_remove, SERIAL_BASE);
> +
> +static int serial_base_init(void)
> +{
> + return bus_register(&serial_base_bus_type);
> +}
> +module_init(serial_base_init);
> +
> +static void serial_base_exit(void)
> +{
> + bus_unregister(&serial_base_bus_type);
> +}
> +module_exit(serial_base_exit);
> +
> +MODULE_AUTHOR("Tony Lindgren <tony@xxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Serial core bus");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/tty/serial/serial_base.h b/drivers/tty/serial/serial_base.h
> new file mode 100644
> --- /dev/null
> +++ b/drivers/tty/serial/serial_base.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +/* Serial core related functions, serial port device drivers do not need this. */
> +
> +struct uart_driver;
> +struct uart_port;
> +struct device_driver;
> +struct device;
> +
> +int serial_base_driver_register(struct device_driver *driver);
> +void serial_base_driver_unregister(struct device_driver *driver);
> +struct device *serial_base_device_add(struct uart_port *port, const char *name,
> + struct device *parent_dev);
> +void serial_base_device_remove(struct device *dev);
> +struct uart_port *serial_base_get_port(struct device *dev);
> +
> +int serial_ctrl_register_port(struct uart_driver *drv, struct uart_port *port);
> +void serial_ctrl_unregister_port(struct uart_driver *drv, struct uart_port *port);
> +
> +int serial_core_register_port(struct uart_driver *drv, struct uart_port *port);
> +void serial_core_unregister_port(struct uart_driver *drv, struct uart_port *port);
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -17,6 +17,7 @@
> #include <linux/gpio/consumer.h>
> #include <linux/kernel.h>
> #include <linux/of.h>
> +#include <linux/pm_runtime.h>
> #include <linux/proc_fs.h>
> #include <linux/seq_file.h>
> #include <linux/device.h>
> @@ -31,6 +32,8 @@
> #include <linux/irq.h>
> #include <linux/uaccess.h>
>
> +#include "serial_base.h"
> +
> /*
> * This is used to lock changes in serial line configuration.
> */
> @@ -134,9 +137,30 @@ static void __uart_start(struct tty_struct *tty)
> {
> struct uart_state *state = tty->driver_data;
> struct uart_port *port = state->uart_port;
> + struct device *port_dev;
> + int err;
> +
> + if (!port || uart_tx_stopped(port))
> + return;
> +
> + port_dev = port->port_dev;
> +
> + /* Increment the runtime PM usage count for the active check below */
> + err = pm_runtime_get(port_dev);
> + if (err < 0) {
> + pm_runtime_put_noidle(port_dev);
> + return;
> + }
>
> - if (port && !uart_tx_stopped(port))
> + /*
> + * Start TX if enabled, and kick runtime PM. If the device is not
> + * enabled, serial_port_runtime_resume() calls start_tx() again
> + * after enabling the device.
> + */
> + if (pm_runtime_active(port_dev))
> port->ops->start_tx(port);
> + pm_runtime_mark_last_busy(port_dev);
> + pm_runtime_put_autosuspend(port_dev);
> }
>
> static void uart_start(struct tty_struct *tty)
> @@ -3036,7 +3060,7 @@ static const struct attribute_group tty_dev_attr_group = {
> };
>
> /**
> - * uart_add_one_port - attach a driver-defined port structure
> + * serial_core_add_one_port - attach a driver-defined port structure
> * @drv: pointer to the uart low level driver structure for this port
> * @uport: uart port structure to use for this port.
> *
> @@ -3046,7 +3070,7 @@ static const struct attribute_group tty_dev_attr_group = {
> * core driver. The main purpose is to allow the low level uart drivers to
> * expand uart_port, rather than having yet more levels of structures.
> */
> -int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
> +static int serial_core_add_one_port(struct uart_driver *drv, struct uart_port *uport)
> {
> struct uart_state *state;
> struct tty_port *port;
> @@ -3136,10 +3160,9 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
>
> return ret;
> }
> -EXPORT_SYMBOL(uart_add_one_port);
>
> /**
> - * uart_remove_one_port - detach a driver defined port structure
> + * serial_core_remove_one_port - detach a driver defined port structure
> * @drv: pointer to the uart low level driver structure for this port
> * @uport: uart port structure for this port
> *
> @@ -3148,7 +3171,8 @@ EXPORT_SYMBOL(uart_add_one_port);
> * This unhooks (and hangs up) the specified port structure from the core
> * driver. No further calls will be made to the low-level code for this port.
> */
> -int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
> +static int serial_core_remove_one_port(struct uart_driver *drv,
> + struct uart_port *uport)
> {
> struct uart_state *state = drv->state + uport->line;
> struct tty_port *port = &state->port;
> @@ -3205,6 +3229,8 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
> * Indicate that there isn't a port here anymore.
> */
> uport->type = PORT_UNKNOWN;
> + uport->port_dev = NULL;
> + uport->ctrl_id = -ENODEV;
>
> mutex_lock(&port->mutex);
> WARN_ON(atomic_dec_return(&state->refcount) < 0);
> @@ -3216,7 +3242,6 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
>
> return ret;
> }
> -EXPORT_SYMBOL(uart_remove_one_port);
>
> /**
> * uart_match_port - are the two ports equivalent?
> @@ -3251,6 +3276,127 @@ bool uart_match_port(const struct uart_port *port1,
> }
> EXPORT_SYMBOL(uart_match_port);
>
> +/*
> + * Find a registered serial core controller device if one exists. Returns
> + * the first device matching the ctrl_id. Caller must hold port_mutex.
> + */
> +static struct device *serial_core_ctrl_find(struct uart_driver *drv,
> + struct device *phys_dev,
> + int ctrl_id)
> +{
> + struct uart_state *state;
> + int i;
> +
> + if (ctrl_id < 0)
> + return NULL;
Why is a negative number special here?
> +
> + lockdep_assert_held(&port_mutex);
> +
> + for (i = 0; i < drv->nr; i++) {
> + state = drv->state + i;
> + if (!state->uart_port || !state->uart_port->port_dev)
> + continue;
> +
> + if (state->uart_port->dev == phys_dev &&
> + state->uart_port->ctrl_id == ctrl_id)
> + return state->uart_port->port_dev->parent;
> + }
> +
> + return NULL;
> +}
> +
> +static struct device *serial_core_ctrl_device_add(struct uart_port *port)
> +{
> + return serial_base_device_add(port, "ctrl", port->dev);
> +}
> +
> +static int serial_core_port_device_add(struct device *ctrl_dev, struct uart_port *port)
> +{
> + struct device *dev;
> +
> + dev = serial_base_device_add(port, "port", ctrl_dev);
magic strings again :)
Do you really just want two different "types" of devices on this bus,
controllers and ports? If so, just do that, don't make the name magic
here.
Then you can have:
serial_base_port_add()
serial_base_ctrl_add()
and one cleanup function will still work.
Otherwise this looks good to me, thanks for doing all of this work.
greg k-h
