Re: [PATCH 1/3] mfd: devicetree: bindings: Add Qualcomm RPM DT binding

[Bjorn Andersson]

On Thu, May 29, 2014 at 9:19 AM, Srinivas Kandagatla
<srinivas.kandagatla@xxxxxxxxxx> wrote:
>> +- reg:
>> +       Usage: required
>> +       Value type: <prop-encoded-array>
>> +       Definition: two entries specifying the RPM's message ram and ipc
>> register
>> +
>> +- reg-names:
>> +       Usage: required
>> +       Value type: <string-array>
>> +       Definition: must contain the following, in order:
>> +                   "msg_ram"
>> +                   "ipc"
>
>
> +1 for kumar's comment.
>
> cant enforce the order here. should fix it in the driver.
>

Yes I can, this is as decided by the devicetree maintainers. The order
of e.g. reg and interrupts must be defined.

>> += SUBDEVICES
>> +
>> +The RPM exposes resources to its subnodes. The below bindings specify the
>> set
>> +of valid subnodes that can operate on these resources.
>
>
> Why should these devices be on sub nodes?
>
> Any reason not to implement it like this,
>
> rpm: rpm@108000 {
>         compatible = "qcom,rpm-msm8960";
>
>         reg = <0x108000 0x1000 0x2011008 0x4>;
>
>         interrupts = <0 19 0>, <0 21 0>, <0 22 0>;
>         interrupt-names = "ack", "err", "wakeup";
> };
>
> pm8921_s1: pm8921-s1 {
>         compatible = "qcom,rpm-pm8921-smps";
>
>         regulator-min-microvolt = <1225000>;
>         regulator-max-microvolt = <1225000>;
>         regulator-always-on;
>
>         qcom,rpm = <&rpm QCOM_RPM_PM8921_S1>;
>         qcom,switch-mode-frequency = <3200000>;
>         qcom,hpm-threshold = <100000>;
> };
>
> This would simplify the driver code too and handle the interface neatly then
> depending on device hierarchy.
> rpm would be a interface library to the clients. Makes the drivers more
> independent, and re-usable if we do this way.

The subnodes doesn't describe separate pieces of hardware but rather
pieces of the rpm, that's why they should live inside the rpm. There
will not be any re-use of these drivers outside having a rpm as
parent.

I do have some patches for family b, where I'm moving things around a
little bit in the implementation to be able to re-use child-devices
where that makes sense (clock implementation is the same for the two).
But that is implementation specific and does not affect the dt.


Implementation wise, having the individual subnodes as children in the
device model makes a lot of sense, as the children should be probed
when the rpm appears and when the rpm goes away it should bring down
all subnodes. If there was any power management it would be the same
thing.

So I think this makes for a cleaner implementation; all I need to do
is to call of_platform_populate at the end of the probe and in remove
I need to tell the children that they should go away. I do not need to
support any phandle based lookups and separate life cycle management.

>
> [...
>
>> +- qcom,force-mode-none:
>> +       Usage: optional (default if no other qcom,force-mode is specified)
>> +       Value type: <empty>
>> +       Defintion: indicates that the regulator should not be forced to
>> any
>> +                  particular mode
>> +
>> +- qcom,force-mode-lpm:
>> +       Usage: optional
>> +       Value type: <empty>
>> +       Definition: indicates that the regulator should be forced to
>> operate in
>> +                   low-power-mode
>> +
>> +- qcom,force-mode-auto:
>> +       Usage: optional (only available for 8960/8064)
>> +       Value type: <empty>
>> +       Definition: indicates that the regulator should be automatically
>> pick
>> +                   operating mode
>> +
>> +- qcom,force-mode-hpm:
>> +       Usage: optional (only available for 8960/8064)
>> +       Value type: <empty>
>> +       Definition: indicates that the regulator should be forced to
>> operate in
>> +                   high-power-mode
>> +
>> +- qcom,force-mode-bypass: (only for 8960/8064)
>> +       Usage: optional (only available for 8960/8064)
>> +       Value type: <empty>
>> +       Definition: indicates that the regulator should be forced to
>> operate in
>> +                   bypass mode
>> +
>
> ...]
>
> Probably qcom,force-mode:
>         Usage: optional.
>         Value type: <string>
>
>         Definition: must be one of:
>         "none"
>         "lpm"
>         "auto"
>         "hpm"
>         "bypass"
>
> Makes it much simpler, as they seems to be mutually exclusive. simillar
> comments apply to other bindings too.

Please see my answer to Kumar.


Thanks for the comments!

Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/