lkml.org 
[lkml]   [2023]   [Sep]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RESEND PATCH] regulator: userspace-consumer: Retrieve supplies from DT
On Sat, Sep 23, 2023 at 05:16:12AM PDT, Zev Weiss 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'.
>
>>>+ pdata->supplies[0].supply = supply_name;
>>>+ }
>>>+ }
>>> }
>>>
>>> if (pdata->num_supplies < 1) {
>>>
>>>base-commit: 451e85e29c9d6f20639d4cfcff4b9dea280178cc
>>>--
>>>2.41.0
>>>

Oh, and sorry for the barrage of self-replies here, but one more thing:
I think we should also update the regulator-output DT binding to reflect
the added flexibility that this provides.


Zev

\
 
 \ /
  Last update: 2023-09-23 20:37    [W:0.078 / U:3.156 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site