Re: [PATCH 1/3] watchdog: at91sam9_wdt.h: Cleanup the header file

[<Balakrishnan.S@microchip.com>]

Hi,

Thanks for the comments.

On 27/02/26 1:22 pm, Alexandre Belloni wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 27/02/2026 13:01:14+0530, Balakrishnan Sambath wrote:
>> From: Andrei Simion <andrei.simion@microchip.com>
>>
>> This patch reorganizes the header file by renaming the registers using
>> a general pattern also this patch simplifies the watchdog disable logic
>> in the at91sam9_wdt.h header by differentiating between modern
>> (SAM9X60, SAMA7G5, SAM9X7) and legacy (SAMA5, AT91SAM9261) chips based
>> on the watchdog disable bit.
>> For modern chips, the disable bit is at bit 12, while for legacy chips
>> it is at bit 15.
>>
>> Signed-off-by: Andrei Simion <andrei.simion@microchip.com>
>> [Remove Kconfig-based WDDIS selection and define explicit legacy and
>> modern masks]
>> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
>> ---
>>   drivers/watchdog/at91sam9_wdt.h | 65 ++++++++++++++++-----------------
>>   1 file changed, 32 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/watchdog/at91sam9_wdt.h b/drivers/watchdog/at91sam9_wdt.h
>> index 298d545df1a1..1e0aeecb489f 100644
>> --- a/drivers/watchdog/at91sam9_wdt.h
>> +++ b/drivers/watchdog/at91sam9_wdt.h
>> @@ -3,59 +3,58 @@
>>    * drivers/watchdog/at91sam9_wdt.h
>>    *
>>    * Copyright (C) 2007 Andrew Victor
>>    * Copyright (C) 2007 Atmel Corporation.
>>    * Copyright (C) 2019 Microchip Technology Inc. and its subsidiaries
>>    *
>>    * Watchdog Timer (WDT) - System peripherals regsters.
>>    * Based on AT91SAM9261 datasheet revision D.
>>    * Based on SAM9X60 datasheet.
>> + * Based on SAMA7G5 datasheet.
>> + * Based on SAM9X75 datasheet.
>>    *
>>    */
>>
>>   #ifndef AT91_WDT_H
>>   #define AT91_WDT_H
>>
>>   #include <linux/bits.h>
>>
>> -#define AT91_WDT_CR          0x00                    /* Watchdog Control Register */
>> -#define  AT91_WDT_WDRSTT     BIT(0)                  /* Restart */
>> -#define  AT91_WDT_KEY                (0xa5UL << 24)          /* KEY Password */
>> -
>> -#define AT91_WDT_MR          0x04                    /* Watchdog Mode Register */
>> -#define  AT91_WDT_WDV                (0xfffUL << 0)          /* Counter Value */
>> -#define  AT91_WDT_SET_WDV(x) ((x) & AT91_WDT_WDV)
>> -#define  AT91_SAM9X60_PERIODRST      BIT(4)          /* Period Reset */
>> -#define  AT91_SAM9X60_RPTHRST        BIT(5)          /* Minimum Restart Period */
>> +#define AT91_WDT_CR          0x00            /* Watchdog Control Register */
>> +#define  AT91_WDT_WDRSTT     BIT(0)          /* Restart */
>> +#define  AT91_WDT_KEY                (0xa5UL << 24)  /* KEY Password */
>> +#define AT91_WDT_MR          0x04            /* Watchdog Mode Register */
>> +#define  AT91_WDT_WDV                (0xfffUL << 0)  /* Counter Value */
>>   #define  AT91_WDT_WDFIEN     BIT(12)         /* Fault Interrupt Enable */
>> -#define  AT91_SAM9X60_WDDIS  BIT(12)         /* Watchdog Disable */
>> -#define  AT91_WDT_WDRSTEN    BIT(13)         /* Reset Processor */
>> -#define  AT91_WDT_WDRPROC    BIT(14)         /* Timer Restart */
>> -#define  AT91_WDT_WDDIS              BIT(15)         /* Watchdog Disable */
>> -#define  AT91_WDT_WDD                (0xfffUL << 16)         /* Delta Value */
>> -#define  AT91_WDT_SET_WDD(x) (((x) << 16) & AT91_WDT_WDD)
>> -#define  AT91_WDT_WDDBGHLT   BIT(28)         /* Debug Halt */
>> -#define  AT91_WDT_WDIDLEHLT  BIT(29)         /* Idle Halt */
>> -
>> +#define  AT91_WDT_WDRSTEN    BIT(13)
>> +#define  AT91_WDT_WDRPROC    BIT(14)
>> +#define  AT91_WDT_WDD                (0xfffUL << 16)
>> +#define  AT91_WDT_WDDBGHLT   BIT(28)
>> +#define  AT91_WDT_WDIDLEHLT  BIT(29)
>>   #define AT91_WDT_SR          0x08            /* Watchdog Status Register */
>>   #define  AT91_WDT_WDUNF              BIT(0)          /* Watchdog Underflow */
>>   #define  AT91_WDT_WDERR              BIT(1)          /* Watchdog Error */
>>
>> -/* Watchdog Timer Value Register */
>> -#define AT91_SAM9X60_VR              0x08
>> +#define  AT91_WDT_SET_WDV(x) ((x) & AT91_WDT_WDV)
>> +#define  AT91_WDT_SET_WDD(x) (((x) << 16) & AT91_WDT_WDD)
>>
>> -/* Watchdog Window Level Register */
>> -#define AT91_SAM9X60_WLR     0x0c
>> -/* Watchdog Period Value */
>> -#define  AT91_SAM9X60_COUNTER        (0xfffUL << 0)
>> -#define  AT91_SAM9X60_SET_COUNTER(x) ((x) & AT91_SAM9X60_COUNTER)
>> +#define AT91_WDT_VR          0x08    /* Watchdog Timer Value Register */
>> +#define AT91_WDT_ISR         0x1c    /* Interrupt Status Register */
>> +#define AT91_WDT_IER         0x14    /* Interrupt Enable Register */
>> +#define AT91_WDT_IDR         0x18    /* Interrupt Disable Register */
>> +#define AT91_WDT_WLR         0x0c    /* Watchdog Window Level Register */
>> +#define AT91_WDT_PERIODRST   BIT(4)  /* Period Reset */
>> +#define AT91_WDT_RPTHRST     BIT(5)          /* Minimum Restart Period */
>> +#define  AT91_WDT_PERINT     BIT(0)  /* Period Interrupt Enable */
>> +#define  AT91_WDT_COUNTER    (0xfffUL << 0)  /* Watchdog Period Value */
>> +#define  AT91_WDT_SET_COUNTER(x)     ((x) & AT91_WDT_COUNTER)
>>
>> -/* Interrupt Enable Register */
>> -#define AT91_SAM9X60_IER     0x14
>> -/* Period Interrupt Enable */
>> -#define  AT91_SAM9X60_PERINT BIT(0)
>> -/* Interrupt Disable Register */
>> -#define AT91_SAM9X60_IDR     0x18
>> -/* Interrupt Status Register */
>> -#define AT91_SAM9X60_ISR     0x1c
>> +/*
>> + * WDDIS bit differs by SoC:
>> + *   - SAMA5, AT91SAM9261: bit 15
>> + *   - SAM9X60, SAMA7G5, SAM9X75: bit 12
>> + * Select at runtime via compatible string.
>> + */
>> +#define AT91_WDT_WDDIS_LEGACY   BIT(15)
>> +#define AT91_WDT_WDDIS_MODERN   BIT(12)
> 
> This is bad naming, we are going to end up with
> AT91_WDT_WDDIS_LEGACY_LEGACY, AT91_WDT_WDDIS_MODERN_LEGACY and
> AT91_WDT_WDDIS_MODERN next time. The proper name would use the name of
> the SoC introducing the new position.

Okay got it. If that's the case I'll retain the soC specific namings 
everywhere in v2.

> 
>>
>>   #endif
>> --
>> 2.34.1
>>
> 
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com