diff mbox series

regulator: qcom_smd: Keep one rpm handle for all vregs

Message ID 20231227-topic-rpm_vreg_cleanup-v1-1-949da0864ac5@linaro.org
State New
Headers
Series regulator: qcom_smd: Keep one rpm handle for all vregs |

Commit Message

Konrad Dybcio Dec. 27, 2023, 1:29 a.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 <konrad.dybcio@linaro.org>
---
 drivers/regulator/qcom_smd-regulator.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)


---
base-commit: 39676dfe52331dba909c617f213fdb21015c8d10
change-id: 20231227-topic-rpm_vreg_cleanup-fa095cd528ec

Best regards,

Comments

kernel test robot Dec. 27, 2023, 11:10 a.m. UTC | #1 
Hi Konrad,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 39676dfe52331dba909c617f213fdb21015c8d10]

url:    https://github.com/intel-lab-lkp/linux/commits/Konrad-Dybcio/regulator-qcom_smd-Keep-one-rpm-handle-for-all-vregs/20231227-093153
base:   39676dfe52331dba909c617f213fdb21015c8d10
patch link:    https://lore.kernel.org/r/20231227-topic-rpm_vreg_cleanup-v1-1-949da0864ac5%40linaro.org
patch subject: [PATCH] regulator: qcom_smd: Keep one rpm handle for all vregs
config: arc-randconfig-001-20231227 (https://download.01.org/0day-ci/archive/20231227/202312271812.C5jldM3L-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231227/202312271812.C5jldM3L-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312271812.C5jldM3L-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/regulator/qcom_smd-regulator.c:1395: warning: Excess function parameter 'rpm' description in 'rpm_regulator_init_vreg'


vim +1395 drivers/regulator/qcom_smd-regulator.c

da65e367b67e15 Bjorn Andersson 2015-08-17  1379  
14e2976fbabdac Konrad Dybcio   2021-12-30  1380  /**
14e2976fbabdac Konrad Dybcio   2021-12-30  1381   * rpm_regulator_init_vreg() - initialize all attributes of a qcom_smd-regulator
14e2976fbabdac Konrad Dybcio   2021-12-30  1382   * @vreg:		Pointer to the individual qcom_smd-regulator resource
14e2976fbabdac Konrad Dybcio   2021-12-30  1383   * @dev:		Pointer to the top level qcom_smd-regulator PMIC device
14e2976fbabdac Konrad Dybcio   2021-12-30  1384   * @node:		Pointer to the individual qcom_smd-regulator resource
14e2976fbabdac Konrad Dybcio   2021-12-30  1385   *			device node
14e2976fbabdac Konrad Dybcio   2021-12-30  1386   * @rpm:		Pointer to the rpm bus node
14e2976fbabdac Konrad Dybcio   2021-12-30  1387   * @pmic_rpm_data:	Pointer to a null-terminated array of qcom_smd-regulator
14e2976fbabdac Konrad Dybcio   2021-12-30  1388   *			resources defined for the top level PMIC device
14e2976fbabdac Konrad Dybcio   2021-12-30  1389   *
14e2976fbabdac Konrad Dybcio   2021-12-30  1390   * Return: 0 on success, errno on failure
14e2976fbabdac Konrad Dybcio   2021-12-30  1391   */
14e2976fbabdac Konrad Dybcio   2021-12-30  1392  static int rpm_regulator_init_vreg(struct qcom_rpm_reg *vreg, struct device *dev,
d7cdd450e31088 Konrad Dybcio   2023-12-27  1393  				   struct device_node *node,
14e2976fbabdac Konrad Dybcio   2021-12-30  1394  				   const struct rpm_regulator_data *pmic_rpm_data)
da65e367b67e15 Bjorn Andersson 2015-08-17 @1395  {
da65e367b67e15 Bjorn Andersson 2015-08-17  1396  	struct regulator_config config = {};
14e2976fbabdac Konrad Dybcio   2021-12-30  1397  	const struct rpm_regulator_data *rpm_data;
da65e367b67e15 Bjorn Andersson 2015-08-17  1398  	struct regulator_dev *rdev;
14e2976fbabdac Konrad Dybcio   2021-12-30  1399  	int ret;
14e2976fbabdac Konrad Dybcio   2021-12-30  1400  
14e2976fbabdac Konrad Dybcio   2021-12-30  1401  	for (rpm_data = pmic_rpm_data; rpm_data->name; rpm_data++)
14e2976fbabdac Konrad Dybcio   2021-12-30  1402  		if (of_node_name_eq(node, rpm_data->name))
14e2976fbabdac Konrad Dybcio   2021-12-30  1403  			break;
14e2976fbabdac Konrad Dybcio   2021-12-30  1404  
14e2976fbabdac Konrad Dybcio   2021-12-30  1405  	if (!rpm_data->name) {
14e2976fbabdac Konrad Dybcio   2021-12-30  1406  		dev_err(dev, "Unknown regulator %pOFn\n", node);
14e2976fbabdac Konrad Dybcio   2021-12-30  1407  		return -EINVAL;
14e2976fbabdac Konrad Dybcio   2021-12-30  1408  	}
14e2976fbabdac Konrad Dybcio   2021-12-30  1409  
14e2976fbabdac Konrad Dybcio   2021-12-30  1410  	vreg->dev	= dev;
14e2976fbabdac Konrad Dybcio   2021-12-30  1411  	vreg->type	= rpm_data->type;
14e2976fbabdac Konrad Dybcio   2021-12-30  1412  	vreg->id	= rpm_data->id;
14e2976fbabdac Konrad Dybcio   2021-12-30  1413  
14e2976fbabdac Konrad Dybcio   2021-12-30  1414  	memcpy(&vreg->desc, rpm_data->desc, sizeof(vreg->desc));
14e2976fbabdac Konrad Dybcio   2021-12-30  1415  	vreg->desc.name = rpm_data->name;
14e2976fbabdac Konrad Dybcio   2021-12-30  1416  	vreg->desc.supply_name = rpm_data->supply;
14e2976fbabdac Konrad Dybcio   2021-12-30  1417  	vreg->desc.owner = THIS_MODULE;
14e2976fbabdac Konrad Dybcio   2021-12-30  1418  	vreg->desc.type = REGULATOR_VOLTAGE;
14e2976fbabdac Konrad Dybcio   2021-12-30  1419  	vreg->desc.of_match = rpm_data->name;
14e2976fbabdac Konrad Dybcio   2021-12-30  1420  
14e2976fbabdac Konrad Dybcio   2021-12-30  1421  	config.dev		= dev;
14e2976fbabdac Konrad Dybcio   2021-12-30  1422  	config.of_node		= node;
14e2976fbabdac Konrad Dybcio   2021-12-30  1423  	config.driver_data	= vreg;
14e2976fbabdac Konrad Dybcio   2021-12-30  1424  
14e2976fbabdac Konrad Dybcio   2021-12-30  1425  	rdev = devm_regulator_register(dev, &vreg->desc, &config);
14e2976fbabdac Konrad Dybcio   2021-12-30  1426  	if (IS_ERR(rdev)) {
14e2976fbabdac Konrad Dybcio   2021-12-30  1427  		ret = PTR_ERR(rdev);
14e2976fbabdac Konrad Dybcio   2021-12-30  1428  		dev_err(dev, "%pOFn: devm_regulator_register() failed, ret=%d\n", node, ret);
14e2976fbabdac Konrad Dybcio   2021-12-30  1429  		return ret;
14e2976fbabdac Konrad Dybcio   2021-12-30  1430  	}
14e2976fbabdac Konrad Dybcio   2021-12-30  1431  
14e2976fbabdac Konrad Dybcio   2021-12-30  1432  	return 0;
14e2976fbabdac Konrad Dybcio   2021-12-30  1433  }
14e2976fbabdac Konrad Dybcio   2021-12-30  1434
Dmitry Baryshkov Dec. 27, 2023, 11:48 a.m. UTC | #2 
On 27/12/2023 03:29, Konrad Dybcio wrote:
> 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 <konrad.dybcio@linaro.org>
> ---
>   drivers/regulator/qcom_smd-regulator.c | 18 +++++++-----------
>   1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
> index d1be9568025e..905c15df8c85 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) {
> @@ -1391,7 +1390,7 @@ MODULE_DEVICE_TABLE(of, rpm_of_match);
>    * 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 +1408,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 +1438,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) {

I thought about having a mutex around (I don't remember if secondary 
PMICs and/or chargers can be routed through RPM or not).

Then I went on checking other RPM and SMD-RPM drivers.

clk-rpm: global variable, field
clk-smd-rpm: struct field
regulator_qcom-smd-rpm: struct field

Probably it's worth using the same approach in all four drivers?

>   		dev_err(&pdev->dev, "Unable to retrieve handle to rpm\n");
>   		return -ENODEV;
>   	}
> @@ -1460,8 +1457,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;
> 
> ---
> base-commit: 39676dfe52331dba909c617f213fdb21015c8d10
> change-id: 20231227-topic-rpm_vreg_cleanup-fa095cd528ec
> 
> Best regards,
Konrad Dybcio Jan. 3, 2024, 10:03 a.m. UTC | #3 
On 27.12.2023 12:48, Dmitry Baryshkov wrote:
> On 27/12/2023 03:29, Konrad Dybcio wrote:
>> 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 <konrad.dybcio@linaro.org>
>> ---

[...]

>>   @@ -1440,11 +1438,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) {
> 
> I thought about having a mutex around (I don't remember if secondary PMICs and/or chargers can be routed through RPM or not).

A mutex for assigning this?

Konrad
> 
> Then I went on checking other RPM and SMD-RPM drivers.
> 
> clk-rpm: global variable, field
> clk-smd-rpm: struct field
> regulator_qcom-smd-rpm: struct field
> 
> Probably it's worth using the same approach in all four drivers?
Dmitry Baryshkov Jan. 3, 2024, 6:31 p.m. UTC | #4 
On Wed, 3 Jan 2024 at 12:03, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 27.12.2023 12:48, Dmitry Baryshkov wrote:
> > On 27/12/2023 03:29, Konrad Dybcio wrote:
> >> 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 <konrad.dybcio@linaro.org>
> >> ---
>
> [...]
>
> >>   @@ -1440,11 +1438,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) {
> >
> > I thought about having a mutex around (I don't remember if secondary PMICs and/or chargers can be routed through RPM or not).
>
> A mutex for assigning this?

Yep.

>
> Konrad
> >
> > Then I went on checking other RPM and SMD-RPM drivers.
> >
> > clk-rpm: global variable, field
> > clk-smd-rpm: struct field
> > regulator_qcom-smd-rpm: struct field
> >
> > Probably it's worth using the same approach in all four drivers?
>
>
diff mbox series

Patch

diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
index d1be9568025e..905c15df8c85 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) {
@@ -1391,7 +1390,7 @@  MODULE_DEVICE_TABLE(of, rpm_of_match);
  * 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 +1408,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 +1438,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 +1457,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;