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

["Rafał Miłecki" <zajec5@gmail.com>]

On 29.10.2021 16:15, Guenter Roeck wrote:
> 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@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@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.

Absolutely right. No idea where did I take it from and how did I miss that.