Re: [PATCH] mfd: devicetree: bindings: Add Qualcomm RPM regulator subnodes

[Stephen Boyd <sboyd@codeaurora.org>]

On 02/17/15 13:48, Bjorn Andersson wrote:
> On Fri, Feb 13, 2015 at 2:13 PM, Andy Gross <agross@codeaurora.org> wrote:
>> On Thu, Jan 29, 2015 at 05:51:06PM -0800, Bjorn Andersson wrote:
>>> Add the regulator subnodes to the Qualcomm RPM MFD device tree bindings.
>>>
>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com>
>>> ---
>>>       #include <dt-bindings/mfd/qcom-rpm.h>
>>> @@ -66,5 +237,18 @@ frequencies.
>>>
>>>               #address-cells = <1>;
>>>               #size-cells = <0>;
>>> +
>>> +             pm8921_smps1: pm8921-smps1 {
>>> +                     compatible = "qcom,rpm-pm8921-smps";
>>> +                     reg = <QCOM_RPM_PM8921_SMPS1>;
>>> +
>>> +                     regulator-min-microvolt = <1225000>;
>>> +                     regulator-max-microvolt = <1225000>;
>>> +                     regulator-always-on;
>>> +
>>> +                     bias-pull-down;
>>> +
>>> +                     qcom,switch-mode-frequency = <3200000>;
>>> +             };
>>>       };
>> My only comment here is that most (all but one) of the other mfd regulator
>> devices use regulators {}.  Still wonder if that's what we should do.
>>
> Looking at the existing mfds they all have a list in the code of the
> regulators supported on a certain mfd. Through the use of
> regulator_desc.{of_match,regulators_node} these regulators are
> populated with properties from of_nodes matched by name (of_match)
> under the specified node (regulators_node).
> But as we've discussed in other cases it's not desirable to maintain
> these lists for the various variants of Qualcomm platforms, so I did
> choose to go with "standalone" platform devices - with matching
> through compatible and all.
>
> But that's all implementation, looking at the binding itself a
> regulator {} (clocks{} and msm_bus{}) would serve as a sort of
> grouping of children based on type. Except for the implications this
> has on the implementation I don't see much benefit of this (and in our
> case the implementation would suffer from the extra grouping).
>
>
> Let me know what you think, I based these ideas on just reading the
> existing code and bindings, and might very well have missed something.
>

The main benefit I can think of is we cut down on runtime memory bloat.

(gdb) p sizeof(struct platform_device)
$1 = 624

Multiply that by 20 regulators and you get 624 * 20 = 12480 bytes in
platform devices. If we had one platform_device for all RPM controlled
regulators that would reduce this number from ~12k to ~0.5K. It would
require of_regulator_match() and the (undesirable) lists of regulator
names for all the different pmic variants, or we would need to pick out
the regulator nodes based on compatible matching. Is that so bad? In the
other cases we were putting lots of data in the driver purely for
debugging, whereas in this case we're doing it to find nodes that we
need to hook up with regulators in software and any associated data for
that regulator.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project