| Message ID | 1347872703-15167-1-git-send-email-richard.zhao@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Richard Zhao <richard.zhao@freescale.com> writes: > Signed-off-by: Richard Zhao <richard.zhao@freescale.com> > Acked-by: Felipe Balbi <balbi@ti.com> Felipe said, > if you add a commit log you can add my: > > Acked-by: Felipe Balbi <balbi@ti.com> but the commit message is still totally missing. I would like to ask you to pay attention to the commit messages in the patches that you submit. They should explain the problem that your patch is solving, how you are solving it and why, so that anyone immediately knows what the patch is about without digging up mailing list conversations. There's also a nice blog entry [1] on how to write good commit messages. [1] http://who-t.blogspot.com/2009/12/on-commit-messages.html > --- > Changes from v2: > - assign host/gadget in mxs_phy_set_host/mxs_phy_set_peripheral > > drivers/usb/otg/mxs-phy.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/drivers/usb/otg/mxs-phy.c b/drivers/usb/otg/mxs-phy.c > index 88db976..3255112 100644 > --- a/drivers/usb/otg/mxs-phy.c > +++ b/drivers/usb/otg/mxs-phy.c > @@ -129,12 +129,28 @@ static int mxs_phy_on_disconnect(struct usb_phy *phy, int port) > return 0; > } > > +static int mxs_phy_set_host(struct usb_otg *otg, struct usb_bus *host) > +{ > + otg->host = host; > + > + return 0; > +} > + > +static int mxs_phy_set_peripheral(struct usb_otg *otg, > + struct usb_gadget *gadget) > +{ > + otg->gadget = gadget; > + > + return 0; > +} > + > static int mxs_phy_probe(struct platform_device *pdev) > { > struct resource *res; > void __iomem *base; > struct clk *clk; > struct mxs_phy *mxs_phy; > + struct usb_otg *otg; > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > if (!res) { > @@ -171,6 +187,15 @@ static int mxs_phy_probe(struct platform_device *pdev) > > mxs_phy->clk = clk; > > + otg = devm_kzalloc(&pdev->dev, sizeof(struct usb_otg), GFP_KERNEL); > + if (!otg) > + return -ENOMEM; > + otg->phy = &mxs_phy->phy; > + otg->set_host = mxs_phy_set_host; > + otg->set_peripheral = mxs_phy_set_peripheral; > + > + mxs_phy->phy.otg = otg; > + > platform_set_drvdata(pdev, &mxs_phy->phy); > > return 0; > -- > 1.7.9.5
The purpose for this patch is to have set_peripheral implementation, but in current chipidea code, we don't need to know gadget at all, as when id switch occurs, the core code know its role (device or host) very well, and will call related stop/start function. We have already many code bind struct otg with struct phy, we'd better split them at later code. A better approach to fix this problem is to either not call set_peripheral if both device and host use chipidea driver, or implement otg struct at chipidea driver. > Changes from v2: > - assign host/gadget in mxs_phy_set_host/mxs_phy_set_peripheral > > drivers/usb/otg/mxs-phy.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/drivers/usb/otg/mxs-phy.c b/drivers/usb/otg/mxs-phy.c > index 88db976..3255112 100644 > --- a/drivers/usb/otg/mxs-phy.c > +++ b/drivers/usb/otg/mxs-phy.c > @@ -129,12 +129,28 @@ static int mxs_phy_on_disconnect(struct usb_phy > *phy, int port) > return 0; > } > > +static int mxs_phy_set_host(struct usb_otg *otg, struct usb_bus *host) > +{ > + otg->host = host; > + > + return 0; > +} > + > +static int mxs_phy_set_peripheral(struct usb_otg *otg, > + struct usb_gadget *gadget) > +{ > + otg->gadget = gadget; > + > + return 0; > +} > + > static int mxs_phy_probe(struct platform_device *pdev) > { > struct resource *res; > void __iomem *base; > struct clk *clk; > struct mxs_phy *mxs_phy; > + struct usb_otg *otg; > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > if (!res) { > @@ -171,6 +187,15 @@ static int mxs_phy_probe(struct platform_device > *pdev) > > mxs_phy->clk = clk; > > + otg = devm_kzalloc(&pdev->dev, sizeof(struct usb_otg), GFP_KERNEL); > + if (!otg) > + return -ENOMEM; > + otg->phy = &mxs_phy->phy; > + otg->set_host = mxs_phy_set_host; > + otg->set_peripheral = mxs_phy_set_peripheral; > + > + mxs_phy->phy.otg = otg; > + > platform_set_drvdata(pdev, &mxs_phy->phy); > > return 0; > -- > 1.7.9.5
Chen Peter-B29397 <B29397@freescale.com> writes: > The purpose for this patch is to have set_peripheral implementation, > but in current chipidea code, we don't need to know gadget at all, > as when id switch occurs, the core code know its role (device or host) > very well, and will call related stop/start function. > > We have already many code bind struct otg with struct phy, we'd better > split them at later code. > > A better approach to fix this problem is to either not call set_peripheral > if both device and host use chipidea driver, or implement otg struct > at chipidea driver. Yes, I think the otg driver should be part of the chipidea driver, the only concern is the msm, although it can probably be converted once we have an otg driver. I'll have a look at it soon unless someone beats me to it. The bigger plan was to implement a generic otg framework and base the chipidea's otg driver on that, instead of dragging in one more state machine and whatnot. Regards, -- Alex
> > > The purpose for this patch is to have set_peripheral implementation, > > but in current chipidea code, we don't need to know gadget at all, > > as when id switch occurs, the core code know its role (device or host) > > very well, and will call related stop/start function. > > > > We have already many code bind struct otg with struct phy, we'd better > > split them at later code. > > > > A better approach to fix this problem is to either not call > set_peripheral > > if both device and host use chipidea driver, or implement otg struct > > at chipidea driver. > > Yes, I think the otg driver should be part of the chipidea driver, the > only concern is the msm, although it can probably be converted once we > have an otg driver. I'll have a look at it soon unless someone beats me > to it. Currently, we still have no auto-suspend and wakeup support, but msm supports them, it may need much effort to move msm to current whole chipidea framework. As it affects we go on implement id-switch function and other usb functions at chipidea, I hope we can have it soon, thanks. I also would like to help it if you are busy on other things. > > The bigger plan was to implement a generic otg framework and base the > chipidea's otg driver on that, instead of dragging in one more state > machine and whatnot. > Yes, we can transfer it to use generic otg framework once it is ready, but first, it is better has own otg driver at chipidea. > Regards, > -- > Alex
On Mon, Sep 17, 2012 at 01:06:21PM +0300, Alexander Shishkin wrote: > Richard Zhao <richard.zhao@freescale.com> writes: > > > Signed-off-by: Richard Zhao <richard.zhao@freescale.com> > > Acked-by: Felipe Balbi <balbi@ti.com> > > Felipe said, > > > if you add a commit log you can add my: > > > > Acked-by: Felipe Balbi <balbi@ti.com> > > but the commit message is still totally missing. I would like to ask you > to pay attention to the commit messages in the patches that you > submit. They should explain the problem that your patch is solving, how > you are solving it and why, so that anyone immediately knows what the > patch is about without digging up mailing list conversations. There's > also a nice blog entry [1] on how to write good commit messages. > > [1] http://who-t.blogspot.com/2009/12/on-commit-messages.html Will you agree to give ack if commit message changed to below? USB: mxs-phy: add fake otg support It has nothing to do with phy driver, but for now chipidea driver calls struct usb_otg callbacks. So we implement a fake one. When chipidea implement its own otg driver, this commit can be reverted. Signed-off-by: Richard Zhao <richard.zhao@freescale.com> Acked-by: Felipe Balbi <balbi@ti.com Thanks Richard > > > --- > > Changes from v2: > > - assign host/gadget in mxs_phy_set_host/mxs_phy_set_peripheral > > > > drivers/usb/otg/mxs-phy.c | 25 +++++++++++++++++++++++++ > > 1 file changed, 25 insertions(+) > > > > diff --git a/drivers/usb/otg/mxs-phy.c b/drivers/usb/otg/mxs-phy.c > > index 88db976..3255112 100644 > > --- a/drivers/usb/otg/mxs-phy.c > > +++ b/drivers/usb/otg/mxs-phy.c > > @@ -129,12 +129,28 @@ static int mxs_phy_on_disconnect(struct usb_phy *phy, int port) > > return 0; > > } > > > > +static int mxs_phy_set_host(struct usb_otg *otg, struct usb_bus *host) > > +{ > > + otg->host = host; > > + > > + return 0; > > +} > > + > > +static int mxs_phy_set_peripheral(struct usb_otg *otg, > > + struct usb_gadget *gadget) > > +{ > > + otg->gadget = gadget; > > + > > + return 0; > > +} > > + > > static int mxs_phy_probe(struct platform_device *pdev) > > { > > struct resource *res; > > void __iomem *base; > > struct clk *clk; > > struct mxs_phy *mxs_phy; > > + struct usb_otg *otg; > > > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > if (!res) { > > @@ -171,6 +187,15 @@ static int mxs_phy_probe(struct platform_device *pdev) > > > > mxs_phy->clk = clk; > > > > + otg = devm_kzalloc(&pdev->dev, sizeof(struct usb_otg), GFP_KERNEL); > > + if (!otg) > > + return -ENOMEM; > > + otg->phy = &mxs_phy->phy; > > + otg->set_host = mxs_phy_set_host; > > + otg->set_peripheral = mxs_phy_set_peripheral; > > + > > + mxs_phy->phy.otg = otg; > > + > > platform_set_drvdata(pdev, &mxs_phy->phy); > > > > return 0; > > -- > > 1.7.9.5 >
On 09/19/2012 03:11 AM, Richard Zhao wrote: > On Mon, Sep 17, 2012 at 01:06:21PM +0300, Alexander Shishkin wrote: >> Richard Zhao <richard.zhao@freescale.com> writes: >> >>> Signed-off-by: Richard Zhao <richard.zhao@freescale.com> >>> Acked-by: Felipe Balbi <balbi@ti.com> >> >> Felipe said, >> >>> if you add a commit log you can add my: >>> >>> Acked-by: Felipe Balbi <balbi@ti.com> >> >> but the commit message is still totally missing. I would like to ask you >> to pay attention to the commit messages in the patches that you >> submit. They should explain the problem that your patch is solving, how >> you are solving it and why, so that anyone immediately knows what the >> patch is about without digging up mailing list conversations. There's >> also a nice blog entry [1] on how to write good commit messages. >> >> [1] http://who-t.blogspot.com/2009/12/on-commit-messages.html > Will you agree to give ack if commit message changed to below? > > USB: mxs-phy: add fake otg support > > It has nothing to do with phy driver, but for now chipidea driver calls > struct usb_otg callbacks. So we implement a fake one. > > When chipidea implement its own otg driver, this commit can be reverted. > > Signed-off-by: Richard Zhao <richard.zhao@freescale.com> > Acked-by: Felipe Balbi <balbi@ti.com > > Thanks > Richard >> >>> --- >>> Changes from v2: >>> - assign host/gadget in mxs_phy_set_host/mxs_phy_set_peripheral >>> >>> drivers/usb/otg/mxs-phy.c | 25 +++++++++++++++++++++++++ >>> 1 file changed, 25 insertions(+) >>> >>> diff --git a/drivers/usb/otg/mxs-phy.c b/drivers/usb/otg/mxs-phy.c >>> index 88db976..3255112 100644 >>> --- a/drivers/usb/otg/mxs-phy.c >>> +++ b/drivers/usb/otg/mxs-phy.c >>> @@ -129,12 +129,28 @@ static int mxs_phy_on_disconnect(struct usb_phy *phy, int port) >>> return 0; >>> } >>> >>> +static int mxs_phy_set_host(struct usb_otg *otg, struct usb_bus *host) >>> +{ >>> + otg->host = host; >>> + >>> + return 0; >>> +} >>> + >>> +static int mxs_phy_set_peripheral(struct usb_otg *otg, >>> + struct usb_gadget *gadget) >>> +{ >>> + otg->gadget = gadget; >>> + >>> + return 0; >>> +} >>> + >>> static int mxs_phy_probe(struct platform_device *pdev) >>> { >>> struct resource *res; >>> void __iomem *base; >>> struct clk *clk; >>> struct mxs_phy *mxs_phy; >>> + struct usb_otg *otg; >>> >>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> if (!res) { >>> @@ -171,6 +187,15 @@ static int mxs_phy_probe(struct platform_device *pdev) >>> >>> mxs_phy->clk = clk; >>> >>> + otg = devm_kzalloc(&pdev->dev, sizeof(struct usb_otg), GFP_KERNEL); In order to save some overhead you can embed the struct usb_otg in the struct mxs_phy. Marc >>> + if (!otg) >>> + return -ENOMEM; >>> + otg->phy = &mxs_phy->phy; >>> + otg->set_host = mxs_phy_set_host; >>> + otg->set_peripheral = mxs_phy_set_peripheral; >>> + >>> + mxs_phy->phy.otg = otg; >>> + >>> platform_set_drvdata(pdev, &mxs_phy->phy); >>> >>> return 0; >>> -- >>> 1.7.9.5 >> >
diff --git a/drivers/usb/otg/mxs-phy.c b/drivers/usb/otg/mxs-phy.c index 88db976..3255112 100644 --- a/drivers/usb/otg/mxs-phy.c +++ b/drivers/usb/otg/mxs-phy.c @@ -129,12 +129,28 @@ static int mxs_phy_on_disconnect(struct usb_phy *phy, int port) return 0; } +static int mxs_phy_set_host(struct usb_otg *otg, struct usb_bus *host) +{ + otg->host = host; + + return 0; +} + +static int mxs_phy_set_peripheral(struct usb_otg *otg, + struct usb_gadget *gadget) +{ + otg->gadget = gadget; + + return 0; +} + static int mxs_phy_probe(struct platform_device *pdev) { struct resource *res; void __iomem *base; struct clk *clk; struct mxs_phy *mxs_phy; + struct usb_otg *otg; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (!res) { @@ -171,6 +187,15 @@ static int mxs_phy_probe(struct platform_device *pdev) mxs_phy->clk = clk; + otg = devm_kzalloc(&pdev->dev, sizeof(struct usb_otg), GFP_KERNEL); + if (!otg) + return -ENOMEM; + otg->phy = &mxs_phy->phy; + otg->set_host = mxs_phy_set_host; + otg->set_peripheral = mxs_phy_set_peripheral; + + mxs_phy->phy.otg = otg; + platform_set_drvdata(pdev, &mxs_phy->phy); return 0;