Re: [PATCH v4 4/8] dt-bindings: Add doc for the Ingenic TCU drivers

[Paul Cercueil]

Hi Rob,

Le mer. 28 mars 2018 à 18:28, Rob Herring <robh@xxxxxxxxxx> a écrit :
> On Wed, Mar 28, 2018 at 10:33 AM, Paul Cercueil 
> <paul@xxxxxxxxxxxxxxx> wrote:
> >  Le 2018-03-27 16:46, Rob Herring a écrit :
> > >  On Sun, Mar 18, 2018 at 12:28:57AM +0100, Paul Cercueil wrote:
> > > >  Add documentation about how to properly use the Ingenic TCU
> > > >  (Timer/Counter Unit) drivers from devicetree.
> > > >
> > > >  Signed-off-by: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> > > >  ---
> > > >   .../bindings/clock/ingenic,tcu-clocks.txt          | 42 
> > > > ++++++++++++++++
> > > >   .../bindings/interrupt-controller/ingenic,tcu.txt  | 39 
> > > > +++++++++++++++
> > > >   .../devicetree/bindings/mfd/ingenic,tcu.txt        | 56
> > > >  ++++++++++++++++++++++
> > > >   .../devicetree/bindings/timer/ingenic,tcu.txt      | 41 
> > > > ++++++++++++++++
> > > >   4 files changed, 178 insertions(+)
> > > >   create mode 100644
> > > >  Documentation/devicetree/bindings/clock/ingenic,tcu-clocks.txt
> > > >   create mode 100644
> > > >  
> > > > Documentation/devicetree/bindings/interrupt-controller/ingenic,tcu.txt
> > > >   create mode 100644 
> > > > Documentation/devicetree/bindings/mfd/ingenic,tcu.txt
> > > >   create mode 100644
> > > >  Documentation/devicetree/bindings/timer/ingenic,tcu.txt
> > > >
> > > >   v4: New patch in this series. Corresponds to V2 patches 3-4-5 
> > > > with
> > > >   added content.
> > > >  +/ {
> > > >  +       tcu: mfd@10002000 {
> > > >  +               compatible = "ingenic,tcu", "simple-mfd", 
> > > > "syscon";
> > > >  +               reg = <0x10002000 0x1000>;
> > > >  +               #address-cells = <1>;
> > > >  +               #size-cells = <1>;
> > > >  +               ranges = <0x0 0x10002000 0x1000>;
> > > >  +
> > > >  +               tcu_timer: timer@10 {
> > > >  +                       compatible = "ingenic,jz4740-tcu";
> > > >  +                       reg = <0x10 0xff0>;
> > > >  +
> > > >  +                       clocks = <&tcu_clk 0>, <&tcu_clk 1>, 
> > > > <&tcu_clk
> > > >  2>, <&tcu_clk 3>,
> > > >  +                                        <&tcu_clk 4>, <&tcu_clk 
> > > > 5>,
> > > >  <&tcu_clk 6>, <&tcu_clk 7>;
> > > >  +                       clock-names = "timer0", "timer1", 
> > > > "timer2",
> > > >  "timer3",
> > > >  +                                                 "timer4", 
> > > > "timer5",
> > > >  "timer6", "timer7";
> > > >  +
> > > >  +                       interrupt-parent = <&tcu_irq>;
> > > >  +                       interrupts = <0 1 2 3 4 5 6 7>;
> > >
> > >
> > >  Thinking about this some more... You simply have 8 timers (and no 
> > > other
> > >  functions?) with some internal clock and irq controls for each 
> > > timer. I
> > >  don't think it really makes sense to create separate clock and irq
> > >  drivers in that case. That would be like creating clock drivers for
> > >  every clock divider in timers, pwms, uarts, etc. Unless the clocks 
> > > get
> > >  exposed to other parts of the system, then there is no point.
> >
> >
> >  I have 8 timers with some internal clock and IRQ controls, that can 
> > be used
> >  as PWM.
>
> Please include how you plan to do the PWM support too. I need a
> complete picture of the h/w, not piecemeal, evolving bindings.

Alright.

> >  But the TCU also controls the IRQ of the OS Timer (which is
> >  separate),
> >  as well as masking of the clocks for the OS timer and the watchdog.
>
> The OS timer and watchdog are different blocks outside the TCU? This
> doesn't seem to be the case based on the register definitions.

Their register areas are mostly separate, although contiguous. On the 
other
hand, the watchdog and OST can be started/stopped from a bit within a 
TCU
register, so they're probably part of the same h/w block.

> >  I thought having clean drivers for different frameworks working on 
> > the same
> >  regmap was cleaner than having one big-ass driver handling 
> > everything.
>
> DT is not the only way to instantiate drivers and how one OS splits
> drivers should not define your DT binding. An MFD driver can create
> child devices and a single DT node can be a provider of multiple
> things. It is appropriate for an MFD to have child nodes primarily
> when the sub devices need their own resources defined as properties in
> DT or when the sub device is an IP block reused in multiple devices.
> Just to have a node per driver/provider is not what should drive the
> decision.

The idea is not to have necesarily one node per driver. I just wanted 
to keep
it simple.

Regards,
-Paul