Date:   Thu, 9 Mar 2023 14:23:11 +0200
From:   Andy Shevchenko <andriy.shevchenko@intel.com>
To:     Tony Lindgren <tony@atomide.com>
Cc:     "Rafael J. Wysocki" <rafael@kernel.org>,
        Len Brown <lenb@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        Ilpo =?iso-8859-1?Q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        Johan Hovold <johan@kernel.org>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        linux-omap@vger.kernel.org, linux-acpi@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org
Subject: Re: [PATCH v6 1/1] serial: core: Start managing serial controllers
 to enable runtime PM
Message-ID: <ZAnPrwLUA/1Bsq26@smile.fi.intel.com>
References: <20230309085713.57700-1-tony@atomide.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230309085713.57700-1-tony@atomide.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

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