Re: [PATCH 2/2] regulator: userspace-consumer: Multiple regulators

[Zev Weiss]

On Thu, Apr 20, 2023 at 01:46:14AM PDT, Naresh Solanki wrote:
> Hi Zev,
>
> On 20-04-2023 05:32 am, Zev Weiss wrote:
> > On Tue, Apr 18, 2023 at 07:50:51AM PDT, Naresh Solanki wrote:
> > > Use property regulator-supplies to determine all regulator
> > > supplies.
> > > This is useful in case of a connector having 2 or more supplies.
> > > Example: PCIe connector on mainboard can be powered by 12V & 3.3V
> > > suplies.
> > >
> > > Signed-off-by: Naresh Solanki <Naresh.Solanki@xxxxxxxxxxxxx>
> > > ---
> > > drivers/regulator/userspace-consumer.c | 19 +++++++++++++++----
> > > 1 file changed, 15 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/regulator/userspace-consumer.c 
> > > b/drivers/regulator/userspace-consumer.c
> > > index 97f075ed68c9..0bb49547b926 100644
> > > --- a/drivers/regulator/userspace-consumer.c
> > > +++ b/drivers/regulator/userspace-consumer.c
> > > @@ -120,7 +120,10 @@ static int 
> > > regulator_userspace_consumer_probe(struct platform_device *pdev)
> > >     struct regulator_userspace_consumer_data tmpdata;
> > >     struct regulator_userspace_consumer_data *pdata;
> > >     struct userspace_consumer_data *drvdata;
> > > -    int ret;
> > > +    struct device_node *np = pdev->dev.of_node;
> > > +    struct property *prop;
> > > +    const char *supply;
> > > +    int ret, count = 0;
> > >
> > >     pdata = dev_get_platdata(&pdev->dev);
> > >     if (!pdata) {
> > > @@ -131,11 +134,19 @@ static int 
> > > regulator_userspace_consumer_probe(struct platform_device *pdev)
> > >         memset(pdata, 0, sizeof(*pdata));
> > >
> > >         pdata->no_autoswitch = true;
> > > -        pdata->num_supplies = 1;
> > > -        pdata->supplies = devm_kzalloc(&pdev->dev, 
> > > sizeof(*pdata->supplies), GFP_KERNEL);
> > > +        pdata->num_supplies = of_property_count_strings(np, 
> > > "regulator-supplies");
> > > +        if (pdata->num_supplies < 0) {
> > > +            dev_err(&pdev->dev, "could not parse property 
> > > regulator-supplies");
> > > +            return -EINVAL;
> > > +        }
> > > +        pdata->supplies = devm_kzalloc(&pdev->dev,
> > > +                           sizeof(*pdata->supplies) * 
> > > pdata->num_supplies,
> > > +                           GFP_KERNEL);
> >
> > AFAICT this doesn't appear to implement the "vout" default specified 
> > in the dt-binding patch?
> The "regulator-supplies" property will hold the default value of 
> "vout" unless specified otherwise. As a result, the string enumeration 
> retrieves the value of "vout" by default, and the "vout-supply" 
> property is utilized for the regulator.

With the disclaimer that I'm not a DT expert, that's not my 
understanding of how DT works.  I don't think the 'default' value 
specified in the binding forces the fdt to always include that value if 
it's not present in the dts (since I'm pretty sure dtc doesn't even look 
at the binding to know that a default exists when compiling the dts); 
rather, it's information meant to be used by the software implementing 
support for that device (e.g. a driver for it) about what value to 
assume if the property isn't present in the fdt.

> > Also, since the core of the userspace-consumer driver itself already 
> > supports multiple regulators, it might be nice for the subject line 
> > to mention DT supplies or something a bit more specifically.
> Sure. How about 'Support multiple supplies' ?

I meant that it should explicitly mention "DT" (or perhaps "OF").  The 
driver's structure has supported multiple supplies since it was first 
introduced in 2009, so "Support multiple supplies" sounds like this 
commit is adding functionality that was already there.  What this patch 
is doing is connecting that existing support to the OF support logic so 
that it can be used in a device-tree context.

> > >         if (!pdata->supplies)
> > >             return -ENOMEM;
> > > -        pdata->supplies[0].supply = "vout";
> > > +
> > > +        of_property_for_each_string(np, "regulator-supplies", 
> > > prop, supply)
> > > +            pdata->supplies[count++].supply = supply;
> > >     }
> > >
> > >     if (pdata->num_supplies < 1) {
> > > --
> > > 2.39.1
> Regards,
> Naresh