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

[Laxman Dewangan <ldewangan@nvidia.com>]

Thanks Lee for your review. I will take care of most of comment on my=20
next patch.

I have reply on some comment and seeking more details for few comments=20
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 whic=
h
>> +  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=20
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 bindi=
ng
>> +  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=20
complexity here. Originally, as3722 is posted by me only. Any good file=20
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=20
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=20
its rail and hence no SW execution on system. Still external supply will=20
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=20
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 prop=
erty.
>> +
>> +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.=20
They can connected to any inputs. That's why fps-enable-input select the=20
related digital input for given FPS.
However, I can reduce the need of "fps-sw-enable" and make this always=20
enable if fps-enable-input=3D <SW>.

Here is excerpt from datasheet:
B3:B1: SRCFPSx[1:0]
     Enable Source. Specifies the enable source for the sequencer.
         0b00=3DEN0 hardware input
         0b01=3DEN1 hardware input
         0b10=3DENFPSx software bit
         0b11=3Dreserved
B0 ENFPSx
     Software Enable
     0=3DDisable FPSx
     1=3DEnable FPSx
     X=3DENFPSx is a don=E2=80=99t care if SRCFPSx[1:0] !=3D 0b10


>
>> +	-maxim,enable-global-lpm: Boolean, enable global LPM when the external
> LPM?
Low Power Mode (LPM). I will add details.

> +Pinmux and GPIO:
> +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> 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=20
differently?
Actually during AS3722, I had different understanding to have single file.


--=20
--=20
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
---=20
You received this message because you are subscribed to the Google Groups "=
rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an e=
mail to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.