Re: [PATCH 2/3] tty: serial: atmel: improve clock management
From: Richard Genoud
Date: Fri Jun 10 2022 - 09:43:41 EST
Hi,
Le 08/06/2022 à 10:18, Claudiu.Beznea@xxxxxxxxxxxxx a écrit :
Hi,
On 02.06.2022 12:29, Richard Genoud wrote:
Hi,
Le 25/05/2022 à 15:37, Claudiu Beznea a écrit :
atmel_port->clk was requested in atmel_init_port() (which is called only
on probe path) only if atmel_port->clk was NULL. But atmel_port->clk is
NULL on probing path. Thus don't check this. Along with this the clock is
now requested with devm_clk_get() and the clock request has been moved in
atmel_serial_probe() function to avoid disabling/re-enabling it on probe
path for multiple times. All the checks of atmel_port->clk were removed
along with clk_put() and atmel_port->clk = NULL. Now, on probing time the
clock is enabled at the beginning and disabled at the end of probe. As
atmel_console_setup() is called in the middle of probe and clock is
already enabled at that time the clk_prepare_enable() in
atmel_console_setup() has also been removed.
Could you split this patch into smaller steps ?
I think it will be easier to read and review.
I kept it as a single patch as it is all related to clock management.
Having the clock enabled only at the beginning of probe and disabled at the
end of probe lead to removing the code in atmel_init_port(), also removing
the code under CONFIG_SERIAL_ATMEL_CONSOLE in probe. Same for the rest of
the removed code.
With this, would you still want to split it in multiple patches?
I think that, at least, the switch from clk_get() to devm_clk_get() can
be in a separate patch.
But in general, I prefer when patches are self-explaining rather than a
bigger patch with a longer explanation of what it does in the commit
message.
Regards,
Richard
Thank you,
Claudiu Beznea
Signed-off-by: Claudiu Beznea <claudiu.beznea@xxxxxxxxxxxxx>
---
drivers/tty/serial/atmel_serial.c | 65 +++++++------------------------
1 file changed, 15 insertions(+), 50 deletions(-)
diff --git a/drivers/tty/serial/atmel_serial.c
b/drivers/tty/serial/atmel_serial.c
index 5c793a23dc54..2955b1012014 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -2501,24 +2501,7 @@ static int atmel_init_port(struct atmel_uart_port
*atmel_port,
if (ret)
return ret;
- /* for console, the clock could already be configured */
- if (!atmel_port->clk) {
- atmel_port->clk = clk_get(&mpdev->dev, "usart");
- if (IS_ERR(atmel_port->clk)) {
- ret = PTR_ERR(atmel_port->clk);
- atmel_port->clk = NULL;
- return ret;
- }
- ret = clk_prepare_enable(atmel_port->clk);
- if (ret) {
- clk_put(atmel_port->clk);
- atmel_port->clk = NULL;
- return ret;
- }
- port->uartclk = clk_get_rate(atmel_port->clk);
- clk_disable_unprepare(atmel_port->clk);
- /* only enable clock when USART is in use */
- }
+ port->uartclk = clk_get_rate(atmel_port->clk);
/*
* Use TXEMPTY for interrupt when rs485 or ISO7816 else TXRDY or
@@ -2640,10 +2623,6 @@ static int __init atmel_console_setup(struct
console *co, char *options)
return -ENODEV;
}
- ret = clk_prepare_enable(atmel_ports[co->index].clk);
- if (ret)
- return ret;
-
Now, "int ret;" is unused, you can remove it.
atmel_uart_writel(port, ATMEL_US_IDR, -1);
atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_RSTSTA |
ATMEL_US_RSTRX);
atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN | ATMEL_US_RXEN);
@@ -2889,14 +2868,23 @@ static int atmel_serial_probe(struct
platform_device *pdev)
atomic_set(&atmel_port->tasklet_shutdown, 0);
spin_lock_init(&atmel_port->lock_suspended);
+ atmel_port->clk = devm_clk_get(&pdev->dev, "usart");
+ if (IS_ERR(atmel_port->clk)) {
+ ret = PTR_ERR(atmel_port->clk);
+ goto err;
+ }
+ ret = clk_prepare_enable(atmel_port->clk);
+ if (ret)
+ goto err;
+
ret = atmel_init_port(atmel_port, pdev);
if (ret)
- goto err_clear_bit;
+ goto err_clk_disable_unprepare;
atmel_port->gpios = mctrl_gpio_init(&atmel_port->uart, 0);
if (IS_ERR(atmel_port->gpios)) {
ret = PTR_ERR(atmel_port->gpios);
- goto err_clear_bit;
+ goto err_clk_disable_unprepare;
}
if (!atmel_use_pdc_rx(&atmel_port->uart)) {
@@ -2905,7 +2893,7 @@ static int atmel_serial_probe(struct
platform_device *pdev)
sizeof(struct atmel_uart_char),
GFP_KERNEL);
if (!data)
- goto err_alloc_ring;
+ goto err_clk_disable_unprepare;
atmel_port->rx_ring.buf = data;
}
@@ -2915,26 +2903,9 @@ static int atmel_serial_probe(struct
platform_device *pdev)
if (ret)
goto err_add_port;
-#ifdef CONFIG_SERIAL_ATMEL_CONSOLE
- if (uart_console(&atmel_port->uart)
- && ATMEL_CONSOLE_DEVICE->flags & CON_ENABLED) {
- /*
- * The serial core enabled the clock for us, so undo
- * the clk_prepare_enable() in atmel_console_setup()
- */
- clk_disable_unprepare(atmel_port->clk);
- }
-#endif
-
device_init_wakeup(&pdev->dev, 1);
platform_set_drvdata(pdev, atmel_port);
- /*
- * The peripheral clock has been disabled by atmel_init_port():
- * enable it before accessing I/O registers
- */
- clk_prepare_enable(atmel_port->clk);
-
if (rs485_enabled) {
atmel_uart_writel(&atmel_port->uart, ATMEL_US_MR,
ATMEL_US_USMODE_NORMAL);
@@ -2958,12 +2929,8 @@ static int atmel_serial_probe(struct
platform_device *pdev)
err_add_port:
kfree(atmel_port->rx_ring.buf);
atmel_port->rx_ring.buf = NULL;
-err_alloc_ring:
- if (!uart_console(&atmel_port->uart)) {
- clk_put(atmel_port->clk);
- atmel_port->clk = NULL;
- }
-err_clear_bit:
+err_clk_disable_unprepare:
+ clk_disable_unprepare(atmel_port->clk);
clear_bit(atmel_port->uart.line, atmel_ports_in_use);
err:
return ret;
@@ -2997,8 +2964,6 @@ static int atmel_serial_remove(struct
platform_device *pdev)
clear_bit(port->line, atmel_ports_in_use);
- clk_put(atmel_port->clk);
- atmel_port->clk = NULL;
pdev->dev.of_node = NULL;
return ret;
Thanks !
Regards,
Richard