Re: [PATCH 04/14] usb: phy: nop: Handle power supply regulator for the PHY

[Roger Quadros <rogerq@ti.com>]

On 01/14/2013 01:25 PM, Russell King - ARM Linux wrote:
> On Mon, Jan 14, 2013 at 11:54:42AM +0200, Roger Quadros wrote:
>> On 01/11/2013 07:17 PM, Russell King - ARM Linux wrote:
>>> On Thu, Jan 10, 2013 at 06:51:24PM +0200, Roger Quadros wrote:
>>>> We use "vcc" as the supply name for the PHY's power supply.
>>>> The power supply will be enabled during .init() and disabled
>>>> during .shutdown()
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>> ---
>>>>  drivers/usb/otg/nop-usb-xceiv.c |   18 ++++++++++++++++++
>>>>  1 files changed, 18 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/otg/nop-usb-xceiv.c b/drivers/usb/otg/nop-usb-xceiv.c
>>>> index 163f972..1c6db10 100644
>>>> --- a/drivers/usb/otg/nop-usb-xceiv.c
>>>> +++ b/drivers/usb/otg/nop-usb-xceiv.c
>>>> @@ -33,11 +33,13 @@
>>>>  #include <linux/usb/nop-usb-xceiv.h>
>>>>  #include <linux/slab.h>
>>>>  #include <linux/clk.h>
>>>> +#include <linux/regulator/consumer.h>
>>>>  
>>>>  struct nop_usb_xceiv {
>>>>  	struct usb_phy		phy;
>>>>  	struct device		*dev;
>>>>  	struct clk		*clk;
>>>> +	struct regulator	*vcc;
>>>>  };
>>>>  
>>>>  static struct platform_device *pd;
>>>> @@ -70,6 +72,11 @@ static int nop_init(struct usb_phy *phy)
>>>>  {
>>>>  	struct nop_usb_xceiv *nop = dev_get_drvdata(phy->dev);
>>>>  
>>>> +	if (nop->vcc) {
>>>> +		if (regulator_enable(nop->vcc))
>>>> +			dev_err(phy->dev, "Failed to enable power\n");
>>>> +	}
>>>> +
>>>>  	if (nop->clk)
>>>>  		clk_enable(nop->clk);
>>>>  
>>>> @@ -82,6 +89,11 @@ static void nop_shutdown(struct usb_phy *phy)
>>>>  
>>>>  	if (nop->clk)
>>>>  		clk_disable(nop->clk);
>>>> +
>>>> +	if (nop->vcc) {
>>>> +		if (regulator_disable(nop->vcc))
>>>> +			dev_err(phy->dev, "Failed to disable power\n");
>>>> +	}
>>>>  }
>>>>  
>>>>  static int nop_set_peripheral(struct usb_otg *otg, struct usb_gadget *gadget)
>>>> @@ -157,6 +169,12 @@ static int nop_usb_xceiv_probe(struct platform_device *pdev)
>>>>  		}
>>>>  	}
>>>>  
>>>> +	nop->vcc = devm_regulator_get(&pdev->dev, "vcc");
>>>> +	if (IS_ERR(nop->vcc)) {
>>>> +		dev_dbg(&pdev->dev, "Error getting vcc regulator\n");
>>>> +		nop->vcc = NULL;
>>>> +	}
>>>
>>> Is it really appropriate for drivers to do this kind of thing with
>>> pointer-returning functions (I mean, setting the pointer to NULL on
>>> error, rather than just using a test for IS_ERR() in the above
>>> locations).  You are imposing driver-local assumptions on an API.
>>>
>>> Practically it probably doesn't make much difference but given the
>>> amount of mistakes that we have with IS_ERR_OR_NULL()...
>>>
>> Makes sense. I'll convert it to use IS_ERR_OR_NULL() throughout.
> 
> IS_ERR_OR_NULL() is going to be deprecated and removed.  Please take a
> look at how these APIs work.

OK. I misunderstood what you meant earlier.

> 
> If regulator support is not enabled, then devm_regulator_get() returns
> NULL, and all the other regulator functions become no-ops.  This is not
> an error.  Do you need the driver to error out if CONFIG_REGULATOR is
> disabled?
> 
No, in fact even if CONFIG_REGULATOR is enabled and regulator_get()
fails, we just print a debug message and continue as usual. So
CONFIG_REGULATOR disabled _and_ regulator_get() error are handled in the
same way.

> If regulator support is enabled, then it can return a pointer-error
> value (as defined when IS_ERR() is true, and _in that case only_ can it
> be decoded by PTR_ERR() - PTR_ERR() is _only_ valid when IS_ERR()
> returns true - it is _not_ valid for all the cases that IS_ERR_OR_NULL()
> returns true.)  Otherwise, it returns a cookie as far as the driver is
> concerned for the regulator suitable for passing into the other regulator
> APIs.
> 
> By having the driver do something different, and make use of NULL as its
> own special sentinel, it's placing additional interpretations on the API,
> which can lead to bugs.  Don't do this.

Agreed.

> 
> If you can start making use of 'nop' vcc/clk members before they've been
> "got" then initialize them to ERR_PTR(-EINVAL) first.
> 
> Also consider that when using the devm interfaces, you can do:
> 
> 
> 	nop->clk = devm_clk_get(&pdev->dev, ...);
> 	nop->vcc = devm_regulator_get(&pdev->dev, "vcc");
> 
> 	if (IS_ERR(nop->clk))
> 		dev_dbg(&pdev->dev, "unable to get clock: %d\n",
> 			PTR_ERR(nop->clk));
> 
> 	if (IS_ERR(nop->vcc))
> 		dev_dbg(&pdev->dev, "unable to get vcc regulator: %d\n",
> 			PTR_ERR(nop->vcc));
> ...
> 	if (!IS_ERR(nop->clk))
> 		clk_enable(nop->clk);

OK I get it now. The only case where nop->clk or nop->vcc will be NULL
is if the framework is not enabled. It is fine to call the
regulator/clock APIs with NULL pointers in that case.

> 
> You may also consider that if you're going to print a warning for
> regulator_enable(), that you should print the error code as well, so that
> you know the reason why the failure happened.

OK.
> 
> Also consider... is dev_err() appropriate for an "error", for which you
> print a message and continue as if nothing went wrong.  To me that sounds
> more like a warning than an error, so maybe dev_warn() would be more
> appropriate?
> 
I used dev_dbg(), because we don't treat not getting the power supply
regulator as that serious.

--
cheers,
-roger