Gah, ignore all the below. I drafted something up, went to something else, reviewed and sent to realize this was addressed already by others. Thanks, Andrew On Thu, Sep 08, 2022 at 09:50:38AM -0500, Andrew Halaney wrote: > On Thu, Sep 08, 2022 at 12:25:25PM +0200, Krzysztof Kozlowski wrote: > > On 07/09/2022 22:49, Andrew Halaney wrote: > > > For RPMH regulators it doesn't make sense to indicate > > > regulator-allow-set-load without saying what modes you can switch to, > > > so be sure to indicate a dependency on regulator-allowed-modes. > > > > > > In general this is true for any regulators that are setting modes > > > instead of setting a load directly, for example RPMH regulators. A > > > counter example would be RPM based regulators, which set a load > > > change directly instead of a mode change. In the RPM case > > > regulator-allow-set-load alone is sufficient to describe the regulator > > > (the regulator can change its output current, here's the new load), > > > but in the RPMH case what valid operating modes exist must also be > > > stated to properly describe the regulator (the new load is this, what > > > is the optimum mode for this regulator with that load, let's change to > > > that mode now). > > > > > > With this in place devicetree validation can catch issues like this: > > > > > > /mnt/extrassd/git/linux-next/arch/arm64/boot/dts/qcom/sm8350-hdk.dtb: pm8350-rpmh-regulators: ldo5: 'regulator-allowed-modes' is a dependency of 'regulator-allow-set-load' > > > From schema: /mnt/extrassd/git/linux-next/Documentation/devicetree/bindings/regulator/qcom,rpmh-regulator.yaml > > > > > > Where the RPMH regulator hardware is described as being settable, but > > > there are no modes described to set it to! > > > > > > Suggested-by: Johan Hovold <johan+kernel@xxxxxxxxxx> > > > Reviewed-by: Johan Hovold <johan+kernel@xxxxxxxxxx> > > > Reviewed-by: Douglas Anderson <dianders@xxxxxxxxxxxx> > > > Signed-off-by: Andrew Halaney <ahalaney@xxxxxxxxxx> > > > --- > > > > > > v2: https://lore.kernel.org/linux-arm-msm/20220906201959.69920-1-ahalaney@xxxxxxxxxx/ > > > Changes since v2: > > > - Updated commit message to explain how this is a property of the > > > hardware, and why it only applies to certain regulators like RPMH > > > (Johan + Krzysztof recommendation) > > > - Added Johan + Douglas' R-B tags > > > > You posted before we finished discussion so let me paste it here: > > > > The bindings don't express it, but the regulator core explicitly asks > > for set_mode with set_load callbacks in drms_uA_update(), which depends > > on REGULATOR_CHANGE_DRMS (toggled with regulator-allow-set-load). > > If I follow correctly it isn't asking for both, just either set_mode() > or set_load(): > > https://elixir.bootlin.com/linux/v6.0-rc4/source/drivers/regulator/core.c#L961 > copy pasta below > /* > * first check to see if we can set modes at all, otherwise just > * tell the consumer everything is OK. > */ > if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_DRMS)) { > rdev_dbg(rdev, "DRMS operation not allowed\n"); > return 0; > } > > if (!rdev->desc->ops->get_optimum_mode && > !rdev->desc->ops->set_load) > return 0; > > if (!rdev->desc->ops->set_mode && > !rdev->desc->ops->set_load) > return -EINVAL; > > I'm interpreting the if statements as: > > 1. Can the core set the load (as you highlighted REGULATOR_CHANGE_DRMS is > toggled by regulator-allow-set-load) for this hardware at all? > > 2. Are we able to determine the best mode to switch to, or can we > just set the load directly? If neither of those the core can't do > much > > 3. Can we set the mode we determined was > optimum with get_optimum_mode()? If the hardware is settable, and > the core can determine a new mode but there's no mode set_mode() > to actually switch that's an error unless we can just call > set_load() directly with our new current requirement > > That's a long winded way of saying I don't think the core asks for > set_mode && set_load callbacks to be implemented (which is how I > interpreted your message above). > > > > > drms_uA_update() later calls regulator_mode_constrain() which checks if > > mode changing is allowed (REGULATOR_CHANGE_MODE). > > If set_load() is implemented this is not checked and the load is set > directly before returning from drms_uA_update(). > https://elixir.bootlin.com/linux/v6.0-rc4/source/drivers/regulator/core.c#L973 > > > > > Therefore based on current implementation and meaning of > > set-load/allowed-modes properties, I would say that this applies to all > > regulators. I don't think that RPMh is special here. > > > > The above comments are why I don't think it applies to *all* regulators. > It does apply to any "mode based" regulator hardware though (which I > attempted to capture in the last paragraph in the commit message), but > unfortunately I do not know of a way to do the below pseudo check at a > regulator-wide binding level: > > if regulator-allow-set-load && !set_load() && set_mode() && !regulator-allowed-modes: > complain_about_invalid_devicetree() > > Basically, the bindings don't really indicate the ops hardware supports > so I can't think of a good way to key check the set_mode() and > !set_load() bits to catch this at a wider level, so I opted to just > attack the dt-binding at a hardware specific level since I can make > assumptions about what operations the hardware supports at that level. > > So, with this approach I do only plug the hole up for RPMh users, other > set_mode() users are still at risk. Other than duplicating this to those > users I can't really think of a generic way to tackle this at the > regulator.yaml level since I don't see a good way to grab the ops > supported. > > We could maybe add extra bindings to indicate what ops are > supported, i.e. regulator-set-load and regulator-set-mode, and then have > (hopefully this is possible in the dt-bindings) some if statements like: > > if (regulator-allow-set-load) { > if (regulator-set-load) > return 0; > else if (regulator-set-mode && !regulator-allowed-modes) > return -EINVAL; > else > return -EINVAL; > } > > But I'm not really sure how I feel about making each dt-binding specify > what ops their hardware supports. > > Regardless I think the current patch helps out RPMh users.. but I'm open > to extending it if we can come up with a good way to do it! > > Thanks, > Andrew > > > Best regards, > > Krzysztof > >
