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

[Guenter Roeck]

On 10/29/21 5:15 AM, Rafał Miłecki wrote:
> On 28.10.2021 18:57, Guenter Roeck wrote:
>> On 10/28/21 9:29 AM, 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.
>>>>
>> Seems unrelated / irrelevant in this commit log, except maybe after '---'.
>>
>>>> Signed-off-by: Rafał Miłecki <rafal at milecki.pl>
>>>> ---
>>>
>>> [snip]
>>>
>>>> +
>>>> +static const u16 bcm7038_wdt_regs_bcm4908[] = {
>>>> +    [BCM63XX_WDT_REG_DEFVAL]    = 0x28,
>>
>> REG_DEFVAL is an odd name for this register. I can see that the
>> bcm63xx driver uses it, but in reality it seems to be the timeout
>> value, not some default value, only the bcm63xx driver doesn't
>> seem to use it properly. I think REG_TIMEOUT or similar would
>> be a much better name.
> 
> I used name used in Broadcom's SDK (and as I guess also in their
> documentation too).
> 
> Take a look at this BCM60333 example:
> 
> typedef struct Timer {
>      uint32    TimerInts;        /* 0x00 */
>      uint32    TimerCtl0;        /* 0x04 */
>      uint32    TimerCtl1;        /* 0x08 */
>      uint32    TimerCtl2;        /* 0x0c */
>      uint32    TimerCnt0;        /* 0x10 */
>      uint32    TimerCnt1;        /* 0x14 */
>      uint32    TimerCnt2;        /* 0x18 */
>      uint32    WatchDogDefCount;    /* 0x1c */
>      uint32    WatchDogCtl;        /* 0x20 */
>      uint32    WDResetCount;        /* 0x24 */
> } Timer;
> 
> I got impression that Linux driver registers names usually follow what
> is used in hardware documentation.

Still, the key part of the register name is "Count", not "Def",
and there is no "val" in there.

Guenter