Message ID | 20230922090330.1570350-1-naresh.solanki@9elements.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:612c:172:b0:3f2:4152:657d with SMTP id h50csp5939701vqi; Fri, 22 Sep 2023 17:05:36 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHT2QojnWBT0gR1sOHlof3+wd4dch7zfQn2TJOyk8iW5taX5EFwfi0WOSdm6zkcYBelHf/P X-Received: by 2002:a17:902:f813:b0:1bc:9c70:b955 with SMTP id ix19-20020a170902f81300b001bc9c70b955mr804998plb.28.1695427535823; Fri, 22 Sep 2023 17:05:35 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1695427535; cv=none; d=google.com; s=arc-20160816; b=oY1JvKSn5S+fejM8mrV1B7jj/ifeV8X9GxAhWxiZ6NcTUujZg/rX7UFVU91C+uApjx 0MwVbpIZsv5ITJ4BKC4yH1JEH+JOPl1Rj56o54DcBOQceYgpVA8hezcQkJ4VTslJ1tQ5 j2t+NSmYQIVVaF4eNCuznJ2l8D8zAFQwkVuFTA5AFvPZL99+2aX8M3ziYfzKiJ4Ei8AR 8opjK3J2ue7GsMjEv7Oy9geegxvIXpYkO9hyglCg+8CYRQEbbZNe3YAXwbvIz5TC8iq7 wpjq1IuVQ+3WvqDVhn1vqSryC5PjymGHFkORIcPHzm4AWWtxtAiZxIy497+JJD94+e+k ymyQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=JwCuwBRzQRtzIIEcUqVCtqKyAERbE1Vs3ON90clTwDo=; fh=8KXh4pBEmy+NtF/opnUgUmKV+7RocDF9urRJBwd73MA=; b=EaHOd2F4+VqjJMB9UqiFHUyjcELs7UFBjY+gUOasOVVAGnI002FtKDA9pVNL5iHIJ/ KJ2AUHdyqdzUg7LdXFd4clXV8iA6nBiRhZAAT7aEXNK7ckHoyAGx3bP2CgcY9zfRhzKn lLsS8nQgjOZzRdWIy4iDx59SFn0jqYjLiFYDRiqR6DlwSwbW9VLLaLJVkdwlN7f9sr6C AODt3mEVxAC+1zMXnd972cg4cQhM7vpnfdYlfOTysjkrRimBtmt+ThRfPi7C4bKaSqMJ E4y6Ajazid1HxnTzDEcZqaGx+B9vRz0kmukYJdj35TPKyzJ1RbrC0AKXRdRugA9/m7ER Bedg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@9elements.com header.s=google header.b="RHLtSQ/d"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=9elements.com Received: from groat.vger.email (groat.vger.email. [23.128.96.35]) by mx.google.com with ESMTPS id s9-20020a170903214900b001c3e9b0baeasi4641820ple.430.2023.09.22.17.05.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 22 Sep 2023 17:05:35 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35; Authentication-Results: mx.google.com; dkim=pass header.i=@9elements.com header.s=google header.b="RHLtSQ/d"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=9elements.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by groat.vger.email (Postfix) with ESMTP id 42DB4832B167; Fri, 22 Sep 2023 02:03:56 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at groat.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232809AbjIVJDq (ORCPT <rfc822;chrisfriedt@gmail.com> + 30 others); Fri, 22 Sep 2023 05:03:46 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45570 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232719AbjIVJDp (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 22 Sep 2023 05:03:45 -0400 Received: from mail-wr1-x42b.google.com (mail-wr1-x42b.google.com [IPv6:2a00:1450:4864:20::42b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4DC6EFB for <linux-kernel@vger.kernel.org>; Fri, 22 Sep 2023 02:03:39 -0700 (PDT) Received: by mail-wr1-x42b.google.com with SMTP id ffacd0b85a97d-32001d16a14so1785727f8f.1 for <linux-kernel@vger.kernel.org>; Fri, 22 Sep 2023 02:03:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=9elements.com; s=google; t=1695373417; x=1695978217; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=JwCuwBRzQRtzIIEcUqVCtqKyAERbE1Vs3ON90clTwDo=; b=RHLtSQ/dcfFp/hTxnjPBGQj74xTbBt7b4lGy81ESxwdy37hrR09Em98GhyGJDhTwiQ WtvfCQx3Vmq48trhhrz4YQ7E1RnTu8fm07ef1/DpD6INrVqVZi2ZxFIVCrtild+EHdPW VUfdG+i//mewoeGxwN0qahTbL91gArR+GTpTp8zD+L/5J0uurY23MnaR7c638eEEEN9a r7F0V/MNqgdRjh+px7thyS/pOaH0KsNu/No9deG04BGQveDUufQjk4Zg/OZqQqrDMad/ 57qfoo/jSqdqgy7kLB9Fd7nSFQb3kMRNzRRDfiK9Q5m45VReUYgxTSlhFO6wOE9DrPu0 1b5w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695373417; x=1695978217; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=JwCuwBRzQRtzIIEcUqVCtqKyAERbE1Vs3ON90clTwDo=; b=Qvipt/vZawiFL0om7ijuvZbQ6zDu3aw5QUk4VI+xPEufLQAHiTSM+0I/7p6uwMW7MG koXCuoHBKhkDdkmHjJmxVBFMa677KjW4Phq8gGDFh/vW4GBofGUl9Oc335ODy6rthTz0 vCNvS6ZvjrJoxWRNmHg3uVS6dfSshZOQnahJf3eklKcP3Z0QiwzDmDUdCrCSzFZOOtai xymTj8qZK3E4GiaDphU+DT0tF7K6ljF1VMj1q6tifHRVgsg5FLkBWxdQa1RNLFZK/f5W i4MDlt+ViZhuRL97p88Gv7Nz51Pv9pXpyjuTK3APDxPEoyLWlObK7tlLO+YmKH3p3Sp1 QQ0Q== X-Gm-Message-State: AOJu0YyVr3EW1XzatWNQvVQ/E1oyvfciv3RArQd6eOU7pD5csPrFE9dd 4gFA1bynl0WW4IVV1VDFgI3injpArgmWEK07Yc23lA== X-Received: by 2002:adf:e7c9:0:b0:317:3f70:9dc4 with SMTP id e9-20020adfe7c9000000b003173f709dc4mr6353076wrn.31.1695373417393; Fri, 22 Sep 2023 02:03:37 -0700 (PDT) Received: from stroh80.sec.9e.network (ip-078-094-000-051.um19.pools.vodafone-ip.de. [78.94.0.51]) by smtp.gmail.com with ESMTPSA id x17-20020a5d6511000000b0031fd849e797sm3917553wru.105.2023.09.22.02.03.36 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 22 Sep 2023 02:03:36 -0700 (PDT) From: Naresh Solanki <naresh.solanki@9elements.com> To: broonie@kernel.org, zev@bewilderbeest.net, Liam Girdwood <lgirdwood@gmail.com> Cc: Naresh Solanki <Naresh.Solanki@9elements.com>, linux-kernel@vger.kernel.org Subject: [RESEND PATCH] regulator: userspace-consumer: Retrieve supplies from DT Date: Fri, 22 Sep 2023 11:03:29 +0200 Message-ID: <20230922090330.1570350-1-naresh.solanki@9elements.com> X-Mailer: git-send-email 2.41.0 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Fri, 22 Sep 2023 02:03:56 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1777784623469893557 X-GMAIL-MSGID: 1777784623469893557 |
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
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
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 >
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 >>
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
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 > >
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 --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) {