Messages in this thread | | | Date | Wed, 7 Dec 2022 22:22:11 +0200 | From | Andy Shevchenko <> | Subject | Re: [RFC PATCH v4 1/1] serial: core: Start managing serial controllers to enable runtime PM |
| |
On Wed, Dec 07, 2022 at 02:43:05PM +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 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. > With the serial core port device we can now flush pending TX on the > runtime PM resume as suggested by Johan.
...
> +static struct platform_device *serial_core_device_add(struct uart_port *port, > + const char *name, > + struct device *parent_dev, > + void *pdata, > + int pdata_size) > +{ > + struct platform_device_info pinfo;
' = {}' can also work instead of memset(), but up to you.
> + memset(&pinfo, 0, sizeof(pinfo)); > + pinfo.name = name; > + pinfo.id = PLATFORM_DEVID_AUTO; > + pinfo.parent = parent_dev; > + pinfo.data = pdata; > + pinfo.size_data = pdata_size; > + > + return platform_device_register_full(&pinfo); > +}
...
> + struct serial_port_platdata pdata;
Ditto.
> + memset(&pdata, 0, sizeof(pdata));
...
> +int serial_core_register_port(struct uart_driver *drv, struct uart_port *port) > +{
> + bool allocated = false;
Not sure why this is needed.
> + struct device *ctrl_dev; > + int ret; > + > + ret = serial_core_add_one_port(drv, port); > + if (ret) > + return ret; > + > + mutex_lock(&port_mutex); > + > + /* 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) > + goto err_remove_port; > + allocated = true; > + }
Wouldn't be slightly better
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) goto err_remove_port;
?
> + /* Initialize a serial core port device */ > + ret = serial_core_port_device_add(ctrl_dev, port); > + if (ret) > + goto err_del_ctrl_dev; > + > + mutex_unlock(&port_mutex); > + > + return 0; > + > +err_del_ctrl_dev: > + if (allocated) > + platform_device_del(to_platform_device(ctrl_dev));
Shouldn't you call platform_device_unregister()?
> +err_remove_port: > + mutex_unlock(&port_mutex); > + > + return serial_core_remove_one_port(drv, port); > +}
...
> + platform_device_del(to_platform_device(port_dev));
Ditto?
...
> + /* Drop the serial core controller device if no ports are using it */ > + if (!serial_core_ctrl_find(drv, port->dev, port->ctrl_id)) > + platform_device_del(to_platform_device(ctrl_dev));
Ditto?
...
> +/* Serial core controller data. Serial port device drivers do not need this. */
Some missing forward declarations?
struct uart_driver: struct uart_port;
?
...
> +static DEFINE_RUNTIME_DEV_PM_OPS(serial_port_pm, NULL, > + serial_port_runtime_resume, NULL);
Wouldn't be more logical to indent like
static DEFINE_RUNTIME_DEV_PM_OPS(serial_port_pm, NULL, serial_port_runtime_resume, NULL);
?
-- With Best Regards, Andy Shevchenko
| |