On Mon, Nov 24, 2014 at 10:49:57AM -0600, Dinh Nguyen wrote:
> +CC: Felipe Balbi
>
> On 11/24/2014 04:48 AM, Dan Carpenter wrote:
> > On Mon, Nov 24, 2014 at 01:46:56PM +0300, Dan Carpenter wrote:
> >> Hello Dinh Nguyen,
> >>
> >> The patch 8d736d8a9c44: "usb: dwc2: gadget: Do not fail probe if
> >> there isn't a clock node" from Nov 11, 2014, leads to the following
> >> static checker warning:
> >>
> >> drivers/usb/dwc2/gadget.c:3436 dwc2_gadget_init()
> >> warn: passing zero to 'PTR_ERR'
> >>
> >> drivers/usb/dwc2/gadget.c
> >> 3432 hsotg->clk = devm_clk_get(dev, "otg");
> >> 3433 if (IS_ERR(hsotg->clk)) {
> >> 3434 hsotg->clk = NULL;
> >>
> >> You need to preserve the error code. NULL means zero means success.
> >>
> >
> > Oh, wait. You are returning success deliberately? Just "return 0;"
> > in that case instead of obfuscating it this way. But shouldn't we
> > continue with the rest of the function anyway? This patch is confusing
> > to me.
>
> Yes, I believe that we can remove the return, and that was Felipe's comment.
my comment was to se clk to NULL because clk API is safe against those.
Then we avoid adding tons of IS_ERR(clk) all over the driver. Returning
is definitely wrong.
> >> 3435 dev_err(dev, "cannot get otg clock\n");
> >
> > Do we need to print this error if it's a success path?
> >
>
> A perhaps a follow up patch to something like this?
>
>
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -3451,8 +3451,7 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int
> irq)
> hsotg->clk = devm_clk_get(dev, "otg");
> if (IS_ERR(hsotg->clk)) {
> hsotg->clk = NULL;
> - dev_err(dev, "cannot get otg clock\n");
> - return PTR_ERR(hsotg->clk);
> + dev_dbg(dev, "cannot get otg clock\n");
> }
>
> hsotg->gadget.max_speed = USB_SPEED_HIGH;
> @@ -3461,7 +3460,12 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg,
> int irq)
>
> /* reset the system */
>
> - clk_prepare_enable(hsotg->clk);
> + ret = clk_prepare_enable(hsotg->clk);
> + if (ret) {
> + dev_err(dev, "failed to enable otg clk\n");
> + goto err_clk;
> + }
makes sense to me.
--
balbi
Attachment:
signature.asc
Description: Digital signature
