Re: [PATCH v1 3/4] usb: dwc3: exynos: Use devm_regulator_bulk_get_enable() helper function

[Anand Moon]

Hi Christophe,

On Sun, 3 Mar 2024 at 00:07, Christophe JAILLET
<christophe.jaillet@xxxxxxxxxx> wrote:
>
> Le 02/03/2024 à 17:48, Anand Moon a écrit :
> > Hi Christophe,
> >
> > On Sat, 2 Mar 2024 at 21:20, Christophe JAILLET
> > <christophe.jaillet@xxxxxxxxxx> wrote:
> >>
> >> Le 01/03/2024 à 20:38, Anand Moon a écrit :
> >>> Use devm_regulator_bulk_get_enable() instead of open coded
> >>> 'devm_regulator_get(), regulator_enable(), regulator_disable().
> >>>
> >>> Signed-off-by: Anand Moon <linux.amoon@xxxxxxxxx>
> >>> ---
> >>>    drivers/usb/dwc3/dwc3-exynos.c | 49 +++-------------------------------
> >>>    1 file changed, 4 insertions(+), 45 deletions(-)
> >>>
> >>> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
> >>> index 5d365ca51771..7c77f3c69825 100644
> >>> --- a/drivers/usb/dwc3/dwc3-exynos.c
> >>> +++ b/drivers/usb/dwc3/dwc3-exynos.c
> >>> @@ -32,9 +32,6 @@ struct dwc3_exynos {
> >>>        struct clk              *clks[DWC3_EXYNOS_MAX_CLOCKS];
> >>>        int                     num_clks;
> >>>        int                     suspend_clk_idx;
> >>> -
> >>> -     struct regulator        *vdd33;
> >>> -     struct regulator        *vdd10;
> >>>    };
> >>>
> >>>    static int dwc3_exynos_probe(struct platform_device *pdev)
> >>> @@ -44,6 +41,7 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
> >>>        struct device_node      *node = dev->of_node;
> >>>        const struct dwc3_exynos_driverdata *driver_data;
> >>>        int                     i, ret;
> >>> +     static const char * const regulators[] = { "vdd33", "vdd10" };
> >>>
> >>>        exynos = devm_kzalloc(dev, sizeof(*exynos), GFP_KERNEL);
> >>>        if (!exynos)
> >>> @@ -78,27 +76,9 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
> >>>        if (exynos->suspend_clk_idx >= 0)
> >>>                clk_prepare_enable(exynos->clks[exynos->suspend_clk_idx]);
> >>>
> >>> -     exynos->vdd33 = devm_regulator_get(dev, "vdd33");
> >>> -     if (IS_ERR(exynos->vdd33)) {
> >>> -             ret = PTR_ERR(exynos->vdd33);
> >>> -             goto vdd33_err;
> >>> -     }
> >>> -     ret = regulator_enable(exynos->vdd33);
> >>> -     if (ret) {
> >>> -             dev_err(dev, "Failed to enable VDD33 supply\n");
> >>> -             goto vdd33_err;
> >>> -     }
> >>> -
> >>> -     exynos->vdd10 = devm_regulator_get(dev, "vdd10");
> >>> -     if (IS_ERR(exynos->vdd10)) {
> >>> -             ret = PTR_ERR(exynos->vdd10);
> >>> -             goto vdd10_err;
> >>> -     }
> >>> -     ret = regulator_enable(exynos->vdd10);
> >>> -     if (ret) {
> >>> -             dev_err(dev, "Failed to enable VDD10 supply\n");
> >>> -             goto vdd10_err;
> >>> -     }
> >>> +     ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulators), regulators);
> >>> +     if (ret)
> >>> +             return dev_err_probe(dev, ret, "Failed to enable regulators\n");
> >>>
> >>>        if (node) {
> >>>                ret = of_platform_populate(node, NULL, NULL, dev);
> >>> @@ -115,10 +95,6 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
> >>>        return 0;
> >>>
> >>>    populate_err:
> >>> -     regulator_disable(exynos->vdd10);
> >>> -vdd10_err:
> >>> -     regulator_disable(exynos->vdd33);
> >>> -vdd33_err:
> >>>        for (i = exynos->num_clks - 1; i >= 0; i--)
> >>>                clk_disable_unprepare(exynos->clks[i]);
> >>>
> >>> @@ -140,9 +116,6 @@ static void dwc3_exynos_remove(struct platform_device *pdev)
> >>>
> >>>        if (exynos->suspend_clk_idx >= 0)
> >>>                clk_disable_unprepare(exynos->clks[exynos->suspend_clk_idx]);
> >>> -
> >>> -     regulator_disable(exynos->vdd33);
> >>> -     regulator_disable(exynos->vdd10);
> >>>    }
> >>>
> >>>    static const struct dwc3_exynos_driverdata exynos5250_drvdata = {
> >>> @@ -196,9 +169,6 @@ static int dwc3_exynos_suspend(struct device *dev)
> >>>        for (i = exynos->num_clks - 1; i >= 0; i--)
> >>>                clk_disable_unprepare(exynos->clks[i]);
> >>>
> >>> -     regulator_disable(exynos->vdd33);
> >>> -     regulator_disable(exynos->vdd10);
> >>
> >> Hi,
> >>
> >> Same here, I don't think that removing regulator_[en|dis]able from the
> >> suspend and resume function is correct.
> >>
> >> The goal is to stop some hardware when the system is suspended, in order
> >> to save some power.
> > Ok,
> >>
> >> Why did you removed it?
> >
> > As per the description of the function  devm_regulator_bulk_get_enable
> >
> > * This helper function allows drivers to get several regulator
> >   * consumers in one operation with management, the regulators will
> >   * automatically be freed when the device is unbound.  If any of the
> >   * regulators cannot be acquired then any regulators that were
> >   * allocated will be freed before returning to the caller.
>
> The code in suspend/resume is not about freeing some resources. It is
> about enabling/disabling some hardware to save some power.
>
> Think to the probe/remove functions as the software in the kernel that
> knows how to handle some hardawre, and the suspend/resume as the on/off
> button to power-on and off the electrical chips.
>
> When the system is suspended, the software is still around. But some
> hardware can be set in a low consumption mode to save some power.
>
> IMHO, part of the code you removed changed this behaviour and increase
> the power consumption when the system is suspended.
>

You are correct, I have changed the regulator API from
devm_regulator_get_enable to devm_regulator_bulk_get_enable
which changes this behavior.
I will fix it in the next version.

> CJ

Thanks
-Anand