On Thu, Mar 09, 2023 at 10:57:08AM +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.
>
> With the serial core port device we can now flush pending TX on the
> runtime PM resume as suggested by Johan.
Thanks, my comments below.
...
> - Devices behind real busses where there is a connector resource
> - are represented as struct spi_device or struct i2c_device. Note
> - that standard UARTs are not busses so there is no struct uart_device,
> - although some of them may be represented by struct serdev_device.
> + are represented as struct spi_device, struct i2c_device or
> + struct serdev_device.
JFYI: the i2c_device will be changed soon to i2c_client in the v6.3-rcX,
so this will have a conflict.
...
> + if (!strncmp(name, "ctrl", 4)) {
Wouldn't str_has_previx() be better to show the intention?
> + id = port->ctrl_id;
> + } else {
> + id = port->line;
> + dev->port = port;
> + }
...
> + dev_set_name(&dev->dev, "%s.%s.%d", name, dev_name(port->dev), id);
No error check?
...
> + ret = device_add(&dev->dev);
> + if (ret) {
> + kfree(dev);
Would it free the device name?
> + return NULL;
> + }
...
> +EXPORT_SYMBOL_GPL(serial_base_device_add);
I'm wondering if we can use namespace from day 1 for this.
...
> +static int serial_base_init(void)
> +{
> + return bus_register(&serial_base_bus_type);
> +}
> +
> +static void serial_base_exit(void)
> +{
> + bus_unregister(&serial_base_bus_type);
> +}
> +
Redundant blank line and...
> +module_init(serial_base_init);
...move this to be after the function itself.
> +module_exit(serial_base_exit);
...
> +extern int serial_base_driver_register(struct device_driver *driver);
> +extern void serial_base_driver_unregister(struct device_driver *driver);
> +extern struct serial_base_device *serial_base_device_add(struct uart_port *port,
> + const char *name,
> + struct device *parent_dev);
> +extern void serial_base_device_remove(struct serial_base_device *dev);
> +
> +extern int serial_ctrl_register_port(struct uart_driver *drv, struct uart_port *port);
> +extern void serial_ctrl_unregister_port(struct uart_driver *drv, struct uart_port *port);
> +
> +extern int serial_core_register_port(struct uart_driver *drv, struct uart_port *port);
> +extern void serial_core_unregister_port(struct uart_driver *drv, struct uart_port *port);
I believe you do not need "extern" for the function declarations here.
...
> + err = pm_runtime_get(port_dev);
Is not sync API a deliberate choice? Do we need to comment on why is so?
...
> + bool added = false;
> + /* Inititalize a serial core controller device if needed */
> + ctrl_dev = serial_core_ctrl_find(drv, port->dev, port->ctrl_id);
> + if (!ctrl_dev) {
> + ctrl_dev = serial_core_ctrl_device_add(port);
> + if (!ctrl_dev) {
> + ret = -ENODEV;
> + goto err_unlock;
> + }
> + added = true;
> + }
> + if (added)
> + serial_base_device_remove(to_serial_base_device(ctrl_dev));
Wondering if it makes sense to add a boolean directly into uart_port and drop
this conditional here and move it to the callee.
...
> +
> +module_init(serial_ctrl_init);
> +module_exit(serial_ctrl_exit);
Can we also move these closer to the respective functions?
...
> +
> +module_init(serial_port_init);
> +module_exit(serial_port_exit);
Ditto.
--
With Best Regards,
Andy Shevchenko
