Messages in this thread | | | Date | Thu, 29 May 2014 22:25:59 +0100 | From | Srinivas Kandagatla <> | Subject | Re: [PATCH 1/3] mfd: devicetree: bindings: Add Qualcomm RPM DT binding |
| |
On 29/05/14 19:38, Bjorn Andersson wrote: > On Thu, May 29, 2014 at 9:19 AM, Srinivas Kandagatla > <srinivas.kandagatla@linaro.org> 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. > Does not make sense. Unless Am missing something obvious. Having reg-names/interrupt-names would give driver flexibility to get the resources by name, as long as the order of reg and reg-names match.
So the order of reg is really not really necessary. Unless the driver is coded to access it via index.
Hardly 1/2 bindings documents enforce this.
>>> += 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. > Good point, Am more of thinking of some other SOCs might have similar pmic.
> > 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. Thats great, you have already thought about it. > > 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. > Am ok with either approaches.
>> >> [... >> >>> +- 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. > Ok. I don’t have a strong feeling on any of those 3 approaches.
Thanks, srini > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |