[PATCH v2 1/3] pmdomain: imx93-pd: replace dev_err() with dev_err_probe()

[PATCH v2 1/3] pmdomain: imx93-pd: replace dev_err() with dev_err_probe()

[Dario Binacchi]

This way, the code becomes more compact, and dev_err_probe() is used in
every error path of the probe() function.

Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

---

Changes in v2:
- Drop the extra { } to be even more compact.

 drivers/pmdomain/imx/imx93-pd.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/pmdomain/imx/imx93-pd.c b/drivers/pmdomain/imx/imx93-pd.c
index d750a7dc58d2..44daecbe5cc3 100644
--- a/drivers/pmdomain/imx/imx93-pd.c
+++ b/drivers/pmdomain/imx/imx93-pd.c
@@ -125,11 +125,10 @@ static int imx93_pd_probe(struct platform_device *pdev)
 	/* Just to sync the status of hardware */
 	if (!domain->init_off) {
 		ret = clk_bulk_prepare_enable(domain->num_clks, domain->clks);
-		if (ret) {
-			dev_err(domain->dev, "failed to enable clocks for domain: %s\n",
-				domain->genpd.name);
-			return ret;
-		}
+		if (ret)
+			return dev_err_probe(domain->dev, ret,
+					     "failed to enable clocks for domain: %s\n",
+					     domain->genpd.name);
 	}
 
 	ret = pm_genpd_init(&domain->genpd, NULL, domain->init_off);
-- 
2.43.0

[PATCH v2 2/3] pmdomain: imx93-pd: don't unprepare clocks on driver remove

[Dario Binacchi]

The removed code was added to handle the case where the power domain is
already on during the driver's probing. In this use case, the "is_off"
parameter is passed as false to pm_genpd_init() to inform it not to call
the power_on() callback, as it's unnecessary to perform the hardware
power-on procedure since the power domain is already on. Therefore, with
the call to clk_bulk_prepare_enable() by probe(), the system is in the
same operational state as when "is_off" is passed as true after the
power_on() callback execution:

 probe() -> is_off == true  -> clk_bulk_prepare_enable() called by power_on()
 probe() -> is_off == false -> clk_bulk_prepare_enable() called by probe()

Reaching the same logical and operational state, it follows that upon
driver removal, there is no need to perform different actions depending
on the power domain's on/off state during probing.

Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
---

(no changes since v1)

 drivers/pmdomain/imx/imx93-pd.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/pmdomain/imx/imx93-pd.c b/drivers/pmdomain/imx/imx93-pd.c
index 44daecbe5cc3..fb53a8e359bc 100644
--- a/drivers/pmdomain/imx/imx93-pd.c
+++ b/drivers/pmdomain/imx/imx93-pd.c
@@ -90,9 +90,6 @@ static void imx93_pd_remove(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 
-	if (!domain->init_off)
-		clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
-
 	of_genpd_del_provider(np);
 	pm_genpd_remove(&domain->genpd);
 }
-- 
2.43.0

Re: [PATCH v2 2/3] pmdomain: imx93-pd: don't unprepare clocks on driver remove

[Ulf Hansson]

On Sun, 25 Aug 2024 at 16:34, Dario Binacchi
<dario.binacchi@amarulasolutions.com> wrote:
>
> The removed code was added to handle the case where the power domain is
> already on during the driver's probing. In this use case, the "is_off"
> parameter is passed as false to pm_genpd_init() to inform it not to call
> the power_on() callback, as it's unnecessary to perform the hardware
> power-on procedure since the power domain is already on. Therefore, with
> the call to clk_bulk_prepare_enable() by probe(), the system is in the
> same operational state as when "is_off" is passed as true after the
> power_on() callback execution:
>
>  probe() -> is_off == true  -> clk_bulk_prepare_enable() called by power_on()
>  probe() -> is_off == false -> clk_bulk_prepare_enable() called by probe()
>
> Reaching the same logical and operational state, it follows that upon
> driver removal, there is no need to perform different actions depending
> on the power domain's on/off state during probing.

Well, I guess in most cases genpd would have tried to turn off the PM
domain when it is getting removed. However, there are really no
guarantees for that to happen. We should probably look into that as a
general improvement for genpd.

>
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

The above said, this still looks like an improvement to me, so I
decided to queue this up for next, thanks!

Kind regards
Uffe


> ---
>
> (no changes since v1)
>
>  drivers/pmdomain/imx/imx93-pd.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/pmdomain/imx/imx93-pd.c b/drivers/pmdomain/imx/imx93-pd.c
> index 44daecbe5cc3..fb53a8e359bc 100644
> --- a/drivers/pmdomain/imx/imx93-pd.c
> +++ b/drivers/pmdomain/imx/imx93-pd.c
> @@ -90,9 +90,6 @@ static void imx93_pd_remove(struct platform_device *pdev)
>         struct device *dev = &pdev->dev;
>         struct device_node *np = dev->of_node;
>
> -       if (!domain->init_off)
> -               clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
> -
>         of_genpd_del_provider(np);
>         pm_genpd_remove(&domain->genpd);
>  }
> --
> 2.43.0
>

[PATCH v2 3/3] pmdomain: imx93-pd: drop the context variable "init_off"

[Dario Binacchi]

This variable is only used within the probe() function, so let's remove
it from the context and define it locally within the same function.

Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

---

(no changes since v1)

 drivers/pmdomain/imx/imx93-pd.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pmdomain/imx/imx93-pd.c b/drivers/pmdomain/imx/imx93-pd.c
index fb53a8e359bc..25ab592945bd 100644
--- a/drivers/pmdomain/imx/imx93-pd.c
+++ b/drivers/pmdomain/imx/imx93-pd.c
@@ -28,7 +28,6 @@ struct imx93_power_domain {
 	void __iomem *addr;
 	struct clk_bulk_data *clks;
 	int num_clks;
-	bool init_off;
 };
 
 #define to_imx93_pd(_genpd) container_of(_genpd, struct imx93_power_domain, genpd)
@@ -99,6 +98,7 @@ static int imx93_pd_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	struct imx93_power_domain *domain;
+	bool init_off;
 	int ret;
 
 	domain = devm_kzalloc(dev, sizeof(*domain), GFP_KERNEL);
@@ -118,9 +118,9 @@ static int imx93_pd_probe(struct platform_device *pdev)
 	domain->genpd.power_on = imx93_pd_on;
 	domain->dev = dev;
 
-	domain->init_off = readl(domain->addr + MIX_FUNC_STAT_OFF) & FUNC_STAT_ISO_STAT_MASK;
+	init_off = readl(domain->addr + MIX_FUNC_STAT_OFF) & FUNC_STAT_ISO_STAT_MASK;
 	/* Just to sync the status of hardware */
-	if (!domain->init_off) {
+	if (!init_off) {
 		ret = clk_bulk_prepare_enable(domain->num_clks, domain->clks);
 		if (ret)
 			return dev_err_probe(domain->dev, ret,
@@ -128,7 +128,7 @@ static int imx93_pd_probe(struct platform_device *pdev)
 					     domain->genpd.name);
 	}
 
-	ret = pm_genpd_init(&domain->genpd, NULL, domain->init_off);
+	ret = pm_genpd_init(&domain->genpd, NULL, init_off);
 	if (ret)
 		goto err_clk_unprepare;
 
@@ -144,7 +144,7 @@ static int imx93_pd_probe(struct platform_device *pdev)
 	pm_genpd_remove(&domain->genpd);
 
 err_clk_unprepare:
-	if (!domain->init_off)
+	if (!init_off)
 		clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
 
 	return ret;
-- 
2.43.0

Re: [PATCH v2 3/3] pmdomain: imx93-pd: drop the context variable "init_off"

[Ulf Hansson]

On Sun, 25 Aug 2024 at 16:34, Dario Binacchi
<dario.binacchi@amarulasolutions.com> wrote:
>
> This variable is only used within the probe() function, so let's remove
> it from the context and define it locally within the same function.
>
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

Applied for next, thanks!

Kind regards
Uffe


>
> ---
>
> (no changes since v1)
>
>  drivers/pmdomain/imx/imx93-pd.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pmdomain/imx/imx93-pd.c b/drivers/pmdomain/imx/imx93-pd.c
> index fb53a8e359bc..25ab592945bd 100644
> --- a/drivers/pmdomain/imx/imx93-pd.c
> +++ b/drivers/pmdomain/imx/imx93-pd.c
> @@ -28,7 +28,6 @@ struct imx93_power_domain {
>         void __iomem *addr;
>         struct clk_bulk_data *clks;
>         int num_clks;
> -       bool init_off;
>  };
>
>  #define to_imx93_pd(_genpd) container_of(_genpd, struct imx93_power_domain, genpd)
> @@ -99,6 +98,7 @@ static int imx93_pd_probe(struct platform_device *pdev)
>         struct device *dev = &pdev->dev;
>         struct device_node *np = dev->of_node;
>         struct imx93_power_domain *domain;
> +       bool init_off;
>         int ret;
>
>         domain = devm_kzalloc(dev, sizeof(*domain), GFP_KERNEL);
> @@ -118,9 +118,9 @@ static int imx93_pd_probe(struct platform_device *pdev)
>         domain->genpd.power_on = imx93_pd_on;
>         domain->dev = dev;
>
> -       domain->init_off = readl(domain->addr + MIX_FUNC_STAT_OFF) & FUNC_STAT_ISO_STAT_MASK;
> +       init_off = readl(domain->addr + MIX_FUNC_STAT_OFF) & FUNC_STAT_ISO_STAT_MASK;
>         /* Just to sync the status of hardware */
> -       if (!domain->init_off) {
> +       if (!init_off) {
>                 ret = clk_bulk_prepare_enable(domain->num_clks, domain->clks);
>                 if (ret)
>                         return dev_err_probe(domain->dev, ret,
> @@ -128,7 +128,7 @@ static int imx93_pd_probe(struct platform_device *pdev)
>                                              domain->genpd.name);
>         }
>
> -       ret = pm_genpd_init(&domain->genpd, NULL, domain->init_off);
> +       ret = pm_genpd_init(&domain->genpd, NULL, init_off);
>         if (ret)
>                 goto err_clk_unprepare;
>
> @@ -144,7 +144,7 @@ static int imx93_pd_probe(struct platform_device *pdev)
>         pm_genpd_remove(&domain->genpd);
>
>  err_clk_unprepare:
> -       if (!domain->init_off)
> +       if (!init_off)
>                 clk_bulk_disable_unprepare(domain->num_clks, domain->clks);
>
>         return ret;
> --
> 2.43.0
>

Re: [PATCH v2 1/3] pmdomain: imx93-pd: replace dev_err() with dev_err_probe()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 590 bytes --]

Hello,

On Sun, Aug 25, 2024 at 04:34:00PM +0200, Dario Binacchi wrote:
> This way, the code becomes more compact, and dev_err_probe() is used in
> every error path of the probe() function.
> 
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

Another improvement worth mentioning is that the new output mentions the
error code returned by clk_bulk_prepare_enable(). If you respin this
series you might want to consider adding that to the commit log. Even
without that added:

Acked-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/3] pmdomain: imx93-pd: replace dev_err() with dev_err_probe()

[Ulf Hansson]

On Sun, 25 Aug 2024 at 16:34, Dario Binacchi
<dario.binacchi@amarulasolutions.com> wrote:
>
> This way, the code becomes more compact, and dev_err_probe() is used in
> every error path of the probe() function.
>
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>

Applied for next, thanks!

Kind regards
Uffe


>
> ---
>
> Changes in v2:
> - Drop the extra { } to be even more compact.
>
>  drivers/pmdomain/imx/imx93-pd.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pmdomain/imx/imx93-pd.c b/drivers/pmdomain/imx/imx93-pd.c
> index d750a7dc58d2..44daecbe5cc3 100644
> --- a/drivers/pmdomain/imx/imx93-pd.c
> +++ b/drivers/pmdomain/imx/imx93-pd.c
> @@ -125,11 +125,10 @@ static int imx93_pd_probe(struct platform_device *pdev)
>         /* Just to sync the status of hardware */
>         if (!domain->init_off) {
>                 ret = clk_bulk_prepare_enable(domain->num_clks, domain->clks);
> -               if (ret) {
> -                       dev_err(domain->dev, "failed to enable clocks for domain: %s\n",
> -                               domain->genpd.name);
> -                       return ret;
> -               }
> +               if (ret)
> +                       return dev_err_probe(domain->dev, ret,
> +                                            "failed to enable clocks for domain: %s\n",
> +                                            domain->genpd.name);
>         }
>
>         ret = pm_genpd_init(&domain->genpd, NULL, domain->init_off);
> --
> 2.43.0
>