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

[Kathiravan Thirumoorthy]

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@xxxxxxxxxxxxxxxx>
> > ---
> >   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",