| Message ID | 20240822033924.32397-5-liulei.rjpt@vivo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tty serial drivers use devm_clk_get_enabled() helpers | expand |
Le 22/08/2024 à 05:39, Lei Liu a écrit : > The devm_clk_get_enabled() helpers: > - call devm_clk_get() > - call clk_prepare_enable() and register what is needed in order to > call clk_disable_unprepare() when needed, as a managed resource. > > This simplifies the code and avoids calls to clk_disable_unprepare(). > > Signed-off-by: Lei Liu <liulei.rjpt@vivo.com> > --- > drivers/tty/serial/atmel_serial.c | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c > index 09b246c9e389..209f3d41a17c 100644 > --- a/drivers/tty/serial/atmel_serial.c > +++ b/drivers/tty/serial/atmel_serial.c > @@ -2910,14 +2910,11 @@ 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"); > + atmel_port->clk = devm_clk_get_enabled(&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; > > atmel_port->gclk = devm_clk_get_optional(&pdev->dev, "gclk"); > if (IS_ERR(atmel_port->gclk)) { > @@ -2968,15 +2965,12 @@ static int atmel_serial_probe(struct platform_device *pdev) > * The peripheral clock can now safely be disabled till the port > * is used > */ > - clk_disable_unprepare(atmel_port->clk); > - Why removing this ? This is not an error path. > return 0; > > err_add_port: > kfree(atmel_port->rx_ring.buf); > atmel_port->rx_ring.buf = NULL; > err_clk_disable_unprepare: > - clk_disable_unprepare(atmel_port->clk); > clear_bit(atmel_port->uart.line, atmel_ports_in_use); > err: > return ret; Thanks, Richard.
On Thu, Aug 22, 2024 at 03:28:40PM +0200, Richard GENOUD wrote: > Le 22/08/2024 à 05:39, Lei Liu a écrit : > > The devm_clk_get_enabled() helpers: > > - call devm_clk_get() > > - call clk_prepare_enable() and register what is needed in order to > > call clk_disable_unprepare() when needed, as a managed resource. > > > > This simplifies the code and avoids calls to clk_disable_unprepare(). ... > > * The peripheral clock can now safely be disabled till the port > > * is used > > */ > > - clk_disable_unprepare(atmel_port->clk); > > - > Why removing this ? > This is not an error path. Good point, I wouldn't apply this patch as well as a few others in this series due to this reason. Instead it might make sense to add a comment on top of devm_clk_get() to explain why _enabled() variant is *not* used.
On 22/08/2024 16:34:28+0300, Andy Shevchenko wrote: > On Thu, Aug 22, 2024 at 03:28:40PM +0200, Richard GENOUD wrote: > > Le 22/08/2024 à 05:39, Lei Liu a écrit : > > > The devm_clk_get_enabled() helpers: > > > - call devm_clk_get() > > > - call clk_prepare_enable() and register what is needed in order to > > > call clk_disable_unprepare() when needed, as a managed resource. > > > > > > This simplifies the code and avoids calls to clk_disable_unprepare(). > > ... > > > > * The peripheral clock can now safely be disabled till the port > > > * is used > > > */ > > > - clk_disable_unprepare(atmel_port->clk); > > > - > > Why removing this ? > > This is not an error path. > > Good point, I wouldn't apply this patch as well as a few others in this series > due to this reason. > > Instead it might make sense to add a comment on top of devm_clk_get() to > explain why _enabled() variant is *not* used. Or maybe stop doing brainded conversions to new APIs. > > -- > With Best Regards, > Andy Shevchenko > >
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c index 09b246c9e389..209f3d41a17c 100644 --- a/drivers/tty/serial/atmel_serial.c +++ b/drivers/tty/serial/atmel_serial.c @@ -2910,14 +2910,11 @@ 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"); + atmel_port->clk = devm_clk_get_enabled(&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; atmel_port->gclk = devm_clk_get_optional(&pdev->dev, "gclk"); if (IS_ERR(atmel_port->gclk)) { @@ -2968,15 +2965,12 @@ static int atmel_serial_probe(struct platform_device *pdev) * The peripheral clock can now safely be disabled till the port * is used */ - clk_disable_unprepare(atmel_port->clk); - return 0; err_add_port: kfree(atmel_port->rx_ring.buf); atmel_port->rx_ring.buf = NULL; err_clk_disable_unprepare: - clk_disable_unprepare(atmel_port->clk); clear_bit(atmel_port->uart.line, atmel_ports_in_use); err: return ret;
The devm_clk_get_enabled() helpers: - call devm_clk_get() - call clk_prepare_enable() and register what is needed in order to call clk_disable_unprepare() when needed, as a managed resource. This simplifies the code and avoids calls to clk_disable_unprepare(). Signed-off-by: Lei Liu <liulei.rjpt@vivo.com> --- drivers/tty/serial/atmel_serial.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)