RE: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

[Hawkins, Nick]

-----Original Message-----
From: Arnd Bergmann [mailto:arnd@xxxxxxxx] 
Sent: Monday, April 4, 2022 5:02 PM
To: Hawkins, Nick <nick.hawkins@xxxxxxx>>
Cc: Arnd Bergmann <arnd@xxxxxxxx>>; Verdun, Jean-Marie <verdun@xxxxxxx>>; Olof Johansson <olof@xxxxxxxxx>>; soc@xxxxxxxxxx; Rob Herring <robh+dt@xxxxxxxxxx>>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
Subject: Re: [PATCH v3 09/10] arch: arm: boot: dts: Introduce HPE GXP Device tree

On Mon, Apr 4, 2022 at 10:22 PM Hawkins, Nick <nick.hawkins@xxxxxxx>> wrote:
>>>> I would put it into drivers/clocksource/, I don't think drivers/mtd would be any better, but there is a chance that the clocksource maintainers don't want to have the watchdog code in their tree.
>>
>> While trying to discover how to creating two devices in one driver I ran across an interesting .dtsi and I was wondering if this would be a valid approach for my situation as well. The pertinent files are:
>> 1) drivers/clocksource/timer-digicolor.c
>> 2) arch/arm/boot/dts/cx92755.dtsi
>> 3) drivers/watchdog/digicolor_wdt.c
>>
>> Here they are just sharing the same register area:
>>
>> timer@f0000fc0 {
>>         compatible = "cnxt,cx92755-timer";
>>         reg = <0xf0000fc0 0x40>>;
>>         interrupts = <19>>, <31>>, <34>>, <35>>, <52>>, <53>>, <54>>, <55>>;
>>         clocks = <&main_clk>>;
>> };
>>
>> rtc@f0000c30 {
>>         compatible = "cnxt,cx92755-rtc";
>>         reg = <0xf0000c30 0x18>>;
>>         interrupts = <25>>;
>> };
>>
>> watchdog@f0000fc0 {
>>         compatible = "cnxt,cx92755-wdt";
>>         reg = <0xf0000fc0 0x8>>;
>>         clocks = <&main_clk>>;
>>         timeout-sec = <15>>;
>> };

> Right, it is possible to make this work, but it's not recommended, and you have to work around the sanity checks in the code that try to keep you from doing it wrong, as well as any tooling that tries to check for these in the DT.

I found an example in the kernel where the timer creates a child watchdog device and passes it the base address when creating it. I used this to model the gxp-timer and gxp-wdt. The following files were what I have referenced:
drivers/watchdog/ixp4xx_wdt.c
drivers/clocksource/timer-ixp4xx.c

This seems very similar to what you suggested previously except I do not see a private interface in there between the parent and the child device. Is it mandatory to have the private interface between the two? If it is, what would you recommend that interface be? So far without the private interface I am not seeing any issues accessing the registers.

Thanks,

-Nick