[PATCH 2/3] regulator: qcom_usb_vbus: Add support for PMI8998 VBUS

[PATCH 2/3] regulator: qcom_usb_vbus: Add support for PMI8998 VBUS

[James A. MacInnes]

This patch extends the Qualcomm USB VBUS regulator driver to support
PMI8998 PMIC alongside the existing support for PM8150B.

Key changes:
- Added current limit tables specific to PMI8998.
- Dynamically configure the VBUS regulator based on the PMIC type.
- Updated debug messages to reflect successful initialization for
  supported PMICs.
- Changed registration log message

These changes ensure proper VBUS current limit configuration and
compatibility across multiple Qualcomm PMICs.

Signed-off-by: James A. MacInnes <james.a.macinnes@gmail.com>
---
 drivers/regulator/qcom_usb_vbus-regulator.c | 33 +++++++++++++++++----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/regulator/qcom_usb_vbus-regulator.c b/drivers/regulator/qcom_usb_vbus-regulator.c
index cd94ed67621f..bfcb77698ba2 100644
--- a/drivers/regulator/qcom_usb_vbus-regulator.c
+++ b/drivers/regulator/qcom_usb_vbus-regulator.c
@@ -20,10 +20,15 @@
 #define OTG_CFG				0x53
 #define OTG_EN_SRC_CFG			BIT(1)
 
-static const unsigned int curr_table[] = {
+static const unsigned int curr_table_pm8150b[] = {
 	500000, 1000000, 1500000, 2000000, 2500000, 3000000,
 };
 
+static const unsigned int curr_table_pmi8998[] = {
+	250000, 500000, 750000, 1000000,
+	1250000, 1500000, 1750000, 2000000,
+};
+
 static const struct regulator_ops qcom_usb_vbus_reg_ops = {
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
@@ -37,8 +42,8 @@ static struct regulator_desc qcom_usb_vbus_rdesc = {
 	.ops = &qcom_usb_vbus_reg_ops,
 	.owner = THIS_MODULE,
 	.type = REGULATOR_VOLTAGE,
-	.curr_table = curr_table,
-	.n_current_limits = ARRAY_SIZE(curr_table),
+	.curr_table = NULL,
+	.n_current_limits = 0,
 };
 
 static int qcom_usb_vbus_regulator_probe(struct platform_device *pdev)
@@ -50,6 +55,7 @@ static int qcom_usb_vbus_regulator_probe(struct platform_device *pdev)
 	struct regulator_init_data *init_data;
 	int ret;
 	u32 base;
+	const char *pmic_type;
 
 	ret = of_property_read_u32(dev->of_node, "reg", &base);
 	if (ret < 0) {
@@ -68,6 +74,19 @@ static int qcom_usb_vbus_regulator_probe(struct platform_device *pdev)
 	if (!init_data)
 		return -ENOMEM;
 
+	// Determine PMIC type
+	pmic_type = of_device_get_match_data(dev);
+	if (pmic_type && strcmp(pmic_type, "pmi8998") == 0) {
+		qcom_usb_vbus_rdesc.curr_table = curr_table_pmi8998;
+		qcom_usb_vbus_rdesc.n_current_limits =
+			ARRAY_SIZE(curr_table_pmi8998);
+	} else if (pmic_type && strcmp(pmic_type, "pm8150b") == 0) {
+		qcom_usb_vbus_rdesc.curr_table = curr_table_pm8150b;
+		qcom_usb_vbus_rdesc.n_current_limits =
+			ARRAY_SIZE(curr_table_pm8150b);
+	} else {
+		return -ENODEV;
+	}
 	qcom_usb_vbus_rdesc.enable_reg = base + CMD_OTG;
 	qcom_usb_vbus_rdesc.enable_mask = OTG_EN;
 	qcom_usb_vbus_rdesc.csel_reg = base + OTG_CURRENT_LIMIT_CFG;
@@ -80,18 +99,22 @@ static int qcom_usb_vbus_regulator_probe(struct platform_device *pdev)
 	rdev = devm_regulator_register(dev, &qcom_usb_vbus_rdesc, &config);
 	if (IS_ERR(rdev)) {
 		ret = PTR_ERR(rdev);
-		dev_err(dev, "not able to register vbus reg %d\n", ret);
+		dev_err(dev, "Failed to register vbus reg %d\n", ret);
 		return ret;
 	}
 
 	/* Disable HW logic for VBUS enable */
 	regmap_update_bits(regmap, base + OTG_CFG, OTG_EN_SRC_CFG, 0);
 
+	dev_info(dev, "Registered QCOM %s VBUS regulator\n",
+		 pmic_type);
+
 	return 0;
 }
 
 static const struct of_device_id qcom_usb_vbus_regulator_match[] = {
-	{ .compatible = "qcom,pm8150b-vbus-reg" },
+	{ .compatible = "qcom,pm8150b-vbus-reg", .data = "pm8150b" },
+	{ .compatible = "qcom,pmi8998-vbus-reg", .data = "pmi8998" },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, qcom_usb_vbus_regulator_match);
-- 
2.43.0

Re: [PATCH 2/3] regulator: qcom_usb_vbus: Add support for PMI8998 VBUS

[Mark Brown]

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

On Tue, Feb 11, 2025 at 11:49:15AM -0800, James A. MacInnes wrote:

> -	.curr_table = curr_table,
> -	.n_current_limits = ARRAY_SIZE(curr_table),
> +	.curr_table = NULL,
> +	.n_current_limits = 0,

Things that are not initialised in static variables are implicitly 0.

> +	// Determine PMIC type
> +	pmic_type = of_device_get_match_data(dev);
> +	if (pmic_type && strcmp(pmic_type, "pmi8998") == 0) {
> +		qcom_usb_vbus_rdesc.curr_table = curr_table_pmi8998;
> +		qcom_usb_vbus_rdesc.n_current_limits =
> +			ARRAY_SIZE(curr_table_pmi8998);
> +	} else if (pmic_type && strcmp(pmic_type, "pm8150b") == 0) {
> +		qcom_usb_vbus_rdesc.curr_table = curr_table_pm8150b;
> +		qcom_usb_vbus_rdesc.n_current_limits =
> +			ARRAY_SIZE(curr_table_pm8150b);
> +	} else {
> +		return -ENODEV;
> +	}

Instead of modifying the static variable (which you had to remove const
from...) the driver should take a copy of it and modify that, that way
nothing goes wrong if we get two of the devices or anything.

>  static const struct of_device_id qcom_usb_vbus_regulator_match[] = {
> -	{ .compatible = "qcom,pm8150b-vbus-reg" },
> +	{ .compatible = "qcom,pm8150b-vbus-reg", .data = "pm8150b" },
> +	{ .compatible = "qcom,pmi8998-vbus-reg", .data = "pmi8998" },

The driver data should be a pointer to some quirk data for the device,
if you want a string to identify the device that should be a member of
the quirk data struct.  This will make the use of the data safer and
more idiomatic.

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

Re: [PATCH 2/3] regulator: qcom_usb_vbus: Add support for PMI8998 VBUS

[Dmitry Baryshkov]

On Tue, Feb 11, 2025 at 11:49:15AM -0800, James A. MacInnes wrote:
> This patch extends the Qualcomm USB VBUS regulator driver to support
> PMI8998 PMIC alongside the existing support for PM8150B.
> 
> Key changes:
> - Added current limit tables specific to PMI8998.
> - Dynamically configure the VBUS regulator based on the PMIC type.
> - Updated debug messages to reflect successful initialization for
>   supported PMICs.
> - Changed registration log message
> 
> These changes ensure proper VBUS current limit configuration and
> compatibility across multiple Qualcomm PMICs.
> 
> Signed-off-by: James A. MacInnes <james.a.macinnes@gmail.com>
> ---
>  drivers/regulator/qcom_usb_vbus-regulator.c | 33 +++++++++++++++++----
>  1 file changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/regulator/qcom_usb_vbus-regulator.c b/drivers/regulator/qcom_usb_vbus-regulator.c
> index cd94ed67621f..bfcb77698ba2 100644
> --- a/drivers/regulator/qcom_usb_vbus-regulator.c
> +++ b/drivers/regulator/qcom_usb_vbus-regulator.c
> @@ -20,10 +20,15 @@
>  #define OTG_CFG				0x53
>  #define OTG_EN_SRC_CFG			BIT(1)
>  
> -static const unsigned int curr_table[] = {
> +static const unsigned int curr_table_pm8150b[] = {
>  	500000, 1000000, 1500000, 2000000, 2500000, 3000000,
>  };
>  
> +static const unsigned int curr_table_pmi8998[] = {
> +	250000, 500000, 750000, 1000000,
> +	1250000, 1500000, 1750000, 2000000,
> +};
> +
>  static const struct regulator_ops qcom_usb_vbus_reg_ops = {
>  	.enable = regulator_enable_regmap,
>  	.disable = regulator_disable_regmap,
> @@ -37,8 +42,8 @@ static struct regulator_desc qcom_usb_vbus_rdesc = {
>  	.ops = &qcom_usb_vbus_reg_ops,
>  	.owner = THIS_MODULE,
>  	.type = REGULATOR_VOLTAGE,
> -	.curr_table = curr_table,
> -	.n_current_limits = ARRAY_SIZE(curr_table),
> +	.curr_table = NULL,
> +	.n_current_limits = 0,
>  };
>  
>  static int qcom_usb_vbus_regulator_probe(struct platform_device *pdev)
> @@ -50,6 +55,7 @@ static int qcom_usb_vbus_regulator_probe(struct platform_device *pdev)
>  	struct regulator_init_data *init_data;
>  	int ret;
>  	u32 base;
> +	const char *pmic_type;
>  
>  	ret = of_property_read_u32(dev->of_node, "reg", &base);
>  	if (ret < 0) {
> @@ -68,6 +74,19 @@ static int qcom_usb_vbus_regulator_probe(struct platform_device *pdev)
>  	if (!init_data)
>  		return -ENOMEM;
>  
> +	// Determine PMIC type
> +	pmic_type = of_device_get_match_data(dev);
> +	if (pmic_type && strcmp(pmic_type, "pmi8998") == 0) {

I think a traditional way is to define an enum and then use that enum
values as match data. Or you can just add a struct with curr_table and
get that as a match data.

> +		qcom_usb_vbus_rdesc.curr_table = curr_table_pmi8998;
> +		qcom_usb_vbus_rdesc.n_current_limits =
> +			ARRAY_SIZE(curr_table_pmi8998);
> +	} else if (pmic_type && strcmp(pmic_type, "pm8150b") == 0) {
> +		qcom_usb_vbus_rdesc.curr_table = curr_table_pm8150b;
> +		qcom_usb_vbus_rdesc.n_current_limits =
> +			ARRAY_SIZE(curr_table_pm8150b);
> +	} else {
> +		return -ENODEV;
> +	}
>  	qcom_usb_vbus_rdesc.enable_reg = base + CMD_OTG;
>  	qcom_usb_vbus_rdesc.enable_mask = OTG_EN;
>  	qcom_usb_vbus_rdesc.csel_reg = base + OTG_CURRENT_LIMIT_CFG;
> @@ -80,18 +99,22 @@ static int qcom_usb_vbus_regulator_probe(struct platform_device *pdev)
>  	rdev = devm_regulator_register(dev, &qcom_usb_vbus_rdesc, &config);
>  	if (IS_ERR(rdev)) {
>  		ret = PTR_ERR(rdev);
> -		dev_err(dev, "not able to register vbus reg %d\n", ret);
> +		dev_err(dev, "Failed to register vbus reg %d\n", ret);
>  		return ret;
>  	}
>  
>  	/* Disable HW logic for VBUS enable */
>  	regmap_update_bits(regmap, base + OTG_CFG, OTG_EN_SRC_CFG, 0);
>  
> +	dev_info(dev, "Registered QCOM %s VBUS regulator\n",
> +		 pmic_type);

dev_dbg, the driver should be silent by default.

> +
>  	return 0;
>  }
>  
>  static const struct of_device_id qcom_usb_vbus_regulator_match[] = {
> -	{ .compatible = "qcom,pm8150b-vbus-reg" },
> +	{ .compatible = "qcom,pm8150b-vbus-reg", .data = "pm8150b" },
> +	{ .compatible = "qcom,pmi8998-vbus-reg", .data = "pmi8998" },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, qcom_usb_vbus_regulator_match);
> -- 
> 2.43.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH 2/3] regulator: qcom_usb_vbus: Add support for PMI8998 VBUS

[Bryan O'Donoghue]

On 11/02/2025 19:49, James A. MacInnes wrote:
> +	// Determine PMIC type
> +	pmic_type = of_device_get_match_data(dev);
> +	if (pmic_type && strcmp(pmic_type, "pmi8998") == 0) {
> +		qcom_usb_vbus_rdesc.curr_table = curr_table_pmi8998;
> +		qcom_usb_vbus_rdesc.n_current_limits =
> +			ARRAY_SIZE(curr_table_pmi8998);
> +	} else if (pmic_type && strcmp(pmic_type, "pm8150b") == 0) {
> +		qcom_usb_vbus_rdesc.curr_table = curr_table_pm8150b;
> +		qcom_usb_vbus_rdesc.n_current_limits =
> +			ARRAY_SIZE(curr_table_pm8150b);
> +	} else {
> +		return -ENODEV;
> +	}
>   	qcom_usb_vbus_rdesc.enable_reg = base + CMD_OTG;
>   	qcom_usb_vbus_rdesc.enable_mask = OTG_EN;
>   	qcom_usb_vbus_rdesc.csel_reg = base + OTG_CURRENT_LIMIT_CFG;
> @@ -80,18 +99,22 @@ static int qcom_usb_vbus_regulator_probe(struct platform_device *pdev)
>   	rdev = devm_regulator_register(dev, &qcom_usb_vbus_rdesc, &config);
>   	if (IS_ERR(rdev)) {
>   		ret = PTR_ERR(rdev);
> -		dev_err(dev, "not able to register vbus reg %d\n", ret);
> +		dev_err(dev, "Failed to register vbus reg %d\n", ret);
>   		return ret;
>   	}
>   
>   	/* Disable HW logic for VBUS enable */
>   	regmap_update_bits(regmap, base + OTG_CFG, OTG_EN_SRC_CFG, 0);
>   
> +	dev_info(dev, "Registered QCOM %s VBUS regulator\n",
> +		 pmic_type);
> +
>   	return 0;
>   }
>   
>   static const struct of_device_id qcom_usb_vbus_regulator_match[] = {
> -	{ .compatible = "qcom,pm8150b-vbus-reg" },
> +	{ .compatible = "qcom,pm8150b-vbus-reg", .data = "pm8150b" },
> +	{ .compatible = "qcom,pmi8998-vbus-reg", .data = "pmi8998" },

I think the other two said much the same thing but .data should point to 
the differentiator instead of being a string which you disjoin on and 
then hook your differentiated data.

i.e.

.data = &my_driver_specific_static_data_here.

---
bod

Re: [PATCH 2/3] regulator: qcom_usb_vbus: Add support for PMI8998 VBUS

[Mark Brown]

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

On Wed, Feb 12, 2025 at 01:16:37AM +0200, Dmitry Baryshkov wrote:
> On Tue, Feb 11, 2025 at 11:49:15AM -0800, James A. MacInnes wrote:

> > +	pmic_type = of_device_get_match_data(dev);
> > +	if (pmic_type && strcmp(pmic_type, "pmi8998") == 0) {

> I think a traditional way is to define an enum and then use that enum
> values as match data. Or you can just add a struct with curr_table and
> get that as a match data.

Either approach works, yeah.

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

[PATCH 0/3] Add PMI8998 VBUS Regulator Support v2

[James A. MacInnes]

Greetings,

Thank you all for your feedback. I have integrated your recommendations
into this revised patch series (v2); please disregard the previous thread.

Summary of Changes:
- Patch 1/3: Updates the Device Tree Schema bindings to include
  "qcom,pmi8998-vbus-reg" for PMI8998 support.
- Patch 2/3: Extends the Qualcomm USB VBUS regulator driver to support
  PMI8998, dynamically configuring the regulator based on the PMIC type.
- Patch 3/3: Adds the VBUS regulator node to pmi8998.dtsi, enabling
  USB Type-C VBUS support.

Motivation:
To enable VBUS operation on the SDM845 platform PMI8998 PMIC.

Kernel Version & Testing:
- These patches were developed and tested on Linux 6.13.
- Attempting to run Linux 6.14-rc2 on our Lantronix SOM resulted in a
  hard crash, making it unsuitable for validation.
- Validation was performed using a modified device tree, confirming proper
  regulator configuration.
- No regressions were observed on existing PMIC configurations.

Next Steps:
If there are any suggestions or required changes, please let me know.
I will be happy to revise and address any concerns.

Thanks again,
James A. MacInnes
james.a.macinnes@gmail.com


James A. MacInnes (3):
  regulator: qcom_usb_vbus: Update DTS binding for PMI8998 support
  regulator: qcom_usb_vbus: Add support for PMI8998 VBUS
  arm64: boot: dts: pmi8998.dtsi: Add VBUS regulator node

 .../regulator/qcom,usb-vbus-regulator.yaml    |  1 +
 arch/arm64/boot/dts/qcom/pmi8998.dtsi         |  6 +++
 drivers/regulator/qcom_usb_vbus-regulator.c   | 38 ++++++++++++++++---
 3 files changed, 40 insertions(+), 5 deletions(-)

-- 
2.43.0

[PATCH 1/3] regulator: qcom_usb_vbus: Update DTS binding for PMI8998 support

[James A. MacInnes]

Update the binding in
Documentation/devicetree/bindings/regulator/qcom,usb-vbus-regulator.yaml
to include the compatible string "qcom,pmi8998-vbus-reg".
This change adds PMI8998 support to the Qualcomm USB VBUS regulator
bindings.

With this patch, device trees that describe a PMI8998 USB VBUS regulator
using the "qcom,pmi8998-vbus-reg" compatible string will now validate
correctly. This is required to support hardware based on PMI8998, which
has different current limit tables and other regulator-specific
configurations.

Tested: The next patch contains the driver and has been tested with
Lantronix SOM.

Signed-off-by: James A. MacInnes <james.a.macinnes@gmail.com>
---
 .../devicetree/bindings/regulator/qcom,usb-vbus-regulator.yaml   | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/regulator/qcom,usb-vbus-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom,usb-vbus-regulator.yaml
index fcefc722ee2a..6092560e9048 100644
--- a/Documentation/devicetree/bindings/regulator/qcom,usb-vbus-regulator.yaml
+++ b/Documentation/devicetree/bindings/regulator/qcom,usb-vbus-regulator.yaml
@@ -22,6 +22,7 @@ properties:
     oneOf:
       - enum:
           - qcom,pm8150b-vbus-reg
+          - qcom,pmi8998-vbus-reg
       - items:
           - enum:
               - qcom,pm4125-vbus-reg
-- 
2.43.0

[PATCH 2/3] regulator: qcom_usb_vbus: Add support for PMI8998 VBUS

[James A. MacInnes]

This patch extends the Qualcomm USB VBUS regulator driver to support
PMI8998 PMIC alongside the existing support for PM8150B.

Key changes:
- Added current limit tables specific to PMI8998.
- Dynamically configure the VBUS regulator based on the PMIC type.
- Updated debug messages to reflect successful initialization for
  supported PMICs.
- Changed registration log message

These changes ensure proper VBUS current limit configuration and
compatibility across multiple Qualcomm PMICs.

Signed-off-by: James A. MacInnes <james.a.macinnes@gmail.com>
---
 drivers/regulator/qcom_usb_vbus-regulator.c | 38 ++++++++++++++++++---
 1 file changed, 33 insertions(+), 5 deletions(-)

diff --git a/drivers/regulator/qcom_usb_vbus-regulator.c b/drivers/regulator/qcom_usb_vbus-regulator.c
index cd94ed67621f..804dd1a9e057 100644
--- a/drivers/regulator/qcom_usb_vbus-regulator.c
+++ b/drivers/regulator/qcom_usb_vbus-regulator.c
@@ -20,10 +20,30 @@
 #define OTG_CFG				0x53
 #define OTG_EN_SRC_CFG			BIT(1)
 
-static const unsigned int curr_table[] = {
+struct msm_vbus_desc {
+	const unsigned int *curr_table;
+	unsigned int n_current_limits;
+};
+
+static const unsigned int curr_table_pm8150b[] = {
 	500000, 1000000, 1500000, 2000000, 2500000, 3000000,
 };
 
+static const unsigned int curr_table_pmi8998[] = {
+	250000, 500000, 750000, 1000000,
+	1250000, 1500000, 1750000, 2000000,
+};
+
+static const struct msm_vbus_desc msm_vbus_desc_pm8150b = {
+	.curr_table = curr_table_pm8150b,
+	.n_current_limits = ARRAY_SIZE(curr_table_pm8150b),
+};
+
+static const struct msm_vbus_desc msm_vbus_desc_pmi8998 = {
+	.curr_table = curr_table_pmi8998,
+	.n_current_limits = ARRAY_SIZE(curr_table_pmi8998),
+};
+
 static const struct regulator_ops qcom_usb_vbus_reg_ops = {
 	.enable = regulator_enable_regmap,
 	.disable = regulator_disable_regmap,
@@ -37,8 +57,6 @@ static struct regulator_desc qcom_usb_vbus_rdesc = {
 	.ops = &qcom_usb_vbus_reg_ops,
 	.owner = THIS_MODULE,
 	.type = REGULATOR_VOLTAGE,
-	.curr_table = curr_table,
-	.n_current_limits = ARRAY_SIZE(curr_table),
 };
 
 static int qcom_usb_vbus_regulator_probe(struct platform_device *pdev)
@@ -48,6 +66,7 @@ static int qcom_usb_vbus_regulator_probe(struct platform_device *pdev)
 	struct regmap *regmap;
 	struct regulator_config config = { };
 	struct regulator_init_data *init_data;
+	const struct msm_vbus_desc *quirks;
 	int ret;
 	u32 base;
 
@@ -68,6 +87,12 @@ static int qcom_usb_vbus_regulator_probe(struct platform_device *pdev)
 	if (!init_data)
 		return -ENOMEM;
 
+	quirks = of_device_get_match_data(dev);
+	if (!quirks)
+		return -ENODEV;
+
+	qcom_usb_vbus_rdesc.curr_table = quirks->curr_table;
+	qcom_usb_vbus_rdesc.n_current_limits = quirks->n_current_limits;
 	qcom_usb_vbus_rdesc.enable_reg = base + CMD_OTG;
 	qcom_usb_vbus_rdesc.enable_mask = OTG_EN;
 	qcom_usb_vbus_rdesc.csel_reg = base + OTG_CURRENT_LIMIT_CFG;
@@ -80,18 +105,21 @@ static int qcom_usb_vbus_regulator_probe(struct platform_device *pdev)
 	rdev = devm_regulator_register(dev, &qcom_usb_vbus_rdesc, &config);
 	if (IS_ERR(rdev)) {
 		ret = PTR_ERR(rdev);
-		dev_err(dev, "not able to register vbus reg %d\n", ret);
+		dev_err(dev, "Failed to register vbus reg %d\n", ret);
 		return ret;
 	}
 
 	/* Disable HW logic for VBUS enable */
 	regmap_update_bits(regmap, base + OTG_CFG, OTG_EN_SRC_CFG, 0);
 
+	dev_dbg(dev, "Registered QCOM VBUS regulator\n");
+
 	return 0;
 }
 
 static const struct of_device_id qcom_usb_vbus_regulator_match[] = {
-	{ .compatible = "qcom,pm8150b-vbus-reg" },
+	{ .compatible = "qcom,pm8150b-vbus-reg", .data = &msm_vbus_desc_pm8150b },
+	{ .compatible = "qcom,pmi8998-vbus-reg", .data = &msm_vbus_desc_pmi8998 },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, qcom_usb_vbus_regulator_match);
-- 
2.43.0

[PATCH 3/3] arm64: boot: dts: pmi8998.dtsi: Add VBUS regulator node

[James A. MacInnes]

In order to enable USB Type-C VBUS support on the SDM845 platform add
device node for the USB Vbus regulator to the PMI8998 PMIC device tree.

Signed-off-by: James A. MacInnes <james.a.macinnes@gmail.com>
---
 arch/arm64/boot/dts/qcom/pmi8998.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/pmi8998.dtsi b/arch/arm64/boot/dts/qcom/pmi8998.dtsi
index cd3f0790fd42..8cb1d851b5a3 100644
--- a/arch/arm64/boot/dts/qcom/pmi8998.dtsi
+++ b/arch/arm64/boot/dts/qcom/pmi8998.dtsi
@@ -29,6 +29,12 @@ pmi8998_charger: charger@1000 {
 			status = "disabled";
 		};
 
+		pmi8998_vbus: usb-vbus-regulator@1100 {
+			compatible = "qcom,pmi8998-vbus-reg";
+			status = "disabled";
+			reg = <0x1100>;
+		};
+
 		pmi8998_gpios: gpio@c000 {
 			compatible = "qcom,pmi8998-gpio", "qcom,spmi-gpio";
 			reg = <0xc000>;
-- 
2.43.0

Re: [PATCH 1/3] regulator: qcom_usb_vbus: Update DTS binding for PMI8998 support

[Dmitry Baryshkov]

On Tue, Feb 11, 2025 at 05:07:42PM -0800, James A. MacInnes wrote:
> Update the binding in
> Documentation/devicetree/bindings/regulator/qcom,usb-vbus-regulator.yaml
> to include the compatible string "qcom,pmi8998-vbus-reg".
> This change adds PMI8998 support to the Qualcomm USB VBUS regulator
> bindings.

The second phrase is obvious.

> 
> With this patch, device trees that describe a PMI8998 USB VBUS regulator
> using the "qcom,pmi8998-vbus-reg" compatible string will now validate
> correctly. This is required to support hardware based on PMI8998, which
> has different current limit tables and other regulator-specific
> configurations.

All of this is also obvious

> 
> Tested: The next patch contains the driver and has been tested with
> Lantronix SOM.

Drop

> 
> Signed-off-by: James A. MacInnes <james.a.macinnes@gmail.com>
> ---
>  .../devicetree/bindings/regulator/qcom,usb-vbus-regulator.yaml   | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/regulator/qcom,usb-vbus-regulator.yaml b/Documentation/devicetree/bindings/regulator/qcom,usb-vbus-regulator.yaml
> index fcefc722ee2a..6092560e9048 100644
> --- a/Documentation/devicetree/bindings/regulator/qcom,usb-vbus-regulator.yaml
> +++ b/Documentation/devicetree/bindings/regulator/qcom,usb-vbus-regulator.yaml
> @@ -22,6 +22,7 @@ properties:
>      oneOf:
>        - enum:
>            - qcom,pm8150b-vbus-reg
> +          - qcom,pmi8998-vbus-reg
>        - items:
>            - enum:
>                - qcom,pm4125-vbus-reg
> -- 
> 2.43.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH 2/3] regulator: qcom_usb_vbus: Add support for PMI8998 VBUS

[Dmitry Baryshkov]

On Tue, Feb 11, 2025 at 05:07:43PM -0800, James A. MacInnes wrote:
> This patch extends the Qualcomm USB VBUS regulator driver to support
> PMI8998 PMIC alongside the existing support for PM8150B.

Please modify this commit message according to follow the example I
provided for the patch 3. If you are unsure `git log drivers/regulator`
will provide you with good enough examples.

> 
> Key changes:
> - Added current limit tables specific to PMI8998.
> - Dynamically configure the VBUS regulator based on the PMIC type.
> - Updated debug messages to reflect successful initialization for
>   supported PMICs.
> - Changed registration log message
> 
> These changes ensure proper VBUS current limit configuration and
> compatibility across multiple Qualcomm PMICs.
> 
> Signed-off-by: James A. MacInnes <james.a.macinnes@gmail.com>
> ---
>  drivers/regulator/qcom_usb_vbus-regulator.c | 38 ++++++++++++++++++---
>  1 file changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/regulator/qcom_usb_vbus-regulator.c b/drivers/regulator/qcom_usb_vbus-regulator.c
> index cd94ed67621f..804dd1a9e057 100644
> --- a/drivers/regulator/qcom_usb_vbus-regulator.c
> +++ b/drivers/regulator/qcom_usb_vbus-regulator.c
> @@ -20,10 +20,30 @@
>  #define OTG_CFG				0x53
>  #define OTG_EN_SRC_CFG			BIT(1)
>  
> -static const unsigned int curr_table[] = {
> +struct msm_vbus_desc {
> +	const unsigned int *curr_table;
> +	unsigned int n_current_limits;
> +};
> +
> +static const unsigned int curr_table_pm8150b[] = {
>  	500000, 1000000, 1500000, 2000000, 2500000, 3000000,
>  };
>  
> +static const unsigned int curr_table_pmi8998[] = {
> +	250000, 500000, 750000, 1000000,
> +	1250000, 1500000, 1750000, 2000000,
> +};
> +
> +static const struct msm_vbus_desc msm_vbus_desc_pm8150b = {
> +	.curr_table = curr_table_pm8150b,
> +	.n_current_limits = ARRAY_SIZE(curr_table_pm8150b),
> +};
> +
> +static const struct msm_vbus_desc msm_vbus_desc_pmi8998 = {
> +	.curr_table = curr_table_pmi8998,
> +	.n_current_limits = ARRAY_SIZE(curr_table_pmi8998),
> +};
> +
>  static const struct regulator_ops qcom_usb_vbus_reg_ops = {
>  	.enable = regulator_enable_regmap,
>  	.disable = regulator_disable_regmap,
> @@ -37,8 +57,6 @@ static struct regulator_desc qcom_usb_vbus_rdesc = {
>  	.ops = &qcom_usb_vbus_reg_ops,
>  	.owner = THIS_MODULE,
>  	.type = REGULATOR_VOLTAGE,
> -	.curr_table = curr_table,
> -	.n_current_limits = ARRAY_SIZE(curr_table),
>  };
>  
>  static int qcom_usb_vbus_regulator_probe(struct platform_device *pdev)
> @@ -48,6 +66,7 @@ static int qcom_usb_vbus_regulator_probe(struct platform_device *pdev)
>  	struct regmap *regmap;
>  	struct regulator_config config = { };
>  	struct regulator_init_data *init_data;
> +	const struct msm_vbus_desc *quirks;
>  	int ret;
>  	u32 base;
>  
> @@ -68,6 +87,12 @@ static int qcom_usb_vbus_regulator_probe(struct platform_device *pdev)
>  	if (!init_data)
>  		return -ENOMEM;
>  
> +	quirks = of_device_get_match_data(dev);
> +	if (!quirks)
> +		return -ENODEV;
> +
> +	qcom_usb_vbus_rdesc.curr_table = quirks->curr_table;
> +	qcom_usb_vbus_rdesc.n_current_limits = quirks->n_current_limits;
>  	qcom_usb_vbus_rdesc.enable_reg = base + CMD_OTG;
>  	qcom_usb_vbus_rdesc.enable_mask = OTG_EN;
>  	qcom_usb_vbus_rdesc.csel_reg = base + OTG_CURRENT_LIMIT_CFG;
> @@ -80,18 +105,21 @@ static int qcom_usb_vbus_regulator_probe(struct platform_device *pdev)
>  	rdev = devm_regulator_register(dev, &qcom_usb_vbus_rdesc, &config);
>  	if (IS_ERR(rdev)) {
>  		ret = PTR_ERR(rdev);
> -		dev_err(dev, "not able to register vbus reg %d\n", ret);
> +		dev_err(dev, "Failed to register vbus reg %d\n", ret);
>  		return ret;
>  	}
>  
>  	/* Disable HW logic for VBUS enable */
>  	regmap_update_bits(regmap, base + OTG_CFG, OTG_EN_SRC_CFG, 0);
>  
> +	dev_dbg(dev, "Registered QCOM VBUS regulator\n");
> +
>  	return 0;
>  }
>  
>  static const struct of_device_id qcom_usb_vbus_regulator_match[] = {
> -	{ .compatible = "qcom,pm8150b-vbus-reg" },
> +	{ .compatible = "qcom,pm8150b-vbus-reg", .data = &msm_vbus_desc_pm8150b },
> +	{ .compatible = "qcom,pmi8998-vbus-reg", .data = &msm_vbus_desc_pmi8998 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, qcom_usb_vbus_regulator_match);
> -- 
> 2.43.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH 0/3] Add PMI8998 VBUS Regulator Support v2

[Konrad Dybcio]

On 12.02.2025 2:07 AM, James A. MacInnes wrote:
> Greetings,
> 
> Thank you all for your feedback. I have integrated your recommendations
> into this revised patch series (v2); please disregard the previous thread.
> 
> Summary of Changes:
> - Patch 1/3: Updates the Device Tree Schema bindings to include
>   "qcom,pmi8998-vbus-reg" for PMI8998 support.
> - Patch 2/3: Extends the Qualcomm USB VBUS regulator driver to support
>   PMI8998, dynamically configuring the regulator based on the PMIC type.
> - Patch 3/3: Adds the VBUS regulator node to pmi8998.dtsi, enabling
>   USB Type-C VBUS support.
> 
> Motivation:
> To enable VBUS operation on the SDM845 platform PMI8998 PMIC.
> 
> Kernel Version & Testing:
> - These patches were developed and tested on Linux 6.13.
> - Attempting to run Linux 6.14-rc2 on our Lantronix SOM resulted in a
>   hard crash, making it unsuitable for validation.

Please try reverting 57a7138d0627309d469719f1845d2778c251f358

Konrad

Re: [PATCH 2/3] regulator: qcom_usb_vbus: Add support for PMI8998 VBUS

[Konrad Dybcio]

On 12.02.2025 2:07 AM, James A. MacInnes wrote:
> This patch extends the Qualcomm USB VBUS regulator driver to support
> PMI8998 PMIC alongside the existing support for PM8150B.
> 
> Key changes:
> - Added current limit tables specific to PMI8998.
> - Dynamically configure the VBUS regulator based on the PMIC type.
> - Updated debug messages to reflect successful initialization for
>   supported PMICs.
> - Changed registration log message
> 
> These changes ensure proper VBUS current limit configuration and
> compatibility across multiple Qualcomm PMICs.
> 
> Signed-off-by: James A. MacInnes <james.a.macinnes@gmail.com>
> ---
>  drivers/regulator/qcom_usb_vbus-regulator.c | 38 ++++++++++++++++++---
>  1 file changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/regulator/qcom_usb_vbus-regulator.c b/drivers/regulator/qcom_usb_vbus-regulator.c
> index cd94ed67621f..804dd1a9e057 100644
> --- a/drivers/regulator/qcom_usb_vbus-regulator.c
> +++ b/drivers/regulator/qcom_usb_vbus-regulator.c
> @@ -20,10 +20,30 @@
>  #define OTG_CFG				0x53
>  #define OTG_EN_SRC_CFG			BIT(1)
>  
> -static const unsigned int curr_table[] = {
> +struct msm_vbus_desc {
> +	const unsigned int *curr_table;
> +	unsigned int n_current_limits;
> +};
> +
> +static const unsigned int curr_table_pm8150b[] = {
>  	500000, 1000000, 1500000, 2000000, 2500000, 3000000,
>  };
>  
> +static const unsigned int curr_table_pmi8998[] = {
> +	250000, 500000, 750000, 1000000,
> +	1250000, 1500000, 1750000, 2000000,
> +};

To the best of my understanding these numbers are correct

> +
> +static const struct msm_vbus_desc msm_vbus_desc_pm8150b = {
> +	.curr_table = curr_table_pm8150b,
> +	.n_current_limits = ARRAY_SIZE(curr_table_pm8150b),
> +};
> +
> +static const struct msm_vbus_desc msm_vbus_desc_pmi8998 = {
> +	.curr_table = curr_table_pmi8998,
> +	.n_current_limits = ARRAY_SIZE(curr_table_pmi8998),
> +};
> +
>  static const struct regulator_ops qcom_usb_vbus_reg_ops = {
>  	.enable = regulator_enable_regmap,
>  	.disable = regulator_disable_regmap,
> @@ -37,8 +57,6 @@ static struct regulator_desc qcom_usb_vbus_rdesc = {
>  	.ops = &qcom_usb_vbus_reg_ops,
>  	.owner = THIS_MODULE,
>  	.type = REGULATOR_VOLTAGE,
> -	.curr_table = curr_table,
> -	.n_current_limits = ARRAY_SIZE(curr_table),
>  };
>  
>  static int qcom_usb_vbus_regulator_probe(struct platform_device *pdev)
> @@ -48,6 +66,7 @@ static int qcom_usb_vbus_regulator_probe(struct platform_device *pdev)
>  	struct regmap *regmap;
>  	struct regulator_config config = { };
>  	struct regulator_init_data *init_data;
> +	const struct msm_vbus_desc *quirks;

'quirks' is one way to put it ;) I'd call it 'desc' or 'data' but it's
totally a potayto/potahto discussion

>  	int ret;
>  	u32 base;
>  
> @@ -68,6 +87,12 @@ static int qcom_usb_vbus_regulator_probe(struct platform_device *pdev)
>  	if (!init_data)
>  		return -ENOMEM;
>  
> +	quirks = of_device_get_match_data(dev);
> +	if (!quirks)
> +		return -ENODEV;
> +
> +	qcom_usb_vbus_rdesc.curr_table = quirks->curr_table;
> +	qcom_usb_vbus_rdesc.n_current_limits = quirks->n_current_limits;
>  	qcom_usb_vbus_rdesc.enable_reg = base + CMD_OTG;
>  	qcom_usb_vbus_rdesc.enable_mask = OTG_EN;
>  	qcom_usb_vbus_rdesc.csel_reg = base + OTG_CURRENT_LIMIT_CFG;
> @@ -80,18 +105,21 @@ static int qcom_usb_vbus_regulator_probe(struct platform_device *pdev)
>  	rdev = devm_regulator_register(dev, &qcom_usb_vbus_rdesc, &config);
>  	if (IS_ERR(rdev)) {
>  		ret = PTR_ERR(rdev);
> -		dev_err(dev, "not able to register vbus reg %d\n", ret);
> +		dev_err(dev, "Failed to register vbus reg %d\n", ret);
>  		return ret;
>  	}
>  
>  	/* Disable HW logic for VBUS enable */
>  	regmap_update_bits(regmap, base + OTG_CFG, OTG_EN_SRC_CFG, 0);
>  
> +	dev_dbg(dev, "Registered QCOM VBUS regulator\n");

Not sure how useful this is given the previous call creates a sysfs entry
on success, but sure

Konrad

Re: [PATCH 2/3] regulator: qcom_usb_vbus: Add support for PMI8998 VBUS

[Caleb Connolly]

Hi James,

On 2/12/25 01:07, James A. MacInnes wrote:
> This patch extends the Qualcomm USB VBUS regulator driver to support
> PMI8998 PMIC alongside the existing support for PM8150B.

Thanks for the patch!
> 
> Key changes:
> - Added current limit tables specific to PMI8998.

I also played around with vbus on PMI8998 before for type-c support 
(unfortunately didn't make it's way to the lists yet...) and I needed 
some additional changes for this to work correctly. I found that it was 
possible for the overcurrent protection to be hit if the type-c port 
manager allowed the peripheral to pull current too early, and it's 
necessary to allow 2.5ms enable time.

PM8150b doesn't have these limitations (and supports the instant power 
role switch feature that's part of the type-c PD spec, allowing the 
power role to be switched without either side losing power e.g. when you 
unplug the power supply from a dock), hence it's only necessary for PMI8998.

I would suggest implementing a proper .is_enabled op to poll the status 
register for OTG_STATE_ENABLED and configuring 
qcom_usb_vbus_rdesc.enable_time = 250000;

Kind regards,

> - Dynamically configure the VBUS regulator based on the PMIC type.
> - Updated debug messages to reflect successful initialization for
>    supported PMICs.
> - Changed registration log message
> 
> These changes ensure proper VBUS current limit configuration and
> compatibility across multiple Qualcomm PMICs.
> 
> Signed-off-by: James A. MacInnes <james.a.macinnes@gmail.com>
> ---
>   drivers/regulator/qcom_usb_vbus-regulator.c | 38 ++++++++++++++++++---
>   1 file changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/regulator/qcom_usb_vbus-regulator.c b/drivers/regulator/qcom_usb_vbus-regulator.c
> index cd94ed67621f..804dd1a9e057 100644
> --- a/drivers/regulator/qcom_usb_vbus-regulator.c
> +++ b/drivers/regulator/qcom_usb_vbus-regulator.c
> @@ -20,10 +20,30 @@
>   #define OTG_CFG				0x53
>   #define OTG_EN_SRC_CFG			BIT(1)
>   
> -static const unsigned int curr_table[] = {
> +struct msm_vbus_desc {
> +	const unsigned int *curr_table;
> +	unsigned int n_current_limits;
> +};
> +
> +static const unsigned int curr_table_pm8150b[] = {
>   	500000, 1000000, 1500000, 2000000, 2500000, 3000000,
>   };
>   
> +static const unsigned int curr_table_pmi8998[] = {
> +	250000, 500000, 750000, 1000000,
> +	1250000, 1500000, 1750000, 2000000,
> +};
> +
> +static const struct msm_vbus_desc msm_vbus_desc_pm8150b = {
> +	.curr_table = curr_table_pm8150b,
> +	.n_current_limits = ARRAY_SIZE(curr_table_pm8150b),
> +};
> +
> +static const struct msm_vbus_desc msm_vbus_desc_pmi8998 = {
> +	.curr_table = curr_table_pmi8998,
> +	.n_current_limits = ARRAY_SIZE(curr_table_pmi8998),
> +};
> +
>   static const struct regulator_ops qcom_usb_vbus_reg_ops = {
>   	.enable = regulator_enable_regmap,
>   	.disable = regulator_disable_regmap,
> @@ -37,8 +57,6 @@ static struct regulator_desc qcom_usb_vbus_rdesc = {
>   	.ops = &qcom_usb_vbus_reg_ops,
>   	.owner = THIS_MODULE,
>   	.type = REGULATOR_VOLTAGE,
> -	.curr_table = curr_table,
> -	.n_current_limits = ARRAY_SIZE(curr_table),
>   };
>   
>   static int qcom_usb_vbus_regulator_probe(struct platform_device *pdev)
> @@ -48,6 +66,7 @@ static int qcom_usb_vbus_regulator_probe(struct platform_device *pdev)
>   	struct regmap *regmap;
>   	struct regulator_config config = { };
>   	struct regulator_init_data *init_data;
> +	const struct msm_vbus_desc *quirks;
>   	int ret;
>   	u32 base;
>   
> @@ -68,6 +87,12 @@ static int qcom_usb_vbus_regulator_probe(struct platform_device *pdev)
>   	if (!init_data)
>   		return -ENOMEM;
>   
> +	quirks = of_device_get_match_data(dev);
> +	if (!quirks)
> +		return -ENODEV;
> +
> +	qcom_usb_vbus_rdesc.curr_table = quirks->curr_table;
> +	qcom_usb_vbus_rdesc.n_current_limits = quirks->n_current_limits;
>   	qcom_usb_vbus_rdesc.enable_reg = base + CMD_OTG;
>   	qcom_usb_vbus_rdesc.enable_mask = OTG_EN;
>   	qcom_usb_vbus_rdesc.csel_reg = base + OTG_CURRENT_LIMIT_CFG;
> @@ -80,18 +105,21 @@ static int qcom_usb_vbus_regulator_probe(struct platform_device *pdev)
>   	rdev = devm_regulator_register(dev, &qcom_usb_vbus_rdesc, &config);
>   	if (IS_ERR(rdev)) {
>   		ret = PTR_ERR(rdev);
> -		dev_err(dev, "not able to register vbus reg %d\n", ret);
> +		dev_err(dev, "Failed to register vbus reg %d\n", ret);
>   		return ret;
>   	}
>   
>   	/* Disable HW logic for VBUS enable */
>   	regmap_update_bits(regmap, base + OTG_CFG, OTG_EN_SRC_CFG, 0);
>   
> +	dev_dbg(dev, "Registered QCOM VBUS regulator\n");
> +
>   	return 0;
>   }
>   
>   static const struct of_device_id qcom_usb_vbus_regulator_match[] = {
> -	{ .compatible = "qcom,pm8150b-vbus-reg" },
> +	{ .compatible = "qcom,pm8150b-vbus-reg", .data = &msm_vbus_desc_pm8150b },
> +	{ .compatible = "qcom,pmi8998-vbus-reg", .data = &msm_vbus_desc_pmi8998 },
>   	{ }
>   };
>   MODULE_DEVICE_TABLE(of, qcom_usb_vbus_regulator_match);

-- 
Caleb (they/them)

Re: [PATCH 2/3] regulator: qcom_usb_vbus: Add support for PMI8998 VBUS

[Mark Brown]

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

On Wed, Feb 12, 2025 at 03:29:54PM +0000, Caleb Connolly wrote:

> I would suggest implementing a proper .is_enabled op to poll the status
> register for OTG_STATE_ENABLED and configuring

No, that would be buggy.  Implement a get_status() operation if the
device can report status.  is_enabled() should report what the driver
asked for.

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

Re: [PATCH 2/3] regulator: qcom_usb_vbus: Add support for PMI8998 VBUS

[Caleb Connolly]

On 2/12/25 15:37, Mark Brown wrote:
> On Wed, Feb 12, 2025 at 03:29:54PM +0000, Caleb Connolly wrote:
> 
>> I would suggest implementing a proper .is_enabled op to poll the status
>> register for OTG_STATE_ENABLED and configuring
> 
> No, that would be buggy.  Implement a get_status() operation if the
> device can report status.  is_enabled() should report what the driver
> asked for.

Ahh yep, that's right. it should implement .get_status() (as per the 
polling code in _regulator_do_enable()).

-- 
Caleb (they/them)

Re: [PATCH 2/3] regulator: qcom_usb_vbus: Add support for PMI8998 VBUS

[James A. MacInnes]

On Wed, 12 Feb 2025 13:55:59 +0100
Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> wrote:

> On 12.02.2025 2:07 AM, James A. MacInnes wrote:
> > This patch extends the Qualcomm USB VBUS regulator driver to support
> > PMI8998 PMIC alongside the existing support for PM8150B.
> > 
> > Key changes:
> > - Added current limit tables specific to PMI8998.
> > - Dynamically configure the VBUS regulator based on the PMIC type.
> > - Updated debug messages to reflect successful initialization for
> >   supported PMICs.
> > - Changed registration log message
> > 
> > These changes ensure proper VBUS current limit configuration and
> > compatibility across multiple Qualcomm PMICs.
> > 
> > Signed-off-by: James A. MacInnes <james.a.macinnes@gmail.com>
> > ---
> >  drivers/regulator/qcom_usb_vbus-regulator.c | 38
> > ++++++++++++++++++--- 1 file changed, 33 insertions(+), 5
> > deletions(-)
> > 
> > diff --git a/drivers/regulator/qcom_usb_vbus-regulator.c
> > b/drivers/regulator/qcom_usb_vbus-regulator.c index
> > cd94ed67621f..804dd1a9e057 100644 ---
> > a/drivers/regulator/qcom_usb_vbus-regulator.c +++
> > b/drivers/regulator/qcom_usb_vbus-regulator.c @@ -20,10 +20,30 @@
> >  #define OTG_CFG				0x53
> >  #define OTG_EN_SRC_CFG			BIT(1)
> >  
> > -static const unsigned int curr_table[] = {
> > +struct msm_vbus_desc {
> > +	const unsigned int *curr_table;
> > +	unsigned int n_current_limits;
> > +};
> > +
> > +static const unsigned int curr_table_pm8150b[] = {
> >  	500000, 1000000, 1500000, 2000000, 2500000, 3000000,
> >  };
> >  
> > +static const unsigned int curr_table_pmi8998[] = {
> > +	250000, 500000, 750000, 1000000,
> > +	1250000, 1500000, 1750000, 2000000,
> > +};  
> 
> To the best of my understanding these numbers are correct
> 

Hopefully it is all correct. I pulled the numbers from the datasheet,
but they are known to lie.

> > +
> > +static const struct msm_vbus_desc msm_vbus_desc_pm8150b = {
> > +	.curr_table = curr_table_pm8150b,
> > +	.n_current_limits = ARRAY_SIZE(curr_table_pm8150b),
> > +};
> > +
> > +static const struct msm_vbus_desc msm_vbus_desc_pmi8998 = {
> > +	.curr_table = curr_table_pmi8998,
> > +	.n_current_limits = ARRAY_SIZE(curr_table_pmi8998),
> > +};
> > +
> >  static const struct regulator_ops qcom_usb_vbus_reg_ops = {
> >  	.enable = regulator_enable_regmap,
> >  	.disable = regulator_disable_regmap,
> > @@ -37,8 +57,6 @@ static struct regulator_desc qcom_usb_vbus_rdesc
> > = { .ops = &qcom_usb_vbus_reg_ops,
> >  	.owner = THIS_MODULE,
> >  	.type = REGULATOR_VOLTAGE,
> > -	.curr_table = curr_table,
> > -	.n_current_limits = ARRAY_SIZE(curr_table),
> >  };
> >  
> >  static int qcom_usb_vbus_regulator_probe(struct platform_device
> > *pdev) @@ -48,6 +66,7 @@ static int
> > qcom_usb_vbus_regulator_probe(struct platform_device *pdev) struct
> > regmap *regmap; struct regulator_config config = { };
> >  	struct regulator_init_data *init_data;
> > +	const struct msm_vbus_desc *quirks;  
> 
> 'quirks' is one way to put it ;) I'd call it 'desc' or 'data' but it's
> totally a potayto/potahto discussion
> 

Is there a reasonable name for that? I suspect that later chips may add
more to the structure. I am happy to change it as there is at least one
more revision for this series.

> >  	int ret;
> >  	u32 base;
> >  
> > @@ -68,6 +87,12 @@ static int qcom_usb_vbus_regulator_probe(struct
> > platform_device *pdev) if (!init_data)
> >  		return -ENOMEM;
> >  
> > +	quirks = of_device_get_match_data(dev);
> > +	if (!quirks)
> > +		return -ENODEV;
> > +
> > +	qcom_usb_vbus_rdesc.curr_table = quirks->curr_table;
> > +	qcom_usb_vbus_rdesc.n_current_limits =
> > quirks->n_current_limits; qcom_usb_vbus_rdesc.enable_reg = base +
> > CMD_OTG; qcom_usb_vbus_rdesc.enable_mask = OTG_EN;
> >  	qcom_usb_vbus_rdesc.csel_reg = base +
> > OTG_CURRENT_LIMIT_CFG; @@ -80,18 +105,21 @@ static int
> > qcom_usb_vbus_regulator_probe(struct platform_device *pdev) rdev =
> > devm_regulator_register(dev, &qcom_usb_vbus_rdesc, &config); if
> > (IS_ERR(rdev)) { ret = PTR_ERR(rdev);
> > -		dev_err(dev, "not able to register vbus reg %d\n",
> > ret);
> > +		dev_err(dev, "Failed to register vbus reg %d\n",
> > ret); return ret;
> >  	}
> >  
> >  	/* Disable HW logic for VBUS enable */
> >  	regmap_update_bits(regmap, base + OTG_CFG, OTG_EN_SRC_CFG,
> > 0); 
> > +	dev_dbg(dev, "Registered QCOM VBUS regulator\n");  
> 
> Not sure how useful this is given the previous call creates a sysfs
> entry on success, but sure
> 
> Konrad

I like having a "I'm here" message so I can see when something is
loaded and to know where in the boot log it happened. Happy to remove it
if that is the standard.

James

Re: [PATCH 2/3] regulator: qcom_usb_vbus: Add support for PMI8998 VBUS

[James A. MacInnes]

On Wed, 12 Feb 2025 15:29:54 +0000
Caleb Connolly <caleb.connolly@linaro.org> wrote:


Hi Caleb,

> Hi James,
> 
> On 2/12/25 01:07, James A. MacInnes wrote:
> > This patch extends the Qualcomm USB VBUS regulator driver to support
> > PMI8998 PMIC alongside the existing support for PM8150B.  
> 
> Thanks for the patch!

Happy to try and contribute. I know that the working Type-C port is
going to be a misery after the relative simplicity of pushing the VBUS
upstream.
> > 
> > Key changes:
> > - Added current limit tables specific to PMI8998.  
> 
> I also played around with vbus on PMI8998 before for type-c support 
> (unfortunately didn't make it's way to the lists yet...) and I needed 
> some additional changes for this to work correctly. I found that it
> was possible for the overcurrent protection to be hit if the type-c
> port manager allowed the peripheral to pull current too early, and
> it's necessary to allow 2.5ms enable time.
> 
> PM8150b doesn't have these limitations (and supports the instant
> power role switch feature that's part of the type-c PD spec, allowing
> the power role to be switched without either side losing power e.g.
> when you unplug the power supply from a dock), hence it's only
> necessary for PMI8998.
> 
> I would suggest implementing a proper .is_enabled op to poll the
> status register for OTG_STATE_ENABLED and configuring 
> qcom_usb_vbus_rdesc.enable_time = 250000;
> 
> Kind regards,
> 

Technical question for you in regards to the VBUS overcurrent and
timing for the PMI8998. I would like to try and reproduce what you have
seen as my system hasn't had switching issues, but then again the TCPM
system may be covering the exact bug you are mentioning. I also
searched for some definite bit in the 4.9 Android driver and was left
wanting. As of yet, I have not had issues with the overcurrent
protecction.

I will be all too happy to migrate to the PM8150B and its associated
SoCs and leave the 845 platform to history.

Thank you for your feedback and I look forward to narrowing down this
issue.

Best wishes,

Re: [PATCH 2/3] regulator: qcom_usb_vbus: Add support for PMI8998 VBUS

[Caleb Connolly]

Hi James,

On 2/12/25 16:56, James A. MacInnes wrote:
> On Wed, 12 Feb 2025 15:29:54 +0000
> Caleb Connolly <caleb.connolly@linaro.org> wrote:
> 
> 
> Hi Caleb,
> 
>> Hi James,
>>
>> On 2/12/25 01:07, James A. MacInnes wrote:
>>> This patch extends the Qualcomm USB VBUS regulator driver to support
>>> PMI8998 PMIC alongside the existing support for PM8150B.
>>
>> Thanks for the patch!
> 
> Happy to try and contribute. I know that the working Type-C port is
> going to be a misery after the relative simplicity of pushing the VBUS
> upstream.

Yeah, it's hard to get used to the process... I'm happy to hear you're 
trying to get type-c working on this platform though! This will make a 
bunch of folks very happy if it finally lands, folks have been doing all 
sorts of workarounds for this over on 
https://wiki.postmarketos.org/wiki/OnePlus_6_(oneplus-enchilada)#OTG_doesn't_work


>>>
>>> Key changes:
>>> - Added current limit tables specific to PMI8998.
>>
>> I also played around with vbus on PMI8998 before for type-c support
>> (unfortunately didn't make it's way to the lists yet...) and I needed
>> some additional changes for this to work correctly. I found that it
>> was possible for the overcurrent protection to be hit if the type-c
>> port manager allowed the peripheral to pull current too early, and
>> it's necessary to allow 2.5ms enable time.
>>
>> PM8150b doesn't have these limitations (and supports the instant
>> power role switch feature that's part of the type-c PD spec, allowing
>> the power role to be switched without either side losing power e.g.
>> when you unplug the power supply from a dock), hence it's only
>> necessary for PMI8998.
>>
>> I would suggest implementing a proper .is_enabled op to poll the
>> status register for OTG_STATE_ENABLED and configuring
>> qcom_usb_vbus_rdesc.enable_time = 250000;
>>
>> Kind regards,
>>
> 
> Technical question for you in regards to the VBUS overcurrent and
> timing for the PMI8998. I would like to try and reproduce what you have
> seen as my system hasn't had switching issues, but then again the TCPM
> system may be covering the exact bug you are mentioning. I also
> searched for some definite bit in the 4.9 Android driver and was left
> wanting. As of yet, I have not had issues with the overcurrent
> protecction.

I guess there could be differences in our implementations, there is a 
delay after enabling the regulator for pm8150b type-c iirc

I never got around to chasing some of the remaining issues and sending 
this upstream, but maybe my patches will be useful for you:

https://git.codelinaro.org/caleb_connolly/kernel/-/commits/b4/pmi8998-typec/?ref_type=heads

I spent many many hours pouring over the smb2 driver downstream, so feel 
free to reach out if you have some specific questions.
> 
> I will be all too happy to migrate to the PM8150B and its associated
> SoCs and leave the 845 platform to history.

Well I for one am always happy to see SDM845 getting attention, it has a 
special place in my heart :>

And still gets love over here, even if not nearly enough of these 
patches have made it upstream: 
https://gitlab.com/sdm845-mainline/linux/-/commits/sdm845/6.13-release?ref_type=heads

Kind regards,
> 
> Thank you for your feedback and I look forward to narrowing down this
> issue.
> 
> Best wishes,

-- 
Caleb (they/them)

Re: [PATCH 2/3] regulator: qcom_usb_vbus: Add support for PMI8998 VBUS

[James A. MacInnes]

On Wed, 12 Feb 2025 16:09:01 +0000
Caleb Connolly <caleb.connolly@linaro.org> wrote:

> On 2/12/25 15:37, Mark Brown wrote:
> > On Wed, Feb 12, 2025 at 03:29:54PM +0000, Caleb Connolly wrote:
> >   
> >> I would suggest implementing a proper .is_enabled op to poll the
> >> status register for OTG_STATE_ENABLED and configuring  
> > 
> > No, that would be buggy.  Implement a get_status() operation if the
> > device can report status.  is_enabled() should report what the
> > driver asked for.  
> 
> Ahh yep, that's right. it should implement .get_status() (as per the 
> polling code in _regulator_do_enable()).
> 

I am happy to implement the proper get_status() operation, but the
other half of this, the type-c driver (that is functional on the 845),
is managing the status portion. With my testing so far, I see the
regulator resets the USB hub when I remove power and then supplies from
its own battery. Is this the expected operation? As of yet, I am not
seeing any failures and the original Android driver lacked the
knowledge of the output is status.

I can dive back into the original driver and the documentation to
verify. 

Any other thoughts?

Thank you,