Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 13 May 2023 20:07:31 +0900
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Tony Lindgren <tony@atomide.com>
Cc:     Jiri Slaby <jirislaby@kernel.org>,
        Andy Shevchenko <andriy.shevchenko@intel.com>,
        Dhruva Gole <d-gole@ti.com>,
        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,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org
Subject: Re: [PATCH v11 1/1] serial: core: Start managing serial controllers
 to enable runtime PM
Message-ID: <2023051332-pretended-spoiler-61fc@gregkh>
References: <20230511065355.47525-1-tony@atomide.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230511065355.47525-1-tony@atomide.com>
Precedence: bulk

On Thu, May 11, 2023 at 09:53:51AM +0300, 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.
> 
> With the serial core port device we can now flush pending TX on the
> runtime PM resume as suggested by Johan.

Much better, thanks!

One thing jumps out at me though, you are passing around "raw" struct
device pointers as the serial port structure, why?

Shouldn't:

> @@ -563,7 +564,8 @@ struct uart_port {
>  	unsigned int		minor;
>  	resource_size_t		mapbase;		/* for ioremap */
>  	resource_size_t		mapsize;
> -	struct device		*dev;			/* parent device */
> +	struct device		*dev;			/* serial port physical parent device */
> +	struct device		*port_dev;		/* serial core port device */

port_dev here be something like "struct serial_port" (or some better
name)?  That way you enforce the type being passed around to the serial
code in this change which will help catch any type mistakes.

Yes, this structure can just be a "wrapper" around 'struct device' but
at least it's a unique type.

Or am I missing why this was done this way?

thanks,

greg k-h