| Message ID | 20240826082140.2311-1-liulei.rjpt@vivo.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | usb drivers use devm_clk_get_enabled() helpers | expand |
Hello, On Mon, Aug 26, 2024 at 04:21:39PM +0800, Lei Liu wrote: > diff --git a/drivers/usb/gadget/udc/aspeed_udc.c b/drivers/usb/gadget/udc/aspeed_udc.c > index f4781e611aaa..a362e31f7550 100644 > --- a/drivers/usb/gadget/udc/aspeed_udc.c > +++ b/drivers/usb/gadget/udc/aspeed_udc.c > @@ -1459,8 +1459,6 @@ static void ast_udc_remove(struct platform_device *pdev) > ctrl = ast_udc_read(udc, AST_UDC_FUNC_CTRL) & ~USB_UPSTREAM_EN; > ast_udc_write(udc, ctrl, AST_UDC_FUNC_CTRL); > > - clk_disable_unprepare(udc->clk); > - > spin_unlock_irqrestore(&udc->lock, flags); Isn't it broken to call clk_disable_unprepare() while holding a spinlock? I guess that means that the remove path is untested in practise and this patches fixes a sleep-in-atomic. IMHO this invalidates Ulf's concern in his reply to the cover letter for this patch at least. > if (udc->ep0_buf) > @@ -1500,16 +1498,11 @@ static int ast_udc_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, udc); > > - udc->clk = devm_clk_get(&pdev->dev, NULL); > + udc->clk = devm_clk_get_enabled(&pdev->dev, NULL); > if (IS_ERR(udc->clk)) { > rc = PTR_ERR(udc->clk); An error message here would be nice. Something like rc = dev_err_probe(&pdev->dev, PTR_ERR(udc->clk), "Failed to get clock\n"); should work. > goto err; > } > - rc = clk_prepare_enable(udc->clk); > - if (rc) { > - dev_err(&pdev->dev, "Failed to enable clock (0x%x)\n", rc); > - goto err; > - } > > /* Check if we need to limit the HW to USB1 */ > max_speed = usb_get_maximum_speed(&pdev->dev); Best regards Uwe
diff --git a/drivers/usb/gadget/udc/aspeed_udc.c b/drivers/usb/gadget/udc/aspeed_udc.c index f4781e611aaa..a362e31f7550 100644 --- a/drivers/usb/gadget/udc/aspeed_udc.c +++ b/drivers/usb/gadget/udc/aspeed_udc.c @@ -1459,8 +1459,6 @@ static void ast_udc_remove(struct platform_device *pdev) ctrl = ast_udc_read(udc, AST_UDC_FUNC_CTRL) & ~USB_UPSTREAM_EN; ast_udc_write(udc, ctrl, AST_UDC_FUNC_CTRL); - clk_disable_unprepare(udc->clk); - spin_unlock_irqrestore(&udc->lock, flags); if (udc->ep0_buf) @@ -1500,16 +1498,11 @@ static int ast_udc_probe(struct platform_device *pdev) platform_set_drvdata(pdev, udc); - udc->clk = devm_clk_get(&pdev->dev, NULL); + udc->clk = devm_clk_get_enabled(&pdev->dev, NULL); if (IS_ERR(udc->clk)) { rc = PTR_ERR(udc->clk); goto err; } - rc = clk_prepare_enable(udc->clk); - if (rc) { - dev_err(&pdev->dev, "Failed to enable clock (0x%x)\n", rc); - goto err; - } /* Check if we need to limit the HW to USB1 */ max_speed = usb_get_maximum_speed(&pdev->dev);
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/usb/gadget/udc/aspeed_udc.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)