serial: 8250_dw: Use devm_add_action_or_reset()
Subsystems (3)
Andy Shevchenko
reviewerPATCH 1/2 serial: 8250_dw: Use devm_add_action_or_reset()
Slightly simplify ->probe() and drop a few goto labels by using devm_add_action_or_reset() for clock and reset cleanup.
--- drivers/tty/serial/8250/8250_dw.c | 63 +++++++++++++++---------------- 1 file changed, 31 insertions(+), 32 deletions(-)
drivers/tty/serial/8250/8250_dw.c +31 -32
@@ -484,6 +484,16 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data) } } +static void dw8250_clk_disable_unprepare (void *data) +{ +clk_disable_unprepare (data); +} + +static void dw8250_reset_control_assert (void *data) +{ +reset_control_assert (data); +} +static int dw8250_probe (struct platform_device *pdev) {struct uart_8250_port uart = {}, *up = &uart;
@@ -585,35 +595,43 @@ static int dw8250_probe(struct platform_device *pdev)if (err)dev_warn (dev,"could not enable optional baudclk: %d\n" , err); + err =devm_add_action_or_reset (dev, dw8250_clk_disable_unprepare, data->clk); +if (err) +return err; +if (data->clk) p->uartclk =clk_get_rate (data->clk);/* If no clock rate is defined, fail. */ if (!p->uartclk) {dev_err (dev,"clock rate not defined\n" ); - err = -EINVAL ; - goto err_clk; +return -EINVAL ; } data->pclk =devm_clk_get_optional (dev,"apb_pclk" ); -if (IS_ERR (data->pclk)) { - err =PTR_ERR (data->pclk); - goto err_clk; - } +if (IS_ERR (data->pclk)) +return PTR_ERR (data->pclk); err =clk_prepare_enable (data->pclk);if (err) {dev_err (dev,"could not enable apb_pclk\n" ); - goto err_clk; +return err; } + err =devm_add_action_or_reset (dev, dw8250_clk_disable_unprepare, data->pclk); +if (err) +return err; + data->rst =devm_reset_control_get_optional_exclusive (dev,NULL ); -if (IS_ERR (data->rst)) { - err =PTR_ERR (data->rst); - goto err_pclk; - } +if (IS_ERR (data->rst)) +return PTR_ERR (data->rst); +reset_control_deassert (data->rst); + err =devm_add_action_or_reset (dev, dw8250_reset_control_assert, data->rst); +if (err) +return err; +dw8250_quirks (p, data);/* If the Busy Functionality is not implemented, don't handle it */
@@ -631,10 +649,8 @@ static int dw8250_probe(struct platform_device *pdev) } data->data.line =serial8250_register_8250_port (up); -if (data->data.line <0 ) { - err = data->data.line; - goto err_reset; - } +if (data->data.line <0 ) +return data->data.line;/* * Some platforms may provide a reference clock shared between several
@@ -655,17 +671,6 @@ static int dw8250_probe(struct platform_device *pdev)pm_runtime_enable (dev);return 0 ; - -err_reset: -reset_control_assert (data->rst); - -err_pclk: -clk_disable_unprepare (data->pclk); - -err_clk: -clk_disable_unprepare (data->clk); - -return err; }static int dw8250_remove (struct platform_device *pdev)
@@ -683,12 +688,6 @@ static int dw8250_remove(struct platform_device *pdev)serial8250_unregister_port (data->data.line); -reset_control_assert (data->rst); - -clk_disable_unprepare (data->pclk); - -clk_disable_unprepare (data->clk); -pm_runtime_disable (dev);pm_runtime_put_noidle (dev);
Signature · 1 line
2.35.1
Andy Shevchenko
reviewerPATCH 2/2 serial: 8250_dw: Use dev_err_probe()
Simplify the error path in ->probe() a bit by using dev_err_probe().
--- drivers/tty/serial/8250/8250_dw.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-)
drivers/tty/serial/8250/8250_dw.c +11 -17
@@ -497,18 +497,17 @@ static void dw8250_reset_control_assert(void *data)static int dw8250_probe (struct platform_device *pdev) {struct uart_8250_port uart = {}, *up = &uart; -struct resource *regs =platform_get_resource (pdev,IORESOURCE_MEM ,0 );struct uart_port *p = &up->port;struct device *dev = &pdev->dev;struct dw8250_data *data; +struct resource *regs;int irq;int err;u32 val; -if (!regs) { -dev_err (dev,"no registers defined\n" ); -return -EINVAL ; - } + regs =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); +if (!regs) +return dev_err_probe (dev, -EINVAL ,"no registers defined\n" ); irq =platform_get_irq (pdev,0 );if (irq <0 )
@@ -593,7 +592,7 @@ static int dw8250_probe(struct platform_device *pdev) err =clk_prepare_enable (data->clk);if (err) -dev_warn (dev,"could not enable optional baudclk: %d\n" , err); +return dev_err_probe (dev, err,"could not enable optional baudclk\n" ); err =devm_add_action_or_reset (dev, dw8250_clk_disable_unprepare, data->clk);if (err)
@@ -603,20 +602,16 @@ static int dw8250_probe(struct platform_device *pdev) p->uartclk =clk_get_rate (data->clk);/* If no clock rate is defined, fail. */ -if (!p->uartclk) { -dev_err (dev,"clock rate not defined\n" ); -return -EINVAL ; - } +if (!p->uartclk) +return dev_err_probe (dev, -EINVAL ,"clock rate not defined\n" ); data->pclk =devm_clk_get_optional (dev,"apb_pclk" );if (IS_ERR (data->pclk))return PTR_ERR (data->pclk); err =clk_prepare_enable (data->pclk); -if (err) { -dev_err (dev,"could not enable apb_pclk\n" ); -return err; - } +if (err) +return dev_err_probe (dev, err,"could not enable apb_pclk\n" ); err =devm_add_action_or_reset (dev, dw8250_clk_disable_unprepare, data->pclk);if (err)
@@ -660,9 +655,8 @@ static int dw8250_probe(struct platform_device *pdev)if (data->clk) { err =clk_notifier_register (data->clk, &data->clk_notifier);if (err) -dev_warn (p->dev,"Failed to set the clock notifier\n" ); -else -queue_work (system_unbound_wq, &data->clk_work); +return dev_err_probe (dev, err,"Failed to set the clock notifier\n" ); +queue_work (system_unbound_wq, &data->clk_work); }platform_set_drvdata (pdev, data);
Signature · 1 line
2.35.1
Le 09/05/2022 à 19:21, Andy Shevchenko a écrit :
Quoted 13 lines
Slightly simplify ->probe() and drop a few goto labels by using
devm_add_action_or_reset()for clock and reset cleanup.Signed-off-by Andy Shevchenko <[email protected]>--- drivers/tty/serial/8250/8250_dw.c | 63 +++++++++++++++---------------- 1 file changed, 31 insertions(+), 32 deletions(-)drivers/tty/serial/8250/8250_dw.c +0
-0
drivers/tty/serial/8250/8250_dw.c static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data) · line 490
Quoted 7 lines
drivers/tty/serial/8250/8250_dw.c +4
-0@@ -484,6 +484,16 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data) } } +static void dw8250_clk_disable_unprepare (void *data) +{ +clk_disable_unprepare (data); +}
Hi,
we already have several time this function in different drivers. Maybe, it would be nice to have something standart for it.
A devm_clk_prepare_enable() or something devm-helpers.h ([1])
Just my 2c.
CJ
[1]: https://elixir.bootlin.com/linux/v5.18-rc6/source/include/linux/devm-helpers.h
Quoted 108 lines
drivers/tty/serial/8250/8250_dw.c +27
-32+ +static void dw8250_reset_control_assert (void *data) +{ +reset_control_assert (data); +} +static int dw8250_probe (struct platform_device *pdev) {struct uart_8250_port uart = {}, *up = &uart;@@ -585,35 +595,43 @@ static int dw8250_probe(struct platform_device *pdev)if (err)dev_warn (dev,"could not enable optional baudclk: %d\n" , err); + err =devm_add_action_or_reset (dev, dw8250_clk_disable_unprepare, data->clk); +if (err) +return err; +if (data->clk) p->uartclk =clk_get_rate (data->clk);/* If no clock rate is defined, fail. */ if (!p->uartclk) {dev_err (dev,"clock rate not defined\n" ); - err = -EINVAL ; - goto err_clk; +return -EINVAL ; } data->pclk =devm_clk_get_optional (dev,"apb_pclk" ); -if (IS_ERR (data->pclk)) { - err =PTR_ERR (data->pclk); - goto err_clk; - } +if (IS_ERR (data->pclk)) +return PTR_ERR (data->pclk); err =clk_prepare_enable (data->pclk);if (err) {dev_err (dev,"could not enable apb_pclk\n" ); - goto err_clk; +return err; } + err =devm_add_action_or_reset (dev, dw8250_clk_disable_unprepare, data->pclk); +if (err) +return err; + data->rst =devm_reset_control_get_optional_exclusive (dev,NULL ); -if (IS_ERR (data->rst)) { - err =PTR_ERR (data->rst); - goto err_pclk; - } +if (IS_ERR (data->rst)) +return PTR_ERR (data->rst); +reset_control_deassert (data->rst); + err =devm_add_action_or_reset (dev, dw8250_reset_control_assert, data->rst); +if (err) +return err; +dw8250_quirks (p, data);/* If the Busy Functionality is not implemented, don't handle it */ @@ -631,10 +649,8 @@ static int dw8250_probe(struct platform_device *pdev) } data->data.line =serial8250_register_8250_port (up); -if (data->data.line <0 ) { - err = data->data.line; - goto err_reset; - } +if (data->data.line <0 ) +return data->data.line;/* * Some platforms may provide a reference clock shared between several @@ -655,17 +671,6 @@ static int dw8250_probe(struct platform_device *pdev)pm_runtime_enable (dev);return 0 ; - -err_reset: -reset_control_assert (data->rst); - -err_pclk: -clk_disable_unprepare (data->pclk); - -err_clk: -clk_disable_unprepare (data->clk); - -return err; }static int dw8250_remove (struct platform_device *pdev)@@ -683,12 +688,6 @@ static int dw8250_remove(struct platform_device *pdev)serial8250_unregister_port (data->data.line); -reset_control_assert (data->rst); - -clk_disable_unprepare (data->pclk); - -clk_disable_unprepare (data->clk); -pm_runtime_disable (dev);pm_runtime_put_noidle (dev);
Quoted 1 line · Christophe JAILLET
Le 09/05/2022 à 19:21, Andy Shevchenko a écrit :
...
Quoted 9 lines
drivers/tty/serial/8250/8250_dw.c static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data) · line 490
Quoted 4 lines
drivers/tty/serial/8250/8250_dw.c +4
-0+static void dw8250_clk_disable_unprepare (void *data) +{ +clk_disable_unprepare (data); +}we already have several time this function in different drivers. Maybe, it would be nice to have something standart for it.
A
devm_clk_prepare_enable()or somethingdevm-helpers.h([1])
Seems you missed the full story. We tried to add that several times
[1] and CCF maintainers refused all the time. You may work with them to convince them.
[1]: https://lore.kernel.org/linux-clk/[email protected]/
(the latest one AFAIK)
Signature · 2 lines
With Best Regards, Andy Shevchenko
Le 09/05/2022 à 19:59, Andy Shevchenko a écrit :
Quoted 12 lines
Quoted 1 line · Christophe JAILLET
Le 09/05/2022 à 19:21, Andy Shevchenko a écrit :
...
Quoted 7 lines
drivers/tty/serial/8250/8250_dw.c static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data) · line 490
Quoted 4 lines
drivers/tty/serial/8250/8250_dw.c +4
-0+static void dw8250_clk_disable_unprepare (void *data) +{ +clk_disable_unprepare (data); +}we already have several time this function in different drivers. Maybe, it would be nice to have something standart for it.
Quoted 8 lines
Quoted 1 line
A
devm_clk_prepare_enable()or somethingdevm-helpers.h([1])Seems you missed the full story. We tried to add that several times
[1] and CCF maintainers refused all the time. You may work with them to convince them.[1]: https://lore.kernel.org/linux-clk/[email protected]/
(the latest one AFAIK)
LoL, got it.
Sorry for the noise.
CJ