On Mon, 25 Jan 2016, Laxman Dewangan wrote: > > Thanks Lee for your review. I will take care of most of comment on > my next patch. > > I have reply on some comment and seeking more details for few > comments as follows. > > On Monday 25 January 2016 05:26 PM, Lee Jones wrote: > >On Tue, 19 Jan 2016, Laxman Dewangan wrote: > > > >>+- interrupt-controller: MAX77620 has internal interrupt controller which > >>+ takes the interrupt request from internal sub-blocks like RTC, > >>+ regulators, GPIOs as well as external input. > >This is how interrupt-controllers usually work. I don't think there > >is any need to explain this. I'd just link to > >../interrupt-controller/interrupts.txt instead. > > Something similar to (just to confirm) > - interrupt-controller: describes the 88pm860x as an interrupt > controller (has its own domain) IRQ domains are Linuxisums, which aren't allowed in DT. I'd prefer something along the lines of: "Identifies the node as an interrupt controller." > >>+- #interrupt-cells: Should be set to 2 for IRQ number and flags. > >>+ The first cell is the IRQ number. IRQ numbers for different interrupt > >>+ source of MAX77620 are defined at dt-bindings/mfd/max77620.h > >>+ The second cell is the flags, encoded as the trigger masks from binding > >>+ document interrupts.txt, using dt-bindings/irq. > >This is a very lengthy read for such little information. Please make > >it more succinct. Take a look at other files for examples. > I started with as3722.txt and it seems I am carrying forward the > complexity here. Originally, as3722 is posted by me only. Any good > file example which I can refer? Just grep for #interrupt-cells in Documentation. There are plenty of nice, short examples. > > Also, > >tell use where interrupts.txt is > >i.e. ../interrupt-controller/interrupts.txt. > > > >These are so much easier to read if you tab out from the property name > >to the description. > > > >- reg: I2C device address. > >- interrupt-controller: MAX77620 has internal interrupt controller which > > takes the interrupt request from internal > > sub-blocks like RTC, regulators, GPIOs as well > > as external input. > >- #interrupt-cells: Should be set to 2 for IRQ number and flags. > > The first cell is the IRQ number. IRQ numbers > > for different interrupt source of MAX77620 are > > defined at dt-bindings/mfd/max77620.h > > The second cell is the flags, encoded as the > > trigger masks from binding document > > interrupts.txt, using dt-bindings/irq. > > > >... don't you think? > > Agree, I can make indenting. And will do whatever places it needs in > this document. > > > > >>+Optional properties: > >>+------------------- > >>+This device also supports the power OFF of system. > >What is the "power OFF of system"? > > PMIC supplies the power. So once PMIC is in OFF state, it turns off > all its rail and hence no SW execution on system. Still external > supply will keep supply to PMIC. "This device controls platform power" or something. > >>+Following properties are used for this purpose: > >>+- system-power-controller: Boolean, This device will be use as > >You don't describe the type of each property, so why is this one > >special? > Hmm. I describe the boolean and tristate only. Do I need to define > type for integer,string also? I wouldn't describe any of them. It's normally pretty obvious which properties are boolean by the lack of required cell description. > >>+power OFF slot and slot period of the device. Device has 3 FPS as FPS0, > >>+FPS1 and FPS2. The details of FPS configuration is provided through > >>+subnode "fps". The details of FPS0, FPS1, FPS2 are provided through the > >>+child node under this subnodes. The FPS number is provided via reg property. > >>+ > >>+The property for fps child nodes as: > >>+Required properties: > >>+ -reg: FPS number like 0, 1, 2 for FPS0, FPS1 and FPS2 respectively. > >I'm surprised Rob Acked this. We don't usually do device numbers in DT. > What is best way to make the child node for FPS and differentiate FPS0,1, 2? > What is your suggestion here? There are lots of ways you can solve this and so many examples of others doing so. I suggest you have a look at some DTS files and figure it out. One possible solution is to use different compatible strings. > >>+ -maxim,active-fps-time-period-us: Active state FPS time period in > >>+ microseconds. > >>+ -maxim,suspend-fps-time-period-us: Suspend state FPS time period in > >>+ microseconds. > >>+ -maxim,fps-enable-input: FPS enable source like EN0, EN1 or SW. The > >>+ macros are defined on dt-bindings/mfd/max77620.h for > >>+ different enable source. > >>+ FPS_EN_SRC_EN0 for EN0 enable source. > >>+ FPS_EN_SRC_EN1 for En1 enable source. > >>+ FPS_EN_SRC_SW for SW based control. > >>+ -maxim,fps-sw-enable: Boolean, applicable if enable input is SW. > >>+ If this property present then enable the FPS else > >"property is present" > > > >If this enables/disables FPS, why does it matter if it's SW or not? > >Why can't you just cal it maxim,fps-enable? Also, is there a case > >where you would supply this sub-node, have FPS enabled and this > >property not present? If not, can't you just remove the entire node? > >Or am I missing something? > > Here, my need is to connect different FPS source inputs EN0, EN1 or > SW. They can connected to any inputs. That's why fps-enable-input > select the related digital input for given FPS. > However, I can reduce the need of "fps-sw-enable" and make this > always enable if fps-enable-input= <SW>. This is exactly how you need to think. Reducing the number of properties you introduce (and this device adds a lot more than I would normally expect by the way) needs to be top priority. > Here is excerpt from datasheet: > B3:B1: SRCFPSx[1:0] > Enable Source. Specifies the enable source for the sequencer. > 0b00=EN0 hardware input > 0b01=EN1 hardware input > 0b10=ENFPSx software bit > 0b11=reserved > B0 ENFPSx > Software Enable > 0=Disable FPSx > 1=Enable FPSx > X=ENFPSx is a don’t care if SRCFPSx[1:0] != 0b10 > > > > > >>+ -maxim,enable-global-lpm: Boolean, enable global LPM when the external > >LPM? > Low Power Mode (LPM). I will add details. > > >+Pinmux and GPIO: > >+=============== > >I think this whole section needs moving to ../pinctrl and needs to be > >reviewed by Linus W. > > Is this mean I need to create DT binding doc for the each subsystem > differently? > Actually during AS3722, I had different understanding to have single file. Yes, that way you have each of the the subsystem experts review your documentation. You can then link to them from this document. -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
