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)
+- #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?
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.
+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?
+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?
+ -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>.
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.
--
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
