On 05/10/2021 17:59, Hector Martin wrote:
> This allows idle UART devices to be suspended using the standard
> runtime-PM framework. The logic is modeled after stm32-usart.
>
> Signed-off-by: Hector Martin <marcan@xxxxxxxxx>
> ---
> drivers/tty/serial/samsung_tty.c | 88 ++++++++++++++++++++------------
> 1 file changed, 54 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index e2f49863e9c2..d68e3341adc6 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -40,6 +40,7 @@
> #include <linux/clk.h>
> #include <linux/cpufreq.h>
> #include <linux/of.h>
> +#include <linux/pm_runtime.h>
> #include <asm/irq.h>
>
> /* UART name and device definitions */
> @@ -1381,31 +1382,49 @@ static void exynos_usi_init(struct uart_port *port)
>
> /* power power management control */
>
> -static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level,
> - unsigned int old)
> +static int __maybe_unused s3c24xx_serial_runtime_suspend(struct device *dev)
> {
> + struct uart_port *port = dev_get_drvdata(dev);
> struct s3c24xx_uart_port *ourport = to_ourport(port);
> int timeout = 10000;
>
> - ourport->pm_level = level;
> + while (--timeout && !s3c24xx_serial_txempty_nofifo(port))
> + udelay(100);
>
> - switch (level) {
> - case 3:
> - while (--timeout && !s3c24xx_serial_txempty_nofifo(port))
> - udelay(100);
> + if (!IS_ERR(ourport->baudclk))
> + clk_disable_unprepare(ourport->baudclk);
>
> - if (!IS_ERR(ourport->baudclk))
> - clk_disable_unprepare(ourport->baudclk);
> + clk_disable_unprepare(ourport->clk);
> + return 0;
> +};
>
> - clk_disable_unprepare(ourport->clk);
> - break;
> +static int __maybe_unused s3c24xx_serial_runtime_resume(struct device *dev)
> +{
> + struct uart_port *port = dev_get_drvdata(dev);
> + struct s3c24xx_uart_port *ourport = to_ourport(port);
>
> - case 0:
> - clk_prepare_enable(ourport->clk);
> + clk_prepare_enable(ourport->clk);
>
> - if (!IS_ERR(ourport->baudclk))
> - clk_prepare_enable(ourport->baudclk);
> + if (!IS_ERR(ourport->baudclk))
> + clk_prepare_enable(ourport->baudclk);
> + return 0;
> +};
> +
> +static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level,
> + unsigned int old)
> +{
> + struct s3c24xx_uart_port *ourport = to_ourport(port);
> +
> + ourport->pm_level = level;
>
> + switch (level) {
> + case UART_PM_STATE_OFF:
> + pm_runtime_mark_last_busy(port->dev);
> + pm_runtime_put_sync(port->dev);
> + break;
> +
> + case UART_PM_STATE_ON:
> + pm_runtime_get_sync(port->dev);
> exynos_usi_init(port);
> break;
> default:
> @@ -2282,18 +2301,15 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
> }
> }
>
> + pm_runtime_get_noresume(&pdev->dev);
> + pm_runtime_set_active(&pdev->dev);
> + pm_runtime_enable(&pdev->dev);
> +
You need to cleanup in error paths (put/disable).
> dev_dbg(&pdev->dev, "%s: adding port\n", __func__);
> uart_add_one_port(&s3c24xx_uart_drv, &ourport->port);
> platform_set_drvdata(pdev, &ourport->port);
>
> - /*
> - * Deactivate the clock enabled in s3c24xx_serial_init_port here,
> - * so that a potential re-enablement through the pm-callback overlaps
> - * and keeps the clock enabled in this case.
> - */
> - clk_disable_unprepare(ourport->clk);
> - if (!IS_ERR(ourport->baudclk))
> - clk_disable_unprepare(ourport->baudclk);
> + pm_runtime_put_sync(&pdev->dev);
>
> ret = s3c24xx_serial_cpufreq_register(ourport);
> if (ret < 0)
> @@ -2309,8 +2325,14 @@ static int s3c24xx_serial_remove(struct platform_device *dev)
> struct uart_port *port = s3c24xx_dev_to_port(&dev->dev);
>
> if (port) {
> + pm_runtime_get_sync(&dev->dev);
1. You need to check return status.
2. Why do you need to resume the device here?
> +
> s3c24xx_serial_cpufreq_deregister(to_ourport(port));
> uart_remove_one_port(&s3c24xx_uart_drv, port);
> +
> + pm_runtime_disable(&dev->dev);
Why disabling it only if port!=NULL? Can remove() be called if
platform_set_drvdata() was not?
> + pm_runtime_set_suspended(&dev->dev);
> + pm_runtime_put_noidle(&dev->dev);
> }
>
> uart_unregister_driver(&s3c24xx_uart_drv);
> @@ -2319,8 +2341,8 @@ static int s3c24xx_serial_remove(struct platform_device *dev)
> }
>
Best regards,
Krzysztof
