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@xxxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> >  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