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

From: Zev Weiss
Date: Sat Sep 23 2023 - 08:03:03 EST


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).

+
+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 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';
+ pdata->supplies[0].supply = supply_name;
+ }
+ }
}

if (pdata->num_supplies < 1) {

base-commit: 451e85e29c9d6f20639d4cfcff4b9dea280178cc
--
2.41.0