Hi Mark,

Thank you for your review.

-----Original Message-----
From: Mark Brown [mailto:broonie at kernel.org]
Sent: 2016?1?16? 1:58
To: Yang, Wenyou <redacted>
Cc: Liam Girdwood <lgirdwood@gmail.com>; Rob Herring <robh+dt@kernel.org>;
Pawel Moll [off-list ref]; Mark Rutland [off-list ref]; Ian
Campbell [off-list ref]; Kumar Gala [off-list ref];
Ferre, Nicolas [off-list ref]; linux-arm-
kernel at lists.infradead.org; linux-kernel at vger.kernel.org;
devicetree at vger.kernel.org
Subject: Re: [PATCH 1/2] regulator: act8945a: add regulator driver for ACT8945A

On Fri, Jan 08, 2016 at 10:08:57AM +0800, Wenyou Yang wrote:

quoted

+	np = of_get_child_by_name(pdev->dev.of_node, "regulators");
+	if (!np) {
+		dev_err(&pdev->dev, "regulator node not found\n");
+		return -EINVAL;
+	}
+
+	matches = act8945a_matches;
+	num_matches = ARRAY_SIZE(act8945a_matches);
+
+	ret = of_regulator_match(&pdev->dev, np, matches, num_matches);

Don't open code this, use the core regulators_node and of_match implementation
instead.

Good. In the next version, I use the code implementation, and remove the redundant code.

Thank you for your advice.

quoted

+		dev_info(act8945a_dev->dev, "%s regulator registered\n",
+			desc->name);

This is just noise, please remove it - let the core handle notifying users.

It is fixed in version 2.

quoted

+static int __init act8945a_pmic_init(void) {
+	return platform_driver_register(&act8945a_pmic_driver);
+}
+subsys_initcall(act8945a_pmic_init);

Please use module_platform_driver() unless there is a strong reason to do
otherwise.

It is fixed in version 2 too.


Best Regards,
Wenyou Yang