Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1475844 > unrolled thread

[PATCH] usb: phy: generic: request regulator optionally

Back to article view | Back to linux.kernel

Contents

  [PATCH] usb: phy: generic: request regulator optionally Stefan Agner <stefan@agner.ch> - 2016-09-04 06:30 +0200
    Re: [PATCH] usb: phy: generic: request regulator optionally Felipe Balbi <balbi@kernel.org> - 2016-09-06 09:50 +0200
      Re: [PATCH] usb: phy: generic: request regulator optionally Mark Brown <broonie@kernel.org> - 2016-09-06 10:30 +0200
        Re: [PATCH] usb: phy: generic: request regulator optionally Stefan Agner <stefan@agner.ch> - 2016-09-06 20:10 +0200
          Re: [PATCH] usb: phy: generic: request regulator optionally Roger Quadros <rogerq@ti.com> - 2016-09-07 09:30 +0200
            Re: [PATCH] usb: phy: generic: request regulator optionally Felipe Balbi <balbi@kernel.org> - 2016-09-07 10:10 +0200
          Re: [PATCH] usb: phy: generic: request regulator optionally Mark Brown <broonie@kernel.org> - 2016-09-07 21:00 +0200
            Re: [PATCH] usb: phy: generic: request regulator optionally Stefan Agner <stefan@agner.ch> - 2016-09-07 22:50 +0200

#1475844 — [PATCH] usb: phy: generic: request regulator optionally

According to the device tree bindings the vcc-supply is optional.
So far the driver did request the regulator using devm_regulator_get
which creates a dummy regulator for convenience. Since we can have
the supply unconnected, we should make use of the optional variant
of the regulator call which does not return a dummy regulator but
-ENODEV. The driver already has checks in case the regulator is an
error pointer.

Note that with this change the behavior is slightly different in
case devm_regulator_get_optional returns -EPROBE_DEFER: The driver
returns -EPROBE_DEFER even if needs_vcc is set false. This is the
correct behavior, since even if the regulator is optional, it might
get initialized later...

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
This gets rid of warnings such as this (seen on i.MX 7):
30800000.aips-bus:usbphynop1 supply vcc not found, using dummy regulator

--
Stefan

 drivers/usb/phy/phy-generic.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c
index 980c9de..38ceadc 100644
--- a/drivers/usb/phy/phy-generic.c
+++ b/drivers/usb/phy/phy-generic.c
@@ -275,12 +275,12 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop,
 		}
 	}
 
-	nop->vcc = devm_regulator_get(dev, "vcc");
+	nop->vcc = devm_regulator_get_optional(dev, "vcc");
 	if (IS_ERR(nop->vcc)) {
 		dev_dbg(dev, "Error getting vcc regulator: %ld\n",
 					PTR_ERR(nop->vcc));
-		if (needs_vcc)
-			return -EPROBE_DEFER;
+		if (needs_vcc || PTR_ERR(nop->vcc) == -EPROBE_DEFER)
+			return PTR_ERR(nop->vcc);
 	}
 
 	nop->dev		= dev;
-- 
2.9.0

[toc] | [next] | [standalone]

#1477128

[Multipart message — attachments visible in raw view] — view raw

Mark,

Stefan Agner <stefan@agner.ch> writes:
> According to the device tree bindings the vcc-supply is optional.
> So far the driver did request the regulator using devm_regulator_get
> which creates a dummy regulator for convenience. Since we can have
> the supply unconnected, we should make use of the optional variant
> of the regulator call which does not return a dummy regulator but
> -ENODEV. The driver already has checks in case the regulator is an
> error pointer.
>
> Note that with this change the behavior is slightly different in
> case devm_regulator_get_optional returns -EPROBE_DEFER: The driver
> returns -EPROBE_DEFER even if needs_vcc is set false. This is the
> correct behavior, since even if the regulator is optional, it might
> get initialized later...
>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
> This gets rid of warnings such as this (seen on i.MX 7):
> 30800000.aips-bus:usbphynop1 supply vcc not found, using dummy regulator
>
> --
> Stefan
>
>  drivers/usb/phy/phy-generic.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c
> index 980c9de..38ceadc 100644
> --- a/drivers/usb/phy/phy-generic.c
> +++ b/drivers/usb/phy/phy-generic.c
> @@ -275,12 +275,12 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop,
>  		}
>  	}
>  
> -	nop->vcc = devm_regulator_get(dev, "vcc");
> +	nop->vcc = devm_regulator_get_optional(dev, "vcc");
>  	if (IS_ERR(nop->vcc)) {
>  		dev_dbg(dev, "Error getting vcc regulator: %ld\n",
>  					PTR_ERR(nop->vcc));
> -		if (needs_vcc)
> -			return -EPROBE_DEFER;
> +		if (needs_vcc || PTR_ERR(nop->vcc) == -EPROBE_DEFER)
> +			return PTR_ERR(nop->vcc);

does this look okay from a regulator API perspective?

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1477157

[Multipart message — attachments visible in raw view] — view raw

On Tue, Sep 06, 2016 at 10:45:19AM +0300, Felipe Balbi wrote:
> Stefan Agner <stefan@agner.ch> writes:

> > According to the device tree bindings the vcc-supply is optional.

This is nonsense unless the device can work without this supply.  Given
that the supply is called VCC that doesn't seem entirely likely.

> > +	nop->vcc = devm_regulator_get_optional(dev, "vcc");
> >  	if (IS_ERR(nop->vcc)) {
> >  		dev_dbg(dev, "Error getting vcc regulator: %ld\n",
> >  					PTR_ERR(nop->vcc));
> > -		if (needs_vcc)
> > -			return -EPROBE_DEFER;
> > +		if (needs_vcc || PTR_ERR(nop->vcc) == -EPROBE_DEFER)
> > +			return PTR_ERR(nop->vcc);

> does this look okay from a regulator API perspective?

That's how to use _get_optional() but it's really unusual that you
should be using _get_optional().

[toc] | [prev] | [next] | [standalone]

#1477723

On 2016-09-06 01:22, Mark Brown wrote:
> On Tue, Sep 06, 2016 at 10:45:19AM +0300, Felipe Balbi wrote:
>> Stefan Agner <stefan@agner.ch> writes:
> 
>> > According to the device tree bindings the vcc-supply is optional.
> 
> This is nonsense unless the device can work without this supply.  Given
> that the supply is called VCC that doesn't seem entirely likely.

Afaik it is kind of a generic device tree binding, I guess the physical
device can have various appearances and properties...

A quick survey showed several device trees which do not specify
vcc-supply...

That said, I checked the device at hand, and it actually has a USB PHY
power supply inputs, but the device tree does not model them.

>> > +	nop->vcc = devm_regulator_get_optional(dev, "vcc");
>> >  	if (IS_ERR(nop->vcc)) {
>> >  		dev_dbg(dev, "Error getting vcc regulator: %ld\n",
>> >  					PTR_ERR(nop->vcc));
>> > -		if (needs_vcc)
>> > -			return -EPROBE_DEFER;
>> > +		if (needs_vcc || PTR_ERR(nop->vcc) == -EPROBE_DEFER)
>> > +			return PTR_ERR(nop->vcc);
> 
>> does this look okay from a regulator API perspective?
> 
> That's how to use _get_optional() but it's really unusual that you
> should be using _get_optional().

Despite the above findings, I still think it is the right thing to do as
long as we specify vcc-supply to be optional.

--
Stefan

[toc] | [prev] | [next] | [standalone]

#1478021

Hi Stefan,

On 06/09/16 21:01, Stefan Agner wrote:
> On 2016-09-06 01:22, Mark Brown wrote:
>> On Tue, Sep 06, 2016 at 10:45:19AM +0300, Felipe Balbi wrote:
>>> Stefan Agner <stefan@agner.ch> writes:
>>
>>>> According to the device tree bindings the vcc-supply is optional.
>>
>> This is nonsense unless the device can work without this supply.  Given
>> that the supply is called VCC that doesn't seem entirely likely.
> 
> Afaik it is kind of a generic device tree binding, I guess the physical
> device can have various appearances and properties...
> 
> A quick survey showed several device trees which do not specify
> vcc-supply...
> 
> That said, I checked the device at hand, and it actually has a USB PHY
> power supply inputs, but the device tree does not model them.
> 
>>>> +	nop->vcc = devm_regulator_get_optional(dev, "vcc");
>>>>  	if (IS_ERR(nop->vcc)) {
>>>>  		dev_dbg(dev, "Error getting vcc regulator: %ld\n",
>>>>  					PTR_ERR(nop->vcc));
>>>> -		if (needs_vcc)
>>>> -			return -EPROBE_DEFER;
>>>> +		if (needs_vcc || PTR_ERR(nop->vcc) == -EPROBE_DEFER)
>>>> +			return PTR_ERR(nop->vcc);
>>
>>> does this look okay from a regulator API perspective?
>>
>> That's how to use _get_optional() but it's really unusual that you
>> should be using _get_optional().
> 
> Despite the above findings, I still think it is the right thing to do as
> long as we specify vcc-supply to be optional.
> 

I think the right behaviour would be that if vcc-supply is specified in the
DT then failure to get that supply is a serious failure and probe should fail.

So the correct fix would be to call devm_regulator_get() only if needs_vcc is true.

cheers,
-roger

[toc] | [prev] | [next] | [standalone]

#1478055

[Multipart message — attachments visible in raw view] — view raw

Hi,

Roger Quadros <rogerq@ti.com> writes:
>>>> Stefan Agner <stefan@agner.ch> writes:
>>>
>>>>> According to the device tree bindings the vcc-supply is optional.
>>>
>>> This is nonsense unless the device can work without this supply.  Given
>>> that the supply is called VCC that doesn't seem entirely likely.
>> 
>> Afaik it is kind of a generic device tree binding, I guess the physical
>> device can have various appearances and properties...
>> 
>> A quick survey showed several device trees which do not specify
>> vcc-supply...
>> 
>> That said, I checked the device at hand, and it actually has a USB PHY
>> power supply inputs, but the device tree does not model them.
>> 
>>>>> +	nop->vcc = devm_regulator_get_optional(dev, "vcc");
>>>>>  	if (IS_ERR(nop->vcc)) {
>>>>>  		dev_dbg(dev, "Error getting vcc regulator: %ld\n",
>>>>>  					PTR_ERR(nop->vcc));
>>>>> -		if (needs_vcc)
>>>>> -			return -EPROBE_DEFER;
>>>>> +		if (needs_vcc || PTR_ERR(nop->vcc) == -EPROBE_DEFER)
>>>>> +			return PTR_ERR(nop->vcc);
>>>
>>>> does this look okay from a regulator API perspective?
>>>
>>> That's how to use _get_optional() but it's really unusual that you
>>> should be using _get_optional().
>> 
>> Despite the above findings, I still think it is the right thing to do as
>> long as we specify vcc-supply to be optional.
>> 
>
> I think the right behaviour would be that if vcc-supply is specified
> in the DT then failure to get that supply is a serious failure and
> probe should fail.
>
> So the correct fix would be to call devm_regulator_get() only if
> needs_vcc is true.

The way it is, AFAICT, regulator fwk will return a dummy regulator for
cases where supply isn't in DT.

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1478557

[Multipart message — attachments visible in raw view] — view raw

On Tue, Sep 06, 2016 at 11:01:15AM -0700, Stefan Agner wrote:
> On 2016-09-06 01:22, Mark Brown wrote:

> > This is nonsense unless the device can work without this supply.  Given
> > that the supply is called VCC that doesn't seem entirely likely.

> Afaik it is kind of a generic device tree binding, I guess the physical
> device can have various appearances and properties...

Is it really realistic that a meaningful proportion of them will work
without power?

> A quick survey showed several device trees which do not specify
> vcc-supply...

The regulator framework will attempt to be forgiving in what it accepts,
the absence of a mandatory supply is sadly not a good indication that
the supply does not physically exist...

> That said, I checked the device at hand, and it actually has a USB PHY
> power supply inputs, but the device tree does not model them.

...like here.

> > That's how to use _get_optional() but it's really unusual that you
> > should be using _get_optional().

> Despite the above findings, I still think it is the right thing to do as
> long as we specify vcc-supply to be optional.

I disagree, and bear in mind that it is more complex all round to handle
optional supples - the reason they exist is that on devices where
supplies may be omitted you usually have to do some kind of special case
handling (like enabling internal regulators or something).  If you don't
have any such special case handling but instead simply omit enables and
disables then that's a fairly clear abuse of the API.

[toc] | [prev] | [next] | [standalone]

#1478604

On 2016-09-07 11:53, Mark Brown wrote:
> On Tue, Sep 06, 2016 at 11:01:15AM -0700, Stefan Agner wrote:
>> On 2016-09-06 01:22, Mark Brown wrote:
> 
>> > This is nonsense unless the device can work without this supply.  Given
>> > that the supply is called VCC that doesn't seem entirely likely.
> 
>> Afaik it is kind of a generic device tree binding, I guess the physical
>> device can have various appearances and properties...
> 
> Is it really realistic that a meaningful proportion of them will work
> without power?
> 

No IP in a SoC runs without power, but still we don't model the supply
to every IP....

I would have guessed that there are SoCs with an internal USB PHY which
are powered implicitly by the SoC, and/or it is unclear how they exactly
get powered... If we make the supply mandatory, we can only guess how it
is wired up and probably end up to assign some "global" SoC supply
regulator.

>> A quick survey showed several device trees which do not specify
>> vcc-supply...
> 
> The regulator framework will attempt to be forgiving in what it accepts,
> the absence of a mandatory supply is sadly not a good indication that
> the supply does not physically exist...
> 
>> That said, I checked the device at hand, and it actually has a USB PHY
>> power supply inputs, but the device tree does not model them.
> 
> ...like here.
> 
>> > That's how to use _get_optional() but it's really unusual that you
>> > should be using _get_optional().
> 
>> Despite the above findings, I still think it is the right thing to do as
>> long as we specify vcc-supply to be optional.
> 
> I disagree, and bear in mind that it is more complex all round to handle
> optional supples - the reason they exist is that on devices where
> supplies may be omitted you usually have to do some kind of special case
> handling (like enabling internal regulators or something).  If you don't
> have any such special case handling but instead simply omit enables and
> disables then that's a fairly clear abuse of the API.

Note that in this case the code already supports an optional supply e.g.
when it has not specified via platform data. The code does not do any
special in that case, it is assumed to be implicitly powered.

I am not really a USB PHY experts and don't have a good overview of what
is out there, I just did a short survey across the device trees we have,
and found some which were lacking the vcc-supply. I guess Felipe has a
better overview and just should make a call on that matter.

In case we decide to make vcc-supply mandatory for the device tree case,
I will send a new patch altering the bindings documentation accordingly
and get rid of the needs_vcc = of_property_read_bool(node,
"vcc-supply");. This should not break existing device trees since
devm_regulator_get returns the dummy regulators.

--
Stefan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web