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

[Dmitry Baryshkov]

On Thu, Jun 04, 2026 at 05:07:35PM +0530, Kathiravan Thirumoorthy wrote:
> 
> 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.

Ack

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

-- 
With best wishes
Dmitry