| Message ID | 1394047776-13827-7-git-send-email-joshc@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 03/05/14 11:29, Josh Cartwright wrote: > diff --git a/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt b/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt > new file mode 100644 > index 0000000..699bd30 > --- /dev/null > +++ b/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt > @@ -0,0 +1,29 @@ > +* Real-Time Clock for Qualcomm 8058/8921 PMICs > + > +Required properties: > +- compatible: should be one of the following. > + * "qcom,pm8058-rtc" > + * "qcom,pm8921-rtc" > +- reg: base address of the register region > +- reg-names: corresponding reg names for the regions listed in the 'reg' > + property, must contain: > + "rtc_base" - base of the RTC register region optional reg-names? > +- interrupts: interrupt list for the RTC, must contain a single interrupt > + specifier for the alarm interrupt > +- interrupt-names: corresponding interrupt names for the interrupts listed in > + the 'interrupts' property, must contain: > + "alarm" - summary interrupt for PMIC peripherals optional interrupt-names? > + > +Option properties: > +- linux,allow-set-time: indicates that the setting of RTC time is allowed by > + the host CPU Is this a "linux" property? It seems like something that other OSes would want to know about and doesn't require any explicit knowledge about operating system things (like keymaps for example). > + > +Example: > + > + rtc { rtc@11d > + compatible = "qcom,pm8921-rtc"; > + reg = <0x11D>; > + reg-names = "rtc_base"; > + interrupts = <0x27 0>; > + interrupt-names = "alarm"; > + };
On Wed, Mar 05, 2014 at 12:58:55PM -0800, Stephen Boyd wrote: > On 03/05/14 11:29, Josh Cartwright wrote: > > diff --git a/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt b/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt > > new file mode 100644 > > index 0000000..699bd30 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt > > @@ -0,0 +1,29 @@ > > +* Real-Time Clock for Qualcomm 8058/8921 PMICs > > + > > +Required properties: > > +- compatible: should be one of the following. > > + * "qcom,pm8058-rtc" > > + * "qcom,pm8921-rtc" > > +- reg: base address of the register region > > +- reg-names: corresponding reg names for the regions listed in the 'reg' > > + property, must contain: > > + "rtc_base" - base of the RTC register region > > optional reg-names? > > > +- interrupts: interrupt list for the RTC, must contain a single interrupt > > + specifier for the alarm interrupt > > +- interrupt-names: corresponding interrupt names for the interrupts listed in > > + the 'interrupts' property, must contain: > > + "alarm" - summary interrupt for PMIC peripherals > > optional interrupt-names? It isn't clear to me why these should be made optional, I hope Rob provides some clarification in the sdhci-msm thread. > > + > > +Option properties: Woops, this should be "Optional" :). > > +- linux,allow-set-time: indicates that the setting of RTC time is allowed by > > + the host CPU > > Is this a "linux" property? It seems like something that other OSes > would want to know about and doesn't require any explicit knowledge > about operating system things (like keymaps for example). Yeah, I wasn't quite sure how to name this property. It's Linux-specific in the sense that the underlying operation method is "set_time", but I agree this should be named something else... How do you feel about simply "allow-set-time"? > > > + > > +Example: > > + > > + rtc { > > rtc@11d Yep, thanks. > > + compatible = "qcom,pm8921-rtc"; > > + reg = <0x11D>; > > + reg-names = "rtc_base"; > > + interrupts = <0x27 0>; > > + interrupt-names = "alarm"; > > + }; > >
On 03/05/14 16:00, Josh Cartwright wrote: > On Wed, Mar 05, 2014 at 12:58:55PM -0800, Stephen Boyd wrote: >> On 03/05/14 11:29, Josh Cartwright wrote: >>> diff --git a/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt b/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt >>> new file mode 100644 >>> index 0000000..699bd30 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt >>> @@ -0,0 +1,29 @@ >>> +* Real-Time Clock for Qualcomm 8058/8921 PMICs >>> + >>> +Required properties: >>> +- compatible: should be one of the following. >>> + * "qcom,pm8058-rtc" >>> + * "qcom,pm8921-rtc" >>> +- reg: base address of the register region >>> +- reg-names: corresponding reg names for the regions listed in the 'reg' >>> + property, must contain: >>> + "rtc_base" - base of the RTC register region >> optional reg-names? >> >>> +- interrupts: interrupt list for the RTC, must contain a single interrupt >>> + specifier for the alarm interrupt >>> +- interrupt-names: corresponding interrupt names for the interrupts listed in >>> + the 'interrupts' property, must contain: >>> + "alarm" - summary interrupt for PMIC peripherals >> optional interrupt-names? > It isn't clear to me why these should be made optional, I hope Rob > provides some clarification in the sdhci-msm thread. Looks like the driver isn't using either of these properties, so I'm not sure why they're needed. Maybe they should just be removed. > > >>> +- linux,allow-set-time: indicates that the setting of RTC time is allowed by >>> + the host CPU >> Is this a "linux" property? It seems like something that other OSes >> would want to know about and doesn't require any explicit knowledge >> about operating system things (like keymaps for example). > Yeah, I wasn't quite sure how to name this property. It's > Linux-specific in the sense that the underlying operation method is > "set_time", but I agree this should be named something else... > > How do you feel about simply "allow-set-time"? Sounds ok to me.
On Wed, Mar 05, 2014 at 05:31:27PM -0800, Stephen Boyd wrote: > On 03/05/14 16:00, Josh Cartwright wrote: > > On Wed, Mar 05, 2014 at 12:58:55PM -0800, Stephen Boyd wrote: > >> On 03/05/14 11:29, Josh Cartwright wrote: > >>> diff --git a/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt b/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt > >>> new file mode 100644 > >>> index 0000000..699bd30 > >>> --- /dev/null > >>> +++ b/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt > >>> @@ -0,0 +1,29 @@ > >>> +* Real-Time Clock for Qualcomm 8058/8921 PMICs > >>> + > >>> +Required properties: > >>> +- compatible: should be one of the following. > >>> + * "qcom,pm8058-rtc" > >>> + * "qcom,pm8921-rtc" > >>> +- reg: base address of the register region > >>> +- reg-names: corresponding reg names for the regions listed in the 'reg' > >>> + property, must contain: > >>> + "rtc_base" - base of the RTC register region > >> optional reg-names? > >> > >>> +- interrupts: interrupt list for the RTC, must contain a single interrupt > >>> + specifier for the alarm interrupt > >>> +- interrupt-names: corresponding interrupt names for the interrupts listed in > >>> + the 'interrupts' property, must contain: > >>> + "alarm" - summary interrupt for PMIC peripherals > >> optional interrupt-names? > > It isn't clear to me why these should be made optional, I hope Rob > > provides some clarification in the sdhci-msm thread. > > Looks like the driver isn't using either of these properties, so I'm not > sure why they're needed. Maybe they should just be removed. The driver does make use of platform_get_irq_byname(pdev, "alarm"), and I expect to make use of platform_get_resource_byname(pdev, IORESOURCE_REG, "rtc_base") in the near future.
On Wed, Mar 5, 2014 at 6:00 PM, Josh Cartwright <joshc@codeaurora.org> wrote: > On Wed, Mar 05, 2014 at 12:58:55PM -0800, Stephen Boyd wrote: >> On 03/05/14 11:29, Josh Cartwright wrote: >> > diff --git a/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt b/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt >> > new file mode 100644 >> > index 0000000..699bd30 >> > --- /dev/null >> > +++ b/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt >> > @@ -0,0 +1,29 @@ >> > +* Real-Time Clock for Qualcomm 8058/8921 PMICs >> > + >> > +Required properties: >> > +- compatible: should be one of the following. >> > + * "qcom,pm8058-rtc" >> > + * "qcom,pm8921-rtc" >> > +- reg: base address of the register region >> > +- reg-names: corresponding reg names for the regions listed in the 'reg' >> > + property, must contain: >> > + "rtc_base" - base of the RTC register region >> >> optional reg-names? >> >> > +- interrupts: interrupt list for the RTC, must contain a single interrupt >> > + specifier for the alarm interrupt >> > +- interrupt-names: corresponding interrupt names for the interrupts listed in >> > + the 'interrupts' property, must contain: >> > + "alarm" - summary interrupt for PMIC peripherals >> >> optional interrupt-names? > > It isn't clear to me why these should be made optional, I hope Rob > provides some clarification in the sdhci-msm thread. Because reg and interrupt names are relatively new and reluctantly added by DT maintainers. Personally, I think it was a mistake and it is simply Linux specific information leaking into the DT, but it did make transition to DT easier. The requirement is still the ordering of reg and interrupts fields must be defined and you cannot rely on the names to define the order. It is quite pointless here since you only have 1 field. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hey Rob- Thanks for the reply. On Mon, Mar 10, 2014 at 10:35:25AM -0500, Rob Herring wrote: > On Wed, Mar 5, 2014 at 6:00 PM, Josh Cartwright <joshc@codeaurora.org> wrote: > > On Wed, Mar 05, 2014 at 12:58:55PM -0800, Stephen Boyd wrote: > >> On 03/05/14 11:29, Josh Cartwright wrote: > >> > +- interrupts: interrupt list for the RTC, must contain a single interrupt > >> > + specifier for the alarm interrupt > >> > +- interrupt-names: corresponding interrupt names for the interrupts listed in > >> > + the 'interrupts' property, must contain: > >> > + "alarm" - summary interrupt for PMIC peripherals > >> > >> optional interrupt-names? > > > > It isn't clear to me why these should be made optional, I hope Rob > > provides some clarification in the sdhci-msm thread. > > Because reg and interrupt names are relatively new and reluctantly > added by DT maintainers. Personally, I think it was a mistake and it > is simply Linux specific information leaking into the DT, but it did > make transition to DT easier. I don't necessarily buy the Linux-specific argument in general. If a devices' datasheet clearly gives names to register regions and interrupts, what about reflecting these names in the bindings is Linux-specific? Now, there are probably abuses of this, where the reg-names and interrupt-names are abused to ensure driver compatibility with devices described in board files, and only in that case will I agree is Linux-specific and should be strongly discouraged. > The requirement is still the ordering of reg and interrupts fields > must be defined and you cannot rely on the names to define the order. Should this requirement also exist for other <foo>-names properties? > It is quite pointless here since you only have 1 field. Indeed in the interrupt case it is worthless, as there is only one alarm interrupt. However for registers I do plan to extend this binding in the future to document a newer RTC which does split registers across multiple named address regions. Thanks again, Josh
diff --git a/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt b/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt new file mode 100644 index 0000000..699bd30 --- /dev/null +++ b/Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt @@ -0,0 +1,29 @@ +* Real-Time Clock for Qualcomm 8058/8921 PMICs + +Required properties: +- compatible: should be one of the following. + * "qcom,pm8058-rtc" + * "qcom,pm8921-rtc" +- reg: base address of the register region +- reg-names: corresponding reg names for the regions listed in the 'reg' + property, must contain: + "rtc_base" - base of the RTC register region +- interrupts: interrupt list for the RTC, must contain a single interrupt + specifier for the alarm interrupt +- interrupt-names: corresponding interrupt names for the interrupts listed in + the 'interrupts' property, must contain: + "alarm" - summary interrupt for PMIC peripherals + +Option properties: +- linux,allow-set-time: indicates that the setting of RTC time is allowed by + the host CPU + +Example: + + rtc { + compatible = "qcom,pm8921-rtc"; + reg = <0x11D>; + reg-names = "rtc_base"; + interrupts = <0x27 0>; + interrupt-names = "alarm"; + };
This RTC is found on the Qualcomm 8921 and 8058 PMICs. Signed-off-by: Josh Cartwright <joshc@codeaurora.org> --- .../devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt | 29 ++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 Documentation/devicetree/bindings/rtc/qcom,pm8xxx-rtc.txt