Re: [PATCH v4 2/8] usb: chipidea: usb2: use devm_clk_get_optional_enabled()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] usb: chipidea: redundant struct and memory allocation for ci_hdrc_usb2_priv
--

commit a1804027a297ffe7216fa43c74cdb463efa0f512
Author: Junzhong Pan <panjunzhong@linux.spacemit.com>

usb: chipidea: usb2: use devm_clk_get_optional_enabled()

Convert devm_clk_get_optional() and clk_prepare_enable() into a single
call devm_clk_get_optional_enabled().

> diff --git a/drivers/usb/chipidea/ci_hdrc_usb2.c b/drivers/usb/chipidea/ci_hdrc_usb2.c
> index 8ffa1e95d8e86..9cef902f36b37 100644
> --- a/drivers/usb/chipidea/ci_hdrc_usb2.c
> +++ b/drivers/usb/chipidea/ci_hdrc_usb2.c
> @@ -20,7 +20,6 @@
>  
>  struct ci_hdrc_usb2_priv {
>  	struct platform_device	*ci_pdev;
> -	struct clk		*clk;
>  };

[Severity: Low]
Does this code still need a wrapper struct now that it only contains a
single pointer?

[ ... ]

> @@ -70,15 +70,10 @@ static int ci_hdrc_usb2_probe(struct platform_device *pdev)
>  	if (!priv)
>  		return -ENOMEM;
>  
> -	priv->clk = devm_clk_get_optional(dev, NULL);
> -	if (IS_ERR(priv->clk))
> -		return PTR_ERR(priv->clk);
> -
> -	ret = clk_prepare_enable(priv->clk);
> -	if (ret) {
> -		dev_err(dev, "failed to enable the clock: %d\n", ret);
> -		return ret;
> -	}
> +	clk = devm_clk_get_optional_enabled(dev, NULL);
> +	if (IS_ERR(clk))
> +		return dev_err_probe(dev, PTR_ERR(clk),
> +				     "failed to get or enable the clock\n");

[Severity: Low]
Could this devm_kzalloc() for the priv struct be eliminated? Since the clk
pointer was removed from ci_hdrc_usb2_priv, the single ci_pdev pointer
could be stored directly using platform_set_drvdata() in
ci_hdrc_usb2_probe(), avoiding the unnecessary heap allocation.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-b4-spacemit-k1-usb2-v4-0-2d98e5324d9a@linux.spacemit.com?part=2