[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CABqG17jKFfv_cfSbWGUXnRn3wNrc6X-rCgT1gRKWopefoO-+oQ@mail.gmail.com>
Date: Wed, 4 Oct 2023 15:03:52 +0530
From: Naresh Solanki <naresh.solanki@...ements.com>
To: Zev Weiss <zev@...ilderbeest.net>
Cc: broonie@...nel.org, Liam Girdwood <lgirdwood@...il.com>,
linux-kernel@...r.kernel.org
Subject: Re: [RESEND PATCH] regulator: userspace-consumer: Retrieve supplies
from DT
Hi
On Sat, 23 Sept 2023 at 17:46, Zev Weiss <zev@...ilderbeest.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@...ements.com>
> >>
> >>Instead of hardcoding a single supply, retrieve supplies from DT.
> >>
> >>Signed-off-by: Naresh Solanki <Naresh.Solanki@...ements.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
> >>
Powered by blists - more mailing lists