| Message ID | cover.1527040878.git.collinsd@codeaurora.org |
|---|---|
| Headers | show |
| Series | regulator: add QCOM RPMh regulator driver | expand |
Hi, On Tue, May 22, 2018 at 7:43 PM, David Collins <collinsd@codeaurora.org> wrote: > + * @ever_enabled: Boolean indicating that the regulator has been > + * explicitly enabled at least once. Voltage > + * requests should be cached when this flag is not > + * set. Do you really need this extra boolean? Can't you just check if "enabled" is still "-EINVAL"? If it is then you don't pass the voltage along. ...this would mean that you'd also need to send the voltage vote when the regulator core tries to disable unused regulators at the end of bootup, but that should be OK right? If we never touched a regulator anywhere at probe time and we're about to vote to disable it, we know there's nobody requiring it to still be on. We can vote for the voltage now without fear of messing up a vote that the BIOS left in place. In theory this should also allow you to assert your vote about the voltage of a regulator that has never been enabled, which (if I understand correctly) you consider to be a feature. --- Other than that comment, everything else here looks good to go if Mark is good with it. As per the previous threads there are some things that I don't like a ton, but I feel it is between you and Mark to decide if you're good with it. A summary of those issues are: 1. I still believe that when we disable a regulator in Linux we should tell RPMh that we vote for the lowest voltage. ...but if Mark is happy with the way the driver works now I won't push it. 2. As per my comments in the bindings patch, I still believe that "qcom,drms-mode-max-microamps" belongs in the core. Again, up to Mark. 3. As per my comments in the bindings patch, I still believe that we're just adding lots of noise to the DT most of the time by specifying both "qcom,regulator-drms-modes" and "regulator-allowed-modes". Again, up to Mark. -Doug -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 22, 2018 at 07:43:16PM -0700, David Collins wrote: > This patch series adds a driver and device tree binding documentation for > PMIC regulator control via Resource Power Manager-hardened (RPMh) on some > Qualcomm Technologies, Inc. SoCs such as SDM845. RPMh is a hardware block So, this is a very big driver and obviously it being RPM based it doesn't look like other regulators which is causing problems, especially when coupled with the desire to implement a bunch of more exotic features like the mode setting. I think this review is going to go a lot more smoothly if you split this up into a base driver with just normal, standard stuff that doesn't add too many custom properties or unusual ways of working and then a series of patches on top of that adding things like the mode adjustment and interaction with other RPM clients. We've got other RPM based regulators in tree already so the baseline bit shouldn't be too hard, that'll make the rest of the patches much smaller and easier to review and mean that the bits that are simpler and easier to cope with don't need to be reposted.
Hello Doug, On 05/29/2018 10:32 PM, Doug Anderson wrote: > On Tue, May 22, 2018 at 7:43 PM, David Collins <collinsd@codeaurora.org> wrote: >> + * @ever_enabled: Boolean indicating that the regulator has been >> + * explicitly enabled at least once. Voltage >> + * requests should be cached when this flag is not >> + * set. > > Do you really need this extra boolean? Can't you just check if > "enabled" is still "-EINVAL"? If it is then you don't pass the > voltage along. > > ...this would mean that you'd also need to send the voltage vote when > the regulator core tries to disable unused regulators at the end of > bootup, but that should be OK right? If we never touched a regulator > anywhere at probe time and we're about to vote to disable it, we know > there's nobody requiring it to still be on. We can vote for the > voltage now without fear of messing up a vote that the BIOS left in > place. > > In theory this should also allow you to assert your vote about the > voltage of a regulator that has never been enabled, which (if I > understand correctly) you consider to be a feature. Removing 'ever_enabled' and caching the voltage when 'enabled == -EINVAL' seems workable. I'm a little concerned about this resulting in voltage = regulator-min-microvolt requests being sent for all regulators that are not explicitly enabled by Linux consumers before late_initcall_sync(). Theoretically all of the boot loader hand-off cases should be taken care of by this point so it should be safe. I'll make this change. Take care, David
Hello Mark, On 05/30/2018 09:33 AM, Mark Brown wrote: > On Tue, May 22, 2018 at 07:43:16PM -0700, David Collins wrote: >> This patch series adds a driver and device tree binding documentation for >> PMIC regulator control via Resource Power Manager-hardened (RPMh) on some >> Qualcomm Technologies, Inc. SoCs such as SDM845. RPMh is a hardware block > > So, this is a very big driver and obviously it being RPM based it > doesn't look like other regulators which is causing problems, especially > when coupled with the desire to implement a bunch of more exotic > features like the mode setting. I think this review is going to go a > lot more smoothly if you split this up into a base driver with just > normal, standard stuff that doesn't add too many custom properties or > unusual ways of working and then a series of patches on top of that > adding things like the mode adjustment and interaction with other RPM > clients. > > We've got other RPM based regulators in tree already so the baseline bit > shouldn't be too hard, that'll make the rest of the patches much smaller > and easier to review and mean that the bits that are simpler and easier > to cope with don't need to be reposted. I'll split up the patches so that reviewing is easier. For the base patch, would you prefer that I remove *all* mode support (handled by generic regulator framework DT properties) or only remove the special purpose drms mode handling support (i.e. qcom,regulator-drms-modes and qcom,drms-mode-max-microamps)? Thanks, David
On Wed, May 30, 2018 at 05:11:54PM -0700, David Collins wrote: > I'll split up the patches so that reviewing is easier. For the base > patch, would you prefer that I remove *all* mode support (handled by > generic regulator framework DT properties) or only remove the special > purpose drms mode handling support (i.e. qcom,regulator-drms-modes and > qcom,drms-mode-max-microamps)? Standard operations should be fine, just weird custom stuff that's been causing issues.