Received: by 10.192.165.156 with SMTP id m28csp412292imm; Tue, 17 Apr 2018 12:16:49 -0700 (PDT) X-Google-Smtp-Source: AIpwx49CSPxWylNpcZzJKATWTy+IR1tRpF+f/6xAZHLGepEmRHfWnNMyYcmm926cItv+V0B0rDqi X-Received: by 10.98.236.86 with SMTP id k83mr3069372pfh.84.1523992609758; Tue, 17 Apr 2018 12:16:49 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1523992609; cv=none; d=google.com; s=arc-20160816; b=Iwq/cmTwvB/3tijaR86n3OICjXialvuF5cFdCS5NxIKxS0+F4lqfm1uWsYmc0PMHDX I3hctqI8S/eAX+U5qrP6r4UECbfd687wsOI/qyEz1fNoPjZHWQOe7N0RdESiHNW6pS6w C5fwbvCsUrad8tCcP9wtwafMn6YfzDMeaBTW1U1dpRQ4Xv1x8fEu1LwKvaUF1jwXTd1N y23ntP7eXtGNZ3unUlWMuJrh9452+KwlccFcG7FGHfJ/oNyhpyMXwo2wM2WySI+xjkYr gUN7OR3ka7QxKp3JsHx3acyKk5ue9shUhcUB++14hKwNHRJgF6pR75z/8m44rewqsEnV oQkw== 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=vYs/buEiObnCG6F1kMXknAEJnrLh9I42Veetq4VH13U=; b=oTpZv26DH4B1Aq6b1V5Zymp07i5wpV0aifjoWV+1GErQmLBUsBr9rwKaNFATb+I/Q7 4H3Att81M87OTf0WasmZ91hQ/F/XY3A0cZHvS3kHVZglbBZxE3oKIlyxdmFEg4uD0t2i dD5q7d49Gio8vRoO6awpRJ+rMQBql251rJs2cdPfV4H5u3jMLqF4dbYpg9eY1Z8syonL 6RtK7B/3cvClGKxKEOpa10lidyJk/X5dxodIVn9V35mxWF/DC39GiC+MDrfM9kw1BMcQ Am0tXPWevUjAsnu5U/nzbPnwZ6Q2fk324thKmKt4Fx46HUvNiytWGNpaR6DzGa5BazMp wnZw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@codeaurora.org header.s=default header.b=GyrNcvRi; dkim=pass header.i=@codeaurora.org header.s=default header.b=Okhb+4Zz; 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 a64si11433597pgc.265.2018.04.17.12.16.34; Tue, 17 Apr 2018 12:16:49 -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=GyrNcvRi; dkim=pass header.i=@codeaurora.org header.s=default header.b=Okhb+4Zz; 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 S1752608AbeDQTPX (ORCPT + 99 others); Tue, 17 Apr 2018 15:15:23 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:34374 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751836AbeDQTPV (ORCPT ); Tue, 17 Apr 2018 15:15:21 -0400 Received: by smtp.codeaurora.org (Postfix, from userid 1000) id D8C7260A4E; Tue, 17 Apr 2018 19:15:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1523992520; bh=QMbSCOL1h94KrabW8A442JWnydKDkGjlcaJSyF4Iuyo=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=GyrNcvRir81pIpNmWi01/JaAxQklvIL8E5LZmnfrrtJpDCWH1Aih7OIkuaNHMwPGf K5dPrlIbWpFhnYq8ovteX6WAvJ7X5TmA4ifDjRYmBhmRx5elV4lMcu77S31u1loYTu vboq2QGy7rTKuM1yMniG2bsPFxLqRgGpY3uAewRk= 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 3D04460264; Tue, 17 Apr 2018 19:15:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1523992519; bh=QMbSCOL1h94KrabW8A442JWnydKDkGjlcaJSyF4Iuyo=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=Okhb+4ZzdT7OA2WmlWb5Iwbo4xT98m5uKGLVgNvUmudpoo+WY01diZ880HkYIHAb5 uXa9VHFHWz8sFh8jLzQXvbDbv5bb/tXr7kdzD5vvJMKOEpdjOysDy1c/XYvChJZtlC GKh6/8RiXkMTwcE9xn+14eC07YPE70HEn2JDNa2I= DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 3D04460264 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: [v2,2/2] regulator: add QCOM RPMh regulator driver To: Matthias Kaehlcke Cc: broonie@kernel.org, lgirdwood@gmail.com, robh+dt@kernel.org, mark.rutland@arm.com, linux-arm-msm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, rnayak@codeaurora.org, sboyd@kernel.org, dianders@chromium.org References: <4b45f41996ea3344340e44fab2dc9e487572e209.1523673467.git.collinsd@codeaurora.org> <20180417182347.GD244487@google.com> From: David Collins Message-ID: <2e45c0e5-7e79-3479-de18-9613e8eacf15@codeaurora.org> Date: Tue, 17 Apr 2018 12:15:18 -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: <20180417182347.GD244487@google.com> 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/17/2018 11:23 AM, Matthias Kaehlcke wrote: > On Fri, Apr 13, 2018 at 07:50:35PM -0700, David Collins wrote: >> Add the QCOM RPMh regulator driver to manage PMIC regulators >> which are controlled via RPMh on some Qualcomm Technologies, Inc. >> SoCs. RPMh is a hardware block which contains several >> accelerators which are used to manage various hardware resources >> that are shared between the processors of the SoC. The final >> hardware state of a regulator is determined within RPMh by >> performing max aggregation of the requests made by all of the >> processors. >> [...] >> +/** >> + * struct rpmh_vreg_hw_data - RPMh regulator hardware configurations >> + * @regulator_type: RPMh accelerator type used to manage this >> + * regulator >> + * @ops: Pointer to regulator ops callback structure >> + * @voltage_range: The single range of voltages supported by this >> + * PMIC regulator type >> + * @n_voltages: The number of unique voltage set points defined >> + * by voltage_range >> + * @pmic_mode_map: Array indexed by regulator framework mode >> + * containing PMIC hardware modes. Must be large >> + * enough to index all framework modes supported >> + * by this regulator hardware type. >> + * @of_map_mode: Maps an RPMH_REGULATOR_MODE_* mode value defined >> + * in device tree to a regulator framework mode > > The name of the field is a bit misleading, this is a map of RPMh mode > to regulator framework mode, the device tree just happens to be the > place where this mapping is defined. of_map_mode name is used here to match the struct regulator_desc field by the same name that it is assigned to [1]. Do you think that the name should be changed to something else? >> +/** >> + * struct rpmh_vreg - individual rpmh regulator data structure encapsulating a >> + * single regulator device >> + * @rpmh_client: Handle used for rpmh communications > > nit: RPMh I'll change this. >> +struct rpmh_vreg { >> + struct rpmh_client *rpmh_client; >> + u32 addr; >> + struct regulator_desc rdesc; >> + const struct rpmh_vreg_hw_data *hw_data; >> + enum rpmh_regulator_type regulator_type; > > This value is already available via rpmh_vreg->hw_data->regulator_type, > why duplicate it? The field is assigned in rpmh_regulator_init_vreg() > and only read once in the same function, there seems to be no need for > it, not even to improve readability. This is present to specifically allow for a future change to support overriding the regulator_type value from device tree in order to force RPMh resources to be handled via XOB instead of VRM in a board-specific manner. I included support of the property qcom,rpmh-resource-type in the first version of this patch. I removed this property from the second version of the patch based upon review feedback since SDM845 does not explicitly need it (though an upcoming chip will). I'll remove regulator_type from struct rpmh_vreg. It shouldn't be particularly painful to add it back in when needed for XOB override support. >> +static int rpmh_regulator_vrm_set_load(struct regulator_dev *rdev, int load_uA) >> +{ >> + struct rpmh_vreg *vreg = rdev_get_drvdata(rdev); >> + int i; >> + >> + for (i = 0; i < vreg->drms_mode_count - 1; i++) >> + if (load_uA < vreg->drms_mode_max_uA[i]) > > Shouldn't this be '<='? > > nit: IMO 'vreg->drms_mode_max_uA[i] >= load_uA' would be more readable. I chose to use '<' here in order to maintain the non-inclusive limit semantics of the downstream RPMh regulator driver. E.g. with an LPM threshold of 10000 uA, load_uA == 10000 would result in a request for HPM instead of LPM. I suppose that I can change this to '<=' to be more logically consistent. >> +static const u32 pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = { >> + [REGULATOR_MODE_STANDBY] = 4, >> + [REGULATOR_MODE_IDLE] = 5, >> + [REGULATOR_MODE_NORMAL] = -EINVAL, >> + [REGULATOR_MODE_FAST] = 7, >> +}; > > Define constants for the modes on the PMIC4 side? Are you suggesting something like this? #define PMIC4_LDO_MODE_RETENTION 4 #define PMIC4_LDO_MODE_LPM 5 #define PMIC4_LDO_MODE_HPM 7 static const u32 pmic_mode_map_pmic4_ldo[REGULATOR_MODE_STANDBY + 1] = { [REGULATOR_MODE_STANDBY] = PMIC4_LDO_MODE_RETENTION, [REGULATOR_MODE_IDLE] = PMIC4_LDO_MODE_LPM, [REGULATOR_MODE_NORMAL] = -EINVAL, [REGULATOR_MODE_FAST] = PMIC4_LDO_MODE_HPM, }; #define PMIC4_SMPS_MODE_RETENTION 4 #define PMIC4_SMPS_MODE_PFM 5 #define PMIC4_SMPS_MODE_AUTO 6 #define PMIC4_SMPS_MODE_PWM 7 static const u32 pmic_mode_map_pmic4_smps[REGULATOR_MODE_STANDBY + 1] = { [REGULATOR_MODE_STANDBY] = PMIC4_SMPS_MODE_RETENTION, [REGULATOR_MODE_IDLE] = PMIC4_SMPS_MODE_PFM, [REGULATOR_MODE_NORMAL] = PMIC4_SMPS_MODE_AUTO, [REGULATOR_MODE_FAST] = PMIC4_SMPS_MODE_PWM, }; I considered using this approach, but it didn't seem like it increased readability and did increase the line count. Each of the constants would only be used once. Would you prefer this style (or something else)? Thanks, David [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/regulator/driver.h?h=v4.17-rc1#n370 -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project