Received: by 10.192.165.156 with SMTP id m28csp897720imm; Thu, 19 Apr 2018 09:18:12 -0700 (PDT) X-Google-Smtp-Source: AIpwx4/cZVnpiwaez3dy+b75CuY3+34GyJOwje0wDiE0Dv+FwR/tUJKN/y+N7nKSZuFWCprLGWyp X-Received: by 10.99.115.28 with SMTP id o28mr5266836pgc.238.1524154692237; Thu, 19 Apr 2018 09:18:12 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1524154692; cv=none; d=google.com; s=arc-20160816; b=rV/IygGcrF87tXXiNoD2PW9WRxExcgTa74jE8MfLX62+YEze3H5frwLXtTRxsK22/D CRPBOVy6kMxzZOtiajRwiuww4im+WJXzdxm2Wtxm+dIGZaAYLUY10qt8p/GdW8jP2ywt fUHe2d1shGI6QKfo7s6vlUgQ0kFo1jjF/AHed85fJYxk0cZwQ/EPQm2Mkujgq3GHYcUp P6TCXqFM3bu2lz6yxAIUAKIjEmBq1k+BHMv/ci3wXfCUgf6YKxxblDF/swtEclQ2b+DH LvO3igxuKzvhLas41l4uK4h4qg20vEi4zL/ck7aHBZQE47ZatKOVuN+6I1bdC/LAdl49 Tlqw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :references:in-reply-to:mime-version:dkim-signature:dkim-signature :arc-authentication-results; bh=lurCPgshz7RukXJOgFziBhxCE3jDDVk5DhKxfBoqJkc=; b=UoAJtKYIrpCQrtKIS7wjKTRgZQcgmQfP2swjx68kBRIPF4vcpWv/p6B23HdwB9bP71 bcCc0PQHwl14uhL+0dY4UzwJifaIWK3us18SYVNW8+cU2Ekjc8w956zYvmCQ+tm42kCE SKaCHF8CMwejC1xZiHcJ1Fa90dT12CWsA0uaZKkqKPK4vd7YnW9ssSs1WM8z34HPa4TW sk+/KMnndjJBCetq0a4Cu25RsbSzZIU9i6dMHVi9TH6M4XM6LJKreJlaP0ZPPSN9bcXT ofW/UxaAUIeDj3BUQBnq1/E1DzM/o7byo/m+vNG4hoBwNFfn4MPrBkfDsRmZHgkogmD4 Bg/Q== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@google.com header.s=20161025 header.b=BMvtCI0M; dkim=fail header.i=@chromium.org header.s=google header.b=jwN8qDQi; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=chromium.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id r4si158613pgp.264.2018.04.19.09.17.56; Thu, 19 Apr 2018 09:18:12 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=fail header.i=@google.com header.s=20161025 header.b=BMvtCI0M; dkim=fail header.i=@chromium.org header.s=google header.b=jwN8qDQi; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753780AbeDSQQn (ORCPT + 99 others); Thu, 19 Apr 2018 12:16:43 -0400 Received: from mail-ua0-f175.google.com ([209.85.217.175]:33969 "EHLO mail-ua0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753527AbeDSQQl (ORCPT ); Thu, 19 Apr 2018 12:16:41 -0400 Received: by mail-ua0-f175.google.com with SMTP id t4so3812431ual.1 for ; Thu, 19 Apr 2018 09:16:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=lurCPgshz7RukXJOgFziBhxCE3jDDVk5DhKxfBoqJkc=; b=BMvtCI0MCL0z4BtfvX4EP0olL7khbzlopjrgqJy1mr+ElrmGWr/54ZGQiF/OJCNJSw BiMO4pqhngwNI8Y9C5Imk234jICwH/q5ZQPaAg2Yevzx90fr4BKtF435rJuy3AXMjIu/ ybgF6zvk+A2HcwvuCyQIrV2HU+JuNS82Eb0FWWyUUEwNCsMWBux8UTcskpx/x7qSO307 sCyQ9IoQjtyF4r/XFNQPsfI6FRAD6HbayLwtpuCi/Xb4HoS4178XOb28dTRjtKKZmQNT kEWqLmA8YqCaAziWfC+URJh3lsDmCHfbBavOWirxhmnb5VkafTnglc5j+DD70xxDI+nK gdOA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=lurCPgshz7RukXJOgFziBhxCE3jDDVk5DhKxfBoqJkc=; b=jwN8qDQiVoCKfrC+MiIozIXryh2Gx8Xu+5G+vbJdmMJJhc0DB9XogPLIRWcqk4vp4V pNvlL8DuIXGVlxR2HFdMUixlr5gZ67v/qI7wLLRGSt+ZztQcE5K15dDz/e4Rb4LHbOVn G1bgaDrZCI0WrOguEfs+TbP9eyYWcPZ85S8xw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=lurCPgshz7RukXJOgFziBhxCE3jDDVk5DhKxfBoqJkc=; b=DijnY/tCTltpjxFmjXOfUKiP4LgZyGcwqFZSs+aY27u14p7N1UaMKLNk7rVQOi3fDV pmAfXvxMI+p9bLHuwlyI8CBb32yLGA60bh17cIELZeK2eZ3AyhpCET2JCykw/w2e48qf y3W4yL1JXQRV7C6cCXe6GsAPM5EZ5+zwTeoSmIkszsF5N3/SX/dE/UH4qXk5cmJebxwR kLeS0tPS8PJClxpZndCfuCTDXC6MESrZ6L+rs8JC14uhuIXysTjW3ARekMg5xl0/liqX SmdmOE2I2D3srLjPWfFOWvIAv/+CTdB93Mro4+x6DZbY0mvaeNk7a4mTaToElVrDWWcj Ygyw== X-Gm-Message-State: ALQs6tAjmTSxDIVviuwkZhRQgOiV0kIcT+zTt3zdx+duWkMvJXlbyVkE FXqx0DDt/qucxOihnB66MSzt9VmkXCUaCGsm1ieZGQ== X-Received: by 10.176.30.132 with SMTP id o4mr5048734uak.154.1524154599204; Thu, 19 Apr 2018 09:16:39 -0700 (PDT) MIME-Version: 1.0 Received: by 10.31.137.13 with HTTP; Thu, 19 Apr 2018 09:16:38 -0700 (PDT) In-Reply-To: <4e3353fe-ebb5-46bb-aa58-49ad04c4d9db@codeaurora.org> References: <4b45f41996ea3344340e44fab2dc9e487572e209.1523673467.git.collinsd@codeaurora.org> <4e3353fe-ebb5-46bb-aa58-49ad04c4d9db@codeaurora.org> From: Doug Anderson Date: Thu, 19 Apr 2018 09:16:38 -0700 X-Google-Sender-Auth: QjW0kJDAsvOLUzZWcuRkstGm10U Message-ID: Subject: Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver To: David Collins Cc: Mark Brown , Liam Girdwood , Rob Herring , Mark Rutland , linux-arm-msm@vger.kernel.org, Linux ARM , devicetree@vger.kernel.org, LKML , Rajendra Nayak , Stephen Boyd , Matthias Kaehlcke Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On Wed, Apr 18, 2018 at 4:30 PM, David Collins wrote: >>> + * @drms_mode: Array of regulator framework modes which can >>> + * be configured dynamically for this regulator >>> + * via the set_load() callback. >> >> Using the singular for something that is an array is confusing. Why >> not "drms_modes" or "drms_mode_arr"? In the past review you said >> 'Perhaps something along the lines of "drms_modes"'. > > It seems awkward to me to use a plural for arrays as it leads to indexing > like this: vreg->drms_modes[i]. "mode i" seems better than "modes i". > However, I'm willing to change this to be drms_modes and drms_mode_max_uAs > if that style is preferred. I'd very much like a plural here. >>> +static int rpmh_regulator_vrm_set_mode_bypass(struct rpmh_vreg *vreg, >>> + unsigned int mode, bool bypassed) >>> +{ >>> + struct tcs_cmd cmd = { >>> + .addr = vreg->addr + RPMH_REGULATOR_REG_VRM_MODE, >>> + }; >> >> Please add: >> >> if (mode & ~(REGULATOR_MODE_STANDBY | >> REGULATOR_MODE_IDLE | >> REGULATOR_MODE_NORMAL | >> REGULATOR_MODE_FAST)) >> return -EINVAL; >> >> That way if someone adds a new mode you don't index off the end of >> your array. Ah, I see, you have this in rpmh_regulator_vrm_set_mode >> by checking if mode > REGULATOR_MODE_STANDBY. That works. Can you >> move it here so it's closer to where the array access is? > > Theoretically, the (mode > REGULATOR_MODE_STANDBY) check in > rpmh_regulator_vrm_set_mode() shouldn't be necessary at all. I felt safer > leaving it in though. The framework ensures that no mode values may be > passed into rpmh_regulator_vrm_set_mode() which is not identified in > constraints.valid_modes_mask. Similar sanitization happens for internal > rpmh_regulator_vrm_set_mode() calls. > > I'll move the (mode > REGULATOR_MODE_STANDBY) check into > rpmh_regulator_vrm_set_mode_bypass(). Ah, good point about the valid_modes_mask! I'm happy with moving the test here. I wouldn't mind a comment saying that the check is probably overkill because the framework already checks valid_modes_mask but shouldn't hurt. >>> + >>> + return rpmh_regulator_vrm_set_mode(rdev, vreg->drms_mode[i]); >> >> Might not hurt to have a comment saying that this calls >> rpmh_regulator_vrm_set_mode() instead of calling >> rpmh_regulator_vrm_set_mode_bypass() directly because this is supposed >> to change the mode returned by a future call to get_mode(). > > This seems pretty clear on inspection of the very closely spaced > functions. I don't see the need for a comment about it. It wasn't clear to me--I thought it might have just been because you didn't want to manually pass in the current bypass state. Then I looked at the function and thought there might have been a bug because it was saving the mode. Then I looked back at the regulator framework and finally came to the conclusion that set_load() is supposed to change the mode (AKA: you'd expect that calling get_mode() after set_load() would show you the mode you ended up at). I guess this is all perhaps obvious to any regulator framework experts, but since I spent 5 minutes digging through all that it seemed like it deserved a comment to save the next person the 5 minutes. ...but I won't insist. >>> +static int rpmh_regulator_vrm_get_bypass(struct regulator_dev *rdev, >>> + bool *enable) >>> +{ >>> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); >>> + >>> + *enable = vreg->bypassed; >>> + >>> + return 0; >> >> Should you return an error code if nobody has ever called set_bypass? >> ...or is it OK to just return "not bypassed"? Please document this in >> the code. > > I think it is fine to return "not bypassed" by default if there is no > configuration in place to enable bypassing. How are you suggesting that > this be documented in the code? I guess I wish the function had comments and then it could be documented there. ...but none of the functions in this file do... I guess you're right that it's clear enough without a comment, so let's just leave it as is. >>> +static int rpmh_regulator_parse_vrm_modes(struct rpmh_vreg *vreg, >>> + struct device *dev, struct device_node *node) >>> +{ >>> + const char *prop; >>> + int i, len, ret, mode; >>> + u32 *buf; >>> + >>> + /* qcom,allowed-drms-modes is optional */ >>> + prop = "qcom,allowed-drms-modes"; >>> + len = of_property_count_elems_of_size(node, prop, sizeof(u32)); >>> + if (len < 0) >>> + return 0; >>> + >>> + vreg->drms_mode = devm_kcalloc(dev, len, sizeof(*vreg->drms_mode), >>> + GFP_KERNEL); >>> + vreg->drms_mode_max_uA = devm_kcalloc(dev, len, >>> + sizeof(*vreg->drms_mode_max_uA), GFP_KERNEL); >>> + if (!vreg->drms_mode || !vreg->drms_mode_max_uA) >>> + return -ENOMEM; >>> + vreg->drms_mode_count = len; >>> + >>> + buf = kcalloc(len, sizeof(*buf), GFP_KERNEL); >>> + if (!buf) >>> + return -ENOMEM; >>> + >>> + ret = of_property_read_u32_array(node, prop, buf, len); >>> + if (ret < 0) { >>> + dev_err(dev, "%s: unable to read %s, ret=%d\n", >>> + node->name, prop, ret); >>> + goto done; >>> + } >>> + >>> + for (i = 0; i < len; i++) { >>> + mode = vreg->hw_data->of_map_mode(buf[i]); >>> + if (mode <= 0) { >> >> Should be < 0, not <= 0 right? Unless we take Javier's suggestion >> (see ) and make 0 be >> invalid... > > It looks like the way forward is REGULATOR_MODE_INVALID == 0 so '<= 0' is > fine here. I suppose that it could be changed to '== > REGULATOR_MODE_INVALID' as well. Yes, assuming my patch lands using "==" is better. Checking whether an unsigned value is <= 0 is confusing. >>> + prop = "qcom,regulator-initial-voltage"; >>> + ret = of_property_read_u32(node, prop, &uV); >>> + if (!ret) { >>> + range = &vreg->hw_data->voltage_range; >>> + selector = DIV_ROUND_UP(uV - range->min_uV, >>> + range->uV_step) + range->min_sel; >>> + if (uV < range->min_uV || selector > range->max_sel) { >>> + dev_err(dev, "%s: %s=%u is invalid\n", >>> + node->name, prop, uV); >>> + return -EINVAL; >>> + } >>> + >>> + vreg->voltage_selector = selector; >>> + >>> + cmd[cmd_count].addr >>> + = vreg->addr + RPMH_REGULATOR_REG_VRM_VOLTAGE; >>> + cmd[cmd_count++].data >>> + = DIV_ROUND_UP(selector * range->uV_step >>> + + range->min_uV, 1000); >>> + } >> >> Seems like you want an "else { vreg->voltage_selector = -EINVAL; }". >> Otherwise "get_voltage_sel" will return selector 0 before the first >> set, right? >> >> Previously Mark said: "If the driver can't read values it should >> return an appropriate error code." >> ...and previously you said: "I'll try this out and see if the >> regulator framework complains during regulator registration." > > I tested out what happens when vreg->voltage_selector = -EINVAL is set > when qcom,regulator-initial-voltage is not present. This results in > devm_regulator_register() failing and subsequently causing the > qcom_rpmh-regulator probe to fail. The error happens in > machine_constraints_voltage() [1]. > > This leaves two courses of action: > 1. (current patch set) allow voltage_selector to stay 0 if uninitialized > 2. Set voltage_selector = -EINVAL by default and specify in DT binding > documentation that qcom,regulator-initial-voltage is required for VRM > managed RPMh regulator resources which have regulator-min-microvolt and > regulator-max-microvolt specified. > > Are you ok with the DT implications of option #2? You'd need to ask Mark if he's OK with it, but a option #3 is to add a patch to your series fix the regulator framework to try setting the voltage if _regulator_get_voltage() fails. Presumably in machine_constraints_voltage() you'd now do something like: int target_min, target_max; int current_uV = _regulator_get_voltage(rdev); if (current_uV < 0) { /* Maybe this regulator's hardware can't be read and needs to be initted */ _regulator_do_set_voltage( rdev, rdev->constraints->min_uV, rdev->constraints->min_uV); current_uV = _regulator_get_voltage(rdev); } if (current_uV < 0) { rdev_err(rdev, "failed to get the current voltage(%d)\n", current_uV); return current_uV; } If Mark doesn't like that then I guess I'd be OK w/ initting it to 0 but this needs to be documented _somewhere_ (unlike for bypass it's not obvious, so you need to find someplace to put it). I'd rather not hack the DT to deal with our software limitations. >>> +static int rpmh_regulator_init_vreg(struct rpmh_vreg *vreg, struct device *dev, >>> + struct device_node *node, const char *pmic_id, >>> + const struct rpmh_vreg_init_data *rpmh_data) >>> +{ >>> + struct regulator_config reg_config = {}; >>> + char rpmh_resource_name[20] = ""; >>> + struct regulator_dev *rdev; >>> + enum rpmh_regulator_type type; >>> + struct regulator_init_data *init_data; >>> + unsigned int mode; >>> + int i, ret; >>> + >>> + for (; rpmh_data->name; rpmh_data++) >>> + if (!strcmp(rpmh_data->name, node->name)) >>> + break; >>> + >>> + if (!rpmh_data->name) { >>> + dev_err(dev, "Unknown regulator %s\n", node->name); >>> + return -EINVAL; >>> + } >>> + >>> + scnprintf(rpmh_resource_name, sizeof(rpmh_resource_name), >>> + rpmh_data->resource_name, pmic_id); >> >> If the resulting string is exactly 20 characters then >> rpmh_resource_name won't be zero terminated. Please handle this >> properly. > > The output of scnprintf() is always null-terminated; therefore, no other > check is needed. Also, RPMh resource names stored in SMEM command DB data > structure are at most 8 bytes (<= 7 char + '\0' or 8 char and no '\0') so > using 20 chars in the buffer is overkill anyway. Wow, not sure where I looked to see that scnprintf() didn't always null-terminate. Sorry. Ignore this. >>> +static const u32 pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = { >> >> I may have suggested using this as an array that could be used as a >> "map" (index by regulator framework mode and get the PMIC mode), but >> now that I see it it doesn't seem super appealing since the regulator >> framework mode is not 0, 1, 2, 3 but is actually 1, 2, 4, 8. ...but I >> guess it's not too bad--you're allocating 9 ints to map 4 elements and >> I guess that's about as efficient as you're going to get even if it >> feels a bit ugly. > > I'm ok with the sparse mapping as it makes indexing as simple as possible > and the extra space used is insignificant. > > >> ...but still a few improvements: >> >> * Don't specify the size of the array as "REGULATOR_MODE_STANDBY + 1". >> Let the compiler handle this. It should do the right thing. Then if >> someone ever changes the order of the #defines things will just work, >> I think. >> >> * Make sure that users of these arrays check that the mode is one of >> the mode you know about. That way if someone does add a new mode you >> don't index off your array. I'll put a comment in the user. > > Even if a new mode was added, the relative ordering of the existing modes > should not change and valid_modes_mask will only allow modes currently > supported by the driver. I'd like to keep the bound checks as simple as > possible. By explicitly sizing the arrays and then only checking for mode >> REGULATOR_MODE_STANDBY when indexing into the array we can be sure that > no out-of-bound access can ever occur. Also, if one of the existing mode > value was made larger than REGULATOR_MODE_STANDBY it would be easy to > catch as it would cause a compilation error. > > Thus, I'd prefer to keep the array sizing and index checking as-in unless > there is a major objection. OK. >>> +static unsigned int rpmh_regulator_pmic4_bob_of_map_mode(unsigned int mode) >>> +{ >>> + static const unsigned int of_mode_map[RPMH_REGULATOR_MODE_COUNT] = { >>> + [RPMH_REGULATOR_MODE_RET] = -EINVAL, >>> + [RPMH_REGULATOR_MODE_LPM] = REGULATOR_MODE_IDLE, >>> + [RPMH_REGULATOR_MODE_AUTO] = REGULATOR_MODE_NORMAL, >>> + [RPMH_REGULATOR_MODE_HPM] = REGULATOR_MODE_FAST, >>> + }; >> >> You're sticking a negative value in an array of unsigned inits. Here >> and in other similar functions. >> >> I know, I know. The function is defined to return an unsigned int. >> It's wrong. of_regulator.c clearly puts the return code in a signed >> int. First attempt at fixing this is at >> . > > I can change the error cases to use REGULATOR_MODE_INVALID which is added > by this change still under review [2]. I haven't seen Mark NAK it (yet), so for lack of a better option I'd start using it in your patch and document in the commit message that it depends on my patch. >>> +static const struct rpmh_vreg_hw_data pmic4_bob = { >>> + .regulator_type = VRM, >>> + .ops = &rpmh_regulator_vrm_bypass_ops, >>> + .voltage_range = REGULATOR_LINEAR_RANGE(1824000, 0, 83, 32000), >>> + .n_voltages = 84, >>> + .pmic_mode_map = pmic_mode_map_pmic4_bob, >>> + .of_map_mode = rpmh_regulator_pmic4_bob_of_map_mode, >>> + .bypass_mode = 0, >> >> Remove .bypass_mode from the structure and just change >> rpmh_regulator_vrm_set_mode_bypass() to set 0 if we're in bypass. >> Right now 100% of PMICs that support bypass use 0 as the bypass mode. >> If you ever have a future PMIC that uses a non-zero mode for bypass >> then we can always add this back. ...and if no future PMICs ever use >> a non-zero bypass mode then we don't need the complexity of having >> another field in this struct. >> >> If you don't do this you might get arguments from some people saying >> that they don't like seeing inits of "= 0" in static structures (Linux >> conventions seem to like you to just assume that structs are >> zero-initted). > > Upcoming PMICs use 2 for bypass mode instead of 0. That is why I left > this in. I suppose that I can remove this member for now and add it back > in when newer PMIC support is added. I'm OK with keeping it as long as there is a real user coming up. IMHO with the #defines as suggested by Matthias this will look better anyway (it will be more obvious that this isn't a boolean, for instance). >>> +cleanup: >>> + rpmh_release(rpmh_client); >> >> Still no devm_rpmh_get_client()? If Lina is too busy spinning her >> patch series for other stuff, just add it to RPMH as a patch in your >> series. I believe it's just this (untested): >> >> --- >> >> int devm_rpmh_release(struct device *dev, void *res) >> { >> struct platform_device *pdev = to_platform_device(dev); >> rpmh_release(pdev); >> } >> >> int devm_rpmh_get_client(struct device *dev) >> { >> struct platform_device *pdev = to_platform_device(dev); >> void *dr; >> int rc; >> >> dr = devres_alloc(devm_rpmh_release, 0, GFP_KERNEL); >> if (!dr) >> return -ENOMEM; >> >> rc = rpmh_get_client(pdev); >> if (!rc) >> devres_add(dev, dr); >> else >> devres_free(dr); >> >> return rc; >> } >> >> --- >> >> Note that you'll get rid of the error handling in probe, the whole >> remove function, and the need to do platform_set_drvdata(). > > My understanding is that Lina is going to remove both rpmh_get_client() > and rpmh_release(). In their place, rpmh functions will use the consumer > device pointer as a handle and manage any necessary state internally [3]. > I'll update this patch once she uploads a new series with this interface > modification. OK, sounds good. Information like this is nice to include somewhere in the cover letter or the patch description so it's more obvious that you wouldn't want this patch to land until that's sorted out. -Doug