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

Anand Moon linux.amoon at gmail.com
Thu Apr 4 23:10:18 PDT 2024


 Hi Christophe, Krzysztof,

On Mon, 4 Mar 2024 at 17:16, Anand Moon <linux.amoon at gmail.com> wrote:
>
> Hi Christophe,
>
> On Sun, 3 Mar 2024 at 00:07, Christophe JAILLET
> <christophe.jaillet at wanadoo.fr> 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 at wanadoo.fr> 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 at gmail.com>
> > >>> ---
> > >>>    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

I could not find any example in the kernel to support
devm_regulator_bulk_disable
but here is my modified file.

If you have any suggestions for this plz let me know.
-----8<----------8<----------
diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
index 6d07592ad022..2f808cb9a006 100644
--- a/drivers/usb/dwc3/dwc3-exynos.c
+++ b/drivers/usb/dwc3/dwc3-exynos.c
@@ -34,6 +34,8 @@ struct dwc3_exynos {
        int                     suspend_clk_idx;
 };

+static const char * const regulators[] = { "vdd33", "vdd10" };
+
 static int dwc3_exynos_probe(struct platform_device *pdev)
 {
        struct dwc3_exynos      *exynos;
@@ -41,7 +43,6 @@ 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)
@@ -166,6 +167,8 @@ static int __maybe_unused
dwc3_exynos_suspend(struct device *dev)
        struct dwc3_exynos *exynos = dev_get_drvdata(dev);
        int i;

+       devm_regulator_bulk_disable(dev);
+
        for (i = exynos->num_clks - 1; i >= 0; i--)
                clk_disable_unprepare(exynos->clks[i]);

@@ -177,6 +180,11 @@ static int __maybe_unused
dwc3_exynos_resume(struct device *dev)
        struct dwc3_exynos *exynos = dev_get_drvdata(dev);
        int i, ret;

+       ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulators),
+                                               regulators);
+       if (ret)
+               dev_err(dev, "Failed to enable regulators\n");
+
        for (i = 0; i < exynos->num_clks; i++) {
                ret = clk_prepare_enable(exynos->clks[i]);
                if (ret) {

To support these changes we need to export the
devm_regulator_bulk_disable function.
-----8<----------8<----------
diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c
index 90bb0d178885..97eed739f929 100644
--- a/drivers/regulator/devres.c
+++ b/drivers/regulator/devres.c
@@ -318,7 +318,7 @@ void devm_regulator_bulk_put(struct
regulator_bulk_data *consumers)
 }
 EXPORT_SYMBOL_GPL(devm_regulator_bulk_put);

-static void devm_regulator_bulk_disable(void *res)
+void devm_regulator_bulk_disable(void *res)
 {
        struct regulator_bulk_devres *devres = res;
        int i;
@@ -326,6 +326,7 @@ static void devm_regulator_bulk_disable(void *res)
        for (i = 0; i < devres->num_consumers; i++)
                regulator_disable(devres->consumers[i].consumer);
 }
+EXPORT_SYMBOL_GPL(devm_regulator_bulk_disable);

 /**
  * devm_regulator_bulk_get_enable - managed get'n enable multiple regulators
diff --git a/include/linux/regulator/consumer.h
b/include/linux/regulator/consumer.h
index 4660582a3302..ce7d28306b17 100644
--- a/include/linux/regulator/consumer.h
+++ b/include/linux/regulator/consumer.h
@@ -214,6 +214,7 @@ int __must_check regulator_bulk_enable(int num_consumers,
                                       struct regulator_bulk_data *consumers);
 int devm_regulator_bulk_get_enable(struct device *dev, int num_consumers,
                                   const char * const *id);
+void devm_regulator_bulk_disable(void *res);
 int regulator_bulk_disable(int num_consumers,
                           struct regulator_bulk_data *consumers);
 int regulator_bulk_force_disable(int num_consumers,
--------------------------------------------------------------------------

Thanks
-Anand



More information about the linux-arm-kernel mailing list