From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.6 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_MUTT autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id F2929C31E5B for ; Mon, 17 Jun 2019 16:04:15 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id C7B97208E4 for ; Mon, 17 Jun 2019 16:04:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1560787455; bh=2JpCv3hokYrtHnxw4SCAykQxGffZf5FfbG19WlhkArg=; h=Date:From:To:Cc:Subject:References:In-Reply-To:List-ID:From; b=wiY7RndvUro2RNeGWK285Y0Ck1Hy3BjGc4e1OQ2RG+eLvdxFSatSmNVTf9jw5xXvX yK0US2JAam2pwRhlBCWMgSkHFrv05BSSnZK87Y7MjP34pk4q+YAO+k9+OM77mb0GKj C+ZzGLVq+uQGWET8JlNr6f+RCLuRycXd2XWKzK4M= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726292AbfFQQEL (ORCPT ); Mon, 17 Jun 2019 12:04:11 -0400 Received: from heliosphere.sirena.org.uk ([172.104.155.198]:33350 "EHLO heliosphere.sirena.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727936AbfFQQEE (ORCPT ); Mon, 17 Jun 2019 12:04:04 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sirena.org.uk; s=20170815-heliosphere; h=In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=CLbL6cpc9U7h527k8o/1MxF91Z/CtBA1frPv8iuvv78=; b=CzHAVvqyU0UCgx6eMsims7dJ2 21zkbdbIB6UEFSRRnA57/6BGsabwS5TzyehbKe065AwrEI+Ea8uu06M5wRo3ll3Jn6jhnbLpwF0Sl IOTHYtT2w6FjoIkYv1ipdWRbjt8XoDTPgR2Md3Qq2tadbnCiQxqlDjm0N7aPvyrwsu9LQ=; Received: from cpc102320-sgyl38-2-0-cust46.18-2.cable.virginm.net ([82.37.168.47] helo=finisterre.sirena.org.uk) by heliosphere.sirena.org.uk with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89) (envelope-from ) id 1hcu6x-000279-GA; Mon, 17 Jun 2019 16:03:59 +0000 Received: by finisterre.sirena.org.uk (Postfix, from userid 1000) id CBA86440046; Mon, 17 Jun 2019 17:03:58 +0100 (BST) Date: Mon, 17 Jun 2019 17:03:58 +0100 From: Mark Brown To: Jeffrey Hugo Cc: lgirdwood@gmail.com, Andy Gross , Bjorn Andersson , Rob Herring , Mark Rutland , MSM , lkml , devicetree@vger.kernel.org Subject: Re: [PATCH v4 4/7] regulator: qcom_spmi: Add support for PM8005 Message-ID: <20190617160358.GC5316@sirena.org.uk> References: <20190613212436.6940-1-jeffrey.l.hugo@gmail.com> <20190613212553.10541-1-jeffrey.l.hugo@gmail.com> <20190613212553.10541-2-jeffrey.l.hugo@gmail.com> <20190617150502.GU5316@sirena.org.uk> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="l8YfOjwMha7d9KWK" Content-Disposition: inline In-Reply-To: X-Cookie: Editing is a rewording activity. User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-arm-msm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org --l8YfOjwMha7d9KWK Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Mon, Jun 17, 2019 at 09:17:21AM -0600, Jeffrey Hugo wrote: > On Mon, Jun 17, 2019 at 9:05 AM Mark Brown wrote: > > > +static int spmi_regulator_ftsmps426_set_voltage(struct regulator_dev *rdev, > > > + unsigned selector) > > > +{ > > > + mV = spmi_regulator_common_list_voltage(rdev, selector) / 1000; > > This could just be a set_voltage_sel(), no need for it to be a > > set_voltage() operation.... > This is a set_voltage_sel() in spmi_ftsmps426_ops. Is the issue because this > function is "spmi_regulator_ftsmps426_set_voltage" and not > "spmi_regulator_ftsmps426_set_voltage_sel"? Well, that's certainly confusing naming and there's some confusion in the code about what a selector is - it's supposed to be a raw register value so if you're having to convert it into a voltage something is going wrong. Just implement a set_voltage() operation? > We already have code in the driver to convert a selector to the > voltage. Why duplicate > that inline in spmi_regulator_ftsmps426_set_voltage? Either work with selectors or work with voltages, don't mix and match the two. > > > + switch (mode) { > > > + case REGULATOR_MODE_NORMAL: > > > + val = SPMI_FTSMPS426_MODE_HPM_MASK; > > > + break; > > > + case REGULATOR_MODE_FAST: > > > + val = SPMI_FTSMPS426_MODE_AUTO_MASK; > > > + break; > > > + default: > > > + val = SPMI_FTSMPS426_MODE_LPM_MASK; > > > + break; > > > + } > > This should validate, it shouldn't just translate invalid values into > > valid ones. > Validate what? The other defines are REGULATOR_MODE_IDLE > and REGULATOR_MODE_STANDBY which correspond to the LPM > mode. Or are you suggesting that regulator framework is going to pass > REGULATOR_MODE_INVALID to this operation? You should be validating that the argument passed in is one that the driver understands, your assumption will break if we add any new modes and in any case there should be a 1:1 mapping between hardware and API modes so you shouldn't be translating two different API modes into the same hardware mode. --l8YfOjwMha7d9KWK Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAl0Hue0ACgkQJNaLcl1U h9BokQf/fWdp7er8/W74adT1KeOqQi4nPS93SO7dWKuu7q1YNud8ppBRNgmG3djL xaZh1dTIqVx2GICGfhZA3uEYPTsgPfNiM0ENiApp8OqfxX8VE5J6Ww01ikPQq08J Hgwb5piOsDD1uQWUr07Y7R3eoZeKmFCWBRAxUSdNSFBdbQy/v1Of2Yl98/ghbP7Z c8Au/mllEECB2Ew4rFXJXse1R19p0feZxxw2DQUPPB+AQ46TstNc487vUoSKNxJp 8D2BxD5Ph+8wT9lHUimBTNyNhBtRo8wiO0yXOe7xWR8w3rJxMUGe1jNJqFO4olqf KFAvYXEW+12HBtSq7ZaqNi5iCB/XJg== =GX4O -----END PGP SIGNATURE----- --l8YfOjwMha7d9KWK--