Received: by 10.192.165.148 with SMTP id m20csp754482imm; Fri, 20 Apr 2018 15:15:40 -0700 (PDT) X-Google-Smtp-Source: AIpwx49R4RfVoF8fypYEk3zBSi0Bg6OUE7yO0kCzxbdVRT003JEicShsC1RcKwRN96NYxd8TM5is X-Received: by 2002:a17:902:7007:: with SMTP id y7-v6mr11696732plk.227.1524262540735; Fri, 20 Apr 2018 15:15:40 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1524262540; cv=none; d=google.com; s=arc-20160816; b=z9k+G5lFo7hnuVLn9mD1dbARiWofyvdtNgLwmVS5+seu01uZ9TtH/Efc5oQYqNoajE 6d6bijdcja2lDRt7kJHtNHZpOD6++xQNY4zsrb0SFpmCeZhxlZZYBnFhGwx79E18+rf7 NVwRzWxFYUQpwSJ0843DnQnuVyEtRHjUTgazL2oUjCjGBWqL4VVnaZkyJWJ4jLx33NsA 7VaKxIlKgnIQt58B8CQ74Ub0YOK93lYMM+bclkxP6goUhc3fUX30bsZVEhTf9Gwak5Q1 05NA1vHlVlfOynG2qHy38/pe0QbCpNTIExXgg4QlnqGm+uPbKxkUnPCa2gKew0MZuVWf WqCA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject:dmarc-filter :dkim-signature:dkim-signature:arc-authentication-results; bh=xWP9f4ZijbeXfnpSEDgDhzhZnqdf/yHDVwDuGvLqeFk=; b=nMGuT/4YW+SmP1EZx/1oixF7Ot7JjLEjnKLMEZa4xxGzo6zzoKU5k2LL5cKu7PaHgG BwiaOE8WYmS8/gUWps6AM43x6SXtQPI+j9c+3DY/RXJVsVs6/ojEOxF0rpl0VM6yzb6y igz0NygSP6ex89RzGVzh9QVg/cFqfn3DNEKEDLTQ2vPvjeDQ/Ru/sj32FoVLindiNmRo hCdIf8uwBBbi4oQUyyKdFdQmIXmB1U4ETaz+OrrVjKjBjFlUnDxOkcgY2BMNgD1itscP CaJhAx8NW1xhfNrprQ+/luZd3sEqRE5lSaEhBMAGZtMLycyP0VkVYutypPafW9BupxcF qncA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@codeaurora.org header.s=default header.b=lZAQ622P; dkim=pass header.i=@codeaurora.org header.s=default header.b=ftiBM6e9; 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 Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id o24si379561pgn.36.2018.04.20.15.15.03; Fri, 20 Apr 2018 15:15:40 -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=pass header.i=@codeaurora.org header.s=default header.b=lZAQ622P; dkim=pass header.i=@codeaurora.org header.s=default header.b=ftiBM6e9; 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 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752844AbeDTWJE (ORCPT + 99 others); Fri, 20 Apr 2018 18:09:04 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:60766 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752792AbeDTWJA (ORCPT ); Fri, 20 Apr 2018 18:09:00 -0400 Received: by smtp.codeaurora.org (Postfix, from userid 1000) id BC37E60881; Fri, 20 Apr 2018 22:08:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1524262139; bh=D5KIkl+Rh5oqzaC4XQlxmjbJaBVuNfj5xhCk8rNG+bc=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=lZAQ622PRSfa8GTi1ZMub4jQKTkS82gL3VcwVN1HXNaWyxJqSkw7iCVWTAQYZGPBi Bk8pXJP11yYRzCLKUB9fFG6TGNbhjtPscHACRWimHjDOso2BL29ThZesSB1qdvkVMV QtM/B7fg23tEWiGIkmwio43TiIvCtAOn6pS0+PYA= X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on pdx-caf-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.8 required=2.0 tests=ALL_TRUSTED,BAYES_00, DKIM_SIGNED,T_DKIM_INVALID autolearn=no autolearn_force=no version=3.4.0 Received: from [10.46.164.143] (i-global254.qualcomm.com [199.106.103.254]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: collinsd@smtp.codeaurora.org) by smtp.codeaurora.org (Postfix) with ESMTPSA id 32A7260310; Fri, 20 Apr 2018 22:08:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1524262138; bh=D5KIkl+Rh5oqzaC4XQlxmjbJaBVuNfj5xhCk8rNG+bc=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=ftiBM6e9r2QlrFcxOV94kS8c0r/6Dj6VA1ixnY2bKiExtkMyAwwODYL/FC8h3sC9U k6ZCZa/onxcAmb2EnESjGOIkDqnAY/XLuhYZweSUQBpIYpDanJyQcFiMJBqZX4z0ls 9LA4j1cw32eS6FWDleBQlECw7ClKOoiWHu8evTTw= DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 32A7260310 Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=collinsd@codeaurora.org Subject: Re: [PATCH v2 2/2] regulator: add QCOM RPMh regulator driver To: Doug Anderson , Mark Brown Cc: 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 References: <4b45f41996ea3344340e44fab2dc9e487572e209.1523673467.git.collinsd@codeaurora.org> <4e3353fe-ebb5-46bb-aa58-49ad04c4d9db@codeaurora.org> From: David Collins Message-ID: <132ab845-52d6-6192-4d8c-5a9c95410688@codeaurora.org> Date: Fri, 20 Apr 2018 15:08:57 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/19/2018 09:16 AM, Doug Anderson wrote: > 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. Ok. I'll change this to be plural. >>>> + 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. I'm not opposed to your option #3 though it does seem a little hacky and tailored to the qcom_rpmh-regulator specific case. Note that I think it would be better to vote for min_uV to max_uV than min_uV to min_uV though. Mark, what are your thoughts on the best way to handle this situation? >>>> +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. Your patch was accepted. I'll switch to using REGULATOR_MODE_INVALID. Thanks, David -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project