RE: [PATCH v4 9/9] usb: chipidea: add support to the generic PHY framework in ChipIdea
> On Fri, Sep 12, 2014 at 06:28:29PM +0800, Peter Chen wrote:
> > On Fri, Sep 12, 2014 at 11:35:50AM +0200, Antoine Tenart wrote:
> > > On Fri, Sep 12, 2014 at 05:27:13PM +0800, Peter Chen wrote:
> > > > On Fri, Sep 12, 2014 at 10:21:25AM +0200, Antoine Tenart wrote:
> > > > > Peter,
> > > > >
> > > > > On Fri, Sep 12, 2014 at 01:10:33AM +, Peter Chen wrote:
> > > > > > > On Thu, Sep 11, 2014 at 08:54:47AM +0800, Peter Chen wrote:
> > > > > > > > On Wed, Sep 03, 2014 at 09:40:40AM +0200, Antoine Tenart wrote:
> > > > > > > > > @@ -595,23 +639,27 @@ static int ci_hdrc_probe(struct
> > > > > > > > > platform_device
> > > > > > > *pdev)
> > > > > > > > > return -ENODEV;
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > - if (ci->platdata->usb_phy)
> > > > > > > > > + if (ci->platdata->phy)
> > > > > > > > > + ci->phy = ci->platdata->phy;
> > > > > > > > > + else if (ci->platdata->usb_phy)
> > > > > > > > > ci->usb_phy = ci->platdata->usb_phy;
> > > > > > > > > else
> > > > > > > > > - ci->usb_phy = devm_usb_get_phy(dev,
> USB_PHY_TYPE_USB2);
> > > > > > > > > + ci->phy = devm_phy_get(dev, "usb-phy");
> > > > > > > > >
> > > > > > > > > - if (IS_ERR(ci->usb_phy)) {
> > > > > > > > > - ret = PTR_ERR(ci->usb_phy);
> > > > > > > > > + if (IS_ERR(ci->phy) || (ci->phy == NULL && ci->usb_phy
> > > > > > > > > +== NULL)) {
> > > > > > > > > /*
> > > > > > > > >* if -ENXIO is returned, it means PHY layer
> > > > > > > > > wasn't
> > > > > > > > >* enabled, so it makes no sense to return -
> EPROBE_DEFER
> > > > > > > > >* in that case, since no PHY driver will ever
> > > > > > > > > probe.
> > > > > > > > >*/
> > > > > > > > > - if (ret == -ENXIO)
> > > > > > > > > - return ret;
> > > > > > > > > + if (PTR_ERR(ci->phy) == -ENXIO)
> > > > > > > > > + return -ENXIO;
> > > > > > > > >
> > > > > > > > > - dev_err(dev, "no usb2 phy configured\n");
> > > > > > > > > - return -EPROBE_DEFER;
> > > > > > > > > + ci->usb_phy = devm_usb_get_phy(dev,
> USB_PHY_TYPE_USB2);
> > > > > > > > > + if (IS_ERR(ci->usb_phy)) {
> > > > > > > > > + dev_err(dev, "no usb2 phy
> configured\n");
> > > > > > > > > + return -EPROBE_DEFER;
> > > > > > > > > + }
> > > > > > > > > }
> > > > > > > >
> > > > > > > > Sorry, I can't accept this change, why
> > > > > > > > devm_usb_get_phy(dev,
> > > > > > > > USB_PHY_TYPE_USB2) is put at error path? Since current get
> > > > > > > > PHY operation is a little complicated, we may have a
> > > > > > > > dedicate function to do it,
> > > > > > > dwc3 driver is a good example.
> > > > > > >
> > > > > > > It's not the error path, it's the case when there is no PHY
> > > > > > > from the generic PHY framework available. Getting an USB PHY is a
> fallback solution.
> > > > > > >
> > > > > > > I agree we can move this to a dedicated function. But even
> > > > > > > if doing so, we'll have to test ci->phy first.
> > > > > > >
> > > > > > > Or do you have something else in mind?
> > > > > > >
> > > > > >
> > > > > > I still want devm_usb_get_phy(dev, USB_PHY_TYPE_USB2) to be
> > > > > > called at the same place like generic_phy, not in later error path,
> > > > > > in
> error path, we only handle error.
> > > > >
> > > > > Would this fit you?
> > > > >
> > > > > if (ci->platdata->phy)
> > > > > ci->phy = ci->platdata->phy; else if
> > > > > (ci->platdata->usb_phy)
> > > > > ci->usb_phy = ci->platdata->usb_phy;
> > > > >
> > > > > if (ci->phy == NULL && ci->usb_phy == NULL) {
> > > >
> > > > You should use else if here.
> > >
> > > Yes, sure.
> > >
> > > >
> > > > > ci->phy = devm_phy_get(dev, "usb-phy");
> > > > > ci->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> > > > >
> > > > > /* if both generic PHY and USB PHY layers aren't enabled */
> > > > > if (PTR_ERR(ci->phy) == -ENOSYS && PTR_ERR(ci->usb_phy) == -
> ENXIO)
> > > > > return -ENXIO;
> > > > >
> > > > > if (IS_ERR(ci->phy) && IS_ERR(ci->usb_phy))
> > > > > return -EPROBE_DEFER;
> > > > >
> > > > > /* if an usb_phy is available, but no phy is there */
> > > > > if (PTR_ERR(ci->phy) == -ENODEV)
> > > > > ci->phy = NULL;
> > > > if (ci->phy)
> > > > ci->usb_phy = NULL;
> > > > if (ci->usb_phy)
> > > > ci->phy = NULL;
> > > >
> > > > How about above?
> > >
> > > When no PHY or USB PHY is found, -ENODEV is returned. Doing the
> > > above would always end up having ci->phy and ci->usb_phy being NULL.
> >
> > When we try to get generic phy, if -ENODEV is returned, the
> > ci->usb_phy should not be NULL, otherwise, your the
Re: [PATCH v4 9/9] usb: chipidea: add support to the generic PHY framework in ChipIdea
On Fri, Sep 12, 2014 at 06:28:29PM +0800, Peter Chen wrote:
> On Fri, Sep 12, 2014 at 11:35:50AM +0200, Antoine Tenart wrote:
> > On Fri, Sep 12, 2014 at 05:27:13PM +0800, Peter Chen wrote:
> > > On Fri, Sep 12, 2014 at 10:21:25AM +0200, Antoine Tenart wrote:
> > > > Peter,
> > > >
> > > > On Fri, Sep 12, 2014 at 01:10:33AM +, Peter Chen wrote:
> > > > > > On Thu, Sep 11, 2014 at 08:54:47AM +0800, Peter Chen wrote:
> > > > > > > On Wed, Sep 03, 2014 at 09:40:40AM +0200, Antoine Tenart wrote:
> > > > > > > > @@ -595,23 +639,27 @@ static int ci_hdrc_probe(struct
> > > > > > > > platform_device
> > > > > > *pdev)
> > > > > > > > return -ENODEV;
> > > > > > > > }
> > > > > > > >
> > > > > > > > - if (ci->platdata->usb_phy)
> > > > > > > > + if (ci->platdata->phy)
> > > > > > > > + ci->phy = ci->platdata->phy;
> > > > > > > > + else if (ci->platdata->usb_phy)
> > > > > > > > ci->usb_phy = ci->platdata->usb_phy;
> > > > > > > > else
> > > > > > > > - ci->usb_phy = devm_usb_get_phy(dev,
> > > > > > > > USB_PHY_TYPE_USB2);
> > > > > > > > + ci->phy = devm_phy_get(dev, "usb-phy");
> > > > > > > >
> > > > > > > > - if (IS_ERR(ci->usb_phy)) {
> > > > > > > > - ret = PTR_ERR(ci->usb_phy);
> > > > > > > > + if (IS_ERR(ci->phy) || (ci->phy == NULL && ci->usb_phy
> > > > > > > > == NULL)) {
> > > > > > > > /*
> > > > > > > > * if -ENXIO is returned, it means PHY layer
> > > > > > > > wasn't
> > > > > > > > * enabled, so it makes no sense to return
> > > > > > > > -EPROBE_DEFER
> > > > > > > > * in that case, since no PHY driver will ever
> > > > > > > > probe.
> > > > > > > > */
> > > > > > > > - if (ret == -ENXIO)
> > > > > > > > - return ret;
> > > > > > > > + if (PTR_ERR(ci->phy) == -ENXIO)
> > > > > > > > + return -ENXIO;
> > > > > > > >
> > > > > > > > - dev_err(dev, "no usb2 phy configured\n");
> > > > > > > > - return -EPROBE_DEFER;
> > > > > > > > + ci->usb_phy = devm_usb_get_phy(dev,
> > > > > > > > USB_PHY_TYPE_USB2);
> > > > > > > > + if (IS_ERR(ci->usb_phy)) {
> > > > > > > > + dev_err(dev, "no usb2 phy
> > > > > > > > configured\n");
> > > > > > > > + return -EPROBE_DEFER;
> > > > > > > > + }
> > > > > > > > }
> > > > > > >
> > > > > > > Sorry, I can't accept this change, why devm_usb_get_phy(dev,
> > > > > > > USB_PHY_TYPE_USB2) is put at error path? Since current get PHY
> > > > > > > operation is a little complicated, we may have a dedicate
> > > > > > > function to do it,
> > > > > > dwc3 driver is a good example.
> > > > > >
> > > > > > It's not the error path, it's the case when there is no PHY from
> > > > > > the generic PHY
> > > > > > framework available. Getting an USB PHY is a fallback solution.
> > > > > >
> > > > > > I agree we can move this to a dedicated function. But even if doing
> > > > > > so, we'll
> > > > > > have to test ci->phy first.
> > > > > >
> > > > > > Or do you have something else in mind?
> > > > > >
> > > > >
> > > > > I still want devm_usb_get_phy(dev, USB_PHY_TYPE_USB2) to be called at
> > > > > the same place
> > > > > like generic_phy, not in later error path, in error path, we only
> > > > > handle error.
> > > >
> > > > Would this fit you?
> > > >
> > > > if (ci->platdata->phy)
> > > > ci->phy = ci->platdata->phy;
> > > > else if (ci->platdata->usb_phy)
> > > > ci->usb_phy = ci->platdata->usb_phy;
> > > >
> > > > if (ci->phy == NULL && ci->usb_phy == NULL) {
> > >
> > > You should use else if here.
> >
> > Yes, sure.
> >
> > >
> > > > ci->phy = devm_phy_get(dev, "usb-phy");
> > > > ci->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> > > >
> > > > /* if both generic PHY and USB PHY layers aren't enabled */
> > > > if (PTR_ERR(ci->phy) == -ENOSYS && PTR_ERR(ci->usb_phy) ==
> > > > -ENXIO)
> > > > return -ENXIO;
> > > >
> > > > if (IS_ERR(ci->phy) && IS_ERR(ci->usb_phy))
> > > > return -EPROBE_DEFER;
> > > >
> > > > /* if an usb_phy is available, but no phy is there */
> > > > if (PTR_ERR(ci->phy) == -ENODEV)
> > > > ci->phy = NULL;
> > > if (ci->phy)
> > > ci->usb_phy = NULL;
> > > if (ci->usb_phy)
> > > ci->phy = NULL;
> > >
> > > How about above?
> >
> > When no PHY or USB PHY is found, -ENODEV is returned. Doing the above
> > would always end up having ci->phy and ci->usb_phy being NULL.
>
> When we try to get generic phy, if -ENODEV is returned, the ci->usb_phy
> should not be NULL, otherwise, your the second condition will return
> -EPROBE_DEFER, it means when the code goes
Re: [PATCH v4 9/9] usb: chipidea: add support to the generic PHY framework in ChipIdea
On Fri, Sep 12, 2014 at 11:35:50AM +0200, Antoine Tenart wrote:
> On Fri, Sep 12, 2014 at 05:27:13PM +0800, Peter Chen wrote:
> > On Fri, Sep 12, 2014 at 10:21:25AM +0200, Antoine Tenart wrote:
> > > Peter,
> > >
> > > On Fri, Sep 12, 2014 at 01:10:33AM +, Peter Chen wrote:
> > > > > On Thu, Sep 11, 2014 at 08:54:47AM +0800, Peter Chen wrote:
> > > > > > On Wed, Sep 03, 2014 at 09:40:40AM +0200, Antoine Tenart wrote:
> > > > > > > @@ -595,23 +639,27 @@ static int ci_hdrc_probe(struct
> > > > > > > platform_device
> > > > > *pdev)
> > > > > > > return -ENODEV;
> > > > > > > }
> > > > > > >
> > > > > > > - if (ci->platdata->usb_phy)
> > > > > > > + if (ci->platdata->phy)
> > > > > > > + ci->phy = ci->platdata->phy;
> > > > > > > + else if (ci->platdata->usb_phy)
> > > > > > > ci->usb_phy = ci->platdata->usb_phy;
> > > > > > > else
> > > > > > > - ci->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> > > > > > > + ci->phy = devm_phy_get(dev, "usb-phy");
> > > > > > >
> > > > > > > - if (IS_ERR(ci->usb_phy)) {
> > > > > > > - ret = PTR_ERR(ci->usb_phy);
> > > > > > > + if (IS_ERR(ci->phy) || (ci->phy == NULL && ci->usb_phy ==
> > > > > > > NULL)) {
> > > > > > > /*
> > > > > > >* if -ENXIO is returned, it means PHY layer wasn't
> > > > > > >* enabled, so it makes no sense to return -EPROBE_DEFER
> > > > > > >* in that case, since no PHY driver will ever probe.
> > > > > > >*/
> > > > > > > - if (ret == -ENXIO)
> > > > > > > - return ret;
> > > > > > > + if (PTR_ERR(ci->phy) == -ENXIO)
> > > > > > > + return -ENXIO;
> > > > > > >
> > > > > > > - dev_err(dev, "no usb2 phy configured\n");
> > > > > > > - return -EPROBE_DEFER;
> > > > > > > + ci->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> > > > > > > + if (IS_ERR(ci->usb_phy)) {
> > > > > > > + dev_err(dev, "no usb2 phy configured\n");
> > > > > > > + return -EPROBE_DEFER;
> > > > > > > + }
> > > > > > > }
> > > > > >
> > > > > > Sorry, I can't accept this change, why devm_usb_get_phy(dev,
> > > > > > USB_PHY_TYPE_USB2) is put at error path? Since current get PHY
> > > > > > operation is a little complicated, we may have a dedicate function
> > > > > > to do it,
> > > > > dwc3 driver is a good example.
> > > > >
> > > > > It's not the error path, it's the case when there is no PHY from the
> > > > > generic PHY
> > > > > framework available. Getting an USB PHY is a fallback solution.
> > > > >
> > > > > I agree we can move this to a dedicated function. But even if doing
> > > > > so, we'll
> > > > > have to test ci->phy first.
> > > > >
> > > > > Or do you have something else in mind?
> > > > >
> > > >
> > > > I still want devm_usb_get_phy(dev, USB_PHY_TYPE_USB2) to be called at
> > > > the same place
> > > > like generic_phy, not in later error path, in error path, we only
> > > > handle error.
> > >
> > > Would this fit you?
> > >
> > > if (ci->platdata->phy)
> > > ci->phy = ci->platdata->phy;
> > > else if (ci->platdata->usb_phy)
> > > ci->usb_phy = ci->platdata->usb_phy;
> > >
> > > if (ci->phy == NULL && ci->usb_phy == NULL) {
> >
> > You should use else if here.
>
> Yes, sure.
>
> >
> > > ci->phy = devm_phy_get(dev, "usb-phy");
> > > ci->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> > >
> > > /* if both generic PHY and USB PHY layers aren't enabled */
> > > if (PTR_ERR(ci->phy) == -ENOSYS && PTR_ERR(ci->usb_phy) == -ENXIO)
> > > return -ENXIO;
> > >
> > > if (IS_ERR(ci->phy) && IS_ERR(ci->usb_phy))
> > > return -EPROBE_DEFER;
> > >
> > > /* if an usb_phy is available, but no phy is there */
> > > if (PTR_ERR(ci->phy) == -ENODEV)
> > > ci->phy = NULL;
> > if (ci->phy)
> > ci->usb_phy = NULL;
> > if (ci->usb_phy)
> > ci->phy = NULL;
> >
> > How about above?
>
> When no PHY or USB PHY is found, -ENODEV is returned. Doing the above
> would always end up having ci->phy and ci->usb_phy being NULL.
When we try to get generic phy, if -ENODEV is returned, the ci->usb_phy
should not be NULL, otherwise, your the second condition will return
-EPROBE_DEFER, it means when the code goes here, there must be one PHY
is found.
>
> We could have:
>
> if (IS_ERR(ci->phy))
> ci->phy = NULL;
> else if (IS_ERR(ci->usb_phy))
> ci->usb_phy = NULL;
>
>
> Antoine
>
> --
> Antoine Ténart, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
--
Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 9/9] usb: chipidea: add support to the generic PHY framework in ChipIdea
On Fri, Sep 12, 2014 at 05:27:13PM +0800, Peter Chen wrote:
> On Fri, Sep 12, 2014 at 10:21:25AM +0200, Antoine Tenart wrote:
> > Peter,
> >
> > On Fri, Sep 12, 2014 at 01:10:33AM +, Peter Chen wrote:
> > > > On Thu, Sep 11, 2014 at 08:54:47AM +0800, Peter Chen wrote:
> > > > > On Wed, Sep 03, 2014 at 09:40:40AM +0200, Antoine Tenart wrote:
> > > > > > @@ -595,23 +639,27 @@ static int ci_hdrc_probe(struct
> > > > > > platform_device
> > > > *pdev)
> > > > > > return -ENODEV;
> > > > > > }
> > > > > >
> > > > > > - if (ci->platdata->usb_phy)
> > > > > > + if (ci->platdata->phy)
> > > > > > + ci->phy = ci->platdata->phy;
> > > > > > + else if (ci->platdata->usb_phy)
> > > > > > ci->usb_phy = ci->platdata->usb_phy;
> > > > > > else
> > > > > > - ci->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> > > > > > + ci->phy = devm_phy_get(dev, "usb-phy");
> > > > > >
> > > > > > - if (IS_ERR(ci->usb_phy)) {
> > > > > > - ret = PTR_ERR(ci->usb_phy);
> > > > > > + if (IS_ERR(ci->phy) || (ci->phy == NULL && ci->usb_phy ==
> > > > > > NULL)) {
> > > > > > /*
> > > > > > * if -ENXIO is returned, it means PHY layer wasn't
> > > > > > * enabled, so it makes no sense to return -EPROBE_DEFER
> > > > > > * in that case, since no PHY driver will ever probe.
> > > > > > */
> > > > > > - if (ret == -ENXIO)
> > > > > > - return ret;
> > > > > > + if (PTR_ERR(ci->phy) == -ENXIO)
> > > > > > + return -ENXIO;
> > > > > >
> > > > > > - dev_err(dev, "no usb2 phy configured\n");
> > > > > > - return -EPROBE_DEFER;
> > > > > > + ci->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> > > > > > + if (IS_ERR(ci->usb_phy)) {
> > > > > > + dev_err(dev, "no usb2 phy configured\n");
> > > > > > + return -EPROBE_DEFER;
> > > > > > + }
> > > > > > }
> > > > >
> > > > > Sorry, I can't accept this change, why devm_usb_get_phy(dev,
> > > > > USB_PHY_TYPE_USB2) is put at error path? Since current get PHY
> > > > > operation is a little complicated, we may have a dedicate function to
> > > > > do it,
> > > > dwc3 driver is a good example.
> > > >
> > > > It's not the error path, it's the case when there is no PHY from the
> > > > generic PHY
> > > > framework available. Getting an USB PHY is a fallback solution.
> > > >
> > > > I agree we can move this to a dedicated function. But even if doing so,
> > > > we'll
> > > > have to test ci->phy first.
> > > >
> > > > Or do you have something else in mind?
> > > >
> > >
> > > I still want devm_usb_get_phy(dev, USB_PHY_TYPE_USB2) to be called at the
> > > same place
> > > like generic_phy, not in later error path, in error path, we only handle
> > > error.
> >
> > Would this fit you?
> >
> > if (ci->platdata->phy)
> > ci->phy = ci->platdata->phy;
> > else if (ci->platdata->usb_phy)
> > ci->usb_phy = ci->platdata->usb_phy;
> >
> > if (ci->phy == NULL && ci->usb_phy == NULL) {
>
> You should use else if here.
Yes, sure.
>
> > ci->phy = devm_phy_get(dev, "usb-phy");
> > ci->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> >
> > /* if both generic PHY and USB PHY layers aren't enabled */
> > if (PTR_ERR(ci->phy) == -ENOSYS && PTR_ERR(ci->usb_phy) == -ENXIO)
> > return -ENXIO;
> >
> > if (IS_ERR(ci->phy) && IS_ERR(ci->usb_phy))
> > return -EPROBE_DEFER;
> >
> > /* if an usb_phy is available, but no phy is there */
> > if (PTR_ERR(ci->phy) == -ENODEV)
> > ci->phy = NULL;
> if (ci->phy)
> ci->usb_phy = NULL;
> if (ci->usb_phy)
> ci->phy = NULL;
>
> How about above?
When no PHY or USB PHY is found, -ENODEV is returned. Doing the above
would always end up having ci->phy and ci->usb_phy being NULL.
We could have:
if (IS_ERR(ci->phy))
ci->phy = NULL;
else if (IS_ERR(ci->usb_phy))
ci->usb_phy = NULL;
Antoine
--
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 9/9] usb: chipidea: add support to the generic PHY framework in ChipIdea
On Fri, Sep 12, 2014 at 10:21:25AM +0200, Antoine Tenart wrote:
> Peter,
>
> On Fri, Sep 12, 2014 at 01:10:33AM +, Peter Chen wrote:
> > > On Thu, Sep 11, 2014 at 08:54:47AM +0800, Peter Chen wrote:
> > > > On Wed, Sep 03, 2014 at 09:40:40AM +0200, Antoine Tenart wrote:
> > > > > @@ -595,23 +639,27 @@ static int ci_hdrc_probe(struct platform_device
> > > *pdev)
> > > > > return -ENODEV;
> > > > > }
> > > > >
> > > > > - if (ci->platdata->usb_phy)
> > > > > + if (ci->platdata->phy)
> > > > > + ci->phy = ci->platdata->phy;
> > > > > + else if (ci->platdata->usb_phy)
> > > > > ci->usb_phy = ci->platdata->usb_phy;
> > > > > else
> > > > > - ci->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> > > > > + ci->phy = devm_phy_get(dev, "usb-phy");
> > > > >
> > > > > - if (IS_ERR(ci->usb_phy)) {
> > > > > - ret = PTR_ERR(ci->usb_phy);
> > > > > + if (IS_ERR(ci->phy) || (ci->phy == NULL && ci->usb_phy ==
> > > > > NULL)) {
> > > > > /*
> > > > >* if -ENXIO is returned, it means PHY layer wasn't
> > > > >* enabled, so it makes no sense to return -EPROBE_DEFER
> > > > >* in that case, since no PHY driver will ever probe.
> > > > >*/
> > > > > - if (ret == -ENXIO)
> > > > > - return ret;
> > > > > + if (PTR_ERR(ci->phy) == -ENXIO)
> > > > > + return -ENXIO;
> > > > >
> > > > > - dev_err(dev, "no usb2 phy configured\n");
> > > > > - return -EPROBE_DEFER;
> > > > > + ci->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> > > > > + if (IS_ERR(ci->usb_phy)) {
> > > > > + dev_err(dev, "no usb2 phy configured\n");
> > > > > + return -EPROBE_DEFER;
> > > > > + }
> > > > > }
> > > >
> > > > Sorry, I can't accept this change, why devm_usb_get_phy(dev,
> > > > USB_PHY_TYPE_USB2) is put at error path? Since current get PHY
> > > > operation is a little complicated, we may have a dedicate function to
> > > > do it,
> > > dwc3 driver is a good example.
> > >
> > > It's not the error path, it's the case when there is no PHY from the
> > > generic PHY
> > > framework available. Getting an USB PHY is a fallback solution.
> > >
> > > I agree we can move this to a dedicated function. But even if doing so,
> > > we'll
> > > have to test ci->phy first.
> > >
> > > Or do you have something else in mind?
> > >
> >
> > I still want devm_usb_get_phy(dev, USB_PHY_TYPE_USB2) to be called at the
> > same place
> > like generic_phy, not in later error path, in error path, we only handle
> > error.
>
> Would this fit you?
>
> if (ci->platdata->phy)
> ci->phy = ci->platdata->phy;
> else if (ci->platdata->usb_phy)
> ci->usb_phy = ci->platdata->usb_phy;
>
> if (ci->phy == NULL && ci->usb_phy == NULL) {
You should use else if here.
> ci->phy = devm_phy_get(dev, "usb-phy");
> ci->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
>
> /* if both generic PHY and USB PHY layers aren't enabled */
> if (PTR_ERR(ci->phy) == -ENOSYS && PTR_ERR(ci->usb_phy) == -ENXIO)
> return -ENXIO;
>
> if (IS_ERR(ci->phy) && IS_ERR(ci->usb_phy))
> return -EPROBE_DEFER;
>
> /* if an usb_phy is available, but no phy is there */
> if (PTR_ERR(ci->phy) == -ENODEV)
> ci->phy = NULL;
if (ci->phy)
ci->usb_phy = NULL;
if (ci->usb_phy)
ci->phy = NULL;
How about above?
--
Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 9/9] usb: chipidea: add support to the generic PHY framework in ChipIdea
Peter,
On Fri, Sep 12, 2014 at 01:10:33AM +, Peter Chen wrote:
> > On Thu, Sep 11, 2014 at 08:54:47AM +0800, Peter Chen wrote:
> > > On Wed, Sep 03, 2014 at 09:40:40AM +0200, Antoine Tenart wrote:
> > > > @@ -595,23 +639,27 @@ static int ci_hdrc_probe(struct platform_device
> > *pdev)
> > > > return -ENODEV;
> > > > }
> > > >
> > > > - if (ci->platdata->usb_phy)
> > > > + if (ci->platdata->phy)
> > > > + ci->phy = ci->platdata->phy;
> > > > + else if (ci->platdata->usb_phy)
> > > > ci->usb_phy = ci->platdata->usb_phy;
> > > > else
> > > > - ci->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> > > > + ci->phy = devm_phy_get(dev, "usb-phy");
> > > >
> > > > - if (IS_ERR(ci->usb_phy)) {
> > > > - ret = PTR_ERR(ci->usb_phy);
> > > > + if (IS_ERR(ci->phy) || (ci->phy == NULL && ci->usb_phy ==
> > > > NULL)) {
> > > > /*
> > > > * if -ENXIO is returned, it means PHY layer wasn't
> > > > * enabled, so it makes no sense to return -EPROBE_DEFER
> > > > * in that case, since no PHY driver will ever probe.
> > > > */
> > > > - if (ret == -ENXIO)
> > > > - return ret;
> > > > + if (PTR_ERR(ci->phy) == -ENXIO)
> > > > + return -ENXIO;
> > > >
> > > > - dev_err(dev, "no usb2 phy configured\n");
> > > > - return -EPROBE_DEFER;
> > > > + ci->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> > > > + if (IS_ERR(ci->usb_phy)) {
> > > > + dev_err(dev, "no usb2 phy configured\n");
> > > > + return -EPROBE_DEFER;
> > > > + }
> > > > }
> > >
> > > Sorry, I can't accept this change, why devm_usb_get_phy(dev,
> > > USB_PHY_TYPE_USB2) is put at error path? Since current get PHY
> > > operation is a little complicated, we may have a dedicate function to do
> > > it,
> > dwc3 driver is a good example.
> >
> > It's not the error path, it's the case when there is no PHY from the
> > generic PHY
> > framework available. Getting an USB PHY is a fallback solution.
> >
> > I agree we can move this to a dedicated function. But even if doing so,
> > we'll
> > have to test ci->phy first.
> >
> > Or do you have something else in mind?
> >
>
> I still want devm_usb_get_phy(dev, USB_PHY_TYPE_USB2) to be called at the
> same place
> like generic_phy, not in later error path, in error path, we only handle
> error.
Would this fit you?
if (ci->platdata->phy)
ci->phy = ci->platdata->phy;
else if (ci->platdata->usb_phy)
ci->usb_phy = ci->platdata->usb_phy;
if (ci->phy == NULL && ci->usb_phy == NULL) {
ci->phy = devm_phy_get(dev, "usb-phy");
ci->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
/* if both generic PHY and USB PHY layers aren't enabled */
if (PTR_ERR(ci->phy) == -ENOSYS && PTR_ERR(ci->usb_phy) == -ENXIO)
return -ENXIO;
if (IS_ERR(ci->phy) && IS_ERR(ci->usb_phy))
return -EPROBE_DEFER;
/* if an usb_phy is available, but no phy is there */
if (PTR_ERR(ci->phy) == -ENODEV)
ci->phy = NULL;
}
Antoine
--
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 9/9] usb: chipidea: add support to the generic PHY framework in ChipIdea
Peter,
On Fri, Sep 12, 2014 at 01:10:33AM +, Peter Chen wrote:
On Thu, Sep 11, 2014 at 08:54:47AM +0800, Peter Chen wrote:
On Wed, Sep 03, 2014 at 09:40:40AM +0200, Antoine Tenart wrote:
@@ -595,23 +639,27 @@ static int ci_hdrc_probe(struct platform_device
*pdev)
return -ENODEV;
}
- if (ci-platdata-usb_phy)
+ if (ci-platdata-phy)
+ ci-phy = ci-platdata-phy;
+ else if (ci-platdata-usb_phy)
ci-usb_phy = ci-platdata-usb_phy;
else
- ci-usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
+ ci-phy = devm_phy_get(dev, usb-phy);
- if (IS_ERR(ci-usb_phy)) {
- ret = PTR_ERR(ci-usb_phy);
+ if (IS_ERR(ci-phy) || (ci-phy == NULL ci-usb_phy ==
NULL)) {
/*
* if -ENXIO is returned, it means PHY layer wasn't
* enabled, so it makes no sense to return -EPROBE_DEFER
* in that case, since no PHY driver will ever probe.
*/
- if (ret == -ENXIO)
- return ret;
+ if (PTR_ERR(ci-phy) == -ENXIO)
+ return -ENXIO;
- dev_err(dev, no usb2 phy configured\n);
- return -EPROBE_DEFER;
+ ci-usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
+ if (IS_ERR(ci-usb_phy)) {
+ dev_err(dev, no usb2 phy configured\n);
+ return -EPROBE_DEFER;
+ }
}
Sorry, I can't accept this change, why devm_usb_get_phy(dev,
USB_PHY_TYPE_USB2) is put at error path? Since current get PHY
operation is a little complicated, we may have a dedicate function to do
it,
dwc3 driver is a good example.
It's not the error path, it's the case when there is no PHY from the
generic PHY
framework available. Getting an USB PHY is a fallback solution.
I agree we can move this to a dedicated function. But even if doing so,
we'll
have to test ci-phy first.
Or do you have something else in mind?
I still want devm_usb_get_phy(dev, USB_PHY_TYPE_USB2) to be called at the
same place
like generic_phy, not in later error path, in error path, we only handle
error.
Would this fit you?
if (ci-platdata-phy)
ci-phy = ci-platdata-phy;
else if (ci-platdata-usb_phy)
ci-usb_phy = ci-platdata-usb_phy;
if (ci-phy == NULL ci-usb_phy == NULL) {
ci-phy = devm_phy_get(dev, usb-phy);
ci-usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
/* if both generic PHY and USB PHY layers aren't enabled */
if (PTR_ERR(ci-phy) == -ENOSYS PTR_ERR(ci-usb_phy) == -ENXIO)
return -ENXIO;
if (IS_ERR(ci-phy) IS_ERR(ci-usb_phy))
return -EPROBE_DEFER;
/* if an usb_phy is available, but no phy is there */
if (PTR_ERR(ci-phy) == -ENODEV)
ci-phy = NULL;
}
Antoine
--
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 9/9] usb: chipidea: add support to the generic PHY framework in ChipIdea
On Fri, Sep 12, 2014 at 10:21:25AM +0200, Antoine Tenart wrote:
Peter,
On Fri, Sep 12, 2014 at 01:10:33AM +, Peter Chen wrote:
On Thu, Sep 11, 2014 at 08:54:47AM +0800, Peter Chen wrote:
On Wed, Sep 03, 2014 at 09:40:40AM +0200, Antoine Tenart wrote:
@@ -595,23 +639,27 @@ static int ci_hdrc_probe(struct platform_device
*pdev)
return -ENODEV;
}
- if (ci-platdata-usb_phy)
+ if (ci-platdata-phy)
+ ci-phy = ci-platdata-phy;
+ else if (ci-platdata-usb_phy)
ci-usb_phy = ci-platdata-usb_phy;
else
- ci-usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
+ ci-phy = devm_phy_get(dev, usb-phy);
- if (IS_ERR(ci-usb_phy)) {
- ret = PTR_ERR(ci-usb_phy);
+ if (IS_ERR(ci-phy) || (ci-phy == NULL ci-usb_phy ==
NULL)) {
/*
* if -ENXIO is returned, it means PHY layer wasn't
* enabled, so it makes no sense to return -EPROBE_DEFER
* in that case, since no PHY driver will ever probe.
*/
- if (ret == -ENXIO)
- return ret;
+ if (PTR_ERR(ci-phy) == -ENXIO)
+ return -ENXIO;
- dev_err(dev, no usb2 phy configured\n);
- return -EPROBE_DEFER;
+ ci-usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
+ if (IS_ERR(ci-usb_phy)) {
+ dev_err(dev, no usb2 phy configured\n);
+ return -EPROBE_DEFER;
+ }
}
Sorry, I can't accept this change, why devm_usb_get_phy(dev,
USB_PHY_TYPE_USB2) is put at error path? Since current get PHY
operation is a little complicated, we may have a dedicate function to
do it,
dwc3 driver is a good example.
It's not the error path, it's the case when there is no PHY from the
generic PHY
framework available. Getting an USB PHY is a fallback solution.
I agree we can move this to a dedicated function. But even if doing so,
we'll
have to test ci-phy first.
Or do you have something else in mind?
I still want devm_usb_get_phy(dev, USB_PHY_TYPE_USB2) to be called at the
same place
like generic_phy, not in later error path, in error path, we only handle
error.
Would this fit you?
if (ci-platdata-phy)
ci-phy = ci-platdata-phy;
else if (ci-platdata-usb_phy)
ci-usb_phy = ci-platdata-usb_phy;
if (ci-phy == NULL ci-usb_phy == NULL) {
You should use else if here.
ci-phy = devm_phy_get(dev, usb-phy);
ci-usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
/* if both generic PHY and USB PHY layers aren't enabled */
if (PTR_ERR(ci-phy) == -ENOSYS PTR_ERR(ci-usb_phy) == -ENXIO)
return -ENXIO;
if (IS_ERR(ci-phy) IS_ERR(ci-usb_phy))
return -EPROBE_DEFER;
/* if an usb_phy is available, but no phy is there */
if (PTR_ERR(ci-phy) == -ENODEV)
ci-phy = NULL;
if (ci-phy)
ci-usb_phy = NULL;
if (ci-usb_phy)
ci-phy = NULL;
How about above?
--
Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 9/9] usb: chipidea: add support to the generic PHY framework in ChipIdea
On Fri, Sep 12, 2014 at 05:27:13PM +0800, Peter Chen wrote:
On Fri, Sep 12, 2014 at 10:21:25AM +0200, Antoine Tenart wrote:
Peter,
On Fri, Sep 12, 2014 at 01:10:33AM +, Peter Chen wrote:
On Thu, Sep 11, 2014 at 08:54:47AM +0800, Peter Chen wrote:
On Wed, Sep 03, 2014 at 09:40:40AM +0200, Antoine Tenart wrote:
@@ -595,23 +639,27 @@ static int ci_hdrc_probe(struct
platform_device
*pdev)
return -ENODEV;
}
- if (ci-platdata-usb_phy)
+ if (ci-platdata-phy)
+ ci-phy = ci-platdata-phy;
+ else if (ci-platdata-usb_phy)
ci-usb_phy = ci-platdata-usb_phy;
else
- ci-usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
+ ci-phy = devm_phy_get(dev, usb-phy);
- if (IS_ERR(ci-usb_phy)) {
- ret = PTR_ERR(ci-usb_phy);
+ if (IS_ERR(ci-phy) || (ci-phy == NULL ci-usb_phy ==
NULL)) {
/*
* if -ENXIO is returned, it means PHY layer wasn't
* enabled, so it makes no sense to return -EPROBE_DEFER
* in that case, since no PHY driver will ever probe.
*/
- if (ret == -ENXIO)
- return ret;
+ if (PTR_ERR(ci-phy) == -ENXIO)
+ return -ENXIO;
- dev_err(dev, no usb2 phy configured\n);
- return -EPROBE_DEFER;
+ ci-usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
+ if (IS_ERR(ci-usb_phy)) {
+ dev_err(dev, no usb2 phy configured\n);
+ return -EPROBE_DEFER;
+ }
}
Sorry, I can't accept this change, why devm_usb_get_phy(dev,
USB_PHY_TYPE_USB2) is put at error path? Since current get PHY
operation is a little complicated, we may have a dedicate function to
do it,
dwc3 driver is a good example.
It's not the error path, it's the case when there is no PHY from the
generic PHY
framework available. Getting an USB PHY is a fallback solution.
I agree we can move this to a dedicated function. But even if doing so,
we'll
have to test ci-phy first.
Or do you have something else in mind?
I still want devm_usb_get_phy(dev, USB_PHY_TYPE_USB2) to be called at the
same place
like generic_phy, not in later error path, in error path, we only handle
error.
Would this fit you?
if (ci-platdata-phy)
ci-phy = ci-platdata-phy;
else if (ci-platdata-usb_phy)
ci-usb_phy = ci-platdata-usb_phy;
if (ci-phy == NULL ci-usb_phy == NULL) {
You should use else if here.
Yes, sure.
ci-phy = devm_phy_get(dev, usb-phy);
ci-usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
/* if both generic PHY and USB PHY layers aren't enabled */
if (PTR_ERR(ci-phy) == -ENOSYS PTR_ERR(ci-usb_phy) == -ENXIO)
return -ENXIO;
if (IS_ERR(ci-phy) IS_ERR(ci-usb_phy))
return -EPROBE_DEFER;
/* if an usb_phy is available, but no phy is there */
if (PTR_ERR(ci-phy) == -ENODEV)
ci-phy = NULL;
if (ci-phy)
ci-usb_phy = NULL;
if (ci-usb_phy)
ci-phy = NULL;
How about above?
When no PHY or USB PHY is found, -ENODEV is returned. Doing the above
would always end up having ci-phy and ci-usb_phy being NULL.
We could have:
if (IS_ERR(ci-phy))
ci-phy = NULL;
else if (IS_ERR(ci-usb_phy))
ci-usb_phy = NULL;
Antoine
--
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 9/9] usb: chipidea: add support to the generic PHY framework in ChipIdea
On Fri, Sep 12, 2014 at 11:35:50AM +0200, Antoine Tenart wrote:
On Fri, Sep 12, 2014 at 05:27:13PM +0800, Peter Chen wrote:
On Fri, Sep 12, 2014 at 10:21:25AM +0200, Antoine Tenart wrote:
Peter,
On Fri, Sep 12, 2014 at 01:10:33AM +, Peter Chen wrote:
On Thu, Sep 11, 2014 at 08:54:47AM +0800, Peter Chen wrote:
On Wed, Sep 03, 2014 at 09:40:40AM +0200, Antoine Tenart wrote:
@@ -595,23 +639,27 @@ static int ci_hdrc_probe(struct
platform_device
*pdev)
return -ENODEV;
}
- if (ci-platdata-usb_phy)
+ if (ci-platdata-phy)
+ ci-phy = ci-platdata-phy;
+ else if (ci-platdata-usb_phy)
ci-usb_phy = ci-platdata-usb_phy;
else
- ci-usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
+ ci-phy = devm_phy_get(dev, usb-phy);
- if (IS_ERR(ci-usb_phy)) {
- ret = PTR_ERR(ci-usb_phy);
+ if (IS_ERR(ci-phy) || (ci-phy == NULL ci-usb_phy ==
NULL)) {
/*
* if -ENXIO is returned, it means PHY layer wasn't
* enabled, so it makes no sense to return -EPROBE_DEFER
* in that case, since no PHY driver will ever probe.
*/
- if (ret == -ENXIO)
- return ret;
+ if (PTR_ERR(ci-phy) == -ENXIO)
+ return -ENXIO;
- dev_err(dev, no usb2 phy configured\n);
- return -EPROBE_DEFER;
+ ci-usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
+ if (IS_ERR(ci-usb_phy)) {
+ dev_err(dev, no usb2 phy configured\n);
+ return -EPROBE_DEFER;
+ }
}
Sorry, I can't accept this change, why devm_usb_get_phy(dev,
USB_PHY_TYPE_USB2) is put at error path? Since current get PHY
operation is a little complicated, we may have a dedicate function
to do it,
dwc3 driver is a good example.
It's not the error path, it's the case when there is no PHY from the
generic PHY
framework available. Getting an USB PHY is a fallback solution.
I agree we can move this to a dedicated function. But even if doing
so, we'll
have to test ci-phy first.
Or do you have something else in mind?
I still want devm_usb_get_phy(dev, USB_PHY_TYPE_USB2) to be called at
the same place
like generic_phy, not in later error path, in error path, we only
handle error.
Would this fit you?
if (ci-platdata-phy)
ci-phy = ci-platdata-phy;
else if (ci-platdata-usb_phy)
ci-usb_phy = ci-platdata-usb_phy;
if (ci-phy == NULL ci-usb_phy == NULL) {
You should use else if here.
Yes, sure.
ci-phy = devm_phy_get(dev, usb-phy);
ci-usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
/* if both generic PHY and USB PHY layers aren't enabled */
if (PTR_ERR(ci-phy) == -ENOSYS PTR_ERR(ci-usb_phy) == -ENXIO)
return -ENXIO;
if (IS_ERR(ci-phy) IS_ERR(ci-usb_phy))
return -EPROBE_DEFER;
/* if an usb_phy is available, but no phy is there */
if (PTR_ERR(ci-phy) == -ENODEV)
ci-phy = NULL;
if (ci-phy)
ci-usb_phy = NULL;
if (ci-usb_phy)
ci-phy = NULL;
How about above?
When no PHY or USB PHY is found, -ENODEV is returned. Doing the above
would always end up having ci-phy and ci-usb_phy being NULL.
When we try to get generic phy, if -ENODEV is returned, the ci-usb_phy
should not be NULL, otherwise, your the second condition will return
-EPROBE_DEFER, it means when the code goes here, there must be one PHY
is found.
We could have:
if (IS_ERR(ci-phy))
ci-phy = NULL;
else if (IS_ERR(ci-usb_phy))
ci-usb_phy = NULL;
Antoine
--
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 9/9] usb: chipidea: add support to the generic PHY framework in ChipIdea
On Fri, Sep 12, 2014 at 06:28:29PM +0800, Peter Chen wrote:
On Fri, Sep 12, 2014 at 11:35:50AM +0200, Antoine Tenart wrote:
On Fri, Sep 12, 2014 at 05:27:13PM +0800, Peter Chen wrote:
On Fri, Sep 12, 2014 at 10:21:25AM +0200, Antoine Tenart wrote:
Peter,
On Fri, Sep 12, 2014 at 01:10:33AM +, Peter Chen wrote:
On Thu, Sep 11, 2014 at 08:54:47AM +0800, Peter Chen wrote:
On Wed, Sep 03, 2014 at 09:40:40AM +0200, Antoine Tenart wrote:
@@ -595,23 +639,27 @@ static int ci_hdrc_probe(struct
platform_device
*pdev)
return -ENODEV;
}
- if (ci-platdata-usb_phy)
+ if (ci-platdata-phy)
+ ci-phy = ci-platdata-phy;
+ else if (ci-platdata-usb_phy)
ci-usb_phy = ci-platdata-usb_phy;
else
- ci-usb_phy = devm_usb_get_phy(dev,
USB_PHY_TYPE_USB2);
+ ci-phy = devm_phy_get(dev, usb-phy);
- if (IS_ERR(ci-usb_phy)) {
- ret = PTR_ERR(ci-usb_phy);
+ if (IS_ERR(ci-phy) || (ci-phy == NULL ci-usb_phy
== NULL)) {
/*
* if -ENXIO is returned, it means PHY layer
wasn't
* enabled, so it makes no sense to return
-EPROBE_DEFER
* in that case, since no PHY driver will ever
probe.
*/
- if (ret == -ENXIO)
- return ret;
+ if (PTR_ERR(ci-phy) == -ENXIO)
+ return -ENXIO;
- dev_err(dev, no usb2 phy configured\n);
- return -EPROBE_DEFER;
+ ci-usb_phy = devm_usb_get_phy(dev,
USB_PHY_TYPE_USB2);
+ if (IS_ERR(ci-usb_phy)) {
+ dev_err(dev, no usb2 phy
configured\n);
+ return -EPROBE_DEFER;
+ }
}
Sorry, I can't accept this change, why devm_usb_get_phy(dev,
USB_PHY_TYPE_USB2) is put at error path? Since current get PHY
operation is a little complicated, we may have a dedicate
function to do it,
dwc3 driver is a good example.
It's not the error path, it's the case when there is no PHY from
the generic PHY
framework available. Getting an USB PHY is a fallback solution.
I agree we can move this to a dedicated function. But even if doing
so, we'll
have to test ci-phy first.
Or do you have something else in mind?
I still want devm_usb_get_phy(dev, USB_PHY_TYPE_USB2) to be called at
the same place
like generic_phy, not in later error path, in error path, we only
handle error.
Would this fit you?
if (ci-platdata-phy)
ci-phy = ci-platdata-phy;
else if (ci-platdata-usb_phy)
ci-usb_phy = ci-platdata-usb_phy;
if (ci-phy == NULL ci-usb_phy == NULL) {
You should use else if here.
Yes, sure.
ci-phy = devm_phy_get(dev, usb-phy);
ci-usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
/* if both generic PHY and USB PHY layers aren't enabled */
if (PTR_ERR(ci-phy) == -ENOSYS PTR_ERR(ci-usb_phy) ==
-ENXIO)
return -ENXIO;
if (IS_ERR(ci-phy) IS_ERR(ci-usb_phy))
return -EPROBE_DEFER;
/* if an usb_phy is available, but no phy is there */
if (PTR_ERR(ci-phy) == -ENODEV)
ci-phy = NULL;
if (ci-phy)
ci-usb_phy = NULL;
if (ci-usb_phy)
ci-phy = NULL;
How about above?
When no PHY or USB PHY is found, -ENODEV is returned. Doing the above
would always end up having ci-phy and ci-usb_phy being NULL.
When we try to get generic phy, if -ENODEV is returned, the ci-usb_phy
should not be NULL, otherwise, your the second condition will return
-EPROBE_DEFER, it means when the code goes here, there must be one PHY
is found.
We first call devm_phy_get() and devm_usb_get_phy(), ci-phy and
ci-usb_phy can't be NULL after this.
Then the if (IS_ERR(ci-phy) IS_ERR(ci-usb_phy)) ensure at least
one of them is a valid PHY.
I don't see the problem here. Am I missing something?
We could have:
if (IS_ERR(ci-phy))
ci-phy = NULL;
else if (IS_ERR(ci-usb_phy))
ci-usb_phy = NULL;
Antoine
--
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info
RE: [PATCH v4 9/9] usb: chipidea: add support to the generic PHY framework in ChipIdea
On Fri, Sep 12, 2014 at 06:28:29PM +0800, Peter Chen wrote:
On Fri, Sep 12, 2014 at 11:35:50AM +0200, Antoine Tenart wrote:
On Fri, Sep 12, 2014 at 05:27:13PM +0800, Peter Chen wrote:
On Fri, Sep 12, 2014 at 10:21:25AM +0200, Antoine Tenart wrote:
Peter,
On Fri, Sep 12, 2014 at 01:10:33AM +, Peter Chen wrote:
On Thu, Sep 11, 2014 at 08:54:47AM +0800, Peter Chen wrote:
On Wed, Sep 03, 2014 at 09:40:40AM +0200, Antoine Tenart wrote:
@@ -595,23 +639,27 @@ static int ci_hdrc_probe(struct
platform_device
*pdev)
return -ENODEV;
}
- if (ci-platdata-usb_phy)
+ if (ci-platdata-phy)
+ ci-phy = ci-platdata-phy;
+ else if (ci-platdata-usb_phy)
ci-usb_phy = ci-platdata-usb_phy;
else
- ci-usb_phy = devm_usb_get_phy(dev,
USB_PHY_TYPE_USB2);
+ ci-phy = devm_phy_get(dev, usb-phy);
- if (IS_ERR(ci-usb_phy)) {
- ret = PTR_ERR(ci-usb_phy);
+ if (IS_ERR(ci-phy) || (ci-phy == NULL ci-usb_phy
+== NULL)) {
/*
* if -ENXIO is returned, it means PHY layer
wasn't
* enabled, so it makes no sense to return -
EPROBE_DEFER
* in that case, since no PHY driver will ever
probe.
*/
- if (ret == -ENXIO)
- return ret;
+ if (PTR_ERR(ci-phy) == -ENXIO)
+ return -ENXIO;
- dev_err(dev, no usb2 phy configured\n);
- return -EPROBE_DEFER;
+ ci-usb_phy = devm_usb_get_phy(dev,
USB_PHY_TYPE_USB2);
+ if (IS_ERR(ci-usb_phy)) {
+ dev_err(dev, no usb2 phy
configured\n);
+ return -EPROBE_DEFER;
+ }
}
Sorry, I can't accept this change, why
devm_usb_get_phy(dev,
USB_PHY_TYPE_USB2) is put at error path? Since current get
PHY operation is a little complicated, we may have a
dedicate function to do it,
dwc3 driver is a good example.
It's not the error path, it's the case when there is no PHY
from the generic PHY framework available. Getting an USB PHY is a
fallback solution.
I agree we can move this to a dedicated function. But even
if doing so, we'll have to test ci-phy first.
Or do you have something else in mind?
I still want devm_usb_get_phy(dev, USB_PHY_TYPE_USB2) to be
called at the same place like generic_phy, not in later error path,
in
error path, we only handle error.
Would this fit you?
if (ci-platdata-phy)
ci-phy = ci-platdata-phy; else if
(ci-platdata-usb_phy)
ci-usb_phy = ci-platdata-usb_phy;
if (ci-phy == NULL ci-usb_phy == NULL) {
You should use else if here.
Yes, sure.
ci-phy = devm_phy_get(dev, usb-phy);
ci-usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
/* if both generic PHY and USB PHY layers aren't enabled */
if (PTR_ERR(ci-phy) == -ENOSYS PTR_ERR(ci-usb_phy) == -
ENXIO)
return -ENXIO;
if (IS_ERR(ci-phy) IS_ERR(ci-usb_phy))
return -EPROBE_DEFER;
/* if an usb_phy is available, but no phy is there */
if (PTR_ERR(ci-phy) == -ENODEV)
ci-phy = NULL;
if (ci-phy)
ci-usb_phy = NULL;
if (ci-usb_phy)
ci-phy = NULL;
How about above?
When no PHY or USB PHY is found, -ENODEV is returned. Doing the
above would always end up having ci-phy and ci-usb_phy being NULL.
When we try to get generic phy, if -ENODEV is returned, the
ci-usb_phy should not be NULL, otherwise, your the second condition
will return -EPROBE_DEFER, it means when the code goes here, there
must be one PHY is found.
We first call devm_phy_get() and devm_usb_get_phy(), ci-phy and
ci-usb_phy can't be NULL after this.
Then the if (IS_ERR(ci-phy) IS_ERR(ci-usb_phy)) ensure at least one of
them is a valid PHY.
I don't see the problem here. Am I missing something?
We could have:
if (IS_ERR(ci-phy))
ci-phy = NULL;
else if (IS_ERR(ci-usb_phy))
ci-usb_phy = NULL;
Both of our implementation work, ok, use yours.
Peter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v4 9/9] usb: chipidea: add support to the generic PHY framework in ChipIdea
> On Thu, Sep 11, 2014 at 08:54:47AM +0800, Peter Chen wrote:
> > On Wed, Sep 03, 2014 at 09:40:40AM +0200, Antoine Tenart wrote:
> > > @@ -595,23 +639,27 @@ static int ci_hdrc_probe(struct platform_device
> *pdev)
> > > return -ENODEV;
> > > }
> > >
> > > - if (ci->platdata->usb_phy)
> > > + if (ci->platdata->phy)
> > > + ci->phy = ci->platdata->phy;
> > > + else if (ci->platdata->usb_phy)
> > > ci->usb_phy = ci->platdata->usb_phy;
> > > else
> > > - ci->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> > > + ci->phy = devm_phy_get(dev, "usb-phy");
> > >
> > > - if (IS_ERR(ci->usb_phy)) {
> > > - ret = PTR_ERR(ci->usb_phy);
> > > + if (IS_ERR(ci->phy) || (ci->phy == NULL && ci->usb_phy == NULL)) {
> > > /*
> > >* if -ENXIO is returned, it means PHY layer wasn't
> > >* enabled, so it makes no sense to return -EPROBE_DEFER
> > >* in that case, since no PHY driver will ever probe.
> > >*/
> > > - if (ret == -ENXIO)
> > > - return ret;
> > > + if (PTR_ERR(ci->phy) == -ENXIO)
> > > + return -ENXIO;
> > >
> > > - dev_err(dev, "no usb2 phy configured\n");
> > > - return -EPROBE_DEFER;
> > > + ci->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> > > + if (IS_ERR(ci->usb_phy)) {
> > > + dev_err(dev, "no usb2 phy configured\n");
> > > + return -EPROBE_DEFER;
> > > + }
> > > }
> >
> > Sorry, I can't accept this change, why devm_usb_get_phy(dev,
> > USB_PHY_TYPE_USB2) is put at error path? Since current get PHY
> > operation is a little complicated, we may have a dedicate function to do it,
> dwc3 driver is a good example.
>
> It's not the error path, it's the case when there is no PHY from the generic
> PHY
> framework available. Getting an USB PHY is a fallback solution.
>
> I agree we can move this to a dedicated function. But even if doing so, we'll
> have to test ci->phy first.
>
> Or do you have something else in mind?
>
I still want devm_usb_get_phy(dev, USB_PHY_TYPE_USB2) to be called at the same
place
like generic_phy, not in later error path, in error path, we only handle error.
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 9/9] usb: chipidea: add support to the generic PHY framework in ChipIdea
Peter,
On Thu, Sep 11, 2014 at 08:54:47AM +0800, Peter Chen wrote:
> On Wed, Sep 03, 2014 at 09:40:40AM +0200, Antoine Tenart wrote:
> > @@ -595,23 +639,27 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> > return -ENODEV;
> > }
> >
> > - if (ci->platdata->usb_phy)
> > + if (ci->platdata->phy)
> > + ci->phy = ci->platdata->phy;
> > + else if (ci->platdata->usb_phy)
> > ci->usb_phy = ci->platdata->usb_phy;
> > else
> > - ci->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> > + ci->phy = devm_phy_get(dev, "usb-phy");
> >
> > - if (IS_ERR(ci->usb_phy)) {
> > - ret = PTR_ERR(ci->usb_phy);
> > + if (IS_ERR(ci->phy) || (ci->phy == NULL && ci->usb_phy == NULL)) {
> > /*
> > * if -ENXIO is returned, it means PHY layer wasn't
> > * enabled, so it makes no sense to return -EPROBE_DEFER
> > * in that case, since no PHY driver will ever probe.
> > */
> > - if (ret == -ENXIO)
> > - return ret;
> > + if (PTR_ERR(ci->phy) == -ENXIO)
> > + return -ENXIO;
> >
> > - dev_err(dev, "no usb2 phy configured\n");
> > - return -EPROBE_DEFER;
> > + ci->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> > + if (IS_ERR(ci->usb_phy)) {
> > + dev_err(dev, "no usb2 phy configured\n");
> > + return -EPROBE_DEFER;
> > + }
> > }
>
> Sorry, I can't accept this change, why devm_usb_get_phy(dev,
> USB_PHY_TYPE_USB2)
> is put at error path? Since current get PHY operation is a little complicated,
> we may have a dedicate function to do it, dwc3 driver is a good example.
It's not the error path, it's the case when there is no PHY from the
generic PHY framework available. Getting an USB PHY is a fallback
solution.
I agree we can move this to a dedicated function. But even if doing so,
we'll have to test ci->phy first.
Or do you have something else in mind?
> > diff --git a/drivers/usb/chipidea/debug.c b/drivers/usb/chipidea/debug.c
> > index 5a7ea93011dd..999e9d683d7a 100644
> > --- a/drivers/usb/chipidea/debug.c
> > +++ b/drivers/usb/chipidea/debug.c
> > @@ -219,7 +219,8 @@ static int ci_otg_show(struct seq_file *s, void *unused)
> > fsm = >fsm;
> >
> > /* -- State - */
> > - usb_otg_state_string(ci->usb_phy->otg.state));
> > + seq_printf(s, "OTG state: %s\n\n",
> > + usb_otg_state_string(ci->otg.state));
>
> Again, rebase error?
Yes, sorry about that. This will be fixed in the next version of this
series.
Antoine
--
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v4 9/9] usb: chipidea: add support to the generic PHY framework in ChipIdea
On Thu, Sep 11, 2014 at 08:54:47AM +0800, Peter Chen wrote:
On Wed, Sep 03, 2014 at 09:40:40AM +0200, Antoine Tenart wrote:
@@ -595,23 +639,27 @@ static int ci_hdrc_probe(struct platform_device
*pdev)
return -ENODEV;
}
- if (ci-platdata-usb_phy)
+ if (ci-platdata-phy)
+ ci-phy = ci-platdata-phy;
+ else if (ci-platdata-usb_phy)
ci-usb_phy = ci-platdata-usb_phy;
else
- ci-usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
+ ci-phy = devm_phy_get(dev, usb-phy);
- if (IS_ERR(ci-usb_phy)) {
- ret = PTR_ERR(ci-usb_phy);
+ if (IS_ERR(ci-phy) || (ci-phy == NULL ci-usb_phy == NULL)) {
/*
* if -ENXIO is returned, it means PHY layer wasn't
* enabled, so it makes no sense to return -EPROBE_DEFER
* in that case, since no PHY driver will ever probe.
*/
- if (ret == -ENXIO)
- return ret;
+ if (PTR_ERR(ci-phy) == -ENXIO)
+ return -ENXIO;
- dev_err(dev, no usb2 phy configured\n);
- return -EPROBE_DEFER;
+ ci-usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
+ if (IS_ERR(ci-usb_phy)) {
+ dev_err(dev, no usb2 phy configured\n);
+ return -EPROBE_DEFER;
+ }
}
Sorry, I can't accept this change, why devm_usb_get_phy(dev,
USB_PHY_TYPE_USB2) is put at error path? Since current get PHY
operation is a little complicated, we may have a dedicate function to do it,
dwc3 driver is a good example.
It's not the error path, it's the case when there is no PHY from the generic
PHY
framework available. Getting an USB PHY is a fallback solution.
I agree we can move this to a dedicated function. But even if doing so, we'll
have to test ci-phy first.
Or do you have something else in mind?
I still want devm_usb_get_phy(dev, USB_PHY_TYPE_USB2) to be called at the same
place
like generic_phy, not in later error path, in error path, we only handle error.
Peter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 9/9] usb: chipidea: add support to the generic PHY framework in ChipIdea
Peter,
On Thu, Sep 11, 2014 at 08:54:47AM +0800, Peter Chen wrote:
On Wed, Sep 03, 2014 at 09:40:40AM +0200, Antoine Tenart wrote:
@@ -595,23 +639,27 @@ static int ci_hdrc_probe(struct platform_device *pdev)
return -ENODEV;
}
- if (ci-platdata-usb_phy)
+ if (ci-platdata-phy)
+ ci-phy = ci-platdata-phy;
+ else if (ci-platdata-usb_phy)
ci-usb_phy = ci-platdata-usb_phy;
else
- ci-usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
+ ci-phy = devm_phy_get(dev, usb-phy);
- if (IS_ERR(ci-usb_phy)) {
- ret = PTR_ERR(ci-usb_phy);
+ if (IS_ERR(ci-phy) || (ci-phy == NULL ci-usb_phy == NULL)) {
/*
* if -ENXIO is returned, it means PHY layer wasn't
* enabled, so it makes no sense to return -EPROBE_DEFER
* in that case, since no PHY driver will ever probe.
*/
- if (ret == -ENXIO)
- return ret;
+ if (PTR_ERR(ci-phy) == -ENXIO)
+ return -ENXIO;
- dev_err(dev, no usb2 phy configured\n);
- return -EPROBE_DEFER;
+ ci-usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
+ if (IS_ERR(ci-usb_phy)) {
+ dev_err(dev, no usb2 phy configured\n);
+ return -EPROBE_DEFER;
+ }
}
Sorry, I can't accept this change, why devm_usb_get_phy(dev,
USB_PHY_TYPE_USB2)
is put at error path? Since current get PHY operation is a little complicated,
we may have a dedicate function to do it, dwc3 driver is a good example.
It's not the error path, it's the case when there is no PHY from the
generic PHY framework available. Getting an USB PHY is a fallback
solution.
I agree we can move this to a dedicated function. But even if doing so,
we'll have to test ci-phy first.
Or do you have something else in mind?
diff --git a/drivers/usb/chipidea/debug.c b/drivers/usb/chipidea/debug.c
index 5a7ea93011dd..999e9d683d7a 100644
--- a/drivers/usb/chipidea/debug.c
+++ b/drivers/usb/chipidea/debug.c
@@ -219,7 +219,8 @@ static int ci_otg_show(struct seq_file *s, void *unused)
fsm = ci-fsm;
/* -- State - */
- usb_otg_state_string(ci-usb_phy-otg.state));
+ seq_printf(s, OTG state: %s\n\n,
+ usb_otg_state_string(ci-otg.state));
Again, rebase error?
Yes, sorry about that. This will be fixed in the next version of this
series.
Antoine
--
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 9/9] usb: chipidea: add support to the generic PHY framework in ChipIdea
On Wed, Sep 03, 2014 at 09:40:40AM +0200, Antoine Tenart wrote:
> This patch adds support of the PHY framework for ChipIdea drivers.
> Changes are done in both the ChipIdea common code and in the drivers
> accessing the PHY. This is done by adding a new PHY member in
> ChipIdea's structures and by taking care of it in the code.
>
> Signed-off-by: Antoine Tenart
> ---
> drivers/usb/chipidea/ci.h | 5 ++-
> drivers/usb/chipidea/core.c| 74
> ++
> drivers/usb/chipidea/debug.c | 3 +-
> drivers/usb/chipidea/host.c| 5 ++-
> drivers/usb/chipidea/otg_fsm.c | 6 +++-
> include/linux/usb/chipidea.h | 2 ++
> 6 files changed, 78 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> index dac5ab6adfa2..7e9e8223672a 100644
> --- a/drivers/usb/chipidea/ci.h
> +++ b/drivers/usb/chipidea/ci.h
> @@ -161,7 +161,8 @@ struct hw_bank {
> * @test_mode: the selected test mode
> * @platdata: platform specific information supplied by parent device
> * @vbus_active: is VBUS active
> - * @usb_phy: pointer to USB PHY, if any
> + * @phy: pointer to PHY, if any
> + * @usb_phy: pointer to USB PHY, if any and if using the USB PHY framework
> * @hcd: pointer to usb_hcd for ehci host driver
> * @debugfs: root dentry for this controller in debugfs
> * @id_event: indicates there is an id event, and handled at ci_otg_work
> @@ -202,6 +203,8 @@ struct ci_hdrc {
>
> struct ci_hdrc_platform_data*platdata;
> int vbus_active;
> + struct phy *phy;
> + /* old usb_phy interface */
> struct usb_phy *usb_phy;
> struct usb_hcd *hcd;
> struct dentry *debugfs;
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index a8cd35fd8175..7c617b765bf2 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -47,6 +47,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -293,6 +294,49 @@ static void hw_phymode_configure(struct ci_hdrc *ci)
> }
>
> /**
> + * _ci_usb_phy_init: initialize phy taking in account both phy and usb_phy
> + * interfaces
> + * @ci: the controller
> + *
> + * This function returns an error code if the phy failed to init
> + */
> +static int _ci_usb_phy_init(struct ci_hdrc *ci)
> +{
> + int ret;
> +
> + if (ci->phy) {
> + ret = phy_init(ci->phy);
> + if (ret)
> + return ret;
> +
> + ret = phy_power_on(ci->phy);
> + if (ret) {
> + phy_exit(ci->phy);
> + return ret;
> + }
> + } else {
> + ret = usb_phy_init(ci->usb_phy);
> + }
> +
> + return ret;
> +}
> +
> +/**
> + * _ci_usb_phy_exit: deinitialize phy taking in account both phy and usb_phy
> + * interfaces
> + * @ci: the controller
> + */
> +static void ci_usb_phy_exit(struct ci_hdrc *ci)
> +{
> + if (ci->phy) {
> + phy_power_off(ci->phy);
> + phy_exit(ci->phy);
> + } else {
> + usb_phy_shutdown(ci->usb_phy);
> + }
> +}
> +
> +/**
> * ci_usb_phy_init: initialize phy according to different phy type
> * @ci: the controller
>*
> @@ -306,7 +350,7 @@ static int ci_usb_phy_init(struct ci_hdrc *ci)
> case USBPHY_INTERFACE_MODE_UTMI:
> case USBPHY_INTERFACE_MODE_UTMIW:
> case USBPHY_INTERFACE_MODE_HSIC:
> - ret = usb_phy_init(ci->usb_phy);
> + ret = _ci_usb_phy_init(ci);
> if (ret)
> return ret;
> hw_phymode_configure(ci);
> @@ -314,12 +358,12 @@ static int ci_usb_phy_init(struct ci_hdrc *ci)
> case USBPHY_INTERFACE_MODE_ULPI:
> case USBPHY_INTERFACE_MODE_SERIAL:
> hw_phymode_configure(ci);
> - ret = usb_phy_init(ci->usb_phy);
> + ret = _ci_usb_phy_init(ci);
> if (ret)
> return ret;
> break;
> default:
> - ret = usb_phy_init(ci->usb_phy);
> + ret = _ci_usb_phy_init(ci);
> }
>
> return ret;
> @@ -595,23 +639,27 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> return -ENODEV;
> }
>
> - if (ci->platdata->usb_phy)
> + if (ci->platdata->phy)
> + ci->phy = ci->platdata->phy;
> + else if (ci->platdata->usb_phy)
> ci->usb_phy = ci->platdata->usb_phy;
> else
> - ci->usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> + ci->phy = devm_phy_get(dev, "usb-phy");
>
> - if (IS_ERR(ci->usb_phy)) {
> - ret = PTR_ERR(ci->usb_phy);
> + if (IS_ERR(ci->phy) || (ci->phy == NULL && ci->usb_phy == NULL)) {
> /*
>* if -ENXIO is returned, it means PHY layer wasn't
>
Re: [PATCH v4 9/9] usb: chipidea: add support to the generic PHY framework in ChipIdea
On Wed, Sep 03, 2014 at 09:40:40AM +0200, Antoine Tenart wrote:
This patch adds support of the PHY framework for ChipIdea drivers.
Changes are done in both the ChipIdea common code and in the drivers
accessing the PHY. This is done by adding a new PHY member in
ChipIdea's structures and by taking care of it in the code.
Signed-off-by: Antoine Tenart antoine.ten...@free-electrons.com
---
drivers/usb/chipidea/ci.h | 5 ++-
drivers/usb/chipidea/core.c| 74
++
drivers/usb/chipidea/debug.c | 3 +-
drivers/usb/chipidea/host.c| 5 ++-
drivers/usb/chipidea/otg_fsm.c | 6 +++-
include/linux/usb/chipidea.h | 2 ++
6 files changed, 78 insertions(+), 17 deletions(-)
diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index dac5ab6adfa2..7e9e8223672a 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -161,7 +161,8 @@ struct hw_bank {
* @test_mode: the selected test mode
* @platdata: platform specific information supplied by parent device
* @vbus_active: is VBUS active
- * @usb_phy: pointer to USB PHY, if any
+ * @phy: pointer to PHY, if any
+ * @usb_phy: pointer to USB PHY, if any and if using the USB PHY framework
* @hcd: pointer to usb_hcd for ehci host driver
* @debugfs: root dentry for this controller in debugfs
* @id_event: indicates there is an id event, and handled at ci_otg_work
@@ -202,6 +203,8 @@ struct ci_hdrc {
struct ci_hdrc_platform_data*platdata;
int vbus_active;
+ struct phy *phy;
+ /* old usb_phy interface */
struct usb_phy *usb_phy;
struct usb_hcd *hcd;
struct dentry *debugfs;
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index a8cd35fd8175..7c617b765bf2 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -47,6 +47,7 @@
#include linux/delay.h
#include linux/device.h
#include linux/dma-mapping.h
+#include linux/phy/phy.h
#include linux/platform_device.h
#include linux/module.h
#include linux/idr.h
@@ -293,6 +294,49 @@ static void hw_phymode_configure(struct ci_hdrc *ci)
}
/**
+ * _ci_usb_phy_init: initialize phy taking in account both phy and usb_phy
+ * interfaces
+ * @ci: the controller
+ *
+ * This function returns an error code if the phy failed to init
+ */
+static int _ci_usb_phy_init(struct ci_hdrc *ci)
+{
+ int ret;
+
+ if (ci-phy) {
+ ret = phy_init(ci-phy);
+ if (ret)
+ return ret;
+
+ ret = phy_power_on(ci-phy);
+ if (ret) {
+ phy_exit(ci-phy);
+ return ret;
+ }
+ } else {
+ ret = usb_phy_init(ci-usb_phy);
+ }
+
+ return ret;
+}
+
+/**
+ * _ci_usb_phy_exit: deinitialize phy taking in account both phy and usb_phy
+ * interfaces
+ * @ci: the controller
+ */
+static void ci_usb_phy_exit(struct ci_hdrc *ci)
+{
+ if (ci-phy) {
+ phy_power_off(ci-phy);
+ phy_exit(ci-phy);
+ } else {
+ usb_phy_shutdown(ci-usb_phy);
+ }
+}
+
+/**
* ci_usb_phy_init: initialize phy according to different phy type
* @ci: the controller
*
@@ -306,7 +350,7 @@ static int ci_usb_phy_init(struct ci_hdrc *ci)
case USBPHY_INTERFACE_MODE_UTMI:
case USBPHY_INTERFACE_MODE_UTMIW:
case USBPHY_INTERFACE_MODE_HSIC:
- ret = usb_phy_init(ci-usb_phy);
+ ret = _ci_usb_phy_init(ci);
if (ret)
return ret;
hw_phymode_configure(ci);
@@ -314,12 +358,12 @@ static int ci_usb_phy_init(struct ci_hdrc *ci)
case USBPHY_INTERFACE_MODE_ULPI:
case USBPHY_INTERFACE_MODE_SERIAL:
hw_phymode_configure(ci);
- ret = usb_phy_init(ci-usb_phy);
+ ret = _ci_usb_phy_init(ci);
if (ret)
return ret;
break;
default:
- ret = usb_phy_init(ci-usb_phy);
+ ret = _ci_usb_phy_init(ci);
}
return ret;
@@ -595,23 +639,27 @@ static int ci_hdrc_probe(struct platform_device *pdev)
return -ENODEV;
}
- if (ci-platdata-usb_phy)
+ if (ci-platdata-phy)
+ ci-phy = ci-platdata-phy;
+ else if (ci-platdata-usb_phy)
ci-usb_phy = ci-platdata-usb_phy;
else
- ci-usb_phy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
+ ci-phy = devm_phy_get(dev, usb-phy);
- if (IS_ERR(ci-usb_phy)) {
- ret = PTR_ERR(ci-usb_phy);
+ if (IS_ERR(ci-phy) || (ci-phy == NULL ci-usb_phy == NULL)) {
/*
* if -ENXIO is returned, it means PHY layer wasn't
* enabled, so it makes
RE: [PATCH v4 9/9] usb: chipidea: add support to the generic PHY framework in ChipIdea
Hi, -Original Message- From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of Antoine Tenart Sent: Wednesday, September 03, 2014 3:41 PM To: ba...@ti.com; gre...@linuxfoundation.org; Chen Peter-B29397; kis...@ti.com; st...@rowland.harvard.edu Cc: Antoine Tenart; sergei.shtyl...@cogentembedded.com; yoshihiro.shimoda...@renesas.com; alexandre.bell...@free-electrons.com; thomas.petazz...@free-electrons.com; z...@marvell.com; jszh...@marvell.com; linux-...@vger.kernel.org; linux-kernel@vger.kernel.org Subject: [PATCH v4 9/9] usb: chipidea: add support to the generic PHY framework in ChipIdea This patch adds support of the PHY framework for ChipIdea drivers. Changes are done in both the ChipIdea common code and in the drivers accessing the PHY. This is done by adding a new PHY member in ChipIdea's structures and by taking care of it in the code. Signed-off-by: Antoine Tenart --- drivers/usb/chipidea/ci.h | 5 ++- drivers/usb/chipidea/core.c| 74 ++ drivers/usb/chipidea/debug.c | 3 +- drivers/usb/chipidea/host.c| 5 ++- drivers/usb/chipidea/otg_fsm.c | 6 +++- include/linux/usb/chipidea.h | 2 ++ 6 files changed, 78 insertions(+), 17 deletions(-) ... diff --git a/drivers/usb/chipidea/debug.c b/drivers/usb/chipidea/debug.c index 5a7ea93011dd..999e9d683d7a 100644 --- a/drivers/usb/chipidea/debug.c +++ b/drivers/usb/chipidea/debug.c @@ -219,7 +219,8 @@ static int ci_otg_show(struct seq_file *s, void *unused) fsm = >fsm; /* -- State - */ - usb_otg_state_string(ci->usb_phy->otg.state)); + seq_printf(s, "OTG state: %s\n\n", Still not resolve this? Li Jun + usb_otg_state_string(ci->otg.state)); /* -- State Machine Variables - */ seq_printf(s, "a_bus_drop: %d\n", fsm->a_bus_drop); diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c index -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v4 9/9] usb: chipidea: add support to the generic PHY framework in ChipIdea
Hi, -Original Message- From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of Antoine Tenart Sent: Wednesday, September 03, 2014 3:41 PM To: ba...@ti.com; gre...@linuxfoundation.org; Chen Peter-B29397; kis...@ti.com; st...@rowland.harvard.edu Cc: Antoine Tenart; sergei.shtyl...@cogentembedded.com; yoshihiro.shimoda...@renesas.com; alexandre.bell...@free-electrons.com; thomas.petazz...@free-electrons.com; z...@marvell.com; jszh...@marvell.com; linux-...@vger.kernel.org; linux-kernel@vger.kernel.org Subject: [PATCH v4 9/9] usb: chipidea: add support to the generic PHY framework in ChipIdea This patch adds support of the PHY framework for ChipIdea drivers. Changes are done in both the ChipIdea common code and in the drivers accessing the PHY. This is done by adding a new PHY member in ChipIdea's structures and by taking care of it in the code. Signed-off-by: Antoine Tenart antoine.ten...@free-electrons.com --- drivers/usb/chipidea/ci.h | 5 ++- drivers/usb/chipidea/core.c| 74 ++ drivers/usb/chipidea/debug.c | 3 +- drivers/usb/chipidea/host.c| 5 ++- drivers/usb/chipidea/otg_fsm.c | 6 +++- include/linux/usb/chipidea.h | 2 ++ 6 files changed, 78 insertions(+), 17 deletions(-) ... diff --git a/drivers/usb/chipidea/debug.c b/drivers/usb/chipidea/debug.c index 5a7ea93011dd..999e9d683d7a 100644 --- a/drivers/usb/chipidea/debug.c +++ b/drivers/usb/chipidea/debug.c @@ -219,7 +219,8 @@ static int ci_otg_show(struct seq_file *s, void *unused) fsm = ci-fsm; /* -- State - */ - usb_otg_state_string(ci-usb_phy-otg.state)); + seq_printf(s, OTG state: %s\n\n, Still not resolve this? Li Jun + usb_otg_state_string(ci-otg.state)); /* -- State Machine Variables - */ seq_printf(s, a_bus_drop: %d\n, fsm-a_bus_drop); diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c index -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
