| Message ID | 1418834727-1602-6-git-send-email-lee.jones@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Wednesday 17 December 2014 16:45:24 Lee Jones wrote: > +- compatible : Must be one of: "st,stih407-lpc" "st,stih416-lpc" > + "st,stih415-lpc" "st,stid127-lpc" > +- reg : LPC registers base address + size > +- interrupts : LPC interrupt line number and associated flags > +- clocks : Clock used by LPC device (See: ../clock/clock-bindings.txt) > +- st,lpc-mode : The LPC can run either one of two modes ST_LPC_MODE_RTC [0] or > + ST_LPC_MODE_WDT [1]. One (and only one) mode must be > + selected. > I'm glad you got it to work with two drivers for the same device. With this binding, I'm still a bit unhappy about the st,lpc-mode property, in particular since you rely on a shared include file for something that can only be set in one way or another and always has to be present. Why not just use a boolean property that enforces one mode when present and another mode when absent? Arnd
On Wed, 17 Dec 2014, Arnd Bergmann wrote: > On Wednesday 17 December 2014 16:45:24 Lee Jones wrote: > > +- compatible : Must be one of: "st,stih407-lpc" "st,stih416-lpc" > > + "st,stih415-lpc" "st,stid127-lpc" > > +- reg : LPC registers base address + size > > +- interrupts : LPC interrupt line number and associated flags > > +- clocks : Clock used by LPC device (See: ../clock/clock-bindings.txt) > > +- st,lpc-mode : The LPC can run either one of two modes ST_LPC_MODE_RTC [0] or > > + ST_LPC_MODE_WDT [1]. One (and only one) mode must be > > + selected. > > > > I'm glad you got it to work with two drivers for the same device. > > With this binding, I'm still a bit unhappy about the st,lpc-mode property, > in particular since you rely on a shared include file for something that > can only be set in one way or another and always has to be present. > > Why not just use a boolean property that enforces one mode when present > and another mode when absent? There is nothing stopping me from doing that, and it was a consideration. I concluded that this method would be more explicit however. Both when describing our choices in DT and at a functional level within each of the drivers. Let me know if you fundamentally disagree and I can fix-up.
On Thursday 18 December 2014 08:13:34 Lee Jones wrote: > On Wed, 17 Dec 2014, Arnd Bergmann wrote: > > > On Wednesday 17 December 2014 16:45:24 Lee Jones wrote: > > > +- compatible : Must be one of: "st,stih407-lpc" "st,stih416-lpc" > > > + "st,stih415-lpc" "st,stid127-lpc" > > > +- reg : LPC registers base address + size > > > +- interrupts : LPC interrupt line number and associated flags > > > +- clocks : Clock used by LPC device (See: ../clock/clock-bindings.txt) > > > +- st,lpc-mode : The LPC can run either one of two modes ST_LPC_MODE_RTC [0] or > > > + ST_LPC_MODE_WDT [1]. One (and only one) mode must be > > > + selected. > > > > > > > I'm glad you got it to work with two drivers for the same device. > > > > With this binding, I'm still a bit unhappy about the st,lpc-mode property, > > in particular since you rely on a shared include file for something that > > can only be set in one way or another and always has to be present. > > > > Why not just use a boolean property that enforces one mode when present > > and another mode when absent? > > There is nothing stopping me from doing that, and it was a > consideration. I concluded that this method would be more explicit > however. Both when describing our choices in DT and at a functional > level within each of the drivers. > > Let me know if you fundamentally disagree and I can fix-up. I generally don't like header files that define interfaces between C code and DT nodes. There are cases where it's the least ugly solution, but I don't think this is one of them. If you want to be more explicit about the modes, how about having one boolean property per mode? That would also allow devices that could be driven in either mode, e.g. if you have only one instance of this device. Arnd
We On Thu, 18 Dec 2014, Arnd Bergmann wrote: > On Thursday 18 December 2014 08:13:34 Lee Jones wrote: > > On Wed, 17 Dec 2014, Arnd Bergmann wrote: > > > > > On Wednesday 17 December 2014 16:45:24 Lee Jones wrote: > > > > +- compatible : Must be one of: "st,stih407-lpc" "st,stih416-lpc" > > > > + "st,stih415-lpc" "st,stid127-lpc" > > > > +- reg : LPC registers base address + size > > > > +- interrupts : LPC interrupt line number and associated flags > > > > +- clocks : Clock used by LPC device (See: ../clock/clock-bindings.txt) > > > > +- st,lpc-mode : The LPC can run either one of two modes ST_LPC_MODE_RTC [0] or > > > > + ST_LPC_MODE_WDT [1]. One (and only one) mode must be > > > > + selected. > > > > > > > > > > I'm glad you got it to work with two drivers for the same device. > > > > > > With this binding, I'm still a bit unhappy about the st,lpc-mode property, > > > in particular since you rely on a shared include file for something that > > > can only be set in one way or another and always has to be present. > > > > > > Why not just use a boolean property that enforces one mode when present > > > and another mode when absent? > > > > There is nothing stopping me from doing that, and it was a > > consideration. I concluded that this method would be more explicit > > however. Both when describing our choices in DT and at a functional > > level within each of the drivers. > > > > Let me know if you fundamentally disagree and I can fix-up. > > I generally don't like header files that define interfaces between C code > and DT nodes. There are cases where it's the least ugly solution, but I don't > think this is one of them. > > If you want to be more explicit about the modes, how about having one > boolean property per mode? That would also allow devices that could be > driven in either mode, e.g. if you have only one instance of this device. Isn't this was you suggested above? Making a decision on the absence is a property is what I'm calling not-explicit. If it's accidentally left off the driver(s) won't issue a warning, it'll just assume that the lack of this boolean property was intentional and go follow the Watchdog path for instance. But as I briefly mentioned to you elsewhere, there are actually 3 devices (Watchdog, RTC and Global Timer). How would you like to handle that with a Boolean property when we introduce this new driver?
On Thursday 18 December 2014 09:04:04 Lee Jones wrote: > We > On Thu, 18 Dec 2014, Arnd Bergmann wrote: > > > On Thursday 18 December 2014 08:13:34 Lee Jones wrote: > > > On Wed, 17 Dec 2014, Arnd Bergmann wrote: > > > > > > > On Wednesday 17 December 2014 16:45:24 Lee Jones wrote: > > > > > +- compatible : Must be one of: "st,stih407-lpc" "st,stih416-lpc" > > > > > + "st,stih415-lpc" "st,stid127-lpc" > > > > > +- reg : LPC registers base address + size > > > > > +- interrupts : LPC interrupt line number and associated flags > > > > > +- clocks : Clock used by LPC device (See: ../clock/clock-bindings.txt) > > > > > +- st,lpc-mode : The LPC can run either one of two modes ST_LPC_MODE_RTC [0] or > > > > > + ST_LPC_MODE_WDT [1]. One (and only one) mode must be > > > > > + selected. > > > > > > > > > > > > > I'm glad you got it to work with two drivers for the same device. > > > > > > > > With this binding, I'm still a bit unhappy about the st,lpc-mode property, > > > > in particular since you rely on a shared include file for something that > > > > can only be set in one way or another and always has to be present. > > > > > > > > Why not just use a boolean property that enforces one mode when present > > > > and another mode when absent? > > > > > > There is nothing stopping me from doing that, and it was a > > > consideration. I concluded that this method would be more explicit > > > however. Both when describing our choices in DT and at a functional > > > level within each of the drivers. > > > > > > Let me know if you fundamentally disagree and I can fix-up. > > > > I generally don't like header files that define interfaces between C code > > and DT nodes. There are cases where it's the least ugly solution, but I don't > > think this is one of them. > > > > If you want to be more explicit about the modes, how about having one > > boolean property per mode? That would also allow devices that could be > > driven in either mode, e.g. if you have only one instance of this device. > > Isn't this was you suggested above? My first suggestion was to just have one boolean property, and use one driver if that is absent. The second one was to have two (or three) separate boolean properties that each refer to whether a particular driver is allowed to use this device or not. > But as I briefly mentioned to you elsewhere, there are actually 3 > devices (Watchdog, RTC and Global Timer). How would you like to > handle that with a Boolean property when we introduce this new driver? Right, this would require having more than one property, but I still think it's better than the header file. Arnd
On Thu, 18 Dec 2014, Arnd Bergmann wrote: > On Thursday 18 December 2014 09:04:04 Lee Jones wrote: > > We > > On Thu, 18 Dec 2014, Arnd Bergmann wrote: > > > > > On Thursday 18 December 2014 08:13:34 Lee Jones wrote: > > > > On Wed, 17 Dec 2014, Arnd Bergmann wrote: > > > > > > > > > On Wednesday 17 December 2014 16:45:24 Lee Jones wrote: > > > > > > +- compatible : Must be one of: "st,stih407-lpc" "st,stih416-lpc" > > > > > > + "st,stih415-lpc" "st,stid127-lpc" > > > > > > +- reg : LPC registers base address + size > > > > > > +- interrupts : LPC interrupt line number and associated flags > > > > > > +- clocks : Clock used by LPC device (See: ../clock/clock-bindings.txt) > > > > > > +- st,lpc-mode : The LPC can run either one of two modes ST_LPC_MODE_RTC [0] or > > > > > > + ST_LPC_MODE_WDT [1]. One (and only one) mode must be > > > > > > + selected. > > > > > > > > > > > > > > > > I'm glad you got it to work with two drivers for the same device. > > > > > > > > > > With this binding, I'm still a bit unhappy about the st,lpc-mode property, > > > > > in particular since you rely on a shared include file for something that > > > > > can only be set in one way or another and always has to be present. > > > > > > > > > > Why not just use a boolean property that enforces one mode when present > > > > > and another mode when absent? > > > > > > > > There is nothing stopping me from doing that, and it was a > > > > consideration. I concluded that this method would be more explicit > > > > however. Both when describing our choices in DT and at a functional > > > > level within each of the drivers. > > > > > > > > Let me know if you fundamentally disagree and I can fix-up. > > > > > > I generally don't like header files that define interfaces between C code > > > and DT nodes. There are cases where it's the least ugly solution, but I don't > > > think this is one of them. > > > > > > If you want to be more explicit about the modes, how about having one > > > boolean property per mode? That would also allow devices that could be > > > driven in either mode, e.g. if you have only one instance of this device. > > > > Isn't this was you suggested above? > > My first suggestion was to just have one boolean property, and use one > driver if that is absent. The second one was to have two (or three) separate > boolean properties that each refer to whether a particular driver is allowed > to use this device or not. > > > But as I briefly mentioned to you elsewhere, there are actually 3 > > devices (Watchdog, RTC and Global Timer). How would you like to > > handle that with a Boolean property when we introduce this new driver? > > Right, this would require having more than one property, but I still think > it's better than the header file. I'll put my point across just once and then become subservient once more. I don't agree that defining 3 properties is better than creating just 1. We have lots of properties containing indexes and flags. Just because we've decided to #define them in order to read them easily shouldn't detract from the fact that it's a better setup. st,lpc-mode <1|2|3>; Must be better than: st,lpc-globaltimer-mode; st,lpc-watchdog-mode; st,lpc-rtc-mode; If each of the drivers only checks for it's own property and fails to probe if it's not present how will we detect and warn about a lack of any of the 3 properties without a central, all-knowing (MFD) driver? This is likely to cause someone [why isn't my driver probing] issues and subsequently waste valuable engineering time in the future.
diff --git a/Documentation/devicetree/bindings/watchdog/st_lpc_wdt.txt b/Documentation/devicetree/bindings/watchdog/st_lpc_wdt.txt new file mode 100644 index 0000000..1bdf023 --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/st_lpc_wdt.txt @@ -0,0 +1,38 @@ +STMicroelectronics Low Power Controller (LPC) - Watchdog +======================================================== + +LPC currently supports Watchdog OR Real Time Clock functionality. + +[See: ../rtc/rtc-st-lpc.txt for RTC options] + +Required properties + +- compatible : Must be one of: "st,stih407-lpc" "st,stih416-lpc" + "st,stih415-lpc" "st,stid127-lpc" +- reg : LPC registers base address + size +- interrupts : LPC interrupt line number and associated flags +- clocks : Clock used by LPC device (See: ../clock/clock-bindings.txt) +- st,lpc-mode : The LPC can run either one of two modes ST_LPC_MODE_RTC [0] or + ST_LPC_MODE_WDT [1]. One (and only one) mode must be + selected. + +Required properties [watchdog mode] + +- st,syscfg : Phandle to syscfg node used to enable watchdog and configure + CPU reset type. +- timeout-sec : Watchdog timeout in seconds + +Optional properties [watchdog mode] + +- st,warm-reset : If present reset type will be 'warm' - if not it will be cold + +Example: + lpc@fde05000 { + compatible = "st,stih416-lpc-watchdog"; + reg = <0xfde05000 0x1000>; + clocks = <&clk_s_d3_flexgen CLK_LPC_0>; + st,syscfg = <&syscfg_core>; + timeout-sec = <120>; + st,lpc-mode = <ST_LPC_MODE_WDT>; + st,warm-reset; + };
On current ST platforms the LPC controls a number of functions including Watchdog and Real Time Clock. This patch provides the bindings used to configure LPC in Watchdog mode. Signed-off-by: Lee Jones <lee.jones@linaro.org> --- .../devicetree/bindings/watchdog/st_lpc_wdt.txt | 38 ++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/st_lpc_wdt.txt