Re: [PATCH V4 1/5] DT: mfd: add device-tree binding doc fro PMIC max77620/max20024

[Laxman Dewangan]

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.