Re: [PATCH v6 1/1] serial: core: Start managing serial controllers to enable runtime PM

Andy Shevchenko
2023-03-09T12:23:21Z

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