[PATCH v1 0/7] Add TI TPS65215 PMIC Regulator Support

[PATCH v1 0/7] Add TI TPS65215 PMIC Regulator Support

[Shree Ramamoorthy]

Happy Holidays!

TPS65215 is a Power Management Integrated Circuit (PMIC) that has
significant register map overlap with TPS65219. The series introduces
TPS65215 and restructures the existing driver to support multiple devices.

This follow-up series is dependent on the TPS65215 MFD Driver Series:
Commit 30fafb69994a ("mfd: tps65215: Add support for TI TPS65215 PMIC")
Commit 07c9c92bd47f ("mfd: tps65215: Remove regmap_read check")

TPS65219 Cleanup Series:
GPIO: https://lore.kernel.org/all/20241217204755.1011731-1-s-ramamoorthy@ti.com/
MFD: https://lore.kernel.org/all/20241217204935.1012106-1-s-ramamoorthy@ti.com/
Reg: https://lore.kernel.org/all/20241217204526.1010989-1-s-ramamoorthy@ti.com/

- Both TPS65215 and TPS65219 have 3 Buck regulators.
- TPS65215 has 2 LDOs, whereas TPS65219 has 4 LDOs.
- TPS65215 and TPS65219's LDO1 are the same.
- TPS65215's LDO2 maps to TPS65219's LDO3.
- TPS65215 has 1 GPO, whereas TPS65219 has 2 GPOs.
- The remaining features are the same.

TPS65215 TRM: https://www.ti.com/lit/pdf/slvucw5/

AM62L + TPS65215 Test Logs:
https://gist.github.com/ramamoorthyhs/7560eca6110fafc77b51894fa2c0fd22

Shree Ramamoorthy (7):
  regulator: dt-bindings: Add TI TPS65215 PMIC bindings
  regulator: tps65215: Update platform_device_id table
  regulator: tps65215: Update function & struct names
  regulator: tps65215: Update IRQ structs to include TPS65215
  regulator: tps65215: Add chip_data struct for multi-PMIC support
  regulator: tps65215: Define probe() helper functions
  regulator: tps65215: Restructure probe() for multi-PMIC support

 .../bindings/regulator/ti,tps65219.yaml       |   9 +-
 drivers/regulator/Kconfig                     |   7 +-
 drivers/regulator/tps65219-regulator.c        | 214 +++++++++++++-----
 3 files changed, 175 insertions(+), 55 deletions(-)

-- 
2.34.1

[PATCH v1 1/7] regulator: dt-bindings: Add TI TPS65215 PMIC bindings

[Shree Ramamoorthy]

TPS65215 is a Power Management IC with 3 Buck regulators and 2 LDOs.

TPS65215 has 2 LDOS and 1 GPO, whereas TPS65219 has 4 LDOs and 2 GPOs. The
remaining features for both devices are the same.

Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
---
 .../devicetree/bindings/regulator/ti,tps65219.yaml       | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/regulator/ti,tps65219.yaml b/Documentation/devicetree/bindings/regulator/ti,tps65219.yaml
index 78e64521d401..ba5f6fcf5219 100644
--- a/Documentation/devicetree/bindings/regulator/ti,tps65219.yaml
+++ b/Documentation/devicetree/bindings/regulator/ti,tps65219.yaml
@@ -4,7 +4,7 @@
 $id: http://devicetree.org/schemas/regulator/ti,tps65219.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: TI tps65219 Power Management Integrated Circuit regulators
+title: TI TPS65215/TPS65219 Power Management Integrated Circuit
 
 maintainers:
   - Jerome Neanne <jerome.neanne@baylibre.com>
@@ -12,10 +12,17 @@ maintainers:
 description: |
   Regulator nodes should be named to buck<number> and ldo<number>.
 
+  TI TPS65219 is a Power Management IC with 3 Buck regulators, 4 Low
+  Drop-out Regulators (LDOs), 1 GPIO, 2 GPOs, and power-button.
+
+  TI TPS65215 is a derivative of TPS65219 with 3 Buck regulators, 2 Low
+  Drop-out Regulators (LDOs), 1 GPIO, 1 GPO, and power-button.
+
 properties:
   compatible:
     enum:
       - ti,tps65219
+      - ti,tps65215
 
   reg:
     maxItems: 1
-- 
2.34.1

Re: [PATCH v1 1/7] regulator: dt-bindings: Add TI TPS65215 PMIC bindings

[Conor Dooley]

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

On Thu, Dec 26, 2024 at 03:54:06PM -0600, Shree Ramamoorthy wrote:
> TPS65215 is a Power Management IC with 3 Buck regulators and 2 LDOs.
> 
> TPS65215 has 2 LDOS and 1 GPO, whereas TPS65219 has 4 LDOs and 2 GPOs. The
> remaining features for both devices are the same.
> 
> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

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

Re: [PATCH v1 1/7] regulator: dt-bindings: Add TI TPS65215 PMIC bindings

[Roger Quadros]

On 26/12/2024 23:54, Shree Ramamoorthy wrote:
> TPS65215 is a Power Management IC with 3 Buck regulators and 2 LDOs.
> 
> TPS65215 has 2 LDOS and 1 GPO, whereas TPS65219 has 4 LDOs and 2 GPOs. The
> remaining features for both devices are the same.
> 
> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
> ---
>  .../devicetree/bindings/regulator/ti,tps65219.yaml       | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/ti,tps65219.yaml b/Documentation/devicetree/bindings/regulator/ti,tps65219.yaml
> index 78e64521d401..ba5f6fcf5219 100644
> --- a/Documentation/devicetree/bindings/regulator/ti,tps65219.yaml
> +++ b/Documentation/devicetree/bindings/regulator/ti,tps65219.yaml
> @@ -4,7 +4,7 @@
>  $id: http://devicetree.org/schemas/regulator/ti,tps65219.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
> -title: TI tps65219 Power Management Integrated Circuit regulators
> +title: TI TPS65215/TPS65219 Power Management Integrated Circuit
>  
>  maintainers:
>    - Jerome Neanne <jerome.neanne@baylibre.com>
> @@ -12,10 +12,17 @@ maintainers:
>  description: |
>    Regulator nodes should be named to buck<number> and ldo<number>.
>  
> +  TI TPS65219 is a Power Management IC with 3 Buck regulators, 4 Low
> +  Drop-out Regulators (LDOs), 1 GPIO, 2 GPOs, and power-button.
> +
> +  TI TPS65215 is a derivative of TPS65219 with 3 Buck regulators, 2 Low
> +  Drop-out Regulators (LDOs), 1 GPIO, 1 GPO, and power-button.
> +
>  properties:
>    compatible:
>      enum:
>        - ti,tps65219
> +      - ti,tps65215

Could be sorted alphanumerically.

>  
>    reg:
>      maxItems: 1

-- 
cheers,
-roger

[PATCH v1 2/7] regulator: tps65215: Update platform_device_id table

[Shree Ramamoorthy]

Add TI TPS65215 PMIC to the existing platform_device_id struct, so the
regulator probe() can match which PMIC chip_data information.

Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
---
 drivers/regulator/tps65219-regulator.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/tps65219-regulator.c b/drivers/regulator/tps65219-regulator.c
index aa65077f9d41..b8a178ae6b42 100644
--- a/drivers/regulator/tps65219-regulator.c
+++ b/drivers/regulator/tps65219-regulator.c
@@ -344,7 +344,8 @@ static int tps65219_regulator_probe(struct platform_device *pdev)
 }
 
 static const struct platform_device_id tps65219_regulator_id_table[] = {
-	{ "tps65219-regulator", },
+	{ "tps65219-regulator", TPS65219 },
+	{ "tps65215-regulator", TPS65215 },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(platform, tps65219_regulator_id_table);
-- 
2.34.1

Re: [PATCH v1 2/7] regulator: tps65215: Update platform_device_id table

[Christophe JAILLET]

Le 26/12/2024 à 22:54, Shree Ramamoorthy a écrit :
> Add TI TPS65215 PMIC to the existing platform_device_id struct, so the
> regulator probe() can match which PMIC chip_data information.
> 
> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
> ---
>   drivers/regulator/tps65219-regulator.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/regulator/tps65219-regulator.c b/drivers/regulator/tps65219-regulator.c
> index aa65077f9d41..b8a178ae6b42 100644
> --- a/drivers/regulator/tps65219-regulator.c
> +++ b/drivers/regulator/tps65219-regulator.c
> @@ -344,7 +344,8 @@ static int tps65219_regulator_probe(struct platform_device *pdev)
>   }
>   
>   static const struct platform_device_id tps65219_regulator_id_table[] = {
> -	{ "tps65219-regulator", },
> +	{ "tps65219-regulator", TPS65219 },
> +	{ "tps65215-regulator", TPS65215 },

Maybe keep alphabetical order? TPS65215, then TPS65219.

CJ

>   	{ /* sentinel */ }
>   };
>   MODULE_DEVICE_TABLE(platform, tps65219_regulator_id_table);

[PATCH v1 3/7] regulator: tps65215: Update function & struct names

[Shree Ramamoorthy]

Update struct and function names to indicate if it supports TPS65219 and/or
TPS65215. The 'common' prefix is added to indicate the resource applies
to both PMICs.

Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
---
 drivers/regulator/Kconfig              |  7 +--
 drivers/regulator/tps65219-regulator.c | 65 +++++++++++++++++---------
 2 files changed, 48 insertions(+), 24 deletions(-)

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 39297f7d8177..6cd87443f9bb 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -1579,10 +1579,11 @@ config REGULATOR_TPS65219
 	tristate "TI TPS65219 Power regulators"
 	depends on MFD_TPS65219 && OF
 	help
-	  This driver supports TPS65219 voltage regulator chips.
+	  This driver supports TPS65219 series and TPS65215 voltage regulator chips.
 	  TPS65219 series of PMICs have 3 single phase BUCKs & 4 LDOs
-	  voltage regulators. It supports software based voltage control
-	  for different voltage domains.
+	  voltage regulators.
+	  TPS65215 PMIC has 3 single phase BUCKs & 2 LDOs.
+	  Both PMICs support software based voltage control for different voltage domains.
 
 config REGULATOR_TPS6594
 	tristate "TI TPS6594 Power regulators"
diff --git a/drivers/regulator/tps65219-regulator.c b/drivers/regulator/tps65219-regulator.c
index b8a178ae6b42..188a988e3bbe 100644
--- a/drivers/regulator/tps65219-regulator.c
+++ b/drivers/regulator/tps65219-regulator.c
@@ -1,10 +1,9 @@
 // SPDX-License-Identifier: GPL-2.0
 //
-// tps65219-regulator.c
-//
-// Regulator driver for TPS65219 PMIC
+// Regulator driver for TPS65215/TPS65219 PMIC
 //
 // Copyright (C) 2022 BayLibre Incorporated - https://www.baylibre.com/
+// Copyright (C) 2024 Texas Instruments Incorporated - https://www.ti.com/
 //
 // This implementation derived from tps65218 authored by
 // "J Keerthy <j-keerthy@ti.com>"
@@ -125,12 +124,22 @@ static const struct linear_range bucks_ranges[] = {
 	REGULATOR_LINEAR_RANGE(3400000, 0x34, 0x3f, 0),
 };
 
-static const struct linear_range ldos_1_2_ranges[] = {
+static const struct linear_range ldo_1_range[] = {
+	REGULATOR_LINEAR_RANGE(600000, 0x0, 0x37, 50000),
+	REGULATOR_LINEAR_RANGE(3400000, 0x38, 0x3f, 0),
+};
+
+static const struct linear_range tps65215_ldo_2_range[] = {
+	REGULATOR_LINEAR_RANGE(1200000, 0x0, 0xC, 50000),
+	REGULATOR_LINEAR_RANGE(3300000, 0x36, 0x3F, 0),
+};
+
+static const struct linear_range tps65219_ldo_2_range[] = {
 	REGULATOR_LINEAR_RANGE(600000, 0x0, 0x37, 50000),
 	REGULATOR_LINEAR_RANGE(3400000, 0x38, 0x3f, 0),
 };
 
-static const struct linear_range ldos_3_4_ranges[] = {
+static const struct linear_range tps65219_ldos_3_4_range[] = {
 	REGULATOR_LINEAR_RANGE(1200000, 0x0, 0xC, 0),
 	REGULATOR_LINEAR_RANGE(1250000, 0xD, 0x35, 50000),
 	REGULATOR_LINEAR_RANGE(3300000, 0x36, 0x3F, 0),
@@ -174,7 +183,7 @@ static unsigned int tps65219_get_mode(struct regulator_dev *dev)
 }
 
 /* Operations permitted on BUCK1/2/3 */
-static const struct regulator_ops tps65219_bucks_ops = {
+static const struct regulator_ops bucks_ops = {
 	.is_enabled		= regulator_is_enabled_regmap,
 	.enable			= regulator_enable_regmap,
 	.disable		= regulator_disable_regmap,
@@ -189,7 +198,7 @@ static const struct regulator_ops tps65219_bucks_ops = {
 };
 
 /* Operations permitted on LDO1/2 */
-static const struct regulator_ops tps65219_ldos_1_2_ops = {
+static const struct regulator_ops ldos_1_2_ops = {
 	.is_enabled		= regulator_is_enabled_regmap,
 	.enable			= regulator_enable_regmap,
 	.disable		= regulator_disable_regmap,
@@ -204,7 +213,7 @@ static const struct regulator_ops tps65219_ldos_1_2_ops = {
 };
 
 /* Operations permitted on LDO3/4 */
-static const struct regulator_ops tps65219_ldos_3_4_ops = {
+static const struct regulator_ops ldos_3_4_ops = {
 	.is_enabled		= regulator_is_enabled_regmap,
 	.enable			= regulator_enable_regmap,
 	.disable		= regulator_disable_regmap,
@@ -216,55 +225,69 @@ static const struct regulator_ops tps65219_ldos_3_4_ops = {
 	.map_voltage		= regulator_map_voltage_linear_range,
 };
 
-static const struct regulator_desc regulators[] = {
+static const struct regulator_desc common_regs[] = {
 	TPS65219_REGULATOR("BUCK1", "buck1", TPS65219_BUCK_1,
-			   REGULATOR_VOLTAGE, tps65219_bucks_ops, 64,
+			   REGULATOR_VOLTAGE, bucks_ops, 64,
 			   TPS65219_REG_BUCK1_VOUT,
 			   TPS65219_BUCKS_LDOS_VOUT_VSET_MASK,
 			   TPS65219_REG_ENABLE_CTRL,
 			   TPS65219_ENABLE_BUCK1_EN_MASK, 0, 0, bucks_ranges,
 			   3, 4000, 0, NULL, 0, 0),
 	TPS65219_REGULATOR("BUCK2", "buck2", TPS65219_BUCK_2,
-			   REGULATOR_VOLTAGE, tps65219_bucks_ops, 64,
+			   REGULATOR_VOLTAGE, bucks_ops, 64,
 			   TPS65219_REG_BUCK2_VOUT,
 			   TPS65219_BUCKS_LDOS_VOUT_VSET_MASK,
 			   TPS65219_REG_ENABLE_CTRL,
 			   TPS65219_ENABLE_BUCK2_EN_MASK, 0, 0, bucks_ranges,
 			   3, 4000, 0, NULL, 0, 0),
 	TPS65219_REGULATOR("BUCK3", "buck3", TPS65219_BUCK_3,
-			   REGULATOR_VOLTAGE, tps65219_bucks_ops, 64,
+			   REGULATOR_VOLTAGE, bucks_ops, 64,
 			   TPS65219_REG_BUCK3_VOUT,
 			   TPS65219_BUCKS_LDOS_VOUT_VSET_MASK,
 			   TPS65219_REG_ENABLE_CTRL,
 			   TPS65219_ENABLE_BUCK3_EN_MASK, 0, 0, bucks_ranges,
 			   3, 0, 0, NULL, 0, 0),
 	TPS65219_REGULATOR("LDO1", "ldo1", TPS65219_LDO_1,
-			   REGULATOR_VOLTAGE, tps65219_ldos_1_2_ops, 64,
+			   REGULATOR_VOLTAGE, ldos_1_2_ops, 64,
 			   TPS65219_REG_LDO1_VOUT,
 			   TPS65219_BUCKS_LDOS_VOUT_VSET_MASK,
 			   TPS65219_REG_ENABLE_CTRL,
-			   TPS65219_ENABLE_LDO1_EN_MASK, 0, 0, ldos_1_2_ranges,
+			   TPS65219_ENABLE_LDO1_EN_MASK, 0, 0, ldo_1_range,
 			   2, 0, 0, NULL, 0, TPS65219_LDOS_BYP_CONFIG_MASK),
+};
+
+static const struct regulator_desc tps65215_regs[] = {
+	// TPS65215's LDO2 is the same as TPS65219's LDO3
+	TPS65219_REGULATOR("LDO2", "ldo2", TPS65215_LDO_2,
+			   REGULATOR_VOLTAGE, ldos_3_4_ops, 64,
+			   TPS65215_REG_LDO2_VOUT,
+			   TPS65219_BUCKS_LDOS_VOUT_VSET_MASK,
+			   TPS65219_REG_ENABLE_CTRL,
+			   TPS65215_ENABLE_LDO2_EN_MASK, 0, 0, tps65215_ldo_2_range,
+			   3, 0, 0, NULL, 0, 0),
+};
+
+static const struct regulator_desc tps65219_regs[] = {
 	TPS65219_REGULATOR("LDO2", "ldo2", TPS65219_LDO_2,
-			   REGULATOR_VOLTAGE, tps65219_ldos_1_2_ops, 64,
+			   REGULATOR_VOLTAGE, ldos_1_2_ops, 64,
 			   TPS65219_REG_LDO2_VOUT,
 			   TPS65219_BUCKS_LDOS_VOUT_VSET_MASK,
 			   TPS65219_REG_ENABLE_CTRL,
-			   TPS65219_ENABLE_LDO2_EN_MASK, 0, 0, ldos_1_2_ranges,
+			   TPS65219_ENABLE_LDO2_EN_MASK, 0, 0, tps65219_ldo_2_range,
 			   2, 0, 0, NULL, 0, TPS65219_LDOS_BYP_CONFIG_MASK),
 	TPS65219_REGULATOR("LDO3", "ldo3", TPS65219_LDO_3,
-			   REGULATOR_VOLTAGE, tps65219_ldos_3_4_ops, 64,
+			   REGULATOR_VOLTAGE, ldos_3_4_ops, 64,
 			   TPS65219_REG_LDO3_VOUT,
 			   TPS65219_BUCKS_LDOS_VOUT_VSET_MASK,
 			   TPS65219_REG_ENABLE_CTRL,
-			   TPS65219_ENABLE_LDO3_EN_MASK, 0, 0, ldos_3_4_ranges,
+			   TPS65219_ENABLE_LDO3_EN_MASK, 0, 0, tps65219_ldos_3_4_range,
 			   3, 0, 0, NULL, 0, 0),
 	TPS65219_REGULATOR("LDO4", "ldo4", TPS65219_LDO_4,
-			   REGULATOR_VOLTAGE, tps65219_ldos_3_4_ops, 64,
+			   REGULATOR_VOLTAGE, ldos_3_4_ops, 64,
 			   TPS65219_REG_LDO4_VOUT,
 			   TPS65219_BUCKS_LDOS_VOUT_VSET_MASK,
 			   TPS65219_REG_ENABLE_CTRL,
-			   TPS65219_ENABLE_LDO4_EN_MASK, 0, 0, ldos_3_4_ranges,
+			   TPS65219_ENABLE_LDO4_EN_MASK, 0, 0, tps65219_ldos_3_4_range,
 			   3, 0, 0, NULL, 0, 0),
 };
 
@@ -362,5 +385,5 @@ static struct platform_driver tps65219_regulator_driver = {
 module_platform_driver(tps65219_regulator_driver);
 
 MODULE_AUTHOR("Jerome Neanne <j-neanne@baylibre.com>");
-MODULE_DESCRIPTION("TPS65219 voltage regulator driver");
+MODULE_DESCRIPTION("TPS65215/TPS65219 voltage regulator driver");
 MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH v1 3/7] regulator: tps65215: Update function & struct names

[Roger Quadros]

On 26/12/2024 23:54, Shree Ramamoorthy wrote:
> Update struct and function names to indicate if it supports TPS65219 and/or
> TPS65215. The 'common' prefix is added to indicate the resource applies
> to both PMICs.
> 
> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
> ---
>  drivers/regulator/Kconfig              |  7 +--
>  drivers/regulator/tps65219-regulator.c | 65 +++++++++++++++++---------
>  2 files changed, 48 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 39297f7d8177..6cd87443f9bb 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -1579,10 +1579,11 @@ config REGULATOR_TPS65219
>  	tristate "TI TPS65219 Power regulators"
>  	depends on MFD_TPS65219 && OF
>  	help
> -	  This driver supports TPS65219 voltage regulator chips.
> +	  This driver supports TPS65219 series and TPS65215 voltage regulator chips.
>  	  TPS65219 series of PMICs have 3 single phase BUCKs & 4 LDOs
> -	  voltage regulators. It supports software based voltage control
> -	  for different voltage domains.
> +	  voltage regulators.
> +	  TPS65215 PMIC has 3 single phase BUCKs & 2 LDOs.
> +	  Both PMICs support software based voltage control for different voltage domains.
>  
>  config REGULATOR_TPS6594
>  	tristate "TI TPS6594 Power regulators"
> diff --git a/drivers/regulator/tps65219-regulator.c b/drivers/regulator/tps65219-regulator.c
> index b8a178ae6b42..188a988e3bbe 100644
> --- a/drivers/regulator/tps65219-regulator.c
> +++ b/drivers/regulator/tps65219-regulator.c
> @@ -1,10 +1,9 @@
>  // SPDX-License-Identifier: GPL-2.0
>  //
> -// tps65219-regulator.c
> -//
> -// Regulator driver for TPS65219 PMIC
> +// Regulator driver for TPS65215/TPS65219 PMIC
>  //
>  // Copyright (C) 2022 BayLibre Incorporated - https://www.baylibre.com/
> +// Copyright (C) 2024 Texas Instruments Incorporated - https://www.ti.com/
>  //
>  // This implementation derived from tps65218 authored by
>  // "J Keerthy <j-keerthy@ti.com>"
> @@ -125,12 +124,22 @@ static const struct linear_range bucks_ranges[] = {
>  	REGULATOR_LINEAR_RANGE(3400000, 0x34, 0x3f, 0),
>  };
>  
> -static const struct linear_range ldos_1_2_ranges[] = {
> +static const struct linear_range ldo_1_range[] = {
> +	REGULATOR_LINEAR_RANGE(600000, 0x0, 0x37, 50000),
> +	REGULATOR_LINEAR_RANGE(3400000, 0x38, 0x3f, 0),
> +};
> +
> +static const struct linear_range tps65215_ldo_2_range[] = {
> +	REGULATOR_LINEAR_RANGE(1200000, 0x0, 0xC, 50000),
> +	REGULATOR_LINEAR_RANGE(3300000, 0x36, 0x3F, 0),
> +};
> +
> +static const struct linear_range tps65219_ldo_2_range[] = {
>  	REGULATOR_LINEAR_RANGE(600000, 0x0, 0x37, 50000),
>  	REGULATOR_LINEAR_RANGE(3400000, 0x38, 0x3f, 0),
>  };
>  
> -static const struct linear_range ldos_3_4_ranges[] = {
> +static const struct linear_range tps65219_ldos_3_4_range[] = {
>  	REGULATOR_LINEAR_RANGE(1200000, 0x0, 0xC, 0),
>  	REGULATOR_LINEAR_RANGE(1250000, 0xD, 0x35, 50000),
>  	REGULATOR_LINEAR_RANGE(3300000, 0x36, 0x3F, 0),
> @@ -174,7 +183,7 @@ static unsigned int tps65219_get_mode(struct regulator_dev *dev)
>  }
>  
>  /* Operations permitted on BUCK1/2/3 */
> -static const struct regulator_ops tps65219_bucks_ops = {
> +static const struct regulator_ops bucks_ops = {
>  	.is_enabled		= regulator_is_enabled_regmap,
>  	.enable			= regulator_enable_regmap,
>  	.disable		= regulator_disable_regmap,
> @@ -189,7 +198,7 @@ static const struct regulator_ops tps65219_bucks_ops = {
>  };
>  
>  /* Operations permitted on LDO1/2 */
> -static const struct regulator_ops tps65219_ldos_1_2_ops = {
> +static const struct regulator_ops ldos_1_2_ops = {
>  	.is_enabled		= regulator_is_enabled_regmap,
>  	.enable			= regulator_enable_regmap,
>  	.disable		= regulator_disable_regmap,
> @@ -204,7 +213,7 @@ static const struct regulator_ops tps65219_ldos_1_2_ops = {
>  };
>  
>  /* Operations permitted on LDO3/4 */
> -static const struct regulator_ops tps65219_ldos_3_4_ops = {
> +static const struct regulator_ops ldos_3_4_ops = {
>  	.is_enabled		= regulator_is_enabled_regmap,
>  	.enable			= regulator_enable_regmap,
>  	.disable		= regulator_disable_regmap,
> @@ -216,55 +225,69 @@ static const struct regulator_ops tps65219_ldos_3_4_ops = {
>  	.map_voltage		= regulator_map_voltage_linear_range,
>  };
>  
> -static const struct regulator_desc regulators[] = {
> +static const struct regulator_desc common_regs[] = {
>  	TPS65219_REGULATOR("BUCK1", "buck1", TPS65219_BUCK_1,
> -			   REGULATOR_VOLTAGE, tps65219_bucks_ops, 64,
> +			   REGULATOR_VOLTAGE, bucks_ops, 64,
>  			   TPS65219_REG_BUCK1_VOUT,
>  			   TPS65219_BUCKS_LDOS_VOUT_VSET_MASK,
>  			   TPS65219_REG_ENABLE_CTRL,
>  			   TPS65219_ENABLE_BUCK1_EN_MASK, 0, 0, bucks_ranges,
>  			   3, 4000, 0, NULL, 0, 0),
>  	TPS65219_REGULATOR("BUCK2", "buck2", TPS65219_BUCK_2,
> -			   REGULATOR_VOLTAGE, tps65219_bucks_ops, 64,
> +			   REGULATOR_VOLTAGE, bucks_ops, 64,
>  			   TPS65219_REG_BUCK2_VOUT,
>  			   TPS65219_BUCKS_LDOS_VOUT_VSET_MASK,
>  			   TPS65219_REG_ENABLE_CTRL,
>  			   TPS65219_ENABLE_BUCK2_EN_MASK, 0, 0, bucks_ranges,
>  			   3, 4000, 0, NULL, 0, 0),
>  	TPS65219_REGULATOR("BUCK3", "buck3", TPS65219_BUCK_3,
> -			   REGULATOR_VOLTAGE, tps65219_bucks_ops, 64,
> +			   REGULATOR_VOLTAGE, bucks_ops, 64,
>  			   TPS65219_REG_BUCK3_VOUT,
>  			   TPS65219_BUCKS_LDOS_VOUT_VSET_MASK,
>  			   TPS65219_REG_ENABLE_CTRL,
>  			   TPS65219_ENABLE_BUCK3_EN_MASK, 0, 0, bucks_ranges,
>  			   3, 0, 0, NULL, 0, 0),
>  	TPS65219_REGULATOR("LDO1", "ldo1", TPS65219_LDO_1,

Could we update macro TPS65219_REGULATOR to TPS6521X_REGULATOR?

> -			   REGULATOR_VOLTAGE, tps65219_ldos_1_2_ops, 64,
> +			   REGULATOR_VOLTAGE, ldos_1_2_ops, 64,
>  			   TPS65219_REG_LDO1_VOUT,
>  			   TPS65219_BUCKS_LDOS_VOUT_VSET_MASK,
>  			   TPS65219_REG_ENABLE_CTRL,
> -			   TPS65219_ENABLE_LDO1_EN_MASK, 0, 0, ldos_1_2_ranges,
> +			   TPS65219_ENABLE_LDO1_EN_MASK, 0, 0, ldo_1_range,
>  			   2, 0, 0, NULL, 0, TPS65219_LDOS_BYP_CONFIG_MASK),
> +};
> +
> +static const struct regulator_desc tps65215_regs[] = {
> +	// TPS65215's LDO2 is the same as TPS65219's LDO3
> +	TPS65219_REGULATOR("LDO2", "ldo2", TPS65215_LDO_2,
> +			   REGULATOR_VOLTAGE, ldos_3_4_ops, 64,
> +			   TPS65215_REG_LDO2_VOUT,
> +			   TPS65219_BUCKS_LDOS_VOUT_VSET_MASK,
> +			   TPS65219_REG_ENABLE_CTRL,
> +			   TPS65215_ENABLE_LDO2_EN_MASK, 0, 0, tps65215_ldo_2_range,
> +			   3, 0, 0, NULL, 0, 0),
> +};
> +
> +static const struct regulator_desc tps65219_regs[] = {
>  	TPS65219_REGULATOR("LDO2", "ldo2", TPS65219_LDO_2,
> -			   REGULATOR_VOLTAGE, tps65219_ldos_1_2_ops, 64,
> +			   REGULATOR_VOLTAGE, ldos_1_2_ops, 64,
>  			   TPS65219_REG_LDO2_VOUT,
>  			   TPS65219_BUCKS_LDOS_VOUT_VSET_MASK,
>  			   TPS65219_REG_ENABLE_CTRL,
> -			   TPS65219_ENABLE_LDO2_EN_MASK, 0, 0, ldos_1_2_ranges,
> +			   TPS65219_ENABLE_LDO2_EN_MASK, 0, 0, tps65219_ldo_2_range,
>  			   2, 0, 0, NULL, 0, TPS65219_LDOS_BYP_CONFIG_MASK),
>  	TPS65219_REGULATOR("LDO3", "ldo3", TPS65219_LDO_3,
> -			   REGULATOR_VOLTAGE, tps65219_ldos_3_4_ops, 64,
> +			   REGULATOR_VOLTAGE, ldos_3_4_ops, 64,
>  			   TPS65219_REG_LDO3_VOUT,
>  			   TPS65219_BUCKS_LDOS_VOUT_VSET_MASK,
>  			   TPS65219_REG_ENABLE_CTRL,
> -			   TPS65219_ENABLE_LDO3_EN_MASK, 0, 0, ldos_3_4_ranges,
> +			   TPS65219_ENABLE_LDO3_EN_MASK, 0, 0, tps65219_ldos_3_4_range,
>  			   3, 0, 0, NULL, 0, 0),
>  	TPS65219_REGULATOR("LDO4", "ldo4", TPS65219_LDO_4,
> -			   REGULATOR_VOLTAGE, tps65219_ldos_3_4_ops, 64,
> +			   REGULATOR_VOLTAGE, ldos_3_4_ops, 64,
>  			   TPS65219_REG_LDO4_VOUT,
>  			   TPS65219_BUCKS_LDOS_VOUT_VSET_MASK,
>  			   TPS65219_REG_ENABLE_CTRL,
> -			   TPS65219_ENABLE_LDO4_EN_MASK, 0, 0, ldos_3_4_ranges,
> +			   TPS65219_ENABLE_LDO4_EN_MASK, 0, 0, tps65219_ldos_3_4_range,
>  			   3, 0, 0, NULL, 0, 0),
>  };
>  
> @@ -362,5 +385,5 @@ static struct platform_driver tps65219_regulator_driver = {
>  module_platform_driver(tps65219_regulator_driver);
>  
>  MODULE_AUTHOR("Jerome Neanne <j-neanne@baylibre.com>");
> -MODULE_DESCRIPTION("TPS65219 voltage regulator driver");
> +MODULE_DESCRIPTION("TPS65215/TPS65219 voltage regulator driver");

"TPS65215X Voltage Regulator Driver"

>  MODULE_LICENSE("GPL");

-- 
cheers,
-roger

[PATCH v1 4/7] regulator: tps65215: Update IRQ structs to include TPS65215

[Shree Ramamoorthy]

Organize  _regulator_irq_type structs into common (applies to TPS65215 and
TPS65219) and separate device-specific structs, if needed.

Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
---
 drivers/regulator/tps65219-regulator.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/tps65219-regulator.c b/drivers/regulator/tps65219-regulator.c
index 188a988e3bbe..2c49613400e1 100644
--- a/drivers/regulator/tps65219-regulator.c
+++ b/drivers/regulator/tps65219-regulator.c
@@ -36,6 +36,14 @@ static struct tps65219_regulator_irq_type tps65219_regulator_irq_types[] = {
 	{ "LDO4_SCG", "LDO4", "short circuit to ground", REGULATOR_EVENT_REGULATION_OUT },
 	{ "LDO4_OC", "LDO4", "overcurrent", REGULATOR_EVENT_OVER_CURRENT },
 	{ "LDO4_UV", "LDO4", "undervoltage", REGULATOR_EVENT_UNDER_VOLTAGE },
+	{ "LDO3_RV", "LDO3", "residual voltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
+	{ "LDO4_RV", "LDO4", "residual voltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
+	{ "LDO3_RV_SD", "LDO3", "residual voltage on shutdown", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
+	{ "LDO4_RV_SD", "LDO4", "residual voltage on shutdown", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
+};
+
+/*  All of TPS65215's irq types are the same as common_regulator_irq_types */
+static struct tps65219_regulator_irq_type common_regulator_irq_types[] = {
 	{ "LDO1_SCG", "LDO1", "short circuit to ground", REGULATOR_EVENT_REGULATION_OUT },
 	{ "LDO1_OC", "LDO1", "overcurrent", REGULATOR_EVENT_OVER_CURRENT },
 	{ "LDO1_UV", "LDO1", "undervoltage", REGULATOR_EVENT_UNDER_VOLTAGE },
@@ -59,8 +67,6 @@ static struct tps65219_regulator_irq_type tps65219_regulator_irq_types[] = {
 	{ "BUCK3_RV", "BUCK3", "residual voltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
 	{ "LDO1_RV", "LDO1", "residual voltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
 	{ "LDO2_RV", "LDO2", "residual voltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
-	{ "LDO3_RV", "LDO3", "residual voltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
-	{ "LDO4_RV", "LDO4", "residual voltage", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
 	{ "BUCK1_RV_SD", "BUCK1", "residual voltage on shutdown",
 	 REGULATOR_EVENT_OVER_VOLTAGE_WARN },
 	{ "BUCK2_RV_SD", "BUCK2", "residual voltage on shutdown",
@@ -69,8 +75,6 @@ static struct tps65219_regulator_irq_type tps65219_regulator_irq_types[] = {
 	 REGULATOR_EVENT_OVER_VOLTAGE_WARN },
 	{ "LDO1_RV_SD", "LDO1", "residual voltage on shutdown", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
 	{ "LDO2_RV_SD", "LDO2", "residual voltage on shutdown", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
-	{ "LDO3_RV_SD", "LDO3", "residual voltage on shutdown", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
-	{ "LDO4_RV_SD", "LDO4", "residual voltage on shutdown", REGULATOR_EVENT_OVER_VOLTAGE_WARN },
 	{ "SENSOR_3_WARM", "SENSOR3", "warm temperature", REGULATOR_EVENT_OVER_TEMP_WARN},
 	{ "SENSOR_2_WARM", "SENSOR2", "warm temperature", REGULATOR_EVENT_OVER_TEMP_WARN },
 	{ "SENSOR_1_WARM", "SENSOR1", "warm temperature", REGULATOR_EVENT_OVER_TEMP_WARN },
-- 
2.34.1

[PATCH v1 5/7] regulator: tps65215: Add chip_data struct for multi-PMIC support

[Shree Ramamoorthy]

'chipid' will identify which PMIC to support, and the corresponding
chip_data struct element to use in probe(). The chip_data struct is helpful
for any new PMICs added to this driver. The goal is to add future PMIC info
to necessary structs and minimize probe() function edits.

Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
---
 drivers/regulator/tps65219-regulator.c | 32 ++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/regulator/tps65219-regulator.c b/drivers/regulator/tps65219-regulator.c
index 2c49613400e1..13f0e68d8e85 100644
--- a/drivers/regulator/tps65219-regulator.c
+++ b/drivers/regulator/tps65219-regulator.c
@@ -314,6 +314,39 @@ static irqreturn_t tps65219_regulator_irq_handler(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+struct chip_data {
+	size_t common_irq_size;
+	size_t common_rdesc_size;
+
+	size_t dev_irq_size
+	size_t rdesc_size;
+	struct tps65219_regulator_irq_type *common_irq_types;
+	struct tps65219_regulator_irq_type *irq_types;
+	const struct regulator_desc *common_rdesc;
+	const struct regulator_desc *rdesc;
+};
+
+static struct chip_data chip_info_table[] = {
+	[TPS65219] = {
+		.rdesc = tps65219_regs,
+		.rdesc_size = ARRAY_SIZE(tps65219_regs),
+		.common_rdesc = common_regs,
+		.common_rdesc_size = ARRAY_SIZE(common_regs),
+		.irq_types = tps65219_regulator_irq_types,
+		.dev_irq_size = ARRAY_SIZE(tps65219_regulator_irq_types),
+		.common_irq_types = common_regulator_irq_types,
+		.common_irq_size = ARRAY_SIZE(common_regulator_irq_types),
+	},
+	[TPS65215] = {
+		.rdesc = tps65215_regs,
+		.rdesc_size = ARRAY_SIZE(tps65215_regs),
+		.common_rdesc = common_regs,
+		.common_rdesc_size = ARRAY_SIZE(common_regs),
+		.common_irq_types = common_regulator_irq_types,
+		.common_irq_size = ARRAY_SIZE(common_regulator_irq_types),
+	},
+};
+
 static int tps65219_regulator_probe(struct platform_device *pdev)
 {
 	struct tps65219 *tps = dev_get_drvdata(pdev->dev.parent);
-- 
2.34.1

[PATCH v1 6/7] regulator: tps65215: Define probe() helper functions

[Shree Ramamoorthy]

Factor register_regulators() and request_irqs() out into smaller functions.
These 2 helper functions are used in the next restructure probe() patch to
go through the common (overlapping) regulators and irqs first, then the
device-specific structs identifed in the chip_data struct.

Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
---
 drivers/regulator/tps65219-regulator.c | 64 ++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/drivers/regulator/tps65219-regulator.c b/drivers/regulator/tps65219-regulator.c
index 13f0e68d8e85..8469ee89802c 100644
--- a/drivers/regulator/tps65219-regulator.c
+++ b/drivers/regulator/tps65219-regulator.c
@@ -346,6 +346,70 @@ static struct chip_data chip_info_table[] = {
 	},
 };
 
+static int tps65219_register_regulators(const struct regulator_desc *regulators,
+					struct tps65219 *tps,
+					struct device *dev,
+					struct regulator_config config,
+					unsigned int arr_size)
+{
+	int i;
+	struct regulator_dev *rdev;
+
+	config.driver_data = tps;
+	config.dev = tps->dev;
+	config.regmap = tps->regmap;
+
+	for (i = 0; i < arr_size; i++) {
+		rdev = devm_regulator_register(dev, &regulators[i],
+						&config);
+		if (IS_ERR(rdev)) {
+			dev_err(tps->dev,
+				"Failed to register %s regulator\n",
+				regulators[i].name);
+
+			return PTR_ERR(rdev);
+		}
+	}
+
+	return 0;
+}
+
+static int tps65219_request_irqs(struct tps65219_regulator_irq_type *irq_types,
+				 struct tps65219 *tps, struct platform_device *pdev,
+				 struct tps65219_regulator_irq_data *irq_data,
+				 unsigned int arr_size)
+{
+	int i;
+	int irq;
+	int error;
+	struct tps65219_regulator_irq_type *irq_type;
+
+	for (i = 0; i < arr_size; ++i) {
+		irq_type = &irq_types[i];
+
+		irq = platform_get_irq_byname(pdev, irq_type->irq_name);
+		if (irq < 0)
+			return -EINVAL;
+
+		irq_data[i].dev = tps->dev;
+		irq_data[i].type = irq_type;
+
+		error = devm_request_threaded_irq(tps->dev, irq, NULL,
+						  tps65219_regulator_irq_handler,
+						  IRQF_ONESHOT,
+						  irq_type->irq_name,
+						  &irq_data[i]);
+		if (error) {
+			dev_err(tps->dev,
+				"Failed to request %s IRQ %d: %d\n",
+				irq_type->irq_name, irq, error);
+			return error;
+		}
+	}
+
+	return 0;
+}
+
 static int tps65219_regulator_probe(struct platform_device *pdev)
 {
 	struct tps65219 *tps = dev_get_drvdata(pdev->dev.parent);
-- 
2.34.1

Re: [PATCH v1 6/7] regulator: tps65215: Define probe() helper functions

[Christophe JAILLET]

Le 26/12/2024 à 22:54, Shree Ramamoorthy a écrit :
> Factor register_regulators() and request_irqs() out into smaller functions.
> These 2 helper functions are used in the next restructure probe() patch to
> go through the common (overlapping) regulators and irqs first, then the
> device-specific structs identifed in the chip_data struct.
> 
> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy-l0cyMroinI0@public.gmane.org>
> ---
>   drivers/regulator/tps65219-regulator.c | 64 ++++++++++++++++++++++++++
>   1 file changed, 64 insertions(+)
> 
> diff --git a/drivers/regulator/tps65219-regulator.c b/drivers/regulator/tps65219-regulator.c
> index 13f0e68d8e85..8469ee89802c 100644
> --- a/drivers/regulator/tps65219-regulator.c
> +++ b/drivers/regulator/tps65219-regulator.c
> @@ -346,6 +346,70 @@ static struct chip_data chip_info_table[] = {
>   	},
>   };
>   
> +static int tps65219_register_regulators(const struct regulator_desc *regulators,
> +					struct tps65219 *tps,
> +					struct device *dev,
> +					struct regulator_config config,
> +					unsigned int arr_size)
> +{
> +	int i;
> +	struct regulator_dev *rdev;
> +
> +	config.driver_data = tps;
> +	config.dev = tps->dev;
> +	config.regmap = tps->regmap;
> +
> +	for (i = 0; i < arr_size; i++) {
> +		rdev = devm_regulator_register(dev, &regulators[i],
> +						&config);
> +		if (IS_ERR(rdev)) {
> +			dev_err(tps->dev,
> +				"Failed to register %s regulator\n",
> +				regulators[i].name);

This will be called from probe in 7/7.
So this could be return dev_err_probe()

> +
> +			return PTR_ERR(rdev);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int tps65219_request_irqs(struct tps65219_regulator_irq_type *irq_types,
> +				 struct tps65219 *tps, struct platform_device *pdev,
> +				 struct tps65219_regulator_irq_data *irq_data,
> +				 unsigned int arr_size)
> +{
> +	int i;
> +	int irq;
> +	int error;
> +	struct tps65219_regulator_irq_type *irq_type;
> +
> +	for (i = 0; i < arr_size; ++i) {
> +		irq_type = &irq_types[i];
> +
> +		irq = platform_get_irq_byname(pdev, irq_type->irq_name);
> +		if (irq < 0)
> +			return -EINVAL;
> +
> +		irq_data[i].dev = tps->dev;
> +		irq_data[i].type = irq_type;
> +
> +		error = devm_request_threaded_irq(tps->dev, irq, NULL,
> +						  tps65219_regulator_irq_handler,
> +						  IRQF_ONESHOT,
> +						  irq_type->irq_name,
> +						  &irq_data[i]);
> +		if (error) {
> +			dev_err(tps->dev,
> +				"Failed to request %s IRQ %d: %d\n",
> +				irq_type->irq_name, irq, error);

This will be called from probe in 7/7.
So this could be return dev_err_probe()

> +			return error;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>   static int tps65219_regulator_probe(struct platform_device *pdev)
>   {
>   	struct tps65219 *tps = dev_get_drvdata(pdev->dev.parent);

Re: [PATCH v1 6/7] regulator: tps65215: Define probe() helper functions

[Shree Ramamoorthy]

Hi,

On 1/1/25 5:01 AM, Christophe JAILLET wrote:
> Le 26/12/2024 à 22:54, Shree Ramamoorthy a écrit :
>> Factor register_regulators() and request_irqs() out into smaller 
>> functions.
>> These 2 helper functions are used in the next restructure probe() 
>> patch to
>> go through the common (overlapping) regulators and irqs first, then the
>> device-specific structs identifed in the chip_data struct.
>>
>> Signed-off-by: Shree Ramamoorthy 
>> <s-ramamoorthy-l0cyMroinI0@public.gmane.org>
>> ---
>>   drivers/regulator/tps65219-regulator.c | 64 ++++++++++++++++++++++++++
>>   1 file changed, 64 insertions(+)
>>
>> diff --git a/drivers/regulator/tps65219-regulator.c 
>> b/drivers/regulator/tps65219-regulator.c
>> index 13f0e68d8e85..8469ee89802c 100644
>> --- a/drivers/regulator/tps65219-regulator.c
>> +++ b/drivers/regulator/tps65219-regulator.c
>> @@ -346,6 +346,70 @@ static struct chip_data chip_info_table[] = {
>>       },
>>   };
>>   +static int tps65219_register_regulators(const struct 
>> regulator_desc *regulators,
>> +                    struct tps65219 *tps,
>> +                    struct device *dev,
>> +                    struct regulator_config config,
>> +                    unsigned int arr_size)
>> +{
>> +    int i;
>> +    struct regulator_dev *rdev;
>> +
>> +    config.driver_data = tps;
>> +    config.dev = tps->dev;
>> +    config.regmap = tps->regmap;
>> +
>> +    for (i = 0; i < arr_size; i++) {
>> +        rdev = devm_regulator_register(dev, &regulators[i],
>> +                        &config);
>> +        if (IS_ERR(rdev)) {
>> +            dev_err(tps->dev,
>> +                "Failed to register %s regulator\n",
>> +                regulators[i].name);
>
> This will be called from probe in 7/7.
> So this could be return dev_err_probe()
>
I left these as dev_err(), since dev_err_probe() is used when there is a chance
-EPROBE_DEFER is returned. For both functions using dev_err() here, -ENOMEM is returned.
Should I still switch these 2 instances to dev_err_probe()?

Thank you for your help!

>> +
>> +            return PTR_ERR(rdev);
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int tps65219_request_irqs(struct tps65219_regulator_irq_type 
>> *irq_types,
>> +                 struct tps65219 *tps, struct platform_device *pdev,
>> +                 struct tps65219_regulator_irq_data *irq_data,
>> +                 unsigned int arr_size)
>> +{
>> +    int i;
>> +    int irq;
>> +    int error;
>> +    struct tps65219_regulator_irq_type *irq_type;
>> +
>> +    for (i = 0; i < arr_size; ++i) {
>> +        irq_type = &irq_types[i];
>> +
>> +        irq = platform_get_irq_byname(pdev, irq_type->irq_name);
>> +        if (irq < 0)
>> +            return -EINVAL;
>> +
>> +        irq_data[i].dev = tps->dev;
>> +        irq_data[i].type = irq_type;
>> +
>> +        error = devm_request_threaded_irq(tps->dev, irq, NULL,
>> +                          tps65219_regulator_irq_handler,
>> +                          IRQF_ONESHOT,
>> +                          irq_type->irq_name,
>> +                          &irq_data[i]);
>> +        if (error) {
>> +            dev_err(tps->dev,
>> +                "Failed to request %s IRQ %d: %d\n",
>> +                irq_type->irq_name, irq, error);
>
> This will be called from probe in 7/7.
> So this could be return dev_err_probe()
>
>> +            return error;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static int tps65219_regulator_probe(struct platform_device *pdev)
>>   {
>>       struct tps65219 *tps = dev_get_drvdata(pdev->dev.parent);
>

-- 
Best,
Shree Ramamoorthy
PMIC Software Engineer

Re: [PATCH v1 6/7] regulator: tps65215: Define probe() helper functions

[Christophe JAILLET]

Le 03/01/2025 à 00:41, Shree Ramamoorthy a écrit :
> Hi,
> 
> On 1/1/25 5:01 AM, Christophe JAILLET wrote:
>> Le 26/12/2024 à 22:54, Shree Ramamoorthy a écrit :
>>> Factor register_regulators() and request_irqs() out into smaller 
>>> functions.
>>> These 2 helper functions are used in the next restructure probe() 
>>> patch to
>>> go through the common (overlapping) regulators and irqs first, then the
>>> device-specific structs identifed in the chip_data struct.
>>>
>>> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy- 
>>> l0cyMroinI0@public.gmane.org>
>>> ---
>>>   drivers/regulator/tps65219-regulator.c | 64 ++++++++++++++++++++++++++
>>>   1 file changed, 64 insertions(+)
>>>
>>> diff --git a/drivers/regulator/tps65219-regulator.c b/drivers/ 
>>> regulator/tps65219-regulator.c
>>> index 13f0e68d8e85..8469ee89802c 100644
>>> --- a/drivers/regulator/tps65219-regulator.c
>>> +++ b/drivers/regulator/tps65219-regulator.c
>>> @@ -346,6 +346,70 @@ static struct chip_data chip_info_table[] = {
>>>       },
>>>   };
>>>   +static int tps65219_register_regulators(const struct 
>>> regulator_desc *regulators,
>>> +                    struct tps65219 *tps,
>>> +                    struct device *dev,
>>> +                    struct regulator_config config,
>>> +                    unsigned int arr_size)
>>> +{
>>> +    int i;
>>> +    struct regulator_dev *rdev;
>>> +
>>> +    config.driver_data = tps;
>>> +    config.dev = tps->dev;
>>> +    config.regmap = tps->regmap;
>>> +
>>> +    for (i = 0; i < arr_size; i++) {
>>> +        rdev = devm_regulator_register(dev, &regulators[i],
>>> +                        &config);
>>> +        if (IS_ERR(rdev)) {
>>> +            dev_err(tps->dev,
>>> +                "Failed to register %s regulator\n",
>>> +                regulators[i].name);
>>
>> This will be called from probe in 7/7.
>> So this could be return dev_err_probe()
>>
> I left these as dev_err(), since dev_err_probe() is used when there is a 
> chance
> -EPROBE_DEFER is returned. For both functions using dev_err() here, - 
> ENOMEM is returned.
> Should I still switch these 2 instances to dev_err_probe()?
> 
> Thank you for your help!
> 
>>> +
>>> +            return PTR_ERR(rdev);
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int tps65219_request_irqs(struct tps65219_regulator_irq_type 
>>> *irq_types,
>>> +                 struct tps65219 *tps, struct platform_device *pdev,
>>> +                 struct tps65219_regulator_irq_data *irq_data,
>>> +                 unsigned int arr_size)
>>> +{
>>> +    int i;
>>> +    int irq;
>>> +    int error;
>>> +    struct tps65219_regulator_irq_type *irq_type;
>>> +
>>> +    for (i = 0; i < arr_size; ++i) {
>>> +        irq_type = &irq_types[i];
>>> +
>>> +        irq = platform_get_irq_byname(pdev, irq_type->irq_name);
>>> +        if (irq < 0)
>>> +            return -EINVAL;
>>> +
>>> +        irq_data[i].dev = tps->dev;
>>> +        irq_data[i].type = irq_type;
>>> +
>>> +        error = devm_request_threaded_irq(tps->dev, irq, NULL,
>>> +                          tps65219_regulator_irq_handler,
>>> +                          IRQF_ONESHOT,
>>> +                          irq_type->irq_name,
>>> +                          &irq_data[i]);
>>> +        if (error) {
>>> +            dev_err(tps->dev,
>>> +                "Failed to request %s IRQ %d: %d\n",
>>> +                irq_type->irq_name, irq, error);
>>
>> This will be called from probe in 7/7.
>> So this could be return dev_err_probe()

Up to you to choose one or the other.

The other advantages of using dev_err_probe() are:
   - log the error code in a human readable format
   - combined with return, it usually saves a few LoC, because some { } 
can be removed most of the time.

CJ

>>
>>> +            return error;
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static int tps65219_regulator_probe(struct platform_device *pdev)
>>>   {
>>>       struct tps65219 *tps = dev_get_drvdata(pdev->dev.parent);
>>
> 

Re: [PATCH v1 6/7] regulator: tps65215: Define probe() helper functions

[Roger Quadros]

On 03/01/2025 01:41, Shree Ramamoorthy wrote:
> Hi,
> 
> On 1/1/25 5:01 AM, Christophe JAILLET wrote:
>> Le 26/12/2024 à 22:54, Shree Ramamoorthy a écrit :
>>> Factor register_regulators() and request_irqs() out into smaller functions.
>>> These 2 helper functions are used in the next restructure probe() patch to
>>> go through the common (overlapping) regulators and irqs first, then the
>>> device-specific structs identifed in the chip_data struct.
>>>
>>> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy-l0cyMroinI0@public.gmane.org>
>>> ---
>>>   drivers/regulator/tps65219-regulator.c | 64 ++++++++++++++++++++++++++
>>>   1 file changed, 64 insertions(+)
>>>
>>> diff --git a/drivers/regulator/tps65219-regulator.c b/drivers/regulator/tps65219-regulator.c
>>> index 13f0e68d8e85..8469ee89802c 100644
>>> --- a/drivers/regulator/tps65219-regulator.c
>>> +++ b/drivers/regulator/tps65219-regulator.c
>>> @@ -346,6 +346,70 @@ static struct chip_data chip_info_table[] = {
>>>       },
>>>   };
>>>   +static int tps65219_register_regulators(const struct regulator_desc *regulators,
>>> +                    struct tps65219 *tps,
>>> +                    struct device *dev,
>>> +                    struct regulator_config config,
>>> +                    unsigned int arr_size)
>>> +{
>>> +    int i;
>>> +    struct regulator_dev *rdev;
>>> +
>>> +    config.driver_data = tps;
>>> +    config.dev = tps->dev;
>>> +    config.regmap = tps->regmap;
>>> +
>>> +    for (i = 0; i < arr_size; i++) {
>>> +        rdev = devm_regulator_register(dev, &regulators[i],
>>> +                        &config);
>>> +        if (IS_ERR(rdev)) {
>>> +            dev_err(tps->dev,
>>> +                "Failed to register %s regulator\n",
>>> +                regulators[i].name);
>>
>> This will be called from probe in 7/7.
>> So this could be return dev_err_probe()
>>
> I left these as dev_err(), since dev_err_probe() is used when there is a chance
> -EPROBE_DEFER is returned. For both functions using dev_err() here, -ENOMEM is returned.
> Should I still switch these 2 instances to dev_err_probe()?
> 
> Thank you for your help!

What you coudld to is simply return error here and
add the dev_err_probe() in the probe function.

> 
>>> +
>>> +            return PTR_ERR(rdev);
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int tps65219_request_irqs(struct tps65219_regulator_irq_type *irq_types,
>>> +                 struct tps65219 *tps, struct platform_device *pdev,
>>> +                 struct tps65219_regulator_irq_data *irq_data,
>>> +                 unsigned int arr_size)
>>> +{
>>> +    int i;
>>> +    int irq;
>>> +    int error;
>>> +    struct tps65219_regulator_irq_type *irq_type;
>>> +
>>> +    for (i = 0; i < arr_size; ++i) {
>>> +        irq_type = &irq_types[i];
>>> +
>>> +        irq = platform_get_irq_byname(pdev, irq_type->irq_name);
>>> +        if (irq < 0)
>>> +            return -EINVAL;
>>> +
>>> +        irq_data[i].dev = tps->dev;
>>> +        irq_data[i].type = irq_type;
>>> +
>>> +        error = devm_request_threaded_irq(tps->dev, irq, NULL,
>>> +                          tps65219_regulator_irq_handler,
>>> +                          IRQF_ONESHOT,
>>> +                          irq_type->irq_name,
>>> +                          &irq_data[i]);
>>> +        if (error) {
>>> +            dev_err(tps->dev,
>>> +                "Failed to request %s IRQ %d: %d\n",
>>> +                irq_type->irq_name, irq, error);
>>
>> This will be called from probe in 7/7.
>> So this could be return dev_err_probe()

Same here, just return error here and leave the error printing
job for the probe function.

>>
>>> +            return error;
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static int tps65219_regulator_probe(struct platform_device *pdev)
>>>   {
>>>       struct tps65219 *tps = dev_get_drvdata(pdev->dev.parent);
>>
> 

-- 
cheers,
-roger

Re: [PATCH v1 6/7] regulator: tps65215: Define probe() helper functions

[Roger Quadros]

On 26/12/2024 23:54, Shree Ramamoorthy wrote:
> Factor register_regulators() and request_irqs() out into smaller functions.
> These 2 helper functions are used in the next restructure probe() patch to
> go through the common (overlapping) regulators and irqs first, then the
> device-specific structs identifed in the chip_data struct.
> 
> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
> ---
>  drivers/regulator/tps65219-regulator.c | 64 ++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/drivers/regulator/tps65219-regulator.c b/drivers/regulator/tps65219-regulator.c
> index 13f0e68d8e85..8469ee89802c 100644
> --- a/drivers/regulator/tps65219-regulator.c
> +++ b/drivers/regulator/tps65219-regulator.c
> @@ -346,6 +346,70 @@ static struct chip_data chip_info_table[] = {
>  	},
>  };
>  
> +static int tps65219_register_regulators(const struct regulator_desc *regulators,
> +					struct tps65219 *tps,
> +					struct device *dev,
> +					struct regulator_config config,
> +					unsigned int arr_size)
> +{
> +	int i;
> +	struct regulator_dev *rdev;

reverse xmas tree?

> +
> +	config.driver_data = tps;
> +	config.dev = tps->dev;
> +	config.regmap = tps->regmap;
> +
> +	for (i = 0; i < arr_size; i++) {
> +		rdev = devm_regulator_register(dev, &regulators[i],
> +						&config);
> +		if (IS_ERR(rdev)) {
> +			dev_err(tps->dev,
> +				"Failed to register %s regulator\n",
> +				regulators[i].name);
> +
> +			return PTR_ERR(rdev);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int tps65219_request_irqs(struct tps65219_regulator_irq_type *irq_types,
> +				 struct tps65219 *tps, struct platform_device *pdev,
> +				 struct tps65219_regulator_irq_data *irq_data,
> +				 unsigned int arr_size)
> +{
> +	int i;
> +	int irq;
> +	int error;
> +	struct tps65219_regulator_irq_type *irq_type;

here too.

> +
> +	for (i = 0; i < arr_size; ++i) {
> +		irq_type = &irq_types[i];
> +

unnecessary new line.

> +		irq = platform_get_irq_byname(pdev, irq_type->irq_name);
> +		if (irq < 0)
> +			return -EINVAL;
> +
> +		irq_data[i].dev = tps->dev;
> +		irq_data[i].type = irq_type;
> +

here too

> +		error = devm_request_threaded_irq(tps->dev, irq, NULL,
> +						  tps65219_regulator_irq_handler,
> +						  IRQF_ONESHOT,
> +						  irq_type->irq_name,
> +						  &irq_data[i]);
> +		if (error) {
> +			dev_err(tps->dev,
> +				"Failed to request %s IRQ %d: %d\n",
> +				irq_type->irq_name, irq, error);
> +			return error;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int tps65219_regulator_probe(struct platform_device *pdev)
>  {
>  	struct tps65219 *tps = dev_get_drvdata(pdev->dev.parent);

This patch by itself will complain during build as there are no users for
these functions.
Could you please squash patches 6 and 7?

-- 
cheers,
-roger

Re: [PATCH v1 6/7] regulator: tps65215: Define probe() helper functions

[Shree Ramamoorthy]

Hi,

On 1/4/2025 12:45 PM, Roger Quadros wrote:
>
> On 26/12/2024 23:54, Shree Ramamoorthy wrote:
>> Factor register_regulators() and request_irqs() out into smaller functions.
>> These 2 helper functions are used in the next restructure probe() patch to
>> go through the common (overlapping) regulators and irqs first, then the
>> device-specific structs identifed in the chip_data struct.
>>
>> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
>> ---
>>  drivers/regulator/tps65219-regulator.c | 64 ++++++++++++++++++++++++++
>>  1 file changed, 64 insertions(+)
>>
>> diff --git a/drivers/regulator/tps65219-regulator.c b/drivers/regulator/tps65219-regulator.c
>> index 13f0e68d8e85..8469ee89802c 100644
>> --- a/drivers/regulator/tps65219-regulator.c
>> +++ b/drivers/regulator/tps65219-regulator.c
>> @@ -346,6 +346,70 @@ static struct chip_data chip_info_table[] = {
>>  	},
>>  };
>>  
>> +static int tps65219_register_regulators(const struct regulator_desc *regulators,
>> +					struct tps65219 *tps,
>> +					struct device *dev,
>> +					struct regulator_config config,
>> +					unsigned int arr_size)
>> +{
>> +	int i;
>> +	struct regulator_dev *rdev;
> reverse xmas tree?

Applied reverse xmas tree style to this file & will review other files as well for this.

>> +
>> +	config.driver_data = tps;
>> +	config.dev = tps->dev;
>> +	config.regmap = tps->regmap;
>> +
>> +	for (i = 0; i < arr_size; i++) {
>> +		rdev = devm_regulator_register(dev, &regulators[i],
>> +						&config);
>> +		if (IS_ERR(rdev)) {
>> +			dev_err(tps->dev,
>> +				"Failed to register %s regulator\n",
>> +				regulators[i].name);
>> +
>> +			return PTR_ERR(rdev);
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int tps65219_request_irqs(struct tps65219_regulator_irq_type *irq_types,
>> +				 struct tps65219 *tps, struct platform_device *pdev,
>> +				 struct tps65219_regulator_irq_data *irq_data,
>> +				 unsigned int arr_size)
>> +{
>> +	int i;
>> +	int irq;
>> +	int error;
>> +	struct tps65219_regulator_irq_type *irq_type;
> here too.
>
>> +
>> +	for (i = 0; i < arr_size; ++i) {
>> +		irq_type = &irq_types[i];
>> +
> unnecessary new line.
>
>> +		irq = platform_get_irq_byname(pdev, irq_type->irq_name);
>> +		if (irq < 0)
>> +			return -EINVAL;
>> +
>> +		irq_data[i].dev = tps->dev;
>> +		irq_data[i].type = irq_type;
>> +
> here too

Removed both new lines.

>> +		error = devm_request_threaded_irq(tps->dev, irq, NULL,
>> +						  tps65219_regulator_irq_handler,
>> +						  IRQF_ONESHOT,
>> +						  irq_type->irq_name,
>> +						  &irq_data[i]);
>> +		if (error) {
>> +			dev_err(tps->dev,
>> +				"Failed to request %s IRQ %d: %d\n",
>> +				irq_type->irq_name, irq, error);
>> +			return error;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static int tps65219_regulator_probe(struct platform_device *pdev)
>>  {
>>  	struct tps65219 *tps = dev_get_drvdata(pdev->dev.parent);
> This patch by itself will complain during build as there are no users for
> these functions.
> Could you please squash patches 6 and 7?

I kept patch 6 and 7 separate as the diff was hard to read & 
the git diff options did not resolve this. Is there a way to keep these 2 patches 
separate for user readability and avoid the build error? Or just squash them to 
prevent build errors knowing the diff will be hard to read? Thank you for your help!

Re: [PATCH v1 6/7] regulator: tps65215: Define probe() helper functions

[Andrew Davis]

On 1/6/25 4:02 PM, Shree Ramamoorthy wrote:
> Hi,
> 
> On 1/4/2025 12:45 PM, Roger Quadros wrote:
>>
>> On 26/12/2024 23:54, Shree Ramamoorthy wrote:
>>> Factor register_regulators() and request_irqs() out into smaller functions.
>>> These 2 helper functions are used in the next restructure probe() patch to
>>> go through the common (overlapping) regulators and irqs first, then the
>>> device-specific structs identifed in the chip_data struct.
>>>
>>> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
>>> ---
>>>   drivers/regulator/tps65219-regulator.c | 64 ++++++++++++++++++++++++++
>>>   1 file changed, 64 insertions(+)
>>>
>>> diff --git a/drivers/regulator/tps65219-regulator.c b/drivers/regulator/tps65219-regulator.c
>>> index 13f0e68d8e85..8469ee89802c 100644
>>> --- a/drivers/regulator/tps65219-regulator.c
>>> +++ b/drivers/regulator/tps65219-regulator.c
>>> @@ -346,6 +346,70 @@ static struct chip_data chip_info_table[] = {
>>>   	},
>>>   };
>>>   
>>> +static int tps65219_register_regulators(const struct regulator_desc *regulators,
>>> +					struct tps65219 *tps,
>>> +					struct device *dev,
>>> +					struct regulator_config config,
>>> +					unsigned int arr_size)
>>> +{
>>> +	int i;
>>> +	struct regulator_dev *rdev;
>> reverse xmas tree?
> 
> Applied reverse xmas tree style to this file & will review other files as well for this.
> 
>>> +
>>> +	config.driver_data = tps;
>>> +	config.dev = tps->dev;
>>> +	config.regmap = tps->regmap;
>>> +
>>> +	for (i = 0; i < arr_size; i++) {
>>> +		rdev = devm_regulator_register(dev, &regulators[i],
>>> +						&config);
>>> +		if (IS_ERR(rdev)) {
>>> +			dev_err(tps->dev,
>>> +				"Failed to register %s regulator\n",
>>> +				regulators[i].name);
>>> +
>>> +			return PTR_ERR(rdev);
>>> +		}
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int tps65219_request_irqs(struct tps65219_regulator_irq_type *irq_types,
>>> +				 struct tps65219 *tps, struct platform_device *pdev,
>>> +				 struct tps65219_regulator_irq_data *irq_data,
>>> +				 unsigned int arr_size)
>>> +{
>>> +	int i;
>>> +	int irq;
>>> +	int error;
>>> +	struct tps65219_regulator_irq_type *irq_type;
>> here too.
>>
>>> +
>>> +	for (i = 0; i < arr_size; ++i) {
>>> +		irq_type = &irq_types[i];
>>> +
>> unnecessary new line.
>>
>>> +		irq = platform_get_irq_byname(pdev, irq_type->irq_name);
>>> +		if (irq < 0)
>>> +			return -EINVAL;
>>> +
>>> +		irq_data[i].dev = tps->dev;
>>> +		irq_data[i].type = irq_type;
>>> +
>> here too
> 
> Removed both new lines.
> 
>>> +		error = devm_request_threaded_irq(tps->dev, irq, NULL,
>>> +						  tps65219_regulator_irq_handler,
>>> +						  IRQF_ONESHOT,
>>> +						  irq_type->irq_name,
>>> +						  &irq_data[i]);
>>> +		if (error) {
>>> +			dev_err(tps->dev,
>>> +				"Failed to request %s IRQ %d: %d\n",
>>> +				irq_type->irq_name, irq, error);
>>> +			return error;
>>> +		}
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>   static int tps65219_regulator_probe(struct platform_device *pdev)
>>>   {
>>>   	struct tps65219 *tps = dev_get_drvdata(pdev->dev.parent);
>> This patch by itself will complain during build as there are no users for
>> these functions.
>> Could you please squash patches 6 and 7?
> 
> I kept patch 6 and 7 separate as the diff was hard to read &
> the git diff options did not resolve this. Is there a way to keep these 2 patches
> separate for user readability and avoid the build error? Or just squash them to
> prevent build errors knowing the diff will be hard to read? Thank you for your help!
> 
> 

Instead of splitting the adding and the using of the functions, could you
split tps65219_register_regulators() and tps65219_request_irqs() into their
own patches? Each patch should add and also make use of the added function.

Andrew

Re: [PATCH v1 6/7] regulator: tps65215: Define probe() helper functions

[Shree Ramamoorthy]

Hi,

On 1/6/25 4:57 PM, Andrew Davis wrote:
> On 1/6/25 4:02 PM, Shree Ramamoorthy wrote:
>> Hi,
>>
>> On 1/4/2025 12:45 PM, Roger Quadros wrote:
>>>
>>> On 26/12/2024 23:54, Shree Ramamoorthy wrote:
>>>> Factor register_regulators() and request_irqs() out into smaller 
>>>> functions.
>>>> These 2 helper functions are used in the next restructure probe() 
>>>> patch to
>>>> go through the common (overlapping) regulators and irqs first, then 
>>>> the
>>>> device-specific structs identifed in the chip_data struct.
>>>>
>>>> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
>>>> ---
>>>>   drivers/regulator/tps65219-regulator.c | 64 
>>>> ++++++++++++++++++++++++++
>>>>   1 file changed, 64 insertions(+)
>>>>
>>>> diff --git a/drivers/regulator/tps65219-regulator.c 
>>>> b/drivers/regulator/tps65219-regulator.c
>>>> index 13f0e68d8e85..8469ee89802c 100644
>>>> --- a/drivers/regulator/tps65219-regulator.c
>>>> +++ b/drivers/regulator/tps65219-regulator.c
>>>> @@ -346,6 +346,70 @@ static struct chip_data chip_info_table[] = {
>>>>       },
>>>>   };
>>>>   +static int tps65219_register_regulators(const struct 
>>>> regulator_desc *regulators,
>>>> +                    struct tps65219 *tps,
>>>> +                    struct device *dev,
>>>> +                    struct regulator_config config,
>>>> +                    unsigned int arr_size)
>>>> +{
>>>> +    int i;
>>>> +    struct regulator_dev *rdev;
>>> reverse xmas tree?
>>
>> Applied reverse xmas tree style to this file & will review other 
>> files as well for this.
>>
>>>> +
>>>> +    config.driver_data = tps;
>>>> +    config.dev = tps->dev;
>>>> +    config.regmap = tps->regmap;
>>>> +
>>>> +    for (i = 0; i < arr_size; i++) {
>>>> +        rdev = devm_regulator_register(dev, &regulators[i],
>>>> +                        &config);
>>>> +        if (IS_ERR(rdev)) {
>>>> +            dev_err(tps->dev,
>>>> +                "Failed to register %s regulator\n",
>>>> +                regulators[i].name);
>>>> +
>>>> +            return PTR_ERR(rdev);
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int tps65219_request_irqs(struct 
>>>> tps65219_regulator_irq_type *irq_types,
>>>> +                 struct tps65219 *tps, struct platform_device *pdev,
>>>> +                 struct tps65219_regulator_irq_data *irq_data,
>>>> +                 unsigned int arr_size)
>>>> +{
>>>> +    int i;
>>>> +    int irq;
>>>> +    int error;
>>>> +    struct tps65219_regulator_irq_type *irq_type;
>>> here too.
>>>
>>>> +
>>>> +    for (i = 0; i < arr_size; ++i) {
>>>> +        irq_type = &irq_types[i];
>>>> +
>>> unnecessary new line.
>>>
>>>> +        irq = platform_get_irq_byname(pdev, irq_type->irq_name);
>>>> +        if (irq < 0)
>>>> +            return -EINVAL;
>>>> +
>>>> +        irq_data[i].dev = tps->dev;
>>>> +        irq_data[i].type = irq_type;
>>>> +
>>> here too
>>
>> Removed both new lines.
>>
>>>> +        error = devm_request_threaded_irq(tps->dev, irq, NULL,
>>>> +                          tps65219_regulator_irq_handler,
>>>> +                          IRQF_ONESHOT,
>>>> +                          irq_type->irq_name,
>>>> +                          &irq_data[i]);
>>>> +        if (error) {
>>>> +            dev_err(tps->dev,
>>>> +                "Failed to request %s IRQ %d: %d\n",
>>>> +                irq_type->irq_name, irq, error);
>>>> +            return error;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>   static int tps65219_regulator_probe(struct platform_device *pdev)
>>>>   {
>>>>       struct tps65219 *tps = dev_get_drvdata(pdev->dev.parent);
>>> This patch by itself will complain during build as there are no 
>>> users for
>>> these functions.
>>> Could you please squash patches 6 and 7?
>>
>> I kept patch 6 and 7 separate as the diff was hard to read &
>> the git diff options did not resolve this. Is there a way to keep 
>> these 2 patches
>> separate for user readability and avoid the build error? Or just 
>> squash them to
>> prevent build errors knowing the diff will be hard to read? Thank you 
>> for your help!
>>
>>
>
> Instead of splitting the adding and the using of the functions, could you
> split tps65219_register_regulators() and tps65219_request_irqs() into 
> their
> own patches? Each patch should add and also make use of the added 
> function.
>
> Andrew

I was able to split up the 2 helper functions & usage into their own patches. The diff is clean
except for a mistaken new function, but it's easy to read compared to squashing this patch with 7/7.


-- 
Best,
Shree Ramamoorthy
PMIC Software Engineer

[PATCH v1 7/7] regulator: tps65215: Restructure probe() for multi-PMIC support

[Shree Ramamoorthy]

The probe() function will now utilize the register_regulators() and
request_irqs() helper functions defined in the previous patch. Probe() will
cycle through common (overlapping) regulators and irqs first, and then
handle device-specific resources identified using the chip_data struct.

Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
---
 drivers/regulator/tps65219-regulator.c | 66 +++++++++++---------------
 1 file changed, 27 insertions(+), 39 deletions(-)

diff --git a/drivers/regulator/tps65219-regulator.c b/drivers/regulator/tps65219-regulator.c
index 8469ee89802c..b27888fd1fa8 100644
--- a/drivers/regulator/tps65219-regulator.c
+++ b/drivers/regulator/tps65219-regulator.c
@@ -413,54 +413,42 @@ static int tps65219_request_irqs(struct tps65219_regulator_irq_type *irq_types,
 static int tps65219_regulator_probe(struct platform_device *pdev)
 {
 	struct tps65219 *tps = dev_get_drvdata(pdev->dev.parent);
-	struct regulator_dev *rdev;
 	struct regulator_config config = { };
-	int i;
 	int error;
-	int irq;
 	struct tps65219_regulator_irq_data *irq_data;
-	struct tps65219_regulator_irq_type *irq_type;
+	struct chip_data *pmic;
 
-	config.dev = tps->dev;
-	config.driver_data = tps;
-	config.regmap = tps->regmap;
 
-	for (i = 0; i < ARRAY_SIZE(regulators); i++) {
-		rdev = devm_regulator_register(&pdev->dev, &regulators[i],
-					       &config);
-		if (IS_ERR(rdev))
-			return dev_err_probe(tps->dev, PTR_ERR(rdev),
-					"Failed to register %s regulator\n",
-					regulators[i].name);
-	}
-
-	irq_data = devm_kmalloc(tps->dev,
-				ARRAY_SIZE(tps65219_regulator_irq_types) *
-				sizeof(struct tps65219_regulator_irq_data),
-				GFP_KERNEL);
-	if (!irq_data)
-		return -ENOMEM;
-
-	for (i = 0; i < ARRAY_SIZE(tps65219_regulator_irq_types); ++i) {
-		irq_type = &tps65219_regulator_irq_types[i];
+	enum pmic_id chip = platform_get_device_id(pdev)->driver_data;
 
-		irq = platform_get_irq_byname(pdev, irq_type->irq_name);
-		if (irq < 0)
-			return -EINVAL;
+	pmic = &chip_info_table[chip];
 
-		irq_data[i].dev = tps->dev;
-		irq_data[i].type = irq_type;
+	config.dev = tps->dev;
+	config.driver_data = tps;
+	config.regmap = tps->regmap;
 
-		error = devm_request_threaded_irq(tps->dev, irq, NULL,
-						  tps65219_regulator_irq_handler,
-						  IRQF_ONESHOT,
-						  irq_type->irq_name,
-						  &irq_data[i]);
-		if (error) {
-			dev_err(tps->dev, "failed to request %s IRQ %d: %d\n",
-				irq_type->irq_name, irq, error);
+	error = tps65219_register_regulators(pmic->common_rdesc, tps,
+						&pdev->dev, config, pmic->common_rdesc_size);
+	if (error)
+		return error;
+
+	error = tps65219_register_regulators(pmic->rdesc, tps, &pdev->dev,
+						config, pmic->rdesc_size);
+	if (error)
+		return error;
+
+	irq_data = devm_kmalloc(tps->dev, pmic->common_irq_size, GFP_KERNEL);
+	error = tps65219_request_irqs(pmic->common_irq_types, tps, pdev,
+					irq_data, pmic->common_irq_size);
+	if (error)
+		return error;
+
+	if (chip == TPS65219) {
+		irq_data = devm_kmalloc(tps->dev, pmic->dev_irq_size, GFP_KERNEL);
+		error = tps65219_request_irqs(pmic->irq_types, tps, pdev,
+						irq_data, pmic->dev_irq_size);
+		if (error)
 			return error;
-		}
 	}
 
 	return 0;
-- 
2.34.1

Re: [PATCH v1 7/7] regulator: tps65215: Restructure probe() for multi-PMIC support

[Christophe JAILLET]

Le 26/12/2024 à 22:54, Shree Ramamoorthy a écrit :
> The probe() function will now utilize the register_regulators() and
> request_irqs() helper functions defined in the previous patch. Probe() will
> cycle through common (overlapping) regulators and irqs first, and then
> handle device-specific resources identified using the chip_data struct.
> 
> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
> ---
>   drivers/regulator/tps65219-regulator.c | 66 +++++++++++---------------
>   1 file changed, 27 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/regulator/tps65219-regulator.c b/drivers/regulator/tps65219-regulator.c
> index 8469ee89802c..b27888fd1fa8 100644
> --- a/drivers/regulator/tps65219-regulator.c
> +++ b/drivers/regulator/tps65219-regulator.c
> @@ -413,54 +413,42 @@ static int tps65219_request_irqs(struct tps65219_regulator_irq_type *irq_types,
>   static int tps65219_regulator_probe(struct platform_device *pdev)
>   {
>   	struct tps65219 *tps = dev_get_drvdata(pdev->dev.parent);
> -	struct regulator_dev *rdev;
>   	struct regulator_config config = { };
> -	int i;
>   	int error;
> -	int irq;
>   	struct tps65219_regulator_irq_data *irq_data;
> -	struct tps65219_regulator_irq_type *irq_type;
> +	struct chip_data *pmic;
>   
> -	config.dev = tps->dev;
> -	config.driver_data = tps;
> -	config.regmap = tps->regmap;
>   
> -	for (i = 0; i < ARRAY_SIZE(regulators); i++) {
> -		rdev = devm_regulator_register(&pdev->dev, &regulators[i],
> -					       &config);
> -		if (IS_ERR(rdev))
> -			return dev_err_probe(tps->dev, PTR_ERR(rdev),
> -					"Failed to register %s regulator\n",
> -					regulators[i].name);
> -	}
> -
> -	irq_data = devm_kmalloc(tps->dev,
> -				ARRAY_SIZE(tps65219_regulator_irq_types) *
> -				sizeof(struct tps65219_regulator_irq_data),
> -				GFP_KERNEL);
> -	if (!irq_data)
> -		return -ENOMEM;
> -
> -	for (i = 0; i < ARRAY_SIZE(tps65219_regulator_irq_types); ++i) {
> -		irq_type = &tps65219_regulator_irq_types[i];
> +	enum pmic_id chip = platform_get_device_id(pdev)->driver_data;
>   
> -		irq = platform_get_irq_byname(pdev, irq_type->irq_name);
> -		if (irq < 0)
> -			return -EINVAL;
> +	pmic = &chip_info_table[chip];
>   
> -		irq_data[i].dev = tps->dev;
> -		irq_data[i].type = irq_type;
> +	config.dev = tps->dev;
> +	config.driver_data = tps;
> +	config.regmap = tps->regmap;
>   
> -		error = devm_request_threaded_irq(tps->dev, irq, NULL,
> -						  tps65219_regulator_irq_handler,
> -						  IRQF_ONESHOT,
> -						  irq_type->irq_name,
> -						  &irq_data[i]);
> -		if (error) {
> -			dev_err(tps->dev, "failed to request %s IRQ %d: %d\n",
> -				irq_type->irq_name, irq, error);
> +	error = tps65219_register_regulators(pmic->common_rdesc, tps,
> +						&pdev->dev, config, pmic->common_rdesc_size);
> +	if (error)
> +		return error;
> +
> +	error = tps65219_register_regulators(pmic->rdesc, tps, &pdev->dev,
> +						config, pmic->rdesc_size);
> +	if (error)
> +		return error;
> +
> +	irq_data = devm_kmalloc(tps->dev, pmic->common_irq_size, GFP_KERNEL);

Error handling, as done previously?

> +	error = tps65219_request_irqs(pmic->common_irq_types, tps, pdev,
> +					irq_data, pmic->common_irq_size);
> +	if (error)
> +		return error;
> +
> +	if (chip == TPS65219) {
> +		irq_data = devm_kmalloc(tps->dev, pmic->dev_irq_size, GFP_KERNEL);

Error handling?

> +		error = tps65219_request_irqs(pmic->irq_types, tps, pdev,
> +						irq_data, pmic->dev_irq_size);
> +		if (error)
>   			return error;
> -		}
>   	}
>   
>   	return 0;

Re: [PATCH v1 7/7] regulator: tps65215: Restructure probe() for multi-PMIC support

[Shree Ramamoorthy]

Hi,

On 1/1/25 5:04 AM, Christophe JAILLET wrote:
> Le 26/12/2024 à 22:54, Shree Ramamoorthy a écrit :
>> The probe() function will now utilize the register_regulators() and
>> request_irqs() helper functions defined in the previous patch. 
>> Probe() will
>> cycle through common (overlapping) regulators and irqs first, and then
>> handle device-specific resources identified using the chip_data struct.
>>
>> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
>> ---
>>   drivers/regulator/tps65219-regulator.c | 66 +++++++++++---------------
>>   1 file changed, 27 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/regulator/tps65219-regulator.c 
>> b/drivers/regulator/tps65219-regulator.c
>> index 8469ee89802c..b27888fd1fa8 100644
>> --- a/drivers/regulator/tps65219-regulator.c
>> +++ b/drivers/regulator/tps65219-regulator.c
>> @@ -413,54 +413,42 @@ static int tps65219_request_irqs(struct 
>> tps65219_regulator_irq_type *irq_types,
>>   static int tps65219_regulator_probe(struct platform_device *pdev)
>>   {
>>       struct tps65219 *tps = dev_get_drvdata(pdev->dev.parent);
>> -    struct regulator_dev *rdev;
>>       struct regulator_config config = { };
>> -    int i;
>>       int error;
>> -    int irq;
>>       struct tps65219_regulator_irq_data *irq_data;
>> -    struct tps65219_regulator_irq_type *irq_type;
>> +    struct chip_data *pmic;
>>   -    config.dev = tps->dev;
>> -    config.driver_data = tps;
>> -    config.regmap = tps->regmap;
>>   -    for (i = 0; i < ARRAY_SIZE(regulators); i++) {
>> -        rdev = devm_regulator_register(&pdev->dev, &regulators[i],
>> -                           &config);
>> -        if (IS_ERR(rdev))
>> -            return dev_err_probe(tps->dev, PTR_ERR(rdev),
>> -                    "Failed to register %s regulator\n",
>> -                    regulators[i].name);
>> -    }
>> -
>> -    irq_data = devm_kmalloc(tps->dev,
>> -                ARRAY_SIZE(tps65219_regulator_irq_types) *
>> -                sizeof(struct tps65219_regulator_irq_data),
>> -                GFP_KERNEL);
>> -    if (!irq_data)
>> -        return -ENOMEM;
>> -
>> -    for (i = 0; i < ARRAY_SIZE(tps65219_regulator_irq_types); ++i) {
>> -        irq_type = &tps65219_regulator_irq_types[i];
>> +    enum pmic_id chip = platform_get_device_id(pdev)->driver_data;
>>   -        irq = platform_get_irq_byname(pdev, irq_type->irq_name);
>> -        if (irq < 0)
>> -            return -EINVAL;
>> +    pmic = &chip_info_table[chip];
>>   -        irq_data[i].dev = tps->dev;
>> -        irq_data[i].type = irq_type;
>> +    config.dev = tps->dev;
>> +    config.driver_data = tps;
>> +    config.regmap = tps->regmap;
>>   -        error = devm_request_threaded_irq(tps->dev, irq, NULL,
>> -                          tps65219_regulator_irq_handler,
>> -                          IRQF_ONESHOT,
>> -                          irq_type->irq_name,
>> -                          &irq_data[i]);
>> -        if (error) {
>> -            dev_err(tps->dev, "failed to request %s IRQ %d: %d\n",
>> -                irq_type->irq_name, irq, error);
>> +    error = tps65219_register_regulators(pmic->common_rdesc, tps,
>> +                        &pdev->dev, config, pmic->common_rdesc_size);
>> +    if (error)
>> +        return error;
>> +
>> +    error = tps65219_register_regulators(pmic->rdesc, tps, &pdev->dev,
>> +                        config, pmic->rdesc_size);
>> +    if (error)
>> +        return error;
>> +
>> +    irq_data = devm_kmalloc(tps->dev, pmic->common_irq_size, 
>> GFP_KERNEL);
>
> Error handling, as done previously?

Thanks for catching this, will add in the previous check:

     if (!irq_data)

         return -ENOMEM;

>> +    error = tps65219_request_irqs(pmic->common_irq_types, tps, pdev,
>> +                    irq_data, pmic->common_irq_size);
>> +    if (error)
>> +        return error;
>> +
>> +    if (chip == TPS65219) {
>> +        irq_data = devm_kmalloc(tps->dev, pmic->dev_irq_size, 
>> GFP_KERNEL);
>
> Error handling?
>
>> +        error = tps65219_request_irqs(pmic->irq_types, tps, pdev,
>> +                        irq_data, pmic->dev_irq_size);
>> +        if (error)
>>               return error;
>> -        }
>>       }
>>         return 0;
>

-- 
Best,
Shree Ramamoorthy
PMIC Software Engineer

Re: [PATCH v1 7/7] regulator: tps65215: Restructure probe() for multi-PMIC support

[Roger Quadros]

On 26/12/2024 23:54, Shree Ramamoorthy wrote:
> The probe() function will now utilize the register_regulators() and
> request_irqs() helper functions defined in the previous patch. Probe() will
> cycle through common (overlapping) regulators and irqs first, and then
> handle device-specific resources identified using the chip_data struct.
> 
> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
> ---
>  drivers/regulator/tps65219-regulator.c | 66 +++++++++++---------------
>  1 file changed, 27 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/regulator/tps65219-regulator.c b/drivers/regulator/tps65219-regulator.c
> index 8469ee89802c..b27888fd1fa8 100644
> --- a/drivers/regulator/tps65219-regulator.c
> +++ b/drivers/regulator/tps65219-regulator.c
> @@ -413,54 +413,42 @@ static int tps65219_request_irqs(struct tps65219_regulator_irq_type *irq_types,
>  static int tps65219_regulator_probe(struct platform_device *pdev)
>  {
>  	struct tps65219 *tps = dev_get_drvdata(pdev->dev.parent);
> -	struct regulator_dev *rdev;
>  	struct regulator_config config = { };
> -	int i;
>  	int error;
> -	int irq;
>  	struct tps65219_regulator_irq_data *irq_data;
> -	struct tps65219_regulator_irq_type *irq_type;
> +	struct chip_data *pmic;

reverse xmas tree.

>  
> -	config.dev = tps->dev;
> -	config.driver_data = tps;
> -	config.regmap = tps->regmap;
>  
> -	for (i = 0; i < ARRAY_SIZE(regulators); i++) {
> -		rdev = devm_regulator_register(&pdev->dev, &regulators[i],
> -					       &config);
> -		if (IS_ERR(rdev))
> -			return dev_err_probe(tps->dev, PTR_ERR(rdev),
> -					"Failed to register %s regulator\n",
> -					regulators[i].name);
> -	}
> -
> -	irq_data = devm_kmalloc(tps->dev,
> -				ARRAY_SIZE(tps65219_regulator_irq_types) *
> -				sizeof(struct tps65219_regulator_irq_data),
> -				GFP_KERNEL);
> -	if (!irq_data)
> -		return -ENOMEM;
> -
> -	for (i = 0; i < ARRAY_SIZE(tps65219_regulator_irq_types); ++i) {
> -		irq_type = &tps65219_regulator_irq_types[i];
> +	enum pmic_id chip = platform_get_device_id(pdev)->driver_data;
>  
> -		irq = platform_get_irq_byname(pdev, irq_type->irq_name);
> -		if (irq < 0)
> -			return -EINVAL;

need a new line after declarations.

> +	pmic = &chip_info_table[chip];
>  
> -		irq_data[i].dev = tps->dev;
> -		irq_data[i].type = irq_type;
> +	config.dev = tps->dev;
> +	config.driver_data = tps;
> +	config.regmap = tps->regmap;
>  
> -		error = devm_request_threaded_irq(tps->dev, irq, NULL,
> -						  tps65219_regulator_irq_handler,
> -						  IRQF_ONESHOT,
> -						  irq_type->irq_name,
> -						  &irq_data[i]);
> -		if (error) {
> -			dev_err(tps->dev, "failed to request %s IRQ %d: %d\n",
> -				irq_type->irq_name, irq, error);
> +	error = tps65219_register_regulators(pmic->common_rdesc, tps,
> +						&pdev->dev, config, pmic->common_rdesc_size);
> +	if (error)

maybe you could use goto and do any cleanup
and in the end use dev_err_probe().

> +		return error;
> +
> +	error = tps65219_register_regulators(pmic->rdesc, tps, &pdev->dev,
> +						config, pmic->rdesc_size);
> +	if (error)
> +		return error;
> +
> +	irq_data = devm_kmalloc(tps->dev, pmic->common_irq_size, GFP_KERNEL);
> +	error = tps65219_request_irqs(pmic->common_irq_types, tps, pdev,
> +					irq_data, pmic->common_irq_size);
> +	if (error)
> +		return error;
> +
> +	if (chip == TPS65219) {
> +		irq_data = devm_kmalloc(tps->dev, pmic->dev_irq_size, GFP_KERNEL);
> +		error = tps65219_request_irqs(pmic->irq_types, tps, pdev,
> +						irq_data, pmic->dev_irq_size);
> +		if (error)
>  			return error;
> -		}
>  	}
>  
>  	return 0;

-- 
cheers,
-roger

Re: [PATCH v1 7/7] regulator: tps65215: Restructure probe() for multi-PMIC support

[Shree Ramamoorthy]

Hi,

On 1/4/25 12:47 PM, Roger Quadros wrote:
>
> On 26/12/2024 23:54, Shree Ramamoorthy wrote:
>> The probe() function will now utilize the register_regulators() and
>> request_irqs() helper functions defined in the previous patch. Probe() will
>> cycle through common (overlapping) regulators and irqs first, and then
>> handle device-specific resources identified using the chip_data struct.
>>
>> Signed-off-by: Shree Ramamoorthy <s-ramamoorthy@ti.com>
>> ---
>>   drivers/regulator/tps65219-regulator.c | 66 +++++++++++---------------
>>   1 file changed, 27 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/regulator/tps65219-regulator.c b/drivers/regulator/tps65219-regulator.c
>> index 8469ee89802c..b27888fd1fa8 100644
>> --- a/drivers/regulator/tps65219-regulator.c
>> +++ b/drivers/regulator/tps65219-regulator.c
>> @@ -413,54 +413,42 @@ static int tps65219_request_irqs(struct tps65219_regulator_irq_type *irq_types,
>>   static int tps65219_regulator_probe(struct platform_device *pdev)
>>   {
>>   	struct tps65219 *tps = dev_get_drvdata(pdev->dev.parent);
>> -	struct regulator_dev *rdev;
>>   	struct regulator_config config = { };
>> -	int i;
>>   	int error;
>> -	int irq;
>>   	struct tps65219_regulator_irq_data *irq_data;
>> -	struct tps65219_regulator_irq_type *irq_type;
>> +	struct chip_data *pmic;
> reverse xmas tree.
>
>>   
>> -	config.dev = tps->dev;
>> -	config.driver_data = tps;
>> -	config.regmap = tps->regmap;
>>   
>> -	for (i = 0; i < ARRAY_SIZE(regulators); i++) {
>> -		rdev = devm_regulator_register(&pdev->dev, &regulators[i],
>> -					       &config);
>> -		if (IS_ERR(rdev))
>> -			return dev_err_probe(tps->dev, PTR_ERR(rdev),
>> -					"Failed to register %s regulator\n",
>> -					regulators[i].name);
>> -	}
>> -
>> -	irq_data = devm_kmalloc(tps->dev,
>> -				ARRAY_SIZE(tps65219_regulator_irq_types) *
>> -				sizeof(struct tps65219_regulator_irq_data),
>> -				GFP_KERNEL);
>> -	if (!irq_data)
>> -		return -ENOMEM;
>> -
>> -	for (i = 0; i < ARRAY_SIZE(tps65219_regulator_irq_types); ++i) {
>> -		irq_type = &tps65219_regulator_irq_types[i];
>> +	enum pmic_id chip = platform_get_device_id(pdev)->driver_data;
>>   
>> -		irq = platform_get_irq_byname(pdev, irq_type->irq_name);
>> -		if (irq < 0)
>> -			return -EINVAL;
> need a new line after declarations.
>
>> +	pmic = &chip_info_table[chip];
>>   
>> -		irq_data[i].dev = tps->dev;
>> -		irq_data[i].type = irq_type;
>> +	config.dev = tps->dev;
>> +	config.driver_data = tps;
>> +	config.regmap = tps->regmap;
>>   
>> -		error = devm_request_threaded_irq(tps->dev, irq, NULL,
>> -						  tps65219_regulator_irq_handler,
>> -						  IRQF_ONESHOT,
>> -						  irq_type->irq_name,
>> -						  &irq_data[i]);
>> -		if (error) {
>> -			dev_err(tps->dev, "failed to request %s IRQ %d: %d\n",
>> -				irq_type->irq_name, irq, error);
>> +	error = tps65219_register_regulators(pmic->common_rdesc, tps,
>> +						&pdev->dev, config, pmic->common_rdesc_size);
>> +	if (error)
> maybe you could use goto and do any cleanup
> and in the end use dev_err_probe().

Looking through all of the feedback/ideas, I am planning to have the helper functions return the error,
use goto to check and return dev_err_probe if an error is detected. This should hopefully minimize the
amount of repeated error checks in this probe function. Thank you for the feedback!

>> +		return error;
>> +
>> +	error = tps65219_register_regulators(pmic->rdesc, tps, &pdev->dev,
>> +						config, pmic->rdesc_size);
>> +	if (error)
>> +		return error;
>> +
>> +	irq_data = devm_kmalloc(tps->dev, pmic->common_irq_size, GFP_KERNEL);
>> +	error = tps65219_request_irqs(pmic->common_irq_types, tps, pdev,
>> +					irq_data, pmic->common_irq_size);
>> +	if (error)
>> +		return error;
>> +
>> +	if (chip == TPS65219) {
>> +		irq_data = devm_kmalloc(tps->dev, pmic->dev_irq_size, GFP_KERNEL);
>> +		error = tps65219_request_irqs(pmic->irq_types, tps, pdev,
>> +						irq_data, pmic->dev_irq_size);
>> +		if (error)
>>   			return error;
>> -		}
>>   	}
>>   
>>   	return 0;

-- 
Best,
Shree Ramamoorthy
PMIC Software Engineer