Re: [PATCH 2/2] regulator: qcom-refgen: add support for the IPQ9650 SoC

[Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>]

On 6/3/2026 6:47 PM, Dmitry Baryshkov wrote:
> On Tue, Jun 02, 2026 at 02:52:00PM +0530, Kathiravan Thirumoorthy wrote:
>> IPQ9650 SoC has 2 REFGEN blocks providing the reference current to the
>> PCIe and USB, UNIPHY PHYs. For the other SoCs, clocks for this block is
>> enabled on power up but that's not the case for IPQ9650 and we have to
>> enable those clocks explicitly to bring up the PHYs properly.
>>
>> As per the design team, REFGEN block provides the reference current.
>> Hence marked the regulator type as REGULATOR_CURRENT.
>>
>> Signed-off-by: Kathiravan Thirumoorthy <kathiravan.thirumoorthy@oss.qualcomm.com>
>> ---
>>   drivers/regulator/qcom-refgen-regulator.c | 94 +++++++++++++++++++++++++++++--
>>   1 file changed, 90 insertions(+), 4 deletions(-)
>>
>> @@ -62,6 +75,49 @@ static int qcom_sdm845_refgen_is_enabled(struct regulator_dev *rdev)
>>   	return 1;
>>   }
>>   
>> +static int qcom_ipq9650_refgen_enable(struct regulator_dev *rdev)
>> +{
>> +	struct qcom_refgen_drvdata *drvdata = rdev_get_drvdata(rdev);
>> +	int ret;
>> +
>> +	ret = clk_bulk_prepare_enable(drvdata->num_clks, drvdata->clks);
>> +	if (ret)
>> +		return ret;
>> +
>> +	drvdata->enable_count++;
> I think, a regulator enable() is called only once. Is there a point in
> having enable_count as int?

Ack. Let me change it to boolean type.

>
>> +
>> +	return 0;
>> +}
>> +
>> +static int qcom_ipq9650_refgen_disable(struct regulator_dev *rdev)
>> +{
>> +	struct qcom_refgen_drvdata *drvdata = rdev_get_drvdata(rdev);
>> +
>> +	clk_bulk_disable_unprepare(drvdata->num_clks, drvdata->clks);
>> +	drvdata->enable_count--;
>> +
>> +	return 0;
>> +}
>> +
>> +static int qcom_ipq9650_refgen_is_enabled(struct regulator_dev *rdev)
>> +{
>> +	struct qcom_refgen_drvdata *drvdata = rdev_get_drvdata(rdev);
>> +
>> +	return drvdata->enable_count > 0;
>> +}
> Linux knows if it had enabled the regulator. I think the usual case for
> the is_enabled is to be able to read the hardware state. What is the
> point of having this callback?

Without the is_enabled(), regulator core assumes that regulator is 
enabled and always-on and is_enabled() is not called. Hence, it is 
needed in this case.

3458 static int _regulator_is_enabled(struct regulator_dev *rdev)
3459 {
3460         /* A GPIO control always takes precedence */
3461         if (rdev->ena_pin)
3462                 return rdev->ena_gpio_state;
3463
3464         /* If we don't know then assume that the regulator is 
always on */
3465         if (!rdev->desc->ops->is_enabled)
3466                 return 1;
3467
3468         return rdev->desc->ops->is_enabled(rdev);
3469 }

>
>> +
>> +static const struct regulator_desc ipq9650_refgen_desc = {
>> +	.enable_time = 5,
>> +	.name = "refgen",
>> +	.owner = THIS_MODULE,
>> +	.type = REGULATOR_CURRENT,
>> +	.ops = &(const struct regulator_ops) {
>> +		.enable		= qcom_ipq9650_refgen_enable,
>> +		.disable	= qcom_ipq9650_refgen_disable,
>> +		.is_enabled	= qcom_ipq9650_refgen_is_enabled,
>> +	},
>> +};
>> +
>>   static const struct regulator_desc sdm845_refgen_desc = {
>>   	.enable_time = 5,
>>   	.name = "refgen",