Messages in this thread | | | From | Naresh Solanki <> | Date | Wed, 4 Oct 2023 15:03:52 +0530 | Subject | Re: [RESEND PATCH] regulator: userspace-consumer: Retrieve supplies from DT |
| |
Hi
On Sat, 23 Sept 2023 at 17:46, Zev Weiss <zev@bewilderbeest.net> wrote: > > On Sat, Sep 23, 2023 at 05:02:59AM PDT, Zev Weiss 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). > > > >>+ > >>+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. > > > >>+ } > >>+ 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. > > > >> 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. > > > >> 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 > > Or rather prop_supply_name_len(), to make the name a bit more accurate. > > >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. > > > >>+ 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'; > > Also, kstrndup() would be a cleaner replacement for these lines, though > then the cleanup would get messy, and sadly a devm_kstrndup() doesn't > currently exist -- maybe it'd be worth adding separately? Or > alternately you could just use devm_kstrdup() and then truncate it by > inserting a '\0'. Sure. Will make it like:
char *supply_name = devm_kstrdup(&pdev->dev, prop_name, GFP_KERNEL); supply_name[supply_len] = '\0'; pdata->supplies[0].supply = supply_name;
Regards, Naresh > > >>+ pdata->supplies[0].supply = supply_name; > >>+ } > >>+ } > >> } > >> > >> if (pdata->num_supplies < 1) { > >> > >>base-commit: 451e85e29c9d6f20639d4cfcff4b9dea280178cc > >>-- > >>2.41.0 > >>
| |