Messages in this thread | | | Date | Thu, 9 Mar 2023 14:23:11 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v6 1/1] serial: core: Start managing serial controllers to enable runtime PM |
| |
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
| |