[v2] regulator: qcom_smd: Keep one rpm handle for all vregs

Commit Message

Konrad Dybcio Jan. 3, 2024, 8:18 p.m. UTC

For no apparent reason (as there's just one RPM per SoC), all vregs
currently store a copy of a pointer to smd_rpm. Introduce a single,
global one to save up on space in each definition.

bloat-o-meter reports:

Total: Before=43944, After=43924, chg -0.05%

plus sizeof(ptr) on every dynamically allocated regulator :)

Signed-off-by: Konrad Dybcio <[e-mail]>
---
Changes in v2:
- Remove unused function argument from rpm_regulator_init_vreg kerneldoc
- Do NOT add a mutex around the rpm assignment, talked to Dmitry offline
  and we concluded it makes no sense
- Link to v1: https://lore.kernel.org/r/[e-mail]
---
 drivers/regulator/qcom_smd-regulator.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)


---
base-commit: 0fef202ac2f8e6d9ad21aead648278f1226b9053
change-id: 20231227-topic-rpm_vreg_cleanup-fa095cd528ec

Best regards,

Comments

Mark Brown Jan. 5, 2024, 4:31 p.m. UTC | #1

On Wed, Jan 03, 2024 at 09:18:37PM +0100, Konrad Dybcio wrote:

> -	rpm = dev_get_drvdata(pdev->dev.parent);
> -	if (!rpm) {
> +	smd_vreg_rpm = dev_get_drvdata(pdev->dev.parent);
> +	if (!smd_vreg_rpm) {
>  		dev_err(&pdev->dev, "Unable to retrieve handle to rpm\n");
>  		return -ENODEV;
>  	}

It'd be slightly more robust to have a check here that we do get the
same RPM back if the variable is already set, just on the off chance
that something changes in some future system and we do end up with a
second RPM somehow.

Konrad Dybcio Jan. 5, 2024, 8:15 p.m. UTC | #2

On 5.01.2024 17:31, Mark Brown wrote:
> On Wed, Jan 03, 2024 at 09:18:37PM +0100, Konrad Dybcio wrote:
> 
>> -	rpm = dev_get_drvdata(pdev->dev.parent);
>> -	if (!rpm) {
>> +	smd_vreg_rpm = dev_get_drvdata(pdev->dev.parent);
>> +	if (!smd_vreg_rpm) {
>>  		dev_err(&pdev->dev, "Unable to retrieve handle to rpm\n");
>>  		return -ENODEV;
>>  	}
> 
> It'd be slightly more robust to have a check here that we do get the
> same RPM back if the variable is already set, just on the off chance
> that something changes in some future system and we do end up with a
> second RPM somehow.

Knowing how improbable this is (currently RPM is responsible for almost all
power and some clock rails, including DDR), I'd say it's excessive, but if
you wish, I can add it.

Konrad

Mark Brown Jan. 5, 2024, 8:23 p.m. UTC | #3

On Fri, Jan 05, 2024 at 09:15:49PM +0100, Konrad Dybcio wrote:
> On 5.01.2024 17:31, Mark Brown wrote:

> > It'd be slightly more robust to have a check here that we do get the
> > same RPM back if the variable is already set, just on the off chance
> > that something changes in some future system and we do end up with a
> > second RPM somehow.

> Knowing how improbable this is (currently RPM is responsible for almost all
> power and some clock rails, including DDR), I'd say it's excessive, but if
> you wish, I can add it.

It really feels like something where if this was a good idea we'd have
an API to do this directly rather than passing around through driver
data.  The fact that it's used for all power management doesn't
immediately preclude having two instances managing the power for two
different bits of the system (eg, a low power island).

Patch

diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
index d1be9568025e..5461c03e2aac 100644
--- a/drivers/regulator/qcom_smd-regulator.c
+++ b/drivers/regulator/qcom_smd-regulator.c
@@ -11,11 +11,10 @@ 
 #include <linux/regulator/of_regulator.h>
 #include <linux/soc/qcom/smd-rpm.h>
 
+struct qcom_smd_rpm *smd_vreg_rpm;
+
 struct qcom_rpm_reg {
 	struct device *dev;
-
-	struct qcom_smd_rpm *rpm;
-
 	u32 type;
 	u32 id;
 
@@ -70,7 +69,7 @@  static int rpm_reg_write_active(struct qcom_rpm_reg *vreg)
 	if (!reqlen)
 		return 0;
 
-	ret = qcom_rpm_smd_write(vreg->rpm, QCOM_SMD_RPM_ACTIVE_STATE,
+	ret = qcom_rpm_smd_write(smd_vreg_rpm, QCOM_SMD_RPM_ACTIVE_STATE,
 				 vreg->type, vreg->id,
 				 req, sizeof(req[0]) * reqlen);
 	if (!ret) {
@@ -1384,14 +1383,13 @@  MODULE_DEVICE_TABLE(of, rpm_of_match);
  * @dev:		Pointer to the top level qcom_smd-regulator PMIC device
  * @node:		Pointer to the individual qcom_smd-regulator resource
  *			device node
- * @rpm:		Pointer to the rpm bus node
  * @pmic_rpm_data:	Pointer to a null-terminated array of qcom_smd-regulator
  *			resources defined for the top level PMIC device
  *
  * Return: 0 on success, errno on failure
  */
 static int rpm_regulator_init_vreg(struct qcom_rpm_reg *vreg, struct device *dev,
-				   struct device_node *node, struct qcom_smd_rpm *rpm,
+				   struct device_node *node,
 				   const struct rpm_regulator_data *pmic_rpm_data)
 {
 	struct regulator_config config = {};
@@ -1409,7 +1407,6 @@  static int rpm_regulator_init_vreg(struct qcom_rpm_reg *vreg, struct device *dev
 	}
 
 	vreg->dev	= dev;
-	vreg->rpm	= rpm;
 	vreg->type	= rpm_data->type;
 	vreg->id	= rpm_data->id;
 
@@ -1440,11 +1437,10 @@  static int rpm_reg_probe(struct platform_device *pdev)
 	const struct rpm_regulator_data *vreg_data;
 	struct device_node *node;
 	struct qcom_rpm_reg *vreg;
-	struct qcom_smd_rpm *rpm;
 	int ret;
 
-	rpm = dev_get_drvdata(pdev->dev.parent);
-	if (!rpm) {
+	smd_vreg_rpm = dev_get_drvdata(pdev->dev.parent);
+	if (!smd_vreg_rpm) {
 		dev_err(&pdev->dev, "Unable to retrieve handle to rpm\n");
 		return -ENODEV;
 	}
@@ -1460,8 +1456,7 @@  static int rpm_reg_probe(struct platform_device *pdev)
 			return -ENOMEM;
 		}
 
-		ret = rpm_regulator_init_vreg(vreg, dev, node, rpm, vreg_data);
-
+		ret = rpm_regulator_init_vreg(vreg, dev, node, vreg_data);
 		if (ret < 0) {
 			of_node_put(node);
 			return ret;