Re: [RESEND PATCH] regulator: userspace-consumer: Retrieve supplies from DT

[Naresh Solanki]

Hi

On Sat, 23 Sept 2023 at 17:33, Zev Weiss <zev@xxxxxxxxxxxxxxxxx> wrote:
>
> Hi Naresh,
>
> This looks basically alright to me, though a few suggested tweaks
> below...
>
> On Fri, Sep 22, 2023 at 02:03:29AM PDT, Naresh Solanki wrote:
> >From: Naresh Solanki <Naresh.Solanki@xxxxxxxxxxxxx>
> >
> >Instead of hardcoding a single supply, retrieve supplies from DT.
> >
> >Signed-off-by: Naresh Solanki <Naresh.Solanki@xxxxxxxxxxxxx>
> >---
> > drivers/regulator/userspace-consumer.c | 43 ++++++++++++++++++++++++--
> > 1 file changed, 40 insertions(+), 3 deletions(-)
> >
> >diff --git a/drivers/regulator/userspace-consumer.c b/drivers/regulator/userspace-consumer.c
> >index 97f075ed68c9..a3d3e1e6ca74 100644
> >--- a/drivers/regulator/userspace-consumer.c
> >+++ b/drivers/regulator/userspace-consumer.c
> >@@ -115,11 +115,32 @@ static const struct attribute_group attr_group = {
> >       .is_visible =  attr_visible,
> > };
> >
> >+#define SUPPLY_SUFFIX "-supply"
> >+#define SUPPLY_SUFFIX_LEN 7
>
> I think 'strlen(SUPPLY_SUFFIX)' would be preferable to a numeric literal
> here; it's less fragile and the compiler can evaluate it at compile-time
> anyway (not that it's likely to be performance-critical in this context
> I'd expect).
Sure.

>
> >+
> >+static int get_num_supplies(struct platform_device *pdev)
> >+{
> >+      struct  property *prop;
> >+      int num_supplies = 0;
> >+
> >+      for_each_property_of_node(pdev->dev.of_node, prop) {
> >+              const char *prop_name = prop->name;
> >+              int len = strlen(prop_name);
> >+
> >+              if (len > SUPPLY_SUFFIX_LEN &&
> >+                  strcmp(prop_name + len - SUPPLY_SUFFIX_LEN, SUPPLY_SUFFIX) == 0) {
> >+                      num_supplies++;
> >+              }
>
> Preferred coding style is to omit braces around single-line 'if' blocks.
Sure
>
> >+      }
> >+      return num_supplies;
> >+}
> >+
> > 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;
> >+      struct  property *prop;
>
> Looks like there's an extra space after 'struct' here.
Will fix that.
>
> >       int ret;
> >
> >       pdata = dev_get_platdata(&pdev->dev);
> >@@ -131,11 +152,27 @@ 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 = get_num_supplies(pdev);
> >+
> >+              pdata->supplies = devm_kzalloc(&pdev->dev, pdata->num_supplies *
> >+                                             sizeof(*pdata->supplies), GFP_KERNEL);
>
> Splitting the multiplication across two lines like that isn't great
> readability-wise IMO; it might be better to just assign it to a variable
> and use that instead to make things fit nicely.
Sure can do that. Will make like:

supplies_size = pdata->num_supplies * sizeof(*pdata->supplies);
pdata->supplies = devm_kzalloc(&pdev->dev, supplies_size, GFP_KERNEL);

>
> >               if (!pdata->supplies)
> >                       return -ENOMEM;
> >-              pdata->supplies[0].supply = "vout";
> >+
> >+              for_each_property_of_node(pdev->dev.of_node, prop) {
> >+                      const char *prop_name = prop->name;
> >+                      int len = strlen(prop_name);
> >+
> >+                      if (len > SUPPLY_SUFFIX_LEN &&
> >+                          strcmp(prop_name + len - SUPPLY_SUFFIX_LEN, SUPPLY_SUFFIX) == 0) {
>
> Rather than duplicating this suffix-checking code, how about factoring
> out a helper function like prop_is_supply() or something to use both
> here and in get_num_supplies()?
>
> Or actually to make it integrate here a little more nicely, you could
> have something like 'size_t prop_supply_name(char*)', returning zero if
> it doesn't end with "-supply", and the length of the name before the
> suffix if it does, so that get_num_supplies() could use it as a boolean
> and the code below could use the length to determine the allocation
> size.
Yes thats better idea will do that.

>
> >+                              char *supply_name = devm_kzalloc(&pdev->dev,
> >+                                                               len - SUPPLY_SUFFIX_LEN + 1,
> >+                                                               GFP_KERNEL);
> >+                              strscpy(supply_name, prop_name, len - SUPPLY_SUFFIX_LEN);
> >+                              supply_name[len - SUPPLY_SUFFIX_LEN] = '\0';
> >+                              pdata->supplies[0].supply = supply_name;
> >+                      }
> >+              }
> >       }
> >
> >       if (pdata->num_supplies < 1) {
> >
> >base-commit: 451e85e29c9d6f20639d4cfcff4b9dea280178cc
> >--
> >2.41.0
> >