[PATCH 3/3] watchdog: bcm7038_wdt: support BCM4908 SoC

[Rob Herring]

On Fri, Oct 29, 2021 at 01:39:02PM +0200, Rafał Miłecki wrote:
> [Rob: please kindly comment on this]
> 
> On 28.10.2021 18:29, Florian Fainelli wrote:
> > On 10/28/21 2:30 AM, Rafał Miłecki wrote:
> > > From: Rafał Miłecki <rafal at milecki.pl>
> > > 
> > > Hardware supported by this driver goes back to the old bcm63xx days. It
> > > was then reused in BCM7038 and later also in BCM4908.
> > > 
> > > Depending on SoC model registers layout differs a bit. This commit
> > > introduces support for per-chipset registers offsets & adds BCM4908
> > > layout.
> > > 
> > > Later on BCM63xx SoCs support should be added too (probably as platform
> > > devices due to missing DT). Eventually this driver should replace
> > > bcm63xx_wdt.c.
> > > 
> > > Signed-off-by: Rafał Miłecki <rafal at milecki.pl>
> > > ---
> > 
> > [snip]
> > 
> > > +
> > > +static const u16 bcm7038_wdt_regs_bcm4908[] = {
> > > +	[BCM63XX_WDT_REG_DEFVAL]	= 0x28,
> > > +	[BCM63XX_WDT_REG_CTL]		= 0x2c,
> > > +	[BCM63XX_WDT_REG_SOFTRESET]	= 0x34,
> > 
> > I don't understand what you are doing here and why you are not
> > offsetting the "reg" property appropriately when you create your
> > bcm4908-wdt Device Tree node such that the base starts at 0, and the
> > existing driver becomes usable as-is. This does not make any sense to me
> > when it is obviously the simplest way to make the driver "accept" the
> > resource being passed.
> 
> I believe that DT binding should cover the whole hardware block and
> describe it (here: use proper compatible to allow recognizing block
> variant).
> 
> That's because (as far as I understand) DT should be used to describe
> hardware as closely as possible. I think it shouldn't be adjusted to
> make mapping match Linux's driver implementation.
> 
> 
> So if:
> 1. Hardware block is mapped at 0xff800400
> 2. It has interesting registers at 0xff800428 and 0xff80042c
> 
> I think mapping should use:
> reg = <0xff800400 0x3c>;
> even if we don't use the first N registers.
> 
> That way, at some point, you can extend Linux (or whatever) driver to
> use extra registers without reworking the whole binding. That's why I
> think we need to map whole hardware block & handle different registers
> layouts in a driver.

Yes, that's the correct thing to do.

The question is whether you'd need sub nodes for the other functions. 
Folks tend to want to have sub nodes for convenience which isn't really 
needed and then requires a DT update ('cause they add nodes as adding 
drivers).

Based on the registers, you really don't need sub nodes here.

Rob