[PATCH v3] USB: mxs-phy: add basic otg support

[PATCH v3] USB: mxs-phy: add basic otg support

[Richard Zhao]

Signed-off-by: Richard Zhao <richard.zhao@freescale.com>
Acked-by: Felipe Balbi <balbi@ti.com>
---
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

[PATCH v3] USB: mxs-phy: add basic otg support

[Alexander Shishkin]

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

[PATCH v3] USB: mxs-phy: add basic otg support

[Richard Zhao]

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
> 

[PATCH v3] USB: mxs-phy: add basic otg support

[Marc Kleine-Budde]

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
>>
> 


-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 259 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120919/170e54b2/attachment.sig>

[PATCH v3] USB: mxs-phy: add basic otg support

[Chen Peter-B29397]

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

[PATCH v3] USB: mxs-phy: add basic otg support

[Alexander Shishkin]

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

[PATCH v3] USB: mxs-phy: add basic otg support

[Chen Peter-B29397]

 
> 
> > 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