by Paul Zimmerman

[permalink] [raw]

Subject: RE: [PATCHv7 6/8] usb: dwc2: gadget: Do not fail probe if there isn't a clock node

> From: [email protected] [mailto:[email protected]]
> Sent: Tuesday, November 11, 2014 9:14 AM
>
> From: Dinh Nguyen <[email protected]>
>
> Since the dwc2 hcd driver is currently not looking for a clock node during
> init, we should not completely fail if there isn't a clock provided.
> By assigning clk = NULL, this allows the driver, when configured for dual-role
> mode, to be able to continue loading the host portion of the driver when
> a clock node is not specified.
>
> Signed-off-by: Dinh Nguyen <[email protected]>
> ---
> v7: Reworked to use clk=NULL and remove the need to is IS_ERR(clk)
> v6: none
> v5: reworked to not access gadget functions from the hcd.
> ---
> drivers/usb/dwc2/gadget.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 37c7916..367689b 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -3431,6 +3431,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);
> }

Whoops, you just broke the return value, since you NULL out
hsotg->clk and then call PTR_ERR() on it.

--
Paul

2014-11-14 21:09:16

by Felipe Balbi

[permalink] [raw]

Subject: Re: [PATCHv7 6/8] usb: dwc2: gadget: Do not fail probe if there isn't a clock node

On Fri, Nov 14, 2014 at 09:01:49PM +0000, Paul Zimmerman wrote:
> > From: [email protected] [mailto:[email protected]]
> > Sent: Tuesday, November 11, 2014 9:14 AM
> >
> > From: Dinh Nguyen <[email protected]>
> >
> > Since the dwc2 hcd driver is currently not looking for a clock node during
> > init, we should not completely fail if there isn't a clock provided.
> > By assigning clk = NULL, this allows the driver, when configured for dual-role
> > mode, to be able to continue loading the host portion of the driver when
> > a clock node is not specified.
> >
> > Signed-off-by: Dinh Nguyen <[email protected]>
> > ---
> > v7: Reworked to use clk=NULL and remove the need to is IS_ERR(clk)
> > v6: none
> > v5: reworked to not access gadget functions from the hcd.
> > ---
> > drivers/usb/dwc2/gadget.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> > index 37c7916..367689b 100644
> > --- a/drivers/usb/dwc2/gadget.c
> > +++ b/drivers/usb/dwc2/gadget.c
> > @@ -3431,6 +3431,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);
> > }
>
> Whoops, you just broke the return value, since you NULL out
> hsotg->clk and then call PTR_ERR() on it.

yeah, the idea was to remove the early return too :-)

--
balbi

Attachments:

(No filename) (1.51 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2014-11-21 02:20:25

by Roy

[permalink] [raw]

Subject: Re: [PATCHv7 1/8] usb: dwc2: Update the gadget driver to use common dwc2_hsotg structure

Hi Dinh:

On 2014/11/12 1:13, [email protected] wrote:
> From: Dinh Nguyen <[email protected]>
>
> Adds the gadget data structure and appropriate data structure pointers
> to the common dwc2_hsotg data structure. To keep the driver data
> dereference code looking clean, the gadget variable declares are only available
> for peripheral and dual-role mode. This is needed so that the dwc2_hsotg data
> structure can be used by the hcd and gadget drivers.
>
> Updates gadget.c to use the dwc2_hsotg data structure and gadget pointers
> that have been moved into the common dwc2_hsotg structure.
>
> Signed-off-by: Dinh Nguyen <[email protected]>
> Signed-off-by: Paul Zimmerman <[email protected]>
> ---
> v7: Addressed comments from Felipe Balbi on which variables should not be just
> limited to gadget(phy, uphy, regulator_bulk_data_supplies, irq, clk,
> debug_root, debug_file, and debug_fifo).
I hope your dual-roal patches could bring the intializtion code of
hsotg->clk and hsotg->phy out from dwc2_gadget_init() into
dwc2_driver_probe() like intializtion of irq and regs base address.
Because the host-only platforms may also need clk gating when enter
suspend for power saving.
> v6: None
> v5: Keep the changes to mininum and maintain hcd and gadget driver to build
> and work separately. Use IS_ENABLED() instead of #if defined
> v3: Updated with paulz's suggestion to avoid double pointers.
> v2: Left the function parameter name as 'hsotg' and just changed its type.
>

--
--------
Roy Li @ Rockchip