On Tue, Mar 14, 2023 at 09:35:59AM +0200, Tony Lindgren wrote:
> We want to enable runtime PM for serial port device drivers in a generic
> way. To do this, we want to have the serial core layer manage the
> registered physical serial controller devices.
>
> To do this, let's set up a struct bus and struct device for the serial
> core controller as suggested by Greg and Jiri. The serial core controller
> devices are children of the physical serial port device. The serial core
> controller device is needed to support multiple different kind of ports
> connected to single physical serial port device.
>
> Let's also set up a struct device for the serial core port. The serial
> core port instances are children of the serial core controller device.
> We need to also update the documentation a bit as suggested by Andy.
Now this can be moved to the comments section (below '---' cutter)
with perhaps addition why it's not done yet.
> With the serial core port device we can now flush pending TX on the
...
> + return (strncmp(dev_name(dev), drv->name, len) == 0);
Outer parentheses are redundant. The ' == 0' can be replaced with !,
but it's up to you.
...
> +struct serial_base_device *serial_base_device_add(struct uart_port *port,
> + const char *name,
> + struct device *parent_dev)
> +{
> + struct serial_base_device *dev;
Can we call this variable (and perhaps everywhere else) like sbd, or sbdev?
This will help to distinguish core device operations and serial one, because,
for example, I have stumbled over kfree(dev) and puzzled a lot.
> + int err, id;
> +
> + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> + if (!dev)
> + return NULL;
> +
> + device_initialize(&dev->dev);
> + dev->dev.parent = parent_dev;
> + dev->dev.bus = &serial_base_bus_type;
Who should provide a ->release() callback?
> + if (str_has_prefix(name, "ctrl")) {
> + id = port->ctrl_id;
> + } else {
> + id = port->line;
> + dev->port = port;
> + }
> +
> + err = dev_set_name(&dev->dev, "%s.%s.%d", name, dev_name(port->dev), id);
> + if (err)
> + goto err_free_dev;
> +
> + err = device_add(&dev->dev);
> + if (err)
> + goto err_free_name;
> +
> + return dev;
> +
> +err_free_name:
> + kfree_const(dev->dev.kobj.name);
It's still missing put_device() call as suggested by device_add() kernel
documentation. (Double) check also the removal path.
> +err_free_dev:
> + kfree(dev);
> + return NULL;
> +}
...
> +struct device;
Since you are embedding the device object this won't suffice,
you need to include device.h.
> +struct uart_driver;
> +struct uart_port;
> +
> +struct serial_base_device {
> + struct device dev;
> + struct uart_port *port;
> +};
> +
> +#define to_serial_base_device(x) container_of((x), struct serial_base_device, dev)
container_of.h
...
> + /* Increment the runtime PM usage count for the active check below */
> + err = pm_runtime_get(port_dev);
The question here is why don't we need to actually turn on the device immediately
(sync) if it's not already powered?
> + if (err < 0) {
> + pm_runtime_put_noidle(port_dev);
> + return;
> + }
> + /*
> + * Start TX if enabled, and kick runtime PM. Otherwise we must
> + * wait for a retry. See also serial_port.c for runtime PM
> + * autosuspend timeout.
> + */
I.o.w. does the start_tx() require device to be powered on at this point?
> + if (pm_runtime_active(port_dev))
> port->ops->start_tx(port);
> + pm_runtime_mark_last_busy(port_dev);
> + pm_runtime_put_autosuspend(port_dev);
...
> +++ b/drivers/tty/serial/serial_ctrl.c
> @@ -0,0 +1,69 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +
> +/*
> + * Serial core controller driver
> + *
> + * This driver manages the serial core controller struct device instances.
> + * The serial core controller devices are children of the physical serial
> + * port device.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/serial_core.h>
Include for the struct device_driver?
Do we need a separete include for EXPORT_SYMBOL_NS()?
...
> +/*
> + * Serial core port device driver
> + */
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/serial_core.h>
Similar questions.
+ spinlock.h?
...
> +static __maybe_unused DEFINE_RUNTIME_DEV_PM_OPS(serial_port_pm,
Do you still need __maybe_unused?
> + NULL,
> + serial_port_runtime_resume,
> + NULL);
--
With Best Regards,
Andy Shevchenko
