diff mbox series

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

Message ID 20230922090330.1570350-1-naresh.solanki@9elements.com
State New
Headers
Series [RESEND] regulator: userspace-consumer: Retrieve supplies from DT |

Commit Message

Naresh Solanki Sept. 22, 2023, 9:03 a.m. UTC 
  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(-)


base-commit: 451e85e29c9d6f20639d4cfcff4b9dea280178cc

Comments

kernel test robot Sept. 23, 2023, 10:44 a.m. UTC | #1 
Hi Naresh,

kernel test robot noticed the following build errors:

[auto build test ERROR on 451e85e29c9d6f20639d4cfcff4b9dea280178cc]

url:    https://github.com/intel-lab-lkp/linux/commits/Naresh-Solanki/regulator-userspace-consumer-Retrieve-supplies-from-DT/20230922-170527
base:   451e85e29c9d6f20639d4cfcff4b9dea280178cc
patch link:    https://lore.kernel.org/r/20230922090330.1570350-1-naresh.solanki%409elements.com
patch subject: [RESEND PATCH] regulator: userspace-consumer: Retrieve supplies from DT
config: x86_64-randconfig-161-20230923 (https://download.01.org/0day-ci/archive/20230923/202309231829.VrGZUH4E-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230923/202309231829.VrGZUH4E-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309231829.VrGZUH4E-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/regulator/userspace-consumer.c: In function 'get_num_supplies':
>> drivers/regulator/userspace-consumer.c:126:9: error: implicit declaration of function 'for_each_property_of_node'; did you mean 'for_each_child_of_node'? [-Werror=implicit-function-declaration]
     126 |         for_each_property_of_node(pdev->dev.of_node, prop) {
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~
         |         for_each_child_of_node
>> drivers/regulator/userspace-consumer.c:126:59: error: expected ';' before '{' token
     126 |         for_each_property_of_node(pdev->dev.of_node, prop) {
         |                                                           ^~
         |                                                           ;
   drivers/regulator/userspace-consumer.c:124:13: warning: unused variable 'num_supplies' [-Wunused-variable]
     124 |         int num_supplies = 0;
         |             ^~~~~~~~~~~~
   drivers/regulator/userspace-consumer.c:136:1: error: no return statement in function returning non-void [-Werror=return-type]
     136 | }
         | ^
   drivers/regulator/userspace-consumer.c: In function 'regulator_userspace_consumer_probe':
   drivers/regulator/userspace-consumer.c:162:67: error: expected ';' before '{' token
     162 |                 for_each_property_of_node(pdev->dev.of_node, prop) {
         |                                                                   ^~
         |                                                                   ;
   cc1: some warnings being treated as errors


vim +126 drivers/regulator/userspace-consumer.c

   120	
   121	static int get_num_supplies(struct platform_device *pdev)
   122	{
   123		struct  property *prop;
   124		int num_supplies = 0;
   125	
 > 126		for_each_property_of_node(pdev->dev.of_node, prop) {
   127			const char *prop_name = prop->name;
   128			int len = strlen(prop_name);
   129	
   130			if (len > SUPPLY_SUFFIX_LEN &&
   131			    strcmp(prop_name + len - SUPPLY_SUFFIX_LEN, SUPPLY_SUFFIX) == 0) {
   132				num_supplies++;
   133			}
   134		}
   135		return num_supplies;
   136	}
   137
Zev Weiss Sept. 23, 2023, 12:02 p.m. UTC | #2 
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 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
>
Zev Weiss Sept. 23, 2023, 12:16 p.m. UTC | #3 
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
>>
Zev Weiss Sept. 23, 2023, 6:35 p.m. UTC | #4 
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
Naresh Solanki Oct. 4, 2023, 9:20 a.m. UTC | #5 
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
> >
Naresh Solanki Oct. 4, 2023, 9:33 a.m. UTC | #6 
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
> >>
diff mbox series

Patch

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
+
+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++;
+		}
+	}
+	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;
 	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);
 		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) {
+				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) {