RE: [PATCH v1 1/2] dt-bindings: timer: Add bindings for Intel Keem Bay SoC timer

["Ayyathurai, Vijayakannan" <vijayakannan.ayyathurai@xxxxxxxxx>]

Hi Rob,

> From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> On Tue, Dec 08, 2020 at 10:12:47AM -0600, Rob Herring wrote:
> > > From: Vijayakannan Ayyathurai <vijayakannan.ayyathurai@xxxxxxxxx>
> > >
> > > Add Device Tree bindings for the Timer IP, which used as clocksource and
> > > clockevent in the Intel Keem Bay SoC.
> 
> ...
> 
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > +    #include <dt-bindings/interrupt-controller/irq.h>
> > > +    #define KEEM_BAY_A53_TIM
> > > +
> > > +    timer@20330010 {
> > > +        compatible = "intel,keembay-timer";
> > > +        reg = <0x20330010 0xc>,
> > > +              <0x20331000 0xc>;
> > > +        interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
> > > +        clocks = <&scmi_clk KEEM_BAY_A53_TIM>;
> > > +    };
> > > +
> > > +    counter@203300e8 {
> > > +        compatible = "intel,keembay-counter";
> > > +        reg = <0x203300e8 0xc>,
> > > +              <0x20331000 0xc>;
> >
> > You have overlapping reg regions here. Don't do that. Define the DT
> > in terms of the h/w, not how you want to split things for Linux.
> >
> > It looks like a single h/w block providing multiple functions.
> 
> Actually a good catch.
> 
> Perhaps it needs to have a parent device that provides three resources (one
> common and one per each of two functions) and in the driver it should
> consume
> them accordingly. Though I'm not an expert in DT, does it sound correct from
> your perspective?
> 

May I know your feedback for the below way, which Andy suggested.
I will adapt the driver based on this in the next version.

timer@20331000 {
    compatible = "intel,keembay-timer";
    reg = <20331000 0xc>;
    
    gpt@20330010 {
        compatible = "intel,keembay-gpt";
        reg = <20330010 0xc>;
        interrupts = <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>;
        clocks = <&scmi_clk KEEM_BAY_A53_TIM>;
    };

    counter@203300e8 {
        compatible = "intel,keembay-counter";
        reg = <203300e8 0xc>;
        clocks = <&scmi_clk KEEM_BAY_A53_TIM>;
    };
};

Thanks,
Vijay