Messages in this thread | | | From | Naresh Solanki <> | Date | Wed, 4 Oct 2023 14:50:02 +0530 | Subject | Re: [RESEND PATCH] regulator: userspace-consumer: Retrieve supplies from DT |
| |
Hi
On Sat, 23 Sept 2023 at 17:33, Zev Weiss <zev@bewilderbeest.net> 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@9elements.com> > > > >Instead of hardcoding a single supply, retrieve supplies from DT. > > > >Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com> > >--- > > 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 > >
| |