Received: by 10.213.65.68 with SMTP id h4csp569069imn; Tue, 27 Mar 2018 04:57:58 -0700 (PDT) X-Google-Smtp-Source: AG47ELsly+QgCgxnjyQ8cACdQj2Xml8bzmtVAU8DNxMhHZN3eeyfEU+QrY6kenuunQGkhDvyd4y2 X-Received: by 10.98.133.86 with SMTP id u83mr36545259pfd.172.1522151878426; Tue, 27 Mar 2018 04:57:58 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1522151878; cv=none; d=google.com; s=arc-20160816; b=twLdXUs7Gz2jab32fkrCdwdkvuT26+pQh5wo1vZT+SlzAgn8KJ5dLn86IXE7NC9IrK gVsJ4DlnHa+W9JTHa3f6P55yAntIBi58e+qw+Ht7tY0O6qy3EnpoDBpQuj62FCSQLxlF R61QU1c9eY5j17zLrAMUg0oLXKW0Rv8kiOAgDZvz5n4QVjRUBsKcayaeZ4cF1WWSEdo0 2I7bpAFOwwVnESzjjBqL6TycmcUHfuw8G6VqeJ55Bbf2XwLMi6OGTPqFSNjjl/H7yCHr 5JDkIN/YhgLH62tNSOdlVgbHkUB53uK5lH5AtU0UIMgglJVz900fzrEDqzkd1DOAGqkM pMuw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:dkim-signature:arc-authentication-results; bh=2b+563WENqgjJvogkeeLzjgq9wZ63rIYQBKjLUMV2HE=; b=LXu3nvQgxpyD8wZRg/lGfk+l+O37b6mVrO8UNaWIdlfZiraiDRqqCOL2Tovm1okDVd AdfpqsmW7wwd+r/503FqInKgh3ORhv9duLBbFAdRkus9OrHGfCHNBbdHvXURFyMmwZJ5 tuDWCwujiQt29czgGAku5QSi60ZeGlJzfYH3L35D9iJZU9FutwBAHlPD2QFGmNOU8Dy1 0OwQ4CFXHw/dOddsRS4hppP3LK9KoOXzan9zu5yj8HGRT6REqpIAtH5IW4MLY0dPgh59 ZWssDzQ/yFpoUND5XnUosoUIFSwPzvouEshJv0dau2+5uIlcxZQ6yXn8+AXowGQXqh+O l6WQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@sirena.org.uk header.s=20170815-heliosphere header.b=PHOexrz+; 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 c9si842741pfj.226.2018.03.27.04.57.44; Tue, 27 Mar 2018 04:57:58 -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=@sirena.org.uk header.s=20170815-heliosphere header.b=PHOexrz+; 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 S1751973AbeC0L4f (ORCPT + 99 others); Tue, 27 Mar 2018 07:56:35 -0400 Received: from heliosphere.sirena.org.uk ([172.104.155.198]:38234 "EHLO heliosphere.sirena.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751096AbeC0L4c (ORCPT ); Tue, 27 Mar 2018 07:56:32 -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=2b+563WENqgjJvogkeeLzjgq9wZ63rIYQBKjLUMV2HE=; b=PHOexrz+wZQH0Yt8PhBtZ02LT KAs1ogk8uE3R4T41trN+12hih29KtbCkb1o4o8yfN1a6aRr+XbowuU85X2pYWZPoYUQyKpFUcGrLF 3F1HxJgJp2oz7JDzWHt6hy3tNTEsQMYDJCgCxDkH+01W1jW6vAM5/3yRNkDFCXhTBANGU=; Received: from [202.155.252.3] (helo=finisterre.ee.mobilebroadband) by heliosphere.sirena.org.uk with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89) (envelope-from ) id 1f0nDA-0006qY-06; Tue, 27 Mar 2018 11:56:21 +0000 Received: by finisterre.ee.mobilebroadband (Postfix, from userid 1000) id CBB9644007A; Tue, 27 Mar 2018 12:56:06 +0100 (BST) Date: Tue, 27 Mar 2018 19:56:06 +0800 From: Mark Brown To: Doug Anderson Cc: David Collins , Liam Girdwood , Rob Herring , Mark Rutland , linux-arm-msm@vger.kernel.org, Linux ARM , devicetree@vger.kernel.org, LKML , Rajendra Nayak , sboyd@kernel.org, ilina@codeaurora.org Subject: Re: [PATCH 1/2] regulator: add QCOM RPMh regulator driver Message-ID: <20180327115606.GC29239@sirena.org.uk> References: <71fab82672524b95632cdb588c16edfc9711866a.1521246069.git.collinsd@codeaurora.org> <184378e4-caf8-6ce3-e089-3690588fcb28@codeaurora.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="TYecfFk8j8mZq+dy" Content-Disposition: inline In-Reply-To: X-Cookie: Serenity through viciousness. User-Agent: Mutt/1.9.4 (2018-02-28) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --TYecfFk8j8mZq+dy Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Mar 23, 2018 at 01:00:47PM -0700, Doug Anderson wrote: > On Thu, Mar 22, 2018 at 3:31 PM, David Collins = wrote: > > There are two cases that I can think of: 1. Having a set_voltage() > > callback allows for delaying for an RPMh request ACK during certain > > voltage set point decreasing scenarios (to be elaborated below). > Can't you still have a delay in set_voltage_sel()? We have specific support for adding ramp delays, no need to open code it in operations. > > 2. > > Having a get_voltage() as opposed to get_voltage_sel() callback allows = an > > uninitialized voltage of 0 to be returned in the case that no initial > > voltage is specified in device tree. This eliminates the possibility of > > the regulator framework short-circuiting a regulator_set_voltage() call > > that happens to match the voltage corresponding to selector 0. > Interesting. I suppose you could mix them (have set_voltage_sel() and > get_voltage()) as long as you documented why you were doing it. Then > we'd have to see if Mark was happy with that... This is a *terrible* idea which will almost certainly break. If the driver can't read values it should return an appropriate error code. > > I'm aware of one important instance in which decreasing voltage needs a > > delay: SD card voltage change from 3.3 V to 1.8 V. This is the use case > > that I had in mind with the 'max_uv < vreg->voltage' check. However, y= ou > > are correct that hardware will report completion of the voltage slewing > > early in this case since the comparator is checking for output >=3D set > > point. I can remove this special case check. You can't usefully wait for voltages to fall, you can never guarantee what the loading on the device is. It's something the user has to manage if they care. > > Here is an explanation for why the "device tree mode" abstraction is > > present in the first place. Between different Qualcomm Technologies, I= nc. > > PMICs, regulators support a subset of the same modes (HPM/PWM, AUTO, > > LPM/PFM, Retention, and pass-through). However, the register values for > > the same modes vary between different regulator types and different PMIC > > families. This patch is adding support for several PMIC4 family PMICs. > > The values needed for to-be-released PMIC5 PMIC regulators are differen= t. > > As an example, here are the different values for LPM/PFM across PMIC > > families and regulator types: PMIC4 LDO/SMPS =3D 5, PMIC4 BOB =3D 1, PM= IC5 > > LDO/HFSMPS/BOB =3D 4, PMIC5 FTSMPS =3D N/A. Having the "device tree mo= de" > > ensures that it is not possible to inadvertently specify a PMIC specific > > mode in device tree which corresponds to the wrong type or family but > > which aliases a value that would be accepted as correct. > I'm OK with having the "device tree mode" abstraction, and in fact the > current regulator framework seems to want you to have this anyway. If > I read the code correctly, you're required to have the conversion > function and there's no default. I didn't spot this in the code but something called "device tree mode" sounds like it's going to be awfully confusing... > > Pass-through mode (PASS) a.k.a. bypass is supported by Qualcomm > > Technologies, Inc. buck-or-boost regulators (BOB). When PASS is select= ed, > > the BOB output is directly connected to the BOB input (typically Vbat). =2E.. > > qcom_rpmh-regulator substantially simpler. I suppose that BOB PASS mode > > could be handled via get_bypass() and set_bypass() regulator ops. Doing > > this would require more complicated ops selections in the driver since = it This is exactly the functionality supported by the bypass operations. Any complexity due to the hardware design is unfortunate but honestly the way the QC regulator stuff is designed they seem like a bit of a lost cause on that front - they look very different to any other hardware we've seen. > I've never poked at the get_bypass() / set_bypass(), but it sounds > better to me to use them. I'm not a fan of the current hack. Even > aside from the bit of hackiness, I'm slightly concerned that some of > your logic that generally assumes lower integers =3D lower power modes > would break. Yes, abusing the framework is just going to make things even worse. =20 > >>> +arch_initcall(rpmh_regulator_init); > >> I always get yelled at when I try to use arch_initcall() for stuff > >> like this. You should do what everyone else does and use > > I agree that consumers should handle probe deferral. Unfortunately, > > reality isn't always so nice. Also, probe deferrals increase boot-up t= ime > > which is particularly problematic in the mobile space. > Sigh, yeah. I'm not a fan either. If you can convince Mark that you > should use arch_initcall() or subsys_initcall() I won't yell. ...but > in the past I've seen others get yelled at. Do you have concrete consumers that have a good reason for doing this? > Note: in actuality it doesn't always increase boot time a whole lot. Note also that we now have the device dependency mechanism that Raphael implemented with the explicit idea that that it'd be used to avoid unneeded deferrals. > ...but deferrals _do_ for sure increase the time for certain > peripherals to come up, and if those peripherals are things like the > LCD displays then it sucks. There's been some discussion of allowing the user to specific certain devices to target as priorities for probing which should deal with that. --TYecfFk8j8mZq+dy Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAlq6MVYACgkQJNaLcl1U h9Bsdwf+KMYIgw/6oBWCQZX5vlrCC26SOPyo9MOVD2aeEmVMFjCfmIiU6LVcz1lK BnCuPp9gdkvu3OLkpVuyC0RV69yZW84L719ORCy8+XHCTohfmOvEaCJT+Q5rz7Mk bZJzf4JctZNPRlPISJ34Equ2wxzTVNYn/FNPD+jEOXB7TAVeCcMdf/yPROhsC49+ sd115Pze3DUD4lX1XfaXQQ5HGgPOagnCPK9VsawyJMASmg8EAedsPPohLTcCQr+N N5Kueh5P25jn5oUtzHxebvJtbYpLe/120HsmGxjSTb9rpQ3Gk72pQpt9XaxoAnms 6Ia1zmSAYHlEJ5svSuERClhY+OUlVA== =0ErK -----END PGP SIGNATURE----- --TYecfFk8j8mZq+dy--