Re: [PATCH 1/3] usb: ohci-exynos: Make provision for vdd regulators

[Vivek Gautam]

Hi Jingoo,


On Thu, Apr 24, 2014 at 6:56 AM, Jingoo Han <jg1.han@xxxxxxxxxxx> wrote:
> On Thursday, April 24, 2014 9:33 AM, Jingoo Han wrote:
>> On Thursday, April 24, 2014 9:18 AM, Anton Tikhomirov wrote:
>> > On Monday, April 21, 2014 9:17 PM, Vivek Gautam wrote:
>> > >
>> > > Facilitate getting required 3.3V and 1.0V VDD supply for
>> > > OHCI controller on Exynos.
>> > >
>> > > With patches for regulators' nodes merged in 3.15:
>> > > c8c253f ARM: dts: Add regulator entries to smdk5420
>> > > 275dcd2 ARM: dts: add max77686 pmic node for smdk5250,
>> > >
>> > > certain perripherals will now need to ensure that,
>> > > they request VDD regulators in their drivers, and enable
>> > > them so as to make them working.
>> > >
>> > > Signed-off-by: Vivek Gautam <gautam.vivek@xxxxxxxxxxx>
>> > > Cc: Jingoo Han <jg1.han@xxxxxxxxxxx>
>> > > ---
>> > >
>> > > Based on 'usb-next' branch of Greg's usb tree.
>> > >
>> > >  drivers/usb/host/ohci-exynos.c |   47
>> > > ++++++++++++++++++++++++++++++++++++++++
>> > >  1 file changed, 47 insertions(+)
>> > >
>> > > diff --git a/drivers/usb/host/ohci-exynos.c b/drivers/usb/host/ohci-
>> > > exynos.c
>> > > index 68588d8..e2e72a8 100644
>> > > --- a/drivers/usb/host/ohci-exynos.c
>> > > +++ b/drivers/usb/host/ohci-exynos.c

[snip]

>> > > @@ -98,6 +101,28 @@ static int exynos_ohci_probe(struct platform_device
>> > > *pdev)
>> > >           exynos_ohci->otg = phy->otg;
>> > >   }
>> > >
>> > > + exynos_ohci->vdd33 = devm_regulator_get(&pdev->dev, "vdd33");
>> > > + if (IS_ERR(exynos_ohci->vdd33)) {
>> > > +         err = PTR_ERR(exynos_ohci->vdd33);
>> > > +         goto fail_regulator1;
>> > > + }
>> > > + err = regulator_enable(exynos_ohci->vdd33);
>> > > + if (err) {
>> > > +         dev_err(&pdev->dev, "Failed to enable VDD33 supply\n");
>> > > +         goto fail_regulator1;
>> > > + }
>> > > +
>> > > + exynos_ohci->vdd10 = devm_regulator_get(&pdev->dev, "vdd10");
>> > > + if (IS_ERR(exynos_ohci->vdd10)) {
>> > > +         err = PTR_ERR(exynos_ohci->vdd10);
>> > > +         goto fail_regulator2;
>> > > + }
>> > > + err = regulator_enable(exynos_ohci->vdd10);
>> > > + if (err) {
>> > > +         dev_err(&pdev->dev, "Failed to enable VDD10 supply\n");
>> > > +         goto fail_regulator2;
>> > > + }
>> > > +
>> >
>> > Do we need to skip regulator settings together with PHY configuration
>> > in case of exynos5440?
>>
>> Oh, right. In the case of exynos5440, regulator settings is not
>> necessary. Vivek, would you fix it in order skip regulator settings
>> in exynos5440? It also applies to ehci-exynos.
>
> Sorry, in the case of exynos5440, this patch already skips
> regulator settings.
>
> In the case of exynos5440, there is no need to set PHY setting
> and regulator setting.

Right, in case of exynos5440, we are skipping PHY setting and regulator setting.
Actually i had missed taking into account 5440, so just curious. Do we
really not need a regulator
settings for Exynos5440 ?

>
> How about making regulator setting "optional"?
> Then, regulator setting can be done, only when regulator
> is supported.

True, so with Exynos5440 not needing the regulator, we should make the
regulator settings optional.

>
> exynos_ohci_probe()
>         exynos_ohci->vdd33 = devm_regulator_get(&pdev->dev, "vdd33");
>         if (IS_ERR(exynos_ohci->vdd33)) {
>                 dev_err(&pdev->dev, "Failed to get VDD33 supply\n");
>         } else {
>                 err = regulator_enable(exynos_ohci->vdd33);
>                 if (err) {
>                         dev_err(&pdev->dev, "Failed to enable VDD33 supply\n");
>                         goto fail_regulator1;
>                 }
>         }
>
>         exynos_ohci->vdd10 = devm_regulator_get(&pdev->dev, "vdd10");
>         if (IS_ERR(exynos_ohci->vdd10)) {
>                 dev_err(&pdev->dev, "Failed to get VDD10 supply\n");
>         } else {
>                 err = regulator_enable(exynos_ohci->vdd10);
>                 if (err) {
>                         dev_err(&pdev->dev, "Failed to enable VDD10 supply\n");
>                         goto fail_regulator2;
>                 }
>         }
>
> In this case, suspend/resume can be fixed as below.
>
> exynos_ohci_suspend()
>         if (exynos_ohci->vdd10)
>                 regulator_disable(exynos_ohci->vdd10);
>         if (exynos_ohci->vdd33)
>                 regulator_disable(exynos_ohci->vdd33);
>
> exynos_ohci_resume()
>
>         if (exynos_ohci->vdd33) {
>                 ret = regulator_enable(exynos_ohci->vdd33);
>                 if (ret) {
>                         dev_err(dev, "Failed to enable VDD33 supply\n");
>                         return ret;
>                 }
>         }
>         if (exynos_ohci->vdd10) {
>                 ret = regulator_enable(exynos_ohci->vdd10);
>                 if (ret) {
>                         dev_err(dev, "Failed to enable VDD10 supply\n");
>                         return ret;
>                 }
>         }

Thanks for the suggestion.
I will make the required changes, and post the patchset again.

[snip]


-- 
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/