Thread (11 messages) 11 messages, 3 authors, 2018-10-10
STALE2799d
Revisions (19)
  1. v1 [diff vs current]
  2. v1 [diff vs current]
  3. v1 [diff vs current]
  4. v1 [diff vs current]
  5. v1 [diff vs current]
  6. v2 [diff vs current]
  7. v3 [diff vs current]
  8. v4 [diff vs current]
  9. v4 [diff vs current]
  10. v4 [diff vs current]
  11. v4 current
  12. v4 [diff vs current]
  13. v4 [diff vs current]
  14. v4 [diff vs current]
  15. v5 [diff vs current]
  16. v6 [diff vs current]
  17. v7 [diff vs current]
  18. v8 [diff vs current]
  19. v9 [diff vs current]

[PATCH V4 2/2] firmware: imx: add SCU power domain driver

From: s.hauer@pengutronix.de (Sascha Hauer)
Date: 2018-10-09 10:35:10
Also in: linux-pm

Hi Dong,

Looks mostly ok to me, some minor comments inside.

On Sun, Oct 07, 2018 at 09:10:07PM +0800, Dong Aisheng wrote:
Some i.MX SoCs contain a system controller that is responsible for
controlling the state of the IPs that are present. Communication
between the host processor running an OS and the system controller
happens through a SCU protocol. This patch adds SCU protocol based
power domains drivers.

Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Fabio Estevam <redacted>
Cc: "Rafael J. Wysocki" <redacted>
Cc: Kevin Hilman <khilman@kernel.org>
Cc: linux-pm at vger.kernel.org
Reviewed-by: Ulf Hansson <redacted>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
+static struct generic_pm_domain *imx_sc_pm_add_one_domain(
+					struct device_node *np,
+					struct generic_pm_domain *genpd_parent)
+{
+	struct imx_sc_pm_domain *imx_sc_pd;
+	u32 rsrc_id;
+	int ret;
+
+	imx_sc_pd = kzalloc(sizeof(*imx_sc_pd), GFP_KERNEL);
+	if (!imx_sc_pd)
+		return ERR_PTR(-ENOMEM);
+
+	if (!of_property_read_u32(np, "reg", &rsrc_id)) {
+		if (rsrc_id > IMX_SC_R_LAST) {
Should this be >= instead?
+			pr_warn("%pOF: invalid rsrc id %d found", np, rsrc_id);
+			ret = -EINVAL;
+			goto err;
+		}
+		imx_sc_pd->rsrc_id = rsrc_id;
+	} else {
+		imx_sc_pd->rsrc_id = IMX_SC_R_LAST;
+	}
+
+	if (imx_sc_pd->rsrc_id != IMX_SC_R_LAST) {
+		imx_sc_pd->pd.power_off = imx_sc_pd_power_off;
+		imx_sc_pd->pd.power_on = imx_sc_pd_power_on;
+	}
Can be moved into the property exists test above.
+
+	imx_sc_pd->pd.name = np->name;
+
+	ret = pm_genpd_init(&imx_sc_pd->pd, NULL, true);
+	if (ret < 0)
+		goto err;
+
+	if (genpd_parent) {
+		ret = pm_genpd_add_subdomain(genpd_parent, &imx_sc_pd->pd);
+		if (ret)
+			goto err;
+	}
+
+	ret = of_genpd_add_provider_simple(np, &imx_sc_pd->pd);
+	if (!ret)
+		return &imx_sc_pd->pd;
+
+	pm_genpd_remove_subdomain(genpd_parent, &imx_sc_pd->pd);
+
+err:
+	pr_warn("failed to add PM domain %pOF: %d\n", np, ret);
You have a struct device *, you should use dev_* functions.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help