| Message ID | 20240404071350.4242-6-linux.amoon@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/6] usb: ehci-exynos: Use devm_clk_get_enabled() helpers | expand |
On 04/04/2024 09:13, Anand Moon wrote: > Use devm_regulator_bulk_get_enable() instead of open coded > 'devm_regulator_get(), regulator_enable(), regulator_disable(). I fail to see how did you replace open-coded suspend/resume paths. > > Signed-off-by: Anand Moon <linux.amoon@gmail.com> > --- > V2: no changes, did not find any regression in pm suspend/resume. No, that's not equivalent code. No explanation in commit msg. You already got comments on this and nothing improved. You just entirely ignored received comments. That's not how it works. I don't think you understand the code and Linux driver model. This patch repeats several previous attempts with similar issues: no logic behind a change. NAK. Best regards, Krzysztof
Hi Krzysztof, On Thu, 4 Apr 2024 at 12:53, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 04/04/2024 09:13, Anand Moon wrote: > > Use devm_regulator_bulk_get_enable() instead of open coded > > 'devm_regulator_get(), regulator_enable(), regulator_disable(). > > I fail to see how did you replace open-coded suspend/resume paths. > > > > > Signed-off-by: Anand Moon <linux.amoon@gmail.com> > > --- > > V2: no changes, did not find any regression in pm suspend/resume. > > No, that's not equivalent code. No explanation in commit msg. > > You already got comments on this and nothing improved. You just entirely > ignored received comments. That's not how it works. > > I don't think you understand the code and Linux driver model. This patch > repeats several previous attempts with similar issues: no logic behind a > change. > > NAK. devm_regulator_get_enable and devm_regulator_bulk_get_enable both remove the dependency from the driver to handle the regulator_enabled and regulator_disabled. ie this removes the regulator from the driver structure. Since these functions set devm_add_action to disable the regulator when the resource is not used. ret = devm_add_action(dev, devm_regulator_bulk_disable, devres); if (!ret) return 0; > > Best regards, > Krzysztof > if you feel it's incorrect, I will drop this patch.. Thanks -Anand
On 04/04/2024 09:38, Anand Moon wrote: > Hi Krzysztof, > > On Thu, 4 Apr 2024 at 12:53, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 04/04/2024 09:13, Anand Moon wrote: >>> Use devm_regulator_bulk_get_enable() instead of open coded >>> 'devm_regulator_get(), regulator_enable(), regulator_disable(). >> >> I fail to see how did you replace open-coded suspend/resume paths. >> >>> >>> Signed-off-by: Anand Moon <linux.amoon@gmail.com> >>> --- >>> V2: no changes, did not find any regression in pm suspend/resume. >> >> No, that's not equivalent code. No explanation in commit msg. >> >> You already got comments on this and nothing improved. You just entirely >> ignored received comments. That's not how it works. >> >> I don't think you understand the code and Linux driver model. This patch >> repeats several previous attempts with similar issues: no logic behind a >> change. >> >> NAK. > > devm_regulator_get_enable and devm_regulator_bulk_get_enable > both remove the dependency from the driver to handle the regulator_enabled > and regulator_disabled. ie this removes the regulator from the driver structure. Not true. Please do not paste some generic knowledge and assume reviewer knows it. Instead provide proof. > > Since these functions set devm_add_action to disable the regulator when the > resource is not used. > > ret = devm_add_action(dev, devm_regulator_bulk_disable, devres); > if (!ret) > return 0; Listen, you already got comments on this at v1. Address previous comments instead of repeating something unrelated. We should not have the same discussion twice. Best regards, Krzysztof
diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c index 5d365ca51771..2d341f0e22a3 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,10 @@ 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 +96,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 +117,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 +170,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); - return 0; } @@ -207,17 +178,6 @@ static int dwc3_exynos_resume(struct device *dev) struct dwc3_exynos *exynos = dev_get_drvdata(dev); int i, ret; - ret = regulator_enable(exynos->vdd33); - if (ret) { - dev_err(dev, "Failed to enable VDD33 supply\n"); - return ret; - } - ret = regulator_enable(exynos->vdd10); - if (ret) { - dev_err(dev, "Failed to enable VDD10 supply\n"); - return ret; - } - for (i = 0; i < exynos->num_clks; i++) { ret = clk_prepare_enable(exynos->clks[i]); if (ret) {
Use devm_regulator_bulk_get_enable() instead of open coded 'devm_regulator_get(), regulator_enable(), regulator_disable(). Signed-off-by: Anand Moon <linux.amoon@gmail.com> --- V2: no changes, did not find any regression in pm suspend/resume. --- drivers/usb/dwc3/dwc3-exynos.c | 50 ++++------------------------------ 1 file changed, 5 insertions(+), 45 deletions(-)