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:   Wed, 23 Nov 2022 20:37:03 +0200
From:   Andy Shevchenko <andriy.shevchenko@intel.com>
To:     Tony Lindgren <tony@atomide.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Ilpo =?iso-8859-1?Q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        Jiri Slaby <jirislaby@kernel.org>,
        Johan Hovold <johan@kernel.org>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        linux-serial@vger.kernel.org, linux-omap@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 2/2] serial: core: Add port port device to flush TX on
 runtime resume
Message-ID: <Y35oT9/3OKRciWCP@smile.fi.intel.com>
References: <20221123082825.32820-1-tony@atomide.com>
 <20221123082825.32820-2-tony@atomide.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221123082825.32820-2-tony@atomide.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

On Wed, Nov 23, 2022 at 10:28:25AM +0200, Tony Lindgren wrote:
> With PM runtime enabled for the serial port controllers, we can now flush
> pending TX for the port on runtime PM resume as suggested by
> Johan Hovold <johan@kernel.org>.

I believe it's quite a duplication of email addresses (they are in Cc and will
be on lore.kernel.org) and also below.

> To flush the pending TX, let's set up each port as a proper device as
> suggested by Greg Kroah-Hartman <gregkh@linuxfoundation.org>.
> 
> We set up each port as a child device for the serial port controller
> device. We use platform device for this and pass the port information
> in platform_data.
> 
> Let's just do mimimal changes needed for now, more port specific code
> can be then moved from serial_core.c to serial_port.c as needed.

...

> +static int serial_core_add_port_device(struct uart_port *port)
> +{
> +	struct serial_port_platdata pd;
> +	struct platform_device *pdev;
> +	int ret;
> +
> +	pdev = platform_device_alloc("serial-port", PLATFORM_DEVID_AUTO);
> +	if (!pdev)
> +		return -ENOMEM;
> +
> +	pdev->dev.parent = port->dev;
> +	pd.state = port->state;
> +
> +	ret = platform_device_add_data(pdev, &pd, sizeof(pd));
> +	if (ret)
> +		goto err_put;
> +
> +	ret = platform_device_add(pdev);
> +	if (ret)
> +		goto err_put;
> +
> +	port->state->port_dev = &pdev->dev;
> +
> +	return 0;
> +
> +err_put:
> +	platform_device_put(pdev);
> +
> +	return ret;

Can we simply utilize platform_device_register_full()?

> +}

...

> +EXPORT_SYMBOL(serial_port_get);

Can we move these to namespace from day 1?

...

> +	return (!uart_tx_stopped(port) &&
> +		uart_circ_chars_pending(&port->state->xmit));

Outer parentheses are redundant.

...

> +static const struct dev_pm_ops serial_port_pm = {
> +	SET_RUNTIME_PM_OPS(NULL, serial_port_runtime_resume, NULL)
> +};

Can't we use new macros / helpers which starts from DEFINE_...

...

> +	pm_runtime_set_autosuspend_delay(&pdev->dev,
> +					 SERIAL_PORT_AUTOSUSPEND_DELAY_MS);

With temporary

	struct device *dev = &pdev->dev;

this in particular become one line.

...

> +		.pm = &serial_port_pm,

As per above, use pm_ptr() ?

...

> +

Unneeded blank line.

> +module_platform_driver(serial_port_driver);

-- 
With Best Regards,
Andy Shevchenko